Message-ID: <DB3PR0402MB39164E87690B928DDBB6FDBBF52D0@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Tue, 9 Apr 2019 12:04:16 +0000
From:   Anson Huang <anson.huang@....com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
CC:     "mark.rutland@....com" <mark.rutland@....com>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        Robin Gong <yibin.gong@....com>,
        "schnitzeltony@...il.com" <schnitzeltony@...il.com>,
        "otavio@...ystems.com.br" <otavio@...ystems.com.br>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "festevam@...il.com" <festevam@...il.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "stefan@...er.ch" <stefan@...er.ch>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        Leonard Crestez <leonard.crestez@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        dl-linux-imx <linux-imx@....com>
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@...gutronix.de]
> Sent: 2019年4月9日 17:29
> To: Anson Huang <anson.huang@....com>
> Cc: mark.rutland@....com; linux-pwm@...r.kernel.org; Robin Gong
> <yibin.gong@....com>; schnitzeltony@...il.com;
> otavio@...ystems.com.br; devicetree@...r.kernel.org;
> festevam@...il.com; s.hauer@...gutronix.de; linux@...linux.org.uk;
> robh+dt@...nel.org; linux-kernel@...r.kernel.org;
> thierry.reding@...il.com; stefan@...er.ch; kernel@...gutronix.de;
> Leonard Crestez <leonard.crestez@....com>; shawnguo@...nel.org; linux-
> arm-kernel@...ts.infradead.org; dl-linux-imx <linux-imx@....com>
> 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.

> 
> > > > +     /* 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&amp;data=02%7C01%7Canson.huang%40nxp.com%7Ceb
> 28bb96ee9844ca92a908d6bccdd40d%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636903989547571591&amp;sdata=tnQGymmW1zZZjcSWm
> W40RUZiuQv0lpT2R8mj3znRmWE%3D&amp;reserved=0  |

Powered by blists - more mailing lists