| Message ID | 20230310191405.2606296-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: meson: Explicitly set .polarity in .get_state() | expand |
Hi Uwe, On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: [...] > There is a complicating fact, that the .apply() callback fakes support > for inversed polarity. This is not (and cannot) be matched by > .get_state(). As fixing this isn't easy, only point it out in a comment > to prevent authors of other drivers from copying that approach. If you have any suggestions on how to fix this then please let us know. I don't recall any board needing support for inverted PWM - but they may be out there somewhere... > Fixes: c375bcbaabdb ("pwm: meson: Read the full hardware state in meson_pwm_get_state()") > Reported-by: Munehisa Kamata <kamatam@amazon.com> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Best regards, Martin
On Sat, Mar 11, 2023 at 10:00:50PM +0100, Martin Blumenstingl wrote: > Hi Uwe, > > On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > [...] > > There is a complicating fact, that the .apply() callback fakes support > > for inversed polarity. This is not (and cannot) be matched by > > .get_state(). As fixing this isn't easy, only point it out in a comment > > to prevent authors of other drivers from copying that approach. > If you have any suggestions on how to fix this then please let us know. > I don't recall any board needing support for inverted PWM - but they > may be out there somewhere... And that's the problem. As the hardware doesn't support inverted polarity there is no way to implement it correctly. The only right way would be to return -EINVAL in this case, but this might break some consumers. I have an idea how to evolve the PWM API. That's by introducing an .offset parameter to struct pwm_state. This would describe the following PWM signal: ___________/¯¯¯¯¯¯¯¯¯\_______________/¯¯¯¯¯¯¯¯¯\____ ^ ^ ^ ^ ^ <------ period ----------> <- offset-> <--------> duty_cycle This is more general than polarity: It can describe normal polarity (.offset = 0) and inversed polarity (.offset = .period - .duty_cycle). Then the policy to implement a pwm_state like that would probably be: - Pick the biggest period not bigger than requested - for that period pick the biggest duty cycle not bigger than requested - for that period and duty_cycle pick the biggest offset not bigger than requested. With these rules in place it would be allowed to configure normal polarity for a request with inverted polarity, but not the other way around. Then the algorithm currently implemented in the meson driver would be allowed. A consumer that doesn't care about the offset (i.e. most drivers) could just pass .offset = .period - 1. To be practical for consumers who care about polarity, we first would need a way to test the capabilities of a PWM though. I have an idea for that, too, but today this is still vapourware. > > Fixes: c375bcbaabdb ("pwm: meson: Read the full hardware state in meson_pwm_get_state()") > > Reported-by: Munehisa Kamata <kamatam@amazon.com> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Thanks Uwe
On Sat, Mar 11, 2023 at 10:44 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Sat, Mar 11, 2023 at 10:00:50PM +0100, Martin Blumenstingl wrote: > > Hi Uwe, > > > > On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > [...] > > > There is a complicating fact, that the .apply() callback fakes support > > > for inversed polarity. This is not (and cannot) be matched by > > > .get_state(). As fixing this isn't easy, only point it out in a comment > > > to prevent authors of other drivers from copying that approach. > > If you have any suggestions on how to fix this then please let us know. > > I don't recall any board needing support for inverted PWM - but they > > may be out there somewhere... > > And that's the problem. As the hardware doesn't support inverted > polarity there is no way to implement it correctly. The only right way > would be to return -EINVAL in this case, but this might break some > consumers. > > I have an idea how to evolve the PWM API. That's by introducing an > .offset parameter to struct pwm_state. This would describe the following > PWM signal: > > > ___________/¯¯¯¯¯¯¯¯¯\_______________/¯¯¯¯¯¯¯¯¯\____ > ^ ^ ^ ^ ^ > <------ period ----------> > <- offset-> > <--------> duty_cycle > > This is more general than polarity: It can describe normal polarity > (.offset = 0) and inversed polarity (.offset = .period - .duty_cycle). > > Then the policy to implement a pwm_state like that would probably be: > > - Pick the biggest period not bigger than requested > - for that period pick the biggest duty cycle not bigger than requested > - for that period and duty_cycle pick the biggest offset not bigger > than requested. > > With these rules in place it would be allowed to configure normal > polarity for a request with inverted polarity, but not the other way > around. Then the algorithm currently implemented in the meson driver > would be allowed. > > A consumer that doesn't care about the offset (i.e. most drivers) could > just pass .offset = .period - 1. > > To be practical for consumers who care about polarity, we first would > need a way to test the capabilities of a PWM though. I have an idea for > that, too, but today this is still vapourware. In my opinion your proposal makes sense. I don't have the time to implement it myself at the moment though. I can help test on some of the Amlogic SBCs that I have (and use a cheap signal analyzer I have to look at the generated PWM signal). Best regards, Martin
On Sat 11 Mar 2023 at 22:00, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Hi Uwe, > > On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > [...] >> There is a complicating fact, that the .apply() callback fakes support >> for inversed polarity. This is not (and cannot) be matched by >> .get_state(). As fixing this isn't easy, only point it out in a comment >> to prevent authors of other drivers from copying that approach. > If you have any suggestions on how to fix this then please let us know. > I don't recall any board needing support for inverted PWM - but they > may be out there somewhere... AFAIK, PWM are essentially used for the SDIO 32k clock and voltage regulators. I don't recall seeing anything else. It should be safe to change polarity if necessary, except for the DVFS PWM regulators maybe ? I fear that if we change the PWM setting it might trigger a glitch on the supply and possibly stall the CPU. That being said, I don't think there is any particular care regarding that right now, so maybe it is fine. > >> Fixes: c375bcbaabdb ("pwm: meson: Read the full hardware state in meson_pwm_get_state()") >> Reported-by: Munehisa Kamata <kamatam@amazon.com> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > Best regards, > Martin
On Mon, Mar 13, 2023 at 10:07:48AM +0100, Jerome Brunet wrote: > > On Sat 11 Mar 2023 at 22:00, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > > Hi Uwe, > > > > On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > [...] > >> There is a complicating fact, that the .apply() callback fakes support > >> for inversed polarity. This is not (and cannot) be matched by > >> .get_state(). As fixing this isn't easy, only point it out in a comment > >> to prevent authors of other drivers from copying that approach. > > If you have any suggestions on how to fix this then please let us know. > > I don't recall any board needing support for inverted PWM - but they > > may be out there somewhere... > > AFAIK, PWM are essentially used for the SDIO 32k clock and voltage > regulators. I don't recall seeing anything else. > > It should be safe to change polarity if necessary, except for the DVFS > PWM regulators maybe ? I fear that if we change the PWM setting it might > trigger a glitch on the supply and possibly stall the CPU. > > That being said, I don't think there is any particular care regarding > that right now, so maybe it is fine. Another option is to do something like that: diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index 16d79ca5d8f5..25a177a3fa00 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -162,8 +162,10 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, duty = state->duty_cycle; period = state->period; - if (state->polarity == PWM_POLARITY_INVERSED) + if (state->polarity == PWM_POLARITY_INVERSED) { + WARN_ONCE(1, "Wrongly trying to support inversed polarity. Please report to linux-pwm@vger.kernel.org if you rely on this\n"); duty = period - duty; + } fin_freq = clk_get_rate(channel->clk); if (fin_freq == 0) { and then drop that faked support in a year or so if nobody spoke up. Disclaimer: I assume Thierry is not a fan of this approach, he opposed similar warnings in the past. Best regards Uwe
On Mon, Mar 13, 2023 at 10:51 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: [...] > Another option is to do something like that: > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 16d79ca5d8f5..25a177a3fa00 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -162,8 +162,10 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > duty = state->duty_cycle; > period = state->period; > > - if (state->polarity == PWM_POLARITY_INVERSED) > + if (state->polarity == PWM_POLARITY_INVERSED) { > + WARN_ONCE(1, "Wrongly trying to support inversed polarity. Please report to linux-pwm@vger.kernel.org if you rely on this\n"); > duty = period - duty; > + } > > fin_freq = clk_get_rate(channel->clk); > if (fin_freq == 0) { > > and then drop that faked support in a year or so if nobody spoke up. > > Disclaimer: I assume Thierry is not a fan of this approach, he opposed > similar warnings in the past. I personally think it's fine to have this warning. If Thierry has no objections in this case then it'll help us find whether we really need proper support in PWM core or we can just remove this fake support from pwm-meson Best regards, Martin
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index 16d79ca5d8f5..5cd7b90872c6 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -162,6 +162,12 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, duty = state->duty_cycle; period = state->period; + /* + * Note this is wrong. The result is an output wave that isn't really + * inverted and so is wrongly identified by .get_state as normal. + * Fixing this needs some care however as some machines might rely on + * this. + */ if (state->polarity == PWM_POLARITY_INVERSED) duty = period - duty; @@ -358,6 +364,8 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, state->duty_cycle = 0; } + state->polarity = PWM_POLARITY_NORMAL; + return 0; }
The driver only supports normal polarity. Complete the implementation of .get_state() by setting .polarity accordingly. This fixes a regression that was possible since commit c73a3107624d ("pwm: Handle .get_state() failures") which stopped to zero-initialize the state passed to the .get_state() callback. This was reported at https://forum.odroid.com/viewtopic.php?f=177&t=46360 . While this was an unintended side effect, the real issue is the driver's callback not setting the polarity. There is a complicating fact, that the .apply() callback fakes support for inversed polarity. This is not (and cannot) be matched by .get_state(). As fixing this isn't easy, only point it out in a comment to prevent authors of other drivers from copying that approach. Fixes: c375bcbaabdb ("pwm: meson: Read the full hardware state in meson_pwm_get_state()") Reported-by: Munehisa Kamata <kamatam@amazon.com> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, similar to the patches for four other drivers[1], I think we should apply this patch as a fix. Best regards Uwe [1] https://lore.kernel.org/linux-pwm/20230228135508.1798428-1-u.kleine-koenig@pengutronix.de drivers/pwm/pwm-meson.c | 8 ++++++++ 1 file changed, 8 insertions(+)