Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

Jonas Gorski
2025-05-19T19:44:28Z

On Mon, May 19, 2025 at 9:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, May 19, 2025 at 07:45:49PM +0200, Jonas Gorski wrote:
> > The RGMII delay type of the PHY interface is intended for the PHY, not
> > the MAC, so we need to configure the opposite. Else we double the delay
> > or don't add one at all if the PHY also supports configuring delays.
> >
> > Additionally, we need to enable RGMII_CTRL_TIMING_SEL for the delay
> > actually being effective.
> >
> > Fixes e.g. BCM54612E connected on RGMII ports that also configures RGMII
> > delays in its driver.
>
> We have to be careful here not to cause regressions. It might be
> wrong, but are there systems using this which actually work? Does this
> change break them?

The only user (of bcm63xx and b53 dsa) I am aware of is OpenWrt, and
we are capable of updating our dts files in case they were using
broken configuration. Though having PHYs on the RGMII ports is a very
rare configuration, and usually there is switch connected with a fixed
link, so likely the issue was never detected.

> >
> > Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> > ---
> > drivers/net/dsa/b53/b53_common.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> > index a316f8c01d0a..b00975189dab 100644
> > --- a/drivers/net/dsa/b53/b53_common.c
> > +++ b/drivers/net/dsa/b53/b53_common.c
> > @@ -1328,19 +1328,19 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
> >
> > switch (interface) {
> > case PHY_INTERFACE_MODE_RGMII_ID:
> > - rgmii_ctrl |= (RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> > + rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> > break;
> > case PHY_INTERFACE_MODE_RGMII_RXID:
> > - rgmii_ctrl &= ~(RGMII_CTRL_DLL_TXC);
> > - rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
> > + rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
> > + rgmii_ctrl &= ~RGMII_CTRL_DLL_RXC;
> > break;
> > case PHY_INTERFACE_MODE_RGMII_TXID:
> > - rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC);
> > - rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
> > + rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
> > + rgmii_ctrl &= ~RGMII_CTRL_DLL_TXC;
> > break;
> > case PHY_INTERFACE_MODE_RGMII:
> > default:
> > - rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> > + rgmii_ctrl |= RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC;
> > break;
>
> These changes look wrong. There is more background here:
>
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

This is what makes it work for me (I tested all four modes, rgmii,
rgmii-id, rgmii-txid and rgmii-rxid). Without this change, b53 will
configure the same delays on the MAC layer as the PHY driver (bcm54xx,
https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/phy/broadcom.c#L73
), which breaks connectivity at least for me.

E.g. with a phy-mode of "rgmii-id", both b53 and the PHY driver would
enable rx and tx delays, causing the delays to be 4 ns instead of 2
ns. So I don't see how this could have ever worked.

Also note that b53_adjust_531x5_rgmii()
https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/dsa/b53/b53_common.c#L1360
already behaves that way, this just makes bcm63xx now work the same
(so these functions could now even be merged).

I did see the part where the document says the MAC driver is supposed
to change the phy mode, but I haven't really found out how I am
supposed to do that within phylink/dsa, since it never passes any phy
modes to any PHYs anywhere, just reports what it supports, then
phylink tells it what to configure AFAICT.

Best regards,
Jonas