| Message ID | 20220607084551.2735922-2-conor.dooley@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Microchip's pwm fpga core | expand |
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, ¤t_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, ®s); > + 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
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
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, ¤t_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.
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
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
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
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.
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, ¤t_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, ®s); + 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");
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