| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DB3PR0402MB391627A8C741C84A2057E576F5470@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date: Mon, 18 Mar 2019 10:08:01 +0000
From: Anson Huang <anson.huang@....com>
To: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
CC: "thierry.reding@...il.com" <thierry.reding@...il.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"festevam@...il.com" <festevam@...il.com>,
"linux@...linux.org.uk" <linux@...linux.org.uk>,
"otavio@...ystems.com.br" <otavio@...ystems.com.br>,
"stefan@...er.ch" <stefan@...er.ch>,
Leonard Crestez <leonard.crestez@....com>,
"schnitzeltony@...il.com" <schnitzeltony@...il.com>,
"jan.tuerk@...rion.com" <jan.tuerk@...rion.com>,
Robin Gong <yibin.gong@....com>,
"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
dl-linux-imx <linux-imx@....com>
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@...gutronix.de]
> Sent: 2019年3月18日 16:08
> To: Anson Huang <anson.huang@....com>
> Cc: thierry.reding@...il.com; robh+dt@...nel.org; mark.rutland@....com;
> shawnguo@...nel.org; s.hauer@...gutronix.de; kernel@...gutronix.de;
> festevam@...il.com; linux@...linux.org.uk; otavio@...ystems.com.br;
> stefan@...er.ch; Leonard Crestez <leonard.crestez@....com>;
> schnitzeltony@...il.com; jan.tuerk@...rion.com; Robin Gong
> <yibin.gong@....com>; linux-pwm@...r.kernel.org;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; dl-linux-imx <linux-imx@....com>
> 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.
>
> > > > + /* 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.
>
> > > > + */
> > > > + 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 |
Powered by blists - more mailing lists