[PATCH v10 0/4] Microchip soft ip corePWM driver

[PATCH v10 0/4] Microchip soft ip corePWM driver

[Conor Dooley]

Hey Uwe, all,

6.0-rc1 has rolled around so here is the ~promised v8~~v9~ v10.
The pre 6.0-rc1 cover letter/series is here:
https://lore.kernel.org/linux-pwm/20220721172109.941900-1-mail@conchuod.ie
I'll take the dts change myself once the rest is merged.

There is one change here that is not directly from your feedback, I
added a test for invalid PERIOD_STEPS values, in which case we abort if
the period is locked and cannot be fixed. Hopefully the rounding is not
ruined..

Thanks,
Conor.

Changes since v9:
- added 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/20220709160206.cw5luo7kxdshoiua@pengutronix.de/

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              | 402 ++++++++++++++++++
 6 files changed, 418 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pwm/pwm-microchip-core.c


base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
-- 
2.36.1

[PATCH v10 1/4] dt-bindings: pwm: fix microchip corePWM's pwm-cells

[Conor Dooley]

corePWM is capable of inverted operation but the binding requires
\#pwm-cells of 2. Expand the binding to support setting the polarity.

Fixes: df77f7735786 ("dt-bindings: pwm: add microchip corepwm binding")
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml b/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
index a7fae1772a81..cd8e9a8907f8 100644
--- a/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
@@ -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: |
-- 
2.36.1

[PATCH v10 2/4] riscv: dts: fix the icicle's #pwm-cells

[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.

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";
 	};
-- 
2.36.1

Re: [PATCH v10 2/4] riscv: dts: fix the icicle's #pwm-cells

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v10 2/4] riscv: dts: fix the icicle's #pwm-cells

[Conor.Dooley]

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.

[PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..e4de8c02c3c0 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7bf1a29f02b8..a65625359ece 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -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
diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c
new file mode 100644
index 000000000000..4ec2f1fce600
--- /dev/null
+++ b/drivers/pwm/pwm-microchip-core.c
@@ -0,0 +1,402 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * corePWM driver for Microchip "soft" FPGA IP cores.
+ *
+ * Copyright (c) 2021-2022 Microchip Corporation. All rights reserved.
+ * Author: Conor Dooley <conor.dooley@microchip.com>
+ * Documentation:
+ * https://www.microsemi.com/document-portal/doc_download/1245275-corepwm-hb
+ *
+ * Limitations:
+ * - If the IP block is configured without "shadow registers", all register
+ *   writes will take effect immediately, causing glitches on the output.
+ *   If shadow registers *are* enabled, a write to the "SYNC_UPDATE" register
+ *   notifies the core that it needs to update the registers defining the
+ *   waveform from the contents of the "shadow registers".
+ * - The IP block has no concept of a duty cycle, only rising/falling edges of
+ *   the waveform. Unfortunately, if the rising & falling edges registers have
+ *   the same value written to them the IP block will do whichever of a rising
+ *   or a falling edge is possible. I.E. a 50% waveform at twice the requested
+ *   period. Therefore to get a 0% waveform, the output is set the max high/low
+ *   time depending on polarity.
+ * - The PWM period is set for the whole IP block not per channel. The driver
+ *   will only change the period if no other PWM output is enabled.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#define PREG_TO_VAL(PREG) ((PREG) + 1)
+
+#define MCHPCOREPWM_PRESCALE_MAX	0x100
+#define MCHPCOREPWM_PERIOD_STEPS_MAX	0xff
+#define MCHPCOREPWM_PERIOD_MAX		0xff00
+
+#define MCHPCOREPWM_PRESCALE	0x00
+#define MCHPCOREPWM_PERIOD	0x04
+#define MCHPCOREPWM_EN(i)	(0x08 + 0x04 * (i)) /* 0x08, 0x0c */
+#define MCHPCOREPWM_POSEDGE(i)	(0x10 + 0x08 * (i)) /* 0x10, 0x18, ..., 0x88 */
+#define MCHPCOREPWM_NEGEDGE(i)	(0x14 + 0x08 * (i)) /* 0x14, 0x1c, ..., 0x8c */
+#define MCHPCOREPWM_SYNC_UPD	0xe4
+
+struct mchp_core_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	struct mutex lock; /* protect the shared period */
+	void __iomem *base;
+	u32 sync_update_mask;
+	u16 channel_enabled;
+};
+
+static inline struct mchp_core_pwm_chip *to_mchp_core_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct mchp_core_pwm_chip, chip);
+}
+
+static void mchp_core_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
+				 bool enable, u64 period)
+{
+	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
+	u8 channel_enable, reg_offset, shift;
+
+	/*
+	 * There are two adjacent 8 bit control regs, the lower reg controls
+	 * 0-7 and the upper reg 8-15. Check if the pwm is in the upper reg
+	 * and if so, offset by the bus width.
+	 */
+	reg_offset = MCHPCOREPWM_EN(pwm->hwpwm >> 3);
+	shift = pwm->hwpwm & 7;
+
+	channel_enable = readb_relaxed(mchp_core_pwm->base + reg_offset);
+	channel_enable &= ~(1 << shift);
+	channel_enable |= (enable << shift);
+
+	writel_relaxed(channel_enable, mchp_core_pwm->base + reg_offset);
+	mchp_core_pwm->channel_enabled &= ~BIT(pwm->hwpwm);
+	mchp_core_pwm->channel_enabled |= enable << pwm->hwpwm;
+
+	/*
+	 * Notify the block to update the waveform from the shadow registers.
+	 * The updated values will not appear on the bus until they have been
+	 * applied to the waveform at the beginning of the next period. We must
+	 * write these registers and wait for them to be applied before
+	 * considering the channel enabled.
+	 * If the delay is under 1 us, sleep for at least 1 us anyway.
+	 */
+	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
+		u64 delay;
+
+		delay = div_u64(period, 1000u) ? : 1u;
+		writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
+		usleep_range(delay, delay * 2);
+	}
+}
+
+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);
+
+	/*
+	 * 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);
+}
+
+static void mchp_core_pwm_apply_duty(struct pwm_chip *chip, struct pwm_device *pwm,
+				     const struct pwm_state *state, u64 duty_steps, u8 period_steps)
+{
+	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
+	u8 posedge, negedge;
+	u8 period_steps_val = PREG_TO_VAL(period_steps);
+
+	/*
+	 * Setting posedge == negedge doesn't yield a constant output,
+	 * so that's an unsuitable setting to model duty_steps = 0.
+	 * In that case set the unwanted edge to a value that never
+	 * triggers.
+	 */
+	if (state->polarity == PWM_POLARITY_INVERSED) {
+		negedge = !duty_steps ? period_steps_val : 0u;
+		posedge = duty_steps;
+	} else {
+		posedge = !duty_steps ? period_steps_val : 0u;
+		negedge = duty_steps;
+	}
+
+	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
+	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
+}
+
+static int mchp_core_pwm_calc_period(struct pwm_chip *chip, const struct pwm_state *state,
+				     u8 *prescale, u8 *period_steps)
+{
+	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
+	u64 tmp, clk_rate;
+
+	/*
+	 * Calculate the period cycles and prescale values.
+	 * The registers are each 8 bits wide & multiplied to compute the period
+	 * using the formula:
+	 * (clock_period) * (prescale + 1) * (period_steps + 1)
+	 * so the maximum period that can be generated is 0x10000 times the
+	 * period of the input clock.
+	 * However, due to the design of the "hardware", it is not possible to
+	 * attain a 100% duty cycle if the full range of period_steps is used.
+	 * Therefore period_steps is restricted to 0xFE and the maximum multiple
+	 * of the clock period attainable is 0xFF00.
+	 */
+	clk_rate = clk_get_rate(mchp_core_pwm->clk);
+
+	/*
+	 * If clk_rate is too big, the following multiplication might overflow.
+	 * However this is implausible, as the fabric of current FPGAs cannot
+	 * provide clocks at a rate high enough.
+	 */
+	if (clk_rate >= NSEC_PER_SEC)
+		return -EINVAL;
+
+	tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC);
+
+	/*
+	 * The hardware adds one to the register value, so decrement by one to
+	 * account for the offset
+	 */
+	if (tmp >= MCHPCOREPWM_PERIOD_MAX) {
+		*prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
+		*period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
+		return 0;
+	}
+
+	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);
+	/* PREG_TO_VAL() can produce a value larger than UINT8_MAX */
+	*period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;
+
+	return 0;
+}
+
+static inline void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *mchp_core_pwm,
+					      u8 prescale, u8 period_steps)
+{
+	writel_relaxed(prescale, mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
+	writel_relaxed(period_steps, mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
+}
+
+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);
+		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
+		 * register to 0xff, which would prevent us from setting a 100%
+		 * duty cycle, as explained in the mchp_core_pwm_calc_period()
+		 * above.
+		 * The period is locked and we cannot change this, so we abort.
+		 */
+		if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX) {
+			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);
+		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.
+		 */
+		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;
+}
+
+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;
+	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) {
+		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));
+	}
+
+	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+
+	mutex_unlock(&mchp_core_pwm->lock);
+}
+
+static const struct pwm_ops mchp_core_pwm_ops = {
+	.apply = mchp_core_pwm_apply,
+	.get_state = mchp_core_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static const struct of_device_id mchp_core_of_match[] = {
+	{
+		.compatible = "microchip,corepwm-rtl-v4",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mchp_core_of_match);
+
+static int mchp_core_pwm_probe(struct platform_device *pdev)
+{
+	struct mchp_core_pwm_chip *mchp_pwm;
+	struct resource *regs;
+	int ret;
+
+	mchp_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_pwm), GFP_KERNEL);
+	if (!mchp_pwm)
+		return -ENOMEM;
+
+	mchp_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, &regs);
+	if (IS_ERR(mchp_pwm->base))
+		return PTR_ERR(mchp_pwm->base);
+
+	mchp_pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(mchp_pwm->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(mchp_pwm->clk),
+				     "failed to get PWM clock\n");
+
+	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
+				 &mchp_pwm->sync_update_mask))
+		mchp_pwm->sync_update_mask = 0u;
+
+	mutex_init(&mchp_pwm->lock);
+
+	mchp_pwm->chip.dev = &pdev->dev;
+	mchp_pwm->chip.ops = &mchp_core_pwm_ops;
+	mchp_pwm->chip.npwm = 16;
+
+	mchp_pwm->channel_enabled = readb_relaxed(mchp_pwm->base + MCHPCOREPWM_EN(0));
+	mchp_pwm->channel_enabled |= readb_relaxed(mchp_pwm->base + MCHPCOREPWM_EN(1)) << 8;
+
+	ret = devm_pwmchip_add(&pdev->dev, &mchp_pwm->chip);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+
+	return 0;
+}
+
+static struct platform_driver mchp_core_pwm_driver = {
+	.driver = {
+		.name = "mchp-core-pwm",
+		.of_match_table = mchp_core_of_match,
+	},
+	.probe = mchp_core_pwm_probe,
+};
+module_platform_driver(mchp_core_pwm_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
+MODULE_DESCRIPTION("corePWM driver for Microchip FPGAs");
-- 
2.36.1

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 22594 bytes --]

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
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 60d13a949bc5..e4de8c02c3c0 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -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
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 7bf1a29f02b8..a65625359ece 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -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
> diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c
> new file mode 100644
> index 000000000000..4ec2f1fce600
> --- /dev/null
> +++ b/drivers/pwm/pwm-microchip-core.c
> @@ -0,0 +1,402 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * corePWM driver for Microchip "soft" FPGA IP cores.
> + *
> + * Copyright (c) 2021-2022 Microchip Corporation. All rights reserved.
> + * Author: Conor Dooley <conor.dooley@microchip.com>
> + * Documentation:
> + * https://www.microsemi.com/document-portal/doc_download/1245275-corepwm-hb
> + *
> + * Limitations:
> + * - If the IP block is configured without "shadow registers", all register
> + *   writes will take effect immediately, causing glitches on the output.
> + *   If shadow registers *are* enabled, a write to the "SYNC_UPDATE" register
> + *   notifies the core that it needs to update the registers defining the
> + *   waveform from the contents of the "shadow registers".
> + * - The IP block has no concept of a duty cycle, only rising/falling edges of
> + *   the waveform. Unfortunately, if the rising & falling edges registers have
> + *   the same value written to them the IP block will do whichever of a rising
> + *   or a falling edge is possible. I.E. a 50% waveform at twice the requested
> + *   period. Therefore to get a 0% waveform, the output is set the max high/low
> + *   time depending on polarity.
> + * - The PWM period is set for the whole IP block not per channel. The driver
> + *   will only change the period if no other PWM output is enabled.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +#define PREG_TO_VAL(PREG) ((PREG) + 1)
> +
> +#define MCHPCOREPWM_PRESCALE_MAX	0x100
> +#define MCHPCOREPWM_PERIOD_STEPS_MAX	0xff
> +#define MCHPCOREPWM_PERIOD_MAX		0xff00
> +
> +#define MCHPCOREPWM_PRESCALE	0x00
> +#define MCHPCOREPWM_PERIOD	0x04
> +#define MCHPCOREPWM_EN(i)	(0x08 + 0x04 * (i)) /* 0x08, 0x0c */
> +#define MCHPCOREPWM_POSEDGE(i)	(0x10 + 0x08 * (i)) /* 0x10, 0x18, ..., 0x88 */
> +#define MCHPCOREPWM_NEGEDGE(i)	(0x14 + 0x08 * (i)) /* 0x14, 0x1c, ..., 0x8c */
> +#define MCHPCOREPWM_SYNC_UPD	0xe4
> +
> +struct mchp_core_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	struct mutex lock; /* protect the shared period */
> +	void __iomem *base;
> +	u32 sync_update_mask;
> +	u16 channel_enabled;
> +};
> +
> +static inline struct mchp_core_pwm_chip *to_mchp_core_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct mchp_core_pwm_chip, chip);
> +}
> +
> +static void mchp_core_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> +				 bool enable, u64 period)
> +{
> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> +	u8 channel_enable, reg_offset, shift;
> +
> +	/*
> +	 * There are two adjacent 8 bit control regs, the lower reg controls
> +	 * 0-7 and the upper reg 8-15. Check if the pwm is in the upper reg
> +	 * and if so, offset by the bus width.
> +	 */
> +	reg_offset = MCHPCOREPWM_EN(pwm->hwpwm >> 3);
> +	shift = pwm->hwpwm & 7;
> +
> +	channel_enable = readb_relaxed(mchp_core_pwm->base + reg_offset);
> +	channel_enable &= ~(1 << shift);
> +	channel_enable |= (enable << shift);
> +
> +	writel_relaxed(channel_enable, mchp_core_pwm->base + reg_offset);
> +	mchp_core_pwm->channel_enabled &= ~BIT(pwm->hwpwm);
> +	mchp_core_pwm->channel_enabled |= enable << pwm->hwpwm;
> +
> +	/*
> +	 * Notify the block to update the waveform from the shadow registers.
> +	 * The updated values will not appear on the bus until they have been
> +	 * applied to the waveform at the beginning of the next period. We must
> +	 * write these registers and wait for them to be applied before
> +	 * considering the channel enabled.
> +	 * If the delay is under 1 us, sleep for at least 1 us anyway.
> +	 */
> +	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
> +		u64 delay;
> +
> +		delay = div_u64(period, 1000u) ? : 1u;
> +		writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
> +		usleep_range(delay, delay * 2);
> +	}
> +}
> +
> +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);
> +
> +	/*
> +	 * 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);
> +}
> +
> +static void mchp_core_pwm_apply_duty(struct pwm_chip *chip, struct pwm_device *pwm,
> +				     const struct pwm_state *state, u64 duty_steps, u8 period_steps)
> +{
> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> +	u8 posedge, negedge;
> +	u8 period_steps_val = PREG_TO_VAL(period_steps);
> +
> +	/*
> +	 * Setting posedge == negedge doesn't yield a constant output,
> +	 * so that's an unsuitable setting to model duty_steps = 0.
> +	 * In that case set the unwanted edge to a value that never
> +	 * triggers.
> +	 */
> +	if (state->polarity == PWM_POLARITY_INVERSED) {
> +		negedge = !duty_steps ? period_steps_val : 0u;
> +		posedge = duty_steps;
> +	} else {
> +		posedge = !duty_steps ? period_steps_val : 0u;
> +		negedge = duty_steps;
> +	}
> +
> +	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
> +	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
> +}
> +
> +static int mchp_core_pwm_calc_period(struct pwm_chip *chip, const struct pwm_state *state,
> +				     u8 *prescale, u8 *period_steps)
> +{
> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> +	u64 tmp, clk_rate;
> +
> +	/*
> +	 * Calculate the period cycles and prescale values.
> +	 * The registers are each 8 bits wide & multiplied to compute the period
> +	 * using the formula:
> +	 * (clock_period) * (prescale + 1) * (period_steps + 1)
> +	 * so the maximum period that can be generated is 0x10000 times the
> +	 * period of the input clock.
> +	 * However, due to the design of the "hardware", it is not possible to
> +	 * attain a 100% duty cycle if the full range of period_steps is used.
> +	 * Therefore period_steps is restricted to 0xFE and the maximum multiple
> +	 * of the clock period attainable is 0xFF00.
> +	 */
> +	clk_rate = clk_get_rate(mchp_core_pwm->clk);
> +
> +	/*
> +	 * If clk_rate is too big, the following multiplication might overflow.
> +	 * However this is implausible, as the fabric of current FPGAs cannot
> +	 * provide clocks at a rate high enough.
> +	 */
> +	if (clk_rate >= NSEC_PER_SEC)
> +		return -EINVAL;
> +
> +	tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC);
> +
> +	/*
> +	 * The hardware adds one to the register value, so decrement by one to
> +	 * account for the offset
> +	 */
> +	if (tmp >= MCHPCOREPWM_PERIOD_MAX) {
> +		*prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
> +		*period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
> +		return 0;
> +	}
> +
> +	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);
> +	/* PREG_TO_VAL() can produce a value larger than UINT8_MAX */
> +	*period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;
> +
> +	return 0;
> +}
> +
> +static inline void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *mchp_core_pwm,
> +					      u8 prescale, u8 period_steps)
> +{
> +	writel_relaxed(prescale, mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
> +	writel_relaxed(period_steps, mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
> +}
> +
> +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.

> +		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/
> +		 * register to 0xff, which would prevent us from setting a 100%

For my understanding: It would also prevent a 0% relative duty, right?

> +		 * 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?

> +			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.

> +		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:

diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c
index 4ec2f1fce600..d1578d73818f 100644
--- a/drivers/pwm/pwm-microchip-core.c
+++ b/drivers/pwm/pwm-microchip-core.c
@@ -197,8 +197,8 @@ static inline void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *mchp_co
 	writel_relaxed(period_steps, mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
 }
 
-static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			       const struct pwm_state *state)
+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;
@@ -208,11 +208,8 @@ static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	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;
 	}
 
@@ -236,10 +233,8 @@ static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		hw_period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
 
 		if ((period_steps + 1) * (prescale + 1) <
-		    (hw_period_steps + 1) * (hw_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
@@ -248,19 +243,16 @@ static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		 * above.
 		 * The period is locked and we cannot change this, so we abort.
 		 */
-		if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX) {
+		if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX)
-			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);
-		if (ret) {
+		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);
@@ -292,11 +284,24 @@ static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	mchp_core_pwm_enable(chip, pwm, true, state->period);
 
-	mutex_unlock(&mchp_core_pwm->lock);
-
 	return 0;
 }
 
+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);
+	int ret;
+
+	mutex_lock(&mchp_core_pwm->lock);
+
+	ret = mchp_core_pwm_apply_locked(chip, pwm, state);
+
+	mutex_unlock(&mchp_core_pwm->lock);
+
+	return ret;
+}
+
 static void mchp_core_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 				    struct pwm_state *state)
 {

The diffstat is negative, so maybe that's subjective.

> +}
> +
> +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.

> +	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?

> +		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.

> +}
> +
> +static const struct pwm_ops mchp_core_pwm_ops = {
> +	.apply = mchp_core_pwm_apply,
> +	.get_state = mchp_core_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id mchp_core_of_match[] = {
> +	{
> +		.compatible = "microchip,corepwm-rtl-v4",
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mchp_core_of_match);
> +
> +static int mchp_core_pwm_probe(struct platform_device *pdev)
> +{
> +	struct mchp_core_pwm_chip *mchp_pwm;
> +	struct resource *regs;
> +	int ret;
> +
> +	mchp_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_pwm), GFP_KERNEL);
> +	if (!mchp_pwm)
> +		return -ENOMEM;
> +
> +	mchp_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, &regs);
> +	if (IS_ERR(mchp_pwm->base))
> +		return PTR_ERR(mchp_pwm->base);
> +
> +	mchp_pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(mchp_pwm->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(mchp_pwm->clk),
> +				     "failed to get PWM clock\n");
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
> +				 &mchp_pwm->sync_update_mask))
> +		mchp_pwm->sync_update_mask = 0u;

That u suffix is unusual. I'd drop it.

> +	mutex_init(&mchp_pwm->lock);
> +
> +	mchp_pwm->chip.dev = &pdev->dev;
> +	mchp_pwm->chip.ops = &mchp_core_pwm_ops;
> +	mchp_pwm->chip.npwm = 16;
> +
> +	mchp_pwm->channel_enabled = readb_relaxed(mchp_pwm->base + MCHPCOREPWM_EN(0));
> +	mchp_pwm->channel_enabled |= readb_relaxed(mchp_pwm->base + MCHPCOREPWM_EN(1)) << 8;
> +
> +	ret = devm_pwmchip_add(&pdev->dev, &mchp_pwm->chip);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mchp_core_pwm_driver = {
> +	.driver = {
> +		.name = "mchp-core-pwm",
> +		.of_match_table = mchp_core_of_match,
> +	},
> +	.probe = mchp_core_pwm_probe,
> +};
> +module_platform_driver(mchp_core_pwm_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
> +MODULE_DESCRIPTION("corePWM driver for Microchip FPGAs");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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.

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 8633 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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.

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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.

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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.

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2437 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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.

Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v10 4/4] MAINTAINERS: add pwm to PolarFire SoC entry

[Conor Dooley]

Add the newly introduced pwm driver to the existing PolarFire SoC entry.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8a5012ba6ff9..5db66c743595 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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
-- 
2.36.1

Re: [PATCH v10 4/4] MAINTAINERS: add pwm to PolarFire SoC entry

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 446 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]