[net-next PATCH v7 0/6] ACPI support for dpaa2 MAC driver.

[net-next PATCH v7 0/6] ACPI support for dpaa2 MAC driver.

[Calvin Johnson]

 This patch series provides ACPI support for dpaa2 MAC driver.
 This also introduces ACPI mechanism to get PHYs registered on a
 MDIO bus and provide them to be connected to MAC.

 Patch "net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver" depends on
 https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git/commit/?h=acpi/for-next&id=c279c4cf5bcd3c55b4fb9709d9036cd1bfe3beb8
 Remaining patches are independent of the above patch and can be applied without
 any issues.

 Device Tree can be tested on LX2160A-RDB with the below change which is also
available in the above referenced patches:

--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -931,6 +931,7 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
        if (error < 0)
                goto error_cleanup_mc_io;

+       mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
        mc->root_mc_bus_dev = mc_bus_dev;
        return 0;


Changes in v7:
- remove unnecessary -ve check for u32 var
- assign flags to phy_dev

Changes in v6:
- change device_mdiobus_register() parameter position
- improve documentation
- change device_mdiobus_register() parameter position
- clean up phylink_fwnode_phy_connect()

Changes in v5:
- add description
- clean up if else
- rename phy_find_by_fwnode() to phy_find_by_mdio_handle()
- add docment for phy_find_by_mdio_handle()
- error out DT in phy_find_by_mdio_handle()
- clean up err return
- return -EINVAL for invalid fwnode

Changes in v4:
- release fwnode_mdio after use
- return ERR_PTR instead of NULL
- introduce device_mdiobus_register()

Changes in v3:
- cleanup based on v2 comments
- Added description for more properties
- Added MDIO node DSDT entry
- introduce fwnode_mdio_find_bus()
- renamed and improved phy_find_by_fwnode()
- cleanup based on v2 comments
- move code into phylink_fwnode_phy_connect()

Changes in v2:
- clean up dpaa2_mac_get_node()
- introduce find_phy_device()
- use acpi_find_child_device()

Calvin Johnson (6):
  Documentation: ACPI: DSD: Document MDIO PHY
  net: phy: introduce device_mdiobus_register()
  net/fsl: use device_mdiobus_register()
  net: phy: introduce phy_find_by_mdio_handle()
  phylink: introduce phylink_fwnode_phy_connect()
  net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

 Documentation/firmware-guide/acpi/dsd/phy.rst | 90 +++++++++++++++++++
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 70 ++++++++-------
 drivers/net/ethernet/freescale/xgmac_mdio.c   |  3 +-
 drivers/net/phy/mdio_bus.c                    | 51 +++++++++++
 drivers/net/phy/phy_device.c                  | 40 +++++++++
 drivers/net/phy/phylink.c                     | 32 +++++++
 include/linux/mdio.h                          |  1 +
 include/linux/phy.h                           |  2 +
 include/linux/phylink.h                       |  3 +
 9 files changed, 260 insertions(+), 32 deletions(-)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst

-- 
2.17.1

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

[Calvin Johnson]

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@oss.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.
+
+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},
+	      }
+	    })
+	  } // 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
+	}
-- 
2.17.1

[net-next PATCH v7 2/6] net: phy: introduce device_mdiobus_register()

[Calvin Johnson]

Introduce device_mdiobus_register() to register mdiobus
in cases of either DT or ACPI.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

---

Changes in v7: None
Changes in v6:
- change device_mdiobus_register() parameter position
- improve documentation

Changes in v5:
- add description
- clean up if else

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
 include/linux/mdio.h       |  1 +
 2 files changed, 27 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 46b33701ad4b..8610f938f81f 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -501,6 +501,32 @@ static int mdiobus_create_device(struct mii_bus *bus,
 	return ret;
 }
 
+/**
+ * device_mdiobus_register - register mdiobus for either DT or ACPI
+ * @bus: target mii_bus
+ * @dev: given MDIO device
+ *
+ * Description: Given an MDIO device and target mii bus, this function
+ * calls of_mdiobus_register() for DT node and mdiobus_register() in
+ * case of ACPI.
+ *
+ * Returns 0 on success or negative error code on failure.
+ */
+int device_mdiobus_register(struct device *dev,
+			    struct mii_bus *bus)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+
+	if (is_of_node(fwnode))
+		return of_mdiobus_register(bus, to_of_node(fwnode));
+	if (fwnode) {
+		bus->dev.fwnode = fwnode;
+		return mdiobus_register(bus);
+	}
+	return -ENODEV;
+}
+EXPORT_SYMBOL(device_mdiobus_register);
+
 /**
  * __mdiobus_register - bring up all the PHYs on a given bus and attach them to bus
  * @bus: target mii_bus
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 898cbf00332a..f454c5435101 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -358,6 +358,7 @@ static inline int mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad,
 	return mdiobus_read(bus, prtad, mdiobus_c45_addr(devad, regnum));
 }
 
+int device_mdiobus_register(struct device *dev, struct mii_bus *bus);
 int mdiobus_register_device(struct mdio_device *mdiodev);
 int mdiobus_unregister_device(struct mdio_device *mdiodev);
 bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
-- 
2.17.1

[net-next PATCH v7 3/6] net/fsl: use device_mdiobus_register()

[Calvin Johnson]

Replace of_mdiobus_register() with device_mdiobus_register()
to take care of both DT and ACPI mdiobus_register.

Remove unused device_node pointer.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

---

Changes in v7: None
Changes in v6:
- change device_mdiobus_register() parameter position

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/net/ethernet/freescale/xgmac_mdio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 98be51d8b08c..704f2b166d0a 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -243,7 +243,6 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 
 static int xgmac_mdio_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
 	struct mii_bus *bus;
 	struct resource *res;
 	struct mdio_fsl_priv *priv;
@@ -285,7 +284,7 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	priv->has_a011043 = device_property_read_bool(&pdev->dev,
 						      "fsl,erratum-a011043");
 
-	ret = of_mdiobus_register(bus, np);
+	ret = device_mdiobus_register(&pdev->dev, bus);
 	if (ret) {
 		dev_err(&pdev->dev, "cannot register MDIO bus\n");
 		goto err_registration;
-- 
2.17.1

[net-next PATCH v7 4/6] net: phy: introduce phy_find_by_mdio_handle()

[Calvin Johnson]

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.

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 phy_find_by_fwnode() to get the PHY connected to the MAC.

Introduce fwnode_mdio_find_bus() to find the mii_bus that corresponds
to given mii_bus fwnode.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

---

Changes in v7:
- remove unnecessary -ve check for u32 var

Changes in v6: None
Changes in v5:
- rename phy_find_by_fwnode() to phy_find_by_mdio_handle()
- add docment for phy_find_by_mdio_handle()
- error out DT in phy_find_by_mdio_handle()
- clean up err return

Changes in v4:
- release fwnode_mdio after use
- return ERR_PTR instead of NULL

Changes in v3:
- introduce fwnode_mdio_find_bus()
- renamed and improved phy_find_by_fwnode()

Changes in v2: None

 drivers/net/phy/mdio_bus.c   | 25 ++++++++++++++++++++++
 drivers/net/phy/phy_device.c | 40 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  2 ++
 3 files changed, 67 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 8610f938f81f..d9597c5b55ae 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -435,6 +435,31 @@ struct mii_bus *of_mdio_find_bus(struct device_node *mdio_bus_np)
 }
 EXPORT_SYMBOL(of_mdio_find_bus);
 
+/**
+ * fwnode_mdio_find_bus - Given an mii_bus fwnode, find the mii_bus.
+ * @mdio_bus_fwnode: fwnode of the mii_bus.
+ *
+ * Returns a reference to the mii_bus, or NULL if none found.  The
+ * embedded struct device will have its reference count incremented,
+ * and this must be put once the bus is finished with.
+ *
+ * Because the association of a fwnode and mii_bus is made via
+ * mdiobus_register(), the mii_bus cannot be found before it is
+ * registered with mdiobus_register().
+ *
+ */
+struct mii_bus *fwnode_mdio_find_bus(struct fwnode_handle *mdio_bus_fwnode)
+{
+	struct device *d;
+
+	if (!mdio_bus_fwnode)
+		return NULL;
+
+	d = class_find_device_by_fwnode(&mdio_bus_class, mdio_bus_fwnode);
+	return d ? to_mii_bus(d) : NULL;
+}
+EXPORT_SYMBOL(fwnode_mdio_find_bus);
+
 /* Walk the list of subnodes of a mdio bus and look for a node that
  * matches the mdio device's address with its 'reg' property. If
  * found, set the of_node pointer for the mdio device. This allows
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7cda95330aea..38d781720670 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -23,8 +23,10 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
+#include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/sfp.h>
 #include <linux/skbuff.h>
@@ -964,6 +966,44 @@ struct phy_device *phy_find_first(struct mii_bus *bus)
 }
 EXPORT_SYMBOL(phy_find_first);
 
+/**
+ * phy_find_by_mdio_handle - get phy device from mdio-handle and phy-channel
+ * @fwnode: a pointer to a &struct fwnode_handle  to get mdio-handle and
+ * phy-channel
+ *
+ * Find fwnode_mdio using mdio-handle reference. Using fwnode_mdio get the
+ * mdio bus. Property phy-channel provides the phy address on the mdio bus.
+ * Pass mdio bus and phy address to mdiobus_get_phy() and get corresponding
+ * phy_device. This method is used for ACPI and not for DT.
+ *
+ * Returns pointer to the phy device on success, else ERR_PTR.
+ */
+struct phy_device *phy_find_by_mdio_handle(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *fwnode_mdio;
+	struct mii_bus *mdio;
+	int addr;
+	int err;
+
+	if (is_of_node(fwnode))
+		return ERR_PTR(-EINVAL);
+
+	fwnode_mdio = fwnode_find_reference(fwnode, "mdio-handle", 0);
+	mdio = fwnode_mdio_find_bus(fwnode_mdio);
+	fwnode_handle_put(fwnode_mdio);
+	if (!mdio)
+		return ERR_PTR(-ENODEV);
+
+	err = fwnode_property_read_u32(fwnode, "phy-channel", &addr);
+	if (err)
+		return ERR_PTR(err);
+	if (addr >= PHY_MAX_ADDR)
+		return ERR_PTR(-EINVAL);
+
+	return mdiobus_get_phy(mdio, addr);
+}
+EXPORT_SYMBOL(phy_find_by_mdio_handle);
+
 static void phy_link_change(struct phy_device *phydev, bool up)
 {
 	struct net_device *netdev = phydev->attached_dev;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0403eb799913..fd383a22eb61 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -334,6 +334,7 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct device *dev)
 }
 
 struct mii_bus *mdio_find_bus(const char *mdio_name);
+struct mii_bus *fwnode_mdio_find_bus(struct fwnode_handle *mdio_bus_fwnode);
 struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
 
 #define PHY_INTERRUPT_DISABLED	false
@@ -1245,6 +1246,7 @@ int phy_sfp_probe(struct phy_device *phydev,
 struct phy_device *phy_attach(struct net_device *dev, const char *bus_id,
 			      phy_interface_t interface);
 struct phy_device *phy_find_first(struct mii_bus *bus);
+struct phy_device *phy_find_by_mdio_handle(struct fwnode_handle *fwnode);
 int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		      u32 flags, phy_interface_t interface);
 int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
-- 
2.17.1

[net-next PATCH v7 5/6] phylink: introduce phylink_fwnode_phy_connect()

[Calvin Johnson]

Define phylink_fwnode_phy_connect() to connect phy specified by
a fwnode to a phylink instance.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

---

Changes in v7:
- assign flags to phy_dev

Changes in v6:
- clean up phylink_fwnode_phy_connect()

Changes in v5:
- return -EINVAL for invalid fwnode

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/net/phy/phylink.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 35 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index dae6c8b51d7f..ba40d3bfa986 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -5,6 +5,7 @@
  *
  * Copyright (C) 2015 Russell King
  */
+#include <linux/acpi.h>
 #include <linux/ethtool.h>
 #include <linux/export.h>
 #include <linux/gpio/consumer.h>
@@ -1017,6 +1018,37 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
 }
 EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
 
+/**
+ * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode.
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @fwnode: a pointer to a &struct fwnode_handle.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
+ *
+ * Connect the phy specified @fwnode to the phylink instance specified
+ * by @pl. Actions specified in phylink_connect_phy() will be
+ * performed.
+ *
+ * Returns 0 on success or a negative errno.
+ */
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags)
+{
+	struct phy_device *phy_dev;
+
+	if (is_of_node(fwnode))
+		return phylink_of_phy_connect(pl, to_of_node(fwnode), flags);
+	if (is_acpi_device_node(fwnode)) {
+		phy_dev = phy_find_by_mdio_handle(fwnode);
+		if (!phy_dev)
+			return -ENODEV;
+		phy_dev->dev_flags |= flags;
+		return phylink_connect_phy(pl, phy_dev);
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);
+
 /**
  * phylink_disconnect_phy() - disconnect any PHY attached to the phylink
  *   instance.
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index b32b8b45421b..b27eed5e49a9 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -368,6 +368,9 @@ void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
 int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags);
 void phylink_disconnect_phy(struct phylink *);
 
 void phylink_mac_change(struct phylink *, bool up);
-- 
2.17.1

[net-next PATCH v7 6/6] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Calvin Johnson]

Modify dpaa2_mac_connect() to support ACPI along with DT.
Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
DT or ACPI.
Replace of_get_phy_mode with fwnode_get_phy_mode to get
phy-mode for a dpmac_node.
Define and use helper function find_phy_device() to find phy_dev
that is later connected to mac->phylink.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

---

Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4:
- introduce device_mdiobus_register()

Changes in v3:
- cleanup based on v2 comments
- move code into phylink_fwnode_phy_connect()

Changes in v2:
- clean up dpaa2_mac_get_node()
- introduce find_phy_device()
- use acpi_find_child_device()

 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 70 +++++++++++--------
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 3ee236c5fc37..297d2dab9e97 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
 /* Copyright 2019 NXP */
 
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+
 #include "dpaa2-eth.h"
 #include "dpaa2-mac.h"
 
@@ -23,38 +26,46 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
 }
 
 /* Caller must call of_node_put on the returned value */
-static struct device_node *dpaa2_mac_get_node(u16 dpmac_id)
+static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
+						u16 dpmac_id)
 {
-	struct device_node *dpmacs, *dpmac = NULL;
-	u32 id;
+	struct fwnode_handle *fsl_mc_fwnode = dev_fwnode(dev->parent->parent);
+	struct fwnode_handle *dpmacs, *dpmac = NULL;
+	struct acpi_device *adev;
 	int err;
+	u32 id;
 
-	dpmacs = of_find_node_by_name(NULL, "dpmacs");
-	if (!dpmacs)
-		return NULL;
-
-	while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
-		err = of_property_read_u32(dpmac, "reg", &id);
-		if (err)
-			continue;
-		if (id == dpmac_id)
-			break;
+	if (is_of_node(fsl_mc_fwnode)) {
+		dpmacs = fwnode_get_named_child_node(fsl_mc_fwnode, "dpmacs");
+		if (!dpmacs)
+			return NULL;
+
+		while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
+			err = fwnode_property_read_u32(dpmac, "reg", &id);
+			if (err)
+				continue;
+			if (id == dpmac_id)
+				return dpmac;
+		}
+		fwnode_handle_put(dpmacs);
+	} else if (is_acpi_device_node(fsl_mc_fwnode)) {
+		adev = acpi_find_child_device(ACPI_COMPANION(dev->parent),
+					      dpmac_id, false);
+		if (adev)
+			return acpi_fwnode_handle(adev);
 	}
-
-	of_node_put(dpmacs);
-
-	return dpmac;
+	return NULL;
 }
 
-static int dpaa2_mac_get_if_mode(struct device_node *node,
+static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node,
 				 struct dpmac_attr attr)
 {
 	phy_interface_t if_mode;
 	int err;
 
-	err = of_get_phy_mode(node, &if_mode);
-	if (!err)
-		return if_mode;
+	err = fwnode_get_phy_mode(dpmac_node);
+	if (err > 0)
+		return err;
 
 	err = phy_mode(attr.eth_if, &if_mode);
 	if (!err)
@@ -231,7 +242,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 {
 	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
 	struct net_device *net_dev = mac->net_dev;
-	struct device_node *dpmac_node;
+	struct fwnode_handle *dpmac_node = NULL;
 	struct phylink *phylink;
 	struct dpmac_attr attr;
 	int err;
@@ -251,7 +262,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 
 	mac->if_link_type = attr.link_type;
 
-	dpmac_node = dpaa2_mac_get_node(attr.id);
+	dpmac_node = dpaa2_mac_get_node(&dpmac_dev->dev, attr.id);
 	if (!dpmac_node) {
 		netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id);
 		err = -ENODEV;
@@ -269,7 +280,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	 * error out if the interface mode requests them and there is no PHY
 	 * to act upon them
 	 */
-	if (of_phy_is_fixed_link(dpmac_node) &&
+	if (of_phy_is_fixed_link(to_of_node(dpmac_node)) &&
 	    (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID ||
 	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
 	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) {
@@ -282,7 +293,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	mac->phylink_config.type = PHYLINK_NETDEV;
 
 	phylink = phylink_create(&mac->phylink_config,
-				 of_fwnode_handle(dpmac_node), mac->if_mode,
+				 dpmac_node, mac->if_mode,
 				 &dpaa2_mac_phylink_ops);
 	if (IS_ERR(phylink)) {
 		err = PTR_ERR(phylink);
@@ -290,20 +301,19 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	}
 	mac->phylink = phylink;
 
-	err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0);
+	err = phylink_fwnode_phy_connect(mac->phylink, dpmac_node, 0);
 	if (err) {
-		netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err);
+		netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err);
 		goto err_phylink_destroy;
 	}
 
-	of_node_put(dpmac_node);
-
+	fwnode_handle_put(dpmac_node);
 	return 0;
 
 err_phylink_destroy:
 	phylink_destroy(mac->phylink);
 err_put_node:
-	of_node_put(dpmac_node);
+	fwnode_handle_put(dpmac_node);
 err_close_dpmac:
 	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
 	return err;
-- 
2.17.1

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

[Florian Fainelli]

On 7/15/2020 2:03 AM, 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@oss.nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

You would probably want to submit an update to the PHY LIBRARY section
of the MAINTAINERS file to add this PHY DSD documentation, can be done
when this series gets merged.
-- 
Florian

Re: [net-next PATCH v7 2/6] net: phy: introduce device_mdiobus_register()

[Florian Fainelli]

On 7/15/2020 2:03 AM, Calvin Johnson wrote:
> Introduce device_mdiobus_register() to register mdiobus
> in cases of either DT or ACPI.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [net-next PATCH v7 3/6] net/fsl: use device_mdiobus_register()

[Florian Fainelli]

On 7/15/2020 2:03 AM, Calvin Johnson wrote:
> Replace of_mdiobus_register() with device_mdiobus_register()
> to take care of both DT and ACPI mdiobus_register.
> 
> Remove unused device_node pointer.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [net-next PATCH v7 4/6] net: phy: introduce phy_find_by_mdio_handle()

[Florian Fainelli]

On 7/15/2020 2:03 AM, Calvin Johnson wrote:
> 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.
> 
> 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 phy_find_by_fwnode() to get the PHY connected to the MAC.
> 
> Introduce fwnode_mdio_find_bus() to find the mii_bus that corresponds
> to given mii_bus fwnode.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Andrew Lunn]

On Wed, Jul 15, 2020 at 08:04:36PM -0700, Florian Fainelli wrote:
> 
> 
> On 7/15/2020 2:03 AM, 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@oss.nxp.com>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

I really would like to see an ACPI maintainer ACK this before it gets
merged. I'm not sure the current reviewers have deep enough knowledge
of ACPI to know if this is going against parts of the standard, or
philosophy of ACPI. And we are setting an ABI here, so we need to be
particularly careful.

	     Andrew

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

[Jeremy Linton]

Hi,

On 7/15/20 4:03 AM, 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@oss.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.

First, this is all perfectly compatible with my literal interpretation 
and understanding of the ACPI spec. The use of _DSD is there to provide 
a way to "extend" if you will the specification for device specific edge 
cases that aren't directly covered by the spec.

OTOH, it may be my lack of knowledge here, but IMHO this is a bit of a 
difficult pill for all arm/sbsa systems though. Primary because I don't 
see how one is expected to use the generic ACPI power states on the 
parent device here. I also have some questions about how one might 
import such a device into a VM. Further AFAIK arm's current 
recommendations for SBSA/ACPI systems point in the direction of RCiEP's.

IMHO what should be clarified in this document is something to the 
effect that the "mdio-handle" is used for systems which have multiple 
nic/mac's sharing a single MDIO bus. Otherwise the MDIO bus and its phy 
should be a child of the nic/mac using it, with standardized 
behaviors/etc left up to the OSPM when it comes to MDIO bus 
enumeration/etc.

Thanks,


> +
> +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.
> +
> +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},
> +	      }
> +	    })
> +	  } // 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
> +	}
> 

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

[Andrew Lunn]

> Otherwise the MDIO bus and its phy should be a
> child of the nic/mac using it, with standardized behaviors/etc left up to
> the OSPM when it comes to MDIO bus enumeration/etc.

Hi Jeremy 

Could you be a bit more specific here please.

DT allows

        macb0: ethernet@fffc4000 {
                compatible = "cdns,at32ap7000-macb";
                reg = <0xfffc4000 0x4000>;
                interrupts = <21>;
                phy-mode = "rmii";
                local-mac-address = [3a 0e 03 04 05 06];
                clock-names = "pclk", "hclk", "tx_clk";
                clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
                ethernet-phy@1 {
                        reg = <0x1>;
                        reset-gpios = <&pioE 6 1>;
                };
        };

So the PHY is a direct child of the MAC. The MDIO bus is not modelled
at all. Although this is allowed, it is deprecated, because it results
in problems with advanced systems which have multiple different
children, and the need to differentiate them. So drivers are slowly
migrating to always modelling the MDIO bus. In that case, the
phy-handle is always used to point to the PHY:

        eth0: ethernet@522d0000 {
                compatible = "socionext,synquacer-netsec";
                reg = <0 0x522d0000 0x0 0x10000>, <0 0x10000000 0x0 0x10000>;
                interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
                clocks = <&clk_netsec>;
                clock-names = "phy_ref_clk";
                phy-mode = "rgmii";
                max-speed = <1000>;
                max-frame-size = <9000>;
                phy-handle = <&phy1>;

                mdio {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        phy1: ethernet-phy@1 {
                                compatible = "ethernet-phy-ieee802.3-c22";
                                reg = <1>;
                        };
                };

"mdio-handle" is just half of phy-handle.

What you seem to be say is that although we have defined a generic
solution for ACPI which should work in all cases, it is suggested to
not use it? What exactly are you suggesting in its place?

	Andrew

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

[Jeremy Linton]

Hi,

On 7/24/20 8:39 AM, Andrew Lunn wrote:
>> Otherwise the MDIO bus and its phy should be a
>> child of the nic/mac using it, with standardized behaviors/etc left up to
>> the OSPM when it comes to MDIO bus enumeration/etc.
> 
> Hi Jeremy
> 
> Could you be a bit more specific here please.
> 
> DT allows
> 
>          macb0: ethernet@fffc4000 {
>                  compatible = "cdns,at32ap7000-macb";
>                  reg = <0xfffc4000 0x4000>;
>                  interrupts = <21>;
>                  phy-mode = "rmii";
>                  local-mac-address = [3a 0e 03 04 05 06];
>                  clock-names = "pclk", "hclk", "tx_clk";
>                  clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
>                  ethernet-phy@1 {
>                          reg = <0x1>;
>                          reset-gpios = <&pioE 6 1>;
>                  };
>          };
> 
> So the PHY is a direct child of the MAC. The MDIO bus is not modelled
> at all. Although this is allowed, it is deprecated, because it results > in problems with advanced systems which have multiple different
> children, and the need to differentiate them. So drivers are slowly

I don't think i'm suggesting that, because AFAIK in ACPI you would have 
to specify the DEVICE() for mdio, in order to nest a further set of 
phy's via _ADR(). I think in general what I was describing would look 
more like what you have below. But..

> migrating to always modelling the MDIO bus. In that case, the
> phy-handle is always used to point to the PHY:
> 
>          eth0: ethernet@522d0000 {
>                  compatible = "socionext,synquacer-netsec";
>                  reg = <0 0x522d0000 0x0 0x10000>, <0 0x10000000 0x0 0x10000>;
>                  interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
>                  clocks = <&clk_netsec>;
>                  clock-names = "phy_ref_clk";
>                  phy-mode = "rgmii";
>                  max-speed = <1000>;
>                  max-frame-size = <9000>;
>                  phy-handle = <&phy1>;
> 
>                  mdio {
>                          #address-cells = <1>;
>                          #size-cells = <0>;
>                          phy1: ethernet-phy@1 {
>                                  compatible = "ethernet-phy-ieee802.3-c22";
>                                  reg = <1>;
>                          };
>                  };
> 
> "mdio-handle" is just half of phy-handle.
> 
> What you seem to be say is that although we have defined a generic
> solution for ACPI which should work in all cases, it is suggested to
> not use it? What exactly are you suggesting in its place?

When you put it that way, what i'm saying sounds crazy.

In this case what are are doing isn't as clean as what you have 
described above, its more like:

mdio: {
   phy1: {}
   phy2: {}
}
...
// somewhere else
dmac1: {
     phy-handle = <&phy1>;
}

... //somewhere else
eth0: {
    //another device talking to the mgmt controller
}


Which is special in a couple ways.

Lets rewind for a moment and say for ARM/ACPI, what we are talking about 
are "edge/server class" devices (with RAS statements/etc) where the 
expectation is that they will be running virtualized workloads using LTS 
distros, or non linux OSes. DT/etc remains an option for networking 
devices which are more "embedded", aren't SBSA, etc. So an Arm 
based/ACPI machine should be more regular and share more in the way of 
system architecture with other SBSA/SBBR/ACPI/etc machines than has been 
the case for DT machines.

A concern is then how we punch networking devices into an arbitrary VM 
in a standardized way using libvirt/whatever. If the networking device 
doesn't look like a simple self contained memory mapped resource with an 
IOMMU domain, I think everything becomes more complicated and you have 
to start going down the path of special caseing the VM firmware beyond 
how its done for self contained PCIe/SRIOV devices. The latter manage to 
pull this all off with a PCIe id, and a couple BARs fed into the VM.

So, I would hope an ACPI nic representation is closer to just a minimal 
resource list like:

eth0: {
       compatible = "cdns,at32ap7000-macb";
       reg = <0xfffc4000 0x4000>;
       interrupts = <21>;
}
or in ACPI speak:
Device (ETH0)
{
       Name (_HID, "CDNSXXX")
       Method (_CRS, 0x0, Serialized)
       {
         Name (RBUF, ResourceTemplate ()
         {
           Memory32Fixed (ReadWrite, 0xfffc4000, 0x4000, )
           Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive)
           {
             21
           }
         })
         Return (RBUF)
       }
}

(Plus methods for pwr mgmt/etc as needed, the iommu info comes from 
another table).

Returning to the NXP part. They avoid the entirety of the above 
discussion because all this MDIO/PHY mgmt is just feeding the data into 
the mgmt controller, and the bits that are punched into the VM are 
fairly standalone.

Anyway, I think this set is generally fine, I would like to see this 
part working well with ACPI given its what we have available today. For 
the future, we also need to continue pushing everyone towards common 
hardware standards. One of the ways of doing this is having hardware 
which can be automatically enumerated/configured. Suggesting that the 
kernel has a recommended way of doing this which aids fragmentation 
isn't what we are trying to achieve with ACPI. Hence my previous comment 
that we should consider this an escape hatch rather than the last word 
in how to describe networking on ACPI/SBSA platforms.

Thanks,

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

[Florian Fainelli]

On 7/24/20 10:26 AM, Jeremy Linton wrote:
> Hi,
> 
> On 7/24/20 8:39 AM, Andrew Lunn wrote:
>>> Otherwise the MDIO bus and its phy should be a
>>> child of the nic/mac using it, with standardized behaviors/etc left
>>> up to
>>> the OSPM when it comes to MDIO bus enumeration/etc.
>>
>> Hi Jeremy
>>
>> Could you be a bit more specific here please.
>>
>> DT allows
>>
>>          macb0: ethernet@fffc4000 {
>>                  compatible = "cdns,at32ap7000-macb";
>>                  reg = <0xfffc4000 0x4000>;
>>                  interrupts = <21>;
>>                  phy-mode = "rmii";
>>                  local-mac-address = [3a 0e 03 04 05 06];
>>                  clock-names = "pclk", "hclk", "tx_clk";
>>                  clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
>>                  ethernet-phy@1 {
>>                          reg = <0x1>;
>>                          reset-gpios = <&pioE 6 1>;
>>                  };
>>          };
>>
>> So the PHY is a direct child of the MAC. The MDIO bus is not modelled
>> at all. Although this is allowed, it is deprecated, because it results
>> > in problems with advanced systems which have multiple different
>> children, and the need to differentiate them. So drivers are slowly
> 
> I don't think i'm suggesting that, because AFAIK in ACPI you would have
> to specify the DEVICE() for mdio, in order to nest a further set of
> phy's via _ADR(). I think in general what I was describing would look
> more like what you have below. But..
> 
>> migrating to always modelling the MDIO bus. In that case, the
>> phy-handle is always used to point to the PHY:
>>
>>          eth0: ethernet@522d0000 {
>>                  compatible = "socionext,synquacer-netsec";
>>                  reg = <0 0x522d0000 0x0 0x10000>, <0 0x10000000 0x0
>> 0x10000>;
>>                  interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
>>                  clocks = <&clk_netsec>;
>>                  clock-names = "phy_ref_clk";
>>                  phy-mode = "rgmii";
>>                  max-speed = <1000>;
>>                  max-frame-size = <9000>;
>>                  phy-handle = <&phy1>;
>>
>>                  mdio {
>>                          #address-cells = <1>;
>>                          #size-cells = <0>;
>>                          phy1: ethernet-phy@1 {
>>                                  compatible =
>> "ethernet-phy-ieee802.3-c22";
>>                                  reg = <1>;
>>                          };
>>                  };
>>
>> "mdio-handle" is just half of phy-handle.
>>
>> What you seem to be say is that although we have defined a generic
>> solution for ACPI which should work in all cases, it is suggested to
>> not use it? What exactly are you suggesting in its place?
> 
> When you put it that way, what i'm saying sounds crazy.
> 
> In this case what are are doing isn't as clean as what you have
> described above, its more like:
> 
> mdio: {
>   phy1: {}
>   phy2: {}
> }
> ...
> // somewhere else
> dmac1: {
>     phy-handle = <&phy1>;
> }
> 
> ... //somewhere else
> eth0: {
>    //another device talking to the mgmt controller
> }
> 
> 
> Which is special in a couple ways.
> 
> Lets rewind for a moment and say for ARM/ACPI, what we are talking about
> are "edge/server class" devices (with RAS statements/etc) where the
> expectation is that they will be running virtualized workloads using LTS
> distros, or non linux OSes. DT/etc remains an option for networking
> devices which are more "embedded", aren't SBSA, etc. So an Arm
> based/ACPI machine should be more regular and share more in the way of
> system architecture with other SBSA/SBBR/ACPI/etc machines than has been
> the case for DT machines.
> 
> A concern is then how we punch networking devices into an arbitrary VM
> in a standardized way using libvirt/whatever. If the networking device
> doesn't look like a simple self contained memory mapped resource with an
> IOMMU domain, I think everything becomes more complicated and you have
> to start going down the path of special caseing the VM firmware beyond
> how its done for self contained PCIe/SRIOV devices. The latter manage to
> pull this all off with a PCIe id, and a couple BARs fed into the VM.
> 
> So, I would hope an ACPI nic representation is closer to just a minimal
> resource list like:
> 
> eth0: {
>       compatible = "cdns,at32ap7000-macb";
>       reg = <0xfffc4000 0x4000>;
>       interrupts = <21>;
> }
> or in ACPI speak:
> Device (ETH0)
> {
>       Name (_HID, "CDNSXXX")
>       Method (_CRS, 0x0, Serialized)
>       {
>         Name (RBUF, ResourceTemplate ()
>         {
>           Memory32Fixed (ReadWrite, 0xfffc4000, 0x4000, )
>           Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive)
>           {
>             21
>           }
>         })
>         Return (RBUF)
>       }
> }
> 
> (Plus methods for pwr mgmt/etc as needed, the iommu info comes from
> another table).
> 
> Returning to the NXP part. They avoid the entirety of the above
> discussion because all this MDIO/PHY mgmt is just feeding the data into
> the mgmt controller, and the bits that are punched into the VM are
> fairly standalone.
> 
> Anyway, I think this set is generally fine, I would like to see this
> part working well with ACPI given its what we have available today. For
> the future, we also need to continue pushing everyone towards common
> hardware standards. One of the ways of doing this is having hardware
> which can be automatically enumerated/configured. Suggesting that the
> kernel has a recommended way of doing this which aids fragmentation
> isn't what we are trying to achieve with ACPI. Hence my previous comment
> that we should consider this an escape hatch rather than the last word
> in how to describe networking on ACPI/SBSA platforms.

We are at v7 of this patch series, and no authoritative ACPI Linux
maintainer appears to have reviewed this, so there is no clear sign of
this converging anywhere. This is looking a lot like busy work for
nothing. Given that the representation appears to be wildly
misunderstood and no one seems to come up with something that reaches
community agreement, what exactly is the plan here?

I am going to suggest something highly unpopular here: how about you
just load Device Tree overlays based on matching a particular board and
ship those overlays somewhere in the kernel that take care of
registering your network devices with the desired network topology?
-- 
Florian

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

[Andrew Lunn]

> Hence my previous comment that we should consider this an escape
> hatch rather than the last word in how to describe networking on
> ACPI/SBSA platforms.

One problem i have is that this patch set suggests ACPI can be used to
describe complex network hardware. It is opening the door for others
to follow and add more ACPI support in networking. How long before it
is not considered an escape hatch, but the front door?

For an example, see

https://patchwork.ozlabs.org/project/netdev/patch/1595417547-18957-3-git-send-email-vikas.singh@puresoftware.com/

It is hard to see what the big picture is here. The [0/2] patch is not
particularly good. But it makes it clear that people are wanting to
add fixed-link PHYs into ACPI. These are pseudo devices, used to make
the MAC think it is connected to a PHY when it is not. The MAC still
gets informed of link speed, etc via the standard PHYLIB API. They are
mostly used for when the Ethernet MAC is directly connected to an
Ethernet Switch, at a MAC to MAC level.

Now i could be wrong, but are Ethernet switches something you expect
to see on ACPI/SBSA platforms? Or is this a legitimate use of the
escape hatch?

       Andrew

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

[Andrew Lunn]

> We are at v7 of this patch series, and no authoritative ACPI Linux
> maintainer appears to have reviewed this, so there is no clear sign of
> this converging anywhere. This is looking a lot like busy work for
> nothing. Given that the representation appears to be wildly
> misunderstood and no one seems to come up with something that reaches
> community agreement, what exactly is the plan here?

I think we need to NACK all attempts to add ACPI support to phylib and
phylink until an authoritative ACPI Linux maintainer makes an
appearance and actively steers the work. And not just this patchset,
but all patchsets in the networking domain which have an ACPI
component.

	   Andrew

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

[Andy Shevchenko]

On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote:

> I think we need to NACK all attempts to add ACPI support to phylib and
> phylink until an authoritative ACPI Linux maintainer makes an
> appearance and actively steers the work. And not just this patchset,
> but all patchsets in the networking domain which have an ACPI
> component.

It's funny, since I see ACPI mailing list and none of the maintainers
in the Cc here...
I'm not sure they pay attention to some (noise-like?) activity which
(from their perspective) happens on unrelated lists.


-- 
With Best Regards,
Andy Shevchenko

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

[Florian Fainelli]

On 7/24/20 1:12 PM, Andy Shevchenko wrote:
> On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
> 
>> I think we need to NACK all attempts to add ACPI support to phylib and
>> phylink until an authoritative ACPI Linux maintainer makes an
>> appearance and actively steers the work. And not just this patchset,
>> but all patchsets in the networking domain which have an ACPI
>> component.
> 
> It's funny, since I see ACPI mailing list and none of the maintainers
> in the Cc here...
> I'm not sure they pay attention to some (noise-like?) activity which
> (from their perspective) happens on unrelated lists.

If you what you describe here is their perception of what is going on
here, that is very encouraging, we are definitively going to make progress.
-- 
Florian

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

[Andy Shevchenko]

On Fri, Jul 24, 2020 at 11:13 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 7/24/20 1:12 PM, Andy Shevchenko wrote:
> > On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> >> I think we need to NACK all attempts to add ACPI support to phylib and
> >> phylink until an authoritative ACPI Linux maintainer makes an
> >> appearance and actively steers the work. And not just this patchset,
> >> but all patchsets in the networking domain which have an ACPI
> >> component.
> >
> > It's funny, since I see ACPI mailing list and none of the maintainers
> > in the Cc here...
> > I'm not sure they pay attention to some (noise-like?) activity which
> > (from their perspective) happens on unrelated lists.
>
> If you what you describe here is their perception of what is going on
> here, that is very encouraging, we are definitively going to make progress.

I can't speak for them. As a maintainer in other areas I expect that
people Cc explicitly maintainer(s) if they want more attention.
Otherwise I look at the mails to the mailing list just from time to
time. But this is my expectation, don't take me wrong.

-- 
With Best Regards,
Andy Shevchenko

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

[Russell King - ARM Linux admin]

On Fri, Jul 24, 2020 at 11:12:15PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > I think we need to NACK all attempts to add ACPI support to phylib and
> > phylink until an authoritative ACPI Linux maintainer makes an
> > appearance and actively steers the work. And not just this patchset,
> > but all patchsets in the networking domain which have an ACPI
> > component.
> 
> It's funny, since I see ACPI mailing list and none of the maintainers
> in the Cc here...
> I'm not sure they pay attention to some (noise-like?) activity which
> (from their perspective) happens on unrelated lists.

That is really disappointing that these patch sets are not being copied
to the appropriate people (ACPI).

I seem to remember I've already stated on at least a couple of
occasions that these patch sets which add ACPI support to phylib need
to be copied to ACPI people.  I guess if ACPI people have been omitted,
there will be a few more patch series iterations.  Then there's that
all the discussion that has already happened is not known to ACPI
people, so we're probably doomed to repeating at least some of that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Calvin Johnson]

On Fri, Jul 24, 2020 at 11:20:04PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 24, 2020 at 11:13 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > On 7/24/20 1:12 PM, Andy Shevchenko wrote:
> > > On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > >> I think we need to NACK all attempts to add ACPI support to phylib and
> > >> phylink until an authoritative ACPI Linux maintainer makes an
> > >> appearance and actively steers the work. And not just this patchset,
> > >> but all patchsets in the networking domain which have an ACPI
> > >> component.
> > >
> > > It's funny, since I see ACPI mailing list and none of the maintainers
> > > in the Cc here...
> > > I'm not sure they pay attention to some (noise-like?) activity which
> > > (from their perspective) happens on unrelated lists.
> >
> > If you what you describe here is their perception of what is going on
> > here, that is very encouraging, we are definitively going to make progress.
> 
> I can't speak for them. As a maintainer in other areas I expect that
> people Cc explicitly maintainer(s) if they want more attention.
> Otherwise I look at the mails to the mailing list just from time to
> time. But this is my expectation, don't take me wrong.

Sorry about this miss.
In some past patch-set, I had added Rafael but somehow missed him this time.

From the "MAINTAINERS" file, I got two maintainers. I don't know who else
can help with this discussion. I'll add others whom I know from ACPI list.
M:      "Rafael J. Wysocki" <rjw@rjwysocki.net>
M:      Len Brown <lenb@kernel.org>

If you know others who can help, please add.

Hi ACPI experts,
Would you please help review this patchset and guide us.

Please see the discussion on this patchset here:
https://lore.kernel.org/linux-acpi/20200715090400.4733-1-calvin.johnson@oss.nxp.com/T/#t

Thanks
Calvin

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

[Andy Shevchenko]

On Sat, Jul 25, 2020 at 10:37 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
> On Fri, Jul 24, 2020 at 11:20:04PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 24, 2020 at 11:13 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > On 7/24/20 1:12 PM, Andy Shevchenko wrote:
> > > > On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote:

...

> > > >> I think we need to NACK all attempts to add ACPI support to phylib and
> > > >> phylink until an authoritative ACPI Linux maintainer makes an
> > > >> appearance and actively steers the work. And not just this patchset,
> > > >> but all patchsets in the networking domain which have an ACPI
> > > >> component.
> > > >
> > > > It's funny, since I see ACPI mailing list and none of the maintainers
> > > > in the Cc here...
> > > > I'm not sure they pay attention to some (noise-like?) activity which
> > > > (from their perspective) happens on unrelated lists.
> > >
> > > If you what you describe here is their perception of what is going on
> > > here, that is very encouraging, we are definitively going to make progress.
> >
> > I can't speak for them. As a maintainer in other areas I expect that
> > people Cc explicitly maintainer(s) if they want more attention.
> > Otherwise I look at the mails to the mailing list just from time to
> > time. But this is my expectation, don't take me wrong.
>
> Sorry about this miss.
> In some past patch-set, I had added Rafael but somehow missed him this time.
>
> From the "MAINTAINERS" file, I got two maintainers. I don't know who else
> can help with this discussion. I'll add others whom I know from ACPI list.
> M:      "Rafael J. Wysocki" <rjw@rjwysocki.net>
> M:      Len Brown <lenb@kernel.org>
>
> If you know others who can help, please add.
>
> Hi ACPI experts,
> Would you please help review this patchset and guide us.
>
> Please see the discussion on this patchset here:
> https://lore.kernel.org/linux-acpi/20200715090400.4733-1-calvin.johnson@oss.nxp.com/T/#t

I would recommend resending the entire series with an appropriate Cc list.
See below as well.

ACPI FOR ARM64 (ACPI/arm64)
M: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
M: Hanjun Guo <guohanjun@huawei.com>
M: Sudeep Holla <sudeep.holla@arm.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Sudeep Holla]

On Fri, Jul 24, 2020 at 09:20:08PM +0200, Andrew Lunn wrote:
> > We are at v7 of this patch series, and no authoritative ACPI Linux
> > maintainer appears to have reviewed this, so there is no clear sign of
> > this converging anywhere. This is looking a lot like busy work for
> > nothing. Given that the representation appears to be wildly
> > misunderstood and no one seems to come up with something that reaches
> > community agreement, what exactly is the plan here?
>
> I think we need to NACK all attempts to add ACPI support to phylib and
> phylink until an authoritative ACPI Linux maintainer makes an
> appearance and actively steers the work. And not just this patchset,
> but all patchsets in the networking domain which have an ACPI
> component.
>

Unfortunately, this is one such problem that can never be solved easily
TBH.

We, in Linux kernel community had lots of discussion around _DSD and
how it can be misused if not moderated after the introduction of ACPI
support on Arm. It is useful property used by the kernel today both
on x86 and Arm. Even other OS vendors do use, but the standard body
recently deprecated the process we introduced few years back[1] as it
really never kicked off. All OS vendors have introduced the properties
as they need and have supported without a formal registry and this is
the argument made to deprecate that process.

As a general rule, we say no to any new property added unless there is
no existing solution for the same. It might just expand exponential if
not controlled. So if networking folks agree that there is a need for
it and there exists no alternative solution, then we may need to add
the support for the same. I don't have strong objection as I have least
knowledge in network domain.

But I agree, there exists a possibility of duplication of properties
amongst different OS vendors and could be argument on the other side.

--
Regards,
Sudeep

[1] http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf

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

[Sudeep Holla]

On Fri, Jul 24, 2020 at 09:14:36PM +0200, Andrew Lunn wrote:
> > Hence my previous comment that we should consider this an escape
> > hatch rather than the last word in how to describe networking on
> > ACPI/SBSA platforms.
>
> One problem i have is that this patch set suggests ACPI can be used to
> describe complex network hardware. It is opening the door for others
> to follow and add more ACPI support in networking. How long before it
> is not considered an escape hatch, but the front door?
>

I understand your concerns here. But as I mentioned in other email in
the same thread, it is very tricky problem to solve as no one is ready
to take up and maintain these.

> For an example, see
>
> https://patchwork.ozlabs.org/project/netdev/patch/1595417547-18957-3-git-send-email-vikas.singh@puresoftware.com/
>
> It is hard to see what the big picture is here. The [0/2] patch is not
> particularly good. But it makes it clear that people are wanting to
> add fixed-link PHYs into ACPI. These are pseudo devices, used to make
> the MAC think it is connected to a PHY when it is not. The MAC still
> gets informed of link speed, etc via the standard PHYLIB API. They are
> mostly used for when the Ethernet MAC is directly connected to an
> Ethernet Switch, at a MAC to MAC level.
>
> Now i could be wrong, but are Ethernet switches something you expect
> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
> escape hatch?
>

My guess is that similar products running on other architectures(namely
x86) might be running ACPI and hence the push to have ACPI on such ARM
systems. It may weak argument for that and I agree with it. I want to
think it as legitimate use here but I am well aware and afraid that this
may become front door instead of escape hatch.

Sorry, I am not helpful at all, but I am just sharing my personal opinion
on this matter.

--
Regards,
Sudeep

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

[Jon Nettleton]

On Fri, Jul 24, 2020 at 9:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Hence my previous comment that we should consider this an escape
> > hatch rather than the last word in how to describe networking on
> > ACPI/SBSA platforms.
>
> One problem i have is that this patch set suggests ACPI can be used to
> describe complex network hardware. It is opening the door for others
> to follow and add more ACPI support in networking. How long before it
> is not considered an escape hatch, but the front door?
>
> For an example, see
>
> https://patchwork.ozlabs.org/project/netdev/patch/1595417547-18957-3-git-send-email-vikas.singh@puresoftware.com/
>
> It is hard to see what the big picture is here. The [0/2] patch is not
> particularly good. But it makes it clear that people are wanting to
> add fixed-link PHYs into ACPI. These are pseudo devices, used to make
> the MAC think it is connected to a PHY when it is not. The MAC still
> gets informed of link speed, etc via the standard PHYLIB API. They are
> mostly used for when the Ethernet MAC is directly connected to an
> Ethernet Switch, at a MAC to MAC level.
>
> Now i could be wrong, but are Ethernet switches something you expect
> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
> escape hatch?

I think with the rise in adoption of Smart-NICs in datacenters there
will definitely be a lot more crossover between ACPI/SBSA and network
appliance oriented hardware.

-Jon

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

[Andrew Lunn]

Hi Everybody

So i think it is time to try to bring this discussion to some sort of
conclusion.

No ACPI maintainer is willing to ACK any of these patches. Nor are
they willing to NACK them. ACPI maintainers simply don't want to get
involved in making use of ACPI in networking.

I personally don't have the knowledge to do ACPI correctly, review
patches, point people in the right direction. I suspect the same can
be said for the other PHY maintainers.

Having said that, there is clearly a wish from vendors to make use of
ACPI in the networking subsystem to describe hardware.

How do we go forward?

For the moment, we will need to NACK all patches adding ACPI support
to the PHY subsystem.

Vendors who really do want to use ACPI, not device tree, probably
need to get involved in standardisation. Vendors need to submit a
proposal to UEFI and get it accepted.

Developers should try to engage with the ACPI maintainers and see
if they can get them involved in networking. Patches with an
Acked-by from an ACPI maintainer will be accepted, assuming they
fulfil all the other usual requirements. But please don't submit
patches until you do have an ACPI maintainer on board. We don't
want to spamming the lists with NACKs all the time.

     Andrew

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

[Andrew Lunn]

On Tue, Jul 28, 2020 at 06:06:26PM +1000, Dan Callaghan wrote:
> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
> > Now i could be wrong, but are Ethernet switches something you expect
> > to see on ACPI/SBSA platforms? Or is this a legitimate use of the
> > escape hatch?
> 
> As an extra data point: right now I am working on an x86 embedded 
> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. 
> I have been watching this patch series with great interest, because 
> right now there is no way for me to configure a complex switch topology 
> in DSA without Device Tree.
> 
> For the device I am working on, we will have units shipping before these 
> questions about how to represent Ethernet switches in ACPI can be 
> resolved. So realistically, we will have to actually configure the 
> switches using software_node structures supplied by an out-of-tree 
> platform driver, or some hackery like that, rather than configuring them 
> through ACPI.

Hi Dan

I also have an x86 platform, but with a single switch. For that, i
have a platform driver, which instantiates a bit banging MDIO bus, and
sets up the switch using platform data. This works, but it is limited
to internal Copper only PHYs.

> An approach I have been toying with is to port all of DSA to use the 
> fwnode_handle abstraction instead of Device Tree nodes, but that is 
> obviously a large task, and frankly I was not sure whether such a patch 
> series would be welcomed.

I would actually suggest you look at using DT. We are struggling to
get ACPI maintainers involved with really simple things, like the ACPI
equivalent of a phandle from the MAC to the PHY. A full DSA binding
for Marvell switches is pretty complex, especially if you need SFP
support. I expect the ACPI maintainers will actively run away
screaming when you make your proposal.

DT can be used on x86, and i suspect it is a much easier path of least
resistance.

	Andrew

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

[Florian Fainelli]

On 7/28/2020 1:45 PM, Andrew Lunn wrote:
> On Tue, Jul 28, 2020 at 06:06:26PM +1000, Dan Callaghan wrote:
>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
>>> Now i could be wrong, but are Ethernet switches something you expect
>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
>>> escape hatch?
>>
>> As an extra data point: right now I am working on an x86 embedded 
>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. 
>> I have been watching this patch series with great interest, because 
>> right now there is no way for me to configure a complex switch topology 
>> in DSA without Device Tree.
>>
>> For the device I am working on, we will have units shipping before these 
>> questions about how to represent Ethernet switches in ACPI can be 
>> resolved. So realistically, we will have to actually configure the 
>> switches using software_node structures supplied by an out-of-tree 
>> platform driver, or some hackery like that, rather than configuring them 
>> through ACPI.
> 
> Hi Dan
> 
> I also have an x86 platform, but with a single switch. For that, i
> have a platform driver, which instantiates a bit banging MDIO bus, and
> sets up the switch using platform data. This works, but it is limited
> to internal Copper only PHYs.

At some point I had a dsa2_platform_data implementation which was
intended to describe more complex switch set-ups and trees, the old code
is still there for your entertainment:

https://github.com/ffainelli/linux/commits/dsa-pdata

> 
>> An approach I have been toying with is to port all of DSA to use the 
>> fwnode_handle abstraction instead of Device Tree nodes, but that is 
>> obviously a large task, and frankly I was not sure whether such a patch 
>> series would be welcomed.
> 
> I would actually suggest you look at using DT. We are struggling to
> get ACPI maintainers involved with really simple things, like the ACPI
> equivalent of a phandle from the MAC to the PHY. A full DSA binding
> for Marvell switches is pretty complex, especially if you need SFP
> support. I expect the ACPI maintainers will actively run away
> screaming when you make your proposal.
> 
> DT can be used on x86, and i suspect it is a much easier path of least
> resistance.

And you can easily overlay Device Tree to an existing system by using
either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and
creating nodes on the fly.
-- 
Florian

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

[Russell King - ARM Linux admin]

On Tue, Jul 28, 2020 at 10:34:37PM +0200, Andrew Lunn wrote:
> Hi Everybody
> 
> So i think it is time to try to bring this discussion to some sort of
> conclusion.
> 
> No ACPI maintainer is willing to ACK any of these patches. Nor are
> they willing to NACK them. ACPI maintainers simply don't want to get
> involved in making use of ACPI in networking.
> 
> I personally don't have the knowledge to do ACPI correctly, review
> patches, point people in the right direction. I suspect the same can
> be said for the other PHY maintainers.
> 
> Having said that, there is clearly a wish from vendors to make use of
> ACPI in the networking subsystem to describe hardware.
> 
> How do we go forward?
> 
> For the moment, we will need to NACK all patches adding ACPI support
> to the PHY subsystem.
> 
> Vendors who really do want to use ACPI, not device tree, probably
> need to get involved in standardisation. Vendors need to submit a
> proposal to UEFI and get it accepted.
> 
> Developers should try to engage with the ACPI maintainers and see
> if they can get them involved in networking. Patches with an
> Acked-by from an ACPI maintainer will be accepted, assuming they
> fulfil all the other usual requirements. But please don't submit
> patches until you do have an ACPI maintainer on board. We don't
> want to spamming the lists with NACKs all the time.

For the record, this statement reflects my position as well (as one
of the named phylib maintainers).  Thanks Andrew.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Andy Shevchenko]

On Tue, Jul 28, 2020 at 11:59 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Tue, Jul 28, 2020 at 10:34:37PM +0200, Andrew Lunn wrote:
> > Hi Everybody
> >
> > So i think it is time to try to bring this discussion to some sort of
> > conclusion.
> >
> > No ACPI maintainer is willing to ACK any of these patches. Nor are
> > they willing to NACK them. ACPI maintainers simply don't want to get
> > involved in making use of ACPI in networking.
> >
> > I personally don't have the knowledge to do ACPI correctly, review
> > patches, point people in the right direction. I suspect the same can
> > be said for the other PHY maintainers.
> >
> > Having said that, there is clearly a wish from vendors to make use of
> > ACPI in the networking subsystem to describe hardware.
> >
> > How do we go forward?
> >
> > For the moment, we will need to NACK all patches adding ACPI support
> > to the PHY subsystem.
> >
> > Vendors who really do want to use ACPI, not device tree, probably
> > need to get involved in standardisation. Vendors need to submit a
> > proposal to UEFI and get it accepted.
> >
> > Developers should try to engage with the ACPI maintainers and see
> > if they can get them involved in networking. Patches with an
> > Acked-by from an ACPI maintainer will be accepted, assuming they
> > fulfil all the other usual requirements. But please don't submit
> > patches until you do have an ACPI maintainer on board. We don't
> > want to spamming the lists with NACKs all the time.
>
> For the record, this statement reflects my position as well (as one
> of the named phylib maintainers).  Thanks Andrew.

Again, folks, you are discussing something without direct Cc'ing to
them (I see a subset? of the maintainers we discussed in another
mail).
I believe that many maintainers are using some type of scoring for
their emails and Cc'ing directly increases chances to get a reply.
Also you have at least two or three people in ACPI/arm64. What do they think?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jul 28, 2020 at 11:56 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 7/28/2020 1:45 PM, Andrew Lunn wrote:
> > On Tue, Jul 28, 2020 at 06:06:26PM +1000, Dan Callaghan wrote:
> >> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
> >>> Now i could be wrong, but are Ethernet switches something you expect
> >>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
> >>> escape hatch?
> >>
> >> As an extra data point: right now I am working on an x86 embedded
> >> appliance (ACPI not Device Tree) with 3x integrated Marvell switches.
> >> I have been watching this patch series with great interest, because
> >> right now there is no way for me to configure a complex switch topology
> >> in DSA without Device Tree.
> >>
> >> For the device I am working on, we will have units shipping before these
> >> questions about how to represent Ethernet switches in ACPI can be
> >> resolved. So realistically, we will have to actually configure the
> >> switches using software_node structures supplied by an out-of-tree
> >> platform driver, or some hackery like that, rather than configuring them
> >> through ACPI.
> >
> > Hi Dan
> >
> > I also have an x86 platform, but with a single switch. For that, i
> > have a platform driver, which instantiates a bit banging MDIO bus, and
> > sets up the switch using platform data. This works, but it is limited
> > to internal Copper only PHYs.
>
> At some point I had a dsa2_platform_data implementation which was
> intended to describe more complex switch set-ups and trees, the old code
> is still there for your entertainment:
>
> https://github.com/ffainelli/linux/commits/dsa-pdata

Platform data in the modern kernel is definitely the wrong approach.
Software nodes of firmware nodes can be much more appreciated.

> >> An approach I have been toying with is to port all of DSA to use the
> >> fwnode_handle abstraction instead of Device Tree nodes, but that is
> >> obviously a large task, and frankly I was not sure whether such a patch
> >> series would be welcomed.
> >
> > I would actually suggest you look at using DT. We are struggling to
> > get ACPI maintainers involved with really simple things, like the ACPI
> > equivalent of a phandle from the MAC to the PHY. A full DSA binding
> > for Marvell switches is pretty complex, especially if you need SFP
> > support. I expect the ACPI maintainers will actively run away
> > screaming when you make your proposal.
> >
> > DT can be used on x86, and i suspect it is a much easier path of least
> > resistance.
>
> And you can easily overlay Device Tree to an existing system by using
> either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and
> creating nodes on the fly.

Why do you need DT on a system that runs without it and Linux has all
means to extend to cover a lot of stuff DT provides for other types of
firmware nodes?

-- 
With Best Regards,
Andy Shevchenko

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

[Florian Fainelli]

On 7/28/2020 2:28 PM, Andy Shevchenko wrote:
> On Tue, Jul 28, 2020 at 11:56 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 7/28/2020 1:45 PM, Andrew Lunn wrote:
>>> On Tue, Jul 28, 2020 at 06:06:26PM +1000, Dan Callaghan wrote:
>>>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
>>>>> Now i could be wrong, but are Ethernet switches something you expect
>>>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
>>>>> escape hatch?
>>>>
>>>> As an extra data point: right now I am working on an x86 embedded
>>>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches.
>>>> I have been watching this patch series with great interest, because
>>>> right now there is no way for me to configure a complex switch topology
>>>> in DSA without Device Tree.
>>>>
>>>> For the device I am working on, we will have units shipping before these
>>>> questions about how to represent Ethernet switches in ACPI can be
>>>> resolved. So realistically, we will have to actually configure the
>>>> switches using software_node structures supplied by an out-of-tree
>>>> platform driver, or some hackery like that, rather than configuring them
>>>> through ACPI.
>>>
>>> Hi Dan
>>>
>>> I also have an x86 platform, but with a single switch. For that, i
>>> have a platform driver, which instantiates a bit banging MDIO bus, and
>>> sets up the switch using platform data. This works, but it is limited
>>> to internal Copper only PHYs.
>>
>> At some point I had a dsa2_platform_data implementation which was
>> intended to describe more complex switch set-ups and trees, the old code
>> is still there for your entertainment:
>>
>> https://github.com/ffainelli/linux/commits/dsa-pdata
> 
> Platform data in the modern kernel is definitely the wrong approach.
> Software nodes of firmware nodes can be much more appreciated.

Yes, yes, thank you. As you can see this was back from 2016 and it was
never submitted. The only viable alternative that I can think of, unless
the ACPI community at large finally decided to get its act together and
invest some serious efforts and time into understanding modern and
complex network topologies is to overlay a Device Tree onto a live system.

> 
>>>> An approach I have been toying with is to port all of DSA to use the
>>>> fwnode_handle abstraction instead of Device Tree nodes, but that is
>>>> obviously a large task, and frankly I was not sure whether such a patch
>>>> series would be welcomed.
>>>
>>> I would actually suggest you look at using DT. We are struggling to
>>> get ACPI maintainers involved with really simple things, like the ACPI
>>> equivalent of a phandle from the MAC to the PHY. A full DSA binding
>>> for Marvell switches is pretty complex, especially if you need SFP
>>> support. I expect the ACPI maintainers will actively run away
>>> screaming when you make your proposal.
>>>
>>> DT can be used on x86, and i suspect it is a much easier path of least
>>> resistance.
>>
>> And you can easily overlay Device Tree to an existing system by using
>> either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and
>> creating nodes on the fly.
> 
> Why do you need DT on a system that runs without it and Linux has all
> means to extend to cover a lot of stuff DT provides for other types of
> firmware nodes?

Because ACPI is beyond useless at providing nearly the same level of
description as what DT can do today?

I am not trying to wage a war of DT is better than ACPI, but when it is
not even capable of describing a simple 1 to 1 mapping between an
Ethernet MAC and a PHY device sitting on an integrated or separate MDIO
bus, describing a full Ethernet switch fabric with 1 to 40 ports, each
with a variety of connectivity options, and you have an pressing need to
get your platform out to customers, then the choice is obvious.
-- 
Florian

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

[Jeremy Linton]

Hi,

On 7/28/20 3:06 AM, Dan Callaghan wrote:
> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
>> Now i could be wrong, but are Ethernet switches something you expect
>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
>> escape hatch?
> 
> As an extra data point: right now I am working on an x86 embedded
> appliance (ACPI not Device Tree) with 3x integrated Marvell switches.
> I have been watching this patch series with great interest, because
> right now there is no way for me to configure a complex switch topology
> in DSA without Device Tree.

DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring 
whether that NIC/MAC is actually hug off PCIe for the moment).

It just has a bunch of phy devices strung out on that single MAC/MDIO. 
So in ACPI land it would still have a relationship similar to the one 
Andrew pointed out with his DT example where the eth0->mdio->phy are all 
contained in their physical parent. The phy in that case associated with 
the parent adapter would be the first direct decedent of the mdio, the 
switch itself could then be represented with another device, with a 
further string of device/phys representing the devices. (I dislike 
drawing acsii art, but if this isn't clear I will, its also worthwhile 
to look at the dpaa2 docs for how the mac/phys work on this device for 
contrast.).

If so, then its probably possible to represent with a fairly regular 
looking set of ACPI objects and avoids part of the core discussion here 
about whether we need a standardized way to pick phy's out of arbitrary 
parts of the hierarchy using a part of the spec intended for one off 
problems.

Am I missing something?



> 
> For the device I am working on, we will have units shipping before these
> questions about how to represent Ethernet switches in ACPI can be
> resolved. So realistically, we will have to actually configure the
> switches using software_node structures supplied by an out-of-tree
> platform driver, or some hackery like that, rather than configuring them
> through ACPI.
> 
> An approach I have been toying with is to port all of DSA to use the
> fwnode_handle abstraction instead of Device Tree nodes, but that is
> obviously a large task, and frankly I was not sure whether such a patch
> series would be welcomed.
> 

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

[Florian Fainelli]

On 7/28/2020 3:30 PM, Jeremy Linton wrote:
> Hi,
> 
> On 7/28/20 3:06 AM, Dan Callaghan wrote:
>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
>>> Now i could be wrong, but are Ethernet switches something you expect
>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
>>> escape hatch?
>>
>> As an extra data point: right now I am working on an x86 embedded
>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches.
>> I have been watching this patch series with great interest, because
>> right now there is no way for me to configure a complex switch topology
>> in DSA without Device Tree.
> 
> DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring
> whether that NIC/MAC is actually hug off PCIe for the moment).

There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches
all supported within the driver framework right now.

> 
> It just has a bunch of phy devices strung out on that single MAC/MDIO.

It has a number of built-in PHYs that typically appear on a MDIO bus,
whether that bus is the switch's internal MDIO bus, or another MDIO bus
(which could be provided with just two GPIOs) depends on how the switch
is connected to its management host.

When the switch is interfaced via MDIO the switch also typically has a
MDIO interface called the pseudo-PHY which is how you can actually tap
into the control interface of the switch, as opposed to reading its
internal PHYs from the MDIO bus.

> So in ACPI land it would still have a relationship similar to the one
> Andrew pointed out with his DT example where the eth0->mdio->phy are all
> contained in their physical parent. The phy in that case associated with
> the parent adapter would be the first direct decedent of the mdio, the
> switch itself could then be represented with another device, with a
> further string of device/phys representing the devices. (I dislike
> drawing acsii art, but if this isn't clear I will, its also worthwhile
> to look at the dpaa2 docs for how the mac/phys work on this device for
> contrast.).

The eth0->mdio->phy relationship you describe is the simple case that
you are well aware of which is say what we have on the Raspberry Pi 4
with GENET and the external Broadcom PHY.

For an Ethernet switch connected to an Ethernet MAC, we have 4 different
types of objects:

- the Ethernet MAC which sits on its specific bus

- the Ethernet switch which also sits on its specific bus

- the built-in PHYs of the Ethernet switch which sit on whatever
bus/interface the switch provides to make them accessible

- the specific bus controller that provides access to the Ethernet switch

and this is a simplification that does not take into account Physical
Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet
land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable modules.

> 
> If so, then its probably possible to represent with a fairly regular
> looking set of ACPI objects and avoids part of the core discussion here
> about whether we need a standardized way to pick phy's out of arbitrary
> parts of the hierarchy using a part of the spec intended for one off
> problems.

Using regular ACPI objects would work, however I do not see how it can
alleviate having this discussion. It has been repeated again and again
that we do not want to see snowflake ACPI representation that each and
every driver writer is going to draw inspiration from.

Upon further reading of the ACPI specification, I do not think we are
going to see much definition or a driving force show up about how the
Ethernet objects (MAC, PHY, switches, SFPs, etc.) relate to one another.
The ACPI specification seems to be more about defining the ACPI objects
and their methods, which is on a different scope than how to tie them
together.

What Calvin has done thus far is the closest to what I believe we can
achieve given how nebulous and arcane ACPI is for the PHY library
maintainers within this context.
-- 
Florian

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

[Jeremy Linton]

Hi,

On 7/28/20 7:39 PM, Florian Fainelli wrote:
> On 7/28/2020 3:30 PM, Jeremy Linton wrote:
>> Hi,
>>
>> On 7/28/20 3:06 AM, Dan Callaghan wrote:
>>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
>>>> Now i could be wrong, but are Ethernet switches something you expect
>>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
>>>> escape hatch?
>>>
>>> As an extra data point: right now I am working on an x86 embedded
>>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches.
>>> I have been watching this patch series with great interest, because
>>> right now there is no way for me to configure a complex switch topology
>>> in DSA without Device Tree.
>>
>> DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring
>> whether that NIC/MAC is actually hug off PCIe for the moment).
> 
> There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches
> all supported within the driver framework right now.
> 
>>
>> It just has a bunch of phy devices strung out on that single MAC/MDIO.
> 
> It has a number of built-in PHYs that typically appear on a MDIO bus,
> whether that bus is the switch's internal MDIO bus, or another MDIO bus
> (which could be provided with just two GPIOs) depends on how the switch
> is connected to its management host.
> 
> When the switch is interfaced via MDIO the switch also typically has a
> MDIO interface called the pseudo-PHY which is how you can actually tap
> into the control interface of the switch, as opposed to reading its
> internal PHYs from the MDIO bus.
> 
>> So in ACPI land it would still have a relationship similar to the one
>> Andrew pointed out with his DT example where the eth0->mdio->phy are all
>> contained in their physical parent. The phy in that case associated with
>> the parent adapter would be the first direct decedent of the mdio, the
>> switch itself could then be represented with another device, with a
>> further string of device/phys representing the devices. (I dislike
>> drawing acsii art, but if this isn't clear I will, its also worthwhile
>> to look at the dpaa2 docs for how the mac/phys work on this device for
>> contrast.).
> 
> The eth0->mdio->phy relationship you describe is the simple case that
> you are well aware of which is say what we have on the Raspberry Pi 4
> with GENET and the external Broadcom PHY.
> 
> For an Ethernet switch connected to an Ethernet MAC, we have 4 different
> types of objects:
> 
> - the Ethernet MAC which sits on its specific bus
> 
> - the Ethernet switch which also sits on its specific bus
> 
> - the built-in PHYs of the Ethernet switch which sit on whatever
> bus/interface the switch provides to make them accessible
> 
> - the specific bus controller that provides access to the Ethernet switch
> 
> and this is a simplification that does not take into account Physical
> Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet
> land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable modules.

Which is why I've stayed away from much of the switch discussion. There 
are a lot of edge cases to fall into, because for whatever reason 
networking seems to be unique in all this non-enumerable customization 
vs many other areas of the system. Storage, being an example i'm more 
familiar with which has very similar problems yet, somehow has managed 
to avoid much of this, despite having run on fabrics significantly more 
complex than basic ethernet (including running on a wide range of hot 
pluggable GBIC/SFP/QSFP/etc media layers).

ACPI's "problem" here is that its strongly influenced by the "Plug and 
Play" revolution of the 1990's where the industry went from having 
humans describing hardware using machine readable languages, to hardware 
which was enumerable using standard protocols. ACPI's device 
descriptions are there as a crutch for the remaining non plug an play 
hardware in the system.

So at a basic level, if your describing hardware in ACPI rather than 
abstracting it, that is a problem.



Thanks,

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

[Florian Fainelli]

On 7/28/2020 7:53 PM, Jeremy Linton wrote:
> Hi,
> 
> On 7/28/20 7:39 PM, Florian Fainelli wrote:
>> On 7/28/2020 3:30 PM, Jeremy Linton wrote:
>>> Hi,
>>>
>>> On 7/28/20 3:06 AM, Dan Callaghan wrote:
>>>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
>>>>> Now i could be wrong, but are Ethernet switches something you expect
>>>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
>>>>> escape hatch?
>>>>
>>>> As an extra data point: right now I am working on an x86 embedded
>>>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches.
>>>> I have been watching this patch series with great interest, because
>>>> right now there is no way for me to configure a complex switch topology
>>>> in DSA without Device Tree.
>>>
>>> DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring
>>> whether that NIC/MAC is actually hug off PCIe for the moment).
>>
>> There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches
>> all supported within the driver framework right now.
>>
>>>
>>> It just has a bunch of phy devices strung out on that single MAC/MDIO.
>>
>> It has a number of built-in PHYs that typically appear on a MDIO bus,
>> whether that bus is the switch's internal MDIO bus, or another MDIO bus
>> (which could be provided with just two GPIOs) depends on how the switch
>> is connected to its management host.
>>
>> When the switch is interfaced via MDIO the switch also typically has a
>> MDIO interface called the pseudo-PHY which is how you can actually tap
>> into the control interface of the switch, as opposed to reading its
>> internal PHYs from the MDIO bus.
>>
>>> So in ACPI land it would still have a relationship similar to the one
>>> Andrew pointed out with his DT example where the eth0->mdio->phy are all
>>> contained in their physical parent. The phy in that case associated with
>>> the parent adapter would be the first direct decedent of the mdio, the
>>> switch itself could then be represented with another device, with a
>>> further string of device/phys representing the devices. (I dislike
>>> drawing acsii art, but if this isn't clear I will, its also worthwhile
>>> to look at the dpaa2 docs for how the mac/phys work on this device for
>>> contrast.).
>>
>> The eth0->mdio->phy relationship you describe is the simple case that
>> you are well aware of which is say what we have on the Raspberry Pi 4
>> with GENET and the external Broadcom PHY.
>>
>> For an Ethernet switch connected to an Ethernet MAC, we have 4 different
>> types of objects:
>>
>> - the Ethernet MAC which sits on its specific bus
>>
>> - the Ethernet switch which also sits on its specific bus
>>
>> - the built-in PHYs of the Ethernet switch which sit on whatever
>> bus/interface the switch provides to make them accessible
>>
>> - the specific bus controller that provides access to the Ethernet switch
>>
>> and this is a simplification that does not take into account Physical
>> Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet
>> land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable
>> modules.
> 
> Which is why I've stayed away from much of the switch discussion. There
> are a lot of edge cases to fall into, because for whatever reason
> networking seems to be unique in all this non-enumerable customization
> vs many other areas of the system. Storage, being an example i'm more
> familiar with which has very similar problems yet, somehow has managed
> to avoid much of this, despite having run on fabrics significantly more
> complex than basic ethernet (including running on a wide range of hot
> pluggable GBIC/SFP/QSFP/etc media layers).
> 
> ACPI's "problem" here is that its strongly influenced by the "Plug and
> Play" revolution of the 1990's where the industry went from having
> humans describing hardware using machine readable languages, to hardware
> which was enumerable using standard protocols. ACPI's device
> descriptions are there as a crutch for the remaining non plug an play
> hardware in the system.
> 
> So at a basic level, if your describing hardware in ACPI rather than
> abstracting it, that is a problem.

I suppose that is a good summary, my impression from this patch series
is that we want the description part, not the abstraction, whether it is
on purpose or because there is a misunderstanding of what ACPI is
intended for, or higher powers have decided this must be done otherwise
nothing gets sold, who knows?
-- 
Florian

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

[Jon Nettleton]

On Wed, Jul 29, 2020 at 4:53 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
> On 7/28/20 7:39 PM, Florian Fainelli wrote:
> > On 7/28/2020 3:30 PM, Jeremy Linton wrote:
> >> Hi,
> >>
> >> On 7/28/20 3:06 AM, Dan Callaghan wrote:
> >>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
> >>>> Now i could be wrong, but are Ethernet switches something you expect
> >>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
> >>>> escape hatch?
> >>>
> >>> As an extra data point: right now I am working on an x86 embedded
> >>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches.
> >>> I have been watching this patch series with great interest, because
> >>> right now there is no way for me to configure a complex switch topology
> >>> in DSA without Device Tree.
> >>
> >> DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring
> >> whether that NIC/MAC is actually hug off PCIe for the moment).
> >
> > There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches
> > all supported within the driver framework right now.
> >
> >>
> >> It just has a bunch of phy devices strung out on that single MAC/MDIO.
> >
> > It has a number of built-in PHYs that typically appear on a MDIO bus,
> > whether that bus is the switch's internal MDIO bus, or another MDIO bus
> > (which could be provided with just two GPIOs) depends on how the switch
> > is connected to its management host.
> >
> > When the switch is interfaced via MDIO the switch also typically has a
> > MDIO interface called the pseudo-PHY which is how you can actually tap
> > into the control interface of the switch, as opposed to reading its
> > internal PHYs from the MDIO bus.
> >
> >> So in ACPI land it would still have a relationship similar to the one
> >> Andrew pointed out with his DT example where the eth0->mdio->phy are all
> >> contained in their physical parent. The phy in that case associated with
> >> the parent adapter would be the first direct decedent of the mdio, the
> >> switch itself could then be represented with another device, with a
> >> further string of device/phys representing the devices. (I dislike
> >> drawing acsii art, but if this isn't clear I will, its also worthwhile
> >> to look at the dpaa2 docs for how the mac/phys work on this device for
> >> contrast.).
> >
> > The eth0->mdio->phy relationship you describe is the simple case that
> > you are well aware of which is say what we have on the Raspberry Pi 4
> > with GENET and the external Broadcom PHY.
> >
> > For an Ethernet switch connected to an Ethernet MAC, we have 4 different
> > types of objects:
> >
> > - the Ethernet MAC which sits on its specific bus
> >
> > - the Ethernet switch which also sits on its specific bus
> >
> > - the built-in PHYs of the Ethernet switch which sit on whatever
> > bus/interface the switch provides to make them accessible
> >
> > - the specific bus controller that provides access to the Ethernet switch
> >
> > and this is a simplification that does not take into account Physical
> > Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet
> > land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable modules.
>
> Which is why I've stayed away from much of the switch discussion. There
> are a lot of edge cases to fall into, because for whatever reason
> networking seems to be unique in all this non-enumerable customization
> vs many other areas of the system. Storage, being an example i'm more
> familiar with which has very similar problems yet, somehow has managed
> to avoid much of this, despite having run on fabrics significantly more
> complex than basic ethernet (including running on a wide range of hot
> pluggable GBIC/SFP/QSFP/etc media layers).
>
> ACPI's "problem" here is that its strongly influenced by the "Plug and
> Play" revolution of the 1990's where the industry went from having
> humans describing hardware using machine readable languages, to hardware
> which was enumerable using standard protocols. ACPI's device
> descriptions are there as a crutch for the remaining non plug an play
> hardware in the system.
>
> So at a basic level, if your describing hardware in ACPI rather than
> abstracting it, that is a problem.
>
This is also my first run with ACPI and this discussion needs to be
brought back to the powers that be regarding sorting this out.  This
is where I see it from an Armada and Layerscape perspective.  This
isn't a silver bullet fix but the small things I think that need to be
done to move this forward.

From Microsoft's documentation.

Device dependencies

Typically, there are hardware dependencies between devices on a
particular platform. Windows requires that all such dependencies be
described so that it can ensure that all devices function correctly as
things change dynamically in the system (device power is removed,
drivers are stopped and started, and so on). In ACPI, dependencies
between devices are described in the following ways:

1) Namespace hierarchy. Any device that is a child device (listed as a
device within the namespace of another device) is dependent on the
parent device. For example, a USB HSIC device is dependent on the port
(parent) and controller (grandparent) it is connected to. Similarly, a
GPU device listed within the namespace of a system memory-management
unit (MMU) device is dependent on the MMU device.

2) Resource connections. Devices connected to GPIO or SPB controllers
are dependent on those controllers. This type of dependency is
described by the inclusion of Connection Resources in the device's
_CRS.

3) OpRegion dependencies. For ASL control methods that use OpRegions
to perform I/O, dependencies are not implicitly known by the operating
system because they are only determined during control method
evaluation. This issue is particularly applicable to GeneralPurposeIO
and GenericSerialBus OpRegions in which Plug and Play drivers provide
access to the region. To mitigate this issue, ACPI defines the
OpRegion Dependency (_DEP) object. _DEP should be used in any device
namespace in which an OpRegion (HW resource) is referenced by a
control method, and neither 1 nor 2 above already applies for the
referenced OpRegion's connection resource. For more information, see
section 6.5.8, "_DEP (Operation Region Dependencies)", of the ACPI 5.0
specification.

We can forget about 3 because even though _DEP would solve many of our
problems, and Intel has kind of used it for some of their
architectures, according to the ACPI spec it should not be used this
way.

1) can be achievable on some platforms like the LX2160a.  We have the
mcbin firmware which is the bus (the ACPI spec does allow you to
define a platform defined bus), which has MACs as the children, which
then can have phy's or SFP modules as their children.  This works okay
for enumeration and parenting but how do they talk?

This is where 2) comes into play.  The big problem is that MDIO isn't
designated as a SPB
(https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/simple-peripheral-bus--spb-)
We have GPIO, I2C, SPI, UART, MIPI and a couple of others.  While not
a silver bullet I think getting MDIO added to the spec would be the
next step forward to being able to implement this in Linux with
phylink / phylib in a sane manner.  Currently SFP definitions are
using the SPB to designate the various GPIO and I2C interfaces that
are needed to probe devices and handle interrupts.

The other alternatives is the ACPI maintainers agree on the _DSD
method (would be quickest and should be easy to migrate to SBP if MDIO
were adopter), or nothing is done at all (which I know seems to be a
popular opinion).

-Jon

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

[Calvin Johnson]

On Wed, Jul 29, 2020 at 10:43:34AM +0200, Jon Nettleton wrote:
> On Wed, Jul 29, 2020 at 4:53 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
> >
> > Hi,
> >
> > On 7/28/20 7:39 PM, Florian Fainelli wrote:
> > > On 7/28/2020 3:30 PM, Jeremy Linton wrote:
> > >> Hi,
> > >>
> > >> On 7/28/20 3:06 AM, Dan Callaghan wrote:
> > >>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00:
> > >>>> Now i could be wrong, but are Ethernet switches something you expect
> > >>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the
> > >>>> escape hatch?
> > >>>
> > >>> As an extra data point: right now I am working on an x86 embedded
> > >>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches.
> > >>> I have been watching this patch series with great interest, because
> > >>> right now there is no way for me to configure a complex switch topology
> > >>> in DSA without Device Tree.
> > >>
> > >> DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring
> > >> whether that NIC/MAC is actually hug off PCIe for the moment).
> > >
> > > There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches
> > > all supported within the driver framework right now.
> > >
> > >>
> > >> It just has a bunch of phy devices strung out on that single MAC/MDIO.
> > >
> > > It has a number of built-in PHYs that typically appear on a MDIO bus,
> > > whether that bus is the switch's internal MDIO bus, or another MDIO bus
> > > (which could be provided with just two GPIOs) depends on how the switch
> > > is connected to its management host.
> > >
> > > When the switch is interfaced via MDIO the switch also typically has a
> > > MDIO interface called the pseudo-PHY which is how you can actually tap
> > > into the control interface of the switch, as opposed to reading its
> > > internal PHYs from the MDIO bus.
> > >
> > >> So in ACPI land it would still have a relationship similar to the one
> > >> Andrew pointed out with his DT example where the eth0->mdio->phy are all
> > >> contained in their physical parent. The phy in that case associated with
> > >> the parent adapter would be the first direct decedent of the mdio, the
> > >> switch itself could then be represented with another device, with a
> > >> further string of device/phys representing the devices. (I dislike
> > >> drawing acsii art, but if this isn't clear I will, its also worthwhile
> > >> to look at the dpaa2 docs for how the mac/phys work on this device for
> > >> contrast.).
> > >
> > > The eth0->mdio->phy relationship you describe is the simple case that
> > > you are well aware of which is say what we have on the Raspberry Pi 4
> > > with GENET and the external Broadcom PHY.
> > >
> > > For an Ethernet switch connected to an Ethernet MAC, we have 4 different
> > > types of objects:
> > >
> > > - the Ethernet MAC which sits on its specific bus
> > >
> > > - the Ethernet switch which also sits on its specific bus
> > >
> > > - the built-in PHYs of the Ethernet switch which sit on whatever
> > > bus/interface the switch provides to make them accessible
> > >
> > > - the specific bus controller that provides access to the Ethernet switch
> > >
> > > and this is a simplification that does not take into account Physical
> > > Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet
> > > land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable modules.
> >
> > Which is why I've stayed away from much of the switch discussion. There
> > are a lot of edge cases to fall into, because for whatever reason
> > networking seems to be unique in all this non-enumerable customization
> > vs many other areas of the system. Storage, being an example i'm more
> > familiar with which has very similar problems yet, somehow has managed
> > to avoid much of this, despite having run on fabrics significantly more
> > complex than basic ethernet (including running on a wide range of hot
> > pluggable GBIC/SFP/QSFP/etc media layers).
> >
> > ACPI's "problem" here is that its strongly influenced by the "Plug and
> > Play" revolution of the 1990's where the industry went from having
> > humans describing hardware using machine readable languages, to hardware
> > which was enumerable using standard protocols. ACPI's device
> > descriptions are there as a crutch for the remaining non plug an play
> > hardware in the system.
> >
> > So at a basic level, if your describing hardware in ACPI rather than
> > abstracting it, that is a problem.
> >
> This is also my first run with ACPI and this discussion needs to be
> brought back to the powers that be regarding sorting this out.  This
> is where I see it from an Armada and Layerscape perspective.  This
> isn't a silver bullet fix but the small things I think that need to be
> done to move this forward.
> 
> From Microsoft's documentation.
> 
> Device dependencies
> 
> Typically, there are hardware dependencies between devices on a
> particular platform. Windows requires that all such dependencies be
> described so that it can ensure that all devices function correctly as
> things change dynamically in the system (device power is removed,
> drivers are stopped and started, and so on). In ACPI, dependencies
> between devices are described in the following ways:
> 
> 1) Namespace hierarchy. Any device that is a child device (listed as a
> device within the namespace of another device) is dependent on the
> parent device. For example, a USB HSIC device is dependent on the port
> (parent) and controller (grandparent) it is connected to. Similarly, a
> GPU device listed within the namespace of a system memory-management
> unit (MMU) device is dependent on the MMU device.
> 
> 2) Resource connections. Devices connected to GPIO or SPB controllers
> are dependent on those controllers. This type of dependency is
> described by the inclusion of Connection Resources in the device's
> _CRS.
> 
> 3) OpRegion dependencies. For ASL control methods that use OpRegions
> to perform I/O, dependencies are not implicitly known by the operating
> system because they are only determined during control method
> evaluation. This issue is particularly applicable to GeneralPurposeIO
> and GenericSerialBus OpRegions in which Plug and Play drivers provide
> access to the region. To mitigate this issue, ACPI defines the
> OpRegion Dependency (_DEP) object. _DEP should be used in any device
> namespace in which an OpRegion (HW resource) is referenced by a
> control method, and neither 1 nor 2 above already applies for the
> referenced OpRegion's connection resource. For more information, see
> section 6.5.8, "_DEP (Operation Region Dependencies)", of the ACPI 5.0
> specification.
> 
> We can forget about 3 because even though _DEP would solve many of our
> problems, and Intel has kind of used it for some of their
> architectures, according to the ACPI spec it should not be used this
> way.
> 
> 1) can be achievable on some platforms like the LX2160a.  We have the
> mcbin firmware which is the bus (the ACPI spec does allow you to
> define a platform defined bus), which has MACs as the children, which
> then can have phy's or SFP modules as their children.  This works okay
> for enumeration and parenting but how do they talk?
> 
> This is where 2) comes into play.  The big problem is that MDIO isn't
> designated as a SPB
> (https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/simple-peripheral-bus--spb-)
> We have GPIO, I2C, SPI, UART, MIPI and a couple of others.  While not
> a silver bullet I think getting MDIO added to the spec would be the
> next step forward to being able to implement this in Linux with
> phylink / phylib in a sane manner.  Currently SFP definitions are
> using the SPB to designate the various GPIO and I2C interfaces that
> are needed to probe devices and handle interrupts.
> 
> The other alternatives is the ACPI maintainers agree on the _DSD
> method (would be quickest and should be easy to migrate to SBP if MDIO
> were adopter), or nothing is done at all (which I know seems to be a
> popular opinion).
> 

Before other ACPI experts miss any further discussion let me add them to the
loop.

Hi ACPI maintainers,  please have a look at the discussion and some conclusions
in this thread:
https://lore.kernel.org/linux-acpi/20200715090400.4733-1-calvin.johnson@oss.nxp.com/T/#t

Discussion is around adding ACPI support into the PHY subsystem.

Thanks
Calvin

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

[Rafael J. Wysocki]

Hi Andrew,

On Tue, Jul 28, 2020 at 10:34 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Hi Everybody
>
> So i think it is time to try to bring this discussion to some sort of
> conclusion.
>
> No ACPI maintainer is willing to ACK any of these patches. Nor are
> they willing to NACK them.

Let's first clarify one thing: ACPI maintainers take care of the
generic code implementing the interactions between the OS and the
platform firmware in accordance with ACPI (which is an interface
specification to be precise).  They do not set rules on what
ACPI-related things device drivers can do.  Those rules are set in the
ACPI specification and other standard definitions (like PCI, USB,
etc.) and they just need to be followed.  So ACPI maintainers cannot
"bless" any new rule to be followed going forward.

An ACPI maintainer can tell you whether or not some driver code
follows the rules set by the ACPI specification (or conventions
related to using the ACPI support code in the kernel) and that's about
it.

In this particular case, a bit of ACPI-related code is there in the
last two patches and it doesn't look broken.  It uses the ACPI side of
the device properties API correctly AFAICS.

Also, from a slightly broader perspective, this patch series adds an
ability to look up certain device properties in the ACPI namespace.
That appears to be done in accordance with all of the rules set so
far, so there is nothing wrong with it in principle.

However, if those properties are never going to be supplied via ACPI
on any production systems, the code added in order to be able to
process them will turn out to be useless and I don't think anyone
wants useless code in the kernel.

So the real question is whether or not there will be production
systems in which those properties will be supplied via ACPI and I
cannot answer that question.  Therefore I cannot ACK the patches
(because I don't know whether or not the code added by them is going
to be useful), but I cannot NACK them either, because the code added
by them looks correct to me.

> ACPI maintainers simply don't want to get
> involved in making use of ACPI in networking.

That's not about making use of ACPI in networking in general (which
already happens in many ways), but about a specific use of ACPI for a
specific purpose related to networking.

> I personally don't have the knowledge to do ACPI correctly, review
> patches, point people in the right direction. I suspect the same can
> be said for the other PHY maintainers.
>
> Having said that, there is clearly a wish from vendors to make use of
> ACPI in the networking subsystem to describe hardware.
>
> How do we go forward?

Basically, the interested vendors need to agree on how exactly they
want ACPI to be used and come up with a specification setting the
rules to be followed by the platform firmware on the one side and by
the code in the kernel on the other side.

> For the moment, we will need to NACK all patches adding ACPI support
> to the PHY subsystem.
>
> Vendors who really do want to use ACPI, not device tree, probably
> need to get involved in standardisation. Vendors need to submit a
> proposal to UEFI and get it accepted.

The UEFI Forum maintains the ACPI specification itself (so changes to
the specification need to be accepted by it), but it is not uncommon
for a group of vendors (or even one vendor in some cases) to come up
with additional rules and specify them separately.  Of course,
involving the UEFI Forum may help to simplify things from the legal
and spec hosting perspective, but I don't think it is mandatory.

In the particular case at hand the additional rules may just be based
on the analogous DT bindings, but they need to be officially defined.

> Developers should try to engage with the ACPI maintainers and see
> if they can get them involved in networking. Patches with an
> Acked-by from an ACPI maintainer will be accepted, assuming they
> fulfil all the other usual requirements. But please don't submit
> patches until you do have an ACPI maintainer on board. We don't
> want to spamming the lists with NACKs all the time.

Well, do you ask for a PCI maintainer ACK on a patch adding a PCI
driver for a NIC as a rule?  If not, I don't see a reason for making
ACPI an exception.

Besides, you really should be asking for a spec the work is based on,
IMO, instead of asking for an ACPI maintainer ACK which is not going
to be sufficient if the former is missing anyway.

Thanks!

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

[Andrew Lunn]

> However, if those properties are never going to be supplied via ACPI
> on any production systems, the code added in order to be able to
> process them will turn out to be useless and I don't think anyone
> wants useless code in the kernel.
> 
> So the real question is whether or not there will be production
> systems in which those properties will be supplied via ACPI and I
> cannot answer that question.

Hi Rafael

I suspect we are going to have a lot of newbie ACPI questions over the
next few weeks/months as vendors and the PHY maintainers get up to
speed on all this.

In the device tree world, we would expect a patch as part of the
patchset to a device tree file somewhere under arch/*/boot/dts to make
use of any new property added. We then know it is used.

How does this work in the ACPI world?  How does somebody show they are
supplying the property in a production system?

> Basically, the interested vendors need to agree on how exactly they
> want ACPI to be used and come up with a specification setting the
> rules to be followed by the platform firmware on the one side and by
> the code in the kernel on the other side.

...

> Besides, you really should be asking for a spec the work is based on,
> IMO, instead of asking for an ACPI maintainer ACK which is not going
> to be sufficient if the former is missing anyway.

Could you point us towards real world example specs? Giving us a good
best practice example would likely help us to do this work. And reduce
the amount of work you need to do keeping the process going in the
correct direction.

Thanks
	Andrew

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

[Andrew Lunn]

> > > DT can be used on x86, and i suspect it is a much easier path of least
> > > resistance.
> >
> > And you can easily overlay Device Tree to an existing system by using
> > either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and
> > creating nodes on the fly.
> 
> Why do you need DT on a system that runs without it and Linux has all
> means to extend to cover a lot of stuff DT provides for other types of
> firmware nodes?

As i said, path of least resistance. It is here today, heavily used,
well understood by lots of network developers, has a very active
maintainer in the form of Rob Herring, and avoids 'showflakes' as
Florian likes to call it, so we are all sharing the same code,
providing a lot of testing and maintenance.

	  Andrew

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

[Grant Likely]

On 31/07/2020 16:14, Andrew Lunn wrote:
>>>> DT can be used on x86, and i suspect it is a much easier path of least
>>>> resistance.
>>>
>>> And you can easily overlay Device Tree to an existing system by using
>>> either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and
>>> creating nodes on the fly.
>>
>> Why do you need DT on a system that runs without it and Linux has all
>> means to extend to cover a lot of stuff DT provides for other types of
>> firmware nodes?
> 
> As i said, path of least resistance. It is here today, heavily used,
> well understood by lots of network developers, has a very active
> maintainer in the form of Rob Herring, and avoids 'showflakes' as
> Florian likes to call it, so we are all sharing the same code,
> providing a lot of testing and maintenance.
> 
> 	  Andrew

Hi Andrew,

I'm just coming into this thread now. With my alumni DT-maintainer had 
on I think that trying to use ACPI & DT on the same system is the worst 
of both worlds. Trying to do so makes the solution far more complicated 
than either an ACPI-only or DT-only approach. There is no good way for 
references between DT & ACPI nodes. I have serious doubts about the 
reliability of the dynamic DT code in the kernel. Perhaps most 
problematic is it excludes platform specific data from the ACPI 
description provided by firmware, which means platform-specific data 
needs to be shipped with the OS. Rather defeats the whole point of 
firmware providing the platform description. An ACPI solution is 
absolutely needed.

Regarding this specific series, I think it is approximately the right 
approach. I have some specific concerns that I've talked with Calvin 
about and I'm going to post as replies to the individual patches. My 
most significant concern is the reference from the ACPI MAC node to the 
MDIO node, which makes little sense. The MAC should have a reference to 
the PHY node.

There have been other issues raised in this thread. I'm going to go back 
and respond to a few of those points in separate emails, but as a larger 
issue I think there is a fair bit of misunderstanding on what ACPI does 
and does not do, and how much is expected to be standardized in ACPI 
specs. In the ACPI world the typical model is the firmware/platform 
vendor decides what data to put into the ACPI nodes that works for them, 
and then the OS just has to deal with it. Linux typically never gets a 
choice about what goes into ACPI nodes.

Already, threads like this one are setting the bar *far* higher on ACPI 
schema than has ever been done before. I do think it is right to be 
asking for a common data model for describing PHY connections. Lining 
the model up with the DT PHY model is also valuable because we can use 
common code. I also think as first through the door, what gets accepted 
(after review) for the layerscape platforms here should become the 
defacto standard that other vendors are expected to adopt, and I have 
very high confidence that it will be acceptable because it follow the 
pattern already used in devicetree.

Cheers,
g.

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

[Grant Likely]

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

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

> +	    })
> +	  } // 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
> +	}
> 

Re: [net-next PATCH v7 0/6] ACPI support for dpaa2 MAC driver.

[Grant Likely]

On 15/07/2020 10:03, Calvin Johnson wrote:
>   This patch series provides ACPI support for dpaa2 MAC driver.
>   This also introduces ACPI mechanism to get PHYs registered on a
>   MDIO bus and provide them to be connected to MAC.
> 
>   Patch "net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver" depends on
>   https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git/commit/?h=acpi/for-next&id=c279c4cf5bcd3c55b4fb9709d9036cd1bfe3beb8
>   Remaining patches are independent of the above patch and can be applied without
>   any issues.
> 
>   Device Tree can be tested on LX2160A-RDB with the below change which is also
> available in the above referenced patches:

Hi Calvin,

In principle, I agree with adding PHY linkage to ACPI, and I sent a 
comment about how the PHYs should be referenced. Unfortunately changing 
that details requires pretty much the entire series to be rewritten 
(sorry!). I won't do any detailed review on patches 2-6 until I see the 
next version.

g.


> 
> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> @@ -931,6 +931,7 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
>          if (error < 0)
>                  goto error_cleanup_mc_io;
> 
> +       mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
>          mc->root_mc_bus_dev = mc_bus_dev;
>          return 0;
> 
> 
> Changes in v7:
> - remove unnecessary -ve check for u32 var
> - assign flags to phy_dev
> 
> Changes in v6:
> - change device_mdiobus_register() parameter position
> - improve documentation
> - change device_mdiobus_register() parameter position
> - clean up phylink_fwnode_phy_connect()
> 
> Changes in v5:
> - add description
> - clean up if else
> - rename phy_find_by_fwnode() to phy_find_by_mdio_handle()
> - add docment for phy_find_by_mdio_handle()
> - error out DT in phy_find_by_mdio_handle()
> - clean up err return
> - return -EINVAL for invalid fwnode
> 
> Changes in v4:
> - release fwnode_mdio after use
> - return ERR_PTR instead of NULL
> - introduce device_mdiobus_register()
> 
> Changes in v3:
> - cleanup based on v2 comments
> - Added description for more properties
> - Added MDIO node DSDT entry
> - introduce fwnode_mdio_find_bus()
> - renamed and improved phy_find_by_fwnode()
> - cleanup based on v2 comments
> - move code into phylink_fwnode_phy_connect()
> 
> Changes in v2:
> - clean up dpaa2_mac_get_node()
> - introduce find_phy_device()
> - use acpi_find_child_device()
> 
> Calvin Johnson (6):
>    Documentation: ACPI: DSD: Document MDIO PHY
>    net: phy: introduce device_mdiobus_register()
>    net/fsl: use device_mdiobus_register()
>    net: phy: introduce phy_find_by_mdio_handle()
>    phylink: introduce phylink_fwnode_phy_connect()
>    net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
> 
>   Documentation/firmware-guide/acpi/dsd/phy.rst | 90 +++++++++++++++++++
>   .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 70 ++++++++-------
>   drivers/net/ethernet/freescale/xgmac_mdio.c   |  3 +-
>   drivers/net/phy/mdio_bus.c                    | 51 +++++++++++
>   drivers/net/phy/phy_device.c                  | 40 +++++++++
>   drivers/net/phy/phylink.c                     | 32 +++++++
>   include/linux/mdio.h                          |  1 +
>   include/linux/phy.h                           |  2 +
>   include/linux/phylink.h                       |  3 +
>   9 files changed, 260 insertions(+), 32 deletions(-)
>   create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
> 

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

[Calvin Johnson]

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

Re: [net-next PATCH v7 0/6] ACPI support for dpaa2 MAC driver.

[Calvin Johnson]

Hi Grant,

On Fri, Sep 25, 2020 at 02:39:21PM +0100, Grant Likely wrote:
> On 15/07/2020 10:03, Calvin Johnson wrote:
> >   This patch series provides ACPI support for dpaa2 MAC driver.
> >   This also introduces ACPI mechanism to get PHYs registered on a
> >   MDIO bus and provide them to be connected to MAC.
> > 
> >   Patch "net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver" depends on
> >   https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git/commit/?h=acpi/for-next&id=c279c4cf5bcd3c55b4fb9709d9036cd1bfe3beb8
> >   Remaining patches are independent of the above patch and can be applied without
> >   any issues.
> > 
> >   Device Tree can be tested on LX2160A-RDB with the below change which is also
> > available in the above referenced patches:
> 
> Hi Calvin,
> 
> In principle, I agree with adding PHY linkage to ACPI, and I sent a comment
> about how the PHYs should be referenced. Unfortunately changing that details
> requires pretty much the entire series to be rewritten (sorry!). I won't do
> any detailed review on patches 2-6 until I see the next version.

Understood. I'll rework on the patches and send a new series.

Thanks
Calvin

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

[Calvin Johnson]

Hi Grant,

On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote:
> > +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.

I'm unable to drop this as the xgmac_mdio driver relies on this variable to
change the io access to little-endian. Default is big-endian.
Please see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55

Thanks
Calvin

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

[Andrew Lunn]

On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote:
> Hi Grant,
> 
> On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote:
> > > +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.
> 
> I'm unable to drop this as the xgmac_mdio driver relies on this variable to
> change the io access to little-endian. Default is big-endian.
> Please see:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55

Hi Calvin

Are we talking about the bus controller endiannes, or the CPU
endianness?

If we are talking about the CPU endiannes, are you plan on supporting
any big endian platforms using ACPI? If not, just hard code it.
Newbie ACPI question: Does ACPI even support big endian CPUs, given
its x86 origins?

If this is the bus controller endianness, are all the SoCs you plan to
support via ACPI the same endianness? If they are all the same, you
can hard code it.

To some extent, this should be a moot point, assuming sane
hardware. Generally, the bus endian is fixed. It is either native, or
like PCI, little endian. The CPU endian is what can change. But in
general, once you figure out what you have, there is an IO macro which
will do the right thing without any configuration.

     Andrew

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

[Andy Shevchenko]

On Tue, Sep 29, 2020 at 4:43 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote:
> > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote:

...

> Newbie ACPI question: Does ACPI even support big endian CPUs, given
> its x86 origins?

I understand the newbie part, but can you elaborate what did you mean
under 'support'?
To me it sounds like 'network stack was developed for BE CPUs, does it
support LE ones?'

-- 
With Best Regards,
Andy Shevchenko

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

[Andrew Lunn]

On Tue, Sep 29, 2020 at 04:55:40PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 29, 2020 at 4:43 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote:
> > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote:
> 
> ...
> 
> > Newbie ACPI question: Does ACPI even support big endian CPUs, given
> > its x86 origins?
> 
> I understand the newbie part, but can you elaborate what did you mean
> under 'support'?
> To me it sounds like 'network stack was developed for BE CPUs, does it
> support LE ones?'

Does ACPI define the endianness of its tables? Is it written in the
standard that they should be little endian?  Does Tianocore, or any
other implementations, have the needed le32_to_cpu() calls so that
they can boot on a big endian CPU? Does it have a standardized way of
saying a device is big endian, swap words around if appropriate when
doing IO?

Is it feasible to boot an ARM system big endian? Can i boot the same
system little endian? The CPU should be able to do it, but are the
needed things in the ACPI specification and implementation to allow
it?

	Andrew

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

[Arnd Bergmann]

On Tue, Sep 29, 2020 at 3:44 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote:
> > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote:
> > > > +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.
> >
> > I'm unable to drop this as the xgmac_mdio driver relies on this variable to
> > change the io access to little-endian. Default is big-endian.
> > Please see:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55
>
> Hi Calvin
>
> Are we talking about the bus controller endiannes, or the CPU
> endianness?
>
> If we are talking about the CPU endiannes, are you plan on supporting
> any big endian platforms using ACPI? If not, just hard code it.
> Newbie ACPI question: Does ACPI even support big endian CPUs, given
> its x86 origins?

IIRC both UEFI and ACPI define only little-endian data structures.
The code does not attempt to convert these into CPU endianness
at the moment.  In theory it could be changed to support either, but
this seems non-practical for the UEFI runtime services that require
calling into firmware code in little-endian mode.

> If this is the bus controller endianness, are all the SoCs you plan to
> support via ACPI the same endianness? If they are all the same, you
> can hard code it.

NXP has a bunch of SoCs that reuse the same on-chip devices but
change the endianness between them based on what the chip
designers guessed the OS would want, which is why the drivers
usually support both register layouts and switch at runtime.
Worse, depending on which SoC was the first to get a DT binding
for a particular NXP on-chip device, the default endianness is
different, and there is either a "big-endian" or "little-endian"
override in the binding.

I would guess that for modern NXP chips that you might boot with
ACPI the endianness is always wired the same way, but I
understand the caution when they have been burned by this
problem before.

       Arnd

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

[Andy Shevchenko]

On Tue, Sep 29, 2020 at 5:32 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Sep 29, 2020 at 04:55:40PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 29, 2020 at 4:43 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote:
> > > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote:
> >
> > ...
> >
> > > Newbie ACPI question: Does ACPI even support big endian CPUs, given
> > > its x86 origins?
> >
> > I understand the newbie part, but can you elaborate what did you mean
> > under 'support'?
> > To me it sounds like 'network stack was developed for BE CPUs, does it
> > support LE ones?'
>
> Does ACPI define the endianness of its tables? Is it written in the
> standard that they should be little endian?

5.2:
"All numeric values in ACPI-defined tables, blocks, and structures are
always encoded in little endian
format. Signature values are stored as fixed-length strings."

>  Does Tianocore, or any
> other implementations, have the needed le32_to_cpu() calls so that
> they can boot on a big endian CPU?

Not of my knowledge.

> Does it have a standardized way of
> saying a device is big endian, swap words around if appropriate when
> doing IO?

I guess this is not applicable to ACPI. Does Linux have a standardized way?
So, what did you mean under doing I/O? I mean in which context?

> Is it feasible to boot an ARM system big endian?

Not an ARM guy.

> Can i boot the same
> system little endian? The CPU should be able to do it, but are the
> needed things in the ACPI specification and implementation to allow
> it?

-- 
With Best Regards,
Andy Shevchenko

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

[Andrew Lunn]

> IIRC both UEFI and ACPI define only little-endian data structures.
> The code does not attempt to convert these into CPU endianness
> at the moment.  In theory it could be changed to support either, but
> this seems non-practical for the UEFI runtime services that require
> calling into firmware code in little-endian mode.

Hi Arnd

Thanks for the info. So we can assume the CPU is little endian.  That
helps narrow down the problem.

> > If this is the bus controller endianness, are all the SoCs you plan to
> > support via ACPI the same endianness? If they are all the same, you
> > can hard code it.
> 
> NXP has a bunch of SoCs that reuse the same on-chip devices but
> change the endianness between them based on what the chip
> designers guessed the OS would want, which is why the drivers
> usually support both register layouts and switch at runtime.
> Worse, depending on which SoC was the first to get a DT binding
> for a particular NXP on-chip device, the default endianness is
> different, and there is either a "big-endian" or "little-endian"
> override in the binding.
> 
> I would guess that for modern NXP chips that you might boot with
> ACPI the endianness is always wired the same way, but I
> understand the caution when they have been burned by this
> problem before.

So it might depend on if NXP is worried it might flip the endianness
of the synthesis of the MDIO controller at some point for devices it
wants to support using ACPI?

Does ACPI have a standard way of declaring the endianness of a device?
We don't really want to put the DT parameter in ACPI, we want to use
the ACPI way of doing it.

    Thanks
	Andrew

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

[Andrew Lunn]

> > Does it have a standardized way of
> > saying a device is big endian, swap words around if appropriate when
> > doing IO?
> 
> I guess this is not applicable to ACPI. Does Linux have a standardized way?

DT does. You add the property 'little-endian' to indicate you should
do IO to the device using little endian.

> So, what did you mean under doing I/O? I mean in which context?

NXP can synthesise the MDIO controller either big endian, or little
endian. So on some SoCs we need to tell the driver to talk to the
hardware using big endian accesses. On other SoCs we need to tell the
driver to talk to the hardware using little endian.

Is there a standard way in ACPI to describe this?

   Andrew

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

[Arnd Bergmann]

On Tue, Sep 29, 2020 at 4:49 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Sep 29, 2020 at 5:32 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > On Tue, Sep 29, 2020 at 04:55:40PM +0300, Andy Shevchenko wrote:
> >  Does Tianocore, or any
> > other implementations, have the needed le32_to_cpu() calls so that
> > they can boot on a big endian CPU?
>
> Not of my knowledge.

> > Is it feasible to boot an ARM system big endian?
>
> Not an ARM guy.

Most CPUs these days support both big-endian and little-endian,
and either allow code to switch between the two modes at runtime
or are stateless in the way that you have two sets of load/store
instructions, making endianness purely a compiler construct (see
also: Intel's icc compiler has a big-endian mode using the MOVBE
instruction).

For Arm kernels, we assume that the firmware is little-endian, but
you can build a big-endian kernel that switches into big-endian
mode before doing anything else. As I said, I don't think that will
ever be used with UEFI (and ACPI, by extension), since it would
be a ton of work and few users care about big-endian kernels.

    Arnd

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

[Grant Likely]

On 29/09/2020 14:43, Andrew Lunn wrote:
> On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote:
>> Hi Grant,
>>
>> On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote:
>>>> +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.
>>
>> I'm unable to drop this as the xgmac_mdio driver relies on this variable to
>> change the io access to little-endian. Default is big-endian.
>> Please see:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55
> 
> Hi Calvin
> 
> Are we talking about the bus controller endiannes, or the CPU
> endianness?

This is orthogonal to the MDIO bus issue. This is a legacy of the xgmac 
IP block originating in PowerPC platforms with a big-endian bus wiring. 
The flag here tells the driver to use little endian when accessing MMIO 
registers.

> If we are talking about the CPU endiannes, are you plan on supporting
> any big endian platforms using ACPI? If not, just hard code it.
> Newbie ACPI question: Does ACPI even support big endian CPUs, given
> its x86 origins? >
> If this is the bus controller endianness, are all the SoCs you plan to
> support via ACPI the same endianness? If they are all the same, you
> can hard code it.

I would agree. The ACPI and DT probe paths are different. It would be 
easy to automatically set the little-endian flag by default when xgmac 
is described via ACPI.

g.

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

[Grant Likely]

On 29/09/2020 15:59, Andrew Lunn wrote:
>> IIRC both UEFI and ACPI define only little-endian data structures.
>> The code does not attempt to convert these into CPU endianness
>> at the moment.  In theory it could be changed to support either, but
>> this seems non-practical for the UEFI runtime services that require
>> calling into firmware code in little-endian mode.
> 
> Hi Arnd
> 
> Thanks for the info. So we can assume the CPU is little endian.  That
> helps narrow down the problem.
> 
>>> If this is the bus controller endianness, are all the SoCs you plan to
>>> support via ACPI the same endianness? If they are all the same, you
>>> can hard code it.
>>
>> NXP has a bunch of SoCs that reuse the same on-chip devices but
>> change the endianness between them based on what the chip
>> designers guessed the OS would want, which is why the drivers
>> usually support both register layouts and switch at runtime.
>> Worse, depending on which SoC was the first to get a DT binding
>> for a particular NXP on-chip device, the default endianness is
>> different, and there is either a "big-endian" or "little-endian"
>> override in the binding.
>>
>> I would guess that for modern NXP chips that you might boot with
>> ACPI the endianness is always wired the same way, but I
>> understand the caution when they have been burned by this
>> problem before.
> 
> So it might depend on if NXP is worried it might flip the endianness
> of the synthesis of the MDIO controller at some point for devices it
> wants to support using ACPI?
> 
> Does ACPI have a standard way of declaring the endianness of a device?
> We don't really want to put the DT parameter in ACPI, we want to use
> the ACPI way of doing it.

No, and it doesn't need one. If a device is wired up big-endian, then it 
is between the device driver and the device. The OS, and the ACPI 
framework doesn't come into play other than providing a generic way of 
encoding data useful to the device driver. Encoding endian hasn't been a 
common problem, and the tools are already there to deal with it when it is.

g.

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

[Calvin Johnson]

On Tue, Sep 29, 2020 at 04:53:47PM +0100, Grant Likely wrote:
> 
> 
> On 29/09/2020 14:43, Andrew Lunn wrote:
> > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote:
> > > Hi Grant,
> > > 
> > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote:
> > > > > +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.
> > > 
> > > I'm unable to drop this as the xgmac_mdio driver relies on this variable to
> > > change the io access to little-endian. Default is big-endian.
> > > Please see:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55
> > 
> > Hi Calvin
> > 
> > Are we talking about the bus controller endiannes, or the CPU
> > endianness?
> 
> This is orthogonal to the MDIO bus issue. This is a legacy of the xgmac IP
> block originating in PowerPC platforms with a big-endian bus wiring. The
> flag here tells the driver to use little endian when accessing MMIO
> registers.
> 
> > If we are talking about the CPU endiannes, are you plan on supporting
> > any big endian platforms using ACPI? If not, just hard code it.
> > Newbie ACPI question: Does ACPI even support big endian CPUs, given
> > its x86 origins? >
> > If this is the bus controller endianness, are all the SoCs you plan to
> > support via ACPI the same endianness? If they are all the same, you
> > can hard code it.
> 
> I would agree. The ACPI and DT probe paths are different. It would be easy
> to automatically set the little-endian flag by default when xgmac is
> described via ACPI.

Thanks Andrew and Grant for this suggestion. Yes, this is an easy way to solve
this problem. Will do that.

Regards
Calvin

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

[Calvin Johnson]

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@oss.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.
+
+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},
+	      }
+	    })
+	  } // 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
+	}
-- 
2.17.1