On Fri, Feb 8, 2019 at 11:29 AM Carlo Caione ccaione@baylibre.com wrote:

Two new helper functions (phy_read_mmd() and phy_write_mmd()) are added to allow access to the MMD PHY registers.

The MMD PHY registers can be accessed by several means:

1. Using two new MMD access function hooks in the PHY driver. These

functions can be implemented when the PHY driver does not support the standard IEEE Compatible clause 45 access mechanism described in clause 22 or if the PHY uses its own non-standard access mechanism.

2. Direct access for C45 PHYs and C22 PHYs when accessing the reachable

DEVADs.

3. The standard clause 45 access extensions to the MMD registers through

the indirection registers (clause 22) in all the other cases.

Signed-off-by: Carlo Caione ccaione@baylibre.com

Except for the off-by-ones below... Acked-by: Joe Hershberger joe.hershberger@ni.com

---

drivers/net/phy/phy.c | 4 +++ include/phy.h | 70 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index cda4caa803..6769047407 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -549,6 +549,10 @@ int phy_register(struct phy_driver *drv) drv->readext += gd->reloc_off; if (drv->writeext) drv->writeext += gd->reloc_off;

•   if (drv->read_mmd)
  

•           drv->read_mmd += gd->reloc_off;
  

•   if (drv->write_mmd)
  

•           drv->write_mmd += gd->reloc_off;
  

#endif return 0; } diff --git a/include/phy.h b/include/phy.h index b86fdfb2ce..7ec2b4e86c 100644 --- a/include/phy.h +++ b/include/phy.h @@ -101,6 +101,14 @@ struct phy_driver { int (*readext)(struct phy_device *phydev, int addr, int devad, int reg); int (*writeext)(struct phy_device *phydev, int addr, int devad, int reg, u16 val);

•   /* Phy specific driver override for reading a MMD register */
  

•   int (*read_mmd)(struct phy_device *phydev, int devad, int reg);
  

•   /* Phy specific driver override for writing a MMD register */
  

•   int (*write_mmd)(struct phy_device *phydev, int devad, int reg,
  

•                    u16 val);
  

•   struct list_head list;
  

};

@@ -164,6 +172,68 @@ static inline int phy_write(struct phy_device *phydev, int devad, int regnum, return bus->write(bus, phydev->addr, devad, regnum, val); }

+static inline void phy_mmd_start_indirect(struct phy_device *phydev, int devad,

•                                     int regnum)
  

+{

•   /* Write the desired MMD Devad */
  

•   phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_CTRL, devad);
  

•   /* Write the desired MMD register address */
  

•   phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, regnum);
  

•   /* Select the Function : DATA with no post increment */
  

•   phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_CTRL,
  

•             (devad | MII_MMD_CTRL_NOINCR));
  

+}

+static inline int phy_read_mmd(struct phy_device *phydev, int devad,

•                          int regnum)
  

+{

•   struct phy_driver *drv = phydev->drv;
  

•   if (regnum > (u16)~0 || devad > 32)
  

Shouldn't this be >= 32?

•           return -EINVAL;
  

•   /* driver-specific access */
  

•   if (drv->read_mmd)
  

•           return drv->read_mmd(phydev, devad, regnum);
  

•   /* direct C45 / C22 access */
  

•   if ((drv->features & PHY_10G_FEATURES) == PHY_10G_FEATURES ||
  

•       devad == MDIO_DEVAD_NONE || !devad)
  

•           return phy_read(phydev, devad, regnum);
  

•   /* indirect C22 access */
  

•   phy_mmd_start_indirect(phydev, devad, regnum);
  

•   /* Read the content of the MMD's selected register */
  

•   return phy_read(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA);
  

+}

+static inline int phy_write_mmd(struct phy_device *phydev, int devad,

•                           int regnum, u16 val)
  

+{

•   struct phy_driver *drv = phydev->drv;
  

•   if (regnum > (u16)~0 || devad > 32)
  

Same here.

•           return -EINVAL;
  

•   /* driver-specific access */
  

•   if (drv->write_mmd)
  

•           return drv->write_mmd(phydev, devad, regnum, val);
  

•   /* direct C45 / C22 access */
  

•   if ((drv->features & PHY_10G_FEATURES) == PHY_10G_FEATURES ||
  

•       devad == MDIO_DEVAD_NONE || !devad)
  

•           return phy_write(phydev, devad, regnum, val);
  

•   /* indirect C22 access */
  

•   phy_mmd_start_indirect(phydev, devad, regnum);
  

•   /* Write the data into MMD's selected register */
  

•   return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val);
  

+}

#ifdef CONFIG_PHYLIB_10G extern struct phy_driver gen10g_driver;

-- 2.19.1

---

U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot

On 2/12/19 2:20 PM, Vladimir Oltean wrote:

On 08.02.2019 19:26, Carlo Caione wrote:

...

Switch to use the generic helpers to access the MMD registers so that we can used the same command also for C45 PHYs, C22 PHYs with direct and indirect access and PHYs implementing a custom way to access the registers.

Signed-off-by: Carlo Caione ccaione@baylibre.com

cmd/mdio.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/cmd/mdio.c b/cmd/mdio.c index 184868063a..efe8c9ef09 100644 --- a/cmd/mdio.c +++ b/cmd/mdio.c @@ -39,21 +39,24 @@ static int extract_range(char *input, int *plo, int *phi) return 0; }

-static int mdio_write_ranges(struct phy_device *phydev, struct mii_dev *bus, +static int mdio_write_ranges(struct mii_dev *bus, int addrlo, int addrhi, int devadlo, int devadhi, int reglo, int reghi, unsigned short data, int extended) {

• struct phy_device *phydev; int addr, devad, reg; int err = 0;

  for (addr = addrlo; addr <= addrhi; addr++) {

• phydev = bus->phymap[addr];
  

• for (devad = devadlo; devad <= devadhi; devad++) {
  	for (reg = reglo; reg <= reghi; reg++) {
  		if (!extended)
  

• 			err = bus->write(bus, addr, devad,
  

• 					 reg, data);
  

• 			err = phy_write_mmd(phydev, devad,
  

• 					    reg, data);
  		else
  			err = phydev->drv->writeext(phydev,
  					addr, devad, reg, data);
  

@@ -68,15 +71,17 @@ err_out: return err; }

-static int mdio_read_ranges(struct phy_device *phydev, struct mii_dev *bus, +static int mdio_read_ranges(struct mii_dev *bus, int addrlo, int addrhi, int devadlo, int devadhi, int reglo, int reghi, int extended) { int addr, devad, reg;

• struct phy_device *phydev;

  printf("Reading from bus %s\n", bus->name); for (addr = addrlo; addr <= addrhi; addr++) {

• phydev = bus->phymap[addr];
  printf("PHY at address %x:\n", addr);
  
  for (devad = devadlo; devad <= devadhi; devad++) {
  

@@ -84,7 +89,7 @@ static int mdio_read_ranges(struct phy_device *phydev, struct mii_dev *bus, int val;

			if (!extended)

• 			val = bus->read(bus, addr, devad, reg);
  

• 			val = phy_read_mmd(phydev, devad, reg);
  		else
  			val = phydev->drv->readext(phydev, addr,
  				devad, reg);
  

@@ -222,14 +227,14 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) bus = phydev->bus; extended = 1; } else {

• 		return -1;
  

• 		return CMD_RET_FAILURE;
  	}
  
  	if (!phydev->drv ||
  	    (!phydev->drv->writeext && (op[0] == 'w')) ||
  	    (!phydev->drv->readext && (op[0] == 'r'))) {
  		puts("PHY does not have extended functions\n");
  

• 		return -1;
  

• 		return CMD_RET_FAILURE;
  	}
  }
  

  }

@@ -242,13 +247,13 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (pos > 1) if (extract_reg_range(argv[pos--], &devadlo, &devadhi, &reglo, &reghi))

• 		return -1;
  

• 		return CMD_RET_FAILURE;
  
  

  default: if (pos > 1) if (extract_phy_range(&argv[2], pos - 1, &bus, &phydev, &addrlo, &addrhi))

• 		return -1;
  

• 		return CMD_RET_FAILURE;
  
  break;
  

  }

@@ -264,12 +269,12 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

switch (op[0]) {
case 'w':

• mdio_write_ranges(phydev, bus, addrlo, addrhi, devadlo, devadhi,
  

• mdio_write_ranges(bus, addrlo, addrhi, devadlo, devadhi,
  		  reglo, reghi, data, extended);
  break;
  
  

  case 'r':

• mdio_read_ranges(phydev, bus, addrlo, addrhi, devadlo, devadhi,
  

• mdio_read_ranges(bus, addrlo, addrhi, devadlo, devadhi,
  		 reglo, reghi, extended);
  break;
  

  }

Hi Carlo,

I tested your patch on the NXP LS1088A-RDB board which is equipped with 2 VSC8514 PHYs and 1 AQR105 PHY. The VSC8514 is a clause 22 QSGMII 10/100/1000 Base-T PHY, but its 802.1az (EEE) registers are accessible through indirect clause 45 (clause 22 registers 13 and 14). The Aquantia AQR105 is a 10G Base-T PHY with native clause 45 addressing.

I started off by making sure that what should work (clause 45 addressing) still works, so I tested the AQR105 without the patchset.

=> mdio list FSL_MDIO0: c - Vitesse VSC8514 <--> DPMAC3@qsgmii d - Vitesse VSC8514 <--> DPMAC4@qsgmii e - Vitesse VSC8514 <--> DPMAC5@qsgmii f - Vitesse VSC8514 <--> DPMAC6@qsgmii 1c - Vitesse VSC8514 <--> DPMAC7@qsgmii 1d - Vitesse VSC8514 <--> DPMAC8@qsgmii 1e - Vitesse VSC8514 <--> DPMAC9@qsgmii 1f - Vitesse VSC8514 <--> DPMAC10@qsgmii FSL_MDIO1: 0 - Generic PHY <--> DPMAC2@xgmii

# AQR105: PMA Standard Device Identifier 1 & 2 registers => mdio read DPMAC2@xgmii 1.2-3 Reading from bus FSL_MDIO1 PHY at address 0: 1.2 - 0x3a1 1.3 - 0xb4a3

Then I applied the patchset and went on to test both PHYs (since now mdio read MMD is supposed to work on both, not just the AQR).

# VSC8514: EEE Capability register => mdio read DPMAC3@qsgmii 3.14 Reading from bus FSL_MDIO0 PHY at address c: 3.20 - 0x6

So indirect addressing works, I will no longer refer to this and instead focus on the native C45 PHYs.

=> mdio read DPMAC2@xgmii 1.2-3 Reading from bus FSL_MDIO1 PHY at address 0: 1.2 - 0xffff 1.3 - 0xffff

Oops, the AQR105 no longer works. But notice that U-boot probes it as "Generic PHY", so this is a board defconfig problem. So I went on to enable CONFIG_PHYLIB_10G=y and repeat the test.

=> mdio list FSL_MDIO0: c - Vitesse VSC8514 <--> DPMAC3@qsgmii d - Vitesse VSC8514 <--> DPMAC4@qsgmii e - Vitesse VSC8514 <--> DPMAC5@qsgmii f - Vitesse VSC8514 <--> DPMAC6@qsgmii 1c - Vitesse VSC8514 <--> DPMAC7@qsgmii 1d - Vitesse VSC8514 <--> DPMAC8@qsgmii 1e - Vitesse VSC8514 <--> DPMAC9@qsgmii 1f - Vitesse VSC8514 <--> DPMAC10@qsgmii FSL_MDIO1: 0 - Generic 10G PHY <--> DPMAC2@xgmii

Notice that the AQR105 is now probed as "Generic 10G PHY". But: # AQR105: PMA Standard Device Identifier 1 & 2 registers => mdio read DPMAC2@xgmii 1.2-3 Reading from bus FSL_MDIO1 PHY at address 0: 1.2 - 0xffff 1.3 - 0xffff

So it still doesn't work. This is because in drivers/net/phy/generic_10g.c, the gen10g_driver.features is 0 and not PHY_10G_FEATURES as it properly should (comments?)

Then I went on to enable the proper driver for the AQR105, which is CONFIG_PHY_AQUANTIA.

=> mdio list FSL_MDIO0: c - Vitesse VSC8514 <--> DPMAC3@qsgmii d - Vitesse VSC8514 <--> DPMAC4@qsgmii e - Vitesse VSC8514 <--> DPMAC5@qsgmii f - Vitesse VSC8514 <--> DPMAC6@qsgmii 1c - Vitesse VSC8514 <--> DPMAC7@qsgmii 1d - Vitesse VSC8514 <--> DPMAC8@qsgmii 1e - Vitesse VSC8514 <--> DPMAC9@qsgmii 1f - Vitesse VSC8514 <--> DPMAC10@qsgmii FSL_MDIO1: 0 - Aquantia AQR105 <--> DPMAC2@xgmii

# AQR105: PMA Standard Device Identifier 1 & 2 registers => mdio read DPMAC2@xgmii 1.2-3 Reading from bus FSL_MDIO1 PHY at address 0: 1.2 - 0x3a1 1.3 - 0xb4a3

So it finally works now with your patchset.

Posting this just to raise awareness that:

• There are boards with 10G PHYs that didn't use to define

CONFIG_PHYLIB_10G. Their MDIO drivers were working because they were only inferring the clause 45 aspect from the fact that a non-zero MMD access was being performed. This no longer works so they have to enable the 10g phylib support now.

• The generic 10G PHY driver doesn't define PHY_10G_FEATURES and needs

to be fixed.

• The PHY vendors define their MMD register addresses in decimal. The

U-boot mdio command has no way of inputting the address in decimal. For the VSC8514, to access its EEE registers I have to specify 3.14 (hex) in order for it to access 3.20 (decimal). This behavior does not originate from this patchset, but I think it's worth pointing it out now since it touches this area.

None of the above is a deal breaker IMO and I think that this patchset does work as intended (with the mention that more patches are needed).

Thanks, -Vladimir

Carlo, Joe,

One additional piece of information that for some reason escaped me initially is that Pankaj Bansal already added support for struct phy_device property is_c45 as part of this patch: http://patchwork.ozlabs.org/patch/998770/ Maybe this patchset should go in the direction of using that.

-Vladimir