Hi Uwe,
Thanks for the feedback.
> Subject: Re: [PATCH v14 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver
>
> Hello,
>
> I have the feeling this driver is more complicated than necessary.
I agree, your suggestions made it less complex.
>
> On Fri, Mar 10, 2023 at 05:06:54PM +0000, Biju Das wrote:
> > The RZ/G2L Multi-Function Timer Pulse Unit 3 (a.k.a MTU3a) uses one
> > counter and two match components to configure duty_cycle and period to
> > generate PWM output waveform.
> >
> > Add basic support for RZ/G2L MTU3a PWM driver by creating separate PWM
> > channels for each IOs.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v13->v14:
> > * Updated commit description
> > * Updated Limitations section.
> > * Replaced the macros RZ_MTU*->RZ_MTU3_CHAN_* in probe()
> > * Fixed a kernel crash in error path by moving rz_mtu3_pwm->chip.dev
> before
> > devm_add_action_or_reset()
> > * Added pm_runtime_idle() and simplified error paths for
> devm_add_action_or_reset()
> > and devm_pwmchip_add().
> >
> > v12->v13:
> > * Updated commit description
> > * Moved RZ_MTU3_TMDR1_MD_* macros to rz_mtu3.h
> > * Updated Limitations section.
> > * Removed PWM mode1 references from the driver.
> > * Dropped prescale and duty_cycle from struct rz_mtu3_pwm_chip.
> > * Replaced rz_mtu3_pwm_mode1_num_ios->rz_mtu3_hw_channel_ios.
> > * Avoided race condition in rz_mtu3_pwm_request()/rz_mtu3_pwm_free().
> > * Updated get_state() by adding dc > pv check and added a comment about
> > overflow condition.
> > * Moved overflow condition check from config->probe()
> > * Replaced pm_runtime_resume_and_get with unconditional
> pm_runtime_get_sync()
> > in config()
> > * Added error check for clk_prepare_enable() in probe() and propagating
> error
> > to the caller for pm_runtime_resume()
> > * clk_get_rate() is called after enabling the clock and
> > clk_rate_exclusive_put()
> > 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 | 482
> > ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 494 insertions(+)
> > create mode 100644 drivers/pwm/pwm-rz-mtu3.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > dae023d783a2..ccc0299fd0dd 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -481,6 +481,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 7bf1a29f02b8..b85fc9fba326 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -44,6 +44,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE) += pwm-raspberrypi-
> poe.o
> > obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
> > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> > +obj-$(CONFIG_PWM_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..146948656755
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rz-mtu3.c
> > @@ -0,0 +1,482 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L MTU3a PWM Timer driver
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corporation
>
> 2023?
OK.
>
> > + *
> > + * 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.
> > + * - HW uses one counter and two match components to configure duty_cycle
> > + * and period.
> > + * - Multi-Function Timer Pulse Unit(a.k.a MTU) has 7 HW channels for
> > + PWM
>
> s/t(/t (/
Done.
>
> > + * operations(The channels are MTU{0..4, 6, 7}).
>
> s/s(/s. (/; s/)./.)/
Done.
>
> > + * - MTU{1, 2} channels have a single IO, whereas all other HW channels
> have
> > + * 2 IOs.
> > + * - Each IO is modelled as an independent PWM channel.
> > + * - rz_mtu3_hw_channel_ios table is used to map the PWM channel to the
> > + * corresponding HW channel as there are difference in number of IOs
> > + * between HW channels.
> > + */
> > +
> > +#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_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)
>
> I assume the other register definitions are in <linux/mfd/rz-mtu3.h>, I
> suggest to move these there, too.
Yep moved the definitions.
/* Macros for setting registers */
+#define RZ_MTU3_TCR_CCLR GENMASK(7, 5)
+#define RZ_MTU3_TCR_CKEG GENMASK(4, 3)
+#define RZ_MTU3_TCR_TPCS GENMASK(2, 0)
#define RZ_MTU3_TCR_CCLR_TGRA BIT(5)
+#define RZ_MTU3_TCR_CCLR_TGRC FIELD_PREP(RZ_MTU3_TCR_CCLR, 5)
+#define RZ_MTU3_TCR_CKEG_RISING FIELD_PREP(RZ_MTU3_TCR_CKEG, 0)
+
+#define RZ_MTU3_TIOR_IOB GENMASK(7, 4)
+#define RZ_MTU3_TIOR_IOA GENMASK(3, 0)
+#define RZ_MTU3_TIOR_OC_RETAIN (0)
+#define RZ_MTU3_TIOR_OC_INIT_OUT_LO_HI_OUT (2)
+#define RZ_MTU3_TIOR_OC_INIT_OUT_HI_TOGGLE_OUT (7)
+
+#define RZ_MTU3_TIOR_OC_IOA_H_COMP_MATCH \
+ FIELD_PREP(RZ_MTU3_TIOR_IOA, RZ_MTU3_TIOR_OC_INIT_OUT_LO_HI_OUT)
+#define RZ_MTU3_TIOR_OC_IOB_TOGGLE \
+ FIELD_PREP(RZ_MTU3_TIOR_IOB, RZ_MTU3_TIOR_OC_INIT_OUT_HI_TOGGLE_OUT)
>
> > +
> > +#define RZ_MTU3_MAX_PWM_CHANNELS (12)
> > +
> > +#define RZ_MTU3_MAX_HW_CHANNELS (7)
> > +
> > +/* The table represents the number of IOs on each MTU HW channel. */
> > +static const u8 rz_mtu3_hw_channel_ios[] = { 2, 1, 1, 2, 2, 2, 2 };
>
> OK, so you have twelve PWM outputs, each one is driven by a "channel".
> Each channel drives one or two PWMs according to the above array, right?
Yes that is correct.
Now as per your suggestion, it is mapped differently.
+/**
+ * struct rz_mtu3_channel_io_map - MTU3 channel io map
+ *
+ * @pwm_index: Index of the lowest pwm channel
+ * @num: number of IOs on the channel.
+ */
+
+struct rz_mtu3_channel_io_map {
+ u8 pwm_index;
+ u8 num;
+};
+
+static const struct rz_mtu3_channel_io_map channel_map[] =
+ { { 0, 2 }, { 2, 1 }, { 3, 1 }, { 4 , 2 }, { 6 ,2 } , { 8, 2 }, { 10, 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_CHANNELS];
> > + struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_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_hw_channel_ios); i++) {
> > + ch_index += rz_mtu3_hw_channel_ios[i];
> > +
> > + if (ch_index > channel)
> > + break;
> > + }
> > +
> > + return rz_mtu3_pwm->ch[i];
> > +}
>
> This function determines the channel that drives pwm $channel. Maybe pick
> more sensible names? Something like "hwpwm" instead of "channel"
> and "channel" instead of "hw_channel" (and "ch"). I think the driver would
> be easier to understand if the naming was used in a consistent way.
OK.
>
> > +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_hw_channel_ios); i++) {
> > + if (ch == rz_mtu3_pwm->ch[i])
> > + break;
> > + }
> > +
> > + return i;
> > +}
>
> This is a complicated way to express
>
> ch - rz_mtu3_pwm->ch
>
> isn't it?
Agreed.
>
> > +
> > +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_hw_channel_ios[i];
> > +
> > + return pwm_ch_index != hwpwm;
> > +}
>
> I think the three functions above could be simplified (or not needed) if you
> represent the channel -> pwm mapping in a different way.
>
> I'd do something like:
>
> ch = rz_mtu3_pwm_get_channel(..., hwpwm);
>
> and in *ch store the index of the lowest PWM and the number of PWMs handled
> by this channel.
Agreed defined below structure to take care this.
+/**
+ * struct rz_mtu3_pwm_channel - MTU3 pwm channel
+ *
+ * @rz_mtu3_channel: HW channel for the PWM
+ * @pwm_index: Index of the pwm channel
+ * @num: number of IOs on the channel.
+ */
+struct rz_mtu3_pwm_channel {
+ struct rz_mtu3_channel *mtu;
+ u8 pwm_index;
+ u8 num;
+};
@@ -67,9 +67,26 @@ struct rz_mtu3_pwm_chip {
struct mutex lock;
unsigned long rate;
u32 user_count[RZ_MTU3_MAX_HW_CHANNELS];
- struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_CHANNELS];
+ struct rz_mtu3_pwm_channel channel[RZ_MTU3_MAX_HW_CHANNELS];
};
static struct rz_mtu3_pwm_channel *
+rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
{
+ unsigned int i;
+ for (i = 0; i < RZ_MTU3_MAX_HW_CHANNELS; i++) {
+ if (rz_mtu3_pwm->channel[i].pwm_index + rz_mtu3_pwm->channel[i].num > hwpwm)
break;
}
+ return &rz_mtu3_pwm->channel[i];
}
>
> > +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));
>
> You could exit early (and so skip reading from the hardware) if
> is_channel_en is false.
Agreed.
>
> > +}
> > +
> > +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;
> > +
> > + mutex_lock(&rz_mtu3_pwm->lock);
> > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
> > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
>
> ch and ch_index can be determined before taking the lock, can it not?
Agreed.
>
> I'd only work with a single variable that holds the value of your ch_index,
> call it "ch" and use rz_mtu3_pwm->channel[ch]. This is simpler and so easier
> to understand for a code reader.
>
> > + if (!rz_mtu3_pwm->user_count[ch_index] &&
> !rz_mtu3_request_channel(ch)) {
> > + mutex_unlock(&rz_mtu3_pwm->lock);
> > + return -EBUSY;
> > + }
>
> This would be easier to read if written as follows:
>
> /*
> * Each channel must be requested only once, so if the channel
> * serves two PWMs and the other is already requested, skip over
> * rz_mtu3_request_channel()
> */
> if (!rz_mtu3_pwm->user_count[ch_index]) {
> ret = rz_mtu3_request_channel(ch);
> if (ret)
> return ret; /* or -EBUSY? */
> }
Ok, it is rewritten as
+ if (!rz_mtu3_pwm->user_count[index]) {
+ ch_idle = rz_mtu3_request_channel(ch->mtu);
+ if (!ch_idle) {
+ mutex_unlock(&rz_mtu3_pwm->lock);
+ return -EBUSY;
+ }
}
Where index = ch - rz_mtu3_pwm->channel;
>
> > +
> > + 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;
> > +
> > + mutex_lock(&rz_mtu3_pwm->lock);
> > + ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
> > + ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
> > +
> > + rz_mtu3_pwm->user_count[ch_index]--;
> > + if (!rz_mtu3_pwm->user_count[ch_index])
> > + rz_mtu3_release_channel(ch);
> > +
> > + mutex_unlock(&rz_mtu3_pwm->lock);
> > +}
> > +
> > +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;
>
> Introduce a symbol for the 4 above? This is the only use of
> RZ_MTU3_TIOR_OC_1_TOGGLE, maybe move the shift to the definition of that
> symbol?
Done.
+ val = RZ_MTU3_TIOR_OC_IOB_TOGGLE | RZ_MTU3_TIOR_OC_IOA_H_COMP_MATCH;
>
> > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_PWMMODE1);
> > + 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);
>
> That looks wrong. user_count signals if the channel is used for one or two
> PWMs, right? If both PWMs are requested before any of them is enabled,
> rz_mtu3_enable is never called, isn't it?
OK, Introduced enable_count to take care this in enable/disable function.
@@ -67,9 +67,26 @@ struct rz_mtu3_pwm_chip {
struct mutex lock;
unsigned long rate;
u32 user_count[RZ_MTU3_MAX_HW_CHANNELS];
+ u32 enable_count[RZ_MTU3_MAX_HW_CHANNELS];
};
Enable:
+ mutex_lock(&rz_mtu3_pwm->lock);
+ if (!rz_mtu3_pwm->enable_count[index])
+ rz_mtu3_enable(ch->mtu);
+
+ rz_mtu3_pwm->enable_count[index]++;
+ mutex_unlock(&rz_mtu3_pwm->lock);
Disable:
+ mutex_lock(&rz_mtu3_pwm->lock);
+ rz_mtu3_pwm->enable_count[index]--;
+ if (!rz_mtu3_pwm->enable_count[index])
+ rz_mtu3_disable(ch->mtu);
+ mutex_unlock(&rz_mtu3_pwm->lock);
>
> > + 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) {
>
> Most variables can have a narrower scope and be declared here.
OK.
>
> > + 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);
> > + }
> > +
> > + /* 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);
> > + tmp = NSEC_PER_SEC * (u64)dc << (2 * prescale);
> > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate);
> > + }
> > +
> > + if (state->duty_cycle > state->period)
> > + state->duty_cycle = state->period;
> > +
> > + 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;
> > + u8 val;
> > +
> > + 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;
> > +
> > + /*
> > + * If the PWM channel is disabled, make sure to turn on the clock
> > + * before writing the register.
> > + */
> > + if (!pwm->state.enabled)
> > + 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);
> > +
> > + return clk_prepare_enable(rz_mtu3_pwm->clk);
> > +}
> > +
> > +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;
> > +
> > + clk_rate_exclusive_put(rz_mtu3_pwm->clk);
> > + 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;
> > +
> > + for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) {
> > + if (i == RZ_MTU3_CHAN_5 || i == RZ_MTU3_CHAN_8)
> > + 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);
> > + ret = clk_prepare_enable(rz_mtu3_pwm->clk);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Clock enable failed\n");
> > +
> > + clk_rate_exclusive_get(rz_mtu3_pwm->clk);
> > +
> > + rz_mtu3_pwm->rate = clk_get_rate(rz_mtu3_pwm->clk);
> > + /*
> > + * Refuse clk rates > 1 GHz to prevent overflow later for computing
> > + * period and duty cycle.
> > + */
> > + if (rz_mtu3_pwm->rate > NSEC_PER_SEC) {
> > + ret = -EINVAL;
> > + clk_rate_exclusive_put(rz_mtu3_pwm->clk);
> > + goto disable_clock;
> > + }
> > +
> > + pm_runtime_set_active(&pdev->dev);
> > + pm_runtime_enable(&pdev->dev);
> > + rz_mtu3_pwm->chip.dev = &pdev->dev;
> > + ret = devm_add_action_or_reset(&pdev->dev,
> > + rz_mtu3_pwm_pm_disable,
> > + rz_mtu3_pwm);
> > + if (ret < 0)
> > + return ret;
> > +
> > + rz_mtu3_pwm->chip.ops = &rz_mtu3_pwm_ops;
> > + rz_mtu3_pwm->chip.npwm = RZ_MTU3_MAX_PWM_CHANNELS;
> > + ret = devm_pwmchip_add(&pdev->dev, &rz_mtu3_pwm->chip);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret, "failed to add PWM
> chip\n");
> > +
> > + pm_runtime_idle(&pdev->dev);
> > +
> > + return 0;
> > +
> > +disable_clock:
> > + clk_disable_unprepare(rz_mtu3_pwm->clk);
> > + return ret;
> > +}
>
> .probe() enables rz_mtu3_pwm->clk, but there is no .remove() that cares
> about disabling it again.
I believe it is ok. I have added WARN_ON for double checking.
Please correct me, if it is wrong.
Probe:
Clock enable by "clk_prepare_enable"
[ 6.923881] rzg2l_mod_clock_endisable+0x130/0x180
[ 6.929295] rzg2l_mod_clock_enable+0x54/0x70
[ 6.934229] clk_core_enable+0xdc/0x278
[ 6.938599] clk_enable+0x28/0x44
[ 6.942403] rz_mtu3_pwm_probe+0xf0/0x230
[ 6.946944] platform_probe+0x64/0xcc
Clock disable by " pm_runtime_idle"
[ 7.283961] rzg2l_mod_clock_endisable+0x130/0x180
[ 7.289383] rzg2l_mod_clock_disable+0x50/0x5c
[ 7.294415] clk_core_disable+0x8c/0xd8
[ 7.298787] clk_disable+0x2c/0x44
[ 7.302685] rz_mtu3_pwm_pm_runtime_suspend+0x1c/0x34
[ 7.308361] pm_generic_runtime_suspend+0x28/0x3c
[ 7.313667] __rpm_callback+0x44/0x134
[ 7.317936] rpm_callback+0x64/0x70
[ 7.321920] rpm_suspend+0x130/0x628
[ 7.325999] rpm_idle+0x118/0x2c0
[ 7.329796] __pm_runtime_idle+0x4c/0x84
[ 7.334250] rz_mtu3_pwm_probe+0x1b8/0x230
Unbind:
cd /sys/bus/platform/drivers/pwm-rz-mtu3/
echo pwm-rz-mtu3.0 > unbind
Clock enable by "__pm_runtime_resume"
[ 263.003787] rzg2l_mod_clock_endisable+0x130/0x180
[ 263.009196] rzg2l_mod_clock_enable+0x54/0x70
[ 263.014122] clk_core_enable+0xdc/0x278
[ 263.018484] clk_enable+0x28/0x44
[ 263.022278] rz_mtu3_pwm_pm_runtime_resume+0x3c/0x5c
[ 263.027850] pm_generic_runtime_resume+0x28/0x3c
[ 263.033054] __rpm_callback+0x44/0x134
[ 263.037314] rpm_callback+0x64/0x70
[ 263.041287] rpm_resume+0x428/0x6a0
[ 263.045260] __pm_runtime_resume+0x50/0x74
[ 263.049893] device_release_driver_internal+0xcc/0x234
[ 263.055670] device_driver_detach+0x14/0x1c
Clock disable by "__pm_runtime_idle"
[ 263.304970] rzg2l_mod_clock_endisable+0x130/0x180
[ 263.309812] rzg2l_mod_clock_disable+0x50/0x5c
[ 263.314301] clk_core_disable+0x8c/0xd8
[ 263.318180] clk_disable+0x2c/0x44
[ 263.321620] rz_mtu3_pwm_pm_runtime_suspend+0x1c/0x34
[ 263.326720] pm_generic_runtime_suspend+0x28/0x3c
[ 263.331472] __rpm_callback+0x44/0x134
[ 263.335261] rpm_callback+0x64/0x70
[ 263.338787] rpm_suspend+0x130/0x628
[ 263.342400] rpm_idle+0x118/0x2c0
[ 263.345751] __pm_runtime_idle+0x4c/0x84
[ 263.349713] device_release_driver_internal+0x1d0/0x234
Please correct me if anything wrong or clarifications needed.
Cheers,
Biju
>
> > +
> > +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@xxxxxxxxxxxxxx>");
> > +MODULE_ALIAS("platform:pwm-rz-mtu3");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a PWM Timer Driver");
> > +MODULE_LICENSE("GPL");
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
