| Message ID | 20240301173343.1086332-3-tgamblin@baylibre.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: add axi-pwm-gen driver | expand |
Hello, 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? > + * - 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? > + * - 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".) > + */ > +#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)) > +#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) > +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. > + 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. > +}
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. > >> + * - 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! > >> +}
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! > >> >>> +} >
Hello, On Thu, Apr 11, 2024 at 10:07:54AM -0400, Trevor Gamblin wrote: > On 2024-04-11 12:59, Uwe Kleine-König wrote: > > > + * - 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? I hesitate to say "yes, please be more specific" because I think it's mood. If all pwm lines restart with their counter = 0 as soon as one line is reconfigured (without completing the current period) being a multiple of each other doesn't help at all. So I think the right thing to write there is: - Reconfiguring a channel doesn't complete the currently running period and resets the counters of all other channels and so very likely introduces glitches on these unrelated outputs. (Even if the period was completed, and only assuming configuration updates that don't modify the period, all channels that don't have a period that is a divider of the just configured line (might) glitch. So if you have one PWM with period = 200 and another with period = 400, everything is fine if you update the latter, however updating the former might make the latter glitch. So essentially you need to have a single period for all channels. That's why I asked if "harmonic" means that all channels should use the same period.) > > 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? Not necessarily squashed, but I suggest to send them in a single series. (Note, this doesn't mean "Don't squash". I didn't look at the other series yet, so make a sensible choice yourself (or wait until I come around reviewing that other series and hope that I remember the context to comment about this question. :-) Best regards Uwe
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. + * - 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. + * - Supports normal polarity. Does not support changing polarity. + */ +#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))) +#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) + +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; + + 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); +} + +static const struct of_device_id axi_pwmgen_ids[] = { + { .compatible = "adi,axi-pwmgen-1.00.a" }, + { } +}; +MODULE_DEVICE_TABLE(of, axi_pwmgen_ids); + +static struct platform_driver axi_pwmgen_driver = { + .driver = { + .name = "axi-pwmgen", + .of_match_table = axi_pwmgen_ids, + }, + .probe = axi_pwmgen_probe, +}; +module_platform_driver(axi_pwmgen_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Sergiu Cuciurean <sergiu.cuciurean@analog.com>"); +MODULE_AUTHOR("Trevor Gamblin <tgamblin@baylibre.com>"); +MODULE_DESCRIPTION("Driver for the Analog Devices AXI PWM generator");