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 <[email protected]> >> --- >> 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, >> ®lo, ®hi)) >> - 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
