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

[Anson Huang <anson.huang@nxp.com>]

Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Anson Huang
> Sent: 2019年3月20日 19:21
> To: 'Uwe Kleine-König' <u.kleine-koenig@pengutronix.de>
> 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; otavio@ossystems.com.br;
> stefan@agner.ch; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.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 V7 2/5] pwm: Add i.MX TPM PWM driver support
> 
> Hi, Uwe
> 
> Best Regards!
> Anson Huang
> 
> > -----Original Message-----
> > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> > Sent: 2019年3月20日 18:58
> > 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;
> > otavio@ossystems.com.br; stefan@agner.ch; Leonard Crestez
> > <leonard.crestez@nxp.com>; schnitzeltony@gmail.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 V7 2/5] pwm: Add i.MX TPM PWM driver support
> >
> > 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.
> 
> Ah, my bad... I did NOT know what I was thinking...
> Yes, I will add the PS check to decide whether disabling counter..

I added below additional check for current PS and the new PS

         cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
         if (saved_cmod && cur_prescale != p->prescale) {
                 val &= ~PWM_IMX_TPM_SC_CMOD;
                 writel(val, tpm->base + PWM_IMX_TPM_SC);
         }


> 
> 
> >
> > > > > +	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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > h
> work.ozlabs.org%2Fpatch%2F1021566%2F&amp;data=02%7C01%7Canson.h
> >
> uang%40nxp.com%7C626a6f5603f74ae0d37e08d6ad22e774%7C686ea1d3bc
> >
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C636886762757876916&amp;sdata=r
> >
> wu%2BUo3GlRX8j4lXSOVuAs7n1yEuP5P2W6vhY%2BjiXdQ%3D&amp;reserve
> > d=0 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.
> 
> Let me try to poll the TOF (timer overflow) before setup the new
> configuration.
> And will also need to add timeout for the polling, what shoud the timeout
> value be, 100ms? As ideally the max period can be very large, several
> seconds or even large, so is the 100mS good here?

After further check, the reference manual has below statement, so I think we no
need to care about it, the hardware make sure of that, I added below comment
before programming the MOD register, if counter is disabled, the MOD register
will be updated immediately, if counter is enabled, the CPWM bit is fixed as 0 in
our driver, so the MOD register will be updated when counter changes from MOD
to zero.

         /*
          * set period count: according to RM, the MOD register is
          * updated immediately if CMOD[1:0] = 2b'00.
          * if CMOD[1:0] != 2b'00, then MOD register is updated
          * according to the CPWMS bit, that is:
          *
          * If the selected mode is not CPWM then MOD register is
          * updated after MOD register was written and the TPM
          * counter changes from MOD to zero.
          *
          * If the selected mode is CPWM then MOD register is updated
          * after MOD register was written and the TPM counter changes
          * from MOD to (MOD – 1).
          */



> 
> >
> > > > > +{
> > > > > +	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:

But the place I used is just to clear the PWM_IMX_TPM_CnSC_ELS field, so
just the MASK is enough for me, if you don't mind, I will leave it as what it is now.

Anson.

> >
> > 	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));
> 
> Ah, OK, I can replace the register field clear with the field prepare definition.
> 
> >
> > > > > +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.
> 
> Thanks,
> 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%7C62
> >
> 6a6f5603f74ae0d37e08d6ad22e774%7C686ea1d3bc2b4c6fa92cd99c5c30163
> >
> 5%7C0%7C0%7C636886762757876916&amp;sdata=JsNRa8DuGYizE7FCyHVuY
> > QSUu4eUu5qTh6Edpf3Azm8%3D&amp;reserved=0  |