netdev - Re: [PATCH] net: phy: realtek: Add rtl8211e rx/tx delays config

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: <20190426191935.ihppqlmtpemzp3kr@mobilestation>
Date:   Fri, 26 Apr 2019 22:19:36 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Serge Semin <Sergey.Semin@...latforms.ru>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: realtek: Add rtl8211e rx/tx delays config

On Fri, Apr 26, 2019 at 03:28:21PM +0200, Andrew Lunn wrote:
> On Fri, Apr 26, 2019 at 12:30:10PM +0300, Serge Semin wrote:
> > The hidden RGMII configs register utilization was found in the
> > rtl8211e U-boot driver:
> > https://elixir.bootlin.com/u-boot/v2019.01/source/drivers/net/phy/realtek.c#L99
> > 
> > It confirms that the register bits field must control the so called
> > configuration pins described in the table 12-13 of the official PHY
> > datasheet:
> > 8:6 = PHY Address
> > 5:4 = Auto-Negotiation
> > 3 = Interface Mode Select
> > 2 = RX Delay
> > 1 = TX Delay
> > 0 = SELRGV
> 
> > +static int rtl8211e_config_init(struct phy_device *phydev)
> > +{
> > +	int ret, oldpage;
> > +	u16 val = 0;
> > +
> > +	ret = genphy_config_init(phydev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* enable TX/RX delay for rgmii-* modes, otherwise disable it */
> > +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> > +		val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY;
> > +	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > +		val = RTL8211E_TX_DELAY;
> > +	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> > +		val = RTL8211E_RX_DELAY;
> 
> Hi Serge
> 
> You need to look for PHY_INTERFACE_MODE_RGMII and disable the bits.
> For any other value, and in particular PHY_INTERFACE_MODE_NA, you
> should leave the bits alone.
> 
> As you found out, u-boot knows how to program these bits. There are
> probably boards out there which rely on u-boot doing this, and the PHY
> driver then not messing with the bits. The way to indicate it should
> not mess with the bits is to not have a phy-mode property in DT, which
> results in PHY_INTERFACE_MODE_NA.
> 

Hello Andrew

Thanks for the comment. I'll alter the code the way you said. The mode will
be changed by the config_init-function only if the interface is selected to be
rgmii-like (rgmii, rgmii-id, rgmii-txid and rgmii-rxid), otherwise it will
be left as is.

But I've got a question regarding this then. What about for instance rtl8211f
phy config_init-method? It setups the delay config in any case, no matter
whether interface is configured to be of rgmii or another mode. Is it correct
to configure rtl8211e and rtl8211f differently? Especially seeing the U-boot
driver also perform the rtl8211f phy configuration.

-Sergey

> 	Andrew