| Message ID | 20221125115003.30308-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 421f8663b3a775c32f724f793264097c60028f2e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: broadcom: Add PTP_1588_CLOCK_OPTIONAL dependency for BCMGENET under ARCH_BCM2835 | expand |
On Fri, Nov 25, 2022, at 12:50, YueHaibing wrote: > commit 8d820bc9d12b ("net: broadcom: Fix BCMGENET Kconfig") fixes the build > that contain 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") > and enable BCMGENET=y but PTP_1588_CLOCK_OPTIONAL=m, which otherwise > leads to a link failure. However this may trigger a runtime failure. > > Fix the original issue by propagating the PTP_1588_CLOCK_OPTIONAL dependency > of BROADCOM_PHY down to BCMGENET. > > Fixes: 8d820bc9d12b ("net: broadcom: Fix BCMGENET Kconfig") > Fixes: 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: YueHaibing <yuehaibing@huawei.com> Thanks for fixing this, Acked-by: Arnd Bergmann <arnd@arndb.de>
On Fri, 25 Nov 2022 19:50:03 +0800 YueHaibing wrote: > commit 8d820bc9d12b ("net: broadcom: Fix BCMGENET Kconfig") fixes the build > that contain 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") > and enable BCMGENET=y but PTP_1588_CLOCK_OPTIONAL=m, which otherwise > leads to a link failure. However this may trigger a runtime failure. > > Fix the original issue by propagating the PTP_1588_CLOCK_OPTIONAL dependency > of BROADCOM_PHY down to BCMGENET. > > Fixes: 8d820bc9d12b ("net: broadcom: Fix BCMGENET Kconfig") > Fixes: 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > drivers/net/ethernet/broadcom/Kconfig | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig > index 55dfdb34e37b..f4ca0c6c0f51 100644 > --- a/drivers/net/ethernet/broadcom/Kconfig > +++ b/drivers/net/ethernet/broadcom/Kconfig > @@ -71,13 +71,14 @@ config BCM63XX_ENET > config BCMGENET > tristate "Broadcom GENET internal MAC support" > depends on HAS_IOMEM > + depends on PTP_1588_CLOCK_OPTIONAL || !ARCH_BCM2835 > select MII > select PHYLIB > select FIXED_PHY > select BCM7XXX_PHY > select MDIO_BCM_UNIMAC > select DIMLIB > - select BROADCOM_PHY if (ARCH_BCM2835 && PTP_1588_CLOCK_OPTIONAL) > + select BROADCOM_PHY if ARCH_BCM2835 > help > This driver supports the built-in Ethernet MACs found in the > Broadcom BCM7xxx Set Top Box family chipset. What's the code path that leads to the failure? I want to double check that the driver is handling the PTP registration return codes correctly. IIUC this is a source of misunderstandings in the PTP API. Richard, here's the original report: https://lore.kernel.org/all/CA+G9fYvKfbJHcMZtybf_0Ru3+6fKPg9HwWTOhdCLrOBXMaeG1A@mail.gmail.com/
On Tue, Nov 29, 2022, at 04:18, Jakub Kicinski wrote: > On Fri, 25 Nov 2022 19:50:03 +0800 YueHaibing wrote: >> diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig >> index 55dfdb34e37b..f4ca0c6c0f51 100644 >> --- a/drivers/net/ethernet/broadcom/Kconfig >> +++ b/drivers/net/ethernet/broadcom/Kconfig >> @@ -71,13 +71,14 @@ config BCM63XX_ENET >> config BCMGENET >> tristate "Broadcom GENET internal MAC support" >> depends on HAS_IOMEM >> + depends on PTP_1588_CLOCK_OPTIONAL || !ARCH_BCM2835 >> select MII >> select PHYLIB >> select FIXED_PHY >> select BCM7XXX_PHY >> select MDIO_BCM_UNIMAC >> select DIMLIB >> - select BROADCOM_PHY if (ARCH_BCM2835 && PTP_1588_CLOCK_OPTIONAL) >> + select BROADCOM_PHY if ARCH_BCM2835 >> help >> This driver supports the built-in Ethernet MACs found in the >> Broadcom BCM7xxx Set Top Box family chipset. > > What's the code path that leads to the failure? I want to double check > that the driver is handling the PTP registration return codes correctly. > IIUC this is a source of misunderstandings in the PTP API. > > Richard, here's the original report: > https://lore.kernel.org/all/CA+G9fYvKfbJHcMZtybf_0Ru3+6fKPg9HwWTOhdCLrOBXMaeG1A@mail.gmail.com The original report was for a different bug that resulted in the BROADCOM_PHY driver not being selectable at all. The remaining problem here is this configuration: CONFIG_ARM=y CONFIG_BCM2835=y CONFIG_BCMGENET=y CONFIG_PTP_1588_CLOCK=m CONFIG_PTP_1588_CLOCK_OPTIONAL=m CONFIG_BROADCOM_PHY=m In this case, BCMGENET should 'select BROADCOM_PHY' to make the driver work correctly, but fails to do this because of the dependency. During early boot, this means it cannot access the PHY because that is in a loadable module, despite commit 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") trying to ensure that it could. Note that many other ethernet drivers don't have this particular 'select' statement and just rely on the .config to contain a sensible set of drivers. In particular that is true when running 64-bit kernels on the same chip, which is now the normal configuration. The alternative to YueHaibing's fix would be to just revert 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") and instead change the defconfig file to include the phy driver, as we do elsewhere. Arnd
[Florian's broadcom.com address bounces, adding him to Cc with his gmail address] On Tue, Nov 29, 2022, at 12:56, Arnd Bergmann wrote: > On Tue, Nov 29, 2022, at 04:18, Jakub Kicinski wrote: >> On Fri, 25 Nov 2022 19:50:03 +0800 YueHaibing wrote: >>> diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig >>> index 55dfdb34e37b..f4ca0c6c0f51 100644 >>> --- a/drivers/net/ethernet/broadcom/Kconfig >>> +++ b/drivers/net/ethernet/broadcom/Kconfig >>> @@ -71,13 +71,14 @@ config BCM63XX_ENET >>> config BCMGENET >>> tristate "Broadcom GENET internal MAC support" >>> depends on HAS_IOMEM >>> + depends on PTP_1588_CLOCK_OPTIONAL || !ARCH_BCM2835 >>> select MII >>> select PHYLIB >>> select FIXED_PHY >>> select BCM7XXX_PHY >>> select MDIO_BCM_UNIMAC >>> select DIMLIB >>> - select BROADCOM_PHY if (ARCH_BCM2835 && PTP_1588_CLOCK_OPTIONAL) >>> + select BROADCOM_PHY if ARCH_BCM2835 >>> help >>> This driver supports the built-in Ethernet MACs found in the >>> Broadcom BCM7xxx Set Top Box family chipset. >> >> What's the code path that leads to the failure? I want to double check >> that the driver is handling the PTP registration return codes correctly. >> IIUC this is a source of misunderstandings in the PTP API. >> >> Richard, here's the original report: >> https://lore.kernel.org/all/CA+G9fYvKfbJHcMZtybf_0Ru3+6fKPg9HwWTOhdCLrOBXMaeG1A@mail.gmail.com > > The original report was for a different bug that resulted in the > BROADCOM_PHY driver not being selectable at all. > > The remaining problem here is this configuration: > > CONFIG_ARM=y > CONFIG_BCM2835=y > CONFIG_BCMGENET=y > CONFIG_PTP_1588_CLOCK=m > CONFIG_PTP_1588_CLOCK_OPTIONAL=m > CONFIG_BROADCOM_PHY=m > > In this case, BCMGENET should 'select BROADCOM_PHY' to make the > driver work correctly, but fails to do this because of the > dependency. During early boot, this means it cannot access the > PHY because that is in a loadable module, despite commit > 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") > trying to ensure that it could. > > Note that many other ethernet drivers don't have this > particular 'select' statement and just rely on the .config > to contain a sensible set of drivers. In particular that > is true when running 64-bit kernels on the same chip, > which is now the normal configuration. > > The alternative to YueHaibing's fix would be to just revert > 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") > and instead change the defconfig file to include the phy driver, > as we do elsewhere. > > Arnd
On 11/29/22 03:58, Arnd Bergmann wrote: > [Florian's broadcom.com address bounces, adding him to Cc > with his gmail address] Yes, because it's not a valid email address :) it's either lastname, or first.lastname. > > On Tue, Nov 29, 2022, at 12:56, Arnd Bergmann wrote: >> On Tue, Nov 29, 2022, at 04:18, Jakub Kicinski wrote: >>> On Fri, 25 Nov 2022 19:50:03 +0800 YueHaibing wrote: >>>> diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig >>>> index 55dfdb34e37b..f4ca0c6c0f51 100644 >>>> --- a/drivers/net/ethernet/broadcom/Kconfig >>>> +++ b/drivers/net/ethernet/broadcom/Kconfig >>>> @@ -71,13 +71,14 @@ config BCM63XX_ENET >>>> config BCMGENET >>>> tristate "Broadcom GENET internal MAC support" >>>> depends on HAS_IOMEM >>>> + depends on PTP_1588_CLOCK_OPTIONAL || !ARCH_BCM2835 >>>> select MII >>>> select PHYLIB >>>> select FIXED_PHY >>>> select BCM7XXX_PHY >>>> select MDIO_BCM_UNIMAC >>>> select DIMLIB >>>> - select BROADCOM_PHY if (ARCH_BCM2835 && PTP_1588_CLOCK_OPTIONAL) >>>> + select BROADCOM_PHY if ARCH_BCM2835 >>>> help >>>> This driver supports the built-in Ethernet MACs found in the >>>> Broadcom BCM7xxx Set Top Box family chipset. >>> >>> What's the code path that leads to the failure? I want to double check >>> that the driver is handling the PTP registration return codes correctly. >>> IIUC this is a source of misunderstandings in the PTP API. >>> >>> Richard, here's the original report: >>> https://lore.kernel.org/all/CA+G9fYvKfbJHcMZtybf_0Ru3+6fKPg9HwWTOhdCLrOBXMaeG1A@mail.gmail.com >> >> The original report was for a different bug that resulted in the >> BROADCOM_PHY driver not being selectable at all. >> >> The remaining problem here is this configuration: >> >> CONFIG_ARM=y >> CONFIG_BCM2835=y >> CONFIG_BCMGENET=y >> CONFIG_PTP_1588_CLOCK=m >> CONFIG_PTP_1588_CLOCK_OPTIONAL=m >> CONFIG_BROADCOM_PHY=m >> >> In this case, BCMGENET should 'select BROADCOM_PHY' to make the >> driver work correctly, but fails to do this because of the >> dependency. During early boot, this means it cannot access the >> PHY because that is in a loadable module, despite commit >> 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") >> trying to ensure that it could. I don't believe this is the failure mechanism because there is always a fallback to the Generic PHY driver, the issue is that we have configured a specific RGMII mode in the Device Tree that is acted on by both the Ethernet MAC and the PHY in a way that is electrically incompatible unless the proper PHY driver is also enabled such that the RGMII mode is enabled on the PHY side.
On Tue, 29 Nov 2022 12:58:23 +0100 Arnd Bergmann wrote: > > The original report was for a different bug that resulted in the > > BROADCOM_PHY driver not being selectable at all. > > > > The remaining problem here is this configuration: > > > > CONFIG_ARM=y > > CONFIG_BCM2835=y > > CONFIG_BCMGENET=y > > CONFIG_PTP_1588_CLOCK=m > > CONFIG_PTP_1588_CLOCK_OPTIONAL=m > > CONFIG_BROADCOM_PHY=m > > > > In this case, BCMGENET should 'select BROADCOM_PHY' to make the > > driver work correctly, but fails to do this because of the > > dependency. During early boot, this means it cannot access the > > PHY because that is in a loadable module, despite commit > > 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") > > trying to ensure that it could. > > > > Note that many other ethernet drivers don't have this > > particular 'select' statement and just rely on the .config > > to contain a sensible set of drivers. In particular that > > is true when running 64-bit kernels on the same chip, > > which is now the normal configuration. > > > > The alternative to YueHaibing's fix would be to just revert > > 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") > > and instead change the defconfig file to include the phy driver, > > as we do elsewhere. Ah, got it now, I think. Alternatively we could flip the 'select' to 'depends on' and make the user do the legwork? Enough brain cycles spend on this simple fix tho, so applying, thanks! :)
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Fri, 25 Nov 2022 19:50:03 +0800 you wrote: > commit 8d820bc9d12b ("net: broadcom: Fix BCMGENET Kconfig") fixes the build > that contain 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") > and enable BCMGENET=y but PTP_1588_CLOCK_OPTIONAL=m, which otherwise > leads to a link failure. However this may trigger a runtime failure. > > Fix the original issue by propagating the PTP_1588_CLOCK_OPTIONAL dependency > of BROADCOM_PHY down to BCMGENET. > > [...] Here is the summary with links: - net: broadcom: Add PTP_1588_CLOCK_OPTIONAL dependency for BCMGENET under ARCH_BCM2835 https://git.kernel.org/netdev/net/c/421f8663b3a7 You are awesome, thank you!
diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig index 55dfdb34e37b..f4ca0c6c0f51 100644 --- a/drivers/net/ethernet/broadcom/Kconfig +++ b/drivers/net/ethernet/broadcom/Kconfig @@ -71,13 +71,14 @@ config BCM63XX_ENET config BCMGENET tristate "Broadcom GENET internal MAC support" depends on HAS_IOMEM + depends on PTP_1588_CLOCK_OPTIONAL || !ARCH_BCM2835 select MII select PHYLIB select FIXED_PHY select BCM7XXX_PHY select MDIO_BCM_UNIMAC select DIMLIB - select BROADCOM_PHY if (ARCH_BCM2835 && PTP_1588_CLOCK_OPTIONAL) + select BROADCOM_PHY if ARCH_BCM2835 help This driver supports the built-in Ethernet MACs found in the Broadcom BCM7xxx Set Top Box family chipset.
commit 8d820bc9d12b ("net: broadcom: Fix BCMGENET Kconfig") fixes the build that contain 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") and enable BCMGENET=y but PTP_1588_CLOCK_OPTIONAL=m, which otherwise leads to a link failure. However this may trigger a runtime failure. Fix the original issue by propagating the PTP_1588_CLOCK_OPTIONAL dependency of BROADCOM_PHY down to BCMGENET. Fixes: 8d820bc9d12b ("net: broadcom: Fix BCMGENET Kconfig") Fixes: 99addbe31f55 ("net: broadcom: Select BROADCOM_PHY for BCMGENET") Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- drivers/net/ethernet/broadcom/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)