| Message ID | 20240425120822.2048012-7-c-vankar@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add support for Ethernet Boot on SK-AM62 | expand |
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. > spl_enable_cache(); > > fixup_a53_cpu_freq_by_speed_grade();
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. >> spl_enable_cache(); >> >> fixup_a53_cpu_freq_by_speed_grade(); >
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(); >>
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. >> >>>> spl_enable_cache(); >>>> fixup_a53_cpu_freq_by_speed_grade(); >>> >
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?
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"); + } + spl_enable_cache(); fixup_a53_cpu_freq_by_speed_grade();