| Message ID | 20230120085009.604797-3-tomi.valkeinen+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [v2,1/7] drm: rcar-du: dsi: add 'select RESET_CONTROLLER' | expand |
Hi Tomi, Thank you for the patch. On Fri, Jan 20, 2023 at 10:50:04AM +0200, Tomi Valkeinen wrote: > Add simple runtime PM suspend and resume functionality. I think you need to depend on PM in Kconfig. That's not a compile-time dependency but a runtime-dependency, with runtime PM support the suspend/resume handler will never be called. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_lvds.c | 43 +++++++++++++++++++++++++---- > 1 file changed, 37 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > index 81a060c2fe3f..8e1be51fbee6 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > @@ -16,6 +16,7 @@ > #include <linux/of_device.h> > #include <linux/of_graph.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > #include <linux/slab.h> > #include <linux/sys_soc.h> > > @@ -316,8 +317,8 @@ int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq) > > dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq); > > - ret = clk_prepare_enable(lvds->clocks.mod); > - if (ret < 0) > + ret = pm_runtime_resume_and_get(lvds->dev); > + if (ret) > return ret; > > __rcar_lvds_pll_setup_d3_e3(lvds, freq, true); > @@ -337,7 +338,7 @@ void rcar_lvds_pclk_disable(struct drm_bridge *bridge) > > rcar_lvds_write(lvds, LVDPLLCR, 0); > > - clk_disable_unprepare(lvds->clocks.mod); > + pm_runtime_put(lvds->dev); Should we use pm_runtime_put_sync() here, to make sure the clock gets disabled right away ? The DU hardware may depend on the exact sequencing of events. I would then do the same in rcar_lvds_atomic_disable(). Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > EXPORT_SYMBOL_GPL(rcar_lvds_pclk_disable); > > @@ -396,8 +397,8 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge, > u32 lvdcr0; > int ret; > > - ret = clk_prepare_enable(lvds->clocks.mod); > - if (ret < 0) > + ret = pm_runtime_resume_and_get(lvds->dev); > + if (ret) > return; > > /* Enable the companion LVDS encoder in dual-link mode. */ > @@ -551,7 +552,7 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge, > lvds->companion->funcs->atomic_disable(lvds->companion, > old_bridge_state); > > - clk_disable_unprepare(lvds->clocks.mod); > + pm_runtime_put(lvds->dev); > } > > static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge, > @@ -844,6 +845,8 @@ static int rcar_lvds_probe(struct platform_device *pdev) > if (ret < 0) > return ret; > > + pm_runtime_enable(&pdev->dev); > + > drm_bridge_add(&lvds->bridge); > > return 0; > @@ -855,6 +858,8 @@ static int rcar_lvds_remove(struct platform_device *pdev) > > drm_bridge_remove(&lvds->bridge); > > + pm_runtime_disable(&pdev->dev); > + > return 0; > } > > @@ -913,11 +918,37 @@ static const struct of_device_id rcar_lvds_of_table[] = { > > MODULE_DEVICE_TABLE(of, rcar_lvds_of_table); > > +static int rcar_lvds_runtime_suspend(struct device *dev) > +{ > + struct rcar_lvds *lvds = dev_get_drvdata(dev); > + > + clk_disable_unprepare(lvds->clocks.mod); > + > + return 0; > +} > + > +static int rcar_lvds_runtime_resume(struct device *dev) > +{ > + struct rcar_lvds *lvds = dev_get_drvdata(dev); > + int ret; > + > + ret = clk_prepare_enable(lvds->clocks.mod); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static const struct dev_pm_ops rcar_lvds_pm_ops = { > + SET_RUNTIME_PM_OPS(rcar_lvds_runtime_suspend, rcar_lvds_runtime_resume, NULL) > +}; > + > static struct platform_driver rcar_lvds_platform_driver = { > .probe = rcar_lvds_probe, > .remove = rcar_lvds_remove, > .driver = { > .name = "rcar-lvds", > + .pm = &rcar_lvds_pm_ops, > .of_match_table = rcar_lvds_of_table, > }, > };
On 20/01/2023 18:16, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Fri, Jan 20, 2023 at 10:50:04AM +0200, Tomi Valkeinen wrote: >> Add simple runtime PM suspend and resume functionality. > > I think you need to depend on PM in Kconfig. That's not a compile-time > dependency but a runtime-dependency, with runtime PM support the > suspend/resume handler will never be called. Yes, LVDS won't work without runtime PM after this patch. >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> drivers/gpu/drm/rcar-du/rcar_lvds.c | 43 +++++++++++++++++++++++++---- >> 1 file changed, 37 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c >> index 81a060c2fe3f..8e1be51fbee6 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c >> @@ -16,6 +16,7 @@ >> #include <linux/of_device.h> >> #include <linux/of_graph.h> >> #include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> #include <linux/slab.h> >> #include <linux/sys_soc.h> >> >> @@ -316,8 +317,8 @@ int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq) >> >> dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq); >> >> - ret = clk_prepare_enable(lvds->clocks.mod); >> - if (ret < 0) >> + ret = pm_runtime_resume_and_get(lvds->dev); >> + if (ret) >> return ret; >> >> __rcar_lvds_pll_setup_d3_e3(lvds, freq, true); >> @@ -337,7 +338,7 @@ void rcar_lvds_pclk_disable(struct drm_bridge *bridge) >> >> rcar_lvds_write(lvds, LVDPLLCR, 0); >> >> - clk_disable_unprepare(lvds->clocks.mod); >> + pm_runtime_put(lvds->dev); > > Should we use pm_runtime_put_sync() here, to make sure the clock gets > disabled right away ? The DU hardware may depend on the exact sequencing > of events. I would then do the same in rcar_lvds_atomic_disable(). That's perhaps a good idea. I think I saw some of the docs saying that startup sequences must begin with the reset. If we don't use _sync, we could end up not resetting at all. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! Tomi
Hi Laurent, On Fri, Jan 20, 2023 at 5:16 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Fri, Jan 20, 2023 at 10:50:04AM +0200, Tomi Valkeinen wrote: > > Add simple runtime PM suspend and resume functionality. > > I think you need to depend on PM in Kconfig. That's not a compile-time > dependency but a runtime-dependency, with runtime PM support the > suspend/resume handler will never be called. While technically that is correct, you'll have a hard time getting here with CONFIG_PM=n, as both ARCH_RCAR_GEN2 and ARCH_RCAR_GEN3 do select PM. 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
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 81a060c2fe3f..8e1be51fbee6 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -16,6 +16,7 @@ #include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #include <linux/slab.h> #include <linux/sys_soc.h> @@ -316,8 +317,8 @@ int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq) dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq); - ret = clk_prepare_enable(lvds->clocks.mod); - if (ret < 0) + ret = pm_runtime_resume_and_get(lvds->dev); + if (ret) return ret; __rcar_lvds_pll_setup_d3_e3(lvds, freq, true); @@ -337,7 +338,7 @@ void rcar_lvds_pclk_disable(struct drm_bridge *bridge) rcar_lvds_write(lvds, LVDPLLCR, 0); - clk_disable_unprepare(lvds->clocks.mod); + pm_runtime_put(lvds->dev); } EXPORT_SYMBOL_GPL(rcar_lvds_pclk_disable); @@ -396,8 +397,8 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge, u32 lvdcr0; int ret; - ret = clk_prepare_enable(lvds->clocks.mod); - if (ret < 0) + ret = pm_runtime_resume_and_get(lvds->dev); + if (ret) return; /* Enable the companion LVDS encoder in dual-link mode. */ @@ -551,7 +552,7 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge, lvds->companion->funcs->atomic_disable(lvds->companion, old_bridge_state); - clk_disable_unprepare(lvds->clocks.mod); + pm_runtime_put(lvds->dev); } static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge, @@ -844,6 +845,8 @@ static int rcar_lvds_probe(struct platform_device *pdev) if (ret < 0) return ret; + pm_runtime_enable(&pdev->dev); + drm_bridge_add(&lvds->bridge); return 0; @@ -855,6 +858,8 @@ static int rcar_lvds_remove(struct platform_device *pdev) drm_bridge_remove(&lvds->bridge); + pm_runtime_disable(&pdev->dev); + return 0; } @@ -913,11 +918,37 @@ static const struct of_device_id rcar_lvds_of_table[] = { MODULE_DEVICE_TABLE(of, rcar_lvds_of_table); +static int rcar_lvds_runtime_suspend(struct device *dev) +{ + struct rcar_lvds *lvds = dev_get_drvdata(dev); + + clk_disable_unprepare(lvds->clocks.mod); + + return 0; +} + +static int rcar_lvds_runtime_resume(struct device *dev) +{ + struct rcar_lvds *lvds = dev_get_drvdata(dev); + int ret; + + ret = clk_prepare_enable(lvds->clocks.mod); + if (ret < 0) + return ret; + + return 0; +} + +static const struct dev_pm_ops rcar_lvds_pm_ops = { + SET_RUNTIME_PM_OPS(rcar_lvds_runtime_suspend, rcar_lvds_runtime_resume, NULL) +}; + static struct platform_driver rcar_lvds_platform_driver = { .probe = rcar_lvds_probe, .remove = rcar_lvds_remove, .driver = { .name = "rcar-lvds", + .pm = &rcar_lvds_pm_ops, .of_match_table = rcar_lvds_of_table, }, };
Add simple runtime PM suspend and resume functionality. Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> --- drivers/gpu/drm/rcar-du/rcar_lvds.c | 43 +++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 6 deletions(-)