Messages in this thread | | | From | Vladimir Oltean <> | Date | Mon, 29 Apr 2019 21:29:37 +0300 | Subject | Re: [PATCH v2 2/2] net: phy: realtek: Change TX-delay setting for RGMII modes only |
| |
On Mon, 29 Apr 2019 at 20:39, Florian Fainelli <f.fainelli@gmail.com> 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@gmail.com> > >>> > >>> --- > >>> 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
| |