Re: [PATCH 09/10] drm/mxsfb: Improve the axi clock usage
On Mi, 2019-01-09 at 18:09 +0100, Stefan Agner wrote:
> On 09.01.2019 15:13, Robert Chiras wrote:
> >
> > Currently, the enable of the axi clock return status is ignored,
> > causing
> > issues when the enable fails then we try to disable it. Therefore,
> > it is
> > better to check the return status and disable it only when enable
> > succeeded.
> > Also, remove the helper functions around clk_axi, since we can
> > directly
> > use the clk API function for enable/disable the clock. Those
> > functions
> > are already checking for NULL clk and returning 0 if that's the
> > case.
> Can we maybe even use the runtime PM system for that (adding two
> callbacks for SET_RUNTIME_PM_OPS)?
>
> I suggested it a while ago, but did not looked deeper into it:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flk
> ml.org%2Flkml%2F2018%2F8%2F1%2F300&data=02%7C01%7Crobert.chiras%4
> 0nxp.com%7Cfb15abdd151b4c643f4f08d67655464a%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C636826505961622511&sdata=MxpNiBXFW6gw9U8rKRF2
> 0ji73tZ5X5GqZWcninDBFWk%3D&reserved=0
>
> Since we basically enable on mxsfb_crtc_enable and disable on
> mxsfb_crtc_disable, I think it would be pretty much the same thing.
>
> --
> Stefan
>
Hi Stefan,
I don't think I fully understand your suggestion here. The axi clock is
used when we are trying to access LCDIF registers while the block is
not running. For example, when the vblank needs to be enabled/disabled
while the block is idle (so it doesn't have anything to do with
mxsfb_crtc_enable/disable). How could the PM callbacks handle this
case?
Thanks,
Robert
>
> >
> >
> > Signed-off-by: Robert Chiras
> > Acked-by: Leonard Crestez
> > ---
> > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 8
> > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 32 +-
> > --
> > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 3 ---
> > 3 files changed, 17 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> > index 8d1b6a6..b9437c7 100644
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> > @@ -411,7 +411,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private
> > *mxsfb)
> > {
> > dma_addr_t paddr;
> >
> > - mxsfb_enable_axi_clk(mxsfb);
> > + clk_prepare_enable(mxsfb->clk_axi);
> > writel(0, mxsfb->base + LCDC_CTRL);
> > mxsfb_crtc_mode_set_nofb(mxsfb);
> >
> > @@ -428,7 +428,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private
> > *mxsfb)
> > void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
> > {
> > mxsfb_disable_controller(mxsfb);
> > - mxsfb_disable_axi_clk(mxsfb);
> > + clk_disable_unprepare(mxsfb->clk_axi);
> > }
> >
> > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
> > @@ -456,9 +456,9 @@ void mxsfb_plane_atomic_update(struct
> > mxsfb_drm_private *mxsfb,
> >
> > paddr = mxsfb_get_fb_paddr(mxsfb);
> > if (paddr) {
> > - mxsfb_enable_axi_clk(mxsfb);
> > + clk_prepare_enable(mxsfb->clk_axi);
> > writel(paddr, mxsfb->base + mxsfb->devdata-
> > >next_buf);
> > - mxsfb_disable_axi_clk(mxsfb);
> > + clk_disable_unprepare(mxsfb->clk_axi);
> > }
> >
> > if (!fb || !old_fb)
> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > index 135b8e1..5e18353 100644
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > @@ -103,18 +103,6 @@ drm_pipe_to_mxsfb_drm_private(struct
> > drm_simple_display_pipe *pipe)
> > return container_of(pipe, struct mxsfb_drm_private, pipe);
> > }
> >
> > -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb)
> > -{
> > - if (mxsfb->clk_axi)
> > - clk_prepare_enable(mxsfb->clk_axi);
> > -}
> > -
> > -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb)
> > -{
> > - if (mxsfb->clk_axi)
> > - clk_disable_unprepare(mxsfb->clk_axi);
> > -}
> > -
> > /**
> > * mxsfb_atomic_helper_check - validate state object
> > * @dev: DRM device
> > @@ -237,25 +225,31 @@ static void mxsfb_pipe_update(struct
> > drm_simple_display_pipe *pipe,
> > static int mxsfb_pipe_enable_vblank(struct drm_simple_display_pipe
> > *pipe)
> > {
> > struct mxsfb_drm_private *mxsfb =
> > drm_pipe_to_mxsfb_drm_private(pipe);
> > + int ret = 0;
> > +
> > + ret = clk_prepare_enable(mxsfb->clk_axi);
> > + if (ret)
> > + return ret;
> >
> > /* Clear and enable VBLANK IRQ */
> > - mxsfb_enable_axi_clk(mxsfb);
> > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1
> > + REG_CLR);
> > writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base +
> > LCDC_CTRL1 + REG_SET);
> > - mxsfb_disable_axi_clk(mxsfb);
> > + clk_disable_unprepare(mxsfb->clk_axi);
> >
> > - return 0;
> > + return ret;
> > }
> >
> > static void mxsfb_pipe_disable_vblank(struct
> > drm_simple_display_pipe *pipe)
> > {
> >
Re: [PATCH 09/10] drm/mxsfb: Improve the axi clock usage
On 09.01.2019 15:13, Robert Chiras wrote:
> Currently, the enable of the axi clock return status is ignored, causing
> issues when the enable fails then we try to disable it. Therefore, it is
> better to check the return status and disable it only when enable
> succeeded.
> Also, remove the helper functions around clk_axi, since we can directly
> use the clk API function for enable/disable the clock. Those functions
> are already checking for NULL clk and returning 0 if that's the case.
Can we maybe even use the runtime PM system for that (adding two
callbacks for SET_RUNTIME_PM_OPS)?
I suggested it a while ago, but did not looked deeper into it:
https://lkml.org/lkml/2018/8/1/300
Since we basically enable on mxsfb_crtc_enable and disable on
mxsfb_crtc_disable, I think it would be pretty much the same thing.
--
Stefan
>
> Signed-off-by: Robert Chiras
> Acked-by: Leonard Crestez
> ---
> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 8
> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 32 +---
> drivers/gpu/drm/mxsfb/mxsfb_drv.h | 3 ---
> 3 files changed, 17 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 8d1b6a6..b9437c7 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> @@ -411,7 +411,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
> {
> dma_addr_t paddr;
>
> - mxsfb_enable_axi_clk(mxsfb);
> + clk_prepare_enable(mxsfb->clk_axi);
> writel(0, mxsfb->base + LCDC_CTRL);
> mxsfb_crtc_mode_set_nofb(mxsfb);
>
> @@ -428,7 +428,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
> void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
> {
> mxsfb_disable_controller(mxsfb);
> - mxsfb_disable_axi_clk(mxsfb);
> + clk_disable_unprepare(mxsfb->clk_axi);
> }
>
> void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
> @@ -456,9 +456,9 @@ void mxsfb_plane_atomic_update(struct
> mxsfb_drm_private *mxsfb,
>
> paddr = mxsfb_get_fb_paddr(mxsfb);
> if (paddr) {
> - mxsfb_enable_axi_clk(mxsfb);
> + clk_prepare_enable(mxsfb->clk_axi);
> writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
> - mxsfb_disable_axi_clk(mxsfb);
> + clk_disable_unprepare(mxsfb->clk_axi);
> }
>
> if (!fb || !old_fb)
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index 135b8e1..5e18353 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -103,18 +103,6 @@ drm_pipe_to_mxsfb_drm_private(struct
> drm_simple_display_pipe *pipe)
> return container_of(pipe, struct mxsfb_drm_private, pipe);
> }
>
> -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb)
> -{
> - if (mxsfb->clk_axi)
> - clk_prepare_enable(mxsfb->clk_axi);
> -}
> -
> -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb)
> -{
> - if (mxsfb->clk_axi)
> - clk_disable_unprepare(mxsfb->clk_axi);
> -}
> -
> /**
> * mxsfb_atomic_helper_check - validate state object
> * @dev: DRM device
> @@ -237,25 +225,31 @@ static void mxsfb_pipe_update(struct
> drm_simple_display_pipe *pipe,
> static int mxsfb_pipe_enable_vblank(struct drm_simple_display_pipe *pipe)
> {
> struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe);
> + int ret = 0;
> +
> + ret = clk_prepare_enable(mxsfb->clk_axi);
> + if (ret)
> + return ret;
>
> /* Clear and enable VBLANK IRQ */
> - mxsfb_enable_axi_clk(mxsfb);
> writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
> writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_SET);
> - mxsfb_disable_axi_clk(mxsfb);
> + clk_disable_unprepare(mxsfb->clk_axi);
>
> - return 0;
> + return ret;
> }
>
> static void mxsfb_pipe_disable_vblank(struct drm_simple_display_pipe *pipe)
> {
> struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe);
>
> + if (clk_prepare_enable(mxsfb->clk_axi))
> + return;
> +
> /* Disable and clear VBLANK IRQ */
> - mxsfb_enable_axi_clk(mxsfb);
> writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR);
> writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
> - mxsfb_disable_axi_clk(mxsfb);
> + clk_disable_unprepare(mxsfb->clk_axi);
> }
>
> static struct drm_simple_display_pipe_funcs mxsfb_funcs = {
> @@ -440,7 +434,7 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data)
> struct mxsfb_drm_private *mxsfb = drm->dev_private;
> u32 reg;
>
> - mxsfb_enable_axi_clk(mxsfb);
> + clk_prepare_enable(mxsfb->clk_axi);
>
> reg = readl(mxsfb->base + LCDC_CTRL1);
>
> @@ -449,7 +443,7 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data)
>
> writ
