Re: [PATCH 04/13] arm: mach-k3: j721s2_init: Probe AM65 CPSW NUSS for R5/A72 SPL

[Roger Quadros <rogerq@kernel.org>]

On 28/01/2025 06:09, Chintan Vankar wrote:
> 
> 
> On 23/01/25 15:10, Roger Quadros wrote:
>> +Vignesh & Siddharth
>>
>> On 22/01/2025 10:53, Chintan Vankar wrote:
>>>
>>>
>>> On 07/01/25 20:03, Roger Quadros wrote:
>>>>
>>>>
>>>> On 07/01/2025 11:38, Chintan Vankar wrote:
>>>>> To support Ethernet boot on AM68-SK, probe AM65 CPSW NUSS driver in
>>>>> board_init_f().
>>>>>
>>>>> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
>>>>> ---
>>>>>    arch/arm/mach-k3/j721s2/j721s2_init.c | 10 ++++++++++
>>>>>    1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/mach-k3/j721s2/j721s2_init.c b/arch/arm/mach-k3/j721s2/j721s2_init.c
>>>>> index 6ce3eb87efb..7208bee5785 100644
>>>>> --- a/arch/arm/mach-k3/j721s2/j721s2_init.c
>>>>> +++ b/arch/arm/mach-k3/j721s2/j721s2_init.c
>>>>> @@ -329,6 +329,16 @@ void board_init_f(ulong dummy)
>>>>>          setup_qos();
>>>>>    +    if (IS_ENABLED(CONFIG_SPL_ETH) && IS_ENABLED(CONFIG_TI_AM65_CPSW_NUSS) &&
>>>>> +        spl_boot_device() == BOOT_DEVICE_ETHERNET) {
>>>>> +        struct udevice *cpswdev;
>>>>> +
>>>>> +        ret = uclass_get_device_by_driver(UCLASS_MISC, DM_DRIVER_GET(am65_cpsw_nuss),
>>>>> +                          &cpswdev);
>>>>> +        if (ret)
>>>>> +            printf("Failed to probe am65_cpsw_nuss driver..\n");
>>>>> +    }
>>>>> +
>>>>
>>>> This looks like a hack. Please find out why the Ethernet driver is not being
>>>> probed when SPL tries to load image over net.
>>>>
>>>
>>> Hello Roger,
>>>
>>> The driver is defined as UCLASS_MISC which should be probed explicitly,
>>
>> The driver was originally built without UCLASS_MISC, but
>>
>> commit 38922b1f4acc ("net: ti: am65-cpsw: Add support for multi port independent MAC mode")
>>
>> Added UCLASS_MISC and states there that
>> " Since top level driver is now UCLASS_MISC, board files would need to
>>    instantiate this driver explicitly."
>>
>> Not an elegant solution. So we need to fix something in the am65-cpuss driver.
>>
>> Looking at drivers/net/mvpp2.c we can see a possible solution.
>> 1) don't define .probe for the parent driver (UCLASS_MISC). Instead define .bind
>> that will scan the device tree for ports and bind the port device and driver.
>> e.g. see mvpp2_base_bind()
>> 2) in port driver .probe (UCLASS_ETH), if parent has not been probed
>> then call the parent probe (am65_cpsw_probe_nuss) . Also set a flag so parent probe
>> only gets called once.
>> 3) update all board files no not explicitly probe the am65-cpsw UCLASS_MISC driver.
>>
>> Siddharth / Vignesh do you see any issues with this solution?
>>
> 
> Hello Roger, I have tried to understand your approach, I will work on
> that. But for now, is it possible to merge the seris with the existing
> approach ?

Hi Chintan,

I'm afraid that if we allow this for one board it will set a wrong example
for others.
It is a very simple solution and shouldn't take much time. In case you are
stuck me or Siddharth could help. Thanks!

> 
>>> I have discussed the same with Nishanth in following thread:
>>> https://lore.kernel.org/all/ee1d16fd-b99b-4b07-97bb-a896e179157a@ti.com/
>>>
>>>> We have the following defined at the am65-cpsw-nuss driver.
>>>>
>>>> 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,
>>>> };
>>>>
>>>> It looks like am65_cpsw_probe_nuss() is not being invoked for you.
>>>>
>>>> Can you please check if am65_cpsw_port_probe() was invoked?
>>>> If yes but am65_cpsw_probe_nuss() was not then we need to fix the
>>>> DM hierarchy for AM65_CPSW?
>>>>
>>> None of the probe function is getting invoked here, since we need
>>
>> That is strange. am65_cpsw_probe_nuss() should be called at least for
>> port 0 since it is defined as UCLASS_ETH.
>>
> 
> am65_cpsw_probe_nuss() is defined as UCLASS_MISC and not UCLASS_ETH, it
> will get invoked if declared as UCLASS_ETH, as per your suggestion above
> it will need driver changes to invoke the probe function without being
> called explicitly and work properly.
> 
>>> Ethernet functionality here we need to probe function in board_init_f().
>>> We have discussed the same at here:
>>> https://lore.kernel.org/r/d68c8045-116a-49c0-9e74-d6a366e6f008@kernel.org/#t
>>>
>>>
>>>>>        if (IS_ENABLED(CONFIG_CPU_V7R) && IS_ENABLED(CONFIG_K3_AVS0)) {
>>>>>            ret = uclass_get_device_by_driver(UCLASS_MISC, DM_DRIVER_GET(k3_avs),
>>>>>                              &dev);
>>>>
>>

-- 
cheers,
-roger