linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/2] Add support for RZ/G2L GPT
@ 2022-08-05 14:57 Biju Das
  2022-08-05 14:57 ` [PATCH v5 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
  2022-08-05 14:57 ` [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT Biju Das
  0 siblings, 2 replies; 6+ messages in thread
From: Biju Das @ 2022-08-05 14:57 UTC (permalink / raw)
  To: Thierry Reding, Lee Jones, Rob Herring, Krzysztof Kozlowski,
	Philipp Zabel
  Cc: Biju Das, Uwe Kleine-König, linux-pwm, devicetree,
	Geert Uytterhoeven, Chris Paterson, Biju Das,
	Prabhakar Mahadev Lad, linux-renesas-soc

RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit timer
(GPT32E). It supports the following functions
 * 32 bits × 8 channels
 * Up-counting or down-counting (saw waves) or up/down-counting
   (triangle waves) for each counter.
 * Clock sources independently selectable for each channel
 * Two I/O pins per channel
 * Two output compare/input capture registers per channel
 * For the two output compare/input capture registers of each channel,
   four registers are provided as buffer registers and are capable of
   operating as comparison registers when buffering is not in use.
 * In output compare operation, buffer switching can be at crests or
   troughs, enabling the generation of laterally asymmetric PWM waveforms.
 * Registers for setting up frame cycles in each channel (with capability
   for generating interrupts at overflow or underflow)
 * Generation of dead times in PWM operation
 * Synchronous starting, stopping and clearing counters for arbitrary
   channels
 * Starting, stopping, clearing and up/down counters in response to input
   level comparison
 * Starting, clearing, stopping and up/down counters in response to a
   maximum of four external triggers
 * Output pin disable function by dead time error and detected
   short-circuits between output pins
 * A/D converter start triggers can be generated (GPT32E0 to GPT32E3)
 * Enables the noise filter for input capture and external trigger
   operation

This patch series aims to add basic pwm support for RZ/G2L GPT driver
by creating separate logical channels for each IOs.

v4->v5:
 * Added Hardware manual details
 * Replaced the comment GTCNT->Counter
 * Removed the macros RZG2L_GPT_IO_PER_CHANNEL and chip.npwm directly
   used in probe.
 * Removed the unsed macro RZG2L_GTPR_MAX_VALUE
 * Added driver prefix for the type name and the variable.
 * Initialization of per_channel data moved from request->probe.
 * Updated clr parameter for rzg2l_gpt_modify for Start count.
 * Started using mutex and usage_count for handling shared
   period and prescalar for the 2 channels.
 * Updated the comment cycle->period.
 * Removed clk_disable from rzg2l_gpt_reset_assert_pm_disable()
 * Replaced pc->rzg2l_gpt.
 * Updated prescale calculation.
 * Moved pm_runtime_{get_sync,put} from {request,free}->{enable,disable}
 * Removed platform_set_drvdata as it is unused
 * Removed the variable pwm_enabled_by_bootloader 
 * Added dev_err_probe in various probe error path.
 * Added an error message, if devm_pwmchip_add fails.
v3->v4:
 * Changed the local variable type i from u16->u8 and prescaled_period_
   cycles from u64->u32 in calculate_prescale().
 * Replaced mul_u64_u64_div_u64()->mul_u64_u32_div()
 * Dropped the comma after the sentinel.
 * Add a variable to track pwm enabled by bootloader and added comments
   in probe().
 * Removed unnecessary rzg2l_gpt_reset_assert_pm_disable() from probe.
 * Replaced devm_clk_get()->devm_clk_get_prepared()
 * Removed devm_clk_get_optional_enabled()
v2->v3:
 * Added Rb tag from Rob for the bindings.
 * Updated limitation section
 * Added prefix "RZG2L_" for all macros
 * Modified prescale calculation
 * Removed pwm_set_chip_data
 * Updated comment related to modifying Mode and Prescaler
 * Updated setting of prescale value in rzg2l_gpt_config()
 * Removed else branch from rzg2l_gpt_get_state()
 * removed the err label from rzg2l_gpt_apply()
 * Added devm_clk_get_optional_enabled() to retain clk on status,
   in case bootloader turns on the clk of pwm.
 * Replaced devm_reset_control_get_exclusive->devm_reset_control_get_shared
   as single reset shared between 8 channels.
V1->v2:
 * Added '|' after 'description:' to preserve formatting.
 * Removed description for pwm_cells as it is common property.
 * Changed the reg size in example from 0xa4->0x100
 * Added Rb tag from Geert for bindings.
 * Added Limitations section
 * dropped "_MASK" from the define names.
 * used named initializer for struct phase
 * Added gpt_pwm_device into a flexible array member in rzg2l_gpt_chip
 * Revised the logic for prescale
 * Added .get_state callback
 * Improved error handling in rzg2l_gpt_apply
 * Removed .remove callback
 * Tested the driver with PWM_DEBUG enabled.

RFC->v1:
 * Added Description in binding patch
 * Removed comments from reg and clock
 * replaced rzg2l_gpt_write_mask()->rzg2l_gpt_modify()
 * Added rzg2l_gpt_read() and updated macros
 * Removed dtsi patches, will send it separately

RFC:
 * https://lore.kernel.org/linux-renesas-soc/20220430075915.5036-1-biju.das.jz@bp.renesas.com/T/#t

Biju Das (2):
  dt-bindings: pwm: Add RZ/G2L GPT binding
  pwm: Add support for RZ/G2L GPT

 .../bindings/pwm/renesas,rzg2l-gpt.yaml       | 129 ++++++
 drivers/pwm/Kconfig                           |  11 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-rzg2l-gpt.c                   | 401 ++++++++++++++++++
 4 files changed, 542 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/renesas,rzg2l-gpt.yaml
 create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c


base-commit: 9250d2f72dc46a808b6aa23bf50dd670f1f52ddc
-- 
2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v5 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding
  2022-08-05 14:57 [PATCH v5 0/2] Add support for RZ/G2L GPT Biju Das
@ 2022-08-05 14:57 ` Biju Das
  2022-08-05 14:57 ` [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT Biju Das
  1 sibling, 0 replies; 6+ messages in thread
From: Biju Das @ 2022-08-05 14:57 UTC (permalink / raw)
  To: Thierry Reding, Lee Jones, Rob Herring, Krzysztof Kozlowski
  Cc: Biju Das, Uwe Kleine-König, linux-pwm, devicetree,
	Geert Uytterhoeven, Chris Paterson, Biju Das,
	Prabhakar Mahadev Lad, linux-renesas-soc, Rob Herring

Add device tree bindings for the General PWM Timer (GPT).

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v4->v5:
 * No change.
v3->v4:
 * No change.
v2->v3:
 * Added Rb tag from Rob.
v1->v2:
 * Added '|' after 'description:' to preserve formatting.
 * Removed description for pwm_cells as it is common property.
 * Changed the reg size in example from 0xa4->0x100
 * Added Rb tag from Geert.
RFC->v1:
 * Added Description
 * Removed comments from reg and clock
---
 .../bindings/pwm/renesas,rzg2l-gpt.yaml       | 129 ++++++++++++++++++
 1 file changed, 129 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/renesas,rzg2l-gpt.yaml

diff --git a/Documentation/devicetree/bindings/pwm/renesas,rzg2l-gpt.yaml b/Documentation/devicetree/bindings/pwm/renesas,rzg2l-gpt.yaml
new file mode 100644
index 000000000000..e8f7b9947eaa
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/renesas,rzg2l-gpt.yaml
@@ -0,0 +1,129 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/renesas,rzg2l-gpt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/G2L General PWM Timer (GPT)
+
+maintainers:
+  - Biju Das <biju.das.jz@bp.renesas.com>
+
+description: |
+  RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit timer
+  (GPT32E). It supports the following functions
+  * 32 bits × 8 channels.
+  * Up-counting or down-counting (saw waves) or up/down-counting
+    (triangle waves) for each counter.
+  * Clock sources independently selectable for each channel.
+  * Two I/O pins per channel.
+  * Two output compare/input capture registers per channel.
+  * For the two output compare/input capture registers of each channel,
+    four registers are provided as buffer registers and are capable of
+    operating as comparison registers when buffering is not in use.
+  * In output compare operation, buffer switching can be at crests or
+    troughs, enabling the generation of laterally asymmetric PWM waveforms.
+  * Registers for setting up frame cycles in each channel (with capability
+    for generating interrupts at overflow or underflow)
+  * Generation of dead times in PWM operation.
+  * Synchronous starting, stopping and clearing counters for arbitrary
+    channels.
+  * Starting, stopping, clearing and up/down counters in response to input
+    level comparison.
+  * Starting, clearing, stopping and up/down counters in response to a
+    maximum of four external triggers.
+  * Output pin disable function by dead time error and detected
+    short-circuits between output pins.
+  * A/D converter start triggers can be generated (GPT32E0 to GPT32E3)
+  * Enables the noise filter for input capture and external trigger
+    operation.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a07g044-gpt  # RZ/G2{L,LC}
+          - renesas,r9a07g054-gpt  # RZ/V2L
+      - const: renesas,rzg2l-gpt
+
+  reg:
+    maxItems: 1
+
+  '#pwm-cells':
+    const: 2
+
+  interrupts:
+    items:
+      - description: GTCCRA input capture/compare match
+      - description: GTCCRB input capture/compare
+      - description: GTCCRC compare match
+      - description: GTCCRD compare match
+      - description: GTCCRE compare match
+      - description: GTCCRF compare match
+      - description: GTADTRA compare match
+      - description: GTADTRB compare match
+      - description: GTCNT overflow/GTPR compare match
+      - description: GTCNT underflow
+
+  interrupt-names:
+    items:
+      - const: ccmpa
+      - const: ccmpb
+      - const: cmpc
+      - const: cmpd
+      - const: cmpe
+      - const: cmpf
+      - const: adtrga
+      - const: adtrgb
+      - const: ovf
+      - const: unf
+
+  clocks:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - power-domains
+  - resets
+
+allOf:
+  - $ref: pwm.yaml#
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/r9a07g044-cpg.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    gpt4: pwm@10048400 {
+        compatible = "renesas,r9a07g044-gpt", "renesas,rzg2l-gpt";
+        reg = <0x10048400 0x100>;
+        interrupts = <GIC_SPI 270 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 271 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 272 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 273 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 274 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 275 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 276 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 277 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 278 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 279 IRQ_TYPE_EDGE_RISING>;
+        interrupt-names = "ccmpa", "ccmpb", "cmpc", "cmpd",
+                          "cmpe", "cmpf", "adtrga", "adtrgb",
+                          "ovf", "unf";
+        clocks = <&cpg CPG_MOD R9A07G044_GPT_PCLK>;
+        power-domains = <&cpg>;
+        resets = <&cpg R9A07G044_GPT_RST_C>;
+        #pwm-cells = <2>;
+    };
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT
  2022-08-05 14:57 [PATCH v5 0/2] Add support for RZ/G2L GPT Biju Das
  2022-08-05 14:57 ` [PATCH v5 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
@ 2022-08-05 14:57 ` Biju Das
  2022-08-05 21:05   ` Uwe Kleine-König
  1 sibling, 1 reply; 6+ messages in thread
From: Biju Das @ 2022-08-05 14:57 UTC (permalink / raw)
  To: Thierry Reding, Lee Jones, Philipp Zabel
  Cc: Biju Das, Uwe Kleine-König, linux-pwm, Geert Uytterhoeven,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad,
	linux-renesas-soc

RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit timer
(GPT32E). It supports the following functions
 * 32 bits × 8 channels
 * Up-counting or down-counting (saw waves) or up/down-counting
   (triangle waves) for each counter.
 * Clock sources independently selectable for each channel
 * Two I/O pins per channel
 * Two output compare/input capture registers per channel
 * For the two output compare/input capture registers of each channel,
   four registers are provided as buffer registers and are capable of
   operating as comparison registers when buffering is not in use.
 * In output compare operation, buffer switching can be at crests or
   troughs, enabling the generation of laterally asymmetric PWM waveforms.
 * Registers for setting up frame cycles in each channel (with capability
   for generating interrupts at overflow or underflow)
 * Generation of dead times in PWM operation
 * Synchronous starting, stopping and clearing counters for arbitrary
   channels
 * Starting, stopping, clearing and up/down counters in response to input
   level comparison
 * Starting, clearing, stopping and up/down counters in response to a
   maximum of four external triggers
 * Output pin disable function by dead time error and detected
   short-circuits between output pins
 * A/D converter start triggers can be generated (GPT32E0 to GPT32E3)
 * Enables the noise filter for input capture and external trigger
   operation

This patch adds basic pwm support for RZ/G2L GPT driver by creating
separate logical channels for each IOs.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4->v5:
 * Added Hardware manual details
 * Replaced the comment GTCNT->Counter
 * Removed the macros RZG2L_GPT_IO_PER_CHANNEL and chip.npwm directly
   used in probe.
 * Removed the unsed macro RZG2L_GTPR_MAX_VALUE
 * Added driver prefix for the type name and the variable.
 * Initialization of per_channel data moved from request->probe.
 * Updated clr parameter for rzg2l_gpt_modify for Start count.
 * Started using mutex and usage_count for handling shared
   period and prescalar for the 2 channels.
 * Updated the comment cycle->period.
 * Removed clk_disable from rzg2l_gpt_reset_assert_pm_disable()
 * Replaced pc->rzg2l_gpt.
 * Updated prescale calculation.
 * Moved pm_runtime_{get_sync,put} from {request,free}->{enable,disable}
 * Removed platform_set_drvdata as it is unused
 * Removed the variable pwm_enabled_by_bootloader 
 * Added dev_err_probe in various error paths in probe.
 * Added an error message, if devm_pwmchip_add() fails.
v3->v4:
 * Changed the local variable type i from u16->u8 and prescaled_period_
   cycles from u64->u32 in calculate_prescale().
 * Replaced mul_u64_u64_div_u64()->mul_u64_u32_div()
 * Dropped the comma after the sentinel.
 * Add a variable to track pwm enabled by bootloader and added comments
   in probe().
 * Removed unnecessary rzg2l_gpt_reset_assert_pm_disable() from probe.
 * Replaced devm_clk_get()->devm_clk_get_prepared()
 * Removed devm_clk_get_optional_enabled()
v2->v3:
 * Updated limitation section
 * Added prefix "RZG2L_" for all macros
 * Modified prescale calculation
 * Removed pwm_set_chip_data
 * Updated comment related to modifying Mode and Prescaler
 * Updated setting of prescale value in rzg2l_gpt_config()
 * Removed else branch from rzg2l_gpt_get_state()
 * removed the err label from rzg2l_gpt_apply()
 * Added devm_clk_get_optional_enabled() to retain clk on status,
   in case bootloader turns on the clk of pwm.
 * Replaced devm_reset_control_get_exclusive->devm_reset_control_get_shared
   as single reset shared between 8 channels.
v1->v2:
 * Added Limitations section
 * dropped "_MASK" from the define names.
 * used named initializer for struct phase
 * Added gpt_pwm_device into a flexible array member in rzg2l_gpt_chip
 * Revised the logic for prescale
 * Added .get_state callback
 * Improved error handling in rzg2l_gpt_apply
 * Removed .remove callback
 * Tested driver with PWM_DEBUG enabled
RFC->V1:
 * Updated macros
 * replaced rzg2l_gpt_write_mask()->rzg2l_gpt_modify()
 * Added rzg2l_gpt_read()
---
 drivers/pwm/Kconfig         |  11 +
 drivers/pwm/Makefile        |   1 +
 drivers/pwm/pwm-rzg2l-gpt.c | 401 ++++++++++++++++++++++++++++++++++++
 3 files changed, 413 insertions(+)
 create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 904de8d61828..a6cf24cb31e0 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -471,6 +471,17 @@ config PWM_ROCKCHIP
 	  Generic PWM framework driver for the PWM controller found on
 	  Rockchip SoCs.
 
+config PWM_RZG2L_GPT
+	tristate "Renesas RZ/G2L General PWM Timer support"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This driver exposes the General PWM Timer controller found in Renesas
+	  RZ/G2L like chips through the PWM API.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-rzg2l-gpt.
+
 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 5c08bdb817b4..12bc2a005e24 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
 obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
+obj-$(CONFIG_PWM_RZG2L_GPT)	+= pwm-rzg2l-gpt.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-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
new file mode 100644
index 000000000000..37097994b955
--- /dev/null
+++ b/drivers/pwm/pwm-rzg2l-gpt.c
@@ -0,0 +1,401 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L General PWM Timer (GPT) driver
+ *
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ *
+ * Hardware manual for this IP can be found here
+ * https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?language=en
+ *
+ * Limitations:
+ * - Counter must be stopped before modifying Mode and Prescaler.
+ * - When PWM is disabled, the output is driven to inactive.
+ * - While the hardware supports both polarities, the driver (for now)
+ *   only handles normal polarity.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/time.h>
+
+#define RZG2L_GTCR		0x2c
+#define RZG2L_GTUDDTYC		0x30
+#define RZG2L_GTIOR		0x34
+#define RZG2L_GTBER		0x40
+#define RZG2L_GTCNT		0x48
+#define RZG2L_GTCCRA		0x4c
+#define RZG2L_GTCCRB		0x50
+#define RZG2L_GTPR		0x64
+
+#define RZG2L_GTCR_CST		BIT(0)
+#define RZG2L_GTCR_MD		GENMASK(18, 16)
+#define RZG2L_GTCR_TPCS		GENMASK(26, 24)
+
+#define RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE	FIELD_PREP(RZG2L_GTCR_MD, 0)
+
+#define RZG2L_GTUDDTYC_UP	BIT(0)
+#define RZG2L_GTUDDTYC_UDF	BIT(1)
+#define RZG2L_UP_COUNTING	(RZG2L_GTUDDTYC_UP | RZG2L_GTUDDTYC_UDF)
+
+#define RZG2L_GTIOR_GTIOA	GENMASK(4, 0)
+#define RZG2L_GTIOR_GTIOB	GENMASK(20, 16)
+#define RZG2L_GTIOR_OAE		BIT(8)
+#define RZG2L_GTIOR_OBE		BIT(24)
+
+#define RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE	0x07
+#define RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE	0x1b
+
+#define RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH \
+	(RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE | RZG2L_GTIOR_OAE)
+#define RZG2L_GTIOR_GTIOA_OUT_LO_END_TOGGLE_CMP_MATCH \
+	(RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE | RZG2L_GTIOR_OAE)
+#define RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH \
+	((RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)
+#define RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH \
+	((RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)
+
+struct rzg2l_gpt_phase {
+	u32 value;
+	u32 mask;
+	u32 duty_reg_offset;
+};
+
+static const struct rzg2l_gpt_phase rzg2l_gpt_phase_params[] = {
+	/* Setting for phase A */
+	{
+		.value = RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH,
+		.mask = RZG2L_GTIOR_GTIOA | RZG2L_GTIOR_OAE,
+		.duty_reg_offset = RZG2L_GTCCRA,
+	},
+	/* Setting for phase B */
+	{
+		.value = RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH,
+		.mask = RZG2L_GTIOR_GTIOB | RZG2L_GTIOR_OBE,
+		.duty_reg_offset = RZG2L_GTCCRB,
+	},
+};
+
+struct rzg2l_gpt_pwm_device {
+	const struct rzg2l_gpt_phase *ph;
+};
+
+struct rzg2l_gpt_chip {
+	struct pwm_chip chip;
+	void __iomem *mmio;
+	struct reset_control *rstc;
+	struct mutex lock;
+	struct rzg2l_gpt_pwm_device gpt[2];
+	u32 user_count;
+	u32 real_period;
+	unsigned long rate;
+};
+
+static inline struct rzg2l_gpt_chip *to_rzg2l_gpt_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct rzg2l_gpt_chip, chip);
+}
+
+static void rzg2l_gpt_write(struct rzg2l_gpt_chip *rzg2l_gpt, u32 reg, u32 data)
+{
+	iowrite32(data, rzg2l_gpt->mmio + reg);
+}
+
+static u32 rzg2l_gpt_read(struct rzg2l_gpt_chip *rzg2l_gpt, u32 reg)
+{
+	return ioread32(rzg2l_gpt->mmio + reg);
+}
+
+static void rzg2l_gpt_modify(struct rzg2l_gpt_chip *rzg2l_gpt, u32 reg, u32 clr,
+			     u32 set)
+{
+	rzg2l_gpt_write(rzg2l_gpt, reg,
+			(rzg2l_gpt_read(rzg2l_gpt, reg) & ~clr) | set);
+}
+
+static u8 rzg2l_calculate_prescale(struct rzg2l_gpt_chip *rzg2l_gpt,
+				   u64 period_cycles)
+{
+	u32 prescaled_period_cycles;
+	u8 prescale;
+
+	prescaled_period_cycles = period_cycles >> 32;
+
+	if (prescaled_period_cycles >= 256)
+		prescale = 5;
+	else
+		prescale = (roundup_pow_of_two(prescaled_period_cycles + 1) + 1) / 2;
+
+	return prescale;
+}
+
+static int rzg2l_gpt_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+
+	mutex_lock(&rzg2l_gpt->lock);
+	rzg2l_gpt->user_count++;
+	mutex_unlock(&rzg2l_gpt->lock);
+
+	return 0;
+}
+
+static void rzg2l_gpt_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+
+	mutex_lock(&rzg2l_gpt->lock);
+	rzg2l_gpt->user_count--;
+	mutex_unlock(&rzg2l_gpt->lock);
+}
+
+static int rzg2l_gpt_enable(struct rzg2l_gpt_chip *rzg2l_gpt)
+{
+	/* Start count */
+	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR, 0, RZG2L_GTCR_CST);
+
+	return 0;
+}
+
+static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *rzg2l_gpt)
+{
+	/* Stop count, Output low on GTIOCx pin when counting stops */
+	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR, RZG2L_GTCR_CST, 0);
+}
+
+static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+	struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
+	unsigned long pv, dc;
+	u64 period_cycles;
+	u64 duty_cycles;
+	u8 prescale;
+
+	/*
+	 * Refuse clk rates > 1 GHz to prevent overflowing the following
+	 * calculation.
+	 */
+	if (rzg2l_gpt->rate > NSEC_PER_SEC)
+		return -EINVAL;
+
+	/*
+	 * GPT counter is shared by multiple channels, so prescale and period
+	 * can NOT be modified when there are multiple channels in use with
+	 * different settings.
+	 */
+	if (state->period != rzg2l_gpt->real_period && rzg2l_gpt->user_count > 1)
+		return -EBUSY;
+
+	period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate, NSEC_PER_SEC);
+	prescale = rzg2l_calculate_prescale(rzg2l_gpt, period_cycles);
+
+	pv = period_cycles >> (2 * prescale);
+	duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate, NSEC_PER_SEC);
+	dc = duty_cycles >> (2 * prescale);
+
+	/* Counter must be stopped before modifying Mode and Prescaler */
+	if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
+		rzg2l_gpt_disable(rzg2l_gpt);
+
+	/* GPT set operating mode (saw-wave up-counting) */
+	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR,
+			 RZG2L_GTCR_MD, RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
+
+	/* Set count direction */
+	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);
+
+	rzg2l_gpt->real_period = state->period;
+	/* Select count clock */
+	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR, RZG2L_GTCR_TPCS,
+			 FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
+
+	/* Set period */
+	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR, pv);
+
+	/* Set duty cycle */
+	rzg2l_gpt_write(rzg2l_gpt, gpt->ph->duty_reg_offset, dc);
+
+	/* Set initial value for counter */
+	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT, 0);
+
+	/* Set no buffer operation */
+	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER, 0);
+
+	/* Enable pin output */
+	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR, gpt->ph->mask, gpt->ph->value);
+
+	return 0;
+}
+
+static void rzg2l_gpt_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				struct pwm_state *state)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+	struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
+	u8 prescale;
+	u64 tmp;
+	u32 val;
+
+	/* get period */
+	state->period = rzg2l_gpt->real_period;
+
+	pm_runtime_get_sync(chip->dev);
+	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR);
+	state->enabled = val & RZG2L_GTCR_CST;
+	if (state->enabled) {
+		prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
+
+		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR);
+		tmp = NSEC_PER_SEC * val << (2 * prescale);
+		state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
+
+		val = rzg2l_gpt_read(rzg2l_gpt, gpt->ph->duty_reg_offset);
+		tmp = NSEC_PER_SEC * val << (2 * prescale);
+		state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
+		/*
+		 * Ordering is important, when we set a period for the second
+		 * channel, as pwm_request_from_chip() calling get_state() will
+		 * have an invalid duty cycle value as the register is not
+		 * initialized yet. So set duty_cycle to zero.
+		 */
+		if (state->duty_cycle > state->period &&
+		    rzg2l_gpt->user_count > 1)
+			state->duty_cycle = 0;
+	}
+
+	state->polarity = PWM_POLARITY_NORMAL;
+	pm_runtime_put(chip->dev);
+}
+
+static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			   const struct pwm_state *state)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+	int ret;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	pm_runtime_get_sync(chip->dev);
+	if (!state->enabled) {
+		rzg2l_gpt_disable(rzg2l_gpt);
+		ret = 0;
+		goto done;
+	}
+
+	mutex_lock(&rzg2l_gpt->lock);
+	ret = rzg2l_gpt_config(chip, pwm, state);
+	mutex_unlock(&rzg2l_gpt->lock);
+	if (ret)
+		goto done;
+
+	return rzg2l_gpt_enable(rzg2l_gpt);
+
+done:
+	pm_runtime_put(chip->dev);
+
+	return ret;
+}
+
+static const struct pwm_ops rzg2l_gpt_ops = {
+	.request = rzg2l_gpt_request,
+	.free = rzg2l_gpt_free,
+	.get_state = rzg2l_gpt_get_state,
+	.apply = rzg2l_gpt_apply,
+	.owner = THIS_MODULE,
+};
+
+static const struct of_device_id rzg2l_gpt_of_table[] = {
+	{ .compatible = "renesas,rzg2l-gpt", },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
+
+static void rzg2l_gpt_reset_assert_pm_disable(void *data)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = data;
+
+	pm_runtime_disable(rzg2l_gpt->chip.dev);
+	reset_control_assert(rzg2l_gpt->rstc);
+}
+
+static int rzg2l_gpt_probe(struct platform_device *pdev)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt;
+	struct clk *clk;
+	int ret, i;
+
+	rzg2l_gpt = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_gpt), GFP_KERNEL);
+	if (!rzg2l_gpt)
+		return -ENOMEM;
+
+	rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(rzg2l_gpt->mmio))
+		return PTR_ERR(rzg2l_gpt->mmio);
+
+	rzg2l_gpt->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
+	if (IS_ERR(rzg2l_gpt->rstc))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->rstc),
+				     "get reset failed\n");
+
+	ret = reset_control_deassert(rzg2l_gpt->rstc);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "cannot deassert reset control\n");
+
+	pm_runtime_enable(&pdev->dev);
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rzg2l_gpt_reset_assert_pm_disable,
+				       rzg2l_gpt);
+	if (ret < 0)
+		return ret;
+
+	clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+				     "cannot get clock\n");
+
+	rzg2l_gpt->rate = clk_get_rate(clk);
+	/*
+	 *  We need to keep the clock on, in case the bootloader enabled PWM and
+	 *  is running during probe().
+	 */
+	if (!(rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST))
+		devm_clk_put(&pdev->dev, clk);
+
+	mutex_init(&rzg2l_gpt->lock);
+
+	rzg2l_gpt->chip.dev = &pdev->dev;
+	rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
+	rzg2l_gpt->chip.npwm = 2;
+	for (i = 0; i < rzg2l_gpt->chip.npwm; i++)
+		rzg2l_gpt->gpt[i].ph = &rzg2l_gpt_phase_params[i];
+
+	ret = devm_pwmchip_add(&pdev->dev, &rzg2l_gpt->chip);
+	if (ret)
+		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+
+	return ret;
+}
+
+static struct platform_driver rzg2l_gpt_driver = {
+	.driver = {
+		.name = "pwm-rzg2l-gpt",
+		.of_match_table = of_match_ptr(rzg2l_gpt_of_table),
+	},
+	.probe = rzg2l_gpt_probe,
+};
+module_platform_driver(rzg2l_gpt_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/G2L General PWM Timer (GPT) Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pwm-rzg2l-gpt");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT
  2022-08-05 14:57 ` [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT Biju Das
@ 2022-08-05 21:05   ` Uwe Kleine-König
  2022-08-23 16:28     ` Biju Das
  0 siblings, 1 reply; 6+ messages in thread
From: Uwe Kleine-König @ 2022-08-05 21:05 UTC (permalink / raw)
  To: Biju Das
  Cc: Thierry Reding, Lee Jones, Philipp Zabel, linux-pwm,
	Geert Uytterhoeven, Chris Paterson, Biju Das,
	Prabhakar Mahadev Lad, linux-renesas-soc

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

Hello,

On Fri, Aug 05, 2022 at 03:57:04PM +0100, Biju Das wrote:
> +#define RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH \
> +	(RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE | RZG2L_GTIOR_OAE)
> +#define RZG2L_GTIOR_GTIOA_OUT_LO_END_TOGGLE_CMP_MATCH \
> +	(RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE | RZG2L_GTIOR_OAE)
> +#define RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH \
> +	((RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)

FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE)

> +#define RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH \
> +	((RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)
> +
> [...]
> +static u8 rzg2l_calculate_prescale(struct rzg2l_gpt_chip *rzg2l_gpt,
> +				   u64 period_cycles)
> +{
> +	u32 prescaled_period_cycles;
> +	u8 prescale;
> +
> +	prescaled_period_cycles = period_cycles >> 32;
> +
> +	if (prescaled_period_cycles >= 256)
> +		prescale = 5;
> +	else
> +		prescale = (roundup_pow_of_two(prescaled_period_cycles + 1) + 1) / 2;

I double checked, this looks correct to me.

> +
> +	return prescale;
> +}
> +
> [...]
> +static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> +	struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
> +	unsigned long pv, dc;
> +	u64 period_cycles;
> +	u64 duty_cycles;
> +	u8 prescale;
> +
> +	/*
> +	 * Refuse clk rates > 1 GHz to prevent overflowing the following
> +	 * calculation.
> +	 */
> +	if (rzg2l_gpt->rate > NSEC_PER_SEC)
> +		return -EINVAL;
> +
> +	/*
> +	 * GPT counter is shared by multiple channels, so prescale and period
> +	 * can NOT be modified when there are multiple channels in use with
> +	 * different settings.
> +	 */
> +	if (state->period != rzg2l_gpt->real_period && rzg2l_gpt->user_count > 1)
> +		return -EBUSY;

Optional improvement here: If a period of (say) 100000 ns is requested the
hardware might likely actually implement 99875 ns. As
rzg2l_gpt->real_period corresponds to the requested period (is that a
misnomer?) you could accept state->period = 99900.

Accepting state->period >= rzg2l_gpt->real_period is fine.

> +
> +	period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate, NSEC_PER_SEC);
> +	prescale = rzg2l_calculate_prescale(rzg2l_gpt, period_cycles);
> +
> +	pv = period_cycles >> (2 * prescale);

If period_cycles is >= (1024 << 32), we get prescale = 5 and so
period_cycles >> (2 * prescale) doesn't fit into 32 bits. This needs
handling.

> +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate, NSEC_PER_SEC);
> +	dc = duty_cycles >> (2 * prescale);
> +
> +	/* Counter must be stopped before modifying Mode and Prescaler */
> +	if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
> +		rzg2l_gpt_disable(rzg2l_gpt);

Does this affect the other channel? If yes, that's a bad thing and it
might be worth to improve here.

> +	/* GPT set operating mode (saw-wave up-counting) */
> +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR,
> +			 RZG2L_GTCR_MD, RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
> +
> +	/* Set count direction */
> +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);
> +
> +	rzg2l_gpt->real_period = state->period;
> +	/* Select count clock */
> +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR, RZG2L_GTCR_TPCS,
> +			 FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> +
> +	/* Set period */
> +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR, pv);
> +
> +	/* Set duty cycle */
> +	rzg2l_gpt_write(rzg2l_gpt, gpt->ph->duty_reg_offset, dc);
> +
> +	/* Set initial value for counter */
> +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT, 0);
> +
> +	/* Set no buffer operation */
> +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER, 0);
> +
> +	/* Enable pin output */
> +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR, gpt->ph->mask, gpt->ph->value);
> +
> +	return 0;
> +}
> +
> +static void rzg2l_gpt_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				struct pwm_state *state)
> +{
> +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> +	struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
> +	u8 prescale;
> +	u64 tmp;
> +	u32 val;
> +
> +	/* get period */
> +	state->period = rzg2l_gpt->real_period;
> +
> +	pm_runtime_get_sync(chip->dev);
> +	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR);
> +	state->enabled = val & RZG2L_GTCR_CST;
> +	if (state->enabled) {
> +		prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> +
> +		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR);
> +		tmp = NSEC_PER_SEC * val << (2 * prescale);
> +		state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> +
> +		val = rzg2l_gpt_read(rzg2l_gpt, gpt->ph->duty_reg_offset);

I still wonder if this is really better/more effective/easier to
understand than just:

/* These are actually called GTCCRA and GTCCRB */
#define RZG2L_GTCCR(i) (0x4c + 4 * (i))

plus

	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm));

	
> +		tmp = NSEC_PER_SEC * val << (2 * prescale);
> +		state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> +		/*
> +		 * Ordering is important, when we set a period for the second
> +		 * channel, as pwm_request_from_chip() calling get_state() will
> +		 * have an invalid duty cycle value as the register is not
> +		 * initialized yet. So set duty_cycle to zero.

I don't understand that issue. Can you just drop the check
"rzg2l_gpt->user_count > 1"?

If you configure channel #0 while channel #1 is still untouched (in
software), does this modify the output of channel #1?

> +		 */
> +		if (state->duty_cycle > state->period &&
> +		    rzg2l_gpt->user_count > 1)
> +			state->duty_cycle = 0;

Does this setting (i.e. GTCCR{A,B} > GTPR) correspond to a 100% relative
duty cycle?

> +	}
> +
> +	state->polarity = PWM_POLARITY_NORMAL;
> +	pm_runtime_put(chip->dev);
> +}
> +
> +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   const struct pwm_state *state)
> +{
> +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> +	int ret;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	pm_runtime_get_sync(chip->dev);
> +	if (!state->enabled) {
> +		rzg2l_gpt_disable(rzg2l_gpt);
> +		ret = 0;
> +		goto done;
> +	}
> +
> +	mutex_lock(&rzg2l_gpt->lock);
> +	ret = rzg2l_gpt_config(chip, pwm, state);
> +	mutex_unlock(&rzg2l_gpt->lock);
> +	if (ret)
> +		goto done;
> +
> +	return rzg2l_gpt_enable(rzg2l_gpt);
> +
> +done:
> +	pm_runtime_put(chip->dev);
> +
> +	return ret;
> +}
> +
> +static const struct pwm_ops rzg2l_gpt_ops = {
> +	.request = rzg2l_gpt_request,
> +	.free = rzg2l_gpt_free,
> +	.get_state = rzg2l_gpt_get_state,
> +	.apply = rzg2l_gpt_apply,
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id rzg2l_gpt_of_table[] = {
> +	{ .compatible = "renesas,rzg2l-gpt", },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
> +
> +static void rzg2l_gpt_reset_assert_pm_disable(void *data)
> +{
> +	struct rzg2l_gpt_chip *rzg2l_gpt = data;
> +
> +	pm_runtime_disable(rzg2l_gpt->chip.dev);
> +	reset_control_assert(rzg2l_gpt->rstc);
> +}
> +
> +static int rzg2l_gpt_probe(struct platform_device *pdev)
> +{
> +	struct rzg2l_gpt_chip *rzg2l_gpt;
> +	struct clk *clk;
> +	int ret, i;
> +
> +	rzg2l_gpt = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_gpt), GFP_KERNEL);
> +	if (!rzg2l_gpt)
> +		return -ENOMEM;
> +
> +	rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rzg2l_gpt->mmio))
> +		return PTR_ERR(rzg2l_gpt->mmio);
> +
> +	rzg2l_gpt->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> +	if (IS_ERR(rzg2l_gpt->rstc))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->rstc),
> +				     "get reset failed\n");
> +
> +	ret = reset_control_deassert(rzg2l_gpt->rstc);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "cannot deassert reset control\n");
> +
> +	pm_runtime_enable(&pdev->dev);
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       rzg2l_gpt_reset_assert_pm_disable,
> +				       rzg2l_gpt);
> +	if (ret < 0)
> +		return ret;
> +
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> +				     "cannot get clock\n");
> +
> +	rzg2l_gpt->rate = clk_get_rate(clk);
> +	/*
> +	 *  We need to keep the clock on, in case the bootloader enabled PWM and
> +	 *  is running during probe().
> +	 */
> +	if (!(rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST))
> +		devm_clk_put(&pdev->dev, clk);

I still think this looks wrong. Please at least comment about the idea
here. ie. devm_clk_put disables the clk and holding a reference on the
clk isn't needed because runtime-pm handles the needed enabling.

Is this really true? Does runtime-pm disable the clk if after the clk
wasn't put here both PWMs are disabled?

> +	mutex_init(&rzg2l_gpt->lock);
> +
> +	rzg2l_gpt->chip.dev = &pdev->dev;
> +	rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> +	rzg2l_gpt->chip.npwm = 2;
> +	for (i = 0; i < rzg2l_gpt->chip.npwm; i++)
> +		rzg2l_gpt->gpt[i].ph = &rzg2l_gpt_phase_params[i];
> +
> +	ret = devm_pwmchip_add(&pdev->dev, &rzg2l_gpt->chip);
> +	if (ret)
> +		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +
> +	return ret;
> +}
> +
> +static struct platform_driver rzg2l_gpt_driver = {
> +	.driver = {
> +		.name = "pwm-rzg2l-gpt",
> +		.of_match_table = of_match_ptr(rzg2l_gpt_of_table),
> +	},
> +	.probe = rzg2l_gpt_probe,
> +};
> +module_platform_driver(rzg2l_gpt_driver);
> +
> +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas RZ/G2L General PWM Timer (GPT) Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-rzg2l-gpt");

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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT
  2022-08-05 21:05   ` Uwe Kleine-König
@ 2022-08-23 16:28     ` Biju Das
  2022-09-05 17:12       ` Biju Das
  0 siblings, 1 reply; 6+ messages in thread
From: Biju Das @ 2022-08-23 16:28 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Thierry Reding, Lee Jones, Philipp Zabel, linux-pwm,
	Geert Uytterhoeven, Chris Paterson, Biju Das,
	Prabhakar Mahadev Lad, linux-renesas-soc

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT
> 
> Hello,
> 
> On Fri, Aug 05, 2022 at 03:57:04PM +0100, Biju Das wrote:
> > +#define RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH \
> > +	(RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE | RZG2L_GTIOR_OAE) #define
> > +RZG2L_GTIOR_GTIOA_OUT_LO_END_TOGGLE_CMP_MATCH \
> > +	(RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE | RZG2L_GTIOR_OAE) #define
> > +RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH \
> > +	((RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)
> 
> FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE)

We cannot use this macro for structure initialization,
I get below compilation errors for variable .value in struct rzg2l_gpt_phase. 
Not sure, Maybe we should use if statement with the "FIELD_PREP" macro and 
drop the variable "value" from struct rzg2l_gpt_phase??

In file included from drivers/pwm/pwm-rzg2l-gpt.c:17:
./include/linux/bitfield.h:113:2: error: braced-group within expression allowed only inside a function
  113 |  ({        \
      |  ^
drivers/pwm/pwm-rzg2l-gpt.c:60:2: note: in expansion of macro 'FIELD_PREP'
   60 |  FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE)
      |  ^~~~~~~~~~
drivers/pwm/pwm-rzg2l-gpt.c:81:12: note: in expansion of macro 'RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH'
   81 |   .value = RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH,
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> 
> > +#define RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH \
> > +	((RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)
> > +
> > [...]
> > +static u8 rzg2l_calculate_prescale(struct rzg2l_gpt_chip *rzg2l_gpt,
> > +				   u64 period_cycles)
> > +{
> > +	u32 prescaled_period_cycles;
> > +	u8 prescale;
> > +
> > +	prescaled_period_cycles = period_cycles >> 32;
> > +
> > +	if (prescaled_period_cycles >= 256)
> > +		prescale = 5;
> > +	else
> > +		prescale = (roundup_pow_of_two(prescaled_period_cycles + 1)
> + 1) /
> > +2;
> 
> I double checked, this looks correct to me.
> 
> > +
> > +	return prescale;
> > +}
> > +
> > [...]
> > +static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > +			    const struct pwm_state *state) {
> > +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > +	struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
> > +	unsigned long pv, dc;
> > +	u64 period_cycles;
> > +	u64 duty_cycles;
> > +	u8 prescale;
> > +
> > +	/*
> > +	 * Refuse clk rates > 1 GHz to prevent overflowing the following
> > +	 * calculation.
> > +	 */
> > +	if (rzg2l_gpt->rate > NSEC_PER_SEC)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * GPT counter is shared by multiple channels, so prescale and
> period
> > +	 * can NOT be modified when there are multiple channels in use
> with
> > +	 * different settings.
> > +	 */
> > +	if (state->period != rzg2l_gpt->real_period && rzg2l_gpt-
> >user_count > 1)
> > +		return -EBUSY;
> 
> Optional improvement here: If a period of (say) 100000 ns is requested
> the hardware might likely actually implement 99875 ns. As rzg2l_gpt-
> >real_period corresponds to the requested period (is that a
> misnomer?)

Yes, it is misnomer here. Actually, it is rzg2l_gpt->state_period,
Where we cache the first period value for both the channels with usage count
incremented.
Here 100000 ns means, we are caching 100000 ns.

>you could accept state->period = 99900.
> 
> Accepting state->period >= rzg2l_gpt->real_period is fine.
> 
> > +
> > +	period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate,
> NSEC_PER_SEC);
> > +	prescale = rzg2l_calculate_prescale(rzg2l_gpt, period_cycles);
> > +
> > +	pv = period_cycles >> (2 * prescale);
> 
> If period_cycles is >= (1024 << 32), we get prescale = 5 and so
> period_cycles >> (2 * prescale) doesn't fit into 32 bits. This needs
> handling.

OK, will do the below change.

+       if (period_cycles >= (1024ULL << 32))
+               period_cycles = 1024ULL << 32;


> 
> > +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate,
> NSEC_PER_SEC);
> > +	dc = duty_cycles >> (2 * prescale);
> > +
> > +	/* Counter must be stopped before modifying Mode and Prescaler */
> > +	if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
> > +		rzg2l_gpt_disable(rzg2l_gpt);
> 
> Does this affect the other channel? If yes, that's a bad thing and it
> might be worth to improve here.

Yes, it affects the other channels, please share any suggestions
for improvement??

> 
> > +	/* GPT set operating mode (saw-wave up-counting) */
> > +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR,
> > +			 RZG2L_GTCR_MD, RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
> > +
> > +	/* Set count direction */
> > +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);
> > +
> > +	rzg2l_gpt->real_period = state->period;
> > +	/* Select count clock */
> > +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR, RZG2L_GTCR_TPCS,
> > +			 FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> > +
> > +	/* Set period */
> > +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR, pv);
> > +
> > +	/* Set duty cycle */
> > +	rzg2l_gpt_write(rzg2l_gpt, gpt->ph->duty_reg_offset, dc);
> > +
> > +	/* Set initial value for counter */
> > +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT, 0);
> > +
> > +	/* Set no buffer operation */
> > +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER, 0);
> > +
> > +	/* Enable pin output */
> > +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR, gpt->ph->mask,
> > +gpt->ph->value);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rzg2l_gpt_get_state(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > +				struct pwm_state *state)
> > +{
> > +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > +	struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
> > +	u8 prescale;
> > +	u64 tmp;
> > +	u32 val;
> > +
> > +	/* get period */
> > +	state->period = rzg2l_gpt->real_period;
> > +
> > +	pm_runtime_get_sync(chip->dev);
> > +	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR);
> > +	state->enabled = val & RZG2L_GTCR_CST;
> > +	if (state->enabled) {
> > +		prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> > +
> > +		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR);
> > +		tmp = NSEC_PER_SEC * val << (2 * prescale);
> > +		state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> > +
> > +		val = rzg2l_gpt_read(rzg2l_gpt, gpt->ph->duty_reg_offset);
> 
> I still wonder if this is really better/more effective/easier to
> understand than just:
> 
> /* These are actually called GTCCRA and GTCCRB */ #define RZG2L_GTCCR(i)
> (0x4c + 4 * (i))
> 
> plus
> 
> 	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm));

Agreed.

> 
> 
> > +		tmp = NSEC_PER_SEC * val << (2 * prescale);
> > +		state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> > +		/*
> > +		 * Ordering is important, when we set a period for the
> second
> > +		 * channel, as pwm_request_from_chip() calling get_state()
> will
> > +		 * have an invalid duty cycle value as the register is not
> > +		 * initialized yet. So set duty_cycle to zero.
> 
> I don't understand that issue. Can you just drop the check "rzg2l_gpt-
> >user_count > 1"?

No, I get a high duty cycle register value (Reset value) for the second channel
as the Register is not initialized yet. I have enabled some debug for better understanding.
Please let me know still it s clear.

pr_info("## %s ch=%d enabled=%d, duty_cycle_reg=%x",__func__,pwm->hwpwm,state->enabled,rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm)));

Please see the test code[1] and corresponding log[2].

The problematic condition is here.
[   36.751009] ## rzg2l_gpt_get_state ch=1 enabled=1, duty_cycle_reg=ffffffff

As you can see with pwm_debug enabled, the state->enable is 1, even though
duty cycle register is not initialized.

[1] Please see the test code with PWM_DEBUG enabled:

echo 0 > /sys/class/pwm/pwmchip0/export
echo 500000000 > /sys/class/pwm/pwmchip0/pwm0/period
echo 250000000 > /sys/class/pwm/pwmchip0/pwm0/duty_cycle                   
echo 1 > /sys/class/pwm/pwmchip0/pwm0/enable                                                                                                           

echo 1 > /sys/class/pwm/pwmchip0/export
echo 150000000 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle  
echo 500000000 > /sys/class/pwm/pwmchip0/pwm1/period      
echo 1 > /sys/class/pwm/pwmchip0/pwm1/enable

echo 0 > /sys/class/pwm/pwmchip0/pwm0/enable
echo 0 > /sys/class/pwm/pwmchip0/pwm1/enable  

echo 1 > /sys/class/pwm/pwmchip0/unexport
echo 0 > /sys/class/pwm/pwmchip0/unexport


[2]logs:-

root@smarc-rzg2l:~# /poeg_dual.sh
[   36.688776] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[   36.698051] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[   36.705625] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[   36.718096] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[   36.725717] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[   36.735656] ## rzg2l_gpt_get_state ch=0 enabled=1, duty_cycle_reg=5f5e10
[   36.743156] ## rzg2l_gpt_get_state ch=0 enabled=1, duty_cycle_reg=5f5e10
[   36.751009] ## rzg2l_gpt_get_state ch=1 enabled=1, duty_cycle_reg=ffffffff
[   36.760968] ## rzg2l_gpt_get_state ch=1 enabled=1, duty_cycle_reg=393870
[   36.768136] ## rzg2l_gpt_get_state ch=1 enabled=1, duty_cycle_reg=393870
[   39.834765] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=5f5e10
[   39.841729] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=5f5e10
[   42.886686] ## rzg2l_gpt_get_state ch=1 enabled=0, duty_cycle_reg=393870

> 
> If you configure channel #0 while channel #1 is still untouched (in
> software), does this modify the output of channel #1?


> 
> > +		 */
> > +		if (state->duty_cycle > state->period &&
> > +		    rzg2l_gpt->user_count > 1)
> > +			state->duty_cycle = 0;
> 
> Does this setting (i.e. GTCCR{A,B} > GTPR) correspond to a 100% relative
> duty cycle?

No. I am setting to a value of 0, Since register is not initialized as mentioned above.

> 
> > +	}
> > +
> > +	state->polarity = PWM_POLARITY_NORMAL;
> > +	pm_runtime_put(chip->dev);
> > +}
> > +
> > +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > +			   const struct pwm_state *state)
> > +{
> > +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > +	int ret;
> > +
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -EINVAL;
> > +
> > +	pm_runtime_get_sync(chip->dev);
> > +	if (!state->enabled) {
> > +		rzg2l_gpt_disable(rzg2l_gpt);
> > +		ret = 0;
> > +		goto done;
> > +	}
> > +
> > +	mutex_lock(&rzg2l_gpt->lock);
> > +	ret = rzg2l_gpt_config(chip, pwm, state);
> > +	mutex_unlock(&rzg2l_gpt->lock);
> > +	if (ret)
> > +		goto done;
> > +
> > +	return rzg2l_gpt_enable(rzg2l_gpt);
> > +
> > +done:
> > +	pm_runtime_put(chip->dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct pwm_ops rzg2l_gpt_ops = {
> > +	.request = rzg2l_gpt_request,
> > +	.free = rzg2l_gpt_free,
> > +	.get_state = rzg2l_gpt_get_state,
> > +	.apply = rzg2l_gpt_apply,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static const struct of_device_id rzg2l_gpt_of_table[] = {
> > +	{ .compatible = "renesas,rzg2l-gpt", },
> > +	{ /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
> > +
> > +static void rzg2l_gpt_reset_assert_pm_disable(void *data) {
> > +	struct rzg2l_gpt_chip *rzg2l_gpt = data;
> > +
> > +	pm_runtime_disable(rzg2l_gpt->chip.dev);
> > +	reset_control_assert(rzg2l_gpt->rstc);
> > +}
> > +
> > +static int rzg2l_gpt_probe(struct platform_device *pdev) {
> > +	struct rzg2l_gpt_chip *rzg2l_gpt;
> > +	struct clk *clk;
> > +	int ret, i;
> > +
> > +	rzg2l_gpt = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_gpt),
> GFP_KERNEL);
> > +	if (!rzg2l_gpt)
> > +		return -ENOMEM;
> > +
> > +	rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(rzg2l_gpt->mmio))
> > +		return PTR_ERR(rzg2l_gpt->mmio);
> > +
> > +	rzg2l_gpt->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> > +	if (IS_ERR(rzg2l_gpt->rstc))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->rstc),
> > +				     "get reset failed\n");
> > +
> > +	ret = reset_control_deassert(rzg2l_gpt->rstc);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "cannot deassert reset control\n");
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rzg2l_gpt_reset_assert_pm_disable,
> > +				       rzg2l_gpt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > +	if (IS_ERR(clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> > +				     "cannot get clock\n");
> > +
> > +	rzg2l_gpt->rate = clk_get_rate(clk);
> > +	/*
> > +	 *  We need to keep the clock on, in case the bootloader enabled
> PWM and
> > +	 *  is running during probe().
> > +	 */
> > +	if (!(rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST))
> > +		devm_clk_put(&pdev->dev, clk);
> 
> I still think this looks wrong. Please at least comment about the idea
> here. ie. devm_clk_put disables the clk and holding a reference on the
> clk isn't needed because runtime-pm handles the needed enabling.

OK, will add the comment. Yes, the code should be improved

1) This code is OK for the case bootloader is not turning on PWM.

2) But if Bootloader turns on the PWM, then we need extra handling to call
  "devm_clk_put" when usage count becomes "0" after activation of PWM channels.

  ie, use a private variable for tracking bootloader on case and call "devm_clk_put"
  during "rzg2l_gpt_free" and reset the private variable.

 After that there is no clock framework resources and PM handles the clocks
 exclusively.
   
> 
> Is this really true? Does runtime-pm disable the clk if after the clk
> wasn't put here both PWMs are disabled?

No it need special handling, See point 2 above.

Cheers,
Biju

> 
> > +	mutex_init(&rzg2l_gpt->lock);
> > +
> > +	rzg2l_gpt->chip.dev = &pdev->dev;
> > +	rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > +	rzg2l_gpt->chip.npwm = 2;
> > +	for (i = 0; i < rzg2l_gpt->chip.npwm; i++)
> > +		rzg2l_gpt->gpt[i].ph = &rzg2l_gpt_phase_params[i];
> > +
> > +	ret = devm_pwmchip_add(&pdev->dev, &rzg2l_gpt->chip);
> > +	if (ret)
> > +		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +
> > +	return ret;
> > +}
> > +
> > +static struct platform_driver rzg2l_gpt_driver = {
> > +	.driver = {
> > +		.name = "pwm-rzg2l-gpt",
> > +		.of_match_table = of_match_ptr(rzg2l_gpt_of_table),
> > +	},
> > +	.probe = rzg2l_gpt_probe,
> > +};
> > +module_platform_driver(rzg2l_gpt_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L General PWM Timer (GPT) Driver");
> > +MODULE_LICENSE("GPL"); MODULE_ALIAS("platform:pwm-rzg2l-gpt");
> |

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT
  2022-08-23 16:28     ` Biju Das
@ 2022-09-05 17:12       ` Biju Das
  0 siblings, 0 replies; 6+ messages in thread
From: Biju Das @ 2022-09-05 17:12 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Thierry Reding, Lee Jones, Philipp Zabel, linux-pwm,
	Geert Uytterhoeven, Chris Paterson, Biju Das,
	Prabhakar Mahadev Lad, linux-renesas-soc

Hi Uwe,

> Subject: RE: [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT
> 
> Hi Uwe,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT
> >
> > Hello,
> >
> > On Fri, Aug 05, 2022 at 03:57:04PM +0100, Biju Das wrote:
> > > +#define RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH \
> > > +	(RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE | RZG2L_GTIOR_OAE) #define
> > > +RZG2L_GTIOR_GTIOA_OUT_LO_END_TOGGLE_CMP_MATCH \
> > > +	(RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE | RZG2L_GTIOR_OAE) #define
> > > +RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH \
> > > +	((RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)
> >
> > FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE)
> 
> We cannot use this macro for structure initialization, I get below
> compilation errors for variable .value in struct rzg2l_gpt_phase.
> Not sure, Maybe we should use if statement with the "FIELD_PREP" macro
> and drop the variable "value" from struct rzg2l_gpt_phase??
> 
> In file included from drivers/pwm/pwm-rzg2l-gpt.c:17:
> ./include/linux/bitfield.h:113:2: error: braced-group within expression
> allowed only inside a function
>   113 |  ({        \
>       |  ^
> drivers/pwm/pwm-rzg2l-gpt.c:60:2: note: in expansion of macro
> 'FIELD_PREP'
>    60 |  FIELD_PREP(RZG2L_GTIOR_GTIOB,
> RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE)
>       |  ^~~~~~~~~~
> drivers/pwm/pwm-rzg2l-gpt.c:81:12: note: in expansion of macro
> 'RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH'
>    81 |   .value = RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH,
>       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> >
> > > +#define RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH \
> > > +	((RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)
> > > +
> > > [...]
> > > +static u8 rzg2l_calculate_prescale(struct rzg2l_gpt_chip *rzg2l_gpt,
> > > +				   u64 period_cycles)
> > > +{
> > > +	u32 prescaled_period_cycles;
> > > +	u8 prescale;
> > > +
> > > +	prescaled_period_cycles = period_cycles >> 32;
> > > +
> > > +	if (prescaled_period_cycles >= 256)
> > > +		prescale = 5;
> > > +	else
> > > +		prescale = (roundup_pow_of_two(prescaled_period_cycles + 1)
> > + 1) /
> > > +2;
> >
> > I double checked, this looks correct to me.
> >
> > > +
> > > +	return prescale;
> > > +}
> > > +
> > > [...]
> > > +static int rzg2l_gpt_config(struct pwm_chip *chip, struct
> > > +pwm_device
> > *pwm,
> > > +			    const struct pwm_state *state) {
> > > +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > > +	struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
> > > +	unsigned long pv, dc;
> > > +	u64 period_cycles;
> > > +	u64 duty_cycles;
> > > +	u8 prescale;
> > > +
> > > +	/*
> > > +	 * Refuse clk rates > 1 GHz to prevent overflowing the following
> > > +	 * calculation.
> > > +	 */
> > > +	if (rzg2l_gpt->rate > NSEC_PER_SEC)
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * GPT counter is shared by multiple channels, so prescale and
> > period
> > > +	 * can NOT be modified when there are multiple channels in use
> > with
> > > +	 * different settings.
> > > +	 */
> > > +	if (state->period != rzg2l_gpt->real_period && rzg2l_gpt-
> > >user_count > 1)
> > > +		return -EBUSY;
> >
> > Optional improvement here: If a period of (say) 100000 ns is requested
> > the hardware might likely actually implement 99875 ns. As rzg2l_gpt-
> > >real_period corresponds to the requested period (is that a
> > misnomer?)
> 
> Yes, it is misnomer here. Actually, it is rzg2l_gpt->state_period, Where
> we cache the first period value for both the channels with usage count
> incremented.
> Here 100000 ns means, we are caching 100000 ns.
> 
> >you could accept state->period = 99900.
> >
> > Accepting state->period >= rzg2l_gpt->real_period is fine.
> >
> > > +
> > > +	period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate,
> > NSEC_PER_SEC);
> > > +	prescale = rzg2l_calculate_prescale(rzg2l_gpt, period_cycles);
> > > +
> > > +	pv = period_cycles >> (2 * prescale);
> >
> > If period_cycles is >= (1024 << 32), we get prescale = 5 and so
> > period_cycles >> (2 * prescale) doesn't fit into 32 bits. This needs
> > handling.
> 
> OK, will do the below change.
> 
> +       if (period_cycles >= (1024ULL << 32))
> +               period_cycles = 1024ULL << 32;
> 
> 
> >
> > > +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate,
> > NSEC_PER_SEC);
> > > +	dc = duty_cycles >> (2 * prescale);
> > > +
> > > +	/* Counter must be stopped before modifying Mode and Prescaler */
> > > +	if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
> > > +		rzg2l_gpt_disable(rzg2l_gpt);
> >
> > Does this affect the other channel? If yes, that's a bad thing and it
> > might be worth to improve here.
> 
> Yes, it affects the other channels, please share any suggestions for
> improvement??
> 
> >
> > > +	/* GPT set operating mode (saw-wave up-counting) */
> > > +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR,
> > > +			 RZG2L_GTCR_MD, RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
> > > +
> > > +	/* Set count direction */
> > > +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);
> > > +
> > > +	rzg2l_gpt->real_period = state->period;
> > > +	/* Select count clock */
> > > +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR, RZG2L_GTCR_TPCS,
> > > +			 FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> > > +
> > > +	/* Set period */
> > > +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR, pv);
> > > +
> > > +	/* Set duty cycle */
> > > +	rzg2l_gpt_write(rzg2l_gpt, gpt->ph->duty_reg_offset, dc);
> > > +
> > > +	/* Set initial value for counter */
> > > +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT, 0);
> > > +
> > > +	/* Set no buffer operation */
> > > +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER, 0);
> > > +
> > > +	/* Enable pin output */
> > > +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR, gpt->ph->mask,
> > > +gpt->ph->value);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void rzg2l_gpt_get_state(struct pwm_chip *chip, struct
> > pwm_device *pwm,
> > > +				struct pwm_state *state)
> > > +{
> > > +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > > +	struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
> > > +	u8 prescale;
> > > +	u64 tmp;
> > > +	u32 val;
> > > +
> > > +	/* get period */
> > > +	state->period = rzg2l_gpt->real_period;
> > > +
> > > +	pm_runtime_get_sync(chip->dev);
> > > +	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR);
> > > +	state->enabled = val & RZG2L_GTCR_CST;
> > > +	if (state->enabled) {
> > > +		prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> > > +
> > > +		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR);
> > > +		tmp = NSEC_PER_SEC * val << (2 * prescale);
> > > +		state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> > > +
> > > +		val = rzg2l_gpt_read(rzg2l_gpt, gpt->ph->duty_reg_offset);
> >
> > I still wonder if this is really better/more effective/easier to
> > understand than just:
> >
> > /* These are actually called GTCCRA and GTCCRB */ #define
> > RZG2L_GTCCR(i) (0x4c + 4 * (i))
> >
> > plus
> >
> > 	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm));
> 
> Agreed.
> 
> >
> >
> > > +		tmp = NSEC_PER_SEC * val << (2 * prescale);
> > > +		state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> > > +		/*
> > > +		 * Ordering is important, when we set a period for the
> > second
> > > +		 * channel, as pwm_request_from_chip() calling get_state()
> > will
> > > +		 * have an invalid duty cycle value as the register is not
> > > +		 * initialized yet. So set duty_cycle to zero.
> >
> > I don't understand that issue. Can you just drop the check "rzg2l_gpt-
> > >user_count > 1"?
> 
> No, I get a high duty cycle register value (Reset value) for the second
> channel as the Register is not initialized yet. I have enabled some debug
> for better understanding.
> Please let me know still it s clear.
> 
> pr_info("## %s ch=%d enabled=%d, duty_cycle_reg=%x",__func__,pwm-
> >hwpwm,state->enabled,rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm-
> >hwpwm)));
> 
> Please see the test code[1] and corresponding log[2].
> 
> The problematic condition is here.
> [   36.751009] ## rzg2l_gpt_get_state ch=1 enabled=1,
> duty_cycle_reg=ffffffff
> 
> As you can see with pwm_debug enabled, the state->enable is 1, even
> though duty cycle register is not initialized.
> 
> [1] Please see the test code with PWM_DEBUG enabled:
> 
> echo 0 > /sys/class/pwm/pwmchip0/export
> echo 500000000 > /sys/class/pwm/pwmchip0/pwm0/period
> echo 250000000 > /sys/class/pwm/pwmchip0/pwm0/duty_cycle
> echo 1 > /sys/class/pwm/pwmchip0/pwm0/enable
> 
> echo 1 > /sys/class/pwm/pwmchip0/export
> echo 150000000 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> echo 500000000 > /sys/class/pwm/pwmchip0/pwm1/period
> echo 1 > /sys/class/pwm/pwmchip0/pwm1/enable
> 
> echo 0 > /sys/class/pwm/pwmchip0/pwm0/enable
> echo 0 > /sys/class/pwm/pwmchip0/pwm1/enable
> 
> echo 1 > /sys/class/pwm/pwmchip0/unexport echo 0 >
> /sys/class/pwm/pwmchip0/unexport
> 
> 
> [2]logs:-
> 
> root@smarc-rzg2l:~# /poeg_dual.sh
> [   36.688776] ## rzg2l_gpt_get_state ch=0 enabled=0,
> duty_cycle_reg=ffffffff
> [   36.698051] ## rzg2l_gpt_get_state ch=0 enabled=0,
> duty_cycle_reg=ffffffff
> [   36.705625] ## rzg2l_gpt_get_state ch=0 enabled=0,
> duty_cycle_reg=ffffffff
> [   36.718096] ## rzg2l_gpt_get_state ch=0 enabled=0,
> duty_cycle_reg=ffffffff
> [   36.725717] ## rzg2l_gpt_get_state ch=0 enabled=0,
> duty_cycle_reg=ffffffff
> [   36.735656] ## rzg2l_gpt_get_state ch=0 enabled=1,
> duty_cycle_reg=5f5e10
> [   36.743156] ## rzg2l_gpt_get_state ch=0 enabled=1,
> duty_cycle_reg=5f5e10
> [   36.751009] ## rzg2l_gpt_get_state ch=1 enabled=1,
> duty_cycle_reg=ffffffff
> [   36.760968] ## rzg2l_gpt_get_state ch=1 enabled=1,
> duty_cycle_reg=393870
> [   36.768136] ## rzg2l_gpt_get_state ch=1 enabled=1,
> duty_cycle_reg=393870
> [   39.834765] ## rzg2l_gpt_get_state ch=0 enabled=0,
> duty_cycle_reg=5f5e10
> [   39.841729] ## rzg2l_gpt_get_state ch=0 enabled=0,
> duty_cycle_reg=5f5e10
> [   42.886686] ## rzg2l_gpt_get_state ch=1 enabled=0,
> duty_cycle_reg=393870
> 
> >
> > If you configure channel #0 while channel #1 is still untouched (in
> > software), does this modify the output of channel #1?
> 
> 
> >
> > > +		 */
> > > +		if (state->duty_cycle > state->period &&
> > > +		    rzg2l_gpt->user_count > 1)
> > > +			state->duty_cycle = 0;
> >
> > Does this setting (i.e. GTCCR{A,B} > GTPR) correspond to a 100%
> > relative duty cycle?
> 
> No. I am setting to a value of 0, Since register is not initialized as
> mentioned above.
> 
> >
> > > +	}
> > > +
> > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > +	pm_runtime_put(chip->dev);
> > > +}
> > > +
> > > +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device
> > *pwm,
> > > +			   const struct pwm_state *state) {
> > > +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > > +	int ret;
> > > +
> > > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > > +		return -EINVAL;
> > > +
> > > +	pm_runtime_get_sync(chip->dev);
> > > +	if (!state->enabled) {
> > > +		rzg2l_gpt_disable(rzg2l_gpt);
> > > +		ret = 0;
> > > +		goto done;
> > > +	}
> > > +
> > > +	mutex_lock(&rzg2l_gpt->lock);
> > > +	ret = rzg2l_gpt_config(chip, pwm, state);
> > > +	mutex_unlock(&rzg2l_gpt->lock);
> > > +	if (ret)
> > > +		goto done;
> > > +
> > > +	return rzg2l_gpt_enable(rzg2l_gpt);
> > > +
> > > +done:
> > > +	pm_runtime_put(chip->dev);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct pwm_ops rzg2l_gpt_ops = {
> > > +	.request = rzg2l_gpt_request,
> > > +	.free = rzg2l_gpt_free,
> > > +	.get_state = rzg2l_gpt_get_state,
> > > +	.apply = rzg2l_gpt_apply,
> > > +	.owner = THIS_MODULE,
> > > +};
> > > +
> > > +static const struct of_device_id rzg2l_gpt_of_table[] = {
> > > +	{ .compatible = "renesas,rzg2l-gpt", },
> > > +	{ /* Sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
> > > +
> > > +static void rzg2l_gpt_reset_assert_pm_disable(void *data) {
> > > +	struct rzg2l_gpt_chip *rzg2l_gpt = data;
> > > +
> > > +	pm_runtime_disable(rzg2l_gpt->chip.dev);
> > > +	reset_control_assert(rzg2l_gpt->rstc);
> > > +}
> > > +
> > > +static int rzg2l_gpt_probe(struct platform_device *pdev) {
> > > +	struct rzg2l_gpt_chip *rzg2l_gpt;
> > > +	struct clk *clk;
> > > +	int ret, i;
> > > +
> > > +	rzg2l_gpt = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_gpt),
> > GFP_KERNEL);
> > > +	if (!rzg2l_gpt)
> > > +		return -ENOMEM;
> > > +
> > > +	rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
> > > +	if (IS_ERR(rzg2l_gpt->mmio))
> > > +		return PTR_ERR(rzg2l_gpt->mmio);
> > > +
> > > +	rzg2l_gpt->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> > > +	if (IS_ERR(rzg2l_gpt->rstc))
> > > +		return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->rstc),
> > > +				     "get reset failed\n");
> > > +
> > > +	ret = reset_control_deassert(rzg2l_gpt->rstc);
> > > +	if (ret)
> > > +		return dev_err_probe(&pdev->dev, ret,
> > > +				     "cannot deassert reset control\n");
> > > +
> > > +	pm_runtime_enable(&pdev->dev);
> > > +	ret = devm_add_action_or_reset(&pdev->dev,
> > > +				       rzg2l_gpt_reset_assert_pm_disable,
> > > +				       rzg2l_gpt);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > > +	if (IS_ERR(clk))
> > > +		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> > > +				     "cannot get clock\n");
> > > +
> > > +	rzg2l_gpt->rate = clk_get_rate(clk);
> > > +	/*
> > > +	 *  We need to keep the clock on, in case the bootloader enabled
> > PWM and
> > > +	 *  is running during probe().
> > > +	 */
> > > +	if (!(rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST))
> > > +		devm_clk_put(&pdev->dev, clk);
> >
> > I still think this looks wrong. Please at least comment about the idea
> > here. ie. devm_clk_put disables the clk and holding a reference on the
> > clk isn't needed because runtime-pm handles the needed enabling.
> 
> OK, will add the comment. Yes, the code should be improved
> 
> 1) This code is OK for the case bootloader is not turning on PWM.
> 
> 2) But if Bootloader turns on the PWM, then we need extra handling to
> call
>   "devm_clk_put" when usage count becomes "0" after activation of PWM
> channels.
> 
>   ie, use a private variable for tracking bootloader on case and call
> "devm_clk_put"
>   during "rzg2l_gpt_free" and reset the private variable.
> 
>  After that there is no clock framework resources and PM handles the
> clocks  exclusively.
> 
> >
> > Is this really true? Does runtime-pm disable the clk if after the clk
> > wasn't put here both PWMs are disabled?
> 
> No it need special handling, See point 2 above.
> 

I have created v6 incorporating the comments above
and also updated handling of bootloader enabling/disabling pwm.

Please provide feedback, if any.

Cheers,
Biju

> 
> >
> > > +	mutex_init(&rzg2l_gpt->lock);
> > > +
> > > +	rzg2l_gpt->chip.dev = &pdev->dev;
> > > +	rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > > +	rzg2l_gpt->chip.npwm = 2;
> > > +	for (i = 0; i < rzg2l_gpt->chip.npwm; i++)
> > > +		rzg2l_gpt->gpt[i].ph = &rzg2l_gpt_phase_params[i];
> > > +
> > > +	ret = devm_pwmchip_add(&pdev->dev, &rzg2l_gpt->chip);
> > > +	if (ret)
> > > +		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static struct platform_driver rzg2l_gpt_driver = {
> > > +	.driver = {
> > > +		.name = "pwm-rzg2l-gpt",
> > > +		.of_match_table = of_match_ptr(rzg2l_gpt_of_table),
> > > +	},
> > > +	.probe = rzg2l_gpt_probe,
> > > +};
> > > +module_platform_driver(rzg2l_gpt_driver);
> > > +
> > > +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> > > +MODULE_DESCRIPTION("Renesas RZ/G2L General PWM Timer (GPT)
> > > +Driver"); MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:pwm-rzg2l-gpt");
> > |

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-09-05 17:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-05 14:57 [PATCH v5 0/2] Add support for RZ/G2L GPT Biju Das
2022-08-05 14:57 ` [PATCH v5 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
2022-08-05 14:57 ` [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT Biju Das
2022-08-05 21:05   ` Uwe Kleine-König
2022-08-23 16:28     ` Biju Das
2022-09-05 17:12       ` Biju Das

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).