Hello, On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: > This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly > found on Xilinx FPGAs. There is another driver for this device located > at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. > This driver was written with reference to Xilinx DS764 for v1.03.a [1]. > > [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf > > Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx> > --- > > arch/arm64/configs/defconfig | 1 + > drivers/pwm/Kconfig | 11 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ > 4 files changed, 335 insertions(+) > create mode 100644 drivers/pwm/pwm-xilinx.c > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index 08c6f769df9a..81794209f287 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y > CONFIG_PWM_SL28CPLD=m > CONFIG_PWM_SUN4I=m > CONFIG_PWM_TEGRA=m > +CONFIG_PWM_XILINX=m > CONFIG_SL28CPLD_INTC=y > CONFIG_QCOM_PDC=y > CONFIG_RESET_IMX7=y I think this should go into a separate patch once this driver is accepted. This can then go via the ARM people. > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index d3371ac7b871..01e62928f4bf 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -628,4 +628,15 @@ config PWM_VT8500 > To compile this driver as a module, choose M here: the module > will be called pwm-vt8500. > > +config PWM_XILINX > + tristate "Xilinx AXI Timer PWM support" > + depends on !MICROBLAZE I don't understand this dependency. > + help > + PWM framework driver for Xilinx LogiCORE IP AXI Timers. This > + timer is typically a soft core which may be present in Xilinx > + FPGAs. If you don't have this IP in your design, choose N. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-xilinx. > + > endif > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index d3879619bd76..fc1bd6ccc9ed 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o > obj-$(CONFIG_PWM_TWL) += pwm-twl.o > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o > +obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o > diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c > new file mode 100644 > index 000000000000..240bd2993f97 > --- /dev/null > +++ b/drivers/pwm/pwm-xilinx.c > @@ -0,0 +1,322 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2021 Sean Anderson <sean.anderson@xxxxxxxx> > + */ Please add a link to a reference manual here (if available) and add a paragraph about hardware properties: Does the hardware complete cycles on reconfiguration or disable? Are there limitiation like "Cannot run with 100% duty cycle"? Please look into e.g. pwm-sifive how this is done and stick to the same format for easy grepping. > +#include <asm/io.h> > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/debugfs.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > + > +#define TCSR0 0x00 > +#define TLR0 0x04 > +#define TCR0 0x08 > +#define TCSR1 0x10 > +#define TLR1 0x14 > +#define TCR1 0x18 > + > +#define TCSR_MDT BIT(0) > +#define TCSR_UDT BIT(1) > +#define TCSR_GENT BIT(2) > +#define TCSR_CAPT BIT(3) > +#define TCSR_ARHT BIT(4) > +#define TCSR_LOAD BIT(5) > +#define TCSR_ENIT BIT(6) > +#define TCSR_ENT BIT(7) > +#define TCSR_TINT BIT(8) > +#define TCSR_PWMA BIT(9) > +#define TCSR_ENALL BIT(10) > +#define TCSR_CASC BIT(11) I'd like to see a prefix for these defines, something like XILINX_PWM_ maybe? > +/* Bits we need to set/clear to enable PWM mode, not including CASC */ > +#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA) > +#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD) Maybe call this XILINX_PWM_TCSR_DEFAULT? > +#define NSEC_PER_SEC_ULL 1000000000ULL > + > +/** > + * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver > + * @chip: PWM controller chip > + * @clk: Parent clock > + * @regs: Base address of this device > + * @width: Width of the counters, in bits > + */ > +struct xilinx_pwm_device { > + struct pwm_chip chip; > + struct clk *clk; > + void __iomem *regs; > + unsigned int width; > +}; > + > +static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip) > +{ > + return container_of(chip, struct xilinx_pwm_device, chip); > +} > + > +static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1) > +{ > + return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) || > + ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR); > +} > + > +static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr, > + unsigned int period) > +{ > + u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk), > + NSEC_PER_SEC_ULL); NSEC_PER_SEC should work just fine here. > + > + if (tcsr & TCSR_UDT) > + return cycles - 2; What happens if cycles <= 1? > + else > + return (BIT_ULL(pwm->width) - 1) - cycles + 2; What happens if cycles > BIT_ULL(pwm->width)? > +} > + > +static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm, > + u32 tlr, u32 tcsr) > +{ > + u64 cycles; > + > + if (tcsr & TCSR_UDT) > + cycles = tlr + 2; > + else > + cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2; > + > + return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL, > + clk_get_rate(pwm->clk)); Please round up here. The idea is that applying the result from .get_state() should not modify the configuration. You might want to enable PWM_DEBUG to test your driver. > +} > + > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, > + const struct pwm_state *state) > +{ > + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); > + u32 tlr0, tlr1; > + u32 tcsr0 = readl(pwm->regs + TCSR0); > + u32 tcsr1 = readl(pwm->regs + TCSR1); > + bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + if (!enabled && state->enabled) > + clk_rate_exclusive_get(pwm->clk); Missing error checking. > + > + tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period); Please care for overflowing. If state->period >= U64_MAX / clk_get_rate(pwm->clk) the multiplication in xilinx_pwm_calc_tlr is susceptible. Please do something like: ... calculate maximal period ... if (state->period > max_period) period = max_period else period = state->period; ... The algorithm to implement is: Configure the biggest possible period not bigger than state->period. With that period configure the biggest duty_cycle not bigger than state->duty_cycle. > + tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle); > + writel(tlr0, pwm->regs + TLR0); > + writel(tlr1, pwm->regs + TLR1); How does the hardware behave here? E.g. can it happen that after writing TLR0 we can see a cycle with the new period but the old duty_cycle? Does it complete the currently running period? If state->enabled == 0 you want to disable first to prevent that the (maybe) new values for period and duty_cycle are emitted by the hardware. > + if (state->enabled) { > + /* Only touch the TCSRs if we aren't already running */ > + if (!enabled) { > + /* Load TLR into TCR */ > + writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0); > + writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1); > + /* Enable timers all at once with ENALL */ > + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); > + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); > + writel(tcsr0, pwm->regs + TCSR0); > + writel(tcsr1, pwm->regs + TCSR1); > + } > + } else { > + writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0); > + writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1); How does the hardware behave on disable? Does it always emit a low level? Does it complete the currently running period? > + } > + > + if (enabled && !state->enabled) > + clk_rate_exclusive_put(pwm->clk); > + > + return 0; > +} > + > +static void xilinx_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *unused, > + struct pwm_state *state) > +{ > + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); > + u32 tlr0, tlr1, tcsr0, tcsr1; > + > + tlr0 = readl(pwm->regs + TLR0); > + tlr1 = readl(pwm->regs + TLR1); > + tcsr0 = readl(pwm->regs + TCSR0); > + tcsr1 = readl(pwm->regs + TCSR1); > + > + state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0); > + state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1); > + state->enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); > + state->polarity = PWM_POLARITY_NORMAL; > +} > + > +static const struct pwm_ops xilinx_pwm_ops = { > + .apply = xilinx_pwm_apply, > + .get_state = xilinx_pwm_get_state, .owner is missing > +}; > + > +static struct dentry *debug_dir; > + > +#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), } > +static struct debugfs_reg32 xilinx_pwm_reg32[] = { > + DEBUG_REG(TCSR0), > + DEBUG_REG(TLR0), > + DEBUG_REG(TCR0), > + DEBUG_REG(TCSR1), > + DEBUG_REG(TLR1), > + DEBUG_REG(TCR1), > +}; > + > +static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm) > +{ > + struct debugfs_regset32 *regset; > + > + if (!IS_ENABLED(CONFIG_DEBUG_FS)) > + return 0; > + > + regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL); > + if (!pwm) > + return -ENOMEM; > + > + regset->regs = xilinx_pwm_reg32; > + regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32); > + regset->base = pwm->regs; > + > + debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir, > + regset); debug_dir might not yet be initialized here. > + return 0; > +} This is unusual to have in a driver. Something like memtool[1] should work just fine, then you don't need this debugfs file. Together with pwm tracing this should be good enough. [1] https://github.com/pengutronix/memtool > +static int xilinx_pwm_probe(struct platform_device *pdev) > +{ > + bool enabled; > + int i, ret; > + struct device *dev = &pdev->dev; > + struct xilinx_pwm_device *pwm; > + u32 one_timer; > + > + ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only", > + &one_timer); > + if (ret || one_timer) { > + dev_err(dev, "two timers are needed for PWM mode\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < 2; i++) { > + char fmt[] = "xlnx,gen%u-assert"; > + char buf[sizeof(fmt)]; > + u32 gen; > + > + snprintf(buf, sizeof(buf), fmt, i); > + ret = of_property_read_u32(dev->of_node, buf, &gen); > + if (ret || !gen) { > + dev_err(dev, "generateout%u must be active high\n", i); > + return -EINVAL; > + } > + } > + > + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); > + if (!pwm) > + return -ENOMEM; > + platform_set_drvdata(pdev, pwm); > + > + pwm->chip.dev = &pdev->dev; > + pwm->chip.ops = &xilinx_pwm_ops; > + pwm->chip.base = -1; Please drop the assignment to .base. See commit f9a8ee8c8bcd118e800d88772c6457381db45224 in next. > + pwm->chip.npwm = 1; > + > + pwm->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(pwm->regs)) > + return PTR_ERR(pwm->regs); > + > + ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width); > + if (ret) { > + dev_err(dev, "missing counter width\n"); > + return -EINVAL; > + } Would it make sense to error out on some values of pwm->width? Values bigger than 32 don't make sense? > + pwm->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(pwm->clk)) > + return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n"); > + > + ret = clk_prepare_enable(pwm->clk); > + if (ret) { > + dev_err(dev, "clock enable failed\n"); > + return ret; You can use dev_err_probe here, too, for consistency. > + } > + > + ret = xilinx_pwm_debug_create(pwm); > + if (ret) { > + clk_disable_unprepare(pwm->clk); > + return ret; > + } > + > + ret = pwmchip_add(&pwm->chip); > + if (ret) { > + clk_disable_unprepare(pwm->clk); Error message please. > + return ret; > + } > + > + enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), > + readl(pwm->regs + TCSR1)); > + if (enabled) > + clk_rate_exclusive_get(pwm->clk); This needs to happen before calling pwmchip_add(), doesn't it? > + > + return ret; > +} > + > +static int xilinx_pwm_remove(struct platform_device *pdev) > +{ > + struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev); > + bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), > + readl(pwm->regs + TCSR1)); > + > + if (enabled) > + clk_rate_exclusive_put(pwm->clk); This looks wrong. You should rely on the consumer that they disable the PWM. > + clk_disable_unprepare(pwm->clk); I assume this stops the PWM, so this must happen after calling pwmchip_remove. > + return pwmchip_remove(&pwm->chip); Note, pwmchip_remove always returns 0 and I plan to change it to return void instead. So please don't check the return value here. > +} > + Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
