| Message ID | 20200220163630.29806-1-nsaenzjulienne@suse.de |
|---|---|
| State | Accepted |
| Commit | 57805f2270c4a47cbc221e0920c58654f7748f0b |
| Headers | show |
| Series | net: bcmgenet: Don't set ID_MODE_DIS when not using RGMII | expand |
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 at 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>
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 at 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)); > >
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 at 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 at 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)); > >
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 at 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>
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 at 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 at 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>
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 at 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>
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));
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 at suse.de> --- drivers/net/bcmgenet.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)