Joonyoung Shim wrote:
>
> 11/03/2011 06:08 PM, Kukjin Kim 쓴 글:
> > Kyungmin Park wrote:
> >> On 11/3/11, Kyungmin Park<kyungmin.park@xxxxxxxxxxx> wrote:
> >>> On 11/3/11, Joonyoung Shim<jy0922.shim@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@xxxxxxxxxxx>
> >>>>>>>> ---
> >>>>>>>> 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@xxxxxxxxxxx>, 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
