Microchip soft ip corePWM driver
Subsystems (4)
Conor Dooley
maintainerv11 cover Microchip soft ip corePWM driver
Hey Uwe, all,
~6.0-rc1 has rolled around so here is the promised v8v9~. v11 is based on 6.0 stuff still & there will be a change to the dts patch in v6.1, but I did a test merge and there was nothing to resolve. I'll take the dts change myself just to be on the safe side.
The pre 6.0-rc1 cover letter/series is here:
https://lore.kernel.org/linux-pwm/[email protected]
Thanks,
Conor.
Changes since v10:
- reword some comments
- try to assign the period if a disable is requested
- drop a cast around a u8 -> u16 conversion
- fix a check on period_steps that should be on the hw_ variant
- split up the period calculation in get_state() to fix the result on 32 bit
- add a rate variable in get_state() to only call get_rate() once.
- redo the locking as suggested to make it more straightforward.
- stop checking for enablement in get_state() that was working around intended behaviour of the sysfs interface
Changes since v9:
- fixed the missing unlock that Dan reported
Changes since v8:
- fixed a(nother) raw 64 bit division (& built it for riscv32!)
- added a check to make sure we don't try to sleep for 0 us
Changes since v7:
- rebased on 6.0-rc1
- reworded comments you highlighted in v7
- fixed the overkill sleeping
- removed the unused variables in calc_duty
- added some extra comments to explain behaviours you questioned in v7
- make the mutexes un-interruptible
- fixed added the 1s you suggested for the if(period_locked) logic
- added setup of the channel_enabled shadowing
- fixed the period reporting for the negedge == posedge case in get_state() I had to add the enabled check, as otherwise it broke setting the period for the first time out of reset.
- added a test for invalid PERIOD_STEPS values, in which case we abort if we cannot fix the period
Changes from v6:
- Dropped an unused variable that I'd missed
- Actually check the return values of the mutex lock()s
- Re-rebased on -next for the MAINTAINERS patch (again...)
Changes from v5:
- switched to a mutex b/c we must sleep with the lock taken
- simplified the locking in apply() and added locking to get_state()
- reworked apply() as requested
- removed the loop in the period calculation (thanks Uwe!)
- add a copy of the enable registers in the driver to save on reads.
- remove the second (useless) write to sync_update
- added some missing rounding in get_state()
- couple other minor cleanups as requested in:
https://lore.kernel.org/linux-riscv/[email protected]/
Changes from v4:
- dropped some accidentally added files
Conor Dooley (4):
dt-bindings: pwm: fix microchip corePWM's pwm-cells
riscv: dts: fix the icicle's #pwm-cells
pwm: add microchip soft ip corePWM driver
MAINTAINERS: add pwm to PolarFire SoC entry
.../bindings/pwm/microchip,corepwm.yaml | 4 +- MAINTAINERS | 1 + .../dts/microchip/mpfs-icicle-kit-fabric.dtsi | 2 +- drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-microchip-core.c | 397 ++++++++++++++++++ 6 files changed, 413 insertions(+), 2 deletions(-) create mode 100644 drivers/pwm/pwm-microchip-core.c
Signature · 1 line
2.37.3
Conor Dooley
maintainerPATCH v11 1/4 dt-bindings: pwm: fix microchip corePWM's pwm-cells
corePWM is capable of inverted operation but the binding requires \#pwm-cells of 2. Expand the binding to support setting the polarity.
--- Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml +3 -1
@@ -30,7 +30,9 @@ properties: maxItems: 1 "#pwm-cells": - const: 2 + enum: [2, 3] + description: + The only flag supported by the controller is PWM_POLARITY_INVERTED. microchip,sync-update-mask: description: |
Signature · 1 line
2.37.3
Conor Dooley
maintainerPATCH v11 2/4 riscv: dts: fix the icicle's #pwm-cells
In reply to Conor Dooley\#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.
--- arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi +1 -1
@@ -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"; };
Signature · 1 line
2.37.3
Conor Dooley
maintainerPATCH v11 3/4 pwm: add microchip soft ip corePWM driver
In reply to Conor DooleyAdd a driver that supports the Microchip FPGA "soft" PWM IP core.
--- drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-microchip-core.c | 397 +++++++++++++++++++++++++++++++ 3 files changed, 408 insertions(+) create mode 100644 drivers/pwm/pwm-microchip-core.c
drivers/pwm/Kconfig +10 -0
@@ -393,6 +393,16 @@ config PWM_MEDIATEK To compile this driver as a module, choose M here: the module will be called pwm-mediatek. +config PWM_MICROCHIP_CORE + tristate "Microchip corePWM PWM support" + depends on SOC_MICROCHIP_POLARFIRE || COMPILE_TEST + depends on HAS_IOMEM && OF + help + PWM driver for Microchip FPGA soft IP core. + + To compile this driver as a module, choose M here: the module + will be called pwm-microchip-core. + config PWM_MXS tristate "Freescale MXS PWM support" depends on ARCH_MXS || COMPILE_TEST
drivers/pwm/Makefile +1 -0
@@ -34,6 +34,7 @@ obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o obj-$(CONFIG_PWM_MESON) += pwm-meson.o obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o +obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o obj-$(CONFIG_PWM_MXS) += pwm-mxs.o obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o
drivers/pwm/pwm-microchip-core.c new file +397 -0
397 lines not on this page · raw message
Signature · 1 line
2.37.3
Re: [PATCH v11 3/4] pwm: add microchip soft ip corePWM driver
Hello,
Quoted 7 lines · Conor Dooley
[...]
drivers/pwm/pwm-microchip-core.c new file +6
-0+static u64 mchp_core_pwm_calc_duty (struct pwm_chip *chip,struct pwm_device *pwm, +const struct pwm_state *state,u8 prescale,u8 period_steps) +{ +struct mchp_core_pwm_chip *mchp_core_pwm =to_mchp_core_pwm (chip); +u64 duty_steps, tmp; +u16 prescale_val =PREG_TO_VAL (prescale);
drivers/pwm/pwm-microchip-core.c line 118
Quoted 10 lines
drivers/pwm/pwm-microchip-core.c new file +10
-0+ +/* +* Calculate the duty cycle in multiples of the prescaled period: +* duty_steps = duty_in_ns / step_in_ns +* step_in_ns = (prescale * NSEC_PER_SEC) / clk_rate +* The code below is rearranged slightly to only divide once. +*/ + duty_steps = state->duty_cycle *clk_get_rate (mchp_core_pwm->clk); + tmp = prescale_val *NSEC_PER_SEC ; +return div64_u64 (duty_steps, tmp);
The assignment to duty_steps can overflow. So you have to use mul_u64_u64_div_u64 here, too.
Quoted 55 lines
drivers/pwm/pwm-microchip-core.c new file +54
-0+} + + +static int mchp_core_pwm_apply_locked (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; + +if (!state->enabled) { +mchp_core_pwm_enable (chip, pwm, false, current_state.period); +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, &prescale, &period_steps); + 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 )) +return -EINVAL ; + +/* +* It is possible that something could have set the period_steps +* register to 0xff, which would prevent us from setting a 100% +* or 0% relative duty cycle, as explained above in +* mchp_core_pwm_calc_period(). +* The period is locked and we cannot change this, so we abort. +*/ +if (hw_period_steps ==MCHPCOREPWM_PERIOD_STEPS_MAX ) +return -EINVAL ; + + prescale = hw_prescale; + period_steps = hw_period_steps; + }else {
drivers/pwm/pwm-microchip-core.c line 260
Quoted 10 lines
drivers/pwm/pwm-microchip-core.c new file +10
-0+int ret; + + ret =mchp_core_pwm_calc_period (chip, state, &prescale, &period_steps); +if (ret) +return ret; + +mchp_core_pwm_apply_period (mchp_core_pwm, prescale, period_steps); + } + + duty_steps =mchp_core_pwm_calc_duty (chip, pwm, state, prescale, period_steps);
Both mchp_core_pwm_calc_period and mchp_core_pwm_calc_duty call clk_get_rate(), I suggest call this only once and pass the rate to these two functions.
Both branches of the if above start with calling mchp_core_pwm_calc_period, this could be simplified, too. (Hmm, in exactly one of them you check the return code, wouldn't that be sensible for both callers?)
Quoted 5 lines
drivers/pwm/pwm-microchip-core.c new file +5
-0+ +/* +* 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.
drivers/pwm/pwm-microchip-core.c line 275
Quoted 10 lines
drivers/pwm/pwm-microchip-core.c new file +10
-0+*/ +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); + +return 0 ; +}
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v11 3/4] pwm: add microchip soft ip corePWM driver
Quoted 1 line · Uwe Kleine-König
Hello,
Hello! Thanks for the review Uwe :)
Quoted 59 lines · Conor Dooley
Quoted 58 lines
drivers/pwm/pwm-microchip-core.c new file +57
-0+static int mchp_core_pwm_apply_locked (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; + +if (!state->enabled) { +mchp_core_pwm_enable (chip, pwm, false, current_state.period); +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, &prescale, &period_steps); + 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 )) +return -EINVAL ; + +/* +* It is possible that something could have set the period_steps +* register to 0xff, which would prevent us from setting a 100% +* or 0% relative duty cycle, as explained above in +* mchp_core_pwm_calc_period(). +* The period is locked and we cannot change this, so we abort. +*/ +if (hw_period_steps ==MCHPCOREPWM_PERIOD_STEPS_MAX ) +return -EINVAL ; + + prescale = hw_prescale; + period_steps = hw_period_steps; + }else { +int ret; + + ret =mchp_core_pwm_calc_period (chip, state, &prescale, &period_steps); +if (ret) +return ret; +
Quoted 8 lines
drivers/pwm/pwm-microchip-core.c line 260
Quoted 4 lines
drivers/pwm/pwm-microchip-core.c new file +4
-0+mchp_core_pwm_apply_period (mchp_core_pwm, prescale, period_steps); + } + + duty_steps =mchp_core_pwm_calc_duty (chip, pwm, state, prescale, period_steps);Both
mchp_core_pwm_calc_periodandmchp_core_pwm_calc_dutycallclk_get_rate(), I suggest call this only once and pass the rate to these two functions.
Sure. I think the signatures of both of those functions could be reduced in the process which would be nice.
Quoted 2 lines
Both branches of the if above start with calling
mchp_core_pwm_calc_period, this could be simplified, too.
ret = mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps);
if (ret)
return ret;
period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm);
if (period_locked) {
u16 hw_prescale;
u8 hw_period_steps;
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))
return -EINVAL;
/*
* It is possible that something could have set the period_steps
* register to 0xff, which would prevent us from setting a 100%
* or 0% relative duty cycle, as explained above in
* mchp_core_pwm_calc_period().
* The period is locked and we cannot change this, so we abort.
*/
if (hw_period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX)
return -EINVAL;
prescale = hw_prescale;
period_steps = hw_period_steps;
} else {
mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps);
}
duty_steps = mchp_core_pwm_calc_duty(chip, pwm, state, prescale, period_steps);I'll aim for something like the (absolutely untested) above then when I respin.
Quoted 3 lines
(Hmm, in
exactly one of them you check the return code, wouldn't that be sensible for both callers?)
Been messing with rust a bit of late, I love the #[must_use] attribute. Looks to be an oversight since it's only going to return an error if the clock rate exceeds what the FPGA is actually capable of.
Thanks again,
Conor.
Conor Dooley
maintainerPATCH v11 4/4 MAINTAINERS: add pwm to PolarFire SoC entry
In reply to Conor DooleyAdd the newly introduced pwm driver to the existing PolarFire SoC entry.
--- MAINTAINERS | 1 + 1 file changed, 1 insertion(+)
MAINTAINERS +1 -0
@@ -17532,6 +17532,7 @@ F: drivers/char/hw_random/mpfs-rng.c F: drivers/clk/microchip/clk-mpfs.c F: drivers/mailbox/mailbox-mpfs.c F: drivers/pci/controller/pcie-microchip-host.c +F: drivers/pwm/pwm-microchip-core.c F: drivers/rtc/rtc-mpfs.c F: drivers/soc/microchip/ F: drivers/spi/spi-microchip-core.c
Signature · 1 line
2.37.3
Re: [PATCH v11 4/4] MAINTAINERS: add pwm to PolarFire SoC entry
Quoted 3 lines · Conor Dooley
Add the newly introduced pwm driver to the existing PolarFire SoC entry.
Signed-off-by Conor Dooley <[email protected]>
I assume you will rework the series and resend this one with the driver patche. Applying patch #4 alone doesn't make sense, so I'm marking this one as "changes requested", too, in the PWM patchwork instance.
IMHO patches #1 and #2 make sense to be applied already without the driver given the binding is already there. I assume they will go in via the riscv tree, so I will mark these two as "handled elsewhere".
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Conor Dooley
maintainerRe: [PATCH v11 4/4] MAINTAINERS: add pwm to PolarFire SoC entry
Quoted 7 lines · Uwe Kleine-König
Quoted 3 lines · Conor Dooley
Add the newly introduced pwm driver to the existing PolarFire SoC entry.
Signed-off-by Conor Dooley <[email protected]>Acked-by Uwe Kleine-König <[email protected]>
Quoted 7 lines
I assume you will rework the series and resend this one with the driver patche. Applying patch #4 alone doesn't make sense, so I'm marking this one as "changes requested", too, in the PWM patchwork instance.
IMHO patches #1 and #2 make sense to be applied already without the driver given the binding is already there. I assume they will go in via the riscv tree, so I will mark these two as "handled elsewhere".
Right. Makes sense to me - I'll take the dt-binding & the dt via the riscv (or soc, we're changing things up there [a]) tree.
Thanks,
Conor.
[a] - https://lore.kernel.org/linux-riscv/mhng-e4210f56-fcc3-4db8-abdb-d43b3ebe695d@palmer-ri-x1c9a/
Conor Dooley
AppliedRe: (subset) [PATCH v11 0/4] Microchip soft ip corePWM driver
In reply to Conor DooleyFrom: Conor Dooley <[email protected]>
Quoted 8 lines · Conor Dooley
Hey Uwe, all,
~6.0-rc1 has rolled around so here is the promised v8v9~. v11 is based on 6.0 stuff still & there will be a change to the dts patch in v6.1, but I did a test merge and there was nothing to resolve. I'll take the dts change myself just to be on the safe side.
[...]
Applied to dt-for-next, thanks!
[1/4] dt-bindings: pwm: fix microchip corePWM's pwm-cells
https://git.kernel.org/conor/c/a62d196e8988 [2/4] riscv: dts: fix the icicle's #pwm-cells https://git.kernel.org/conor/c/ac2bcd194cc5
Thanks,
Conor.
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv