linux-kernel - Re: [PATCH RFC v1 1/4] dt-bindings: net: dwmac-meson: use picoseconds for the RGMII RX delay

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFBinCAmExHsCpP8hh_K6M6pg8SjoF1WNNj9fwSKR3dhgBGJdA@mail.gmail.com>
Date:   Sun, 15 Nov 2020 10:22:06 +0100
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, kuba@...nel.org,
        linux-amlogic@...ts.infradead.org, devicetree@...r.kernel.org,
        robh+dt@...nel.org, jianxin.pan@...ogic.com,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        khilman@...libre.com, Neil Armstrong <narmstrong@...libre.com>,
        jbrunet@...libre.com
Subject: Re: [PATCH RFC v1 1/4] dt-bindings: net: dwmac-meson: use picoseconds
 for the RGMII RX delay

Hi Andrew,

On Sat, Nov 14, 2020 at 11:32 PM Andrew Lunn <andrew@...n.ch> wrote:
[...]
> > +        amlogic,rgmii-rx-delay-ps:
> > +          default: 0
> >            description:
> >              The internal RGMII RX clock delay (provided by this IP block) in
> > -            nanoseconds. When phy-mode is set to "rgmii" then the RX delay
> > +            picoseconds. When phy-mode is set to "rgmii" then the RX delay
> >              should be explicitly configured. When the phy-mode is set to
> >              either "rgmii-id" or "rgmii-rxid" the RX clock delay is already
> >              provided by the PHY. Any configuration is ignored when the
> >              phy-mode is set to "rmii".
>
> Hi Martin
>
> I don't think the wording matches what the driver is actually doing:
>
>         if (dwmac->rx_delay_ns == 2)
>                 rx_dly_config = PRG_ETH0_ADJ_ENABLE | PRG_ETH0_ADJ_SETUP;
>         else
>                 rx_dly_config = 0;
>
>         switch (dwmac->phy_mode) {
>         case PHY_INTERFACE_MODE_RGMII:
>                 delay_config = tx_dly_config | rx_dly_config;
>                 break;
>         case PHY_INTERFACE_MODE_RGMII_RXID:
>                 delay_config = tx_dly_config;
>                 break;
>         case PHY_INTERFACE_MODE_RGMII_TXID:
>                 delay_config = rx_dly_config;
>                 break;
>         case PHY_INTERFACE_MODE_RGMII_ID:
>         case PHY_INTERFACE_MODE_RMII:
>                 delay_config = 0;
>                 break;
>
> So rx_delay is used for both rgmii and rgmii-txid. The binding says
> nothing about rgmii-txid.
interesting point here. it's been like this before this patch. still I
would like to understand what the proper way to fix it is so I can
also include a fix for it:
1. should rgmii-txid not add any RX delay on the MAC side? that would
mean for my board I will switch to phy-mode rgmii so the MAC applies
both the RX and TX delays
2. update the documentation to clarify that rgmii-txid would also add
the RX delay on the MAC side

> And while i'm looking at the code, i wonder about this:
>
>        if (rx_dly_config & PRG_ETH0_ADJ_ENABLE) {
>                 if (!dwmac->timing_adj_clk) {
>                         dev_err(dwmac->dev,
>                                 "The timing-adjustment clock is mandatory for the RX delay re-timing\n");
>                         return -EINVAL;
>                 }
>
>                 /* The timing adjustment logic is driven by a separate clock */
>                 ret = meson8b_devm_clk_prepare_enable(dwmac,
>                                                       dwmac->timing_adj_clk);
>                 if (ret) {
>                         dev_err(dwmac->dev,
>                                 "Failed to enable the timing-adjustment clock\n");
>                         return ret;
>                 }
>         }
>
> It looks like it can be that rx_dly_config & PRG_ETH0_ADJ_ENABLE is
> true, so the clock is enabled, but delay_config does not contain
> rx_delay_config, so it is pointless turning it on.
that is a good point and also a bug with one of the previous patches
I'll include a patch fixing this in v2


Best regards,
Martin