Re: [PATCH] drm/rockchip: vop: Correct enabled clocks during setup

[Yakir Yang]

On 09/29/2015 05:55 PM, Yakir Yang wrote:
> On 09/29/2015 05:28 PM, Sjoerd Simons wrote:
> > When doing the initial setup both the hclk and the aclk need to be
> > enabled otherwise the board will simply hang. This only occurs when
> > building the vop driver as a module, when its built-in the initial setup

Hmm... My previous test was built-in the vop driver, and just notice that
you say problem only occurred when building the vop driver as module.
That's to say my test was wrong, so I try to do the right things.

But I found that vop driver module and rockchipdrm driver module in
dependency cycles, here are the build message:
    depmod: ERROR: Found 2 modules in dependency cycles!
    depmod: ERROR: Cycle detected: rockchip_drm_vop -> rockchipdrm -> 
rockchip_drm_vop
    Makefile:1054: recipe for target '_modinst_post' failed

And past my Makefile:
vop-y := rockchip_drm_drv.o rockchip_drm_fb.o rockchip_drm_fbdev.o 
rockchip_drm_gem.o
obj-m = rockchip_drm_vop.o vop.o

Very like to know how you handle this dependency cycles :)

Thanks,
- Yakir

> > happens to run before the clock framework shuts of unused clocks
> > (including the aclk).
> >
> > While there also switch to doing prepare and enable in one step rather
> > then separate steps to reduce the amount of code required.
> >
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@xxxxxxxxxxxxxxx>
>
> Looks good and test on chromeos-3.14 tree, no problem, so
>
> Tested-by: Yakir Yang <ykk@xxxxxxxxxxxxxx>
>
> > ---
> >
> >   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 36 
> > +++++++++++------------------
> >   1 file changed, 14 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
> > b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index 5d8ae5e..48719df 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -1575,32 +1575,25 @@ static int vop_initial(struct vop *vop)
> >           return PTR_ERR(vop->dclk);
> >       }
> >   -    ret = clk_prepare(vop->hclk);
> > -    if (ret < 0) {
> > -        dev_err(vop->dev, "failed to prepare hclk\n");
> > -        return ret;
> > -    }
> > -
> >       ret = clk_prepare(vop->dclk);
> >       if (ret < 0) {
> >           dev_err(vop->dev, "failed to prepare dclk\n");
> > -        goto err_unprepare_hclk;
> > +        return ret;
> >       }
> >   -    ret = clk_prepare(vop->aclk);
> > +    /* Enable both the hclk and aclk to setup the vop */
> > +    ret = clk_prepare_enable(vop->hclk);
> >       if (ret < 0) {
> > -        dev_err(vop->dev, "failed to prepare aclk\n");
> > +        dev_err(vop->dev, "failed to prepare/enable hclk\n");
> >           goto err_unprepare_dclk;
> >       }
> >   -    /*
> > -     * enable hclk, so that we can config vop register.
> > -     */
> > -    ret = clk_enable(vop->hclk);
> > +    ret = clk_prepare_enable(vop->aclk);
> >       if (ret < 0) {
> > -        dev_err(vop->dev, "failed to prepare aclk\n");
> > -        goto err_unprepare_aclk;
> > +        dev_err(vop->dev, "failed to prepare/enable aclk\n");
> > +        goto err_disable_hclk;
> >       }
> > +
> >       /*
> >        * do hclk_reset, reset all vop registers.
> >        */
> > @@ -1608,7 +1601,7 @@ static int vop_initial(struct vop *vop)
> >       if (IS_ERR(ahb_rst)) {
> >           dev_err(vop->dev, "failed to get ahb reset\n");
> >           ret = PTR_ERR(ahb_rst);
> > -        goto err_disable_hclk;
> > +        goto err_disable_aclk;
> >       }
> >       reset_control_assert(ahb_rst);
> >       usleep_range(10, 20);
> > @@ -1634,26 +1627,25 @@ static int vop_initial(struct vop *vop)
> >       if (IS_ERR(vop->dclk_rst)) {
> >           dev_err(vop->dev, "failed to get dclk reset\n");
> >           ret = PTR_ERR(vop->dclk_rst);
> > -        goto err_unprepare_aclk;
> > +        goto err_disable_aclk;
> >       }
> >       reset_control_assert(vop->dclk_rst);
> >       usleep_range(10, 20);
> >       reset_control_deassert(vop->dclk_rst);
> >         clk_disable(vop->hclk);
> > +    clk_disable(vop->aclk);
> >         vop->is_enabled = false;
> >         return 0;
> >   +err_disable_aclk:
> > +    clk_disable_unprepare(vop->aclk);
> >   err_disable_hclk:
> > -    clk_disable(vop->hclk);
> > -err_unprepare_aclk:
> > -    clk_unprepare(vop->aclk);
> > +    clk_disable_unprepare(vop->hclk);
> >   err_unprepare_dclk:
> >       clk_unprepare(vop->dclk);
> > -err_unprepare_hclk:
> > -    clk_unprepare(vop->hclk);
> >       return ret;
> >   }


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/