| Message ID | 019301cc9a08$17c69d50$4753d7f0$%kim@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 03, 2011 at 06:08:03PM +0900, Kukjin Kim wrote: > diff --git a/arch/arm/plat-samsung/pwm-clock.c b/arch/arm/plat-samsung/pwm-clock.c > index a35ff3b..37a159b 100644 > --- a/arch/arm/plat-samsung/pwm-clock.c > +++ b/arch/arm/plat-samsung/pwm-clock.c > @@ -449,6 +449,8 @@ __init void s3c_pwmclk_init(void) > return; > } > > + clk_enable(clk_timers); Error checking? clk_prepare() as well?
On 11/3/11, Kukjin Kim <kgene.kim@samsung.com> wrote: > Kyungmin Park wrote: >> >> On 11/3/11, Kyungmin Park <kyungmin.park@samsung.com> wrote: >> > On 11/3/11, Joonyoung Shim <jy0922.shim@samsung.com> wrote: >> >> 11/03/2011 04:46 PM, Kukjin Kim 쓴 글: >> >>> Joonyoung Shim wrote: >> >>>> 11/03/2011 10:59 AM, Kukjin Kim 쓴 글: >> >>>>> Joonyoung Shim wrote: >> >>>>>> PWM timers use pclk("timers" clk) as parent clk. If this pclk is >> >>>>>> the >> >>>>>> disabled state when PWM driver is probed, then it causes wrong read >> >>>>>> and >> >>>>>> write operation about registers of PWM. >> >>>>>> >> >>>>>> Signed-off-by: Joonyoung Shim<jy0922.shim@samsung.com> >> >>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> >> >>>>>> --- >> >>>>>> arch/arm/plat-samsung/pwm.c | 7 +++++++ >> >>>>>> 1 files changed, 7 insertions(+), 0 deletions(-) >> >>>>>> >> >>>>>> diff --git a/arch/arm/plat-samsung/pwm.c >> >>>>>> b/arch/arm/plat-samsung/pwm.c >> >>>>>> index f37457c..dc1185d 100644 >> >>>>>> --- a/arch/arm/plat-samsung/pwm.c >> >>>>>> +++ b/arch/arm/plat-samsung/pwm.c >> >>>>>> @@ -299,6 +299,9 @@ static int s3c_pwm_probe(struct platform_device >> >>>>>> *pdev) >> >>>>>> goto err_clk_tin; >> >>>>>> } >> >>>>>> >> >>>>>> + clk_enable(pwm->clk); >> >>>>>> + clk_enable(pwm->clk_div); >> >>>>>> + >> >>>>>> local_irq_save(flags); >> >>>>>> >> >>>>>> tcon = __raw_readl(S3C2410_TCON); >> >>>>>> @@ -326,6 +329,8 @@ static int s3c_pwm_probe(struct platform_device >> >>>>>> *pdev) >> >>>>>> return 0; >> >>>>>> >> >>>>>> err_clk_tdiv: >> >>>>>> + clk_disable(pwm->clk_div); >> >>>>>> + clk_disable(pwm->clk); >> >>>>>> clk_put(pwm->clk_div); >> >>>>>> >> >>>>>> err_clk_tin: >> >>>>>> @@ -340,6 +345,8 @@ static int __devexit s3c_pwm_remove(struct >> >>>>>> platform_device *pdev) >> >>>>>> { >> >>>>>> struct pwm_device *pwm = platform_get_drvdata(pdev); >> >>>>>> >> >>>>>> + clk_disable(pwm->clk_div); >> >>>>>> + clk_disable(pwm->clk); >> >>>>>> clk_put(pwm->clk_div); >> >>>>>> clk_put(pwm->clk); >> >>>>>> kfree(pwm); >> >>>>>> -- >> >>>>>> 1.7.5.4 >> >>>>> Well, I wonder when this is needed. I think it should be enabled >> >>>>> during >> >>>>> kernel booting... >> >>>> The exynos4 machine using just timer turns on "timer" clock in the >> >>>> past, >> >>>> but "timer" clock is disable state when boot since MCT is used. MCT >> >>>> doesn't control "timer" clock. >> >>>> >> >>>> I think pwm driver should control(enable/disable) using clocks >> >>>> regardless of their parents clock. >> >>>> >> >>> I mean, why that is disabled when kernel boot... >> >>> >> >> >> >> Please see arch/arm/mach-exynos4/clock.c >> >> There is "timers' clock in the clk init_clocks_off[]. >> One more, Don't assume some clocks are enabled at bootloader. user can >> use any bootloader and any configuration. >> > Kyungmin, > NO! I didn't. Just I thought your bootloader disables the clock and why it > is needed. As you don't unaware the power consumption. you don't care the clock gating. You think this clock disable is meaningless but the basic idea of PM is disable the clock and power domain and use it only enable during use See your team product kernel codes. why they are register the unused clock at there and implement the clock gating and runtime pm not at mainline. > > Anyway, Joonyoung, > How about following? And it seems enough... > -- > diff --git a/arch/arm/plat-samsung/pwm-clock.c > b/arch/arm/plat-samsung/pwm-clock.c > index a35ff3b..37a159b 100644 > --- a/arch/arm/plat-samsung/pwm-clock.c > +++ b/arch/arm/plat-samsung/pwm-clock.c > @@ -449,6 +449,8 @@ __init void s3c_pwmclk_init(void) > return; > } > > + clk_enable(clk_timers); > + > for (clk = 0; clk < ARRAY_SIZE(clk_timer_scaler); clk++) > clk_timer_scaler[clk].parent = clk_timers; > > -- > Or needs separate clk_enable like following? > > diff --git a/arch/arm/plat-samsung/pwm.c b/arch/arm/plat-samsung/pwm.c > index f37457c..7a77e36 100644 > --- a/arch/arm/plat-samsung/pwm.c > +++ b/arch/arm/plat-samsung/pwm.c > @@ -292,6 +292,8 @@ static int s3c_pwm_probe(struct platform_device *pdev) > goto err_alloc; > } > > + clk_enable(pwm->clk); > + > pwm->clk_div = clk_get(dev, "pwm-tdiv"); > if (IS_ERR(pwm->clk_div)) { > dev_err(dev, "failed to get pwm tdiv clk\n"); > @@ -299,6 +301,8 @@ static int s3c_pwm_probe(struct platform_device *pdev) > goto err_clk_tin; > } > > + clk_enable(pwm->clk_div); > + > local_irq_save(flags); > > tcon = __raw_readl(S3C2410_TCON); > @@ -326,9 +330,11 @@ static int s3c_pwm_probe(struct platform_device *pdev) > return 0; > > err_clk_tdiv: > + clk_disable(pwm->clk_div); > clk_put(pwm->clk_div); > > err_clk_tin: > + clk_disable(pwm->clk); > clk_put(pwm->clk); > > err_alloc: > -- > > BTW, I think this is not really needed for this merge window because the > pwm.c is used for only s3c24xx and as you know, it doesn't matter because of > arch/arm/plat-samsung/time.c. And if required, this can be fixed during -rc. Did you see the dev-pwm.c? and dev-backlight.c? > > Thanks. > > Best regards, > Kgene. > -- > Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, > SW Solution Development Team, Samsung Electronics Co., Ltd. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
11/03/2011 06:08 PM, Kukjin Kim 쓴 글: > Kyungmin Park wrote: >> On 11/3/11, Kyungmin Park<kyungmin.park@samsung.com> wrote: >>> On 11/3/11, Joonyoung Shim<jy0922.shim@samsung.com> wrote: >>>> 11/03/2011 04:46 PM, Kukjin Kim 쓴 글: >>>>> Joonyoung Shim wrote: >>>>>> 11/03/2011 10:59 AM, Kukjin Kim 쓴 글: >>>>>>> Joonyoung Shim wrote: >>>>>>>> PWM timers use pclk("timers" clk) as parent clk. If this pclk is the >>>>>>>> disabled state when PWM driver is probed, then it causes wrong read >>>>>>>> and >>>>>>>> write operation about registers of PWM. >>>>>>>> >>>>>>>> Signed-off-by: Joonyoung Shim<jy0922.shim@samsung.com> >>>>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> >>>>>>>> --- >>>>>>>> arch/arm/plat-samsung/pwm.c | 7 +++++++ >>>>>>>> 1 files changed, 7 insertions(+), 0 deletions(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm/plat-samsung/pwm.c >>>>>>>> b/arch/arm/plat-samsung/pwm.c >>>>>>>> index f37457c..dc1185d 100644 >>>>>>>> --- a/arch/arm/plat-samsung/pwm.c >>>>>>>> +++ b/arch/arm/plat-samsung/pwm.c >>>>>>>> @@ -299,6 +299,9 @@ static int s3c_pwm_probe(struct platform_device >>>>>>>> *pdev) >>>>>>>> goto err_clk_tin; >>>>>>>> } >>>>>>>> >>>>>>>> + clk_enable(pwm->clk); >>>>>>>> + clk_enable(pwm->clk_div); >>>>>>>> + >>>>>>>> local_irq_save(flags); >>>>>>>> >>>>>>>> tcon = __raw_readl(S3C2410_TCON); >>>>>>>> @@ -326,6 +329,8 @@ static int s3c_pwm_probe(struct platform_device >>>>>>>> *pdev) >>>>>>>> return 0; >>>>>>>> >>>>>>>> err_clk_tdiv: >>>>>>>> + clk_disable(pwm->clk_div); >>>>>>>> + clk_disable(pwm->clk); >>>>>>>> clk_put(pwm->clk_div); >>>>>>>> >>>>>>>> err_clk_tin: >>>>>>>> @@ -340,6 +345,8 @@ static int __devexit s3c_pwm_remove(struct >>>>>>>> platform_device *pdev) >>>>>>>> { >>>>>>>> struct pwm_device *pwm = platform_get_drvdata(pdev); >>>>>>>> >>>>>>>> + clk_disable(pwm->clk_div); >>>>>>>> + clk_disable(pwm->clk); >>>>>>>> clk_put(pwm->clk_div); >>>>>>>> clk_put(pwm->clk); >>>>>>>> kfree(pwm); >>>>>>>> -- >>>>>>>> 1.7.5.4 >>>>>>> Well, I wonder when this is needed. I think it should be enabled >>>>>>> during >>>>>>> kernel booting... >>>>>> The exynos4 machine using just timer turns on "timer" clock in the >>>>>> past, >>>>>> but "timer" clock is disable state when boot since MCT is used. MCT >>>>>> doesn't control "timer" clock. >>>>>> >>>>>> I think pwm driver should control(enable/disable) using clocks >>>>>> regardless of their parents clock. >>>>>> >>>>> I mean, why that is disabled when kernel boot... >>>>> >>>> Please see arch/arm/mach-exynos4/clock.c >>>> There is "timers' clock in the clk init_clocks_off[]. >> One more, Don't assume some clocks are enabled at bootloader. user can >> use any bootloader and any configuration. >> > Kyungmin, > NO! I didn't. Just I thought your bootloader disables the clock and why it is needed. > > Anyway, Joonyoung, > How about following? And it seems enough... > -- > diff --git a/arch/arm/plat-samsung/pwm-clock.c b/arch/arm/plat-samsung/pwm-clock.c > index a35ff3b..37a159b 100644 > --- a/arch/arm/plat-samsung/pwm-clock.c > +++ b/arch/arm/plat-samsung/pwm-clock.c > @@ -449,6 +449,8 @@ __init void s3c_pwmclk_init(void) > return; > } > > + clk_enable(clk_timers); > + > for (clk = 0; clk< ARRAY_SIZE(clk_timer_scaler); clk++) > clk_timer_scaler[clk].parent = clk_timers; > No, there is no the reason "timers" clock turns on always. > -- > Or needs separate clk_enable like following? > > diff --git a/arch/arm/plat-samsung/pwm.c b/arch/arm/plat-samsung/pwm.c > index f37457c..7a77e36 100644 > --- a/arch/arm/plat-samsung/pwm.c > +++ b/arch/arm/plat-samsung/pwm.c > @@ -292,6 +292,8 @@ static int s3c_pwm_probe(struct platform_device *pdev) > goto err_alloc; > } > > + clk_enable(pwm->clk); > + > pwm->clk_div = clk_get(dev, "pwm-tdiv"); > if (IS_ERR(pwm->clk_div)) { > dev_err(dev, "failed to get pwm tdiv clk\n"); > @@ -299,6 +301,8 @@ static int s3c_pwm_probe(struct platform_device *pdev) > goto err_clk_tin; > } > > + clk_enable(pwm->clk_div); > + > local_irq_save(flags); > > tcon = __raw_readl(S3C2410_TCON); > @@ -326,9 +330,11 @@ static int s3c_pwm_probe(struct platform_device *pdev) > return 0; > > err_clk_tdiv: > + clk_disable(pwm->clk_div); > clk_put(pwm->clk_div); > > err_clk_tin: > + clk_disable(pwm->clk); > clk_put(pwm->clk); > > err_alloc: > -- I don't care about this. > > BTW, I think this is not really needed for this merge window because the pwm.c is used for only s3c24xx and as you know, it doesn't matter because of arch/arm/plat-samsung/time.c. And if required, this can be fixed during -rc. It's the problem to assume that the related clocks turned on by other and the pwm is used for exynos4 also. > Thanks. > > Best regards, > Kgene. > -- > Kukjin Kim<kgene.kim@samsung.com>, Senior Engineer, > SW Solution Development Team, Samsung Electronics Co., Ltd. > >
Joonyoung Shim wrote: > > 11/03/2011 06:08 PM, Kukjin Kim 쓴 글: > > Kyungmin Park wrote: > >> On 11/3/11, Kyungmin Park<kyungmin.park@samsung.com> wrote: > >>> On 11/3/11, Joonyoung Shim<jy0922.shim@samsung.com> wrote: > >>>> 11/03/2011 04:46 PM, Kukjin Kim 쓴 글: > >>>>> Joonyoung Shim wrote: > >>>>>> 11/03/2011 10:59 AM, Kukjin Kim 쓴 글: > >>>>>>> Joonyoung Shim wrote: > >>>>>>>> PWM timers use pclk("timers" clk) as parent clk. If this pclk is the > >>>>>>>> disabled state when PWM driver is probed, then it causes wrong read > >>>>>>>> and > >>>>>>>> write operation about registers of PWM. > >>>>>>>> > >>>>>>>> Signed-off-by: Joonyoung Shim<jy0922.shim@samsung.com> > >>>>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> > >>>>>>>> --- > >>>>>>>> arch/arm/plat-samsung/pwm.c | 7 +++++++ > >>>>>>>> 1 files changed, 7 insertions(+), 0 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/arch/arm/plat-samsung/pwm.c > >>>>>>>> b/arch/arm/plat-samsung/pwm.c > >>>>>>>> index f37457c..dc1185d 100644 > >>>>>>>> --- a/arch/arm/plat-samsung/pwm.c > >>>>>>>> +++ b/arch/arm/plat-samsung/pwm.c > >>>>>>>> @@ -299,6 +299,9 @@ static int s3c_pwm_probe(struct > platform_device > >>>>>>>> *pdev) > >>>>>>>> goto err_clk_tin; > >>>>>>>> } > >>>>>>>> > >>>>>>>> + clk_enable(pwm->clk); > >>>>>>>> + clk_enable(pwm->clk_div); > >>>>>>>> + > >>>>>>>> local_irq_save(flags); > >>>>>>>> > >>>>>>>> tcon = __raw_readl(S3C2410_TCON); > >>>>>>>> @@ -326,6 +329,8 @@ static int s3c_pwm_probe(struct > platform_device > >>>>>>>> *pdev) > >>>>>>>> return 0; > >>>>>>>> > >>>>>>>> err_clk_tdiv: > >>>>>>>> + clk_disable(pwm->clk_div); > >>>>>>>> + clk_disable(pwm->clk); > >>>>>>>> clk_put(pwm->clk_div); > >>>>>>>> > >>>>>>>> err_clk_tin: > >>>>>>>> @@ -340,6 +345,8 @@ static int __devexit s3c_pwm_remove(struct > >>>>>>>> platform_device *pdev) > >>>>>>>> { > >>>>>>>> struct pwm_device *pwm = platform_get_drvdata(pdev); > >>>>>>>> > >>>>>>>> + clk_disable(pwm->clk_div); > >>>>>>>> + clk_disable(pwm->clk); > >>>>>>>> clk_put(pwm->clk_div); > >>>>>>>> clk_put(pwm->clk); > >>>>>>>> kfree(pwm); > >>>>>>>> -- > >>>>>>>> 1.7.5.4 > >>>>>>> Well, I wonder when this is needed. I think it should be enabled > >>>>>>> during > >>>>>>> kernel booting... > >>>>>> The exynos4 machine using just timer turns on "timer" clock in the > >>>>>> past, > >>>>>> but "timer" clock is disable state when boot since MCT is used. MCT > >>>>>> doesn't control "timer" clock. > >>>>>> > >>>>>> I think pwm driver should control(enable/disable) using clocks > >>>>>> regardless of their parents clock. > >>>>>> > >>>>> I mean, why that is disabled when kernel boot... > >>>>> > >>>> Please see arch/arm/mach-exynos4/clock.c > >>>> There is "timers' clock in the clk init_clocks_off[]. > >> One more, Don't assume some clocks are enabled at bootloader. user can > >> use any bootloader and any configuration. > >> > > Kyungmin, > > NO! I didn't. Just I thought your bootloader disables the clock and why it is > needed. > > > > Anyway, Joonyoung, > > How about following? And it seems enough... > > -- > > diff --git a/arch/arm/plat-samsung/pwm-clock.c b/arch/arm/plat-samsung/pwm- > clock.c > > index a35ff3b..37a159b 100644 > > --- a/arch/arm/plat-samsung/pwm-clock.c > > +++ b/arch/arm/plat-samsung/pwm-clock.c > > @@ -449,6 +449,8 @@ __init void s3c_pwmclk_init(void) > > return; > > } > > > > + clk_enable(clk_timers); > > + > > for (clk = 0; clk< ARRAY_SIZE(clk_timer_scaler); clk++) > > clk_timer_scaler[clk].parent = clk_timers; > > > > No, there is no the reason "timers" clock turns on always. > > > -- > > Or needs separate clk_enable like following? > > > > diff --git a/arch/arm/plat-samsung/pwm.c b/arch/arm/plat-samsung/pwm.c > > index f37457c..7a77e36 100644 > > --- a/arch/arm/plat-samsung/pwm.c > > +++ b/arch/arm/plat-samsung/pwm.c > > @@ -292,6 +292,8 @@ static int s3c_pwm_probe(struct platform_device *pdev) > > goto err_alloc; > > } > > > > + clk_enable(pwm->clk); > > + > > pwm->clk_div = clk_get(dev, "pwm-tdiv"); > > if (IS_ERR(pwm->clk_div)) { > > dev_err(dev, "failed to get pwm tdiv clk\n"); > > @@ -299,6 +301,8 @@ static int s3c_pwm_probe(struct platform_device *pdev) > > goto err_clk_tin; > > } > > > > + clk_enable(pwm->clk_div); > > + > > local_irq_save(flags); > > > > tcon = __raw_readl(S3C2410_TCON); > > @@ -326,9 +330,11 @@ static int s3c_pwm_probe(struct platform_device *pdev) > > return 0; > > > > err_clk_tdiv: > > + clk_disable(pwm->clk_div); > > clk_put(pwm->clk_div); > > > > err_clk_tin: > > + clk_disable(pwm->clk); > > clk_put(pwm->clk); > > > > err_alloc: > > -- > > I don't care about this. > > > > > BTW, I think this is not really needed for this merge window because the pwm.c > is used for only s3c24xx and as you know, it doesn't matter because of > arch/arm/plat-samsung/time.c. And if required, this can be fixed during -rc. > > It's the problem to assume that the related clocks turned on by other > and the pwm is used for exynos4 also. > OK, I agree, applied. Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
diff --git a/arch/arm/plat-samsung/pwm-clock.c b/arch/arm/plat-samsung/pwm-clock.c index a35ff3b..37a159b 100644 --- a/arch/arm/plat-samsung/pwm-clock.c +++ b/arch/arm/plat-samsung/pwm-clock.c @@ -449,6 +449,8 @@ __init void s3c_pwmclk_init(void) return; } + clk_enable(clk_timers); + for (clk = 0; clk < ARRAY_SIZE(clk_timer_scaler); clk++) clk_timer_scaler[clk].parent = clk_timers; -- Or needs separate clk_enable like following? diff --git a/arch/arm/plat-samsung/pwm.c b/arch/arm/plat-samsung/pwm.c index f37457c..7a77e36 100644 --- a/arch/arm/plat-samsung/pwm.c +++ b/arch/arm/plat-samsung/pwm.c @@ -292,6 +292,8 @@ static int s3c_pwm_probe(struct platform_device *pdev) goto err_alloc; } + clk_enable(pwm->clk); + pwm->clk_div = clk_get(dev, "pwm-tdiv"); if (IS_ERR(pwm->clk_div)) { dev_err(dev, "failed to get pwm tdiv clk\n"); @@ -299,6 +301,8 @@ static int s3c_pwm_probe(struct platform_device *pdev) goto err_clk_tin; } + clk_enable(pwm->clk_div); + local_irq_save(flags); tcon = __raw_readl(S3C2410_TCON); @@ -326,9 +330,11 @@ static int s3c_pwm_probe(struct platform_device *pdev) return 0; err_clk_tdiv: + clk_disable(pwm->clk_div); clk_put(pwm->clk_div); err_clk_tin: + clk_disable(pwm->clk); clk_put(pwm->clk); err_alloc: