Message-ID: <20211208171720.6a297011@thinkpad>
Date:   Wed, 8 Dec 2021 17:17:20 +0100
From:   Marek Behún <kabel@...nel.org>
To:     Holger Brunck <holger.brunck@...achienergy.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>
Subject: Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output
 amplitude configurable

On Wed, 8 Dec 2021 15:49:19 +0000
Holger Brunck <holger.brunck@...achienergy.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@...n.ch>
> > > > > CC: Jakub Kicinski <kuba@...nel.org>
> > > > > CC: Marek Behún <kabel@...nel.org>
> > > > > Signed-off-by: Holger Brunck <holger.brunck@...achienergy.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.

Marek

Powered by blists - more mailing lists