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

[Uwe Kleine-König]

Hello Anson,

On Wed, Mar 20, 2019 at 10:17:50AM +0000, Anson Huang wrote:
> > On Wed, Mar 20, 2019 at 05:06:21AM +0000, Anson Huang wrote:
> > > +	/* make sure counter is disabled for programming prescale */
> > > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > > +	if (saved_cmod) {
> > > +		val &= ~PWM_IMX_TPM_SC_CMOD;
> > > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > 
> > I thought we agreed on not stopping the counter if the PS field isn't changed?
> 
> If the PS field no need to change, the round state should already return the period
> equal to current period settings, so this function will NOT be called, right?
> 
>          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;
>         } 

If the PS field is already right .period might still not match as its
value depends on SC.PS and MOD.MOD.

> > > +	val &= ~PWM_IMX_TPM_SC_PS;
> > > +	val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p.prescale);
> > > +	writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > +
> > > +	/*
> > > +	 * set period count: according to RM, the MOD register is
> > > +	 * updated immediately after CMOD[1:0] = 2b'00 above
> > > +	 */
> > 
> > So the current period isn't completed? That's wrong.
> 
> So you mean we have to wait for the current period to finish here?
> I did NOT know this, so we have to compare the counter value with

Yeah, see https://patchwork.ozlabs.org/patch/1021566/ which documents
this but waits for feedback by Thierry since some time.

> the MOD value until they match then proceed the period change?

If the hardware doesn't support you here (usually it does) I think it's
not reliable enough to try to sync in software. I assume you can get the
right wave form if you don't change SC.PS but only MOD.MOD? So maybe the
sanest approach is to refuse changing SC.PS if the PWM is running.

Or disable (which also should wait for the current period to finish),
poll for the end (or use an irq?) and then setup the new configuration.

> > > +{
> > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > +	struct pwm_state c;
> > > +	u32 val, sc_val;
> > > +	u64 tmp;
> > > +
> > > +	pwm_imx_tpm_get_state(chip, pwm, &c);
> > > +
> > > +	if (state.duty_cycle != c.duty_cycle) {
> > > +		/* set duty counter */
> > > +		tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD;
> > > +		tmp *= state.duty_cycle;
> > > +		val = DIV_ROUND_CLOSEST_ULL(tmp, state.period);
> > > +		writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > +	}
> > > +
> > > +	if (state.enabled != c.enabled) {
> > 
> > This is wrong. If the PWM was running (c.enabled == true) and you are
> > supposed to disable (state.enabled == false) you enable the hardware once
> > more.
> 
> A little confused here, as the case you assume, inside this block, there is another
> check for state.enabled, if it is false, the polarity will be set to channel disabled mode,
> the polarity setting is combined together with the enable status here, am I wrong?  

Ah, you're right. I missed that probably because I saw register accesses
after the state.enabled != c.enabled check.

> > > +			val |= (state.polarity == PWM_POLARITY_NORMAL) ?
> > > +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> > > +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
> > 
> > Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it
> > together with PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you
> > put the FIELD_PREP into the definition the line doesn't get excessively long.
> > 
> 
> I put the FIELD_PREP into definition, the line still long, but I agree using definition is better.
> 
> #define PWM_IMX_TPM_CnSC_ELS_INVERSED   FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1)
> #define PWM_IMX_TPM_CnSC_ELS_NORMAL     FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2)
> 
>                          val |= (state->polarity == PWM_POLARITY_NORMAL) ?
>                                  PWM_IMX_TPM_CnSC_ELS_NORMAL :
>                                  PWM_IMX_TPM_CnSC_ELS_INVERSED;
> 
> > Maybe also add
> > 
> > 	#define PWM_IMX_TPM_CnSC_ELS_INACTIVE FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0)
> > 
> 
> I did NOT use the FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) at all, so why add it?
> I don't quite understand.

You use it implicitly in pwm_imx_tpm_apply_hw() if state.enabled ==
false and c.enabled == true:

	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
	val &= ~(PWM_IMX_TPM_CnSC_ELS | ...);
	...
	writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));

> > > +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_config_counter(chip, param);
> > > +		tpm->real_period = real_state.period;
> > > +	}
> > 
> > Maybe add a comment that this could still be optimized. For example if
> > pwm_imx_tpm_round_state returned prescale = 5 but prescale is currently 6,
> > you might still be able to configure
> 
> You meant for multiple users request different period case? In this block, if there is
> ONLY one user and the requested period can be met by HW, it will anyway re-configure
> the HW for the prescale and period I think, or I did NOT get your point?

My idea has a flaw. I thought that if there is another user, the
duty_cycle can still be represented if the actually used prescale value
is slightly higher. But then there is still a problem with the period
length that I missed. So my remark was wrong, sorry for that.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |