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

Marek Behún
2021-12-08T16:21:13Z

Hello Vladimir,

On Wed, 8 Dec 2021 17:17:20 +0100
Marek Behún <kabel@kernel.org> wrote:

> On Wed, 8 Dec 2021 15:49:19 +0000
> Holger Brunck <holger.brunck@hitachienergy.com> wrote:
>
> > > >
> > > > > > The mv88e6352, mv88e6240 and mv88e6176 have a serdes interface.
> > > > > > This patch allows to configure the output swing to a desired value
> > > > > > in the devicetree node of the port. As the chips only supports
> > > > > > eight dedicated values we return EINVAL if the value in the DTS does not
> > > match.
> > > > > >
> > > > > > CC: Andrew Lunn <andrew@lunn.ch>
> > > > > > CC: Jakub Kicinski <kuba@kernel.org>
> > > > > > CC: Marek Behún <kabel@kernel.org>
> > > > > > Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
> > > > >
> > > > > Holger, Andrew,
> > > > >
> > > > > there is another issue with this, which I only realized yesterday.
> > > > > What if the different amplitude needs to be set only for certain SerDes
> > > modes?
> > > > >
> > > > > I am bringing this up because I discovered that on Turris Mox we
> > > > > need to increase SerDes output amplitude when A3720 SOC is connected
> > > > > directly to
> > > > > 88E6141 switch, but only for 2500base-x mode. For 1000base-x, the
> > > > > default amplitude is okay. (Also when the SOC is connected to
> > > > > 88E6190, the amplitude does not need to be changed at all.)
> > > > >
> > > >
> > > > on my board I have a fixed link connected with SGMII and there is no
> > > > dedicated value given in the manual.
> > > >
> > > > > I plan to solve this in the comphy driver, not in device-tree.
> > > > >
> > > > > But if the solution is to be done in DTS, shouldn't there be a
> > > > > possibility to define the amplitude for a specific serdes mode only?
> > > > >
> > > > > For example
> > > > > serdes-2500base-x-tx-amplitude-millivolt
> > > > > or
> > > > > serdes-tx-amplitude-millivolt-2500base-x
> > > > > or
> > > > > serdes-tx-amplitude-millivolt,2500base-x
> > > > > ?
> > > > >
> > > > > What do you think?
> > > > >
> > > >
> > > > in the data sheet for the MV6352 I am using there are no dedicated
> > > > values stated for different modes at all, they can be chosen
> > > > arbitrary. So in my case I think it makes sense to keep it as it is
> > > > for now. Other driver may have other needs and may enhance this later on.
> > >
> > > Hi Holger,
> > >
> > > 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.

Marek