Re: [LINUX PATCH v2 3/3] pwm: pwm-cadence: Add support for TTC PWM

[Uwe Kleine-König]

Hello,

On Tue, Nov 14, 2023 at 06:17:48PM +0530, Mubin Sayyed wrote:
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8ebcddf91f7b..7fd493f06496 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -152,6 +152,17 @@ config PWM_BRCMSTB
>  	  To compile this driver as a module, choose M Here: the module
>  	  will be called pwm-brcmstb.c.
>  
> +config PWM_CADENCE
> +        tristate "Cadence PWM support"
> +        depends on OF
> +        depends on COMMON_CLK

An additional dependency on a SoC || COMPILE_TEST would be good to spare
users on (say) mips and x86 the question for PWM_CADENCE during
oldconfig.

> +        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_CLK
>  	tristate "Clock based PWM support"
>  	depends on HAVE_CLK || COMPILE_TEST
> [...]
> diff --git a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c
> new file mode 100644
> index 000000000000..12aaa004bf7f
> --- /dev/null
> +++ b/drivers/pwm/pwm-cadence.c
> @@ -0,0 +1,370 @@
> +// 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) 2023, Advanced Micro Devices, Inc.
> + */
> +
> [...]
> +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;

You only use cstate.enabled, so there is no need to copy the whole
struct to the stack.

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

If you set cstate.enabled = false here you can save another call to
ttc_pwm_disable() below.

> +		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);
> +	period_cycles = mul_u64_u64_div_u64(period_cycles, 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));
> +		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);

.apply() is supposed to yield the biggest possible period that isn't
bigger than the requested period.

I didn't do the complete maths, but I suspect this to be wrong for
several reasons:

 - div gets big if state->period is big. So div > 15 shouldn't result in
   -ERANGE but setting in a possible big period.
 - if (div) div -= 1; smells like you configure a too big period if
   div=0 was calculated. (Then you should return -ERANGE)
 - ROUND_CLOSEST is nearly always wrong in .apply()

If you test your driver with CONFIG_PWM_DEBUG enabled and then something
like:

	echo 0 > /sys/class/pwm/pwmchip0/export
	cd /sys/class/pwm/pwmchip0/pwm0
	echo 0 > duty_cycle
	seq 10000 500000 | while read p; do
		echo p > period
	done
	seq 500000 10000 -1 | while read p; do
		echo p > period
	done

should help you to get this right. (Pick a reasonable range for period
and test in 1ns steps.)

> +		flag = true;
> +	}
> +
> [...]
>
> +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);
> +
> +	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;
> +	else
> +		state->enabled = true;
> +
> +	rate = priv->rate;
> +
> +	pres_en =  ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL);

s/  / /

> +	pres_en	&= TTC_CLK_CNTRL_PS_EN;
> +
> +	if (pres_en) {
> +		pres = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL) & TTC_CLK_CNTRL_PSV;
> +		pres >>= TTC_CNTR_CTRL_PRESCALE_SHIFT;

Consider using FIELD_GET

> +		/* If prescale is enabled, the count rate is divided by 2^(pres + 1) */
> +		pres = BIT(pres + 1);
> +	}
> +
> [...]
> +
> +static const struct pwm_ops ttc_pwm_ops = {
> +	.apply = ttc_pwm_apply,
> +	.get_state = ttc_pwm_get_state,
> +	.owner = THIS_MODULE,

Assigning .owner isn't needed any more since
384461abcab6602abc06c2dfb8fb99beeeaa12b0.

> +};
> [...]
> +static 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);

Hmm, if there was a devm_clk_rate_exclusive_get, you could get rid of
the remove callback.

> +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature