Re: [PATCH 2/2] pwm: Add support for Xilinx AXI Timer

From: Uwe Kleine-König
Date: Tue May 04 2021 - 04:51:20 EST

Next message: Frieder Schrempf: "Re: [PATCH V2 0/4] soc: imx: add i.MX BLK-CTL support"
Previous message: Yassine Oudjana: "[PATCH 2/2] dt-bindings: input: Add binding for cypress-sf"
In reply to: Alvaro G. M.: "Re: [PATCH 2/2] pwm: Add support for Xilinx AXI Timer"
Next in thread: Michal Simek: "Re: [PATCH 2/2] pwm: Add support for Xilinx AXI Timer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Frieder Schrempf: "Re: [PATCH V2 0/4] soc: imx: add i.MX BLK-CTL support"
Previous message: Yassine Oudjana: "[PATCH 2/2] dt-bindings: input: Add binding for cypress-sf"
In reply to: Alvaro G. M.: "Re: [PATCH 2/2] pwm: Add support for Xilinx AXI Timer"
Next in thread: Michal Simek: "Re: [PATCH 2/2] pwm: Add support for Xilinx AXI Timer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]