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