Message-ID: <nwdxbyynffy7kkxdznxrogzqg2r5bje45ywgxa6jj7mcix65ze@dtabj46eut5i>
Date: Mon, 24 Mar 2025 12:29:41 +0100
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Mubin Sayyed <mubin.sayyed@....com>
Cc: krzysztof.kozlowski+dt@...aro.org, daniel.lezcano@...aro.org, 
	tglx@...utronix.de, michal.simek@....com, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org, git@....com
Subject: Re: [PATCH v4 3/3] pwm: pwm-cadence: Add support for TTC PWM

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@....com>
> ---
> 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@....com>");
> +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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists