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@xxxxxxx> wrote:
>
>
>
> 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.

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!