| Message ID | 20200508223216.6611-1-f.fainelli@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: broadcom: Imply BROADCOM_PHY for BCMGENET | expand |
Hi Florian, Thanks for your patch! On Sat, May 9, 2020 at 12:32 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > The GENET controller on the Raspberry Pi 4 (2711) is typically > interfaced with an external Broadcom PHY via a RGMII electrical > interface. To make sure that delays are properly configured at the PHY > side, ensure that we get a chance to have the dedicated Broadcom PHY > driver (CONFIG_BROADCOM_PHY) enabled for this to happen. I guess it can be interfaced to a different external PHY, too? > Fixes: 402482a6a78e ("net: bcmgenet: Clear ID_MODE_DIS in EXT_RGMII_OOB_CTRL when not needed") > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- a/drivers/net/ethernet/broadcom/Kconfig > +++ b/drivers/net/ethernet/broadcom/Kconfig > @@ -69,6 +69,7 @@ config BCMGENET > select BCM7XXX_PHY > select MDIO_BCM_UNIMAC > select DIMLIB > + imply BROADCOM_PHY if ARCH_BCM2835 Which means support for the BROADCOM_PHY is always included on ARCH_BCM2835, even if a different PHY is used? > help > This driver supports the built-in Ethernet MACs found in the > Broadcom BCM7xxx Set Top Box family chipset. Gr{oetje,eeting}s, Geert
On 5/9/2020 12:38 AM, Geert Uytterhoeven wrote: > Hi Florian, > > Thanks for your patch! > > On Sat, May 9, 2020 at 12:32 AM Florian Fainelli <f.fainelli@gmail.com> wrote: >> The GENET controller on the Raspberry Pi 4 (2711) is typically >> interfaced with an external Broadcom PHY via a RGMII electrical >> interface. To make sure that delays are properly configured at the PHY >> side, ensure that we get a chance to have the dedicated Broadcom PHY >> driver (CONFIG_BROADCOM_PHY) enabled for this to happen. > > I guess it can be interfaced to a different external PHY, too? Yes, although this has not happened yet to the best of my knowledge. > >> Fixes: 402482a6a78e ("net: bcmgenet: Clear ID_MODE_DIS in EXT_RGMII_OOB_CTRL when not needed") >> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- a/drivers/net/ethernet/broadcom/Kconfig >> +++ b/drivers/net/ethernet/broadcom/Kconfig >> @@ -69,6 +69,7 @@ config BCMGENET >> select BCM7XXX_PHY >> select MDIO_BCM_UNIMAC >> select DIMLIB >> + imply BROADCOM_PHY if ARCH_BCM2835 > > Which means support for the BROADCOM_PHY is always included > on ARCH_BCM2835, even if a different PHY is used? It is included by default on and can be deselected if needed, which is exactly what we want here, a sane default, but without the inflexibility of "select". > >> help >> This driver supports the built-in Ethernet MACs found in the >> Broadcom BCM7xxx Set Top Box family chipset. > > Gr{oetje,eeting}s, > > Geert >
Hi Florian, On Sat, May 9, 2020 at 7:12 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > On 5/9/2020 12:38 AM, Geert Uytterhoeven wrote: > > On Sat, May 9, 2020 at 12:32 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > >> The GENET controller on the Raspberry Pi 4 (2711) is typically > >> interfaced with an external Broadcom PHY via a RGMII electrical > >> interface. To make sure that delays are properly configured at the PHY > >> side, ensure that we get a chance to have the dedicated Broadcom PHY > >> driver (CONFIG_BROADCOM_PHY) enabled for this to happen. > > > > I guess it can be interfaced to a different external PHY, too? > > Yes, although this has not happened yet to the best of my knowledge. > > > > >> Fixes: 402482a6a78e ("net: bcmgenet: Clear ID_MODE_DIS in EXT_RGMII_OOB_CTRL when not needed") > >> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > >> --- a/drivers/net/ethernet/broadcom/Kconfig > >> +++ b/drivers/net/ethernet/broadcom/Kconfig > >> @@ -69,6 +69,7 @@ config BCMGENET > >> select BCM7XXX_PHY > >> select MDIO_BCM_UNIMAC > >> select DIMLIB > >> + imply BROADCOM_PHY if ARCH_BCM2835 > > > > Which means support for the BROADCOM_PHY is always included > > on ARCH_BCM2835, even if a different PHY is used? > > It is included by default on and can be deselected if needed, which is > exactly what we want here, a sane default, but without the inflexibility > of "select". I stand corrected: I can confirm the "imply" no longer selects the target symbol, but merely changes the default. Gr{oetje,eeting}s, Geert
Hi Florian, On 09.05.2020 00:32, Florian Fainelli wrote: > The GENET controller on the Raspberry Pi 4 (2711) is typically > interfaced with an external Broadcom PHY via a RGMII electrical > interface. To make sure that delays are properly configured at the PHY > side, ensure that we get a chance to have the dedicated Broadcom PHY > driver (CONFIG_BROADCOM_PHY) enabled for this to happen. > > Fixes: 402482a6a78e ("net: bcmgenet: Clear ID_MODE_DIS in EXT_RGMII_OOB_CTRL when not needed") > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > David, > > I would like Marek to indicate whether he is okay or not with this > change. Thanks! It is better. It fixes the default values for ARM 32bit bcm2835_defconfig and ARM 64bit defconfig, so you can add: Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> There is still an issue there. In case of ARM 64bit, when Genet driver is configured as a module, BROADCOM_PHY is also set to module. When I changed Genet to be built-in, BROADCOM_PHY stayed selected as module. This case doesn't work, as Genet driver is loaded much earlier than the rootfs/initrd/etc is available, thus broadcom phy driver is not loaded at all. It looks that some kind of deferred probe is missing there. Best regards
On 5/11/2020 12:21 AM, Marek Szyprowski wrote: > Hi Florian, > > On 09.05.2020 00:32, Florian Fainelli wrote: >> The GENET controller on the Raspberry Pi 4 (2711) is typically >> interfaced with an external Broadcom PHY via a RGMII electrical >> interface. To make sure that delays are properly configured at the PHY >> side, ensure that we get a chance to have the dedicated Broadcom PHY >> driver (CONFIG_BROADCOM_PHY) enabled for this to happen. >> >> Fixes: 402482a6a78e ("net: bcmgenet: Clear ID_MODE_DIS in EXT_RGMII_OOB_CTRL when not needed") >> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> David, >> >> I would like Marek to indicate whether he is okay or not with this >> change. Thanks! > > It is better. It fixes the default values for ARM 32bit > bcm2835_defconfig and ARM 64bit defconfig, so you can add: > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > > There is still an issue there. In case of ARM 64bit, when Genet driver > is configured as a module, BROADCOM_PHY is also set to module. When I > changed Genet to be built-in, BROADCOM_PHY stayed selected as module. OK. > This case doesn't work, as Genet driver is loaded much earlier than the > rootfs/initrd/etc is available, thus broadcom phy driver is not loaded > at all. It looks that some kind of deferred probe is missing there. In the absence of a specific PHY driver the Generic PHY driver gets used instead. This is a valid situation as I described in my other email because the boot loader/firmware could have left the PHY properly configured with the expected RGMII delays and configuration such that Linux does not need to be aware of anything. I suppose we could change the GENET driver when running on the 2711 platform to reject the PHY driver being "Generic PHY" on the premise that a specialized driver should be used instead, but that seems a bit too restrictive IMHO. Do you prefer a "select BROADCOM_PHY if ARCH_BCM2835" then?
Hi Florian, On 11.05.2020 20:19, Florian Fainelli wrote: > On 5/11/2020 12:21 AM, Marek Szyprowski wrote: >> On 09.05.2020 00:32, Florian Fainelli wrote: >>> The GENET controller on the Raspberry Pi 4 (2711) is typically >>> interfaced with an external Broadcom PHY via a RGMII electrical >>> interface. To make sure that delays are properly configured at the PHY >>> side, ensure that we get a chance to have the dedicated Broadcom PHY >>> driver (CONFIG_BROADCOM_PHY) enabled for this to happen. >>> >>> Fixes: 402482a6a78e ("net: bcmgenet: Clear ID_MODE_DIS in EXT_RGMII_OOB_CTRL when not needed") >>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>> --- >>> David, >>> >>> I would like Marek to indicate whether he is okay or not with this >>> change. Thanks! >> It is better. It fixes the default values for ARM 32bit >> bcm2835_defconfig and ARM 64bit defconfig, so you can add: >> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> >> >> There is still an issue there. In case of ARM 64bit, when Genet driver >> is configured as a module, BROADCOM_PHY is also set to module. When I >> changed Genet to be built-in, BROADCOM_PHY stayed selected as module. > OK. > >> This case doesn't work, as Genet driver is loaded much earlier than the >> rootfs/initrd/etc is available, thus broadcom phy driver is not loaded >> at all. It looks that some kind of deferred probe is missing there. > In the absence of a specific PHY driver the Generic PHY driver gets used > instead. This is a valid situation as I described in my other email > because the boot loader/firmware could have left the PHY properly > configured with the expected RGMII delays and configuration such that > Linux does not need to be aware of anything. I suppose we could change > the GENET driver when running on the 2711 platform to reject the PHY > driver being "Generic PHY" on the premise that a specialized driver > should be used instead, but that seems a bit too restrictive IMHO. > > Do you prefer a "select BROADCOM_PHY if ARCH_BCM2835" then? Yes. It solves the issue after switching Genet to be built-in without the need to change the CONFIG_BROADCOM_PHY. Best regards
diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig index 53055ce5dfd6..8a70b9152f7c 100644 --- a/drivers/net/ethernet/broadcom/Kconfig +++ b/drivers/net/ethernet/broadcom/Kconfig @@ -69,6 +69,7 @@ config BCMGENET select BCM7XXX_PHY select MDIO_BCM_UNIMAC select DIMLIB + imply BROADCOM_PHY if ARCH_BCM2835 help This driver supports the built-in Ethernet MACs found in the Broadcom BCM7xxx Set Top Box family chipset.
The GENET controller on the Raspberry Pi 4 (2711) is typically interfaced with an external Broadcom PHY via a RGMII electrical interface. To make sure that delays are properly configured at the PHY side, ensure that we get a chance to have the dedicated Broadcom PHY driver (CONFIG_BROADCOM_PHY) enabled for this to happen. Fixes: 402482a6a78e ("net: bcmgenet: Clear ID_MODE_DIS in EXT_RGMII_OOB_CTRL when not needed") Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- David, I would like Marek to indicate whether he is okay or not with this change. Thanks! drivers/net/ethernet/broadcom/Kconfig | 1 + 1 file changed, 1 insertion(+)