[net-next PATCH v1 0/7] ACPI support for dpaa2 driver

[net-next PATCH v1 0/7] ACPI support for dpaa2 driver

[Calvin Johnson]

This patch set provides ACPI support to DPAA2 network drivers.

It also introduces new fwnode based APIs to support phylink and phy layers
Following functions are defined:
  phylink_fwnode_phy_connect()
  fwnode_mdiobus_register_phy()
  fwnode_get_phy_id()
  fwnode_phy_find_device()
  device_phy_find_device()
  fwnode_get_phy_node()

First one helps in connecting phy to phylink instance.
Next two helps in getting phy_id and registering phy to mdiobus
Next two help in finding a phy on a mdiobus.
Next one helps in getting phy_node from a fwnode.


Calvin Johnson (7):
  Documentation: ACPI: DSD: Document MDIO PHY
  net: phy: Introduce phy related fwnode functions
  net: phy: Introduce fwnode_get_phy_id()
  net: mdiobus: Introduce fwnode_mdiobus_register_phy()
  phylink: introduce phylink_fwnode_phy_connect()
  net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
  net/fsl: Use _ADR ACPI object to register PHYs

 Documentation/firmware-guide/acpi/dsd/phy.rst | 78 ++++++++++++++++++
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 79 +++++++++++-------
 drivers/net/ethernet/freescale/xgmac_mdio.c   | 48 ++++++++++-
 drivers/net/phy/mdio_bus.c                    | 40 +++++++++
 drivers/net/phy/phy_device.c                  | 82 +++++++++++++++++++
 drivers/net/phy/phylink.c                     | 51 ++++++++++++
 include/linux/mdio.h                          |  2 +
 include/linux/phy.h                           | 25 ++++++
 include/linux/phylink.h                       |  3 +
 9 files changed, 375 insertions(+), 33 deletions(-)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst

-- 
2.17.1

[net-next PATCH v1 1/7] Documentation: ACPI: DSD: Document MDIO PHY

[Calvin Johnson]

Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
provide them to be connected to MAC.

Describe properties "phy-handle" and "phy-mode".

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

 Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst

diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
new file mode 100644
index 000000000000..f10feb24ec1c
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
@@ -0,0 +1,78 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+MDIO bus and PHYs in ACPI
+=========================
+
+The PHYs on an mdiobus are probed and registered using
+fwnode_mdiobus_register_phy().
+Later, for connecting these PHYs to MAC, the PHYs registered on the
+mdiobus have to be referenced.
+
+phy-handle
+-----------
+For each MAC node, a property "phy-handle" is used to reference the
+PHY that is registered on an MDIO bus.
+
+phy-mode
+--------
+Property "phy-mode" defines the type of PHY interface.
+
+An example of this is shown below::
+
+DSDT entry for MACs where PHY nodes are referenced
+--------------------------------------------------
+	Scope(\_SB.MCE0.PR17) // 1G
+	{
+	  Name (_DSD, Package () {
+	     ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package (2) {"phy-mode", "rgmii-id"},
+		     Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
+	      }
+	   })
+	}
+
+	Scope(\_SB.MCE0.PR18) // 1G
+	{
+	  Name (_DSD, Package () {
+	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		Package () {
+		    Package (2) {"phy-mode", "rgmii-id"},
+		    Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}}
+	    }
+	  })
+	}
+
+DSDT entry for MDIO node
+------------------------
+a) Silicon Component
+--------------------
+	Scope(_SB)
+	{
+	  Device(MDI0) {
+	    Name(_HID, "NXP0006")
+	    Name(_CCA, 1)
+	    Name(_UID, 0)
+	    Name(_CRS, ResourceTemplate() {
+	      Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
+	      Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
+	       {
+		 MDI0_IT
+	       }
+	    }) // end of _CRS for MDI0
+	  } // end of MDI0
+	}
+
+b) Platform Component
+---------------------
+	Scope(\_SB.MDI0)
+	{
+	  Device(PHY1) {
+	    Name (_ADR, 0x1)
+	  } // end of PHY1
+
+	  Device(PHY2) {
+	    Name (_ADR, 0x2)
+	  } // end of PHY2
+	}
-- 
2.17.1

Re: [net-next PATCH v1 1/7] Documentation: ACPI: DSD: Document MDIO PHY

[Rafael J. Wysocki]

On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> provide them to be connected to MAC.
>
> Describe properties "phy-handle" and "phy-mode".
>
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
>
>  Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
>
> diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
> new file mode 100644
> index 000000000000..f10feb24ec1c
> --- /dev/null
> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
> @@ -0,0 +1,78 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================
> +MDIO bus and PHYs in ACPI
> +=========================
> +
> +The PHYs on an mdiobus are probed and registered using
> +fwnode_mdiobus_register_phy().
> +Later, for connecting these PHYs to MAC, the PHYs registered on the
> +mdiobus have to be referenced.
> +
> +phy-handle
> +-----------
> +For each MAC node, a property "phy-handle" is used to reference the
> +PHY that is registered on an MDIO bus.

It is not clear what "a property" means in this context.

This should refer to the documents introducing the _DSD-based generic
device properties rules, including the GUID used below.

You need to say whether or not the property is mandatory and if it
isn't mandatory, you need to say what the lack of it means.

> +
> +phy-mode
> +--------
> +Property "phy-mode" defines the type of PHY interface.

This needs to be more detailed too, IMO.  At the very least, please
list all of the possible values of it and document their meaning.

> +
> +An example of this is shown below::
> +
> +DSDT entry for MACs where PHY nodes are referenced
> +--------------------------------------------------
> +       Scope(\_SB.MCE0.PR17) // 1G
> +       {
> +         Name (_DSD, Package () {
> +            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> +                Package () {
> +                    Package (2) {"phy-mode", "rgmii-id"},
> +                    Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}

What is "phy-handle"?

You haven't introduced it above.

> +             }
> +          })
> +       }
> +
> +       Scope(\_SB.MCE0.PR18) // 1G
> +       {
> +         Name (_DSD, Package () {
> +           ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> +               Package () {
> +                   Package (2) {"phy-mode", "rgmii-id"},
> +                   Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}}
> +           }
> +         })
> +       }
> +
> +DSDT entry for MDIO node
> +------------------------
> +a) Silicon Component

What is this device, exactly?

> +--------------------
> +       Scope(_SB)
> +       {
> +         Device(MDI0) {
> +           Name(_HID, "NXP0006")
> +           Name(_CCA, 1)
> +           Name(_UID, 0)
> +           Name(_CRS, ResourceTemplate() {
> +             Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
> +             Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
> +              {
> +                MDI0_IT
> +              }
> +           }) // end of _CRS for MDI0
> +         } // end of MDI0
> +       }
> +
> +b) Platform Component
> +---------------------
> +       Scope(\_SB.MDI0)
> +       {
> +         Device(PHY1) {
> +           Name (_ADR, 0x1)
> +         } // end of PHY1
> +
> +         Device(PHY2) {
> +           Name (_ADR, 0x2)
> +         } // end of PHY2
> +       }
> --

What is the connection between the last two pieces of ASL and the _DSD
definitions above?

Re: [net-next PATCH v1 1/7] Documentation: ACPI: DSD: Document MDIO PHY

[Calvin Johnson]

Hi Rafael,

On Wed, Sep 30, 2020 at 06:37:09PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> > provide them to be connected to MAC.
> >
> > Describe properties "phy-handle" and "phy-mode".
> >
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
> >
> >  Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >  create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
> >
> > diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
> > new file mode 100644
> > index 000000000000..f10feb24ec1c
> > --- /dev/null
> > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
> > @@ -0,0 +1,78 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +=========================
> > +MDIO bus and PHYs in ACPI
> > +=========================
> > +
> > +The PHYs on an mdiobus are probed and registered using
> > +fwnode_mdiobus_register_phy().
> > +Later, for connecting these PHYs to MAC, the PHYs registered on the
> > +mdiobus have to be referenced.
> > +
> > +phy-handle
> > +-----------
> > +For each MAC node, a property "phy-handle" is used to reference the
> > +PHY that is registered on an MDIO bus.
> 
> It is not clear what "a property" means in this context.
> 
In rev-2, I'll add more info on this.
During the MDIO bus driver initialization, PHYs on this bus are probed
using the _ADR object as shown below and are registered on the mdio bus.

      Scope(\_SB.MDI0)
      {
        Device(PHY1) {
          Name (_ADR, 0x1)
        } // end of PHY1

        Device(PHY2) {
          Name (_ADR, 0x2)
        } // end of PHY2
      }

Later, during the MAC driver initialization, the registered PHY devices
have to be retrieved from the mdio bus. For this, MAC driver needs reference
to the previously registered PHYs which are provided using reference to the
device as {\_SB.MDI0.PHY1}.

> This should refer to the documents introducing the _DSD-based generic
> device properties rules, including the GUID used below.
> 
Sure. I'll refer "Documentation/firmware-guide/acpi/DSD-properties-rules.rst"

> You need to say whether or not the property is mandatory and if it
> isn't mandatory, you need to say what the lack of it means.
> 
I'll do that.

> > +
> > +phy-mode
> > +--------
> > +Property "phy-mode" defines the type of PHY interface.
> 
> This needs to be more detailed too, IMO.  At the very least, please
> list all of the possible values of it and document their meaning.
> 
> > +
> > +An example of this is shown below::
> > +
> > +DSDT entry for MACs where PHY nodes are referenced
> > +--------------------------------------------------
> > +       Scope(\_SB.MCE0.PR17) // 1G
> > +       {
> > +         Name (_DSD, Package () {
> > +            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > +                Package () {
> > +                    Package (2) {"phy-mode", "rgmii-id"},
> > +                    Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
> 
> What is "phy-handle"?
> 
> You haven't introduced it above.
I thought I introduced it earlier in this document as a property. Ofcourse,
more info needs to be added as you mentioned. Other than that am I missing
something?

I've a correction here.
Based on referring some more documents, I'll be using
	Package (2) {"phy-handle",\_SB.MDI0.PHY1}
instead of
        Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
.

> > +             }
> > +          })
> > +       }
> > +
> > +       Scope(\_SB.MCE0.PR18) // 1G
> > +       {
> > +         Name (_DSD, Package () {
> > +           ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > +               Package () {
> > +                   Package (2) {"phy-mode", "rgmii-id"},
> > +                   Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}}
> > +           }
> > +         })
> > +       }
> > +
> > +DSDT entry for MDIO node
> > +------------------------
> > +a) Silicon Component
> 
> What is this device, exactly?

I'll explain it more clearly.

> 
> > +--------------------
> > +       Scope(_SB)
> > +       {
> > +         Device(MDI0) {
> > +           Name(_HID, "NXP0006")
> > +           Name(_CCA, 1)
> > +           Name(_UID, 0)
> > +           Name(_CRS, ResourceTemplate() {
> > +             Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
> > +             Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
> > +              {
> > +                MDI0_IT
> > +              }
> > +           }) // end of _CRS for MDI0
> > +         } // end of MDI0
> > +       }
> > +
> > +b) Platform Component
> > +---------------------
> > +       Scope(\_SB.MDI0)
> > +       {
> > +         Device(PHY1) {
> > +           Name (_ADR, 0x1)
> > +         } // end of PHY1
> > +
> > +         Device(PHY2) {
> > +           Name (_ADR, 0x2)
> > +         } // end of PHY2
> > +       }
> > --
> 
> What is the connection between the last two pieces of ASL and the _DSD
> definitions above?

In rev-2, I'll explain the relation between these pieces. What I tried to show
is that the MDIO bus has an SoC component(mdio controller) and a platform
component(PHYs on the mdiobus).

In the MAC nodes, the PHYs are referenced and that is done using the "phy-handle"
property.

Thanks
Calvin

Re: [net-next PATCH v1 1/7] Documentation: ACPI: DSD: Document MDIO PHY

[Grant Likely]

On 30/09/2020 17:37, Rafael J. Wysocki wrote:
> On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
>>
>> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
>> provide them to be connected to MAC.
>>
>> Describe properties "phy-handle" and "phy-mode".
>>
>> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
>> ---
>>
>>   Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>>   create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
>>
>> diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
>> new file mode 100644
>> index 000000000000..f10feb24ec1c
>> --- /dev/null
>> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
>> @@ -0,0 +1,78 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=========================
>> +MDIO bus and PHYs in ACPI
>> +=========================
>> +
>> +The PHYs on an mdiobus are probed and registered using
>> +fwnode_mdiobus_register_phy().
>> +Later, for connecting these PHYs to MAC, the PHYs registered on the
>> +mdiobus have to be referenced.
>> +
>> +phy-handle
>> +-----------
>> +For each MAC node, a property "phy-handle" is used to reference the
>> +PHY that is registered on an MDIO bus.
> 
> It is not clear what "a property" means in this context.
> 
> This should refer to the documents introducing the _DSD-based generic
> device properties rules, including the GUID used below.
> 
> You need to say whether or not the property is mandatory and if it
> isn't mandatory, you need to say what the lack of it means.
> 
>> +
>> +phy-mode
>> +--------
>> +Property "phy-mode" defines the type of PHY interface.
> 
> This needs to be more detailed too, IMO.  At the very least, please
> list all of the possible values of it and document their meaning.

If the goal is to align with DT, it would be appropriate to point to 
where those properties are defined for DT rather than to have a separate 
description here. I suggest something along the lines of:

    The "phy-mode" _DSD property is used to describe the connection to
    the PHY. The valid values for "phy-mode" are defined in
    Documentation/devicetree/bindings/ethernet-controller.yaml

> 
>> +
>> +An example of this is shown below::
>> +
>> +DSDT entry for MACs where PHY nodes are referenced
>> +--------------------------------------------------
>> +       Scope(\_SB.MCE0.PR17) // 1G
>> +       {
>> +         Name (_DSD, Package () {
>> +            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> +                Package () {
>> +                    Package (2) {"phy-mode", "rgmii-id"},
>> +                    Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
> 
> What is "phy-handle"?
> 
> You haven't introduced it above.

Can you elaborate? "phy-handle" has a section to itself in this 
document. Agree that it needs to be defined more, but it does read to me 
as having been defined.

> 
>> +             }
>> +          })
>> +       }
>> +
>> +       Scope(\_SB.MCE0.PR18) // 1G
>> +       {
>> +         Name (_DSD, Package () {
>> +           ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> +               Package () {
>> +                   Package (2) {"phy-mode", "rgmii-id"},
>> +                   Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}}
>> +           }
>> +         })
>> +       }
>> +
>> +DSDT entry for MDIO node
>> +------------------------
>> +a) Silicon Component
> 
> What is this device, exactly?
> 
>> +--------------------
>> +       Scope(_SB)
>> +       {
>> +         Device(MDI0) {
>> +           Name(_HID, "NXP0006")
>> +           Name(_CCA, 1)
>> +           Name(_UID, 0)
>> +           Name(_CRS, ResourceTemplate() {
>> +             Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
>> +             Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
>> +              {
>> +                MDI0_IT
>> +              }
>> +           }) // end of _CRS for MDI0
>> +         } // end of MDI0
>> +       }
>> +
>> +b) Platform Component
>> +---------------------
>> +       Scope(\_SB.MDI0)
>> +       {
>> +         Device(PHY1) {
>> +           Name (_ADR, 0x1)
>> +         } // end of PHY1
>> +
>> +         Device(PHY2) {
>> +           Name (_ADR, 0x2)
>> +         } // end of PHY2
>> +       }
>> --
> 
> What is the connection between the last two pieces of ASL and the _DSD
> definitions above?
> 

Re: [net-next PATCH v1 1/7] Documentation: ACPI: DSD: Document MDIO PHY

[Rafael J. Wysocki]

On Fri, Oct 2, 2020 at 1:09 PM Grant Likely <grant.likely@arm.com> wrote:
>
>
>
> On 30/09/2020 17:37, Rafael J. Wysocki wrote:
> > On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:
> >>
> >> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> >> provide them to be connected to MAC.
> >>
> >> Describe properties "phy-handle" and "phy-mode".
> >>
> >> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> >> ---
> >>
> >>   Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
> >>   1 file changed, 78 insertions(+)
> >>   create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
> >>
> >> diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
> >> new file mode 100644
> >> index 000000000000..f10feb24ec1c
> >> --- /dev/null
> >> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
> >> @@ -0,0 +1,78 @@
> >> +.. SPDX-License-Identifier: GPL-2.0
> >> +
> >> +=========================
> >> +MDIO bus and PHYs in ACPI
> >> +=========================
> >> +
> >> +The PHYs on an mdiobus are probed and registered using
> >> +fwnode_mdiobus_register_phy().
> >> +Later, for connecting these PHYs to MAC, the PHYs registered on the
> >> +mdiobus have to be referenced.
> >> +
> >> +phy-handle
> >> +-----------
> >> +For each MAC node, a property "phy-handle" is used to reference the
> >> +PHY that is registered on an MDIO bus.
> >
> > It is not clear what "a property" means in this context.
> >
> > This should refer to the documents introducing the _DSD-based generic
> > device properties rules, including the GUID used below.
> >
> > You need to say whether or not the property is mandatory and if it
> > isn't mandatory, you need to say what the lack of it means.
> >
> >> +
> >> +phy-mode
> >> +--------
> >> +Property "phy-mode" defines the type of PHY interface.
> >
> > This needs to be more detailed too, IMO.  At the very least, please
> > list all of the possible values of it and document their meaning.
>
> If the goal is to align with DT, it would be appropriate to point to
> where those properties are defined for DT rather than to have a separate
> description here. I suggest something along the lines of:
>
>     The "phy-mode" _DSD property is used to describe the connection to
>     the PHY. The valid values for "phy-mode" are defined in
>     Documentation/devicetree/bindings/ethernet-controller.yaml
>
> >
> >> +
> >> +An example of this is shown below::
> >> +
> >> +DSDT entry for MACs where PHY nodes are referenced
> >> +--------------------------------------------------
> >> +       Scope(\_SB.MCE0.PR17) // 1G
> >> +       {
> >> +         Name (_DSD, Package () {
> >> +            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >> +                Package () {
> >> +                    Package (2) {"phy-mode", "rgmii-id"},
> >> +                    Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
> >
> > What is "phy-handle"?
> >
> > You haven't introduced it above.
>
> Can you elaborate? "phy-handle" has a section to itself in this
> document.

Yes, it does.

I overlooked it, sorry.

> Agree that it needs to be defined more, but it does read to me
> as having been defined.

Yup.

Cheers!

[net-next PATCH v1 2/7] net: phy: Introduce phy related fwnode functions

[Calvin Johnson]

Define fwnode_phy_find_device() to iterate an mdiobus and find the
phy device of the provided phy fwnode. Additionally define
device_phy_find_device() to find phy device of provided device.

Define fwnode_get_phy_node() to get phy_node using named reference.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

 drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 20 ++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5dab6be6fc38..c4aec56d0a95 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2818,6 +2818,58 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->ack_interrupt;
 }
 
+/**
+ * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
+ * phy_fwnode.
+ * @phy_fwnode: Pointer to the phy's fwnode.
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ */
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	struct device *d;
+	struct mdio_device *mdiodev;
+
+	if (!phy_fwnode)
+		return NULL;
+
+	d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
+	if (d) {
+		mdiodev = to_mdio_device(d);
+		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+			return to_phy_device(d);
+		put_device(d);
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(fwnode_phy_find_device);
+
+/**
+ * device_phy_find_device - For the given device, get the phy_device
+ * @dev: Pointer to the given device
+ *
+ * Refer return conditions of fwnode_phy_find_device().
+ */
+struct phy_device *device_phy_find_device(struct device *dev)
+{
+	return fwnode_phy_find_device(dev_fwnode(dev));
+}
+EXPORT_SYMBOL_GPL(device_phy_find_device);
+
+/**
+ * fwnode_get_phy_node - Get the phy_node using the named reference.
+ * @fwnode: Pointer to fwnode from which phy_node has to be obtained.
+ *
+ * Refer return conditions of fwnode_find_reference().
+ */
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+	return fwnode_find_reference(fwnode, "phy-handle", 0);
+}
+EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
+
 /**
  * phy_probe - probe and init a PHY device
  * @dev: device to probe and init
diff --git a/include/linux/phy.h b/include/linux/phy.h
index eb3cb1a98b45..7b1bf3d46fd3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1378,10 +1378,30 @@ 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)
+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);
 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
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	return NULL;
+}
+
+static inline struct phy_device *device_phy_find_device(struct device *dev)
+{
+	return NULL;
+}
+
+static inline
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
 static inline
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
-- 
2.17.1

Re: [net-next PATCH v1 2/7] net: phy: Introduce phy related fwnode functions

[David Miller]

From: Calvin Johnson <calvin.johnson@oss.nxp.com>
Date: Wed, 30 Sep 2020 21:34:25 +0530

> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> +	struct device *d;
> +	struct mdio_device *mdiodev;

Please use reverse christmas tree ordering for local variables.

Re: [net-next PATCH v1 2/7] net: phy: Introduce phy related fwnode functions

[Calvin Johnson]

On Wed, Sep 30, 2020 at 03:03:49PM -0700, David Miller wrote:
> From: Calvin Johnson <calvin.johnson@oss.nxp.com>
> Date: Wed, 30 Sep 2020 21:34:25 +0530
> 
> > +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> > +{
> > +	struct device *d;
> > +	struct mdio_device *mdiodev;
> 
> Please use reverse christmas tree ordering for local variables.

Sure. In next rev, I'll make sure all the patches follow this.

Thanks
Calvin

[net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

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 <calvin.johnson@oss.nxp.com>
---

 drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
 include/linux/phy.h          |  5 +++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c4aec56d0a95..162abde6223d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi.h>
 #include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -845,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
 	return 0;
 }
 
+/* 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)
+{
+	unsigned int upper, lower;
+	const char *cp;
+	int ret;
+
+	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
+	if (ret)
+		return 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_device - reads the specified PHY device and returns its @phy_device
  *		    struct
@@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
  */
 struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
 {
-	return fwnode_find_reference(fwnode, "phy-handle", 0);
+	struct fwnode_handle *phy_node;
+
+	phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
+	if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
+		return phy_node;
+	phy_node = fwnode_find_reference(fwnode, "phy", 0);
+	if (IS_ERR(phy_node))
+		phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
+	return phy_node;
 }
 EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7b1bf3d46fd3..b6814e04092f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1378,6 +1378,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);
@@ -1385,6 +1386,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

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Andrew Lunn]

> +/* 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)
> +{
> +	unsigned int upper, lower;
> +	const char *cp;
> +	int ret;
> +
> +	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> +	if (ret)
> +		return 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);

Hi Calvin

Do you really need this? Do you have a board with a broken PHY ID?

>  /**
>   * get_phy_device - reads the specified PHY device and returns its @phy_device
>   *		    struct
> @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
>   */
>  struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
>  {
> -	return fwnode_find_reference(fwnode, "phy-handle", 0);
> +	struct fwnode_handle *phy_node;
> +
> +	phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> +	if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> +		return phy_node;
> +	phy_node = fwnode_find_reference(fwnode, "phy", 0);
> +	if (IS_ERR(phy_node))
> +		phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
> +	return phy_node;

Why do you have three different ways to reference a PHY?

    Andrew

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Russell King - ARM Linux admin]

On Wed, Sep 30, 2020 at 06:34:40PM +0200, Andrew Lunn wrote:
> > @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
> >   */
> >  struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> >  {
> > -	return fwnode_find_reference(fwnode, "phy-handle", 0);
> > +	struct fwnode_handle *phy_node;
> > +
> > +	phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> > +	if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> > +		return phy_node;
> > +	phy_node = fwnode_find_reference(fwnode, "phy", 0);
> > +	if (IS_ERR(phy_node))
> > +		phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
> > +	return phy_node;
> 
> Why do you have three different ways to reference a PHY?

Compatibility with the DT version - note that "phy" and "phy-device"
are only used for non-ACPI fwnodes. This should allow us to convert
drivers where necessary without fear of causing DT regressions.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Andrew Lunn]

On Wed, Sep 30, 2020 at 07:07:25PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Sep 30, 2020 at 06:34:40PM +0200, Andrew Lunn wrote:
> > > @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
> > >   */
> > >  struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> > >  {
> > > -	return fwnode_find_reference(fwnode, "phy-handle", 0);
> > > +	struct fwnode_handle *phy_node;
> > > +
> > > +	phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> > > +	if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> > > +		return phy_node;
> > > +	phy_node = fwnode_find_reference(fwnode, "phy", 0);
> > > +	if (IS_ERR(phy_node))
> > > +		phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
> > > +	return phy_node;
> > 
> > Why do you have three different ways to reference a PHY?
> 
> Compatibility with the DT version - note that "phy" and "phy-device"
> are only used for non-ACPI fwnodes. This should allow us to convert
> drivers where necessary without fear of causing DT regressions.

Ah.

A comment would be good here.

  Andrew

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

On Wed, Sep 30, 2020 at 08:19:02PM +0200, Andrew Lunn wrote:
> On Wed, Sep 30, 2020 at 07:07:25PM +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Sep 30, 2020 at 06:34:40PM +0200, Andrew Lunn wrote:
> > > > @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
> > > >   */
> > > >  struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> > > >  {
> > > > -	return fwnode_find_reference(fwnode, "phy-handle", 0);
> > > > +	struct fwnode_handle *phy_node;
> > > > +
> > > > +	phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> > > > +	if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> > > > +		return phy_node;
> > > > +	phy_node = fwnode_find_reference(fwnode, "phy", 0);
> > > > +	if (IS_ERR(phy_node))
> > > > +		phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
> > > > +	return phy_node;
> > > 
> > > Why do you have three different ways to reference a PHY?
> > 
> > Compatibility with the DT version - note that "phy" and "phy-device"
> > are only used for non-ACPI fwnodes. This should allow us to convert
> > drivers where necessary without fear of causing DT regressions.
> 
> Ah.
> 
> A comment would be good here.

Sure. Will add comment.

Thanks
Calvin

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Grant Likely]

On 01/10/2020 05:00, Calvin Johnson wrote:
> On Wed, Sep 30, 2020 at 08:19:02PM +0200, Andrew Lunn wrote:
>> On Wed, Sep 30, 2020 at 07:07:25PM +0100, Russell King - ARM Linux admin wrote:
>>> On Wed, Sep 30, 2020 at 06:34:40PM +0200, Andrew Lunn wrote:
>>>>> @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
>>>>>    */
>>>>>   struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
>>>>>   {
>>>>> -	return fwnode_find_reference(fwnode, "phy-handle", 0);
>>>>> +	struct fwnode_handle *phy_node;
>>>>> +
>>>>> +	phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
>>>>> +	if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
>>>>> +		return phy_node;
>>>>> +	phy_node = fwnode_find_reference(fwnode, "phy", 0);
>>>>> +	if (IS_ERR(phy_node))
>>>>> +		phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
>>>>> +	return phy_node;
>>>>
>>>> Why do you have three different ways to reference a PHY?
>>>
>>> Compatibility with the DT version - note that "phy" and "phy-device"
>>> are only used for non-ACPI fwnodes. This should allow us to convert
>>> drivers where necessary without fear of causing DT regressions.
>>
>> Ah.
>>
>> A comment would be good here.
> 
> Sure. Will add comment.

Actually, I agree with Andrew. I don't see why new properties are being 
defined for following a reference from a property to a PHY node. This 
function shouldn't need to change at all.

g.

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Grant Likely]

On 30/09/2020 17:04, 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 <calvin.johnson@oss.nxp.com>
> ---
> 
>   drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
>   include/linux/phy.h          |  5 +++++
>   2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c4aec56d0a95..162abde6223d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -9,6 +9,7 @@
>   
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>   
> +#include <linux/acpi.h>
>   #include <linux/bitmap.h>
>   #include <linux/delay.h>
>   #include <linux/errno.h>
> @@ -845,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
>   	return 0;
>   }
>   
> +/* 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)
> +{
> +	unsigned int upper, lower;
> +	const char *cp;
> +	int ret;
> +
> +	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> +	if (ret)
> +		return 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);

This block, and the changes in patch 4 duplicate functions from 
drivers/of/of_mdio.c, but it doesn't refactor anything in 
drivers/of/of_mdio.c to use the new path. Is your intent to bring all of 
the parsing in these functions of "compatible" into the ACPI code path?

If so, then the existing code path needs to be refactored to work with 
fwnode_handle instead of device_node.

If not, then the DT path in these functions should call out to of_mdio, 
while the ACPI path only does what is necessary.

> +
>   /**
>    * get_phy_device - reads the specified PHY device and returns its @phy_device
>    *		    struct
> @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
>    */
>   struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
>   {
> -	return fwnode_find_reference(fwnode, "phy-handle", 0);
> +	struct fwnode_handle *phy_node;
> +
> +	phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> +	if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> +		return phy_node;
> +	phy_node = fwnode_find_reference(fwnode, "phy", 0);
> +	if (IS_ERR(phy_node))
> +		phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
> +	return phy_node;
>   }
>   EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
>   
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 7b1bf3d46fd3..b6814e04092f 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1378,6 +1378,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);
> @@ -1385,6 +1386,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)
>   {
> 

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Florian Fainelli]

On 10/2/2020 4:05 AM, Grant Likely wrote:
> 
> 
> On 30/09/2020 17:04, 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 <calvin.johnson@oss.nxp.com>
>> ---
>>
>>   drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
>>   include/linux/phy.h          |  5 +++++
>>   2 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index c4aec56d0a95..162abde6223d 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -9,6 +9,7 @@
>>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +#include <linux/acpi.h>
>>   #include <linux/bitmap.h>
>>   #include <linux/delay.h>
>>   #include <linux/errno.h>
>> @@ -845,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus, 
>> int addr, u32 *phy_id)
>>       return 0;
>>   }
>> +/* 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)
>> +{
>> +    unsigned int upper, lower;
>> +    const char *cp;
>> +    int ret;
>> +
>> +    ret = fwnode_property_read_string(fwnode, "compatible", &cp);
>> +    if (ret)
>> +        return 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);
> 
> This block, and the changes in patch 4 duplicate functions from 
> drivers/of/of_mdio.c, but it doesn't refactor anything in 
> drivers/of/of_mdio.c to use the new path. Is your intent to bring all of 
> the parsing in these functions of "compatible" into the ACPI code path?
> 
> If so, then the existing code path needs to be refactored to work with 
> fwnode_handle instead of device_node.
> 
> If not, then the DT path in these functions should call out to of_mdio, 
> while the ACPI path only does what is necessary.

Rob has been asking before to have drivers/of/of_mdio.c be merged or at 
least relocated within drivers/net/phy where it would naturally belong. 
As a preliminary step towards ACPI support that would seem reasonable to do.

Then, as Grant suggests you can start re-factoring as much as possible 
with using fwnode_handle.
-- 
Florian

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Russell King - ARM Linux admin]

On Fri, Oct 02, 2020 at 08:14:07AM -0700, Florian Fainelli wrote:
> On 10/2/2020 4:05 AM, Grant Likely wrote:
> > On 30/09/2020 17:04, 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 <calvin.johnson@oss.nxp.com>
> > > ---
> > > 
> > >   drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
> > >   include/linux/phy.h          |  5 +++++
> > >   2 files changed, 36 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index c4aec56d0a95..162abde6223d 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -9,6 +9,7 @@
> > >   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +#include <linux/acpi.h>
> > >   #include <linux/bitmap.h>
> > >   #include <linux/delay.h>
> > >   #include <linux/errno.h>
> > > @@ -845,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus,
> > > int addr, u32 *phy_id)
> > >       return 0;
> > >   }
> > > +/* 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)
> > > +{
> > > +    unsigned int upper, lower;
> > > +    const char *cp;
> > > +    int ret;
> > > +
> > > +    ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> > > +    if (ret)
> > > +        return 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);
> > 
> > This block, and the changes in patch 4 duplicate functions from
> > drivers/of/of_mdio.c, but it doesn't refactor anything in
> > drivers/of/of_mdio.c to use the new path. Is your intent to bring all of
> > the parsing in these functions of "compatible" into the ACPI code path?
> > 
> > If so, then the existing code path needs to be refactored to work with
> > fwnode_handle instead of device_node.
> > 
> > If not, then the DT path in these functions should call out to of_mdio,
> > while the ACPI path only does what is necessary.
> 
> Rob has been asking before to have drivers/of/of_mdio.c be merged or at
> least relocated within drivers/net/phy where it would naturally belong. As a
> preliminary step towards ACPI support that would seem reasonable to do.

I think even I have commented on specific functions while reviewing
patches from NXP that the DT/ACPI code should use common bases...

I have been planning that if that doesn't get done, then I'd do it,
but really NXP should do it being the ones adding this infrastructure;
they should do the job properly and not take advantage of volunteers
in the community cleaning up their resulting submissions.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

Hi Russell and Florian,

On Fri, Oct 02, 2020 at 04:50:26PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Oct 02, 2020 at 08:14:07AM -0700, Florian Fainelli wrote:
> > On 10/2/2020 4:05 AM, Grant Likely wrote:
> > > On 30/09/2020 17:04, 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 <calvin.johnson@oss.nxp.com>
> > > > ---
> > > > 
> > > >   drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
> > > >   include/linux/phy.h          |  5 +++++
> > > >   2 files changed, 36 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > > index c4aec56d0a95..162abde6223d 100644
> > > > --- a/drivers/net/phy/phy_device.c
> > > > +++ b/drivers/net/phy/phy_device.c
> > > > @@ -9,6 +9,7 @@
> > > >   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > +#include <linux/acpi.h>
> > > >   #include <linux/bitmap.h>
> > > >   #include <linux/delay.h>
> > > >   #include <linux/errno.h>
> > > > @@ -845,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus,
> > > > int addr, u32 *phy_id)
> > > >       return 0;
> > > >   }
> > > > +/* 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)
> > > > +{
> > > > +    unsigned int upper, lower;
> > > > +    const char *cp;
> > > > +    int ret;
> > > > +
> > > > +    ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> > > > +    if (ret)
> > > > +        return 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);
> > > 
> > > This block, and the changes in patch 4 duplicate functions from
> > > drivers/of/of_mdio.c, but it doesn't refactor anything in
> > > drivers/of/of_mdio.c to use the new path. Is your intent to bring all of
> > > the parsing in these functions of "compatible" into the ACPI code path?
> > > 
> > > If so, then the existing code path needs to be refactored to work with
> > > fwnode_handle instead of device_node.
> > > 
> > > If not, then the DT path in these functions should call out to of_mdio,
> > > while the ACPI path only does what is necessary.
> > 
> > Rob has been asking before to have drivers/of/of_mdio.c be merged or at
> > least relocated within drivers/net/phy where it would naturally belong. As a
> > preliminary step towards ACPI support that would seem reasonable to do.
> 
> I think even I have commented on specific functions while reviewing
> patches from NXP that the DT/ACPI code should use common bases...
> 
> I have been planning that if that doesn't get done, then I'd do it,
> but really NXP should do it being the ones adding this infrastructure;
> they should do the job properly and not take advantage of volunteers
> in the community cleaning up their resulting submissions.

I'll work on it.

Thanks
Calvin

Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

On Fri, Oct 02, 2020 at 12:05:14PM +0100, Grant Likely wrote:
> 
> 
> On 30/09/2020 17:04, 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 <calvin.johnson@oss.nxp.com>
> > ---
> > 
> >   drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
> >   include/linux/phy.h          |  5 +++++
> >   2 files changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index c4aec56d0a95..162abde6223d 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -9,6 +9,7 @@
> >   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +#include <linux/acpi.h>
> >   #include <linux/bitmap.h>
> >   #include <linux/delay.h>
> >   #include <linux/errno.h>
> > @@ -845,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
> >   	return 0;
> >   }
> > +/* 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)
> > +{
> > +	unsigned int upper, lower;
> > +	const char *cp;
> > +	int ret;
> > +
> > +	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> > +	if (ret)
> > +		return 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);
> 
> This block, and the changes in patch 4 duplicate functions from
> drivers/of/of_mdio.c, but it doesn't refactor anything in
> drivers/of/of_mdio.c to use the new path. Is your intent to bring all of the
> parsing in these functions of "compatible" into the ACPI code path?
> 
> If so, then the existing code path needs to be refactored to work with
> fwnode_handle instead of device_node.
> 
> If not, then the DT path in these functions should call out to of_mdio,
> while the ACPI path only does what is necessary.

I'll work on refactoring as Florian and Rob are also suggesting the same.

Thanks
Calvin

[net-next PATCH v1 4/7] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Calvin Johnson]

Introduce fwnode_mdiobus_register_phy() to register PHYs on the
mdiobus. From the compatible string, identify whether the PHY is
c45 and based on this create a PHY device instance which is
registered on the mdiobus.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

 drivers/net/phy/mdio_bus.c | 40 ++++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h       |  2 ++
 2 files changed, 42 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 0af20faad69d..693eb752cbf7 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -106,6 +106,46 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev)
 }
 EXPORT_SYMBOL(mdiobus_unregister_device);
 
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+				struct fwnode_handle *child, u32 addr)
+{
+	struct phy_device *phy;
+	bool is_c45;
+	const char *cp;
+	u32 phy_id;
+	int rc;
+
+	rc = fwnode_property_read_string(child, "compatible", &cp);
+	is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45"));
+
+	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
+		phy = get_phy_device(bus, addr, is_c45);
+	else
+		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	phy->irq = bus->irq[addr];
+
+	/* Associate the fwnode with the device structure so it
+	 * can be looked up later.
+	 */
+	phy->mdio.dev.fwnode = child;
+
+	/* All data is now stored in the phy struct, so register it */
+	rc = phy_device_register(phy);
+	if (rc) {
+		phy_device_free(phy);
+		fwnode_handle_put(phy->mdio.dev.fwnode);
+		return rc;
+	}
+
+	dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
+
+	return 0;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
+
 struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
 {
 	struct mdio_device *mdiodev = bus->mdio_map[addr];
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index dbd69b3d170b..f138ec333725 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -368,6 +368,8 @@ int mdiobus_register_device(struct mdio_device *mdiodev);
 int mdiobus_unregister_device(struct mdio_device *mdiodev);
 bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
 struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+				      struct fwnode_handle *child, u32 addr);
 
 /**
  * mdio_module_driver() - Helper macro for registering mdio drivers
-- 
2.17.1

Re: [net-next PATCH v1 4/7] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[David Miller]

From: Calvin Johnson <calvin.johnson@oss.nxp.com>
Date: Wed, 30 Sep 2020 21:34:27 +0530

> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -106,6 +106,46 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev)
>  }
>  EXPORT_SYMBOL(mdiobus_unregister_device);
>  
> +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> +				struct fwnode_handle *child, u32 addr)
> +{
> +	struct phy_device *phy;
> +	bool is_c45;
> +	const char *cp;
> +	u32 phy_id;
> +	int rc;

Reverse christmas tree here please.

[net-next PATCH v1 5/7] phylink: introduce phylink_fwnode_phy_connect()

[Calvin Johnson]

Define phylink_fwnode_phy_connect() to connect phy specified by
a fwnode to a phylink instance.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

 drivers/net/phy/phylink.c | 51 +++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 54 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index fe2296fdda19..ca88bd90d605 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -5,6 +5,7 @@
  *
  * Copyright (C) 2015 Russell King
  */
+#include <linux/acpi.h>
 #include <linux/ethtool.h>
 #include <linux/export.h>
 #include <linux/gpio/consumer.h>
@@ -1120,6 +1121,56 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
 }
 EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
 
+/**
+ * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode.
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @fwnode: a pointer to a &struct fwnode_handle.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
+ *
+ * Connect the phy specified @fwnode to the phylink instance specified
+ * by @pl.
+ *
+ * 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_fwnode;
+	struct phy_device *phy_dev;
+	int ret;
+
+	if (is_of_node(fwnode))
+		return phylink_of_phy_connect(pl, to_of_node(fwnode), flags);
+	if (is_acpi_device_node(fwnode)) {
+		phy_fwnode = fwnode_get_phy_node(fwnode);
+		if (IS_ERR(phy_fwnode)) {
+			if (pl->cfg_link_an_mode == MLO_AN_PHY)
+				return -ENODEV;
+			return 0;
+		}
+
+		phy_dev = fwnode_phy_find_device(phy_fwnode);
+		fwnode_handle_put(phy_fwnode);
+		if (!phy_dev)
+			return -ENODEV;
+
+		ret = phy_attach_direct(pl->netdev, phy_dev, flags,
+					pl->link_interface);
+		if (ret)
+			return ret;
+
+		ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
+		if (ret)
+			phy_detach(phy_dev);
+
+		return ret;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);
+
 /**
  * phylink_disconnect_phy() - disconnect any PHY attached to the phylink
  *   instance.
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index d81a714cfbbd..75d4f99090fd 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -439,6 +439,9 @@ void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
 int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags);
 void phylink_disconnect_phy(struct phylink *);
 
 void phylink_mac_change(struct phylink *, bool up);
-- 
2.17.1

[net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Calvin Johnson]

Modify dpaa2_mac_connect() to support ACPI along with DT.
Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
DT or ACPI.

Replace of_get_phy_mode with fwnode_get_phy_mode to get
phy-mode for a dpmac_node.

Use helper function phylink_fwnode_phy_connect() to find phy_dev and
connect to mac->phylink.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 79 ++++++++++++-------
 1 file changed, 50 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 90cd243070d7..18502ee83e46 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -3,6 +3,7 @@
 
 #include "dpaa2-eth.h"
 #include "dpaa2-mac.h"
+#include <linux/acpi.h>
 
 #define phylink_to_dpaa2_mac(config) \
 	container_of((config), struct dpaa2_mac, phylink_config)
@@ -35,38 +36,56 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
 }
 
 /* Caller must call of_node_put on the returned value */
-static struct device_node *dpaa2_mac_get_node(u16 dpmac_id)
+static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
+						u16 dpmac_id)
 {
-	struct device_node *dpmacs, *dpmac = NULL;
-	u32 id;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct fwnode_handle *dpmacs, *dpmac = NULL;
+	unsigned long long adr;
+	acpi_status status;
 	int err;
+	u32 id;
 
-	dpmacs = of_find_node_by_name(NULL, "dpmacs");
-	if (!dpmacs)
-		return NULL;
+	if (is_of_node(dev->parent->fwnode)) {
+		dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
+		if (!dpmacs)
+			return NULL;
+
+		while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
+			err = fwnode_property_read_u32(dpmac, "reg", &id);
+			if (err)
+				continue;
+			if (id == dpmac_id)
+				return dpmac;
+		}
 
-	while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
-		err = of_property_read_u32(dpmac, "reg", &id);
-		if (err)
-			continue;
-		if (id == dpmac_id)
-			break;
+	} else if (is_acpi_node(dev->parent->fwnode)) {
+		device_for_each_child_node(dev->parent, dpmac) {
+			status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
+						       "_ADR", NULL, &adr);
+			if (ACPI_FAILURE(status)) {
+				pr_debug("_ADR returned %d on %s\n",
+					 status, (char *)buffer.pointer);
+				continue;
+			} else {
+				id = (u32)adr;
+				if (id == dpmac_id)
+					return dpmac;
+			}
+		}
 	}
-
-	of_node_put(dpmacs);
-
-	return dpmac;
+	return NULL;
 }
 
-static int dpaa2_mac_get_if_mode(struct device_node *node,
+static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node,
 				 struct dpmac_attr attr)
 {
 	phy_interface_t if_mode;
 	int err;
 
-	err = of_get_phy_mode(node, &if_mode);
-	if (!err)
-		return if_mode;
+	err = fwnode_get_phy_mode(dpmac_node);
+	if (err > 0)
+		return err;
 
 	err = phy_mode(attr.eth_if, &if_mode);
 	if (!err)
@@ -303,7 +322,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 {
 	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
 	struct net_device *net_dev = mac->net_dev;
-	struct device_node *dpmac_node;
+	struct fwnode_handle *dpmac_node = NULL;
 	struct phylink *phylink;
 	struct dpmac_attr attr;
 	int err;
@@ -323,7 +342,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 
 	mac->if_link_type = attr.link_type;
 
-	dpmac_node = dpaa2_mac_get_node(attr.id);
+	dpmac_node = dpaa2_mac_get_node(&mac->mc_dev->dev, attr.id);
 	if (!dpmac_node) {
 		netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id);
 		err = -ENODEV;
@@ -341,7 +360,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	 * error out if the interface mode requests them and there is no PHY
 	 * to act upon them
 	 */
-	if (of_phy_is_fixed_link(dpmac_node) &&
+	if (of_phy_is_fixed_link(to_of_node(dpmac_node)) &&
 	    (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID ||
 	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
 	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) {
@@ -352,7 +371,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 
 	if (attr.link_type == DPMAC_LINK_TYPE_PHY &&
 	    attr.eth_if != DPMAC_ETH_IF_RGMII) {
-		err = dpaa2_pcs_create(mac, dpmac_node, attr.id);
+		err = dpaa2_pcs_create(mac, to_of_node(dpmac_node), attr.id);
 		if (err)
 			goto err_put_node;
 	}
@@ -361,7 +380,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	mac->phylink_config.type = PHYLINK_NETDEV;
 
 	phylink = phylink_create(&mac->phylink_config,
-				 of_fwnode_handle(dpmac_node), mac->if_mode,
+				 dpmac_node, mac->if_mode,
 				 &dpaa2_mac_phylink_ops);
 	if (IS_ERR(phylink)) {
 		err = PTR_ERR(phylink);
@@ -372,13 +391,14 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	if (mac->pcs)
 		phylink_set_pcs(mac->phylink, &mac->pcs->pcs);
 
-	err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0);
+	err = phylink_fwnode_phy_connect(mac->phylink, dpmac_node, 0);
 	if (err) {
-		netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err);
+		netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err);
 		goto err_phylink_destroy;
 	}
 
-	of_node_put(dpmac_node);
+	if (is_of_node(dpmac_node))
+		of_node_put(to_of_node(dpmac_node));
 
 	return 0;
 
@@ -387,7 +407,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 err_pcs_destroy:
 	dpaa2_pcs_destroy(mac);
 err_put_node:
-	of_node_put(dpmac_node);
+	if (is_of_node(dpmac_node))
+		of_node_put(to_of_node(dpmac_node));
 err_close_dpmac:
 	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
 	return err;
-- 
2.17.1

Re: [net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Andy Shevchenko]

On Wed, Sep 30, 2020 at 7:06 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Modify dpaa2_mac_connect() to support ACPI along with DT.
> Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
> DT or ACPI.
>
> Replace of_get_phy_mode with fwnode_get_phy_mode to get
> phy-mode for a dpmac_node.
>
> Use helper function phylink_fwnode_phy_connect() to find phy_dev and
> connect to mac->phylink.

...

>  #include "dpaa2-eth.h"
>  #include "dpaa2-mac.h"

> +#include <linux/acpi.h>

Please, put generic headers first.

> +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +       struct fwnode_handle *dpmacs, *dpmac = NULL;
> +       unsigned long long adr;
> +       acpi_status status;
>         int err;
> +       u32 id;
>
> -       dpmacs = of_find_node_by_name(NULL, "dpmacs");
> -       if (!dpmacs)
> -               return NULL;
> +       if (is_of_node(dev->parent->fwnode)) {
> +               dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
> +               if (!dpmacs)
> +                       return NULL;
> +
> +               while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> +                       err = fwnode_property_read_u32(dpmac, "reg", &id);
> +                       if (err)
> +                               continue;
> +                       if (id == dpmac_id)
> +                               return dpmac;
> +               }
>
> +       } else if (is_acpi_node(dev->parent->fwnode)) {
> +               device_for_each_child_node(dev->parent, dpmac) {
> +                       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> +                                                      "_ADR", NULL, &adr);
> +                       if (ACPI_FAILURE(status)) {
> +                               pr_debug("_ADR returned %d on %s\n",
> +                                        status, (char *)buffer.pointer);
> +                               continue;
> +                       } else {
> +                               id = (u32)adr;
> +                               if (id == dpmac_id)
> +                                       return dpmac;
> +                       }
> +               }

Can you rather implement generic one which will be

int fwnode_get_child_id(struct fwnode_handle *fwnode, u64 *id);

and put the logic of retrieving 'reg' or _ADR? Also, for the latter we
have a special macro
METHOD_NAME__ADR.

See [1] as well. Same idea I have shared already.

[1]: https://lore.kernel.org/linux-iio/20200824054347.3805-1-william.sung@advantech.com.tw/T/#m5f61921fa67a5b40522b7f7b17216e0d204647be

...

> -       of_node_put(dpmac_node);
> +       if (is_of_node(dpmac_node))
> +               of_node_put(to_of_node(dpmac_node));

I'm not sure why you can't use fwnode_handle_put()?

> +       if (is_of_node(dpmac_node))
> +               of_node_put(to_of_node(dpmac_node));

Ditto.

--
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Calvin Johnson]

Hi Andy,

On Thu, Oct 01, 2020 at 06:36:06PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 30, 2020 at 7:06 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Modify dpaa2_mac_connect() to support ACPI along with DT.
> > Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
> > DT or ACPI.
> >
> > Replace of_get_phy_mode with fwnode_get_phy_mode to get
> > phy-mode for a dpmac_node.
> >
> > Use helper function phylink_fwnode_phy_connect() to find phy_dev and
> > connect to mac->phylink.
> 
> ...
> 
> >  #include "dpaa2-eth.h"
> >  #include "dpaa2-mac.h"
> 
> > +#include <linux/acpi.h>
> 
> Please, put generic headers first.
> 
> > +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +       struct fwnode_handle *dpmacs, *dpmac = NULL;
> > +       unsigned long long adr;
> > +       acpi_status status;
> >         int err;
> > +       u32 id;
> >
> > -       dpmacs = of_find_node_by_name(NULL, "dpmacs");
> > -       if (!dpmacs)
> > -               return NULL;
> > +       if (is_of_node(dev->parent->fwnode)) {
> > +               dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
> > +               if (!dpmacs)
> > +                       return NULL;
> > +
> > +               while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> > +                       err = fwnode_property_read_u32(dpmac, "reg", &id);
> > +                       if (err)
> > +                               continue;
> > +                       if (id == dpmac_id)
> > +                               return dpmac;
> > +               }
> >
> > +       } else if (is_acpi_node(dev->parent->fwnode)) {
> > +               device_for_each_child_node(dev->parent, dpmac) {
> > +                       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> > +                                                      "_ADR", NULL, &adr);
> > +                       if (ACPI_FAILURE(status)) {
> > +                               pr_debug("_ADR returned %d on %s\n",
> > +                                        status, (char *)buffer.pointer);
> > +                               continue;
> > +                       } else {
> > +                               id = (u32)adr;
> > +                               if (id == dpmac_id)
> > +                                       return dpmac;
> > +                       }
> > +               }
> 
> Can you rather implement generic one which will be
> 
> int fwnode_get_child_id(struct fwnode_handle *fwnode, u64 *id);
> 
> and put the logic of retrieving 'reg' or _ADR? Also, for the latter we
> have a special macro
> METHOD_NAME__ADR.
> 
> See [1] as well. Same idea I have shared already.
> 
> [1]: https://lore.kernel.org/linux-iio/20200824054347.3805-1-william.sung@advantech.com.tw/T/#m5f61921fa67a5b40522b7f7b17216e0d204647be
> 
> ...
> 
> > -       of_node_put(dpmac_node);
> > +       if (is_of_node(dpmac_node))
> > +               of_node_put(to_of_node(dpmac_node));
> 
> I'm not sure why you can't use fwnode_handle_put()?
> 
> > +       if (is_of_node(dpmac_node))
> > +               of_node_put(to_of_node(dpmac_node));
> 
> Ditto.

Sure. I'll take care of these comments.
Thanks
Calvin

Re: [net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Grant Likely]

On 30/09/2020 17:04, Calvin Johnson wrote:
> Modify dpaa2_mac_connect() to support ACPI along with DT.
> Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
> DT or ACPI.
> 
> Replace of_get_phy_mode with fwnode_get_phy_mode to get
> phy-mode for a dpmac_node.
> 
> Use helper function phylink_fwnode_phy_connect() to find phy_dev and
> connect to mac->phylink.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
>   .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 79 ++++++++++++-------
>   1 file changed, 50 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> index 90cd243070d7..18502ee83e46 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> @@ -3,6 +3,7 @@
>   
>   #include "dpaa2-eth.h"
>   #include "dpaa2-mac.h"
> +#include <linux/acpi.h>
>   
>   #define phylink_to_dpaa2_mac(config) \
>   	container_of((config), struct dpaa2_mac, phylink_config)
> @@ -35,38 +36,56 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
>   }
>   
>   /* Caller must call of_node_put on the returned value */
> -static struct device_node *dpaa2_mac_get_node(u16 dpmac_id)
> +static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
> +						u16 dpmac_id)
>   {
> -	struct device_node *dpmacs, *dpmac = NULL;
> -	u32 id;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	struct fwnode_handle *dpmacs, *dpmac = NULL;
> +	unsigned long long adr;
> +	acpi_status status;
>   	int err;
> +	u32 id;
>   
> -	dpmacs = of_find_node_by_name(NULL, "dpmacs");
> -	if (!dpmacs)
> -		return NULL;
> +	if (is_of_node(dev->parent->fwnode)) {
> +		dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
> +		if (!dpmacs)
> +			return NULL;
> +
> +		while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> +			err = fwnode_property_read_u32(dpmac, "reg", &id);
> +			if (err)
> +				continue;
> +			if (id == dpmac_id)
> +				return dpmac;
> +		}
There is a change of behaviour here that isn't described in the patch 
description, so I'm having trouble following the intent. The original 
code searches the tree for a node named "dpmacs", but the new code 
constrains the search to just the parent device.

Also, because the new code path is only used in the DT case, I don't see 
why the behaviour change is required. If it is a bug fix, it should be 
broken out into a separate patch. If it is something else, can you 
describe what the reasoning is?

I also see that the change to the code has dropped the of_node_put() on 
the exit path.

>   
> -	while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
> -		err = of_property_read_u32(dpmac, "reg", &id);
> -		if (err)
> -			continue;
> -		if (id == dpmac_id)
> -			break;
> +	} else if (is_acpi_node(dev->parent->fwnode)) {
> +		device_for_each_child_node(dev->parent, dpmac) {
> +			status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> +						       "_ADR", NULL, &adr);
> +			if (ACPI_FAILURE(status)) {
> +				pr_debug("_ADR returned %d on %s\n",
> +					 status, (char *)buffer.pointer);
> +				continue;
> +			} else {
> +				id = (u32)adr;
> +				if (id == dpmac_id)
> +					return dpmac;
> +			}
> +		}
>   	}
> -
> -	of_node_put(dpmacs);
> -
> -	return dpmac;
> +	return NULL;
>   }
>   
> -static int dpaa2_mac_get_if_mode(struct device_node *node,
> +static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node,
>   				 struct dpmac_attr attr)
>   {
>   	phy_interface_t if_mode;
>   	int err;
>   
> -	err = of_get_phy_mode(node, &if_mode);
> -	if (!err)
> -		return if_mode;
> +	err = fwnode_get_phy_mode(dpmac_node);
> +	if (err > 0)
> +		return err;

Is this correct? The function prototype from patch 2 is:

struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)

It returns struct fwnode_handle *. Does this compile?

>   
>   	err = phy_mode(attr.eth_if, &if_mode);
>   	if (!err)
> @@ -303,7 +322,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>   {
>   	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
>   	struct net_device *net_dev = mac->net_dev;
> -	struct device_node *dpmac_node;
> +	struct fwnode_handle *dpmac_node = NULL;
>   	struct phylink *phylink;
>   	struct dpmac_attr attr;
>   	int err;
> @@ -323,7 +342,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>   
>   	mac->if_link_type = attr.link_type;
>   
> -	dpmac_node = dpaa2_mac_get_node(attr.id);
> +	dpmac_node = dpaa2_mac_get_node(&mac->mc_dev->dev, attr.id);
>   	if (!dpmac_node) {
>   		netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id);
>   		err = -ENODEV;
> @@ -341,7 +360,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>   	 * error out if the interface mode requests them and there is no PHY
>   	 * to act upon them
>   	 */
> -	if (of_phy_is_fixed_link(dpmac_node) &&
> +	if (of_phy_is_fixed_link(to_of_node(dpmac_node)) &&
>   	    (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>   	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
>   	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) {
> @@ -352,7 +371,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>   
>   	if (attr.link_type == DPMAC_LINK_TYPE_PHY &&
>   	    attr.eth_if != DPMAC_ETH_IF_RGMII) {
> -		err = dpaa2_pcs_create(mac, dpmac_node, attr.id);
> +		err = dpaa2_pcs_create(mac, to_of_node(dpmac_node), attr.id);
>   		if (err)
>   			goto err_put_node;
>   	}
> @@ -361,7 +380,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>   	mac->phylink_config.type = PHYLINK_NETDEV;
>   
>   	phylink = phylink_create(&mac->phylink_config,
> -				 of_fwnode_handle(dpmac_node), mac->if_mode,
> +				 dpmac_node, mac->if_mode,
>   				 &dpaa2_mac_phylink_ops);
>   	if (IS_ERR(phylink)) {
>   		err = PTR_ERR(phylink);
> @@ -372,13 +391,14 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>   	if (mac->pcs)
>   		phylink_set_pcs(mac->phylink, &mac->pcs->pcs);
>   
> -	err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0);
> +	err = phylink_fwnode_phy_connect(mac->phylink, dpmac_node, 0);
>   	if (err) {
> -		netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err);
> +		netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err);
>   		goto err_phylink_destroy;
>   	}
>   
> -	of_node_put(dpmac_node);
> +	if (is_of_node(dpmac_node))
> +		of_node_put(to_of_node(dpmac_node));
>   
>   	return 0;
>   
> @@ -387,7 +407,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>   err_pcs_destroy:
>   	dpaa2_pcs_destroy(mac);
>   err_put_node:
> -	of_node_put(dpmac_node);
> +	if (is_of_node(dpmac_node))
> +		of_node_put(to_of_node(dpmac_node));
>   err_close_dpmac:
>   	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
>   	return err;
> 

Re: [net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Calvin Johnson]

Hi Grant,

On Fri, Oct 02, 2020 at 12:22:37PM +0100, Grant Likely wrote:
> 
> 
> On 30/09/2020 17:04, Calvin Johnson wrote:
> > Modify dpaa2_mac_connect() to support ACPI along with DT.
> > Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
> > DT or ACPI.
> > 
> > Replace of_get_phy_mode with fwnode_get_phy_mode to get
> > phy-mode for a dpmac_node.
> > 
> > Use helper function phylink_fwnode_phy_connect() to find phy_dev and
> > connect to mac->phylink.
> > 
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
> > 
> >   .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 79 ++++++++++++-------
> >   1 file changed, 50 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > index 90cd243070d7..18502ee83e46 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > @@ -3,6 +3,7 @@
> >   #include "dpaa2-eth.h"
> >   #include "dpaa2-mac.h"
> > +#include <linux/acpi.h>
> >   #define phylink_to_dpaa2_mac(config) \
> >   	container_of((config), struct dpaa2_mac, phylink_config)
> > @@ -35,38 +36,56 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
> >   }
> >   /* Caller must call of_node_put on the returned value */
> > -static struct device_node *dpaa2_mac_get_node(u16 dpmac_id)
> > +static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
> > +						u16 dpmac_id)
> >   {
> > -	struct device_node *dpmacs, *dpmac = NULL;
> > -	u32 id;
> > +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +	struct fwnode_handle *dpmacs, *dpmac = NULL;
> > +	unsigned long long adr;
> > +	acpi_status status;
> >   	int err;
> > +	u32 id;
> > -	dpmacs = of_find_node_by_name(NULL, "dpmacs");
> > -	if (!dpmacs)
> > -		return NULL;
> > +	if (is_of_node(dev->parent->fwnode)) {
> > +		dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
> > +		if (!dpmacs)
> > +			return NULL;
> > +
> > +		while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> > +			err = fwnode_property_read_u32(dpmac, "reg", &id);
> > +			if (err)
> > +				continue;
> > +			if (id == dpmac_id)
> > +				return dpmac;
> > +		}
> There is a change of behaviour here that isn't described in the patch
> description, so I'm having trouble following the intent. The original code
> searches the tree for a node named "dpmacs", but the new code constrains the
> search to just the parent device.
> 
> Also, because the new code path is only used in the DT case, I don't see why
> the behaviour change is required. If it is a bug fix, it should be broken
> out into a separate patch. If it is something else, can you describe what
> the reasoning is?

Yes, the behaviour for ACPI had to be changed as I couldn't find an ACPI method
to find named nodes. I did this change some time back and it didn't work for
ACPI. I'll revisit this once again and keep original code if needed.
Behaviour for DT hasn't changed although the APIs changed.

> 
> I also see that the change to the code has dropped the of_node_put() on the
> exit path.

Sure, I'll fix it.
> 
> > -	while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
> > -		err = of_property_read_u32(dpmac, "reg", &id);
> > -		if (err)
> > -			continue;
> > -		if (id == dpmac_id)
> > -			break;
> > +	} else if (is_acpi_node(dev->parent->fwnode)) {
> > +		device_for_each_child_node(dev->parent, dpmac) {
> > +			status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> > +						       "_ADR", NULL, &adr);
> > +			if (ACPI_FAILURE(status)) {
> > +				pr_debug("_ADR returned %d on %s\n",
> > +					 status, (char *)buffer.pointer);
> > +				continue;
> > +			} else {
> > +				id = (u32)adr;
> > +				if (id == dpmac_id)
> > +					return dpmac;
> > +			}
> > +		}
> >   	}
> > -
> > -	of_node_put(dpmacs);
> > -
> > -	return dpmac;
> > +	return NULL;
> >   }
> > -static int dpaa2_mac_get_if_mode(struct device_node *node,
> > +static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node,
> >   				 struct dpmac_attr attr)
> >   {
> >   	phy_interface_t if_mode;
> >   	int err;
> > -	err = of_get_phy_mode(node, &if_mode);
> > -	if (!err)
> > -		return if_mode;
> > +	err = fwnode_get_phy_mode(dpmac_node);
> > +	if (err > 0)
> > +		return err;
> 
> Is this correct? The function prototype from patch 2 is:
> 
> struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> 
> It returns struct fwnode_handle *. Does this compile?

Will correct this.

Thanks
Calvin

Re: [net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Calvin Johnson]

On Sat, Oct 03, 2020 at 11:09:49PM +0530, Calvin Johnson wrote:
> Hi Grant,
> 
> On Fri, Oct 02, 2020 at 12:22:37PM +0100, Grant Likely wrote:
> > 
> > 

<snip>

> > > -static int dpaa2_mac_get_if_mode(struct device_node *node,
> > > +static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node,
> > >   				 struct dpmac_attr attr)
> > >   {
> > >   	phy_interface_t if_mode;
> > >   	int err;
> > > -	err = of_get_phy_mode(node, &if_mode);
> > > -	if (!err)
> > > -		return if_mode;
> > > +	err = fwnode_get_phy_mode(dpmac_node);
> > > +	if (err > 0)
> > > +		return err;
> > 
> > Is this correct? The function prototype from patch 2 is:
> > 
> > struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> > 
> > It returns struct fwnode_handle *. Does this compile?
> 
> Will correct this.

Sorry, this change looks correct to me. Actully, the function prototype is:
int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
It is from drivers/base/property.c.

fwnode_get_phy_node() defined in patch-2 is used in phylink_fwnode_phy_connect()

The confusion maybe due to one letter difference in the fn names.

Thanks
Calvin

[net-next PATCH v1 7/7] net/fsl: Use _ADR ACPI object to register PHYs

[Calvin Johnson]

PHYs on an mdio bus has address which can be obtained from ACPI
DSDT table using the _ADR object.

DSDT Eg: PHYs connected to MDI0 bus.
-------------------------
Scope(\_SB.MDI0)
{
  Device(PHY1) {
    Name (_ADR, 0x1)
  } // end of PHY1

  Device(PHY2) {
    Name (_ADR, 0x2)
  } // end of PHY2
} // end of MDI0
-------------------------

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

 drivers/net/ethernet/freescale/xgmac_mdio.c | 48 +++++++++++++++++++--
 1 file changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 98be51d8b08c..fb272564855e 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -2,6 +2,7 @@
  * QorIQ 10G MDIO Controller
  *
  * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2020 NXP
  *
  * Authors: Andy Fleming <afleming@freescale.com>
  *          Timur Tabi <timur@freescale.com>
@@ -11,6 +12,7 @@
  * kind, whether express or implied.
  */
 
+#include <linux/acpi.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
@@ -248,6 +250,10 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct mdio_fsl_priv *priv;
 	int ret;
+	struct fwnode_handle *fwnode;
+	struct fwnode_handle *child;
+	unsigned long long addr;
+	acpi_status status;
 
 	/* In DPAA-1, MDIO is one of the many FMan sub-devices. The FMan
 	 * defines a register space that spans a large area, covering all the
@@ -284,10 +290,44 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 
 	priv->has_a011043 = device_property_read_bool(&pdev->dev,
 						      "fsl,erratum-a011043");
-
-	ret = of_mdiobus_register(bus, np);
-	if (ret) {
-		dev_err(&pdev->dev, "cannot register MDIO bus\n");
+	if (is_of_node(pdev->dev.fwnode)) {
+		ret = of_mdiobus_register(bus, np);
+		if (ret) {
+			dev_err(&pdev->dev, "cannot register MDIO bus\n");
+			goto err_registration;
+		}
+	} else if (is_acpi_node(pdev->dev.fwnode)) {
+		priv->is_little_endian = true;
+		/* Mask out all PHYs from auto probing. */
+		bus->phy_mask = ~0;
+		ret = mdiobus_register(bus);
+		if (ret) {
+			dev_err(&pdev->dev, "mdiobus register err(%d)\n", ret);
+			return ret;
+		}
+
+		fwnode = pdev->dev.fwnode;
+	/* Loop over the child nodes and register a phy_device for each PHY */
+		fwnode_for_each_child_node(fwnode, child) {
+			status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
+						       "_ADR", NULL, &addr);
+			if (ACPI_FAILURE(status)) {
+				pr_debug("_ADR returned %d\n", status);
+				continue;
+			}
+
+			if (addr < 0 || addr >= PHY_MAX_ADDR)
+				continue;
+
+			ret = fwnode_mdiobus_register_phy(bus, child, addr);
+			if (ret == -ENODEV)
+				dev_err(&bus->dev,
+					"MDIO device at address %lld is missing.\n",
+					addr);
+		}
+	} else {
+		dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n");
+		ret = -ENXIO;
 		goto err_registration;
 	}
 
-- 
2.17.1

Re: [net-next PATCH v1 7/7] net/fsl: Use _ADR ACPI object to register PHYs

[Andrew Lunn]

Hi Calvin

>  	priv->has_a011043 = device_property_read_bool(&pdev->dev,
>  						      "fsl,erratum-a011043");
> -
> -	ret = of_mdiobus_register(bus, np);
> -	if (ret) {
> -		dev_err(&pdev->dev, "cannot register MDIO bus\n");
> +	if (is_of_node(pdev->dev.fwnode)) {
> +		ret = of_mdiobus_register(bus, np);
> +		if (ret) {
> +			dev_err(&pdev->dev, "cannot register MDIO bus\n");
> +			goto err_registration;
> +		}
> +	} else if (is_acpi_node(pdev->dev.fwnode)) {
> +		priv->is_little_endian = true;
> +		/* Mask out all PHYs from auto probing. */
> +		bus->phy_mask = ~0;
> +		ret = mdiobus_register(bus);
> +		if (ret) {
> +			dev_err(&pdev->dev, "mdiobus register err(%d)\n", ret);
> +			return ret;
> +		}
> +
> +		fwnode = pdev->dev.fwnode;

> +	/* Loop over the child nodes and register a phy_device for each PHY */
> +		fwnode_for_each_child_node(fwnode, child) {
> +			status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
> +						       "_ADR", NULL, &addr);
> +			if (ACPI_FAILURE(status)) {
> +				pr_debug("_ADR returned %d\n", status);
> +				continue;
> +			}
> +
> +			if (addr < 0 || addr >= PHY_MAX_ADDR)
> +				continue;
> +
> +			ret = fwnode_mdiobus_register_phy(bus, child, addr);
> +			if (ret == -ENODEV)
> +				dev_err(&bus->dev,
> +					"MDIO device at address %lld is missing.\n",
> +					addr);
> +		}

Hi Calvin

This looping over the properties should be in the core, in the same
way of_mdiobus_register() loops over the OF properties in the core.
We don't want MDIO drivers doing this in their own way, with their own
bugs.

    Andrew

Re: [net-next PATCH v1 7/7] net/fsl: Use _ADR ACPI object to register PHYs

[David Miller]

From: Calvin Johnson <calvin.johnson@oss.nxp.com>
Date: Wed, 30 Sep 2020 21:34:30 +0530

> @@ -248,6 +250,10 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	struct mdio_fsl_priv *priv;
>  	int ret;
> +	struct fwnode_handle *fwnode;
> +	struct fwnode_handle *child;
> +	unsigned long long addr;
> +	acpi_status status;
>  

Reverse chrstimas tree here too, please.