| Message ID | 1544171373-29618-6-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pwm: rcar: Add support "atomic" API and workaround | expand |
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
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/ |
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
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/ |
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
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/ |
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.
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
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/ |
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);
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(+)