| Message ID | 20190408082928.22840-1-horms+renesas@verge.net.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [PATCH/RFC,net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3 | expand |
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?
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.
^ > > > +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.
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?
> 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
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
> > @@ -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
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.
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
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.
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);
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(-)