On Mon, Feb 4, 2019 at 5:39 PM Vladimir Oltean vladimir.oltean@nxp.com wrote:
On 2/5/19 1:28 AM, Joe Hershberger wrote:
On Fri, Jan 25, 2019 at 7:12 AM Vladimir Oltean vladimir.oltean@nxp.com wrote:
On 25.01.2019 12:12, Carlo Caione wrote:
On 24/01/2019 20:48, Vladimir Oltean wrote:
On 1/24/19 10:19 PM, Carlo Caione wrote:
On 24/01/2019 20:12, Vladimir Oltean wrote: >
I can't completely answer that, TBH I don't even know who is supposed to make that distinction.
In the kernel that distinction is made by the driver itself, hence my question. See [0].
For Freescale parts that is a call for the MDIO bus driver to make, for good or bad (see drivers/net/fm/memac_phy.c where dev_addr is compared to MDIO_DEVAD_NONE).
And in your patch, phy_write_mmd is only a wrapper over bus->write in the end, with some more logic to handle C22 indirection. So my question of unifying "mdio rmmd" with "mdio read" translates into:
Does it make sense to also handle the check with MDIO_DEVAD_NONE in phy_write_mmd, instead of jumping straight ahead to perform indirection?
Honestly I'm not quite sure of all the possible implications here IMO the safest bet here is just to follow what's done by the kernel. Maybe Joe can step in about this.
In general we have 3 possible cases:
- your driver is doing something non-standard when accessing the MMDs
and we deal with that using the PHY driver hooks 2) your PHY is C22 and you have to use the indirect method 3) your PHY is C45 and you can use the direct register reading (mangling a bit the address apparently)
The kernel is dealing with all the cases, U-Boot is only dealing with C22 PHYs (cases 1 and 2) because AFAICT there isn't yet a generic way to detect if the PHY is C22 or C45.
I'm not sure if the indirect method works also for C45 PHYs.
The goal would then be to just call phy_write_mmd from cmd/mdio.c regardless of the target PHY's clause.
Again I wrote that patch only assuming that we were going to deal with C22 PHYs. At this point I wonder if the C22 indirect method works also for C45 PHYs. If that's the case than the phy_write_mmd should already work regardless of the target PHY clause.
Cheers.
[0] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bo...
I'm not suggesting to use C22 indirection if the PHY already supports native C45 addressing. Even if that worked, it would be a pointless exercise in all but a few cases (like the MDIO controller does not support C22, but the PHY does support both C22 and C45). I was just wondering out loud whether the introduction of the "mdio rmmd" command is justified or not. I now understand that using e.g. "mdio read 1.3" will confuse the command for clause 45 PHY's because it won't know whether it should access the PHY via native C45 or via indirect C22 (obviously it shouldn't do the latter). So in lack of a clear distinction mechanism, I now think that a new command truly is necessary for performing indirect C45 access on C22. What I am still not convinced of, however, is whether those commands should be called "rmmd" and "wmmd". It is not immediately obvious from the command description that this is what they are for, and a user may attempt to use them for C45 PHY's as well, which will probably not yield the intended result.
I agree. The MMD in the register name is simply "MDIO Manageable Devices"... i.e. the phys.
I think the commands should be "iread" and "iwrite" to denote the indirect access in use.
Which brings me to my next point. If we can't properly make the distinction between an indirect C22 MMD access and a proper C45 MMD access, and hence not keeping proper API compatibility with Linux kernel, aren't we better off going back to square 1 and using phy_read_mmd_indirect and phy_write_mmd_indirect?
I think we can and should make the new wrapper functions remain named phy_*_mmd_indirect and the names of the override functions in the phy driver ops should be *_mmd_indirect. The override is still for an indirect access of c45 registers, just an apparently non-standard one. It is this way in Linux as well.
-Joe
-Vladimir _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot