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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 26 Sep 2020 10:00:25 +0530
From:   Calvin Johnson <calvin.johnson@....nxp.com>
To:     Grant Likely <grant.likely@....com>
Cc:     Jeremy Linton <jeremy.linton@....com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Jon <jon@...id-run.com>,
        Cristi Sovaiala <cristian.sovaiala@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        netdev@...r.kernel.org, linux.cj@...il.com,
        linux-acpi@...r.kernel.org, nd <nd@....com>
Subject: Re: [net-next PATCH v7 1/6] Documentation: ACPI: DSD: Document MDIO
 PHY

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