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

["Marek Behún" <kabel@kernel.org>]

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