[PATCH] pwm: atmel-hlcdc: add at91sam9x5 and sama5d3 errata handling

[PATCH] pwm: atmel-hlcdc: add at91sam9x5 and sama5d3 errata handling

[Boris Brezillon]

at91sam9x5 has an errata forbidding the use of slow clk as a clk source and
sama5d3 SoCs has another errata forbidding the use of div1 prescaler.

Take both of these erratas into account.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-atmel-hlcdc.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
index eaf8b12..405f8a5 100644
--- a/drivers/pwm/pwm-atmel-hlcdc.c
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -36,6 +36,8 @@ struct atmel_hlcdc_pwm {
 	struct pwm_chip chip;
 	struct atmel_hlcdc *hlcdc;
 	struct clk *cur_clk;
+	bool slow_clk_errata;
+	bool div1_clk_errata;
 };
 
 static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
@@ -56,20 +58,28 @@ static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
 	u32 pwmcfg;
 	int pres;
 
-	clk_freq = clk_get_rate(new_clk);
-	clk_period_ns = (u64)NSEC_PER_SEC * 256;
-	do_div(clk_period_ns, clk_freq);
+	if (!chip->slow_clk_errata) {
+		clk_freq = clk_get_rate(new_clk);
+		clk_period_ns = (u64)NSEC_PER_SEC * 256;
+		do_div(clk_period_ns, clk_freq);
+	}
 
-	if (clk_period_ns > period_ns) {
+	/* Errata: cannot use slow clk on some IP revisions */
+	if (chip->slow_clk_errata || clk_period_ns > period_ns) {
 		new_clk = hlcdc->sys_clk;
 		clk_freq = clk_get_rate(new_clk);
 		clk_period_ns = (u64)NSEC_PER_SEC * 256;
 		do_div(clk_period_ns, clk_freq);
 	}
 
-	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++)
+	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
+		/* Errata: cannot divide by 1 on some IP revisions */
+		if (!pres && chip->div1_clk_errata)
+			continue;
+
 		if ((clk_period_ns << pres) >= period_ns)
 			break;
+	}
 
 	if (pres > ATMEL_HLCDC_PWMPS_MAX)
 		return -EINVAL;
@@ -204,6 +214,14 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (of_device_is_compatible(dev->parent->of_node,
+				    "atmel,sama5d3-hlcdc"))
+		chip->div1_clk_errata = true;
+
+	if (of_device_is_compatible(dev->parent->of_node,
+				    "atmel,at91sam9x5-hlcdc"))
+		chip->slow_clk_errata = true;
+
 	chip->hlcdc = hlcdc;
 	chip->chip.ops = &atmel_hlcdc_pwm_ops;
 	chip->chip.dev = dev;
-- 
1.9.1

Re: [PATCH] pwm: atmel-hlcdc: add at91sam9x5 and sama5d3 errata handling

[Nicolas Ferre]

On 18/11/2014 13:40, Boris Brezillon :
> at91sam9x5 has an errata forbidding the use of slow clk as a clk source and
> sama5d3 SoCs has another errata forbidding the use of div1 prescaler.
> 
> Take both of these erratas into account.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Thanks for having dealt with this.

Bye,

> ---
>  drivers/pwm/pwm-atmel-hlcdc.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> index eaf8b12..405f8a5 100644
> --- a/drivers/pwm/pwm-atmel-hlcdc.c
> +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> @@ -36,6 +36,8 @@ struct atmel_hlcdc_pwm {
>  	struct pwm_chip chip;
>  	struct atmel_hlcdc *hlcdc;
>  	struct clk *cur_clk;
> +	bool slow_clk_errata;
> +	bool div1_clk_errata;
>  };
>  
>  static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
> @@ -56,20 +58,28 @@ static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
>  	u32 pwmcfg;
>  	int pres;
>  
> -	clk_freq = clk_get_rate(new_clk);
> -	clk_period_ns = (u64)NSEC_PER_SEC * 256;
> -	do_div(clk_period_ns, clk_freq);
> +	if (!chip->slow_clk_errata) {
> +		clk_freq = clk_get_rate(new_clk);
> +		clk_period_ns = (u64)NSEC_PER_SEC * 256;
> +		do_div(clk_period_ns, clk_freq);
> +	}
>  
> -	if (clk_period_ns > period_ns) {
> +	/* Errata: cannot use slow clk on some IP revisions */
> +	if (chip->slow_clk_errata || clk_period_ns > period_ns) {
>  		new_clk = hlcdc->sys_clk;
>  		clk_freq = clk_get_rate(new_clk);
>  		clk_period_ns = (u64)NSEC_PER_SEC * 256;
>  		do_div(clk_period_ns, clk_freq);
>  	}
>  
> -	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++)
> +	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
> +		/* Errata: cannot divide by 1 on some IP revisions */
> +		if (!pres && chip->div1_clk_errata)
> +			continue;
> +
>  		if ((clk_period_ns << pres) >= period_ns)
>  			break;
> +	}
>  
>  	if (pres > ATMEL_HLCDC_PWMPS_MAX)
>  		return -EINVAL;
> @@ -204,6 +214,14 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	if (of_device_is_compatible(dev->parent->of_node,
> +				    "atmel,sama5d3-hlcdc"))
> +		chip->div1_clk_errata = true;
> +
> +	if (of_device_is_compatible(dev->parent->of_node,
> +				    "atmel,at91sam9x5-hlcdc"))
> +		chip->slow_clk_errata = true;
> +
>  	chip->hlcdc = hlcdc;
>  	chip->chip.ops = &atmel_hlcdc_pwm_ops;
>  	chip->chip.dev = dev;
> 


-- 
Nicolas Ferre

Re: [PATCH] pwm: atmel-hlcdc: add at91sam9x5 and sama5d3 errata handling

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4015 bytes --]

On Tue, Nov 18, 2014 at 01:40:53PM +0100, Boris Brezillon wrote:
> at91sam9x5 has an errata forbidding the use of slow clk as a clk source and
> sama5d3 SoCs has another errata forbidding the use of div1 prescaler.
> 
> Take both of these erratas into account.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/pwm-atmel-hlcdc.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> index eaf8b12..405f8a5 100644
> --- a/drivers/pwm/pwm-atmel-hlcdc.c
> +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> @@ -36,6 +36,8 @@ struct atmel_hlcdc_pwm {
>  	struct pwm_chip chip;
>  	struct atmel_hlcdc *hlcdc;
>  	struct clk *cur_clk;
> +	bool slow_clk_errata;
> +	bool div1_clk_errata;
>  };
>  
>  static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
> @@ -56,20 +58,28 @@ static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
>  	u32 pwmcfg;
>  	int pres;
>  
> -	clk_freq = clk_get_rate(new_clk);
> -	clk_period_ns = (u64)NSEC_PER_SEC * 256;
> -	do_div(clk_period_ns, clk_freq);
> +	if (!chip->slow_clk_errata) {
> +		clk_freq = clk_get_rate(new_clk);
> +		clk_period_ns = (u64)NSEC_PER_SEC * 256;
> +		do_div(clk_period_ns, clk_freq);
> +	}
>  
> -	if (clk_period_ns > period_ns) {
> +	/* Errata: cannot use slow clk on some IP revisions */
> +	if (chip->slow_clk_errata || clk_period_ns > period_ns) {
>  		new_clk = hlcdc->sys_clk;
>  		clk_freq = clk_get_rate(new_clk);
>  		clk_period_ns = (u64)NSEC_PER_SEC * 256;
>  		do_div(clk_period_ns, clk_freq);
>  	}
>  
> -	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++)
> +	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
> +		/* Errata: cannot divide by 1 on some IP revisions */
> +		if (!pres && chip->div1_clk_errata)
> +			continue;
> +
>  		if ((clk_period_ns << pres) >= period_ns)
>  			break;
> +	}
>  
>  	if (pres > ATMEL_HLCDC_PWMPS_MAX)
>  		return -EINVAL;
> @@ -204,6 +214,14 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	if (of_device_is_compatible(dev->parent->of_node,
> +				    "atmel,sama5d3-hlcdc"))
> +		chip->div1_clk_errata = true;
> +
> +	if (of_device_is_compatible(dev->parent->of_node,
> +				    "atmel,at91sam9x5-hlcdc"))
> +		chip->slow_clk_errata = true;

Generally I'd prefer this to be done as "SoC data", where the idea is to
not rely on these checks at probe time (because they don't scale very
well in the long term). Somewhat like the following:

	struct atmel_hlcdc_soc {
		bool slow_clk_errata;
		bool div1_clk_errata;
	};

	static const struct atmel_hlcdc_soc sama5d3_hlcdc = {
		.slow_clk_errata = false,
		.div1_clk_errata = true,
	};

	static const struct atmel_hlcdc_soc at91sam9x5_hlcdc = {
		.slow_clk_errata = true,
		.div1_clk_errata = false,
	};

Then put those into a struct of_device_id table and do the matching
using of_match_device(). Then you can simply do something like:

	match = of_match_device(dev->parent, atmel_hlcdc_matches);
	if (match)
		chip->soc = match->data;

And then check on the fields set therein. This works optimally if the
device isn't a subdevice because you have most of that code anyway. In
this particular case the lookup needs to happen on the parent, which
isn't so nice.

I'm willing to let this go for now, but if this list is going to grow
I'll request that it be done differently so that .probe() doesn't need
to be cluttered.

Also I wonder if it would be better to just add these new compatibles to
the PWM block binding, too, since it's obviously the IP blocks that have
these errata rather than the HLCDC block. So technically I think you'd
have to make the driver support atmel,at91sam9x5-hlcdc-pwm and
atmel,sama5d3-hlcdc-pwm anyway, and then you could just as well move to
the above matching and SoC data.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pwm: atmel-hlcdc: add at91sam9x5 and sama5d3 errata handling

[Boris Brezillon]

Hi Thierry,

On Tue, 18 Nov 2014 16:00:23 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Nov 18, 2014 at 01:40:53PM +0100, Boris Brezillon wrote:
> > at91sam9x5 has an errata forbidding the use of slow clk as a clk source and
> > sama5d3 SoCs has another errata forbidding the use of div1 prescaler.
> > 
> > Take both of these erratas into account.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/pwm/pwm-atmel-hlcdc.c | 28 +++++++++++++++++++++++-----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> > index eaf8b12..405f8a5 100644
> > --- a/drivers/pwm/pwm-atmel-hlcdc.c
> > +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> > @@ -36,6 +36,8 @@ struct atmel_hlcdc_pwm {
> >  	struct pwm_chip chip;
> >  	struct atmel_hlcdc *hlcdc;
> >  	struct clk *cur_clk;
> > +	bool slow_clk_errata;
> > +	bool div1_clk_errata;
> >  };
> >  
> >  static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
> > @@ -56,20 +58,28 @@ static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
> >  	u32 pwmcfg;
> >  	int pres;
> >  
> > -	clk_freq = clk_get_rate(new_clk);
> > -	clk_period_ns = (u64)NSEC_PER_SEC * 256;
> > -	do_div(clk_period_ns, clk_freq);
> > +	if (!chip->slow_clk_errata) {
> > +		clk_freq = clk_get_rate(new_clk);
> > +		clk_period_ns = (u64)NSEC_PER_SEC * 256;
> > +		do_div(clk_period_ns, clk_freq);
> > +	}
> >  
> > -	if (clk_period_ns > period_ns) {
> > +	/* Errata: cannot use slow clk on some IP revisions */
> > +	if (chip->slow_clk_errata || clk_period_ns > period_ns) {
> >  		new_clk = hlcdc->sys_clk;
> >  		clk_freq = clk_get_rate(new_clk);
> >  		clk_period_ns = (u64)NSEC_PER_SEC * 256;
> >  		do_div(clk_period_ns, clk_freq);
> >  	}
> >  
> > -	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++)
> > +	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
> > +		/* Errata: cannot divide by 1 on some IP revisions */
> > +		if (!pres && chip->div1_clk_errata)
> > +			continue;
> > +
> >  		if ((clk_period_ns << pres) >= period_ns)
> >  			break;
> > +	}
> >  
> >  	if (pres > ATMEL_HLCDC_PWMPS_MAX)
> >  		return -EINVAL;
> > @@ -204,6 +214,14 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (of_device_is_compatible(dev->parent->of_node,
> > +				    "atmel,sama5d3-hlcdc"))
> > +		chip->div1_clk_errata = true;
> > +
> > +	if (of_device_is_compatible(dev->parent->of_node,
> > +				    "atmel,at91sam9x5-hlcdc"))
> > +		chip->slow_clk_errata = true;
> 
> Generally I'd prefer this to be done as "SoC data", where the idea is to
> not rely on these checks at probe time (because they don't scale very
> well in the long term). Somewhat like the following:
> 
> 	struct atmel_hlcdc_soc {
> 		bool slow_clk_errata;
> 		bool div1_clk_errata;
> 	};
> 
> 	static const struct atmel_hlcdc_soc sama5d3_hlcdc = {
> 		.slow_clk_errata = false,
> 		.div1_clk_errata = true,
> 	};
> 
> 	static const struct atmel_hlcdc_soc at91sam9x5_hlcdc = {
> 		.slow_clk_errata = true,
> 		.div1_clk_errata = false,
> 	};
> 
> Then put those into a struct of_device_id table and do the matching
> using of_match_device(). Then you can simply do something like:
> 
> 	match = of_match_device(dev->parent, atmel_hlcdc_matches);
> 	if (match)
> 		chip->soc = match->data;

I'm perfectly with using device_id data field to achieve that.
I'll change that for my next version.

> 
> And then check on the fields set therein. This works optimally if the
> device isn't a subdevice because you have most of that code anyway. In
> this particular case the lookup needs to happen on the parent, which
> isn't so nice.
> 
> I'm willing to let this go for now, but if this list is going to grow
> I'll request that it be done differently so that .probe() doesn't need
> to be cluttered.
> 
> Also I wonder if it would be better to just add these new compatibles to
> the PWM block binding, too, since it's obviously the IP blocks that have
> these errata rather than the HLCDC block. So technically I think you'd
> have to make the driver support atmel,at91sam9x5-hlcdc-pwm and
> atmel,sama5d3-hlcdc-pwm anyway, and then you could just as well move to
> the above matching and SoC data.

Well, actually I did it because the datasheet describe the HLCDC IP
not the HLCDC-PWM and HLCDC-DC IPs (I mean, IP revision is attached to
the HLCDC block, not the HLCDC-PWM and HLCDC-DC sub devices).

Anyway, I'm okay to move MFD compatible string to the sub-device nodes,
but if I do so, I'm not sure it makes sense to have a specific
compatible string for the parent device ("atmel-hlcdc" would do the
job).

Another point in favor of the "one compatible string for all sub-device
revisions" is that it limits the number of mfd cells declared in the MFD
driver.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com