* [PATCH v7 0/2] Add support for RZ/G2L GPT
@ 2022-09-21 14:57 Biju Das
2022-09-21 14:57 ` [PATCH v7 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
2022-09-21 14:57 ` [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT Biju Das
0 siblings, 2 replies; 16+ messages in thread
From: Biju Das @ 2022-09-21 14:57 UTC (permalink / raw)
To: Thierry Reding, Lee Jones, Rob Herring, Krzysztof Kozlowski,
Philipp Zabel
Cc: Biju Das, Uwe Kleine-König, linux-pwm, devicetree,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc
RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit timer
(GPT32E). It supports the following functions
* 32 bits × 8 channels
* Up-counting or down-counting (saw waves) or up/down-counting
(triangle waves) for each counter.
* Clock sources independently selectable for each channel
* Two I/O pins per channel
* Two output compare/input capture registers per channel
* For the two output compare/input capture registers of each channel,
four registers are provided as buffer registers and are capable of
operating as comparison registers when buffering is not in use.
* In output compare operation, buffer switching can be at crests or
troughs, enabling the generation of laterally asymmetric PWM waveforms.
* Registers for setting up frame cycles in each channel (with capability
for generating interrupts at overflow or underflow)
* Generation of dead times in PWM operation
* Synchronous starting, stopping and clearing counters for arbitrary
channels
* Starting, stopping, clearing and up/down counters in response to input
level comparison
* Starting, clearing, stopping and up/down counters in response to a
maximum of four external triggers
* Output pin disable function by dead time error and detected
short-circuits between output pins
* A/D converter start triggers can be generated (GPT32E0 to GPT32E3)
* Enables the noise filter for input capture and external trigger
operation
This patch series aims to add basic pwm support for RZ/G2L GPT driver
by creating separate logical channels for each IOs.
v6->v7:
* Added the comment for cacheing rzg2l_gpt->state_period.
* Fixed boundary values for pv and dc.
* Added comment for modifying mode, prescaler, timer counter and buffer enable
registers.
* Fixed buffer overflow in get_state()
* Removed unnecessary assignment of state->period value in get_state().
* Fixed state->duty_cycle value in get_state().
* Added a limitation for disabling the channels, when both channels used
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 | 423 ++++++++++++++++++
4 files changed, 564 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] 16+ messages in thread
* [PATCH v7 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding
2022-09-21 14:57 [PATCH v7 0/2] Add support for RZ/G2L GPT Biju Das
@ 2022-09-21 14:57 ` Biju Das
2022-09-21 14:57 ` [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT Biju Das
1 sibling, 0 replies; 16+ messages in thread
From: Biju Das @ 2022-09-21 14:57 UTC (permalink / raw)
To: Thierry Reding, Lee Jones, Rob Herring, Krzysztof Kozlowski
Cc: Biju Das, Uwe Kleine-König, linux-pwm, devicetree,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc, Rob Herring
Add device tree bindings for the General PWM Timer (GPT).
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v6->v7:
* No change.
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] 16+ messages in thread
* [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-21 14:57 [PATCH v7 0/2] Add support for RZ/G2L GPT Biju Das
2022-09-21 14:57 ` [PATCH v7 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
@ 2022-09-21 14:57 ` Biju Das
2022-09-28 13:50 ` Thierry Reding
2022-09-29 17:36 ` Biju Das
1 sibling, 2 replies; 16+ messages in thread
From: Biju Das @ 2022-09-21 14:57 UTC (permalink / raw)
To: Thierry Reding, Lee Jones, Philipp Zabel
Cc: Biju Das, Uwe Kleine-König, linux-pwm, Geert Uytterhoeven,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad,
linux-renesas-soc
RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit timer
(GPT32E). It supports the following functions
* 32 bits × 8 channels
* Up-counting or down-counting (saw waves) or up/down-counting
(triangle waves) for each counter.
* Clock sources independently selectable for each channel
* Two I/O pins per channel
* Two output compare/input capture registers per channel
* For the two output compare/input capture registers of each channel,
four registers are provided as buffer registers and are capable of
operating as comparison registers when buffering is not in use.
* In output compare operation, buffer switching can be at crests or
troughs, enabling the generation of laterally asymmetric PWM waveforms.
* Registers for setting up frame cycles in each channel (with capability
for generating interrupts at overflow or underflow)
* Generation of dead times in PWM operation
* Synchronous starting, stopping and clearing counters for arbitrary
channels
* Starting, stopping, clearing and up/down counters in response to input
level comparison
* Starting, clearing, stopping and up/down counters in response to a
maximum of four external triggers
* Output pin disable function by dead time error and detected
short-circuits between output pins
* A/D converter start triggers can be generated (GPT32E0 to GPT32E3)
* Enables the noise filter for input capture and external trigger
operation
This patch adds basic pwm support for RZ/G2L GPT driver by creating
separate logical channels for each IOs.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v6->v7:
* Added the comment for cacheing rzg2l_gpt->state_period.
* Fixed boundary values for pv and dc.
* Added comment for modifying mode, prescaler, timer counter and buffer enable
registers.
* Fixed buffer overflow in get_state()
* Removed unnecessary assignment of state->period value in get_state().
* Fixed state->duty_cycle value in get_state().
* Added a limitation for disabling the channels, when both channels used.
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 | 423 ++++++++++++++++++++++++++++++++++++
3 files changed, 435 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..c4e011f2a843
--- /dev/null
+++ b/drivers/pwm/pwm-rzg2l-gpt.c
@@ -0,0 +1,423 @@
+// 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.
+ * - When both channels are used, disabling the channel on one stops the
+ * other.
+ */
+
+#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);
+ bool is_counter_running;
+ 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;
+
+ /*
+ * GPT counter is shared by multiple channels, we cache the period value
+ * from the first enabled channel and use the same value for both
+ * channels.
+ */
+ 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 >> (2 * prescale) <= U32_MAX)
+ pv = period_cycles >> (2 * prescale);
+ else
+ pv = U32_MAX;
+
+ duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate, NSEC_PER_SEC);
+
+ if (duty_cycles >> (2 * prescale) <= U32_MAX)
+ dc = duty_cycles >> (2 * prescale);
+ else
+ dc = U32_MAX;
+
+ /*
+ * 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);
+ }
+
+ /* 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;
+
+ 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 * (u64)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 * (u64)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, the pwm_request_from_chip() calling get_state()
+ * will have an invalid duty cycle value as the register is not
+ * initialized yet.
+ */
+ if (state->duty_cycle > state->period)
+ state->duty_cycle = state->period;
+ }
+
+ 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] 16+ messages in thread
* Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-21 14:57 ` [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT Biju Das
@ 2022-09-28 13:50 ` Thierry Reding
2022-09-28 14:09 ` Geert Uytterhoeven
2022-09-28 17:34 ` Biju Das
2022-09-29 17:36 ` Biju Das
1 sibling, 2 replies; 16+ messages in thread
From: Thierry Reding @ 2022-09-28 13:50 UTC (permalink / raw)
To: Biju Das
Cc: Lee Jones, Philipp Zabel, Uwe Kleine-König, linux-pwm,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 11059 bytes --]
On Wed, Sep 21, 2022 at 03:57:41PM +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>
> ---
> v6->v7:
> * Added the comment for cacheing rzg2l_gpt->state_period.
> * Fixed boundary values for pv and dc.
> * Added comment for modifying mode, prescaler, timer counter and buffer enable
> registers.
> * Fixed buffer overflow in get_state()
> * Removed unnecessary assignment of state->period value in get_state().
> * Fixed state->duty_cycle value in get_state().
> * Added a limitation for disabling the channels, when both channels used.
> 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 | 423 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 435 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..c4e011f2a843
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c
[...]
> +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);
> +}
Do you really need to support PCI I/O space in this driver? If not, make
this writel() and readl() instead.
[...]
> +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.
> + */
Where does this happen? I'm not aware of any code that would
automatically enable clocks for runtime PM. Typically you would need to
implement driver-specific callbacks for that to happen.
> + 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);
So in either case I would expect you to want to hold on to the clock
pointer here and use that in the runtime PM callbacks.
This whole business of keeping a separate variable to track this also
seems a bit fishy to me because it only partially reflects in the
software state what's really going on. So I think what you really want
here is to call pm_runtime_set_active() before pm_runtime_enable() to
make sure that your device is marked as such.
I wonder if that alone wouldn't already solve this problem. IIRC, the
runtime PM infrastructure will consider a device to be runtime suspended
after ->probe() by default. And if the clock is indeed managed by
runtime PM somehow, then that might just cause it to get disabled.
Again, it'd be great to know how exactly runtime PM knows how to manage
the clock if you don't tell it here. Is the clock perhaps shared between
multiple IPs? Perhaps a parent device that managed it in runtime PM?
> +
> + mutex_init(&rzg2l_gpt->lock);
> +
> + rzg2l_gpt->chip.dev = &pdev->dev;
> + rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> + rzg2l_gpt->chip.npwm = 2;
The changelog above mentions that you use a shared reset because the
reset is shared between 8 channels, but here you only expose 2. What's
going on there?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-28 13:50 ` Thierry Reding
@ 2022-09-28 14:09 ` Geert Uytterhoeven
2022-09-28 17:34 ` Biju Das
1 sibling, 0 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2022-09-28 14:09 UTC (permalink / raw)
To: Thierry Reding
Cc: Biju Das, Lee Jones, Philipp Zabel, Uwe Kleine-König,
linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc
Hi Thierry,
On Wed, Sep 28, 2022 at 3:50 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Sep 21, 2022 at 03:57:41PM +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>
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > +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.
> > + */
>
> Where does this happen? I'm not aware of any code that would
> automatically enable clocks for runtime PM. Typically you would need to
> implement driver-specific callbacks for that to happen.
>
> > + 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);
>
> So in either case I would expect you to want to hold on to the clock
> pointer here and use that in the runtime PM callbacks.
>
> This whole business of keeping a separate variable to track this also
> seems a bit fishy to me because it only partially reflects in the
> software state what's really going on. So I think what you really want
> here is to call pm_runtime_set_active() before pm_runtime_enable() to
> make sure that your device is marked as such.
>
> I wonder if that alone wouldn't already solve this problem. IIRC, the
> runtime PM infrastructure will consider a device to be runtime suspended
> after ->probe() by default. And if the clock is indeed managed by
> runtime PM somehow, then that might just cause it to get disabled.
> Again, it'd be great to know how exactly runtime PM knows how to manage
> the clock if you don't tell it here. Is the clock perhaps shared between
> multiple IPs? Perhaps a parent device that managed it in runtime PM?
It's handled by the clock domain code in the PM Domain framework,
cfr. GENPD_FLAG_PM_CLK. All members of the PM Domain have
their module clock(s) managed automatically through Runtime PM.
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] 16+ messages in thread
* RE: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-28 13:50 ` Thierry Reding
2022-09-28 14:09 ` Geert Uytterhoeven
@ 2022-09-28 17:34 ` Biju Das
2022-09-28 18:07 ` Geert Uytterhoeven
2022-09-29 9:48 ` Thierry Reding
1 sibling, 2 replies; 16+ messages in thread
From: Biju Das @ 2022-09-28 17:34 UTC (permalink / raw)
To: Thierry Reding
Cc: Lee Jones, Philipp Zabel, Uwe Kleine-König, linux-pwm,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc
Hi Thierry Reding,
Thanks for the feedback.
> Subject: Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
>
> On Wed, Sep 21, 2022 at 03:57:41PM +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>
> > ---
> > v6->v7:
> > * Added the comment for cacheing rzg2l_gpt->state_period.
> > * Fixed boundary values for pv and dc.
> > * Added comment for modifying mode, prescaler, timer counter and
> buffer enable
> > registers.
> > * Fixed buffer overflow in get_state()
> > * Removed unnecessary assignment of state->period value in
> get_state().
> > * Fixed state->duty_cycle value in get_state().
> > * Added a limitation for disabling the channels, when both channels
> used.
> > 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 | 423
> > ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 435 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..c4e011f2a843
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> [...]
> > +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); }
>
> Do you really need to support PCI I/O space in this driver? If not,
> make this writel() and readl() instead.
Agreed.
>
> [...]
> > +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.
> > + */
>
> Where does this happen? I'm not aware of any code that would
> automatically enable clocks for runtime PM. Typically you would need
> to implement driver-specific callbacks for that to happen.
As we are using clock domain in the PM framework and supports multiple clocks
in clock domain, a single call to pm calls such as pm_runtime_get_{sync, resume)
enables multiple clocks on each device.
>
> > + 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);
>
> So in either case I would expect you to want to hold on to the clock
> pointer here and use that in the runtime PM callbacks.
But the api used here is "devm_clk_get_enabled".
This will enable the clocks and holds the reference
(for pwm enabled by bootloader case) as it avoids turning "off"
the clock during later part of the boot process
(it prevents clock off by clk_disable_unused())
On the other case, this api will simply waste power, so we need to
disable and remove the clk reference.
>
> This whole business of keeping a separate variable to track this also
> seems a bit fishy to me because it only partially reflects in the
> software state what's really going on. So I think what you really want
> here is to call pm_runtime_set_active() before pm_runtime_enable() to
> make sure that your device is marked as such.
>
I can add pm_runtime_set_active() followed by pm_runtime_enable() in the
next version( This will make RPM_ACTIVE active state, But clock is not "on"
as we did not call pm_runtime_get_{sync, resume) to change it to RPM_RESUME state)
> I wonder if that alone wouldn't already solve this problem. IIRC, the
> runtime PM infrastructure will consider a device to be runtime
> suspended after ->probe() by default. And if the clock is indeed
> managed by runtime PM somehow, then that might just cause it to get
> disabled.
Yes, that won't solve.
Here we have 2 cases in probe,
in 1 case clk to be "on" after probe(PM RESUME STATE) --> pwm enabled by bootloader
and other case clk to be "off" after probe(PM SUSPENDING state) --> pwm disabled by bootloader
> Again, it'd be great to know how exactly runtime PM knows how to
> manage the clock if you don't tell it here.
pm_runtime_get_{sync, resume) will change the state to RPM_RESUME
and
pm_runtime_put will change the state to RPM_SUSPENDING.
Please see get_state() and apply().
> Is the clock perhaps
> shared between multiple IPs? Perhaps a parent device that managed it
> in runtime PM?
No, it doesn't shared.
Currently we need to take care of the 2 cases in probe as mentioned above
1) pwm enabled by bootloader:-
we are calling "devm_clk_get_enabled" which will hold
reference of enabled Clk( this will prevent clk_disable_unused() turning off the clocks
during late init)
2) pwm disabled by bootloader:-
Clock is off during probe, but in apply state we will turn it on by calling
pm_runtime_get_sync.
>
> > +
> > + mutex_init(&rzg2l_gpt->lock);
> > +
> > + rzg2l_gpt->chip.dev = &pdev->dev;
> > + rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > + rzg2l_gpt->chip.npwm = 2;
>
> The changelog above mentions that you use a shared reset because the
> reset is shared between 8 channels, but here you only expose 2. What's
> going on there?
Each hwchannel has 2 IOs. Each IO is modelled as separate channel.
Basically, we have 8 hwchannels * 2 IO's = 16 pwm channels in total.
Please correct me if anything wrong here.
Cheers,
Biju
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-28 17:34 ` Biju Das
@ 2022-09-28 18:07 ` Geert Uytterhoeven
2022-09-28 19:21 ` Biju Das
2022-09-29 9:48 ` Thierry Reding
1 sibling, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2022-09-28 18:07 UTC (permalink / raw)
To: Biju Das
Cc: Thierry Reding, Lee Jones, Philipp Zabel, Uwe Kleine-König,
linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc
Hi Biju,
On Wed, Sep 28, 2022 at 7:34 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > On Wed, Sep 21, 2022 at 03:57:41PM +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>
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > > + 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);
> >
> > So in either case I would expect you to want to hold on to the clock
> > pointer here and use that in the runtime PM callbacks.
>
> But the api used here is "devm_clk_get_enabled".
> This will enable the clocks and holds the reference
> (for pwm enabled by bootloader case) as it avoids turning "off"
> the clock during later part of the boot process
> (it prevents clock off by clk_disable_unused())
The clock will still be stopped if the driver is unloaded, or if the device
is unbound manually, right? Or am I missing something?
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] 16+ messages in thread
* RE: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-28 18:07 ` Geert Uytterhoeven
@ 2022-09-28 19:21 ` Biju Das
0 siblings, 0 replies; 16+ messages in thread
From: Biju Das @ 2022-09-28 19:21 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Thierry Reding, Lee Jones, Philipp Zabel, Uwe Kleine-König,
linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc
Hi Geert,
> Subject: Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
>
> Hi Biju,
>
> On Wed, Sep 28, 2022 at 7:34 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > On Wed, Sep 21, 2022 at 03:57:41PM +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>
>
> > > > --- /dev/null
> > > > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > > > + 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);
> > >
> > > So in either case I would expect you to want to hold on to the
> clock
> > > pointer here and use that in the runtime PM callbacks.
> >
> > But the api used here is "devm_clk_get_enabled".
> > This will enable the clocks and holds the reference (for pwm enabled
> > by bootloader case) as it avoids turning "off"
> > the clock during later part of the boot process (it prevents clock
> off
> > by clk_disable_unused())
>
> The clock will still be stopped if the driver is unloaded, or if the
> device is unbound manually, right? Or am I missing something?
Yes, it is turned off during unloading.
root@smarc-rzg2l:~# devmem2 0x11010540
/dev/mem opened.
Memory mapped at address 0xffff81d93000.
Read at address 0x11010540 (0xffff81d93540): 0x00000001
root@smarc-rzg2l:~# lsmod
Module Size Used by
rzg2l_mtu3_cnt 16384 0
counter 28672 1 rzg2l_mtu3_cnt
crct10dif_ce 20480 1
snd_soc_simple_card 20480 0
snd_soc_simple_card_utils 24576 1 snd_soc_simple_card
panfrost 69632 0
drm_shmem_helper 28672 1 panfrost
renesas_usbhs 65536 0
gpu_sched 40960 1 panfrost
drm 561152 4 gpu_sched,drm_shmem_helper,panfrost
rcar_canfd 36864 0
renesas_rpc_if 20480 0
snd_soc_wm8978 45056 1
snd_soc_rz_ssi 24576 1
rzg2l_adc 24576 0
can_dev 40960 1 rcar_canfd
rzg2l_poeg 20480 0
rzg2l_mtu3 16384 1 rzg2l_mtu3_cnt
pwm_rzg2l_gpt 16384 1 rzg2l_poeg
ipv6 458752 18
root@smarc-rzg2l:~# rmmod rzg2l_poeg
root@smarc-rzg2l:~# rmmod pwm_rzg2l_gpt
root@smarc-rzg2l:~# devmem2 0x11010540
/dev/mem opened.
Memory mapped at address 0xffffa6e6d000.
Read at address 0x11010540 (0xffffa6e6d540): 0x00000000
Cheers,
Biju
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-28 17:34 ` Biju Das
2022-09-28 18:07 ` Geert Uytterhoeven
@ 2022-09-29 9:48 ` Thierry Reding
2022-10-01 14:10 ` Biju Das
2022-10-11 11:53 ` Biju Das
1 sibling, 2 replies; 16+ messages in thread
From: Thierry Reding @ 2022-09-29 9:48 UTC (permalink / raw)
To: Biju Das
Cc: Lee Jones, Philipp Zabel, Uwe Kleine-König, linux-pwm,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 16288 bytes --]
On Wed, Sep 28, 2022 at 05:34:49PM +0000, Biju Das wrote:
> Hi Thierry Reding,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
> >
> > On Wed, Sep 21, 2022 at 03:57:41PM +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>
> > > ---
> > > v6->v7:
> > > * Added the comment for cacheing rzg2l_gpt->state_period.
> > > * Fixed boundary values for pv and dc.
> > > * Added comment for modifying mode, prescaler, timer counter and
> > buffer enable
> > > registers.
> > > * Fixed buffer overflow in get_state()
> > > * Removed unnecessary assignment of state->period value in
> > get_state().
> > > * Fixed state->duty_cycle value in get_state().
> > > * Added a limitation for disabling the channels, when both channels
> > used.
> > > 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 | 423
> > > ++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 435 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..c4e011f2a843
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > [...]
> > > +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); }
> >
> > Do you really need to support PCI I/O space in this driver? If not,
> > make this writel() and readl() instead.
>
> Agreed.
>
> >
> > [...]
> > > +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.
> > > + */
> >
> > Where does this happen? I'm not aware of any code that would
> > automatically enable clocks for runtime PM. Typically you would need
> > to implement driver-specific callbacks for that to happen.
>
> As we are using clock domain in the PM framework and supports multiple clocks
> in clock domain, a single call to pm calls such as pm_runtime_get_{sync, resume)
> enables multiple clocks on each device.
>
> >
> > > + 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);
> >
> > So in either case I would expect you to want to hold on to the clock
> > pointer here and use that in the runtime PM callbacks.
>
> But the api used here is "devm_clk_get_enabled".
> This will enable the clocks and holds the reference
> (for pwm enabled by bootloader case) as it avoids turning "off"
> the clock during later part of the boot process
> (it prevents clock off by clk_disable_unused())
>
> On the other case, this api will simply waste power, so we need to
> disable and remove the clk reference.
>
> >
> > This whole business of keeping a separate variable to track this also
> > seems a bit fishy to me because it only partially reflects in the
> > software state what's really going on. So I think what you really want
> > here is to call pm_runtime_set_active() before pm_runtime_enable() to
> > make sure that your device is marked as such.
> >
>
> I can add pm_runtime_set_active() followed by pm_runtime_enable() in the
> next version( This will make RPM_ACTIVE active state, But clock is not "on"
> as we did not call pm_runtime_get_{sync, resume) to change it to RPM_RESUME state)
>
> > I wonder if that alone wouldn't already solve this problem. IIRC, the
> > runtime PM infrastructure will consider a device to be runtime
> > suspended after ->probe() by default. And if the clock is indeed
> > managed by runtime PM somehow, then that might just cause it to get
> > disabled.
>
> Yes, that won't solve.
>
> Here we have 2 cases in probe,
>
> in 1 case clk to be "on" after probe(PM RESUME STATE) --> pwm enabled by bootloader
> and other case clk to be "off" after probe(PM SUSPENDING state) --> pwm disabled by bootloader
>
> > Again, it'd be great to know how exactly runtime PM knows how to
> > manage the clock if you don't tell it here.
>
> pm_runtime_get_{sync, resume) will change the state to RPM_RESUME
> and
> pm_runtime_put will change the state to RPM_SUSPENDING.
>
> Please see get_state() and apply().
>
> > Is the clock perhaps
> > shared between multiple IPs? Perhaps a parent device that managed it
> > in runtime PM?
>
> No, it doesn't shared.
>
> Currently we need to take care of the 2 cases in probe as mentioned above
>
> 1) pwm enabled by bootloader:-
>
> we are calling "devm_clk_get_enabled" which will hold
> reference of enabled Clk( this will prevent clk_disable_unused() turning off the clocks
> during late init)
>
> 2) pwm disabled by bootloader:-
>
> Clock is off during probe, but in apply state we will turn it on by calling
> pm_runtime_get_sync.
I understand what and why you're doing this, but I think you're doing
this at the wrong level of abstraction. And that's what's leading to
this strange construct where you need to manually fiddle with the clock.
If you want to abstract all of this behind PM domains, then this should
be reflected properly within those PM domains.
That is, if the PWM has been enabled by the bootloader, then effectively
it's had to enable the PM domain as well (at least conceptually, even if
the bootloader may not have the same objects as Linux). So you need to
find a way to mark the PM domain as enabled as a whole rather than just
one of the clocks that happens to be part of it.
Remember, the purpose of abstraction is to hide the implementation
details (i.e. the clock). You break that abstraction if you go and
fiddle with the implementation details.
> > > + mutex_init(&rzg2l_gpt->lock);
> > > +
> > > + rzg2l_gpt->chip.dev = &pdev->dev;
> > > + rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > > + rzg2l_gpt->chip.npwm = 2;
> >
> > The changelog above mentions that you use a shared reset because the
> > reset is shared between 8 channels, but here you only expose 2. What's
> > going on there?
>
> Each hwchannel has 2 IOs. Each IO is modelled as separate channel.
> Basically, we have 8 hwchannels * 2 IO's = 16 pwm channels in total.
>
> Please correct me if anything wrong here.
I'm asking because I recently came across a similar driver but where the
mistake was made to describe the hardware as 4 separate devices with 2
channels (or, depending on SoC generation, 1 channel) per device.
Looking at the device tree it's pretty obvious that in that case this
was really a single device with 8 (or 4, depending) channels. Most of
the time this doesn't matter and everything works, but then on some HW
generations all of a sudden you have a shared interrupt for all 8
channels, and now this becomes really difficult to describe because the
interrupt needs to be shared between 4 devices, or an extra layer is
needed to slot in the interrupt somehow.
So I'm just trying to avoid another such situation. Looking at the DTS
example from the binding in patch 1, you have gpt4 at 0x10048400 with
0x100 bytes. Does gpt3 sit at 0x10048300 with 0x100 bytes? If so, it's
likely that this is really a single large IP block that you're
artificially subdividing and that could turn into a similar issue as
above.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-21 14:57 ` [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT Biju Das
2022-09-28 13:50 ` Thierry Reding
@ 2022-09-29 17:36 ` Biju Das
2022-09-30 6:34 ` Uwe Kleine-König
1 sibling, 1 reply; 16+ messages in thread
From: Biju Das @ 2022-09-29 17:36 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc, Thierry Reding,
Lee Jones, Philipp Zabel
Hi Uwe,
> Subject: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
>
> 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>
> ---
> v6->v7:
> * Added the comment for cacheing rzg2l_gpt->state_period.
> * Fixed boundary values for pv and dc.
> * Added comment for modifying mode, prescaler, timer counter and
> buffer enable
> registers.
> * Fixed buffer overflow in get_state()
> * Removed unnecessary assignment of state->period value in
> get_state().
> * Fixed state->duty_cycle value in get_state().
> * Added a limitation for disabling the channels, when both channels
> used.
> 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 | 423
> ++++++++++++++++++++++++++++++++++++
> 3 files changed, 435 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..c4e011f2a843
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> @@ -0,0 +1,423 @@
> +// 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-
> use
> +rs-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.
> + * - When both channels are used, disabling the channel on one stops
> the
> + * other.
> + */
> +
> +#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;
This algorithm won't give desired result.
prescaled_period_cycles Expected result
0 ->0
1..3 ->1
4..15 ->2
16..63 ->3
64..255 ->4
256 > ->5
I believe we need a for loop like in patch[4] to get the desired result.
Please correct me if you think otherwise.
[4] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220728162526.330542-3-biju.das.jz@bp.renesas.com/
Cheers,
Biju
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-29 17:36 ` Biju Das
@ 2022-09-30 6:34 ` Uwe Kleine-König
2022-09-30 6:51 ` Biju Das
0 siblings, 1 reply; 16+ messages in thread
From: Uwe Kleine-König @ 2022-09-30 6:34 UTC (permalink / raw)
To: Biju Das
Cc: linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc, Thierry Reding,
Lee Jones, Philipp Zabel
[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]
Hello Biju,
On Thu, Sep 29, 2022 at 05:36:38PM +0000, Biju Das wrote:
> > +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;
>
>
> This algorithm won't give desired result.
>
> prescaled_period_cycles Expected result
> 0 ->0
> 1..3 ->1
> 4..15 ->2
> 16..63 ->3
> 64..255 ->4
> 256 > ->5
Oh, indeed, it fails for prescaled_period_cycles ∈ { 0, 3, 15, 63, 255 }.
The correct formula is:
if (prescaled_period_cycles >= 256)
prescale = 5;
else
prescale = (roundup_pow_of_two(prescaled_period_cycles) + 1) / 2;
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] 16+ messages in thread
* RE: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-30 6:34 ` Uwe Kleine-König
@ 2022-09-30 6:51 ` Biju Das
2022-09-30 7:03 ` Uwe Kleine-König
0 siblings, 1 reply; 16+ messages in thread
From: Biju Das @ 2022-09-30 6:51 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc, Thierry Reding,
Lee Jones, Philipp Zabel
Hi Uwe,
> Subject: Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
>
> Hello Biju,
>
> On Thu, Sep 29, 2022 at 05:36:38PM +0000, Biju Das wrote:
> > > +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;
> >
> >
> > This algorithm won't give desired result.
> >
> > prescaled_period_cycles Expected result
> > 0 ->0
> > 1..3 ->1
> > 4..15 ->2
> > 16..63 ->3
> > 64..255 ->4
> > 256 > ->5
>
> Oh, indeed, it fails for prescaled_period_cycles ∈ { 0, 3, 15, 63, 255
> }.
>
> The correct formula is:
>
> if (prescaled_period_cycles >= 256)
> prescale = 5;
> else
> prescale = (roundup_pow_of_two(prescaled_period_cycles) +
> 1) / 2;
>
Round_pow_of_two(n) --> n=0 is not acceptable
Round_pow_of_two(58)--> 64 as per the above formula, it becomes 64 + 1 /2 = 32
Which is very high value.
I am sure since the expected result is log4, we don't get the desired result with
The above equation. Please correct me, if I am wrong.
0
100
10000
1000000
100000000
Cheers,
Biju
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-30 6:51 ` Biju Das
@ 2022-09-30 7:03 ` Uwe Kleine-König
2022-09-30 7:43 ` Biju Das
0 siblings, 1 reply; 16+ messages in thread
From: Uwe Kleine-König @ 2022-09-30 7:03 UTC (permalink / raw)
To: Biju Das
Cc: linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc, Thierry Reding,
Lee Jones, Philipp Zabel
[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]
On Fri, Sep 30, 2022 at 06:51:12AM +0000, Biju Das wrote:
> Hi Uwe,
>
> > Subject: Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
> >
> > Hello Biju,
> >
> > On Thu, Sep 29, 2022 at 05:36:38PM +0000, Biju Das wrote:
> > > > +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;
> > >
> > >
> > > This algorithm won't give desired result.
> > >
> > > prescaled_period_cycles Expected result
> > > 0 ->0
> > > 1..3 ->1
> > > 4..15 ->2
> > > 16..63 ->3
> > > 64..255 ->4
> > > 256 > ->5
> >
> > Oh, indeed, it fails for prescaled_period_cycles ∈ { 0, 3, 15, 63, 255
> > }.
> >
> > The correct formula is:
> >
> > if (prescaled_period_cycles >= 256)
> > prescale = 5;
> > else
> > prescale = (roundup_pow_of_two(prescaled_period_cycles) +
> > 1) / 2;
> >
>
> Round_pow_of_two(n) --> n=0 is not acceptable
>
> Round_pow_of_two(58)--> 64 as per the above formula, it becomes 64 + 1 /2 = 32
> Which is very high value.
Oh, I translated my (Python) prototype wrongly to Kernel-C, please make
this:
if (prescaled_period_cycles >= 256)
prescale = 5;
else
prescale = (fls(prescaled_period_cycles) + 1) / 2;
With fls(58) = 6 the result is 3 as intended.
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] 16+ messages in thread
* RE: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-30 7:03 ` Uwe Kleine-König
@ 2022-09-30 7:43 ` Biju Das
0 siblings, 0 replies; 16+ messages in thread
From: Biju Das @ 2022-09-30 7:43 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc, Thierry Reding,
Lee Jones, Philipp Zabel
Hi Uwe,
Jones <lee.jones@linaro.org>; Philipp
> Zabel <p.zabel@pengutronix.de>
> Subject: Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
>
> On Fri, Sep 30, 2022 at 06:51:12AM +0000, Biju Das wrote:
> > Hi Uwe,
> >
> > > Subject: Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
> > >
> > > Hello Biju,
> > >
> > > On Thu, Sep 29, 2022 at 05:36:38PM +0000, Biju Das wrote:
> > > > > +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;
> > > >
> > > >
> > > > This algorithm won't give desired result.
> > > >
> > > > prescaled_period_cycles Expected result
> > > > 0 ->0
> > > > 1..3 ->1
> > > > 4..15 ->2
> > > > 16..63 ->3
> > > > 64..255 ->4
> > > > 256 > ->5
> > >
> > > Oh, indeed, it fails for prescaled_period_cycles ∈ { 0, 3, 15, 63,
> > > 255 }.
> > >
> > > The correct formula is:
> > >
> > > if (prescaled_period_cycles >= 256)
> > > prescale = 5;
> > > else
> > > prescale = (roundup_pow_of_two(prescaled_period_cycles) +
> > > 1) / 2;
> > >
> >
> > Round_pow_of_two(n) --> n=0 is not acceptable
> >
> > Round_pow_of_two(58)--> 64 as per the above formula, it becomes 64
> +
> > 1 /2 = 32 Which is very high value.
>
> Oh, I translated my (Python) prototype wrongly to Kernel-C, please
> make
> this:
>
> if (prescaled_period_cycles >= 256)
> prescale = 5;
> else
> prescale = (fls(prescaled_period_cycles) + 1) / 2;
>
> With fls(58) = 6 the result is 3 as intended.
Thanks.
Tested on target and results are ok now.
+ for (i =0 ; i < 256; i++) {
+ pr_info("####### %s ####%u/%u\n",__func__,i,(fls(i) + 1) / 2);
+ }
[ 34.740902] ####### rzg2l_gpt_calculate_prescale ####0/0
[ 34.759151] ####### rzg2l_gpt_calculate_prescale ####3/1
[ 34.825619] ####### rzg2l_gpt_calculate_prescale ####15/2
[ 35.094655] ####### rzg2l_gpt_calculate_prescale ####63/3
[ 36.184126] ####### rzg2l_gpt_calculate_prescale ####255/4
Cheers,
Biju
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-29 9:48 ` Thierry Reding
@ 2022-10-01 14:10 ` Biju Das
2022-10-11 11:53 ` Biju Das
1 sibling, 0 replies; 16+ messages in thread
From: Biju Das @ 2022-10-01 14:10 UTC (permalink / raw)
To: Thierry Reding
Cc: Lee Jones, Philipp Zabel, Uwe Kleine-König, linux-pwm,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc, Rob Herring
Hi Thierry Reding,
+ Rob H.
Thanks for feedback.
> Subject: Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
>
> On Wed, Sep 28, 2022 at 05:34:49PM +0000, Biju Das wrote:
> > Hi Thierry Reding,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
> > >
> > > On Wed, Sep 21, 2022 at 03:57:41PM +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>
> > > > ---
> > > > v6->v7:
> > > > * Added the comment for cacheing rzg2l_gpt->state_period.
> > > > * Fixed boundary values for pv and dc.
> > > > * Added comment for modifying mode, prescaler, timer counter
> and
> > > buffer enable
> > > > registers.
> > > > * Fixed buffer overflow in get_state()
> > > > * Removed unnecessary assignment of state->period value in
> > > get_state().
> > > > * Fixed state->duty_cycle value in get_state().
> > > > * Added a limitation for disabling the channels, when both
> > > > channels
> > > used.
> > > > 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 | 423
> > > > ++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 435 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..c4e011f2a843
> > > > --- /dev/null
> > > > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > > [...]
> > > > +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); }
> > >
> > > Do you really need to support PCI I/O space in this driver? If
> not,
> > > make this writel() and readl() instead.
> >
> > Agreed.
> >
> > >
> > > [...]
> > > > +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.
> > > > + */
> > >
> > > Where does this happen? I'm not aware of any code that would
> > > automatically enable clocks for runtime PM. Typically you would
> need
> > > to implement driver-specific callbacks for that to happen.
> >
> > As we are using clock domain in the PM framework and supports
> multiple
> > clocks in clock domain, a single call to pm calls such as
> > pm_runtime_get_{sync, resume) enables multiple clocks on each
> device.
> >
> > >
> > > > + 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);
> > >
> > > So in either case I would expect you to want to hold on to the
> clock
> > > pointer here and use that in the runtime PM callbacks.
> >
> > But the api used here is "devm_clk_get_enabled".
> > This will enable the clocks and holds the reference (for pwm enabled
> > by bootloader case) as it avoids turning "off"
> > the clock during later part of the boot process (it prevents clock
> off
> > by clk_disable_unused())
> >
> > On the other case, this api will simply waste power, so we need to
> > disable and remove the clk reference.
> >
> > >
> > > This whole business of keeping a separate variable to track this
> > > also seems a bit fishy to me because it only partially reflects in
> > > the software state what's really going on. So I think what you
> > > really want here is to call pm_runtime_set_active() before
> > > pm_runtime_enable() to make sure that your device is marked as
> such.
> > >
> >
> > I can add pm_runtime_set_active() followed by pm_runtime_enable() in
> > the next version( This will make RPM_ACTIVE active state, But clock
> is not "on"
> > as we did not call pm_runtime_get_{sync, resume) to change it to
> > RPM_RESUME state)
> >
> > > I wonder if that alone wouldn't already solve this problem. IIRC,
> > > the runtime PM infrastructure will consider a device to be runtime
> > > suspended after ->probe() by default. And if the clock is indeed
> > > managed by runtime PM somehow, then that might just cause it to
> get
> > > disabled.
> >
> > Yes, that won't solve.
> >
> > Here we have 2 cases in probe,
> >
> > in 1 case clk to be "on" after probe(PM RESUME STATE) --> pwm
> enabled
> > by bootloader and other case clk to be "off" after probe(PM
> SUSPENDING
> > state) --> pwm disabled by bootloader
> >
> > > Again, it'd be great to know how exactly runtime PM knows how to
> > > manage the clock if you don't tell it here.
> >
> > pm_runtime_get_{sync, resume) will change the state to RPM_RESUME
> and
> > pm_runtime_put will change the state to RPM_SUSPENDING.
> >
> > Please see get_state() and apply().
> >
> > > Is the clock perhaps
> > > shared between multiple IPs? Perhaps a parent device that managed
> it
> > > in runtime PM?
> >
> > No, it doesn't shared.
> >
> > Currently we need to take care of the 2 cases in probe as mentioned
> > above
> >
> > 1) pwm enabled by bootloader:-
> >
> > we are calling "devm_clk_get_enabled" which will hold reference of
> > enabled Clk( this will prevent clk_disable_unused() turning off the
> > clocks during late init)
> >
> > 2) pwm disabled by bootloader:-
> >
> > Clock is off during probe, but in apply state we will turn it on by
> > calling pm_runtime_get_sync.
>
> I understand what and why you're doing this, but I think you're doing
> this at the wrong level of abstraction. And that's what's leading to
> this strange construct where you need to manually fiddle with the
> clock.
> If you want to abstract all of this behind PM domains, then this
> should be reflected properly within those PM domains.
>
> That is, if the PWM has been enabled by the bootloader, then
> effectively it's had to enable the PM domain as well (at least
> conceptually, even if the bootloader may not have the same objects as
> Linux). So you need to find a way to mark the PM domain as enabled as
> a whole rather than just one of the clocks that happens to be part of
> it.
>
> Remember, the purpose of abstraction is to hide the implementation
> details (i.e. the clock). You break that abstraction if you go and
> fiddle with the implementation details.
OK, I have added PM runtime callbacks and holding the clock reference in probe()
and added below code in probe() which will take care of both the cases.
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ clk_prepare_enable(rzg2l_gpt->clk);
+ ch0_en = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, 0);
+ if (ch0_en)
+ pm_runtime_get_sync(&pdev->dev);
+ ch1_en = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, 1);
+ if (ch1_en)
+ pm_runtime_get_sync(&pdev->dev);
>
> > > > + mutex_init(&rzg2l_gpt->lock);
> > > > +
> > > > + rzg2l_gpt->chip.dev = &pdev->dev;
> > > > + rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > > > + rzg2l_gpt->chip.npwm = 2;
> > >
> > > The changelog above mentions that you use a shared reset because
> the
> > > reset is shared between 8 channels, but here you only expose 2.
> > > What's going on there?
> >
> > Each hwchannel has 2 IOs. Each IO is modelled as separate channel.
> > Basically, we have 8 hwchannels * 2 IO's = 16 pwm channels in
> total.
> >
> > Please correct me if anything wrong here.
>
> I'm asking because I recently came across a similar driver but where
> the mistake was made to describe the hardware as 4 separate devices
> with 2 channels (or, depending on SoC generation, 1 channel) per
> device.
> Looking at the device tree it's pretty obvious that in that case this
> was really a single device with 8 (or 4, depending) channels. Most of
> the time this doesn't matter and everything works, but then on some HW
> generations all of a sudden you have a shared interrupt for all 8
> channels, and now this becomes really difficult to describe because
> the interrupt needs to be shared between 4 devices, or an extra layer
> is needed to slot in the interrupt somehow.
Yes it a large IP block with 8 hardware channels. Each channel has its
own interrupts, register space and dedicated IO pins.
>
> So I'm just trying to avoid another such situation. Looking at the DTS
> example from the binding in patch 1, you have gpt4 at 0x10048400 with
> 0x100 bytes. Does gpt3 sit at 0x10048300 with 0x100 bytes? If so, it's
Yes, Base Address of the larger IP block: 0x10048000
Each Channel Offset Address = Base Address + H’0100×m -> where m = 0 to 7.
> likely that this is really a single large IP block that you're
> artificially subdividing and that could turn into a similar issue as
> above.
Yes, you are correct. But each channel has 10 interrupts and 2 dedicated IO pins.
It has 4 shared External trigger input pins for all channels. The output of
dedicated IO pins can be controlled by another dedicated IP called POEG which has 4 channels.
Each GPT needs to link with any 1 POEG channel for output disable control.
Looks like in our case we need separate channels rather than
combining into 1 block like option 3. I have listed 3 options.
Please let me know your feedback on these options.
Option1 (Current case):-
Channel specific enablement, but we won't be able to handle the shared cases as you mentioned.
gpt3: pwm@10048300 {
compatible = "renesas,r9a07g044-gpt",
"renesas,rzg2l-gpt";
reg = <0 0x10048300 0 0x100>;
#pwm-cells = <2>;
interrupts = <GIC_SPI 257 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 258 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 259 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 260 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 261 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 262 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 263 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 264 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 265 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 266 IRQ_TYPE_EDGE_RISING>;
interrupt-names = "ccmpa", "ccmpb", "cmpc", "cmpd",
"cmpe", "cmpf", "adtrga", "adtrgb",
"ovf", "unf";
clocks = <&cpg CPG_MOD R9A07G044_GPT_PCLK>;
resets = <&cpg R9A07G044_GPT_RST_C>;
power-domains = <&cpg>;
status = "disabled";
##################################################
This part may be added in future, when we link poeg with gpt for Output control
Using GPT request signal or using POEG register.
port {
gpt3_channel: endpoint {
remote-endpoint = <&poeg_group_d>;
};
};
###################
};
Option 2:
Channel specific enablement, We will be able to handle the shared cases as you mentioned.
gpt: pwm@10048000 {
compatible = "renesas,r9a07g044-gpt",
"renesas,rzg2l-gpt";
reg = <0 0x10048000 0 0x800>;
clocks = <&cpg CPG_MOD R9A07G044_GPT_PCLK>;
resets = <&cpg R9A07G044_GPT_RST_C>;
power-domains = <&cpg>;
status = "disabled";
gpt0: pwm@0 {
reg = <0>;
#pwm-cells = <2>;
interrupts = <GIC_SPI 218 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 219 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 220 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 221 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 222 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 223 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 224 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 225 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 226 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 227 IRQ_TYPE_EDGE_RISING>;
interrupt-names = "ccmpa", "ccmpb", "cmpc", "cmpd",
"cmpe", "cmpf", "adtrga", "adtrgb",
"ovf", "unf";
status = "disabled";
##################################################
This part may be added in future, when we link poeg with gpt for Output control
Using GPT request signal or using POEG register.
port {
gpt0_channel: endpoint {
remote-endpoint = <&poeg_group_d>;
};
};
###################
};
gpt1: pwm@100 {
reg = <0x100>;
#pwm-cells = <2>;
interrupts = <GIC_SPI 231 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 232 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 233 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 234 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 235 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 236 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 237 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 238 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 239 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 240 IRQ_TYPE_EDGE_RISING>;
interrupt-names = "ccmpa", "ccmpb", "cmpc", "cmpd",
"cmpe", "cmpf", "adtrga", "adtrgb",
"ovf", "unf";
status = "disabled";
};
gpt2: pwm@200 {
reg = <0x200>;
#pwm-cells = <2>;
interrupts = <GIC_SPI 244 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 245 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 246 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 247 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 248 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 249 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 250 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 251 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 252 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 253 IRQ_TYPE_EDGE_RISING>;
interrupt-names = "ccmpa", "ccmpb", "cmpc", "cmpd",
"cmpe", "cmpf", "adtrga", "adtrgb",
"ovf", "unf";
status = "disabled";
};
gpt3: pwm@300 {
reg = <0x300>;
#pwm-cells = <2>;
interrupts = <GIC_SPI 257 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 258 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 259 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 260 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 261 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 262 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 263 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 264 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 265 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 266 IRQ_TYPE_EDGE_RISING>;
interrupt-names = "ccmpa", "ccmpb", "cmpc", "cmpd",
"cmpe", "cmpf", "adtrga", "adtrgb",
"ovf", "unf";
status = "disabled";
};
gpt4: pwm@400 {
reg = <0x400>;
#pwm-cells = <2>;
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";
status = "disabled";
};
gpt5: pwm@500 {
reg = <0x500>;
#pwm-cells = <2>;
interrupts = <GIC_SPI 283 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 284 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 285 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 286 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 287 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 288 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 289 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 290 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 291 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 292 IRQ_TYPE_EDGE_RISING>;
interrupt-names = "ccmpa", "ccmpb", "cmpc", "cmpd",
"cmpe", "cmpf", "adtrga", "adtrgb",
"ovf", "unf";
clocks = <&cpg CPG_MOD R9A07G044_GPT_PCLK>;
status = "disabled";
};
gpt6: pwm@600 {
reg = <0x600>;
#pwm-cells = <2>;
interrupts = <GIC_SPI 296 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 297 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 298 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 299 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 300 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 301 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 302 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 303 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 304 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 305 IRQ_TYPE_EDGE_RISING>;
interrupt-names = "ccmpa", "ccmpb", "cmpc", "cmpd",
"cmpe", "cmpf", "adtrga", "adtrgb",
"ovf", "unf";
status = "disabled";
};
gpt7: pwm@700 {
reg = <0x700>;
#pwm-cells = <2>;
interrupts = <GIC_SPI 309 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 310 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 311 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 312 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 313 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 314 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 315 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 316 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 317 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 318 IRQ_TYPE_EDGE_RISING>;
interrupt-names = "ccmpa", "ccmpb", "cmpc", "cmpd",
"cmpe", "cmpf", "adtrga", "adtrgb",
"ovf", "unf";
status = "disabled";
};
};
Option 3:
All 16 channels(8 hw channels * 2 IO) enabled by default. Associating channels with pins and linking
With POEG modules will be a problem.
gpt: pwm@10048000 {
compatible = "renesas,r9a07g044-gpt",
"renesas,rzg2l-gpt";
reg = <0 0x10048000 0 0x800>;
clocks = <&cpg CPG_MOD R9A07G044_GPT_PCLK>;
resets = <&cpg R9A07G044_GPT_RST_C>;
power-domains = <&cpg>;
status = "disabled";
#pwm-cells = <2>;
interrupts = <GIC_SPI 218 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 219 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 220 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 221 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 222 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 223 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 224 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 225 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 226 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 227 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 231 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 232 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 233 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 234 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 235 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 236 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 237 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 238 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 239 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 240 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 244 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 245 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 246 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 247 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 248 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 249 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 250 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 251 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 252 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 253 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 257 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 258 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 259 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 260 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 261 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 262 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 263 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 264 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 265 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 266 IRQ_TYPE_EDGE_RISING>,
<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>,
<GIC_SPI 283 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 284 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 285 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 286 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 287 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 288 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 289 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 290 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 291 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 292 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 296 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 297 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 298 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 299 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 300 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 301 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 302 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 303 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 304 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 305 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 309 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 310 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 311 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 312 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 313 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 314 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 315 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 316 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 317 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 318 IRQ_TYPE_EDGE_RISING>;
interrupt-names = "ccmpa0", "ccmpb0", "cmpc0", "cmpd0",
"cmpe0", "cmpf0", "adtrga0", "adtrgb0",
"ovf0", "unf0";
"ccmpa1", "ccmpb1", "cmpc1", "cmpd1",
"cmpe1", "cmpf1", "adtrga1", "adtrgb1",
"ovf1", "unf1";
"ccmpa2", "ccmpb2", "cmpc2", "cmpd2",
"cmpe2", "cmpf2", "adtrga2", "adtrgb2",
"ovf2", "unf2";
"ccmpa3", "ccmpb3", "cmpc3", "cmpd3",
"cmpe3", "cmpf3", "adtrga3", "adtrgb3",
"ovf3", "unf3";
"ccmpa4", "ccmpb4", "cmpc4", "cmpd4",
"cmpe4", "cmpf4", "adtrga4", "adtrgb4",
"ovf4", "unf4";
"ccmpa5", "ccmpb5", "cmpc5", "cmpd5",
"cmpe5", "cmpf5", "adtrga5", "adtrgb5",
"ovf5", "unf5";
"ccmpa6", "ccmpb6", "cmpc6", "cmpd6",
"cmpe6", "cmpf6", "adtrga6", "adtrgb6",
"ovf6", "unf6";
"ccmpa7", "ccmpb7", "cmpc7", "cmpd7",
"cmpe7", "cmpf7", "adtrga7", "adtrgb7",
"ovf7", "unf7";
};
};
Cheers,
Biju
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT
2022-09-29 9:48 ` Thierry Reding
2022-10-01 14:10 ` Biju Das
@ 2022-10-11 11:53 ` Biju Das
1 sibling, 0 replies; 16+ messages in thread
From: Biju Das @ 2022-10-11 11:53 UTC (permalink / raw)
To: Thierry Reding
Cc: Lee Jones, Philipp Zabel, Uwe Kleine-König, linux-pwm,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad, linux-renesas-soc
Hi Thierry,
>
> > > > + mutex_init(&rzg2l_gpt->lock);
> > > > +
> > > > + rzg2l_gpt->chip.dev = &pdev->dev;
> > > > + rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > > > + rzg2l_gpt->chip.npwm = 2;
> > >
> > > The changelog above mentions that you use a shared reset because
> the
> > > reset is shared between 8 channels, but here you only expose 2.
> > > What's going on there?
> >
> > Each hwchannel has 2 IOs. Each IO is modelled as separate channel.
> > Basically, we have 8 hwchannels * 2 IO's = 16 pwm channels in
> total.
> >
> > Please correct me if anything wrong here.
>
> I'm asking because I recently came across a similar driver but where
> the mistake was made to describe the hardware as 4 separate devices
> with 2 channels (or, depending on SoC generation, 1 channel) per
> device.
> Looking at the device tree it's pretty obvious that in that case this
> was really a single device with 8 (or 4, depending) channels. Most of
> the time this doesn't matter and everything works, but then on some HW
> generations all of a sudden you have a shared interrupt for all 8
> channels, and now this becomes really difficult to describe because
> the interrupt needs to be shared between 4 devices, or an extra layer
> is needed to slot in the interrupt somehow.
>
> So I'm just trying to avoid another such situation. Looking at the DTS
> example from the binding in patch 1, you have gpt4 at 0x10048400 with
> 0x100 bytes. Does gpt3 sit at 0x10048300 with 0x100 bytes? If so, it's
> likely that this is really a single large IP block that you're
> artificially subdividing and that could turn into a similar issue as
> above.
OK, I have modelled as single PWM device with 16 channels
and test results are looks ok.
I will be sending next version soon.
Cheers,
Biju
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-10-11 11:53 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-21 14:57 [PATCH v7 0/2] Add support for RZ/G2L GPT Biju Das
2022-09-21 14:57 ` [PATCH v7 1/2] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
2022-09-21 14:57 ` [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT Biju Das
2022-09-28 13:50 ` Thierry Reding
2022-09-28 14:09 ` Geert Uytterhoeven
2022-09-28 17:34 ` Biju Das
2022-09-28 18:07 ` Geert Uytterhoeven
2022-09-28 19:21 ` Biju Das
2022-09-29 9:48 ` Thierry Reding
2022-10-01 14:10 ` Biju Das
2022-10-11 11:53 ` Biju Das
2022-09-29 17:36 ` Biju Das
2022-09-30 6:34 ` Uwe Kleine-König
2022-09-30 6:51 ` Biju Das
2022-09-30 7:03 ` Uwe Kleine-König
2022-09-30 7:43 ` Biju Das
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).