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

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

[Nicolas Saenz Julienne]

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 <nsaenzjulienne@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);
 
 	writel(speed << CMD_SPEED_SHIFT, (priv->mac_reg + UMAC_CMD));
 
-- 
2.25.0

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

[Nicolas Saenz Julienne]

On Thu, 2020-02-20 at 17:36 +0100, 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 <nsaenzjulienne@suse.de>

I forgot to add:

Fixes: d53e3fa385 ("net: Add support for Broadcom GENETv5 Ethernet controller")

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200220/9d3f1075/attachment-0001.sig>

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

[Matthias Brugger]

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 <nsaenzjulienne@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?

BTW Joe, will you take patches for this driver through your branch? For now I
delegated it to me, but I'm fine either way.

Regards,
Matthias

>  
>  	writel(speed << CMD_SPEED_SHIFT, (priv->mac_reg + UMAC_CMD));
>  
> 

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

[Florian Fainelli]

On 2/20/20 8:36 AM, 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 <nsaenzjulienne@suse.de>

Does the failure look like the following: you have a driver for the
Broadcom PHY used on the Pi4 in u-boot, and the phy_dev->interface value
is being used to configure the Ethernet PHY chip in a certain way.

Later when you boot Linux, you do not have CONFIG_BROADCOM_PHY enabled
so the Generic PHY driver gets used instead, and there is a disagreement
between the AMAC and PHY as to whom should be adding the delays?

At any rate:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  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);
>  
>  	writel(speed << CMD_SPEED_SHIFT, (priv->mac_reg + UMAC_CMD));
>  
> 


-- 
Florian

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

[Nicolas Saenz Julienne]

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 <nsaenzjulienne@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).

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200221/f0024b31/attachment.sig>

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

[Nicolas Saenz Julienne]

Hi Florian,

On Thu, 2020-02-20 at 11:05 -0800, Florian Fainelli wrote:
> On 2/20/20 8:36 AM, 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 <nsaenzjulienne@suse.de>
> 
> Does the failure look like the following: you have a driver for the
> Broadcom PHY used on the Pi4 in u-boot, and the phy_dev->interface value
> is being used to configure the Ethernet PHY chip in a certain way.
> 
> Later when you boot Linux, you do not have CONFIG_BROADCOM_PHY enabled
> so the Generic PHY driver gets used instead, and there is a disagreement
> between the AMAC and PHY as to whom should be adding the delays?

I added an explanation to Matthias' response. I think it fits yours, modulo my
limited knowledge in the area :)

> At any rate:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200221/8431dedf/attachment.sig>

[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 <nsaenzjulienne@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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200221/b3b0b39d/attachment.sig>

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

[Nicolas Saenz Julienne]

On Thu, 2020-02-20 at 18:48 +0100, Nicolas Saenz Julienne wrote:
> On Thu, 2020-02-20 at 17:36 +0100, 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 <nsaenzjulienne@suse.de>
> 
> I forgot to add:
> 
> Fixes: d53e3fa385 ("net: Add support for Broadcom GENETv5 Ethernet
> controller")

Ping :)

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200311/81793e0e/attachment.sig>