[PATCH 0/6] Add ACPI bindings to the genet

This patch series allows the BCM GENET, as used on the RPi4,
to attach when booted in an ACPI environment. The DSDT entry to
trigger this is seen below. Of note, the first patch adds a
small extension to the mdio layer which allows drivers to find
the mii_bus without firmware assistance. The fifth patch in
the set retrieves the MAC address from the umac registers
rather than carrying it directly in the DSDT. This of course
requires the firmware to pre-program it, so we continue to fall
back on a random one if it appears to be garbage.

v1 -> v2:
     add a core routine mdio_find_bus(), then use it to attach
         to the phy rather than hard-coding the id.
     Broke initial ACPI support patch into 3 parts, two for bcmmii.c
     Address some review comments from Florian
     Lower the severity of a few dev_warns that happen all the time.

+    Device (ETH0)
+    {
+      Name (_HID, "BCM6E4E")
+      Name (_UID, 0)
+      Name (_CCA, 0x0)
+      Method (_STA)
+      {
+        Return (0xf)
+      }
+      Method (_CRS, 0x0, Serialized)
+      {
+        Name (RBUF, ResourceTemplate ()
+        {
+          Memory32Fixed (ReadWrite, 0xFd580000, 0x10000, )
+          Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 0xBD }
+          Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 0xBE }
+        })
+        Return (RBUF)
+      }
+      Name (_DSD, Package () {
+        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+          Package () {
+          Package () { "phy-mode", "rgmii" },
+        }
+      })
+    }
+

Jeremy Linton (6):
  mdio_bus: Add generic mdio_find_bus()
  net: bcmgenet: refactor phy mode configuration
  net: bcmgenet: enable automatic phy discovery
  net: bcmgenet: Initial bcmgenet ACPI support
  net: bcmgenet: Fetch MAC address from the adapter
  net: bcmgenet: reduce severity of missing clock warnings

 .../net/ethernet/broadcom/genet/bcmgenet.c    | 66 +++++++++++-----
 drivers/net/ethernet/broadcom/genet/bcmmii.c  | 79 +++++++++++++------
 drivers/net/phy/mdio_bus.c                    | 17 ++++
 include/linux/phy.h                           |  1 +
 4 files changed, 122 insertions(+), 41 deletions(-)

-- 
2.24.1

The DT phy mode is similar to what we want for ACPI
lets factor it out of the of path, and change the
of_ call to device_. Further if the phy-mode property
cannot be found instead of failing the driver load lets
just default it to RGMII.

Signed-off-by: Jeremy Linton <redacted>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 39 +++++++++++---------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 6392a2530183..2049f8218589 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c

@@ -477,12 +477,30 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
 	return ret;
 }
 
+static int bcmgenet_phy_interface_init(struct bcmgenet_priv *priv)
+{
+	struct device *kdev = &priv->pdev->dev;
+
+	priv->phy_interface = device_get_phy_mode(kdev);
+	if (priv->phy_interface < 0) {
+		dev_dbg(kdev, "invalid PHY mode property\n");
+		priv->phy_interface = PHY_INTERFACE_MODE_RGMII;
+	}
+
+	/* We need to specifically look up whether this PHY interface is internal
+	 * or not *before* we even try to probe the PHY driver over MDIO as we
+	 * may have shut down the internal PHY for power saving purposes.
+	 */
+	if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL)
+		priv->internal_phy = true;
+
+	return 0;
+}
+
 static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
 {
 	struct device_node *dn = priv->pdev->dev.of_node;
-	struct device *kdev = &priv->pdev->dev;
 	struct phy_device *phydev;
-	phy_interface_t phy_mode;
 	int ret;
 
 	/* Fetch the PHY phandle */

@@ -500,23 +518,10 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
 	}
 
 	/* Get the link mode */
-	ret = of_get_phy_mode(dn, &phy_mode);
-	if (ret) {
-		dev_err(kdev, "invalid PHY mode property\n");
-		return ret;
-	}
-
-	priv->phy_interface = phy_mode;
-
-	/* We need to specifically look up whether this PHY interface is internal
-	 * or not *before* we even try to probe the PHY driver over MDIO as we
-	 * may have shut down the internal PHY for power saving purposes.
-	 */
-	if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL)
-		priv->internal_phy = true;
+	bcmgenet_phy_interface_init(priv);
 
 	/* Make sure we initialize MoCA PHYs with a link down */
-	if (phy_mode == PHY_INTERFACE_MODE_MOCA) {
+	if (priv->phy_interface == PHY_INTERFACE_MODE_MOCA) {
 		phydev = of_phy_find_device(dn);
 		if (phydev) {
 			phydev->link = 0;

-- 
2.24.1

It appears most ethernet drivers follow one of two main strategies
for mdio bus/phy management. A monolithic model where the net driver
itself creates, probes and uses the phy, and one where an external
mdio/phy driver instantiates the mdio bus/phy and the net driver
only attaches to a known phy. Usually in this latter model the phys
are discovered via DT relationships or simply phy name/address
hardcoding.

This is a shame because modern well behaved mdio buses are self
describing and can be probed. The mdio layer itself is fully capable
of this, yet there isn't a clean way for a standalone net driver
to attach and enumerate the discovered devices. This is because
outside of of_mdio_find_bus() there isn't a straightforward way
to acquire the mii_bus pointer.

So, lets add a mdio_find_bus which can return the mii_bus based
only on its name.

Signed-off-by: Jeremy Linton <redacted>
---
 drivers/net/phy/mdio_bus.c | 17 +++++++++++++++++
 include/linux/phy.h        |  1 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 229e480179ff..46dfb112fd04 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c

@@ -260,6 +260,23 @@ static struct class mdio_bus_class = {
 	.dev_release	= mdiobus_release,
 };
 
+/**
+ * mdio_find_bus - Given the name of a mdiobus, find the mii_bus.
+ * @mdio_bus_np: Pointer to 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_deviced'ed once the bus is finished with.
+ */
+struct mii_bus *mdio_find_bus(const char *mdio_name)
+{
+	struct device *d;
+
+	d = class_find_device_by_name(&mdio_bus_class, mdio_name);
+	return d ? to_mii_bus(d) : NULL;
+}
+EXPORT_SYMBOL(mdio_find_bus);
+
 #if IS_ENABLED(CONFIG_OF_MDIO)
 /**
  * of_mdio_find_bus - Given an mii_bus node, find the mii_bus.

diff --git a/include/linux/phy.h b/include/linux/phy.h
index dd4a91f1feaa..5700dd4dbb6f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -273,6 +273,7 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct device *dev)
 	return devm_mdiobus_alloc_size(dev, 0);
 }
 
+struct mii_bus *mdio_find_bus(const char *mdio_name);
 void devm_mdiobus_free(struct device *dev, struct mii_bus *bus);
 struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
 

-- 
2.24.1

The unimac mdio driver falls back to scanning the
entire bus if its given an appropriate mask. In ACPI
mode we expect that the system is well behaved and
conforms to recent versions of the specification.

We then utilize phy_find_first(), and
phy_connect_direct() to find and attach to the
discovered phy during net_device open.

Signed-off-by: Jeremy Linton <redacted>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 40 +++++++++++++++++---
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 2049f8218589..f3271975b375 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c

@@ -5,7 +5,7 @@
  * Copyright (c) 2014-2017 Broadcom
  */
 
-
+#include <linux/acpi.h>
 #include <linux/types.h>
 #include <linux/delay.h>
 #include <linux/wait.h>

@@ -311,7 +311,9 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
 int bcmgenet_mii_probe(struct net_device *dev)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
-	struct device_node *dn = priv->pdev->dev.of_node;
+	struct device *kdev = &priv->pdev->dev;
+	struct device_node *dn = kdev->of_node;
+
 	struct phy_device *phydev;
 	u32 phy_flags = 0;
 	int ret;

@@ -334,7 +336,27 @@ int bcmgenet_mii_probe(struct net_device *dev)
 			return -ENODEV;
 		}
 	} else {
-		phydev = dev->phydev;
+		if (has_acpi_companion(kdev)) {
+			char mdio_bus_id[MII_BUS_ID_SIZE];
+			struct mii_bus *unimacbus;
+
+			snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
+				 UNIMAC_MDIO_DRV_NAME, priv->pdev->id);
+
+			unimacbus = mdio_find_bus(mdio_bus_id);
+			if (!unimacbus) {
+				pr_err("Unable to find mii\n");
+				return -ENODEV;
+			}
+			phydev = phy_find_first(unimacbus);
+			put_device(&unimacbus->dev);
+			if (!phydev) {
+				pr_err("Unable to find PHY\n");
+				return -ENODEV;
+			}
+		} else {
+			phydev = dev->phydev;
+		}
 		phydev->dev_flags = phy_flags;
 
 		ret = phy_connect_direct(dev, phydev, bcmgenet_mii_setup,

@@ -455,9 +477,12 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
 	/* Retain this platform_device pointer for later cleanup */
 	priv->mii_pdev = ppdev;
 	ppdev->dev.parent = &pdev->dev;
-	ppdev->dev.of_node = bcmgenet_mii_of_find_mdio(priv);
-	if (pdata)
+	if (dn)
+		ppdev->dev.of_node = bcmgenet_mii_of_find_mdio(priv);
+	else if (pdata)
 		bcmgenet_mii_pdata_init(priv, &ppd);
+	else
+		ppd.phy_mask = ~0;
 
 	ret = platform_device_add_resources(ppdev, &res, 1);
 	if (ret)

@@ -586,10 +611,13 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
 
 static int bcmgenet_mii_bus_init(struct bcmgenet_priv *priv)
 {
-	struct device_node *dn = priv->pdev->dev.of_node;
+	struct device *kdev = &priv->pdev->dev;
+	struct device_node *dn = kdev->of_node;
 
 	if (dn)
 		return bcmgenet_mii_of_init(priv);
+	else if (has_acpi_companion(kdev))
+		return bcmgenet_phy_interface_init(priv);
 	else
 		return bcmgenet_mii_pd_init(priv);
 }

-- 
2.24.1

ARM/ACPI machines should utilize self describing hardware
when possible. The MAC address on the BCMGENET can be
read from the adapter if a full featured firmware has already
programmed it. Lets try using the address already programmed,
if it appears to be valid.

It should be noted that while we move the macaddr logic below
the clock and power logic in the driver, none of that code will
ever be active in an ACPI environment as the device will be
attached to the acpi power domain, and brought to full power with
all clocks enabled immediately before the device probe routine is
called.

Signed-off-by: Jeremy Linton <redacted>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../net/ethernet/broadcom/genet/bcmgenet.c    | 43 ++++++++++++++-----
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index c736700f829e..dbf96fc96689 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

@@ -2779,6 +2779,21 @@ static void bcmgenet_set_hw_addr(struct bcmgenet_priv *priv,
 	bcmgenet_umac_writel(priv, (addr[4] << 8) | addr[5], UMAC_MAC1);
 }
 
+static void bcmgenet_get_hw_addr(struct bcmgenet_priv *priv,
+				 unsigned char *addr)
+{
+	u32 addr_tmp;
+
+	addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC0);
+	addr[0] = addr_tmp >> 24;
+	addr[1] = (addr_tmp >> 16) & 0xff;
+	addr[2] = (addr_tmp >>	8) & 0xff;
+	addr[3] = addr_tmp & 0xff;
+	addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC1);
+	addr[4] = (addr_tmp >> 8) & 0xff;
+	addr[5] = addr_tmp & 0xff;
+}
+
 /* Returns a reusable dma control register value */
 static u32 bcmgenet_dma_disable(struct bcmgenet_priv *priv)
 {

@@ -3509,11 +3524,6 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	}
 	priv->wol_irq = platform_get_irq_optional(pdev, 2);
 
-	if (dn)
-		macaddr = of_get_mac_address(dn);
-	else if (pd)
-		macaddr = pd->mac_address;
-
 	priv->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->base)) {
 		err = PTR_ERR(priv->base);

@@ -3524,12 +3534,6 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	SET_NETDEV_DEV(dev, &pdev->dev);
 	dev_set_drvdata(&pdev->dev, dev);
-	if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) {
-		dev_warn(&pdev->dev, "using random Ethernet MAC\n");
-		eth_hw_addr_random(dev);
-	} else {
-		ether_addr_copy(dev->dev_addr, macaddr);
-	}
 	dev->watchdog_timeo = 2 * HZ;
 	dev->ethtool_ops = &bcmgenet_ethtool_ops;
 	dev->netdev_ops = &bcmgenet_netdev_ops;

@@ -3601,6 +3605,23 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	    !strcasecmp(phy_mode_str, "internal"))
 		bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
 
+	if (dn)
+		macaddr = of_get_mac_address(dn);
+	else if (pd)
+		macaddr = pd->mac_address;
+
+	if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) {
+		if (has_acpi_companion(&pdev->dev))
+			bcmgenet_get_hw_addr(priv, dev->dev_addr);
+
+		if (!is_valid_ether_addr(dev->dev_addr)) {
+			dev_warn(&pdev->dev, "using random Ethernet MAC\n");
+			eth_hw_addr_random(dev);
+		}
+	} else {
+		ether_addr_copy(dev->dev_addr, macaddr);
+	}
+
 	reset_umac(priv);
 
 	err = bcmgenet_mii_init(dev);

-- 
2.24.1

If one types "failed to get enet clock" or similar into google
there are ~370k hits. The vast majority are people debugging
problems unrelated to this adapter, or bragging about their
rpi's. Given that its not a fatal situation with common DT based
systems, lets reduce the severity so people aren't seeing failure
messages in everyday operation.

Signed-off-by: Jeremy Linton <redacted>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index dbf96fc96689..115977e2edef 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

@@ -3572,7 +3572,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	priv->clk = devm_clk_get(&priv->pdev->dev, "enet");
 	if (IS_ERR(priv->clk)) {
-		dev_warn(&priv->pdev->dev, "failed to get enet clock\n");
+		dev_dbg(&priv->pdev->dev, "failed to get enet clock\n");
 		priv->clk = NULL;
 	}
 

@@ -3588,13 +3588,13 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	priv->clk_wol = devm_clk_get(&priv->pdev->dev, "enet-wol");
 	if (IS_ERR(priv->clk_wol)) {
-		dev_warn(&priv->pdev->dev, "failed to get enet-wol clock\n");
+		dev_dbg(&priv->pdev->dev, "failed to get enet-wol clock\n");
 		priv->clk_wol = NULL;
 	}
 
 	priv->clk_eee = devm_clk_get(&priv->pdev->dev, "enet-eee");
 	if (IS_ERR(priv->clk_eee)) {
-		dev_warn(&priv->pdev->dev, "failed to get enet-eee clock\n");
+		dev_dbg(&priv->pdev->dev, "failed to get enet-eee clock\n");
 		priv->clk_eee = NULL;
 	}
 

-- 
2.24.1

The rpi4 is capable of booting in ACPI mode with the latest
edk2-platform commits. As such it would be helpful if the genet
platform device were usable.

To achieve this we add a new MODULE_DEVICE_TABLE, and convert
a few dt specific methods to their generic device_ calls. Until
the next patch, ACPI based machines will fallback on random
mac addresses.

Signed-off-by: Jeremy Linton <redacted>
---
 .../net/ethernet/broadcom/genet/bcmgenet.c    | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 120fa05a39ff..c736700f829e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

@@ -7,6 +7,7 @@
 
 #define pr_fmt(fmt)				"bcmgenet: " fmt
 
+#include <linux/acpi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sched.h>

@@ -3476,7 +3477,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	const struct bcmgenet_plat_data *pdata;
 	struct bcmgenet_priv *priv;
 	struct net_device *dev;
-	const void *macaddr;
+	const void *macaddr = NULL;
 	unsigned int i;
 	int err = -EIO;
 	const char *phy_mode_str;

@@ -3510,7 +3511,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	if (dn)
 		macaddr = of_get_mac_address(dn);
-	else
+	else if (pd)
 		macaddr = pd->mac_address;
 
 	priv->base = devm_platform_ioremap_resource(pdev, 0);

@@ -3555,8 +3556,9 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	priv->dev = dev;
 	priv->pdev = pdev;
-	if (of_id) {
-		pdata = of_id->data;
+
+	pdata = device_get_match_data(&pdev->dev);
+	if (pdata) {
 		priv->version = pdata->version;
 		priv->dma_max_burst_length = pdata->dma_max_burst_length;
 	} else {

@@ -3595,7 +3597,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	/* If this is an internal GPHY, power it on now, before UniMAC is
 	 * brought out of reset as absolutely no UniMAC activity is allowed
 	 */
-	if (dn && !of_property_read_string(dn, "phy-mode", &phy_mode_str) &&
+	if (!device_property_read_string(&pdev->dev, "phy-mode", &phy_mode_str) &&
 	    !strcasecmp(phy_mode_str, "internal"))
 		bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
 

@@ -3768,6 +3770,12 @@ static int bcmgenet_suspend(struct device *d)
 
 static SIMPLE_DEV_PM_OPS(bcmgenet_pm_ops, bcmgenet_suspend, bcmgenet_resume);
 
+static const struct acpi_device_id genet_acpi_match[] = {
+	{ "BCM6E4E", (kernel_ulong_t)&bcm2711_plat_data },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, genet_acpi_match);
+
 static struct platform_driver bcmgenet_driver = {
 	.probe	= bcmgenet_probe,
 	.remove	= bcmgenet_remove,

@@ -3776,6 +3784,7 @@ static struct platform_driver bcmgenet_driver = {
 		.name	= "bcmgenet",
 		.of_match_table = bcmgenet_match,
 		.pm	= &bcmgenet_pm_ops,
+		.acpi_match_table = ACPI_PTR(genet_acpi_match),
 	},
 };
 module_platform_driver(bcmgenet_driver);

-- 
2.24.1

On Sat, Feb 01, 2020 at 01:46:22AM -0600, Jeremy Linton wrote:

Hi Jeremy

phy_find_first() is not recommended. Only use it if you have no other
option. If the hardware is more complex, two PHYs on one bus, you are
going to have a problem. So i suggest this is used only for PCI cards
where the hardware is very fixed, and there is only ever one MAC and
PHY on the PCI card. When you do have this split between MAC and MDIO
bus, each being independent devices, it is more likely that you do
have multiple PHYs on one shared MDIO bus.

In the DT world, you use a phy-handle to point to the PHY node in the
device tree. Does ACPI have the same concept, a pointer to some other
device in ACPI?

Thanks
	Andrew

The code you are modifying appears to be old and out of date. For a
long time there has been a helper, of_get_phy_mode(). You should look
at fwnode_get_phy_mode().

   Andrew

Could you also maybe put in here somewhere a call to
device_get_mac_address(), to support getting the MAC address out of
ACPI?

	Andrew

On 1/31/2020 11:46 PM, Jeremy Linton wrote:

If one types "failed to get enet clock" or similar into google
there are ~370k hits. The vast majority are people debugging
problems unrelated to this adapter, or bragging about their
rpi's. Given that its not a fatal situation with common DT based
systems, lets reduce the severity so people aren't seeing failure
messages in everyday operation.

Signed-off-by: Jeremy Linton <redacted>

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

On 1/31/2020 11:46 PM, Jeremy Linton wrote:

The DT phy mode is similar to what we want for ACPI
lets factor it out of the of path, and change the
of_ call to device_. Further if the phy-mode property
cannot be found instead of failing the driver load lets
just default it to RGMII.

Humm no please do not provide a fallback, if we cannot find a valid
'phy-mode' property we error out. This controller can be used with a
variety of configurations (internal EPHY/GPHY, MoCA, external
MII/Reverse MII/RGMII) and from a support perspective it is much easier
for us if the driver errors out if one of those essential properties are
omitted.

Other than that, this looks OK.
-- 
Florian

Hi Jeremy,

[add Nicolas as BCM2835 maintainer]

Am 01.02.20 um 08:46 schrieb Jeremy Linton:

If one types "failed to get enet clock" or similar into google
there are ~370k hits. The vast majority are people debugging
problems unrelated to this adapter, or bragging about their
rpi's. Given that its not a fatal situation with common DT based
systems, lets reduce the severity so people aren't seeing failure
messages in everyday operation.

i'm fine with your patch, since the clocks are optional according to the
binding. But instead of hiding of those warning, it would be better to
fix the root cause (missing clocks). Unfortunately i don't have the
necessary documentation, just some answers from the RPi guys.

This is what i got so far:

diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
index 961bed8..d4ff370 100644
--- a/arch/arm/boot/dts/bcm2711.dtsi
+++ b/arch/arm/boot/dts/bcm2711.dtsi

@@ -338,6 +338,8 @@

                        reg = <0x0 0x7d580000 0x10000>;
                        #address-cells = <0x1>;
                        #size-cells = <0x1>;
+                       clocks = <&clocks BCM2711_CLOCK_GENET250>;
+                       clock-names = "enet";
                        interrupts = <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
                                     <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>;
                        status = "disabled";

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index ded13cc..627f1b1 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c

@@ -116,6 +116,10 @@

 #define CM_EMMCDIV             0x1c4
 #define CM_EMMC2CTL            0x1d0
 #define CM_EMMC2DIV            0x1d4
+#define CM_GENET250CTL         0x1e8
+#define CM_GENET250DIV         0x1ec
+#define CM_GENET125CTL         0x210
+#define CM_GENET125DIV         0x214
 
 /* General bits for the CM_*CTL regs */
 # define CM_ENABLE                     BIT(4)

@@ -2021,6 +2025,25 @@ static const struct bcm2835_clk_desc

clk_desc_array[] = {
                .frac_bits = 8,
                .tcnt_mux = 42),
 
+       /* GENET clocks (only available for BCM2711) */
+       [BCM2711_CLOCK_GENET250]        = REGISTER_PER_CLK(
+               SOC_BCM2711,
+               .name = "genet250",
+               .ctl_reg = CM_GENET250CTL,
+               .div_reg = CM_GENET250DIV,
+               .int_bits = 4,
+               .frac_bits = 8,
+               .tcnt_mux = 45),
+
+       [BCM2711_CLOCK_GENET125]        = REGISTER_PER_CLK(
+               SOC_BCM2711,
+               .name = "genet125",
+               .ctl_reg = CM_GENET125CTL,
+               .div_reg = CM_GENET125DIV,
+               .int_bits = 4,
+               .frac_bits = 8,
+               .tcnt_mux = 50),
+
        /* General purpose (GPIO) clocks */
        [BCM2835_CLOCK_GP0]     = REGISTER_PER_CLK(
                SOC_ALL,

diff --git a/include/dt-bindings/clock/bcm2835.h

b/include/dt-bindings/clock/bcm2835.h
index b60c0343..fca65ab 100644

--- a/include/dt-bindings/clock/bcm2835.h
+++ b/include/dt-bindings/clock/bcm2835.h

@@ -60,3 +60,5 @@

 #define BCM2835_CLOCK_DSI1P            50
 
 #define BCM2711_CLOCK_EMMC2            51
+#define BCM2711_CLOCK_GENET250         52
+#define BCM2711_CLOCK_GENET125         53

Hi,

First thanks for looking at this!

On 2/1/20 9:25 AM, Andrew Lunn wrote:

On Sat, Feb 01, 2020 at 01:46:22AM -0600, Jeremy Linton wrote:

quoted

The unimac mdio driver falls back to scanning the
entire bus if its given an appropriate mask. In ACPI
mode we expect that the system is well behaved and
conforms to recent versions of the specification.

We then utilize phy_find_first(), and
phy_connect_direct() to find and attach to the
discovered phy during net_device open.

Signed-off-by: Jeremy Linton <redacted>
---
  drivers/net/ethernet/broadcom/genet/bcmmii.c | 40 +++++++++++++++++---
  1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 2049f8218589..f3271975b375 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c

@@ -5,7 +5,7 @@
   * Copyright (c) 2014-2017 Broadcom
   */
  
-
+#include <linux/acpi.h>
  #include <linux/types.h>
  #include <linux/delay.h>
  #include <linux/wait.h>

@@ -311,7 +311,9 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
  int bcmgenet_mii_probe(struct net_device *dev)
  {
  	struct bcmgenet_priv *priv = netdev_priv(dev);
-	struct device_node *dn = priv->pdev->dev.of_node;
+	struct device *kdev = &priv->pdev->dev;
+	struct device_node *dn = kdev->of_node;
+
  	struct phy_device *phydev;
  	u32 phy_flags = 0;
  	int ret;

@@ -334,7 +336,27 @@ int bcmgenet_mii_probe(struct net_device *dev)
  			return -ENODEV;
  		}
  	} else {
-		phydev = dev->phydev;
+		if (has_acpi_companion(kdev)) {
+			char mdio_bus_id[MII_BUS_ID_SIZE];
+			struct mii_bus *unimacbus;
+
+			snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
+				 UNIMAC_MDIO_DRV_NAME, priv->pdev->id);
+
+			unimacbus = mdio_find_bus(mdio_bus_id);
+			if (!unimacbus) {
+				pr_err("Unable to find mii\n");
+				return -ENODEV;
+			}
+			phydev = phy_find_first(unimacbus);
+			put_device(&unimacbus->dev);
+			if (!phydev) {
+				pr_err("Unable to find PHY\n");
+				return -ENODEV;

Hi Jeremy

phy_find_first() is not recommended. Only use it if you have no other
option. If the hardware is more complex, two PHYs on one bus, you are
going to have a problem. So i suggest this is used only for PCI cards
where the hardware is very fixed, and there is only ever one MAC and
PHY on the PCI card. When you do have this split between MAC and MDIO
bus, each being independent devices, it is more likely that you do
have multiple PHYs on one shared MDIO bus.

Understood.

In the DT world, you use a phy-handle to point to the PHY node in the
device tree. Does ACPI have the same concept, a pointer to some other
device in ACPI?

There aren't a lot of good options here. ACPI is mostly a power mgmt 
abstraction and is directly silent on this topic. So while it can be 
quite descriptive like DT, frequently choosing to use a bunch of DT 
properties in ACPI _DSD methods is a mistake. Both for cross OS booting 
as well as long term support. Similar silence from SBSA, which attempts 
to setup some guide rails for situations like this. I think that is 
because there aren't any non-obsolete industry standards for NICs.

So, in an attempt to fall back on the idea that the hardware should be 
self describing, and it shouldn't be involving the system firmware in 
basic device specific introspection I've been trying to avoid the use of 
any DSD properties. In the majority of cases (including DT) these 
properties aren't being auto-detected by the firmware either, they are 
just being hard-coded into DT or DSDT tables.

Part of the arm standardization effort has been to clamp down on all the 
creative ways that these machines can be built. It seems a guide rail 
that says for this adapter it must have a MDIO bus per MAC for ACPI 
support as though it were on PCI isn't unreasonable. Another easily 
understood one, might be to assign the PHY's the the same order as the 
MAC's UIDs if there were a shared bus (less ideal without example hardware).

I'm not really sure what the right answer here is, but I like to avoid 
hardcoding DT properties in DSD unless there simply isn't an alternative.

Hi,

On 2/1/20 9:33 AM, Andrew Lunn wrote:

quoted

@@ -3595,7 +3597,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
  	/* If this is an internal GPHY, power it on now, before UniMAC is
  	 * brought out of reset as absolutely no UniMAC activity is allowed
  	 */
-	if (dn && !of_property_read_string(dn, "phy-mode", &phy_mode_str) &&
+	if (!device_property_read_string(&pdev->dev, "phy-mode", &phy_mode_str) &&
  	    !strcasecmp(phy_mode_str, "internal"))
  		bcmgenet_power_up(priv, GENET_POWER_PASSIVE);

The code you are modifying appears to be old and out of date. For a
long time there has been a helper, of_get_phy_mode(). You should look
at fwnode_get_phy_mode().

Yes, thanks, I did that in the other phy path but not here.

Hi,

Thanks for taking a look at this again!

On 2/1/20 10:24 AM, Florian Fainelli wrote:

On 1/31/2020 11:46 PM, Jeremy Linton wrote:

quoted

The DT phy mode is similar to what we want for ACPI
lets factor it out of the of path, and change the
of_ call to device_. Further if the phy-mode property
cannot be found instead of failing the driver load lets
just default it to RGMII.

Humm no please do not provide a fallback, if we cannot find a valid
'phy-mode' property we error out. This controller can be used with a
variety of configurations (internal EPHY/GPHY, MoCA, external
MII/Reverse MII/RGMII) and from a support perspective it is much easier
for us if the driver errors out if one of those essential properties are
omitted.

Other than that, this looks OK.

Sure, I went around in circles about this one because it cluttered the 
code path up a bit.

Hi,

On 2/1/20 9:37 AM, Andrew Lunn wrote:

quoted

@@ -3601,6 +3605,23 @@ static int bcmgenet_probe(struct platform_device *pdev)
  	    !strcasecmp(phy_mode_str, "internal"))
  		bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
  
+	if (dn)
+		macaddr = of_get_mac_address(dn);
+	else if (pd)
+		macaddr = pd->mac_address;
+
+	if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) {
+		if (has_acpi_companion(&pdev->dev))
+			bcmgenet_get_hw_addr(priv, dev->dev_addr);
+
+		if (!is_valid_ether_addr(dev->dev_addr)) {
+			dev_warn(&pdev->dev, "using random Ethernet MAC\n");
+			eth_hw_addr_random(dev);
+		}
+	} else {
+		ether_addr_copy(dev->dev_addr, macaddr);
+	}
+

Could you also maybe put in here somewhere a call to
device_get_mac_address(), to support getting the MAC address out of
ACPI?

I had that here until right before I posted it, mostly because I was 
trying to consolidate the DT/ACPI paths. I pulled it out because it 
wasn't making the code any clearer, and as I mentioned in my response to 
the general _DSD properties I would rather entirely depend on non DSD 
properties if possible.

I will put it back in, but IMHO we shouldn't be finding firmware using 
it. Since the discussion a few years back, its become clearer to me its 
not usually needed. As in this example, the addresses can usually be 
picked off the adapter if the firmware bothers to set them up.

Hi,

First, thanks for looking at this!

On 2/1/20 10:44 AM, Stefan Wahren wrote:

Hi Jeremy,

[add Nicolas as BCM2835 maintainer]

Am 01.02.20 um 08:46 schrieb Jeremy Linton:

quoted

If one types "failed to get enet clock" or similar into google
there are ~370k hits. The vast majority are people debugging
problems unrelated to this adapter, or bragging about their
rpi's. Given that its not a fatal situation with common DT based
systems, lets reduce the severity so people aren't seeing failure
messages in everyday operation.

i'm fine with your patch, since the clocks are optional according to the
binding. But instead of hiding of those warning, it would be better to
fix the root cause (missing clocks). Unfortunately i don't have the
necessary documentation, just some answers from the RPi guys.

The DT case just added to my ammunition here :)

But really, I'm fixing an ACPI problem because the ACPI power management 
methods are also responsible for managing the clocks. Which means if I 
don't lower the severity (or otherwise tweak the code path) these errors 
are going to happen on every ACPI boot.

This is what i got so far:

BTW: For DT, is part of the problem here that the videocore mailbox has 
a clock management method? For ACPI one of the paths of investigation is 
to write AML which just interfaces to that mailbox interface for clock 
control here. (there is also SCMII to be considered).

Hi,

On 2/1/20 9:25 AM, Andrew Lunn wrote:

On Sat, Feb 01, 2020 at 01:46:22AM -0600, Jeremy Linton wrote:

quoted

The unimac mdio driver falls back to scanning the
entire bus if its given an appropriate mask. In ACPI
mode we expect that the system is well behaved and
conforms to recent versions of the specification.

We then utilize phy_find_first(), and
phy_connect_direct() to find and attach to the
discovered phy during net_device open.

Signed-off-by: Jeremy Linton <redacted>
---
  drivers/net/ethernet/broadcom/genet/bcmmii.c | 40 +++++++++++++++++---
  1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 2049f8218589..f3271975b375 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c

@@ -5,7 +5,7 @@
   * Copyright (c) 2014-2017 Broadcom
   */
  
-
+#include <linux/acpi.h>
  #include <linux/types.h>
  #include <linux/delay.h>
  #include <linux/wait.h>

@@ -311,7 +311,9 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
  int bcmgenet_mii_probe(struct net_device *dev)
  {
  	struct bcmgenet_priv *priv = netdev_priv(dev);
-	struct device_node *dn = priv->pdev->dev.of_node;
+	struct device *kdev = &priv->pdev->dev;
+	struct device_node *dn = kdev->of_node;
+
  	struct phy_device *phydev;
  	u32 phy_flags = 0;
  	int ret;

@@ -334,7 +336,27 @@ int bcmgenet_mii_probe(struct net_device *dev)
  			return -ENODEV;
  		}
  	} else {
-		phydev = dev->phydev;
+		if (has_acpi_companion(kdev)) {
+			char mdio_bus_id[MII_BUS_ID_SIZE];
+			struct mii_bus *unimacbus;
+
+			snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
+				 UNIMAC_MDIO_DRV_NAME, priv->pdev->id);
+
+			unimacbus = mdio_find_bus(mdio_bus_id);
+			if (!unimacbus) {
+				pr_err("Unable to find mii\n");
+				return -ENODEV;
+			}
+			phydev = phy_find_first(unimacbus);
+			put_device(&unimacbus->dev);
+			if (!phydev) {
+				pr_err("Unable to find PHY\n");
+				return -ENODEV;

Hi Jeremy

phy_find_first() is not recommended. Only use it if you have no other
option. If the hardware is more complex, two PHYs on one bus, you are
going to have a problem. So i suggest this is used only for PCI cards
where the hardware is very fixed, and there is only ever one MAC and
PHY on the PCI card. When you do have this split between MAC and MDIO
bus, each being independent devices, it is more likely that you do
have multiple PHYs on one shared MDIO bus.

In the DT world, you use a phy-handle to point to the PHY node in the
device tree. Does ACPI have the same concept, a pointer to some other
device in ACPI?

I though I should clarify the direct question here about ACPI. ACPI does 
have the ability to do what you describe, but it a more rigorous way. If 
you look at the ACPI GenericSerialBus abstraction you will see how ACPI 
would likely handle this situation. I've been considering making a 
similar comment in that large fwnode patch set posted the other day.

I though I should clarify the direct question here about ACPI. ACPI does
have the ability to do what you describe, but it a more rigorous way. If you
look at the ACPI GenericSerialBus abstraction you will see how ACPI would
likely handle this situation. I've been considering making a similar comment
in that large fwnode patch set posted the other day.

I know ~0 about ACPI. But it does not seem unreasonable to describe an
MDIO bus in the same way as an i2c bus, or an spi bus. Each can have
devices on it, at specific addresses. Each needs common properties
like interrupts, and each needs bus specific properties like SPI
polarity. And you need pointers to these devices, so that other
subsystems can use them.

So maybe the correct way to describe this is to use ACPI
GenericSerialBus?

What the kernel community really seems to miss is a "Rob Herring" for
ACPI.

     Andrew

On Sat, Feb 01, 2020 at 08:24:14AM -0800, Florian Fainelli wrote:

On 1/31/2020 11:46 PM, Jeremy Linton wrote:

quoted

The DT phy mode is similar to what we want for ACPI
lets factor it out of the of path, and change the
of_ call to device_. Further if the phy-mode property
cannot be found instead of failing the driver load lets
just default it to RGMII.

Humm no please do not provide a fallback, if we cannot find a valid
'phy-mode' property we error out. This controller can be used with a
variety of configurations (internal EPHY/GPHY, MoCA, external
MII/Reverse MII/RGMII) and from a support perspective it is much easier
for us if the driver errors out if one of those essential properties are
omitted.

Hi Florian

Does any of the silicon variants have two or more MACs sharing one
MDIO bus? I'm thinking about the next patch in the series.

     Thanks
	Andrew

On 2/2/2020 5:17 PM, Andrew Lunn wrote:

On Sat, Feb 01, 2020 at 08:24:14AM -0800, Florian Fainelli wrote:

quoted

On 1/31/2020 11:46 PM, Jeremy Linton wrote:

quoted

The DT phy mode is similar to what we want for ACPI
lets factor it out of the of path, and change the
of_ call to device_. Further if the phy-mode property
cannot be found instead of failing the driver load lets
just default it to RGMII.

Humm no please do not provide a fallback, if we cannot find a valid
'phy-mode' property we error out. This controller can be used with a
variety of configurations (internal EPHY/GPHY, MoCA, external
MII/Reverse MII/RGMII) and from a support perspective it is much easier
for us if the driver errors out if one of those essential properties are
omitted.

Hi Florian

Does any of the silicon variants have two or more MACs sharing one
MDIO bus? I'm thinking about the next patch in the series.

Have not come across a customer doing that, but the hardware
definitively permits it, and so does the top-level chip pinmuxing.
-- 
Florian

Hi,
BTW the patch looks good to me too:

Reviewed-by: Nicolas Saenz Julienne <redacted>

On Sat, 2020-02-01 at 13:27 -0600, Jeremy Linton wrote:

Hi,

First, thanks for looking at this!

On 2/1/20 10:44 AM, Stefan Wahren wrote:

quoted

Hi Jeremy,

[add Nicolas as BCM2835 maintainer]

Am 01.02.20 um 08:46 schrieb Jeremy Linton:

quoted

If one types "failed to get enet clock" or similar into google
there are ~370k hits. The vast majority are people debugging
problems unrelated to this adapter, or bragging about their
rpi's. Given that its not a fatal situation with common DT based
systems, lets reduce the severity so people aren't seeing failure
messages in everyday operation.

i'm fine with your patch, since the clocks are optional according to the
binding. But instead of hiding of those warning, it would be better to
fix the root cause (missing clocks). Unfortunately i don't have the
necessary documentation, just some answers from the RPi guys.

The DT case just added to my ammunition here :)

But really, I'm fixing an ACPI problem because the ACPI power management 
methods are also responsible for managing the clocks. Which means if I 
don't lower the severity (or otherwise tweak the code path) these errors 
are going to happen on every ACPI boot.

quoted

This is what i got so far:

Stefan, Apart from the lack of documentation (and maybe also time), is there
any specific reason you didn't sent the genet clock patch yet? It should be OK
functionally isn't it?

BTW: For DT, is part of the problem here that the videocore mailbox has 
a clock management method?

I don't think it'll be the case for these clocks. We try to only use the
mailbox interface if access to the clock is shared with videocore's firmware.
The only example for now is 'pllb' which drives the CPU. See clk-raspberrypi.c
for the firmware part and clk-bcm2835.c for the rest.

Note that the firmware interface has some shortcomings, it isn't fine grained
nor provides a full clock tree to work with, also some clock changes, from
videocore's point of view, might change multiple plls behind your back. See for
example the ARM clock, at offset 0x3[1]: if you don't explicitly disable turbo
mode, it'll change both pllb and pllc. Affecting a whole lot of peripherals.

In an Ideal world I'd love to see them implement ARM's SCMI[2]. It would make
our lives easier.

For ACPI one of the paths of investigation is to write AML which just
interfaces to that mailbox interface for clock control here. (there is also
SCMII to be considered).

As we're on the topic of integrating the mailbox interfaces with ACPI, have you
looked at VCHIQ in the staging directory? It serves as an interface to
videocore for the camera, HDMI audio and video codec drivers. It ultimately
depends on the mailbox interface mentioned above. It might be interesting for
you to look into it before writing the AML interface to the mailbox.

Regards,
Nicolas

[1] https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
[2] https://github.com/raspberrypi/firmware/issues/1139

Hi,

On 2/2/20 9:24 PM, Florian Fainelli wrote:

On 2/2/2020 5:17 PM, Andrew Lunn wrote:

quoted

On Sat, Feb 01, 2020 at 08:24:14AM -0800, Florian Fainelli wrote:

quoted

On 1/31/2020 11:46 PM, Jeremy Linton wrote:

quoted

The DT phy mode is similar to what we want for ACPI
lets factor it out of the of path, and change the
of_ call to device_. Further if the phy-mode property
cannot be found instead of failing the driver load lets
just default it to RGMII.

Humm no please do not provide a fallback, if we cannot find a valid
'phy-mode' property we error out. This controller can be used with a
variety of configurations (internal EPHY/GPHY, MoCA, external
MII/Reverse MII/RGMII) and from a support perspective it is much easier
for us if the driver errors out if one of those essential properties are
omitted.

Hi Florian

Does any of the silicon variants have two or more MACs sharing one
MDIO bus? I'm thinking about the next patch in the series.

Have not come across a customer doing that, but the hardware
definitively permits it, and so does the top-level chip pinmuxing.

Does the genet driver?

I might be missing something in the driver, but it looks like the whole 
thing is 1:1:1:1 with platform dev:mdio:phy:netdev at the moment. Given 
the way bcmgenet_mii_register is throwing a bcmgenet MII bus for every 
device _probe().

On 2/3/20 10:46 AM, Jeremy Linton wrote:

Hi,

On 2/2/20 9:24 PM, Florian Fainelli wrote:

quoted

On 2/2/2020 5:17 PM, Andrew Lunn wrote:

quoted

On Sat, Feb 01, 2020 at 08:24:14AM -0800, Florian Fainelli wrote:

quoted

On 1/31/2020 11:46 PM, Jeremy Linton wrote:

quoted

The DT phy mode is similar to what we want for ACPI
lets factor it out of the of path, and change the
of_ call to device_. Further if the phy-mode property
cannot be found instead of failing the driver load lets
just default it to RGMII.

Humm no please do not provide a fallback, if we cannot find a valid
'phy-mode' property we error out. This controller can be used with a
variety of configurations (internal EPHY/GPHY, MoCA, external
MII/Reverse MII/RGMII) and from a support perspective it is much easier
for us if the driver errors out if one of those essential properties
are
omitted.

Hi Florian

Does any of the silicon variants have two or more MACs sharing one
MDIO bus? I'm thinking about the next patch in the series.

Have not come across a customer doing that, but the hardware
definitively permits it, and so does the top-level chip pinmuxing.

Does the genet driver?

I might be missing something in the driver, but it looks like the whole
thing is 1:1:1:1 with platform dev:mdio:phy:netdev at the moment. Given
the way bcmgenet_mii_register is throwing a bcmgenet MII bus for every
device _probe().

Andrew's question was hypothetical, and I answered it the best way I
could. I did not say this was a *currently* supported combination from a
software perspective or that it would work today, but the hardware would
allow it. This question probably belongs to patch #3 which is about
using (or not phy_find_first).
-- 
Florian

Hi,

Am 03.02.20 um 19:36 schrieb Nicolas Saenz Julienne:

Hi,
BTW the patch looks good to me too:

Reviewed-by: Nicolas Saenz Julienne <redacted>

On Sat, 2020-02-01 at 13:27 -0600, Jeremy Linton wrote:

quoted

Hi,

First, thanks for looking at this!

On 2/1/20 10:44 AM, Stefan Wahren wrote:

quoted

Hi Jeremy,

[add Nicolas as BCM2835 maintainer]

Am 01.02.20 um 08:46 schrieb Jeremy Linton:

quoted

If one types "failed to get enet clock" or similar into google
there are ~370k hits. The vast majority are people debugging
problems unrelated to this adapter, or bragging about their
rpi's. Given that its not a fatal situation with common DT based
systems, lets reduce the severity so people aren't seeing failure
messages in everyday operation.

i'm fine with your patch, since the clocks are optional according to the
binding. But instead of hiding of those warning, it would be better to
fix the root cause (missing clocks). Unfortunately i don't have the
necessary documentation, just some answers from the RPi guys.

The DT case just added to my ammunition here :)

But really, I'm fixing an ACPI problem because the ACPI power management
methods are also responsible for managing the clocks. Which means if I
don't lower the severity (or otherwise tweak the code path) these errors
are going to happen on every ACPI boot.

quoted

This is what i got so far:

Stefan, Apart from the lack of documentation (and maybe also time), is there
any specific reason you didn't sent the genet clock patch yet? It should be OK
functionally isn't it?

last time i tried to specify the both clocks as suggest by the binding
document (took genet125 for wol, not sure this is correct), but this
caused an abort on the BCM2711. In the lack of documentation i stopped
further investigations. As i saw that Jeremy send this patch, i wanted
to share my current results and retestet it with this version which
doesn't crash. I don't know the reason why both clocks should be
specified, but this patch should be acceptable since the RPi 4 doesn't
support wake on LAN.

Best regards
Stefan

On 2/1/20 11:07 AM, Jeremy Linton wrote:

Hi,

First thanks for looking at this!

On 2/1/20 9:25 AM, Andrew Lunn wrote:

quoted

On Sat, Feb 01, 2020 at 01:46:22AM -0600, Jeremy Linton wrote:

quoted

The unimac mdio driver falls back to scanning the
entire bus if its given an appropriate mask. In ACPI
mode we expect that the system is well behaved and
conforms to recent versions of the specification.

We then utilize phy_find_first(), and
phy_connect_direct() to find and attach to the
discovered phy during net_device open.

Signed-off-by: Jeremy Linton <redacted>
---
  drivers/net/ethernet/broadcom/genet/bcmmii.c | 40 +++++++++++++++++---
  1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c

b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 2049f8218589..f3271975b375 100644

--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c

@@ -5,7 +5,7 @@

   * Copyright (c) 2014-2017 Broadcom
   */
  -
+#include <linux/acpi.h>
  #include <linux/types.h>
  #include <linux/delay.h>
  #include <linux/wait.h>

@@ -311,7 +311,9 @@ int bcmgenet_mii_config(struct net_device *dev,

bool init)
  int bcmgenet_mii_probe(struct net_device *dev)
  {
      struct bcmgenet_priv *priv = netdev_priv(dev);
-    struct device_node *dn = priv->pdev->dev.of_node;
+    struct device *kdev = &priv->pdev->dev;
+    struct device_node *dn = kdev->of_node;
+
      struct phy_device *phydev;
      u32 phy_flags = 0;
      int ret;

@@ -334,7 +336,27 @@ int bcmgenet_mii_probe(struct net_device *dev)

              return -ENODEV;
          }
      } else {
-        phydev = dev->phydev;
+        if (has_acpi_companion(kdev)) {
+            char mdio_bus_id[MII_BUS_ID_SIZE];
+            struct mii_bus *unimacbus;
+
+            snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
+                 UNIMAC_MDIO_DRV_NAME, priv->pdev->id);
+
+            unimacbus = mdio_find_bus(mdio_bus_id);
+            if (!unimacbus) {
+                pr_err("Unable to find mii\n");
+                return -ENODEV;
+            }
+            phydev = phy_find_first(unimacbus);
+            put_device(&unimacbus->dev);
+            if (!phydev) {
+                pr_err("Unable to find PHY\n");
+                return -ENODEV;

Hi Jeremy

phy_find_first() is not recommended. Only use it if you have no other
option. If the hardware is more complex, two PHYs on one bus, you are
going to have a problem. So i suggest this is used only for PCI cards
where the hardware is very fixed, and there is only ever one MAC and
PHY on the PCI card. When you do have this split between MAC and MDIO
bus, each being independent devices, it is more likely that you do
have multiple PHYs on one shared MDIO bus.

Understood.

quoted

In the DT world, you use a phy-handle to point to the PHY node in the
device tree. Does ACPI have the same concept, a pointer to some other
device in ACPI?

There aren't a lot of good options here. ACPI is mostly a power mgmt
abstraction and is directly silent on this topic. So while it can be
quite descriptive like DT, frequently choosing to use a bunch of DT
properties in ACPI _DSD methods is a mistake. Both for cross OS booting
as well as long term support. Similar silence from SBSA, which attempts
to setup some guide rails for situations like this. I think that is
because there aren't any non-obsolete industry standards for NICs.

I suppose you have two options from here:

- try to set a good example and provide a representation of the devices
that is as comprehensive as possible and paves the way for others to
draw as a good (or not bad at least) example

- continue to do an ad-hoc solution since there is little to no interest
in any standardization (more likely, no need).

So, in an attempt to fall back on the idea that the hardware should be
self describing, and it shouldn't be involving the system firmware in
basic device specific introspection I've been trying to avoid the use of
any DSD properties. In the majority of cases (including DT) these
properties aren't being auto-detected by the firmware either, they are
just being hard-coded into DT or DSDT tables.

Part of the arm standardization effort has been to clamp down on all the
creative ways that these machines can be built. It seems a guide rail
that says for this adapter it must have a MDIO bus per MAC for ACPI
support as though it were on PCI isn't unreasonable. Another easily
understood one, might be to assign the PHY's the the same order as the
MAC's UIDs if there were a shared bus (less ideal without example
hardware).

I'm not really sure what the right answer here is, but I like to avoid
hardcoding DT properties in DSD unless there simply isn't an alternative.

I do not think we are asking to get properties hard coded, we are asking
to get a proper representation of these MDIO devices, including their
supserset that PHY devices are into ACPI, in a way that is usable by the
core MDIO layer without drivers cutting corners.
-- 
Florian

Hi,

On 2/2/20 7:15 PM, Andrew Lunn wrote:

quoted

I though I should clarify the direct question here about ACPI. ACPI does
have the ability to do what you describe, but it a more rigorous way. If you
look at the ACPI GenericSerialBus abstraction you will see how ACPI would
likely handle this situation. I've been considering making a similar comment
in that large fwnode patch set posted the other day.

I should have been a lot more specific here, but I didn't want to write 
a book.

I know ~0 about ACPI. But it does not seem unreasonable to describe an
MDIO bus in the same way as an i2c bus, or an spi bus. Each can have
devices on it, at specific addresses. Each needs common properties
like interrupts, and each needs bus specific properties like SPI
polarity. And you need pointers to these devices, so that other
subsystems can use them.

So maybe the correct way to describe this is to use ACPI
GenericSerialBus?

AFAIK, not as the specification stands today.

First its not defined for MDIO (see 6-240 in acpi 6.3) , and secondly 
because its intended to be used from AML (one of the examples IIRC is to 
read battery vendor info). That implies to me, that the ACPI standards 
body's would also have to add some additional methods which configure 
and return state about the phys. AKA some of the linux phy_() functions 
would just redirect to AML equivalents the same way there are AML 
battery functions for returning status/etc.

On 2/3/20 11:08 AM, Stefan Wahren wrote:

Hi,

Am 03.02.20 um 19:36 schrieb Nicolas Saenz Julienne:

quoted

Hi,
BTW the patch looks good to me too:

Reviewed-by: Nicolas Saenz Julienne <redacted>

On Sat, 2020-02-01 at 13:27 -0600, Jeremy Linton wrote:

quoted

Hi,

First, thanks for looking at this!

On 2/1/20 10:44 AM, Stefan Wahren wrote:

quoted

Hi Jeremy,

[add Nicolas as BCM2835 maintainer]

Am 01.02.20 um 08:46 schrieb Jeremy Linton:

quoted

If one types "failed to get enet clock" or similar into google
there are ~370k hits. The vast majority are people debugging
problems unrelated to this adapter, or bragging about their
rpi's. Given that its not a fatal situation with common DT based
systems, lets reduce the severity so people aren't seeing failure
messages in everyday operation.

i'm fine with your patch, since the clocks are optional according to the
binding. But instead of hiding of those warning, it would be better to
fix the root cause (missing clocks). Unfortunately i don't have the
necessary documentation, just some answers from the RPi guys.

The DT case just added to my ammunition here :)

But really, I'm fixing an ACPI problem because the ACPI power management
methods are also responsible for managing the clocks. Which means if I
don't lower the severity (or otherwise tweak the code path) these errors
are going to happen on every ACPI boot.

quoted

This is what i got so far:

Stefan, Apart from the lack of documentation (and maybe also time), is there
any specific reason you didn't sent the genet clock patch yet? It should be OK
functionally isn't it?

last time i tried to specify the both clocks as suggest by the binding
document (took genet125 for wol, not sure this is correct), but this
caused an abort on the BCM2711. In the lack of documentation i stopped
further investigations. As i saw that Jeremy send this patch, i wanted
to share my current results and retestet it with this version which
doesn't crash. I don't know the reason why both clocks should be
specified, but this patch should be acceptable since the RPi 4 doesn't
support wake on LAN.

Your clock changes look correct, but there is also a CLKGEN register
block which has separate clocks for the GENET controller, which lives at
register offset 0x7d5e0048 and which has the following layout:

bit 0: GENET_CLK_250_CLOCK_ENABLE
bit 1: GENET_EEE_CLOCK_ENABLE
bit 2: GENET_GISB_CLOCK_ENABLE
bit 3: GENET_GMII_CLOCK_ENABLE
bit 4: GENET_HFB_CLOCK_ENABLE
bit 5: GENET_L2_INTR_CLOCK_ENABLE
bit 6: GENET_SCB_CLOCK_ENABLE
bit 7: GENET_UNIMAC_SYS_RX_CLOCK_ENABLE
bit 8: GENET_UNIMAC_SYS_TX_CLOCK_ENABLE

you will need all of those clocks turned on for normal operation minus
EEE, unless EEE is desirable which is why it is a separate clock. Those
clocks default to ON unless turned off, and the main gate that you
control is probably enough.

The Pi4 could support Wake-on-LAN with appropriate VPU firmware changes,
but I do not believe there is any interest in doing that. I would not
"bend" the clock representation just so as to please the driver with its
clocking needs.
-- 
Florian

I do not think we are asking to get properties hard coded, we are asking
to get a proper representation of these MDIO devices, including their
supserset that PHY devices are into ACPI, in a way that is usable by the
core MDIO layer without drivers cutting corners.

Yes.

And i'm also interested in this in a generic way. Ethernet switches
often have MDIO busses, with lots of PHYs on them. Some might argue
that switches are out of scope for ACPI, but people have shown
interest in getting ACPI working on Espressobin,
http://espressobin.net/ which has a marvell Switch on it.

There are also some broadcom SoCs with generic PHYs, not ethernet PHYs
as devices on MDIO busses.

And we have people submitting patches to other drivers at the moment,
which just seem to stuff DT properties into ACPI tables. At least in
networking, ACPI seems to be a wild west, anything goes, no
documentation, no standardisation, nobody has an ACPI maintainer role
over the whole kernel who cares about it.

   Andrew

Hi Florian,

Am 03.02.20 um 22:21 schrieb Florian Fainelli:

On 2/3/20 11:08 AM, Stefan Wahren wrote:

quoted

Hi,

Am 03.02.20 um 19:36 schrieb Nicolas Saenz Julienne:

quoted

Hi,
BTW the patch looks good to me too:

Reviewed-by: Nicolas Saenz Julienne <redacted>

On Sat, 2020-02-01 at 13:27 -0600, Jeremy Linton wrote:

quoted

Hi,

First, thanks for looking at this!

On 2/1/20 10:44 AM, Stefan Wahren wrote:

quoted

Hi Jeremy,

[add Nicolas as BCM2835 maintainer]

Am 01.02.20 um 08:46 schrieb Jeremy Linton:

quoted

If one types "failed to get enet clock" or similar into google
there are ~370k hits. The vast majority are people debugging
problems unrelated to this adapter, or bragging about their
rpi's. Given that its not a fatal situation with common DT based
systems, lets reduce the severity so people aren't seeing failure
messages in everyday operation.

i'm fine with your patch, since the clocks are optional according to the
binding. But instead of hiding of those warning, it would be better to
fix the root cause (missing clocks). Unfortunately i don't have the
necessary documentation, just some answers from the RPi guys.

The DT case just added to my ammunition here :)

But really, I'm fixing an ACPI problem because the ACPI power management
methods are also responsible for managing the clocks. Which means if I
don't lower the severity (or otherwise tweak the code path) these errors
are going to happen on every ACPI boot.

quoted

This is what i got so far:

Stefan, Apart from the lack of documentation (and maybe also time), is there
any specific reason you didn't sent the genet clock patch yet? It should be OK
functionally isn't it?

last time i tried to specify the both clocks as suggest by the binding
document (took genet125 for wol, not sure this is correct), but this
caused an abort on the BCM2711. In the lack of documentation i stopped
further investigations. As i saw that Jeremy send this patch, i wanted
to share my current results and retestet it with this version which
doesn't crash. I don't know the reason why both clocks should be
specified, but this patch should be acceptable since the RPi 4 doesn't
support wake on LAN.

Your clock changes look correct, but there is also a CLKGEN register
block which has separate clocks for the GENET controller, which lives at
register offset 0x7d5e0048 and which has the following layout:

bit 0: GENET_CLK_250_CLOCK_ENABLE
bit 1: GENET_EEE_CLOCK_ENABLE
bit 2: GENET_GISB_CLOCK_ENABLE
bit 3: GENET_GMII_CLOCK_ENABLE
bit 4: GENET_HFB_CLOCK_ENABLE
bit 5: GENET_L2_INTR_CLOCK_ENABLE
bit 6: GENET_SCB_CLOCK_ENABLE
bit 7: GENET_UNIMAC_SYS_RX_CLOCK_ENABLE
bit 8: GENET_UNIMAC_SYS_TX_CLOCK_ENABLE

you will need all of those clocks turned on for normal operation minus
EEE, unless EEE is desirable which is why it is a separate clock. Those
clocks default to ON unless turned off, and the main gate that you
control is probably enough.

so you suggest to add these clock gate(s) to the clk-bcm2835 or
introduce a "clk-genet" from DT perspective?

The Pi4 could support Wake-on-LAN with appropriate VPU firmware changes,
but I do not believe there is any interest in doing that. I would not
"bend" the clock representation just so as to please the driver with its
clocking needs.

Hi Jeremy,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.5]
[also build test WARNING on next-20200205]
[cannot apply to net/master net-next/master linus/master ipvs/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jeremy-Linton/Add-ACPI-bindings-to-the-genet/20200203-101928
base:    d5226fa6dbae0569ee43ecfc08bdcd6770fc4755

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <redacted>

smatch warnings:
drivers/net/ethernet/broadcom/genet/bcmmii.c:485 bcmgenet_phy_interface_init() warn: unsigned 'priv->phy_interface' is never less than zero.

vim +485 drivers/net/ethernet/broadcom/genet/bcmmii.c

   479	
   480	static int bcmgenet_phy_interface_init(struct bcmgenet_priv *priv)
   481	{
   482		struct device *kdev = &priv->pdev->dev;
   483	
   484		priv->phy_interface = device_get_phy_mode(kdev);
 > 485		if (priv->phy_interface < 0) {
   486			dev_dbg(kdev, "invalid PHY mode property\n");
   487			priv->phy_interface = PHY_INTERFACE_MODE_RGMII;
   488		}
   489	
   490		/* We need to specifically look up whether this PHY interface is internal
   491		 * or not *before* we even try to probe the PHY driver over MDIO as we
   492		 * may have shut down the internal PHY for power saving purposes.
   493		 */
   494		if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL)
   495			priv->internal_phy = true;
   496	
   497		return 0;
   498	}
   499	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation