| Message ID | 1340411056-18988-2-git-send-email-ddaney.cavm@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: David Daney <ddaney.cavm@gmail.com> Date: Fri, 22 Jun 2012 17:24:13 -0700 > From: David Daney <david.daney@cavium.com> > > The IEEE802.3 clause 45 MDIO bus protocol allows for directly > addressing PHY registers using a 21 bit address, and is used by many > 10G Ethernet PHYS. Already existing is the ability of MDIO bus > drivers to use clause 45, with the MII_ADDR_C45 flag. Here we add > struct phy_c45_device_ids to hold the device identifier registers > present in clause 45. struct phy_device gets a couple of new fields: > c45_ids to hold the identifiers and is_c45 to signal that it is clause > 45. > > Normally the MII_ADDR_C45 flag is ORed with the register address to > indicate a clause 45 transaction. Here we also use this flag in the > *device* address passed to get_phy_device() to indicate that probing > should be done with clause 45 transactions. > > EXPORT phy_device_create() so that the follow-on patch to of_mdio.c > can use it to create phy devices for PHYs, that have non-standard > device identifier registers, based on the device tree bindings. > > Signed-off-by: David Daney <david.daney@cavium.com> I see no value in having two ways to say that clause-45 transactions should be used. Either make it a PHY device attribute, or specify it in the address in the register accesses, but not both. Also your patch is full of coding style errors, I simply couldn't stomache applying this even if I agreed with the substance of the changes: > + i < ARRAY_SIZE(c45_ids->device_ids) && > + c45_ids->devices_in_package == 0; c45_ids on the second line should line up with the initial 'i' on the first line. > + c45_ids->devices_in_package = (phy_reg & 0xffff) << 16; > + > + > + reg_addr = MII_ADDR_C45 | i << 16 | 5; There is not reason in the world to have two empty lines there, it looks awful. > + /* > + * If mostly Fs, there is no device there, > + * let's get out of here. > + */ Format comments: /* Like * this. */ Not. /* * Like * this. */ > + c45_ids->device_ids[i] = (phy_reg & 0xffff) << 16; > + > + > + reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2; Two empty lines. This is extremely irritating, it looks like you had some kind of debugging code here and then were very lazy about removing it. > +/* > + * Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the > + * 21 bit IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy > + * chips. Also may be ORed into the device address in > + * get_phy_device(). > + */ Comment formatting. > +/* > + * phy_c45_device_ids: 802.3-c45 Device Identifiers > + * > + * devices_in_package: Bit vector of devices present. > + * device_ids: The device identifer for each present device. > + */ If you're going to list the struct members use the correct kerneldoc format to do so. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/25/2012 03:34 PM, David Miller wrote: > From: David Daney<ddaney.cavm@gmail.com> > Date: Fri, 22 Jun 2012 17:24:13 -0700 > >> From: David Daney<david.daney@cavium.com> >> >> The IEEE802.3 clause 45 MDIO bus protocol allows for directly >> addressing PHY registers using a 21 bit address, and is used by many >> 10G Ethernet PHYS. Already existing is the ability of MDIO bus >> drivers to use clause 45, with the MII_ADDR_C45 flag. Here we add >> struct phy_c45_device_ids to hold the device identifier registers >> present in clause 45. struct phy_device gets a couple of new fields: >> c45_ids to hold the identifiers and is_c45 to signal that it is clause >> 45. >> >> Normally the MII_ADDR_C45 flag is ORed with the register address to >> indicate a clause 45 transaction. Here we also use this flag in the >> *device* address passed to get_phy_device() to indicate that probing >> should be done with clause 45 transactions. >> >> EXPORT phy_device_create() so that the follow-on patch to of_mdio.c >> can use it to create phy devices for PHYs, that have non-standard >> device identifier registers, based on the device tree bindings. >> >> Signed-off-by: David Daney<david.daney@cavium.com> > > I see no value in having two ways to say that clause-45 transactions > should be used. > > Either make it a PHY device attribute, or specify it in the address > in the register accesses, but not both. > Do you realize that at the time get_phy_device() is called, there is no PHY device? So there can be no attribute, nor are we passing a register address. Neither of these suggestions apply to this situation. We need to know a priori if it is c22 or c45. So we need to communicate the type somehow to get_phy_device(). I chose an unused bit in the addr parameter to do this, another option would be to add a separate parameter to get_phy_device() specifying the type. > Also your patch is full of coding style errors, I simply couldn't > stomache applying this even if I agreed with the substance of the > changes: > >> + i< ARRAY_SIZE(c45_ids->device_ids)&& >> + c45_ids->devices_in_package == 0; > > c45_ids on the second line should line up with the initial 'i' > on the first line. > >> + c45_ids->devices_in_package = (phy_reg& 0xffff)<< 16; >> + >> + >> + reg_addr = MII_ADDR_C45 | i<< 16 | 5; > > There is not reason in the world to have two empty lines there, it > looks awful. OK, I will fix those... > >> + /* >> + * If mostly Fs, there is no device there, >> + * let's get out of here. >> + */ > > Format comments: > > /* Like > * this. > */ > > Not. > > /* > * Like > * this. > */ ... and this one too I guess. Really you and Linus should come to a consensus on this one. [...] > >> +/* >> + * phy_c45_device_ids: 802.3-c45 Device Identifiers >> + * >> + * devices_in_package: Bit vector of devices present. >> + * device_ids: The device identifer for each present device. >> + */ > > If you're going to list the struct members use the correct kerneldoc > format to do so. OK. David Daney -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Daney <ddaney.cavm@gmail.com> Date: Mon, 25 Jun 2012 16:11:23 -0700 > Do you realize that at the time get_phy_device() is called, there is > no PHY device? So there can be no attribute, nor are we passing a > register address. Neither of these suggestions apply to this > situation. > > We need to know a priori if it is c22 or c45. So we need to > communicate the type somehow to get_phy_device(). I chose an unused > bit in the addr parameter to do this, another option would be to add a > separate parameter to get_phy_device() specifying the type. Then pass it in to the get() routine and store the attribute there in the device we end up with. There are many parameters that go into a PHY register access, so we'll probably some day end up with a descriptor struct that the caller prepares on-stack to pass into the actual read/write ops via reference. > ... and this one too I guess. Really you and Linus should come to a > consensus on this one. We did come up with a consensus, which is that subsystem maintainers such as myself are at liberty to enforce localized coding style for the bodies of code they maintain. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/25/2012 04:33 PM, David Miller wrote: > From: David Daney<ddaney.cavm@gmail.com> > Date: Mon, 25 Jun 2012 16:11:23 -0700 > >> Do you realize that at the time get_phy_device() is called, there is >> no PHY device? So there can be no attribute, nor are we passing a >> register address. Neither of these suggestions apply to this >> situation. >> >> We need to know a priori if it is c22 or c45. So we need to >> communicate the type somehow to get_phy_device(). I chose an unused >> bit in the addr parameter to do this, another option would be to add a >> separate parameter to get_phy_device() specifying the type. > > Then pass it in to the get() routine and store the attribute there > in the device we end up with. OK. addr has only 5 significant bits, and the patch *does* pass the information (c22 vs. c45) in one of the high order bits. So it is essentially as you say, but you don't like the idea of multiplexing the arguments into a single int. Therefore, I am going to propose that we add a 'flags' parameter to get_phy_device() and change the (two) callers. Does that seem better (or at least acceptable)? Or do you really want to pass the address of a (one bit) structure instead? David Daney > > There are many parameters that go into a PHY register access, so > we'll probably some day end up with a descriptor struct that the > caller prepares on-stack to pass into the actual read/write ops > via reference. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Daney <ddaney.cavm@gmail.com> Date: Mon, 25 Jun 2012 16:48:40 -0700 > Therefore, I am going to propose that we add a 'flags' parameter to > get_phy_device() and change the (two) callers. Since there is only one flag, make it simply a bool for now. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 18ab0da..fa0f558 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -152,8 +152,8 @@ int phy_scan_fixups(struct phy_device *phydev) } EXPORT_SYMBOL(phy_scan_fixups); -static struct phy_device* phy_device_create(struct mii_bus *bus, - int addr, int phy_id) +struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id, + struct phy_c45_device_ids *c45_ids) { struct phy_device *dev; @@ -174,8 +174,12 @@ static struct phy_device* phy_device_create(struct mii_bus *bus, dev->autoneg = AUTONEG_ENABLE; + dev->is_c45 = (addr & MII_ADDR_C45) != 0; + addr &= ~MII_ADDR_C45; dev->addr = addr; dev->phy_id = phy_id; + if (c45_ids) + dev->c45_ids = *c45_ids; dev->bus = bus; dev->dev.parent = bus->parent; dev->dev.bus = &mdio_bus_type; @@ -200,20 +204,104 @@ static struct phy_device* phy_device_create(struct mii_bus *bus, return dev; } +EXPORT_SYMBOL(phy_device_create); + +/** + * get_phy_c45_ids - reads the specified addr for its 802.3-c45 IDs. + * @bus: the target MII bus + * @addr: PHY address on the MII bus + * @phy_id: where to store the ID retrieved. + * @c45_ids: where to store the c45 ID information. + * + * If the PHY devices-in-package appears to be valid, it and the + * corresponding identifiers are stored in @c45_ids, zero is stored + * in @phy_id. Otherwise 0xffffffff is stored in @phy_id. Returns + * zero on success. + * + */ +static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, + struct phy_c45_device_ids *c45_ids) { + int phy_reg; + int i, reg_addr; + + /* + * Find first non-zero Devices In package. Device + * zero is reserved, so don't probe it. + */ + for (i = 1; + i < ARRAY_SIZE(c45_ids->device_ids) && + c45_ids->devices_in_package == 0; + i++) { + reg_addr = MII_ADDR_C45 | i << 16 | 6; + 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 | i << 16 | 5; + 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; + } + } + + /* Now probe Device Identifiers for each device present. */ + for (i = 1; i < ARRAY_SIZE(c45_ids->device_ids); i++) { + if (!(c45_ids->devices_in_package & (1 << i))) + continue; + + reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID1; + phy_reg = mdiobus_read(bus, addr, reg_addr); + if (phy_reg < 0) + return -EIO; + c45_ids->device_ids[i] = (phy_reg & 0xffff) << 16; + + + reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2; + phy_reg = mdiobus_read(bus, addr, reg_addr); + if (phy_reg < 0) + return -EIO; + c45_ids->device_ids[i] |= (phy_reg & 0xffff); + } + *phy_id = 0; + return 0; +} /** * get_phy_id - reads the specified addr for its ID. * @bus: the target MII bus * @addr: PHY address on the MII bus * @phy_id: where to store the ID retrieved. + * @c45_ids: where to store the c45 ID information. + * + * Description: In the case of a 802.3-c22 PHY, reads the ID registers + * of the PHY at @addr on the @bus, stores it in @phy_id and returns + * zero on success. + * + * In the case of a 802.3-c45 PHY, get_phy_c45_ids() is invoked, and + * its return value is in turn returned. * - * Description: Reads the ID registers of the PHY at @addr on the - * @bus, stores it in @phy_id and returns zero on success. */ -static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id) +static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id, + struct phy_c45_device_ids *c45_ids) { int phy_reg; + if (addr & MII_ADDR_C45) { + addr &= ~MII_ADDR_C45; + + return get_phy_c45_ids(bus, addr, phy_id, c45_ids); + } /* Grab the bits from PHYIR1, and put them * in the upper half */ phy_reg = mdiobus_read(bus, addr, MII_PHYSID1); @@ -241,14 +329,17 @@ static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id) * * Description: Reads the ID registers of the PHY at @addr on the * @bus, then allocates and returns the phy_device to represent it. + * If @addr & MII_ADDR_C45 is not zero, the PHY is assumed to be + * 802.3-c45. */ struct phy_device * get_phy_device(struct mii_bus *bus, int addr) { struct phy_device *dev = NULL; u32 phy_id; + struct phy_c45_device_ids c45_ids = {0}; int r; - r = get_phy_id(bus, addr, &phy_id); + r = get_phy_id(bus, addr, &phy_id, &c45_ids); if (r) return ERR_PTR(r); @@ -256,7 +347,7 @@ struct phy_device * get_phy_device(struct mii_bus *bus, int addr) if ((phy_id & 0x1fffffff) == 0x1fffffff) return NULL; - dev = phy_device_create(bus, addr, phy_id); + dev = phy_device_create(bus, addr, phy_id, &c45_ids); return dev; } @@ -449,6 +540,11 @@ static int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, /* Assume that if there is no driver, that it doesn't * exist, and we should use the genphy driver. */ if (NULL == d->driver) { + if (phydev->is_c45) { + pr_err("No driver for phy %x\n", phydev->phy_id); + return -ENODEV; + } + d->driver = &genphy_driver.driver; err = d->driver->probe(d); diff --git a/include/linux/phy.h b/include/linux/phy.h index c291cae..92d53ee 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -83,8 +83,12 @@ typedef enum { */ #define MII_BUS_ID_SIZE (20 - 3) -/* Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the 21 bit - IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy chips. */ +/* + * Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the + * 21 bit IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy + * chips. Also may be ORed into the device address in + * get_phy_device(). + */ #define MII_ADDR_C45 (1<<30) struct device; @@ -243,6 +247,16 @@ enum phy_state { PHY_RESUMING }; +/* + * phy_c45_device_ids: 802.3-c45 Device Identifiers + * + * devices_in_package: Bit vector of devices present. + * device_ids: The device identifer for each present device. + */ +struct phy_c45_device_ids { + u32 devices_in_package; + u32 device_ids[8]; +}; /* phy_device: An instance of a PHY * @@ -250,6 +264,8 @@ enum phy_state { * bus: Pointer to the bus this PHY is on * dev: driver model device structure for this PHY * phy_id: UID for this device found during discovery + * c45_ids: 802.3-c45 Device Identifers if is_c45. + * is_c45: Set to true if this phy uses clause 45 addressing. * state: state of the PHY for management purposes * dev_flags: Device-specific flags used by the PHY driver. * addr: Bus address of PHY @@ -285,6 +301,9 @@ struct phy_device { u32 phy_id; + struct phy_c45_device_ids c45_ids; + bool is_c45; + enum phy_state state; u32 dev_flags; @@ -480,6 +499,8 @@ static inline int phy_write(struct phy_device *phydev, u32 regnum, u16 val) return mdiobus_write(phydev->bus, phydev->addr, regnum, val); } +struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id, + struct phy_c45_device_ids *c45_ids); struct phy_device* get_phy_device(struct mii_bus *bus, int addr); int phy_device_register(struct phy_device *phy); int phy_init_hw(struct phy_device *phydev);