| Message ID | 20200803093559.12289-7-michael@walle.cc (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
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, ®); > + > + 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, ®); > + 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
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, ®); >> + >> + 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, ®); >> + 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
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, ®); > > > + > > > + 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, ®); > > > + 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
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, ®); >> > > + >> > > + 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, ®); >> > > + 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
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, ®); > > > > > + > > > > > + 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, ®); > > > > > + 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
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?
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
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
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.
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, ®); + + 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, ®); + 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");