* [PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm
@ 2011-11-03 9:08 ` Kukjin Kim
0 siblings, 0 replies; 24+ messages in thread
From: Kukjin Kim @ 2011-11-03 9:08 UTC (permalink / raw)
To: linux-arm-kernel
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;
--
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.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm
2011-11-03 9:08 ` Kukjin Kim
@ 2011-11-03 9:10 ` Russell King - ARM Linux
-1 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2011-11-03 9:10 UTC (permalink / raw)
To: Kukjin Kim
Cc: 'Kyungmin Park', 'Joonyoung Shim',
linux-samsung-soc, ben-linux, linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm
@ 2011-11-03 9:10 ` Russell King - ARM Linux
0 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2011-11-03 9:10 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm
2011-11-03 9:08 ` Kukjin Kim
@ 2011-11-03 9:43 ` Kyungmin Park
-1 siblings, 0 replies; 24+ messages in thread
From: Kyungmin Park @ 2011-11-03 9:43 UTC (permalink / raw)
To: Kukjin Kim; +Cc: Joonyoung Shim, linux-arm-kernel, linux-samsung-soc, ben-linux
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm
@ 2011-11-03 9:43 ` Kyungmin Park
0 siblings, 0 replies; 24+ messages in thread
From: Kyungmin Park @ 2011-11-03 9:43 UTC (permalink / raw)
To: linux-arm-kernel
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 at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm
2011-11-03 9:08 ` Kukjin Kim
@ 2011-11-03 10:20 ` Joonyoung Shim
-1 siblings, 0 replies; 24+ messages in thread
From: Joonyoung Shim @ 2011-11-03 10:20 UTC (permalink / raw)
To: Kukjin Kim
Cc: 'Kyungmin Park', linux-arm-kernel, linux-samsung-soc,
ben-linux
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.
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm
@ 2011-11-03 10:20 ` Joonyoung Shim
0 siblings, 0 replies; 24+ messages in thread
From: Joonyoung Shim @ 2011-11-03 10:20 UTC (permalink / raw)
To: linux-arm-kernel
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.
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* RE: [PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm
2011-11-03 10:20 ` Joonyoung Shim
@ 2011-11-04 5:01 ` Kukjin Kim
-1 siblings, 0 replies; 24+ messages in thread
From: Kukjin Kim @ 2011-11-04 5:01 UTC (permalink / raw)
To: 'Joonyoung Shim'
Cc: 'Kyungmin Park', linux-arm-kernel, linux-samsung-soc,
ben-linux
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.
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH] ARM: SAMSUNG: Add clk enable/disable of pwm
@ 2011-11-04 5:01 ` Kukjin Kim
0 siblings, 0 replies; 24+ messages in thread
From: Kukjin Kim @ 2011-11-04 5:01 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 24+ messages in thread