Message-ID: <20200525094536.GK1551@shell.armlinux.org.uk>
Date:   Mon, 25 May 2020 10:45:36 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Jeremy Linton <jeremy.linton@....com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, andrew@...n.ch,
        f.fainelli@...il.com, hkallweit1@...il.com,
        madalin.bucur@....nxp.com, calvin.johnson@....nxp.com,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC 01/11] net: phy: Don't report success if devices weren't
 found

On Sun, May 24, 2020 at 09:46:55PM -0500, Jeremy Linton wrote:
> Hi,
> 
> Thanks for taking a look at this.
> 
> On 5/23/20 1:20 PM, Russell King - ARM Linux admin wrote:
> > On Fri, May 22, 2020 at 04:30:49PM -0500, Jeremy Linton wrote:
> > > C45 devices are to return 0 for registers they haven't
> > > implemented. This means in theory we can terminate the
> > > device search loop without finding any MMDs. In that
> > > case we want to immediately return indicating that
> > > nothing was found rather than continuing to probe
> > > and falling into the success state at the bottom.
> > 
> > This is a little confusing when you read this comment:
> > 
> >                          /*  If mostly Fs, there is no device there,
> >                           *  then let's continue to probe more, as some
> >                           *  10G PHYs have zero Devices In package,
> >                           *  e.g. Cortina CS4315/CS4340 PHY.
> >                           */
> > 
> > Since it appears to be talking about the case of a PHY where *devs will
> > be zero.  However, tracking down the original submission, it seems this
> > is not the case at all, and the comment is grossly misleading.
> > 
> > It seems these PHYs only report a valid data in the Devices In Package
> > registers for devad=0 - it has nothing to do with a zero Devices In
> > Package.
> 
> Yes, this ended up being my understanding of this commit, and is part of my
> justification for starting the devices search at the reserved address 0
> rather than 1.
> 
> > 
> > Can I suggest that this comment is fixed while we're changing the code
> > to explicitly reject this "zero Devices In package" so that it's not
> > confusing?
> 
> Its probably better to kill it in 5/11 with a mention that we are starting
> at a reserved address?
> 
> OTOH, I'm a bit concerned that reading at 0 as the first address will cause
> problems because the original code was only triggering it after a read
> returning 0xFFFFFFFF at a valid MMD address. It does simplify/clarify the
> loop though. If it weren't for this 0 read, I would have tried to avoid some
> of the additional MMD reserved addresses.

Yes, that is the worry, and as MMD 0 is marked as reserved, I don't
think we should routinely probe it.

As I've already mentioned, note that bit 0 of devices-in-package does
not mean that there is a MMD 0 implemented, even if bit 0 is set.  Bit
0 means that the clause 22 register set is available through clause 22
cycles.  So, simplifying the loop to start at 0 and removing the work-
around could end up breaking Cortina PHYs if they don't set bit 0 in
their devices in package - but I don't see why we should depend on bit 0
being set for their workaround.

So, I think you're going to have to add a work-around to ignore bit 0,
which brings up the question whether this is worth it or not.

Hence, I think this is a "simplifcation" too far.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Powered by blists - more mailing lists