From: Jeremy Linton <jeremy.linton@arm.com>
To: Calvin Johnson <calvin.johnson@oss.nxp.com>,
"linux.cj@gmail.com" <linux.cj@gmail.com>,
Jon Nettleton <jon@solid-run.com>,
"linux@armlinux.org.uk" <linux@armlinux.org.uk>,
Makarand Pawagi <makarand.pawagi@nxp.com>,
Cristi Sovaiala <cristian.sovaiala@nxp.com>,
Laurentiu Tudor <laurentiu.tudor@nxp.com>,
Ioana Ciornei <ioana.ciornei@nxp.com>,
Varun Sethi <V.Sethi@nxp.com>,
Pankaj Bansal <pankaj.bansal@nxp.com>,
"Rajesh V. Bikkina" <rajesh.bikkina@nxp.com>,
Leo Li <leoyang.li@nxp.com>
Cc: linux-acpi@vger.kernel.org, Al Stone <ahs3@redhat.com>
Subject: Re: [EXT] Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers
Date: Tue, 25 Feb 2020 14:42:07 -0600 [thread overview]
Message-ID: <1e97119b-1073-33cc-2d53-622b7c17070a@arm.com> (raw)
In-Reply-To: <20200225101240.GA8966@lsv03152.swis.in-blr01.nxp.com>
Hi,
On 2/25/20 4:12 AM, Calvin Johnson wrote:
> 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
>>> environment.
>
> 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?
It appears most of the callers are using acpi_evaluate_integer().
> 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.
Its really only useful for "buses with standard enumeration functions",
so you wouldn't see it being used in device drivers, only bus drivers.
As such, mfd_acpi_add_device() is an example that lives outside the
drivers/acpi directory although I think the pci bits in the drivers/acpi
might be a better example.
>
>>>
>>> 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 vaguely the equivalent 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?
+Al Stone, who looked at this a while back, may be able to shed more
light. But again, standardization of Device Specific Data (DSD)
properties is a bit odd.
>
>>>
>>> Anyway, the use of phy-handle with a reference to the phy device on a
>>> shared MDIO is a technically 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() definition in the spec
>>> 19.6.30) only to define DSD references is a bit overkill.
>>>
>>> Put another way, while seemingly not necessary if a bus can be probed, a
>>> nic/device->mdio->phy can be described in the normal ACPI hierarchy with
>>> only appropriately 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
>
next prev parent reply other threads:[~2020-02-25 20:42 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200131153440.20870-1-calvin.johnson@nxp.com>
[not found] ` <fcda49b6-7a45-cd86-e33e-f8dea07c0684@gmail.com>
2020-02-05 8:31 ` [EXT] Re: [PATCH v1 0/7] ACPI support for xgmac_mdio and dpaa2-mac drivers Calvin Johnson (OSS)
[not found] ` <20200131153440.20870-7-calvin.johnson@nxp.com>
[not found] ` <20200203184121.GR25745@shell.armlinux.org.uk>
2020-02-05 11:33 ` [EXT] Re: [PATCH v1 6/7] net: phylink: Introduce phylink_fwnode_phy_connect() Calvin Johnson (OSS)
[not found] ` <20200131153440.20870-2-calvin.johnson@nxp.com>
[not found] ` <20200131162814.GB17185@lunn.ch>
2020-02-05 7:11 ` [EXT] Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers Calvin Johnson (OSS)
[not found] ` <371ff9b4-4de6-7a03-90f8-a1eae4d5402d@arm.com>
2020-02-07 9:42 ` Calvin Johnson (OSS)
2020-02-25 10:12 ` Calvin Johnson
2020-02-25 20:42 ` Jeremy Linton [this message]
2020-03-17 11:36 ` Calvin Johnson
2020-03-17 14:04 ` Andrew Lunn
2020-03-18 6:03 ` Calvin Johnson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1e97119b-1073-33cc-2d53-622b7c17070a@arm.com \
--to=jeremy.linton@arm.com \
--cc=V.Sethi@nxp.com \
--cc=ahs3@redhat.com \
--cc=calvin.johnson@oss.nxp.com \
--cc=cristian.sovaiala@nxp.com \
--cc=ioana.ciornei@nxp.com \
--cc=jon@solid-run.com \
--cc=laurentiu.tudor@nxp.com \
--cc=leoyang.li@nxp.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux.cj@gmail.com \
--cc=linux@armlinux.org.uk \
--cc=makarand.pawagi@nxp.com \
--cc=pankaj.bansal@nxp.com \
--cc=rajesh.bikkina@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).