Hello,
On Wed, Oct 23, 2024 at 01:20:06AM +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>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> Co-developed-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-airoha.c | 386 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 398 insertions(+)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16d451e987dcc5f10e0b57edc32ee1..99aa87136c272555c10102590fcf9f911161c3d3 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -54,6 +54,17 @@ config PWM_ADP5585
> This option enables support for the PWM function found in the Analog
> Devices ADP5585.
>
> +config PWM_AIROHA
> + tristate "Airoha PWM support"
> + depends on ARCH_AIROHA || COMPILE_TEST
> + depends on OF
> + select REGMAP_MMIO
> + 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 9081e0c0e9e09713fe05479c257eebe5f02b91e9..fbf7723d845807fd1e2893c6ea4f736785841b0d 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -2,6 +2,7 @@
> obj-$(CONFIG_PWM) += core.o
> obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
> obj-$(CONFIG_PWM_ADP5585) += pwm-adp5585.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 0000000000000000000000000000000000000000..658884852e7ad7a76ff499879eda9c50dfc5f745
> --- /dev/null
> +++ b/drivers/pwm/pwm-airoha.c
> @@ -0,0 +1,386 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2022 Markus Gothe <markus.gothe@xxxxxxxxxx>
> + *
> + * Limitations:
> + * - No disable bit, so a disabled PWM is simulated by setting duty_cycle to 0
> + * - Only 8 concurrent waveform generators are available for 8 combinations of
> + * duty_cycle and period. Waveform generators are shared between 16 GPIO
> + * pins and 17 SIPO GPIO pins.
> + * - Supports only normal polarity.
> + * - On configuration the currently running period is completed.
Maybe I already asked before: Is there a public manual? If so, please
add a link here.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mfd/syscon.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 <linux/regmap.h>
> +#include <asm/div64.h>
> +
> +#define REG_SGPIO_LED_DATA 0x0024
> +#define SGPIO_LED_DATA_SHIFT_FLAG BIT(31)
> +#define SGPIO_LED_DATA_DATA GENMASK(16, 0)
> +
> +#define REG_SGPIO_CLK_DIVR 0x0028
> +#define REG_SGPIO_CLK_DIVR_MASK GENMASK(1, 0)
> +#define REG_SGPIO_CLK_DLY 0x002c
> +
> +#define REG_SIPO_FLASH_MODE_CFG 0x0030
> +#define SERIAL_GPIO_FLASH_MODE BIT(1)
> +#define SERIAL_GPIO_MODE_74HC164 BIT(0)
> +
> +#define REG_GPIO_FLASH_PRD_SET(_n) (0x003c + ((_n) << 2))
> +#define GPIO_FLASH_PRD_MASK(_n) GENMASK(15 + ((_n) << 4), ((_n) << 4))
> +
> +#define REG_GPIO_FLASH_MAP(_n) (0x004c + ((_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) (0x0054 + ((_n) << 2))
> +
> +#define REG_CYCLE_CFG_VALUE(_n) (0x0098 + ((_n) << 2))
> +#define WAVE_GEN_CYCLE_MASK(_n) GENMASK(7 + ((_n) << 3), ((_n) << 3))
> +
> +#define PWM_NUM_BUCKETS 8
> +
> +struct airoha_pwm_bucket {
> + /* Bitmask of PWM channels using this bucket */
> + u64 used;
> + u64 period_ns;
> + u64 duty_ns;
> +};
> +
> +struct airoha_pwm {
> + struct regmap *regmap;
> +
> + struct device_node *np;
> + u64 initialized;
> +
> + struct airoha_pwm_bucket bucket[PWM_NUM_BUCKETS];
> +};
> +
> +/*
> + * The first 16 GPIO pins, GPIO0-GPIO15, are mapped into 16 PWM channels, 0-15.
> + * The SIPO GPIO pins are 17 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 (4 * NSEC_PER_MSEC)
> +#define PERIOD_MAX_NS (1 * NSEC_PER_SEC)
> +/* 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
Please add proper namespacing to these constants, some of these make a
more generic impression than justified. (i.e. add a common prefix like
"AIROHA_PWM_")
> +static int airoha_pwm_get_generator(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)
I wonder: The PWM supports periods starting at 4ms, but 255 is the
maximal value for duty_ns? Isn't that a misnomer and is about
duty_ticks?
> + return i;
> + }
> +
> + return -1;
> +}
> +
> +static void airoha_pwm_release_bucket_config(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_generator(struct airoha_pwm *pc,
> + u64 duty_ns, u64 period_ns,
> + unsigned int hwpwm)
> +{
> + int id = airoha_pwm_get_generator(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;
> + }
> + }
> + }
I think airoha_pwm_get_generator could return an unused generator, then
you don't need to loop twice over all buckets. Just an idea, not sure
this becomes considerably faster or easier to understand.
> + if (id >= 0) {
> + airoha_pwm_release_bucket_config(pc, hwpwm);
> + pc->bucket[id].used |= BIT_ULL(hwpwm);
> + pc->bucket[id].period_ns = period_ns;
> + pc->bucket[id].duty_ns = duty_ns;
> + }
> +
> + return id;
> +}
> +
> +static int airoha_pwm_sipo_init(struct airoha_pwm *pc)
> +{
> + u32 val;
> +
> + if (!(pc->initialized >> PWM_NUM_GPIO))
> + return 0;
> +
> + regmap_clear_bits(pc->regmap, REG_SIPO_FLASH_MODE_CFG,
> + SERIAL_GPIO_MODE_74HC164);
> +
> + /* Configure shift register timings, use 32x divisor */
> + regmap_write(pc->regmap, REG_SGPIO_CLK_DIVR,
> + FIELD_PREP(REG_SGPIO_CLK_DIVR_MASK, 0x3));
> +
> + /*
> + * The actual delay is clock + 1.
> + * Notice that clock delay should not be greater
> + * than (divisor / 2) - 1.
> + * Set to 0 by default. (aka 1)
> + */
> + regmap_write(pc->regmap, REG_SGPIO_CLK_DLY, 0x0);
> +
> + /*
> + * 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 (regmap_read_poll_timeout(pc->regmap, REG_SGPIO_LED_DATA, val,
> + !(val & SGPIO_LED_DATA_SHIFT_FLAG), 10,
> + 200 * USEC_PER_MSEC))
> + return -ETIMEDOUT;
> +
> + regmap_clear_bits(pc->regmap, REG_SGPIO_LED_DATA, SGPIO_LED_DATA_DATA);
> + if (regmap_read_poll_timeout(pc->regmap, REG_SGPIO_LED_DATA, val,
> + !(val & SGPIO_LED_DATA_SHIFT_FLAG), 10,
> + 200 * USEC_PER_MSEC))
> + return -ETIMEDOUT;
> +
> + /* Set SIPO in PWM mode */
> + regmap_set_bits(pc->regmap, REG_SIPO_FLASH_MODE_CFG,
> + SERIAL_GPIO_FLASH_MODE);
> +
> + return 0;
> +}
> +
> +static void airoha_pwm_calc_bucket_config(struct airoha_pwm *pc, int index,
> + u64 duty_ns, u64 period_ns)
> +{
> + u32 period, duty, mask, val;
Please use duty_ns for values in nanoseconds and duty_ticks (or
something similar) for values in clock ticks consistently.
> + u64 tmp;
> +
> + tmp = duty_ns * DUTY_FULL;
This might overflow. => Use mul_u64_u64_div_u64() or a suitable cousin
of it.
> + duty = clamp_val(div64_u64(tmp, period_ns), 0, DUTY_FULL);
> + tmp = period_ns * 25;
> + period = clamp_val(div64_u64(tmp, 100000000), PERIOD_MIN, PERIOD_MAX);
Huh, we already know that period_ns >= PERIOD_MIN_NS = 4000000, so
period is >= PERIOD_MIN = 1 already. So the lower bound part of that
clamping is not needed. (And it looks wrong because you're not supposed
to round up.) Ditto for the duty_clamp, tmp/period_ns will never be less
than 0.
> + /* Configure frequency divisor */
> + mask = WAVE_GEN_CYCLE_MASK(index % 4);
> + val = (period << __ffs(mask)) & mask;
FIELD_PREP please.
> + regmap_update_bits(pc->regmap, REG_CYCLE_CFG_VALUE(index / 4),
> + mask, val);
> +
> + /* Configure duty cycle */
> + duty = ((DUTY_FULL - duty) << 8) | duty;
Please use proper defines for the bit fields and then use FIELD_PREP
here instead of shifts with an explicit number as RHS.
> + mask = GPIO_FLASH_PRD_MASK(index % 2);
> + val = (duty << __ffs(mask)) & mask;
> + regmap_update_bits(pc->regmap, 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
Together with "No disable bit, so a disabled PWM is simulated by setting
duty_cycle to 0" from the Limitations paragraph above I don't understand
that. And if disabling has some delay but reconfiguration doesn't, it
would give a more consistent behaviour to configure a duty_cycle = 0
here?
> + */
> + regmap_clear_bits(pc->regmap, addr, GPIO_FLASH_EN(hwpwm % 8));
> + return;
> + }
> +
> + mask = GPIO_FLASH_SETID_MASK(hwpwm % 8);
> + val = ((index & 7) << __ffs(mask)) & mask;
> + regmap_update_bits(pc->regmap, addr, mask, val);
> + regmap_set_bits(pc->regmap, 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)
> +{
> + int index = -1;
This initialisation isn't needed.
> + index = airoha_pwm_consume_generator(pc, duty_ns, period_ns,
> + 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) {
After the return above index >= 0 is always true.
> + airoha_pwm_calc_bucket_config(pc, index, duty_ns, period_ns);
> + airoha_pwm_config_flash_map(pc, pwm->hwpwm, index);
> + } else {
> + airoha_pwm_config_flash_map(pc, pwm->hwpwm, index);
> + airoha_pwm_release_bucket_config(pc, pwm->hwpwm);
> + }
> +
> + pc->initialized |= BIT_ULL(pwm->hwpwm);
> +
> + return 0;
> +}
> +
> +static void airoha_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct airoha_pwm *pc = pwmchip_get_drvdata(chip);
> +
> + /* Disable PWM and release the waveform */
s/waveform/bucket/ ?
> + airoha_pwm_config_flash_map(pc, pwm->hwpwm, -1);
> + airoha_pwm_release_bucket_config(pc, pwm->hwpwm);
> +
> + pc->initialized &= ~BIT_ULL(pwm->hwpwm);
> + if (!(pc->initialized >> PWM_NUM_GPIO))
> + regmap_clear_bits(pc->regmap, REG_SIPO_FLASH_MODE_CFG,
> + SERIAL_GPIO_FLASH_MODE);
> +}
> +
> +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;
> + u64 period = state->period;
> +
> + /* Only normal polarity is supported */
> + if (state->polarity == PWM_POLARITY_INVERSED)
> + return -EINVAL;
> +
> + if (!state->enabled) {
> + airoha_pwm_disable(chip, pwm);
> + return 0;
> + }
> +
> + if (period < PERIOD_MIN_NS)
> + return -EINVAL;
> +
> + if (period > PERIOD_MAX_NS)
> + period = PERIOD_MAX_NS;
If you ask me, don't check the nanosecond values, but convert them to
ticks and check only then. That feels more natural.
> + return airoha_pwm_config(pc, pwm, duty, period);
> +}
> +
> +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 = PWM_POLARITY_NORMAL;
> + state->period = pc->bucket[i].period_ns;
> + state->duty_cycle = pc->bucket[i].duty_ns;
> + break;
> + }
> + }
Is it worth to maintain the information about index of the used bucket
for a given pwm_device in airoha_pwm to save a few loops?
> + 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,
I'm a big fan of consistency, so you can make me a bit more happy if you
define airoha_pwm_get_state() before airoha_pwm_apply() or switch the
order here. (The latter is slightly preferred as this matches the order
of their definition in struct pwm_ops.)
> +};
> +
> +static int airoha_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct airoha_pwm *pc;
> + struct pwm_chip *chip;
> +
> + chip = devm_pwmchip_alloc(dev, PWM_NUM_GPIO + PWM_NUM_SIPO,
> + sizeof(*pc));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + chip->ops = &airoha_pwm_ops;
> + pc = pwmchip_get_drvdata(chip);
> + pc->np = dev->of_node;
.np is a write only struct member and so can be dropped.
> +
> + pc->regmap = device_node_to_regmap(dev->parent->of_node);
> + if (IS_ERR(pc->regmap))
> + return PTR_ERR(pc->regmap);
> +
> + return devm_pwmchip_add(&pdev->dev, chip);
Please add error messages to .probe() using dev_err_probe().
> +}
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
