by Philipp Zabel

[permalink] [raw]

Subject: Re: [PATCH v3 02/11] pwm: imx: remove ipg clock

Am Dienstag, den 01.11.2016, 08:10 +0100 schrieb Lukasz Majewski:
> From: Sascha Hauer <[email protected]>
>
> The use of the ipg clock was introduced with commit 7b27c160c681
> ("pwm: i.MX: fix clock lookup").
> In the commit message it was claimed that the ipg clock is enabled for
> register accesses. This is true for the ->config() callback, but not
> for the ->set_enable() callback. Given that the ipg clock is not
> consistently enabled for all register accesses we can assume that either
> it is not required at all or that the current code does not work.
> Remove the ipg clock code for now so that it's no longer in the way of
> refactoring the driver.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Cc: Philipp Zabel <[email protected]>

I don't remember the details, but since I had only worked with i.MX53
and i.MX6 at the time, and Sascha now verified that the i.MX53 PWM
registers can in fact be accessed with the pwmX_ipg_clk bits gated, I
can only assume that this patch is the result of a misinterpretation of
the i.MX53 technical reference manual: Contrary to the i.MX6 TRM it does
not mention the ungated peripheral access clock (ipg_clk_s) at all, but
calls the gated ipg_clk "block interface clock" in Table 18-3.

Acked-by: Philipp Zabel <[email protected]>

regards
Philipp

2016-11-08 22:24:44

On 2016-11-23 00:38, Boris Brezillon wrote:
> On Tue, 22 Nov 2016 13:55:33 -0800
> Stefan Agner <[email protected]> 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 <[email protected]>
>> > Suggested-by: Boris Brezillon <[email protected]>
>> > Signed-off-by: Lukasz Majewski <[email protected]>
>> > Reviewed-by: Boris Brezillon <[email protected]>
>> > ---
>> > 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) {

Oops, yes, got that wrong. state->enabled is what I meant.

> 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

Not sure if that is true. The RM says: "A change in the period value due
to a write in PWM_PWMPR results in the counter being reset to zero and
the start of a new count period."

And for PWMSAR: "When a new value is written, the duty cycle changes
after the current period is over."

So I guess writing the period basically makes sure the next value from
PWMSAR will be active immediately...

> that it is actually applied when the pwm_apply/config/enable/disable()
> functions return.

Given that the driver did it like that since quite some time, I am
assuming it mostly works in practice.

I would rather prefer to do that conversion to atomic PWM API now, and
fix that in a second step...

>
> 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.
>

--
Stefan

2016-11-28 05:59:09

by Lukasz Majewski

[permalink] [raw]

Subject: Re: [PATCH v3 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

Dear Stefan, Boris,

> On 2016-11-23 00:38, Boris Brezillon wrote:
> > On Tue, 22 Nov 2016 13:55:33 -0800
> > Stefan Agner <[email protected]> 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 <[email protected]>
> >> > Suggested-by: Boris Brezillon
> >> > <[email protected]> Signed-off-by: Lukasz
> >> > Majewski <[email protected]> Reviewed-by: Boris Brezillon
> >> > <[email protected]> ---
> >> > 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) {
>
> Oops, yes, got that wrong. state->enabled is what I meant.
>
> > 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
>
> Not sure if that is true. The RM says: "A change in the period value
> due to a write in PWM_PWMPR results in the counter being reset to
> zero and the start of a new count period."
>
> And for PWMSAR: "When a new value is written, the duty cycle changes
> after the current period is over."
>
> So I guess writing the period basically makes sure the next value from
> PWMSAR will be active immediately...
>
>
> > that it is actually applied when the
> > pwm_apply/config/enable/disable() functions return.
>
>
> Given that the driver did it like that since quite some time, I am
> assuming it mostly works in practice.
>
> I would rather prefer to do that conversion to atomic PWM API now, and
> fix that in a second step...

I'm also for fixing one problem in a time. The "PWM ->apply()" set of
patches now tries to fix all problems in IMX PWM driver.

Could we agree on the scope of this work? I mean what should be
included to "->apply()" rework and what will be fixed latter?

Frankly, I think that this patch series comes to the point where it is
not manageable anymore.

Please also keep in mind that I do have iMX6q system, Stefan has imx7
and Sasha has HW with PWMv1 working.

Changing the driver in N different places not related to the
"->apply()" atomicity support (the ipg clock, FIFO) requires far more
work and testing.

Best regards,
Łukasz Majewski

>
> >
> > 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.
> >
>
> --
> Stefan

Attachments:

(No filename) (181.00 B)
OpenPGP digital signature

2016-11-28 06:39:09

Dear Boris, Stefan,

> On Mon, 28 Nov 2016 06:50:31 +0100
> Lukasz Majewski <[email protected]> wrote:
>
> > Dear Stefan, Boris,
> >
> > > On 2016-11-23 00:38, Boris Brezillon wrote:
> > > > On Tue, 22 Nov 2016 13:55:33 -0800
> > > > Stefan Agner <[email protected]> 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 <[email protected]>
> > > >> > Suggested-by: Boris Brezillon
> > > >> > <[email protected]> Signed-off-by: Lukasz
> > > >> > Majewski <[email protected]> Reviewed-by: Boris Brezillon
> > > >> > <[email protected]> ---
> > > >> > 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) {
> > >
> > > Oops, yes, got that wrong. state->enabled is what I meant.
> > >
> > > > 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
> > >
> > > Not sure if that is true. The RM says: "A change in the period
> > > value due to a write in PWM_PWMPR results in the counter being
> > > reset to zero and the start of a new count period."
> > >
> > > And for PWMSAR: "When a new value is written, the duty cycle
> > > changes after the current period is over."
> > >
> > > So I guess writing the period basically makes sure the next value
> > > from PWMSAR will be active immediately...
> > >
> > >
> > > > that it is actually applied when the
> > > > pwm_apply/config/enable/disable() functions return.
> > >
> > >
> > > Given that the driver did it like that since quite some time, I am
> > > assuming it mostly works in practice.
> > >
> > > I would rather prefer to do that conversion to atomic PWM API
> > > now, and fix that in a second step...
> >
> > I'm also for fixing one problem in a time. The "PWM ->apply()" set
> > of patches now tries to fix all problems in IMX PWM driver.
> >
> > Could we agree on the scope of this work? I mean what should be
> > included to "->apply()" rework and what will be fixed latter?
>
> I never asked to fix that in this series ;-), I was just pointing the
> weird behavior of the existing code.
>
> Let's focus on the atomic support for now.

So Boris, you don't have any comments to the atomic support patches? :-)

Stefan, do you require to change the ipg stuff in the atomic series or
could it be done as a subsequent patch?

If you don't have any more questions, I will prepare next patch
iteration according to Stefan comments.

Best regards,
Łukasz Majewski

>
> >
> > Frankly, I think that this patch series comes to the point where it
> > is not manageable anymore.
> >
> > Please also keep in mind that I do have iMX6q system, Stefan has
> > imx7 and Sasha has HW with PWMv1 working.
> >
> > Changing the driver in N different places not related to the
> > "->apply()" atomicity support (the ipg clock, FIFO) requires far
> > more work and testing.
> >
> >
> > Best regards,
> > Łukasz Majewski
> >
> > >
> > > >
> > > > 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.
> > > >
> > >
> > > --
> > > Stefan
> >
>

Attachments:

(No filename) (181.00 B)
OpenPGP digital signature

2016-11-29 08:24:19

by Boris Brezillon

[permalink] [raw]

Subject: Re: [PATCH v3 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

On Mon, 28 Nov 2016 21:48:57 +0100
Lukasz Majewski <[email protected]> wrote:

> Dear Boris, Stefan,
>
> > On Mon, 28 Nov 2016 06:50:31 +0100
> > Lukasz Majewski <[email protected]> wrote:
> >
> > > Dear Stefan, Boris,
> > >
> > > > On 2016-11-23 00:38, Boris Brezillon wrote:
> > > > > On Tue, 22 Nov 2016 13:55:33 -0800
> > > > > Stefan Agner <[email protected]> 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 <[email protected]>
> > > > >> > Suggested-by: Boris Brezillon
> > > > >> > <[email protected]> Signed-off-by: Lukasz
> > > > >> > Majewski <[email protected]> Reviewed-by: Boris Brezillon
> > > > >> > <[email protected]> ---
> > > > >> > 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) {
> > > >
> > > > Oops, yes, got that wrong. state->enabled is what I meant.
> > > >
> > > > > 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
> > > >
> > > > Not sure if that is true. The RM says: "A change in the period
> > > > value due to a write in PWM_PWMPR results in the counter being
> > > > reset to zero and the start of a new count period."
> > > >
> > > > And for PWMSAR: "When a new value is written, the duty cycle
> > > > changes after the current period is over."
> > > >
> > > > So I guess writing the period basically makes sure the next value
> > > > from PWMSAR will be active immediately...
> > > >
> > > >
> > > > > that it is actually applied when the
> > > > > pwm_apply/config/enable/disable() functions return.
> > > >
> > > >
> > > > Given that the driver did it like that since quite some time, I am
> > > > assuming it mostly works in practice.
> > > >
> > > > I would rather prefer to do that conversion to atomic PWM API
> > > > now, and fix that in a second step...
> > >
> > > I'm also for fixing one problem in a time. The "PWM ->apply()" set
> > > of patches now tries to fix all problems in IMX PWM driver.
> > >
> > > Could we agree on the scope of this work? I mean what should be
> > > included to "->apply()" rework and what will be fixed latter?
> >
> > I never asked to fix that in this series ;-), I was just pointing the
> > weird behavior of the existing code.
> >
> > Let's focus on the atomic support for now.
>
> So Boris, you don't have any comments to the atomic support patches? :-)

Nope.

>
> Stefan, do you require to change the ipg stuff in the atomic series or
> could it be done as a subsequent patch?
>
> If you don't have any more questions, I will prepare next patch
> iteration according to Stefan comments.
>
> Best regards,
> Łukasz Majewski