| Message ID | 1416314453-28597-1-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
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; >
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
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
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;
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(-)