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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux