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

From: Grant Likely
Date: Fri Oct 02 2020 - 07:09:38 EST




On 30/09/2020 17:37, Rafael J. Wysocki wrote:
On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
<calvin.johnson@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---

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?