[v7,06/13] pwm: add support for sl28cpld PWM controller
diff mbox series

Message ID 20200803093559.12289-7-michael@walle.cc
State New
Headers show
Series
  • Add support for Kontron sl28cpld
Related show

Commit Message

Michael Walle Aug. 3, 2020, 9:35 a.m. UTC
Add support for the PWM controller of the sl28cpld board management
controller. This is part of a multi-function device driver.

The controller has one PWM channel and can just generate four distinct
frequencies.

Signed-off-by: Michael Walle <michael@walle.cc>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
---
Changes since v6:
 - added new row "period length"
 - fixed parenthesis mistake in the description of a calculation
 - added sl28cpld_pwm_{read/write}()
 - added more error messages

Changes since v5:
 - added brief description of the PWM hardware implementation
 - added hardware limitations
 - dropped the frequency mode table, instead calculate the prescaler
   value on the fly.
 - round the requested parameters instead of support just distinct
   periods.
 - prefix the macros by SL28CPLD_ to make them less generic
 - set polarity to PWM_POLARITY_NORMAL and reject inverted polarity
   requests.
 - apply the workaround just for prescaler value of 0.
 - make errors during probing more verbose

Changes since v4:
 - update copyright year
 - remove #include <linux/of_device.h>, suggested by Andy.
 - make the pwm mode table look nicer, suggested by Lee.
 - use dev_get_drvdata(chip->dev) instead of container_of(), suggested by
   Lee.
 - use whole sentence in comments, suggested by Lee.
 - renamed the local "struct sl28cpld_pwm" variable to "priv" everywhere,
   suggested by Lee.
 - use pwm_{get,set}_relative_duty_cycle(), suggested by Andy.
 - make the comment about the 250Hz hardware limitation clearer
 - don't use "if (ret < 0)", but only "if (ret)", suggested by Andy.
 - don't use KBUID_MODNAME
 - remove comma in terminator line of the compatible strings list
 - remove the platform device table

Changes since v3:
 - see cover letter

 drivers/pwm/Kconfig        |  10 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-sl28cpld.c | 235 +++++++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/pwm/pwm-sl28cpld.c

Comments

Uwe Kleine-König Aug. 6, 2020, 8:40 a.m. UTC | #1
Hello Michael,

I'm nearly happy now; see below.

On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf6973d33..a0d50d70c3b9 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -428,6 +428,16 @@ config PWM_SIFIVE
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sifive.
>  
> +config PWM_SL28CPLD
> +	tristate "Kontron sl28cpld PWM support"
> +	select MFD_SIMPLE_MFD_I2C

Is it sensible to present this option to everyone? Maybe

	depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST

.

> +	help
> +	  Generic PWM framework driver for board management controller
> +	  found on the Kontron sl28 CPLD.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-sl28cpld.
> +
>  config PWM_SPEAR
>  	tristate "STMicroelectronics SPEAr PWM support"
>  	depends on PLAT_SPEAR || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a03557..cbdcd55d69ee 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
>  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> +obj-$(CONFIG_PWM_SL28CPLD)	+= pwm-sl28cpld.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> new file mode 100644
> index 000000000000..bb298af36f0b
> --- /dev/null
> +++ b/drivers/pwm/pwm-sl28cpld.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sl28cpld PWM driver
> + *
> + * Copyright (c) 2020 Michael Walle <michael@walle.cc>
> + *
> + * There is no public datasheet available for this PWM core. But it is easy
> + * enough to be briefly explained. It consists of one 8-bit counter. The PWM
> + * supports four distinct frequencies by selecting when to reset the counter.
> + * With the prescaler setting you can select which bit of the counter is used
> + * to reset it. This implies that the higher the frequency the less remaining
> + * bits are available for the actual counter.
> + *
> + * Let cnt[7:0] be the counter, clocked at 32kHz:
> + * +-----------+--------+--------------+-----------+---------------+
> + * | prescaler |  reset | counter bits | frequency | period length |
> + * +-----------+--------+--------------+-----------+---------------+
> + * |         0 | cnt[7] |     cnt[6:0] |    250 Hz |    4000000 ns |
> + * |         1 | cnt[6] |     cnt[5:0] |    500 Hz |    2000000 ns |
> + * |         2 | cnt[5] |     cnt[4:0] |     1 kHz |    1000000 ns |
> + * |         3 | cnt[4] |     cnt[3:0] |     2 kHz |     500000 ns |
> + * +-----------+--------+--------------+-----------+---------------+
> + *
> + * Limitations:
> + * - The hardware cannot generate a 100% duty cycle if the prescaler is 0.
> + * - The hardware cannot atomically set the prescaler and the counter value,
> + *   which might lead to glitches and inconsistent states if a write fails.
> + * - The counter is not reset if you switch the prescaler which leads
> + *   to glitches, too.
> + * - The duty cycle will switch immediately and not after a complete cycle.
> + * - Depending on the actual implementation, disabling the PWM might have
> + *   side effects. For example, if the output pin is shared with a GPIO pin
> + *   it will automatically switch back to GPIO mode.

Very nice.

> + */
> +
> +#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 SL28CPLD_PWM_CTRL			0x00
> +#define   SL28CPLD_PWM_CTRL_ENABLE		BIT(7)
> +#define   SL28CPLD_PWM_CTRL_PRESCALER_MASK	GENMASK(1, 0)
> +#define SL28CPLD_PWM_CYCLE			0x01
> +#define   SL28CPLD_PWM_CYCLE_MAX		GENMASK(6, 0)
> +
> +#define SL28CPLD_PWM_CLK			32000 /* 32 kHz */
> +#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)	(1 << (7 - (prescaler)))
> +#define SL28CPLD_PWM_PERIOD(prescaler) \
> +	(NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler))
> +
> +/*
> + * We calculate the duty cycle like this:
> + *   duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle
> + *
> + * With
> + *   max_period_ns = 1 << (7 - prescaler) / pwm_clk * NSEC_PER_SEC
> + *   max_duty_cycle = 1 << (7 - prescaler)
> + * this then simplifies to:
> + *   duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC
> + *
> + * NSEC_PER_SEC and SL28CPLD_PWM_CLK is integer here, so we're not losing
> + * precision by doing the divison first.

Apart from the grammatical issue (s/is/are/) this is not the relevant
fact. The relevant thing is that NSEC_PER_SEC / SL28CPLD_PWM_CLK is
integer.

(In case this is not clear, assume SL28CPLD_PWM_CLK to be 30000 and reg
0x12345.

Then we have: 

	NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg) -> 0x94255749
	NSEC_PER_SEC * (reg) / SL28CPLD_PWM_CLK -> 0x9425b860

.)

> + */
> +#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \
> +	(NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg))
> +#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \
> +	(DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK))
> +
> +#define sl28cpld_pwm_read(priv, reg, val) \
> +	regmap_read((priv)->regmap, (priv)->offset + (reg), (val))
> +#define sl28cpld_pwm_write(priv, reg, val) \
> +	regmap_write((priv)->regmap, (priv)->offset + (reg), (val))
> +
> +struct sl28cpld_pwm {
> +	struct pwm_chip pwm_chip;
> +	struct regmap *regmap;
> +	u32 offset;
> +};
> +
> +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);
> +	unsigned int reg;
> +	int prescaler;
> +
> +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, &reg);
> +
> +	state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
> +
> +	prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
> +	state->period = SL28CPLD_PWM_PERIOD(prescaler);
> +
> +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, &reg);
> +	state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);

Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed that
the upper bits are zero?

> +	state->polarity = PWM_POLARITY_NORMAL;
> +}

Best regards
Uwe
Michael Walle Aug. 7, 2020, 7:28 a.m. UTC | #2
Hi Uwe, Hi Lee,

Am 2020-08-06 10:40, schrieb Uwe Kleine-König:
> On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 7dbcf6973d33..a0d50d70c3b9 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -428,6 +428,16 @@ config PWM_SIFIVE
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-sifive.
>> 
>> +config PWM_SL28CPLD
>> +	tristate "Kontron sl28cpld PWM support"
>> +	select MFD_SIMPLE_MFD_I2C
> 
> Is it sensible to present this option to everyone? Maybe
> 
> 	depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST

Because there is now no real MFD driver anymore, there is also
no symbol for that. The closest would be ARCH_ARM64 but I don't
think that is a good idea.

Lee, what do you think about adding a symbol to the MFD, which
selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules?

I.e.
config MFD_SL28CPLD
     tristate "Kontron sl28cpld"
     select MFD_SIMPLE_MFD_I2C
     help
       Say yes here to add support for the Kontron sl28cpld board
       management controller.

Then all the other device driver could depend on the MFD_SL28CPLD
symbol.

[..]

>> +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);
>> +	unsigned int reg;
>> +	int prescaler;
>> +
>> +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, &reg);
>> +
>> +	state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
>> +
>> +	prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
>> +	state->period = SL28CPLD_PWM_PERIOD(prescaler);
>> +
>> +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, &reg);
>> +	state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);
> 
> Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed 
> that
> the upper bits are zero?

Mh, the hardware guarantees that bit7 is zero. So masking with
SL28CPLD_PWM_CYCLE_MAX won't buy us much. But what I could think
could go wrong is this: someone set the prescaler to != 0 and the
duty cycle to a value greater than the max value for this particular
prescaler mode. For the above calculations this would result in a
duty_cycle greater than the period, if I'm not mistaken.

The behavior of the hardware is undefined in that case (at the moment
it will be always on, I guess). So this isn't a valid setting.
Nevertheless it might happen. So what about the following:

state->duty_cycle = min(state->duty_cycle, state->period);

-michael
Uwe Kleine-König Aug. 7, 2020, 7:45 a.m. UTC | #3
On Fri, Aug 07, 2020 at 09:28:31AM +0200, Michael Walle wrote:
> Hi Uwe, Hi Lee,
> 
> Am 2020-08-06 10:40, schrieb Uwe Kleine-König:
> > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 7dbcf6973d33..a0d50d70c3b9 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -428,6 +428,16 @@ config PWM_SIFIVE
> > >  	  To compile this driver as a module, choose M here: the module
> > >  	  will be called pwm-sifive.
> > > 
> > > +config PWM_SL28CPLD
> > > +	tristate "Kontron sl28cpld PWM support"
> > > +	select MFD_SIMPLE_MFD_I2C
> > 
> > Is it sensible to present this option to everyone? Maybe
> > 
> > 	depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST
> 
> Because there is now no real MFD driver anymore, there is also
> no symbol for that. The closest would be ARCH_ARM64 but I don't
> think that is a good idea.
> 
> Lee, what do you think about adding a symbol to the MFD, which
> selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules?
> 
> I.e.
> config MFD_SL28CPLD
>     tristate "Kontron sl28cpld"
>     select MFD_SIMPLE_MFD_I2C
>     help
>       Say yes here to add support for the Kontron sl28cpld board
>       management controller.
> 
> Then all the other device driver could depend on the MFD_SL28CPLD
> symbol.
> 
> [..]
> 
> > > +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);
> > > +	unsigned int reg;
> > > +	int prescaler;
> > > +
> > > +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, &reg);
> > > +
> > > +	state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
> > > +
> > > +	prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
> > > +	state->period = SL28CPLD_PWM_PERIOD(prescaler);
> > > +
> > > +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, &reg);
> > > +	state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);
> > 
> > Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed that
> > the upper bits are zero?
> 
> Mh, the hardware guarantees that bit7 is zero. So masking with
> SL28CPLD_PWM_CYCLE_MAX won't buy us much. But what I could think
> could go wrong is this: someone set the prescaler to != 0 and the
> duty cycle to a value greater than the max value for this particular
> prescaler mode. For the above calculations this would result in a
> duty_cycle greater than the period, if I'm not mistaken.
> 
> The behavior of the hardware is undefined in that case (at the moment
> it will be always on, I guess). So this isn't a valid setting.
> Nevertheless it might happen. So what about the following:
> 
> state->duty_cycle = min(state->duty_cycle, state->period);

If you care about this: This can also happen (at least shortly) in
sl28cpld_pwm_apply() as you write SL28CPLD_PWM_CTRL before
SL28CPLD_PWM_CYCLE there.

I wonder if we want to sanitize the values returned from driver's
.get_state in the core; or scream loud (maybe only if PWM_DEBUG is on).

Something like:

	if (state->enabled && state->duty_cycle > state->period) {
		if (IS_ENABLED(CONFIG_PWM_DEBUG))
			dev_warn(chip->dev, ".get_state() returned invalid setting.\n");

		state->duty_cycle = state->period;
	}

Do we want to catch state->period = 0, too? Do we interpret this as
disabled?

Best regards
Uwe
Michael Walle Aug. 7, 2020, 7:55 a.m. UTC | #4
Am 2020-08-07 09:45, schrieb Uwe Kleine-König:
> On Fri, Aug 07, 2020 at 09:28:31AM +0200, Michael Walle wrote:
>> Hi Uwe, Hi Lee,
>> 
>> Am 2020-08-06 10:40, schrieb Uwe Kleine-König:
>> > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:
>> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> > > index 7dbcf6973d33..a0d50d70c3b9 100644
>> > > --- a/drivers/pwm/Kconfig
>> > > +++ b/drivers/pwm/Kconfig
>> > > @@ -428,6 +428,16 @@ config PWM_SIFIVE
>> > >  	  To compile this driver as a module, choose M here: the module
>> > >  	  will be called pwm-sifive.
>> > >
>> > > +config PWM_SL28CPLD
>> > > +	tristate "Kontron sl28cpld PWM support"
>> > > +	select MFD_SIMPLE_MFD_I2C
>> >
>> > Is it sensible to present this option to everyone? Maybe
>> >
>> > 	depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST
>> 
>> Because there is now no real MFD driver anymore, there is also
>> no symbol for that. The closest would be ARCH_ARM64 but I don't
>> think that is a good idea.
>> 
>> Lee, what do you think about adding a symbol to the MFD, which
>> selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules?
>> 
>> I.e.
>> config MFD_SL28CPLD
>>     tristate "Kontron sl28cpld"
>>     select MFD_SIMPLE_MFD_I2C
>>     help
>>       Say yes here to add support for the Kontron sl28cpld board
>>       management controller.
>> 
>> Then all the other device driver could depend on the MFD_SL28CPLD
>> symbol.
>> 
>> [..]
>> 
>> > > +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);
>> > > +	unsigned int reg;
>> > > +	int prescaler;
>> > > +
>> > > +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, &reg);
>> > > +
>> > > +	state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
>> > > +
>> > > +	prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
>> > > +	state->period = SL28CPLD_PWM_PERIOD(prescaler);
>> > > +
>> > > +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, &reg);
>> > > +	state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);
>> >
>> > Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed that
>> > the upper bits are zero?
>> 
>> Mh, the hardware guarantees that bit7 is zero. So masking with
>> SL28CPLD_PWM_CYCLE_MAX won't buy us much. But what I could think
>> could go wrong is this: someone set the prescaler to != 0 and the
>> duty cycle to a value greater than the max value for this particular
>> prescaler mode. For the above calculations this would result in a
>> duty_cycle greater than the period, if I'm not mistaken.
>> 
>> The behavior of the hardware is undefined in that case (at the moment
>> it will be always on, I guess). So this isn't a valid setting.
>> Nevertheless it might happen. So what about the following:
>> 
>> state->duty_cycle = min(state->duty_cycle, state->period);
> 
> If you care about this: This can also happen (at least shortly) in
> sl28cpld_pwm_apply() as you write SL28CPLD_PWM_CTRL before
> SL28CPLD_PWM_CYCLE there.

It could also happen if it was the other way around, couldn't it?
Changing modes might glitch.

I care more about returning valid values to the PWM core ;)

-michael
Uwe Kleine-König Aug. 7, 2020, 10:24 a.m. UTC | #5
Hi Michael,

On Fri, Aug 07, 2020 at 09:55:19AM +0200, Michael Walle wrote:
> Am 2020-08-07 09:45, schrieb Uwe Kleine-König:
> > On Fri, Aug 07, 2020 at 09:28:31AM +0200, Michael Walle wrote:
> > > Am 2020-08-06 10:40, schrieb Uwe Kleine-König:
> > > > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:
> > > > > +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);
> > > > > +	unsigned int reg;
> > > > > +	int prescaler;
> > > > > +
> > > > > +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, &reg);
> > > > > +
> > > > > +	state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
> > > > > +
> > > > > +	prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
> > > > > +	state->period = SL28CPLD_PWM_PERIOD(prescaler);
> > > > > +
> > > > > +	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, &reg);
> > > > > +	state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);
> > > >
> > > > Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed that
> > > > the upper bits are zero?
> > > 
> > > Mh, the hardware guarantees that bit7 is zero. So masking with
> > > SL28CPLD_PWM_CYCLE_MAX won't buy us much. But what I could think
> > > could go wrong is this: someone set the prescaler to != 0 and the
> > > duty cycle to a value greater than the max value for this particular
> > > prescaler mode. For the above calculations this would result in a
> > > duty_cycle greater than the period, if I'm not mistaken.
> > > 
> > > The behavior of the hardware is undefined in that case (at the moment
> > > it will be always on, I guess). So this isn't a valid setting.
> > > Nevertheless it might happen. So what about the following:
> > > 
> > > state->duty_cycle = min(state->duty_cycle, state->period);
> > 
> > If you care about this: This can also happen (at least shortly) in
> > sl28cpld_pwm_apply() as you write SL28CPLD_PWM_CTRL before
> > SL28CPLD_PWM_CYCLE there.
> 
> It could also happen if it was the other way around, couldn't it?
> Changing modes might glitch.

If you want to prevent this, you have to order the writes depending on
prescaler increasing or decreasing.

Best regards
Uwe
Lee Jones Aug. 10, 2020, 7:13 a.m. UTC | #6
On Fri, 07 Aug 2020, Michael Walle wrote:

> Hi Uwe, Hi Lee,
> 
> Am 2020-08-06 10:40, schrieb Uwe Kleine-König:
> > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 7dbcf6973d33..a0d50d70c3b9 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -428,6 +428,16 @@ config PWM_SIFIVE
> > >  	  To compile this driver as a module, choose M here: the module
> > >  	  will be called pwm-sifive.
> > > 
> > > +config PWM_SL28CPLD
> > > +	tristate "Kontron sl28cpld PWM support"
> > > +	select MFD_SIMPLE_MFD_I2C
> > 
> > Is it sensible to present this option to everyone? Maybe
> > 
> > 	depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST
> 
> Because there is now no real MFD driver anymore, there is also
> no symbol for that. The closest would be ARCH_ARM64 but I don't
> think that is a good idea.
> 
> Lee, what do you think about adding a symbol to the MFD, which
> selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules?
> 
> I.e.
> config MFD_SL28CPLD
>     tristate "Kontron sl28cpld"
>     select MFD_SIMPLE_MFD_I2C
>     help
>       Say yes here to add support for the Kontron sl28cpld board
>       management controller.
> 
> Then all the other device driver could depend on the MFD_SL28CPLD
> symbol.

You want to add a virtual symbol to prevent having to present a real
one?  How is that a reasonable solution?
Michael Walle Aug. 10, 2020, 7:31 a.m. UTC | #7
Am 2020-08-10 09:13, schrieb Lee Jones:
> On Fri, 07 Aug 2020, Michael Walle wrote:
> 
>> Hi Uwe, Hi Lee,
>> 
>> Am 2020-08-06 10:40, schrieb Uwe Kleine-König:
>> > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:
>> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> > > index 7dbcf6973d33..a0d50d70c3b9 100644
>> > > --- a/drivers/pwm/Kconfig
>> > > +++ b/drivers/pwm/Kconfig
>> > > @@ -428,6 +428,16 @@ config PWM_SIFIVE
>> > >  	  To compile this driver as a module, choose M here: the module
>> > >  	  will be called pwm-sifive.
>> > >
>> > > +config PWM_SL28CPLD
>> > > +	tristate "Kontron sl28cpld PWM support"
>> > > +	select MFD_SIMPLE_MFD_I2C
>> >
>> > Is it sensible to present this option to everyone? Maybe
>> >
>> > 	depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST
>> 
>> Because there is now no real MFD driver anymore, there is also
>> no symbol for that. The closest would be ARCH_ARM64 but I don't
>> think that is a good idea.
>> 
>> Lee, what do you think about adding a symbol to the MFD, which
>> selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules?
>> 
>> I.e.
>> config MFD_SL28CPLD
>>     tristate "Kontron sl28cpld"
>>     select MFD_SIMPLE_MFD_I2C
>>     help
>>       Say yes here to add support for the Kontron sl28cpld board
>>       management controller.
>> 
>> Then all the other device driver could depend on the MFD_SL28CPLD
>> symbol.
> 
> You want to add a virtual symbol to prevent having to present a real
> one?  How is that a reasonable solution?

(1) Its a symbol on which all sl28cpld will depend on. Thus they will
     all be hidden if that is not set.
(2) the drivers itself wouldn't need to depend on MFD_SIMPLE_MFD_I2C,
     which is more correct, because they don't have anything to do with
     i2c.

-michael
Michael Walle Aug. 13, 2020, 7:09 a.m. UTC | #8
Am 2020-08-10 09:31, schrieb Michael Walle:
> Am 2020-08-10 09:13, schrieb Lee Jones:
>> On Fri, 07 Aug 2020, Michael Walle wrote:
>> 
>>> Hi Uwe, Hi Lee,
>>> 
>>> Am 2020-08-06 10:40, schrieb Uwe Kleine-König:
>>> > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:
>>> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> > > index 7dbcf6973d33..a0d50d70c3b9 100644
>>> > > --- a/drivers/pwm/Kconfig
>>> > > +++ b/drivers/pwm/Kconfig
>>> > > @@ -428,6 +428,16 @@ config PWM_SIFIVE
>>> > >  	  To compile this driver as a module, choose M here: the module
>>> > >  	  will be called pwm-sifive.
>>> > >
>>> > > +config PWM_SL28CPLD
>>> > > +	tristate "Kontron sl28cpld PWM support"
>>> > > +	select MFD_SIMPLE_MFD_I2C
>>> >
>>> > Is it sensible to present this option to everyone? Maybe
>>> >
>>> > 	depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST
>>> 
>>> Because there is now no real MFD driver anymore, there is also
>>> no symbol for that. The closest would be ARCH_ARM64 but I don't
>>> think that is a good idea.
>>> 
>>> Lee, what do you think about adding a symbol to the MFD, which
>>> selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules?
>>> 
>>> I.e.
>>> config MFD_SL28CPLD
>>>     tristate "Kontron sl28cpld"
>>>     select MFD_SIMPLE_MFD_I2C
>>>     help
>>>       Say yes here to add support for the Kontron sl28cpld board
>>>       management controller.
>>> 
>>> Then all the other device driver could depend on the MFD_SL28CPLD
>>> symbol.
>> 
>> You want to add a virtual symbol to prevent having to present a real
>> one?  How is that a reasonable solution?
> 
> (1) Its a symbol on which all sl28cpld will depend on. Thus they will
>     all be hidden if that is not set.
> (2) the drivers itself wouldn't need to depend on MFD_SIMPLE_MFD_I2C,
>     which is more correct, because they don't have anything to do with
>     i2c.

Lee, would you accept such a symbol? Otherwise, I'd leave it as is.

-michael
Lee Jones Aug. 13, 2020, 8:21 a.m. UTC | #9
On Mon, 10 Aug 2020, Michael Walle wrote:

> Am 2020-08-10 09:13, schrieb Lee Jones:
> > On Fri, 07 Aug 2020, Michael Walle wrote:
> > 
> > > Hi Uwe, Hi Lee,
> > > 
> > > Am 2020-08-06 10:40, schrieb Uwe Kleine-König:
> > > > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index 7dbcf6973d33..a0d50d70c3b9 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -428,6 +428,16 @@ config PWM_SIFIVE
> > > > >  	  To compile this driver as a module, choose M here: the module
> > > > >  	  will be called pwm-sifive.
> > > > >
> > > > > +config PWM_SL28CPLD
> > > > > +	tristate "Kontron sl28cpld PWM support"
> > > > > +	select MFD_SIMPLE_MFD_I2C
> > > >
> > > > Is it sensible to present this option to everyone? Maybe
> > > >
> > > > 	depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST
> > > 
> > > Because there is now no real MFD driver anymore, there is also
> > > no symbol for that. The closest would be ARCH_ARM64 but I don't
> > > think that is a good idea.
> > > 
> > > Lee, what do you think about adding a symbol to the MFD, which
> > > selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules?
> > > 
> > > I.e.
> > > config MFD_SL28CPLD
> > >     tristate "Kontron sl28cpld"
> > >     select MFD_SIMPLE_MFD_I2C
> > >     help
> > >       Say yes here to add support for the Kontron sl28cpld board
> > >       management controller.
> > > 
> > > Then all the other device driver could depend on the MFD_SL28CPLD
> > > symbol.
> > 
> > You want to add a virtual symbol to prevent having to present a real
> > one?  How is that a reasonable solution?
> 
> (1) Its a symbol on which all sl28cpld will depend on. Thus they will
>     all be hidden if that is not set.
> (2) the drivers itself wouldn't need to depend on MFD_SIMPLE_MFD_I2C,
>     which is more correct, because they don't have anything to do with
>     i2c.

Yes, okay.

Patch
diff mbox series

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7dbcf6973d33..a0d50d70c3b9 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -428,6 +428,16 @@  config PWM_SIFIVE
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sifive.
 
+config PWM_SL28CPLD
+	tristate "Kontron sl28cpld PWM support"
+	select MFD_SIMPLE_MFD_I2C
+	help
+	  Generic PWM framework driver for board management controller
+	  found on the Kontron sl28 CPLD.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sl28cpld.
+
 config PWM_SPEAR
 	tristate "STMicroelectronics SPEAr PWM support"
 	depends on PLAT_SPEAR || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 2c2ba0a03557..cbdcd55d69ee 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -40,6 +40,7 @@  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
+obj-$(CONFIG_PWM_SL28CPLD)	+= pwm-sl28cpld.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
new file mode 100644
index 000000000000..bb298af36f0b
--- /dev/null
+++ b/drivers/pwm/pwm-sl28cpld.c
@@ -0,0 +1,235 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld PWM driver
+ *
+ * Copyright (c) 2020 Michael Walle <michael@walle.cc>
+ *
+ * There is no public datasheet available for this PWM core. But it is easy
+ * enough to be briefly explained. It consists of one 8-bit counter. The PWM
+ * supports four distinct frequencies by selecting when to reset the counter.
+ * With the prescaler setting you can select which bit of the counter is used
+ * to reset it. This implies that the higher the frequency the less remaining
+ * bits are available for the actual counter.
+ *
+ * Let cnt[7:0] be the counter, clocked at 32kHz:
+ * +-----------+--------+--------------+-----------+---------------+
+ * | prescaler |  reset | counter bits | frequency | period length |
+ * +-----------+--------+--------------+-----------+---------------+
+ * |         0 | cnt[7] |     cnt[6:0] |    250 Hz |    4000000 ns |
+ * |         1 | cnt[6] |     cnt[5:0] |    500 Hz |    2000000 ns |
+ * |         2 | cnt[5] |     cnt[4:0] |     1 kHz |    1000000 ns |
+ * |         3 | cnt[4] |     cnt[3:0] |     2 kHz |     500000 ns |
+ * +-----------+--------+--------------+-----------+---------------+
+ *
+ * Limitations:
+ * - The hardware cannot generate a 100% duty cycle if the prescaler is 0.
+ * - The hardware cannot atomically set the prescaler and the counter value,
+ *   which might lead to glitches and inconsistent states if a write fails.
+ * - The counter is not reset if you switch the prescaler which leads
+ *   to glitches, too.
+ * - The duty cycle will switch immediately and not after a complete cycle.
+ * - Depending on the actual implementation, disabling the PWM might have
+ *   side effects. For example, if the output pin is shared with a GPIO pin
+ *   it will automatically switch back to GPIO mode.
+ */
+
+#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 SL28CPLD_PWM_CTRL			0x00
+#define   SL28CPLD_PWM_CTRL_ENABLE		BIT(7)
+#define   SL28CPLD_PWM_CTRL_PRESCALER_MASK	GENMASK(1, 0)
+#define SL28CPLD_PWM_CYCLE			0x01
+#define   SL28CPLD_PWM_CYCLE_MAX		GENMASK(6, 0)
+
+#define SL28CPLD_PWM_CLK			32000 /* 32 kHz */
+#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)	(1 << (7 - (prescaler)))
+#define SL28CPLD_PWM_PERIOD(prescaler) \
+	(NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler))
+
+/*
+ * We calculate the duty cycle like this:
+ *   duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle
+ *
+ * With
+ *   max_period_ns = 1 << (7 - prescaler) / pwm_clk * NSEC_PER_SEC
+ *   max_duty_cycle = 1 << (7 - prescaler)
+ * this then simplifies to:
+ *   duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC
+ *
+ * NSEC_PER_SEC and SL28CPLD_PWM_CLK is integer here, so we're not losing
+ * precision by doing the divison first.
+ */
+#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \
+	(NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg))
+#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \
+	(DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK))
+
+#define sl28cpld_pwm_read(priv, reg, val) \
+	regmap_read((priv)->regmap, (priv)->offset + (reg), (val))
+#define sl28cpld_pwm_write(priv, reg, val) \
+	regmap_write((priv)->regmap, (priv)->offset + (reg), (val))
+
+struct sl28cpld_pwm {
+	struct pwm_chip pwm_chip;
+	struct regmap *regmap;
+	u32 offset;
+};
+
+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);
+	unsigned int reg;
+	int prescaler;
+
+	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, &reg);
+
+	state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
+
+	prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
+	state->period = SL28CPLD_PWM_PERIOD(prescaler);
+
+	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, &reg);
+	state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);
+	state->polarity = PWM_POLARITY_NORMAL;
+}
+
+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);
+	unsigned int cycle, prescaler;
+	int ret;
+	u8 ctrl;
+
+	/* Polarity inversion is not supported */
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	/*
+	 * Calculate the prescaler. Pick the biggest period that isn't
+	 * bigger than the requested period.
+	 */
+	prescaler = DIV_ROUND_UP_ULL(SL28CPLD_PWM_PERIOD(0), state->period);
+	prescaler = order_base_2(prescaler);
+
+	if (prescaler > field_max(SL28CPLD_PWM_CTRL_PRESCALER_MASK))
+		return -ERANGE;
+
+	ctrl = FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, prescaler);
+	if (state->enabled)
+		ctrl |= SL28CPLD_PWM_CTRL_ENABLE;
+
+	cycle = SL28CPLD_PWM_FROM_DUTY_CYCLE(state->duty_cycle);
+	cycle = min_t(unsigned int, cycle, SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler));
+
+	/*
+	 * Work around the hardware limitation. See also above. Trap 100% duty
+	 * cycle if the prescaler is 0. Set prescaler to 1 instead. We don't
+	 * care about the frequency because its "all-one" in either case.
+	 *
+	 * We don't need to check the actual prescaler setting, because only
+	 * if the prescaler is 0 we can have this particular value.
+	 */
+	if (cycle == SL28CPLD_PWM_MAX_DUTY_CYCLE(0)) {
+		ctrl &= ~SL28CPLD_PWM_CTRL_PRESCALER_MASK;
+		ctrl |= FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, 1);
+		cycle = SL28CPLD_PWM_MAX_DUTY_CYCLE(1);
+	}
+
+	ret = sl28cpld_pwm_write(priv, SL28CPLD_PWM_CTRL, ctrl);
+	if (ret)
+		return ret;
+
+	return sl28cpld_pwm_write(priv, SL28CPLD_PWM_CYCLE, cycle);
+}
+
+static const struct pwm_ops sl28cpld_pwm_ops = {
+	.apply = sl28cpld_pwm_apply,
+	.get_state = sl28cpld_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int sl28cpld_pwm_probe(struct platform_device *pdev)
+{
+	struct sl28cpld_pwm *priv;
+	struct pwm_chip *chip;
+	int ret;
+
+	if (!pdev->dev.parent) {
+		dev_err(&pdev->dev, "no parent device\n");
+		return -ENODEV;
+	}
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!priv->regmap) {
+		dev_err(&pdev->dev, "could not get parent regmap\n");
+		return -ENODEV;
+	}
+
+	ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset);
+	if (ret) {
+		dev_err(&pdev->dev, "no 'reg' property found (%pe)\n",
+			ERR_PTR(ret));
+		return -EINVAL;
+	}
+
+	/* Initialize the pwm_chip structure */
+	chip = &priv->pwm_chip;
+	chip->dev = &pdev->dev;
+	chip->ops = &sl28cpld_pwm_ops;
+	chip->base = -1;
+	chip->npwm = 1;
+
+	ret = pwmchip_add(&priv->pwm_chip);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add PWM chip (%pe)",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int sl28cpld_pwm_remove(struct platform_device *pdev)
+{
+	struct sl28cpld_pwm *priv = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&priv->pwm_chip);
+}
+
+static const struct of_device_id sl28cpld_pwm_of_match[] = {
+	{ .compatible = "kontron,sl28cpld-pwm" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sl28cpld_pwm_of_match);
+
+static struct platform_driver sl28cpld_pwm_driver = {
+	.probe = sl28cpld_pwm_probe,
+	.remove	= sl28cpld_pwm_remove,
+	.driver = {
+		.name = "sl28cpld-pwm",
+		.of_match_table = sl28cpld_pwm_of_match,
+	},
+};
+module_platform_driver(sl28cpld_pwm_driver);
+
+MODULE_DESCRIPTION("sl28cpld PWM Driver");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_LICENSE("GPL");