* [PATCH v6 0/2] Add support for RZ/G2L GPT
@ 2022-09-05 17:13 Biju Das
2022-09-05 17:13 ` [PATCH v6 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
2022-09-05 17:13 ` [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT Biju Das
0 siblings, 2 replies; 18+ messages in thread
From: Biju Das @ 2022-09-05 17:13 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.
v5->v6:
* Updated macros RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH and
RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH with computation
involving FIELD_PREP macro.
* Removed struct rzg2l_gpt_phase and started using RZG2L_GTCCR macro
for duty_offset.
* replaced misnomer real_period->state_period.
* Added handling for values >= (1024 << 32) for both period
and duty cycle.
* Added comments for pwm {en,dis}abled by bootloader during probe.
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 | 409 ++++++++++++++++++
4 files changed, 550 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/renesas,rzg2l-gpt.yaml
create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding
2022-09-05 17:13 [PATCH v6 0/2] Add support for RZ/G2L GPT Biju Das
@ 2022-09-05 17:13 ` Biju Das
2022-09-05 17:13 ` [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT Biju Das
1 sibling, 0 replies; 18+ messages in thread
From: Biju Das @ 2022-09-05 17:13 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>
---
v5->v6:
* No change.
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] 18+ messages in thread
* [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-05 17:13 [PATCH v6 0/2] Add support for RZ/G2L GPT Biju Das
2022-09-05 17:13 ` [PATCH v6 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
@ 2022-09-05 17:13 ` Biju Das
2022-09-19 7:57 ` Uwe Kleine-König
1 sibling, 1 reply; 18+ messages in thread
From: Biju Das @ 2022-09-05 17:13 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>
---
v5->v6:
* Updated macros RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH and
RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH with computation
involving FIELD_PREP macro.
* Removed struct rzg2l_gpt_phase and started using RZG2L_GTCCR macro
for duty_offset.
* replaced misnomer real_period->state_period.
* Added handling for values >= (1024 << 32) for both period
and duty cycle.
* Added comments for pwm {en,dis}abled by bootloader during probe.
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 | 409 ++++++++++++++++++++++++++++++++++++
3 files changed, 421 insertions(+)
create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..2723a3e9ff65 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -481,6 +481,17 @@ config PWM_ROCKCHIP
Generic PWM framework driver for the PWM controller found on
Rockchip SoCs.
+config PWM_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 7bf1a29f02b8..cac39b18d1ee 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE) += pwm-raspberrypi-poe.o
obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
+obj-$(CONFIG_PWM_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..9913dd995891
--- /dev/null
+++ b/drivers/pwm/pwm-rzg2l-gpt.c
@@ -0,0 +1,409 @@
+// 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/limits.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 \
+ (FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE) | RZG2L_GTIOR_OBE)
+#define RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH \
+ (FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE) | RZG2L_GTIOR_OBE)
+
+#define RZG2L_GTCCR(i) (0x4c + 4 * (i))
+
+struct rzg2l_gpt_chip {
+ struct pwm_chip chip;
+ void __iomem *mmio;
+ struct reset_control *rstc;
+ struct clk *clk;
+ struct mutex lock;
+ u32 user_count;
+ u32 state_period;
+ unsigned long rate;
+ bool pwm_enabled_by_bootloader;
+};
+
+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);
+ 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->state_period && rzg2l_gpt->user_count > 1)
+ return -EBUSY;
+
+ rzg2l_gpt->state_period = state->period;
+
+ period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate, NSEC_PER_SEC);
+ prescale = rzg2l_calculate_prescale(rzg2l_gpt, period_cycles);
+
+ if (period_cycles >= (1024ULL << 32))
+ pv = U32_MAX;
+ else
+ pv = period_cycles >> (2 * prescale);
+
+ duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate, NSEC_PER_SEC);
+
+ if (duty_cycles >= (1024ULL << 32))
+ dc = U32_MAX;
+ else
+ 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);
+
+ /* 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, RZG2L_GTCCR(pwm->hwpwm), 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 */
+ if (pwm->hwpwm)
+ rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR,
+ RZG2L_GTIOR_GTIOB | RZG2L_GTIOR_OBE,
+ RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH);
+ else
+ rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR,
+ RZG2L_GTIOR_GTIOA | RZG2L_GTIOR_OAE,
+ RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH);
+
+ 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);
+ u8 prescale;
+ u64 tmp;
+ u32 val;
+
+ /* get period */
+ state->period = rzg2l_gpt->state_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, 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.
+ */
+ 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) {
+ if (rzg2l_gpt->pwm_enabled_by_bootloader) {
+ /*
+ * For PWM enabled by bootloader case, Release the clk
+ * framework resources and set the pwm_enabled_by_bootloader
+ * variable to false. After this clock is managed by PM.
+ */
+ rzg2l_gpt->pwm_enabled_by_bootloader = false;
+ devm_clk_put(chip->dev, rzg2l_gpt->clk);
+ }
+
+ 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;
+ int ret;
+
+ 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;
+
+ rzg2l_gpt->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+ if (IS_ERR(rzg2l_gpt->clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->clk),
+ "cannot get clock\n");
+
+ rzg2l_gpt->rate = clk_get_rate(rzg2l_gpt->clk);
+ /*
+ * We need to keep the clock on, in case the bootloader has enabled the
+ * PWM and is running during probe(). A variable pwm_enabled_by_
+ * bootloader is set to true in that case and during first
+ * pwm_disable(), it is set to false and release the clock resource.
+ *
+ * In case the pwm is not enabled by the bootloader, devm_clk_put
+ * disables the clk and holding a reference on the clk isn't needed
+ * because runtime-pm handles the needed enabling.
+ */
+ if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
+ rzg2l_gpt->pwm_enabled_by_bootloader = true;
+ else
+ devm_clk_put(&pdev->dev, rzg2l_gpt->clk);
+
+ mutex_init(&rzg2l_gpt->lock);
+
+ rzg2l_gpt->chip.dev = &pdev->dev;
+ rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
+ rzg2l_gpt->chip.npwm = 2;
+
+ 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] 18+ messages in thread
* Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-05 17:13 ` [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT Biju Das
@ 2022-09-19 7:57 ` Uwe Kleine-König
2022-09-20 15:31 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Uwe Kleine-König @ 2022-09-19 7:57 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: 21331 bytes --]
Hello,
On Mon, Sep 05, 2022 at 06:13:28PM +0100, Biju Das wrote:
> 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>
> ---
> v5->v6:
> * Updated macros RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH and
> RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH with computation
> involving FIELD_PREP macro.
> * Removed struct rzg2l_gpt_phase and started using RZG2L_GTCCR macro
> for duty_offset.
> * replaced misnomer real_period->state_period.
> * Added handling for values >= (1024 << 32) for both period
> and duty cycle.
> * Added comments for pwm {en,dis}abled by bootloader during probe.
> 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 | 409 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 421 insertions(+)
> create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 60d13a949bc5..2723a3e9ff65 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -481,6 +481,17 @@ config PWM_ROCKCHIP
> Generic PWM framework driver for the PWM controller found on
> Rockchip SoCs.
>
> +config PWM_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 7bf1a29f02b8..cac39b18d1ee 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE) += pwm-raspberrypi-poe.o
> obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
> obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> +obj-$(CONFIG_PWM_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..9913dd995891
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> @@ -0,0 +1,409 @@
> +// 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/limits.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 \
> + (FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE) | RZG2L_GTIOR_OBE)
> +#define RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH \
> + (FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE) | RZG2L_GTIOR_OBE)
> +
> +#define RZG2L_GTCCR(i) (0x4c + 4 * (i))
> +
> +struct rzg2l_gpt_chip {
> + struct pwm_chip chip;
> + void __iomem *mmio;
> + struct reset_control *rstc;
> + struct clk *clk;
> + struct mutex lock;
> + u32 user_count;
> + u32 state_period;
> + unsigned long rate;
> + bool pwm_enabled_by_bootloader;
> +};
> +
> +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);
> + 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->state_period && rzg2l_gpt->user_count > 1)
> + return -EBUSY;
I had feedback to that one in v5. Please at least state the potential to
improve in a comment.
> +
> + rzg2l_gpt->state_period = state->period;
> +
> + period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate, NSEC_PER_SEC);
> + prescale = rzg2l_calculate_prescale(rzg2l_gpt, period_cycles);
> +
> + if (period_cycles >= (1024ULL << 32))
> + pv = U32_MAX;
> + else
> + pv = period_cycles >> (2 * prescale);
You're assuming that pv <= U32_MAX after this block, right? Then maybe
if (period_cycles >> (2 * prescale) <= U32_MAX)
is the more intuitive check?
> + duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate, NSEC_PER_SEC);
> +
> + if (duty_cycles >= (1024ULL << 32))
> + dc = U32_MAX;
> + else
> + 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);
For v5 I asked if this affects other channels, you said yes and in the
follow up I failed to reply how to improve this.
I wonder how this affects other channels. Does it restart a period
afterwards, or is the effect only that the currently running period is a
bit stretched? At least point that this stops the global counter and so
affects the other PWMs provided by this chip.
> +
> + /* 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);
> +
> + /* 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, RZG2L_GTCCR(pwm->hwpwm), 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 */
> + if (pwm->hwpwm)
> + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR,
> + RZG2L_GTIOR_GTIOB | RZG2L_GTIOR_OBE,
> + RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH);
> + else
> + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR,
> + RZG2L_GTIOR_GTIOA | RZG2L_GTIOR_OAE,
> + RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH);
> +
> + 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);
> + u8 prescale;
> + u64 tmp;
> + u32 val;
> +
> + /* get period */
> + state->period = rzg2l_gpt->state_period;
This is useless. For one thing .state_period is only set by .apply()
which might not have been called and in the interesting cases (i.e. with
.enabled = true) the value is overwritten anyhow. I'd just drop this
line.
> + 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);
This can overflow.
> + state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> +
> + val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm));
> + tmp = NSEC_PER_SEC * val << (2 * prescale);
ditto.
> + 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;
I would expect
if (state->duty_cycle > state->period)
state->duty_cycle = state->period;
here?
> + }
> +
> + 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) {
> + if (rzg2l_gpt->pwm_enabled_by_bootloader) {
> + /*
> + * For PWM enabled by bootloader case, Release the clk
> + * framework resources and set the pwm_enabled_by_bootloader
> + * variable to false. After this clock is managed by PM.
> + */
> + rzg2l_gpt->pwm_enabled_by_bootloader = false;
> + devm_clk_put(chip->dev, rzg2l_gpt->clk);
> + }
> +
> + 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;
> + int ret;
> +
> + 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;
> +
> + rzg2l_gpt->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> + if (IS_ERR(rzg2l_gpt->clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->clk),
> + "cannot get clock\n");
> +
> + rzg2l_gpt->rate = clk_get_rate(rzg2l_gpt->clk);
> + /*
> + * We need to keep the clock on, in case the bootloader has enabled the
> + * PWM and is running during probe(). A variable pwm_enabled_by_
> + * bootloader is set to true in that case and during first
> + * pwm_disable(), it is set to false and release the clock resource.
> + *
> + * In case the pwm is not enabled by the bootloader, devm_clk_put
> + * disables the clk and holding a reference on the clk isn't needed
> + * because runtime-pm handles the needed enabling.
> + */
> + if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
> + rzg2l_gpt->pwm_enabled_by_bootloader = true;
> + else
> + devm_clk_put(&pdev->dev, rzg2l_gpt->clk);
While it works I don't particularly like this construct. But maybe
that's only me and I don't have a nice alternative to offer.
> +
> + mutex_init(&rzg2l_gpt->lock);
> +
> + rzg2l_gpt->chip.dev = &pdev->dev;
> + rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> + rzg2l_gpt->chip.npwm = 2;
> +
> + 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] 18+ messages in thread
* RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-19 7:57 ` Uwe Kleine-König
@ 2022-09-20 15:31 ` Biju Das
2022-09-20 15:53 ` Uwe Kleine-König
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2022-09-20 15:31 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 v6 2/2] pwm: Add support for RZ/G2L GPT
>
> Hello,
>
> On Mon, Sep 05, 2022 at 06:13:28PM +0100, Biju Das wrote:
> > 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>
> > ---
> > v5->v6:
> > * Updated macros RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH and
> > RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH with computation
> > involving FIELD_PREP macro.
> > * Removed struct rzg2l_gpt_phase and started using RZG2L_GTCCR
> macro
> > for duty_offset.
> > * replaced misnomer real_period->state_period.
> > * Added handling for values >= (1024 << 32) for both period
> > and duty cycle.
> > * Added comments for pwm {en,dis}abled by bootloader during probe.
> > 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 | 409
> > ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 421 insertions(+)
> > create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 60d13a949bc5..2723a3e9ff65 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -481,6 +481,17 @@ config PWM_ROCKCHIP
> > Generic PWM framework driver for the PWM controller found on
> > Rockchip SoCs.
> >
> > +config PWM_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 7bf1a29f02b8..cac39b18d1ee 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -44,6 +44,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE) += pwm-
> raspberrypi-poe.o
> > obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
> > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> > +obj-$(CONFIG_PWM_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..9913dd995891
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > @@ -0,0 +1,409 @@
> > +// 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-u
> > +sers-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/limits.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 \
> > + (FIELD_PREP(RZG2L_GTIOR_GTIOB,
> RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE)
> > +| RZG2L_GTIOR_OBE) #define
> RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH \
> > + (FIELD_PREP(RZG2L_GTIOR_GTIOB,
> RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE)
> > +| RZG2L_GTIOR_OBE)
> > +
> > +#define RZG2L_GTCCR(i) (0x4c + 4 * (i))
> > +
> > +struct rzg2l_gpt_chip {
> > + struct pwm_chip chip;
> > + void __iomem *mmio;
> > + struct reset_control *rstc;
> > + struct clk *clk;
> > + struct mutex lock;
> > + u32 user_count;
> > + u32 state_period;
> > + unsigned long rate;
> > + bool pwm_enabled_by_bootloader;
> > +};
> > +
> > +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);
> > + 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->state_period && rzg2l_gpt-
> >user_count > 1)
> > + return -EBUSY;
>
> I had feedback to that one in v5. Please at least state the potential
> to improve in a comment.
Oops, I missed to update the comment.
GPT counter is shared by multiple channels, we cache the period value
from the first channel used in Linux and use it for both the
channels.
I will update the comment in the next version.
>
> > +
> > + rzg2l_gpt->state_period = state->period;
> > +
> > + period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate,
> NSEC_PER_SEC);
> > + prescale = rzg2l_calculate_prescale(rzg2l_gpt, period_cycles);
> > +
> > + if (period_cycles >= (1024ULL << 32))
> > + pv = U32_MAX;
> > + else
> > + pv = period_cycles >> (2 * prescale);
>
> You're assuming that pv <= U32_MAX after this block, right? Then maybe
Yes, That is correct.
>
> if (period_cycles >> (2 * prescale) <= U32_MAX)
>
> is the more intuitive check?
Ok will add like below, so we support up to (U32_MAX * 1024);
Is it ok for you?
if (!(period_cycles >> (2 * prescale) <= U32_MAX))
+ return -EINVAL;
+
+ pv = period_cycles >> (2 * prescale);
Same case for duty cycle.
>
> > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate,
> > +NSEC_PER_SEC);
> > +
> > + if (duty_cycles >= (1024ULL << 32))
> > + dc = U32_MAX;
> > + else
> > + 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);
>
> For v5 I asked if this affects other channels, you said yes and in the
> follow up I failed to reply how to improve this.
>
> I wonder how this affects other channels. Does it restart a period
> afterwards, or is the effect only that the currently running period is
> a bit stretched?
If we stops the counter, it resets to starting count position.
>At least point that this stops the global counter and
> so affects the other PWMs provided by this chip.
We should not allow Counter to stop if it is running.
We should allow changing mode and prescalar only for the first
enabled channel in Linux.
Also as per the HW manual, we should not change RZG2L_GTCNT, RZG2L_GTBER while
Counter is running.
Will add bool is_counter_running to take care of this conditions.
Is it ok with you?
>
> > +
> > + /* 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);
> > +
> > + /* 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, RZG2L_GTCCR(pwm->hwpwm), 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 */
> > + if (pwm->hwpwm)
> > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR,
> > + RZG2L_GTIOR_GTIOB | RZG2L_GTIOR_OBE,
> > +
> RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH);
> > + else
> > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR,
> > + RZG2L_GTIOR_GTIOA | RZG2L_GTIOR_OAE,
> > +
> RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH);
> > +
> > + 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);
> > + u8 prescale;
> > + u64 tmp;
> > + u32 val;
> > +
> > + /* get period */
> > + state->period = rzg2l_gpt->state_period;
>
> This is useless. For one thing .state_period is only set by .apply()
> which might not have been called and in the interesting cases (i.e.
> with .enabled = true) the value is overwritten anyhow. I'd just drop
> this line.
OK , Agreed Will drop it.
>
> > + 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);
>
> This can overflow.
OK will use inverse calculation to avoid overflow.
mul_u64_u32_div(val << (2 * prescale), NSEC_PER_SEC, rzg2l_gpt->rate);
Is it ok?
>
> > + state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm));
> > + tmp = NSEC_PER_SEC * val << (2 * prescale);
>
> ditto.
>
> > + 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;
>
> I would expect
>
> if (state->duty_cycle > state->period)
> state->duty_cycle = state->period;
>
> here?
OK. Agreed.
>
> > + }
> > +
> > + 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) {
> > + if (rzg2l_gpt->pwm_enabled_by_bootloader) {
> > + /*
> > + * For PWM enabled by bootloader case, Release the
> clk
> > + * framework resources and set the
> pwm_enabled_by_bootloader
> > + * variable to false. After this clock is managed by
> PM.
> > + */
> > + rzg2l_gpt->pwm_enabled_by_bootloader = false;
> > + devm_clk_put(chip->dev, rzg2l_gpt->clk);
> > + }
> > +
> > + 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;
> > + int ret;
> > +
> > + 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;
> > +
> > + rzg2l_gpt->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > + if (IS_ERR(rzg2l_gpt->clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->clk),
> > + "cannot get clock\n");
> > +
> > + rzg2l_gpt->rate = clk_get_rate(rzg2l_gpt->clk);
> > + /*
> > + * We need to keep the clock on, in case the bootloader has
> enabled the
> > + * PWM and is running during probe(). A variable pwm_enabled_by_
> > + * bootloader is set to true in that case and during first
> > + * pwm_disable(), it is set to false and release the clock
> resource.
> > + *
> > + * In case the pwm is not enabled by the bootloader,
> devm_clk_put
> > + * disables the clk and holding a reference on the clk isn't
> needed
> > + * because runtime-pm handles the needed enabling.
> > + */
> > + if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
> > + rzg2l_gpt->pwm_enabled_by_bootloader = true;
> > + else
> > + devm_clk_put(&pdev->dev, rzg2l_gpt->clk);
>
> While it works I don't particularly like this construct. But maybe
> that's only me and I don't have a nice alternative to offer.
OK.
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;
> > +
> > + 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/ |
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-20 15:31 ` Biju Das
@ 2022-09-20 15:53 ` Uwe Kleine-König
2022-09-20 17:00 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Uwe Kleine-König @ 2022-09-20 15:53 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: 3417 bytes --]
Hello,
On Tue, Sep 20, 2022 at 03:31:16PM +0000, Biju Das wrote:
> > On Mon, Sep 05, 2022 at 06:13:28PM +0100, Biju Das wrote:
> > > + if (period_cycles >= (1024ULL << 32))
> > > + pv = U32_MAX;
> > > + else
> > > + pv = period_cycles >> (2 * prescale);
> >
> > You're assuming that pv <= U32_MAX after this block, right? Then maybe
> Yes, That is correct.
>
> >
> > if (period_cycles >> (2 * prescale) <= U32_MAX)
> >
> > is the more intuitive check?
>
> Ok will add like below, so we support up to (U32_MAX * 1024);
> Is it ok for you?
>
> if (!(period_cycles >> (2 * prescale) <= U32_MAX))
> + return -EINVAL;
> +
> + pv = period_cycles >> (2 * prescale);
Not -EINVAL, using pv = U32_MAX is correct.
> Same case for duty cycle.
> >
> > > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate,
> > > +NSEC_PER_SEC);
> > > +
> > > + if (duty_cycles >= (1024ULL << 32))
> > > + dc = U32_MAX;
> > > + else
> > > + 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);
> >
> > For v5 I asked if this affects other channels, you said yes and in the
> > follow up I failed to reply how to improve this.
> >
> > I wonder how this affects other channels. Does it restart a period
> > afterwards, or is the effect only that the currently running period is
> > a bit stretched?
>
> If we stops the counter, it resets to starting count position.
So if I update pwm#1, pwm#0 doesn't only freeze for a moment, but
starts a new period. Hui.
> >At least point that this stops the global counter and
> > so affects the other PWMs provided by this chip.
>
> We should not allow Counter to stop if it is running.
> We should allow changing mode and prescalar only for the first
> enabled channel in Linux.
>
> Also as per the HW manual, we should not change RZG2L_GTCNT, RZG2L_GTBER while
> Counter is running.
>
> Will add bool is_counter_running to take care of this conditions.
>
> Is it ok with you?
I'm torn here. Resetting the period for the other counter is quite
disturbing. If you cannot prevent that, please document that in the
Limitations section above.
> > > + 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);
> >
> > This can overflow.
>
> OK will use inverse calculation to avoid overflow.
> mul_u64_u32_div(val << (2 * prescale), NSEC_PER_SEC, rzg2l_gpt->rate);
>
> Is it ok?
It uses the wrong rounding direction :-\ Using
tmp = NSEC_PER_SEC * (u64)val << (2 * prescale);
should be enough to fix the problem I pointed out.
> > > + state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> > > +
> > > + val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm));
> > > + tmp = NSEC_PER_SEC * val << (2 * prescale);
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] 18+ messages in thread
* RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-20 15:53 ` Uwe Kleine-König
@ 2022-09-20 17:00 ` Biju Das
2022-09-21 10:50 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2022-09-20 17:00 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 v6 2/2] pwm: Add support for RZ/G2L GPT
>
> Hello,
>
> On Tue, Sep 20, 2022 at 03:31:16PM +0000, Biju Das wrote:
> > > On Mon, Sep 05, 2022 at 06:13:28PM +0100, Biju Das wrote:
> > > > + if (period_cycles >= (1024ULL << 32))
> > > > + pv = U32_MAX;
> > > > + else
> > > > + pv = period_cycles >> (2 * prescale);
> > >
> > > You're assuming that pv <= U32_MAX after this block, right? Then
> > > maybe
> > Yes, That is correct.
> >
> > >
> > > if (period_cycles >> (2 * prescale) <= U32_MAX)
> > >
> > > is the more intuitive check?
> >
> > Ok will add like below, so we support up to (U32_MAX * 1024); Is it
> ok
> > for you?
> >
> > if (!(period_cycles >> (2 * prescale) <= U32_MAX))
> > + return -EINVAL;
> > +
> > + pv = period_cycles >> (2 * prescale);
>
> Not -EINVAL, using pv = U32_MAX is correct.
OK.
>
> > Same case for duty cycle.
> > >
> > > > + duty_cycles = mul_u64_u32_div(state->duty_cycle,
> > > > +rzg2l_gpt->rate, NSEC_PER_SEC);
> > > > +
> > > > + if (duty_cycles >= (1024ULL << 32))
> > > > + dc = U32_MAX;
> > > > + else
> > > > + 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);
> > >
> > > For v5 I asked if this affects other channels, you said yes and in
> > > the follow up I failed to reply how to improve this.
> > >
> > > I wonder how this affects other channels. Does it restart a period
> > > afterwards, or is the effect only that the currently running
> period
> > > is a bit stretched?
> >
> > If we stops the counter, it resets to starting count position.
>
> So if I update pwm#1, pwm#0 doesn't only freeze for a moment, but
> starts a new period. Hui.
>
> > >At least point that this stops the global counter and so affects
> the
> > >other PWMs provided by this chip.
> >
> > We should not allow Counter to stop if it is running.
> > We should allow changing mode and prescalar only for the first
> enabled
> > channel in Linux.
> >
> > Also as per the HW manual, we should not change RZG2L_GTCNT,
> > RZG2L_GTBER while Counter is running.
> >
> > Will add bool is_counter_running to take care of this conditions.
> >
> > Is it ok with you?
>
> I'm torn here. Resetting the period for the other counter is quite
> disturbing. If you cannot prevent that, please document that in the
> Limitations section above.
I think, we should be able to prevent resetting the counter value
By not stopping it for the second channel. For second channel
We just update duty cycle and GTIO pins.
>
> > > > + 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);
> > >
> > > This can overflow.
> >
> > OK will use inverse calculation to avoid overflow.
> > mul_u64_u32_div(val << (2 * prescale), NSEC_PER_SEC, rzg2l_gpt-
> >rate);
> >
> > Is it ok?
>
> It uses the wrong rounding direction :-\ Using
>
> tmp = NSEC_PER_SEC * (u64)val << (2 * prescale);
>
> should be enough to fix the problem I pointed out.
OK, Thanks for the pointer.
Cheers,
Biju
>
> > > > + state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt-
> >rate);
> > > > +
> > > > + val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm-
> >hwpwm));
> > > > + tmp = NSEC_PER_SEC * val << (2 * prescale);
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König
> |
> Industrial Linux Solutions |
> https://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-20 17:00 ` Biju Das
@ 2022-09-21 10:50 ` Biju Das
2022-09-21 13:35 ` Uwe Kleine-König
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2022-09-21 10:50 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
> -----Original Message-----
> From: Biju Das
> Sent: 20 September 2022 18:01
> To: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Thierry Reding <thierry.reding@gmail.com>; Lee Jones
> <lee.jones@linaro.org>; Philipp Zabel <p.zabel@pengutronix.de>; linux-
> pwm@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
> Chris Paterson <Chris.Paterson2@renesas.com>; Biju Das
> <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
>
> Hi Uwe,
>
> > Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> >
> > Hello,
> >
> > On Tue, Sep 20, 2022 at 03:31:16PM +0000, Biju Das wrote:
> > > > On Mon, Sep 05, 2022 at 06:13:28PM +0100, Biju Das wrote:
> > > > > + if (period_cycles >= (1024ULL << 32))
> > > > > + pv = U32_MAX;
> > > > > + else
> > > > > + pv = period_cycles >> (2 * prescale);
> > > >
> > > > You're assuming that pv <= U32_MAX after this block, right? Then
> > > > maybe
> > > Yes, That is correct.
> > >
> > > >
> > > > if (period_cycles >> (2 * prescale) <= U32_MAX)
> > > >
> > > > is the more intuitive check?
> > >
> > > Ok will add like below, so we support up to (U32_MAX * 1024); Is
> it
> > ok
> > > for you?
> > >
> > > if (!(period_cycles >> (2 * prescale) <= U32_MAX))
> > > + return -EINVAL;
> > > +
> > > + pv = period_cycles >> (2 * prescale);
> >
> > Not -EINVAL, using pv = U32_MAX is correct.
>
> OK.
>
> >
> > > Same case for duty cycle.
> > > >
> > > > > + duty_cycles = mul_u64_u32_div(state->duty_cycle,
> > > > > +rzg2l_gpt->rate, NSEC_PER_SEC);
> > > > > +
> > > > > + if (duty_cycles >= (1024ULL << 32))
> > > > > + dc = U32_MAX;
> > > > > + else
> > > > > + 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);
> > > >
> > > > For v5 I asked if this affects other channels, you said yes and
> in
> > > > the follow up I failed to reply how to improve this.
> > > >
> > > > I wonder how this affects other channels. Does it restart a
> period
> > > > afterwards, or is the effect only that the currently running
> > period
> > > > is a bit stretched?
> > >
> > > If we stops the counter, it resets to starting count position.
> >
> > So if I update pwm#1, pwm#0 doesn't only freeze for a moment, but
> > starts a new period. Hui.
> >
> > > >At least point that this stops the global counter and so affects
> > the
> > > >other PWMs provided by this chip.
> > >
> > > We should not allow Counter to stop if it is running.
> > > We should allow changing mode and prescalar only for the first
> > enabled
> > > channel in Linux.
> > >
> > > Also as per the HW manual, we should not change RZG2L_GTCNT,
> > > RZG2L_GTBER while Counter is running.
> > >
> > > Will add bool is_counter_running to take care of this conditions.
> > >
> > > Is it ok with you?
> >
> > I'm torn here. Resetting the period for the other counter is quite
> > disturbing. If you cannot prevent that, please document that in the
> > Limitations section above.
>
OK, I will document this in limitation section.
* - While using dual channels, both the channels should be enabled and
* disabled at the same time as it uses shared register for controlling
* counter start/stop.
Cheers,
Biju
> >
> > > > > + 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);
> > > >
> > > > This can overflow.
> > >
> > > OK will use inverse calculation to avoid overflow.
> > > mul_u64_u32_div(val << (2 * prescale), NSEC_PER_SEC, rzg2l_gpt-
> > >rate);
> > >
> > > Is it ok?
> >
> > It uses the wrong rounding direction :-\ Using
> >
> > tmp = NSEC_PER_SEC * (u64)val << (2 * prescale);
> >
> > should be enough to fix the problem I pointed out.
>
> OK, Thanks for the pointer.
>
> Cheers,
> Biju
>
> >
> > > > > + state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt-
> > >rate);
> > > > > +
> > > > > + val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm-
> > >hwpwm));
> > > > > + tmp = NSEC_PER_SEC * val << (2 * prescale);
> >
> > Best regards
> > Uwe
> >
> > --
> > Pengutronix e.K. | Uwe Kleine-König
> > |
> > Industrial Linux Solutions |
> > https://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-21 10:50 ` Biju Das
@ 2022-09-21 13:35 ` Uwe Kleine-König
2022-09-21 13:46 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Uwe Kleine-König @ 2022-09-21 13:35 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: 4236 bytes --]
Hello,
On Wed, Sep 21, 2022 at 10:50:48AM +0000, Biju Das wrote:
> > -----Original Message-----
> > From: Biju Das
> > Sent: 20 September 2022 18:01
> > To: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Cc: Thierry Reding <thierry.reding@gmail.com>; Lee Jones
> > <lee.jones@linaro.org>; Philipp Zabel <p.zabel@pengutronix.de>; linux-
> > pwm@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
> > Chris Paterson <Chris.Paterson2@renesas.com>; Biju Das
> > <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> > lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> > Subject: RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> >
> > Hi Uwe,
> >
> > > Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> > >
> > > Hello,
> > >
> > > On Tue, Sep 20, 2022 at 03:31:16PM +0000, Biju Das wrote:
> > > > > On Mon, Sep 05, 2022 at 06:13:28PM +0100, Biju Das wrote:
> > > > > > + if (period_cycles >= (1024ULL << 32))
> > > > > > + pv = U32_MAX;
> > > > > > + else
> > > > > > + pv = period_cycles >> (2 * prescale);
> > > > >
> > > > > You're assuming that pv <= U32_MAX after this block, right? Then
> > > > > maybe
> > > > Yes, That is correct.
> > > >
> > > > >
> > > > > if (period_cycles >> (2 * prescale) <= U32_MAX)
> > > > >
> > > > > is the more intuitive check?
> > > >
> > > > Ok will add like below, so we support up to (U32_MAX * 1024); Is
> > it
> > > ok
> > > > for you?
> > > >
> > > > if (!(period_cycles >> (2 * prescale) <= U32_MAX))
> > > > + return -EINVAL;
> > > > +
> > > > + pv = period_cycles >> (2 * prescale);
> > >
> > > Not -EINVAL, using pv = U32_MAX is correct.
> >
> > OK.
> >
> > >
> > > > Same case for duty cycle.
> > > > >
> > > > > > + duty_cycles = mul_u64_u32_div(state->duty_cycle,
> > > > > > +rzg2l_gpt->rate, NSEC_PER_SEC);
> > > > > > +
> > > > > > + if (duty_cycles >= (1024ULL << 32))
> > > > > > + dc = U32_MAX;
> > > > > > + else
> > > > > > + 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);
> > > > >
> > > > > For v5 I asked if this affects other channels, you said yes and
> > in
> > > > > the follow up I failed to reply how to improve this.
> > > > >
> > > > > I wonder how this affects other channels. Does it restart a
> > period
> > > > > afterwards, or is the effect only that the currently running
> > > period
> > > > > is a bit stretched?
> > > >
> > > > If we stops the counter, it resets to starting count position.
> > >
> > > So if I update pwm#1, pwm#0 doesn't only freeze for a moment, but
> > > starts a new period. Hui.
> > >
> > > > >At least point that this stops the global counter and so affects
> > > the
> > > > >other PWMs provided by this chip.
> > > >
> > > > We should not allow Counter to stop if it is running.
> > > > We should allow changing mode and prescalar only for the first
> > > enabled
> > > > channel in Linux.
> > > >
> > > > Also as per the HW manual, we should not change RZG2L_GTCNT,
> > > > RZG2L_GTBER while Counter is running.
> > > >
> > > > Will add bool is_counter_running to take care of this conditions.
> > > >
> > > > Is it ok with you?
> > >
> > > I'm torn here. Resetting the period for the other counter is quite
> > > disturbing. If you cannot prevent that, please document that in the
> > > Limitations section above.
> >
>
> OK, I will document this in limitation section.
>
> * - While using dual channels, both the channels should be enabled and
> * disabled at the same time as it uses shared register for controlling
> * counter start/stop.
Actually it's worse:
- When both channels are used, setting the duty-cycle on one aborts the
currently running period on the other and starts it anew.
(Did I get this correctly?)
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] 18+ messages in thread
* RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-21 13:35 ` Uwe Kleine-König
@ 2022-09-21 13:46 ` Biju Das
2022-09-22 5:36 ` Uwe Kleine-König
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2022-09-21 13:46 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 v6 2/2] pwm: Add support for RZ/G2L GPT
>
> Hello,
>
> On Wed, Sep 21, 2022 at 10:50:48AM +0000, Biju Das wrote:
> > > -----Original Message-----
> > > From: Biju Das
> > > Sent: 20 September 2022 18:01
> > > To: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Cc: Thierry Reding <thierry.reding@gmail.com>; Lee Jones
> > > <lee.jones@linaro.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> > > linux- pwm@vger.kernel.org; Geert Uytterhoeven
> > > <geert+renesas@glider.be>; Chris Paterson
> > > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>;
> > > Prabhakar Mahadev Lad <prabhakar.mahadev- lad.rj@bp.renesas.com>;
> > > linux-renesas-soc@vger.kernel.org
> > > Subject: RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> > >
> > > Hi Uwe,
> > >
> > > > Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> > > >
> > > > Hello,
> > > >
> > > > On Tue, Sep 20, 2022 at 03:31:16PM +0000, Biju Das wrote:
> > > > > > On Mon, Sep 05, 2022 at 06:13:28PM +0100, Biju Das wrote:
> > > > > > > + if (period_cycles >= (1024ULL << 32))
> > > > > > > + pv = U32_MAX;
> > > > > > > + else
> > > > > > > + pv = period_cycles >> (2 * prescale);
> > > > > >
> > > > > > You're assuming that pv <= U32_MAX after this block, right?
> > > > > > Then maybe
> > > > > Yes, That is correct.
> > > > >
> > > > > >
> > > > > > if (period_cycles >> (2 * prescale) <= U32_MAX)
> > > > > >
> > > > > > is the more intuitive check?
> > > > >
> > > > > Ok will add like below, so we support up to (U32_MAX * 1024);
> Is
> > > it
> > > > ok
> > > > > for you?
> > > > >
> > > > > if (!(period_cycles >> (2 * prescale) <= U32_MAX))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + pv = period_cycles >> (2 * prescale);
> > > >
> > > > Not -EINVAL, using pv = U32_MAX is correct.
> > >
> > > OK.
> > >
> > > >
> > > > > Same case for duty cycle.
> > > > > >
> > > > > > > + duty_cycles = mul_u64_u32_div(state->duty_cycle,
> > > > > > > +rzg2l_gpt->rate, NSEC_PER_SEC);
> > > > > > > +
> > > > > > > + if (duty_cycles >= (1024ULL << 32))
> > > > > > > + dc = U32_MAX;
> > > > > > > + else
> > > > > > > + 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);
> > > > > >
> > > > > > For v5 I asked if this affects other channels, you said yes
> > > > > > and
> > > in
> > > > > > the follow up I failed to reply how to improve this.
> > > > > >
> > > > > > I wonder how this affects other channels. Does it restart a
> > > period
> > > > > > afterwards, or is the effect only that the currently running
> > > > period
> > > > > > is a bit stretched?
> > > > >
> > > > > If we stops the counter, it resets to starting count position.
> > > >
> > > > So if I update pwm#1, pwm#0 doesn't only freeze for a moment,
> but
> > > > starts a new period. Hui.
> > > >
> > > > > >At least point that this stops the global counter and so
> > > > > >affects
> > > > the
> > > > > >other PWMs provided by this chip.
> > > > >
> > > > > We should not allow Counter to stop if it is running.
> > > > > We should allow changing mode and prescalar only for the first
> > > > enabled
> > > > > channel in Linux.
> > > > >
> > > > > Also as per the HW manual, we should not change RZG2L_GTCNT,
> > > > > RZG2L_GTBER while Counter is running.
> > > > >
> > > > > Will add bool is_counter_running to take care of this
> conditions.
> > > > >
> > > > > Is it ok with you?
> > > >
> > > > I'm torn here. Resetting the period for the other counter is
> quite
> > > > disturbing. If you cannot prevent that, please document that in
> > > > the Limitations section above.
> > >
> >
> > OK, I will document this in limitation section.
> >
> > * - While using dual channels, both the channels should be enabled
> and
> > * disabled at the same time as it uses shared register for
> controlling
> > * counter start/stop.
>
> Actually it's worse:
>
> - When both channels are used, setting the duty-cycle on one aborts
> the
> currently running period on the other and starts it anew.
>
> (Did I get this correctly?)
I think, I have fixed that issue with the below logic
Which allows to update duty cycle on the fly.
Now the only limitation is w.r.to disabling channels
as we need to disable together as stopping the counter
affects both.
/*
* Counter must be stopped before modifying mode, prescaler, timer
* counter and buffer enable registers. These registers are shared
* between both channels. So allow updating these registers only for the
* first enabled channel.
*/
if (rzg2l_gpt->user_count <= 1)
rzg2l_gpt_disable(rzg2l_gpt);
is_counter_running = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST;
if (!is_counter_running)
/* 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);
if (!is_counter_running)
/* 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, RZG2L_GTCCR(pwm->hwpwm), dc);
if (!is_counter_running) {
/* 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);
}
Cheers,
Biju
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-21 13:46 ` Biju Das
@ 2022-09-22 5:36 ` Uwe Kleine-König
2022-09-22 6:15 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Uwe Kleine-König @ 2022-09-22 5:36 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: 2124 bytes --]
Hello,
On Wed, Sep 21, 2022 at 01:46:54PM +0000, Biju Das wrote:
> > Actually it's worse:
> >
> > - When both channels are used, setting the duty-cycle on one aborts the
> > currently running period on the other and starts it anew.
> >
> > (Did I get this correctly?)
>
> I think, I have fixed that issue with the below logic
> Which allows to update duty cycle on the fly.
>
> Now the only limitation is w.r.to disabling channels
> as we need to disable together as stopping the counter
> affects both.
>
> /*
> * Counter must be stopped before modifying mode, prescaler, timer
> * counter and buffer enable registers. These registers are shared
> * between both channels. So allow updating these registers only for the
> * first enabled channel.
> */
> if (rzg2l_gpt->user_count <= 1)
> rzg2l_gpt_disable(rzg2l_gpt);
>
> is_counter_running = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST;
> if (!is_counter_running)
> /* 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);
So if the PWM is already running (e.g. from the bootloader) and the mode
is wrong, this isn't fixed? Similar problems in the if blocks below.
> /* Set count direction */
> rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);
>
> if (!is_counter_running)
> /* 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, RZG2L_GTCCR(pwm->hwpwm), dc);
>
> if (!is_counter_running) {
> /* 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);
> }
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] 18+ messages in thread
* RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-22 5:36 ` Uwe Kleine-König
@ 2022-09-22 6:15 ` Biju Das
2022-09-24 10:53 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2022-09-22 6:15 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 v6 2/2] pwm: Add support for RZ/G2L GPT
>
> Hello,
>
> On Wed, Sep 21, 2022 at 01:46:54PM +0000, Biju Das wrote:
> > > Actually it's worse:
> > >
> > > - When both channels are used, setting the duty-cycle on one
> aborts the
> > > currently running period on the other and starts it anew.
> > >
> > > (Did I get this correctly?)
> >
> > I think, I have fixed that issue with the below logic Which allows
> to
> > update duty cycle on the fly.
> >
> > Now the only limitation is w.r.to disabling channels as we need to
> > disable together as stopping the counter affects both.
> >
> > /*
> > * Counter must be stopped before modifying mode, prescaler,
> timer
> > * counter and buffer enable registers. These registers are
> shared
> > * between both channels. So allow updating these registers only
> for the
> > * first enabled channel.
> > */
> > if (rzg2l_gpt->user_count <= 1)
> > rzg2l_gpt_disable(rzg2l_gpt);
> >
> > is_counter_running = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) &
> RZG2L_GTCR_CST;
> > if (!is_counter_running)
> > /* 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);
>
> So if the PWM is already running (e.g. from the bootloader) and the
> mode is wrong, this isn't fixed? Similar problems in the if blocks
> below.
This is taken care by the above code. It stops the counter for first enabled channel in Linux
and then changes the mode as per Linux.
<snippet>
if (rzg2l_gpt->user_count <= 1)
rzg2l_gpt_disable(rzg2l_gpt);
</snippet>
Cheers,
Biju
>
> > /* Set count direction */
> > rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);
> >
> > if (!is_counter_running)
> > /* 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, RZG2L_GTCCR(pwm->hwpwm), dc);
> >
> > if (!is_counter_running) {
> > /* 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);
> > }
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König
> |
> Industrial Linux Solutions |
> https://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-22 6:15 ` Biju Das
@ 2022-09-24 10:53 ` Biju Das
2022-09-24 13:42 ` Uwe Kleine-König
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2022-09-24 10:53 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 v6 2/2] pwm: Add support for RZ/G2L GPT
>
> Hi Uwe,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> >
> > Hello,
> >
> > On Wed, Sep 21, 2022 at 01:46:54PM +0000, Biju Das wrote:
> > > > Actually it's worse:
> > > >
> > > > - When both channels are used, setting the duty-cycle on one
> > aborts the
> > > > currently running period on the other and starts it anew.
> > > >
> > > > (Did I get this correctly?)
> > >
> > > I think, I have fixed that issue with the below logic Which allows
> > to
> > > update duty cycle on the fly.
> > >
> > > Now the only limitation is w.r.to disabling channels as we need to
> > > disable together as stopping the counter affects both.
> > >
> > > /*
> > > * Counter must be stopped before modifying mode, prescaler,
> > timer
> > > * counter and buffer enable registers. These registers are
> > shared
> > > * between both channels. So allow updating these registers only
> > for the
> > > * first enabled channel.
> > > */
> > > if (rzg2l_gpt->user_count <= 1)
> > > rzg2l_gpt_disable(rzg2l_gpt);
> > >
> > > is_counter_running = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) &
> > RZG2L_GTCR_CST;
> > > if (!is_counter_running)
> > > /* 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);
> >
> > So if the PWM is already running (e.g. from the bootloader) and the
> > mode is wrong, this isn't fixed? Similar problems in the if blocks
> > below.
What is your thought on caching the registers that needs counter to be stopped
for updating values. Basically, we don't stop the counter if the values are same?
This allows updating period/duty cycle on the fly without stopping the counter
even for the single channel use case.
Please share your thoughts.
Cheers,
Biju
>
> This is taken care by the above code. It stops the counter for first
> enabled channel in Linux and then changes the mode as per Linux.
>
> <snippet>
> if (rzg2l_gpt->user_count <= 1)
> rzg2l_gpt_disable(rzg2l_gpt);
> </snippet>
>
> Cheers,
> Biju
>
> >
> > > /* Set count direction */
> > > rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);
> > >
> > > if (!is_counter_running)
> > > /* 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, RZG2L_GTCCR(pwm->hwpwm), dc);
> > >
> > > if (!is_counter_running) {
> > > /* 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);
> > > }
> >
> > Best regards
> > Uwe
> >
> > --
> > Pengutronix e.K. | Uwe Kleine-König
> > |
> > Industrial Linux Solutions |
> > https://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-24 10:53 ` Biju Das
@ 2022-09-24 13:42 ` Uwe Kleine-König
2022-09-24 16:10 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Uwe Kleine-König @ 2022-09-24 13:42 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: 2438 bytes --]
Hello Biju,
On Sat, Sep 24, 2022 at 10:53:30AM +0000, Biju Das wrote:
> > Subject: RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> > >
> > > Hello,
> > >
> > > On Wed, Sep 21, 2022 at 01:46:54PM +0000, Biju Das wrote:
> > > > > Actually it's worse:
> > > > >
> > > > > - When both channels are used, setting the duty-cycle on one
> > > aborts the
> > > > > currently running period on the other and starts it anew.
> > > > >
> > > > > (Did I get this correctly?)
> > > >
> > > > I think, I have fixed that issue with the below logic Which allows
> > > to
> > > > update duty cycle on the fly.
> > > >
> > > > Now the only limitation is w.r.to disabling channels as we need to
> > > > disable together as stopping the counter affects both.
> > > >
> > > > /*
> > > > * Counter must be stopped before modifying mode, prescaler,
> > > timer
> > > > * counter and buffer enable registers. These registers are
> > > shared
> > > > * between both channels. So allow updating these registers only
> > > for the
> > > > * first enabled channel.
> > > > */
> > > > if (rzg2l_gpt->user_count <= 1)
> > > > rzg2l_gpt_disable(rzg2l_gpt);
> > > >
> > > > is_counter_running = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) &
> > > RZG2L_GTCR_CST;
> > > > if (!is_counter_running)
> > > > /* 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);
> > >
> > > So if the PWM is already running (e.g. from the bootloader) and the
> > > mode is wrong, this isn't fixed? Similar problems in the if blocks
> > > below.
>
> What is your thought on caching the registers that needs counter to be stopped
> for updating values. Basically, we don't stop the counter if the values are same?
I don't see a very relevant difference between caching and reading the
registers. Whatever is fine for you.
> This allows updating period/duty cycle on the fly without stopping the counter
> even for the single channel use case.
I didn't get the relevant difference, but the result sounds good.
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] 18+ messages in thread
* RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-24 13:42 ` Uwe Kleine-König
@ 2022-09-24 16:10 ` Biju Das
2022-09-26 7:30 ` Geert Uytterhoeven
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2022-09-24 16:10 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 v6 2/2] pwm: Add support for RZ/G2L GPT
>
> Hello Biju,
>
> On Sat, Sep 24, 2022 at 10:53:30AM +0000, Biju Das wrote:
> > > Subject: RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> > > >
> > > > Hello,
> > > >
> > > > On Wed, Sep 21, 2022 at 01:46:54PM +0000, Biju Das wrote:
> > > > > > Actually it's worse:
> > > > > >
> > > > > > - When both channels are used, setting the duty-cycle on one
> > > > aborts the
> > > > > > currently running period on the other and starts it anew.
> > > > > >
> > > > > > (Did I get this correctly?)
> > > > >
> > > > > I think, I have fixed that issue with the below logic Which
> > > > > allows
> > > > to
> > > > > update duty cycle on the fly.
> > > > >
> > > > > Now the only limitation is w.r.to disabling channels as we
> need
> > > > > to disable together as stopping the counter affects both.
> > > > >
> > > > > /*
> > > > > * Counter must be stopped before modifying mode,
> prescaler,
> > > > timer
> > > > > * counter and buffer enable registers. These registers are
> > > > shared
> > > > > * between both channels. So allow updating these registers
> > > > > only
> > > > for the
> > > > > * first enabled channel.
> > > > > */
> > > > > if (rzg2l_gpt->user_count <= 1)
> > > > > rzg2l_gpt_disable(rzg2l_gpt);
> > > > >
> > > > > is_counter_running = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR)
> &
> > > > RZG2L_GTCR_CST;
> > > > > if (!is_counter_running)
> > > > > /* 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);
> > > >
> > > > So if the PWM is already running (e.g. from the bootloader) and
> > > > the mode is wrong, this isn't fixed? Similar problems in the if
> > > > blocks below.
> >
> > What is your thought on caching the registers that needs counter to
> be
> > stopped for updating values. Basically, we don't stop the counter if
> the values are same?
>
> I don't see a very relevant difference between caching and reading the
> registers. Whatever is fine for you.
Ok.
>
> > This allows updating period/duty cycle on the fly without stopping
> the
> > counter even for the single channel use case.
>
> I didn't get the relevant difference, but the result sounds good.
OK. Will send v8, along with any feedback for v7 series [1]
[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220921145741.901784-3-biju.das.jz@bp.renesas.com/
Note:
I have a plan to develop another PWM driver using MTU IP on the same SoC.
The work is not started yet.
For this IP, I planned to use MFD framework for the MTU driver and
Will add counter driver, timer driver(clock source, clock event)
and pwm driver as child devices.
Currently the MFD driver and 16-Bit Phase Counting using counter framework
is almost done.
Changes,
Biju
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-24 16:10 ` Biju Das
@ 2022-09-26 7:30 ` Geert Uytterhoeven
2022-09-26 8:00 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Geert Uytterhoeven @ 2022-09-26 7:30 UTC (permalink / raw)
To: Biju Das
Cc: Uwe Kleine-König, Thierry Reding, Lee Jones, Philipp Zabel,
linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc
Hi Biju,
On Sat, Sep 24, 2022 at 6:10 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Note:
> I have a plan to develop another PWM driver using MTU IP on the same SoC.
> The work is not started yet.
That is the MTU3, which seems to be a further evolution of the MTU2
in e.g. RZ/A1, which is already supported as a timer through the
sh_mtu2 driver?
> For this IP, I planned to use MFD framework for the MTU driver and
> Will add counter driver, timer driver(clock source, clock event)
> and pwm driver as child devices.
>
> Currently the MFD driver and 16-Bit Phase Counting using counter framework
> is almost done.
Do you really need an MFD? (MFDs trigger a red flag for me ;-)
E.g. there are two sets of bindings for renesas,tpu: when #pwm-cells
is present, it is used for PWM, otherwise it is used as a timer.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-26 7:30 ` Geert Uytterhoeven
@ 2022-09-26 8:00 ` Biju Das
2022-09-26 12:16 ` Geert Uytterhoeven
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2022-09-26 8:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Uwe Kleine-König, Thierry Reding, Lee Jones, Philipp Zabel,
linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc, linux-iio
Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
>
> Hi Biju,
>
> On Sat, Sep 24, 2022 at 6:10 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Note:
> > I have a plan to develop another PWM driver using MTU IP on the same
> SoC.
> > The work is not started yet.
>
> That is the MTU3, which seems to be a further evolution of the MTU2 in
> e.g. RZ/A1, which is already supported as a timer through the
> sh_mtu2 driver?
sh_mtu2 is just supports clock events. MTU2 is much powerful and we are
not supporting more advanced features like phase counting(counter framework),
PWM(frame wok) etc...
>
> > For this IP, I planned to use MFD framework for the MTU driver and
> > Will add counter driver, timer driver(clock source, clock event) and
> > pwm driver as child devices.
> >
> > Currently the MFD driver and 16-Bit Phase Counting using counter
> > framework is almost done.
>
> Do you really need an MFD? (MFDs trigger a red flag for me ;-) E.g.
Similar concept is already available in mainline[1].
See STM32 timers where there is an MFD driver supports timer, counter
And pwm as child devices.
[1] https://elixir.bootlin.com/linux/v6.0-rc5/C/ident/TIM_ARR
> there are two sets of bindings for renesas,tpu: when #pwm-cells is
> present, it is used for PWM, otherwise it is used as a timer.
[2]
Yes, we could encapsulate all in PWM. But then we need to call
Other susbsytem from pwm (eg:- counter and timer).
I am not sure, PWM subsystem people allows to call counter and
Timer subsystem calls from pwm driver?? If yes, then that will simplifies a lot.
[3]
I almost have an RFC ready for MFD + 16-bit phase counting mode
Using counter device with MTU3 which is tested on MTU{1,2} channels.
So basically, we need to decide whether to go with approach [2]
Or [3]??
Please share your views, I can post RFC patch to get a clear picture
if needed. Please let me know.
Cheers,
Biju
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
2022-09-26 8:00 ` Biju Das
@ 2022-09-26 12:16 ` Geert Uytterhoeven
0 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2022-09-26 12:16 UTC (permalink / raw)
To: Biju Das
Cc: Uwe Kleine-König, Thierry Reding, Lee Jones, Philipp Zabel,
linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc, linux-iio
Hi Biju,
On Mon, Sep 26, 2022 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> > On Sat, Sep 24, 2022 at 6:10 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > Note:
> > > I have a plan to develop another PWM driver using MTU IP on the same
> > SoC.
> > > The work is not started yet.
> >
> > That is the MTU3, which seems to be a further evolution of the MTU2 in
> > e.g. RZ/A1, which is already supported as a timer through the
> > sh_mtu2 driver?
>
> sh_mtu2 is just supports clock events. MTU2 is much powerful and we are
> not supporting more advanced features like phase counting(counter framework),
> PWM(frame wok) etc...
OK.
> > > For this IP, I planned to use MFD framework for the MTU driver and
> > > Will add counter driver, timer driver(clock source, clock event) and
> > > pwm driver as child devices.
> > >
> > > Currently the MFD driver and 16-Bit Phase Counting using counter
> > > framework is almost done.
> >
> > Do you really need an MFD? (MFDs trigger a red flag for me ;-) E.g.
>
> Similar concept is already available in mainline[1].
> See STM32 timers where there is an MFD driver supports timer, counter
> And pwm as child devices.
>
> [1] https://elixir.bootlin.com/linux/v6.0-rc5/C/ident/TIM_ARR
>
> > there are two sets of bindings for renesas,tpu: when #pwm-cells is
> > present, it is used for PWM, otherwise it is used as a timer.
>
> [2]
> Yes, we could encapsulate all in PWM. But then we need to call
> Other susbsytem from pwm (eg:- counter and timer).
>
> I am not sure, PWM subsystem people allows to call counter and
> Timer subsystem calls from pwm driver?? If yes, then that will simplifies a lot.
>
> [3]
> I almost have an RFC ready for MFD + 16-bit phase counting mode
> Using counter device with MTU3 which is tested on MTU{1,2} channels.
>
> So basically, we need to decide whether to go with approach [2]
> Or [3]??
>
> Please share your views, I can post RFC patch to get a clear picture
> if needed. Please let me know.
I see you've done your homework ;-) OK, fine for me!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-09-26 14:06 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-05 17:13 [PATCH v6 0/2] Add support for RZ/G2L GPT Biju Das
2022-09-05 17:13 ` [PATCH v6 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
2022-09-05 17:13 ` [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT Biju Das
2022-09-19 7:57 ` Uwe Kleine-König
2022-09-20 15:31 ` Biju Das
2022-09-20 15:53 ` Uwe Kleine-König
2022-09-20 17:00 ` Biju Das
2022-09-21 10:50 ` Biju Das
2022-09-21 13:35 ` Uwe Kleine-König
2022-09-21 13:46 ` Biju Das
2022-09-22 5:36 ` Uwe Kleine-König
2022-09-22 6:15 ` Biju Das
2022-09-24 10:53 ` Biju Das
2022-09-24 13:42 ` Uwe Kleine-König
2022-09-24 16:10 ` Biju Das
2022-09-26 7:30 ` Geert Uytterhoeven
2022-09-26 8:00 ` Biju Das
2022-09-26 12:16 ` Geert Uytterhoeven
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).