| Message ID | 20230825072324.2809260-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next] drm: xlnx: zynqmp_dpsub: Use devm_clk_get_enabled() helper function | expand |
Hi Jinjie, (CC'ing Tomi) Thank you for the patch. On Fri, Aug 25, 2023 at 03:23:24PM +0800, Jinjie Ruan wrote: > The devm_clk_get_enabled() helper: > - calls devm_clk_get() > - calls clk_prepare_enable() and registers what is needed in order to > call clk_disable_unprepare() when needed, as a managed resource. > > This simplifies the code. While this indeed simplifies the code, I think we should instead control the clock dynamically at runtime. I don't have access to the hardware at the moment. Tomi, would you be able to give this a go ? I can also write a patch and let you test it if desired. > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c > index 88eb33acd5f0..92e61434473f 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c > @@ -92,16 +92,10 @@ unsigned int zynqmp_dpsub_get_audio_clk_rate(struct zynqmp_dpsub *dpsub) > > static int zynqmp_dpsub_init_clocks(struct zynqmp_dpsub *dpsub) > { > - int ret; > - > - dpsub->apb_clk = devm_clk_get(dpsub->dev, "dp_apb_clk"); > - if (IS_ERR(dpsub->apb_clk)) > - return PTR_ERR(dpsub->apb_clk); > - > - ret = clk_prepare_enable(dpsub->apb_clk); > - if (ret) { > + dpsub->apb_clk = devm_clk_get_enabled(dpsub->dev, "dp_apb_clk"); > + if (IS_ERR(dpsub->apb_clk)) { > dev_err(dpsub->dev, "failed to enable the APB clock\n"); > - return ret; > + return PTR_ERR(dpsub->apb_clk); > } > > /* > @@ -274,7 +268,6 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev) > zynqmp_dp_remove(dpsub); > err_pm: > pm_runtime_disable(&pdev->dev); > - clk_disable_unprepare(dpsub->apb_clk); > err_mem: > of_reserved_mem_device_release(&pdev->dev); > if (!dpsub->drm) > @@ -295,7 +288,6 @@ static void zynqmp_dpsub_remove(struct platform_device *pdev) > zynqmp_dp_remove(dpsub); > > pm_runtime_disable(&pdev->dev); > - clk_disable_unprepare(dpsub->apb_clk); > of_reserved_mem_device_release(&pdev->dev); > > if (!dpsub->drm)
On 29/08/2023 10:55, Laurent Pinchart wrote: > Hi Jinjie, > > (CC'ing Tomi) > > Thank you for the patch. > > On Fri, Aug 25, 2023 at 03:23:24PM +0800, Jinjie Ruan wrote: >> The devm_clk_get_enabled() helper: >> - calls devm_clk_get() >> - calls clk_prepare_enable() and registers what is needed in order to >> call clk_disable_unprepare() when needed, as a managed resource. >> >> This simplifies the code. > > While this indeed simplifies the code, I think we should instead control > the clock dynamically at runtime. > > I don't have access to the hardware at the moment. Tomi, would you be > able to give this a go ? I can also write a patch and let you test it if > desired. I have a small patch that adds runtime resume & suspend callbacks, and enables & disables the clock there. But the driver doesn't seem to do a proper job of power management: - It accesses registers without pm_runtime_get - It initializes registers at probe time, apparently presuming that the IP is never turned off, which might cause registers getting reset. - It uses pm_runtime_get in a couple of places where it's starting the video stream, but it seems to forget that there's also the DP AUX and HPD. Then again, as the HPD is, I think, supposed to be always enabled, the device is also always enabled, making the PM management a bit pointless. I'll look at this a bit more to see if I can sort this out. Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c index 88eb33acd5f0..92e61434473f 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c @@ -92,16 +92,10 @@ unsigned int zynqmp_dpsub_get_audio_clk_rate(struct zynqmp_dpsub *dpsub) static int zynqmp_dpsub_init_clocks(struct zynqmp_dpsub *dpsub) { - int ret; - - dpsub->apb_clk = devm_clk_get(dpsub->dev, "dp_apb_clk"); - if (IS_ERR(dpsub->apb_clk)) - return PTR_ERR(dpsub->apb_clk); - - ret = clk_prepare_enable(dpsub->apb_clk); - if (ret) { + dpsub->apb_clk = devm_clk_get_enabled(dpsub->dev, "dp_apb_clk"); + if (IS_ERR(dpsub->apb_clk)) { dev_err(dpsub->dev, "failed to enable the APB clock\n"); - return ret; + return PTR_ERR(dpsub->apb_clk); } /* @@ -274,7 +268,6 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev) zynqmp_dp_remove(dpsub); err_pm: pm_runtime_disable(&pdev->dev); - clk_disable_unprepare(dpsub->apb_clk); err_mem: of_reserved_mem_device_release(&pdev->dev); if (!dpsub->drm) @@ -295,7 +288,6 @@ static void zynqmp_dpsub_remove(struct platform_device *pdev) zynqmp_dp_remove(dpsub); pm_runtime_disable(&pdev->dev); - clk_disable_unprepare(dpsub->apb_clk); of_reserved_mem_device_release(&pdev->dev); if (!dpsub->drm)
The devm_clk_get_enabled() helper: - calls devm_clk_get() - calls clk_prepare_enable() and registers what is needed in order to call clk_disable_unprepare() when needed, as a managed resource. This simplifies the code. Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)