| Message ID | 20250408-rk3576-pwm-v1-5-a49286c2ca8e@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Rockchip RK3576 PWM Support Through MFPWM | expand |
Hello Nicolas, On Tue, Apr 08, 2025 at 02:32:17PM +0200, Nicolas Frattaroli wrote: > The Rockchip RK3576 brings with it a new PWM IP, in downstream code > referred to as "v4". This new IP is different enough from the previous > Rockchip IP that I felt it necessary to add a new driver for it, instead > of shoehorning it in the old one. > > Add this new driver, based on the PWM core's waveform APIs. Its platform > device is registered by the parent mfpwm driver, from which it also > receives a little platform data struct, so that mfpwm can guarantee that > all the platform device drivers spread across different subsystems for > this specific hardware IP do not interfere with each other. > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> > --- > MAINTAINERS | 1 + > drivers/pwm/Kconfig | 13 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-rockchip-v4.c | 336 ++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 351 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20891,6 +20891,7 @@ L: linux-rockchip@lists.infradead.org > L: linux-pwm@vger.kernel.org > S: Maintained > F: Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml > +F: drivers/pwm/pwm-rockchip-v4.c > F: drivers/soc/rockchip/mfpwm.c > F: include/soc/rockchip/mfpwm.h > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -540,6 +540,19 @@ config PWM_ROCKCHIP > Generic PWM framework driver for the PWM controller found on > Rockchip SoCs. > > +config PWM_ROCKCHIP_V4 > + tristate "Rockchip PWM v4 support" > + depends on ARCH_ROCKCHIP || COMPILE_TEST > + depends on ROCKCHIP_MFPWM > + depends on HAS_IOMEM > + help > + Generic PWM framework driver for the PWM controller found on > + later Rockchip SoCs such as the RK3576. > + > + Uses the Rockchip Multi-function PWM controller driver infrastructure > + to guarantee fearlessly concurrent operation with other functions of > + the same device implemented by drivers in other subsystems. > + > config PWM_RZ_MTU3 > tristate "Renesas RZ/G2L MTU3a PWM Timer support" > depends on RZ_MTU3 > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE) += pwm-raspberrypi-poe.o > obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > +obj-$(CONFIG_PWM_ROCKCHIP_V4) += pwm-rockchip-v4.o > obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c > new file mode 100644 > index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e > --- /dev/null > +++ b/drivers/pwm/pwm-rockchip-v4.c > @@ -0,0 +1,336 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2025 Collabora Ltd. > + * > + * A Pulse-Width-Modulation (PWM) generator driver for the generators found in > + * Rockchip SoCs such as the RK3576, internally referred to as "PWM v4". Uses > + * the MFPWM infrastructure to guarantee exclusive use over the device without > + * other functions of the device from different drivers interfering with its > + * operation while it's active. Can you please add a "Limitations" paragraph here similar to the other newer drivers that explains how the hardware behave on disable (inactive? High-Z? freeze?), if there are glitches possible when settings are changed or if the currently running period is completed on reconfiguration. > + * > + * Authors: > + * Nicolas Frattaroli <nicolas.frattaroli@collabora.com> > + */ > + > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <soc/rockchip/mfpwm.h> > + > +struct rockchip_pwm_v4 { > + struct rockchip_mfpwm_func *pwmf; > + struct pwm_chip chip; > +}; > + > +struct rockchip_pwm_v4_wf { > + u32 period; > + u32 duty; > + u32 offset; > + u8 enable; > +}; > + > +static inline struct rockchip_pwm_v4 *to_rockchip_pwm_v4(struct pwm_chip *chip) > +{ > + return pwmchip_get_drvdata(chip); > +} > + > +/** > + * rockchip_pwm_v4_round_single - convert a PWM parameter to hardware > + * @rate: clock rate of the PWM clock, as per clk_get_rate > + * @in_val: parameter in nanoseconds to convert > + * @out_val: pointer to location where converted result should be stored. > + * > + * If @out_val is %NULL, no calculation is performed. > + * > + * Return: > + * * %0 - Success > + * * %-EOVERFLOW - Result too large for target type > + */ > +static int rockchip_pwm_v4_round_single(unsigned long rate, u64 in_val, > + u32 *out_val) > +{ > + u64 tmp; > + > + if (!out_val) > + return 0; This is never hit, so better drop it. > + tmp = mult_frac(rate, in_val, NSEC_PER_SEC); > + if (tmp > U32_MAX) > + return -EOVERFLOW; Is it clear that this cannot overflow the u64? > + *out_val = tmp; > + > + return 0; > +} > + > +/** > + * rockchip_pwm_v4_round_params - convert PWM parameters to hardware > + * @rate: PWM clock rate to do the calculations at > + * @duty: PWM duty cycle in nanoseconds > + * @period: PWM period in nanoseconds > + * @offset: PWM offset in nanoseconds > + * @out_duty: pointer to where the rounded duty value should be stored > + * if NULL, don't calculate or store it > + * @out_period: pointer to where the rounded period value should be stored > + * if NULL, don't calculate or store it > + * @out_offset: pointer to where the rounded offset value should be stored > + * if NULL, don't calculate or store it > + * > + * Convert nanosecond-based duty/period/offset parameters to the PWM hardware's > + * native rounded representation in number of cycles at clock rate @rate. If an > + * out_ parameter is a NULL pointer, the corresponding parameter will not be > + * calculated or stored. Should an overflow error occur for any of the > + * parameters, assume the data at all the out_ locations is invalid and may not > + * even have been touched at all. > + * > + * Return: > + * * %0 - Success > + * * %-EOVERFLOW - One of the results is too large for the PWM hardware > + */ > +static int rockchip_pwm_v4_round_params(unsigned long rate, u64 duty, > + u64 period, u64 offset, u32 *out_duty, > + u32 *out_period, u32 *out_offset) > +{ > + int ret; > + > + ret = rockchip_pwm_v4_round_single(rate, duty, out_duty); > + if (ret) > + return ret; > + > + ret = rockchip_pwm_v4_round_single(rate, period, out_period); > + if (ret) > + return ret; > + > + ret = rockchip_pwm_v4_round_single(rate, offset, out_offset); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int rockchip_pwm_v4_round_wf_tohw(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const struct pwm_waveform *wf, > + void *_wfhw) > +{ > + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); > + struct rockchip_pwm_v4_wf *wfhw = _wfhw; > + unsigned long rate; > + int ret = 0; > + > + /* We do not want chosen_clk to change out from under us here */ > + ret = mfpwm_acquire(pc->pwmf); > + if (ret) > + return ret; > + > + rate = mfpwm_clk_get_rate(pc->pwmf->parent); > + > + ret = rockchip_pwm_v4_round_params(rate, wf->duty_length_ns, > + wf->period_length_ns, > + wf->duty_offset_ns, &wfhw->duty, > + &wfhw->period, &wfhw->offset); > + > + if (wf->period_length_ns > 0) > + wfhw->enable = PWMV4_EN_BOTH_MASK; > + else > + wfhw->enable = 0; > + > + dev_dbg(&chip->dev, "tohw: duty = %u, period = %u, offset = %u, rate %lu\n", > + wfhw->duty, wfhw->period, wfhw->offset, rate); > + > + mfpwm_release(pc->pwmf); > + return ret; This is wrong. If a too high value for (say) period_length_ns is requested, you're supposed to configure the maximal possible period_length and not return a failure. > +} > + > +static int rockchip_pwm_v4_round_wf_fromhw(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const void *_wfhw, > + struct pwm_waveform *wf) > +{ > + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); > + const struct rockchip_pwm_v4_wf *wfhw = _wfhw; > + unsigned long rate; > + int ret = 0; > + > + /* We do not want chosen_clk to change out from under us here */ This is also true while the PWM is enabled. > + ret = mfpwm_acquire(pc->pwmf); > + if (ret) > + return ret; > + > + rate = mfpwm_clk_get_rate(pc->pwmf->parent); Why isn't that a proper clock that you can call clk_get_rate() (and clk_rate_exclusive_get()) for? > + /* Let's avoid a cool division-by-zero if the clock's busted. */ > + if (!rate) { > + ret = -EINVAL; > + goto out_mfpwm_release; > + } > + > + wf->duty_length_ns = mult_frac(wfhw->duty, NSEC_PER_SEC, rate); > + > + if (pwmv4_is_enabled(wfhw->enable)) > + wf->period_length_ns = mult_frac(wfhw->period, NSEC_PER_SEC, > + rate); > + else > + wf->period_length_ns = 0; > + > + wf->duty_offset_ns = mult_frac(wfhw->offset, NSEC_PER_SEC, rate); > + > + dev_dbg(&chip->dev, "fromhw: duty = %llu, period = %llu, offset = %llu\n", > + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns); In my experience it't helpful to include rate in the output here. > +out_mfpwm_release: > + mfpwm_release(pc->pwmf); > + return ret; > +} > + > +static int rockchip_pwm_v4_read_wf(struct pwm_chip *chip, struct pwm_device *pwm, > + void *_wfhw) > +{ > + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); > + struct rockchip_pwm_v4_wf *wfhw = _wfhw; > + int ret = 0; > + > + > + ret = mfpwm_acquire(pc->pwmf); > + if (ret) > + return ret; > + > + wfhw->period = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_PERIOD); > + wfhw->duty = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_DUTY); > + wfhw->offset = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_OFFSET); > + wfhw->enable = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_ENABLE) & PWMV4_EN_BOTH_MASK; > + > + mfpwm_release(pc->pwmf); > + > + return 0; > +} > + > +static int rockchip_pwm_v4_write_wf(struct pwm_chip *chip, struct pwm_device *pwm, > + const void *_wfhw) > +{ > + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); > + const struct rockchip_pwm_v4_wf *wfhw = _wfhw; > + bool was_enabled = false; > + int ret = 0; > + > + ret = mfpwm_acquire(pc->pwmf); > + if (ret) > + return ret; > + > + was_enabled = pwmv4_is_enabled(mfpwm_reg_read(pc->pwmf->base, > + PWMV4_REG_ENABLE)); > + > + /* > + * "But Nicolas", you ask with valid concerns, "why would you enable the > + * PWM before setting all the parameter registers?" Funny, I had this thought alread for mfpwm_acquire() above. Do you also need that if wfhw->enable == 0? > + * Excellent question, Mr. Reader M. Strawman! The RK3576 TRM Part 1 > + * Section 34.6.3 specifies that this is the intended order of writes. > + * Doing the PWM_EN and PWM_CLK_EN writes after the params but before > + * the CTRL_UPDATE_EN, or even after the CTRL_UPDATE_EN, results in > + * erratic behaviour where repeated turning on and off of the PWM may > + * not turn it off under all circumstances. This is also why we don't > + * use relaxed writes; it's not worth the footgun. > + */ > + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE, > + REG_UPDATE_WE(wfhw->enable, 0, 1)); > + > + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_PERIOD, wfhw->period); > + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_DUTY, wfhw->duty); > + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_OFFSET, wfhw->offset); > + > + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, PWMV4_CTRL_CONT_FLAGS); > + > + /* Commit new configuration to hardware output. */ > + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE, > + PWMV4_CTRL_UPDATE_EN(1)); PWMV4_CTRL_UPDATE_EN is always only used with 1 as parameter, so maybe simplify the definition to BIT(2)? > + if (pwmv4_is_enabled(wfhw->enable)) { > + if (!was_enabled) { > + dev_dbg(&chip->dev, "enabling PWM output\n"); > + ret = mfpwm_pwmclk_enable(pc->pwmf); > + if (ret) > + goto err_mfpwm_release; > + > + /* > + * Output should be on now, acquire device to guarantee > + * exclusion with other device functions while it's on. > + */ > + ret = mfpwm_acquire(pc->pwmf); > + if (ret) > + goto err_mfpwm_release; Alternatively to calling mfpwm_acquire once more, you could also skip the mfpwm_release() below. That makes the code a bit more complicated, but also reduces the number of possible failing points. > + } > + } else if (was_enabled) { > + dev_dbg(&chip->dev, "disabling PWM output\n"); > + mfpwm_pwmclk_disable(pc->pwmf); > + /* Output is off now, extra release to balance extra acquire */ > + mfpwm_release(pc->pwmf); > + } > + > +err_mfpwm_release: > + mfpwm_release(pc->pwmf); > + > + return ret; > +} > + > +/* We state the PWM chip is atomic, so none of these functions should sleep. */ > +static const struct pwm_ops rockchip_pwm_v4_ops = { > + .sizeof_wfhw = sizeof(struct rockchip_pwm_v4_wf), > + .round_waveform_tohw = rockchip_pwm_v4_round_wf_tohw, > + .round_waveform_fromhw = rockchip_pwm_v4_round_wf_fromhw, > + .read_waveform = rockchip_pwm_v4_read_wf, > + .write_waveform = rockchip_pwm_v4_write_wf, > +}; > + > +static int rockchip_pwm_v4_probe(struct platform_device *pdev) > +{ > + struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev); > + struct rockchip_pwm_v4 *pc; > + struct pwm_chip *chip; > + int ret; > + > + chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc)); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + > + pc = to_rockchip_pwm_v4(chip); > + pc->pwmf = pwmf; > + > + platform_set_drvdata(pdev, pc); > + > + chip->ops = &rockchip_pwm_v4_ops; > + chip->atomic = true; > + If the PWM is already enabled you better call mfpwm_acquire() here? > + ret = pwmchip_add(chip); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > + > + return 0; > +} > + > +static void rockchip_pwm_v4_remove(struct platform_device *pdev) > +{ > + struct rockchip_pwm_v4 *pc = platform_get_drvdata(pdev); pwmchip_remove()? > + mfpwm_remove_func(pc->pwmf); Maybe make this a devm function? > +} > + > +static const struct platform_device_id rockchip_pwm_v4_ids[] = { > + { .name = "pwm-rockchip-v4", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(platform, rockchip_pwm_v4_ids); > + > +static struct platform_driver rockchip_pwm_v4_driver = { > + .probe = rockchip_pwm_v4_probe, > + .remove = rockchip_pwm_v4_remove, > + .driver = { > + .name = "pwm-rockchip-v4", > + }, > + .id_table = rockchip_pwm_v4_ids, > +}; > +module_platform_driver(rockchip_pwm_v4_driver); > + > +MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>"); > +MODULE_DESCRIPTION("Rockchip PWMv4 Driver"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS("ROCKCHIP_MFPWM"); There are different tastes, but if you ask me that MODULE_IMPORT_NS can go into the header declaring the functions from that namespace. Best regards Uwe
[Dropped Jonas Karlman from Cc as his email address doesn't work.] Hello Nicolas, On Thu, May 22, 2025 at 03:02:29PM +0200, Nicolas Frattaroli wrote: > On Tuesday, 13 May 2025 19:26:31 Central European Summer Time Uwe Kleine-König wrote: > > On Tue, Apr 08, 2025 at 02:32:17PM +0200, Nicolas Frattaroli wrote: > > > The Rockchip RK3576 brings with it a new PWM IP, in downstream code > > > referred to as "v4". This new IP is different enough from the previous > > > Rockchip IP that I felt it necessary to add a new driver for it, instead > > > of shoehorning it in the old one. > > > > > > Add this new driver, based on the PWM core's waveform APIs. Its platform > > > device is registered by the parent mfpwm driver, from which it also > > > receives a little platform data struct, so that mfpwm can guarantee that > > > all the platform device drivers spread across different subsystems for > > > this specific hardware IP do not interfere with each other. > > > > > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/pwm/Kconfig | 13 ++ > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-rockchip-v4.c | 336 ++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 351 insertions(+) > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -20891,6 +20891,7 @@ L: linux-rockchip@lists.infradead.org > > > L: linux-pwm@vger.kernel.org > > > S: Maintained > > > F: Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml > > > +F: drivers/pwm/pwm-rockchip-v4.c > > > F: drivers/soc/rockchip/mfpwm.c > > > F: include/soc/rockchip/mfpwm.h > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > index 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -540,6 +540,19 @@ config PWM_ROCKCHIP > > > Generic PWM framework driver for the PWM controller found on > > > Rockchip SoCs. > > > > > > +config PWM_ROCKCHIP_V4 > > > + tristate "Rockchip PWM v4 support" > > > + depends on ARCH_ROCKCHIP || COMPILE_TEST > > > + depends on ROCKCHIP_MFPWM > > > + depends on HAS_IOMEM > > > + help > > > + Generic PWM framework driver for the PWM controller found on > > > + later Rockchip SoCs such as the RK3576. > > > + > > > + Uses the Rockchip Multi-function PWM controller driver infrastructure > > > + to guarantee fearlessly concurrent operation with other functions of > > > + the same device implemented by drivers in other subsystems. > > > + > > > config PWM_RZ_MTU3 > > > tristate "Renesas RZ/G2L MTU3a PWM Timer support" > > > depends on RZ_MTU3 > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644 > > > --- a/drivers/pwm/Makefile > > > +++ b/drivers/pwm/Makefile > > > @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE) += pwm-raspberrypi-poe.o > > > obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > > > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > > > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > > > +obj-$(CONFIG_PWM_ROCKCHIP_V4) += pwm-rockchip-v4.o > > > obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o > > > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > > > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > > > diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c > > > new file mode 100644 > > > index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e > > > --- /dev/null > > > +++ b/drivers/pwm/pwm-rockchip-v4.c > > > @@ -0,0 +1,336 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > +/* > > > + * Copyright (c) 2025 Collabora Ltd. > > > + * > > > + * A Pulse-Width-Modulation (PWM) generator driver for the generators found in > > > + * Rockchip SoCs such as the RK3576, internally referred to as "PWM v4". Uses > > > + * the MFPWM infrastructure to guarantee exclusive use over the device without > > > + * other functions of the device from different drivers interfering with its > > > + * operation while it's active. > > > > Can you please add a "Limitations" paragraph here similar to the other > > newer drivers that explains how the hardware behave on disable > > (inactive? High-Z? freeze?), if there are glitches possible when > > settings are changed or if the currently running period is completed on > > reconfiguration. > > Will do. Might need a few long hours with the logic analyzer and poking at > the common clock framework to cover all bases. Usually it's simpler. e.g. if you set period=1s,duty=0 and then period=2s,duty=2 an LED is enough to determine if the current period is completed before a change. You don't find High-Z with an LED but can distinguish between "inactive when off" and "freeze when off". The datasheet might know about High-Z. Apropos datasheet: If that is publically available, a reference to it in the driver's header would be awesome. > > > +static int rockchip_pwm_v4_round_wf_tohw(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + const struct pwm_waveform *wf, > > > + void *_wfhw) > > > +{ > > > + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); > > > + struct rockchip_pwm_v4_wf *wfhw = _wfhw; > > > + unsigned long rate; > > > + int ret = 0; > > > + > > > + /* We do not want chosen_clk to change out from under us here */ > > > + ret = mfpwm_acquire(pc->pwmf); > > > + if (ret) > > > + return ret; > > > + > > > + rate = mfpwm_clk_get_rate(pc->pwmf->parent); > > > + > > > + ret = rockchip_pwm_v4_round_params(rate, wf->duty_length_ns, > > > + wf->period_length_ns, > > > + wf->duty_offset_ns, &wfhw->duty, > > > + &wfhw->period, &wfhw->offset); > > > + > > > + if (wf->period_length_ns > 0) > > > + wfhw->enable = PWMV4_EN_BOTH_MASK; > > > + else > > > + wfhw->enable = 0; > > > + > > > + dev_dbg(&chip->dev, "tohw: duty = %u, period = %u, offset = %u, rate %lu\n", > > > + wfhw->duty, wfhw->period, wfhw->offset, rate); > > > + > > > + mfpwm_release(pc->pwmf); > > > + return ret; > > > > This is wrong. If a too high value for (say) period_length_ns is > > requested, you're supposed to configure the maximal possible > > period_length and not return a failure. > > Ack. What if offset > period - duty is requested? Should I just saturate it > to period - duty in that case? If you configure period = 10, duty = offset = 6 the period restart is supposed to happen during the active phase, that is it looks as follows: __ _____ _____ _____ ____ \___/ \___/ \___/ \___/ ^ ^ ^ ^ ^ 01234567890 ('^' marks the period start.) > > > + ret = mfpwm_acquire(pc->pwmf); > > > + if (ret) > > > + return ret; > > > + > > > + rate = mfpwm_clk_get_rate(pc->pwmf->parent); > > > > Why isn't that a proper clock that you can call clk_get_rate() (and > > clk_rate_exclusive_get()) for? > > Because I didn't know clk-mux.c existed :( But even with it, I'm not sure > if letting mfpwm function drivers touch the clk directly is a good idea, > as this either means storing it in their pwmf struct or making the members > of the mfpwm struct part of the shared header. The different drivers shouldn't need to touch the clk directly, the clk API functions should be enough?! > > > + wfhw->period = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_PERIOD); > > > + wfhw->duty = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_DUTY); > > > + wfhw->offset = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_OFFSET); > > > + wfhw->enable = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_ENABLE) & PWMV4_EN_BOTH_MASK; > > > + > > > + mfpwm_release(pc->pwmf); > > > + > > > + return 0; > > > +} > > > + > > > +static int rockchip_pwm_v4_write_wf(struct pwm_chip *chip, struct pwm_device *pwm, > > > + const void *_wfhw) > > > +{ > > > + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); > > > + const struct rockchip_pwm_v4_wf *wfhw = _wfhw; > > > + bool was_enabled = false; > > > + int ret = 0; > > > + > > > + ret = mfpwm_acquire(pc->pwmf); > > > + if (ret) > > > + return ret; > > > + > > > + was_enabled = pwmv4_is_enabled(mfpwm_reg_read(pc->pwmf->base, > > > + PWMV4_REG_ENABLE)); Just noticed: pwmv4_is_enabled has the wrong prefix. Please use "rockchip_pwm_v4" consistently. > > > + /* > > > + * "But Nicolas", you ask with valid concerns, "why would you enable the > > > + * PWM before setting all the parameter registers?" > > > > Funny, I had this thought alread for mfpwm_acquire() above. Do you also > > need that if wfhw->enable == 0? > > The only thing mfpwm_acquire does is check that this driver is the only > one using the hardware, and enabling the bus clk so registers can be > accessed. The clock that the PWM signal is derived from, as well as > enabling and disabling PWM, is a separate step. In this case, we need > to have acquired mfpwm beforehand to do the reg read for was_enabled. ok. > > > + if (pwmv4_is_enabled(wfhw->enable)) { > > > + if (!was_enabled) { > > > + dev_dbg(&chip->dev, "enabling PWM output\n"); > > > + ret = mfpwm_pwmclk_enable(pc->pwmf); > > > + if (ret) > > > + goto err_mfpwm_release; > > > + > > > + /* > > > + * Output should be on now, acquire device to guarantee > > > + * exclusion with other device functions while it's on. > > > + */ > > > + ret = mfpwm_acquire(pc->pwmf); > > > + if (ret) > > > + goto err_mfpwm_release; > > > > Alternatively to calling mfpwm_acquire once more, you could also skip > > the mfpwm_release() below. That makes the code a bit more complicated, > > but also reduces the number of possible failing points. > > I think I did this at some stage but it'd just necessitate duplicating the > if condition at the release point. The else-if branch just down below still > needs to exist since we need to not just balance this function's acquire, > but the fact that we kept it acquired when we turned it on so we need to > release it an extra time when we turn it off. I don't think changing this > to eliminate the additional acquire call has clear benefits here. I'll keep that in mind and will try it maybe myself on top of v2. > > > +static int rockchip_pwm_v4_probe(struct platform_device *pdev) > > > +{ > > > + struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev); > > > + struct rockchip_pwm_v4 *pc; > > > + struct pwm_chip *chip; > > > + int ret; > > > + > > > + chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc)); > > > + if (IS_ERR(chip)) > > > + return PTR_ERR(chip); > > > + > > > + pc = to_rockchip_pwm_v4(chip); > > > + pc->pwmf = pwmf; > > > + > > > + platform_set_drvdata(pdev, pc); > > > + > > > + chip->ops = &rockchip_pwm_v4_ops; > > > + chip->atomic = true; > > > + > > > > If the PWM is already enabled you better call mfpwm_acquire() here? > > As in, if the hardware set the PWM on before the driver probed? I hadn't > considered this case, and will need to think about it. Could very well be > a possibility as u-boot does things before us. The typical application is that the bootloader already shows a splash screen and then you don't want Linux booting result in a flashing display. Best regards Uwe
Hi Uwe, On Friday, 23 May 2025 17:02:34 Central European Summer Time Uwe Kleine-König wrote: > [Dropped Jonas Karlman from Cc as his email address doesn't work.] Strange, I don't think I got any bounces, and your reply still has him in the Cc ;) Just letting you know so that it doesn't look like I re-added him. > Hello Nicolas, > > On Thu, May 22, 2025 at 03:02:29PM +0200, Nicolas Frattaroli wrote: > > On Tuesday, 13 May 2025 19:26:31 Central European Summer Time Uwe Kleine-König wrote: > > > On Tue, Apr 08, 2025 at 02:32:17PM +0200, Nicolas Frattaroli wrote: > > > > The Rockchip RK3576 brings with it a new PWM IP, in downstream code > > > > referred to as "v4". This new IP is different enough from the previous > > > > Rockchip IP that I felt it necessary to add a new driver for it, instead > > > > of shoehorning it in the old one. > > > > > > > > Add this new driver, based on the PWM core's waveform APIs. Its platform > > > > device is registered by the parent mfpwm driver, from which it also > > > > receives a little platform data struct, so that mfpwm can guarantee that > > > > all the platform device drivers spread across different subsystems for > > > > this specific hardware IP do not interfere with each other. > > > > > > > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/pwm/Kconfig | 13 ++ > > > > drivers/pwm/Makefile | 1 + > > > > drivers/pwm/pwm-rockchip-v4.c | 336 ++++++++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 351 insertions(+) > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644 > > > > --- a/MAINTAINERS > > > > +++ b/MAINTAINERS > > > > @@ -20891,6 +20891,7 @@ L: linux-rockchip@lists.infradead.org > > > > L: linux-pwm@vger.kernel.org > > > > S: Maintained > > > > F: Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml > > > > +F: drivers/pwm/pwm-rockchip-v4.c > > > > F: drivers/soc/rockchip/mfpwm.c > > > > F: include/soc/rockchip/mfpwm.h > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > index 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644 > > > > --- a/drivers/pwm/Kconfig > > > > +++ b/drivers/pwm/Kconfig > > > > @@ -540,6 +540,19 @@ config PWM_ROCKCHIP > > > > Generic PWM framework driver for the PWM controller found on > > > > Rockchip SoCs. > > > > > > > > +config PWM_ROCKCHIP_V4 > > > > + tristate "Rockchip PWM v4 support" > > > > + depends on ARCH_ROCKCHIP || COMPILE_TEST > > > > + depends on ROCKCHIP_MFPWM > > > > + depends on HAS_IOMEM > > > > + help > > > > + Generic PWM framework driver for the PWM controller found on > > > > + later Rockchip SoCs such as the RK3576. > > > > + > > > > + Uses the Rockchip Multi-function PWM controller driver infrastructure > > > > + to guarantee fearlessly concurrent operation with other functions of > > > > + the same device implemented by drivers in other subsystems. > > > > + > > > > config PWM_RZ_MTU3 > > > > tristate "Renesas RZ/G2L MTU3a PWM Timer support" > > > > depends on RZ_MTU3 > > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > > index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644 > > > > --- a/drivers/pwm/Makefile > > > > +++ b/drivers/pwm/Makefile > > > > @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE) += pwm-raspberrypi-poe.o > > > > obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > > > > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > > > > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > > > > +obj-$(CONFIG_PWM_ROCKCHIP_V4) += pwm-rockchip-v4.o > > > > obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o > > > > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > > > > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > > > > diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c > > > > new file mode 100644 > > > > index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e > > > > --- /dev/null > > > > +++ b/drivers/pwm/pwm-rockchip-v4.c > > > > @@ -0,0 +1,336 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > > +/* > > > > + * Copyright (c) 2025 Collabora Ltd. > > > > + * > > > > + * A Pulse-Width-Modulation (PWM) generator driver for the generators found in > > > > + * Rockchip SoCs such as the RK3576, internally referred to as "PWM v4". Uses > > > > + * the MFPWM infrastructure to guarantee exclusive use over the device without > > > > + * other functions of the device from different drivers interfering with its > > > > + * operation while it's active. > > > > > > Can you please add a "Limitations" paragraph here similar to the other > > > newer drivers that explains how the hardware behave on disable > > > (inactive? High-Z? freeze?), if there are glitches possible when > > > settings are changed or if the currently running period is completed on > > > reconfiguration. > > > > Will do. Might need a few long hours with the logic analyzer and poking at > > the common clock framework to cover all bases. > > Usually it's simpler. e.g. if you set period=1s,duty=0 and then > period=2s,duty=2 an LED is enough to determine if the current period is > completed before a change. > > You don't find High-Z with an LED but can distinguish between "inactive > when off" and "freeze when off". The datasheet might know about High-Z. I've used a logic analyzer to quickly determine this, it went fairly smoothly and didn't take long at all. The PWM output stops immediately and freezes in whatever state it was in at that point in time, i.e. stays either low or high. Changes to period/duty/offset params only seem to take effect at the start of the next period, so changing them should be glitch-free. I will add a full limitations paragraph, including a TODO for the future with an idea for how to change the driver so that it lets the current period complete when turning the PWM off. I'm not implementing it right away in this series because that'd involve adding some IRQ handlers to this driver as well and the series is big enough as it is already :) > Apropos datasheet: If that is publically available, a reference to it in > the driver's header would be awesome. I've noted that down as a thing to ask Rockchip. Currently, the technical reference manual of this SoC unfortunately is not publicly available, at least not in an official capacity. I'll mention it'd also be sufficient if we had just the PWM section of that TRM so that I can link to it. > > > > +static int rockchip_pwm_v4_round_wf_tohw(struct pwm_chip *chip, > > > > + struct pwm_device *pwm, > > > > + const struct pwm_waveform *wf, > > > > + void *_wfhw) > > > > +{ > > > > + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); > > > > + struct rockchip_pwm_v4_wf *wfhw = _wfhw; > > > > + unsigned long rate; > > > > + int ret = 0; > > > > + > > > > + /* We do not want chosen_clk to change out from under us here */ > > > > + ret = mfpwm_acquire(pc->pwmf); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + rate = mfpwm_clk_get_rate(pc->pwmf->parent); > > > > + > > > > + ret = rockchip_pwm_v4_round_params(rate, wf->duty_length_ns, > > > > + wf->period_length_ns, > > > > + wf->duty_offset_ns, &wfhw->duty, > > > > + &wfhw->period, &wfhw->offset); > > > > + > > > > + if (wf->period_length_ns > 0) > > > > + wfhw->enable = PWMV4_EN_BOTH_MASK; > > > > + else > > > > + wfhw->enable = 0; > > > > + > > > > + dev_dbg(&chip->dev, "tohw: duty = %u, period = %u, offset = %u, rate %lu\n", > > > > + wfhw->duty, wfhw->period, wfhw->offset, rate); > > > > + > > > > + mfpwm_release(pc->pwmf); > > > > + return ret; > > > > > > This is wrong. If a too high value for (say) period_length_ns is > > > requested, you're supposed to configure the maximal possible > > > period_length and not return a failure. > > > > Ack. What if offset > period - duty is requested? Should I just saturate it > > to period - duty in that case? > > If you configure period = 10, duty = offset = 6 the period restart is > supposed to happen during the active phase, that is it looks as follows: > > __ _____ _____ _____ ____ > \___/ \___/ \___/ \___/ > ^ ^ ^ ^ ^ > 01234567890 > > ('^' marks the period start.) Okay, this might make this a bit more complicated then. The hardware in the TRM at least states for the offset register The value ranges from 0 to (period-duty) which I think means that I have to actually make use of the hardware's polarity setting, set it to inverse polarity, and then set the offset to duty and the duty to period - duty if I understand this right. Offset right now is just used by the pwm core to do inversion, right? As in there's no handy sysfs knob I can shove values into to set it to an arbitrary number? I may also just shove a value above (period - duty) into the offset reg to see if the hardware already behaves in the expected way and doing the math manually would be overcomplicating things. > > > > + ret = mfpwm_acquire(pc->pwmf); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + rate = mfpwm_clk_get_rate(pc->pwmf->parent); > > > > > > Why isn't that a proper clock that you can call clk_get_rate() (and > > > clk_rate_exclusive_get()) for? > > > > Because I didn't know clk-mux.c existed :( But even with it, I'm not sure > > if letting mfpwm function drivers touch the clk directly is a good idea, > > as this either means storing it in their pwmf struct or making the members > > of the mfpwm struct part of the shared header. > > The different drivers shouldn't need to touch the clk directly, the clk > API functions should be enough?! It's not just enough, it's too much. I don't want to give every pwmf instance a pointer to the clock mux and then let the function drivers call every common clock framework function on it that they wish to call. I'll think about it more. clk_rate_exclusive_get/_put should protect against the kinds of cross-function-driver interference hijinks I'm worried about, so maybe the indirection isn't needed. > > > > + wfhw->period = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_PERIOD); > > > > + wfhw->duty = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_DUTY); > > > > + wfhw->offset = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_OFFSET); > > > > + wfhw->enable = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_ENABLE) & PWMV4_EN_BOTH_MASK; > > > > + > > > > + mfpwm_release(pc->pwmf); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int rockchip_pwm_v4_write_wf(struct pwm_chip *chip, struct pwm_device *pwm, > > > > + const void *_wfhw) > > > > +{ > > > > + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); > > > > + const struct rockchip_pwm_v4_wf *wfhw = _wfhw; > > > > + bool was_enabled = false; > > > > + int ret = 0; > > > > + > > > > + ret = mfpwm_acquire(pc->pwmf); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + was_enabled = pwmv4_is_enabled(mfpwm_reg_read(pc->pwmf->base, > > > > + PWMV4_REG_ENABLE)); > > Just noticed: pwmv4_is_enabled has the wrong prefix. Please use > "rockchip_pwm_v4" consistently. Will do. > [...] > > > > > +static int rockchip_pwm_v4_probe(struct platform_device *pdev) > > > > +{ > > > > + struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev); > > > > + struct rockchip_pwm_v4 *pc; > > > > + struct pwm_chip *chip; > > > > + int ret; > > > > + > > > > + chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc)); > > > > + if (IS_ERR(chip)) > > > > + return PTR_ERR(chip); > > > > + > > > > + pc = to_rockchip_pwm_v4(chip); > > > > + pc->pwmf = pwmf; > > > > + > > > > + platform_set_drvdata(pdev, pc); > > > > + > > > > + chip->ops = &rockchip_pwm_v4_ops; > > > > + chip->atomic = true; > > > > + > > > > > > If the PWM is already enabled you better call mfpwm_acquire() here? > > > > As in, if the hardware set the PWM on before the driver probed? I hadn't > > considered this case, and will need to think about it. Could very well be > > a possibility as u-boot does things before us. > > The typical application is that the bootloader already shows a splash > screen and then you don't want Linux booting result in a flashing > display. Gotcha, that does sound fairly important and I've implemented it for v2 now. Managed to successfully test it with some manual register writes from u-boot. Haven't really decided yet whether I'll send v2 out soon or wait for -rc1 to release to base it against. I'm currently leaning towards sending it out before -rc1 just because I don't want to rob reviewers of up to two additional weeks of potential review time, especially since v2 is already substantially different based on the changes I've staged for it so far. > > Best regards > Uwe Kind regards, Nicolas Frattaroli
diff --git a/MAINTAINERS b/MAINTAINERS index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20891,6 +20891,7 @@ L: linux-rockchip@lists.infradead.org L: linux-pwm@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml +F: drivers/pwm/pwm-rockchip-v4.c F: drivers/soc/rockchip/mfpwm.c F: include/soc/rockchip/mfpwm.h diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -540,6 +540,19 @@ config PWM_ROCKCHIP Generic PWM framework driver for the PWM controller found on Rockchip SoCs. +config PWM_ROCKCHIP_V4 + tristate "Rockchip PWM v4 support" + depends on ARCH_ROCKCHIP || COMPILE_TEST + depends on ROCKCHIP_MFPWM + depends on HAS_IOMEM + help + Generic PWM framework driver for the PWM controller found on + later Rockchip SoCs such as the RK3576. + + Uses the Rockchip Multi-function PWM controller driver infrastructure + to guarantee fearlessly concurrent operation with other functions of + the same device implemented by drivers in other subsystems. + config PWM_RZ_MTU3 tristate "Renesas RZ/G2L MTU3a PWM Timer support" depends on RZ_MTU3 diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE) += pwm-raspberrypi-poe.o obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o +obj-$(CONFIG_PWM_ROCKCHIP_V4) += pwm-rockchip-v4.o obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c new file mode 100644 index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e --- /dev/null +++ b/drivers/pwm/pwm-rockchip-v4.c @@ -0,0 +1,336 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2025 Collabora Ltd. + * + * A Pulse-Width-Modulation (PWM) generator driver for the generators found in + * Rockchip SoCs such as the RK3576, internally referred to as "PWM v4". Uses + * the MFPWM infrastructure to guarantee exclusive use over the device without + * other functions of the device from different drivers interfering with its + * operation while it's active. + * + * Authors: + * Nicolas Frattaroli <nicolas.frattaroli@collabora.com> + */ + +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <soc/rockchip/mfpwm.h> + +struct rockchip_pwm_v4 { + struct rockchip_mfpwm_func *pwmf; + struct pwm_chip chip; +}; + +struct rockchip_pwm_v4_wf { + u32 period; + u32 duty; + u32 offset; + u8 enable; +}; + +static inline struct rockchip_pwm_v4 *to_rockchip_pwm_v4(struct pwm_chip *chip) +{ + return pwmchip_get_drvdata(chip); +} + +/** + * rockchip_pwm_v4_round_single - convert a PWM parameter to hardware + * @rate: clock rate of the PWM clock, as per clk_get_rate + * @in_val: parameter in nanoseconds to convert + * @out_val: pointer to location where converted result should be stored. + * + * If @out_val is %NULL, no calculation is performed. + * + * Return: + * * %0 - Success + * * %-EOVERFLOW - Result too large for target type + */ +static int rockchip_pwm_v4_round_single(unsigned long rate, u64 in_val, + u32 *out_val) +{ + u64 tmp; + + if (!out_val) + return 0; + + tmp = mult_frac(rate, in_val, NSEC_PER_SEC); + if (tmp > U32_MAX) + return -EOVERFLOW; + + *out_val = tmp; + + return 0; +} + +/** + * rockchip_pwm_v4_round_params - convert PWM parameters to hardware + * @rate: PWM clock rate to do the calculations at + * @duty: PWM duty cycle in nanoseconds + * @period: PWM period in nanoseconds + * @offset: PWM offset in nanoseconds + * @out_duty: pointer to where the rounded duty value should be stored + * if NULL, don't calculate or store it + * @out_period: pointer to where the rounded period value should be stored + * if NULL, don't calculate or store it + * @out_offset: pointer to where the rounded offset value should be stored + * if NULL, don't calculate or store it + * + * Convert nanosecond-based duty/period/offset parameters to the PWM hardware's + * native rounded representation in number of cycles at clock rate @rate. If an + * out_ parameter is a NULL pointer, the corresponding parameter will not be + * calculated or stored. Should an overflow error occur for any of the + * parameters, assume the data at all the out_ locations is invalid and may not + * even have been touched at all. + * + * Return: + * * %0 - Success + * * %-EOVERFLOW - One of the results is too large for the PWM hardware + */ +static int rockchip_pwm_v4_round_params(unsigned long rate, u64 duty, + u64 period, u64 offset, u32 *out_duty, + u32 *out_period, u32 *out_offset) +{ + int ret; + + ret = rockchip_pwm_v4_round_single(rate, duty, out_duty); + if (ret) + return ret; + + ret = rockchip_pwm_v4_round_single(rate, period, out_period); + if (ret) + return ret; + + ret = rockchip_pwm_v4_round_single(rate, offset, out_offset); + if (ret) + return ret; + + return 0; +} + +static int rockchip_pwm_v4_round_wf_tohw(struct pwm_chip *chip, + struct pwm_device *pwm, + const struct pwm_waveform *wf, + void *_wfhw) +{ + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); + struct rockchip_pwm_v4_wf *wfhw = _wfhw; + unsigned long rate; + int ret = 0; + + /* We do not want chosen_clk to change out from under us here */ + ret = mfpwm_acquire(pc->pwmf); + if (ret) + return ret; + + rate = mfpwm_clk_get_rate(pc->pwmf->parent); + + ret = rockchip_pwm_v4_round_params(rate, wf->duty_length_ns, + wf->period_length_ns, + wf->duty_offset_ns, &wfhw->duty, + &wfhw->period, &wfhw->offset); + + if (wf->period_length_ns > 0) + wfhw->enable = PWMV4_EN_BOTH_MASK; + else + wfhw->enable = 0; + + dev_dbg(&chip->dev, "tohw: duty = %u, period = %u, offset = %u, rate %lu\n", + wfhw->duty, wfhw->period, wfhw->offset, rate); + + mfpwm_release(pc->pwmf); + return ret; +} + +static int rockchip_pwm_v4_round_wf_fromhw(struct pwm_chip *chip, + struct pwm_device *pwm, + const void *_wfhw, + struct pwm_waveform *wf) +{ + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); + const struct rockchip_pwm_v4_wf *wfhw = _wfhw; + unsigned long rate; + int ret = 0; + + /* We do not want chosen_clk to change out from under us here */ + ret = mfpwm_acquire(pc->pwmf); + if (ret) + return ret; + + rate = mfpwm_clk_get_rate(pc->pwmf->parent); + + /* Let's avoid a cool division-by-zero if the clock's busted. */ + if (!rate) { + ret = -EINVAL; + goto out_mfpwm_release; + } + + wf->duty_length_ns = mult_frac(wfhw->duty, NSEC_PER_SEC, rate); + + if (pwmv4_is_enabled(wfhw->enable)) + wf->period_length_ns = mult_frac(wfhw->period, NSEC_PER_SEC, + rate); + else + wf->period_length_ns = 0; + + wf->duty_offset_ns = mult_frac(wfhw->offset, NSEC_PER_SEC, rate); + + dev_dbg(&chip->dev, "fromhw: duty = %llu, period = %llu, offset = %llu\n", + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns); + +out_mfpwm_release: + mfpwm_release(pc->pwmf); + return ret; +} + +static int rockchip_pwm_v4_read_wf(struct pwm_chip *chip, struct pwm_device *pwm, + void *_wfhw) +{ + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); + struct rockchip_pwm_v4_wf *wfhw = _wfhw; + int ret = 0; + + + ret = mfpwm_acquire(pc->pwmf); + if (ret) + return ret; + + wfhw->period = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_PERIOD); + wfhw->duty = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_DUTY); + wfhw->offset = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_OFFSET); + wfhw->enable = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_ENABLE) & PWMV4_EN_BOTH_MASK; + + mfpwm_release(pc->pwmf); + + return 0; +} + +static int rockchip_pwm_v4_write_wf(struct pwm_chip *chip, struct pwm_device *pwm, + const void *_wfhw) +{ + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); + const struct rockchip_pwm_v4_wf *wfhw = _wfhw; + bool was_enabled = false; + int ret = 0; + + ret = mfpwm_acquire(pc->pwmf); + if (ret) + return ret; + + was_enabled = pwmv4_is_enabled(mfpwm_reg_read(pc->pwmf->base, + PWMV4_REG_ENABLE)); + + /* + * "But Nicolas", you ask with valid concerns, "why would you enable the + * PWM before setting all the parameter registers?" + * + * Excellent question, Mr. Reader M. Strawman! The RK3576 TRM Part 1 + * Section 34.6.3 specifies that this is the intended order of writes. + * Doing the PWM_EN and PWM_CLK_EN writes after the params but before + * the CTRL_UPDATE_EN, or even after the CTRL_UPDATE_EN, results in + * erratic behaviour where repeated turning on and off of the PWM may + * not turn it off under all circumstances. This is also why we don't + * use relaxed writes; it's not worth the footgun. + */ + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE, + REG_UPDATE_WE(wfhw->enable, 0, 1)); + + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_PERIOD, wfhw->period); + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_DUTY, wfhw->duty); + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_OFFSET, wfhw->offset); + + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, PWMV4_CTRL_CONT_FLAGS); + + /* Commit new configuration to hardware output. */ + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE, + PWMV4_CTRL_UPDATE_EN(1)); + + if (pwmv4_is_enabled(wfhw->enable)) { + if (!was_enabled) { + dev_dbg(&chip->dev, "enabling PWM output\n"); + ret = mfpwm_pwmclk_enable(pc->pwmf); + if (ret) + goto err_mfpwm_release; + + /* + * Output should be on now, acquire device to guarantee + * exclusion with other device functions while it's on. + */ + ret = mfpwm_acquire(pc->pwmf); + if (ret) + goto err_mfpwm_release; + } + } else if (was_enabled) { + dev_dbg(&chip->dev, "disabling PWM output\n"); + mfpwm_pwmclk_disable(pc->pwmf); + /* Output is off now, extra release to balance extra acquire */ + mfpwm_release(pc->pwmf); + } + +err_mfpwm_release: + mfpwm_release(pc->pwmf); + + return ret; +} + +/* We state the PWM chip is atomic, so none of these functions should sleep. */ +static const struct pwm_ops rockchip_pwm_v4_ops = { + .sizeof_wfhw = sizeof(struct rockchip_pwm_v4_wf), + .round_waveform_tohw = rockchip_pwm_v4_round_wf_tohw, + .round_waveform_fromhw = rockchip_pwm_v4_round_wf_fromhw, + .read_waveform = rockchip_pwm_v4_read_wf, + .write_waveform = rockchip_pwm_v4_write_wf, +}; + +static int rockchip_pwm_v4_probe(struct platform_device *pdev) +{ + struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev); + struct rockchip_pwm_v4 *pc; + struct pwm_chip *chip; + int ret; + + chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc)); + if (IS_ERR(chip)) + return PTR_ERR(chip); + + pc = to_rockchip_pwm_v4(chip); + pc->pwmf = pwmf; + + platform_set_drvdata(pdev, pc); + + chip->ops = &rockchip_pwm_v4_ops; + chip->atomic = true; + + ret = pwmchip_add(chip); + if (ret) + return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); + + return 0; +} + +static void rockchip_pwm_v4_remove(struct platform_device *pdev) +{ + struct rockchip_pwm_v4 *pc = platform_get_drvdata(pdev); + + mfpwm_remove_func(pc->pwmf); +} + +static const struct platform_device_id rockchip_pwm_v4_ids[] = { + { .name = "pwm-rockchip-v4", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(platform, rockchip_pwm_v4_ids); + +static struct platform_driver rockchip_pwm_v4_driver = { + .probe = rockchip_pwm_v4_probe, + .remove = rockchip_pwm_v4_remove, + .driver = { + .name = "pwm-rockchip-v4", + }, + .id_table = rockchip_pwm_v4_ids, +}; +module_platform_driver(rockchip_pwm_v4_driver); + +MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>"); +MODULE_DESCRIPTION("Rockchip PWMv4 Driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS("ROCKCHIP_MFPWM");
The Rockchip RK3576 brings with it a new PWM IP, in downstream code referred to as "v4". This new IP is different enough from the previous Rockchip IP that I felt it necessary to add a new driver for it, instead of shoehorning it in the old one. Add this new driver, based on the PWM core's waveform APIs. Its platform device is registered by the parent mfpwm driver, from which it also receives a little platform data struct, so that mfpwm can guarantee that all the platform device drivers spread across different subsystems for this specific hardware IP do not interfere with each other. Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> --- MAINTAINERS | 1 + drivers/pwm/Kconfig | 13 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-rockchip-v4.c | 336 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 351 insertions(+)