RE: [EXT] Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

[Calvin Johnson (OSS)]

Hi Jeremy,

> -----Original Message-----
> From: Jeremy Linton <jeremy.linton@xxxxxxx>
> Sent: Wednesday, February 5, 2020 7:48 PM

<snip>

> > +static int fwnode_mdio_parse_addr(struct device *dev,
> > +                               const struct fwnode_handle *fwnode) {
> > +     u32 addr;
> > +     int ret;
> > +
> > +     ret = fwnode_property_read_u32(fwnode, "reg", &addr);
> > +     if (ret < 0) {
> > +             dev_err(dev, "PHY node has no 'reg' property\n");
> > +             return ret;
> > +     }
> > +
> > +     /* A PHY must have a reg property in the range [0-31] */
> > +     if (addr < 0 || addr >= PHY_MAX_ADDR) {
> > +             dev_err(dev, "PHY address %i is invalid\n", addr);
> > +             return -EINVAL;
> > +     }
> > +
> > +     return addr;
> > +}
> 
> Almost assuredly this is wrong, the _ADR method exists to identify a device
> on its parent bus. The DT reg property shouldn't be used like this in an ACPI
> enviroment.
> 
> Further, there are a number of other dt bindings in this set that seem
> inappropriate in common/shared ACPI code paths. That is because AFAIK the
> _DSD methods are there to provide device implementation specific
> behaviors, not as standardized methods for a generic classes of devices.
> Its vaguly the equivlant of the "vendor,xxxx" properties in DT.
> 
> This has been a discussion point on/off for a while with any attempt to
> publicly specify/standardize for all OS vendors what they might find encoded
> in a DSD property. The few year old "WORK_IN_PROGRESS" link on the uefi
> page has a few suggested ones
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.
> org%2Fsites%2Fdefault%2Ffiles%2Fresources%2Fnic-request-
> v2.pdf&amp;data=02%7C01%7Ccalvin.johnson%40nxp.com%7Cf16350b8314
> b4992063008d7ab4f6486%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1
> %7C637166229795374486&amp;sdata=zcXu%2Fu%2Fxw5%2Ff7eJd%2FledR%
> 2FgnabvFcCUtOfwTXtMoDBI%3D&amp;reserved=0
> 
> Anyway, the use of phy-handle with a reference to the phy device on a
> shared MDIO is a techically workable solution to the problem brought up in
> the RPI/ACPI thread as well. OTOH, it suffers from the use of DSD and at a
> minimum should probably be added to the document linked above if its
> found to be the best way to handle this. Although, in the common case of a
> mdio bus, matching acpi described devices with their discovered
> counterparts (note the device() defintion in the spec
> 19.6.30) only to define DSD refrences is a bit overkill.
> 
> Put another way, while seemingly not nessiary if a bus can be probed, a
> nic/device->mdio->phy can be described in the normal ACPI heirarchy with
> only appropriatly nested CRS and ADR resources. Its the shared nature of the
> MDIO bus that causes problems.

Thanks! I'll definitely consider your suggestions along with the others received earlier.

While I do more study on this, thought of forwarding DSTD tables used by this patch-set.
https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms/tree/Platform/NXP/LX2160aRdbPkg/AcpiTables/Dsdt/Mdio.asl?h=LX2160_UEFI_ACPI_EAR1
https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms/tree/Platform/NXP/LX2160aRdbPkg/AcpiTables/Dsdt/Mc.asl?h=LX2160_UEFI_ACPI_EAR1

Regards
Calvin