linux-kernel - Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231102113016.jgsh7jru6vjv4vsp@pengutronix.de>
Date:   Thu, 2 Nov 2023 12:30:16 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     William Qiu <william.qiu@...rfivetech.com>
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-riscv@...ts.infradead.org, linux-pwm@...r.kernel.org,
        Emil Renner Berthing <kernel@...il.dk>,
        Rob Herring <robh+dt@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Hal Feng <hal.feng@...rfivetech.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

Hello William,

On Wed, Nov 01, 2023 at 10:22:44AM +0800, William Qiu wrote:
> 
> 
> On 2023/10/20 19:25, Uwe Kleine-König wrote:
> >> +	void __iomem *base = pwm->data->get_ch_base ?
> >> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
> >> +	u32 period_data, duty_data, ctrl_data;
> >> +
> >> +	period_data = readl(REG_OCPWM_LRC(base));
> >> +	duty_data = readl(REG_OCPWM_HRC(base));
> >> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
> >> +
> >> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
> >> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
> > 
> > Please test your driver with PWM_DEBUG enabled. The rounding is wrong
> > here.
> 
> The conclusion after checking is: when the period or duty_cycle value set
> by the user is not divisible (1000000000/49.5M), there will be an error.
> This error is due to hardware accuracy. So why is rounding is wrong?
> rockchip also has a similar implementation drivers/pwm/ pwm-rockchip.c

I fail to follow. Where is an error?

The general policy (for new drivers at least) is to implement the
biggest period possible not bigger than the requested period. That means
that .apply must round down and to make .apply ∘ .get_state idempotent
.get_state must round up to match.

Assuming a clkrate of 49500000 Hz the actual period for REG_OCPWM_LRC =
400 is 8080.808ns and for REG_OCPWM_LRC = 401 is 8101.010.

So with REG_OCPWM_LRC = 401 .get_state should report state.period = 8102
[ns] because if you call .apply with .period = 8101 [ns] you're supposed
to use REG_OCPWM_LRC = 400.

Rounding using DIV_ROUND_CLOSEST doesn't give consistent behaviour in
some cases. Consider a PWM that can implement the following periods (and
none in between):

	20.1 ns
	20.4 ns
	21.7 ns

With round-to-nearest a request to configure 21 ns will yield 20.4 ns.
If you call .get_state there the driver will return 20 ns. However
configuring 20 ns results in a period of 20.1 ns.

With rounding as requested above you get a consistent behaviour. After
.apply_state(period=21) .get_state() returns period=21.

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)