| Message ID | 20231120084606.4083194-9-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 |
On 11/20/23 11:46 AM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > For RZ/G3S and RZ/G2L SoCs the Ethernet's reference clock is part of the > Ethernet's power domain. It is controlled though CPG driver that is > providing the support for power domain that Ethernet belongs. Thus, > to be able to implement runtime PM (at least for RZ/G3S at the moment) > w/o the need to add clock enable/disable specific calls in runtime PM > ops of ravb driver and interfere with other IP specific implementations, > add a new variable to struct_hw_info and enable the reference clock > based on the value of this variable (the variable states if reference > clock is part of the Ethernet's power domain). > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
Hi Claudiu, Thanks for your patch (which seems to have been delayed by 3 days, ouch)! On Thu, Nov 23, 2023 at 5:35 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > For RZ/G3S and RZ/G2L SoCs the Ethernet's reference clock is part of the > Ethernet's power domain. It is controlled though CPG driver that is > providing the support for power domain that Ethernet belongs. Thus, > to be able to implement runtime PM (at least for RZ/G3S at the moment) Why only for RZ/G3S? > w/o the need to add clock enable/disable specific calls in runtime PM > ops of ravb driver and interfere with other IP specific implementations, > add a new variable to struct_hw_info and enable the reference clock > based on the value of this variable (the variable states if reference > clock is part of the Ethernet's power domain). > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -1043,6 +1043,7 @@ struct ravb_hw_info { > unsigned nc_queues:1; /* AVB-DMAC has RX and TX NC queues */ > unsigned magic_pkt:1; /* E-MAC supports magic packet detection */ > unsigned half_duplex:1; /* E-MAC supports half duplex mode */ > + unsigned refclk_in_pd:1; /* Reference clock is part of a power domain. */ > }; > > struct ravb_private { > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 836fdb4b3bfd..ddd8cd2c0f89 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2502,6 +2502,7 @@ static const struct ravb_hw_info gbeth_hw_info = { > .tx_counters = 1, > .carrier_counters = 1, > .half_duplex = 1, > + .refclk_in_pd = 1, > }; > > static const struct of_device_id ravb_match_table[] = { > @@ -2749,12 +2750,14 @@ static int ravb_probe(struct platform_device *pdev) > goto out_release; > } > > - priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); > - if (IS_ERR(priv->refclk)) { > - error = PTR_ERR(priv->refclk); > - goto out_release; > + if (!info->refclk_in_pd) { > + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); > + if (IS_ERR(priv->refclk)) { > + error = PTR_ERR(priv->refclk); > + goto out_release; > + } > + clk_prepare_enable(priv->refclk); > } > - clk_prepare_enable(priv->refclk); Is this patch really needed? It doesn't hurt to manually enable a clock that is also under Runtime PM control. Clock prepare/enable refcounting will take care of that. > > if (info->gptp_ref_clk) { > priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp"); > @@ -2869,7 +2872,8 @@ static int ravb_probe(struct platform_device *pdev) > if (info->ccc_gac) > ravb_ptp_stop(ndev); > out_disable_refclk: > - clk_disable_unprepare(priv->refclk); > + if (!info->refclk_in_pd) > + clk_disable_unprepare(priv->refclk); > out_release: > free_netdev(ndev); > pm_runtime_put: > @@ -2890,7 +2894,8 @@ static void ravb_remove(struct platform_device *pdev) > if (info->ccc_gac) > ravb_ptp_stop(ndev); > > - clk_disable_unprepare(priv->refclk); > + if (!info->refclk_in_pd) > + clk_disable_unprepare(priv->refclk); > > /* Set reset mode */ > ravb_write(ndev, CCC_OPC_RESET, CCC); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi, Geert, On 23.11.2023 10:48, Geert Uytterhoeven wrote: > Hi Claudiu, > > Thanks for your patch (which seems to have been delayed by 3 days, ouch)! > > On Thu, Nov 23, 2023 at 5:35 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> For RZ/G3S and RZ/G2L SoCs the Ethernet's reference clock is part of the >> Ethernet's power domain. It is controlled though CPG driver that is >> providing the support for power domain that Ethernet belongs. Thus, >> to be able to implement runtime PM (at least for RZ/G3S at the moment) > > Why only for RZ/G3S? (I'm copy pasting here what I already replied to Sergey) The reasons I've limited only to RZ/G3S are: 1/ I don't have all the platforms to test it 2/ on G1H this doesn't work. I tried to debugged it but I don't have a platform at hand, only remotely, and is hardly to debug once the ethernet fails to work: probe is working(), open is executed, PHY is initialized and then TX/RX is not working... don't know why ATM. > >> w/o the need to add clock enable/disable specific calls in runtime PM >> ops of ravb driver and interfere with other IP specific implementations, >> add a new variable to struct_hw_info and enable the reference clock >> based on the value of this variable (the variable states if reference >> clock is part of the Ethernet's power domain). >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> --- a/drivers/net/ethernet/renesas/ravb.h >> +++ b/drivers/net/ethernet/renesas/ravb.h >> @@ -1043,6 +1043,7 @@ struct ravb_hw_info { >> unsigned nc_queues:1; /* AVB-DMAC has RX and TX NC queues */ >> unsigned magic_pkt:1; /* E-MAC supports magic packet detection */ >> unsigned half_duplex:1; /* E-MAC supports half duplex mode */ >> + unsigned refclk_in_pd:1; /* Reference clock is part of a power domain. */ >> }; >> >> struct ravb_private { >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index 836fdb4b3bfd..ddd8cd2c0f89 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -2502,6 +2502,7 @@ static const struct ravb_hw_info gbeth_hw_info = { >> .tx_counters = 1, >> .carrier_counters = 1, >> .half_duplex = 1, >> + .refclk_in_pd = 1, >> }; >> >> static const struct of_device_id ravb_match_table[] = { >> @@ -2749,12 +2750,14 @@ static int ravb_probe(struct platform_device *pdev) >> goto out_release; >> } >> >> - priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); >> - if (IS_ERR(priv->refclk)) { >> - error = PTR_ERR(priv->refclk); >> - goto out_release; >> + if (!info->refclk_in_pd) { >> + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); >> + if (IS_ERR(priv->refclk)) { >> + error = PTR_ERR(priv->refclk); >> + goto out_release; >> + } >> + clk_prepare_enable(priv->refclk); >> } >> - clk_prepare_enable(priv->refclk); > > Is this patch really needed? It doesn't hurt to manually enable a > clock that is also under Runtime PM control. Clock prepare/enable > refcounting will take care of that. I agree with that. I chose this path to not interfere w/ the comments ravb_runtime_nop() which I didn't understand. Also I fail to understand why the ravb_runtime_nop() is there... > >> >> if (info->gptp_ref_clk) { >> priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp"); >> @@ -2869,7 +2872,8 @@ static int ravb_probe(struct platform_device *pdev) >> if (info->ccc_gac) >> ravb_ptp_stop(ndev); >> out_disable_refclk: >> - clk_disable_unprepare(priv->refclk); >> + if (!info->refclk_in_pd) >> + clk_disable_unprepare(priv->refclk); >> out_release: >> free_netdev(ndev); >> pm_runtime_put: >> @@ -2890,7 +2894,8 @@ static void ravb_remove(struct platform_device *pdev) >> if (info->ccc_gac) >> ravb_ptp_stop(ndev); >> >> - clk_disable_unprepare(priv->refclk); >> + if (!info->refclk_in_pd) >> + clk_disable_unprepare(priv->refclk); >> >> /* Set reset mode */ >> ravb_write(ndev, CCC_OPC_RESET, CCC); > > Gr{oetje,eeting}s, > > Geert > > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On 11/23/23 8:10 PM, claudiu beznea wrote: [...] >> On Thu, Nov 23, 2023 at 5:35 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> [...] >>> w/o the need to add clock enable/disable specific calls in runtime PM >>> ops of ravb driver and interfere with other IP specific implementations, >>> add a new variable to struct_hw_info and enable the reference clock >>> based on the value of this variable (the variable states if reference >>> clock is part of the Ethernet's power domain). >>> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> [...] >>> static const struct of_device_id ravb_match_table[] = { >>> @@ -2749,12 +2750,14 @@ static int ravb_probe(struct platform_device *pdev) >>> goto out_release; >>> } >>> >>> - priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); >>> - if (IS_ERR(priv->refclk)) { >>> - error = PTR_ERR(priv->refclk); >>> - goto out_release; >>> + if (!info->refclk_in_pd) { >>> + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); >>> + if (IS_ERR(priv->refclk)) { >>> + error = PTR_ERR(priv->refclk); >>> + goto out_release; >>> + } >>> + clk_prepare_enable(priv->refclk); >>> } >>> - clk_prepare_enable(priv->refclk); >> >> Is this patch really needed? It doesn't hurt to manually enable a >> clock that is also under Runtime PM control. Clock prepare/enable >> refcounting will take care of that. > > I agree with that. I chose this path to not interfere w/ the comments > ravb_runtime_nop() which I didn't understand. Also I fail to understand why > the ravb_runtime_nop() is there... Looks like it was blindly copied from the sh_eth driver and doesn't (yet?) apply to ravb... [...] MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index e0f8276cffed..c2d8d890031f 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -1043,6 +1043,7 @@ struct ravb_hw_info { unsigned nc_queues:1; /* AVB-DMAC has RX and TX NC queues */ unsigned magic_pkt:1; /* E-MAC supports magic packet detection */ unsigned half_duplex:1; /* E-MAC supports half duplex mode */ + unsigned refclk_in_pd:1; /* Reference clock is part of a power domain. */ }; struct ravb_private { diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 836fdb4b3bfd..ddd8cd2c0f89 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2502,6 +2502,7 @@ static const struct ravb_hw_info gbeth_hw_info = { .tx_counters = 1, .carrier_counters = 1, .half_duplex = 1, + .refclk_in_pd = 1, }; static const struct of_device_id ravb_match_table[] = { @@ -2749,12 +2750,14 @@ static int ravb_probe(struct platform_device *pdev) goto out_release; } - priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); - if (IS_ERR(priv->refclk)) { - error = PTR_ERR(priv->refclk); - goto out_release; + if (!info->refclk_in_pd) { + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); + if (IS_ERR(priv->refclk)) { + error = PTR_ERR(priv->refclk); + goto out_release; + } + clk_prepare_enable(priv->refclk); } - clk_prepare_enable(priv->refclk); if (info->gptp_ref_clk) { priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp"); @@ -2869,7 +2872,8 @@ static int ravb_probe(struct platform_device *pdev) if (info->ccc_gac) ravb_ptp_stop(ndev); out_disable_refclk: - clk_disable_unprepare(priv->refclk); + if (!info->refclk_in_pd) + clk_disable_unprepare(priv->refclk); out_release: free_netdev(ndev); pm_runtime_put: @@ -2890,7 +2894,8 @@ static void ravb_remove(struct platform_device *pdev) if (info->ccc_gac) ravb_ptp_stop(ndev); - clk_disable_unprepare(priv->refclk); + if (!info->refclk_in_pd) + clk_disable_unprepare(priv->refclk); /* Set reset mode */ ravb_write(ndev, CCC_OPC_RESET, CCC);