| Message ID | 20240831-en7581-pinctrl-v3-5-98eebfb4da66@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add mfd, pinctrl and pwm support to EN7581 SoC | expand |
Hello, On Sat, Aug 31, 2024 at 04:27:50PM +0200, Lorenzo Bianconi wrote: > From: Benjamin Larsson <benjamin.larsson@genexis.eu> > > Introduce driver for PWM module available on EN7581 SoC. > > Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu> > Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > drivers/pwm/Kconfig | 10 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-airoha.c | 435 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 446 insertions(+) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 3e53838990f5..0a78bda0707d 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -47,6 +47,16 @@ config PWM_AB8500 > To compile this driver as a module, choose M here: the module > will be called pwm-ab8500. > > +config PWM_AIROHA > + tristate "Airoha PWM support" > + depends on ARCH_AIROHA || COMPILE_TEST > + depends on OF > + help > + Generic PWM framework driver for Airoha SoC. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-airoha. > + > config PWM_APPLE > tristate "Apple SoC PWM support" > depends on ARCH_APPLE || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 0be4f3e6dd43..7ee61822d88d 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_PWM) += core.o > obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o > +obj-$(CONFIG_PWM_AIROHA) += pwm-airoha.o > obj-$(CONFIG_PWM_APPLE) += pwm-apple.o > obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o > obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o > diff --git a/drivers/pwm/pwm-airoha.c b/drivers/pwm/pwm-airoha.c > new file mode 100644 > index 000000000000..54dc12d20da4 > --- /dev/null > +++ b/drivers/pwm/pwm-airoha.c > @@ -0,0 +1,435 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2022 Markus Gothe <markus.gothe@genexis.eu> > + */ Would you please add a "Limitations" paragraph here covering the following questions: - How does the hardware behave on changes of configuration (does it complete the currently running period? Are there any glitches?) - How does the hardware behave on disabling? Please stick to the format used in several other drivers such that sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c emits the informations. > + > +#include <linux/bitfield.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/iopoll.h> > +#include <linux/mfd/airoha-en7581-mfd.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/gpio.h> > +#include <linux/bitops.h> > +#include <asm/div64.h> > + > +#define REG_SGPIO_CFG 0x0024 > +#define REG_FLASH_CFG 0x0038 > +#define REG_CYCLE_CFG 0x0098 > + > +#define REG_SGPIO_LED_DATE 0x0000 > +#define SGPIO_LED_SHIFT_FLAG BIT(31) > +#define SGPIO_LED_DATA GENMASK(16, 0) Please make the bit fields's names start with the register name. > +#define REG_SGPIO_CLK_DIVR 0x0004 > +#define REG_SGPIO_CLK_DLY 0x0008 > + > +#define REG_SIPO_FLASH_MODE_CFG 0x000c > +#define SERIAL_GPIO_FLASH_MODE BIT(1) > +#define SERIAL_GPIO_MODE BIT(0) > + > +#define REG_GPIO_FLASH_PRD_SET(_n) (0x0004 + ((_n) << 2)) > +#define GPIO_FLASH_PRD_MASK(_n) GENMASK(15 + ((_n) << 4), ((_n) << 4)) > + > +#define REG_GPIO_FLASH_MAP(_n) (0x0014 + ((_n) << 2)) > +#define GPIO_FLASH_SETID_MASK(_n) GENMASK(2 + ((_n) << 2), ((_n) << 2)) > +#define GPIO_FLASH_EN(_n) BIT(3 + ((_n) << 2)) > + > +#define REG_SIPO_FLASH_MAP(_n) (0x001c + ((_n) << 2)) > + > +#define REG_CYCLE_CFG_VALUE(_n) (0x0000 + ((_n) << 2)) > +#define WAVE_GEN_CYCLE_MASK(_n) GENMASK(7 + ((_n) << 3), ((_n) << 3)) > + > +struct airoha_pwm { > + void __iomem *sgpio_cfg; > + void __iomem *flash_cfg; > + void __iomem *cycle_cfg; > + > + struct device_node *np; > + u64 initialized; > + > + struct { > + /* Bitmask of PWM channels using this bucket */ > + u64 used; > + u64 period_ns; > + u64 duty_ns; > + enum pwm_polarity polarity; > + } bucket[8]; > +}; > + > +/* > + * The first 16 GPIO pins, GPIO0-GPIO15, are mapped into 16 PWM channels, 0-15. > + * The SIPO GPIO pins are 16 pins which are mapped into 17 PWM channels, 16-32. How can 16 pins be mapped to 17 PWM channels? > + * However, we've only got 8 concurrent waveform generators and can therefore > + * only use up to 8 different combinations of duty cycle and period at a time. That's an information to add to the Limitations paragraph. > + */ > +#define PWM_NUM_GPIO 16 > +#define PWM_NUM_SIPO 17 > + > +/* The PWM hardware supports periods between 4 ms and 1 s */ > +#define PERIOD_MIN_NS 4000000 > +#define PERIOD_MAX_NS 1000000000 > +/* It is represented internally as 1/250 s between 1 and 250 */ > +#define PERIOD_MIN 1 > +#define PERIOD_MAX 250 > +/* Duty cycle is relative with 255 corresponding to 100% */ > +#define DUTY_FULL 255 > + > +static u32 airoha_pwm_rmw(struct airoha_pwm *pc, void __iomem *addr, > + u32 mask, u32 val) > +{ > + val |= (readl(addr) & ~mask); > + writel(val, addr); > + > + return val; > +} pc is unused here, please drop it. > +#define airoha_pwm_sgpio_rmw(pc, offset, mask, val) \ > + airoha_pwm_rmw((pc), (pc)->sgpio_cfg + (offset), (mask), (val)) > +#define airoha_pwm_flash_rmw(pc, offset, mask, val) \ > + airoha_pwm_rmw((pc), (pc)->flash_cfg + (offset), (mask), (val)) > +#define airoha_pwm_cycle_rmw(pc, offset, mask, val) \ > + airoha_pwm_rmw((pc), (pc)->cycle_cfg + (offset), (mask), (val)) > + > +#define airoha_pwm_sgpio_set(pc, offset, val) \ > + airoha_pwm_sgpio_rmw((pc), (offset), 0, (val)) That does the right thing, but I'd consider #define airoha_pwm_sgpio_set(pc, offset, val) \ airoha_pwm_sgpio_rmw((pc), (offset), (val), (val)) to be more understandable (or ~0 instead of the last (val)?) > +#define airoha_pwm_sgpio_clear(pc, offset, mask) \ > + airoha_pwm_sgpio_rmw((pc), (offset), (mask), 0) > +#define airoha_pwm_flash_set(pc, offset, val) \ > + airoha_pwm_flash_rmw((pc), (offset), 0, (val)) > +#define airoha_pwm_flash_clear(pc, offset, mask) \ > + airoha_pwm_flash_rmw((pc), (offset), (mask), 0) > + > +static int airoha_pwm_get_waveform(struct airoha_pwm *pc, u64 duty_ns, > + u64 period_ns) Given that "waveform" will gain some specific semantic soon, but this usage is different, I'd like to see this function renamed. > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) { > + if (!pc->bucket[i].used) > + continue; > + > + if (duty_ns == pc->bucket[i].duty_ns && > + period_ns == pc->bucket[i].period_ns) > + return i; > + > + /* > + * Unlike duty cycle zero, which can be handled by > + * disabling PWM, a generator is needed for full duty > + * cycle but it can be reused regardless of period > + */ > + if (duty_ns == DUTY_FULL && pc->bucket[i].duty_ns == DUTY_FULL) > + return i; > + } > + > + return -1; > +} > + > +static void airoha_pwm_free_waveform(struct airoha_pwm *pc, unsigned int hwpwm) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) > + pc->bucket[i].used &= ~BIT_ULL(hwpwm); > +} > + > +static int airoha_pwm_consume_waveform(struct airoha_pwm *pc, > + u64 duty_ns, u64 period_ns, > + enum pwm_polarity polarity, > + unsigned int hwpwm) > +{ > + int id = airoha_pwm_get_waveform(pc, duty_ns, period_ns); > + > + if (id < 0) { > + int i; > + > + /* find an unused waveform generator */ > + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) { > + if (!(pc->bucket[i].used & ~BIT_ULL(hwpwm))) { > + id = i; > + break; > + } > + } > + } > + > + if (id >= 0) { > + airoha_pwm_free_waveform(pc, hwpwm); > + pc->bucket[id].used |= BIT_ULL(hwpwm); > + pc->bucket[id].period_ns = period_ns; > + pc->bucket[id].duty_ns = duty_ns; > + pc->bucket[id].polarity = polarity; > + } One downside of the (nearly) maximal flexibility implemented here is that if you have 9 (or more) requested pwm devices configuration is quite limited. So it might happen that a consumer changes the configuration for pwm#2 from pwm_state A to pwm_state B but cannot change back to A later. If you limit the number of requested pwm devices to 8, the code gets a tad simpler (because you can map a fixed bucket to each pwm device and don't need to search during .apply()) and each consumer has maximal freedom to configure its device. > + > + return id; > +} > + > +static int airoha_pwm_sipo_init(struct airoha_pwm *pc) > +{ > + u32 clk_divr_val = 3, sipo_clock_delay = 1; > + u32 val, sipo_clock_divisor = 32; > + > + if (!(pc->initialized >> PWM_NUM_GPIO)) > + return 0; > + > + /* Select the right shift register chip */ > + if (of_property_read_bool(pc->np, "hc74595")) > + airoha_pwm_sgpio_set(pc, REG_SIPO_FLASH_MODE_CFG, > + SERIAL_GPIO_MODE); > + else > + airoha_pwm_sgpio_clear(pc, REG_SIPO_FLASH_MODE_CFG, > + SERIAL_GPIO_MODE); > + > + if (!of_property_read_u32(pc->np, "sipo-clock-divisor", > + &sipo_clock_divisor)) { > + switch (sipo_clock_divisor) { > + case 4: > + clk_divr_val = 0; > + break; > + case 8: > + clk_divr_val = 1; > + break; > + case 16: > + clk_divr_val = 2; > + break; > + case 32: > + clk_divr_val = 3; > + break; > + default: > + return -EINVAL; > + } > + } > + /* Configure shift register timings */ > + writel(clk_divr_val, pc->sgpio_cfg + REG_SGPIO_CLK_DIVR); > + > + of_property_read_u32(pc->np, "sipo-clock-delay", &sipo_clock_delay); > + if (sipo_clock_delay < 1 || sipo_clock_delay > sipo_clock_divisor / 2) > + return -EINVAL; > + > + /* > + * The actual delay is sclkdly + 1 so subtract 1 from > + * sipo-clock-delay to calculate the register value > + */ > + sipo_clock_delay--; > + writel(sipo_clock_delay, pc->sgpio_cfg + REG_SGPIO_CLK_DLY); > + > + /* > + * It it necessary to after muxing explicitly shift out all > + * zeroes to initialize the shift register before enabling PWM > + * mode because in PWM mode SIPO will not start shifting until > + * it needs to output a non-zero value (bit 31 of led_data > + * indicates shifting in progress and it must return to zero > + * before led_data can be written or PWM mode can be set) > + */ > + if (readl_poll_timeout(pc->sgpio_cfg + REG_SGPIO_LED_DATE, val, > + !(val & SGPIO_LED_SHIFT_FLAG), 10, > + 200 * USEC_PER_MSEC)) > + return -ETIMEDOUT; > + > + airoha_pwm_sgpio_clear(pc, REG_SGPIO_LED_DATE, SGPIO_LED_DATA); > + if (readl_poll_timeout(pc->sgpio_cfg + REG_SGPIO_LED_DATE, val, > + !(val & SGPIO_LED_SHIFT_FLAG), 10, > + 200 * USEC_PER_MSEC)) > + return -ETIMEDOUT; > + > + /* Set SIPO in PWM mode */ > + airoha_pwm_sgpio_set(pc, REG_SIPO_FLASH_MODE_CFG, > + SERIAL_GPIO_FLASH_MODE); > + > + return 0; > +} > + > +static void airoha_pwm_config_waveform(struct airoha_pwm *pc, int index, > + u64 duty_ns, u64 period_ns, > + enum pwm_polarity polarity) > +{ > + u32 period, duty, mask, val; > + > + duty = clamp_val(div64_u64(DUTY_FULL * duty_ns, period_ns), 0, > + DUTY_FULL); DUTY_FULL * duty_ns might overflow. Also the calculation is wrong. For example if the following is requested: .period = 23999999, .duty_cycle = 8000000, you're supposed to configure period = 16000000 ns and duty_cycle = 8000000 ns. (I.e.: Pick the biggest possible period not bigger than the requested period. For that pick the biggest possible duty_cycle not bigger than the requested duty_cycle.) If you implement .get_state() in a way to return the actually configured state, enabling PWM_DEBUG and intensive testing helps to get this right. > + if (polarity == PWM_POLARITY_INVERSED) > + duty = DUTY_FULL - duty; This alone doesn't switch the polarity of the signal. Please only claim to be able to implement the settings that the hardware actually can do. > + period = clamp_val(div64_u64(25 * period_ns, 100000000), PERIOD_MIN, > + PERIOD_MAX); > + > + /* Configure frequency divisor */ > + mask = WAVE_GEN_CYCLE_MASK(index % 4); > + val = (period << __ffs(mask)) & mask; > + airoha_pwm_cycle_rmw(pc, REG_CYCLE_CFG_VALUE(index / 4), mask, val); > + > + /* Configure duty cycle */ > + duty = ((DUTY_FULL - duty) << 8) | duty; > + mask = GPIO_FLASH_PRD_MASK(index % 2); > + val = (duty << __ffs(mask)) & mask; > + airoha_pwm_flash_rmw(pc, REG_GPIO_FLASH_PRD_SET(index / 2), mask, val); > +} > + > +static void airoha_pwm_config_flash_map(struct airoha_pwm *pc, > + unsigned int hwpwm, int index) > +{ > + u32 addr, mask, val; > + > + if (hwpwm < PWM_NUM_GPIO) { > + addr = REG_GPIO_FLASH_MAP(hwpwm / 8); > + } else { > + addr = REG_SIPO_FLASH_MAP(hwpwm / 8); > + hwpwm -= PWM_NUM_GPIO; > + } > + > + if (index < 0) { > + /* > + * Change of waveform takes effect immediately but > + * disabling has some delay so to prevent glitching > + * only the enable bit is touched when disabling > + */ > + airoha_pwm_flash_clear(pc, addr, GPIO_FLASH_EN(hwpwm % 8)); > + return; > + } > + > + mask = GPIO_FLASH_SETID_MASK(hwpwm % 8); > + val = ((index & 7) << __ffs(mask)) & mask; > + airoha_pwm_flash_rmw(pc, addr, mask, val); > + airoha_pwm_flash_set(pc, addr, GPIO_FLASH_EN(hwpwm % 8)); > +} > + > +static int airoha_pwm_config(struct airoha_pwm *pc, struct pwm_device *pwm, > + u64 duty_ns, u64 period_ns, > + enum pwm_polarity polarity) > +{ > + int index = -1; > + > + index = airoha_pwm_consume_waveform(pc, duty_ns, period_ns, polarity, > + pwm->hwpwm); > + if (index < 0) > + return -EBUSY; > + > + if (!(pc->initialized & BIT_ULL(pwm->hwpwm)) && > + pwm->hwpwm >= PWM_NUM_GPIO) > + airoha_pwm_sipo_init(pc); > + > + if (index >= 0) { > + airoha_pwm_config_waveform(pc, index, duty_ns, period_ns, > + polarity); > + airoha_pwm_config_flash_map(pc, pwm->hwpwm, index); > + } else { > + airoha_pwm_config_flash_map(pc, pwm->hwpwm, index); > + airoha_pwm_free_waveform(pc, pwm->hwpwm); > + } > + > + pc->initialized |= BIT_ULL(pwm->hwpwm); > + > + return 0; > +} > + > +static void airoha_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct airoha_pwm *pc = pwmchip_get_drvdata(chip); > + > + /* Disable PWM and release the waveform */ > + airoha_pwm_config_flash_map(pc, pwm->hwpwm, -1); > + airoha_pwm_free_waveform(pc, pwm->hwpwm); > + > + pc->initialized &= ~BIT_ULL(pwm->hwpwm); > + if (!(pc->initialized >> PWM_NUM_GPIO)) > + airoha_pwm_sgpio_clear(pc, REG_SIPO_FLASH_MODE_CFG, > + SERIAL_GPIO_FLASH_MODE); > + > + /* > + * Clear the state to force re-initialization the next time > + * this PWM channel is used since we cannot retain state in > + * hardware due to limited number of waveform generators > + */ > + memset(&pwm->state, 0, sizeof(pwm->state)); Please don't reconfigure the hardware in .free(). Instead assume that the consumer disabled the PWM before releasing it or that they know what they do. Also don't write to pwm->state, that is supposed to only happen in the core. > +} > + > +static int airoha_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct airoha_pwm *pc = pwmchip_get_drvdata(chip); > + u64 duty = state->enabled ? state->duty_cycle : 0; > + > + if (!state->enabled) { > + airoha_pwm_free(chip, pwm); > + return 0; > + } > + > + if (state->period < PERIOD_MIN_NS || state->period > PERIOD_MAX_NS) > + return -EINVAL; Please handle state->period > PERIOD_MAX_NS by configuring a period of PERIOD_MAX_NS. > + return airoha_pwm_config(pc, pwm, duty, state->period, > + state->polarity); > +} > + > +static int airoha_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct airoha_pwm *pc = pwmchip_get_drvdata(chip); > + int i; > + > + /* find hwpwm in waveform generator bucket */ > + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) { > + if (pc->bucket[i].used & BIT_ULL(pwm->hwpwm)) { > + state->enabled = pc->initialized & BIT_ULL(pwm->hwpwm); > + state->polarity = pc->bucket[i].polarity; > + state->period = pc->bucket[i].period_ns; > + state->duty_cycle = pc->bucket[i].duty_ns; > + break; > + } > + } > + > + if (i == ARRAY_SIZE(pc->bucket)) > + state->enabled = false; > + > + return 0; > +} > + > +static const struct pwm_ops airoha_pwm_ops = { > + .get_state = airoha_pwm_get_state, > + .apply = airoha_pwm_apply, > + .free = airoha_pwm_free, > +}; > + > +static int airoha_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct airoha_mfd *mfd; > + struct airoha_pwm *pc; > + struct pwm_chip *chip; > + > + /* Assign parent MFD of_node to dev */ > + dev->of_node = of_node_get(dev->parent->of_node); I think you want device_set_of_node_from_dev(dev->parent, dev) here. > + mfd = dev_get_drvdata(dev->parent); > + > + chip = devm_pwmchip_alloc(dev, PWM_NUM_GPIO + PWM_NUM_SIPO, > + sizeof(*pc)); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + > + pc = pwmchip_get_drvdata(chip); > + pc->np = dev->of_node; > + pc->sgpio_cfg = mfd->base + REG_SGPIO_CFG; > + pc->flash_cfg = mfd->base + REG_FLASH_CFG; > + pc->cycle_cfg = mfd->base + REG_CYCLE_CFG; Is it really worth to store these values in the private data struct? My intuition would be to only store .base in pc and then define the register accessors macros like: #define airoha_pwm_cycle_rmw(pc, offset, mask, val) \ airoha_pwm_rmw((pc), (pc)->base + REG_CYCLE_CFG + (offset), (mask), (val)) > + chip->ops = &airoha_pwm_ops; > + chip->of_xlate = of_pwm_xlate_with_flags; Please don't assign .of_xlate, the core does if (!chip->of_xlate) chip->of_xlate = of_pwm_xlate_with_flags; > + > + return devm_pwmchip_add(&pdev->dev, chip); > +} > + > +static struct platform_driver airoha_pwm_driver = { > + .driver = { > + .name = "airoha-pwm", > + }, > + .probe = airoha_pwm_probe, > +}; > +module_platform_driver(airoha_pwm_driver); > + > +MODULE_AUTHOR("Lorenzo Bianconi <lorenzo@kernel.org>"); > +MODULE_AUTHOR("Markus Gothe <markus.gothe@genexis.eu>"); > +MODULE_AUTHOR("Benjamin Larsson <benjamin.larsson@genexis.eu>"); > +MODULE_DESCRIPTION("Airoha EN7581 PWM driver"); > +MODULE_LICENSE("GPL"); Best regards Uwe
Hi. On 2024-09-03 12:46, Uwe Kleine-König wrote: > Hello, > > On Sat, Aug 31, 2024 at 04:27:50PM +0200, Lorenzo Bianconi wrote: >> From: Benjamin Larsson <benjamin.larsson@genexis.eu> >> >> Introduce driver for PWM module available on EN7581 SoC. >> >> Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu> >> Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org> >> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >> --- >> drivers/pwm/Kconfig | 10 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-airoha.c | 435 +++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 446 insertions(+) >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index 3e53838990f5..0a78bda0707d 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -47,6 +47,16 @@ config PWM_AB8500 >> To compile this driver as a module, choose M here: the module >> will be called pwm-ab8500. >> >> +config PWM_AIROHA >> + tristate "Airoha PWM support" >> + depends on ARCH_AIROHA || COMPILE_TEST >> + depends on OF >> + help >> + Generic PWM framework driver for Airoha SoC. >> + >> + To compile this driver as a module, choose M here: the module >> + will be called pwm-airoha. >> + >> config PWM_APPLE >> tristate "Apple SoC PWM support" >> depends on ARCH_APPLE || COMPILE_TEST >> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile >> index 0be4f3e6dd43..7ee61822d88d 100644 >> --- a/drivers/pwm/Makefile >> +++ b/drivers/pwm/Makefile >> @@ -1,6 +1,7 @@ >> # SPDX-License-Identifier: GPL-2.0 >> obj-$(CONFIG_PWM) += core.o >> obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o >> +obj-$(CONFIG_PWM_AIROHA) += pwm-airoha.o >> obj-$(CONFIG_PWM_APPLE) += pwm-apple.o >> obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o >> obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o >> diff --git a/drivers/pwm/pwm-airoha.c b/drivers/pwm/pwm-airoha.c >> new file mode 100644 >> index 000000000000..54dc12d20da4 >> --- /dev/null >> +++ b/drivers/pwm/pwm-airoha.c >> @@ -0,0 +1,435 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright 2022 Markus Gothe <markus.gothe@genexis.eu> >> + */ > Would you please add a "Limitations" paragraph here covering the > following questions: > > - How does the hardware behave on changes of configuration (does it > complete the currently running period? Are there any glitches?) > - How does the hardware behave on disabling? > > Please stick to the format used in several other drivers such that > > sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c > > emits the informations. The answer to your questions are currently unknown. Is this information needed for a merge of the driver ? > >> + >> +#include <linux/bitfield.h> >> +#include <linux/err.h> >> +#include <linux/io.h> >> +#include <linux/iopoll.h> >> +#include <linux/mfd/airoha-en7581-mfd.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/pwm.h> >> +#include <linux/gpio.h> >> +#include <linux/bitops.h> >> +#include <asm/div64.h> >> + >> +#define REG_SGPIO_CFG 0x0024 >> +#define REG_FLASH_CFG 0x0038 >> +#define REG_CYCLE_CFG 0x0098 >> + >> +#define REG_SGPIO_LED_DATE 0x0000 >> +#define SGPIO_LED_SHIFT_FLAG BIT(31) >> +#define SGPIO_LED_DATA GENMASK(16, 0) > Please make the bit fields's names start with the register name. > I noticed REG_SGPIO_LED_DATE DATE should be DATA. >> +#define REG_SGPIO_CLK_DIVR 0x0004 >> +#define REG_SGPIO_CLK_DLY 0x0008 >> + >> +#define REG_SIPO_FLASH_MODE_CFG 0x000c >> +#define SERIAL_GPIO_FLASH_MODE BIT(1) >> +#define SERIAL_GPIO_MODE BIT(0) >> + >> +#define REG_GPIO_FLASH_PRD_SET(_n) (0x0004 + ((_n) << 2)) >> +#define GPIO_FLASH_PRD_MASK(_n) GENMASK(15 + ((_n) << 4), ((_n) << 4)) >> + >> +#define REG_GPIO_FLASH_MAP(_n) (0x0014 + ((_n) << 2)) >> +#define GPIO_FLASH_SETID_MASK(_n) GENMASK(2 + ((_n) << 2), ((_n) << 2)) >> +#define GPIO_FLASH_EN(_n) BIT(3 + ((_n) << 2)) >> + >> +#define REG_SIPO_FLASH_MAP(_n) (0x001c + ((_n) << 2)) >> + >> +#define REG_CYCLE_CFG_VALUE(_n) (0x0000 + ((_n) << 2)) >> +#define WAVE_GEN_CYCLE_MASK(_n) GENMASK(7 + ((_n) << 3), ((_n) << 3)) >> + >> +struct airoha_pwm { >> + void __iomem *sgpio_cfg; >> + void __iomem *flash_cfg; >> + void __iomem *cycle_cfg; >> + >> + struct device_node *np; >> + u64 initialized; >> + >> + struct { >> + /* Bitmask of PWM channels using this bucket */ >> + u64 used; >> + u64 period_ns; >> + u64 duty_ns; >> + enum pwm_polarity polarity; >> + } bucket[8]; >> +}; >> + >> +/* >> + * The first 16 GPIO pins, GPIO0-GPIO15, are mapped into 16 PWM channels, 0-15. >> + * The SIPO GPIO pins are 16 pins which are mapped into 17 PWM channels, 16-32. > How can 16 pins be mapped to 17 PWM channels? The text is incorrect. There can be 17 pins in 17 slots. > >> + * However, we've only got 8 concurrent waveform generators and can therefore >> + * only use up to 8 different combinations of duty cycle and period at a time. > That's an information to add to the Limitations paragraph. > >> + */ >> +#define PWM_NUM_GPIO 16 >> +#define PWM_NUM_SIPO 17 >> + >> +/* The PWM hardware supports periods between 4 ms and 1 s */ >> +#define PERIOD_MIN_NS 4000000 >> +#define PERIOD_MAX_NS 1000000000 >> +/* It is represented internally as 1/250 s between 1 and 250 */ >> +#define PERIOD_MIN 1 >> +#define PERIOD_MAX 250 >> +/* Duty cycle is relative with 255 corresponding to 100% */ >> +#define DUTY_FULL 255 >> + >> +static u32 airoha_pwm_rmw(struct airoha_pwm *pc, void __iomem *addr, >> + u32 mask, u32 val) >> +{ >> + val |= (readl(addr) & ~mask); >> + writel(val, addr); >> + >> + return val; >> +} > pc is unused here, please drop it. > >> +#define airoha_pwm_sgpio_rmw(pc, offset, mask, val) \ >> + airoha_pwm_rmw((pc), (pc)->sgpio_cfg + (offset), (mask), (val)) >> +#define airoha_pwm_flash_rmw(pc, offset, mask, val) \ >> + airoha_pwm_rmw((pc), (pc)->flash_cfg + (offset), (mask), (val)) >> +#define airoha_pwm_cycle_rmw(pc, offset, mask, val) \ >> + airoha_pwm_rmw((pc), (pc)->cycle_cfg + (offset), (mask), (val)) >> + >> +#define airoha_pwm_sgpio_set(pc, offset, val) \ >> + airoha_pwm_sgpio_rmw((pc), (offset), 0, (val)) > That does the right thing, but I'd consider > > #define airoha_pwm_sgpio_set(pc, offset, val) \ > airoha_pwm_sgpio_rmw((pc), (offset), (val), (val)) > > to be more understandable (or ~0 instead of the last (val)?) > >> +#define airoha_pwm_sgpio_clear(pc, offset, mask) \ >> + airoha_pwm_sgpio_rmw((pc), (offset), (mask), 0) >> +#define airoha_pwm_flash_set(pc, offset, val) \ >> + airoha_pwm_flash_rmw((pc), (offset), 0, (val)) >> +#define airoha_pwm_flash_clear(pc, offset, mask) \ >> + airoha_pwm_flash_rmw((pc), (offset), (mask), 0) >> + >> +static int airoha_pwm_get_waveform(struct airoha_pwm *pc, u64 duty_ns, >> + u64 period_ns) > Given that "waveform" will gain some specific semantic soon, but this > usage is different, I'd like to see this function renamed. I suggest pwm_generator. Is that acceptable ? > >> +{ >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) { >> + if (!pc->bucket[i].used) >> + continue; >> + >> + if (duty_ns == pc->bucket[i].duty_ns && >> + period_ns == pc->bucket[i].period_ns) >> + return i; >> + >> + /* >> + * Unlike duty cycle zero, which can be handled by >> + * disabling PWM, a generator is needed for full duty >> + * cycle but it can be reused regardless of period >> + */ >> + if (duty_ns == DUTY_FULL && pc->bucket[i].duty_ns == DUTY_FULL) >> + return i; >> + } >> + >> + return -1; >> +} >> + >> +static void airoha_pwm_free_waveform(struct airoha_pwm *pc, unsigned int hwpwm) >> +{ >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) >> + pc->bucket[i].used &= ~BIT_ULL(hwpwm); >> +} >> + >> +static int airoha_pwm_consume_waveform(struct airoha_pwm *pc, >> + u64 duty_ns, u64 period_ns, >> + enum pwm_polarity polarity, >> + unsigned int hwpwm) >> +{ >> + int id = airoha_pwm_get_waveform(pc, duty_ns, period_ns); >> + >> + if (id < 0) { >> + int i; >> + >> + /* find an unused waveform generator */ >> + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) { >> + if (!(pc->bucket[i].used & ~BIT_ULL(hwpwm))) { >> + id = i; >> + break; >> + } >> + } >> + } >> + >> + if (id >= 0) { >> + airoha_pwm_free_waveform(pc, hwpwm); >> + pc->bucket[id].used |= BIT_ULL(hwpwm); >> + pc->bucket[id].period_ns = period_ns; >> + pc->bucket[id].duty_ns = duty_ns; >> + pc->bucket[id].polarity = polarity; >> + } > One downside of the (nearly) maximal flexibility implemented here is > that if you have 9 (or more) requested pwm devices configuration is > quite limited. So it might happen that a consumer changes the > configuration for pwm#2 from pwm_state A to pwm_state B but cannot > change back to A later. Correct. > > If you limit the number of requested pwm devices to 8, the code gets a > tad simpler (because you can map a fixed bucket to each pwm device and > don't need to search during .apply()) and each consumer has maximal > freedom to configure its device. The main use for this solution is for led-dimming which uses the same timing among groups of leds. Most (of our) products have more then 8 leds in total, with a limit of only 8 pwm devices it would not be possible to use the mainline driver with our hardware. I suggest that the current logic is kept but properly documented in the limitations section. > >> + >> + return id; >> +} >> + >> +static int airoha_pwm_sipo_init(struct airoha_pwm *pc) >> +{ >> + u32 clk_divr_val = 3, sipo_clock_delay = 1; >> + u32 val, sipo_clock_divisor = 32; >> + >> + if (!(pc->initialized >> PWM_NUM_GPIO)) >> + return 0; >> + >> + /* Select the right shift register chip */ >> + if (of_property_read_bool(pc->np, "hc74595")) >> + airoha_pwm_sgpio_set(pc, REG_SIPO_FLASH_MODE_CFG, >> + SERIAL_GPIO_MODE); >> + else >> + airoha_pwm_sgpio_clear(pc, REG_SIPO_FLASH_MODE_CFG, >> + SERIAL_GPIO_MODE); >> + >> + if (!of_property_read_u32(pc->np, "sipo-clock-divisor", >> + &sipo_clock_divisor)) { >> + switch (sipo_clock_divisor) { >> + case 4: >> + clk_divr_val = 0; >> + break; >> + case 8: >> + clk_divr_val = 1; >> + break; >> + case 16: >> + clk_divr_val = 2; >> + break; >> + case 32: >> + clk_divr_val = 3; >> + break; >> + default: >> + return -EINVAL; >> + } >> + } >> + /* Configure shift register timings */ >> + writel(clk_divr_val, pc->sgpio_cfg + REG_SGPIO_CLK_DIVR); >> + >> + of_property_read_u32(pc->np, "sipo-clock-delay", &sipo_clock_delay); >> + if (sipo_clock_delay < 1 || sipo_clock_delay > sipo_clock_divisor / 2) >> + return -EINVAL; >> + >> + /* >> + * The actual delay is sclkdly + 1 so subtract 1 from >> + * sipo-clock-delay to calculate the register value >> + */ >> + sipo_clock_delay--; >> + writel(sipo_clock_delay, pc->sgpio_cfg + REG_SGPIO_CLK_DLY); >> + >> + /* >> + * It it necessary to after muxing explicitly shift out all >> + * zeroes to initialize the shift register before enabling PWM >> + * mode because in PWM mode SIPO will not start shifting until >> + * it needs to output a non-zero value (bit 31 of led_data >> + * indicates shifting in progress and it must return to zero >> + * before led_data can be written or PWM mode can be set) >> + */ >> + if (readl_poll_timeout(pc->sgpio_cfg + REG_SGPIO_LED_DATE, val, >> + !(val & SGPIO_LED_SHIFT_FLAG), 10, >> + 200 * USEC_PER_MSEC)) >> + return -ETIMEDOUT; >> + >> + airoha_pwm_sgpio_clear(pc, REG_SGPIO_LED_DATE, SGPIO_LED_DATA); >> + if (readl_poll_timeout(pc->sgpio_cfg + REG_SGPIO_LED_DATE, val, >> + !(val & SGPIO_LED_SHIFT_FLAG), 10, >> + 200 * USEC_PER_MSEC)) >> + return -ETIMEDOUT; >> + >> + /* Set SIPO in PWM mode */ >> + airoha_pwm_sgpio_set(pc, REG_SIPO_FLASH_MODE_CFG, >> + SERIAL_GPIO_FLASH_MODE); >> + >> + return 0; >> +} >> + >> +static void airoha_pwm_config_waveform(struct airoha_pwm *pc, int index, >> + u64 duty_ns, u64 period_ns, >> + enum pwm_polarity polarity) >> +{ >> + u32 period, duty, mask, val; >> + >> + duty = clamp_val(div64_u64(DUTY_FULL * duty_ns, period_ns), 0, >> + DUTY_FULL); > DUTY_FULL * duty_ns might overflow. Also the calculation is wrong. > For example if the following is requested: > > .period = 23999999, > .duty_cycle = 8000000, > > you're supposed to configure period = 16000000 ns and duty_cycle = > 8000000 ns. > > (I.e.: Pick the biggest possible period not bigger than the requested > period. For that pick the biggest possible duty_cycle not bigger than > the requested duty_cycle.) > > If you implement .get_state() in a way to return the actually configured > state, enabling PWM_DEBUG and intensive testing helps to get this right. > >> + if (polarity == PWM_POLARITY_INVERSED) >> + duty = DUTY_FULL - duty; > This alone doesn't switch the polarity of the signal. Please only claim > to be able to implement the settings that the hardware actually can do. I am not sure I agree, but I will investigate this further. MvH Benjamin Larsson
Hello Benjamin, On Tue, Sep 03, 2024 at 01:58:30PM +0200, Benjamin Larsson wrote: > On 2024-09-03 12:46, Uwe Kleine-König wrote: > > Would you please add a "Limitations" paragraph here covering the > > following questions: > > > > - How does the hardware behave on changes of configuration (does it > > complete the currently running period? Are there any glitches?) > > - How does the hardware behave on disabling? > > > > Please stick to the format used in several other drivers such that > > > > sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c > > > > emits the informations. > > The answer to your questions are currently unknown. Is this information > needed for a merge of the driver ? It would be very welcome and typically isn't that hard to work out if you have an LED connected to the output or a similar means to observe the output. An oscilloscope makes it still easier. For example to check if the current period is completed configure the PWM with period = 1s and duty_cycle = 0 disabling the LED. (I leave it as an exercise for the reader what to do if duty_cycle = 0 enables the LED :-) Then do: pwm_apply_might_sleep(mypwm, &(struct pwm_state){ .period = NSEC_PER_SEC, .duty_cycle = NSEC_PER_SEC, .enabled = true, }); pwm_apply_might_sleep(mypwm, &(struct pwm_state){ .period = NSEC_PER_SEC, .duty_cycle = 0, .enabled = true, }); Iff that enables the LED for a second, the period is completed. The question about glitches is a bit harder to answer, but with a tool like memtool should be possible to answer. Alternatively add delays and printk output to .apply() in the critical places. > > > +#define airoha_pwm_sgpio_clear(pc, offset, mask) \ > > > + airoha_pwm_sgpio_rmw((pc), (offset), (mask), 0) > > > +#define airoha_pwm_flash_set(pc, offset, val) \ > > > + airoha_pwm_flash_rmw((pc), (offset), 0, (val)) > > > +#define airoha_pwm_flash_clear(pc, offset, mask) \ > > > + airoha_pwm_flash_rmw((pc), (offset), (mask), 0) > > > + > > > +static int airoha_pwm_get_waveform(struct airoha_pwm *pc, u64 duty_ns, > > > + u64 period_ns) > > Given that "waveform" will gain some specific semantic soon, but this > > usage is different, I'd like to see this function renamed. > > I suggest pwm_generator. Is that acceptable ? This function determines the register values to write for a given duty_ns + period_ns combination, right? airoha_pwm_calc_bucket_config()? > > If you limit the number of requested pwm devices to 8, the code gets a > > tad simpler (because you can map a fixed bucket to each pwm device and > > don't need to search during .apply()) and each consumer has maximal > > freedom to configure its device. > > The main use for this solution is for led-dimming which uses the same timing > among groups of leds. Most (of our) products have more then 8 leds in total, > with a limit of only 8 pwm devices it would not be possible to use the > mainline driver with our hardware. I suggest that the current logic is kept > but properly documented in the limitations section. Fine for me. Best regards Uwe
Hi. On 03/09/2024 17:47, Uwe Kleine-König wrote: > Hello Benjamin, > > On Tue, Sep 03, 2024 at 01:58:30PM +0200, Benjamin Larsson wrote: >> On 2024-09-03 12:46, Uwe Kleine-König wrote: >>> Would you please add a "Limitations" paragraph here covering the >>> following questions: >>> >>> - How does the hardware behave on changes of configuration (does it >>> complete the currently running period? Are there any glitches?) >>> - How does the hardware behave on disabling? >>> >>> Please stick to the format used in several other drivers such that >>> >>> sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c >>> >>> emits the informations. >> The answer to your questions are currently unknown. Is this information >> needed for a merge of the driver ? > It would be very welcome and typically isn't that hard to work out if > you have an LED connected to the output or a similar means to observe > the output. An oscilloscope makes it still easier. > > For example to check if the current period is completed configure the > PWM with period = 1s and duty_cycle = 0 disabling the LED. (I leave it > as an exercise for the reader what to do if duty_cycle = 0 enables the > LED :-) Then do: > > pwm_apply_might_sleep(mypwm, &(struct pwm_state){ > .period = NSEC_PER_SEC, > .duty_cycle = NSEC_PER_SEC, > .enabled = true, > }); > pwm_apply_might_sleep(mypwm, &(struct pwm_state){ > .period = NSEC_PER_SEC, > .duty_cycle = 0, > .enabled = true, > }); > > Iff that enables the LED for a second, the period is completed. The > question about glitches is a bit harder to answer, but with a tool like > memtool should be possible to answer. Alternatively add delays and > printk output to .apply() in the critical places. > > I connected a logic analyzer to a pin and configured the pwm for it. I then configured the pwm with these parameters (setup for 2Hz). echo 1000000000 > /sys/class/pwm/pwmchip0/pwm12/period echo 0 > /sys/class/pwm/pwmchip0/pwm12/duty_cycle If I then ran the following (in a script) no pulse was detected: echo 500000000 > /sys/class/pwm/pwmchip0/pwm12/duty_cycle echo 0 > /sys/class/pwm/pwmchip0/pwm12/duty_cycle If I added a sleep 1 in between I always got 1 500ms pulse. I then did the same but with direct register access with the same result. Setting the duty cycle to 0 disables the pwm function on the pin, it seems to take a while before it properly activates but before it disables it the cycle completes. I also tested with enabling the pwn signal and then setting a 0 duty cycle. The last observed pulse was always 500ms long. I am not sure what of your questions this answers and is there some other tests I should perform ? For the record while toggling the registers I noticed that it was actually possible to generate 1 second long pulses. The documentation is not clear on this part. MvH Benjamin Larsson
Hello, On Thu, Sep 05, 2024 at 01:09:48AM +0200, Benjamin Larsson wrote: > On 03/09/2024 17:47, Uwe Kleine-König wrote: > > Hello Benjamin, > > > > On Tue, Sep 03, 2024 at 01:58:30PM +0200, Benjamin Larsson wrote: > > > On 2024-09-03 12:46, Uwe Kleine-König wrote: > > > > Would you please add a "Limitations" paragraph here covering the > > > > following questions: > > > > > > > > - How does the hardware behave on changes of configuration (does it > > > > complete the currently running period? Are there any glitches?) > > > > - How does the hardware behave on disabling? > > > > > > > > Please stick to the format used in several other drivers such that > > > > > > > > sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c > > > > > > > > emits the informations. > > > The answer to your questions are currently unknown. Is this information > > > needed for a merge of the driver ? > > It would be very welcome and typically isn't that hard to work out if > > you have an LED connected to the output or a similar means to observe > > the output. An oscilloscope makes it still easier. > > > > For example to check if the current period is completed configure the > > PWM with period = 1s and duty_cycle = 0 disabling the LED. (I leave it > > as an exercise for the reader what to do if duty_cycle = 0 enables the > > LED :-) Then do: > > > > pwm_apply_might_sleep(mypwm, &(struct pwm_state){ > > .period = NSEC_PER_SEC, > > .duty_cycle = NSEC_PER_SEC, > > .enabled = true, > > }); > > pwm_apply_might_sleep(mypwm, &(struct pwm_state){ > > .period = NSEC_PER_SEC, > > .duty_cycle = 0, > > .enabled = true, > > }); > > > > Iff that enables the LED for a second, the period is completed. The > > question about glitches is a bit harder to answer, but with a tool like > > memtool should be possible to answer. Alternatively add delays and > > printk output to .apply() in the critical places. > > > > > > I connected a logic analyzer to a pin and configured the pwm for it. > > I then configured the pwm with these parameters (setup for 2Hz). > > echo 1000000000 > /sys/class/pwm/pwmchip0/pwm12/period > echo 0 > /sys/class/pwm/pwmchip0/pwm12/duty_cycle > > If I then ran the following (in a script) no pulse was detected: > > echo 500000000 > /sys/class/pwm/pwmchip0/pwm12/duty_cycle > echo 0 > /sys/class/pwm/pwmchip0/pwm12/duty_cycle > > If I added a sleep 1 in between I always got 1 500ms pulse. > > I then did the same but with direct register access with the same result. > Setting the duty cycle to 0 disables the pwm function on the pin, it seems > to take a while before it properly activates but before it disables it the > cycle completes. > > > I also tested with enabling the pwn signal and then setting a 0 duty cycle. > The last observed pulse was always 500ms long. > > > I am not sure what of your questions this answers and is there some other > tests I should perform ? IIUC that means to add: On configuration the currently running period is completed. to the Limitations paragraph. > For the record while toggling the registers I noticed that it was actually > possible to generate 1 second long pulses. The documentation is not clear on > this part. 1 second long pulses with a period size of 1 second, so a constant high signal? Another thing that would be interesting is, if it can happen that you get a mixed signal. That is, if you update from .period = A .duty_cycle = B to .period = C .duty_cycle = D that you get one period with length C and duty_cycle B when the period completes after configuring period but before duty_cycle. Best regards UWe
On 2024-09-05 11:30, Uwe Kleine-König wrote: > 1 second long pulses with a period size of 1 second, so a constant high > signal? Hi, I think I was unclear. The SoC documentation is not that detailed. But I think I understand how it works now. One register contains the minimum duration (d_min). And then there is one 8 bit register for the signal low period (lp) and then another 8bit register for the high period (hp). Per my understanding a change of polarity is then just a swap of lp and hp. The period is d_min * (lp + hp) and duty_cycle (on time) is then d_min*hp (per my understanding of the linux api). This means that there can be different settings that result in the same pwm signal (if you double d_min and halving lp and hp the signal should be the same). This means that when requesting a period and duty cycle you need to search through the configuration space to find the optimal value. > > Another thing that would be interesting is, if it can happen that you > get a mixed signal. That is, if you update from > > .period = A > .duty_cycle = B > > to > > .period = C > .duty_cycle = D > > that you get one period with length C and duty_cycle B when the period > completes after configuring period but before duty_cycle. > > Best regards > UWe I will perform this test also. MvH Benjamin Larsson
Hello Benjamin, On Thu, Sep 05, 2024 at 02:18:41PM +0200, Benjamin Larsson wrote: > On 2024-09-05 11:30, Uwe Kleine-König wrote: > > 1 second long pulses with a period size of 1 second, so a constant high > > signal? > > Hi, I think I was unclear. The SoC documentation is not that detailed. But I > think I understand how it works now. > > One register contains the minimum duration (d_min). And then there is one 8 > bit register for the signal low period (lp) and then another 8bit register > for the high period (hp). Per my understanding a change of polarity is then > just a swap of lp and hp. that doesn't sound right. A "normal" waveform with period = 10 ns and duty_cycle = 2 ns looks as follows: _ _ _ / \_______/ \_______/ \_______/ ^ ^ ^ ^ assuming a monospace font that's 1 char per ns, the ^ marking the period start. Ignoring scaling, your hardware needs to have hp = 2 and lp = 8. If you switch that (assuming you mean switching in the same way as I do) to hp = 8 and lp = 2, you get: _______ _______ _______ / \_/ \_/ \_/ ^ ^ ^ ^ which is still a "normal" polarity output as a period starts with the active part. I admit that's a bit artificial, because the waveform for period = 10 ns duty_cycle = 2 ns polarity = inversed looks as follows: _______ _______ _______ \_/ \_/ \_/ \_/ ^ ^ ^ ^ which isn't any different from the 2nd waveform above if you ignore the period start markers (which are not observable apart from the moments where you reconfigure the output). However it matters if you have a chip with >1 output that are not independent. > The period is d_min * (lp + hp) and duty_cycle (on time) is then d_min*hp > (per my understanding of the linux api). This means that there can be > different settings that result in the same pwm signal (if you double d_min > and halving lp and hp the signal should be the same). Sounds correct. > This means that when requesting a period and duty cycle you need to search > through the configuration space to find the optimal value. Or restrict yourself consistently to something simpler than a exhaustive search through the complete configuration space. > MvH (BTW, I had to research the meaning of MvH. In case someone else doesn't know it: It's the usual abbreviation for "Med vänliga hälsningar" in Sweden or "Med vennlig hilsen" in Norway; both meaning "With friendly greetings".) Best regards Uwe
Hi. On 05/09/2024 17:39, Uwe Kleine-König wrote: > Hello Benjamin, > > On Thu, Sep 05, 2024 at 02:18:41PM +0200, Benjamin Larsson wrote: >> On 2024-09-05 11:30, Uwe Kleine-König wrote: >>> 1 second long pulses with a period size of 1 second, so a constant high >>> signal? >> Hi, I think I was unclear. The SoC documentation is not that detailed. But I >> think I understand how it works now. >> >> One register contains the minimum duration (d_min). And then there is one 8 >> bit register for the signal low period (lp) and then another 8bit register >> for the high period (hp). Per my understanding a change of polarity is then >> just a swap of lp and hp. > that doesn't sound right. > > A "normal" waveform with period = 10 ns and duty_cycle = 2 ns looks as > follows: > > _ _ _ > / \_______/ \_______/ \_______/ > ^ ^ ^ ^ > > assuming a monospace font that's 1 char per ns, the ^ marking the period > start. > > Ignoring scaling, your hardware needs to have hp = 2 and lp = 8. If you > switch that (assuming you mean switching in the same way as I do) to hp > = 8 and lp = 2, you get: > > _______ _______ _______ > / \_/ \_/ \_/ > ^ ^ ^ ^ > > which is still a "normal" polarity output as a period starts with the > active part. > > I admit that's a bit artificial, because the waveform for > > period = 10 ns > duty_cycle = 2 ns > polarity = inversed > > looks as follows: > > _______ _______ _______ > \_/ \_/ \_/ \_/ > ^ ^ ^ ^ > > which isn't any different from the 2nd waveform above if you ignore the > period start markers (which are not observable apart from the moments > where you reconfigure the output). > > However it matters if you have a chip with >1 output that are not > independent. Ok that was a clear explanation, anyway the pwm hardware is then not capable of a polarity change. It is possible to change the polarity via other means but there is no way for the pwm block (and driver) to handle this. >> This means that when requesting a period and duty cycle you need to search >> through the configuration space to find the optimal value. > Or restrict yourself consistently to something simpler than a exhaustive > search through the complete configuration space. Is there a recommendation on what is more important? Period duration or duty cycle percentage? >> MvH > (BTW, I had to research the meaning of MvH. In case someone else doesn't > know it: It's the usual abbreviation for "Med vänliga hälsningar" in > Sweden or "Med vennlig hilsen" in Norway; both meaning "With friendly > greetings".) > > Best regards > Uwe MvH Benjamin Larsson
Hello Benjamin, On Thu, Sep 05, 2024 at 08:35:17PM +0200, Benjamin Larsson wrote: > On 05/09/2024 17:39, Uwe Kleine-König wrote: > > On Thu, Sep 05, 2024 at 02:18:41PM +0200, Benjamin Larsson wrote: > > > On 2024-09-05 11:30, Uwe Kleine-König wrote: > > > > 1 second long pulses with a period size of 1 second, so a constant high > > > > signal? > > > Hi, I think I was unclear. The SoC documentation is not that detailed. But I > > > think I understand how it works now. > > > > > > One register contains the minimum duration (d_min). And then there is one 8 > > > bit register for the signal low period (lp) and then another 8bit register > > > for the high period (hp). Per my understanding a change of polarity is then > > > just a swap of lp and hp. > > that doesn't sound right. > > > > A "normal" waveform with period = 10 ns and duty_cycle = 2 ns looks as > > follows: > > > > _ _ _ > > / \_______/ \_______/ \_______/ > > ^ ^ ^ ^ > > > > assuming a monospace font that's 1 char per ns, the ^ marking the period > > start. > > > > Ignoring scaling, your hardware needs to have hp = 2 and lp = 8. If you > > switch that (assuming you mean switching in the same way as I do) to hp > > = 8 and lp = 2, you get: > > > > _______ _______ _______ > > / \_/ \_/ \_/ > > ^ ^ ^ ^ > > > > which is still a "normal" polarity output as a period starts with the > > active part. > > > > I admit that's a bit artificial, because the waveform for > > > > period = 10 ns > > duty_cycle = 2 ns > > polarity = inversed > > > > looks as follows: > > > > _______ _______ _______ > > \_/ \_/ \_/ \_/ > > ^ ^ ^ ^ > > > > which isn't any different from the 2nd waveform above if you ignore the > > period start markers (which are not observable apart from the moments > > where you reconfigure the output). > > > > However it matters if you have a chip with >1 output that are not > > independent. > > > Ok that was a clear explanation, \o/ > anyway the pwm hardware is then not capable > of a polarity change. It is possible to change the polarity via other means > but there is no way for the pwm block (and driver) to handle this. That's ok. Just do something like if (state->polarity != PWM_POLARITY_NORMAL) return -EINVAL; It's quite usual that drivers only support a single polarity. > > > This means that when requesting a period and duty cycle you need to search > > > through the configuration space to find the optimal value. > > Or restrict yourself consistently to something simpler than a exhaustive > > search through the complete configuration space. > > Is there a recommendation on what is more important? Period duration or duty > cycle percentage? That really depends on your usage domain. Just pick an algorithm that is sound, ideally easy to review and serves your purpose. If you pick something that is too simple for the next consumer, we can add the needed complexity still later. So in my book even something like restricting the period to a single fixed value and just modify the duty cycle is fine. In that case add a comment that there is room for improvement and I'm happy. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 3e53838990f5..0a78bda0707d 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -47,6 +47,16 @@ config PWM_AB8500 To compile this driver as a module, choose M here: the module will be called pwm-ab8500. +config PWM_AIROHA + tristate "Airoha PWM support" + depends on ARCH_AIROHA || COMPILE_TEST + depends on OF + help + Generic PWM framework driver for Airoha SoC. + + To compile this driver as a module, choose M here: the module + will be called pwm-airoha. + config PWM_APPLE tristate "Apple SoC PWM support" depends on ARCH_APPLE || COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 0be4f3e6dd43..7ee61822d88d 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_PWM) += core.o obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o +obj-$(CONFIG_PWM_AIROHA) += pwm-airoha.o obj-$(CONFIG_PWM_APPLE) += pwm-apple.o obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o diff --git a/drivers/pwm/pwm-airoha.c b/drivers/pwm/pwm-airoha.c new file mode 100644 index 000000000000..54dc12d20da4 --- /dev/null +++ b/drivers/pwm/pwm-airoha.c @@ -0,0 +1,435 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2022 Markus Gothe <markus.gothe@genexis.eu> + */ + +#include <linux/bitfield.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/iopoll.h> +#include <linux/mfd/airoha-en7581-mfd.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/gpio.h> +#include <linux/bitops.h> +#include <asm/div64.h> + +#define REG_SGPIO_CFG 0x0024 +#define REG_FLASH_CFG 0x0038 +#define REG_CYCLE_CFG 0x0098 + +#define REG_SGPIO_LED_DATE 0x0000 +#define SGPIO_LED_SHIFT_FLAG BIT(31) +#define SGPIO_LED_DATA GENMASK(16, 0) + +#define REG_SGPIO_CLK_DIVR 0x0004 +#define REG_SGPIO_CLK_DLY 0x0008 + +#define REG_SIPO_FLASH_MODE_CFG 0x000c +#define SERIAL_GPIO_FLASH_MODE BIT(1) +#define SERIAL_GPIO_MODE BIT(0) + +#define REG_GPIO_FLASH_PRD_SET(_n) (0x0004 + ((_n) << 2)) +#define GPIO_FLASH_PRD_MASK(_n) GENMASK(15 + ((_n) << 4), ((_n) << 4)) + +#define REG_GPIO_FLASH_MAP(_n) (0x0014 + ((_n) << 2)) +#define GPIO_FLASH_SETID_MASK(_n) GENMASK(2 + ((_n) << 2), ((_n) << 2)) +#define GPIO_FLASH_EN(_n) BIT(3 + ((_n) << 2)) + +#define REG_SIPO_FLASH_MAP(_n) (0x001c + ((_n) << 2)) + +#define REG_CYCLE_CFG_VALUE(_n) (0x0000 + ((_n) << 2)) +#define WAVE_GEN_CYCLE_MASK(_n) GENMASK(7 + ((_n) << 3), ((_n) << 3)) + +struct airoha_pwm { + void __iomem *sgpio_cfg; + void __iomem *flash_cfg; + void __iomem *cycle_cfg; + + struct device_node *np; + u64 initialized; + + struct { + /* Bitmask of PWM channels using this bucket */ + u64 used; + u64 period_ns; + u64 duty_ns; + enum pwm_polarity polarity; + } bucket[8]; +}; + +/* + * The first 16 GPIO pins, GPIO0-GPIO15, are mapped into 16 PWM channels, 0-15. + * The SIPO GPIO pins are 16 pins which are mapped into 17 PWM channels, 16-32. + * However, we've only got 8 concurrent waveform generators and can therefore + * only use up to 8 different combinations of duty cycle and period at a time. + */ +#define PWM_NUM_GPIO 16 +#define PWM_NUM_SIPO 17 + +/* The PWM hardware supports periods between 4 ms and 1 s */ +#define PERIOD_MIN_NS 4000000 +#define PERIOD_MAX_NS 1000000000 +/* It is represented internally as 1/250 s between 1 and 250 */ +#define PERIOD_MIN 1 +#define PERIOD_MAX 250 +/* Duty cycle is relative with 255 corresponding to 100% */ +#define DUTY_FULL 255 + +static u32 airoha_pwm_rmw(struct airoha_pwm *pc, void __iomem *addr, + u32 mask, u32 val) +{ + val |= (readl(addr) & ~mask); + writel(val, addr); + + return val; +} + +#define airoha_pwm_sgpio_rmw(pc, offset, mask, val) \ + airoha_pwm_rmw((pc), (pc)->sgpio_cfg + (offset), (mask), (val)) +#define airoha_pwm_flash_rmw(pc, offset, mask, val) \ + airoha_pwm_rmw((pc), (pc)->flash_cfg + (offset), (mask), (val)) +#define airoha_pwm_cycle_rmw(pc, offset, mask, val) \ + airoha_pwm_rmw((pc), (pc)->cycle_cfg + (offset), (mask), (val)) + +#define airoha_pwm_sgpio_set(pc, offset, val) \ + airoha_pwm_sgpio_rmw((pc), (offset), 0, (val)) +#define airoha_pwm_sgpio_clear(pc, offset, mask) \ + airoha_pwm_sgpio_rmw((pc), (offset), (mask), 0) +#define airoha_pwm_flash_set(pc, offset, val) \ + airoha_pwm_flash_rmw((pc), (offset), 0, (val)) +#define airoha_pwm_flash_clear(pc, offset, mask) \ + airoha_pwm_flash_rmw((pc), (offset), (mask), 0) + +static int airoha_pwm_get_waveform(struct airoha_pwm *pc, u64 duty_ns, + u64 period_ns) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) { + if (!pc->bucket[i].used) + continue; + + if (duty_ns == pc->bucket[i].duty_ns && + period_ns == pc->bucket[i].period_ns) + return i; + + /* + * Unlike duty cycle zero, which can be handled by + * disabling PWM, a generator is needed for full duty + * cycle but it can be reused regardless of period + */ + if (duty_ns == DUTY_FULL && pc->bucket[i].duty_ns == DUTY_FULL) + return i; + } + + return -1; +} + +static void airoha_pwm_free_waveform(struct airoha_pwm *pc, unsigned int hwpwm) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) + pc->bucket[i].used &= ~BIT_ULL(hwpwm); +} + +static int airoha_pwm_consume_waveform(struct airoha_pwm *pc, + u64 duty_ns, u64 period_ns, + enum pwm_polarity polarity, + unsigned int hwpwm) +{ + int id = airoha_pwm_get_waveform(pc, duty_ns, period_ns); + + if (id < 0) { + int i; + + /* find an unused waveform generator */ + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) { + if (!(pc->bucket[i].used & ~BIT_ULL(hwpwm))) { + id = i; + break; + } + } + } + + if (id >= 0) { + airoha_pwm_free_waveform(pc, hwpwm); + pc->bucket[id].used |= BIT_ULL(hwpwm); + pc->bucket[id].period_ns = period_ns; + pc->bucket[id].duty_ns = duty_ns; + pc->bucket[id].polarity = polarity; + } + + return id; +} + +static int airoha_pwm_sipo_init(struct airoha_pwm *pc) +{ + u32 clk_divr_val = 3, sipo_clock_delay = 1; + u32 val, sipo_clock_divisor = 32; + + if (!(pc->initialized >> PWM_NUM_GPIO)) + return 0; + + /* Select the right shift register chip */ + if (of_property_read_bool(pc->np, "hc74595")) + airoha_pwm_sgpio_set(pc, REG_SIPO_FLASH_MODE_CFG, + SERIAL_GPIO_MODE); + else + airoha_pwm_sgpio_clear(pc, REG_SIPO_FLASH_MODE_CFG, + SERIAL_GPIO_MODE); + + if (!of_property_read_u32(pc->np, "sipo-clock-divisor", + &sipo_clock_divisor)) { + switch (sipo_clock_divisor) { + case 4: + clk_divr_val = 0; + break; + case 8: + clk_divr_val = 1; + break; + case 16: + clk_divr_val = 2; + break; + case 32: + clk_divr_val = 3; + break; + default: + return -EINVAL; + } + } + /* Configure shift register timings */ + writel(clk_divr_val, pc->sgpio_cfg + REG_SGPIO_CLK_DIVR); + + of_property_read_u32(pc->np, "sipo-clock-delay", &sipo_clock_delay); + if (sipo_clock_delay < 1 || sipo_clock_delay > sipo_clock_divisor / 2) + return -EINVAL; + + /* + * The actual delay is sclkdly + 1 so subtract 1 from + * sipo-clock-delay to calculate the register value + */ + sipo_clock_delay--; + writel(sipo_clock_delay, pc->sgpio_cfg + REG_SGPIO_CLK_DLY); + + /* + * It it necessary to after muxing explicitly shift out all + * zeroes to initialize the shift register before enabling PWM + * mode because in PWM mode SIPO will not start shifting until + * it needs to output a non-zero value (bit 31 of led_data + * indicates shifting in progress and it must return to zero + * before led_data can be written or PWM mode can be set) + */ + if (readl_poll_timeout(pc->sgpio_cfg + REG_SGPIO_LED_DATE, val, + !(val & SGPIO_LED_SHIFT_FLAG), 10, + 200 * USEC_PER_MSEC)) + return -ETIMEDOUT; + + airoha_pwm_sgpio_clear(pc, REG_SGPIO_LED_DATE, SGPIO_LED_DATA); + if (readl_poll_timeout(pc->sgpio_cfg + REG_SGPIO_LED_DATE, val, + !(val & SGPIO_LED_SHIFT_FLAG), 10, + 200 * USEC_PER_MSEC)) + return -ETIMEDOUT; + + /* Set SIPO in PWM mode */ + airoha_pwm_sgpio_set(pc, REG_SIPO_FLASH_MODE_CFG, + SERIAL_GPIO_FLASH_MODE); + + return 0; +} + +static void airoha_pwm_config_waveform(struct airoha_pwm *pc, int index, + u64 duty_ns, u64 period_ns, + enum pwm_polarity polarity) +{ + u32 period, duty, mask, val; + + duty = clamp_val(div64_u64(DUTY_FULL * duty_ns, period_ns), 0, + DUTY_FULL); + if (polarity == PWM_POLARITY_INVERSED) + duty = DUTY_FULL - duty; + + period = clamp_val(div64_u64(25 * period_ns, 100000000), PERIOD_MIN, + PERIOD_MAX); + + /* Configure frequency divisor */ + mask = WAVE_GEN_CYCLE_MASK(index % 4); + val = (period << __ffs(mask)) & mask; + airoha_pwm_cycle_rmw(pc, REG_CYCLE_CFG_VALUE(index / 4), mask, val); + + /* Configure duty cycle */ + duty = ((DUTY_FULL - duty) << 8) | duty; + mask = GPIO_FLASH_PRD_MASK(index % 2); + val = (duty << __ffs(mask)) & mask; + airoha_pwm_flash_rmw(pc, REG_GPIO_FLASH_PRD_SET(index / 2), mask, val); +} + +static void airoha_pwm_config_flash_map(struct airoha_pwm *pc, + unsigned int hwpwm, int index) +{ + u32 addr, mask, val; + + if (hwpwm < PWM_NUM_GPIO) { + addr = REG_GPIO_FLASH_MAP(hwpwm / 8); + } else { + addr = REG_SIPO_FLASH_MAP(hwpwm / 8); + hwpwm -= PWM_NUM_GPIO; + } + + if (index < 0) { + /* + * Change of waveform takes effect immediately but + * disabling has some delay so to prevent glitching + * only the enable bit is touched when disabling + */ + airoha_pwm_flash_clear(pc, addr, GPIO_FLASH_EN(hwpwm % 8)); + return; + } + + mask = GPIO_FLASH_SETID_MASK(hwpwm % 8); + val = ((index & 7) << __ffs(mask)) & mask; + airoha_pwm_flash_rmw(pc, addr, mask, val); + airoha_pwm_flash_set(pc, addr, GPIO_FLASH_EN(hwpwm % 8)); +} + +static int airoha_pwm_config(struct airoha_pwm *pc, struct pwm_device *pwm, + u64 duty_ns, u64 period_ns, + enum pwm_polarity polarity) +{ + int index = -1; + + index = airoha_pwm_consume_waveform(pc, duty_ns, period_ns, polarity, + pwm->hwpwm); + if (index < 0) + return -EBUSY; + + if (!(pc->initialized & BIT_ULL(pwm->hwpwm)) && + pwm->hwpwm >= PWM_NUM_GPIO) + airoha_pwm_sipo_init(pc); + + if (index >= 0) { + airoha_pwm_config_waveform(pc, index, duty_ns, period_ns, + polarity); + airoha_pwm_config_flash_map(pc, pwm->hwpwm, index); + } else { + airoha_pwm_config_flash_map(pc, pwm->hwpwm, index); + airoha_pwm_free_waveform(pc, pwm->hwpwm); + } + + pc->initialized |= BIT_ULL(pwm->hwpwm); + + return 0; +} + +static void airoha_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct airoha_pwm *pc = pwmchip_get_drvdata(chip); + + /* Disable PWM and release the waveform */ + airoha_pwm_config_flash_map(pc, pwm->hwpwm, -1); + airoha_pwm_free_waveform(pc, pwm->hwpwm); + + pc->initialized &= ~BIT_ULL(pwm->hwpwm); + if (!(pc->initialized >> PWM_NUM_GPIO)) + airoha_pwm_sgpio_clear(pc, REG_SIPO_FLASH_MODE_CFG, + SERIAL_GPIO_FLASH_MODE); + + /* + * Clear the state to force re-initialization the next time + * this PWM channel is used since we cannot retain state in + * hardware due to limited number of waveform generators + */ + memset(&pwm->state, 0, sizeof(pwm->state)); +} + +static int airoha_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct airoha_pwm *pc = pwmchip_get_drvdata(chip); + u64 duty = state->enabled ? state->duty_cycle : 0; + + if (!state->enabled) { + airoha_pwm_free(chip, pwm); + return 0; + } + + if (state->period < PERIOD_MIN_NS || state->period > PERIOD_MAX_NS) + return -EINVAL; + + return airoha_pwm_config(pc, pwm, duty, state->period, + state->polarity); +} + +static int airoha_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct airoha_pwm *pc = pwmchip_get_drvdata(chip); + int i; + + /* find hwpwm in waveform generator bucket */ + for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) { + if (pc->bucket[i].used & BIT_ULL(pwm->hwpwm)) { + state->enabled = pc->initialized & BIT_ULL(pwm->hwpwm); + state->polarity = pc->bucket[i].polarity; + state->period = pc->bucket[i].period_ns; + state->duty_cycle = pc->bucket[i].duty_ns; + break; + } + } + + if (i == ARRAY_SIZE(pc->bucket)) + state->enabled = false; + + return 0; +} + +static const struct pwm_ops airoha_pwm_ops = { + .get_state = airoha_pwm_get_state, + .apply = airoha_pwm_apply, + .free = airoha_pwm_free, +}; + +static int airoha_pwm_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct airoha_mfd *mfd; + struct airoha_pwm *pc; + struct pwm_chip *chip; + + /* Assign parent MFD of_node to dev */ + dev->of_node = of_node_get(dev->parent->of_node); + mfd = dev_get_drvdata(dev->parent); + + chip = devm_pwmchip_alloc(dev, PWM_NUM_GPIO + PWM_NUM_SIPO, + sizeof(*pc)); + if (IS_ERR(chip)) + return PTR_ERR(chip); + + pc = pwmchip_get_drvdata(chip); + pc->np = dev->of_node; + pc->sgpio_cfg = mfd->base + REG_SGPIO_CFG; + pc->flash_cfg = mfd->base + REG_FLASH_CFG; + pc->cycle_cfg = mfd->base + REG_CYCLE_CFG; + + chip->ops = &airoha_pwm_ops; + chip->of_xlate = of_pwm_xlate_with_flags; + + return devm_pwmchip_add(&pdev->dev, chip); +} + +static struct platform_driver airoha_pwm_driver = { + .driver = { + .name = "airoha-pwm", + }, + .probe = airoha_pwm_probe, +}; +module_platform_driver(airoha_pwm_driver); + +MODULE_AUTHOR("Lorenzo Bianconi <lorenzo@kernel.org>"); +MODULE_AUTHOR("Markus Gothe <markus.gothe@genexis.eu>"); +MODULE_AUTHOR("Benjamin Larsson <benjamin.larsson@genexis.eu>"); +MODULE_DESCRIPTION("Airoha EN7581 PWM driver"); +MODULE_LICENSE("GPL");