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

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(-)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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