Re: [PATCH] net: bcmgenet: Don't set ID_MODE_DIS when not using RGMII

[Nicolas Saenz Julienne]

Florian,

On Fri, 2020-02-21 at 11:54 +0100, Nicolas Saenz Julienne wrote:
> Hi Matthias,
> 
> On Thu, 2020-02-20 at 19:58 +0100, Matthias Brugger wrote:
> > On 20/02/2020 17:36, Nicolas Saenz Julienne wrote:
> > > As per Linux's driver, ID_MODE_DIS is only set when the PHY interface is
> > > RGMII. Don't enable it for the rest of setups.
> > > 
> > > This has been seen to misconfigure RPi4's PHY when booting Linux.
> > > 
> > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulie...@suse.de>
> > > ---
> > >  drivers/net/bcmgenet.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/bcmgenet.c b/drivers/net/bcmgenet.c
> > > index 8f4848aec6..e971b556ac 100644
> > > --- a/drivers/net/bcmgenet.c
> > > +++ b/drivers/net/bcmgenet.c
> > > @@ -448,7 +448,10 @@ static int bcmgenet_adjust_link(struct
> > > bcmgenet_eth_priv *priv)
> > >   }
> > >  
> > >   clrsetbits_32(priv->mac_reg + EXT_RGMII_OOB_CTRL, OOB_DISABLE,
> > > -                 RGMII_LINK | RGMII_MODE_EN | ID_MODE_DIS);
> > > +                 RGMII_LINK | RGMII_MODE_EN);
> > > +
> > > + if (phy_dev->interface == PHY_INTERFACE_MODE_RGMII)
> > > +         setbits_32(priv->mac_reg + EXT_RGMII_OOB_CTRL, ID_MODE_DIS);
> > 
> > Is this given because by different DTS? Shouldn't that be uniform on the
> > RPi4?
> 
> The interface type is read from DT, the 'phy-mode' property. In the case of
> the
> RPi4 it's 'rgmii-rxid'.
> 
> The downstream DT used to be configured differently ('rgmii' and using
> 'ethernet-phy-ieee802.3-c22'), that's why you might have seen the board
> working
> at some point with this driver. But as we updated the DT to match upstream's
> we
> switched to 'rgmii-rxid' which is being misconfigured as 'rgmii' in u-boot. So
> you have u-boot configuring 'rgmii' while Linux configures 'rgmii-rxid', which
> fails to clear the ID_MODE_DIS bit. This, I imagine, blocks the delay
> configuration process from the PHY (I don't have any documentation).

With this in mind, would it make sens to do this in the Linux driver?

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c
b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 6392a2530183..10244941a7a6 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -294,6 +294,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
        */
        if (priv->ext_phy) {
                reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL);
+               reg &= ~ID_MODE_DIS;
                reg |= id_mode_dis;
                if (GENET_IS_V1(priv) || GENET_IS_V2(priv) || GENET_IS_V3(priv))
                        reg |= RGMII_MODE_EN_V123;

It all comes down to wheter we trust bootloader's config or not.

Regards,
Nicolas

signature.asc
Description: This is a digitally signed message part