RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
From: Anson Huang
Date: Thu Mar 14 2019 - 10:25:41 EST
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-KÃnig [mailto:u.kleine-koenig@xxxxxxxxxxxxxx]
> Sent: 2019å3æ14æ 18:01
> 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; stefan@xxxxxxxx;
> otavio@xxxxxxxxxxxxxxxx; Leonard Crestez <leonard.crestez@xxxxxxx>;
> schnitzeltony@xxxxxxxxx; jan.tuerk@xxxxxxxxxxx; 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 V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> Hello,
>
> On Thu, Mar 14, 2019 at 09:49:06AM +0000, Anson Huang wrote:
> > > On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > > > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + struct pwm_state *state) {
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + static bool tpm_cnt_initialized;
> > > > + unsigned int duty_cnt;
> > > > + u32 val;
> > > > + u64 tmp;
> > > > +
> > > > + /*
> > > > + * TPM counter is shared by multi channels, let's make it to be
> > > > + * ONLY first channel can config TPM counter's precale and period
> > > > + * count.
> > > > + */
> > > > + if (!tpm_cnt_initialized) {
> > > > + imx_tpm_pwm_config_counter(chip, state->period);
> > > > + tpm_cnt_initialized = true;
> > > > + }
> > >
> > > So the period can only be configured once. That is not as good as it could
> be.
> > > You should allow a change whenever there is exactly one PWM in use.
> >
> > OK, maybe I can add check for other channels' statue here, and allow
> > the period update if ONLY 1 channel is enabled.
>
> See how the SiFive patch that I already pointed out solves this same problem.
Thanks, I will add a user_count to determine how many channels are used.
>
> > > > + /* set duty counter */
> > > > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > > > + tmp *= state->duty_cycle;
> > > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> > >
> > > Uah, you use state->period here even though for the 2nd PWM the
> > > Divider might not be setup appropriately.
> >
> > I think that is 1 limitation here, the dts should make sure the period
> > used for different channels are same or at least they can share same
> > divider, otherwise, what if multiple channels can NOT find a divider
> > good for every channel? How to deal with this case?
>
> You should return -ERANGE or -EINVAL for the calls that cannot be satisfied.
In my latest implementation, I have added the period setting check at the beginning
of the .apply, as the limitation states, all channels should use same period settings, so
the period here is same for all channels, once it is different, the .apply call will return failed
and the duty cycle setting will be skipped as well.
232 if (state->period != curstate.period) {
233 /*
234 * TPM counter is shared by multiple channels, so the prescale
235 * and period can NOT be modified when any other channel is used.
236 */
237 if (tpm->user_count != 1)
238 return -EBUSY;
239 ret = imx_tpm_pwm_config_counter(chip, state->period);
240 if (ret)
241 return ret;
242 }
243
244 if (!state->enabled)
245 duty_cycle = 0;
246
247 if (state->duty_cycle != curstate.duty_cycle ||
248 state->polarity != curstate.polarity)
249 imx_tpm_pwm_config(chip, pwm,
250 state->period, duty_cycle, state->polarity);
>
> > > > [...]
> > > > +static int imx_tpm_pwm_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 pwm_state curstate;
> > > > + unsigned long flags;
> > > > +
> > > > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > > > +
> > > > + spin_lock_irqsave(&tpm->lock, flags);
> > > > +
> > > > + if (state->period != curstate.period ||
> > > > + state->duty_cycle != curstate.duty_cycle ||
> > > > + state->polarity != curstate.polarity)
> > > > + imx_tpm_pwm_config(chip, pwm, state);
> > > > +
> > > > + if (state->enabled != curstate.enabled)
> > > > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
> > >
> > > This is wrong. This sequence:
> > >
> > > pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> > > true });
> > > pwm_apply_state(pwm, { .duty_cycle = 10000, .period = 10000,
> > > .enabled = false });
> > >
> > > must keep the output pin low which isn't guaranteed here.
> >
> > So you mean for every .apply operation, the channel MUST be disabled
> > first, then config it, then enable it?
>
> No. I only say that you should not configure the new period and duty cycle if
> in the end the hardware should be disabled. Always disabling is wrong, too.
>
Understand now, I will check the state->enable first, if it is false, I will set the duty_cycle
to 0 before configuring the channel.
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%7C6f3
> 0bb40fb9d460cd73a08d6a863f4c9%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636881544600449875&sdata=SRBTn%2BcquzRRRLrzOTG
> vlC5WNvof28J0%2B77iSxtFVYA%3D&reserved=0 |