RE: [PATCH V7 2/5] pwm: Add i.MX TPM PWM driver support
From: Anson Huang
Date: Wed Mar 20 2019 - 08:44:12 EST
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@xxxxxxxxxxxxxx>
> Cc: thierry.reding@xxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx;
> shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; otavio@xxxxxxxxxxxxxxxx;
> stefan@xxxxxxxx; Leonard Crestez <leonard.crestez@xxxxxxx>;
> schnitzeltony@xxxxxxxxx; 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 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@xxxxxxxxxxxxxx]
> > Sent: 2019å3æ20æ 18:58
> > 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;
> > otavio@xxxxxxxxxxxxxxxx; stefan@xxxxxxxx; Leonard Crestez
> > <leonard.crestez@xxxxxxx>; schnitzeltony@xxxxxxxxx; 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 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&data=02%7C01%7Canson.h
> >
> uang%40nxp.com%7C626a6f5603f74ae0d37e08d6ad22e774%7C686ea1d3bc
> >
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C636886762757876916&sdata=r
> >
> wu%2BUo3GlRX8j4lXSOVuAs7n1yEuP5P2W6vhY%2BjiXdQ%3D&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, ¶m, 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&data=02%7C01%7Canson.huang%40nxp.com%7C62
> >
> 6a6f5603f74ae0d37e08d6ad22e774%7C686ea1d3bc2b4c6fa92cd99c5c30163
> >
> 5%7C0%7C0%7C636886762757876916&sdata=JsNRa8DuGYizE7FCyHVuY
> > QSUu4eUu5qTh6Edpf3Azm8%3D&reserved=0 |