Date:   Mon, 18 Feb 2019 10:43:02 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Antoine Tenart <antoine.tenart@...tlin.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
        maxime.chevallier@...tlin.com, gregory.clement@...tlin.com,
        miquel.raynal@...tlin.com, nadavh@...vell.com, stefanc@...vell.com,
        ymarkman@...vell.com, mw@...ihalf.com
Subject: Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while
 reconfiguring the serdes lanes

On Mon, Feb 18, 2019 at 11:26:30AM +0100, Antoine Tenart wrote:
> Hi Russell,
> 
> On Fri, Feb 15, 2019 at 05:12:24PM +0000, Russell King - ARM Linux admin wrote:
> > On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote:
> > > The documentation advises to set the XPCS in reset while reconfiguring
> > > the serdes lanes. This seems to be a good thing to do, but the PPv2
> > > driver wasn't doing it. This patch fixes it.
> > 
> > Hmm.  That statment seems to have some ambiguity in it - we do two
> > "reconfigurations" - one may be upon initialisation, where the lane
> > is already configured for 10Gbase-KR, and we're re-initialising it
> > for the same mode.  The other case is when we're switching between
> > 10Gbase-KR and SGMII, or as will be the case with 2.5G support for
> > the Alaska PHYs, 2500base-X.
> 
> The configuration at the lane at boot time is dependent to the
> firmware or bootloader configuration. On the mcbin, the lane may be
> configured in 10Gbase-KR, but it could be configured in SGMII as well.
> The configuration upon initialization and the re-configuration are quite
> similar then, as we might change mode as well at boot time.
> 
> You're right in that we might be re-configuring the lane for the same
> exact mode at boot time, if it was already configured in the same mode.
> 
> > Does this apply to reconfiguration of the serdes lane between
> > 10Gbase-KR and slower modes?
> 
> This applies only when configuring a line in a 10G mode,
> mvpp22_gop_init_10gkr isn't called otherwise.
> 
> When switching to an non-10G mode we might want to put the XPCS in reset
> though, which is not done today with this patch.

I'm merely pointing out the discrepency between the commit message and
what is actually being done.  I'm not particularly concerned about what
happens at boot.

We have four different transitions a port can go through, all of which
reconfigure the serdes lanes:

1. 10gkr -> 10gkr
2. 10gkr -> non-10gkr
3. non-10gkr -> non-10gkr
4. non-10gkr -> 10gkr

With this patch, XPCS is only placed into reset during the
reconfiguration for cases 1 and 4.  Case 3 doesn't matter (the XPCS
should already be in reset right?)  Case 2 isn't covered, and this
leaves a rather big hole.

It seems to me that if the documentation states that the XPCS needs to
be placed in reset while the serdes is reconfigured, then what should
be happening is:

- at boot, place the XPCS into reset.
- when we configure for 10gkr, release the reset once we've finished
  configuring the serdes.
- when we configure away from 10gkr, place the XPCS back into reset
  before configuring the serdes.

Merely placing the XPCS into reset while we configure the serdes for
10gkr doesn't seem to be "fixing" the driver to conform to your commit
message opening statement.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Powered by blists - more mailing lists