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

Vladimir Oltean vladimir.oltean at nxp.com
Tue Feb 12 12:20:25 UTC 2019


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 at 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 at qsgmii
d - Vitesse VSC8514 <--> DPMAC4 at qsgmii
e - Vitesse VSC8514 <--> DPMAC5 at qsgmii
f - Vitesse VSC8514 <--> DPMAC6 at qsgmii
1c - Vitesse VSC8514 <--> DPMAC7 at qsgmii
1d - Vitesse VSC8514 <--> DPMAC8 at qsgmii
1e - Vitesse VSC8514 <--> DPMAC9 at qsgmii
1f - Vitesse VSC8514 <--> DPMAC10 at qsgmii
FSL_MDIO1:
0 - Generic PHY <--> DPMAC2 at xgmii

# AQR105: PMA Standard Device Identifier 1 & 2 registers
=> mdio read DPMAC2 at 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 at 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 at 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 at qsgmii
d - Vitesse VSC8514 <--> DPMAC4 at qsgmii
e - Vitesse VSC8514 <--> DPMAC5 at qsgmii
f - Vitesse VSC8514 <--> DPMAC6 at qsgmii
1c - Vitesse VSC8514 <--> DPMAC7 at qsgmii
1d - Vitesse VSC8514 <--> DPMAC8 at qsgmii
1e - Vitesse VSC8514 <--> DPMAC9 at qsgmii
1f - Vitesse VSC8514 <--> DPMAC10 at qsgmii
FSL_MDIO1:
0 - Generic 10G PHY <--> DPMAC2 at xgmii

Notice that the AQR105 is now probed as "Generic 10G PHY".
But:
# AQR105: PMA Standard Device Identifier 1 & 2 registers
=> mdio read DPMAC2 at 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 at qsgmii
d - Vitesse VSC8514 <--> DPMAC4 at qsgmii
e - Vitesse VSC8514 <--> DPMAC5 at qsgmii
f - Vitesse VSC8514 <--> DPMAC6 at qsgmii
1c - Vitesse VSC8514 <--> DPMAC7 at qsgmii
1d - Vitesse VSC8514 <--> DPMAC8 at qsgmii
1e - Vitesse VSC8514 <--> DPMAC9 at qsgmii
1f - Vitesse VSC8514 <--> DPMAC10 at qsgmii
FSL_MDIO1:
0 - Aquantia AQR105 <--> DPMAC2 at xgmii

# AQR105: PMA Standard Device Identifier 1 & 2 registers
=> mdio read DPMAC2 at 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


More information about the U-Boot mailing list