> -----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 > > --- > > 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ʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I