| 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: <20161123093848.206ad78f@bbrezillon>
Date: Wed, 23 Nov 2016 09:38:48 +0100
From: Boris Brezillon <boris.brezillon@...e-electrons.com>
To: Stefan Agner <stefan@...er.ch>
Cc: Lukasz Majewski <l.majewski@...ess.pl>,
Thierry Reding <thierry.reding@...il.com>,
Sascha Hauer <s.hauer@...gutronix.de>,
linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
Fabio Estevam <fabio.estevam@....com>,
Fabio Estevam <festevam@...il.com>,
Lothar Wassmann <LW@...o-electronics.de>,
Bhuvanchandra DV <bhuvanchandra.dv@...adex.com>,
kernel@...gutronix.de
Subject: Re: [PATCH v3 07/11] pwm: imx: Provide atomic PWM support for i.MX
PWMv2
On Tue, 22 Nov 2016 13:55:33 -0800
Stefan Agner <stefan@...er.ch> wrote:
> On 2016-11-01 00:10, Lukasz Majewski wrote:
> > This commit provides apply() callback implementation for i.MX's PWMv2.
> >
> > Suggested-by: Stefan Agner <stefan@...er.ch>
> > Suggested-by: Boris Brezillon <boris.brezillon@...e-electrons.com>
> > Signed-off-by: Lukasz Majewski <l.majewski@...ess.pl>
> > Reviewed-by: Boris Brezillon <boris.brezillon@...e-electrons.com>
> > ---
> > Changes for v3:
> > - Remove ipg clock enable/disable functions
> >
> > Changes for v2:
> > - None
> > ---
> > drivers/pwm/pwm-imx.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 70 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index ebe9b0c..cd53c05 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
> > }
> > }
> >
> > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + unsigned long period_cycles, duty_cycles, prescale;
> > + struct imx_chip *imx = to_imx_chip(chip);
> > + struct pwm_state cstate;
> > + unsigned long long c;
> > + u32 cr = 0;
> > + int ret;
> > +
> > + pwm_get_state(pwm, &cstate);
> > +
>
> Couldn't we do:
>
> if (cstate.enabled) { ...
>
> > + c = clk_get_rate(imx->clk_per);
> > + c *= state->period;
> > +
> > + do_div(c, 1000000000);
> > + period_cycles = c;
> > +
> > + prescale = period_cycles / 0x10000 + 1;
> > +
> > + period_cycles /= prescale;
> > + c = (unsigned long long)period_cycles * state->duty_cycle;
> > + do_div(c, state->period);
> > + duty_cycles = c;
> > +
> > + /*
> > + * according to imx pwm RM, the real period value should be
> > + * PERIOD value in PWMPR plus 2.
> > + */
> > + if (period_cycles > 2)
> > + period_cycles -= 2;
> > + else
> > + period_cycles = 0;
> > +
> > + /* Enable the clock if the PWM is being enabled. */
> > + if (state->enabled && !cstate.enabled) {
> > + ret = clk_prepare_enable(imx->clk_per);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + /*
> > + * Wait for a free FIFO slot if the PWM is already enabled, and flush
> > + * the FIFO if the PWM was disabled and is about to be enabled.
> > + */
> > + if (cstate.enabled)
> > + imx_pwm_wait_fifo_slot(chip, pwm);
> > + else if (state->enabled)
> > + imx_pwm_sw_reset(chip);
> > +
> > + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > + writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > +
> > + cr |= MX3_PWMCR_PRESCALER(prescale) |
> > + MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > + MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> > +
> > + if (state->enabled)
> > + cr |= MX3_PWMCR_EN;
>
> } else if (state->enabled) {
> imx_pwm_sw_reset(chip);
> }
>
> and get rid of the if (state->enabled) in between? This would safe us
> useless recalculation when disabling the controller...
I get your point, but I'm pretty sure your proposal does not do what
you want (remember that cstate is the current state, and state is the
new state to apply).
Something like that would work better:
if (state->enabled) {
c = clk_get_rate(imx->clk_per);
c *= state->period;
do_div(c, 1000000000);
period_cycles = c;
prescale = period_cycles / 0x10000 + 1;
period_cycles /= prescale;
c = (unsigned long long)period_cycles *
state->duty_cycle;
do_div(c, state->period);
duty_cycles = c;
/*
* According to imx pwm RM, the real period value
* should be PERIOD value in PWMPR plus 2.
*/
if (period_cycles > 2)
period_cycles -= 2;
else
period_cycles = 0;
/*
* Enable the clock if the PWM is not already
* enabled.
*/
if (!cstate.enabled) {
ret = clk_prepare_enable(imx->clk_per);
if (ret)
return ret;
}
/*
* Wait for a free FIFO slot if the PWM is already
* enabled, and flush the FIFO if the PWM was disabled
* and is about to be enabled.
*/
if (cstate.enabled)
imx_pwm_wait_fifo_slot(chip, pwm);
else
imx_pwm_sw_reset(chip);
writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
writel(period_cycles, imx->mmio_base + MX3_PWMPR);
writel(MX3_PWMCR_PRESCALER(prescale) |
MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
MX3_PWMCR_EN,
imx->mmio_base + MX3_PWMCR);
} else {
writel(0, imx->mmio_base + MX3_PWMCR);
/* Disable the clock if the PWM is currently enabled. */
if (cstate.enabled)
clk_disable_unprepare(imx->clk_per);
}
This being said, I'm a bit concerned by the way this driver handles PWM
config requests.
It seems that the new config request is queued, and nothing guarantees
that it is actually applied when the pwm_apply/config/enable/disable()
functions return.
This approach has several flaws IMO:
1/ I'm not sure this is what the PWM users expect. Getting your request
queued with no guarantees that it is applied can be weird in some
cases (especially when the user changes the PWM config several times
in a short period of time).
2/ In the disable path, you queue a "stop PWM" request, but you're not
sure that it's actually dequeued before the per clk is disabled.
What happens in that case? And more importantly, what happens when
the PWM is re-enabled to apply a new config? AFAICS, there might be
a short period of time where the re-enabled PWM is actually running
with the old config until we flush the command queue and queue a new
command.
3/ The queueing approach complicates the whole logic. You have to
flush the FIFO in some cases, or wait for an empty slots if too many
commands are queued.
Forcing imx_pwm_apply_v2() to wait for the config request to be
applied should simplify the whole thing, because you will always be
guaranteed that the FIFO is empty, and that the current
configuration is the last requested one.
Regards,
Boris
Powered by blists - more mailing lists