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

On Wed, 8 Dec 2021 18:10:09 +0100
Marek Behún [off-list ref] wrote:

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

quoted

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

quoted

quoted

quoted

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.    

Yes, the pair
  serdes-tx-amplitude-millivolt
  serdes-tx-amplitude-millivolt-names
is the best.

If the second is not defined, the first should contain only one value,
and that is used as default.

If multiple values are defined, but "default" is not, the driver should
set default value as the default value of the corresponding register.

The only remaining question is this: I need to implement this also for
comphy driver. In this case, the properties should be defined in the
comphy node, not in the MAC node. But the comphy also supports PCIe,
USB3 and SATA modes. We don't have strings for them. So this will need
to be extended in the future.

But for now this proposal seems most legit. I think the properties
should be defined in common PHY bindings, and other bindings should
refer to them via $ref.    

I wouldn't $ref the tx-amplitude-millivolt-names from the phy-mode,
because (a) not all phy-mode values are valid (think of parallel interfaces
like rgmii) and (b) because sata, pcie, usb are also valid SERDES
protocols as you point out. With the risk of a bit of duplication, I
think I'd keep the SERDES protocol names a separate thing for the YAML
validator.  

Not what I meant. What I meant was that the tx-amplitude-millivolt*
properties should be defined in binding for common PHY (not network PHY)
  Documentation/devicetree/bindings/phy/phy-bindings.txt,
and then the mv88e6xxx binding should refer it's
tx-amplitude-millivolt* properties from there.

And the definition in common PHY binding should list all modes in an
enum, containing all network SerDes modes, plus the other modes like
PCIe, USB3, DisplayPort, LVDS, SATA, ...

Marek

Note that one PHY binding already defines such property:
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/phy/brcm,sata-phy.yaml?h=v5.16-rc4#n86

So I think we need to make it generic.