[PATCH 0/5] pwm: rcar: Add support "atomic" API and workaround

[PATCH 0/5] pwm: rcar: Add support "atomic" API and workaround

[Yoshihiro Shimoda]

This patch adds support for PWM "atomic" API.

This patch also adds a workaround to output "pseudo" low level.
Otherwise, the PWM backlight driver doesn't work correctly, especially
it cannot output maximum brightness actually.

Yoshihiro Shimoda (5):
  pwm: rcar: add rcar_pwm_calc_counter() to calculate PWMCNT value only
  pwm: rcar: Add support "atomic" API
  pwm: rcar: Use "atomic" API on rcar_pwm_resume()
  pwm: rcar: remove legacy APIs
  pwm: rcar: add workaround to output "pseudo" low level

 drivers/pwm/pwm-rcar.c | 108 ++++++++++++++++++++++++++++---------------------
 1 file changed, 62 insertions(+), 46 deletions(-)

-- 
1.9.1

[PATCH 1/5] pwm: rcar: add rcar_pwm_calc_counter() to calculate PWMCNT value only

[Yoshihiro Shimoda]

To add a workaround in the future, this patch adds a function
rcar_pwm_calc_counter() from rcar_pwm_set_counter().
The rcar_pwm_calc_counter() calculates PWMCNT value only, and
rcar_pwm_set_counter() set the PWMCNT register by using
the calculated value. No change in behavior.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/pwm-rcar.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index a41812f..9cf4567 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -36,6 +36,7 @@ struct rcar_pwm_chip {
 	struct pwm_chip chip;
 	void __iomem *base;
 	struct clk *clk;
+	u32 pwmcnt;
 };
 
 static inline struct rcar_pwm_chip *to_rcar_pwm_chip(struct pwm_chip *chip)
@@ -102,8 +103,8 @@ static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp,
 	rcar_pwm_write(rp, value, RCAR_PWMCR);
 }
 
-static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
-				int period_ns)
+static void rcar_pwm_calc_counter(struct rcar_pwm_chip *rp, int div,
+				  int duty_ns, int period_ns)
 {
 	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
 	unsigned long clk_rate = clk_get_rate(rp->clk);
@@ -120,11 +121,17 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
 	do_div(tmp, one_cycle);
 	ph = tmp & RCAR_PWMCNT_PH0_MASK;
 
+	rp->pwmcnt = cyc | ph;
+}
+
+static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp)
+{
 	/* Avoid prohibited setting */
-	if (cyc == 0 || ph == 0)
+	if ((rp->pwmcnt & RCAR_PWMCNT_CYC0_MASK) == 0 ||
+	    (rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
 		return -EINVAL;
 
-	rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
+	rcar_pwm_write(rp, rp->pwmcnt, RCAR_PWMCNT);
 
 	return 0;
 }
@@ -159,7 +166,8 @@ static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
 
-	ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
+	rcar_pwm_calc_counter(rp, div, duty_ns, period_ns);
+	ret = rcar_pwm_set_counter(rp);
 	if (!ret)
 		rcar_pwm_set_clock_control(rp, div);
 
-- 
1.9.1

Re: [PATCH 1/5] pwm: rcar: add rcar_pwm_calc_counter() to calculate PWMCNT value only

[Uwe Kleine-König]

Hello,

On Fri, Dec 07, 2018 at 05:29:29PM +0900, Yoshihiro Shimoda wrote:
> -static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
> -				int period_ns)
> +static void rcar_pwm_calc_counter(struct rcar_pwm_chip *rp, int div,
> +				  int duty_ns, int period_ns)
>  {
>  	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
>  	unsigned long clk_rate = clk_get_rate(rp->clk);
> @@ -120,11 +121,17 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
>  	do_div(tmp, one_cycle);
>  	ph = tmp & RCAR_PWMCNT_PH0_MASK;
>  
> +	rp->pwmcnt = cyc | ph;

Wouldn't it be more natural to let rcar_pwm_calc_counter return cyc | ph
and then ...

> +}
> +
> +static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp)
> +{
>  	/* Avoid prohibited setting */
> -	if (cyc == 0 || ph == 0)
> +	if ((rp->pwmcnt & RCAR_PWMCNT_CYC0_MASK) == 0 ||
> +	    (rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
>  		return -EINVAL;
>  
> -	rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> +	rcar_pwm_write(rp, rp->pwmcnt, RCAR_PWMCNT);
>  
>  	return 0;
>  }
> @@ -159,7 +166,8 @@ static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
>  
> -	ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
> +	rcar_pwm_calc_counter(rp, div, duty_ns, period_ns);
> +	ret = rcar_pwm_set_counter(rp);
>  	if (!ret)
>  		rcar_pwm_set_clock_control(rp, div);

... pass this value to rcar_pwm_set_counter as u32. (Or pass div,
duty_ns and period_ns to rcar_pwm_set_counter and let this then call
rcar_pwm_calc_counter. Then you don't need a new member in the struct
rcar_pwm_chip.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/5] pwm: rcar: add rcar_pwm_calc_counter() to calculate PWMCNT value only

[Uwe Kleine-König]

Hello,

On Fri, Dec 07, 2018 at 05:29:29PM +0900, Yoshihiro Shimoda wrote:
> -static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
> -				int period_ns)
> +static void rcar_pwm_calc_counter(struct rcar_pwm_chip *rp, int div,
> +				  int duty_ns, int period_ns)
>  {
>  	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
>  	unsigned long clk_rate = clk_get_rate(rp->clk);
> @@ -120,11 +121,17 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
>  	do_div(tmp, one_cycle);
>  	ph = tmp & RCAR_PWMCNT_PH0_MASK;
>  
> +	rp->pwmcnt = cyc | ph;

Wouldn't it be more natural to let rcar_pwm_calc_counter return cyc | ph
and then ...

> +}
> +
> +static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp)
> +{
>  	/* Avoid prohibited setting */
> -	if (cyc == 0 || ph == 0)
> +	if ((rp->pwmcnt & RCAR_PWMCNT_CYC0_MASK) == 0 ||
> +	    (rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
>  		return -EINVAL;
>  
> -	rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> +	rcar_pwm_write(rp, rp->pwmcnt, RCAR_PWMCNT);
>  
>  	return 0;
>  }
> @@ -159,7 +166,8 @@ static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
>  
> -	ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
> +	rcar_pwm_calc_counter(rp, div, duty_ns, period_ns);
> +	ret = rcar_pwm_set_counter(rp);
>  	if (!ret)
>  		rcar_pwm_set_clock_control(rp, div);

... pass this value to rcar_pwm_set_counter as u32. (Or pass div,
duty_ns and period_ns to rcar_pwm_set_counter and let this then call
rcar_pwm_calc_counter. Then you don't need a new member in the struct
rcar_pwm_chip.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/5] pwm: rcar: Add support "atomic" API

[Yoshihiro Shimoda]

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

Re: [PATCH 2/5] pwm: rcar: Add support "atomic" API

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/5] pwm: rcar: Add support "atomic" API

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/5] pwm: rcar: Add support "atomic" API

[Geert Uytterhoeven]

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

Re: [PATCH 2/5] pwm: rcar: Add support "atomic" API

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/5] pwm: rcar: Add support "atomic" API

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 2/5] pwm: rcar: Add support "atomic" API

[Yoshihiro Shimoda]

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 3/5] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Yoshihiro Shimoda]

To remove legacy API related functions in the future, this patch
uses "atomic" related function instead. No change in behavior.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/pwm-rcar.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index a5ea0f3..1bcb662 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -319,18 +319,16 @@ static int rcar_pwm_suspend(struct device *dev)
 static int rcar_pwm_resume(struct device *dev)
 {
 	struct pwm_device *pwm = rcar_pwm_dev_to_pwm_dev(dev);
+	struct pwm_state state;
 
 	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
 		return 0;
 
 	pm_runtime_get_sync(dev);
 
-	rcar_pwm_config(pwm->chip, pwm, pwm->state.duty_cycle,
-			pwm->state.period);
-	if (pwm_is_enabled(pwm))
-		rcar_pwm_enable(pwm->chip, pwm);
+	pwm_get_state(pwm, &state);
 
-	return 0;
+	return rcar_pwm_apply(pwm->chip, pwm, &state);
 }
 #endif /* CONFIG_PM_SLEEP */
 static SIMPLE_DEV_PM_OPS(rcar_pwm_pm_ops, rcar_pwm_suspend, rcar_pwm_resume);
-- 
1.9.1

[PATCH 4/5] pwm: rcar: remove legacy APIs

[Yoshihiro Shimoda]

This patch removes legacy APIs. Since rcar_pwm_{en,dis}able()
functions are reused on "atomic" API, this patch changes
the arguments of these functions. No change in behavior.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/pwm-rcar.c | 45 ++++-----------------------------------------
 1 file changed, 4 insertions(+), 41 deletions(-)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index 1bcb662..e479b6a 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -146,40 +146,8 @@ static void rcar_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	pm_runtime_put(chip->dev);
 }
 
-static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			   int duty_ns, int period_ns)
+static int rcar_pwm_enable(struct rcar_pwm_chip *rp)
 {
-	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
-	int div, ret;
-
-	div = rcar_pwm_get_clock_division(rp, period_ns);
-	if (div < 0)
-		return div;
-
-	/*
-	 * Let the core driver set pwm->period if disabled and duty_ns == 0.
-	 * But, this driver should prevent to set the new duty_ns if current
-	 * duty_cycle is not set
-	 */
-	if (!pwm_is_enabled(pwm) && !duty_ns && !pwm->state.duty_cycle)
-		return 0;
-
-	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
-
-	rcar_pwm_calc_counter(rp, div, duty_ns, period_ns);
-	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);
-
-	return ret;
-}
-
-static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
 	u32 value;
 
 	/* Don't enable the PWM device if CYC0 or PH0 is 0 */
@@ -193,10 +161,8 @@ static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	return 0;
 }
 
-static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
 {
-	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
-
 	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
 }
 
@@ -221,10 +187,10 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
 
 	if (!ret && state->enabled)
-		ret = rcar_pwm_enable(chip, pwm);
+		ret = rcar_pwm_enable(rp);
 
 	if (!state->enabled) {
-		rcar_pwm_disable(chip, pwm);
+		rcar_pwm_disable(rp);
 		ret = 0;
 	}
 
@@ -234,9 +200,6 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 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,
 };
-- 
1.9.1

[PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

This PWM Timer cannot output low because setting 0x000 is prohibited
on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
the prohibited, this patch adds a workaround function to change
the value from 0 to 1 as pseudo low level.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/pwm-rcar.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index e479b6a..888cb37 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -166,6 +166,20 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
 	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
 }
 
+static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
+{
+	/*
+	 * This PWM Timer cannot output low because setting 0x000 is
+	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
+	 * the prohibited, this function changes the value from 0 to 1 as
+	 * pseudo low level.
+	 *
+	 * TODO: Add GPIO handling to output low level.
+	 */
+	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
+		rp->pwmcnt |= 1;
+}
+
 static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			  struct pwm_state *state)
 {
@@ -179,6 +193,7 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
 
 	rcar_pwm_calc_counter(rp, div, state->duty_cycle, state->period);
+	rcar_pwm_workaround_output_low(rp);
 	ret = rcar_pwm_set_counter(rp);
 	if (!ret)
 		rcar_pwm_set_clock_control(rp, div);
-- 
1.9.1

Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Uwe Kleine-König]

On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> This PWM Timer cannot output low because setting 0x000 is prohibited
> on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> the prohibited, this patch adds a workaround function to change
> the value from 0 to 1 as pseudo low level.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pwm/pwm-rcar.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index e479b6a..888cb37 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -166,6 +166,20 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
>  	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
>  }
>  
> +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> +{
> +	/*
> +	 * This PWM Timer cannot output low because setting 0x000 is
> +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> +	 * the prohibited, this function changes the value from 0 to 1 as
> +	 * pseudo low level.
> +	 *
> +	 * TODO: Add GPIO handling to output low level.
> +	 */
> +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> +		rp->pwmcnt |= 1;

In my eyes this is too broken to do. Not sure I have the complete
picture, but given a small period (say 2) this 1 cycle might result in
50 % duty cycle. Depending on how the hardware behaves if you disable
it, better do this instead.

Are you aware of the series adding such gpio support to the imx driver?

@Thierry: So there are three drivers now that could benefit for a
generic approach.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Uwe Kleine-König]

On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> This PWM Timer cannot output low because setting 0x000 is prohibited
> on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> the prohibited, this patch adds a workaround function to change
> the value from 0 to 1 as pseudo low level.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pwm/pwm-rcar.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index e479b6a..888cb37 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -166,6 +166,20 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
>  	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
>  }
>  
> +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> +{
> +	/*
> +	 * This PWM Timer cannot output low because setting 0x000 is
> +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> +	 * the prohibited, this function changes the value from 0 to 1 as
> +	 * pseudo low level.
> +	 *
> +	 * TODO: Add GPIO handling to output low level.
> +	 */
> +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> +		rp->pwmcnt |= 1;

In my eyes this is too broken to do. Not sure I have the complete
picture, but given a small period (say 2) this 1 cycle might result in
50 % duty cycle. Depending on how the hardware behaves if you disable
it, better do this instead.

Are you aware of the series adding such gpio support to the imx driver?

@Thierry: So there are three drivers now that could benefit for a
generic approach.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

Hi Uwem

Thank you for your review!

> From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> 
> On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
<snip>
> > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > +{
> > +	/*
> > +	 * This PWM Timer cannot output low because setting 0x000 is
> > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > +	 * the prohibited, this function changes the value from 0 to 1 as
> > +	 * pseudo low level.
> > +	 *
> > +	 * TODO: Add GPIO handling to output low level.
> > +	 */
> > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > +		rp->pwmcnt |= 1;
> 
> In my eyes this is too broken to do. Not sure I have the complete
> picture, but given a small period (say 2) this 1 cycle might result in
> 50 % duty cycle. Depending on how the hardware behaves if you disable
> it, better do this instead.

You're right.

> Are you aware of the series adding such gpio support to the imx driver?

I didn't know that.

> @Thierry: So there are three drivers now that could benefit for a
> generic approach.

Should I wait for Thierry's opinion whether PWM subsystem will have
a generic approach or not?

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Uwe Kleine-König]

Hello,

On Mon, Dec 10, 2018 at 04:49:17AM +0000, Yoshihiro Shimoda wrote:
> > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > 
> > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> <snip>
> > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > +{
> > > +	/*
> > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > +	 * pseudo low level.
> > > +	 *
> > > +	 * TODO: Add GPIO handling to output low level.
> > > +	 */
> > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > +		rp->pwmcnt |= 1;
> > 
> > In my eyes this is too broken to do. Not sure I have the complete
> > picture, but given a small period (say 2) this 1 cycle might result in
> > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > it, better do this instead.
> 
> You're right.

But in the meantime I learned that the pwm gets active on disable, so
this won't help.
 
> > Are you aware of the series adding such gpio support to the imx driver?
> 
> I didn't know that.
> 
> > @Thierry: So there are three drivers now that could benefit for a
> > generic approach.
> 
> Should I wait for Thierry's opinion whether PWM subsystem will have
> a generic approach or not?

Not sure how to preceed here. The needed procedure would be:

	set duty_cycle to 0%
	delay long enough to be sure the duty cycle is active
	switch to gpio
	disable the hardware

The additional blocker for rcar is that it doesn't support duty_cycle
0%.

So unless your hardware guys confirm that 0% works even though not
supported according to the hardware manual I have no good idea.

In the past I suggested to weaken the requirements after pwm_disable,
but Thierry didn't like it.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Uwe Kleine-König]

Hello,

On Mon, Dec 10, 2018 at 04:49:17AM +0000, Yoshihiro Shimoda wrote:
> > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > 
> > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> <snip>
> > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > +{
> > > +	/*
> > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > +	 * pseudo low level.
> > > +	 *
> > > +	 * TODO: Add GPIO handling to output low level.
> > > +	 */
> > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > +		rp->pwmcnt |= 1;
> > 
> > In my eyes this is too broken to do. Not sure I have the complete
> > picture, but given a small period (say 2) this 1 cycle might result in
> > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > it, better do this instead.
> 
> You're right.

But in the meantime I learned that the pwm gets active on disable, so
this won't help.
 
> > Are you aware of the series adding such gpio support to the imx driver?
> 
> I didn't know that.
> 
> > @Thierry: So there are three drivers now that could benefit for a
> > generic approach.
> 
> Should I wait for Thierry's opinion whether PWM subsystem will have
> a generic approach or not?

Not sure how to preceed here. The needed procedure would be:

	set duty_cycle to 0%
	delay long enough to be sure the duty cycle is active
	switch to gpio
	disable the hardware

The additional blocker for rcar is that it doesn't support duty_cycle
0%.

So unless your hardware guys confirm that 0% works even though not
supported according to the hardware manual I have no good idea.

In the past I suggested to weaken the requirements after pwm_disable,
but Thierry didn't like it.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

Hi Uwe,

> From: Uwe Kleine-Konig, Sent: Monday, December 10, 2018 5:11 PM
> 
> Hello,
> 
> On Mon, Dec 10, 2018 at 04:49:17AM +0000, Yoshihiro Shimoda wrote:
> > > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > >
> > > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > <snip>
> > > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > > +{
> > > > +	/*
> > > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > > +	 * pseudo low level.
> > > > +	 *
> > > > +	 * TODO: Add GPIO handling to output low level.
> > > > +	 */
> > > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > > +		rp->pwmcnt |= 1;
> > >
> > > In my eyes this is too broken to do. Not sure I have the complete
> > > picture, but given a small period (say 2) this 1 cycle might result in
> > > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > > it, better do this instead.
> >
> > You're right.
> 
> But in the meantime I learned that the pwm gets active on disable, so
> this won't help.
> 
> > > Are you aware of the series adding such gpio support to the imx driver?
> >
> > I didn't know that.
> >
> > > @Thierry: So there are three drivers now that could benefit for a
> > > generic approach.
> >
> > Should I wait for Thierry's opinion whether PWM subsystem will have
> > a generic approach or not?
> 
> Not sure how to preceed here. The needed procedure would be:
> 
> 	set duty_cycle to 0%
> 	delay long enough to be sure the duty cycle is active
> 	switch to gpio
> 	disable the hardware
> 
> The additional blocker for rcar is that it doesn't support duty_cycle
> 0%.
> 
> So unless your hardware guys confirm that 0% works even though not
> supported according to the hardware manual I have no good idea.
> 
> In the past I suggested to weaken the requirements after pwm_disable,
> but Thierry didn't like it.

I read the following discussion once:
https://patchwork.ozlabs.org/patch/959776/

I could not understand all this yet, but I think I should try to add a special gpio handling
to the pwm-rcar.c driver instead of a generic approach because as you mentioned above,
such special handling needs for the hardware.

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

Hi Uwe,

> From: Uwe Kleine-Konig, Sent: Monday, December 10, 2018 5:11 PM
> 
> Hello,
> 
> On Mon, Dec 10, 2018 at 04:49:17AM +0000, Yoshihiro Shimoda wrote:
> > > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > >
> > > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > <snip>
> > > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > > +{
> > > > +	/*
> > > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > > +	 * pseudo low level.
> > > > +	 *
> > > > +	 * TODO: Add GPIO handling to output low level.
> > > > +	 */
> > > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > > +		rp->pwmcnt |= 1;
> > >
> > > In my eyes this is too broken to do. Not sure I have the complete
> > > picture, but given a small period (say 2) this 1 cycle might result in
> > > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > > it, better do this instead.
> >
> > You're right.
> 
> But in the meantime I learned that the pwm gets active on disable, so
> this won't help.
> 
> > > Are you aware of the series adding such gpio support to the imx driver?
> >
> > I didn't know that.
> >
> > > @Thierry: So there are three drivers now that could benefit for a
> > > generic approach.
> >
> > Should I wait for Thierry's opinion whether PWM subsystem will have
> > a generic approach or not?
> 
> Not sure how to preceed here. The needed procedure would be:
> 
> 	set duty_cycle to 0%
> 	delay long enough to be sure the duty cycle is active
> 	switch to gpio
> 	disable the hardware
> 
> The additional blocker for rcar is that it doesn't support duty_cycle
> 0%.
> 
> So unless your hardware guys confirm that 0% works even though not
> supported according to the hardware manual I have no good idea.
> 
> In the past I suggested to weaken the requirements after pwm_disable,
> but Thierry didn't like it.

I read the following discussion once:
https://patchwork.ozlabs.org/patch/959776/

I could not understand all this yet, but I think I should try to add a special gpio handling
to the pwm-rcar.c driver instead of a generic approach because as you mentioned above,
such special handling needs for the hardware.

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Uwe Kleine-König]

On Wed, Dec 12, 2018 at 03:19:40AM +0000, Yoshihiro Shimoda wrote:
> Hi Uwe,
> 
> > From: Uwe Kleine-Konig, Sent: Monday, December 10, 2018 5:11 PM
> > 
> > Hello,
> > 
> > On Mon, Dec 10, 2018 at 04:49:17AM +0000, Yoshihiro Shimoda wrote:
> > > > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > > >
> > > > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > > <snip>
> > > > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > > > +{
> > > > > +	/*
> > > > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > > > +	 * pseudo low level.
> > > > > +	 *
> > > > > +	 * TODO: Add GPIO handling to output low level.
> > > > > +	 */
> > > > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > > > +		rp->pwmcnt |= 1;
> > > >
> > > > In my eyes this is too broken to do. Not sure I have the complete
> > > > picture, but given a small period (say 2) this 1 cycle might result in
> > > > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > > > it, better do this instead.
> > >
> > > You're right.
> > 
> > But in the meantime I learned that the pwm gets active on disable, so
> > this won't help.
> > 
> > > > Are you aware of the series adding such gpio support to the imx driver?
> > >
> > > I didn't know that.
> > >
> > > > @Thierry: So there are three drivers now that could benefit for a
> > > > generic approach.
> > >
> > > Should I wait for Thierry's opinion whether PWM subsystem will have
> > > a generic approach or not?
> > 
> > Not sure how to preceed here. The needed procedure would be:
> > 
> > 	set duty_cycle to 0%
> > 	delay long enough to be sure the duty cycle is active
> > 	switch to gpio
> > 	disable the hardware
> > 
> > The additional blocker for rcar is that it doesn't support duty_cycle
> > 0%.
> > 
> > So unless your hardware guys confirm that 0% works even though not
> > supported according to the hardware manual I have no good idea.
> > 
> > In the past I suggested to weaken the requirements after pwm_disable,
> > but Thierry didn't like it.
> 
> I read the following discussion once:
> https://patchwork.ozlabs.org/patch/959776/

Yeah, that's (part of) the discussion I meant. Thierry doesn't agree
though, so for now that's a dead end. My plan is to watch the pwm list
for a while to get a better picture about the different pwm
implementations because just one or two problematic cases are not
enough to justify a generic solution in the core in his eyes.
 
> I could not understand all this yet, but I think I should try to add a special gpio handling
> to the pwm-rcar.c driver instead of a generic approach because as you mentioned above,
> such special handling needs for the hardware.

Being able to set PHO to zero would be still better, so I hope you
follow up on the question to your hardware guys if this is really
forbidden. (If I had access to such hardware, I'd bluntly try what
happens.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Uwe Kleine-König]

On Wed, Dec 12, 2018 at 03:19:40AM +0000, Yoshihiro Shimoda wrote:
> Hi Uwe,
> 
> > From: Uwe Kleine-Konig, Sent: Monday, December 10, 2018 5:11 PM
> > 
> > Hello,
> > 
> > On Mon, Dec 10, 2018 at 04:49:17AM +0000, Yoshihiro Shimoda wrote:
> > > > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > > >
> > > > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > > <snip>
> > > > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > > > +{
> > > > > +	/*
> > > > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > > > +	 * pseudo low level.
> > > > > +	 *
> > > > > +	 * TODO: Add GPIO handling to output low level.
> > > > > +	 */
> > > > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > > > +		rp->pwmcnt |= 1;
> > > >
> > > > In my eyes this is too broken to do. Not sure I have the complete
> > > > picture, but given a small period (say 2) this 1 cycle might result in
> > > > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > > > it, better do this instead.
> > >
> > > You're right.
> > 
> > But in the meantime I learned that the pwm gets active on disable, so
> > this won't help.
> > 
> > > > Are you aware of the series adding such gpio support to the imx driver?
> > >
> > > I didn't know that.
> > >
> > > > @Thierry: So there are three drivers now that could benefit for a
> > > > generic approach.
> > >
> > > Should I wait for Thierry's opinion whether PWM subsystem will have
> > > a generic approach or not?
> > 
> > Not sure how to preceed here. The needed procedure would be:
> > 
> > 	set duty_cycle to 0%
> > 	delay long enough to be sure the duty cycle is active
> > 	switch to gpio
> > 	disable the hardware
> > 
> > The additional blocker for rcar is that it doesn't support duty_cycle
> > 0%.
> > 
> > So unless your hardware guys confirm that 0% works even though not
> > supported according to the hardware manual I have no good idea.
> > 
> > In the past I suggested to weaken the requirements after pwm_disable,
> > but Thierry didn't like it.
> 
> I read the following discussion once:
> https://patchwork.ozlabs.org/patch/959776/

Yeah, that's (part of) the discussion I meant. Thierry doesn't agree
though, so for now that's a dead end. My plan is to watch the pwm list
for a while to get a better picture about the different pwm
implementations because just one or two problematic cases are not
enough to justify a generic solution in the core in his eyes.
 
> I could not understand all this yet, but I think I should try to add a special gpio handling
> to the pwm-rcar.c driver instead of a generic approach because as you mentioned above,
> such special handling needs for the hardware.

Being able to set PHO to zero would be still better, so I hope you
follow up on the question to your hardware guys if this is really
forbidden. (If I had access to such hardware, I'd bluntly try what
happens.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

Hi Uwe,

> From: Uwe Kleine-König, Sent: Wednesday, December 12, 2018 4:38 PM
<snip>
> > > In the past I suggested to weaken the requirements after pwm_disable,
> > > but Thierry didn't like it.
> >
> > I read the following discussion once:
> > https://patchwork.ozlabs.org/patch/959776/
> 
> Yeah, that's (part of) the discussion I meant. Thierry doesn't agree
> though, so for now that's a dead end. My plan is to watch the pwm list
> for a while to get a better picture about the different pwm
> implementations because just one or two problematic cases are not
> enough to justify a generic solution in the core in his eyes.

I got it.

> > I could not understand all this yet, but I think I should try to add a special gpio handling
> > to the pwm-rcar.c driver instead of a generic approach because as you mentioned above,
> > such special handling needs for the hardware.
> 
> Being able to set PHO to zero would be still better, so I hope you
> follow up on the question to your hardware guys if this is really
> forbidden. (If I had access to such hardware, I'd bluntly try what
> happens.)

The hardware guy said that the output level is always high if PH0 is set to 0.
However, the PH0 bitfields means "PWM High-Level Period" so that the behavior of
the zero differs between the document and the hardware. So, we have to assume
this is really forbidden unfortunately...

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

Hi Uwe,

> From: Uwe Kleine-König, Sent: Wednesday, December 12, 2018 4:38 PM
<snip>
> > > In the past I suggested to weaken the requirements after pwm_disable,
> > > but Thierry didn't like it.
> >
> > I read the following discussion once:
> > https://patchwork.ozlabs.org/patch/959776/
> 
> Yeah, that's (part of) the discussion I meant. Thierry doesn't agree
> though, so for now that's a dead end. My plan is to watch the pwm list
> for a while to get a better picture about the different pwm
> implementations because just one or two problematic cases are not
> enough to justify a generic solution in the core in his eyes.

I got it.

> > I could not understand all this yet, but I think I should try to add a special gpio handling
> > to the pwm-rcar.c driver instead of a generic approach because as you mentioned above,
> > such special handling needs for the hardware.
> 
> Being able to set PHO to zero would be still better, so I hope you
> follow up on the question to your hardware guys if this is really
> forbidden. (If I had access to such hardware, I'd bluntly try what
> happens.)

The hardware guy said that the output level is always high if PH0 is set to 0.
However, the PH0 bitfields means "PWM High-Level Period" so that the behavior of
the zero differs between the document and the hardware. So, we have to assume
this is really forbidden unfortunately...

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

Hi Thierry, Uwe,

> From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> 
> On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > This PWM Timer cannot output low because setting 0x000 is prohibited
> > on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > the prohibited, this patch adds a workaround function to change
> > the value from 0 to 1 as pseudo low level.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pwm/pwm-rcar.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> > index e479b6a..888cb37 100644
> > --- a/drivers/pwm/pwm-rcar.c
> > +++ b/drivers/pwm/pwm-rcar.c
> > @@ -166,6 +166,20 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
> >  	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
> >  }
> >
> > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > +{
> > +	/*
> > +	 * This PWM Timer cannot output low because setting 0x000 is
> > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > +	 * the prohibited, this function changes the value from 0 to 1 as
> > +	 * pseudo low level.
> > +	 *
> > +	 * TODO: Add GPIO handling to output low level.
> > +	 */
> > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > +		rp->pwmcnt |= 1;
> 
> In my eyes this is too broken to do. Not sure I have the complete
> picture, but given a small period (say 2) this 1 cycle might result in
> 50 % duty cycle. Depending on how the hardware behaves if you disable
> it, better do this instead.

My colleague suggests that this workaround code also changes the period
as maximum (1023) to avoid 50 % duty cycle for such a case.

What do you think that this idea is acceptable for upstream? Or, should
I add gpio handling that Uwe suggested?

Best regards,
Yoshihiro Shimoda

> Are you aware of the series adding such gpio support to the imx driver?
> 
> @Thierry: So there are three drivers now that could benefit for a
> generic approach.

RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

Hi Thierry, Uwe,

> From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> 
> On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > This PWM Timer cannot output low because setting 0x000 is prohibited
> > on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > the prohibited, this patch adds a workaround function to change
> > the value from 0 to 1 as pseudo low level.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pwm/pwm-rcar.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> > index e479b6a..888cb37 100644
> > --- a/drivers/pwm/pwm-rcar.c
> > +++ b/drivers/pwm/pwm-rcar.c
> > @@ -166,6 +166,20 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
> >  	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
> >  }
> >
> > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > +{
> > +	/*
> > +	 * This PWM Timer cannot output low because setting 0x000 is
> > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > +	 * the prohibited, this function changes the value from 0 to 1 as
> > +	 * pseudo low level.
> > +	 *
> > +	 * TODO: Add GPIO handling to output low level.
> > +	 */
> > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > +		rp->pwmcnt |= 1;
> 
> In my eyes this is too broken to do. Not sure I have the complete
> picture, but given a small period (say 2) this 1 cycle might result in
> 50 % duty cycle. Depending on how the hardware behaves if you disable
> it, better do this instead.

My colleague suggests that this workaround code also changes the period
as maximum (1023) to avoid 50 % duty cycle for such a case.

What do you think that this idea is acceptable for upstream? Or, should
I add gpio handling that Uwe suggested?

Best regards,
Yoshihiro Shimoda

> Are you aware of the series adding such gpio support to the imx driver?
> 
> @Thierry: So there are three drivers now that could benefit for a
> generic approach.

Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Uwe Kleine-König]

Hello,

On Thu, Dec 13, 2018 at 09:47:00AM +0000, Yoshihiro Shimoda wrote:
> > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > +{
> > > +	/*
> > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > +	 * pseudo low level.
> > > +	 *
> > > +	 * TODO: Add GPIO handling to output low level.
> > > +	 */
> > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > +		rp->pwmcnt |= 1;
> > 
> > In my eyes this is too broken to do. Not sure I have the complete
> > picture, but given a small period (say 2) this 1 cycle might result in
> > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > it, better do this instead.
> 
> My colleague suggests that this workaround code also changes the period
> as maximum (1023) to avoid 50 % duty cycle for such a case.

A negative side effect of that is that reenabling the pwm then takes
longer, right? For my mileage even a duty cycle of 1/1023 if 0 is
requested is rather unfortunate.

> What do you think that this idea is acceptable for upstream? Or, should
> I add gpio handling that Uwe suggested?

Given that it's impossible to implement a gpio handling that results in
well defined periods only I'm not a big fan of that either.

I let Thierry the joy of deciding here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Uwe Kleine-König]

Hello,

On Thu, Dec 13, 2018 at 09:47:00AM +0000, Yoshihiro Shimoda wrote:
> > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > +{
> > > +	/*
> > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > +	 * pseudo low level.
> > > +	 *
> > > +	 * TODO: Add GPIO handling to output low level.
> > > +	 */
> > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > +		rp->pwmcnt |= 1;
> > 
> > In my eyes this is too broken to do. Not sure I have the complete
> > picture, but given a small period (say 2) this 1 cycle might result in
> > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > it, better do this instead.
> 
> My colleague suggests that this workaround code also changes the period
> as maximum (1023) to avoid 50 % duty cycle for such a case.

A negative side effect of that is that reenabling the pwm then takes
longer, right? For my mileage even a duty cycle of 1/1023 if 0 is
requested is rather unfortunate.

> What do you think that this idea is acceptable for upstream? Or, should
> I add gpio handling that Uwe suggested?

Given that it's impossible to implement a gpio handling that results in
well defined periods only I'm not a big fan of that either.

I let Thierry the joy of deciding here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

Hello,

> From: Uwe Kleine-Konig, Sent: Thursday, December 13, 2018 6:53 PM
> 
> Hello,
> 
> On Thu, Dec 13, 2018 at 09:47:00AM +0000, Yoshihiro Shimoda wrote:
> > > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > > +{
> > > > +	/*
> > > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > > +	 * pseudo low level.
> > > > +	 *
> > > > +	 * TODO: Add GPIO handling to output low level.
> > > > +	 */
> > > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > > +		rp->pwmcnt |= 1;
> > >
> > > In my eyes this is too broken to do. Not sure I have the complete
> > > picture, but given a small period (say 2) this 1 cycle might result in
> > > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > > it, better do this instead.
> >
> > My colleague suggests that this workaround code also changes the period
> > as maximum (1023) to avoid 50 % duty cycle for such a case.
> 
> A negative side effect of that is that reenabling the pwm then takes
> longer, right? For my mileage even a duty cycle of 1/1023 if 0 is
> requested is rather unfortunate.

You're right.

> > What do you think that this idea is acceptable for upstream? Or, should
> > I add gpio handling that Uwe suggested?
> 
> Given that it's impossible to implement a gpio handling that results in
> well defined periods only I'm not a big fan of that either.

I got it.

By the way, I checked R-Car Gen3 manual again (which is not public yet and
RZ/G series manual doesn't mention it though), and then changing the pinctrl
setting at runtime is not guarantee. So, I have no change to use gpio on
the pwm-rcar.c. So, I only have a workaround about this at the moment...

> I let Thierry the joy of deciding here.

I hope Thierry accepts this workaround.

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda]

Hello,

> From: Uwe Kleine-Konig, Sent: Thursday, December 13, 2018 6:53 PM
> 
> Hello,
> 
> On Thu, Dec 13, 2018 at 09:47:00AM +0000, Yoshihiro Shimoda wrote:
> > > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > > +{
> > > > +	/*
> > > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > > +	 * pseudo low level.
> > > > +	 *
> > > > +	 * TODO: Add GPIO handling to output low level.
> > > > +	 */
> > > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > > +		rp->pwmcnt |= 1;
> > >
> > > In my eyes this is too broken to do. Not sure I have the complete
> > > picture, but given a small period (say 2) this 1 cycle might result in
> > > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > > it, better do this instead.
> >
> > My colleague suggests that this workaround code also changes the period
> > as maximum (1023) to avoid 50 % duty cycle for such a case.
> 
> A negative side effect of that is that reenabling the pwm then takes
> longer, right? For my mileage even a duty cycle of 1/1023 if 0 is
> requested is rather unfortunate.

You're right.

> > What do you think that this idea is acceptable for upstream? Or, should
> > I add gpio handling that Uwe suggested?
> 
> Given that it's impossible to implement a gpio handling that results in
> well defined periods only I'm not a big fan of that either.

I got it.

By the way, I checked R-Car Gen3 manual again (which is not public yet and
RZ/G series manual doesn't mention it though), and then changing the pinctrl
setting at runtime is not guarantee. So, I have no change to use gpio on
the pwm-rcar.c. So, I only have a workaround about this at the moment...

> I let Thierry the joy of deciding here.

I hope Thierry accepts this workaround.

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

pwm: rcar: improve calculation of divider

[Uwe Kleine-König]

Hello,

while looking at the driver I noticed another patch opportunity: In
rcar_pwm_get_clock_division() there is a loop:

	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
			(1 << div);
		do_div(max, clk_rate);
		if (period_ns <= max)
			break;
	}

The value of div should be calculatable without a loop. Something like:

   divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
   tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
   do_div(tmp, divider);
   div = ilog2(tmp - 1) + 1;

You might want to check if my maths are right, I didn't test.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

pwm: rcar: improve calculation of divider

[Uwe Kleine-König]

Hello,

while looking at the driver I noticed another patch opportunity: In
rcar_pwm_get_clock_division() there is a loop:

	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
			(1 << div);
		do_div(max, clk_rate);
		if (period_ns <= max)
			break;
	}

The value of div should be calculatable without a loop. Something like:

   divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
   tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
   do_div(tmp, divider);
   div = ilog2(tmp - 1) + 1;

You might want to check if my maths are right, I didn't test.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: pwm: rcar: improve calculation of divider

[Laurent Pinchart]

Hi Uwe,

On Friday, 7 December 2018 23:49:32 EET Uwe Kleine-König wrote:
> Hello,
> 
> while looking at the driver I noticed another patch opportunity: In
> rcar_pwm_get_clock_division() there is a loop:
> 
> 	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> 		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> 			(1 << div);
> 		do_div(max, clk_rate);
> 		if (period_ns <= max)
> 			break;
> 	}
> 
> The value of div should be calculatable without a loop. Something like:
> 
>    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
>    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
>    do_div(tmp, divider);
>    div = ilog2(tmp - 1) + 1;
> 
> You might want to check if my maths are right, I didn't test.

I've noticed the same, and wrote the following patch last week, also untested.
I was planning to give it a try before sending it out, but as you've noticed
the same issue, here's the code if anyone wants to give it a try before I can.
Our calculations are similar, the main difference is the last line, and I
think yours read better.

From 22f7149916f590d3dbcc673dacc738441c741900 Mon Sep 17 00:00:00 2001
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Date: Sun, 25 Nov 2018 16:02:39 +0200
Subject: [PATCH] pwm: rcar: Optimize rcar_pwm_get_clock_division()

Get rid of the loop over all possible divisor values by computing the
divisor directly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/pwm/pwm-rcar.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index a41812fc6f95..e6d73b94d5cf 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -68,21 +68,27 @@ static void rcar_pwm_update(struct rcar_pwm_chip *rp, u32 mask, u32 data,
 static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
 {
 	unsigned long clk_rate = clk_get_rate(rp->clk);
-	unsigned long long max; /* max cycle / nanoseconds */
-	unsigned int div;
+	u64 max_period_ns;
+	u32 div;
 
 	if (clk_rate == 0)
 		return -EINVAL;
 
-	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
-		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
-			(1 << div);
-		do_div(max, clk_rate);
-		if (period_ns <= max)
-			break;
-	}
+	/*
+	 * The maximum achievable period is 2^24 * 1023 cycles of the internal
+	 * bus clock.
+	 */
+	max_period_ns = (1ULL << RCAR_PWM_MAX_DIVISION) * RCAR_PWM_MAX_CYCLE
+		      * NSEC_PER_SEC;
+	do_div(max_period_ns, clk_rate);
+
+	if (period_ns > max_period_ns)
+		return -ERANGE;
 
-	return (div <= RCAR_PWM_MAX_DIVISION) ? div : -ERANGE;
+	/* Calculate the divisor and round it up to the next power of two. */
+	div = DIV64_U64_ROUND_UP((u64)period_ns * clk_rate,
+				 (u64)RCAR_PWM_MAX_CYCLE * NSEC_PER_SEC);
+	return fls(2 * div - 1) - 1;
 }
 
 static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp,

-- 
Regards,

Laurent Pinchart

RE: pwm: rcar: improve calculation of divider

[Yoshihiro Shimoda]

Hi Uwe, Laurent,

Thank you very much for your patches!
I tested patches and both codes work correctly.

> From: Laurent Pinchart, Sent: Monday, December 10, 2018 5:55 AM
> 
> Hi Uwe,
> 
> On Friday, 7 December 2018 23:49:32 EET Uwe Kleine-König wrote:
> > Hello,
> >
> > while looking at the driver I noticed another patch opportunity: In
> > rcar_pwm_get_clock_division() there is a loop:
> >
> > 	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> > 		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> > 			(1 << div);
> > 		do_div(max, clk_rate);
> > 		if (period_ns <= max)
> > 			break;
> > 	}
> >
> > The value of div should be calculatable without a loop. Something like:
> >
> >    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> >    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
> >    do_div(tmp, divider);

This should be do_div64_u64() because the divider is 1,023,000,000,000 (over 32-bits).

> >    div = ilog2(tmp - 1) + 1;
> >
> > You might want to check if my maths are right, I didn't test.
> 
> I've noticed the same, and wrote the following patch last week, also untested.
> I was planning to give it a try before sending it out, but as you've noticed
> the same issue, here's the code if anyone wants to give it a try before I can.
> Our calculations are similar, the main difference is the last line, and I
> think yours read better.

So, I'd like to apply Uwe's code to mainline. Uwe, may I send such a patch
with your author and Singed-off-by?

Best regards,
Yoshihiro Shimoda

> From 22f7149916f590d3dbcc673dacc738441c741900 Mon Sep 17 00:00:00 2001
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Date: Sun, 25 Nov 2018 16:02:39 +0200
> Subject: [PATCH] pwm: rcar: Optimize rcar_pwm_get_clock_division()
> 
> Get rid of the loop over all possible divisor values by computing the
> divisor directly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/pwm/pwm-rcar.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index a41812fc6f95..e6d73b94d5cf 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -68,21 +68,27 @@ static void rcar_pwm_update(struct rcar_pwm_chip *rp, u32 mask, u32 data,
>  static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
>  {
>  	unsigned long clk_rate = clk_get_rate(rp->clk);
> -	unsigned long long max; /* max cycle / nanoseconds */
> -	unsigned int div;
> +	u64 max_period_ns;
> +	u32 div;
> 
>  	if (clk_rate == 0)
>  		return -EINVAL;
> 
> -	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> -		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> -			(1 << div);
> -		do_div(max, clk_rate);
> -		if (period_ns <= max)
> -			break;
> -	}
> +	/*
> +	 * The maximum achievable period is 2^24 * 1023 cycles of the internal
> +	 * bus clock.
> +	 */
> +	max_period_ns = (1ULL << RCAR_PWM_MAX_DIVISION) * RCAR_PWM_MAX_CYCLE
> +		      * NSEC_PER_SEC;
> +	do_div(max_period_ns, clk_rate);
> +
> +	if (period_ns > max_period_ns)
> +		return -ERANGE;
> 
> -	return (div <= RCAR_PWM_MAX_DIVISION) ? div : -ERANGE;
> +	/* Calculate the divisor and round it up to the next power of two. */
> +	div = DIV64_U64_ROUND_UP((u64)period_ns * clk_rate,
> +				 (u64)RCAR_PWM_MAX_CYCLE * NSEC_PER_SEC);
> +	return fls(2 * div - 1) - 1;
>  }
> 
>  static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp,
> 
> --
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: pwm: rcar: improve calculation of divider

[Uwe Kleine-König]

Hello,

On Mon, Dec 10, 2018 at 05:09:31AM +0000, Yoshihiro Shimoda wrote:
> Thank you very much for your patches!
> I tested patches and both codes work correctly.

\o/, that's actually better than I expected :-)
 
> > > The value of div should be calculatable without a loop. Something like:
> > >
> > >    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> > >    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
> > >    do_div(tmp, divider);
> 
> This should be do_div64_u64() because the divider is 1,023,000,000,000 (over 32-bits).

Yes, I think Laurent did this part right.
 
> > >    div = ilog2(tmp - 1) + 1;
> > >
> > > You might want to check if my maths are right, I didn't test.
> > 
> > I've noticed the same, and wrote the following patch last week, also untested.
> > I was planning to give it a try before sending it out, but as you've noticed
> > the same issue, here's the code if anyone wants to give it a try before I can.
> > Our calculations are similar, the main difference is the last line, and I
> > think yours read better.
> 
> So, I'd like to apply Uwe's code to mainline. Uwe, may I send such a patch
> with your author and Singed-off-by?

Please no, I cannot sing good enough for this :-)

Honestly: If you take the authorship and write something like "Algorithm
suggested by Uwe Kleine-K�nig and Laurent Pinchart" that's IMHO fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: pwm: rcar: improve calculation of divider

[Uwe Kleine-König]

Hello,

On Mon, Dec 10, 2018 at 05:09:31AM +0000, Yoshihiro Shimoda wrote:
> Thank you very much for your patches!
> I tested patches and both codes work correctly.

\o/, that's actually better than I expected :-)
 
> > > The value of div should be calculatable without a loop. Something like:
> > >
> > >    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> > >    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
> > >    do_div(tmp, divider);
> 
> This should be do_div64_u64() because the divider is 1,023,000,000,000 (over 32-bits).

Yes, I think Laurent did this part right.
 
> > >    div = ilog2(tmp - 1) + 1;
> > >
> > > You might want to check if my maths are right, I didn't test.
> > 
> > I've noticed the same, and wrote the following patch last week, also untested.
> > I was planning to give it a try before sending it out, but as you've noticed
> > the same issue, here's the code if anyone wants to give it a try before I can.
> > Our calculations are similar, the main difference is the last line, and I
> > think yours read better.
> 
> So, I'd like to apply Uwe's code to mainline. Uwe, may I send such a patch
> with your author and Singed-off-by?

Please no, I cannot sing good enough for this :-)

Honestly: If you take the authorship and write something like "Algorithm
suggested by Uwe Kleine-König and Laurent Pinchart" that's IMHO fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: pwm: rcar: improve calculation of divider

[Yoshihiro Shimoda]

Hi Uwe,

> From: Uwe Kleine-König, Sent: Monday, December 10, 2018 5:04 PM
> 
> Hello,
> 
> On Mon, Dec 10, 2018 at 05:09:31AM +0000, Yoshihiro Shimoda wrote:
> > Thank you very much for your patches!
> > I tested patches and both codes work correctly.
> 
> \o/, that's actually better than I expected :-)
> 
> > > > The value of div should be calculatable without a loop. Something like:
> > > >
> > > >    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> > > >    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
> > > >    do_div(tmp, divider);
> >
> > This should be do_div64_u64() because the divider is 1,023,000,000,000 (over 32-bits).
> 
> Yes, I think Laurent did this part right.
> 
> > > >    div = ilog2(tmp - 1) + 1;
> > > >
> > > > You might want to check if my maths are right, I didn't test.
> > >
> > > I've noticed the same, and wrote the following patch last week, also untested.
> > > I was planning to give it a try before sending it out, but as you've noticed
> > > the same issue, here's the code if anyone wants to give it a try before I can.
> > > Our calculations are similar, the main difference is the last line, and I
> > > think yours read better.
> >
> > So, I'd like to apply Uwe's code to mainline. Uwe, may I send such a patch
> > with your author and Singed-off-by?
> 
> Please no, I cannot sing good enough for this :-)
> 
> Honestly: If you take the authorship and write something like "Algorithm
> suggested by Uwe Kleine-König and Laurent Pinchart" that's IMHO fine.

Thank you for your reply. I got it. I'll make a patch with such comment :)

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: pwm: rcar: improve calculation of divider

[Yoshihiro Shimoda]

Hi Uwe,

> From: Uwe Kleine-König, Sent: Monday, December 10, 2018 5:04 PM
> 
> Hello,
> 
> On Mon, Dec 10, 2018 at 05:09:31AM +0000, Yoshihiro Shimoda wrote:
> > Thank you very much for your patches!
> > I tested patches and both codes work correctly.
> 
> \o/, that's actually better than I expected :-)
> 
> > > > The value of div should be calculatable without a loop. Something like:
> > > >
> > > >    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> > > >    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
> > > >    do_div(tmp, divider);
> >
> > This should be do_div64_u64() because the divider is 1,023,000,000,000 (over 32-bits).
> 
> Yes, I think Laurent did this part right.
> 
> > > >    div = ilog2(tmp - 1) + 1;
> > > >
> > > > You might want to check if my maths are right, I didn't test.
> > >
> > > I've noticed the same, and wrote the following patch last week, also untested.
> > > I was planning to give it a try before sending it out, but as you've noticed
> > > the same issue, here's the code if anyone wants to give it a try before I can.
> > > Our calculations are similar, the main difference is the last line, and I
> > > think yours read better.
> >
> > So, I'd like to apply Uwe's code to mainline. Uwe, may I send such a patch
> > with your author and Singed-off-by?
> 
> Please no, I cannot sing good enough for this :-)
> 
> Honestly: If you take the authorship and write something like "Algorithm
> suggested by Uwe Kleine-König and Laurent Pinchart" that's IMHO fine.

Thank you for your reply. I got it. I'll make a patch with such comment :)

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 0/5] pwm: rcar: Add support "atomic" API and workaround

[Laurent Pinchart]

Hi Shimoda-san,

Thank you for the patches.

On Friday, 7 December 2018 10:29:28 EET Yoshihiro Shimoda wrote:
> This patch adds support for PWM "atomic" API.
> 
> This patch also adds a workaround to output "pseudo" low level.
> Otherwise, the PWM backlight driver doesn't work correctly, especially
> it cannot output maximum brightness actually.
> 
> Yoshihiro Shimoda (5):
>   pwm: rcar: add rcar_pwm_calc_counter() to calculate PWMCNT value only
>   pwm: rcar: Add support "atomic" API
>   pwm: rcar: Use "atomic" API on rcar_pwm_resume()
>   pwm: rcar: remove legacy APIs
>   pwm: rcar: add workaround to output "pseudo" low level
> 
>  drivers/pwm/pwm-rcar.c | 108 ++++++++++++++++++++++++++--------------------
>  1 file changed, 62 insertions(+), 46 deletions(-)

For the whole series,

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

with backlight on the Draak board.

I do however agree with Uwe's comments.

-- 
Regards,

Laurent Pinchart