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

Vladimir Oltean vladimir.oltean at nxp.com
Fri Feb 15 22:46:17 UTC 2019 

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 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
> 

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

More information about the U-Boot mailing list