linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Michael Walle <michael@walle.cc>,
	Thierry Reding <thierry.reding@gmail.com>
Cc: 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@linuxfoundation.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
Date: Mon, 13 Jul 2020 10:47:50 +0200	[thread overview]
Message-ID: <20200713084750.qj4hquzd6uz6y526@pengutronix.de> (raw)
In-Reply-To: <72858253a9094074e9c8cd7a4e1db09f@walle.cc>

[-- Attachment #1: Type: text/plain, Size: 13403 bytes --]

Hello Michael,

On Sat, Jul 11, 2020 at 07:28:05PM +0200, Michael Walle wrote:
> Am 2020-07-09 10:50, schrieb Uwe Kleine-König:
> > On Mon, Jul 06, 2020 at 07:53:47PM +0200, Michael Walle wrote:
> > > diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> > > new file mode 100644
> > > index 000000000000..8ee286b605bf
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sl28cpld.c
> > > @@ -0,0 +1,187 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * sl28cpld PWM driver
> > > + *
> > > + * Copyright 2020 Kontron Europe GmbH
> > > + */
> > 
> > Is there publically available documenation available? If so please add a
> > link here.
> 
> Unfortunately not. But it should be easy enough and I'll describe it
> briefly in the header.

That's fine.

> > > +#include <linux/bitfield.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +/*
> > > + * PWM timer block registers.
> > > + */
> > > +#define PWM_CTRL		0x00
> > > +#define   PWM_ENABLE		BIT(7)
> > > +#define   PWM_MODE_250HZ	0
> > > +#define   PWM_MODE_500HZ	1
> > > +#define   PWM_MODE_1KHZ		2
> > > +#define   PWM_MODE_2KHZ		3
> > > +#define   PWM_MODE_MASK		GENMASK(1, 0)
> > > +#define PWM_CYCLE		0x01
> > > +#define   PWM_CYCLE_MAX		0x7f
> > 
> > Please use a less generic prefix for your defines. Also I like having
> > the defines for field names include register name. Something like:
> > 
> > 	#define PWM_SL28CPLD_CTRL		0x00
> > 	#define PWM_SL28CPLD_CTRL_ENABLE		BIT(7)
> > 	#define PWM_SL28CPLD_CTRL_MODE_MASK		GENMASK(1, 0)
> 
> Ok.
> 
> > 	#define
> > PWM_SL28CPLD_CTRL_MODE_250HZ		FIELD_PREP(PWM_SL28CPLD_CTRL_MODE_MASK,
> > 0)
> 
> Shouldn't we just "#define ..MODE_250HZ 1" use FIELD_PREP inside the code,
> so you can actually use the normalized enumeration values, too?

yeah, looks sane.

> Actually, I'll rename the PWM_MODE to PWM_PRESCALER, because that is
> more accurate.

Whatever suits you and is consistent is fine for me.

> > > +struct sl28cpld_pwm {
> > > +	struct pwm_chip pwm_chip;
> > > +	struct regmap *regmap;
> > > +	u32 offset;
> > > +};
> > > +
> > > +struct sl28cpld_pwm_periods {
> > > +	u8 ctrl;
> > > +	unsigned long duty_cycle;
> > > +};
> > > +
> > > +struct sl28cpld_pwm_config {
> > > +	unsigned long period_ns;
> > > +	u8 max_duty_cycle;
> > > +};
> > > +
> > > +static struct sl28cpld_pwm_config sl28cpld_pwm_config[] = {
> > 
> > const ? (Or drop as the values can be easily computed, see below.)
> > 
> > > +	[PWM_MODE_250HZ] = { .period_ns = 4000000, .max_duty_cycle = 0x80 },
> > > +	[PWM_MODE_500HZ] = { .period_ns = 2000000, .max_duty_cycle = 0x40 },
> > > +	[PWM_MODE_1KHZ]  = { .period_ns = 1000000, .max_duty_cycle = 0x20 },
> > > +	[PWM_MODE_2KHZ]  = { .period_ns =  500000, .max_duty_cycle = 0x10 },
> > > +};
> > > +
> > > +static void sl28cpld_pwm_get_state(struct pwm_chip *chip,
> > > +				   struct pwm_device *pwm,
> > > +				   struct pwm_state *state)
> > > +{
> > > +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> > > +	static struct sl28cpld_pwm_config *config;
> > > +	unsigned int reg;
> > > +	unsigned int mode;
> > > +
> > > +	regmap_read(priv->regmap, priv->offset + PWM_CTRL, &reg);
> > > +
> > > +	state->enabled = reg & PWM_ENABLE;
> > 
> > Would it be more consisted to use FIELD_GET here, too?
> 
> I had used FIELD_GET only for bit-fields with more than one bit,
> i.e. no flags. But that is just a matter of taste, I guess. I'd
> prefer to keep the simple "reg & PWM_ENABLE". If you insist on
> the FIELD_GET() I'll change it ;)

I think using FIELD_GET is more consistent, but I won't insist.

> > > +	mode = FIELD_GET(PWM_MODE_MASK, reg);
> > > +	config = &sl28cpld_pwm_config[mode];
> > > +	state->period = config->period_ns;
> > 
> > I wonder if this could be done more effectively without the above table.
> > Something like:
> > 
> > 	state->period = 4000000 >> mode.
> 
> The reason I introduced a lookup table here was that I need a
> list of the supported modes; I wasn't aware of the rounding.

List of supported modes = [0, 1, 2, 3], isn't it?

> See also below.
> 
> > (with a #define for 4000000 of course).
> > 
> > > +	regmap_read(priv->regmap, priv->offset + PWM_CYCLE, &reg);
> > > +	pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle);
> > 
> > Oh, what a creative idea to use pwm_set_relative_duty_cycle here.
> 
> What is that helper for then? The former versions did the same
> calculations (i.e. DIV_ROUND_CLOSEST_ULL()) just open coded. But
> I guess then it was also rounding the wrong way.

Yes. In my book pwm_set_relative_duty_cycle is for consumers. And if
DIV_ROUND_CLOSEST_ULL is the right thing for them depends on their use
case.

> > Unfortunately it's using the wrong rounding strategy. Please enable
> > PWM_DEBUG which should diagnose these problems (given enough testing).
> 
> Is there any written documentation on how to round, i.e. up or down?

There are the checks implemented for PWM_DEBUG. I started to work on the
documentation
(https://patchwork.ozlabs.org/project/linux-pwm/patch/20191209213233.29574-2-u.kleine-koenig@pengutronix.de/)
but didn't get feedback yet. (And the rounding rules are not included
there.)

> I had a look Documentation/driver-api/pwm.rst again. But couldn't find
> anything. A grep DIV_ROUND_CLOSEST_ULL() turns out that quite a few
> drivers use it, so I did the same ;)

Yes, the rounding requirement is new and many driver's are not
conforming (yet).

> > (Hmm, on second thought I'm not sure that rounding is relevant with the
> > numbers of this hardware. Still it's wrong in general and I don't want
> > to have others copy this.)
> > 
> > > +}
> > > +
> > > +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct
> > > pwm_device *pwm,
> > > +			      const struct pwm_state *state)
> > > +{
> > > +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> > > +	struct sl28cpld_pwm_config *config;
> > > +	unsigned int cycle;
> > > +	int ret;
> > > +	int mode;
> > > +	u8 ctrl;
> > > +
> > > +	/* Get the configuration by comparing the period */
> > > +	for (mode = 0; mode < ARRAY_SIZE(sl28cpld_pwm_config); mode++) {
> > > +		config = &sl28cpld_pwm_config[mode];
> > > +		if (state->period == config->period_ns)
> > > +			break;
> > > +	}
> > > +
> > > +	if (mode == ARRAY_SIZE(sl28cpld_pwm_config))
> > > +		return -EINVAL;
> > 
> > You're supposed to pick the biggest period that isn't bigger than the
> > requested period. So something like:
> > 
> > 	switch(period) {
> > 	case 4000000 ... UINT_MAX:
> > 		mode = 0;
> > 		break;
> > 	case 2000000 ... 3999999:
> > 		mode = 1;
> > 		break;
> > 	...
> > 	}
> > 
> > (or:
> > 
> > 	if period >= 4000000:
> > 		mode = 0
> > 	else:
> > 		// I think ... please double-check
> > 		mode = ilog2(4000000 / (period + 1)) + 1
> > 
> > 	if mode > 3:
> > 		return -ERANGE;
> > )
> 
> I see. In this case I can of course drop the table. But the rounding
> will be then very coarse for this driver. And there is no way to get
> the value which is actually set, right? You can just read the cached
> value. So that value might be far off the actual one set in the
> hardware.

Yes, we once changed pwm_get_rate to return the actually implemented
setting, but this broke some stuff; see commit
40a6b9a00930fd6b59aa2eb6135abc2efe5440c3.

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.

> During testing I've also found the following problem: Assume we set
> a period of 5000000ns; this will be rounded to 4000000ns and written
> to the hardware. But the usable duty cycle is still 0..5000000ns. The
> driver will translate this input in the following manner:
>  - 0..4000000 -> 0%..100%
>  - >4000000 -> 100%
> Is this behavior intended?

It's expected.

> Even for PWM hardware which supports finer
> grained frequencies there will be some upper and lower limits. Is
> the user of the PWM supposed to know these?

There is nothing we can do on hardware imposed limits. In practise it
doesn't seem to matter. Also note that most consumers get a proposed
period length.

> > 	real_period = 4000000 >> mode;
> > 
> > > +	ctrl = FIELD_PREP(PWM_MODE_MASK, mode);
> > > +	if (state->enabled)
> > > +		ctrl |= PWM_ENABLE;
> > > +
> > > +	cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle);
> > 
> > Again the rounding is wrong. You need need to round down the requested
> > duty_cycle to the next possible value. So something like:
> > 
> > 	duty_cycle = min(real_period, state->duty_cycle);
> > 
> > 	cycle = duty_cycle * (0x80 >> mode) / (4000000 >> mode);
> > 
> > which can be further simplified to
> > 
> > 	cycle = duty_cycle / 31250
> 
> Mh, this made me think where that "magic" number is coming from. Turns
> out this is the NSECS_PE_SEC / base clock of the PWM.

It's a simplification of the line above, so it is 4000000 / 0x80. (But
it's not by chance this matches NSECS_PER_SEC / base clock of course.)

> I guess I'll rework the get_state() and apply() to just use this
> base frequency, dropping the table etc.
> 
> Btw what about the polarity. Do I have to support it or can I
> return an error code if its != PWM_POLARITY_NORMAL? If so, which
> error code? EINVAL?

..ooOO(Did I really miss that during review? Bummer)

If your hardware only support normal polarity, only implement this and
return -EINVAL for inverted polarity requests.

> I know I could just invert the duty cycle in
> software, but shouldn't this be done in the core for any controller
> which doesn't support changing the polarity in hardware?

Please don't. This should indeed be done at the framework level. (But
I'm not convinced doing this unconditionally is a good idea.)

> > > +	/*
> > > +	 * The hardware doesn't allow to set max_duty_cycle if the
> > > +	 * 250Hz mode is enabled, thus we have to trap that here.
> > > +	 * But because a 100% duty cycle is equal on all modes, i.e.
> > 
> > It depends on how picky you are if you can agree here.
> 
> why is that? The only drawback is that the mode is changed without
> the user seeing it.

Ideally periods are completed before they change. So if a user requests
.period = .duty_cycle = 100ms with having the PWM disabled before and
afterwards, the expectation is that it is active for (an integer
multiple of) 100 ms. But honestly there are not many hardware
implementation + driver combos that get this right.

> 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. 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?) 

> > > +	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.)

> For example, have a look at pwm-iqs620a.c.
> Btw. the get_state might also fail, but there is no return value to
> return the error.

Yes, changing this is on my todo list.

> > If this cannot be avoided, please document this in the Limitations
> > paragraph.
> 
> Sure. There might be (or most likely are) gliches when you change the
> mode.

If you change only cycle but not mode, does the hardware complete the
currently running period? What about disable()?

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2020-07-13  8:48 UTC|newest]

Thread overview: 38+ 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
2020-07-06 17:53 ` [PATCH v5 02/13] mfd: add simple regmap based I2C driver Michael Walle
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 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 06/13] watchdog: add support for sl28cpld watchdog Michael Walle
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
2020-07-11 17:28     ` Michael Walle
2020-07-13  8:47       ` Uwe Kleine-König [this message]
2020-07-14 11:31         ` Michael Walle
2020-07-14 16:08           ` Uwe Kleine-König
2020-07-14 21:09             ` Michael Walle
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
2020-07-08  7:35   ` Linus Walleij
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 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 13/13] arm64: dts: freescale: sl28: enable fan support Michael Walle
2020-07-08  7:39   ` Linus Walleij
2020-07-08  7:56     ` Michael Walle

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=20200713084750.qj4hquzd6uz6y526@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --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=michael@walle.cc \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=thierry.reding@gmail.com \
    --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).