| Message ID | 20211019145719.122751-1-kory.maincent@bootlin.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: renesas: Fix rgmii-id delays | expand |
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
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
> 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
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
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
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
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
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
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
> > 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
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; } }
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(-)