[PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Simon Horman]

According to the R-Car Gen3 Hardware Manual Errata for Rev 1.00 of
August 24, 2018, the TX clock internal delay mode isn't supported
on R-Car E3 (r8a77990) and D3 (r8a77995).

Based on work by Kazuya Mizuguchi.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb_main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 4f648394e645..be8af4a382cf 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1969,6 +1969,12 @@ static void ravb_set_config_mode(struct net_device *ndev)
 	}
 }
 
+static const struct soc_device_attribute ravb_delay_mode_quirk_match[] = {
+	{ .soc_id = "r8a77990", .revision = "ES1.*" },
+	{ .soc_id = "r8a77995", .revision = "ES1.*" },
+	{ /* sentinel */ }
+};
+
 /* Set tx and rx clock internal delay modes */
 static void ravb_set_delay_mode(struct net_device *ndev)
 {
@@ -1979,8 +1985,9 @@ static void ravb_set_delay_mode(struct net_device *ndev)
 	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
 		set |= APSR_DM_RDM;
 
-	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
-	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
+	if ((priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	     priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
+	    !soc_device_match(ravb_delay_mode_quirk_match))
 		set |= APSR_DM_TDM;
 
 	ravb_modify(ndev, APSR, APSR_DM, set);
-- 
2.11.0

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Mon, Apr 08, 2019 at 10:29:28AM +0200, Simon Horman wrote:
> According to the R-Car Gen3 Hardware Manual Errata for Rev 1.00 of
> August 24, 2018, the TX clock internal delay mode isn't supported
> on R-Car E3 (r8a77990) and D3 (r8a77995).

Yes, it made it also into the revised documentation v1.50, see chapter
50.2.7., bit 14.

> +static const struct soc_device_attribute ravb_delay_mode_quirk_match[] = {
> +	{ .soc_id = "r8a77990", .revision = "ES1.*" },
> +	{ .soc_id = "r8a77995", .revision = "ES1.*" },
> +	{ /* sentinel */ }
> +};

I might have missed it but is there a plan to fix this in later
revisions of D3/E3? I was under the impression that it is not and then
we could base it on compatible rather than soc_device_match?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Simon Horman]

On Mon, Apr 08, 2019 at 11:01:04AM +0200, Wolfram Sang wrote:
> On Mon, Apr 08, 2019 at 10:29:28AM +0200, Simon Horman wrote:
> > According to the R-Car Gen3 Hardware Manual Errata for Rev 1.00 of
> > August 24, 2018, the TX clock internal delay mode isn't supported
> > on R-Car E3 (r8a77990) and D3 (r8a77995).
> 
> Yes, it made it also into the revised documentation v1.50, see chapter
> 50.2.7., bit 14.
> 
> > +static const struct soc_device_attribute ravb_delay_mode_quirk_match[] = {
> > +	{ .soc_id = "r8a77990", .revision = "ES1.*" },
> > +	{ .soc_id = "r8a77995", .revision = "ES1.*" },
> > +	{ /* sentinel */ }
> > +};
> 
> I might have missed it but is there a plan to fix this in later
> revisions of D3/E3? I was under the impression that it is not and then
> we could base it on compatible rather than soc_device_match?

I am not aware of any such plan (or the absence of such a plan).

I was unsure weather to go with the compat approach of the quirk approach.
In the end I went with the quirk approach as it seems simpler. But
I'm happy to re-arrange things.

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

^
> > > +static const struct soc_device_attribute ravb_delay_mode_quirk_match[] = {
> > > +	{ .soc_id = "r8a77990", .revision = "ES1.*" },
> > > +	{ .soc_id = "r8a77995", .revision = "ES1.*" },
> > > +	{ /* sentinel */ }
> > > +};
> > 
> > I might have missed it but is there a plan to fix this in later
> > revisions of D3/E3? I was under the impression that it is not and then
> > we could base it on compatible rather than soc_device_match?
> 
> I am not aware of any such plan (or the absence of such a plan).
> 
> I was unsure weather to go with the compat approach of the quirk approach.
> In the end I went with the quirk approach as it seems simpler. But
> I'm happy to re-arrange things.

I see. Well, I don't care super much. The tiny drawback here is that we
will have a potentially broken D3/E3 ES2.0, if they have not fixed TXID
there. Then we need to update the above pattern. So, revision = "*" (or
the compatible approach) might be a tad better. Then we "only" have 1G
disabled for no reason until we whitelist it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Simon Horman]

On Mon, Apr 08, 2019 at 01:12:52PM +0200, Wolfram Sang wrote:
> ^
> > > > +static const struct soc_device_attribute ravb_delay_mode_quirk_match[] = {
> > > > +	{ .soc_id = "r8a77990", .revision = "ES1.*" },
> > > > +	{ .soc_id = "r8a77995", .revision = "ES1.*" },
> > > > +	{ /* sentinel */ }
> > > > +};
> > > 
> > > I might have missed it but is there a plan to fix this in later
> > > revisions of D3/E3? I was under the impression that it is not and then
> > > we could base it on compatible rather than soc_device_match?
> > 
> > I am not aware of any such plan (or the absence of such a plan).
> > 
> > I was unsure weather to go with the compat approach of the quirk approach.
> > In the end I went with the quirk approach as it seems simpler. But
> > I'm happy to re-arrange things.
> 
> I see. Well, I don't care super much. The tiny drawback here is that we
> will have a potentially broken D3/E3 ES2.0, if they have not fixed TXID
> there. Then we need to update the above pattern. So, revision = "*" (or
> the compatible approach) might be a tad better. Then we "only" have 1G
> disabled for no reason until we whitelist it.

I do think that the quirk approach is cleaner, So all things being equal
I'd slightly prefer to stick with that approach. Shall I drop
the .revision portion?

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]


> I do think that the quirk approach is cleaner, So all things being equal
> I'd slightly prefer to stick with that approach. Shall I drop
> the .revision portion?

Yup, sounds good to me. This way we can also properly describe if TXID
is fixed in a potential ES2.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Sergei Shtylyov]

On 04/08/2019 11:29 AM, Simon Horman wrote:

> According to the R-Car Gen3 Hardware Manual Errata for Rev 1.00 of
> August 24, 2018,

   Rummaged in Cogent's archives but was unable to find that errata...

> the TX clock internal delay mode isn't supported
> on R-Car E3 (r8a77990) and D3 (r8a77995).
> 
> Based on work by Kazuya Mizuguchi.
> 
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 4f648394e645..be8af4a382cf 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1969,6 +1969,12 @@ static void ravb_set_config_mode(struct net_device *ndev)
>  	}
>  }
>  
> +static const struct soc_device_attribute ravb_delay_mode_quirk_match[] = {
> +	{ .soc_id = "r8a77990", .revision = "ES1.*" },
> +	{ .soc_id = "r8a77995", .revision = "ES1.*" },
> +	{ /* sentinel */ }
> +};

   I'm OK with this approach (modulo the revisions)...

[...]
> @@ -1979,8 +1985,9 @@ static void ravb_set_delay_mode(struct net_device *ndev)
>  	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
>  		set |= APSR_DM_RDM;
>  
> -	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +	if ((priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	     priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
> +	    !soc_device_match(ravb_delay_mode_quirk_match))

   But don't we need to error out of the probing as we can't set the delay mode
requested?

>  		set |= APSR_DM_TDM;
>  
>  	ravb_modify(ndev, APSR, APSR_DM, set);

MBR, Sergei

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Andrew Lunn]

> > @@ -1979,8 +1985,9 @@ static void ravb_set_delay_mode(struct net_device *ndev)
> >  	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
> >  		set |= APSR_DM_RDM;
> >  
> > -	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > -	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > +	if ((priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +	     priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
> > +	    !soc_device_match(ravb_delay_mode_quirk_match))
> 
>    But don't we need to error out of the probing as we can't set the delay mode
> requested?

Yes, if we can, we should error out. It just depends on if there are
broken DT blobs out there. We recently had a lot of pain from broken
DT blobs using the at803x PHY and getting RGMII modes wrong. In the
long run, it is best to if DT, but i've no idea how many boards are
affected.

	Andrew

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Simon Horman]

On Mon, Apr 08, 2019 at 07:49:03PM +0200, Andrew Lunn wrote:
> > > @@ -1979,8 +1985,9 @@ static void ravb_set_delay_mode(struct net_device *ndev)
> > >  	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
> > >  		set |= APSR_DM_RDM;
> > >  
> > > -	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > > -	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > > +	if ((priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > > +	     priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
> > > +	    !soc_device_match(ravb_delay_mode_quirk_match))
> > 
> >    But don't we need to error out of the probing as we can't set the delay mode
> > requested?
> 
> Yes, if we can, we should error out. It just depends on if there are
> broken DT blobs out there. We recently had a lot of pain from broken
> DT blobs using the at803x PHY and getting RGMII modes wrong. In the
> long run, it is best to if DT, but i've no idea how many boards are
> affected.

Hi Andrew,

I suspect there are such blobs out there and I'm not sure
what the fall-out may or may not be.

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Sergei Shtylyov]

Hello!

On 04/09/2019 01:45 PM, Simon Horman wrote:

>>>> @@ -1979,8 +1985,9 @@ static void ravb_set_delay_mode(struct net_device *ndev)
>>>>  	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
>>>>  		set |= APSR_DM_RDM;
>>>>  
>>>> -	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
>>>> -	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
>>>> +	if ((priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
>>>> +	     priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
>>>> +	    !soc_device_match(ravb_delay_mode_quirk_match))
>>>
>>>    But don't we need to error out of the probing as we can't set the delay mode
>>> requested?
>>
>> Yes, if we can, we should error out. It just depends on if there are
>> broken DT blobs out there. We recently had a lot of pain from broken
>> DT blobs using the at803x PHY and getting RGMII modes wrong. In the
>> long run, it is best to if DT, but i've no idea how many boards are
>> affected.
> 
> Hi Andrew,
> 
> I suspect there are such blobs out there and I'm not sure
> what the fall-out may or may not be.

   You mean the out-of-tree blobs? Because I'm not seeing any in-kernel
breakage... 

MBR, Sergei

Re: [PATCH/RFC net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3

[Simon Horman]

On Tue, Apr 09, 2019 at 06:03:24PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 04/09/2019 01:45 PM, Simon Horman wrote:
> 
> >>>> @@ -1979,8 +1985,9 @@ static void ravb_set_delay_mode(struct net_device *ndev)
> >>>>  	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
> >>>>  		set |= APSR_DM_RDM;
> >>>>  
> >>>> -	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> >>>> -	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> >>>> +	if ((priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> >>>> +	     priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
> >>>> +	    !soc_device_match(ravb_delay_mode_quirk_match))
> >>>
> >>>    But don't we need to error out of the probing as we can't set the delay mode
> >>> requested?
> >>
> >> Yes, if we can, we should error out. It just depends on if there are
> >> broken DT blobs out there. We recently had a lot of pain from broken
> >> DT blobs using the at803x PHY and getting RGMII modes wrong. In the
> >> long run, it is best to if DT, but i've no idea how many boards are
> >> affected.
> > 
> > Hi Andrew,
> > 
> > I suspect there are such blobs out there and I'm not sure
> > what the fall-out may or may not be.
> 
>    You mean the out-of-tree blobs? Because I'm not seeing any in-kernel
> breakage... 

I am referring to older upstream blobs built from older upstream or
out-of-tree BSP kernel source where the phy mode is rgmii-txid or rgmii-id.
F.e. Ebisu and upstream v5.0 or out-of-tree R-Car Gen3 BSP v3.8.0.