[PATCH v2 04/20] rockchip: pine64: pinebook-pro: migrate to rockchip_early_misc_init_r

[Quentin Schulz]

Hi Peter,

On 2/12/24 14:41, Peter Robinson wrote:
> On Fri, 9 Feb 2024 at 09:50, Quentin Schulz <foss+uboot at 0leil.net> wrote:
>>
>> From: Quentin Schulz <quentin.schulz at theobroma-systems.com>
>>
>> Compared to the original misc_init_r from Rockchip mach code,
>> setup_iodomain() is added and rockchip_setup_macaddr() is not called.
>>
>> It is assumed adding rockchip_setup_macaddr() back is fine.
>> Let's use rockchip_early_misc_init_r instead of reimplementing the whole
>> misc_init_r from Rockchip (the side effect being that
>> rockchip_setup_macaddr() is back).
> 
> The Pinebook Pro doesn't have onboard ethernet, nor an alias defined,
> I wonder if rockchip_setup_macaddr should be gated on GMAC_ROCKCHIP or
> something similar. Otherwise:

This was discussed here:

https://lore.kernel.org/u-boot/0d74e5e5482cd32d7cad714e731d4fb8@manjaro.org/ 
is the first message in the discussion, which raised the same question.

In short:
- gating on GMAC_ROCKCHIP is not wise,
- onboard Ethernet or not doesn't matter, it provides a sane mac address 
for any Ethernet device by default, based on the CPU ID.

Quoting Jonas:

"""
  > diff --git a/arch/arm/mach-rockchip/misc.c
  > b/arch/arm/mach-rockchip/misc.c
  > index 7d03f0c2b679..ed5bdab5a648 100644
  > --- a/arch/arm/mach-rockchip/misc.c
  > +++ b/arch/arm/mach-rockchip/misc.c
  > @@ -23,7 +23,8 @@
  >
  >   int rockchip_setup_macaddr(void)
  >   {
  > -#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256)
  > +#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256) && \
  > +    CONFIG_IS_ENABLED(GMAC_ROCKCHIP)

This would exclude any board not enabling GMAC_ROCKCHIP in U-Boot but
want a mac-address being set in DT fixup. And for newer RK35xx SoCs the
GMAC_ROCKCHIP driver is not used.

A new Kconfig option should be introduced if there is a need for some
boards to disable this.
"""

What is the actual concern of yours for pinebook-pro and this function 
now running? I think what ultimately would result from this is at worst 
the environment "polluted" with ethaddrX env variable?

Cheers,
Quentin