[PATCH v5 0/4] Add RZ/V2{M, MA} PWM driver support

[PATCH v5 0/4] Add RZ/V2{M, MA} PWM driver support

[Biju Das]

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.

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.

Note:
 Hardware manual for this IP can be found here
 https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardware?language=en

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                       | 451 ++++++++++++++++++
 6 files changed, 721 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/renesas,rzv2m-pwm.yaml
 create mode 100644 drivers/pwm/pwm-rzv2m.c


base-commit: 92554cdd428fce212d2a71a06939e7cab90f7c77
-- 
2.25.1

[PATCH v5 1/4] dt-bindings: pwm: Add RZ/V2M PWM binding

[Biju Das]

Add device tree bindings for the RZ/V2{M, MA} PWM Timer (PWM).

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
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 <biju.das.jz@bp.renesas.com>
+
+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.25.1

[PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver

[Biju Das]

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 <biju.das.jz@bp.renesas.com>
---
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 | 451 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 463 insertions(+)
 create mode 100644 drivers/pwm/pwm-rzv2m.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 6210babb0741..1c8dbb064ee5 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -514,6 +514,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 c822389c2a24..ee190df16279 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -47,6 +47,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..0c5c0b9f0e70
--- /dev/null
+++ b/drivers/pwm/pwm-rzv2m.c
@@ -0,0 +1,451 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2M PWM Timer (PWM) driver
+ *
+ * Copyright (C) 2023 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/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)
+
+struct rzv2m_pwm_chip {
+	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 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 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);
+	u32 period, low, val;
+	u8 prescale;
+
+	pm_runtime_get_sync(chip->dev);
+	val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
+	state->enabled = FIELD_GET(RZV2M_PWMCTR_PWME, val);
+	state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, val) ?
+		PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, val);
+	period = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
+	low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
+	if (low > period) /* 0% duty cycle */
+		val = 0;
+	else if (!low) /* 100% duty cycle */
+		val = period;
+	else
+		val = period - low;
+
+	if (period)
+		period += 1;
+
+	state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period << (4 * prescale),
+					 rzv2m_pwm->rate);
+	state->duty_cycle = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val << (4 * prescale),
+					     rzv2m_pwm->rate);
+	pm_runtime_put(chip->dev);
+
+	return 0;
+}
+
+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);
+	unsigned long pwm_cyc, pwm_low;
+	u8 prescale;
+	u32 pwm_ctr;
+	u64 pc, dc;
+
+	/*
+	 * Refuse clk rates > 1 GHz to prevent overflowing the following
+	 * calculation.
+	 */
+	if (rzv2m_pwm->rate > NSEC_PER_SEC)
+		return -EINVAL;
+
+	/*
+	 * Formula for calculating PWM Cycle Setting Register
+	 * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) × Div ratio)) - 1
+	 */
+	pc = mul_u64_u32_div(state->period, rzv2m_pwm->rate, NSEC_PER_SEC);
+	dc = mul_u64_u32_div(state->duty_cycle, rzv2m_pwm->rate, NSEC_PER_SEC);
+	prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pc);
+
+	pwm_cyc = pc >> (4 * prescale);
+	if (pwm_cyc)
+		pwm_cyc -= 1;
+
+	if (!FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc))
+		pwm_cyc = FIELD_MAX(RZV2M_PWMCYC_PERIOD);
+
+	/*
+	 * Formula for calculating PWMLOW register
+	 * PWMLOW register = PWM cycle * Low pulse width ratio (%)
+	 */
+	pwm_low = dc >> (4 * prescale);
+	if (!dc) /* 0% duty cycle */
+		pwm_low = pwm_cyc + 1;
+	else if (pc == dc) /* 100% duty cycle */
+		pwm_low = 0;
+	else
+		pwm_low = pwm_cyc - pwm_low;
+
+	/*
+	 * 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,
+	.owner = THIS_MODULE,
+};
+
+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 err;
+
+	err = clk_prepare_enable(rzv2m_pwm->apb_clk);
+	if (err)
+		return err;
+
+	err = clk_prepare_enable(rzv2m_pwm->pwm_clk);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+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;
+
+	/*
+	 * 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 = of_match_ptr(rzv2m_pwm_of_table),
+	},
+	.probe = rzv2m_pwm_probe,
+};
+module_platform_driver(rzv2m_pwm_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/V2M PWM Timer Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pwm-rzv2m");
-- 
2.25.1

[PATCH v5 3/4] arm64: dts: renesas: r9a09g011: Add pwm nodes

[Biju Das]

Add device nodes for the pwm timer channels that are not assigned
to the ISP.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
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 46d67b200a66..b0af7404269b 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";
+		};
+
 		i2c0: i2c@a4030000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
2.25.1

[PATCH v5 4/4] arm64: dts: renesas: rzv2m evk: Enable pwm

[Biju Das]

Enable pwm{8..14} on RZ/V2M EVK.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
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.25.1

RE: [PATCH v5 0/4] Add RZ/V2{M, MA} PWM driver support

[Biju Das]

Hi Uwe,

Gentle ping.

Cheers,
Biju

> Subject: [PATCH v5 0/4] Add RZ/V2{M, MA} PWM driver support
> 
> 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.
> 
> 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.
> 
> Note:
>  Hardware manual for this IP can be found here
> https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-
> hardware?language=en
> 
> 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                       | 451 ++++++++++++++++++
>  6 files changed, 721 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/renesas,rzv2m-
> pwm.yaml
>  create mode 100644 drivers/pwm/pwm-rzv2m.c
> 
> 
> base-commit: 92554cdd428fce212d2a71a06939e7cab90f7c77
> --
> 2.25.1

RE: [PATCH v5 0/4] Add RZ/V2{M, MA} PWM driver support

[Biju Das]

Hi Uwe,

Gentle ping.

> Subject: RE: [PATCH v5 0/4] Add RZ/V2{M, MA} PWM driver support
> 
> Hi Uwe,
> 
> Gentle ping.
> 
> Cheers,
> Biju
> 
> > Subject: [PATCH v5 0/4] Add RZ/V2{M, MA} PWM driver support
> >
> > 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.
> >
> > 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.
> >
> > Note:
> >  Hardware manual for this IP can be found here
> > https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-
> > hardware?language=en
> >
> > 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                       | 451 ++++++++++++++++++
> >  6 files changed, 721 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pwm/renesas,rzv2m-
> > pwm.yaml
> >  create mode 100644 drivers/pwm/pwm-rzv2m.c
> >
> >
> > base-commit: 92554cdd428fce212d2a71a06939e7cab90f7c77
> > --
> > 2.25.1

Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 15370 bytes --]

Hello,

sorry this took so long.

On Fri, Jun 30, 2023 at 12:40:01PM +0100, Biju Das wrote:
> 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 <biju.das.jz@bp.renesas.com>
> ---
> 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 | 451 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 463 insertions(+)
>  create mode 100644 drivers/pwm/pwm-rzv2m.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 6210babb0741..1c8dbb064ee5 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -514,6 +514,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 c822389c2a24..ee190df16279 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -47,6 +47,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..0c5c0b9f0e70
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzv2m.c
> @@ -0,0 +1,451 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2M PWM Timer (PWM) driver
> + *
> + * Copyright (C) 2023 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/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)
> +
> +struct rzv2m_pwm_chip {
> +	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 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 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);
> +	u32 period, low, val;
> +	u8 prescale;
> +
> +	pm_runtime_get_sync(chip->dev);
> +	val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
> +	state->enabled = FIELD_GET(RZV2M_PWMCTR_PWME, val);
> +	state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, val) ?
> +		PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +	prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, val);
> +	period = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> +	low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> +	if (low > period) /* 0% duty cycle */
> +		val = 0;
> +	else if (!low) /* 100% duty cycle */
> +		val = period;
> +	else
> +		val = period - low;

This could be simplified to:

	if (low > period) /* 0% duty cycle */
		val = 0;
	else
		val = period - low;

Maybe it makes sense add another variable "duty_cycle" here and use that
for improved clarity? And then maybe rename "val" to "ctr"?

> +	if (period)
> +		period += 1;

This looks bogus, why don't you add 1 if RZV2M_PWMCYC is 0?

Also it suggests that the hardware cannot do a 100% relative duty cycle?
If I didn't miss anything here, please add that to the list of
Limitations above.

> +	state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period << (4 * prescale),
> +					 rzv2m_pwm->rate);

The multiplication can overflow. I see no easy way to prevent this
without introducing a mul_u64_u64_div_roundup helper. Maybe I miss
something?

> +	state->duty_cycle = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val << (4 * prescale),
> +					     rzv2m_pwm->rate);
> +	pm_runtime_put(chip->dev);
> +
> +	return 0;
> +}
> +
> +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);
> +	unsigned long pwm_cyc, pwm_low;
> +	u8 prescale;
> +	u32 pwm_ctr;
> +	u64 pc, dc;
> +
> +	/*
> +	 * Refuse clk rates > 1 GHz to prevent overflowing the following
> +	 * calculation.
> +	 */
> +	if (rzv2m_pwm->rate > NSEC_PER_SEC)
> +		return -EINVAL;
> +
> +	/*
> +	 * Formula for calculating PWM Cycle Setting Register
> +	 * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) × Div ratio)) - 1
> +	 */
> +	pc = mul_u64_u32_div(state->period, rzv2m_pwm->rate, NSEC_PER_SEC);

rzv2m_pwm->rate is an unsigned long. So mul_u64_u32_div is the wrong
function (at least on archs where sizeof(long) > 4).

> +	dc = mul_u64_u32_div(state->duty_cycle, rzv2m_pwm->rate, NSEC_PER_SEC);
> +	prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pc);
> +
> +	pwm_cyc = pc >> (4 * prescale);

The division above might have round down the result, so you're losing
precision here?

> +	if (pwm_cyc)
> +		pwm_cyc -= 1;

If the requested period is too small, please refuse the request.
PWM_DEBUG should catch that and emit a warning.

> +	if (!FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc))
> +		pwm_cyc = FIELD_MAX(RZV2M_PWMCYC_PERIOD);
> +
> +	/*
> +	 * Formula for calculating PWMLOW register
> +	 * PWMLOW register = PWM cycle * Low pulse width ratio (%)
> +	 */
> +	pwm_low = dc >> (4 * prescale);
> +	if (!dc) /* 0% duty cycle */
> +		pwm_low = pwm_cyc + 1;

if pwm_cyc == FIELD_MAX(RZV2M_PWMCYC_PERIOD) that + 1 isn't a good idea,
is it?

> +	else if (pc == dc) /* 100% duty cycle */
> +		pwm_low = 0;
> +	else
> +		pwm_low = pwm_cyc - pwm_low;
> +
> +	/*
> +	 * 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);

Can it happen that this already makes the hardware emit a (probably
wrong) signal?

> +	/*
> +	 * 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,
> +	.owner = THIS_MODULE,
> +};
> +
> +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 err;
> +
> +	err = clk_prepare_enable(rzv2m_pwm->apb_clk);
> +	if (err)
> +		return err;
> +
> +	err = clk_prepare_enable(rzv2m_pwm->pwm_clk);
> +	if (err)
> +		return err;

It would be consequent here to disable apb_clk in the error case to
prevent a clk enable imbalance.

> +
> +	return 0;
> +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver

[Biju Das]

Hi Uwe,

Thanks for the feedback.

As I am busy with other activities, Fabrizio will respond to your comments as he has RZ/V2M board.

Cheers,
Biju

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Saturday, September 16, 2023 4:33 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>; Philipp Zabel
> <p.zabel@pengutronix.de>; Geert Uytterhoeven <geert+renesas@glider.be>;
> Magnus Damm <magnus.damm@gmail.com>; linux-pwm@vger.kernel.org; linux-
> renesas-soc@vger.kernel.org; Fabrizio Castro
> <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver
> 
> Hello,
> 
> sorry this took so long.
> 
> On Fri, Jun 30, 2023 at 12:40:01PM +0100, Biju Das wrote:
> > 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 <biju.das.jz@bp.renesas.com>
> > ---
> > 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 | 451
> > ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 463 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-rzv2m.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 6210babb0741..1c8dbb064ee5 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -514,6 +514,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 c822389c2a24..ee190df16279 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -47,6 +47,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..0c5c0b9f0e70
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzv2m.c
> > @@ -0,0 +1,451 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/V2M PWM Timer (PWM) driver
> > + *
> > + * Copyright (C) 2023 Renesas Electronics Corporation
> > + *
> > + * Hardware manual for this IP can be found here
> > + *
> > +https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardwar
> > +e?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/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)
> > +
> > +struct rzv2m_pwm_chip {
> > +	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 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 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);
> > +	u32 period, low, val;
> > +	u8 prescale;
> > +
> > +	pm_runtime_get_sync(chip->dev);
> > +	val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
> > +	state->enabled = FIELD_GET(RZV2M_PWMCTR_PWME, val);
> > +	state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, val) ?
> > +		PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > +	prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, val);
> > +	period = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> > +	low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> > +	if (low > period) /* 0% duty cycle */
> > +		val = 0;
> > +	else if (!low) /* 100% duty cycle */
> > +		val = period;
> > +	else
> > +		val = period - low;
> 
> This could be simplified to:
> 
> 	if (low > period) /* 0% duty cycle */
> 		val = 0;
> 	else
> 		val = period - low;
> 
> Maybe it makes sense add another variable "duty_cycle" here and use that
> for improved clarity? And then maybe rename "val" to "ctr"?
> 
> > +	if (period)
> > +		period += 1;
> 
> This looks bogus, why don't you add 1 if RZV2M_PWMCYC is 0?
> 
> Also it suggests that the hardware cannot do a 100% relative duty cycle?
> If I didn't miss anything here, please add that to the list of Limitations
> above.
> 
> > +	state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period << (4 *
> prescale),
> > +					 rzv2m_pwm->rate);
> 
> The multiplication can overflow. I see no easy way to prevent this without
> introducing a mul_u64_u64_div_roundup helper. Maybe I miss something?
> 
> > +	state->duty_cycle = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val << (4 *
> prescale),
> > +					     rzv2m_pwm->rate);
> > +	pm_runtime_put(chip->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +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);
> > +	unsigned long pwm_cyc, pwm_low;
> > +	u8 prescale;
> > +	u32 pwm_ctr;
> > +	u64 pc, dc;
> > +
> > +	/*
> > +	 * Refuse clk rates > 1 GHz to prevent overflowing the following
> > +	 * calculation.
> > +	 */
> > +	if (rzv2m_pwm->rate > NSEC_PER_SEC)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Formula for calculating PWM Cycle Setting Register
> > +	 * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) × Div ratio)) -
> 1
> > +	 */
> > +	pc = mul_u64_u32_div(state->period, rzv2m_pwm->rate, NSEC_PER_SEC);
> 
> rzv2m_pwm->rate is an unsigned long. So mul_u64_u32_div is the wrong
> function (at least on archs where sizeof(long) > 4).
> 
> > +	dc = mul_u64_u32_div(state->duty_cycle, rzv2m_pwm->rate,
> NSEC_PER_SEC);
> > +	prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pc);
> > +
> > +	pwm_cyc = pc >> (4 * prescale);
> 
> The division above might have round down the result, so you're losing
> precision here?
> 
> > +	if (pwm_cyc)
> > +		pwm_cyc -= 1;
> 
> If the requested period is too small, please refuse the request.
> PWM_DEBUG should catch that and emit a warning.
> 
> > +	if (!FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc))
> > +		pwm_cyc = FIELD_MAX(RZV2M_PWMCYC_PERIOD);
> > +
> > +	/*
> > +	 * Formula for calculating PWMLOW register
> > +	 * PWMLOW register = PWM cycle * Low pulse width ratio (%)
> > +	 */
> > +	pwm_low = dc >> (4 * prescale);
> > +	if (!dc) /* 0% duty cycle */
> > +		pwm_low = pwm_cyc + 1;
> 
> if pwm_cyc == FIELD_MAX(RZV2M_PWMCYC_PERIOD) that + 1 isn't a good idea, is
> it?
> 
> > +	else if (pc == dc) /* 100% duty cycle */
> > +		pwm_low = 0;
> > +	else
> > +		pwm_low = pwm_cyc - pwm_low;
> > +
> > +	/*
> > +	 * 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);
> 
> Can it happen that this already makes the hardware emit a (probably
> wrong) signal?
> 
> > +	/*
> > +	 * 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,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +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 err;
> > +
> > +	err = clk_prepare_enable(rzv2m_pwm->apb_clk);
> > +	if (err)
> > +		return err;
> > +
> > +	err = clk_prepare_enable(rzv2m_pwm->pwm_clk);
> > +	if (err)
> > +		return err;
> 
> It would be consequent here to disable apb_clk in the error case to prevent
> a clk enable imbalance.
> 
> > +
> > +	return 0;
> > +}
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

RE: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver

[Fabrizio Castro]

Hi Uwe,

Thanks for your reply!

> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Subject: Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver
> 

[..]

> > +	if (low > period) /* 0% duty cycle */
> > +		val = 0;
> > +	else if (!low) /* 100% duty cycle */
> > +		val = period;
> > +	else
> > +		val = period - low;
> 
> This could be simplified to:
> 
> 	if (low > period) /* 0% duty cycle */
> 		val = 0;
> 	else
> 		val = period - low;

Agreed. And also, I believe there is a tidier way of addressing this.
I'll rework this for v6.

> 
> Maybe it makes sense add another variable "duty_cycle" here and use
> that
> for improved clarity? And then maybe rename "val" to "ctr"?

Agreed.

> 
> > +	if (period)
> > +		period += 1;
> 
> This looks bogus, why don't you add 1 if RZV2M_PWMCYC is 0?

Agreed. We should always add 1.

> 
> Also it suggests that the hardware cannot do a 100% relative duty
> cycle?

It does support 100% duty cycle.
PWMCYC = 0 actually means 1 clock cycle, that's why the faff with
increment and decrement operations, and that's why the confusion.

> If I didn't miss anything here, please add that to the list of
> Limitations above.

Thankfully not a limitation.

> 
> > +	state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period << (4
> * prescale),
> > +					 rzv2m_pwm->rate);
> 
> The multiplication can overflow. I see no easy way to prevent this
> without introducing a mul_u64_u64_div_roundup helper. Maybe I miss
> something?

It does overflow, good catch!
I think we could split this in three operations for now, and maybe
improve it later on:
period = NSEC_PER_SEC * (cyc + 1);
period = DIV64_U64_ROUND_UP(period, rzv2m_pwm->rate);
period <<= rzv2m_pwm_prescale_to_shift(prescale);

with rzv2m_pwm_prescale_to_shift as below:
static inline int rzv2m_pwm_prescale_to_shift(u8 prescale)
{
    return prescale == 3 ? 11 : prescale * 4;
}

As it turns out "<< (4 * prescale)" and ">> (4 * prescale)" are not
correct for prescale == 3.
As per manual: PWMPS = 3 means a frequency division by 2048, and not
4096.
I'll address this in v6.

> 
> > +	state->duty_cycle = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val <<
> (4 * prescale),
> > +					     rzv2m_pwm->rate);
> > +	pm_runtime_put(chip->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +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);
> > +	unsigned long pwm_cyc, pwm_low;
> > +	u8 prescale;
> > +	u32 pwm_ctr;
> > +	u64 pc, dc;
> > +
> > +	/*
> > +	 * Refuse clk rates > 1 GHz to prevent overflowing the following
> > +	 * calculation.
> > +	 */
> > +	if (rzv2m_pwm->rate > NSEC_PER_SEC)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Formula for calculating PWM Cycle Setting Register
> > +	 * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) × Div
> ratio)) - 1
> > +	 */
> > +	pc = mul_u64_u32_div(state->period, rzv2m_pwm->rate,
> NSEC_PER_SEC);
> 
> rzv2m_pwm->rate is an unsigned long. So mul_u64_u32_div is the wrong
> function (at least on archs where sizeof(long) > 4).

Indeed. I think mul_u64_u64_div_u64 is more appropriate.

> 
> > +	dc = mul_u64_u32_div(state->duty_cycle, rzv2m_pwm->rate,
> NSEC_PER_SEC);
> > +	prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pc);
> > +
> > +	pwm_cyc = pc >> (4 * prescale);
> 
> The division above might have round down the result, so you're losing
> precision here?

Indeed. I think DIV_ROUND_UP will do better.

> 
> > +	if (pwm_cyc)
> > +		pwm_cyc -= 1;
> 
> If the requested period is too small, please refuse the request.

Agreed.

> PWM_DEBUG should catch that and emit a warning.
> 
> > +	if (!FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc))
> > +		pwm_cyc = FIELD_MAX(RZV2M_PWMCYC_PERIOD);
> > +
> > +	/*
> > +	 * Formula for calculating PWMLOW register
> > +	 * PWMLOW register = PWM cycle * Low pulse width ratio (%)
> > +	 */
> > +	pwm_low = dc >> (4 * prescale);
> > +	if (!dc) /* 0% duty cycle */
> > +		pwm_low = pwm_cyc + 1;
> 
> if pwm_cyc == FIELD_MAX(RZV2M_PWMCYC_PERIOD) that + 1 isn't a good
> idea,
> is it?

Indeed, the final value should be tested. Perhaps we should refuse the
request if the value of PWMCYC doesn't fit, and we should probably do
the same thing with the value of PWMLOW (which is higher than PWMCYC
when duty cycle is 0%).

> 
> > +	else if (pc == dc) /* 100% duty cycle */
> > +		pwm_low = 0;
> > +	else
> > +		pwm_low = pwm_cyc - pwm_low;
> > +
> > +	/*
> > +	 * 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);
> 
> Can it happen that this already makes the hardware emit a (probably
> wrong) signal?

It doesn't look like this can happen with v5.
PWME set to 1 basically enables operations, and that happens after the
PWM has been configured.

[..]

> > +
> > +static int rzv2m_pwm_pm_runtime_resume(struct device *dev)
> > +{
> > +	struct rzv2m_pwm_chip *rzv2m_pwm = dev_get_drvdata(dev);
> > +	int err;
> > +
> > +	err = clk_prepare_enable(rzv2m_pwm->apb_clk);
> > +	if (err)
> > +		return err;
> > +
> > +	err = clk_prepare_enable(rzv2m_pwm->pwm_clk);
> > +	if (err)
> > +		return err;
> 
> It would be consequent here to disable apb_clk in the error case to
> prevent a clk enable imbalance.

Indeed.

Cheers,
Fab

> 
> > +
> > +	return 0;
> > +}
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://www.pengutronix.de/ |

Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]

Hello Fabrizio,

On Tue, Oct 03, 2023 at 09:19:41PM +0000, Fabrizio Castro wrote:
> > > +	if (period)
> > > +		period += 1;
> > 
> > This looks bogus, why don't you add 1 if RZV2M_PWMCYC is 0?
> 
> Agreed. We should always add 1.
> 
> > Also it suggests that the hardware cannot do a 100% relative duty
> > cycle?
> 
> It does support 100% duty cycle.
> PWMCYC = 0 actually means 1 clock cycle, that's why the faff with
> increment and decrement operations, and that's why the confusion.

So it doesn't support a 0% relative duty cycle?

> > If I didn't miss anything here, please add that to the list of
> > Limitations above.
> 
> Thankfully not a limitation.
> 
> > 
> > > +	state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period << (4
> > * prescale),
> > > +					 rzv2m_pwm->rate);
> > 
> > The multiplication can overflow. I see no easy way to prevent this
> > without introducing a mul_u64_u64_div_roundup helper. Maybe I miss
> > something?
> 
> It does overflow, good catch!
> I think we could split this in three operations for now, and maybe
> improve it later on:
> period = NSEC_PER_SEC * (cyc + 1);
> period = DIV64_U64_ROUND_UP(period, rzv2m_pwm->rate);
> period <<= rzv2m_pwm_prescale_to_shift(prescale);

You're loosing precision here though. With /^ = div_round_up:

	1000000000 * 5 /^ 3 << 2 == 6666666668
	1000000000 * 5 << 2 /^ 3 == 6666666667

So I correct my statement to: I see no easy and exact way to prevent an
overflow without introducing a mul_u64_u64_div_roundup helper. :-)

> with rzv2m_pwm_prescale_to_shift as below:
> static inline int rzv2m_pwm_prescale_to_shift(u8 prescale)
> {
>     return prescale == 3 ? 11 : prescale * 4;
> }
> 
> As it turns out "<< (4 * prescale)" and ">> (4 * prescale)" are not
> correct for prescale == 3.
> As per manual: PWMPS = 3 means a frequency division by 2048, and not
> 4096.

funny hardware.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver

[Fabrizio Castro]

Hi Uwe,

Thanks for your reply!

> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Wednesday, October 4, 2023 3:15 PM
> Subject: Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver
> 
> Hello Fabrizio,
> 
> On Tue, Oct 03, 2023 at 09:19:41PM +0000, Fabrizio Castro wrote:
> > > > +	if (period)
> > > > +		period += 1;
> > >
> > > This looks bogus, why don't you add 1 if RZV2M_PWMCYC is 0?
> >
> > Agreed. We should always add 1.
> >
> > > Also it suggests that the hardware cannot do a 100% relative duty
> > > cycle?
> >
> > It does support 100% duty cycle.
> > PWMCYC = 0 actually means 1 clock cycle, that's why the faff with
> > increment and decrement operations, and that's why the confusion.
> 
> So it doesn't support a 0% relative duty cycle?

It does support 0% duty cycle.

> 
> > > If I didn't miss anything here, please add that to the list of
> > > Limitations above.
> >
> > Thankfully not a limitation.
> >
> > >
> > > > +	state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period
> << (4
> > > * prescale),
> > > > +					 rzv2m_pwm->rate);
> > >
> > > The multiplication can overflow. I see no easy way to prevent this
> > > without introducing a mul_u64_u64_div_roundup helper. Maybe I miss
> > > something?
> >
> > It does overflow, good catch!
> > I think we could split this in three operations for now, and maybe
> > improve it later on:
> > period = NSEC_PER_SEC * (cyc + 1);
> > period = DIV64_U64_ROUND_UP(period, rzv2m_pwm->rate);
> > period <<= rzv2m_pwm_prescale_to_shift(prescale);
> 
> You're loosing precision here though. With /^ = div_round_up:
> 
> 	1000000000 * 5 /^ 3 << 2 == 6666666668
> 	1000000000 * 5 << 2 /^ 3 == 6666666667

Yep.

> 
> So I correct my statement to: I see no easy and exact way to prevent
> an
> overflow without introducing a mul_u64_u64_div_roundup helper. :-)

Indeed. In my mind, the best way to tackle this problem is for such an
API to use an internal representation of 128-bit, so that it can calculate
(a*b + c - 1)/c accurately, and return both the high and low part of the
128-bit result (and maybe keeping the high part result optional?).
Something like
u64 mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c, u64 *res_high);
or
void mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c, u64 *res_high, u64 *res_low);

Needless to say, the API may take quite some time to perform the calculations,
but if precision/accuracy is what we need, there is probably no better
way of addressing this, unless you have a better idea?

Looking around, my understanding is that other drivers may benefit from
this sort of accurate rounded up result when a, b, and c are 64-bit, is that
why you think it has come the time to address this, or do you think this
is the first instance of such need?

Thanks,
Fab

> 
> > with rzv2m_pwm_prescale_to_shift as below:
> > static inline int rzv2m_pwm_prescale_to_shift(u8 prescale)
> > {
> >     return prescale == 3 ? 11 : prescale * 4;
> > }
> >
> > As it turns out "<< (4 * prescale)" and ">> (4 * prescale)" are not
> > correct for prescale == 3.
> > As per manual: PWMPS = 3 means a frequency division by 2048, and not
> > 4096.
> 
> funny hardware.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://www.pengutronix.de/ |

Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3666 bytes --]

On Tue, Oct 10, 2023 at 10:48:19AM +0000, Fabrizio Castro wrote:
> Hi Uwe,
> 
> Thanks for your reply!
> 
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: Wednesday, October 4, 2023 3:15 PM
> > Subject: Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver
> > 
> > Hello Fabrizio,
> > 
> > On Tue, Oct 03, 2023 at 09:19:41PM +0000, Fabrizio Castro wrote:
> > > > > +	if (period)
> > > > > +		period += 1;
> > > >
> > > > This looks bogus, why don't you add 1 if RZV2M_PWMCYC is 0?
> > >
> > > Agreed. We should always add 1.
> > >
> > > > Also it suggests that the hardware cannot do a 100% relative duty
> > > > cycle?
> > >
> > > It does support 100% duty cycle.
> > > PWMCYC = 0 actually means 1 clock cycle, that's why the faff with
> > > increment and decrement operations, and that's why the confusion.
> > 
> > So it doesn't support a 0% relative duty cycle?
> 
> It does support 0% duty cycle.
> 
> > 
> > > > If I didn't miss anything here, please add that to the list of
> > > > Limitations above.
> > >
> > > Thankfully not a limitation.
> > >
> > > >
> > > > > +	state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period
> > << (4
> > > > * prescale),
> > > > > +					 rzv2m_pwm->rate);
> > > >
> > > > The multiplication can overflow. I see no easy way to prevent this
> > > > without introducing a mul_u64_u64_div_roundup helper. Maybe I miss
> > > > something?
> > >
> > > It does overflow, good catch!
> > > I think we could split this in three operations for now, and maybe
> > > improve it later on:
> > > period = NSEC_PER_SEC * (cyc + 1);
> > > period = DIV64_U64_ROUND_UP(period, rzv2m_pwm->rate);
> > > period <<= rzv2m_pwm_prescale_to_shift(prescale);
> > 
> > You're loosing precision here though. With /^ = div_round_up:
> > 
> > 	1000000000 * 5 /^ 3 << 2 == 6666666668
> > 	1000000000 * 5 << 2 /^ 3 == 6666666667
> 
> Yep.
> 
> > 
> > So I correct my statement to: I see no easy and exact way to prevent
> > an
> > overflow without introducing a mul_u64_u64_div_roundup helper. :-)
> 
> Indeed. In my mind, the best way to tackle this problem is for such an
> API to use an internal representation of 128-bit, so that it can calculate
> (a*b + c - 1)/c accurately, and return both the high and low part of the
> 128-bit result (and maybe keeping the high part result optional?).
> Something like
> u64 mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c, u64 *res_high);
> or
> void mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c, u64 *res_high, u64 *res_low);
> 
> Needless to say, the API may take quite some time to perform the calculations,
> but if precision/accuracy is what we need, there is probably no better
> way of addressing this, unless you have a better idea?
> 
> Looking around, my understanding is that other drivers may benefit from
> this sort of accurate rounded up result when a, b, and c are 64-bit, is that
> why you think it has come the time to address this, or do you think this
> is the first instance of such need?

The assumption of mul_u64_u64_div_u64 is that the result won't overflow
a u64. Iff your hardware doesn't support a period > U64_MAX I think
there is no need for an overflow-checking variant of
mul_u64_u64_div_u64(). I didn't recheck, but this would be the first
driver I'm aware of that can support a period > U64_MAX ns. But maybe it
depends on the clkrate and if that's small enough it can happen?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]