| Message ID | 20240216184237.259954-2-florian.fainelli@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rework GENET MDIO controller clocking | expand |
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>
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
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"
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(-)