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

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

[Serge Semin]

There are two chip pins named TXDLY and RXDLY which actually addes
the 2ns delays to TXC and RXC for TXD/RXD latching. Alas it is only
documented info regarding the RGMII timing control configurations
the PHY provides. It turnes out the same settings can be setup
via MDIO registers hidden in the extension pages layout.
Particularly the extension page 0xa4 provides a register 0x1c,
which bits 1 and 2 control the described delays. They are used
to implemet the "rgmii-{id,rxid,txid}" phy-mode.

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

There is also a freebsd discussion regarding this register:
https://reviews.freebsd.org/D13591

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

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/net/phy/realtek.c | 44 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 10df52ccddfe..8776b94d91ed 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -23,11 +23,14 @@
 
 #define RTL821x_INSR				0x13
 
+#define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
 #define RTL8211F_INSR				0x1d
 
 #define RTL8211F_TX_DELAY			BIT(8)
+#define RTL8211E_TX_DELAY			BIT(1)
+#define RTL8211E_RX_DELAY			BIT(2)
 
 #define RTL8201F_ISR				0x1e
 #define RTL8201F_IER				0x13
@@ -174,6 +177,46 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
 }
 
+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;
+
+	/* According to a sample driver there is a 0x1c config register on
+	 * a 0xa4 extension page (0x7) layout. It can be used to disable/enable
+	 * the RX/TX delays otherwise controlled by hardware strobes. It can
+	 * also be used to customize the whole configuration register:
+	 * 8:6 = PHY Address, 5:4 = Auto-Negotiation, 3 = Interface Mode Select,
+	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
+	 * for details).
+	 */
+	oldpage = phy_select_page(phydev, 0x7);
+	if (oldpage < 0)
+		goto err_restore_page;
+
+	ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
+	if (!ret)
+		goto err_restore_page;
+
+	ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
+			 val);
+
+err_restore_page:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8211b_suspend(struct phy_device *phydev)
 {
 	phy_write(phydev, MII_MMD_DATA, BIT(9));
@@ -257,6 +300,7 @@ static struct phy_driver realtek_drvs[] = {
 		PHY_ID_MATCH_EXACT(0x001cc915),
 		.name		= "RTL8211E Gigabit Ethernet",
 		.features	= PHY_GBIT_FEATURES,
+		.config_init	= &rtl8211e_config_init,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
 		.suspend	= genphy_suspend,
-- 
2.21.0

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

[Andrew Lunn]

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.

	Andrew

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

[Serge Semin]

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

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

[Andrew Lunn]

> 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.

Hi Sergey

It is not correct and rtl8211f should be changed. Please add a second
patch for that.

We got burnt recently with PHY drivers doing the wrong thing with
respect to RGMII delays. We have learnt from that experience, but
there is code in the kernel which still does things wrong.

      Andrew

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

[Serge Semin]

On Fri, Apr 26, 2019 at 10:05:21PM +0200, Andrew Lunn wrote:
> > 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.
> 
> Hi Sergey
> 
> It is not correct and rtl8211f should be changed. Please add a second
> patch for that.
> 
> We got burnt recently with PHY drivers doing the wrong thing with
> respect to RGMII delays. We have learnt from that experience, but
> there is code in the kernel which still does things wrong.
> 
>       Andrew

Ok. I'll send the updated patchset with rtl8211f_config_init fixed soon.

-Sergey

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

[Heiner Kallweit]

On 26.04.2019 11:30, Serge Semin wrote:
> There are two chip pins named TXDLY and RXDLY which actually addes
> the 2ns delays to TXC and RXC for TXD/RXD latching. Alas it is only
> documented info regarding the RGMII timing control configurations
> the PHY provides. It turnes out the same settings can be setup
> via MDIO registers hidden in the extension pages layout.
> Particularly the extension page 0xa4 provides a register 0x1c,
> which bits 1 and 2 control the described delays. They are used
> to implemet the "rgmii-{id,rxid,txid}" phy-mode.
> 
There are few typos in the commit message.

> 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
> 
> There is also a freebsd discussion regarding this register:
> https://reviews.freebsd.org/D13591
> 
> 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
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/net/phy/realtek.c | 44 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 10df52ccddfe..8776b94d91ed 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -23,11 +23,14 @@
>  
>  #define RTL821x_INSR				0x13
>  
> +#define RTL821x_EXT_PAGE_SELECT			0x1e
>  #define RTL821x_PAGE_SELECT			0x1f
>  
>  #define RTL8211F_INSR				0x1d
>  
>  #define RTL8211F_TX_DELAY			BIT(8)
> +#define RTL8211E_TX_DELAY			BIT(1)
> +#define RTL8211E_RX_DELAY			BIT(2)
>  
>  #define RTL8201F_ISR				0x1e
>  #define RTL8201F_IER				0x13
> @@ -174,6 +177,46 @@ static int rtl8211f_config_init(struct phy_device *phydev)
>  	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
>  }
>  
> +static int rtl8211e_config_init(struct phy_device *phydev)
> +{
> +	int ret, oldpage;
> +	u16 val = 0;
> +
> +	ret = genphy_config_init(phydev);

There's no need to call genphy_config_init().

> +	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;
> +
> +	/* According to a sample driver there is a 0x1c config register on
> +	 * a 0xa4 extension page (0x7) layout. It can be used to disable/enable
> +	 * the RX/TX delays otherwise controlled by hardware strobes. It can
> +	 * also be used to customize the whole configuration register:
> +	 * 8:6 = PHY Address, 5:4 = Auto-Negotiation, 3 = Interface Mode Select,
> +	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
> +	 * for details).
> +	 */
> +	oldpage = phy_select_page(phydev, 0x7);
> +	if (oldpage < 0)
> +		goto err_restore_page;
> +
> +	ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
> +	if (!ret)

I think this should be: if (ret)

> +		goto err_restore_page;
> +
> +	ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
> +			 val);
> +
> +err_restore_page:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
>  static int rtl8211b_suspend(struct phy_device *phydev)
>  {
>  	phy_write(phydev, MII_MMD_DATA, BIT(9));
> @@ -257,6 +300,7 @@ static struct phy_driver realtek_drvs[] = {
>  		PHY_ID_MATCH_EXACT(0x001cc915),
>  		.name		= "RTL8211E Gigabit Ethernet",
>  		.features	= PHY_GBIT_FEATURES,
> +		.config_init	= &rtl8211e_config_init,
>  		.ack_interrupt	= &rtl821x_ack_interrupt,
>  		.config_intr	= &rtl8211e_config_intr,
>  		.suspend	= genphy_suspend,
> 

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

[Serge Semin]

On Fri, Apr 26, 2019 at 07:17:44PM +0200, Heiner Kallweit wrote:
> On 26.04.2019 11:30, Serge Semin wrote:
> > There are two chip pins named TXDLY and RXDLY which actually addes
> > the 2ns delays to TXC and RXC for TXD/RXD latching. Alas it is only
> > documented info regarding the RGMII timing control configurations
> > the PHY provides. It turnes out the same settings can be setup
> > via MDIO registers hidden in the extension pages layout.
> > Particularly the extension page 0xa4 provides a register 0x1c,
> > which bits 1 and 2 control the described delays. They are used
> > to implemet the "rgmii-{id,rxid,txid}" phy-mode.
> > 
> There are few typos in the commit message.
> 

I'll fix them in v2. Thank you.

> > 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
> > 
> > There is also a freebsd discussion regarding this register:
> > https://reviews.freebsd.org/D13591
> > 
> > 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
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  drivers/net/phy/realtek.c | 44 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index 10df52ccddfe..8776b94d91ed 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -23,11 +23,14 @@
> >  
> >  #define RTL821x_INSR				0x13
> >  
> > +#define RTL821x_EXT_PAGE_SELECT			0x1e
> >  #define RTL821x_PAGE_SELECT			0x1f
> >  
> >  #define RTL8211F_INSR				0x1d
> >  
> >  #define RTL8211F_TX_DELAY			BIT(8)
> > +#define RTL8211E_TX_DELAY			BIT(1)
> > +#define RTL8211E_RX_DELAY			BIT(2)
> >  
> >  #define RTL8201F_ISR				0x1e
> >  #define RTL8201F_IER				0x13
> > @@ -174,6 +177,46 @@ static int rtl8211f_config_init(struct phy_device *phydev)
> >  	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
> >  }
> >  
> > +static int rtl8211e_config_init(struct phy_device *phydev)
> > +{
> > +	int ret, oldpage;
> > +	u16 val = 0;
> > +
> > +	ret = genphy_config_init(phydev);
> 
> There's no need to call genphy_config_init().
> 

Right. I'll fix it in v2. It's working even with calling the function though.

> > +	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;
> > +
> > +	/* According to a sample driver there is a 0x1c config register on
> > +	 * a 0xa4 extension page (0x7) layout. It can be used to disable/enable
> > +	 * the RX/TX delays otherwise controlled by hardware strobes. It can
> > +	 * also be used to customize the whole configuration register:
> > +	 * 8:6 = PHY Address, 5:4 = Auto-Negotiation, 3 = Interface Mode Select,
> > +	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
> > +	 * for details).
> > +	 */
> > +	oldpage = phy_select_page(phydev, 0x7);
> > +	if (oldpage < 0)
> > +		goto err_restore_page;
> > +
> > +	ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
> > +	if (!ret)
> 
> I think this should be: if (ret)
> 

My goodness. What a shame on me to miss this! I ported this function from an older
kernel, and apparently didn't do this accurate enough. Sorry for this awful
bug. I'll fix it in the next patch revision.

-Sergey

> > +		goto err_restore_page;
> > +
> > +	ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
> > +			 val);
> > +
> > +err_restore_page:
> > +	return phy_restore_page(phydev, oldpage, ret);
> > +}
> > +
> >  static int rtl8211b_suspend(struct phy_device *phydev)
> >  {
> >  	phy_write(phydev, MII_MMD_DATA, BIT(9));
> > @@ -257,6 +300,7 @@ static struct phy_driver realtek_drvs[] = {
> >  		PHY_ID_MATCH_EXACT(0x001cc915),
> >  		.name		= "RTL8211E Gigabit Ethernet",
> >  		.features	= PHY_GBIT_FEATURES,
> > +		.config_init	= &rtl8211e_config_init,
> >  		.ack_interrupt	= &rtl821x_ack_interrupt,
> >  		.config_intr	= &rtl8211e_config_intr,
> >  		.suspend	= genphy_suspend,
> > 
> 

[PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Serge Semin]

There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
delays to TXC and RXC for TXD/RXD latching. Alas this is the only
documented info regarding the RGMII timing control configurations the PHY
provides. It turns out the same settings can be setup via MDIO registers
hidden in the extension pages layout. Particularly the extension page 0xa4
provides a register 0x1c, which bits 1 and 2 control the described delays.
They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.

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

There is also a freebsd-folks discussion regarding this register:
https://reviews.freebsd.org/D13591

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

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---
Changelog v2
- Disable delays for rgmii mode and leave them as is for the rest of
  the modes.
- Remove genphy_config_init() invocation. It's redundant for rtl8211e phy.
- Fix confused return value checking of extended-page selector call.
- Fix commit message typos.

---
 drivers/net/phy/realtek.c | 50 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 10df52ccddfe..ab567a1923ad 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -23,11 +23,14 @@
 
 #define RTL821x_INSR				0x13
 
+#define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
 #define RTL8211F_INSR				0x1d
 
 #define RTL8211F_TX_DELAY			BIT(8)
+#define RTL8211E_TX_DELAY			BIT(1)
+#define RTL8211E_RX_DELAY			BIT(2)
 
 #define RTL8201F_ISR				0x1e
 #define RTL8201F_IER				0x13
@@ -174,6 +177,52 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
 }
 
+static int rtl8211e_config_init(struct phy_device *phydev)
+{
+	int ret, oldpage;
+	u16 val;
+
+	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		val = 0;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		val = RTL8211E_RX_DELAY;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val = RTL8211E_TX_DELAY;
+		break;
+	default: /* the rest of the modes imply leaving delays as is. */
+		return 0;
+	}
+
+	/* According to a sample driver there is a 0x1c config register on the
+	 * 0xa4 extension page (0x7) layout. It can be used to disable/enable
+	 * the RX/TX delays otherwise controlled by hardware strobes. It can
+	 * also be used to customize the whole configuration register:
+	 * 8:6 = PHY Address, 5:4 = Auto-Negotiation, 3 = Interface Mode Select,
+	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
+	 * for details).
+	 */
+	oldpage = phy_select_page(phydev, 0x7);
+	if (oldpage < 0)
+		goto err_restore_page;
+
+	ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
+	if (ret)
+		goto err_restore_page;
+
+	ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
+			 val);
+
+err_restore_page:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8211b_suspend(struct phy_device *phydev)
 {
 	phy_write(phydev, MII_MMD_DATA, BIT(9));
@@ -257,6 +306,7 @@ static struct phy_driver realtek_drvs[] = {
 		PHY_ID_MATCH_EXACT(0x001cc915),
 		.name		= "RTL8211E Gigabit Ethernet",
 		.features	= PHY_GBIT_FEATURES,
+		.config_init	= &rtl8211e_config_init,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
 		.suspend	= genphy_suspend,
-- 
2.21.0

Re: [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Andrew Lunn]

On Sat, Apr 27, 2019 at 12:21:11AM +0300, Serge Semin wrote:
> There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
> delays to TXC and RXC for TXD/RXD latching. Alas this is the only
> documented info regarding the RGMII timing control configurations the PHY
> provides. It turns out the same settings can be setup via MDIO registers
> hidden in the extension pages layout. Particularly the extension page 0xa4
> provides a register 0x1c, which bits 1 and 2 control the described delays.
> They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.
> 
> 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
> 
> There is also a freebsd-folks discussion regarding this register:
> https://reviews.freebsd.org/D13591
> 
> 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
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>


Hi Serge

Next time please include a patch 0 containing a cover note explaining
the who series.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Serge Semin]

On Fri, Apr 26, 2019 at 11:40:50PM +0200, Andrew Lunn wrote:
> On Sat, Apr 27, 2019 at 12:21:11AM +0300, Serge Semin wrote:
> > There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
> > delays to TXC and RXC for TXD/RXD latching. Alas this is the only
> > documented info regarding the RGMII timing control configurations the PHY
> > provides. It turns out the same settings can be setup via MDIO registers
> > hidden in the extension pages layout. Particularly the extension page 0xa4
> > provides a register 0x1c, which bits 1 and 2 control the described delays.
> > They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.
> > 
> > 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
> > 
> > There is also a freebsd-folks discussion regarding this register:
> > https://reviews.freebsd.org/D13591
> > 
> > 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
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> 
> 
> Hi Serge
> 
> Next time please include a patch 0 containing a cover note explaining
> the who series.
> 

Sure as long as the patchset gets to be much bigger than two small
patches with an obvious reason to be merged.

> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 

Thanks.)

-Sergey

>     Andrew

Re: [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Florian Fainelli]

On 4/26/2019 4:45 PM, Serge Semin wrote:
> On Fri, Apr 26, 2019 at 11:40:50PM +0200, Andrew Lunn wrote:
>> On Sat, Apr 27, 2019 at 12:21:11AM +0300, Serge Semin wrote:
>>> There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
>>> delays to TXC and RXC for TXD/RXD latching. Alas this is the only
>>> documented info regarding the RGMII timing control configurations the PHY
>>> provides. It turns out the same settings can be setup via MDIO registers
>>> hidden in the extension pages layout. Particularly the extension page 0xa4
>>> provides a register 0x1c, which bits 1 and 2 control the described delays.
>>> They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.
>>>
>>> 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
>>>
>>> There is also a freebsd-folks discussion regarding this register:
>>> https://reviews.freebsd.org/D13591
>>>
>>> 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
>>>
>>> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
>>
>>
>> Hi Serge
>>
>> Next time please include a patch 0 containing a cover note explaining
>> the who series.
>>
> 
> Sure as long as the patchset gets to be much bigger than two small
> patches with an obvious reason to be merged.

netdev likes to have a cover letter for patch count >= 1, probably
something to be added to netdev-FAQ.rst.
-- 
Florian

Re: [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Serge Semin]

On Fri, Apr 26, 2019 at 08:11:50PM -0700, Florian Fainelli wrote:
> 
> 
> On 4/26/2019 4:45 PM, Serge Semin wrote:
> > On Fri, Apr 26, 2019 at 11:40:50PM +0200, Andrew Lunn wrote:
> >> On Sat, Apr 27, 2019 at 12:21:11AM +0300, Serge Semin wrote:
> >>> There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
> >>> delays to TXC and RXC for TXD/RXD latching. Alas this is the only
> >>> documented info regarding the RGMII timing control configurations the PHY
> >>> provides. It turns out the same settings can be setup via MDIO registers
> >>> hidden in the extension pages layout. Particularly the extension page 0xa4
> >>> provides a register 0x1c, which bits 1 and 2 control the described delays.
> >>> They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.
> >>>
> >>> 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
> >>>
> >>> There is also a freebsd-folks discussion regarding this register:
> >>> https://reviews.freebsd.org/D13591
> >>>
> >>> 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
> >>>
> >>> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> >>
> >>
> >> Hi Serge
> >>
> >> Next time please include a patch 0 containing a cover note explaining
> >> the who series.
> >>
> > 
> > Sure as long as the patchset gets to be much bigger than two small
> > patches with an obvious reason to be merged.
> 
> netdev likes to have a cover letter for patch count >= 1, probably
> something to be added to netdev-FAQ.rst.
> -- 
> Florian

Hello Florian
Really, even with count = 1? So just one patch with cover-letter? Doesn't it
seem redundant since at least a single patch can be thoroughly described in
it' commit message?

-Sergey

Re: [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Andrew Lunn]

> > >> Hi Serge
> > >>
> > >> Next time please include a patch 0 containing a cover note explaining
> > >> the who series.
> > >>
> > > 
> > > Sure as long as the patchset gets to be much bigger than two small
> > > patches with an obvious reason to be merged.
> > 
> > netdev likes to have a cover letter for patch count >= 1, probably
> > something to be added to netdev-FAQ.rst.
> > -- 
> > Florian
> 
> Hello Florian
> Really, even with count = 1? So just one patch with cover-letter? Doesn't it
> seem redundant since at least a single patch can be thoroughly described in
> it' commit message?

Hi Serge

David workflow is to put the patch set into a branch, and then merge
the branch into his master, using the cover note as the merge commit
message.

You occasionally see commit messages for count == 1, but not often.
For > 1, if there is no cover note, somebody is likely to ask for one
:-)

    Andrew

Re: [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Serge Semin]

On Sat, Apr 27, 2019 at 05:21:09PM +0200, Andrew Lunn wrote:
> > > >> Hi Serge
> > > >>
> > > >> Next time please include a patch 0 containing a cover note explaining
> > > >> the who series.
> > > >>
> > > > 
> > > > Sure as long as the patchset gets to be much bigger than two small
> > > > patches with an obvious reason to be merged.
> > > 
> > > netdev likes to have a cover letter for patch count >= 1, probably
> > > something to be added to netdev-FAQ.rst.
> > > -- 
> > > Florian
> > 
> > Hello Florian
> > Really, even with count = 1? So just one patch with cover-letter? Doesn't it
> > seem redundant since at least a single patch can be thoroughly described in
> > it' commit message?
> 
> Hi Serge
> 
> David workflow is to put the patch set into a branch, and then merge
> the branch into his master, using the cover note as the merge commit
> message.
> 
> You occasionally see commit messages for count == 1, but not often.
> For > 1, if there is no cover note, somebody is likely to ask for one
> :-)
> 
>     Andrew

Hello Andrew and Florian

Alright. I see your point. Though I am not really agree with it at this
situation, the next version of the mini-patchset will be sent with a
cover-letter as per maintainers request.

Meanwhile lets put aside this discussion and get back to the topic-related
one. Could you respond to the email I've sent in response to the Andrew's
comment on the second patch of v2 duet?

-Sergey

Re: [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Florian Fainelli]

On 4/27/2019 12:44 AM, Serge Semin wrote:
> On Fri, Apr 26, 2019 at 08:11:50PM -0700, Florian Fainelli wrote:
>>
>>
>> On 4/26/2019 4:45 PM, Serge Semin wrote:
>>> On Fri, Apr 26, 2019 at 11:40:50PM +0200, Andrew Lunn wrote:
>>>> On Sat, Apr 27, 2019 at 12:21:11AM +0300, Serge Semin wrote:
>>>>> There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
>>>>> delays to TXC and RXC for TXD/RXD latching. Alas this is the only
>>>>> documented info regarding the RGMII timing control configurations the PHY
>>>>> provides. It turns out the same settings can be setup via MDIO registers
>>>>> hidden in the extension pages layout. Particularly the extension page 0xa4
>>>>> provides a register 0x1c, which bits 1 and 2 control the described delays.
>>>>> They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.
>>>>>
>>>>> 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
>>>>>
>>>>> There is also a freebsd-folks discussion regarding this register:
>>>>> https://reviews.freebsd.org/D13591
>>>>>
>>>>> 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
>>>>>
>>>>> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
>>>>
>>>>
>>>> Hi Serge
>>>>
>>>> Next time please include a patch 0 containing a cover note explaining
>>>> the who series.
>>>>
>>>
>>> Sure as long as the patchset gets to be much bigger than two small
>>> patches with an obvious reason to be merged.
>>
>> netdev likes to have a cover letter for patch count >= 1, probably
>> something to be added to netdev-FAQ.rst.
>> -- 
>> Florian
> 
> Hello Florian
> Really, even with count = 1? So just one patch with cover-letter? Doesn't it
> seem redundant since at least a single patch can be thoroughly described in
> it' commit message?

I was off by one, it's for count > 1 that a cover letter is usually
requested.
-- 
Florian

[PATCH v3 0/2] net: phy: realtek: Fix RGMII TX/RX-delays initial config of rtl8211(e|f)

[Serge Semin]

It has been discovered that RX/TX delays of rtl8211e ethernet PHY
can be configured via a MDIO register hidden in the extension pages
layout. Particularly the extension page 0xa4 provides a register 0x1c,
which bits 1 and 2 control the described delays. They are used to
implement the "rgmii-{id,rxid,txid}" phy-mode support in patch 1.

The second patch makes sure the rtl8211f TX-delay is configured only
if RGMII interface mode is specified including the rgmii-rxid one.
In other cases (most importantly for NA mode) the delays are supposed
to be preconfigured by some other software or hardware and should be
left as is without any modification. The similar thing is also done
for rtl8211e in the patch 1 of this series.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

Changelog v3
- Add this cover-letter.
- Add Andrew' Reviewed-by tag to patch 1.
- Accept RGMII_RXID interface mode for rtl8211f and clear the TX_DELAY
  bit in this case.
- Initialize ret variable with 0 to prevent the "may be used uninitialized"
  warning in patch 1.


Serge Semin (2):
  net: phy: realtek: Add rtl8211e rx/tx delays config
  net: phy: realtek: Change TX-delay setting for RGMII modes only

 drivers/net/phy/realtek.c | 70 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 4 deletions(-)

-- 
2.21.0

[PATCH v3 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Serge Semin]

There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
delays to TXC and RXC for TXD/RXD latching. Alas this is the only
documented info regarding the RGMII timing control configurations the PHY
provides. It turns out the same settings can be setup via MDIO registers
hidden in the extension pages layout. Particularly the extension page 0xa4
provides a register 0x1c, which bits 1 and 2 control the described delays.
They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.

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

There is also a freebsd-folks discussion regarding this register:
https://reviews.freebsd.org/D13591

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

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---
Changelog v2
- Disable delays for rgmii mode and leave them as is for the rest of
  the modes.
- Remove genphy_config_init() invocation. It's redundant for rtl8211e phy.
- Fix confused return value checking of extended-page selector call.
- Fix commit message typos.

Changelog v3
- Add Andrew' Reviewed-by tag
- Initialize ret with 0 to prevent the "may be used uninitialized" warning.
---
 drivers/net/phy/realtek.c | 51 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 10df52ccddfe..d27f072f227b 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -23,11 +23,15 @@
 
 #define RTL821x_INSR				0x13
 
+#define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
 #define RTL8211F_INSR				0x1d
 
 #define RTL8211F_TX_DELAY			BIT(8)
+#define RTL8211E_TX_DELAY			BIT(1)
+#define RTL8211E_RX_DELAY			BIT(2)
+#define RTL8211E_MODE_MII_GMII			BIT(3)
 
 #define RTL8201F_ISR				0x1e
 #define RTL8201F_IER				0x13
@@ -174,6 +178,52 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
 }
 
+static int rtl8211e_config_init(struct phy_device *phydev)
+{
+	int ret = 0, oldpage;
+	u16 val;
+
+	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		val = 0;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		val = RTL8211E_RX_DELAY;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val = RTL8211E_TX_DELAY;
+		break;
+	default: /* the rest of the modes imply leaving delays as is. */
+		return 0;
+	}
+
+	/* According to a sample driver there is a 0x1c config register on the
+	 * 0xa4 extension page (0x7) layout. It can be used to disable/enable
+	 * the RX/TX delays otherwise controlled by hardware strobes. It can
+	 * also be used to customize the whole configuration register:
+	 * 8:6 = PHY Address, 5:4 = Auto-Negotiation, 3 = Interface Mode Select,
+	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
+	 * for details).
+	 */
+	oldpage = phy_select_page(phydev, 0x7);
+	if (oldpage < 0)
+		goto err_restore_page;
+
+	ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
+	if (ret)
+		goto err_restore_page;
+
+	ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
+			 val);
+
+err_restore_page:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8211b_suspend(struct phy_device *phydev)
 {
 	phy_write(phydev, MII_MMD_DATA, BIT(9));
@@ -257,6 +307,7 @@ static struct phy_driver realtek_drvs[] = {
 		PHY_ID_MATCH_EXACT(0x001cc915),
 		.name		= "RTL8211E Gigabit Ethernet",
 		.features	= PHY_GBIT_FEATURES,
+		.config_init	= &rtl8211e_config_init,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
 		.suspend	= genphy_suspend,
-- 
2.21.0

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

[Serge Semin]

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>

---
Changelog v3
- Accept RGMII_RXID interface mode by clearing the TX_DELAY bit
  in this case.
---
 drivers/net/phy/realtek.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index d27f072f227b..134b5fc7926d 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -164,16 +164,27 @@ 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-txid, and disable it for rgmii and
+	 * rgmii-rxid. The last one might be enabled by external RXDLY pin
+	 * strapping.
+	 */
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		val = 0;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
 		val = RTL8211F_TX_DELAY;
+		break;
+	default: /* the rest of the modes imply leaving delay as is. */
+		return 0;
+	}
 
 	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
 }
-- 
2.21.0

Re: [PATCH v3 0/2] net: phy: realtek: Fix RGMII TX/RX-delays initial config of rtl8211(e|f)

[David Miller]

From: Serge Semin <fancer.lancer@gmail.com>
Date: Wed,  8 May 2019 04:29:18 +0300

> It has been discovered that RX/TX delays of rtl8211e ethernet PHY
> can be configured via a MDIO register hidden in the extension pages
> layout. Particularly the extension page 0xa4 provides a register 0x1c,
> which bits 1 and 2 control the described delays. They are used to
> implement the "rgmii-{id,rxid,txid}" phy-mode support in patch 1.
> 
> The second patch makes sure the rtl8211f TX-delay is configured only
> if RGMII interface mode is specified including the rgmii-rxid one.
> In other cases (most importantly for NA mode) the delays are supposed
> to be preconfigured by some other software or hardware and should be
> left as is without any modification. The similar thing is also done
> for rtl8211e in the patch 1 of this series.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

These patches do not apply to the current net GIT tree, please respin.

[PATCH v4 0/2] net: phy: realtek: Fix RGMII TX/RX-delays initial config of rtl8211(e|f)

[Serge Semin]

It has been discovered that RX/TX delays of rtl8211e ethernet PHY
can be configured via a MDIO register hidden in the extension pages
layout. Particularly the extension page 0xa4 provides a register 0x1c,
which bits 1 and 2 control the described delays. They are used to
implement the "rgmii-{id,rxid,txid}" phy-mode support in patch 1.

The second patch makes sure the rtl8211f TX-delay is configured only
if RGMII interface mode is specified including the rgmii-rxid one.
In other cases (most importantly for NA mode) the delays are supposed
to be preconfigured by some other software or hardware and should be
left as is without any modification. The similar thing is also done
for rtl8211e in the patch 1 of this series.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

Changelog v3
- Add this cover-letter.
- Add Andrew' Reviewed-by tag to patch 1.
- Accept RGMII_RXID interface mode for rtl8211f and clear the TX_DELAY
  bit in this case.
- Initialize ret variable with 0 to prevent the "may be used uninitialized"
  warning in patch 1.

Changelog v4
- Rebase onto net-next


Serge Semin (2):
  net: phy: realtek: Add rtl8211e rx/tx delays config
  net: phy: realtek: Change TX-delay setting for RGMII modes only

 drivers/net/phy/realtek.c | 70 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 4 deletions(-)

-- 
2.21.0

[PATCH v4 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Serge Semin]

There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
delays to TXC and RXC for TXD/RXD latching. Alas this is the only
documented info regarding the RGMII timing control configurations the PHY
provides. It turns out the same settings can be setup via MDIO registers
hidden in the extension pages layout. Particularly the extension page 0xa4
provides a register 0x1c, which bits 1 and 2 control the described delays.
They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.

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

There is also a freebsd-folks discussion regarding this register:
https://reviews.freebsd.org/D13591

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

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---
Changelog v2
- Disable delays for rgmii mode and leave them as is for the rest of
  the modes.
- Remove genphy_config_init() invocation. It's redundant for rtl8211e phy.
- Fix confused return value checking of extended-page selector call.
- Fix commit message typos.

Changelog v3
- Add Andrew' Reviewed-by tag
- Initialize ret with 0 to prevent the "may be used uninitialized" warning.

Changelog v4
- Rebase onto net-next
---
 drivers/net/phy/realtek.c | 51 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index d6a10f323117..cfbc0ca61123 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -23,11 +23,15 @@
 
 #define RTL821x_INSR				0x13
 
+#define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
 #define RTL8211F_INSR				0x1d
 
 #define RTL8211F_TX_DELAY			BIT(8)
+#define RTL8211E_TX_DELAY			BIT(1)
+#define RTL8211E_RX_DELAY			BIT(2)
+#define RTL8211E_MODE_MII_GMII			BIT(3)
 
 #define RTL8201F_ISR				0x1e
 #define RTL8201F_IER				0x13
@@ -167,6 +171,52 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
 }
 
+static int rtl8211e_config_init(struct phy_device *phydev)
+{
+	int ret = 0, oldpage;
+	u16 val;
+
+	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		val = 0;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		val = RTL8211E_RX_DELAY;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val = RTL8211E_TX_DELAY;
+		break;
+	default: /* the rest of the modes imply leaving delays as is. */
+		return 0;
+	}
+
+	/* According to a sample driver there is a 0x1c config register on the
+	 * 0xa4 extension page (0x7) layout. It can be used to disable/enable
+	 * the RX/TX delays otherwise controlled by RXDLY/TXDLY pins. It can
+	 * also be used to customize the whole configuration register:
+	 * 8:6 = PHY Address, 5:4 = Auto-Negotiation, 3 = Interface Mode Select,
+	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
+	 * for details).
+	 */
+	oldpage = phy_select_page(phydev, 0x7);
+	if (oldpage < 0)
+		goto err_restore_page;
+
+	ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
+	if (ret)
+		goto err_restore_page;
+
+	ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
+			 val);
+
+err_restore_page:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8211b_suspend(struct phy_device *phydev)
 {
 	phy_write(phydev, MII_MMD_DATA, BIT(9));
@@ -239,6 +289,7 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc915),
 		.name		= "RTL8211E Gigabit Ethernet",
+		.config_init	= &rtl8211e_config_init,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
 		.suspend	= genphy_suspend,
-- 
2.21.0

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

[Serge Semin]

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. This only
concerns rtl8211f chips.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---
Changelog v3
- Accept id and rxid interface mode by clearing the TX_DELAY bit
  in this case.

Changelog v4
- Rebase onto net-next
---
 drivers/net/phy/realtek.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index cfbc0ca61123..761ce3b1e7bd 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -161,12 +161,23 @@ static int rtl8211c_config_init(struct phy_device *phydev)
 
 static int rtl8211f_config_init(struct phy_device *phydev)
 {
-	u16 val = 0;
+	u16 val;
 
-	/* 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,txid}, and disable it for rgmii and
+	 * rgmii-rxid. The RX-delay can be enabled by the external RXDLY pin.
+	 */
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		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;
+	}
 
 	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
 }
-- 
2.21.0

Re: [PATCH v4 0/2] net: phy: realtek: Fix RGMII TX/RX-delays initial config of rtl8211(e|f)

[David Miller]

From: Serge Semin <fancer.lancer@gmail.com>
Date: Thu,  9 May 2019 00:51:13 +0300

> It has been discovered that RX/TX delays of rtl8211e ethernet PHY
> can be configured via a MDIO register hidden in the extension pages
> layout. Particularly the extension page 0xa4 provides a register 0x1c,
> which bits 1 and 2 control the described delays. They are used to
> implement the "rgmii-{id,rxid,txid}" phy-mode support in patch 1.
> 
> The second patch makes sure the rtl8211f TX-delay is configured only
> if RGMII interface mode is specified including the rgmii-rxid one.
> In other cases (most importantly for NA mode) the delays are supposed
> to be preconfigured by some other software or hardware and should be
> left as is without any modification. The similar thing is also done
> for rtl8211e in the patch 1 of this series.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> 
> Changelog v3
> - Add this cover-letter.
> - Add Andrew' Reviewed-by tag to patch 1.
> - Accept RGMII_RXID interface mode for rtl8211f and clear the TX_DELAY
>   bit in this case.
> - Initialize ret variable with 0 to prevent the "may be used uninitialized"
>   warning in patch 1.
> 
> Changelog v4
> - Rebase onto net-next

Series applied.

Re: [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Guenter Roeck]

Hi,

On Sat, Apr 27, 2019 at 12:21:11AM +0300, Serge Semin wrote:
> There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
> delays to TXC and RXC for TXD/RXD latching. Alas this is the only
> documented info regarding the RGMII timing control configurations the PHY
> provides. It turns out the same settings can be setup via MDIO registers
> hidden in the extension pages layout. Particularly the extension page 0xa4
> provides a register 0x1c, which bits 1 and 2 control the described delays.
> They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.
> 
> 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
> 
> There is also a freebsd-folks discussion regarding this register:
> https://reviews.freebsd.org/D13591
> 
> 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
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

This patch results in a crash when running arm:ast2500-evb in qemu.

[    4.894572] [00000000] *pgd=00000000
[    4.895329] Internal error: Oops: 80000005 [#1] ARM
[    4.896066] CPU: 0 PID: 1 Comm: swapper Not tainted 5.1.0-09698-g1fb3b52 #1
[    4.896364] Hardware name: Generic DT based system
[    4.896823] PC is at 0x0
[    4.897037] LR is at phy_select_page+0x3c/0x7c

Debugging shows that phydev->drv->write_page and phydev->drv->read_page
are NULL, so the crash isn't entirely surprising.

What I don't understand is how this can work in the first place.
The modified entry in realtek_drvs[] doesn't have read_page/write_page
functions defined, yet rtl8211e_config_init() depends on it.
What am I missing here ?

Thanks,
Guenter

Re: [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config

[Serge Semin]

Hello Guenter,

On Sun, May 12, 2019 at 10:41:32PM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Sat, Apr 27, 2019 at 12:21:11AM +0300, Serge Semin wrote:
> > There are two chip pins named TXDLY and RXDLY which actually adds the 2ns
> > delays to TXC and RXC for TXD/RXD latching. Alas this is the only
> > documented info regarding the RGMII timing control configurations the PHY
> > provides. It turns out the same settings can be setup via MDIO registers
> > hidden in the extension pages layout. Particularly the extension page 0xa4
> > provides a register 0x1c, which bits 1 and 2 control the described delays.
> > They are used to implement the "rgmii-{id,rxid,txid}" phy-mode.
> > 
> > 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
> > 
> > There is also a freebsd-folks discussion regarding this register:
> > https://reviews.freebsd.org/D13591
> > 
> > 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
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> This patch results in a crash when running arm:ast2500-evb in qemu.
> 
> [    4.894572] [00000000] *pgd=00000000
> [    4.895329] Internal error: Oops: 80000005 [#1] ARM
> [    4.896066] CPU: 0 PID: 1 Comm: swapper Not tainted 5.1.0-09698-g1fb3b52 #1
> [    4.896364] Hardware name: Generic DT based system
> [    4.896823] PC is at 0x0
> [    4.897037] LR is at phy_select_page+0x3c/0x7c
> 
> Debugging shows that phydev->drv->write_page and phydev->drv->read_page
> are NULL, so the crash isn't entirely surprising.
> 
> What I don't understand is how this can work in the first place.
> The modified entry in realtek_drvs[] doesn't have read_page/write_page
> functions defined, yet rtl8211e_config_init() depends on it.
> What am I missing here ?
> 
> Thanks,
> Guenter

Thanks for sending the report. The problem has already been fixed in the net:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=daf3ddbe11a2ff74c95bc814df8e5fe3201b4cb5

-Sergey

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

[Serge Semin]

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;
+	}
 
 	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
 }
-- 
2.21.0

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

[Andrew Lunn]

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?

That means PHY_INTERFACE_MODE_RGMII_ID and
PHY_INTERFACE_MODE_RGMII_RXID are not supported, and you should return
-EINVAL.

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?

	Andrew

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

[Serge Semin]

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. But what about the rest of the modes like GMII, MII
and others? Shouldn't we also return an error instead of leaving a default
delay value?

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

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

[Florian Fainelli]

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

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@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

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

[Serge Semin]

On Mon, Apr 29, 2019 at 09:29:37PM +0300, Vladimir Oltean wrote:
> 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

Hello Vladimir, Florian and Andrew

Thanks for your comments on this matter. Let me sum my understanding of
this up so we'd set a checkpoint of the rtl8211* interface delays
discussion.

rtl8211(e|f) TX/RX delays can be configured either by external pins
strapping or via software registers. This is one of the clue to provide
a proper config_init method code. But not all rtl8211f phys provide
that software register, and if they do it only concerns TX-delay (as we
aware of). So we need to take this into account when creating the updated
versions of these functions.

(Martin, I also Cc'ed you in this discussion, so if you have anything to
say in this matter, please don't hesitate to comment.)

As Vladimir fairly pointed out DT should be considered as a "hardware
description", and not a "hardware configuration". But I would correct
the interpretation you suggested a bit. Current phy-mode property meaning
I would explain as a MAC-PHY interface description (so it complies with
"DT - hardware description" rule). So "rgmii" means TXC/RXC, TXD[4] RXD[4],
RX_CTL lanes, etc; "gmii" - GTXCLK, TXCLK/RXCLK, TXD[8], RXD[8],
TXEN, RXEN, COL, CS lanes, etc and so on. The "-*id" suffixes are something
that describe the logical signal delays, which due to the physical lanes design
might be required so the corresponding "{r,g,rg}mii" interface would work
correctly.

Then in accordance with the phy-mode property value MAC and PHY drivers
determine which way the MAC and PHY are connected to each other and how
their settings are supposed to be customized to comply with it. This
interpretation perfectly fits with the "DT is the hardware description"
rule.

Finally I dig into the available rtl8211(e|f) datasheets a bit deeper
and found out, that rtl8211f PHYs provide the RGMII interface only.
While there is only one model of rtl8211e PHY which aside from the default
RGMII-interface can work over MII/GMII-interfaces (which BTW can be
enabled via a pin strapping and via a register I used for delay
settings). So even if MAC provides a way to enable MII/GMII/RGMII/SGMII/etc
interfaces the PHY driver should refuse to work over interfaces
it' device doesn't support by design. This is what lacks the current
rtl8211(f|e)_config_init methods design. (Andrew also might have
suggested something like this, though I am not sure I fully understood
what he meant.)

So as all of this info must be taken into account to create a proper driver
rtl8211(e|f) config_init methods. As I see it we need to provide the following
alterations to the realtek driver:
rtl8211f config_init:
- RGMII-interface is only supported, so return an error if any other
interface mode is requested. Andrew also suggested to accept
the PHY_INTERFACE_MODE_NA mode as an implication to leave the mode as is.
Although I have a doubt about this since none of the PHY' drivers
does this at the moment. Any comment on this?
- phy-mode = "rgmii", no delays need to be set on the PHY side, so clear
{page=0xd08, register=0x11, bit=8} bit and rely on the RXD pin being pulled
low (RX delay is also disabled).
- phy-mode = "rgmii-id", all delays are supposed to be set on the PHY side.
We can manually set the TX-delay, while RX-delay is set over an external pin.
So set the {page=0xd08, register=0x11, bit=8} bit only, and rely on the
hardware designer pulling the RXD pin high.
- phy-mode = "rgmii-txid", set the {page=0xd08, register=0x11, bit=8} bit only,
and rely on the hardware designer pulling the RXD pin low.
- phy-mode = "rgmii-rxid", clear the {page=0xd08, register=0x11, bit=8} bit only,
and rely on the hardware designer pulling the RXD pin high.

rtl8211e config_init:
- MII/GMII/RGMII-interfaces are only supported, so return an error if any other
interface mode is requested. The same thought about PHY_INTERFACE_MODE_NA is
applicable here. 
- MII/GMII-interface can be enabled via the Mode pin strapping or via the
configuration register I've used to set the delays. So use it to enable/disable
either MII/GMII or RGMII interface mode.
- phy-mode = ("rgmii"|"rgmii-id"|"rgmii-txid"|"rgmii-rxid") - enable and disable
the TX/RX-delay bits of the corresponding register, since either of these modes
can be configured by software.


Could you comment on these thoughts so we'd finally come up with a final
decision and I'd send out a new version of the patchset.

-Sergey

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

[Vladimir Oltean]

Hi Serge,

On Tue, 30 Apr 2019 at 00:12, Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Mon, Apr 29, 2019 at 09:29:37PM +0300, Vladimir Oltean wrote:
> > 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
>
> Hello Vladimir, Florian and Andrew
>
> Thanks for your comments on this matter. Let me sum my understanding of
> this up so we'd set a checkpoint of the rtl8211* interface delays
> discussion.
>
> rtl8211(e|f) TX/RX delays can be configured either by external pins
> strapping or via software registers. This is one of the clue to provide
> a proper config_init method code. But not all rtl8211f phys provide
> that software register, and if they do it only concerns TX-delay (as we
> aware of). So we need to take this into account when creating the updated
> versions of these functions.
>
> (Martin, I also Cc'ed you in this discussion, so if you have anything to
> say in this matter, please don't hesitate to comment.)
>
> As Vladimir fairly pointed out DT should be considered as a "hardware
> description", and not a "hardware configuration". But I would correct
> the interpretation you suggested a bit. Current phy-mode property meaning
> I would explain as a MAC-PHY interface description (so it complies with
> "DT - hardware description" rule). So "rgmii" means TXC/RXC, TXD[4] RXD[4],
> RX_CTL lanes, etc; "gmii" - GTXCLK, TXCLK/RXCLK, TXD[8], RXD[8],
> TXEN, RXEN, COL, CS lanes, etc and so on. The "-*id" suffixes are something
> that describe the logical signal delays, which due to the physical lanes design
> might be required so the corresponding "{r,g,rg}mii" interface would work
> correctly.
>

As Andrew pointed out, only RGMII needs clock skew, mainly because it
is an interface with source-synchronous clocking that runs at 125 MHz
(when the link speed is 1000 Mbps, otherwise the delays are less
important) and is double data rate (data gets sampled on both rising
and falling clock edges).
Moreover, RGMII *always* needs clock skew. As a fact, all delays
applied on RX and RX, by the PHY, MAC or traces, should always amount
to a logical "rgmii-id". There's nothing that needs to be described
about that. Everybody knows it.
What Linux gets told through the phy-mode property for RGMII is where
there's extra stuff to do, and where there's nothing to do. There are
also unwritten rules about whose job it is to apply the clock skew
(MAC or PHY).That is 100% configuration and 0% description.

> Then in accordance with the phy-mode property value MAC and PHY drivers
> determine which way the MAC and PHY are connected to each other and how
> their settings are supposed to be customized to comply with it. This
> interpretation perfectly fits with the "DT is the hardware description"
> rule.
>

Most of the phy-mode properties really mean nothing. I changed the
phy-mode from "sgmii" to "rgmii" on a PHY binding I had at hand and
nothing happened (traffic still runs normally). I think this behavior
is 100% within expectation.

> Finally I dig into the available rtl8211(e|f) datasheets a bit deeper
> and found out, that rtl8211f PHYs provide the RGMII interface only.
> While there is only one model of rtl8211e PHY which aside from the default
> RGMII-interface can work over MII/GMII-interfaces (which BTW can be
> enabled via a pin strapping and via a register I used for delay
> settings). So even if MAC provides a way to enable MII/GMII/RGMII/SGMII/etc
> interfaces the PHY driver should refuse to work over interfaces
> it' device doesn't support by design. This is what lacks the current
> rtl8211(f|e)_config_init methods design. (Andrew also might have
> suggested something like this, though I am not sure I fully understood
> what he meant.)
>
> So as all of this info must be taken into account to create a proper driver
> rtl8211(e|f) config_init methods. As I see it we need to provide the following
> alterations to the realtek driver:
> rtl8211f config_init:
> - RGMII-interface is only supported, so return an error if any other
> interface mode is requested. Andrew also suggested to accept
> the PHY_INTERFACE_MODE_NA mode as an implication to leave the mode as is.
> Although I have a doubt about this since none of the PHY' drivers
> does this at the moment. Any comment on this?

I think this is way overboard, since "configuring the interface as
SGMII" is not something that software alone can do (and many times,
the software can't do anything at all to make a MAC speak SGMII vs
RGMII vs whatever else). And there are times when the software can't
even know what the MAC speaks, unless the driver hardcodes it. So the
phy-mode checking would only protect against mismatches of hardcoded
values. So it would error out whilst the hardware would keep working
and minding its own business as if nothing happened... You can't
really connect an RGMII MAC to an SGMII PHY, in real life :)

> - phy-mode = "rgmii", no delays need to be set on the PHY side, so clear
> {page=0xd08, register=0x11, bit=8} bit and rely on the RXD pin being pulled
> low (RX delay is also disabled).
> - phy-mode = "rgmii-id", all delays are supposed to be set on the PHY side.
> We can manually set the TX-delay, while RX-delay is set over an external pin.
> So set the {page=0xd08, register=0x11, bit=8} bit only, and rely on the
> hardware designer pulling the RXD pin high.
> - phy-mode = "rgmii-txid", set the {page=0xd08, register=0x11, bit=8} bit only,
> and rely on the hardware designer pulling the RXD pin low.
> - phy-mode = "rgmii-rxid", clear the {page=0xd08, register=0x11, bit=8} bit only,
> and rely on the hardware designer pulling the RXD pin high.
>

I would remove the portions where you say "rely on hw doing
this/that". The phy-mode only concerns software (MDIO) business. The
"hardware description vs hardware configuration" on my part is just
wishful thinking. You *can* have the phy-mode as "rgmii-txid" and the
RXDLY pin enabled in the strapping config, and that would amount to a
total, logical "rgmii-id" on the PHY, and it would be fine (Linux not
having an accurate "hardware description" of the system).

> rtl8211e config_init:
> - MII/GMII/RGMII-interfaces are only supported, so return an error if any other
> interface mode is requested. The same thought about PHY_INTERFACE_MODE_NA is
> applicable here.
> - MII/GMII-interface can be enabled via the Mode pin strapping or via the
> configuration register I've used to set the delays. So use it to enable/disable
> either MII/GMII or RGMII interface mode.
> - phy-mode = ("rgmii"|"rgmii-id"|"rgmii-txid"|"rgmii-rxid") - enable and disable
> the TX/RX-delay bits of the corresponding register, since either of these modes
> can be configured by software.
>
>
> Could you comment on these thoughts so we'd finally come up with a final
> decision and I'd send out a new version of the patchset.

Erroring out on rgmii-rxid and rgmii-id is probably justifiable, but
then again, just because you can, doesn't mean you should. Could you
please explain again what problem this is trying to solve? I only see
the problems that this will create :)

>
> -Sergey

Thanks,
-Vladimir

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

[Serge Semin]

On Tue, Apr 30, 2019 at 01:36:59AM +0300, Vladimir Oltean wrote:
> Hi Serge,
> 
> On Tue, 30 Apr 2019 at 00:12, Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Mon, Apr 29, 2019 at 09:29:37PM +0300, Vladimir Oltean wrote:
> > > 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
> >
> > Hello Vladimir, Florian and Andrew
> >
> > Thanks for your comments on this matter. Let me sum my understanding of
> > this up so we'd set a checkpoint of the rtl8211* interface delays
> > discussion.
> >
> > rtl8211(e|f) TX/RX delays can be configured either by external pins
> > strapping or via software registers. This is one of the clue to provide
> > a proper config_init method code. But not all rtl8211f phys provide
> > that software register, and if they do it only concerns TX-delay (as we
> > aware of). So we need to take this into account when creating the updated
> > versions of these functions.
> >
> > (Martin, I also Cc'ed you in this discussion, so if you have anything to
> > say in this matter, please don't hesitate to comment.)
> >
> > As Vladimir fairly pointed out DT should be considered as a "hardware
> > description", and not a "hardware configuration". But I would correct
> > the interpretation you suggested a bit. Current phy-mode property meaning
> > I would explain as a MAC-PHY interface description (so it complies with
> > "DT - hardware description" rule). So "rgmii" means TXC/RXC, TXD[4] RXD[4],
> > RX_CTL lanes, etc; "gmii" - GTXCLK, TXCLK/RXCLK, TXD[8], RXD[8],
> > TXEN, RXEN, COL, CS lanes, etc and so on. The "-*id" suffixes are something
> > that describe the logical signal delays, which due to the physical lanes design
> > might be required so the corresponding "{r,g,rg}mii" interface would work
> > correctly.
> >
> 
> As Andrew pointed out, only RGMII needs clock skew, mainly because it
> is an interface with source-synchronous clocking that runs at 125 MHz
> (when the link speed is 1000 Mbps, otherwise the delays are less
> important) and is double data rate (data gets sampled on both rising
> and falling clock edges).

Right, I should have said "rgmii" instead of "{r,g,rg}mii". I noticed this
after the letter had already been sent.

> Moreover, RGMII *always* needs clock skew. As a fact, all delays
> applied on RX and RX, by the PHY, MAC or traces, should always amount
> to a logical "rgmii-id". There's nothing that needs to be described
> about that. Everybody knows it.
> What Linux gets told through the phy-mode property for RGMII is where
> there's extra stuff to do, and where there's nothing to do. There are
> also unwritten rules about whose job it is to apply the clock skew
> (MAC or PHY).That is 100% configuration and 0% description.
> 
> > Then in accordance with the phy-mode property value MAC and PHY drivers
> > determine which way the MAC and PHY are connected to each other and how
> > their settings are supposed to be customized to comply with it. This
> > interpretation perfectly fits with the "DT is the hardware description"
> > rule.
> >
> 
> Most of the phy-mode properties really mean nothing. I changed the
> phy-mode from "sgmii" to "rgmii" on a PHY binding I had at hand and
> nothing happened (traffic still runs normally). I think this behavior
> is 100% within expectation.
> 
> > Finally I dig into the available rtl8211(e|f) datasheets a bit deeper
> > and found out, that rtl8211f PHYs provide the RGMII interface only.
> > While there is only one model of rtl8211e PHY which aside from the default
> > RGMII-interface can work over MII/GMII-interfaces (which BTW can be
> > enabled via a pin strapping and via a register I used for delay
> > settings). So even if MAC provides a way to enable MII/GMII/RGMII/SGMII/etc
> > interfaces the PHY driver should refuse to work over interfaces
> > it' device doesn't support by design. This is what lacks the current
> > rtl8211(f|e)_config_init methods design. (Andrew also might have
> > suggested something like this, though I am not sure I fully understood
> > what he meant.)
> >
> > So as all of this info must be taken into account to create a proper driver
> > rtl8211(e|f) config_init methods. As I see it we need to provide the following
> > alterations to the realtek driver:
> > rtl8211f config_init:
> > - RGMII-interface is only supported, so return an error if any other
> > interface mode is requested. Andrew also suggested to accept
> > the PHY_INTERFACE_MODE_NA mode as an implication to leave the mode as is.
> > Although I have a doubt about this since none of the PHY' drivers
> > does this at the moment. Any comment on this?
> 
> I think this is way overboard, since "configuring the interface as
> SGMII" is not something that software alone can do (and many times,
> the software can't do anything at all to make a MAC speak SGMII vs
> RGMII vs whatever else). And there are times when the software can't
> even know what the MAC speaks, unless the driver hardcodes it. So the
> phy-mode checking would only protect against mismatches of hardcoded
> values. So it would error out whilst the hardware would keep working
> and minding its own business as if nothing happened... You can't
> really connect an RGMII MAC to an SGMII PHY, in real life :)
> 
> > - phy-mode = "rgmii", no delays need to be set on the PHY side, so clear
> > {page=0xd08, register=0x11, bit=8} bit and rely on the RXD pin being pulled
> > low (RX delay is also disabled).
> > - phy-mode = "rgmii-id", all delays are supposed to be set on the PHY side.
> > We can manually set the TX-delay, while RX-delay is set over an external pin.
> > So set the {page=0xd08, register=0x11, bit=8} bit only, and rely on the
> > hardware designer pulling the RXD pin high.
> > - phy-mode = "rgmii-txid", set the {page=0xd08, register=0x11, bit=8} bit only,
> > and rely on the hardware designer pulling the RXD pin low.
> > - phy-mode = "rgmii-rxid", clear the {page=0xd08, register=0x11, bit=8} bit only,
> > and rely on the hardware designer pulling the RXD pin high.
> >
> 
> I would remove the portions where you say "rely on hw doing
> this/that". The phy-mode only concerns software (MDIO) business. The
> "hardware description vs hardware configuration" on my part is just
> wishful thinking. You *can* have the phy-mode as "rgmii-txid" and the
> RXDLY pin enabled in the strapping config, and that would amount to a
> total, logical "rgmii-id" on the PHY, and it would be fine (Linux not
> having an accurate "hardware description" of the system).
> 

Alright I see your point on this. Thanks for the comments. Lets see what
the subsystem maintainers think about your and my suggestions.

> > rtl8211e config_init:
> > - MII/GMII/RGMII-interfaces are only supported, so return an error if any other
> > interface mode is requested. The same thought about PHY_INTERFACE_MODE_NA is
> > applicable here.
> > - MII/GMII-interface can be enabled via the Mode pin strapping or via the
> > configuration register I've used to set the delays. So use it to enable/disable
> > either MII/GMII or RGMII interface mode.
> > - phy-mode = ("rgmii"|"rgmii-id"|"rgmii-txid"|"rgmii-rxid") - enable and disable
> > the TX/RX-delay bits of the corresponding register, since either of these modes
> > can be configured by software.
> >
> >
> > Could you comment on these thoughts so we'd finally come up with a final
> > decision and I'd send out a new version of the patchset.
> 
> Erroring out on rgmii-rxid and rgmii-id is probably justifiable, but
> then again, just because you can, doesn't mean you should. Could you
> please explain again what problem this is trying to solve? I only see
> the problems that this will create :)
> 

My question was regarding the Andrew' comment on [Patch v2 2/2]:

> On Fri, Apr 26, 2019 at 11:46:31PM +0200, Andrew Lunn wrote:
> So there is no control of the RX delay?
>
> That means PHY_INTERFACE_MODE_RGMII_ID and
> PHY_INTERFACE_MODE_RGMII_RXID are not supported, and you should return
> -EINVAL.
>
> 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?

I was a bit surprised about this because first of all my patch didn't change
the code behavior in this matter. This was the way the original code worked
even without my changes. 

Secondly I was also surprised that we should return -EINVAL for just these two
modes, while the rest of them are just ignored even though we are sure that
rtl8211f PHYs support the RGMII mode only. This also concerns the rtl8211e
config_init which performs the RGMII delay configuration while ignoring
the rest of the interface modes specified in the phy_device structure.
So the main question is why should one code skip the configuration procedure
while another one returns an error, and why should we return an error only
for rgmii-id and rgmii-rxid modes while the really unsupported modes are
just ignored?

That's why I raised the whole discussion regarding the interface modes
validation performed by MACs and PHYs.

Then you send an email here and pointed out the confusion regarding
rtl8211f RXD/TXD pin strapping, which made the Andrew' request even more
questionable to me. Then I decided to sum up my understanding of the discussion
so to collect all my thoughts in a single text and then to proceed with the
conversation from it.

So Vladimir, could you please take a look at [Patch v2 2/2] patch and
give a comment on the changes it provides?

-Sergey

> >
> > -Sergey
> 
> Thanks,
> -Vladimir

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

[Martin Blumenstingl]

Hello Vladimir,

On Tue, Apr 30, 2019 at 12:37 AM Vladimir Oltean <olteanv@gmail.com> wrote:
[...]
> Moreover, RGMII *always* needs clock skew. As a fact, all delays
> applied on RX and RX, by the PHY, MAC or traces, should always amount
> to a logical "rgmii-id". There's nothing that needs to be described
> about that. Everybody knows it.
thank you for mentioning this - I didn't know about it. I thought that
the delays have to be added in "some cases" only (without knowing the
definition of "some cases").

> What Linux gets told through the phy-mode property for RGMII is where
> there's extra stuff to do, and where there's nothing to do. There are
> also unwritten rules about whose job it is to apply the clock skew
> (MAC or PHY).That is 100% configuration and 0% description.
the phy-mode property is documented here [0] and the rgmii modes have
a short explanation about the delays.
that said: the documentation currently ignores the fact that a PCB
designer might have added a delay

> > Then in accordance with the phy-mode property value MAC and PHY drivers
> > determine which way the MAC and PHY are connected to each other and how
> > their settings are supposed to be customized to comply with it. This
> > interpretation perfectly fits with the "DT is the hardware description"
> > rule.
> >
>
> Most of the phy-mode properties really mean nothing. I changed the
> phy-mode from "sgmii" to "rgmii" on a PHY binding I had at hand and
> nothing happened (traffic still runs normally). I think this behavior
> is 100% within expectation.
the PHY drivers I know of don't complain if the phy-mode is not supported.
however, there are MAC drivers which complain in this case, see [1]
for one example


Martin


[0] https://github.com/torvalds/linux/blob/bf3bd966dfd7d9582f50e9bd08b15922197cd277/Documentation/devicetree/bindings/net/ethernet.txt#L17
[1] https://github.com/torvalds/linux/blob/bf3bd966dfd7d9582f50e9bd08b15922197cd277/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c#L187

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

[Serge Semin]

Hello folks

On Tue, Apr 30, 2019 at 10:44:00PM +0200, Martin Blumenstingl wrote:
> Hello Vladimir,
> 
> On Tue, Apr 30, 2019 at 12:37 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> [...]
> > Moreover, RGMII *always* needs clock skew. As a fact, all delays
> > applied on RX and RX, by the PHY, MAC or traces, should always amount
> > to a logical "rgmii-id". There's nothing that needs to be described
> > about that. Everybody knows it.
> thank you for mentioning this - I didn't know about it. I thought that
> the delays have to be added in "some cases" only (without knowing the
> definition of "some cases").
> 
> > What Linux gets told through the phy-mode property for RGMII is where
> > there's extra stuff to do, and where there's nothing to do. There are
> > also unwritten rules about whose job it is to apply the clock skew
> > (MAC or PHY).That is 100% configuration and 0% description.
> the phy-mode property is documented here [0] and the rgmii modes have
> a short explanation about the delays.
> that said: the documentation currently ignores the fact that a PCB
> designer might have added a delay
> 
> > > Then in accordance with the phy-mode property value MAC and PHY drivers
> > > determine which way the MAC and PHY are connected to each other and how
> > > their settings are supposed to be customized to comply with it. This
> > > interpretation perfectly fits with the "DT is the hardware description"
> > > rule.
> > >
> >
> > Most of the phy-mode properties really mean nothing. I changed the
> > phy-mode from "sgmii" to "rgmii" on a PHY binding I had at hand and
> > nothing happened (traffic still runs normally). I think this behavior
> > is 100% within expectation.
> the PHY drivers I know of don't complain if the phy-mode is not supported.
> however, there are MAC drivers which complain in this case, see [1]
> for one example
> 
> 
> Martin
> 
> 
> [0] https://github.com/torvalds/linux/blob/bf3bd966dfd7d9582f50e9bd08b15922197cd277/Documentation/devicetree/bindings/net/ethernet.txt#L17
> [1] https://github.com/torvalds/linux/blob/bf3bd966dfd7d9582f50e9bd08b15922197cd277/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c#L187

So as far as I can conclude from the whole discussion, we don't really
want the realtek driver to return an error in case if any unsupported
mode is passed to the config_init method including the RGMII_RXID one,
which in some cases could be enabled via RXDLY pin strapping (like on
rtl8211F). The reason is quite justified: even though there is no
known way to alter the RX-delay on the rtl8211F PHY side, the delay
can be enabled via external pin. In this case rgmii-rxid mode can be
enabled in dts and should be considered a correct error-less PHY node
property.

In order to have this all summed up, I'll shortly send a v3 patchset with
alterations based on the discussion results. Maintainers then will be able
to post their comments in reply to it.

Cheers.
-Sergey

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

[Martin Blumenstingl]

 Hello Serge,

On Mon, Apr 29, 2019 at 11:12 PM Serge Semin <fancer.lancer@gmail.com> wrote:
[...]
> > > > 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.
let me give you a bit of context on that patch:
most boards (SBCs and TV boxes) with an Amlogic SoC and a Gigabit
Ethernet PHY use a Realtek RTL8211F PHY. we were seeing high packet
loss when transmitting from the board to another device.
it took us very long to understand that a combination of different
hardware and driver pieces lead to this issue:
- in the MAC driver we enabled a 2ns TX delay by default, like Amlogic
does it in their vendor (BSP) kernel
- we used the upstream Realtek RTL8211F PHY driver which only enabled
the TX delay if requested (it never disabled the TX delay)
- hardware defaults or pin strapping of the Realtek RTL8211F PHY
enabled the TX delay in the PHY

This means that the TX delay was applied twice: once at the MAC and
once at the PHY.
That lead to high packet loss when transmitting data.
To solve that I wrote the patch you mentioned, which has since been
ported over to u-boot (for a non-Amlogic related board)

> > > > 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.
I don't have any datasheet for the Realtek RTL8211F PHY and I'm not in
the position to get one (company contracts seem to be required for
this).
Linux is not my main job, I do driver development in my spare time.

there may or may not be a register or pin strapping to configure the RX delay.
due to this I decided to leave the RX delay behavior "not defined"
instead of rejecting RGMII_RXID and RGMII_ID.

> > > 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.
the changes in patch 1 are looking good to me (except that I would use
phy_modify_paged instead of open-coding it, functionally it's
identical with what you have already)

I'm not sure about patch 2:
personally I would wait for someone to come up with the requirement to
use RGMII_RXID with a RTL8211F PHY.
that person will then a board to test the changes and (hopefully) a
datasheet to explain the RX delay situation with that PHY.
that way we only change the RGMII_RXID behavior once (when someone
requests support for it) instead of twice (now with your change, later
on when someone needs RGMII_RXID support in the RTL8211F driver)

that said, the change in patch 2 itself looks fine on Amlogic boards
(because all upstream .dts let the MAC generate the TX delay). I
haven't runtime-tested your patch there yet.
but there seem to be other boards (than the Amlogic ones, the RTL8211F
PHY driver discussion in u-boot was not related to an Amlogic board)
out there with a RTL8211F PHY (these may or may not be supported in
mainline Linux or u-boot and may or may not use RGMII_RXID where you
are now changing the behavior). that's not a problem by itself, but
you should be aware of this.

[...]
> rtl8211(e|f) TX/RX delays can be configured either by external pins
> strapping or via software registers. This is one of the clue to provide
> a proper config_init method code. But not all rtl8211f phys provide
> that software register, and if they do it only concerns TX-delay (as we
> aware of). So we need to take this into account when creating the updated
> versions of these functions.
>
> (Martin, I also Cc'ed you in this discussion, so if you have anything to
> say in this matter, please don't hesitate to comment.)
Amlogic boards, such as the Hardkernel Odroid-C1 and Odroid-C2 as well
as the Khadas VIM2 use a "RTL8211F" RGMII PHY. I don't know whether
there are multiple versions of this PHY. all RTL8211F I have seen so
far did behave exactly the same.

I also don't know whether the RX delay is configurable (by pin
strapping or some register) on RTL8211F PHYs because I don't have
access to the datasheet.


Martin

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

[Vladimir Oltean]

On Wed, 1 May 2019 at 00:16, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
>  Hello Serge,
>
> On Mon, Apr 29, 2019 at 11:12 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> [...]
> > > > > 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.
> let me give you a bit of context on that patch:
> most boards (SBCs and TV boxes) with an Amlogic SoC and a Gigabit
> Ethernet PHY use a Realtek RTL8211F PHY. we were seeing high packet
> loss when transmitting from the board to another device.
> it took us very long to understand that a combination of different
> hardware and driver pieces lead to this issue:
> - in the MAC driver we enabled a 2ns TX delay by default, like Amlogic
> does it in their vendor (BSP) kernel
> - we used the upstream Realtek RTL8211F PHY driver which only enabled
> the TX delay if requested (it never disabled the TX delay)
> - hardware defaults or pin strapping of the Realtek RTL8211F PHY
> enabled the TX delay in the PHY
>
> This means that the TX delay was applied twice: once at the MAC and
> once at the PHY.
> That lead to high packet loss when transmitting data.
> To solve that I wrote the patch you mentioned, which has since been
> ported over to u-boot (for a non-Amlogic related board)
>
> > > > > 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.
> I don't have any datasheet for the Realtek RTL8211F PHY and I'm not in
> the position to get one (company contracts seem to be required for
> this).
> Linux is not my main job, I do driver development in my spare time.
>
> there may or may not be a register or pin strapping to configure the RX delay.
> due to this I decided to leave the RX delay behavior "not defined"
> instead of rejecting RGMII_RXID and RGMII_ID.
>
> > > > 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.
> the changes in patch 1 are looking good to me (except that I would use
> phy_modify_paged instead of open-coding it, functionally it's
> identical with what you have already)
>
> I'm not sure about patch 2:
> personally I would wait for someone to come up with the requirement to
> use RGMII_RXID with a RTL8211F PHY.
> that person will then a board to test the changes and (hopefully) a
> datasheet to explain the RX delay situation with that PHY.
> that way we only change the RGMII_RXID behavior once (when someone
> requests support for it) instead of twice (now with your change, later
> on when someone needs RGMII_RXID support in the RTL8211F driver)
>
> that said, the change in patch 2 itself looks fine on Amlogic boards
> (because all upstream .dts let the MAC generate the TX delay). I
> haven't runtime-tested your patch there yet.
> but there seem to be other boards (than the Amlogic ones, the RTL8211F
> PHY driver discussion in u-boot was not related to an Amlogic board)
> out there with a RTL8211F PHY (these may or may not be supported in
> mainline Linux or u-boot and may or may not use RGMII_RXID where you
> are now changing the behavior). that's not a problem by itself, but
> you should be aware of this.
>
> [...]
> > rtl8211(e|f) TX/RX delays can be configured either by external pins
> > strapping or via software registers. This is one of the clue to provide
> > a proper config_init method code. But not all rtl8211f phys provide
> > that software register, and if they do it only concerns TX-delay (as we
> > aware of). So we need to take this into account when creating the updated
> > versions of these functions.
> >
> > (Martin, I also Cc'ed you in this discussion, so if you have anything to
> > say in this matter, please don't hesitate to comment.)
> Amlogic boards, such as the Hardkernel Odroid-C1 and Odroid-C2 as well
> as the Khadas VIM2 use a "RTL8211F" RGMII PHY. I don't know whether
> there are multiple versions of this PHY. all RTL8211F I have seen so
> far did behave exactly the same.
>
> I also don't know whether the RX delay is configurable (by pin
> strapping or some register) on RTL8211F PHYs because I don't have
> access to the datasheet.
>
>
> Martin

Hi Martin, Sergey,

I had another look and it seems that Realtek has designated the same
PHY ID for the RTL8211F and RTL8211FS. However the F is a 40-pin
package and the FS is a 48-pin package. The datasheet for the F
doesn't mention about the MDIO bit for TXDLY being implemented,
whereas for FS it does. That can mean anything, really, but as I only
have access to a board with the FS chip, I can't easily check. Maybe
Martin can confirm that his chip's designation is really not FS.
But my point still remains, though. The F and FS share the same PHY
ID, and one supports only RGMII while the other can be configured for
SGMII as well. Good luck being ultra-correct in the phy-mode checking
when you can't distinguish the chip. But in general DT description is
chief and should not be contradicted. Perhaps an argument could be
made for the RGMII delays as they constitute an exception to the "HW
description" rule.

Thanks,
-Vladimir

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

[Martin Blumenstingl]

Hi Vladimir,

On Thu, May 2, 2019 at 1:03 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Wed, 1 May 2019 at 00:16, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> >
> >  Hello Serge,
> >
> > On Mon, Apr 29, 2019 at 11:12 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > [...]
> > > > > > 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.
> > let me give you a bit of context on that patch:
> > most boards (SBCs and TV boxes) with an Amlogic SoC and a Gigabit
> > Ethernet PHY use a Realtek RTL8211F PHY. we were seeing high packet
> > loss when transmitting from the board to another device.
> > it took us very long to understand that a combination of different
> > hardware and driver pieces lead to this issue:
> > - in the MAC driver we enabled a 2ns TX delay by default, like Amlogic
> > does it in their vendor (BSP) kernel
> > - we used the upstream Realtek RTL8211F PHY driver which only enabled
> > the TX delay if requested (it never disabled the TX delay)
> > - hardware defaults or pin strapping of the Realtek RTL8211F PHY
> > enabled the TX delay in the PHY
> >
> > This means that the TX delay was applied twice: once at the MAC and
> > once at the PHY.
> > That lead to high packet loss when transmitting data.
> > To solve that I wrote the patch you mentioned, which has since been
> > ported over to u-boot (for a non-Amlogic related board)
> >
> > > > > > 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.
> > I don't have any datasheet for the Realtek RTL8211F PHY and I'm not in
> > the position to get one (company contracts seem to be required for
> > this).
> > Linux is not my main job, I do driver development in my spare time.
> >
> > there may or may not be a register or pin strapping to configure the RX delay.
> > due to this I decided to leave the RX delay behavior "not defined"
> > instead of rejecting RGMII_RXID and RGMII_ID.
> >
> > > > > 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.
> > the changes in patch 1 are looking good to me (except that I would use
> > phy_modify_paged instead of open-coding it, functionally it's
> > identical with what you have already)
> >
> > I'm not sure about patch 2:
> > personally I would wait for someone to come up with the requirement to
> > use RGMII_RXID with a RTL8211F PHY.
> > that person will then a board to test the changes and (hopefully) a
> > datasheet to explain the RX delay situation with that PHY.
> > that way we only change the RGMII_RXID behavior once (when someone
> > requests support for it) instead of twice (now with your change, later
> > on when someone needs RGMII_RXID support in the RTL8211F driver)
> >
> > that said, the change in patch 2 itself looks fine on Amlogic boards
> > (because all upstream .dts let the MAC generate the TX delay). I
> > haven't runtime-tested your patch there yet.
> > but there seem to be other boards (than the Amlogic ones, the RTL8211F
> > PHY driver discussion in u-boot was not related to an Amlogic board)
> > out there with a RTL8211F PHY (these may or may not be supported in
> > mainline Linux or u-boot and may or may not use RGMII_RXID where you
> > are now changing the behavior). that's not a problem by itself, but
> > you should be aware of this.
> >
> > [...]
> > > rtl8211(e|f) TX/RX delays can be configured either by external pins
> > > strapping or via software registers. This is one of the clue to provide
> > > a proper config_init method code. But not all rtl8211f phys provide
> > > that software register, and if they do it only concerns TX-delay (as we
> > > aware of). So we need to take this into account when creating the updated
> > > versions of these functions.
> > >
> > > (Martin, I also Cc'ed you in this discussion, so if you have anything to
> > > say in this matter, please don't hesitate to comment.)
> > Amlogic boards, such as the Hardkernel Odroid-C1 and Odroid-C2 as well
> > as the Khadas VIM2 use a "RTL8211F" RGMII PHY. I don't know whether
> > there are multiple versions of this PHY. all RTL8211F I have seen so
> > far did behave exactly the same.
> >
> > I also don't know whether the RX delay is configurable (by pin
> > strapping or some register) on RTL8211F PHYs because I don't have
> > access to the datasheet.
> >
> >
> > Martin
>
> Hi Martin, Sergey,
>
> I had another look and it seems that Realtek has designated the same
> PHY ID for the RTL8211F and RTL8211FS. However the F is a 40-pin
> package and the FS is a 48-pin package. The datasheet for the F
> doesn't mention about the MDIO bit for TXDLY being implemented,
> whereas for FS it does. That can mean anything, really, but as I only
> have access to a board with the FS chip, I can't easily check. Maybe
> Martin can confirm that his chip's designation is really not FS.
I just checked two of my boards:
- the PHY on my Hardkernel Odroid-C1+ is marked as "RTL8211F"
- the PHY on my Khadas VIM2 is marked as "RTL8211FDI"

I have not heard of a RTL8211FS chip / package before.

> But my point still remains, though. The F and FS share the same PHY
> ID, and one supports only RGMII while the other can be configured for
> SGMII as well. Good luck being ultra-correct in the phy-mode checking
> when you can't distinguish the chip. But in general DT description is
> chief and should not be contradicted. Perhaps an argument could be
> made for the RGMII delays as they constitute an exception to the "HW
> description" rule.
PHY ID being shared across various PHY revisions and packages is also
the case for some ICPlus IP101A / IP101G PHYs, see [0]
The (32-pin QFN) IP101GR uses a register to switch a pin function
between "interrupt" and "receive error".
There's no rule in the driver to enforce that this only applies to
IP101GR PHYs, it's only described in the documentation (I don't even
know if we can enforce it in software because I'm not sure we can
detect the different variants of the IP101A / IP101G PHYs)


Martin


[0] https://github.com/torvalds/linux/blob/fdc13a9effd5359ae00705708c8c846b1cb2b69c/Documentation/devicetree/bindings/net/icplus-ip101ag.txt

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

[Serge Semin]

Hello Martin.

On Tue, Apr 30, 2019 at 11:16:21PM +0200, Martin Blumenstingl wrote:
>  Hello Serge,
> 
> On Mon, Apr 29, 2019 at 11:12 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> [...]
> > > > > 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.
> let me give you a bit of context on that patch:
> most boards (SBCs and TV boxes) with an Amlogic SoC and a Gigabit
> Ethernet PHY use a Realtek RTL8211F PHY. we were seeing high packet
> loss when transmitting from the board to another device.
> it took us very long to understand that a combination of different
> hardware and driver pieces lead to this issue:
> - in the MAC driver we enabled a 2ns TX delay by default, like Amlogic
> does it in their vendor (BSP) kernel
> - we used the upstream Realtek RTL8211F PHY driver which only enabled
> the TX delay if requested (it never disabled the TX delay)
> - hardware defaults or pin strapping of the Realtek RTL8211F PHY
> enabled the TX delay in the PHY
> 
> This means that the TX delay was applied twice: once at the MAC and
> once at the PHY.
> That lead to high packet loss when transmitting data.
> To solve that I wrote the patch you mentioned, which has since been
> ported over to u-boot (for a non-Amlogic related board)
> 

Yeah. This is a standard problem if you ever worked with a hardware just
designed, when you try to make MAC+PHY working together. If you experienced
packets loss and it's RGMII, then most likely the problem with delays.

> > > > > 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.
> I don't have any datasheet for the Realtek RTL8211F PHY and I'm not in
> the position to get one (company contracts seem to be required for
> this).
> Linux is not my main job, I do driver development in my spare time.
> 
> there may or may not be a register or pin strapping to configure the RX delay.
> due to this I decided to leave the RX delay behavior "not defined"
> instead of rejecting RGMII_RXID and RGMII_ID.
> 
> > > > 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.
> the changes in patch 1 are looking good to me (except that I would use
> phy_modify_paged instead of open-coding it, functionally it's
> identical with what you have already)
> 

Nah, this isn't going to work since the config register is placed on an extension
page. So in order to reach the register first I needed to enable a standard page,
then select an extended page, then modify the register bits.

> I'm not sure about patch 2:
> personally I would wait for someone to come up with the requirement to
> use RGMII_RXID with a RTL8211F PHY.
> that person will then a board to test the changes and (hopefully) a
> datasheet to explain the RX delay situation with that PHY.
> that way we only change the RGMII_RXID behavior once (when someone
> requests support for it) instead of twice (now with your change, later
> on when someone needs RGMII_RXID support in the RTL8211F driver)
> 
> that said, the change in patch 2 itself looks fine on Amlogic boards
> (because all upstream .dts let the MAC generate the TX delay). I
> haven't runtime-tested your patch there yet.
> but there seem to be other boards (than the Amlogic ones, the RTL8211F
> PHY driver discussion in u-boot was not related to an Amlogic board)
> out there with a RTL8211F PHY (these may or may not be supported in
> mainline Linux or u-boot and may or may not use RGMII_RXID where you
> are now changing the behavior). that's not a problem by itself, but
> you should be aware of this.
> 
> [...]
> > rtl8211(e|f) TX/RX delays can be configured either by external pins
> > strapping or via software registers. This is one of the clue to provide
> > a proper config_init method code. But not all rtl8211f phys provide
> > that software register, and if they do it only concerns TX-delay (as we
> > aware of). So we need to take this into account when creating the updated
> > versions of these functions.
> >
> > (Martin, I also Cc'ed you in this discussion, so if you have anything to
> > say in this matter, please don't hesitate to comment.)
> Amlogic boards, such as the Hardkernel Odroid-C1 and Odroid-C2 as well
> as the Khadas VIM2 use a "RTL8211F" RGMII PHY. I don't know whether
> there are multiple versions of this PHY. all RTL8211F I have seen so
> far did behave exactly the same.
> 
> I also don't know whether the RX delay is configurable (by pin
> strapping or some register) on RTL8211F PHYs because I don't have
> access to the datasheet.
> 
> 
> Martin

Ok. Thanks for the comments. I am sure the RX-delay is configurable at list
via external RXD pin strapping at the chip powering up procedure. The only
problem with a way of software to change the setting.

I don't think there is going to be anyone revealing that realtek black boxed
registers layout anytime soon. So as I see it it's better to leave the
rtl8211f-part as is for now.

-Sergey

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

[Martin Blumenstingl]

Hi Serge,

On Mon, May 6, 2019 at 4:39 PM Serge Semin <fancer.lancer@gmail.com> wrote:
[...]
> > the changes in patch 1 are looking good to me (except that I would use
> > phy_modify_paged instead of open-coding it, functionally it's
> > identical with what you have already)
> >
>
> Nah, this isn't going to work since the config register is placed on an extension
> page. So in order to reach the register first I needed to enable a standard page,
> then select an extended page, then modify the register bits.
I'm probably missing something here. my understanding about
phy_modify_paged is that it is equal to:
- select extension page
- read register
- calculate the new register value
- write register
- restore the original extension page

if phy_modify_paged doesn't work for your use-case then ignore my comment.

[...]
> > > (Martin, I also Cc'ed you in this discussion, so if you have anything to
> > > say in this matter, please don't hesitate to comment.)
> > Amlogic boards, such as the Hardkernel Odroid-C1 and Odroid-C2 as well
> > as the Khadas VIM2 use a "RTL8211F" RGMII PHY. I don't know whether
> > there are multiple versions of this PHY. all RTL8211F I have seen so
> > far did behave exactly the same.
> >
> > I also don't know whether the RX delay is configurable (by pin
> > strapping or some register) on RTL8211F PHYs because I don't have
> > access to the datasheet.
> >
> >
> > Martin
>
> Ok. Thanks for the comments. I am sure the RX-delay is configurable at list
> via external RXD pin strapping at the chip powering up procedure. The only
> problem with a way of software to change the setting.
>
> I don't think there is going to be anyone revealing that realtek black boxed
> registers layout anytime soon. So as I see it it's better to leave the
> rtl8211f-part as is for now.
with the RTL8211F I was not sure whether interrupt support was
implemented correctly in the mainline driver.
I asked Realtek for more details:
initially they declined to send me a datasheet and referred me to my
"partner contact" (which I don't have because I'm doing this in my
spare time).
I explained that I am trying to improve the Linux driver for this PHY.
They gave me the relevant bits (about interrupt support) from the
datasheet (I never got the full datasheet though).

if you don't want to touch the RTL8211F part for now then I'm fine
with that as well


Regards
Martin

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

[Heiner Kallweit]

On 06.05.2019 19:21, Martin Blumenstingl wrote:
> Hi Serge,
> 
> On Mon, May 6, 2019 at 4:39 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> [...]
>>> the changes in patch 1 are looking good to me (except that I would use
>>> phy_modify_paged instead of open-coding it, functionally it's
>>> identical with what you have already)
>>>
>>
>> Nah, this isn't going to work since the config register is placed on an extension
>> page. So in order to reach the register first I needed to enable a standard page,
>> then select an extended page, then modify the register bits.
> I'm probably missing something here. my understanding about
> phy_modify_paged is that it is equal to:
> - select extension page
> - read register
> - calculate the new register value
> - write register
> - restore the original extension page
> 
What maybe causes the confusion: Realtek has two kinds of pages.
First there is the following, let's call it simple page:
You select a page via register 0x1f and then access the paged register.

Then there are extended pages. First you select a page via register 0x1f,
then the extended page via register 0x1e, and then the paged register.

> if phy_modify_paged doesn't work for your use-case then ignore my comment.
> 
> [...]
>>>> (Martin, I also Cc'ed you in this discussion, so if you have anything to
>>>> say in this matter, please don't hesitate to comment.)
>>> Amlogic boards, such as the Hardkernel Odroid-C1 and Odroid-C2 as well
>>> as the Khadas VIM2 use a "RTL8211F" RGMII PHY. I don't know whether
>>> there are multiple versions of this PHY. all RTL8211F I have seen so
>>> far did behave exactly the same.
>>>
>>> I also don't know whether the RX delay is configurable (by pin
>>> strapping or some register) on RTL8211F PHYs because I don't have
>>> access to the datasheet.
>>>
>>>
>>> Martin
>>
>> Ok. Thanks for the comments. I am sure the RX-delay is configurable at list
>> via external RXD pin strapping at the chip powering up procedure. The only
>> problem with a way of software to change the setting.
>>
>> I don't think there is going to be anyone revealing that realtek black boxed
>> registers layout anytime soon. So as I see it it's better to leave the
>> rtl8211f-part as is for now.
> with the RTL8211F I was not sure whether interrupt support was
> implemented correctly in the mainline driver.
> I asked Realtek for more details:
> initially they declined to send me a datasheet and referred me to my
> "partner contact" (which I don't have because I'm doing this in my
> spare time).
> I explained that I am trying to improve the Linux driver for this PHY.
> They gave me the relevant bits (about interrupt support) from the
> datasheet (I never got the full datasheet though).
> 
Same with me for the r8169 driver: They answer single questions quite
exact and fast, but no chance to get a datasheet or errata even for
10 yrs old chips.

> if you don't want to touch the RTL8211F part for now then I'm fine
> with that as well
> 
> 
> Regards
> Martin
> 
Heiner

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

[Martin Blumenstingl]

Hi Heiner,

On Tue, May 7, 2019 at 7:37 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 06.05.2019 19:21, Martin Blumenstingl wrote:
> > Hi Serge,
> >
> > On Mon, May 6, 2019 at 4:39 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > [...]
> >>> the changes in patch 1 are looking good to me (except that I would use
> >>> phy_modify_paged instead of open-coding it, functionally it's
> >>> identical with what you have already)
> >>>
> >>
> >> Nah, this isn't going to work since the config register is placed on an extension
> >> page. So in order to reach the register first I needed to enable a standard page,
> >> then select an extended page, then modify the register bits.
> > I'm probably missing something here. my understanding about
> > phy_modify_paged is that it is equal to:
> > - select extension page
> > - read register
> > - calculate the new register value
> > - write register
> > - restore the original extension page
> >
> What maybe causes the confusion: Realtek has two kinds of pages.
> First there is the following, let's call it simple page:
> You select a page via register 0x1f and then access the paged register.
>
> Then there are extended pages. First you select a page via register 0x1f,
> then the extended page via register 0x1e, and then the paged register.
I totally missed that, thank you for pointing me in the right direction!
that means I don't have anything obvious to complain about in patch 1.


Regards
Martin