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

[Uwe Kleine-König]

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/ |

Attachment: signature.asc
Description: PGP signature