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

[Grant Likely <grant.likely@arm.com>]

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.

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

> +	    })
> +	  } // 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
> +	}
>