Re: [U-Boot] [PATCH v5 3/3] cmd: mdio: Switch to generic helpers when accessing the registers

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