[PATCH v13 0/2] Microchip Soft IP corePWM driver

[PATCH v13 0/2] Microchip Soft IP corePWM driver

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Hey,

v13 is rebased on top of Uwe's series converting get_state() to return
int instead of void. Given other changes here, it turns out that I do
not need it at all unfortunately!

I was wrong about the behaviour of the sync-update bit:
It /does not/ get cleared at the start of a new period. I did actually
modify the driver to do a read_poll_timeout() on that bit which seemed
to work [0], but it turns out that that bit holds it's value until the
IP block is reset. I'm really not sure how it worked when I tested the
other week...

I initially implemented a wait with timers and a timeout - but Uwe
suggested dropping the timeouts entirely & instead going back to
waiting. The driver now waits in apply() & get_state() for a update to
have been applied to the waveform before allowing further modification.
Doing so is required as the applied state doesn't appear in the
registers until it has appeared at the output.
I yoinked the msleep/udelay stuff from the sun4i that you mentioned &
modified it a little as the cost of waiting has been placed on
subsequent calls.

Other than that, v13 has some minor bits of cleanup that Uwe suggested
in mchp_core_pwm_apply_locked().

Thanks,
Conor.

0 - https://lore.kernel.org/linux-riscv/Y3uZY5mt%2FZIWk3sS@wendy/

Changes since v13:
- couple bits of cleanup to apply_locked(), suggested by Uwe: getting
  rid of a variable & using unsigned long for another.
- move the overhead waiting for a change to be applied, for channels
  with shadow registers, to subsequent calls to apply(). This has the
  benefit of only waiting when two calls to apply() are close in time
  rather than eating the delay in every call.

Changes since v11:
- swap a "bare" multiply & divide for the corresponding helper to
  prevent overflow
- factor out duplicate clk rate acquisition & period calculation
- make the period calculation return void by checking the validity of
  the clock rate in the caller
- drop the binding & dt patch, they're on-track for v6.2 via my tree

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/20220709160206.cw5luo7kxdshoiua@pengutronix.de/

Changes from v4:
- dropped some accidentally added files

Changes before v4:
https://lore.kernel.org/linux-pwm/20220721172109.941900-1-mail@conchuod.ie

Conor Dooley (2):
  pwm: add microchip soft ip corePWM driver
  MAINTAINERS: add pwm to PolarFire SoC entry

 MAINTAINERS                      |   1 +
 drivers/pwm/Kconfig              |  10 +
 drivers/pwm/Makefile             |   1 +
 drivers/pwm/pwm-microchip-core.c | 436 +++++++++++++++++++++++++++++++
 4 files changed, 448 insertions(+)
 create mode 100644 drivers/pwm/pwm-microchip-core.c

-- 
2.38.1

[PATCH v13 1/2] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

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 | 436 +++++++++++++++++++++++++++++++
 3 files changed, 447 insertions(+)
 create mode 100644 drivers/pwm/pwm-microchip-core.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index dae023d783a2..f42756a014ed 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..047fa708b9fc
--- /dev/null
+++ b/drivers/pwm/pwm-microchip-core.c
@@ -0,0 +1,436 @@
+// 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/ktime.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
+#define MCHPCOREPWM_TIMEOUT_MS	100u
+
+struct mchp_core_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	void __iomem *base;
+	struct mutex lock; /* protect the shared period */
+	ktime_t update_timestamp;
+	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.
+	 * This is a NO-OP if the channel does not have shadow registers.
+	 */
+	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm))
+		mchp_core_pwm->update_timestamp = ktime_add_ns(ktime_get(), period);
+}
+
+static void mchp_core_pwm_wait_for_sync_update(struct mchp_core_pwm_chip *mchp_core_pwm,
+					       unsigned int channel)
+{
+	/*
+	 * If a shadow register is used for this PWM channel, and iff there is
+	 * a pending update to the waveform, we must wait for it to be applied
+	 * before attempting to read its state. Reading the registers yields
+	 * the currently implemented settings & the new ones are only readable
+	 * once the current period has ended.
+	 */
+
+	if (mchp_core_pwm->sync_update_mask & (1 << channel)) {
+		ktime_t current_time = ktime_get();
+		s64 remaining_ns;
+		u32 delay_us;
+
+		remaining_ns = ktime_to_ns(ktime_sub(mchp_core_pwm->update_timestamp,
+						     current_time));
+
+		/*
+		 * If the update has gone through, don't bother waiting for
+		 * obvious reasons. Otherwise wait around for an appropriate
+		 * amount of time for the update to go through.
+		 */
+		if (remaining_ns <= 0)
+			return;
+
+		delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC);
+		if ((delay_us / 1000) > MAX_UDELAY_MS)
+			msleep(delay_us / 1000 + 1);
+		else
+			usleep_range(delay_us, delay_us * 2);
+	}
+}
+
+static u64 mchp_core_pwm_calc_duty(const struct pwm_state *state, u64 clk_rate,
+				   u8 prescale, u8 period_steps)
+{
+	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.
+	 */
+	tmp = prescale_val * NSEC_PER_SEC;
+	duty_steps = mul_u64_u64_div_u64(state->duty_cycle, clk_rate, tmp);
+
+	return duty_steps;
+}
+
+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 void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
+				      u16 *prescale, u8 *period_steps)
+{
+	u64 tmp;
+
+	/*
+	 * 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.
+	 */
+	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;
+	}
+
+	*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;
+}
+
+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_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);
+	bool period_locked;
+	unsigned long clk_rate;
+	u64 duty_steps;
+	u16 prescale;
+	u8 period_steps;
+
+	if (!state->enabled) {
+		mchp_core_pwm_enable(chip, pwm, false, pwm->state.period);
+		return 0;
+	}
+
+	/*
+	 * 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.
+	 */
+	clk_rate = clk_get_rate(mchp_core_pwm->clk);
+	if (clk_rate >= NSEC_PER_SEC)
+		return -EINVAL;
+
+	mchp_core_pwm_calc_period(state, clk_rate, &prescale, &period_steps);
+
+	/*
+	 * 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;
+
+		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(state, clk_rate, 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, pwm->state.period);
+
+	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);
+
+	mchp_core_pwm_wait_for_sync_update(mchp_core_pwm, pwm->hwpwm);
+
+	ret = mchp_core_pwm_apply_locked(chip, pwm, state);
+
+	mutex_unlock(&mchp_core_pwm->lock);
+
+	return ret;
+}
+
+static int 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);
+	u64 rate;
+	u16 prescale;
+	u8 period_steps, duty_steps, posedge, negedge;
+
+	mutex_lock(&mchp_core_pwm->lock);
+
+	mchp_core_pwm_wait_for_sync_update(mchp_core_pwm, pwm->hwpwm);
+
+	if (mchp_core_pwm->channel_enabled & (1 << pwm->hwpwm))
+		state->enabled = true;
+	else
+		state->enabled = false;
+
+	rate = clk_get_rate(mchp_core_pwm->clk);
+
+	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;
+	state->period *= NSEC_PER_SEC;
+	state->period = DIV64_U64_ROUND_UP(state->period, rate);
+
+	posedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
+	negedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
+
+	mutex_unlock(&mchp_core_pwm->lock);
+
+	if (negedge == posedge) {
+		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, rate);
+	}
+
+	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+
+	return 0;
+}
+
+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_core_pwm;
+	struct resource *regs;
+	int ret;
+
+	mchp_core_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_core_pwm), GFP_KERNEL);
+	if (!mchp_core_pwm)
+		return -ENOMEM;
+
+	mchp_core_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, &regs);
+	if (IS_ERR(mchp_core_pwm->base))
+		return PTR_ERR(mchp_core_pwm->base);
+
+	mchp_core_pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(mchp_core_pwm->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(mchp_core_pwm->clk),
+				     "failed to get PWM clock\n");
+
+	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
+				 &mchp_core_pwm->sync_update_mask))
+		mchp_core_pwm->sync_update_mask = 0;
+
+	mutex_init(&mchp_core_pwm->lock);
+
+	mchp_core_pwm->chip.dev = &pdev->dev;
+	mchp_core_pwm->chip.ops = &mchp_core_pwm_ops;
+	mchp_core_pwm->chip.npwm = 16;
+
+	mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0));
+	mchp_core_pwm->channel_enabled |=
+		readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(1)) << 8;
+
+	ret = devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+
+	/*
+	 * Enabled synchronous update for channels with shadow registers
+	 * enabled. For channels without shadow registers, this has no effect
+	 * at all so is unconditionally enabled.
+	 */
+	writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
+	mchp_core_pwm->update_timestamp = ktime_get();
+
+	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.38.1

[PATCH v13 2/2] MAINTAINERS: add pwm to PolarFire SoC entry

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

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 7729a30b9609..e459d539fdd1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17979,6 +17979,7 @@ F:	drivers/clk/microchip/clk-mpfs.c
 F:	drivers/i2c/busses/i2c-microchip-corei2c.c
 F:	drivers/mailbox/mailbox-mpfs.c
 F:	drivers/pci/controller/pcie-microchip-host.c
+F:	drivers/pwm/pwm-microchip-core.c
 F:	drivers/reset/reset-mpfs.c
 F:	drivers/rtc/rtc-mpfs.c
 F:	drivers/soc/microchip/mpfs-sys-controller.c
-- 
2.38.1

Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

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

Hello Conor,

On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> 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 | 436 +++++++++++++++++++++++++++++++
>  3 files changed, 447 insertions(+)
>  create mode 100644 drivers/pwm/pwm-microchip-core.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index dae023d783a2..f42756a014ed 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..047fa708b9fc
> --- /dev/null
> +++ b/drivers/pwm/pwm-microchip-core.c
> @@ -0,0 +1,436 @@
> +// 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/ktime.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
> +#define MCHPCOREPWM_TIMEOUT_MS	100u
> +
> +struct mchp_core_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *base;
> +	struct mutex lock; /* protect the shared period */
> +	ktime_t update_timestamp;
> +	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.
> +	 * This is a NO-OP if the channel does not have shadow registers.
> +	 */
> +	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm))
> +		mchp_core_pwm->update_timestamp = ktime_add_ns(ktime_get(), period);
> +}
> +
> +static void mchp_core_pwm_wait_for_sync_update(struct mchp_core_pwm_chip *mchp_core_pwm,
> +					       unsigned int channel)
> +{
> +	/*
> +	 * If a shadow register is used for this PWM channel, and iff there is
> +	 * a pending update to the waveform, we must wait for it to be applied
> +	 * before attempting to read its state. Reading the registers yields
> +	 * the currently implemented settings & the new ones are only readable
> +	 * once the current period has ended.
> +	 */
> +
> +	if (mchp_core_pwm->sync_update_mask & (1 << channel)) {
> +		ktime_t current_time = ktime_get();
> +		s64 remaining_ns;
> +		u32 delay_us;
> +
> +		remaining_ns = ktime_to_ns(ktime_sub(mchp_core_pwm->update_timestamp,
> +						     current_time));
> +
> +		/*
> +		 * If the update has gone through, don't bother waiting for
> +		 * obvious reasons. Otherwise wait around for an appropriate
> +		 * amount of time for the update to go through.
> +		 */
> +		if (remaining_ns <= 0)
> +			return;
> +
> +		delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC);
> +		if ((delay_us / 1000) > MAX_UDELAY_MS)
> +			msleep(delay_us / 1000 + 1);

Is this better than

	msleep(DIV_ROUND_UP(delay_us, 1000);

? Also I wonder about your usage of MAX_UDELAY_MS. This is about
udelay() but you're using usleep_range()?

> +		else
> +			usleep_range(delay_us, delay_us * 2);

I wonder if there isn't a function that implements something like

	wait_until(mchp_core_pwm->update_timestamp);

which would be a bit nicer than doing this by hand. Maybe fsleep()?

> +	}
> +}
> +
> +[...]
> +
> +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;

IMHO

		negedge = duty_steps ? 0 : period_steps_val;

is a bit easier to parse.

> +		posedge = duty_steps;
> +	} else {
> +		posedge = !duty_steps ? period_steps_val : 0u;
> +		negedge = duty_steps;
> +	}

The following code is equivalent:

	u8 first_edge = 0, second_edge = duty_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 (duty_steps == 0)
		first_edge = period_steps_val;

	if (state->polarity == PWM_POLARITY_INVERSED) {
		negedge = first_edge;
		posedge = second_edge;
	} else {
		posedge = first_edge;
		negedge = second_edge;
	}

I'm not sure if it's easier to understand. What do you think?

> +	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
> +	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
> +}
> +
> +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> +				      u16 *prescale, u8 *period_steps)
> +{
> +	u64 tmp;
> +
> +	/*
> +	 * 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.
> +	 */
> +	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;
> +	}
> +
> +	*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;

This looks wrong, but I didn't think long about that. Did we discuss
this already and/or are you sure this is correct?

(We have:
	          (prescale + 1) * (period_steps + 1)
	period = ------------------------------------
	                       clk_rate

You calculate
	            period * clk_rate
	prescale = -------------------
	           NSEC_PER_SEC * 0xff

	                     period * clk_rate
	period_steps = ----------------------------- - 1
	               NSEC_PER_SEC * (prescale + 1)

assuming exact arithmetic putting these into the above equation we get:


    period * clk_rate                period * clk_rate
  (------------------- + 1) * (-----------------------------) / clk_rate
   NSEC_PER_SEC * 0xff         NSEC_PER_SEC * (prescale + 1)

and then substituting prescale this doesn't resolve to period, does it?
Correct me if I'm wrong.)

> +}
> +
> +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);
> +}

There is only one caller for this two-line function. I suggest to unroll it?

> [...]
> +static int mchp_core_pwm_probe(struct platform_device *pdev)
> +{
> +	struct mchp_core_pwm_chip *mchp_core_pwm;
> +	struct resource *regs;
> +	int ret;
> +
> +	mchp_core_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_core_pwm), GFP_KERNEL);
> +	if (!mchp_core_pwm)
> +		return -ENOMEM;
> +
> +	mchp_core_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, &regs);
> +	if (IS_ERR(mchp_core_pwm->base))
> +		return PTR_ERR(mchp_core_pwm->base);
> +
> +	mchp_core_pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(mchp_core_pwm->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(mchp_core_pwm->clk),
> +				     "failed to get PWM clock\n");
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
> +				 &mchp_core_pwm->sync_update_mask))
> +		mchp_core_pwm->sync_update_mask = 0;
> +
> +	mutex_init(&mchp_core_pwm->lock);
> +
> +	mchp_core_pwm->chip.dev = &pdev->dev;
> +	mchp_core_pwm->chip.ops = &mchp_core_pwm_ops;
> +	mchp_core_pwm->chip.npwm = 16;
> +
> +	mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0));
> +	mchp_core_pwm->channel_enabled |=
> +		readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(1)) << 8;
> +
> +	ret = devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +
> +	/*
> +	 * Enabled synchronous update for channels with shadow registers
> +	 * enabled. For channels without shadow registers, this has no effect
> +	 * at all so is unconditionally enabled.
> +	 */
> +	writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
> +	mchp_core_pwm->update_timestamp = ktime_get();

This needs to be done before devm_pwmchip_add().

> +
> +	return 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 v13 2/2] MAINTAINERS: add pwm to PolarFire SoC entry

[Uwe Kleine-König]

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

Hello,

On Wed, Dec 21, 2022 at 11:29:13AM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Add the newly introduced pwm driver to the existing PolarFire SoC entry.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

the patch looks fine, but as the first patch has to change, I'm
discarding this one from the PWM patchwork, too.

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 v13 1/2] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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

On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>

> > +		delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC);
> > +		if ((delay_us / 1000) > MAX_UDELAY_MS)
> > +			msleep(delay_us / 1000 + 1);
> 
> Is this better than
> 
> 	msleep(DIV_ROUND_UP(delay_us, 1000);
> 
> ? Also I wonder about your usage of MAX_UDELAY_MS. This is about

I probably started hacking on the example you gave and didn't notice
the U. What I have here is ~what you suggested last time.

> udelay() but you're using usleep_range()?
> 
> > +		else
> > +			usleep_range(delay_us, delay_us * 2);
> 
> I wonder if there isn't a function that implements something like
> 
> 	wait_until(mchp_core_pwm->update_timestamp);
> 
> which would be a bit nicer than doing this by hand. Maybe fsleep()?

That'd be fsleep(delay_us), but does at least clean up some of the
messing.

> > +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;
> 
> IMHO
> 
> 		negedge = duty_steps ? 0 : period_steps_val;
> 
> is a bit easier to parse.
> 
> > +		posedge = duty_steps;
> > +	} else {
> > +		posedge = !duty_steps ? period_steps_val : 0u;
> > +		negedge = duty_steps;
> > +	}
> 
> The following code is equivalent:
> 
> 	u8 first_edge = 0, second_edge = duty_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 (duty_steps == 0)
> 		first_edge = period_steps_val;
> 
> 	if (state->polarity == PWM_POLARITY_INVERSED) {
> 		negedge = first_edge;
> 		posedge = second_edge;
> 	} else {
> 		posedge = first_edge;
> 		negedge = second_edge;
> 	}
> 
> I'm not sure if it's easier to understand. What do you think?

Despite having used them, I dislike ternary statements.

> > +	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
> > +	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
> > +}
> > +
> > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > +				      u16 *prescale, u8 *period_steps)
> > +{
> > +	u64 tmp;
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> > +	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;
> > +	}
> > +
> > +	*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;
> 
> This looks wrong, but I didn't think long about that. Did we discuss
> this already and/or are you sure this is correct?

We did discuss it previously AFAICT;
https://lore.kernel.org/linux-pwm/896d73ac-05af-8673-8379-29011800be83@microchip.com/

In that version of the code, prescale_val meant the mathematical value
used for calculations & "prescale" was the value written into the
register.

Now, we have ditched prescale_val and operate directly with what gets
written into the register.

I ran a test case through the calculation, and it seemed to work out?

> (We have:
> 	          (prescale + 1) * (period_steps + 1)
> 	period = ------------------------------------
> 	                       clk_rate
> 
> You calculate
> 	            period * clk_rate
> 	prescale = -------------------
> 	           NSEC_PER_SEC * 0xff

Say period = 2000 ns, clk_rate = 62.5 Mhz, giving a register value for
prescale of 0.49019... 

> 	                     period * clk_rate
> 	period_steps = ----------------------------- - 1
> 	               NSEC_PER_SEC * (prescale + 1)

Same numbers, but we use the PREG_TO_VAL() macro so the mathematical value
is 1.49019.

     2000 * 62.5E6
--------------------- - 1 = 82.88360....
  1E9 * (0.49016 + 1)

> 
> assuming exact arithmetic putting these into the above equation we get:
> 
> 
>     period * clk_rate                period * clk_rate
>   (------------------- + 1) * (-----------------------------) / clk_rate
>    NSEC_PER_SEC * 0xff         NSEC_PER_SEC * (prescale + 1)
> 
> and then substituting prescale this doesn't resolve to period, does it?
> Correct me if I'm wrong.)

(0.49016 + 1) * (82.88360 + 1)      124.99...
------------------------------ = ------------- = 0.00000199999
            62.5E6                  62.5E6

And then accounting for that fact that 2000 was really 2000E-9,
we arrive back where we started, give or take some rounding?

Doing that with integer maths works out more cleanly since 0.49016
becomes 0.
   2000 * 62.5E6
------------------ - 1 = 124
  1E9 * (0 + 1)

(0 + 1) * (124 + 1)      125
------------------- = --------- = 0.000002
      62.5E6            62.5E6

Unfortunately, I don't think I am seeing what you're seeing.

>     period * clk_rate                period * clk_rate
>   (------------------- + 1) * (-----------------------------) / clk_rate
>    NSEC_PER_SEC * 0xff         NSEC_PER_SEC * (prescale + 1)
                          ^
It may be this + 1, which I don't seem to have accounted for in my quick
run through a calculation?

*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);

*period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;

The code does not add a 1 when it calculates prescale, only when it uses
the result to calculate period_steps, since prescale & period_steps are
the register values, not the "mathematical" ones.

Hopefully I've not gone and made a fool of myself...

> > +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);
> > +}
> 
> There is only one caller for this two-line function. I suggest to unroll it?

Sure.

> > +	ret = devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip);
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +
> > +	/*
> > +	 * Enabled synchronous update for channels with shadow registers
> > +	 * enabled. For channels without shadow registers, this has no effect
> > +	 * at all so is unconditionally enabled.
> > +	 */
> > +	writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
> > +	mchp_core_pwm->update_timestamp = ktime_get();
> 
> This needs to be done before devm_pwmchip_add().

Makes sense, woops. I think I've revised this to the point that my
blinkers have turned on & I'll wait a while before resubmitting in order
to hopefully reset that.

Perhaps I need to watch a lecture on how to write a PWM driver since I
am clearly no good at it, given the 15 revisions. Do you know of any?

Thanks,
Conor.


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

Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

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

Hello Conor,

On Wed, Jan 11, 2023 at 12:15:29AM +0000, Conor Dooley wrote:
> On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> > On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > > From: Conor Dooley <conor.dooley@microchip.com>
> 
> > > +		delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC);
> > > +		if ((delay_us / 1000) > MAX_UDELAY_MS)
> > > +			msleep(delay_us / 1000 + 1);
> > 
> > Is this better than
> > 
> > 	msleep(DIV_ROUND_UP(delay_us, 1000);
> > 
> > ? Also I wonder about your usage of MAX_UDELAY_MS. This is about
> 
> I probably started hacking on the example you gave and didn't notice
> the U. What I have here is ~what you suggested last time.

A series with (up to now) 13 revisions and long delays between the
review rounds (which are mostly attributed to my time schedule) is
difficult to handle on both sides. Some repetition isn't easy to prevent
in such a case. Sorry for that. 

> > udelay() but you're using usleep_range()?
> > 
> > > +		else
> > > +			usleep_range(delay_us, delay_us * 2);
> > 
> > I wonder if there isn't a function that implements something like
> > 
> > 	wait_until(mchp_core_pwm->update_timestamp);
> > 
> > which would be a bit nicer than doing this by hand. Maybe fsleep()?
> 
> That'd be fsleep(delay_us), but does at least clean up some of the
> messing.
> 
> > > +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;
> > 
> > IMHO
> > 
> > 		negedge = duty_steps ? 0 : period_steps_val;
> > 
> > is a bit easier to parse.
> > 
> > > +		posedge = duty_steps;
> > > +	} else {
> > > +		posedge = !duty_steps ? period_steps_val : 0u;
> > > +		negedge = duty_steps;
> > > +	}
> > 
> > The following code is equivalent:
> > 
> > 	u8 first_edge = 0, second_edge = duty_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 (duty_steps == 0)
> > 		first_edge = period_steps_val;
> > 
> > 	if (state->polarity == PWM_POLARITY_INVERSED) {
> > 		negedge = first_edge;
> > 		posedge = second_edge;
> > 	} else {
> > 		posedge = first_edge;
> > 		negedge = second_edge;
> > 	}
> > 
> > I'm not sure if it's easier to understand. What do you think?
> 
> Despite having used them, I dislike ternary statements.

My variant is a bit longer and uses more variables, but has less
repetition. I don't expect a relevant change on the generated code. I
slightly prefer my variant, but I let you choose which one you prefer.

> > > +	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
> > > +	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
> > > +}
> > > +
> > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > > +				      u16 *prescale, u8 *period_steps)
> > > +{
> > > +	u64 tmp;
> > > +
> > > +	/*
> > > +	 * 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.
> > > +	 */
> > > +	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;
> > > +	}
> > > +
> > > +	*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;
> > 
> > This looks wrong, but I didn't think long about that. Did we discuss
> > this already and/or are you sure this is correct?
> 
> We did discuss it previously AFAICT;
> https://lore.kernel.org/linux-pwm/896d73ac-05af-8673-8379-29011800be83@microchip.com/
> 
> [...]
> Unfortunately, I don't think I am seeing what you're seeing.

Well, the calculation lands in the right ballpark for sure, but if my
intuition is right, it's not as exact as it could be. I need some time
with pencil and paper ...

> [...]
> Perhaps I need to watch a lecture on how to write a PWM driver since I
> am clearly no good at it, given the 15 revisions. Do you know of any?

I'm not aware of such a lecture. I'm willing to take the blame for some
of the revisions because I'm very picky and the math involved here isn't
trivial. And I sometimes wonder about myself pointing out an issue in
(say) v5 which was there unnoticed already in v1.

In sum a patch series going through such a high number of revisions is
mostly a good sign. In the end we can be sure that the merged code is
checked deep-rootedly and that both you and me have a certain amount of
endurance. :-)

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 v13 1/2] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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

Hey Uwe,

On Wed, Jan 11, 2023 at 08:02:50AM +0100, Uwe Kleine-König wrote:
> On Wed, Jan 11, 2023 at 12:15:29AM +0000, Conor Dooley wrote:
> > On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> > > On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > > > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > > > +		delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC);
> > > > +		if ((delay_us / 1000) > MAX_UDELAY_MS)
> > > > +			msleep(delay_us / 1000 + 1);
> > > 
> > > Is this better than
> > > 
> > > 	msleep(DIV_ROUND_UP(delay_us, 1000);
> > > 
> > > ? Also I wonder about your usage of MAX_UDELAY_MS. This is about
> > 
> > I probably started hacking on the example you gave and didn't notice
> > the U. What I have here is ~what you suggested last time.
> 
> A series with (up to now) 13 revisions and long delays between the
> review rounds (which are mostly attributed to my time schedule) is
> difficult to handle on both sides. Some repetition isn't easy to prevent
> in such a case. Sorry for that.

It is what it is, you've only got so much time :)

> > > udelay() but you're using usleep_range()?
> > > 
> > > > +		else
> > > > +			usleep_range(delay_us, delay_us * 2);
> > > 
> > > I wonder if there isn't a function that implements something like
> > > 
> > > 	wait_until(mchp_core_pwm->update_timestamp);
> > > 
> > > which would be a bit nicer than doing this by hand. Maybe fsleep()?
> > 
> > That'd be fsleep(delay_us), but does at least clean up some of the
> > messing.
> > 
> > > > +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;
> > > 
> > > IMHO
> > > 
> > > 		negedge = duty_steps ? 0 : period_steps_val;
> > > 
> > > is a bit easier to parse.
> > > 
> > > > +		posedge = duty_steps;
> > > > +	} else {
> > > > +		posedge = !duty_steps ? period_steps_val : 0u;
> > > > +		negedge = duty_steps;
> > > > +	}
> > > 
> > > The following code is equivalent:
> > > 
> > > 	u8 first_edge = 0, second_edge = duty_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 (duty_steps == 0)
> > > 		first_edge = period_steps_val;
> > > 
> > > 	if (state->polarity == PWM_POLARITY_INVERSED) {
> > > 		negedge = first_edge;
> > > 		posedge = second_edge;
> > > 	} else {
> > > 		posedge = first_edge;
> > > 		negedge = second_edge;
> > > 	}
> > > 
> > > I'm not sure if it's easier to understand. What do you think?
> > 
> > Despite having used them, I dislike ternary statements.
> 
> My variant is a bit longer and uses more variables, but has less
> repetition. I don't expect a relevant change on the generated code. I
> slightly prefer my variant, but I let you choose which one you prefer.

Yah, I prefer anything that doesn't have ternarys in it.

> > > > +	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
> > > > +	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
> > > > +}
> > > > +
> > > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > > > +				      u16 *prescale, u8 *period_steps)
> > > > +{
> > > > +	u64 tmp;
> > > > +
> > > > +	/*
> > > > +	 * 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.
> > > > +	 */
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	*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;
> > > 
> > > This looks wrong, but I didn't think long about that. Did we discuss
> > > this already and/or are you sure this is correct?
> > 
> > We did discuss it previously AFAICT;
> > https://lore.kernel.org/linux-pwm/896d73ac-05af-8673-8379-29011800be83@microchip.com/
> > 
> > [...]
> > Unfortunately, I don't think I am seeing what you're seeing.
> 
> Well, the calculation lands in the right ballpark for sure, but if my
> intuition is right, it's not as exact as it could be. I need some time
> with pencil and paper ...
> 
> > [...]
> > Perhaps I need to watch a lecture on how to write a PWM driver since I
> > am clearly no good at it, given the 15 revisions. Do you know of any?
> 
> I'm not aware of such a lecture.

I thought you were doing one at FOSDEM!

> I'm willing to take the blame for some
> of the revisions because I'm very picky and the math involved here isn't
> trivial.

I'd rather the maths was right. Fixing it up front is better than trying
to debug it from a customer complaint - so thanks for that.
And the maths was very naive to begin with, although I only submitted it
in the summer, I think I wrote the driver prior to having upstreamed a
single patch, and I think that showed in the maths.

> And I sometimes wonder about myself pointing out an issue in
> (say) v5 which was there unnoticed already in v1.

I dunno. I feel bad if I do that too - but if the problem you didn't
notice earlier on is a bug, rather than some sort of style comment, I
don't see why you wouldn't call it out.

> In sum a patch series going through such a high number of revisions is
> mostly a good sign.

Or that it was poor to begin with & barely improved over time...

> In the end we can be sure that the merged code is
> checked deep-rootedly and that both you and me have a certain amount of
> endurance. :-)

I wasn't really blaming you for the number of revisions, the number of
silly mistakes I've made really irritates me.

Thanks,
Conor.


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

Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

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

Hello Conor,

On Wed, Jan 11, 2023 at 08:00:51AM +0000, Conor Dooley wrote:
> > > [...]
> > > Perhaps I need to watch a lecture on how to write a PWM driver since I
> > > am clearly no good at it, given the 15 revisions. Do you know of any?
> > 
> > I'm not aware of such a lecture.
> 
> I thought you were doing one at FOSDEM!

Oh that knowledge already spread? Anyhow that doesn't help you today
:-)

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 v13 1/2] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

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

Hello,

On Wed, Jan 11, 2023 at 08:02:50AM +0100, Uwe Kleine-König wrote:
> On Wed, Jan 11, 2023 at 12:15:29AM +0000, Conor Dooley wrote:
> > On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> > > On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > > > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > > > +		delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC);
> > > > +		if ((delay_us / 1000) > MAX_UDELAY_MS)
> > > > +			msleep(delay_us / 1000 + 1);
> > > 
> > > Is this better than
> > > 
> > > 	msleep(DIV_ROUND_UP(delay_us, 1000);
> > > 
> > > ? Also I wonder about your usage of MAX_UDELAY_MS. This is about
> > 
> > I probably started hacking on the example you gave and didn't notice
> > the U. What I have here is ~what you suggested last time.
> 
> A series with (up to now) 13 revisions and long delays between the
> review rounds (which are mostly attributed to my time schedule) is
> difficult to handle on both sides. Some repetition isn't easy to prevent
> in such a case. Sorry for that. 
> 
> > > udelay() but you're using usleep_range()?
> > > 
> > > > +		else
> > > > +			usleep_range(delay_us, delay_us * 2);
> > > 
> > > I wonder if there isn't a function that implements something like
> > > 
> > > 	wait_until(mchp_core_pwm->update_timestamp);
> > > 
> > > which would be a bit nicer than doing this by hand. Maybe fsleep()?
> > 
> > That'd be fsleep(delay_us), but does at least clean up some of the
> > messing.
> > 
> > > > +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;
> > > 
> > > IMHO
> > > 
> > > 		negedge = duty_steps ? 0 : period_steps_val;
> > > 
> > > is a bit easier to parse.
> > > 
> > > > +		posedge = duty_steps;
> > > > +	} else {
> > > > +		posedge = !duty_steps ? period_steps_val : 0u;
> > > > +		negedge = duty_steps;
> > > > +	}
> > > 
> > > The following code is equivalent:
> > > 
> > > 	u8 first_edge = 0, second_edge = duty_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 (duty_steps == 0)
> > > 		first_edge = period_steps_val;
> > > 
> > > 	if (state->polarity == PWM_POLARITY_INVERSED) {
> > > 		negedge = first_edge;
> > > 		posedge = second_edge;
> > > 	} else {
> > > 		posedge = first_edge;
> > > 		negedge = second_edge;
> > > 	}
> > > 
> > > I'm not sure if it's easier to understand. What do you think?
> > 
> > Despite having used them, I dislike ternary statements.
> 
> My variant is a bit longer and uses more variables, but has less
> repetition. I don't expect a relevant change on the generated code. I
> slightly prefer my variant, but I let you choose which one you prefer.
> 
> > > > +	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
> > > > +	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
> > > > +}
> > > > +
> > > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > > > +				      u16 *prescale, u8 *period_steps)
> > > > +{
> > > > +	u64 tmp;
> > > > +
> > > > +	/*
> > > > +	 * 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.
> > > > +	 */
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	*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;
> > > 
> > > This looks wrong, but I didn't think long about that. Did we discuss
> > > this already and/or are you sure this is correct?
> > 
> > We did discuss it previously AFAICT;
> > https://lore.kernel.org/linux-pwm/896d73ac-05af-8673-8379-29011800be83@microchip.com/
> > 
> > [...]
> > Unfortunately, I don't think I am seeing what you're seeing.
> 
> Well, the calculation lands in the right ballpark for sure, but if my
> intuition is right, it's not as exact as it could be. I need some time
> with pencil and paper ...

Just a small heads up:

I thought a bit about that (without pencil and paper on my way to work
today), and the optimal solution is non-trivial. In fact you have to
pick parameters A and B such that for a given C

	(A + 1) * (B + 1) is maximal with

	0 <= A < 0x100
	0 <= B < 0xff
	(A + 1) * (B + 1) <= C

A consistent non-optimal choice that is easier to calculate than a
complete search would be nice. But I think we're not there yet.

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 v13 1/2] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

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

Hello,

On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> Hello Conor,
> 
> On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > +				      u16 *prescale, u8 *period_steps)
> > +{
> > +	u64 tmp;
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> > +	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;
> > +	}
> > +
> > +	*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;
> 
> This looks wrong, but I didn't think long about that. Did we discuss
> this already and/or are you sure this is correct?
> 
> (We have:
> 	          (prescale + 1) * (period_steps + 1)
> 	period = ------------------------------------
> 	                       clk_rate
> 

We want prescale small such that period_steps can be big to give
fine-grained control over the available duty_cycles. period_steps is a
8-bit value < 0xff, so we get:

                    period * clk_rate   
	prescale = ------------------- - 1
                   NSEC_PER_SEC * 0xff

which in code then reads:

	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX)
	if (*prescale)
		*prescale -= 1;


> You calculate
> 	            period * clk_rate
> 	prescale = -------------------
> 	           NSEC_PER_SEC * 0xff
> 
> 	                     period * clk_rate
> 	period_steps = ----------------------------- - 1
> 	               NSEC_PER_SEC * (prescale + 1)

The formula for period_steps is right.

Please double-check!

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 v13 1/2] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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

On Thu, Jan 12, 2023 at 01:01:41PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> > Hello Conor,
> > 
> > On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > > +				      u16 *prescale, u8 *period_steps)
> > > +{
> > > +	u64 tmp;
> > > +
> > > +	/*
> > > +	 * 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.
> > > +	 */
> > > +	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;
> > > +	}
> > > +
> > > +	*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;
> > 
> > This looks wrong, but I didn't think long about that. Did we discuss
> > this already and/or are you sure this is correct?
> > 
> > (We have:
> > 	          (prescale + 1) * (period_steps + 1)
> > 	period = ------------------------------------
> > 	                       clk_rate
> > 
> 
> We want prescale small such that period_steps can be big to give
> fine-grained control over the available duty_cycles. period_steps is a
> 8-bit value < 0xff, so we get:
> 
>                     period * clk_rate   
> 	prescale = ------------------- - 1
>                    NSEC_PER_SEC * 0xff
> 
> which in code then reads:
> 
> 	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX)
> 	if (*prescale)
> 		*prescale -= 1;
> 
> 
> > You calculate
> > 	            period * clk_rate
> > 	prescale = -------------------
> > 	           NSEC_PER_SEC * 0xff
> > 
> > 	                     period * clk_rate
> > 	period_steps = ----------------------------- - 1
> > 	               NSEC_PER_SEC * (prescale + 1)
> 
> The formula for period_steps is right.

I stood in front of the whiteboard for a bit to puzzle it all out and
have come to the realisation that you are right. I was implicitly
converting in my head from "mathematical" values to register values &
therefore not subtracting. I must've hyperfocused on the underflow when
I adjusted your suggestion back in v5 or w/e it was.

I must also have got rather unlucky that the configurations I picked to
check whether the calculations worked out, happened to. I suppose as
well, the way in which it was mis-calculating also avoids the PWM_DEBUG
complaints too.

Perhaps I'll insert your nice formulae in the next version in comments,
so they're there for next time.

Thanks again & sorry for consuming so much of your time on this,
Conor.


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

Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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

On Thu, Jan 12, 2023 at 03:10:09PM +0000, Conor Dooley wrote:
> On Thu, Jan 12, 2023 at 01:01:41PM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> > > Hello Conor,
> > > 
> > > On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > > > +				      u16 *prescale, u8 *period_steps)
> > > > +{
> > > > +	u64 tmp;
> > > > +
> > > > +	/*
> > > > +	 * 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.
> > > > +	 */
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	*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;
> > > 
> > > This looks wrong, but I didn't think long about that. Did we discuss
> > > this already and/or are you sure this is correct?
> > > 
> > > (We have:
> > > 	          (prescale + 1) * (period_steps + 1)
> > > 	period = ------------------------------------
> > > 	                       clk_rate
> > > 
> > 
> > We want prescale small such that period_steps can be big to give
> > fine-grained control over the available duty_cycles. period_steps is a
> > 8-bit value < 0xff, so we get:
> > 
> >                     period * clk_rate   
> > 	prescale = ------------------- - 1
> >                    NSEC_PER_SEC * 0xff
> > 
> > which in code then reads:
> > 
> > 	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX)
> > 	if (*prescale)
> > 		*prescale -= 1;
> > 
> > 
> > > You calculate
> > > 	            period * clk_rate
> > > 	prescale = -------------------
> > > 	           NSEC_PER_SEC * 0xff
> > > 
> > > 	                     period * clk_rate
> > > 	period_steps = ----------------------------- - 1
> > > 	               NSEC_PER_SEC * (prescale + 1)
> > 
> > The formula for period_steps is right.
> 
> I stood in front of the whiteboard for a bit to puzzle it all out and
> have come to the realisation that you are right. I was implicitly
> converting in my head from "mathematical" values to register values &
> therefore not subtracting. I must've hyperfocused on the underflow when
> I adjusted your suggestion back in v5 or w/e it was.
> 
> I must also have got rather unlucky that the configurations I picked to
> check whether the calculations worked out, happened to. I suppose as
> well, the way in which it was mis-calculating also avoids the PWM_DEBUG
> complaints too.
> 
> Perhaps I'll insert your nice formulae in the next version in comments,
> so they're there for next time.

*sigh*, me again...

Picked this up this afternoon to try and respin a v14 and ran into a bit
of an issue with this suggestion:

> > 	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX)
> > 	if (*prescale)
> > 		*prescale -= 1;

In theory this works out, but with integer division it doesn't AFAICT.

Take for example:
period_ns = 10,000
clk_rate = 62.5 MHz

The prescale calculation is:
(10000 * 62.5E6) / (1E9 * 255) - 1 = 1.451 -> 1

For period_steps then:
(10000 * 62.5E6) / (1E9 * (1 + 1)) - 1 = 311.5 -> 311

prescale is a u16, since it's valid for that division to return 256.
The register is only 8 bits wide though, so 311 overflows it and 55 ends
up in the register.
The period then is:
(55 + 1) * (1 + 1) / 62.5E6, or a period of 1792 ns.

I think it needs to be a div_u64_round_up(), which I know is not a real
function, otherwise we compute invalid values for period steps.

Quoting you previously:
> I thought a bit about that (without pencil and paper on my way to work
> today), and the optimal solution is non-trivial. In fact you have to
> pick parameters A and B such that for a given C
> 
> 	(A + 1) * (B + 1) is maximal with
> 
> 	0 <= A < 0x100
> 	0 <= B < 0xff
> 	(A + 1) * (B + 1) <= C

As we need (period_steps + 1) * (prescale + 1) maximal, *but* also want
to maximise period_steps so that duty resolution etc is maximal too.
We pick prescale, using the formula below, by setting period_steps to
the maximum possible value:
> 	            period * clk_rate
> 	prescale = -------------------
> 	           NSEC_PER_SEC * 0xff

Given we set period_steps to the max here, we only actually pick the
lower bound on prescale, not the actual value of it.
We must round it to the nearest possible value before using it to go
back and calculate period_steps.

With the previous code, missing the -= 1, the division result was purely
truncated:
(10000 * 62.5E6) / (1E9 * 255) = 2.451 -> 2
That 2 was written into the register, and then the hardware rounds that
up to 3, as does the period_steps calculation.

period_steps here is:
(10000 * 62.5E6) / (1E9 * (2 + 1)) - 1 = 207.833 -> 207

Finally, the period:
(207 + 1) * (2 + 1) / 62.5E6, or a period of 9984 ns.

This is the same operation, unless I am yet again overlooking something,
as doing a div_u64_round_up() type thing, followed by a subtraction of 1

We don't need to worry about writing something too big into the register
either, as we are protected against values of tmp that, when divided by
0xff, would produce values larger than 255:
> /*
>  * The hardware adds one to the register value, so decrement by one to
>  * account for the offset
>  */
> if (tmp >= MCHPCOREPWM_PERIOD_MAX) { (PERIOD_MAX is 0xff00)
> 	*prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
> 	*period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
> 
> 	return;
> }

What am I missing this time? haha

Thanks,
Conor.


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

Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver

[Conor Dooley]

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

Hey Uwe!

Was nice to meet you at FOSDEM, if I'd seen that presentation before
working on this driver I think the version count would have been
substantially lower!

On Fri, Jan 20, 2023 at 07:40:32PM +0000, Conor Dooley wrote:
> On Thu, Jan 12, 2023 at 03:10:09PM +0000, Conor Dooley wrote:
> > On Thu, Jan 12, 2023 at 01:01:41PM +0100, Uwe Kleine-König wrote:
> > > On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:

> *sigh*, me again...
> 
> Picked this up this afternoon to try and respin a v14 and ran into a bit
> of an issue with this suggestion:
> 
> > > 	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX)
> > > 	if (*prescale)
> > > 		*prescale -= 1;
> 
> In theory this works out, but with integer division it doesn't AFAICT.
> 
> Take for example:
> period_ns = 10,000
> clk_rate = 62.5 MHz
> 
> The prescale calculation is:
> (10000 * 62.5E6) / (1E9 * 255) - 1 = 1.451 -> 1
> 
> For period_steps then:
> (10000 * 62.5E6) / (1E9 * (1 + 1)) - 1 = 311.5 -> 311
> 
> prescale is a u16, since it's valid for that division to return 256.
> The register is only 8 bits wide though, so 311 overflows it and 55 ends
> up in the register.
> The period then is:
> (55 + 1) * (1 + 1) / 62.5E6, or a period of 1792 ns.
> 
> I think it needs to be a div_u64_round_up(), which I know is not a real
> function, otherwise we compute invalid values for period steps.
> 
> Quoting you previously:
> > I thought a bit about that (without pencil and paper on my way to work
> > today), and the optimal solution is non-trivial. In fact you have to
> > pick parameters A and B such that for a given C
> > 
> > 	(A + 1) * (B + 1) is maximal with
> > 
> > 	0 <= A < 0x100
> > 	0 <= B < 0xff
> > 	(A + 1) * (B + 1) <= C
> 
> As we need (period_steps + 1) * (prescale + 1) maximal, *but* also want
> to maximise period_steps so that duty resolution etc is maximal too.
> We pick prescale, using the formula below, by setting period_steps to
> the maximum possible value:
> > 	            period * clk_rate
> > 	prescale = -------------------
> > 	           NSEC_PER_SEC * 0xff
> 
> Given we set period_steps to the max here, we only actually pick the
> lower bound on prescale, not the actual value of it.
> We must round it to the nearest possible value before using it to go
> back and calculate period_steps.
> 
> With the previous code, missing the -= 1, the division result was purely
> truncated:
> (10000 * 62.5E6) / (1E9 * 255) = 2.451 -> 2
> That 2 was written into the register, and then the hardware rounds that
> up to 3, as does the period_steps calculation.
> 
> period_steps here is:
> (10000 * 62.5E6) / (1E9 * (2 + 1)) - 1 = 207.833 -> 207
> 
> Finally, the period:
> (207 + 1) * (2 + 1) / 62.5E6, or a period of 9984 ns.
> 
> This is the same operation, unless I am yet again overlooking something,
> as doing a div_u64_round_up() type thing, followed by a subtraction of 1
> 
> We don't need to worry about writing something too big into the register
> either, as we are protected against values of tmp that, when divided by
> 0xff, would produce values larger than 255:
> > /*
> >  * The hardware adds one to the register value, so decrement by one to
> >  * account for the offset
> >  */
> > if (tmp >= MCHPCOREPWM_PERIOD_MAX) { (PERIOD_MAX is 0xff00)
> > 	*prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
> > 	*period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
> > 
> > 	return;
> > }
> 
> What am I missing this time? haha

So my "plan" I guess is to rebase this on v6.3-rc1 & re-submit with the
various minor bits fixed.
Trying the proposed "fixes" to the period calculation that we had
discussed pointed out the above issue, and it appears, to me at least,
that we made a mess of translating from regular calculations to integer
ones.

Cheers,
Conor.


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