Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
I somehow missed this mail.. On Mon, Apr 05, 2021 at 10:12:36PM +0100, Russell King - ARM Linux admin wrote: > On Mon, Apr 05, 2021 at 08:58:07PM +0200, Danilo Krummrich wrote: > > On Mon, Apr 05, 2021 at 03:33:55PM +0200, Andrew Lunn wrote: > > However, this was about something else - Russell wrote: > > > > > We have established that MDIO drivers need to reject accesses for > > > > > reads/writes that they do not support [..] > > The MDIO drivers do this by checking the MII_ADDR_C45 flag if it's a C45 bus > > request. In case they don't support it they return -EOPNOTSUPP. So > > basically, > > the bus drivers read/write functions (should) encode the capability of doing > > C45 transfers. > > > > I just noted that this is redundant to the bus' capabilities field of > > struct mii_bus which also encodes the bus' capabilities of doing C22 and/or > > C45 > > transfers. > > > > Now, instead of encoding this information of the bus' capabilities at both > > places, I'd propose just checking the mii_bus->capabilities field in the > > mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having > > two > > places where this information is stored. What do you think about that? > > It would be good to clean that up, but that's a lot of work given the > number of drivers - not only in terms of making the changes but also > making sure that the changes are as correct as would be sensibly > achievable... then there's the problem of causing regressions by doing > so. > As mentioned in the answer to Andrew, I think it wouldn't be too many as we'd only need to look for the ones considering MII_ADDR_C45 at all. > The two ways were introduced at different times to do two different > things: the checking in the read/write methods in each driver was the > first method, which was being added to newer drivers. Then more > recently came the ->capabilities field. > > So now we have some drivers that: > - do no checks and don't fill in ->capabilities either (some of which > are likely C22-only.) Guess they could be left untouched completely and simply go for MDIOBUS_NO_CAP implicitly and therefore C45 requests would be correctly rejected by mdiobus_c45_*() functions. I think this would be the main advantage apart from making it more clean. > - check in their read/write methods for access types they don't support > (e.g. drivers/net/ethernet/marvell/mvmdio.c) and don't fill in > ->capabilities. Note, mvmdio supports both C22-only and C45-only > interfaces with no C22-and-C45 interfaces. Yes, I think the correct capability could still be easily assigned per instance within the probe() function: switch (type) { case BUS_TYPE_SMI: bus->capabilities = MDIOBUS_C22; [...] break; case BUS_TYPE_XSMI: bus->capabilities = MDIOBUS_C45; [...] break; } > - do fill in ->capabilities with MDIOBUS_C22_C45 (and hence have no > checks in their read/write functions). > Nothing to be done for them. > Sometimes, its best to leave stuff alone... if it ain't broke, don't > make regressions. > Yes, I can perfectly understand that. However, maybe I go for a patch anyways, and if so I will also send it. If it's taken by you in the end is on another piece of paper. Thanks again! Danilo > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Tue, Apr 06, 2021 at 12:30:11AM +0200, Danilo Krummrich wrote: > On Mon, Apr 05, 2021 at 09:27:44PM +0200, Andrew Lunn wrote: > > > Now, instead of encoding this information of the bus' capabilities at both > > > places, I'd propose just checking the mii_bus->capabilities field in the > > > mdiobus_c45_*() functions. IMHO this would be a little cleaner, than > > > having two > > > places where this information is stored. What do you think about that? > > > > You will need to review all the MDIO bus drivers to make sure they > > correctly set the capabilities. There is something like 55 using > > of_mdiobus_register() and 45 using mdiobus_register(). So you have 100 > > drivers to review. > Yes, but I think it would be enough to look at the drivers handling the > MII_ADDR_C45 flag, because those are either > - actually capable to do C45 bus transfers or > - do properly return -EOPNOTSUPP. > > I counted 27 drivers handling the MII_ADDR_C45 flag. Setting the capabilities > for those should be pretty easy. > > The remaining ones, which should be about 73 then, could be left untouched, > because the default capability MDIOBUS_NO_CAP would indicate they can C22 > only. > Since they don't handle the MII_ADDR_C45 flag at all, this should be the > correct assumption. Forgot to mention, this would also automatically fixup that userspace C45 requests for those "remaining" drivers results in garbage on the bus. :-) > > > > Andrew > >
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Mon, Apr 05, 2021 at 09:27:44PM +0200, Andrew Lunn wrote: > > Now, instead of encoding this information of the bus' capabilities at both > > places, I'd propose just checking the mii_bus->capabilities field in the > > mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having > > two > > places where this information is stored. What do you think about that? > > You will need to review all the MDIO bus drivers to make sure they > correctly set the capabilities. There is something like 55 using > of_mdiobus_register() and 45 using mdiobus_register(). So you have 100 > drivers to review. Yes, but I think it would be enough to look at the drivers handling the MII_ADDR_C45 flag, because those are either - actually capable to do C45 bus transfers or - do properly return -EOPNOTSUPP. I counted 27 drivers handling the MII_ADDR_C45 flag. Setting the capabilities for those should be pretty easy. The remaining ones, which should be about 73 then, could be left untouched, because the default capability MDIOBUS_NO_CAP would indicate they can C22 only. Since they don't handle the MII_ADDR_C45 flag at all, this should be the correct assumption. > > Andrew >
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Mon, Apr 05, 2021 at 08:58:07PM +0200, Danilo Krummrich wrote: > On Mon, Apr 05, 2021 at 03:33:55PM +0200, Andrew Lunn wrote: > However, this was about something else - Russell wrote: > > > > We have established that MDIO drivers need to reject accesses for > > > > reads/writes that they do not support [..] > The MDIO drivers do this by checking the MII_ADDR_C45 flag if it's a C45 bus > request. In case they don't support it they return -EOPNOTSUPP. So basically, > the bus drivers read/write functions (should) encode the capability of doing > C45 transfers. > > I just noted that this is redundant to the bus' capabilities field of > struct mii_bus which also encodes the bus' capabilities of doing C22 and/or > C45 > transfers. > > Now, instead of encoding this information of the bus' capabilities at both > places, I'd propose just checking the mii_bus->capabilities field in the > mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having > two > places where this information is stored. What do you think about that? It would be good to clean that up, but that's a lot of work given the number of drivers - not only in terms of making the changes but also making sure that the changes are as correct as would be sensibly achievable... then there's the problem of causing regressions by doing so. The two ways were introduced at different times to do two different things: the checking in the read/write methods in each driver was the first method, which was being added to newer drivers. Then more recently came the ->capabilities field. So now we have some drivers that: - do no checks and don't fill in ->capabilities either (some of which are likely C22-only.) - check in their read/write methods for access types they don't support (e.g. drivers/net/ethernet/marvell/mvmdio.c) and don't fill in ->capabilities. Note, mvmdio supports both C22-only and C45-only interfaces with no C22-and-C45 interfaces. - do fill in ->capabilities with MDIOBUS_C22_C45 (and hence have no checks in their read/write functions). Sometimes, its best to leave stuff alone... if it ain't broke, don't make regressions. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
> Now, instead of encoding this information of the bus' capabilities at both > places, I'd propose just checking the mii_bus->capabilities field in the > mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having > two > places where this information is stored. What do you think about that? You will need to review all the MDIO bus drivers to make sure they correctly set the capabilities. There is something like 55 using of_mdiobus_register() and 45 using mdiobus_register(). So you have 100 drivers to review. Andrew
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Mon, Apr 05, 2021 at 03:33:55PM +0200, Andrew Lunn wrote: > On Sun, Apr 04, 2021 at 09:23:55PM +0200, Danilo Krummrich wrote: > > So currently every driver should check for the flag MII_ADDR_C45 and report > > an > > error in case it's unsupported. > > > > What do you think about checking the bus' capabilities instead in > > mdiobus_c45_*()? This way the check if C45 is supported can even happen > > before > > calling the driver at all. I think that would be a little cleaner than > > having > > two places where information of the bus' capabilities are stored (return > > value > > of read/write functions and the capabilities field). > > > > I think there are not too many drivers setting their capabilities though, > > but > > it should be easy to derive this information from how and if they handle the > > MII_ADDR_C45 flag. > > I actually don't think anything needs to change. The Marvell PHY > probably probes due to its C22 IDs. The driver will then requests C45 > access which automagically get converted into C45 over C22 for your > hardware, but remain C45 access for bus drivers which support C45. > Thanks Andrew - I agree, for the Marvell PHY to work I likly don't need any change, since I also expect that it will probe with the C22 IDs. I'll try this soon. However, this was about something else - Russell wrote: > > > We have established that MDIO drivers need to reject accesses for > > > reads/writes that they do not support [..] The MDIO drivers do this by checking the MII_ADDR_C45 flag if it's a C45 bus request. In case they don't support it they return -EOPNOTSUPP. So basically, the bus drivers read/write functions (should) encode the capability of doing C45 transfers. I just noted that this is redundant to the bus' capabilities field of struct mii_bus which also encodes the bus' capabilities of doing C22 and/or C45 transfers. Now, instead of encoding this information of the bus' capabilities at both places, I'd propose just checking the mii_bus->capabilities field in the mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having two places where this information is stored. What do you think about that? > Andrew
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Sun, Apr 04, 2021 at 09:23:55PM +0200, Danilo Krummrich wrote: > On Fri, Apr 02, 2021 at 01:58:58PM +0100, Russell King - ARM Linux admin > wrote: > > On Fri, Apr 02, 2021 at 03:10:49AM +0200, Danilo Krummrich wrote: > > > On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin > > > wrote: > > > > One could also argue this is a feature, and it allows userspace to > > > > know whether C45 cycles are supported or not. > > > > > > > No, if the userspace requests such a transfer although the MDIO controller > > > does not support real c45 framing the kernel will call mdiobus_c45_addr() > > > to > > > join the devaddr and and regaddr in one parameter and pass it to > > > mdiobus_read() or mdiobus_write(). A bus driver not supporting c45 framing > > > will not care and just mask/shift the joined value and write it to the > > > particular register. Obviously, this will result into complete garbage > > > being > > > read or (even worse) written. > > > > > > We have established that MDIO drivers need to reject accesses for > > reads/writes that they do not support - this isn't something that > > they have historically checked for because it is only recent that > > phylib has really started to support clause 45 PHYs. > > > I see, that's why you consider it a feature - because it is. > What do you think about adding a flag MDIO_PHY_ID_MMD (or similar) analog to > MDIO_PHY_ID_C45 for phy_mii_ioctl() to check for, such that userspace can ask > for an indirect access in order to save userspace doing the indirect access > itself. A nice side effect would be saving 3 syscalls per request. We don't care about the performance of this IOCTL interface. It is for debug only, and you need to be very careful how you use it, because you can very easily shoot yourself in the foot. > So currently every driver should check for the flag MII_ADDR_C45 and report an > error in case it's unsupported. > > What do you think about checking the bus' capabilities instead in > mdiobus_c45_*()? This way the check if C45 is supported can even happen before > calling the driver at all. I think that would be a little cleaner than having > two places where information of the bus' capabilities are stored (return value > of read/write functions and the capabilities field). > > I think there are not too many drivers setting their capabilities though, but > it should be easy to derive this information from how and if they handle the > MII_ADDR_C45 flag. I actually don't think anything needs to change. The Marvell PHY probably probes due to its C22 IDs. The driver will then requests C45 access which automagically get converted into C45 over C22 for your hardware, but remain C45 access for bus drivers which support C45. Andrew
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Fri, Apr 02, 2021 at 01:58:58PM +0100, Russell King - ARM Linux admin wrote: > On Fri, Apr 02, 2021 at 03:10:49AM +0200, Danilo Krummrich wrote: > > On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin > > wrote: > > > One could also argue this is a feature, and it allows userspace to > > > know whether C45 cycles are supported or not. > > > > > No, if the userspace requests such a transfer although the MDIO controller > > does not support real c45 framing the kernel will call mdiobus_c45_addr() to > > join the devaddr and and regaddr in one parameter and pass it to > > mdiobus_read() or mdiobus_write(). A bus driver not supporting c45 framing > > will not care and just mask/shift the joined value and write it to the > > particular register. Obviously, this will result into complete garbage being > > read or (even worse) written. > > > We have established that MDIO drivers need to reject accesses for > reads/writes that they do not support - this isn't something that > they have historically checked for because it is only recent that > phylib has really started to support clause 45 PHYs. > I see, that's why you consider it a feature - because it is. What do you think about adding a flag MDIO_PHY_ID_MMD (or similar) analog to MDIO_PHY_ID_C45 for phy_mii_ioctl() to check for, such that userspace can ask for an indirect access in order to save userspace doing the indirect access itself. A nice side effect would be saving 3 syscalls per request. > More modern MDIO drivers check the requested access type and error > out - we need the older MDIO drivers to do the same. > So currently every driver should check for the flag MII_ADDR_C45 and report an error in case it's unsupported. What do you think about checking the bus' capabilities instead in mdiobus_c45_*()? This way the check if C45 is supported can even happen before calling the driver at all. I think that would be a little cleaner than having two places where information of the bus' capabilities are stored (return value of read/write functions and the capabilities field). I think there are not too many drivers setting their capabilities though, but it should be easy to derive this information from how and if they handle the MII_ADDR_C45 flag.
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Fri, Apr 02, 2021 at 02:28:54PM +0200, Andrew Lunn wrote: > > > Do you actually have a requirement for this? > > > > > Yes, the Marvell 88Q2112 1000Base-T1 PHY. But actually, I just recognize > > that it > > should be possible to just register it with the compatible string > > "ethernet-phy-ieee802.3-c22" instead of "ethernet-phy-ieee802.3-c45", this > > should result in probing it as c22 PHY and doing indirect accesses through > > phy_*_mmd(). > > Hi Danilo > > Do you plan to submit a driver for this? > Hi Andrew, Yes, I'll get it ready once I got some spare time. > Does this device have an ID in register 2 and 3 in C22 space? > Currently, I don't have access to the datasheet and I don't remember. In a couple of days I should have access to the HW again and I will try. > Andrew Cheers, Danilo
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Fri, Apr 02, 2021 at 03:10:49AM +0200, Danilo Krummrich wrote: > On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin > wrote: > > Do you actually have a requirement for this? > > > Yes, the Marvell 88Q2112 1000Base-T1 PHY. But actually, I just recognize that > it > should be possible to just register it with the compatible string > "ethernet-phy-ieee802.3-c22" instead of "ethernet-phy-ieee802.3-c45", this > should result in probing it as c22 PHY and doing indirect accesses through > phy_*_mmd(). Unfortunately, I let my Marvell Extranet access expire last year, so I can't grab the datasheet for the 88Q2112 to see what Marvell claim it supports. > > One could also argue this is a feature, and it allows userspace to > > know whether C45 cycles are supported or not. > > > No, if the userspace requests such a transfer although the MDIO controller > does not support real c45 framing the kernel will call mdiobus_c45_addr() to > join the devaddr and and regaddr in one parameter and pass it to > mdiobus_read() or mdiobus_write(). A bus driver not supporting c45 framing > will not care and just mask/shift the joined value and write it to the > particular register. Obviously, this will result into complete garbage being > read or (even worse) written. We have established that MDIO drivers need to reject accesses for reads/writes that they do not support - this isn't something that they have historically checked for because it is only recent that phylib has really started to support clause 45 PHYs. More modern MDIO drivers check the requested access type and error out - we need the older MDIO drivers to do the same. > > In summary, I think you need to show us that you have a real world > > use case for these changes - in other words, a real world PHY setup > > that doesn't work today. > > > My concrete setup would have been the PHY I mentioned above, but if I'm right > with my assumption that I can just use "ethernet-phy-ieee802.3-c22" for it, > I don't have a concreate setup anymore. > > However, the kernel provides the option to register arbitrary MDIO drivers > with > mdio_driver_register() where a device could support c45 and live on a c22 only > bus. For such devices the fallback to indirect accesses would be useful as > well, > since they can't use the existing indirect access functions, because they're > specific to PHYs. However, currently there aren't such drivers in the kernel. > I just want to mention this for completeness. Right, and in such a scenario, I think using the PHY compatible property that allows you to specify the IDs will do exactly what you need. Yes, is_c45 will be false, which is exactly what you want in this case. The PHY specific C45 driver will still recognise the PHY ID, and bind to the PHY device. It will then issue the MMD accesses, which will go through the indirect registers. So, I don't immediately see any need to change the kernel code for this. However, you say you don't have this setup anymore, so there's no way to test this. I suggest we wait until we do have such a setup where it can be tested and proven to work, rather than changing the kernel code to try adding something that we've no way to test - but at the same time where making those changes has its own risk of causing regressions. Without it being tested, the cost/benefit ratio is weighed to the change being costly. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
> > Do you actually have a requirement for this? > > > Yes, the Marvell 88Q2112 1000Base-T1 PHY. But actually, I just recognize that > it > should be possible to just register it with the compatible string > "ethernet-phy-ieee802.3-c22" instead of "ethernet-phy-ieee802.3-c45", this > should result in probing it as c22 PHY and doing indirect accesses through > phy_*_mmd(). Hi Danilo Do you plan to submit a driver for this? Does this device have an ID in register 2 and 3 in C22 space? Andrew
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin wrote: > On Thu, Apr 01, 2021 at 03:23:05AM +0200, danilokrummr...@dk-develop.de wrote: > > On 2021-03-31 20:35, Russell King - ARM Linux admin wrote: > > > On Wed, Mar 31, 2021 at 07:58:33PM +0200, danilokrummr...@dk-develop.de > > > wrote: > > > > For this cited change the only thing happening is that if > > > > get_phy_device() > > > > already failed for probing with is_c45==false (C22 devices) it tries > > > > to > > > > probe with is_c45==true (C45 devices) which then either results into > > > > actual > > > > C45 frame transfers or indirect accesses by calling mdiobus_c45_*() > > > > functions. > > > > > > Please explain why and how a PHY may not appear to be present using > > > C22 frames to read the ID registers, but does appear to be present > > > when using C22 frames to the C45 indirect registers - and summarise > > > which PHYs have this behaviour. > > > > > > It seems very odd that any PHY would only implement C45 indirect > > > registers in the C22 register space. > > > > Honestly, I can't list examples of that case (at least none that have an > > upstream driver already). This part of my patch, to fall back to c45 bus > > probing when c22 probing does not succeed, is also motivated by the fact > > that this behaviour was already introduced with this patch: > > So, if I understand what you've just said, you want to make a change to > the kernel to add support for something that you don't need and don't > know that there's any hardware that needs it. Is that correct? > No, not at all. As I explained this part of the patch in mdiobus_scan() I did based on the patch of Jeremy only. It was an indicator for me that there might be c45 PHYs that don't respond to c22 requests. I interpreted his commit message in a way that those c45 PHYs are capable of processing c22 requests in general but implement the indirect registers only, since he said "its possible that a c45 device doesn't respond despite being a standard phy". You said that this behaviour would be very odd and I agree. Now, likely I just misinterpreted this and Jeremy actually tells that the PHYs he's referring to don't support c22 access at all. In this case we can for sure just forget about the changes in mdiobus_scan() of this patch. I'll remove them. Again, just to sort this out, this part of the patch is not it's main purpose. However, since I implemented the fallback to indirect accesses anyways I thought it doesn't hurt to consider the case that a PHY implements indirect access registers only. Anyways, I admit that this is likely pointless and as said, I'll remove it from the patch. > > commit 0cc8fecf041d3e5285380da62cc6662bdc942d8c > > Author: Jeremy Linton > > Date: Mon Jun 22 20:35:32 2020 +0530 > > > > net: phy: Allow mdio buses to auto-probe c45 devices > > > > The mdiobus_scan logic is currently hardcoded to only > > work with c22 devices. This works fairly well in most > > cases, but its possible that a c45 device doesn't respond > > despite being a standard phy. If the parent hardware > > is capable, it makes sense to scan for c22 devices before > > falling back to c45. > > > > As we want this to reflect the capabilities of the STA, > > lets add a field to the mii_bus structure to represent > > the capability. That way devices can opt into the extended > > scanning. Existing users should continue to default to c22 > > only scanning as long as they are zero'ing the structure > > before use. > > > > Signed-off-by: Jeremy Linton > > Signed-off-by: Calvin Johnson > > Signed-off-by: David S. Miller > > > > In this patch i.a. the following lines were added. > > > > + case MDIOBUS_C22_C45: > > + phydev = get_phy_device(bus, addr, false); > > + if (IS_ERR(phydev)) > > + phydev = get_phy_device(bus, addr, true); > > + break; > > > > I'm applying the same logic for MDIOBUS_NO_CAP and MDIOBUS_C22, since > > with my patch MDIO controllers with those capabilities can handle c45 bus > > probing with indirect accesses. > > If the PHY doesn't respond to C22 accesses but is a C45 PHY, then how > can this work (you seem to have essentially said it doesn't above.) > As stated above likely I wrongly interpreted his commit message as if only the indirect registers are implemented. > > [By the way, I'm unsure if this order for MDIO bus controllers with the > > capability MDIOBUS_C22_C45 makes sense, because if we assume that the > > majority of c45 PHYs responds well to c22 probing (which I'm convinced of) > > There are some which don't - Clause 45 allows PHYs not to implement > support for Clause 22 accesses. > Yes, I agree. > > the PHY would still be registered as is_c45==false, which results in the > > fact > > that even though the underlying bus is capable of real c45 framing only > > indirect accessing would be performed. But thi
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Thu, Apr 01, 2021 at 03:23:05AM +0200, danilokrummr...@dk-develop.de wrote: > On 2021-03-31 20:35, Russell King - ARM Linux admin wrote: > > On Wed, Mar 31, 2021 at 07:58:33PM +0200, danilokrummr...@dk-develop.de > > wrote: > > > For this cited change the only thing happening is that if > > > get_phy_device() > > > already failed for probing with is_c45==false (C22 devices) it tries > > > to > > > probe with is_c45==true (C45 devices) which then either results into > > > actual > > > C45 frame transfers or indirect accesses by calling mdiobus_c45_*() > > > functions. > > > > Please explain why and how a PHY may not appear to be present using > > C22 frames to read the ID registers, but does appear to be present > > when using C22 frames to the C45 indirect registers - and summarise > > which PHYs have this behaviour. > > > > It seems very odd that any PHY would only implement C45 indirect > > registers in the C22 register space. > > Honestly, I can't list examples of that case (at least none that have an > upstream driver already). This part of my patch, to fall back to c45 bus > probing when c22 probing does not succeed, is also motivated by the fact > that this behaviour was already introduced with this patch: So, if I understand what you've just said, you want to make a change to the kernel to add support for something that you don't need and don't know that there's any hardware that needs it. Is that correct? > commit 0cc8fecf041d3e5285380da62cc6662bdc942d8c > Author: Jeremy Linton > Date: Mon Jun 22 20:35:32 2020 +0530 > > net: phy: Allow mdio buses to auto-probe c45 devices > > The mdiobus_scan logic is currently hardcoded to only > work with c22 devices. This works fairly well in most > cases, but its possible that a c45 device doesn't respond > despite being a standard phy. If the parent hardware > is capable, it makes sense to scan for c22 devices before > falling back to c45. > > As we want this to reflect the capabilities of the STA, > lets add a field to the mii_bus structure to represent > the capability. That way devices can opt into the extended > scanning. Existing users should continue to default to c22 > only scanning as long as they are zero'ing the structure > before use. > > Signed-off-by: Jeremy Linton > Signed-off-by: Calvin Johnson > Signed-off-by: David S. Miller > > In this patch i.a. the following lines were added. > > + case MDIOBUS_C22_C45: > + phydev = get_phy_device(bus, addr, false); > + if (IS_ERR(phydev)) > + phydev = get_phy_device(bus, addr, true); > + break; > > I'm applying the same logic for MDIOBUS_NO_CAP and MDIOBUS_C22, since > with my patch MDIO controllers with those capabilities can handle c45 bus > probing with indirect accesses. If the PHY doesn't respond to C22 accesses but is a C45 PHY, then how can this work (you seem to have essentially said it doesn't above.) > [By the way, I'm unsure if this order for MDIO bus controllers with the > capability MDIOBUS_C22_C45 makes sense, because if we assume that the > majority of c45 PHYs responds well to c22 probing (which I'm convinced of) There are some which don't - Clause 45 allows PHYs not to implement support for Clause 22 accesses. > the PHY would still be registered as is_c45==false, which results in the > fact > that even though the underlying bus is capable of real c45 framing only > indirect accessing would be performed. But this is another topic and > unrelated to the patch.] We make no distinction between IDs found via Clause 22 probing and IDs found via Clause 45 probing; the PHY driver will match either way. We don't know of any cases where the IDs are different between these two. So we still end up with the same driver being matched. Also note that there are MDIO controllers that support clause 22 and clause 45, but which require clause 22 to be probed first. Not everything is MDIO at the bus level anymore - there's PHYs that support I2C, and the I2C format of a clause 45 read is identical to a clause 22 write. > In the case a PHY is registered as a c45 compatible PHY in the device tree, > it is probed in c45 mode and therefore finally mdiobus_c45_read() is > called, > which as by now just expects the underlying MDIO bus controller to be > capable > to do c45 framing and therefore the operation would fail in case it is not. > Hence, in my opinion it is useful to fall back to indirect accesses in such > a > case to be able to support those PHYs. Do you actually have a requirement for this? > There is a similar issue in phy_mii_ioctl(). Let's assume a c45 capable PHY > is > connected to a MDIO bus controller that is not capable of c45 framing. We > can > also assume that it was probed with c22 bus probing, since without this > patch > nothing else is possible. > Now, there might be an ioctl() asking for a c45 transfer by specifying > MDIO_PHY
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On 2021-03-31 20:35, Russell King - ARM Linux admin wrote: On Wed, Mar 31, 2021 at 07:58:33PM +0200, danilokrummr...@dk-develop.de wrote: For this cited change the only thing happening is that if get_phy_device() already failed for probing with is_c45==false (C22 devices) it tries to probe with is_c45==true (C45 devices) which then either results into actual C45 frame transfers or indirect accesses by calling mdiobus_c45_*() functions. Please explain why and how a PHY may not appear to be present using C22 frames to read the ID registers, but does appear to be present when using C22 frames to the C45 indirect registers - and summarise which PHYs have this behaviour. It seems very odd that any PHY would only implement C45 indirect registers in the C22 register space. Honestly, I can't list examples of that case (at least none that have an upstream driver already). This part of my patch, to fall back to c45 bus probing when c22 probing does not succeed, is also motivated by the fact that this behaviour was already introduced with this patch: commit 0cc8fecf041d3e5285380da62cc6662bdc942d8c Author: Jeremy Linton Date: Mon Jun 22 20:35:32 2020 +0530 net: phy: Allow mdio buses to auto-probe c45 devices The mdiobus_scan logic is currently hardcoded to only work with c22 devices. This works fairly well in most cases, but its possible that a c45 device doesn't respond despite being a standard phy. If the parent hardware is capable, it makes sense to scan for c22 devices before falling back to c45. As we want this to reflect the capabilities of the STA, lets add a field to the mii_bus structure to represent the capability. That way devices can opt into the extended scanning. Existing users should continue to default to c22 only scanning as long as they are zero'ing the structure before use. Signed-off-by: Jeremy Linton Signed-off-by: Calvin Johnson Signed-off-by: David S. Miller In this patch i.a. the following lines were added. + case MDIOBUS_C22_C45: + phydev = get_phy_device(bus, addr, false); + if (IS_ERR(phydev)) + phydev = get_phy_device(bus, addr, true); + break; I'm applying the same logic for MDIOBUS_NO_CAP and MDIOBUS_C22, since with my patch MDIO controllers with those capabilities can handle c45 bus probing with indirect accesses. [By the way, I'm unsure if this order for MDIO bus controllers with the capability MDIOBUS_C22_C45 makes sense, because if we assume that the majority of c45 PHYs responds well to c22 probing (which I'm convinced of) the PHY would still be registered as is_c45==false, which results in the fact that even though the underlying bus is capable of real c45 framing only indirect accessing would be performed. But this is another topic and unrelated to the patch.] However, this is not the main motivation of my patch. The main driver is of_mdiobus_register_phy(): is_c45 = of_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"); if (!is_c45 && !of_get_phy_id(child, &phy_id)) phy = phy_device_create(mdio, addr, phy_id, 0, NULL); else phy = get_phy_device(mdio, addr, is_c45); In the case a PHY is registered as a c45 compatible PHY in the device tree, it is probed in c45 mode and therefore finally mdiobus_c45_read() is called, which as by now just expects the underlying MDIO bus controller to be capable to do c45 framing and therefore the operation would fail in case it is not. Hence, in my opinion it is useful to fall back to indirect accesses in such a case to be able to support those PHYs. There is a similar issue in phy_mii_ioctl(). Let's assume a c45 capable PHY is connected to a MDIO bus controller that is not capable of c45 framing. We can also assume that it was probed with c22 bus probing, since without this patch nothing else is possible. Now, there might be an ioctl() asking for a c45 transfer by specifying MDIO_PHY_ID_C45_MASK e.g. in order to access a different MMD's register, since the PHY is actually capable of c45. Currently, this would result in devad = mdiobus_c45_addr(devad, mii_data->reg_num); mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad, devad); calls, which would fail, since the bus doesn't support it. Instead falling back to indirect access might be the better option. Surely, the userspace program could implement the indirect access as well, but I think this way it's just more convenient, e.g. "phytool read iface/addr:devad/reg".
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Wed, Mar 31, 2021 at 07:58:33PM +0200, danilokrummr...@dk-develop.de wrote: > For this cited change the only thing happening is that if get_phy_device() > already failed for probing with is_c45==false (C22 devices) it tries to > probe with is_c45==true (C45 devices) which then either results into actual > C45 frame transfers or indirect accesses by calling mdiobus_c45_*() functions. Please explain why and how a PHY may not appear to be present using C22 frames to read the ID registers, but does appear to be present when using C22 frames to the C45 indirect registers - and summarise which PHYs have this behaviour. It seems very odd that any PHY would only implement C45 indirect registers in the C22 register space. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
Hi Andrew, On 2021-03-31 18:27, Andrew Lunn wrote: @@ -670,19 +670,21 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr) struct phy_device *phydev = ERR_PTR(-ENODEV); int err; + /* In case of NO_CAP and C22 only, we still can try to scan for C45 + * devices, since indirect access will be used for busses that are not +* capable of C45 frame format. +*/ switch (bus->capabilities) { case MDIOBUS_NO_CAP: case MDIOBUS_C22: - phydev = get_phy_device(bus, addr, false); - break; - case MDIOBUS_C45: - phydev = get_phy_device(bus, addr, true); - break; case MDIOBUS_C22_C45: phydev = get_phy_device(bus, addr, false); if (IS_ERR(phydev)) phydev = get_phy_device(bus, addr, true); break; + case MDIOBUS_C45: + phydev = get_phy_device(bus, addr, true); + break; } I think this is going to cause problems. Please note that this patch does not change already existing behaviour, it does only extend it with the option to drive C45 peripherals from a bus not being capable of C45 framing. For this cited change the only thing happening is that if get_phy_device() already failed for probing with is_c45==false (C22 devices) it tries to probe with is_c45==true (C45 devices) which then either results into actual C45 frame transfers or indirect accesses by calling mdiobus_c45_*() functions. This is a valid approach, since we made sure that even if the MDIO controller does not support C45 framing we can go with indirect accesses. commit 0231b1a074c672f8c00da00a57144072890d816b Author: Kevin Hao Date: Tue Mar 20 09:44:53 2018 +0800 net: phy: realtek: Use the dummy stubs for MMD register access for rtl8211b The Ethernet on mpc8315erdb is broken since commit b6b5e8a69118 ("gianfar: Disable EEE autoneg by default"). The reason is that even though the rtl8211b doesn't support the MMD extended registers access, it does return some random values if we trying to access the MMD register via indirect method. This makes it seem that the EEE is supported by this phy device. And the subsequent writing to the MMD registers does cause the phy malfunction. So use the dummy stubs for the MMD register access to fix this issue. This is something different, here the issue is that an indirect access does not work with a PHY being registered with is_c45==false. This is not related to this change. Indirect access to C45 via C22 is not a guaranteed part of C22. So there are C22 only PHYs which return random junk when you try to use this access method. For C22 only PHYs nothing will change. If there is not an indirect access to a C22 PHY already, then there will not be one by applying this patch either. I'm also a bit confused why this is actually needed. PHY drivers which make use of C45 use the functions phy_read_mmd(), phy_write_mmd(). I'm looking on it from the perspective of the MDIO controller. If the controller is not capable of C45 framing this doesn't help. From only the PHY's capability point of view this is fine, indeed. int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum) { int val; if (regnum > (u16)~0 || devad > 32) return -EINVAL; if (phydev->drv && phydev->drv->read_mmd) { val = phydev->drv->read_mmd(phydev, devad, regnum); } else if (phydev->is_c45) { val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr, devad, regnum); } else { struct mii_bus *bus = phydev->mdio.bus; int phy_addr = phydev->mdio.addr; mmd_phy_indirect(bus, phy_addr, devad, regnum); /* Read the content of the MMD's selected register */ val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA); } return val; } So if the device is a c45 device, C45 transfers are used, otherwise it That's the issue I'm addressing, if the device is a C45 device and the MDIO controller is not capable of C45 framing, currently, the device won't be probed as a C45 device, because mdiobus_c45_read() will fail. Instead with this patch mdiobus_c45_read() can check the bus's capabilities and perform indirect accesses in case the MDIO controller is not capable of C45 framing, and therefore be able to probe C45 PHYs from a MDIO controller not being capable of C45 framing. falls back to mmd_phy_indirect(), which is C45 over C22. Only if is_c45==false, that's not what I'm addressing. I'm addressing the case that is_c45==true, but the MDIO controller does not support C45 framing. Why does this not work for you? As explained inline above. Andrew Kind regards, Danilo
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
> @@ -670,19 +670,21 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, > int addr) > struct phy_device *phydev = ERR_PTR(-ENODEV); > int err; > > + /* In case of NO_CAP and C22 only, we still can try to scan for C45 > + * devices, since indirect access will be used for busses that are not > + * capable of C45 frame format. > + */ > switch (bus->capabilities) { > case MDIOBUS_NO_CAP: > case MDIOBUS_C22: > - phydev = get_phy_device(bus, addr, false); > - break; > - case MDIOBUS_C45: > - phydev = get_phy_device(bus, addr, true); > - break; > case MDIOBUS_C22_C45: > phydev = get_phy_device(bus, addr, false); > if (IS_ERR(phydev)) > phydev = get_phy_device(bus, addr, true); > break; > + case MDIOBUS_C45: > + phydev = get_phy_device(bus, addr, true); > + break; > } I think this is going to cause problems. commit 0231b1a074c672f8c00da00a57144072890d816b Author: Kevin Hao Date: Tue Mar 20 09:44:53 2018 +0800 net: phy: realtek: Use the dummy stubs for MMD register access for rtl8211b The Ethernet on mpc8315erdb is broken since commit b6b5e8a69118 ("gianfar: Disable EEE autoneg by default"). The reason is that even though the rtl8211b doesn't support the MMD extended registers access, it does return some random values if we trying to access the MMD register via indirect method. This makes it seem that the EEE is supported by this phy device. And the subsequent writing to the MMD registers does cause the phy malfunction. So use the dummy stubs for the MMD register access to fix this issue. Indirect access to C45 via C22 is not a guaranteed part of C22. So there are C22 only PHYs which return random junk when you try to use this access method. I'm also a bit confused why this is actually needed. PHY drivers which make use of C45 use the functions phy_read_mmd(), phy_write_mmd(). int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum) { int val; if (regnum > (u16)~0 || devad > 32) return -EINVAL; if (phydev->drv && phydev->drv->read_mmd) { val = phydev->drv->read_mmd(phydev, devad, regnum); } else if (phydev->is_c45) { val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr, devad, regnum); } else { struct mii_bus *bus = phydev->mdio.bus; int phy_addr = phydev->mdio.addr; mmd_phy_indirect(bus, phy_addr, devad, regnum); /* Read the content of the MMD's selected register */ val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA); } return val; } So if the device is a c45 device, C45 transfers are used, otherwise it falls back to mmd_phy_indirect(), which is C45 over C22. Why does this not work for you? Andrew
[PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
There are still a lot of mdio controllers which don't support the clause 45 frame format as well as drivers for mdio controllers which don't implement the cause 45 mode of the controller even if natively supported by the hardware. Therefore it makes sense to support clause 45 peripherals on busses that support clause 22 transfers only by indirect access. In order to do so we can use the capabilitiy field of the struct mii_bus to distinguish between busses that natively support clause 45 and those who don't. Based on that the mdiobus_c45_*() functions can either issue a MII_ADDR_C45 flagged request to the bus driver or perform an indirect access. The indirect access is performed by the introduced mdiobus_*_mmd() functions. While performing the indirect access sequence in mdiobus_indirect_mmd() we check for potential errors occurring in the sequence, which was not done previously and just assumed to be successful. Signed-off-by: Danilo Krummrich --- drivers/net/phy/mdio_bus.c | 265 - drivers/net/phy/phy-core.c | 46 ++- drivers/net/phy/phy.c | 19 ++- include/linux/mdio.h | 36 ++--- 4 files changed, 298 insertions(+), 68 deletions(-) diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index d03e40a0fbae..c80ed65666ac 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -670,19 +670,21 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr) struct phy_device *phydev = ERR_PTR(-ENODEV); int err; + /* In case of NO_CAP and C22 only, we still can try to scan for C45 +* devices, since indirect access will be used for busses that are not +* capable of C45 frame format. +*/ switch (bus->capabilities) { case MDIOBUS_NO_CAP: case MDIOBUS_C22: - phydev = get_phy_device(bus, addr, false); - break; - case MDIOBUS_C45: - phydev = get_phy_device(bus, addr, true); - break; case MDIOBUS_C22_C45: phydev = get_phy_device(bus, addr, false); if (IS_ERR(phydev)) phydev = get_phy_device(bus, addr, true); break; + case MDIOBUS_C45: + phydev = get_phy_device(bus, addr, true); + break; } if (IS_ERR(phydev)) @@ -903,6 +905,259 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val) } EXPORT_SYMBOL(mdiobus_write); +/** + * mdiobus_indirect_mmd - Prepares MMD indirect access + * @bus: the mii_bus struct + * @addr: the phy address + * @devad: the device address + * @regnum: register number to read + * + * Prepares indirect MMD access, such that only the MII_MMD_DATA register is + * left to be read or written. Caller must hold the mdio bus lock. + * + * NOTE: MUST NOT be called from interrupt context. + */ +static int mdiobus_indirect_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum) +{ + int err; + + /* Write the desired MMD Devad */ + err = __mdiobus_write(bus, addr, MII_MMD_CTRL, devad); + if (err) + goto out; + + /* Write the desired MMD register address */ + err = __mdiobus_write(bus, addr, MII_MMD_DATA, regnum); + if (err) + goto out; + + /* Select the Function : DATA with no post increment */ + err = __mdiobus_write(bus, addr, MII_MMD_CTRL, + devad | MII_MMD_CTRL_NOINCR); + +out: + return err; +} + +/** + * __mdiobus_read_mmd - Unlocked version of the mdiobus_read_mmd function + * @bus: the mii_bus struct + * @addr: the phy address + * @devad: the device address + * @regnum: register number to read + * + * Read a MDIO bus register. Caller must hold the mdio bus lock. + * + * NOTE: MUST NOT be called from interrupt context. + */ +int __mdiobus_read_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum) +{ + int retval; + + retval = mdiobus_indirect_mmd(bus, addr, devad, regnum); + if (retval) + goto out; + + /* Read the content of the MMD's selected register */ + retval = __mdiobus_read(bus, addr, MII_MMD_DATA); + +out: + return retval; +} +EXPORT_SYMBOL(__mdiobus_read_mmd); + +/** + * __mdiobus_write_mmd - Unlocked version of the mdiobus_write_mmd function + * @bus: the mii_bus struct + * @addr: the phy address + * @devad: the device address + * @regnum: register number to write + * @val: value to write to @regnum + * + * Write a MDIO bus register. Caller must hold the mdio bus lock. + * + * NOTE: MUST NOT be called from interrupt context. + */ +int __mdiobus_write_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum, + u16 val) +{ + int err; + + err = mdiobus_indirect_mmd(bus, addr, devad, regnum); + if (err) + goto out; + + /* Write the data into MMD's selected register */ + err = __mdiobus_w