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

[PATCH v2] 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>
---
Hi Thierry,

I've addressed the "erratas stored in of_device_id data" part, but still
haven't modified the compatible strings of the HLCDC subdevices.

Please let me know if you really want to handle erratas through pwm
compatibles instead of parent device compatibles.

Regards,

Boris

Changes since v1:
- use data field in of_device_id to attach erratas to an IP revision

 drivers/pwm/pwm-atmel-hlcdc.c | 50 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
index eaf8b12..e7c4400 100644
--- a/drivers/pwm/pwm-atmel-hlcdc.c
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -32,10 +32,16 @@
 #define ATMEL_HLCDC_PWMPS_MAX		0x6
 #define ATMEL_HLCDC_PWMPS(x)		((x) & ATMEL_HLCDC_PWMPS_MASK)
 
+struct atmel_hlcdc_pwm_erratas {
+	bool slow_clk_errata;
+	bool div1_clk_errata;
+};
+
 struct atmel_hlcdc_pwm {
 	struct pwm_chip chip;
 	struct atmel_hlcdc *hlcdc;
 	struct clk *cur_clk;
+	const struct atmel_hlcdc_pwm_erratas *erratas;
 };
 
 static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
@@ -56,20 +62,29 @@ 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->erratas || !chip->erratas->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->erratas && chip->erratas->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->erratas && chip->erratas->div1_clk_errata)
+			continue;
+
 		if ((clk_period_ns << pres) >= period_ns)
 			break;
+	}
 
 	if (pres > ATMEL_HLCDC_PWMPS_MAX)
 		return -EINVAL;
@@ -187,8 +202,29 @@ static const struct pwm_ops atmel_hlcdc_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_at91sam9x5_erratas = {
+	.slow_clk_errata = true,
+};
+
+const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_sama5d3_erratas = {
+	.div1_clk_errata = true,
+};
+
+static const struct of_device_id atmel_hlcdc_dt_ids[] = {
+	{
+		.compatible = "atmel,at91sam9x5-hlcdc",
+		.data = &atmel_hlcdc_pwm_at91sam9x5_erratas,
+	},
+	{
+		.compatible = "atmel,sama5d3-hlcdc",
+		.data = &atmel_hlcdc_pwm_sama5d3_erratas,
+	},
+	{ /* sentinel */ },
+};
+
 static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
 	struct device *dev = &pdev->dev;
 	struct atmel_hlcdc_pwm *chip;
 	struct atmel_hlcdc *hlcdc;
@@ -204,6 +240,10 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	match = of_match_node(atmel_hlcdc_dt_ids, dev->parent->of_node);
+	if (match)
+		chip->erratas = match->data;
+
 	chip->hlcdc = hlcdc;
 	chip->chip.ops = &atmel_hlcdc_pwm_ops;
 	chip->chip.dev = dev;
-- 
1.9.1

[PATCH v2] 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>
---
Hi Thierry,

I've addressed the "erratas stored in of_device_id data" part, but still
haven't modified the compatible strings of the HLCDC subdevices.

Please let me know if you really want to handle erratas through pwm
compatibles instead of parent device compatibles.

Regards,

Boris

Changes since v1:
- use data field in of_device_id to attach erratas to an IP revision

 drivers/pwm/pwm-atmel-hlcdc.c | 50 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
index eaf8b12..e7c4400 100644
--- a/drivers/pwm/pwm-atmel-hlcdc.c
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -32,10 +32,16 @@
 #define ATMEL_HLCDC_PWMPS_MAX		0x6
 #define ATMEL_HLCDC_PWMPS(x)		((x) & ATMEL_HLCDC_PWMPS_MASK)
 
+struct atmel_hlcdc_pwm_erratas {
+	bool slow_clk_errata;
+	bool div1_clk_errata;
+};
+
 struct atmel_hlcdc_pwm {
 	struct pwm_chip chip;
 	struct atmel_hlcdc *hlcdc;
 	struct clk *cur_clk;
+	const struct atmel_hlcdc_pwm_erratas *erratas;
 };
 
 static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
@@ -56,20 +62,29 @@ 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->erratas || !chip->erratas->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->erratas && chip->erratas->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->erratas && chip->erratas->div1_clk_errata)
+			continue;
+
 		if ((clk_period_ns << pres) >= period_ns)
 			break;
+	}
 
 	if (pres > ATMEL_HLCDC_PWMPS_MAX)
 		return -EINVAL;
@@ -187,8 +202,29 @@ static const struct pwm_ops atmel_hlcdc_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_at91sam9x5_erratas = {
+	.slow_clk_errata = true,
+};
+
+const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_sama5d3_erratas = {
+	.div1_clk_errata = true,
+};
+
+static const struct of_device_id atmel_hlcdc_dt_ids[] = {
+	{
+		.compatible = "atmel,at91sam9x5-hlcdc",
+		.data = &atmel_hlcdc_pwm_at91sam9x5_erratas,
+	},
+	{
+		.compatible = "atmel,sama5d3-hlcdc",
+		.data = &atmel_hlcdc_pwm_sama5d3_erratas,
+	},
+	{ /* sentinel */ },
+};
+
 static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
 	struct device *dev = &pdev->dev;
 	struct atmel_hlcdc_pwm *chip;
 	struct atmel_hlcdc *hlcdc;
@@ -204,6 +240,10 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	match = of_match_node(atmel_hlcdc_dt_ids, dev->parent->of_node);
+	if (match)
+		chip->erratas = match->data;
+
 	chip->hlcdc = hlcdc;
 	chip->chip.ops = &atmel_hlcdc_pwm_ops;
 	chip->chip.dev = dev;
-- 
1.9.1

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

[Thierry Reding]

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

On Wed, Nov 19, 2014 at 03:33:09PM +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>
> ---
> Hi Thierry,
> 
> I've addressed the "erratas stored in of_device_id data" part, but still
> haven't modified the compatible strings of the HLCDC subdevices.
> 
> Please let me know if you really want to handle erratas through pwm
> compatibles instead of parent device compatibles.
> 
> Regards,
> 
> Boris
> 
> Changes since v1:
> - use data field in of_device_id to attach erratas to an IP revision
> 
>  drivers/pwm/pwm-atmel-hlcdc.c | 50 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)

I applied this, but this was really much more difficult than I would've
wanted. Since the MFD driver hasn't been merged into Linus' tree yet I
wasn't able to actually build test this driver at all without manually
pulling in the patches that add the MFD support. I went through this
trouble this time because it's what we had agreed upon, but for the
record, next time I'll request a stable branch that I can pull into the
PWM tree to resolve this kind of dependency. Or patches will have to
wait until the next merge window. I don't want to have to jump through
hoops just to make sure the code in my tree actually compiles.

Oh, and it's good that I do compile tests because...

> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
[...]
> +const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_at91sam9x5_erratas = {
> +	.slow_clk_errata = true,
> +};
> +
> +const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_sama5d3_erratas = {
> +	.div1_clk_errata = true,
> +};

... these actually should be static.

Also I took the liberty of substituting "erratum" for "errata" and
"errata" for "erratas".

Thierry

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

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

[Thierry Reding]

On Wed, Nov 19, 2014 at 03:33:09PM +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>
> ---
> Hi Thierry,
> 
> I've addressed the "erratas stored in of_device_id data" part, but still
> haven't modified the compatible strings of the HLCDC subdevices.
> 
> Please let me know if you really want to handle erratas through pwm
> compatibles instead of parent device compatibles.
> 
> Regards,
> 
> Boris
> 
> Changes since v1:
> - use data field in of_device_id to attach erratas to an IP revision
> 
>  drivers/pwm/pwm-atmel-hlcdc.c | 50 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)

I applied this, but this was really much more difficult than I would've
wanted. Since the MFD driver hasn't been merged into Linus' tree yet I
wasn't able to actually build test this driver at all without manually
pulling in the patches that add the MFD support. I went through this
trouble this time because it's what we had agreed upon, but for the
record, next time I'll request a stable branch that I can pull into the
PWM tree to resolve this kind of dependency. Or patches will have to
wait until the next merge window. I don't want to have to jump through
hoops just to make sure the code in my tree actually compiles.

Oh, and it's good that I do compile tests because...

> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
[...]
> +const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_at91sam9x5_erratas = {
> +	.slow_clk_errata = true,
> +};
> +
> +const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_sama5d3_erratas = {
> +	.div1_clk_errata = true,
> +};

... these actually should be static.

Also I took the liberty of substituting "erratum" for "errata" and
"errata" for "erratas".

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141204/fb2eedf3/attachment-0001.sig>

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

[Boris Brezillon]

Hi Thierry

On Thu, 4 Dec 2014 14:12:31 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Nov 19, 2014 at 03:33:09PM +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>
> > ---
> > Hi Thierry,
> > 
> > I've addressed the "erratas stored in of_device_id data" part, but still
> > haven't modified the compatible strings of the HLCDC subdevices.
> > 
> > Please let me know if you really want to handle erratas through pwm
> > compatibles instead of parent device compatibles.
> > 
> > Regards,
> > 
> > Boris
> > 
> > Changes since v1:
> > - use data field in of_device_id to attach erratas to an IP revision
> > 
> >  drivers/pwm/pwm-atmel-hlcdc.c | 50 ++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> I applied this, but this was really much more difficult than I would've
> wanted. Since the MFD driver hasn't been merged into Linus' tree yet I
> wasn't able to actually build test this driver at all without manually
> pulling in the patches that add the MFD support. I went through this
> trouble this time because it's what we had agreed upon, but for the
> record, next time I'll request a stable branch that I can pull into the
> PWM tree to resolve this kind of dependency. Or patches will have to
> wait until the next merge window. I don't want to have to jump through
> hoops just to make sure the code in my tree actually compiles.

Thanks for doing that.
Just for my understanding, am I the one responsible for providing this
stable branch, or were you expecting Lee to provide it ?

> 
> Oh, and it's good that I do compile tests because...
> 
> > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> [...]
> > +const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_at91sam9x5_erratas = {
> > +	.slow_clk_errata = true,
> > +};
> > +
> > +const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_sama5d3_erratas = {
> > +	.div1_clk_errata = true,
> > +};
> 
> ... these actually should be static.
> 
> Also I took the liberty of substituting "erratum" for "errata" and
> "errata" for "erratas".

Indeed, thanks for fixing those problems.

Best Regards,

Boris

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

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

[Boris Brezillon]

Hi Thierry

On Thu, 4 Dec 2014 14:12:31 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Nov 19, 2014 at 03:33:09PM +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>
> > ---
> > Hi Thierry,
> > 
> > I've addressed the "erratas stored in of_device_id data" part, but still
> > haven't modified the compatible strings of the HLCDC subdevices.
> > 
> > Please let me know if you really want to handle erratas through pwm
> > compatibles instead of parent device compatibles.
> > 
> > Regards,
> > 
> > Boris
> > 
> > Changes since v1:
> > - use data field in of_device_id to attach erratas to an IP revision
> > 
> >  drivers/pwm/pwm-atmel-hlcdc.c | 50 ++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> I applied this, but this was really much more difficult than I would've
> wanted. Since the MFD driver hasn't been merged into Linus' tree yet I
> wasn't able to actually build test this driver at all without manually
> pulling in the patches that add the MFD support. I went through this
> trouble this time because it's what we had agreed upon, but for the
> record, next time I'll request a stable branch that I can pull into the
> PWM tree to resolve this kind of dependency. Or patches will have to
> wait until the next merge window. I don't want to have to jump through
> hoops just to make sure the code in my tree actually compiles.

Thanks for doing that.
Just for my understanding, am I the one responsible for providing this
stable branch, or were you expecting Lee to provide it ?

> 
> Oh, and it's good that I do compile tests because...
> 
> > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> [...]
> > +const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_at91sam9x5_erratas = {
> > +	.slow_clk_errata = true,
> > +};
> > +
> > +const struct atmel_hlcdc_pwm_erratas atmel_hlcdc_pwm_sama5d3_erratas = {
> > +	.div1_clk_errata = true,
> > +};
> 
> ... these actually should be static.
> 
> Also I took the liberty of substituting "erratum" for "errata" and
> "errata" for "erratas".

Indeed, thanks for fixing those problems.

Best Regards,

Boris

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