| Message ID | 20190410114502.30362-1-horms+renesas@verge.net.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2,net-next] ravb: Avoid unsupported internal delay mode for R-Car E3/D3 | expand |
On Wed, Apr 10, 2019 at 01:45:02PM +0200, Simon Horman wrote: > According to the R-Car Gen3 Hardware Manual Rev 1.50 of Nov 30, 2018, the > TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 > (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). > > This matches all ES versions of the affected SoCs as it is > not clear if this problem will be resolved in newer chips. > This can be revisited, as necessary. > > This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or > PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay > mode is not supported as there is a risk of introducing a regression > when used in conjunction with older DT blobs present in the field. > > Based on work by Kazuya Mizuguchi. > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On Wed, Apr 10, 2019 at 01:45:02PM +0200, Simon Horman wrote: > According to the R-Car Gen3 Hardware Manual Rev 1.50 of Nov 30, 2018, the > TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 > (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). > > This matches all ES versions of the affected SoCs as it is > not clear if this problem will be resolved in newer chips. > This can be revisited, as necessary. > > This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or > PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay > mode is not supported as there is a risk of introducing a regression > when used in conjunction with older DT blobs present in the field. Hi Simon I think it should at least WARN_ON(). Such blobs are broken, even if they do kind of work. Andrew
On 04/10/2019 07:19 PM, Andrew Lunn wrote: >> According to the R-Car Gen3 Hardware Manual Rev 1.50 of Nov 30, 2018, the >> TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 >> (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). >> >> This matches all ES versions of the affected SoCs as it is >> not clear if this problem will be resolved in newer chips. >> This can be revisited, as necessary. >> >> This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or >> PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay >> mode is not supported as there is a risk of introducing a regression >> when used in conjunction with older DT blobs present in the field. > > Hi Simon > > I think it should at least WARN_ON(). Such blobs are broken, even if > they do kind of work. Good idea! Simon, third time's a charm? :-) > Andrew MBR, Sergei
On Wed, Apr 10, 2019 at 08:17:16PM +0300, Sergei Shtylyov wrote: > On 04/10/2019 07:19 PM, Andrew Lunn wrote: > > >> According to the R-Car Gen3 Hardware Manual Rev 1.50 of Nov 30, 2018, the > >> TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 > >> (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). > >> > >> This matches all ES versions of the affected SoCs as it is > >> not clear if this problem will be resolved in newer chips. > >> This can be revisited, as necessary. > >> > >> This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or > >> PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay > >> mode is not supported as there is a risk of introducing a regression > >> when used in conjunction with older DT blobs present in the field. > > > > Hi Simon > > > > I think it should at least WARN_ON(). Such blobs are broken, even if > > they do kind of work. > > Good idea! Simon, third time's a charm? :-) Sure, can do. > > > Andrew > > MBR, Sergei >
On Thu, Apr 11, 2019 at 10:40:27AM +0200, Simon Horman wrote: > On Wed, Apr 10, 2019 at 08:17:16PM +0300, Sergei Shtylyov wrote: > > On 04/10/2019 07:19 PM, Andrew Lunn wrote: > > > > >> According to the R-Car Gen3 Hardware Manual Rev 1.50 of Nov 30, 2018, the > > >> TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 > > >> (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). > > >> > > >> This matches all ES versions of the affected SoCs as it is > > >> not clear if this problem will be resolved in newer chips. > > >> This can be revisited, as necessary. > > >> > > >> This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or > > >> PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay > > >> mode is not supported as there is a risk of introducing a regression > > >> when used in conjunction with older DT blobs present in the field. > > > > > > Hi Simon > > > > > > I think it should at least WARN_ON(). Such blobs are broken, even if > > > they do kind of work. > > > > Good idea! Simon, third time's a charm? :-) > > Sure, can do. How about something like this? --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1980,8 +1987,14 @@ static void ravb_set_delay_mode(struct net_device *ndev) set |= APSR_DM_RDM; if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID || - priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) - set |= APSR_DM_TDM; + priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) { + if (soc_device_match(ravb_delay_mode_quirk_match)) + dev_warn(ndev->dev.parent, + "phy-mode %s requires TX clock internal delay mode which is not supported by this hardwre revision", + phy_modes(priv->phy_interface)); + else + set |= APSR_DM_TDM; + } ravb_modify(ndev, APSR, APSR_DM, set); }
On Thu, Apr 11, 2019 at 11:20:57AM +0200, Simon Horman wrote: > On Thu, Apr 11, 2019 at 10:40:27AM +0200, Simon Horman wrote: > > On Wed, Apr 10, 2019 at 08:17:16PM +0300, Sergei Shtylyov wrote: > > > On 04/10/2019 07:19 PM, Andrew Lunn wrote: > > > > > > >> According to the R-Car Gen3 Hardware Manual Rev 1.50 of Nov 30, 2018, the > > > >> TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 > > > >> (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). > > > >> > > > >> This matches all ES versions of the affected SoCs as it is > > > >> not clear if this problem will be resolved in newer chips. > > > >> This can be revisited, as necessary. > > > >> > > > >> This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or > > > >> PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay > > > >> mode is not supported as there is a risk of introducing a regression > > > >> when used in conjunction with older DT blobs present in the field. > > > > > > > > Hi Simon > > > > > > > > I think it should at least WARN_ON(). Such blobs are broken, even if > > > > they do kind of work. > > > > > > Good idea! Simon, third time's a charm? :-) > > > > Sure, can do. > > How about something like this? > > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1980,8 +1987,14 @@ static void ravb_set_delay_mode(struct net_device *ndev) > set |= APSR_DM_RDM; > > if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID || > - priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) > - set |= APSR_DM_TDM; > + priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) { > + if (soc_device_match(ravb_delay_mode_quirk_match)) > + dev_warn(ndev->dev.parent, > + "phy-mode %s requires TX clock internal delay mode which is not supported by this hardwre revision", > + phy_modes(priv->phy_interface)); Hi Simon The point of the warning is to tell users they should upgrade their DT blob to one that is not broken. So i think the message should say this. Also, we want users to notice this, which is why i said WARN_ON(). Something big so it gets noticed. Andrew
On 04/11/2019 04:33 PM, Andrew Lunn wrote: >>>>>> According to the R-Car Gen3 Hardware Manual Rev 1.50 of Nov 30, 2018, the >>>>>> TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 >>>>>> (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). >>>>>> >>>>>> This matches all ES versions of the affected SoCs as it is >>>>>> not clear if this problem will be resolved in newer chips. >>>>>> This can be revisited, as necessary. >>>>>> >>>>>> This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or >>>>>> PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay >>>>>> mode is not supported as there is a risk of introducing a regression >>>>>> when used in conjunction with older DT blobs present in the field. >>>>> >>>>> Hi Simon >>>>> >>>>> I think it should at least WARN_ON(). Such blobs are broken, even if >>>>> they do kind of work. >>>> >>>> Good idea! Simon, third time's a charm? :-) >>> >>> Sure, can do. >> >> How about something like this? >> >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -1980,8 +1987,14 @@ static void ravb_set_delay_mode(struct net_device *ndev) >> set |= APSR_DM_RDM; >> >> if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID || >> - priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) >> - set |= APSR_DM_TDM; >> + priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) { >> + if (soc_device_match(ravb_delay_mode_quirk_match)) >> + dev_warn(ndev->dev.parent, >> + "phy-mode %s requires TX clock internal delay mode which is not supported by this hardwre revision", >> + phy_modes(priv->phy_interface)); > > Hi Simon > > The point of the warning is to tell users they should upgrade their DT > blob to one that is not broken. So i think the message should say > this. Also, we want users to notice this, which is why i said > WARN_ON(). Something big so it gets noticed. I agree in general but I guess you meant WARN() -- WARN_ON() doesn't take a string arg... > Andrew MBR, Sergei
On Thu, Apr 11, 2019 at 07:36:35PM +0300, Sergei Shtylyov wrote: > On 04/11/2019 04:33 PM, Andrew Lunn wrote: > > >>>>>> According to the R-Car Gen3 Hardware Manual Rev 1.50 of Nov 30, 2018, the > >>>>>> TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 > >>>>>> (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). > >>>>>> > >>>>>> This matches all ES versions of the affected SoCs as it is > >>>>>> not clear if this problem will be resolved in newer chips. > >>>>>> This can be revisited, as necessary. > >>>>>> > >>>>>> This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or > >>>>>> PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay > >>>>>> mode is not supported as there is a risk of introducing a regression > >>>>>> when used in conjunction with older DT blobs present in the field. > >>>>> > >>>>> Hi Simon > >>>>> > >>>>> I think it should at least WARN_ON(). Such blobs are broken, even if > >>>>> they do kind of work. > >>>> > >>>> Good idea! Simon, third time's a charm? :-) > >>> > >>> Sure, can do. > >> > >> How about something like this? > >> > >> --- a/drivers/net/ethernet/renesas/ravb_main.c > >> +++ b/drivers/net/ethernet/renesas/ravb_main.c > >> @@ -1980,8 +1987,14 @@ static void ravb_set_delay_mode(struct net_device *ndev) > >> set |= APSR_DM_RDM; > >> > >> if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID || > >> - priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) > >> - set |= APSR_DM_TDM; > >> + priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) { > >> + if (soc_device_match(ravb_delay_mode_quirk_match)) > >> + dev_warn(ndev->dev.parent, > >> + "phy-mode %s requires TX clock internal delay mode which is not supported by this hardwre revision", > >> + phy_modes(priv->phy_interface)); > > > > Hi Simon > > > > The point of the warning is to tell users they should upgrade their DT > > blob to one that is not broken. So i think the message should say > > this. Also, we want users to notice this, which is why i said > > WARN_ON(). Something big so it gets noticed. > > I agree in general but I guess you meant WARN() -- WARN_ON() doesn't take > a string arg... Thanks, how about this? diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 4f648394e645..9618c4881c83 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1980,8 +1987,12 @@ static void ravb_set_delay_mode(struct net_device *ndev) set |= APSR_DM_RDM; if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID || - priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) - set |= APSR_DM_TDM; + priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) { + if (!WARN(soc_device_match(ravb_delay_mode_quirk_match), + "phy-mode %s requires TX clock internal delay mode which is not supported by this hardware revision. Please update device tree", + phy_modes(priv->phy_interface))) + set |= APSR_DM_TDM; + } ravb_modify(ndev, APSR, APSR_DM, set); }
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 4f648394e645..9618c4881c83 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1980,8 +1987,12 @@ static void ravb_set_delay_mode(struct net_device *ndev) > set |= APSR_DM_RDM; > > if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID || > - priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) > - set |= APSR_DM_TDM; > + priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) { > + if (!WARN(soc_device_match(ravb_delay_mode_quirk_match), > + "phy-mode %s requires TX clock internal delay mode which is not supported by this hardware revision. Please update device tree", > + phy_modes(priv->phy_interface))) > + set |= APSR_DM_TDM; > + } > > ravb_modify(ndev, APSR, APSR_DM, set); > } Hi Simon This looks good. Please add my: Reviewed-by: Andrew Lunn <andrew@lunn.ch> To the real patch. Andrew
On 04/15/2019 02:45 PM, Simon Horman wrote: >>>>>>>> According to the R-Car Gen3 Hardware Manual Rev 1.50 of Nov 30, 2018, the >>>>>>>> TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 >>>>>>>> (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). >>>>>>>> >>>>>>>> This matches all ES versions of the affected SoCs as it is >>>>>>>> not clear if this problem will be resolved in newer chips. >>>>>>>> This can be revisited, as necessary. >>>>>>>> >>>>>>>> This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or >>>>>>>> PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay >>>>>>>> mode is not supported as there is a risk of introducing a regression >>>>>>>> when used in conjunction with older DT blobs present in the field. >>>>>>> >>>>>>> Hi Simon >>>>>>> >>>>>>> I think it should at least WARN_ON(). Such blobs are broken, even if >>>>>>> they do kind of work. >>>>>> >>>>>> Good idea! Simon, third time's a charm? :-) >>>>> >>>>> Sure, can do. >>>> >>>> How about something like this? >>>> >>>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>>> @@ -1980,8 +1987,14 @@ static void ravb_set_delay_mode(struct net_device *ndev) >>>> set |= APSR_DM_RDM; >>>> >>>> if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID || >>>> - priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) >>>> - set |= APSR_DM_TDM; >>>> + priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) { >>>> + if (soc_device_match(ravb_delay_mode_quirk_match)) >>>> + dev_warn(ndev->dev.parent, >>>> + "phy-mode %s requires TX clock internal delay mode which is not supported by this hardwre revision", >>>> + phy_modes(priv->phy_interface)); >>> >>> Hi Simon >>> >>> The point of the warning is to tell users they should upgrade their DT >>> blob to one that is not broken. So i think the message should say >>> this. Also, we want users to notice this, which is why i said >>> WARN_ON(). Something big so it gets noticed. >> >> I agree in general but I guess you meant WARN() -- WARN_ON() doesn't take >> a string arg... > > Thanks, how about this? > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 4f648394e645..9618c4881c83 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1980,8 +1987,12 @@ static void ravb_set_delay_mode(struct net_device *ndev) > set |= APSR_DM_RDM; > > if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID || > - priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) > - set |= APSR_DM_TDM; > + priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) { > + if (!WARN(soc_device_match(ravb_delay_mode_quirk_match), > + "phy-mode %s requires TX clock internal delay mode which is not supported by this hardware revision. Please update device tree", > + phy_modes(priv->phy_interface))) > + set |= APSR_DM_TDM; > + } > > ravb_modify(ndev, APSR, APSR_DM, set); > } Looks fine, thanx! :-) MBR, Sergei
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 4f648394e645..99a12bf06d35 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1969,6 +1969,13 @@ static void ravb_set_config_mode(struct net_device *ndev) } } +static const struct soc_device_attribute ravb_delay_mode_quirk_match[] = { + { .soc_id = "r8a774c0" }, + { .soc_id = "r8a77990" }, + { .soc_id = "r8a77995" }, + { /* sentinel */ } +}; + /* Set tx and rx clock internal delay modes */ static void ravb_set_delay_mode(struct net_device *ndev) { @@ -1979,8 +1986,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 Rev 1.50 of Nov 30, 2018, the TX clock internal delay mode isn't supported on R-Car E3 (r8a77990) or D3 (r8a77995). And by extension it is also not supported by RZ/G2E (r9a774c0). This matches all ES versions of the affected SoCs as it is not clear if this problem will be resolved in newer chips. This can be revisited, as necessary. This patch does not error-out if PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_TXID are used on SoCs where TX clock delay mode is not supported as there is a risk of introducing a regression when used in conjunction with older DT blobs present in the field. Based on work by Kazuya Mizuguchi. Signed-off-by: Simon Horman <horms+renesas@verge.net.au> --- v2 * Dropped RFC designation * Also correct RZ/G2E (r8a774c0) * Remove revision (ES version) portion of soc match as it is not clear this problem will be resolved in newer chips. * Refer to user manual v1.50 rather than errata for v1.00 in changelog * Note absence of error handling in Changelog --- drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)