On 29.05.2019 18:00, Alex Marginean wrote: > On 5/29/2019 5:19 PM, Vladimir Oltean wrote: >> On 29.05.2019 15:27, Alex Marginean wrote: >>> >>> Hi Carlo, everyone, >>> >>> I'm doing some MDIO work on a freescale/NXP platform and I bumped into >>> errors with this command: >>> => mdio r emdio#3 5 3 >>> Reading from bus emdio#3 >>> "Synchronous Abort" handler, esr 0x8600000e >>> elr: ffffffff862b8000 lr : 000000008200cce4 (reloc) >>> ... >>> >>> mdio list does not list any PHYs currently because ethernet is using DM >>> and the interfaces are not probed at this time. The PHY does exist >>> on the bus though. >>> The above scenario works with this commit reverted: >>> e55047ec51a662c12ed53ff543ec7cdf158b2137 cmd: mdio: Switch to generic >>> helpers when accessing the registers >>> >>> The current code using generic helpers only works for PHYs that have >>> been registered and show up in bus->phymap and crashes for arbitrary >>> IDs. I find it useful to allow reading from other addresses over MDIO >>> too, certainly helpful for people debugging MDIO on various boards. >>> >>> What's your take on this? Could we revert the above patch, or should I >>> add some code handling the case when bus->phymap comes up empty and then >>> go to bus->read? >>> >>> Thanks! >>> Alex >>> >> >> Hi Alex, >> >> Thanks for reporting this. >> I think that breaking access to arbitrary PHY addresses was only an >> unintentional side-effect here. Even moreso since having MDIO access to >> PHYs not defined in DT is desirable, and not only for debugging. It >> certainly seems easy to add it back by introducing something like the >> following (not tested): >> >>> diff --git a/cmd/mdio.c b/cmd/mdio.c >>> index efe8c9ef0954..5e219f699d8d 100644 >>> --- a/cmd/mdio.c >>> +++ b/cmd/mdio.c >>> @@ -54,7 +54,10 @@ static int mdio_write_ranges(struct mii_dev *bus, >>> >>> for (devad = devadlo; devad <= devadhi; devad++) { >>> for (reg = reglo; reg <= reghi; reg++) { >>> - if (!extended) >>> + if (!phydev) >>> + err = bus->write(bus, addr, devad, >>> + reg, data); >>> + else if (!extended) >>> err = phy_write_mmd(phydev, devad, >>> reg, data); >>> else >>> @@ -88,7 +91,9 @@ static int mdio_read_ranges(struct mii_dev *bus, >>> for (reg = reglo; reg <= reghi; reg++) { >>> int val; >>> >>> - if (!extended) >>> + if (!phydev) >>> + val = bus->read(bus, addr, devad, reg); >>> + else if (!extended) >>> val = phy_read_mmd(phydev, devad, >>> reg); >>> else >>> val = phydev->drv->readext(phydev, >>> addr, >> >> Of course in this case we're back to not having the indirect C45 >> convenience commands, but it's still much better than not having access >> at all. >> >> If there are no objections I can volunteer to either test a patch or >> even submit one later today. >> >> Regards, >> -Vladimir >> > > Hey Vladimir, > > The downside is that the original patch from Carlo replaced old API > calls with the more generic API, and now we would switch to using > both generic API and old API although this change doesn't actually > add any functionality. It may be simpler to just switch to the > old code, it's simpler than the three branches (!phydev, !extended > and else) and it does the same thing. >
Hi Alex, I don't think that reverting is necessary. And in fact, the generic API does bring some functionality (albeit only for user convenience, but that matters sometimes too). Say a C22 PHY had a register at 7.1. To write a value it, previously you had to: mdio write <dev> 0xd 0x7 mdio write <dev> 0xe 0x1 mdio write <dev> 0xd 0x4007 mdio write <dev> 0xe <value> Now you can just: mdio write <dev> 7.1 <value> > Back to the change you suggested, I have the changes applied to my tree > and it works: > => mdio list > emdio#3: > => mdio r emdio#3 5 3 > Reading from bus emdio#3 > PHY at address 5: > 3 - 0xd072 > > I can't test C45 right now on my set-up, I would expect it to work > though. > > Thanks! > Alex > Thanks for testing, good to know that it works. I'll send a patch later today then. -Vladimir _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot