[PATCH] net: thunderx: use proper interface type for RGMII

[PATCH] net: thunderx: use proper interface type for RGMII

[Tim Harvey]

The configuration of the OCTEONTX XCV_DLL_CTL register via
xcv_init_hw() is such that the RGMII RX delay is bypassed
leaving the RGMII TX delay enabled in the MAC:

	/* Configure DLL - enable or bypass
	 * TX no bypass, RX bypass
	 */
	cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL);
	cfg &= ~0xFF03;
	cfg |= CLKRX_BYP;
	writeq_relaxed(cfg, xcv->reg_base + XCV_DLL_CTL);

This would coorespond to a interface type of PHY_INTERFACE_MODE_RGMII_RXID
and not PHY_INTERFACE_MODE_RGMII.

Fixing this allows RGMII PHY drivers to do the right thing (enable
RX delay in the PHY) instead of erroneously enabling both delays in the
PHY.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index c4f6ec0..17a4110 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
@@ -1039,7 +1039,7 @@ static int phy_interface_mode(u8 lmac_type)
 	if (lmac_type == BGX_MODE_QSGMII)
 		return PHY_INTERFACE_MODE_QSGMII;
 	if (lmac_type == BGX_MODE_RGMII)
-		return PHY_INTERFACE_MODE_RGMII;
+		return PHY_INTERFACE_MODE_RGMII_RXID;
 
 	return PHY_INTERFACE_MODE_SGMII;
 }
-- 
2.7.4

Re: [PATCH] net: thunderx: use proper interface type for RGMII

[Andrew Lunn]

On Fri, Feb 07, 2020 at 12:40:26PM -0800, Tim Harvey wrote:
> The configuration of the OCTEONTX XCV_DLL_CTL register via
> xcv_init_hw() is such that the RGMII RX delay is bypassed
> leaving the RGMII TX delay enabled in the MAC:
> 
> 	/* Configure DLL - enable or bypass
> 	 * TX no bypass, RX bypass
> 	 */
> 	cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL);
> 	cfg &= ~0xFF03;
> 	cfg |= CLKRX_BYP;
> 	writeq_relaxed(cfg, xcv->reg_base + XCV_DLL_CTL);
> 
> This would coorespond to a interface type of PHY_INTERFACE_MODE_RGMII_RXID
> and not PHY_INTERFACE_MODE_RGMII.
> 
> Fixing this allows RGMII PHY drivers to do the right thing (enable
> RX delay in the PHY) instead of erroneously enabling both delays in the
> PHY.

Hi Tim

This seems correct. But how has it worked in the past? Does this
suggest there is PHY driver out there which is doing the wrong thing
when passed PHY_INTERFACE_MODE_RGMII?

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

    Andrew

Re: [PATCH] net: thunderx: use proper interface type for RGMII

[David Miller]

From: Tim Harvey <tharvey@gateworks.com>
Date: Fri,  7 Feb 2020 12:40:26 -0800

> The configuration of the OCTEONTX XCV_DLL_CTL register via
> xcv_init_hw() is such that the RGMII RX delay is bypassed
> leaving the RGMII TX delay enabled in the MAC:
> 
> 	/* Configure DLL - enable or bypass
> 	 * TX no bypass, RX bypass
> 	 */
> 	cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL);
> 	cfg &= ~0xFF03;
> 	cfg |= CLKRX_BYP;
> 	writeq_relaxed(cfg, xcv->reg_base + XCV_DLL_CTL);
> 
> This would coorespond to a interface type of PHY_INTERFACE_MODE_RGMII_RXID
> and not PHY_INTERFACE_MODE_RGMII.
> 
> Fixing this allows RGMII PHY drivers to do the right thing (enable
> RX delay in the PHY) instead of erroneously enabling both delays in the
> PHY.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

Applied, thanks.

Re: [PATCH] net: thunderx: use proper interface type for RGMII

[Tim Harvey]

On Fri, Feb 7, 2020 at 1:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Feb 07, 2020 at 12:40:26PM -0800, Tim Harvey wrote:
> > The configuration of the OCTEONTX XCV_DLL_CTL register via
> > xcv_init_hw() is such that the RGMII RX delay is bypassed
> > leaving the RGMII TX delay enabled in the MAC:
> >
> >       /* Configure DLL - enable or bypass
> >        * TX no bypass, RX bypass
> >        */
> >       cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL);
> >       cfg &= ~0xFF03;
> >       cfg |= CLKRX_BYP;
> >       writeq_relaxed(cfg, xcv->reg_base + XCV_DLL_CTL);
> >
> > This would coorespond to a interface type of PHY_INTERFACE_MODE_RGMII_RXID
> > and not PHY_INTERFACE_MODE_RGMII.
> >
> > Fixing this allows RGMII PHY drivers to do the right thing (enable
> > RX delay in the PHY) instead of erroneously enabling both delays in the
> > PHY.
>
> Hi Tim
>
> This seems correct. But how has it worked in the past? Does this
> suggest there is PHY driver out there which is doing the wrong thing
> when passed PHY_INTERFACE_MODE_RGMII?
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>

Andrew,

Yes, the DP83867 phy driver used on the Gateworks Newport boards would
configure the delay in an incompatible way when enabled.

Tim

Re: [PATCH] net: thunderx: use proper interface type for RGMII

[Andrew Lunn]

On Wed, Feb 12, 2020 at 08:55:39AM -0800, Tim Harvey wrote:
> On Fri, Feb 7, 2020 at 1:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Fri, Feb 07, 2020 at 12:40:26PM -0800, Tim Harvey wrote:
> > > The configuration of the OCTEONTX XCV_DLL_CTL register via
> > > xcv_init_hw() is such that the RGMII RX delay is bypassed
> > > leaving the RGMII TX delay enabled in the MAC:
> > >
> > >       /* Configure DLL - enable or bypass
> > >        * TX no bypass, RX bypass
> > >        */
> > >       cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL);
> > >       cfg &= ~0xFF03;
> > >       cfg |= CLKRX_BYP;
> > >       writeq_relaxed(cfg, xcv->reg_base + XCV_DLL_CTL);
> > >
> > > This would coorespond to a interface type of PHY_INTERFACE_MODE_RGMII_RXID
> > > and not PHY_INTERFACE_MODE_RGMII.
> > >
> > > Fixing this allows RGMII PHY drivers to do the right thing (enable
> > > RX delay in the PHY) instead of erroneously enabling both delays in the
> > > PHY.
> >
> > Hi Tim
> >
> > This seems correct. But how has it worked in the past? Does this
> > suggest there is PHY driver out there which is doing the wrong thing
> > when passed PHY_INTERFACE_MODE_RGMII?
> >
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> >
> 
> Andrew,
> 
> Yes, the DP83867 phy driver used on the Gateworks Newport boards would
> configure the delay in an incompatible way when enabled.

Hi Tim

So it was broken? Maybe find the appropriate Fixes tag, and have David
add it to stable?

    Andrew