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

[PATCH v3 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.

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

-- 
2.25.1

[PATCH v3 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>
---
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 v3 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>
---
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 | 398 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 410 insertions(+)
 create mode 100644 drivers/pwm/pwm-rzv2m.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index dae023d783a2..31cdc9dae3c5 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -473,6 +473,17 @@ config PWM_RENESAS_TPU
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-renesas-tpu.
 
+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_ROCKCHIP
 	tristate "Rockchip PWM support"
 	depends on ARCH_ROCKCHIP || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7bf1a29f02b8..a95aabae9115 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
 obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
+obj-$(CONFIG_PWM_RZV2M)		+= pwm-rzv2m.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
diff --git a/drivers/pwm/pwm-rzv2m.c b/drivers/pwm/pwm-rzv2m.c
new file mode 100644
index 000000000000..80fb3523026d
--- /dev/null
+++ b/drivers/pwm/pwm-rzv2m.c
@@ -0,0 +1,398 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2M PWM Timer (PWM) driver
+ *
+ * Copyright (C) 2022 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
+ *
+ */
+
+#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 U24_MASK	GENMASK(23, 0)
+#define U24_MAX		(U24_MASK)
+
+#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 F2CYCLE_NSEC(f)		(1000000000 / (f))
+
+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;
+};
+
+static const int rzv2m_pwm_freq_div[] = { 1, 16, 256, 2048 };
+
+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 *chip)
+{
+	/* delay timer when change the setting register */
+	ndelay(chip->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 bool rzv2m_pwm_is_ch_enabled(struct rzv2m_pwm_chip *rzv2m_pwm)
+{
+	return !!(rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR) & RZV2M_PWMCTR_PWME);
+}
+
+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);
+
+	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);
+}
+
+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;
+	u64 pc, dc;
+	int err;
+
+	/*
+	 * 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 / rzv2m_pwm_freq_div[prescale];
+	if (pc / rzv2m_pwm_freq_div[prescale] <= U24_MAX)
+		pwm_cyc = pwm_cyc ? (pwm_cyc - 1) : 0;
+	else
+		pwm_cyc = U24_MAX;
+
+	pwm_low = dc / rzv2m_pwm_freq_div[prescale];
+	if (pwm_low <= U24_MAX)
+		pwm_low = pwm_low ? (pwm_low - 1) : 0;
+	else
+		pwm_low = U24_MAX;
+
+	/*
+	 * If the PWM channel is disabled, make sure to turn on the clock
+	 * before writing the register.
+	 */
+	if (!pwm_is_enabled(pwm)) {
+		err = pm_runtime_resume_and_get(rzv2m_pwm->chip.dev);
+		if (err)
+			return err;
+	}
+
+	rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMTM, 0);
+	rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMPS,
+			 FIELD_PREP(RZV2M_PWMCTR_PWMPS, prescale));
+
+	rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMCYC, pwm_cyc);
+	rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMLOW, pwm_low);
+
+	if (state->polarity == PWM_POLARITY_NORMAL)
+		rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMHL, 0);
+	else
+		rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMHL,
+				 RZV2M_PWMCTR_PWMHL);
+
+	rzv2m_pwm_wait_delay(rzv2m_pwm);
+
+	/*
+	 * If the PWM is not enabled, turn the clock off again to save power.
+	 */
+	if (!pwm_is_enabled(pwm))
+		pm_runtime_put(rzv2m_pwm->chip.dev);
+
+	return 0;
+}
+
+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);
+	u8 prescale;
+	u64 tmp;
+	u32 val;
+
+	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);
+	prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, val);
+	val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
+	val = val ? val + 1 : 0;
+	tmp = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val, rzv2m_pwm->rate);
+	state->period = tmp * rzv2m_pwm_freq_div[prescale];
+
+	val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
+	val = val ? val + 1 : 0;
+	tmp = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val, rzv2m_pwm->rate);
+	state->duty_cycle = tmp * rzv2m_pwm_freq_div[prescale];
+	pm_runtime_put(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);
+
+	clk_prepare_enable(rzv2m_pwm->apb_clk);
+	clk_prepare_enable(rzv2m_pwm->pwm_clk);
+
+	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.
+	 */
+	clk_prepare_enable(rzv2m_pwm->apb_clk);
+	clk_prepare_enable(rzv2m_pwm->pwm_clk);
+
+	if (rzv2m_pwm_is_ch_enabled(rzv2m_pwm))
+		pm_runtime_put(rzv2m_pwm->chip.dev);
+
+	clk_disable_unprepare(rzv2m_pwm->pwm_clk);
+	clk_disable_unprepare(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");
+
+	apb_clk_rate = clk_get_rate(rzv2m_pwm->apb_clk);
+	if (!apb_clk_rate)
+		return dev_err_probe(&pdev->dev, -EINVAL, "apb clk rate is 0");
+
+	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->rate = clk_get_rate(rzv2m_pwm->pwm_clk);
+	if (!rzv2m_pwm->rate)
+		return dev_err_probe(&pdev->dev, -EINVAL, "pwm clk rate is 0");
+
+	/* delay = 6 * PCLK + 9 * PWM_CLK */
+	rzv2m_pwm->delay = F2CYCLE_NSEC(apb_clk_rate) * 6 +
+		F2CYCLE_NSEC(rzv2m_pwm->rate) * 9;
+
+	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);
+	clk_prepare_enable(rzv2m_pwm->apb_clk);
+	clk_prepare_enable(rzv2m_pwm->pwm_clk);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	ret = reset_control_deassert(rzv2m_pwm->rstc);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret,
+			      "cannot deassert reset control\n");
+		goto clk_disable;
+	}
+
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rzv2m_pwm_reset_assert_pm_disable,
+				       rzv2m_pwm);
+	if (ret < 0)
+		goto clk_disable;
+
+	/*
+	 *  We need to keep the clock on, in case the bootloader has enabled the
+	 *  PWM and is running during probe().
+	 */
+	if (rzv2m_pwm_is_ch_enabled(rzv2m_pwm))
+		pm_runtime_get_sync(&pdev->dev);
+
+	rzv2m_pwm->chip.dev = &pdev->dev;
+	rzv2m_pwm->chip.ops = &rzv2m_pwm_ops;
+	rzv2m_pwm->chip.npwm = 1;
+	ret = devm_pwmchip_add(&pdev->dev, &rzv2m_pwm->chip);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+		goto clk_disable;
+	}
+
+	return 0;
+
+clk_disable:
+	clk_disable_unprepare(rzv2m_pwm->pwm_clk);
+	clk_disable_unprepare(rzv2m_pwm->apb_clk);
+	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 v3 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>
---
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 0373ec409d54..dcd3a05e54fe 100644
--- a/arch/arm64/boot/dts/renesas/r9a09g011.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a09g011.dtsi
@@ -135,6 +135,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 v3 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>
---
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 11e1d51c7c0e..73d7481b468e 100644
--- a/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts
+++ b/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts
@@ -78,6 +78,76 @@ i2c2_pins: i2c2 {
 		pinmux = <RZV2M_PORT_PINMUX(3, 8, 2)>, /* SDA */
 			 <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 */
+	};
+};
+
+&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";
 };
 
 &uart0 {
-- 
2.25.1

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

[Biju Das]

Hi Thierry, Uwe, Geert and PWM folks,

Gentle ping for review. 

Cheers,
Biju

> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: 13 December 2022 18:58
> To: Thierry Reding <thierry.reding@gmail.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Michael Turquette
> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Philipp Zabel
> <p.zabel@pengutronix.de>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>; Uwe Kleine-König <u.kleine-
> koenig@pengutronix.de>; Geert Uytterhoeven <geert+renesas@glider.be>; Magnus
> Damm <magnus.damm@gmail.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> clk@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: [PATCH v3 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.
> 
> 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                       | 398 ++++++++++++++++++
>  6 files changed, 668 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/renesas,rzv2m-
> pwm.yaml
>  create mode 100644 drivers/pwm/pwm-rzv2m.c
> 
> --
> 2.25.1

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

[Uwe Kleine-König]

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

Hello,

now I come around to review your driver. Sorry that it took so long, I
was busy with other stuff.

On Tue, Dec 13, 2022 at 06:58:25PM +0000, 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>
> ---
> 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 | 398 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 410 insertions(+)
>  create mode 100644 drivers/pwm/pwm-rzv2m.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index dae023d783a2..31cdc9dae3c5 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -473,6 +473,17 @@ config PWM_RENESAS_TPU
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-renesas-tpu.
>  
> +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_ROCKCHIP
>  	tristate "Rockchip PWM support"
>  	depends on ARCH_ROCKCHIP || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 7bf1a29f02b8..a95aabae9115 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
>  obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
>  obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
>  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
> +obj-$(CONFIG_PWM_RZV2M)		+= pwm-rzv2m.o
>  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> diff --git a/drivers/pwm/pwm-rzv2m.c b/drivers/pwm/pwm-rzv2m.c
> new file mode 100644
> index 000000000000..80fb3523026d
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzv2m.c
> @@ -0,0 +1,398 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2M PWM Timer (PWM) driver
> + *
> + * Copyright (C) 2022 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
> + *
Please document the hardware properties here in a "Limitations"
paragraph similar to e.g. pwm-sl28cpld.c. The idea is to get the
information about how the hardware behaves on .apply (are there
glitches? Can a mixed period happen that has the previous period but the
new duty_cycle? Or maybe duty_cycle and period are shadowed until the
currently running period ends, but polarity takes effect immediately?
etc. pp) when doing

	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/pwm-rzv2m.c

> + */
> +
> +#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 U24_MASK	GENMASK(23, 0)
> +#define U24_MAX		(U24_MASK)
> +
> +#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 F2CYCLE_NSEC(f)		(1000000000 / (f))

Driver prefix please.

> +
> +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;
> +};
> +
> +static const int rzv2m_pwm_freq_div[] = { 1, 16, 256, 2048 };

These are powers of 16, so you can use

	1 << (4 * i)

instead of rzv2m_pwm_freq_div[i]. This might make it easier for the
compiler to optimize.

> [...]
> +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;
> +	u64 pc, dc;
> +	int err;
> +
> +	/*
> +	 * 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 / rzv2m_pwm_freq_div[prescale];

That's a 64 bit division, that's not allowed. (I guess the compiler
optimizes that, otherwise I would have expected a build bot to point out
that problem.)

> +	if (pc / rzv2m_pwm_freq_div[prescale] <= U24_MAX)
> +		pwm_cyc = pwm_cyc ? (pwm_cyc - 1) : 0;
> +	else
> +		pwm_cyc = U24_MAX;

I'd do that part as follows:

#define RZV2M_PWMCYC_PERIOD GENMASK(23, 0)

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);

(Ideally the hardware manual specifies a name for the bitfield. Then of
course pick that one instead of PERIOD.)

> +	pwm_low = dc / rzv2m_pwm_freq_div[prescale];
> +	if (pwm_low <= U24_MAX)
> +		pwm_low = pwm_low ? (pwm_low - 1) : 0;
> +	else
> +		pwm_low = U24_MAX;
> +
> +	/*
> +	 * If the PWM channel is disabled, make sure to turn on the clock
> +	 * before writing the register.
> +	 */
> +	if (!pwm_is_enabled(pwm)) {
> +		err = pm_runtime_resume_and_get(rzv2m_pwm->chip.dev);
> +		if (err)
> +			return err;
> +	}
> +
> +	rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMTM, 0);
> +	rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMPS,
> +			 FIELD_PREP(RZV2M_PWMCTR_PWMPS, prescale));

Is there a need to do this in two register writes? If yes, please add
this information in a comment, if not, please do it in one access.

> +	rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMCYC, pwm_cyc);
> +	rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMLOW, pwm_low);
> +
> +	if (state->polarity == PWM_POLARITY_NORMAL)
> +		rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMHL, 0);
> +	else
> +		rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMHL,
> +				 RZV2M_PWMCTR_PWMHL);

This can be merged into the above write to RZV2M_PWMCTR, too?!

> +	rzv2m_pwm_wait_delay(rzv2m_pwm);
> +
> +	/*
> +	 * If the PWM is not enabled, turn the clock off again to save power.
> +	 */
> +	if (!pwm_is_enabled(pwm))
> +		pm_runtime_put(rzv2m_pwm->chip.dev);

This is wrong for two reasons: First, you should not use pwm consumer
functions in a low level driver and second, this gives information about
the old state, I think you want:

	if (!state->enabled)
		pm_runtime_put(rzv2m_pwm->chip.dev);

Thinking again: rzv2m_pwm_config() is only ever called when
state->enabled is true, to this can be further simplified.

(And for the check above you want:

	if (!pwm->state.enabled) {
		err = pm_runtime_resume_and_get(...)
		...
	}
)

> +	return 0;
> +}
> +
> +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);
> +	u8 prescale;
> +	u64 tmp;
> +	u32 val;
> +
> +	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);

Please don't use a bit value as enum pwm_polarity.

Something like

	state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, val) ?  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;

should be identical for the compiler, but doesn't implicitly hardcode
the value of the PWM_POLARITY_* constants.

> +	prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, val);
> +	val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> +	val = val ? val + 1 : 0;

oh, how unusual. Are you sure here?

> +	tmp = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val, rzv2m_pwm->rate);
> +	state->period = tmp * rzv2m_pwm_freq_div[prescale];

rate is usually 48 MHz, right? Assuming val = 1 and prescale = 3 the
actual period value is

	1000000000 * 1 * 2048 / (48 MHz) ≅ 42666.6667

however your calculation yields 43008 in that case. Can you please try
to make this 42667 instead? The needed calculation is:

	divroundup(NSEC_PER_SEC * (u64)val * rzv2m_pwm_freq_div[prescale], rzv2m_pwm->rate);

To prevent an overflow, we might need an uprounding variant of mul_u64_u64_div_u64().

(Mental note for myself: Update PWM_DEBUG to catch that problem.)

> +	val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> +	val = val ? val + 1 : 0;
> +	tmp = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val, rzv2m_pwm->rate);
> +	state->duty_cycle = tmp * rzv2m_pwm_freq_div[prescale];
> +	pm_runtime_put(chip->dev);
> +
> +	return 0;
> +}

I'd reorder the functions to have rzv2m_pwm_config() directly before
rzv2m_pwm_apply().

> +static int rzv2m_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   const struct pwm_state *state)
> [...]
> +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");
> +
> +	apb_clk_rate = clk_get_rate(rzv2m_pwm->apb_clk);
> +	if (!apb_clk_rate)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "apb clk rate is 0");
> +
> +	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->rate = clk_get_rate(rzv2m_pwm->pwm_clk);
> +	if (!rzv2m_pwm->rate)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "pwm clk rate is 0");
> +
> +	/* delay = 6 * PCLK + 9 * PWM_CLK */
> +	rzv2m_pwm->delay = F2CYCLE_NSEC(apb_clk_rate) * 6 +
> +		F2CYCLE_NSEC(rzv2m_pwm->rate) * 9;

F2CYCLE_NSEC contains a division, so you're loosing precision here. Also
as this is a minimal delay, you might want to use up-rounding division.

> +	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);
> +	clk_prepare_enable(rzv2m_pwm->apb_clk);
> +	clk_prepare_enable(rzv2m_pwm->pwm_clk);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	ret = reset_control_deassert(rzv2m_pwm->rstc);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret,
> +			      "cannot deassert reset control\n");
> +		goto clk_disable;
> +	}
> +
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       rzv2m_pwm_reset_assert_pm_disable,
> +				       rzv2m_pwm);
> +	if (ret < 0)
> +		goto clk_disable;
> +
> +	/*
> +	 *  We need to keep the clock on, in case the bootloader has enabled the
> +	 *  PWM and is running during probe().
> +	 */
> +	if (rzv2m_pwm_is_ch_enabled(rzv2m_pwm))
> +		pm_runtime_get_sync(&pdev->dev);
> +
> +	rzv2m_pwm->chip.dev = &pdev->dev;
> +	rzv2m_pwm->chip.ops = &rzv2m_pwm_ops;
> +	rzv2m_pwm->chip.npwm = 1;
> +	ret = devm_pwmchip_add(&pdev->dev, &rzv2m_pwm->chip);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +		goto clk_disable;

If you fail here, rzv2m_pwm_reset_assert_pm_disable is called which
calls pm_runtime_set_suspended(). I assume that results in
rzv2m_pwm_pm_runtime_suspend() being called and to the clks are already
disabled?

> +	}
> +
> +	return 0;
> +
> +clk_disable:
> +	clk_disable_unprepare(rzv2m_pwm->pwm_clk);
> +	clk_disable_unprepare(rzv2m_pwm->apb_clk);
> +	return ret;
> +}
> [...]

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 v3 2/4] pwm: Add support for RZ/V2M PWM driver

[Uwe Kleine-König]

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

Hello,

> + * https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardware?language=en

I took a look into that now, and there are a few things I noticed.

The PWMCYC register description has:

	To change the setting value of the PWM cycle setting register
	(PWMm_PWMCYC), set the PWME bit of the PWM control register
	(PWMm_PWMCTR) to 0b and stop the counter operation. If it is
	changed during counter operation, PWM output may not be
	performed correctly.

This isn't repected in the driver. Please either fix that or add a
comment why you think this is not necessary. If you choose to adhere to
that, also note it in the Limitations section that I asked you to add.

In .apply() you subtract 1 from the calculated value of PWMCYC. When
looking through section 17.4 Function Details I don't see this
justified. However in 17.3.2.2 the formula is as you quoted in the
driver (i.e. PWMm_PWMCYC = (PWM period (ns) / (PWM_CLK period (ns) ×
Division ratio)) − 1). Can you maybe test which of the two is correct,
maybe adapt the driver code and note in a comment about the difference?

Also comment would be nice about the fact that the native polarity of
the hardware is inverted (i.e. it starts with the low part). I didn't
recheck, maybe the inversion bit handling must be switched?

A 100% duty cycle is only possible (according to Figure 17.4-2) with
PWMLOW > PWMCYC. Assuming this is correct, there is the problem that the
two registers have the same width, so if PWMCYC is 0xffffff a 100% duty
isn't possible. So please stick to only using values < 0xffffff for
PWMCYC.

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 v3 2/4] pwm: Add support for RZ/V2M PWM driver

[Biju Das]

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/4] pwm: Add support for RZ/V2M PWM driver
> 
> Hello,
> 
> now I come around to review your driver. Sorry that it took so long, I was
> busy with other stuff.
> 
> On Tue, Dec 13, 2022 at 06:58:25PM +0000, 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>
> > ---
> > 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 | 398
> > ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 410 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-rzv2m.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > dae023d783a2..31cdc9dae3c5 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -473,6 +473,17 @@ config PWM_RENESAS_TPU
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-renesas-tpu.
> >
> > +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_ROCKCHIP
> >  	tristate "Rockchip PWM support"
> >  	depends on ARCH_ROCKCHIP || COMPILE_TEST diff --git
> > a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 7bf1a29f02b8..a95aabae9115 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> >  obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
> >  obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
> >  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
> > +obj-$(CONFIG_PWM_RZV2M)		+= pwm-rzv2m.o
> >  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> >  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> > diff --git a/drivers/pwm/pwm-rzv2m.c b/drivers/pwm/pwm-rzv2m.c new
> > file mode 100644 index 000000000000..80fb3523026d
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzv2m.c
> > @@ -0,0 +1,398 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/V2M PWM Timer (PWM) driver
> > + *
> > + * Copyright (C) 2022 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
> > + *
> Please document the hardware properties here in a "Limitations"
> paragraph similar to e.g. pwm-sl28cpld.c. The idea is to get the information
> about how the hardware behaves on .apply (are there glitches? Can a mixed
> period happen that has the previous period but the new duty_cycle? Or maybe
> duty_cycle and period are shadowed until the currently running period ends,
> but polarity takes effect immediately?
> etc. pp) when doing
> 
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/pwm-rzv2m.c

I have added below limitations now

* Limitations:
 * - If the PWMLOW value is changed during PWM operation, the changed
 *   value is reflected in the next PWM cycle.
 * - 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.
 * - To change the setting value of the PWM cycle setting register
 *   (PWMm_PWMCYC), set the PWME bit of the PWM control register (PWMm_PWMCTR)
 *   to 0b and stop the counter operation. If it is changed during counter
 *   operation, PWM output may not be performed correctly.
 * - 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.

> 
> > + */
> > +
> > +#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 U24_MASK	GENMASK(23, 0)
> > +#define U24_MAX		(U24_MASK)
> > +
> > +#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 F2CYCLE_NSEC(f)		(1000000000 / (f))
> 
> Driver prefix please.

I am dropping this macro. Please see below.

> 
> > +
> > +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;
> > +};
> > +
> > +static const int rzv2m_pwm_freq_div[] = { 1, 16, 256, 2048 };
> 
> These are powers of 16, so you can use
> 
> 	1 << (4 * i)
> 
> instead of rzv2m_pwm_freq_div[i]. This might make it easier for the compiler
> to optimize

OK. Agreed.

> 
> > [...]
> > +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;
> > +	u64 pc, dc;
> > +	int err;
> > +
> > +	/*
> > +	 * 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 / rzv2m_pwm_freq_div[prescale];
> 
> That's a 64 bit division, that's not allowed. (I guess the compiler
> optimizes that, otherwise I would have expected a build bot to point out
> that problem.)
> 
> > +	if (pc / rzv2m_pwm_freq_div[prescale] <= U24_MAX)
> > +		pwm_cyc = pwm_cyc ? (pwm_cyc - 1) : 0;
> > +	else
> > +		pwm_cyc = U24_MAX;
> 
> I'd do that part as follows:
> 
> #define RZV2M_PWMCYC_PERIOD GENMASK(23, 0)
> 
> 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);
> 
> (Ideally the hardware manual specifies a name for the bitfield. Then of
> course pick that one instead of PERIOD.)

OK agreed.

> 
> > +	pwm_low = dc / rzv2m_pwm_freq_div[prescale];
> > +	if (pwm_low <= U24_MAX)
> > +		pwm_low = pwm_low ? (pwm_low - 1) : 0;
> > +	else
> > +		pwm_low = U24_MAX;
> > +
> > +	/*
> > +	 * If the PWM channel is disabled, make sure to turn on the clock
> > +	 * before writing the register.
> > +	 */
> > +	if (!pwm_is_enabled(pwm)) {
> > +		err = pm_runtime_resume_and_get(rzv2m_pwm->chip.dev);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMTM, 0);
> > +	rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMPS,
> > +			 FIELD_PREP(RZV2M_PWMCTR_PWMPS, prescale));
> 
> Is there a need to do this in two register writes? If yes, please add this
> information in a comment, if not, please do it in one access.
> 
> > +	rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMCYC, pwm_cyc);
> > +	rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMLOW, pwm_low);
> > +
> > +	if (state->polarity == PWM_POLARITY_NORMAL)
> > +		rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMHL,
> 0);
> > +	else
> > +		rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMHL,
> > +				 RZV2M_PWMCTR_PWMHL);
> 
> This can be merged into the above write to RZV2M_PWMCTR, too?!

OK will merge.
> 
> > +	rzv2m_pwm_wait_delay(rzv2m_pwm);
> > +
> > +	/*
> > +	 * If the PWM is not enabled, turn the clock off again to save power.
> > +	 */
> > +	if (!pwm_is_enabled(pwm))
> > +		pm_runtime_put(rzv2m_pwm->chip.dev);
> 
> This is wrong for two reasons: First, you should not use pwm consumer
> functions in a low level driver and second, this gives information about the
> old state, I think you want:
> 
> 	if (!state->enabled)
> 		pm_runtime_put(rzv2m_pwm->chip.dev);
> 
> Thinking again: rzv2m_pwm_config() is only ever called when
> state->enabled is true, to this can be further simplified.
> 
> (And for the check above you want:
> 
> 	if (!pwm->state.enabled) {
> 		err = pm_runtime_resume_and_get(...)
> 		...
> 	}
> )
OK, Agreed.


> 
> > +	return 0;
> > +}
> > +
> > +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);
> > +	u8 prescale;
> > +	u64 tmp;
> > +	u32 val;
> > +
> > +	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);
> 
> Please don't use a bit value as enum pwm_polarity.
> 
> Something like
> 
> 	state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, val) ?
> PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> 
> should be identical for the compiler, but doesn't implicitly hardcode the
> value of the PWM_POLARITY_* constants.

OK, Agreed.

> 
> > +	prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, val);
> > +	val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> > +	val = val ? val + 1 : 0;
> 
> oh, how unusual. Are you sure here?

OK I will add if (val)  val +=1; see config we r doing - 1

> 
> > +	tmp = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val, rzv2m_pwm->rate);
> > +	state->period = tmp * rzv2m_pwm_freq_div[prescale];
> 
> rate is usually 48 MHz, right? Assuming val = 1 and prescale = 3 the actual
> period value is
> 
> 	1000000000 * 1 * 2048 / (48 MHz) ≅ 42666.6667

> 
> however your calculation yields 43008 in that case. Can you please try to
> make this 42667 instead? The needed calculation is:

Looks the above equation is giving correct results.

I have added below print and it is giving 42667.
pr_err("########%s %llu",__func__,DIV_ROUND_UP_ULL(NSEC_PER_SEC * 2048, 48 *1000000));
[   44.177920] ########rzv2m_pwm_get_state 42667

Moreover, There is no warnings at all with PWM_DEBUG enabled.

> 
> 	divroundup(NSEC_PER_SEC * (u64)val * rzv2m_pwm_freq_div[prescale],
> rzv2m_pwm->rate);
> 
> To prevent an overflow, we might need an uprounding variant of
> mul_u64_u64_div_u64().
> 
> (Mental note for myself: Update PWM_DEBUG to catch that problem.)
> 
> > +	val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> > +	val = val ? val + 1 : 0;
> > +	tmp = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val, rzv2m_pwm->rate);
> > +	state->duty_cycle = tmp * rzv2m_pwm_freq_div[prescale];
> > +	pm_runtime_put(chip->dev);
> > +
> > +	return 0;
> > +}
> 
> I'd reorder the functions to have rzv2m_pwm_config() directly before
> rzv2m_pwm_apply().

OK, Agreed.

> 
> > +static int rzv2m_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			   const struct pwm_state *state)
> > [...]
> > +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");
> > +
> > +	apb_clk_rate = clk_get_rate(rzv2m_pwm->apb_clk);
> > +	if (!apb_clk_rate)
> > +		return dev_err_probe(&pdev->dev, -EINVAL, "apb clk rate is 0");
> > +
> > +	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->rate = clk_get_rate(rzv2m_pwm->pwm_clk);
> > +	if (!rzv2m_pwm->rate)
> > +		return dev_err_probe(&pdev->dev, -EINVAL, "pwm clk rate is 0");
> > +
> > +	/* delay = 6 * PCLK + 9 * PWM_CLK */
> > +	rzv2m_pwm->delay = F2CYCLE_NSEC(apb_clk_rate) * 6 +
> > +		F2CYCLE_NSEC(rzv2m_pwm->rate) * 9;
> 
> F2CYCLE_NSEC contains a division, so you're loosing precision here. Also as
> this is a minimal delay, you might want to use up-rounding division.

I will use below eqn.

	rzv2m_pwm->delay = DIV_ROUND_UP(6 * 1000000000UL, apb_clk_rate) +
		DIV_ROUND_UP(9 * 1000000000UL, rzv2m_pwm->rate);


> 
> > +	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);
> > +	clk_prepare_enable(rzv2m_pwm->apb_clk);
> > +	clk_prepare_enable(rzv2m_pwm->pwm_clk);
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +	ret = reset_control_deassert(rzv2m_pwm->rstc);
> > +	if (ret) {
> > +		dev_err_probe(&pdev->dev, ret,
> > +			      "cannot deassert reset control\n");
> > +		goto clk_disable;
> > +	}
> > +
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rzv2m_pwm_reset_assert_pm_disable,
> > +				       rzv2m_pwm);
> > +	if (ret < 0)
> > +		goto clk_disable;
> > +
> > +	/*
> > +	 *  We need to keep the clock on, in case the bootloader has enabled
> the
> > +	 *  PWM and is running during probe().
> > +	 */
> > +	if (rzv2m_pwm_is_ch_enabled(rzv2m_pwm))
> > +		pm_runtime_get_sync(&pdev->dev);
> > +
> > +	rzv2m_pwm->chip.dev = &pdev->dev;
> > +	rzv2m_pwm->chip.ops = &rzv2m_pwm_ops;
> > +	rzv2m_pwm->chip.npwm = 1;
> > +	ret = devm_pwmchip_add(&pdev->dev, &rzv2m_pwm->chip);
> > +	if (ret) {
> > +		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +		goto clk_disable;
> 
> If you fail here, rzv2m_pwm_reset_assert_pm_disable is called which calls
> pm_runtime_set_suspended(). I assume that results in
> rzv2m_pwm_pm_runtime_suspend() being called and to the clks are already
> disabled?

This part will be modified as

	ret = reset_control_deassert(rzv2m_pwm->rstc);
	if (ret) {
		dev_err_probe(&pdev->dev, ret,
			      "cannot deassert reset control\n");
		goto clk_disable;
	}

	/*
	 *  We need to keep the clock on, in case the bootloader has enabled the
	 *  PWM and is running during probe().
	 */
	if (rzv2m_pwm_is_ch_enabled(rzv2m_pwm))
		pm_runtime_get_sync(&pdev->dev);

	ret = devm_add_action_or_reset(&pdev->dev,
				       rzv2m_pwm_reset_assert_pm_disable,
				       rzv2m_pwm);
	if (ret < 0)
		return ret;

	rzv2m_pwm->chip.dev = &pdev->dev;
	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");

	return 0;

clk_disable:
	pm_runtime_disable(&pdev->dev);
	pm_runtime_set_suspended(&pdev->dev);
	clk_disable_unprepare(rzv2m_pwm->pwm_clk);
	clk_disable_unprepare(rzv2m_pwm->apb_clk);
	return ret;

Cheers,
Biju

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

[Biju Das]

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/4] pwm: Add support for RZ/V2M PWM driver
> 
> Hello,
> 
> > + *
> > + https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardwa
> > + re?language=en
> 
> I took a look into that now, and there are a few things I noticed.
> 
> The PWMCYC register description has:
> 
> 	To change the setting value of the PWM cycle setting register
> 	(PWMm_PWMCYC), set the PWME bit of the PWM control register
> 	(PWMm_PWMCTR) to 0b and stop the counter operation. If it is
> 	changed during counter operation, PWM output may not be
> 	performed correctly.

OK, I will fix it in the driver.

> 
> This isn't repected in the driver. Please either fix that or add a comment
> why you think this is not necessary. If you choose to adhere to that, also
> note it in the Limitations section that I asked you to add.
> 
> In .apply() you subtract 1 from the calculated value of PWMCYC. When looking
> through section 17.4 Function Details I don't see this justified. However in
> 17.3.2.2 the formula is as you quoted in the driver (i.e. PWMm_PWMCYC = (PWM
> period (ns) / (PWM_CLK period (ns) × Division ratio)) − 1). Can you maybe
> test which of the two is correct, maybe adapt the driver code and note in a
> comment about the difference?

I got confirmation from HW team that below formula is correct.

PWMm_PWMCYC = (PWM period (ns) / (PWM_CLK period (ns) × Division ratio)) − 1)


> 
> Also comment would be nice about the fact that the native polarity of the
> hardware is inverted (i.e. it starts with the low part). I didn't recheck,
> maybe the inversion bit handling must be switched?
> 
> A 100% duty cycle is only possible (according to Figure 17.4-2) with PWMLOW
> > PWMCYC. Assuming this is correct, there is the problem that the two
> registers have the same width, so if PWMCYC is 0xffffff a 100% duty isn't
> possible. So please stick to only using values < 0xffffff for PWMCYC.

Will add below code to take care this.

	/*
	 * A 100% duty cycle is only possible with PWMLOW > PWMCYC. if PWMCYC is 0xffffff
	 * a 100% duty cycle is not possible.
	 */
	if (pwm_cyc == pwm_low && (pwm_cyc == FIELD_MAX(RZV2M_PWMCYC_PERIOD)))
		pwm_cyc -= 1;

Cheers,
Biju

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

[Biju Das]

Hi Uwe,

> Subject: RE: [PATCH v3 2/4] pwm: Add support for RZ/V2M PWM driver
> 
> Hi Uwe,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v3 2/4] pwm: Add support for RZ/V2M PWM driver
> >
> > Hello,
> >
> > now I come around to review your driver. Sorry that it took so long, I
> > was busy with other stuff.
> >
> > On Tue, Dec 13, 2022 at 06:58:25PM +0000, 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>
> > > ---
> > > 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_share
> > > d
> > > ---
> > >  drivers/pwm/Kconfig     |  11 ++
> > >  drivers/pwm/Makefile    |   1 +
> > >  drivers/pwm/pwm-rzv2m.c | 398
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 410 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-rzv2m.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > > dae023d783a2..31cdc9dae3c5 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -473,6 +473,17 @@ config PWM_RENESAS_TPU
> > >  	  To compile this driver as a module, choose M here: the module
> > >  	  will be called pwm-renesas-tpu.
> > >
> > > +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_ROCKCHIP
> > >  	tristate "Rockchip PWM support"
> > >  	depends on ARCH_ROCKCHIP || COMPILE_TEST diff --git
> > > a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > > 7bf1a29f02b8..a95aabae9115 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> > >  obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
> > >  obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
> > >  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
> > > +obj-$(CONFIG_PWM_RZV2M)		+= pwm-rzv2m.o
> > >  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> > >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> > >  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> > > diff --git a/drivers/pwm/pwm-rzv2m.c b/drivers/pwm/pwm-rzv2m.c new
> > > file mode 100644 index 000000000000..80fb3523026d
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-rzv2m.c
> > > @@ -0,0 +1,398 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Renesas RZ/V2M PWM Timer (PWM) driver
> > > + *
> > > + * Copyright (C) 2022 Renesas Electronics Corporation
> > > + *
> > > + * Hardware manual for this IP can be found here
> > > + *
> > > +https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardw
> > > +ar
> > > +e?language=en
> > > + *
> > Please document the hardware properties here in a "Limitations"
> > paragraph similar to e.g. pwm-sl28cpld.c. The idea is to get the
> > information about how the hardware behaves on .apply (are there
> > glitches? Can a mixed period happen that has the previous period but
> > the new duty_cycle? Or maybe duty_cycle and period are shadowed until
> > the currently running period ends, but polarity takes effect immediately?
> > etc. pp) when doing
> >
> > 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/pwm-rzv2m.c
> 
> I have added below limitations now
> 
> * Limitations:
>  * - If the PWMLOW value is changed during PWM operation, the changed
>  *   value is reflected in the next PWM cycle.
>  * - 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.
>  * - To change the setting value of the PWM cycle setting register
>  *   (PWMm_PWMCYC), set the PWME bit of the PWM control register
> (PWMm_PWMCTR)
>  *   to 0b and stop the counter operation. If it is changed during counter
>  *   operation, PWM output may not be performed correctly.
>  * - 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->chip.dev = &pdev->dev;
> > > +	rzv2m_pwm->chip.ops = &rzv2m_pwm_ops;
> > > +	rzv2m_pwm->chip.npwm = 1;
> > > +	ret = devm_pwmchip_add(&pdev->dev, &rzv2m_pwm->chip);
> > > +	if (ret) {
> > > +		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > > +		goto clk_disable;
> >
> > If you fail here, rzv2m_pwm_reset_assert_pm_disable is called which
> > calls pm_runtime_set_suspended(). I assume that results in
> > rzv2m_pwm_pm_runtime_suspend() being called and to the clks are
> > already disabled?

I have restructured the probe() as below and tested various scenarios, It looks ok to me.
Please let me know, if you have any suggestions.

static int rzv2m_pwm_probe(struct platform_device *pdev)
{
...
...
	pm_runtime_enable(&pdev->dev);
	ret = reset_control_deassert(rzv2m_pwm->rstc);
	if (ret) {
		dev_err_probe(&pdev->dev, ret,
			      "cannot deassert reset control\n");
		goto pm_clk_disable;
	}

	rzv2m_pwm->chip.dev = &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_is_ch_enabled(rzv2m_pwm))
		pm_runtime_get_sync(&pdev->dev);

	ret = devm_add_action_or_reset(&pdev->dev,
				       rzv2m_pwm_reset_assert_pm_disable,
				       rzv2m_pwm);
	ret = -EINVAL;
	if (ret < 0)
		goto channel_disable;

	rzv2m_pwm->chip.ops = &rzv2m_pwm_ops;
	rzv2m_pwm->chip.npwm = 1;
	ret = devm_pwmchip_add(&pdev->dev, &rzv2m_pwm->chip);
	if (ret) {
		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
		goto channel_disable;
	}

	return 0;

pm_clk_disable:
	pm_runtime_disable(&pdev->dev);
	pm_runtime_set_suspended(&pdev->dev);
channel_disable:
	if (rzv2m_pwm_is_ch_enabled(rzv2m_pwm))
		rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME, 0);

	clk_disable_unprepare(rzv2m_pwm->pwm_clk);
	clk_disable_unprepare(rzv2m_pwm->apb_clk);
	return ret;
}

Test cases:

1) PWM-disabled by bootloader on case
----------------------------------
root@rzv2m:~# dmesg | grep pwm8
[    3.739239] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[    3.747804] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:~# /pwm-clk-dump.sh
Read at address  0xA3500434 (0xffffb55b6434): 0x00000801 --> BIT5 PWM8 clk
Read at address  0xA4010400 (0xffff98c3f400): 0x00000000 --> BIT2 PWM8 Channel enable

root@rzv2m:~# cd /sys/bus/platform/drivers/pwm-rzv2m
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# echo a4010400.pwm > unbind
[   84.025155] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[   84.039214] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
[   84.060958] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[   84.074759] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# /pwm-clk-dump.sh
Read at address  0xA3500434 (0xffffbbd5e434): 0x00000801
Read at address  0xA4010400 (0xffff8a796400): 0x00000000
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m#

root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# echo a4010400.pwm > bind
[  139.273250] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[  139.299582] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# /pwm-clk-dump.sh
Read at address  0xA3500434 (0xffffa0f28434): 0x00000801
Read at address  0xA4010400 (0xffffa1b66400): 0x00000000
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m#

2) PWM-enabled by bootloader on case
---------------------------------
root@rzv2m:~# dmesg | grep pwm8
[    3.738742] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
root@rzv2m:~# /pwm-clk-dump.sh
Read at address  0xA3500434 (0xffffa68de434): 0x00000811
Read at address  0xA4010400 (0xffff9eaad400): 0x00000003

root@rzv2m:~# cd /sys/bus/platform/drivers/pwm-rzv2m
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# echo a4010400.pwm > unbind
[  125.579970] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# /pwm-clk-dump.sh
Read at address  0xA3500434 (0xffffbb4db434): 0x00000801
Read at address  0xA4010400 (0xffffa7194400): 0x00000000

root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# echo a4010400.pwm > bind
[  170.837264] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[  170.863046] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# /pwm-clk-dump.sh
Read at address  0xA3500434 (0xffff91fd4434): 0x00000801
Read at address  0xA4010400 (0xffff91010400): 0x00000000
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m#

3) Error case 1:
---------------
        ret = reset_control_deassert(rzv2m_pwm->rstc);
+       ret = -EINVAL;

PWM off/ON during boot:
[    3.709913] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[    3.716953] pwm-rzv2m a4010400.pwm: error -EINVAL: cannot deassert reset control
[    3.725186] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
[    3.732425] pwm-rzv2m: probe of a4010400.pwm failed with error -22

root@rzv2m:~# /pwm-clk-dump.sh
Read at address  0xA3500434 (0xffffa5ddc434): 0x00000801
Read at address  0xA3500614 (0xffffb5800614): 0x00000000

4) Error case 2:
---------------
	ret = devm_add_action_or_reset(&pdev->dev,
				       rzv2m_pwm_reset_assert_pm_disable,
				       rzv2m_pwm);
+	ret = -EINVAL;

PWM on/off during boot:
[    3.714182] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[    3.721798] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
[    3.730662] pwm-rzv2m: probe of a4010400.pwm failed with error -22
[    3.739956] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[    3.755238] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF

root@rzv2m:~# /pwm-clk-dump.sh
Read at address  0xA3500434 (0xffff92dfc434): 0x00000801
Read at address  0xA4010400 (0xffffa7021400): 0x00000000

5) Error case 3:
----------------
	ret = devm_pwmchip_add(&pdev->dev, &rzv2m_pwm->chip);
+	ret = -EINVAL;
	
PWM on/off during boot:
[    3.712732] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[    3.720408] pwm-rzv2m a4010400.pwm: error -EINVAL: failed to add PWM chip
[    3.731991] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
[    3.739237] pwm-rzv2m: probe of a4010400.pwm failed with error -22
[    3.754369] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[    3.761684] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF

root@rzv2m:~# /pwm-clk-dump.sh
Read at address  0xA3500434 (0xffff92dfc434): 0x00000801
Read at address  0xA4010400 (0xffffa7021400): 0x00000000

Cheers,
Biju

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

[Biju Das]

Hi Uwe,

I will send next version incorporating the review comments/discussion based on this patch series.

Cheers,
Biju

> Subject: RE: [PATCH v3 2/4] pwm: Add support for RZ/V2M PWM driver
> 
> Hi Uwe,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v3 2/4] pwm: Add support for RZ/V2M PWM driver
> >
> > Hello,
> >
> > > + *
> > > + https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hard
> > > + wa
> > > + re?language=en
> >
> > I took a look into that now, and there are a few things I noticed.
> >
> > The PWMCYC register description has:
> >
> > 	To change the setting value of the PWM cycle setting register
> > 	(PWMm_PWMCYC), set the PWME bit of the PWM control register
> > 	(PWMm_PWMCTR) to 0b and stop the counter operation. If it is
> > 	changed during counter operation, PWM output may not be
> > 	performed correctly.
> 
> OK, I will fix it in the driver.
> 
> >
> > This isn't repected in the driver. Please either fix that or add a
> > comment why you think this is not necessary. If you choose to adhere
> > to that, also note it in the Limitations section that I asked you to add.
> >
> > In .apply() you subtract 1 from the calculated value of PWMCYC. When
> > looking through section 17.4 Function Details I don't see this
> > justified. However in
> > 17.3.2.2 the formula is as you quoted in the driver (i.e. PWMm_PWMCYC
> > = (PWM period (ns) / (PWM_CLK period (ns) × Division ratio)) − 1). Can
> > you maybe test which of the two is correct, maybe adapt the driver
> > code and note in a comment about the difference?
> 
> I got confirmation from HW team that below formula is correct.
> 
> PWMm_PWMCYC = (PWM period (ns) / (PWM_CLK period (ns) × Division ratio)) −
> 1)
> 
> 
> >
> > Also comment would be nice about the fact that the native polarity of
> > the hardware is inverted (i.e. it starts with the low part). I didn't
> > recheck, maybe the inversion bit handling must be switched?
> >
> > A 100% duty cycle is only possible (according to Figure 17.4-2) with
> > PWMLOW
> > > PWMCYC. Assuming this is correct, there is the problem that the two
> > registers have the same width, so if PWMCYC is 0xffffff a 100% duty
> > isn't possible. So please stick to only using values < 0xffffff for
> PWMCYC.
> 
> Will add below code to take care this.
> 
> 	/*
> 	 * A 100% duty cycle is only possible with PWMLOW > PWMCYC. if PWMCYC
> is 0xffffff
> 	 * a 100% duty cycle is not possible.
> 	 */
> 	if (pwm_cyc == pwm_low && (pwm_cyc == FIELD_MAX(RZV2M_PWMCYC_PERIOD)))
> 		pwm_cyc -= 1;