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

[PATCH 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.

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
tablets based on the RK3576 may need to do the power key stuff at some
stage, but I'm still not entirely clear on what that'd look like in a
schematic. The IR transmission stuff 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.

Some instances of the PWM silicon allow switching between a fixed
crystal oscillator as the PWM clock, and the default PLL clock, which is
just a mux between that very same crystal oscillator and two other fixed
oscillators on RK3576. The downstream vendor driver does not implement
this feature, but I did because it seemed funny, so now there's a sysfs
interface to switch between them and it makes sure you don't
accidentally ruin any PWM user's day while switching. The potential
benefit is that this switching is per-PWM-channel, but aside from that
it doesn't seem super useful and should've probably just been some
per-channel clock muxes in the hardware's CRU instead.

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>
---
Nicolas Frattaroli (7):
      dt-bindings: pinctrl: rockchip: increase max amount of device functions
      dt-bindings: pwm: Add a new binding for rockchip,rk3576-pwm
      soc: rockchip: add utils header for things shared across drivers
      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          |  94 ++++
 MAINTAINERS                                        |  11 +
 arch/arm64/boot/dts/rockchip/rk3576.dtsi           | 192 +++++++
 drivers/counter/Kconfig                            |  13 +
 drivers/counter/Makefile                           |   1 +
 drivers/counter/rockchip-pwm-capture.c             | 341 ++++++++++++
 drivers/pwm/Kconfig                                |  13 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-rockchip-v4.c                      | 336 ++++++++++++
 drivers/soc/rockchip/Kconfig                       |  13 +
 drivers/soc/rockchip/Makefile                      |   1 +
 drivers/soc/rockchip/mfpwm.c                       | 608 +++++++++++++++++++++
 include/soc/rockchip/mfpwm.h                       | 505 +++++++++++++++++
 include/soc/rockchip/utils.h                       |  76 +++
 15 files changed, 2206 insertions(+), 1 deletion(-)
---
base-commit: 64e9fdfc89a76fed38d8ddeed72d42ec71957ed9
change-id: 20250407-rk3576-pwm-46761bd0deaa

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

[PATCH 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.

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

The "osc" clock is an optional clock available on some instances of the
PWM controller. If present, it allows the controller to switch between
either the "pwm" clock or the "osc" clock for deriving its PWM signal
on a per-channel basis, through a hardware register write.

However, not all instances have this feature, and the hardware copes
just fine without this additional clock, so it's optional.

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          | 94 ++++++++++++++++++++++
 MAINTAINERS                                        |  7 ++
 2 files changed, 101 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..143d4df5df8fa89d508faca5ddf67603fb7cb3f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml
@@ -0,0 +1,94 @@
+# 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:
+    minItems: 2
+    items:
+      - description: Used to derive the PWM signal.
+      - description: Used as the APB bus clock.
+      - description: Used as an added alternative to derive the PWM signal.
+
+  clock-names:
+    minItems: 2
+    items:
+      - const: pwm
+      - const: pclk
+      - const: osc
+
+  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>;
+            clock-names = "pwm", "pclk", "osc";
+            interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
+            #pwm-cells = <3>;
+        };
+    };
+  - |
+    #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@2ade3000 {
+            compatible = "rockchip,rk3576-pwm";
+            reg = <0x0 0x2ade3000 0x0 0x1000>;
+            clocks = <&cru CLK_PWM2>, <&cru PCLK_PWM2>;
+            clock-names = "pwm", "pclk";
+            interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
+            #pwm-cells = <3>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 96b82704950184bd71623ff41fc4df31e4c7fe87..407179d2a90dd49800f2bb5770a1280c5afebb5a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20885,6 +20885,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 3/7] soc: rockchip: add utils header for things shared across drivers

[Nicolas Frattaroli]

Rockchip hardware has some functionality that is shared across many
hardware IPs, and therefore many drivers for them.

Most notably is "HIWORD_UPDATE", a macro with slightly different
semantics replicated across many a rockchip driver. It currently can be
found defined in 19 files, of which 18 are Rockchip drivers.

Instead of continuing this tradition with yet another version of it in
my new drivers, add a rockchip soc header for utility macros and such.
In this header, we define a new set of macros: REG_UPDATE_WE and its
little brother REG_UPDATE_BIT_WE. These are purposefully named something
other than "HIWORD_UPDATE", to reduce the likelihood of macro
redefinitions and also reduce the potential to mislead any adopter into
thinking this HIWORD_UPDATE is just like their HIWORD_UPDATE.

Old drivers can be moved over to the new macros over the next while if
their maintainers think it makes sense for them, which it probably does.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 include/soc/rockchip/utils.h | 76 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/include/soc/rockchip/utils.h b/include/soc/rockchip/utils.h
new file mode 100644
index 0000000000000000000000000000000000000000..3349069e75ff51ebd7a22089af796feafd227ffb
--- /dev/null
+++ b/include/soc/rockchip/utils.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2025 Collabora Ltd.
+ *
+ * Utility types, inline functions, and macros that are used across several
+ * Rockchip-specific drivers.
+ *
+ * Authors:
+ *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
+ */
+
+#ifndef __SOC_ROCKCHIP_UTILS_H__
+#define __SOC_ROCKCHIP_UTILS_H__
+
+#include <linux/bits.h>
+#include <linux/build_bug.h>
+#include <linux/limits.h>
+
+/*
+ * Incoming macro basilisks, stare directly at them at your own peril.
+ * As a gentle reminder to help with code comprehension: BUILD_BUG_ON_ZERO
+ * is confusingly named; it's a version of BUILD_BUG_ON that evaluates to zero
+ * if it does not trigger, i.e. the assertion within the macro still checks
+ * for a truthy value, not zero.
+ */
+
+/**
+ * REG_UPDATE_WE - generate a register write value with a write-enable mask
+ * @_val: unshifted value we wish to update between @_low and @_high
+ * @_low: index of the low bit of the bit range we want to update
+ * @_high: index of the high bit of the bit range we want to update
+ *
+ * This macro statically generates a value consisting of @_val shifted to the
+ * left by @_low, and a write-enable mask in the upper 16 bits of the value
+ * that sets bit ``i << 16`` to ``1`` if bit ``i`` is within the @_low to @_high
+ * range. Only up to bit (@_high - @_low) of @_val is used for safety, i.e.
+ * trying to write a value that doesn't fit in the specified range will simply
+ * truncate it.
+ *
+ * This is useful for some hardware, like some of Rockchip's registers, where
+ * a 32-bit width register is divided into a value low half, and a write enable
+ * high half. Bits in the low half are only update if the corresponding bit in
+ * the high half is ``1``, allowing for lock-free atomic updates of a register.
+ *
+ * This macro replaces the venerable ``HIWORD_UPDATE``, which is copied and
+ * pasted in slightly different forms across many different Rockchip drivers.
+ * Before switching drivers to use it, familiarise yourself with the semantics
+ * of your specific ``HIWORD_UPDATE`` compared to this function-like macro's
+ * semantics.
+ *
+ * Return: the value, shifted into place, with the required write-enable bits
+ */
+#define REG_UPDATE_WE(_val, _low, _high) ( \
+	BUILD_BUG_ON_ZERO(const_true((_low) > (_high))) + \
+	BUILD_BUG_ON_ZERO(const_true((_high) > 15)) + \
+	BUILD_BUG_ON_ZERO(const_true((_low) < 0)) + \
+	BUILD_BUG_ON_ZERO(const_true((u64) (_val) > U16_MAX)) + \
+	((_val & GENMASK((_high) - (_low), 0)) << (_low) | \
+	(GENMASK((_high), (_low)) << 16)))
+
+/**
+ * REG_UPDATE_BIT_WE - update a bit with a write-enable mask
+ * @__val: new value of the bit, either ``0`` 0r ``1``
+ * @__bit: bit index to modify, 0 <= @__bit < 16.
+ *
+ * This is like REG_UPDATE_WE() but only modifies a single bit, thereby making
+ * invocation easier by avoiding having to pass a repeated value.
+ *
+ * Return: a value with bit @__bit set to @__val and @__bit << 16 set to ``1``
+ */
+#define REG_UPDATE_BIT_WE(__val, __bit) ( \
+	BUILD_BUG_ON_ZERO(const_true((__val) > 1)) + \
+	BUILD_BUG_ON_ZERO(const_true((__val) < 0)) + \
+	REG_UPDATE_WE((__val), (__bit), (__bit)))
+
+#endif /* __SOC_ROCKCHIP_UTILS_H__ */

-- 
2.49.0

[PATCH 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  | 608 ++++++++++++++++++++++++++++++++++++++++++
 include/soc/rockchip/mfpwm.h  | 505 +++++++++++++++++++++++++++++++++++
 5 files changed, 1129 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 407179d2a90dd49800f2bb5770a1280c5afebb5a..e6a9347be1e7889089e1d9e655cb23c2d8399b40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20891,6 +20891,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..9331c530f0581573e2b74f62a6622b8625c5b2c5
--- /dev/null
+++ b/drivers/soc/rockchip/mfpwm.c
@@ -0,0 +1,608 @@
+// 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/clk.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
+ * @chosen_clk: is one of either @pwm_clk or @osc_clk, depending on choice.
+ *              May only be swapped out while holding @state_lock.
+ * @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.
+ * @pwmclk_enable_cnt: number of times @active_func has enabled the pwmclk sans
+ *                     disabling it. Must only be checked or modified while
+ *                     holding @state_lock. Only exists to fix a splat on mfpwm
+ *                     driver remove.
+ * @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 *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;
+	unsigned int pwmclk_enable_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);
+}
+
+unsigned long mfpwm_clk_get_rate(struct rockchip_mfpwm *mfpwm)
+{
+	if (!mfpwm || !mfpwm->chosen_clk)
+		return 0;
+
+	return clk_get_rate(mfpwm->chosen_clk);
+}
+EXPORT_SYMBOL_NS_GPL(mfpwm_clk_get_rate, "ROCKCHIP_MFPWM");
+
+static int mfpwm_check_pwmf(const struct rockchip_mfpwm_func *pwmf,
+			    const char *fname)
+{
+	if (IS_ERR_OR_NULL(pwmf)) {
+		WARN(1, "called %s with an erroneous handle, no effect\n",
+		     fname);
+		return -EINVAL;
+	}
+
+	if (IS_ERR_OR_NULL(pwmf->parent)) {
+		WARN(1, "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;
+}
+
+int mfpwm_pwmclk_enable(struct rockchip_mfpwm_func *pwmf)
+{
+	unsigned long flags;
+	int ret;
+
+	ret = mfpwm_check_pwmf(pwmf, "mfpwm_pwmclk_enable");
+	if (ret)
+		return ret;
+
+	spin_lock_irqsave(&pwmf->parent->state_lock, flags);
+	if (mfpwm_pwmf_is_active_pwmf(pwmf)) {
+		ret = clk_enable(pwmf->parent->chosen_clk);
+		pwmf->parent->pwmclk_enable_cnt++;
+	} else {
+		ret = -EBUSY;
+	}
+
+	spin_unlock_irqrestore(&pwmf->parent->state_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(mfpwm_pwmclk_enable, "ROCKCHIP_MFPWM");
+
+void mfpwm_pwmclk_disable(struct rockchip_mfpwm_func *pwmf)
+{
+	unsigned long flags;
+
+	if (mfpwm_check_pwmf(pwmf, "mfpwm_pwmclk_enable"))
+		return;
+
+	spin_lock_irqsave(&pwmf->parent->state_lock, flags);
+	if (mfpwm_pwmf_is_active_pwmf(pwmf)) {
+		clk_disable(pwmf->parent->chosen_clk);
+		pwmf->parent->pwmclk_enable_cnt--;
+	}
+	spin_unlock_irqrestore(&pwmf->parent->state_lock, flags);
+}
+EXPORT_SYMBOL_NS_GPL(mfpwm_pwmclk_disable, "ROCKCHIP_MFPWM");
+
+__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 {
+		WARN(1, "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);
+		for (; mfpwm->pwmclk_enable_cnt > 0; mfpwm->pwmclk_enable_cnt--)
+			clk_disable(mfpwm->chosen_clk);
+
+		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 {
+		WARN(1, "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;
+	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;
+}
+
+/**
+ * mfpwm_get_clk_src - read the currently selected clock source
+ * @mfpwm: pointer to the driver's private &struct rockchip_mfpwm instance
+ *
+ * Read the device register to extract the currently selected clock source,
+ * and return it.
+ *
+ * Returns:
+ * * the numeric clock source ID on success, 0 <= id <= 2
+ * * negative errno on error
+ */
+static int mfpwm_get_clk_src(struct rockchip_mfpwm *mfpwm)
+{
+	u32 val;
+
+	clk_enable(mfpwm->pclk);
+	val = mfpwm_reg_read(mfpwm->base, PWMV4_REG_CLK_CTRL);
+	clk_disable(mfpwm->pclk);
+
+	return (val & PWMV4_CLK_SRC_MASK) >> PWMV4_CLK_SRC_SHIFT;
+}
+
+static int mfpwm_choose_clk(struct rockchip_mfpwm *mfpwm)
+{
+	int ret;
+
+	ret = mfpwm_get_clk_src(mfpwm);
+	if (ret < 0) {
+		dev_err(&mfpwm->pdev->dev, "couldn't get current clock source: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+	if (ret == PWMV4_CLK_SRC_CRYSTAL) {
+		if (mfpwm->osc_clk) {
+			mfpwm->chosen_clk = mfpwm->osc_clk;
+		} else {
+			dev_warn(&mfpwm->pdev->dev, "initial state wanted 'osc' as clock source, but it's unavailable. Defaulting to 'pwm'.\n");
+			mfpwm->chosen_clk = mfpwm->pwm_clk;
+		}
+	} else {
+		mfpwm->chosen_clk = mfpwm->pwm_clk;
+	}
+
+	return clk_rate_exclusive_get(mfpwm->chosen_clk);
+}
+
+/**
+ * mfpwm_switch_clk_src - switch between PWM clock sources
+ * @mfpwm: pointer to &struct rockchip_mfpwm driver data
+ * @clk_src: one of either %PWMV4_CLK_SRC_CRYSTAL or %PWMV4_CLK_SRC_PLL
+ *
+ * Switch between clock sources, ``_exclusive_put``ing the old rate,
+ * ``clk_rate_exclusive_get``ing the new one, writing the registers and
+ * swapping out the &struct_rockchip_mfpwm->chosen_clk.
+ *
+ * Returns:
+ * * %0        - Success
+ * * %-EINVAL  - A wrong @clk_src was given or it is unavailable
+ * * %-EBUSY   - Device is currently in use, try again later
+ */
+__attribute__((nonnull))
+static int mfpwm_switch_clk_src(struct rockchip_mfpwm *mfpwm,
+					  unsigned int clk_src)
+{
+	struct clk *prev;
+	int ret = 0;
+
+	scoped_cond_guard(spinlock_try, return -EBUSY, &mfpwm->state_lock) {
+		/* Don't fiddle with any of this stuff if the PWM is on */
+		if (mfpwm->active_func)
+			return -EBUSY;
+
+		prev = mfpwm->chosen_clk;
+		ret = mfpwm_get_clk_src(mfpwm);
+		if (ret < 0)
+			return ret;
+		if (ret == clk_src)
+			return 0;
+
+		switch (clk_src) {
+		case PWMV4_CLK_SRC_PLL:
+			mfpwm->chosen_clk = mfpwm->pwm_clk;
+			break;
+		case PWMV4_CLK_SRC_CRYSTAL:
+			if (!mfpwm->osc_clk)
+				return -EINVAL;
+			mfpwm->chosen_clk = mfpwm->osc_clk;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		clk_enable(mfpwm->pclk);
+
+		mfpwm_reg_write(mfpwm->base, PWMV4_REG_CLK_CTRL,
+				PWMV4_CLK_SRC(clk_src));
+		clk_rate_exclusive_get(mfpwm->chosen_clk);
+		if (prev)
+			clk_rate_exclusive_put(prev);
+
+		clk_disable(mfpwm->pclk);
+	}
+
+	return ret;
+}
+
+static ssize_t chosen_clock_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
+	unsigned long clk_src = 0;
+
+	/*
+	 * Why the weird indirection here? I have the suspicion that if we
+	 * emitted to sysfs with the lock still held, then a nefarious program
+	 * could hog the lock by somehow forcing a full buffer condition and
+	 * then refusing to read from it. Don't know whether that's feasible
+	 * to achieve in reality, but I don't want to find out the hard way
+	 * either.
+	 */
+	scoped_guard(spinlock, &mfpwm->state_lock) {
+		if (mfpwm->chosen_clk == mfpwm->pwm_clk)
+			clk_src = PWMV4_CLK_SRC_PLL;
+		else if (mfpwm->osc_clk && mfpwm->chosen_clk == mfpwm->osc_clk)
+			clk_src = PWMV4_CLK_SRC_CRYSTAL;
+		else
+			return -ENODEV;
+	}
+
+	if (clk_src == PWMV4_CLK_SRC_PLL)
+		return sysfs_emit(buf, "pll\n");
+	else if (clk_src == PWMV4_CLK_SRC_CRYSTAL)
+		return sysfs_emit(buf, "crystal\n");
+
+	return -ENODEV;
+}
+
+static ssize_t chosen_clock_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
+	int ret;
+
+	if (sysfs_streq(buf, "pll")) {
+		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_PLL);
+		if (ret)
+			return ret;
+		return count;
+	} else if (sysfs_streq(buf, "crystal")) {
+		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_CRYSTAL);
+		if (ret)
+			return ret;
+		return count;
+	} else {
+		return -EINVAL;
+	}
+}
+
+static DEVICE_ATTR_RW(chosen_clock);
+
+static ssize_t available_clocks_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
+	ssize_t size = 0;
+
+	size += sysfs_emit_at(buf, size, "pll\n");
+	if (mfpwm->osc_clk)
+		size += sysfs_emit_at(buf, size, "crystal\n");
+
+	return size;
+}
+
+static DEVICE_ATTR_RO(available_clocks);
+
+static struct attribute *mfpwm_attrs[] = {
+	&dev_attr_available_clocks.attr,
+	&dev_attr_chosen_clock.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(mfpwm);
+
+static int rockchip_mfpwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rockchip_mfpwm *mfpwm;
+	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_optional_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");
+
+	ret = mfpwm_choose_clk(mfpwm);
+	if (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_rate_exclusive_put(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,
+		.dev_groups = mfpwm_groups,
+	},
+	.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..345f13f438b57159a15cb2e0ae250800fb96ed43
--- /dev/null
+++ b/include/soc/rockchip/mfpwm.h
@@ -0,0 +1,505 @@
+/* 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/clk.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
+#include <soc/rockchip/utils.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
+ */
+struct rockchip_mfpwm_func {
+	int id;
+	void __iomem *base;
+	struct rockchip_mfpwm *parent;
+	int irq;
+};
+
+/*
+ * 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 pwmv4_ver_chn_num(u32 val)
+{
+	return (val & GENMASK(3, 0));
+}
+
+static inline __pure u32 pwmv4_ver_chn_idx(u32 val)
+{
+	return (val & GENMASK(7, 4)) >> 4;
+}
+
+static inline __pure u32 pwmv4_ver_major(u32 val)
+{
+	return (val & GENMASK(31, 24)) >> 24;
+}
+
+static inline __pure u32 pwmv4_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)		REG_UPDATE_BIT_WE((v), 5)
+/*
+ * [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)		REG_UPDATE_BIT_WE((v), 4)
+/*
+ * [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)		REG_UPDATE_BIT_WE((v), 3)
+/*
+ * [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(v)		REG_UPDATE_BIT_WE((v), 2)
+#define PWMV4_CTRL_UPDATE_EN_MASK	BIT(2)
+/*
+ * [01:01] RW  | PWM Enable, 1 = enabled
+ *               If in one-shot mode, clears after end of operation
+ */
+#define PWMV4_EN(v)			REG_UPDATE_BIT_WE((v), 1)
+#define PWMV4_EN_MASK			BIT(1)
+/*
+ * [00:00] RW  | PWM Clock Enable, 1 = enabled
+ *               If in one-shot mode, clears after end of operation
+ */
+#define PWMV4_CLK_EN(v)			REG_UPDATE_BIT_WE((v), 0)
+#define PWMV4_CLK_EN_MASK		BIT(0)
+#define PWMV4_EN_BOTH_MASK		(PWMV4_EN_MASK | PWMV4_CLK_EN_MASK)
+static inline __pure bool pwmv4_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)		REG_UPDATE_BIT_WE((v), 15)
+/*
+ * [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: This reg is of questionable usefulness on RK3576, as it
+ *                     just muxes between 100m_50m_24m and 24m directly. The
+ *                     only use-case I can come up with is if you must use 100m
+ *                     or 50m PWM on one channel but absolutely require to use
+ *                     the lower rate 24m clock on another channel on the same
+ *                     chip, which doesn't seem like a realistic use case. I
+ *                     suspect that's why downstream doesn't use it.
+ */
+#define PWMV4_CLK_SRC_PLL		0x0U
+#define PWMV4_CLK_SRC_CRYSTAL		0x1U
+#define PWMV4_CLK_SRC_RC		0x2U
+#define PWMV4_CLK_SRC(v)		REG_UPDATE_WE((v), 13, 14)
+#define PWMV4_CLK_SRC_SHIFT		13
+#define PWMV4_CLK_SRC_MASK		GENMASK(14, 13)
+/*
+ * [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)		REG_UPDATE_WE((v), 4, 12)
+/*
+ * [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)		REG_UPDATE_WE((v), 0, 2)
+
+#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)		REG_UPDATE_WE((v), 6, 8)
+/* [05:05] RW  | Aligned Mode, 0 = Valid, 1 = Invalid */
+#define PWMV4_CTRL_UNALIGNED(v)		REG_UPDATE_BIT_WE((v), 5)
+/* [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)		REG_UPDATE_BIT_WE((v), 4)
+/*
+ * [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)		REG_UPDATE_BIT_WE((v), 3)
+/*
+ * [02:02] RW  | Duty Cycle Polarity to use at the start of the waveform.
+ *               0 = Negative, 1 = Positive
+ */
+#define PWMV4_DUTY_POL(v)		REG_UPDATE_BIT_WE((v), 2)
+#define PWMV4_DUTY_POL_MASK		BIT(2)
+#define PWMV4_DUTY_POL_SHIFT		2
+/*
+ * [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(v)			REG_UPDATE_WE((v), 0, 1)
+#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)		REG_UPDATE_BIT_WE(((v) ? 1 : 0), 0)
+#define PWMV4_INT_HPC_W(v)		REG_UPDATE_BIT_WE(((v) ? 1 : 0), 1)
+
+#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
+ */
+
+inline u32 mfpwm_reg_read(void __iomem *base, u32 reg)
+{
+	return readl(base + reg);
+}
+
+inline void mfpwm_reg_write(void __iomem *base, u32 reg, u32 val)
+{
+	writel(val, base + reg);
+}
+
+/**
+ * mfpwm_clk_get_rate - get the currently used clock's rate, in Hz
+ * @mfpwm: pointer to the &struct rockchip_mfpwm instance
+ *
+ * Returns: %0 on error, clock rate in Hz on success
+ */
+unsigned long mfpwm_clk_get_rate(struct rockchip_mfpwm *mfpwm);
+
+/**
+ * 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_pwmclk_enable - enable the pwm clock the signal and timing is based on
+ * @pwmf: pointer to the calling driver instance's &struct rockchip_mfpwm_func
+ *
+ * Context: must be the active device function to call this
+ *
+ * Returns: 0 on success, negative errno on error.
+ */
+int mfpwm_pwmclk_enable(struct rockchip_mfpwm_func *pwmf);
+
+/**
+ * mfpwm_pwmclk_disable - disable the pwm clock the signal and timing is based on
+ * @pwmf: pointer to the calling driver instance's &struct rockchip_mfpwm_func
+ *
+ * Context: must be the active device function to call this
+ */
+void mfpwm_pwmclk_disable(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 and its pwmclk_enables
+ * balanced and be removed as the active device function driver.
+ */
+void mfpwm_remove_func(struct rockchip_mfpwm_func *pwmf);
+
+#endif /* __SOC_ROCKCHIP_MFPWM_H__ */

-- 
2.49.0

[PATCH 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 | 336 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 351 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20891,6 +20891,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 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -540,6 +540,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_RZ_MTU3
 	tristate "Renesas RZ/G2L MTU3a PWM Timer support"
 	depends on RZ_MTU3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
 obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
+obj-$(CONFIG_PWM_ROCKCHIP_V4)	+= pwm-rockchip-v4.o
 obj-$(CONFIG_PWM_RZ_MTU3)	+= pwm-rz-mtu3.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c
new file mode 100644
index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e
--- /dev/null
+++ b/drivers/pwm/pwm-rockchip-v4.c
@@ -0,0 +1,336 @@
+// 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>
+ */
+
+#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.
+ *
+ * If @out_val is %NULL, no calculation is performed.
+ *
+ * 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;
+
+	if (!out_val)
+		return 0;
+
+	tmp = mult_frac(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
+ *            if NULL, don't calculate or store it
+ * @out_period: pointer to where the rounded period value should be stored
+ *              if NULL, don't calculate or store it
+ * @out_offset: pointer to where the rounded offset value should be stored
+ *              if NULL, don't calculate or store it
+ *
+ * Convert nanosecond-based duty/period/offset parameters to the PWM hardware's
+ * native rounded representation in number of cycles at clock rate @rate. If an
+ * out_ parameter is a NULL pointer, the corresponding parameter will not be
+ * calculated or stored. Should an overflow error occur for any of the
+ * parameters, assume the data at all the out_ locations is invalid and may not
+ * even have been touched at all.
+ *
+ * Return:
+ * * %0          - Success
+ * * %-EOVERFLOW - One of the results is too large for the PWM hardware
+ */
+static int 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, duty, out_duty);
+	if (ret)
+		return ret;
+
+	ret = rockchip_pwm_v4_round_single(rate, period, out_period);
+	if (ret)
+		return ret;
+
+	ret = rockchip_pwm_v4_round_single(rate, offset, out_offset);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+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 = 0;
+
+	/* We do not want chosen_clk to change out from under us here */
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	rate = mfpwm_clk_get_rate(pc->pwmf->parent);
+
+	ret = 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 ret;
+}
+
+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 = mfpwm_clk_get_rate(pc->pwmf->parent);
+
+	/* Let's avoid a cool division-by-zero if the clock's busted. */
+	if (!rate) {
+		ret = -EINVAL;
+		goto out_mfpwm_release;
+	}
+
+	wf->duty_length_ns = mult_frac(wfhw->duty, NSEC_PER_SEC, rate);
+
+	if (pwmv4_is_enabled(wfhw->enable))
+		wf->period_length_ns = mult_frac(wfhw->period, NSEC_PER_SEC,
+						 rate);
+	else
+		wf->period_length_ns = 0;
+
+	wf->duty_offset_ns = mult_frac(wfhw->offset, NSEC_PER_SEC, rate);
+
+	dev_dbg(&chip->dev, "fromhw: duty = %llu, period = %llu, offset = %llu\n",
+		wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns);
+
+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 = pwmv4_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,
+			REG_UPDATE_WE(wfhw->enable, 0, 1));
+
+	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(1));
+
+	if (pwmv4_is_enabled(wfhw->enable)) {
+		if (!was_enabled) {
+			dev_dbg(&chip->dev, "enabling PWM output\n");
+			ret = mfpwm_pwmclk_enable(pc->pwmf);
+			if (ret)
+				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");
+		mfpwm_pwmclk_disable(pc->pwmf);
+		/* Output is off now, extra release to balance extra acquire */
+		mfpwm_release(pc->pwmf);
+	}
+
+err_mfpwm_release:
+	mfpwm_release(pc->pwmf);
+
+	return ret;
+}
+
+/* We state the PWM chip is atomic, so none of these functions should sleep. */
+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 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;
+	int ret;
+
+	chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	pc = to_rockchip_pwm_v4(chip);
+	pc->pwmf = pwmf;
+
+	platform_set_drvdata(pdev, pc);
+
+	chip->ops = &rockchip_pwm_v4_ops;
+	chip->atomic = true;
+
+	ret = pwmchip_add(chip);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+
+	return 0;
+}
+
+static void rockchip_pwm_v4_remove(struct platform_device *pdev)
+{
+	struct rockchip_pwm_v4 *pc = platform_get_drvdata(pdev);
+
+	mfpwm_remove_func(pc->pwmf);
+}
+
+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 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. Additionally, periods longer than the timeout
value will cause the count to "flicker" between the correct period and
duty cycle values, and zero. This is because there doesn't appear to be
a way to reset the hardware's internal counters, even when writing to
the registers.

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 | 341 +++++++++++++++++++++++++++++++++
 4 files changed, 356 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34..e5d26256d05a04a9642371cf3dbb4dd0c1c34e68 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20891,6 +20891,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..b2bfa2c6e04dfa0410fa0d7ef1c395217e4a9db2
--- /dev/null
+++ b/drivers/counter/rockchip-pwm-capture.c
@@ -0,0 +1,341 @@
+// 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/math.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 = mfpwm_pwmclk_enable(pc->pwmf);
+			if (ret)
+				goto err_release;
+
+			ret = mfpwm_acquire(pc->pwmf);
+			if (ret)
+				goto err_disable_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);
+			mfpwm_pwmclk_disable(pc->pwmf);
+			pc->is_enabled = false;
+			mfpwm_release(pc->pwmf);
+		}
+
+		mfpwm_release(pc->pwmf);
+	}
+
+	return 0;
+
+err_disable_pwm_clk:
+	mfpwm_pwmclk_disable(pc->pwmf);
+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 = mfpwm_clk_get_rate(pc->pwmf->parent);
+	if (!rate) {
+		ret = -EINVAL;
+		goto out_release;
+	}
+
+	hpc = (u32) atomic_read(&pc->hpc);
+
+	if (count->id == COUNT_PERIOD) {
+		lpc = (u32) atomic_read(&pc->lpc);
+		period = mult_frac((hpc + lpc), NSEC_PER_SEC, rate);
+		*value = period;
+	} else {
+		*value = mult_frac(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 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 | 192 +++++++++++++++++++++++++++++++
 1 file changed, 192 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3576.dtsi b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
index ebb5fc8bb8b1363127b9d3782801c4a79b678a92..b6ba1d5569b3d961707b182eb5f960939de67c84 100644
--- a/arch/arm64/boot/dts/rockchip/rk3576.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
@@ -700,6 +700,30 @@ uart1: serial@27310000 {
 			status = "disabled";
 		};
 
+		pwm0_2ch_0: pwm@27330000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x27330000 0x0 0x1000>;
+			interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
+			#pwm-cells = <3>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm0m0_ch0>;
+			clocks = <&cru CLK_PMU1PWM>, <&cru PCLK_PMU1PWM>;
+			clock-names = "pwm", "pclk";
+			status = "disabled";
+		};
+
+		pwm0_2ch_1: pwm@27331000 {
+			compatible = "rockchip,rk3576-pwm";
+			reg = <0x0 0x27331000 0x0 0x1000>;
+			interrupts = <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>;
+			#pwm-cells = <3>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwm0m0_ch1>;
+			clocks = <&cru CLK_PMU1PWM>, <&cru PCLK_PMU1PWM>;
+			clock-names = "pwm", "pclk";
+			status = "disabled";
+		};
+
 		pmu: power-management@27380000 {
 			compatible = "rockchip,rk3576-pmu", "syscon", "simple-mfd";
 			reg = <0x0 0x27380000 0x0 0x800>;
@@ -1841,6 +1865,174 @@ 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>;
+			clock-names = "pwm", "pclk", "osc";
+			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>;
+			clock-names = "pwm", "pclk", "osc";
+			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>;
+			clock-names = "pwm", "pclk", "osc";
+			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>;
+			clock-names = "pwm", "pclk", "osc";
+			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>;
+			clock-names = "pwm", "pclk", "osc";
+			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>;
+			clock-names = "pwm", "pclk", "osc";
+			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>;
+			clock-names = "pwm", "pclk";
+			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>;
+			clock-names = "pwm", "pclk";
+			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>;
+			clock-names = "pwm", "pclk";
+			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>;
+			clock-names = "pwm", "pclk";
+			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>;
+			clock-names = "pwm", "pclk";
+			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>;
+			clock-names = "pwm", "pclk";
+			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>;
+			clock-names = "pwm", "pclk";
+			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>;
+			clock-names = "pwm", "pclk";
+			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 2/7] dt-bindings: pwm: Add a new binding for rockchip,rk3576-pwm

[Conor Dooley]

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

On Tue, Apr 08, 2025 at 02:32:14PM +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.
> 
> The "osc" clock is an optional clock available on some instances of the
> PWM controller. If present, it allows the controller to switch between
> either the "pwm" clock or the "osc" clock for deriving its PWM signal
> on a per-channel basis, through a hardware register write.
> 
> However, not all instances have this feature, and the hardware copes
> just fine without this additional clock, so it's optional.
> 
> 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>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

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

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

[Conor Dooley]

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

On Tue, Apr 08, 2025 at 02:32:13PM +0200, Nicolas Frattaroli 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.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Acked-by: Conor Dooley <conor.dooley@microchip.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
> 

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

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

[Rob Herring]

On Tue, Apr 08, 2025 at 02:32:13PM +0200, Nicolas Frattaroli 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.

Sounds like we need a way to disable that for validation. We'd need a 
dtc flag to ignore that and then set that flag for CHECK_DTBS.

Rob

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

[Heiko Stübner]

Hi,

not a full review, just me making a first pass.

> +unsigned long mfpwm_clk_get_rate(struct rockchip_mfpwm *mfpwm)
> +{
> +	if (!mfpwm || !mfpwm->chosen_clk)
> +		return 0;
> +
> +	return clk_get_rate(mfpwm->chosen_clk);
> +}
> +EXPORT_SYMBOL_NS_GPL(mfpwm_clk_get_rate, "ROCKCHIP_MFPWM");

aren't you just re-implemeting a clk-mux with the whole chosen-clk
mechanism? See drivers/clk/clk-mux.c, so in theory you should be
able to just do a clk_register_mux(...) similar to for example
sound/soc/samsung/i2s.c .


> +
> +__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 {
> +		WARN(1, "prevented acquire counter overflow in %s\n", __func__);

dev_warn, as you have the mfpwm pointing to a pdev?

> +		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);
> +}

> +/**
> + * mfpwm_get_clk_src - read the currently selected clock source
> + * @mfpwm: pointer to the driver's private &struct rockchip_mfpwm instance
> + *
> + * Read the device register to extract the currently selected clock source,
> + * and return it.
> + *
> + * Returns:
> + * * the numeric clock source ID on success, 0 <= id <= 2
> + * * negative errno on error
> + */
> +static int mfpwm_get_clk_src(struct rockchip_mfpwm *mfpwm)
> +{
> +	u32 val;
> +
> +	clk_enable(mfpwm->pclk);
> +	val = mfpwm_reg_read(mfpwm->base, PWMV4_REG_CLK_CTRL);
> +	clk_disable(mfpwm->pclk);
> +
> +	return (val & PWMV4_CLK_SRC_MASK) >> PWMV4_CLK_SRC_SHIFT;
> +}
> +
> +static int mfpwm_choose_clk(struct rockchip_mfpwm *mfpwm)
> +{
> +	int ret;
> +
> +	ret = mfpwm_get_clk_src(mfpwm);
> +	if (ret < 0) {
> +		dev_err(&mfpwm->pdev->dev, "couldn't get current clock source: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +	if (ret == PWMV4_CLK_SRC_CRYSTAL) {
> +		if (mfpwm->osc_clk) {
> +			mfpwm->chosen_clk = mfpwm->osc_clk;
> +		} else {
> +			dev_warn(&mfpwm->pdev->dev, "initial state wanted 'osc' as clock source, but it's unavailable. Defaulting to 'pwm'.\n");
> +			mfpwm->chosen_clk = mfpwm->pwm_clk;
> +		}
> +	} else {
> +		mfpwm->chosen_clk = mfpwm->pwm_clk;
> +	}
> +
> +	return clk_rate_exclusive_get(mfpwm->chosen_clk);
> +}
>
> +/**
> + * mfpwm_switch_clk_src - switch between PWM clock sources
> + * @mfpwm: pointer to &struct rockchip_mfpwm driver data
> + * @clk_src: one of either %PWMV4_CLK_SRC_CRYSTAL or %PWMV4_CLK_SRC_PLL
> + *
> + * Switch between clock sources, ``_exclusive_put``ing the old rate,
> + * ``clk_rate_exclusive_get``ing the new one, writing the registers and
> + * swapping out the &struct_rockchip_mfpwm->chosen_clk.
> + *
> + * Returns:
> + * * %0        - Success
> + * * %-EINVAL  - A wrong @clk_src was given or it is unavailable
> + * * %-EBUSY   - Device is currently in use, try again later
> + */
> +__attribute__((nonnull))
> +static int mfpwm_switch_clk_src(struct rockchip_mfpwm *mfpwm,
> +					  unsigned int clk_src)
> +{
> +	struct clk *prev;
> +	int ret = 0;
> +
> +	scoped_cond_guard(spinlock_try, return -EBUSY, &mfpwm->state_lock) {
> +		/* Don't fiddle with any of this stuff if the PWM is on */
> +		if (mfpwm->active_func)
> +			return -EBUSY;
> +
> +		prev = mfpwm->chosen_clk;
> +		ret = mfpwm_get_clk_src(mfpwm);
> +		if (ret < 0)
> +			return ret;
> +		if (ret == clk_src)
> +			return 0;
> +
> +		switch (clk_src) {
> +		case PWMV4_CLK_SRC_PLL:
> +			mfpwm->chosen_clk = mfpwm->pwm_clk;
> +			break;
> +		case PWMV4_CLK_SRC_CRYSTAL:
> +			if (!mfpwm->osc_clk)
> +				return -EINVAL;
> +			mfpwm->chosen_clk = mfpwm->osc_clk;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		clk_enable(mfpwm->pclk);
> +
> +		mfpwm_reg_write(mfpwm->base, PWMV4_REG_CLK_CTRL,
> +				PWMV4_CLK_SRC(clk_src));
> +		clk_rate_exclusive_get(mfpwm->chosen_clk);
> +		if (prev)
> +			clk_rate_exclusive_put(prev);
> +
> +		clk_disable(mfpwm->pclk);
> +	}
> +
> +	return ret;
> +}

ok, the relevant part might be the 
	/* Don't fiddle with any of this stuff if the PWM is on */
thing, which will require special set_rate operation, but in general I
think, if it ticks like a clock, it probably should be a real clock ;-) .


> +static ssize_t chosen_clock_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
> +	unsigned long clk_src = 0;
> +
> +	/*
> +	 * Why the weird indirection here? I have the suspicion that if we
> +	 * emitted to sysfs with the lock still held, then a nefarious program
> +	 * could hog the lock by somehow forcing a full buffer condition and
> +	 * then refusing to read from it. Don't know whether that's feasible
> +	 * to achieve in reality, but I don't want to find out the hard way
> +	 * either.
> +	 */
> +	scoped_guard(spinlock, &mfpwm->state_lock) {
> +		if (mfpwm->chosen_clk == mfpwm->pwm_clk)
> +			clk_src = PWMV4_CLK_SRC_PLL;
> +		else if (mfpwm->osc_clk && mfpwm->chosen_clk == mfpwm->osc_clk)
> +			clk_src = PWMV4_CLK_SRC_CRYSTAL;
> +		else
> +			return -ENODEV;
> +	}
> +
> +	if (clk_src == PWMV4_CLK_SRC_PLL)
> +		return sysfs_emit(buf, "pll\n");
> +	else if (clk_src == PWMV4_CLK_SRC_CRYSTAL)
> +		return sysfs_emit(buf, "crystal\n");
> +
> +	return -ENODEV;
> +}

which brings me to my main point of contention. Why does userspace
need to select a clock source for the driver via sysfs.

Neither the commit message nor the code does seem to explain that,
or I'm just blind - which is also a real possibility.

In general I really think, userspace should not need to care about if
a PLL or directly the oscillator is used a clock input.
I assume which is needed results from some runtime factor, so the
driver should be able to select the correct one?

A mux-clock could ust use clk_mux_determine_rate_flags() to select
the best parent depending on a requested rate instead.


> +static ssize_t chosen_clock_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (sysfs_streq(buf, "pll")) {
> +		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_PLL);
> +		if (ret)
> +			return ret;
> +		return count;
> +	} else if (sysfs_streq(buf, "crystal")) {
> +		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_CRYSTAL);
> +		if (ret)
> +			return ret;
> +		return count;
> +	} else {
> +		return -EINVAL;
> +	}
> +}
> +
> +static DEVICE_ATTR_RW(chosen_clock);
> +
> +static ssize_t available_clocks_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
> +	ssize_t size = 0;
> +
> +	size += sysfs_emit_at(buf, size, "pll\n");
> +	if (mfpwm->osc_clk)
> +		size += sysfs_emit_at(buf, size, "crystal\n");
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RO(available_clocks);
> +
> +static struct attribute *mfpwm_attrs[] = {
> +	&dev_attr_available_clocks.attr,
> +	&dev_attr_chosen_clock.attr,
> +	NULL,
> +};

Not understanding the need for the sysfs stuff was my main point this
evening :-)

Heiko

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

[Nicolas Frattaroli]

On Tuesday, 8 April 2025 22:03:01 Central European Summer Time Heiko Stübner wrote:
> Hi,
> 
> not a full review, just me making a first pass.
> 
> > +unsigned long mfpwm_clk_get_rate(struct rockchip_mfpwm *mfpwm)
> > +{
> > +	if (!mfpwm || !mfpwm->chosen_clk)
> > +		return 0;
> > +
> > +	return clk_get_rate(mfpwm->chosen_clk);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(mfpwm_clk_get_rate, "ROCKCHIP_MFPWM");
> 
> aren't you just re-implemeting a clk-mux with the whole chosen-clk
> mechanism? See drivers/clk/clk-mux.c, so in theory you should be
> able to just do a clk_register_mux(...) similar to for example
> sound/soc/samsung/i2s.c .

Probably yes. I didn't know clk-mux was a thing. If I do decide to keep the
clock switching at all (more on that below), then I'll rewrite it around
clk-mux.

> > +
> > +__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 {
> > +		WARN(1, "prevented acquire counter overflow in %s\n", __func__);
> 
> dev_warn, as you have the mfpwm pointing to a pdev?

Will do.

> > +		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);
> > +}
> 
> > +/**
> > + * mfpwm_get_clk_src - read the currently selected clock source
> > + * @mfpwm: pointer to the driver's private &struct rockchip_mfpwm instance
> > + *
> > + * Read the device register to extract the currently selected clock source,
> > + * and return it.
> > + *
> > + * Returns:
> > + * * the numeric clock source ID on success, 0 <= id <= 2
> > + * * negative errno on error
> > + */
> > +static int mfpwm_get_clk_src(struct rockchip_mfpwm *mfpwm)
> > +{
> > +	u32 val;
> > +
> > +	clk_enable(mfpwm->pclk);
> > +	val = mfpwm_reg_read(mfpwm->base, PWMV4_REG_CLK_CTRL);
> > +	clk_disable(mfpwm->pclk);
> > +
> > +	return (val & PWMV4_CLK_SRC_MASK) >> PWMV4_CLK_SRC_SHIFT;
> > +}
> > +
> > +static int mfpwm_choose_clk(struct rockchip_mfpwm *mfpwm)
> > +{
> > +	int ret;
> > +
> > +	ret = mfpwm_get_clk_src(mfpwm);
> > +	if (ret < 0) {
> > +		dev_err(&mfpwm->pdev->dev, "couldn't get current clock source: %pe\n",
> > +			ERR_PTR(ret));
> > +		return ret;
> > +	}
> > +	if (ret == PWMV4_CLK_SRC_CRYSTAL) {
> > +		if (mfpwm->osc_clk) {
> > +			mfpwm->chosen_clk = mfpwm->osc_clk;
> > +		} else {
> > +			dev_warn(&mfpwm->pdev->dev, "initial state wanted 'osc' as clock source, but it's unavailable. Defaulting to 'pwm'.\n");
> > +			mfpwm->chosen_clk = mfpwm->pwm_clk;
> > +		}
> > +	} else {
> > +		mfpwm->chosen_clk = mfpwm->pwm_clk;
> > +	}
> > +
> > +	return clk_rate_exclusive_get(mfpwm->chosen_clk);
> > +}
> >
> > +/**
> > + * mfpwm_switch_clk_src - switch between PWM clock sources
> > + * @mfpwm: pointer to &struct rockchip_mfpwm driver data
> > + * @clk_src: one of either %PWMV4_CLK_SRC_CRYSTAL or %PWMV4_CLK_SRC_PLL
> > + *
> > + * Switch between clock sources, ``_exclusive_put``ing the old rate,
> > + * ``clk_rate_exclusive_get``ing the new one, writing the registers and
> > + * swapping out the &struct_rockchip_mfpwm->chosen_clk.
> > + *
> > + * Returns:
> > + * * %0        - Success
> > + * * %-EINVAL  - A wrong @clk_src was given or it is unavailable
> > + * * %-EBUSY   - Device is currently in use, try again later
> > + */
> > +__attribute__((nonnull))
> > +static int mfpwm_switch_clk_src(struct rockchip_mfpwm *mfpwm,
> > +					  unsigned int clk_src)
> > +{
> > +	struct clk *prev;
> > +	int ret = 0;
> > +
> > +	scoped_cond_guard(spinlock_try, return -EBUSY, &mfpwm->state_lock) {
> > +		/* Don't fiddle with any of this stuff if the PWM is on */
> > +		if (mfpwm->active_func)
> > +			return -EBUSY;
> > +
> > +		prev = mfpwm->chosen_clk;
> > +		ret = mfpwm_get_clk_src(mfpwm);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (ret == clk_src)
> > +			return 0;
> > +
> > +		switch (clk_src) {
> > +		case PWMV4_CLK_SRC_PLL:
> > +			mfpwm->chosen_clk = mfpwm->pwm_clk;
> > +			break;
> > +		case PWMV4_CLK_SRC_CRYSTAL:
> > +			if (!mfpwm->osc_clk)
> > +				return -EINVAL;
> > +			mfpwm->chosen_clk = mfpwm->osc_clk;
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +		clk_enable(mfpwm->pclk);
> > +
> > +		mfpwm_reg_write(mfpwm->base, PWMV4_REG_CLK_CTRL,
> > +				PWMV4_CLK_SRC(clk_src));
> > +		clk_rate_exclusive_get(mfpwm->chosen_clk);
> > +		if (prev)
> > +			clk_rate_exclusive_put(prev);
> > +
> > +		clk_disable(mfpwm->pclk);
> > +	}
> > +
> > +	return ret;
> > +}
> 
> ok, the relevant part might be the 
> 	/* Don't fiddle with any of this stuff if the PWM is on */
> thing, which will require special set_rate operation, but in general I
> think, if it ticks like a clock, it probably should be a real clock ;-) .

I agree; we can guarantee it doesn't get changed after all by just marking it
as exclusive instead of marking either pwm_clk or osc_clk as exclusive.

> > +static ssize_t chosen_clock_show(struct device *dev,
> > +				 struct device_attribute *attr, char *buf)
> > +{
> > +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
> > +	unsigned long clk_src = 0;
> > +
> > +	/*
> > +	 * Why the weird indirection here? I have the suspicion that if we
> > +	 * emitted to sysfs with the lock still held, then a nefarious program
> > +	 * could hog the lock by somehow forcing a full buffer condition and
> > +	 * then refusing to read from it. Don't know whether that's feasible
> > +	 * to achieve in reality, but I don't want to find out the hard way
> > +	 * either.
> > +	 */
> > +	scoped_guard(spinlock, &mfpwm->state_lock) {
> > +		if (mfpwm->chosen_clk == mfpwm->pwm_clk)
> > +			clk_src = PWMV4_CLK_SRC_PLL;
> > +		else if (mfpwm->osc_clk && mfpwm->chosen_clk == mfpwm->osc_clk)
> > +			clk_src = PWMV4_CLK_SRC_CRYSTAL;
> > +		else
> > +			return -ENODEV;
> > +	}
> > +
> > +	if (clk_src == PWMV4_CLK_SRC_PLL)
> > +		return sysfs_emit(buf, "pll\n");
> > +	else if (clk_src == PWMV4_CLK_SRC_CRYSTAL)
> > +		return sysfs_emit(buf, "crystal\n");
> > +
> > +	return -ENODEV;
> > +}
> 
> which brings me to my main point of contention. Why does userspace
> need to select a clock source for the driver via sysfs.

It doesn't need to. Basically, this is a weird hardware feature. Downstream did
not bother implementing it at all, and I found out through the TRM's register
listing and thought "that's weird, I wonder if it even works", and lo and behold
it does. At that point, like two rewrites ago, I was committed to ensuring that
the driver can handle this edge case of the PWM clock being changed. As I lacked
the imagination as to why someone would change it and the knowledge as to which
kernel interfaces exist to change it, sysfs offered itself as a natural dumping
ground for switches that probably shouldn't exist.

> Neither the commit message nor the code does seem to explain that,
> or I'm just blind - which is also a real possibility.
> 
> In general I really think, userspace should not need to care about if
> a PLL or directly the oscillator is used a clock input.
> I assume which is needed results from some runtime factor, so the
> driver should be able to select the correct one?
> 
> A mux-clock could ust use clk_mux_determine_rate_flags() to select
> the best parent depending on a requested rate instead.

Yeah, the only use-case I can come up with is that we really want to use an
either 100 MHz or 50 MHz clock on one chip, but have a channel hit a precise
timing with the 24 MHz clock on the same chip. If the fixed crystal oscillator
were 25 MHz instead of 24 MHz, this would be entirely pointless, as they're all
multiples of it.

Thanks for the hint about clk_mux_determine_rate_flags, it doesn't appear to be
documented (classic) but it looks to do at least half of what a proper solution
would need to do. The other half is figuring out what ideal target rate we
actually want to optimise for for a given e.g. waveform consisting of period
and duty cycle in nanoseconds. There's some logic to think about regarding where
rounding errors are acceptable, e.g. a long period with a low duty cycle is
probably better off using the 100-50-24 mux with 100 MHz as the rate. I'm not
sure if 50 MHz is ever a sensible option since it is a dividend of 100 MHz, and
I'm not about to reason about imagined power draw of the PWM hardware without
laboratory grade test equipment.

For what it's worth, this is a niche enough hardware feature that if it causes
too much friction getting it supported in a driver, I'll just drop it entirely
instead. I tried to preemptively combat technical debt by supporting this in
some way, but instead managed to introduce scope creep.

One option is to always just choose the PLL muxed clock and then always set it
to 100 MHz, because it's probably the best option unless there are specific
PWM-based applications that make heavy use of 24-derived timings (maybe the IR
stuff?)

> 
> > +static ssize_t chosen_clock_store(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  const char *buf, size_t count)
> > +{
> > +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	if (sysfs_streq(buf, "pll")) {
> > +		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_PLL);
> > +		if (ret)
> > +			return ret;
> > +		return count;
> > +	} else if (sysfs_streq(buf, "crystal")) {
> > +		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_CRYSTAL);
> > +		if (ret)
> > +			return ret;
> > +		return count;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static DEVICE_ATTR_RW(chosen_clock);
> > +
> > +static ssize_t available_clocks_show(struct device *dev,
> > +				     struct device_attribute *attr, char *buf)
> > +{
> > +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
> > +	ssize_t size = 0;
> > +
> > +	size += sysfs_emit_at(buf, size, "pll\n");
> > +	if (mfpwm->osc_clk)
> > +		size += sysfs_emit_at(buf, size, "crystal\n");
> > +
> > +	return size;
> > +}
> > +
> > +static DEVICE_ATTR_RO(available_clocks);
> > +
> > +static struct attribute *mfpwm_attrs[] = {
> > +	&dev_attr_available_clocks.attr,
> > +	&dev_attr_chosen_clock.attr,
> > +	NULL,
> > +};
> 
> Not understanding the need for the sysfs stuff was my main point this
> evening :-)
> 
> Heiko
> 

Thank you for your quick preliminary review! This already gives me some good
points to look into for a v2.

Kind regards,
Nicolas Frattaroli

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

[William Breathitt Gray]

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

On Tue, Apr 08, 2025 at 02:32:18PM +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. Additionally, periods longer than the timeout
> value will cause the count to "flicker" between the correct period and
> duty cycle values, and zero. This is because there doesn't appear to be
> a way to reset the hardware's internal counters, even when writing to
> the registers.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Hi Nicolas,

I just want to give you a heads-up that I'm looking over this but it's
going to take me a couple more weeks or so; this hardware is a little
weird so I want to properly grok it before I Ack such a driver. In
particular, I'm not sure yet that the counter subsystem is necessarily
the right place for this functionality if you're ultimately after values
in units of time (sounds more like a clk framework feature) -- but we'll
determine so together.

That being said, please continue on to a version 2 of this patchset if
you have other changes ready -- I don't want the counter driver
bottlenecking the development of the rest of this series when progress
can be made independent of it.

Thanks,

William Breathitt Gray

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

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

[Damon Ding]

Hi Nicolas:

On 2025/4/9 21:01, Nicolas Frattaroli wrote:
> On Tuesday, 8 April 2025 22:03:01 Central European Summer Time Heiko Stübner wrote:
>> Hi,
>>
>> not a full review, just me making a first pass.
>>
>>> +unsigned long mfpwm_clk_get_rate(struct rockchip_mfpwm *mfpwm)
>>> +{
>>> +	if (!mfpwm || !mfpwm->chosen_clk)
>>> +		return 0;
>>> +
>>> +	return clk_get_rate(mfpwm->chosen_clk);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(mfpwm_clk_get_rate, "ROCKCHIP_MFPWM");
>>
>> aren't you just re-implemeting a clk-mux with the whole chosen-clk
>> mechanism? See drivers/clk/clk-mux.c, so in theory you should be
>> able to just do a clk_register_mux(...) similar to for example
>> sound/soc/samsung/i2s.c .
> 
> Probably yes. I didn't know clk-mux was a thing. If I do decide to keep the
> clock switching at all (more on that below), then I'll rewrite it around
> clk-mux.
> 
>>> +
>>> +__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 {
>>> +		WARN(1, "prevented acquire counter overflow in %s\n", __func__);
>>
>> dev_warn, as you have the mfpwm pointing to a pdev?
> 
> Will do.
> 
>>> +		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);
>>> +}
>>
>>> +/**
>>> + * mfpwm_get_clk_src - read the currently selected clock source
>>> + * @mfpwm: pointer to the driver's private &struct rockchip_mfpwm instance
>>> + *
>>> + * Read the device register to extract the currently selected clock source,
>>> + * and return it.
>>> + *
>>> + * Returns:
>>> + * * the numeric clock source ID on success, 0 <= id <= 2
>>> + * * negative errno on error
>>> + */
>>> +static int mfpwm_get_clk_src(struct rockchip_mfpwm *mfpwm)
>>> +{
>>> +	u32 val;
>>> +
>>> +	clk_enable(mfpwm->pclk);
>>> +	val = mfpwm_reg_read(mfpwm->base, PWMV4_REG_CLK_CTRL);
>>> +	clk_disable(mfpwm->pclk);
>>> +
>>> +	return (val & PWMV4_CLK_SRC_MASK) >> PWMV4_CLK_SRC_SHIFT;
>>> +}
>>> +
>>> +static int mfpwm_choose_clk(struct rockchip_mfpwm *mfpwm)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = mfpwm_get_clk_src(mfpwm);
>>> +	if (ret < 0) {
>>> +		dev_err(&mfpwm->pdev->dev, "couldn't get current clock source: %pe\n",
>>> +			ERR_PTR(ret));
>>> +		return ret;
>>> +	}
>>> +	if (ret == PWMV4_CLK_SRC_CRYSTAL) {
>>> +		if (mfpwm->osc_clk) {
>>> +			mfpwm->chosen_clk = mfpwm->osc_clk;
>>> +		} else {
>>> +			dev_warn(&mfpwm->pdev->dev, "initial state wanted 'osc' as clock source, but it's unavailable. Defaulting to 'pwm'.\n");
>>> +			mfpwm->chosen_clk = mfpwm->pwm_clk;
>>> +		}
>>> +	} else {
>>> +		mfpwm->chosen_clk = mfpwm->pwm_clk;
>>> +	}
>>> +
>>> +	return clk_rate_exclusive_get(mfpwm->chosen_clk);
>>> +}
>>>
>>> +/**
>>> + * mfpwm_switch_clk_src - switch between PWM clock sources
>>> + * @mfpwm: pointer to &struct rockchip_mfpwm driver data
>>> + * @clk_src: one of either %PWMV4_CLK_SRC_CRYSTAL or %PWMV4_CLK_SRC_PLL
>>> + *
>>> + * Switch between clock sources, ``_exclusive_put``ing the old rate,
>>> + * ``clk_rate_exclusive_get``ing the new one, writing the registers and
>>> + * swapping out the &struct_rockchip_mfpwm->chosen_clk.
>>> + *
>>> + * Returns:
>>> + * * %0        - Success
>>> + * * %-EINVAL  - A wrong @clk_src was given or it is unavailable
>>> + * * %-EBUSY   - Device is currently in use, try again later
>>> + */
>>> +__attribute__((nonnull))
>>> +static int mfpwm_switch_clk_src(struct rockchip_mfpwm *mfpwm,
>>> +					  unsigned int clk_src)
>>> +{
>>> +	struct clk *prev;
>>> +	int ret = 0;
>>> +
>>> +	scoped_cond_guard(spinlock_try, return -EBUSY, &mfpwm->state_lock) {
>>> +		/* Don't fiddle with any of this stuff if the PWM is on */
>>> +		if (mfpwm->active_func)
>>> +			return -EBUSY;
>>> +
>>> +		prev = mfpwm->chosen_clk;
>>> +		ret = mfpwm_get_clk_src(mfpwm);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		if (ret == clk_src)
>>> +			return 0;
>>> +
>>> +		switch (clk_src) {
>>> +		case PWMV4_CLK_SRC_PLL:
>>> +			mfpwm->chosen_clk = mfpwm->pwm_clk;
>>> +			break;
>>> +		case PWMV4_CLK_SRC_CRYSTAL:
>>> +			if (!mfpwm->osc_clk)
>>> +				return -EINVAL;
>>> +			mfpwm->chosen_clk = mfpwm->osc_clk;
>>> +			break;
>>> +		default:
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		clk_enable(mfpwm->pclk);
>>> +
>>> +		mfpwm_reg_write(mfpwm->base, PWMV4_REG_CLK_CTRL,
>>> +				PWMV4_CLK_SRC(clk_src));
>>> +		clk_rate_exclusive_get(mfpwm->chosen_clk);
>>> +		if (prev)
>>> +			clk_rate_exclusive_put(prev);
>>> +
>>> +		clk_disable(mfpwm->pclk);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>

First of all, I'm truly delighted to see your nice PWM v4 driver codes. 
I was once intensely struggling with porting the v4 PWM implementation 
into the Linux 6.1 ​​PWM framework. ;-)

>> ok, the relevant part might be the
>> 	/* Don't fiddle with any of this stuff if the PWM is on */
>> thing, which will require special set_rate operation, but in general I
>> think, if it ticks like a clock, it probably should be a real clock ;-) .
> 
> I agree; we can guarantee it doesn't get changed after all by just marking it
> as exclusive instead of marking either pwm_clk or osc_clk as exclusive.
> 
>>> +static ssize_t chosen_clock_show(struct device *dev,
>>> +				 struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
>>> +	unsigned long clk_src = 0;
>>> +
>>> +	/*
>>> +	 * Why the weird indirection here? I have the suspicion that if we
>>> +	 * emitted to sysfs with the lock still held, then a nefarious program
>>> +	 * could hog the lock by somehow forcing a full buffer condition and
>>> +	 * then refusing to read from it. Don't know whether that's feasible
>>> +	 * to achieve in reality, but I don't want to find out the hard way
>>> +	 * either.
>>> +	 */
>>> +	scoped_guard(spinlock, &mfpwm->state_lock) {
>>> +		if (mfpwm->chosen_clk == mfpwm->pwm_clk)
>>> +			clk_src = PWMV4_CLK_SRC_PLL;
>>> +		else if (mfpwm->osc_clk && mfpwm->chosen_clk == mfpwm->osc_clk)
>>> +			clk_src = PWMV4_CLK_SRC_CRYSTAL;
>>> +		else
>>> +			return -ENODEV;
>>> +	}
>>> +
>>> +	if (clk_src == PWMV4_CLK_SRC_PLL)
>>> +		return sysfs_emit(buf, "pll\n");
>>> +	else if (clk_src == PWMV4_CLK_SRC_CRYSTAL)
>>> +		return sysfs_emit(buf, "crystal\n");
>>> +
>>> +	return -ENODEV;
>>> +}
>>
>> which brings me to my main point of contention. Why does userspace
>> need to select a clock source for the driver via sysfs.
> 
> It doesn't need to. Basically, this is a weird hardware feature. Downstream did
> not bother implementing it at all, and I found out through the TRM's register
> listing and thought "that's weird, I wonder if it even works", and lo and behold
> it does. At that point, like two rewrites ago, I was committed to ensuring that
> the driver can handle this edge case of the PWM clock being changed. As I lacked
> the imagination as to why someone would change it and the knowledge as to which
> kernel interfaces exist to change it, sysfs offered itself as a natural dumping
> ground for switches that probably shouldn't exist.
> 
>> Neither the commit message nor the code does seem to explain that,
>> or I'm just blind - which is also a real possibility.
>>
>> In general I really think, userspace should not need to care about if
>> a PLL or directly the oscillator is used a clock input.
>> I assume which is needed results from some runtime factor, so the
>> driver should be able to select the correct one?
>>
>> A mux-clock could ust use clk_mux_determine_rate_flags() to select
>> the best parent depending on a requested rate instead.
> 
> Yeah, the only use-case I can come up with is that we really want to use an
> either 100 MHz or 50 MHz clock on one chip, but have a channel hit a precise
> timing with the 24 MHz clock on the same chip. If the fixed crystal oscillator
> were 25 MHz instead of 24 MHz, this would be entirely pointless, as they're all
> multiples of it.
> 

The 24MHz OSC clock source is mainly for the use of the IR input, which 
is also called the power key capture mode. When the system enters 
suspend state, it relies on the OSC to ensure the PWM remains 
operational and can serve as a system wake-up source.

In addition, the PWM v4 also supports the 400KHz RC clock source for the 
wave generator mode, and it can be used for the deeper sleep state.

> Thanks for the hint about clk_mux_determine_rate_flags, it doesn't appear to be
> documented (classic) but it looks to do at least half of what a proper solution
> would need to do. The other half is figuring out what ideal target rate we
> actually want to optimise for for a given e.g. waveform consisting of period
> and duty cycle in nanoseconds. There's some logic to think about regarding where
> rounding errors are acceptable, e.g. a long period with a low duty cycle is
> probably better off using the 100-50-24 mux with 100 MHz as the rate. I'm not
> sure if 50 MHz is ever a sensible option since it is a dividend of 100 MHz, and
> I'm not about to reason about imagined power draw of the PWM hardware without
> laboratory grade test equipment.
> 
> For what it's worth, this is a niche enough hardware feature that if it causes
> too much friction getting it supported in a driver, I'll just drop it entirely
> instead. I tried to preemptively combat technical debt by supporting this in
> some way, but instead managed to introduce scope creep.
> 
> One option is to always just choose the PLL muxed clock and then always set it
> to 100 MHz, because it's probably the best option unless there are specific
> PWM-based applications that make heavy use of 24-derived timings (maybe the IR
> stuff?)
> 
>>
>>> +static ssize_t chosen_clock_store(struct device *dev,
>>> +				  struct device_attribute *attr,
>>> +				  const char *buf, size_t count)
>>> +{
>>> +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	if (sysfs_streq(buf, "pll")) {
>>> +		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_PLL);
>>> +		if (ret)
>>> +			return ret;
>>> +		return count;
>>> +	} else if (sysfs_streq(buf, "crystal")) {
>>> +		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_CRYSTAL);
>>> +		if (ret)
>>> +			return ret;
>>> +		return count;
>>> +	} else {
>>> +		return -EINVAL;
>>> +	}
>>> +}
>>> +
>>> +static DEVICE_ATTR_RW(chosen_clock);
>>> +
>>> +static ssize_t available_clocks_show(struct device *dev,
>>> +				     struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
>>> +	ssize_t size = 0;
>>> +
>>> +	size += sysfs_emit_at(buf, size, "pll\n");
>>> +	if (mfpwm->osc_clk)
>>> +		size += sysfs_emit_at(buf, size, "crystal\n");
>>> +
>>> +	return size;
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO(available_clocks);
>>> +
>>> +static struct attribute *mfpwm_attrs[] = {
>>> +	&dev_attr_available_clocks.attr,
>>> +	&dev_attr_chosen_clock.attr,
>>> +	NULL,
>>> +};
>>
>> Not understanding the need for the sysfs stuff was my main point this
>> evening :-)
>>
>> Heiko
>>
> 
> Thank you for your quick preliminary review! This already gives me some good
> points to look into for a v2.
> 

I'm eagerly looking forward to your v2 patch submission, and I will 
thoroughly validate the proper functioning of the PWM 
backlight/regulator based on your code changes.

Best regards,
Damon

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

[Uwe Kleine-König]

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

Hello Nicolas,

On Tue, Apr 08, 2025 at 02:32:17PM +0200, Nicolas Frattaroli wrote:
> 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 | 336 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 351 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20891,6 +20891,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 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -540,6 +540,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_RZ_MTU3
>  	tristate "Renesas RZ/G2L MTU3a PWM Timer support"
>  	depends on RZ_MTU3
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
>  obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
>  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
>  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> +obj-$(CONFIG_PWM_ROCKCHIP_V4)	+= pwm-rockchip-v4.o
>  obj-$(CONFIG_PWM_RZ_MTU3)	+= pwm-rz-mtu3.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e
> --- /dev/null
> +++ b/drivers/pwm/pwm-rockchip-v4.c
> @@ -0,0 +1,336 @@
> +// 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.

Can you please add a "Limitations" paragraph here similar to the other
newer drivers that explains how the hardware behave on disable
(inactive? High-Z? freeze?), if there are glitches possible when
settings are changed or if the currently running period is completed on
reconfiguration.

> + *
> + * Authors:
> + *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> + */
> +
> +#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.
> + *
> + * If @out_val is %NULL, no calculation is performed.
> + *
> + * 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;
> +
> +	if (!out_val)
> +		return 0;

This is never hit, so better drop it.

> +	tmp = mult_frac(rate, in_val, NSEC_PER_SEC);
> +	if (tmp > U32_MAX)
> +		return -EOVERFLOW;

Is it clear that this cannot overflow the u64?

> +	*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
> + *            if NULL, don't calculate or store it
> + * @out_period: pointer to where the rounded period value should be stored
> + *              if NULL, don't calculate or store it
> + * @out_offset: pointer to where the rounded offset value should be stored
> + *              if NULL, don't calculate or store it
> + *
> + * Convert nanosecond-based duty/period/offset parameters to the PWM hardware's
> + * native rounded representation in number of cycles at clock rate @rate. If an
> + * out_ parameter is a NULL pointer, the corresponding parameter will not be
> + * calculated or stored. Should an overflow error occur for any of the
> + * parameters, assume the data at all the out_ locations is invalid and may not
> + * even have been touched at all.
> + *
> + * Return:
> + * * %0          - Success
> + * * %-EOVERFLOW - One of the results is too large for the PWM hardware
> + */
> +static int 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, duty, out_duty);
> +	if (ret)
> +		return ret;
> +
> +	ret = rockchip_pwm_v4_round_single(rate, period, out_period);
> +	if (ret)
> +		return ret;
> +
> +	ret = rockchip_pwm_v4_round_single(rate, offset, out_offset);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +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 = 0;
> +
> +	/* We do not want chosen_clk to change out from under us here */
> +	ret = mfpwm_acquire(pc->pwmf);
> +	if (ret)
> +		return ret;
> +
> +	rate = mfpwm_clk_get_rate(pc->pwmf->parent);
> +
> +	ret = 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 ret;

This is wrong. If a too high value for (say) period_length_ns is
requested, you're supposed to configure the maximal possible
period_length and not return a failure.

> +}
> +
> +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 */

This is also true while the PWM is enabled. 

> +	ret = mfpwm_acquire(pc->pwmf);
> +	if (ret)
> +		return ret;
> +
> +	rate = mfpwm_clk_get_rate(pc->pwmf->parent);

Why isn't that a proper clock that you can call clk_get_rate() (and
clk_rate_exclusive_get()) for?

> +	/* Let's avoid a cool division-by-zero if the clock's busted. */
> +	if (!rate) {
> +		ret = -EINVAL;
> +		goto out_mfpwm_release;
> +	}
> +
> +	wf->duty_length_ns = mult_frac(wfhw->duty, NSEC_PER_SEC, rate);
> +
> +	if (pwmv4_is_enabled(wfhw->enable))
> +		wf->period_length_ns = mult_frac(wfhw->period, NSEC_PER_SEC,
> +						 rate);
> +	else
> +		wf->period_length_ns = 0;
> +
> +	wf->duty_offset_ns = mult_frac(wfhw->offset, NSEC_PER_SEC, rate);
> +
> +	dev_dbg(&chip->dev, "fromhw: duty = %llu, period = %llu, offset = %llu\n",
> +		wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns);

In my experience it't helpful to include rate in the output here.

> +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 = pwmv4_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?"

Funny, I had this thought alread for mfpwm_acquire() above. Do you also
need that if wfhw->enable == 0?

> +	 * 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,
> +			REG_UPDATE_WE(wfhw->enable, 0, 1));
> +
> +	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(1));

PWMV4_CTRL_UPDATE_EN is always only used with 1 as parameter, so maybe
simplify the definition to BIT(2)?

> +	if (pwmv4_is_enabled(wfhw->enable)) {
> +		if (!was_enabled) {
> +			dev_dbg(&chip->dev, "enabling PWM output\n");
> +			ret = mfpwm_pwmclk_enable(pc->pwmf);
> +			if (ret)
> +				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;

Alternatively to calling mfpwm_acquire once more, you could also skip
the mfpwm_release() below. That makes the code a bit more complicated,
but also reduces the number of possible failing points.

> +		}
> +	} else if (was_enabled) {
> +		dev_dbg(&chip->dev, "disabling PWM output\n");
> +		mfpwm_pwmclk_disable(pc->pwmf);
> +		/* Output is off now, extra release to balance extra acquire */
> +		mfpwm_release(pc->pwmf);
> +	}
> +
> +err_mfpwm_release:
> +	mfpwm_release(pc->pwmf);
> +
> +	return ret;
> +}
> +
> +/* We state the PWM chip is atomic, so none of these functions should sleep. */
> +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 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;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	pc = to_rockchip_pwm_v4(chip);
> +	pc->pwmf = pwmf;
> +
> +	platform_set_drvdata(pdev, pc);
> +
> +	chip->ops = &rockchip_pwm_v4_ops;
> +	chip->atomic = true;
> +

If the PWM is already enabled you better call mfpwm_acquire() here?

> +	ret = pwmchip_add(chip);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +
> +	return 0;
> +}
> +
> +static void rockchip_pwm_v4_remove(struct platform_device *pdev)
> +{
> +	struct rockchip_pwm_v4 *pc = platform_get_drvdata(pdev);

	pwmchip_remove()?

> +	mfpwm_remove_func(pc->pwmf);

Maybe make this a devm function?

> +}
> +
> +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");

There are different tastes, but if you ask me that MODULE_IMPORT_NS can
go into the header declaring the functions from that namespace.

Best regards
Uwe

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

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

[Nicolas Frattaroli]

Hi Uwe,

thanks for the review. Just got started on v2, I'll respond or ack comments
inline.

On Tuesday, 13 May 2025 19:26:31 Central European Summer Time Uwe Kleine-König wrote:
> Hello Nicolas,
> 
> On Tue, Apr 08, 2025 at 02:32:17PM +0200, Nicolas Frattaroli wrote:
> > 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 | 336 ++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 351 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -20891,6 +20891,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 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -540,6 +540,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_RZ_MTU3
> >  	tristate "Renesas RZ/G2L MTU3a PWM Timer support"
> >  	depends on RZ_MTU3
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
> >  obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
> >  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
> >  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> > +obj-$(CONFIG_PWM_ROCKCHIP_V4)	+= pwm-rockchip-v4.o
> >  obj-$(CONFIG_PWM_RZ_MTU3)	+= pwm-rz-mtu3.o
> >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> >  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> > diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rockchip-v4.c
> > @@ -0,0 +1,336 @@
> > +// 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.
> 
> Can you please add a "Limitations" paragraph here similar to the other
> newer drivers that explains how the hardware behave on disable
> (inactive? High-Z? freeze?), if there are glitches possible when
> settings are changed or if the currently running period is completed on
> reconfiguration.

Will do. Might need a few long hours with the logic analyzer and poking at
the common clock framework to cover all bases.

> 
> > + *
> > + * Authors:
> > + *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > + */
> > +
> > +#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.
> > + *
> > + * If @out_val is %NULL, no calculation is performed.
> > + *
> > + * 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;
> > +
> > +	if (!out_val)
> > +		return 0;
> 
> This is never hit, so better drop it.

Will do.

> 
> > +	tmp = mult_frac(rate, in_val, NSEC_PER_SEC);
> > +	if (tmp > U32_MAX)
> > +		return -EOVERFLOW;
> 
> Is it clear that this cannot overflow the u64?

Good point. mult_frac in this case will expand to
  {
  	unsigned long x_ = (rate);
  	u64 n_ = (in_val);
  	long d_ = (NSEC_PER_SEC);
  	
  	unsigned long q = x_ / d_;
  	unsigned long r = x_ % d_;
  	q * n_ + r * n_ / d_;
  }

Whether or not this works out depends on the size of the unsigned long
type that it infers from rate, which will be 64 bits on arm64 and it
should only overflow in extreme cases, or 32 bits on arm32 (where this
driver may get used as well) where it goes horribly wrong way more often
as a result.

I'll replace it with the less suspect

  mul_u64_u64_div_u64(rate, in_val, NSEC_PER_SEC)

to avoid the C footguns.

> 
> > +	*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
> > + *            if NULL, don't calculate or store it
> > + * @out_period: pointer to where the rounded period value should be stored
> > + *              if NULL, don't calculate or store it
> > + * @out_offset: pointer to where the rounded offset value should be stored
> > + *              if NULL, don't calculate or store it
> > + *
> > + * Convert nanosecond-based duty/period/offset parameters to the PWM hardware's
> > + * native rounded representation in number of cycles at clock rate @rate. If an
> > + * out_ parameter is a NULL pointer, the corresponding parameter will not be
> > + * calculated or stored. Should an overflow error occur for any of the
> > + * parameters, assume the data at all the out_ locations is invalid and may not
> > + * even have been touched at all.
> > + *
> > + * Return:
> > + * * %0          - Success
> > + * * %-EOVERFLOW - One of the results is too large for the PWM hardware
> > + */
> > +static int 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, duty, out_duty);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = rockchip_pwm_v4_round_single(rate, period, out_period);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = rockchip_pwm_v4_round_single(rate, offset, out_offset);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +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 = 0;
> > +
> > +	/* We do not want chosen_clk to change out from under us here */
> > +	ret = mfpwm_acquire(pc->pwmf);
> > +	if (ret)
> > +		return ret;
> > +
> > +	rate = mfpwm_clk_get_rate(pc->pwmf->parent);
> > +
> > +	ret = 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 ret;
> 
> This is wrong. If a too high value for (say) period_length_ns is
> requested, you're supposed to configure the maximal possible
> period_length and not return a failure.

Ack. What if offset > period - duty is requested? Should I just saturate it
to period - duty in that case?

> 
> > +}
> > +
> > +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 */
> 
> This is also true while the PWM is enabled. 
> 
> > +	ret = mfpwm_acquire(pc->pwmf);
> > +	if (ret)
> > +		return ret;
> > +
> > +	rate = mfpwm_clk_get_rate(pc->pwmf->parent);
> 
> Why isn't that a proper clock that you can call clk_get_rate() (and
> clk_rate_exclusive_get()) for?

Because I didn't know clk-mux.c existed :( But even with it, I'm not sure
if letting mfpwm function drivers touch the clk directly is a good idea,
as this either means storing it in their pwmf struct or making the members
of the mfpwm struct part of the shared header.

> 
> > +	/* Let's avoid a cool division-by-zero if the clock's busted. */
> > +	if (!rate) {
> > +		ret = -EINVAL;
> > +		goto out_mfpwm_release;
> > +	}
> > +
> > +	wf->duty_length_ns = mult_frac(wfhw->duty, NSEC_PER_SEC, rate);
> > +
> > +	if (pwmv4_is_enabled(wfhw->enable))
> > +		wf->period_length_ns = mult_frac(wfhw->period, NSEC_PER_SEC,
> > +						 rate);
> > +	else
> > +		wf->period_length_ns = 0;
> > +
> > +	wf->duty_offset_ns = mult_frac(wfhw->offset, NSEC_PER_SEC, rate);

Note: I'll also need to replace all the mult_frac's here I think, as the
macro infers u32 as the result type here, which is needlessly restrictive
and probably prone to overflowing.

> > +
> > +	dev_dbg(&chip->dev, "fromhw: duty = %llu, period = %llu, offset = %llu\n",
> > +		wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns);
> 
> In my experience it't helpful to include rate in the output here.

Will do.

> 
> > +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 = pwmv4_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?"
> 
> Funny, I had this thought alread for mfpwm_acquire() above. Do you also
> need that if wfhw->enable == 0?

The only thing mfpwm_acquire does is check that this driver is the only
one using the hardware, and enabling the bus clk so registers can be
accessed. The clock that the PWM signal is derived from, as well as
enabling and disabling PWM, is a separate step. In this case, we need
to have acquired mfpwm beforehand to do the reg read for was_enabled.

> 
> > +	 * 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,
> > +			REG_UPDATE_WE(wfhw->enable, 0, 1));
> > +
> > +	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(1));
> 
> PWMV4_CTRL_UPDATE_EN is always only used with 1 as parameter, so maybe
> simplify the definition to BIT(2)?

Will do.

> 
> > +	if (pwmv4_is_enabled(wfhw->enable)) {
> > +		if (!was_enabled) {
> > +			dev_dbg(&chip->dev, "enabling PWM output\n");
> > +			ret = mfpwm_pwmclk_enable(pc->pwmf);
> > +			if (ret)
> > +				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;
> 
> Alternatively to calling mfpwm_acquire once more, you could also skip
> the mfpwm_release() below. That makes the code a bit more complicated,
> but also reduces the number of possible failing points.
> 

I think I did this at some stage but it'd just necessitate duplicating the
if condition at the release point. The else-if branch just down below still
needs to exist since we need to not just balance this function's acquire,
but the fact that we kept it acquired when we turned it on so we need to
release it an extra time when we turn it off. I don't think changing this
to eliminate the additional acquire call has clear benefits here.

> > +		}
> > +	} else if (was_enabled) {
> > +		dev_dbg(&chip->dev, "disabling PWM output\n");
> > +		mfpwm_pwmclk_disable(pc->pwmf);
> > +		/* Output is off now, extra release to balance extra acquire */
> > +		mfpwm_release(pc->pwmf);
> > +	}
> > +
> > +err_mfpwm_release:
> > +	mfpwm_release(pc->pwmf);
> > +
> > +	return ret;
> > +}
> > +
> > +/* We state the PWM chip is atomic, so none of these functions should sleep. */
> > +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 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;
> > +	int ret;
> > +
> > +	chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc));
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> > +
> > +	pc = to_rockchip_pwm_v4(chip);
> > +	pc->pwmf = pwmf;
> > +
> > +	platform_set_drvdata(pdev, pc);
> > +
> > +	chip->ops = &rockchip_pwm_v4_ops;
> > +	chip->atomic = true;
> > +
> 
> If the PWM is already enabled you better call mfpwm_acquire() here?

As in, if the hardware set the PWM on before the driver probed? I hadn't
considered this case, and will need to think about it. Could very well be
a possibility as u-boot does things before us.

> 
> > +	ret = pwmchip_add(chip);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static void rockchip_pwm_v4_remove(struct platform_device *pdev)
> > +{
> > +	struct rockchip_pwm_v4 *pc = platform_get_drvdata(pdev);
> 
> 	pwmchip_remove()?

Good catch, thanks. Will add.

> 
> > +	mfpwm_remove_func(pc->pwmf);
> 
> Maybe make this a devm function?

I vaguely recall trying that and running into issues with it but it's
possible I'm misremembering which remove caused issues.

> 
> > +}
> > +
> > +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");
> 
> There are different tastes, but if you ask me that MODULE_IMPORT_NS can
> go into the header declaring the functions from that namespace.
> 
> Best regards
> Uwe
> 

Kind regards,
Nicolas Frattaroli

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

[Uwe Kleine-König]

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

[Dropped Jonas Karlman from Cc as his email address doesn't work.]

Hello Nicolas,

On Thu, May 22, 2025 at 03:02:29PM +0200, Nicolas Frattaroli wrote:
> On Tuesday, 13 May 2025 19:26:31 Central European Summer Time Uwe Kleine-König wrote:
> > On Tue, Apr 08, 2025 at 02:32:17PM +0200, Nicolas Frattaroli wrote:
> > > 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 | 336 ++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 351 insertions(+)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -20891,6 +20891,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 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -540,6 +540,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_RZ_MTU3
> > >  	tristate "Renesas RZ/G2L MTU3a PWM Timer support"
> > >  	depends on RZ_MTU3
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
> > >  obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
> > >  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
> > >  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> > > +obj-$(CONFIG_PWM_ROCKCHIP_V4)	+= pwm-rockchip-v4.o
> > >  obj-$(CONFIG_PWM_RZ_MTU3)	+= pwm-rz-mtu3.o
> > >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> > >  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> > > diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-rockchip-v4.c
> > > @@ -0,0 +1,336 @@
> > > +// 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.
> > 
> > Can you please add a "Limitations" paragraph here similar to the other
> > newer drivers that explains how the hardware behave on disable
> > (inactive? High-Z? freeze?), if there are glitches possible when
> > settings are changed or if the currently running period is completed on
> > reconfiguration.
> 
> Will do. Might need a few long hours with the logic analyzer and poking at
> the common clock framework to cover all bases.

Usually it's simpler. e.g. if you set period=1s,duty=0 and then
period=2s,duty=2 an LED is enough to determine if the current period is
completed before a change.

You don't find High-Z with an LED but can distinguish between "inactive
when off" and "freeze when off". The datasheet might know about High-Z.

Apropos datasheet: If that is publically available, a reference to it in
the driver's header would be awesome.

> > > +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 = 0;
> > > +
> > > +	/* We do not want chosen_clk to change out from under us here */
> > > +	ret = mfpwm_acquire(pc->pwmf);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	rate = mfpwm_clk_get_rate(pc->pwmf->parent);
> > > +
> > > +	ret = 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 ret;
> > 
> > This is wrong. If a too high value for (say) period_length_ns is
> > requested, you're supposed to configure the maximal possible
> > period_length and not return a failure.
> 
> Ack. What if offset > period - duty is requested? Should I just saturate it
> to period - duty in that case?

If you configure period = 10, duty = offset = 6 the period restart is
supposed to happen during the active phase, that is it looks as follows:

    __     _____     _____     _____     ____
      \___/     \___/     \___/     \___/    
    ^         ^         ^         ^         ^
    01234567890

('^' marks the period start.)

> > > +	ret = mfpwm_acquire(pc->pwmf);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	rate = mfpwm_clk_get_rate(pc->pwmf->parent);
> > 
> > Why isn't that a proper clock that you can call clk_get_rate() (and
> > clk_rate_exclusive_get()) for?
> 
> Because I didn't know clk-mux.c existed :( But even with it, I'm not sure
> if letting mfpwm function drivers touch the clk directly is a good idea,
> as this either means storing it in their pwmf struct or making the members
> of the mfpwm struct part of the shared header.

The different drivers shouldn't need to touch the clk directly, the clk
API functions should be enough?!
 
> > > +	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 = pwmv4_is_enabled(mfpwm_reg_read(pc->pwmf->base,
> > > +						      PWMV4_REG_ENABLE));

Just noticed: pwmv4_is_enabled has the wrong prefix. Please use
"rockchip_pwm_v4" consistently.

> > > +	/*
> > > +	 * "But Nicolas", you ask with valid concerns, "why would you enable the
> > > +	 * PWM before setting all the parameter registers?"
> > 
> > Funny, I had this thought alread for mfpwm_acquire() above. Do you also
> > need that if wfhw->enable == 0?
> 
> The only thing mfpwm_acquire does is check that this driver is the only
> one using the hardware, and enabling the bus clk so registers can be
> accessed. The clock that the PWM signal is derived from, as well as
> enabling and disabling PWM, is a separate step. In this case, we need
> to have acquired mfpwm beforehand to do the reg read for was_enabled.

ok.

> > > +	if (pwmv4_is_enabled(wfhw->enable)) {
> > > +		if (!was_enabled) {
> > > +			dev_dbg(&chip->dev, "enabling PWM output\n");
> > > +			ret = mfpwm_pwmclk_enable(pc->pwmf);
> > > +			if (ret)
> > > +				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;
> > 
> > Alternatively to calling mfpwm_acquire once more, you could also skip
> > the mfpwm_release() below. That makes the code a bit more complicated,
> > but also reduces the number of possible failing points.
> 
> I think I did this at some stage but it'd just necessitate duplicating the
> if condition at the release point. The else-if branch just down below still
> needs to exist since we need to not just balance this function's acquire,
> but the fact that we kept it acquired when we turned it on so we need to
> release it an extra time when we turn it off. I don't think changing this
> to eliminate the additional acquire call has clear benefits here.

I'll keep that in mind and will try it maybe myself on top of v2.

> > > +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;
> > > +	int ret;
> > > +
> > > +	chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc));
> > > +	if (IS_ERR(chip))
> > > +		return PTR_ERR(chip);
> > > +
> > > +	pc = to_rockchip_pwm_v4(chip);
> > > +	pc->pwmf = pwmf;
> > > +
> > > +	platform_set_drvdata(pdev, pc);
> > > +
> > > +	chip->ops = &rockchip_pwm_v4_ops;
> > > +	chip->atomic = true;
> > > +
> > 
> > If the PWM is already enabled you better call mfpwm_acquire() here?
> 
> As in, if the hardware set the PWM on before the driver probed? I hadn't
> considered this case, and will need to think about it. Could very well be
> a possibility as u-boot does things before us.

The typical application is that the bootloader already shows a splash
screen and then you don't want Linux booting result in a flashing
display.

Best regards
Uwe

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

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

[Nicolas Frattaroli]

Hi Uwe,

On Friday, 23 May 2025 17:02:34 Central European Summer Time Uwe Kleine-König wrote:
> [Dropped Jonas Karlman from Cc as his email address doesn't work.]

Strange, I don't think I got any bounces, and your reply still has him in
the Cc ;) Just letting you know so that it doesn't look like I re-added him.

> Hello Nicolas,
> 
> On Thu, May 22, 2025 at 03:02:29PM +0200, Nicolas Frattaroli wrote:
> > On Tuesday, 13 May 2025 19:26:31 Central European Summer Time Uwe Kleine-König wrote:
> > > On Tue, Apr 08, 2025 at 02:32:17PM +0200, Nicolas Frattaroli wrote:
> > > > 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 | 336 ++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 351 insertions(+)
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -20891,6 +20891,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 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644
> > > > --- a/drivers/pwm/Kconfig
> > > > +++ b/drivers/pwm/Kconfig
> > > > @@ -540,6 +540,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_RZ_MTU3
> > > >  	tristate "Renesas RZ/G2L MTU3a PWM Timer support"
> > > >  	depends on RZ_MTU3
> > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > > index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644
> > > > --- a/drivers/pwm/Makefile
> > > > +++ b/drivers/pwm/Makefile
> > > > @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
> > > >  obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
> > > >  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
> > > >  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> > > > +obj-$(CONFIG_PWM_ROCKCHIP_V4)	+= pwm-rockchip-v4.o
> > > >  obj-$(CONFIG_PWM_RZ_MTU3)	+= pwm-rz-mtu3.o
> > > >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> > > >  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> > > > diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e
> > > > --- /dev/null
> > > > +++ b/drivers/pwm/pwm-rockchip-v4.c
> > > > @@ -0,0 +1,336 @@
> > > > +// 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.
> > > 
> > > Can you please add a "Limitations" paragraph here similar to the other
> > > newer drivers that explains how the hardware behave on disable
> > > (inactive? High-Z? freeze?), if there are glitches possible when
> > > settings are changed or if the currently running period is completed on
> > > reconfiguration.
> > 
> > Will do. Might need a few long hours with the logic analyzer and poking at
> > the common clock framework to cover all bases.
> 
> Usually it's simpler. e.g. if you set period=1s,duty=0 and then
> period=2s,duty=2 an LED is enough to determine if the current period is
> completed before a change.
> 
> You don't find High-Z with an LED but can distinguish between "inactive
> when off" and "freeze when off". The datasheet might know about High-Z.

I've used a logic analyzer to quickly determine this, it went fairly
smoothly and didn't take long at all. The PWM output stops immediately
and freezes in whatever state it was in at that point in time, i.e.
stays either low or high. Changes to period/duty/offset params only
seem to take effect at the start of the next period, so changing them
should be glitch-free.

I will add a full limitations paragraph, including a TODO for the future
with an idea for how to change the driver so that it lets the current
period complete when turning the PWM off. I'm not implementing it right
away in this series because that'd involve adding some IRQ handlers to
this driver as well and the series is big enough as it is already :)

> Apropos datasheet: If that is publically available, a reference to it in
> the driver's header would be awesome.

I've noted that down as a thing to ask Rockchip. Currently, the technical
reference manual of this SoC unfortunately is not publicly available, at
least not in an official capacity. I'll mention it'd also be sufficient
if we had just the PWM section of that TRM so that I can link to it.

> > > > +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 = 0;
> > > > +
> > > > +	/* We do not want chosen_clk to change out from under us here */
> > > > +	ret = mfpwm_acquire(pc->pwmf);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	rate = mfpwm_clk_get_rate(pc->pwmf->parent);
> > > > +
> > > > +	ret = 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 ret;
> > > 
> > > This is wrong. If a too high value for (say) period_length_ns is
> > > requested, you're supposed to configure the maximal possible
> > > period_length and not return a failure.
> > 
> > Ack. What if offset > period - duty is requested? Should I just saturate it
> > to period - duty in that case?
> 
> If you configure period = 10, duty = offset = 6 the period restart is
> supposed to happen during the active phase, that is it looks as follows:
> 
>     __     _____     _____     _____     ____
>       \___/     \___/     \___/     \___/    
>     ^         ^         ^         ^         ^
>     01234567890
> 
> ('^' marks the period start.)

Okay, this might make this a bit more complicated then. The hardware in the
TRM at least states for the offset register

  The value ranges from 0 to (period-duty)

which I think means that I have to actually make use of the hardware's
polarity setting, set it to inverse polarity, and then set the offset
to duty and the duty to period - duty if I understand this right.

Offset right now is just used by the pwm core to do inversion, right? As
in there's no handy sysfs knob I can shove values into to set it to an
arbitrary number?

I may also just shove a value above (period - duty) into the offset reg
to see if the hardware already behaves in the expected way and doing the
math manually would be overcomplicating things.

> > > > +	ret = mfpwm_acquire(pc->pwmf);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	rate = mfpwm_clk_get_rate(pc->pwmf->parent);
> > > 
> > > Why isn't that a proper clock that you can call clk_get_rate() (and
> > > clk_rate_exclusive_get()) for?
> > 
> > Because I didn't know clk-mux.c existed :( But even with it, I'm not sure
> > if letting mfpwm function drivers touch the clk directly is a good idea,
> > as this either means storing it in their pwmf struct or making the members
> > of the mfpwm struct part of the shared header.
> 
> The different drivers shouldn't need to touch the clk directly, the clk
> API functions should be enough?!

It's not just enough, it's too much. I don't want to give every pwmf
instance a pointer to the clock mux and then let the function drivers call
every common clock framework function on it that they wish to call.

I'll think about it more. clk_rate_exclusive_get/_put should protect
against the kinds of cross-function-driver interference hijinks I'm
worried about, so maybe the indirection isn't needed.

> > > > +	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 = pwmv4_is_enabled(mfpwm_reg_read(pc->pwmf->base,
> > > > +						      PWMV4_REG_ENABLE));
> 
> Just noticed: pwmv4_is_enabled has the wrong prefix. Please use
> "rockchip_pwm_v4" consistently.

Will do.

> [...]
>
> > > > +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;
> > > > +	int ret;
> > > > +
> > > > +	chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc));
> > > > +	if (IS_ERR(chip))
> > > > +		return PTR_ERR(chip);
> > > > +
> > > > +	pc = to_rockchip_pwm_v4(chip);
> > > > +	pc->pwmf = pwmf;
> > > > +
> > > > +	platform_set_drvdata(pdev, pc);
> > > > +
> > > > +	chip->ops = &rockchip_pwm_v4_ops;
> > > > +	chip->atomic = true;
> > > > +
> > > 
> > > If the PWM is already enabled you better call mfpwm_acquire() here?
> > 
> > As in, if the hardware set the PWM on before the driver probed? I hadn't
> > considered this case, and will need to think about it. Could very well be
> > a possibility as u-boot does things before us.
> 
> The typical application is that the bootloader already shows a splash
> screen and then you don't want Linux booting result in a flashing
> display.

Gotcha, that does sound fairly important and I've implemented it for v2
now. Managed to successfully test it with some manual register writes
from u-boot.

Haven't really decided yet whether I'll send v2 out soon or wait for -rc1
to release to base it against. I'm currently leaning towards sending it
out before -rc1 just because I don't want to rob reviewers of up to two
additional weeks of potential review time, especially since v2 is already
substantially different based on the changes I've staged for it so far.

> 
> Best regards
> Uwe

Kind regards,
Nicolas Frattaroli

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

[Heiko Stübner]

Am Dienstag, 8. April 2025, 14:32:13 Mitteleuropäische Sommerzeit schrieb 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.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH 3/7] soc: rockchip: add utils header for things shared across drivers

[Heiko Stübner]

Hi,

Am Dienstag, 8. April 2025, 14:32:15 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> Rockchip hardware has some functionality that is shared across many
> hardware IPs, and therefore many drivers for them.
> 
> Most notably is "HIWORD_UPDATE", a macro with slightly different
> semantics replicated across many a rockchip driver. It currently can be
> found defined in 19 files, of which 18 are Rockchip drivers.
> 
> Instead of continuing this tradition with yet another version of it in
> my new drivers, add a rockchip soc header for utility macros and such.
> In this header, we define a new set of macros: REG_UPDATE_WE and its
> little brother REG_UPDATE_BIT_WE. These are purposefully named something
> other than "HIWORD_UPDATE", to reduce the likelihood of macro
> redefinitions and also reduce the potential to mislead any adopter into
> thinking this HIWORD_UPDATE is just like their HIWORD_UPDATE.
> 
> Old drivers can be moved over to the new macros over the next while if
> their maintainers think it makes sense for them, which it probably does.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

when you're doing these fancy nice new macros, I think they might want to
be even more centrally located for _everyone_ :-) .


Because while true, Rockchip seems to be the biggest user of hiword-mask-
registers, they're not the only one.

Just simply grepping for HIWORD in kernel drivers revealed
- Sunplus sp7021 clock and reset drivers [0]
- a number of hisilicon clock drivers [1]
- some other clock drivers

and as the naming is not really standarized, I guess there will be more
of the same thing under different names in other places.

Similarly, we already have a FIELD_PREP_HIWORD in [2], so all in all
I think all of this wants to move in with the other bitfield stuff like
FIELD_PREP.


Heiko


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-sp7021.c#n42
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/reset-sunplus.c
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/hisilicon/clk-hi3620.c
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/hisilicon/clk-hi3660.c
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/hisilicon/clk-hi3670.c
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/hisilicon/clk-hi6220.c
[2] https://elixir.bootlin.com/linux/v6.15/source/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c#L23

> ---
>  include/soc/rockchip/utils.h | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/include/soc/rockchip/utils.h b/include/soc/rockchip/utils.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..3349069e75ff51ebd7a22089af796feafd227ffb
> --- /dev/null
> +++ b/include/soc/rockchip/utils.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2025 Collabora Ltd.
> + *
> + * Utility types, inline functions, and macros that are used across several
> + * Rockchip-specific drivers.
> + *
> + * Authors:
> + *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> + */
> +
> +#ifndef __SOC_ROCKCHIP_UTILS_H__
> +#define __SOC_ROCKCHIP_UTILS_H__
> +
> +#include <linux/bits.h>
> +#include <linux/build_bug.h>
> +#include <linux/limits.h>
> +
> +/*
> + * Incoming macro basilisks, stare directly at them at your own peril.
> + * As a gentle reminder to help with code comprehension: BUILD_BUG_ON_ZERO
> + * is confusingly named; it's a version of BUILD_BUG_ON that evaluates to zero
> + * if it does not trigger, i.e. the assertion within the macro still checks
> + * for a truthy value, not zero.
> + */
> +
> +/**
> + * REG_UPDATE_WE - generate a register write value with a write-enable mask
> + * @_val: unshifted value we wish to update between @_low and @_high
> + * @_low: index of the low bit of the bit range we want to update
> + * @_high: index of the high bit of the bit range we want to update
> + *
> + * This macro statically generates a value consisting of @_val shifted to the
> + * left by @_low, and a write-enable mask in the upper 16 bits of the value
> + * that sets bit ``i << 16`` to ``1`` if bit ``i`` is within the @_low to @_high
> + * range. Only up to bit (@_high - @_low) of @_val is used for safety, i.e.
> + * trying to write a value that doesn't fit in the specified range will simply
> + * truncate it.
> + *
> + * This is useful for some hardware, like some of Rockchip's registers, where
> + * a 32-bit width register is divided into a value low half, and a write enable
> + * high half. Bits in the low half are only update if the corresponding bit in
> + * the high half is ``1``, allowing for lock-free atomic updates of a register.
> + *
> + * This macro replaces the venerable ``HIWORD_UPDATE``, which is copied and
> + * pasted in slightly different forms across many different Rockchip drivers.
> + * Before switching drivers to use it, familiarise yourself with the semantics
> + * of your specific ``HIWORD_UPDATE`` compared to this function-like macro's
> + * semantics.
> + *
> + * Return: the value, shifted into place, with the required write-enable bits
> + */
> +#define REG_UPDATE_WE(_val, _low, _high) ( \
> +	BUILD_BUG_ON_ZERO(const_true((_low) > (_high))) + \
> +	BUILD_BUG_ON_ZERO(const_true((_high) > 15)) + \
> +	BUILD_BUG_ON_ZERO(const_true((_low) < 0)) + \
> +	BUILD_BUG_ON_ZERO(const_true((u64) (_val) > U16_MAX)) + \
> +	((_val & GENMASK((_high) - (_low), 0)) << (_low) | \
> +	(GENMASK((_high), (_low)) << 16)))
> +
> +/**
> + * REG_UPDATE_BIT_WE - update a bit with a write-enable mask
> + * @__val: new value of the bit, either ``0`` 0r ``1``
> + * @__bit: bit index to modify, 0 <= @__bit < 16.
> + *
> + * This is like REG_UPDATE_WE() but only modifies a single bit, thereby making
> + * invocation easier by avoiding having to pass a repeated value.
> + *
> + * Return: a value with bit @__bit set to @__val and @__bit << 16 set to ``1``
> + */
> +#define REG_UPDATE_BIT_WE(__val, __bit) ( \
> +	BUILD_BUG_ON_ZERO(const_true((__val) > 1)) + \
> +	BUILD_BUG_ON_ZERO(const_true((__val) < 0)) + \
> +	REG_UPDATE_WE((__val), (__bit), (__bit)))
> +
> +#endif /* __SOC_ROCKCHIP_UTILS_H__ */
> 
> 

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

[Heiko Stübner]

Am Dienstag, 8. April 2025, 14:32:16 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>

actually trying to compile this, led me to

aarch64-linux-gnu-ld: drivers/soc/rockchip/mfpwm.o: in function `mfpwm_reg_read':
/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:423: multiple definition of `mfpwm_reg_read'; drivers/pwm/pwm-rockchip-v4.o:/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:423: first defined here
aarch64-linux-gnu-ld: drivers/soc/rockchip/mfpwm.o: in function `mfpwm_reg_write':
/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:428: multiple definition of `mfpwm_reg_write'; drivers/pwm/pwm-rockchip-v4.o:/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:428: first defined here
make[3]: *** [../scripts/Makefile.vmlinux_o:72: vmlinux.o] Fehler 1


during the linking stage - with the driver as builtin


> +inline u32 mfpwm_reg_read(void __iomem *base, u32 reg)
> +{
> +	return readl(base + reg);
> +}
> +
> +inline void mfpwm_reg_write(void __iomem *base, u32 reg, u32 val)
> +{
> +	writel(val, base + reg);
> +}

making that a "static inline ..." solves that.


On a more general note, what is the differentiation to an MFD here?

Like you can already bind dt-nodes to MFD subdevices, and can implement
the exclusivity API thing on top of a general mfd device, to make sure only
one mfd-cell gets activated at one time.

Other than that, this looks like it reimplements MFDs?

Also handing around a regmap might be nicer, compared to readl/writel.


Heiko

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

[Nicolas Frattaroli]

On Saturday, 31 May 2025 23:48:29 Central European Summer Time Heiko Stübner wrote:
> Am Dienstag, 8. April 2025, 14:32:16 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>
> 
> actually trying to compile this, led me to
> 
> aarch64-linux-gnu-ld: drivers/soc/rockchip/mfpwm.o: in function `mfpwm_reg_read':
> /home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:423: multiple definition of `mfpwm_reg_read'; drivers/pwm/pwm-rockchip-v4.o:/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:423: first defined here
> aarch64-linux-gnu-ld: drivers/soc/rockchip/mfpwm.o: in function `mfpwm_reg_write':
> /home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:428: multiple definition of `mfpwm_reg_write'; drivers/pwm/pwm-rockchip-v4.o:/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:428: first defined here
> make[3]: *** [../scripts/Makefile.vmlinux_o:72: vmlinux.o] Fehler 1
> 
> 
> during the linking stage - with the driver as builtin
> 
> 
> > +inline u32 mfpwm_reg_read(void __iomem *base, u32 reg)
> > +{
> > +	return readl(base + reg);
> > +}
> > +
> > +inline void mfpwm_reg_write(void __iomem *base, u32 reg, u32 val)
> > +{
> > +	writel(val, base + reg);
> > +}
> 
> making that a "static inline ..." solves that.

Ack, will change

> 
> 
> On a more general note, what is the differentiation to an MFD here?
> 
> Like you can already bind dt-nodes to MFD subdevices, and can implement
> the exclusivity API thing on top of a general mfd device, to make sure only
> one mfd-cell gets activated at one time.
> 
> Other than that, this looks like it reimplements MFDs?

What initially made me not make this an MFD was Uwe Kleine-König expressing
some doubts, which lead me to alternatives like the auxiliary bus. Reading the
auxiliary bus docs I found:

  A key requirement for utilizing the auxiliary bus is that there is no
  dependency on a physical bus, device, register accesses or regmap support.
  These individual devices split from the core cannot live on the platform
  bus as they are not physical devices that are controlled by DT/ACPI. The
  same argument applies for not using MFD in this scenario as MFD relies on
  individual function devices being physical devices.

Additionally, LWN[1] about the auxiliary bus, which I've read up on during my
ill-fated journey into that version of the driver, also goes further into why
MFD is sometimes a bad fit:

  Linux already includes a number of drivers for multi-function devices. One
  of the ways to support them is the Multi-Function Devices (MFD) subsystem.
  It handles independent devices "glued" together into one hardware block
  which may contain some shared resources. MFD allows access to device
  registers either directly, or using a common bus. In this second case, it
  conveniently multiplexes accesses on Inter-Integrated Circuit (I2C) or
  Serial Peripheral Interface (SPI) buses. As the MFD sub-devices are
  separate, MFD drivers do not share a common state.

  The devices Ertman addresses do not fit well into the MFD model. Devices
  using the auxiliary bus provide subsets of the capabilities of a single
  hardware device. They do not expose separate register sets for each
  function; thus they cannot be described by devicetrees or discovered by
  ACPI. Their drivers need to share access to the hardware. Events concerning
  all sub-functionalities (like power management) need to be properly handled
  by all drivers.

The individual function devices may be all pointing at the same physical
device here, but they're not distinct parts of the device. However, there
still *is* a physical device, which convinced me that auxiliary bus wasn't
the right one either, and the idea for just using the platform bus came
during a work meeting. If someone with experience on aux bus vs platform bus
(what this uses) vs MFD, then feel free to chime in. Unfortunately, as is the
norm, I can't seem to find much in terms of MFD documentation. Needing to know
what type of exclusion they guarantee and what type of abstractions they bring
with them that would make them more useful than my solution would need some
justification in more than just an auto-generated header listing.

I am very inclined to start pretending things that aren't documented do
not actually exist in the kernel, because it's very annoying to have to
constantly deal with this.

> 
> Also handing around a regmap might be nicer, compared to readl/writel.

Strong disagree, adding error handling around every single register read
and write, and needing to always read into a variable rather than getting
the read value as a return value, made the drivers a lot uglier in a
previous iteration of this.

> 
> 
> Heiko
> 

Kind regards,
Nicolas Frattaroli

[1]: https://lwn.net/Articles/840416/

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

[Heiko Stübner]

Am Montag, 2. Juni 2025, 14:15:45 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> On Saturday, 31 May 2025 23:48:29 Central European Summer Time Heiko Stübner wrote:
> > Am Dienstag, 8. April 2025, 14:32:16 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:

> > On a more general note, what is the differentiation to an MFD here?
> > 
> > Like you can already bind dt-nodes to MFD subdevices, and can implement
> > the exclusivity API thing on top of a general mfd device, to make sure only
> > one mfd-cell gets activated at one time.
> > 
> > Other than that, this looks like it reimplements MFDs?
> 
> What initially made me not make this an MFD was Uwe Kleine-König expressing
> some doubts, which lead me to alternatives like the auxiliary bus. Reading the
> auxiliary bus docs I found:
> 
>   A key requirement for utilizing the auxiliary bus is that there is no
>   dependency on a physical bus, device, register accesses or regmap support.
>   These individual devices split from the core cannot live on the platform
>   bus as they are not physical devices that are controlled by DT/ACPI. The
>   same argument applies for not using MFD in this scenario as MFD relies on
>   individual function devices being physical devices.

Interestingly the 5 year old LWN article seems to have been overtaken by
real-world usage ;-) .

I see pinctrl/pinctrl-ep93xx.c using regmaps (and thus registers), similarly
in gpu/drm/bridge/ti-sn65dsi86.c and a number more.


> Additionally, LWN[1] about the auxiliary bus, which I've read up on during my
> ill-fated journey into that version of the driver, also goes further into why
> MFD is sometimes a bad fit:

[...] LWN excerpt [...]

> The individual function devices may be all pointing at the same physical
> device here, but they're not distinct parts of the device. However, there
> still *is* a physical device, which convinced me that auxiliary bus wasn't
> the right one either, and the idea for just using the platform bus came
> during a work meeting. If someone with experience on aux bus vs platform bus
> (what this uses) vs MFD, then feel free to chime in. Unfortunately, as is the
> norm, I can't seem to find much in terms of MFD documentation. Needing to know
> what type of exclusion they guarantee and what type of abstractions they bring
> with them that would make them more useful than my solution would need some
> justification in more than just an auto-generated header listing.

I think MFD itself does not provide any exclusivity - aka allowing definitions
that combinations of sub-devices cannot be used at the same time.

But as I see it right now, you have sort of a mfd-device in there, creating
all the sub-devices and then the aquire/release logic on top making sure
only one device is ever active at the same time.

Right now I really don't see (prone to code-blindness though) why the
aquire/release logic could not live in a mfd-device.


> I am very inclined to start pretending things that aren't documented do
> not actually exist in the kernel, because it's very annoying to have to
> constantly deal with this.

Sadly the "ostrich method" won't work ;-)

So as a way forward, I'd suggest you posting your v2, so that all the
current review comments get addressed and amending the
cover-letter with the aux-bux / mfd discussion thing (ideally in a
somewhat highlighed block so that people skimming along will notice)
and include the relevant people:

- for aux-bux get_maintainer.pl says:
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (maintainer:AUXILIARY BUS DRIVER)
Dave Ertman <david.m.ertman@intel.com> (reviewer:AUXILIARY BUS DRIVER)
Ira Weiny <ira.weiny@intel.com> (reviewer:AUXILIARY BUS DRIVER)
Leon Romanovsky <leon@kernel.org> (reviewer:AUXILIARY BUS DRIVER)

- and for MFD it's of course Lee:
Lee Jones <lee@kernel.org> (maintainer:MULTIFUNCTION DEVICES (MFD))


Heiko


> [1]: https://lwn.net/Articles/840416/