| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20231114215906.2q45o4w4epr6rpk5@pengutronix.de>
Date: Tue, 14 Nov 2023 22:59:06 +0100
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Mubin Sayyed <mubin.sayyed@....com>
Cc: krzysztof.kozlowski+dt@...aro.org, thierry.reding@...il.com,
robh+dt@...nel.org, conor+dt@...nel.org, tglx@...utronix.de,
daniel.lezcano@...aro.org, michal.simek@....com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-pwm@...r.kernel.org, git@....com,
mubin10@...il.com
Subject: Re: [LINUX PATCH v2 3/3] pwm: pwm-cadence: Add support for TTC PWM
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/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists