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 wrote:
> Dear Boris, Stefan,
>
> > On Mon, 28 Nov 2016 06:50:31 +0100
> > Lukasz Majewski wrote:
> >
> > > Dear Stefan, Boris,
> > >
> > > > On 2016-11-23 00:38, Boris Brezillon wrote:
> > > > > On Tue, 22 Nov 2016 13:55:33 -0800
> > > > > Stefan Agner 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
> > > > >> > Suggested-by: Boris Brezillon
> > > > >> > Signed-off-by: Lukasz
> > > > >> > Majewski Reviewed-by: Boris Brezillon
> > > > >> > ---
> > > > >> > 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, 10);
> > > > >> > + period_cycles = c;
> > > > >> > +
> > > > >> > + prescale = period_cycles / 0x1 + 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, 10);
> > > > > period_cycles = c;
> > > > >
> > > > > prescale = period_cycles / 0x1 + 1;
> > > > >
> > > > > period_cycles /= prescale;
> > > > > c
Re: [PATCH v3 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2
Dear Boris, Stefan,
> On Mon, 28 Nov 2016 06:50:31 +0100
> Lukasz Majewski wrote:
>
> > Dear Stefan, Boris,
> >
> > > On 2016-11-23 00:38, Boris Brezillon wrote:
> > > > On Tue, 22 Nov 2016 13:55:33 -0800
> > > > Stefan Agner 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
> > > >> > Suggested-by: Boris Brezillon
> > > >> > Signed-off-by: Lukasz
> > > >> > Majewski Reviewed-by: Boris Brezillon
> > > >> > ---
> > > >> > 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, 10);
> > > >> > +period_cycles = c;
> > > >> > +
> > > >> > +prescale = period_cycles / 0x1 + 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, 10);
> > > > period_cycles = c;
> > > >
> > > > prescale = period_cycles / 0x1 + 1;
> > > >
> > > > period_cycles /= prescale;
> > > > c = (unsigned long long)period_cycles *
> > > > state->duty_cycle;
> > > > do_div(c, state->period);
> > > > duty_cycles = c;
> > > >
> > > > /*
> > > > * Acco
Re: [PATCH v3 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2
On Mon, 28 Nov 2016 06:50:31 +0100
Lukasz Majewski wrote:
> Dear Stefan, Boris,
>
> > On 2016-11-23 00:38, Boris Brezillon wrote:
> > > On Tue, 22 Nov 2016 13:55:33 -0800
> > > Stefan Agner 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
> > >> > Suggested-by: Boris Brezillon
> > >> > Signed-off-by: Lukasz
> > >> > Majewski Reviewed-by: Boris Brezillon
> > >> > ---
> > >> > 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, 10);
> > >> > + period_cycles = c;
> > >> > +
> > >> > + prescale = period_cycles / 0x1 + 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, 10);
> > > period_cycles = c;
> > >
> > > prescale = period_cycles / 0x1 + 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;
> > >
> > > /*
> > >
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 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
> >> > Suggested-by: Boris Brezillon
> >> > Signed-off-by: Lukasz
> >> > Majewski Reviewed-by: Boris Brezillon
> >> > ---
> >> > 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, 10);
> >> > +period_cycles = c;
> >> > +
> >> > +prescale = period_cycles / 0x1 + 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, 10);
> > period_cycles = c;
> >
> > prescale = period_cycles / 0x1 + 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);
> >
Re: [PATCH v3 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2
On 2016-11-23 00:38, Boris Brezillon wrote:
> On Tue, 22 Nov 2016 13:55:33 -0800
> Stefan Agner 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
>> > Suggested-by: Boris Brezillon
>> > Signed-off-by: Lukasz Majewski
>> > Reviewed-by: Boris Brezillon
>> > ---
>> > 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, 10);
>> > + period_cycles = c;
>> > +
>> > + prescale = period_cycles / 0x1 + 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, 10);
> period_cycles = c;
>
> prescale = period_cycles / 0x1 + 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_cycle
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 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
> > Suggested-by: Boris Brezillon
> > Signed-off-by: Lukasz Majewski
> > Reviewed-by: Boris Brezillon
> > ---
> > 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, 10);
> > + period_cycles = c;
> > +
> > + prescale = period_cycles / 0x1 + 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, 10);
period_cycles = c;
prescale = period_cycles / 0x1 + 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
Re: [PATCH v3 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2
On 2016-11-01 00:10, Lukasz Majewski wrote:
> This commit provides apply() callback implementation for i.MX's PWMv2.
>
> Suggested-by: Stefan Agner
> Suggested-by: Boris Brezillon
> Signed-off-by: Lukasz Majewski
> Reviewed-by: Boris Brezillon
> ---
> 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, 10);
> + period_cycles = c;
> +
> + prescale = period_cycles / 0x1 + 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...
--
Stefan
> +
> + writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> + /* Disable the clock if the PWM is being disabled. */
> + if (!state->enabled && cstate.enabled)
> + clk_disable_unprepare(imx->clk_per);
> +
> + return 0;
> +}
> +
> static int imx_pwm_config_v2(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> @@ -273,6 +342,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> .enable = imx_pwm_enable,
> .disable = imx_pwm_disable,
> .config = imx_pwm_config,
> + .apply = imx_pwm_apply_v2,
> .owner = THIS_MODULE,
> };
[PATCH v3 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2
This commit provides apply() callback implementation for i.MX's PWMv2.
Suggested-by: Stefan Agner
Suggested-by: Boris Brezillon
Signed-off-by: Lukasz Majewski
Reviewed-by: Boris Brezillon
---
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);
+
+ c = clk_get_rate(imx->clk_per);
+ c *= state->period;
+
+ do_div(c, 10);
+ period_cycles = c;
+
+ prescale = period_cycles / 0x1 + 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;
+
+ writel(cr, imx->mmio_base + MX3_PWMCR);
+
+ /* Disable the clock if the PWM is being disabled. */
+ if (!state->enabled && cstate.enabled)
+ clk_disable_unprepare(imx->clk_per);
+
+ return 0;
+}
+
static int imx_pwm_config_v2(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
@@ -273,6 +342,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
.enable = imx_pwm_enable,
.disable = imx_pwm_disable,
.config = imx_pwm_config,
+ .apply = imx_pwm_apply_v2,
.owner = THIS_MODULE,
};
--
2.1.4
