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

[Calvin Johnson <calvin.johnson@oss.nxp.com>]

On Fri, Feb 07, 2020 at 09:42:56AM +0000, Calvin Johnson (OSS) wrote:
Hi Jeremy,

 
> > -----Original Message-----
> > From: Jeremy Linton <jeremy.linton@arm.com>
> > 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.

In the ACPI environment, can we use _ADR to get the PHY device address
from the DSDT? Is there any function to get this value?
acpi_ut_evaluate_numeric_object is called from inside drivers/acpi/acpica.
However, I don't see any other driver outside drivers/acpi using _ADR to get
the address.

> > 
> > 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://uefi.org/sites/default/files/resources/nic-request-v2.pdf

Having this as part of spec would be helpful.
Do you know who can help get this nic-request integrated into spec?

> > 
> > 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.

Were you able to take a look at the shared DSDT tables? Is this the ACPI hierarchy
with nested CRS and ADR resources which you mentioned above?

> 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

Thanks
Calvin