Re: [PATCH v2 06/10] arm: mach-k3: am625_init: Probe AM65 CPSW NUSS

[Roger Quadros <rogerq@kernel.org>]

On 25/04/2024 15:59, Chintan Vankar wrote:
> 
> 
> On 25/04/24 17:57, Roger Quadros wrote:
>>
>>
>> On 25/04/2024 15:08, Chintan Vankar wrote:
>>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>>
>>> In order to support Ethernet boot on AM62x, probe AM65 CPSW NUSS
>>> driver in board_init_f().
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
>>> ---
>>>
>>> Link to v1:
>>> https://lore.kernel.org/r/20240112064759.1801600-8-s-vadapalli@ti.com/
>>>
>>> Changes from v1 to v2:
>>> - No changes.
>>>
>>>   arch/arm/mach-k3/am625_init.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-k3/am625_init.c b/arch/arm/mach-k3/am625_init.c
>>> index 668f9a51ef..9bf61b1e83 100644
>>> --- a/arch/arm/mach-k3/am625_init.c
>>> +++ b/arch/arm/mach-k3/am625_init.c
>>> @@ -277,6 +277,16 @@ void board_init_f(ulong dummy)
>>>           if (ret)
>>>               panic("DRAM init failed: %d\n", ret);
>>>       }
>>> +
>>> +    if (IS_ENABLED(CONFIG_SPL_ETH) && IS_ENABLED(CONFIG_TI_AM65_CPSW_NUSS) &&
>>> +        spl_boot_device() == BOOT_DEVICE_ETHERNET) {
>>> +        struct udevice *cpswdev;
>>> +
>>> +        if (uclass_get_device_by_driver(UCLASS_MISC, DM_DRIVER_GET(am65_cpsw_nuss),
>>> +                        &cpswdev))
>>> +            printf("Failed to probe am65_cpsw_nuss driver\n");
>>> +    }
>>> +
>>
>> This looks like a HACK.
>> The network driver should be probed only when the networking feature is actually required.
>>
> 
> Driver is probed only when the condition above
> "spl_boot_device() == BOOT_DEVICE_ETHERNET" is true, which says Boot
> device is Ethernet, and here we are booting with Ethernet so driver is
> getting probed.

I think you misunderstood what I was saying as am625_init.c is not using
any Ethernet function it should not cause the Ethernet driver to probe.

Instead it should be probed by the networking use case.

What happens if you don't use this patch? Where is it failing?

> 
>>>       spl_enable_cache();
>>>         fixup_a53_cpu_freq_by_speed_grade();
>>

-- 
cheers,
-roger