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

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

On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote:
> On 15/07/2020 10:03, Calvin Johnson wrote:
> > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> > provide them to be connected to MAC.
> > 
> > An ACPI node property "mdio-handle" is introduced to reference the
> > MDIO bus on which PHYs are registered with autoprobing method used
> > by mdiobus_register().
> > 
> > Describe properties "phy-channel" and "phy-mode"
> > 
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > 
> > ---
> > 
> > Changes in v7: None
> > Changes in v6: None
> > Changes in v5: None
> > Changes in v4: None
> > Changes in v3:
> > - cleanup based on v2 comments
> > - Added description for more properties
> > - Added MDIO node DSDT entry
> > 
> > Changes in v2: None
> > 
> >   Documentation/firmware-guide/acpi/dsd/phy.rst | 90 +++++++++++++++++++
> >   1 file changed, 90 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..0132fee10b45
> > --- /dev/null
> > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
> > @@ -0,0 +1,90 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +=========================
> > +MDIO bus and PHYs in ACPI
> > +=========================
> > +
> > +The PHYs on an mdiobus are probed and registered using mdiobus_register().
> > +Later, for connecting these PHYs to MAC, the PHYs registered on the
> > +mdiobus have to be referenced.
> > +
> > +mdio-handle
> > +-----------
> > +For each MAC node, a property "mdio-handle" is used to reference the
> > +MDIO bus on which the PHYs are registered. On getting hold of the MDIO
> > +bus, use find_phy_device() to get the PHY connected to the MAC.
> > +
> > +phy-channel
> > +-----------
> > +Property "phy-channel" defines the address of the PHY on the mdiobus.
> 
> Hi Calvin,
> 
> As we discussed offline, using 'mdio-handle'+'phy-channel' doesn't make a
> lot of sense. The MAC should be referencing the PHY it is attached to, not
> the MDIO bus. Referencing the PHY makes assumptions about how the PHY is
> wired into the system, which may not be via a traditional MDIO bus. These
> two properties should be dropped, and replaced with a single property
> reference to the PHY node.
> 
> e.g.,
>     Package () {"phy-handle", Package (){\_SB.MDI0.PHY1}}
> ​
> This is also future proof against any changes to how MDIO busses may get
> modeled in the future. They can be modeled as normal devices now, but if a
> future version of the ACPI spec adds an MDIO bus type, then the reference to
> the PHY from the MAC doesn't need to change.
> 

Hi Grant,

Understood. I'll make the changes.
> > +
> > +phy-mode
> > +--------
> > +Property "phy-mode" defines the type of PHY interface.
> > +
> > +An example of this is shown below::
> > +
> > +DSDT entry for MAC where MDIO node is referenced
> > +------------------------------------------------
> > +	Scope(\_SB.MCE0.PR17) // 1G
> > +	{
> > +	  Name (_DSD, Package () {
> > +	     ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > +		 Package () {
> > +		     Package () {"phy-channel", 1},
> > +		     Package () {"phy-mode", "rgmii-id"},
> > +		     Package () {"mdio-handle", Package (){\_SB.MDI0}}
> > +	      }
> > +	   })
> > +	}
> > +
> > +	Scope(\_SB.MCE0.PR18) // 1G
> > +	{
> > +	  Name (_DSD, Package () {
> > +	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > +		Package () {
> > +		    Package () {"phy-channel", 2},
> > +		    Package () {"phy-mode", "rgmii-id"},
> > +		    Package () {"mdio-handle", Package (){\_SB.MDI0}}
> > +	    }
> > +	  })
> > +	}
> > +
> > +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
> > +	    Name (_DSD, Package () {
> > +	      ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > +	      Package () {
> > +		 Package () {"little-endian", 1},
> > +	      }
> 
> Adopting the 'little-endian' property here makes little sense. This looks
> like legacy from old PowerPC DT platforms that doesn't belong here. I would
> drop this bit.

Ok. Will drop it.

Thanks
Calvin