[PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm

[Kukjin Kim]

Joonyoung Shim wrote:
> 
> 11/03/2011 06:08 PM, Kukjin Kim 쓴 글:
> > Kyungmin Park wrote:
> >> On 11/3/11, Kyungmin Park<kyungmin.park at samsung.com>  wrote:
> >>> On 11/3/11, Joonyoung Shim<jy0922.shim at 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 at samsung.com>
> >>>>>>>> Signed-off-by: Kyungmin Park<kyungmin.park at 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 at samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.