> -----Original Message-----
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: Wednesday, April 15, 2015 1:45 AM
> To: Liu Shengzhou-B36685; net...@vger.kernel.org; da...@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....@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�����r��y����b�X��ǧv�^�){.n�+����{����zX����ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z� �w���?�����&�)ߢf��^jǫy�m��@A�a���
0��h���i
