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

From: Anson Huang
Date: Tue Apr 09 2019 - 08:11:08 EST

Next message: Christoph Hellwig: "Re: [RFC PATCH 4/5] acpi/hmat: Register special purpose memory as a device"
Previous message: Alexandre Belloni: "[PATCH 3/5] usb: gadget: udc: lpc32xx: properly setup phy interrupts"
In reply to: Anson Huang: "RE: [EXT] Re: [PATCH V10 2/5] pwm: Add i.MX TPM PWM driver support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Best Regards!
Anson Huang

> -----Original Message-----
> From: Anson Huang
> Sent: 2019å4æ9æ 20:04
> To: 'Uwe Kleine-KÃnig' <u.kleine-koenig@xxxxxxxxxxxxxx>
> Cc: mark.rutland@xxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; Robin Gong
> <yibin.gong@xxxxxxx>; schnitzeltony@xxxxxxxxx;
> otavio@xxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; linux@xxxxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> thierry.reding@xxxxxxxxx; stefan@xxxxxxxx; kernel@xxxxxxxxxxxxxx;
> Leonard Crestez <leonard.crestez@xxxxxxx>; shawnguo@xxxxxxxxxx; linux-
> arm-kernel@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> Subject: RE: [EXT] Re: [PATCH V10 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å4æ9æ 17:29
> > To: Anson Huang <anson.huang@xxxxxxx>
> > Cc: mark.rutland@xxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; Robin Gong
> > <yibin.gong@xxxxxxx>; schnitzeltony@xxxxxxxxx;
> > otavio@xxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > festevam@xxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; linux@xxxxxxxxxxxxxxx;
> > robh+dt@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > thierry.reding@xxxxxxxxx; stefan@xxxxxxxx; kernel@xxxxxxxxxxxxxx;
> > Leonard Crestez <leonard.crestez@xxxxxxx>; shawnguo@xxxxxxxxxx;
> linux-
> > arm-kernel@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> > Subject: Re: [EXT] Re: [PATCH V10 2/5] pwm: Add i.MX TPM PWM driver
> > support
> >
> > WARNING: This email was created outside of NXP. DO NOT CLICK links or
> > attachments unless you recognize the sender and know the content is safe.
> >
> >
> >
> > Hello,
> >
> > On Tue, Apr 09, 2019 at 08:51:48AM +0000, Anson Huang wrote:
> > > > On Tue, Mar 26, 2019 at 06:52:33AM +0000, Anson Huang wrote:
> > > > > + /* get polarity */
> > > > > + if (chan) {
> > > > > + state->polarity = chan->polarity;
> > > > > + } else {
> > > > > + /* in case no channel requested yet, return HW status */
> > > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > >hwpwm));
> > > > > + if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ==
> > > > > + PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED)
> > > > > + state->polarity = PWM_POLARITY_INVERSED;
> > > > > + else
> > > > > + /*
> > > > > + * Assume reserved values (2b00 and 2b11) to yield
> > > > > + * normal polarity.
> > > > > + */
> > > > > + state->polarity = PWM_POLARITY_NORMAL;
> > > > > + }
> > > >
> > > > What is the good reason to prefer chan->polarity over reading out
> > > > the hardware state?
> > >
> > > Reading it from DDR is faster than accessing HW register as per
> > > previous comment?
> >
> > How much time do you save here? Is it worth to complicate the function
> > for that?
>
> My intention is NOT to save much time here, it is just because that I
> remembered there was comment before to suggest using variable stored in
> DRAM instead of accessing HW register, so I am a little confused where
> should use variable and where should access HW register.
>
> Also, variable can be used directly, while reading HW register will need to
> translate the register field value to polarity.
>
> If it is better to read hardware state based on your experience, I will follow
> the suggestion.

Sorry for it, after looking into the code deeper, as there is already accessing HW
register code in case of channel NOT requested, I think your suggestion makes
more sense, I can remove all the channel private data to make code easy.

Thanks,
Anson

>
> >
> > > > > + /* get channel status */
> > > > > + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ?
> true :
> > > > > +false; }
> > > > > +
> > > > > +/* this function is supposed to be called with mutex hold */
> > > > > +static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > > > > + struct pwm_device *pwm,
> > > > > + struct pwm_state *state,
> > > > > + struct imx_tpm_pwm_param *p) {
> > > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > > + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
> > > > > + bool period_update = false;
> > > > > + bool duty_update = false;
> > > > > + u32 val, cmod, cur_prescale;
> > > > > + unsigned long timeout;
> > > > > + struct pwm_state c;
> > > > > +
> > > > > + if (state->period != tpm->real_period) {
> > > > > + /*
> > > > > + * 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 (tpm->user_count > 1)
> > > > > + return -EBUSY;
> > > > > +
> > > > > + 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;
> > > > > +
> > > > > + /* set TPM counter prescale */
> > > > > + 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:
> > > > > + * if the PWM is disabled (CMOD[1:0] = 2b00), then
> > > > > + MOD
> > register
> > > > > + * is updated when MOD register is written.
> > > > > + *
> > > > > + * if the PWM is enabled (CMOD[1:0] â 2b00), the
> > > > > + period
> > length
> > > > > + * is latched into hardware when the next period starts.
> > > > > + */
> > > > > + writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);
> > > > > + tpm->real_period = state->period;
> > > > > + period_update = true;
> > > > > + }
> > > > > +
> > > > > + pwm_imx_tpm_get_state(chip, pwm, &c);
> > > >
> > > > If you move this call above the previous if block you can use
> > > > c.period instead of tpm->real_period which is easier to follow.
> > >
> > > I think the period could be changed by the if block, so duty also be
> > > changed, need to put the .get_state here, am I right?
> >
> > As you don't use c.period below this shouldn't matter. Where does duty
> > change?
>
> The "prescale" is used during computing the duty, and it could be changed in
> period change.
>
> >
> > > > > + if (state->duty_cycle != c.duty_cycle) {
> > > > > + /*
> > > > > + * set channel value:
> > > > > + * if the PWM is disabled (CMOD[1:0] = 2b00), then CnV
> register
> > > > > + * is updated when CnV register is written.
> > > > > + *
> > > > > + * if the PWM is enabled (CMOD[1:0] â 2b00), the duty
> length
> > > > > + * is latched into hardware when the next period starts.
> > > > > + */
> > > > > + writel(p->val, tpm->base + PWM_IMX_TPM_CnV(pwm-
> > >hwpwm));
> > > > > + duty_update = true;
> > > > > + }
> > > > > +
> > > > > + /* make sure MOD & CnV registers are updated */
> > > > > + if (period_update || duty_update) {
> > > > > + timeout = jiffies + msecs_to_jiffies(tpm->real_period /
> > > > > + NSEC_PER_MSEC + 1);
> > > > > + while (readl(tpm->base + PWM_IMX_TPM_MOD) != p->mod
> > > > > + || readl(tpm->base + PWM_IMX_TPM_CnV(pwm-
> > >hwpwm))
> > > > > + != p->val) {
> > > > > + if (time_after(jiffies, timeout))
> > > > > + return -ETIME;
> > > > > + cpu_relax();
> > > > > + }
> > > > > + }
> > > >
> > > > If the PWM is running you wait in the above loop until the new
> > > > values are active but before you configure the period. I think in
> > > > the case where the PWM is active and a change of polarity is
> > > > requested it would be more correct to refuse the change.
> > >
> > > Not very understand, the period is changed at the beginning, and
> > > most of the time, period should be fixed, changing polarity should
> > > be allowed
> > even PWM is active?
> >
> > Changing polarity should be atomic (that is, get active with the next
> > period's start). As the hardware doesn't support that, claiming it does is a
> bad idea.
> >
>
> OK, that even makes driver easy, will change it in next version.
>
>
> > > That does NOT introduce too many trouble, is it a common case that
> > > dynamic changing polarity is NOT good?
> > >
> > >
> > > >
> > > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > > + val &= ~(PWM_IMX_TPM_CnSC_ELS |
> PWM_IMX_TPM_CnSC_MSA |
> > > > > + PWM_IMX_TPM_CnSC_MSB);
> > > > > + if (state->enabled) {
> > > > > + /*
> > > > > + * set polarity (for edge-aligned PWM modes)
> > > > > + *
> > > > > + * ELS[1:0] = 2b10 yields normal polarity behaviour,
> > > > > + * ELS[1:0] = 2b01 yields inversed polarity.
> > > > > + * The other values are reserved.
> > > > > + *
> > > > > + * polarity settings will enabled/disable output status
> > > > > + * immediately, so if the channel is disabled, need to
> > > > > + * make sure MSA/MSB/ELS are set to 0 which means channel
> > > > > + * disabled.
> > > >
> > > > I don't understand this comment. Either ELS = 0 is reserved or it
> > > > can be
> > used.
> > > > What is an output status?
> > >
> > > The reference manual ONLY states it as reserved, so how to add
> > > comments
> > here?
> >
> > The problem might just be, that I don't get what you intend to say in
> > the last paragraph.
>
> For the configuration, MSA/MSB = 0/1 means edge-aligned PWM mode, in
> this mode, ELS[1:0]=2b00 is reserved. But with MSA/MSB/ELS all set to 0, that
> means channel disabled.
>
> But I think you are right, putting the last paragraph into the clearing of
> MSA/MSB/ELS is better, as below:
>
> 250 /*
> 251 * polarity settings will enabled/disable output status
> 252 * immediately, so if the channel is disabled, need to
> 253 * make sure MSA/MSB/ELS are set to 0 which means channel
> 254 * disabled.
> 255 */
> 256 val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> 257 val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA |
> 258 PWM_IMX_TPM_CnSC_MSB);
> 259 if (state->enabled) {
>
>
> 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%7Ceb
> >
> 28bb96ee9844ca92a908d6bccdd40d%7C686ea1d3bc2b4c6fa92cd99c5c30163
> > 5%7C0%7C0%7C636903989547571591&sdata=tnQGymmW1zZZjcSWm
> > W40RUZiuQv0lpT2R8mj3znRmWE%3D&reserved=0 |

Next message: Christoph Hellwig: "Re: [RFC PATCH 4/5] acpi/hmat: Register special purpose memory as a device"
Previous message: Alexandre Belloni: "[PATCH 3/5] usb: gadget: udc: lpc32xx: properly setup phy interrupts"
In reply to: Anson Huang: "RE: [EXT] Re: [PATCH V10 2/5] pwm: Add i.MX TPM PWM driver support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]