| Message ID | 1483573014-13185-8-git-send-email-lukma@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 5 Jan 2017 00:36:50 +0100 Lukasz Majewski <lukma@denx.de> wrote: > This commit provides apply() callback implementation for i.MX's PWMv2. > > Suggested-by: Stefan Agner <stefan@agner.ch> > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > Reviewed-by: Boris Brezillon <boris.brezillon@free-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; 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, -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 5 Jan 2017 09:50:35 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Thu, 5 Jan 2017 00:36:50 +0100 > Lukasz Majewski <lukma@denx.de> wrote: > > > This commit provides apply() callback implementation for i.MX's > > PWMv2. > > > > Suggested-by: Stefan Agner <stefan@agner.ch> > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > Reviewed-by: Boris Brezillon <boris.brezillon@free-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. Now it depends on cstate.enabled flag. So we end up with if (state.enabled && !cstate.enabled) clk_preapre_enable(); which was the reason of iMX7 failure reported by Stefan to v3. > > 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@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 5 Jan 2017 10:03:47 +0100 Lukasz Majewski <lukma@denx.de> wrote: > On Thu, 5 Jan 2017 09:50:35 +0100 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > On Thu, 5 Jan 2017 00:36:50 +0100 > > Lukasz Majewski <lukma@denx.de> wrote: > > > > > This commit provides apply() callback implementation for i.MX's > > > PWMv2. > > > > > > Suggested-by: Stefan Agner <stefan@agner.ch> > > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > > Reviewed-by: Boris Brezillon <boris.brezillon@free-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. 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. > > 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. What's for sure is that your implementation is introducing possible unbalanced ref counting on the per clk. > > > > > > 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@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 5 Jan 2017 10:19:35 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Thu, 5 Jan 2017 10:03:47 +0100 > Lukasz Majewski <lukma@denx.de> wrote: > > > On Thu, 5 Jan 2017 09:50:35 +0100 > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > > > On Thu, 5 Jan 2017 00:36:50 +0100 > > > Lukasz Majewski <lukma@denx.de> wrote: > > > > > > > This commit provides apply() callback implementation for i.MX's > > > > PWMv2. > > > > > > > > Suggested-by: Stefan Agner <stefan@agner.ch> > > > > Suggested-by: Boris Brezillon > > > > <boris.brezillon@free-electrons.com> Signed-off-by: Lukasz > > > > Majewski <l.majewski@majess.pl> Reviewed-by: Boris Brezillon > > > > <boris.brezillon@free-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@denx.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@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 5 Jan 2017 10:35:05 +0100 Lukasz Majewski <lukma@denx.de> wrote: > On Thu, 5 Jan 2017 10:19:35 +0100 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > On Thu, 5 Jan 2017 10:03:47 +0100 > > Lukasz Majewski <lukma@denx.de> wrote: > > > > > On Thu, 5 Jan 2017 09:50:35 +0100 > > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > > > > > On Thu, 5 Jan 2017 00:36:50 +0100 > > > > Lukasz Majewski <lukma@denx.de> wrote: > > > > > > > > > This commit provides apply() callback implementation for i.MX's > > > > > PWMv2. > > > > > > > > > > Suggested-by: Stefan Agner <stefan@agner.ch> > > > > > Suggested-by: Boris Brezillon > > > > > <boris.brezillon@free-electrons.com> Signed-off-by: Lukasz > > > > > Majewski <l.majewski@majess.pl> Reviewed-by: Boris Brezillon > > > > > <boris.brezillon@free-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 .....). Because I thought the unconditional clk_enable/disable was not part of your example and would be added later, and I must admit I didn't carefully read the code chunk. Anyway, it's not a big deal, you're only at v4. > > > > > > 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. Yes, that was needed because the enable/disable path were not separated, and we were unconditionally writing to the IP registers even when the PWM was already disabled (which is probably the case generating the fault reported by Stefan). This is not the case anymore, but let's wait for Stefan to confirm this. Also note that Stefan's patch is introducing an 'unbalanced clk prepapre/enable refcount' bug if ->apply() is called several times consecutively with the same state->enabled value. > > > > > > > > > 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". Yep, when someone calls twice ->apply() with state->enabled set to true or twice with state->enabled set to false. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 5, 2017 at 11:19 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Thu, 5 Jan 2017 10:03:47 +0100 > Lukasz Majewski <lukma@denx.de> wrote: >> > /* >> > * 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) { if (pwm_is_enabled()) ? I think it's better to do whatever API provides to be less error prone. >> > imx_pwm_wait_fifo_slot(chip, pwm); >> > } else { >> > ret = clk_prepare_enable(imx->clk_per); >> > if (ret) >> > return ret; >> if (state.enabled && !cstate.enabled) >> clk_preapre_enable(); > > Yep, and that's correct. !pwm_is_enabled() ?
On Thu, 5 Jan 2017 23:15:06 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jan 5, 2017 at 11:19 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Thu, 5 Jan 2017 10:03:47 +0100 > > Lukasz Majewski <lukma@denx.de> wrote: > >> > /* > >> > * 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) { > > if (pwm_is_enabled()) ? > > I think it's better to do whatever API provides to be less error prone. Both pwm_is_enabled() and pwm_get_state()+struct pwm_state are part of the PWM API, and I don't see how 'if (pwm_is_enabled())' is less error prone than 'if (cstate.enabled)'. This being said, I don't care much. It's mainly a matter of taste IMO, so if others agree to switch to pwm_is_enabled() I'm fine with that. > > >> > imx_pwm_wait_fifo_slot(chip, pwm); > >> > } else { > >> > ret = clk_prepare_enable(imx->clk_per); > >> > if (ret) > >> > return ret; > > >> if (state.enabled && !cstate.enabled) > >> clk_preapre_enable(); > > > > Yep, and that's correct. > > !pwm_is_enabled() ? > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stefan, > On Thu, 5 Jan 2017 10:03:47 +0100 > Lukasz Majewski <lukma@denx.de> wrote: > > > On Thu, 5 Jan 2017 09:50:35 +0100 > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > > > On Thu, 5 Jan 2017 00:36:50 +0100 > > > Lukasz Majewski <lukma@denx.de> wrote: > > > > > > > This commit provides apply() callback implementation for i.MX's > > > > PWMv2. > > > > > > > > Suggested-by: Stefan Agner <stefan@agner.ch> > > > > Suggested-by: Boris Brezillon > > > > <boris.brezillon@free-electrons.com> Signed-off-by: Lukasz > > > > Majewski <l.majewski@majess.pl> Reviewed-by: Boris Brezillon > > > > <boris.brezillon@free-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. > > 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. > > > > > 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. Stefan, could you respond on this issue? Thanks in advance, Łukasz Majewski > > What's for sure is that your implementation is introducing possible > unbalanced ref counting on the per clk. > > > > > > > > > > > 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@denx.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@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lukasz, Sry for the delay, I was traveling and had no access to hardware... On 2017-01-05 01:53, Boris Brezillon wrote: > On Thu, 5 Jan 2017 10:35:05 +0100 > Lukasz Majewski <lukma@denx.de> wrote: > >> On Thu, 5 Jan 2017 10:19:35 +0100 >> Boris Brezillon <boris.brezillon@free-electrons.com> wrote: >> >> > On Thu, 5 Jan 2017 10:03:47 +0100 >> > Lukasz Majewski <lukma@denx.de> wrote: >> > >> > > On Thu, 5 Jan 2017 09:50:35 +0100 >> > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: >> > > >> > > > On Thu, 5 Jan 2017 00:36:50 +0100 >> > > > Lukasz Majewski <lukma@denx.de> wrote: >> > > > >> > > > > This commit provides apply() callback implementation for i.MX's >> > > > > PWMv2. >> > > > > >> > > > > Suggested-by: Stefan Agner <stefan@agner.ch> >> > > > > Suggested-by: Boris Brezillon >> > > > > <boris.brezillon@free-electrons.com> Signed-off-by: Lukasz >> > > > > Majewski <l.majewski@majess.pl> Reviewed-by: Boris Brezillon >> > > > > <boris.brezillon@free-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 .....). > > Because I thought the unconditional clk_enable/disable was not part of > your example and would be added later, and I must admit I didn't > carefully read the code chunk. Anyway, it's not a big deal, you're only > at v4. > >> >> >> > >> > 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. > > Yes, that was needed because the enable/disable path were not > separated, and we were unconditionally writing to the IP registers > even when the PWM was already disabled (which is probably the case > generating the fault reported by Stefan). This is not the case anymore, > but let's wait for Stefan to confirm this. With v4 as is, the kernel crashes/hangs on i.MX 7. The function imx_pwm_apply_v2 gets first called with state->enabled 0, cstate->enabled 0. This branches to else and leads to a register access with clocks disabled (and if that would succeed, also an unbalanced disable?...) With the proposed change plus the additional change in the else branch it works for me: @@ -192,19 +193,20 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm, else period_cycles = 0; - 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) + if (cstate.enabled) { imx_pwm_wait_fifo_slot(chip, pwm); - else if (state->enabled) + } else if (state->enabled) { + ret = clk_prepare_enable(imx->clk_per); + if (ret) + return ret; + imx_pwm_sw_reset(chip); + } writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); writel(period_cycles, imx->mmio_base + MX3_PWMPR); @@ -218,7 +220,7 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm, cr |= MX3_PWMCR_POUTC; writel(cr, imx->mmio_base + MX3_PWMCR); - } else { + } else if (cstate.enabled) { writel(0, imx->mmio_base + MX3_PWMCR); clk_disable_unprepare(imx->clk_per); This would not disable a disabled PWM anymore, I guess at normal use not a problem. Only at bootup it could end up left on, but I guess if we care about boot time transition we should implement get_state, but something which we can do in a follow up patch. > > Also note that Stefan's patch is introducing an 'unbalanced clk > prepapre/enable refcount' bug if ->apply() is called several times > consecutively with the same state->enabled value. Hm, agreed on that one. -- Stefan > >> >> > >> > > >> > > 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". > > Yep, when someone calls twice ->apply() with state->enabled set to true > or twice with state->enabled set to false. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 09 Jan 2017 19:14:43 -0800 Stefan Agner <stefan@agner.ch> wrote: > >> > >> > > >> > 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. > > > > Yes, that was needed because the enable/disable path were not > > separated, and we were unconditionally writing to the IP registers > > even when the PWM was already disabled (which is probably the case > > generating the fault reported by Stefan). This is not the case anymore, > > but let's wait for Stefan to confirm this. > > With v4 as is, the kernel crashes/hangs on i.MX 7. > > The function imx_pwm_apply_v2 gets first called with state->enabled 0, > cstate->enabled 0. This branches to else and leads to a register access > with clocks disabled (and if that would succeed, also an unbalanced > disable?...) > > With the proposed change plus the additional change in the else branch > it works for me: > > @@ -192,19 +193,20 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, > struct pwm_device *pwm, > else > period_cycles = 0; > > - 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) > + if (cstate.enabled) { > imx_pwm_wait_fifo_slot(chip, pwm); > - else if (state->enabled) > + } else if (state->enabled) { > + ret = clk_prepare_enable(imx->clk_per); > + if (ret) > + return ret; > + > imx_pwm_sw_reset(chip); > + } > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > @@ -218,7 +220,7 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, > struct pwm_device *pwm, > cr |= MX3_PWMCR_POUTC; > > writel(cr, imx->mmio_base + MX3_PWMCR); > - } else { > + } else if (cstate.enabled) { > writel(0, imx->mmio_base + MX3_PWMCR); > > clk_disable_unprepare(imx->clk_per); > > > This would not disable a disabled PWM anymore, I guess at normal use not > a problem. Only at bootup it could end up left on, but I guess if we > care about boot time transition we should implement get_state, but > something which we can do in a follow up patch. Yep, that's a different problem which could be addressed by implementing ->get_state(). Note that you don't necessary want to disable the PWM at boot time, in some situation, when the PWM is driving a critical device (like the VDDIODDR regulator), you want the transition between the bootloader/firmware and Linux to be as smooth as possible. Actually, 'initial state retrieval' and 'atomic changes' were added to handle this case. Stefan, one last thing, can you apply patch 2 alone and check if it doesn't introduce a regression? Thanks, Boris -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + + 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); + + 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); + + 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,