Re: [PATCH v2 2/2] net: phy: realtek: Change TX-delay setting for RGMII modes only

[Vladimir Oltean]

On Mon, 29 Apr 2019 at 20:39, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
>
> On 4/26/19 4:35 PM, Serge Semin wrote:
> > On Fri, Apr 26, 2019 at 11:46:31PM +0200, Andrew Lunn wrote:
> >> On Sat, Apr 27, 2019 at 12:21:12AM +0300, Serge Semin wrote:
> >>> It's prone to problems if delay is cleared out for other than RGMII
> >>> modes. So lets set/clear the TX-delay in the config register only
> >>> if actually RGMII-like interface mode is requested.
> >>>
> >>> Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> >>>
> >>> ---
> >>>  drivers/net/phy/realtek.c | 16 ++++++++++++----
> >>>  1 file changed, 12 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> >>> index ab567a1923ad..a18cb01158f9 100644
> >>> --- a/drivers/net/phy/realtek.c
> >>> +++ b/drivers/net/phy/realtek.c
> >>> @@ -163,16 +163,24 @@ static int rtl8211c_config_init(struct phy_device *phydev)
> >>>  static int rtl8211f_config_init(struct phy_device *phydev)
> >>>  {
> >>>     int ret;
> >>> -   u16 val = 0;
> >>> +   u16 val;
> >>>
> >>>     ret = genphy_config_init(phydev);
> >>>     if (ret < 0)
> >>>             return ret;
> >>>
> >>> -   /* enable TX-delay for rgmii-id and rgmii-txid, otherwise disable it */
> >>> -   if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> >>> -       phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> >>> +   /* enable TX-delay for rgmii-id/rgmii-txid, and disable it for rgmii */
> >>> +   switch (phydev->interface) {
> >>> +   case PHY_INTERFACE_MODE_RGMII:
> >>> +           val = 0;
> >>> +           break;
> >>> +   case PHY_INTERFACE_MODE_RGMII_ID:
> >>> +   case PHY_INTERFACE_MODE_RGMII_TXID:
> >>>             val = RTL8211F_TX_DELAY;
> >>> +           break;
> >>> +   default: /* the rest of the modes imply leaving delay as is. */
> >>> +           return 0;
> >>> +   }
> >>
> >> So there is no control of the RX delay?
> >>
> >
> > As you can see it hasn't been there even before this change. So I suppose
> > either the hardware just doesn't support it (although the openly available
> > datasheet states that there is an RXD pin) or the original driver developer
> > decided to set TX-delay only.
> >
> > Just to make sure you understand. I am not working for realtek and don't
> > posses any inside info regarding these PHYs. I was working on a project,
> > which happened to utilize a rtl8211e PHY. We needed to find a way to
> > programmatically change the delays setting. So I searched the Internet
> > and found the U-boot rtl8211f driver and freebsd-folks discussion. This
> > info has been used to write the config_init method for Linux version of the
> > PHY' driver. That's it.
> >
> >> That means PHY_INTERFACE_MODE_RGMII_ID and
> >> PHY_INTERFACE_MODE_RGMII_RXID are not supported, and you should return
> >> -EINVAL.
> >>
> >
> > Apparently the current config_init method doesn't support RXID setting.
> > The patch introduced current function code was submitted by
> > Martin Blumenstingl in 2016:
> > https://patchwork.kernel.org/patch/9447581/
> > and was reviewed by Florian. So we'd better ask him why it was ok to mark
> > the RGMII_ID as supported while only TX-delay could be set.
> > I also failed to find anything regarding programmatic rtl8211f delays setting
> > in the Internet. So at this point we can set TX-delay only for f-model of the PHY.
> >
> > Anyway lets clarify the situation before to proceed further. You are suggesting
> > to return an error in case if either RGMII_ID or RGMII_RXID interface mode is
> > requested to be enabled for the PHY. It's fair seeing the driver can't fully
> > support either of them.
>
> That is how I read Andrew's suggestion and it is reasonable. WRT to the
> original changes from Martin, he is probably the one you would want to
> add to this conversation in case there are any RX delay control knobs
> available, I certainly don't have the datasheet, and Martin's change
> looks and looked reasonable, seemingly independent of the direction of
> this very conversation we are having.
>
> But what about the rest of the modes like GMII, MII
> > and others?
>
> The delays should be largely irrelevant for GMII and MII, since a) the
> PCB is required to have matching length traces, and b) these are not
> double data rate interfaces
>
> > Shouldn't we also return an error instead of leaving a default
> > delay value?
>
> That seems a bit harsh, those could have been configured by firmware,
> whatever before Linux comes up and be correct and valid. We don't know
> of a way to configure it, but that does not mean it does not exist and
> some software is doing it already.
>
> >
> > The same question can be actually asked regarding the config_init method of
> > rtl8211e PHY, which BTW you already tagged as Reviewed-by.
> >
> >> This is where we get into interesting backwards compatibility
> >> issues. Are there any broken DT blobs with rgmii-id or rgmii-rxid,
> >> which will break with such a change?
> >>
> >
> > Not that I am aware of and which simple grep rtl8211 could find. Do you
> > know about one?
> >
> > -Sergey
> >
> >>      Andrew
>
>
> --
> Florian

There seems to be some confusion here.
The "normal" RTL8211F has RXDLY and TXDLY configurable only via pin
strapping (pull-up/pull-down), not via MDIO.
The "1588-capable" RTL8211FS has RXDLY configurable via pin strapping
(different pin than the regular 8211F) and TXDLY via page 0xd08,
register 17, bit 8.
I think setting the Tx delay via MDIO for the normal RTL8211F is snake oil.
Disclaimer: I don't work for Realtek either, so I have no insight on
why it is like that.
>From Linux' point of view, there are two aspects:
* Erroring out now will likely just break something that was working
(since it was relying on hardware strapping and the DT phy-mode
property was more or less informative).
* Arguably what is wrong here is the semantics of the phy-mode
bindings for RGMII. It gets said a lot that DT means "hardware
description", not "hardware configuration". So having said that, the
correct interpretation of phy-mode = "rgmii-id" is that the operating
system is informed that RGMII delays were handled in both directions
(either the PHY was strapped, or PCB traces were lengthened). But the
current meaning of "rgmii-id" in practice is an imperative "PHY
driver, please apply delays in both directions" (or MAC driver, if
it's fixed-link).

Thanks,
-Vladimir