[PATCH] net: renesas: Fix rgmii-id delays

[PATCH] net: renesas: Fix rgmii-id delays

[Kory Maincent]

Invert the configuration of the RGMII delay selected by RGMII_RXID and
RGMII_TXID.

The ravb MAC is adding RX delay if RGMII_RXID is selected and TX delay
if RGMII_TXID but that behavior is wrong.
Indeed according to the ethernet.txt documentation the ravb configuration
should be inverted:
  * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
     should not add an RX delay in this case)
  * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
     should not add an TX delay in this case)

This patch inverts the behavior, i.e adds TX delay when RGMII_RXID is
selected and RX delay when RGMII_TXID is selected.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 0f85f2d97b18..89cd88e5b450 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2114,13 +2114,13 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
 	/* Fall back to legacy rgmii-*id behavior */
 	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
 	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID) {
-		priv->rxcidm = 1;
+		priv->txcidm = 1;
 		priv->rgmii_override = 1;
 	}
 
 	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
 	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) {
-		priv->txcidm = 1;
+		priv->rxcidm = 1;
 		priv->rgmii_override = 1;
 	}
 }
-- 
2.25.1

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Geert Uytterhoeven]

Hi Kory,

Thanks for your patch!

On Tue, Oct 19, 2021 at 4:57 PM Kory Maincent <kory.maincent@bootlin.com> wrote:
> Invert the configuration of the RGMII delay selected by RGMII_RXID and
> RGMII_TXID.
>
> The ravb MAC is adding RX delay if RGMII_RXID is selected and TX delay
> if RGMII_TXID but that behavior is wrong.
> Indeed according to the ethernet.txt documentation the ravb configuration

Do you mean ethernet-controller.yaml?

> should be inverted:
>   * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
>      should not add an RX delay in this case)
>   * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
>      should not add an TX delay in this case)
>
> This patch inverts the behavior, i.e adds TX delay when RGMII_RXID is
> selected and RX delay when RGMII_TXID is selected.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Does this fix an actual problem for you?

> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2114,13 +2114,13 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
>         /* Fall back to legacy rgmii-*id behavior */

Note that in accordance with the comment above, the code section
below is only present to support old DTBs.  Contemporary DTBs rely
on the now mandatory "rx-internal-delay-ps" and "tx-internal-delay-ps"
properties instead.
Hence changing this code has no effect on DTS files as supplied with
the kernel, but may have ill effects on DTB files in the field, which
rely on the current behavior.

>         if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
>             priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> -               priv->rxcidm = 1;
> +               priv->txcidm = 1;
>                 priv->rgmii_override = 1;
>         }
>
>         if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
>             priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> -               priv->txcidm = 1;
> +               priv->rxcidm = 1;
>                 priv->rgmii_override = 1;
>         }
>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Köry Maincent]

Hello Geert,

Thanks for the review.

On Tue, 19 Oct 2021 17:13:52 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > The ravb MAC is adding RX delay if RGMII_RXID is selected and TX delay
> > if RGMII_TXID but that behavior is wrong.
> > Indeed according to the ethernet.txt documentation the ravb configuration  
> 
> Do you mean ethernet-controller.yaml?

Doh, yes, I paste the commit log from the older Kernel git and forget to
change that.

> 
> > should be inverted:
> >   * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
> >      should not add an RX delay in this case)
> >   * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
> >      should not add an TX delay in this case)
> >
> > This patch inverts the behavior, i.e adds TX delay when RGMII_RXID is
> > selected and RX delay when RGMII_TXID is selected.
> >
> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>  
> 
> Does this fix an actual problem for you?

In fact, this fix a problem for an older 4.14 Kernel on my current project.
I wanted to push my fix in the mainline kernel also, but as you say below, now
this code is legacy.
Does it matter to fix legacy code?

> Note that in accordance with the comment above, the code section
> below is only present to support old DTBs.  Contemporary DTBs rely
> on the now mandatory "rx-internal-delay-ps" and "tx-internal-delay-ps"
> properties instead.
> Hence changing this code has no effect on DTS files as supplied with
> the kernel, but may have ill effects on DTB files in the field, which
> rely on the current behavior.

When people update the kernel version don't they update also the devicetree?

Regards,
Köry

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Andrew Lunn]

> When people update the kernel version don't they update also the devicetree?

DT is ABI. Driver writers should not break old blobs running on new
kernels. Often the DT blob is updated with the kernel, but it is not
required. It could be stored in a hard to reach place, shared with
u-boot etc.

	Andrew

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Thomas Petazzoni]

On Tue, 19 Oct 2021 17:41:49 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > When people update the kernel version don't they update also the devicetree?  
> 
> DT is ABI. Driver writers should not break old blobs running on new
> kernels. Often the DT blob is updated with the kernel, but it is not
> required. It could be stored in a hard to reach place, shared with
> u-boot etc.

Right, but conversely if someone reads the DT bindings that exists
today, specifies phy-mode = "rgmii-rxid" or phy-mmode = "rmgii-txid",
this person will get incorrect behavior. Sure a behavior that is
backward compatible with older DTs, but a terribly wrong one when you
write a new DT and read the DT binding documentation. This is exactly
the problem that happened to us.

I know that those properties are considered obsolete, but even though
they are considered as such, they are still supported, but for this
particular MAC driver, with an inverted meaning compared to what the DT
binding documentation says.

What wins: DT ABI backward compatibility, or correctness of the DT
binding ? :-)

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Geert Uytterhoeven]

Hi Thomas,

On Tue, Oct 19, 2021 at 5:57 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
> On Tue, 19 Oct 2021 17:41:49 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> > > When people update the kernel version don't they update also the devicetree?
> >
> > DT is ABI. Driver writers should not break old blobs running on new
> > kernels. Often the DT blob is updated with the kernel, but it is not
> > required. It could be stored in a hard to reach place, shared with
> > u-boot etc.
>
> Right, but conversely if someone reads the DT bindings that exists
> today, specifies phy-mode = "rgmii-rxid" or phy-mmode = "rmgii-txid",

Today == v5.10-rc1 and later?

> this person will get incorrect behavior. Sure a behavior that is
> backward compatible with older DTs, but a terribly wrong one when you
> write a new DT and read the DT binding documentation. This is exactly
> the problem that happened to us.

If you write a new DT, you read the DT binding documentation, and
"make dtbs_check" will inform you politely if you use the legacy/wrong
DT (i.e. lacking "[rt]x-internal-delay-ps")?

> I know that those properties are considered obsolete, but even though
> they are considered as such, they are still supported, but for this
> particular MAC driver, with an inverted meaning compared to what the DT
> binding documentation says.
>
> What wins: DT ABI backward compatibility, or correctness of the DT
> binding ? :-)

Both ;-)

The current driver is backwards-compatible with the legacy/wrong DTB.
The current DT bindings (as of v5.10-rc1), using "[rt]x-internal-delay-ps"
are correct.
Or am I missing something here?

BTW, it's still not clear to me why the inversion would be needed.
Cfr. Andrew's comment:

| So with rgmii-rxid, what is actually passed to the PHY? Is your
| problem you get twice the delay in one direction, and no delay in the
| other?

We know the ravb driver misbehaved in the past by applying the
rgmii-*id values to the MAC, while they are meant for the PHY, thus
causing bad interaction with PHY drivers.  But that was fixed
by commit 9b23203c32ee02cd ("ravb: Mask PHY mode to avoid inserting
delays twice") and a6f51f2efa742df0 ("ravb: Add support for explicit
internal clock delay configuration").

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Köry Maincent]

Hello Geert, Thomas

On Tue, 19 Oct 2021 22:05:41 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Thomas,
> 
> On Tue, Oct 19, 2021 at 5:57 PM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
> > On Tue, 19 Oct 2021 17:41:49 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:  
> > > > When people update the kernel version don't they update also the
> > > > devicetree?  
> > >
> > > DT is ABI. Driver writers should not break old blobs running on new
> > > kernels. Often the DT blob is updated with the kernel, but it is not
> > > required. It could be stored in a hard to reach place, shared with
> > > u-boot etc.  
> >
> > Right, but conversely if someone reads the DT bindings that exists
> > today, specifies phy-mode = "rgmii-rxid" or phy-mmode = "rmgii-txid",  
> 
> Today == v5.10-rc1 and later?
> 
> > this person will get incorrect behavior. Sure a behavior that is
> > backward compatible with older DTs, but a terribly wrong one when you
> > write a new DT and read the DT binding documentation. This is exactly
> > the problem that happened to us.  
> 
> If you write a new DT, you read the DT binding documentation, and
> "make dtbs_check" will inform you politely if you use the legacy/wrong
> DT (i.e. lacking "[rt]x-internal-delay-ps")?

Indeed this command will inform the missing required "[rt]x-internal-delay-ps".
I am not use to that command, as it seems to check all the devicetree each time
where I want only to check one.

> 
> The current driver is backwards-compatible with the legacy/wrong DTB.
> The current DT bindings (as of v5.10-rc1), using "[rt]x-internal-delay-ps"
> are correct.
> Or am I missing something here?

You are correct.

> 
> BTW, it's still not clear to me why the inversion would be needed.
> Cfr. Andrew's comment:
> 
> | So with rgmii-rxid, what is actually passed to the PHY? Is your
> | problem you get twice the delay in one direction, and no delay in the
> | other?

Yes, it was the problem I got.
The PHY I use have RX delay enabled by default, currently the PHY driver does
not support delay configuration, therefore I let the MAC handle TX delay. I
have stumbling over that legacy/wrong DTS on the old Kernel.

Regards,
Köry

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Geert Uytterhoeven]

Hi Köry,

On Wed, Oct 20, 2021 at 10:53 AM Köry Maincent
<kory.maincent@bootlin.com> wrote:
> On Tue, 19 Oct 2021 22:05:41 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > The current driver is backwards-compatible with the legacy/wrong DTB.
> > The current DT bindings (as of v5.10-rc1), using "[rt]x-internal-delay-ps"
> > are correct.
> > Or am I missing something here?
>
> You are correct.

Thanks for confirming!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Andrew Lunn]

> > BTW, it's still not clear to me why the inversion would be needed.
> > Cfr. Andrew's comment:
> > 
> > | So with rgmii-rxid, what is actually passed to the PHY? Is your
> > | problem you get twice the delay in one direction, and no delay in the
> > | other?
> 
> Yes, it was the problem I got.
> The PHY I use have RX delay enabled by default, currently the PHY driver does
> not support delay configuration, therefore I let the MAC handle TX delay. I
> have stumbling over that legacy/wrong DTS on the old Kernel.

This is where we get into the horrible mess of RGMII delays.

The real solution is to fix the PHY driver. If it is asked to do
rgmii, but is actually doing rgmii-id, the PHY driver is broken. It
either should do what it is told, or return -EINVAL/-EOPNOTSUPP etc to
indicate it does not support what it is asked to do.

But fixing things like this often breaks working systems, because the
DT says rgmii, the PHY actually does rgmii-id, the board works, until
the PHY is fixed to do what it is told, and all the bugs in the DT
suddenly come to light.

Now, you said you are using an old kernel. So it could be we have
already fixed this, and had the pain of fixing broken DT. Please look
at the current kernel PHY driver, and see if all you need to do is
back port some PHY fixes, or better still, throw away your old kernel
and use a modern one.

       Andrew

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Geert Uytterhoeven]

Hi Köry,

On Tue, Oct 19, 2021 at 5:35 PM Köry Maincent <kory.maincent@bootlin.com> wrote:
> On Tue, 19 Oct 2021 17:13:52 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > The ravb MAC is adding RX delay if RGMII_RXID is selected and TX delay
> > > if RGMII_TXID but that behavior is wrong.
> > > Indeed according to the ethernet.txt documentation the ravb configuration
> > > should be inverted:
> > >   * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
> > >      should not add an RX delay in this case)
> > >   * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
> > >      should not add an TX delay in this case)
> > >
> > > This patch inverts the behavior, i.e adds TX delay when RGMII_RXID is
> > > selected and RX delay when RGMII_TXID is selected.
> > >
> > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> >
> > Does this fix an actual problem for you?
>
> In fact, this fix a problem for an older 4.14 Kernel on my current project.
> I wanted to push my fix in the mainline kernel also, but as you say below, now
> this code is legacy.
> Does it matter to fix legacy code?

I don't think so.  If you're stuck on v4.14, you may want to backport
commit a6f51f2efa742df0 ("ravb: Add support for explicit internal
clock delay configuration").  However, you have to be careful, as
it interacts with related changes to PHY drivers, as explained in
that commit.

> > Note that in accordance with the comment above, the code section
> > below is only present to support old DTBs.  Contemporary DTBs rely
> > on the now mandatory "rx-internal-delay-ps" and "tx-internal-delay-ps"
> > properties instead.
> > Hence changing this code has no effect on DTS files as supplied with
> > the kernel, but may have ill effects on DTB files in the field, which
> > rely on the current behavior.
>
> When people update the kernel version don't they update also the devicetree?

I hope they do ;-)
But we do our best to preserve backwards-compatibility with old DTBS.
If you change behavior of v4.14, it may actually introduce
backwards-incompatibility we're not aware of, as the behavior you
started to rely on never existed in mainline.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] net: renesas: Fix rgmii-id delays

[Andrew Lunn]

On Tue, Oct 19, 2021 at 04:57:17PM +0200, Kory Maincent wrote:
> Invert the configuration of the RGMII delay selected by RGMII_RXID and
> RGMII_TXID.
> 
> The ravb MAC is adding RX delay if RGMII_RXID is selected and TX delay
> if RGMII_TXID but that behavior is wrong.
> Indeed according to the ethernet.txt documentation the ravb configuration
> should be inverted:
>   * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
>      should not add an RX delay in this case)
>   * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
>      should not add an TX delay in this case)
> 
> This patch inverts the behavior, i.e adds TX delay when RGMII_RXID is
> selected and RX delay when RGMII_TXID is selected.

This gets messy. As a general rule of thumb, the MAC should not be
adding delays, the PHY should. ravb has historically ignored that, and
it adds delays. It then needs to be careful with what it passes to the
PHY.

So with rgmii-rxid, what is actually passed to the PHY? Is your
problem you get twice the delay in one direction, and no delay in the
other?

	Andrew