From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vikas Sajjan Subject: Re: [PATCH v3] drm/exynos: enable FIMD clocks Date: Mon, 22 Apr 2013 11:42:25 +0530 Message-ID: References: <1364805830-6129-1-git-send-email-vikas.sajjan@linaro.org> <07b501ce3f1d$359f5900$a0de0b00$%dae@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0945669283==" Return-path: In-Reply-To: <07b501ce3f1d$359f5900$a0de0b00$%dae@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Inki Dae Cc: "kgene.kim" , Viresh Kumar , DRI mailing list , linux-samsung-soc@vger.kernel.org, linaro-kernel@lists.linaro.org, linux-media@vger.kernel.org List-Id: linux-samsung-soc@vger.kernel.org --===============0945669283== Content-Type: multipart/alternative; boundary=001a11c34dce20f14b04daecf5aa --001a11c34dce20f14b04daecf5aa Content-Type: text/plain; charset=ISO-8859-1 Hi Mr Dae, On 22 April 2013 11:19, Inki Dae wrote: > Hi, Mr. Vikas > > Please fix the below typos Viresh pointed out and my comments. > > > -----Original Message----- > > From: Viresh Kumar [mailto:viresh.kumar@linaro.org] > > Sent: Monday, April 01, 2013 5:51 PM > > To: Vikas Sajjan > > Cc: dri-devel@lists.freedesktop.org; linux-samsung-soc@vger.kernel.org; > > jy0922.shim@samsung.com; inki.dae@samsung.com; kgene.kim@samsung.com; > > linaro-kernel@lists.linaro.org; linux-media@vger.kernel.org > > Subject: Re: [PATCH v3] drm/exynos: enable FIMD clocks > > > > On 1 April 2013 14:13, Vikas Sajjan wrote: > > > While migrating to common clock framework (CCF), found that the FIMD > > clocks > > > > s/found/we found/ > > > > > were pulled down by the CCF. > > > If CCF finds any clock(s) which has NOT been claimed by any of the > > > drivers, then such clock(s) are PULLed low by CCF. > > > > > > By calling clk_prepare_enable() for FIMD clocks fixes the issue. > > > > s/By calling/Calling/ > > > > and > > > > s/the/this > > > > > this patch also replaces clk_disable() with clk_disable_unprepare() > > > > s/this/This > > > > > during exit. > > > > Sorry but your log doesn't say what you are doing. You are just adding > > relevant calls to clk_prepare/unprepare() before calling > > clk_enable/disable. > > > I shall modify the commit message as below as suggested by tomaz figa, " Common Clock Framework introduced the need to prepare clocks before enabling them, otherwise clk_enable() fails. This patch adds clk_prepare calls to the driver. This patch also removes clk_disable() from fimd_remove() as it will be done by pm_runtime_put_sync". > > > Signed-off-by: Vikas Sajjan > > > --- > > > Changes since v2: > > > - moved clk_prepare_enable() and clk_disable_unprepare() from > > > fimd_probe() to fimd_clock() as suggested by Inki Dae > > > > > Changes since v1: > > > - added error checking for clk_prepare_enable() and also > replaced > > > clk_disable() with clk_disable_unprepare() during exit. > > > --- > > > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > > > index 9537761..f2400c8 100644 > > > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > > > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > > > @@ -799,18 +799,18 @@ static int fimd_clock(struct fimd_context *ctx, > > bool enable) > > > if (enable) { > > > int ret; > > > > > > - ret = clk_enable(ctx->bus_clk); > > > + ret = clk_prepare_enable(ctx->bus_clk); > > > if (ret < 0) > > > return ret; > > > > > > - ret = clk_enable(ctx->lcd_clk); > > > + ret = clk_prepare_enable(ctx->lcd_clk); > > > if (ret < 0) { > > > - clk_disable(ctx->bus_clk); > > > + clk_disable_unprepare(ctx->bus_clk); > > > return ret; > > > } > > > } else { > > > - clk_disable(ctx->lcd_clk); > > > - clk_disable(ctx->bus_clk); > > > + clk_disable_unprepare(ctx->lcd_clk); > > > + clk_disable_unprepare(ctx->bus_clk); > > > } > > > > > > return 0; > > > @@ -981,8 +981,8 @@ static int fimd_remove(struct platform_device > *pdev) > > > if (ctx->suspended) > > > goto out; > > > > > > - clk_disable(ctx->lcd_clk); > > > - clk_disable(ctx->bus_clk); > > > + clk_disable_unprepare(ctx->lcd_clk); > > > + clk_disable_unprepare(ctx->bus_clk); > > Just remove the above codes. It seems that clk_disable(also > clk_disable_unprepare) isn't needed because it will be done by > pm_runtime_put_sync and please re-post it(probably patch v5??) > > So you mean I should work on v3 version, and keep the clk_prepare_enable() code as is in fimd_clock() and just remove the clk_disable_unprepare() and also clk_disable() from fimd_remove(), right? Viresh and Tomaz Figa, let me know if these changes are fine with you guys. Thanks, > Inki Dae > > > > > You are doing things at the right place but i have a suggestion. Are you > > doing > > anything in your clk_prepare() atleast for this device? Probably not. > > > > If not, then its better to call clk_prepare/unprepare only once at > > probe/remove > > and keep clk_enable/disable calls as is. > > > > -- > > viresh > > -- Thanks and Regards Vikas Sajjan --001a11c34dce20f14b04daecf5aa Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

Hi Mr Dae,

On 22 April 2013 11:19, = Inki Dae <inki.dae@samsung.com> wrote:
Hi, Mr. Vikas

Please fix the below typos Viresh pointed out and my comments.

> -----Original Message-----
> From: Viresh Kumar [mailto:= viresh.kumar@linaro.org]
> Sent: Monday, April 01, 2013 5:51 PM
> To: Vikas Sajjan
> Cc: dri-devel@lists= .freedesktop.org; = linux-samsung-soc@vger.kernel.org;
> jy0922.shim@samsung.com= ; inki.dae@samsung.com; kgene.kim@samsung.com;
> linaro-kernel@lists.= linaro.org; linux-media@= vger.kernel.org
> Subject: Re: [PATCH v3] drm/exynos: enable FIMD clocks
>
> On 1 April 2013 14:13, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> > While migrating to common clock framework (CCF), found that the F= IMD
> clocks
>
> s/found/we found/
>
> > were pulled down by the CCF.
> > If CCF finds any clock(s) which has NOT been claimed by any of th= e
> > drivers, then such clock(s) are PULLed low by CCF.
> >
> > By calling clk_prepare_enable() for FIMD clocks fixes the issue.<= br> >
> s/By calling/Calling/
>
> and
>
> s/the/this
>
> > this patch also replaces clk_disable() with clk_disable_unprepare= ()
>
> s/this/This
>
> > during exit.
>
> Sorry but your log doesn't say what you are doing. You are just ad= ding
> relevant calls to clk_prepare/unprepare() before calling
> clk_enable/disable.
>

=A0 I shall modify the= commit message as below as suggested by tomaz figa,

=A0 " Comm= on Clock Framework introduced the need to prepare clocks before
enabling them, otherwise clk_enable() fails. This patch adds clk_prepare calls to the driver. This patch also removes clk_disable() from fimd_remove= () as it will be done by pm_runtime_put_sync".
=A0
> > Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> > ---
> > Changes since v2:
> > =A0 =A0 =A0 =A0 - moved clk_prepare_enable() and clk_disable_unpr= epare() from
> > =A0 =A0 =A0 =A0 fimd_probe() to fimd_clock() as suggested by Inki= Dae
> <inki.dae@samsung.com&g= t;
> > Changes since v1:
> > =A0 =A0 =A0 =A0 - added error checking for clk_prepare_enable() a= nd also
replaced
> > =A0 =A0 =A0 =A0 clk_disable() with clk_disable_unprepare() during= exit.
> > ---
> > =A0drivers/gpu/drm/exynos/exynos_drm_fimd.c | =A0 14 +++++++-----= --
> > =A01 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > index 9537761..f2400c8 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > @@ -799,18 +799,18 @@ static int fimd_clock(struct fimd_context *= ctx,
> bool enable)
> > =A0 =A0 =A0 =A0 if (enable) {
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int ret;
> >
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D clk_enable(ctx->bus_clk)= ;
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D clk_prepare_enable(ctx->= bus_clk);
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret < 0)
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ret;
> >
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D clk_enable(ctx->lcd_clk)= ;
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D clk_prepare_enable(ctx->= lcd_clk);
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if =A0(ret < 0) {
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk_disable(ctx->= ;bus_clk);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk_disable_unprepa= re(ctx->bus_clk);
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ret;
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> > =A0 =A0 =A0 =A0 } else {
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk_disable(ctx->lcd_clk);
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk_disable(ctx->bus_clk);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk_disable_unprepare(ctx->lcd_c= lk);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk_disable_unprepare(ctx->bus_c= lk);
> > =A0 =A0 =A0 =A0 }
> >
> > =A0 =A0 =A0 =A0 return 0;
> > @@ -981,8 +981,8 @@ static int fimd_remove(struct platform_device= *pdev)
> > =A0 =A0 =A0 =A0 if (ctx->suspended)
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
> >
> > - =A0 =A0 =A0 clk_disable(ctx->lcd_clk);
> > - =A0 =A0 =A0 clk_disable(ctx->bus_clk);
> > + =A0 =A0 =A0 clk_disable_unprepare(ctx->lcd_clk);
> > + =A0 =A0 =A0 clk_disable_unprepare(ctx->bus_clk);

Just remove the above codes. It seems that clk_disable(also
clk_disable_unprepare) isn't needed because it will be done by
pm_runtime_put_sync and please re-post it(probably patch v5??)

=A0=A0 So you mean I should work on v3 version, and k= eep=A0 the clk_prepare_enable() code as is in fimd_clock() and just remove = the clk_disable_unprepare() and also clk_disable() from fimd_remove(), righ= t?

Viresh and Tomaz Figa, let me know if these changes are fine= with you guys.

Thanks,
Inki Dae

>
> You are doing things at the right place but i have a suggestion. Are y= ou
> doing
> anything in your clk_prepare() atleast for this device? Probably not.<= br> >
> If not, then its better to call clk_prepare/unprepare only once at
> probe/remove
> and keep clk_enable/disable calls as is.
>
> --
> viresh




--
Thanks and = Regards
=A0Vikas Sajjan
--001a11c34dce20f14b04daecf5aa-- --===============0945669283== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============0945669283==--