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

2011-11-03 Thread 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 Shimjy0922.s...@samsung.com
  Signed-off-by: Kyungmin Parkkyungmin.p...@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...

Thanks.

Best regards,
Kgene.
--
Kukjin Kim kgene@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-03 Thread Joonyoung Shim 

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 Shimjy0922.s...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@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[].

Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-03 Thread Kyungmin Park 
On 11/3/11, Joonyoung Shim jy0922.s...@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 Shimjy0922.s...@samsung.com
 Signed-off-by: Kyungmin Parkkyungmin.p...@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[].

 Thanks.
 --
 To unsubscribe from this list: send the line unsubscribe linux-samsung-soc
 in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-03 Thread Kyungmin Park 
On 11/3/11, Kyungmin Park kyungmin.p...@samsung.com wrote:
 On 11/3/11, Joonyoung Shim jy0922.s...@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 Shimjy0922.s...@samsung.com
 Signed-off-by: Kyungmin Parkkyungmin.p...@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.

Thanks.

 Thanks.
 --
 To unsubscribe from this list: send the line unsubscribe
 linux-samsung-soc
 in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-03 Thread Kukjin Kim 
Kyungmin Park wrote:
 
 On 11/3/11, Kyungmin Park kyungmin.p...@samsung.com wrote:
  On 11/3/11, Joonyoung Shim jy0922.s...@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 Shimjy0922.s...@samsung.com
  Signed-off-by: Kyungmin Parkkyungmin.p...@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@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-03 Thread Russell King - ARM Linux 
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?
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-03 Thread Kyungmin Park 
On 11/3/11, Kukjin Kim kgene@samsung.com wrote:
 Kyungmin Park wrote:

 On 11/3/11, Kyungmin Park kyungmin.p...@samsung.com wrote:
  On 11/3/11, Joonyoung Shim jy0922.s...@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 Shimjy0922.s...@samsung.com
  Signed-off-by: Kyungmin Parkkyungmin.p...@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@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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To

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

2011-11-03 Thread Joonyoung Shim 

11/03/2011 06:08 PM, Kukjin Kim 쓴 글:

Kyungmin Park wrote:

On 11/3/11, Kyungmin Parkkyungmin.p...@samsung.com  wrote:

On 11/3/11, Joonyoung Shimjy0922.s...@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 Shimjy0922.s...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@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 Kimkgene@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-03 Thread Kukjin Kim 
Joonyoung Shim wrote:
 
 11/03/2011 06:08 PM, Kukjin Kim 쓴 글:
  Kyungmin Park wrote:
  On 11/3/11, Kyungmin Parkkyungmin.p...@samsung.com  wrote:
  On 11/3/11, Joonyoung Shimjy0922.s...@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 Shimjy0922.s...@samsung.com
  Signed-off-by: Kyungmin Parkkyungmin.p...@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@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-02 Thread 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.s...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@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...

Thanks.

Best regards,
Kgene.
--
Kukjin Kim kgene@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-02 Thread Joonyoung Shim 

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 Shimjy0922.s...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@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.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html