On 1/5/24 11:23 AM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> > > Reference clock could be or not part of the power domain. If it is part of > the power domain, the power domain takes care of propertly setting it. In > case it is not part of the power domain and full runtime PM support is > available in driver the clock will not be propertly disabled/enabled at > runtime. For this, keep the prepare/unprepare operations in the driver's > probe()/remove() functions and move the enable/disable in runtime PM > functions. > > Along with it, the other clock request operations were moved close to > reference clock request and prepare to have all the clock requests > specific code grouped together. > > Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx> It's not that I reviewed the squashed version of this patch... > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> > --- > > Changes in v3: > - squashed with patch 17/21 ("net: ravb: Keep clock request operations grouped > together") from v2 > - collected tags > > Changes in v2: > - this patch is new and follows the recommendations proposed in the > discussion of patch 08/13 ("net: ravb: Rely on PM domain to enable refclk") > from v2 > > drivers/net/ethernet/renesas/ravb_main.c | 110 ++++++++++++----------- > 1 file changed, 57 insertions(+), 53 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 844ac3306e93..4673cc2faec0 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c [...] > @@ -2697,10 +2692,37 @@ static int ravb_probe(struct platform_device *pdev) > priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE; > } > > + priv->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(priv->clk)) { > + error = PTR_ERR(priv->clk); > + goto out_reset_assert; > + } > + > + if (info->gptp_ref_clk) { > + priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp"); > + if (IS_ERR(priv->gptp_clk)) { > + error = PTR_ERR(priv->gptp_clk); > + goto out_reset_assert; > + } > + } > + > + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); > + if (IS_ERR(priv->refclk)) { > + error = PTR_ERR(priv->refclk); > + goto out_reset_assert; > + } > + clk_prepare(priv->refclk); > + > + platform_set_drvdata(pdev, ndev); Why exactly you had to move this line? > + pm_runtime_enable(&pdev->dev); > + error = pm_runtime_resume_and_get(&pdev->dev); > + if (error < 0) > + goto out_rpm_disable; > + > priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > if (IS_ERR(priv->addr)) { > error = PTR_ERR(priv->addr); > - goto out_release; > + goto out_rpm_put; > } > > /* The Ether-specific entries in the device structure. */ [...] > @@ -2871,8 +2872,6 @@ static int ravb_probe(struct platform_device *pdev) > netdev_info(ndev, "Base address at %#x, %pM, IRQ %d.\n", > (u32)ndev->base_addr, ndev->dev_addr, ndev->irq); > > - platform_set_drvdata(pdev, ndev); Hm, wasn't calling it here racy? > - > return 0; > > out_napi_del: [...] > @@ -3060,21 +3058,27 @@ static int ravb_resume(struct device *dev) > return ret; > } > > -static int ravb_runtime_nop(struct device *dev) > +static int ravb_runtime_suspend(struct device *dev) > { > - /* Runtime PM callback shared between ->runtime_suspend() > - * and ->runtime_resume(). Simply returns success. > - * > - * This driver re-initializes all registers after > - * pm_runtime_get_sync() anyway so there is no need > - * to save and restore registers here. > - */ Perhaps even worth a separate patch to completely remove this function which doesn't seem to make sense? [...] MBR, Sergey