| Message ID | 1477984230-18071-8-git-send-email-l.majewski@majess.pl |
|---|---|
| State | Superseded |
| Headers | show |
On 2016-11-01 00:10, Lukasz Majewski wrote: > This commit provides apply() callback implementation for i.MX's PWMv2. > > Suggested-by: Stefan Agner <stefan@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 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... -- 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, > }; -- 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 Tue, 22 Nov 2016 13:55:33 -0800 Stefan Agner <stefan@agner.ch> wrote: > On 2016-11-01 00:10, Lukasz Majewski wrote: > > This commit provides apply() callback implementation for i.MX's PWMv2. > > > > Suggested-by: Stefan Agner <stefan@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 v3: > > - Remove ipg clock enable/disable functions > > > > Changes for v2: > > - None > > --- > > drivers/pwm/pwm-imx.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 70 insertions(+) > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > index ebe9b0c..cd53c05 100644 > > --- a/drivers/pwm/pwm-imx.c > > +++ b/drivers/pwm/pwm-imx.c > > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip, > > } > > } > > > > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + unsigned long period_cycles, duty_cycles, prescale; > > + struct imx_chip *imx = to_imx_chip(chip); > > + struct pwm_state cstate; > > + unsigned long long c; > > + u32 cr = 0; > > + int ret; > > + > > + pwm_get_state(pwm, &cstate); > > + > > Couldn't we do: > > if (cstate.enabled) { ... > > > + c = clk_get_rate(imx->clk_per); > > + c *= state->period; > > + > > + do_div(c, 1000000000); > > + period_cycles = c; > > + > > + prescale = period_cycles / 0x10000 + 1; > > + > > + period_cycles /= prescale; > > + c = (unsigned long long)period_cycles * state->duty_cycle; > > + do_div(c, state->period); > > + duty_cycles = c; > > + > > + /* > > + * according to imx pwm RM, the real period value should be > > + * PERIOD value in PWMPR plus 2. > > + */ > > + if (period_cycles > 2) > > + period_cycles -= 2; > > + else > > + period_cycles = 0; > > + > > + /* Enable the clock if the PWM is being enabled. */ > > + if (state->enabled && !cstate.enabled) { > > + ret = clk_prepare_enable(imx->clk_per); > > + if (ret) > > + return ret; > > + } > > + > > + /* > > + * Wait for a free FIFO slot if the PWM is already enabled, and flush > > + * the FIFO if the PWM was disabled and is about to be enabled. > > + */ > > + if (cstate.enabled) > > + imx_pwm_wait_fifo_slot(chip, pwm); > > + else if (state->enabled) > > + imx_pwm_sw_reset(chip); > > + > > + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > + writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > + > > + cr |= MX3_PWMCR_PRESCALER(prescale) | > > + MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN | > > + MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH; > > + > > + if (state->enabled) > > + cr |= MX3_PWMCR_EN; > > } else if (state->enabled) { > imx_pwm_sw_reset(chip); > } > > and get rid of the if (state->enabled) in between? This would safe us > useless recalculation when disabling the controller... I get your point, but I'm pretty sure your proposal does not do what you want (remember that cstate is the current state, and state is the new state to apply). Something like that would work better: if (state->enabled) { c = clk_get_rate(imx->clk_per); c *= state->period; do_div(c, 1000000000); period_cycles = c; prescale = period_cycles / 0x10000 + 1; period_cycles /= prescale; c = (unsigned long long)period_cycles * state->duty_cycle; do_div(c, state->period); duty_cycles = c; /* * According to imx pwm RM, the real period value * should be PERIOD value in PWMPR plus 2. */ if (period_cycles > 2) period_cycles -= 2; else period_cycles = 0; /* * Enable the clock if the PWM is not already * enabled. */ if (!cstate.enabled) { ret = clk_prepare_enable(imx->clk_per); if (ret) return ret; } /* * Wait for a free FIFO slot if the PWM is already * enabled, and flush the FIFO if the PWM was disabled * and is about to be enabled. */ if (cstate.enabled) imx_pwm_wait_fifo_slot(chip, pwm); else imx_pwm_sw_reset(chip); writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); writel(period_cycles, imx->mmio_base + MX3_PWMPR); writel(MX3_PWMCR_PRESCALER(prescale) | MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH | MX3_PWMCR_EN, imx->mmio_base + MX3_PWMCR); } else { writel(0, imx->mmio_base + MX3_PWMCR); /* Disable the clock if the PWM is currently enabled. */ if (cstate.enabled) clk_disable_unprepare(imx->clk_per); } This being said, I'm a bit concerned by the way this driver handles PWM config requests. It seems that the new config request is queued, and nothing guarantees that it is actually applied when the pwm_apply/config/enable/disable() functions return. This approach has several flaws IMO: 1/ I'm not sure this is what the PWM users expect. Getting your request queued with no guarantees that it is applied can be weird in some cases (especially when the user changes the PWM config several times in a short period of time). 2/ In the disable path, you queue a "stop PWM" request, but you're not sure that it's actually dequeued before the per clk is disabled. What happens in that case? And more importantly, what happens when the PWM is re-enabled to apply a new config? AFAICS, there might be a short period of time where the re-enabled PWM is actually running with the old config until we flush the command queue and queue a new command. 3/ The queueing approach complicates the whole logic. You have to flush the FIFO in some cases, or wait for an empty slots if too many commands are queued. Forcing imx_pwm_apply_v2() to wait for the config request to be applied should simplify the whole thing, because you will always be guaranteed that the FIFO is empty, and that the current configuration is the last requested one. Regards, Boris -- 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 2016-11-23 00:38, Boris Brezillon wrote: > On Tue, 22 Nov 2016 13:55:33 -0800 > Stefan Agner <stefan@agner.ch> wrote: > >> On 2016-11-01 00:10, Lukasz Majewski wrote: >> > This commit provides apply() callback implementation for i.MX's PWMv2. >> > >> > Suggested-by: Stefan Agner <stefan@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 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 -- 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
Dear Stefan, Boris, > On 2016-11-23 00:38, Boris Brezillon wrote: > > On Tue, 22 Nov 2016 13:55:33 -0800 > > Stefan Agner <stefan@agner.ch> wrote: > > > >> On 2016-11-01 00:10, Lukasz Majewski wrote: > >> > This commit provides apply() callback implementation for i.MX's > >> > PWMv2. > >> > > >> > Suggested-by: Stefan Agner <stefan@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 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
On Mon, 28 Nov 2016 06:50:31 +0100 Lukasz Majewski <l.majewski@majess.pl> wrote: > Dear Stefan, Boris, > > > On 2016-11-23 00:38, Boris Brezillon wrote: > > > On Tue, 22 Nov 2016 13:55:33 -0800 > > > Stefan Agner <stefan@agner.ch> wrote: > > > > > >> On 2016-11-01 00:10, Lukasz Majewski wrote: > > >> > This commit provides apply() callback implementation for i.MX's > > >> > PWMv2. > > >> > > > >> > Suggested-by: Stefan Agner <stefan@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 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. > > 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 > -- 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
Dear Boris, Stefan, > On Mon, 28 Nov 2016 06:50:31 +0100 > Lukasz Majewski <l.majewski@majess.pl> wrote: > > > Dear Stefan, Boris, > > > > > On 2016-11-23 00:38, Boris Brezillon wrote: > > > > On Tue, 22 Nov 2016 13:55:33 -0800 > > > > Stefan Agner <stefan@agner.ch> wrote: > > > > > > > >> On 2016-11-01 00:10, Lukasz Majewski wrote: > > > >> > This commit provides apply() callback implementation for > > > >> > i.MX's PWMv2. > > > >> > > > > >> > Suggested-by: Stefan Agner <stefan@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 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 > > >
On Mon, 28 Nov 2016 21:48:57 +0100 Lukasz Majewski <l.majewski@majess.pl> wrote: > Dear Boris, Stefan, > > > On Mon, 28 Nov 2016 06:50:31 +0100 > > Lukasz Majewski <l.majewski@majess.pl> wrote: > > > > > Dear Stefan, Boris, > > > > > > > On 2016-11-23 00:38, Boris Brezillon wrote: > > > > > On Tue, 22 Nov 2016 13:55:33 -0800 > > > > > Stefan Agner <stefan@agner.ch> wrote: > > > > > > > > > >> On 2016-11-01 00:10, Lukasz Majewski wrote: > > > > >> > This commit provides apply() callback implementation for > > > > >> > i.MX's PWMv2. > > > > >> > > > > > >> > Suggested-by: Stefan Agner <stefan@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 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 -- 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 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, 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; + + 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, };