linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/5] Add RZ/G2L MTU3a Core, Counter and pwm driver
@ 2022-11-13 17:15 Biju Das
  2022-11-13 17:15 ` [PATCH v6 1/5] dt-bindings: timer: Document RZ/G2L MTU3a bindings Biju Das
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Biju Das @ 2022-11-13 17:15 UTC (permalink / raw)
  To: Rob Herring, Philipp Zabel, Daniel Lezcano,
	William Breathitt Gray, Thierry Reding, Uwe Kleine-König,
	Krzysztof Kozlowski
  Cc: Biju Das, Thomas Gleixner, devicetree, Geert Uytterhoeven,
	Chris Paterson, Prabhakar Mahadev Lad, linux-renesas-soc

The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded in
the Renesas RZ/G2L family SoC's. It consists of eight 16-bit timer
channels and one 32-bit timer channel. It supports the following
functions
 - Counter
 - Timer
 - PWM

This patch series aims to add core, counter and pwm driver for
MTU3a. The core instantiates child devices using mfd api.

The 8/16/32 bit registers are mixed in each channel. The HW
specifications of the IP is described in patch#1.

Current patch set is tested for PWM mode1 on MTU3 channel
and 16 and 32 bit phase counting modes on MTU1 and MTU2 channels.

Clock source and clock event driver will be added later.

v5->v6:
 * Added Rb tag from Rob and Krzysztof for the binding patch.
 * Updated commit and KConfig description for the driver patches
 * Selected MFD_CORE to avoid build error if CONFIG_MFD_CORE not set.
 * Improved error handling in core driver's probe().
 * Fixed RZ_MTU3_GET_HW_CH Macro for argument reuse 'id' - 
   possible side-effects?
 * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed
   __maybe_unused from suspend/resume()
 * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable()
 * Sorted header files for all driver files.
v4->v5:
 * Modelled as timer bindings.
 * Fixed the typo in bindings.
 * Moved core driver from MFD to timer
 * Child devices instatiated using mfd_add_devices()
 * Documented sysfs entries external_input_phase_clock_select and
   long_word_access_ctrl_mode.
 * Updated the Kconfig with SoC vendor name
 * Introduced rz_mtu3_is_counter_invalid()
 * replaced pointer to an array of struct rz_mtu3_channel with
   a simple pointer to struct rz_mtu3_channel.
 * Added long_word_access_ctrl_mode sysfs entry for 16-bit and
   32-bit access
 * Added external_input_phase_clock_select sysfs entry for
   selecting input clocks.
 * used preprocessor defines represent SIGNAL_{A,B,C,D}_ID instead of
   signal ids.
v3->v4:
 * Dropped counter and pwm compatibeles as they don't have any resources.
 * Made rz-mtu3 as pwm provider.
 * Updated the example and description.
 * A single driver that registers both the counter and the pwm functionalities
   that binds against "renesas,rz-mtu3".
 * Moved PM handling from child devices to here.
 * replaced include/linux/mfd/rz-mtu3.h->drivers/mfd/rz-mtu3.h
 * Removed "remove" callback from mfd driver
 * There is no resource associated with "rz-mtu3-counter" and "rz-mtu3-pwm"
   compatible and moved the code to mfd subsystem as it binds against "rz-mtu".
 * Removed struct platform_driver rz_mtu3_cnt_driver.
 * Removed struct platform_driver rz_mtu3_pwm_driver.
 * Updated commit description
 * Updated Kconfig description
 * Added macros RZ_MTU3_16_BIT_MTU{1,2}_CH for MTU1 and MTU2 channels
 * Added RZ_MTU3_GET_HW_CH macro for getting channel ID.
 * replaced priv->ch[id]->priv->ch[0] in rz_mtu3_count_read()
 * Cached counter max values
 * replaced cnt->tsr in rz_mtu3_count_direction_read()
 * Added comments for RZ_MTU3_TCR_CCLR_NONE
 * Replaced if with switch in rz_mtu3_initialize_counter() and
   rz_mtu3_count_ceiling_write()
 * Added locks in initialize, terminate and enable_read to prevent races.
 * Updated rz_mtu3_action_read to take care of MTU2 signals.
 * Added separate distinct array for each group of Synapse.
 * Moved pm handling to parent.
v2->v3:
 * Dropped counter bindings and integrated with mfd as it has only one property.
 * Removed "#address-cells" and "#size-cells" as it do not have children with
   unit addresses.
 * Removed quotes from counter and pwm.
 * Provided full path for pwm bindings.
 * Updated the binding example.
 * removed unwanted header files
 * Added LUT for 32 bit registers as it needed for 32-bit cascade counting.
 * Exported 32 bit read/write functions.
 * Modelled as a counter device supporting 3 counters(2 16-bit and 
   32-bit)
 * Add kernel-doc comments to document struct rz_mtu3_cnt
 * Removed mmio variable from struct rz_mtu3_cnt
 * Removed cnt local variable from rz_mtu3_count_read()
 * Replaced -EINVAL->-ERANGE for out of range error conditions.
 * Removed explicit cast from write functions.
 * Removed local variable val from rz_mtu3_count_ceiling_read()
 * Added lock for RMW for counter/ceiling updates.
 * Added different synapses for counter0 and counter{1,2}
 * Used ARRAY for assigning num_counts.
 * Added PM runtime for managing clocks.
 * Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.

RFC->v2:
 * replaced devm_reset_control_get->devm_reset_control_get_exclusive
 * Dropped 'bindings' from the binding title
 * Updated the binding example
 * Added additionalProperties: false for counter bindings
 * Squashed all the binding patches
 * Modelled as a single counter device providing both 16-bit
   and 32-bit phase counting modes
 * Modelled as a single pwm device for supporting different pwm modes.
 * Moved counter and pwm bindings to respective subsystems.

Biju Das (5):
  dt-bindings: timer: Document RZ/G2L MTU3a bindings
  clocksource/drivers: Add Renesas RZ/G2L MTU3a core driver
  Documentation: ABI: sysfs-bus-counter: add
    external_input_phase_clock_select & long_word_access_ctrl_mode items
  counter: Add Renesas RZ/G2L MTU3a counter driver
  pwm: Add Renesas RZ/G2L MTU3a PWM driver

 Documentation/ABI/testing/sysfs-bus-counter   |  16 +
 .../bindings/timer/renesas,rz-mtu3.yaml       | 302 ++++++++
 drivers/clocksource/Kconfig                   |  11 +
 drivers/clocksource/Makefile                  |   1 +
 drivers/clocksource/rz-mtu3.c                 | 443 +++++++++++
 drivers/counter/Kconfig                       |  11 +
 drivers/counter/Makefile                      |   1 +
 drivers/counter/rz-mtu3-cnt.c                 | 717 ++++++++++++++++++
 drivers/pwm/Kconfig                           |  11 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-rz-mtu3.c                     | 455 +++++++++++
 include/clocksource/rz-mtu3.h                 | 206 +++++
 12 files changed, 2175 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/renesas,rz-mtu3.yaml
 create mode 100644 drivers/clocksource/rz-mtu3.c
 create mode 100644 drivers/counter/rz-mtu3-cnt.c
 create mode 100644 drivers/pwm/pwm-rz-mtu3.c
 create mode 100644 include/clocksource/rz-mtu3.h

-- 
2.25.1


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

* [PATCH v6 1/5] dt-bindings: timer: Document RZ/G2L MTU3a bindings
  2022-11-13 17:15 [PATCH v6 0/5] Add RZ/G2L MTU3a Core, Counter and pwm driver Biju Das
@ 2022-11-13 17:15 ` Biju Das
  2022-11-13 17:15 ` [PATCH v6 2/5] clocksource/drivers: Add Renesas RZ/G2L MTU3a core driver Biju Das
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Biju Das @ 2022-11-13 17:15 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski
  Cc: Biju Das, Daniel Lezcano, Thomas Gleixner, devicetree,
	Geert Uytterhoeven, Chris Paterson, Prabhakar Mahadev Lad,
	linux-renesas-soc, Krzysztof Kozlowski, Rob Herring

The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded in
the Renesas RZ/G2L family SoC's. It consists of eight 16-bit timer
channels and one 32-bit timer channel. It supports the following
functions
 - Counter
 - Timer
 - PWM

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v5->v6:
 * Added Rb tag from Rob and Krzysztof.
v4->v5:
 * Modelled as timer bindings.
 * Fixed the typo.
v3->v4:
 * Dropped counter and pwm compatibeles as they don't have any resources.
 * Made rz-mtu3 as pwm provider.
 * Updated the example and description.
v2->v3:
 * Dropped counter bindings and integrated with mfd as it has only one property.
 * Removed "#address-cells" and "#size-cells" as it do not have children with
   unit addresses.
 * Removed quotes from counter and pwm.
 * Provided full path for pwm bindings.
 * Updated the example.
v1->v2:
 * Modelled counter and pwm as a single device that handles
   multiple channels.
 * Moved counter and pwm bindings to respective subsystems
 * Dropped 'bindings' from MFD binding title.
 * Updated the example
 * Changed the compatible names.
---
 .../bindings/timer/renesas,rz-mtu3.yaml       | 302 ++++++++++++++++++
 1 file changed, 302 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/renesas,rz-mtu3.yaml

diff --git a/Documentation/devicetree/bindings/timer/renesas,rz-mtu3.yaml b/Documentation/devicetree/bindings/timer/renesas,rz-mtu3.yaml
new file mode 100644
index 000000000000..bffdab0b0185
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/renesas,rz-mtu3.yaml
@@ -0,0 +1,302 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/renesas,rz-mtu3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/G2L Multi-Function Timer Pulse Unit 3 (MTU3a)
+
+maintainers:
+  - Biju Das <biju.das.jz@bp.renesas.com>
+
+description: |
+  This hardware block consists of eight 16-bit timer channels and one
+  32- bit timer channel. It supports the following specifications:
+    - Pulse input/output: 28 lines max.
+    - Pulse input 3 lines
+    - Count clock 11 clocks for each channel (14 clocks for MTU0, 12 clocks
+      for MTU2, and 10 clocks for MTU5, four clocks for MTU1-MTU2 combination
+      (when LWA = 1))
+    - Operating frequency Up to 100 MHz
+    - Available operations [MTU0 to MTU4, MTU6, MTU7, and MTU8]
+        - Waveform output on compare match
+        - Input capture function (noise filter setting available)
+        - Counter-clearing operation
+        - Simultaneous writing to multiple timer counters (TCNT)
+          (excluding MTU8).
+        - Simultaneous clearing on compare match or input capture
+          (excluding MTU8).
+        - Simultaneous input and output to registers in synchronization with
+          counter operations           (excluding MTU8).
+        - Up to 12-phase PWM output in combination with synchronous operation
+          (excluding MTU8)
+    - [MTU0 MTU3, MTU4, MTU6, MTU7, and MTU8]
+        - Buffer operation specifiable
+    - [MTU1, MTU2]
+        - Phase counting mode can be specified independently
+        - 32-bit phase counting mode can be specified for interlocked operation
+          of MTU1 and MTU2 (when TMDR3.LWA = 1)
+        - Cascade connection operation available
+    - [MTU3, MTU4, MTU6, and MTU7]
+        - Through interlocked operation of MTU3/4 and MTU6/7, the positive and
+          negative signals in six phases (12 phases in total) can be output in
+          complementary PWM and reset-synchronized PWM operation.
+        - In complementary PWM mode, values can be transferred from buffer
+          registers to temporary registers at crests and troughs of the timer-
+          counter values or when the buffer registers (TGRD registers in MTU4
+          and MTU7) are written to.
+        - Double-buffering selectable in complementary PWM mode.
+    - [MTU3 and MTU4]
+        - Through interlocking with MTU0, a mode for driving AC synchronous
+          motors (brushless DC motors) by using complementary PWM output and
+          reset-synchronized PWM output is settable and allows the selection
+          of two types of waveform output (chopping or level).
+    - [MTU5]
+        - Capable of operation as a dead-time compensation counter.
+    - [MTU0/MTU5, MTU1, MTU2, and MTU8]
+        - 32-bit phase counting mode specifiable by combining MTU1 and MTU2 and
+          through interlocked operation with MTU0/MTU5 and MTU8.
+    - Interrupt-skipping function
+        - In complementary PWM mode, interrupts on crests and troughs of counter
+          values and triggers to start conversion by the A/D converter can be
+          skipped.
+    - Interrupt sources: 43 sources.
+    - Buffer operation:
+        - Automatic transfer of register data (transfer from the buffer
+          register to the timer register).
+    - Trigger generation
+        - A/D converter start triggers can be generated
+        - A/D converter start request delaying function enables A/D converter
+          to be started with any desired timing and to be synchronized with
+          PWM output.
+    - Low power consumption function
+        - The MTU3a can be placed in the module-stop state.
+
+    There are two phase counting modes. 16-bit phase counting mode in which
+    MTU1 and MTU2 operate independently, and cascade connection 32-bit phase
+    counting mode in which MTU1 and MTU2 are cascaded.
+
+    In phase counting mode, the phase difference between two external input
+    clocks is detected and the corresponding TCNT is incremented or
+    decremented.
+    The below counters are supported
+      count0 - MTU1 16-bit phase counting
+      count1 - MTU2 16-bit phase counting
+      count2 - MTU1+ MTU2 32-bit phase counting
+
+    The module supports PWM mode{1,2}, Reset-synchronized PWM mode and
+    complementary PWM mode{1,2,3}.
+
+    In complementary PWM mode, six positive-phase and six negative-phase PWM
+    waveforms (12 phases in total) with dead time can be output by
+    combining MTU{3,4} and MTU{6,7}.
+
+    The below pwm channels are supported in pwm mode 1.
+      pwm0  - MTU0.MTIOC0A PWM mode 1
+      pwm1  - MTU0.MTIOC0C PWM mode 1
+      pwm2  - MTU1.MTIOC1A PWM mode 1
+      pwm3  - MTU2.MTIOC2A PWM mode 1
+      pwm4  - MTU3.MTIOC3A PWM mode 1
+      pwm5  - MTU3.MTIOC3C PWM mode 1
+      pwm6  - MTU4.MTIOC4A PWM mode 1
+      pwm7  - MTU4.MTIOC4C PWM mode 1
+      pwm8  - MTU6.MTIOC6A PWM mode 1
+      pwm9  - MTU6.MTIOC6C PWM mode 1
+      pwm10 - MTU7.MTIOC7A PWM mode 1
+      pwm11 - MTU7.MTIOC7C PWM mode 1
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a07g044-mtu3  # RZ/G2{L,LC}
+          - renesas,r9a07g054-mtu3  # RZ/V2L
+      - const: renesas,rz-mtu3
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: MTU0.TGRA input capture/compare match
+      - description: MTU0.TGRB input capture/compare match
+      - description: MTU0.TGRC input capture/compare match
+      - description: MTU0.TGRD input capture/compare match
+      - description: MTU0.TCNT overflow
+      - description: MTU0.TGRE compare match
+      - description: MTU0.TGRF compare match
+      - description: MTU1.TGRA input capture/compare match
+      - description: MTU1.TGRB input capture/compare match
+      - description: MTU1.TCNT overflow
+      - description: MTU1.TCNT underflow
+      - description: MTU2.TGRA input capture/compare match
+      - description: MTU2.TGRB input capture/compare match
+      - description: MTU2.TCNT overflow
+      - description: MTU2.TCNT underflow
+      - description: MTU3.TGRA input capture/compare match
+      - description: MTU3.TGRB input capture/compare match
+      - description: MTU3.TGRC input capture/compare match
+      - description: MTU3.TGRD input capture/compare match
+      - description: MTU3.TCNT overflow
+      - description: MTU4.TGRA input capture/compare match
+      - description: MTU4.TGRB input capture/compare match
+      - description: MTU4.TGRC input capture/compare match
+      - description: MTU4.TGRD input capture/compare match
+      - description: MTU4.TCNT overflow/underflow
+      - description: MTU5.TGRU input capture/compare match
+      - description: MTU5.TGRV input capture/compare match
+      - description: MTU5.TGRW input capture/compare match
+      - description: MTU6.TGRA input capture/compare match
+      - description: MTU6.TGRB input capture/compare match
+      - description: MTU6.TGRC input capture/compare match
+      - description: MTU6.TGRD input capture/compare match
+      - description: MTU6.TCNT overflow
+      - description: MTU7.TGRA input capture/compare match
+      - description: MTU7.TGRB input capture/compare match
+      - description: MTU7.TGRC input capture/compare match
+      - description: MTU7.TGRD input capture/compare match
+      - description: MTU7.TCNT overflow/underflow
+      - description: MTU8.TGRA input capture/compare match
+      - description: MTU8.TGRB input capture/compare match
+      - description: MTU8.TGRC input capture/compare match
+      - description: MTU8.TGRD input capture/compare match
+      - description: MTU8.TCNT overflow
+      - description: MTU8.TCNT underflow
+
+  interrupt-names:
+    items:
+      - const: tgia0
+      - const: tgib0
+      - const: tgic0
+      - const: tgid0
+      - const: tgiv0
+      - const: tgie0
+      - const: tgif0
+      - const: tgia1
+      - const: tgib1
+      - const: tgiv1
+      - const: tgiu1
+      - const: tgia2
+      - const: tgib2
+      - const: tgiv2
+      - const: tgiu2
+      - const: tgia3
+      - const: tgib3
+      - const: tgic3
+      - const: tgid3
+      - const: tgiv3
+      - const: tgia4
+      - const: tgib4
+      - const: tgic4
+      - const: tgid4
+      - const: tgiv4
+      - const: tgiu5
+      - const: tgiv5
+      - const: tgiw5
+      - const: tgia6
+      - const: tgib6
+      - const: tgic6
+      - const: tgid6
+      - const: tgiv6
+      - const: tgia7
+      - const: tgib7
+      - const: tgic7
+      - const: tgid7
+      - const: tgiv7
+      - const: tgia8
+      - const: tgib8
+      - const: tgic8
+      - const: tgid8
+      - const: tgiv8
+      - const: tgiu8
+
+  clocks:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - power-domains
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/r9a07g044-cpg.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    mtu3: timer@10001200 {
+      compatible = "renesas,r9a07g044-mtu3", "renesas,rz-mtu3";
+      reg = <0x10001200 0xb00>;
+      interrupts = <GIC_SPI 170 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 171 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 172 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 173 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 174 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 175 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 176 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 177 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 178 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 179 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 180 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 181 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 182 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 183 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 184 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 185 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 186 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 189 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 190 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 191 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 192 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 193 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 194 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 195 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 196 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 197 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 198 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 199 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 200 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 201 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 202 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 203 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 204 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 205 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 208 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 209 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 210 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 211 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 212 IRQ_TYPE_EDGE_RISING>,
+                   <GIC_SPI 213 IRQ_TYPE_EDGE_RISING>;
+      interrupt-names = "tgia0", "tgib0", "tgic0", "tgid0", "tgiv0", "tgie0",
+                        "tgif0",
+                        "tgia1", "tgib1", "tgiv1", "tgiu1",
+                        "tgia2", "tgib2", "tgiv2", "tgiu2",
+                        "tgia3", "tgib3", "tgic3", "tgid3", "tgiv3",
+                        "tgia4", "tgib4", "tgic4", "tgid4", "tgiv4",
+                        "tgiu5", "tgiv5", "tgiw5",
+                        "tgia6", "tgib6", "tgic6", "tgid6", "tgiv6",
+                        "tgia7", "tgib7", "tgic7", "tgid7", "tgiv7",
+                        "tgia8", "tgib8", "tgic8", "tgid8", "tgiv8", "tgiu8";
+      clocks = <&cpg CPG_MOD R9A07G044_MTU_X_MCK_MTU3>;
+      power-domains = <&cpg>;
+      resets = <&cpg R9A07G044_MTU_X_PRESET_MTU3>;
+      #pwm-cells = <2>;
+    };
-- 
2.25.1


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

* [PATCH v6 2/5] clocksource/drivers: Add Renesas RZ/G2L MTU3a core driver
  2022-11-13 17:15 [PATCH v6 0/5] Add RZ/G2L MTU3a Core, Counter and pwm driver Biju Das
  2022-11-13 17:15 ` [PATCH v6 1/5] dt-bindings: timer: Document RZ/G2L MTU3a bindings Biju Das
@ 2022-11-13 17:15 ` Biju Das
  2022-11-13 17:15 ` [PATCH v6 3/5] Documentation: ABI: sysfs-bus-counter: add external_input_phase_clock_select & long_word_access_ctrl_mode items Biju Das
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Biju Das @ 2022-11-13 17:15 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: Biju Das, Daniel Lezcano, Thomas Gleixner, Geert Uytterhoeven,
	Chris Paterson, Prabhakar Mahadev Lad, linux-renesas-soc

The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded in
the Renesas RZ/G2L family SoCs. It consists of eight 16-bit timer
channels and one 32-bit timer channel. It supports the following
functions
 - Counter
 - Timer
 - PWM

The 8/16/32 bit registers are mixed in each channel.

Add MTU3a core driver for RZ/G2L SoC. The core driver shares the
clk and channel register access for the other child devices like
Counter, PWM, Clock Source, and Clock event.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v5->v6:
 * Updated commit and KConfig description
 * Selected MFD_CORE to avoid build error if CONFIG_MFD_CORE not set.
 * Improved error handling in probe().
 * Updated MODULE_DESCRIPTION and title.
v4->v5:
 * Moved core driver from MFD to timer
 * Child devices instatiated using mfd_add_devices()
v3->v4:
 * A single driver that registers both the counter and the pwm functionalities
   that binds against "renesas,rz-mtu3".
 * Moved PM handling from child devices to here.
 * replaced include/linux/mfd/rz-mtu3.h->drivers/mfd/rz-mtu3.h
 * Removed "remove" callback
v2->v3:
 * removed unwanted header files
 * Added LUT for 32 bit registers as it needed for 32-bit cascade counting.
 * Exported 32 bit read/write functions.
v1->v2:
 * Changed the compatible name
 * Replaced devm_reset_control_get->devm_reset_control_get_exclusive
 * Renamed function names rzg2l_mtu3->rz_mtu3 as this is generic IP
   in RZ family SoC's.
---
 drivers/clocksource/Kconfig   |  11 +
 drivers/clocksource/Makefile  |   1 +
 drivers/clocksource/rz-mtu3.c | 443 ++++++++++++++++++++++++++++++++++
 include/clocksource/rz-mtu3.h | 206 ++++++++++++++++
 4 files changed, 661 insertions(+)
 create mode 100644 drivers/clocksource/rz-mtu3.c
 create mode 100644 include/clocksource/rz-mtu3.h

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4469e7f555e9..cd7d549d18f3 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -524,6 +524,17 @@ config SH_TIMER_MTU2
 	  Timer Pulse Unit 2 (MTU2) hardware available on SoCs from Renesas.
 	  This hardware comes with 16-bit timer registers.
 
+config RZ_MTU3
+	bool "Renesas RZ/G2L MTU3a core driver"
+	select MFD_CORE
+	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
+	help
+	  Select this option to enable Renesas RZ/G2L MTU3a core driver for
+	  the Multi-Function Timer Pulse Unit 3 (MTU3a) hardware available
+	  on SoCs from Renesas. The core driver shares the clk and channel
+	  register access for the other child devices like Counter, PWM,
+	  Clock Source, and Clock event.
+
 config RENESAS_OSTM
 	bool "Renesas OSTM timer driver"
 	depends on ARCH_RENESAS || COMPILE_TEST
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 64ab547de97b..9ffb46614d98 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC)	+= timer-cs5535.o
 obj-$(CONFIG_CLKSRC_JCORE_PIT)		+= jcore-pit.o
 obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
 obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
+obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
 obj-$(CONFIG_RENESAS_OSTM)	+= renesas-ostm.o
 obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
 obj-$(CONFIG_EM_TIMER_STI)	+= em_sti.o
diff --git a/drivers/clocksource/rz-mtu3.c b/drivers/clocksource/rz-mtu3.c
new file mode 100644
index 000000000000..9b3a6c74b5ef
--- /dev/null
+++ b/drivers/clocksource/rz-mtu3.c
@@ -0,0 +1,443 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L Multi-Function Timer Pulse Unit 3(MTU3a) Core driver
+ *
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mfd/core.h>
+#include <linux/of_platform.h>
+#include <linux/reset.h>
+
+#include <clocksource/rz-mtu3.h>
+
+static const unsigned long rz_mtu3_8bit_ch_reg_offs[][13] = {
+	{
+		[RZ_MTU3_TIER] = 0x4, [RZ_MTU3_NFCR] = 0x70,
+		[RZ_MTU3_TCR] = 0x0, [RZ_MTU3_TCR2] = 0x28,
+		[RZ_MTU3_TMDR1] = 0x1, [RZ_MTU3_TIORH] = 0x2,
+		[RZ_MTU3_TIORL] = 0x3
+	},
+	{
+		[RZ_MTU3_TIER] = 0x4, [RZ_MTU3_NFCR] = 0xef,
+		[RZ_MTU3_TSR] = 0x5, [RZ_MTU3_TCR] = 0x0,
+		[RZ_MTU3_TCR2] = 0x14, [RZ_MTU3_TMDR1] = 0x1,
+		[RZ_MTU3_TIOR] = 0x2
+	},
+	{
+		[RZ_MTU3_TIER] = 0x4, [RZ_MTU3_NFCR] = 0x16e,
+		[RZ_MTU3_TSR] = 0x5, [RZ_MTU3_TCR] = 0x0,
+		[RZ_MTU3_TCR2] = 0xc, [RZ_MTU3_TMDR1] = 0x1,
+		[RZ_MTU3_TIOR] = 0x2
+	},
+	{
+		[RZ_MTU3_TIER] = 0x8, [RZ_MTU3_NFCR] = 0x93,
+		[RZ_MTU3_TSR] = 0x2c, [RZ_MTU3_TCR] = 0x0,
+		[RZ_MTU3_TCR2] = 0x4c, [RZ_MTU3_TMDR1] = 0x2,
+		[RZ_MTU3_TIORH] = 0x4, [RZ_MTU3_TIORL] = 0x5,
+		[RZ_MTU3_TBTM] = 0x38
+	},
+	{
+		[RZ_MTU3_TIER] = 0x8, [RZ_MTU3_NFCR] = 0x93,
+		[RZ_MTU3_TSR] = 0x2c, [RZ_MTU3_TCR] = 0x0,
+		[RZ_MTU3_TCR2] = 0x4c, [RZ_MTU3_TMDR1] = 0x2,
+		[RZ_MTU3_TIORH] = 0x5, [RZ_MTU3_TIORL] = 0x6,
+		[RZ_MTU3_TBTM] = 0x38
+	},
+	{
+		[RZ_MTU3_TIER] = 0x32, [RZ_MTU3_NFCR] = 0x1eb,
+		[RZ_MTU3_TSTR] = 0x34, [RZ_MTU3_TCNTCMPCLR] = 0x36,
+		[RZ_MTU3_TCRU] = 0x4, [RZ_MTU3_TCR2U] = 0x5,
+		[RZ_MTU3_TIORU] = 0x6, [RZ_MTU3_TCRV] = 0x14,
+		[RZ_MTU3_TCR2V] = 0x15, [RZ_MTU3_TIORV] = 0x16,
+		[RZ_MTU3_TCRW] = 0x24, [RZ_MTU3_TCR2W] = 0x25,
+		[RZ_MTU3_TIORW] = 0x26
+	},
+	{
+		[RZ_MTU3_TIER] = 0x8, [RZ_MTU3_NFCR] = 0x93,
+		[RZ_MTU3_TSR] = 0x2c, [RZ_MTU3_TCR] = 0x0,
+		[RZ_MTU3_TCR2] = 0x4c, [RZ_MTU3_TMDR1] = 0x2,
+		[RZ_MTU3_TIORH] = 0x4, [RZ_MTU3_TIORL] = 0x5,
+		[RZ_MTU3_TBTM] = 0x38
+	},
+	{
+		[RZ_MTU3_TIER] = 0x8, [RZ_MTU3_NFCR] = 0x93,
+		[RZ_MTU3_TSR] = 0x2c, [RZ_MTU3_TCR] = 0x0,
+		[RZ_MTU3_TCR2] = 0x4c, [RZ_MTU3_TMDR1] = 0x2,
+		[RZ_MTU3_TIORH] = 0x5, [RZ_MTU3_TIORL] = 0x6,
+		[RZ_MTU3_TBTM] = 0x38
+	},
+	{
+		[RZ_MTU3_TIER] = 0x4, [RZ_MTU3_NFCR] = 0x368,
+		[RZ_MTU3_TCR] = 0x0, [RZ_MTU3_TCR2] = 0x6,
+		[RZ_MTU3_TMDR1] = 0x1, [RZ_MTU3_TIORH] = 0x2,
+		[RZ_MTU3_TIORL] = 0x3
+	}
+};
+
+static const unsigned long rz_mtu3_16bit_ch_reg_offs[][12] = {
+	{
+		[RZ_MTU3_TCNT] = 0x6, [RZ_MTU3_TGRA] = 0x8,
+		[RZ_MTU3_TGRB] = 0xa, [RZ_MTU3_TGRC] = 0xc,
+		[RZ_MTU3_TGRD] = 0xe, [RZ_MTU3_TGRE] = 0x20,
+		[RZ_MTU3_TGRF] = 0x22
+	},
+	{
+		[RZ_MTU3_TCNT] = 0x6, [RZ_MTU3_TGRA] = 0x8,
+		[RZ_MTU3_TGRB] = 0xa
+	},
+	{
+		[RZ_MTU3_TCNT] = 0x6, [RZ_MTU3_TGRA] = 0x8,
+		[RZ_MTU3_TGRB] = 0xa
+	},
+	{
+		[RZ_MTU3_TCNT] = 0x10, [RZ_MTU3_TGRA] = 0x18,
+		[RZ_MTU3_TGRB] = 0x1a, [RZ_MTU3_TGRC] = 0x24,
+		[RZ_MTU3_TGRD] = 0x26, [RZ_MTU3_TGRE] = 0x72
+	},
+	{
+		[RZ_MTU3_TCNT] = 0x11, [RZ_MTU3_TGRA] = 0x1b,
+		[RZ_MTU3_TGRB] = 0x1d, [RZ_MTU3_TGRC] = 0x27,
+		[RZ_MTU3_TGRD] = 0x29, [RZ_MTU3_TGRE] = 0x73,
+		[RZ_MTU3_TGRF] = 0x75, [RZ_MTU3_TADCR] = 0x3f,
+		[RZ_MTU3_TADCORA] = 0x43, [RZ_MTU3_TADCORB] = 0x45,
+		[RZ_MTU3_TADCOBRA] = 0x47,
+		[RZ_MTU3_TADCOBRB] = 0x49
+	},
+	{
+		[RZ_MTU3_TCNTU] = 0x0, [RZ_MTU3_TGRU] = 0x2,
+		[RZ_MTU3_TCNTV] = 0x10, [RZ_MTU3_TGRV] = 0x12,
+		[RZ_MTU3_TCNTW] = 0x20, [RZ_MTU3_TGRW] = 0x22
+	},
+	{
+		[RZ_MTU3_TCNT] = 0x10, [RZ_MTU3_TGRA] = 0x18,
+		[RZ_MTU3_TGRB] = 0x1a, [RZ_MTU3_TGRC] = 0x24,
+		[RZ_MTU3_TGRD] = 0x26, [RZ_MTU3_TGRE] = 0x72
+	},
+	{
+		[RZ_MTU3_TCNT] = 0x11, [RZ_MTU3_TGRA] = 0x1b,
+		[RZ_MTU3_TGRB] = 0x1d, [RZ_MTU3_TGRC] = 0x27,
+		[RZ_MTU3_TGRD] = 0x29, [RZ_MTU3_TGRE] = 0x73,
+		[RZ_MTU3_TGRF] = 0x75, [RZ_MTU3_TADCR] = 0x3f,
+		[RZ_MTU3_TADCORA] = 0x43, [RZ_MTU3_TADCORB] = 0x45,
+		[RZ_MTU3_TADCOBRA] = 0x47,
+		[RZ_MTU3_TADCOBRB] = 0x49
+	},
+};
+
+static const unsigned long rz_mtu3_32bit_ch_reg_offs[][5] = {
+	{
+		[RZ_MTU3_TCNTLW] = 0x20, [RZ_MTU3_TGRALW] = 0x24,
+		[RZ_MTU3_TGRBLW] = 0x28
+	},
+	{	[RZ_MTU3_TCNT] = 0x8, [RZ_MTU3_TGRA] = 0xc,
+		[RZ_MTU3_TGRB] = 0x10, [RZ_MTU3_TGRC] = 0x14,
+		[RZ_MTU3_TGRD] = 0x18
+	}
+};
+
+static bool rz_mtu3_is_16bit_shared_reg(u16 off)
+{
+	return (off == RZ_MTU3_TDDRA || off == RZ_MTU3_TDDRB ||
+		off == RZ_MTU3_TCDRA || off == RZ_MTU3_TCDRB ||
+		off == RZ_MTU3_TCBRA || off == RZ_MTU3_TCBRB ||
+		off == RZ_MTU3_TCNTSA || off == RZ_MTU3_TCNTSB);
+}
+
+u16 rz_mtu3_shared_reg_read(struct rz_mtu3_channel *ch, u16 off)
+{
+	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
+
+	if (rz_mtu3_is_16bit_shared_reg(off))
+		return readw(mtu->mmio + off);
+	else
+		return readb(mtu->mmio + off);
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_shared_reg_read);
+
+u8 rz_mtu3_8bit_ch_read(struct rz_mtu3_channel *ch, u16 off)
+{
+	u16 ch_offs;
+
+	ch_offs = rz_mtu3_8bit_ch_reg_offs[ch->index][off];
+	if (off != RZ_MTU3_TCR && ch_offs == 0)
+		return -EINVAL;
+
+	/*
+	 * NFCR register addresses on MTU{0,1,2,5,8} channels are smaller than
+	 * channel's base address.
+	 */
+	if (off == RZ_MTU3_NFCR && (ch->index <= RZ_MTU2 ||
+				    ch->index == RZ_MTU5 ||
+				    ch->index == RZ_MTU8))
+		return readb(ch->base - ch_offs);
+	else
+		return readb(ch->base + ch_offs);
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_8bit_ch_read);
+
+u16 rz_mtu3_16bit_ch_read(struct rz_mtu3_channel *ch, u16 off)
+{
+	u16 ch_offs;
+
+	/* MTU8 doesn't have 16-bit registers */
+	if (ch->index == RZ_MTU8)
+		return 0;
+
+	ch_offs = rz_mtu3_16bit_ch_reg_offs[ch->index][off];
+	if (ch->index != RZ_MTU5 && off != RZ_MTU3_TCNTU && ch_offs == 0)
+		return 0;
+
+	return readw(ch->base + ch_offs);
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_16bit_ch_read);
+
+u32 rz_mtu3_32bit_ch_read(struct rz_mtu3_channel *ch, u16 off)
+{
+	u16 ch_offs;
+
+	if (ch->index == RZ_MTU1)
+		ch_offs = rz_mtu3_32bit_ch_reg_offs[0][off];
+	else if (ch->index == RZ_MTU8)
+		ch_offs = rz_mtu3_32bit_ch_reg_offs[1][off];
+
+	if (!ch_offs)
+		return -EINVAL;
+
+	return readl(ch->base + ch_offs);
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_32bit_ch_read);
+
+void rz_mtu3_8bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u8 val)
+{
+	u16 ch_offs;
+
+	ch_offs = rz_mtu3_8bit_ch_reg_offs[ch->index][off];
+	if (ch->index != RZ_MTU5 && off != RZ_MTU3_TCR && ch_offs == 0)
+		return;
+
+	/*
+	 * NFCR register addresses on MTU{0,1,2,5,8} channels are smaller than
+	 * channel's base address.
+	 */
+	if (off == RZ_MTU3_NFCR && (ch->index <= RZ_MTU2 ||
+				    ch->index == RZ_MTU5 ||
+				    ch->index == RZ_MTU8))
+		writeb(val, ch->base - ch_offs);
+	else
+		writeb(val, ch->base + ch_offs);
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_8bit_ch_write);
+
+void rz_mtu3_16bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u16 val)
+{
+	u16 ch_offs;
+
+	/* MTU8 doesn't have 16-bit registers */
+	if (ch->index == RZ_MTU8)
+		return;
+
+	ch_offs = rz_mtu3_16bit_ch_reg_offs[ch->index][off];
+	if (ch->index != RZ_MTU5 && off != RZ_MTU3_TCNTU && ch_offs == 0)
+		return;
+
+	writew(val, ch->base + ch_offs);
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_16bit_ch_write);
+
+void rz_mtu3_32bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u32 val)
+{
+	u16 ch_offs;
+
+	if (ch->index == RZ_MTU1)
+		ch_offs = rz_mtu3_32bit_ch_reg_offs[0][off];
+	else if (ch->index == RZ_MTU8)
+		ch_offs = rz_mtu3_32bit_ch_reg_offs[1][off];
+
+	if (!ch_offs)
+		return;
+
+	writel(val, ch->base + ch_offs);
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_32bit_ch_write);
+
+void rz_mtu3_shared_reg_write(struct rz_mtu3_channel *ch, u16 off, u16 value)
+{
+	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
+
+	if (rz_mtu3_is_16bit_shared_reg(off))
+		writew(value, mtu->mmio + off);
+	else
+		writeb((u8)value, mtu->mmio + off);
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_shared_reg_write);
+
+static void rz_mtu3_start_stop_ch(struct rz_mtu3_channel *ch, bool start)
+{
+	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
+	unsigned long flags, value;
+	u8 offs;
+
+	/* start stop register shared by multiple timer channels */
+	raw_spin_lock_irqsave(&mtu->lock, flags);
+
+	if (ch->index == RZ_MTU6 || ch->index == RZ_MTU7) {
+		value = rz_mtu3_shared_reg_read(ch, RZ_MTU3_TSTRB);
+		if (start)
+			value |= 1 << ch->index;
+		else
+			value &= ~(1 << ch->index);
+		rz_mtu3_shared_reg_write(ch, RZ_MTU3_TSTRB, value);
+	} else if (ch->index != RZ_MTU5) {
+		value = rz_mtu3_shared_reg_read(ch, RZ_MTU3_TSTRA);
+		if (ch->index == RZ_MTU8)
+			offs = 0x08;
+		else if (ch->index < RZ_MTU3)
+			offs = 1 << ch->index;
+		else
+			offs = 1 << (ch->index + 3);
+		if (start)
+			value |= offs;
+		else
+			value &= ~offs;
+		rz_mtu3_shared_reg_write(ch, RZ_MTU3_TSTRA, value);
+	}
+
+	raw_spin_unlock_irqrestore(&mtu->lock, flags);
+}
+
+bool rz_mtu3_is_enabled(struct rz_mtu3_channel *ch)
+{
+	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
+	unsigned long flags, value;
+	bool ret = false;
+	u8 offs;
+
+	/* start stop register shared by multiple timer channels */
+	raw_spin_lock_irqsave(&mtu->lock, flags);
+
+	if (ch->index == RZ_MTU6 || ch->index == RZ_MTU7) {
+		value = rz_mtu3_shared_reg_read(ch, RZ_MTU3_TSTRB);
+		ret = value & (1 << ch->index);
+	} else if (ch->index != RZ_MTU5) {
+		value = rz_mtu3_shared_reg_read(ch, RZ_MTU3_TSTRA);
+		if (ch->index == RZ_MTU8)
+			offs = 0x08;
+		else if (ch->index < RZ_MTU3)
+			offs = 1 << ch->index;
+		else
+			offs = 1 << (ch->index + 3);
+
+		ret = value & offs;
+	}
+
+	raw_spin_unlock_irqrestore(&mtu->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_is_enabled);
+
+int rz_mtu3_enable(struct rz_mtu3_channel *ch)
+{
+	/* enable channel */
+	rz_mtu3_start_stop_ch(ch, true);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_enable);
+
+void rz_mtu3_disable(struct rz_mtu3_channel *ch)
+{
+	/* disable channel */
+	rz_mtu3_start_stop_ch(ch, false);
+}
+EXPORT_SYMBOL_GPL(rz_mtu3_disable);
+
+static const unsigned int ch_reg_offsets[] = {
+	0x100, 0x180, 0x200, 0x000, 0x001, 0xa80, 0x800, 0x801, 0x400
+};
+
+static void rz_mtu3_reset_assert(void *data)
+{
+	struct rz_mtu3 *mtu = dev_get_drvdata(data);
+
+	mfd_remove_devices(data);
+	reset_control_assert(mtu->rstc);
+}
+
+static const struct mfd_cell rz_mtu3_devs[] = {
+	{
+		.name = "rz-mtu3-counter",
+	},
+	{
+		.name = "pwm-rz-mtu3",
+	},
+};
+
+static int rz_mtu3_probe(struct platform_device *pdev)
+{
+	struct rz_mtu3 *ddata;
+	unsigned int i;
+	int ret;
+
+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	ddata->mmio = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ddata->mmio))
+		return PTR_ERR(ddata->mmio);
+
+	ddata->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(ddata->rstc))
+		return PTR_ERR(ddata->rstc);
+
+	ddata->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ddata->clk))
+		return PTR_ERR(ddata->clk);
+
+	reset_control_deassert(ddata->rstc);
+	raw_spin_lock_init(&ddata->lock);
+	platform_set_drvdata(pdev, ddata);
+
+	for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) {
+		ddata->channels[i].index = i;
+		ddata->channels[i].function = RZ_MTU3_NORMAL;
+		ddata->channels[i].base = ddata->mmio + ch_reg_offsets[i];
+	}
+
+	ret = mfd_add_devices(&pdev->dev, 0, rz_mtu3_devs,
+			      ARRAY_SIZE(rz_mtu3_devs), NULL, 0, NULL);
+	if (ret < 0)
+		goto err_assert;
+
+	return devm_add_action_or_reset(&pdev->dev, rz_mtu3_reset_assert,
+					&pdev->dev);
+
+err_assert:
+	reset_control_assert(ddata->rstc);
+	return ret;
+}
+
+static const struct of_device_id rz_mtu3_of_match[] = {
+	{ .compatible = "renesas,rz-mtu3", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rz_mtu3_of_match);
+
+static struct platform_driver rz_mtu3_driver = {
+	.probe = rz_mtu3_probe,
+	.driver	= {
+		.name = "rz-mtu3",
+		.of_match_table = rz_mtu3_of_match,
+	},
+};
+module_platform_driver(rz_mtu3_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a Core Driver");
+MODULE_LICENSE("GPL");
diff --git a/include/clocksource/rz-mtu3.h b/include/clocksource/rz-mtu3.h
new file mode 100644
index 000000000000..a9ab470499e7
--- /dev/null
+++ b/include/clocksource/rz-mtu3.h
@@ -0,0 +1,206 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ */
+#ifndef __LINUX_RZ_MTU3_H__
+#define __LINUX_RZ_MTU3_H__
+
+#include <linux/clk.h>
+
+/* 8-bit shared register offsets macros */
+#define RZ_MTU3_TSTRA	0x080 /* Timer start register A */
+#define RZ_MTU3_TSTRB	0x880 /* Timer start register B */
+
+/* 16-bit shared register offset macros */
+#define RZ_MTU3_TDDRA	0x016 /* Timer dead time data register A */
+#define RZ_MTU3_TDDRB	0x816 /* Timer dead time data register B */
+#define RZ_MTU3_TCDRA	0x014 /* Timer cycle data register A */
+#define RZ_MTU3_TCDRB	0x814 /* Timer cycle data register B */
+#define RZ_MTU3_TCBRA	0x022 /* Timer cycle buffer register A */
+#define RZ_MTU3_TCBRB	0x822 /* Timer cycle buffer register B */
+#define RZ_MTU3_TCNTSA	0x020 /* Timer subcounter A */
+#define RZ_MTU3_TCNTSB	0x820 /* Timer subcounter B */
+
+/*
+ * MTU5 contains 3 timer counter registers and is totaly different
+ * from other channels, so we must separate its offset
+ */
+
+/* 8-bit register offset macros of MTU3 channels except MTU5 */
+#define RZ_MTU3_TIER	0 /* Timer interrupt register */
+#define RZ_MTU3_NFCR	1 /* Noise filter control register */
+#define RZ_MTU3_TSR	2 /* Timer status register */
+#define RZ_MTU3_TCR	3 /* Timer control register */
+#define RZ_MTU3_TCR2	4 /* Timer control register 2 */
+#define RZ_MTU3_TMDR1	5 /* Timer mode register 1 */
+#define RZ_MTU3_TIOR	6 /* Timer I/O control register */
+#define RZ_MTU3_TIORH	6 /* Timer I/O control register H */
+#define RZ_MTU3_TIORL	7 /* Timer I/O control register L */
+/* Only MTU3/4/6/7 have TBTM registers */
+#define RZ_MTU3_TBTM	8 /* Timer buffer operation transfer mode register */
+
+/* 8-bit MTU5 register offset macros */
+#define RZ_MTU3_TSTR		2 /* MTU5 Timer start register */
+#define RZ_MTU3_TCNTCMPCLR	3 /* MTU5 Timer compare match clear register */
+#define RZ_MTU3_TCRU		4 /* Timer control register U */
+#define RZ_MTU3_TCR2U		5 /* Timer control register 2U */
+#define RZ_MTU3_TIORU		6 /* Timer I/O control register U */
+#define RZ_MTU3_TCRV		7 /* Timer control register V */
+#define RZ_MTU3_TCR2V		8 /* Timer control register 2V */
+#define RZ_MTU3_TIORV		9 /* Timer I/O control register V */
+#define RZ_MTU3_TCRW		10 /* Timer control register W */
+#define RZ_MTU3_TCR2W		11 /* Timer control register 2W */
+#define RZ_MTU3_TIORW		12 /* Timer I/O control register W */
+
+/* 16-bit register offset macros of MTU3 channels except MTU5 */
+#define RZ_MTU3_TCNT		0 /* Timer counter */
+#define RZ_MTU3_TGRA		1 /* Timer general register A */
+#define RZ_MTU3_TGRB		2 /* Timer general register B */
+#define RZ_MTU3_TGRC		3 /* Timer general register C */
+#define RZ_MTU3_TGRD		4 /* Timer general register D */
+#define RZ_MTU3_TGRE		5 /* Timer general register E */
+#define RZ_MTU3_TGRF		6 /* Timer general register F */
+/* Timer A/D converter start request registers */
+#define RZ_MTU3_TADCR		7 /* control register */
+#define RZ_MTU3_TADCORA		8 /* cycle set register A */
+#define RZ_MTU3_TADCORB		9 /* cycle set register B */
+#define RZ_MTU3_TADCOBRA	10 /* cycle set buffer register A */
+#define RZ_MTU3_TADCOBRB	11 /* cycle set buffer register B */
+
+/* 16-bit MTU5 register offset macros */
+#define RZ_MTU3_TCNTU		0 /* MTU5 Timer counter U */
+#define RZ_MTU3_TGRU		1 /* MTU5 Timer general register U */
+#define RZ_MTU3_TCNTV		2 /* MTU5 Timer counter V */
+#define RZ_MTU3_TGRV		3 /* MTU5 Timer general register V */
+#define RZ_MTU3_TCNTW		4 /* MTU5 Timer counter W */
+#define RZ_MTU3_TGRW		5 /* MTU5 Timer general register W */
+
+/* 32-bit register offset */
+#define RZ_MTU3_TCNTLW		0 /* Timer longword counter */
+#define RZ_MTU3_TGRALW		1 /* Timer longword general register A */
+#define RZ_MTU3_TGRBLW		2 /* Timer longowrd general register B */
+
+#define RZ_MTU3_TMDR3		0x191 /* MTU1 Timer Mode Register 3 */
+
+/* Macros for setting registers */
+#define RZ_MTU3_TCR_CCLR_TGRA	BIT(5)
+
+enum rz_mtu3_channels {
+	RZ_MTU0,
+	RZ_MTU1,
+	RZ_MTU2,
+	RZ_MTU3,
+	RZ_MTU4,
+	RZ_MTU5,
+	RZ_MTU6,
+	RZ_MTU7,
+	RZ_MTU8,
+	RZ_MTU_NUM_CHANNELS
+};
+
+enum rz_mtu3_functions {
+	RZ_MTU3_NORMAL,
+	RZ_MTU3_16BIT_PHASE_COUNTING,
+	RZ_MTU3_32BIT_PHASE_COUNTING,
+	RZ_MTU3_PWM_MODE_1,
+};
+
+/**
+ * struct rz_mtu3_channel - MTU3 channel private data
+ *
+ * @dev: device handle
+ * @index: channel index
+ * @base: channel base address
+ * @function: channel function
+ */
+struct rz_mtu3_channel {
+	struct device *dev;
+	unsigned int index;
+	void __iomem *base;
+	enum rz_mtu3_functions function;
+};
+
+/**
+ * struct rz_mtu3 - MTU3 MFD private data
+ *
+ * @clk: MTU3 module clock
+ * @mmio: MTU3 module clock
+ * @lock: Lock to protect shared register access
+ * @rz_mtu3_channel: HW channels
+ */
+struct rz_mtu3 {
+	void *priv_rz_mtu3;
+	void __iomem *mmio;
+	struct clk *clk;
+	struct reset_control *rstc;
+	raw_spinlock_t lock; /* Protect the shared registers */
+	struct rz_mtu3_channel channels[RZ_MTU_NUM_CHANNELS];
+};
+
+#if IS_ENABLED(CONFIG_RZ_MTU3)
+bool rz_mtu3_is_enabled(struct rz_mtu3_channel *ch);
+void rz_mtu3_disable(struct rz_mtu3_channel *ch);
+int rz_mtu3_enable(struct rz_mtu3_channel *ch);
+
+u8 rz_mtu3_8bit_ch_read(struct rz_mtu3_channel *ch, u16 off);
+u16 rz_mtu3_16bit_ch_read(struct rz_mtu3_channel *ch, u16 off);
+u32 rz_mtu3_32bit_ch_read(struct rz_mtu3_channel *ch, u16 off);
+u16 rz_mtu3_shared_reg_read(struct rz_mtu3_channel *ch, u16 off);
+
+void rz_mtu3_8bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u8 val);
+void rz_mtu3_16bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u16 val);
+void rz_mtu3_32bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u32 val);
+void rz_mtu3_shared_reg_write(struct rz_mtu3_channel *ch, u16 off, u16 val);
+#else
+static inline bool rz_mtu3_is_enabled(struct rz_mtu3_channel *ch)
+{
+	return false;
+}
+
+static inline void rz_mtu3_disable(struct rz_mtu3_channel *ch)
+{
+}
+
+static inline int rz_mtu3_enable(struct rz_mtu3_channel *ch)
+{
+	return 0;
+}
+
+static inline u8 rz_mtu3_8bit_ch_read(struct rz_mtu3_channel *ch, u16 off)
+{
+	return 0;
+}
+
+static inline u16 rz_mtu3_16bit_ch_read(struct rz_mtu3_channel *ch, u16 off)
+{
+	return 0;
+}
+
+static inline u32 rz_mtu3_32bit_ch_read(struct rz_mtu3_channel *ch, u16 off)
+{
+	return 0;
+}
+
+static inline u16 rz_mtu3_shared_reg_read(struct rz_mtu3_channel *ch, u16 off)
+{
+	return 0;
+}
+
+static inline void rz_mtu3_8bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u8 val)
+{
+}
+
+static inline void rz_mtu3_16bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u16 val)
+{
+}
+
+static inline void rz_mtu3_32bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u32 val)
+{
+}
+
+static inline void rz_mtu3_shared_reg_write(struct rz_mtu3_channel *ch, u16 off, u16 val)
+{
+}
+#endif
+
+#endif /* __LINUX_RZ_MTU3_H__ */
-- 
2.25.1


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

* [PATCH v6 3/5] Documentation: ABI: sysfs-bus-counter: add external_input_phase_clock_select & long_word_access_ctrl_mode items
  2022-11-13 17:15 [PATCH v6 0/5] Add RZ/G2L MTU3a Core, Counter and pwm driver Biju Das
  2022-11-13 17:15 ` [PATCH v6 1/5] dt-bindings: timer: Document RZ/G2L MTU3a bindings Biju Das
  2022-11-13 17:15 ` [PATCH v6 2/5] clocksource/drivers: Add Renesas RZ/G2L MTU3a core driver Biju Das
@ 2022-11-13 17:15 ` Biju Das
  2022-11-13 17:15 ` [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver Biju Das
  2022-11-13 17:15 ` [PATCH v6 5/5] pwm: Add Renesas RZ/G2L MTU3a PWM driver Biju Das
  4 siblings, 0 replies; 15+ messages in thread
From: Biju Das @ 2022-11-13 17:15 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: Biju Das, linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

This commit adds external_input_phase_clock_select and long_word_
access_ctrl_mode items to counter ABI file.
(e.g. for Renesas MTU3 hardware used for phase counting).

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v5->v6:
 * No change
v5:
 * New patch
---
 Documentation/ABI/testing/sysfs-bus-counter | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index ff83320b4255..2880f3b346e2 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -215,6 +215,22 @@ Contact:	linux-iio@vger.kernel.org
 Description:
 		This attribute indicates the number of overflows of count Y.
 
+What:		/sys/bus/counter/devices/counterX/external_input_phase_clock_select
+KernelVersion:	6.2
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute selects the external clock pin for phase
+		counting mode of counter X.
+
+What:		/sys/bus/counter/devices/counterX/long_word_access_ctrl_mode
+KernelVersion:	6.2
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute indicates the 16-bit or 32 bit-access of
+		counter X.
+
+What:		/sys/bus/counter/devices/counterX/external_input_phase_clock_select
+What:		/sys/bus/counter/devices/counterX/long_word_access_ctrl_mode
 What:		/sys/bus/counter/devices/counterX/countY/capture_component_id
 What:		/sys/bus/counter/devices/counterX/countY/ceiling_component_id
 What:		/sys/bus/counter/devices/counterX/countY/floor_component_id
-- 
2.25.1


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

* [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-13 17:15 [PATCH v6 0/5] Add RZ/G2L MTU3a Core, Counter and pwm driver Biju Das
                   ` (2 preceding siblings ...)
  2022-11-13 17:15 ` [PATCH v6 3/5] Documentation: ABI: sysfs-bus-counter: add external_input_phase_clock_select & long_word_access_ctrl_mode items Biju Das
@ 2022-11-13 17:15 ` Biju Das
  2022-11-14  3:47   ` William Breathitt Gray
  2022-11-13 17:15 ` [PATCH v6 5/5] pwm: Add Renesas RZ/G2L MTU3a PWM driver Biju Das
  4 siblings, 1 reply; 15+ messages in thread
From: Biju Das @ 2022-11-13 17:15 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: Biju Das, linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

Add RZ/G2L MTU3a counter driver. This IP supports the following
phase counting modes on MTU1 and MTU2 channels

1) 16-bit phase counting modes on MTU1 and MTU2 channels.
2) 32-bit phase counting mode by cascading MTU1 and MTU2.

This patch adds 3 counters by creating 3 logical channels
	counter0: 16-bit phase counter on MTU1 channel
	counter1: 16-bit phase counter on MTU2 channel
	counter2: 32-bit phase counter by cascading MTU1 and MTU2
		  channels.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v5->v6:
 * Updated KConfig and commit description
 * Sorted header
 * Fixed RZ_MTU3_GET_HW_CH Macro for argument reuse 'id' - 
   possible side-effects?
 * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed
   __maybe_unused from suspend/resume()
v4->v5:
 * Updated the Kconfig with SoC vendor name
 * Introduced rz_mtu3_is_counter_invalid()
 * replaced pointer to an array of struct rz_mtu3_channel with
   a simple pointer to struct rz_mtu3_channel.
 * Added long_word_access_ctrl_mode sysfs entry for 16-bit and
   32-bit access
 * Added external_input_phase_clock_select sysfs entry for
   selecting input clocks.
 * used preprocessor defines represent SIGNAL_{A,B,C,D}_ID instead of
   signal ids.
v3->v4:
 * There is no resource associated with "rz-mtu3-counter" compatible
   and moved the code to mfd subsystem as it binds against "rz-mtu".
 * Removed struct platform_driver rz_mtu3_cnt_driver.
 * Updated commit description
 * Updated Kconfig description
 * Added macros RZ_MTU3_16_BIT_MTU{1,2}_CH for MTU1 and MTU2 channels
 * Added RZ_MTU3_GET_HW_CH macro for getting channel ID.
 * replaced priv->ch[id]->priv->ch[0] in rz_mtu3_count_read()
 * Cached counter max values
 * replaced cnt->tsr in rz_mtu3_count_direction_read()
 * Added comments for RZ_MTU3_TCR_CCLR_NONE
 * Replaced if with switch in rz_mtu3_initialize_counter() and
   rz_mtu3_count_ceiling_write()
 * Added locks in initialize, terminate and enable_read to prevent races.
 * Updated rz_mtu3_action_read to take care of MTU2 signals.
 * Added separate distinct array for each group of Synapse.
 * Moved pm handling to parent.

v1->v3:
 * Modelled as a counter device supporting 3 counters(2 16-bit and 
   32-bit)
 * Add kernel-doc comments to document struct rz_mtu3_cnt
 * Removed mmio variable from struct rz_mtu3_cnt
 * Removed cnt local variable from rz_mtu3_count_read()
 * Replaced -EINVAL->-ERANGE for out of range error conditions.
 * Removed explicit cast from write functions.
 * Removed local variable val from rz_mtu3_count_ceiling_read()
 * Added lock for RMW for counter/ceiling updates.
 * Added different synapses for counter0 and counter{1,2}
 * Used ARRAY for assigning num_counts.
 * Added PM runtime for managing clocks.
 * Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.
---
 drivers/counter/Kconfig       |  11 +
 drivers/counter/Makefile      |   1 +
 drivers/counter/rz-mtu3-cnt.c | 717 ++++++++++++++++++++++++++++++++++
 3 files changed, 729 insertions(+)
 create mode 100644 drivers/counter/rz-mtu3-cnt.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index d388bf26f4dc..daa352c7392d 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -39,6 +39,17 @@ config INTERRUPT_CNT
 	  To compile this driver as a module, choose M here: the
 	  module will be called interrupt-cnt.
 
+config RZ_MTU3_CNT
+	tristate "Renesas RZ/G2L MTU3a counter driver"
+	depends on RZ_MTU3 || COMPILE_TEST
+	help
+	  Enable support for MTU3a counter driver found on Renesas RZ/G2L alike
+	  SoCs. This IP supports both 16-bit and 32-bit phase counting mode
+	  support.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called rz-mtu3-cnt.
+
 config STM32_TIMER_CNT
 	tristate "STM32 Timer encoder counter driver"
 	depends on MFD_STM32_TIMERS || COMPILE_TEST
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index b9a369e0d4fc..933fdd50b3e4 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -8,6 +8,7 @@ counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
 
 obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_INTERRUPT_CNT)		+= interrupt-cnt.o
+obj-$(CONFIG_RZ_MTU3_CNT)	+= rz-mtu3-cnt.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
new file mode 100644
index 000000000000..cfddbbe38ed1
--- /dev/null
+++ b/drivers/counter/rz-mtu3-cnt.c
@@ -0,0 +1,717 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L MTU3a Counter driver
+ *
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ */
+
+#include <linux/clk.h>
+#include <linux/counter.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/types.h>
+
+#include <clocksource/rz-mtu3.h>
+
+#define RZ_MTU3_TSR_TCFD	BIT(7)
+#define RZ_MTU3_MAX_HW_CNTR_CHANNELS	(2)
+
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_1	(4)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_2	(5)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_3	(6)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_4	(7)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_5	(9)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_MASK	(0xf)
+
+#define RZ_MTU3_TMDR3_LWA	BIT(0)
+#define RZ_MTU3_TMDR3_PHCKSEL	BIT(1)
+
+#define RZ_MTU3_16_BIT_MTU1_CH	(0)
+#define RZ_MTU3_16_BIT_MTU2_CH	(1)
+#define RZ_MTU3_32_BIT_CH		(2)
+
+#define RZ_MTU3_TIOR_NO_OUTPUT	(0)
+#define RZ_MTU3_TIOR_IC_BOTH	(10)
+
+#define RZ_MTU3_GET_HW_CH(id) \
+({ \
+	size_t _id = (id); _id = (_id == RZ_MTU3_32_BIT_CH) ? 0 : _id; \
+})
+
+#define SIGNAL_A_ID	(0)
+#define SIGNAL_B_ID	(1)
+#define SIGNAL_C_ID	(2)
+#define SIGNAL_D_ID	(3)
+
+/**
+ * struct rz_mtu3_cnt - MTU3 counter private data
+ *
+ * @clk: MTU3 module clock
+ * @lock: Lock to prevent concurrent access for ceiling and count
+ * @ch: HW channels for the counters
+ * @mtu_16bit_max: Cache for 16-bit counters
+ * @mtu_32bit_max: Cache for 32-bit counters
+ */
+struct rz_mtu3_cnt {
+	struct clk *clk;
+	struct mutex lock;
+	struct rz_mtu3_channel *ch;
+	u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
+	u32 mtu_32bit_max;
+};
+
+static const enum counter_function rz_mtu3_count_functions[] = {
+	COUNTER_FUNCTION_QUADRATURE_X4,
+	COUNTER_FUNCTION_PULSE_DIRECTION,
+	COUNTER_FUNCTION_QUADRATURE_X2_B,
+};
+
+static bool rz_mtu3_is_counter_invalid(struct counter_device *counter, int id)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u16 val;
+
+	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3) & RZ_MTU3_TMDR3_LWA;
+	if (id == RZ_MTU3_32_BIT_CH && val)
+		return false;
+
+	if (id != RZ_MTU3_32_BIT_CH && !val)
+		return false;
+
+	return true;
+}
+
+static int rz_mtu3_count_read(struct counter_device *counter,
+			      struct counter_count *count, u64 *val)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+
+	if (rz_mtu3_is_counter_invalid(counter, count->id))
+		return -EINVAL;
+
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		*val = rz_mtu3_32bit_ch_read(priv->ch + ch_id, RZ_MTU3_TCNTLW);
+	else
+		*val = rz_mtu3_16bit_ch_read(priv->ch + ch_id, RZ_MTU3_TCNT);
+
+	return 0;
+}
+
+static int rz_mtu3_count_write(struct counter_device *counter,
+			       struct counter_count *count, const u64 val)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+	u32 ceiling;
+
+	if (rz_mtu3_is_counter_invalid(counter, count->id))
+		return -EINVAL;
+
+	mutex_lock(&priv->lock);
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		ceiling = priv->mtu_32bit_max;
+	else
+		ceiling = priv->mtu_16bit_max[ch_id];
+
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		rz_mtu3_32bit_ch_write(priv->ch + ch_id, RZ_MTU3_TCNTLW, val);
+	else
+		rz_mtu3_16bit_ch_write(priv->ch + ch_id, RZ_MTU3_TCNT, val);
+
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static int rz_mtu3_count_function_read(struct counter_device *counter,
+				       struct counter_count *count,
+				       enum counter_function *function)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+	u8 val;
+
+	val = rz_mtu3_8bit_ch_read(priv->ch + ch_id, RZ_MTU3_TMDR1);
+
+	switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
+	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
+		*function = COUNTER_FUNCTION_QUADRATURE_X4;
+		break;
+	case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
+		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
+		break;
+	case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
+		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rz_mtu3_count_function_write(struct counter_device *counter,
+					struct counter_count *count,
+					enum counter_function function)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+	u8 mode;
+
+	switch (function) {
+	case COUNTER_FUNCTION_QUADRATURE_X4:
+		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_1;
+		break;
+	case COUNTER_FUNCTION_PULSE_DIRECTION:
+		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_2;
+		break;
+	case COUNTER_FUNCTION_QUADRATURE_X2_B:
+		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_4;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	rz_mtu3_8bit_ch_write(priv->ch + ch_id, RZ_MTU3_TMDR1, mode);
+
+	return 0;
+}
+
+static int rz_mtu3_count_direction_read(struct counter_device *counter,
+					struct counter_count *count,
+					enum counter_count_direction *direction)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+	u8 tsr;
+
+	tsr = rz_mtu3_8bit_ch_read(priv->ch + ch_id, RZ_MTU3_TSR);
+
+	if (tsr & RZ_MTU3_TSR_TCFD)
+		*direction = COUNTER_COUNT_DIRECTION_FORWARD;
+	else
+		*direction = COUNTER_COUNT_DIRECTION_BACKWARD;
+
+	return 0;
+}
+
+static int rz_mtu3_count_ceiling_read(struct counter_device *counter,
+				      struct counter_count *count,
+				      u64 *ceiling)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		*ceiling = priv->mtu_32bit_max;
+	else
+		*ceiling = priv->mtu_16bit_max[ch_id];
+
+	return 0;
+}
+
+static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
+				       struct counter_count *count,
+				       u64 ceiling)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+
+	if (rz_mtu3_is_counter_invalid(counter, count->id))
+		return -EINVAL;
+
+	switch (count->id) {
+	case RZ_MTU3_16_BIT_MTU1_CH:
+	case RZ_MTU3_16_BIT_MTU2_CH:
+		if (ceiling > U16_MAX)
+			return -ERANGE;
+		priv->mtu_16bit_max[ch_id] = ceiling;
+		break;
+	case RZ_MTU3_32_BIT_CH:
+		if (ceiling > U32_MAX)
+			return -ERANGE;
+		priv->mtu_32bit_max = ceiling;
+		break;
+	}
+
+	mutex_lock(&priv->lock);
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		rz_mtu3_32bit_ch_write(priv->ch + ch_id, RZ_MTU3_TGRALW, ceiling);
+	else
+		rz_mtu3_16bit_ch_write(priv->ch + ch_id, RZ_MTU3_TGRA, ceiling);
+
+	rz_mtu3_8bit_ch_write(priv->ch + ch_id, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter, int id)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	struct rz_mtu3_channel *ch1 = priv->ch;
+	struct rz_mtu3_channel *ch2 = ch1 + 1;
+
+	/*
+	 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit cascade
+	 * counter.
+	 */
+	ch1->function = RZ_MTU3_32BIT_PHASE_COUNTING;
+	ch2->function = RZ_MTU3_32BIT_PHASE_COUNTING;
+
+	/* Phase counting mode 1 is used as default in initialization. */
+	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_PH_CNT_MODE_1);
+
+	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
+	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_IC_BOTH);
+
+	rz_mtu3_enable(ch1);
+	rz_mtu3_enable(ch2);
+}
+
+static void rz_mtu3_16bit_cnt_setting(struct counter_device *counter, int id)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	struct rz_mtu3_channel *ch = priv->ch + id;
+
+	ch->function = RZ_MTU3_16BIT_PHASE_COUNTING;
+
+	/* Phase counting mode 1 is used as default in initialization. */
+	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_PH_CNT_MODE_1);
+
+	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
+	rz_mtu3_enable(ch);
+}
+
+static int rz_mtu3_initialize_counter(struct counter_device *counter, int id)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	struct rz_mtu3_channel *ch1 = priv->ch;
+	struct rz_mtu3_channel *ch2 = ch1 + 1;
+
+	mutex_lock(&priv->lock);
+	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_NO_OUTPUT);
+	switch (id) {
+	case RZ_MTU3_16_BIT_MTU1_CH:
+	case RZ_MTU3_16_BIT_MTU2_CH:
+		if ((priv->ch + id)->function != RZ_MTU3_NORMAL) {
+			mutex_unlock(&priv->lock);
+			return -EBUSY;
+		}
+
+		rz_mtu3_16bit_cnt_setting(counter, id);
+		break;
+	case RZ_MTU3_32_BIT_CH:
+		if (ch1->function != RZ_MTU3_NORMAL ||
+		    ch2->function != RZ_MTU3_NORMAL) {
+			mutex_unlock(&priv->lock);
+			return -EBUSY;
+		}
+		rz_mtu3_32bit_cnt_setting(counter, id);
+		break;
+	}
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static void rz_mtu3_terminate_counter(struct counter_device *counter, int id)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	struct rz_mtu3_channel *ch1 = priv->ch;
+	struct rz_mtu3_channel *ch2 = ch1 + 1;
+
+	mutex_lock(&priv->lock);
+	if (id == RZ_MTU3_32_BIT_CH) {
+		ch1->function = RZ_MTU3_NORMAL;
+		ch2->function = RZ_MTU3_NORMAL;
+		rz_mtu3_disable(ch2);
+		rz_mtu3_disable(ch1);
+	} else {
+		(priv->ch + id)->function = RZ_MTU3_NORMAL;
+		rz_mtu3_disable(priv->ch + id);
+	}
+	mutex_unlock(&priv->lock);
+}
+
+static int rz_mtu3_count_enable_read(struct counter_device *counter,
+				     struct counter_count *count, u8 *enable)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	struct rz_mtu3_channel *ch1 = priv->ch;
+	struct rz_mtu3_channel *ch2 = ch1 + 1;
+
+	if (count->id == RZ_MTU3_32_BIT_CH) {
+		mutex_lock(&priv->lock);
+		*enable = rz_mtu3_is_enabled(ch1) &&
+			rz_mtu3_is_enabled(ch2);
+		mutex_unlock(&priv->lock);
+	} else {
+		*enable = rz_mtu3_is_enabled(priv->ch + count->id);
+	}
+
+	return 0;
+}
+
+static int rz_mtu3_count_enable_write(struct counter_device *counter,
+				      struct counter_count *count, u8 enable)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+	struct rz_mtu3_channel *ch = priv->ch + ch_id;
+	int ret = 0;
+
+	if (enable) {
+		pm_runtime_get_sync(ch->dev);
+		ret = rz_mtu3_initialize_counter(counter, count->id);
+	} else {
+		rz_mtu3_terminate_counter(counter, count->id);
+		pm_runtime_put(ch->dev);
+	}
+
+	return ret;
+}
+
+static int rz_mtu3_long_word_access_ctrl_mode_get(struct counter_device *counter,
+						  u32 *lwa_ctrl_mode)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u16 val;
+
+	pm_runtime_get_sync(priv->ch->dev);
+	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
+	*lwa_ctrl_mode = val & RZ_MTU3_TMDR3_LWA;
+	pm_runtime_put(priv->ch->dev);
+
+	return 0;
+}
+
+static int rz_mtu3_long_word_access_ctrl_mode_set(struct counter_device *counter,
+						  u32 lwa_ctrl_mode)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u16 val;
+
+	pm_runtime_get_sync(priv->ch->dev);
+	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
+	if (lwa_ctrl_mode)
+		val |= RZ_MTU3_TMDR3_LWA;
+	else
+		val &= ~RZ_MTU3_TMDR3_LWA;
+
+	rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, val);
+	pm_runtime_put(priv->ch->dev);
+
+	return 0;
+}
+
+static int rz_mtu3_ext_input_phase_clock_select_get(struct counter_device *counter,
+						    u32 *ext_input_phase_clock_select)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u16 val;
+
+	pm_runtime_get_sync(priv->ch->dev);
+	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
+	*ext_input_phase_clock_select = (val & RZ_MTU3_TMDR3_PHCKSEL) >> 1;
+	pm_runtime_put(priv->ch->dev);
+
+	return 0;
+}
+
+static int rz_mtu3_ext_input_phase_clock_select_set(struct counter_device *counter,
+						    u32 ext_input_phase_clock_select)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u16 val;
+
+	pm_runtime_get_sync(priv->ch->dev);
+	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
+	if (ext_input_phase_clock_select)
+		val |= RZ_MTU3_TMDR3_PHCKSEL;
+	else
+		val &= ~RZ_MTU3_TMDR3_PHCKSEL;
+
+	rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, val);
+	pm_runtime_put(priv->ch->dev);
+
+	return 0;
+}
+
+static struct counter_comp rz_mtu3_count_ext[] = {
+	COUNTER_COMP_DIRECTION(rz_mtu3_count_direction_read),
+	COUNTER_COMP_ENABLE(rz_mtu3_count_enable_read,
+			    rz_mtu3_count_enable_write),
+	COUNTER_COMP_CEILING(rz_mtu3_count_ceiling_read,
+			     rz_mtu3_count_ceiling_write),
+};
+
+static const enum counter_synapse_action rz_mtu3_synapse_actions[] = {
+	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+	COUNTER_SYNAPSE_ACTION_NONE,
+};
+
+static int rz_mtu3_action_read(struct counter_device *counter,
+			       struct counter_count *count,
+			       struct counter_synapse *synapse,
+			       enum counter_synapse_action *action)
+{
+	enum counter_function function;
+	int err;
+
+	err = rz_mtu3_count_function_read(counter, count, &function);
+	if (err)
+		return err;
+
+	/* Default action mode */
+	*action = COUNTER_SYNAPSE_ACTION_NONE;
+
+	switch (function) {
+	case COUNTER_FUNCTION_PULSE_DIRECTION:
+		/*
+		 * Rising edges on signal A updates the respective count.
+		 * The input level of signal B determines direction.
+		 */
+		if (synapse->signal->id == SIGNAL_A_ID ||
+		    synapse->signal->id == SIGNAL_C_ID)
+			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
+		break;
+	case COUNTER_FUNCTION_QUADRATURE_X2_B:
+		/*
+		 * Any state transition on quadrature pair signal B updates
+		 * the respective count.
+		 */
+		if (synapse->signal->id == SIGNAL_B_ID ||
+		    synapse->signal->id == SIGNAL_D_ID)
+			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
+		break;
+	case COUNTER_FUNCTION_QUADRATURE_X4:
+		/* counts up/down on both edges of A and B signal*/
+		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct counter_ops rz_mtu3_cnt_ops = {
+	.count_read = rz_mtu3_count_read,
+	.count_write = rz_mtu3_count_write,
+	.function_read = rz_mtu3_count_function_read,
+	.function_write = rz_mtu3_count_function_write,
+	.action_read = rz_mtu3_action_read,
+};
+
+#define RZ_MTU3_PHASE_SIGNAL(_id, _name) {		\
+	.id = (_id),				\
+	.name = (_name),			\
+}
+
+static struct counter_signal rz_mtu3_signals[] = {
+	RZ_MTU3_PHASE_SIGNAL(SIGNAL_A_ID, "MTU1 MTCLKA"),
+	RZ_MTU3_PHASE_SIGNAL(SIGNAL_B_ID, "MTU1 MTCLKB"),
+	RZ_MTU3_PHASE_SIGNAL(SIGNAL_C_ID, "MTU2 MTCLKC"),
+	RZ_MTU3_PHASE_SIGNAL(SIGNAL_D_ID, "MTU2 MTCLKD"),
+};
+
+static struct counter_synapse rz_mtu3_mtu1_count_synapses[] = {
+	{
+		.actions_list = rz_mtu3_synapse_actions,
+		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
+		.signal = rz_mtu3_signals,
+	},
+	{
+		.actions_list = rz_mtu3_synapse_actions,
+		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
+		.signal = rz_mtu3_signals + 1,
+	}
+};
+
+static struct counter_synapse rz_mtu3_mtu2_count_synapses[] = {
+	{
+		.actions_list = rz_mtu3_synapse_actions,
+		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
+		.signal = rz_mtu3_signals,
+	},
+	{
+		.actions_list = rz_mtu3_synapse_actions,
+		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
+		.signal = rz_mtu3_signals + 1,
+	},
+	{
+		.actions_list = rz_mtu3_synapse_actions,
+		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
+		.signal = rz_mtu3_signals + 2,
+	},
+	{
+		.actions_list = rz_mtu3_synapse_actions,
+		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
+		.signal = rz_mtu3_signals + 3,
+	}
+};
+
+static struct counter_count rz_mtu3_counts[] = {
+	{
+		.id = RZ_MTU3_16_BIT_MTU1_CH,
+		.name = "Channel 1 Count",
+		.functions_list = rz_mtu3_count_functions,
+		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
+		.synapses = rz_mtu3_mtu1_count_synapses,
+		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu1_count_synapses),
+		.ext = rz_mtu3_count_ext,
+		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
+	},
+	{
+		.id = RZ_MTU3_16_BIT_MTU2_CH,
+		.name = "Channel 2 Count",
+		.functions_list = rz_mtu3_count_functions,
+		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
+		.synapses = rz_mtu3_mtu2_count_synapses,
+		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu2_count_synapses),
+		.ext = rz_mtu3_count_ext,
+		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
+	},
+	{
+		.id = RZ_MTU3_32_BIT_CH,
+		.name = "Channel 1 and 2 (combined) Count",
+		.functions_list = rz_mtu3_count_functions,
+		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
+		.synapses = rz_mtu3_mtu2_count_synapses,
+		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu2_count_synapses),
+		.ext = rz_mtu3_count_ext,
+		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
+	}
+};
+
+static const char *const rz_mtu3_long_word_access_ctrl_modes[] = {
+	"16-bit",
+	"32-bit",
+};
+
+static DEFINE_COUNTER_ENUM(rz_mtu3_long_word_access_ctrl_mode_enum,
+			   rz_mtu3_long_word_access_ctrl_modes);
+
+static const char *const rz_mtu3_ext_input_phase_clock_select[] = {
+	"MTCLKA-MTCLKB",
+	"MTCLKC-MTCLKD",
+};
+
+static DEFINE_COUNTER_ENUM(rz_mtu3_ext_input_phase_clock_select_enum,
+			   rz_mtu3_ext_input_phase_clock_select);
+
+static struct counter_comp rz_mtu3_device_ext[] = {
+	COUNTER_COMP_DEVICE_ENUM("long_word_access_ctrl_mode",
+				 rz_mtu3_long_word_access_ctrl_mode_get,
+				 rz_mtu3_long_word_access_ctrl_mode_set,
+				 rz_mtu3_long_word_access_ctrl_mode_enum),
+	COUNTER_COMP_DEVICE_ENUM("external_input_phase_clock_select",
+				 rz_mtu3_ext_input_phase_clock_select_get,
+				 rz_mtu3_ext_input_phase_clock_select_set,
+				 rz_mtu3_ext_input_phase_clock_select_enum),
+};
+
+static int rz_mtu3_cnt_pm_runtime_suspend(struct device *dev)
+{
+	struct clk *const clk = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(clk);
+
+	return 0;
+}
+
+static int rz_mtu3_cnt_pm_runtime_resume(struct device *dev)
+{
+	struct clk *const clk = dev_get_drvdata(dev);
+
+	clk_prepare_enable(clk);
+
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(rz_mtu3_cnt_pm_ops,
+				 rz_mtu3_cnt_pm_runtime_suspend,
+				 rz_mtu3_cnt_pm_runtime_resume, NULL);
+
+static void rz_mtu3_cnt_pm_disable(void *data)
+{
+	struct device *dev = data;
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+}
+
+static int rz_mtu3_cnt_probe(struct platform_device *pdev)
+{
+	struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct counter_device *counter;
+	struct rz_mtu3_cnt *priv;
+	unsigned int i;
+	int ret;
+
+	counter = devm_counter_alloc(dev, sizeof(*priv));
+	if (!counter)
+		return -ENOMEM;
+
+	priv = counter_priv(counter);
+	priv->clk = ddata->clk;
+	priv->mtu_32bit_max = U32_MAX;
+	priv->ch = &ddata->channels[RZ_MTU1];
+	for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
+		(priv->ch + i)->dev = dev;
+		priv->mtu_16bit_max[i] = U16_MAX;
+	}
+
+	mutex_init(&priv->lock);
+	platform_set_drvdata(pdev, priv->clk);
+	clk_prepare_enable(priv->clk);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	ret = devm_add_action_or_reset(&pdev->dev, rz_mtu3_cnt_pm_disable, dev);
+	if (ret < 0)
+		goto disable_clock;
+
+	counter->name = dev_name(dev);
+	counter->parent = dev;
+	counter->ops = &rz_mtu3_cnt_ops;
+	counter->counts = rz_mtu3_counts;
+	counter->num_counts = ARRAY_SIZE(rz_mtu3_counts);
+	counter->signals = rz_mtu3_signals;
+	counter->num_signals = ARRAY_SIZE(rz_mtu3_signals);
+	counter->ext = rz_mtu3_device_ext;
+	counter->num_ext = ARRAY_SIZE(rz_mtu3_device_ext);
+
+	/* Register Counter device */
+	ret = devm_counter_add(dev, counter);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to add counter\n");
+		goto disable_clock;
+	}
+
+	return 0;
+
+disable_clock:
+	clk_disable_unprepare(priv->clk);
+
+	return ret;
+}
+
+static struct platform_driver rz_mtu3_cnt_driver = {
+	.probe = rz_mtu3_cnt_probe,
+	.driver = {
+		.name = "rz-mtu3-counter",
+		.pm = pm_ptr(&rz_mtu3_cnt_pm_ops),
+	},
+};
+module_platform_driver(rz_mtu3_cnt_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_ALIAS("platform:rz-mtu3-counter");
+MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(COUNTER);
-- 
2.25.1


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

* [PATCH v6 5/5] pwm: Add Renesas RZ/G2L MTU3a PWM driver
  2022-11-13 17:15 [PATCH v6 0/5] Add RZ/G2L MTU3a Core, Counter and pwm driver Biju Das
                   ` (3 preceding siblings ...)
  2022-11-13 17:15 ` [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver Biju Das
@ 2022-11-13 17:15 ` Biju Das
  4 siblings, 0 replies; 15+ messages in thread
From: Biju Das @ 2022-11-13 17:15 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Biju Das, Uwe Kleine-König, linux-pwm, Geert Uytterhoeven,
	Chris Paterson, Prabhakar Mahadev Lad, linux-renesas-soc

Add support for RZ/G2L MTU3a PWM driver. The IP supports
following PWM modes

1) PWM mode{1,2}
2) Reset-synchronized PWM mode
3) Complementary PWM mode{1,2,3}

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

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v5->v6:
 * Updated commit and Kconfig description
 * Sorted the header
 * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable()
 * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed
   __maybe_unused from suspend/resume()
v4->v5:
 * pwm device is instantiated by mtu3a core driver.
v3->v4:
 * There is no resource associated with "rz-mtu3-pwm" compatible
   and moved the code to mfd subsystem as it binds against "rz-mtu".
 * Removed struct platform_driver rz_mtu3_pwm_driver.
v2->v3:
 * No change.
v1->v2:
 * Modelled as a single PWM device handling multiple channles.
 * Used PM framework to manage the clocks.
---
 drivers/pwm/Kconfig       |  11 +
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-rz-mtu3.c | 455 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 467 insertions(+)
 create mode 100644 drivers/pwm/pwm-rz-mtu3.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..18a7cc6a7fc9 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_RZ_MTU3
+	tristate "Renesas RZ/G2L MTU3a PWM Timer support"
+	depends on RZ_MTU3 || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This driver exposes the MTU3a 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-rz-mtu3.
+
 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..b85fc9fba326 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_RZ_MTU3)	+= pwm-rz-mtu3.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-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
new file mode 100644
index 000000000000..838d94604788
--- /dev/null
+++ b/drivers/pwm/pwm-rz-mtu3.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L MTU3a PWM Timer 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:
+ * - When PWM is disabled, the output is driven to Hi-Z.
+ * - While the hardware supports both polarities, the driver (for now)
+ *   only handles normal polarity.
+ * - While the hardware supports pwm mode{1,2}, reset-synchronized pwm and
+ *   complementary pwm modes, the driver (for now) only handles pwm mode1.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/time.h>
+
+#include <clocksource/rz-mtu3.h>
+
+#define RZ_MTU3_TMDR1_MD_NORMAL		(0)
+#define RZ_MTU3_TMDR1_MD_PWM_MODE_1	(2)
+
+#define RZ_MTU3_TIOR_OC_RETAIN		(0)
+#define RZ_MTU3_TIOR_OC_0_H_COMP_MATCH	(2)
+#define RZ_MTU3_TIOR_OC_1_TOGGLE	(7)
+#define RZ_MTU3_TIOR_OC_IOA		GENMASK(3, 0)
+
+#define RZ_MTU3_TCR_CCLR_TGRC		(5 << 5)
+#define RZ_MTU3_TCR_CKEG_RISING		(0 << 3)
+
+#define RZ_MTU3_TCR_TPCS		GENMASK(2, 0)
+
+#define RZ_MTU3_MAX_PWM_MODE1_CHANNELS	(12)
+
+#define RZ_MTU3_MAX_HW_PWM_CHANNELS	(7)
+
+static const u8 rz_mtu3_pwm_mode1_num_ios[] = { 2, 1, 1, 2, 2, 2, 2 };
+
+/**
+ * struct rz_mtu3_pwm_chip - MTU3 pwm private data
+ *
+ * @chip: MTU3 pwm chip data
+ * @clk: MTU3 module clock
+ * @lock: Lock to prevent concurrent access for usage count
+ * @rate: MTU3 clock rate
+ * @user_count: MTU3 usage count
+ * @rz_mtu3_channel: HW channels for the PWM
+ */
+
+struct rz_mtu3_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	struct mutex lock;
+	unsigned long rate;
+	u32 user_count[RZ_MTU3_MAX_HW_PWM_CHANNELS];
+	struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_PWM_CHANNELS];
+};
+
+static inline struct rz_mtu3_pwm_chip *to_rz_mtu3_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct rz_mtu3_pwm_chip, chip);
+}
+
+static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip *rz_mtu3,
+					 u64 period_cycles)
+{
+	u32 prescaled_period_cycles;
+	u8 prescale;
+
+	prescaled_period_cycles = period_cycles >> 16;
+	if (prescaled_period_cycles >= 16)
+		prescale = 3;
+	else
+		prescale = (fls(prescaled_period_cycles) + 1) / 2;
+
+	return prescale;
+}
+
+static struct rz_mtu3_channel *
+rz_mtu3_get_hw_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 channel)
+{
+	unsigned int i, ch_index = 0;
+
+	for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) {
+		ch_index += rz_mtu3_pwm_mode1_num_ios[i];
+
+		if (ch_index > channel)
+			break;
+	}
+
+	return rz_mtu3_pwm->ch[i];
+}
+
+static u32 rz_mtu3_get_hw_channel_index(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
+					struct rz_mtu3_channel *ch)
+{
+	u32 i;
+
+	for (i = 0; i < ARRAY_SIZE(rz_mtu3_pwm_mode1_num_ios); i++) {
+		if (ch == rz_mtu3_pwm->ch[i])
+			break;
+	}
+
+	return i;
+}
+
+static bool rz_mtu3_pwm_is_second_channel(u32 ch_index, u32 hwpwm)
+{
+	u32 i, pwm_ch_index = 0;
+
+	for (i = 0; i < ch_index; i++)
+		pwm_ch_index += rz_mtu3_pwm_mode1_num_ios[i];
+
+	return pwm_ch_index != hwpwm;
+}
+
+static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
+				      u32 hwpwm)
+{
+	struct rz_mtu3_channel *ch;
+	bool is_channel_en;
+	u32 ch_index;
+	u8 val;
+
+	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, hwpwm);
+	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
+	is_channel_en = rz_mtu3_is_enabled(ch);
+
+	if (rz_mtu3_pwm_is_second_channel(ch_index, hwpwm))
+		val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORL);
+	else
+		val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORH);
+
+	return (is_channel_en && (val & RZ_MTU3_TIOR_OC_IOA));
+}
+
+static int rz_mtu3_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
+	struct rz_mtu3_channel *ch;
+	u32 ch_index;
+
+	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
+	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
+
+	mutex_lock(&rz_mtu3_pwm->lock);
+	rz_mtu3_pwm->user_count[ch_index]++;
+	mutex_unlock(&rz_mtu3_pwm->lock);
+
+	ch->function = RZ_MTU3_PWM_MODE_1;
+
+	return 0;
+}
+
+static void rz_mtu3_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
+	struct rz_mtu3_channel *ch;
+	u32 ch_index;
+
+	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
+	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
+
+	mutex_lock(&rz_mtu3_pwm->lock);
+	rz_mtu3_pwm->user_count[ch_index]--;
+	mutex_unlock(&rz_mtu3_pwm->lock);
+
+	if (!rz_mtu3_pwm->user_count[ch_index])
+		ch->function = RZ_MTU3_NORMAL;
+}
+
+static int rz_mtu3_pwm_enable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
+			      struct pwm_device *pwm)
+{
+	struct rz_mtu3_channel *ch;
+	u32 ch_index;
+	u8 val;
+
+	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
+	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
+	val = (RZ_MTU3_TIOR_OC_1_TOGGLE << 4) | RZ_MTU3_TIOR_OC_0_H_COMP_MATCH;
+
+	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_PWM_MODE_1);
+	if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm))
+		rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, val);
+	else
+		rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, val);
+
+	if (rz_mtu3_pwm->user_count[ch_index] <= 1)
+		rz_mtu3_enable(ch);
+
+	return 0;
+}
+
+static void rz_mtu3_pwm_disable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
+				struct pwm_device *pwm)
+{
+	struct rz_mtu3_channel *ch;
+	u32 ch_index;
+
+	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
+	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
+
+	/* Return to normal mode and disable output pins of MTU3 channel */
+	if (rz_mtu3_pwm->user_count[ch_index] <= 1)
+		rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_NORMAL);
+
+	if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm))
+		rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, RZ_MTU3_TIOR_OC_RETAIN);
+	else
+		rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, RZ_MTU3_TIOR_OC_RETAIN);
+
+	if (rz_mtu3_pwm->user_count[ch_index] <= 1)
+		rz_mtu3_disable(ch);
+}
+
+static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			      const struct pwm_state *state)
+{
+	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
+	struct rz_mtu3_channel *ch;
+	unsigned long pv, dc;
+	u64 period_cycles;
+	u64 duty_cycles;
+	u32 ch_index;
+	u8 prescale;
+	u8 val;
+
+	/*
+	 * Refuse clk rates > 1 GHz to prevent overflowing the following
+	 * calculation.
+	 */
+	if (rz_mtu3_pwm->rate > NSEC_PER_SEC)
+		return -EINVAL;
+
+	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
+	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
+	duty_cycles = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycles = 0;
+
+	period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
+					NSEC_PER_SEC);
+	prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm, period_cycles);
+
+	if (period_cycles >> (2 * prescale) <= U16_MAX)
+		pv = period_cycles >> (2 * prescale);
+	else
+		pv = U16_MAX;
+
+	duty_cycles = mul_u64_u32_div(duty_cycles, rz_mtu3_pwm->rate,
+				      NSEC_PER_SEC);
+	if (duty_cycles >> (2 * prescale) <= U16_MAX)
+		dc = duty_cycles >> (2 * prescale);
+	else
+		dc = U16_MAX;
+
+	val = RZ_MTU3_TCR_CKEG_RISING | prescale;
+	if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) {
+		rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR,
+				      RZ_MTU3_TCR_CCLR_TGRC | val);
+		rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRD, dc);
+		rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRC, pv);
+	} else {
+		rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR,
+				      RZ_MTU3_TCR_CCLR_TGRA | val);
+		rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRB, dc);
+		rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRA, pv);
+	}
+
+	return 0;
+}
+
+static void rz_mtu3_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
+	struct rz_mtu3_channel *ch;
+	u8 prescale, val;
+	u32 ch_index;
+	u16 dc, pv;
+	u64 tmp;
+
+	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
+	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
+	pm_runtime_get_sync(chip->dev);
+	state->enabled = rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, pwm->hwpwm);
+	if (state->enabled) {
+		val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TCR);
+		prescale = FIELD_GET(RZ_MTU3_TCR_TPCS, val);
+
+		if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) {
+			dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRD);
+			pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRC);
+		} else {
+			dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRB);
+			pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRA);
+		}
+
+		tmp = NSEC_PER_SEC * (u64)pv << (2 * prescale);
+		state->period = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate);
+
+		tmp = NSEC_PER_SEC * (u64)dc << (2 * prescale);
+		state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate);
+	}
+
+	state->polarity = PWM_POLARITY_NORMAL;
+	pm_runtime_put(chip->dev);
+}
+
+static int rz_mtu3_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
+{
+	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
+	struct pwm_state cur_state;
+	bool enabled;
+	int ret;
+
+	cur_state = pwm->state;
+	enabled = cur_state.enabled;
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (!enabled && state->enabled)
+		pm_runtime_get_sync(chip->dev);
+
+	ret = rz_mtu3_pwm_config(chip, pwm, state);
+	if (ret && state->enabled)
+		goto done;
+
+	if (!state->enabled) {
+		if (enabled)
+			rz_mtu3_pwm_disable(rz_mtu3_pwm, pwm);
+		ret = 0;
+		goto done;
+	}
+
+	return rz_mtu3_pwm_enable(rz_mtu3_pwm, pwm);
+done:
+	if (enabled && !state->enabled)
+		pm_runtime_put(chip->dev);
+	return ret;
+}
+
+static const struct pwm_ops rz_mtu3_pwm_ops = {
+	.request = rz_mtu3_pwm_request,
+	.free = rz_mtu3_pwm_free,
+	.get_state = rz_mtu3_pwm_get_state,
+	.apply = rz_mtu3_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static int rz_mtu3_pwm_pm_runtime_suspend(struct device *dev)
+{
+	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(rz_mtu3_pwm->clk);
+
+	return 0;
+}
+
+static int rz_mtu3_pwm_pm_runtime_resume(struct device *dev)
+{
+	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = dev_get_drvdata(dev);
+
+	clk_prepare_enable(rz_mtu3_pwm->clk);
+
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(rz_mtu3_pwm_pm_ops,
+				 rz_mtu3_pwm_pm_runtime_suspend,
+				 rz_mtu3_pwm_pm_runtime_resume, NULL);
+
+static void rz_mtu3_pwm_pm_disable(void *data)
+{
+	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = data;
+
+	pm_runtime_disable(rz_mtu3_pwm->chip.dev);
+	pm_runtime_set_suspended(rz_mtu3_pwm->chip.dev);
+}
+
+static int rz_mtu3_pwm_probe(struct platform_device *pdev)
+{
+	struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
+	struct rz_mtu3_pwm_chip *rz_mtu3_pwm;
+	struct device *dev = &pdev->dev;
+	int num_pwm_hw_ch = 0;
+	unsigned int i;
+	int ret;
+
+	rz_mtu3_pwm = devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm), GFP_KERNEL);
+	if (!rz_mtu3_pwm)
+		return -ENOMEM;
+
+	rz_mtu3_pwm->clk = ddata->clk;
+	rz_mtu3_pwm->rate = clk_get_rate(rz_mtu3_pwm->clk);
+	for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) {
+		if (i == RZ_MTU5 || i == RZ_MTU8)
+			continue;
+
+		rz_mtu3_pwm->ch[num_pwm_hw_ch] = &ddata->channels[i];
+		rz_mtu3_pwm->ch[num_pwm_hw_ch]->dev = dev;
+		num_pwm_hw_ch++;
+	}
+
+	mutex_init(&rz_mtu3_pwm->lock);
+	platform_set_drvdata(pdev, rz_mtu3_pwm);
+	clk_prepare_enable(rz_mtu3_pwm->clk);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rz_mtu3_pwm_pm_disable,
+				       rz_mtu3_pwm);
+	if (ret < 0)
+		goto disable_clock;
+
+	rz_mtu3_pwm->chip.dev = &pdev->dev;
+	rz_mtu3_pwm->chip.ops = &rz_mtu3_pwm_ops;
+	rz_mtu3_pwm->chip.npwm = RZ_MTU3_MAX_PWM_MODE1_CHANNELS;
+	ret = devm_pwmchip_add(&pdev->dev, &rz_mtu3_pwm->chip);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+		goto disable_clock;
+	}
+
+	return 0;
+
+disable_clock:
+	clk_disable_unprepare(rz_mtu3_pwm->clk);
+	return ret;
+}
+
+static struct platform_driver rz_mtu3_pwm_driver = {
+	.driver = {
+		.name = "pwm-rz-mtu3",
+		.pm = pm_ptr(&rz_mtu3_pwm_pm_ops),
+	},
+	.probe = rz_mtu3_pwm_probe,
+};
+module_platform_driver(rz_mtu3_pwm_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_ALIAS("platform:pwm-rz-mtu3");
+MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a PWM Timer Driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1


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

* Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-13 17:15 ` [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver Biju Das
@ 2022-11-14  3:47   ` William Breathitt Gray
  2022-11-14 13:53     ` William Breathitt Gray
  2022-11-14 15:24     ` Biju Das
  0 siblings, 2 replies; 15+ messages in thread
From: William Breathitt Gray @ 2022-11-14  3:47 UTC (permalink / raw)
  To: Biju Das
  Cc: linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

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

On Sun, Nov 13, 2022 at 05:15:44PM +0000, Biju Das wrote:
> Add RZ/G2L MTU3a counter driver. This IP supports the following
> phase counting modes on MTU1 and MTU2 channels
> 
> 1) 16-bit phase counting modes on MTU1 and MTU2 channels.
> 2) 32-bit phase counting mode by cascading MTU1 and MTU2.
> 
> This patch adds 3 counters by creating 3 logical channels
> 	counter0: 16-bit phase counter on MTU1 channel
> 	counter1: 16-bit phase counter on MTU2 channel
> 	counter2: 32-bit phase counter by cascading MTU1 and MTU2
> 		  channels.

Within the context of the Counter subsystem, the term "counter"
specifically refers to the device (Counts + Synapses + Signals). Instead
you should use "count" here to refer to the counter value channels (i.e.
count0, count1, and count2).

> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v5->v6:
>  * Updated KConfig and commit description
>  * Sorted header
>  * Fixed RZ_MTU3_GET_HW_CH Macro for argument reuse 'id' - 
>    possible side-effects?
>  * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed
>    __maybe_unused from suspend/resume()
> v4->v5:
>  * Updated the Kconfig with SoC vendor name
>  * Introduced rz_mtu3_is_counter_invalid()
>  * replaced pointer to an array of struct rz_mtu3_channel with
>    a simple pointer to struct rz_mtu3_channel.
>  * Added long_word_access_ctrl_mode sysfs entry for 16-bit and
>    32-bit access
>  * Added external_input_phase_clock_select sysfs entry for
>    selecting input clocks.
>  * used preprocessor defines represent SIGNAL_{A,B,C,D}_ID instead of
>    signal ids.
> v3->v4:
>  * There is no resource associated with "rz-mtu3-counter" compatible
>    and moved the code to mfd subsystem as it binds against "rz-mtu".
>  * Removed struct platform_driver rz_mtu3_cnt_driver.
>  * Updated commit description
>  * Updated Kconfig description
>  * Added macros RZ_MTU3_16_BIT_MTU{1,2}_CH for MTU1 and MTU2 channels
>  * Added RZ_MTU3_GET_HW_CH macro for getting channel ID.
>  * replaced priv->ch[id]->priv->ch[0] in rz_mtu3_count_read()
>  * Cached counter max values
>  * replaced cnt->tsr in rz_mtu3_count_direction_read()
>  * Added comments for RZ_MTU3_TCR_CCLR_NONE
>  * Replaced if with switch in rz_mtu3_initialize_counter() and
>    rz_mtu3_count_ceiling_write()
>  * Added locks in initialize, terminate and enable_read to prevent races.
>  * Updated rz_mtu3_action_read to take care of MTU2 signals.
>  * Added separate distinct array for each group of Synapse.
>  * Moved pm handling to parent.
> 
> v1->v3:
>  * Modelled as a counter device supporting 3 counters(2 16-bit and 
>    32-bit)
>  * Add kernel-doc comments to document struct rz_mtu3_cnt
>  * Removed mmio variable from struct rz_mtu3_cnt
>  * Removed cnt local variable from rz_mtu3_count_read()
>  * Replaced -EINVAL->-ERANGE for out of range error conditions.
>  * Removed explicit cast from write functions.
>  * Removed local variable val from rz_mtu3_count_ceiling_read()
>  * Added lock for RMW for counter/ceiling updates.
>  * Added different synapses for counter0 and counter{1,2}
>  * Used ARRAY for assigning num_counts.
>  * Added PM runtime for managing clocks.
>  * Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.
> ---
>  drivers/counter/Kconfig       |  11 +
>  drivers/counter/Makefile      |   1 +
>  drivers/counter/rz-mtu3-cnt.c | 717 ++++++++++++++++++++++++++++++++++
>  3 files changed, 729 insertions(+)
>  create mode 100644 drivers/counter/rz-mtu3-cnt.c
> 
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index d388bf26f4dc..daa352c7392d 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -39,6 +39,17 @@ config INTERRUPT_CNT
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called interrupt-cnt.
>  
> +config RZ_MTU3_CNT
> +	tristate "Renesas RZ/G2L MTU3a counter driver"
> +	depends on RZ_MTU3 || COMPILE_TEST
> +	help
> +	  Enable support for MTU3a counter driver found on Renesas RZ/G2L alike
> +	  SoCs. This IP supports both 16-bit and 32-bit phase counting mode
> +	  support.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called rz-mtu3-cnt.
> +
>  config STM32_TIMER_CNT
>  	tristate "STM32 Timer encoder counter driver"
>  	depends on MFD_STM32_TIMERS || COMPILE_TEST
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index b9a369e0d4fc..933fdd50b3e4 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -8,6 +8,7 @@ counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
>  
>  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_INTERRUPT_CNT)		+= interrupt-cnt.o
> +obj-$(CONFIG_RZ_MTU3_CNT)	+= rz-mtu3-cnt.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
>  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
>  obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
> diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
> new file mode 100644
> index 000000000000..cfddbbe38ed1
> --- /dev/null
> +++ b/drivers/counter/rz-mtu3-cnt.c
> @@ -0,0 +1,717 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/G2L MTU3a Counter driver
> + *
> + * Copyright (C) 2022 Renesas Electronics Corporation
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/counter.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/types.h>
> +
> +#include <clocksource/rz-mtu3.h>
> +
> +#define RZ_MTU3_TSR_TCFD	BIT(7)

It is not always clear what these acronyms represent to someone not as
familiar with the hardware, so I recommend including a comment block
above these defines that provides a brief table of registers
descriptions. Something like:

    TSR: Timer Status Register
    TCFD: Count Direction Flag
    TMDR: Timer Mode Register
    TIOR: Timer I/O Control Register
    TCNT: Timer count
    ...

> +#define RZ_MTU3_MAX_HW_CNTR_CHANNELS	(2)
> +
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_1	(4)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_2	(5)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_3	(6)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_4	(7)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_5	(9)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_MASK	(0xf)
> +
> +#define RZ_MTU3_TMDR3_LWA	BIT(0)
> +#define RZ_MTU3_TMDR3_PHCKSEL	BIT(1)
> +
> +#define RZ_MTU3_16_BIT_MTU1_CH	(0)
> +#define RZ_MTU3_16_BIT_MTU2_CH	(1)
> +#define RZ_MTU3_32_BIT_CH		(2)
> +
> +#define RZ_MTU3_TIOR_NO_OUTPUT	(0)
> +#define RZ_MTU3_TIOR_IC_BOTH	(10)
> +
> +#define RZ_MTU3_GET_HW_CH(id) \
> +({ \
> +	size_t _id = (id); _id = (_id == RZ_MTU3_32_BIT_CH) ? 0 : _id; \
> +})

I probably missed a discussion about this change in a previous thread;
what is the purpose of using a local size_t variable here? Is this due
to the "possible side-effects" mentioned in the patch changes note?

> +
> +#define SIGNAL_A_ID	(0)
> +#define SIGNAL_B_ID	(1)
> +#define SIGNAL_C_ID	(2)
> +#define SIGNAL_D_ID	(3)
> +
> +/**
> + * struct rz_mtu3_cnt - MTU3 counter private data
> + *
> + * @clk: MTU3 module clock
> + * @lock: Lock to prevent concurrent access for ceiling and count
> + * @ch: HW channels for the counters
> + * @mtu_16bit_max: Cache for 16-bit counters
> + * @mtu_32bit_max: Cache for 32-bit counters
> + */
> +struct rz_mtu3_cnt {
> +	struct clk *clk;
> +	struct mutex lock;
> +	struct rz_mtu3_channel *ch;
> +	u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> +	u32 mtu_32bit_max;

Does the ceiling set on the device get clobbered when you change between
16-bit and 32-bit phase modes (i.e. writing to TGRALW vs TGRA)? You have
a separate cache for the 32-bit ceiling value here, but if it is getting
clobbered then as a small optimization you may reimplement this cache as
a union such as:

    union {
            u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
            u32 mtu_32bit_max;
    }

> +};
> +
> +static const enum counter_function rz_mtu3_count_functions[] = {
> +	COUNTER_FUNCTION_QUADRATURE_X4,
> +	COUNTER_FUNCTION_PULSE_DIRECTION,
> +	COUNTER_FUNCTION_QUADRATURE_X2_B,
> +};
> +
> +static bool rz_mtu3_is_counter_invalid(struct counter_device *counter, int id)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u16 val;
> +
> +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3) & RZ_MTU3_TMDR3_LWA;

You should use a more descriptive variable name for this such as
"lwa_mode_enabled" to make its meaning clearer.

> +	if (id == RZ_MTU3_32_BIT_CH && val)
> +		return false;
> +
> +	if (id != RZ_MTU3_32_BIT_CH && !val)
> +		return false;
> +
> +	return true;
> +}
> +
> +static int rz_mtu3_count_read(struct counter_device *counter,
> +			      struct counter_count *count, u64 *val)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +
> +	if (rz_mtu3_is_counter_invalid(counter, count->id))
> +		return -EINVAL;
> +
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		*val = rz_mtu3_32bit_ch_read(priv->ch + ch_id, RZ_MTU3_TCNTLW);
> +	else
> +		*val = rz_mtu3_16bit_ch_read(priv->ch + ch_id, RZ_MTU3_TCNT);
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_write(struct counter_device *counter,
> +			       struct counter_count *count, const u64 val)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +	u32 ceiling;
> +
> +	if (rz_mtu3_is_counter_invalid(counter, count->id))
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->lock);
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		ceiling = priv->mtu_32bit_max;
> +	else
> +		ceiling = priv->mtu_16bit_max[ch_id];
> +
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		rz_mtu3_32bit_ch_write(priv->ch + ch_id, RZ_MTU3_TCNTLW, val);
> +	else
> +		rz_mtu3_16bit_ch_write(priv->ch + ch_id, RZ_MTU3_TCNT, val);
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_function_read(struct counter_device *counter,
> +				       struct counter_count *count,
> +				       enum counter_function *function)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +	u8 val;
> +
> +	val = rz_mtu3_8bit_ch_read(priv->ch + ch_id, RZ_MTU3_TMDR1);

A more descriptive name for this variable would be helpful to make
clearer what this value represents (maybe "timer_mode").

> +
> +	switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
> +	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
> +		*function = COUNTER_FUNCTION_QUADRATURE_X4;
> +		break;
> +	case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
> +		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
> +		break;
> +	case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
> +		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Sorry if I asked this before: what are counting modes 3 and 5, and are
they not supported by this device? If they are not supported, please
include a comment stating so in the default case block so that it is
clear for future reviewers as well.

> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_function_write(struct counter_device *counter,
> +					struct counter_count *count,
> +					enum counter_function function)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +	u8 mode;
> +
> +	switch (function) {
> +	case COUNTER_FUNCTION_QUADRATURE_X4:
> +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_1;
> +		break;
> +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_2;
> +		break;
> +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_4;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	rz_mtu3_8bit_ch_write(priv->ch + ch_id, RZ_MTU3_TMDR1, mode);
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_direction_read(struct counter_device *counter,
> +					struct counter_count *count,
> +					enum counter_count_direction *direction)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +	u8 tsr;
> +
> +	tsr = rz_mtu3_8bit_ch_read(priv->ch + ch_id, RZ_MTU3_TSR);
> +
> +	if (tsr & RZ_MTU3_TSR_TCFD)
> +		*direction = COUNTER_COUNT_DIRECTION_FORWARD;
> +	else
> +		*direction = COUNTER_COUNT_DIRECTION_BACKWARD;

The following is completely up to you if you want to do so, but I think
this conditional looks nicer as a ternary expression:

    *direction = (tsr & RZ_MTU3_TSR_TCFD) ? COUNTER_COUNT_DIRECTION_FORWARD :
            COUNTER_COUNT_DIRECTION_BACKWARD;

> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_ceiling_read(struct counter_device *counter,
> +				      struct counter_count *count,
> +				      u64 *ceiling)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		*ceiling = priv->mtu_32bit_max;
> +	else
> +		*ceiling = priv->mtu_16bit_max[ch_id];

Use a switch statement here to be consistent with the ceiling_write()
callback.

> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
> +				       struct counter_count *count,
> +				       u64 ceiling)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +
> +	if (rz_mtu3_is_counter_invalid(counter, count->id))
> +		return -EINVAL;
> +
> +	switch (count->id) {
> +	case RZ_MTU3_16_BIT_MTU1_CH:
> +	case RZ_MTU3_16_BIT_MTU2_CH:
> +		if (ceiling > U16_MAX)
> +			return -ERANGE;
> +		priv->mtu_16bit_max[ch_id] = ceiling;
> +		break;
> +	case RZ_MTU3_32_BIT_CH:
> +		if (ceiling > U32_MAX)
> +			return -ERANGE;
> +		priv->mtu_32bit_max = ceiling;
> +		break;
> +	}

Provide a default case for this switch statement. In theory we should
never reach that code path, but it's good practice to have in case a bug
appears in the Counter subsystem:

    default:
            /* should never reach this path */
            return -EINVAL;

Make sure to provide similar default cases in the other switch
statements as well.

> +
> +	mutex_lock(&priv->lock);
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		rz_mtu3_32bit_ch_write(priv->ch + ch_id, RZ_MTU3_TGRALW, ceiling);
> +	else
> +		rz_mtu3_16bit_ch_write(priv->ch + ch_id, RZ_MTU3_TGRA, ceiling);
> +
> +	rz_mtu3_8bit_ch_write(priv->ch + ch_id, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter, int id)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	struct rz_mtu3_channel *ch1 = priv->ch;
> +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> +
> +	/*
> +	 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit cascade
> +	 * counter.
> +	 */
> +	ch1->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> +	ch2->function = RZ_MTU3_32BIT_PHASE_COUNTING;

Can these "function" members be modified from outside this driver? If
so, you could have a race condition here.

> +
> +	/* Phase counting mode 1 is used as default in initialization. */
> +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> +
> +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_IC_BOTH);
> +
> +	rz_mtu3_enable(ch1);
> +	rz_mtu3_enable(ch2);
> +}
> +
> +static void rz_mtu3_16bit_cnt_setting(struct counter_device *counter, int id)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	struct rz_mtu3_channel *ch = priv->ch + id;
> +
> +	ch->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> +
> +	/* Phase counting mode 1 is used as default in initialization. */
> +	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> +
> +	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> +	rz_mtu3_enable(ch);
> +}
> +
> +static int rz_mtu3_initialize_counter(struct counter_device *counter, int id)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	struct rz_mtu3_channel *ch1 = priv->ch;
> +	struct rz_mtu3_channel *ch2 = ch1 + 1;

No need to complicate this, just use priv->ch[0], priv->ch[1], and
priv->ch[id]. Same advice applies to the other functions as well.

> +
> +	mutex_lock(&priv->lock);
> +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_NO_OUTPUT);
> +	switch (id) {
> +	case RZ_MTU3_16_BIT_MTU1_CH:
> +	case RZ_MTU3_16_BIT_MTU2_CH:
> +		if ((priv->ch + id)->function != RZ_MTU3_NORMAL) {
> +			mutex_unlock(&priv->lock);
> +			return -EBUSY;
> +		}
> +
> +		rz_mtu3_16bit_cnt_setting(counter, id);
> +		break;
> +	case RZ_MTU3_32_BIT_CH:
> +		if (ch1->function != RZ_MTU3_NORMAL ||
> +		    ch2->function != RZ_MTU3_NORMAL) {
> +			mutex_unlock(&priv->lock);
> +			return -EBUSY;
> +		}
> +		rz_mtu3_32bit_cnt_setting(counter, id);
> +		break;
> +	}
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static void rz_mtu3_terminate_counter(struct counter_device *counter, int id)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	struct rz_mtu3_channel *ch1 = priv->ch;
> +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> +
> +	mutex_lock(&priv->lock);
> +	if (id == RZ_MTU3_32_BIT_CH) {
> +		ch1->function = RZ_MTU3_NORMAL;
> +		ch2->function = RZ_MTU3_NORMAL;
> +		rz_mtu3_disable(ch2);
> +		rz_mtu3_disable(ch1);
> +	} else {
> +		(priv->ch + id)->function = RZ_MTU3_NORMAL;
> +		rz_mtu3_disable(priv->ch + id);
> +	}
> +	mutex_unlock(&priv->lock);
> +}
> +
> +static int rz_mtu3_count_enable_read(struct counter_device *counter,
> +				     struct counter_count *count, u8 *enable)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	struct rz_mtu3_channel *ch1 = priv->ch;
> +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> +
> +	if (count->id == RZ_MTU3_32_BIT_CH) {
> +		mutex_lock(&priv->lock);
> +		*enable = rz_mtu3_is_enabled(ch1) &&
> +			rz_mtu3_is_enabled(ch2);
> +		mutex_unlock(&priv->lock);
> +	} else {
> +		*enable = rz_mtu3_is_enabled(priv->ch + count->id);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_enable_write(struct counter_device *counter,
> +				      struct counter_count *count, u8 enable)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +	struct rz_mtu3_channel *ch = priv->ch + ch_id;
> +	int ret = 0;
> +
> +	if (enable) {
> +		pm_runtime_get_sync(ch->dev);
> +		ret = rz_mtu3_initialize_counter(counter, count->id);

The "enable" Count component serves to pause/resume counting operation;
that means the existing count should not be lost when a Count is
disabled. The rz_mtu3_initialize_counter() function will clear the
current Count, so you'll need to restore it before returning.

Alternatively, the "enable" Count component is optional so you can
remove it if you don't want to implement it; just initialize the counter
at probe time instead.

> +	} else {
> +		rz_mtu3_terminate_counter(counter, count->id);
> +		pm_runtime_put(ch->dev);
> +	}
> +
> +	return ret;
> +}
> +
> +static int rz_mtu3_long_word_access_ctrl_mode_get(struct counter_device *counter,
> +						  u32 *lwa_ctrl_mode)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u16 val;
> +
> +	pm_runtime_get_sync(priv->ch->dev);
> +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> +	*lwa_ctrl_mode = val & RZ_MTU3_TMDR3_LWA;
> +	pm_runtime_put(priv->ch->dev);
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_long_word_access_ctrl_mode_set(struct counter_device *counter,
> +						  u32 lwa_ctrl_mode)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u16 val;
> +
> +	pm_runtime_get_sync(priv->ch->dev);
> +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> +	if (lwa_ctrl_mode)
> +		val |= RZ_MTU3_TMDR3_LWA;
> +	else
> +		val &= ~RZ_MTU3_TMDR3_LWA;
> +
> +	rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, val);
> +	pm_runtime_put(priv->ch->dev);

When you want to assign a bit to a buffer, you can use __assign_bit() to
simplify your code:

    unsigned long tmdr;
    ...
    tmdr = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
    __assign_bit(RZ_MTU3_TMDR3_LWA, &tmdr, !!lwa_ctrl_node);
    rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, tmdr);

> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_ext_input_phase_clock_select_get(struct counter_device *counter,
> +						    u32 *ext_input_phase_clock_select)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u16 val;
> +
> +	pm_runtime_get_sync(priv->ch->dev);
> +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> +	*ext_input_phase_clock_select = (val & RZ_MTU3_TMDR3_PHCKSEL) >> 1;
> +	pm_runtime_put(priv->ch->dev);
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_ext_input_phase_clock_select_set(struct counter_device *counter,
> +						    u32 ext_input_phase_clock_select)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u16 val;
> +
> +	pm_runtime_get_sync(priv->ch->dev);
> +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> +	if (ext_input_phase_clock_select)
> +		val |= RZ_MTU3_TMDR3_PHCKSEL;
> +	else
> +		val &= ~RZ_MTU3_TMDR3_PHCKSEL;
> +
> +	rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, val);
> +	pm_runtime_put(priv->ch->dev);
> +
> +	return 0;
> +}
> +
> +static struct counter_comp rz_mtu3_count_ext[] = {
> +	COUNTER_COMP_DIRECTION(rz_mtu3_count_direction_read),
> +	COUNTER_COMP_ENABLE(rz_mtu3_count_enable_read,
> +			    rz_mtu3_count_enable_write),
> +	COUNTER_COMP_CEILING(rz_mtu3_count_ceiling_read,
> +			     rz_mtu3_count_ceiling_write),
> +};
> +
> +static const enum counter_synapse_action rz_mtu3_synapse_actions[] = {
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_NONE,
> +};
> +
> +static int rz_mtu3_action_read(struct counter_device *counter,
> +			       struct counter_count *count,
> +			       struct counter_synapse *synapse,
> +			       enum counter_synapse_action *action)
> +{
> +	enum counter_function function;
> +	int err;
> +
> +	err = rz_mtu3_count_function_read(counter, count, &function);
> +	if (err)
> +		return err;
> +
> +	/* Default action mode */
> +	*action = COUNTER_SYNAPSE_ACTION_NONE;

You can exit early here depending on which ext_input_phase_clock mode is
currently selected: if "MTCLKA-MTCLKB" then return early if id is signal
C or D, while if "MTCLKC-MTCLKD" return early if id is signal A or B.

> +
> +	switch (function) {
> +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> +		/*
> +		 * Rising edges on signal A updates the respective count.
> +		 * The input level of signal B determines direction.
> +		 */

Since this also applies for signal C and signal D, you should make that
clear in this comment block; maybe adjust to "signal A (signal C)" and
"signal B (signal D)" will do.

> +		if (synapse->signal->id == SIGNAL_A_ID ||
> +		    synapse->signal->id == SIGNAL_C_ID)
> +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> +		break;
> +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> +		/*
> +		 * Any state transition on quadrature pair signal B updates
> +		 * the respective count.
> +		 */
> +		if (synapse->signal->id == SIGNAL_B_ID ||
> +		    synapse->signal->id == SIGNAL_D_ID)
> +			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;

Same suggestion as above.

> +		break;
> +	case COUNTER_FUNCTION_QUADRATURE_X4:
> +		/* counts up/down on both edges of A and B signal*/
> +		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct counter_ops rz_mtu3_cnt_ops = {
> +	.count_read = rz_mtu3_count_read,
> +	.count_write = rz_mtu3_count_write,
> +	.function_read = rz_mtu3_count_function_read,
> +	.function_write = rz_mtu3_count_function_write,
> +	.action_read = rz_mtu3_action_read,
> +};
> +
> +#define RZ_MTU3_PHASE_SIGNAL(_id, _name) {		\
> +	.id = (_id),				\
> +	.name = (_name),			\
> +}
> +
> +static struct counter_signal rz_mtu3_signals[] = {
> +	RZ_MTU3_PHASE_SIGNAL(SIGNAL_A_ID, "MTU1 MTCLKA"),
> +	RZ_MTU3_PHASE_SIGNAL(SIGNAL_B_ID, "MTU1 MTCLKB"),
> +	RZ_MTU3_PHASE_SIGNAL(SIGNAL_C_ID, "MTU2 MTCLKC"),
> +	RZ_MTU3_PHASE_SIGNAL(SIGNAL_D_ID, "MTU2 MTCLKD"),
> +};
> +
> +static struct counter_synapse rz_mtu3_mtu1_count_synapses[] = {
> +	{
> +		.actions_list = rz_mtu3_synapse_actions,
> +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
> +		.signal = rz_mtu3_signals,
> +	},
> +	{
> +		.actions_list = rz_mtu3_synapse_actions,
> +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
> +		.signal = rz_mtu3_signals + 1,
> +	}
> +};
> +
> +static struct counter_synapse rz_mtu3_mtu2_count_synapses[] = {
> +	{
> +		.actions_list = rz_mtu3_synapse_actions,
> +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
> +		.signal = rz_mtu3_signals,
> +	},
> +	{
> +		.actions_list = rz_mtu3_synapse_actions,
> +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
> +		.signal = rz_mtu3_signals + 1,
> +	},
> +	{
> +		.actions_list = rz_mtu3_synapse_actions,
> +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
> +		.signal = rz_mtu3_signals + 2,
> +	},
> +	{
> +		.actions_list = rz_mtu3_synapse_actions,
> +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),
> +		.signal = rz_mtu3_signals + 3,
> +	}
> +};
> +
> +static struct counter_count rz_mtu3_counts[] = {
> +	{
> +		.id = RZ_MTU3_16_BIT_MTU1_CH,
> +		.name = "Channel 1 Count",
> +		.functions_list = rz_mtu3_count_functions,
> +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> +		.synapses = rz_mtu3_mtu1_count_synapses,
> +		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu1_count_synapses),
> +		.ext = rz_mtu3_count_ext,
> +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> +	},
> +	{
> +		.id = RZ_MTU3_16_BIT_MTU2_CH,
> +		.name = "Channel 2 Count",
> +		.functions_list = rz_mtu3_count_functions,
> +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> +		.synapses = rz_mtu3_mtu2_count_synapses,
> +		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu2_count_synapses),
> +		.ext = rz_mtu3_count_ext,
> +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> +	},
> +	{
> +		.id = RZ_MTU3_32_BIT_CH,
> +		.name = "Channel 1 and 2 (combined) Count",
> +		.functions_list = rz_mtu3_count_functions,
> +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> +		.synapses = rz_mtu3_mtu2_count_synapses,

I'm just checking again for my benefit of understanding: in 32-bit phase
counting mode, if ext_input_phase_clock_select is "MTCLKC-MTCLKD", is
signal C and D used instead of signal A and B? In other words, is the
ext_input_phase_clock_select only valid for 16-bit phase counting mode,
or does it also apply for 32-bit phase counting mode?

> +		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu2_count_synapses),
> +		.ext = rz_mtu3_count_ext,
> +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> +	}
> +};
> +
> +static const char *const rz_mtu3_long_word_access_ctrl_modes[] = {
> +	"16-bit",
> +	"32-bit",
> +};
> +
> +static DEFINE_COUNTER_ENUM(rz_mtu3_long_word_access_ctrl_mode_enum,
> +			   rz_mtu3_long_word_access_ctrl_modes);
> +
> +static const char *const rz_mtu3_ext_input_phase_clock_select[] = {
> +	"MTCLKA-MTCLKB",
> +	"MTCLKC-MTCLKD",
> +};
> +
> +static DEFINE_COUNTER_ENUM(rz_mtu3_ext_input_phase_clock_select_enum,
> +			   rz_mtu3_ext_input_phase_clock_select);
> +
> +static struct counter_comp rz_mtu3_device_ext[] = {
> +	COUNTER_COMP_DEVICE_ENUM("long_word_access_ctrl_mode",

Cascading Counts seems like a feature that other counter devices may
also want to expose so it'll be prudent to rename this to something more
general that other drivers can expose. Perhaps reimplement this as a
COUNTER_COMP_DEVICE_BOOL and call it "cascade_enable", or something
similar.

> +				 rz_mtu3_long_word_access_ctrl_mode_get,
> +				 rz_mtu3_long_word_access_ctrl_mode_set,
> +				 rz_mtu3_long_word_access_ctrl_mode_enum),
> +	COUNTER_COMP_DEVICE_ENUM("external_input_phase_clock_select",
> +				 rz_mtu3_ext_input_phase_clock_select_get,
> +				 rz_mtu3_ext_input_phase_clock_select_set,
> +				 rz_mtu3_ext_input_phase_clock_select_enum),
> +};
> +
> +static int rz_mtu3_cnt_pm_runtime_suspend(struct device *dev)
> +{
> +	struct clk *const clk = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(clk);
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_cnt_pm_runtime_resume(struct device *dev)
> +{
> +	struct clk *const clk = dev_get_drvdata(dev);
> +
> +	clk_prepare_enable(clk);
> +
> +	return 0;
> +}

Do the current counts need to be saved and restored when the pm
suspend/resume ops occur?

William Breathitt Gray

> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(rz_mtu3_cnt_pm_ops,
> +				 rz_mtu3_cnt_pm_runtime_suspend,
> +				 rz_mtu3_cnt_pm_runtime_resume, NULL);
> +
> +static void rz_mtu3_cnt_pm_disable(void *data)
> +{
> +	struct device *dev = data;
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +}
> +
> +static int rz_mtu3_cnt_probe(struct platform_device *pdev)
> +{
> +	struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct counter_device *counter;
> +	struct rz_mtu3_cnt *priv;
> +	unsigned int i;
> +	int ret;
> +
> +	counter = devm_counter_alloc(dev, sizeof(*priv));
> +	if (!counter)
> +		return -ENOMEM;
> +
> +	priv = counter_priv(counter);
> +	priv->clk = ddata->clk;
> +	priv->mtu_32bit_max = U32_MAX;
> +	priv->ch = &ddata->channels[RZ_MTU1];
> +	for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
> +		(priv->ch + i)->dev = dev;
> +		priv->mtu_16bit_max[i] = U16_MAX;
> +	}
> +
> +	mutex_init(&priv->lock);
> +	platform_set_drvdata(pdev, priv->clk);
> +	clk_prepare_enable(priv->clk);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	ret = devm_add_action_or_reset(&pdev->dev, rz_mtu3_cnt_pm_disable, dev);
> +	if (ret < 0)
> +		goto disable_clock;
> +
> +	counter->name = dev_name(dev);
> +	counter->parent = dev;
> +	counter->ops = &rz_mtu3_cnt_ops;
> +	counter->counts = rz_mtu3_counts;
> +	counter->num_counts = ARRAY_SIZE(rz_mtu3_counts);
> +	counter->signals = rz_mtu3_signals;
> +	counter->num_signals = ARRAY_SIZE(rz_mtu3_signals);
> +	counter->ext = rz_mtu3_device_ext;
> +	counter->num_ext = ARRAY_SIZE(rz_mtu3_device_ext);
> +
> +	/* Register Counter device */
> +	ret = devm_counter_add(dev, counter);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "Failed to add counter\n");
> +		goto disable_clock;
> +	}
> +
> +	return 0;
> +
> +disable_clock:
> +	clk_disable_unprepare(priv->clk);
> +
> +	return ret;
> +}
> +
> +static struct platform_driver rz_mtu3_cnt_driver = {
> +	.probe = rz_mtu3_cnt_probe,
> +	.driver = {
> +		.name = "rz-mtu3-counter",
> +		.pm = pm_ptr(&rz_mtu3_cnt_pm_ops),
> +	},
> +};
> +module_platform_driver(rz_mtu3_cnt_driver);
> +
> +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> +MODULE_ALIAS("platform:rz-mtu3-counter");
> +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(COUNTER);
> -- 
> 2.25.1
> 

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

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

* Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-14  3:47   ` William Breathitt Gray
@ 2022-11-14 13:53     ` William Breathitt Gray
  2022-11-14 16:27       ` Biju Das
  2022-11-14 15:24     ` Biju Das
  1 sibling, 1 reply; 15+ messages in thread
From: William Breathitt Gray @ 2022-11-14 13:53 UTC (permalink / raw)
  To: Biju Das
  Cc: linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

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

Hi Biju,

I have a few follow-up comments that came to my mind.

On Sun, Nov 13, 2022 at 10:47:13PM -0500, William Breathitt Gray wrote:
> On Sun, Nov 13, 2022 at 05:15:44PM +0000, Biju Das wrote:
> > Add RZ/G2L MTU3a counter driver. This IP supports the following
> > phase counting modes on MTU1 and MTU2 channels
> > 
> > 1) 16-bit phase counting modes on MTU1 and MTU2 channels.
> > 2) 32-bit phase counting mode by cascading MTU1 and MTU2.
> > 
> > This patch adds 3 counters by creating 3 logical channels
> > 	counter0: 16-bit phase counter on MTU1 channel
> > 	counter1: 16-bit phase counter on MTU2 channel
> > 	counter2: 32-bit phase counter by cascading MTU1 and MTU2
> > 		  channels.
> 
> Within the context of the Counter subsystem, the term "counter"
> specifically refers to the device (Counts + Synapses + Signals). Instead
> you should use "count" here to refer to the counter value channels (i.e.
> count0, count1, and count2).

Include a brief description of the Signals and their relationship to the
three Counts as well in this commit message. In particular, mention how
"MTCLKA-MTCLKB" and "MTCLKC-MTCLKD" can be toggled for MTU2, etc.

> > +static int rz_mtu3_long_word_access_ctrl_mode_set(struct counter_device *counter,
> > +						  u32 lwa_ctrl_mode)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u16 val;
> > +
> > +	pm_runtime_get_sync(priv->ch->dev);
> > +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> > +	if (lwa_ctrl_mode)
> > +		val |= RZ_MTU3_TMDR3_LWA;
> > +	else
> > +		val &= ~RZ_MTU3_TMDR3_LWA;
> > +
> > +	rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, val);
> > +	pm_runtime_put(priv->ch->dev);
> 
> When you want to assign a bit to a buffer, you can use __assign_bit() to
> simplify your code:
> 
>     unsigned long tmdr;
>     ...
>     tmdr = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
>     __assign_bit(RZ_MTU3_TMDR3_LWA, &tmdr, !!lwa_ctrl_node);
>     rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, tmdr);

You should consider implementing a rz_mtu3_shared_reg_update_bits() that
will perform this read => assign bits => write sequence so that you can
reuse this pattern in the rz_mtu3_ext_input_phase_clock_select_set().

> > +static int rz_mtu3_action_read(struct counter_device *counter,
> > +			       struct counter_count *count,
> > +			       struct counter_synapse *synapse,
> > +			       enum counter_synapse_action *action)
> > +{
> > +	enum counter_function function;
> > +	int err;
> > +
> > +	err = rz_mtu3_count_function_read(counter, count, &function);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Default action mode */
> > +	*action = COUNTER_SYNAPSE_ACTION_NONE;
> 
> You can exit early here depending on which ext_input_phase_clock mode is
> currently selected: if "MTCLKA-MTCLKB" then return early if id is signal
> C or D, while if "MTCLKC-MTCLKD" return early if id is signal A or B.

IIUC count0 is always "MTCLKA-MTCLKB", so this exit early check won't
apply in that particular case; check count->id to see which Count we're
handling.

William Breathitt Gray

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

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

* RE: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-14  3:47   ` William Breathitt Gray
  2022-11-14 13:53     ` William Breathitt Gray
@ 2022-11-14 15:24     ` Biju Das
  2022-11-14 17:52       ` Biju Das
  2022-11-16  3:27       ` William Breathitt Gray
  1 sibling, 2 replies; 15+ messages in thread
From: Biju Das @ 2022-11-14 15:24 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

Hi William Breathitt Gray,

Thanks for the feedback.

> -----Original Message-----
> From: William Breathitt Gray <william.gray@linaro.org>
> Sent: 14 November 2022 03:47
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: linux-iio@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
> Chris Paterson <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
> 
> On Sun, Nov 13, 2022 at 05:15:44PM +0000, Biju Das wrote:
> 
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > +#define RZ_MTU3_GET_HW_CH(id) \
> > +({ \
> > +	size_t _id = (id); _id = (_id == RZ_MTU3_32_BIT_CH) ? 0 : _id; \
> > +})
> 
> I probably missed a discussion about this change in a previous thread; what
> is the purpose of using a local size_t variable here? Is this due to the
> "possible side-effects" mentioned in the patch changes note?

Check patch is complaining 
"CHECK: Macro argument reuse 'id' - possible side-effects?"

By using local size_t variable, it fixed the check patch warning.

> 
> > +
> > +#define SIGNAL_A_ID	(0)
> > +#define SIGNAL_B_ID	(1)
> > +#define SIGNAL_C_ID	(2)
> > +#define SIGNAL_D_ID	(3)
> > +
> > +/**
> > + * struct rz_mtu3_cnt - MTU3 counter private data
> > + *
> > + * @clk: MTU3 module clock
> > + * @lock: Lock to prevent concurrent access for ceiling and count
> > + * @ch: HW channels for the counters
> > + * @mtu_16bit_max: Cache for 16-bit counters
> > + * @mtu_32bit_max: Cache for 32-bit counters  */ struct rz_mtu3_cnt {
> > +	struct clk *clk;
> > +	struct mutex lock;
> > +	struct rz_mtu3_channel *ch;
> > +	u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> > +	u32 mtu_32bit_max;
> 
> Does the ceiling set on the device get clobbered when you change between 16-
> bit and 32-bit phase modes (i.e. writing to TGRALW vs TGRA)? You have a
> separate cache for the 32-bit ceiling value here, but if it is getting
> clobbered then as a small optimization you may reimplement this cache as a
> union such as:
> 
>     union {
>             u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
>             u32 mtu_32bit_max;
>     }

Yes, it gets clobbered when we change between 16-bit and 32-bit mode.

For eg: 0xbe1352 value 
Split up into mtu1.TGRA=0xbe and mtu2.TGRA=0x1352.

OK will use the union.

> 
> > +};
> > +
> 
> > +
> > +	switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
> > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
> > +		*function = COUNTER_FUNCTION_QUADRATURE_X4;
> > +		break;
> > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
> > +		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
> > +		break;
> > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
> > +		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> Sorry if I asked this before: what are counting modes 3 and 5, and are they
> not supported by this device? If they are not supported, please include a
> comment stating so in the default case block so that it is clear for future
> reviewers as well.

Our hardware supports 5 phase counting modes. From that list, I match up some of the functions 
supported by the counter driver.

counting modes 3 and 5 are supported by the Devices, but currently counter driver is not supported this.

Please see the attached counting modes 3 and 5.
https://ibb.co/3YJByG1

OK, I will add a comment for the details for modes not supported by the current driver in the default block.

> > +
> > +static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> > +
> > +	/*
> > +	 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit cascade
> > +	 * counter.
> > +	 */
> > +	ch1->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > +	ch2->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> 
> Can these "function" members be modified from outside this driver? If so, you
> could have a race condition here.

OK will add channel specific locks to avoid the races.

Do you prefer mutex or spin lock here? As channel selection is based on runtime decision
For both PWM and counter??

> 
> > +
> > +	/* Phase counting mode 1 is used as default in initialization. */
> > +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TMDR1,
> > +RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> > +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_IC_BOTH);
> > +
> > +	rz_mtu3_enable(ch1);
> > +	rz_mtu3_enable(ch2);
> > +}
> > +
> > +static void rz_mtu3_16bit_cnt_setting(struct counter_device *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	struct rz_mtu3_channel *ch = priv->ch + id;
> > +
> > +	ch->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> > +
> > +	/* Phase counting mode 1 is used as default in initialization. */
> > +	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1,
> > +RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > +	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> > +	rz_mtu3_enable(ch);
> > +}
> > +
> > +static int rz_mtu3_initialize_counter(struct counter_device *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> 
> No need to complicate this, just use priv->ch[0], priv->ch[1], and
> priv->ch[id]. Same advice applies to the other functions as well.

I get below error when I use array susbscripts. "*ch1 = priv->ch[0];"


drivers/counter/rz-mtu3-cnt.c:291:32: error: incompatible types when initialising type 'struct rz_mtu3_channel *' using type 'struct rz_mtu3_channel'
  291 |  struct rz_mtu3_channel *ch1 = priv->ch[0];



> 
> > +
> > +	mutex_lock(&priv->lock);
> > +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_NO_OUTPUT);
> > +	switch (id) {
> > +	case RZ_MTU3_16_BIT_MTU1_CH:
> > +	case RZ_MTU3_16_BIT_MTU2_CH:
> > +		if ((priv->ch + id)->function != RZ_MTU3_NORMAL) {
> > +			mutex_unlock(&priv->lock);
> > +			return -EBUSY;
> > +		}
> > +
> > +		rz_mtu3_16bit_cnt_setting(counter, id);
> > +		break;
> > +	case RZ_MTU3_32_BIT_CH:
> > +		if (ch1->function != RZ_MTU3_NORMAL ||
> > +		    ch2->function != RZ_MTU3_NORMAL) {
> > +			mutex_unlock(&priv->lock);
> > +			return -EBUSY;
> > +		}
> > +		rz_mtu3_32bit_cnt_setting(counter, id);
> > +		break;
> > +	}
> > +	mutex_unlock(&priv->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rz_mtu3_terminate_counter(struct counter_device *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> > +
> > +	mutex_lock(&priv->lock);
> > +	if (id == RZ_MTU3_32_BIT_CH) {
> > +		ch1->function = RZ_MTU3_NORMAL;
> > +		ch2->function = RZ_MTU3_NORMAL;
> > +		rz_mtu3_disable(ch2);
> > +		rz_mtu3_disable(ch1);
> > +	} else {
> > +		(priv->ch + id)->function = RZ_MTU3_NORMAL;
> > +		rz_mtu3_disable(priv->ch + id);
> > +	}
> > +	mutex_unlock(&priv->lock);
> > +}
> > +
> > +static int rz_mtu3_count_enable_read(struct counter_device *counter,
> > +				     struct counter_count *count, u8 *enable) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> > +
> > +	if (count->id == RZ_MTU3_32_BIT_CH) {
> > +		mutex_lock(&priv->lock);
> > +		*enable = rz_mtu3_is_enabled(ch1) &&
> > +			rz_mtu3_is_enabled(ch2);
> > +		mutex_unlock(&priv->lock);
> > +	} else {
> > +		*enable = rz_mtu3_is_enabled(priv->ch + count->id);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_enable_write(struct counter_device *counter,
> > +				      struct counter_count *count, u8 enable) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> > +	struct rz_mtu3_channel *ch = priv->ch + ch_id;
> > +	int ret = 0;
> > +
> > +	if (enable) {
> > +		pm_runtime_get_sync(ch->dev);
> > +		ret = rz_mtu3_initialize_counter(counter, count->id);
> 
> The "enable" Count component serves to pause/resume counting operation; that
> means the existing count should not be lost when a Count is disabled. The
> rz_mtu3_initialize_counter() function will clear the current Count, so you'll
> need to restore it before returning.

Yes, it is doing pause/resume operation only. It is using clock gating and PM operations.
During enable, Channel is enabled, clk is on. 

During disable, Channel is disabled, clk is off.

Here we are not losing the count when it is disabled and then enable particular count.

But we will loss the count, after disable, if it is used by other devices such as PWM
Or we are switching to 16-bit and 32-bit and vice versa.

Maybe Will rename it to "rz_mtu3_{resume,pause}_counter" to make it clear.

Compared to PWM framework we are missing export/unexport calls here in counter subsystem.

For PWM, we have an export/unexport calls for creating runtime pwm devices such as pwm0, pwm1 for pwmdevice.
Here, count0, count1 and count2 are created during probe.

My current test sequence is,

1) Set phase clk
2) Set cascade_enable
3) Set enable(Since we don't have export/unexport, I am using disable calls for freeing Channels for other subsystem)
4) Set count
5) Set ceiling

> 
> Alternatively, the "enable" Count component is optional so you can remove it
> if you don't want to implement it; just initialize the counter at probe time
> instead.

Let me know your opinion based on the above?

> 
> > +	} else {
> > +		rz_mtu3_terminate_counter(counter, count->id);
> > +		pm_runtime_put(ch->dev);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int rz_mtu3_long_word_access_ctrl_mode_get(struct counter_device
> *counter,
> > +						  u32 *lwa_ctrl_mode)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u16 val;
> > +
> > +	pm_runtime_get_sync(priv->ch->dev);
> > +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> > +	*lwa_ctrl_mode = val & RZ_MTU3_TMDR3_LWA;
> > +	pm_runtime_put(priv->ch->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_long_word_access_ctrl_mode_set(struct counter_device
> *counter,
> > +						  u32 lwa_ctrl_mode)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u16 val;
> > +
> > +	pm_runtime_get_sync(priv->ch->dev);
> > +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> > +	if (lwa_ctrl_mode)
> > +		val |= RZ_MTU3_TMDR3_LWA;
> > +	else
> > +		val &= ~RZ_MTU3_TMDR3_LWA;
> > +
> > +	rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, val);
> > +	pm_runtime_put(priv->ch->dev);
> 
> When you want to assign a bit to a buffer, you can use __assign_bit() to
> simplify your code:

OK will use and remove BIT macro for both RZ_MTU3_TMDR3_LWA & RZ_MTU3_TMDR3_PHCKSEL
And use bit apis.

> > +static struct counter_count rz_mtu3_counts[] = {
> > +	{
> > +		.id = RZ_MTU3_16_BIT_MTU1_CH,
> > +		.name = "Channel 1 Count",
> > +		.functions_list = rz_mtu3_count_functions,
> > +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > +		.synapses = rz_mtu3_mtu1_count_synapses,
> > +		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu1_count_synapses),
> > +		.ext = rz_mtu3_count_ext,
> > +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > +	},
> > +	{
> > +		.id = RZ_MTU3_16_BIT_MTU2_CH,
> > +		.name = "Channel 2 Count",
> > +		.functions_list = rz_mtu3_count_functions,
> > +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > +		.synapses = rz_mtu3_mtu2_count_synapses,
> > +		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu2_count_synapses),
> > +		.ext = rz_mtu3_count_ext,
> > +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > +	},
> > +	{
> > +		.id = RZ_MTU3_32_BIT_CH,
> > +		.name = "Channel 1 and 2 (combined) Count",
> > +		.functions_list = rz_mtu3_count_functions,
> > +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > +		.synapses = rz_mtu3_mtu2_count_synapses,
> 
> I'm just checking again for my benefit of understanding: in 32-bit phase
> counting mode, if ext_input_phase_clock_select is "MTCLKC-MTCLKD", is signal
> C and D used instead of signal A and B? In other words, is the
> ext_input_phase_clock_select only valid for 16-bit phase counting mode, or
> does it also apply for 32-bit phase counting mode?

MTU1(16-bit mode) :-   "MTCLKA-MTCLKB",
MTU2(16-bit mode) :-   "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
32-bit mode       :- "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"

> 
> > +		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu2_count_synapses),
> > +		.ext = rz_mtu3_count_ext,
> > +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > +	}
> > +};
> > +
> > +static const char *const rz_mtu3_long_word_access_ctrl_modes[] = {
> > +	"16-bit",
> > +	"32-bit",
> > +};
> > +
> > +static DEFINE_COUNTER_ENUM(rz_mtu3_long_word_access_ctrl_mode_enum,
> > +			   rz_mtu3_long_word_access_ctrl_modes);
> > +
> > +static const char *const rz_mtu3_ext_input_phase_clock_select[] = {
> > +	"MTCLKA-MTCLKB",
> > +	"MTCLKC-MTCLKD",
> > +};
> > +
> > +static DEFINE_COUNTER_ENUM(rz_mtu3_ext_input_phase_clock_select_enum,
> > +			   rz_mtu3_ext_input_phase_clock_select);
> > +
> > +static struct counter_comp rz_mtu3_device_ext[] = {
> > +	COUNTER_COMP_DEVICE_ENUM("long_word_access_ctrl_mode",
> 
> Cascading Counts seems like a feature that other counter devices may also
> want to expose so it'll be prudent to rename this to something more general
> that other drivers can expose. Perhaps reimplement this as a
> COUNTER_COMP_DEVICE_BOOL and call it "cascade_enable", or something similar.

OK.Will do.

> 
> > +				 rz_mtu3_long_word_access_ctrl_mode_get,
> > +				 rz_mtu3_long_word_access_ctrl_mode_set,
> > +				 rz_mtu3_long_word_access_ctrl_mode_enum),
> > +	COUNTER_COMP_DEVICE_ENUM("external_input_phase_clock_select",
> > +				 rz_mtu3_ext_input_phase_clock_select_get,
> > +				 rz_mtu3_ext_input_phase_clock_select_set,
> > +				 rz_mtu3_ext_input_phase_clock_select_enum),
> > +};
> > +
> > +static int rz_mtu3_cnt_pm_runtime_suspend(struct device *dev) {
> > +	struct clk *const clk = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_cnt_pm_runtime_resume(struct device *dev) {
> > +	struct clk *const clk = dev_get_drvdata(dev);
> > +
> > +	clk_prepare_enable(clk);
> > +
> > +	return 0;
> > +}
> 
> Do the current counts need to be saved and restored when the pm
> suspend/resume ops occur?

For runtime PM it is not required. But it is required for PM Sleep.

Currently we don't have any PM sleep functionality.

Cheers,
Biju

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

* RE: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-14 13:53     ` William Breathitt Gray
@ 2022-11-14 16:27       ` Biju Das
  0 siblings, 0 replies; 15+ messages in thread
From: Biju Das @ 2022-11-14 16:27 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

Hi William Breathitt Gray,

Thanks for the feedback.

> -----Original Message-----
> From: William Breathitt Gray <william.gray@linaro.org>
> Sent: 14 November 2022 13:53
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: linux-iio@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
> Chris Paterson <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
> 
> Hi Biju,
> 
> I have a few follow-up comments that came to my mind.
> 
> On Sun, Nov 13, 2022 at 10:47:13PM -0500, William Breathitt Gray wrote:
> > On Sun, Nov 13, 2022 at 05:15:44PM +0000, Biju Das wrote:
> > > Add RZ/G2L MTU3a counter driver. This IP supports the following
> > > phase counting modes on MTU1 and MTU2 channels
> > >
> > > 1) 16-bit phase counting modes on MTU1 and MTU2 channels.
> > > 2) 32-bit phase counting mode by cascading MTU1 and MTU2.
> > >
> > > This patch adds 3 counters by creating 3 logical channels
> > > 	counter0: 16-bit phase counter on MTU1 channel
> > > 	counter1: 16-bit phase counter on MTU2 channel
> > > 	counter2: 32-bit phase counter by cascading MTU1 and MTU2
> > > 		  channels.
> >
> > Within the context of the Counter subsystem, the term "counter"
> > specifically refers to the device (Counts + Synapses + Signals).
> > Instead you should use "count" here to refer to the counter value channels
> (i.e.
> > count0, count1, and count2).
> 
> Include a brief description of the Signals and their relationship to the
> three Counts as well in this commit message. In particular, mention how
> "MTCLKA-MTCLKB" and "MTCLKC-MTCLKD" can be toggled for MTU2, etc.
> 
> > > +static int rz_mtu3_long_word_access_ctrl_mode_set(struct counter_device
> *counter,
> > > +						  u32 lwa_ctrl_mode)
> > > +{
> > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > +	u16 val;
> > > +
> > > +	pm_runtime_get_sync(priv->ch->dev);
> > > +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> > > +	if (lwa_ctrl_mode)
> > > +		val |= RZ_MTU3_TMDR3_LWA;
> > > +	else
> > > +		val &= ~RZ_MTU3_TMDR3_LWA;
> > > +
> > > +	rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, val);
> > > +	pm_runtime_put(priv->ch->dev);
> >
> > When you want to assign a bit to a buffer, you can use __assign_bit()
> > to simplify your code:
> >
> >     unsigned long tmdr;
> >     ...
> >     tmdr = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> >     __assign_bit(RZ_MTU3_TMDR3_LWA, &tmdr, !!lwa_ctrl_node);
> >     rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, tmdr);
> 
> You should consider implementing a rz_mtu3_shared_reg_update_bits() that will
> perform this read => assign bits => write sequence so that you can reuse this
> pattern in the rz_mtu3_ext_input_phase_clock_select_set().

It is already taken care while working on cascade_enable.

> 
> > > +static int rz_mtu3_action_read(struct counter_device *counter,
> > > +			       struct counter_count *count,
> > > +			       struct counter_synapse *synapse,
> > > +			       enum counter_synapse_action *action) {
> > > +	enum counter_function function;
> > > +	int err;
> > > +
> > > +	err = rz_mtu3_count_function_read(counter, count, &function);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	/* Default action mode */
> > > +	*action = COUNTER_SYNAPSE_ACTION_NONE;
> >
> > You can exit early here depending on which ext_input_phase_clock mode
> > is currently selected: if "MTCLKA-MTCLKB" then return early if id is
> > signal C or D, while if "MTCLKC-MTCLKD" return early if id is signal A or
> B.
> 
> IIUC count0 is always "MTCLKA-MTCLKB", so this exit early check won't apply
> in that particular case; check count->id to see which Count we're handling.

It is already taken care.

Cheers,
Biju

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

* RE: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-14 15:24     ` Biju Das
@ 2022-11-14 17:52       ` Biju Das
  2022-11-15  4:53         ` William Breathitt Gray
  2022-11-16  3:27       ` William Breathitt Gray
  1 sibling, 1 reply; 15+ messages in thread
From: Biju Das @ 2022-11-14 17:52 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

> > -----Original Message-----
> > From: William Breathitt Gray <william.gray@linaro.org>
> > Sent: 14 November 2022 03:47
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Cc: linux-iio@vger.kernel.org; Geert Uytterhoeven
> > <geert+renesas@glider.be>; Chris Paterson
> > <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad
> > <prabhakar.mahadev-lad.rj@bp.renesas.com>;
> > linux-renesas-soc@vger.kernel.org
> > Subject: Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter
> > driver
> >
> > On Sun, Nov 13, 2022 at 05:15:44PM +0000, Biju Das wrote:
> >
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > +#define RZ_MTU3_GET_HW_CH(id) \
> > > +({ \
> > > +	size_t _id = (id); _id = (_id == RZ_MTU3_32_BIT_CH) ? 0 : _id; \
> > > +})
> >
> > I probably missed a discussion about this change in a previous thread;
> > what is the purpose of using a local size_t variable here? Is this due
> > to the "possible side-effects" mentioned in the patch changes note?
> 
> Check patch is complaining
> "CHECK: Macro argument reuse 'id' - possible side-effects?"
> 
> By using local size_t variable, it fixed the check patch warning.
> 
> >
> > > +
> > > +#define SIGNAL_A_ID	(0)
> > > +#define SIGNAL_B_ID	(1)
> > > +#define SIGNAL_C_ID	(2)
> > > +#define SIGNAL_D_ID	(3)
> > > +
> > > +/**
> > > + * struct rz_mtu3_cnt - MTU3 counter private data
> > > + *
> > > + * @clk: MTU3 module clock
> > > + * @lock: Lock to prevent concurrent access for ceiling and count
> > > + * @ch: HW channels for the counters
> > > + * @mtu_16bit_max: Cache for 16-bit counters
> > > + * @mtu_32bit_max: Cache for 32-bit counters  */ struct rz_mtu3_cnt {
> > > +	struct clk *clk;
> > > +	struct mutex lock;
> > > +	struct rz_mtu3_channel *ch;
> > > +	u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> > > +	u32 mtu_32bit_max;
> >
> > Does the ceiling set on the device get clobbered when you change
> > between 16- bit and 32-bit phase modes (i.e. writing to TGRALW vs
> > TGRA)? You have a separate cache for the 32-bit ceiling value here,
> > but if it is getting clobbered then as a small optimization you may
> > reimplement this cache as a union such as:
> >
> >     union {
> >             u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> >             u32 mtu_32bit_max;
> >     }
> 
> Yes, it gets clobbered when we change between 16-bit and 32-bit mode.
> 
> For eg: 0xbe1352 value
> Split up into mtu1.TGRA=0xbe and mtu2.TGRA=0x1352.
> 
> OK will use the union.
> 
> >
> > > +};
> > > +
> >
> > > +
> > > +	switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
> > > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
> > > +		*function = COUNTER_FUNCTION_QUADRATURE_X4;
> > > +		break;
> > > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
> > > +		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
> > > +		break;
> > > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
> > > +		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> >
> > Sorry if I asked this before: what are counting modes 3 and 5, and are
> > they not supported by this device? If they are not supported, please
> > include a comment stating so in the default case block so that it is
> > clear for future reviewers as well.
> 
> Our hardware supports 5 phase counting modes. From that list, I match up some
> of the functions supported by the counter driver.
> 
> counting modes 3 and 5 are supported by the Devices, but currently counter
> driver is not supported this.
> 
> Please see the attached counting modes 3 and 5.
> https://ibb.co/3YJByG1
> 
> OK, I will add a comment for the details for modes not supported by the
> current driver in the default block.
> 
> > > +
> > > +static void rz_mtu3_32bit_cnt_setting(struct counter_device
> > > +*counter, int id) {
> > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> > > +
> > > +	/*
> > > +	 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit cascade
> > > +	 * counter.
> > > +	 */
> > > +	ch1->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > > +	ch2->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> >
> > Can these "function" members be modified from outside this driver? If
> > so, you could have a race condition here.
> 
> OK will add channel specific locks to avoid the races.
> 
> Do you prefer mutex or spin lock here? As channel selection is based on
> runtime decision For both PWM and counter??
> 
> >
> > > +
> > > +	/* Phase counting mode 1 is used as default in initialization. */
> > > +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TMDR1,
> > > +RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > > +
> > > +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> > > +	rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_IC_BOTH);
> > > +
> > > +	rz_mtu3_enable(ch1);
> > > +	rz_mtu3_enable(ch2);
> > > +}
> > > +
> > > +static void rz_mtu3_16bit_cnt_setting(struct counter_device
> > > +*counter, int id) {
> > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > +	struct rz_mtu3_channel *ch = priv->ch + id;
> > > +
> > > +	ch->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> > > +
> > > +	/* Phase counting mode 1 is used as default in initialization. */
> > > +	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1,
> > > +RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > > +
> > > +	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> > > +	rz_mtu3_enable(ch);
> > > +}
> > > +
> > > +static int rz_mtu3_initialize_counter(struct counter_device
> > > +*counter, int id) {
> > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> >
> > No need to complicate this, just use priv->ch[0], priv->ch[1], and
> > priv->ch[id]. Same advice applies to the other functions as well.
> 
> I get below error when I use array susbscripts. "*ch1 = priv->ch[0];"

> drivers/counter/rz-mtu3-cnt.c:291:32: error: incompatible types when
> initialising type 'struct rz_mtu3_channel *' using type 'struct
> rz_mtu3_channel'
>   291 |  struct rz_mtu3_channel *ch1 = priv->ch[0];
> 

I could use "*ch1 = &priv->ch[0];" please let me know is it ok?

Cheers,
Biju

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

* Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-14 17:52       ` Biju Das
@ 2022-11-15  4:53         ` William Breathitt Gray
  2022-11-15 10:38           ` Biju Das
  0 siblings, 1 reply; 15+ messages in thread
From: William Breathitt Gray @ 2022-11-15  4:53 UTC (permalink / raw)
  To: Biju Das
  Cc: linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

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

On Mon, Nov 14, 2022 at 05:52:11PM +0000, Biju Das wrote:
> > > > +static int rz_mtu3_initialize_counter(struct counter_device
> > > > +*counter, int id) {
> > > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > > > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> > >
> > > No need to complicate this, just use priv->ch[0], priv->ch[1], and
> > > priv->ch[id]. Same advice applies to the other functions as well.
> > 
> > I get below error when I use array susbscripts. "*ch1 = priv->ch[0];"
> 
> > drivers/counter/rz-mtu3-cnt.c:291:32: error: incompatible types when
> > initialising type 'struct rz_mtu3_channel *' using type 'struct
> > rz_mtu3_channel'
> >   291 |  struct rz_mtu3_channel *ch1 = priv->ch[0];
> > 
> 
> I could use "*ch1 = &priv->ch[0];" please let me know is it ok?
> 
> Cheers,
> Biju

Hi Biju,

I meant to use the array subscripts inline (e.g. priv->ch[id].function).
However, I can see the benefit of using the ch1 and ch2 local variables,
so perhaps something like this would be clearer to read:

    struct rz_mtu3_chanel *const ch = priv->ch;
    struct rz_mtu3_chanel *const ch1 = &ch[0];
    struct rz_mtu3_chanel *const ch2 = &ch[1];
    ...
    case RZ_MTU3_16_BIT_MTU1_CH:
    case RZ_MTU3_16_BIT_MTU2_CH:
            if (ch[id].function != RZ_MTU3_NORMAL) {
    ...

William Breathitt Gray

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

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

* RE: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-15  4:53         ` William Breathitt Gray
@ 2022-11-15 10:38           ` Biju Das
  2022-11-16  2:12             ` William Breathitt Gray
  0 siblings, 1 reply; 15+ messages in thread
From: Biju Das @ 2022-11-15 10:38 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

Hi William Breathitt Gray,

Thanks for the feedback.

> -----Original Message-----
> From: William Breathitt Gray <william.gray@linaro.org>
> Sent: 15 November 2022 04:53
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: linux-iio@vger.kernel.org; Geert Uytterhoeven
> <geert+renesas@glider.be>; Chris Paterson
> <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas-
> soc@vger.kernel.org
> Subject: Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter
> driver
> 
> On Mon, Nov 14, 2022 at 05:52:11PM +0000, Biju Das wrote:
> > > > > +static int rz_mtu3_initialize_counter(struct counter_device
> > > > > +*counter, int id) {
> > > > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > > > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > > > > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> > > >
> > > > No need to complicate this, just use priv->ch[0], priv->ch[1],
> and
> > > > priv->ch[id]. Same advice applies to the other functions as
> well.
> > >
> > > I get below error when I use array susbscripts. "*ch1 = priv-
> >ch[0];"
> >
> > > drivers/counter/rz-mtu3-cnt.c:291:32: error: incompatible types
> when
> > > initialising type 'struct rz_mtu3_channel *' using type 'struct
> > > rz_mtu3_channel'
> > >   291 |  struct rz_mtu3_channel *ch1 = priv->ch[0];
> > >
> >
> > I could use "*ch1 = &priv->ch[0];" please let me know is it ok?
> >
> > Cheers,
> > Biju
> 
> Hi Biju,
> 
> I meant to use the array subscripts inline (e.g. priv-
> >ch[id].function).
> However, I can see the benefit of using the ch1 and ch2 local
> variables, so perhaps something like this would be clearer to read:
> 
>     struct rz_mtu3_chanel *const ch = priv->ch;
>     struct rz_mtu3_chanel *const ch1 = &ch[0];
>     struct rz_mtu3_chanel *const ch2 = &ch[1];
>     ...
>     case RZ_MTU3_16_BIT_MTU1_CH:
>     case RZ_MTU3_16_BIT_MTU2_CH:
>             if (ch[id].function != RZ_MTU3_NORMAL) {
>     ...


OK, I have added below inline function which simplifies the code
in each function. Is it ok?

For eg:

+static inline struct rz_mtu3_channel *
+rz_mtu3_get_ch(struct counter_device *counter, int id)
+{
+       struct rz_mtu3_cnt *const priv = counter_priv(counter);
+       const size_t ch_id = RZ_MTU3_GET_HW_CH(id);
+
+       return &priv->ch[ch_id];
+}


@@ -154,11 +163,10 @@ static int rz_mtu3_count_function_read(struct counter_device *counter,
                                       struct counter_count *count,
                                       enum counter_function *function)
 {
-       struct rz_mtu3_cnt *const priv = counter_priv(counter);
-       const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+       struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, count->id);


Cheers,
Biju

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

* Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-15 10:38           ` Biju Das
@ 2022-11-16  2:12             ` William Breathitt Gray
  0 siblings, 0 replies; 15+ messages in thread
From: William Breathitt Gray @ 2022-11-16  2:12 UTC (permalink / raw)
  To: Biju Das
  Cc: linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

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

On Tue, Nov 15, 2022 at 10:38:32AM +0000, Biju Das wrote:
> Hi William Breathitt Gray,
> 
> Thanks for the feedback.
> 
> > -----Original Message-----
> > From: William Breathitt Gray <william.gray@linaro.org>
> > Sent: 15 November 2022 04:53
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Cc: linux-iio@vger.kernel.org; Geert Uytterhoeven
> > <geert+renesas@glider.be>; Chris Paterson
> > <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad
> > <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas-
> > soc@vger.kernel.org
> > Subject: Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter
> > driver
> > 
> > On Mon, Nov 14, 2022 at 05:52:11PM +0000, Biju Das wrote:
> > > > > > +static int rz_mtu3_initialize_counter(struct counter_device
> > > > > > +*counter, int id) {
> > > > > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > > > > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > > > > > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> > > > >
> > > > > No need to complicate this, just use priv->ch[0], priv->ch[1],
> > and
> > > > > priv->ch[id]. Same advice applies to the other functions as
> > well.
> > > >
> > > > I get below error when I use array susbscripts. "*ch1 = priv-
> > >ch[0];"
> > >
> > > > drivers/counter/rz-mtu3-cnt.c:291:32: error: incompatible types
> > when
> > > > initialising type 'struct rz_mtu3_channel *' using type 'struct
> > > > rz_mtu3_channel'
> > > >   291 |  struct rz_mtu3_channel *ch1 = priv->ch[0];
> > > >
> > >
> > > I could use "*ch1 = &priv->ch[0];" please let me know is it ok?
> > >
> > > Cheers,
> > > Biju
> > 
> > Hi Biju,
> > 
> > I meant to use the array subscripts inline (e.g. priv-
> > >ch[id].function).
> > However, I can see the benefit of using the ch1 and ch2 local
> > variables, so perhaps something like this would be clearer to read:
> > 
> >     struct rz_mtu3_chanel *const ch = priv->ch;
> >     struct rz_mtu3_chanel *const ch1 = &ch[0];
> >     struct rz_mtu3_chanel *const ch2 = &ch[1];
> >     ...
> >     case RZ_MTU3_16_BIT_MTU1_CH:
> >     case RZ_MTU3_16_BIT_MTU2_CH:
> >             if (ch[id].function != RZ_MTU3_NORMAL) {
> >     ...
> 
> 
> OK, I have added below inline function which simplifies the code
> in each function. Is it ok?
> 
> For eg:
> 
> +static inline struct rz_mtu3_channel *
> +rz_mtu3_get_ch(struct counter_device *counter, int id)
> +{
> +       struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +       const size_t ch_id = RZ_MTU3_GET_HW_CH(id);
> +
> +       return &priv->ch[ch_id];
> +}
> 
> 
> @@ -154,11 +163,10 @@ static int rz_mtu3_count_function_read(struct counter_device *counter,
>                                        struct counter_count *count,
>                                        enum counter_function *function)
>  {
> -       struct rz_mtu3_cnt *const priv = counter_priv(counter);
> -       const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +       struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, count->id);
> 
> 
> Cheers,
> Biju

Sure, I think that function will be okay to use.

William Breathitt Gray

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

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

* Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
  2022-11-14 15:24     ` Biju Das
  2022-11-14 17:52       ` Biju Das
@ 2022-11-16  3:27       ` William Breathitt Gray
  1 sibling, 0 replies; 15+ messages in thread
From: William Breathitt Gray @ 2022-11-16  3:27 UTC (permalink / raw)
  To: Biju Das, thierry.reding
  Cc: linux-iio, Geert Uytterhoeven, Chris Paterson,
	Prabhakar Mahadev Lad, linux-renesas-soc

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

On Mon, Nov 14, 2022 at 03:24:26PM +0000, Biju Das wrote:
> > > +#define RZ_MTU3_GET_HW_CH(id) \
> > > +({ \
> > > +	size_t _id = (id); _id = (_id == RZ_MTU3_32_BIT_CH) ? 0 : _id; \
> > > +})
> > 
> > I probably missed a discussion about this change in a previous thread; what
> > is the purpose of using a local size_t variable here? Is this due to the
> > "possible side-effects" mentioned in the patch changes note?
> 
> Check patch is complaining 
> "CHECK: Macro argument reuse 'id' - possible side-effects?"
> 
> By using local size_t variable, it fixed the check patch warning.

Ah, I see what you mean: 'id' could be an expression (e.g. x++) which
would be evaluated twice if passed as the macro argument. I think
there's a compiler hint that will supress this warning, but I can't
quite remember it right now.

For now, I'd prefer this to be implemented as an inline function so that
the purpose of the code remains clear:

    static inline size_t rz_mtu3_get_hw_ch(const size_t id)
    {
            return (id == RZ_MTU3_32_BIT_CH) ? 0 : id;
    }

> > > +/**
> > > + * struct rz_mtu3_cnt - MTU3 counter private data
> > > + *
> > > + * @clk: MTU3 module clock
> > > + * @lock: Lock to prevent concurrent access for ceiling and count
> > > + * @ch: HW channels for the counters
> > > + * @mtu_16bit_max: Cache for 16-bit counters
> > > + * @mtu_32bit_max: Cache for 32-bit counters  */ struct rz_mtu3_cnt {
> > > +	struct clk *clk;
> > > +	struct mutex lock;
> > > +	struct rz_mtu3_channel *ch;
> > > +	u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> > > +	u32 mtu_32bit_max;
> > 
> > Does the ceiling set on the device get clobbered when you change between 16-
> > bit and 32-bit phase modes (i.e. writing to TGRALW vs TGRA)? You have a
> > separate cache for the 32-bit ceiling value here, but if it is getting
> > clobbered then as a small optimization you may reimplement this cache as a
> > union such as:
> > 
> >     union {
> >             u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> >             u32 mtu_32bit_max;
> >     }
> 
> Yes, it gets clobbered when we change between 16-bit and 32-bit mode.
> 
> For eg: 0xbe1352 value 
> Split up into mtu1.TGRA=0xbe and mtu2.TGRA=0x1352.
> 
> OK will use the union.

Be sure also to check rz_mtu3_is_counter_invalid() in
rz_mtu3_count_ceiling_read() to make sure the proper ceiling is only
returned when its valid.

> > > +	switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
> > > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
> > > +		*function = COUNTER_FUNCTION_QUADRATURE_X4;
> > > +		break;
> > > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
> > > +		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
> > > +		break;
> > > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
> > > +		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > 
> > Sorry if I asked this before: what are counting modes 3 and 5, and are they
> > not supported by this device? If they are not supported, please include a
> > comment stating so in the default case block so that it is clear for future
> > reviewers as well.
> 
> Our hardware supports 5 phase counting modes. From that list, I match up some of the functions 
> supported by the counter driver.
> 
> counting modes 3 and 5 are supported by the Devices, but currently counter driver is not supported this.
> 
> Please see the attached counting modes 3 and 5.
> https://ibb.co/3YJByG1
> 
> OK, I will add a comment for the details for modes not supported by the current driver in the default block.

Those are interesting counting modes; it looks like counting mode 5 is
pulse-direction with an inverted direction, but I'm not sure what to
call counting mode 3. I haven't come across these counting modes before
so I wonder what sort of applications they're typically used for.

Typically we would add a new COUNTER_FUNCTION_* to represent each new
counting mode. However, for the sake of keeping this patch series simple
it will be okay to leave a comment in the default block for now
describing the missing counting modes and indicating that support for
them are TODO items for the future.

> > > +static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter,
> > > +int id) {
> > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> > > +
> > > +	/*
> > > +	 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit cascade
> > > +	 * counter.
> > > +	 */
> > > +	ch1->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > > +	ch2->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > 
> > Can these "function" members be modified from outside this driver? If so, you
> > could have a race condition here.
> 
> OK will add channel specific locks to avoid the races.
> 
> Do you prefer mutex or spin lock here? As channel selection is based on runtime decision
> For both PWM and counter??

Hmm, I'm not sure yet how best to handle this because if the PWM driver
changes the function in the middle of the Counter driver's operation
(or vice versa) we'll obviously have problems.

Thierry, do you have any ideas here for how we can gracefully handle
transfer of control between the PWM and Counter driver. I think a simple
mutex in struct rz_mtu3 should be fine: one driver locks it and the
other driver can return -EBUSY until its unlocked.

> > > +static int rz_mtu3_count_enable_write(struct counter_device *counter,
> > > +				      struct counter_count *count, u8 enable) {
> > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > +	const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> > > +	struct rz_mtu3_channel *ch = priv->ch + ch_id;
> > > +	int ret = 0;
> > > +
> > > +	if (enable) {
> > > +		pm_runtime_get_sync(ch->dev);
> > > +		ret = rz_mtu3_initialize_counter(counter, count->id);
> > 
> > The "enable" Count component serves to pause/resume counting operation; that
> > means the existing count should not be lost when a Count is disabled. The
> > rz_mtu3_initialize_counter() function will clear the current Count, so you'll
> > need to restore it before returning.
> 
> Yes, it is doing pause/resume operation only. It is using clock gating and PM operations.
> During enable, Channel is enabled, clk is on. 
> 
> During disable, Channel is disabled, clk is off.
> 
> Here we are not losing the count when it is disabled and then enable particular count.
> 
> But we will loss the count, after disable, if it is used by other devices such as PWM
> Or we are switching to 16-bit and 32-bit and vice versa.
> 
> Maybe Will rename it to "rz_mtu3_{resume,pause}_counter" to make it clear.
> 
> Compared to PWM framework we are missing export/unexport calls here in counter subsystem.
> 
> For PWM, we have an export/unexport calls for creating runtime pwm devices such as pwm0, pwm1 for pwmdevice.
> Here, count0, count1 and count2 are created during probe.
> 
> My current test sequence is,
> 
> 1) Set phase clk
> 2) Set cascade_enable
> 3) Set enable(Since we don't have export/unexport, I am using disable calls for freeing Channels for other subsystem)
> 4) Set count
> 5) Set ceiling
> 
> > 
> > Alternatively, the "enable" Count component is optional so you can remove it
> > if you don't want to implement it; just initialize the counter at probe time
> > instead.
> 
> Let me know your opinion based on the above?

Okay, I think I understand, the count is only lost if another driver
takes control of that channel. In that case, you can leave the code as
it is; there is no need to rename the functions. Once another driver
such as PWM has control over the device channel, it's unreasonable to
expect the Counter driver to maintain the previous device state, so
don't worry about trying to resolve that.

William Breathitt Gray

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

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

end of thread, other threads:[~2022-11-16  3:27 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-13 17:15 [PATCH v6 0/5] Add RZ/G2L MTU3a Core, Counter and pwm driver Biju Das
2022-11-13 17:15 ` [PATCH v6 1/5] dt-bindings: timer: Document RZ/G2L MTU3a bindings Biju Das
2022-11-13 17:15 ` [PATCH v6 2/5] clocksource/drivers: Add Renesas RZ/G2L MTU3a core driver Biju Das
2022-11-13 17:15 ` [PATCH v6 3/5] Documentation: ABI: sysfs-bus-counter: add external_input_phase_clock_select & long_word_access_ctrl_mode items Biju Das
2022-11-13 17:15 ` [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver Biju Das
2022-11-14  3:47   ` William Breathitt Gray
2022-11-14 13:53     ` William Breathitt Gray
2022-11-14 16:27       ` Biju Das
2022-11-14 15:24     ` Biju Das
2022-11-14 17:52       ` Biju Das
2022-11-15  4:53         ` William Breathitt Gray
2022-11-15 10:38           ` Biju Das
2022-11-16  2:12             ` William Breathitt Gray
2022-11-16  3:27       ` William Breathitt Gray
2022-11-13 17:15 ` [PATCH v6 5/5] pwm: Add Renesas RZ/G2L MTU3a PWM driver 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).