[LINUX,3/3] pwm: pwm-cadence: Add support for TTC PWM

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.

Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
---
 drivers/pwm/Kconfig       |  11 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-cadence.c | 363 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 375 insertions(+)
 create mode 100644 drivers/pwm/pwm-cadence.c

Hi Mubin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on robh/for-next tip/timers/core linus/master v6.2-rc3 next-20230112]
[cannot apply to xilinx-xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mubin-Sayyed/clocksource-timer-cadence-ttc-Do-not-probe-TTC-device-configured-as-PWM/20230112-151710
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
patch link:    https://lore.kernel.org/r/20230112071526.3035949-4-mubin.sayyed%40amd.com
patch subject: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC PWM
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/c7f230f98c7cac02cea4ecd4237c1707344b99ef
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mubin-Sayyed/clocksource-timer-cadence-ttc-Do-not-probe-TTC-device-configured-as-PWM/20230112-151710
        git checkout c7f230f98c7cac02cea4ecd4237c1707344b99ef
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   m68k-linux-ld: drivers/pwm/pwm-cadence.o: in function `ttc_pwm_apply':
>> pwm-cadence.c:(.text+0x4e8): undefined reference to `__udivdi3'
>> m68k-linux-ld: pwm-cadence.c:(.text+0x53c): undefined reference to `__udivdi3'
   m68k-linux-ld: pwm-cadence.c:(.text+0x592): undefined reference to `__udivdi3'
   m68k-linux-ld: pwm-cadence.c:(.text+0x5fa): undefined reference to `__udivdi3'
   `.exit.text' referenced in section `.data' of sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o

Hello,

On Thu, Jan 12, 2023 at 12:45:26PM +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.
> 
> Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
> ---
>  drivers/pwm/Kconfig       |  11 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-cadence.c | 363 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 375 insertions(+)
>  create mode 100644 drivers/pwm/pwm-cadence.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index dae023d783a2..9e0f1fae4711 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -504,6 +504,17 @@ config PWM_SIFIVE
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sifive.
>  
> +config PWM_CADENCE

Please keep both Kconfig and Makefile sorted alphabetically.

> +	tristate "Cadence PWM support"
> +	depends on OF
> +	depends on COMMON_CLK
> +	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_SL28CPLD
>  	tristate "Kontron sl28cpld PWM support"
>  	depends on MFD_SL28CPLD || COMPILE_TEST
> [...]
> diff --git a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c
> new file mode 100644
> index 000000000000..de981df3ec5a
> --- /dev/null
> +++ b/drivers/pwm/pwm-cadence.c
> @@ -0,0 +1,363 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver to configure cadence TTC timer as PWM
> + * generator
> + *
> + * Copyright (C) 2022, Advanced Micro Devices, Inc.
> + */

Is there a public manual for the hardware? If yes, please mention a link
here.

> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

clk-provider looks wronge here, your device is only a consumer, isn't
it?

> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/of_address.h>
> +
> +#define TTC_CLK_CNTRL_OFFSET		0x00
> +#define TTC_CNT_CNTRL_OFFSET		0x0C
> +#define TTC_MATCH_CNT_VAL_OFFSET	0x30
> +#define TTC_COUNT_VAL_OFFSET		0x18
> +#define TTC_INTR_VAL_OFFSET		0x24
> +#define TTC_ISR_OFFSET			0x54
> +#define TTC_IER_OFFSET			0x60
> +#define TTC_PWM_CHANNEL_OFFSET		0x4
> +
> +#define TTC_CLK_CNTRL_CSRC_MASK		BIT(5)
> +#define TTC_CLK_CNTRL_PSV_MASK		GENMASK(4, 1)
> +
> +#define TTC_CNTR_CTRL_DIS_MASK		BIT(0)
> +#define TTC_CNTR_CTRL_INTR_MODE_EN_MASK	BIT(1)
> +#define TTC_CNTR_CTRL_MATCH_MODE_EN_MASK	BIT(3)
> +#define TTC_CNTR_CTRL_RST_MASK		BIT(4)
> +#define TTC_CNTR_CTRL_WAVE_EN_MASK	BIT(5)
> +#define TTC_CNTR_CTRL_WAVE_POL_MASK	BIT(6)

If you ask me do s/_OFFSET// and s/_MASK// on all these definitions.

> +#define TTC_CLK_CNTRL_PSV_SHIFT		1

This is unused.

> +
> +#define TTC_PWM_MAX_CH			3
> +
> +/**
> + * struct ttc_pwm_priv - Private data for TTC PWM drivers
> + * @chip:	PWM chip structure representing PWM controller
> + * @clk:	TTC input clock
> + * @max:	Maximum value of the counters
> + * @base:	Base address of TTC instance
> + */
> +struct ttc_pwm_priv {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	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,
> +				   struct pwm_device *pwm,
> +				   unsigned long offset)
> +{
> +	unsigned long pwm_ch_offset = offset +
> +				       (TTC_PWM_CHANNEL_OFFSET * pwm->hwpwm);
> +
> +	return ttc_pwm_readl(priv, pwm_ch_offset);
> +}
> +
> +static inline void ttc_pwm_ch_writel(struct ttc_pwm_priv *priv,
> +				     struct pwm_device *pwm,
> +				     unsigned long offset,
> +				     unsigned long val)
> +{
> +	unsigned long pwm_ch_offset = offset +
> +				       (TTC_PWM_CHANNEL_OFFSET * pwm->hwpwm);
> +
> +	ttc_pwm_writel(priv, pwm_ch_offset, val);
> +}
> +
> +static inline struct ttc_pwm_priv *xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct ttc_pwm_priv, 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, TTC_CNT_CNTRL_OFFSET);
> +	ctrl_reg |= (TTC_CNTR_CTRL_INTR_MODE_EN_MASK
> +				 | TTC_CNTR_CTRL_MATCH_MODE_EN_MASK | TTC_CNTR_CTRL_RST_MASK);
> +	ctrl_reg &= (~(TTC_CNTR_CTRL_DIS_MASK | TTC_CNTR_CTRL_WAVE_EN_MASK));

superflous parenthesis.

> +	ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_disable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
> +{
> +	u32 ctrl_reg;
> +
> +	ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> +	ctrl_reg |= TTC_CNTR_CTRL_DIS_MASK;
> +
> +	ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_rev_polarity(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
> +{
> +	u32 ctrl_reg;
> +
> +	ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> +	ctrl_reg ^= TTC_CNTR_CTRL_WAVE_POL_MASK;
> +	ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);

I prefer to have this more explicit like

	ttc_pwm_set_polarity(..., enum pwm_polarity polarity)

> +}
> +
> +static void ttc_pwm_set_counters(struct ttc_pwm_priv *priv,
> +				 struct pwm_device *pwm,
> +				 u32 div,
> +				 u32 period_cycles,
> +				 u32 duty_cycles)
> +{
> +	u32 clk_reg;
> +
> +	/* Set up prescalar */
> +	clk_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CLK_CNTRL_OFFSET);
> +	clk_reg &= ~TTC_CLK_CNTRL_PSV_MASK;
> +	clk_reg |= div;

We have

#define TTC_CLK_CNTRL_PSV_MASK		GENMASK(4, 1)

so the LSB of div sets a bit outside of TTC_CLK_CNTRL_PSV_MASK; that
looks wrong.

> +	ttc_pwm_ch_writel(priv, pwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
> +
> +	/* Set up period */
> +	ttc_pwm_ch_writel(priv, pwm, TTC_INTR_VAL_OFFSET, period_cycles);
> +
> +	/* Set up duty cycle */
> +	ttc_pwm_ch_writel(priv, pwm, TTC_MATCH_CNT_VAL_OFFSET, duty_cycles);

how does the output pin behave between the writes in this function (and
the others in .apply())?

> +}
> +
> +static int ttc_pwm_apply(struct pwm_chip *chip,
> +			 struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	u32 div = 0;
> +	u64 period_cycles;
> +	u64 duty_cycles;
> +	unsigned long rate;
> +	struct pwm_state cstate;
> +	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +
> +	pwm_get_state(pwm, &cstate);

Please don't use pwm consumer functions. You can use pwm->state
directly.

> +	if (state->polarity != cstate.polarity) {
> +		if (cstate.enabled)
> +			ttc_pwm_disable(priv, pwm);
> +
> +		ttc_pwm_rev_polarity(priv, pwm);
> +
> +		if (cstate.enabled)
> +			ttc_pwm_enable(priv, pwm);

It would (probably) be nice to delay that enable until you configured
duty_cycle and period to reduce the amount of wrong signalling.

> +	}
> +

You can exit early here if state->enabled == false.

> +	if (state->period != cstate.period ||
> +	    state->duty_cycle != cstate.duty_cycle) {

(With an early exit above this check becomes wrong, simply drop it.)

> +		rate = clk_get_rate(priv->clk);
> +
> +		/* Prevent overflow by limiting to the maximum possible period */
> +		period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> +		period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate, NSEC_PER_SEC);

DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
reason for the build bot report.)

The usual alternative trick here is to do:

	if (rate > NSEC_PER_SEC)
		return -EINVAL;

	period_cycles = mul_u64_u64_div_u64(state->period, rate, NSEC_PER_SEC);

> +		if (period_cycles > priv->max) {
> +			/* prescale frequency to fit requested period cycles within limit */
> +			div = 1;
> +
> +			while ((period_cycles  > priv->max) && (div < 65536)) {

s/  / /

Also this can be calculated without a loop. Maybe it's easier to store
the timer width in priv instead of max = BIT(width) - 1.

> +				rate = DIV_ROUND_CLOSEST(rate, BIT(div));
> +				period_cycles = DIV_ROUND_CLOSEST(state->period * rate,
> +								  NSEC_PER_SEC);
> +				if (period_cycles < priv->max)
> +					break;
> +				div++;
> +			}
> +
> +			if (period_cycles  > priv->max)
> +				return -ERANGE;
> +		}
> +
> +		duty_cycles = DIV_ROUND_CLOSEST(state->duty_cycle * rate,
> +						NSEC_PER_SEC);

Please ensure that you configure a period that is not bigger than
state->period. Ditto for duty_cycle.

> +		if (cstate.enabled)
> +			ttc_pwm_disable(priv, pwm);
> +
> +		ttc_pwm_set_counters(priv, pwm, div, period_cycles, duty_cycles);
> +
> +		if (cstate.enabled)
> +			ttc_pwm_enable(priv, pwm);

Another possible glitch here.

> +	}
> +
> +	if (state->enabled != cstate.enabled) {
> +		if (state->enabled)
> +			ttc_pwm_enable(priv, pwm);
> +		else
> +			ttc_pwm_disable(priv, pwm);
> +	}
> +
> +	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);
> +	unsigned long rate;
> +	u32 value;
> +	u64 tmp;
> +
> +	value = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> +
> +	if (value & TTC_CNTR_CTRL_WAVE_POL_MASK)
> +		state->polarity = PWM_POLARITY_INVERSED;
> +	else
> +		state->polarity = PWM_POLARITY_NORMAL;
> +
> +	if (value & TTC_CNTR_CTRL_DIS_MASK)
> +		state->enabled = false;
> +	else
> +		state->enabled = true;
> +
> +	rate = clk_get_rate(priv->clk);
> +
> +	tmp = ttc_pwm_ch_readl(priv, pwm, TTC_INTR_VAL_OFFSET);
> +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> +	tmp = ttc_pwm_ch_readl(priv, pwm, TTC_MATCH_CNT_VAL_OFFSET);
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);

This looks wrong for several reasons. Have you tested with PWM_DEBUG
enabled? What is rate typically?

.get_state is supposed to round up in its divisions. Also I miss a
factor of NSEC_PER_SEC here, and also div doesn't appear here?!

> +	return 0;
> +}
> +
> +static struct pwm_device *
> +ttc_pwm_of_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
> +{
> +	struct pwm_device *pwm;
> +
> +	if (args->args[0] >= TTC_PWM_MAX_CH)
> +		return NULL;
> +
> +	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> +	if (IS_ERR(pwm))
> +		return pwm;
> +
> +	if (args->args[1])
> +		pwm->args.period = args->args[1];
> +
> +	if (args->args[2])
> +		pwm->args.polarity = args->args[2];
> +
> +	return pwm;
> +}

I don't see an advantage in this function over the default
of_pwm_xlate_with_flags(). (There are downsides though I think.)

If you drop this function there is only one usage of TTC_PWM_MAX_CH left
and you can drop this symbol, too. (In favour of

	priv->chip.npwm = 3;

.)

> +static const struct pwm_ops ttc_pwm_ops = {
> +	.apply = ttc_pwm_apply,
> +	.get_state = ttc_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int ttc_pwm_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	int clksel;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct ttc_pwm_priv *priv;
> +	u32 pwm_cells;
> +	u32 timer_width;
> +	struct clk *clk_cs;
> +

Please add a comment here about coexistence with the timer mode.

> +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> +	if (ret == -EINVAL)
> +		return -ENODEV;
> +
> +	if (ret)
> +		return dev_err_probe(dev, ret, "could not read #pwm-cells\n");
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	ret = of_property_read_u32(np, "timer-width", &timer_width);
> +	if (ret)
> +		timer_width = 16;
> +
> +	priv->max = BIT(timer_width) - 1;
> +
> +	clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);

TTC_CLK_CNTRL_OFFSET is as parameter for ttc_pwm_ch_readl and
ttc_pwm_readl, is this correct?

> +	clksel = !!(clksel & TTC_CLK_CNTRL_CSRC_MASK);
> +	clk_cs = of_clk_get(np, clksel);

Can you use devm_clk_get_enabled here?

> +	if (IS_ERR(clk_cs))
> +		return dev_err_probe(dev, PTR_ERR(clk_cs),
> +				     "ERROR: timer input clock not found\n");
> +
> +	priv->clk = clk_cs;
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Clock enable failed\n");
> +
> +	clk_rate_exclusive_get(priv->clk);
> +
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &ttc_pwm_ops;
> +	priv->chip.npwm = TTC_PWM_MAX_CH;
> +	priv->chip.of_xlate = ttc_pwm_of_xlate;
> +	ret = pwmchip_add(&priv->chip);
> +	if (ret) {
> +		clk_rate_exclusive_put(priv->clk);
> +		clk_disable_unprepare(priv->clk);
> +		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}
> +
> +static int 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);
> +	clk_disable_unprepare(priv->clk);

missing clk_put() (unless you switch to a devm_clk_get variant as
suggested above).

> +
> +	return 0;
> +}

Best regards
Uwe

On 12/01/2023 08:15, 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.
> 

(...)

> +
> +static const struct of_device_id ttc_pwm_of_match[] = {
> +	{ .compatible = "cdns,ttc"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ttc_pwm_of_match);
> +
> +static struct platform_driver ttc_pwm_driver = {
> +	.probe = ttc_pwm_probe,
> +	.remove = ttc_pwm_remove,
> +	.driver = {
> +		.name = "ttc-pwm",
> +		.of_match_table = of_match_ptr(ttc_pwm_of_match),

This leads to warnings. Drop it or use maybe_unused.

Best regards,
Krzysztof

Hi Uwe,

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Friday, January 13, 2023 2:55 AM
> To: Sayyed, Mubin <mubin.sayyed@amd.com>
> Cc: robh+dt@kernel.org; treding@nvidia.com; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>; Simek, Michal
> <michal.simek@amd.com>; Paladugu, Siva Durga Prasad
> <siva.durga.prasad.paladugu@amd.com>; mubin10@gmail.com
> Subject: Re: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC
> PWM
> 
> Hello,
> 
> On Thu, Jan 12, 2023 at 12:45:26PM +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.
> >
> > Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
> > ---
> >  drivers/pwm/Kconfig       |  11 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-cadence.c | 363
> > ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 375 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-cadence.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > dae023d783a2..9e0f1fae4711 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -504,6 +504,17 @@ config PWM_SIFIVE
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-sifive.
> >
> > +config PWM_CADENCE
> 
> Please keep both Kconfig and Makefile sorted alphabetically.
[Mubin]: OK
> 
> > +	tristate "Cadence PWM support"
> > +	depends on OF
> > +	depends on COMMON_CLK
> > +	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_SL28CPLD
> >  	tristate "Kontron sl28cpld PWM support"
> >  	depends on MFD_SL28CPLD || COMPILE_TEST [...] diff --git
> > a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c new file
> mode
> > 100644 index 000000000000..de981df3ec5a
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-cadence.c
> > @@ -0,0 +1,363 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver to configure cadence TTC timer as PWM
> > + * generator
> > + *
> > + * Copyright (C) 2022, Advanced Micro Devices, Inc.
> > + */
> 
> Is there a public manual for the hardware? If yes, please mention a link here.
[Mubin]: I did not find any public manual from cadence. However, details can be found in Zynq-7000/ Zynq UltraScale/Versal ACAP TRM available publicly.
> 
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> 
> clk-provider looks wronge here, your device is only a consumer, isn't it?
[Mubin]: will remove it
> 
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/of_address.h>
> > +
> > +#define TTC_CLK_CNTRL_OFFSET		0x00
> > +#define TTC_CNT_CNTRL_OFFSET		0x0C
> > +#define TTC_MATCH_CNT_VAL_OFFSET	0x30
> > +#define TTC_COUNT_VAL_OFFSET		0x18
> > +#define TTC_INTR_VAL_OFFSET		0x24
> > +#define TTC_ISR_OFFSET			0x54
> > +#define TTC_IER_OFFSET			0x60
> > +#define TTC_PWM_CHANNEL_OFFSET		0x4
> > +
> > +#define TTC_CLK_CNTRL_CSRC_MASK		BIT(5)
> > +#define TTC_CLK_CNTRL_PSV_MASK		GENMASK(4, 1)
> > +
> > +#define TTC_CNTR_CTRL_DIS_MASK		BIT(0)
> > +#define TTC_CNTR_CTRL_INTR_MODE_EN_MASK	BIT(1)
> > +#define TTC_CNTR_CTRL_MATCH_MODE_EN_MASK	BIT(3)
> > +#define TTC_CNTR_CTRL_RST_MASK		BIT(4)
> > +#define TTC_CNTR_CTRL_WAVE_EN_MASK	BIT(5)
> > +#define TTC_CNTR_CTRL_WAVE_POL_MASK	BIT(6)
> 
> If you ask me do s/_OFFSET// and s/_MASK// on all these definitions.
[Mubin]: will update them
> 
> > +#define TTC_CLK_CNTRL_PSV_SHIFT		1
> 
> This is unused.
[Mubin]: will remove it .
> > +
> > +#define TTC_PWM_MAX_CH			3
> > +
> > +/**
> > + * struct ttc_pwm_priv - Private data for TTC PWM drivers
> > + * @chip:	PWM chip structure representing PWM controller
> > + * @clk:	TTC input clock
> > + * @max:	Maximum value of the counters
> > + * @base:	Base address of TTC instance
> > + */
> > +struct ttc_pwm_priv {
> > +	struct pwm_chip chip;
> > +	struct clk *clk;
> > +	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,
> > +				   struct pwm_device *pwm,
> > +				   unsigned long offset)
> > +{
> > +	unsigned long pwm_ch_offset = offset +
> > +				       (TTC_PWM_CHANNEL_OFFSET * pwm-
> >hwpwm);
> > +
> > +	return ttc_pwm_readl(priv, pwm_ch_offset); }
> > +
> > +static inline void ttc_pwm_ch_writel(struct ttc_pwm_priv *priv,
> > +				     struct pwm_device *pwm,
> > +				     unsigned long offset,
> > +				     unsigned long val)
> > +{
> > +	unsigned long pwm_ch_offset = offset +
> > +				       (TTC_PWM_CHANNEL_OFFSET * pwm-
> >hwpwm);
> > +
> > +	ttc_pwm_writel(priv, pwm_ch_offset, val); }
> > +
> > +static inline struct ttc_pwm_priv *xilinx_pwm_chip_to_priv(struct
> > +pwm_chip *chip) {
> > +	return container_of(chip, struct ttc_pwm_priv, 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, TTC_CNT_CNTRL_OFFSET);
> > +	ctrl_reg |= (TTC_CNTR_CTRL_INTR_MODE_EN_MASK
> > +				 |
> TTC_CNTR_CTRL_MATCH_MODE_EN_MASK | TTC_CNTR_CTRL_RST_MASK);
> > +	ctrl_reg &= (~(TTC_CNTR_CTRL_DIS_MASK |
> > +TTC_CNTR_CTRL_WAVE_EN_MASK));
> 
> superflous parenthesis.
[Mubin]: will remove it in v2
> 
> > +	ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg); }
> > +
> > +static void ttc_pwm_disable(struct ttc_pwm_priv *priv, struct
> > +pwm_device *pwm) {
> > +	u32 ctrl_reg;
> > +
> > +	ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> > +	ctrl_reg |= TTC_CNTR_CTRL_DIS_MASK;
> > +
> > +	ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg); }
> > +
> > +static void ttc_pwm_rev_polarity(struct ttc_pwm_priv *priv, struct
> > +pwm_device *pwm) {
> > +	u32 ctrl_reg;
> > +
> > +	ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> > +	ctrl_reg ^= TTC_CNTR_CTRL_WAVE_POL_MASK;
> > +	ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> 
> I prefer to have this more explicit like
> 
> 	ttc_pwm_set_polarity(..., enum pwm_polarity polarity)
[Mubin]: Agreed
> 
> > +}
> > +
> > +static void ttc_pwm_set_counters(struct ttc_pwm_priv *priv,
> > +				 struct pwm_device *pwm,
> > +				 u32 div,
> > +				 u32 period_cycles,
> > +				 u32 duty_cycles)
> > +{
> > +	u32 clk_reg;
> > +
> > +	/* Set up prescalar */
> > +	clk_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CLK_CNTRL_OFFSET);
> > +	clk_reg &= ~TTC_CLK_CNTRL_PSV_MASK;
> > +	clk_reg |= div;
> 
> We have
> 
> #define TTC_CLK_CNTRL_PSV_MASK		GENMASK(4, 1)
> 
> so the LSB of div sets a bit outside of TTC_CLK_CNTRL_PSV_MASK; that looks
> wrong.
[Mubin]: will fix it in v2
> 
> > +	ttc_pwm_ch_writel(priv, pwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
> > +
> > +	/* Set up period */
> > +	ttc_pwm_ch_writel(priv, pwm, TTC_INTR_VAL_OFFSET,
> period_cycles);
> > +
> > +	/* Set up duty cycle */
> > +	ttc_pwm_ch_writel(priv, pwm, TTC_MATCH_CNT_VAL_OFFSET,
> duty_cycles);
> 
> how does the output pin behave between the writes in this function (and the
> others in .apply())?
[Mubin]:  ttc_pwm_apply  is disabling counters before calling this function, and enabling it back immediately after it.  So, output pin would be low during configuration. 
> 
> > +}
> > +
> > +static int ttc_pwm_apply(struct pwm_chip *chip,
> > +			 struct pwm_device *pwm,
> > +			 const struct pwm_state *state)
> > +{
> > +	u32 div = 0;
> > +	u64 period_cycles;
> > +	u64 duty_cycles;
> > +	unsigned long rate;
> > +	struct pwm_state cstate;
> > +	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> > +
> > +	pwm_get_state(pwm, &cstate);
> 
> Please don't use pwm consumer functions. You can use pwm->state directly.
[Mubin]: will fix in v2
> 
> > +	if (state->polarity != cstate.polarity) {
> > +		if (cstate.enabled)
> > +			ttc_pwm_disable(priv, pwm);
> > +
> > +		ttc_pwm_rev_polarity(priv, pwm);
> > +
> > +		if (cstate.enabled)
> > +			ttc_pwm_enable(priv, pwm);
> 
> It would (probably) be nice to delay that enable until you configured
> duty_cycle and period to reduce the amount of wrong signalling.
[Mubin]: Agreed
> 
> > +	}
> > +
> 
> You can exit early here if state->enabled == false.
> 
> > +	if (state->period != cstate.period ||
> > +	    state->duty_cycle != cstate.duty_cycle) {
> 
> (With an early exit above this check becomes wrong, simply drop it.)
[Mubin]: OK
> 
> > +		rate = clk_get_rate(priv->clk);
> > +
> > +		/* Prevent overflow by limiting to the maximum possible
> period */
> > +		period_cycles = min_t(u64, state->period, ULONG_MAX *
> NSEC_PER_SEC);
> > +		period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate,
> > +NSEC_PER_SEC);
> 
> DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
> reason for the build bot report.)
> 
> The usual alternative trick here is to do:
> 
> 	if (rate > NSEC_PER_SEC)
> 		return -EINVAL;
> 
> 	period_cycles = mul_u64_u64_div_u64(state->period, rate,
> NSEC_PER_SEC);
[Mubin]: Initially I tried mul_u64_u64_div_u64, it was impacting accuracy while generating PWM signal of high frequency(output frequency above 5 MHZ,  input 100 MHZ ). Usage of DIV_ROUND_CLOSEST improved accuracy. Can you please suggest better alternative for improving accuracy.
> 
> > +		if (period_cycles > priv->max) {
> > +			/* prescale frequency to fit requested period cycles
> within limit */
> > +			div = 1;
> > +
> > +			while ((period_cycles  > priv->max) && (div < 65536))
> {
> 
> s/  / /
[Mubin]: OK
> 
> Also this can be calculated without a loop. Maybe it's easier to store the
> timer width in priv instead of max = BIT(width) - 1.
> 
> > +				rate = DIV_ROUND_CLOSEST(rate, BIT(div));
> > +				period_cycles = DIV_ROUND_CLOSEST(state-
> >period * rate,
> > +
> NSEC_PER_SEC);
[Mubin]: Agreed, will implement that logic in v2.
> > +				if (period_cycles < priv->max)
> > +					break;
> > +				div++;
> > +			}
> > +
> > +			if (period_cycles  > priv->max)
> > +				return -ERANGE;
> > +		}
> > +
> > +		duty_cycles = DIV_ROUND_CLOSEST(state->duty_cycle * rate,
> > +						NSEC_PER_SEC);
> 
> Please ensure that you configure a period that is not bigger than
> state->period. Ditto for duty_cycle.
[Mubin]: OK
> 
> > +		if (cstate.enabled)
> > +			ttc_pwm_disable(priv, pwm);
> > +
> > +		ttc_pwm_set_counters(priv, pwm, div, period_cycles,
> duty_cycles);
> > +
> > +		if (cstate.enabled)
> > +			ttc_pwm_enable(priv, pwm);
> 
> Another possible glitch here.
[Mubin]: Can you please elaborate.
> 
> > +	}
> > +
> > +	if (state->enabled != cstate.enabled) {
> > +		if (state->enabled)
> > +			ttc_pwm_enable(priv, pwm);
> > +		else
> > +			ttc_pwm_disable(priv, pwm);
> > +	}
> > +
> > +	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);
> > +	unsigned long rate;
> > +	u32 value;
> > +	u64 tmp;
> > +
> > +	value = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> > +
> > +	if (value & TTC_CNTR_CTRL_WAVE_POL_MASK)
> > +		state->polarity = PWM_POLARITY_INVERSED;
> > +	else
> > +		state->polarity = PWM_POLARITY_NORMAL;
> > +
> > +	if (value & TTC_CNTR_CTRL_DIS_MASK)
> > +		state->enabled = false;
> > +	else
> > +		state->enabled = true;
> > +
> > +	rate = clk_get_rate(priv->clk);
> > +
> > +	tmp = ttc_pwm_ch_readl(priv, pwm, TTC_INTR_VAL_OFFSET);
> > +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > +	tmp = ttc_pwm_ch_readl(priv, pwm,
> TTC_MATCH_CNT_VAL_OFFSET);
> > +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> 
> This looks wrong for several reasons. Have you tested with PWM_DEBUG
> enabled? What is rate typically?
> 
> .get_state is supposed to round up in its divisions. Also I miss a factor of
> NSEC_PER_SEC here, and also div doesn't appear here?!
[Mubin]: Did not tested it with PWM_DEBUG. Typical rate is 100 MHZ. I will fix it in v2.
> 
> > +	return 0;
> > +}
> > +
> > +static struct pwm_device *
> > +ttc_pwm_of_xlate(struct pwm_chip *chip, const struct of_phandle_args
> > +*args) {
> > +	struct pwm_device *pwm;
> > +
> > +	if (args->args[0] >= TTC_PWM_MAX_CH)
> > +		return NULL;
> > +
> > +	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> > +	if (IS_ERR(pwm))
> > +		return pwm;
> > +
> > +	if (args->args[1])
> > +		pwm->args.period = args->args[1];
> > +
> > +	if (args->args[2])
> > +		pwm->args.polarity = args->args[2];
> > +
> > +	return pwm;
> > +}
> 
> I don't see an advantage in this function over the default
> of_pwm_xlate_with_flags(). (There are downsides though I think.)
> 
> If you drop this function there is only one usage of TTC_PWM_MAX_CH left
> and you can drop this symbol, too. (In favour of
> 
> 	priv->chip.npwm = 3;
> 
> .)
[Mubin]: Agreed
> 
> > +static const struct pwm_ops ttc_pwm_ops = {
> > +	.apply = ttc_pwm_apply,
> > +	.get_state = ttc_pwm_get_state,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int ttc_pwm_probe(struct platform_device *pdev) {
> > +	int ret;
> > +	int clksel;
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct ttc_pwm_priv *priv;
> > +	u32 pwm_cells;
> > +	u32 timer_width;
> > +	struct clk *clk_cs;
> > +
> 
> Please add a comment here about coexistence with the timer mode.
[Mubin]: OK
> 
> > +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> > +	if (ret == -EINVAL)
> > +		return -ENODEV;
> > +
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "could not read #pwm-
> cells\n");
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	ret = of_property_read_u32(np, "timer-width", &timer_width);
> > +	if (ret)
> > +		timer_width = 16;
> > +
> > +	priv->max = BIT(timer_width) - 1;
> > +
> > +	clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);
> 
> TTC_CLK_CNTRL_OFFSET is as parameter for ttc_pwm_ch_readl and
> ttc_pwm_readl, is this correct?
[Mubin]: Here TTC_CLK_CNTRL_OFFSET is being read only for 0th channel, so using ttc_pwm_readl does not impact offsets.
> 
> > +	clksel = !!(clksel & TTC_CLK_CNTRL_CSRC_MASK);
> > +	clk_cs = of_clk_get(np, clksel);
> 
> Can you use devm_clk_get_enabled here?
[Mubin]: Yes, will switch to devm_clk_get_enabled  in v2.
> 
> > +	if (IS_ERR(clk_cs))
> > +		return dev_err_probe(dev, PTR_ERR(clk_cs),
> > +				     "ERROR: timer input clock not found\n");
> > +
> > +	priv->clk = clk_cs;
> > +	ret = clk_prepare_enable(priv->clk);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Clock enable failed\n");
> > +
> > +	clk_rate_exclusive_get(priv->clk);
> > +
> > +	priv->chip.dev = dev;
> > +	priv->chip.ops = &ttc_pwm_ops;
> > +	priv->chip.npwm = TTC_PWM_MAX_CH;
> > +	priv->chip.of_xlate = ttc_pwm_of_xlate;
> > +	ret = pwmchip_add(&priv->chip);
> > +	if (ret) {
> > +		clk_rate_exclusive_put(priv->clk);
> > +		clk_disable_unprepare(priv->clk);
> > +		return dev_err_probe(dev, ret, "Could not register PWM
> chip\n");
> > +	}
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	return 0;
> > +}
> > +
> > +static int 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);
> > +	clk_disable_unprepare(priv->clk);
> 
> missing clk_put() (unless you switch to a devm_clk_get variant as suggested
> above).
[Mubin] will be switching to devm_clk_get in next version.

Thanks,
Mubin
> 
> > +
> > +	return 0;
> > +}
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Hello Mubin,

On Tue, Jan 17, 2023 at 09:58:10AM +0000, Sayyed, Mubin wrote:
> > On Thu, Jan 12, 2023 at 12:45:26PM +0530, Mubin Sayyed wrote:
> > Is there a public manual for the hardware? If yes, please mention a link here.
> [Mubin]: I did not find any public manual from cadence. However, details can be found in Zynq-7000/ Zynq UltraScale/Versal ACAP TRM available publicly.

Thenk please add a link to that one.

> > how does the output pin behave between the writes in this function (and the
> > others in .apply())?
> [Mubin]:  ttc_pwm_apply  is disabling counters before calling this
> function, and enabling it back immediately after it.  So, output pin
> would be low during configuration. 

Please document this behaviour (i.e. "A disabled PWM emits a zero") in a
paragraph at the top of the driver in the format that e.g.
drivers/pwm/pwm-pxa.c is using. Also please test if it emits a zero or
the inactive level, i.e. if the output depends on the polarity setting.

> > > +		rate = clk_get_rate(priv->clk);
> > > +
> > > +		/* Prevent overflow by limiting to the maximum possible period */
> > > +		period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> > > +		period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate, NSEC_PER_SEC);
> > 
> > DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
> > reason for the build bot report.)
> > 
> > The usual alternative trick here is to do:
> > 
> > 	if (rate > NSEC_PER_SEC)
> > 		return -EINVAL;
> > 
> > 	period_cycles = mul_u64_u64_div_u64(state->period, rate,
> > NSEC_PER_SEC);
> [Mubin]: Initially I tried mul_u64_u64_div_u64, it was impacting
> accuracy while generating PWM signal of high frequency(output
> frequency above 5 MHZ,  input 100 MHZ ). Usage of DIV_ROUND_CLOSEST
> improved accuracy. Can you please suggest better alternative for
> improving accuracy.

Unless I'm missing something, you have to adjust your definition of
accuracy :-)

If you request (say) a period of 149 ns and the nearest actual values
your hardware can provide left and right of that is 140 ns and 150 ns,
140ns is the one to select. That is pick the biggest possible period not
bigger than the requested period. And with that pick the biggest
possible duty_cycle not bigger than the requested duty_cycle. (i.e. it's
a bug to emit period=150ns if 149 was requested.)

There are ideas to implement something like

	pwm_apply_nearest(...);

but that's still in the idea stage (and will depend on the lowlevel
drivers implementing the strategy described above).

> > Another possible glitch here.
> [Mubin]: Can you please elaborate.

If you switch polarity (say from normal to inverted) and duty/period you do (simplified)

	ttc_pwm_disable(priv, pwm); // might yield a falling edge
	set polarity                // might yield a raising edge
	ttc_pwm_enable(priv, pwm);  // might yield a falling edge
	... some calculations
	ttc_pwm_disable(priv, pwm); // might yield a raising edge
	setup counters
	ttc_pwm_enable(priv, pwm);  // might yield a falling edge

so during apply() the output might toggle several times at a high(?)
frequency. Depending on what you have connected to the PWM this is bad.
(A LED might blink, a backlight might flicker, a motor might stutter and
enter an error state or accelerate for a moment.)

> > > +	clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);
> > 
> > TTC_CLK_CNTRL_OFFSET is as parameter for ttc_pwm_ch_readl and
> > ttc_pwm_readl, is this correct?
> [Mubin]: Here TTC_CLK_CNTRL_OFFSET is being read only for 0th channel, so using ttc_pwm_readl does not impact offsets.

So which clk is selected (for all channels?) depends on how channel 0's
clock setting is setup by the bootloader (or bootrom)? Sounds strange.

Best regards
Uwe

Hi Uwe,

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Tuesday, January 17, 2023 4:57 PM
> To: Sayyed, Mubin <mubin.sayyed@amd.com>
> Cc: robh+dt@kernel.org; treding@nvidia.com; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>; Simek, Michal
> <michal.simek@amd.com>; Paladugu, Siva Durga Prasad
> <siva.durga.prasad.paladugu@amd.com>; mubin10@gmail.com;
> krzk@kernel.org
> Subject: Re: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC
> PWM
> 
> Hello Mubin,
> 
> On Tue, Jan 17, 2023 at 09:58:10AM +0000, Sayyed, Mubin wrote:
> > > On Thu, Jan 12, 2023 at 12:45:26PM +0530, Mubin Sayyed wrote:
> > > Is there a public manual for the hardware? If yes, please mention a link
> here.
> > [Mubin]: I did not find any public manual from cadence. However, details
> can be found in Zynq-7000/ Zynq UltraScale/Versal ACAP TRM available
> publicly.
> 
> Thenk please add a link to that one.
> 
> > > how does the output pin behave between the writes in this function
> > > (and the others in .apply())?
> > [Mubin]:  ttc_pwm_apply  is disabling counters before calling this
> > function, and enabling it back immediately after it.  So, output pin
> > would be low during configuration.
> 
> Please document this behaviour (i.e. "A disabled PWM emits a zero") in a
> paragraph at the top of the driver in the format that e.g.
> drivers/pwm/pwm-pxa.c is using. Also please test if it emits a zero or the
> inactive level, i.e. if the output depends on the polarity setting.

[Mubin]: will confirm behavior on hardware and document it.
> 
> > > > +		rate = clk_get_rate(priv->clk);
> > > > +
> > > > +		/* Prevent overflow by limiting to the maximum possible
> period */
> > > > +		period_cycles = min_t(u64, state->period, ULONG_MAX *
> NSEC_PER_SEC);
> > > > +		period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate,
> > > > +NSEC_PER_SEC);
> > >
> > > DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
> > > reason for the build bot report.)
> > >
> > > The usual alternative trick here is to do:
> > >
> > > 	if (rate > NSEC_PER_SEC)
> > > 		return -EINVAL;
> > >
> > > 	period_cycles = mul_u64_u64_div_u64(state->period, rate,
> > > NSEC_PER_SEC);
> > [Mubin]: Initially I tried mul_u64_u64_div_u64, it was impacting
> > accuracy while generating PWM signal of high frequency(output
> > frequency above 5 MHZ,  input 100 MHZ ). Usage of DIV_ROUND_CLOSEST
> > improved accuracy. Can you please suggest better alternative for
> > improving accuracy.
> 
> Unless I'm missing something, you have to adjust your definition of accuracy
> :-)
> 
> If you request (say) a period of 149 ns and the nearest actual values your
> hardware can provide left and right of that is 140 ns and 150 ns, 140ns is the
> one to select. That is pick the biggest possible period not bigger than the
> requested period. And with that pick the biggest possible duty_cycle not
> bigger than the requested duty_cycle. (i.e. it's a bug to emit period=150ns if
> 149 was requested.)
> 
> There are ideas to implement something like
> 
> 	pwm_apply_nearest(...);
> 
> but that's still in the idea stage (and will depend on the lowlevel drivers
> implementing the strategy described above).
[Mubin]: Thanks for explaining, will switch to mul_u64_u64_div_u64, though percentage of deviation would be more for PWM signal of high frequency. For example, requested period is 50 ns,  requested duty cycle is 49 ns(98%), actual duty cycle set by driver would be 40ns (80%).

> 
> > > Another possible glitch here.
> > [Mubin]: Can you please elaborate.
> 
> If you switch polarity (say from normal to inverted) and duty/period you do
> (simplified)
> 
> 	ttc_pwm_disable(priv, pwm); // might yield a falling edge
> 	set polarity                // might yield a raising edge
> 	ttc_pwm_enable(priv, pwm);  // might yield a falling edge
> 	... some calculations
> 	ttc_pwm_disable(priv, pwm); // might yield a raising edge
> 	setup counters
> 	ttc_pwm_enable(priv, pwm);  // might yield a falling edge
> 
> so during apply() the output might toggle several times at a high(?)
> frequency. Depending on what you have connected to the PWM this is bad.
> (A LED might blink, a backlight might flicker, a motor might stutter and enter
> an error state or accelerate for a moment.)

[Mubin]: Agreed, will modify logic to avoid toggling
> 
> > > > +	clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);
> > >
> > > TTC_CLK_CNTRL_OFFSET is as parameter for ttc_pwm_ch_readl and
> > > ttc_pwm_readl, is this correct?
> > [Mubin]: Here TTC_CLK_CNTRL_OFFSET is being read only for 0th channel,
> so using ttc_pwm_readl does not impact offsets.
> 
> So which clk is selected (for all channels?) depends on how channel 0's clock
> setting is setup by the bootloader (or bootrom)? Sounds strange.
[Mubin]: I confirmed that each channel can have separate clk source. I will update it for all channels and modify ttc_pwm_priv structure accordingly.

Thanks,
Mubin
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

On Tue, Jan 17, 2023 at 12:55:06PM +0000, Sayyed, Mubin wrote:
> Hi Uwe,
> 
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: Tuesday, January 17, 2023 4:57 PM
> > To: Sayyed, Mubin <mubin.sayyed@amd.com>
> > Cc: robh+dt@kernel.org; treding@nvidia.com; linux-pwm@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>; Simek, Michal
> > <michal.simek@amd.com>; Paladugu, Siva Durga Prasad
> > <siva.durga.prasad.paladugu@amd.com>; mubin10@gmail.com;
> > krzk@kernel.org
> > Subject: Re: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC
> > PWM
> > 
> > Hello Mubin,
> > 
> > On Tue, Jan 17, 2023 at 09:58:10AM +0000, Sayyed, Mubin wrote:
> > > > On Thu, Jan 12, 2023 at 12:45:26PM +0530, Mubin Sayyed wrote:
> > > > Is there a public manual for the hardware? If yes, please mention a link
> > here.
> > > [Mubin]: I did not find any public manual from cadence. However, details
> > can be found in Zynq-7000/ Zynq UltraScale/Versal ACAP TRM available
> > publicly.
> > 
> > Thenk please add a link to that one.
> > 
> > > > how does the output pin behave between the writes in this function
> > > > (and the others in .apply())?
> > > [Mubin]:  ttc_pwm_apply  is disabling counters before calling this
> > > function, and enabling it back immediately after it.  So, output pin
> > > would be low during configuration.
> > 
> > Please document this behaviour (i.e. "A disabled PWM emits a zero") in a
> > paragraph at the top of the driver in the format that e.g.
> > drivers/pwm/pwm-pxa.c is using. Also please test if it emits a zero or the
> > inactive level, i.e. if the output depends on the polarity setting.
> 
> [Mubin]: will confirm behavior on hardware and document it.
> > 
> > > > > +		rate = clk_get_rate(priv->clk);
> > > > > +
> > > > > +		/* Prevent overflow by limiting to the maximum possible
> > period */
> > > > > +		period_cycles = min_t(u64, state->period, ULONG_MAX *
> > NSEC_PER_SEC);
> > > > > +		period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate,
> > > > > +NSEC_PER_SEC);
> > > >
> > > > DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
> > > > reason for the build bot report.)
> > > >
> > > > The usual alternative trick here is to do:
> > > >
> > > > 	if (rate > NSEC_PER_SEC)
> > > > 		return -EINVAL;
> > > >
> > > > 	period_cycles = mul_u64_u64_div_u64(state->period, rate,
> > > > NSEC_PER_SEC);
> > > [Mubin]: Initially I tried mul_u64_u64_div_u64, it was impacting
> > > accuracy while generating PWM signal of high frequency(output
> > > frequency above 5 MHZ,  input 100 MHZ ). Usage of DIV_ROUND_CLOSEST
> > > improved accuracy. Can you please suggest better alternative for
> > > improving accuracy.
> > 
> > Unless I'm missing something, you have to adjust your definition of accuracy
> > :-)
> > 
> > If you request (say) a period of 149 ns and the nearest actual values your
> > hardware can provide left and right of that is 140 ns and 150 ns, 140ns is the
> > one to select. That is pick the biggest possible period not bigger than the
> > requested period. And with that pick the biggest possible duty_cycle not
> > bigger than the requested duty_cycle. (i.e. it's a bug to emit period=150ns if
> > 149 was requested.)
> > 
> > There are ideas to implement something like
> > 
> > 	pwm_apply_nearest(...);
> > 
> > but that's still in the idea stage (and will depend on the lowlevel drivers
> > implementing the strategy described above).
> [Mubin]: Thanks for explaining, will switch to mul_u64_u64_div_u64,
> though percentage of deviation would be more for PWM signal of high
> frequency. For example, requested period is 50 ns,  requested duty
> cycle is 49 ns(98%), actual duty cycle set by driver would be 40ns
> (80%).

Note it's a trade-off to let drivers round down and not nearest. The
motivating reasons are:

 - division rounding down is the default in C, so it's a tad easier
 - there are less corner cases
   (There are two strange effects that I'm aware of:
    - Consider for example a hardware that can implement periods of
      length 49.2 ns, 49.4 ns and 50.7 ns (and nothing in-between). If
      you request 50 ns, you get 49.4. .get_state would then tell you
      that the hardware has configured 49 ns. But if you request 49
      ns, you don't get 49.4 ns.
    - Rounding to the nearest time is different to rounding to the
      nearest frequency:
      Consider a hardware than can configure a period of 200 ns (freq:
      5 MHz) and 300 ns (freq: 3.33333 MHz). If you request 249 ns
      (freq: 4.016 MHz) you'd get 200 ns if you round to the nearest
      time, but 300 if you round to the nearest frequency.
   Both problems don't happen with rounding down.)
 - .get_state which is supposed to be the inverse of .apply() needs one
   more distinction of cases.
   (That is, if the nearest integer above or below the actual value
   should be returned. And additionally another arbitraty choice what to
   return for 50.5 ns)

Best regards
Uwe