lkml.org 
[lkml]   [2019]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V8 2/5] pwm: Add i.MX TPM PWM driver support
Date
Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月21日 21:42
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux@armlinux.org.uk; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V8 2/5] pwm: Add i.MX TPM PWM driver support
>
> Hello,
>
> On Thu, Mar 21, 2019 at 12:47:32PM +0000, Anson Huang wrote:
> > > On Thu, Mar 21, 2019 at 09:54:15AM +0000, Anson Huang wrote:
> > > > > On Thu, Mar 21, 2019 at 12:47:57AM +0000, Anson Huang wrote:
> > > > > > +static void pwm_imx_tpm_setup_period(struct pwm_chip *chip,
> > > > > > + struct imx_tpm_pwm_param *p) {
> > > > > > + struct imx_tpm_pwm_chip *tpm =
> to_imx_tpm_pwm_chip(chip);
> > > > > > + u32 val, saved_cmod, cur_prescale;
> > > > > > +
> > > > > > + /* make sure counter is disabled for programming prescale
> */
> > > > >
> > > > > @Thierry: What is your thought here? IMHO this should only be
> > > > > allowed if all affected PWMs are off.
> > > >
> > > > As we already make sure that ONLY when there is ONLY 1 user and
> > > > the requested period is different from the current period, then
> > > > this function will be called, so there is impossible that multiple
> > > > PWMs are active
> > > and the period is requested to be changed.
> > > > Am I right?
> > >
> > > This problem is not about two PWMs. If you reconfigure a running PWM
> > > the requirement is that the hardware completes a whole period with
> > > the old configuration and then immediately starts a new period with
> > > the new parameters. If you stop the counter, the last period with
> > > the old parameters is disturbed.
> >
> > So, I think simply return error if the counter is running and there is
> > a new PS change request, right?
>
> That's the general idea yes. If you cannot fulfil the request without violating
> the guarantees you have to adhere, refuse the request with an error.

Already add below check in next version:

val = readl(tpm->base + PWM_IMX_TPM_SC);
cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
if (cmod && cur_prescale != p->prescale)
return -EBUSY;

>
> > > > > > + */
> > > > > > + state->polarity = PWM_POLARITY_NORMAL;
> > > > > > +
> > > > > > + /* get channel status */
> > > > > > + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ?
> true :
> > > > > > +false; }
> > > > > > +
> > > > > > +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > > > > > + struct pwm_device *pwm,
> > > > > > + struct pwm_state *state)
> > > > >
> > > > > pwm_imx_tpm_apply_hw is called with the mutex hold. Is this
> necessary?
> > > > > Please either call it without the mutex or annotate the function
> > > > > that the caller is supposed to hold it.
> > > >
> > > > OK, will make sure call it without mutex hold.

After further check, since this function will call get_state and it will access
shared registers, so mutex is necessary I think, so I will add annotate for this
function.

>
> Reading through the reference manual I noticed that there might be a
> stall: If you write two values to CnV the second write is ignored if the first
> wasn't latched yet. That might mean that you cannot release the mutex
> before the newly configured state is active. This is related to the request to
> not let .apply return before the configured state is active, but I didn't thought
> this to an end what the real consequences have to be.

The reference manual says the register is NOT updated until the current period finished
If counter is running, so I added below check for both period update and duty update, we
Can just wait the register value read matches what we write:

Period update:
writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);

/* make sure MOD register is updated */
timeout = jiffies + msecs_to_jiffies(tpm->real_period /
NSEC_PER_MSEC + 1);
while (readl(tpm->base + PWM_IMX_TPM_MOD != p->mod)) {
if (time_after(jiffies, timeout))
return = -ETIME;
cpu_relax();
}

Duty update:
writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));

/* make sure CnV register is updated */
timeout = jiffies + msecs_to_jiffies(tpm->real_period /
NSEC_PER_MSEC + 1);
while (readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)) != val) {
if (time_after(jiffies, timeout))
return = -ETIME;
cpu_relax();
}


>
> > > > If counter is enabled, and for edge aligned PWM mode(EPWM), the
> > > > register is updated After written and TPM counter changes from MOD
> > > > to zero, same as period count update, HW will make sure the period
> finish..
> > >
> > > Looking into my concern again, it is actually the other way around:
> > > Assuming a single used PWM channel that runs at duty_cycle=500 +
> > > period=1000. Then pwm_imx_tpm_apply() is called with state-
> > > >duty_cycle=700 and state->period=800. pwm_imx_tpm_apply() calls
> > > pwm_imx_tpm_setup_period() to configure for .period=1000. Now if the
> > > PWM completes a period before pwm_imx_tpm_apply_hw() sets up CnV
> to
> > > the value corresponding to duty_cycle=700, it produces a waveform
> > > with
> > > duty_cycle=500 and period=800 which is bad. This is another
> > > limitation that can be worked around in software with some effort
> > > (which might or might not be worth to spend).
> >
> > I am sure that on i.MX7ULP platform we used for backlight ONLY, it
> > should NOT that matter if this case happen, unless the counter is
> > disabled, then the effort spend on this case will be huge, so I plan to leave
> it as what it is if you don't mind.
>
> That means you have to add this to the list of limitations.

OK, will try best to describe this scenario in limitation note;

>
> > > > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip,
> > > > > > + struct pwm_device *pwm,
> > > > > > + struct pwm_state *state) {
> > > > > > + struct imx_tpm_pwm_chip *tpm =
> to_imx_tpm_pwm_chip(chip);
> > > > > > + struct imx_tpm_pwm_param param;
> > > > > > + struct pwm_state real_state;
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = pwm_imx_tpm_round_state(chip, &param, state,
> &real_state);
> > > > > > + if (ret)
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + mutex_lock(&tpm->lock);
> > > > > > +
> > > > > > + /*
> > > > > > + * TPM counter is shared by multiple channels, so
> > > > > > + * prescale and period can NOT be modified when
> > > > > > + * there are multiple channels in use with different
> > > > > > + * period settings.
> > > > > > + */
> > > > > > + if (real_state.period != tpm->real_period) {
> > > > > > + if (tpm->user_count > 1) {
> > > > > > + ret = -EBUSY;
> > > > > > + goto exit;
> > > > > > + }
> > > > > > +
> > > > > > + pwm_imx_tpm_setup_period(chip, &param);
> > > > > > + tpm->real_period = real_state.period;
> > > > > > + }
> > > > > > +
> > > > > > + pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
> > > > > > +
> > > > > > +exit:
> > > > > > + mutex_unlock(&tpm->lock);
> > > > >
> > > > > .apply is supposed to sleep until the newly configured state is active.
> > > > > This is missing here, right?
> > > >
> > > > NOT quite understand, you meant .apply could be sleep if mutex is
> > > > hold by other thread?
> > >
> > > No. .apply is supposed to only return when the new configuration is
> active.
> > > So if the PWM is running in its previous configuration, you setup
> > > the registers such that the new configuration gets active in the
> > > next period, you must not yet return to the caller until the new period
> started.
> >
> > That bring me back to previous question, we can add waiting for period
> > finish And then return from .apply, but we also need a timeout for the
> > wait, what should The timeout value be? 100mS? Or even several seconds?
>
> A sane value would be the duration of the previously configured period
> length as this is the theoretical upper limit for this delay.

I will just use tpm->real_period saved in driver data, for first time update, although
Tpm->real_period is 0, but the counter is disabled, so the register update is immediately,
From second time, the tpm->real_period will be previous period configured.

Anson.

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7Cae
> 5949f109c5401a2b5d08d6ae030d7f%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636887725468509805&amp;sdata=4qU1eJzlIle9v%2BHqEqwb
> Jm%2FkkAQlCH2LtsFDMaeSGHE%3D&amp;reserved=0 |
\
 
 \ /
  Last update: 2019-03-21 15:54    [W:0.079 / U:79.768 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site