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 <[email protected]>
---
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-idAAAA.BBBB.
+ */
+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 & 0xFFFF) << 16) | (lower & 0xFFFF);
+ 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
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 & 0xFFFF) << 16) | (lower & 0xFFFF);
>
> 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
On Tue, May 5, 2020 at 4:29 PM Calvin Johnson
<[email protected]> 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 & 0xFFFF) << 16) | (lower & 0xFFFF);
How upper can be bigger than 0xfff? Same for lower.
> + return 0;
> + }
> + }
> + return -EINVAL;
> +}
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> ---
>
> 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-idAAAA.BBBB.
> + */
> +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 & 0xFFFF) << 16) | (lower & 0xFFFF);
> + 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,
On Thu, May 7, 2020 at 4:26 PM Jeremy Linton <[email protected]> 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 & 0xFFFF) << 16) | (lower & 0xFFFF);
> > + 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
Hi,
On 5/7/20 12:27 PM, Andy Shevchenko wrote:
> On Thu, May 7, 2020 at 4:26 PM Jeremy Linton <[email protected]> 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 & 0xFFFF) << 16) | (lower & 0xFFFF);
>>> + 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_vvvv&DEV_dddd"
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_IDvvvv&ID_DDDD" 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.
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 <[email protected]> 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 & 0xFFFF) << 16) | (lower & 0xFFFF);
> > > > + 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_vvvv&DEV_dddd"
>
> 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_IDvvvv&ID_DDDD" 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_IDvvvv&ID_DDDD" 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
> > It does have a numeric version defined for EISA types. OTOH I suspect that
> > your right. If there were a "PHY\VEN_IDvvvv&ID_DDDD" 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
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_IDvvvv&ID_DDDD" 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,
> > 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-idXXXX.YYYY 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
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-idXXXX.YYYY 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".
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
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! :)
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_IDvvvv&ID_DDDD" 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.
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
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
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
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
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
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
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_IDvvvv&ID_DDDD" 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
> 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
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
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