| Message ID | 1544171373-29618-3-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pwm: rcar: Add support "atomic" API and workaround | expand |
Hello, On Fri, Dec 07, 2018 at 05:29:30PM +0900, Yoshihiro Shimoda wrote: > This patch adds support for "atomic" API. Behavior is the same as > when using legacy APIs. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/pwm/pwm-rcar.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c > index 9cf4567..a5ea0f3 100644 > --- a/drivers/pwm/pwm-rcar.c > +++ b/drivers/pwm/pwm-rcar.c > @@ -200,12 +200,44 @@ static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR); > } > > +static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip); > + int div, ret; > + > + div = rcar_pwm_get_clock_division(rp, state->period); > + if (div < 0) > + return div; > + > + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR); > + > + rcar_pwm_calc_counter(rp, div, state->duty_cycle, state->period); > + ret = rcar_pwm_set_counter(rp); > + if (!ret) > + rcar_pwm_set_clock_control(rp, div); > + > + /* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */ > + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR); > + > + if (!ret && state->enabled) > + ret = rcar_pwm_enable(chip, pwm); > + > + if (!state->enabled) { > + rcar_pwm_disable(chip, pwm); > + ret = 0; > + } > + > + return ret; state->polarity isn't used here which is a bug I think. Is the documentation for this hardware publically available? If the pwm runs at say 30% duty cycle and then pwm_apply is called with .period = 1000, .duty_cycle = 600, .enabled = false, can it happen that for some time a duty cycle of 60% is provided? If so, that's a bug. Out of interest: How does the output behave if you disable the hardware? Does it give a 0, or high-z? Best regards Uwe
Hi Uwe, On Fri, Dec 7, 2018 at 10:08 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Fri, Dec 07, 2018 at 05:29:30PM +0900, Yoshihiro Shimoda wrote: > > This patch adds support for "atomic" API. Behavior is the same as > > when using legacy APIs. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/pwm/pwm-rcar.c | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c > > index 9cf4567..a5ea0f3 100644 > > --- a/drivers/pwm/pwm-rcar.c > > +++ b/drivers/pwm/pwm-rcar.c > > @@ -200,12 +200,44 @@ static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > > rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR); > > } > > > > +static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip); > > + int div, ret; > > + > > + div = rcar_pwm_get_clock_division(rp, state->period); > > + if (div < 0) > > + return div; > > + > > + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR); > > + > > + rcar_pwm_calc_counter(rp, div, state->duty_cycle, state->period); > > + ret = rcar_pwm_set_counter(rp); > > + if (!ret) > > + rcar_pwm_set_clock_control(rp, div); > > + > > + /* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */ > > + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR); > > + > > + if (!ret && state->enabled) > > + ret = rcar_pwm_enable(chip, pwm); > > + > > + if (!state->enabled) { > > + rcar_pwm_disable(chip, pwm); > > + ret = 0; > > + } > > + > > + return ret; > > state->polarity isn't used here which is a bug I think. > > Is the documentation for this hardware publically available? Please check Section 59 of the "User's Manual: Hardware" at https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hello, On Fri, Dec 07, 2018 at 10:57:48AM +0100, Geert Uytterhoeven wrote: > > Is the documentation for this hardware publically available? > > Please check Section 59 of the "User's Manual: Hardware" at > https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html Thanks. So the ugly things here are: a) if the pwm is stopped (PWMCR.ENO = 0) the output is set to high after completion of the currently running period. b) setting a duty_cycle of 0 is forbidden both are bad. A workaround for both is implementation of something similar to the switch to a gpio suggested by Michal for imx. But this cannot be done reliably because the current period's end isn't observable. Alternatively a confirmation from the Renesas engineers that PWMCNT.PHO can be set to 0 with the intended effect despite being forbidden in the reference manual would be great. Did someone with access to such hardware test what happens if the PHO field is set to 0? Maybe the forbidden value is just a wrong copy&paste from the CYCO field? I think it would be a good idea to add the link to the documentation into a comment at the top of the driver. @Thierry: Given that nobody seems to have an overview about the features and ugly implementation details of all the PWMs, what about documenting them in the driver files in a greppable way. For the rcar driver something like: - duty-counter-bits: 10 - period-counter-bits: 10 - hardware-polarity-support: false - uglyness: - OUTPUT-ACTIVE-ON-DISABLE - NO-ZERO-DUTY-CYCLE Best regards Uwe
Hi Uwe, > From: Uwe Kleine-König, Sent: Friday, December 7, 2018 7:46 PM > > Hello, > > On Fri, Dec 07, 2018 at 10:57:48AM +0100, Geert Uytterhoeven wrote: > > > Is the documentation for this hardware publically available? > > > > Please check Section 59 of the "User's Manual: Hardware" at > > https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html > > Thanks. > > So the ugly things here are: > > a) if the pwm is stopped (PWMCR.ENO = 0) the output is set to high after > completion of the currently running period. > b) setting a duty_cycle of 0 is forbidden Thank you for checking the hardware specification. > both are bad. A workaround for both is implementation of something > similar to the switch to a gpio suggested by Michal for imx. But this > cannot be done reliably because the current period's end isn't > observable. I agree. This R-Car PWM needs special handling when the driver disable the PWM. > Alternatively a confirmation from the Renesas engineers that PWMCNT.PHO > can be set to 0 with the intended effect despite being forbidden in the > reference manual would be great. Did someone with access to such > hardware test what happens if the PHO field is set to 0? Maybe the > forbidden value is just a wrong copy&paste from the CYCO field? I'm asking HW guy about this specification now. # According to the state machine in the manual, it seems the PWM cannot output # low level when the PWM is enabled though... > I think it would be a good idea to add the link to the documentation > into a comment at the top of the driver. I think so. Best regards, Yoshihiro Shimoda > @Thierry: Given that nobody seems to have an overview about the features > and ugly implementation details of all the PWMs, what about documenting > them in the driver files in a greppable way. For the rcar driver > something like: > > - duty-counter-bits: 10 > - period-counter-bits: 10 > - hardware-polarity-support: false > - uglyness: > - OUTPUT-ACTIVE-ON-DISABLE > - NO-ZERO-DUTY-CYCLE > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c index 9cf4567..a5ea0f3 100644 --- a/drivers/pwm/pwm-rcar.c +++ b/drivers/pwm/pwm-rcar.c @@ -200,12 +200,44 @@ static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR); } +static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip); + int div, ret; + + div = rcar_pwm_get_clock_division(rp, state->period); + if (div < 0) + return div; + + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR); + + rcar_pwm_calc_counter(rp, div, state->duty_cycle, state->period); + ret = rcar_pwm_set_counter(rp); + if (!ret) + rcar_pwm_set_clock_control(rp, div); + + /* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */ + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR); + + if (!ret && state->enabled) + ret = rcar_pwm_enable(chip, pwm); + + if (!state->enabled) { + rcar_pwm_disable(chip, pwm); + ret = 0; + } + + return ret; +} + static const struct pwm_ops rcar_pwm_ops = { .request = rcar_pwm_request, .free = rcar_pwm_free, .config = rcar_pwm_config, .enable = rcar_pwm_enable, .disable = rcar_pwm_disable, + .apply = rcar_pwm_apply, .owner = THIS_MODULE, };
This patch adds support for "atomic" API. Behavior is the same as when using legacy APIs. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pwm/pwm-rcar.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)