RE: [PATCH V8 2/5] pwm: Add i.MX TPM PWM driver support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@xxxxxxxxxxxxxx]
> Sent: 2019年3月21日 21:42
> To: Anson Huang <anson.huang@xxxxxxx>
> Cc: thierry.reding@xxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx;
> shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; stefan@xxxxxxxx;
> otavio@xxxxxxxxxxxxxxxx; Leonard Crestez <leonard.crestez@xxxxxxx>;
> schnitzeltony@xxxxxxxxx; jan.tuerk@xxxxxxxxxxx; Robin Gong
> <yibin.gong@xxxxxxx>; linux-pwm@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> 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  |




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux