| 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
| ||
|
Date: Thu, 5 Jan 2017 10:35:05 +0100
From: Lukasz Majewski <lukma@...x.de>
To: Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc: Thierry Reding <thierry.reding@...il.com>,
Sascha Hauer <s.hauer@...gutronix.de>,
Stefan Agner <stefan@...er.ch>, linux-pwm@...r.kernel.org,
Bhuvanchandra DV <bhuvanchandra.dv@...adex.com>,
linux-kernel@...r.kernel.org,
Lothar Wassmann <LW@...o-electronics.de>,
kernel@...gutronix.de, Fabio Estevam <fabio.estevam@....com>,
Lukasz Majewski <l.majewski@...ess.pl>
Subject: Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX
PWMv2
On Thu, 5 Jan 2017 10:19:35 +0100
Boris Brezillon <boris.brezillon@...e-electrons.com> wrote:
> On Thu, 5 Jan 2017 10:03:47 +0100
> Lukasz Majewski <lukma@...x.de> wrote:
>
> > On Thu, 5 Jan 2017 09:50:35 +0100
> > Boris Brezillon <boris.brezillon@...e-electrons.com> wrote:
> >
> > > On Thu, 5 Jan 2017 00:36:50 +0100
> > > Lukasz Majewski <lukma@...x.de> 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 v4:
> > > > - Avoid recalculation of PWM parameters when disabling PWM
> > > > signal
> > > > - Unconditionally call clk_prepare_enable(imx->clk_per) and
> > > > clk_disable_unprepare(imx->clk_per)
> > >
> > > I don't see that one, but I'm not sure this is actually needed.
> > > In the enabled path we enable the clk before accessing the
> > > registers, and in the disable path, assuming you change the code
> > > according to my suggestion, the clk should already be enabled
> > > when you write to MX3_PWMCR.
> > >
> > > >
> > > > Changes for v3:
> > > > - Remove ipg clock enable/disable functions
> > > >
> > > > Changes for v2:
> > > > - None
> > > > ---
> > > > drivers/pwm/pwm-imx.c | 67
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > changed, 67 insertions(+)
> > > >
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index 60cdc5c..134dd66 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip
> > > > *chip, return ret;
> > > > }
> > > >
> > > > +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;
> > > > + int ret;
> > > > +
> > > > + pwm_get_state(pwm, &cstate);
> > > > +
> > > > + 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;
> > > > +
> > >
> > > Starting here...
> > >
> > > > + 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);
> > >
> > > ... till here, should be replaced by:
> > >
> > >
> > > /*
> > > * 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 {
> > > ret = clk_prepare_enable(imx->clk_per);
> > > if (ret)
> > > return ret;
> >
> > In the other mail you mentioned that the clock should be enabled
> > unconditionally to fix issue on iMX7 when we want to access
> > registers.
>
> When I said unconditionally, I meant call
> clk_prepare_enable(imx->clk_per) just after pwm_get_state() (at the
> beginning of the function) and call clk_disable_(imx->clk_per) just
> before returning 0 at the end of the function. This way you're
> guaranteed that the clk is always enabled when you access registers in
> between. Of course, you still need the the clk_prepare_enable() and
> clk_disable_unprepare() in the enable and disable path to keep the clk
> enabled when the PWM is enabled, and to disable it when the clk is
> being disabled.
Why you didn't said this when I pasted pseudo code to the other e-mail
and explicitly asked if this is correct? By doing that I wanted to
avoid situations like this (so I will have to repost this again and
again .....).
>
> But, while reviewing your patch I realized this was actually unneeded
> (see the explanation in my previous review).
>
> >
> > Now it depends on cstate.enabled flag.
> >
> > So we end up with
> >
> > if (state.enabled && !cstate.enabled)
> > clk_preapre_enable();
>
> Yep, and that's correct.
And following patch:
http://patchwork.ozlabs.org/patch/709510/
address this issue.
>
> >
> > which was the reason of iMX7 failure reported by Stefan to v3.
>
> Stefan, do you confirm that? I don't see how this can possibly fail
> since the clk is either already enabled (cstate.enabled) or we enable
> it (!cstate.enable) before accessing registers.
http://patchwork.ozlabs.org/patch/709510/
>
> What's for sure is that your implementation is introducing possible
> unbalanced ref counting on the per clk.
You mean when somebody call twice ->apply() with "state->enabled".
>
> >
> >
> > >
> > > imx_pwm_sw_reset(chip);
> > > }
> > >
> > > Otherwise you keep incrementing the prepare/enable counters of
> > > the clk when you change the config of an already enabled PWM.
> > >
> > > > +
> > > > + 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 {
> > >
> > > } else if (cstate.enabled) {
> > >
> > > Again, this is to avoid unbalanced prepare/enable counters on the
> > > per clk if the PWM config is changed several times when the PWM is
> > > disabled.
> > >
> > > > + writel(0, imx->mmio_base + MX3_PWMCR);
> > > > +
> > > > + clk_disable_unprepare(imx->clk_per);
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static int imx_pwm_enable(struct pwm_chip *chip, struct
> > > > pwm_device *pwm) {
> > > > struct imx_chip *imx = to_imx_chip(chip);
> > > > @@ -280,6 +346,7 @@ static struct pwm_ops imx_pwm_ops_v1 = {
> > > > };
> > > >
> > > > static struct pwm_ops imx_pwm_ops_v2 = {
> > > > + .apply = imx_pwm_apply_v2,
> > > > .enable = imx_pwm_enable,
> > > > .disable = imx_pwm_disable,
> > > > .config = imx_pwm_config,
> > >
> >
> >
> >
> >
> > Best regards,
> >
> > Lukasz Majewski
> >
> > --
> >
> > DENX Software Engineering GmbH, Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
> > wd@...x.de
>
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@...x.de
Powered by blists - more mailing lists