| Message ID | 20220824091215.141577-1-conor.dooley@microchip.com |
|---|---|
| Headers | show |
| Series | Microchip soft ip corePWM driver | expand |
Hello, On Wed, Aug 24, 2022 at 10:12:13AM +0100, Conor Dooley wrote: > \#pwm-cells for the Icicle kit's fabric PWM was incorrectly set to 2 & > blindly overridden by the (out of tree) driver anyway. The core can > support inverted operation, so update the entry to correctly report its > capabilities. > > Fixes: 72560c6559b8 ("riscv: dts: microchip: add fpga fabric section to icicle kit") > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi > index 0d28858b83f2..e09a13aef268 100644 > --- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi > +++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi > @@ -8,7 +8,7 @@ core_pwm0: pwm@41000000 { > compatible = "microchip,corepwm-rtl-v4"; > reg = <0x0 0x41000000 0x0 0xF0>; > microchip,sync-update-mask = /bits/ 32 <0>; > - #pwm-cells = <2>; > + #pwm-cells = <3>; > clocks = <&fabric_clk3>; > status = "disabled"; > }; there are no phandles on that PWM, so there is nothing that needs a followup adaption. Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
On Wed, Aug 24, 2022 at 10:12:15AM +0100, Conor Dooley wrote: > Add the newly introduced pwm driver to the existing PolarFire SoC entry. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
On 14/09/2022 20:59, Uwe Kleine-König wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > there are no phandles on that PWM, so there is nothing that needs a > followup adaption. > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Hey Uwe, Thanks for checking the pwm consumers :) I assume you going to take the corresponding binding change via either pwm-fixes or pwm-for-next, I think you can take the dts too, I don't think the dependency I previously thought existed exists. For that purpose: Acked-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor.
Hey Uwe, Thanks (as always). I've switched up my email setup a bit so I hope that I've not mangled anything here. On Thu, Sep 15, 2022 at 09:21:52AM +0200, Uwe Kleine-König wrote: > Hello, > > On Wed, Aug 24, 2022 at 10:12:14AM +0100, Conor Dooley wrote: > > Add a driver that supports the Microchip FPGA "soft" PWM IP core. > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > --- > > drivers/pwm/Kconfig | 10 + > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-microchip-core.c | 402 +++++++++++++++++++++++++++++++ > > 3 files changed, 413 insertions(+) > > create mode 100644 drivers/pwm/pwm-microchip-core.c > > > > +static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + const struct pwm_state *state) > > +{ > > + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); > > + struct pwm_state current_state = pwm->state; > > + bool period_locked; > > + u64 duty_steps; > > + u16 prescale; > > + u8 period_steps; > > + int ret; > > + > > + mutex_lock(&mchp_core_pwm->lock); > > + > > + if (!state->enabled) { > > + mchp_core_pwm_enable(chip, pwm, false, current_state.period); > > + mutex_unlock(&mchp_core_pwm->lock); > > + return 0; > > + } > > + > > + /* > > + * If the only thing that has changed is the duty cycle or the polarity, > > + * we can shortcut the calculations and just compute/apply the new duty > > + * cycle pos & neg edges > > + * As all the channels share the same period, do not allow it to be > > + * changed if any other channels are enabled. > > + * If the period is locked, it may not be possible to use a period > > + * less than that requested. In that case, we just abort. > > + */ > > + period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm); > > + > > + if (period_locked) { > > + u16 hw_prescale; > > + u8 hw_period_steps; > > + > > + mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps); > > Huh, if (u8 *)&prescale works depends on endianness. Big endian? What's that? ;) I think the cast can just be dropped and the u16 used directly instead. > > > + hw_prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE); > > + hw_period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD); > > + > > + if ((period_steps + 1) * (prescale + 1) < > > + (hw_period_steps + 1) * (hw_prescale + 1)) { > > + mutex_unlock(&mchp_core_pwm->lock); > > + return -EINVAL; > > + } > > + > > + /* > > + * It is possible that something could have set the period_steps > > My German feel for the English language says s/could have/has/ What I wrote is _fine_ but the could is redudant given the possible. I'll change it over. > > + * register to 0xff, which would prevent us from setting a 100% > > For my understanding: It would also prevent a 0% relative duty, right? Yeah, I guess the comment could reflect that. > > > + * duty cycle, as explained in the mchp_core_pwm_calc_period() > > s/duty/relative duty/; s/the // > > > + * above. > > + * The period is locked and we cannot change this, so we abort. > > + */ > > + if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX) { > > Don't you need to check hw_period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX > here? D'oh. > > > + mutex_unlock(&mchp_core_pwm->lock); > > + return -EINVAL; > > + } > > + > > + prescale = hw_prescale; > > + period_steps = hw_period_steps; > > + } else if (!current_state.enabled || current_state.period != state->period) { > > + ret = mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps); > > ret is only used in this block, so the declaration can go into here, > too. > > > + if (ret) { > > + mutex_unlock(&mchp_core_pwm->lock); > > + return ret; > > + } > > + mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps); > > + } else { > > + prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE); > > + period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD); > > + /* > > + * As above, it is possible that something could have set the > > + * period_steps register to 0xff, which would prevent us from > > + * setting a 100% duty cycle, as explained above. > > + * As the period is not locked, we are free to fix this. > > + */ > > Are you sure this is safe? I think it isn't. Consider: > > pwm_apply_state(mypwm, { .duty = 0, .period = A, .enabled = true, }); > pwm_apply_state(mypwm, { .duty = 0, .period = B, .enabled = false, }); > pwm_apply_state(mypwm, { .duty = 0, .period = B, .enabled = true, }); > > Then you have in the third call prescale and period_steps still > corresponding to A because you didn't update these registers in the 2nd > call as you exited early. Riiight. I think I am a little confused here - this comment does not refer to my comment but rather to the whole logic I have? As in, what you're concerned about is the early exit if the state is disabled & that I take the values in the hardware as accurate? What makes sense to me to do here (assuming I understood correctly) is to compare state->period against what is in the hardare rather than against what the pwm core thinks? Or else I could stop exiting early if the pwm is to be disabled & instead allow the period and duty to be set so that the state of the hardware is as close to the pwm core's representation of it as possible. Keeping the core's interpretation as close to correct as possible seems like a good idea to me - my only concern is that the apply() will fail. But if I have read & understand the core code correctly, the core will not update pwm->state from {.duty = 0, .period = A, .enabled = true} if callying apply with {.duty = 0, .period = B, .enabled = false} fails? > > > + if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX) { > > + period_steps -= 1; > > + mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps); > > + } > > + } > > + > > + duty_steps = mchp_core_pwm_calc_duty(chip, pwm, state, prescale, period_steps); > > + > > + /* > > + * Because the period is per channel, it is possible that the requested > > + * duty cycle is longer than the period, in which case cap it to the > > + * period, IOW a 100% duty cycle. > > + */ > > + if (duty_steps > period_steps) > > + duty_steps = period_steps + 1; > > + > > + mchp_core_pwm_apply_duty(chip, pwm, state, duty_steps, period_steps); > > + > > + mchp_core_pwm_enable(chip, pwm, true, state->period); > > + > > + mutex_unlock(&mchp_core_pwm->lock); > > + > > + return 0; > Locking could be a bit simplified by doing: > The diffstat is negative, so maybe that's subjective. Much simplier at the cost of 4 lines sounds objective to me! > > +} > > + > > +static void mchp_core_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); > > + u16 prescale; > > + u8 period_steps, duty_steps, posedge, negedge; > > + > > + mutex_lock(&mchp_core_pwm->lock); > > + > > + if (mchp_core_pwm->channel_enabled & (1 << pwm->hwpwm)) > > + state->enabled = true; > > + else > > + state->enabled = false; > > + > > + prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE)); > > + > > + period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD)); > > + state->period = period_steps * prescale * NSEC_PER_SEC; > > This is broken on 32 bit archs (here: arm): > > $ cat test.c > #include <inttypes.h> > #include <stdio.h> > #include <stdlib.h> > > int main(int argc, char *argv[]) > { > uint8_t period_steps = atoi(argv[1]); > uint16_t prescale = atoi(argv[2]); > uint64_t period; > > period = period_steps * prescale * 1000000000L; > > printf("period_steps = %" PRIu8 "\n", period_steps); > printf("prescale = %" PRIu16 "\n", prescale); > printf("period = %" PRIu64 "\n", period); > > return 0; > } > > $ make test > cc test.c -o test > > $ ./test 255 65535 > period_steps = 255 > prescale = 65535 > period = 18446744073018591744 > > The problem is that the result of 16711425 * 1000000000L isn't affected > by the type of period and so it's promoted to L which isn't big enough > to hold 16711425000000000 where longs are only 32 bit wide. I don't think this is ever going to be hit in the wild, since prescale comes from the hardware where it is limited to 255 - but preventing the issue seems trivially done by splitting the multiplication so no reason not to. Thanks for providing the test program btw :) > > > + state->period = DIV64_U64_ROUND_UP(state->period, clk_get_rate(mchp_core_pwm->clk)); > > + > > + posedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm)); > > + negedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm)); > > + > > + if ((negedge == posedge) && state->enabled) { > > Why do you need that state->enabled? Because I was running into conflicts between the reporting here and some of the checks that I have added to prevent the PWM being put into an invalid state. On boot both negedge and posedge will be zero & this was preventing me from setting the period at all. > > > + state->duty_cycle = state->period; > > + state->period *= 2; > > + } else { > > + duty_steps = abs((s16)posedge - (s16)negedge); > > + state->duty_cycle = duty_steps * prescale * NSEC_PER_SEC; > > + state->duty_cycle = DIV64_U64_ROUND_UP(state->duty_cycle, > > + clk_get_rate(mchp_core_pwm->clk)); > > Micro optimisation: Call clk_get_rate() only once. > > > + } > > + > > + state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL; > > + > > + mutex_unlock(&mchp_core_pwm->lock); > > You could release the lock a bit earlier. > Sure, I'll move it after the last register access. Thanks, Conor.
On Mon, Sep 19, 2022 at 01:53:56PM +0100, Conor Dooley wrote: > Hey Uwe, > Thanks (as always). I've switched up my email setup a bit so I hope > that I've not mangled anything here. > > On Thu, Sep 15, 2022 at 09:21:52AM +0200, Uwe Kleine-König wrote: > > Hello, > > > > On Wed, Aug 24, 2022 at 10:12:14AM +0100, Conor Dooley wrote: > > > Add a driver that supports the Microchip FPGA "soft" PWM IP core. > > > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > --- > > > drivers/pwm/Kconfig | 10 + > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-microchip-core.c | 402 +++++++++++++++++++++++++++++++ > > > 3 files changed, 413 insertions(+) > > > create mode 100644 drivers/pwm/pwm-microchip-core.c > > > > > > > +static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > + const struct pwm_state *state) > > > +{ > > > + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); > > > + struct pwm_state current_state = pwm->state; > > > + bool period_locked; > > > + u64 duty_steps; > > > + u16 prescale; > > > + u8 period_steps; > > > + int ret; > > > + > > > + mutex_lock(&mchp_core_pwm->lock); > > > + > > > + if (!state->enabled) { > > > + mchp_core_pwm_enable(chip, pwm, false, current_state.period); > > > + mutex_unlock(&mchp_core_pwm->lock); > > > + return 0; > > > + } > > > + > > > + /* > > > + * If the only thing that has changed is the duty cycle or the polarity, > > > + * we can shortcut the calculations and just compute/apply the new duty > > > + * cycle pos & neg edges > > > + * As all the channels share the same period, do not allow it to be > > > + * changed if any other channels are enabled. > > > + * If the period is locked, it may not be possible to use a period > > > + * less than that requested. In that case, we just abort. > > > + */ > > > + period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm); > > > + > > > + if (period_locked) { > > > + u16 hw_prescale; > > > + u8 hw_period_steps; > > > + > > > + mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps); > > > > Huh, if (u8 *)&prescale works depends on endianness. > > Big endian? What's that? ;) > I think the cast can just be dropped and the u16 used directly instead. > > > > > > + hw_prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE); > > > + hw_period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD); > > > + > > > + if ((period_steps + 1) * (prescale + 1) < > > > + (hw_period_steps + 1) * (hw_prescale + 1)) { > > > + mutex_unlock(&mchp_core_pwm->lock); > > > + return -EINVAL; > > > + } > > > + > > > + /* > > > + * It is possible that something could have set the period_steps > > > > My German feel for the English language says s/could have/has/ > > What I wrote is _fine_ but the could is redudant given the possible. > I'll change it over. > > > > + * register to 0xff, which would prevent us from setting a 100% > > > > For my understanding: It would also prevent a 0% relative duty, right? > > Yeah, I guess the comment could reflect that. > > > > > > + * duty cycle, as explained in the mchp_core_pwm_calc_period() > > > > s/duty/relative duty/; s/the // > > > > > + * above. > > > + * The period is locked and we cannot change this, so we abort. > > > + */ > > > + if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX) { > > > > Don't you need to check hw_period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX > > here? > > D'oh. > > > > > > + mutex_unlock(&mchp_core_pwm->lock); > > > + return -EINVAL; > > > + } > > > + > > > + prescale = hw_prescale; > > > + period_steps = hw_period_steps; > > > + } else if (!current_state.enabled || current_state.period != state->period) { > > > + ret = mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps); > > > > ret is only used in this block, so the declaration can go into here, > > too. > > > > > + if (ret) { > > > + mutex_unlock(&mchp_core_pwm->lock); > > > + return ret; > > > + } > > > + mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps); > > > + } else { > > > + prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE); > > > + period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD); > > > + /* > > > + * As above, it is possible that something could have set the > > > + * period_steps register to 0xff, which would prevent us from > > > + * setting a 100% duty cycle, as explained above. > > > + * As the period is not locked, we are free to fix this. > > > + */ > > > > Are you sure this is safe? I think it isn't. Consider: > > > > pwm_apply_state(mypwm, { .duty = 0, .period = A, .enabled = true, }); > > pwm_apply_state(mypwm, { .duty = 0, .period = B, .enabled = false, }); > > pwm_apply_state(mypwm, { .duty = 0, .period = B, .enabled = true, }); > > > > Then you have in the third call prescale and period_steps still > > corresponding to A because you didn't update these registers in the 2nd > > call as you exited early. > > Riiight. I think I am a little confused here - this comment does not > refer to my comment but rather to the whole logic I have? > > As in, what you're concerned about is the early exit if the state is > disabled & that I take the values in the hardware as accurate? No, the thing I'm concerned about is assuming MCHPCOREPWM_PRESCALE and MCHPCOREPWM_PERIOD correspond to state->period. So I'd drop the last block use the 2nd last instead without further condition. > What makes sense to me to do here (assuming I understood correctly) > is to compare state->period against what is in the hardare rather than > against what the pwm core thinks? > Or else I could stop exiting early if the pwm is to be disabled & > instead allow the period and duty to be set so that the state of the > hardware is as close to the pwm core's representation of it as possible. exiting early is fine. > > > [...] > > > + period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD)); > > > + state->period = period_steps * prescale * NSEC_PER_SEC; > > > > This is broken on 32 bit archs (here: arm): > > > > $ cat test.c > > #include <inttypes.h> > > #include <stdio.h> > > #include <stdlib.h> > > > > int main(int argc, char *argv[]) > > { > > uint8_t period_steps = atoi(argv[1]); > > uint16_t prescale = atoi(argv[2]); > > uint64_t period; > > > > period = period_steps * prescale * 1000000000L; > > > > printf("period_steps = %" PRIu8 "\n", period_steps); > > printf("prescale = %" PRIu16 "\n", prescale); > > printf("period = %" PRIu64 "\n", period); > > > > return 0; > > } > > > > $ make test > > cc test.c -o test > > > > $ ./test 255 65535 > > period_steps = 255 > > prescale = 65535 > > period = 18446744073018591744 > > > > The problem is that the result of 16711425 * 1000000000L isn't affected > > by the type of period and so it's promoted to L which isn't big enough > > to hold 16711425000000000 where longs are only 32 bit wide. > > I don't think this is ever going to be hit in the wild, since prescale > comes from the hardware where it is limited to 255 - but preventing the > issue seems trivially done by splitting the multiplication so no reason > not to. Thanks for providing the test program btw :) Even 255 * 255 * 1000000000 overflows. With a maintainer's hat on, it is very valuable to prevent such issues because your driver might be used as a template for the next driver. > > > + state->period = DIV64_U64_ROUND_UP(state->period, clk_get_rate(mchp_core_pwm->clk)); > > > + > > > + posedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm)); > > > + negedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm)); > > > + > > > + if ((negedge == posedge) && state->enabled) { > > > > Why do you need that state->enabled? > > Because I was running into conflicts between the reporting here and some > of the checks that I have added to prevent the PWM being put into an > invalid state. On boot both negedge and posedge will be zero & this was > preventing me from setting the period at all. I don't understood that. Best regards Uwe
Hey Uwe, On Mon, Sep 19, 2022 at 03:50:08PM +0200, Uwe Kleine-König wrote: > On Mon, Sep 19, 2022 at 01:53:56PM +0100, Conor Dooley wrote: > > Hey Uwe, > > Thanks (as always). I've switched up my email setup a bit so I hope > > that I've not mangled anything here. > > > > On Thu, Sep 15, 2022 at 09:21:52AM +0200, Uwe Kleine-König wrote: > > > Hello, > > > > > > On Wed, Aug 24, 2022 at 10:12:14AM +0100, Conor Dooley wrote: > > > > Add a driver that supports the Microchip FPGA "soft" PWM IP core. > > > > > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > > --- > > > > drivers/pwm/Kconfig | 10 + > > > > drivers/pwm/Makefile | 1 + > > > > drivers/pwm/pwm-microchip-core.c | 402 +++++++++++++++++++++++++++++++ > > > > 3 files changed, 413 insertions(+) > > > > create mode 100644 drivers/pwm/pwm-microchip-core.c > > > > > > > > > > +static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > + const struct pwm_state *state) > > > > +{ > > > > + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); > > > > + struct pwm_state current_state = pwm->state; > > > > + bool period_locked; > > > > + u64 duty_steps; > > > > + u16 prescale; > > > > + u8 period_steps; > > > > + int ret; > > > > + > > > > + mutex_lock(&mchp_core_pwm->lock); > > > > + > > > > + if (!state->enabled) { > > > > + mchp_core_pwm_enable(chip, pwm, false, current_state.period); > > > > + mutex_unlock(&mchp_core_pwm->lock); > > > > + return 0; > > > > + } > > > > + > > > > + /* > > > > + * If the only thing that has changed is the duty cycle or the polarity, > > > > + * we can shortcut the calculations and just compute/apply the new duty > > > > + * cycle pos & neg edges > > > > + * As all the channels share the same period, do not allow it to be > > > > + * changed if any other channels are enabled. > > > > + * If the period is locked, it may not be possible to use a period > > > > + * less than that requested. In that case, we just abort. > > > > + */ > > > > + period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm); > > > > + > > > > + if (period_locked) { > > > > + u16 hw_prescale; > > > > + u8 hw_period_steps; > > > > + > > > > + mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps); > > > > > > Huh, if (u8 *)&prescale works depends on endianness. > > > > Big endian? What's that? ;) > > I think the cast can just be dropped and the u16 used directly instead. > > > > > > > > > + hw_prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE); > > > > + hw_period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD); > > > > + > > > > + if ((period_steps + 1) * (prescale + 1) < > > > > + (hw_period_steps + 1) * (hw_prescale + 1)) { > > > > + mutex_unlock(&mchp_core_pwm->lock); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + /* > > > > + * It is possible that something could have set the period_steps > > > > > > My German feel for the English language says s/could have/has/ > > > > What I wrote is _fine_ but the could is redudant given the possible. > > I'll change it over. > > > > > > + * register to 0xff, which would prevent us from setting a 100% > > > > > > For my understanding: It would also prevent a 0% relative duty, right? > > > > Yeah, I guess the comment could reflect that. > > > > > > > > > + * duty cycle, as explained in the mchp_core_pwm_calc_period() > > > > > > s/duty/relative duty/; s/the // > > > > > > > + * above. > > > > + * The period is locked and we cannot change this, so we abort. > > > > + */ > > > > + if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX) { > > > > > > Don't you need to check hw_period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX > > > here? > > > > D'oh. > > > > > > > > > + mutex_unlock(&mchp_core_pwm->lock); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + prescale = hw_prescale; > > > > + period_steps = hw_period_steps; > > > > + } else if (!current_state.enabled || current_state.period != state->period) { > > > > + ret = mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps); > > > > > > ret is only used in this block, so the declaration can go into here, > > > too. > > > > > > > + if (ret) { > > > > + mutex_unlock(&mchp_core_pwm->lock); > > > > + return ret; > > > > + } > > > > + mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps); > > > > + } else { > > > > + prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE); > > > > + period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD); > > > > + /* > > > > + * As above, it is possible that something could have set the > > > > + * period_steps register to 0xff, which would prevent us from > > > > + * setting a 100% duty cycle, as explained above. > > > > + * As the period is not locked, we are free to fix this. > > > > + */ > > > > > > Are you sure this is safe? I think it isn't. Consider: > > > > > > pwm_apply_state(mypwm, { .duty = 0, .period = A, .enabled = true, }); > > > pwm_apply_state(mypwm, { .duty = 0, .period = B, .enabled = false, }); > > > pwm_apply_state(mypwm, { .duty = 0, .period = B, .enabled = true, }); > > > > > > Then you have in the third call prescale and period_steps still > > > corresponding to A because you didn't update these registers in the 2nd > > > call as you exited early. > > > > Riiight. I think I am a little confused here - this comment does not > > refer to my comment but rather to the whole logic I have? > > > > As in, what you're concerned about is the early exit if the state is > > disabled & that I take the values in the hardware as accurate? > > No, the thing I'm concerned about is assuming MCHPCOREPWM_PRESCALE and > MCHPCOREPWM_PERIOD correspond to state->period. So I'd drop the last > block use the 2nd last instead without further condition. So, if the period isn't locked always re-configure it. Makes life easier for me. > > > What makes sense to me to do here (assuming I understood correctly) > > is to compare state->period against what is in the hardare rather than > > against what the pwm core thinks? > > Or else I could stop exiting early if the pwm is to be disabled & > > instead allow the period and duty to be set so that the state of the > > hardware is as close to the pwm core's representation of it as possible. > > exiting early is fine. > > > > > [...] > > > > + period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD)); > > > > + state->period = period_steps * prescale * NSEC_PER_SEC; > > > > > > This is broken on 32 bit archs (here: arm): > > > > > > $ cat test.c > > > #include <inttypes.h> > > > #include <stdio.h> > > > #include <stdlib.h> > > > > > > int main(int argc, char *argv[]) > > > { > > > uint8_t period_steps = atoi(argv[1]); > > > uint16_t prescale = atoi(argv[2]); > > > uint64_t period; > > > > > > period = period_steps * prescale * 1000000000L; > > > > > > printf("period_steps = %" PRIu8 "\n", period_steps); > > > printf("prescale = %" PRIu16 "\n", prescale); > > > printf("period = %" PRIu64 "\n", period); > > > > > > return 0; > > > } > > > > > > $ make test > > > cc test.c -o test > > > > > > $ ./test 255 65535 > > > period_steps = 255 > > > prescale = 65535 > > > period = 18446744073018591744 > > > > > > The problem is that the result of 16711425 * 1000000000L isn't affected > > > by the type of period and so it's promoted to L which isn't big enough > > > to hold 16711425000000000 where longs are only 32 bit wide. > > > > I don't think this is ever going to be hit in the wild, since prescale > > comes from the hardware where it is limited to 255 - but preventing the > > issue seems trivially done by splitting the multiplication so no reason > > not to. Thanks for providing the test program btw :) > > Even 255 * 255 * 1000000000 overflows. With a maintainer's hat on, it is > very valuable to prevent such issues because your driver might be used > as a template for the next driver. > > > > > + state->period = DIV64_U64_ROUND_UP(state->period, clk_get_rate(mchp_core_pwm->clk)); > > > > + > > > > + posedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm)); > > > > + negedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm)); > > > > + > > > > + if ((negedge == posedge) && state->enabled) { > > > > > > Why do you need that state->enabled? > > > > Because I was running into conflicts between the reporting here and some > > of the checks that I have added to prevent the PWM being put into an > > invalid state. On boot both negedge and posedge will be zero & this was > > preventing me from setting the period at all. > > I don't understood that. On startup, (negedge == posedge) is true as both are zero, but the reset values for prescale and period are actually 0x8. If on reset I try to set a small period, say "echo 1000 > period" apply() returns -EINVAL because of a check in the pwm core in pwm_apply_state() as I am attempting to set the period to lower than the out-of-reset duty cycle. I considered zeroing the registers, but if something below Linux had been using the PWM I felt that may not be the right thing to do. Can I continue to check for the enablement here or would you rather I did something different? Thanks, Conor.
On Mon, Sep 19, 2022 at 03:29:19PM +0100, Conor Dooley wrote: > Hey Uwe, > > On Mon, Sep 19, 2022 at 03:50:08PM +0200, Uwe Kleine-König wrote: > > On Mon, Sep 19, 2022 at 01:53:56PM +0100, Conor Dooley wrote: > > > Hey Uwe, > > > Thanks (as always). I've switched up my email setup a bit so I hope > > > that I've not mangled anything here. > > > > > > On Thu, Sep 15, 2022 at 09:21:52AM +0200, Uwe Kleine-König wrote: > > > > Hello, > > > > > > > > On Wed, Aug 24, 2022 at 10:12:14AM +0100, Conor Dooley wrote: > > > > > Add a driver that supports the Microchip FPGA "soft" PWM IP core. > > > > > > > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > > > --- > > > > $ ./test 255 65535 > > > > period_steps = 255 > > > > prescale = 65535 > > > > period = 18446744073018591744 > > > > > > > > The problem is that the result of 16711425 * 1000000000L isn't affected > > > > by the type of period and so it's promoted to L which isn't big enough > > > > to hold 16711425000000000 where longs are only 32 bit wide. > > > > > > I don't think this is ever going to be hit in the wild, since prescale > > > comes from the hardware where it is limited to 255 - but preventing the > > > issue seems trivially done by splitting the multiplication so no reason > > > not to. Thanks for providing the test program btw :) > > > > Even 255 * 255 * 1000000000 overflows. With a maintainer's hat on, it is > > very valuable to prevent such issues because your driver might be used > > as a template for the next driver. > > > > > > > + state->period = DIV64_U64_ROUND_UP(state->period, clk_get_rate(mchp_core_pwm->clk)); > > > > > + > > > > > + posedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm)); > > > > > + negedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm)); > > > > > + > > > > > + if ((negedge == posedge) && state->enabled) { > > > > > > > > Why do you need that state->enabled? > > > > > > Because I was running into conflicts between the reporting here and some > > > of the checks that I have added to prevent the PWM being put into an > > > invalid state. On boot both negedge and posedge will be zero & this was > > > preventing me from setting the period at all. > > > > I don't understood that. > > On startup, (negedge == posedge) is true as both are zero, but the reset > values for prescale and period are actually 0x8. If on reset I try to > set a small period, say "echo 1000 > period" apply() returns -EINVAL > because of a check in the pwm core in pwm_apply_state() as I am > attempting to set the period to lower than the out-of-reset duty cycle. > > I considered zeroing the registers, but if something below Linux had > been using the PWM I felt that may not be the right thing to do. Can I > continue to check for the enablement here or would you rather I did > something different? Hey Uwe, Just bumping here ICYMI. Should I leave the behaviour as-was and just document what the default values out of reset may be? That would leave the check here making more sense & head off confusion about why apply() fails? Thanks, Conor.
On Mon, Sep 19, 2022 at 03:29:19PM +0100, Conor Dooley wrote: > Hey Uwe, > > On Mon, Sep 19, 2022 at 03:50:08PM +0200, Uwe Kleine-König wrote: > > On Mon, Sep 19, 2022 at 01:53:56PM +0100, Conor Dooley wrote: > > > Because I was running into conflicts between the reporting here and some > > > of the checks that I have added to prevent the PWM being put into an > > > invalid state. On boot both negedge and posedge will be zero & this was > > > preventing me from setting the period at all. > > > > I don't understood that. > > On startup, (negedge == posedge) is true as both are zero, but the reset > values for prescale and period are actually 0x8. If on reset I try to > set a small period, say "echo 1000 > period" apply() returns -EINVAL > because of a check in the pwm core in pwm_apply_state() as I am > attempting to set the period to lower than the out-of-reset duty cycle. You're supposed to keep the period for pwm#1 untouched while configuring pwm#0 only if pwm#1 already has a consumer. So if pwm#1 isn't requested, you can change the period for pwm#0. Best regards Uwe
On Fri, Sep 30, 2022 at 11:13:16AM +0200, Uwe Kleine-König wrote: > On Mon, Sep 19, 2022 at 03:29:19PM +0100, Conor Dooley wrote: > > Hey Uwe, > > > > On Mon, Sep 19, 2022 at 03:50:08PM +0200, Uwe Kleine-König wrote: > > > On Mon, Sep 19, 2022 at 01:53:56PM +0100, Conor Dooley wrote: > > > > Because I was running into conflicts between the reporting here and some > > > > of the checks that I have added to prevent the PWM being put into an > > > > invalid state. On boot both negedge and posedge will be zero & this was > > > > preventing me from setting the period at all. > > > > > > I don't understood that. > > > > On startup, (negedge == posedge) is true as both are zero, but the reset > > values for prescale and period are actually 0x8. If on reset I try to > > set a small period, say "echo 1000 > period" apply() returns -EINVAL > > because of a check in the pwm core in pwm_apply_state() as I am > > attempting to set the period to lower than the out-of-reset duty cycle. > > You're supposed to keep the period for pwm#1 untouched while configuring > pwm#0 only if pwm#1 already has a consumer. So if pwm#1 isn't requested, > you can change the period for pwm#0. I must have done a bad job of explaining here, as I don't think this is an answer to my question. On reset, the prescale and period_steps registers are set to 0x8. If I attempt to set the period to do "echo 1000 > period", I get -EINVAL back from pwm_apply_state() (in next-20220928 it's @ L562 in pwm/core.c) as the duty cycle is computed as twice the period as, on reset, we have posedge = negedge = 0x0. The check of state->duty_cycle > state->period fails in pwm_apply_state() as a result. This failure to assign a value is unrelated to having multiple PWMs, I think I may have horribly worded my statement when I originally replied to you with: > Because I was running into conflicts between the reporting here and some > of the checks that I have added to prevent the PWM being put into an > invalid state. "reporting here" from that quote being the period/duty cycle calculations in the drivers get_state(). By "the checks" I meant making sure that a period where posedge = negedge is not set by the driver. I think I also may have mistakenly assumed the -EINVAL came from my code and not from the core - but I cannot be sure as it has been a few weeks. The check in the core looks to be things "working as intended", and it looks like I am working around it here. Should I just note what the values are on reset in the "limitations" comment and the top & it is up to applications that control the PWMs to first "fix" the duty cycle before changing the period? Hopefully I've done a better job at explaning this time, Conor.
On Fri, Sep 30, 2022 at 10:45:56AM +0100, Conor Dooley wrote: > On Fri, Sep 30, 2022 at 11:13:16AM +0200, Uwe Kleine-König wrote: > > On Mon, Sep 19, 2022 at 03:29:19PM +0100, Conor Dooley wrote: > > > Hey Uwe, > > > > > > On Mon, Sep 19, 2022 at 03:50:08PM +0200, Uwe Kleine-König wrote: > > > > On Mon, Sep 19, 2022 at 01:53:56PM +0100, Conor Dooley wrote: > > > > > Because I was running into conflicts between the reporting here and some > > > > > of the checks that I have added to prevent the PWM being put into an > > > > > invalid state. On boot both negedge and posedge will be zero & this was > > > > > preventing me from setting the period at all. > > > > > > > > I don't understood that. > > > > > > On startup, (negedge == posedge) is true as both are zero, but the reset > > > values for prescale and period are actually 0x8. If on reset I try to > > > set a small period, say "echo 1000 > period" apply() returns -EINVAL > > > because of a check in the pwm core in pwm_apply_state() as I am > > > attempting to set the period to lower than the out-of-reset duty cycle. > > > > You're supposed to keep the period for pwm#1 untouched while configuring > > pwm#0 only if pwm#1 already has a consumer. So if pwm#1 isn't requested, > > you can change the period for pwm#0. > > I must have done a bad job of explaining here, as I don't think this is > an answer to my question. > > On reset, the prescale and period_steps registers are set to 0x8. If I > attempt to set the period to do "echo 1000 > period", I get -EINVAL back > from pwm_apply_state() (in next-20220928 it's @ L562 in pwm/core.c) as > the duty cycle is computed as twice the period as, on reset, we have > posedge = negedge = 0x0. The check of state->duty_cycle > state->period > fails in pwm_apply_state() as a result. So set duty_cycle to 0 first? A problem of the sysfs interface is that you can only set one parameter after the other. So there you have to find a sequence of valid pwm_states that only differ in a single parameter between the initial and the desired state. That's nothing a "normal" pwm consumer would be affected by. (IMHO we should have a userspace API that benefits from the properties of pwm_apply().) Best regards Uwe
On Fri, Sep 30, 2022 at 03:39:33PM +0200, Uwe Kleine-König wrote: > On Fri, Sep 30, 2022 at 10:45:56AM +0100, Conor Dooley wrote: > > On Fri, Sep 30, 2022 at 11:13:16AM +0200, Uwe Kleine-König wrote: > > > On Mon, Sep 19, 2022 at 03:29:19PM +0100, Conor Dooley wrote: > > > > Hey Uwe, > > > > > > > > On Mon, Sep 19, 2022 at 03:50:08PM +0200, Uwe Kleine-König wrote: > > > > > On Mon, Sep 19, 2022 at 01:53:56PM +0100, Conor Dooley wrote: > > > > > > Because I was running into conflicts between the reporting here and some > > > > > > of the checks that I have added to prevent the PWM being put into an > > > > > > invalid state. On boot both negedge and posedge will be zero & this was > > > > > > preventing me from setting the period at all. > > > > > > > > > > I don't understood that. > > > > > > > > On startup, (negedge == posedge) is true as both are zero, but the reset > > > > values for prescale and period are actually 0x8. If on reset I try to > > > > set a small period, say "echo 1000 > period" apply() returns -EINVAL > > > > because of a check in the pwm core in pwm_apply_state() as I am > > > > attempting to set the period to lower than the out-of-reset duty cycle. > > > > > > You're supposed to keep the period for pwm#1 untouched while configuring > > > pwm#0 only if pwm#1 already has a consumer. So if pwm#1 isn't requested, > > > you can change the period for pwm#0. > > > > I must have done a bad job of explaining here, as I don't think this is > > an answer to my question. > > > > On reset, the prescale and period_steps registers are set to 0x8. If I > > attempt to set the period to do "echo 1000 > period", I get -EINVAL back > > from pwm_apply_state() (in next-20220928 it's @ L562 in pwm/core.c) as > > the duty cycle is computed as twice the period as, on reset, we have > > posedge = negedge = 0x0. The check of state->duty_cycle > state->period > > fails in pwm_apply_state() as a result. > > So set duty_cycle to 0 first? > > A problem of the sysfs interface is that you can only set one parameter > after the other. So there you have to find a sequence of valid > pwm_states that only differ in a single parameter between the initial > and the desired state. > > That's nothing a "normal" pwm consumer would be affected by. (IMHO we > should have a userspace API that benefits from the properties of > pwm_apply().) Right, so I guess I will drop the check so. That's good to know, thanks. Would you rather I waited until after the mw to send v11? Thanks, Conor.
On Fri, Sep 30, 2022 at 02:49:12PM +0100, Conor Dooley wrote:
> Would you rather I waited until after the mw to send v11?
I don't see much sense in waiting.
Best regards
Uwe