Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically

[Paul Cercueil]

Hi Uwe,



Le lun. 22 juil. 2019 à 15:34, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@xxxxxxxxxxxxxx> a écrit :
> Hello Paul,
>
> On Fri, Jun 07, 2019 at 05:44:07PM +0200, Paul Cercueil wrote:
> >  -static int jz4740_pwm_config(struct pwm_chip *chip, struct 
> > pwm_device *pwm,
> >  -			     int duty_ns, int period_ns)
> >  +static int jz4740_pwm_apply(struct pwm_chip *chip, struct 
> > pwm_device *pwm,
> >  +			    struct pwm_state *state)
> >   {
> >   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> >   	unsigned long long tmp;
> >   	unsigned long period, duty;
> >   	unsigned int prescaler = 0;
> >   	uint16_t ctrl;
> >  -	bool is_enabled;
> >
> >  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
> >  +	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * 
> > state->period;
> >   	do_div(tmp, 1000000000);
> >   	period = tmp;
> >
> >  @@ -96,16 +95,14 @@ static int jz4740_pwm_config(struct pwm_chip 
> > *chip, struct pwm_device *pwm,
> >   	if (prescaler == 6)
> >   		return -EINVAL;
> >
> >  -	tmp = (unsigned long long)period * duty_ns;
> >  -	do_div(tmp, period_ns);
> >  +	tmp = (unsigned long long)period * state->duty_cycle;
> >  +	do_div(tmp, state->period);
> >   	duty = period - tmp;
> >
> >   	if (duty >= period)
> >   		duty = period - 1;
> >
> >  -	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
> >  -	if (is_enabled)
> >  -		jz4740_pwm_disable(chip, pwm);
> >  +	jz4740_pwm_disable(chip, pwm);
>
> I assume this stops the PWM. Does this complete the currently running
> period? How does the PWM behave then? (Does it still drive the output?
> If so, on which level?)

Some PWM channels work in one mode "TCU1" and others work in "TCU2". The
mode in which channels work depends on the version of the SoC.

When stopped, the pins of TCU1 channels will be driven to the inactive
level (which depends on the polarity). It is unknown whether or not the
currently running period is completed. We set a bit to configure for
"abrupt shutdown", so I expect that it's not, but somebody would need
to hook up a logic analyzer to see what's the exact behaviour with
and without that bit.

TCU2 channels on the other hand will stop in the middle of a period,
leaving the pin hanging at whatever level it was before the stop.
That's the rationale behind the trick in commit 6580fd173070 ("pwm:
jz4740: Force TCU2 channels to return to their init level").

Regards,
-Paul


> >   	jz4740_timer_set_count(pwm->hwpwm, 0);
> >   	jz4740_timer_set_duty(pwm->hwpwm, duty);
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |