Hello, On Sat, Aug 31, 2024 at 04:27:50PM +0200, Lorenzo Bianconi wrote: > From: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx> > > Introduce driver for PWM module available on EN7581 SoC. > > Signed-off-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx> > Co-developed-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx> > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> > + */ 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@xxxxxxxxxx>"); > +MODULE_AUTHOR("Markus Gothe <markus.gothe@xxxxxxxxxx>"); > +MODULE_AUTHOR("Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>"); > +MODULE_DESCRIPTION("Airoha EN7581 PWM driver"); > +MODULE_LICENSE("GPL"); Best regards Uwe
Attachment:
signature.asc
Description: PGP signature