| Message ID | 20210610163917.4138412-1-ciorneiioana@gmail.com |
|---|---|
| Headers | show |
| Series | ACPI support for dpaa2 driver | expand |
On Thu, Jun 10, 2021 at 6:56 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Thu, Jun 10, 2021 at 07:39:02PM +0300, Ioana Ciornei wrote: > > From: Ioana Ciornei <ioana.ciornei@nxp.com> > > > > This patch set provides ACPI support to DPAA2 network drivers. > > Just to be clear and avoid confusion, there is a standing NACK against > this patchset. Please see the discussion here: > > https://patchwork.kernel.org/project/linux-acpi/patch/20200715090400.4733-2-calvin.johnson@oss.nxp.com/#23518385 > > So far, i've not seen any indication the issues raised there have been > resolved. I don't see any Acked-by from an ACPI maintainer. So this > code remains NACKed. Andrew, The ACPI maintainers did bless the use of the ACPI standards followed in this patchset, and their only abstinence from ACK'ing the patchset was whether the code was used in production systems. Well currently, not only have we, SolidRun, been using this patchset and the associated ACPI tables in our SystemsReady certified firmware for the HoneyComb, but we also have customers using this same patchset and firmware on their systems rolled out to customers. Additionally we have an entire new product line based on Marvell's Armada CN913x series, which also needs this patchset to be fully functional. I am quite certain this is more than enough production systems using this ACPI description method for networking to progress this patchset forward. -Jon
On Thu, Jun 10, 2021 at 6:40 PM Ioana Ciornei <ciorneiioana@gmail.com> wrote: > > From: Calvin Johnson <calvin.johnson@oss.nxp.com> > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > provide them to be connected to MAC. This is not an "ACPI mechanism", because it is not part of the ACPI specification or support documentation thereof. I would call it "a mechanism based on generic ACPI _DSD device properties definition []1]". And provide a reference to the _DSD properties definition document. With that changed, you can add Acked-by: Rafael J. Wysocki <rafael@kernel.org> to this patch. Note, however, that within the traditional ACPI framework, the _DSD properties are consumed by the driver that binds to the device represented by the ACPI device object containing the _DSD in question in its scope, while in this case IIUC the properties are expected to be consumed by the general networking code in the kernel. That is not wrong in principle, but it means that operating systems other than Linux are not likely to be using them. > Describe properties "phy-handle" and "phy-mode". > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- > > Changes in v8: None > Changes in v7: None > Changes in v6: > - Minor cleanup > > Changes in v5: > - More cleanup > > Changes in v4: > - More cleanup > > Changes in v3: None > Changes in v2: > - Updated with more description in document > > Documentation/firmware-guide/acpi/dsd/phy.rst | 133 ++++++++++++++++++ > 1 file changed, 133 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..7d01ae8b3cc6 > --- /dev/null > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > @@ -0,0 +1,133 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +========================= > +MDIO bus and PHYs in ACPI > +========================= > + > +The PHYs on an MDIO bus [1] are probed and registered using > +fwnode_mdiobus_register_phy(). > + > +Later, for connecting these PHYs to their respective MACs, the PHYs registered > +on the MDIO bus have to be referenced. > + > +This document introduces two _DSD properties that are to be used > +for connecting PHYs on the MDIO bus [3] to the MAC layer. > + > +These properties are defined in accordance with the "Device > +Properties UUID For _DSD" [2] document and the > +daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device > +Data Descriptors containing them. > + > +phy-handle > +---------- > +For each MAC node, a device property "phy-handle" is used to reference > +the PHY that is registered on an MDIO bus. This is mandatory for > +network interfaces that have PHYs connected to MAC via MDIO bus. > + > +During the MDIO bus driver initialization, PHYs on this bus are probed > +using the _ADR object as shown below and are registered on the MDIO bus. > + > +:: > + Scope(\_SB.MDI0) > + { > + Device(PHY1) { > + Name (_ADR, 0x1) > + } // end of PHY1 > + > + Device(PHY2) { > + Name (_ADR, 0x2) > + } // end of PHY2 > + } > + > +Later, during the MAC driver initialization, the registered PHY devices > +have to be retrieved from the MDIO bus. For this, the MAC driver needs > +references to the previously registered PHYs which are provided > +as device object references (e.g. \_SB.MDI0.PHY1). > + > +phy-mode > +-------- > +The "phy-mode" _DSD property is used to describe the connection to > +the PHY. The valid values for "phy-mode" are defined in [4]. > + > +The following ASL example illustrates the usage of these properties. > + > +DSDT entry for MDIO node > +------------------------ > + > +The MDIO bus has an SoC component (MDIO controller) and a platform > +component (PHYs on the MDIO bus). > + > +a) Silicon Component > +This node describes the MDIO controller, MDI0 > +--------------------------------------------- > +:: > + 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 > +The PHY1 and PHY2 nodes represent the PHYs connected to MDIO bus MDI0 > +--------------------------------------------------------------------- > +:: > + Scope(\_SB.MDI0) > + { > + Device(PHY1) { > + Name (_ADR, 0x1) > + } // end of PHY1 > + > + Device(PHY2) { > + Name (_ADR, 0x2) > + } // end of PHY2 > + } > + > +DSDT entries representing MAC nodes > +----------------------------------- > + > +Below are the MAC nodes where PHY nodes are referenced. > +phy-mode and phy-handle are used as explained earlier. > +------------------------------------------------------ > +:: > + Scope(\_SB.MCE0.PR17) > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package (2) {"phy-mode", "rgmii-id"}, > + Package (2) {"phy-handle", \_SB.MDI0.PHY1} > + } > + }) > + } > + > + Scope(\_SB.MCE0.PR18) > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package (2) {"phy-mode", "rgmii-id"}, > + Package (2) {"phy-handle", \_SB.MDI0.PHY2}} > + } > + }) > + } > + > +References > +========== > + > +[1] Documentation/networking/phy.rst > + > +[2] https://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf > + > +[3] Documentation/firmware-guide/acpi/DSD-properties-rules.rst > + > +[4] Documentation/devicetree/bindings/net/ethernet-controller.yaml > -- > 2.31.1 >
On Thu, Jun 10, 2021 at 6:40 PM Ioana Ciornei <ciorneiioana@gmail.com> wrote: > > From: Calvin Johnson <calvin.johnson@oss.nxp.com> > > Introduce a wrapper around the _ADR evaluation. > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> Acked-by: Rafael J. Wysocki <rafael@kernel.org> > --- > > Changes in v8: None > Changes in v7: None > Changes in v6: None > Changes in v5: > - Replace fwnode_get_id() with acpi_get_local_address() > > Changes in v4: > - Improve code structure to handle all cases > > Changes in v3: > - Modified to retrieve reg property value for ACPI as well > - Resolved compilation issue with CONFIG_ACPI = n > - Added more info into documentation > > drivers/acpi/utils.c | 14 ++++++++++++++ > include/linux/acpi.h | 7 +++++++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 3b54b8fd7396..e7ddd281afff 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -277,6 +277,20 @@ acpi_evaluate_integer(acpi_handle handle, > > EXPORT_SYMBOL(acpi_evaluate_integer); > > +int acpi_get_local_address(acpi_handle handle, u32 *addr) > +{ > + unsigned long long adr; > + acpi_status status; > + > + status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr); > + if (ACPI_FAILURE(status)) > + return -ENODATA; > + > + *addr = (u32)adr; > + return 0; > +} > +EXPORT_SYMBOL(acpi_get_local_address); > + > acpi_status > acpi_evaluate_reference(acpi_handle handle, > acpi_string pathname, > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index c60745f657e9..6ace3a0f1415 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -710,6 +710,8 @@ static inline u64 acpi_arch_get_root_pointer(void) > } > #endif > > +int acpi_get_local_address(acpi_handle handle, u32 *addr); > + > #else /* !CONFIG_ACPI */ > > #define acpi_disabled 1 > @@ -965,6 +967,11 @@ static inline struct acpi_device *acpi_resource_consumer(struct resource *res) > return NULL; > } > > +static inline int acpi_get_local_address(acpi_handle handle, u32 *addr) > +{ > + return -ENODEV; > +} > + > #endif /* !CONFIG_ACPI */ > > #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC > -- > 2.31.1 >
On 10/06/2021 17:39, Ioana Ciornei wrote: > From: Calvin Johnson <calvin.johnson@oss.nxp.com> > > 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@oss.nxp.com> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> Looks reasonable to me. I'm not a kernel maintainer any more, so my Acked-by: may not be very valuable, but here it is anyway: Acked-by: Grant Likely <grant.likely@arm.com> > --- > > Changes in v8: None > Changes in v7: None > Changes in v6: > - Minor cleanup > > Changes in v5: > - More cleanup > > Changes in v4: > - More cleanup > > Changes in v3: None > Changes in v2: > - Updated with more description in document > > Documentation/firmware-guide/acpi/dsd/phy.rst | 133 ++++++++++++++++++ > 1 file changed, 133 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..7d01ae8b3cc6 > --- /dev/null > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > @@ -0,0 +1,133 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +========================= > +MDIO bus and PHYs in ACPI > +========================= > + > +The PHYs on an MDIO bus [1] are probed and registered using > +fwnode_mdiobus_register_phy(). > + > +Later, for connecting these PHYs to their respective MACs, the PHYs registered > +on the MDIO bus have to be referenced. > + > +This document introduces two _DSD properties that are to be used > +for connecting PHYs on the MDIO bus [3] to the MAC layer. > + > +These properties are defined in accordance with the "Device > +Properties UUID For _DSD" [2] document and the > +daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device > +Data Descriptors containing them. > + > +phy-handle > +---------- > +For each MAC node, a device property "phy-handle" is used to reference > +the PHY that is registered on an MDIO bus. This is mandatory for > +network interfaces that have PHYs connected to MAC via MDIO bus. > + > +During the MDIO bus driver initialization, PHYs on this bus are probed > +using the _ADR object as shown below and are registered on the MDIO bus. > + > +:: > + Scope(\_SB.MDI0) > + { > + Device(PHY1) { > + Name (_ADR, 0x1) > + } // end of PHY1 > + > + Device(PHY2) { > + Name (_ADR, 0x2) > + } // end of PHY2 > + } > + > +Later, during the MAC driver initialization, the registered PHY devices > +have to be retrieved from the MDIO bus. For this, the MAC driver needs > +references to the previously registered PHYs which are provided > +as device object references (e.g. \_SB.MDI0.PHY1). > + > +phy-mode > +-------- > +The "phy-mode" _DSD property is used to describe the connection to > +the PHY. The valid values for "phy-mode" are defined in [4]. > + > +The following ASL example illustrates the usage of these properties. > + > +DSDT entry for MDIO node > +------------------------ > + > +The MDIO bus has an SoC component (MDIO controller) and a platform > +component (PHYs on the MDIO bus). > + > +a) Silicon Component > +This node describes the MDIO controller, MDI0 > +--------------------------------------------- > +:: > + 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 > +The PHY1 and PHY2 nodes represent the PHYs connected to MDIO bus MDI0 > +--------------------------------------------------------------------- > +:: > + Scope(\_SB.MDI0) > + { > + Device(PHY1) { > + Name (_ADR, 0x1) > + } // end of PHY1 > + > + Device(PHY2) { > + Name (_ADR, 0x2) > + } // end of PHY2 > + } > + > +DSDT entries representing MAC nodes > +----------------------------------- > + > +Below are the MAC nodes where PHY nodes are referenced. > +phy-mode and phy-handle are used as explained earlier. > +------------------------------------------------------ > +:: > + Scope(\_SB.MCE0.PR17) > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package (2) {"phy-mode", "rgmii-id"}, > + Package (2) {"phy-handle", \_SB.MDI0.PHY1} > + } > + }) > + } > + > + Scope(\_SB.MCE0.PR18) > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package (2) {"phy-mode", "rgmii-id"}, > + Package (2) {"phy-handle", \_SB.MDI0.PHY2}} > + } > + }) > + } > + > +References > +========== > + > +[1] Documentation/networking/phy.rst > + > +[2] https://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf > + > +[3] Documentation/firmware-guide/acpi/DSD-properties-rules.rst > + > +[4] Documentation/devicetree/bindings/net/ethernet-controller.yaml >
On 10/06/2021 17:39, Ioana Ciornei wrote: > From: Ioana Ciornei <ioana.ciornei@nxp.com> > > This patch set provides ACPI support to DPAA2 network drivers. > > It also introduces new fwnode based APIs to support phylink and phy > layers > Following functions are defined: > phylink_fwnode_phy_connect() > fwnode_mdiobus_register_phy() > fwnode_get_phy_id() > fwnode_phy_find_device() > device_phy_find_device() > fwnode_get_phy_node() > fwnode_mdio_find_device() > acpi_get_local_address() > > First one helps in connecting phy to phylink instance. > Next three helps in getting phy_id and registering phy to mdiobus > Next two help in finding a phy on a mdiobus. > Next one helps in getting phy_node from a fwnode. > Last one is used to get local address from _ADR object. > > Corresponding OF functions are refactored. In terms of design, this whole series looks right to me. The way data is encoded in ACPI is entirely appropriate. As long as Rob Herring is okay with the DT code changes I support this series being merged. For the whole series: Acked-by: Grant Likely <grant.likely@arm.com> > > Tested-on: LX2160ARDB > > Changes in v8: > - fixed some checkpatch warnings/checks > - included linux/fwnode_mdio.h in fwnode_mdio.c (fixed the build warnings) > - added fwnode_find_mii_timestamper() and > fwnode_mdiobus_phy_device_register() in order to get rid of the cycle > dependency. > - change to 'depends on (ACPI || OF) || COMPILE_TEST (for FWNODE_MDIO) > - remove the fwnode_mdiobus_register from fwnode_mdio.c since it > introduces a cycle of dependencies. > > Changes in v7: > - correct fwnode_mdio_find_device() description > - check NULL in unregister_mii_timestamper() > - Call unregister_mii_timestamper() without NULL check > - Create fwnode_mdio.c and move fwnode_mdiobus_register_phy() > - include fwnode_mdio.h > - Include headers directly used in acpi_mdio.c > - Move fwnode_mdiobus_register() to fwnode_mdio.c > - Include fwnode_mdio.h > - Alphabetically sort header inclusions > - remove unnecassary checks > > Changes in v6: > - Minor cleanup > - fix warning for function parameter of fwnode_mdio_find_device() > - Initialize mii_ts to NULL > - use GENMASK() and ACPI_COMPANION_SET() > - some cleanup > - remove unwanted header inclusion > - remove OF check for fixed-link > - use dev_fwnode() > - remove useless else > - replace of_device_is_available() to fwnode_device_is_available() > > Changes in v5: > - More cleanup > - Replace fwnode_get_id() with acpi_get_local_address() > - add missing MODULE_LICENSE() > - replace fwnode_get_id() with OF and ACPI function calls > - replace fwnode_get_id() with OF and ACPI function calls > > Changes in v4: > - More cleanup > - Improve code structure to handle all cases > - Remove redundant else from fwnode_mdiobus_register() > - Cleanup xgmac_mdio_probe() > - call phy_device_free() before returning > > Changes in v3: > - Add more info on legacy DT properties "phy" and "phy-device" > - Redefine fwnode_phy_find_device() to follow of_phy_find_device() > - Use traditional comparison pattern > - Use GENMASK > - Modified to retrieve reg property value for ACPI as well > - Resolved compilation issue with CONFIG_ACPI = n > - Added more info into documentation > - Use acpi_mdiobus_register() > - Avoid unnecessary line removal > - Remove unused inclusion of acpi.h > > Changes in v2: > - Updated with more description in document > - use reverse christmas tree ordering for local variables > - Refactor OF functions to use fwnode functions > > Calvin Johnson (15): > Documentation: ACPI: DSD: Document MDIO PHY > net: phy: Introduce fwnode_mdio_find_device() > net: phy: Introduce phy related fwnode functions > of: mdio: Refactor of_phy_find_device() > net: phy: Introduce fwnode_get_phy_id() > of: mdio: Refactor of_get_phy_id() > net: mii_timestamper: check NULL in unregister_mii_timestamper() > net: mdiobus: Introduce fwnode_mdiobus_register_phy() > of: mdio: Refactor of_mdiobus_register_phy() > ACPI: utils: Introduce acpi_get_local_address() > net: mdio: Add ACPI support code for mdio > net/fsl: Use [acpi|of]_mdiobus_register > net: phylink: introduce phylink_fwnode_phy_connect() > net: phylink: Refactor phylink_of_phy_connect() > net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver > > Documentation/firmware-guide/acpi/dsd/phy.rst | 133 ++++++++++++++++ > MAINTAINERS | 2 + > drivers/acpi/utils.c | 14 ++ > .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 88 ++++++----- > .../net/ethernet/freescale/dpaa2/dpaa2-mac.h | 2 +- > drivers/net/ethernet/freescale/xgmac_mdio.c | 30 ++-- > drivers/net/mdio/Kconfig | 14 ++ > drivers/net/mdio/Makefile | 4 +- > drivers/net/mdio/acpi_mdio.c | 56 +++++++ > drivers/net/mdio/fwnode_mdio.c | 144 ++++++++++++++++++ > drivers/net/mdio/of_mdio.c | 138 ++--------------- > drivers/net/phy/mii_timestamper.c | 3 + > drivers/net/phy/phy_device.c | 109 ++++++++++++- > drivers/net/phy/phylink.c | 41 +++-- > include/linux/acpi.h | 7 + > include/linux/acpi_mdio.h | 26 ++++ > include/linux/fwnode_mdio.h | 35 +++++ > include/linux/phy.h | 32 ++++ > include/linux/phylink.h | 3 + > 19 files changed, 691 insertions(+), 190 deletions(-) > create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst > create mode 100644 drivers/net/mdio/acpi_mdio.c > create mode 100644 drivers/net/mdio/fwnode_mdio.c > create mode 100644 include/linux/acpi_mdio.h > create mode 100644 include/linux/fwnode_mdio.h >
On 10/06/2021 19:05, Rafael J. Wysocki wrote: > On Thu, Jun 10, 2021 at 6:40 PM Ioana Ciornei <ciorneiioana@gmail.com> wrote: >> >> From: Calvin Johnson <calvin.johnson@oss.nxp.com> >> >> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and >> provide them to be connected to MAC. > > This is not an "ACPI mechanism", because it is not part of the ACPI > specification or support documentation thereof. > > I would call it "a mechanism based on generic ACPI _DSD device > properties definition []1]". And provide a reference to the _DSD > properties definition document. > > With that changed, you can add > > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > > to this patch. > > Note, however, that within the traditional ACPI framework, the _DSD > properties are consumed by the driver that binds to the device > represented by the ACPI device object containing the _DSD in question > in its scope, while in this case IIUC the properties are expected to > be consumed by the general networking code in the kernel. That is not > wrong in principle, but it means that operating systems other than > Linux are not likely to be using them. > Doesn't this land at the level of device drivers though? None of this data needs to be consumed by the OS generic ACPI parsing code, but the network device driver can use it to parse the MDIO and MAC configuraiton and set itself up appropriately. The only difference in the Linux case is that the code is implemented in a way that can be leveraged by other network drivers instead of being entirely contained within the dpaa driver. g. >> Describe properties "phy-handle" and "phy-mode". >> >> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> >> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> >> --- >> >> Changes in v8: None >> Changes in v7: None >> Changes in v6: >> - Minor cleanup >> >> Changes in v5: >> - More cleanup >> >> Changes in v4: >> - More cleanup >> >> Changes in v3: None >> Changes in v2: >> - Updated with more description in document >> >> Documentation/firmware-guide/acpi/dsd/phy.rst | 133 ++++++++++++++++++ >> 1 file changed, 133 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..7d01ae8b3cc6 >> --- /dev/null >> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst >> @@ -0,0 +1,133 @@ >> +.. SPDX-License-Identifier: GPL-2.0 >> + >> +========================= >> +MDIO bus and PHYs in ACPI >> +========================= >> + >> +The PHYs on an MDIO bus [1] are probed and registered using >> +fwnode_mdiobus_register_phy(). >> + >> +Later, for connecting these PHYs to their respective MACs, the PHYs registered >> +on the MDIO bus have to be referenced. >> + >> +This document introduces two _DSD properties that are to be used >> +for connecting PHYs on the MDIO bus [3] to the MAC layer. >> + >> +These properties are defined in accordance with the "Device >> +Properties UUID For _DSD" [2] document and the >> +daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device >> +Data Descriptors containing them. >> + >> +phy-handle >> +---------- >> +For each MAC node, a device property "phy-handle" is used to reference >> +the PHY that is registered on an MDIO bus. This is mandatory for >> +network interfaces that have PHYs connected to MAC via MDIO bus. >> + >> +During the MDIO bus driver initialization, PHYs on this bus are probed >> +using the _ADR object as shown below and are registered on the MDIO bus. >> + >> +:: >> + Scope(\_SB.MDI0) >> + { >> + Device(PHY1) { >> + Name (_ADR, 0x1) >> + } // end of PHY1 >> + >> + Device(PHY2) { >> + Name (_ADR, 0x2) >> + } // end of PHY2 >> + } >> + >> +Later, during the MAC driver initialization, the registered PHY devices >> +have to be retrieved from the MDIO bus. For this, the MAC driver needs >> +references to the previously registered PHYs which are provided >> +as device object references (e.g. \_SB.MDI0.PHY1). >> + >> +phy-mode >> +-------- >> +The "phy-mode" _DSD property is used to describe the connection to >> +the PHY. The valid values for "phy-mode" are defined in [4]. >> + >> +The following ASL example illustrates the usage of these properties. >> + >> +DSDT entry for MDIO node >> +------------------------ >> + >> +The MDIO bus has an SoC component (MDIO controller) and a platform >> +component (PHYs on the MDIO bus). >> + >> +a) Silicon Component >> +This node describes the MDIO controller, MDI0 >> +--------------------------------------------- >> +:: >> + 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 >> +The PHY1 and PHY2 nodes represent the PHYs connected to MDIO bus MDI0 >> +--------------------------------------------------------------------- >> +:: >> + Scope(\_SB.MDI0) >> + { >> + Device(PHY1) { >> + Name (_ADR, 0x1) >> + } // end of PHY1 >> + >> + Device(PHY2) { >> + Name (_ADR, 0x2) >> + } // end of PHY2 >> + } >> + >> +DSDT entries representing MAC nodes >> +----------------------------------- >> + >> +Below are the MAC nodes where PHY nodes are referenced. >> +phy-mode and phy-handle are used as explained earlier. >> +------------------------------------------------------ >> +:: >> + Scope(\_SB.MCE0.PR17) >> + { >> + Name (_DSD, Package () { >> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >> + Package () { >> + Package (2) {"phy-mode", "rgmii-id"}, >> + Package (2) {"phy-handle", \_SB.MDI0.PHY1} >> + } >> + }) >> + } >> + >> + Scope(\_SB.MCE0.PR18) >> + { >> + Name (_DSD, Package () { >> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >> + Package () { >> + Package (2) {"phy-mode", "rgmii-id"}, >> + Package (2) {"phy-handle", \_SB.MDI0.PHY2}} >> + } >> + }) >> + } >> + >> +References >> +========== >> + >> +[1] Documentation/networking/phy.rst >> + >> +[2] https://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf >> + >> +[3] Documentation/firmware-guide/acpi/DSD-properties-rules.rst >> + >> +[4] Documentation/devicetree/bindings/net/ethernet-controller.yaml >> -- >> 2.31.1 >>
On 10/06/2021 19:25, Rafael J. Wysocki wrote: > On Thu, Jun 10, 2021 at 7:51 PM Jon Nettleton <jon@solid-run.com> wrote: >> >> On Thu, Jun 10, 2021 at 6:56 PM Andrew Lunn <andrew@lunn.ch> wrote: >>> >>> On Thu, Jun 10, 2021 at 07:39:02PM +0300, Ioana Ciornei wrote: >>>> From: Ioana Ciornei <ioana.ciornei@nxp.com> >>>> >>>> This patch set provides ACPI support to DPAA2 network drivers. >>> >>> Just to be clear and avoid confusion, there is a standing NACK against >>> this patchset. Please see the discussion here: >>> >>> https://patchwork.kernel.org/project/linux-acpi/patch/20200715090400.4733-2-calvin.johnson@oss.nxp.com/#23518385 >>> >>> So far, i've not seen any indication the issues raised there have been >>> resolved. I don't see any Acked-by from an ACPI maintainer. So this >>> code remains NACKed. >> >> Andrew, >> >> The ACPI maintainers did bless the use of the ACPI standards followed >> in this patchset, and their only abstinence from ACK'ing the patchset >> was whether the code was used in production systems. Well currently, >> not only have we, SolidRun, been using this patchset and the associated >> ACPI tables in our SystemsReady certified firmware for the HoneyComb, >> but we also have customers using this same patchset and firmware on >> their systems rolled out to customers. >> >> Additionally we have an entire new product line based on Marvell's >> Armada CN913x series, which also needs this patchset to be fully >> functional. >> >> I am quite certain this is more than enough production systems using >> this ACPI description method for networking to progress this patchset >> forward. > > And I believe that you have all of the requisite ACKs from the ACPI > side now, so it is up to the networking guys to decide what to do > next. You've also my ACK as emeritus Devicetree maintainer. It is well past time for this series to get merged. g.
> And I believe that you have all of the requisite ACKs from the ACPI > side now, so it is up to the networking guys to decide what to do > next. Thanks for the Acked-by's. Since they were missing, the networking guys have deliberately been ignoring this code. Now they have been given, we will start the review work. Thanks Andrew
On Thu, Jun 10, 2021 at 7:40 PM Ioana Ciornei <ciorneiioana@gmail.com> wrote: > > From: Ioana Ciornei <ioana.ciornei@nxp.com> > > This patch set provides ACPI support to DPAA2 network drivers. > > It also introduces new fwnode based APIs to support phylink and phy > layers > Following functions are defined: > phylink_fwnode_phy_connect() > fwnode_mdiobus_register_phy() > fwnode_get_phy_id() > fwnode_phy_find_device() > device_phy_find_device() > fwnode_get_phy_node() > fwnode_mdio_find_device() > acpi_get_local_address() > > First one helps in connecting phy to phylink instance. > Next three helps in getting phy_id and registering phy to mdiobus > Next two help in finding a phy on a mdiobus. > Next one helps in getting phy_node from a fwnode. > Last one is used to get local address from _ADR object. > > Corresponding OF functions are refactored. In general it looks fine to me. What really worries me is the calls like of_foo -> fwnode_bar -> of_baz. As I have commented in one patch the idea of fwnode APIs is to have a common ground for all resource providers. So, at the end it shouldn't be a chain of calls like above mentioned. Either fix the name (so, the first one will be in fwnode or device namespace) or fix the API that it will be fwnode/device API.
From: Ioana Ciornei <ioana.ciornei@nxp.com> This patch set provides ACPI support to DPAA2 network drivers. It also introduces new fwnode based APIs to support phylink and phy layers Following functions are defined: phylink_fwnode_phy_connect() fwnode_mdiobus_register_phy() fwnode_get_phy_id() fwnode_phy_find_device() device_phy_find_device() fwnode_get_phy_node() fwnode_mdio_find_device() acpi_get_local_address() First one helps in connecting phy to phylink instance. Next three helps in getting phy_id and registering phy to mdiobus Next two help in finding a phy on a mdiobus. Next one helps in getting phy_node from a fwnode. Last one is used to get local address from _ADR object. Corresponding OF functions are refactored. Tested-on: LX2160ARDB Changes in v8: - fixed some checkpatch warnings/checks - included linux/fwnode_mdio.h in fwnode_mdio.c (fixed the build warnings) - added fwnode_find_mii_timestamper() and fwnode_mdiobus_phy_device_register() in order to get rid of the cycle dependency. - change to 'depends on (ACPI || OF) || COMPILE_TEST (for FWNODE_MDIO) - remove the fwnode_mdiobus_register from fwnode_mdio.c since it introduces a cycle of dependencies. Changes in v7: - correct fwnode_mdio_find_device() description - check NULL in unregister_mii_timestamper() - Call unregister_mii_timestamper() without NULL check - Create fwnode_mdio.c and move fwnode_mdiobus_register_phy() - include fwnode_mdio.h - Include headers directly used in acpi_mdio.c - Move fwnode_mdiobus_register() to fwnode_mdio.c - Include fwnode_mdio.h - Alphabetically sort header inclusions - remove unnecassary checks Changes in v6: - Minor cleanup - fix warning for function parameter of fwnode_mdio_find_device() - Initialize mii_ts to NULL - use GENMASK() and ACPI_COMPANION_SET() - some cleanup - remove unwanted header inclusion - remove OF check for fixed-link - use dev_fwnode() - remove useless else - replace of_device_is_available() to fwnode_device_is_available() Changes in v5: - More cleanup - Replace fwnode_get_id() with acpi_get_local_address() - add missing MODULE_LICENSE() - replace fwnode_get_id() with OF and ACPI function calls - replace fwnode_get_id() with OF and ACPI function calls Changes in v4: - More cleanup - Improve code structure to handle all cases - Remove redundant else from fwnode_mdiobus_register() - Cleanup xgmac_mdio_probe() - call phy_device_free() before returning Changes in v3: - Add more info on legacy DT properties "phy" and "phy-device" - Redefine fwnode_phy_find_device() to follow of_phy_find_device() - Use traditional comparison pattern - Use GENMASK - Modified to retrieve reg property value for ACPI as well - Resolved compilation issue with CONFIG_ACPI = n - Added more info into documentation - Use acpi_mdiobus_register() - Avoid unnecessary line removal - Remove unused inclusion of acpi.h Changes in v2: - Updated with more description in document - use reverse christmas tree ordering for local variables - Refactor OF functions to use fwnode functions Calvin Johnson (15): Documentation: ACPI: DSD: Document MDIO PHY net: phy: Introduce fwnode_mdio_find_device() net: phy: Introduce phy related fwnode functions of: mdio: Refactor of_phy_find_device() net: phy: Introduce fwnode_get_phy_id() of: mdio: Refactor of_get_phy_id() net: mii_timestamper: check NULL in unregister_mii_timestamper() net: mdiobus: Introduce fwnode_mdiobus_register_phy() of: mdio: Refactor of_mdiobus_register_phy() ACPI: utils: Introduce acpi_get_local_address() net: mdio: Add ACPI support code for mdio net/fsl: Use [acpi|of]_mdiobus_register net: phylink: introduce phylink_fwnode_phy_connect() net: phylink: Refactor phylink_of_phy_connect() net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver Documentation/firmware-guide/acpi/dsd/phy.rst | 133 ++++++++++++++++ MAINTAINERS | 2 + drivers/acpi/utils.c | 14 ++ .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 88 ++++++----- .../net/ethernet/freescale/dpaa2/dpaa2-mac.h | 2 +- drivers/net/ethernet/freescale/xgmac_mdio.c | 30 ++-- drivers/net/mdio/Kconfig | 14 ++ drivers/net/mdio/Makefile | 4 +- drivers/net/mdio/acpi_mdio.c | 56 +++++++ drivers/net/mdio/fwnode_mdio.c | 144 ++++++++++++++++++ drivers/net/mdio/of_mdio.c | 138 ++--------------- drivers/net/phy/mii_timestamper.c | 3 + drivers/net/phy/phy_device.c | 109 ++++++++++++- drivers/net/phy/phylink.c | 41 +++-- include/linux/acpi.h | 7 + include/linux/acpi_mdio.h | 26 ++++ include/linux/fwnode_mdio.h | 35 +++++ include/linux/phy.h | 32 ++++ include/linux/phylink.h | 3 + 19 files changed, 691 insertions(+), 190 deletions(-) create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst create mode 100644 drivers/net/mdio/acpi_mdio.c create mode 100644 drivers/net/mdio/fwnode_mdio.c create mode 100644 include/linux/acpi_mdio.h create mode 100644 include/linux/fwnode_mdio.h