Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically
Hi Uwe,
Le mer. 24 juil. 2019 à 2:47, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
a écrit :
Hello Paul,
On Tue, Jul 23, 2019 at 04:46:40PM -0400, Paul Cercueil wrote:
Le lun. 22 juil. 2019 à 15:34, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
a écrit :
> On Fri, Jun 07, 2019 at 05:44:07PM +0200, Paul Cercueil wrote:
> > - 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.
This might be done even without a logic analyzer. Just do something
like:
pwm_apply_state(pwm, { .enabled = 1, .period = 5s })
pwm_apply_state(pwm, { .enabled = 1, .period = 5s, .duty = 5s })
and if that takes less then 5s the period is not completed.
And note that "abrupt shutdown" is a bug.
I remember you asked that already in an older patchset.
The result of this test is that the period is never completed,
independently of the "abrupt shutdown" bit.
Cheers,
-Paul
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").
Strange, but ok.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König
|
Industrial Linux Solutions |
http://www.pengutronix.de/ |
Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically
Hello Paul,
On Tue, Jul 23, 2019 at 04:46:40PM -0400, Paul Cercueil wrote:
> Le lun. 22 juil. 2019 à 15:34, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> a écrit :
> > On Fri, Jun 07, 2019 at 05:44:07PM +0200, Paul Cercueil wrote:
> > > -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.
This might be done even without a logic analyzer. Just do something
like:
pwm_apply_state(pwm, { .enabled = 1, .period = 5s })
pwm_apply_state(pwm, { .enabled = 1, .period = 5s, .duty = 5s })
and if that takes less then 5s the period is not completed.
And note that "abrupt shutdown" is a bug.
> 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").
Strange, but ok.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically
Hi Uwe,
Le lun. 22 juil. 2019 à 15:34, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
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, 10);
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/ |
Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically
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, 10);
> 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?)
>
> 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/ |
