RE: [PATCH V4 2/5] pwm: Add i.MX TPM PWM driver support
From: Anson Huang
Date: Mon Mar 18 2019 - 10:04:14 EST
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Anson Huang
> Sent: 2019å3æ18æ 18:08
> 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; 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 V4 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æ18æ 16:08
> > 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;
> > 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 V4 2/5] pwm: Add i.MX TPM PWM driver support
> >
> > On Mon, Mar 18, 2019 at 07:41:02AM +0000, Anson Huang wrote:
> > > Hi,Uwe
> > > > > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > >
> > > > As this interrupts the output, please only do it if necessary.
> > >
> > > OK, will do it ONLY when it is enabled previously.
> >
> > I think you only need to do that when the value actually changes.
>
> OK, I will save the period/div count and ONLY do it when the value actually
> changes.
After further think, I added below tpm->period to save the current period settings,
ONLY when the new period to be set is different from the current period, then the
pwm_imx_tpm_config_counter() is called, so I think no need to care about the value
changes, the value is always changed when pwm_imx_tpm_config_counter() is called.
if (tpm->user_count != 1 && state->period != tpm->period)
return -EBUSY;
ret = pwm_imx_tpm_config_counter(chip, state->period);
if (ret)
return ret;
>
> >
> > > > > + /* set duty counter */
> > > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > > > > +PWM_IMX_TPM_MOD_MOD_MASK;
> > > >
> > > > I recommend storing this value in driver data.
> > >
> > > NOT quite understand, as we did NOT use it in other places except
> > > the get_state, just reading the register once should be OK there.
> >
> > I had the impression it is used more than once. Will look again in the
> > next iteration. But also note that shadowing the value might already
> > be beneficial for a single call site as driver data might occupy more
> > RAM than necessary anyhow and reading from RAM is faster than from the
> hardware's register.
> > Probably this is not a fast path, so not worth the optimisation?!
>
> OK, will save it in driver data and avoid accessing register again.
>
> >
> > > > I wonder why MSA and MSB are two bits instead of making this a
> > > > field of width 2 with 2b10 meaning PWM mode. But maybe it's just
> > > > me not understanding the independent semantic of these two bits?
> > >
> > > I think making them a field makes more sense, but anyway we just
> > > follow the RM.
> >
> > If it makes the driver easier to understand (which I think it does)
> > feel free to derivate from the RM. Just add a comment to the definition,
> then it's fine.
>
> OK, I will change the register definition and a comment for it.
>
> >
> > > > Reading the reference manual I'd say in PWM mode the semantic of
> > > > ELSA and ELSB is:
> > > >
> > > > On counter reload set the output to ELSB
> > > > On counter match set the output to ELSA
> > > >
> > > > Noting that in a comment would ease understanding the code here.
> > >
> > > I added below comment for PWM modes:
> > >
> > > 137 /*
> > > 138 * set polarity (for edge-aligned PWM modes)
> > > 139 *
> > > 140 * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > > 141 * 0 10 10 PWM High-true pulse
> > > 142 * 0 10 00 PWM Reserved
> > > 143 * 0 10 01 PWM Low-true pulse
> > > 144 * 0 10 11 PWM Reserved
> > > 145 *
> > > 146 * High-true pulse: clear output on counter match, set output on
> > > 147 * counter reload, set output when counter first enabled or
> paused.
> > > 148 *
> > > 149 * Low-true pulse: set output on counter match, clear output on
> > > 150 * counter reload, clear output when counter first enabled or
> > paused.
> > > 151 */
> >
> > I stumbled over "high-true" and "low-true" in the RM, too. In my
> > bubble this is an uncommon wording. I'd write instead:
> >
> > /*
> > * set polarity
> > *
> > * ELSB:ELSA = 2b10 yields normal polarity behaviour, ELSB:ELSA
> > * = 2b01 yields inversed polarity. The other values are
> > * reserved.
> > */
> >
> > And don't write about CPWM, MSA and MSB which are always used with
> > fixed values anyhow in the driver.
> >
>
> OK.
>
> > > > > + /* disable channel */
> > > > > + writel(PWM_IMX_TPM_CnSC_CHF,
> > > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > > >
> > > > Clearing CHF doens't disable the channel as I read the manual.
> > >
> > > This write clears CHF as well as writing other bits 0, to disable
> > > the output. Maybe I can explicitly clear MSA/MSB/ELSA/ELSB to avoid
> > confusion.
> >
> > Ah, I misinterpreted the value written.
> >
> > > > > +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 pwm_state curstate;
> > > > > + u32 duty_cycle = state->duty_cycle;
> > > > > + int ret;
> > > > > +
> > > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > > > > +
> > > > > + mutex_lock(&tpm->lock);
> > > >
> > > > What should this lock protect? Does it hurt if the state changes
> > > > between pwm_imx_tpm_get_state releasing the lock and
> > > > pwm_imx_tpm_apply taking it?
> > >
> > > The idea is to protect the share resourced by multiple channels, but
> > > I think I can make the mutex_lock includes get_state and remove the
> > > lock in
> > get_state function.
> > A
> > You might need it in .get_state to return a consistent state to the
> > caller. In this case just introduce an unlocked variant of .get_state
> > to share code between the two functions.
> >
> > And BTW the question was honest. I'm not entirely sure that you need
> > to hold the lock.
>
> Agreed, if the different channel configuration ONLY access its own register,
> NOT any shared registers, then I think this lock is unnecessary.
Since all the functions in .apply function will need to access registers and these
registers are shared by different channels, so I think the lock is necessary.
Anson.
>
> >
> > > > > + */
> > > > > + if (tpm->user_count != 1)
> > > > > + return -EBUSY;
> > > >
> > > > Ideally if say period = 37 is requested but currently we have
> > > > period =
> > > > 36 and configuring 37 would result in 36 anyhow, don't return EBUSY.
> > >
> > > I think here the protection is just for making sure that is there
> > > are multiple users, period can NOT be changed, since all channels
> > > will be
> > impacted.
> >
> > I think you misunderstood what I intended to say here.
> >
> > Consider that in the case that there is only a single PWM in use
> > configuring for a period of 37 ns actually yields 36 ns because the
> > hardware cannot provide 37 ns and 36 ns is the best match.
> >
> > Then if a second user comes and requests 37 ns, the result here is,
> > that the second user gets the -EBUSY. This is ridiculous however
> > because the request is denied even though the period is already
> > configured for the length that would be configured if the second user were
> the only one.
>
> Ah, if I understand correctly, I think I can change it to, if second user try to set
> a period different from first user, then return -EBUSY, otherwise, return
> success.
>
> >
> > > > > + tpm->chip.dev = &pdev->dev;
> > > > > + tpm->chip.ops = &imx_tpm_pwm_ops;
> > > > > + tpm->chip.base = -1;
> > > > > + tpm->chip.npwm = PWM_IMX_TPM_MAX_CHANNEL_NUM;
> > > >
> > > > This is wrong, as some only have 2 channels?
> > >
> > > I saw we can get channel number from register, will read register to
> > > determine the channel number, but for the channel config and status
> > > saved in struct, I will still use the MAX channel number to define the array.
> >
> > Yeah, that is sensible.
>
> I will resend V5 patch set, since there are some mis-understanding for
> previous comments.
>
> 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%7C31
> >
> d10d3f4a7c46eea9d708d6ab78d965%7C686ea1d3bc2b4c6fa92cd99c5c30163
> >
> 5%7C0%7C0%7C636884932859808816&sdata=LD2D%2BwEhNKFllK2FaI
> > Wvjttmre0YPV%2BXwv7sdvkZSpo%3D&reserved=0 |