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

[Joonyoung Shim <jy0922.shim@samsung.com>]

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.
>
>