linux-kernel - Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190314100042.fl4kdwn7awcw4aov@pengutronix.de>
Date:   Thu, 14 Mar 2019 11:00:42 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Anson Huang <anson.huang@....com>
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>,
        "stefan@...er.ch" <stefan@...er.ch>,
        "otavio@...ystems.com.br" <otavio@...ystems.com.br>,
        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 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.

> > > +	/* 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.

> > > [...]
> > > +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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |