[2/5] pwm: rcar: Add support "atomic" API
diff mbox series

Message ID 1544171373-29618-3-git-send-email-yoshihiro.shimoda.uh@renesas.com
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series
  • pwm: rcar: Add support "atomic" API and workaround
Related show

Commit Message

Yoshihiro Shimoda Dec. 7, 2018, 8:29 a.m. UTC
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(+)

Comments

Uwe Kleine-König Dec. 7, 2018, 9:07 a.m. UTC | #1
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
Geert Uytterhoeven Dec. 7, 2018, 9:57 a.m. UTC | #2
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
Uwe Kleine-König Dec. 7, 2018, 10:45 a.m. UTC | #3
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
Yoshihiro Shimoda Dec. 10, 2018, 4:58 a.m. UTC | #4
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/  |

Patch
diff mbox series

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,
 };