linux-pwm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michael Walle <michael@walle.cc>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
	linux-pwm@vger.kernel.org, linux-watchdog@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Rob Herring <robh+dt@kernel.org>,
	Jean Delvare <jdelvare@suse.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Lee Jones <lee.jones@linaro.org>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Shawn Guo <shawnguo@kernel.org>, Li Yang <leoyang.li@nxp.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Jason Cooper <jason@lakedaemon.net>,
	Marc Zyngier <maz@kernel.org>, Mark Brown <broonie@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundat>
Subject: Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
Date: Tue, 14 Jul 2020 23:09:28 +0200	[thread overview]
Message-ID: <eedceb44cba9b54e0634f0e8e4f96f70@walle.cc> (raw)
In-Reply-To: <20200714160856.rjqi7lv63geil3hm@pengutronix.de>

Hi Uwe,

Am 2020-07-14 18:08, schrieb Uwe Kleine-König:
> On Tue, Jul 14, 2020 at 01:31:13PM +0200, Michael Walle wrote:
>> Am 2020-07-13 10:47, schrieb Uwe Kleine-König:
>> > I already thought about proposing pwm_get_rate_hw(), but for now there
>> > is (AFAICT) no user who would need it. And it's hard to know which
>> > variant is actually preferred by consumers. My expectation is that most
>> > don't even care.
>> >
>> > I also have a pwm_round_rate() function in mind that will give you the
>> > actual rate without applying it. This can then be used by consumers who
>> > care. But also there is no user who would need it today.
>> 
>> Ok. I take it that all such improvements are still in the making ;)
> 
> If you have a real use case, present it, then I give it a boost on my
> todo list.

Not really.

>> > > But the PWM subsystem returns the cached state,
>> > > right? get_state() is called only on device request (and during
>> > > debug it seems). Actually, enabling PWM_DEBUG might choke on this
>> > > workaround (".apply didn't pick the best available period"). Is
>> > > this ok?
>> >
>> > hmm, I didn't consider this when writing the checks for PWM_DEBUG.
>> > According to the currently checked rules the expected configuration is
>> > to pick the 250Hz mode and use cycle = 0x7f.
>> 
>> Not to use 0x80, which is the max_duty_cycle? Like its 0x40 for the 
>> 500Hz
>> mode.
> 
> No, when I thought about a sane set of rules (and so checks for
> PWM_DEBUG) I didn't consider a PWM that can implement 100% but not for
> all otherwise available period lengths. I'm still amazed sometimes how
> different the capabilities of different implementations for something 
> so
> seemingly easy like a PWM are.
> 
>> > Hmm, I have to think about
>> > this. Maybe we should weaken the check to the cases with
>> > 0 < duty_cycle < period. Thierry, what do you think?
>> >
>> > Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f seems
>> > to be far from reality. (Is it?)
>> 
>> If you mean by insisting to clip at 0x7f, yeah thats bad IMHO, because
>> the user wants an all-high line, but in the end it would be a toggling
>> line. It wouldn't be that bad for anything in between 0% and 100% but
>> IMHO its bad for exactly 0% and 100%.
>> 
>> You could also ask the driver about known quirks, like special 0% and
>> 100% handling and exclude it from the tests accordingly.
> 
> Do you care enough to propose a patch? You're in the situation to test
> it.

Ok. I'll come up with something outside of this patch series.

>> > > > > +	ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl);
>> > > > > +	if (ret)
>> > > > > +		return ret;
>> > > > > +
>> > > > > +	return regmap_write(priv->regmap, priv->offset + PWM_CYCLE,
>> > > > > (u8)cycle);
>> > > >
>> > > > I assume this can result in broken output? Consider the hardware runs
>> > > > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle =
>> > > > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23?
>> > >
>> > > Isn't that always the case if a write may fail and there are more than
>> > > one register to configure?
>> >
>> > Depending on hardware capabilities you might not be able to prevent
>> > this yes. Unfortunately this is quite common.
>> >
>> > But there are hardware implementations that are not prone to such
>> > failures. (E.g. the registers written can be only shadow values that are
>> > latched into hardware only when the last value is written.)
>> 
>> Maybe this could be improved in the future.
> 
> We should somewhere describe, what an ideal PWM can do.
> My wishlist (just as it comes to my mind, so no guarantee of
> completeness):
> 
>  - can do 0% duty cycle for all supported period lengths
>  - can do 100% duty cycle for all supported period lengths
>  - supports both polarities
>  - supports immediate change of configuration and after completion of
>    the currently running period
>  - atomic update (i.e. if you go from configuration A to configuration 
> B
>    the hardware guarantees to only emit periods of type A and then type
>    B. (Depending on the item above, the last A period might be cut 
> off.)

We actually discussed this, because the implementation would be easier. 
But
if the change takes place immediately you might end up with a longer 
duty
cycle. Assume the PWM runs at 80% duty cycle and starts with the 
on-period.
If you now change that to 50% you might end up with one successive duty
cycle of "130%". Eg. the 80% of the old and right after that you switch 
to
the new 50% and then you'd have a high output which corresponds to a 
130%
cycle. I don't know if that is acceptable for all applications.

>  - emits an irq when configuration changes

Why would you need the interrupt?

> 
>> > If you change only cycle but not mode, does the hardware complete the
>> > currently running period?
>> 
>> No it does not.
> 
> Please document this as a Limitation.

I've discussed this internally, for now its a limitation. In the worst
case you'd do one 100% duty cycle. Maybe we can fix the hardware. I
acknowledge that this is a severe limitation, esp. if you use the PWM
for controlling stuff (for now its only LCD backlight.. so thats ok).

>> > What about disable()?
>> 
>> Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
>> fix that (in hardware), not much we can do in the driver here. We are
>> _very_ constraint in size, therefore all that little edge cases fall 
>> off
>> the table.
> 
> You're saying that on disable the hardware emits a constant high level
> for one cycle? I hope not ...

Mh, I was mistaken, disabling the PWM will turn it off immediately, but
one 100% duty cycle may happen if you change from a higher to a lower
duty cycle setting. See above.

> I never programmed a CPLD to emulate a hardware PWM, but I wonder if
> these are really edge cases that increase the size of the binary?!

At the moment there is only one 8bit register which stores the value
which is used for matching. If you want to change that setting after
a whole cycle, you'd use another 8bit register to cache the new value.
So this would at least needs 8 additional flip-flops. This doesn't
sound much, but we are already near 100% usage of the CPLD. So its
hard to convince people why this is really necessary.

-michael

  reply	other threads:[~2020-07-14 21:09 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
2020-07-06 17:53 ` [PATCH v5 01/13] regmap-irq: use fwnode instead of device node in add_irq_chip() Michael Walle
2020-07-08 10:22   ` Mark Brown
     [not found] ` <20200706175353.16404-1-michael-QKn5cuLxLXY@public.gmane.org>
2020-07-06 17:53   ` [PATCH v5 02/13] mfd: add simple regmap based I2C driver Michael Walle
     [not found]     ` <20200706175353.16404-3-michael-QKn5cuLxLXY@public.gmane.org>
2020-07-17  9:04       ` Lee Jones
2020-07-19 22:25         ` Michael Walle
2020-07-17  9:06     ` Lee Jones
2020-07-19 22:31       ` Michael Walle
2020-07-20  7:43         ` Lee Jones
2020-07-06 17:53   ` [PATCH v5 06/13] watchdog: add support for sl28cpld watchdog Michael Walle
2020-07-06 17:53   ` [PATCH v5 12/13] arm64: dts: freescale: sl28: enable LED support Michael Walle
2020-07-17  8:36     ` Pavel Machek
2020-07-17 21:54       ` Michael Walle
2020-07-06 17:53 ` [PATCH v5 03/13] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
2020-07-13 16:04   ` Rob Herring
2020-07-06 17:53 ` [PATCH v5 04/13] mfd: simple-mfd-i2c: add sl28cpld support Michael Walle
2020-07-06 17:53 ` [PATCH v5 05/13] irqchip: add sl28cpld interrupt controller support Michael Walle
2020-07-08  9:37   ` Marc Zyngier
2020-07-06 17:53 ` [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller Michael Walle
2020-07-09  8:50   ` Uwe Kleine-König
     [not found]     ` <20200709085006.b54ype3p4yu64upl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2020-07-11 17:28       ` Michael Walle
2020-07-13  8:47         ` Uwe Kleine-König
2020-07-14 11:31           ` Michael Walle
2020-07-14 16:08             ` Uwe Kleine-König
2020-07-14 21:09               ` Michael Walle [this message]
2020-07-15 16:36                 ` Uwe Kleine-König
2020-07-15 17:45                   ` Michael Walle
2020-07-15 18:18                     ` Uwe Kleine-König
2020-07-15 20:41                       ` Michael Walle
2020-07-16  6:10                         ` Uwe Kleine-König
2020-07-06 17:53 ` [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller Michael Walle
     [not found]   ` <20200706175353.16404-9-michael-QKn5cuLxLXY@public.gmane.org>
2020-07-08  7:35     ` Linus Walleij
2020-07-17  0:25   ` kernel test robot
2020-07-06 17:53 ` [PATCH v5 09/13] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
2020-07-06 17:53 ` [PATCH v5 10/13] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
2020-07-06 17:53 ` [PATCH v5 11/13] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
2020-07-06 17:53 ` [PATCH v5 13/13] arm64: dts: freescale: sl28: enable fan support Michael Walle
     [not found]   ` <20200706175353.16404-14-michael-QKn5cuLxLXY@public.gmane.org>
2020-07-08  7:39     ` Linus Walleij
     [not found]       ` <CACRpkdaPO7CGNrxmjL5QH1cxP5wqku1oMtQaQgJfeKiKqiGAOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-07-08  7:56         ` Michael Walle
2020-07-08 17:00 ` [PATCH v5 00/13] Add support for Kontron sl28cpld Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=eedceb44cba9b54e0634f0e8e4f96f70@walle.cc \
    --to=michael@walle.cc \
    --cc=bgolaszewski@baylibre.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundat \
    --cc=jason@lakedaemon.net \
    --cc=jdelvare@suse.com \
    --cc=lee.jones@linaro.org \
    --cc=leoyang.li@nxp.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=maz@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).