| Message ID | 20231214114600.2451162-18-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On 12/14/23 2:45 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Keep clock request operations grouped togeter to have all clock-related > code in a single place. This makes the code simpler to follow. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v2: > - none; this patch is new > > drivers/net/ethernet/renesas/ravb_main.c | 28 ++++++++++++------------ > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 38999ef1ea85..a2a64c22ec41 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2768,6 +2768,20 @@ static int ravb_probe(struct platform_device *pdev) > if (error) > goto out_reset_assert; > > + 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); Hmm... I think we currently have all these calls in one place. Perhaps you just shouldn't have moved this code around? MBR, Sergey
On 16.12.2023 21:43, Sergey Shtylyov wrote: > On 12/14/23 2:45 PM, Claudiu wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Keep clock request operations grouped togeter to have all clock-related >> code in a single place. This makes the code simpler to follow. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> Changes in v2: >> - none; this patch is new >> >> drivers/net/ethernet/renesas/ravb_main.c | 28 ++++++++++++------------ >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index 38999ef1ea85..a2a64c22ec41 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -2768,6 +2768,20 @@ static int ravb_probe(struct platform_device *pdev) >> if (error) >> goto out_reset_assert; >> >> + 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); > > Hmm... I think we currently have all these calls in one place. > Perhaps you just shouldn't have moved this code around? refclk have been moved at this point due to runtime PM. As refclk was changed to be part of driver's runtime PM APIs we need to have it requested (and prepared) before pm_runtime_resume_and_get(). Calling pm_runtime_resume_and_get() will call driver's runtime PM resume. The idea with this patch was to have all clock requests (clk, gptp, refclk) in a single place (it's easier to follow the code this way, in my opinion). If you prefer I can squash this patch with patch 07/21 "net: ravb: Move reference clock enable/disable on runtime PM APIs". Please, let me know what do you think. > > MBR, Sergey
On 12/17/23 4:22 PM, claudiu beznea wrote: [...] >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> Keep clock request operations grouped togeter to have all clock-related >>> code in a single place. This makes the code simpler to follow. >>> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> --- >>> >>> Changes in v2: >>> - none; this patch is new >>> >>> drivers/net/ethernet/renesas/ravb_main.c | 28 ++++++++++++------------ >>> 1 file changed, 14 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>> index 38999ef1ea85..a2a64c22ec41 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -2768,6 +2768,20 @@ static int ravb_probe(struct platform_device *pdev) >>> if (error) >>> goto out_reset_assert; >>> >>> + 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); >> >> Hmm... I think we currently have all these calls in one place. >> Perhaps you just shouldn't have moved this code around? > > refclk have been moved at this point due to runtime PM. As refclk was > changed to be part of driver's runtime PM APIs we need to have it requested > (and prepared) before pm_runtime_resume_and_get(). Calling > pm_runtime_resume_and_get() will call driver's runtime PM resume. > > The idea with this patch was to have all clock requests (clk, gptp, refclk) > in a single place (it's easier to follow the code this way, in my opinion). > If you prefer I can squash this patch with patch 07/21 "net: ravb: Move > reference clock enable/disable on runtime PM APIs". Please, let me know > what do you think. Yes, please move all 3 clocks at once. MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 38999ef1ea85..a2a64c22ec41 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2768,6 +2768,20 @@ static int ravb_probe(struct platform_device *pdev) if (error) goto out_reset_assert; + 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); @@ -2801,20 +2815,6 @@ static int ravb_probe(struct platform_device *pdev) priv->avb_link_active_low = of_property_read_bool(np, "renesas,ether-link-active-low"); - priv->clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(priv->clk)) { - error = PTR_ERR(priv->clk); - goto out_rpm_put; - } - - 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_rpm_put; - } - } - ndev->max_mtu = info->rx_max_buf_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); ndev->min_mtu = ETH_MIN_MTU;