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

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

[Conor Dooley]

Hey all,
Small series here, adding a driver for the "soft" pwm IP core
for microchip FPGAs. The binding for them was already added
in 5.18.
Thanks,
Conor.

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

-- 
2.36.1


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

[PATCH 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 | 289 +++++++++++++++++++++++++++++++
 3 files changed, 300 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..2cc1de163bcc
--- /dev/null
+++ b/drivers/pwm/pwm-microchip-core.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * corePWM driver for Microchip FPGAs
+ *
+ * Copyright (c) 2021-2022 Microchip Corporation. All rights reserved.
+ * Author: Conor Dooley <conor.dooley@microchip.com>
+ *
+ */
+
+#include <linux/clk.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 PRESCALE_REG	0x00u
+#define PERIOD_REG	0x04u
+#define PWM_EN_LOW_REG	0x08u
+#define PWM_EN_HIGH_REG	0x0Cu
+#define SYNC_UPD_REG	0xE4u
+#define POSEDGE_OFFSET	0x10u
+#define NEGEDGE_OFFSET	0x14u
+#define CHANNEL_OFFSET	0x08u
+
+struct mchp_core_pwm_registers {
+	u8 posedge;
+	u8 negedge;
+	u8 period_steps;
+	u8 prescale;
+};
+
+struct mchp_core_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	void __iomem *base;
+	struct mchp_core_pwm_registers *regs;
+};
+
+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 = PWM_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);
+}
+
+static void mchp_core_pwm_calculate_duty(struct pwm_chip *chip,
+					 const struct pwm_state *desired_state,
+					 struct mchp_core_pwm_registers *regs)
+{
+	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
+	u64 clk_period = NSEC_PER_SEC;
+	u64 duty_steps;
+
+	/* Calculate the clk period and then the duty cycle edges */
+	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
+
+	duty_steps = desired_state->duty_cycle * PREG_TO_VAL(regs->period_steps);
+	do_div(duty_steps, (clk_period * PREG_TO_VAL(regs->period_steps)));
+	if (desired_state->polarity == PWM_POLARITY_INVERSED) {
+		regs->negedge = 0u;
+		regs->posedge = duty_steps;
+	} else {
+		regs->posedge = 0u;
+		regs->negedge = duty_steps;
+	}
+}
+
+static void mchp_core_pwm_apply_duty(const u8 channel,
+				     struct mchp_core_pwm_chip *pwm_chip,
+				     struct mchp_core_pwm_registers *regs)
+{
+	void __iomem *channel_base = pwm_chip->base + channel * CHANNEL_OFFSET;
+
+	writel_relaxed(regs->posedge, channel_base + POSEDGE_OFFSET);
+	writel_relaxed(regs->negedge, channel_base + NEGEDGE_OFFSET);
+}
+
+static void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *pwm_chip,
+				       struct mchp_core_pwm_registers *regs)
+{
+	writel_relaxed(regs->prescale, pwm_chip->base + PRESCALE_REG);
+	writel_relaxed(regs->period_steps, pwm_chip->base + PERIOD_REG);
+}
+
+static int mchp_core_pwm_calculate_base(struct pwm_chip *chip,
+					const struct pwm_state *desired_state,
+					u8 *period_steps_r, u8 *prescale_r)
+{
+	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
+	u64 tmp = desired_state->period;
+
+	/* Calculate the period cycles and prescale value */
+	tmp *= clk_get_rate(mchp_core_pwm->clk);
+	do_div(tmp, NSEC_PER_SEC);
+
+	if (tmp > 65535) {
+		dev_err(chip->dev,
+			"requested prescale exceeds the maximum possible\n");
+		return -EINVAL;
+	} else if (tmp <= 256) {
+		*prescale_r = 0;
+		*period_steps_r = tmp - 1;
+	} else {
+		*prescale_r = fls(tmp) - 8;
+		*period_steps_r = (tmp >> *prescale_r) - 1;
+	}
+
+	return 0;
+}
+
+static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			       const struct pwm_state *desired_state)
+{
+	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
+	struct pwm_state current_state;
+	u8 period_steps_r, prescale_r;
+	int ret;
+	u8 channel = pwm->hwpwm;
+
+	pwm_get_state(pwm, &current_state);
+
+	if (desired_state->enabled) {
+		if (current_state.enabled &&
+		    current_state.period == desired_state->period &&
+		    current_state.polarity == desired_state->polarity) {
+			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
+			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
+		} else {
+			ret = mchp_core_pwm_calculate_base(chip, desired_state, &period_steps_r,
+							   &prescale_r);
+			if (ret) {
+				dev_err(chip->dev, "failed to calculate base\n");
+				return ret;
+			}
+
+			mchp_core_pwm->regs->period_steps = period_steps_r;
+			mchp_core_pwm->regs->prescale = prescale_r;
+
+			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
+			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
+			mchp_core_pwm_apply_period(mchp_core_pwm, mchp_core_pwm->regs);
+		}
+
+		if (mchp_core_pwm->regs->posedge == mchp_core_pwm->regs->negedge)
+			mchp_core_pwm_enable(chip, pwm, false);
+		else
+			mchp_core_pwm_enable(chip, pwm, true);
+
+	} else if (!desired_state->enabled) {
+		mchp_core_pwm_enable(chip, pwm, false);
+	}
+
+	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 * CHANNEL_OFFSET;
+	u64 clk_period = NSEC_PER_SEC;
+	u8 prescale, period_steps, duty_steps;
+	u8 posedge, negedge;
+	u16 channel_enabled;
+
+	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + PWM_EN_HIGH_REG) << 8) |
+		readb_relaxed(mchp_core_pwm->base + PWM_EN_LOW_REG));
+
+	posedge = readb_relaxed(channel_base + POSEDGE_OFFSET);
+	negedge = readb_relaxed(channel_base + NEGEDGE_OFFSET);
+
+	duty_steps = abs((s8)posedge - (s8)negedge);
+	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+
+	prescale = readb_relaxed(mchp_core_pwm->base + PRESCALE_REG);
+	period_steps = readb_relaxed(mchp_core_pwm->base + PERIOD_REG);
+
+	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
+	state->duty_cycle = PREG_TO_VAL(prescale) * clk_period * duty_steps;
+	state->period = PREG_TO_VAL(prescale) * clk_period * PREG_TO_VAL(period_steps);
+
+	if (channel_enabled & 1 << pwm->hwpwm)
+		state->enabled = true;
+	else
+		state->enabled = false;
+}
+
+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->regs = devm_kzalloc(&pdev->dev, sizeof(*regs), GFP_KERNEL);
+	if (!mchp_pwm->regs)
+		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);
+
+	ret = clk_prepare(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.base = -1;
+	mchp_pwm->chip.npwm = 16;
+
+	ret = pwmchip_add(&mchp_pwm->chip);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+
+	writel_relaxed(0u, mchp_pwm->base + PWM_EN_LOW_REG);
+	writel_relaxed(0u, mchp_pwm->base + PWM_EN_HIGH_REG);
+
+	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)
+{
+	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

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

[Uwe Kleine-König]

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

Hello,

On Tue, Jun 07, 2022 at 09:45:51AM +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 | 289 +++++++++++++++++++++++++++++++
>  3 files changed, 300 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..2cc1de163bcc
> --- /dev/null
> +++ b/drivers/pwm/pwm-microchip-core.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * corePWM driver for Microchip FPGAs
> + *
> + * Copyright (c) 2021-2022 Microchip Corporation. All rights reserved.
> + * Author: Conor Dooley <conor.dooley@microchip.com>
> + *

Is there a public datasheet for that hardware? If yes, please add a link
here.

> + */
> +
> +#include <linux/clk.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 PRESCALE_REG	0x00u
> +#define PERIOD_REG	0x04u
> +#define PWM_EN_LOW_REG	0x08u
> +#define PWM_EN_HIGH_REG	0x0Cu
> +#define SYNC_UPD_REG	0xE4u
> +#define POSEDGE_OFFSET	0x10u
> +#define NEGEDGE_OFFSET	0x14u
> +#define CHANNEL_OFFSET	0x08u

Can you please prefix these register symbols with a driver prefix?
Unique register names are good for tools like ctags and then it's
obvious to the reader, that the defines are driver specific.

> +struct mchp_core_pwm_registers {
> +	u8 posedge;
> +	u8 negedge;
> +	u8 period_steps;
> +	u8 prescale;

these are the four registers for each channel, right? Can you add a
short overview how these registers define the resulting output wave.

> +};
> +
> +struct mchp_core_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *base;
> +	struct mchp_core_pwm_registers *regs;
> +};
> +
> +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 = PWM_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);
> +}
> +
> +static void mchp_core_pwm_calculate_duty(struct pwm_chip *chip,
> +					 const struct pwm_state *desired_state,
> +					 struct mchp_core_pwm_registers *regs)
> +{
> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> +	u64 clk_period = NSEC_PER_SEC;
> +	u64 duty_steps;
> +
> +	/* Calculate the clk period and then the duty cycle edges */
> +	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
> +
> +	duty_steps = desired_state->duty_cycle * PREG_TO_VAL(regs->period_steps);
> +	do_div(duty_steps, (clk_period * PREG_TO_VAL(regs->period_steps)));

Don't divide by a result of a division.

> +	if (desired_state->polarity == PWM_POLARITY_INVERSED) {
> +		regs->negedge = 0u;
> +		regs->posedge = duty_steps;
> +	} else {
> +		regs->posedge = 0u;
> +		regs->negedge = duty_steps;
> +	}
> +}
> +
> +static void mchp_core_pwm_apply_duty(const u8 channel,
> +				     struct mchp_core_pwm_chip *pwm_chip,
> +				     struct mchp_core_pwm_registers *regs)
> +{
> +	void __iomem *channel_base = pwm_chip->base + channel * CHANNEL_OFFSET;
> +
> +	writel_relaxed(regs->posedge, channel_base + POSEDGE_OFFSET);
> +	writel_relaxed(regs->negedge, channel_base + NEGEDGE_OFFSET);
> +}
> +
> +static void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *pwm_chip,
> +				       struct mchp_core_pwm_registers *regs)
> +{
> +	writel_relaxed(regs->prescale, pwm_chip->base + PRESCALE_REG);
> +	writel_relaxed(regs->period_steps, pwm_chip->base + PERIOD_REG);
> +}
> +
> +static int mchp_core_pwm_calculate_base(struct pwm_chip *chip,
> +					const struct pwm_state *desired_state,
> +					u8 *period_steps_r, u8 *prescale_r)
> +{
> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> +	u64 tmp = desired_state->period;
> +
> +	/* Calculate the period cycles and prescale value */
> +	tmp *= clk_get_rate(mchp_core_pwm->clk);
> +	do_div(tmp, NSEC_PER_SEC);
> +
> +	if (tmp > 65535) {

If a too long period is requested, please configure the biggest possible
period.

> +		dev_err(chip->dev,
> +			"requested prescale exceeds the maximum possible\n");

No error message in .apply() please.

> +		return -EINVAL;
> +	} else if (tmp <= 256) {
> +		*prescale_r = 0;
> +		*period_steps_r = tmp - 1;
> +	} else {
> +		*prescale_r = fls(tmp) - 8;
> +		*period_steps_r = (tmp >> *prescale_r) - 1;
> +	}

*prescale_r = fls(tmp >> 8);
*period_steps_r = (tmp >> *prescale_r) - 1;

can be used in both branches with the same result.

> +
> +	return 0;
> +}
> +
> +static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       const struct pwm_state *desired_state)

please s/desired_state/state/ for consistency with other drivers.

> +{
> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> +	struct pwm_state current_state;
> +	u8 period_steps_r, prescale_r;
> +	int ret;
> +	u8 channel = pwm->hwpwm;
> +
> +	pwm_get_state(pwm, &current_state);

Please don't call pwm API functions in a PWM driver. Simply use
pwm->state for the previously applied state.

> +	if (desired_state->enabled) {
> +		if (current_state.enabled &&
> +		    current_state.period == desired_state->period &&
> +		    current_state.polarity == desired_state->polarity) {

If everything is as before, why are you doing something at all?

> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
> +		} else {
> +			ret = mchp_core_pwm_calculate_base(chip, desired_state, &period_steps_r,
> +							   &prescale_r);
> +			if (ret) {
> +				dev_err(chip->dev, "failed to calculate base\n");

mchp_core_pwm_calculate_base might already emit an error message. Apply
shouldn't emit an error message at all.

> +				return ret;
> +			}
> +
> +			mchp_core_pwm->regs->period_steps = period_steps_r;
> +			mchp_core_pwm->regs->prescale = prescale_r;
> +
> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
> +			mchp_core_pwm_apply_period(mchp_core_pwm, mchp_core_pwm->regs);

Is there a race where e.g. the output is defined by the previous period
and the new duty_cycle?

> +		}
> +
> +		if (mchp_core_pwm->regs->posedge == mchp_core_pwm->regs->negedge)
> +			mchp_core_pwm_enable(chip, pwm, false);
> +		else
> +			mchp_core_pwm_enable(chip, pwm, true);

Here is a race: If the PWM is running and it configured to be disabled
with a different duty_cycle/period the duty_cycle/period might be
(shortly) visible on the output with is undesirable.

> +	} else if (!desired_state->enabled) {

This can be simplified to just "} else {"

> +		mchp_core_pwm_enable(chip, pwm, false);
> +	}
> +
> +	return 0;

I think this can be considerably simplified by unrolling the helper
functions.

> +}
> +
> +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 * CHANNEL_OFFSET;
> +	u64 clk_period = NSEC_PER_SEC;
> +	u8 prescale, period_steps, duty_steps;
> +	u8 posedge, negedge;
> +	u16 channel_enabled;
> +
> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + PWM_EN_HIGH_REG) << 8) |
> +		readb_relaxed(mchp_core_pwm->base + PWM_EN_LOW_REG));
> +
> +	posedge = readb_relaxed(channel_base + POSEDGE_OFFSET);
> +	negedge = readb_relaxed(channel_base + NEGEDGE_OFFSET);
> +
> +	duty_steps = abs((s8)posedge - (s8)negedge);
> +	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +
> +	prescale = readb_relaxed(mchp_core_pwm->base + PRESCALE_REG);
> +	period_steps = readb_relaxed(mchp_core_pwm->base + PERIOD_REG);
> +
> +	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
> +	state->duty_cycle = PREG_TO_VAL(prescale) * clk_period * duty_steps;

Dividing early results in rounding errors.

> +	state->period = PREG_TO_VAL(prescale) * clk_period * PREG_TO_VAL(period_steps);
> +
> +	if (channel_enabled & 1 << pwm->hwpwm)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;
> +}

Have you tested your driver with PWM_DEBUG enabled? The rounding is
wrong here. (The supposed rounding behaviour is that .apply rounds down
and to make .apply(.getstate()) a noop, .get_state() must round up.

If you do something like:

        echo 0 > duty_cycle

        for i in $(seq 10000 -1 1); do
                echo $i > period
        done

        for i in $(seq 1 10000); do
                echo $i > period
        done

        for i in $(seq 10000 -1 1); do
                echo $i > duty_cycle
        done

        for i in $(seq 1 10000); do
                echo $i > duty_cycle
        done

with PWM_DEBUG enabled, you should catch most rounding errors. (Maybe
adapt the boundaries according to your driver's capabilities.)

> +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->regs = devm_kzalloc(&pdev->dev, sizeof(*regs), GFP_KERNEL);
> +	if (!mchp_pwm->regs)
> +		return -ENOMEM;

If you make regs not a pointer but embed the structure directly in
struct mchp_core_pwm_chip, you only need a single allocation. Having
said that I wonder if you need to store this in driver data at all.

> +	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);
> +
> +	ret = clk_prepare(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.base = -1;

Please drop the assignment to base.

> +	mchp_pwm->chip.npwm = 16;
> +
> +	ret = pwmchip_add(&mchp_pwm->chip);
> +	if (ret < 0)

clk_unprepare missing here.

> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +
> +	writel_relaxed(0u, mchp_pwm->base + PWM_EN_LOW_REG);
> +	writel_relaxed(0u, mchp_pwm->base + PWM_EN_HIGH_REG);

What is the effect here? Don't modify the hardware after pwmchip_add().
When that function returns a consumer might already have configured the
hardware.

> +	platform_set_drvdata(pdev, mchp_pwm);

This is unused, please drop.

> +	dev_info(&pdev->dev, "Successfully registered Microchip corePWM\n");
> +
> +	return 0;
> +}
> +
> +static int mchp_core_pwm_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}

Here you're missing to undo pwmchip_add and clk_prepare.

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

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

[Conor.Dooley]

On 07/06/2022 21:07, Uwe Kleine-König wrote:

Mail client is acting weird with quoting, but thanks for
all the feedback Uwe.
Conor
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor.Dooley]

Hey Uwe,
Anything I've ignored here I will change for v2 - just have
a few questions about your feedback/responses to questions.

On 07/06/2022 21:07, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jun 07, 2022 at 09:45:51AM +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 | 289 +++++++++++++++++++++++++++++++
>>   3 files changed, 300 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..2cc1de163bcc
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-microchip-core.c
>> @@ -0,0 +1,289 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * corePWM driver for Microchip FPGAs
>> + *
>> + * Copyright (c) 2021-2022 Microchip Corporation. All rights reserved.
>> + * Author: Conor Dooley <conor.dooley@microchip.com>
>> + *
> 
> Is there a public datasheet for that hardware? If yes, please add a link
> here.

Not quite a datasheet since this is for an FPGA core not a "real" part.
I can link the following "handbook" for it though (direct download link):
https://www.microsemi.com/document-portal/doc_download/1245275-corepwm-hb

> 
>> + */
>> +
>> +#include <linux/clk.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 PRESCALE_REG	0x00u
>> +#define PERIOD_REG	0x04u
>> +#define PWM_EN_LOW_REG	0x08u
>> +#define PWM_EN_HIGH_REG	0x0Cu
>> +#define SYNC_UPD_REG	0xE4u
>> +#define POSEDGE_OFFSET	0x10u
>> +#define NEGEDGE_OFFSET	0x14u
>> +#define CHANNEL_OFFSET	0x08u
> 
> Can you please prefix these register symbols with a driver prefix?
> Unique register names are good for tools like ctags and then it's
> obvious to the reader, that the defines are driver specific.
> 
>> +struct mchp_core_pwm_registers {
>> +	u8 posedge;
>> +	u8 negedge;
>> +	u8 period_steps;
>> +	u8 prescale;
> 
> these are the four registers for each channel, right? Can you add a
> short overview how these registers define the resulting output wave.

Ehh, only the edges are per channel. Does that change anything about
your feedback?
I'll add an explanation for each, sure.

> 
>> +};
>> +
>> +struct mchp_core_pwm_chip {
>> +	struct pwm_chip chip;
>> +	struct clk *clk;
>> +	void __iomem *base;
>> +	struct mchp_core_pwm_registers *regs;
>> +};
>> +
>> +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 = PWM_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);
>> +}
>> +
>> +static void mchp_core_pwm_calculate_duty(struct pwm_chip *chip,
>> +					 const struct pwm_state *desired_state,
>> +					 struct mchp_core_pwm_registers *regs)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	u64 clk_period = NSEC_PER_SEC;
>> +	u64 duty_steps;
>> +
>> +	/* Calculate the clk period and then the duty cycle edges */
>> +	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
>> +
>> +	duty_steps = desired_state->duty_cycle * PREG_TO_VAL(regs->period_steps);
>> +	do_div(duty_steps, (clk_period * PREG_TO_VAL(regs->period_steps)));
> 
> Don't divide by a result of a division.
> 
>> +	if (desired_state->polarity == PWM_POLARITY_INVERSED) {
>> +		regs->negedge = 0u;
>> +		regs->posedge = duty_steps;
>> +	} else {
>> +		regs->posedge = 0u;
>> +		regs->negedge = duty_steps;
>> +	}
>> +}
>> +
>> +static void mchp_core_pwm_apply_duty(const u8 channel,
>> +				     struct mchp_core_pwm_chip *pwm_chip,
>> +				     struct mchp_core_pwm_registers *regs)
>> +{
>> +	void __iomem *channel_base = pwm_chip->base + channel * CHANNEL_OFFSET;
>> +
>> +	writel_relaxed(regs->posedge, channel_base + POSEDGE_OFFSET);
>> +	writel_relaxed(regs->negedge, channel_base + NEGEDGE_OFFSET);
>> +}
>> +
>> +static void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *pwm_chip,
>> +				       struct mchp_core_pwm_registers *regs)
>> +{
>> +	writel_relaxed(regs->prescale, pwm_chip->base + PRESCALE_REG);
>> +	writel_relaxed(regs->period_steps, pwm_chip->base + PERIOD_REG);
>> +}
>> +
>> +static int mchp_core_pwm_calculate_base(struct pwm_chip *chip,
>> +					const struct pwm_state *desired_state,
>> +					u8 *period_steps_r, u8 *prescale_r)
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	u64 tmp = desired_state->period;
>> +
>> +	/* Calculate the period cycles and prescale value */
>> +	tmp *= clk_get_rate(mchp_core_pwm->clk);
>> +	do_div(tmp, NSEC_PER_SEC);
>> +
>> +	if (tmp > 65535) {
> 
> If a too long period is requested, please configure the biggest possible
> period.
> 
>> +		dev_err(chip->dev,
>> +			"requested prescale exceeds the maximum possible\n");
> 
> No error message in .apply() please.

No /error/ or no error /message/?
As in, can I make it a dev_warn or do you want it removed entirely
and replaced by capping at the max value?

> 
>> +		return -EINVAL;
>> +	} else if (tmp <= 256) {
>> +		*prescale_r = 0;
>> +		*period_steps_r = tmp - 1;
>> +	} else {
>> +		*prescale_r = fls(tmp) - 8;
>> +		*period_steps_r = (tmp >> *prescale_r) - 1;
>> +	}
> 
> *prescale_r = fls(tmp >> 8);
> *period_steps_r = (tmp >> *prescale_r) - 1;
> 
> can be used in both branches with the same result.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			       const struct pwm_state *desired_state)
> 
> please s/desired_state/state/ for consistency with other drivers.
> 
>> +{
>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> +	struct pwm_state current_state;
>> +	u8 period_steps_r, prescale_r;
>> +	int ret;
>> +	u8 channel = pwm->hwpwm;
>> +
>> +	pwm_get_state(pwm, &current_state);
> 
> Please don't call pwm API functions in a PWM driver. Simply use
> pwm->state for the previously applied state.
> 
>> +	if (desired_state->enabled) {
>> +		if (current_state.enabled &&
>> +		    current_state.period == desired_state->period &&
>> +		    current_state.polarity == desired_state->polarity) {
> 
> If everything is as before, why are you doing something at all?

This is a change in duty without any other change.
Could just remove this & recalculate everything when apply is called
to simply the logic?

> 
>> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
>> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
>> +		} else {
>> +			ret = mchp_core_pwm_calculate_base(chip, desired_state, &period_steps_r,
>> +							   &prescale_r);
>> +			if (ret) {
>> +				dev_err(chip->dev, "failed to calculate base\n");
> 
> mchp_core_pwm_calculate_base might already emit an error message. Apply
> shouldn't emit an error message at all.
> 
>> +				return ret;
>> +			}
>> +
>> +			mchp_core_pwm->regs->period_steps = period_steps_r;
>> +			mchp_core_pwm->regs->prescale = prescale_r;
>> +
>> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
>> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
>> +			mchp_core_pwm_apply_period(mchp_core_pwm, mchp_core_pwm->regs);
> 
> Is there a race where e.g. the output is defined by the previous period
> and the new duty_cycle?

"Yes". It depends on how the IP block is configured. I'll add a write to
the sync register (which is a NOP if not configured for sync mode).
> 
>> +		}
>> +
>> +		if (mchp_core_pwm->regs->posedge == mchp_core_pwm->regs->negedge)
>> +			mchp_core_pwm_enable(chip, pwm, false);
>> +		else
>> +			mchp_core_pwm_enable(chip, pwm, true);
> 
> Here is a race: If the PWM is running and it configured to be disabled
> with a different duty_cycle/period the duty_cycle/period might be
> (shortly) visible on the output with is undesirable.

This is trying to work around some nasty behaviour in the IP block.
If negedge == posedge, it gives you a 50% duty cycle at twice the
period you asked for.
ie since the negedge and posedge are at the same time, it does
whichever edge is actually possible each time that period step is
reached.

If the state requested is disabled, it should be caught by the if()
prior to entering this & exit early & avoid this entirely.

I'll put the sync reg write after this, so if the block is configured
to support sync updates, the output waveform won't do anything odd.

> 
>> +	} else if (!desired_state->enabled) {
> 
> This can be simplified to just "} else {"
> 
>> +		mchp_core_pwm_enable(chip, pwm, false);
>> +	}
>> +
>> +	return 0;
> 
> I think this can be considerably simplified by unrolling the helper
> functions.
> 
>> +}
>> +
>> +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 * CHANNEL_OFFSET;
>> +	u64 clk_period = NSEC_PER_SEC;
>> +	u8 prescale, period_steps, duty_steps;
>> +	u8 posedge, negedge;
>> +	u16 channel_enabled;
>> +
>> +	channel_enabled = (((u16)readb_relaxed(mchp_core_pwm->base + PWM_EN_HIGH_REG) << 8) |
>> +		readb_relaxed(mchp_core_pwm->base + PWM_EN_LOW_REG));
>> +
>> +	posedge = readb_relaxed(channel_base + POSEDGE_OFFSET);
>> +	negedge = readb_relaxed(channel_base + NEGEDGE_OFFSET);
>> +
>> +	duty_steps = abs((s8)posedge - (s8)negedge);
>> +	state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
>> +
>> +	prescale = readb_relaxed(mchp_core_pwm->base + PRESCALE_REG);
>> +	period_steps = readb_relaxed(mchp_core_pwm->base + PERIOD_REG);
>> +
>> +	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
>> +	state->duty_cycle = PREG_TO_VAL(prescale) * clk_period * duty_steps;
> 
> Dividing early results in rounding errors.
> 
>> +	state->period = PREG_TO_VAL(prescale) * clk_period * PREG_TO_VAL(period_steps);
>> +
>> +	if (channel_enabled & 1 << pwm->hwpwm)
>> +		state->enabled = true;
>> +	else
>> +		state->enabled = false;
>> +}
> 
> Have you tested your driver with PWM_DEBUG enabled? The rounding is
> wrong here. (The supposed rounding behaviour is that .apply rounds down
> and to make .apply(.getstate()) a noop, .get_state() must round up.

I have, but not exhaustively.

> 
> If you do something like:
> 
>          echo 0 > duty_cycle
> 
>          for i in $(seq 10000 -1 1); do
>                  echo $i > period
>          done
> 
>          for i in $(seq 1 10000); do
>                  echo $i > period
>          done
> 
>          for i in $(seq 10000 -1 1); do
>                  echo $i > duty_cycle
>          done
> 
>          for i in $(seq 1 10000); do
>                  echo $i > duty_cycle
>          done
> 
> with PWM_DEBUG enabled, you should catch most rounding errors. (Maybe
> adapt the boundaries according to your driver's capabilities.)

Cool, willdo thanks. Not sure why I didn't run this in a loop.
:facepalm:

Again, thanks for all the feedback Uwe.
Conor.

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

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

[Uwe Kleine-König]

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

Hello Conor,

On Wed, Jun 08, 2022 at 12:12:37PM +0000, Conor.Dooley@microchip.com wrote:
> On 07/06/2022 21:07, Uwe Kleine-König wrote:
> > On Tue, Jun 07, 2022 at 09:45:51AM +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 | 289 +++++++++++++++++++++++++++++++
> >>   3 files changed, 300 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..2cc1de163bcc
> >> --- /dev/null
> >> +++ b/drivers/pwm/pwm-microchip-core.c
> >> @@ -0,0 +1,289 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * corePWM driver for Microchip FPGAs
> >> + *
> >> + * Copyright (c) 2021-2022 Microchip Corporation. All rights reserved.
> >> + * Author: Conor Dooley <conor.dooley@microchip.com>
> >> + *
> > 
> > Is there a public datasheet for that hardware? If yes, please add a link
> > here.
> 
> Not quite a datasheet since this is for an FPGA core not a "real" part.
> I can link the following "handbook" for it though (direct download link):
> https://www.microsemi.com/document-portal/doc_download/1245275-corepwm-hb

Anything that might be worth reading if a question about the driver's
behaviour pops up is helpful. But ideally the comments about the
registers are good enough that it's not needed. Still having a document
around is good.
 
> >> + */
> >> +
> >> +#include <linux/clk.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 PRESCALE_REG	0x00u
> >> +#define PERIOD_REG	0x04u
> >> +#define PWM_EN_LOW_REG	0x08u
> >> +#define PWM_EN_HIGH_REG	0x0Cu
> >> +#define SYNC_UPD_REG	0xE4u
> >> +#define POSEDGE_OFFSET	0x10u
> >> +#define NEGEDGE_OFFSET	0x14u
> >> +#define CHANNEL_OFFSET	0x08u
> > 
> > Can you please prefix these register symbols with a driver prefix?
> > Unique register names are good for tools like ctags and then it's
> > obvious to the reader, that the defines are driver specific.
> > 
> >> +struct mchp_core_pwm_registers {
> >> +	u8 posedge;
> >> +	u8 negedge;
> >> +	u8 period_steps;
> >> +	u8 prescale;
> > 
> > these are the four registers for each channel, right? Can you add a
> > short overview how these registers define the resulting output wave.
> 
> Ehh, only the edges are per channel. Does that change anything about
> your feedback?
> I'll add an explanation for each, sure.

So the channels share the same period? If so you'll have to keep track
of which PWM channels are enabled and only change the period if no other
running channel is affected.

> >> +};
> >> +
> >> +struct mchp_core_pwm_chip {
> >> +	struct pwm_chip chip;
> >> +	struct clk *clk;
> >> +	void __iomem *base;
> >> +	struct mchp_core_pwm_registers *regs;
> >> +};
> >> +
> >> +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 = PWM_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);
> >> +}
> >> +
> >> +static void mchp_core_pwm_calculate_duty(struct pwm_chip *chip,
> >> +					 const struct pwm_state *desired_state,
> >> +					 struct mchp_core_pwm_registers *regs)
> >> +{
> >> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> >> +	u64 clk_period = NSEC_PER_SEC;
> >> +	u64 duty_steps;
> >> +
> >> +	/* Calculate the clk period and then the duty cycle edges */
> >> +	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
> >> +
> >> +	duty_steps = desired_state->duty_cycle * PREG_TO_VAL(regs->period_steps);
> >> +	do_div(duty_steps, (clk_period * PREG_TO_VAL(regs->period_steps)));
> > 
> > Don't divide by a result of a division.
> > 
> >> +	if (desired_state->polarity == PWM_POLARITY_INVERSED) {
> >> +		regs->negedge = 0u;
> >> +		regs->posedge = duty_steps;
> >> +	} else {
> >> +		regs->posedge = 0u;
> >> +		regs->negedge = duty_steps;
> >> +	}
> >> +}
> >> +
> >> +static void mchp_core_pwm_apply_duty(const u8 channel,
> >> +				     struct mchp_core_pwm_chip *pwm_chip,
> >> +				     struct mchp_core_pwm_registers *regs)
> >> +{
> >> +	void __iomem *channel_base = pwm_chip->base + channel * CHANNEL_OFFSET;
> >> +
> >> +	writel_relaxed(regs->posedge, channel_base + POSEDGE_OFFSET);
> >> +	writel_relaxed(regs->negedge, channel_base + NEGEDGE_OFFSET);
> >> +}
> >> +
> >> +static void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *pwm_chip,
> >> +				       struct mchp_core_pwm_registers *regs)
> >> +{
> >> +	writel_relaxed(regs->prescale, pwm_chip->base + PRESCALE_REG);
> >> +	writel_relaxed(regs->period_steps, pwm_chip->base + PERIOD_REG);
> >> +}
> >> +
> >> +static int mchp_core_pwm_calculate_base(struct pwm_chip *chip,
> >> +					const struct pwm_state *desired_state,
> >> +					u8 *period_steps_r, u8 *prescale_r)
> >> +{
> >> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> >> +	u64 tmp = desired_state->period;
> >> +
> >> +	/* Calculate the period cycles and prescale value */
> >> +	tmp *= clk_get_rate(mchp_core_pwm->clk);
> >> +	do_div(tmp, NSEC_PER_SEC);
> >> +
> >> +	if (tmp > 65535) {
> > 
> > If a too long period is requested, please configure the biggest possible
> > period.
> > 
> >> +		dev_err(chip->dev,
> >> +			"requested prescale exceeds the maximum possible\n");
> > 
> > No error message in .apply() please.
> 
> No /error/ or no error /message/?

No error message. Otherwise a userspace application might easily trash
the kernel log.

> As in, can I make it a dev_warn or do you want it removed entirely
> and replaced by capping at the max value?

Yes, just cap to the max value. So the rule is always to pick the
biggest possible period not bigger than the requested period. And for
that one pick the biggest duty_cycle not bigger than the requested
duty_cycle.

> >> +	if (desired_state->enabled) {
> >> +		if (current_state.enabled &&
> >> +		    current_state.period == desired_state->period &&
> >> +		    current_state.polarity == desired_state->polarity) {
> > 
> > If everything is as before, why are you doing something at all?
> 
> This is a change in duty without any other change.
> Could just remove this & recalculate everything when apply is called
> to simply the logic?

Ah, right. A comment (e.g. "only duty cycle changed") would be good for
such stupid readers like me :-)

I don't feel strong here. For many cases the period (and polarity) is
kept constant and only duty_cycle changes. So optimizing for that case
looks ok.

> >> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
> >> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
> >> +		} else {
> >> +			ret = mchp_core_pwm_calculate_base(chip, desired_state, &period_steps_r,
> >> +							   &prescale_r);
> >> +			if (ret) {
> >> +				dev_err(chip->dev, "failed to calculate base\n");
> > 
> > mchp_core_pwm_calculate_base might already emit an error message. Apply
> > shouldn't emit an error message at all.
> > 
> >> +				return ret;
> >> +			}
> >> +
> >> +			mchp_core_pwm->regs->period_steps = period_steps_r;
> >> +			mchp_core_pwm->regs->prescale = prescale_r;
> >> +
> >> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
> >> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
> >> +			mchp_core_pwm_apply_period(mchp_core_pwm, mchp_core_pwm->regs);
> > 
> > Is there a race where e.g. the output is defined by the previous period
> > and the new duty_cycle?
> 
> "Yes". It depends on how the IP block is configured. I'll add a write to
> the sync register (which is a NOP if not configured for sync mode).

Several drivers have a "Limitations" section at the top of the driver.
Something like that would be good to document there. Please stick to the
format found in e.g. pwm-sl28cpld.c, that is: "Limitations:" (even if
it's more about "Hardware Details") and then a list of items without
empty line in between for easy greppability.

> > 
> >> +		}
> >> +
> >> +		if (mchp_core_pwm->regs->posedge == mchp_core_pwm->regs->negedge)
> >> +			mchp_core_pwm_enable(chip, pwm, false);
> >> +		else
> >> +			mchp_core_pwm_enable(chip, pwm, true);
> > 
> > Here is a race: If the PWM is running and it configured to be disabled
> > with a different duty_cycle/period the duty_cycle/period might be
> > (shortly) visible on the output with is undesirable.
> 
> This is trying to work around some nasty behaviour in the IP block.
> If negedge == posedge, it gives you a 50% duty cycle at twice the
> period you asked for.
> ie since the negedge and posedge are at the same time, it does
> whichever edge is actually possible each time that period step is
> reached.

I didn't understand the normal behaviour of these registers yet, so
cannot comment. Usually it's a good idea to document corner cases in
comments.

> If the state requested is disabled, it should be caught by the if()
> prior to entering this & exit early & avoid this entirely.
> 
> I'll put the sync reg write after this, so if the block is configured
> to support sync updates, the output waveform won't do anything odd.

Sounds good.

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

[Conor.Dooley]

Hey Uwe, one last one for ya..

On 08/06/2022 16:13, Uwe Kleine-König wrote:
> Hello Conor,
> 
> On Wed, Jun 08, 2022 at 12:12:37PM +0000, Conor.Dooley@microchip.com wrote:
>> On 07/06/2022 21:07, Uwe Kleine-König wrote:
>>> On Tue, Jun 07, 2022 at 09:45:51AM +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>
>>>> ---
---8<---
>>>> +struct mchp_core_pwm_registers {
>>>> +	u8 posedge;
>>>> +	u8 negedge;
>>>> +	u8 period_steps;
>>>> +	u8 prescale;
>>>
>>> these are the four registers for each channel, right? Can you add a
>>> short overview how these registers define the resulting output wave.
>>
>> Ehh, only the edges are per channel. Does that change anything about
>> your feedback?
>> I'll add an explanation for each, sure.
> 
> So the channels share the same period? If so you'll have to keep track
> of which PWM channels are enabled and only change the period if no other
> running channel is affected.

When I am capping the period (or not allowing it to be changed in this case
here) should I correct the duty cycle so that the the ratio is preserved?
Thanks,
Conor.

> 
>>>> +};
>>>> +
>>>> +struct mchp_core_pwm_chip {
>>>> +	struct pwm_chip chip;
>>>> +	struct clk *clk;
>>>> +	void __iomem *base;
>>>> +	struct mchp_core_pwm_registers *regs;
>>>> +};
>>>> +
>>>> +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 = PWM_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);
>>>> +}
>>>> +
>>>> +static void mchp_core_pwm_calculate_duty(struct pwm_chip *chip,
>>>> +					 const struct pwm_state *desired_state,
>>>> +					 struct mchp_core_pwm_registers *regs)
>>>> +{
>>>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>>>> +	u64 clk_period = NSEC_PER_SEC;
>>>> +	u64 duty_steps;
>>>> +
>>>> +	/* Calculate the clk period and then the duty cycle edges */
>>>> +	do_div(clk_period, clk_get_rate(mchp_core_pwm->clk));
>>>> +
>>>> +	duty_steps = desired_state->duty_cycle * PREG_TO_VAL(regs->period_steps);
>>>> +	do_div(duty_steps, (clk_period * PREG_TO_VAL(regs->period_steps)));
>>>
>>> Don't divide by a result of a division.
>>>
>>>> +	if (desired_state->polarity == PWM_POLARITY_INVERSED) {
>>>> +		regs->negedge = 0u;
>>>> +		regs->posedge = duty_steps;
>>>> +	} else {
>>>> +		regs->posedge = 0u;
>>>> +		regs->negedge = duty_steps;
>>>> +	}
>>>> +}
>>>> +
>>>> +static void mchp_core_pwm_apply_duty(const u8 channel,
>>>> +				     struct mchp_core_pwm_chip *pwm_chip,
>>>> +				     struct mchp_core_pwm_registers *regs)
>>>> +{
>>>> +	void __iomem *channel_base = pwm_chip->base + channel * CHANNEL_OFFSET;
>>>> +
>>>> +	writel_relaxed(regs->posedge, channel_base + POSEDGE_OFFSET);
>>>> +	writel_relaxed(regs->negedge, channel_base + NEGEDGE_OFFSET);
>>>> +}
>>>> +
>>>> +static void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *pwm_chip,
>>>> +				       struct mchp_core_pwm_registers *regs)
>>>> +{
>>>> +	writel_relaxed(regs->prescale, pwm_chip->base + PRESCALE_REG);
>>>> +	writel_relaxed(regs->period_steps, pwm_chip->base + PERIOD_REG);
>>>> +}
>>>> +
>>>> +static int mchp_core_pwm_calculate_base(struct pwm_chip *chip,
>>>> +					const struct pwm_state *desired_state,
>>>> +					u8 *period_steps_r, u8 *prescale_r)
>>>> +{
>>>> +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>>>> +	u64 tmp = desired_state->period;
>>>> +
>>>> +	/* Calculate the period cycles and prescale value */
>>>> +	tmp *= clk_get_rate(mchp_core_pwm->clk);
>>>> +	do_div(tmp, NSEC_PER_SEC);
>>>> +
>>>> +	if (tmp > 65535) {
>>>
>>> If a too long period is requested, please configure the biggest possible
>>> period.
>>>
>>>> +		dev_err(chip->dev,
>>>> +			"requested prescale exceeds the maximum possible\n");
>>>
>>> No error message in .apply() please.
>>
>> No /error/ or no error /message/?
> 
> No error message. Otherwise a userspace application might easily trash
> the kernel log.
> 
>> As in, can I make it a dev_warn or do you want it removed entirely
>> and replaced by capping at the max value?
> 
> Yes, just cap to the max value. So the rule is always to pick the
> biggest possible period not bigger than the requested period. And for
> that one pick the biggest duty_cycle not bigger than the requested
> duty_cycle.
> 
>>>> +	if (desired_state->enabled) {
>>>> +		if (current_state.enabled &&
>>>> +		    current_state.period == desired_state->period &&
>>>> +		    current_state.polarity == desired_state->polarity) {
>>>
>>> If everything is as before, why are you doing something at all?
>>
>> This is a change in duty without any other change.
>> Could just remove this & recalculate everything when apply is called
>> to simply the logic?
> 
> Ah, right. A comment (e.g. "only duty cycle changed") would be good for
> such stupid readers like me :-)
> 
> I don't feel strong here. For many cases the period (and polarity) is
> kept constant and only duty_cycle changes. So optimizing for that case
> looks ok.
> 
>>>> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
>>>> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
>>>> +		} else {
>>>> +			ret = mchp_core_pwm_calculate_base(chip, desired_state, &period_steps_r,
>>>> +							   &prescale_r);
>>>> +			if (ret) {
>>>> +				dev_err(chip->dev, "failed to calculate base\n");
>>>
>>> mchp_core_pwm_calculate_base might already emit an error message. Apply
>>> shouldn't emit an error message at all.
>>>
>>>> +				return ret;
>>>> +			}
>>>> +
>>>> +			mchp_core_pwm->regs->period_steps = period_steps_r;
>>>> +			mchp_core_pwm->regs->prescale = prescale_r;
>>>> +
>>>> +			mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs);
>>>> +			mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs);
>>>> +			mchp_core_pwm_apply_period(mchp_core_pwm, mchp_core_pwm->regs);
>>>
>>> Is there a race where e.g. the output is defined by the previous period
>>> and the new duty_cycle?
>>
>> "Yes". It depends on how the IP block is configured. I'll add a write to
>> the sync register (which is a NOP if not configured for sync mode).
> 
> Several drivers have a "Limitations" section at the top of the driver.
> Something like that would be good to document there. Please stick to the
> format found in e.g. pwm-sl28cpld.c, that is: "Limitations:" (even if
> it's more about "Hardware Details") and then a list of items without
> empty line in between for easy greppability.
> 
>>>
>>>> +		}
>>>> +
>>>> +		if (mchp_core_pwm->regs->posedge == mchp_core_pwm->regs->negedge)
>>>> +			mchp_core_pwm_enable(chip, pwm, false);
>>>> +		else
>>>> +			mchp_core_pwm_enable(chip, pwm, true);
>>>
>>> Here is a race: If the PWM is running and it configured to be disabled
>>> with a different duty_cycle/period the duty_cycle/period might be
>>> (shortly) visible on the output with is undesirable.
>>
>> This is trying to work around some nasty behaviour in the IP block.
>> If negedge == posedge, it gives you a 50% duty cycle at twice the
>> period you asked for.
>> ie since the negedge and posedge are at the same time, it does
>> whichever edge is actually possible each time that period step is
>> reached.
> 
> I didn't understand the normal behaviour of these registers yet, so
> cannot comment. Usually it's a good idea to document corner cases in
> comments.
> 
>> If the state requested is disabled, it should be caught by the if()
>> prior to entering this & exit early & avoid this entirely.
>>
>> I'll put the sync reg write after this, so if the block is configured
>> to support sync updates, the output waveform won't do anything odd.
> 
> Sounds good.
> 
> Best regards
> Uwe
> 
> 
> _______________________________________________
> 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

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

[Uwe Kleine-König]

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

On Sun, Jun 12, 2022 at 01:00:53PM +0000, Conor.Dooley@microchip.com wrote:
> Hey Uwe, one last one for ya..
> 
> On 08/06/2022 16:13, Uwe Kleine-König wrote:
> > Hello Conor,
> > 
> > On Wed, Jun 08, 2022 at 12:12:37PM +0000, Conor.Dooley@microchip.com wrote:
> >> On 07/06/2022 21:07, Uwe Kleine-König wrote:
> >>> On Tue, Jun 07, 2022 at 09:45:51AM +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>
> >>>> ---
> ---8<---
> >>>> +struct mchp_core_pwm_registers {
> >>>> +	u8 posedge;
> >>>> +	u8 negedge;
> >>>> +	u8 period_steps;
> >>>> +	u8 prescale;
> >>>
> >>> these are the four registers for each channel, right? Can you add a
> >>> short overview how these registers define the resulting output wave.
> >>
> >> Ehh, only the edges are per channel. Does that change anything about
> >> your feedback?
> >> I'll add an explanation for each, sure.
> > 
> > So the channels share the same period? If so you'll have to keep track
> > of which PWM channels are enabled and only change the period if no other
> > running channel is affected.
> 
> When I am capping the period (or not allowing it to be changed in this case
> here) should I correct the duty cycle so that the the ratio is preserved?

No, the thing to do is: Pick the biggest possible period not bigger
than the requested period. For that period pick the biggest possible
duty_cycle not bigger than the requested duty_cycle.

The focus here is to do something somewhat sensible and simple.

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

[Conor.Dooley]

On 12/06/2022 22:16, Uwe Kleine-König wrote:
> On Sun, Jun 12, 2022 at 01:00:53PM +0000, Conor.Dooley@microchip.com wrote:
>> Hey Uwe, one last one for ya..
>>
>> On 08/06/2022 16:13, Uwe Kleine-König wrote:
>>> Hello Conor,
>>>
>>> On Wed, Jun 08, 2022 at 12:12:37PM +0000, Conor.Dooley@microchip.com wrote:
>>>> On 07/06/2022 21:07, Uwe Kleine-König wrote:
>>>>> On Tue, Jun 07, 2022 at 09:45:51AM +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>
>>>>>> ---
>> ---8<---
>>>>>> +struct mchp_core_pwm_registers {
>>>>>> +	u8 posedge;
>>>>>> +	u8 negedge;
>>>>>> +	u8 period_steps;
>>>>>> +	u8 prescale;
>>>>>
>>>>> these are the four registers for each channel, right? Can you add a
>>>>> short overview how these registers define the resulting output wave.
>>>>
>>>> Ehh, only the edges are per channel. Does that change anything about
>>>> your feedback?
>>>> I'll add an explanation for each, sure.
>>>
>>> So the channels share the same period? If so you'll have to keep track
>>> of which PWM channels are enabled and only change the period if no other
>>> running channel is affected.
>>
>> When I am capping the period (or not allowing it to be changed in this case
>> here) should I correct the duty cycle so that the the ratio is preserved?
> 
> No, the thing to do is: Pick the biggest possible period not bigger
> than the requested period. For that period pick the biggest possible
> duty_cycle not bigger than the requested duty_cycle.
> 
> The focus here is to do something somewhat sensible and simple.

Cool, thanks!
Conor.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor.Dooley]

On 07/06/2022 09:45, Conor Dooley wrote:
> Hey all,
> Small series here, adding a driver for the "soft" pwm IP core
> for microchip FPGAs. The binding for them was already added
> in 5.18.
> Thanks,
> Conor.

I realised that I forgot to mention this is one of
three patchsets I have sent this cycle that touches
this MAINTAINERS entry. The others are to the USB and
SPI trees. The SPI one is already applied.
I will include this in the covers for future revisions
Thanks,
Conor

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

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