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@xxxxxxxxxxx>
> >
> > ---
> >
> > 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
