[PATCH net-next 0/3] Rework GENET MDIO controller clocking

[PATCH net-next 0/3] Rework GENET MDIO controller clocking

[Florian Fainelli]

This patch series reworks the way that we manage the GENET MDIO
controller clocks around I/O accesses. During testing with a fully
modular build where bcmgenet, mdio-bcm-unimac, and the Broadcom PHY
driver (broadcom) are all loaded as modules, with no particular care
being taken to order them to mimize deferred probing the following bus
error was obtained:

[    4.344831] printk: console [ttyS0] enabled
[    4.351102] 840d000.serial: ttyS1 at MMIO 0x840d000 (irq = 29, base_baud = 5062500) is a Broadcom BCM7271 UART
[    4.363110] 840e000.serial: ttyS2 at MMIO 0x840e000 (irq = 30, base_baud = 5062500) is a Broadcom BCM7271 UART
[    4.387392] iproc-rng200 8402000.rng: hwrng registered
[    4.398012] Consider using thermal netlink events interface
[    4.403717] brcmstb_thermal a581500.thermal: registered AVS TMON of-sensor driver
[    4.440085] bcmgenet 8f00000.ethernet: GENET 5.0 EPHY: 0x0000
[    4.482526] unimac-mdio unimac-mdio.0: Broadcom UniMAC MDIO bus
[    4.514019] bridge: filtering via arp/ip/ip6tables is no longer available by default. Update your scripts to load br_netfilter if you need this.
[    4.551304] SError Interrupt on CPU2, code 0x00000000bf000002 -- SError
[    4.551324] CPU: 2 PID: 8 Comm: kworker/u8:0 Not tainted 6.1.53-0.1pre-g5a26d98e908c #2
[    4.551330] Hardware name: BCM972180HB_V20 (DT)
[    4.551336] Workqueue: events_unbound deferred_probe_work_func
[    4.551363] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    4.551368] pc : el1_abort+0x2c/0x58
[    4.551376] lr : el1_abort+0x20/0x58
[    4.551379] sp : ffffffc00a383960
[    4.551380] x29: ffffffc00a383960 x28: ffffff80029fd780 x27: 0000000000000000
[    4.551385] x26: 0000000000000000 x25: ffffff8002839005 x24: ffffffc00a1f9bd0
[    4.551390] x23: 0000000040000005 x22: ffffffc000a48084 x21: ffffffc00a3dde14
[    4.551394] x20: 0000000096000210 x19: ffffffc00a3839a0 x18: 0000000000000579
[    4.551399] x17: 0000000000000000 x16: 0000000100000000 x15: ffffffc00a3838c0
[    4.551403] x14: 000000000000000a x13: 6e69622f7273752f x12: 3a6e6962732f7273
[    4.551408] x11: 752f3a6e69622f3a x10: 6e6962732f3d4854 x9 : ffffffc0086466a8
[    4.551412] x8 : ffffff80049ee100 x7 : ffffff8003231938 x6 : 0000000000000000
[    4.551416] x5 : 0000002200000000 x4 : ffffffc00a3839a0 x3 : 0000002000000000
[    4.551420] x2 : 0000000000000025 x1 : 0000000096000210 x0 : 0000000000000000
[    4.551429] Kernel panic - not syncing: Asynchronous SError Interrupt
[    4.551432] CPU: 2 PID: 8 Comm: kworker/u8:0 Not tainted 6.1.53-0.1pre-g5a26d98e908c #2
[    4.551435] Hardware name: BCM972180HB_V20 (DT)
[    4.551437] Workqueue: events_unbound deferred_probe_work_func
[    4.551443] Call trace:
[    4.551445]  dump_backtrace+0xe4/0x124
[    4.551452]  show_stack+0x1c/0x28
[    4.551455]  dump_stack_lvl+0x60/0x78
[    4.551462]  dump_stack+0x14/0x2c
[    4.551467]  panic+0x134/0x304
[    4.551472]  nmi_panic+0x50/0x70
[    4.551480]  arm64_serror_panic+0x70/0x7c
[    4.551484]  do_serror+0x2c/0x5c
[    4.551487]  el1h_64_error_handler+0x2c/0x40
[    4.551491]  el1h_64_error+0x64/0x68
[    4.551496]  el1_abort+0x2c/0x58
[    4.551499]  el1h_64_sync_handler+0x8c/0xb4
[    4.551502]  el1h_64_sync+0x64/0x68
[    4.551505]  unimac_mdio_readl.isra.0+0x4/0xc [mdio_bcm_unimac]
[    4.551519]  __mdiobus_read+0x2c/0x88
[    4.551526]  mdiobus_read+0x40/0x60
[    4.551530]  phy_read+0x18/0x20
[    4.551534]  bcm_phy_config_intr+0x20/0x84
[    4.551537]  phy_disable_interrupts+0x2c/0x3c
[    4.551543]  phy_probe+0x80/0x1b0
[    4.551545]  really_probe+0x1b8/0x390
[    4.551550]  __driver_probe_device+0x134/0x14c
[    4.551554]  driver_probe_device+0x40/0xf8
[    4.551559]  __device_attach_driver+0x108/0x11c
[    4.551563]  bus_for_each_drv+0xa4/0xcc
[    4.551567]  __device_attach+0xdc/0x190
[    4.551571]  device_initial_probe+0x18/0x20
[    4.551575]  bus_probe_device+0x34/0x94
[    4.551579]  deferred_probe_work_func+0xd4/0xe8
[    4.551583]  process_one_work+0x1ac/0x25c
[    4.551590]  worker_thread+0x1f4/0x260
[    4.551595]  kthread+0xc0/0xd0
[    4.551600]  ret_from_fork+0x10/0x20
[    4.551608] SMP: stopping secondary CPUs
[    4.551617] Kernel Offset: disabled
[    4.551619] CPU features: 0x00000,00c00080,0000420b
[    4.551622] Memory Limit: none
[    4.833838] ---[ end Kernel panic - not syncing: Asynchronous SError Interrupt ]---

The issue here is that we managed to probe the GENET controller, the
mdio-bcm-unimac MDIO controller, but the PHY was still being held in a
probe deferral state because it depended upon a GPIO controller provider
not loaded yet. As soon as that provider is loaded however, the PHY
continues to probe, tries to disable the interrupts, and this causes a
MDIO transaction. That MDIO transaction requires I/O register accesses
within the GENET's larger block, and since its clocks are turned off,
the CPU gets a bus error signaled as a System Error.

The patch series takes the simplest approach of keeping the clocks
enabled just for the duration of the I/O accesses. This is also
beneficial to other drivers like bcmasp2 which make use of the same MDIO
controller driver.

Florian Fainelli (3):
  net: mdio: mdio-bcm-unimac: Manage clock around I/O accesses
  net: bcmgenet: Pass "main" clock down to the MDIO driver
  Revert "net: bcmgenet: Ensure MDIO unregistration has clocks enabled"

 drivers/net/ethernet/broadcom/genet/bcmmii.c  |  6 +-
 drivers/net/mdio/mdio-bcm-unimac.c            | 91 ++++++++++---------
 include/linux/platform_data/mdio-bcm-unimac.h |  3 +
 3 files changed, 55 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCH net-next 1/3] net: mdio: mdio-bcm-unimac: Manage clock around I/O accesses

[Florian Fainelli]

Up until now we have managed not to have the mdio-bcm-unimac manage its
clock except during probe and suspend/resume. This works most of the
time, except where it does not.

With a fully modular build, we can get into a situation whereby the
GENET driver is fully registered, and so is the mdio-bcm-unimac driver,
however the Ethernet PHY driver is not yet, because it depends on a
resource that is not yet available (e.g.: GPIO provider). In that state,
the network device is not usable yet, and so to conserve power, the
GENET driver will have turned off its "main" clock which feeds its MDIO
controller.

When the PHY driver finally probes however, we make an access to the PHY
registers to e.g.: disable interrupts, and this causes a bus error
within the MDIO controller space because the MDIO controller clock(s)
are turned off.

To remedy that, we manage the clock around all of the I/O accesses to
the hardware which are done exclusively during read, write and clock
divider configuration.

This ensures that the register space is accessible, and this also
ensures that there are not unnecessarily elevated reference counts
keeping the clocks active when the network device is administratively
turned off. It would be the case with the previous way of managing the
clock.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/mdio/mdio-bcm-unimac.c            | 91 ++++++++++---------
 include/linux/platform_data/mdio-bcm-unimac.h |  3 +
 2 files changed, 51 insertions(+), 43 deletions(-)

diff --git a/drivers/net/mdio/mdio-bcm-unimac.c b/drivers/net/mdio/mdio-bcm-unimac.c
index 68f8ee0ec8ba..0619e5d596d1 100644
--- a/drivers/net/mdio/mdio-bcm-unimac.c
+++ b/drivers/net/mdio/mdio-bcm-unimac.c
@@ -94,6 +94,10 @@ static int unimac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
 	int ret;
 	u32 cmd;
 
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
 	/* Prepare the read operation */
 	cmd = MDIO_RD | (phy_id << MDIO_PMD_SHIFT) | (reg << MDIO_REG_SHIFT);
 	unimac_mdio_writel(priv, cmd, MDIO_CMD);
@@ -103,7 +107,7 @@ static int unimac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
 
 	ret = priv->wait_func(priv->wait_func_data);
 	if (ret)
-		return ret;
+		goto out;
 
 	cmd = unimac_mdio_readl(priv, MDIO_CMD);
 
@@ -112,10 +116,15 @@ static int unimac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
 	 * that condition here and ignore the MDIO controller read failure
 	 * indication.
 	 */
-	if (!(bus->phy_ignore_ta_mask & 1 << phy_id) && (cmd & MDIO_READ_FAIL))
-		return -EIO;
+	if (!(bus->phy_ignore_ta_mask & 1 << phy_id) && (cmd & MDIO_READ_FAIL)) {
+		ret = -EIO;
+		goto out;
+	}
 
-	return cmd & 0xffff;
+	ret = cmd & 0xffff;
+out:
+	clk_disable_unprepare(priv->clk);
+	return ret;
 }
 
 static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
@@ -123,6 +132,11 @@ static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
 {
 	struct unimac_mdio_priv *priv = bus->priv;
 	u32 cmd;
+	int ret;
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
 
 	/* Prepare the write operation */
 	cmd = MDIO_WR | (phy_id << MDIO_PMD_SHIFT) |
@@ -131,7 +145,10 @@ static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
 
 	unimac_mdio_start(priv);
 
-	return priv->wait_func(priv->wait_func_data);
+	ret = priv->wait_func(priv->wait_func_data);
+	clk_disable_unprepare(priv->clk);
+
+	return ret;
 }
 
 /* Workaround for integrated BCM7xxx Gigabit PHYs which have a problem with
@@ -178,14 +195,19 @@ static int unimac_mdio_reset(struct mii_bus *bus)
 	return 0;
 }
 
-static void unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
+static int unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
 {
 	unsigned long rate;
 	u32 reg, div;
+	int ret;
 
 	/* Keep the hardware default values */
 	if (!priv->clk_freq)
-		return;
+		return 0;
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
 
 	if (!priv->clk)
 		rate = 250000000;
@@ -195,7 +217,8 @@ static void unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
 	div = (rate / (2 * priv->clk_freq)) - 1;
 	if (div & ~MDIO_CLK_DIV_MASK) {
 		pr_warn("Incorrect MDIO clock frequency, ignoring\n");
-		return;
+		ret = 0;
+		goto out;
 	}
 
 	/* The MDIO clock is the reference clock (typically 250Mhz) divided by
@@ -205,6 +228,9 @@ static void unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
 	reg &= ~(MDIO_CLK_DIV_MASK << MDIO_CLK_DIV_SHIFT);
 	reg |= div << MDIO_CLK_DIV_SHIFT;
 	unimac_mdio_writel(priv, reg, MDIO_CFG);
+out:
+	clk_disable_unprepare(priv->clk);
+	return ret;
 }
 
 static int unimac_mdio_probe(struct platform_device *pdev)
@@ -235,24 +261,12 @@ static int unimac_mdio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	priv->clk = devm_clk_get_optional(&pdev->dev, NULL);
-	if (IS_ERR(priv->clk))
-		return PTR_ERR(priv->clk);
-
-	ret = clk_prepare_enable(priv->clk);
-	if (ret)
-		return ret;
-
 	if (of_property_read_u32(np, "clock-frequency", &priv->clk_freq))
 		priv->clk_freq = 0;
 
-	unimac_mdio_clk_set(priv);
-
 	priv->mii_bus = mdiobus_alloc();
-	if (!priv->mii_bus) {
-		ret = -ENOMEM;
-		goto out_clk_disable;
-	}
+	if (!priv->mii_bus)
+		return -ENOMEM;
 
 	bus = priv->mii_bus;
 	bus->priv = priv;
@@ -261,17 +275,27 @@ static int unimac_mdio_probe(struct platform_device *pdev)
 		priv->wait_func = pdata->wait_func;
 		priv->wait_func_data = pdata->wait_func_data;
 		bus->phy_mask = ~pdata->phy_mask;
+		priv->clk = pdata->clk;
 	} else {
 		bus->name = "unimac MII bus";
 		priv->wait_func_data = priv;
 		priv->wait_func = unimac_mdio_poll;
+		priv->clk = devm_clk_get_optional(&pdev->dev, NULL);
 	}
+
+	if (IS_ERR(priv->clk))
+		goto out_mdio_free;
+
 	bus->parent = &pdev->dev;
 	bus->read = unimac_mdio_read;
 	bus->write = unimac_mdio_write;
 	bus->reset = unimac_mdio_reset;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d", pdev->name, pdev->id);
 
+	ret = unimac_mdio_clk_set(priv);
+	if (ret)
+		goto out_mdio_free;
+
 	ret = of_mdiobus_register(bus, np);
 	if (ret) {
 		dev_err(&pdev->dev, "MDIO bus registration failed\n");
@@ -286,8 +310,6 @@ static int unimac_mdio_probe(struct platform_device *pdev)
 
 out_mdio_free:
 	mdiobus_free(bus);
-out_clk_disable:
-	clk_disable_unprepare(priv->clk);
 	return ret;
 }
 
@@ -297,34 +319,17 @@ static void unimac_mdio_remove(struct platform_device *pdev)
 
 	mdiobus_unregister(priv->mii_bus);
 	mdiobus_free(priv->mii_bus);
-	clk_disable_unprepare(priv->clk);
-}
-
-static int __maybe_unused unimac_mdio_suspend(struct device *d)
-{
-	struct unimac_mdio_priv *priv = dev_get_drvdata(d);
-
-	clk_disable_unprepare(priv->clk);
-
-	return 0;
 }
 
 static int __maybe_unused unimac_mdio_resume(struct device *d)
 {
 	struct unimac_mdio_priv *priv = dev_get_drvdata(d);
-	int ret;
 
-	ret = clk_prepare_enable(priv->clk);
-	if (ret)
-		return ret;
-
-	unimac_mdio_clk_set(priv);
-
-	return 0;
+	return unimac_mdio_clk_set(priv);
 }
 
 static SIMPLE_DEV_PM_OPS(unimac_mdio_pm_ops,
-			 unimac_mdio_suspend, unimac_mdio_resume);
+			 NULL, unimac_mdio_resume);
 
 static const struct of_device_id unimac_mdio_ids[] = {
 	{ .compatible = "brcm,asp-v2.1-mdio", },
diff --git a/include/linux/platform_data/mdio-bcm-unimac.h b/include/linux/platform_data/mdio-bcm-unimac.h
index 8a5f9f0b2c52..724e1f57b81f 100644
--- a/include/linux/platform_data/mdio-bcm-unimac.h
+++ b/include/linux/platform_data/mdio-bcm-unimac.h
@@ -1,11 +1,14 @@
 #ifndef __MDIO_BCM_UNIMAC_PDATA_H
 #define __MDIO_BCM_UNIMAC_PDATA_H
 
+struct clk;
+
 struct unimac_mdio_pdata {
 	u32 phy_mask;
 	int (*wait_func)(void *data);
 	void *wait_func_data;
 	const char *bus_name;
+	struct clk *clk;
 };
 
 #define UNIMAC_MDIO_DRV_NAME	"unimac-mdio"
-- 
2.34.1

[PATCH net-next 2/3] net: bcmgenet: Pass "main" clock down to the MDIO driver

[Florian Fainelli]

GENET has historically had to create a MDIO platform device for its
controller and pass some auxiliary data to it, like a MDIO completion
callback. Now we also pass the "main" clock to allow for the MDIO bus
controller to manage that clock adequately around I/O accesses.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index cbbe004621bc..7a21950da77c 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -476,6 +476,10 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
 	ppd.wait_func = bcmgenet_mii_wait;
 	ppd.wait_func_data = priv;
 	ppd.bus_name = "bcmgenet MII bus";
+	/* Pass a reference to our "main" clock which is used for MDIO
+	 * transfers
+	 */
+	ppd.clk = priv->clk;
 
 	/* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD
 	 * and is 2 * 32-bits word long, 8 bytes total.
-- 
2.34.1

[PATCH net-next 3/3] Revert "net: bcmgenet: Ensure MDIO unregistration has clocks enabled"

[Florian Fainelli]

This reverts commit eac0aac07f6af47b8ba57c8064bf04ed6df73d1d ("net:
bcmgenet: Ensure MDIO unregistration has clocks enabled"). This is no
longer necessary now that the MDIO bus controller has a clock that it
can manage around the I/O accesses.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 7a21950da77c..9ada89355747 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -678,7 +678,5 @@ void bcmgenet_mii_exit(struct net_device *dev)
 	if (of_phy_is_fixed_link(dn))
 		of_phy_deregister_fixed_link(dn);
 	of_node_put(priv->phy_dn);
-	clk_prepare_enable(priv->clk);
 	platform_device_unregister(priv->mii_pdev);
-	clk_disable_unprepare(priv->clk);
 }
-- 
2.34.1

Re: [PATCH net-next 1/3] net: mdio: mdio-bcm-unimac: Manage clock around I/O accesses

[Jacob Keller]

On 2/16/2024 10:42 AM, Florian Fainelli wrote:
> Up until now we have managed not to have the mdio-bcm-unimac manage its
> clock except during probe and suspend/resume. This works most of the
> time, except where it does not.
> 

This made me chuckle :)

> With a fully modular build, we can get into a situation whereby the
> GENET driver is fully registered, and so is the mdio-bcm-unimac driver,
> however the Ethernet PHY driver is not yet, because it depends on a
> resource that is not yet available (e.g.: GPIO provider). In that state,
> the network device is not usable yet, and so to conserve power, the
> GENET driver will have turned off its "main" clock which feeds its MDIO
> controller.
> 
> When the PHY driver finally probes however, we make an access to the PHY
> registers to e.g.: disable interrupts, and this causes a bus error
> within the MDIO controller space because the MDIO controller clock(s)
> are turned off.
> 
> To remedy that, we manage the clock around all of the I/O accesses to
> the hardware which are done exclusively during read, write and clock
> divider configuration.
> 
> This ensures that the register space is accessible, and this also
> ensures that there are not unnecessarily elevated reference counts
> keeping the clocks active when the network device is administratively
> turned off. It would be the case with the previous way of managing the
> clock.
> 

Good description of the issues.

> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net-next 2/3] net: bcmgenet: Pass "main" clock down to the MDIO driver

[Jacob Keller]

On 2/16/2024 10:42 AM, Florian Fainelli wrote:
> GENET has historically had to create a MDIO platform device for its
> controller and pass some auxiliary data to it, like a MDIO completion
> callback. Now we also pass the "main" clock to allow for the MDIO bus
> controller to manage that clock adequately around I/O accesses.
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> index cbbe004621bc..7a21950da77c 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -476,6 +476,10 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
>  	ppd.wait_func = bcmgenet_mii_wait;
>  	ppd.wait_func_data = priv;
>  	ppd.bus_name = "bcmgenet MII bus";
> +	/* Pass a reference to our "main" clock which is used for MDIO
> +	 * transfers
> +	 */
> +	ppd.clk = priv->clk;
>  
>  	/* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD
>  	 * and is 2 * 32-bits word long, 8 bytes total.

Is this missing a modification of the header file to add the clk field
to struct unimac_mdio_pdata? I don't see that field in the
include/linux/platform_data/mdio-bcm-unimac.h header currently...

Oh. you included that in the first patch of the series. I see.

It feels like the series would be more natural of this was 1/3 instead
of 2/3, since the current 1/3 patch depends on this clk value being set, no?

The result of the series makes sense tho:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net-next 3/3] Revert "net: bcmgenet: Ensure MDIO unregistration has clocks enabled"

[Jacob Keller]

On 2/16/2024 10:42 AM, Florian Fainelli wrote:
> This reverts commit eac0aac07f6af47b8ba57c8064bf04ed6df73d1d ("net:
> bcmgenet: Ensure MDIO unregistration has clocks enabled"). This is no
> longer necessary now that the MDIO bus controller has a clock that it
> can manage around the I/O accesses.
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net-next 2/3] net: bcmgenet: Pass "main" clock down to the MDIO driver

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]

On 2/16/24 14:41, Jacob Keller wrote:
> 
> 
> On 2/16/2024 10:42 AM, Florian Fainelli wrote:
>> GENET has historically had to create a MDIO platform device for its
>> controller and pass some auxiliary data to it, like a MDIO completion
>> callback. Now we also pass the "main" clock to allow for the MDIO bus
>> controller to manage that clock adequately around I/O accesses.
>>
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
>>   drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> index cbbe004621bc..7a21950da77c 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> @@ -476,6 +476,10 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
>>   	ppd.wait_func = bcmgenet_mii_wait;
>>   	ppd.wait_func_data = priv;
>>   	ppd.bus_name = "bcmgenet MII bus";
>> +	/* Pass a reference to our "main" clock which is used for MDIO
>> +	 * transfers
>> +	 */
>> +	ppd.clk = priv->clk;
>>   
>>   	/* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD
>>   	 * and is 2 * 32-bits word long, 8 bytes total.
> 
> Is this missing a modification of the header file to add the clk field
> to struct unimac_mdio_pdata? I don't see that field in the
> include/linux/platform_data/mdio-bcm-unimac.h header currently...
> 
> Oh. you included that in the first patch of the series. I see.

I suppose I could have included it in patch #1, and have introduced in 
patch #2 the hunk that dealt with fetching pdata->clk, but since I was 
re-organizing the mdio-bcm-unimac.c driver's probe function, it felt 
more natural to arrange it that way.

> 
> It feels like the series would be more natural of this was 1/3 instead
> of 2/3, since the current 1/3 patch depends on this clk value being set, no?

I sort of debated that with myself, and ended up going with: put the 
plumbing first, wire it later, rather than the opposite. If someone was 
to run a bisection there would not be any difference in behavior until 
patch #2 regardless of their ordering.

> 
> The result of the series makes sense tho:
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks Jacob!
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 1/3] net: mdio: mdio-bcm-unimac: Manage clock around I/O accesses

[kernel test robot]

Hi Florian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Florian-Fainelli/net-mdio-mdio-bcm-unimac-Manage-clock-around-I-O-accesses/20240217-024738
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240216184237.259954-2-florian.fainelli%40broadcom.com
patch subject: [PATCH net-next 1/3] net: mdio: mdio-bcm-unimac: Manage clock around I/O accesses
config: arm-defconfig (https://download.01.org/0day-ci/archive/20240217/202402171801.J560K7Fo-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240217/202402171801.J560K7Fo-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402171801.J560K7Fo-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/mdio/mdio-bcm-unimac.c:286:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (IS_ERR(priv->clk))
               ^~~~~~~~~~~~~~~~~
   drivers/net/mdio/mdio-bcm-unimac.c:313:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   drivers/net/mdio/mdio-bcm-unimac.c:286:2: note: remove the 'if' if its condition is always false
           if (IS_ERR(priv->clk))
           ^~~~~~~~~~~~~~~~~~~~~~
   drivers/net/mdio/mdio-bcm-unimac.c:243:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +286 drivers/net/mdio/mdio-bcm-unimac.c

   235	
   236	static int unimac_mdio_probe(struct platform_device *pdev)
   237	{
   238		struct unimac_mdio_pdata *pdata = pdev->dev.platform_data;
   239		struct unimac_mdio_priv *priv;
   240		struct device_node *np;
   241		struct mii_bus *bus;
   242		struct resource *r;
   243		int ret;
   244	
   245		np = pdev->dev.of_node;
   246	
   247		priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
   248		if (!priv)
   249			return -ENOMEM;
   250	
   251		r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   252		if (!r)
   253			return -EINVAL;
   254	
   255		/* Just ioremap, as this MDIO block is usually integrated into an
   256		 * Ethernet MAC controller register range
   257		 */
   258		priv->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
   259		if (!priv->base) {
   260			dev_err(&pdev->dev, "failed to remap register\n");
   261			return -ENOMEM;
   262		}
   263	
   264		if (of_property_read_u32(np, "clock-frequency", &priv->clk_freq))
   265			priv->clk_freq = 0;
   266	
   267		priv->mii_bus = mdiobus_alloc();
   268		if (!priv->mii_bus)
   269			return -ENOMEM;
   270	
   271		bus = priv->mii_bus;
   272		bus->priv = priv;
   273		if (pdata) {
   274			bus->name = pdata->bus_name;
   275			priv->wait_func = pdata->wait_func;
   276			priv->wait_func_data = pdata->wait_func_data;
   277			bus->phy_mask = ~pdata->phy_mask;
   278			priv->clk = pdata->clk;
   279		} else {
   280			bus->name = "unimac MII bus";
   281			priv->wait_func_data = priv;
   282			priv->wait_func = unimac_mdio_poll;
   283			priv->clk = devm_clk_get_optional(&pdev->dev, NULL);
   284		}
   285	
 > 286		if (IS_ERR(priv->clk))
   287			goto out_mdio_free;
   288	
   289		bus->parent = &pdev->dev;
   290		bus->read = unimac_mdio_read;
   291		bus->write = unimac_mdio_write;
   292		bus->reset = unimac_mdio_reset;
   293		snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d", pdev->name, pdev->id);
   294	
   295		ret = unimac_mdio_clk_set(priv);
   296		if (ret)
   297			goto out_mdio_free;
   298	
   299		ret = of_mdiobus_register(bus, np);
   300		if (ret) {
   301			dev_err(&pdev->dev, "MDIO bus registration failed\n");
   302			goto out_mdio_free;
   303		}
   304	
   305		platform_set_drvdata(pdev, priv);
   306	
   307		dev_info(&pdev->dev, "Broadcom UniMAC MDIO bus\n");
   308	
   309		return 0;
   310	
   311	out_mdio_free:
   312		mdiobus_free(bus);
   313		return ret;
   314	}
   315	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 2/3] net: bcmgenet: Pass "main" clock down to the MDIO driver

[Andrew Lunn]

On Fri, Feb 16, 2024 at 10:42:36AM -0800, Florian Fainelli wrote:
> GENET has historically had to create a MDIO platform device for its
> controller and pass some auxiliary data to it, like a MDIO completion
> callback. Now we also pass the "main" clock to allow for the MDIO bus
> controller to manage that clock adequately around I/O accesses.

I guess this code comes from before the times of DT? I would normally
expect to see a clock added to the DT node for the MDIO bus. But if
there is no node, because it is not in DT....

      Andrew

Re: [PATCH net-next 2/3] net: bcmgenet: Pass "main" clock down to the MDIO driver

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

Hi Andrew,

On 2/17/2024 7:21 AM, Andrew Lunn wrote:
> On Fri, Feb 16, 2024 at 10:42:36AM -0800, Florian Fainelli wrote:
>> GENET has historically had to create a MDIO platform device for its
>> controller and pass some auxiliary data to it, like a MDIO completion
>> callback. Now we also pass the "main" clock to allow for the MDIO bus
>> controller to manage that clock adequately around I/O accesses.
> 
> I guess this code comes from before the times of DT? I would normally
> expect to see a clock added to the DT node for the MDIO bus. But if
> there is no node, because it is not in DT....

The driver started being DT-only from the get go, however it was also my 
group's first attempt at upstreaming a driver and we did not get 
everything right in terms of the DT binding. In particular there was no 
"mdio" sub-node initially, but we still wanted to be able to split up 
the MDIO controller part since we knew it was going to be re-used in 
other designs (bcm_sf2 and later asp2). The platform device was the best 
we could come up with at the time.

All of our DTBs deployed out there do not have a "clocks" property 
within the "mdio" sub-mode of the GENET Ethernet node, so that is why we 
are doing this.

Thanks!
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]