| Message ID | 20230202165732.305650-7-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add RZ/G2L MTU3a Core, Counter and pwm driver | expand |
Hello, I'm working on my review backlog, sorry that it took so long. On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > Add support for RZ/G2L MTU3a PWM driver. The IP supports > following PWM modes > > 1) PWM mode{1,2} > 2) Reset-synchronized PWM mode > 3) Complementary PWM mode{1,2,3} It's unclear to me what "PWM mode1" and the other modes are. I suspect this is some chip specific naming that isn't understandable for outsiders? Would be great to explain that a bit more. > This patch adds basic pwm mode 1 support for RZ/G2L MTU3a pwm driver > by creating separate logical channels for each IOs. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v11->v12: > * Updated header file to <linux/mfd/rz-mtu3.h> as core driver is in MFD. > * Reordered get_state() > v10->v11: > * No change. > v9->v10: > * No change. > v8->v9: > * Added prescale/duty_cycle variables to struct rz_mtu3_pwm_chip and > cached this values in rz_mtu3_pwm_config and used this cached values > in get_state(), if PWM is disabled. > * Added return code for get_state() > v7->v8: > * Simplified rz_mtu3_pwm_request by calling rz_mtu3_request_channel() > * Simplified rz_mtu3_pwm_free by calling rz_mtu3_release_channel() > v6->v7: > * Added channel specific mutex lock to avoid race between counter > device and rz_mtu3_pwm_{request,free} > * Added pm_runtime_resume_and_get in rz_mtu3_pwm_enable() > * Added pm_runtime_put_sync in rz_mtu3_pwm_disable() > * Updated rz_mtu3_pwm_config() > * Updated rz_mtu3_pwm_apply() > v5->v6: > * Updated commit and Kconfig description > * Sorted the header > * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable() > * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed > __maybe_unused from suspend/resume() > v4->v5: > * pwm device is instantiated by mtu3a core driver. > v3->v4: > * There is no resource associated with "rz-mtu3-pwm" compatible > and moved the code to mfd subsystem as it binds against "rz-mtu". > * Removed struct platform_driver rz_mtu3_pwm_driver. > v2->v3: > * No change. > v1->v2: > * Modelled as a single PWM device handling multiple channles. > * Used PM framework to manage the clocks. > --- > drivers/pwm/Kconfig | 11 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-rz-mtu3.c | 485 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 497 insertions(+) > create mode 100644 drivers/pwm/pwm-rz-mtu3.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 31cdc9dae3c5..c54cbeabe093 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -492,6 +492,17 @@ config PWM_ROCKCHIP > Generic PWM framework driver for the PWM controller found on > Rockchip SoCs. > > +config PWM_RZ_MTU3 > + tristate "Renesas RZ/G2L MTU3a PWM Timer support" > + depends on RZ_MTU3 || COMPILE_TEST > + depends on HAS_IOMEM > + help > + This driver exposes the MTU3a PWM Timer controller found in Renesas > + RZ/G2L like chips through the PWM API. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-rz-mtu3. > + > config PWM_SAMSUNG > tristate "Samsung PWM support" > depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index a95aabae9115..6b75c0145336 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -45,6 +45,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > obj-$(CONFIG_PWM_RZV2M) += pwm-rzv2m.o > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > +obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o > diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c > new file mode 100644 > index 000000000000..d94e3fc36dfb > --- /dev/null > +++ b/drivers/pwm/pwm-rz-mtu3.c > @@ -0,0 +1,485 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Renesas RZ/G2L MTU3a PWM Timer driver > + * > + * Copyright (C) 2022 Renesas Electronics Corporation > + * > + * Hardware manual for this IP can be found here > + * https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?language=en > + * > + * Limitations: > + * - When PWM is disabled, the output is driven to Hi-Z. > + * - While the hardware supports both polarities, the driver (for now) > + * only handles normal polarity. > + * - While the hardware supports pwm mode{1,2}, reset-synchronized pwm and > + * complementary pwm modes, the driver (for now) only handles pwm mode1. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/limits.h> > +#include <linux/mfd/rz-mtu3.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/pwm.h> > +#include <linux/time.h> > + > +#define RZ_MTU3_TMDR1_MD_NORMAL (0) > +#define RZ_MTU3_TMDR1_MD_PWM_MODE_1 (2) IMHO it would make sense to put these definitions to where RZ_MTU3_TMDR1 is defined. And I'd do it like this: * Timer mode register 1 */ #define RZ_MTU3_TMDR1 5 #define RZ_MTU3_TMDR1_MD GENMASK(3, 0) #define RZ_MTU3_TMDR1_MD_NORMAL FIELD_PREP(RZ_MTU3_TMDR1_MD, 0) #define RZ_MTU3_TMDR1_MD_PWMMODE1 FIELD_PREP(RZ_MTU3_TMDR1_MD, 2) > +#define RZ_MTU3_TIOR_OC_RETAIN (0) > +#define RZ_MTU3_TIOR_OC_0_H_COMP_MATCH (2) > +#define RZ_MTU3_TIOR_OC_1_TOGGLE (7) > +#define RZ_MTU3_TIOR_OC_IOA GENMASK(3, 0) > + > +#define RZ_MTU3_TCR_CCLR_TGRC (5 << 5) > +#define RZ_MTU3_TCR_CKEG_RISING (0 << 3) > + > +#define RZ_MTU3_TCR_TPCS GENMASK(2, 0) > + > +#define RZ_MTU3_MAX_PWM_MODE1_CHANNELS (12) > + > +#define RZ_MTU3_MAX_HW_PWM_CHANNELS (7) > + > +static const u8 rz_mtu3_pwm_mode1_num_ios[] = { 2, 1, 1, 2, 2, 2, 2 }; > + > +/** > + * struct rz_mtu3_pwm_chip - MTU3 pwm private data > + * > + * @chip: MTU3 pwm chip data > + * @clk: MTU3 module clock > + * @lock: Lock to prevent concurrent access for usage count > + * @rate: MTU3 clock rate > + * @user_count: MTU3 usage count > + * @rz_mtu3_channel: HW channels for the PWM > + */ > + > +struct rz_mtu3_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + struct mutex lock; > + unsigned long rate; > + u32 user_count[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > + struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > + > + /* > + * The driver cannot read the current duty cycle/prescale from the > + * hardware if the hardware is disabled. Cache the last programmed > + * duty cycle/prescale value to return in that case. If the hardware is disabled, just doing .enabled = false in .get_state is fine and easier. So this can be dropped I think. > + */ > + u8 prescale[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > + unsigned int duty_cycle[RZ_MTU3_MAX_PWM_MODE1_CHANNELS]; > +}; > + > +static inline struct rz_mtu3_pwm_chip *to_rz_mtu3_pwm_chip(struct pwm_chip *chip) > +{ > + return container_of(chip, struct rz_mtu3_pwm_chip, chip); > +} > + > +static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip *rz_mtu3, > + u64 period_cycles) > +{ > + u32 prescaled_period_cycles; > + u8 prescale; > + > + prescaled_period_cycles = period_cycles >> 16; > + if (prescaled_period_cycles >= 16) > + prescale = 3; > + else > + prescale = (fls(prescaled_period_cycles) + 1) / 2; > + > + return prescale; > +} > + > +static struct rz_mtu3_channel * > +rz_mtu3_get_hw_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 channel) > +{ > + unsigned int i, ch_index = 0; > + > + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { > + ch_index += rz_mtu3_pwm_mode1_num_ios[i]; > + > + if (ch_index > channel) > + break; > + } > + > + return rz_mtu3_pwm->ch[i]; > +} > + > +static u32 rz_mtu3_get_hw_channel_index(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, > + struct rz_mtu3_channel *ch) > +{ > + u32 i; > + > + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { > + if (ch == rz_mtu3_pwm->ch[i]) > + break; > + } > + > + return i; > +} > + > +static bool rz_mtu3_pwm_is_second_channel(u32 ch_index, u32 hwpwm) > +{ > + u32 i, pwm_ch_index = 0; > + > + for (i = 0; i < ch_index; i++) > + pwm_ch_index += rz_mtu3_pwm_mode1_num_ios[i]; > + > + return pwm_ch_index != hwpwm; > +} I don't understand that channel allocation, maybe worth an explaining comment?! > +static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, > + u32 hwpwm) > +{ > + struct rz_mtu3_channel *ch; > + bool is_channel_en; > + u32 ch_index; > + u8 val; > + > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, hwpwm); > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + is_channel_en = rz_mtu3_is_enabled(ch); > + > + if (rz_mtu3_pwm_is_second_channel(ch_index, hwpwm)) > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORL); > + else > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORH); > + > + return (is_channel_en && (val & RZ_MTU3_TIOR_OC_IOA)); > +} > + > +static int rz_mtu3_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > + struct rz_mtu3_channel *ch; > + u32 ch_index; > + > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + if (!rz_mtu3_pwm->user_count[ch_index] && !rz_mtu3_request_channel(ch)) > + return -EBUSY; > + > + mutex_lock(&rz_mtu3_pwm->lock); > + rz_mtu3_pwm->user_count[ch_index]++; > + mutex_unlock(&rz_mtu3_pwm->lock); The lock must protect the check, too, otherwise that's racy. > + > + return 0; > +} > + > +static void rz_mtu3_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > + struct rz_mtu3_channel *ch; > + u32 ch_index; > + > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + > + mutex_lock(&rz_mtu3_pwm->lock); > + rz_mtu3_pwm->user_count[ch_index]--; > + mutex_unlock(&rz_mtu3_pwm->lock); > + > + if (!rz_mtu3_pwm->user_count[ch_index]) > + rz_mtu3_release_channel(ch); I didn't check what rz_mtu3_release_channel() does, but I wonder what happens if another thread calls rz_mtu3_pwm_request for the same channel just after the if check. > +} > + > +static int rz_mtu3_pwm_enable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, > + struct pwm_device *pwm) > +{ > + struct rz_mtu3_channel *ch; > + u32 ch_index; > + u8 val; > + int rc; > + > + rc = pm_runtime_resume_and_get(rz_mtu3_pwm->chip.dev); > + if (rc) > + return rc; > + > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + val = (RZ_MTU3_TIOR_OC_1_TOGGLE << 4) | RZ_MTU3_TIOR_OC_0_H_COMP_MATCH; > + > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_PWM_MODE_1); > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, val); > + else > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, val); > + > + if (rz_mtu3_pwm->user_count[ch_index] <= 1) > + rz_mtu3_enable(ch); > + > + return 0; > +} > + > +static void rz_mtu3_pwm_disable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, > + struct pwm_device *pwm) > +{ > + struct rz_mtu3_channel *ch; > + u32 ch_index; > + > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + > + /* Return to normal mode and disable output pins of MTU3 channel */ > + if (rz_mtu3_pwm->user_count[ch_index] <= 1) > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_NORMAL); > + > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, RZ_MTU3_TIOR_OC_RETAIN); > + else > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, RZ_MTU3_TIOR_OC_RETAIN); > + > + if (rz_mtu3_pwm->user_count[ch_index] <= 1) > + rz_mtu3_disable(ch); > + > + pm_runtime_put_sync(rz_mtu3_pwm->chip.dev); > +} > + > +static int rz_mtu3_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > + struct rz_mtu3_channel *ch; > + u8 prescale, val; > + u32 ch_index; > + u16 dc, pv; > + u64 tmp; > + > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + pm_runtime_get_sync(chip->dev); > + state->enabled = rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, pwm->hwpwm); > + if (state->enabled) { > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TCR); > + prescale = FIELD_GET(RZ_MTU3_TCR_TPCS, val); > + > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { > + dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRD); > + pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRC); > + } else { > + dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRB); > + pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRA); > + } > + Add a comment like: /* With prescale <= 7 and pv <= 0xffff this doesn't overflow. */ > + tmp = NSEC_PER_SEC * (u64)pv << (2 * prescale); > + state->period = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate); > + } else { > + /* If the PWM is disabled, use the cached value. */ > + prescale = rz_mtu3_pwm->prescale[ch_index]; > + dc = rz_mtu3_pwm->duty_cycle[pwm->hwpwm]; > + } > + > + tmp = NSEC_PER_SEC * (u64)dc << (2 * prescale); > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate); > + state->polarity = PWM_POLARITY_NORMAL; > + pm_runtime_put(chip->dev); Can it happen that dc > pv? I assume this implements a 100% relative duty then. Please set .duty_cycle = .period in this case. > + > + return 0; > +} > + > +static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > + struct rz_mtu3_channel *ch; > + unsigned long pv, dc; > + u64 period_cycles; > + u64 duty_cycles; > + u32 ch_index; > + u8 prescale; > + int err; > + u8 val; > + > + /* > + * Refuse clk rates > 1 GHz to prevent overflowing the following > + * calculation. > + */ > + if (rz_mtu3_pwm->rate > NSEC_PER_SEC) > + return -EINVAL; Maybe refuse this case in .probe() already? > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate, > + NSEC_PER_SEC); > + prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm, period_cycles); > + > + if (period_cycles >> (2 * prescale) <= U16_MAX) > + pv = period_cycles >> (2 * prescale); > + else > + pv = U16_MAX; > + > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rz_mtu3_pwm->rate, > + NSEC_PER_SEC); > + if (duty_cycles >> (2 * prescale) <= U16_MAX) > + dc = duty_cycles >> (2 * prescale); > + else > + dc = U16_MAX; > + > + /* > + * Store the duty cycle/prescale for future reference in cases where the > + * corresponding registers can't be read (i.e. when the PWM is disabled). > + */ > + rz_mtu3_pwm->prescale[ch_index] = prescale; > + rz_mtu3_pwm->duty_cycle[pwm->hwpwm] = dc; Above I suggested to drop this, but if you don't: This is broken. rz_mtu3_pwm_config is only ever called with .enable = 1 and the values are not updated when .apply() is called with .enable = 0, so you're investing some effort to report an outdated value that is ignored in the end. > + /* > + * If the PWM channel is disabled, make sure to turn on the clock > + * before writing the register. > + */ > + if (!pwm->state.enabled) { > + err = pm_runtime_resume_and_get(chip->dev); > + if (err) > + return err; > + } Maybe it's easier to call pm_runtime_resume_and_get() unconditionally? > + val = RZ_MTU3_TCR_CKEG_RISING | prescale; > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > + RZ_MTU3_TCR_CCLR_TGRC | val); > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRD, dc); > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRC, pv); > + } else { > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > + RZ_MTU3_TCR_CCLR_TGRA | val); > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRB, dc); > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRA, pv); > + } > + > + /* If the PWM is not enabled, turn the clock off again to save power. */ > + if (!pwm->state.enabled) > + pm_runtime_put(chip->dev); > + > + return 0; > +} > + > +static int rz_mtu3_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > + bool enabled = pwm->state.enabled; > + int ret; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + if (!state->enabled) { > + if (enabled) > + rz_mtu3_pwm_disable(rz_mtu3_pwm, pwm); > + > + return 0; > + } > + > + ret = rz_mtu3_pwm_config(chip, pwm, state); > + if (ret) > + return ret; > + > + if (!enabled) > + ret = rz_mtu3_pwm_enable(rz_mtu3_pwm, pwm); > + > + return ret; > +} > + > +static const struct pwm_ops rz_mtu3_pwm_ops = { > + .request = rz_mtu3_pwm_request, > + .free = rz_mtu3_pwm_free, > + .get_state = rz_mtu3_pwm_get_state, > + .apply = rz_mtu3_pwm_apply, > + .owner = THIS_MODULE, > +}; > + > +static int rz_mtu3_pwm_pm_runtime_suspend(struct device *dev) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = dev_get_drvdata(dev); > + > + clk_disable_unprepare(rz_mtu3_pwm->clk); > + > + return 0; > +} > + > +static int rz_mtu3_pwm_pm_runtime_resume(struct device *dev) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = dev_get_drvdata(dev); > + > + clk_prepare_enable(rz_mtu3_pwm->clk); > + > + return 0; > +} > + > +static DEFINE_RUNTIME_DEV_PM_OPS(rz_mtu3_pwm_pm_ops, > + rz_mtu3_pwm_pm_runtime_suspend, > + rz_mtu3_pwm_pm_runtime_resume, NULL); > + > +static void rz_mtu3_pwm_pm_disable(void *data) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = data; > + > + pm_runtime_disable(rz_mtu3_pwm->chip.dev); > + pm_runtime_set_suspended(rz_mtu3_pwm->chip.dev); > +} > + > +static int rz_mtu3_pwm_probe(struct platform_device *pdev) > +{ > + struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent); > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm; > + struct device *dev = &pdev->dev; > + int num_pwm_hw_ch = 0; > + unsigned int i; > + int ret; > + > + rz_mtu3_pwm = devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm), GFP_KERNEL); > + if (!rz_mtu3_pwm) > + return -ENOMEM; > + > + rz_mtu3_pwm->clk = ddata->clk; > + rz_mtu3_pwm->rate = clk_get_rate(rz_mtu3_pwm->clk); Note that clk_get_rate isn't reliable for disabled clocks, so please enable first and then call clk_get_rate(). Also consider calling clk_rate_exclusive_get(). > + for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) { > + if (i == RZ_MTU5 || i == RZ_MTU8) > + continue; > + > + rz_mtu3_pwm->ch[num_pwm_hw_ch] = &ddata->channels[i]; > + rz_mtu3_pwm->ch[num_pwm_hw_ch]->dev = dev; > + num_pwm_hw_ch++; > + } > + > + mutex_init(&rz_mtu3_pwm->lock); > + platform_set_drvdata(pdev, rz_mtu3_pwm); This is unused. > + clk_prepare_enable(rz_mtu3_pwm->clk); Missing error checking. > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + ret = devm_add_action_or_reset(&pdev->dev, > + rz_mtu3_pwm_pm_disable, > + rz_mtu3_pwm); > + if (ret < 0) > + goto disable_clock; > + > + rz_mtu3_pwm->chip.dev = &pdev->dev; > + rz_mtu3_pwm->chip.ops = &rz_mtu3_pwm_ops; > + rz_mtu3_pwm->chip.npwm = RZ_MTU3_MAX_PWM_MODE1_CHANNELS; > + ret = devm_pwmchip_add(&pdev->dev, &rz_mtu3_pwm->chip); > + if (ret) { > + dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > + goto disable_clock; > + } > + > + return 0; > + > +disable_clock: > + clk_disable_unprepare(rz_mtu3_pwm->clk); > + return ret; > +} On .remove the clk isn't disabled. > + > +static struct platform_driver rz_mtu3_pwm_driver = { > + .driver = { > + .name = "pwm-rz-mtu3", > + .pm = pm_ptr(&rz_mtu3_pwm_pm_ops), > + }, > + .probe = rz_mtu3_pwm_probe, > +}; > +module_platform_driver(rz_mtu3_pwm_driver); > + > +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>"); > +MODULE_ALIAS("platform:pwm-rz-mtu3"); > +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a PWM Timer Driver"); > +MODULE_LICENSE("GPL"); Best regards Uwe
Hi Uwe, Thanks for the feedback. > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver > > Hello, > > I'm working on my review backlog, sorry that it took so long. OK. > > On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > > Add support for RZ/G2L MTU3a PWM driver. The IP supports following PWM > > modes > > > > 1) PWM mode{1,2} > > 2) Reset-synchronized PWM mode > > 3) Complementary PWM mode{1,2,3} > > It's unclear to me what "PWM mode1" and the other modes are. I suspect this > is some chip specific naming that isn't understandable for outsiders? Would > be great to explain that a bit more. I will give some details about PWM modes mentioned in the HW manual here. I will respond to other comments later. PWM Mode 1 ------------ n = {0,1,2,3,4,6,7} MTIOC0A:-MTU0 TGRA input capture input/output compare output/PWM output pin TGRA: Timer General Register A TIOR: Timer I/O control register In PWM mode 1, PWM waveforms in up to 12 phases can be output PWM waveforms are output from the MTIOCnA and MTIOCnC pins by pairing TGRA with TGRB and TGRC with TGRD. The levels specified by the TIOR.IOA[3:0] and IOC[3:0] bits are output from the MTIOCnA and MTIOCnC pins at compare matches A and C, and the level specified by the TIOR.IOB[3:0] and IOD[3:0] bits are output at compare matches B and D (n = 0 to 4, 6, 7). The initial output value is set in TGRA or TGRC. If the values set in paired TGRs are identical, the output value does not change even when a compare match occurs. PWM Mode 2 ---------- n = {0,1,2} PWM waveform output is generated using one TGR as the cycle register and the others as duty registers. The level specified in TIOR is output at compare matches. Upon counter clearing by a cycle register compare match, the initial value set in TIOR is output from each pin. If the values set in the cycle and duty registers are identical, the output value does not change even when a compare match occurs. In PWM mode 2, up to eight phases of PWM waveforms can be output when synchronous clearing is used as synchronous operation in the channels that cannot be placed in PWM mode 2. Reset-Synchronized PWM Mode: --------------------------- In the reset-synchronized PWM mode, three phases of positive and negative PWM waveforms (six phases in total) that share a common wave transition point can be output by combining MTU3 and MTU4 and MTU6 and MTU7. When set for reset-synchronized PWM mode, the MTIOC3B, MTIOC3D, MTIOC4A, MTIOC4C, MTIOC4B, MTIOC4D, MTIOC6B, MTIOC6D, MTIOC7A, MTIOC7C, MTIOC7B, and MTIOC7D pins function as PWM output pins and timer counters 6 and 12 (MTU3.TCNT and MTU6.TCNT) functions as an up-counter Complementary PWM Mode: ---------------------- In complementary PWM mode, dead time can be set for PWM waveforms to be output. The dead time is the period during which the upper and lower arm transistors are set to the inactive level in order to prevent short-circuiting of the arms. Six positive-phase and six negative-phase PWM waveforms (12 phases in total) with dead time can be output by combining MTU3/ MTU4 and MTU6/MTU7. PWM waveforms without dead time can also be output. In complementary PWM mode, nine registers (compare registers, buffer registers, and temporary registers) are used to control the duty ratio for the PWM output. Complementary PWM mode 1 (transfer at crest) Complementary PWM mode 2 (transfer at trough) Complementary PWM mode 3 (transfer at crest and trough) Note: I will respond to other comments later. Cheers, Biju > > > This patch adds basic pwm mode 1 support for RZ/G2L MTU3a pwm driver > > by creating separate logical channels for each IOs. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v11->v12: > > * Updated header file to <linux/mfd/rz-mtu3.h> as core driver is in MFD. > > * Reordered get_state() > > v10->v11: > > * No change. > > v9->v10: > > * No change. > > v8->v9: > > * Added prescale/duty_cycle variables to struct rz_mtu3_pwm_chip and > > cached this values in rz_mtu3_pwm_config and used this cached values > > in get_state(), if PWM is disabled. > > * Added return code for get_state() > > v7->v8: > > * Simplified rz_mtu3_pwm_request by calling rz_mtu3_request_channel() > > * Simplified rz_mtu3_pwm_free by calling rz_mtu3_release_channel() > > v6->v7: > > * Added channel specific mutex lock to avoid race between counter > > device and rz_mtu3_pwm_{request,free} > > * Added pm_runtime_resume_and_get in rz_mtu3_pwm_enable() > > * Added pm_runtime_put_sync in rz_mtu3_pwm_disable() > > * Updated rz_mtu3_pwm_config() > > * Updated rz_mtu3_pwm_apply() > > v5->v6: > > * Updated commit and Kconfig description > > * Sorted the header > > * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable() > > * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed > > __maybe_unused from suspend/resume() > > v4->v5: > > * pwm device is instantiated by mtu3a core driver. > > v3->v4: > > * There is no resource associated with "rz-mtu3-pwm" compatible > > and moved the code to mfd subsystem as it binds against "rz-mtu". > > * Removed struct platform_driver rz_mtu3_pwm_driver. > > v2->v3: > > * No change. > > v1->v2: > > * Modelled as a single PWM device handling multiple channles. > > * Used PM framework to manage the clocks. > > --- > > drivers/pwm/Kconfig | 11 + > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-rz-mtu3.c | 485 > > ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 497 insertions(+) > > create mode 100644 drivers/pwm/pwm-rz-mtu3.c > >
Hello Biju, On Wed, Feb 15, 2023 at 10:31:20AM +0000, Biju Das wrote: > > On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > > > Add support for RZ/G2L MTU3a PWM driver. The IP supports following PWM > > > modes > > > > > > 1) PWM mode{1,2} > > > 2) Reset-synchronized PWM mode > > > 3) Complementary PWM mode{1,2,3} > > > > It's unclear to me what "PWM mode1" and the other modes are. I suspect this > > is some chip specific naming that isn't understandable for outsiders? Would > > be great to explain that a bit more. > > I will give some details about PWM modes mentioned in the HW manual here. > I will respond to other comments later. > > PWM Mode 1 > ------------ > n = {0,1,2,3,4,6,7} > MTIOC0A:-MTU0 TGRA input capture input/output compare output/PWM output pin > TGRA: Timer General Register A > TIOR: Timer I/O control register > In PWM mode 1, PWM waveforms in up to 12 phases can be output > > PWM waveforms are output from the MTIOCnA and MTIOCnC pins by pairing TGRA > with TGRB and TGRC with TGRD. The levels specified by the TIOR.IOA[3:0] and > IOC[3:0] bits are output from the MTIOCnA and MTIOCnC pins at compare matches > A and C, and the level specified by the TIOR.IOB[3:0] and IOD[3:0] bits are > output at compare matches B and D (n = 0 to 4, 6, 7). The initial output value > is set in TGRA or TGRC. If the values set in paired TGRs are identical, the > output value does not change even when a compare match occurs. > > PWM Mode 2 > ---------- > n = {0,1,2} > > PWM waveform output is generated using one TGR as the cycle register and the > others as duty registers. The level specified in TIOR is output at compare matches. > Upon counter clearing by a cycle register compare match, the initial value set > in TIOR is output from each pin. If the values set in the cycle and duty registers > are identical, the output value does not change even when a compare match occurs. > > In PWM mode 2, up to eight phases of PWM waveforms can be output when synchronous > clearing is used as synchronous operation in the channels that cannot be placed in > PWM mode 2. Why is PWM Mode 1 about two outputs? These could be abstracted as two individual PWMs, couldn't they? In this mode you could implement a phase shift, but the period is limited to the overflow time of the timers. In Mode 2 the period is more flexible, but no phase shift is possible (apart from inversed polarity). Did I get this right? > Reset-Synchronized PWM Mode: > --------------------------- > In the reset-synchronized PWM mode, three phases of positive and negative PWM > waveforms (six phases in total) that share a common wave transition point can > be output by combining MTU3 and MTU4 and MTU6 and MTU7. > > When set for reset-synchronized PWM mode, the MTIOC3B, MTIOC3D, MTIOC4A, MTIOC4C, > MTIOC4B, MTIOC4D, MTIOC6B, MTIOC6D, MTIOC7A, MTIOC7C, MTIOC7B, and MTIOC7D pins > function as PWM output pins and timer counters 6 and 12 (MTU3.TCNT and MTU6.TCNT) > functions as an up-counter > > > Complementary PWM Mode: > ---------------------- > > In complementary PWM mode, dead time can be set for PWM waveforms to be output. > The dead time is the period during which the upper and lower arm transistors are > set to the inactive level in order to prevent short-circuiting of the arms. > Six positive-phase and six negative-phase PWM waveforms (12 phases in total) > with dead time can be output by combining MTU3/ MTU4 and MTU6/MTU7. PWM waveforms > without dead time can also be output. > > In complementary PWM mode, nine registers (compare registers, buffer registers, > and temporary registers) are used to control the duty ratio for the PWM output. > > Complementary PWM mode 1 (transfer at crest) > Complementary PWM mode 2 (transfer at trough) > Complementary PWM mode 3 (transfer at crest and trough) These two modes are more general than the PWM framework supports. There was a series some time ago to implement settings with two outputs, but we didn't agree on an abstraction and the effort died. So for now these are out of scope, right? Best regards Uwe
Hi Uwe, Thanks for feedback. > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver > > Hello Biju, > > On Wed, Feb 15, 2023 at 10:31:20AM +0000, Biju Das wrote: > > > On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > > > > Add support for RZ/G2L MTU3a PWM driver. The IP supports following > > > > PWM modes > > > > > > > > 1) PWM mode{1,2} > > > > 2) Reset-synchronized PWM mode > > > > 3) Complementary PWM mode{1,2,3} > > > > > > It's unclear to me what "PWM mode1" and the other modes are. I > > > suspect this is some chip specific naming that isn't understandable > > > for outsiders? Would be great to explain that a bit more. > > > > I will give some details about PWM modes mentioned in the HW manual here. > > I will respond to other comments later. > > > > PWM Mode 1 > > ------------ > > n = {0,1,2,3,4,6,7} > > MTIOC0A:-MTU0 TGRA input capture input/output compare output/PWM > > output pin > > TGRA: Timer General Register A > > TIOR: Timer I/O control register > > In PWM mode 1, PWM waveforms in up to 12 phases can be output > > > > PWM waveforms are output from the MTIOCnA and MTIOCnC pins by pairing > > TGRA with TGRB and TGRC with TGRD. The levels specified by the > > TIOR.IOA[3:0] and IOC[3:0] bits are output from the MTIOCnA and > > MTIOCnC pins at compare matches A and C, and the level specified by > > the TIOR.IOB[3:0] and IOD[3:0] bits are output at compare matches B > > and D (n = 0 to 4, 6, 7). The initial output value is set in TGRA or > > TGRC. If the values set in paired TGRs are identical, the output value > does not change even when a compare match occurs. > > > > PWM Mode 2 > > ---------- > > n = {0,1,2} > > > > PWM waveform output is generated using one TGR as the cycle register > > and the others as duty registers. The level specified in TIOR is output at > compare matches. > > Upon counter clearing by a cycle register compare match, the initial > > value set in TIOR is output from each pin. If the values set in the > > cycle and duty registers are identical, the output value does not change > even when a compare match occurs. > > > > In PWM mode 2, up to eight phases of PWM waveforms can be output when > > synchronous clearing is used as synchronous operation in the channels > > that cannot be placed in PWM mode 2. > > Why is PWM Mode 1 about two outputs? These could be abstracted as two > individual PWMs, couldn't they? In this mode you could implement a phase > shift, but the period is limited to the overflow time of the timers. Please see the PWM Output Registers and Output Pins below [1] [1] https://paste.pics/7086f969d99b3205b8287e3b328529b9 Here same MTUs have 4 TGR's, in that case it has 2 outputs. Where as some MTU's have only 2 TGR's, in that case 1 ouput. Out of 2 TGR's, 1 TGR used for setting cycle and other one for setting duty. Please see the waveform for PWM mode1 [2] [2] https://paste.pics/44d75192ca0a8926fdf37796e4fe2915 > > In Mode 2 the period is more flexible, but no phase shift is possible (apart > from inversed polarity). > > Did I get this right? Here there is 1 TGR is used for setting cycle and others used for setting duty cycle. Please see example[2] in pwm mode2 outputting 5-phase PWM waveforms. [2] https://paste.pics/025f67e874d655d3eebff65ccc123970 > > > Reset-Synchronized PWM Mode: > > --------------------------- > > In the reset-synchronized PWM mode, three phases of positive and > > negative PWM waveforms (six phases in total) that share a common wave > > transition point can be output by combining MTU3 and MTU4 and MTU6 and > MTU7. > > > > When set for reset-synchronized PWM mode, the MTIOC3B, MTIOC3D, > > MTIOC4A, MTIOC4C, MTIOC4B, MTIOC4D, MTIOC6B, MTIOC6D, MTIOC7A, > > MTIOC7C, MTIOC7B, and MTIOC7D pins function as PWM output pins and > > timer counters 6 and 12 (MTU3.TCNT and MTU6.TCNT) functions as an > > up-counter > > > > > > Complementary PWM Mode: > > ---------------------- > > > > In complementary PWM mode, dead time can be set for PWM waveforms to be > output. > > The dead time is the period during which the upper and lower arm > > transistors are set to the inactive level in order to prevent short- > circuiting of the arms. > > Six positive-phase and six negative-phase PWM waveforms (12 phases in > > total) with dead time can be output by combining MTU3/ MTU4 and > > MTU6/MTU7. PWM waveforms without dead time can also be output. > > > > In complementary PWM mode, nine registers (compare registers, buffer > > registers, and temporary registers) are used to control the duty ratio for > the PWM output. > > > > Complementary PWM mode 1 (transfer at crest) Complementary PWM mode 2 > > (transfer at trough) Complementary PWM mode 3 (transfer at crest and > > trough) > > These two modes are more general than the PWM framework supports. There was > a series some time ago to implement settings with two outputs, but we didn't > agree on an abstraction and the effort died. So for now these are out of > scope, right? I haven't investigated much on complementary PWM, but our Verified Linux Package (VLP) for RZ/G2L supports it. I have a plan to add support for atleast complementary PWM mode with output pin protection(using POE3 module) later similar to GPT[3] with POEG [4]. [3] https://lore.kernel.org/linux-renesas-soc/20230113122343.769329-3-biju.das.jz@bp.renesas.com/ [4] https://lore.kernel.org/linux-renesas-soc/20221215213206.56666-1-biju.das.jz@bp.renesas.com/ Cheers, Biju
Hi Uwe, > Subject: RE: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver > > Hi Uwe, > > Thanks for feedback. > > > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver > > > > Hello Biju, > > > > On Wed, Feb 15, 2023 at 10:31:20AM +0000, Biju Das wrote: > > > > On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > > > > > Add support for RZ/G2L MTU3a PWM driver. The IP supports > > > > > following PWM modes > > > > > > > > > > 1) PWM mode{1,2} > > > > > 2) Reset-synchronized PWM mode > > > > > 3) Complementary PWM mode{1,2,3} > > > > > > > > It's unclear to me what "PWM mode1" and the other modes are. I > > > > suspect this is some chip specific naming that isn't > > > > understandable for outsiders? Would be great to explain that a bit > more. > > > > > > I will give some details about PWM modes mentioned in the HW manual > here. > > > I will respond to other comments later. > > > > > > PWM Mode 1 > > > ------------ > > > n = {0,1,2,3,4,6,7} > > > MTIOC0A:-MTU0 TGRA input capture input/output compare output/PWM > > > output pin > > > TGRA: Timer General Register A > > > TIOR: Timer I/O control register > > > In PWM mode 1, PWM waveforms in up to 12 phases can be output > > > > > > PWM waveforms are output from the MTIOCnA and MTIOCnC pins by > > > pairing TGRA with TGRB and TGRC with TGRD. The levels specified by > > > the TIOR.IOA[3:0] and IOC[3:0] bits are output from the MTIOCnA and > > > MTIOCnC pins at compare matches A and C, and the level specified by > > > the TIOR.IOB[3:0] and IOD[3:0] bits are output at compare matches B > > > and D (n = 0 to 4, 6, 7). The initial output value is set in TGRA or > > > TGRC. If the values set in paired TGRs are identical, the output > > > value > > does not change even when a compare match occurs. > > > > > > PWM Mode 2 > > > ---------- > > > n = {0,1,2} > > > > > > PWM waveform output is generated using one TGR as the cycle register > > > and the others as duty registers. The level specified in TIOR is > > > output at > > compare matches. > > > Upon counter clearing by a cycle register compare match, the initial > > > value set in TIOR is output from each pin. If the values set in the > > > cycle and duty registers are identical, the output value does not > > > change > > even when a compare match occurs. > > > > > > In PWM mode 2, up to eight phases of PWM waveforms can be output > > > when synchronous clearing is used as synchronous operation in the > > > channels that cannot be placed in PWM mode 2. > > > > Why is PWM Mode 1 about two outputs? These could be abstracted as two > > individual PWMs, couldn't they? Yes, 2 outputs can be model as 2 individual PWM's. In this mode you could implement a > > phase shift, but the period is limited to the overflow time of the timers. Yes, that is correct. For eg:- case MTU0 It has TGRA and TGRB -> MTIOC0A output (PWM0) TGRC and TGRD -> MTIOC0C output (PWM1) By using these registers, we can implement phase shift. Cheers, Biju
Hi Uwe, Thanks for the feedback. > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver > > Hello, > > I'm working on my review backlog, sorry that it took so long. > > On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > > Add support for RZ/G2L MTU3a PWM driver. The IP supports following PWM > > modes > > > > 1) PWM mode{1,2} > > 2) Reset-synchronized PWM mode > > 3) Complementary PWM mode{1,2,3} > > It's unclear to me what "PWM mode1" and the other modes are. I suspect this > is some chip specific naming that isn't understandable for outsiders? Would > be great to explain that a bit more. Ok I will add below to Limitation sections. Is it ok? PWM Mode 1: PWM waveforms are output from the MTIOCnA and MTIOCnC pins by pairing TGRA with TGRB and TGRC with TGRD. The levels specified by the TIOR.IOA[3:0] and IOC[3:0] bits are output from the MTIOCnA and MTIOCnC pins at compare matches A and C, and the level specified by the TIOR.IOB[3:0] and IOD[3:0] bits are output at compare matches B and D (n = 0 to 4, 6, 7). PWM Mode 2: PWM waveform output is generated using one TGR as the cycle register and the others as duty registers. Reset-Synchronized PWM Mode: In the reset-synchronized PWM mode, three phases of positive and negative PWM waveforms (six phases in total) that share a common wave transition point can be output by combining MTU3 and MTU4 and MTU6 and MTU7. Complementary PWM Mode: In complementary PWM mode, dead time can be set for PWM waveforms to be output. The dead time is the period during which the upper and lower arm transistors are set to the inactive level in order to prevent short-circuiting of the arms.Six positive-phase and six negative-phase PWM waveforms (12 phases in total)with dead time can be output by combining MTU3/ MTU4 and MTU6/MTU7. In complementary PWM mode, nine registers (compare registers, buffer registers, and temporary registers) are used to control the duty ratio for the PWM output. Complementary PWM mode 1 (transfer at crest) Complementary PWM mode 2 (transfer at trough) Complementary PWM mode 3 (transfer at crest and trough) > > > This patch adds basic pwm mode 1 support for RZ/G2L MTU3a pwm driver > > by creating separate logical channels for each IOs. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v11->v12: > > * Updated header file to <linux/mfd/rz-mtu3.h> as core driver is in MFD. > > * Reordered get_state() > > v10->v11: > > * No change. > > v9->v10: > > * No change. > > v8->v9: > > * Added prescale/duty_cycle variables to struct rz_mtu3_pwm_chip and > > cached this values in rz_mtu3_pwm_config and used this cached values > > in get_state(), if PWM is disabled. > > * Added return code for get_state() > > v7->v8: > > * Simplified rz_mtu3_pwm_request by calling rz_mtu3_request_channel() > > * Simplified rz_mtu3_pwm_free by calling rz_mtu3_release_channel() > > v6->v7: > > * Added channel specific mutex lock to avoid race between counter > > device and rz_mtu3_pwm_{request,free} > > * Added pm_runtime_resume_and_get in rz_mtu3_pwm_enable() > > * Added pm_runtime_put_sync in rz_mtu3_pwm_disable() > > * Updated rz_mtu3_pwm_config() > > * Updated rz_mtu3_pwm_apply() > > v5->v6: > > * Updated commit and Kconfig description > > * Sorted the header > > * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable() > > * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed > > __maybe_unused from suspend/resume() > > v4->v5: > > * pwm device is instantiated by mtu3a core driver. > > v3->v4: > > * There is no resource associated with "rz-mtu3-pwm" compatible > > and moved the code to mfd subsystem as it binds against "rz-mtu". > > * Removed struct platform_driver rz_mtu3_pwm_driver. > > v2->v3: > > * No change. > > v1->v2: > > * Modelled as a single PWM device handling multiple channles. > > * Used PM framework to manage the clocks. > > --- > > drivers/pwm/Kconfig | 11 + > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-rz-mtu3.c | 485 > > ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 497 insertions(+) > > create mode 100644 drivers/pwm/pwm-rz-mtu3.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > 31cdc9dae3c5..c54cbeabe093 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -492,6 +492,17 @@ config PWM_ROCKCHIP > > Generic PWM framework driver for the PWM controller found on > > Rockchip SoCs. > > > > +config PWM_RZ_MTU3 > > + tristate "Renesas RZ/G2L MTU3a PWM Timer support" > > + depends on RZ_MTU3 || COMPILE_TEST > > + depends on HAS_IOMEM > > + help > > + This driver exposes the MTU3a PWM Timer controller found in Renesas > > + RZ/G2L like chips through the PWM API. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-rz-mtu3. > > + > > config PWM_SAMSUNG > > tristate "Samsung PWM support" > > depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || > > COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > index a95aabae9115..6b75c0145336 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -45,6 +45,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > > obj-$(CONFIG_PWM_RZV2M) += pwm-rzv2m.o > > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > > +obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o > > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > > obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o > > diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c new > > file mode 100644 index 000000000000..d94e3fc36dfb > > --- /dev/null > > +++ b/drivers/pwm/pwm-rz-mtu3.c > > @@ -0,0 +1,485 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Renesas RZ/G2L MTU3a PWM Timer driver > > + * > > + * Copyright (C) 2022 Renesas Electronics Corporation > > + * > > + * Hardware manual for this IP can be found here > > + * > > +https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-u > > +sers-manual-hardware-0?language=en > > + * > > + * Limitations: > > + * - When PWM is disabled, the output is driven to Hi-Z. > > + * - While the hardware supports both polarities, the driver (for now) > > + * only handles normal polarity. > > + * - While the hardware supports pwm mode{1,2}, reset-synchronized pwm > and > > + * complementary pwm modes, the driver (for now) only handles pwm > mode1. > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/clk.h> > > +#include <linux/limits.h> > > +#include <linux/mfd/rz-mtu3.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/pwm.h> > > +#include <linux/time.h> > > + > > +#define RZ_MTU3_TMDR1_MD_NORMAL (0) > > +#define RZ_MTU3_TMDR1_MD_PWM_MODE_1 (2) > > IMHO it would make sense to put these definitions to where RZ_MTU3_TMDR1 is > defined. And I'd do it like this: > > * Timer mode register 1 */ > #define RZ_MTU3_TMDR1 5 > #define RZ_MTU3_TMDR1_MD GENMASK(3, 0) > #define RZ_MTU3_TMDR1_MD_NORMAL > FIELD_PREP(RZ_MTU3_TMDR1_MD, 0) > #define RZ_MTU3_TMDR1_MD_PWMMODE1 > FIELD_PREP(RZ_MTU3_TMDR1_MD, 2) Agreed, will move to include/linux/mfd/rz-mtu3.h. > > > +#define RZ_MTU3_TIOR_OC_RETAIN (0) > > +#define RZ_MTU3_TIOR_OC_0_H_COMP_MATCH (2) > > +#define RZ_MTU3_TIOR_OC_1_TOGGLE (7) > > +#define RZ_MTU3_TIOR_OC_IOA GENMASK(3, 0) > > + > > +#define RZ_MTU3_TCR_CCLR_TGRC (5 << 5) > > +#define RZ_MTU3_TCR_CKEG_RISING (0 << 3) > > + > > +#define RZ_MTU3_TCR_TPCS GENMASK(2, 0) > > + > > +#define RZ_MTU3_MAX_PWM_MODE1_CHANNELS (12) > > + > > +#define RZ_MTU3_MAX_HW_PWM_CHANNELS (7) > > + > > +static const u8 rz_mtu3_pwm_mode1_num_ios[] = { 2, 1, 1, 2, 2, 2, 2 > > +}; > > + > > +/** > > + * struct rz_mtu3_pwm_chip - MTU3 pwm private data > > + * > > + * @chip: MTU3 pwm chip data > > + * @clk: MTU3 module clock > > + * @lock: Lock to prevent concurrent access for usage count > > + * @rate: MTU3 clock rate > > + * @user_count: MTU3 usage count > > + * @rz_mtu3_channel: HW channels for the PWM */ > > + > > +struct rz_mtu3_pwm_chip { > > + struct pwm_chip chip; > > + struct clk *clk; > > + struct mutex lock; > > + unsigned long rate; > > + u32 user_count[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > > + struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > > + > > + /* > > + * The driver cannot read the current duty cycle/prescale from the > > + * hardware if the hardware is disabled. Cache the last programmed > > + * duty cycle/prescale value to return in that case. > > If the hardware is disabled, just doing .enabled = false in .get_state is > fine and easier. So this can be dropped I think. Yes, it can be dropped, after adding below check in get_state() + if (state->duty_cycle > state->period) + state->duty_cycle = state->period; + > > > + */ > > + u8 prescale[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > > + unsigned int duty_cycle[RZ_MTU3_MAX_PWM_MODE1_CHANNELS]; > > +}; > > + > > +static inline struct rz_mtu3_pwm_chip *to_rz_mtu3_pwm_chip(struct > > +pwm_chip *chip) { > > + return container_of(chip, struct rz_mtu3_pwm_chip, chip); } > > + > > +static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip > *rz_mtu3, > > + u64 period_cycles) > > +{ > > + u32 prescaled_period_cycles; > > + u8 prescale; > > + > > + prescaled_period_cycles = period_cycles >> 16; > > + if (prescaled_period_cycles >= 16) > > + prescale = 3; > > + else > > + prescale = (fls(prescaled_period_cycles) + 1) / 2; > > + > > + return prescale; > > +} > > + > > +static struct rz_mtu3_channel * > > +rz_mtu3_get_hw_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 > > +channel) { > > + unsigned int i, ch_index = 0; > > + > > + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { > > + ch_index += rz_mtu3_pwm_mode1_num_ios[i]; > > + > > + if (ch_index > channel) > > + break; > > + } > > + > > + return rz_mtu3_pwm->ch[i]; > > +} > > + > > +static u32 rz_mtu3_get_hw_channel_index(struct rz_mtu3_pwm_chip > *rz_mtu3_pwm, > > + struct rz_mtu3_channel *ch) > > +{ > > + u32 i; > > + > > + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { > > + if (ch == rz_mtu3_pwm->ch[i]) > > + break; > > + } > > + > > + return i; > > +} > > + > > +static bool rz_mtu3_pwm_is_second_channel(u32 ch_index, u32 hwpwm) { > > + u32 i, pwm_ch_index = 0; > > + > > + for (i = 0; i < ch_index; i++) > > + pwm_ch_index += rz_mtu3_pwm_mode1_num_ios[i]; > > + > > + return pwm_ch_index != hwpwm; > > +} > > I don't understand that channel allocation, maybe worth an explaining > comment?! I will add below comment just above rz_mtu3_get_hw_channel(). Is it ok? +/* + * PWM Mode1 has MTU{0..4}, MTU6 and MTU7, Probe function skips MTU5 and MTU8. + * So struct rz_mtu3_channel *ch contains only PWM mode1 MTU channels. + * MTU{1, 2} channels has a single IO each compared to 2 IOs for the rest of the + * channels. A LUT rz_mtu3_pwm_mode1_num_ios introduced to get the PWM channel + * and HW channel. + */ > > > +static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip > *rz_mtu3_pwm, > > + u32 hwpwm) > > +{ > > + struct rz_mtu3_channel *ch; > > + bool is_channel_en; > > + u32 ch_index; > > + u8 val; > > + > > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, hwpwm); > > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > > + is_channel_en = rz_mtu3_is_enabled(ch); > > + > > + if (rz_mtu3_pwm_is_second_channel(ch_index, hwpwm)) > > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORL); > > + else > > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORH); > > + > > + return (is_channel_en && (val & RZ_MTU3_TIOR_OC_IOA)); } > > + > > +static int rz_mtu3_pwm_request(struct pwm_chip *chip, struct > > +pwm_device *pwm) { > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > > + struct rz_mtu3_channel *ch; > > + u32 ch_index; > > + > > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > > + if (!rz_mtu3_pwm->user_count[ch_index] && > !rz_mtu3_request_channel(ch)) > > + return -EBUSY; > > + > > + mutex_lock(&rz_mtu3_pwm->lock); > > + rz_mtu3_pwm->user_count[ch_index]++; > > + mutex_unlock(&rz_mtu3_pwm->lock); > > The lock must protect the check, too, otherwise that's racy. Agreed, will move lock above 'ch' assignment. > > > + > > + return 0; > > +} > > + > > +static void rz_mtu3_pwm_free(struct pwm_chip *chip, struct pwm_device > > +*pwm) { > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > > + struct rz_mtu3_channel *ch; > > + u32 ch_index; > > + > > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > > + > > + mutex_lock(&rz_mtu3_pwm->lock); > > + rz_mtu3_pwm->user_count[ch_index]--; > > + mutex_unlock(&rz_mtu3_pwm->lock); > > + > > + if (!rz_mtu3_pwm->user_count[ch_index]) > > + rz_mtu3_release_channel(ch); > > I didn't check what rz_mtu3_release_channel() does, but I wonder what > happens if another thread calls rz_mtu3_pwm_request for the same channel > just after the if check. Agreed, will protect the entire section of this code. > > > +} > > + > > +static int rz_mtu3_pwm_enable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, > > + struct pwm_device *pwm) > > +{ > > + struct rz_mtu3_channel *ch; > > + u32 ch_index; > > + u8 val; > > + int rc; > > + > > + rc = pm_runtime_resume_and_get(rz_mtu3_pwm->chip.dev); > > + if (rc) > > + return rc; > > + > > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > > + val = (RZ_MTU3_TIOR_OC_1_TOGGLE << 4) | > > +RZ_MTU3_TIOR_OC_0_H_COMP_MATCH; > > + > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_PWM_MODE_1); > > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, val); > > + else > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, val); > > + > > + if (rz_mtu3_pwm->user_count[ch_index] <= 1) > > + rz_mtu3_enable(ch); > > + > > + return 0; > > +} > > + > > +static void rz_mtu3_pwm_disable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, > > + struct pwm_device *pwm) > > +{ > > + struct rz_mtu3_channel *ch; > > + u32 ch_index; > > + > > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > > + > > + /* Return to normal mode and disable output pins of MTU3 channel */ > > + if (rz_mtu3_pwm->user_count[ch_index] <= 1) > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, > RZ_MTU3_TMDR1_MD_NORMAL); > > + > > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, > RZ_MTU3_TIOR_OC_RETAIN); > > + else > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, > RZ_MTU3_TIOR_OC_RETAIN); > > + > > + if (rz_mtu3_pwm->user_count[ch_index] <= 1) > > + rz_mtu3_disable(ch); > > + > > + pm_runtime_put_sync(rz_mtu3_pwm->chip.dev); > > +} > > + > > +static int rz_mtu3_pwm_get_state(struct pwm_chip *chip, struct pwm_device > *pwm, > > + struct pwm_state *state) > > +{ > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > > + struct rz_mtu3_channel *ch; > > + u8 prescale, val; > > + u32 ch_index; > > + u16 dc, pv; > > + u64 tmp; > > + > > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > > + pm_runtime_get_sync(chip->dev); > > + state->enabled = rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, pwm->hwpwm); > > + if (state->enabled) { > > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TCR); > > + prescale = FIELD_GET(RZ_MTU3_TCR_TPCS, val); > > + > > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { > > + dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRD); > > + pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRC); > > + } else { > > + dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRB); > > + pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRA); > > + } > > + > Add a comment like: > > /* With prescale <= 7 and pv <= 0xffff this doesn't overflow. */ > Agreed. > > + tmp = NSEC_PER_SEC * (u64)pv << (2 * prescale); > > + state->period = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate); > > + } else { > > + /* If the PWM is disabled, use the cached value. */ > > + prescale = rz_mtu3_pwm->prescale[ch_index]; > > + dc = rz_mtu3_pwm->duty_cycle[pwm->hwpwm]; > > + } > > + > > + tmp = NSEC_PER_SEC * (u64)dc << (2 * prescale); > > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate); > > + state->polarity = PWM_POLARITY_NORMAL; > > + pm_runtime_put(chip->dev); > > Can it happen that dc > pv? I assume this implements a 100% relative duty > then. Please set .duty_cycle = .period in this case. OK, will add this check. > > > + > > + return 0; > > +} > > + > > +static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device > *pwm, > > + const struct pwm_state *state) { > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > > + struct rz_mtu3_channel *ch; > > + unsigned long pv, dc; > > + u64 period_cycles; > > + u64 duty_cycles; > > + u32 ch_index; > > + u8 prescale; > > + int err; > > + u8 val; > > + > > + /* > > + * Refuse clk rates > 1 GHz to prevent overflowing the following > > + * calculation. > > + */ > > + if (rz_mtu3_pwm->rate > NSEC_PER_SEC) > > + return -EINVAL; > > Maybe refuse this case in .probe() already? OK, will move to probe() + /* + * Refuse clk rates > 1 GHz to prevent overflow later for computing + * period and duty cycle. + */ > > > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > > + period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate, > > + NSEC_PER_SEC); > > + prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm, > > +period_cycles); > > + > > + if (period_cycles >> (2 * prescale) <= U16_MAX) > > + pv = period_cycles >> (2 * prescale); > > + else > > + pv = U16_MAX; > > + > > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rz_mtu3_pwm->rate, > > + NSEC_PER_SEC); > > + if (duty_cycles >> (2 * prescale) <= U16_MAX) > > + dc = duty_cycles >> (2 * prescale); > > + else > > + dc = U16_MAX; > > + > > + /* > > + * Store the duty cycle/prescale for future reference in cases where > the > > + * corresponding registers can't be read (i.e. when the PWM is > disabled). > > + */ > > + rz_mtu3_pwm->prescale[ch_index] = prescale; > > + rz_mtu3_pwm->duty_cycle[pwm->hwpwm] = dc; > > Above I suggested to drop this, but if you don't: This is broken. > rz_mtu3_pwm_config is only ever called with .enable = 1 and the values are > not updated when .apply() is called with .enable = 0, so you're investing > some effort to report an outdated value that is ignored in the end. OK will drop this. > > > + /* > > + * If the PWM channel is disabled, make sure to turn on the clock > > + * before writing the register. > > + */ > > + if (!pwm->state.enabled) { > > + err = pm_runtime_resume_and_get(chip->dev); > > + if (err) > > + return err; > > + } > > Maybe it's easier to call pm_runtime_resume_and_get() unconditionally? OK, will use below unconditional call instead. Is it ok? pm_runtime_get_sync(chip->dev); > > > + val = RZ_MTU3_TCR_CKEG_RISING | prescale; > > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > > + RZ_MTU3_TCR_CCLR_TGRC | val); > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRD, dc); > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRC, pv); > > + } else { > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > > + RZ_MTU3_TCR_CCLR_TGRA | val); > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRB, dc); > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRA, pv); > > + } > > + > > + /* If the PWM is not enabled, turn the clock off again to save power. > */ > > + if (!pwm->state.enabled) > > + pm_runtime_put(chip->dev); > > + > > + return 0; > > +} > > + > > +static int rz_mtu3_pwm_apply(struct pwm_chip *chip, struct pwm_device > *pwm, > > + const struct pwm_state *state) { > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); > > + bool enabled = pwm->state.enabled; > > + int ret; > > + > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -EINVAL; > > + > > + if (!state->enabled) { > > + if (enabled) > > + rz_mtu3_pwm_disable(rz_mtu3_pwm, pwm); > > + > > + return 0; > > + } > > + > > + ret = rz_mtu3_pwm_config(chip, pwm, state); > > + if (ret) > > + return ret; > > + > > + if (!enabled) > > + ret = rz_mtu3_pwm_enable(rz_mtu3_pwm, pwm); > > + > > + return ret; > > +} > > + > > +static const struct pwm_ops rz_mtu3_pwm_ops = { > > + .request = rz_mtu3_pwm_request, > > + .free = rz_mtu3_pwm_free, > > + .get_state = rz_mtu3_pwm_get_state, > > + .apply = rz_mtu3_pwm_apply, > > + .owner = THIS_MODULE, > > +}; > > + > > +static int rz_mtu3_pwm_pm_runtime_suspend(struct device *dev) { > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = dev_get_drvdata(dev); > > + > > + clk_disable_unprepare(rz_mtu3_pwm->clk); > > + > > + return 0; > > +} > > + > > +static int rz_mtu3_pwm_pm_runtime_resume(struct device *dev) { > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = dev_get_drvdata(dev); > > + > > + clk_prepare_enable(rz_mtu3_pwm->clk); > > + > > + return 0; > > +} > > + > > +static DEFINE_RUNTIME_DEV_PM_OPS(rz_mtu3_pwm_pm_ops, > > + rz_mtu3_pwm_pm_runtime_suspend, > > + rz_mtu3_pwm_pm_runtime_resume, NULL); > > + > > +static void rz_mtu3_pwm_pm_disable(void *data) { > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = data; > > + > > + pm_runtime_disable(rz_mtu3_pwm->chip.dev); > > + pm_runtime_set_suspended(rz_mtu3_pwm->chip.dev); > > +} > > + > > +static int rz_mtu3_pwm_probe(struct platform_device *pdev) { > > + struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent); > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm; > > + struct device *dev = &pdev->dev; > > + int num_pwm_hw_ch = 0; > > + unsigned int i; > > + int ret; > > + > > + rz_mtu3_pwm = devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm), > GFP_KERNEL); > > + if (!rz_mtu3_pwm) > > + return -ENOMEM; > > + > > + rz_mtu3_pwm->clk = ddata->clk; > > + rz_mtu3_pwm->rate = clk_get_rate(rz_mtu3_pwm->clk); > > Note that clk_get_rate isn't reliable for disabled clocks, so please enable > first and then call clk_get_rate(). Also consider calling > clk_rate_exclusive_get(). OK, will call get_rate() after enable. Runtime PM use clockenable/disable Frequently, so unnecessary to use clk_rate_exclusive_{get,put}. Is it ok? > > > + for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) { > > + if (i == RZ_MTU5 || i == RZ_MTU8) > > + continue; > > + > > + rz_mtu3_pwm->ch[num_pwm_hw_ch] = &ddata->channels[i]; > > + rz_mtu3_pwm->ch[num_pwm_hw_ch]->dev = dev; > > + num_pwm_hw_ch++; > > + } > > + > > + mutex_init(&rz_mtu3_pwm->lock); > > + platform_set_drvdata(pdev, rz_mtu3_pwm); > > This is unused. Nope. It used in runtime PM calls. > > > + clk_prepare_enable(rz_mtu3_pwm->clk); > > Missing error checking. OK, will add error check. > > > + pm_runtime_set_active(&pdev->dev); > > + pm_runtime_enable(&pdev->dev); > > + ret = devm_add_action_or_reset(&pdev->dev, > > + rz_mtu3_pwm_pm_disable, > > + rz_mtu3_pwm); > > + if (ret < 0) > > + goto disable_clock; > > + > > + rz_mtu3_pwm->chip.dev = &pdev->dev; > > + rz_mtu3_pwm->chip.ops = &rz_mtu3_pwm_ops; > > + rz_mtu3_pwm->chip.npwm = RZ_MTU3_MAX_PWM_MODE1_CHANNELS; > > + ret = devm_pwmchip_add(&pdev->dev, &rz_mtu3_pwm->chip); > > + if (ret) { > > + dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > > + goto disable_clock; > > + } > > + > > + return 0; > > + > > +disable_clock: > > + clk_disable_unprepare(rz_mtu3_pwm->clk); > > + return ret; > > +} > > On .remove the clk isn't disabled. It is not required. It is already disabled after probe. Clock_enable_prepare enables the clk during probe, Then later PM runtime suspend turns off the clock using clk_prepare_disable(). On error case, in probe, PM suspend won't get called, So we need to use clk_prepare_disable(). After probe: [ 15.680492] rzg2l-cpg 11010000.clock-controller: CLK_ON 1336/mtu_x_mck ON [ 15.725015] rzg2l-cpg 11010000.clock-controller: CLK_ON 1336/mtu_x_mck OFF devmem2 0x11010538 | grep Read Unbind and bind call reports, balanced clk usage. Please correct me, if I am wrong or need any clarification. Cheers, Biju
Hello Biju, On Wed, Feb 15, 2023 at 07:14:12PM +0000, Biju Das wrote: > > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver > > > > On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > > > Add support for RZ/G2L MTU3a PWM driver. The IP supports following PWM > > > modes > > > > > > 1) PWM mode{1,2} > > > 2) Reset-synchronized PWM mode > > > 3) Complementary PWM mode{1,2,3} > > > > It's unclear to me what "PWM mode1" and the other modes are. I suspect this > > is some chip specific naming that isn't understandable for outsiders? Would > > be great to explain that a bit more. > > Ok I will add below to Limitation sections. Is it ok? > > PWM Mode 1: PWM waveforms are output from the MTIOCnA and MTIOCnC pins by > pairing TGRA with TGRB and TGRC with TGRD. The levels specified by the > TIOR.IOA[3:0] and IOC[3:0] bits are output from the MTIOCnA and MTIOCnC pins > at compare matches A and C, and the level specified by the TIOR.IOB[3:0] and > IOD[3:0] bits are output at compare matches B and D (n = 0 to 4, 6, 7). > > > PWM Mode 2: PWM waveform output is generated using one TGR as the cycle > register and the others as duty registers. > > Reset-Synchronized PWM Mode: In the reset-synchronized PWM mode, three phases > of positive and negative PWM waveforms (six phases in total) that share a > common wave transition point can be output by combining MTU3 and MTU4 and > MTU6 and MTU7. > > Complementary PWM Mode: In complementary PWM mode, dead time can be set for > PWM waveforms to be output. The dead time is the period during which the upper > and lower arm transistors are set to the inactive level in order to prevent > short-circuiting of the arms.Six positive-phase and six negative-phase PWM > waveforms (12 phases in total)with dead time can be output by combining > MTU3/ MTU4 and MTU6/MTU7. > > In complementary PWM mode, nine registers (compare registers, buffer registers, > and temporary registers) are used to control the duty ratio for the PWM output. > Complementary PWM mode 1 (transfer at crest) > Complementary PWM mode 2 (transfer at trough) > Complementary PWM mode 3 (transfer at crest and trough) I read it five times now and still don't get it. The problem (maybe) is that there are many abbreviations I don't understand. Most critical is: What is a TGR? I think I understand the following: Your hardware has one(?) freerunning counter "TCNT". TGRs get set to a value where something happens when TCNT reaches that value. In figure 16.27, reaching TGRA resets TCNT to 0 and the output value to low. Reaching TGRB sets the output value to high. So far so easy. (Apart from your description of mode 1 talking about four TGRs and two pins which could just be two independent PWMs (in the sense of the pwm framework)?) PWM mode 2 (figure 16.28) is the same just with more outputs. MTU1.TGRB defines the period (as did TGRB in the above example) and there are several other TGRx that define the duty cycle of one output each which all share the same period. So if I restrict mode 2 to just MTU1.TRGB and MTU1.TRGA I have mode 1. So I don't get the difference between the two modes. Describing modes (Reset-Synchronized PWM Mode and Complementary PWM Mode) that are not relevant to the driver (yet?), doesn't help understanding the hardware either. > > > This patch adds basic pwm mode 1 support for RZ/G2L MTU3a pwm driver > > > by creating separate logical channels for each IOs. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > v11->v12: > > > * Updated header file to <linux/mfd/rz-mtu3.h> as core driver is in MFD. > > > * Reordered get_state() > > > v10->v11: > > > * No change. > > > v9->v10: > > > * No change. > > > v8->v9: > > > * Added prescale/duty_cycle variables to struct rz_mtu3_pwm_chip and > > > cached this values in rz_mtu3_pwm_config and used this cached values > > > in get_state(), if PWM is disabled. > > > * Added return code for get_state() > > > v7->v8: > > > * Simplified rz_mtu3_pwm_request by calling rz_mtu3_request_channel() > > > * Simplified rz_mtu3_pwm_free by calling rz_mtu3_release_channel() > > > v6->v7: > > > * Added channel specific mutex lock to avoid race between counter > > > device and rz_mtu3_pwm_{request,free} > > > * Added pm_runtime_resume_and_get in rz_mtu3_pwm_enable() > > > * Added pm_runtime_put_sync in rz_mtu3_pwm_disable() > > > * Updated rz_mtu3_pwm_config() > > > * Updated rz_mtu3_pwm_apply() > > > v5->v6: > > > * Updated commit and Kconfig description > > > * Sorted the header > > > * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable() > > > * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed > > > __maybe_unused from suspend/resume() > > > v4->v5: > > > * pwm device is instantiated by mtu3a core driver. > > > v3->v4: > > > * There is no resource associated with "rz-mtu3-pwm" compatible > > > and moved the code to mfd subsystem as it binds against "rz-mtu". > > > * Removed struct platform_driver rz_mtu3_pwm_driver. > > > v2->v3: > > > * No change. > > > v1->v2: > > > * Modelled as a single PWM device handling multiple channles. > > > * Used PM framework to manage the clocks. > > > --- > > > drivers/pwm/Kconfig | 11 + > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-rz-mtu3.c | 485 > > > ++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 497 insertions(+) > > > create mode 100644 drivers/pwm/pwm-rz-mtu3.c > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > > 31cdc9dae3c5..c54cbeabe093 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -492,6 +492,17 @@ config PWM_ROCKCHIP > > > Generic PWM framework driver for the PWM controller found on > > > Rockchip SoCs. > > > > > > +config PWM_RZ_MTU3 > > > + tristate "Renesas RZ/G2L MTU3a PWM Timer support" > > > + depends on RZ_MTU3 || COMPILE_TEST > > > + depends on HAS_IOMEM > > > + help > > > + This driver exposes the MTU3a PWM Timer controller found in Renesas > > > + RZ/G2L like chips through the PWM API. > > > + > > > + To compile this driver as a module, choose M here: the module > > > + will be called pwm-rz-mtu3. > > > + > > > config PWM_SAMSUNG > > > tristate "Samsung PWM support" > > > depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || > > > COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > index a95aabae9115..6b75c0145336 100644 > > > --- a/drivers/pwm/Makefile > > > +++ b/drivers/pwm/Makefile > > > @@ -45,6 +45,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > > > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > > > obj-$(CONFIG_PWM_RZV2M) += pwm-rzv2m.o > > > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > > > +obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o > > > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > > > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > > > obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o > > > diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c new > > > file mode 100644 index 000000000000..d94e3fc36dfb > > > --- /dev/null > > > +++ b/drivers/pwm/pwm-rz-mtu3.c > > > @@ -0,0 +1,485 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Renesas RZ/G2L MTU3a PWM Timer driver > > > + * > > > + * Copyright (C) 2022 Renesas Electronics Corporation > > > + * > > > + * Hardware manual for this IP can be found here > > > + * > > > +https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-u > > > +sers-manual-hardware-0?language=en > > > + * > > > + * Limitations: > > > + * - When PWM is disabled, the output is driven to Hi-Z. > > > + * - While the hardware supports both polarities, the driver (for now) > > > + * only handles normal polarity. > > > + * - While the hardware supports pwm mode{1,2}, reset-synchronized pwm > > and > > > + * complementary pwm modes, the driver (for now) only handles pwm > > mode1. > > > + */ > > > + > > > +#include <linux/bitfield.h> > > > +#include <linux/clk.h> > > > +#include <linux/limits.h> > > > +#include <linux/mfd/rz-mtu3.h> > > > +#include <linux/module.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/pm_runtime.h> > > > +#include <linux/pwm.h> > > > +#include <linux/time.h> > > > + > > > +#define RZ_MTU3_TMDR1_MD_NORMAL (0) > > > +#define RZ_MTU3_TMDR1_MD_PWM_MODE_1 (2) > > > > IMHO it would make sense to put these definitions to where RZ_MTU3_TMDR1 is > > defined. And I'd do it like this: > > > > * Timer mode register 1 */ > > #define RZ_MTU3_TMDR1 5 > > #define RZ_MTU3_TMDR1_MD GENMASK(3, 0) > > #define RZ_MTU3_TMDR1_MD_NORMAL > > FIELD_PREP(RZ_MTU3_TMDR1_MD, 0) > > #define RZ_MTU3_TMDR1_MD_PWMMODE1 > > FIELD_PREP(RZ_MTU3_TMDR1_MD, 2) > > Agreed, will move to include/linux/mfd/rz-mtu3.h. > > > > > > +#define RZ_MTU3_TIOR_OC_RETAIN (0) > > > +#define RZ_MTU3_TIOR_OC_0_H_COMP_MATCH (2) > > > +#define RZ_MTU3_TIOR_OC_1_TOGGLE (7) > > > +#define RZ_MTU3_TIOR_OC_IOA GENMASK(3, 0) > > > + > > > +#define RZ_MTU3_TCR_CCLR_TGRC (5 << 5) > > > +#define RZ_MTU3_TCR_CKEG_RISING (0 << 3) > > > + > > > +#define RZ_MTU3_TCR_TPCS GENMASK(2, 0) > > > + > > > +#define RZ_MTU3_MAX_PWM_MODE1_CHANNELS (12) > > > + > > > +#define RZ_MTU3_MAX_HW_PWM_CHANNELS (7) > > > + > > > +static const u8 rz_mtu3_pwm_mode1_num_ios[] = { 2, 1, 1, 2, 2, 2, 2 > > > +}; > > > + > > > +/** > > > + * struct rz_mtu3_pwm_chip - MTU3 pwm private data > > > + * > > > + * @chip: MTU3 pwm chip data > > > + * @clk: MTU3 module clock > > > + * @lock: Lock to prevent concurrent access for usage count > > > + * @rate: MTU3 clock rate > > > + * @user_count: MTU3 usage count > > > + * @rz_mtu3_channel: HW channels for the PWM */ > > > + > > > +struct rz_mtu3_pwm_chip { > > > + struct pwm_chip chip; > > > + struct clk *clk; > > > + struct mutex lock; > > > + unsigned long rate; > > > + u32 user_count[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > > > + struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > > > + > > > + /* > > > + * The driver cannot read the current duty cycle/prescale from the > > > + * hardware if the hardware is disabled. Cache the last programmed > > > + * duty cycle/prescale value to return in that case. > > > > If the hardware is disabled, just doing .enabled = false in .get_state is > > fine and easier. So this can be dropped I think. > > Yes, it can be dropped, after adding below check in get_state() > > + if (state->duty_cycle > state->period) > + state->duty_cycle = state->period; > + > > > > > > + */ > > > + u8 prescale[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > > > + unsigned int duty_cycle[RZ_MTU3_MAX_PWM_MODE1_CHANNELS]; > > > +}; > > > + > > > +static inline struct rz_mtu3_pwm_chip *to_rz_mtu3_pwm_chip(struct > > > +pwm_chip *chip) { > > > + return container_of(chip, struct rz_mtu3_pwm_chip, chip); } > > > + > > > +static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip > > *rz_mtu3, > > > + u64 period_cycles) > > > +{ > > > + u32 prescaled_period_cycles; > > > + u8 prescale; > > > + > > > + prescaled_period_cycles = period_cycles >> 16; > > > + if (prescaled_period_cycles >= 16) > > > + prescale = 3; > > > + else > > > + prescale = (fls(prescaled_period_cycles) + 1) / 2; > > > + > > > + return prescale; > > > +} > > > + > > > +static struct rz_mtu3_channel * > > > +rz_mtu3_get_hw_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 > > > +channel) { > > > + unsigned int i, ch_index = 0; > > > + > > > + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { > > > + ch_index += rz_mtu3_pwm_mode1_num_ios[i]; > > > + > > > + if (ch_index > channel) > > > + break; > > > + } > > > + > > > + return rz_mtu3_pwm->ch[i]; > > > +} > > > + > > > +static u32 rz_mtu3_get_hw_channel_index(struct rz_mtu3_pwm_chip > > *rz_mtu3_pwm, > > > + struct rz_mtu3_channel *ch) > > > +{ > > > + u32 i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { > > > + if (ch == rz_mtu3_pwm->ch[i]) > > > + break; > > > + } > > > + > > > + return i; > > > +} > > > + > > > +static bool rz_mtu3_pwm_is_second_channel(u32 ch_index, u32 hwpwm) { > > > + u32 i, pwm_ch_index = 0; > > > + > > > + for (i = 0; i < ch_index; i++) > > > + pwm_ch_index += rz_mtu3_pwm_mode1_num_ios[i]; > > > + > > > + return pwm_ch_index != hwpwm; > > > +} > > > > I don't understand that channel allocation, maybe worth an explaining > > comment?! > > I will add below comment just above rz_mtu3_get_hw_channel(). Is it ok? > > +/* > + * PWM Mode1 has MTU{0..4}, MTU6 and MTU7, Probe function skips MTU5 and MTU8. What is MTU? > + * So struct rz_mtu3_channel *ch contains only PWM mode1 MTU channels. > + * MTU{1, 2} channels has a single IO each compared to 2 IOs for the rest of the > + * channels. A LUT rz_mtu3_pwm_mode1_num_ios introduced to get the PWM channel > + * and HW channel. What is LUT? These channels are about which TRG can influence which output? > + */ > > > > > > +static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip > > *rz_mtu3_pwm, > > > + u32 hwpwm) > > > +{ > > > + struct rz_mtu3_channel *ch; > > > + bool is_channel_en; > > > + u32 ch_index; > > > + u8 val; > > > + > > > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, hwpwm); > > > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > > > + is_channel_en = rz_mtu3_is_enabled(ch); > > > + > > > + if (rz_mtu3_pwm_is_second_channel(ch_index, hwpwm)) > > > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORL); > > > + else > > > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORH); > > > + > > > + return (is_channel_en && (val & RZ_MTU3_TIOR_OC_IOA)); } > > > + > > > [...] > > > + /* > > > + * If the PWM channel is disabled, make sure to turn on the clock > > > + * before writing the register. > > > + */ > > > + if (!pwm->state.enabled) { > > > + err = pm_runtime_resume_and_get(chip->dev); > > > + if (err) > > > + return err; > > > + } > > > > Maybe it's easier to call pm_runtime_resume_and_get() unconditionally? > > OK, will use below unconditional call instead. Is it ok? > > pm_runtime_get_sync(chip->dev); Try it and if it's simpler stick to it, if not, my suggestion was wrong. > > > + val = RZ_MTU3_TCR_CKEG_RISING | prescale; > > > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { > > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > > > + RZ_MTU3_TCR_CCLR_TGRC | val); > > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRD, dc); > > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRC, pv); > > > + } else { > > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > > > + RZ_MTU3_TCR_CCLR_TGRA | val); > > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRB, dc); > > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRA, pv); > > > + } > > > + > > > [...] > > > +static DEFINE_RUNTIME_DEV_PM_OPS(rz_mtu3_pwm_pm_ops, > > > + rz_mtu3_pwm_pm_runtime_suspend, > > > + rz_mtu3_pwm_pm_runtime_resume, NULL); > > > + > > > +static void rz_mtu3_pwm_pm_disable(void *data) { > > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = data; > > > + > > > + pm_runtime_disable(rz_mtu3_pwm->chip.dev); > > > + pm_runtime_set_suspended(rz_mtu3_pwm->chip.dev); > > > +} > > > + > > > +static int rz_mtu3_pwm_probe(struct platform_device *pdev) { > > > + struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent); > > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm; > > > + struct device *dev = &pdev->dev; > > > + int num_pwm_hw_ch = 0; > > > + unsigned int i; > > > + int ret; > > > + > > > + rz_mtu3_pwm = devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm), > > GFP_KERNEL); > > > + if (!rz_mtu3_pwm) > > > + return -ENOMEM; > > > + > > > + rz_mtu3_pwm->clk = ddata->clk; > > > + rz_mtu3_pwm->rate = clk_get_rate(rz_mtu3_pwm->clk); > > > > Note that clk_get_rate isn't reliable for disabled clocks, so please enable > > first and then call clk_get_rate(). Also consider calling > > clk_rate_exclusive_get(). > > > OK, will call get_rate() after enable. Runtime PM use clockenable/disable > Frequently, so unnecessary to use clk_rate_exclusive_{get,put}. Is it ok? One doesn't have to do with the other. After calling clk_rate_exclusive_get() you can be sure that no other driver does anything to change the clk which would mess with the emitted signals. I don't know the exact semantics of clk_rate_exclusive_get(), but I'd assume that even if you disable the clock you should be able to rely on it running at the same rate after reenable. > > > + for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) { > > > + if (i == RZ_MTU5 || i == RZ_MTU8) > > > + continue; > > > + > > > + rz_mtu3_pwm->ch[num_pwm_hw_ch] = &ddata->channels[i]; > > > + rz_mtu3_pwm->ch[num_pwm_hw_ch]->dev = dev; > > > + num_pwm_hw_ch++; > > > + } > > > + > > > + mutex_init(&rz_mtu3_pwm->lock); > > > + platform_set_drvdata(pdev, rz_mtu3_pwm); > > > > This is unused. > > Nope. It used in runtime PM calls. Oh, indeed. Looking that up I saw that you didn't check the return values of clk_disable_unprepare() and clk_prepare_enable() in them. > > > + pm_runtime_set_active(&pdev->dev); > > > + pm_runtime_enable(&pdev->dev); > > > + ret = devm_add_action_or_reset(&pdev->dev, > > > + rz_mtu3_pwm_pm_disable, > > > + rz_mtu3_pwm); > > > + if (ret < 0) > > > + goto disable_clock; > > > + > > > + rz_mtu3_pwm->chip.dev = &pdev->dev; > > > + rz_mtu3_pwm->chip.ops = &rz_mtu3_pwm_ops; > > > + rz_mtu3_pwm->chip.npwm = RZ_MTU3_MAX_PWM_MODE1_CHANNELS; > > > + ret = devm_pwmchip_add(&pdev->dev, &rz_mtu3_pwm->chip); > > > + if (ret) { > > > + dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > > > + goto disable_clock; > > > + } > > > + > > > + return 0; > > > + > > > +disable_clock: > > > + clk_disable_unprepare(rz_mtu3_pwm->clk); > > > + return ret; > > > +} > > > > On .remove the clk isn't disabled. > > It is not required. It is already disabled after probe. > > Clock_enable_prepare enables the clk during probe, Then later > PM runtime suspend turns off the clock using clk_prepare_disable(). > > On error case, in probe, PM suspend won't get called, So we need to use > clk_prepare_disable(). > > After probe: > [ 15.680492] rzg2l-cpg 11010000.clock-controller: CLK_ON 1336/mtu_x_mck ON > [ 15.725015] rzg2l-cpg 11010000.clock-controller: CLK_ON 1336/mtu_x_mck OFF > devmem2 0x11010538 | grep Read > > Unbind and bind call reports, balanced clk usage. > > Please correct me, if I am wrong or need any clarification. As I missed the PM callbacks you are right I think. (I didn't recheck the logic now though.) Best regards Uwe
Hi Uwe, Thanks for the feedback. > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver > > Hello Biju, > > On Wed, Feb 15, 2023 at 07:14:12PM +0000, Biju Das wrote: > > > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM > > > driver > > > > > > On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > > > > Add support for RZ/G2L MTU3a PWM driver. The IP supports following > > > > PWM modes > > > > > > > > 1) PWM mode{1,2} > > > > 2) Reset-synchronized PWM mode > > > > 3) Complementary PWM mode{1,2,3} > > > > > > It's unclear to me what "PWM mode1" and the other modes are. I > > > suspect this is some chip specific naming that isn't understandable > > > for outsiders? Would be great to explain that a bit more. > > > > Ok I will add below to Limitation sections. Is it ok? > > > > PWM Mode 1: PWM waveforms are output from the MTIOCnA and MTIOCnC pins > > by pairing TGRA with TGRB and TGRC with TGRD. The levels specified by > > the TIOR.IOA[3:0] and IOC[3:0] bits are output from the MTIOCnA and > > MTIOCnC pins at compare matches A and C, and the level specified by > > the TIOR.IOB[3:0] and IOD[3:0] bits are output at compare matches B and D > (n = 0 to 4, 6, 7). > > > > > > PWM Mode 2: PWM waveform output is generated using one TGR as the > > cycle register and the others as duty registers. > > > > Reset-Synchronized PWM Mode: In the reset-synchronized PWM mode, three > > phases of positive and negative PWM waveforms (six phases in total) > > that share a common wave transition point can be output by combining > > MTU3 and MTU4 and > > MTU6 and MTU7. > > > > Complementary PWM Mode: In complementary PWM mode, dead time can be > > set for PWM waveforms to be output. The dead time is the period during > > which the upper and lower arm transistors are set to the inactive > > level in order to prevent short-circuiting of the arms.Six > > positive-phase and six negative-phase PWM waveforms (12 phases in > > total)with dead time can be output by combining MTU3/ MTU4 and MTU6/MTU7. > > > > In complementary PWM mode, nine registers (compare registers, buffer > > registers, and temporary registers) are used to control the duty ratio for > the PWM output. > > Complementary PWM mode 1 (transfer at crest) Complementary PWM mode 2 > > (transfer at trough) Complementary PWM mode 3 (transfer at crest and > > trough) > > I read it five times now and still don't get it. The problem (maybe) is that > there are many abbreviations I don't understand. Most critical is: > What is a TGR? Basically it is Timer General register(TGR) functions as either output compare or input capture or buffer registers. For the PWMMode1, it is just output-compare TGRA is used for setting period and also the compare-match output is used as clearing the counter. TGRB is used for setting duty cycle. This will produce PWM output on MTIOC0A pin. The output of wave form depends upon setting on TIOR Currently it is set as, Initial output is low. High output at compare match. > > I think I understand the following: Your hardware has one(?) freerunning > counter "TCNT". TGRs get set to a value where something happens when TCNT > reaches that value. If I understand correctly, that is called Compare-match. In figure 16.27, reaching TGRA resets TCNT to 0 and the > output value to low. Reaching TGRB sets the output value to high. So far so > easy. (Apart from your description of mode 1 talking about four TGRs and two > pins which could just be two independent PWMs (in the sense of the pwm > framework)?) Exactly, in the sense of the PWM framework each IO is modelled as an independent PWM > > PWM mode 2 (figure 16.28) is the same just with more outputs. MTU1.TGRB > defines the period (as did TGRB in the above example) and there are several > other TGRx that define the duty cycle of one output each which all share the > same period. So if I restrict mode 2 to just MTU1.TRGB and MTU1.TRGA I have > mode 1. > > So I don't get the difference between the two modes. I haven't explored PWM mode 2 much. But from the description, In PWM mode2, there is only one TGR for setting period. and rest of the TGRs are used for setting duty cycle. Here MTU0 can be combined with MTU1 and MTU2 as well. So probably to differentiate PWM mode2 from PWM mode1, we need to add a property in device tree to link mtu channels for PWM mode 2 operation and then we can use pwm frame work to achieve PWM mode2 as shown in Figure 16.28. > > Describing modes (Reset-Synchronized PWM Mode and Complementary PWM > Mode) that are not relevant to the driver (yet?), doesn't help understanding > the hardware either. OK, will drop now and will revisit later. > > > > > This patch adds basic pwm mode 1 support for RZ/G2L MTU3a pwm > > > > driver by creating separate logical channels for each IOs. > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > --- > > > > v11->v12: > > > > * Updated header file to <linux/mfd/rz-mtu3.h> as core driver is in > MFD. > > > > * Reordered get_state() > > > > v10->v11: > > > > * No change. > > > > v9->v10: > > > > * No change. > > > > v8->v9: > > > > * Added prescale/duty_cycle variables to struct rz_mtu3_pwm_chip and > > > > cached this values in rz_mtu3_pwm_config and used this cached > values > > > > in get_state(), if PWM is disabled. > > > > * Added return code for get_state() > > > > v7->v8: > > > > * Simplified rz_mtu3_pwm_request by calling > > > > rz_mtu3_request_channel() > > > > * Simplified rz_mtu3_pwm_free by calling > > > > rz_mtu3_release_channel() > > > > v6->v7: > > > > * Added channel specific mutex lock to avoid race between counter > > > > device and rz_mtu3_pwm_{request,free} > > > > * Added pm_runtime_resume_and_get in rz_mtu3_pwm_enable() > > > > * Added pm_runtime_put_sync in rz_mtu3_pwm_disable() > > > > * Updated rz_mtu3_pwm_config() > > > > * Updated rz_mtu3_pwm_apply() > > > > v5->v6: > > > > * Updated commit and Kconfig description > > > > * Sorted the header > > > > * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable() > > > > * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed > > > > __maybe_unused from suspend/resume() > > > > v4->v5: > > > > * pwm device is instantiated by mtu3a core driver. > > > > v3->v4: > > > > * There is no resource associated with "rz-mtu3-pwm" compatible > > > > and moved the code to mfd subsystem as it binds against "rz-mtu". > > > > * Removed struct platform_driver rz_mtu3_pwm_driver. > > > > v2->v3: > > > > * No change. > > > > v1->v2: > > > > * Modelled as a single PWM device handling multiple channles. > > > > * Used PM framework to manage the clocks. > > > > --- > > > > drivers/pwm/Kconfig | 11 + > > > > drivers/pwm/Makefile | 1 + > > > > drivers/pwm/pwm-rz-mtu3.c | 485 > > > > ++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 497 insertions(+) create mode 100644 > > > > drivers/pwm/pwm-rz-mtu3.c > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > > > 31cdc9dae3c5..c54cbeabe093 100644 > > > > --- a/drivers/pwm/Kconfig > > > > +++ b/drivers/pwm/Kconfig > > > > @@ -492,6 +492,17 @@ config PWM_ROCKCHIP > > > > Generic PWM framework driver for the PWM controller found on > > > > Rockchip SoCs. > > > > > > > > +config PWM_RZ_MTU3 > > > > + tristate "Renesas RZ/G2L MTU3a PWM Timer support" > > > > + depends on RZ_MTU3 || COMPILE_TEST > > > > + depends on HAS_IOMEM > > > > + help > > > > + This driver exposes the MTU3a PWM Timer controller found in > Renesas > > > > + RZ/G2L like chips through the PWM API. > > > > + > > > > + To compile this driver as a module, choose M here: the module > > > > + will be called pwm-rz-mtu3. > > > > + > > > > config PWM_SAMSUNG > > > > tristate "Samsung PWM support" > > > > depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || > > > > COMPILE_TEST diff --git a/drivers/pwm/Makefile > > > > b/drivers/pwm/Makefile index a95aabae9115..6b75c0145336 100644 > > > > --- a/drivers/pwm/Makefile > > > > +++ b/drivers/pwm/Makefile > > > > @@ -45,6 +45,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > > > > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > > > > obj-$(CONFIG_PWM_RZV2M) += pwm-rzv2m.o > > > > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > > > > +obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o > > > > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > > > > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > > > > obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o > > > > diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c > > > > new file mode 100644 index 000000000000..d94e3fc36dfb > > > > --- /dev/null > > > > +++ b/drivers/pwm/pwm-rz-mtu3.c > > > > @@ -0,0 +1,485 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * Renesas RZ/G2L MTU3a PWM Timer driver > > > > + * > > > > + * Copyright (C) 2022 Renesas Electronics Corporation > > > > + * > > > > + * Hardware manual for this IP can be found here > > > > + * > > > > +https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-gro > > > > +up-u sers-manual-hardware-0?language=en > > > > + * > > > > + * Limitations: > > > > + * - When PWM is disabled, the output is driven to Hi-Z. > > > > + * - While the hardware supports both polarities, the driver (for > now) > > > > + * only handles normal polarity. > > > > + * - While the hardware supports pwm mode{1,2}, > > > > +reset-synchronized pwm > > > and > > > > + * complementary pwm modes, the driver (for now) only handles pwm > > > mode1. > > > > + */ > > > > + > > > > +#include <linux/bitfield.h> > > > > +#include <linux/clk.h> > > > > +#include <linux/limits.h> > > > > +#include <linux/mfd/rz-mtu3.h> > > > > +#include <linux/module.h> > > > > +#include <linux/platform_device.h> #include <linux/pm_runtime.h> > > > > +#include <linux/pwm.h> #include <linux/time.h> > > > > + > > > > +#define RZ_MTU3_TMDR1_MD_NORMAL (0) > > > > +#define RZ_MTU3_TMDR1_MD_PWM_MODE_1 (2) > > > > > > IMHO it would make sense to put these definitions to where > > > RZ_MTU3_TMDR1 is defined. And I'd do it like this: > > > > > > * Timer mode register 1 */ > > > #define RZ_MTU3_TMDR1 5 > > > #define RZ_MTU3_TMDR1_MD GENMASK(3, 0) > > > #define RZ_MTU3_TMDR1_MD_NORMAL > > > FIELD_PREP(RZ_MTU3_TMDR1_MD, 0) > > > #define RZ_MTU3_TMDR1_MD_PWMMODE1 > > > FIELD_PREP(RZ_MTU3_TMDR1_MD, 2) > > > > Agreed, will move to include/linux/mfd/rz-mtu3.h. > > > > > > > > > +#define RZ_MTU3_TIOR_OC_RETAIN (0) > > > > +#define RZ_MTU3_TIOR_OC_0_H_COMP_MATCH (2) > > > > +#define RZ_MTU3_TIOR_OC_1_TOGGLE (7) > > > > +#define RZ_MTU3_TIOR_OC_IOA GENMASK(3, 0) > > > > + > > > > +#define RZ_MTU3_TCR_CCLR_TGRC (5 << 5) > > > > +#define RZ_MTU3_TCR_CKEG_RISING (0 << 3) > > > > + > > > > +#define RZ_MTU3_TCR_TPCS GENMASK(2, 0) > > > > + > > > > +#define RZ_MTU3_MAX_PWM_MODE1_CHANNELS (12) > > > > + > > > > +#define RZ_MTU3_MAX_HW_PWM_CHANNELS (7) > > > > + > > > > +static const u8 rz_mtu3_pwm_mode1_num_ios[] = { 2, 1, 1, 2, 2, 2, > > > > +2 }; > > > > + > > > > +/** > > > > + * struct rz_mtu3_pwm_chip - MTU3 pwm private data > > > > + * > > > > + * @chip: MTU3 pwm chip data > > > > + * @clk: MTU3 module clock > > > > + * @lock: Lock to prevent concurrent access for usage count > > > > + * @rate: MTU3 clock rate > > > > + * @user_count: MTU3 usage count > > > > + * @rz_mtu3_channel: HW channels for the PWM */ > > > > + > > > > +struct rz_mtu3_pwm_chip { > > > > + struct pwm_chip chip; > > > > + struct clk *clk; > > > > + struct mutex lock; > > > > + unsigned long rate; > > > > + u32 user_count[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > > > > + struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > > > > + > > > > + /* > > > > + * The driver cannot read the current duty cycle/prescale from > the > > > > + * hardware if the hardware is disabled. Cache the last > programmed > > > > + * duty cycle/prescale value to return in that case. > > > > > > If the hardware is disabled, just doing .enabled = false in > > > .get_state is fine and easier. So this can be dropped I think. > > > > Yes, it can be dropped, after adding below check in get_state() > > > > + if (state->duty_cycle > state->period) > > + state->duty_cycle = state->period; > > + > > > > > > > > > + */ > > > > + u8 prescale[RZ_MTU3_MAX_HW_PWM_CHANNELS]; > > > > + unsigned int duty_cycle[RZ_MTU3_MAX_PWM_MODE1_CHANNELS]; > > > > +}; > > > > + > > > > +static inline struct rz_mtu3_pwm_chip *to_rz_mtu3_pwm_chip(struct > > > > +pwm_chip *chip) { > > > > + return container_of(chip, struct rz_mtu3_pwm_chip, chip); } > > > > + > > > > +static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip > > > *rz_mtu3, > > > > + u64 period_cycles) > > > > +{ > > > > + u32 prescaled_period_cycles; > > > > + u8 prescale; > > > > + > > > > + prescaled_period_cycles = period_cycles >> 16; > > > > + if (prescaled_period_cycles >= 16) > > > > + prescale = 3; > > > > + else > > > > + prescale = (fls(prescaled_period_cycles) + 1) / 2; > > > > + > > > > + return prescale; > > > > +} > > > > + > > > > +static struct rz_mtu3_channel * > > > > +rz_mtu3_get_hw_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 > > > > +channel) { > > > > + unsigned int i, ch_index = 0; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { > > > > + ch_index += rz_mtu3_pwm_mode1_num_ios[i]; > > > > + > > > > + if (ch_index > channel) > > > > + break; > > > > + } > > > > + > > > > + return rz_mtu3_pwm->ch[i]; > > > > +} > > > > + > > > > +static u32 rz_mtu3_get_hw_channel_index(struct rz_mtu3_pwm_chip > > > *rz_mtu3_pwm, > > > > + struct rz_mtu3_channel *ch) > > > > +{ > > > > + u32 i; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { > > > > + if (ch == rz_mtu3_pwm->ch[i]) > > > > + break; > > > > + } > > > > + > > > > + return i; > > > > +} > > > > + > > > > +static bool rz_mtu3_pwm_is_second_channel(u32 ch_index, u32 hwpwm) { > > > > + u32 i, pwm_ch_index = 0; > > > > + > > > > + for (i = 0; i < ch_index; i++) > > > > + pwm_ch_index += rz_mtu3_pwm_mode1_num_ios[i]; > > > > + > > > > + return pwm_ch_index != hwpwm; > > > > +} > > > > > > I don't understand that channel allocation, maybe worth an > > > explaining comment?! > > > > I will add below comment just above rz_mtu3_get_hw_channel(). Is it ok? > > > > +/* > > + * PWM Mode1 has MTU{0..4}, MTU6 and MTU7, Probe function skips MTU5 and > MTU8. > > What is MTU? MTU is name of the IP, stands for Multi-Function Timer Pulse Unit And index represents HW channels. > > > + * So struct rz_mtu3_channel *ch contains only PWM mode1 MTU channels. > > + * MTU{1, 2} channels has a single IO each compared to 2 IOs for the > > + rest of the > > + * channels. A LUT rz_mtu3_pwm_mode1_num_ios introduced to get the > > + PWM channel > > + * and HW channel. > > What is LUT? LUT is look up table /* * PWMMode1 channels are MTU{0, 1, 2, 3, 4, 6, 7}. MTU{1, 2} channels has a * single IO each, whereas all other channels has 2 IOs. The below LUT represent * IOs on PWMMode1 channels. */ static const u8 rz_mtu3_pwm_mode1_num_ios[] = { 2, 1, 1, 2, 2, 2, 2 }; > > These channels are about which TRG can influence which output? Yes, For PWM mode 1 case From the LUT, MTU0 has 2 IO's MTIOC0A and MTIOC0C MTU1 has 1 IO MTIOC1A MTU2 has 1 IO MTIOC2A MTU3 has 2 IO's MTIOC3A and MTIOC3C MTU4 has 2 IO's MTIOC4A and MTIOC4C MTU6 has 2 IO's MTIOC6A and MTIOC6C MTU7 has 2 IO's MTIOC7A and MTIOC7C TGRA(perioid) and TGRB(duty) can influence MTIOCA output TGRC(perioid) and TGRD(duty) can influence MTIOCD output > > > + */ > > > > > > > > > +static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip > > > *rz_mtu3_pwm, > > > > + u32 hwpwm) > > > > +{ > > > > + struct rz_mtu3_channel *ch; > > > > + bool is_channel_en; > > > > + u32 ch_index; > > > > + u8 val; > > > > + > > > > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, hwpwm); > > > > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > > > > + is_channel_en = rz_mtu3_is_enabled(ch); > > > > + > > > > + if (rz_mtu3_pwm_is_second_channel(ch_index, hwpwm)) > > > > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORL); > > > > + else > > > > + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORH); > > > > + > > > > + return (is_channel_en && (val & RZ_MTU3_TIOR_OC_IOA)); } > > > > + > > > > [...] > > > > + /* > > > > + * If the PWM channel is disabled, make sure to turn on the > clock > > > > + * before writing the register. > > > > + */ > > > > + if (!pwm->state.enabled) { > > > > + err = pm_runtime_resume_and_get(chip->dev); > > > > + if (err) > > > > + return err; > > > > + } > > > > > > Maybe it's easier to call pm_runtime_resume_and_get() unconditionally? > > > > OK, will use below unconditional call instead. Is it ok? > > > > pm_runtime_get_sync(chip->dev); > > Try it and if it's simpler stick to it, if not, my suggestion was wrong. OK, will do. > > > > > + val = RZ_MTU3_TCR_CKEG_RISING | prescale; > > > > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { > > > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > > > > + RZ_MTU3_TCR_CCLR_TGRC | val); > > > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRD, dc); > > > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRC, pv); > > > > + } else { > > > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > > > > + RZ_MTU3_TCR_CCLR_TGRA | val); > > > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRB, dc); > > > > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRA, pv); > > > > + } > > > > + > > > > [...] > > > > +static DEFINE_RUNTIME_DEV_PM_OPS(rz_mtu3_pwm_pm_ops, > > > > + rz_mtu3_pwm_pm_runtime_suspend, > > > > + rz_mtu3_pwm_pm_runtime_resume, NULL); > > > > + > > > > +static void rz_mtu3_pwm_pm_disable(void *data) { > > > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = data; > > > > + > > > > + pm_runtime_disable(rz_mtu3_pwm->chip.dev); > > > > + pm_runtime_set_suspended(rz_mtu3_pwm->chip.dev); > > > > +} > > > > + > > > > +static int rz_mtu3_pwm_probe(struct platform_device *pdev) { > > > > + struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent); > > > > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm; > > > > + struct device *dev = &pdev->dev; > > > > + int num_pwm_hw_ch = 0; > > > > + unsigned int i; > > > > + int ret; > > > > + > > > > + rz_mtu3_pwm = devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm), > > > GFP_KERNEL); > > > > + if (!rz_mtu3_pwm) > > > > + return -ENOMEM; > > > > + > > > > + rz_mtu3_pwm->clk = ddata->clk; > > > > + rz_mtu3_pwm->rate = clk_get_rate(rz_mtu3_pwm->clk); > > > > > > Note that clk_get_rate isn't reliable for disabled clocks, so please > > > enable first and then call clk_get_rate(). Also consider calling > > > clk_rate_exclusive_get(). > > > > > > OK, will call get_rate() after enable. Runtime PM use > > clockenable/disable Frequently, so unnecessary to use > clk_rate_exclusive_{get,put}. Is it ok? > > One doesn't have to do with the other. After calling > clk_rate_exclusive_get() you can be sure that no other driver does anything > to change the clk which would mess with the emitted signals. > > I don't know the exact semantics of clk_rate_exclusive_get(), but I'd assume > that even if you disable the clock you should be able to rely on it running > at the same rate after reenable. I believe usage of clk_rate_exclusive_get() is platform Dependent. We don't support any frequency scaling other than cpufreq scaling. Peripheral may have internal dividers, but the frequency of peripherals are fixed. If platforms having QoS, ie, based on the performance change the OPP for the particular peripheral domain it makes sense to use clk_rate_exclusive_get(). One such platform which support QoS in my knowledge is ST U8500 platform.(+ linus Wallej) If I remember correctly, Based on the demand peripherals can request frequency and it sets the corresponding OPP (eg: low power audio, USB etc). Here it is not the case. Please let me know still you prefer clk_rate_exclusive_get()? Cheers, Biju > > > > > + for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) { > > > > + if (i == RZ_MTU5 || i == RZ_MTU8) > > > > + continue; > > > > + > > > > + rz_mtu3_pwm->ch[num_pwm_hw_ch] = &ddata->channels[i]; > > > > + rz_mtu3_pwm->ch[num_pwm_hw_ch]->dev = dev; > > > > + num_pwm_hw_ch++; > > > > + } > > > > + > > > > + mutex_init(&rz_mtu3_pwm->lock); > > > > + platform_set_drvdata(pdev, rz_mtu3_pwm); > > > > > > This is unused. > > > > Nope. It used in runtime PM calls. > > Oh, indeed. Looking that up I saw that you didn't check the return values of > clk_disable_unprepare() and clk_prepare_enable() in them. > > > > > + pm_runtime_set_active(&pdev->dev); > > > > + pm_runtime_enable(&pdev->dev); > > > > + ret = devm_add_action_or_reset(&pdev->dev, > > > > + rz_mtu3_pwm_pm_disable, > > > > + rz_mtu3_pwm); > > > > + if (ret < 0) > > > > + goto disable_clock; > > > > + > > > > + rz_mtu3_pwm->chip.dev = &pdev->dev; > > > > + rz_mtu3_pwm->chip.ops = &rz_mtu3_pwm_ops; > > > > + rz_mtu3_pwm->chip.npwm = RZ_MTU3_MAX_PWM_MODE1_CHANNELS; > > > > + ret = devm_pwmchip_add(&pdev->dev, &rz_mtu3_pwm->chip); > > > > + if (ret) { > > > > + dev_err_probe(&pdev->dev, ret, "failed to add PWM > chip\n"); > > > > + goto disable_clock; > > > > + } > > > > + > > > > + return 0; > > > > + > > > > +disable_clock: > > > > + clk_disable_unprepare(rz_mtu3_pwm->clk); > > > > + return ret; > > > > +} > > > > > > On .remove the clk isn't disabled. > > > > It is not required. It is already disabled after probe. > > > > Clock_enable_prepare enables the clk during probe, Then later PM > > runtime suspend turns off the clock using clk_prepare_disable(). > > > > On error case, in probe, PM suspend won't get called, So we need to > > use clk_prepare_disable(). > > > > After probe: > > [ 15.680492] rzg2l-cpg 11010000.clock-controller: CLK_ON 1336/mtu_x_mck > ON > > [ 15.725015] rzg2l-cpg 11010000.clock-controller: CLK_ON 1336/mtu_x_mck > OFF > > devmem2 0x11010538 | grep Read > > > > Unbind and bind call reports, balanced clk usage. > > > > Please correct me, if I am wrong or need any clarification. > > As I missed the PM callbacks you are right I think. (I didn't recheck the > logic now though.) > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Biju, On Thu, Feb 16, 2023 at 10:06:42AM +0000, Biju Das wrote: > > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver > > On Wed, Feb 15, 2023 at 07:14:12PM +0000, Biju Das wrote: > > > > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM > > > > driver > > > > > > > > On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > > > > > Add support for RZ/G2L MTU3a PWM driver. The IP supports following > > > > > PWM modes > > > > > > > > > > 1) PWM mode{1,2} > > > > > 2) Reset-synchronized PWM mode > > > > > 3) Complementary PWM mode{1,2,3} > > > > > > > > It's unclear to me what "PWM mode1" and the other modes are. I > > > > suspect this is some chip specific naming that isn't understandable > > > > for outsiders? Would be great to explain that a bit more. > > > > > > Ok I will add below to Limitation sections. Is it ok? > > > > > > PWM Mode 1: PWM waveforms are output from the MTIOCnA and MTIOCnC pins > > > by pairing TGRA with TGRB and TGRC with TGRD. The levels specified by > > > the TIOR.IOA[3:0] and IOC[3:0] bits are output from the MTIOCnA and > > > MTIOCnC pins at compare matches A and C, and the level specified by > > > the TIOR.IOB[3:0] and IOD[3:0] bits are output at compare matches B and D > > (n = 0 to 4, 6, 7). > > > > > > > > > PWM Mode 2: PWM waveform output is generated using one TGR as the > > > cycle register and the others as duty registers. > > > > > > Reset-Synchronized PWM Mode: In the reset-synchronized PWM mode, three > > > phases of positive and negative PWM waveforms (six phases in total) > > > that share a common wave transition point can be output by combining > > > MTU3 and MTU4 and > > > MTU6 and MTU7. > > > > > > Complementary PWM Mode: In complementary PWM mode, dead time can be > > > set for PWM waveforms to be output. The dead time is the period during > > > which the upper and lower arm transistors are set to the inactive > > > level in order to prevent short-circuiting of the arms.Six > > > positive-phase and six negative-phase PWM waveforms (12 phases in > > > total)with dead time can be output by combining MTU3/ MTU4 and MTU6/MTU7. > > > > > > In complementary PWM mode, nine registers (compare registers, buffer > > > registers, and temporary registers) are used to control the duty ratio for > > the PWM output. > > > Complementary PWM mode 1 (transfer at crest) Complementary PWM mode 2 > > > (transfer at trough) Complementary PWM mode 3 (transfer at crest and > > > trough) > > > > I read it five times now and still don't get it. The problem (maybe) is that > > there are many abbreviations I don't understand. Most critical is: > > What is a TGR? > > Basically it is Timer General register(TGR) functions as either output compare or > input capture or buffer registers. > > For the PWMMode1, it is just output-compare > > TGRA is used for setting period and also the compare-match output is used as clearing the counter. > TGRB is used for setting duty cycle. > > This will produce PWM output on MTIOC0A pin. > > The output of wave form depends upon setting on TIOR > Currently it is set as, Initial output is low. High output at compare match. Then I suggest to not talk at all about mode 1 or mode 2. Just mention that you use one counter and two match components to configure duty_cycle and period. > > > OK, will call get_rate() after enable. Runtime PM use > > > clockenable/disable Frequently, so unnecessary to use > > > clk_rate_exclusive_{get,put}. Is it ok? > > > > One doesn't have to do with the other. After calling > > clk_rate_exclusive_get() you can be sure that no other driver does anything > > to change the clk which would mess with the emitted signals. > > > > I don't know the exact semantics of clk_rate_exclusive_get(), but I'd assume > > that even if you disable the clock you should be able to rely on it running > > at the same rate after reenable. > > I believe usage of clk_rate_exclusive_get() is platform > Dependent. Yeah, it depends on the platform if the clk will actually change behind your back. But making it explicit that the clk must not change is a good idea in general. Your driver might be used later on a platform where it matters or it might be used as a template for the next pwm driver. Best regards Uwe
Hi Uwe, Thanks for the feedback. > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver > > Hello Biju, > > On Thu, Feb 16, 2023 at 10:06:42AM +0000, Biju Das wrote: > > > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM > > > driver On Wed, Feb 15, 2023 at 07:14:12PM +0000, Biju Das wrote: > > > > > Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM > > > > > driver > > > > > > > > > > On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote: > > > > > > Add support for RZ/G2L MTU3a PWM driver. The IP supports > > > > > > following PWM modes > > > > > > > > > > > > 1) PWM mode{1,2} > > > > > > 2) Reset-synchronized PWM mode > > > > > > 3) Complementary PWM mode{1,2,3} > > > > > > > > > > It's unclear to me what "PWM mode1" and the other modes are. I > > > > > suspect this is some chip specific naming that isn't > > > > > understandable for outsiders? Would be great to explain that a bit > more. > > > > > > > > Ok I will add below to Limitation sections. Is it ok? > > > > > > > > PWM Mode 1: PWM waveforms are output from the MTIOCnA and MTIOCnC > > > > pins by pairing TGRA with TGRB and TGRC with TGRD. The levels > > > > specified by the TIOR.IOA[3:0] and IOC[3:0] bits are output from > > > > the MTIOCnA and MTIOCnC pins at compare matches A and C, and the > > > > level specified by the TIOR.IOB[3:0] and IOD[3:0] bits are output > > > > at compare matches B and D > > > (n = 0 to 4, 6, 7). > > > > > > > > > > > > PWM Mode 2: PWM waveform output is generated using one TGR as the > > > > cycle register and the others as duty registers. > > > > > > > > Reset-Synchronized PWM Mode: In the reset-synchronized PWM mode, > > > > three phases of positive and negative PWM waveforms (six phases in > > > > total) that share a common wave transition point can be output by > > > > combining > > > > MTU3 and MTU4 and > > > > MTU6 and MTU7. > > > > > > > > Complementary PWM Mode: In complementary PWM mode, dead time can > > > > be set for PWM waveforms to be output. The dead time is the period > > > > during which the upper and lower arm transistors are set to the > > > > inactive level in order to prevent short-circuiting of the > > > > arms.Six positive-phase and six negative-phase PWM waveforms (12 > > > > phases in total)with dead time can be output by combining MTU3/ MTU4 > and MTU6/MTU7. > > > > > > > > In complementary PWM mode, nine registers (compare registers, > > > > buffer registers, and temporary registers) are used to control the > > > > duty ratio for > > > the PWM output. > > > > Complementary PWM mode 1 (transfer at crest) Complementary PWM > > > > mode 2 (transfer at trough) Complementary PWM mode 3 (transfer at > > > > crest and > > > > trough) > > > > > > I read it five times now and still don't get it. The problem (maybe) > > > is that there are many abbreviations I don't understand. Most critical > is: > > > What is a TGR? > > > > Basically it is Timer General register(TGR) functions as either output > > compare or input capture or buffer registers. > > > > For the PWMMode1, it is just output-compare > > > > TGRA is used for setting period and also the compare-match output is used > as clearing the counter. > > TGRB is used for setting duty cycle. > > > > This will produce PWM output on MTIOC0A pin. > > > > The output of wave form depends upon setting on TIOR Currently it is > > set as, Initial output is low. High output at compare match. > > Then I suggest to not talk at all about mode 1 or mode 2. Just mention that > you use one counter and two match components to configure duty_cycle and > period. Agreed. > > > > > OK, will call get_rate() after enable. Runtime PM use > > > > clockenable/disable Frequently, so unnecessary to use > > > > clk_rate_exclusive_{get,put}. Is it ok? > > > > > > One doesn't have to do with the other. After calling > > > clk_rate_exclusive_get() you can be sure that no other driver does > > > anything to change the clk which would mess with the emitted signals. > > > > > > I don't know the exact semantics of clk_rate_exclusive_get(), but > > > I'd assume that even if you disable the clock you should be able to > > > rely on it running at the same rate after reenable. > > > > I believe usage of clk_rate_exclusive_get() is platform Dependent. > > Yeah, it depends on the platform if the clk will actually change behind your > back. But making it explicit that the clk must not change is a good idea in > general. Your driver might be used later on a platform where it matters or > it might be used as a template for the next pwm driver. OK, will use clk_rate_exclusive_get(). Cheers, Biju
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 31cdc9dae3c5..c54cbeabe093 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -492,6 +492,17 @@ config PWM_ROCKCHIP Generic PWM framework driver for the PWM controller found on Rockchip SoCs. +config PWM_RZ_MTU3 + tristate "Renesas RZ/G2L MTU3a PWM Timer support" + depends on RZ_MTU3 || COMPILE_TEST + depends on HAS_IOMEM + help + This driver exposes the MTU3a PWM Timer controller found in Renesas + RZ/G2L like chips through the PWM API. + + To compile this driver as a module, choose M here: the module + will be called pwm-rz-mtu3. + config PWM_SAMSUNG tristate "Samsung PWM support" depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index a95aabae9115..6b75c0145336 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -45,6 +45,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o obj-$(CONFIG_PWM_RZV2M) += pwm-rzv2m.o obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o +obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c new file mode 100644 index 000000000000..d94e3fc36dfb --- /dev/null +++ b/drivers/pwm/pwm-rz-mtu3.c @@ -0,0 +1,485 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Renesas RZ/G2L MTU3a PWM Timer driver + * + * Copyright (C) 2022 Renesas Electronics Corporation + * + * Hardware manual for this IP can be found here + * https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?language=en + * + * Limitations: + * - When PWM is disabled, the output is driven to Hi-Z. + * - While the hardware supports both polarities, the driver (for now) + * only handles normal polarity. + * - While the hardware supports pwm mode{1,2}, reset-synchronized pwm and + * complementary pwm modes, the driver (for now) only handles pwm mode1. + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/limits.h> +#include <linux/mfd/rz-mtu3.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/pwm.h> +#include <linux/time.h> + +#define RZ_MTU3_TMDR1_MD_NORMAL (0) +#define RZ_MTU3_TMDR1_MD_PWM_MODE_1 (2) + +#define RZ_MTU3_TIOR_OC_RETAIN (0) +#define RZ_MTU3_TIOR_OC_0_H_COMP_MATCH (2) +#define RZ_MTU3_TIOR_OC_1_TOGGLE (7) +#define RZ_MTU3_TIOR_OC_IOA GENMASK(3, 0) + +#define RZ_MTU3_TCR_CCLR_TGRC (5 << 5) +#define RZ_MTU3_TCR_CKEG_RISING (0 << 3) + +#define RZ_MTU3_TCR_TPCS GENMASK(2, 0) + +#define RZ_MTU3_MAX_PWM_MODE1_CHANNELS (12) + +#define RZ_MTU3_MAX_HW_PWM_CHANNELS (7) + +static const u8 rz_mtu3_pwm_mode1_num_ios[] = { 2, 1, 1, 2, 2, 2, 2 }; + +/** + * struct rz_mtu3_pwm_chip - MTU3 pwm private data + * + * @chip: MTU3 pwm chip data + * @clk: MTU3 module clock + * @lock: Lock to prevent concurrent access for usage count + * @rate: MTU3 clock rate + * @user_count: MTU3 usage count + * @rz_mtu3_channel: HW channels for the PWM + */ + +struct rz_mtu3_pwm_chip { + struct pwm_chip chip; + struct clk *clk; + struct mutex lock; + unsigned long rate; + u32 user_count[RZ_MTU3_MAX_HW_PWM_CHANNELS]; + struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_PWM_CHANNELS]; + + /* + * The driver cannot read the current duty cycle/prescale from the + * hardware if the hardware is disabled. Cache the last programmed + * duty cycle/prescale value to return in that case. + */ + u8 prescale[RZ_MTU3_MAX_HW_PWM_CHANNELS]; + unsigned int duty_cycle[RZ_MTU3_MAX_PWM_MODE1_CHANNELS]; +}; + +static inline struct rz_mtu3_pwm_chip *to_rz_mtu3_pwm_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct rz_mtu3_pwm_chip, chip); +} + +static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip *rz_mtu3, + u64 period_cycles) +{ + u32 prescaled_period_cycles; + u8 prescale; + + prescaled_period_cycles = period_cycles >> 16; + if (prescaled_period_cycles >= 16) + prescale = 3; + else + prescale = (fls(prescaled_period_cycles) + 1) / 2; + + return prescale; +} + +static struct rz_mtu3_channel * +rz_mtu3_get_hw_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 channel) +{ + unsigned int i, ch_index = 0; + + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { + ch_index += rz_mtu3_pwm_mode1_num_ios[i]; + + if (ch_index > channel) + break; + } + + return rz_mtu3_pwm->ch[i]; +} + +static u32 rz_mtu3_get_hw_channel_index(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, + struct rz_mtu3_channel *ch) +{ + u32 i; + + for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) { + if (ch == rz_mtu3_pwm->ch[i]) + break; + } + + return i; +} + +static bool rz_mtu3_pwm_is_second_channel(u32 ch_index, u32 hwpwm) +{ + u32 i, pwm_ch_index = 0; + + for (i = 0; i < ch_index; i++) + pwm_ch_index += rz_mtu3_pwm_mode1_num_ios[i]; + + return pwm_ch_index != hwpwm; +} + +static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, + u32 hwpwm) +{ + struct rz_mtu3_channel *ch; + bool is_channel_en; + u32 ch_index; + u8 val; + + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, hwpwm); + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); + is_channel_en = rz_mtu3_is_enabled(ch); + + if (rz_mtu3_pwm_is_second_channel(ch_index, hwpwm)) + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORL); + else + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORH); + + return (is_channel_en && (val & RZ_MTU3_TIOR_OC_IOA)); +} + +static int rz_mtu3_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); + struct rz_mtu3_channel *ch; + u32 ch_index; + + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); + if (!rz_mtu3_pwm->user_count[ch_index] && !rz_mtu3_request_channel(ch)) + return -EBUSY; + + mutex_lock(&rz_mtu3_pwm->lock); + rz_mtu3_pwm->user_count[ch_index]++; + mutex_unlock(&rz_mtu3_pwm->lock); + + return 0; +} + +static void rz_mtu3_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); + struct rz_mtu3_channel *ch; + u32 ch_index; + + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); + + mutex_lock(&rz_mtu3_pwm->lock); + rz_mtu3_pwm->user_count[ch_index]--; + mutex_unlock(&rz_mtu3_pwm->lock); + + if (!rz_mtu3_pwm->user_count[ch_index]) + rz_mtu3_release_channel(ch); +} + +static int rz_mtu3_pwm_enable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, + struct pwm_device *pwm) +{ + struct rz_mtu3_channel *ch; + u32 ch_index; + u8 val; + int rc; + + rc = pm_runtime_resume_and_get(rz_mtu3_pwm->chip.dev); + if (rc) + return rc; + + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); + val = (RZ_MTU3_TIOR_OC_1_TOGGLE << 4) | RZ_MTU3_TIOR_OC_0_H_COMP_MATCH; + + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_PWM_MODE_1); + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, val); + else + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, val); + + if (rz_mtu3_pwm->user_count[ch_index] <= 1) + rz_mtu3_enable(ch); + + return 0; +} + +static void rz_mtu3_pwm_disable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, + struct pwm_device *pwm) +{ + struct rz_mtu3_channel *ch; + u32 ch_index; + + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); + + /* Return to normal mode and disable output pins of MTU3 channel */ + if (rz_mtu3_pwm->user_count[ch_index] <= 1) + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_NORMAL); + + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, RZ_MTU3_TIOR_OC_RETAIN); + else + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, RZ_MTU3_TIOR_OC_RETAIN); + + if (rz_mtu3_pwm->user_count[ch_index] <= 1) + rz_mtu3_disable(ch); + + pm_runtime_put_sync(rz_mtu3_pwm->chip.dev); +} + +static int rz_mtu3_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); + struct rz_mtu3_channel *ch; + u8 prescale, val; + u32 ch_index; + u16 dc, pv; + u64 tmp; + + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); + pm_runtime_get_sync(chip->dev); + state->enabled = rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, pwm->hwpwm); + if (state->enabled) { + val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TCR); + prescale = FIELD_GET(RZ_MTU3_TCR_TPCS, val); + + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { + dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRD); + pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRC); + } else { + dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRB); + pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRA); + } + + tmp = NSEC_PER_SEC * (u64)pv << (2 * prescale); + state->period = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate); + } else { + /* If the PWM is disabled, use the cached value. */ + prescale = rz_mtu3_pwm->prescale[ch_index]; + dc = rz_mtu3_pwm->duty_cycle[pwm->hwpwm]; + } + + tmp = NSEC_PER_SEC * (u64)dc << (2 * prescale); + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate); + state->polarity = PWM_POLARITY_NORMAL; + pm_runtime_put(chip->dev); + + return 0; +} + +static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); + struct rz_mtu3_channel *ch; + unsigned long pv, dc; + u64 period_cycles; + u64 duty_cycles; + u32 ch_index; + u8 prescale; + int err; + u8 val; + + /* + * Refuse clk rates > 1 GHz to prevent overflowing the following + * calculation. + */ + if (rz_mtu3_pwm->rate > NSEC_PER_SEC) + return -EINVAL; + + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); + period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate, + NSEC_PER_SEC); + prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm, period_cycles); + + if (period_cycles >> (2 * prescale) <= U16_MAX) + pv = period_cycles >> (2 * prescale); + else + pv = U16_MAX; + + duty_cycles = mul_u64_u32_div(state->duty_cycle, rz_mtu3_pwm->rate, + NSEC_PER_SEC); + if (duty_cycles >> (2 * prescale) <= U16_MAX) + dc = duty_cycles >> (2 * prescale); + else + dc = U16_MAX; + + /* + * Store the duty cycle/prescale for future reference in cases where the + * corresponding registers can't be read (i.e. when the PWM is disabled). + */ + rz_mtu3_pwm->prescale[ch_index] = prescale; + rz_mtu3_pwm->duty_cycle[pwm->hwpwm] = dc; + + /* + * If the PWM channel is disabled, make sure to turn on the clock + * before writing the register. + */ + if (!pwm->state.enabled) { + err = pm_runtime_resume_and_get(chip->dev); + if (err) + return err; + } + + val = RZ_MTU3_TCR_CKEG_RISING | prescale; + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, + RZ_MTU3_TCR_CCLR_TGRC | val); + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRD, dc); + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRC, pv); + } else { + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, + RZ_MTU3_TCR_CCLR_TGRA | val); + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRB, dc); + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRA, pv); + } + + /* If the PWM is not enabled, turn the clock off again to save power. */ + if (!pwm->state.enabled) + pm_runtime_put(chip->dev); + + return 0; +} + +static int rz_mtu3_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip); + bool enabled = pwm->state.enabled; + int ret; + + if (state->polarity != PWM_POLARITY_NORMAL) + return -EINVAL; + + if (!state->enabled) { + if (enabled) + rz_mtu3_pwm_disable(rz_mtu3_pwm, pwm); + + return 0; + } + + ret = rz_mtu3_pwm_config(chip, pwm, state); + if (ret) + return ret; + + if (!enabled) + ret = rz_mtu3_pwm_enable(rz_mtu3_pwm, pwm); + + return ret; +} + +static const struct pwm_ops rz_mtu3_pwm_ops = { + .request = rz_mtu3_pwm_request, + .free = rz_mtu3_pwm_free, + .get_state = rz_mtu3_pwm_get_state, + .apply = rz_mtu3_pwm_apply, + .owner = THIS_MODULE, +}; + +static int rz_mtu3_pwm_pm_runtime_suspend(struct device *dev) +{ + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = dev_get_drvdata(dev); + + clk_disable_unprepare(rz_mtu3_pwm->clk); + + return 0; +} + +static int rz_mtu3_pwm_pm_runtime_resume(struct device *dev) +{ + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = dev_get_drvdata(dev); + + clk_prepare_enable(rz_mtu3_pwm->clk); + + return 0; +} + +static DEFINE_RUNTIME_DEV_PM_OPS(rz_mtu3_pwm_pm_ops, + rz_mtu3_pwm_pm_runtime_suspend, + rz_mtu3_pwm_pm_runtime_resume, NULL); + +static void rz_mtu3_pwm_pm_disable(void *data) +{ + struct rz_mtu3_pwm_chip *rz_mtu3_pwm = data; + + pm_runtime_disable(rz_mtu3_pwm->chip.dev); + pm_runtime_set_suspended(rz_mtu3_pwm->chip.dev); +} + +static int rz_mtu3_pwm_probe(struct platform_device *pdev) +{ + struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent); + struct rz_mtu3_pwm_chip *rz_mtu3_pwm; + struct device *dev = &pdev->dev; + int num_pwm_hw_ch = 0; + unsigned int i; + int ret; + + rz_mtu3_pwm = devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm), GFP_KERNEL); + if (!rz_mtu3_pwm) + return -ENOMEM; + + rz_mtu3_pwm->clk = ddata->clk; + rz_mtu3_pwm->rate = clk_get_rate(rz_mtu3_pwm->clk); + for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) { + if (i == RZ_MTU5 || i == RZ_MTU8) + continue; + + rz_mtu3_pwm->ch[num_pwm_hw_ch] = &ddata->channels[i]; + rz_mtu3_pwm->ch[num_pwm_hw_ch]->dev = dev; + num_pwm_hw_ch++; + } + + mutex_init(&rz_mtu3_pwm->lock); + platform_set_drvdata(pdev, rz_mtu3_pwm); + clk_prepare_enable(rz_mtu3_pwm->clk); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + ret = devm_add_action_or_reset(&pdev->dev, + rz_mtu3_pwm_pm_disable, + rz_mtu3_pwm); + if (ret < 0) + goto disable_clock; + + rz_mtu3_pwm->chip.dev = &pdev->dev; + rz_mtu3_pwm->chip.ops = &rz_mtu3_pwm_ops; + rz_mtu3_pwm->chip.npwm = RZ_MTU3_MAX_PWM_MODE1_CHANNELS; + ret = devm_pwmchip_add(&pdev->dev, &rz_mtu3_pwm->chip); + if (ret) { + dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); + goto disable_clock; + } + + return 0; + +disable_clock: + clk_disable_unprepare(rz_mtu3_pwm->clk); + return ret; +} + +static struct platform_driver rz_mtu3_pwm_driver = { + .driver = { + .name = "pwm-rz-mtu3", + .pm = pm_ptr(&rz_mtu3_pwm_pm_ops), + }, + .probe = rz_mtu3_pwm_probe, +}; +module_platform_driver(rz_mtu3_pwm_driver); + +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>"); +MODULE_ALIAS("platform:pwm-rz-mtu3"); +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a PWM Timer Driver"); +MODULE_LICENSE("GPL");
Add support for RZ/G2L MTU3a PWM driver. The IP supports following PWM modes 1) PWM mode{1,2} 2) Reset-synchronized PWM mode 3) Complementary PWM mode{1,2,3} This patch adds basic pwm mode 1 support for RZ/G2L MTU3a pwm driver by creating separate logical channels for each IOs. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v11->v12: * Updated header file to <linux/mfd/rz-mtu3.h> as core driver is in MFD. * Reordered get_state() v10->v11: * No change. v9->v10: * No change. v8->v9: * Added prescale/duty_cycle variables to struct rz_mtu3_pwm_chip and cached this values in rz_mtu3_pwm_config and used this cached values in get_state(), if PWM is disabled. * Added return code for get_state() v7->v8: * Simplified rz_mtu3_pwm_request by calling rz_mtu3_request_channel() * Simplified rz_mtu3_pwm_free by calling rz_mtu3_release_channel() v6->v7: * Added channel specific mutex lock to avoid race between counter device and rz_mtu3_pwm_{request,free} * Added pm_runtime_resume_and_get in rz_mtu3_pwm_enable() * Added pm_runtime_put_sync in rz_mtu3_pwm_disable() * Updated rz_mtu3_pwm_config() * Updated rz_mtu3_pwm_apply() v5->v6: * Updated commit and Kconfig description * Sorted the header * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable() * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed __maybe_unused from suspend/resume() v4->v5: * pwm device is instantiated by mtu3a core driver. v3->v4: * There is no resource associated with "rz-mtu3-pwm" compatible and moved the code to mfd subsystem as it binds against "rz-mtu". * Removed struct platform_driver rz_mtu3_pwm_driver. v2->v3: * No change. v1->v2: * Modelled as a single PWM device handling multiple channles. * Used PM framework to manage the clocks. --- drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-rz-mtu3.c | 485 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 497 insertions(+) create mode 100644 drivers/pwm/pwm-rz-mtu3.c