| 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: <20211221091830.7jsklhvt7c3bhmjp@pengutronix.de>
Date: Tue, 21 Dec 2021 10:18:30 +0100
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: hammer hsieh <hammerh0314@...il.com>
Cc: thierry.reding@...il.com, lee.jones@...aro.org, robh+dt@...nel.org,
linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, wells.lu@...plus.com,
Hammer Hsieh <hammer.hsieh@...plus.com>
Subject: Re: [PATCH v1 2/2] pwm:sunplus-pwm:Add Sunplus SoC PWM Driver
Hello,
On Tue, Dec 21, 2021 at 02:28:24PM +0800, hammer hsieh wrote:
> Uwe Kleine-König <u.kleine-koenig@...gutronix.de> 於 2021年12月17日 週五
> 下午11:28寫道：
>
> > On Fri, Dec 17, 2021 at 07:46:08PM +0800, Hammer Hsieh wrote:
> > > Add Sunplus SoC PWM Driver
> > >
> > > Signed-off-by: Hammer Hsieh <hammer.hsieh@...plus.com>
> > > ---
> > > MAINTAINERS | 1 +
> > > drivers/pwm/Kconfig | 11 +++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sunplus.c | 192
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 205 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sunplus.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 721ed79..1c9e3c5 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -18246,6 +18246,7 @@ SUNPLUS PWM DRIVER
> > > M: Hammer Hsieh <hammer.hsieh@...plus.com>
> > > S: Maintained
> > > F: Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> > > +F: drivers/pwm/pwm-sunplus.c
> > >
> > > SUPERH
> > > M: Yoshinori Sato <ysato@...rs.sourceforge.jp>
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 21e3b05..9df5d5f 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -526,6 +526,17 @@ config PWM_SPRD
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-sprd.
> > >
> > > +config PWM_SUNPLUS
> > > + tristate "Sunplus PWM support"
> > > + depends on ARCH_SUNPLUS || COMPILE_TEST
> > > + depends on HAS_IOMEM && OF
> > > + help
> > > + Generic PWM framework driver for the PWM controller on
> > > + Sunplus SoCs.
> > > +
> > > + To compile this driver as a module, choose M here: the module
> > > + will be called pwm-sunplus.
> > > +
> > > config PWM_STI
> > > tristate "STiH4xx PWM support"
> > > depends on ARCH_STI || COMPILE_TEST
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 708840b..be58616 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -53,6 +53,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> > > obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
> > > obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
> > > obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
> > > +obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
> > > obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> > > obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> > > obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
> > > diff --git a/drivers/pwm/pwm-sunplus.c b/drivers/pwm/pwm-sunplus.c
> > > new file mode 100644
> > > index 0000000..0ae59fc
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sunplus.c
> > > @@ -0,0 +1,192 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PWM device driver for SUNPLUS SoCs
> > > + *
> > > + * Author: Hammer Hsieh <hammer.hsieh@...plus.com>
> > > + */
> >
> > Please add a section here about your hardware limitations. Please stick
> > to the format used in e.g. pwm-sifive.c. That is a block starting with
> >
> > * Limitations:
> >
> > and then a list of issues. One such item is: Only supports normal
> > polarity.
>
> ok, will add it.
Can you please fix your mailer to properly quote and mark your text
accordingly? (I fixed it up, so the above looks right, in your mail
however it doesn't.)
> > > +#include <linux/clk.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pwm.h>
> > > +
> > > +#define PWM_SUP_CONTROL0 0x000
> > > +#define PWM_SUP_CONTROL1 0x004
> > > +#define PWM_SUP_FREQ_BASE 0x008
> > > +#define PWM_SUP_DUTY_BASE 0x018
> > > +#define PWM_SUP_FREQ(ch) (PWM_SUP_FREQ_BASE + 4 * (ch))
> > > +#define PWM_SUP_DUTY(ch) (PWM_SUP_DUTY_BASE + 4 * (ch))
> > > +#define PWM_SUP_FREQ_MAX GENMASK(15, 0)
> > > +#define PWM_SUP_DUTY_MAX GENMASK(7, 0)
> > > +
> > > +#define PWM_SUP_NUM 4
> > > +#define PWM_BYPASS_BIT_SHIFT 8
> > > +#define PWM_DD_SEL_BIT_SHIFT 8
> > > +#define PWM_SUP_FREQ_SCALER 256
> > > +
> > > +struct sunplus_pwm {
> > > + struct pwm_chip chip;
> > > + void __iomem *base;
> > > + struct clk *clk;
> > > +};
> > > +
> > > +static inline struct sunplus_pwm *to_sunplus_pwm(struct pwm_chip *chip)
> > > +{
> > > + return container_of(chip, struct sunplus_pwm, chip);
> > > +}
> > > +
> > > +static void sunplus_reg_init(void __iomem *base)
> > > +{
> > > + u32 i, value;
> > > +
> > > + /* turn off all pwm channel output */
> > > + value = readl(base + PWM_SUP_CONTROL0);
> > > + value &= ~GENMASK((PWM_SUP_NUM - 1), 0);
> > > + writel(value, base + PWM_SUP_CONTROL0);
> > > +
> > > + /* init all pwm channel clock source */
> > > + value = readl(base + PWM_SUP_CONTROL1);
> > > + value |= GENMASK((PWM_SUP_NUM - 1), 0);
> > > + writel(value, base + PWM_SUP_CONTROL1);
> > > +
> > > + /* init all freq and duty setting */
> > > + for (i = 0; i < PWM_SUP_NUM; i++) {
> > > + writel(0, base + PWM_SUP_FREQ(i));
> > > + writel(0, base + PWM_SUP_DUTY(i));
> > > + }
> >
> > Please keep the PWM in their boot-up state. That is, if the bootloader
> > enabled a display with a bootsplash, don't disable the backlight when
> > the PWM driver loads.
>
> ok, will remove the init register code.
>
>
> > > +}
> > > +
> > > +static int sunplus_pwm_apply(struct pwm_chip *chip, struct pwm_device
> > *pwm,
> > > + const struct pwm_state *state)
> > > +{
> > > + struct sunplus_pwm *priv = to_sunplus_pwm(chip);
> > > + u32 period_ns, duty_ns, value;
> > > + u32 dd_freq, duty;
> > > + u64 tmp;
> > > +
> >
> > if (state->polarity != PWM_POLARITY_NORMAL)
> > return -EINVAL;
> >
> > > + if (!state->enabled) {
> > > + value = readl(priv->base + PWM_SUP_CONTROL0);
> > > + value &= ~BIT(pwm->hwpwm);
> > > + writel(value, priv->base + PWM_SUP_CONTROL0);
> > > + return 0;
> > > + }
> > > +
> > > + period_ns = state->period;
> >
> > state->period is an u64, so you might loose precision here.
> >
> > > + duty_ns = state->duty_cycle;
> >
> > ditto
> >
> > > +
> > > + /* cal pwm freq and check value under range */
> > > + tmp = clk_get_rate(priv->clk) * (u64)period_ns;
> >
> > This might overflow?
> >
> > > + tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > > + tmp = DIV_ROUND_CLOSEST_ULL(tmp, PWM_SUP_FREQ_SCALER);
> >
> > In general you should pick the highest period that isn't bigger than the
> > requested period. I didn't check in detail, but using round-closest is a
> > strong hint that you get that wrong.
> >
> > > + dd_freq = (u32)tmp;
> > > +
> > > + if (dd_freq == 0)
> > > + return -EINVAL;
> > > +
> > > + if (dd_freq > PWM_SUP_FREQ_MAX)
> > > + dd_freq = PWM_SUP_FREQ_MAX;
> > > +
> > > + writel(dd_freq, priv->base + PWM_SUP_FREQ(pwm->hwpwm));
> > > +
> > > + /* cal and set pwm duty */
> > > + value = readl(priv->base + PWM_SUP_CONTROL0);
> > > + value |= BIT(pwm->hwpwm);
> > > + if (duty_ns == period_ns) {
> > > + value |= BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> > > + duty = PWM_SUP_DUTY_MAX;
> > > + } else {
> > > + value &= ~BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> > > + tmp = (u64)duty_ns * PWM_SUP_FREQ_SCALER + (period_ns >>
> > 1);
> > > + tmp = DIV_ROUND_CLOSEST_ULL(tmp, (u64)period_ns);
> > > + duty = (u32)tmp;
> > > + duty |= (pwm->hwpwm << PWM_DD_SEL_BIT_SHIFT);
> >
> > This is also more inexact than necessary. In general don't use period_ns
> > in the calculation of duty register settings. As with period you're
> > supposed to pick the biggest possible dutycycle not bigger than the
> > requested value.
> >
> > Consider a PWM that with register P = P and register D = D implements a
> > PWM output with period = 1000 * P ns and duty_cycle = 1000 * D ns
> >
> > For a request of period = 39900 and duty_cycle = 12100, you have to pick
> > P = 39 and D = 12. However P * duty_ns / period_ns = 11.82 ...
> >
> >
> static int sunplus_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> const struct pwm_state *state)
> {
> struct sunplus_pwm *priv = to_sunplus_pwm(chip);
> u32 dd_freq, duty, value, value1;
> u64 period_ns, duty_ns, tmp;
> u64 period_ns_max;
>
> if (state->polarity != pwm->state.polarity)
> return -EINVAL;
>
> if (!state->enabled) {
> /* disable pwm channel output */
> value = readl(priv->base + PWM_SUP_CONTROL0);
> value &= ~BIT(pwm->hwpwm);
> writel(value, priv->base + PWM_SUP_CONTROL0);
> /* disable pwm channel clk source */
> value = readl(priv->base + PWM_SUP_CONTROL1);
> value &= ~BIT(pwm->hwpwm);
> writel(value, priv->base + PWM_SUP_CONTROL1);
> return 0;
> }
>
> tmp = PWM_SUP_FREQ_SCALER * NSEC_PER_SEC;
> tmp = DIV64_U64_ROUND_CLOSEST(tmp, clk_get_rate(priv->clk));
> period_ns_max = PWM_SUP_FREQ_MAX * tmp;
>
> if (state->period > period_ns_max)
> return -EINVAL;
>
> period_ns = state->period;
> duty_ns = state->duty_cycle;
>
> /* cal pwm freq and check value under range */
> tmp = DIV64_U64_ROUND_CLOSEST(clk_get_rate(priv->clk),
> PWM_SUP_FREQ_SCALER);
> tmp = tmp * period_ns >> 10;
> tmp = DIV64_U64_ROUND_CLOSEST(tmp, NSEC_PER_SEC >> 10);
> dd_freq = (u32)tmp;
>
> if (dd_freq == 0)
> return -EINVAL;
>
> if (dd_freq > PWM_SUP_FREQ_MAX)
> dd_freq = PWM_SUP_FREQ_MAX;
>
> writel(dd_freq, priv->base + PWM_SUP_FREQ(pwm->hwpwm));
>
> /* cal and set pwm duty */
> value = readl(priv->base + PWM_SUP_CONTROL0);
> value |= BIT(pwm->hwpwm);
> value1 = readl(priv->base + PWM_SUP_CONTROL1);
> value1 |= BIT(pwm->hwpwm);
> if (duty_ns == period_ns) {
> value |= BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> duty = PWM_SUP_DUTY_MAX;
> } else {
> value &= ~BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> tmp = (duty_ns >> 10) * PWM_SUP_FREQ_SCALER;
> tmp = DIV64_U64_ROUND_CLOSEST(tmp, (period_ns >> 10));
> duty = (u32)tmp;
> duty |= (pwm->hwpwm << PWM_DD_SEL_BIT_SHIFT);
> }
> writel(value, priv->base + PWM_SUP_CONTROL0);
> writel(value1, priv->base + PWM_SUP_CONTROL1);
> writel(duty, priv->base + PWM_SUP_DUTY(pwm->hwpwm));
>
> return 0;
> }
This is badly indented, I'm delaying review until the next patch.
> while turn on PWM_DEBUG.
> I still get warning message like
> sunplus-pwm 9c007a00.pwm: .apply is not idempotent (ena=1 pol=0
> 9998240/19996480)->(ena=1 pol=0 9996976/19993952)
After you applied a setting the debug code calls .get_state and applies
its result. The problem is that applying
duty_cycle = 9998240 + period = 19996480 yields duty_cycle = 9996976 +
period = 19993952 though there should be an exact match.
There is still a ROUND_CLOSEST in your .apply callback ... that probably
cannot implement "pick the biggest possible period not bigger than the
rquested period".
> echo 20000000 > period
> echo 10000000 > duty_cycle
> echo 1 > enable
> I'm not sure it's a issue or not?
> get_state calculate reg value to state->period and state->duty_cycle.
> apply calculate state->period and state->duty_cycle to reg value.
> Can't match always.
Yes, it's not a 1:1 relation. If however applying
period = $pa
duty_cycle = $da
yields a state with
period = $pr
duty_cycle = $dr
then applying $dr / $pr should give an exact match.
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