Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Andrew Lunn <andrew@lunn.ch>]

> > So this indicates any board might need this feature, not just this one
> > board. Putting the board name in the driver then does not scale.
> > 
> 
> Should I ignore this if I choose to use the following standard properties?

You should always follow standard properties unless they don't
work. And if they don't work, your commit message needs to explain why
they don't work forcing your to do something special.

> > > This means the time
> > > delay introduced by the PC board may not be zero. Therefore, it's necessary
> > > for software developers to tune both the RX programming swap bit and the
> > > delay to ensure correct sampling.
> > 
> > O.K. Now look at how other boards tune their delays. There are
> > standard properties for this:
> > 
> >          rx-internal-delay-ps:
> >            description:
> >              RGMII Receive Clock Delay defined in pico seconds. This is used for
> >              controllers that have configurable RX internal delays. If this
> >              property is present then the MAC applies the RX delay.
> >          tx-internal-delay-ps:
> >            description:
> >              RGMII Transmit Clock Delay defined in pico seconds. This is used for
> >              controllers that have configurable TX internal delays. If this
> >              property is present then the MAC applies the TX delay.
> > 
> > I think you can use these properties, maybe with an additional comment
> > in the binding. RGMII running at 1G has a clock of 125MHz. That is a
> > period of 8ns. So a half clock cycle delay is then 4ns.
> > 
> > So an rx-internal-delay-ps of 0-2000 means this clock invert should be
> > disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert
> > should be enabled.
> 
> This board was designed to operate at different speed rates, not a fixed
> speed, and the clock rate varies for each speed. Thus, the delay introduced
> by inverting the clock is not fixed. Additionally, I noticed that some
> vendors apply the same routine for this property across all speeds in their
> driver code. Can this property be used just as a flag, regardless of its
> actual value?

Maybe you should go read the RGMII standard, and then think about how
your hardware actually works.

RGMII always has a variable clock, with different clock speeds for
10/100/1G. So your board design is just plain normal, not
special. Does the standard talk about different delays for different
speeds? As you say, other drivers apply the same delay for all
speeds. Why should your hardware be special?

RGMII has been around for 25 years. Do you really think your RGMII
implementation needs something special which no other implementation
has needed in the last 25 years?

> > Now, ideally, you want the PHY to add the RGMII delays, that is what i
> > request all MAC/PHY pairs do, so we have a uniform setup across all
> > boards. So unless the PHY does not support RGMII delays, you would
> > expect rx-internal-delay-ps to be either just a small number of
> > picoseconds for fine tuning, or a small number of picoseconds + 4ns
> > for fine tuning.
> 
> The delay for both TX and RX sides is added by the MAC in the Qualcomm
> driver, which was introduced by the initial patch. I believe there may be a
> refactor in the future to ensure it follows the requirements.
 
You can do it in the MAC. But you probably want to clearly document
this, that your design is different to > 95% of systems in Linux which
have the PHY do the delays.

	Andrew