Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

On Wed, 8 Dec 2021 18:49:32 +0200
Vladimir Oltean [off-list ref] wrote:

On Wed, Dec 08, 2021 at 06:41:31PM +0200, Vladimir Oltean wrote:

quoted

On Wed, Dec 08, 2021 at 05:21:04PM +0100, Marek Behún wrote:  

quoted

Hello Vladimir,  

quoted

quoted

quoted

but the mv88e6xxx driver also drives switches that allow changing serdes
modes. There does not need be dedicated TX amplitude register for each serdes
mode, the point is that we may want to declare different amplitudes for
different modes.

So the question is: if we go with your binding proposal for the whole mv88e6xxx
driver, and in the future someone will want to declare different amplitudes for
different modes on another model, would he need to deprecate your binding or
would it be easy to extend?
 

ok I see. So if I follow your proposal in my case it would be something like:
serdes-sgmii-tx-amplitude-millivolt to start with ?

I can do that. Andrew what do you think?  

Or maybe two properties:
  serdes-tx-amplitude-millivolt = <700 1000 1100>;
  serdes-tx-amplitude-modes = "sgmii", "2500base-x", "10gbase-r";
?

If
  serdes-tx-amplitude-modes
is omitted, then
  serdes-tx-amplitude-millivolt
should only contain one value, and this is used for all serdes modes.

This would be compatible with your change. You only need to define the
bidning for now, your code can stay the same - you don't need to add
support for multiple values or for the second property now, it can be
done later when needed. But the binding should be defined to support
those different modes.  

Vladimir, can you send your thoughts about this proposal? We are trying
to propose binding for defining serdes TX amplitude.  

I don't have any specific concern here. It sounds reasonable for
different data rates to require different transmitter configurations.
Having separate "serdes-tx-amplitude-millivolt" and "serdes-tx-amplitude-modes"
properties sounds okay, although I think a prefix with "-names" at the
end is more canonical ("pinctrl-names", "clock-names", "reg-names" etc),
so maybe "serdes-tx-amplitude-millivolt-names"?
Maybe we could name the first element "default", and just the others
would be named after a phy-mode. This way, if a specific TX amplitude is
found in the device tree for the currently operating PHY mode, it can be
used, otherwise the default (first) amplitude can be used.  

Also, maybe drop the "serdes-" prefix? The property will sit under a
SERDES lane node, so it would be a bit redundant?

Hmm. Holger's proposal adds the property into the port node, not SerDes
lane node. mv88e6xxx does not define bindings for SerDes lane nodes
(yet).