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.
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;
}
@@ -1350,6 +1350,7 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
rgmii_ctrl |= RGMII_CTRL_ENABLE_GMII;
}
+ rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
b53_write8(dev, B53_CTRL_PAGE, off, rgmii_ctrl);
--
2.43.0On 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?
>
> 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
Andrew
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
On 5/19/25 12:44, Jonas Gorski wrote:
> 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.
It's possible this was tested with an external PHY which was not
configured by drivers/net/phy/broadcom.c?
>
> 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).
Yes, I was going to suggest we should be doing that.
There are precedents with other Broadcom drivers for doing things
incorrectly, and having to put quirks to deal with that, yours truly
having greatly contributed to doing that:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b972b54a68b2512a7528658ecd023aea108c03a5
--
Florian
> > 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).
So you have rgmii-id which works, and the rest are broken?
> 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.
In this situation, the switch port is playing the role of MAC. Hence i
would stick to what is suggested in ethernet-controller.yaml. The MAC
does not add any delays, and leaves it to the PHY.
phylink_fwnode_phy_connect() gets the phy-mode from the DT blob, and
passes it to phylib when it calls phy_attach_direct().
There is a second use case for DSA, when the port is connected to the
host, i.e. the port is a CPU port. In that setup, the port is playing
the PHY side of the RGMII connection, with the host conduit interface
being the MAC. In that setup, the above recommendations is that the
conduit interface does not add delays, with the expectation the 'PHY'
adds the delays. Then the DSA port does need to add delays. So you
might want to use dsa_is_cpu_port() to decide if to add delays or not.
Andrew
---
pw-bot: cr
On Mon, May 19, 2025 at 10:34 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > 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). > > So you have rgmii-id which works, and the rest are broken? I have four rgmii ports with PHYs, I configured each one to a different mode (rgmii, rgmii-txid, rgmii-rxid, rgmii-id). Without this change no mode/port works, since there is always either a 0 ns delay or a 4 ns delay in the rx/tx paths (I assume, I have no equipment to measure). With this change all modes/ports work. With "rgmii-id" the mac doesn't configure any delays (and the phy does instead), with "rgmii" it's vice versa, so there is always the expected 2 ns delay. Same for rxid and txid. Though on testing again RGMII_CTRL_TIMING_SEL doesn't seem to be needed. > > 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. > > In this situation, the switch port is playing the role of MAC. Hence i > would stick to what is suggested in ethernet-controller.yaml. The MAC > does not add any delays, and leaves it to the PHY. > phylink_fwnode_phy_connect() gets the phy-mode from the DT blob, and > passes it to phylib when it calls phy_attach_direct(). > > There is a second use case for DSA, when the port is connected to the > host, i.e. the port is a CPU port. In that setup, the port is playing > the PHY side of the RGMII connection, with the host conduit interface > being the MAC. In that setup, the above recommendations is that the > conduit interface does not add delays, with the expectation the 'PHY' > adds the delays. Then the DSA port does need to add delays. So you > might want to use dsa_is_cpu_port() to decide if to add delays or not. The Switch is always integrated into the host SoC, so there is no (r)gmii cpu port to configure. There's basically directly attached DMA to/from the buffers of the cpu port. Not sure if there are even buffers, or if it is a direct to DMA delivery. Best Regards, Jonas
> Without this change no mode/port works, since there is always either a > 0 ns delay or a 4 ns delay in the rx/tx paths (I assume, I have no > equipment to measure). > > With this change all modes/ports work. Which is wrong. > With "rgmii-id" the mac doesn't > configure any delays (and the phy does instead), with "rgmii" it's > vice versa, so there is always the expected 2 ns delay. Same for rxid > and txid. If you read the description of what these four modes mean, you should understand why only one should work. And given the most likely PCB design, the only mode that should work is rgmii-id. You would have to change the PCB design, to make the other modes work. > The Switch is always integrated into the host SoC, so there is no > (r)gmii cpu port to configure. There's basically directly attached DMA > to/from the buffers of the cpu port. Not sure if there are even > buffers, or if it is a direct to DMA delivery. That makes it a lot simpler. It always plays the MAC side. So i recommend you just hard code it no delay, and let the PHY add the delays as needed. Andrew
On Tue, May 20, 2025 at 2:15 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > Without this change no mode/port works, since there is always either a > > 0 ns delay or a 4 ns delay in the rx/tx paths (I assume, I have no > > equipment to measure). > > > > With this change all modes/ports work. > > Which is wrong. > > > With "rgmii-id" the mac doesn't > > configure any delays (and the phy does instead), with "rgmii" it's > > vice versa, so there is always the expected 2 ns delay. Same for rxid > > and txid. > > If you read the description of what these four modes mean, you should > understand why only one should work. And given the most likely PCB > design, the only mode that should work is rgmii-id. You would have to > change the PCB design, to make the other modes work. Since I also have BCM6368 with a BCM53115 connected to one of the RGMII ports lying around, I played around with it, and it was surprisingly hard to make it *not* work. Only if I enabled delay on *both* sides it stopped working, no delay or delay only on one side continued working (and I used iperf to try if larger amounts of traffic break it). So in way, with BCM6368 enabling no (sampling) delays on its MAC side, all four modes work. I understand that they shouldn't, but the reality is that they do. Maybe the switches can auto-detect/adapt to (missing) delays for a certain amount. @Florian, do you know if this is expected? And yes, I even added the RGMII delay workaround (change link speed to force the pll to resync) to ensure that the delays are applied. Though I guess the way Linux works it isn't needed, and only when changing delays while the link is up. > > The Switch is always integrated into the host SoC, so there is no > > (r)gmii cpu port to configure. There's basically directly attached DMA > > to/from the buffers of the cpu port. Not sure if there are even > > buffers, or if it is a direct to DMA delivery. > > That makes it a lot simpler. It always plays the MAC side. So i > recommend you just hard code it no delay, and let the PHY add the > delays as needed. Sure thing. I saw that there are device tree properties that can be used to explicitly enable delays on the MAC side, so in case we ever need them we can implement them (e.g. a PHY that can't do delays). Best regards, Jonas
On Fri, May 23, 2025 at 11:08:55AM +0200, Jonas Gorski wrote: > On Tue, May 20, 2025 at 2:15 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > Without this change no mode/port works, since there is always either a > > > 0 ns delay or a 4 ns delay in the rx/tx paths (I assume, I have no > > > equipment to measure). > > > > > > With this change all modes/ports work. > > > > Which is wrong. > > > > > With "rgmii-id" the mac doesn't > > > configure any delays (and the phy does instead), with "rgmii" it's > > > vice versa, so there is always the expected 2 ns delay. Same for rxid > > > and txid. > > > > If you read the description of what these four modes mean, you should > > understand why only one should work. And given the most likely PCB > > design, the only mode that should work is rgmii-id. You would have to > > change the PCB design, to make the other modes work. > > Since I also have BCM6368 with a BCM53115 connected to one of the > RGMII ports lying around, I played around with it, and it was > surprisingly hard to make it *not* work. Only if I enabled delay on > *both* sides it stopped working, no delay or delay only on one side > continued working (and I used iperf to try if larger amounts of > traffic break it). Interesting. You see the Rockchip people insisting their devices need fine tuning, 2ns is not good enough, it needs to be 1.9ns. And then they end up in a mess with interpreting what phy-mode actually means. In some ways, this just as bad. You can get away with using 'rgmii' which is wrong, when it should be 'rgmii-id'. It would be better if only just one mode worked, then DT developers would get it correct. Andrew
© 2016 - 2025 Red Hat, Inc.