Message ID | 20240209170459.4143861-6-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net: ravb: Add runtime PM support (part 2) | expand |
On 2/9/24 8:04 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add runtime PM support for the ravb driver. As the driver is used by > different IP variants, with different behaviors, to be able to have the > runtime PM support available for all devices, the preparatory commits > moved all the resources parsing and allocations in the driver's probe > function and kept the settings for ravb_open(). This is due to the fact > that on some IP variants-platforms tuples disabling/enabling the clocks > will switch the IP to the reset operation mode where registers' content is This pesky "registers' content" somehow evaded me -- should be "register contents" as well... > lost and reconfiguration needs to be done. For this the rabv_open() > function enables the clocks, switches the IP to configuration mode, applies > all the registers settings and switches the IP to the operational mode. At > the end of ravb_open() IP is ready to send/receive data. > > In ravb_close() necessary reverts are done (compared with ravb_open()), the > IP is switched to reset mode and clocks are disabled. > > The ethtool APIs or IOCTLs that might execute while the interface is down > are either cached (and applied in ravb_open()) or rejected (as at that time > the IP is in reset mode). Keeping the IP in the reset mode also increases > the power saved (according to the hardware manual). > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index f4be08f0198d..5bbfdfeef8a9 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev) > { > struct ravb_private *priv = netdev_priv(ndev); > const struct ravb_hw_info *info = priv->info; > + struct device *dev = &priv->pdev->dev; > int error; > > napi_enable(&priv->napi[RAVB_BE]); > if (info->nc_queues) > napi_enable(&priv->napi[RAVB_NC]); > > + error = pm_runtime_resume_and_get(dev); > + if (error < 0) > + goto out_napi_off; Well, s/error/ret/ -- it would fit better here... [...] > @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev) > struct net_device *ndev = platform_get_drvdata(pdev); > struct ravb_private *priv = netdev_priv(ndev); > const struct ravb_hw_info *info = priv->info; > + struct device *dev = &priv->pdev->dev; > + int error; > + > + error = pm_runtime_resume_and_get(dev); > + if (error < 0) > + return; Again, s/erorr/ret/ in this case. [...] MBR, Sergey
On 09.02.2024 23:00, Sergey Shtylyov wrote: > On 2/9/24 8:04 PM, Claudiu wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Add runtime PM support for the ravb driver. As the driver is used by >> different IP variants, with different behaviors, to be able to have the >> runtime PM support available for all devices, the preparatory commits >> moved all the resources parsing and allocations in the driver's probe >> function and kept the settings for ravb_open(). This is due to the fact >> that on some IP variants-platforms tuples disabling/enabling the clocks >> will switch the IP to the reset operation mode where registers' content is > > This pesky "registers' content" somehow evaded me -- should be "register > contents" as well... > >> lost and reconfiguration needs to be done. For this the rabv_open() >> function enables the clocks, switches the IP to configuration mode, applies >> all the registers settings and switches the IP to the operational mode. At >> the end of ravb_open() IP is ready to send/receive data. >> >> In ravb_close() necessary reverts are done (compared with ravb_open()), the >> IP is switched to reset mode and clocks are disabled. >> >> The ethtool APIs or IOCTLs that might execute while the interface is down >> are either cached (and applied in ravb_open()) or rejected (as at that time >> the IP is in reset mode). Keeping the IP in the reset mode also increases >> the power saved (according to the hardware manual). >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > [...] > >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index f4be08f0198d..5bbfdfeef8a9 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev) >> { >> struct ravb_private *priv = netdev_priv(ndev); >> const struct ravb_hw_info *info = priv->info; >> + struct device *dev = &priv->pdev->dev; >> int error; >> >> napi_enable(&priv->napi[RAVB_BE]); >> if (info->nc_queues) >> napi_enable(&priv->napi[RAVB_NC]); >> >> + error = pm_runtime_resume_and_get(dev); >> + if (error < 0) >> + goto out_napi_off; > > Well, s/error/ret/ -- it would fit better here... Using error is the "trademark" of this driver, it is used all around the driver. I haven't introduced it here, I don't like it. The variable error in this particular function is here from the beginning of the driver. So, I don't consider changing error to ret is the scope of this series. > > [...] >> @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev) >> struct net_device *ndev = platform_get_drvdata(pdev); >> struct ravb_private *priv = netdev_priv(ndev); >> const struct ravb_hw_info *info = priv->info; >> + struct device *dev = &priv->pdev->dev; >> + int error; >> + >> + error = pm_runtime_resume_and_get(dev); >> + if (error < 0) >> + return; > > Again, s/erorr/ret/ in this case. error was used here to comply with the rest of the driver. So, if you still want me to change it here and in ravb_remove() please confirm. Thank you, Claudiu Beznea > > [...] > > MBR, Sergey
On 2/12/24 10:56 AM, claudiu beznea wrote: [...] >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> Add runtime PM support for the ravb driver. As the driver is used by >>> different IP variants, with different behaviors, to be able to have the >>> runtime PM support available for all devices, the preparatory commits >>> moved all the resources parsing and allocations in the driver's probe >>> function and kept the settings for ravb_open(). This is due to the fact >>> that on some IP variants-platforms tuples disabling/enabling the clocks >>> will switch the IP to the reset operation mode where registers' content is >> >> This pesky "registers' content" somehow evaded me -- should be "register >> contents" as well... >> >>> lost and reconfiguration needs to be done. For this the rabv_open() >>> function enables the clocks, switches the IP to configuration mode, applies >>> all the registers settings and switches the IP to the operational mode. At >>> the end of ravb_open() IP is ready to send/receive data. >>> >>> In ravb_close() necessary reverts are done (compared with ravb_open()), the >>> IP is switched to reset mode and clocks are disabled. >>> >>> The ethtool APIs or IOCTLs that might execute while the interface is down >>> are either cached (and applied in ravb_open()) or rejected (as at that time >>> the IP is in reset mode). Keeping the IP in the reset mode also increases >>> the power saved (according to the hardware manual). >>> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> [...] >> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>> index f4be08f0198d..5bbfdfeef8a9 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev) >>> { >>> struct ravb_private *priv = netdev_priv(ndev); >>> const struct ravb_hw_info *info = priv->info; >>> + struct device *dev = &priv->pdev->dev; >>> int error; >>> >>> napi_enable(&priv->napi[RAVB_BE]); >>> if (info->nc_queues) >>> napi_enable(&priv->napi[RAVB_NC]); >>> >>> + error = pm_runtime_resume_and_get(dev); >>> + if (error < 0) >>> + goto out_napi_off; >> >> Well, s/error/ret/ -- it would fit better here... > > Using error is the "trademark" of this driver, it is used all around the > driver. I haven't introduced it here, I don't like it. The variable error Heh, because it's my usual style. Too bad you don't like it... :-) > in this particular function is here from the beginning of the driver. I think it's well suited for the functions returning either 0 or a (negative) error code. It's *if* (error < 0) that confuses me (as this API can return positive numbers in case of success... > So, I don't consider changing error to ret is the scope of this series. OK, you're probably right... are you going to respin the series because of Biju's comments? [...] >>> @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev) >>> struct net_device *ndev = platform_get_drvdata(pdev); >>> struct ravb_private *priv = netdev_priv(ndev); >>> const struct ravb_hw_info *info = priv->info; >>> + struct device *dev = &priv->pdev->dev; >>> + int error; >>> + >>> + error = pm_runtime_resume_and_get(dev); >>> + if (error < 0) >>> + return; >> >> Again, s/erorr/ret/ in this case. > > error was used here to comply with the rest of the driver. So, if you still > want me to change it here and in ravb_remove() please confirm. No, we are good enough without that; I'll consider doing a cleanup when/if I have time. :-) > Thank you, > Claudiu Beznea MBR, Sergey
On 12.02.2024 22:19, Sergey Shtylyov wrote: > On 2/12/24 10:56 AM, claudiu beznea wrote: > > [...] > >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> Add runtime PM support for the ravb driver. As the driver is used by >>>> different IP variants, with different behaviors, to be able to have the >>>> runtime PM support available for all devices, the preparatory commits >>>> moved all the resources parsing and allocations in the driver's probe >>>> function and kept the settings for ravb_open(). This is due to the fact >>>> that on some IP variants-platforms tuples disabling/enabling the clocks >>>> will switch the IP to the reset operation mode where registers' content is >>> >>> This pesky "registers' content" somehow evaded me -- should be "register >>> contents" as well... >>> >>>> lost and reconfiguration needs to be done. For this the rabv_open() >>>> function enables the clocks, switches the IP to configuration mode, applies >>>> all the registers settings and switches the IP to the operational mode. At >>>> the end of ravb_open() IP is ready to send/receive data. >>>> >>>> In ravb_close() necessary reverts are done (compared with ravb_open()), the >>>> IP is switched to reset mode and clocks are disabled. >>>> >>>> The ethtool APIs or IOCTLs that might execute while the interface is down >>>> are either cached (and applied in ravb_open()) or rejected (as at that time >>>> the IP is in reset mode). Keeping the IP in the reset mode also increases >>>> the power saved (according to the hardware manual). >>>> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> [...] >>> >>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>>> index f4be08f0198d..5bbfdfeef8a9 100644 >>>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>>> @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev) >>>> { >>>> struct ravb_private *priv = netdev_priv(ndev); >>>> const struct ravb_hw_info *info = priv->info; >>>> + struct device *dev = &priv->pdev->dev; >>>> int error; >>>> >>>> napi_enable(&priv->napi[RAVB_BE]); >>>> if (info->nc_queues) >>>> napi_enable(&priv->napi[RAVB_NC]); >>>> >>>> + error = pm_runtime_resume_and_get(dev); >>>> + if (error < 0) >>>> + goto out_napi_off; >>> >>> Well, s/error/ret/ -- it would fit better here... >> >> Using error is the "trademark" of this driver, it is used all around the >> driver. I haven't introduced it here, I don't like it. The variable error > > Heh, because it's my usual style. Too bad you don't like it... :-) > >> in this particular function is here from the beginning of the driver. > > I think it's well suited for the functions returning either 0 or a > (negative) error code. It's *if* (error < 0) that confuses me (as this > API can return positive numbers in case of success... > >> So, I don't consider changing error to ret is the scope of this series. > > OK, you're probably right... are you going to respin the series because > of Biju's comments? Yes! Thank you, Claudiu Beznea > > [...] >>>> @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev) >>>> struct net_device *ndev = platform_get_drvdata(pdev); >>>> struct ravb_private *priv = netdev_priv(ndev); >>>> const struct ravb_hw_info *info = priv->info; >>>> + struct device *dev = &priv->pdev->dev; >>>> + int error; >>>> + >>>> + error = pm_runtime_resume_and_get(dev); >>>> + if (error < 0) >>>> + return; >>> >>> Again, s/erorr/ret/ in this case. >> >> error was used here to comply with the rest of the driver. So, if you still >> want me to change it here and in ravb_remove() please confirm. > > No, we are good enough without that; I'll consider doing a cleanup > when/if I have time. :-) > >> Thank you, >> Claudiu Beznea > > MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index f4be08f0198d..5bbfdfeef8a9 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev) { struct ravb_private *priv = netdev_priv(ndev); const struct ravb_hw_info *info = priv->info; + struct device *dev = &priv->pdev->dev; int error; napi_enable(&priv->napi[RAVB_BE]); if (info->nc_queues) napi_enable(&priv->napi[RAVB_NC]); + error = pm_runtime_resume_and_get(dev); + if (error < 0) + goto out_napi_off; + /* Set AVB config mode */ error = ravb_set_config_mode(ndev); if (error) - goto out_napi_off; + goto out_rpm_put; ravb_set_delay_mode(ndev); ravb_write(ndev, priv->desc_bat_dma, DBAT); @@ -1982,6 +1987,9 @@ static int ravb_open(struct net_device *ndev) ravb_stop_dma(ndev); out_set_reset: ravb_set_opmode(ndev, CCC_OPC_RESET); +out_rpm_put: + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); out_napi_off: if (info->nc_queues) napi_disable(&priv->napi[RAVB_NC]); @@ -2322,6 +2330,8 @@ static int ravb_close(struct net_device *ndev) struct ravb_private *priv = netdev_priv(ndev); const struct ravb_hw_info *info = priv->info; struct ravb_tstamp_skb *ts_skb, *ts_skb2; + struct device *dev = &priv->pdev->dev; + int error; netif_tx_stop_all_queues(ndev); @@ -2371,7 +2381,14 @@ static int ravb_close(struct net_device *ndev) ravb_get_stats(ndev); /* Set reset mode. */ - return ravb_set_opmode(ndev, CCC_OPC_RESET); + error = ravb_set_opmode(ndev, CCC_OPC_RESET); + if (error) + return error; + + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + + return 0; } static int ravb_hwtstamp_get(struct net_device *ndev, struct ifreq *req) @@ -2931,6 +2948,8 @@ static int ravb_probe(struct platform_device *pdev) clk_prepare(priv->refclk); platform_set_drvdata(pdev, ndev); + pm_runtime_set_autosuspend_delay(&pdev->dev, 100); + pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_enable(&pdev->dev); error = pm_runtime_resume_and_get(&pdev->dev); if (error < 0) @@ -3036,6 +3055,9 @@ 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); + pm_runtime_mark_last_busy(&pdev->dev); + pm_runtime_put_autosuspend(&pdev->dev); + return 0; out_napi_del: @@ -3053,6 +3075,7 @@ static int ravb_probe(struct platform_device *pdev) pm_runtime_put(&pdev->dev); out_rpm_disable: pm_runtime_disable(&pdev->dev); + pm_runtime_dont_use_autosuspend(&pdev->dev); clk_unprepare(priv->refclk); out_reset_assert: reset_control_assert(rstc); @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev) struct net_device *ndev = platform_get_drvdata(pdev); struct ravb_private *priv = netdev_priv(ndev); const struct ravb_hw_info *info = priv->info; + struct device *dev = &priv->pdev->dev; + int error; + + error = pm_runtime_resume_and_get(dev); + if (error < 0) + return; unregister_netdev(ndev); if (info->nc_queues) @@ -3077,8 +3106,9 @@ static void ravb_remove(struct platform_device *pdev) dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat, priv->desc_bat_dma); - pm_runtime_put_sync(&pdev->dev); + pm_runtime_put_sync_suspend(&pdev->dev); pm_runtime_disable(&pdev->dev); + pm_runtime_dont_use_autosuspend(dev); clk_unprepare(priv->refclk); reset_control_assert(priv->rstc); free_netdev(ndev); @@ -3160,6 +3190,10 @@ static int ravb_suspend(struct device *dev) if (ret) return ret; + ret = pm_runtime_force_suspend(&priv->pdev->dev); + if (ret) + return ret; + reset_assert: return reset_control_assert(priv->rstc); } @@ -3182,16 +3216,28 @@ static int ravb_resume(struct device *dev) ret = ravb_wol_restore(ndev); if (ret) return ret; + } else { + ret = pm_runtime_force_resume(dev); + if (ret) + return ret; } /* Reopening the interface will restore the device to the working state. */ ret = ravb_open(ndev); if (ret < 0) - return ret; + goto out_rpm_put; ravb_set_rx_mode(ndev); netif_device_attach(ndev); + return 0; + +out_rpm_put: + if (!priv->wol_enabled) { + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + } + return ret; }