linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* RE: [EXT] Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers
       [not found]   ` <20200131162814.GB17185@lunn.ch>
@ 2020-02-05  7:11     ` Calvin Johnson (OSS)
  0 siblings, 0 replies; 9+ messages in thread
From: Calvin Johnson (OSS) @ 2020-02-05  7:11 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: linux.cj, Jon Nettleton, linux, Makarand Pawagi, Cristi Sovaiala,
	Laurentiu Tudor, Ioana Ciornei, Varun Sethi, Pankaj Bansal,
	Rajesh V. Bikkina, Marcin Wojtas, Calvin Johnson (OSS),
	David S. Miller, Florian Fainelli, Heiner Kallweit, linux-kernel,
	netdev, linux-kernel, linux-acpi

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, January 31, 2020 9:58 PM

<snip>

> On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote:
> > From: Marcin Wojtas <mw@semihalf.com>
> >
> > This patch introduces fwnode helper for registering MDIO bus, as well
> > as one for finding the PHY, basing on its firmware node pointer.
> > Comparing to existing OF equivalent,
> > fwnode_mdiobus_register() does not support:
> >  * deprecated bindings (device whitelist, nor the PHY ID embedded
> >    in the compatible string)
> >  * MDIO bus auto scanning
> >
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> 
> Hi Calvin
> no
> This appears to but a cut and paste, follow by an intelligent s/of/fwnode/g.

In this patchset, I tried to reuse Marcin's patch which was posted on 2017/12/18.
https://lkml.org/lkml/2017/12/18/211
With my patch([v1,2/7] mdio_bus: modify fwnode phy related functions), I've made 
modifications to this(v1,1/7) patch to adapt to the changes in the kernel.

> Did you make any attempt to consolidate the two implementations?  It
> seems like there should be some level of abstraction that hides away the
> difference between DT properties, and DT properties stuffed into ACPI
> tables?

Yes attempt is to consolidate DT and ACPI into fwnode. Sure, I'll revisit the patch
and try to work on your recommendation.

Thanks
Calvin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [EXT] Re: [PATCH v1 0/7] ACPI support for xgmac_mdio and dpaa2-mac drivers.
       [not found] ` <fcda49b6-7a45-cd86-e33e-f8dea07c0684@gmail.com>
@ 2020-02-05  8:31   ` Calvin Johnson (OSS)
  0 siblings, 0 replies; 9+ messages in thread
From: Calvin Johnson (OSS) @ 2020-02-05  8:31 UTC (permalink / raw)
  To: Florian Fainelli, linux.cj, Jon Nettleton, linux,
	Makarand Pawagi, Cristi Sovaiala, Laurentiu Tudor, Ioana Ciornei,
	Varun Sethi, Pankaj Bansal, Rajesh V. Bikkina, Jeremy Linton
  Cc: Calvin Johnson (OSS),
	Andrew Lunn, Andy Shevchenko, Antoine Tenart, David S. Miller,
	Dmitry Torokhov, Greg Kroah-Hartman, Heikki Krogerus,
	Heiner Kallweit, Ioana Ciocoi Radulescu, Madalin Bucur (OSS),
	Matteo Croce, Maxime Chevallier, Rafael J. Wysocki, Russell King,
	Sakari Ailus, Thomas Gleixner, linux-kernel, netdev, linux-acpi

Hi Florian

> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: Monday, February 3, 2020 11:32 PM
> To: Calvin Johnson <calvin.johnson@nxp.com>; linux.cj@gmail.com; Jon

<snip>

> On 1/31/20 7:34 AM, Calvin Johnson wrote:
> > From: Calvin Johnson <calvin.johnson@oss.nxp.com>
> >
> > This patch series provides ACPI support for xgmac_mdio and dpaa2-mac
> > driver. Most of the DT APIs are replaced with fwnode APIs to handle
> > both DT and ACPI nodes.
> >
> > Old patch by Marcin Wojtas: (mdio_bus: Introduce fwnode MDIO helpers),
> > is reused in this series to get some fwnode mdio helper APIs.
> 
> Andrew's comment on your first patch is a good summary of what this patch
> series does, instead of consolidating the existing code and making it less of_*
> centric and more firmware agnostic, this duplicates the existing infrastructure
> almost line for line to create a fwnode specific implementation. The
> preference would be for you to move away from that and use device_*
> properties as much as possible while making the code capable of handling all
> firmware implementations.

Thanks for the suggestion. Will take this direction for v2.

> Can you also show a few DSDT for the devices that you are working so we can
> a feeling of how you represented the various properties and parent/child
> devices dependencies?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [EXT] Re: [PATCH v1 6/7] net: phylink: Introduce phylink_fwnode_phy_connect()
       [not found]   ` <20200203184121.GR25745@shell.armlinux.org.uk>
@ 2020-02-05 11:33     ` Calvin Johnson (OSS)
  0 siblings, 0 replies; 9+ messages in thread
From: Calvin Johnson (OSS) @ 2020-02-05 11:33 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: linux.cj, Jon Nettleton, Makarand Pawagi, Cristi Sovaiala,
	Laurentiu Tudor, Ioana Ciornei, Varun Sethi, Pankaj Bansal,
	Rajesh V. Bikkina, Calvin Johnson (OSS),
	Andrew Lunn, David S. Miller, Florian Fainelli, Heiner Kallweit,
	linux-kernel, netdev, linux-acpi

> -----Original Message-----
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Sent: Tuesday, February 4, 2020 12:11 AM
> To: Calvin Johnson <calvin.johnson@nxp.com>

<snip>

> > Introduce phylink_fwnode_phy_connect API to connect the PHY using
> > fwnode.
> >
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
> >
> >  drivers/net/phy/phylink.c | 64
> +++++++++++++++++++++++++++++++++++++++
> >  include/linux/phylink.h   |  2 ++
> >  2 files changed, 66 insertions(+)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index ee7a718662c6..f211f62283b5 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/timer.h>
> >  #include <linux/workqueue.h>
> > +#include <linux/acpi.h>
> >
> >  #include "sfp.h"
> >  #include "swphy.h"
> > @@ -817,6 +818,69 @@ int phylink_connect_phy(struct phylink *pl,
> > struct phy_device *phy)  }  EXPORT_SYMBOL_GPL(phylink_connect_phy);
> >
> > +/**
> > + * phylink_fwnode_phy_connect() - connect the PHY specified in the
> fwnode.
> > + * @pl: a pointer to a &struct phylink returned from phylink_create()
> > + * @dn: a pointer to a &struct device_node.
> > + * @flags: PHY-specific flags to communicate to the PHY device driver
> > + *
> > + * Connect the phy specified in the device node @dn to the phylink
> > +instance
> > + * specified by @pl. Actions specified in phylink_connect_phy() will
> > +be
> > + * performed.
> > + *
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int phylink_fwnode_phy_connect(struct phylink *pl,
> > +                            struct fwnode_handle *fwnode,
> > +                            u32 flags) {
> > +     struct fwnode_handle *phy_node;
> > +     struct phy_device *phy_dev;
> > +     int ret;
> > +     int status;
> > +     struct fwnode_reference_args args;
> > +
> > +     /* Fixed links and 802.3z are handled without needing a PHY */
> > +     if (pl->link_an_mode == MLO_AN_FIXED ||
> > +         (pl->link_an_mode == MLO_AN_INBAND &&
> > +          phy_interface_mode_is_8023z(pl->link_interface)))
> > +             return 0;
> > +
> > +     status = acpi_node_get_property_reference(fwnode, "phy-handle", 0,
> > +                                               &args);
> > +     if (ACPI_FAILURE(status) || !is_acpi_device_node(args.fwnode))
> > +             status = acpi_node_get_property_reference(fwnode, "phy", 0,
> > +                                                       &args);
> > +     if (ACPI_FAILURE(status) || !is_acpi_device_node(args.fwnode))
> > +             status = acpi_node_get_property_reference(fwnode,
> > +                                                       "phy-device", 0,
> > +                                                       &args);
> 
> This is a copy-and-paste of phylink_of_phy_connect() without much thought.
> 
> There is no need to duplicate the legacy DT functionality of phy/phy-
> device/phy-handle in ACPI - there is no legacy to support, so it's pointless
> trying to find one of three properties here.

Ok. I'll remove it.

> I'd prefer both the DT and ACPI variants to be more integrated, so we don't
> have two almost identical functions except for the firmware specific detail.

Did you mean phylink_of_phy_connect replaced with phylink_fwnode_phy_connect?
I can add DT handling also inside phylink_fwnode_phy_connect. Please let me know.

Thanks for pointing out about adding linux-acpi ML. I started added them in my responses.
I was assuming they would be added by get_maintainer.pl. 

Thanks
Calvin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [EXT] Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers
       [not found]   ` <371ff9b4-4de6-7a03-90f8-a1eae4d5402d@arm.com>
@ 2020-02-07  9:42     ` Calvin Johnson (OSS)
  2020-02-25 10:12       ` Calvin Johnson
  0 siblings, 1 reply; 9+ messages in thread
From: Calvin Johnson (OSS) @ 2020-02-07  9:42 UTC (permalink / raw)
  To: Jeremy Linton, linux.cj, Jon Nettleton, linux, Makarand Pawagi,
	Cristi Sovaiala, Laurentiu Tudor, Ioana Ciornei, Varun Sethi,
	Pankaj Bansal, Rajesh V. Bikkina, Leo Li
  Cc: Marcin Wojtas, Calvin Johnson (OSS),
	Andrew Lunn, David S. Miller, Florian Fainelli, Heiner Kallweit,
	linux-kernel, netdev, linux-acpi

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [EXT] Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers
  2020-02-07  9:42     ` Calvin Johnson (OSS)
@ 2020-02-25 10:12       ` Calvin Johnson
  2020-02-25 20:42         ` Jeremy Linton
  0 siblings, 1 reply; 9+ messages in thread
From: Calvin Johnson @ 2020-02-25 10:12 UTC (permalink / raw)
  To: Jeremy Linton, linux.cj, Jon Nettleton, linux, Makarand Pawagi,
	Cristi Sovaiala, Laurentiu Tudor, Ioana Ciornei, Varun Sethi,
	Pankaj Bansal, Rajesh V. Bikkina, Leo Li
  Cc: linux-acpi

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [EXT] Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers
  2020-02-25 10:12       ` Calvin Johnson
@ 2020-02-25 20:42         ` Jeremy Linton
  0 siblings, 0 replies; 9+ messages in thread
From: Jeremy Linton @ 2020-02-25 20:42 UTC (permalink / raw)
  To: Calvin Johnson, linux.cj, Jon Nettleton, linux, Makarand Pawagi,
	Cristi Sovaiala, Laurentiu Tudor, Ioana Ciornei, Varun Sethi,
	Pankaj Bansal, Rajesh V. Bikkina, Leo Li
  Cc: linux-acpi, Al Stone

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
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers
       [not found] ` <20200131153440.20870-2-calvin.johnson@nxp.com>
       [not found]   ` <20200131162814.GB17185@lunn.ch>
       [not found]   ` <371ff9b4-4de6-7a03-90f8-a1eae4d5402d@arm.com>
@ 2020-03-17 11:36   ` Calvin Johnson
  2020-03-17 14:04     ` Andrew Lunn
  2 siblings, 1 reply; 9+ messages in thread
From: Calvin Johnson @ 2020-03-17 11:36 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: linux.cj, Jon Nettleton, linux, Makarand Pawagi,
	cristian.sovaiala, laurentiu.tudor, ioana.ciornei, V.Sethi,
	pankaj.bansal, Rajesh V . Bikkina, Marcin Wojtas, Andrew Lunn,
	David S. Miller, Florian Fainelli, Heiner Kallweit, linux-kernel,
	netdev, linux-acpi

Hi,

On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote:

<snip>

> +/**
> + * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
> + * It must either:
> + * o Compatible string of "ethernet-phy-ieee802.3-c45"
> + * o Compatible string of "ethernet-phy-ieee802.3-c22"
> + * Checking "compatible" property is done, in order to follow the DT binding.
> + */
> +static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
> +{
> +	int ret;
> +
> +	ret = fwnode_property_match_string(child, "compatible",
> +					   "ethernet-phy-ieee802.3-c45");
> +	if (!ret)
> +		return true;
> +
> +	ret = fwnode_property_match_string(child, "compatible",
> +					   "ethernet-phy-ieee802.3-c22");
> +	if (!ret)
> +		return true;
> +
> +	if (!fwnode_property_present(child, "compatible"))
> +		return true;
> +
> +	return false;
> +}

Can we use _CID in ACPI to get the compatible string? Is there any other method
to handle this kind of situation where we would like to pass C45 or C22 info to
the mdiobus driver?

Thanks
Calvin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers
  2020-03-17 11:36   ` Calvin Johnson
@ 2020-03-17 14:04     ` Andrew Lunn
  2020-03-18  6:03       ` Calvin Johnson
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Lunn @ 2020-03-17 14:04 UTC (permalink / raw)
  To: Calvin Johnson
  Cc: Jeremy Linton, linux.cj, Jon Nettleton, linux, Makarand Pawagi,
	cristian.sovaiala, laurentiu.tudor, ioana.ciornei, V.Sethi,
	pankaj.bansal, Rajesh V . Bikkina, Marcin Wojtas,
	David S. Miller, Florian Fainelli, Heiner Kallweit, linux-kernel,
	netdev, linux-acpi

On Tue, Mar 17, 2020 at 05:06:50PM +0530, Calvin Johnson wrote:
> Hi,
> 
> On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote:
> 
> <snip>
> 
> > +/**
> > + * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
> > + * It must either:
> > + * o Compatible string of "ethernet-phy-ieee802.3-c45"
> > + * o Compatible string of "ethernet-phy-ieee802.3-c22"
> > + * Checking "compatible" property is done, in order to follow the DT binding.
> > + */
> > +static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
> > +{
> > +	int ret;
> > +
> > +	ret = fwnode_property_match_string(child, "compatible",
> > +					   "ethernet-phy-ieee802.3-c45");
> > +	if (!ret)
> > +		return true;
> > +
> > +	ret = fwnode_property_match_string(child, "compatible",
> > +					   "ethernet-phy-ieee802.3-c22");
> > +	if (!ret)
> > +		return true;
> > +
> > +	if (!fwnode_property_present(child, "compatible"))
> > +		return true;
> > +
> > +	return false;
> > +}
> 
> Can we use _CID in ACPI to get the compatible string? Is there any other method
> to handle this kind of situation where we would like to pass C45 or C22 info to
> the mdiobus driver?

Hi Calvin

Is there any defacto standardised way to stuff this device tree
property into ACPI? It is one of the key properties, so either there
is one standard way, or lots of variants because nobody can be
bothered to go to the ACPI standardisation body and get it formalised.

     Andrew

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers
  2020-03-17 14:04     ` Andrew Lunn
@ 2020-03-18  6:03       ` Calvin Johnson
  0 siblings, 0 replies; 9+ messages in thread
From: Calvin Johnson @ 2020-03-18  6:03 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Jeremy Linton, linux.cj, Jon Nettleton, linux, Makarand Pawagi,
	cristian.sovaiala, laurentiu.tudor, ioana.ciornei, V.Sethi,
	pankaj.bansal, Rajesh V . Bikkina, Marcin Wojtas,
	David S. Miller, Florian Fainelli, Heiner Kallweit, linux-kernel,
	netdev, linux-acpi

Hi Andrew,

On Tue, Mar 17, 2020 at 03:04:26PM +0100, Andrew Lunn wrote:
> On Tue, Mar 17, 2020 at 05:06:50PM +0530, Calvin Johnson wrote:
> > Hi,
> > 
> > On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote:
> > 
> > <snip>
> > 
> > > +/**
> > > + * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
> > > + * It must either:
> > > + * o Compatible string of "ethernet-phy-ieee802.3-c45"
> > > + * o Compatible string of "ethernet-phy-ieee802.3-c22"
> > > + * Checking "compatible" property is done, in order to follow the DT binding.
> > > + */
> > > +static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = fwnode_property_match_string(child, "compatible",
> > > +					   "ethernet-phy-ieee802.3-c45");
> > > +	if (!ret)
> > > +		return true;
> > > +
> > > +	ret = fwnode_property_match_string(child, "compatible",
> > > +					   "ethernet-phy-ieee802.3-c22");
> > > +	if (!ret)
> > > +		return true;
> > > +
> > > +	if (!fwnode_property_present(child, "compatible"))
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > 
> > Can we use _CID in ACPI to get the compatible string? Is there any other method
> > to handle this kind of situation where we would like to pass C45 or C22 info to
> > the mdiobus driver?
> 
> Hi Calvin
> 
> Is there any defacto standardised way to stuff this device tree
> property into ACPI? It is one of the key properties, so either there
> is one standard way, or lots of variants because nobody can be
> bothered to go to the ACPI standardisation body and get it formalised.

_DSD package is used to stuff this kind of DT property. IMO, this is not the
standard way as C22 and C45 are key properties for MDIO.

Eg usage of _DSD:
https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms/tree/Platform/NXP/LX2160aRdbPkg/AcpiTables/Dsdt/Mdio.asl?h=LX2160_UEFI_ACPI_EAR1

      Name (_DSD, Package () {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
          Package () {
            Package () {"reg", 5},
            Package () {"phy-addr", 5},
            Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
        }
      })

Ideally, MDIO bus should be part of the ACPI spec.
Maybe this property can be included in:
https://uefi.org/sites/default/files/resources/nic-request-v2.pdf

I'm still looking for a better approach than _DSD till ACPI spec defines it.

Regards
Calvin

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-03-18  6:04 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [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
2020-03-17 11:36   ` Calvin Johnson
2020-03-17 14:04     ` Andrew Lunn
2020-03-18  6:03       ` Calvin Johnson

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