linux-kernel - Re: [PATCH v3 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC

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: <20201002233630.GB500800@latitude>
Date:   Sat, 3 Oct 2020 01:36:30 +0200
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Heiko Stuebner <heiko@...ech.de>, devicetree@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Sam Ravnborg <sam@...nborg.org>, linux-rtc@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Fabio Estevam <festevam@...il.com>,
        Daniel Palmer <daniel@...f.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Andreas Kemnade <andreas@...nade.info>,
        NXP Linux Team <linux-imx@....com>,
        linux-pwm@...r.kernel.org, Stephan Gerhold <stephan@...hold.net>,
        allen <allen.chen@....com.tw>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Lubomir Rintel <lkundrak@...sk>,
        Rob Herring <robh+dt@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        linux-arm-kernel@...ts.infradead.org,
        Alessandro Zummo <a.zummo@...ertech.it>,
        linux-kernel@...r.kernel.org, Mark Brown <broonie@...nel.org>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Heiko Stuebner <heiko.stuebner@...obroma-systems.com>,
        Josua Mayer <josua.mayer@....eu>,
        Shawn Guo <shawnguo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v3 4/7] pwm: ntxec: Add driver for PWM function in
 Netronix EC

On Mon, Sep 28, 2020 at 10:35:46AM +0200, Uwe Kleine-König wrote:
> Hello Jonathan,
> 
> On Sun, Sep 27, 2020 at 11:10:44PM +0200, Jonathan Neuschäfer wrote:
[...]
> > > > +	if (state->enabled && duty != 0) {
> > > > +		res = regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> > > > +		if (res)
> > > > +			return res;
> > > > +
> > > > +		/* Disable the auto-off timer */
> > > > +		res = regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> > > > +		if (res)
> > > > +			return res;
> > > > +
> > > > +		return regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> > > > +	} else {
> > > > +		return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> > > > +	}
> > > 
> > > This code is wrong for state->enabled = false.
> > 
> > Why?
> 
> Hm, I wonder the same. Probably I just misunderstood the code, sorry :-\
> 
> > > How does the PWM behave when .apply is called? Does it complete the
> > > currently running period? Can it happen that when you switch from say
> > > 
> > > 	.duty_cycle = 900 * TIME_BASE_NS (0x384)
> > > 	.period = 1800 * TIME_BASE_NS (0x708)
> > > 
> > > to
> > > 
> > > 	.duty_cycle = 300 * TIME_BASE_NS (0x12c)
> > > 	.period = 600 * TIME_BASE_NS (0x258)
> > > 
> > > that a period with
> > > 
> > > 	.duty_cycle = 388 * TIME_BASE_NS (0x184)
> > > 	.period = 1800 * TIME_BASE_NS (0x708)
> > > 	
> > > (because only NTXEC_REG_PERIOD_HIGH was written when the new period
> > > started) or something similar is emitted?
> > 
> > Changes take effect after the low byte is written, so a result like 0x184
> > in the above example should not happen.
> > 
> > When the period and duty cycle are both changed, it temporarily results
> > in an inconsistent state:
> > 
> >  - period = 1800ns, duty cycle = 900ns
> >  - period =  600ns, duty cycle = 900ns (!)
> >  - period =  600ns, duty cycle = 300ns
> 
> Does this always happen, or only if a new cycle starts at an unlucky
> moment?

Just based on thinking about the code, the register writes setting this
intermediate state would always happen, but I don't know if the state
changes are applied in the middle of a running period, or when the new
period starts, because I can't measure the signal in good enough detail
at the moment.

> > The inconsistent state of duty cycle > period is handled gracefully by
> > the EC and it outputs a 100% duty cycle, as far as I can tell.
> 
> OK.
> 
> > I currently don't have a logic analyzer / oscilloscope to measure
> > whether we get full PWM periods, or some kind of glitch when the new
> > period starts in the middle of the last one.
> 
> You can even check this with an LED using something like:
> 
> 	pwm_apply(mypwm, {.enabled = true, .duty_cycle = $big, .period = $big});
> 	pwm_apply(mypwm, {.enabled = false, ... });
> 
> . If the period is completed the LED is on for $big ns, if not the LED
> is on for a timespan that is probably hardly noticable with the human
> eye.

The longest configurable period is about 8ms, so it's not long enough to
see anything. However, after writing enable=0, it can take about a
second for the PWM signal to turn off... this hardware is a bit weird.

> > > > +}
> > > > +
> > > > +static struct pwm_ops ntxec_pwm_ops = {
> > > > +	.apply = ntxec_pwm_apply,
> > > 
> > > Please implement a .get_state() callback. And enable PWM_DEBUG during
> > > your tests.
> > 
> > The device doesn't support reading back the PWM state. What should a
> > driver do in this case?
> 
> Document it as a limitation, please.

Okay.


Thanks,
Jonathan Neuschäfer

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)