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

[Roger Quadros <rogerq@kernel.org>]

On 21/05/2024 08:34, Chintan Vankar wrote:
> 
> 
> On 20/05/24 17:42, Roger Quadros wrote:
>>
>>
>> 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?
>>
> 
> You are correct that it should be probed by the networking use case and
> we are using networking functionality of "Booting via Ethernet".
> Regarding its probing, I already had discussion that why do I have to
> probe it explicitly at here:
> https://lore.kernel.org/r/ee1d16fd-b99b-4b07-97bb-a896e179157a@ti.com/
> 
> Now if I don't use this patch then Ethernet will not get initialized at
> SPL stage, and in "spl_net_load_image()" function, there will be an
> error saying "No Ethernet Found", and since we selected booting via
> Ethernet it will fail to boot.

OK Now I see the problem.

in am65-cpsw-nuss.c the following is defined:

> U_BOOT_DRIVER(am65_cpsw_nuss) = {
>         .name   = "am65_cpsw_nuss",
>         .id     = UCLASS_MISC,
>         .of_match = am65_cpsw_nuss_ids,
>         .probe  = am65_cpsw_probe_nuss,
>         .priv_auto = sizeof(struct am65_cpsw_common),
> };
> 
> U_BOOT_DRIVER(am65_cpsw_nuss_port) = {
>         .name   = "am65_cpsw_nuss_port",
>         .id     = UCLASS_ETH,
>         .probe  = am65_cpsw_port_probe,
>         .ops    = &am65_cpsw_ops,
>         .priv_auto      = sizeof(struct am65_cpsw_priv),
>         .plat_auto      = sizeof(struct eth_pdata),
>         .flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE,
> };

Now I suppose spl_net_load_image() tries to probe all UCLASS_ETH devices
which should call am65_cpsw_port_probe() but it fails to probe am65_cpsw_probe_nuss().

This limitation was introduced by commit
38922b1f4acc ("net: ti: am65-cpsw: Add support for multi port independent MAC mode")

which says "Since top level driver is now UCLASS_MISC, board
files would need to instantiate this driver explicitly.

I wonder if there can be a better solution to this?

-- 
cheers,
-roger