Merge branch 'rework-genet-mdioclocking'

Florian Fainelli says:

====================
Rework GENET MDIO controller clocking

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.

Changes in v2:

- added missing ret assignment in the if (IS_ERR(priv->clk)) branch

- added Jacob's R-by tags

- corrected the commit ID being reverted in patch #3
====================

Signed-off-by: David S. Miller <[e-mail]>

drivers/net/ethernet/broadcom/genet/bcmmii.c

···
 	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.
···
 	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);
 }

drivers/net/mdio/mdio-bcm-unimac.c

···
 	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);
···
 
 	ret = priv->wait_func(priv->wait_func_data);
 	if (ret)
-		return ret;
+		goto out;
 
 	cmd = unimac_mdio_readl(priv, MDIO_CMD);
 
···
 	 * 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,
···
 {
 	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) |
···
 
 	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
···
 	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;
···
 	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
···
 	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)
···
 		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;
···
 		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)) {
+		ret = PTR_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) {
···
 
 out_mdio_free:
 	mdiobus_free(bus);
-out_clk_disable:
-	clk_disable_unprepare(priv->clk);
 	return ret;
 }
 
···
 
 	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", },

include/linux/platform_data/mdio-bcm-unimac.h

···
 #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"