Messages in this thread | | | Date | Thu, 11 Apr 2024 10:29:49 -0400 | Subject | Re: [PATCH 2/2 v4] pwm: Add driver for AXI PWM generator | From | Trevor Gamblin <> |
| |
On 2024-04-11 16:07, Trevor Gamblin wrote: > > On 2024-04-11 12:59, Uwe Kleine-König wrote: >> Hello, > Hi, >> >> On Fri, Mar 01, 2024 at 12:33:42PM -0500, Trevor Gamblin wrote: >>> From: Drew Fustini <dfustini@baylibre.com> >>> >>> Add support for the Analog Devices AXI PWM Generator. This device is an >>> FPGA-implemented peripheral used as PWM signal generator and can be >>> interfaced with AXI4. The register map of this peripheral makes it >>> possible to configure the period and duty cycle of the output signal. >>> >>> Link: https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen >>> Co-developed-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> >>> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> >>> Co-developed-by: David Lechner <dlechner@baylibre.com> >>> Signed-off-by: David Lechner <dlechner@baylibre.com> >>> Signed-off-by: Drew Fustini <dfustini@baylibre.com> >>> Acked-by: Nuno Sa <nuno.sa@analog.com> >>> Co-developed-by: Trevor Gamblin <tgamblin@baylibre.com> >>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com> >>> --- >>> v4 changes: >>> * Address feedback for driver in v3: >>> * Update to use devm_pwmchip_alloc() function >>> * Simplify use of dev symbol in axi_pwmgen_probe >>> * Remove unnecessary axi_pwmgen_from_chip function and use >>> pwmchip_get_drvdata directly >>> >>> v3 changes: >>> * Address feedback for driver in v2: >>> * Remove unnecessary blank line in axi_pwmgen_apply >>> * Use macros already defined in <linux/fpga/adi-axi-common.h> for >>> version checking >>> >>> v2 changes: >>> * Address feedback for driver and device tree in v1: >>> * Use more reasonable Kconfig approach >>> * Use common prefixes for all functions >>> * Rename axi_pwmgen struct to axi_pwmgen_ddata >>> * Change use of "pwm" to "ddata" >>> * Set and check state->polarity >>> * Multiply safely with mul_u64_u64_div_u64() >>> * Improve handling of max and zero periods >>> * Error if clk_rate_hz > NSEC_PER_SEC >>> * Add "Limitations" section at top of pwm-axi-pwmgen.c >>> * Don't disable outputs by default >>> * Remove unnecessary macros for period, duty, offset >>> * Fix axi_pwmgen_ddata alignment >>> * Don't artificially limit npwm to four >>> * Use clk_rate_exclusive_get(), balance with >>> clk_rate_exclusive_put() >>> * Cache clk rate in axi_pwmgen_ddata >>> * Don't assign pwm->chip.base, do assign pwm->chip.atomic >>> * Remove redundant calls to clk_get_rate >>> * Test contents of AXI_PWMGEN_REG_CORE_MAGIC instead of >>> arbitrary AXI_PWMGEN_TEST_DATA in AXI_PWMGEN_REG_SCRATCHPAD >>> * Remove redundant clk struct from axi_pwmgen_ddata >>> * Add self as module author >>> * Add major version check for IP core >>> --- >>> MAINTAINERS | 1 + >>> drivers/pwm/Kconfig | 13 ++ >>> drivers/pwm/Makefile | 1 + >>> drivers/pwm/pwm-axi-pwmgen.c | 244 >>> +++++++++++++++++++++++++++++++++++ >>> 4 files changed, 259 insertions(+) >>> create mode 100644 drivers/pwm/pwm-axi-pwmgen.c >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index 8a4ed5545680..2baa7a0a1c8c 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -3438,6 +3438,7 @@ L: linux-pwm@vger.kernel.org >>> S: Supported >>> W: https://ez.analog.com/linux-software-drivers >>> F: Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >>> +F: drivers/pwm/pwm-axi-pwmgen.c >>> AXXIA I2C CONTROLLER >>> M: Krzysztof Adamski <krzysztof.adamski@nokia.com> >>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >>> index 4b956d661755..d44b0e86adee 100644 >>> --- a/drivers/pwm/Kconfig >>> +++ b/drivers/pwm/Kconfig >>> @@ -98,6 +98,19 @@ config PWM_ATMEL_TCB >>> To compile this driver as a module, choose M here: the module >>> will be called pwm-atmel-tcb. >>> +config PWM_AXI_PWMGEN >>> + tristate "Analog Devices AXI PWM generator" >>> + depends on MICROBLAZE || NIOS2 || ARCH_ZYNQ || ARCH_ZYNQMP || >>> ARCH_INTEL_SOCFPGA || COMPILE_TEST >>> + select REGMAP_MMIO >>> + help >>> + This enables support for the Analog Devices AXI PWM generator. >>> + >>> + This is a configurable PWM generator with variable pulse >>> width and >>> + period. >>> + >>> + To compile this driver as a module, choose M here: the module >>> will be >>> + called pwm-axi-pwmgen. >>> + >>> config PWM_BCM_IPROC >>> tristate "iProc PWM support" >>> depends on ARCH_BCM_IPROC || COMPILE_TEST >>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile >>> index c5ec9e168ee7..8322089954e9 100644 >>> --- a/drivers/pwm/Makefile >>> +++ b/drivers/pwm/Makefile >>> @@ -6,6 +6,7 @@ obj-$(CONFIG_PWM_APPLE) += pwm-apple.o >>> obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o >>> obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o >>> obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o >>> +obj-$(CONFIG_PWM_AXI_PWMGEN) += pwm-axi-pwmgen.o >>> obj-$(CONFIG_PWM_BCM_IPROC) += pwm-bcm-iproc.o >>> obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o >>> obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o >>> diff --git a/drivers/pwm/pwm-axi-pwmgen.c >>> b/drivers/pwm/pwm-axi-pwmgen.c >>> new file mode 100644 >>> index 000000000000..0c8f7f893a21 >>> --- /dev/null >>> +++ b/drivers/pwm/pwm-axi-pwmgen.c >>> @@ -0,0 +1,244 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Analog Devices AXI PWM generator >>> + * >>> + * Copyright 2024 Analog Devices Inc. >>> + * Copyright 2024 Baylibre SAS >>> + * >>> + * Limitations: >>> + * - The writes to registers for period and duty are shadowed until >>> + * LOAD_CONFIG is written to AXI_PWMGEN_REG_CONFIG at the end of the >>> + * current period. >> This implies that a currently running period is completed before the new >> settings get active, right? > Right. Sorry, my other reply is wrong here. It restarts the period right away, so the end of the comment is wrong. I'll fix that for the next rev as well. >> >>> + * - Writing LOAD_CONFIG also has the effect of re-synchronizing all >>> + * enabled channels, which could cause glitching on other >>> channels. It >>> + * is therefore expected that channels are assigned harmonic periods >>> + * and all have a single user coordinating this. >> What does "re-synchronize" mean here? Are all counters reset to zero? >> "harmonic" means that all channels should use the same period length? > Yes, it means that all counters are reset to zero. Harmonic in this > case means that channels can have different periods, but they should > be integer multiples of each other. Should I rewrite the comment to be > more explicit? >> >>> + * - Supports normal polarity. Does not support changing polarity. >> Please add an item documenting how the HW behaves on disable. (Typical >> behaviours are "freeze" or "inactive".) > Will do. >> >>> + */ >>> +#include <linux/bits.h> >>> +#include <linux/clk.h> >>> +#include <linux/err.h> >>> +#include <linux/fpga/adi-axi-common.h> >>> +#include <linux/io.h> >>> +#include <linux/module.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/pwm.h> >>> +#include <linux/regmap.h> >>> +#include <linux/slab.h> >>> + >>> +#define AXI_PWMGEN_REG_CORE_VERSION 0x00 >>> +#define AXI_PWMGEN_REG_ID 0x04 >>> +#define AXI_PWMGEN_REG_SCRATCHPAD 0x08 >>> +#define AXI_PWMGEN_REG_CORE_MAGIC 0x0C >>> +#define AXI_PWMGEN_REG_CONFIG 0x10 >>> +#define AXI_PWMGEN_REG_NPWM 0x14 >>> +#define AXI_PWMGEN_CHX_PERIOD(ch) (0x40 + (12 * (ch))) >>> +#define AXI_PWMGEN_CHX_DUTY(ch) (0x44 + (12 * (ch))) >>> +#define AXI_PWMGEN_CHX_OFFSET(ch) (0x48 + (12 * (ch))) >> Reading https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen I would >> have expected: >> >> /* ch in { 0, ... 15 } */ >> #define AXI_PWMGEN_REG_PULSE_X_PERIOD(ch) (0x40 + 4 * (ch)) >> #define AXI_PWMGEN_REG_PULSE_X_WIDTH(ch) (0x80 + 4 * (ch)) >> #define AXI_PWMGEN_REG_PULSE_X_OFFSET (0xc0 + 4 * (ch)) > > The regmap you find there now reflects v2 of the pwmgen IP; v1 used a > step of 12 instead of 4. The v2 series sent a little bit later on adds > this extra support: > https://lore.kernel.org/linux-pwm/20240314204722.1291993-1-tgamblin@baylibre.com/ > > I've added support for both versions since v1 of the IP could still be > in use on some devices. Would it be better to have the two patch > series squashed together into a v5 of the axi-pwmgen driver? > >> >>> +#define AXI_PWMGEN_REG_CORE_MAGIC_VAL 0x601A3471 /* >>> Identification number to test during setup */ >>> +#define AXI_PWMGEN_LOAD_CONFIG BIT(1) >>> +#define AXI_PWMGEN_RESET BIT(0) >> I'm a fan of including the register name in bitfield definitions. So >> maybe: >> >> #define AXI_PWMGEN_REG_CONFIG_RESET BIT(0) > Will do. >> >>> +struct axi_pwmgen_ddata { >>> + struct regmap *regmap; >>> + unsigned long clk_rate_hz; >>> +}; >>> + >>> +static const struct regmap_config axi_pwmgen_regmap_config = { >>> + .reg_bits = 32, >>> + .reg_stride = 4, >>> + .val_bits = 32, >>> +}; >>> + >>> +static int axi_pwmgen_apply(struct pwm_chip *chip, struct >>> pwm_device *pwm, >>> + const struct pwm_state *state) >>> +{ >>> + struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip); >>> + unsigned int ch = pwm->hwpwm; >>> + struct regmap *regmap = ddata->regmap; >>> + u64 period_cnt, duty_cnt; >>> + int ret; >>> + >>> + if (state->polarity != PWM_POLARITY_NORMAL) >>> + return -EINVAL; >>> + >>> + if (state->enabled) { >>> + period_cnt = mul_u64_u64_div_u64(state->period, >>> ddata->clk_rate_hz, NSEC_PER_SEC); >>> + if (period_cnt > UINT_MAX) >>> + period_cnt = UINT_MAX; >>> + >>> + if (period_cnt == 0) >>> + return -EINVAL; >>> + >>> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), >>> period_cnt); >>> + if (ret) >>> + return ret; >>> + >>> + duty_cnt = mul_u64_u64_div_u64(state->duty_cycle, >>> ddata->clk_rate_hz, NSEC_PER_SEC); >>> + if (duty_cnt > UINT_MAX) >>> + duty_cnt = UINT_MAX; >>> + >>> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt); >>> + if (ret) >>> + return ret; >>> + } else { >>> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), 0); >>> + if (ret) >>> + return ret; >>> + >>> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), 0); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, >>> AXI_PWMGEN_LOAD_CONFIG); >>> +} >>> + >>> +static int axi_pwmgen_get_state(struct pwm_chip *chip, struct >>> pwm_device *pwm, >>> + struct pwm_state *state) >>> +{ >>> + struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip); >>> + struct regmap *regmap = ddata->regmap; >>> + unsigned int ch = pwm->hwpwm; >>> + u32 cnt; >>> + int ret; >>> + >>> + ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt); >>> + if (ret) >>> + return ret; >>> + >>> + state->enabled = cnt != 0; >>> + >>> + state->period = DIV_ROUND_UP_ULL((u64)cnt * NSEC_PER_SEC, >>> ddata->clk_rate_hz); >>> + >>> + ret = regmap_read(regmap, AXI_PWMGEN_CHX_DUTY(ch), &cnt); >>> + if (ret) >>> + return ret; >>> + >>> + state->duty_cycle = DIV_ROUND_UP_ULL((u64)cnt * NSEC_PER_SEC, >>> ddata->clk_rate_hz); >>> + >>> + state->polarity = PWM_POLARITY_NORMAL; >>> + >>> + return 0; >>> +} >>> + >>> +static const struct pwm_ops axi_pwmgen_pwm_ops = { >>> + .apply = axi_pwmgen_apply, >>> + .get_state = axi_pwmgen_get_state, >>> +}; >>> + >>> +static int axi_pwmgen_setup(struct regmap *regmap, struct device *dev) >>> +{ >>> + int ret; >>> + u32 val; >>> + >>> + ret = regmap_read(regmap, AXI_PWMGEN_REG_CORE_MAGIC, &val); >>> + if (ret) >>> + return ret; >>> + >>> + if (val != AXI_PWMGEN_REG_CORE_MAGIC_VAL) >>> + return dev_err_probe(dev, -ENODEV, >>> + "failed to read expected value from register: got %08x, >>> expected %08x\n", >>> + val, >>> + AXI_PWMGEN_REG_CORE_MAGIC_VAL); >>> + >>> + ret = regmap_read(regmap, AXI_PWMGEN_REG_CORE_VERSION, &val); >>> + if (ret) >>> + return ret; >>> + >>> + if (ADI_AXI_PCORE_VER_MAJOR(val) != 1) { >>> + return dev_err_probe(dev, -ENODEV, "Unsupported peripheral >>> version %u.%u.%u\n", >>> + ADI_AXI_PCORE_VER_MAJOR(val), >>> + ADI_AXI_PCORE_VER_MINOR(val), >>> + ADI_AXI_PCORE_VER_PATCH(val)); >>> + } >>> + >>> + /* Enable the core */ >>> + ret = regmap_update_bits(regmap, AXI_PWMGEN_REG_CONFIG, >>> AXI_PWMGEN_RESET, 0); >>> + if (ret) >>> + return ret; >>> + >>> + ret = regmap_read(regmap, AXI_PWMGEN_REG_NPWM, &val); >>> + if (ret) >>> + return ret; >>> + >>> + /* Return the number of PWMs */ >>> + return val; >>> +} >>> + >>> +static void axi_pwmgen_clk_rate_exclusive_put(void *data) >>> +{ >>> + clk_rate_exclusive_put(data); >>> +} >>> + >>> +static int axi_pwmgen_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct regmap *regmap; >>> + struct pwm_chip *chip; >>> + struct axi_pwmgen_ddata *ddata; >>> + struct clk *clk; >>> + void __iomem *io_base; >>> + int ret; >>> + >>> + io_base = devm_platform_ioremap_resource(pdev, 0); >>> + if (IS_ERR(io_base)) >>> + return PTR_ERR(io_base); >>> + >>> + regmap = devm_regmap_init_mmio(dev, io_base, >>> &axi_pwmgen_regmap_config); >>> + if (IS_ERR(regmap)) >>> + return dev_err_probe(dev, PTR_ERR(regmap), >>> + "failed to init register map\n"); >>> + >>> + ret = axi_pwmgen_setup(regmap, dev); >>> + if (ret < 0) >>> + return ret; >>> + >>> + chip = devm_pwmchip_alloc(dev, ret, sizeof(*ddata)); >>> + if (IS_ERR(chip)) >>> + return PTR_ERR(chip); >>> + ddata = pwmchip_get_drvdata(chip); >>> + ddata->regmap = regmap; >>> + >>> + clk = devm_clk_get_enabled(dev, NULL); >>> + if (IS_ERR(clk)) >>> + return dev_err_probe(dev, PTR_ERR(clk), "failed to get >>> clock\n"); >>> + >>> + ret = clk_rate_exclusive_get(clk); >>> + if (ret) >>> + return dev_err_probe(dev, ret, "failed to get exclusive >>> rate\n"); >>> + >>> + ret = devm_add_action_or_reset(dev, >>> axi_pwmgen_clk_rate_exclusive_put, clk); >>> + if (ret) >>> + return ret; >> There is a devm variant of clk_rate_exclusive_get() in the meantime. > Okay, I'll try using that. >> >>> + ddata->clk_rate_hz = clk_get_rate(clk); >>> + if (!ddata->clk_rate_hz || ddata->clk_rate_hz > NSEC_PER_SEC) >>> + return dev_err_probe(dev, -EINVAL, >>> + "Invalid clock rate: %lu\n", ddata->clk_rate_hz); >>> + >>> + chip->ops = &axi_pwmgen_pwm_ops; >>> + chip->atomic = true; >>> + >>> + return devm_pwmchip_add(dev, chip); >> Error message in case devm_pwmchip_add fails, please. > > Will do. > > Thanks! > >> >>> +} >
| |