[PATCH v2 0/7] Add Rockchip RK3576 PWM Support Through MFPWM

[PATCH v2 0/7] Add Rockchip RK3576 PWM Support Through MFPWM

[Nicolas Frattaroli]

This series introduces support for some of the functions of the new PWM
silicon found on Rockchip's RK3576 SoC. Due to the wide range of
functionalities offered by it, including many parts which this series'
first iteration does not attempt to implement for now, it uses multiple
drivers hanging on a platform bus on which the parent "mfpwm" driver
registers them.

AUXBUS/MFD/PLATFORM BUS DISCUSSION: if you are currently wondering why
you were Cc'd and are about to move onto the next e-mail, it may be
because we're currently trying to figure out whether this smorgasbord of
drivers should be a MFD driver, a platform bus driver, or an auxbus
driver. It is currently implemented as a platform bus driver. Any
additional insight from people who know the conceptual place that MFD
has would be appreciated.

Here's some of the features of the hardware:
- Continuous PWM output (implemented in this series)
- One-shot/Finite repetition PWM output
- PWM capture by counting high/low cycles (implemented in this series)
- Sending IR transmissions in several TV remote protocols
- Generating an interrupt based on the input being one of 16
  user-specified values ("Power key capture")
- Biphasic counter support
- Using the hardware to measure a clock signal's frequency
- Using the hardware to count a clock signal's pulses
- Generating PWM output waveforms through a user-specified lookup table

As you can tell, there's a lot. I've focused on continuous PWM output
for now as the most important one for things like controlling fans. The
PWM capture driver is an added bonus, because I needed at least two
drivers to test things. Anyone doing consumer electronic devices like
TVs based on the RK3576 may need to do the power key stuff at some
stage, as it can be used to wake up the SoC with an IR remote. The IR
transmission stuff in general may be a funny weekend project for someone
at some point; I assume it's there so TV boxes can turn on and off TVs
without needing the HDMI control stuff.

At first, I considered simply integrating support for this new IP into
the old pwm-rockchip driver, as the downstream vendor kernel did.
However, the IP is significantly different from previous iterations.
Especially if the goal is to support some of the additional
functionality that the new silicon brings, doing it all in a single pwm
driver would be untenable. Especially one that already supports other
hardware with a way different set of registers.

Hence, the mfpwm pattern: each device functionality is its own driver,
and they all get registered as platform drivers by the parent mfpwm
driver, which is the one that binds to the DT compatible. Each device
function driver then has to _acquire and _release the hardware when it
needs control of it. If some other device function is using the device
already, -EBUSY is returned, which the device function driver can then
forward to the user and everyone is happy.

The PWM output driver, pwm-rockchip-v4, uses the new waveform APIs. I
thought while writing a new driver that I might as well use the new
APIs.

The PWM capture driver, implemented as a counter driver, is somewhat
primitive, in that it doesn't make use of things like the biphasic
counter support or clock measuring, but it serves as a good way to
showcase and test the mutual exclusion that the mfpwm framework tries to
achieve. It also goes one step beyond just exposing the raw LPC/HPC
counts and actually makes them usable in a non-theoretically-racey way
by turning them into nanosecond period/duty cycle values based on the
clock's rate. Shoutouts to the counter subsystem's documentation by the
way, it is some of the best subsystem documentation I've come across so
far, and was a great help.

All instances of the PWM controller have three clocks that they can pick
and choose to derive the PWM signal from. One is the default PLL from
the CRU, one is the 24 MHz crystal oscillator (gated by the CRU), and
one is an RC oscillator (also gated by the CRU). Each PWM channel can
switch between these with a clock selection register in the PWM register
range, hence this is implemented as a clock mux.

Along the way, I also finally took the time to give us The One True
HIWORD_UPDATE macro, which aims to replace all other copies of the
HIWORD_UPDATE macro, except it's not called HIWORD_UPDATE because all of
them have slightly different semantics and I don't want to introduce
even more confusion. It does, however, do some compile-time checking of
the function-like macro parameters.

This series went through two substantial rewrites, because after I
realised it needed to be multiple drivers (first rewrite) I then first
wrote it as a couple of auxiliary bus drivers, until I became unsure
about whether this should be auxiliary bus drivers at all, so it became
a bunch of platform bus drivers instead. If anything looks like a weird
vestigial leftover in the code, then that's probably why, but I made
sure to get rid of all the ones I could find before submitting this.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Changes in v2:
- bindings: make osc required (as it's present in all instances of the
  hardware I'm aware of) and add the rc clock as well. I thought it
  wasn't present on some instances of the PWM IP due to the vendor SoC
  dtsi, but checking the CRU made me realise those clocks do exist for
  all instances. Did not include Conor's R-b as this constitutes a
  substantial enough change to necessitate a re-review
- move bitfield write-enable mask macros into bitfield.h by replacing
  the original rockchip-specific utils header patch with a bitfield.h
  patch.
- mfpwm: change all instances of WARN to be dev_warn instead, as we have
  a device pointer.
- mfpwm: replace the ad-hoc clock mux implementation that used a sysfs
  interface with a generic clk-mux.
- mfpwm: add the rc clock
- mfpwm: rename all the pwmv4_ prefixed functions to have the
  rockchip_pwm_v4_ prefix instead
- mfpwm: remove the pwmclk indirection, hand chosen_clk to pwmf
- mfpwm: move to use the new bitfield macros for the WE mask
- mfpwm: mark reg access inline functions as static to fix build errors
- pwm-rockchip-v4 pwm output: replace mult_frac with mul_u64_u64_div_u64
- pwm-rockchip-v4 pwm output: don't return error if parameters are out
  of range, just set them to the maximum
- pwm-rockchip-v4 pwm output: add rate to debug message
- pwm-rockchip-v4 pwm output: if rate is 0 and pwm is disabled, set
  waveform parameters to 0. The clock is expected to not have a rate in
  this case.
- pwm-rockchip-v4 pwm output: add pwmchip_remove in remove callback,
  which also necessitated using chip as the platdata instead of the
  driver private struct
- pwm-rockchip-v4 pwm output: rework PWMV4_CTRL_UPDATE_EN since it never
  needs to be set to 0 by the driver
- pwm-rockchip-v4 pwm output: add a limitations list
- pwm-rockchip-v4 pwm output: handle initial hardware state during
  probe, enabling the pwm clock if the PWM is on and in continuous mode
- pwm-rockchip-v4 pwm output: rename pwmv4_is_enabled to use the
  rockchip_pwm_v4_ prefix instead
- pwm-rockchip-v4 pwm output: remove pwmclk indirection, use clk API
  directly
- pwm-rockchip-v4 pwm output: no longer claim the chip as being atomic,
  as the clk_rate_exclusive_get calls may sleep.
- rockchip-pwm-capture counter: remove pwmclk indirection, use clk API
  directly
- rockchip-pwm-capture counter: replace mult_frac with
  mul_u64_u64_div_u64
- rockchip-pwm-capture counter: don't output periods/duty cycles if the
  period is longer than the chosen timeout; this works around the
  hardware cycle counter seemingly being impossible to clear
- dts: added osc and rc to every pwm node
- dts: reordered properties in pwm0 to be sorted
- Link to v1: https://lore.kernel.org/r/20250408-rk3576-pwm-v1-0-a49286c2ca8e@collabora.com

---
Nicolas Frattaroli (7):
      dt-bindings: pinctrl: rockchip: increase max amount of device functions
      dt-bindings: pwm: Add a new binding for rockchip,rk3576-pwm
      bitfield: introduce HI16_WE bitfield prep macros
      soc: rockchip: add mfpwm driver
      pwm: Add rockchip PWMv4 driver
      counter: Add rockchip-pwm-capture driver
      arm64: dts: rockchip: add PWM nodes to RK3576 SoC dtsi

 .../bindings/pinctrl/rockchip,pinctrl.yaml         |   2 +-
 .../bindings/pwm/rockchip,rk3576-pwm.yaml          |  77 ++++
 MAINTAINERS                                        |  11 +
 arch/arm64/boot/dts/rockchip/rk3576.dtsi           | 208 +++++++++
 drivers/counter/Kconfig                            |  13 +
 drivers/counter/Makefile                           |   1 +
 drivers/counter/rockchip-pwm-capture.c             | 352 +++++++++++++++
 drivers/pwm/Kconfig                                |  13 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-rockchip-v4.c                      | 372 ++++++++++++++++
 drivers/soc/rockchip/Kconfig                       |  13 +
 drivers/soc/rockchip/Makefile                      |   1 +
 drivers/soc/rockchip/mfpwm.c                       | 398 +++++++++++++++++
 include/linux/bitfield.h                           |  47 ++
 include/soc/rockchip/mfpwm.h                       | 484 +++++++++++++++++++++
 15 files changed, 1992 insertions(+), 1 deletion(-)
---
base-commit: ba2b2250bbaf005016ba85e345add6e19116a1ea
change-id: 20250407-rk3576-pwm-46761bd0deaa

Best regards,
-- 
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH v2 1/7] dt-bindings: pinctrl: rockchip: increase max amount of device functions

[Nicolas Frattaroli]

With the introduction of the RK3576, the maximum device function ID used
increased to 14, as anyone can easily verify for themselves with:

  rg -g '*-pinctrl.dtsi' '<\d+\s+RK_P..\s+(?<func>\d+)\s.*>;$' --trim \
  -NI -r '$func' arch/arm64/boot/dts/rockchip/ | sort -g | uniq

Unfortunately, this wasn't caught by dt-validate as those pins are
omit-if-no-ref and we had no reference to them in any tree so far.

Once again kick the can down the road by increasing the limit to 14.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
index 960758dc417f7405010fab067bfbf6f5c4704179..125af766b99297dc229db158846daea974dda28e 100644
--- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
@@ -135,7 +135,7 @@ additionalProperties:
               description:
                 Pin bank index.
             - minimum: 0
-              maximum: 13
+              maximum: 14
               description:
                 Mux 0 means GPIO and mux 1 to N means
                 the specific device function.

-- 
2.49.0

[PATCH v2 2/7] dt-bindings: pwm: Add a new binding for rockchip,rk3576-pwm

[Nicolas Frattaroli]

The Rockchip RK3576 SoC has a newer PWM controller IP revision than
previous Rockchip SoCs. This IP, called "PWMv4" by Rockchip, introduces
several new features, and consequently differs in its bindings.

Instead of expanding the ever-growing rockchip-pwm binding that already
has an if-condition, add an entirely new binding to handle this.

There are two additional clocks, "osc" and "rc". These are available for
every PWM instance, and the PWM hardware can switch between the "pwm",
"osc" and "rc" clock at runtime.

The PWM controller also comes with an interrupt now. This interrupt is
used to signal various conditions.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 .../bindings/pwm/rockchip,rk3576-pwm.yaml          | 77 ++++++++++++++++++++++
 MAINTAINERS                                        |  7 ++
 2 files changed, 84 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml b/Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..48d5055c8b069fff431c62e67bda11f2e086c9a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/rockchip,rk3576-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip PWMv4 controller
+
+maintainers:
+  - Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
+
+description: |
+  The Rockchip PWMv4 controller is a PWM controller found on several Rockchip
+  SoCs, such as the RK3576.
+
+  It supports both generating and capturing PWM signals.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: rockchip,rk3576-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Used to derive the PWM signal.
+      - description: Used as the APB bus clock.
+      - description: Used as an alternative to derive the PWM signal.
+      - description: Used as another alternative to derive the PWM signal.
+
+  clock-names:
+    items:
+      - const: pwm
+      - const: pclk
+      - const: osc
+      - const: rc
+
+  interrupts:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rockchip,rk3576-cru.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pwm@2add0000 {
+            compatible = "rockchip,rk3576-pwm";
+            reg = <0x0 0x2add0000 0x0 0x1000>;
+            clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>, <&cru CLK_OSC_PWM1>,
+                     <&cru CLK_RC_PWM1>;
+            clock-names = "pwm", "pclk", "osc", "rc";
+            interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
+            #pwm-cells = <3>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 2f13e1602ae68d808b2e8a4711d3c6d40bf5f752..ed5cf56b3ebf9e3153cb9171908a1d36c246197d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21418,6 +21418,13 @@ F:	Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst
 F:	drivers/media/platform/rockchip/rkisp1
 F:	include/uapi/linux/rkisp1-config.h
 
+ROCKCHIP MFPWM
+M:	Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
+L:	linux-rockchip@lists.infradead.org
+L:	linux-pwm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml
+
 ROCKCHIP RK3568 RANDOM NUMBER GENERATOR SUPPORT
 M:	Daniel Golle <daniel@makrotopia.org>
 M:	Aurelien Jarno <aurelien@aurel32.net>

-- 
2.49.0

[PATCH v2 3/7] bitfield: introduce HI16_WE bitfield prep macros

[Nicolas Frattaroli]

Hardware of various vendors, but very notably Rockchip, often uses
32-bit registers where the upper 16-bit half of the register is a
write-enable mask for the lower half.

This type of hardware setup allows for more granular concurrent register
write access.

Over the years, many drivers have hand-rolled their own version of this
macro, usually without any checks, often called something like
HIWORD_UPDATE or FIELD_PREP_HIWORD, commonly with slightly different
semantics between them.

Clearly there is a demand for such a macro, and thus the demand should
be satisfied in a common header file.

Add two macros: FIELD_PREP_HI16_WE, and FIELD_PREP_HI16_WE_CONST. The
latter is a version that can be used in initializers, like
FIELD_PREP_CONST. The macro names are chosen to explicitly reference the
assumed half-register width, and its function, while not clashing with
any potential other macros that drivers may already have implemented
themselves.

Future drivers should use these macros instead of handrolling their own,
and old drivers can be ported to the new macros as time and opportunity
allows.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 include/linux/bitfield.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 6d9a53db54b66c0833973c880444bd289d9667b1..2b3e7cb90ccb5d48f510104f61443b06748bb7eb 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -8,6 +8,7 @@
 #define _LINUX_BITFIELD_H
 
 #include <linux/build_bug.h>
+#include <linux/limits.h>
 #include <linux/typecheck.h>
 #include <asm/byteorder.h>
 
@@ -142,6 +143,52 @@
 		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
 	)
 
+/**
+ * FIELD_PREP_HI16_WE() - prepare a bitfield element with a write-enable mask
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PREP_HI16_WE() masks and shifts up the value, as well as bitwise ORs
+ * the result with the mask shifted up by 16.
+ *
+ * This is useful for a common design of hardware registers where the upper
+ * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
+ * register, a bit in the lower half is only updated if the corresponding bit
+ * in the upper half is high.
+ */
+#define FIELD_PREP_HI16_WE(_mask, _val)					\
+	({								\
+		__BF_FIELD_CHECK(_mask, ((u16) 0U), _val,		\
+				 "FIELD_PREP_HI16_WE: ");		\
+		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |	\
+		((_mask) << 16);					\
+	})
+
+/**
+ * FIELD_PREP_HI16_WE_CONST() - prepare a constant bitfield element with a
+ *                              write-enable mask
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PREP_HI16_WE_CONST() masks and shifts up the value, as well as bitwise
+ * ORs the result with the mask shifted up by 16.
+ *
+ * This is useful for a common design of hardware registers where the upper
+ * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
+ * register, a bit in the lower half is only updated if the corresponding bit
+ * in the upper half is high.
+ *
+ * Unlike FIELD_PREP_HI16_WE(), this is a constant expression and can therefore
+ * be used in initializers. Error checking is less comfortable for this
+ * version, and non-constant masks cannot be used.
+ */
+#define FIELD_PREP_HI16_WE_CONST(_mask, _val)				 \
+	(								 \
+		FIELD_PREP_CONST(_mask, _val) |				 \
+		(BUILD_BUG_ON_ZERO(const_true((u64) (_mask) > U16_MAX)) + \
+		 ((_mask) << 16))					 \
+	)
+
 /**
  * FIELD_GET() - extract a bitfield element
  * @_mask: shifted mask defining the field's length and position

-- 
2.49.0

[PATCH v2 4/7] soc: rockchip: add mfpwm driver

[Nicolas Frattaroli]

With the Rockchip RK3576, the PWM IP used by Rockchip has changed
substantially. Looking at both the downstream pwm-rockchip driver as
well as the mainline pwm-rockchip driver made it clear that with all its
additional features and its differences from previous IP revisions, it
is best supported in a new driver.

This brings us to the question as to what such a new driver should be.
To me, it soon became clear that it should actually be several new
drivers, most prominently when Uwe Kleine-König let me know that I
should not implement the pwm subsystem's capture callback, but instead
write a counter driver for this functionality.

Combined with the other as-of-yet unimplemented functionality of this
new IP, it became apparent that it needs to be spread across several
subsystems.

For this reason, we add a new platform bus based driver, called mfpwm
(short for "Multi-function PWM"). This "parent" driver makes sure that
only one device function driver is using the device at a time, and is in
charge of registering the platform bus devices for the individual device
functions offered by the device.

An acquire/release pattern is used to guarantee that device function
drivers don't step on each other's toes.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 MAINTAINERS                   |   2 +
 drivers/soc/rockchip/Kconfig  |  13 ++
 drivers/soc/rockchip/Makefile |   1 +
 drivers/soc/rockchip/mfpwm.c  | 398 ++++++++++++++++++++++++++++++++++
 include/soc/rockchip/mfpwm.h  | 484 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 898 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ed5cf56b3ebf9e3153cb9171908a1d36c246197d..39177b52be34d48967075b4f5983365725e9c055 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21424,6 +21424,8 @@ L:	linux-rockchip@lists.infradead.org
 L:	linux-pwm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml
+F:	drivers/soc/rockchip/mfpwm.c
+F:	include/soc/rockchip/mfpwm.h
 
 ROCKCHIP RK3568 RANDOM NUMBER GENERATOR SUPPORT
 M:	Daniel Golle <daniel@makrotopia.org>
diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
index 785f60c6f3ad1a09f517e69a69726a8178bed168..4e1e4926c514a5a2c4d4caf8cf9809a098badc7d 100644
--- a/drivers/soc/rockchip/Kconfig
+++ b/drivers/soc/rockchip/Kconfig
@@ -30,4 +30,17 @@ config ROCKCHIP_DTPM
 	  on this platform. That will create all the power capping capable
 	  devices.
 
+config ROCKCHIP_MFPWM
+	tristate "Rockchip multi-function PWM controller"
+	depends on OF
+	depends on HAS_IOMEM
+	help
+	  Some Rockchip SoCs, such as the RK3576, use a PWM controller that has
+	  several different functions, such as generating PWM waveforms but also
+	  counting waveforms.
+
+	  This driver manages the overall device, and selects between different
+	  functionalities at runtime as needed, with drivers for them
+	  implemented in their respective subsystems.
+
 endif
diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
index 23d414433c8c58557effc214337ec8e6ff17a461..ba12dbd01ac794910d9407c268e89071cd2b3139 100644
--- a/drivers/soc/rockchip/Makefile
+++ b/drivers/soc/rockchip/Makefile
@@ -5,3 +5,4 @@
 obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
 obj-$(CONFIG_ROCKCHIP_IODOMAIN) += io-domain.o
 obj-$(CONFIG_ROCKCHIP_DTPM) += dtpm.o
+obj-$(CONFIG_ROCKCHIP_MFPWM) += mfpwm.o
diff --git a/drivers/soc/rockchip/mfpwm.c b/drivers/soc/rockchip/mfpwm.c
new file mode 100644
index 0000000000000000000000000000000000000000..e68d8e71cc590e7ced324cdde7ac79a4b01cae3d
--- /dev/null
+++ b/drivers/soc/rockchip/mfpwm.c
@@ -0,0 +1,398 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2025 Collabora Ltd.
+ *
+ * A driver to manage all the different functionalities exposed by Rockchip's
+ * PWMv4 hardware.
+ *
+ * This driver is chiefly focused on guaranteeing non-concurrent operation
+ * between the different device functions, as well as setting the clocks.
+ * It registers the device function platform devices, e.g. PWM output or
+ * PWM capture.
+ *
+ * Authors:
+ *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
+ */
+
+#include <linux/array_size.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/overflow.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <soc/rockchip/mfpwm.h>
+
+/**
+ * struct rockchip_mfpwm - private mfpwm driver instance state struct
+ * @pdev: pointer to this instance's &struct platform_device
+ * @base: pointer to the memory mapped registers of this device
+ * @pwm_clk: pointer to the PLL clock the PWM signal may be derived from
+ * @osc_clk: pointer to the fixed crystal the PWM signal may be derived from
+ * @rc_clk: pointer to the RC oscillator the PWM signal may be derived from
+ * @chosen_clk: a clk-mux of pwm_clk, osc_clk and rc_clk
+ * @pclk: pointer to the APB bus clock needed for mmio register access
+ * @pwm_dev: pointer to the &struct platform_device of the pwm output driver
+ * @counter_dev: pointer to the &struct platform_device of the counter driver
+ * @active_func: pointer to the currently active device function, or %NULL if no
+ *              device function is currently actively using any of the shared
+ *              resources. May only be checked/modified with @state_lock held.
+ * @acquire_cnt: number of times @active_func has currently mfpwm_acquire()'d
+ *               it. Must only be checked or modified while holding @state_lock.
+ * @state_lock: this lock is held while either the active device function, the
+ *              enable register, or the chosen clock is being changed.
+ * @irq: the IRQ number of this device
+ */
+struct rockchip_mfpwm {
+	struct platform_device *pdev;
+	void __iomem *base;
+	struct clk *pwm_clk;
+	struct clk *osc_clk;
+	struct clk *rc_clk;
+	struct clk *chosen_clk;
+	struct clk *pclk;
+	struct platform_device *pwm_dev;
+	struct platform_device *counter_dev;
+	struct rockchip_mfpwm_func *active_func;
+	unsigned int acquire_cnt;
+	spinlock_t state_lock;
+	int irq;
+};
+
+static atomic_t subdev_id = ATOMIC_INIT(0);
+
+static inline struct rockchip_mfpwm *to_rockchip_mfpwm(struct platform_device *pdev)
+{
+	return platform_get_drvdata(pdev);
+}
+
+static int mfpwm_check_pwmf(const struct rockchip_mfpwm_func *pwmf,
+			    const char *fname)
+{
+	struct device *dev = &pwmf->parent->pdev->dev;
+
+	if (IS_ERR_OR_NULL(pwmf)) {
+		dev_warn(dev, "called %s with an erroneous handle, no effect\n",
+			 fname);
+		return -EINVAL;
+	}
+
+	if (IS_ERR_OR_NULL(pwmf->parent)) {
+		dev_warn(dev, "called %s with an erroneous mfpwm_func parent, no effect\n",
+			 fname);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+__attribute__((nonnull))
+static bool mfpwm_pwmf_is_active_pwmf(const struct rockchip_mfpwm_func *pwmf)
+{
+	if (pwmf->parent->active_func) {
+		if (pwmf->parent->active_func->id == pwmf->id)
+			return true;
+	}
+
+	return false;
+}
+
+__attribute__((nonnull))
+static int mfpwm_do_acquire(struct rockchip_mfpwm_func *pwmf)
+{
+	struct rockchip_mfpwm *mfpwm = pwmf->parent;
+	unsigned int cnt;
+
+	if (mfpwm->active_func && pwmf->id != mfpwm->active_func->id)
+		return -EBUSY;
+
+	if (!mfpwm->active_func)
+		mfpwm->active_func = pwmf;
+
+	if (!check_add_overflow(mfpwm->acquire_cnt, 1, &cnt)) {
+		mfpwm->acquire_cnt = cnt;
+	} else {
+		dev_warn(&mfpwm->pdev->dev, "prevented acquire counter overflow in %s\n",
+			 __func__);
+		return -EOVERFLOW;
+	}
+
+	dev_dbg(&mfpwm->pdev->dev, "%d acquired mfpwm, acquires now at %u\n",
+		pwmf->id, mfpwm->acquire_cnt);
+
+	return clk_enable(mfpwm->pclk);
+}
+
+int mfpwm_acquire(struct rockchip_mfpwm_func *pwmf)
+{
+	struct rockchip_mfpwm *mfpwm;
+	unsigned long flags;
+	int ret = 0;
+
+	ret = mfpwm_check_pwmf(pwmf, "mfpwm_acquire");
+	if (ret)
+		return ret;
+
+	mfpwm = pwmf->parent;
+	dev_dbg(&mfpwm->pdev->dev, "%d is attempting to acquire\n", pwmf->id);
+
+	if (!spin_trylock_irqsave(&mfpwm->state_lock, flags))
+		return -EBUSY;
+
+	ret = mfpwm_do_acquire(pwmf);
+
+	spin_unlock_irqrestore(&mfpwm->state_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(mfpwm_acquire, "ROCKCHIP_MFPWM");
+
+__attribute__((nonnull))
+static void mfpwm_do_release(const struct rockchip_mfpwm_func *pwmf)
+{
+	struct rockchip_mfpwm *mfpwm = pwmf->parent;
+
+	if (!mfpwm->active_func)
+		return;
+
+	if (mfpwm->active_func->id != pwmf->id)
+		return;
+
+	/*
+	 * No need to check_sub_overflow here, !mfpwm->active_func above catches
+	 * this type of problem already.
+	 */
+	mfpwm->acquire_cnt--;
+
+	if (!mfpwm->acquire_cnt)
+		mfpwm->active_func = NULL;
+
+	clk_disable(mfpwm->pclk);
+}
+
+void mfpwm_release(const struct rockchip_mfpwm_func *pwmf)
+{
+	struct rockchip_mfpwm *mfpwm;
+	unsigned long flags;
+
+	if (mfpwm_check_pwmf(pwmf, "mfpwm_release"))
+		return;
+
+	mfpwm = pwmf->parent;
+
+	spin_lock_irqsave(&mfpwm->state_lock, flags);
+	mfpwm_do_release(pwmf);
+	dev_dbg(&mfpwm->pdev->dev, "%d released mfpwm, acquires now at %u\n",
+		pwmf->id, mfpwm->acquire_cnt);
+	spin_unlock_irqrestore(&mfpwm->state_lock, flags);
+}
+EXPORT_SYMBOL_NS_GPL(mfpwm_release, "ROCKCHIP_MFPWM");
+
+void mfpwm_remove_func(struct rockchip_mfpwm_func *pwmf)
+{
+	struct rockchip_mfpwm *mfpwm;
+	unsigned long flags;
+
+	if (mfpwm_check_pwmf(pwmf, "mfpwm_remove_func"))
+		return;
+
+	mfpwm = pwmf->parent;
+	spin_lock_irqsave(&mfpwm->state_lock, flags);
+
+	if (mfpwm_pwmf_is_active_pwmf(pwmf)) {
+		dev_dbg(&mfpwm->pdev->dev, "removing active function %d\n",
+			pwmf->id);
+
+		while (mfpwm->acquire_cnt > 0)
+			mfpwm_do_release(pwmf);
+
+		mfpwm_reg_write(mfpwm->base, PWMV4_REG_ENABLE,
+				PWMV4_EN(false) | PWMV4_CLK_EN(false));
+	}
+
+	if (mfpwm->pwm_dev && mfpwm->pwm_dev->id == pwmf->id) {
+		dev_dbg(&mfpwm->pdev->dev, "clearing pwm_dev pointer\n");
+		mfpwm->pwm_dev = NULL;
+	} else if (mfpwm->counter_dev && mfpwm->counter_dev->id == pwmf->id) {
+		dev_dbg(&mfpwm->pdev->dev, "clearing counter_dev pointer\n");
+		mfpwm->counter_dev = NULL;
+	} else {
+		dev_warn(&mfpwm->pdev->dev, "trying to remove an unknown mfpwm device function");
+	}
+
+	spin_unlock_irqrestore(&mfpwm->state_lock, flags);
+}
+EXPORT_SYMBOL_NS_GPL(mfpwm_remove_func, "ROCKCHIP_MFPWM");
+
+/**
+ * mfpwm_register_subdev - register a single mfpwm_func
+ * @mfpwm: pointer to the parent &struct rockchip_mfpwm
+ * @target: pointer to where the &struct platform_device pointer should be
+ *          stored, usually a member of @mfpwm
+ * @name: sub-device name string
+ *
+ * Allocate a single &struct mfpwm_func, fill its members with appropriate data,
+ * and register a new platform device, saving its pointer to @target. The
+ * allocation is devres tracked, so will be automatically freed on mfpwm remove.
+ *
+ * Returns: 0 on success, negative errno on error
+ */
+static int mfpwm_register_subdev(struct rockchip_mfpwm *mfpwm,
+				 struct platform_device **target,
+				 const char *name)
+{
+	struct rockchip_mfpwm_func *func;
+	struct platform_device *child;
+
+	func = devm_kzalloc(&mfpwm->pdev->dev, sizeof(*func), GFP_KERNEL);
+	if (IS_ERR(func))
+		return PTR_ERR(func);
+	func->irq = mfpwm->irq;
+	func->parent = mfpwm;
+	func->id = atomic_inc_return(&subdev_id);
+	func->base = mfpwm->base;
+	func->core = mfpwm->chosen_clk;
+	child = platform_device_register_data(&mfpwm->pdev->dev, name, func->id,
+					      func, sizeof(*func));
+
+	if (IS_ERR(child))
+		return PTR_ERR(child);
+
+	*target = child;
+
+	return 0;
+}
+
+static int mfpwm_register_subdevs(struct rockchip_mfpwm *mfpwm)
+{
+	int ret;
+
+	ret = mfpwm_register_subdev(mfpwm, &mfpwm->pwm_dev, "pwm-rockchip-v4");
+	if (ret)
+		return ret;
+
+	ret = mfpwm_register_subdev(mfpwm, &mfpwm->counter_dev,
+				    "rockchip-pwm-capture");
+	if (ret)
+		goto err_unreg_pwm_dev;
+
+	return 0;
+
+err_unreg_pwm_dev:
+	platform_device_unregister(mfpwm->pwm_dev);
+
+	return ret;
+}
+
+static int rockchip_mfpwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rockchip_mfpwm *mfpwm;
+	char *clk_mux_name;
+	const char *mux_p_names[3];
+	int ret = 0;
+
+	mfpwm = devm_kzalloc(&pdev->dev, sizeof(*mfpwm), GFP_KERNEL);
+	if (IS_ERR(mfpwm))
+		return PTR_ERR(mfpwm);
+
+	mfpwm->pdev = pdev;
+
+	spin_lock_init(&mfpwm->state_lock);
+
+	mfpwm->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mfpwm->base))
+		return dev_err_probe(dev, PTR_ERR(mfpwm->base),
+				     "failed to ioremap address\n");
+
+	mfpwm->pclk = devm_clk_get_prepared(dev, "pclk");
+	if (IS_ERR(mfpwm->pclk))
+		return dev_err_probe(dev, PTR_ERR(mfpwm->pclk),
+				     "couldn't get and prepare 'pclk' clock\n");
+
+	mfpwm->irq = platform_get_irq(pdev, 0);
+	if (mfpwm->irq < 0)
+		return dev_err_probe(dev, mfpwm->irq, "couldn't get irq 0\n");
+
+	mfpwm->pwm_clk = devm_clk_get_prepared(dev, "pwm");
+	if (IS_ERR(mfpwm->pwm_clk))
+		return dev_err_probe(dev, PTR_ERR(mfpwm->pwm_clk),
+				     "couldn't get and prepare 'pwm' clock\n");
+
+	mfpwm->osc_clk = devm_clk_get_prepared(dev, "osc");
+	if (IS_ERR(mfpwm->osc_clk))
+		return dev_err_probe(dev, PTR_ERR(mfpwm->osc_clk),
+				     "couldn't get and prepare 'osc' clock\n");
+
+	mfpwm->rc_clk = devm_clk_get_prepared(dev, "rc");
+	if (IS_ERR(mfpwm->rc_clk))
+		return dev_err_probe(dev, PTR_ERR(mfpwm->rc_clk),
+				     "couldn't get and prepare 'rc' clock\n");
+
+	clk_mux_name = devm_kasprintf(dev, GFP_KERNEL, "%s_chosen", dev_name(dev));
+	if (!clk_mux_name)
+		return -ENOMEM;
+
+	mux_p_names[0] = __clk_get_name(mfpwm->pwm_clk);
+	mux_p_names[1] = __clk_get_name(mfpwm->osc_clk);
+	mux_p_names[2] = __clk_get_name(mfpwm->rc_clk);
+	mfpwm->chosen_clk = clk_register_mux(dev, clk_mux_name, mux_p_names,
+					     ARRAY_SIZE(mux_p_names),
+					     CLK_SET_RATE_PARENT,
+					     mfpwm->base + PWMV4_REG_CLK_CTRL,
+					     PWMV4_CLK_SRC_SHIFT, PWMV4_CLK_SRC_WIDTH,
+					     CLK_MUX_HIWORD_MASK, NULL);
+	ret = clk_prepare(mfpwm->chosen_clk);
+	if (ret) {
+		dev_err(dev, "failed to prepare PWM clock mux: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, mfpwm);
+
+	ret = mfpwm_register_subdevs(mfpwm);
+	if (ret) {
+		dev_err(dev, "failed to register sub-devices: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	return ret;
+}
+
+static void rockchip_mfpwm_remove(struct platform_device *pdev)
+{
+	struct rockchip_mfpwm *mfpwm = to_rockchip_mfpwm(pdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&mfpwm->state_lock, flags);
+
+	if (mfpwm->chosen_clk)
+		clk_unregister_mux(mfpwm->chosen_clk);
+
+	spin_unlock_irqrestore(&mfpwm->state_lock, flags);
+}
+
+static const struct of_device_id rockchip_mfpwm_of_match[] = {
+	{
+		.compatible = "rockchip,rk3576-pwm",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rockchip_mfpwm_of_match);
+
+static struct platform_driver rockchip_mfpwm_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = rockchip_mfpwm_of_match,
+	},
+	.probe = rockchip_mfpwm_probe,
+	.remove = rockchip_mfpwm_remove,
+};
+module_platform_driver(rockchip_mfpwm_driver);
+
+MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
+MODULE_DESCRIPTION("Rockchip MFPWM Driver");
+MODULE_LICENSE("GPL");
diff --git a/include/soc/rockchip/mfpwm.h b/include/soc/rockchip/mfpwm.h
new file mode 100644
index 0000000000000000000000000000000000000000..05fc63ffb2f3155e13b254fc1492de77d03ff087
--- /dev/null
+++ b/include/soc/rockchip/mfpwm.h
@@ -0,0 +1,484 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2025 Collabora Ltd.
+ *
+ * Common header file for all the Rockchip Multi-function PWM controller
+ * drivers that are spread across subsystems.
+ *
+ * Authors:
+ *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
+ */
+
+#ifndef __SOC_ROCKCHIP_MFPWM_H__
+#define __SOC_ROCKCHIP_MFPWM_H__
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
+
+struct rockchip_mfpwm;
+
+/**
+ * struct rockchip_mfpwm_func - struct representing a single function driver
+ *
+ * @id: unique id for this function driver instance
+ * @base: pointer to start of MMIO registers
+ * @parent: a pointer to the parent mfpwm struct
+ * @irq: the shared IRQ gotten from the parent mfpwm device
+ * @core: a pointer to the clk mux that drives this channel's PWM
+ */
+struct rockchip_mfpwm_func {
+	int id;
+	void __iomem *base;
+	struct rockchip_mfpwm *parent;
+	int irq;
+	struct clk *core;
+};
+
+/*
+ * PWMV4 Register Definitions
+ * --------------------------
+ *
+ * Attributes:
+ *  RW  - Read-Write
+ *  RO  - Read-Only
+ *  WO  - Write-Only
+ *  W1T - Write high, Self-clearing
+ *  W1C - Write high to clear interrupt
+ *
+ * Bit ranges to be understood with Verilog-like semantics,
+ * e.g. [03:00] is 4 bits: 0, 1, 2 and 3.
+ *
+ * All registers must be accessed with 32-bit width accesses only
+ */
+
+#define PWMV4_REG_VERSION		0x000
+/*
+ * VERSION Register Description
+ * [31:24] RO  | Hardware Major Version
+ * [23:16] RO  | Hardware Minor Version			docArrive
+ * [15:15] RO  | Reserved
+ * [14:14] RO  | Hardware supports biphasic counters
+ * [13:13] RO  | Hardware supports filters
+ * [12:12] RO  | Hardware supports waveform generation
+ * [11:11] RO  | Hardware supports counter
+ * [10:10] RO  | Hardware supports frequency metering
+ * [09:09] RO  | Hardware supports power key functionality
+ * [08:08] RO  | Hardware supports infrared transmissions
+ * [07:04] RO  | Channel index of this instance
+ * [03:00] RO  | Number of channels the base instance supports
+ */
+static inline __pure u32 rockchip_pwm_v4_ver_chn_num(u32 val)
+{
+	return (val & GENMASK(3, 0));
+}
+
+static inline __pure u32 rockchip_pwm_v4_ver_chn_idx(u32 val)
+{
+	return (val & GENMASK(7, 4)) >> 4;
+}
+
+static inline __pure u32 rockchip_pwm_v4_ver_major(u32 val)
+{
+	return (val & GENMASK(31, 24)) >> 24;
+}
+
+static inline __pure u32 rockchip_pwm_v4_ver_minor(u32 val)
+{
+	return (val & GENMASK(23, 16)) >> 16;
+}
+
+#define PWMV4_REG_ENABLE		0x004
+/*
+ * ENABLE Register Description
+ * [31:16] WO  | Write Enable Mask for the lower half of the register
+ *               Set bit `n` here to 1 if you wish to modify bit `n >> 16` in
+ *               the same write operation
+ * [15:06] RO  | Reserved
+ * [05:05] RW  | PWM Channel Counter Read Enable, 1 = enabled
+ */
+#define PWMV4_CHN_CNT_RD_EN(v)		FIELD_PREP_HI16_WE(BIT(5), (v))
+/*
+ * [04:04] W1T | PWM Globally Joined Control Enable
+ *               1 = this PWM channel will be enabled by a global pwm enable
+ *               bit instead of the PWM Enable bit.
+ */
+#define PWMV4_GLOBAL_CTRL_EN(v)		FIELD_PREP_HI16_WE(BIT(4), (v))
+/*
+ * [03:03] RW  | Force Clock Enable
+ *               0 = disabled, if the PWM channel is inactive then so is the
+ *               clock prescale module
+ */
+#define PWMV4_FORCE_CLK_EN(v)		FIELD_PREP_HI16_WE(BIT(3), (v))
+/*
+ * [02:02] W1T | PWM Control Update Enable
+ *               1 = enabled, commits modifications of _CTRL, _PERIOD, _DUTY and
+ *               _OFFSET registers once 1 is written to it
+ */
+#define PWMV4_CTRL_UPDATE_EN		FIELD_PREP_HI16_WE_CONST(BIT(2), 1)
+/*
+ * [01:01] RW  | PWM Enable, 1 = enabled
+ *               If in one-shot mode, clears after end of operation
+ */
+#define PWMV4_EN_MASK			BIT(1)
+#define PWMV4_EN(v)			FIELD_PREP_HI16_WE(PWMV4_EN_MASK, \
+							   ((v) ? 1 : 0))
+/*
+ * [00:00] RW  | PWM Clock Enable, 1 = enabled
+ *               If in one-shot mode, clears after end of operation
+ */
+#define PWMV4_CLK_EN_MASK		BIT(0)
+#define PWMV4_CLK_EN(v)			FIELD_PREP_HI16_WE(PWMV4_CLK_EN_MASK, \
+							   ((v) ? 1 : 0))
+#define PWMV4_EN_BOTH_MASK		(PWMV4_EN_MASK | PWMV4_CLK_EN_MASK)
+static inline __pure bool rockchip_pwm_v4_is_enabled(unsigned int val)
+{
+	return (val & PWMV4_EN_BOTH_MASK);
+}
+
+#define PWMV4_REG_CLK_CTRL		0x008
+/*
+ * CLK_CTRL Register Description
+ * [31:16] WO  | Write Enable Mask for the lower half of the register
+ *               Set bit `n` here to 1 if you wish to modify bit `n >> 16` in
+ *               the same write operation
+ * [15:15] RW  | Clock Global Selection
+ *               0 = current channel scale clock
+ *               1 = global channel scale clock
+ */
+#define PWMV4_CLK_GLOBAL(v)		FIELD_PREP_HI16_WE(BIT(15), (v))
+/*
+ * [14:13] RW  | Clock Source Selection
+ *               0 = Clock from PLL, frequency can be configured
+ *               1 = Clock from crystal oscillator, frequency is fixed
+ *               2 = Clock from RC oscillator, frequency is fixed
+ *               3 = Reserved
+ *               NOTE: The purpose for this clock-mux-outside-CRU construct is
+ *                     to let the SoC go into a sleep state with the PWM
+ *                     hardware still having a clock signal for IR input, which
+ *                     can then wake up the SoC.
+ */
+#define PWMV4_CLK_SRC_PLL		0x0U
+#define PWMV4_CLK_SRC_CRYSTAL		0x1U
+#define PWMV4_CLK_SRC_RC		0x2U
+#define PWMV4_CLK_SRC_SHIFT		13
+#define PWMV4_CLK_SRC_WIDTH		2
+/*
+ * [12:04] RW  | Scale Factor to apply to pre-scaled clock
+ *               1 <= v <= 256, v means clock divided by 2*v
+ */
+#define PWMV4_CLK_SCALE_F(v)		FIELD_PREP_HI16_WE(GENMASK(12, 4), (v))
+/*
+ * [03:03] RO  | Reserved
+ * [02:00] RW  | Prescale Factor
+ *               v here means the input clock is divided by pow(2, v)
+ */
+#define PWMV4_CLK_PRESCALE_F(v)		FIELD_PREP_HI16_WE(GENMASK(2, 0), (v))
+
+#define PWMV4_REG_CTRL			0x00C
+/*
+ * CTRL Register Description
+ * [31:16] WO  | Write Enable Mask for the lower half of the register
+ *               Set bit `n` here to 1 if you wish to modify bit `n >> 16` in
+ *               the same write operation
+ * [15:09] RO  | Reserved
+ * [08:06] RW  | PWM Input Channel Selection
+ *               By default, the channel selects its own input, but writing v
+ *               here selects PWM input from channel v instead.
+ */
+#define PWMV4_CTRL_IN_SEL(v)		FIELD_PREP_HI16_WE(GENMASK(8, 6), (v))
+/* [05:05] RW  | Aligned Mode, 0 = Valid, 1 = Invalid */
+#define PWMV4_CTRL_UNALIGNED(v)		FIELD_PREP_HI16_WE(BIT(5), (v))
+/* [04:04] RW  | Output Mode, 0 = Left Aligned, 1 = Centre Aligned */
+#define PWMV4_LEFT_ALIGNED		0x0U
+#define PWMV4_CENTRE_ALIGNED		0x1U
+#define PWMV4_CTRL_OUT_MODE(v)		FIELD_PREP_HI16_WE(BIT(4), (v))
+/*
+ * [03:03] RW  | Inactive Polarity for when the channel is either disabled or
+ *               has completed outputting the entire waveform in one-shot mode.
+ *               0 = Negative, 1 = Positive
+ */
+#define PWMV4_POLARITY_N		0x0U
+#define PWMV4_POLARITY_P		0x1U
+#define PWMV4_INACTIVE_POL(v)		FIELD_PREP_HI16_WE(BIT(3), (v))
+/*
+ * [02:02] RW  | Duty Cycle Polarity to use at the start of the waveform.
+ *               0 = Negative, 1 = Positive
+ */
+#define PWMV4_DUTY_POL_SHIFT		2
+#define PWMV4_DUTY_POL_MASK		BIT(PWMV4_DUTY_POL_SHIFT)
+#define PWMV4_DUTY_POL(v)		FIELD_PREP_HI16_WE(PWMV4_DUTY_POL_MASK, \
+							   (v))
+/*
+ * [01:00] RW  | PWM Mode
+ *               0 = One-shot mode, PWM generates waveform RPT times
+ *               1 = Continuous mode
+ *               2 = Capture mode, PWM measures cycles of input waveform
+ *               3 = Reserved
+ */
+#define PWMV4_MODE_ONESHOT		0x0U
+#define PWMV4_MODE_CONT			0x1U
+#define PWMV4_MODE_CAPTURE		0x2U
+#define PWMV4_MODE_MASK			GENMASK(1, 0)
+#define PWMV4_MODE(v)			FIELD_PREP_HI16_WE(PWMV4_MODE_MASK, (v))
+#define PWMV4_CTRL_COM_FLAGS	(PWMV4_INACTIVE_POL(PWMV4_POLARITY_N) | \
+				 PWMV4_DUTY_POL(PWMV4_POLARITY_P) | \
+				 PWMV4_CTRL_OUT_MODE(PWMV4_LEFT_ALIGNED) | \
+				 PWMV4_CTRL_UNALIGNED(true))
+#define PWMV4_CTRL_CONT_FLAGS	(PWMV4_MODE(PWMV4_MODE_CONT) | \
+				 PWMV4_CTRL_COM_FLAGS)
+#define PWMV4_CTRL_CAP_FLAGS	(PWMV4_MODE(PWMV4_MODE_CAPTURE) | \
+				 PWMV4_CTRL_COM_FLAGS)
+
+#define PWMV4_REG_PERIOD		0x010
+/*
+ * PERIOD Register Description
+ * [31:00] RW  | Period of the output waveform
+ *               Constraints: should be even if CTRL_OUT_MODE is CENTRE_ALIGNED
+ */
+
+#define PWMV4_REG_DUTY			0x014
+/*
+ * DUTY Register Description
+ * [31:00] RW  | Duty cycle of the output waveform
+ *               Constraints: should be even if CTRL_OUT_MODE is CENTRE_ALIGNED
+ */
+
+#define PWMV4_REG_OFFSET		0x018
+/*
+ * OFFSET Register Description
+ * [31:00] RW  | Offset of the output waveform, based on the PWM clock
+ *               Constraints: 0 <= v <= (PERIOD - DUTY)
+ */
+
+#define PWMV4_REG_RPT			0x01C
+/*
+ * RPT Register Description
+ * [31:16] RW  | Second dimensional of the effective number of waveform
+ *               repetitions. Increases by one every first dimensional times.
+ *               Value `n` means `n + 1` repetitions. The final number of
+ *               repetitions of the waveform in one-shot mode is:
+ *               `(first_dimensional + 1) * (second_dimensional + 1)`
+ * [15:00] RW  | First dimensional of the effective number of waveform
+ *               repetitions. Value `n` means `n + 1` repetitions.
+ */
+
+#define PWMV4_REG_FILTER_CTRL		0x020
+/*
+ * FILTER_CTRL Register Description
+ * [31:16] WO  | Write Enable Mask for the lower half of the register
+ *               Set bit `n` here to 1 if you wish to modify bit `n >> 16` in
+ *               the same write operation
+ * [15:10] RO  | Reserved
+ * [09:04] RW  | Filter window number
+ * [03:01] RO  | Reserved
+ * [00:00] RW  | Filter Enable, 0 = disabled, 1 = enabled
+ */
+
+#define PWMV4_REG_CNT			0x024
+/*
+ * CNT Register Description
+ * [31:00] RO  | Current value of the PWM Channel 0 counter in pwm clock cycles,
+ *               0 <= v <= 2^32-1
+ */
+
+#define PWMV4_REG_ENABLE_DELAY		0x028
+/*
+ * ENABLE_DELAY Register Description
+ * [31:16] RO  | Reserved
+ * [15:00] RW  | PWM enable delay, in an unknown unit but probably cycles
+ */
+
+#define PWMV4_REG_HPC			0x02C
+/*
+ * HPC Register Description
+ * [31:00] RW  | Number of effective high polarity cycles of the input waveform
+ *               in capture mode. Based on the PWM clock. 0 <= v <= 2^32-1
+ */
+
+#define PWMV4_REG_LPC			0x030
+/*
+ * LPC Register Description
+ * [31:00] RW  | Number of effective low polarity cycles of the input waveform
+ *               in capture mode. Based on the PWM clock. 0 <= v <= 2^32-1
+ */
+
+#define PWMV4_REG_BIPHASIC_CNT_CTRL0	0x040
+/*
+ * BIPHASIC_CNT_CTRL0 Register Description
+ * [31:16] WO  | Write Enable Mask for the lower half of the register
+ *               Set bit `n` here to 1 if you wish to modify bit `n >> 16` in
+ *               the same write operation
+ * [15:10] RO  | Reserved
+ * [09:09] RW  | Biphasic Counter Phase Edge Selection for mode 0,
+ *               0 = rising edge (posedge), 1 = falling edge (negedge)
+ * [08:08] RW  | Biphasic Counter Clock force enable, 1 = force enable
+ * [07:07] W1T | Synchronous Enable
+ * [06:06] W1T | Mode Switch
+ *               0 = Normal Mode, 1 = Switch timer clock and measured clock
+ *               Constraints: "Biphasic Counter Mode" must be 0 if this is 1
+ * [05:03] RW  | Biphasic Counter Mode
+ *               0x0 = Mode 0, 0x1 = Mode 1, 0x2 = Mode 2, 0x3 = Mode 3,
+ *               0x4 = Mode 4, 0x5 = Reserved
+ * [02:02] RW  | Biphasic Counter Clock Selection
+ *               0 = clock is from PLL and frequency can be configured
+ *               1 = clock is from crystal oscillator and frequency is fixed
+ * [01:01] RW  | Biphasic Counter Continuous Mode
+ * [00:00] W1T | Biphasic Counter Enable
+ */
+
+#define PWMV4_REG_BIPHASIC_CNT_CTRL1	0x044
+/*
+ * BIPHASIC_CNT_CTRL1 Register Description
+ * [31:16] WO  | Write Enable Mask for the lower half of the register
+ *               Set bit `n` here to 1 if you wish to modify bit `n >> 16` in
+ *               the same write operation
+ * [15:11] RO  | Reserved
+ * [10:04] RW  | Biphasic Counter Filter Window Number
+ * [03:01] RO  | Reserved
+ * [00:00] RW  | Biphasic Counter Filter Enable
+ */
+
+#define PWMV4_REG_BIPHASIC_CNT_TIMER	0x048
+/*
+ * BIPHASIC_CNT_TIMER Register Description
+ * [31:00] RW  | Biphasic Counter Timer Value, in number of biphasic counter
+ *               timer clock cycles
+ */
+
+#define PWMV4_REG_BIPHASIC_CNT_RES	0x04C
+/*
+ * BIPHASIC_CNT_RES Register Description
+ * [31:00] RO  | Biphasic Counter Result Value
+ *               Constraints: Can only be read after INTSTS[9] is asserted
+ */
+
+#define PWMV4_REG_BIPHASIC_CNT_RES_S	0x050
+/*
+ * BIPHASIC_CNT_RES_S Register Description
+ * [31:00] RO  | Biphasic Counter Result Value with synchronised processing
+ *               Can be read in real-time if BIPHASIC_CNT_CTRL0[7] was set to 1
+ */
+
+#define PWMV4_REG_INTSTS		0x070
+/*
+ * INTSTS Register Description
+ * [31:10] RO  | Reserved
+ * [09:09] W1C | Biphasic Counter Interrupt Status, 1 = interrupt asserted
+ * [08:08] W1C | Waveform Middle Interrupt Status, 1 = interrupt asserted
+ * [07:07] W1C | Waveform Max Interrupt Status, 1 = interrupt asserted
+ * [06:06] W1C | IR Transmission End Interrupt Status, 1 = interrupt asserted
+ * [05:05] W1C | Power Key Match Interrupt Status, 1 = interrupt asserted
+ * [04:04] W1C | Frequency Meter Interrupt Status, 1 = interrupt asserted
+ * [03:03] W1C | Reload Interrupt Status, 1 = interrupt asserted
+ * [02:02] W1C | Oneshot End Interrupt Status, 1 = interrupt asserted
+ * [01:01] W1C | HPC Capture Interrupt Status, 1 = interrupt asserted
+ * [00:00] W1C | LPC Capture Interrupt Status, 1 = interrupt asserted
+ */
+#define PWMV4_INT_LPC			BIT(0)
+#define PWMV4_INT_HPC			BIT(1)
+#define PWMV4_INT_LPC_W(v)		FIELD_PREP_HI16_WE(PWMV4_INT_LPC, \
+							   ((v) ? 1 : 0))
+#define PWMV4_INT_HPC_W(v)		FIELD_PREP_HI16_WE(PWMV4_INT_HPC, \
+							   ((v) ? 1 : 0))
+
+#define PWMV4_REG_INT_EN		0x074
+/*
+ * INT_EN Register Description
+ * [31:16] WO  | Write Enable Mask for the lower half of the register
+ *               Set bit `n` here to 1 if you wish to modify bit `n >> 16` in
+ *               the same write operation
+ * [15:10] RO  | Reserved
+ * [09:09] RW  | Biphasic Counter Interrupt Enable, 1 = enabled
+ * [08:08] W1C | Waveform Middle Interrupt Enable, 1 = enabled
+ * [07:07] W1C | Waveform Max Interrupt Enable, 1 = enabled
+ * [06:06] W1C | IR Transmission End Interrupt Enable, 1 = enabled
+ * [05:05] W1C | Power Key Match Interrupt Enable, 1 = enabled
+ * [04:04] W1C | Frequency Meter Interrupt Enable, 1 = enabled
+ * [03:03] W1C | Reload Interrupt Enable, 1 = enabled
+ * [02:02] W1C | Oneshot End Interrupt Enable, 1 = enabled
+ * [01:01] W1C | HPC Capture Interrupt Enable, 1 = enabled
+ * [00:00] W1C | LPC Capture Interrupt Enable, 1 = enabled
+ */
+
+#define PWMV4_REG_INT_MASK		0x078
+/*
+ * INT_MASK Register Description
+ * [31:16] WO  | Write Enable Mask for the lower half of the register
+ *               Set bit `n` here to 1 if you wish to modify bit `n >> 16` in
+ *               the same write operation
+ * [15:10] RO  | Reserved
+ * [09:09] RW  | Biphasic Counter Interrupt Masked, 1 = masked
+ * [08:08] W1C | Waveform Middle Interrupt Masked, 1 = masked
+ * [07:07] W1C | Waveform Max Interrupt Masked, 1 = masked
+ * [06:06] W1C | IR Transmission End Interrupt Masked, 1 = masked
+ * [05:05] W1C | Power Key Match Interrupt Masked, 1 = masked
+ * [04:04] W1C | Frequency Meter Interrupt Masked, 1 = masked
+ * [03:03] W1C | Reload Interrupt Masked, 1 = masked
+ * [02:02] W1C | Oneshot End Interrupt Masked, 1 = masked
+ * [01:01] W1C | HPC Capture Interrupt Masked, 1 = masked
+ * [00:00] W1C | LPC Capture Interrupt Masked, 1 = masked
+ */
+
+static inline u32 mfpwm_reg_read(void __iomem *base, u32 reg)
+{
+	return readl(base + reg);
+}
+
+static inline void mfpwm_reg_write(void __iomem *base, u32 reg, u32 val)
+{
+	writel(val, base + reg);
+}
+
+/**
+ * mfpwm_acquire - try becoming the active mfpwm function device
+ * @pwmf: pointer to the calling driver instance's &struct rockchip_mfpwm_func
+ *
+ * mfpwm device "function" drivers must call this function before doing anything
+ * that either modifies or relies on the parent device's state, such as clocks,
+ * enabling/disabling outputs, modifying shared regs etc.
+ *
+ * The return statues should always be checked.
+ *
+ * All mfpwm_acquire() calls must be balanced with corresponding mfpwm_release()
+ * calls once the device is no longer making changes that affect other devices,
+ * or stops producing user-visible effects that depend on the current device
+ * state being kept as-is. (e.g. after the PWM output signal is stopped)
+ *
+ * The same device function may mfpwm_acquire() multiple times while it already
+ * is active, i.e. it is re-entrant, though it needs to balance this with the
+ * same number of mfpwm_release() calls.
+ *
+ * Context: This function does not sleep.
+ *
+ * Return:
+ * * %0                 - success
+ * * %-EBUSY            - a different device function is active
+ * * %-EOVERFLOW        - the acquire counter is at its maximum
+ */
+int __must_check mfpwm_acquire(struct rockchip_mfpwm_func *pwmf);
+
+/**
+ * mfpwm_release - drop usage of active mfpwm device function by 1
+ * @pwmf: pointer to the calling driver instance's &struct rockchip_mfpwm_func
+ *
+ * This is the balancing call to mfpwm_acquire(). If no users of the device
+ * function remain, set the mfpwm device to have no active device function,
+ * allowing other device functions to claim it.
+ */
+void mfpwm_release(const struct rockchip_mfpwm_func *pwmf);
+
+/**
+ * mfpwm_remove_func - remove a device function driver from the mfpwm
+ * @pwmf: pointer to the calling driver instance's &struct rockchip_mfpwm_func
+ *
+ * If the device function driver described by @pwmf is the currently active
+ * device function, then it'll have its mfpwm_acquires balanced and be removed
+ * as the active device function driver. Should only be called once all the
+ * clock enables have been balanced, as the function cannot balance this.
+ */
+void mfpwm_remove_func(struct rockchip_mfpwm_func *pwmf);
+
+#endif /* __SOC_ROCKCHIP_MFPWM_H__ */

-- 
2.49.0

[PATCH v2 5/7] pwm: Add rockchip PWMv4 driver

[Nicolas Frattaroli]

The Rockchip RK3576 brings with it a new PWM IP, in downstream code
referred to as "v4". This new IP is different enough from the previous
Rockchip IP that I felt it necessary to add a new driver for it, instead
of shoehorning it in the old one.

Add this new driver, based on the PWM core's waveform APIs. Its platform
device is registered by the parent mfpwm driver, from which it also
receives a little platform data struct, so that mfpwm can guarantee that
all the platform device drivers spread across different subsystems for
this specific hardware IP do not interfere with each other.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 MAINTAINERS                   |   1 +
 drivers/pwm/Kconfig           |  13 ++
 drivers/pwm/Makefile          |   1 +
 drivers/pwm/pwm-rockchip-v4.c | 372 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 387 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 39177b52be34d48967075b4f5983365725e9c055..0014af1cbd3a7729a04bfdd0b0ed50e9df425693 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21424,6 +21424,7 @@ L:	linux-rockchip@lists.infradead.org
 L:	linux-pwm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml
+F:	drivers/pwm/pwm-rockchip-v4.c
 F:	drivers/soc/rockchip/mfpwm.c
 F:	include/soc/rockchip/mfpwm.h
 
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index d9bcd1e8413eaed1602d6686873e263767c58f5f..903138128bca910276fe16efc28f55d05657e385 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -586,6 +586,19 @@ config PWM_ROCKCHIP
 	  Generic PWM framework driver for the PWM controller found on
 	  Rockchip SoCs.
 
+config PWM_ROCKCHIP_V4
+	tristate "Rockchip PWM v4 support"
+	depends on ARCH_ROCKCHIP || COMPILE_TEST
+	depends on ROCKCHIP_MFPWM
+	depends on HAS_IOMEM
+	help
+	  Generic PWM framework driver for the PWM controller found on
+	  later Rockchip SoCs such as the RK3576.
+
+	  Uses the Rockchip Multi-function PWM controller driver infrastructure
+	  to guarantee fearlessly concurrent operation with other functions of
+	  the same device implemented by drivers in other subsystems.
+
 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 96160f4257fcb0e0951581af0090615c0edf5260..c03083de5dbf38d68caee6b7e089ddaa235b538b 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_PWM_RENESAS_RZG2L_GPT)	+= pwm-rzg2l-gpt.o
 obj-$(CONFIG_PWM_RENESAS_RZ_MTU3)	+= pwm-rz-mtu3.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
+obj-$(CONFIG_PWM_ROCKCHIP_V4)	+= pwm-rockchip-v4.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-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c
new file mode 100644
index 0000000000000000000000000000000000000000..9af71e79c2c7e006e805604fb66b4448ab5ecbc4
--- /dev/null
+++ b/drivers/pwm/pwm-rockchip-v4.c
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2025 Collabora Ltd.
+ *
+ * A Pulse-Width-Modulation (PWM) generator driver for the generators found in
+ * Rockchip SoCs such as the RK3576, internally referred to as "PWM v4". Uses
+ * the MFPWM infrastructure to guarantee exclusive use over the device without
+ * other functions of the device from different drivers interfering with its
+ * operation while it's active.
+ *
+ * Authors:
+ *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
+ *
+ * Limitations:
+ * - When the output is disabled, it will end abruptly without letting the
+ *   current period complete.
+ *   TODO: This can be fixed in the driver in the future by having the enable-
+ *         to-disable transition switch to oneshot mode with one repetition,
+ *         and then disable the pwmclk and release mfpwm when the oneshot
+ *         complete interrupt fires.
+ * - When the output is disabled, the pin will remain driven to whatever state
+ *   it last had.
+ * - Adjustments to the duty cycle will only take effect during the next period.
+ * - Adjustments to the period length will only take effect during the next
+ *   period.
+ */
+
+#include <linux/math64.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <soc/rockchip/mfpwm.h>
+
+struct rockchip_pwm_v4 {
+	struct rockchip_mfpwm_func *pwmf;
+	struct pwm_chip chip;
+};
+
+struct rockchip_pwm_v4_wf {
+	u32 period;
+	u32 duty;
+	u32 offset;
+	u8 enable;
+};
+
+static inline struct rockchip_pwm_v4 *to_rockchip_pwm_v4(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+/**
+ * rockchip_pwm_v4_round_single - convert a PWM parameter to hardware
+ * @rate: clock rate of the PWM clock, as per clk_get_rate
+ * @in_val: parameter in nanoseconds to convert
+ * @out_val: pointer to location where converted result should be stored.
+ *
+ * Return:
+ * * %0          - Success
+ * * %-EOVERFLOW - Result too large for target type
+ */
+static int rockchip_pwm_v4_round_single(unsigned long rate, u64 in_val,
+					u32 *out_val)
+{
+	u64 tmp;
+
+	tmp = mul_u64_u64_div_u64(rate, in_val, NSEC_PER_SEC);
+	if (tmp > U32_MAX)
+		return -EOVERFLOW;
+
+	*out_val = tmp;
+
+	return 0;
+}
+
+/**
+ * rockchip_pwm_v4_round_params - convert PWM parameters to hardware
+ * @rate: PWM clock rate to do the calculations at
+ * @duty: PWM duty cycle in nanoseconds
+ * @period: PWM period in nanoseconds
+ * @offset: PWM offset in nanoseconds
+ * @out_duty: pointer to where the rounded duty value should be stored
+ * @out_period: pointer to where the rounded period value should be stored
+ * @out_offset: pointer to where the rounded offset value should be stored
+ *
+ * Convert nanosecond-based duty/period/offset parameters to the PWM hardware's
+ * native rounded representation in number of cycles at clock rate @rate. Should
+ * any of the input parameters be out of range for the hardware, the
+ * corresponding output parameter is the maximum permissible value for said
+ * parameter with considerations to the others.
+ */
+static void rockchip_pwm_v4_round_params(unsigned long rate, u64 duty,
+					u64 period, u64 offset, u32 *out_duty,
+					u32 *out_period, u32 *out_offset)
+{
+	int ret;
+
+	ret = rockchip_pwm_v4_round_single(rate, period, out_period);
+	if (ret)
+		*out_period = U32_MAX;
+
+	ret = rockchip_pwm_v4_round_single(rate, duty, out_duty);
+	if (ret || *out_duty > *out_period)
+		*out_duty = *out_period;
+
+	ret = rockchip_pwm_v4_round_single(rate, offset, out_offset);
+	if (ret || *out_offset > (*out_period - *out_duty))
+		*out_offset = *out_period - *out_duty;
+}
+
+static int rockchip_pwm_v4_round_wf_tohw(struct pwm_chip *chip,
+					 struct pwm_device *pwm,
+					 const struct pwm_waveform *wf,
+					 void *_wfhw)
+{
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+	struct rockchip_pwm_v4_wf *wfhw = _wfhw;
+	unsigned long rate;
+	int ret;
+
+	/* We do not want chosen_clk to change out from under us here */
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	rate = clk_get_rate(pc->pwmf->core);
+
+	rockchip_pwm_v4_round_params(rate, wf->duty_length_ns,
+				     wf->period_length_ns, wf->duty_offset_ns,
+				     &wfhw->duty, &wfhw->period, &wfhw->offset);
+
+	if (wf->period_length_ns > 0)
+		wfhw->enable = PWMV4_EN_BOTH_MASK;
+	else
+		wfhw->enable = 0;
+
+	dev_dbg(&chip->dev, "tohw: duty = %u, period = %u, offset = %u, rate %lu\n",
+		wfhw->duty, wfhw->period, wfhw->offset, rate);
+
+	mfpwm_release(pc->pwmf);
+	return 0;
+}
+
+static int rockchip_pwm_v4_round_wf_fromhw(struct pwm_chip *chip,
+					   struct pwm_device *pwm,
+					   const void *_wfhw,
+					   struct pwm_waveform *wf)
+{
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+	const struct rockchip_pwm_v4_wf *wfhw = _wfhw;
+	unsigned long rate;
+	int ret = 0;
+
+	/* We do not want chosen_clk to change out from under us here */
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	rate = clk_get_rate(pc->pwmf->core);
+
+	if (rockchip_pwm_v4_is_enabled(wfhw->enable)) {
+		if (!rate) {
+			ret = -EINVAL;
+			goto out_mfpwm_release;
+		}
+		wf->period_length_ns = mul_u64_u64_div_u64(wfhw->period, NSEC_PER_SEC, rate);
+		wf->duty_length_ns = mul_u64_u64_div_u64(wfhw->duty, NSEC_PER_SEC, rate);
+		wf->duty_offset_ns = mul_u64_u64_div_u64(wfhw->offset, NSEC_PER_SEC, rate);
+	} else {
+		wf->period_length_ns = 0;
+		wf->duty_length_ns = 0;
+		wf->duty_offset_ns = 0;
+	}
+
+	dev_dbg(&chip->dev, "fromhw: duty = %llu, period = %llu, offset = %llu, rate = %lu\n",
+		wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, rate);
+
+out_mfpwm_release:
+	mfpwm_release(pc->pwmf);
+	return ret;
+}
+
+static int rockchip_pwm_v4_read_wf(struct pwm_chip *chip, struct pwm_device *pwm,
+				   void *_wfhw)
+{
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+	struct rockchip_pwm_v4_wf *wfhw = _wfhw;
+	int ret = 0;
+
+
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	wfhw->period = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_PERIOD);
+	wfhw->duty = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_DUTY);
+	wfhw->offset = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_OFFSET);
+	wfhw->enable = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_ENABLE) & PWMV4_EN_BOTH_MASK;
+
+	mfpwm_release(pc->pwmf);
+
+	return 0;
+}
+
+static int rockchip_pwm_v4_write_wf(struct pwm_chip *chip, struct pwm_device *pwm,
+				    const void *_wfhw)
+{
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+	const struct rockchip_pwm_v4_wf *wfhw = _wfhw;
+	bool was_enabled = false;
+	int ret = 0;
+
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	was_enabled = rockchip_pwm_v4_is_enabled(mfpwm_reg_read(pc->pwmf->base,
+								PWMV4_REG_ENABLE));
+
+	/*
+	 * "But Nicolas", you ask with valid concerns, "why would you enable the
+	 * PWM before setting all the parameter registers?"
+	 *
+	 * Excellent question, Mr. Reader M. Strawman! The RK3576 TRM Part 1
+	 * Section 34.6.3 specifies that this is the intended order of writes.
+	 * Doing the PWM_EN and PWM_CLK_EN writes after the params but before
+	 * the CTRL_UPDATE_EN, or even after the CTRL_UPDATE_EN, results in
+	 * erratic behaviour where repeated turning on and off of the PWM may
+	 * not turn it off under all circumstances. This is also why we don't
+	 * use relaxed writes; it's not worth the footgun.
+	 */
+	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
+			FIELD_PREP_HI16_WE(PWMV4_EN_BOTH_MASK, wfhw->enable));
+
+	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_PERIOD, wfhw->period);
+	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_DUTY, wfhw->duty);
+	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_OFFSET, wfhw->offset);
+
+	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, PWMV4_CTRL_CONT_FLAGS);
+
+	/* Commit new configuration to hardware output. */
+	mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
+			PWMV4_CTRL_UPDATE_EN);
+
+	if (rockchip_pwm_v4_is_enabled(wfhw->enable)) {
+		if (!was_enabled) {
+			dev_dbg(&chip->dev, "enabling PWM output\n");
+			ret = clk_enable(pc->pwmf->core);
+			if (ret)
+				goto err_mfpwm_release;
+			ret = clk_rate_exclusive_get(pc->pwmf->core);
+			if (ret) {
+				clk_disable(pc->pwmf->core);
+				goto err_mfpwm_release;
+			}
+
+			/*
+			 * Output should be on now, acquire device to guarantee
+			 * exclusion with other device functions while it's on.
+			 */
+			ret = mfpwm_acquire(pc->pwmf);
+			if (ret)
+				goto err_mfpwm_release;
+		}
+	} else if (was_enabled) {
+		dev_dbg(&chip->dev, "disabling PWM output\n");
+		clk_rate_exclusive_put(pc->pwmf->core);
+		clk_disable(pc->pwmf->core);
+		/* Output is off now, extra release to balance extra acquire */
+		mfpwm_release(pc->pwmf);
+	}
+
+err_mfpwm_release:
+	mfpwm_release(pc->pwmf);
+
+	return ret;
+}
+
+static const struct pwm_ops rockchip_pwm_v4_ops = {
+	.sizeof_wfhw = sizeof(struct rockchip_pwm_v4_wf),
+	.round_waveform_tohw = rockchip_pwm_v4_round_wf_tohw,
+	.round_waveform_fromhw = rockchip_pwm_v4_round_wf_fromhw,
+	.read_waveform = rockchip_pwm_v4_read_wf,
+	.write_waveform = rockchip_pwm_v4_write_wf,
+};
+
+static bool rockchip_pwm_v4_on_and_continuous(struct rockchip_pwm_v4 *pc)
+{
+	bool en;
+	u32 val;
+
+	en = rockchip_pwm_v4_is_enabled(mfpwm_reg_read(pc->pwmf->base,
+						       PWMV4_REG_ENABLE));
+	val = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_CTRL);
+
+	return en && ((val & PWMV4_MODE_MASK) == PWMV4_MODE_CONT);
+}
+
+static int rockchip_pwm_v4_probe(struct platform_device *pdev)
+{
+	struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev);
+	struct rockchip_pwm_v4 *pc;
+	struct pwm_chip *chip;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	chip = devm_pwmchip_alloc(dev, 1, sizeof(*pc));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	pc = to_rockchip_pwm_v4(chip);
+	pc->pwmf = pwmf;
+
+	ret = mfpwm_acquire(pwmf);
+	if (ret == -EBUSY)
+		dev_warn(dev, "pwm hardware already in use, can't check initial state\n");
+	else if (ret < 0)
+		return dev_err_probe(dev, ret, "couldn't acquire mfpwm in probe\n");
+
+	if (!rockchip_pwm_v4_on_and_continuous(pc))
+		mfpwm_release(pwmf);
+	else {
+		dev_dbg(dev, "pwm was already on at probe time\n");
+		ret = clk_enable(pwmf->core);
+		if (ret)
+			return dev_err_probe(dev, ret, "enabling pwm clock failed\n");
+		ret = clk_rate_exclusive_get(pc->pwmf->core);
+		if (ret) {
+			clk_disable(pwmf->core);
+			return dev_err_probe(dev, ret, "protecting pwm clock failed\n");
+		}
+	}
+
+	platform_set_drvdata(pdev, chip);
+
+	chip->ops = &rockchip_pwm_v4_ops;
+
+	ret = pwmchip_add(chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add PWM chip\n");
+
+	return 0;
+}
+
+static void rockchip_pwm_v4_remove(struct platform_device *pdev)
+{
+	struct pwm_chip *chip = platform_get_drvdata(pdev);
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+
+	mfpwm_remove_func(pc->pwmf);
+
+	pwmchip_remove(chip);
+}
+
+static const struct platform_device_id rockchip_pwm_v4_ids[] = {
+	{ .name = "pwm-rockchip-v4", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, rockchip_pwm_v4_ids);
+
+static struct platform_driver rockchip_pwm_v4_driver = {
+	.probe = rockchip_pwm_v4_probe,
+	.remove = rockchip_pwm_v4_remove,
+	.driver = {
+		.name = "pwm-rockchip-v4",
+	},
+	.id_table = rockchip_pwm_v4_ids,
+};
+module_platform_driver(rockchip_pwm_v4_driver);
+
+MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
+MODULE_DESCRIPTION("Rockchip PWMv4 Driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("ROCKCHIP_MFPWM");

-- 
2.49.0

[PATCH v2 6/7] counter: Add rockchip-pwm-capture driver

[Nicolas Frattaroli]

Among many other things, Rockchip's new PWMv4 IP in the RK3576 supports
PWM capture functionality.

Add a basic driver for this that works to capture period and duty cycle
values and return them as nanoseconds to the user. It's quite basic, but
works well enough to demonstrate the device function exclusion stuff
that mfpwm does, in order to eventually support all the functions of
this device in drivers within their appropriate subsystems, without them
interfering with each other.

Once enabled, the counter driver waits for enough high-to-low and
low-to-high interrupt signals to arrive, and then writes the cycle count
register values into some atomic members of the driver instance's state
struct. The read callback can then do the conversion from cycle count to
the more useful period and duty cycle nanosecond values, which require
knowledge of the clock rate, which requires a call that the interrupt
handler cannot make itself because said call may sleep.

To detect the condition of a PWM signal disappearing, i.e. turning off,
we modify the delay value of a delayed worker whose job it is to simply
set those atomic members to zero. Should the "timeout" so to speak be
reached, we assume the PWM signal is off. This isn't perfect; it
obviously introduces a latency between it being off and the counter
reporting it as such. Because there isn't a way to reset the internal
double-buffered cycle count in the hardware, we filter out unreliable
periods above the timeout value in the counter read callback.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 MAINTAINERS                            |   1 +
 drivers/counter/Kconfig                |  13 ++
 drivers/counter/Makefile               |   1 +
 drivers/counter/rockchip-pwm-capture.c | 352 +++++++++++++++++++++++++++++++++
 4 files changed, 367 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0014af1cbd3a7729a04bfdd0b0ed50e9df425693..9690f00053df17f8459aa2c6c8f0c62c6a25107e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21424,6 +21424,7 @@ L:	linux-rockchip@lists.infradead.org
 L:	linux-pwm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml
+F:	drivers/counter/rockchip-pwm-capture.c
 F:	drivers/pwm/pwm-rockchip-v4.c
 F:	drivers/soc/rockchip/mfpwm.c
 F:	include/soc/rockchip/mfpwm.h
diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index d30d22dfe57741b145a45632b6325d5f9680590e..01b4f5c326478c73b518041830ee0d65b37f6833 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -90,6 +90,19 @@ config MICROCHIP_TCB_CAPTURE
 	  To compile this driver as a module, choose M here: the
 	  module will be called microchip-tcb-capture.
 
+config ROCKCHIP_PWM_CAPTURE
+	tristate "Rockchip PWM Counter Capture driver"
+	depends on ARCH_ROCKCHIP || COMPILE_TEST
+	depends on ROCKCHIP_MFPWM
+	depends on HAS_IOMEM
+	help
+	  Generic counter framework driver for the multi-function PWM on
+	  Rockchip SoCs such as the RK3576.
+
+	  Uses the Rockchip Multi-function PWM controller driver infrastructure
+	  to guarantee exclusive operation with other functions of the same
+	  device implemented by drivers in other subsystems.
+
 config RZ_MTU3_CNT
 	tristate "Renesas RZ/G2L MTU3a counter driver"
 	depends on RZ_MTU3
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index fa3c1d08f7068835aa912aa13bc92bcfd44d16fb..2bfcfc2c584bd174a9885064746a98f15b204aec 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
 obj-$(CONFIG_MICROCHIP_TCB_CAPTURE)	+= microchip-tcb-capture.o
 obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
 obj-$(CONFIG_TI_ECAP_CAPTURE)	+= ti-ecap-capture.o
+obj-$(CONFIG_ROCKCHIP_PWM_CAPTURE)	+= rockchip-pwm-capture.o
diff --git a/drivers/counter/rockchip-pwm-capture.c b/drivers/counter/rockchip-pwm-capture.c
new file mode 100644
index 0000000000000000000000000000000000000000..03fe9cf32e7f273c0091c4743642eda6bee76222
--- /dev/null
+++ b/drivers/counter/rockchip-pwm-capture.c
@@ -0,0 +1,352 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2025 Collabora Ltd.
+ *
+ * A counter driver for the Pulse-Width-Modulation (PWM) hardware found on
+ * Rockchip SoCs such as the RK3576, internally referred to as "PWM v4". It
+ * allows for measuring the period and duty cycle in nanoseconds through the
+ * generic counter framework, while guaranteeing exclusive use over the MFPWM
+ * device while the counter is enabled.
+ *
+ * Authors:
+ *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
+ */
+
+#include <linux/cleanup.h>
+#include <linux/counter.h>
+#include <linux/devm-helpers.h>
+#include <linux/interrupt.h>
+#include <linux/math64.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+#include <soc/rockchip/mfpwm.h>
+
+#define RKPWMC_INT_MASK			(PWMV4_INT_LPC | PWMV4_INT_HPC)
+/*
+ * amount of jiffies between no PWM signal change and us deciding PWM is off.
+ * PWM signals with a period longer than this won't be detected by us, which is
+ * the trade-off we make to have a faster response time when a signal is turned
+ * off.
+ */
+#define RKPWMC_CLEAR_DELAY		(max(msecs_to_jiffies(100), 1))
+
+struct rockchip_pwm_capture {
+	struct rockchip_mfpwm_func *pwmf;
+	bool is_enabled;
+	spinlock_t enable_lock;
+	atomic_t lpc;
+	atomic_t hpc;
+	atomic_t captures_left;
+	struct delayed_work clear_capture;
+};
+
+static struct counter_signal rkpwmc_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 1"
+	},
+};
+
+static const enum counter_synapse_action rkpwmc_hpc_lpc_actions[] = {
+	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
+
+};
+
+static struct counter_synapse rkpwmc_pwm_synapses[] = {
+	{
+		.actions_list = rkpwmc_hpc_lpc_actions,
+		.num_actions = ARRAY_SIZE(rkpwmc_hpc_lpc_actions),
+		.signal = &rkpwmc_signals[0]
+	},
+};
+
+static const enum counter_function rkpwmc_functions[] = {
+	COUNTER_FUNCTION_INCREASE,
+};
+
+static int rkpwmc_enable_read(struct counter_device *counter,
+			       struct counter_count *count,
+			       u8 *enable)
+{
+	struct rockchip_pwm_capture *pc = counter_priv(counter);
+
+	guard(spinlock)(&pc->enable_lock);
+
+	*enable = pc->is_enabled;
+
+	return 0;
+}
+
+static int rkpwmc_enable_write(struct counter_device *counter,
+			       struct counter_count *count,
+			       u8 enable)
+{
+	struct rockchip_pwm_capture *pc = counter_priv(counter);
+	int ret;
+
+	guard(spinlock)(&pc->enable_lock);
+
+	if (!!enable != pc->is_enabled) {
+		ret = mfpwm_acquire(pc->pwmf);
+		if (ret)
+			return ret;
+
+		if (enable) {
+			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
+					 PWMV4_EN(false));
+			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL,
+					 PWMV4_CTRL_CAP_FLAGS);
+			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INT_EN,
+					 PWMV4_INT_LPC_W(true) |
+					 PWMV4_INT_HPC_W(true));
+			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
+					 PWMV4_EN(true) | PWMV4_CLK_EN(true));
+
+			ret = clk_enable(pc->pwmf->core);
+			if (ret)
+				goto err_release;
+
+			ret = clk_rate_exclusive_get(pc->pwmf->core);
+			if (ret)
+				goto err_disable_pwm_clk;
+
+			ret = mfpwm_acquire(pc->pwmf);
+			if (ret)
+				goto err_unprotect_pwm_clk;
+
+			atomic_set(&pc->captures_left, 4);
+			pc->is_enabled = true;
+		} else {
+			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INT_EN,
+					 PWMV4_INT_LPC_W(false) |
+					 PWMV4_INT_HPC_W(false));
+			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
+					 PWMV4_EN(false) | PWMV4_CLK_EN(false));
+			/*
+			 * Do not use cancel_delayed_work here unless you want
+			 * to cause the interrupt handler, which may still be
+			 * running at this point, to stall. Similarly, don't do
+			 * flush_delayed_work since it may sleep.
+			 */
+			mod_delayed_work(system_bh_wq, &pc->clear_capture, 0);
+			clk_rate_exclusive_put(pc->pwmf->core);
+			clk_disable(pc->pwmf->core);
+			pc->is_enabled = false;
+			mfpwm_release(pc->pwmf);
+		}
+
+		mfpwm_release(pc->pwmf);
+	}
+
+	return 0;
+
+err_unprotect_pwm_clk:
+	clk_rate_exclusive_put(pc->pwmf->core);
+err_disable_pwm_clk:
+	clk_disable(pc->pwmf->core);
+err_release:
+	mfpwm_release(pc->pwmf);
+
+	return ret;
+}
+
+static struct counter_comp rkpwmc_ext[] = {
+	COUNTER_COMP_ENABLE(rkpwmc_enable_read, rkpwmc_enable_write),
+};
+
+enum rkpwmc_count_id {
+	COUNT_PERIOD = 0,
+	COUNT_DUTY = 1,
+};
+
+static struct counter_count rkpwmc_counts[] = {
+	{
+		.id = COUNT_PERIOD,
+		.name = "Period in nanoseconds",
+		.functions_list = rkpwmc_functions,
+		.num_functions = ARRAY_SIZE(rkpwmc_functions),
+		.synapses = rkpwmc_pwm_synapses,
+		.num_synapses = ARRAY_SIZE(rkpwmc_pwm_synapses),
+		.ext = rkpwmc_ext,
+		.num_ext = ARRAY_SIZE(rkpwmc_ext),
+	},
+	{
+		.id = COUNT_DUTY,
+		.name = "Duty cycle in nanoseconds",
+		.functions_list = rkpwmc_functions,
+		.num_functions = ARRAY_SIZE(rkpwmc_functions),
+		.synapses = rkpwmc_pwm_synapses,
+		.num_synapses = ARRAY_SIZE(rkpwmc_pwm_synapses),
+		.ext = rkpwmc_ext,
+		.num_ext = ARRAY_SIZE(rkpwmc_ext),
+	},
+};
+
+static int rkpwmc_count_read(struct counter_device *counter,
+			     struct counter_count *count, u64 *value)
+{
+	struct rockchip_pwm_capture *pc = counter_priv(counter);
+	unsigned long rate;
+	u64 period;
+	u64 lpc;
+	u64 hpc;
+	int ret = 0;
+
+	if (count->id != COUNT_PERIOD && count->id != COUNT_DUTY)
+		return -EINVAL;
+
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	rate = clk_get_rate(pc->pwmf->core);
+	if (!rate) {
+		ret = -EINVAL;
+		goto out_release;
+	}
+
+	hpc = (u32) atomic_read(&pc->hpc);
+	lpc = (u32) atomic_read(&pc->lpc);
+	period = mul_u64_u64_div_u64(hpc + lpc, NSEC_PER_SEC, rate);
+
+	if (period > jiffies_to_msecs(RKPWMC_CLEAR_DELAY) * NSEC_PER_MSEC) {
+		*value = 0;
+		goto out_release;
+	}
+
+	if (count->id == COUNT_PERIOD)
+		*value = period;
+	else
+		*value = mul_u64_u64_div_u64(hpc, NSEC_PER_SEC, rate);
+
+out_release:
+	mfpwm_release(pc->pwmf);
+
+	return ret;
+}
+
+static const struct counter_ops rkpwmc_ops = {
+	.count_read = rkpwmc_count_read,
+};
+
+static irqreturn_t rkpwmc_irq_handler(int irq, void *data)
+{
+	struct rockchip_pwm_capture *pc = data;
+	u32 intsts;
+	u32 clr = 0;
+	u32 lpc;
+	u32 hpc;
+
+	intsts = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_INTSTS);
+
+	if (!(intsts & RKPWMC_INT_MASK))
+		return IRQ_NONE;
+
+	if (intsts & PWMV4_INT_LPC) {
+		clr |= PWMV4_INT_LPC;
+		atomic_dec_if_positive(&pc->captures_left);
+	}
+
+	if (intsts & PWMV4_INT_HPC) {
+		clr |= PWMV4_INT_HPC;
+		atomic_dec_if_positive(&pc->captures_left);
+	}
+
+	/* After 4 interrupts, reset to 4 captures left and read the regs */
+	if (!atomic_cmpxchg(&pc->captures_left, 0, 4)) {
+		lpc = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_LPC);
+		hpc = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_HPC);
+
+		atomic_set(&pc->lpc, lpc);
+		atomic_set(&pc->hpc, hpc);
+		mod_delayed_work(system_bh_wq, &pc->clear_capture,
+				 RKPWMC_CLEAR_DELAY);
+	}
+
+	if (clr)
+		mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INTSTS, clr);
+
+	if (intsts ^ clr)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static void rkpwmc_clear_capture_worker(struct work_struct *work)
+{
+	struct rockchip_pwm_capture *pc = container_of(
+		work, struct rockchip_pwm_capture, clear_capture.work);
+
+	atomic_set(&pc->hpc, 0);
+	atomic_set(&pc->lpc, 0);
+}
+
+static int rockchip_pwm_capture_probe(struct platform_device *pdev)
+{
+	struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev);
+	struct rockchip_pwm_capture *pc;
+	struct counter_device *counter;
+	int ret;
+
+	counter = devm_counter_alloc(&pdev->dev, sizeof(*pc));
+	if (IS_ERR(counter))
+		return PTR_ERR(counter);
+
+	pc = counter_priv(counter);
+	pc->pwmf = pwmf;
+	spin_lock_init(&pc->enable_lock);
+
+	platform_set_drvdata(pdev, pc);
+
+	ret = devm_request_irq(&pdev->dev, pwmf->irq, rkpwmc_irq_handler,
+			       IRQF_SHARED, pdev->name, pc);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed requesting IRQ\n");
+
+	ret = devm_delayed_work_autocancel(&pdev->dev, &pc->clear_capture,
+					   rkpwmc_clear_capture_worker);
+
+	counter->name = pdev->name;
+	counter->signals = rkpwmc_signals;
+	counter->num_signals = ARRAY_SIZE(rkpwmc_signals);
+	counter->ops = &rkpwmc_ops;
+	counter->counts = rkpwmc_counts;
+	counter->num_counts = ARRAY_SIZE(rkpwmc_counts);
+
+	ret = devm_counter_add(&pdev->dev, counter);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "Failed to add counter\n");
+
+	return 0;
+}
+
+static void rockchip_pwm_capture_remove(struct platform_device *pdev)
+{
+	struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev);
+
+	mfpwm_remove_func(pwmf);
+}
+
+static const struct platform_device_id rockchip_pwm_capture_id_table[] = {
+	{ .name = "rockchip-pwm-capture", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(platform, rockchip_pwm_capture_id_table);
+
+static struct platform_driver rockchip_pwm_capture_driver = {
+	.probe = rockchip_pwm_capture_probe,
+	.remove = rockchip_pwm_capture_remove,
+	.id_table = rockchip_pwm_capture_id_table,
+	.driver = {
+		.name = "rockchip-pwm-capture",
+	},
+};
+module_platform_driver(rockchip_pwm_capture_driver);
+
+MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
+MODULE_DESCRIPTION("Rockchip PWM Counter Capture Driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("ROCKCHIP_MFPWM");
+MODULE_IMPORT_NS("COUNTER");

-- 
2.49.0

[PATCH v2 7/7] arm64: dts: rockchip: add PWM nodes to RK3576 SoC dtsi

[Nicolas Frattaroli]

The RK3576 SoC features three distinct PWM controllers, with variable
numbers of channels. Add each channel as a separate node to the SoC's
device tree, as they don't really overlap in register ranges.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3576.dtsi | 208 +++++++++++++++++++++++++++++++
 1 file changed, 208 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3576.dtsi b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
index 1086482f04792325dc4c22fb8ceeb27eef59afe4..9e7a41d721d29842dc9bde39170b8127584b0b2c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3576.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
@@ -892,6 +892,32 @@ uart1: serial@27310000 {
 			status = "disabled";
 		};
 
+		pwm0_2ch_0: pwm@27330000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x27330000 0x0 0x1000>;
+			clocks = <&cru CLK_PMU1PWM>, <&cru PCLK_PMU1PWM>,
+				 <&cru CLK_PMU1PWM_OSC>, <&cru CLK_PMU1PWM_RC>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm0m0_ch0>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm0_2ch_1: pwm@27331000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x27331000 0x0 0x1000>;
+			clocks = <&cru CLK_PMU1PWM>, <&cru PCLK_PMU1PWM>,
+				 <&cru CLK_PMU1PWM_OSC>, <&cru CLK_PMU1PWM_RC>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm0m0_ch1>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		pmu: power-management@27380000 {
 			compatible = "rockchip,rk3576-pmu", "syscon", "simple-mfd";
 			reg = <0x0 0x27380000 0x0 0x800>;
@@ -2273,6 +2299,188 @@ uart9: serial@2adc0000 {
 			status = "disabled";
 		};
 
+		pwm1_6ch_0: pwm@2add0000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2add0000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>,
+				 <&cru CLK_OSC_PWM1>, <&cru CLK_RC_PWM1>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm1m0_ch0>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm1_6ch_1: pwm@2add1000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2add1000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>,
+				 <&cru CLK_OSC_PWM1>, <&cru CLK_RC_PWM1>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm1m0_ch1>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm1_6ch_2: pwm@2add2000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2add2000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>,
+				 <&cru CLK_OSC_PWM1>, <&cru CLK_RC_PWM1>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm1m0_ch2>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm1_6ch_3: pwm@2add3000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2add3000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>,
+				 <&cru CLK_OSC_PWM1>, <&cru CLK_RC_PWM1>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm1m0_ch3>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm1_6ch_4: pwm@2add4000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2add4000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>,
+				 <&cru CLK_OSC_PWM1>, <&cru CLK_RC_PWM1>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm1m0_ch4>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm1_6ch_5: pwm@2add5000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2add5000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>,
+				 <&cru CLK_OSC_PWM1>, <&cru CLK_RC_PWM1>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm1m0_ch5>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm2_8ch_0: pwm@2ade0000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2ade0000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM2>, <&cru PCLK_PWM2>,
+				 <&cru CLK_OSC_PWM2>, <&cru CLK_RC_PWM2>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm2m0_ch0>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm2_8ch_1: pwm@2ade1000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2ade1000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM2>, <&cru PCLK_PWM2>,
+				 <&cru CLK_OSC_PWM2>, <&cru CLK_RC_PWM2>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm2m0_ch1>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm2_8ch_2: pwm@2ade2000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2ade2000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM2>, <&cru PCLK_PWM2>,
+				 <&cru CLK_OSC_PWM2>, <&cru CLK_RC_PWM2>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm2m0_ch2>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm2_8ch_3: pwm@2ade3000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2ade3000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM2>, <&cru PCLK_PWM2>,
+				 <&cru CLK_OSC_PWM2>, <&cru CLK_RC_PWM2>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm2m0_ch3>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm2_8ch_4: pwm@2ade4000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2ade4000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM2>, <&cru PCLK_PWM2>,
+				 <&cru CLK_OSC_PWM2>, <&cru CLK_RC_PWM2>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm2m0_ch4>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm2_8ch_5: pwm@2ade5000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2ade5000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM2>, <&cru PCLK_PWM2>,
+				 <&cru CLK_OSC_PWM2>, <&cru CLK_RC_PWM2>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm2m0_ch5>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm2_8ch_6: pwm@2ade6000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2ade6000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM2>, <&cru PCLK_PWM2>,
+				 <&cru CLK_OSC_PWM2>, <&cru CLK_RC_PWM2>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm2m0_ch6>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm2_8ch_7: pwm@2ade7000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x2ade7000 0x0 0x1000>;
+			clocks = <&cru CLK_PWM2>, <&cru PCLK_PWM2>,
+				 <&cru CLK_OSC_PWM2>, <&cru CLK_RC_PWM2>;
+			clock-names = "pwm", "pclk", "osc", "rc";
+			interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm2m0_ch7>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		saradc: adc@2ae00000 {
 			compatible = "rockchip,rk3576-saradc", "rockchip,rk3588-saradc";
 			reg = <0x0 0x2ae00000 0x0 0x10000>;

-- 
2.49.0

Re: [PATCH v2 3/7] bitfield: introduce HI16_WE bitfield prep macros

[Heiko Stübner]

Am Montag, 2. Juni 2025, 18:19:14 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> Hardware of various vendors, but very notably Rockchip, often uses
> 32-bit registers where the upper 16-bit half of the register is a
> write-enable mask for the lower half.
> 
> This type of hardware setup allows for more granular concurrent register
> write access.
> 
> Over the years, many drivers have hand-rolled their own version of this
> macro, usually without any checks, often called something like
> HIWORD_UPDATE or FIELD_PREP_HIWORD, commonly with slightly different
> semantics between them.
> 
> Clearly there is a demand for such a macro, and thus the demand should
> be satisfied in a common header file.
> 
> Add two macros: FIELD_PREP_HI16_WE, and FIELD_PREP_HI16_WE_CONST. The
> latter is a version that can be used in initializers, like
> FIELD_PREP_CONST. The macro names are chosen to explicitly reference the
> assumed half-register width, and its function, while not clashing with
> any potential other macros that drivers may already have implemented
> themselves.
> 
> Future drivers should use these macros instead of handrolling their own,
> and old drivers can be ported to the new macros as time and opportunity
> allows.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  include/linux/bitfield.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 6d9a53db54b66c0833973c880444bd289d9667b1..2b3e7cb90ccb5d48f510104f61443b06748bb7eb 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -8,6 +8,7 @@
>  #define _LINUX_BITFIELD_H
>  
>  #include <linux/build_bug.h>
> +#include <linux/limits.h>
>  #include <linux/typecheck.h>
>  #include <asm/byteorder.h>
>  
> @@ -142,6 +143,52 @@
>  		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
>  	)
>  
> +/**
> + * FIELD_PREP_HI16_WE() - prepare a bitfield element with a write-enable mask
> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * FIELD_PREP_HI16_WE() masks and shifts up the value, as well as bitwise ORs
> + * the result with the mask shifted up by 16.
> + *
> + * This is useful for a common design of hardware registers where the upper
> + * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
> + * register, a bit in the lower half is only updated if the corresponding bit
> + * in the upper half is high.
> + */
> +#define FIELD_PREP_HI16_WE(_mask, _val)					\
> +	({								\
> +		__BF_FIELD_CHECK(_mask, ((u16) 0U), _val,		\
> +				 "FIELD_PREP_HI16_WE: ");		\
> +		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |	\

gcc is quite adamant about suggesting more parentheses here:

../include/linux/bitfield.h:163:70: warning: suggest parentheses around arithmetic in operand of ‘|’ [-Wparentheses]
  163 |                 ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |  \
      |                                                                      ^
../include/soc/rockchip/mfpwm.h:225:41: note: in expansion of macro ‘FIELD_PREP_HI16_WE’
  225 | #define PWMV4_MODE(v)                   FIELD_PREP_HI16_WE(PWMV4_MODE_MASK, (v))
      |                                         ^~~~~~~~~~~~~~~~~~
../include/soc/rockchip/mfpwm.h:230:34: note: in expansion of macro ‘PWMV4_MODE’
  230 | #define PWMV4_CTRL_CONT_FLAGS   (PWMV4_MODE(PWMV4_MODE_CONT) | \
      |                                  ^~~~~~~~~~
../drivers/pwm/pwm-rockchip-v4.c:237:57: note: in expansion of macro ‘PWMV4_CTRL_CONT_FLAGS’
  237 |         mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, PWMV4_CTRL_CONT_FLAGS);
      |                                                         ^~~~~~~~~~~~~~~~~~~~~


Heiko

Re: [PATCH v2 3/7] bitfield: introduce HI16_WE bitfield prep macros

[Yury Norov]

On Mon, Jun 02, 2025 at 06:19:14PM +0200, Nicolas Frattaroli wrote:
> Hardware of various vendors, but very notably Rockchip, often uses
> 32-bit registers where the upper 16-bit half of the register is a
> write-enable mask for the lower half.

Can you list them all explicitly please? I grepped myself for the
'HIGHWORD_UPDATE' and 'FIELD_PREP_HIGWORD', and found just 4 or 5 in
addition to the rockchip.
 
> This type of hardware setup allows for more granular concurrent register
> write access.
> 
> Over the years, many drivers have hand-rolled their own version of this
> macro, usually without any checks, often called something like
> HIWORD_UPDATE or FIELD_PREP_HIWORD, commonly with slightly different
> semantics between them.
> 
> Clearly there is a demand for such a macro, and thus the demand should
> be satisfied in a common header file.

I agree. Nice catch.

> Add two macros: FIELD_PREP_HI16_WE, and FIELD_PREP_HI16_WE_CONST. The
> latter is a version that can be used in initializers, like
> FIELD_PREP_CONST.

I'm not sure that the name you've chosen reflects the intention. If
you just give me the name without any background, I'd bet it updates
the HI16 part of presumably 32-bit field. The 'WE' part here is most
likely excessive because at this level of abstraction you can't
guarantee that 'write-enable mask' is the only purpose for the macro.

> The macro names are chosen to explicitly reference the
> assumed half-register width, and its function, while not clashing with
> any potential other macros that drivers may already have implemented
> themselves.
>
> Future drivers should use these macros instead of handrolling their own,
> and old drivers can be ported to the new macros as time and opportunity
> allows.

This is a wrong way to go. Once you introduce a macro that replaces
functionality of few other arch or driver macros, you should consolidate
them all in the same series. Otherwise, it will be just another flavor
of the same, but now living in a core header. 

Can you please prepare a series that introduces the new macro and
wires all arch duplications to it?
 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  include/linux/bitfield.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 6d9a53db54b66c0833973c880444bd289d9667b1..2b3e7cb90ccb5d48f510104f61443b06748bb7eb 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -8,6 +8,7 @@
>  #define _LINUX_BITFIELD_H
>  
>  #include <linux/build_bug.h>
> +#include <linux/limits.h>
>  #include <linux/typecheck.h>
>  #include <asm/byteorder.h>
>  
> @@ -142,6 +143,52 @@
>  		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
>  	)
>  
> +/**
> + * FIELD_PREP_HI16_WE() - prepare a bitfield element with a write-enable mask
> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * FIELD_PREP_HI16_WE() masks and shifts up the value, as well as bitwise ORs
> + * the result with the mask shifted up by 16.
> + *
> + * This is useful for a common design of hardware registers where the upper
> + * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
> + * register, a bit in the lower half is only updated if the corresponding bit
> + * in the upper half is high.
> + */
> +#define FIELD_PREP_HI16_WE(_mask, _val)					\
> +	({								\
> +		__BF_FIELD_CHECK(_mask, ((u16) 0U), _val,		\
> +				 "FIELD_PREP_HI16_WE: ");		\
> +		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |	\
> +		((_mask) << 16);					\
> +	})

This pretty much is a duplication of the FIELD_PREP(), isn't? Why don't
you borrow the approach from drivers/clk/clk-sp7021.c:

	/* HIWORD_MASK FIELD_PREP */
	#define HWM_FIELD_PREP(mask, value)             \
	({                                              \
	        u64 _m = mask;                          \
	        (_m << 16) | FIELD_PREP(_m, value);     \
	})

If you do so, the existing FIELD_PREP() will do all the work without
copy-pasting. The only questionI have  to the above macro is why '_m'
is u64? Seemingly, it should be u32?

Regarding the name... I can't invent a good one as well, so the best
thing I can suggest is not to invent something that can mislead. The
HWM_FIELD_PREP() is not bad because it tells almost nothing and
encourages one to refer to the documentation. If you want something
self-explaining, maybe MASK_HI_FIELD_LO_PREP_U16(), or something? 

Thanks,
Yury

> +
> +/**
> + * FIELD_PREP_HI16_WE_CONST() - prepare a constant bitfield element with a
> + *                              write-enable mask
> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * FIELD_PREP_HI16_WE_CONST() masks and shifts up the value, as well as bitwise
> + * ORs the result with the mask shifted up by 16.
> + *
> + * This is useful for a common design of hardware registers where the upper
> + * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
> + * register, a bit in the lower half is only updated if the corresponding bit
> + * in the upper half is high.
> + *
> + * Unlike FIELD_PREP_HI16_WE(), this is a constant expression and can therefore
> + * be used in initializers. Error checking is less comfortable for this
> + * version, and non-constant masks cannot be used.
> + */
> +#define FIELD_PREP_HI16_WE_CONST(_mask, _val)				 \
> +	(								 \
> +		FIELD_PREP_CONST(_mask, _val) |				 \
> +		(BUILD_BUG_ON_ZERO(const_true((u64) (_mask) > U16_MAX)) + \
> +		 ((_mask) << 16))					 \
> +	)
> +
>  /**
>   * FIELD_GET() - extract a bitfield element
>   * @_mask: shifted mask defining the field's length and position
> 
> -- 
> 2.49.0

Re: [PATCH v2 3/7] bitfield: introduce HI16_WE bitfield prep macros

[Nicolas Frattaroli]

On Monday, 2 June 2025 22:02:19 Central European Summer Time Yury Norov wrote:
> On Mon, Jun 02, 2025 at 06:19:14PM +0200, Nicolas Frattaroli wrote:
> > Hardware of various vendors, but very notably Rockchip, often uses
> > 32-bit registers where the upper 16-bit half of the register is a
> > write-enable mask for the lower half.
> 
> Can you list them all explicitly please? I grepped myself for the
> 'HIGHWORD_UPDATE' and 'FIELD_PREP_HIGWORD', and found just 4 or 5 in
> addition to the rockchip.

Most of the ones Heiko brought up[1] just appear to be the clock stuff,
I'm only aware of the drivers/mmc/host/sdhci-of-arasan.c one outside of
Rockchip. For a complete listing I'd have to do a semantic search with
e.g. Coccinelle, which I've never used before and would need to wrap
my head around first. grep is a bad fit for catching them all as some
macros are split across lines, or reverse the operators of the OR.
Weggli[2] is another possibility but it's abandoned and undocumented, and
I've ran into its limitations before fairly quickly.

>  
> > This type of hardware setup allows for more granular concurrent register
> > write access.
> > 
> > Over the years, many drivers have hand-rolled their own version of this
> > macro, usually without any checks, often called something like
> > HIWORD_UPDATE or FIELD_PREP_HIWORD, commonly with slightly different
> > semantics between them.
> > 
> > Clearly there is a demand for such a macro, and thus the demand should
> > be satisfied in a common header file.
> 
> I agree. Nice catch.
> 
> > Add two macros: FIELD_PREP_HI16_WE, and FIELD_PREP_HI16_WE_CONST. The
> > latter is a version that can be used in initializers, like
> > FIELD_PREP_CONST.
> 
> I'm not sure that the name you've chosen reflects the intention. If
> you just give me the name without any background, I'd bet it updates
> the HI16 part of presumably 32-bit field. The 'WE' part here is most
> likely excessive because at this level of abstraction you can't
> guarantee that 'write-enable mask' is the only purpose for the macro.
> 
> > The macro names are chosen to explicitly reference the
> > assumed half-register width, and its function, while not clashing with
> > any potential other macros that drivers may already have implemented
> > themselves.
> >
> > Future drivers should use these macros instead of handrolling their own,
> > and old drivers can be ported to the new macros as time and opportunity
> > allows.
> 
> This is a wrong way to go. Once you introduce a macro that replaces
> functionality of few other arch or driver macros, you should consolidate
> them all in the same series. Otherwise, it will be just another flavor
> of the same, but now living in a core header. 
> 
> Can you please prepare a series that introduces the new macro and
> wires all arch duplications to it?

Okay, I will do that after I learn Coccinelle. Though I suspect the reason
why I'm the first person to address this is because it's much easier to
hide duplicated macros away in drivers than go the long route of fixing up
every single other user. I'm not too miffed about it though, it's cleanup
of technical debt that's long overdue.

>  
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  include/linux/bitfield.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index 6d9a53db54b66c0833973c880444bd289d9667b1..2b3e7cb90ccb5d48f510104f61443b06748bb7eb 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -8,6 +8,7 @@
> >  #define _LINUX_BITFIELD_H
> >  
> >  #include <linux/build_bug.h>
> > +#include <linux/limits.h>
> >  #include <linux/typecheck.h>
> >  #include <asm/byteorder.h>
> >  
> > @@ -142,6 +143,52 @@
> >  		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
> >  	)
> >  
> > +/**
> > + * FIELD_PREP_HI16_WE() - prepare a bitfield element with a write-enable mask
> > + * @_mask: shifted mask defining the field's length and position
> > + * @_val:  value to put in the field
> > + *
> > + * FIELD_PREP_HI16_WE() masks and shifts up the value, as well as bitwise ORs
> > + * the result with the mask shifted up by 16.
> > + *
> > + * This is useful for a common design of hardware registers where the upper
> > + * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
> > + * register, a bit in the lower half is only updated if the corresponding bit
> > + * in the upper half is high.
> > + */
> > +#define FIELD_PREP_HI16_WE(_mask, _val)					\
> > +	({								\
> > +		__BF_FIELD_CHECK(_mask, ((u16) 0U), _val,		\
> > +				 "FIELD_PREP_HI16_WE: ");		\
> > +		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |	\
> > +		((_mask) << 16);					\
> > +	})
> 
> This pretty much is a duplication of the FIELD_PREP(), isn't? Why don't
> you borrow the approach from drivers/clk/clk-sp7021.c:
> 
> 	/* HIWORD_MASK FIELD_PREP */
> 	#define HWM_FIELD_PREP(mask, value)             \
> 	({                                              \
> 	        u64 _m = mask;                          \
> 	        (_m << 16) | FIELD_PREP(_m, value);     \
> 	})
> 
> If you do so, the existing FIELD_PREP() will do all the work without
> copy-pasting.

Because then the __BF_FIELD_CHECK macro will be invoked twice, once without
the proper prefix. Factoring the actual prep-no-check operation out into a
separate macro is macro definition + 1-line invocation * 2, whereas copy-
pasting the implementation that will never change is 1-line invocation*2.

> The only questionI have  to the above macro is why '_m'
> is u64? Seemingly, it should be u32?

I didn't write the HWM_FIELD_PREP macro in clk-sp7021.c, nor am I familiar
with the hardware. It's possible they were trying to prevent an overflow
wraparound here though, but they're not checking if the result ends up
greater than 32 bits so that seems suspect.

> Regarding the name... I can't invent a good one as well, so the best
> thing I can suggest is not to invent something that can mislead. The
> HWM_FIELD_PREP() is not bad because it tells almost nothing and
> encourages one to refer to the documentation. If you want something
> self-explaining, maybe MASK_HI_FIELD_LO_PREP_U16(), or something?

This seems a bit unwieldy, at 25 characters. "FIELD32_HIMASK_LOPREP"
(or FIELD16, depending on which end of the cornet to eat) would be 21
characters but I'm also not in love with it.

I think the name should include the following parts:
1. it's a field
2. the field is halved into two halves of 16 bits
3. the mask is copied into the upper 16 bits

Since we're on the subject of bit widths, I have a somewhat sacrilegious
point to raise: should this be a function-like macro at all, as opposed
to a static __pure inline function? It's not generic with regards to the
data types, as we're always assuming a u16 value and mask input and a
u32 output. The __pure inline definition should let the compiler treat it
essentially similar to what the pre-processor expanded macro does, which
is as not a function call at all but a bunch of code to constant fold away
if possible. What we get in return is type checking and less awful syntax.
Then we could call it something like `himask_field_prep_u32`, which is
also 21 characters but the ambiguity of whether the u32 refers to the mask
or the whole register width is cleared up by the types of the function
arguments.

The const version of the macro may still need to remain though because I'm
not sure C11 can do that for us. With C23 maybe there's a way with
constexpr but I've never used it before.

> 
> Thanks,
> Yury
> 

Kind Regards,
Nicolas Frattaroli

Link: https://lore.kernel.org/linux-rockchip/1895349.atdPhlSkOF@diego/ [1]
Link: https://github.com/weggli-rs/weggli [2]

> > +
> > +/**
> > + * FIELD_PREP_HI16_WE_CONST() - prepare a constant bitfield element with a
> > + *                              write-enable mask
> > + * @_mask: shifted mask defining the field's length and position
> > + * @_val:  value to put in the field
> > + *
> > + * FIELD_PREP_HI16_WE_CONST() masks and shifts up the value, as well as bitwise
> > + * ORs the result with the mask shifted up by 16.
> > + *
> > + * This is useful for a common design of hardware registers where the upper
> > + * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
> > + * register, a bit in the lower half is only updated if the corresponding bit
> > + * in the upper half is high.
> > + *
> > + * Unlike FIELD_PREP_HI16_WE(), this is a constant expression and can therefore
> > + * be used in initializers. Error checking is less comfortable for this
> > + * version, and non-constant masks cannot be used.
> > + */
> > +#define FIELD_PREP_HI16_WE_CONST(_mask, _val)				 \
> > +	(								 \
> > +		FIELD_PREP_CONST(_mask, _val) |				 \
> > +		(BUILD_BUG_ON_ZERO(const_true((u64) (_mask) > U16_MAX)) + \
> > +		 ((_mask) << 16))					 \
> > +	)
> > +
> >  /**
> >   * FIELD_GET() - extract a bitfield element
> >   * @_mask: shifted mask defining the field's length and position
> > 
> 

Re: [PATCH v2 3/7] bitfield: introduce HI16_WE bitfield prep macros

[Yury Norov]

On Tue, Jun 03, 2025 at 02:55:40PM +0200, Nicolas Frattaroli wrote:
> On Monday, 2 June 2025 22:02:19 Central European Summer Time Yury Norov wrote:
> > On Mon, Jun 02, 2025 at 06:19:14PM +0200, Nicolas Frattaroli wrote:
> > > Hardware of various vendors, but very notably Rockchip, often uses
> > > 32-bit registers where the upper 16-bit half of the register is a
> > > write-enable mask for the lower half.
> > 
> > Can you list them all explicitly please? I grepped myself for the
> > 'HIGHWORD_UPDATE' and 'FIELD_PREP_HIGWORD', and found just 4 or 5 in
> > addition to the rockchip.
> 
> Most of the ones Heiko brought up[1] just appear to be the clock stuff,
> I'm only aware of the drivers/mmc/host/sdhci-of-arasan.c one outside of
> Rockchip. For a complete listing I'd have to do a semantic search with
> e.g. Coccinelle, which I've never used before and would need to wrap
> my head around first. grep is a bad fit for catching them all as some
> macros are split across lines, or reverse the operators of the OR.
> Weggli[2] is another possibility but it's abandoned and undocumented, and
> I've ran into its limitations before fairly quickly.

Going Coccinelle way is fine, but I think it's an endless route. :)

What I meant is: you caught this HIWORD_UPDATE() duplication, and it's
great. When people copy-paste a macro implementation and even a name,
their intention is clear: they need this functionality, but the core
headers lack it, so: I'll make another small copy in my small driver,
and nobody cares.

I think your consolidation should at first target the above users.

Those having different names or substantially different implementation,
may also be a target. But they are:
 1. Obviously a minority in terms of LOCs, and
 2. More likely have their reasons to have custom flavors of the same.

...

> > Can you please prepare a series that introduces the new macro and
> > wires all arch duplications to it?
> 
> Okay, I will do that after I learn Coccinelle. Though I suspect the reason
> why I'm the first person to address this is because it's much easier to
> hide duplicated macros away in drivers than go the long route of fixing up
> every single other user. I'm not too miffed about it though, it's cleanup
> of technical debt that's long overdue.
 
I just fired 

        $ git grep "define HIWORD"

and found 27 matches. The relevant 'hiwords' have the following
semantics:

 - HIWORD_UPDATE(val, mask, shift)
 - HIWORD_UPDATE(val, mask)
 - HIWORD_UPDATE(mask, val)
 - HIWORD_UPDATE(v, h, l)
 - HIWORD_UPDATE_BIT(val)
 - HIWORD_DISABLE_BIT(val)

Most of them don't bother doing any static checks at all.

If you will just consolidate the above, and wire those drivers
to generic version with all that checks - it would be a decent
consolidation by any measure.

Something like this:

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 0470d7c175f4..d5e74d555a3d 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -30,7 +30,7 @@

 #define DMC_MAX_CHANNELS       4

-#define HIWORD_UPDATE(val, mask)       ((val) | (mask) << 16)
+#define HIWORD_UPDATE(val, mask)       HWORD_UPDATE(mask, val)

 /* DDRMON_CTRL */
 #define DDRMON_CTRL    0x04

...

> > Regarding the name... I can't invent a good one as well, so the best
> > thing I can suggest is not to invent something that can mislead. The
> > HWM_FIELD_PREP() is not bad because it tells almost nothing and
> > encourages one to refer to the documentation. If you want something
> > self-explaining, maybe MASK_HI_FIELD_LO_PREP_U16(), or something?
> 
> This seems a bit unwieldy, at 25 characters. "FIELD32_HIMASK_LOPREP"
> (or FIELD16, depending on which end of the cornet to eat) would be 21
> characters but I'm also not in love with it.
> 
> I think the name should include the following parts:
> 1. it's a field
> 2. the field is halved into two halves of 16 bits
> 3. the mask is copied into the upper 16 bits

Or just keep the HIWORD_UPDATE name as it already has over 300 users.
If it's commented well, and has an implementation based on FIELD_PREP,
I don't think users will struggle to understand what is actually
happening there.
 
> Since we're on the subject of bit widths, I have a somewhat sacrilegious
> point to raise: should this be a function-like macro at all, as opposed
> to a static __pure inline function? It's not generic with regards to the
> data types, as we're always assuming a u16 value and mask input and a
> u32 output. The __pure inline definition should let the compiler treat it
> essentially similar to what the pre-processor expanded macro does, which
> is as not a function call at all but a bunch of code to constant fold away
> if possible. What we get in return is type checking and less awful syntax.
> Then we could call it something like `himask_field_prep_u32`, which is
> also 21 characters but the ambiguity of whether the u32 refers to the mask
> or the whole register width is cleared up by the types of the function
> arguments.
> 
> The const version of the macro may still need to remain though because I'm
> not sure C11 can do that for us. With C23 maybe there's a way with
> constexpr but I've never used it before.

Inline function will disable parameters compile checks, and will be
too diverged from _CONST counterpart.

Thanks,
Yury

Re: [PATCH v2 1/7] dt-bindings: pinctrl: rockchip: increase max amount of device functions

[Linus Walleij]

On Mon, Jun 2, 2025 at 6:20 PM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:

> With the introduction of the RK3576, the maximum device function ID used
> increased to 14, as anyone can easily verify for themselves with:
>
>   rg -g '*-pinctrl.dtsi' '<\d+\s+RK_P..\s+(?<func>\d+)\s.*>;$' --trim \
>   -NI -r '$func' arch/arm64/boot/dts/rockchip/ | sort -g | uniq
>
> Unfortunately, this wasn't caught by dt-validate as those pins are
> omit-if-no-ref and we had no reference to them in any tree so far.
>
> Once again kick the can down the road by increasing the limit to 14.
>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Is this something I can just apply?

Yours,
Linus Walleij

Re: [PATCH v2 1/7] dt-bindings: pinctrl: rockchip: increase max amount of device functions

[Nicolas Frattaroli]

On Thursday, 5 June 2025 15:29:03 Central European Summer Time Linus Walleij wrote:
> On Mon, Jun 2, 2025 at 6:20 PM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> 
> > With the introduction of the RK3576, the maximum device function ID used
> > increased to 14, as anyone can easily verify for themselves with:
> >
> >   rg -g '*-pinctrl.dtsi' '<\d+\s+RK_P..\s+(?<func>\d+)\s.*>;$' --trim \
> >   -NI -r '$func' arch/arm64/boot/dts/rockchip/ | sort -g | uniq
> >
> > Unfortunately, this wasn't caught by dt-validate as those pins are
> > omit-if-no-ref and we had no reference to them in any tree so far.
> >
> > Once again kick the can down the road by increasing the limit to 14.
> >
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Is this something I can just apply?
> 
> Yours,
> Linus Walleij
> 

Absolutely, there's no harm in it landing early, and is only tangentially
related to the rest of the series because it came up while I was pinmuxing
my PWM pins.

Kind regards,
Nicolas Frattaroli

Re: [PATCH v2 2/7] dt-bindings: pwm: Add a new binding for rockchip,rk3576-pwm

[Rob Herring (Arm)]

On Mon, 02 Jun 2025 18:19:13 +0200, Nicolas Frattaroli wrote:
> The Rockchip RK3576 SoC has a newer PWM controller IP revision than
> previous Rockchip SoCs. This IP, called "PWMv4" by Rockchip, introduces
> several new features, and consequently differs in its bindings.
> 
> Instead of expanding the ever-growing rockchip-pwm binding that already
> has an if-condition, add an entirely new binding to handle this.
> 
> There are two additional clocks, "osc" and "rc". These are available for
> every PWM instance, and the PWM hardware can switch between the "pwm",
> "osc" and "rc" clock at runtime.
> 
> The PWM controller also comes with an interrupt now. This interrupt is
> used to signal various conditions.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  .../bindings/pwm/rockchip,rk3576-pwm.yaml          | 77 ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 ++
>  2 files changed, 84 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v2 1/7] dt-bindings: pinctrl: rockchip: increase max amount of device functions

[Linus Walleij]

On Mon, Jun 2, 2025 at 6:20 PM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:

> With the introduction of the RK3576, the maximum device function ID used
> increased to 14, as anyone can easily verify for themselves with:
>
>   rg -g '*-pinctrl.dtsi' '<\d+\s+RK_P..\s+(?<func>\d+)\s.*>;$' --trim \
>   -NI -r '$func' arch/arm64/boot/dts/rockchip/ | sort -g | uniq
>
> Unfortunately, this wasn't caught by dt-validate as those pins are
> omit-if-no-ref and we had no reference to them in any tree so far.
>
> Once again kick the can down the road by increasing the limit to 14.
>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

This  patch 1/7 applied to the pinctrl git tree for v6.17!

Yours,
Linus Walleij

Re: [PATCH v2 5/7] pwm: Add rockchip PWMv4 driver

[Uwe Kleine-König]

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

Hello Nicolas,

On Mon, Jun 02, 2025 at 06:19:16PM +0200, Nicolas Frattaroli wrote:
> +/**
> + * rockchip_pwm_v4_round_params - convert PWM parameters to hardware
> + * @rate: PWM clock rate to do the calculations at
> + * @duty: PWM duty cycle in nanoseconds
> + * @period: PWM period in nanoseconds
> + * @offset: PWM offset in nanoseconds
> + * @out_duty: pointer to where the rounded duty value should be stored
> + * @out_period: pointer to where the rounded period value should be stored
> + * @out_offset: pointer to where the rounded offset value should be stored
> + *
> + * Convert nanosecond-based duty/period/offset parameters to the PWM hardware's
> + * native rounded representation in number of cycles at clock rate @rate. Should
> + * any of the input parameters be out of range for the hardware, the
> + * corresponding output parameter is the maximum permissible value for said
> + * parameter with considerations to the others.
> + */
> +static void rockchip_pwm_v4_round_params(unsigned long rate, u64 duty,
> +					u64 period, u64 offset, u32 *out_duty,
> +					u32 *out_period, u32 *out_offset)
> +{
> +	int ret;
> +
> +	ret = rockchip_pwm_v4_round_single(rate, period, out_period);
> +	if (ret)
> +		*out_period = U32_MAX;

It's strange to let rockchip_pwm_v4_round_single return failure just to
reset it to U32_MAX here then. I'd make rockchip_pwm_v4_round_single do:

	tmp = mul_u64_u64_div_u64(rate, in_val, NSEC_PER_SEC);
	if (tmp > U32_MAX)
		return U32_MAX
	return tmp;

and then just do

	*out_period = rockchip_pwm_v4_round_single(rate, period);
	*out_duty = rockchip_pwm_v4_round_single(rate, duty)
	...

> +
> +	ret = rockchip_pwm_v4_round_single(rate, duty, out_duty);
> +	if (ret || *out_duty > *out_period)
> +		*out_duty = *out_period;

You can assume that .round_wf_tohw() is called only with duty <= period.

> +	ret = rockchip_pwm_v4_round_single(rate, offset, out_offset);
> +	if (ret || *out_offset > (*out_period - *out_duty))
> +		*out_offset = *out_period - *out_duty;

Is this a hardware limitation? In general

	.period_length_ns = 1000
	.duty_length_ns = 600
	.duty_offset_ns = 600

is a valid waveform.

> +}
> +
> +static int rockchip_pwm_v4_round_wf_tohw(struct pwm_chip *chip,
> +					 struct pwm_device *pwm,
> +					 const struct pwm_waveform *wf,
> +					 void *_wfhw)
> +{
> +	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
> +	struct rockchip_pwm_v4_wf *wfhw = _wfhw;
> +	unsigned long rate;
> +	int ret;
> +
> +	/* We do not want chosen_clk to change out from under us here */
> +	ret = mfpwm_acquire(pc->pwmf);
> +	if (ret)
> +		return ret;
> +
> +	rate = clk_get_rate(pc->pwmf->core);
> +
> +	rockchip_pwm_v4_round_params(rate, wf->duty_length_ns,
> +				     wf->period_length_ns, wf->duty_offset_ns,
> +				     &wfhw->duty, &wfhw->period, &wfhw->offset);
> +
> +	if (wf->period_length_ns > 0)
> +		wfhw->enable = PWMV4_EN_BOTH_MASK;
> +	else
> +		wfhw->enable = 0;
> +
> +	dev_dbg(&chip->dev, "tohw: duty = %u, period = %u, offset = %u, rate %lu\n",
> +		wfhw->duty, wfhw->period, wfhw->offset, rate);

This is more helpful if the input parameters (i.e. wf) is also emitted.

> +	mfpwm_release(pc->pwmf);
> +	return 0;
> +}
> +
> +static int rockchip_pwm_v4_round_wf_fromhw(struct pwm_chip *chip,
> +					   struct pwm_device *pwm,
> +					   const void *_wfhw,
> +					   struct pwm_waveform *wf)
> +{
> +	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
> +	const struct rockchip_pwm_v4_wf *wfhw = _wfhw;
> +	unsigned long rate;
> +	int ret = 0;
> +
> +	/* We do not want chosen_clk to change out from under us here */
> +	ret = mfpwm_acquire(pc->pwmf);
> +	if (ret)
> +		return ret;

Hmm, there is little gain here. Correct me if I'm wrong, but you prevent
a rate change only until mfpwm_release() is called, so the assertion
ends before the caller can use the calculated parameters anyhow. So
maybe drop the acquire/release pair?

> +	rate = clk_get_rate(pc->pwmf->core);
> +
> +	if (rockchip_pwm_v4_is_enabled(wfhw->enable)) {
> +		if (!rate) {
> +			ret = -EINVAL;
> +			goto out_mfpwm_release;
> +		}
> +		wf->period_length_ns = mul_u64_u64_div_u64(wfhw->period, NSEC_PER_SEC, rate);

(u64)wfhw->period * NSEC_PER_SEC cannot overflow, so a plain
multiplication and then a division is cheaper here.

> +		wf->duty_length_ns = mul_u64_u64_div_u64(wfhw->duty, NSEC_PER_SEC, rate);
> +		wf->duty_offset_ns = mul_u64_u64_div_u64(wfhw->offset, NSEC_PER_SEC, rate);
> +	} else {
> +		wf->period_length_ns = 0;
> +		wf->duty_length_ns = 0;
> +		wf->duty_offset_ns = 0;
> +	}
> +
> +	dev_dbg(&chip->dev, "fromhw: duty = %llu, period = %llu, offset = %llu, rate = %lu\n",
> +		wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, rate);

As above, please include wfhw in the output.

> +out_mfpwm_release:
> +	mfpwm_release(pc->pwmf);
> +	return ret;
> +}
> +
> [...]
> +static int rockchip_pwm_v4_probe(struct platform_device *pdev)
> +{
> +	struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev);
> +	struct rockchip_pwm_v4 *pc;
> +	struct pwm_chip *chip;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, 1, sizeof(*pc));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	pc = to_rockchip_pwm_v4(chip);
> +	pc->pwmf = pwmf;
> +
> +	ret = mfpwm_acquire(pwmf);
> +	if (ret == -EBUSY)
> +		dev_warn(dev, "pwm hardware already in use, can't check initial state\n");
> +	else if (ret < 0)
> +		return dev_err_probe(dev, ret, "couldn't acquire mfpwm in probe\n");
> +
> +	if (!rockchip_pwm_v4_on_and_continuous(pc))
> +		mfpwm_release(pwmf);
> +	else {
> +		dev_dbg(dev, "pwm was already on at probe time\n");
> +		ret = clk_enable(pwmf->core);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "enabling pwm clock failed\n");
> +		ret = clk_rate_exclusive_get(pc->pwmf->core);
> +		if (ret) {
> +			clk_disable(pwmf->core);
> +			return dev_err_probe(dev, ret, "protecting pwm clock failed\n");
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	chip->ops = &rockchip_pwm_v4_ops;
> +
> +	ret = pwmchip_add(chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add PWM chip\n");

I like error messages starting with a capital letter.

> +
> +	return 0;
> +}
> +
> +static void rockchip_pwm_v4_remove(struct platform_device *pdev)
> +{
> +	struct pwm_chip *chip = platform_get_drvdata(pdev);
> +	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
> +
> +	mfpwm_remove_func(pc->pwmf);

What does this function do? It is not used in .probe()'s error path?!

> +	pwmchip_remove(chip);

Wrong order (I think). If mfpwm_remove_func() affects operation,
pwmchip_remove() must be called first.

> +}
> +
> +static const struct platform_device_id rockchip_pwm_v4_ids[] = {
> +	{ .name = "pwm-rockchip-v4", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, rockchip_pwm_v4_ids);
> +
> +static struct platform_driver rockchip_pwm_v4_driver = {
> +	.probe = rockchip_pwm_v4_probe,
> +	.remove = rockchip_pwm_v4_remove,
> +	.driver = {
> +		.name = "pwm-rockchip-v4",
> +	},
> +	.id_table = rockchip_pwm_v4_ids,
> +};
> +module_platform_driver(rockchip_pwm_v4_driver);
> +
> +MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
> +MODULE_DESCRIPTION("Rockchip PWMv4 Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("ROCKCHIP_MFPWM");

Best regards
Uwe

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

Re: [PATCH v2 4/7] soc: rockchip: add mfpwm driver

[Heiko Stübner]

Hi Nicolas,

Am Montag, 2. Juni 2025, 18:19:15 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> With the Rockchip RK3576, the PWM IP used by Rockchip has changed
> substantially. Looking at both the downstream pwm-rockchip driver as
> well as the mainline pwm-rockchip driver made it clear that with all its
> additional features and its differences from previous IP revisions, it
> is best supported in a new driver.
> 
> This brings us to the question as to what such a new driver should be.
> To me, it soon became clear that it should actually be several new
> drivers, most prominently when Uwe Kleine-König let me know that I
> should not implement the pwm subsystem's capture callback, but instead
> write a counter driver for this functionality.
> 
> Combined with the other as-of-yet unimplemented functionality of this
> new IP, it became apparent that it needs to be spread across several
> subsystems.
> 
> For this reason, we add a new platform bus based driver, called mfpwm
> (short for "Multi-function PWM"). This "parent" driver makes sure that
> only one device function driver is using the device at a time, and is in
> charge of registering the platform bus devices for the individual device
> functions offered by the device.
> 
> An acquire/release pattern is used to guarantee that device function
> drivers don't step on each other's toes.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---

> +/**
> + * mfpwm_register_subdev - register a single mfpwm_func
> + * @mfpwm: pointer to the parent &struct rockchip_mfpwm
> + * @target: pointer to where the &struct platform_device pointer should be
> + *          stored, usually a member of @mfpwm
> + * @name: sub-device name string
> + *
> + * Allocate a single &struct mfpwm_func, fill its members with appropriate data,
> + * and register a new platform device, saving its pointer to @target. The
> + * allocation is devres tracked, so will be automatically freed on mfpwm remove.
> + *
> + * Returns: 0 on success, negative errno on error
> + */
> +static int mfpwm_register_subdev(struct rockchip_mfpwm *mfpwm,
> +				 struct platform_device **target,
> +				 const char *name)
> +{
> +	struct rockchip_mfpwm_func *func;
> +	struct platform_device *child;
> +
> +	func = devm_kzalloc(&mfpwm->pdev->dev, sizeof(*func), GFP_KERNEL);
> +	if (IS_ERR(func))
> +		return PTR_ERR(func);
> +	func->irq = mfpwm->irq;
> +	func->parent = mfpwm;
> +	func->id = atomic_inc_return(&subdev_id);
> +	func->base = mfpwm->base;
> +	func->core = mfpwm->chosen_clk;
> +	child = platform_device_register_data(&mfpwm->pdev->dev, name, func->id,
> +					      func, sizeof(*func));
> +
> +	if (IS_ERR(child))
> +		return PTR_ERR(child);
> +
> +	*target = child;
> +
> +	return 0;
> +}
> +
> +static int mfpwm_register_subdevs(struct rockchip_mfpwm *mfpwm)
> +{
> +	int ret;
> +
> +	ret = mfpwm_register_subdev(mfpwm, &mfpwm->pwm_dev, "pwm-rockchip-v4");
> +	if (ret)
> +		return ret;
> +
> +	ret = mfpwm_register_subdev(mfpwm, &mfpwm->counter_dev,
> +				    "rockchip-pwm-capture");
> +	if (ret)
> +		goto err_unreg_pwm_dev;
> +
> +	return 0;
> +
> +err_unreg_pwm_dev:
> +	platform_device_unregister(mfpwm->pwm_dev);
> +
> +	return ret;
> +}

I still had this lingering feeling that this _is_ a MFD just with added
sprinkles, so asked Lee on IRC about it:

	<lag> Looks like an MFD to me
	<lag> Yes, you can use an MFD core driver to control state / manage single-use resources

So, citing Jean Luc Picard, "Make it so" ... please :-)

Heiko

Re: [PATCH v2 6/7] counter: Add rockchip-pwm-capture driver

[William Breathitt Gray]

On Mon, Jun 02, 2025 at 06:19:17PM +0200, Nicolas Frattaroli wrote:
> Among many other things, Rockchip's new PWMv4 IP in the RK3576 supports
> PWM capture functionality.
> 
> Add a basic driver for this that works to capture period and duty cycle
> values and return them as nanoseconds to the user. It's quite basic, but
> works well enough to demonstrate the device function exclusion stuff
> that mfpwm does, in order to eventually support all the functions of
> this device in drivers within their appropriate subsystems, without them
> interfering with each other.
> 
> Once enabled, the counter driver waits for enough high-to-low and
> low-to-high interrupt signals to arrive, and then writes the cycle count
> register values into some atomic members of the driver instance's state
> struct. The read callback can then do the conversion from cycle count to
> the more useful period and duty cycle nanosecond values, which require
> knowledge of the clock rate, which requires a call that the interrupt
> handler cannot make itself because said call may sleep.
> 
> To detect the condition of a PWM signal disappearing, i.e. turning off,
> we modify the delay value of a delayed worker whose job it is to simply
> set those atomic members to zero. Should the "timeout" so to speak be
> reached, we assume the PWM signal is off. This isn't perfect; it
> obviously introduces a latency between it being off and the counter
> reporting it as such. Because there isn't a way to reset the internal
> double-buffered cycle count in the hardware, we filter out unreliable
> periods above the timeout value in the counter read callback.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Hi Nicolas,

Would you help me understand the computations in this driver?

If I understand the purpose of this driver correctly, it's meant to
compute the period and duty cycle of a PWM signal. What do LPC and HPC
represent? I'm guessing they are the low period count (LPC) and the high
period count (HPC). So then you calculate the total period by adding
LPC and HPC, whereas the duty cycle derives from HPC.

Am I understanding the algorithm correctly? What are the units of HPC
and LPC; are they ticks from the core clock? Are PWMV4_INT_LPC and
PWM4_INT_HPC the change-of-state interrupts for LPC and HPC
respectively?

The Counter subsystem can be used to derive the period and duty cycle of
a signal, but I believe there's a more idiomatic way to implement this.
Existing counter drivers such as microchip-tcb-capture achieve this by
leveraging Counter events exposed via the Counter chrdev interface.

The basic idea would be:
    * Expose LPC and HPC as count0 and count1;
    * Push the PWMV4_INT_LPC and PWMV4_INT_HPC interrupts as
      COUNTER_EVENT_CHANGE_OF_STATE events on channel 0 and channel 1
      respectively;
    * Register Counter watches in userspace to capture LPC and HPC on
      each interrupt;

The Counter chrdev interface records a timestamp in nanoseconds with
each event capture. So to compute period and duty cycle, you would
subtract the difference between two HPC/LPC captures; the difference in
the timestamps gives you the elapsed time between the two captures in
nanoseconds.

Would that design work for your use case?

William Breathitt Gray

Re: [PATCH v2 6/7] counter: Add rockchip-pwm-capture driver

[Nicolas Frattaroli]

On Sunday, 20 July 2025 02:20:15 Central European Summer Time William Breathitt Gray wrote:
> On Mon, Jun 02, 2025 at 06:19:17PM +0200, Nicolas Frattaroli wrote:
> > Among many other things, Rockchip's new PWMv4 IP in the RK3576 supports
> > PWM capture functionality.
> > 
> > Add a basic driver for this that works to capture period and duty cycle
> > values and return them as nanoseconds to the user. It's quite basic, but
> > works well enough to demonstrate the device function exclusion stuff
> > that mfpwm does, in order to eventually support all the functions of
> > this device in drivers within their appropriate subsystems, without them
> > interfering with each other.
> > 
> > Once enabled, the counter driver waits for enough high-to-low and
> > low-to-high interrupt signals to arrive, and then writes the cycle count
> > register values into some atomic members of the driver instance's state
> > struct. The read callback can then do the conversion from cycle count to
> > the more useful period and duty cycle nanosecond values, which require
> > knowledge of the clock rate, which requires a call that the interrupt
> > handler cannot make itself because said call may sleep.
> > 
> > To detect the condition of a PWM signal disappearing, i.e. turning off,
> > we modify the delay value of a delayed worker whose job it is to simply
> > set those atomic members to zero. Should the "timeout" so to speak be
> > reached, we assume the PWM signal is off. This isn't perfect; it
> > obviously introduces a latency between it being off and the counter
> > reporting it as such. Because there isn't a way to reset the internal
> > double-buffered cycle count in the hardware, we filter out unreliable
> > periods above the timeout value in the counter read callback.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> 
> Hi Nicolas,

Hi William,

> 
> Would you help me understand the computations in this driver?
> 
> If I understand the purpose of this driver correctly, it's meant to
> compute the period and duty cycle of a PWM signal. What do LPC and HPC
> represent? I'm guessing they are the low period count (LPC) and the high
> period count (HPC). So then you calculate the total period by adding
> LPC and HPC, whereas the duty cycle derives from HPC.
> 
> Am I understanding the algorithm correctly? What are the units of HPC
> and LPC; are they ticks from the core clock? Are PWMV4_INT_LPC and
> PWM4_INT_HPC the change-of-state interrupts for LPC and HPC
> respectively?

HPC = High Polarity Cycles, LPC = Low Polarity Cycles. They are counted
based on the pwm clock that the hardware runs at. PWMV4_INT_LPC and
PWMV4_INT_HPC are one-bit flags that are raised in the interrupt register
of this hardware when the interrupt is fired, to signal that LPC or HPC
changed state.

Your understanding of the algorithm appears to be correct, from my memory
of when I wrote the code. The 4 captures left logic is because the
hardware needs 4 level transitions before it can provide a useful
number for those two counts; thinking about it more now, I'm surprised it
can't do it in 3, so I'll need to double-check that next time I work on
this.

As an aside note, Rockchip has recently published the RK3506 Technical
Reference Manual, and the RK3506 SoC uses the same PWM IP as the RK3576
for which this driver is for. You can find it here in Chapter 31:

https://opensource.rock-chips.com/images/3/36/Rockchip_RK3506_TRM_Part_1_V1.2-20250811.pdf

This driver specifically implements "31.3.1 Capture Mode", later down
the line I may want to add "31.3.4 Clock Counter", "31.3.5 Clock
Frequency Meter" and "31.3.6 Biphasic Counter" but I lack a proper
signal generator so didn't want to bite off more than I could test.

> 
> The Counter subsystem can be used to derive the period and duty cycle of
> a signal, but I believe there's a more idiomatic way to implement this.
> Existing counter drivers such as microchip-tcb-capture achieve this by
> leveraging Counter events exposed via the Counter chrdev interface.
> 
> The basic idea would be:
>     * Expose LPC and HPC as count0 and count1;
>     * Push the PWMV4_INT_LPC and PWMV4_INT_HPC interrupts as
>       COUNTER_EVENT_CHANGE_OF_STATE events on channel 0 and channel 1
>       respectively;
>     * Register Counter watches in userspace to capture LPC and HPC on
>       each interrupt;
> 
> The Counter chrdev interface records a timestamp in nanoseconds with
> each event capture. So to compute period and duty cycle, you would
> subtract the difference between two HPC/LPC captures; the difference in
> the timestamps gives you the elapsed time between the two captures in
> nanoseconds.
> 
> Would that design work for your use case?

Basically, any design would work for me. I've only implemented the
counter driver to make sure the PWM reading part of the hardware works,
and Uwe advised me that new PWM drivers should use the counter
subsystem as opposed to implementing the PWM capture operation.

So I think your design makes more sense; any user of this driver would
likely want it to work like the other counter drivers, and exposing
LPC and HPC directly would also let me get rid of the quirky "is the
PWM actually off now" logic.

I'll do this when I get the chance to work on this patch series again,
as it needs a rewrite anyway to plug it into the MFD subsystem.

> 
> William Breathitt Gray
> 

Thank you for your review and suggestions!

Kind regards,
Nicolas Frattaroli