[5/5] pwm: rcar: add workaround to output "pseudo" low level
diff mbox series

Message ID 1544171373-29618-6-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 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(+)

Comments

Uwe Kleine-König Dec. 7, 2018, 9:13 a.m. UTC | #1
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
Yoshihiro Shimoda Dec. 10, 2018, 4:49 a.m. UTC | #2
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/  |
Uwe Kleine-König Dec. 10, 2018, 8:11 a.m. UTC | #3
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
Yoshihiro Shimoda Dec. 12, 2018, 3:19 a.m. UTC | #4
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/  |
Uwe Kleine-König Dec. 12, 2018, 7:37 a.m. UTC | #5
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
Yoshihiro Shimoda Dec. 12, 2018, 10:49 a.m. UTC | #6
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/  |
Yoshihiro Shimoda Dec. 13, 2018, 9:47 a.m. UTC | #7
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.
Uwe Kleine-König Dec. 13, 2018, 9:52 a.m. UTC | #8
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
Yoshihiro Shimoda Dec. 13, 2018, 10:53 a.m. UTC | #9
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/  |

Patch
diff mbox series

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);