ARM: SAMSUNG: Add clk enable/disable of pwm

PATCH New Joonyoung Shim 4 participants 12 messages Oct 25, 2011 – Nov 4, 2011

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm Samsung Exynos

lore

Share & tools

lore

Joonyoung Shim

author

2011-10-25 01:21 UTC #0 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

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 <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 arch/arm/plat-samsung/pwm.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

arch/arm/plat-samsung/pwm.c +7 -0

index f37457c..dc1185d 100644
@@ -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);

Signature · 1 line

1.7.5.4

Kukjin Kim

2011-11-03 01:59 UTC #1 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

Quoted 43 lines · Joonyoung Shim

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 <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 arch/arm/plat-samsung/pwm.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

arch/arm/plat-samsung/pwm.c +7 -0

index f37457c..dc1185d 100644
@@ -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);

Quoted 3 lines

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 <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

Joonyoung Shim

author

2011-11-03 02:24 UTC #2 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

11/03/2011 10:59 AM, Kukjin Kim 쓴 글:

Quoted 39 lines

Quoted 38 lines · Joonyoung Shim

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<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---
  arch/arm/plat-samsung/pwm.c |    7 +++++++
  1 files changed, 7 insertions(+), 0 deletions(-)

arch/arm/plat-samsung/pwm.c +5 -0

index f37457c..dc1185d 100644
@@ -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);

Quoted 9 lines

Quoted 7 lines
arch/arm/plat-samsung/pwm.c +2 -0
+	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.

Kukjin Kim

2011-11-03 07:46 UTC #3 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

Quoted 51 lines · Joonyoung Shim

11/03/2011 10:59 AM, Kukjin Kim ? ?:
Quoted 39 lines
Quoted 38 lines · Joonyoung Shim
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<[email protected]>
Signed-off-by Kyungmin Park<[email protected]>
---
  arch/arm/plat-samsung/pwm.c |    7 +++++++
  1 files changed, 7 insertions(+), 0 deletions(-)
arch/arm/plat-samsung/pwm.c +5 -0
index f37457c..dc1185d 100644
@@ -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);
 
Quoted 9 lines
Quoted 7 lines
arch/arm/plat-samsung/pwm.c +2 -0
+	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...

Quoted 7 lines

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 <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

Joonyoung Shim

author

2011-11-03 07:53 UTC #4 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

11/03/2011 04:46 PM, Kukjin Kim 쓴 글:

Quoted 50 lines

Quoted 49 lines · Joonyoung Shim
11/03/2011 10:59 AM, Kukjin Kim 쓴 글:
Quoted 48 lines
Quoted 43 lines · Joonyoung Shim
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<[email protected]>
Signed-off-by Kyungmin Park<[email protected]>
---
   arch/arm/plat-samsung/pwm.c |    7 +++++++
   1 files changed, 7 insertions(+), 0 deletions(-)
arch/arm/plat-samsung/pwm.c +7 -0
index f37457c..dc1185d 100644
@@ -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);
Quoted 2 lines
--
1.7.5.4
Well, I wonder when this is needed. I think it should be enabled during kernel booting...

Quoted 9 lines

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

Kyungmin Park

2011-11-03 08:25 UTC #5 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

Quoted 73 lines · Joonyoung Shim

11/03/2011 04:46 PM, Kukjin Kim ? ?:
Quoted 52 lines
Quoted 51 lines · Joonyoung Shim
11/03/2011 10:59 AM, Kukjin Kim ? ?:
Quoted 50 lines
Quoted 45 lines · Joonyoung Shim
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<[email protected]>
Signed-off-by Kyungmin Park<[email protected]>
---
   arch/arm/plat-samsung/pwm.c |    7 +++++++
   1 files changed, 7 insertions(+), 0 deletions(-)
arch/arm/plat-samsung/pwm.c +3 -0
index f37457c..dc1185d 100644
@@ -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);
arch/arm/plat-samsung/pwm.c +2 -0
@@ -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:
arch/arm/plat-samsung/pwm.c +2 -0
@@ -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);
Quoted 2 lines
--
1.7.5.4
Well, I wonder when this is needed. I think it should be enabled during kernel booting...
Quoted 9 lines
Quoted 7 lines
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 majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kyungmin Park

2011-11-03 08:28 UTC #6 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

Quoted 62 lines · Kyungmin Park

Quoted 61 lines · Joonyoung Shim
11/03/2011 04:46 PM, Kukjin Kim ? ?:
Quoted 60 lines
Quoted 59 lines · Joonyoung Shim
11/03/2011 10:59 AM, Kukjin Kim ? ?:
Quoted 43 lines
Quoted 42 lines · Joonyoung Shim
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<[email protected]>
Signed-off-by Kyungmin Park<[email protected]>
---
   arch/arm/plat-samsung/pwm.c |    7 +++++++
   1 files changed, 7 insertions(+), 0 deletions(-)
arch/arm/plat-samsung/pwm.c +3 -0
index f37457c..dc1185d 100644
@@ -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);
arch/arm/plat-samsung/pwm.c +2 -0
@@ -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:
arch/arm/plat-samsung/pwm.c +0 -0
@@ -340,6 +345,8 @@ static int __devexit s3c_pwm_remove(struct
 platform_device *pdev)
   {
   	struct pwm_device *pwm = platform_get_drvdata(pdev);
 
Quoted 10 lines
Quoted 7 lines
arch/arm/plat-samsung/pwm.c +2 -0
+	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.

Quoted 8 lines

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.

Quoted 10 lines

Quoted 9 lines
Thanks.
--
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

Kukjin Kim

2011-11-03 09:08 UTC #7 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

Quoted 67 lines · Kyungmin Park

Quoted 65 lines · Kyungmin Park
Quoted 64 lines · Joonyoung Shim
11/03/2011 04:46 PM, Kukjin Kim ? ?:
Quoted 63 lines
Quoted 62 lines · Joonyoung Shim
11/03/2011 10:59 AM, Kukjin Kim ? ?:
Quoted 43 lines
Quoted 42 lines · Joonyoung Shim
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<[email protected]>
Signed-off-by Kyungmin Park<[email protected]>
---
   arch/arm/plat-samsung/pwm.c |    7 +++++++
   1 files changed, 7 insertions(+), 0 deletions(-)
arch/arm/plat-samsung/pwm.c +3 -0
index f37457c..dc1185d 100644
@@ -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);
arch/arm/plat-samsung/pwm.c +2 -0
@@ -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:
arch/arm/plat-samsung/pwm.c +0 -0
@@ -340,6 +345,8 @@ static int __devexit s3c_pwm_remove(struct
 platform_device *pdev)
   {
   	struct pwm_device *pwm = platform_get_drvdata(pdev);
 
Quoted 10 lines
Quoted 7 lines
arch/arm/plat-samsung/pwm.c +2 -0
+	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.

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
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...
--

arch/arm/plat-samsung/pwm-clock.c +2 -0

index a35ff3b..37a159b 100644
@@ -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?

arch/arm/plat-samsung/pwm.c +6 -0

index f37457c..7a77e36 100644
@@ -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 <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

Russell King - ARM Linux

2011-11-03 09:10 UTC #8 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

arch/arm/plat-samsung/pwm-clock.c __init void s3c_pwmclk_init(void) · line 452

Quoted 9 lines · Kukjin Kim

arch/arm/plat-samsung/pwm-clock.c +1 -0

index a35ff3b..37a159b 100644
@@ -449,6 +449,8 @@ __init void s3c_pwmclk_init(void)
 		return;
 	}
 
+	clk_enable(clk_timers);

Error checking? clk_prepare() as well?

Kyungmin Park

2011-11-03 09:43 UTC #9 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

In reply to Kukjin Kim

Quoted 72 lines · Kukjin Kim

Quoted 71 lines · Kyungmin Park
Quoted 69 lines · Kyungmin Park
Quoted 68 lines · Joonyoung Shim
11/03/2011 04:46 PM, Kukjin Kim ? ?:
Quoted 66 lines
Quoted 55 lines · Joonyoung Shim
11/03/2011 10:59 AM, Kukjin Kim ? ?:
Quoted 44 lines
Quoted 43 lines · Joonyoung Shim
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<[email protected]>
Signed-off-by Kyungmin Park<[email protected]>
---
   arch/arm/plat-samsung/pwm.c |    7 +++++++
   1 files changed, 7 insertions(+), 0 deletions(-)
arch/arm/plat-samsung/pwm.c +3 -0
index f37457c..dc1185d 100644
@@ -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);
arch/arm/plat-samsung/pwm.c +2 -0
@@ -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:
arch/arm/plat-samsung/pwm.c +0 -0
@@ -340,6 +345,8 @@ static int __devexit s3c_pwm_remove(struct
 platform_device *pdev)
   {
   	struct pwm_device *pwm = platform_get_drvdata(pdev);
 
Quoted 10 lines
Quoted 7 lines
arch/arm/plat-samsung/pwm.c +2 -0
+	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...
Quoted 8 lines
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...

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
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.

Quoted 57 lines

Anyway, Joonyoung,
How about following? And it seems enough...
--

arch/arm/plat-samsung/pwm-clock.c +2 -1

index a35ff3b..37a159b 100644
@@ -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?

arch/arm/plat-samsung/pwm.c +6 -1

index f37457c..7a77e36 100644
@@ -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:
--

Quoted 3 lines

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?

Quoted 15 lines

Thanks.
Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, 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

Joonyoung Shim

author

2011-11-03 10:20 UTC #10 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

In reply to Kukjin Kim

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

Quoted 85 lines

Quoted 66 lines · Kyungmin Park
Quoted 65 lines · Kyungmin Park
Quoted 64 lines · Joonyoung Shim
11/03/2011 04:46 PM, Kukjin Kim 쓴 글:
Quoted 63 lines
Quoted 62 lines · Joonyoung Shim
11/03/2011 10:59 AM, Kukjin Kim 쓴 글:
Quoted 43 lines
Quoted 42 lines · Joonyoung Shim
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<[email protected]>
Signed-off-by Kyungmin Park<[email protected]>
---
    arch/arm/plat-samsung/pwm.c |    7 +++++++
    1 files changed, 7 insertions(+), 0 deletions(-)
arch/arm/plat-samsung/pwm.c +3 -0
index f37457c..dc1185d 100644
@@ -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);
arch/arm/plat-samsung/pwm.c +2 -0
@@ -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:
arch/arm/plat-samsung/pwm.c +0 -0
@@ -340,6 +345,8 @@ static int __devexit s3c_pwm_remove(struct
 platform_device *pdev)
    {
    	struct pwm_device *pwm = platform_get_drvdata(pdev);
 
Quoted 10 lines
Quoted 7 lines
arch/arm/plat-samsung/pwm.c +2 -0
+	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.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
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...
--
arch/arm/plat-samsung/pwm-clock.c +0 -0
index a35ff3b..37a159b 100644

arch/arm/plat-samsung/pwm-clock.c __init void s3c_pwmclk_init(void) · line 456

Quoted 8 lines

arch/arm/plat-samsung/pwm-clock.c +2 -0

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

Quoted 28 lines

--
Or needs separate clk_enable like following?

arch/arm/plat-samsung/pwm.c +4 -0

index f37457c..7a77e36 100644
@@ -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;

arch/arm/plat-samsung/pwm.c static int s3c_pwm_probe(struct platform_device *pdev)

Quoted 10 lines

arch/arm/plat-samsung/pwm.c +2 -1

  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.

Quoted 2 lines

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.

Quoted 9 lines

Thanks.
Best regards,
Kgene.
--
Kukjin Kim<[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

Kukjin Kim

2011-11-04 05:01 UTC #11 lore raw

PATCH 1/1 ARM: SAMSUNG: Add clk enable/disable of pwm

Quoted 144 lines · Joonyoung Shim

11/03/2011 06:08 PM, Kukjin Kim ? ?:
Quoted 18 lines
Quoted 17 lines · Kyungmin Park
Quoted 16 lines · Kyungmin Park
Quoted 15 lines · Joonyoung Shim
11/03/2011 04:46 PM, Kukjin Kim ? ?:
Quoted 14 lines
Quoted 13 lines · Joonyoung Shim
11/03/2011 10:59 AM, Kukjin Kim ? ?:
Quoted 12 lines
Quoted 11 lines · Joonyoung Shim
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<[email protected]>
Signed-off-by Kyungmin Park<[email protected]>
---
    arch/arm/plat-samsung/pwm.c |    7 +++++++
    1 files changed, 7 insertions(+), 0 deletions(-)
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
arch/arm/plat-samsung/pwm.c static int s3c_pwm_probe(struct
Quoted 6 lines
arch/arm/plat-samsung/pwm.c +0 -0
index f37457c..dc1185d 100644
@@ -299,6 +299,9 @@ static int s3c_pwm_probe(struct
platform_device
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
arch/arm/plat-samsung/pwm.c static int s3c_pwm_probe(struct
Quoted 11 lines
arch/arm/plat-samsung/pwm.c +3 -0
 *pdev)
    		goto err_clk_tin;
    	}
 
+	clk_enable(pwm->clk);
+	clk_enable(pwm->clk_div);
+
    	local_irq_save(flags);
 
    	tcon = __raw_readl(S3C2410_TCON);
arch/arm/plat-samsung/pwm.c +0 -0
@@ -326,6 +329,8 @@ static int s3c_pwm_probe(struct
platform_device
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 24 lines
Quoted 14 lines
Quoted 14 lines
arch/arm/plat-samsung/pwm.c +2 -0
 *pdev)
    	return 0;
 
     err_clk_tdiv:
+	clk_disable(pwm->clk_div);
+	clk_disable(pwm->clk);
    	clk_put(pwm->clk_div);
 
err_clk_tin:
arch/arm/plat-samsung/pwm.c +0 -0
@@ -340,6 +345,8 @@ static int __devexit s3c_pwm_remove(struct
 platform_device *pdev)
    {
    	struct pwm_device *pwm = platform_get_drvdata(pdev);
 
Quoted 10 lines
Quoted 7 lines
arch/arm/plat-samsung/pwm.c +2 -0
+	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...
Quoted 8 lines
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...
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
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.
arch/arm/plat-samsung/pwm-
Quoted 5 lines
Anyway, Joonyoung,
How about following? And it seems enough...
--
arch/arm/plat-samsung/pwm- renamed from arch/arm/plat-samsung/pwm-clock.c +0 -0
clock.c
arch/arm/plat-samsung/pwm-clock.c __init void s3c_pwmclk_init(void) · line 456
Quoted 12 lines
index a35ff3b..37a159b 100644
arch/arm/plat-samsung/pwm-clock.c +2 -0
@@ -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.
Quoted 28 lines
--
Or needs separate clk_enable like following?
arch/arm/plat-samsung/pwm.c +4 -0
index f37457c..7a77e36 100644
@@ -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;
 
arch/arm/plat-samsung/pwm.c static int s3c_pwm_probe(struct platform_device *pdev)
Quoted 10 lines
arch/arm/plat-samsung/pwm.c +2 -1
  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.
Quoted 1 line

Quoted 7 lines

Quoted 1 line
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 <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.