| Message ID | 20230306094858.1614819-2-conor.dooley@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Microchip Soft IP corePWM driver | expand |
Hello, On Mon, Mar 06, 2023 at 09:48:58AM +0000, 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 | 441 +++++++++++++++++++++++++++++++ > 3 files changed, 452 insertions(+) > create mode 100644 drivers/pwm/pwm-microchip-core.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index dae023d783a22..f42756a014ed4 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -393,6 +393,16 @@ config PWM_MEDIATEK > To compile this driver as a module, choose M here: the module > will be called pwm-mediatek. > > +config PWM_MICROCHIP_CORE > + tristate "Microchip corePWM PWM support" > + depends on SOC_MICROCHIP_POLARFIRE || COMPILE_TEST > + depends on HAS_IOMEM && OF > + help > + PWM driver for Microchip FPGA soft IP core. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-microchip-core. > + > config PWM_MXS > tristate "Freescale MXS PWM support" > depends on ARCH_MXS || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 7bf1a29f02b84..a65625359ece4 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -34,6 +34,7 @@ obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o > obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o > obj-$(CONFIG_PWM_MESON) += pwm-meson.o > obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o > +obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o > obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o > obj-$(CONFIG_PWM_MXS) += pwm-mxs.o > obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o > diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c > new file mode 100644 > index 0000000000000..a2a2e28f39031 > --- /dev/null > +++ b/drivers/pwm/pwm-microchip-core.c > @@ -0,0 +1,441 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * corePWM driver for Microchip "soft" FPGA IP cores. > + * > + * Copyright (c) 2021-2023 Microchip Corporation. All rights reserved. > + * Author: Conor Dooley <conor.dooley@microchip.com> > + * Documentation: > + * https://www.microsemi.com/document-portal/doc_download/1245275-corepwm-hb > + * > + * Limitations: > + * - If the IP block is configured without "shadow registers", all register > + * writes will take effect immediately, causing glitches on the output. > + * If shadow registers *are* enabled, a write to the "SYNC_UPDATE" register > + * notifies the core that it needs to update the registers defining the > + * waveform from the contents of the "shadow registers". > + * - The IP block has no concept of a duty cycle, only rising/falling edges of > + * the waveform. Unfortunately, if the rising & falling edges registers have > + * the same value written to them the IP block will do whichever of a rising > + * or a falling edge is possible. I.E. a 50% waveform at twice the requested > + * period. Therefore to get a 0% waveform, the output is set the max high/low > + * time depending on polarity. > + * - The PWM period is set for the whole IP block not per channel. The driver > + * will only change the period if no other PWM output is enabled. > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/ktime.h> > +#include <linux/math.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > + > +#define PREG_TO_VAL(PREG) ((PREG) + 1) > + > +#define MCHPCOREPWM_PRESCALE_MAX 0x100 > +#define MCHPCOREPWM_PERIOD_STEPS_MAX 0xff > +#define MCHPCOREPWM_PERIOD_MAX 0xff00 > + > +#define MCHPCOREPWM_PRESCALE 0x00 > +#define MCHPCOREPWM_PERIOD 0x04 > +#define MCHPCOREPWM_EN(i) (0x08 + 0x04 * (i)) /* 0x08, 0x0c */ > +#define MCHPCOREPWM_POSEDGE(i) (0x10 + 0x08 * (i)) /* 0x10, 0x18, ..., 0x88 */ > +#define MCHPCOREPWM_NEGEDGE(i) (0x14 + 0x08 * (i)) /* 0x14, 0x1c, ..., 0x8c */ > +#define MCHPCOREPWM_SYNC_UPD 0xe4 > +#define MCHPCOREPWM_TIMEOUT_MS 100u > + > +struct mchp_core_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + void __iomem *base; > + struct mutex lock; /* protects the shared period */ > + ktime_t update_timestamp; > + u32 sync_update_mask; > + u16 channel_enabled; > +}; > + > +static inline struct mchp_core_pwm_chip *to_mchp_core_pwm(struct pwm_chip *chip) > +{ > + return container_of(chip, struct mchp_core_pwm_chip, chip); > +} > + > +static void mchp_core_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm, > + bool enable, u64 period) > +{ > + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); > + u8 channel_enable, reg_offset, shift; > + > + /* > + * There are two adjacent 8 bit control regs, the lower reg controls > + * 0-7 and the upper reg 8-15. Check if the pwm is in the upper reg > + * and if so, offset by the bus width. > + */ > + reg_offset = MCHPCOREPWM_EN(pwm->hwpwm >> 3); > + shift = pwm->hwpwm & 7; > + > + channel_enable = readb_relaxed(mchp_core_pwm->base + reg_offset); > + channel_enable &= ~(1 << shift); > + channel_enable |= (enable << shift); > + > + writel_relaxed(channel_enable, mchp_core_pwm->base + reg_offset); > + mchp_core_pwm->channel_enabled &= ~BIT(pwm->hwpwm); > + mchp_core_pwm->channel_enabled |= enable << pwm->hwpwm; > + > + /* > + * Notify the block to update the waveform from the shadow registers. > + * The updated values will not appear on the bus until they have been > + * applied to the waveform at the beginning of the next period. > + * This is a NO-OP if the channel does not have shadow registers. > + */ > + if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) > + mchp_core_pwm->update_timestamp = ktime_add_ns(ktime_get(), period); > +} > + > +static void mchp_core_pwm_wait_for_sync_update(struct mchp_core_pwm_chip *mchp_core_pwm, > + unsigned int channel) > +{ > + /* > + * If a shadow register is used for this PWM channel, and iff there is > + * a pending update to the waveform, we must wait for it to be applied > + * before attempting to read its state. Reading the registers yields > + * the currently implemented settings & the new ones are only readable > + * once the current period has ended. > + */ > + > + if (mchp_core_pwm->sync_update_mask & (1 << channel)) { > + ktime_t current_time = ktime_get(); > + s64 remaining_ns; > + u32 delay_us; > + > + remaining_ns = ktime_to_ns(ktime_sub(mchp_core_pwm->update_timestamp, > + current_time)); > + > + /* > + * If the update has gone through, don't bother waiting for > + * obvious reasons. Otherwise wait around for an appropriate > + * amount of time for the update to go through. > + */ > + if (remaining_ns <= 0) > + return; > + > + delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC); > + fsleep(delay_us); > + } > +} > + > +static u64 mchp_core_pwm_calc_duty(const struct pwm_state *state, u64 clk_rate, > + u8 prescale, u8 period_steps) > +{ > + u64 duty_steps, tmp; > + u16 prescale_val = PREG_TO_VAL(prescale); > + > + /* > + * Calculate the duty cycle in multiples of the prescaled period: > + * duty_steps = duty_in_ns / step_in_ns > + * step_in_ns = (prescale * NSEC_PER_SEC) / clk_rate > + * The code below is rearranged slightly to only divide once. > + */ > + tmp = prescale_val * NSEC_PER_SEC; > + duty_steps = mul_u64_u64_div_u64(state->duty_cycle, clk_rate, tmp); > + > + return duty_steps; > +} > + > +static void mchp_core_pwm_apply_duty(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state, u64 duty_steps, > + u16 period_steps) > +{ > + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); > + u8 posedge, negedge; > + u8 first_edge = 0, second_edge = duty_steps; > + > + /* > + * Setting posedge == negedge doesn't yield a constant output, > + * so that's an unsuitable setting to model duty_steps = 0. > + * In that case set the unwanted edge to a value that never > + * triggers. > + */ > + if (duty_steps == 0) > + first_edge = PREG_TO_VAL(period_steps); > + > + if (state->polarity == PWM_POLARITY_INVERSED) { > + negedge = first_edge; > + posedge = second_edge; > + } else { > + posedge = first_edge; > + negedge = second_edge; > + } > + > + writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm)); > + writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm)); > +} > + > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate, > + u16 *prescale, u16 *period_steps) > +{ > + u64 tmp; > + > + /* > + * Calculate the period cycles and prescale values. > + * The registers are each 8 bits wide & multiplied to compute the period > + * using the formula: > + * (prescale + 1) * (period_steps + 1) > + * period = ------------------------------------- > + * clk_rate > + * so the maximum period that can be generated is 0x10000 times the > + * period of the input clock. > + * However, due to the design of the "hardware", it is not possible to > + * attain a 100% duty cycle if the full range of period_steps is used. > + * Therefore period_steps is restricted to 0xFE and the maximum multiple > + * of the clock period attainable is 0xFF00. > + */ > + tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC); > + > + /* > + * The hardware adds one to the register value, so decrement by one to > + * account for the offset > + */ > + if (tmp >= MCHPCOREPWM_PERIOD_MAX) { > + *prescale = MCHPCOREPWM_PRESCALE_MAX - 1; > + *period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1; > + > + return; > + } > + > + /* > + * The optimal value for prescale can be calculated using the maximum > + * permitted value of period_steps, 0xff. I had to think about that one for a while. The maximal value for (period_steps + 1) is 0xff with the reasoning above?! That's also what the code uses. Also as the comment is written here, it's wrong (or misleading) depending on the semantic of "optimal". If you want to achive (prescale + 1) * (period_steps + 1) <= 64009 you should pick prescale == period_steps == 252 to get optimally near 64009. However the idea is to pick a set of values with period_steps being big to allow a finegrained selection for the duty cycle, right? Consider clk_rate = 1000000 period = 64009000 then your code gives: period * clk_rate tmp = ----------------- = 64009 NSEC_PER_SEC and so *prescale = 251 and *period_steps = 253. Wasn't the intention to pick *prescale = 250 and then *period_steps = 255? Depending on your semantics of "optimal", either (252, 252) or (250, 255) is better than (251, 253). I think that means you shouldn't ignore the -1? One thing I think is strange is that with clk_rate = 1000001 and your algorithm we get: requested period = 1274998 ns -> real period = 1269998.73000127 (prescale = 4, period_steps = 253) requested period = 1274999 ns -> real period = 1271998.728001272 (prescale = 5, period_steps = 211) while 1271998.728001272 would be a better match for a request with period = 1274998 than 1269998.73000127. I spend too much time to think about that now. I'm unsure if this is because the -1 is missing, or if there is a bug in the idea to pick a small prescale to allow a big period_steps value (in combination with the request to pick the biggest possible period). Hmm, maybe you understand that better than me? I'll have to think about it. > + * > + * period * clk_rate > + * prescale = ------------------- - 1 > + * NSEC_PER_SEC * 0xff > + * > + * However, we are purely interested in the integer upper bound of this > + * calculation, so ignore the subtraction & rely on the truncation done > + * by the division. > + * > + * period * clk_rate > + * ------------------- was precomputed as `tmp` > + * NSEC_PER_SEC > + * > + * period_steps is then computed using the result: > + * period * clk_rate > + * period_steps = ----------------------------- - 1 > + * NSEC_PER_SEC * (prescale + 1) > + */ > + *prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX); > + *period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1; > +} > + > [..] > +static int mchp_core_pwm_probe(struct platform_device *pdev) > +{ > + struct mchp_core_pwm_chip *mchp_core_pwm; > + struct resource *regs; > + > + mchp_core_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_core_pwm), GFP_KERNEL); > + if (!mchp_core_pwm) > + return -ENOMEM; > + > + mchp_core_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, ®s); > + if (IS_ERR(mchp_core_pwm->base)) > + return PTR_ERR(mchp_core_pwm->base); > + > + mchp_core_pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL); > + if (IS_ERR(mchp_core_pwm->clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(mchp_core_pwm->clk), > + "failed to get PWM clock\n"); > + > + if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask", > + &mchp_core_pwm->sync_update_mask)) > + mchp_core_pwm->sync_update_mask = 0; > + > + mutex_init(&mchp_core_pwm->lock); > + > + mchp_core_pwm->chip.dev = &pdev->dev; > + mchp_core_pwm->chip.ops = &mchp_core_pwm_ops; > + mchp_core_pwm->chip.npwm = 16; > + > + mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0)); > + mchp_core_pwm->channel_enabled |= > + readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(1)) << 8; > + > + writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD); This one is just for the case where there is an unapplied configuration in the registers, right? > + mchp_core_pwm->update_timestamp = ktime_get(); > + > + return devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip); An error message if devm_pwmchip_add() fails would be nice. > +} > + > +static struct platform_driver mchp_core_pwm_driver = { > + .driver = { > + .name = "mchp-core-pwm", > + .of_match_table = mchp_core_of_match, > + }, > + .probe = mchp_core_pwm_probe, > +}; > +module_platform_driver(mchp_core_pwm_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>"); > +MODULE_DESCRIPTION("corePWM driver for Microchip FPGAs"); > -- > 2.39.2 > >
On Wed, Mar 22, 2023 at 11:55:36AM +0100, Uwe Kleine-König wrote: > On Mon, Mar 06, 2023 at 09:48:58AM +0000, Conor Dooley wrote: > > Add a driver that supports the Microchip FPGA "soft" PWM IP core. > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate, > > + u16 *prescale, u16 *period_steps) > > +{ > > + u64 tmp; > > + > > + /* > > + * Calculate the period cycles and prescale values. > > + * The registers are each 8 bits wide & multiplied to compute the period > > + * using the formula: > > + * (prescale + 1) * (period_steps + 1) > > + * period = ------------------------------------- > > + * clk_rate > > + * so the maximum period that can be generated is 0x10000 times the > > + * period of the input clock. > > + * However, due to the design of the "hardware", it is not possible to > > + * attain a 100% duty cycle if the full range of period_steps is used. > > + * Therefore period_steps is restricted to 0xFE and the maximum multiple > > + * of the clock period attainable is 0xFF00. > > + */ > > + tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC); > > + > > + /* > > + * The hardware adds one to the register value, so decrement by one to > > + * account for the offset > > + */ > > + if (tmp >= MCHPCOREPWM_PERIOD_MAX) { > > + *prescale = MCHPCOREPWM_PRESCALE_MAX - 1; > > + *period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1; > > + > > + return; > > + } > > + > > + /* > > + * The optimal value for prescale can be calculated using the maximum > > + * permitted value of period_steps, 0xff. > > I had to think about that one for a while. The maximal value for > (period_steps + 1) is 0xff with the reasoning above?! That's also what > the code uses. I've become really disenfranchised with these register/variable names. I feel like just changing them to disconnect the variables used for calculation from the register names a little, so that the "is there a +1 needed here or not" stuff becomes a little clearer. It always makes sense to be when I am in an "I respun the patch today" mode, but by the time we're in the review stage I get muddled. God forbid I have to look at this in 10 years time. That said, there is a bit of a mistake here. The comment two above says "Therefore period_steps is restricted to 0xFE" when I'm capping things off. Some inaccuracies have probably snuck in during the various respins, and I think the comment above is "correct" but misleading, as it muddies the waters about variable versus register names. > Also as the comment is written here, it's wrong (or misleading) > depending on the semantic of "optimal". If you want to achive > > (prescale + 1) * (period_steps + 1) <= 64009 > > you should pick prescale == period_steps == 252 to get optimally near > 64009. > However the idea is to pick a set of values with period_steps being big > to allow a finegrained selection for the duty cycle, right? Correct. I'll update the comments with an explanation as to what the objective is, rather than just referring to it as "optimal". > Consider > > clk_rate = 1000000 > period = 64009000 > > then your code gives: > > period * clk_rate > tmp = ----------------- = 64009 > NSEC_PER_SEC > > and so *prescale = 251 and *period_steps = 253. > > Wasn't the intention to pick *prescale = 250 and then > *period_steps = 255? > > Depending on your semantics of "optimal", either (252, 252) or (250, > 255) is better than (251, 253). I think that means you shouldn't ignore > the -1? > > One thing I think is strange is that with clk_rate = 1000001 and your > algorithm we get: > > requested period = 1274998 ns -> real period = 1269998.73000127 (prescale = 4, period_steps = 253) > requested period = 1274999 ns -> real period = 1271998.728001272 (prescale = 5, period_steps = 211) > > while 1271998.728001272 would be a better match for a request with > period = 1274998 than 1269998.73000127. > > I spend too much time to think about that now. I'm unsure if this is > because the -1 is missing, or if there is a bug in the idea to pick a > small prescale to allow a big period_steps value (in combination with > the request to pick the biggest possible period). > > Hmm, maybe you understand that better than me? I'll have to think about > it. I'll have to think about it too, I'll clear a space among the todo-lists on my whiteboard tomorrow or Friday and get back to you... > > > + * > > + * period * clk_rate > > + * prescale = ------------------- - 1 > > + * NSEC_PER_SEC * 0xff > > + * > > + * However, we are purely interested in the integer upper bound of this > > + * calculation, so ignore the subtraction & rely on the truncation done > > + * by the division. > > + * > > + * period * clk_rate > > + * ------------------- was precomputed as `tmp` > > + * NSEC_PER_SEC > > + * > > + * period_steps is then computed using the result: > > + * period * clk_rate > > + * period_steps = ----------------------------- - 1 > > + * NSEC_PER_SEC * (prescale + 1) > > + */ > > + *prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX); > > + *period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1; > > +} > > + > > [..] > > +static int mchp_core_pwm_probe(struct platform_device *pdev) > > +{ > > + struct mchp_core_pwm_chip *mchp_core_pwm; > > + struct resource *regs; > > + > > + mchp_core_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_core_pwm), GFP_KERNEL); > > + if (!mchp_core_pwm) > > + return -ENOMEM; > > + > > + mchp_core_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, ®s); > > + if (IS_ERR(mchp_core_pwm->base)) > > + return PTR_ERR(mchp_core_pwm->base); > > + > > + mchp_core_pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL); > > + if (IS_ERR(mchp_core_pwm->clk)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(mchp_core_pwm->clk), > > + "failed to get PWM clock\n"); > > + > > + if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask", > > + &mchp_core_pwm->sync_update_mask)) > > + mchp_core_pwm->sync_update_mask = 0; > > + > > + mutex_init(&mchp_core_pwm->lock); > > + > > + mchp_core_pwm->chip.dev = &pdev->dev; > > + mchp_core_pwm->chip.ops = &mchp_core_pwm_ops; > > + mchp_core_pwm->chip.npwm = 16; > > + > > + mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0)); > > + mchp_core_pwm->channel_enabled |= > > + readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(1)) << 8; > > + > > + writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD); > > This one is just for the case where there is an unapplied configuration > in the registers, right? No, this is me realising that I had a misconception about how that register works. You write the bit once, and it enables the mode for channels that have been configured that way at synthesis-time, rather than how I somehow thought it worked which was as a "flush" from the shadow registers into the "real" ones. > > > + mchp_core_pwm->update_timestamp = ktime_get(); > > + > > + return devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip); > > An error message if devm_pwmchip_add() fails would be nice. Sure, can do! Thanks, Conor.
Hello Conor, On Wed, Mar 22, 2023 at 10:49:40PM +0000, Conor Dooley wrote: > On Wed, Mar 22, 2023 at 11:55:36AM +0100, Uwe Kleine-König wrote: > > On Mon, Mar 06, 2023 at 09:48:58AM +0000, Conor Dooley wrote: > > > Add a driver that supports the Microchip FPGA "soft" PWM IP core. > > > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate, > > > + u16 *prescale, u16 *period_steps) > > > +{ > > > + u64 tmp; > > > + > > > + /* > > > + * Calculate the period cycles and prescale values. > > > + * The registers are each 8 bits wide & multiplied to compute the period > > > + * using the formula: > > > + * (prescale + 1) * (period_steps + 1) > > > + * period = ------------------------------------- > > > + * clk_rate > > > + * so the maximum period that can be generated is 0x10000 times the > > > + * period of the input clock. > > > + * However, due to the design of the "hardware", it is not possible to > > > + * attain a 100% duty cycle if the full range of period_steps is used. > > > + * Therefore period_steps is restricted to 0xFE and the maximum multiple > > > + * of the clock period attainable is 0xFF00. > > > + */ > > > + tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC); > > > + > > > + /* > > > + * The hardware adds one to the register value, so decrement by one to > > > + * account for the offset > > > + */ > > > + if (tmp >= MCHPCOREPWM_PERIOD_MAX) { > > > + *prescale = MCHPCOREPWM_PRESCALE_MAX - 1; > > > + *period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1; > > > + > > > + return; > > > + } > > > + > > > + /* > > > + * The optimal value for prescale can be calculated using the maximum > > > + * permitted value of period_steps, 0xff. > > > > I had to think about that one for a while. The maximal value for > > (period_steps + 1) is 0xff with the reasoning above?! That's also what > > the code uses. > > I've become really disenfranchised with these register/variable names. > I feel like just changing them to disconnect the variables used for > calculation from the register names a little, so that the "is there a +1 > needed here or not" stuff becomes a little clearer. Full ack, I considered asking for that, but after some time I was in the "I reviewed the patch today"-mode (which is quite similar to the mode you described :-) and forgot. (Even in that mode the PREG_TO_VAL macro annoyed me a bit.) > It always makes sense to be when I am in an "I respun the patch today" > mode, but by the time we're in the review stage I get muddled. > God forbid I have to look at this in 10 years time. > > That said, there is a bit of a mistake here. The comment two above says > "Therefore period_steps is restricted to 0xFE" when I'm capping things > off. Some inaccuracies have probably snuck in during the various > respins, and I think the comment above is "correct" but misleading, as > it muddies the waters about variable versus register names. I think it's sensible to only talk about either the register values or the factors. I tend to think that talking about the register values is easier at the end and recommend not to hide the +1 (or -1) in a macro. Having said that here are my results of thinking a bit about how to choose register values: Let r(p) denote the period that is actually configured if p is requested. The nice properties we want (i.e. those a consumer might expect?) are: a) ∀p: r(p) ≤ p i.e. never configure a bigger period than requested (This is a bit arbitrary, but nice to get a consistent behaviour for all drivers and easier to handle than requesting the nearest possible period.) b) ∀p, q: p ≤ q ⟹ r(p) ≤ r(q) i.e. monotonicity c) ∀p: r(roundup(r(p))) = r(p) i.e. idempotency d) ∀p, q: r(p) ≤ q ⟹ r(p) ≤ r(q) i.e. pick the biggest possible period (Sidenote: d) and a) imply b) and c)) If you always set period_steps to 0xfe (in Python syntax: tmp = p * clkrate // NSPS period_steps = 0xfe prescale = tmp // (period_steps + 1) - 1 ) you get this consistent behaviour. This has the upside of being easy to implement and cheap to run. Downside is that it's coarse and fails to implement periods that would require e.g prescale = 0 and period_steps < 0xfe. As period_steps is big, the domain to chose the duty cycle from is good. If you pick period_steps and prescale such that (period_steps + 1) * (prescale + 1) is the maximal value that makes r(p) ≤ p you have an increased runtime because you have to test multiple values for period_steps. I don't think there is an algorithm without a loop to determine an optimal pair?! Usually this gives a better match that the easy algorithm above for the period, but the domain for duty_cycle is (usually) smaller. I think we have a) and d) here, so that's good. I think for most consumers a big domain for duty_cycle is more important that a good match for the requested period. So I tend to recommend the easy algorithm, but I'm not religious about that and open for other opinion and reasoning. > > > + writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD); > > > > This one is just for the case where there is an unapplied configuration > > in the registers, right? > > No, this is me realising that I had a misconception about how that > register works. You write the bit once, and it enables the mode for > channels that have been configured that way at synthesis-time, rather > than how I somehow thought it worked which was as a "flush" from the > shadow registers into the "real" ones. Then I think I got that wrong in the same way. Should this be better described in a comment then? (I didn't recheck, maybe there is a good description already?) Best regards Uwe
On Thu, Mar 23, 2023 at 10:14:09AM +0100, Uwe Kleine-König wrote: > On Wed, Mar 22, 2023 at 10:49:40PM +0000, Conor Dooley wrote: > > On Wed, Mar 22, 2023 at 11:55:36AM +0100, Uwe Kleine-König wrote: > > > On Mon, Mar 06, 2023 at 09:48:58AM +0000, Conor Dooley wrote: > > > > Add a driver that supports the Microchip FPGA "soft" PWM IP core. > > > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate, > > > > + u16 *prescale, u16 *period_steps) > > > > +{ > > > > + u64 tmp; > > > > + > > > > + /* > > > > + * Calculate the period cycles and prescale values. > > > > + * The registers are each 8 bits wide & multiplied to compute the period > > > > + * using the formula: > > > > + * (prescale + 1) * (period_steps + 1) > > > > + * period = ------------------------------------- > > > > + * clk_rate > > > > + * so the maximum period that can be generated is 0x10000 times the > > > > + * period of the input clock. > > > > + * However, due to the design of the "hardware", it is not possible to > > > > + * attain a 100% duty cycle if the full range of period_steps is used. > > > > + * Therefore period_steps is restricted to 0xFE and the maximum multiple > > > > + * of the clock period attainable is 0xFF00. > > > > + */ > > > > + tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC); > > > > + > > > > + /* > > > > + * The hardware adds one to the register value, so decrement by one to > > > > + * account for the offset > > > > + */ > > > > + if (tmp >= MCHPCOREPWM_PERIOD_MAX) { > > > > + *prescale = MCHPCOREPWM_PRESCALE_MAX - 1; > > > > + *period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1; > > > > + > > > > + return; > > > > + } > > > > + > > > > + /* > > > > + * The optimal value for prescale can be calculated using the maximum > > > > + * permitted value of period_steps, 0xff. > > > > > > I had to think about that one for a while. The maximal value for > > > (period_steps + 1) is 0xff with the reasoning above?! That's also what > > > the code uses. > > > > I've become really disenfranchised with these register/variable names. > > I feel like just changing them to disconnect the variables used for > > calculation from the register names a little, so that the "is there a +1 > > needed here or not" stuff becomes a little clearer. > > Full ack, I considered asking for that, but after some time I was in the > "I reviewed the patch today"-mode (which is quite similar to the mode > you described :-) and forgot. (Even in that mode the PREG_TO_VAL macro > annoyed me a bit.) > > > It always makes sense to be when I am in an "I respun the patch today" > > mode, but by the time we're in the review stage I get muddled. > > God forbid I have to look at this in 10 years time. > > > > That said, there is a bit of a mistake here. The comment two above says > > "Therefore period_steps is restricted to 0xFE" when I'm capping things > > off. Some inaccuracies have probably snuck in during the various > > respins, and I think the comment above is "correct" but misleading, as > > it muddies the waters about variable versus register names. > > I think it's sensible to only talk about either the register values or > the factors. I tend to think that talking about the register values is > easier at the end and recommend not to hide the +1 (or -1) in a macro. Yeah, I think the macro had value about 14 versions ago, but the number of users has dropped over the revisions. I think what I am going to to do for the next version is drop that macro, and only ever hold the registers values in variables. That had the advantage of making the maths in get_state() better match the comments that are now quite prevalent in the driver, as the +1s done there are more obvious. I'm loathe to link a git tree but, without changes to the period calculation logic, this is what it looks like w/ a consistent approach to what period_steps and prescale mean: https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/tree/drivers/pwm/pwm-microchip-core.c?h=pwm-dev-v15 [I blindly pushed that before leaving work & without even building it, so there's probably some silly mistake in it, but that's besides the point of displaying variable/comment changes] From the chopped out bits of the previous email: > Consider > > clk_rate = 1000000 > period = 64009000 > > then your code gives: > > period * clk_rate > tmp = ----------------- = 64009 > NSEC_PER_SEC > > and so *prescale = 251 and *period_steps = 253. > > Wasn't the intention to pick *prescale = 250 and then > *period_steps = 255? > > Depending on your semantics of "optimal", either (252, 252) or (250, > 255) is better than (251, 253). I think that means you shouldn't ignore > the -1? So, putting this one aside because it merits more thinking about. I went through and checked some arbitrary values of tmp, rather than dealing with "real" ones computed from frequencies I know are easily available for me to use in the FPGA bitstream I use to test this stuff. I think my "integer maths" truncation approach is not actually valid. Consider tmp = 255... *prescale gets computed as 255/(254 + 1) = 1, per my algorithm. Then, *period_steps is 255/(1 + 1) - 1 = 127. The period is (1 + 1)(127 + 1), which is not 255. Or take tmp = 510. prescale is 510/(254 + 1) = 2. period_steps is then 510/(2 + 1) - 1 = 169. period is (2 + 1)(169 + 1), which is 510. The calculated period is right, but that is not the "optimal" value! I think you're right in that I do actually need to consider the -1, and do a ceiling operation, when calculating prescale, IOW: *prescale = ceil(div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX + 1)) - 1; *period_steps = div_u64(tmp, *prescale + 1) - 1; [I know I can't actually call ceil()] That'd do the same thing as the truncation where div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX + 1) is not a round number, but it improves the round number case, eg tmp = 510: prescale = 510/(254 + 1) - 1 = 1, period_steps = 510/(1 + 1) - 1 = 254 period = (1 + 1)(254 + 1) = 510 It does mean a zero period would need to be special cased, but I don't mind that. > One thing I think is strange is that with clk_rate = 1000001 and your > algorithm we get: > > requested period = 1274998 ns -> real period = 1269998.73000127 (prescale = 4, period_steps = 253) > requested period = 1274999 ns -> real period = 1271998.728001272 (prescale = 5, period_steps = 211) This second one here, where tmp = 1275, is a good example of the above. With the ceil() change, this would be prescale = 4, period_steps = 254 which I think makes more sense. > > while 1271998.728001272 would be a better match for a request with > period = 1274998 than 1269998.73000127. > > I spend too much time to think about that now. I'm unsure if this is > because the -1 is missing, or if there is a bug in the idea to pick a > small prescale to allow a big period_steps value (in combination with > the request to pick the biggest possible period). With the inconsistency fixed, I think getting the slightly less accurate period is a byproduct of prioritising the finegrainedness of the duty cycle. > Hmm, maybe you understand that better than me? I'll have to think about > it. [end of snip from the last mail] > > Having said that here are my results of thinking a bit about how to > choose register values: > > Let r(p) denote the period that is actually configured if p is > requested. > > The nice properties we want (i.e. those a consumer might expect?) are: > > a) ∀p: r(p) ≤ p > i.e. never configure a bigger period than requested > (This is a bit arbitrary, but nice to get a consistent behaviour for > all drivers and easier to handle than requesting the nearest > possible period.) > > b) ∀p, q: p ≤ q ⟹ r(p) ≤ r(q) > i.e. monotonicity > > c) ∀p: r(roundup(r(p))) = r(p) > i.e. idempotency > > d) ∀p, q: r(p) ≤ q ⟹ r(p) ≤ r(q) > i.e. pick the biggest possible period > > (Sidenote: d) and a) imply b) and c)) > > If you always set period_steps to 0xfe > (in Python syntax: > > tmp = p * clkrate // NSPS > period_steps = 0xfe > prescale = tmp // (period_steps + 1) - 1 > > ) you get this consistent behaviour. > > This has the upside of being easy to implement and cheap to run. > Downside is that it's coarse and fails to implement periods that would > require e.g prescale = 0 and period_steps < 0xfe. As period_steps is > big, the domain to chose the duty cycle from is good. I want to maintain support for prescale = 0, so I'm not really interested in a computation that forsakes that. > If you pick period_steps and prescale such that > (period_steps + 1) * (prescale + 1) > is the maximal value that makes r(p) ≤ p you have an increased runtime > because you have to test multiple values for period_steps. I don't think > there is an algorithm without a loop to determine an optimal pair?! > Usually this gives a better match that the easy algorithm above for the > period, but the domain for duty_cycle is (usually) smaller. > I think we have a) and d) here, so that's good. > > I think for most consumers a big domain for duty_cycle is more important > that a good match for the requested period. So I tend to recommend the > easy algorithm, but I'm not religious about that and open for other > opinion and reasoning. I'll be honest and say that I am getting a bit fatigued with the way that issues w/ the calculations keep cropping up. I'll put a bit of time into trying to figure out how to fix the tmp = 6400900 case that you mentioned above, but if it comes out in the wash that that is a facet of the way this stuff is computed, then I might be inclined to forge ahead without resolving it... I'd certainly want to explain in a comment somewhere (limitations section?) the point at which it starts getting less accurate though. I'll write a script to iterate through the values of tmp & see if the reason is obvious. I would like to keep (something resembling) the current simple implementation of the period calculation, as simplicity is a significant advantage! I do also like the strategy of trying to maximise the number of available duty cycle steps, I think it makes perfect sense to make that the goal. Thanks again for spending time on this Uwe, Conor.
On Fri, Mar 24, 2023 at 06:43:03PM +0000, Conor Dooley wrote: > On Thu, Mar 23, 2023 at 10:14:09AM +0100, Uwe Kleine-König wrote: > > On Wed, Mar 22, 2023 at 10:49:40PM +0000, Conor Dooley wrote: > > > On Wed, Mar 22, 2023 at 11:55:36AM +0100, Uwe Kleine-König wrote: > > > > On Mon, Mar 06, 2023 at 09:48:58AM +0000, Conor Dooley wrote: > > > > > Add a driver that supports the Microchip FPGA "soft" PWM IP core. > > > > > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate, > > > > > + u16 *prescale, u16 *period_steps) > > > > > +{ > > > > > + u64 tmp; > > > > > + > > > > > + /* > > > > > + * Calculate the period cycles and prescale values. > > > > > + * The registers are each 8 bits wide & multiplied to compute the period > > > > > + * using the formula: > > > > > + * (prescale + 1) * (period_steps + 1) > > > > > + * period = ------------------------------------- > > > > > + * clk_rate > > > > > + * so the maximum period that can be generated is 0x10000 times the > > > > > + * period of the input clock. > > > > > + * However, due to the design of the "hardware", it is not possible to > > > > > + * attain a 100% duty cycle if the full range of period_steps is used. > > > > > + * Therefore period_steps is restricted to 0xFE and the maximum multiple > > > > > + * of the clock period attainable is 0xFF00. > > > > > + */ > > > > > + tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC); > > > > > + > > > > > + /* > > > > > + * The hardware adds one to the register value, so decrement by one to > > > > > + * account for the offset > > > > > + */ > > > > > + if (tmp >= MCHPCOREPWM_PERIOD_MAX) { > > > > > + *prescale = MCHPCOREPWM_PRESCALE_MAX - 1; > > > > > + *period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1; > > > > > + > > > > > + return; > > > > > + } > > > > > + > > > > > + /* > > > > > + * The optimal value for prescale can be calculated using the maximum > > > > > + * permitted value of period_steps, 0xff. > > > > > > > > I had to think about that one for a while. The maximal value for > > > > (period_steps + 1) is 0xff with the reasoning above?! That's also what > > > > the code uses. > > > > > > I've become really disenfranchised with these register/variable names. > > > I feel like just changing them to disconnect the variables used for > > > calculation from the register names a little, so that the "is there a +1 > > > needed here or not" stuff becomes a little clearer. > > > > Full ack, I considered asking for that, but after some time I was in the > > "I reviewed the patch today"-mode (which is quite similar to the mode > > you described :-) and forgot. (Even in that mode the PREG_TO_VAL macro > > annoyed me a bit.) > > > > > It always makes sense to be when I am in an "I respun the patch today" > > > mode, but by the time we're in the review stage I get muddled. > > > God forbid I have to look at this in 10 years time. > > > > > > That said, there is a bit of a mistake here. The comment two above says > > > "Therefore period_steps is restricted to 0xFE" when I'm capping things > > > off. Some inaccuracies have probably snuck in during the various > > > respins, and I think the comment above is "correct" but misleading, as > > > it muddies the waters about variable versus register names. > > > > I think it's sensible to only talk about either the register values or > > the factors. I tend to think that talking about the register values is > > easier at the end and recommend not to hide the +1 (or -1) in a macro. > > Yeah, I think the macro had value about 14 versions ago, but the number > of users has dropped over the revisions. > I think what I am going to to do for the next version is drop that > macro, and only ever hold the registers values in variables. > That had the advantage of making the maths in get_state() better match > the comments that are now quite prevalent in the driver, as the +1s done > there are more obvious. > I'm loathe to link a git tree but, without changes to the period > calculation logic, this is what it looks like w/ a consistent approach > to what period_steps and prescale mean: > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/tree/drivers/pwm/pwm-microchip-core.c?h=pwm-dev-v15 > [I blindly pushed that before leaving work & without even building it, so > there's probably some silly mistake in it, but that's besides the point > of displaying variable/comment changes] > > From the chopped out bits of the previous email: > > Consider > > > > clk_rate = 1000000 > > period = 64009000 > > > > then your code gives: > > > > period * clk_rate > > tmp = ----------------- = 64009 > > NSEC_PER_SEC > > > > and so *prescale = 251 and *period_steps = 253. > > > > Wasn't the intention to pick *prescale = 250 and then > > *period_steps = 255? > > > > Depending on your semantics of "optimal", either (252, 252) or (250, > > 255) is better than (251, 253). I think that means you shouldn't ignore > > the -1? > > So, putting this one aside because it merits more thinking about. > > I went through and checked some arbitrary values of tmp, rather than > dealing with "real" ones computed from frequencies I know are easily > available for me to use in the FPGA bitstream I use to test this stuff. > > I think my "integer maths" truncation approach is not actually valid. > Consider tmp = 255... *prescale gets computed as 255/(254 + 1) = 1, per my > algorithm. Then, *period_steps is 255/(1 + 1) - 1 = 127. > The period is (1 + 1)(127 + 1), which is not 255. > > Or take tmp = 510. prescale is 510/(254 + 1) = 2. period_steps is then > 510/(2 + 1) - 1 = 169. period is (2 + 1)(169 + 1), which is 510. The > calculated period is right, but that is not the "optimal" value! > > I think you're right in that I do actually need to consider the -1, and > do a ceiling operation, when calculating prescale, IOW: > *prescale = ceil(div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX + 1)) - 1; > *period_steps = div_u64(tmp, *prescale + 1) - 1; > [I know I can't actually call ceil()] > > That'd do the same thing as the truncation where > div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX + 1) is not a round number, > but it improves the round number case, eg tmp = 510: > prescale = 510/(254 + 1) - 1 = 1, period_steps = 510/(1 + 1) - 1 = 254 > period = (1 + 1)(254 + 1) = 510 > > It does mean a zero period would need to be special cased, but I don't > mind that. > > > One thing I think is strange is that with clk_rate = 1000001 and your > > algorithm we get: > > > > requested period = 1274998 ns -> real period = 1269998.73000127 (prescale = 4, period_steps = 253) > > requested period = 1274999 ns -> real period = 1271998.728001272 (prescale = 5, period_steps = 211) > > This second one here, where tmp = 1275, is a good example of the above. > With the ceil() change, this would be prescale = 4, period_steps = 254 > which I think makes more sense. > > > > while 1271998.728001272 would be a better match for a request with > > period = 1274998 than 1269998.73000127. > > > > I spend too much time to think about that now. I'm unsure if this is > > because the -1 is missing, or if there is a bug in the idea to pick a > > small prescale to allow a big period_steps value (in combination with > > the request to pick the biggest possible period). > > With the inconsistency fixed, I think getting the slightly less accurate > period is a byproduct of prioritising the finegrainedness of the duty > cycle. > > > Hmm, maybe you understand that better than me? I'll have to think about > > it. > > [end of snip from the last mail] > > > > > Having said that here are my results of thinking a bit about how to > > choose register values: > > > > Let r(p) denote the period that is actually configured if p is > > requested. > > > > The nice properties we want (i.e. those a consumer might expect?) are: > > > > a) ∀p: r(p) ≤ p > > i.e. never configure a bigger period than requested > > (This is a bit arbitrary, but nice to get a consistent behaviour for > > all drivers and easier to handle than requesting the nearest > > possible period.) > > > > b) ∀p, q: p ≤ q ⟹ r(p) ≤ r(q) > > i.e. monotonicity > > > > c) ∀p: r(roundup(r(p))) = r(p) > > i.e. idempotency > > > > d) ∀p, q: r(p) ≤ q ⟹ r(p) ≤ r(q) > > i.e. pick the biggest possible period > > > > (Sidenote: d) and a) imply b) and c)) > > > > If you always set period_steps to 0xfe > > (in Python syntax: > > > > tmp = p * clkrate // NSPS > > period_steps = 0xfe > > prescale = tmp // (period_steps + 1) - 1 > > > > ) you get this consistent behaviour. > > > > This has the upside of being easy to implement and cheap to run. > > Downside is that it's coarse and fails to implement periods that would > > require e.g prescale = 0 and period_steps < 0xfe. As period_steps is > > big, the domain to chose the duty cycle from is good. > > I want to maintain support for prescale = 0, so I'm not really > interested in a computation that forsakes that. > > > If you pick period_steps and prescale such that > > (period_steps + 1) * (prescale + 1) > > is the maximal value that makes r(p) ≤ p you have an increased runtime > > because you have to test multiple values for period_steps. I don't think > > there is an algorithm without a loop to determine an optimal pair?! > > Usually this gives a better match that the easy algorithm above for the > > period, but the domain for duty_cycle is (usually) smaller. > > I think we have a) and d) here, so that's good. > > > > I think for most consumers a big domain for duty_cycle is more important > > that a good match for the requested period. So I tend to recommend the > > easy algorithm, but I'm not religious about that and open for other > > opinion and reasoning. > > I'll be honest and say that I am getting a bit fatigued with the way > that issues w/ the calculations keep cropping up. I'll put a bit of time > into trying to figure out how to fix the tmp = 6400900 case that you > mentioned above, but if it comes out in the wash that that is a facet of > the way this stuff is computed, then I might be inclined to forge ahead > without resolving it... I'd certainly want to explain in a comment > somewhere (limitations section?) the point at which it starts getting > less accurate though. I'll write a script to iterate through the values > of tmp & see if the reason is obvious. I threw together a python script to go through the various values of tmp & check which ones ended up with these "non-optimal" prescale values. For tmp < 1000, I saw: tmp: 511 | 255 - period_steps: 86 tmp: 766 | 255 - period_steps: 65 tmp: 767 | 255 - period_steps: 65 That then grows as tmp moves upwards, so: tmp: 1021 | 255 - period_steps: 52 tmp: 1022 | 255 - period_steps: 52 tmp: 1023 | 255 - period_steps: 52 That looks oddly like (tmp % (254 + 1)) < prescale and seems to be an "artifact" of using 254 to calculate the prescaler rather than 255. I think that that can be worked around while keeping the current simple approach to calculating prescale/period_steps. Looks like 32385 out of the 65280 valid values for tmp can benefit! That's in comparison to the 127 that would produce the invalid period_steps value of 255. I'll go submit something fixing this next week then I suppose. > I would like to keep (something resembling) the current simple > implementation of the period calculation, as simplicity is a significant > advantage! I do also like the strategy of trying to maximise the number > of available duty cycle steps, I think it makes perfect sense to make > that the goal. > > Thanks again for spending time on this Uwe, > Conor. >
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index dae023d783a22..f42756a014ed4 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -393,6 +393,16 @@ config PWM_MEDIATEK To compile this driver as a module, choose M here: the module will be called pwm-mediatek. +config PWM_MICROCHIP_CORE + tristate "Microchip corePWM PWM support" + depends on SOC_MICROCHIP_POLARFIRE || COMPILE_TEST + depends on HAS_IOMEM && OF + help + PWM driver for Microchip FPGA soft IP core. + + To compile this driver as a module, choose M here: the module + will be called pwm-microchip-core. + config PWM_MXS tristate "Freescale MXS PWM support" depends on ARCH_MXS || COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 7bf1a29f02b84..a65625359ece4 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -34,6 +34,7 @@ obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o obj-$(CONFIG_PWM_MESON) += pwm-meson.o obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o +obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o obj-$(CONFIG_PWM_MXS) += pwm-mxs.o obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c new file mode 100644 index 0000000000000..a2a2e28f39031 --- /dev/null +++ b/drivers/pwm/pwm-microchip-core.c @@ -0,0 +1,441 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * corePWM driver for Microchip "soft" FPGA IP cores. + * + * Copyright (c) 2021-2023 Microchip Corporation. All rights reserved. + * Author: Conor Dooley <conor.dooley@microchip.com> + * Documentation: + * https://www.microsemi.com/document-portal/doc_download/1245275-corepwm-hb + * + * Limitations: + * - If the IP block is configured without "shadow registers", all register + * writes will take effect immediately, causing glitches on the output. + * If shadow registers *are* enabled, a write to the "SYNC_UPDATE" register + * notifies the core that it needs to update the registers defining the + * waveform from the contents of the "shadow registers". + * - The IP block has no concept of a duty cycle, only rising/falling edges of + * the waveform. Unfortunately, if the rising & falling edges registers have + * the same value written to them the IP block will do whichever of a rising + * or a falling edge is possible. I.E. a 50% waveform at twice the requested + * period. Therefore to get a 0% waveform, the output is set the max high/low + * time depending on polarity. + * - The PWM period is set for the whole IP block not per channel. The driver + * will only change the period if no other PWM output is enabled. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/ktime.h> +#include <linux/math.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> + +#define PREG_TO_VAL(PREG) ((PREG) + 1) + +#define MCHPCOREPWM_PRESCALE_MAX 0x100 +#define MCHPCOREPWM_PERIOD_STEPS_MAX 0xff +#define MCHPCOREPWM_PERIOD_MAX 0xff00 + +#define MCHPCOREPWM_PRESCALE 0x00 +#define MCHPCOREPWM_PERIOD 0x04 +#define MCHPCOREPWM_EN(i) (0x08 + 0x04 * (i)) /* 0x08, 0x0c */ +#define MCHPCOREPWM_POSEDGE(i) (0x10 + 0x08 * (i)) /* 0x10, 0x18, ..., 0x88 */ +#define MCHPCOREPWM_NEGEDGE(i) (0x14 + 0x08 * (i)) /* 0x14, 0x1c, ..., 0x8c */ +#define MCHPCOREPWM_SYNC_UPD 0xe4 +#define MCHPCOREPWM_TIMEOUT_MS 100u + +struct mchp_core_pwm_chip { + struct pwm_chip chip; + struct clk *clk; + void __iomem *base; + struct mutex lock; /* protects the shared period */ + ktime_t update_timestamp; + u32 sync_update_mask; + u16 channel_enabled; +}; + +static inline struct mchp_core_pwm_chip *to_mchp_core_pwm(struct pwm_chip *chip) +{ + return container_of(chip, struct mchp_core_pwm_chip, chip); +} + +static void mchp_core_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm, + bool enable, u64 period) +{ + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); + u8 channel_enable, reg_offset, shift; + + /* + * There are two adjacent 8 bit control regs, the lower reg controls + * 0-7 and the upper reg 8-15. Check if the pwm is in the upper reg + * and if so, offset by the bus width. + */ + reg_offset = MCHPCOREPWM_EN(pwm->hwpwm >> 3); + shift = pwm->hwpwm & 7; + + channel_enable = readb_relaxed(mchp_core_pwm->base + reg_offset); + channel_enable &= ~(1 << shift); + channel_enable |= (enable << shift); + + writel_relaxed(channel_enable, mchp_core_pwm->base + reg_offset); + mchp_core_pwm->channel_enabled &= ~BIT(pwm->hwpwm); + mchp_core_pwm->channel_enabled |= enable << pwm->hwpwm; + + /* + * Notify the block to update the waveform from the shadow registers. + * The updated values will not appear on the bus until they have been + * applied to the waveform at the beginning of the next period. + * This is a NO-OP if the channel does not have shadow registers. + */ + if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) + mchp_core_pwm->update_timestamp = ktime_add_ns(ktime_get(), period); +} + +static void mchp_core_pwm_wait_for_sync_update(struct mchp_core_pwm_chip *mchp_core_pwm, + unsigned int channel) +{ + /* + * If a shadow register is used for this PWM channel, and iff there is + * a pending update to the waveform, we must wait for it to be applied + * before attempting to read its state. Reading the registers yields + * the currently implemented settings & the new ones are only readable + * once the current period has ended. + */ + + if (mchp_core_pwm->sync_update_mask & (1 << channel)) { + ktime_t current_time = ktime_get(); + s64 remaining_ns; + u32 delay_us; + + remaining_ns = ktime_to_ns(ktime_sub(mchp_core_pwm->update_timestamp, + current_time)); + + /* + * If the update has gone through, don't bother waiting for + * obvious reasons. Otherwise wait around for an appropriate + * amount of time for the update to go through. + */ + if (remaining_ns <= 0) + return; + + delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC); + fsleep(delay_us); + } +} + +static u64 mchp_core_pwm_calc_duty(const struct pwm_state *state, u64 clk_rate, + u8 prescale, u8 period_steps) +{ + u64 duty_steps, tmp; + u16 prescale_val = PREG_TO_VAL(prescale); + + /* + * Calculate the duty cycle in multiples of the prescaled period: + * duty_steps = duty_in_ns / step_in_ns + * step_in_ns = (prescale * NSEC_PER_SEC) / clk_rate + * The code below is rearranged slightly to only divide once. + */ + tmp = prescale_val * NSEC_PER_SEC; + duty_steps = mul_u64_u64_div_u64(state->duty_cycle, clk_rate, tmp); + + return duty_steps; +} + +static void mchp_core_pwm_apply_duty(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state, u64 duty_steps, + u16 period_steps) +{ + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); + u8 posedge, negedge; + u8 first_edge = 0, second_edge = duty_steps; + + /* + * Setting posedge == negedge doesn't yield a constant output, + * so that's an unsuitable setting to model duty_steps = 0. + * In that case set the unwanted edge to a value that never + * triggers. + */ + if (duty_steps == 0) + first_edge = PREG_TO_VAL(period_steps); + + if (state->polarity == PWM_POLARITY_INVERSED) { + negedge = first_edge; + posedge = second_edge; + } else { + posedge = first_edge; + negedge = second_edge; + } + + writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm)); + writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm)); +} + +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate, + u16 *prescale, u16 *period_steps) +{ + u64 tmp; + + /* + * Calculate the period cycles and prescale values. + * The registers are each 8 bits wide & multiplied to compute the period + * using the formula: + * (prescale + 1) * (period_steps + 1) + * period = ------------------------------------- + * clk_rate + * so the maximum period that can be generated is 0x10000 times the + * period of the input clock. + * However, due to the design of the "hardware", it is not possible to + * attain a 100% duty cycle if the full range of period_steps is used. + * Therefore period_steps is restricted to 0xFE and the maximum multiple + * of the clock period attainable is 0xFF00. + */ + tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC); + + /* + * The hardware adds one to the register value, so decrement by one to + * account for the offset + */ + if (tmp >= MCHPCOREPWM_PERIOD_MAX) { + *prescale = MCHPCOREPWM_PRESCALE_MAX - 1; + *period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1; + + return; + } + + /* + * The optimal value for prescale can be calculated using the maximum + * permitted value of period_steps, 0xff. + * + * period * clk_rate + * prescale = ------------------- - 1 + * NSEC_PER_SEC * 0xff + * + * However, we are purely interested in the integer upper bound of this + * calculation, so ignore the subtraction & rely on the truncation done + * by the division. + * + * period * clk_rate + * ------------------- was precomputed as `tmp` + * NSEC_PER_SEC + * + * period_steps is then computed using the result: + * period * clk_rate + * period_steps = ----------------------------- - 1 + * NSEC_PER_SEC * (prescale + 1) + */ + *prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX); + *period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1; +} + +static int mchp_core_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); + bool period_locked; + unsigned long clk_rate; + u64 duty_steps; + u16 prescale, period_steps; + + if (!state->enabled) { + mchp_core_pwm_enable(chip, pwm, false, pwm->state.period); + return 0; + } + + /* + * If clk_rate is too big, the following multiplication might overflow. + * However this is implausible, as the fabric of current FPGAs cannot + * provide clocks at a rate high enough. + */ + clk_rate = clk_get_rate(mchp_core_pwm->clk); + if (clk_rate >= NSEC_PER_SEC) + return -EINVAL; + + mchp_core_pwm_calc_period(state, clk_rate, &prescale, &period_steps); + + /* + * If the only thing that has changed is the duty cycle or the polarity, + * we can shortcut the calculations and just compute/apply the new duty + * cycle pos & neg edges + * As all the channels share the same period, do not allow it to be + * changed if any other channels are enabled. + * If the period is locked, it may not be possible to use a period + * less than that requested. In that case, we just abort. + */ + period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm); + + if (period_locked) { + u16 hw_prescale; + u16 hw_period_steps; + + hw_prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE); + hw_period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD); + + if ((period_steps + 1) * (prescale + 1) < + (hw_period_steps + 1) * (hw_prescale + 1)) + return -EINVAL; + + /* + * It is possible that something could have set the period_steps + * register to 0xff, which would prevent us from setting a 100% + * or 0% relative duty cycle, as explained above in + * mchp_core_pwm_calc_period(). + * The period is locked and we cannot change this, so we abort. + */ + if (hw_period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX) + return -EINVAL; + + prescale = hw_prescale; + period_steps = hw_period_steps; + } else { + writel_relaxed(prescale, mchp_core_pwm->base + MCHPCOREPWM_PRESCALE); + writel_relaxed(period_steps, mchp_core_pwm->base + MCHPCOREPWM_PERIOD); + } + + duty_steps = mchp_core_pwm_calc_duty(state, clk_rate, prescale, period_steps); + + /* + * Because the period is per channel, it is possible that the requested + * duty cycle is longer than the period, in which case cap it to the + * period, IOW a 100% duty cycle. + */ + if (duty_steps > period_steps) + duty_steps = period_steps + 1; + + mchp_core_pwm_apply_duty(chip, pwm, state, duty_steps, period_steps); + + mchp_core_pwm_enable(chip, pwm, true, pwm->state.period); + + return 0; +} + +static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); + int ret; + + mutex_lock(&mchp_core_pwm->lock); + + mchp_core_pwm_wait_for_sync_update(mchp_core_pwm, pwm->hwpwm); + + ret = mchp_core_pwm_apply_locked(chip, pwm, state); + + mutex_unlock(&mchp_core_pwm->lock); + + return ret; +} + +static int mchp_core_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); + u64 rate; + u16 prescale, period_steps; + u8 duty_steps, posedge, negedge; + + mutex_lock(&mchp_core_pwm->lock); + + mchp_core_pwm_wait_for_sync_update(mchp_core_pwm, pwm->hwpwm); + + if (mchp_core_pwm->channel_enabled & (1 << pwm->hwpwm)) + state->enabled = true; + else + state->enabled = false; + + rate = clk_get_rate(mchp_core_pwm->clk); + + prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE)); + + period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD)); + state->period = period_steps * prescale; + state->period *= NSEC_PER_SEC; + state->period = DIV64_U64_ROUND_UP(state->period, rate); + + posedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm)); + negedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm)); + + mutex_unlock(&mchp_core_pwm->lock); + + if (negedge == posedge) { + state->duty_cycle = state->period; + state->period *= 2; + } else { + duty_steps = abs((s16)posedge - (s16)negedge); + state->duty_cycle = duty_steps * prescale * NSEC_PER_SEC; + state->duty_cycle = DIV64_U64_ROUND_UP(state->duty_cycle, rate); + } + + state->polarity = negedge < posedge ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL; + + return 0; +} + +static const struct pwm_ops mchp_core_pwm_ops = { + .apply = mchp_core_pwm_apply, + .get_state = mchp_core_pwm_get_state, + .owner = THIS_MODULE, +}; + +static const struct of_device_id mchp_core_of_match[] = { + { + .compatible = "microchip,corepwm-rtl-v4", + }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, mchp_core_of_match); + +static int mchp_core_pwm_probe(struct platform_device *pdev) +{ + struct mchp_core_pwm_chip *mchp_core_pwm; + struct resource *regs; + + mchp_core_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_core_pwm), GFP_KERNEL); + if (!mchp_core_pwm) + return -ENOMEM; + + mchp_core_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, ®s); + if (IS_ERR(mchp_core_pwm->base)) + return PTR_ERR(mchp_core_pwm->base); + + mchp_core_pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL); + if (IS_ERR(mchp_core_pwm->clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(mchp_core_pwm->clk), + "failed to get PWM clock\n"); + + if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask", + &mchp_core_pwm->sync_update_mask)) + mchp_core_pwm->sync_update_mask = 0; + + mutex_init(&mchp_core_pwm->lock); + + mchp_core_pwm->chip.dev = &pdev->dev; + mchp_core_pwm->chip.ops = &mchp_core_pwm_ops; + mchp_core_pwm->chip.npwm = 16; + + mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0)); + mchp_core_pwm->channel_enabled |= + readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(1)) << 8; + + writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD); + mchp_core_pwm->update_timestamp = ktime_get(); + + return devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip); +} + +static struct platform_driver mchp_core_pwm_driver = { + .driver = { + .name = "mchp-core-pwm", + .of_match_table = mchp_core_of_match, + }, + .probe = mchp_core_pwm_probe, +}; +module_platform_driver(mchp_core_pwm_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>"); +MODULE_DESCRIPTION("corePWM driver for Microchip FPGAs");
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 | 441 +++++++++++++++++++++++++++++++ 3 files changed, 452 insertions(+) create mode 100644 drivers/pwm/pwm-microchip-core.c