| Message ID | 20230117135154.387208-3-tomi.valkeinen+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: rcar-du: Misc patches | expand |
Hi Tomi, On Tue, Jan 17, 2023 at 2:54 PM Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> wrote: > From: Koji Matsuoka <koji.matsuoka.xm@renesas.com> > > Reset LVDS using the reset control as CPG reset/release is required in > H/W manual sequence. > > Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com> > Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com> > [tomi.valkeinen: Rewrite the patch description] > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> Thanks for your patch! > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > @@ -844,6 +853,12 @@ static int rcar_lvds_probe(struct platform_device *pdev) > if (ret < 0) > return ret; > > + lvds->rstc = devm_reset_control_get(&pdev->dev, NULL); devm_reset_control_get_exclusive() > + if (IS_ERR(lvds->rstc)) { > + dev_err(&pdev->dev, "failed to get cpg reset\n"); > + return PTR_ERR(lvds->rstc); return dev_err_probe(...); > + } > + > drm_bridge_add(&lvds->bridge); > > return 0; 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 Tomi, Thank you for the patch. On Tue, Jan 17, 2023 at 03:51:50PM +0200, Tomi Valkeinen wrote: > From: Koji Matsuoka <koji.matsuoka.xm@renesas.com> > > Reset LVDS using the reset control as CPG reset/release is required in > H/W manual sequence. s@H/W@the hardware@ > Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com> > Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com> > [tomi.valkeinen: Rewrite the patch description] > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/Kconfig | 1 + > drivers/gpu/drm/rcar-du/rcar_lvds.c | 15 +++++++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig > index a8f862c68b4f..151e400b996d 100644 > --- a/drivers/gpu/drm/rcar-du/Kconfig > +++ b/drivers/gpu/drm/rcar-du/Kconfig > @@ -43,6 +43,7 @@ config DRM_RCAR_LVDS > select DRM_PANEL > select OF_FLATTREE > select OF_OVERLAY > + select RESET_CONTROLLER > > config DRM_RCAR_USE_MIPI_DSI > bool "R-Car DU MIPI DSI Encoder Support" > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > index 81a060c2fe3f..674b727cdaa2 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/reset.h> > #include <linux/slab.h> > #include <linux/sys_soc.h> > > @@ -60,6 +61,7 @@ struct rcar_lvds_device_info { > struct rcar_lvds { > struct device *dev; > const struct rcar_lvds_device_info *info; > + struct reset_control *rstc; > > struct drm_bridge bridge; > > @@ -316,6 +318,8 @@ int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq) > > dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq); > > + reset_control_deassert(lvds->rstc); Can this fail ? Same in __rcar_lvds_atomic_enable(). > + > ret = clk_prepare_enable(lvds->clocks.mod); It would be nice to switch this driver to runtime PM and move reset and clock handling to the suspend/resume handlers. A candidate for a future patch. > if (ret < 0) Missing reset_control_assert(). Same in other error paths if applicable, here and in __rcar_lvds_atomic_enable(). > return ret; > @@ -338,6 +342,8 @@ void rcar_lvds_pclk_disable(struct drm_bridge *bridge) > rcar_lvds_write(lvds, LVDPLLCR, 0); > > clk_disable_unprepare(lvds->clocks.mod); > + > + reset_control_assert(lvds->rstc); > } > EXPORT_SYMBOL_GPL(rcar_lvds_pclk_disable); > > @@ -396,6 +402,8 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge, > u32 lvdcr0; > int ret; > > + reset_control_deassert(lvds->rstc); > + > ret = clk_prepare_enable(lvds->clocks.mod); > if (ret < 0) > return; > @@ -552,6 +560,7 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge, > old_bridge_state); > > clk_disable_unprepare(lvds->clocks.mod); > + reset_control_assert(lvds->rstc); > } > > static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge, > @@ -844,6 +853,12 @@ static int rcar_lvds_probe(struct platform_device *pdev) > if (ret < 0) > return ret; > > + lvds->rstc = devm_reset_control_get(&pdev->dev, NULL); > + if (IS_ERR(lvds->rstc)) { > + dev_err(&pdev->dev, "failed to get cpg reset\n"); > + return PTR_ERR(lvds->rstc); > + } > + > drm_bridge_add(&lvds->bridge); > > return 0;
On 17/01/2023 18:04, Geert Uytterhoeven wrote: > Hi Tomi, > > On Tue, Jan 17, 2023 at 2:54 PM Tomi Valkeinen > <tomi.valkeinen+renesas@ideasonboard.com> wrote: >> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com> >> >> Reset LVDS using the reset control as CPG reset/release is required in >> H/W manual sequence. >> >> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com> >> Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com> >> [tomi.valkeinen: Rewrite the patch description] >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > > Thanks for your patch! > >> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > >> @@ -844,6 +853,12 @@ static int rcar_lvds_probe(struct platform_device *pdev) >> if (ret < 0) >> return ret; >> >> + lvds->rstc = devm_reset_control_get(&pdev->dev, NULL); > > devm_reset_control_get_exclusive() Ok. >> + if (IS_ERR(lvds->rstc)) { >> + dev_err(&pdev->dev, "failed to get cpg reset\n"); >> + return PTR_ERR(lvds->rstc); > > return dev_err_probe(...); Right. Thanks! Tomi
On 18/01/2023 23:06, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tue, Jan 17, 2023 at 03:51:50PM +0200, Tomi Valkeinen wrote: >> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com> >> >> Reset LVDS using the reset control as CPG reset/release is required in >> H/W manual sequence. > > s@H/W@the hardware@ > >> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com> >> Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com> >> [tomi.valkeinen: Rewrite the patch description] >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> drivers/gpu/drm/rcar-du/Kconfig | 1 + >> drivers/gpu/drm/rcar-du/rcar_lvds.c | 15 +++++++++++++++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig >> index a8f862c68b4f..151e400b996d 100644 >> --- a/drivers/gpu/drm/rcar-du/Kconfig >> +++ b/drivers/gpu/drm/rcar-du/Kconfig >> @@ -43,6 +43,7 @@ config DRM_RCAR_LVDS >> select DRM_PANEL >> select OF_FLATTREE >> select OF_OVERLAY >> + select RESET_CONTROLLER >> >> config DRM_RCAR_USE_MIPI_DSI >> bool "R-Car DU MIPI DSI Encoder Support" >> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c >> index 81a060c2fe3f..674b727cdaa2 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/reset.h> >> #include <linux/slab.h> >> #include <linux/sys_soc.h> >> >> @@ -60,6 +61,7 @@ struct rcar_lvds_device_info { >> struct rcar_lvds { >> struct device *dev; >> const struct rcar_lvds_device_info *info; >> + struct reset_control *rstc; >> >> struct drm_bridge bridge; >> >> @@ -316,6 +318,8 @@ int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq) >> >> dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq); >> >> + reset_control_deassert(lvds->rstc); > > Can this fail ? Same in __rcar_lvds_atomic_enable(). Yes. Too hasty in picking a patch from the BSP =). >> + >> ret = clk_prepare_enable(lvds->clocks.mod); > > It would be nice to switch this driver to runtime PM and move reset and > clock handling to the suspend/resume handlers. A candidate for a future > patch. I have the runtime PM patch in my work branch, but on top. I'll pick that into this series, before adding the reset control. Makes error handling a bit simpler. >> if (ret < 0) > > Missing reset_control_assert(). Same in other error paths if applicable, > here and in __rcar_lvds_atomic_enable(). Yep. Tomi
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig index a8f862c68b4f..151e400b996d 100644 --- a/drivers/gpu/drm/rcar-du/Kconfig +++ b/drivers/gpu/drm/rcar-du/Kconfig @@ -43,6 +43,7 @@ config DRM_RCAR_LVDS select DRM_PANEL select OF_FLATTREE select OF_OVERLAY + select RESET_CONTROLLER config DRM_RCAR_USE_MIPI_DSI bool "R-Car DU MIPI DSI Encoder Support" diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 81a060c2fe3f..674b727cdaa2 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/reset.h> #include <linux/slab.h> #include <linux/sys_soc.h> @@ -60,6 +61,7 @@ struct rcar_lvds_device_info { struct rcar_lvds { struct device *dev; const struct rcar_lvds_device_info *info; + struct reset_control *rstc; struct drm_bridge bridge; @@ -316,6 +318,8 @@ int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq) dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq); + reset_control_deassert(lvds->rstc); + ret = clk_prepare_enable(lvds->clocks.mod); if (ret < 0) return ret; @@ -338,6 +342,8 @@ void rcar_lvds_pclk_disable(struct drm_bridge *bridge) rcar_lvds_write(lvds, LVDPLLCR, 0); clk_disable_unprepare(lvds->clocks.mod); + + reset_control_assert(lvds->rstc); } EXPORT_SYMBOL_GPL(rcar_lvds_pclk_disable); @@ -396,6 +402,8 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge, u32 lvdcr0; int ret; + reset_control_deassert(lvds->rstc); + ret = clk_prepare_enable(lvds->clocks.mod); if (ret < 0) return; @@ -552,6 +560,7 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge, old_bridge_state); clk_disable_unprepare(lvds->clocks.mod); + reset_control_assert(lvds->rstc); } static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge, @@ -844,6 +853,12 @@ static int rcar_lvds_probe(struct platform_device *pdev) if (ret < 0) return ret; + lvds->rstc = devm_reset_control_get(&pdev->dev, NULL); + if (IS_ERR(lvds->rstc)) { + dev_err(&pdev->dev, "failed to get cpg reset\n"); + return PTR_ERR(lvds->rstc); + } + drm_bridge_add(&lvds->bridge); return 0;