Re: [net-next PATCH 10/13] net: dsa: qca8k: add explicit SGMII PLL enable

On Thu, Oct 07, 2021 at 08:05:56PM +0200, Andrew Lunn wrote:

On Thu, Oct 07, 2021 at 03:35:54PM +0200, Ansuel Smith wrote:

quoted

On Thu, Oct 07, 2021 at 02:29:46AM +0200, Andrew Lunn wrote:

quoted

On Thu, Oct 07, 2021 at 12:36:00AM +0200, Ansuel Smith wrote:

quoted

Support enabling PLL on the SGMII CPU port. Some device require this
special configuration or no traffic is transmitted and the switch
doesn't work at all. A dedicated binding is added to the CPU node
port to apply the correct reg on mac config.

Why not just enable this all the time when the CPU port is in SGMII
mode?

I don't know if you missed the cover letter with the reason. Sgmii PLL
is a mess. Some device needs it and some doesn't. With a wrong
configuration the result is not traffic. As it's all messy we decided to
set the PLL to be enabled with a dedicated binding and set it disabled
by default. We enouncer more device that require it disabled than device
that needs it enabled. (in the order of 70 that doesn't needed it and 2
that requires it enabled or port instability/no traffic/leds problem)

What exactly does this PLL do? Clock recovery of the SGMII clock, and
then using it in the opposite direction? What combinations of PHYs
need it, and which don't?

I will quote from Documentation
bit 1: enable SGMII PLL (it's disabled by default)
bit 2: enable rx chain. By default it's disabled and CLK125M_RX and
DOUT_RX can be any logic of 1 or 0
bit 3: enable TX driver. By default is in idle and kept in 900mV
As you can see normally all these bit are disabled and I think forcing
them on seems wrong.
We can think about setting it based on the switch type and revision but
again we found some that needed it anyway and goes out of any logic.
The original idea is to add a biding to force the setting and make the
driver decide itself the correct option. But considering we found that
in 90% of the case, this is not needed, the switch default doesn't
enable it and that only 2-3 device require it, I see the binding the
only way to implement this and do not pollute the code with tons of
condition.

quoted

quoted

Is it also needed for 1000BaseX?

We assume it really depends on the device.

That i find surprising. 1000BaseX and SGMII are very similar. I would
expect a device with requires the PLL enabled for SGMII also needs it
for 1000BaseX.

With assume I mean we have no device with 1000BaseX (and I honestly
don't know if it does exist) I think if it does require PLL for sgmii
then it does require it also for 1000BaseX

quoted

quoted

DT properties like this are hard to use. It would be better if the
switch can decide for itself if it needs the PLL enabled.

Again reason in the cover letter sgmii part. Some qca driver have some
logic based on switch revision. We tried that and it didn't work since
some device had no traffic with pll enabled (and with the revision set
to enable pll)

This is my main problem with this patchset. You are adding lots of
poorly documented properties which are proprietary to this switch. And
you are saying, please try all 2^N combinations and see what works
best. That is not very friendly at all.

We have many device and we notice a common config for both qca8327 and
qca8337. Wonder if this would be improved by adding some hint in the
documentation on the correct configuration based on the switch model?
We waited so much to push this just to check if all this bad stuff was
actually needed or could be implemented differently, but we really find
any logic. So I think the only way is ""spam"" the documentation with
data on how to decide the correct option.

So it would be good to explain each one in detail. Maybe given the
explanation, we can figure out a way to detect at runtime, and not
need the option. If not, you can add it to the DT binding to help
somebody pick a likely starting point for the 2^N search.

	 Andrew

-- 
	Ansuel