[PATCH] pwm: imx: Add support for low-speed PWM

[PATCH] pwm: imx: Add support for low-speed PWM

[Sean Cross]

The i.MX PWM driver has a selectable clock source.  In order to get very
long period lengths, the driver must select the 32.768 kHz clock.

It's unclear what the threshold should be to switch clocks, so set it
arbitrarily to 100 usecs.

Sean Cross (1):
  pwm: imx: Support very long period lengths

 drivers/pwm/pwm-imx.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

-- 
1.9.2

[PATCH] pwm: imx: Support very long period lengths

[Sean Cross]

The IMX PWM block supports using both the system clock and a 32 kHz
clock for driving PWM events.  For very long period lengths, use the
32 kHz clock instead of the high-speed clock.

Signed-off-by: Sean Cross <xobs-nXMMniAx+RbQT0dZR+AlfA@public.gmane.org>
---
 drivers/pwm/pwm-imx.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index cc47733..8410455 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -36,9 +36,11 @@
 #define MX3_PWMCR_DOZEEN                (1 << 24)
 #define MX3_PWMCR_WAITEN                (1 << 23)
 #define MX3_PWMCR_DBGEN			(1 << 22)
+#define MX3_PWMCR_CLKSRC_IPG_32K  (3 << 16)
 #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
 #define MX3_PWMCR_CLKSRC_IPG      (1 << 16)
 #define MX3_PWMCR_EN              (1 << 0)
+#define MX3_SLOW_THRESHOLD_NS	100000
 
 struct imx_chip {
 	struct clk	*clk_per;
@@ -107,7 +109,13 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	unsigned long period_cycles, duty_cycles, prescale;
 	u32 cr;
 
-	c = clk_get_rate(imx->clk_per);
+	if (duty_ns > MX3_SLOW_THRESHOLD_NS) {
+		cr = MX3_PWMCR_CLKSRC_IPG_32K;
+		c = 32768;
+	} else {
+		cr = MX3_PWMCR_CLKSRC_IPG_HIGH;
+		c = clk_get_rate(imx->clk_per);
+	}
 	c = c * period_ns;
 	do_div(c, 1000000000);
 	period_cycles = c;
@@ -131,9 +139,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
-	cr = MX3_PWMCR_PRESCALER(prescale) |
+	cr |= MX3_PWMCR_PRESCALER(prescale) |
 		MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
-		MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
+		MX3_PWMCR_DBGEN;
 
 	if (test_bit(PWMF_ENABLED, &pwm->flags))
 		cr |= MX3_PWMCR_EN;
-- 
1.9.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pwm: imx: Support very long period lengths

[Sascha Hauer]

On Mon, Apr 28, 2014 at 04:55:31PM +0800, Sean Cross wrote:
> The IMX PWM block supports using both the system clock and a 32 kHz
> clock for driving PWM events.  For very long period lengths, use the
> 32 kHz clock instead of the high-speed clock.
> 
> Signed-off-by: Sean Cross <xobs@kosagi.com>
> ---
>  drivers/pwm/pwm-imx.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index cc47733..8410455 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -36,9 +36,11 @@
>  #define MX3_PWMCR_DOZEEN                (1 << 24)
>  #define MX3_PWMCR_WAITEN                (1 << 23)
>  #define MX3_PWMCR_DBGEN			(1 << 22)
> +#define MX3_PWMCR_CLKSRC_IPG_32K  (3 << 16)
>  #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
>  #define MX3_PWMCR_CLKSRC_IPG      (1 << 16)
>  #define MX3_PWMCR_EN              (1 << 0)
> +#define MX3_SLOW_THRESHOLD_NS	100000
>  
>  struct imx_chip {
>  	struct clk	*clk_per;
> @@ -107,7 +109,13 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  	unsigned long period_cycles, duty_cycles, prescale;
>  	u32 cr;
>  
> -	c = clk_get_rate(imx->clk_per);
> +	if (duty_ns > MX3_SLOW_THRESHOLD_NS) {

If anything you have to check the period_cycles, not the duty_cycles.

100000 seems to be some arbitrary value suitable for some unspecified
ipg clock rate. I think you should use it when the period_cycles
register overflows.

> +		cr = MX3_PWMCR_CLKSRC_IPG_32K;
> +		c = 32768;

How about providing a proper clock instead of hardcoding this?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] pwm: imx: Support very long period lengths

[Sean Cross]

On 04/28/14 17:22, Sascha Hauer wrote:
> On Mon, Apr 28, 2014 at 04:55:31PM +0800, Sean Cross wrote:
>> The IMX PWM block supports using both the system clock and a 32 kHz
>> clock for driving PWM events.  For very long period lengths, use the
>> 32 kHz clock instead of the high-speed clock.
>>
>> Signed-off-by: Sean Cross <xobs@kosagi.com>
>> ---
>>   drivers/pwm/pwm-imx.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> index cc47733..8410455 100644
>> --- a/drivers/pwm/pwm-imx.c
>> +++ b/drivers/pwm/pwm-imx.c
>> @@ -36,9 +36,11 @@
>>   #define MX3_PWMCR_DOZEEN                (1 << 24)
>>   #define MX3_PWMCR_WAITEN                (1 << 23)
>>   #define MX3_PWMCR_DBGEN			(1 << 22)
>> +#define MX3_PWMCR_CLKSRC_IPG_32K  (3 << 16)
>>   #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
>>   #define MX3_PWMCR_CLKSRC_IPG      (1 << 16)
>>   #define MX3_PWMCR_EN              (1 << 0)
>> +#define MX3_SLOW_THRESHOLD_NS	100000
>>   
>>   struct imx_chip {
>>   	struct clk	*clk_per;
>> @@ -107,7 +109,13 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>>   	unsigned long period_cycles, duty_cycles, prescale;
>>   	u32 cr;
>>   
>> -	c = clk_get_rate(imx->clk_per);
>> +	if (duty_ns > MX3_SLOW_THRESHOLD_NS) {
> If anything you have to check the period_cycles, not the duty_cycles.
>
> 100000 seems to be some arbitrary value suitable for some unspecified
> ipg clock rate. I think you should use it when the period_cycles
> register overflows.
In this case, it's for a backlight controller that requires a 100 Hz 
clock.  But you're right, hard-coding the values is a bad idea.

With the prescaler and the period_cycles register, it might not actually 
overflow, but instead the resolution might become too poor to be 
useful.  The prescaler supports dividing by up to 4096, but it gets 
difficult to hit a particular value.

So with my target of a 100 Hz clock, it appears as though no values 
overflow yet I can't use IPG_HIGH.

What is the best way to handle this?
>> +		cr = MX3_PWMCR_CLKSRC_IPG_32K;
>> +		c = 32768;
> How about providing a proper clock instead of hardcoding this?
>
That seems like a lot of work, and I don't see any other PWM devices 
that do this.  If I understand the clock system, I'd have to add the 
following to pwm-imx.c:

     - Each PWM would get three new clocks added to its struct imx_chip 
-- a clk_ipg_mux, a clk_ipg_32k, and a clk_ipg_high
     - The struct imx_chip would also gain a clk_ckil entry for the 
32768 Hz clock
     - These three clocks would need to be created on _probe() and 
destroyed on _remove()
     - clk_ipg_32k would get reparented to ckil (or whatever the 32768 
Hz clock is on that platform)
     - clk_ipg_high would get reparented to imx->clk_per
     - The recalc_rate(), round_rate(), and set_rate() would need to be 
implemented (and calculated) for clk_ipg_32k and clk_ipg_high

Additionally, "ckil" would need to be added to the pwm entry for every 
dts file.

Is it worth it to add that much complexity?  What does it get us to have 
a proper clock?  Or have I overestimated the amount of rework involved?  
I'm happy to do it if you think it's necessary.


Sean

Re: [PATCH] pwm: imx: Support very long period lengths

[Sascha Hauer]

On Mon, Apr 28, 2014 at 08:10:09PM +0800, Sean Cross wrote:
> On 04/28/14 17:22, Sascha Hauer wrote:
> >On Mon, Apr 28, 2014 at 04:55:31PM +0800, Sean Cross wrote:
> >>The IMX PWM block supports using both the system clock and a 32 kHz
> >>clock for driving PWM events.  For very long period lengths, use the
> >>32 kHz clock instead of the high-speed clock.
> >>
> >>Signed-off-by: Sean Cross <xobs@kosagi.com>
> >>---
> >>  drivers/pwm/pwm-imx.c | 14 +++++++++++---
> >>  1 file changed, 11 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> >>index cc47733..8410455 100644
> >>--- a/drivers/pwm/pwm-imx.c
> >>+++ b/drivers/pwm/pwm-imx.c
> >>@@ -36,9 +36,11 @@
> >>  #define MX3_PWMCR_DOZEEN                (1 << 24)
> >>  #define MX3_PWMCR_WAITEN                (1 << 23)
> >>  #define MX3_PWMCR_DBGEN			(1 << 22)
> >>+#define MX3_PWMCR_CLKSRC_IPG_32K  (3 << 16)
> >>  #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
> >>  #define MX3_PWMCR_CLKSRC_IPG      (1 << 16)
> >>  #define MX3_PWMCR_EN              (1 << 0)
> >>+#define MX3_SLOW_THRESHOLD_NS	100000
> >>  struct imx_chip {
> >>  	struct clk	*clk_per;
> >>@@ -107,7 +109,13 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> >>  	unsigned long period_cycles, duty_cycles, prescale;
> >>  	u32 cr;
> >>-	c = clk_get_rate(imx->clk_per);
> >>+	if (duty_ns > MX3_SLOW_THRESHOLD_NS) {
> >If anything you have to check the period_cycles, not the duty_cycles.
> >
> >100000 seems to be some arbitrary value suitable for some unspecified
> >ipg clock rate. I think you should use it when the period_cycles
> >register overflows.
> In this case, it's for a backlight controller that requires a 100 Hz
> clock.  But you're right, hard-coding the values is a bad idea.
> 
> With the prescaler and the period_cycles register, it might not
> actually overflow, but instead the resolution might become too poor
> to be useful.  The prescaler supports dividing by up to 4096, but it
> gets difficult to hit a particular value.

Hm, this seems to be a common problem. grep for prescale in the pwm
drivers. It seems several drivers iterate over the possible prescalers
to find the best values. Maybe it's time to add some helper function
for this task?

> 
> So with my target of a 100 Hz clock, it appears as though no values
> overflow yet I can't use IPG_HIGH.
> 
> What is the best way to handle this?
> >>+		cr = MX3_PWMCR_CLKSRC_IPG_32K;
> >>+		c = 32768;
> >How about providing a proper clock instead of hardcoding this?
> >
> That seems like a lot of work, and I don't see any other PWM devices
> that do this.  If I understand the clock system, I'd have to add the
> following to pwm-imx.c:
> 
>     - Each PWM would get three new clocks added to its struct
> imx_chip -- a clk_ipg_mux, a clk_ipg_32k, and a clk_ipg_high
>     - The struct imx_chip would also gain a clk_ckil entry for the
> 32768 Hz clock
>     - These three clocks would need to be created on _probe() and
> destroyed on _remove()
>     - clk_ipg_32k would get reparented to ckil (or whatever the
> 32768 Hz clock is on that platform)
>     - clk_ipg_high would get reparented to imx->clk_per
>     - The recalc_rate(), round_rate(), and set_rate() would need to
> be implemented (and calculated) for clk_ipg_32k and clk_ipg_high

I didn't mean you should implement the PWM internal mux as clocks. This
is completely driver internal and it's not necessary to make them real
clocks.

> 
> Additionally, "ckil" would need to be added to the pwm entry for
> every dts file.

Yes, indeed. This way we do not rely on hardcoded values for ckil. ckil
is 32000Hz on some (admittedly older) SoCs.

So my suggestion is: Add ckil lookup to the pwm nodes of imx6qdl.dtsi,
do a clk_get(dev, "ckil") in the driver and use it when available.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |