[PATCH v3 0/2] Add support for Microchip's pwm fpga core

[PATCH v3 0/2] Add support for Microchip's pwm fpga core

[Conor Dooley]

Hey Uwe,
Got a ~v2~ v3 for you...
I added some comments explaining the calculations and a documentation link
so hopefully things are a bit easier to follow.

Code wise, I went through and sorted out a bunch of issues that cycling
through the different periods/duties threw up. Along the way I found
some other problems - especially with the longer periods which I have
fixed. I also added a write to the sync register in the apply function,
which will resolve to a NOP for channels without "shadow registers".

Other than that, I managed to ditch the mchp_core_pwm_registers struct
entirely but had to add a short delay before reading back the registers
in order to compute the duty.

Thanks,
Conor.

Changes from v2:
- fix percieved idempotency and rounding issues when shadow registers were
  enabled
- use do_div() for divide of u64 tmp in apply_period()

Changes from v1:
- account for edge "quirk" while inverted
- block changing enabled channels' period
- document the hardware/driver limitations
- rearrange get_state() more logically
- fix cast sizes in get_state()
- fix remove() and probe error paths
- delete mchp_core_pwm_registers
- simplify .apply() logic
- don't warn in calculate_base()
- fix period calculation
- fix duty cycle calculation
- add COREPWM prefix to defines
- add a documentation link

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


base-commit: 61114e734ccb804bc12561ab4020745e02c468c2
-- 
2.36.1

[PATCH v3 0/2] Add support for Microchip's pwm fpga core

[Conor Dooley]

Hey Uwe,
Got a ~v2~ v3 for you...
I added some comments explaining the calculations and a documentation link
so hopefully things are a bit easier to follow.

Code wise, I went through and sorted out a bunch of issues that cycling
through the different periods/duties threw up. Along the way I found
some other problems - especially with the longer periods which I have
fixed. I also added a write to the sync register in the apply function,
which will resolve to a NOP for channels without "shadow registers".

Other than that, I managed to ditch the mchp_core_pwm_registers struct
entirely but had to add a short delay before reading back the registers
in order to compute the duty.

Thanks,
Conor.

Changes from v2:
- fix percieved idempotency and rounding issues when shadow registers were
  enabled
- use do_div() for divide of u64 tmp in apply_period()

Changes from v1:
- account for edge "quirk" while inverted
- block changing enabled channels' period
- document the hardware/driver limitations
- rearrange get_state() more logically
- fix cast sizes in get_state()
- fix remove() and probe error paths
- delete mchp_core_pwm_registers
- simplify .apply() logic
- don't warn in calculate_base()
- fix period calculation
- fix duty cycle calculation
- add COREPWM prefix to defines
- add a documentation link

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


base-commit: 61114e734ccb804bc12561ab4020745e02c468c2
-- 
2.36.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v3 1/2] 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 | 325 +++++++++++++++++++++++++++++++
 3 files changed, 336 insertions(+)
 create mode 100644 drivers/pwm/pwm-microchip-core.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 21e3b05a5153..a651848e444b 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -383,6 +383,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 708840b7fba8..d29754c20f91 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -33,6 +33,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..abbfc1cd23c4
--- /dev/null
+++ b/drivers/pwm/pwm-microchip-core.c
@@ -0,0 +1,325 @@
+// 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/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/math.h>
+
+#define PREG_TO_VAL(PREG) ((PREG) + 1)
+
+#define COREPWM_PRESCALE_REG	0x00u
+#define COREPWM_PERIOD_REG	0x04u
+#define COREPWM_EN_LOW_REG	0x08u
+#define COREPWM_EN_HIGH_REG	0x0Cu
+#define COREPWM_SYNC_UPD_REG	0xE4u
+#define COREPWM_POSEDGE_OFFSET	0x10u
+#define COREPWM_NEGEDGE_OFFSET	0x14u
+#define COREPWM_CHANNEL_OFFSET	0x08u
+
+struct mchp_core_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	void __iomem *base;
+	u32 sync_update_mask;
+};
+
+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)
+{
+	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 = COREPWM_EN_LOW_REG + (pwm->hwpwm >> 3) * sizeof(u32);
+	shift = pwm->hwpwm > 7 ? pwm->hwpwm - 8 : pwm->hwpwm;
+
+	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);
+
+	/*
+	 * Write to the sync update registers so that channels with shadow
+	 * registers will also get their enable update. This operation is a NOP
+	 * for channels without shadow registers.
+	 */
+	writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
+}
+
+static void mchp_core_pwm_apply_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);
+	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
+	u64 duty_steps, period, tmp;
+	u8 posedge, negedge;
+	u8 prescale_val = PREG_TO_VAL(prescale);
+	u8 period_steps_val = PREG_TO_VAL(period_steps);
+
+	period = period_steps_val * prescale_val * NSEC_PER_SEC;
+	period = div64_u64(period, clk_get_rate(mchp_core_pwm->clk));
+
+	/*
+	 * 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.
+	 *
+	 * 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.
+	 */
+	if (state->duty_cycle > period) {
+		duty_steps = period_steps;
+	} else {
+		duty_steps = state->duty_cycle * clk_get_rate(mchp_core_pwm->clk);
+		tmp = prescale_val * NSEC_PER_SEC;
+		duty_steps = div64_u64(duty_steps, tmp);
+	}
+
+	/*
+	 * Turn the output on unless posedge == negedge, in which case the
+	 * duty is intended to be 0, but limitations of the IP block don't
+	 * allow a zero length duty cycle - so just set the max high/low time
+	 * respectively.
+	 */
+	if (state->polarity == PWM_POLARITY_INVERSED) {
+		negedge = !duty_steps ? period_steps : 0u;
+		posedge = duty_steps;
+	} else {
+		posedge = !duty_steps ? period_steps : 0u;
+		negedge = duty_steps;
+	}
+
+	writel_relaxed(posedge, channel_base + COREPWM_POSEDGE_OFFSET);
+	writel_relaxed(negedge, channel_base + COREPWM_NEGEDGE_OFFSET);
+}
+
+static void mchp_core_pwm_apply_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 = state->period;
+
+	/*
+	 * Calculate the period cycles and prescale values.
+	 * The registers are each 8 bits wide & multiplied to compute the period
+	 * so the maximum period that can be generated is 0xFFFF times the period
+	 * of the input clock.
+	 */
+	tmp *= clk_get_rate(mchp_core_pwm->clk);
+	do_div(tmp, NSEC_PER_SEC);
+
+	if (tmp > 0xFFFFu) {
+		*prescale = 0xFFu;
+		*period_steps = 0xFFu;
+	} else {
+		*prescale = tmp >> 8;
+		do_div(tmp, PREG_TO_VAL(*prescale));
+		*period_steps = tmp - 1;
+	}
+
+	writel_relaxed(*prescale, mchp_core_pwm->base + COREPWM_PRESCALE_REG);
+	writel_relaxed(*period_steps, mchp_core_pwm->base + COREPWM_PERIOD_REG);
+}
+
+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;
+	u16 channel_enabled;
+	u8 prescale, period_steps;
+
+	if (!state->enabled) {
+		mchp_core_pwm_enable(chip, pwm, false);
+		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.
+	 */
+	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
+		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
+	period_locked = channel_enabled & ~(1 << pwm->hwpwm);
+
+	if ((!current_state.enabled || current_state.period != state->period) && !period_locked) {
+		mchp_core_pwm_apply_period(chip, state, &prescale, &period_steps);
+	} else {
+		prescale = readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG);
+		period_steps = readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG);
+	}
+
+	mchp_core_pwm_apply_duty(chip, pwm, state, prescale, period_steps);
+
+	/*
+	 * 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.
+	 */
+	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
+		writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
+		usleep_range(state->period, state->period * 2);
+	}
+
+	mchp_core_pwm_enable(chip, pwm, true);
+
+	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);
+	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
+	u8 prescale, period_steps, duty_steps;
+	u8 posedge, negedge;
+	u16 channel_enabled;
+
+	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
+		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
+
+	if (channel_enabled & 1 << pwm->hwpwm)
+		state->enabled = true;
+	else
+		state->enabled = false;
+
+	prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG));
+
+	posedge = readb_relaxed(channel_base + COREPWM_POSEDGE_OFFSET);
+	negedge = readb_relaxed(channel_base + COREPWM_NEGEDGE_OFFSET);
+
+	duty_steps = abs((s16)posedge - (s16)negedge);
+	state->duty_cycle = duty_steps * prescale * NSEC_PER_SEC;
+	do_div(state->duty_cycle, clk_get_rate(mchp_core_pwm->clk));
+
+	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+
+	period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG));
+	state->period = period_steps * prescale * NSEC_PER_SEC;
+	do_div(state->period, clk_get_rate(mchp_core_pwm->clk));
+}
+
+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(&pdev->dev, NULL);
+	if (IS_ERR(mchp_pwm->clk))
+		return PTR_ERR(mchp_pwm->clk);
+
+	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
+				 &mchp_pwm->sync_update_mask))
+		mchp_pwm->sync_update_mask = 0u;
+
+	ret = clk_prepare_enable(mchp_pwm->clk);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to prepare PWM clock\n");
+
+	mchp_pwm->chip.dev = &pdev->dev;
+	mchp_pwm->chip.ops = &mchp_core_pwm_ops;
+	mchp_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
+	mchp_pwm->chip.of_pwm_n_cells = 3;
+	mchp_pwm->chip.npwm = 16;
+
+	ret = pwmchip_add(&mchp_pwm->chip);
+	if (ret < 0) {
+		clk_disable_unprepare(mchp_pwm->clk);
+		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+	}
+
+	platform_set_drvdata(pdev, mchp_pwm);
+	dev_info(&pdev->dev, "Successfully registered Microchip corePWM\n");
+
+	return 0;
+}
+
+static int mchp_core_pwm_remove(struct platform_device *pdev)
+{
+	struct mchp_core_pwm_chip *mchp_pwm = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&mchp_pwm->chip);
+	clk_disable_unprepare(mchp_pwm->clk);
+
+	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,
+	.remove = mchp_core_pwm_remove,
+};
+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

[PATCH v3 1/2] 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 | 325 +++++++++++++++++++++++++++++++
 3 files changed, 336 insertions(+)
 create mode 100644 drivers/pwm/pwm-microchip-core.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 21e3b05a5153..a651848e444b 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -383,6 +383,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 708840b7fba8..d29754c20f91 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -33,6 +33,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..abbfc1cd23c4
--- /dev/null
+++ b/drivers/pwm/pwm-microchip-core.c
@@ -0,0 +1,325 @@
+// 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/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/math.h>
+
+#define PREG_TO_VAL(PREG) ((PREG) + 1)
+
+#define COREPWM_PRESCALE_REG	0x00u
+#define COREPWM_PERIOD_REG	0x04u
+#define COREPWM_EN_LOW_REG	0x08u
+#define COREPWM_EN_HIGH_REG	0x0Cu
+#define COREPWM_SYNC_UPD_REG	0xE4u
+#define COREPWM_POSEDGE_OFFSET	0x10u
+#define COREPWM_NEGEDGE_OFFSET	0x14u
+#define COREPWM_CHANNEL_OFFSET	0x08u
+
+struct mchp_core_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	void __iomem *base;
+	u32 sync_update_mask;
+};
+
+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)
+{
+	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 = COREPWM_EN_LOW_REG + (pwm->hwpwm >> 3) * sizeof(u32);
+	shift = pwm->hwpwm > 7 ? pwm->hwpwm - 8 : pwm->hwpwm;
+
+	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);
+
+	/*
+	 * Write to the sync update registers so that channels with shadow
+	 * registers will also get their enable update. This operation is a NOP
+	 * for channels without shadow registers.
+	 */
+	writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
+}
+
+static void mchp_core_pwm_apply_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);
+	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
+	u64 duty_steps, period, tmp;
+	u8 posedge, negedge;
+	u8 prescale_val = PREG_TO_VAL(prescale);
+	u8 period_steps_val = PREG_TO_VAL(period_steps);
+
+	period = period_steps_val * prescale_val * NSEC_PER_SEC;
+	period = div64_u64(period, clk_get_rate(mchp_core_pwm->clk));
+
+	/*
+	 * 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.
+	 *
+	 * 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.
+	 */
+	if (state->duty_cycle > period) {
+		duty_steps = period_steps;
+	} else {
+		duty_steps = state->duty_cycle * clk_get_rate(mchp_core_pwm->clk);
+		tmp = prescale_val * NSEC_PER_SEC;
+		duty_steps = div64_u64(duty_steps, tmp);
+	}
+
+	/*
+	 * Turn the output on unless posedge == negedge, in which case the
+	 * duty is intended to be 0, but limitations of the IP block don't
+	 * allow a zero length duty cycle - so just set the max high/low time
+	 * respectively.
+	 */
+	if (state->polarity == PWM_POLARITY_INVERSED) {
+		negedge = !duty_steps ? period_steps : 0u;
+		posedge = duty_steps;
+	} else {
+		posedge = !duty_steps ? period_steps : 0u;
+		negedge = duty_steps;
+	}
+
+	writel_relaxed(posedge, channel_base + COREPWM_POSEDGE_OFFSET);
+	writel_relaxed(negedge, channel_base + COREPWM_NEGEDGE_OFFSET);
+}
+
+static void mchp_core_pwm_apply_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 = state->period;
+
+	/*
+	 * Calculate the period cycles and prescale values.
+	 * The registers are each 8 bits wide & multiplied to compute the period
+	 * so the maximum period that can be generated is 0xFFFF times the period
+	 * of the input clock.
+	 */
+	tmp *= clk_get_rate(mchp_core_pwm->clk);
+	do_div(tmp, NSEC_PER_SEC);
+
+	if (tmp > 0xFFFFu) {
+		*prescale = 0xFFu;
+		*period_steps = 0xFFu;
+	} else {
+		*prescale = tmp >> 8;
+		do_div(tmp, PREG_TO_VAL(*prescale));
+		*period_steps = tmp - 1;
+	}
+
+	writel_relaxed(*prescale, mchp_core_pwm->base + COREPWM_PRESCALE_REG);
+	writel_relaxed(*period_steps, mchp_core_pwm->base + COREPWM_PERIOD_REG);
+}
+
+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;
+	u16 channel_enabled;
+	u8 prescale, period_steps;
+
+	if (!state->enabled) {
+		mchp_core_pwm_enable(chip, pwm, false);
+		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.
+	 */
+	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
+		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
+	period_locked = channel_enabled & ~(1 << pwm->hwpwm);
+
+	if ((!current_state.enabled || current_state.period != state->period) && !period_locked) {
+		mchp_core_pwm_apply_period(chip, state, &prescale, &period_steps);
+	} else {
+		prescale = readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG);
+		period_steps = readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG);
+	}
+
+	mchp_core_pwm_apply_duty(chip, pwm, state, prescale, period_steps);
+
+	/*
+	 * 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.
+	 */
+	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
+		writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
+		usleep_range(state->period, state->period * 2);
+	}
+
+	mchp_core_pwm_enable(chip, pwm, true);
+
+	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);
+	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
+	u8 prescale, period_steps, duty_steps;
+	u8 posedge, negedge;
+	u16 channel_enabled;
+
+	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
+		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
+
+	if (channel_enabled & 1 << pwm->hwpwm)
+		state->enabled = true;
+	else
+		state->enabled = false;
+
+	prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG));
+
+	posedge = readb_relaxed(channel_base + COREPWM_POSEDGE_OFFSET);
+	negedge = readb_relaxed(channel_base + COREPWM_NEGEDGE_OFFSET);
+
+	duty_steps = abs((s16)posedge - (s16)negedge);
+	state->duty_cycle = duty_steps * prescale * NSEC_PER_SEC;
+	do_div(state->duty_cycle, clk_get_rate(mchp_core_pwm->clk));
+
+	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+
+	period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG));
+	state->period = period_steps * prescale * NSEC_PER_SEC;
+	do_div(state->period, clk_get_rate(mchp_core_pwm->clk));
+}
+
+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(&pdev->dev, NULL);
+	if (IS_ERR(mchp_pwm->clk))
+		return PTR_ERR(mchp_pwm->clk);
+
+	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
+				 &mchp_pwm->sync_update_mask))
+		mchp_pwm->sync_update_mask = 0u;
+
+	ret = clk_prepare_enable(mchp_pwm->clk);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to prepare PWM clock\n");
+
+	mchp_pwm->chip.dev = &pdev->dev;
+	mchp_pwm->chip.ops = &mchp_core_pwm_ops;
+	mchp_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
+	mchp_pwm->chip.of_pwm_n_cells = 3;
+	mchp_pwm->chip.npwm = 16;
+
+	ret = pwmchip_add(&mchp_pwm->chip);
+	if (ret < 0) {
+		clk_disable_unprepare(mchp_pwm->clk);
+		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+	}
+
+	platform_set_drvdata(pdev, mchp_pwm);
+	dev_info(&pdev->dev, "Successfully registered Microchip corePWM\n");
+
+	return 0;
+}
+
+static int mchp_core_pwm_remove(struct platform_device *pdev)
+{
+	struct mchp_core_pwm_chip *mchp_pwm = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&mchp_pwm->chip);
+	clk_disable_unprepare(mchp_pwm->clk);
+
+	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,
+	.remove = mchp_core_pwm_remove,
+};
+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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v3 2/2] 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 f1b4b77daa5f..d0b39fa4f309 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17091,6 +17091,7 @@ L:	linux-riscv@lists.infradead.org
 S:	Supported
 F:	arch/riscv/boot/dts/microchip/
 F:	drivers/mailbox/mailbox-mpfs.c
+F:	drivers/pwm/pwm-microchip-core.c
 F:	drivers/soc/microchip/
 F:	include/soc/microchip/mpfs.h
 
-- 
2.36.1

[PATCH v3 2/2] 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 f1b4b77daa5f..d0b39fa4f309 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17091,6 +17091,7 @@ L:	linux-riscv@lists.infradead.org
 S:	Supported
 F:	arch/riscv/boot/dts/microchip/
 F:	drivers/mailbox/mailbox-mpfs.c
+F:	drivers/pwm/pwm-microchip-core.c
 F:	drivers/soc/microchip/
 F:	include/soc/microchip/mpfs.h
 
-- 
2.36.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 0/2] Add support for Microchip's pwm fpga core

[Conor.Dooley]

On 17/06/2022 12:44, Conor Dooley wrote:
> Hey Uwe,
> Got a ~v2~ v3 for you...
> I added some comments explaining the calculations and a documentation link
> so hopefully things are a bit easier to follow.
> 
> Code wise, I went through and sorted out a bunch of issues that cycling
> through the different periods/duties threw up. Along the way I found
> some other problems - especially with the longer periods which I have
> fixed. I also added a write to the sync register in the apply function,
> which will resolve to a NOP for channels without "shadow registers".
> 
> Other than that, I managed to ditch the mchp_core_pwm_registers struct
> entirely but had to add a short delay before reading back the registers
> in order to compute the duty.
> 
> Thanks,
> Conor.

*sigh* yet again I forgot to mention the potential maintainers conflict
with spi-next..

> 
> Changes from v2:
> - fix percieved idempotency and rounding issues when shadow registers were
>    enabled
> - use do_div() for divide of u64 tmp in apply_period()
> 
> Changes from v1:
> - account for edge "quirk" while inverted
> - block changing enabled channels' period
> - document the hardware/driver limitations
> - rearrange get_state() more logically
> - fix cast sizes in get_state()
> - fix remove() and probe error paths
> - delete mchp_core_pwm_registers
> - simplify .apply() logic
> - don't warn in calculate_base()
> - fix period calculation
> - fix duty cycle calculation
> - add COREPWM prefix to defines
> - add a documentation link
> 
> 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 | 325 +++++++++++++++++++++++++++++++
>   4 files changed, 337 insertions(+)
>   create mode 100644 drivers/pwm/pwm-microchip-core.c
> 
> 
> base-commit: 61114e734ccb804bc12561ab4020745e02c468c2

Re: [PATCH v3 0/2] Add support for Microchip's pwm fpga core

[Conor.Dooley]

On 17/06/2022 12:44, Conor Dooley wrote:
> Hey Uwe,
> Got a ~v2~ v3 for you...
> I added some comments explaining the calculations and a documentation link
> so hopefully things are a bit easier to follow.
> 
> Code wise, I went through and sorted out a bunch of issues that cycling
> through the different periods/duties threw up. Along the way I found
> some other problems - especially with the longer periods which I have
> fixed. I also added a write to the sync register in the apply function,
> which will resolve to a NOP for channels without "shadow registers".
> 
> Other than that, I managed to ditch the mchp_core_pwm_registers struct
> entirely but had to add a short delay before reading back the registers
> in order to compute the duty.
> 
> Thanks,
> Conor.

*sigh* yet again I forgot to mention the potential maintainers conflict
with spi-next..

> 
> Changes from v2:
> - fix percieved idempotency and rounding issues when shadow registers were
>    enabled
> - use do_div() for divide of u64 tmp in apply_period()
> 
> Changes from v1:
> - account for edge "quirk" while inverted
> - block changing enabled channels' period
> - document the hardware/driver limitations
> - rearrange get_state() more logically
> - fix cast sizes in get_state()
> - fix remove() and probe error paths
> - delete mchp_core_pwm_registers
> - simplify .apply() logic
> - don't warn in calculate_base()
> - fix period calculation
> - fix duty cycle calculation
> - add COREPWM prefix to defines
> - add a documentation link
> 
> 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 | 325 +++++++++++++++++++++++++++++++
>   4 files changed, 337 insertions(+)
>   create mode 100644 drivers/pwm/pwm-microchip-core.c
> 
> 
> base-commit: 61114e734ccb804bc12561ab4020745e02c468c2

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 0/2] Add support for Microchip's pwm fpga core

[Uwe Kleine-König]

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

Hello,

On Fri, Jun 17, 2022 at 11:50:13AM +0000, Conor.Dooley@microchip.com wrote:
> On 17/06/2022 12:44, Conor Dooley wrote:
> > Hey Uwe,
> > Got a ~v2~ v3 for you...
> > I added some comments explaining the calculations and a documentation link
> > so hopefully things are a bit easier to follow.
> > 
> > Code wise, I went through and sorted out a bunch of issues that cycling
> > through the different periods/duties threw up. Along the way I found
> > some other problems - especially with the longer periods which I have
> > fixed. I also added a write to the sync register in the apply function,
> > which will resolve to a NOP for channels without "shadow registers".
> > 
> > Other than that, I managed to ditch the mchp_core_pwm_registers struct
> > entirely but had to add a short delay before reading back the registers
> > in order to compute the duty.
> > 
> > Thanks,
> > Conor.
> 
> *sigh* yet again I forgot to mention the potential maintainers conflict
> with spi-next..

I'm not a maintainer of a very active subsystem where MAINTAINER
conflicts are normal, but my expectation up to now was that conflicts in
that file are quite usual and trivial to resolve such that mentioning
these isn't very important.

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 v3 0/2] Add support for Microchip's pwm fpga core

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1436 bytes --]

Hello,

On Fri, Jun 17, 2022 at 11:50:13AM +0000, Conor.Dooley@microchip.com wrote:
> On 17/06/2022 12:44, Conor Dooley wrote:
> > Hey Uwe,
> > Got a ~v2~ v3 for you...
> > I added some comments explaining the calculations and a documentation link
> > so hopefully things are a bit easier to follow.
> > 
> > Code wise, I went through and sorted out a bunch of issues that cycling
> > through the different periods/duties threw up. Along the way I found
> > some other problems - especially with the longer periods which I have
> > fixed. I also added a write to the sync register in the apply function,
> > which will resolve to a NOP for channels without "shadow registers".
> > 
> > Other than that, I managed to ditch the mchp_core_pwm_registers struct
> > entirely but had to add a short delay before reading back the registers
> > in order to compute the duty.
> > 
> > Thanks,
> > Conor.
> 
> *sigh* yet again I forgot to mention the potential maintainers conflict
> with spi-next..

I'm not a maintainer of a very active subsystem where MAINTAINER
conflicts are normal, but my expectation up to now was that conflicts in
that file are quite usual and trivial to resolve such that mentioning
these isn't very important.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 0/2] Add support for Microchip's pwm fpga core

[Conor.Dooley]

On 17/06/2022 14:09, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Jun 17, 2022 at 11:50:13AM +0000, Conor.Dooley@microchip.com wrote:
>> On 17/06/2022 12:44, Conor Dooley wrote:
>>> Hey Uwe,
>>> Got a ~v2~ v3 for you...
>>> I added some comments explaining the calculations and a documentation link
>>> so hopefully things are a bit easier to follow.
>>>
>>> Code wise, I went through and sorted out a bunch of issues that cycling
>>> through the different periods/duties threw up. Along the way I found
>>> some other problems - especially with the longer periods which I have
>>> fixed. I also added a write to the sync register in the apply function,
>>> which will resolve to a NOP for channels without "shadow registers".
>>>
>>> Other than that, I managed to ditch the mchp_core_pwm_registers struct
>>> entirely but had to add a short delay before reading back the registers
>>> in order to compute the duty.
>>>
>>> Thanks,
>>> Conor.
>>
>> *sigh* yet again I forgot to mention the potential maintainers conflict
>> with spi-next..
> 
> I'm not a maintainer of a very active subsystem where MAINTAINER
> conflicts are normal, but my expectation up to now was that conflicts in
> that file are quite usual and trivial to resolve such that mentioning
> these isn't very important.
> 

Yeah, I figured such conflicts were rather normal but felt like it was
better to say it just in case.
Thanks,
Conor.

Re: [PATCH v3 0/2] Add support for Microchip's pwm fpga core

[Conor.Dooley]

On 17/06/2022 14:09, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Jun 17, 2022 at 11:50:13AM +0000, Conor.Dooley@microchip.com wrote:
>> On 17/06/2022 12:44, Conor Dooley wrote:
>>> Hey Uwe,
>>> Got a ~v2~ v3 for you...
>>> I added some comments explaining the calculations and a documentation link
>>> so hopefully things are a bit easier to follow.
>>>
>>> Code wise, I went through and sorted out a bunch of issues that cycling
>>> through the different periods/duties threw up. Along the way I found
>>> some other problems - especially with the longer periods which I have
>>> fixed. I also added a write to the sync register in the apply function,
>>> which will resolve to a NOP for channels without "shadow registers".
>>>
>>> Other than that, I managed to ditch the mchp_core_pwm_registers struct
>>> entirely but had to add a short delay before reading back the registers
>>> in order to compute the duty.
>>>
>>> Thanks,
>>> Conor.
>>
>> *sigh* yet again I forgot to mention the potential maintainers conflict
>> with spi-next..
> 
> I'm not a maintainer of a very active subsystem where MAINTAINER
> conflicts are normal, but my expectation up to now was that conflicts in
> that file are quite usual and trivial to resolve such that mentioning
> these isn't very important.
> 

Yeah, I figured such conflicts were rather normal but felt like it was
better to say it just in case.
Thanks,
Conor.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Uwe Kleine-König]

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

Hello Conor,

On Fri, Jun 17, 2022 at 12:44:42PM +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 | 325 +++++++++++++++++++++++++++++++
>  3 files changed, 336 insertions(+)
>  create mode 100644 drivers/pwm/pwm-microchip-core.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 21e3b05a5153..a651848e444b 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -383,6 +383,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 708840b7fba8..d29754c20f91 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -33,6 +33,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..abbfc1cd23c4
> --- /dev/null
> +++ b/drivers/pwm/pwm-microchip-core.c
> @@ -0,0 +1,325 @@
> +// 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.

Ah, that behaviour explains how the hardware works. The logic is:

	if $currently_high:
	    if $clkcnt = $negedge:
	        set(low)
	else:
	    if $clkcnt = $posedge:
	        set(high)

The same problem exists for 100% relative duty cycle, doesn't it?

How does the PWM behave with:

	PERIOD = 0xfe
	POSEDGE = 0xff
	NEGEDGE = 0

I assume this yields constant low output. How does that change if you set
PERIOD = 0xff? If the output isn't constant low then, maybe that's a
reason to not permit PERIOD = 0xff.

Below you configure for duty_cycle = 0:

	POSEDGE = PERIOD
	NEGEDGE = 0

In my understanding this doesn't result in a constant output?!

> + * - 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/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/math.h>
> +
> +#define PREG_TO_VAL(PREG) ((PREG) + 1)
> +
> +#define COREPWM_PRESCALE_REG	0x00u
> +#define COREPWM_PERIOD_REG	0x04u
> +#define COREPWM_EN_LOW_REG	0x08u
> +#define COREPWM_EN_HIGH_REG	0x0Cu
> +#define COREPWM_SYNC_UPD_REG	0xE4u
> +#define COREPWM_POSEDGE_OFFSET	0x10u
> +#define COREPWM_NEGEDGE_OFFSET	0x14u
> +#define COREPWM_CHANNEL_OFFSET	0x08u

I'd define the registers as follows:

	#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

This is IMHO a bit better to understand and simplifies usage.

> +
> +struct mchp_core_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *base;
> +	u32 sync_update_mask;
> +};
> +
> +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)
> +{
> +	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 = COREPWM_EN_LOW_REG + (pwm->hwpwm >> 3) * sizeof(u32);
> +	shift = pwm->hwpwm > 7 ? pwm->hwpwm - 8 : pwm->hwpwm;
> +
> +	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);
> +
> +	/*
> +	 * Write to the sync update registers so that channels with shadow
> +	 * registers will also get their enable update. This operation is a NOP
> +	 * for channels without shadow registers.
> +	 */
> +	writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);

Hmmmmm, this is racy. Consider there are two PWMs in use and two
pwm_apply calls are run in parallel. Then the sync update in the first
execution thread triggers an update for the second which might just be
in the middle of updating registers and so there is a glitch for the 2nd
PWM. So this needs locking to behave correctly.

> +}
> +
> +static void mchp_core_pwm_apply_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);
> +	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
> +	u64 duty_steps, period, tmp;
> +	u8 posedge, negedge;
> +	u8 prescale_val = PREG_TO_VAL(prescale);

If prescale is 0xff we get prescale_val = 0 which is bogus.

> +	u8 period_steps_val = PREG_TO_VAL(period_steps);
> +
> +	period = period_steps_val * prescale_val * NSEC_PER_SEC;
> +	period = div64_u64(period, clk_get_rate(mchp_core_pwm->clk));

You need to round up here.

> +	/*
> +	 * 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.
> +	 *
> +	 * 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.
> +	 */
> +	if (state->duty_cycle > period) {
> +		duty_steps = period_steps;
> +	} else {
> +		duty_steps = state->duty_cycle * clk_get_rate(mchp_core_pwm->clk);
> +		tmp = prescale_val * NSEC_PER_SEC;
> +		duty_steps = div64_u64(duty_steps, tmp);
> +	}
> +
> +	/*
> +	 * Turn the output on unless posedge == negedge, in which case the
> +	 * duty is intended to be 0, but limitations of the IP block don't
> +	 * allow a zero length duty cycle - so just set the max high/low time
> +	 * respectively.
> +	 */
> +	if (state->polarity == PWM_POLARITY_INVERSED) {
> +		negedge = !duty_steps ? period_steps : 0u;
> +		posedge = duty_steps;
> +	} else {
> +		posedge = !duty_steps ? period_steps : 0u;
> +		negedge = duty_steps;
> +	}
> +
> +	writel_relaxed(posedge, channel_base + COREPWM_POSEDGE_OFFSET);
> +	writel_relaxed(negedge, channel_base + COREPWM_NEGEDGE_OFFSET);
> +}
> +
> +static void mchp_core_pwm_apply_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 = state->period;
> +
> +	/*
> +	 * Calculate the period cycles and prescale values.
> +	 * The registers are each 8 bits wide & multiplied to compute the period
> +	 * so the maximum period that can be generated is 0xFFFF times the period

0xff * 0xff != 0xffff

> +	 * of the input clock.
> +	 */
> +	tmp *= clk_get_rate(mchp_core_pwm->clk);

This might overflow. Better use mul_u64_u64_div_u64 and require
clk_get_rate(mchp_core_pwm->clk) < NSEC_PER_SEC.

> +	do_div(tmp, NSEC_PER_SEC);
> +
> +	if (tmp > 0xFFFFu) {
> +		*prescale = 0xFFu;
> +		*period_steps = 0xFFu;
> +	} else {
> +		*prescale = tmp >> 8;
> +		do_div(tmp, PREG_TO_VAL(*prescale));
> +		*period_steps = tmp - 1;
> +	}

The goal here is to determine prescale and period_steps such that (in
order of importance):

 a) Both are in [0, 255]
 b) (prescale + 1) * (period_steps + 1) <= tmp
 c) (prescale + 1) * (period_steps + 1) should be big
 d) period_steps should be big[1]

(If it simplifies the implementation you can also put d) above c))

So there are a few things to improve here. For example with tmp = 0xfffe
you get prescale = 0xff + period_steps = 0xfe which violates d)

[1] In most cases this is beneficial as a big period_steps value allows
    a more finegrained selection for duty_cycle.

> +	writel_relaxed(*prescale, mchp_core_pwm->base + COREPWM_PRESCALE_REG);
> +	writel_relaxed(*period_steps, mchp_core_pwm->base + COREPWM_PERIOD_REG);
> +}
> +
> +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;
> +	u16 channel_enabled;
> +	u8 prescale, period_steps;
> +
> +	if (!state->enabled) {
> +		mchp_core_pwm_enable(chip, pwm, false);
> +		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.
> +	 */
> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
> +		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));

The bits 15:8 of COREPWM_EN_LOW_REG are always zero I assume?

> +	period_locked = channel_enabled & ~(1 << pwm->hwpwm);

This is racy, too.

> +	if ((!current_state.enabled || current_state.period != state->period) && !period_locked) {
> +		mchp_core_pwm_apply_period(chip, state, &prescale, &period_steps);
> +	} else {
> +		prescale = readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG);
> +		period_steps = readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG);
> +	}

If the configured period is bigger than the requested one, you should
return -EINVAL.

> +	mchp_core_pwm_apply_duty(chip, pwm, state, prescale, period_steps);
> +
> +	/*
> +	 * 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.
> +	 */
> +	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
> +		writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
> +		usleep_range(state->period, state->period * 2);
> +	}
> +
> +	mchp_core_pwm_enable(chip, pwm, true);

mchp_core_pwm_enable writes the COREPWM_SYNC_UPD_REG register, too. Do
you really need to write it twice?

> +	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);
> +	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
> +	u8 prescale, period_steps, duty_steps;
> +	u8 posedge, negedge;
> +	u16 channel_enabled;
> +
> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
> +		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
> +
> +	if (channel_enabled & 1 << pwm->hwpwm)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;
> +
> +	prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG));
> +
> +	posedge = readb_relaxed(channel_base + COREPWM_POSEDGE_OFFSET);
> +	negedge = readb_relaxed(channel_base + COREPWM_NEGEDGE_OFFSET);
> +
> +	duty_steps = abs((s16)posedge - (s16)negedge);
> +	state->duty_cycle = duty_steps * prescale * NSEC_PER_SEC;
> +	do_div(state->duty_cycle, clk_get_rate(mchp_core_pwm->clk));
> +
> +	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +
> +	period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG));
> +	state->period = period_steps * prescale * NSEC_PER_SEC;
> +	do_div(state->period, clk_get_rate(mchp_core_pwm->clk));
> +}
> +
> +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(&pdev->dev, NULL);
> +	if (IS_ERR(mchp_pwm->clk))
> +		return PTR_ERR(mchp_pwm->clk);
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
> +				 &mchp_pwm->sync_update_mask))
> +		mchp_pwm->sync_update_mask = 0u;
> +
> +	ret = clk_prepare_enable(mchp_pwm->clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to prepare PWM clock\n");
> +
> +	mchp_pwm->chip.dev = &pdev->dev;
> +	mchp_pwm->chip.ops = &mchp_core_pwm_ops;
> +	mchp_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> +	mchp_pwm->chip.of_pwm_n_cells = 3;

You can drop these two, assuming you have #pwm-cells = <3> in the dtb,
they are also assigned by the core like that. (And if you don't it's
ugly to assign these and you're better of the the stuff that the pwm
core does.)

> +	mchp_pwm->chip.npwm = 16;
> +
> +	ret = pwmchip_add(&mchp_pwm->chip);
> +	if (ret < 0) {
> +		clk_disable_unprepare(mchp_pwm->clk);
> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +	}
> +
> +	platform_set_drvdata(pdev, mchp_pwm);
> +	dev_info(&pdev->dev, "Successfully registered Microchip corePWM\n");

I recommend to drop this message. If every driver does that, you get
quite a lot of messages that are not very helpful (once development of
the driver is done) and delay the boot up time.

> +	return 0;
> +}
> +
> +static int mchp_core_pwm_remove(struct platform_device *pdev)
> +{
> +	struct mchp_core_pwm_chip *mchp_pwm = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(&mchp_pwm->chip);
> +	clk_disable_unprepare(mchp_pwm->clk);

If you use devm_clk_get_enabled() and devm_pwmchip_add() in .probe(),
you can drop .remove()

> +
> +	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,
> +	.remove = mchp_core_pwm_remove,
> +};
> +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");

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 17505 bytes --]

Hello Conor,

On Fri, Jun 17, 2022 at 12:44:42PM +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 | 325 +++++++++++++++++++++++++++++++
>  3 files changed, 336 insertions(+)
>  create mode 100644 drivers/pwm/pwm-microchip-core.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 21e3b05a5153..a651848e444b 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -383,6 +383,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 708840b7fba8..d29754c20f91 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -33,6 +33,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..abbfc1cd23c4
> --- /dev/null
> +++ b/drivers/pwm/pwm-microchip-core.c
> @@ -0,0 +1,325 @@
> +// 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.

Ah, that behaviour explains how the hardware works. The logic is:

	if $currently_high:
	    if $clkcnt = $negedge:
	        set(low)
	else:
	    if $clkcnt = $posedge:
	        set(high)

The same problem exists for 100% relative duty cycle, doesn't it?

How does the PWM behave with:

	PERIOD = 0xfe
	POSEDGE = 0xff
	NEGEDGE = 0

I assume this yields constant low output. How does that change if you set
PERIOD = 0xff? If the output isn't constant low then, maybe that's a
reason to not permit PERIOD = 0xff.

Below you configure for duty_cycle = 0:

	POSEDGE = PERIOD
	NEGEDGE = 0

In my understanding this doesn't result in a constant output?!

> + * - 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/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/math.h>
> +
> +#define PREG_TO_VAL(PREG) ((PREG) + 1)
> +
> +#define COREPWM_PRESCALE_REG	0x00u
> +#define COREPWM_PERIOD_REG	0x04u
> +#define COREPWM_EN_LOW_REG	0x08u
> +#define COREPWM_EN_HIGH_REG	0x0Cu
> +#define COREPWM_SYNC_UPD_REG	0xE4u
> +#define COREPWM_POSEDGE_OFFSET	0x10u
> +#define COREPWM_NEGEDGE_OFFSET	0x14u
> +#define COREPWM_CHANNEL_OFFSET	0x08u

I'd define the registers as follows:

	#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

This is IMHO a bit better to understand and simplifies usage.

> +
> +struct mchp_core_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *base;
> +	u32 sync_update_mask;
> +};
> +
> +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)
> +{
> +	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 = COREPWM_EN_LOW_REG + (pwm->hwpwm >> 3) * sizeof(u32);
> +	shift = pwm->hwpwm > 7 ? pwm->hwpwm - 8 : pwm->hwpwm;
> +
> +	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);
> +
> +	/*
> +	 * Write to the sync update registers so that channels with shadow
> +	 * registers will also get their enable update. This operation is a NOP
> +	 * for channels without shadow registers.
> +	 */
> +	writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);

Hmmmmm, this is racy. Consider there are two PWMs in use and two
pwm_apply calls are run in parallel. Then the sync update in the first
execution thread triggers an update for the second which might just be
in the middle of updating registers and so there is a glitch for the 2nd
PWM. So this needs locking to behave correctly.

> +}
> +
> +static void mchp_core_pwm_apply_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);
> +	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
> +	u64 duty_steps, period, tmp;
> +	u8 posedge, negedge;
> +	u8 prescale_val = PREG_TO_VAL(prescale);

If prescale is 0xff we get prescale_val = 0 which is bogus.

> +	u8 period_steps_val = PREG_TO_VAL(period_steps);
> +
> +	period = period_steps_val * prescale_val * NSEC_PER_SEC;
> +	period = div64_u64(period, clk_get_rate(mchp_core_pwm->clk));

You need to round up here.

> +	/*
> +	 * 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.
> +	 *
> +	 * 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.
> +	 */
> +	if (state->duty_cycle > period) {
> +		duty_steps = period_steps;
> +	} else {
> +		duty_steps = state->duty_cycle * clk_get_rate(mchp_core_pwm->clk);
> +		tmp = prescale_val * NSEC_PER_SEC;
> +		duty_steps = div64_u64(duty_steps, tmp);
> +	}
> +
> +	/*
> +	 * Turn the output on unless posedge == negedge, in which case the
> +	 * duty is intended to be 0, but limitations of the IP block don't
> +	 * allow a zero length duty cycle - so just set the max high/low time
> +	 * respectively.
> +	 */
> +	if (state->polarity == PWM_POLARITY_INVERSED) {
> +		negedge = !duty_steps ? period_steps : 0u;
> +		posedge = duty_steps;
> +	} else {
> +		posedge = !duty_steps ? period_steps : 0u;
> +		negedge = duty_steps;
> +	}
> +
> +	writel_relaxed(posedge, channel_base + COREPWM_POSEDGE_OFFSET);
> +	writel_relaxed(negedge, channel_base + COREPWM_NEGEDGE_OFFSET);
> +}
> +
> +static void mchp_core_pwm_apply_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 = state->period;
> +
> +	/*
> +	 * Calculate the period cycles and prescale values.
> +	 * The registers are each 8 bits wide & multiplied to compute the period
> +	 * so the maximum period that can be generated is 0xFFFF times the period

0xff * 0xff != 0xffff

> +	 * of the input clock.
> +	 */
> +	tmp *= clk_get_rate(mchp_core_pwm->clk);

This might overflow. Better use mul_u64_u64_div_u64 and require
clk_get_rate(mchp_core_pwm->clk) < NSEC_PER_SEC.

> +	do_div(tmp, NSEC_PER_SEC);
> +
> +	if (tmp > 0xFFFFu) {
> +		*prescale = 0xFFu;
> +		*period_steps = 0xFFu;
> +	} else {
> +		*prescale = tmp >> 8;
> +		do_div(tmp, PREG_TO_VAL(*prescale));
> +		*period_steps = tmp - 1;
> +	}

The goal here is to determine prescale and period_steps such that (in
order of importance):

 a) Both are in [0, 255]
 b) (prescale + 1) * (period_steps + 1) <= tmp
 c) (prescale + 1) * (period_steps + 1) should be big
 d) period_steps should be big[1]

(If it simplifies the implementation you can also put d) above c))

So there are a few things to improve here. For example with tmp = 0xfffe
you get prescale = 0xff + period_steps = 0xfe which violates d)

[1] In most cases this is beneficial as a big period_steps value allows
    a more finegrained selection for duty_cycle.

> +	writel_relaxed(*prescale, mchp_core_pwm->base + COREPWM_PRESCALE_REG);
> +	writel_relaxed(*period_steps, mchp_core_pwm->base + COREPWM_PERIOD_REG);
> +}
> +
> +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;
> +	u16 channel_enabled;
> +	u8 prescale, period_steps;
> +
> +	if (!state->enabled) {
> +		mchp_core_pwm_enable(chip, pwm, false);
> +		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.
> +	 */
> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
> +		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));

The bits 15:8 of COREPWM_EN_LOW_REG are always zero I assume?

> +	period_locked = channel_enabled & ~(1 << pwm->hwpwm);

This is racy, too.

> +	if ((!current_state.enabled || current_state.period != state->period) && !period_locked) {
> +		mchp_core_pwm_apply_period(chip, state, &prescale, &period_steps);
> +	} else {
> +		prescale = readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG);
> +		period_steps = readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG);
> +	}

If the configured period is bigger than the requested one, you should
return -EINVAL.

> +	mchp_core_pwm_apply_duty(chip, pwm, state, prescale, period_steps);
> +
> +	/*
> +	 * 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.
> +	 */
> +	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
> +		writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
> +		usleep_range(state->period, state->period * 2);
> +	}
> +
> +	mchp_core_pwm_enable(chip, pwm, true);

mchp_core_pwm_enable writes the COREPWM_SYNC_UPD_REG register, too. Do
you really need to write it twice?

> +	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);
> +	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
> +	u8 prescale, period_steps, duty_steps;
> +	u8 posedge, negedge;
> +	u16 channel_enabled;
> +
> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
> +		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
> +
> +	if (channel_enabled & 1 << pwm->hwpwm)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;
> +
> +	prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG));
> +
> +	posedge = readb_relaxed(channel_base + COREPWM_POSEDGE_OFFSET);
> +	negedge = readb_relaxed(channel_base + COREPWM_NEGEDGE_OFFSET);
> +
> +	duty_steps = abs((s16)posedge - (s16)negedge);
> +	state->duty_cycle = duty_steps * prescale * NSEC_PER_SEC;
> +	do_div(state->duty_cycle, clk_get_rate(mchp_core_pwm->clk));
> +
> +	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +
> +	period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG));
> +	state->period = period_steps * prescale * NSEC_PER_SEC;
> +	do_div(state->period, clk_get_rate(mchp_core_pwm->clk));
> +}
> +
> +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(&pdev->dev, NULL);
> +	if (IS_ERR(mchp_pwm->clk))
> +		return PTR_ERR(mchp_pwm->clk);
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
> +				 &mchp_pwm->sync_update_mask))
> +		mchp_pwm->sync_update_mask = 0u;
> +
> +	ret = clk_prepare_enable(mchp_pwm->clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to prepare PWM clock\n");
> +
> +	mchp_pwm->chip.dev = &pdev->dev;
> +	mchp_pwm->chip.ops = &mchp_core_pwm_ops;
> +	mchp_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> +	mchp_pwm->chip.of_pwm_n_cells = 3;

You can drop these two, assuming you have #pwm-cells = <3> in the dtb,
they are also assigned by the core like that. (And if you don't it's
ugly to assign these and you're better of the the stuff that the pwm
core does.)

> +	mchp_pwm->chip.npwm = 16;
> +
> +	ret = pwmchip_add(&mchp_pwm->chip);
> +	if (ret < 0) {
> +		clk_disable_unprepare(mchp_pwm->clk);
> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +	}
> +
> +	platform_set_drvdata(pdev, mchp_pwm);
> +	dev_info(&pdev->dev, "Successfully registered Microchip corePWM\n");

I recommend to drop this message. If every driver does that, you get
quite a lot of messages that are not very helpful (once development of
the driver is done) and delay the boot up time.

> +	return 0;
> +}
> +
> +static int mchp_core_pwm_remove(struct platform_device *pdev)
> +{
> +	struct mchp_core_pwm_chip *mchp_pwm = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(&mchp_pwm->chip);
> +	clk_disable_unprepare(mchp_pwm->clk);

If you use devm_clk_get_enabled() and devm_pwmchip_add() in .probe(),
you can drop .remove()

> +
> +	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,
> +	.remove = mchp_core_pwm_remove,
> +};
> +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");

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

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Uwe Kleine-König]

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

Hello,

On Fri, Jun 17, 2022 at 12:44:43PM +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>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f1b4b77daa5f..d0b39fa4f309 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17091,6 +17091,7 @@ L:	linux-riscv@lists.infradead.org
>  S:	Supported
>  F:	arch/riscv/boot/dts/microchip/
>  F:	drivers/mailbox/mailbox-mpfs.c
> +F:	drivers/pwm/pwm-microchip-core.c
>  F:	drivers/soc/microchip/
>  F:	include/soc/microchip/mpfs.h

The change looks okish to me, but it doesn't make sense without patch
1/2. So I'm discarding this one from our patchwork instance, 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 v3 2/2] MAINTAINERS: add pwm to PolarFire SoC entry

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 989 bytes --]

Hello,

On Fri, Jun 17, 2022 at 12:44:43PM +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>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f1b4b77daa5f..d0b39fa4f309 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17091,6 +17091,7 @@ L:	linux-riscv@lists.infradead.org
>  S:	Supported
>  F:	arch/riscv/boot/dts/microchip/
>  F:	drivers/mailbox/mailbox-mpfs.c
> +F:	drivers/pwm/pwm-microchip-core.c
>  F:	drivers/soc/microchip/
>  F:	include/soc/microchip/mpfs.h

The change looks okish to me, but it doesn't make sense without patch
1/2. So I'm discarding this one from our patchwork instance, too.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor.Dooley]

On 01/07/2022 10:51, Uwe Kleine-König wrote:
> Hello Conor,

Hey Uwe, thanks for the review!
I am on leave from work atm, doing my civic duty and all that so I don't
have a logic analyser on hand to do any testing with.

(comments I didn't reply to I agree with)
> 
> On Fri, Jun 17, 2022 at 12:44:42PM +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 | 325 +++++++++++++++++++++++++++++++
>>  3 files changed, 336 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-microchip-core.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 21e3b05a5153..a651848e444b 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -383,6 +383,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 708840b7fba8..d29754c20f91 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -33,6 +33,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..abbfc1cd23c4
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-microchip-core.c
>> @@ -0,0 +1,325 @@
>> +// 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.
> 
> Ah, that behaviour explains how the hardware works. The logic is:
> 
> 	if $currently_high:
> 	    if $clkcnt = $negedge:
> 	        set(low)
> 	else:
> 	    if $clkcnt = $posedge:
> 	        set(high)

I am just going to ask for the rtl when I get a chance I think.

 
> The same problem exists for 100% relative duty cycle, doesn't it?
> 
> How does the PWM behave with:
> 
> 	PERIOD = 0xfe
> 	POSEDGE = 0xff
> 	NEGEDGE = 0
> 
> I assume this yields constant low output.

This specific combo I have not tested, but afaik yes.

How does that change if you set
> PERIOD = 0xff? If the output isn't constant low then, maybe that's a
> reason to not permit PERIOD = 0xff.
> 
> Below you configure for duty_cycle = 0:
> 
> 	POSEDGE = PERIOD
> 	NEGEDGE = 0
> 
> In my understanding this doesn't result in a constant output?!

Hopefully the RTL will help me clear this up. I switched the code
to this because of the 50% thing mentioned above. In testing it,
I could not get my scope to trigger on the output for a range of
periods. But that does not really make very much sense looking at
the code now.

At the moment, a /PERIOD/ of 0xE (so a reg value of 0xD) will count
0x0 -> 0xD and then roll over to 0x0. 

posedge (the register value) is being set to 0xD not 0xE, so this
should not be a constant output. I think what must have happened
is that I had been using the variable "period_steps" to be the
real number of steps during my development/testing of V3. But when
I, at the very end, was testing with shadow register enabled I
found an idempotency issue. I had attempted to remove the
period_steps & prescale args to mchp_core_pwm_apply_duty() and just
read them locally - but with shadow registers enabled the reg values
visible on the bus were the old values so the calculation was wrong
in cases where the period was changing.

When I readded them to the signature, I renamed the real period_steps
value to period_steps_val & must not have updated or properly tested
the 0/100% duty cycles. Bleh.

Not 100% on this, but fairly sure that this is the case. Sorry.

> 
>> + * - 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/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/math.h>
>> +
>> +#define PREG_TO_VAL(PREG) ((PREG) + 1)
>> +
>> +#define COREPWM_PRESCALE_REG	0x00u
>> +#define COREPWM_PERIOD_REG	0x04u
>> +#define COREPWM_EN_LOW_REG	0x08u
>> +#define COREPWM_EN_HIGH_REG	0x0Cu
>> +#define COREPWM_SYNC_UPD_REG	0xE4u
>> +#define COREPWM_POSEDGE_OFFSET	0x10u
>> +#define COREPWM_NEGEDGE_OFFSET	0x14u
>> +#define COREPWM_CHANNEL_OFFSET	0x08u
> 
> I'd define the registers as follows:
> 
> 	#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
> 
> This is IMHO a bit better to understand and simplifies usage.

Sure.

> 
>> +
>> +struct mchp_core_pwm_chip {
>> +	struct pwm_chip chip;
>> +	struct clk *clk;
>> +	void __iomem *base;
>> +	u32 sync_update_mask;
>> +};
>> +
>> +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)
>> +{
>> +	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 = COREPWM_EN_LOW_REG + (pwm->hwpwm >> 3) * sizeof(u32);
>> +	shift = pwm->hwpwm > 7 ? pwm->hwpwm - 8 : pwm->hwpwm;
>> +
>> +	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);
>> +
>> +	/*
>> +	 * Write to the sync update registers so that channels with shadow
>> +	 * registers will also get their enable update. This operation is a NOP
>> +	 * for channels without shadow registers.
>> +	 */
>> +	writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
> 
> Hmmmmm, this is racy. Consider there are two PWMs in use and two
> pwm_apply calls are run in parallel. Then the sync update in the first
> execution thread triggers an update for the second which might just be
> in the middle of updating registers and so there is a glitch for the 2nd
> PWM. So this needs locking to behave correctly.

Yeah, SGTM.

> 
>> +}
>> +
>> +static void mchp_core_pwm_apply_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);
>> +	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
>> +	u64 duty_steps, period, tmp;
>> +	u8 posedge, negedge;
>> +	u8 prescale_val = PREG_TO_VAL(prescale);
> 
> If prescale is 0xff we get prescale_val = 0 which is bogus.

"yes"
> 
>> +	u8 period_steps_val = PREG_TO_VAL(period_steps);
>> +
>> +	period = period_steps_val * prescale_val * NSEC_PER_SEC;
>> +	period = div64_u64(period, clk_get_rate(mchp_core_pwm->clk));
> 
> You need to round up here.
> 
>> +	/*
>> +	 * 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.
>> +	 *
>> +	 * 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.
>> +	 */
>> +	if (state->duty_cycle > period) {
>> +		duty_steps = period_steps;
>> +	} else {
>> +		duty_steps = state->duty_cycle * clk_get_rate(mchp_core_pwm->clk);
>> +		tmp = prescale_val * NSEC_PER_SEC;
>> +		duty_steps = div64_u64(duty_steps, tmp);
>> +	}
>> +
>> +	/*
>> +	 * Turn the output on unless posedge == negedge, in which case the
>> +	 * duty is intended to be 0, but limitations of the IP block don't
>> +	 * allow a zero length duty cycle - so just set the max high/low time
>> +	 * respectively.
>> +	 */
>> +	if (state->polarity == PWM_POLARITY_INVERSED) {
>> +		negedge = !duty_steps ? period_steps : 0u;
>> +		posedge = duty_steps;
>> +	} else {
>> +		posedge = !duty_steps ? period_steps : 0u;
>> +		negedge = duty_steps;
>> +	}
>> +
>> +	writel_relaxed(posedge, channel_base + COREPWM_POSEDGE_OFFSET);
>> +	writel_relaxed(negedge, channel_base + COREPWM_NEGEDGE_OFFSET);
>> +}
>> +
>> +static void mchp_core_pwm_apply_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 = state->period;
>> +
>> +	/*
>> +	 * Calculate the period cycles and prescale values.
>> +	 * The registers are each 8 bits wide & multiplied to compute the period
>> +	 * so the maximum period that can be generated is 0xFFFF times the period
> 
> 0xff * 0xff != 0xffff

Gah.. That was silly of me.
(0xff + 0x1)^2 is the correct max as the period calculation is:
(clock_period) * (prescale_reg + 1) * (period_reg + 1)
Still wrong but less wrong than 0xff * 0xff I guess.

> 
>> +	 * of the input clock.
>> +	 */
>> +	tmp *= clk_get_rate(mchp_core_pwm->clk);
> 
> This might overflow. Better use mul_u64_u64_div_u64 and require

mul_u64_u64_div_u64(), what a mouthful haha.
willdo.

> clk_get_rate(mchp_core_pwm->clk) < NSEC_PER_SEC.
> 
>> +	do_div(tmp, NSEC_PER_SEC);
>> +
>> +	if (tmp > 0xFFFFu) {
>> +		*prescale = 0xFFu;
>> +		*period_steps = 0xFFu;
>> +	} else {
>> +		*prescale = tmp >> 8;
>> +		do_div(tmp, PREG_TO_VAL(*prescale));
>> +		*period_steps = tmp - 1;
>> +	}
> 
> The goal here is to determine prescale and period_steps such that (in
> order of importance):
> 
>  a) Both are in [0, 255]
>  b) (prescale + 1) * (period_steps + 1) <= tmp
>  c) (prescale + 1) * (period_steps + 1) should be big
>  d) period_steps should be big[1]
> 
> (If it simplifies the implementation you can also put d) above c))> 
> So there are a few things to improve here. For example with tmp = 0xfffe
> you get prescale = 0xff + period_steps = 0xfe which violates d)

Aight, I'll take another look at optimising this so.


> 
> [1] In most cases this is beneficial as a big period_steps value allows
>     a more finegrained selection for duty_cycle.
> 
>> +	writel_relaxed(*prescale, mchp_core_pwm->base + COREPWM_PRESCALE_REG);
>> +	writel_relaxed(*period_steps, mchp_core_pwm->base + COREPWM_PERIOD_REG);
>> +}
>> +
>> +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;
>> +	u16 channel_enabled;
>> +	u8 prescale, period_steps;
>> +
>> +	if (!state->enabled) {
>> +		mchp_core_pwm_enable(chip, pwm, false);
>> +		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.
>> +	 */
>> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
>> +		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
> 
> The bits 15:8 of COREPWM_EN_LOW_REG are always zero I assume?

Yeah

> 
>> +	period_locked = channel_enabled & ~(1 << pwm->hwpwm);
> 
> This is racy, too.
> 
>> +	if ((!current_state.enabled || current_state.period != state->period) && !period_locked) {
>> +		mchp_core_pwm_apply_period(chip, state, &prescale, &period_steps);
>> +	} else {
>> +		prescale = readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG);
>> +		period_steps = readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG);
>> +	}
> 
> If the configured period is bigger than the requested one, you should
> return -EINVAL.

Cool, willdo.

> 
>> +	mchp_core_pwm_apply_duty(chip, pwm, state, prescale, period_steps);
>> +
>> +	/*
>> +	 * 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.
>> +	 */
>> +	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
>> +		writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
>> +		usleep_range(state->period, state->period * 2);
>> +	}
>> +
>> +	mchp_core_pwm_enable(chip, pwm, true);
> 
> mchp_core_pwm_enable writes the COREPWM_SYNC_UPD_REG register, too. Do
> you really need to write it twice?

Yeah, good point.

> 
>> +	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);
>> +	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
>> +	u8 prescale, period_steps, duty_steps;
>> +	u8 posedge, negedge;
>> +	u16 channel_enabled;
>> +
>> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
>> +		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
>> +
>> +	if (channel_enabled & 1 << pwm->hwpwm)
>> +		state->enabled = true;
>> +	else
>> +		state->enabled = false;
>> +
>> +	prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG));
>> +
>> +	posedge = readb_relaxed(channel_base + COREPWM_POSEDGE_OFFSET);
>> +	negedge = readb_relaxed(channel_base + COREPWM_NEGEDGE_OFFSET);
>> +
>> +	duty_steps = abs((s16)posedge - (s16)negedge);
>> +	state->duty_cycle = duty_steps * prescale * NSEC_PER_SEC;
>> +	do_div(state->duty_cycle, clk_get_rate(mchp_core_pwm->clk));
>> +
>> +	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
>> +
>> +	period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG));
>> +	state->period = period_steps * prescale * NSEC_PER_SEC;
>> +	do_div(state->period, clk_get_rate(mchp_core_pwm->clk));
>> +}
>> +
>> +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(&pdev->dev, NULL);
>> +	if (IS_ERR(mchp_pwm->clk))
>> +		return PTR_ERR(mchp_pwm->clk);
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
>> +				 &mchp_pwm->sync_update_mask))
>> +		mchp_pwm->sync_update_mask = 0u;
>> +
>> +	ret = clk_prepare_enable(mchp_pwm->clk);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "failed to prepare PWM clock\n");
>> +
>> +	mchp_pwm->chip.dev = &pdev->dev;
>> +	mchp_pwm->chip.ops = &mchp_core_pwm_ops;
>> +	mchp_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
>> +	mchp_pwm->chip.of_pwm_n_cells = 3;
> 
> You can drop these two, assuming you have #pwm-cells = <3> in the dtb,
> they are also assigned by the core like that. (And if you don't it's
> ugly to assign these and you're better of the the stuff that the pwm
> core does.)

The dts actually says 2 at the moment so that needs fixing..

> 
>> +	mchp_pwm->chip.npwm = 16;
>> +
>> +	ret = pwmchip_add(&mchp_pwm->chip);
>> +	if (ret < 0) {
>> +		clk_disable_unprepare(mchp_pwm->clk);
>> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
>> +	}
>> +
>> +	platform_set_drvdata(pdev, mchp_pwm);
>> +	dev_info(&pdev->dev, "Successfully registered Microchip corePWM\n");
> 
> I recommend to drop this message. If every driver does that, you get
> quite a lot of messages that are not very helpful (once development of
> the driver is done) and delay the boot up time.

My CI likes them, but if you don't - I am happy to drop it.

> 
>> +	return 0;
>> +}
>> +
>> +static int mchp_core_pwm_remove(struct platform_device *pdev)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_pwm = platform_get_drvdata(pdev);
>> +
>> +	pwmchip_remove(&mchp_pwm->chip);
>> +	clk_disable_unprepare(mchp_pwm->clk);
> 
> If you use devm_clk_get_enabled() and devm_pwmchip_add() in .probe(),
> you can drop .remove()

SGTM

> 
>> +
>> +	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,
>> +	.remove = mchp_core_pwm_remove,
>> +};
>> +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");
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor.Dooley]

On 01/07/2022 10:51, Uwe Kleine-König wrote:
> Hello Conor,

Hey Uwe, thanks for the review!
I am on leave from work atm, doing my civic duty and all that so I don't
have a logic analyser on hand to do any testing with.

(comments I didn't reply to I agree with)
> 
> On Fri, Jun 17, 2022 at 12:44:42PM +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 | 325 +++++++++++++++++++++++++++++++
>>  3 files changed, 336 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-microchip-core.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 21e3b05a5153..a651848e444b 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -383,6 +383,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 708840b7fba8..d29754c20f91 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -33,6 +33,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..abbfc1cd23c4
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-microchip-core.c
>> @@ -0,0 +1,325 @@
>> +// 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.
> 
> Ah, that behaviour explains how the hardware works. The logic is:
> 
> 	if $currently_high:
> 	    if $clkcnt = $negedge:
> 	        set(low)
> 	else:
> 	    if $clkcnt = $posedge:
> 	        set(high)

I am just going to ask for the rtl when I get a chance I think.

 
> The same problem exists for 100% relative duty cycle, doesn't it?
> 
> How does the PWM behave with:
> 
> 	PERIOD = 0xfe
> 	POSEDGE = 0xff
> 	NEGEDGE = 0
> 
> I assume this yields constant low output.

This specific combo I have not tested, but afaik yes.

How does that change if you set
> PERIOD = 0xff? If the output isn't constant low then, maybe that's a
> reason to not permit PERIOD = 0xff.
> 
> Below you configure for duty_cycle = 0:
> 
> 	POSEDGE = PERIOD
> 	NEGEDGE = 0
> 
> In my understanding this doesn't result in a constant output?!

Hopefully the RTL will help me clear this up. I switched the code
to this because of the 50% thing mentioned above. In testing it,
I could not get my scope to trigger on the output for a range of
periods. But that does not really make very much sense looking at
the code now.

At the moment, a /PERIOD/ of 0xE (so a reg value of 0xD) will count
0x0 -> 0xD and then roll over to 0x0. 

posedge (the register value) is being set to 0xD not 0xE, so this
should not be a constant output. I think what must have happened
is that I had been using the variable "period_steps" to be the
real number of steps during my development/testing of V3. But when
I, at the very end, was testing with shadow register enabled I
found an idempotency issue. I had attempted to remove the
period_steps & prescale args to mchp_core_pwm_apply_duty() and just
read them locally - but with shadow registers enabled the reg values
visible on the bus were the old values so the calculation was wrong
in cases where the period was changing.

When I readded them to the signature, I renamed the real period_steps
value to period_steps_val & must not have updated or properly tested
the 0/100% duty cycles. Bleh.

Not 100% on this, but fairly sure that this is the case. Sorry.

> 
>> + * - 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/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/math.h>
>> +
>> +#define PREG_TO_VAL(PREG) ((PREG) + 1)
>> +
>> +#define COREPWM_PRESCALE_REG	0x00u
>> +#define COREPWM_PERIOD_REG	0x04u
>> +#define COREPWM_EN_LOW_REG	0x08u
>> +#define COREPWM_EN_HIGH_REG	0x0Cu
>> +#define COREPWM_SYNC_UPD_REG	0xE4u
>> +#define COREPWM_POSEDGE_OFFSET	0x10u
>> +#define COREPWM_NEGEDGE_OFFSET	0x14u
>> +#define COREPWM_CHANNEL_OFFSET	0x08u
> 
> I'd define the registers as follows:
> 
> 	#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
> 
> This is IMHO a bit better to understand and simplifies usage.

Sure.

> 
>> +
>> +struct mchp_core_pwm_chip {
>> +	struct pwm_chip chip;
>> +	struct clk *clk;
>> +	void __iomem *base;
>> +	u32 sync_update_mask;
>> +};
>> +
>> +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)
>> +{
>> +	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 = COREPWM_EN_LOW_REG + (pwm->hwpwm >> 3) * sizeof(u32);
>> +	shift = pwm->hwpwm > 7 ? pwm->hwpwm - 8 : pwm->hwpwm;
>> +
>> +	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);
>> +
>> +	/*
>> +	 * Write to the sync update registers so that channels with shadow
>> +	 * registers will also get their enable update. This operation is a NOP
>> +	 * for channels without shadow registers.
>> +	 */
>> +	writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
> 
> Hmmmmm, this is racy. Consider there are two PWMs in use and two
> pwm_apply calls are run in parallel. Then the sync update in the first
> execution thread triggers an update for the second which might just be
> in the middle of updating registers and so there is a glitch for the 2nd
> PWM. So this needs locking to behave correctly.

Yeah, SGTM.

> 
>> +}
>> +
>> +static void mchp_core_pwm_apply_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);
>> +	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
>> +	u64 duty_steps, period, tmp;
>> +	u8 posedge, negedge;
>> +	u8 prescale_val = PREG_TO_VAL(prescale);
> 
> If prescale is 0xff we get prescale_val = 0 which is bogus.

"yes"
> 
>> +	u8 period_steps_val = PREG_TO_VAL(period_steps);
>> +
>> +	period = period_steps_val * prescale_val * NSEC_PER_SEC;
>> +	period = div64_u64(period, clk_get_rate(mchp_core_pwm->clk));
> 
> You need to round up here.
> 
>> +	/*
>> +	 * 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.
>> +	 *
>> +	 * 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.
>> +	 */
>> +	if (state->duty_cycle > period) {
>> +		duty_steps = period_steps;
>> +	} else {
>> +		duty_steps = state->duty_cycle * clk_get_rate(mchp_core_pwm->clk);
>> +		tmp = prescale_val * NSEC_PER_SEC;
>> +		duty_steps = div64_u64(duty_steps, tmp);
>> +	}
>> +
>> +	/*
>> +	 * Turn the output on unless posedge == negedge, in which case the
>> +	 * duty is intended to be 0, but limitations of the IP block don't
>> +	 * allow a zero length duty cycle - so just set the max high/low time
>> +	 * respectively.
>> +	 */
>> +	if (state->polarity == PWM_POLARITY_INVERSED) {
>> +		negedge = !duty_steps ? period_steps : 0u;
>> +		posedge = duty_steps;
>> +	} else {
>> +		posedge = !duty_steps ? period_steps : 0u;
>> +		negedge = duty_steps;
>> +	}
>> +
>> +	writel_relaxed(posedge, channel_base + COREPWM_POSEDGE_OFFSET);
>> +	writel_relaxed(negedge, channel_base + COREPWM_NEGEDGE_OFFSET);
>> +}
>> +
>> +static void mchp_core_pwm_apply_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 = state->period;
>> +
>> +	/*
>> +	 * Calculate the period cycles and prescale values.
>> +	 * The registers are each 8 bits wide & multiplied to compute the period
>> +	 * so the maximum period that can be generated is 0xFFFF times the period
> 
> 0xff * 0xff != 0xffff

Gah.. That was silly of me.
(0xff + 0x1)^2 is the correct max as the period calculation is:
(clock_period) * (prescale_reg + 1) * (period_reg + 1)
Still wrong but less wrong than 0xff * 0xff I guess.

> 
>> +	 * of the input clock.
>> +	 */
>> +	tmp *= clk_get_rate(mchp_core_pwm->clk);
> 
> This might overflow. Better use mul_u64_u64_div_u64 and require

mul_u64_u64_div_u64(), what a mouthful haha.
willdo.

> clk_get_rate(mchp_core_pwm->clk) < NSEC_PER_SEC.
> 
>> +	do_div(tmp, NSEC_PER_SEC);
>> +
>> +	if (tmp > 0xFFFFu) {
>> +		*prescale = 0xFFu;
>> +		*period_steps = 0xFFu;
>> +	} else {
>> +		*prescale = tmp >> 8;
>> +		do_div(tmp, PREG_TO_VAL(*prescale));
>> +		*period_steps = tmp - 1;
>> +	}
> 
> The goal here is to determine prescale and period_steps such that (in
> order of importance):
> 
>  a) Both are in [0, 255]
>  b) (prescale + 1) * (period_steps + 1) <= tmp
>  c) (prescale + 1) * (period_steps + 1) should be big
>  d) period_steps should be big[1]
> 
> (If it simplifies the implementation you can also put d) above c))> 
> So there are a few things to improve here. For example with tmp = 0xfffe
> you get prescale = 0xff + period_steps = 0xfe which violates d)

Aight, I'll take another look at optimising this so.


> 
> [1] In most cases this is beneficial as a big period_steps value allows
>     a more finegrained selection for duty_cycle.
> 
>> +	writel_relaxed(*prescale, mchp_core_pwm->base + COREPWM_PRESCALE_REG);
>> +	writel_relaxed(*period_steps, mchp_core_pwm->base + COREPWM_PERIOD_REG);
>> +}
>> +
>> +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;
>> +	u16 channel_enabled;
>> +	u8 prescale, period_steps;
>> +
>> +	if (!state->enabled) {
>> +		mchp_core_pwm_enable(chip, pwm, false);
>> +		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.
>> +	 */
>> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
>> +		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
> 
> The bits 15:8 of COREPWM_EN_LOW_REG are always zero I assume?

Yeah

> 
>> +	period_locked = channel_enabled & ~(1 << pwm->hwpwm);
> 
> This is racy, too.
> 
>> +	if ((!current_state.enabled || current_state.period != state->period) && !period_locked) {
>> +		mchp_core_pwm_apply_period(chip, state, &prescale, &period_steps);
>> +	} else {
>> +		prescale = readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG);
>> +		period_steps = readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG);
>> +	}
> 
> If the configured period is bigger than the requested one, you should
> return -EINVAL.

Cool, willdo.

> 
>> +	mchp_core_pwm_apply_duty(chip, pwm, state, prescale, period_steps);
>> +
>> +	/*
>> +	 * 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.
>> +	 */
>> +	if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
>> +		writel_relaxed(1U, mchp_core_pwm->base + COREPWM_SYNC_UPD_REG);
>> +		usleep_range(state->period, state->period * 2);
>> +	}
>> +
>> +	mchp_core_pwm_enable(chip, pwm, true);
> 
> mchp_core_pwm_enable writes the COREPWM_SYNC_UPD_REG register, too. Do
> you really need to write it twice?

Yeah, good point.

> 
>> +	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);
>> +	void __iomem *channel_base = mchp_core_pwm->base + pwm->hwpwm * COREPWM_CHANNEL_OFFSET;
>> +	u8 prescale, period_steps, duty_steps;
>> +	u8 posedge, negedge;
>> +	u16 channel_enabled;
>> +
>> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + COREPWM_EN_HIGH_REG) << 8) |
>> +		readb_relaxed(mchp_core_pwm->base + COREPWM_EN_LOW_REG));
>> +
>> +	if (channel_enabled & 1 << pwm->hwpwm)
>> +		state->enabled = true;
>> +	else
>> +		state->enabled = false;
>> +
>> +	prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PRESCALE_REG));
>> +
>> +	posedge = readb_relaxed(channel_base + COREPWM_POSEDGE_OFFSET);
>> +	negedge = readb_relaxed(channel_base + COREPWM_NEGEDGE_OFFSET);
>> +
>> +	duty_steps = abs((s16)posedge - (s16)negedge);
>> +	state->duty_cycle = duty_steps * prescale * NSEC_PER_SEC;
>> +	do_div(state->duty_cycle, clk_get_rate(mchp_core_pwm->clk));
>> +
>> +	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
>> +
>> +	period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + COREPWM_PERIOD_REG));
>> +	state->period = period_steps * prescale * NSEC_PER_SEC;
>> +	do_div(state->period, clk_get_rate(mchp_core_pwm->clk));
>> +}
>> +
>> +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(&pdev->dev, NULL);
>> +	if (IS_ERR(mchp_pwm->clk))
>> +		return PTR_ERR(mchp_pwm->clk);
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
>> +				 &mchp_pwm->sync_update_mask))
>> +		mchp_pwm->sync_update_mask = 0u;
>> +
>> +	ret = clk_prepare_enable(mchp_pwm->clk);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "failed to prepare PWM clock\n");
>> +
>> +	mchp_pwm->chip.dev = &pdev->dev;
>> +	mchp_pwm->chip.ops = &mchp_core_pwm_ops;
>> +	mchp_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
>> +	mchp_pwm->chip.of_pwm_n_cells = 3;
> 
> You can drop these two, assuming you have #pwm-cells = <3> in the dtb,
> they are also assigned by the core like that. (And if you don't it's
> ugly to assign these and you're better of the the stuff that the pwm
> core does.)

The dts actually says 2 at the moment so that needs fixing..

> 
>> +	mchp_pwm->chip.npwm = 16;
>> +
>> +	ret = pwmchip_add(&mchp_pwm->chip);
>> +	if (ret < 0) {
>> +		clk_disable_unprepare(mchp_pwm->clk);
>> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
>> +	}
>> +
>> +	platform_set_drvdata(pdev, mchp_pwm);
>> +	dev_info(&pdev->dev, "Successfully registered Microchip corePWM\n");
> 
> I recommend to drop this message. If every driver does that, you get
> quite a lot of messages that are not very helpful (once development of
> the driver is done) and delay the boot up time.

My CI likes them, but if you don't - I am happy to drop it.

> 
>> +	return 0;
>> +}
>> +
>> +static int mchp_core_pwm_remove(struct platform_device *pdev)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_pwm = platform_get_drvdata(pdev);
>> +
>> +	pwmchip_remove(&mchp_pwm->chip);
>> +	clk_disable_unprepare(mchp_pwm->clk);
> 
> If you use devm_clk_get_enabled() and devm_pwmchip_add() in .probe(),
> you can drop .remove()

SGTM

> 
>> +
>> +	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,
>> +	.remove = mchp_core_pwm_remove,
>> +};
>> +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");
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv