From: "Shengzhou.Liu@freescale.com" <Shengzhou.Liu@freescale.com>
To: Florian Fainelli <f.fainelli@gmail.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2] net/phy: tune get_phy_c45_ids to support more c45 phy
Thread-Topic: [PATCH v2] net/phy: tune get_phy_c45_ids to support more c45 phy
Thread-Index: AQHQdpucZY0bwhXwQkWOrAM0PKeak51MyESAgADkifA=
Date: Wed, 15 Apr 2015 08:27:05 +0000
Message-ID: <DM2PR03MB398B7FB38DB8C40CF8D65A8F8E50@DM2PR03MB398.namprd03.prod.outlook.com>
References: <1429006192-21403-1-git-send-email-Shengzhou.Liu@freescale.com>
 <552D5221.1030703@gmail.com>
In-Reply-To: <552D5221.1030703@gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Apr 2015 08:27:05.1571
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR03MB399
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id t3F8RfoS014932
Content-Length: 3873
Lines: 80

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Wednesday, April 15, 2015 1:45 AM
> To: Liu Shengzhou-B36685; netdev@vger.kernel.org; davem@davemloft.net
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] net/phy: tune get_phy_c45_ids to support more c45 phy
> 
> On 14/04/15 03:09, Shengzhou Liu wrote:
> > As some C45 10G PHYs(e.g. Cortina CS4315/CS4340 PHY) have zero Devices
> > In package, current driver can't get correct devices_in_package value
> > by non-zero Devices In package.
> > so let's probe more with zero Devices In package to support more C45
> > PHYs.
> 
> I cannot remember exactly how many times this patch has been posted, but it
> still is not clear to me what you are doing here is helping with these
> Cortina PHYs.
> 
> Could you post a dump of the mdiobus_read() arguments and values for the old
> code and the new code you are proposing? That way it might be clearer what is
> the goal here?
> 
The key point is that standard C45 PHYs use non-zero Devices in package i(reg_addr = MII_ADDR_C45 | i << 16 | MDIO_DEVS) to read  devices_in_package, device zero is reserved, but Cortina CS4315/CS4340 PHY use zero Devices in package(reg_addr = MII_ADDR_C45 | 0 << 16 | MDIO_DEVS) to read devices_in_package. 

This is caused by Cortina non-standard PHY, e.g. standard PHY has MII_PHYSID1=0x02, MII_PHYSID2=0x03, but CS4315/CS4340 PHY has non-standard offset of PHY ID registers(MII_PHYSID1=0x00, MII_PHYSID2=0x01).
  
> >
> > Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com>
> > ---
> > v2: use MDIO_DEVS1 and MDIO_DEVS2 instead of constant '6', '5'
> >
> >  drivers/net/phy/phy_device.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy_device.c
> > b/drivers/net/phy/phy_device.c index bdfe51f..c4f836f 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -242,12 +242,29 @@ static int get_phy_c45_ids(struct mii_bus *bus, int
> addr, u32 *phy_id,
> >  			return -EIO;
> >  		c45_ids->devices_in_package |= (phy_reg & 0xffff);
> >
> > -		/* If mostly Fs, there is no device there,
> > -		 * let's get out of here.
> > +		/* If mostly Fs, let's continue to probe more
> > +		 * as some c45 PHYs have zero Devices In package,
> > +		 * e.g. Cortina CS4315/CS4340 PHY.
> >  		 */
> >  		if ((c45_ids->devices_in_package & 0x1fffffff) == 0x1fffffff) {
> > -			*phy_id = 0xffffffff;
> > -			return 0;
> > +			reg_addr = MII_ADDR_C45 | 0 << 16 | MDIO_DEVS2;
> > +			phy_reg = mdiobus_read(bus, addr, reg_addr);
> > +			if (phy_reg < 0)
> > +				return -EIO;
> > +			c45_ids->devices_in_package = (phy_reg & 0xffff) << 16;
> > +			reg_addr = MII_ADDR_C45 | 0 << 16 | MDIO_DEVS1;
> > +			phy_reg = mdiobus_read(bus, addr, reg_addr);
> > +			if (phy_reg < 0)
> > +				return -EIO;
> > +			c45_ids->devices_in_package |= (phy_reg & 0xffff);
> > +			/* If mostly Fs, there is no device there,
> > +			 * let's get out of here.
> > +			 */
> > +			if ((c45_ids->devices_in_package & 0x1fffffff) ==
> > +							0x1fffffff) {
> > +				*phy_id = 0xffffffff;
> > +				return 0;
> > +			}
> 
> Could not we somehow be a bit more clever and utilize the loop, with an
> adjusted i = 0 during this condition? Some something like this (untested):
> 
> Florian

I had done it to utilize the loop with i = 0 as your thought, but David Miller said that the way of utilizing the loop makes no sense to test 'i' for zero vs. non-zero until the looping construct it is contained within can actually hit a zero value, adding such a check here makes the code confusing.
So I dropped the way of utilizing the loop to make code readable.

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?