[BISECTED, REGRESSION] Broken networking on MIPS/OCTEON EdgeRouter Lite

[BISECTED, REGRESSION] Broken networking on MIPS/OCTEON EdgeRouter Lite

[Aaro Koskinen]

Hi,

When booting v5.1-rc1 on EdgeRouter Lite (MIPS/OCTEON), with at803x phy
driver enabled, networking no longer works - I even need to go physically
power cycle the board before getting networking to work again (otherwise
bootloader cannot tftp an older working image).

Bisected to:

	commit 6d4cd041f0af5b4c8fc742b4a68eac22e420e28c
	Author: Vinod Koul <vkoul@kernel.org>
	Date:   Thu Feb 21 15:53:15 2019 +0530

	    net: phy: at803x: disable delay only for RGMII mode

Booting v5.1-rc1 with this commit reverted makes networking to work
fine again.

A.

Re: [BISECTED, REGRESSION] Broken networking on MIPS/OCTEON EdgeRouter Lite

[Vinod Koul]

On 22-03-19, 02:21, Aaro Koskinen wrote:
> Hi,
> 
> When booting v5.1-rc1 on EdgeRouter Lite (MIPS/OCTEON), with at803x phy
> driver enabled, networking no longer works - I even need to go physically
> power cycle the board before getting networking to work again (otherwise
> bootloader cannot tftp an older working image).
> 
> Bisected to:
> 
> 	commit 6d4cd041f0af5b4c8fc742b4a68eac22e420e28c
> 	Author: Vinod Koul <vkoul@kernel.org>
> 	Date:   Thu Feb 21 15:53:15 2019 +0530
> 
> 	    net: phy: at803x: disable delay only for RGMII mode

Hello,

So with cd28d1d6e52e ("net: phy: at803x: Disable phy delay for RGMII
mode") it works for you but not 6d4cd041f0af ("net: phy: at803x: disable
delay only for RGMII mode"). That is bit more weird case :)

So does the ethernet expect RGMII mode or RGMII_ID mode here, looks like
disable delay is expected as well?

Can you point me to the DT node as well..

> Booting v5.1-rc1 with this commit reverted makes networking to work
> fine again.

-- 
~Vinod

Re: [BISECTED, REGRESSION] Broken networking on MIPS/OCTEON EdgeRouter Lite

[Aaro Koskinen]

Hi,

On Fri, Mar 22, 2019 at 12:15:06PM +0530, Vinod Koul wrote:
> On 22-03-19, 02:21, Aaro Koskinen wrote:
> > When booting v5.1-rc1 on EdgeRouter Lite (MIPS/OCTEON), with at803x phy
> > driver enabled, networking no longer works - I even need to go physically
> > power cycle the board before getting networking to work again (otherwise
> > bootloader cannot tftp an older working image).
> > 
> > Bisected to:
> > 
> > 	commit 6d4cd041f0af5b4c8fc742b4a68eac22e420e28c
> > 	Author: Vinod Koul <vkoul@kernel.org>
> > 	Date:   Thu Feb 21 15:53:15 2019 +0530
> > 
> > 	    net: phy: at803x: disable delay only for RGMII mode
> 
> Hello,
> 
> So with cd28d1d6e52e ("net: phy: at803x: Disable phy delay for RGMII
> mode") it works for you but not 6d4cd041f0af ("net: phy: at803x: disable
> delay only for RGMII mode"). That is bit more weird case :)

Yes, I guess it's the new "disable by default" behaviour that breaks it.

> So does the ethernet expect RGMII mode or RGMII_ID mode here, looks like
> disable delay is expected as well?

The OCTEON HW code knows only about RGMII. And looking at
octeon ethernet staging driver it does phy connect always with
PHY_INTERFACE_MODE_GMII. I did some experimentation, and it seems that
with PHY_INTERFACE_MODE_RGMII_RXID it starts to work.. In the DT we have
for ethernet for this board:

	rx-delay = <0>;
	tx-delay = <0x10>;

which I guess matches, or does this make any sense?

> Can you point me to the DT node as well..

arch/mips/boot/dts/cavium-octeon/ubnt_e100.dts

A.

Re: [BISECTED, REGRESSION] Broken networking on MIPS/OCTEON EdgeRouter Lite

[Andrew Lunn]

> The OCTEON HW code knows only about RGMII. And looking at
> octeon ethernet staging driver it does phy connect always with
> PHY_INTERFACE_MODE_GMII. I did some experimentation, and it seems that
> with PHY_INTERFACE_MODE_RGMII_RXID it starts to work.. In the DT we have
> for ethernet for this board:
> 
> 	rx-delay = <0>;
> 	tx-delay = <0x10>;

These are not PHY properties. 

Looking at the code, it looks like these control delays the MAC
inserts. I don't see a binding document for these properties, so i've
no idea what 0x10 means. Before this driver moves out of staging,
these values should be changed to be in ns.

However, PHY_INTERFACE_MODE_RGMII_RXID would make sense if 0x10 is
sufficient to add the TX delay.

What the driver should however do is call of_of_get_phy_mode() to get
the phy-mode from the DT blob and pass that to of_phy_connect().

    Andrew

Re: [BISECTED, REGRESSION] Broken networking on MIPS/OCTEON EdgeRouter Lite

[Aaro Koskinen]

Hi,

On Fri, Mar 22, 2019 at 10:25:57PM +0100, Andrew Lunn wrote:
> > The OCTEON HW code knows only about RGMII. And looking at
> > octeon ethernet staging driver it does phy connect always with
> > PHY_INTERFACE_MODE_GMII. I did some experimentation, and it seems that
> > with PHY_INTERFACE_MODE_RGMII_RXID it starts to work.. In the DT we have
> > for ethernet for this board:
> > 
> > 	rx-delay = <0>;
> > 	tx-delay = <0x10>;
> 
> These are not PHY properties. 
> 
> Looking at the code, it looks like these control delays the MAC
> inserts. I don't see a binding document for these properties, so i've
> no idea what 0x10 means. Before this driver moves out of staging,
> these values should be changed to be in ns.

Documentation/devicetree/bindings/net/cavium-pip.txt

Not sure how I could figure out the ns values?

> However, PHY_INTERFACE_MODE_RGMII_RXID would make sense if 0x10 is
> sufficient to add the TX delay.
> 
> What the driver should however do is call of_of_get_phy_mode() to get
> the phy-mode from the DT blob and pass that to of_phy_connect().

OK, thanks, I'll try to work on this.

A.

Re: [BISECTED, REGRESSION] Broken networking on MIPS/OCTEON EdgeRouter Lite

[Andrew Lunn]

On Fri, Mar 22, 2019 at 11:41:20PM +0200, Aaro Koskinen wrote:
> Hi,
> 
> On Fri, Mar 22, 2019 at 10:25:57PM +0100, Andrew Lunn wrote:
> > > The OCTEON HW code knows only about RGMII. And looking at
> > > octeon ethernet staging driver it does phy connect always with
> > > PHY_INTERFACE_MODE_GMII. I did some experimentation, and it seems that
> > > with PHY_INTERFACE_MODE_RGMII_RXID it starts to work.. In the DT we have
> > > for ethernet for this board:
> > > 
> > > 	rx-delay = <0>;
> > > 	tx-delay = <0x10>;
> > 
> > These are not PHY properties. 
> > 
> > Looking at the code, it looks like these control delays the MAC
> > inserts. I don't see a binding document for these properties, so i've
> > no idea what 0x10 means. Before this driver moves out of staging,
> > these values should be changed to be in ns.
> 
> Documentation/devicetree/bindings/net/cavium-pip.txt

Hi Aaro

Ah, sorry, missed that.

- rx-delay: Delay value for RGMII receive clock. Optional. Disabled if 0.
  Value range is 1-31, and mapping to the actual delay varies depending on HW.

- tx-delay: Delay value for RGMII transmit clock. Optional. Disabled if 0.
  Value range is 1-31, and mapping to the actual delay varies depending on HW.

I'm surprised this made it passed review. We try to avoid having DT
poke magic values into registers. That is what this appears to be,
since it is unclear what the value actually means.

It is also good to state what happens if the property is not
present. It often means it defaults to zero. But this implementation
just leaves the value alone. So to be on the safe side, the DT blob
should probably have these properties, so the behaviour is well
defined.

	 Andrew

Re: [BISECTED, REGRESSION] Broken networking on MIPS/OCTEON EdgeRouter Lite

[Aaro Koskinen]

Hi,

On Sun, Mar 24, 2019 at 09:17:34PM +0100, Andrew Lunn wrote:
> On Fri, Mar 22, 2019 at 11:41:20PM +0200, Aaro Koskinen wrote:
> > On Fri, Mar 22, 2019 at 10:25:57PM +0100, Andrew Lunn wrote:
> > > > The OCTEON HW code knows only about RGMII. And looking at
> > > > octeon ethernet staging driver it does phy connect always with
> > > > PHY_INTERFACE_MODE_GMII. I did some experimentation, and it seems that
> > > > with PHY_INTERFACE_MODE_RGMII_RXID it starts to work.. In the DT we have
> > > > for ethernet for this board:
> > > > 
> > > > 	rx-delay = <0>;
> > > > 	tx-delay = <0x10>;
> > > 
> > > These are not PHY properties. 
> > > 
> > > Looking at the code, it looks like these control delays the MAC
> > > inserts. I don't see a binding document for these properties, so i've
> > > no idea what 0x10 means. Before this driver moves out of staging,
> > > these values should be changed to be in ns.
> > 
> > Documentation/devicetree/bindings/net/cavium-pip.txt
> 
> Hi Aaro
> 
> Ah, sorry, missed that.
> 
> - rx-delay: Delay value for RGMII receive clock. Optional. Disabled if 0.
>   Value range is 1-31, and mapping to the actual delay varies depending on HW.
> 
> - tx-delay: Delay value for RGMII transmit clock. Optional. Disabled if 0.
>   Value range is 1-31, and mapping to the actual delay varies depending on HW.
> 
> I'm surprised this made it passed review. We try to avoid having DT
> poke magic values into registers. That is what this appears to be,
> since it is unclear what the value actually means.

It's probably not too late to improve this since those are
likely used only in-tree. Vendor GPL bundles provide some more
documentation for this setting, as does the FreeBSD source tree:
https://github.com/freebsd/freebsd/blob/master/sys/contrib/octeon-sdk/cvmx-asxx-defs.h#L911

But reading that, I think it could be still difficult to change it to
rx-delay-ps as it seems to be too coarse/unreliable.

> It is also good to state what happens if the property is not
> present. It often means it defaults to zero. But this implementation
> just leaves the value alone. So to be on the safe side, the DT blob
> should probably have these properties, so the behaviour is well
> defined.

The default is to set both to 16 on CN50XX and 24 on other chips.

A.