Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Mon, May 11, 2020 at 03:04:57PM +0200, Andrew Lunn wrote: > > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection. > > > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) > > MDIO-2 ==> one 25G PHY > > It has been suggested that ACPI only support a one to one > mapping. Each MAC has one MDIO bus, with one PHY on it. KISS. > > This clearly does not work for your hardware. So not only do we need > to solve how PHY properties are described, we also need an equivalent > of phy-handle, so a MAC can indicate which PHY it is connected to. > Right. I had introduced fwnode_get_phy_node() to take care of phy-handle. > So in effect, you seem to be heading towards a pretty full > reproduction of the DT binding. Before you go too much further and > waste too much of your time, you might want confirmation from the ACPI > people this is not too advanced for what ACPI can do and they tell you > to forget ACPI for this hardware and stick with DT. I've copied the patchset to Rafael and acpi ML. Waiting for their response. Thanks Calvin
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Mon, May 11, 2020 at 03:04:57PM +0200, Andrew Lunn wrote: > > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection. > > > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) > > MDIO-2 ==> one 25G PHY > > It has been suggested that ACPI only support a one to one > mapping. Each MAC has one MDIO bus, with one PHY on it. KISS. > > This clearly does not work for your hardware. So not only do we need > to solve how PHY properties are described, we also need an equivalent > of phy-handle, so a MAC can indicate which PHY it is connected to. I'd suggest that doesn't work for a lot of hardware. It won't work for the Macchiatobin for example, where there are two Clause 45 NBASE-T PHYs on one MDIO bus. The same is likely true on the LX2160A - there can be multiple ethernet interfaces, but IIRC only two external MDIO buses that one can hang PHYs off of. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
> NXP's LX2160ARDB platform currently has the following MDIO-PHY connection. > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) > MDIO-2 ==> one 25G PHY It has been suggested that ACPI only support a one to one mapping. Each MAC has one MDIO bus, with one PHY on it. KISS. This clearly does not work for your hardware. So not only do we need to solve how PHY properties are described, we also need an equivalent of phy-handle, so a MAC can indicate which PHY it is connected to. So in effect, you seem to be heading towards a pretty full reproduction of the DT binding. Before you go too much further and waste too much of your time, you might want confirmation from the ACPI people this is not too advanced for what ACPI can do and they tell you to forget ACPI for this hardware and stick with DT. Andrew
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Mon, May 11, 2020 at 11:22:31AM +0530, Calvin Johnson wrote: > Thanks Andrew and Jeremy for the detailed discussion! > > On Fri, May 08, 2020 at 08:13:01PM +0200, Andrew Lunn wrote: > > > > It does have a numeric version defined for EISA types. OTOH I suspect > > > > that > > > > your right. If there were a "PHY\VEN_ID&ID_" definition, it may > > > > not > > > > be ideal to parse it. Instead the normal ACPI model of exactly matching > > > > the > > > > complete string in the phy driver might be more appropriate. > > > > > > IMO, it should be fine to parse the string to extract the phy_id. Is > > > there any > > > reason why we cannot do this? > > > > Some background here, about what the PHY core does. > > > > PHYs have two ID registers. This contains vendor, device, and often > > revision of the PHY. Only the vendor part is standardised, vendors can > > decide how to use the device part, but it is common for the lowest > > nibble to be revision. The core will read these ID registers, and then > > go through all the PHY drivers registered and ask them if they support > > this ID. The drivers provide a table of IDs and masks. The mask is > > applied, and then if the ID matches, the driver is used. The mask > > allows the revision to be ignored, etc. > > > > There is a very small number of devices where the vendor messed up, > > and did not put valid contents in the ID registers. In such cases, we > > can read the IDs from device tree. These are then used in exactly the > > same way as if they were read from the device. > > > > If you want the ACPI model to be used, an exact match on the string, > > you are going to have to modify the core and the drivers. They > > currently don't have any string, and have no idea about different > > revisions which are out in the wild. > > I don't think ACPI mandates that OS driver use exact string match and not > parse > the string. > > First of all, I would suggest that we use "compatible" property instead of > _CID. > Not sure of a reason why we cannot. This will simplify implementation of > fwnode > APIs. > > Already I've pointed out couple of ASL files in tianocore where they are > already > used. > one > eg:https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl#L280 > > Even if we use _CID, I'm not sure we are prohibited from extracting characters > (phy_id) from it. > If we decide to use _CID, then we need to define somewhere and standardize > exactly how we are going to use it. I'm not sure where we can do this. Hi Calvin Whatever is decided needs to be documented as it becomes a defacto standard. Once this is in the Linux PHY core, that is how it is done for all boards using ACPI. Maybe sometime in the future when the ACPI standards committee definitively defines how this should be done, we can add a second implementation which is standards conformant. Andrew
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Mon, May 11, 2020 at 11:48:17AM +0100, Russell King - ARM Linux admin wrote: > On Mon, May 11, 2020 at 03:59:30PM +0530, Calvin Johnson wrote: > > On Mon, May 11, 2020 at 10:38:49AM +0100, Russell King - ARM Linux admin > > wrote: > > > On Mon, May 11, 2020 at 01:30:40PM +0530, Calvin Johnson wrote: > > > > On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote: > > > > > On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote: > > > > > > Hi, > > > > > > > > > > > > On 5/8/20 3:27 PM, Andrew Lunn wrote: > > > > > > > > > There is a very small number of devices where the vendor > > > > > > > > > messed up, > > > > > > > > > and did not put valid contents in the ID registers. In such > > > > > > > > > cases, we > > > > > > > > > can read the IDs from device tree. These are then used in > > > > > > > > > exactly the > > > > > > > > > same way as if they were read from the device. > > > > > > > > > > > > > > > > > > > > > > > > > Is that the case here? > > > > > > > > > > > > > > Sorry, I don't understand the question? > > > > > > > > > > > > I was asking in general, does this machine report the ID's > > > > > > correctly. > > > > > > > > > > Very likely, it does. > > > > > > > > > > > The embedded single mac:mdio per nic case seems like the normal > > > > > > case, and > > > > > > most of the existing ACPI described devices are setup that way. > > > > > > > > > > Somebody in this thread pointed to ACPI patches for the > > > > > MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet > > > > > interfaces, and two MDIO bus masters. One of the bus masters can only > > > > > do C22 and the other can only do C45. It is expected that the busses > > > > > are shared, not a nice one to one mapping. > > > > > > > > > > > But at the same time, that shifts the c22/45 question to the nic > > > > > > driver, where use of a DSD property before instantiating/probing > > > > > > MDIO isn't really a problem if needed. > > > > > > > > > > This in fact does not help you. The MAC driver has no idea what PHY is > > > > > connected to it. The MAC does not know if it is C22 or C45. It uses > > > > > the phylib abstraction which hides all this. Even if you assume 1:1, > > > > > use phy_find_first(), it will not find a C45 PHY because without > > > > > knowing there is a C45 PHY, we don't scan for it. And we should expect > > > > > C45 PHYs to become more popular in the next few years. > > > > > > > > Agree. > > > > > > > > NXP's LX2160ARDB platform currently has the following MDIO-PHY > > > > connection. > > > > > > > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) > > > > > > I'm not entirely sure you have that correct. The Clause 45 register set > > > as defined by IEEE 802.3 does not define registers for 1G negotiation, > > > unless the PHY either supports Clause 22 accesses, or implements some > > > kind of vendor extension. For a 1G PHY, this would be wasteful, and > > > likely incompatible with a lot of hardware/software. > > > > > > Conversely, Clause 22 does not define registers for 10G speeds, except > > > accessing Clause 45 registers indirectly through clause 22 registers, > > > which would also be wasteful. > > > > > Got your point. > > Let me try to clarify. > > > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) > > MDIO-2 ==> one 25G PHY > > This is the physical connection of MDIO & PHYs on the platform. > > > > For the c45 PHYs(two 10G), we use compatible > > "ethernet-phy-ieee802.3-c45"(not > > yet upstreamed). > > For c22 PHYs(two 1G), we don't mention the c45 compatible string and hence > > the > > access also will be using c22, if I'm not wrong. > > You seem to have just repeated the same mistake (it seems to be a direct > copy-n-paste of what you sent in the email I replied to) - and then gone > on to say something different. Either you're confused or you're not > writing in your email what you intend to. > > You first say "MDIO-1 ==> two 1G PHYs(C45)". You then say lower down > "For C22 PHYs (two 1G)". Both these statements can't be true. > > Similarly, you first say "MDIO-1 ==> two 10G PHYs(C22)". You then say > lower down "For the c45 PHYs(two 10G)". Again, both these statements > can't be true. > > Given that this discussion in this thread has been about C22 vs C45, I > would have thought accuracy in regard to this point would have been of > the up-most importance. > It was my mistake. Sorry about that. 1G PHYs are C22 and 10G PHYs are C45. I didn't notice the mistake, hence copied. Correcting: MDIO-1 ==> one 40G PHY, two 1G PHYs(C22), two 10G PHYs(C45) Thanks Calvin
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Mon, May 11, 2020 at 03:59:30PM +0530, Calvin Johnson wrote: > On Mon, May 11, 2020 at 10:38:49AM +0100, Russell King - ARM Linux admin > wrote: > > On Mon, May 11, 2020 at 01:30:40PM +0530, Calvin Johnson wrote: > > > On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote: > > > > On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote: > > > > > Hi, > > > > > > > > > > On 5/8/20 3:27 PM, Andrew Lunn wrote: > > > > > > > > There is a very small number of devices where the vendor messed > > > > > > > > up, > > > > > > > > and did not put valid contents in the ID registers. In such > > > > > > > > cases, we > > > > > > > > can read the IDs from device tree. These are then used in > > > > > > > > exactly the > > > > > > > > same way as if they were read from the device. > > > > > > > > > > > > > > > > > > > > > > Is that the case here? > > > > > > > > > > > > Sorry, I don't understand the question? > > > > > > > > > > I was asking in general, does this machine report the ID's correctly. > > > > > > > > Very likely, it does. > > > > > > > > > The embedded single mac:mdio per nic case seems like the normal case, > > > > > and > > > > > most of the existing ACPI described devices are setup that way. > > > > > > > > Somebody in this thread pointed to ACPI patches for the > > > > MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet > > > > interfaces, and two MDIO bus masters. One of the bus masters can only > > > > do C22 and the other can only do C45. It is expected that the busses > > > > are shared, not a nice one to one mapping. > > > > > > > > > But at the same time, that shifts the c22/45 question to the nic > > > > > driver, where use of a DSD property before instantiating/probing > > > > > MDIO isn't really a problem if needed. > > > > > > > > This in fact does not help you. The MAC driver has no idea what PHY is > > > > connected to it. The MAC does not know if it is C22 or C45. It uses > > > > the phylib abstraction which hides all this. Even if you assume 1:1, > > > > use phy_find_first(), it will not find a C45 PHY because without > > > > knowing there is a C45 PHY, we don't scan for it. And we should expect > > > > C45 PHYs to become more popular in the next few years. > > > > > > Agree. > > > > > > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection. > > > > > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) > > > > I'm not entirely sure you have that correct. The Clause 45 register set > > as defined by IEEE 802.3 does not define registers for 1G negotiation, > > unless the PHY either supports Clause 22 accesses, or implements some > > kind of vendor extension. For a 1G PHY, this would be wasteful, and > > likely incompatible with a lot of hardware/software. > > > > Conversely, Clause 22 does not define registers for 10G speeds, except > > accessing Clause 45 registers indirectly through clause 22 registers, > > which would also be wasteful. > > > Got your point. > Let me try to clarify. > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) > MDIO-2 ==> one 25G PHY > This is the physical connection of MDIO & PHYs on the platform. > > For the c45 PHYs(two 10G), we use compatible "ethernet-phy-ieee802.3-c45"(not > yet upstreamed). > For c22 PHYs(two 1G), we don't mention the c45 compatible string and hence the > access also will be using c22, if I'm not wrong. You seem to have just repeated the same mistake (it seems to be a direct copy-n-paste of what you sent in the email I replied to) - and then gone on to say something different. Either you're confused or you're not writing in your email what you intend to. You first say "MDIO-1 ==> two 1G PHYs(C45)". You then say lower down "For C22 PHYs (two 1G)". Both these statements can't be true. Similarly, you first say "MDIO-1 ==> two 10G PHYs(C22)". You then say lower down "For the c45 PHYs(two 10G)". Again, both these statements can't be true. Given that this discussion in this thread has been about C22 vs C45, I would have thought accuracy in regard to this point would have been of the up-most importance. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Mon, May 11, 2020 at 10:38:49AM +0100, Russell King - ARM Linux admin wrote: > On Mon, May 11, 2020 at 01:30:40PM +0530, Calvin Johnson wrote: > > On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote: > > > On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote: > > > > Hi, > > > > > > > > On 5/8/20 3:27 PM, Andrew Lunn wrote: > > > > > > > There is a very small number of devices where the vendor messed > > > > > > > up, > > > > > > > and did not put valid contents in the ID registers. In such > > > > > > > cases, we > > > > > > > can read the IDs from device tree. These are then used in exactly > > > > > > > the > > > > > > > same way as if they were read from the device. > > > > > > > > > > > > > > > > > > > Is that the case here? > > > > > > > > > > Sorry, I don't understand the question? > > > > > > > > I was asking in general, does this machine report the ID's correctly. > > > > > > Very likely, it does. > > > > > > > The embedded single mac:mdio per nic case seems like the normal case, > > > > and > > > > most of the existing ACPI described devices are setup that way. > > > > > > Somebody in this thread pointed to ACPI patches for the > > > MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet > > > interfaces, and two MDIO bus masters. One of the bus masters can only > > > do C22 and the other can only do C45. It is expected that the busses > > > are shared, not a nice one to one mapping. > > > > > > > But at the same time, that shifts the c22/45 question to the nic > > > > driver, where use of a DSD property before instantiating/probing > > > > MDIO isn't really a problem if needed. > > > > > > This in fact does not help you. The MAC driver has no idea what PHY is > > > connected to it. The MAC does not know if it is C22 or C45. It uses > > > the phylib abstraction which hides all this. Even if you assume 1:1, > > > use phy_find_first(), it will not find a C45 PHY because without > > > knowing there is a C45 PHY, we don't scan for it. And we should expect > > > C45 PHYs to become more popular in the next few years. > > > > Agree. > > > > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection. > > > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) > > I'm not entirely sure you have that correct. The Clause 45 register set > as defined by IEEE 802.3 does not define registers for 1G negotiation, > unless the PHY either supports Clause 22 accesses, or implements some > kind of vendor extension. For a 1G PHY, this would be wasteful, and > likely incompatible with a lot of hardware/software. > > Conversely, Clause 22 does not define registers for 10G speeds, except > accessing Clause 45 registers indirectly through clause 22 registers, > which would also be wasteful. > Got your point. Let me try to clarify. MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) MDIO-2 ==> one 25G PHY This is the physical connection of MDIO & PHYs on the platform. For the c45 PHYs(two 10G), we use compatible "ethernet-phy-ieee802.3-c45"(not yet upstreamed). For c22 PHYs(two 1G), we don't mention the c45 compatible string and hence the access also will be using c22, if I'm not wrong. Regards Calvin k
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Mon, May 11, 2020 at 01:30:40PM +0530, Calvin Johnson wrote: > On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote: > > On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote: > > > Hi, > > > > > > On 5/8/20 3:27 PM, Andrew Lunn wrote: > > > > > > There is a very small number of devices where the vendor messed up, > > > > > > and did not put valid contents in the ID registers. In such cases, > > > > > > we > > > > > > can read the IDs from device tree. These are then used in exactly > > > > > > the > > > > > > same way as if they were read from the device. > > > > > > > > > > > > > > > > Is that the case here? > > > > > > > > Sorry, I don't understand the question? > > > > > > I was asking in general, does this machine report the ID's correctly. > > > > Very likely, it does. > > > > > The embedded single mac:mdio per nic case seems like the normal case, and > > > most of the existing ACPI described devices are setup that way. > > > > Somebody in this thread pointed to ACPI patches for the > > MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet > > interfaces, and two MDIO bus masters. One of the bus masters can only > > do C22 and the other can only do C45. It is expected that the busses > > are shared, not a nice one to one mapping. > > > > > But at the same time, that shifts the c22/45 question to the nic > > > driver, where use of a DSD property before instantiating/probing > > > MDIO isn't really a problem if needed. > > > > This in fact does not help you. The MAC driver has no idea what PHY is > > connected to it. The MAC does not know if it is C22 or C45. It uses > > the phylib abstraction which hides all this. Even if you assume 1:1, > > use phy_find_first(), it will not find a C45 PHY because without > > knowing there is a C45 PHY, we don't scan for it. And we should expect > > C45 PHYs to become more popular in the next few years. > > Agree. > > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection. > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) I'm not entirely sure you have that correct. The Clause 45 register set as defined by IEEE 802.3 does not define registers for 1G negotiation, unless the PHY either supports Clause 22 accesses, or implements some kind of vendor extension. For a 1G PHY, this would be wasteful, and likely incompatible with a lot of hardware/software. Conversely, Clause 22 does not define registers for 10G speeds, except accessing Clause 45 registers indirectly through clause 22 registers, which would also be wasteful. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote: > On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote: > > Hi, > > > > On 5/8/20 3:27 PM, Andrew Lunn wrote: > > > > > There is a very small number of devices where the vendor messed up, > > > > > and did not put valid contents in the ID registers. In such cases, we > > > > > can read the IDs from device tree. These are then used in exactly the > > > > > same way as if they were read from the device. > > > > > > > > > > > > > Is that the case here? > > > > > > Sorry, I don't understand the question? > > > > I was asking in general, does this machine report the ID's correctly. > > Very likely, it does. > > > The embedded single mac:mdio per nic case seems like the normal case, and > > most of the existing ACPI described devices are setup that way. > > Somebody in this thread pointed to ACPI patches for the > MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet > interfaces, and two MDIO bus masters. One of the bus masters can only > do C22 and the other can only do C45. It is expected that the busses > are shared, not a nice one to one mapping. > > > But at the same time, that shifts the c22/45 question to the nic > > driver, where use of a DSD property before instantiating/probing > > MDIO isn't really a problem if needed. > > This in fact does not help you. The MAC driver has no idea what PHY is > connected to it. The MAC does not know if it is C22 or C45. It uses > the phylib abstraction which hides all this. Even if you assume 1:1, > use phy_find_first(), it will not find a C45 PHY because without > knowing there is a C45 PHY, we don't scan for it. And we should expect > C45 PHYs to become more popular in the next few years. Agree. NXP's LX2160ARDB platform currently has the following MDIO-PHY connection. MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22) MDIO-2 ==> one 25G PHY Both the MDIOs are capable of C45. There can be other complex use cases as well. So, it is important to configure MDIO talk both C22 and C45 for different PHYs connected on the same bus. Regards Calvin
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote: > Hi, > > On 5/8/20 3:27 PM, Andrew Lunn wrote: > > > > There is a very small number of devices where the vendor messed up, > > > > and did not put valid contents in the ID registers. In such cases, we > > > > can read the IDs from device tree. These are then used in exactly the > > > > same way as if they were read from the device. > > > > > > > > > > Is that the case here? > > > > Sorry, I don't understand the question? > > I was asking in general, does this machine report the ID's correctly. More > directed at Calvin, but part of it is the board vendor too. So I suspect no > one can really answer "yes", despite that seeming to be the case. I had tested RGMII PHYs(AR8035) with c22 using mdiobus_register. Vendor ID was read out correctly while autoprobing. AQR107(c45) PHYs that were connected gave back 0x00. This is expected because the MDIO bus is configured to talk c22. Regards Calvin
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
Thanks Andrew and Jeremy for the detailed discussion! On Fri, May 08, 2020 at 08:13:01PM +0200, Andrew Lunn wrote: > > > It does have a numeric version defined for EISA types. OTOH I suspect that > > > your right. If there were a "PHY\VEN_ID&ID_" definition, it may > > > not > > > be ideal to parse it. Instead the normal ACPI model of exactly matching > > > the > > > complete string in the phy driver might be more appropriate. > > > > IMO, it should be fine to parse the string to extract the phy_id. Is there > > any > > reason why we cannot do this? > > Some background here, about what the PHY core does. > > PHYs have two ID registers. This contains vendor, device, and often > revision of the PHY. Only the vendor part is standardised, vendors can > decide how to use the device part, but it is common for the lowest > nibble to be revision. The core will read these ID registers, and then > go through all the PHY drivers registered and ask them if they support > this ID. The drivers provide a table of IDs and masks. The mask is > applied, and then if the ID matches, the driver is used. The mask > allows the revision to be ignored, etc. > > There is a very small number of devices where the vendor messed up, > and did not put valid contents in the ID registers. In such cases, we > can read the IDs from device tree. These are then used in exactly the > same way as if they were read from the device. > > If you want the ACPI model to be used, an exact match on the string, > you are going to have to modify the core and the drivers. They > currently don't have any string, and have no idea about different > revisions which are out in the wild. I don't think ACPI mandates that OS driver use exact string match and not parse the string. First of all, I would suggest that we use "compatible" property instead of _CID. Not sure of a reason why we cannot. This will simplify implementation of fwnode APIs. Already I've pointed out couple of ASL files in tianocore where they are already used. one eg:https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl#L280 Even if we use _CID, I'm not sure we are prohibited from extracting characters (phy_id) from it. If we decide to use _CID, then we need to define somewhere and standardize exactly how we are going to use it. I'm not sure where we can do this. > > > > Similarly to how I suspect the next patch's use of "compatible" isn't > > > ideal > > > either, because whether a device is c45 or not, should tend to be fixed > > > to a > > > particular vendor/device implementation and not a firmware provided > > > property. > > Not exactly true. It is the combination of can the bus master do C45 > and can the device do C45. Unfortunately, we have no knowledge of the > bus masters capabilities, if it can do C45. And many MDIO drivers will > do a C22 transaction when asked to perform a C45 transaction. All new > submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if > C45 is not supported. But we cannot rely on that. Too much history. Makes sense to me. > > > > I tend to agree with you on this. Even for DT, ideal case, IMO should be: > > > > 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from > > registers 2 and 3 > > 2) if not found scan for c45 devices <= looks like this is missing in Linux > > 3) look for phy_id from compatible string. > > It is somewhat more complex, in that there are a small number of > devices which will respond to both C22 and C45. Generally, you want to > use C45 if supported. So you would want to do the C45 scan first. But > then the earlier problem comes to play, you have no idea if the bus > master actually correctly supports C45. > > Given the issues, we assume all bus masters and PHY devices are C22 > unless DT says the bus master and PHY combination is compatible with > C45. Makes sense to me.
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On 5/8/20 6:42 PM, Andrew Lunn wrote: On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote: Hi, On 5/8/20 3:27 PM, Andrew Lunn wrote: There is a very small number of devices where the vendor messed up, and did not put valid contents in the ID registers. In such cases, we can read the IDs from device tree. These are then used in exactly the same way as if they were read from the device. Is that the case here? Sorry, I don't understand the question? I was asking in general, does this machine report the ID's correctly. Very likely, it does. The embedded single mac:mdio per nic case seems like the normal case, and most of the existing ACPI described devices are setup that way. Somebody in this thread pointed to ACPI patches for the MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet interfaces, and two MDIO bus masters. One of the bus masters can only do C22 and the other can only do C45. It is expected that the busses are shared, not a nice one to one mapping. Thats whats going on with the NXP too AFAIK. But the mcbin is one of the ones with the "compatible" embedded in the DSD properties.. AKA, they buried the entire DT node there. But at the same time, that shifts the c22/45 question to the nic driver, where use of a DSD property before instantiating/probing MDIO isn't really a problem if needed. This in fact does not help you. The MAC driver has no idea what PHY is connected to it. The MAC does not know if it is C22 or C45. It uses Thats all I was trying to say (the mac side capability is implied by the HID/CID that instantiates it). the phylib abstraction which hides all this. Even if you assume 1:1, use phy_find_first(), it will not find a C45 PHY because without knowing there is a C45 PHY, we don't scan for it. And we should expect C45 PHYs to become more popular in the next few years. In fact this embedded nic/mac/mdio/phy 1:1:1 case, is likely a requirement for passthrough into a generic VM, otherwise someone has to create a virtual mdio, and pass the phy in for the nic/mac. AFAIK, NXP's part avoids this despite having a shared MDIO, because the phy state never leaves the mgmt side of the picture. It monitors the state and then feeds that back into their nic mgmt complex rather than using it directly. That is the other model. Don't use Linux to drive the PHY, use firmware in the MAC. A number of MACs do that, but it has the usual problems of firmware. It limits you on your choice of PHYs, bugs in the firmware cannot be fixed by the community, no sharing of drivers because firmware is generally proprietary, no 'for free features' because somebody else added features to the linux PHY driver etc. But it will make ACPI support simple, this whole discussion goes away, no ACPI needed at all. Open source firmware! :)
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote: > Hi, > > On 5/8/20 3:27 PM, Andrew Lunn wrote: > > > > There is a very small number of devices where the vendor messed up, > > > > and did not put valid contents in the ID registers. In such cases, we > > > > can read the IDs from device tree. These are then used in exactly the > > > > same way as if they were read from the device. > > > > > > > > > > Is that the case here? > > > > Sorry, I don't understand the question? > > I was asking in general, does this machine report the ID's correctly. Very likely, it does. > The embedded single mac:mdio per nic case seems like the normal case, and > most of the existing ACPI described devices are setup that way. Somebody in this thread pointed to ACPI patches for the MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet interfaces, and two MDIO bus masters. One of the bus masters can only do C22 and the other can only do C45. It is expected that the busses are shared, not a nice one to one mapping. > But at the same time, that shifts the c22/45 question to the nic > driver, where use of a DSD property before instantiating/probing > MDIO isn't really a problem if needed. This in fact does not help you. The MAC driver has no idea what PHY is connected to it. The MAC does not know if it is C22 or C45. It uses the phylib abstraction which hides all this. Even if you assume 1:1, use phy_find_first(), it will not find a C45 PHY because without knowing there is a C45 PHY, we don't scan for it. And we should expect C45 PHYs to become more popular in the next few years. > In fact this embedded nic/mac/mdio/phy 1:1:1 case, is likely a requirement > for passthrough into a generic VM, otherwise someone has to create a virtual > mdio, and pass the phy in for the nic/mac. > > AFAIK, NXP's part avoids this despite having a shared MDIO, because the phy > state never leaves the mgmt side of the picture. It monitors the state and > then feeds that back into their nic mgmt complex rather than using it > directly. That is the other model. Don't use Linux to drive the PHY, use firmware in the MAC. A number of MACs do that, but it has the usual problems of firmware. It limits you on your choice of PHYs, bugs in the firmware cannot be fixed by the community, no sharing of drivers because firmware is generally proprietary, no 'for free features' because somebody else added features to the linux PHY driver etc. But it will make ACPI support simple, this whole discussion goes away, no ACPI needed at all. Andrew
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
Hi, On 5/8/20 3:27 PM, Andrew Lunn wrote: There is a very small number of devices where the vendor messed up, and did not put valid contents in the ID registers. In such cases, we can read the IDs from device tree. These are then used in exactly the same way as if they were read from the device. Is that the case here? Sorry, I don't understand the question? I was asking in general, does this machine report the ID's correctly. More directed at Calvin, but part of it is the board vendor too. So I suspect no one can really answer "yes", despite that seeming to be the case. Also, how much of this was caused by uboot being deficient None. It is a silicon issue. The PHY chip simply has the wrong or no ID value in the registers. Not exactly true. It is the combination of can the bus master do C45 and can the device do C45. Unfortunately, we have no knowledge of the bus masters capabilities, if it can do C45. And many MDIO drivers will do a C22 transaction when asked to perform a C45 transaction. All new submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if C45 is not supported. But we cannot rely on that. Too much history > I tend to agree with you on this. Even for DT, ideal case, IMO should be: 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from registers 2 and 3 2) if not found scan for c45 devices <= looks like this is missing in Linux 3) look for phy_id from compatible string. It is somewhat more complex, in that there are a small number of devices which will respond to both C22 and C45. Generally, you want to use C45 if supported. So you would want to do the C45 scan first. But then the earlier problem comes to play, you have no idea if the bus master actually correctly supports C45. But this shouldn't this be implied by the mdio vendor/model? Nope. Many MDIO bus masters don't even appear in DT, because they are embedded into the MAC driver. The MAC driver just instantiates an MDIO device, maybe passing a pointer where to find the PHY properties in DT. If the MDIO bus master is in its own address range, then it probably does exist in device tree, and has a compatible string. But that just gets the driver loaded, it says nothing about what it is capable of, C22 and or C45. And there are cases where the MDIO bus is embedded inside an Ethernet switch, which is hanging off another MDIO bus, etc. The embedded single mac:mdio per nic case seems like the normal case, and most of the existing ACPI described devices are setup that way. But at the same time, that shifts the c22/45 question to the nic driver, where use of a DSD property before instantiating/probing MDIO isn't really a problem if needed. In fact this embedded nic/mac/mdio/phy 1:1:1 case, is likely a requirement for passthrough into a generic VM, otherwise someone has to create a virtual mdio, and pass the phy in for the nic/mac. AFAIK, NXP's part avoids this despite having a shared MDIO, because the phy state never leaves the mgmt side of the picture. It monitors the state and then feeds that back into their nic mgmt complex rather than using it directly. How much of this can be simplified for ACPI buy ignoring the legacy and putting some guides around the ACPI/platform requirements? You can probably ignore the phy-id. compatible, since that is working around silicon issues, and put in place some guidelines that the PHY silicon needs to conform to the basics of C22 and C45 in terms of ID registers. C45 you are going to need. ACPI tends to be more high end devices, which in general have higher speed network interfaces. Multi-Gige PHYs tend to be C45. But there is also interest in using ACPI on 1G PHYs where the majority is C22. Oh, I was just trying to see if we can get away with saying things like "your phy's must respond as specified by the spec" and leave it at that for the time being to simplify the probing sequence. I'm not really sure we can represent the more complex switch/etc situations in ACPI either. There is a certain amount of "use DT if you machine doesn't conform to standards".
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
> > There is a very small number of devices where the vendor messed up, > > and did not put valid contents in the ID registers. In such cases, we > > can read the IDs from device tree. These are then used in exactly the > > same way as if they were read from the device. > > > > Is that the case here? Sorry, I don't understand the question? > Also, how much of this was caused by uboot being deficient None. It is a silicon issue. The PHY chip simply has the wrong or no ID value in the registers. > > Not exactly true. It is the combination of can the bus master do C45 > > and can the device do C45. Unfortunately, we have no knowledge of the > > bus masters capabilities, if it can do C45. And many MDIO drivers will > > do a C22 transaction when asked to perform a C45 transaction. All new > > submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if > > C45 is not supported. But we cannot rely on that. Too much history > > > > > > > I tend to agree with you on this. Even for DT, ideal case, IMO should be: > > > > > > 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from > > > registers 2 and 3 > > > 2) if not found scan for c45 devices <= looks like this is missing in > > > Linux > > > 3) look for phy_id from compatible string. > > > > It is somewhat more complex, in that there are a small number of > > devices which will respond to both C22 and C45. Generally, you want to > > use C45 if supported. So you would want to do the C45 scan first. But > > then the earlier problem comes to play, you have no idea if the bus > > master actually correctly supports C45. > > But this shouldn't this be implied by the mdio vendor/model? Nope. Many MDIO bus masters don't even appear in DT, because they are embedded into the MAC driver. The MAC driver just instantiates an MDIO device, maybe passing a pointer where to find the PHY properties in DT. If the MDIO bus master is in its own address range, then it probably does exist in device tree, and has a compatible string. But that just gets the driver loaded, it says nothing about what it is capable of, C22 and or C45. And there are cases where the MDIO bus is embedded inside an Ethernet switch, which is hanging off another MDIO bus, etc. > How much of this can be simplified for ACPI buy ignoring the legacy and > putting some guides around the ACPI/platform requirements? You can probably ignore the phy-id. compatible, since that is working around silicon issues, and put in place some guidelines that the PHY silicon needs to conform to the basics of C22 and C45 in terms of ID registers. C45 you are going to need. ACPI tends to be more high end devices, which in general have higher speed network interfaces. Multi-Gige PHYs tend to be C45. But there is also interest in using ACPI on 1G PHYs where the majority is C22. Andrew
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
Hi, On 5/8/20 1:13 PM, Andrew Lunn wrote: It does have a numeric version defined for EISA types. OTOH I suspect that your right. If there were a "PHY\VEN_ID&ID_" definition, it may not be ideal to parse it. Instead the normal ACPI model of exactly matching the complete string in the phy driver might be more appropriate. IMO, it should be fine to parse the string to extract the phy_id. Is there any reason why we cannot do this? Some background here, about what the PHY core does. PHYs have two ID registers. This contains vendor, device, and often revision of the PHY. Only the vendor part is standardised, vendors can decide how to use the device part, but it is common for the lowest nibble to be revision. The core will read these ID registers, and then go through all the PHY drivers registered and ask them if they support this ID. The drivers provide a table of IDs and masks. The mask is applied, and then if the ID matches, the driver is used. The mask allows the revision to be ignored, etc. There is a very small number of devices where the vendor messed up, and did not put valid contents in the ID registers. In such cases, we can read the IDs from device tree. These are then used in exactly the same way as if they were read from the device. Is that the case here? Also, how much of this was caused by uboot being deficient, and failing to do vendor specific setup? AKA like what has been happening with the mac addresses, where it turns out the difference between a chip being used on x86 vs arm has frequently been that no one bothered to port all the option rom functionality to uboot (and in some cases edk2). If you want the ACPI model to be used, an exact match on the string, you are going to have to modify the core and the drivers. They currently don't have any string, and have no idea about different revisions which are out in the wild. Right, not pretty. But _DSD should never be used to provide functionally provided elsewhere in the spec, and with ACPI the attempt is to make the firmware less linux focused, and more generic. Similarly to how I suspect the next patch's use of "compatible" isn't ideal either, because whether a device is c45 or not, should tend to be fixed to a particular vendor/device implementation and not a firmware provided property. Not exactly true. It is the combination of can the bus master do C45 and can the device do C45. Unfortunately, we have no knowledge of the bus masters capabilities, if it can do C45. And many MDIO drivers will do a C22 transaction when asked to perform a C45 transaction. All new submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if C45 is not supported. But we cannot rely on that. Too much history > I tend to agree with you on this. Even for DT, ideal case, IMO should be: 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from registers 2 and 3 2) if not found scan for c45 devices <= looks like this is missing in Linux 3) look for phy_id from compatible string. It is somewhat more complex, in that there are a small number of devices which will respond to both C22 and C45. Generally, you want to use C45 if supported. So you would want to do the C45 scan first. But then the earlier problem comes to play, you have no idea if the bus master actually correctly supports C45. But this shouldn't this be implied by the mdio vendor/model? AKA, you wouldn't have a part with a given _HID() where the capabilities would change.You would only have a situation where the phy's capabilities change, but they must in the end support whatever is provided by the master. Given the issues, we assume all bus masters and PHY devices are C22 unless DT says the bus master and PHY combination is compatible with C45. How much of this can be simplified for ACPI buy ignoring the legacy and putting some guides around the ACPI/platform requirements? Thanks,
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
> > It does have a numeric version defined for EISA types. OTOH I suspect that > > your right. If there were a "PHY\VEN_ID&ID_" definition, it may not > > be ideal to parse it. Instead the normal ACPI model of exactly matching the > > complete string in the phy driver might be more appropriate. > > IMO, it should be fine to parse the string to extract the phy_id. Is there any > reason why we cannot do this? Some background here, about what the PHY core does. PHYs have two ID registers. This contains vendor, device, and often revision of the PHY. Only the vendor part is standardised, vendors can decide how to use the device part, but it is common for the lowest nibble to be revision. The core will read these ID registers, and then go through all the PHY drivers registered and ask them if they support this ID. The drivers provide a table of IDs and masks. The mask is applied, and then if the ID matches, the driver is used. The mask allows the revision to be ignored, etc. There is a very small number of devices where the vendor messed up, and did not put valid contents in the ID registers. In such cases, we can read the IDs from device tree. These are then used in exactly the same way as if they were read from the device. If you want the ACPI model to be used, an exact match on the string, you are going to have to modify the core and the drivers. They currently don't have any string, and have no idea about different revisions which are out in the wild. > > Similarly to how I suspect the next patch's use of "compatible" isn't ideal > > either, because whether a device is c45 or not, should tend to be fixed to a > > particular vendor/device implementation and not a firmware provided > > property. Not exactly true. It is the combination of can the bus master do C45 and can the device do C45. Unfortunately, we have no knowledge of the bus masters capabilities, if it can do C45. And many MDIO drivers will do a C22 transaction when asked to perform a C45 transaction. All new submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if C45 is not supported. But we cannot rely on that. Too much history. > > I tend to agree with you on this. Even for DT, ideal case, IMO should be: > > 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from > registers 2 and 3 > 2) if not found scan for c45 devices <= looks like this is missing in Linux > 3) look for phy_id from compatible string. It is somewhat more complex, in that there are a small number of devices which will respond to both C22 and C45. Generally, you want to use C45 if supported. So you would want to do the C45 scan first. But then the earlier problem comes to play, you have no idea if the bus master actually correctly supports C45. Given the issues, we assume all bus masters and PHY devices are C22 unless DT says the bus master and PHY combination is compatible with C45. Andrew
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Thu, May 07, 2020 at 02:54:09PM -0500, Jeremy Linton wrote:
> Hi,
>
> On 5/7/20 12:27 PM, Andy Shevchenko wrote:
> > On Thu, May 7, 2020 at 4:26 PM Jeremy Linton wrote:
> > > On 5/5/20 8:29 AM, Calvin Johnson wrote:
> >
> > > > + if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> > > > +&upper, &lower) == 2) {
> > > > + *phy_id = ((upper & 0x) << 16) | (lower &
> > > > 0x);
> > > > + return 0;
> > > > + }
> >
> > > Isn't the ACPI _CID() conceptually similar to the DT compatible
> > > property?
> >
> > Where?
>
> Not, sure I understand exactly what your asking. AFAIK, in general the dt
> property is used to select a device driver/etc based on a more to less
> compatible set of substrings. The phy case is a bit different because it
> codes a numerical part number into the string rather than just using
> arbitrary strings to select a driver and device. But it uses that as a
> vendor selector for binding to the correct driver/device.
>
> Rephrasing the ACPI spec, the _CID() is either a single compatible id, or a
> list of ids in order of preference. Each id is either a HID (string or EISA
> type id) or a bus specific string encoding vendor/device/etc.
> (https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/acpi/acpica/utids.c#L186).
> One of the examples is "PCI\VEN_&DEV_"
>
> So that latter case seems to be almost exactly what we have here.
Got your point. Yes, the ACPI spec says the same.
If we are using _CID as a string, then it must be a string that uses a
bus-specific nomenclature. This AFAIU may take the format
"PHY\VEN_ID&ID_" as you mentioned below and not
"ethernet-phy-id004d.d072" as used in DT.
So, we need to define it some where in the Linux ACPI Documentation.
I don't see any best place to document this. Any suggestions?
>
> >
> > > It even appears to be getting used in a similar way to
> > > identify particular phy drivers in this case.
> >
> > _CID() is a string. It can't be used as pure number.
> >
>
> It does have a numeric version defined for EISA types. OTOH I suspect that
> your right. If there were a "PHY\VEN_ID&ID_" definition, it may not
> be ideal to parse it. Instead the normal ACPI model of exactly matching the
> complete string in the phy driver might be more appropriate.
IMO, it should be fine to parse the string to extract the phy_id. Is there any
reason why we cannot do this?
>
> Similarly to how I suspect the next patch's use of "compatible" isn't ideal
> either, because whether a device is c45 or not, should tend to be fixed to a
> particular vendor/device implementation and not a firmware provided
> property.
I tend to agree with you on this. Even for DT, ideal case, IMO should be:
1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from
registers 2 and 3
2) if not found scan for c45 devices <= looks like this is missing in Linux
3) look for phy_id from compatible string.
Meanwhile, please note some usage of compatible property in edk2-platforms:
https://github.com/tianocore/edk2-platforms/blob/master/Platform/96Boards/Secure96Dxe/Secure96.asl#L20
https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl#L280
https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Optee.asl#L17
Regards
Calvin
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
Hi,
On 5/7/20 12:27 PM, Andy Shevchenko wrote:
On Thu, May 7, 2020 at 4:26 PM Jeremy Linton wrote:
On 5/5/20 8:29 AM, Calvin Johnson wrote:
+ if (sscanf(cp, "ethernet-phy-id%4x.%4x",
+&upper, &lower) == 2) {
+ *phy_id = ((upper & 0x) << 16) | (lower & 0x);
+ return 0;
+ }
Isn't the ACPI _CID() conceptually similar to the DT compatible
property?
Where?
Not, sure I understand exactly what your asking. AFAIK, in general the
dt property is used to select a device driver/etc based on a more to
less compatible set of substrings. The phy case is a bit different
because it codes a numerical part number into the string rather than
just using arbitrary strings to select a driver and device. But it uses
that as a vendor selector for binding to the correct driver/device.
Rephrasing the ACPI spec, the _CID() is either a single compatible id,
or a list of ids in order of preference. Each id is either a HID (string
or EISA type id) or a bus specific string encoding vendor/device/etc.
(https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/acpi/acpica/utids.c#L186).
One of the examples is "PCI\VEN_&DEV_"
So that latter case seems to be almost exactly what we have here.
It even appears to be getting used in a similar way to
identify particular phy drivers in this case.
_CID() is a string. It can't be used as pure number.
It does have a numeric version defined for EISA types. OTOH I suspect
that your right. If there were a "PHY\VEN_ID&ID_" definition, it
may not be ideal to parse it. Instead the normal ACPI model of exactly
matching the complete string in the phy driver might be more appropriate.
Similarly to how I suspect the next patch's use of "compatible" isn't
ideal either, because whether a device is c45 or not, should tend to be
fixed to a particular vendor/device implementation and not a firmware
provided property.
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Thu, May 7, 2020 at 4:26 PM Jeremy Linton wrote:
> On 5/5/20 8:29 AM, Calvin Johnson wrote:
> > + if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> > +&upper, &lower) == 2) {
> > + *phy_id = ((upper & 0x) << 16) | (lower & 0x);
> > + return 0;
> > + }
> Isn't the ACPI _CID() conceptually similar to the DT compatible
> property?
Where?
> It even appears to be getting used in a similar way to
> identify particular phy drivers in this case.
_CID() is a string. It can't be used as pure number.
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
Hi,
On 5/5/20 8:29 AM, Calvin Johnson wrote:
Extract phy_id from compatible string. This will be used by
fwnode_mdiobus_register_phy() to create phy device using the
phy_id.
Signed-off-by: Calvin Johnson
---
Changes in v3: None
Changes in v2: None
drivers/net/phy/phy_device.c | 21 +
include/linux/phy.h | 5 +
2 files changed, 26 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 4204d49586cd..f4ad47f73f8d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -662,6 +662,27 @@ struct phy_device *phy_device_create(struct mii_bus *bus,
int addr, u32 phy_id,
}
EXPORT_SYMBOL(phy_device_create);
+/* Extract the phy ID from the compatible string of the form
+ * ethernet-phy-id..
+ */
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+ const char *cp;
+ unsigned int upper, lower;
+ int ret;
+
+ ret = fwnode_property_read_string(fwnode, "compatible", &cp);
+ if (!ret) {
+ if (sscanf(cp, "ethernet-phy-id%4x.%4x",
+ &upper, &lower) == 2) {
+ *phy_id = ((upper & 0x) << 16) | (lower & 0x);
+ return 0;
+ }
Isn't the ACPI _CID() conceptually similar to the DT compatible
property? It even appears to be getting used in a similar way to
identify particular phy drivers in this case.
Thanks,
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Tue, May 05, 2020 at 05:15:16PM +0300, Andy Shevchenko wrote:
> On Tue, May 5, 2020 at 4:29 PM Calvin Johnson
> > + if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> > + &upper, &lower) == 2) {
>
> > + *phy_id = ((upper & 0x) << 16) | (lower &
> > 0x);
>
> How upper can be bigger than 0xfff? Same for lower.
I think your comment is incorrect here. Four hex digits can be larger
than 0xfff. "1000" interpreted as hex is four hex digits and larger
than 0xfff, for example.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On Tue, May 5, 2020 at 4:29 PM Calvin Johnson
wrote:
>
> Extract phy_id from compatible string. This will be used by
> fwnode_mdiobus_register_phy() to create phy device using the
> phy_id.
> +int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
> +{
> + const char *cp;
> + unsigned int upper, lower;
> + int ret;
> +
> + ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> + if (!ret) {
if (ret)
return ret;
will help a lot with readability of this.
> + if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> + &upper, &lower) == 2) {
> + *phy_id = ((upper & 0x) << 16) | (lower & 0x);
How upper can be bigger than 0xfff? Same for lower.
> + return 0;
> + }
> + }
> + return -EINVAL;
> +}
--
With Best Regards,
Andy Shevchenko
[net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
Extract phy_id from compatible string. This will be used by
fwnode_mdiobus_register_phy() to create phy device using the
phy_id.
Signed-off-by: Calvin Johnson
---
Changes in v3: None
Changes in v2: None
drivers/net/phy/phy_device.c | 21 +
include/linux/phy.h | 5 +
2 files changed, 26 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 4204d49586cd..f4ad47f73f8d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -662,6 +662,27 @@ struct phy_device *phy_device_create(struct mii_bus *bus,
int addr, u32 phy_id,
}
EXPORT_SYMBOL(phy_device_create);
+/* Extract the phy ID from the compatible string of the form
+ * ethernet-phy-id..
+ */
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+ const char *cp;
+ unsigned int upper, lower;
+ int ret;
+
+ ret = fwnode_property_read_string(fwnode, "compatible", &cp);
+ if (!ret) {
+ if (sscanf(cp, "ethernet-phy-id%4x.%4x",
+ &upper, &lower) == 2) {
+ *phy_id = ((upper & 0x) << 16) | (lower & 0x);
+ return 0;
+ }
+ }
+ return -EINVAL;
+}
+EXPORT_SYMBOL(fwnode_get_phy_id);
+
/* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers.
* @bus: the target MII bus
* @addr: PHY address on the MII bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f2664730a331..d78ae56509e1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1141,6 +1141,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus,
int addr, u32 phy_id,
bool is_c45,
struct phy_c45_device_ids *c45_ids);
#if IS_ENABLED(CONFIG_PHYLIB)
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
struct phy_device *device_phy_find_device(struct device *dev);
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
@@ -1148,6 +1149,10 @@ struct phy_device *get_phy_device(struct mii_bus *bus,
int addr, bool is_c45);
int phy_device_register(struct phy_device *phy);
void phy_device_free(struct phy_device *phydev);
#else
+static inline int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+ return 0;
+}
static inline
struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
{
--
2.17.1
