Re: [PATCH v4 3/3] pwm: pwm-cadence: Add support for TTC PWM
From: Uwe Kleine-König
Date: Mon Mar 24 2025 - 07:29:53 EST
Hello,
On Wed, Jan 15, 2025 at 05:05:56PM +0530, Mubin Sayyed wrote:
> Cadence TTC timer can be configured as clocksource/clockevent or PWM
> device.Specific TTC device would be configured as PWM device, if
> pwm-cells property is present in the device tree node.
>
> In case of Zynq, ZynqMP and Versal SoC's, each TTC device has 3
> timers/counters, so maximum 3 PWM channels can be configured for each TTC
> IP instance. Also, output of 0th PWM channel of each TTC device can be
> routed to MIO or EMIO, and output of 2nd and 3rd PWM channel can be
> routed only to EMIO.
>
> Period for given PWM channel is configured through interval timer and
> duty cycle through match counter.
>
> Details for cadence TTC IP can be found in Zynq UltraScale+ TRM.
>
> Signed-off-by: Mubin Sayyed <mubin.sayyed@xxxxxxx>
> ---
> Refer link given below for Zynq UltraScale+ TRM
> https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm
I would prefer to have a link to the TRM in the source file. When I
follow that link today however I only get "The document you are looking
for has been moved or deleted" :-\
> Changes for v4:
> Configure it as part of TTC clocksource/clockevent driver
> drivers/clocksource/timer-cadence-ttc.c.
> Move probe/remove function to timer-cadence-ttc.c.
> Changes for v3:
> None
> Changes for v2:
> Use maybe_unused attribute for ttc_pwm_of_match_driver structure
> Add new function ttc_pwm_set_polarity
> Removed calls to pwm_get_state
> Replace DIV_ROUNF_CLOSEST with mul_u64_u64_div_u64
> Modify ttc_pwm_apply to remove while loop in prescalar logic
> and avoid glitch
> Calculate rate in probe and add it to private structure for further
> Drop ttc_pwm_of_xlate
> Replace of_clk_get with devm_clk_get_enabled
> Drop _OFFSET and _MASK from definitions
> Keep Kconfig and Makefile changes alphabetically sorted
> Use remove_new instead of remove
> Document limitations in driver file
> ---
> drivers/pwm/Kconfig | 10 +
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-cadence.c | 323 ++++++++++++++++++++++++++++++
> include/linux/timer-cadence-ttc.h | 22 +-
> 4 files changed, 355 insertions(+), 1 deletion(-)
> create mode 100644 drivers/pwm/pwm-cadence.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16..b418e5d8fa42 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -202,6 +202,16 @@ config PWM_CROS_EC
> PWM driver for exposing a PWM attached to the ChromeOS Embedded
> Controller.
>
> +config PWM_CADENCE
> + bool "Cadence TTC PWM driver"
tristate please
> + depends on CADENCE_TTC_TIMER
> + help
> + Generic PWM framework driver for cadence TTC IP found on
> + Xilinx Zynq/ZynqMP/Versal SOCs. Each TTC device has 3 PWM
> + channels. Output of 0th PWM channel of each TTC device can
> + be routed to MIO or EMIO, and output of 1st and 2nd PWM
> + channels can be routed only to EMIO.
> +
> config PWM_DWC_CORE
> tristate
> depends on HAS_IOMEM
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9081e0c0e9e0..246380391a63 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o
> obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o
> obj-$(CONFIG_PWM_BERLIN) += pwm-berlin.o
> obj-$(CONFIG_PWM_BRCMSTB) += pwm-brcmstb.o
> +obj-$(CONFIG_PWM_CADENCE) += pwm-cadence.o
> obj-$(CONFIG_PWM_CLK) += pwm-clk.o
> obj-$(CONFIG_PWM_CLPS711X) += pwm-clps711x.o
> obj-$(CONFIG_PWM_CRC) += pwm-crc.o
> diff --git a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c
> new file mode 100644
> index 000000000000..e7c337fe956b
> --- /dev/null
> +++ b/drivers/pwm/pwm-cadence.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver to configure cadence TTC timer as PWM
> + * generator
> + *
> + * Limitations:
> + * - When PWM is stopped, timer counter gets stopped immediately. This
> + * doesn't allow the current PWM period to complete and stops abruptly.
> + * - Disabled PWM emits inactive level.
> + * - When user requests a change in any parameter of PWM (period/duty cycle/polarity)
s/ / /
> + * while PWM is in enabled state:
> + * - PWM is stopped abruptly.
> + * - Requested parameter is changed.
> + * - Fresh PWM cycle is started.
> + *
> + * Copyright (C) 2025, Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
I didn't check, but <linux/device.h> is unusual. Do you really need it?
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/of_address.h>
Also I think this can be dropped.
> +#include <linux/timer-cadence-ttc.h>
> +
> +/**
> + * struct ttc_pwm_priv - Private data for TTC PWM drivers
> + * @chip: PWM chip structure representing PWM controller
> + * @clk: TTC input clock
> + * @rate: TTC input clock rate
> + * @max: Maximum value of the counters
> + * @base: Base address of TTC instance
> + */
> +struct ttc_pwm_priv {
> + struct pwm_chip chip;
> + struct clk *clk;
> + unsigned long rate;
> + u32 max;
> + void __iomem *base;
> +};
> +
> +static inline u32 ttc_pwm_readl(struct ttc_pwm_priv *priv,
> + unsigned long offset)
> +{
> + return readl_relaxed(priv->base + offset);
> +}
> +
> +static inline void ttc_pwm_writel(struct ttc_pwm_priv *priv,
> + unsigned long offset,
> + unsigned long val)
> +{
> + writel_relaxed(val, priv->base + offset);
> +}
> +
> +static inline u32 ttc_pwm_ch_readl(struct ttc_pwm_priv *priv,
> + unsigned int chnum,
> + unsigned long offset)
> +{
> + unsigned long pwm_ch_offset = offset +
> + (TTC_PWM_CHANNEL * chnum);
> +
> + return ttc_pwm_readl(priv, pwm_ch_offset);
> +}
> +
> +static inline void ttc_pwm_ch_writel(struct ttc_pwm_priv *priv,
> + unsigned int chnum,
> + unsigned long offset,
> + unsigned long val)
> +{
> + unsigned long pwm_ch_offset = offset +
> + (TTC_PWM_CHANNEL * chnum);
> +
> + ttc_pwm_writel(priv, pwm_ch_offset, val);
> +}
> +
> +static inline struct ttc_pwm_priv *xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
Can you please stick to a single unique function prefix? The involved
prefixes here are ttc_pwm, xilinx_pwm and the driver is called
"cadence". Unifying them all to a single name would be good.
> +{
> + return pwmchip_get_drvdata(chip);
> +}
> +
> +static void ttc_pwm_enable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
> +{
> + u32 ctrl_reg;
> +
> + ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
> + ctrl_reg |= (TTC_CNTR_CTRL_INTR_MODE_EN
> + | TTC_CNTR_CTRL_MATCH_MODE_EN | TTC_CNTR_CTRL_RST);
> + ctrl_reg &= ~(TTC_CNTR_CTRL_DIS | TTC_CNTR_CTRL_WAVE_EN);
> + ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_disable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
This function only needs .hwpwm from *pwm. Maybe pass hwpwm as parameter
instead of pwm?
> +{
> + u32 ctrl_reg;
> +
> + ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
> + ctrl_reg |= TTC_CNTR_CTRL_DIS;
> +
> + ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_set_polarity(struct ttc_pwm_priv *priv, struct pwm_device *pwm,
> + enum pwm_polarity polarity)
> +{
> + u32 ctrl_reg;
> +
> + ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
> +
> + if (polarity == PWM_POLARITY_NORMAL)
> + ctrl_reg |= TTC_CNTR_CTRL_WAVE_POL;
> + else
> + ctrl_reg &= (~TTC_CNTR_CTRL_WAVE_POL);
The parenthesis can be dropped.
> +
> + ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_set_counters(struct ttc_pwm_priv *priv,
> + struct pwm_device *pwm,
> + u32 period_cycles,
> + u32 duty_cycles)
> +{
> + /* Set up period */
> + ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_INTR_VAL_OFFSET, period_cycles);
> +
> + /* Set up duty cycle */
> + ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_MATCH_CNT_VAL_OFFSET, duty_cycles);
> +}
> +
> +static void ttc_pwm_set_prescalar(struct ttc_pwm_priv *priv,
> + struct pwm_device *pwm,
> + u32 div, bool is_enable)
> +{
> + u32 clk_reg;
> +
> + if (is_enable) {
> + /* Set up prescalar */
> + clk_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET);
> + clk_reg &= ~TTC_CLK_CNTRL_PSV_MASK;
> + clk_reg |= (div << TTC_CNTR_CTRL_PRESCALE_SHIFT);
> + clk_reg |= TTC_CLK_CNTRL_PS_EN;
> + ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
> + } else {
> + /* Disable prescalar */
> + clk_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET);
> + clk_reg &= ~TTC_CLK_CNTRL_PS_EN;
> + ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
> + }
> +}
> +
> +static int ttc_pwm_apply(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> + u64 duty_cycles, period_cycles;
> + struct pwm_state cstate;
> + unsigned long rate;
> + bool flag = false;
> + u32 div = 0;
> +
> + cstate = pwm->state;
A pointer would be enough here. No need to copy the whole struct.
> + if (state->polarity != cstate.polarity) {
> + if (cstate.enabled)
> + ttc_pwm_disable(priv, pwm);
> +
> + ttc_pwm_set_polarity(priv, pwm, state->polarity);
> + }
> +
> + rate = priv->rate;
> +
> + /* Prevent overflow by limiting to the maximum possible period */
> + period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
ULONG_MAX * NSEC_PER_SEC overflows before it's casted to u64.
> + period_cycles = mul_u64_u64_div_u64(period_cycles, rate, NSEC_PER_SEC);
mul_u64_u64_div_u64() doesn't overflow if rate <= NSEC_PER_SEC.
> + if (period_cycles > priv->max) {
> + /*
> + * Prescale frequency to fit requested period cycles within limit.
> + * Prescalar divides input clock by 2^(prescale_value + 1). Maximum
> + * supported prescalar value is 15.
> + */
> + div = mul_u64_u64_div_u64(state->period, rate, (NSEC_PER_SEC * priv->max));
No parenthesis needed around the 3rd parameter. Can NSEC_PER_SEC *
priv->max overflow? Is it intended that you use state->period here and
don't cap the value first as you did above?
> + div = order_base_2(div);
> + if (div)
> + div -= 1;
> +
> + if (div > 15)
> + return -ERANGE;
> +
> + rate = DIV_ROUND_CLOSEST(rate, BIT(div + 1));
> + period_cycles = mul_u64_u64_div_u64(state->period, rate,
> + NSEC_PER_SEC);
Dividing twice decreases precision. Also rounding to closest looks
wrong. I think this should just be:
period_cycles = mul_u64_u64_div_u64(state->period, rate, NSEC_PER_SEC * BIT(div + 1));
Wouldn't it be simpler to just use:
dif = order_base_2(period_cycles / priv->max);
(modulo correctness)?
> + flag = true;
> + }
> +
> + if (cstate.enabled)
> + ttc_pwm_disable(priv, pwm);
> +
> + duty_cycles = mul_u64_u64_div_u64(state->duty_cycle, rate,
> + NSEC_PER_SEC);
> + ttc_pwm_set_counters(priv, pwm, period_cycles, duty_cycles);
> +
> + ttc_pwm_set_prescalar(priv, pwm, div, flag);
> +
> + if (state->enabled)
> + ttc_pwm_enable(priv, pwm);
> + else
> + ttc_pwm_disable(priv, pwm);
The hardware is already disabled, so I'd expect that this
ttc_pwm_disable() can be dropped?
> + return 0;
> +}
> +
> +static int ttc_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> + u32 value, pres_en, pres = 1;
> + unsigned long rate;
> + u64 tmp;
> +
> + value = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL_OFFSET);
> +
> + if (value & TTC_CNTR_CTRL_WAVE_POL)
> + state->polarity = PWM_POLARITY_NORMAL;
> + else
> + state->polarity = PWM_POLARITY_INVERSED;
> +
> + if (value & TTC_CNTR_CTRL_DIS)
> + state->enabled = false;
You can exit early here.
> + else
> + state->enabled = true;
> +
> + rate = priv->rate;
> +
> + pres_en = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET);
> + pres_en &= TTC_CLK_CNTRL_PS_EN;
> +
> + if (pres_en) {
> + pres = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL_OFFSET)
> + & TTC_CLK_CNTRL_PSV_MASK;
> + pres >>= TTC_CNTR_CTRL_PRESCALE_SHIFT;
pres = FIELD_GET(...)
> + /* If prescale is enabled, the count rate is divided by 2^(pres + 1) */
> + pres = BIT(pres + 1);
> + }
> +
> + tmp = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_INTR_VAL_OFFSET);
> + tmp *= pres;
you can drop `pres = BIT(pres + 1);` above if you use
tmp <<= pres + 1
here.
> + state->period = DIV64_U64_ROUND_UP(tmp * NSEC_PER_SEC, rate);
Can this overflow?
> + tmp = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_MATCH_CNT_VAL_OFFSET);
> + tmp *= pres;
> + state->duty_cycle = DIV64_U64_ROUND_UP(tmp * NSEC_PER_SEC, rate);
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops ttc_pwm_ops = {
> + .apply = ttc_pwm_apply,
> + .get_state = ttc_pwm_get_state,
> +};
> +
> +int ttc_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct ttc_pwm_priv *priv;
> + struct pwm_chip *chip;
> + u32 timer_width;
> + int ret;
> +
> + ret = of_property_read_u32(np, "timer-width", &timer_width);
> + if (ret)
> + timer_width = 16;
> +
> + chip = devm_pwmchip_alloc(dev, TTC_PWM_MAX_CH, sizeof(*priv));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + priv = xilinx_pwm_chip_to_priv(chip);
> + priv->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(priv->base))
> + return PTR_ERR(priv->base);
> +
> + priv->max = BIT(timer_width) - 1;
> +
> + priv->clk = devm_clk_get_enabled(dev, NULL);
> + if (IS_ERR(priv->clk)) {
> + return dev_err_probe(dev, PTR_ERR(priv->clk),
> + "ERROR: timer input clock not found\n");
> + }
> +
> + priv->rate = clk_get_rate(priv->clk);
> +
> + clk_rate_exclusive_get(priv->clk);
Only call clk_get_rate() after clk_rate_exclusive_get(). Also note there
is a devm variant of clk_rate_exclusive_get().
> + chip->ops = &ttc_pwm_ops;
> + chip->npwm = TTC_PWM_MAX_CH;
> +
> + ret = devm_pwmchip_add(dev, chip);
> + if (ret) {
> + clk_rate_exclusive_put(priv->clk);
> + return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> + }
> +
> + platform_set_drvdata(pdev, priv);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ttc_pwm_probe);
Putting these functions in a name space would be great.
> +void ttc_pwm_remove(struct platform_device *pdev)
> +{
> + struct ttc_pwm_priv *priv = platform_get_drvdata(pdev);
> +
> + pwmchip_remove(&priv->chip);
> + clk_rate_exclusive_put(priv->clk);
> +}
Did you test the remove path? Hint: Don't call pwmchip_remove() if you
registered the chip using devm_pwmchip_add() and priv->chip is
uninitialized.
> +EXPORT_SYMBOL_GPL(ttc_pwm_remove);
> +
> +MODULE_AUTHOR("Mubin Sayyed <mubin.sayyed@xxxxxxx>");
> +MODULE_DESCRIPTION("Cadence TTC PWM driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/timer-cadence-ttc.h b/include/linux/timer-cadence-ttc.h
> index d938991371e5..6b6135d0ba0c 100644
> --- a/include/linux/timer-cadence-ttc.h
> +++ b/include/linux/timer-cadence-ttc.h
> @@ -12,13 +12,14 @@
> #define TTC_CNT_CNTRL_OFFSET 0x0C /* Counter Control Reg, RW */
> #define TTC_COUNT_VAL_OFFSET 0x18 /* Counter Value Reg, RO */
> #define TTC_INTR_VAL_OFFSET 0x24 /* Interval Count Reg, RW */
> +#define TTC_MATCH_CNT_VAL_OFFSET 0x30 /* Match Count Reg, RW */
I assume all these constants are register addresses. I'd drop _OFFSET
here. If you want a designator for these, I'd suggest REG or ADDR, but
IMHO plain TTC_MATCH_CNT_VAL is fine.
> #define TTC_ISR_OFFSET 0x54 /* Interrupt Status Reg, RO */
> #define TTC_IER_OFFSET 0x60 /* Interrupt Enable Reg, RW */
>
> #define TTC_CNT_CNTRL_DISABLE_MASK 0x1
>
> #define TTC_CLK_CNTRL_CSRC_MASK (1 << 5) /* clock source */
> -#define TTC_CLK_CNTRL_PSV_MASK 0x1e
> +#define TTC_CLK_CNTRL_PSV_MASK 0x1e
> #define TTC_CLK_CNTRL_PSV_SHIFT 1
>
> /*
> @@ -33,3 +34,22 @@
>
> #define MAX_F_ERR 50
>
> +#define TTC_PWM_CHANNEL 0x4
That define is misnamed IMHO. That's the offset between register ranges
for different channels. *Here* _OFFSET is fine.
> +
> +#define TTC_CLK_CNTRL_CSRC BIT(5)
> +#define TTC_CLK_CNTRL_PS_EN BIT(0)
> +#define TTC_CNTR_CTRL_DIS BIT(0)
> +#define TTC_CNTR_CTRL_INTR_MODE_EN BIT(1)
> +#define TTC_CNTR_CTRL_MATCH_MODE_EN BIT(3)
> +#define TTC_CNTR_CTRL_RST BIT(4)
> +#define TTC_CNTR_CTRL_WAVE_EN BIT(5)
> +#define TTC_CNTR_CTRL_WAVE_POL BIT(6)
> +#define TTC_CNTR_CTRL_WAVE_POL_SHIFT 6
> +#define TTC_CNTR_CTRL_PRESCALE_SHIFT 1
> +#define TTC_PWM_MAX_CH 3
> +
> +#if defined(CONFIG_PWM_CADENCE)
> +int ttc_pwm_probe(struct platform_device *pdev);
> +void ttc_pwm_remove(struct platform_device *pdev);
> +#endif
No need for the ifdef.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature