The RZ/V2{M, MA} PWM Timer (PWM) is composed of 16 channels.
Linux is only allowed access to channels 8 to 14 on RZ/V2M,
while there is no restriction for RZ/V2MA.
The RZ/V2{M, MA} PWM Timer (PWM) supports the following functions:
* The PWM has 24-bit counters which operate at PWM_CLK (48 MHz).
* The frequency division ratio for internal counter operation is
selectable as PWM_CLK divided by 1, 16, 256, or 2048.
* The period as well as the duty cycle is adjustable.
* The low-level and high-level order of the PWM signals can be
inverted.
* The duty cycle of the PWM signal is selectable in the range from
0 to 100%.
* The minimum resolution is 20.83 ns.
* Three interrupt sources: Rising and falling edges of the PWM signal
and clearing of the counter.
* Counter operation and the bus interface are asynchronous and both can
operate independently of the magnitude relationship of the respective
clock periods.
v6->v7:
* Addressed the build issue reported by the kernel test robot.
* Replaced / with div64_u64 in driver.
* Added rzv2m_pwm_mul_u64_u64_div_u64_rounddown to driver.
v5->v6:
* Updated copyright in driver (2023->2024).
* Several improvements to the driver, as suggested by Uwe.
v4->v5:
* rebased to pwm for-next.
* Sorted KConfig file
* Sorted Make file
* Updated copyright header 2022->2023.
* Updated limitation section.
* Replaced the variable chip->rzv2m_pwm in rzv2m_pwm_wait_delay()
* Replaced polarity logic as per HW manual dutycycle = Ton/Ton+Toff, so
eventhough native polarity is inverted from period point of view it
is correct.
* Updated logic for supporting 0% , 100% and remaining duty cycles.
* On config() replaced
pm_runtime_resume_and_get()->pm_runtime_get_sync()
* Counter is stopped while updating period/polarity to avoid glitches.
* Added error check for clk_prepare_enable()
* Introduced is_ch_enabled variable to cache channel enable status.
* clk_get_rate is called after enabling the clock and
clk_rate_exclusive_get()
* Added comment for delay
* Replaced 1000000000UL->NSEC_PER_SEC.
* Improved error handling in probe().
v3->v4:
* Documented the hardware properties in "Limitations" section
* Dropped the macros F2CYCLE_NSEC, U24_MASK and U24_MAX.
* Added RZV2M_PWMCYC_PERIOD macro for U24_MAX
* Dropped rzv2m_pwm_freq_div variable and started using 1 << (4 * i)
for calculating divider as it is power of 16.
* Reordered the functions to have rzv2m_pwm_config() directly before
rzv2m_pwm_apply().
* Improved the logic for calculating period and duty cycle in config()
* Merged multiple RZV2M_PWMCTR register writes to a single write in
config()
* Replaced pwm_is_enabled()->pwm->state.enabled
* Avoided assigning bit value as enum pwm_polarity instead used enum
constant.
* Fixed various issues in probe error path.
* Updated the logic for PWM cycle setting register
* A 100% duty cycle is only possible with PWMLOW > PWMCYC. So
restricting PWMCYC values < 0xffffff
* The native polarity of the hardware is inverted (i.e. it starts with
the low part). So switched the inversion bit handling.
v2->v3:
* Removed clock patch#1 as it is queued for 6.3 renesas-clk
* Added Rb tag from Geert for bindings and dt patches
* Added return code for rzv2m_pwm_get_state()
* Added comment in rzv2m_pwm_reset_assert_pm_disable()
v1->v2:
* Updated commit description
* Replaced pwm8_15_pclk->cperi_grpf
* Added reset entry R9A09G011_PWM_GPF_PRESETN
* Added Rb tag from Krzysztof for bindings and the keep the Rb tag as
the below changes are trivial
* Updated the description for APB clock
* Added resets required property
* Updated the example with resets property
* Replaced
devm_reset_control_get_optional_shared->devm_reset_control_get_shared
* Added resets property in pwm nodes.
Biju Das (4):
dt-bindings: pwm: Add RZ/V2M PWM binding
pwm: Add support for RZ/V2M PWM driver
arm64: dts: renesas: r9a09g011: Add pwm nodes
arm64: dts: renesas: rzv2m evk: Enable pwm
.../bindings/pwm/renesas,rzv2m-pwm.yaml | 90 ++++
.../boot/dts/renesas/r9a09g011-v2mevk2.dts | 70 +++
arch/arm64/boot/dts/renesas/r9a09g011.dtsi | 98 ++++
drivers/pwm/Kconfig | 11 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-rzv2m.c | 480 ++++++++++++++++++
6 files changed, 750 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/renesas,rzv2m-pwm.yaml
create mode 100644 drivers/pwm/pwm-rzv2m.c
--
2.34.1
From: Biju Das <[email protected]>
Add device tree bindings for the RZ/V2{M, MA} PWM Timer (PWM).
Signed-off-by: Biju Das <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
v6->v7:
* No change
v5->v6:
* No change
v4->v5:
* No change
v3->v4:
* No change
v2->v3:
* Added Rb tag from Geert.
v1->v2:
* Added Rb tag from Krzysztof and the keep the Rb tag as the below
* changes
are trivial
* Updated the description for APB clock
* Added resets required property
* Updated the example with resets property
.../bindings/pwm/renesas,rzv2m-pwm.yaml | 90 +++++++++++++++++++
1 file changed, 90 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/renesas,rzv2m-pwm.yaml
diff --git a/Documentation/devicetree/bindings/pwm/renesas,rzv2m-pwm.yaml b/Documentation/devicetree/bindings/pwm/renesas,rzv2m-pwm.yaml
new file mode 100644
index 000000000000..ddeed7550923
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/renesas,rzv2m-pwm.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/renesas,rzv2m-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2{M, MA} PWM Timer (PWM)
+
+maintainers:
+ - Biju Das <[email protected]>
+
+description: |
+ The RZ/V2{M, MA} PWM Timer (PWM) composed of 16 channels. It supports the
+ following functions
+ * The PWM has 24-bit counters which operate at PWM_CLK (48 MHz).
+ * The frequency division ratio for internal counter operation is selectable
+ as PWM_CLK divided by 1, 16, 256, or 2048.
+ * The period as well as the duty cycle is adjustable.
+ * The low-level and high-level order of the PWM signals can be inverted.
+ * The duty cycle of the PWM signal is selectable in the range from 0 to 100%.
+ * The minimum resolution is 20.83 ns.
+ * Three interrupt sources: Rising and falling edges of the PWM signal and
+ clearing of the counter
+ * Counter operation and the bus interface are asynchronous and both can
+ operate independently of the magnitude relationship of the respective
+ clock periods.
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - renesas,r9a09g011-pwm # RZ/V2M
+ - renesas,r9a09g055-pwm # RZ/V2MA
+ - const: renesas,rzv2m-pwm
+
+ reg:
+ maxItems: 1
+
+ '#pwm-cells':
+ const: 2
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: APB clock
+ - description: PWM clock
+
+ clock-names:
+ items:
+ - const: apb
+ - const: pwm
+
+ power-domains:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - power-domains
+ - resets
+
+allOf:
+ - $ref: pwm.yaml#
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/r9a09g011-cpg.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ pwm8: pwm@a4010400 {
+ compatible = "renesas,r9a09g011-pwm", "renesas,rzv2m-pwm";
+ reg = <0xa4010400 0x80>;
+ interrupts = <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD R9A09G011_CPERI_GRPF_PCLK>,
+ <&cpg CPG_MOD R9A09G011_PWM8_CLK>;
+ clock-names = "apb", "pwm";
+ power-domains = <&cpg>;
+ resets = <&cpg R9A09G011_PWM_GPF_PRESETN>;
+ #pwm-cells = <2>;
+ };
--
2.34.1
From: Biju Das <[email protected]>
The RZ/V2{M, MA} PWM Timer supports the following functions:
* The PWM has 24-bit counters which operate at PWM_CLK (48 MHz).
* The frequency division ratio for internal counter operation is
selectable as PWM_CLK divided by 1, 16, 256, or 2048.
* The period as well as the duty cycle is adjustable.
* The low-level and high-level order of the PWM signals can be
inverted.
* The duty cycle of the PWM signal is selectable in the range from
0 to 100%.
* The minimum resolution is 20.83 ns.
* Three interrupt sources: Rising and falling edges of the PWM signal
and clearing of the counter
* Counter operation and the bus interface are asynchronous and both
can operate independently of the magnitude relationship of the
respective clock periods.
Signed-off-by: Biju Das <[email protected]>
Signed-off-by: Fabrizio Castro <[email protected]>
---
v6->v7:
* Addressed the build issue reported by the kernel test robot.
* Added include math64.h.
* Reworked rzv2m_pwm_mul_u64_u64_div_u64_roundup to make use of
div64_u64 and to get rid of % while keeping the same formula.
* Added rzv2m_pwm_mul_u64_u64_div_u64_rounddown.
* Replaced / with div64_u64 wherever necessary.
v5->v6:
* Added Fab's Signed-off-by.
* Updated copyright year to 2024.
* Added include of limits.h.
* Added variable max_period to rzv2m_pwm_chip.
* Simplified the calculations by calculating max_period during probe,
based on the numerical limits of the formula and the u64 data type.
* Added rzv2m_pwm_mul_u64_u64_div_u64_roundup.
* Added rzv2m_pwm_prescale_to_shift to fix the calculation of the
frequency divider.
* Improved the calculations and the variable names of
rzv2m_pwm_get_state.
* Improved the calculations of rzv2m_pwm_config.
* Removed .owner from rzv2m_pwm_ops.
* Improved rzv2m_pwm_pm_runtime_resume and renamed its err variable to
ret.
* Removed of_match_ptr.
* Added Fab as module author.
v4->v5:
* Sorted KConfig file
* Sorted Make file
* Updated copyright header 2022->2023.
* Updated limitation section.
* Replaced the variable chip->rzv2m_pwm in rzv2m_pwm_wait_delay()
* Replaced polarity logic as per HW manual dutycycle = Ton/Ton+Toff, so
eventhough native polarity is inverted from period point of view it
is correct.
* Added logic for supporting 0% , 100% and remaining duty cycle.
* On config() replaced
pm_runtime_resume_and_get()->pm_runtime_get_sync()
* Counter is stopped while updating period/polarity to avoid glitches.
* Added error check for clk_prepare_enable()
* Introduced is_ch_enabled variable to cache channel enable status.
* clk_get_rate is called after enabling the clock and
clk_rate_exclusive_get()
* Added comment for delay
* Replaced 1000000000UL->NSEC_PER_SEC.
* Improved error handling in probe().
v3->v4:
* Documented the hardware properties in "Limitations" section
* Dropped the macros F2CYCLE_NSEC, U24_MASK and U24_MAX.
* Added RZV2M_PWMCYC_PERIOD macro for U24_MAX
* Dropped rzv2m_pwm_freq_div variable and started using 1 << (4 * i)
for calculating divider as it is power of 16.
* Reordered the functions to have rzv2m_pwm_config() directly before
rzv2m_pwm_apply().
* Improved the logic for calculating period and duty cycle in config()
* Merged multiple RZV2M_PWMCTR register writes to a single write in
* config()
* replaced pwm_is_enabled()->pwm->state.enabled
* Avoided assigning bit value as enum pwm_polarity instead used enum
* constant.
* Fixed various issues in probe error path.
* Updated the logic for PWM cycle setting register
* A 100% duty cycle is only possible with PWMLOW > PWMCYC. So
restricting PWMCYC values < 0xffffff
* The native polarity of the hardware is inverted (i.e. it starts with
the low part). So switched the inversion bit handling.
v2->v3:
* Added return code for rzv2m_pwm_get_state()
* Added comment in rzv2m_pwm_reset_assert_pm_disable()
v1->v2:
* Replaced
devm_reset_control_get_optional_shared->devm_reset_control_get_shared
drivers/pwm/Kconfig | 11 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-rzv2m.c | 480 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 492 insertions(+)
create mode 100644 drivers/pwm/pwm-rzv2m.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..55d46e6183a2 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -524,6 +524,17 @@ config PWM_RZ_MTU3
To compile this driver as a module, choose M here: the module
will be called pwm-rz-mtu3.
+config PWM_RZV2M
+ tristate "Renesas RZ/V2M PWM support"
+ depends on ARCH_R9A09G011 || COMPILE_TEST
+ depends on HAS_IOMEM
+ help
+ This driver exposes the PWM controller found in Renesas
+ RZ/V2M like chips through the PWM API.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-rzv2m.
+
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 c5ec9e168ee7..cf5a4a1c3b1a 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -48,6 +48,7 @@ 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_RZV2M) += pwm-rzv2m.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-rzv2m.c b/drivers/pwm/pwm-rzv2m.c
new file mode 100644
index 000000000000..eb9062293590
--- /dev/null
+++ b/drivers/pwm/pwm-rzv2m.c
@@ -0,0 +1,480 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2M PWM Timer (PWM) driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corporation
+ *
+ * Hardware manual for this IP can be found here
+ * https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardware?language=en
+ *
+ * Limitations:
+ * - Changes to the duty cycle configuration get effective only after the next
+ * period end.
+ * - The duty cycle can be changed only by modifying the PWMLOW register
+ * value and changing the pulse width at low level. The duty cycle becomes
+ * 0% for the low width when the value of the PWMLOW register is 0x0h
+ * and 100% for the low width when the value of the PWMLOW > PWMCYC.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/limits.h>
+#include <linux/math64.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/time.h>
+
+#define RZV2M_PWMCTR 0x0
+#define RZV2M_PWMCYC 0x4
+#define RZV2M_PWMLOW 0x8
+#define RZV2M_PWMCNT 0xc
+
+#define RZV2M_PWMCTR_PWMPS GENMASK(17, 16)
+#define RZV2M_PWMCTR_PWMHL BIT(3)
+#define RZV2M_PWMCTR_PWMTM BIT(2)
+#define RZV2M_PWMCTR_PWME BIT(1)
+
+#define RZV2M_PWMCYC_PERIOD GENMASK(23, 0)
+#define RZV2M_PWMLOW_PERIOD GENMASK(23, 0)
+
+struct rzv2m_pwm_chip {
+ u64 max_period;
+ struct pwm_chip chip;
+ void __iomem *mmio;
+ struct reset_control *rstc;
+ struct clk *apb_clk;
+ struct clk *pwm_clk;
+ unsigned long rate;
+ unsigned long delay;
+ unsigned long pwm_cyc;
+ enum pwm_polarity polarity;
+ bool is_ch_enabled;
+};
+
+static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c)
+{
+ u64 ab = a * b;
+ u64 d = div64_u64(ab, c);
+ u64 e = d * c;
+
+ return d + ((ab - e) ? 1 : 0);
+}
+
+static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_rounddown(u64 a, u64 b, u64 c)
+{
+ return div64_u64(a * b, c);
+}
+
+static inline struct rzv2m_pwm_chip *to_rzv2m_pwm_chip(struct pwm_chip *chip)
+{
+ return container_of(chip, struct rzv2m_pwm_chip, chip);
+}
+
+static void rzv2m_pwm_wait_delay(struct rzv2m_pwm_chip *rzv2m_pwm)
+{
+ /* delay timer when change the setting register */
+ ndelay(rzv2m_pwm->delay);
+}
+
+static void rzv2m_pwm_write(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg, u32 data)
+{
+ writel(data, rzv2m_pwm->mmio + reg);
+}
+
+static u32 rzv2m_pwm_read(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg)
+{
+ return readl(rzv2m_pwm->mmio + reg);
+}
+
+static void rzv2m_pwm_modify(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg, u32 clr,
+ u32 set)
+{
+ rzv2m_pwm_write(rzv2m_pwm, reg,
+ (rzv2m_pwm_read(rzv2m_pwm, reg) & ~clr) | set);
+}
+
+static u8 rzv2m_pwm_calculate_prescale(struct rzv2m_pwm_chip *rzv2m_pwm,
+ u64 period_cycles)
+{
+ u32 prescaled_period_cycles;
+ u8 prescale;
+
+ prescaled_period_cycles = period_cycles >> 24;
+ if (prescaled_period_cycles >= 256)
+ prescale = 3;
+ else
+ prescale = (fls(prescaled_period_cycles) + 3) / 4;
+
+ return prescale;
+}
+
+static inline int rzv2m_pwm_prescale_to_shift(u8 prescale)
+{
+ return prescale == 3 ? 11 : prescale * 4;
+}
+
+static int rzv2m_pwm_enable(struct rzv2m_pwm_chip *rzv2m_pwm)
+{
+ int rc;
+
+ rc = pm_runtime_resume_and_get(rzv2m_pwm->chip.dev);
+ if (rc)
+ return rc;
+
+ rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
+ RZV2M_PWMCTR_PWME);
+ rzv2m_pwm_wait_delay(rzv2m_pwm);
+ rzv2m_pwm->is_ch_enabled = true;
+
+ return 0;
+}
+
+static void rzv2m_pwm_disable(struct rzv2m_pwm_chip *rzv2m_pwm)
+{
+ rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME, 0);
+ rzv2m_pwm_wait_delay(rzv2m_pwm);
+ pm_runtime_put_sync(rzv2m_pwm->chip.dev);
+ rzv2m_pwm->is_ch_enabled = false;
+}
+
+static int rzv2m_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
+ u16 frequency_divisor;
+ u32 ctr, cyc, low;
+ u8 prescale;
+
+ pm_runtime_get_sync(chip->dev);
+ ctr = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
+ state->enabled = FIELD_GET(RZV2M_PWMCTR_PWME, ctr);
+ state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, ctr) ?
+ PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+ prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, ctr);
+ frequency_divisor = 1 << rzv2m_pwm_prescale_to_shift(prescale);
+
+ cyc = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
+ state->period = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1,
+ NSEC_PER_SEC * frequency_divisor,
+ rzv2m_pwm->rate);
+
+ low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
+ state->duty_cycle = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1 - low,
+ NSEC_PER_SEC * frequency_divisor,
+ rzv2m_pwm->rate);
+
+ return pm_runtime_put(chip->dev);
+}
+
+static int rzv2m_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
+ u64 period = state->period, duty_cycle = state->duty_cycle;
+ u16 frequency_divisor;
+ u64 pwm_cyc, pwm_low;
+ u8 prescale;
+ u32 pwm_ctr;
+
+ /*
+ * Clamp period and duty cycle to their maximum values for our current
+ * configuration rather than letting our calculations overflow.
+ */
+ if (period > rzv2m_pwm->max_period) {
+ period = rzv2m_pwm->max_period;
+ if (duty_cycle > rzv2m_pwm->max_period)
+ duty_cycle = period;
+ }
+
+ /*
+ * Formula for calculating PWM Cycle Setting Register:
+ * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) × Div ratio)) - 1
+ */
+ pwm_cyc = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(period,
+ rzv2m_pwm->rate,
+ NSEC_PER_SEC);
+ pwm_cyc = pwm_cyc ? pwm_cyc : 1;
+
+ prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pwm_cyc - 1);
+ frequency_divisor = 1 << rzv2m_pwm_prescale_to_shift(prescale);
+ if (frequency_divisor > 1) {
+ pwm_cyc = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(period,
+ rzv2m_pwm->rate,
+ NSEC_PER_SEC * frequency_divisor);
+ pwm_cyc = pwm_cyc ? pwm_cyc : 1;
+ }
+
+ if (pwm_cyc && !FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc - 1))
+ pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;
+
+ /*
+ * Formula for calculating PWMLOW register:
+ * PWMLOW register = PWM cycle * Low pulse width ratio (%)
+ */
+ pwm_low = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(duty_cycle,
+ rzv2m_pwm->rate, NSEC_PER_SEC * frequency_divisor);
+
+ pwm_low = pwm_cyc - pwm_low;
+ if (!FIELD_FIT(RZV2M_PWMLOW_PERIOD, pwm_low))
+ pwm_low = RZV2M_PWMLOW_PERIOD;
+
+ pwm_cyc--;
+
+ /*
+ * 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(rzv2m_pwm->chip.dev);
+
+ /*
+ * To change the setting value of the PWM cycle setting register
+ * (PWMm_PWMCYC) or polarity, set the PWME bit of the PWM control
+ * register (PWMm_PWMCTR) to 0b and stop the counter operation.
+ */
+ if (rzv2m_pwm->polarity != state->polarity || rzv2m_pwm->pwm_cyc != pwm_cyc) {
+ rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME, 0);
+ rzv2m_pwm_wait_delay(rzv2m_pwm);
+ }
+
+ rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMCYC, pwm_cyc);
+ rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMLOW, pwm_low);
+
+ pwm_ctr = FIELD_PREP(RZV2M_PWMCTR_PWMPS, prescale);
+ if (state->polarity == PWM_POLARITY_INVERSED)
+ pwm_ctr |= RZV2M_PWMCTR_PWMHL;
+
+ rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMTM |
+ RZV2M_PWMCTR_PWMPS | RZV2M_PWMCTR_PWMHL, pwm_ctr);
+
+ if (rzv2m_pwm->polarity != state->polarity || rzv2m_pwm->pwm_cyc != pwm_cyc) {
+ rzv2m_pwm->polarity = state->polarity;
+ rzv2m_pwm->pwm_cyc = pwm_cyc;
+ rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
+ RZV2M_PWMCTR_PWME);
+ }
+
+ rzv2m_pwm_wait_delay(rzv2m_pwm);
+
+ /* If the PWM is not enabled, turn the clock off again to save power. */
+ if (!pwm->state.enabled)
+ pm_runtime_put(rzv2m_pwm->chip.dev);
+
+ return 0;
+}
+
+static int rzv2m_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
+ bool enabled = pwm->state.enabled;
+ int ret;
+
+ if (!state->enabled) {
+ if (enabled)
+ rzv2m_pwm_disable(rzv2m_pwm);
+
+ return 0;
+ }
+
+ ret = rzv2m_pwm_config(chip, pwm, state);
+ if (ret)
+ return ret;
+
+ if (!enabled)
+ ret = rzv2m_pwm_enable(rzv2m_pwm);
+
+ return ret;
+}
+
+static const struct pwm_ops rzv2m_pwm_ops = {
+ .get_state = rzv2m_pwm_get_state,
+ .apply = rzv2m_pwm_apply,
+};
+
+static int rzv2m_pwm_pm_runtime_suspend(struct device *dev)
+{
+ struct rzv2m_pwm_chip *rzv2m_pwm = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(rzv2m_pwm->pwm_clk);
+ clk_disable_unprepare(rzv2m_pwm->apb_clk);
+
+ return 0;
+}
+
+static int rzv2m_pwm_pm_runtime_resume(struct device *dev)
+{
+ struct rzv2m_pwm_chip *rzv2m_pwm = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(rzv2m_pwm->apb_clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(rzv2m_pwm->pwm_clk);
+ if (ret)
+ clk_disable_unprepare(rzv2m_pwm->apb_clk);
+
+ return ret;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(rzv2m_pwm_pm_ops,
+ rzv2m_pwm_pm_runtime_suspend,
+ rzv2m_pwm_pm_runtime_resume, NULL);
+
+static void rzv2m_pwm_reset_assert_pm_disable(void *data)
+{
+ struct rzv2m_pwm_chip *rzv2m_pwm = data;
+
+ /*
+ * The below check is for making balanced PM usage count in probe/remove
+ * eg: boot loader is turning on PWM and probe increments the PM usage
+ * count. Before apply, if there is unbind/remove callback we need to
+ * decrement the PM usage count.
+ */
+ if (rzv2m_pwm->is_ch_enabled)
+ pm_runtime_put(rzv2m_pwm->chip.dev);
+
+ clk_rate_exclusive_put(rzv2m_pwm->pwm_clk);
+ clk_rate_exclusive_put(rzv2m_pwm->apb_clk);
+ pm_runtime_disable(rzv2m_pwm->chip.dev);
+ pm_runtime_set_suspended(rzv2m_pwm->chip.dev);
+ reset_control_assert(rzv2m_pwm->rstc);
+}
+
+static int rzv2m_pwm_probe(struct platform_device *pdev)
+{
+ struct rzv2m_pwm_chip *rzv2m_pwm;
+ unsigned long apb_clk_rate;
+ int ret;
+
+ rzv2m_pwm = devm_kzalloc(&pdev->dev, sizeof(*rzv2m_pwm), GFP_KERNEL);
+ if (!rzv2m_pwm)
+ return -ENOMEM;
+
+ rzv2m_pwm->mmio = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(rzv2m_pwm->mmio))
+ return PTR_ERR(rzv2m_pwm->mmio);
+
+ rzv2m_pwm->apb_clk = devm_clk_get(&pdev->dev, "apb");
+ if (IS_ERR(rzv2m_pwm->apb_clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->apb_clk),
+ "cannot get apb clock\n");
+
+ rzv2m_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
+ if (IS_ERR(rzv2m_pwm->pwm_clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->pwm_clk),
+ "cannot get pwm clock\n");
+
+ rzv2m_pwm->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
+ if (IS_ERR(rzv2m_pwm->rstc))
+ return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->rstc),
+ "get reset failed\n");
+
+ platform_set_drvdata(pdev, rzv2m_pwm);
+ ret = reset_control_deassert(rzv2m_pwm->rstc);
+ if (ret) {
+ return dev_err_probe(&pdev->dev, ret,
+ "cannot deassert reset control\n");
+ }
+
+ ret = clk_prepare_enable(rzv2m_pwm->apb_clk);
+ if (ret < 0)
+ goto err_reset;
+
+ ret = clk_prepare_enable(rzv2m_pwm->pwm_clk);
+ if (ret < 0)
+ goto disable_apb_clk;
+
+ clk_rate_exclusive_get(rzv2m_pwm->apb_clk);
+ clk_rate_exclusive_get(rzv2m_pwm->pwm_clk);
+ apb_clk_rate = clk_get_rate(rzv2m_pwm->apb_clk);
+ if (!apb_clk_rate)
+ goto err_rate_put;
+
+ rzv2m_pwm->rate = clk_get_rate(rzv2m_pwm->pwm_clk);
+ if (!rzv2m_pwm->rate)
+ goto err_rate_put;
+ rzv2m_pwm->max_period = div64_u64(U64_MAX, rzv2m_pwm->rate);
+
+ /*
+ * The registers other than the PWM interrupt register (PWMINT) are
+ * always synchronized with PWM_CLK at regular intervals. It takes some
+ * time (Min: 2 × PCLK + 4 × PWM_CLK to Max: 6 × PCLK + 9 × PWM_CLK) for
+ * the value set in the register to be reflected in the PWM circuit
+ * because there is a synchronizer between the register and the PWM
+ * circuit.
+ */
+ rzv2m_pwm->delay = 6 * DIV_ROUND_UP(NSEC_PER_SEC, apb_clk_rate) +
+ 9 * DIV_ROUND_UP(NSEC_PER_SEC, rzv2m_pwm->rate);
+
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+
+ /*
+ * We need to keep the clock on, in case the bootloader has enabled the
+ * PWM and is running during probe().
+ */
+ if (!!(rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR) & RZV2M_PWMCTR_PWME)) {
+ u32 val;
+
+ pm_runtime_get_sync(&pdev->dev);
+ rzv2m_pwm->is_ch_enabled = true;
+ rzv2m_pwm->pwm_cyc = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
+ val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
+ rzv2m_pwm->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, val) ?
+ PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
+ }
+
+ rzv2m_pwm->chip.dev = &pdev->dev;
+ ret = devm_add_action_or_reset(&pdev->dev,
+ rzv2m_pwm_reset_assert_pm_disable,
+ rzv2m_pwm);
+ if (ret)
+ return ret;
+
+ rzv2m_pwm->chip.ops = &rzv2m_pwm_ops;
+ rzv2m_pwm->chip.npwm = 1;
+ ret = devm_pwmchip_add(&pdev->dev, &rzv2m_pwm->chip);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+
+ pm_runtime_idle(&pdev->dev);
+
+ return 0;
+
+err_rate_put:
+ clk_rate_exclusive_put(rzv2m_pwm->pwm_clk);
+ clk_rate_exclusive_put(rzv2m_pwm->apb_clk);
+ clk_disable_unprepare(rzv2m_pwm->pwm_clk);
+disable_apb_clk:
+ clk_disable_unprepare(rzv2m_pwm->apb_clk);
+err_reset:
+ reset_control_assert(rzv2m_pwm->rstc);
+ return ret;
+}
+
+static const struct of_device_id rzv2m_pwm_of_table[] = {
+ { .compatible = "renesas,rzv2m-pwm", },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzv2m_pwm_of_table);
+
+static struct platform_driver rzv2m_pwm_driver = {
+ .driver = {
+ .name = "pwm-rzv2m",
+ .pm = pm_ptr(&rzv2m_pwm_pm_ops),
+ .of_match_table = rzv2m_pwm_of_table,
+ },
+ .probe = rzv2m_pwm_probe,
+};
+module_platform_driver(rzv2m_pwm_driver);
+
+MODULE_AUTHOR("Biju Das <[email protected]>");
+MODULE_AUTHOR("Fabrizio Castro <[email protected]>");
+MODULE_DESCRIPTION("Renesas RZ/V2M PWM Timer Driver");
+MODULE_LICENSE("GPL");
--
2.34.1
From: Biju Das <[email protected]>
Add device nodes for the pwm timer channels that are not assigned
to the ISP.
Signed-off-by: Biju Das <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
v6->v7:
* No change.
v5->v6:
* No change.
v4->v5:
* No change.
v3->v4:
* No change
v2->v3:
* Added Rb tag from Geert
v1->v2:
* Added resets property
arch/arm64/boot/dts/renesas/r9a09g011.dtsi | 98 ++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r9a09g011.dtsi b/arch/arm64/boot/dts/renesas/r9a09g011.dtsi
index 50ed66d42a24..bb006772e31e 100644
--- a/arch/arm64/boot/dts/renesas/r9a09g011.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a09g011.dtsi
@@ -236,6 +236,104 @@ sys: system-controller@a3f03000 {
reg = <0 0xa3f03000 0 0x400>;
};
+ pwm8: pwm@a4010400 {
+ compatible = "renesas,r9a09g011-pwm",
+ "renesas,rzv2m-pwm";
+ reg = <0 0xa4010400 0 0x80>;
+ interrupts = <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD R9A09G011_CPERI_GRPF_PCLK>,
+ <&cpg CPG_MOD R9A09G011_PWM8_CLK>;
+ clock-names = "apb", "pwm";
+ resets = <&cpg R9A09G011_PWM_GPF_PRESETN>;
+ power-domains = <&cpg>;
+ #pwm-cells = <2>;
+ status = "disabled";
+ };
+
+ pwm9: pwm@a4010480 {
+ compatible = "renesas,r9a09g011-pwm",
+ "renesas,rzv2m-pwm";
+ reg = <0 0xa4010480 0 0x80>;
+ interrupts = <GIC_SPI 377 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD R9A09G011_CPERI_GRPF_PCLK>,
+ <&cpg CPG_MOD R9A09G011_PWM9_CLK>;
+ clock-names = "apb", "pwm";
+ resets = <&cpg R9A09G011_PWM_GPF_PRESETN>;
+ power-domains = <&cpg>;
+ #pwm-cells = <2>;
+ status = "disabled";
+ };
+
+ pwm10: pwm@a4010500 {
+ compatible = "renesas,r9a09g011-pwm",
+ "renesas,rzv2m-pwm";
+ reg = <0 0xa4010500 0 0x80>;
+ interrupts = <GIC_SPI 378 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD R9A09G011_CPERI_GRPF_PCLK>,
+ <&cpg CPG_MOD R9A09G011_PWM10_CLK>;
+ clock-names = "apb", "pwm";
+ resets = <&cpg R9A09G011_PWM_GPF_PRESETN>;
+ power-domains = <&cpg>;
+ #pwm-cells = <2>;
+ status = "disabled";
+ };
+
+ pwm11: pwm@a4010580 {
+ compatible = "renesas,r9a09g011-pwm",
+ "renesas,rzv2m-pwm";
+ reg = <0 0xa4010580 0 0x80>;
+ interrupts = <GIC_SPI 379 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD R9A09G011_CPERI_GRPF_PCLK>,
+ <&cpg CPG_MOD R9A09G011_PWM11_CLK>;
+ clock-names = "apb", "pwm";
+ resets = <&cpg R9A09G011_PWM_GPF_PRESETN>;
+ power-domains = <&cpg>;
+ #pwm-cells = <2>;
+ status = "disabled";
+ };
+
+ pwm12: pwm@a4010600 {
+ compatible = "renesas,r9a09g011-pwm",
+ "renesas,rzv2m-pwm";
+ reg = <0 0xa4010600 0 0x80>;
+ interrupts = <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD R9A09G011_CPERI_GRPF_PCLK>,
+ <&cpg CPG_MOD R9A09G011_PWM12_CLK>;
+ clock-names = "apb", "pwm";
+ resets = <&cpg R9A09G011_PWM_GPF_PRESETN>;
+ power-domains = <&cpg>;
+ #pwm-cells = <2>;
+ status = "disabled";
+ };
+
+ pwm13: pwm@a4010680 {
+ compatible = "renesas,r9a09g011-pwm",
+ "renesas,rzv2m-pwm";
+ reg = <0 0xa4010680 0 0x80>;
+ interrupts = <GIC_SPI 381 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD R9A09G011_CPERI_GRPF_PCLK>,
+ <&cpg CPG_MOD R9A09G011_PWM13_CLK>;
+ clock-names = "apb", "pwm";
+ resets = <&cpg R9A09G011_PWM_GPF_PRESETN>;
+ power-domains = <&cpg>;
+ #pwm-cells = <2>;
+ status = "disabled";
+ };
+
+ pwm14: pwm@a4010700 {
+ compatible = "renesas,r9a09g011-pwm",
+ "renesas,rzv2m-pwm";
+ reg = <0 0xa4010700 0 0x80>;
+ interrupts = <GIC_SPI 382 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD R9A09G011_CPERI_GRPF_PCLK>,
+ <&cpg CPG_MOD R9A09G011_PWM14_CLK>;
+ clock-names = "apb", "pwm";
+ resets = <&cpg R9A09G011_PWM_GPF_PRESETN>;
+ power-domains = <&cpg>;
+ #pwm-cells = <2>;
+ status = "disabled";
+ };
+
csi0: spi@a4020000 {
compatible = "renesas,rzv2m-csi";
reg = <0 0xa4020000 0 0x80>;
--
2.34.1
From: Biju Das <[email protected]>
Enable pwm{8..14} on RZ/V2M EVK.
Signed-off-by: Biju Das <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
v6->v7:
* No change.
v5->v6:
* No change.
v4->v5:
* No change
v3->v4:
* No change
v2->v3:
* Added Rb tag from Geert.
v1->v2:
* No change
.../boot/dts/renesas/r9a09g011-v2mevk2.dts | 70 +++++++++++++++++++
1 file changed, 70 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts b/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts
index 39fe3f94991e..6e636ac2d190 100644
--- a/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts
+++ b/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts
@@ -196,6 +196,34 @@ i2c2_pins: i2c2 {
<RZV2M_PORT_PINMUX(3, 9, 2)>; /* SCL */
};
+ pwm8_pins: pwm8 {
+ pinmux = <RZV2M_PORT_PINMUX(1, 8, 1)>; /* PM8 */
+ };
+
+ pwm9_pins: pwm9 {
+ pinmux = <RZV2M_PORT_PINMUX(1, 9, 1)>; /* PM9 */
+ };
+
+ pwm10_pins: pwm10 {
+ pinmux = <RZV2M_PORT_PINMUX(1, 10, 1)>; /* PM10 */
+ };
+
+ pwm11_pins: pwm11 {
+ pinmux = <RZV2M_PORT_PINMUX(1, 11, 1)>; /* PM11 */
+ };
+
+ pwm12_pins: pwm12 {
+ pinmux = <RZV2M_PORT_PINMUX(1, 12, 1)>; /* PM12 */
+ };
+
+ pwm13_pins: pwm13 {
+ pinmux = <RZV2M_PORT_PINMUX(1, 13, 1)>; /* PM13 */
+ };
+
+ pwm14_pins: pwm14 {
+ pinmux = <RZV2M_PORT_PINMUX(1, 14, 1)>; /* PM14 */
+ };
+
sdhi0_pins: sd0 {
data {
pinmux = <RZV2M_PORT_PINMUX(8, 2, 1)>, /* SD0DAT0 */
@@ -251,6 +279,48 @@ &pwc {
status = "okay";
};
+&pwm8 {
+ pinctrl-0 = <&pwm8_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
+
+&pwm9 {
+ pinctrl-0 = <&pwm9_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
+
+&pwm10 {
+ pinctrl-0 = <&pwm10_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
+
+&pwm11 {
+ pinctrl-0 = <&pwm11_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
+
+&pwm12 {
+ pinctrl-0 = <&pwm12_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
+
+&pwm13 {
+ pinctrl-0 = <&pwm13_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
+
+&pwm14 {
+ pinctrl-0 = <&pwm14_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
+
&sdhi0 {
pinctrl-0 = <&sdhi0_pins>;
pinctrl-1 = <&sdhi0_pins_uhs>;
--
2.34.1
On Mon, Feb 12, 2024 at 09:06:49PM +0000, Fabrizio Castro wrote:
> From: Biju Das <[email protected]>
>
> Add device tree bindings for the RZ/V2{M, MA} PWM Timer (PWM).
>
> Signed-off-by: Biju Das <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
If you send a patch, it needs your S-o-b. (Though you could probably
trick me into applying v6 :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Feb 12, 2024 at 09:06:50PM +0000, Fabrizio Castro wrote:
> From: Biju Das <[email protected]>
>
> The RZ/V2{M, MA} PWM Timer supports the following functions:
>
> * The PWM has 24-bit counters which operate at PWM_CLK (48 MHz).
> * The frequency division ratio for internal counter operation is
> selectable as PWM_CLK divided by 1, 16, 256, or 2048.
> * The period as well as the duty cycle is adjustable.
> * The low-level and high-level order of the PWM signals can be
> inverted.
> * The duty cycle of the PWM signal is selectable in the range from
> 0 to 100%.
> * The minimum resolution is 20.83 ns.
> * Three interrupt sources: Rising and falling edges of the PWM signal
> and clearing of the counter
> * Counter operation and the bus interface are asynchronous and both
> can operate independently of the magnitude relationship of the
> respective clock periods.
>
> Signed-off-by: Biju Das <[email protected]>
> Signed-off-by: Fabrizio Castro <[email protected]>
> ---
>
> v6->v7:
> * Addressed the build issue reported by the kernel test robot.
> * Added include math64.h.
> * Reworked rzv2m_pwm_mul_u64_u64_div_u64_roundup to make use of
> div64_u64 and to get rid of % while keeping the same formula.
> * Added rzv2m_pwm_mul_u64_u64_div_u64_rounddown.
> * Replaced / with div64_u64 wherever necessary.
> v5->v6:
> * Added Fab's Signed-off-by.
> * Updated copyright year to 2024.
> * Added include of limits.h.
> * Added variable max_period to rzv2m_pwm_chip.
> * Simplified the calculations by calculating max_period during probe,
> based on the numerical limits of the formula and the u64 data type.
> * Added rzv2m_pwm_mul_u64_u64_div_u64_roundup.
> * Added rzv2m_pwm_prescale_to_shift to fix the calculation of the
> frequency divider.
> * Improved the calculations and the variable names of
> rzv2m_pwm_get_state.
> * Improved the calculations of rzv2m_pwm_config.
> * Removed .owner from rzv2m_pwm_ops.
> * Improved rzv2m_pwm_pm_runtime_resume and renamed its err variable to
> ret.
> * Removed of_match_ptr.
> * Added Fab as module author.
> v4->v5:
> * Sorted KConfig file
> * Sorted Make file
> * Updated copyright header 2022->2023.
> * Updated limitation section.
> * Replaced the variable chip->rzv2m_pwm in rzv2m_pwm_wait_delay()
> * Replaced polarity logic as per HW manual dutycycle = Ton/Ton+Toff, so
> eventhough native polarity is inverted from period point of view it
> is correct.
> * Added logic for supporting 0% , 100% and remaining duty cycle.
> * On config() replaced
> pm_runtime_resume_and_get()->pm_runtime_get_sync()
> * Counter is stopped while updating period/polarity to avoid glitches.
> * Added error check for clk_prepare_enable()
> * Introduced is_ch_enabled variable to cache channel enable status.
> * clk_get_rate is called after enabling the clock and
> clk_rate_exclusive_get()
> * Added comment for delay
> * Replaced 1000000000UL->NSEC_PER_SEC.
> * Improved error handling in probe().
> v3->v4:
> * Documented the hardware properties in "Limitations" section
> * Dropped the macros F2CYCLE_NSEC, U24_MASK and U24_MAX.
> * Added RZV2M_PWMCYC_PERIOD macro for U24_MAX
> * Dropped rzv2m_pwm_freq_div variable and started using 1 << (4 * i)
> for calculating divider as it is power of 16.
> * Reordered the functions to have rzv2m_pwm_config() directly before
> rzv2m_pwm_apply().
> * Improved the logic for calculating period and duty cycle in config()
> * Merged multiple RZV2M_PWMCTR register writes to a single write in
> * config()
> * replaced pwm_is_enabled()->pwm->state.enabled
> * Avoided assigning bit value as enum pwm_polarity instead used enum
> * constant.
> * Fixed various issues in probe error path.
> * Updated the logic for PWM cycle setting register
> * A 100% duty cycle is only possible with PWMLOW > PWMCYC. So
> restricting PWMCYC values < 0xffffff
> * The native polarity of the hardware is inverted (i.e. it starts with
> the low part). So switched the inversion bit handling.
> v2->v3:
> * Added return code for rzv2m_pwm_get_state()
> * Added comment in rzv2m_pwm_reset_assert_pm_disable()
> v1->v2:
> * Replaced
> devm_reset_control_get_optional_shared->devm_reset_control_get_shared
>
> drivers/pwm/Kconfig | 11 +
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-rzv2m.c | 480 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 492 insertions(+)
> create mode 100644 drivers/pwm/pwm-rzv2m.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..55d46e6183a2 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -524,6 +524,17 @@ config PWM_RZ_MTU3
> To compile this driver as a module, choose M here: the module
> will be called pwm-rz-mtu3.
>
> +config PWM_RZV2M
> + tristate "Renesas RZ/V2M PWM support"
> + depends on ARCH_R9A09G011 || COMPILE_TEST
> + depends on HAS_IOMEM
> + help
> + This driver exposes the PWM controller found in Renesas
> + RZ/V2M like chips through the PWM API.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-rzv2m.
> +
> 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 c5ec9e168ee7..cf5a4a1c3b1a 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -48,6 +48,7 @@ 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_RZV2M) += pwm-rzv2m.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-rzv2m.c b/drivers/pwm/pwm-rzv2m.c
> new file mode 100644
> index 000000000000..eb9062293590
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzv2m.c
> @@ -0,0 +1,480 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2M PWM Timer (PWM) driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corporation
> + *
> + * Hardware manual for this IP can be found here
> + * https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardware?language=en
> + *
> + * Limitations:
> + * - Changes to the duty cycle configuration get effective only after the next
> + * period end.
> +?* - The duty cycle can be changed only by modifying the PWMLOW register
> +?* ? value and changing the pulse width at low level. The duty cycle becomes
> +?* ? 0% for the low width when the value of the PWMLOW register is 0x0h
> +?* ? and 100% for the low width when the value of the PWMLOW > PWMCYC.
If polarity or period is changed, the hardware has to be stopped, to
this yields glitches.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/limits.h>
> +#include <linux/math64.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +#include <linux/time.h>
> +
> +#define RZV2M_PWMCTR 0x0
> +#define RZV2M_PWMCYC 0x4
> +#define RZV2M_PWMLOW 0x8
> +#define RZV2M_PWMCNT 0xc
> +
> +#define RZV2M_PWMCTR_PWMPS GENMASK(17, 16)
> +#define RZV2M_PWMCTR_PWMHL BIT(3)
> +#define RZV2M_PWMCTR_PWMTM BIT(2)
> +#define RZV2M_PWMCTR_PWME BIT(1)
> +
> +#define RZV2M_PWMCYC_PERIOD GENMASK(23, 0)
> +#define RZV2M_PWMLOW_PERIOD GENMASK(23, 0)
> +
> +struct rzv2m_pwm_chip {
> + u64 max_period;
> + struct pwm_chip chip;
> + void __iomem *mmio;
> + struct reset_control *rstc;
> + struct clk *apb_clk;
> + struct clk *pwm_clk;
> + unsigned long rate;
> + unsigned long delay;
> + unsigned long pwm_cyc;
> + enum pwm_polarity polarity;
> + bool is_ch_enabled;
> +};
> +
> +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c)
> +{
> + u64 ab = a * b;
a * b might overflow?!
> + u64 d = div64_u64(ab, c);
> + u64 e = d * c;
> +
> + return d + ((ab - e) ? 1 : 0);
> +}
> +
> +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_rounddown(u64 a, u64 b, u64 c)
> +{
> + return div64_u64(a * b, c);
ditto. This is the same function as mul_u64_u64_div_u64() isn't it?
> +}
> +
> +static inline struct rzv2m_pwm_chip *to_rzv2m_pwm_chip(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct rzv2m_pwm_chip, chip);
> +}
> +
> +static void rzv2m_pwm_wait_delay(struct rzv2m_pwm_chip *rzv2m_pwm)
> +{
> + /* delay timer when change the setting register */
> + ndelay(rzv2m_pwm->delay);
> +}
> +
> +static void rzv2m_pwm_write(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg, u32 data)
> +{
> + writel(data, rzv2m_pwm->mmio + reg);
> +}
> +
> +static u32 rzv2m_pwm_read(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg)
> +{
> + return readl(rzv2m_pwm->mmio + reg);
> +}
> +
> +static void rzv2m_pwm_modify(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg, u32 clr,
> + u32 set)
> +{
> + rzv2m_pwm_write(rzv2m_pwm, reg,
> + (rzv2m_pwm_read(rzv2m_pwm, reg) & ~clr) | set);
> +}
> +
> +static u8 rzv2m_pwm_calculate_prescale(struct rzv2m_pwm_chip *rzv2m_pwm,
> + u64 period_cycles)
> +{
> + u32 prescaled_period_cycles;
> + u8 prescale;
> +
> + prescaled_period_cycles = period_cycles >> 24;
> + if (prescaled_period_cycles >= 256)
> + prescale = 3;
> + else
> + prescale = (fls(prescaled_period_cycles) + 3) / 4;
> +
> + return prescale;
> +}
> +
> +static inline int rzv2m_pwm_prescale_to_shift(u8 prescale)
> +{
> + return prescale == 3 ? 11 : prescale * 4;
> +}
> +
> +static int rzv2m_pwm_enable(struct rzv2m_pwm_chip *rzv2m_pwm)
> +{
> + int rc;
> +
> + rc = pm_runtime_resume_and_get(rzv2m_pwm->chip.dev);
> + if (rc)
> + return rc;
> +
> + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
> + RZV2M_PWMCTR_PWME);
> + rzv2m_pwm_wait_delay(rzv2m_pwm);
> + rzv2m_pwm->is_ch_enabled = true;
> +
> + return 0;
> +}
> +
> +static void rzv2m_pwm_disable(struct rzv2m_pwm_chip *rzv2m_pwm)
> +{
> + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME, 0);
> + rzv2m_pwm_wait_delay(rzv2m_pwm);
> + pm_runtime_put_sync(rzv2m_pwm->chip.dev);
> + rzv2m_pwm->is_ch_enabled = false;
> +}
> +
> +static int rzv2m_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> + u16 frequency_divisor;
> + u32 ctr, cyc, low;
> + u8 prescale;
> +
> + pm_runtime_get_sync(chip->dev);
> + ctr = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
> + state->enabled = FIELD_GET(RZV2M_PWMCTR_PWME, ctr);
> + state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, ctr) ?
> + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> + prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, ctr);
> + frequency_divisor = 1 << rzv2m_pwm_prescale_to_shift(prescale);
Giving a shift is cheaper than a multiplication, I suggest to do
frequency_divisor = rzv2m_pwm_prescale_to_shift(prescale);
and instead of multiply by frequency_divisor, use a left-shift
operation.
> + cyc = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> + state->period = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1,
> + NSEC_PER_SEC * frequency_divisor,
> + rzv2m_pwm->rate);
> +
> + low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> + state->duty_cycle = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1 - low,
> + NSEC_PER_SEC * frequency_divisor,
> + rzv2m_pwm->rate);
The register semantic makes me wonder if each period starts with the low
part. In that case the hardware called "normal" what is called inverted
in the pwm framework?!
> + return pm_runtime_put(chip->dev);
If you evaluate the return value of pm_runtime_put() maybe check
pm_runtime_get_sync() for symmetry, too?
> +}
> +
> +static int rzv2m_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> + u64 period = state->period, duty_cycle = state->duty_cycle;
> + u16 frequency_divisor;
> + u64 pwm_cyc, pwm_low;
> + u8 prescale;
> + u32 pwm_ctr;
> +
> + /*
> + * Clamp period and duty cycle to their maximum values for our current
> + * configuration rather than letting our calculations overflow.
> + */
> + if (period > rzv2m_pwm->max_period) {
> + period = rzv2m_pwm->max_period;
> + if (duty_cycle > rzv2m_pwm->max_period)
> + duty_cycle = period;
> + }
> +
> + /*
> + * Formula for calculating PWM Cycle Setting Register:
> + * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) ? Div ratio)) - 1
> + */
> + pwm_cyc = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(period,
> + rzv2m_pwm->rate,
> + NSEC_PER_SEC);
> + pwm_cyc = pwm_cyc ? pwm_cyc : 1;
if pwm_cyc is 0 here, the period is too short to be realized, right?
That's an error condition.
> + prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pwm_cyc - 1);
> + frequency_divisor = 1 << rzv2m_pwm_prescale_to_shift(prescale);
> + if (frequency_divisor > 1) {
> + pwm_cyc = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(period,
> + rzv2m_pwm->rate,
> + NSEC_PER_SEC * frequency_divisor);
> + pwm_cyc = pwm_cyc ? pwm_cyc : 1;
> + }
> +
> + if (pwm_cyc && !FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc - 1))
> + pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;
I don't understand the relevance of FIELD_FIT(RZV2M_PWMCYC_PERIOD,
pwm_cyc - 1).
> +
> + /*
> + * Formula for calculating PWMLOW register:
> + * PWMLOW register = PWM cycle * Low pulse width ratio (%)
> + */
> + pwm_low = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(duty_cycle,
> + rzv2m_pwm->rate, NSEC_PER_SEC * frequency_divisor);
> +
> + pwm_low = pwm_cyc - pwm_low;
Either the old or the new value of pwm_low doesn't match the variable's
name. Please add another variable for the wrong one. The compiler should
optimize that out and the reader can more easily understand the code.
> + if (!FIELD_FIT(RZV2M_PWMLOW_PERIOD, pwm_low))
> + pwm_low = RZV2M_PWMLOW_PERIOD;
> +
> + pwm_cyc--;
> +
> + /*
> + * 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(rzv2m_pwm->chip.dev);
> +
> + /*
> + * To change the setting value of the PWM cycle setting register
> + * (PWMm_PWMCYC) or polarity, set the PWME bit of the PWM control
> + * register (PWMm_PWMCTR) to 0b and stop the counter operation.
> + */
> + if (rzv2m_pwm->polarity != state->polarity || rzv2m_pwm->pwm_cyc != pwm_cyc) {
> + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME, 0);
> + rzv2m_pwm_wait_delay(rzv2m_pwm);
> + }
> +
> + rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMCYC, pwm_cyc);
> + rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMLOW, pwm_low);
> +
> + pwm_ctr = FIELD_PREP(RZV2M_PWMCTR_PWMPS, prescale);
> + if (state->polarity == PWM_POLARITY_INVERSED)
> + pwm_ctr |= RZV2M_PWMCTR_PWMHL;
> +
> + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMTM |
> + RZV2M_PWMCTR_PWMPS | RZV2M_PWMCTR_PWMHL, pwm_ctr);
> +
> + if (rzv2m_pwm->polarity != state->polarity || rzv2m_pwm->pwm_cyc != pwm_cyc) {
> + rzv2m_pwm->polarity = state->polarity;
> + rzv2m_pwm->pwm_cyc = pwm_cyc;
> + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
> + RZV2M_PWMCTR_PWME);
> + }
> +
> + rzv2m_pwm_wait_delay(rzv2m_pwm);
> +
> + /* If the PWM is not enabled, turn the clock off again to save power. */
> + if (!pwm->state.enabled)
> + pm_runtime_put(rzv2m_pwm->chip.dev);
> +
> + return 0;
> +}
> [...]
> +static int rzv2m_pwm_probe(struct platform_device *pdev)
> +{
> + struct rzv2m_pwm_chip *rzv2m_pwm;
> + unsigned long apb_clk_rate;
> + int ret;
> +
> + rzv2m_pwm = devm_kzalloc(&pdev->dev, sizeof(*rzv2m_pwm), GFP_KERNEL);
> + if (!rzv2m_pwm)
> + return -ENOMEM;
> +
> + rzv2m_pwm->mmio = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(rzv2m_pwm->mmio))
> + return PTR_ERR(rzv2m_pwm->mmio);
> +
> + rzv2m_pwm->apb_clk = devm_clk_get(&pdev->dev, "apb");
> + if (IS_ERR(rzv2m_pwm->apb_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->apb_clk),
> + "cannot get apb clock\n");
> +
> + rzv2m_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
> + if (IS_ERR(rzv2m_pwm->pwm_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->pwm_clk),
> + "cannot get pwm clock\n");
> +
> + rzv2m_pwm->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> + if (IS_ERR(rzv2m_pwm->rstc))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->rstc),
> + "get reset failed\n");
> +
> + platform_set_drvdata(pdev, rzv2m_pwm);
> + ret = reset_control_deassert(rzv2m_pwm->rstc);
> + if (ret) {
> + return dev_err_probe(&pdev->dev, ret,
> + "cannot deassert reset control\n");
> + }
> +
> + ret = clk_prepare_enable(rzv2m_pwm->apb_clk);
> + if (ret < 0)
> + goto err_reset;
> +
> + ret = clk_prepare_enable(rzv2m_pwm->pwm_clk);
> + if (ret < 0)
> + goto disable_apb_clk;
> +
> + clk_rate_exclusive_get(rzv2m_pwm->apb_clk);
There is a devm_clk_rate_exclusive_get() in next starting from tomorrow
I hope. Using it should simplify the driver.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe,
thanks for your feedback.
I'll add my S-o-b in v8.
Cheers,
Fab
> -----Original Message-----
> From: Uwe Kleine-K?nig <[email protected]>
> Sent: Thursday, February 29, 2024 4:22 PM
> To: Fabrizio Castro <[email protected]>
> Cc: Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Geert Uytterhoeven <[email protected]>; Biju Das
> <[email protected]>; Magnus Damm <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Krzysztof
> Kozlowski <[email protected]>
> Subject: Re: [PATCH v7 1/4] dt-bindings: pwm: Add RZ/V2M PWM binding
>
> On Mon, Feb 12, 2024 at 09:06:49PM +0000, Fabrizio Castro wrote:
> > From: Biju Das <[email protected]>
> >
> > Add device tree bindings for the RZ/V2{M, MA} PWM Timer (PWM).
> >
> > Signed-off-by: Biju Das <[email protected]>
> > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > Reviewed-by: Geert Uytterhoeven <[email protected]>
>
> If you send a patch, it needs your S-o-b. (Though you could probably
> trick me into applying v6 :-)
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe,
Thanks for your feedback!
> From: Uwe Kleine-K?nig <[email protected]>
> Sent: Thursday, February 29, 2024 4:42 PM
> To: Fabrizio Castro <[email protected]>
> Subject: Re: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver
>
> On Mon, Feb 12, 2024 at 09:06:50PM +0000, Fabrizio Castro wrote:
> > From: Biju Das <[email protected]>
> >
> > The RZ/V2{M, MA} PWM Timer supports the following functions:
> >
> > * The PWM has 24-bit counters which operate at PWM_CLK (48 MHz).
> > * The frequency division ratio for internal counter operation is
> > selectable as PWM_CLK divided by 1, 16, 256, or 2048.
> > * The period as well as the duty cycle is adjustable.
> > * The low-level and high-level order of the PWM signals can be
> > inverted.
> > * The duty cycle of the PWM signal is selectable in the range from
> > 0 to 100%.
> > * The minimum resolution is 20.83 ns.
> > * Three interrupt sources: Rising and falling edges of the PWM signal
> > and clearing of the counter
> > * Counter operation and the bus interface are asynchronous and both
> > can operate independently of the magnitude relationship of the
> > respective clock periods.
> >
> > Signed-off-by: Biju Das <[email protected]>
> > Signed-off-by: Fabrizio Castro <[email protected]>
> > ---
> >
> > v6->v7:
> > * Addressed the build issue reported by the kernel test robot.
> > * Added include math64.h.
> > * Reworked rzv2m_pwm_mul_u64_u64_div_u64_roundup to make use of
> > div64_u64 and to get rid of % while keeping the same formula.
> > * Added rzv2m_pwm_mul_u64_u64_div_u64_rounddown.
> > * Replaced / with div64_u64 wherever necessary.
> > v5->v6:
> > * Added Fab's Signed-off-by.
> > * Updated copyright year to 2024.
> > * Added include of limits.h.
> > * Added variable max_period to rzv2m_pwm_chip.
> > * Simplified the calculations by calculating max_period during probe,
> > based on the numerical limits of the formula and the u64 data type.
> > * Added rzv2m_pwm_mul_u64_u64_div_u64_roundup.
> > * Added rzv2m_pwm_prescale_to_shift to fix the calculation of the
> > frequency divider.
> > * Improved the calculations and the variable names of
> > rzv2m_pwm_get_state.
> > * Improved the calculations of rzv2m_pwm_config.
> > * Removed .owner from rzv2m_pwm_ops.
> > * Improved rzv2m_pwm_pm_runtime_resume and renamed its err variable to
> > ret.
> > * Removed of_match_ptr.
> > * Added Fab as module author.
> > v4->v5:
> > * Sorted KConfig file
> > * Sorted Make file
> > * Updated copyright header 2022->2023.
> > * Updated limitation section.
> > * Replaced the variable chip->rzv2m_pwm in rzv2m_pwm_wait_delay()
> > * Replaced polarity logic as per HW manual dutycycle = Ton/Ton+Toff, so
> > eventhough native polarity is inverted from period point of view it
> > is correct.
> > * Added logic for supporting 0% , 100% and remaining duty cycle.
> > * On config() replaced
> > pm_runtime_resume_and_get()->pm_runtime_get_sync()
> > * Counter is stopped while updating period/polarity to avoid glitches.
> > * Added error check for clk_prepare_enable()
> > * Introduced is_ch_enabled variable to cache channel enable status.
> > * clk_get_rate is called after enabling the clock and
> > clk_rate_exclusive_get()
> > * Added comment for delay
> > * Replaced 1000000000UL->NSEC_PER_SEC.
> > * Improved error handling in probe().
> > v3->v4:
> > * Documented the hardware properties in "Limitations" section
> > * Dropped the macros F2CYCLE_NSEC, U24_MASK and U24_MAX.
> > * Added RZV2M_PWMCYC_PERIOD macro for U24_MAX
> > * Dropped rzv2m_pwm_freq_div variable and started using 1 << (4 * i)
> > for calculating divider as it is power of 16.
> > * Reordered the functions to have rzv2m_pwm_config() directly before
> > rzv2m_pwm_apply().
> > * Improved the logic for calculating period and duty cycle in config()
> > * Merged multiple RZV2M_PWMCTR register writes to a single write in
> > * config()
> > * replaced pwm_is_enabled()->pwm->state.enabled
> > * Avoided assigning bit value as enum pwm_polarity instead used enum
> > * constant.
> > * Fixed various issues in probe error path.
> > * Updated the logic for PWM cycle setting register
> > * A 100% duty cycle is only possible with PWMLOW > PWMCYC. So
> > restricting PWMCYC values < 0xffffff
> > * The native polarity of the hardware is inverted (i.e. it starts with
> > the low part). So switched the inversion bit handling.
> > v2->v3:
> > * Added return code for rzv2m_pwm_get_state()
> > * Added comment in rzv2m_pwm_reset_assert_pm_disable()
> > v1->v2:
> > * Replaced
> > devm_reset_control_get_optional_shared->devm_reset_control_get_shared
> >
> > drivers/pwm/Kconfig | 11 +
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-rzv2m.c | 480 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 492 insertions(+)
> > create mode 100644 drivers/pwm/pwm-rzv2m.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 4b956d661755..55d46e6183a2 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -524,6 +524,17 @@ config PWM_RZ_MTU3
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-rz-mtu3.
> >
> > +config PWM_RZV2M
> > + tristate "Renesas RZ/V2M PWM support"
> > + depends on ARCH_R9A09G011 || COMPILE_TEST
> > + depends on HAS_IOMEM
> > + help
> > + This driver exposes the PWM controller found in Renesas
> > + RZ/V2M like chips through the PWM API.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-rzv2m.
> > +
> > 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 c5ec9e168ee7..cf5a4a1c3b1a 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -48,6 +48,7 @@ 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_RZV2M) += pwm-rzv2m.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-rzv2m.c b/drivers/pwm/pwm-rzv2m.c
> > new file mode 100644
> > index 000000000000..eb9062293590
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzv2m.c
> > @@ -0,0 +1,480 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/V2M PWM Timer (PWM) driver
> > + *
> > + * Copyright (C) 2024 Renesas Electronics Corporation
> > + *
> > + * Hardware manual for this IP can be found here
> > + * https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-
> hardware?language=en
> > + *
> > + * Limitations:
> > + * - Changes to the duty cycle configuration get effective only after
> the next
> > + * period end.
> > +?* - The duty cycle can be changed only by modifying the PWMLOW register
> > +?* ? value and changing the pulse width at low level. The duty cycle
> becomes
> > +?* ? 0% for the low width when the value of the PWMLOW register is 0x0h
> > +?* ? and 100% for the low width when the value of the PWMLOW > PWMCYC.
>
> If polarity or period is changed, the hardware has to be stopped, to
> this yields glitches.
Thanks for the suggestion, I'll add it.
>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/limits.h>
> > +#include <linux/math64.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pwm.h>
> > +#include <linux/reset.h>
> > +#include <linux/time.h>
> > +
> > +#define RZV2M_PWMCTR 0x0
> > +#define RZV2M_PWMCYC 0x4
> > +#define RZV2M_PWMLOW 0x8
> > +#define RZV2M_PWMCNT 0xc
> > +
> > +#define RZV2M_PWMCTR_PWMPS GENMASK(17, 16)
> > +#define RZV2M_PWMCTR_PWMHL BIT(3)
> > +#define RZV2M_PWMCTR_PWMTM BIT(2)
> > +#define RZV2M_PWMCTR_PWME BIT(1)
> > +
> > +#define RZV2M_PWMCYC_PERIOD GENMASK(23, 0)
> > +#define RZV2M_PWMLOW_PERIOD GENMASK(23, 0)
> > +
> > +struct rzv2m_pwm_chip {
> > + u64 max_period;
> > + struct pwm_chip chip;
> > + void __iomem *mmio;
> > + struct reset_control *rstc;
> > + struct clk *apb_clk;
> > + struct clk *pwm_clk;
> > + unsigned long rate;
> > + unsigned long delay;
> > + unsigned long pwm_cyc;
> > + enum pwm_polarity polarity;
> > + bool is_ch_enabled;
> > +};
> > +
> > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b,
> u64 c)
> > +{
> > + u64 ab = a * b;
>
> a * b might overflow?!
In the context of this driver, this cannot overflow.
The 2 formulas the above is needed for are:
1) period = (cyc + 1)*(NSEC_PER_SEC * frequency_divisor)/rate
2) duty_cycle = (cyc + 1 - low)*(NSEC_PER_SEC * frequency_divisor)/rate
With respect to 1), the dividend overflows when period * rate also
overflows (its product is calculated in rzv2m_pwm_config).
However, limiting the period to a maximum value of U64_MAX / rate
prevents the calculations from overflowing (in both directions, from period to cyc, and from cyc to period). v6 introduced max_period for this.
The situation for 2) is very similar to 1), with duty_cycle<=period,
therefore limiting period to a max value (and clamping the duty cycle
accordingly) will ensure that the calculation for duty_cycle won't
overflow, either.
>
> > + u64 d = div64_u64(ab, c);
> > + u64 e = d * c;
> > +
> > + return d + ((ab - e) ? 1 : 0);
> > +}
> > +
> > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_rounddown(u64 a, u64 b,
> u64 c)
> > +{
> > + return div64_u64(a * b, c);
>
> ditto. This is the same function as mul_u64_u64_div_u64() isn't it?
Since a * b cannot overflow in the case of this driver, I believe the
above to be a better option than mul_u64_u64_div_u64.
>
> > +}
> > +
> > +static inline struct rzv2m_pwm_chip *to_rzv2m_pwm_chip(struct pwm_chip
> *chip)
> > +{
> > + return container_of(chip, struct rzv2m_pwm_chip, chip);
> > +}
> > +
> > +static void rzv2m_pwm_wait_delay(struct rzv2m_pwm_chip *rzv2m_pwm)
> > +{
> > + /* delay timer when change the setting register */
> > + ndelay(rzv2m_pwm->delay);
> > +}
> > +
> > +static void rzv2m_pwm_write(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg,
> u32 data)
> > +{
> > + writel(data, rzv2m_pwm->mmio + reg);
> > +}
> > +
> > +static u32 rzv2m_pwm_read(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg)
> > +{
> > + return readl(rzv2m_pwm->mmio + reg);
> > +}
> > +
> > +static void rzv2m_pwm_modify(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg,
> u32 clr,
> > + u32 set)
> > +{
> > + rzv2m_pwm_write(rzv2m_pwm, reg,
> > + (rzv2m_pwm_read(rzv2m_pwm, reg) & ~clr) | set);
> > +}
> > +
> > +static u8 rzv2m_pwm_calculate_prescale(struct rzv2m_pwm_chip *rzv2m_pwm,
> > + u64 period_cycles)
> > +{
> > + u32 prescaled_period_cycles;
> > + u8 prescale;
> > +
> > + prescaled_period_cycles = period_cycles >> 24;
> > + if (prescaled_period_cycles >= 256)
> > + prescale = 3;
> > + else
> > + prescale = (fls(prescaled_period_cycles) + 3) / 4;
> > +
> > + return prescale;
> > +}
> > +
> > +static inline int rzv2m_pwm_prescale_to_shift(u8 prescale)
> > +{
> > + return prescale == 3 ? 11 : prescale * 4;
> > +}
> > +
> > +static int rzv2m_pwm_enable(struct rzv2m_pwm_chip *rzv2m_pwm)
> > +{
> > + int rc;
> > +
> > + rc = pm_runtime_resume_and_get(rzv2m_pwm->chip.dev);
> > + if (rc)
> > + return rc;
> > +
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
> > + RZV2M_PWMCTR_PWME);
> > + rzv2m_pwm_wait_delay(rzv2m_pwm);
> > + rzv2m_pwm->is_ch_enabled = true;
> > +
> > + return 0;
> > +}
> > +
> > +static void rzv2m_pwm_disable(struct rzv2m_pwm_chip *rzv2m_pwm)
> > +{
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME, 0);
> > + rzv2m_pwm_wait_delay(rzv2m_pwm);
> > + pm_runtime_put_sync(rzv2m_pwm->chip.dev);
> > + rzv2m_pwm->is_ch_enabled = false;
> > +}
> > +
> > +static int rzv2m_pwm_get_state(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> > + u16 frequency_divisor;
> > + u32 ctr, cyc, low;
> > + u8 prescale;
> > +
> > + pm_runtime_get_sync(chip->dev);
> > + ctr = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
> > + state->enabled = FIELD_GET(RZV2M_PWMCTR_PWME, ctr);
> > + state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, ctr) ?
> > + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > + prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, ctr);
> > + frequency_divisor = 1 << rzv2m_pwm_prescale_to_shift(prescale);
>
> Giving a shift is cheaper than a multiplication, I suggest to do
>
> frequency_divisor = rzv2m_pwm_prescale_to_shift(prescale);
>
> and instead of multiply by frequency_divisor, use a left-shift
> operation.
Will do, thanks for the suggestion.
>
> > + cyc = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> > + state->period = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1,
> > + NSEC_PER_SEC * frequency_divisor,
> > + rzv2m_pwm->rate);
> > +
> > + low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> > + state->duty_cycle = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1 -
> low,
> > + NSEC_PER_SEC * frequency_divisor,
> > + rzv2m_pwm->rate);
>
> The register semantic makes me wonder if each period starts with the low
> part. In that case the hardware called "normal" what is called inverted
> in the pwm framework?!
My understanding is that the PWM framework defines "normal" polarity a
signal that starts high (and stays high) for the duration of the duty cycle,
and goes low for the remainder of the period. Conversely, a signal with
"inversed" polarity starts low (and stays low) for the duration of the duty
cycle and goes high for the remainder of the period.
This IP _does_ start low, but it _doesn't_ stay low for the duration of the
duty cycle, as it then goes high for the duration of the duty cycle,
therefore this IP doesn't perfectly fit either ("normal" or "inverted")
definitions.
I think you can say that the "normal" signal is _shifted_ in phase for this
IP, rather than being "inverted".
>
> > + return pm_runtime_put(chip->dev);
>
> If you evaluate the return value of pm_runtime_put() maybe check
> pm_runtime_get_sync() for symmetry, too?
Or I could just discard it and return 0?
I am fine with either, what's your preference?
>
> > +}
> > +
> > +static int rzv2m_pwm_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> > + u64 period = state->period, duty_cycle = state->duty_cycle;
> > + u16 frequency_divisor;
> > + u64 pwm_cyc, pwm_low;
> > + u8 prescale;
> > + u32 pwm_ctr;
> > +
> > + /*
> > + * Clamp period and duty cycle to their maximum values for our
> current
> > + * configuration rather than letting our calculations overflow.
> > + */
> > + if (period > rzv2m_pwm->max_period) {
> > + period = rzv2m_pwm->max_period;
> > + if (duty_cycle > rzv2m_pwm->max_period)
> > + duty_cycle = period;
> > + }
> > +
> > + /*
> > + * Formula for calculating PWM Cycle Setting Register:
> > + * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) ? Div ratio)) -
> 1
> > + */
> > + pwm_cyc = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(period,
> > + rzv2m_pwm->rate,
> > + NSEC_PER_SEC);
> > + pwm_cyc = pwm_cyc ? pwm_cyc : 1;
>
> if pwm_cyc is 0 here, the period is too short to be realized, right?
> That's an error condition.
I will return an error instead.
>
> > + prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pwm_cyc - 1);
> > + frequency_divisor = 1 << rzv2m_pwm_prescale_to_shift(prescale);
> > + if (frequency_divisor > 1) {
> > + pwm_cyc = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(period,
> > + rzv2m_pwm->rate,
> > + NSEC_PER_SEC * frequency_divisor);
> > + pwm_cyc = pwm_cyc ? pwm_cyc : 1;
> > + }
> > +
> > + if (pwm_cyc && !FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc - 1))
> > + pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;
>
> I don't understand the relevance of FIELD_FIT(RZV2M_PWMCYC_PERIOD,
> pwm_cyc - 1).
CYC is only made of 24 bits, therefore this is to make sure we don't
go beyond a 24-bit representation.
>
> > +
> > + /*
> > + * Formula for calculating PWMLOW register:
> > + * PWMLOW register = PWM cycle * Low pulse width ratio (%)
> > + */
> > + pwm_low = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(duty_cycle,
> > + rzv2m_pwm->rate, NSEC_PER_SEC * frequency_divisor);
> > +
> > + pwm_low = pwm_cyc - pwm_low;
>
> Either the old or the new value of pwm_low doesn't match the variable's
> name. Please add another variable for the wrong one. The compiler should
> optimize that out and the reader can more easily understand the code.
You are right, I'll use an intermediate variable rather than leveraging
pwm_low, as it's confusing.
>
> > + if (!FIELD_FIT(RZV2M_PWMLOW_PERIOD, pwm_low))
> > + pwm_low = RZV2M_PWMLOW_PERIOD;
> > +
> > + pwm_cyc--;
> > +
> > + /*
> > + * 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(rzv2m_pwm->chip.dev);
> > +
> > + /*
> > + * To change the setting value of the PWM cycle setting register
> > + * (PWMm_PWMCYC) or polarity, set the PWME bit of the PWM control
> > + * register (PWMm_PWMCTR) to 0b and stop the counter operation.
> > + */
> > + if (rzv2m_pwm->polarity != state->polarity || rzv2m_pwm->pwm_cyc !=
> pwm_cyc) {
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
> 0);
> > + rzv2m_pwm_wait_delay(rzv2m_pwm);
> > + }
> > +
> > + rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMCYC, pwm_cyc);
> > + rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMLOW, pwm_low);
> > +
> > + pwm_ctr = FIELD_PREP(RZV2M_PWMCTR_PWMPS, prescale);
> > + if (state->polarity == PWM_POLARITY_INVERSED)
> > + pwm_ctr |= RZV2M_PWMCTR_PWMHL;
> > +
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMTM |
> > + RZV2M_PWMCTR_PWMPS | RZV2M_PWMCTR_PWMHL, pwm_ctr);
> > +
> > + if (rzv2m_pwm->polarity != state->polarity || rzv2m_pwm->pwm_cyc !=
> pwm_cyc) {
> > + rzv2m_pwm->polarity = state->polarity;
> > + rzv2m_pwm->pwm_cyc = pwm_cyc;
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
> > + RZV2M_PWMCTR_PWME);
> > + }
> > +
> > + rzv2m_pwm_wait_delay(rzv2m_pwm);
> > +
> > + /* If the PWM is not enabled, turn the clock off again to save power.
> */
> > + if (!pwm->state.enabled)
> > + pm_runtime_put(rzv2m_pwm->chip.dev);
> > +
> > + return 0;
> > +}
> > [...]
> > +static int rzv2m_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct rzv2m_pwm_chip *rzv2m_pwm;
> > + unsigned long apb_clk_rate;
> > + int ret;
> > +
> > + rzv2m_pwm = devm_kzalloc(&pdev->dev, sizeof(*rzv2m_pwm), GFP_KERNEL);
> > + if (!rzv2m_pwm)
> > + return -ENOMEM;
> > +
> > + rzv2m_pwm->mmio = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(rzv2m_pwm->mmio))
> > + return PTR_ERR(rzv2m_pwm->mmio);
> > +
> > + rzv2m_pwm->apb_clk = devm_clk_get(&pdev->dev, "apb");
> > + if (IS_ERR(rzv2m_pwm->apb_clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->apb_clk),
> > + "cannot get apb clock\n");
> > +
> > + rzv2m_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
> > + if (IS_ERR(rzv2m_pwm->pwm_clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->pwm_clk),
> > + "cannot get pwm clock\n");
> > +
> > + rzv2m_pwm->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> > + if (IS_ERR(rzv2m_pwm->rstc))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->rstc),
> > + "get reset failed\n");
> > +
> > + platform_set_drvdata(pdev, rzv2m_pwm);
> > + ret = reset_control_deassert(rzv2m_pwm->rstc);
> > + if (ret) {
> > + return dev_err_probe(&pdev->dev, ret,
> > + "cannot deassert reset control\n");
> > + }
> > +
> > + ret = clk_prepare_enable(rzv2m_pwm->apb_clk);
> > + if (ret < 0)
> > + goto err_reset;
> > +
> > + ret = clk_prepare_enable(rzv2m_pwm->pwm_clk);
> > + if (ret < 0)
> > + goto disable_apb_clk;
> > +
> > + clk_rate_exclusive_get(rzv2m_pwm->apb_clk);
>
> There is a devm_clk_rate_exclusive_get() in next starting from tomorrow
> I hope. Using it should simplify the driver.
I'll have a look and adjust accordingly.
Thanks!
Cheers,
Fab
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Fabrizio,
your MUA introduces strange line breaks. You could do every reader of
you mails a favour and fix that. I fixed it up for my reply.
On Thu, Feb 29, 2024 at 10:45:01PM +0000, Fabrizio Castro wrote:
> > From: Uwe Kleine-K?nig <[email protected]>
> > Sent: Thursday, February 29, 2024 4:42 PM
> > To: Fabrizio Castro <[email protected]>
> > Subject: Re: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver
> >
> > On Mon, Feb 12, 2024 at 09:06:50PM +0000, Fabrizio Castro wrote:
> > > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b,
> > u64 c)
> > > +{
> > > + u64 ab = a * b;
> >
> > a * b might overflow?!
>
> In the context of this driver, this cannot overflow.
> The 2 formulas the above is needed for are:
> 1) period = (cyc + 1)*(NSEC_PER_SEC * frequency_divisor)/rate
> 2) duty_cycle = (cyc + 1 - low)*(NSEC_PER_SEC * frequency_divisor)/rate
>
> With respect to 1), the dividend overflows when period * rate also
> overflows (its product is calculated in rzv2m_pwm_config).
> However, limiting the period to a maximum value of U64_MAX / rate
> prevents the calculations from overflowing (in both directions, from period to cyc, and from cyc to period). v6 introduced max_period for this.
> The situation for 2) is very similar to 1), with duty_cycle<=period,
> therefore limiting period to a max value (and clamping the duty cycle
> accordingly) will ensure that the calculation for duty_cycle won't
> overflow, either.
OK, so it might be right from a technical POV. From a maintainer POV
this is still bad. Authors for other drivers might copy it, or the driver
might be changed and there is no indication that the the function relies
on only be called with certain parameters.
> > > + u64 d = div64_u64(ab, c);
> > > + u64 e = d * c;
> > > +
> > > + return d + ((ab - e) ? 1 : 0);
> > > +}
> > > +
> > > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_rounddown(u64 a, u64 b,
> > u64 c)
> > > +{
> > > + return div64_u64(a * b, c);
> >
> > ditto. This is the same function as mul_u64_u64_div_u64() isn't it?
>
> Since a * b cannot overflow in the case of this driver, I believe the
> above to be a better option than mul_u64_u64_div_u64.
Same technical POV vs maintainer POV as above. Plus: Even if
mul_u64_u64_div_u64 is a tad slower, reusing it has some benefits
nevertheless.
> > > [...]
> > > + cyc = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> > > + state->period = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1,
> > > + NSEC_PER_SEC * frequency_divisor,
> > > + rzv2m_pwm->rate);
> > > +
> > > + low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> > > + state->duty_cycle = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1 - low,
> > > + NSEC_PER_SEC * frequency_divisor,
> > > + rzv2m_pwm->rate);
> >
> > The register semantic makes me wonder if each period starts with the low
> > part. In that case the hardware called "normal" what is called inverted
> > in the pwm framework?!
>
> My understanding is that the PWM framework defines "normal" polarity a
> signal that starts high (and stays high) for the duration of the duty cycle,
> and goes low for the remainder of the period. Conversely, a signal with
> "inversed" polarity starts low (and stays low) for the duration of the duty
> cycle and goes high for the remainder of the period.
Ack.
> This IP _does_ start low, but it _doesn't_ stay low for the duration of the
> duty cycle, as it then goes high for the duration of the duty cycle,
> therefore this IP doesn't perfectly fit either ("normal" or "inverted")
> definitions.
> I think you can say that the "normal" signal is _shifted_ in phase for this
> IP, rather than being "inverted".
Alternatively (and a better match): What you describe is an inverted
wave form with duty_cycle = period - duty_cycle.
> > > + return pm_runtime_put(chip->dev);
> >
> > If you evaluate the return value of pm_runtime_put() maybe check
> > pm_runtime_get_sync() for symmetry, too?
>
> Or I could just discard it and return 0?
> I am fine with either, what's your preference?
My preference would be to always check the return value, but given that
many drivers don't care for that, I agree to accept never checking it.
So choose one option and do it consistently please.
> > > + if (pwm_cyc && !FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc - 1))
> > > + pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;
> >
> > I don't understand the relevance of FIELD_FIT(RZV2M_PWMCYC_PERIOD,
> > pwm_cyc - 1).
>
> CYC is only made of 24 bits, therefore this is to make sure we don't
> go beyond a 24-bit representation.
I would have understood:
if (FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc + 1))
pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;
Notice there are three changes compared to your variant:
- drop pwm_cyc != 0 check
- drop ! from FIELD_FIT
- pwm_cyc + 1 instead of pwm_cyc - 1
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Uwe,
Thanks for your feedback!
> From: Uwe Kleine-K?nig <[email protected]>
> Sent: Friday, March 1, 2024 9:15 AM
> To: Fabrizio Castro <[email protected]>
> Subject: Re: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver
>
> Hello Fabrizio,
>
> your MUA introduces strange line breaks. You could do every reader of you mails a favour and fix that.
I think I understand what you mean, long lines would be wrapped onto new
lines, which are then indented incorrectly?
Hopefully it's been fixed now.
Thanks for highlighting this, as it has been going on for some time,
and was never noticed before.
> I fixed it up for my reply.
Thanks for that.
>
> On Thu, Feb 29, 2024 at 10:45:01PM +0000, Fabrizio Castro wrote:
> > > From: Uwe Kleine-K?nig <[email protected]>
> > > Sent: Thursday, February 29, 2024 4:42 PM
> > > To: Fabrizio Castro <[email protected]>
> > > Subject: Re: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver
> > >
> > > On Mon, Feb 12, 2024 at 09:06:50PM +0000, Fabrizio Castro wrote:
> > > > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_roundup(u64 a,
> > > > +u64 b,
> > > u64 c)
> > > > +{
> > > > + u64 ab = a * b;
> > >
> > > a * b might overflow?!
> >
> > In the context of this driver, this cannot overflow.
> > The 2 formulas the above is needed for are:
> > 1) period = (cyc + 1)*(NSEC_PER_SEC * frequency_divisor)/rate
> > 2) duty_cycle = (cyc + 1 - low)*(NSEC_PER_SEC *
> > frequency_divisor)/rate
> >
> > With respect to 1), the dividend overflows when period * rate also
> > overflows (its product is calculated in rzv2m_pwm_config).
> > However, limiting the period to a maximum value of U64_MAX / rate
> > prevents the calculations from overflowing (in both directions, from period to cyc, and from cyc to
> period). v6 introduced max_period for this.
> > The situation for 2) is very similar to 1), with duty_cycle<=period,
> > therefore limiting period to a max value (and clamping the duty cycle
> > accordingly) will ensure that the calculation for duty_cycle won't
> > overflow, either.
>
> OK, so it might be right from a technical POV. From a maintainer POV this is still bad. Authors for
> other drivers might copy it, or the driver might be changed and there is no indication that the the
> function relies on only be called with certain parameters.
I could add comments to clarify this, or checks to make sure the parameters
are passed as expected, or both?
Or if you have a better suggestion?
I would still like to be able to use the below formula if possible, as it
allows for the smallest restriction on the period:
(a * b) / c + ( (a * b) - (((a * b) / c) * c) ? 1 : 0 )
>
> > > > + u64 d = div64_u64(ab, c);
> > > > + u64 e = d * c;
> > > > +
> > > > + return d + ((ab - e) ? 1 : 0);
> > > > +}
> > > > +
> > > > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_rounddown(u64 a,
> > > > +u64 b,
> > > u64 c)
> > > > +{
> > > > + return div64_u64(a * b, c);
> > >
> > > ditto. This is the same function as mul_u64_u64_div_u64() isn't it?
> >
> > Since a * b cannot overflow in the case of this driver, I believe the
> > above to be a better option than mul_u64_u64_div_u64.
>
> Same technical POV vs maintainer POV as above. Plus: Even if
> mul_u64_u64_div_u64 is a tad slower, reusing it has some benefits nevertheless.
I'll use mul_u64_u64_div_u64 instead.
>
> > > > [...]
> > > > + cyc = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> > > > + state->period = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1,
> > > > + NSEC_PER_SEC * frequency_divisor,
> > > > + rzv2m_pwm->rate);
> > > > +
> > > > + low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> > > > + state->duty_cycle = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1 - low,
> > > > + NSEC_PER_SEC * frequency_divisor,
> > > > + rzv2m_pwm->rate);
> > >
> > > The register semantic makes me wonder if each period starts with the
> > > low part. In that case the hardware called "normal" what is called
> > > inverted in the pwm framework?!
> >
> > My understanding is that the PWM framework defines "normal" polarity a
> > signal that starts high (and stays high) for the duration of the duty
> > cycle, and goes low for the remainder of the period. Conversely, a
> > signal with "inversed" polarity starts low (and stays low) for the
> > duration of the duty cycle and goes high for the remainder of the period.
>
> Ack.
>
> > This IP _does_ start low, but it _doesn't_ stay low for the duration
> > of the duty cycle, as it then goes high for the duration of the duty
> > cycle, therefore this IP doesn't perfectly fit either ("normal" or
> > "inverted") definitions.
> > I think you can say that the "normal" signal is _shifted_ in phase for
> > this IP, rather than being "inverted".
>
> Alternatively (and a better match): What you describe is an inverted wave form with duty_cycle = period
> - duty_cycle.
That is also true. Also, it'll have the benefit of getting the first
period out of the door without a shift in phase.
I'll adjust accordingly.
>
> > > > + return pm_runtime_put(chip->dev);
> > >
> > > If you evaluate the return value of pm_runtime_put() maybe check
> > > pm_runtime_get_sync() for symmetry, too?
> >
> > Or I could just discard it and return 0?
> > I am fine with either, what's your preference?
>
> My preference would be to always check the return value, but given that many drivers don't care for
> that, I agree to accept never checking it.
> So choose one option and do it consistently please.
Thanks.
>
> > > > + if (pwm_cyc && !FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc - 1))
> > > > + pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;
> > >
> > > I don't understand the relevance of FIELD_FIT(RZV2M_PWMCYC_PERIOD,
> > > pwm_cyc - 1).
> >
> > CYC is only made of 24 bits, therefore this is to make sure we don't
> > go beyond a 24-bit representation.
>
> I would have understood:
>
> if (FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc + 1))
> pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;
By the time the above test is run, pwm_cyc is incremented by 1, therefore
it needs to be decremented in order to be tested, and incremented when
re-assigned. pwm_cyc was left incremented to simplify the pwm_low calculation,
but I can see it's very confusing, therefore I'll improve this in v8.
Cheers,
Fab
>
> Notice there are three changes compared to your variant:
> - drop pwm_cyc != 0 check
> - drop ! from FIELD_FIT
> - pwm_cyc + 1 instead of pwm_cyc - 1
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | https://www.pengutronix.de/ |