Add support for Allwinner PWM on D1/T113s/R329 SoCs

Allwinner's D1, T113-S3 and R329 SoCs have a new pwm controller witch is different from the previous pwm-sun4i.

The D1 and T113 are identical in terms of peripherals, they differ only in the architecture of the CPU core, and even share the majority of their DT. Because of that, using the same compatible makes sense.
The R329 is a different SoC though, and should have a different compatible string added, especially as there is a difference in the number of channels.

D1 and T113s SoCs have one PWM controller with 8 channels. R329 SoC has two PWM controllers in both power domains, one of them has 9 channels (CPUX one) and the other has 6 (CPUS one).

Add a device tree binding for them.

Reviewed-by Conor Dooley <[email protected]>

Signed-off-by Aleksandr Shubin <[email protected]>

---
 .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +35 -0

new file mode 100644
index 000000000000..4b25e94a8e46
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/allwinner,sun20i-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner D1, T113-S3 and R329 PWM
+
+maintainers:
+  - Aleksandr Shubin <[email protected]>
+  - Brandon Cheo Fusi <[email protected]>
+
+properties:
+  compatible:
+    oneOf:
+      - const: allwinner,sun20i-d1-pwm
+      - items:
+          - const: allwinner,sun50i-r329-pwm
+          - const: allwinner,sun20i-d1-pwm
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    items:
+      - description: Bus clock
+      - description: 24 MHz oscillator
+      - description: APB clock
+
+  clock-names:
+    items:
+      - const: bus

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +26 -0

+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+  - clocks
+  - clock-names
+  - resets
+
+examples:
+  - |
+    #include <dt-bindings/clock/sun20i-d1-ccu.h>
+    #include <dt-bindings/reset/sun20i-d1-ccu.h>
+
+    pwm: pwm@2000c00 {
+      compatible = "allwinner,sun20i-d1-pwm";
+      reg = <0x02000c00 0x400>;
+      clocks = <&ccu CLK_BUS_PWM>, <&dcxo>, <&ccu CLK_APB0>;
+      clock-names = "bus", "hosc", "apb";
+      resets = <&ccu RST_BUS_PWM>;
+      #pwm-cells = <0x3>;
+    };
+
+...

On Sun, 27 Apr 2025 17:24:53 +0300
Aleksandr Shubin <[email protected]> wrote:

Hi,

Quoted 65 lines

Allwinner's D1, T113-S3 and R329 SoCs have a new pwm controller witch is different from the previous pwm-sun4i.

The D1 and T113 are identical in terms of peripherals, they differ only in the architecture of the CPU core, and even share the majority of their DT. Because of that, using the same compatible makes sense.
The R329 is a different SoC though, and should have a different compatible string added, especially as there is a difference in the number of channels.

D1 and T113s SoCs have one PWM controller with 8 channels. R329 SoC has two PWM controllers in both power domains, one of them has 9 channels (CPUX one) and the other has 6 (CPUS one).

Add a device tree binding for them.

Reviewed-by Conor Dooley <[email protected]>

Signed-off-by Aleksandr Shubin <[email protected]>

---
 .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +35 -0

new file mode 100644
index 000000000000..4b25e94a8e46
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/allwinner,sun20i-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner D1, T113-S3 and R329 PWM
+
+maintainers:
+  - Aleksandr Shubin <[email protected]>
+  - Brandon Cheo Fusi <[email protected]>
+
+properties:
+  compatible:
+    oneOf:
+      - const: allwinner,sun20i-d1-pwm
+      - items:
+          - const: allwinner,sun50i-r329-pwm
+          - const: allwinner,sun20i-d1-pwm
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    items:
+      - description: Bus clock
+      - description: 24 MHz oscillator
+      - description: APB clock
+
+  clock-names:
+    items:
+      - const: bus

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml line 45

Quoted 10 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +10 -0

+      - const: hosc
+      - const: apb
+
+  resets:
+    maxItems: 1
+
+  allwinner,npwms:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: The number of PWM channels configured for this instance
+    enum: [6, 8, 9]

Do we really need to be so restrictive here? The IP has an "architectural" limit of 16 channels (due to the MMIO register layout

and status/control bits usage in some registers), so can't we just leave this value to be anything between 1 and 16 here? If people configure this wrongly, it's their fault, I'd say? Without confining this further based on the respective compatible strings this enum is less useful anyway, I think. The Allwinner A523 uses the same IP, and supports all 16 channels, the V853 implements 12, that's what I quickly found already, and there might be more examples in the future, so I'd rather open this up.

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml line 58

Quoted 13 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +13 -0

+
+allOf:
+  - $ref: pwm.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: allwinner,sun50i-r329-pwm
+
+    then:
+      required:
+        - allwinner,npwms

Can't we just simplify this by always requiring this property? As mentioned, there will be more SoCs with different values, so just omitting this for the D1 seems odd.

Cheers,
Andre

Quoted 16 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +16 -0

+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+  - clocks
+  - clock-names
+  - resets
+
+examples:
+  - |
+    #include <dt-bindings/clock/sun20i-d1-ccu.h>
+    #include <dt-bindings/reset/sun20i-d1-ccu.h>
+

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml line 84

Quoted 10 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +10 -0

+    pwm: pwm@2000c00 {
+      compatible = "allwinner,sun20i-d1-pwm";
+      reg = <0x02000c00 0x400>;
+      clocks = <&ccu CLK_BUS_PWM>, <&dcxo>, <&ccu CLK_APB0>;
+      clock-names = "bus", "hosc", "apb";
+      resets = <&ccu RST_BUS_PWM>;
+      #pwm-cells = <0x3>;
+    };
+
+...

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 83 lines

On Sun, 27 Apr 2025 17:24:53 +0300
Aleksandr Shubin <[email protected]> wrote:

Hi,

Quoted 65 lines

Allwinner's D1, T113-S3 and R329 SoCs have a new pwm controller witch is different from the previous pwm-sun4i.

The D1 and T113 are identical in terms of peripherals, they differ only in the architecture of the CPU core, and even share the majority of their DT. Because of that, using the same compatible makes sense.
The R329 is a different SoC though, and should have a different compatible string added, especially as there is a difference in the number of channels.

D1 and T113s SoCs have one PWM controller with 8 channels. R329 SoC has two PWM controllers in both power domains, one of them has 9 channels (CPUX one) and the other has 6 (CPUS one).

Add a device tree binding for them.

Reviewed-by Conor Dooley <[email protected]>

Signed-off-by Aleksandr Shubin <[email protected]>

---
 .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +35 -0

new file mode 100644
index 000000000000..4b25e94a8e46
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/allwinner,sun20i-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner D1, T113-S3 and R329 PWM
+
+maintainers:
+  - Aleksandr Shubin <[email protected]>
+  - Brandon Cheo Fusi <[email protected]>
+
+properties:
+  compatible:
+    oneOf:
+      - const: allwinner,sun20i-d1-pwm
+      - items:
+          - const: allwinner,sun50i-r329-pwm
+          - const: allwinner,sun20i-d1-pwm
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    items:
+      - description: Bus clock
+      - description: 24 MHz oscillator
+      - description: APB clock
+
+  clock-names:
+    items:
+      - const: bus

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml line 45

Quoted 10 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +10 -0

+      - const: hosc
+      - const: apb
+
+  resets:
+    maxItems: 1
+
+  allwinner,npwms:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: The number of PWM channels configured for this instance
+    enum: [6, 8, 9]  

Do we really need to be so restrictive here? The IP has an "architectural" limit of 16 channels (due to the MMIO register layout

Quoted 9 lines

and status/control bits usage in some registers), so can't we just leave this value to be anything between 1 and 16 here? If people configure this wrongly, it's their fault, I'd say? Without confining this further based on the respective compatible strings this enum is less useful anyway, I think. The Allwinner A523 uses the same IP, and supports all 16 channels, the V853 implements 12, that's what I quickly found already, and there might be more examples in the future, so I'd rather open this up.

Do we really need this property? I feel like the number of PWM channels should be something the driver could infer from the compatible string as we are going to have one compatible string per SoC anyway.

Quoted 55 lines

Quoted 4 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +3 -0

+
+allOf:
+  - $ref: pwm.yaml#

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml line 58

Quoted 10 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +10 -0

+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: allwinner,sun50i-r329-pwm
+
+    then:
+      required:
+        - allwinner,npwms  

Can't we just simplify this by always requiring this property? As mentioned, there will be more SoCs with different values, so just omitting this for the D1 seems odd.

Cheers,
Andre

Quoted 16 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +16 -0

+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+  - clocks
+  - clock-names
+  - resets
+
+examples:
+  - |
+    #include <dt-bindings/clock/sun20i-d1-ccu.h>
+    #include <dt-bindings/reset/sun20i-d1-ccu.h>
+

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml line 84

Quoted 10 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +10 -0

+    pwm: pwm@2000c00 {
+      compatible = "allwinner,sun20i-d1-pwm";
+      reg = <0x02000c00 0x400>;
+      clocks = <&ccu CLK_BUS_PWM>, <&dcxo>, <&ccu CLK_APB0>;
+      clock-names = "bus", "hosc", "apb";
+      resets = <&ccu RST_BUS_PWM>;
+      #pwm-cells = <0x3>;
+    };
+
+...  

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

On Thu, 19 Jun 2025 11:44:07 +0200
Alexandre Belloni <[email protected]> wrote:

Hi Alexandre,

Quoted 94 lines · Andre Przywara

Quoted 83 lines

On Sun, 27 Apr 2025 17:24:53 +0300
Aleksandr Shubin <[email protected]> wrote:

Hi,

Quoted 65 lines

Allwinner's D1, T113-S3 and R329 SoCs have a new pwm controller witch is different from the previous pwm-sun4i.

The D1 and T113 are identical in terms of peripherals, they differ only in the architecture of the CPU core, and even share the majority of their DT. Because of that, using the same compatible makes sense.
The R329 is a different SoC though, and should have a different compatible string added, especially as there is a difference in the number of channels.

D1 and T113s SoCs have one PWM controller with 8 channels. R329 SoC has two PWM controllers in both power domains, one of them has 9 channels (CPUX one) and the other has 6 (CPUS one).

Add a device tree binding for them.

Reviewed-by Conor Dooley <[email protected]>

Signed-off-by Aleksandr Shubin <[email protected]>

---
 .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +35 -0

new file mode 100644
index 000000000000..4b25e94a8e46
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/allwinner,sun20i-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner D1, T113-S3 and R329 PWM
+
+maintainers:
+  - Aleksandr Shubin <[email protected]>
+  - Brandon Cheo Fusi <[email protected]>
+
+properties:
+  compatible:
+    oneOf:
+      - const: allwinner,sun20i-d1-pwm
+      - items:
+          - const: allwinner,sun50i-r329-pwm
+          - const: allwinner,sun20i-d1-pwm
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    items:
+      - description: Bus clock
+      - description: 24 MHz oscillator
+      - description: APB clock
+
+  clock-names:
+    items:
+      - const: bus

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml line 45

Quoted 10 lines

Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +10 -0

+      - const: hosc
+      - const: apb
+
+  resets:
+    maxItems: 1
+
+  allwinner,npwms:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: The number of PWM channels configured for this instance
+    enum: [6, 8, 9]  

Do we really need to be so restrictive here? The IP has an "architectural" limit of 16 channels (due to the MMIO register layout

Quoted 9 lines

and status/control bits usage in some registers), so can't we just leave this value to be anything between 1 and 16 here? If people configure this wrongly, it's their fault, I'd say? Without confining this further based on the respective compatible strings this enum is less useful anyway, I think. The Allwinner A523 uses the same IP, and supports all 16 channels, the V853 implements 12, that's what I quickly found already, and there might be more examples in the future, so I'd rather open this up.

Quoted 3 lines

Do we really need this property? I feel like the number of PWM channels should be something the driver could infer from the compatible string as we are going to have one compatible string per SoC anyway.

Well yes, this would work, but I feel like it creates unnecessary churn to touch the driver every time some new SoC with the same IP pops up, and where just the number of channels is different - see above for a list of SoCs we already know about, and there are more in the pipe. It also means stable kernels would already work.

Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM controllers with ones supported by pwm-sun4i driver.

This patch adds a PWM controller driver for Allwinner's D1, T113-S3 and R329 SoCs. The main difference between these SoCs is the number of channels defined by the DT property.

Co-developed-by Brandon Cheo Fusi <[email protected]>

Signed-off-by Brandon Cheo Fusi <[email protected]>

Signed-off-by Aleksandr Shubin <[email protected]>

---
 drivers/pwm/Kconfig      |  10 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-sun20i.c | 379 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 390 insertions(+)
 create mode 100644 drivers/pwm/pwm-sun20i.c

drivers/pwm/Kconfig +10 -0

index 4731d5b90d7e..230a5561385b 100644
@@ -662,6 +662,16 @@ config PWM_SUN4I
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sun4i.
 
+config PWM_SUN20I
+	tristate "Allwinner D1/T113s/R329 PWM support"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	depends on COMMON_CLK
+	help
+	  Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sun20i.
+
 config PWM_SUNPLUS
 	tristate "Sunplus PWM support"
 	depends on ARCH_SUNPLUS || COMPILE_TEST

drivers/pwm/Makefile +1 -0

index 539e0def3f82..ef7515d2f974 100644
@@ -61,6 +61,7 @@ obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
 obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
 obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
+obj-$(CONFIG_PWM_SUN20I)	+= pwm-sun20i.o
 obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o

drivers/pwm/pwm-sun20i.c new file +205 -0

new file mode 100644
index 000000000000..22486617f1ef
@@ -0,0 +1,379 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
+ *
+ * Limitations:
+ * - When the parameters change, the current running period is not completed
+ *   and new settings are applied immediately.
+ * - The PWM output goes to a HIGH-Z state when the channel is disabled.
+ * - Changing the clock configuration (SUN20I_PWM_CLK_CFG)
+ *   may cause a brief output glitch.
+ *
+ * Copyright (c) 2023 Aleksandr Shubin <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+
+#define SUN20I_PWM_CLK_CFG(pair)		(0x20 + ((pair) * 0x4))
+#define SUN20I_PWM_CLK_CFG_SRC			GENMASK(8, 7)
+#define SUN20I_PWM_CLK_CFG_DIV_M		GENMASK(3, 0)
+#define SUN20I_PWM_CLK_DIV_M_MAX		8
+
+#define SUN20I_PWM_CLK_GATE			0x40
+#define SUN20I_PWM_CLK_GATE_BYPASS(chan)	BIT((chan) + 16)
+#define SUN20I_PWM_CLK_GATE_GATING(chan)	BIT(chan)
+
+#define SUN20I_PWM_ENABLE			0x80
+#define SUN20I_PWM_ENABLE_EN(chan)		BIT(chan)
+
+#define SUN20I_PWM_CTL(chan)			(0x100 + (chan) * 0x20)
+#define SUN20I_PWM_CTL_ACT_STA			BIT(8)
+#define SUN20I_PWM_CTL_PRESCAL_K		GENMASK(7, 0)
+#define SUN20I_PWM_CTL_PRESCAL_K_MAX		field_max(SUN20I_PWM_CTL_PRESCAL_K)
+
+#define SUN20I_PWM_PERIOD(chan)			(0x104 + (chan) * 0x20)
+#define SUN20I_PWM_PERIOD_ENTIRE_CYCLE		GENMASK(31, 16)
+#define SUN20I_PWM_PERIOD_ACT_CYCLE		GENMASK(15, 0)
+
+#define SUN20I_PWM_PCNTR_SIZE			BIT(16)
+
+/*
+ * SUN20I_PWM_MAGIC is used to quickly compute the values of the clock dividers
+ * div_m (SUN20I_PWM_CLK_CFG_DIV_M) & prescale_k (SUN20I_PWM_CTL_PRESCAL_K)
+ * without using a loop. These dividers limit the # of cycles in a period
+ * to SUN20I_PWM_PCNTR_SIZE (65536) by applying a scaling factor of
+ * 1/(div_m * (prescale_k + 1)) to the clock source.
+ *
+ * SUN20I_PWM_MAGIC is derived by solving for div_m and prescale_k
+ * such that for a given requested period,
+ *
+ * i) div_m is minimized for any prescale_k ≤ SUN20I_PWM_CTL_PRESCAL_K_MAX,
+ * ii) prescale_k is minimized.
+ *
+ * The derivation proceeds as follows, with val = # of cycles for requested
+ * period:
+ *
+ * for a given value of div_m we want the smallest prescale_k such that
+ *
+ * (val >> div_m) // (prescale_k + 1) ≤ 65536 (= SUN20I_PWM_PCNTR_SIZE)
+ *
+ * This is equivalent to:
+ *
+ * (val >> div_m) ≤ 65536 * (prescale_k + 1) + prescale_k
+ * ⟺ (val >> div_m) ≤ 65537 * prescale_k + 65536
+ * ⟺ (val >> div_m) - 65536 ≤ 65537 * prescale_k
+ * ⟺ ((val >> div_m) - 65536) / 65537 ≤ prescale_k
+ *
+ * As prescale_k is integer, this becomes
+ *
+ * ((val >> div_m) - 65536) // 65537 ≤ prescale_k
+ *
+ * And is minimized at
+ *
+ * ((val >> div_m) - 65536) // 65537
+ *
+ * Now we pick the smallest div_m that satifies prescale_k ≤ 255
+ * (i.e SUN20I_PWM_CTL_PRESCAL_K_MAX),
+ *
+ * ((val >> div_m) - 65536) // 65537 ≤ 255
+ * ⟺ (val >> div_m) - 65536 ≤ 255 * 65537 + 65536
+ * ⟺ val >> div_m ≤ 255 * 65537 + 2 * 65536
+ * ⟺ val >> div_m < (255 * 65537 + 2 * 65536 + 1)
+ * ⟺ div_m = fls((val) / (255 * 65537 + 2 * 65536 + 1))
+ *
+ * Suggested by Uwe Kleine-König
+ */
+#define SUN20I_PWM_MAGIC			(255 * 65537 + 2 * 65536 + 1)
+#define SUN20I_PWM_DIV_CONST			65537
+
+struct sun20i_pwm_chip {
+	struct clk *clk_hosc, *clk_apb;
+	void __iomem *base;
+};
+
+static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static inline u32 sun20i_pwm_readl(struct sun20i_pwm_chip *chip,
+				   unsigned long offset)
+{
+	return readl(chip->base + offset);
+}
+
+static inline void sun20i_pwm_writel(struct sun20i_pwm_chip *chip,
+				     u32 val, unsigned long offset)
+{
+	writel(val, chip->base + offset);
+}
+
+static int sun20i_pwm_get_state(struct pwm_chip *chip,
+				struct pwm_device *pwm,
+				struct pwm_state *state)
+{
+	struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
+	u16 ent_cycle, act_cycle, prescale_k;
+	u64 clk_rate, tmp;
+	u8 div_m;
+	u32 val;
+
+	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm / 2));
+	div_m = FIELD_GET(SUN20I_PWM_CLK_CFG_DIV_M, val);
+	if (div_m > SUN20I_PWM_CLK_DIV_M_MAX)
+		div_m = SUN20I_PWM_CLK_DIV_M_MAX;
+
+	/*
+	 * If CLK_CFG_SRC is 0, use the hosc clock;
+	 * otherwise (any nonzero value) use the APB clock.
+	 */
+	if (FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, val) == 0)
+		clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
+	else
+		clk_rate = clk_get_rate(sun20i_chip->clk_apb);
+
+	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
+	state->polarity = (SUN20I_PWM_CTL_ACT_STA & val) ?
+			   PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
+
+	prescale_k = FIELD_GET(SUN20I_PWM_CTL_PRESCAL_K, val) + 1;
+
+	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
+	state->enabled = (SUN20I_PWM_ENABLE_EN(pwm->hwpwm) & val) ? true : false;
+
+	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_PERIOD(pwm->hwpwm));
+	act_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ACT_CYCLE, val);
+
+	ent_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, val);
+
+	/*
+	 * The duration of the active phase should not be longer
+	 * than the duration of the period
+	 */
+	if (act_cycle > ent_cycle)
+		act_cycle = ent_cycle;
+
+	/*
+	 * We have act_cycle <= ent_cycle <= 0xffff, prescale_k <= 0x100,
+	 * div_m <= 8. So the multiplication fits into an u64 without
+	 * overflow.
+	 */
+	tmp = ((u64)(act_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
+	state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
+	tmp = ((u64)(ent_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
+	state->period = DIV_ROUND_UP_ULL(tmp, clk_rate);
+
+	return 0;
+}
+
+static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
+	u64 bus_rate, hosc_rate, val, ent_cycle, act_cycle;
+	u32 clk_gate, clk_cfg, pwm_en, ctl, reg_period;
+	u32 prescale_k, div_m;
+	bool use_bus_clk;
+
+	pwm_en = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
+	clk_gate = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_GATE);
+
+	if (!state->enabled) {
+		if (state->enabled != pwm->state.enabled) {
+			clk_gate &= ~SUN20I_PWM_CLK_GATE_GATING(pwm->hwpwm);
+			pwm_en &= ~SUN20I_PWM_ENABLE_EN(pwm->hwpwm);
+			sun20i_pwm_writel(sun20i_chip, pwm_en, SUN20I_PWM_ENABLE);
+			sun20i_pwm_writel(sun20i_chip, clk_gate, SUN20I_PWM_CLK_GATE);
+		}
+		return 0;
+	}
+
+	ctl = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
+	clk_cfg = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm / 2));
+	hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
+	bus_rate = clk_get_rate(sun20i_chip->clk_apb);
+	if (pwm_en & SUN20I_PWM_ENABLE_EN(pwm->hwpwm ^ 1)) {
+		/* If the neighbor channel is enabled, use the current clock settings */
+		use_bus_clk = FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, clk_cfg) != 0;

drivers/pwm/pwm-sun20i.c new file +80 -0

+			use_bus_clk = true;
+			val = mul_u64_u64_div_u64(state->period, bus_rate, NSEC_PER_SEC);
+			if (val <= 1)
+				return -EINVAL;
+		}
+		div_m = fls(DIV_ROUND_DOWN_ULL(val, SUN20I_PWM_MAGIC));
+		if (div_m > SUN20I_PWM_CLK_DIV_M_MAX)
+			return -EINVAL;
+
+		/* Set up the CLK_DIV_M and clock CLK_SRC */
+		clk_cfg = FIELD_PREP(SUN20I_PWM_CLK_CFG_DIV_M, div_m);
+		clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_SRC, use_bus_clk);
+
+		sun20i_pwm_writel(sun20i_chip, clk_cfg, SUN20I_PWM_CLK_CFG(pwm->hwpwm / 2));
+	}
+
+	/* Calculate prescale_k and determine the number of cycles for a full PWM period */
+	ent_cycle = val >> div_m;
+	prescale_k = DIV_ROUND_DOWN_ULL(ent_cycle, SUN20I_PWM_DIV_CONST);
+	if (prescale_k > SUN20I_PWM_CTL_PRESCAL_K_MAX)
+		prescale_k = SUN20I_PWM_CTL_PRESCAL_K_MAX;
+
+	do_div(ent_cycle, prescale_k + 1);
+
+	/* ent_cycle must not be zero */
+	if (ent_cycle == 0)
+		return -EINVAL;
+
+	/* For N cycles, PPRx.PWM_ENTIRE_CYCLE = (N-1) */
+	reg_period = FIELD_PREP(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, ent_cycle - 1);
+
+	/* Calculate the active cycles (duty cycle) */
+	val = mul_u64_u64_div_u64(state->duty_cycle,
+				  (use_bus_clk ? bus_rate : hosc_rate),
+				  NSEC_PER_SEC);
+	act_cycle = val >> div_m;
+	do_div(act_cycle, prescale_k + 1);
+
+	/*
+	 * The formula of the output period and the duty-cycle for PWM are as follows.
+	 * T period = PWM0_PRESCALE_K / PWM01_CLK * (PPR0.PWM_ENTIRE_CYCLE + 1)
+	 * T high-level = PWM0_PRESCALE_K / PWM01_CLK * PPR0.PWM_ACT_CYCLE
+	 * Duty-cycle = T high-level / T period
+	 */
+	reg_period |= FIELD_PREP(SUN20I_PWM_PERIOD_ACT_CYCLE, act_cycle);
+	sun20i_pwm_writel(sun20i_chip, reg_period, SUN20I_PWM_PERIOD(pwm->hwpwm));
+
+	ctl = FIELD_PREP(SUN20I_PWM_CTL_PRESCAL_K, prescale_k);
+	if (state->polarity == PWM_POLARITY_NORMAL)
+		ctl |= SUN20I_PWM_CTL_ACT_STA;
+
+	sun20i_pwm_writel(sun20i_chip, ctl, SUN20I_PWM_CTL(pwm->hwpwm));
+
+	if (state->enabled != pwm->state.enabled) {
+		clk_gate &= ~SUN20I_PWM_CLK_GATE_BYPASS(pwm->hwpwm);
+		clk_gate |= SUN20I_PWM_CLK_GATE_GATING(pwm->hwpwm);
+		pwm_en |= SUN20I_PWM_ENABLE_EN(pwm->hwpwm);
+		sun20i_pwm_writel(sun20i_chip, pwm_en, SUN20I_PWM_ENABLE);
+		sun20i_pwm_writel(sun20i_chip, clk_gate, SUN20I_PWM_CLK_GATE);
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops sun20i_pwm_ops = {
+	.apply = sun20i_pwm_apply,
+	.get_state = sun20i_pwm_get_state,
+};
+
+static const struct of_device_id sun20i_pwm_dt_ids[] = {
+	{ .compatible = "allwinner,sun20i-d1-pwm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sun20i_pwm_dt_ids);
+
+static int sun20i_pwm_probe(struct platform_device *pdev)
+{
+	struct pwm_chip *chip;
+	struct sun20i_pwm_chip *sun20i_chip;
+	struct clk *clk_bus;

drivers/pwm/pwm-sun20i.c new file +13 -0

+		npwm = 16;
+	}
+
+	chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*sun20i_chip));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	sun20i_chip = to_sun20i_pwm_chip(chip);
+
+	sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(sun20i_chip->base))
+		return PTR_ERR(sun20i_chip->base);
+
+	clk_bus = devm_clk_get_enabled(&pdev->dev, "bus");

drivers/pwm/pwm-sun20i.c new file +27 -0

+
+	rst = devm_reset_control_get_exclusive_deasserted(&pdev->dev, NULL);
+	if (IS_ERR(rst))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rst),
+				     "Failed to get reset control\n");
+
+	chip->ops = &sun20i_pwm_ops;
+
+	ret = devm_pwmchip_add(&pdev->dev, chip);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "Failed to add PWM chip\n");
+
+	return 0;
+}
+
+static struct platform_driver sun20i_pwm_driver = {
+	.driver = {
+		.name = "sun20i-pwm",
+		.of_match_table = sun20i_pwm_dt_ids,
+	},
+	.probe = sun20i_pwm_probe,
+};
+module_platform_driver(sun20i_pwm_driver);
+
+MODULE_AUTHOR("Aleksandr Shubin <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun20i PWM driver");
+MODULE_LICENSE("GPL");

On Sun, 27 Apr 2025 17:24:54 +0300
Aleksandr Shubin <[email protected]> wrote:

Hi Aleksandr,

thanks for your patience and persistence with this series! It seems like some people have some motivation now for getting mainline, so hopefully we can use this momentum now. Also the "new" Allwinner A523 uses the same IP as the D1, just with all 16 channels.

Quoted 261 lines

Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM controllers with ones supported by pwm-sun4i driver.

This patch adds a PWM controller driver for Allwinner's D1, T113-S3 and R329 SoCs. The main difference between these SoCs is the number of channels defined by the DT property.

Co-developed-by Brandon Cheo Fusi <[email protected]>

Signed-off-by Brandon Cheo Fusi <[email protected]>

Signed-off-by Aleksandr Shubin <[email protected]>

---
 drivers/pwm/Kconfig      |  10 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-sun20i.c | 379 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 390 insertions(+)
 create mode 100644 drivers/pwm/pwm-sun20i.c

drivers/pwm/Kconfig +10 -0

index 4731d5b90d7e..230a5561385b 100644
@@ -662,6 +662,16 @@ config PWM_SUN4I
        To compile this driver as a module, choose M here: the module
        will be called pwm-sun4i.
 
+config PWM_SUN20I
+     tristate "Allwinner D1/T113s/R329 PWM support"
+     depends on ARCH_SUNXI || COMPILE_TEST
+     depends on COMMON_CLK
+     help
+       Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs.
+
+       To compile this driver as a module, choose M here: the module
+       will be called pwm-sun20i.
+
 config PWM_SUNPLUS
      tristate "Sunplus PWM support"
      depends on ARCH_SUNPLUS || COMPILE_TEST

drivers/pwm/Makefile +1 -0

index 539e0def3f82..ef7515d2f974 100644
@@ -61,6 +61,7 @@ obj-$(CONFIG_PWM_STM32)             += pwm-stm32.o
 obj-$(CONFIG_PWM_STM32_LP)   += pwm-stm32-lp.o
 obj-$(CONFIG_PWM_STMPE)              += pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)              += pwm-sun4i.o
+obj-$(CONFIG_PWM_SUN20I)     += pwm-sun20i.o
 obj-$(CONFIG_PWM_SUNPLUS)    += pwm-sunplus.o
 obj-$(CONFIG_PWM_TEGRA)              += pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)     += pwm-tiecap.o

drivers/pwm/pwm-sun20i.c new file +205 -0

new file mode 100644
index 000000000000..22486617f1ef
@@ -0,0 +1,379 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
+ *
+ * Limitations:
+ * - When the parameters change, the current running period is not completed
+ *   and new settings are applied immediately.
+ * - The PWM output goes to a HIGH-Z state when the channel is disabled.
+ * - Changing the clock configuration (SUN20I_PWM_CLK_CFG)
+ *   may cause a brief output glitch.
+ *
+ * Copyright (c) 2023 Aleksandr Shubin <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+
+#define SUN20I_PWM_CLK_CFG(pair)             (0x20 + ((pair) * 0x4))
+#define SUN20I_PWM_CLK_CFG_SRC                       GENMASK(8, 7)
+#define SUN20I_PWM_CLK_CFG_DIV_M             GENMASK(3, 0)
+#define SUN20I_PWM_CLK_DIV_M_MAX             8
+
+#define SUN20I_PWM_CLK_GATE                  0x40
+#define SUN20I_PWM_CLK_GATE_BYPASS(chan)     BIT((chan) + 16)
+#define SUN20I_PWM_CLK_GATE_GATING(chan)     BIT(chan)
+
+#define SUN20I_PWM_ENABLE                    0x80
+#define SUN20I_PWM_ENABLE_EN(chan)           BIT(chan)
+
+#define SUN20I_PWM_CTL(chan)                 (0x100 + (chan) * 0x20)
+#define SUN20I_PWM_CTL_ACT_STA                       BIT(8)
+#define SUN20I_PWM_CTL_PRESCAL_K             GENMASK(7, 0)
+#define SUN20I_PWM_CTL_PRESCAL_K_MAX         field_max(SUN20I_PWM_CTL_PRESCAL_K)
+
+#define SUN20I_PWM_PERIOD(chan)                      (0x104 + (chan) * 0x20)
+#define SUN20I_PWM_PERIOD_ENTIRE_CYCLE               GENMASK(31, 16)
+#define SUN20I_PWM_PERIOD_ACT_CYCLE          GENMASK(15, 0)
+
+#define SUN20I_PWM_PCNTR_SIZE                        BIT(16)
+
+/*
+ * SUN20I_PWM_MAGIC is used to quickly compute the values of the clock dividers
+ * div_m (SUN20I_PWM_CLK_CFG_DIV_M) & prescale_k (SUN20I_PWM_CTL_PRESCAL_K)
+ * without using a loop. These dividers limit the # of cycles in a period
+ * to SUN20I_PWM_PCNTR_SIZE (65536) by applying a scaling factor of
+ * 1/(div_m * (prescale_k + 1)) to the clock source.
+ *
+ * SUN20I_PWM_MAGIC is derived by solving for div_m and prescale_k
+ * such that for a given requested period,
+ *
+ * i) div_m is minimized for any prescale_k ≤ SUN20I_PWM_CTL_PRESCAL_K_MAX,
+ * ii) prescale_k is minimized.
+ *
+ * The derivation proceeds as follows, with val = # of cycles for requested
+ * period:
+ *
+ * for a given value of div_m we want the smallest prescale_k such that
+ *
+ * (val >> div_m) // (prescale_k + 1) ≤ 65536 (= SUN20I_PWM_PCNTR_SIZE)
+ *
+ * This is equivalent to:
+ *
+ * (val >> div_m) ≤ 65536 * (prescale_k + 1) + prescale_k
+ * ⟺ (val >> div_m) ≤ 65537 * prescale_k + 65536
+ * ⟺ (val >> div_m) - 65536 ≤ 65537 * prescale_k
+ * ⟺ ((val >> div_m) - 65536) / 65537 ≤ prescale_k
+ *
+ * As prescale_k is integer, this becomes
+ *
+ * ((val >> div_m) - 65536) // 65537 ≤ prescale_k
+ *
+ * And is minimized at
+ *
+ * ((val >> div_m) - 65536) // 65537
+ *
+ * Now we pick the smallest div_m that satifies prescale_k ≤ 255
+ * (i.e SUN20I_PWM_CTL_PRESCAL_K_MAX),
+ *
+ * ((val >> div_m) - 65536) // 65537 ≤ 255
+ * ⟺ (val >> div_m) - 65536 ≤ 255 * 65537 + 65536
+ * ⟺ val >> div_m ≤ 255 * 65537 + 2 * 65536
+ * ⟺ val >> div_m < (255 * 65537 + 2 * 65536 + 1)
+ * ⟺ div_m = fls((val) / (255 * 65537 + 2 * 65536 + 1))
+ *
+ * Suggested by Uwe Kleine-König
+ */
+#define SUN20I_PWM_MAGIC                     (255 * 65537 + 2 * 65536 + 1)
+#define SUN20I_PWM_DIV_CONST                 65537
+
+struct sun20i_pwm_chip {
+     struct clk *clk_hosc, *clk_apb;
+     void __iomem *base;
+};
+
+static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
+{
+     return pwmchip_get_drvdata(chip);
+}
+
+static inline u32 sun20i_pwm_readl(struct sun20i_pwm_chip *chip,
+                                unsigned long offset)
+{
+     return readl(chip->base + offset);
+}
+
+static inline void sun20i_pwm_writel(struct sun20i_pwm_chip *chip,
+                                  u32 val, unsigned long offset)
+{
+     writel(val, chip->base + offset);
+}
+
+static int sun20i_pwm_get_state(struct pwm_chip *chip,
+                             struct pwm_device *pwm,
+                             struct pwm_state *state)
+{
+     struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
+     u16 ent_cycle, act_cycle, prescale_k;
+     u64 clk_rate, tmp;
+     u8 div_m;
+     u32 val;
+
+     val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm / 2));
+     div_m = FIELD_GET(SUN20I_PWM_CLK_CFG_DIV_M, val);
+     if (div_m > SUN20I_PWM_CLK_DIV_M_MAX)
+             div_m = SUN20I_PWM_CLK_DIV_M_MAX;
+
+     /*
+      * If CLK_CFG_SRC is 0, use the hosc clock;
+      * otherwise (any nonzero value) use the APB clock.
+      */
+     if (FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, val) == 0)
+             clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
+     else
+             clk_rate = clk_get_rate(sun20i_chip->clk_apb);
+
+     val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
+     state->polarity = (SUN20I_PWM_CTL_ACT_STA & val) ?
+                        PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
+
+     prescale_k = FIELD_GET(SUN20I_PWM_CTL_PRESCAL_K, val) + 1;
+
+     val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
+     state->enabled = (SUN20I_PWM_ENABLE_EN(pwm->hwpwm) & val) ? true : false;
+
+     val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_PERIOD(pwm->hwpwm));
+     act_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ACT_CYCLE, val);
+
+     ent_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, val);
+
+     /*
+      * The duration of the active phase should not be longer
+      * than the duration of the period
+      */
+     if (act_cycle > ent_cycle)
+             act_cycle = ent_cycle;
+
+     /*
+      * We have act_cycle <= ent_cycle <= 0xffff, prescale_k <= 0x100,
+      * div_m <= 8. So the multiplication fits into an u64 without
+      * overflow.
+      */
+     tmp = ((u64)(act_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
+     state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
+     tmp = ((u64)(ent_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
+     state->period = DIV_ROUND_UP_ULL(tmp, clk_rate);
+
+     return 0;
+}
+
+static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+                         const struct pwm_state *state)
+{
+     struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
+     u64 bus_rate, hosc_rate, val, ent_cycle, act_cycle;
+     u32 clk_gate, clk_cfg, pwm_en, ctl, reg_period;
+     u32 prescale_k, div_m;
+     bool use_bus_clk;
+
+     pwm_en = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
+     clk_gate = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_GATE);
+
+     if (!state->enabled) {
+             if (state->enabled != pwm->state.enabled) {
+                     clk_gate &= ~SUN20I_PWM_CLK_GATE_GATING(pwm->hwpwm);
+                     pwm_en &= ~SUN20I_PWM_ENABLE_EN(pwm->hwpwm);
+                     sun20i_pwm_writel(sun20i_chip, pwm_en, SUN20I_PWM_ENABLE);
+                     sun20i_pwm_writel(sun20i_chip, clk_gate, SUN20I_PWM_CLK_GATE);
+             }
+             return 0;
+     }
+
+     ctl = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
+     clk_cfg = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm / 2));
+     hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
+     bus_rate = clk_get_rate(sun20i_chip->clk_apb);
+     if (pwm_en & SUN20I_PWM_ENABLE_EN(pwm->hwpwm ^ 1)) {
+             /* If the neighbor channel is enabled, use the current clock settings */
+             use_bus_clk = FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, clk_cfg) != 0;

drivers/pwm/pwm-sun20i.c line 215

Quoted 10 lines

drivers/pwm/pwm-sun20i.c new file +10 -0

+             val = mul_u64_u64_div_u64(state->period,
+                                       (use_bus_clk ? bus_rate : hosc_rate),
+                                       NSEC_PER_SEC);
+
+             div_m = FIELD_GET(SUN20I_PWM_CLK_CFG_DIV_M, clk_cfg);
+     } else {
+             /*
+              * Select the clock source based on the period.
+              * Since bus_rate > hosc_rate, which means bus_rate
+              * can provide a higher frequency than hosc_rate.

What does that mean, exactly? That we should prefer the faster clock? Because that's not what the code does, is it?

drivers/pwm/pwm-sun20i.c line 223

Quoted 8 lines

drivers/pwm/pwm-sun20i.c new file +8 -0

+              */
+             use_bus_clk = false;
+             val = mul_u64_u64_div_u64(state->period, hosc_rate, NSEC_PER_SEC);
+             /*
+              * If the calculated value is ≤ 1, the period is too short
+              * for proper PWM operation
+              */
+             if (val <= 1) {

So if I get the code correctly, it prefers HOSC over APB? Is that really the best way? Shouldn't it be the other way around: we use the faster clock, since this will not limit the sibling channel?

And another thing to consider are rounding errors due to integer
division: certain period rates might be better achievable with one or the other source clock: 3 MHz works best as 24MHz/8, 3.125MHz as 100MHz/32.
So shall we calculate the values and compare the errors instead? Oh, and also we need to consider bypassing, I feel like this should be checked first.

In any case I think there should be a comment describing the strategy and give some rationale, I think.

Quoted 81 lines

drivers/pwm/pwm-sun20i.c new file +80 -0

+                     use_bus_clk = true;
+                     val = mul_u64_u64_div_u64(state->period, bus_rate, NSEC_PER_SEC);
+                     if (val <= 1)
+                             return -EINVAL;
+             }
+             div_m = fls(DIV_ROUND_DOWN_ULL(val, SUN20I_PWM_MAGIC));
+             if (div_m > SUN20I_PWM_CLK_DIV_M_MAX)
+                     return -EINVAL;
+
+             /* Set up the CLK_DIV_M and clock CLK_SRC */
+             clk_cfg = FIELD_PREP(SUN20I_PWM_CLK_CFG_DIV_M, div_m);
+             clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_SRC, use_bus_clk);
+
+             sun20i_pwm_writel(sun20i_chip, clk_cfg, SUN20I_PWM_CLK_CFG(pwm->hwpwm / 2));
+     }
+
+     /* Calculate prescale_k and determine the number of cycles for a full PWM period */
+     ent_cycle = val >> div_m;
+     prescale_k = DIV_ROUND_DOWN_ULL(ent_cycle, SUN20I_PWM_DIV_CONST);
+     if (prescale_k > SUN20I_PWM_CTL_PRESCAL_K_MAX)
+             prescale_k = SUN20I_PWM_CTL_PRESCAL_K_MAX;
+
+     do_div(ent_cycle, prescale_k + 1);
+
+     /* ent_cycle must not be zero */
+     if (ent_cycle == 0)
+             return -EINVAL;
+
+     /* For N cycles, PPRx.PWM_ENTIRE_CYCLE = (N-1) */
+     reg_period = FIELD_PREP(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, ent_cycle - 1);
+
+     /* Calculate the active cycles (duty cycle) */
+     val = mul_u64_u64_div_u64(state->duty_cycle,
+                               (use_bus_clk ? bus_rate : hosc_rate),
+                               NSEC_PER_SEC);
+     act_cycle = val >> div_m;
+     do_div(act_cycle, prescale_k + 1);
+
+     /*
+      * The formula of the output period and the duty-cycle for PWM are as follows.
+      * T period = PWM0_PRESCALE_K / PWM01_CLK * (PPR0.PWM_ENTIRE_CYCLE + 1)
+      * T high-level = PWM0_PRESCALE_K / PWM01_CLK * PPR0.PWM_ACT_CYCLE
+      * Duty-cycle = T high-level / T period
+      */
+     reg_period |= FIELD_PREP(SUN20I_PWM_PERIOD_ACT_CYCLE, act_cycle);
+     sun20i_pwm_writel(sun20i_chip, reg_period, SUN20I_PWM_PERIOD(pwm->hwpwm));
+
+     ctl = FIELD_PREP(SUN20I_PWM_CTL_PRESCAL_K, prescale_k);
+     if (state->polarity == PWM_POLARITY_NORMAL)
+             ctl |= SUN20I_PWM_CTL_ACT_STA;
+
+     sun20i_pwm_writel(sun20i_chip, ctl, SUN20I_PWM_CTL(pwm->hwpwm));
+
+     if (state->enabled != pwm->state.enabled) {
+             clk_gate &= ~SUN20I_PWM_CLK_GATE_BYPASS(pwm->hwpwm);
+             clk_gate |= SUN20I_PWM_CLK_GATE_GATING(pwm->hwpwm);
+             pwm_en |= SUN20I_PWM_ENABLE_EN(pwm->hwpwm);
+             sun20i_pwm_writel(sun20i_chip, pwm_en, SUN20I_PWM_ENABLE);
+             sun20i_pwm_writel(sun20i_chip, clk_gate, SUN20I_PWM_CLK_GATE);
+     }
+
+     return 0;
+}
+
+static const struct pwm_ops sun20i_pwm_ops = {
+     .apply = sun20i_pwm_apply,
+     .get_state = sun20i_pwm_get_state,
+};
+
+static const struct of_device_id sun20i_pwm_dt_ids[] = {
+     { .compatible = "allwinner,sun20i-d1-pwm" },
+     { }
+};
+MODULE_DEVICE_TABLE(of, sun20i_pwm_dt_ids);
+
+static int sun20i_pwm_probe(struct platform_device *pdev)
+{
+     struct pwm_chip *chip;
+     struct sun20i_pwm_chip *sun20i_chip;
+     struct clk *clk_bus;

drivers/pwm/pwm-sun20i.c line 313

Quoted 10 lines

drivers/pwm/pwm-sun20i.c new file +10 -0

+     struct reset_control *rst;
+     u32 npwm;
+     int ret;
+
+     ret = of_property_read_u32(pdev->dev.of_node, "allwinner,npwms", &npwm);
+     if (ret < 0)
+             npwm = 8; /* Default value */
+
+     if (npwm > 16) {
+             dev_info(&pdev->dev, "Limiting number of PWM lines from %u to 16", npwm);

I don't think we should proceed if the firmware information is clearly wrong. Just bail out with -EINVAL or so here, so that gets fixed in the DT.

Quoted 13 lines

drivers/pwm/pwm-sun20i.c new file +13 -0

+             npwm = 16;
+     }
+
+     chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*sun20i_chip));
+     if (IS_ERR(chip))
+             return PTR_ERR(chip);
+     sun20i_chip = to_sun20i_pwm_chip(chip);
+
+     sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(sun20i_chip->base))
+             return PTR_ERR(sun20i_chip->base);
+
+     clk_bus = devm_clk_get_enabled(&pdev->dev, "bus");

drivers/pwm/pwm-sun20i.c line 336

Quoted 10 lines

drivers/pwm/pwm-sun20i.c new file +10 -0

+     if (IS_ERR(clk_bus))
+             return dev_err_probe(&pdev->dev, PTR_ERR(clk_bus),
+                                  "Failed to get bus clock\n");
+
+     sun20i_chip->clk_hosc = devm_clk_get_enabled(&pdev->dev, "hosc");
+     if (IS_ERR(sun20i_chip->clk_hosc))
+             return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_hosc),
+                                  "Failed to get hosc clock\n");
+
+     ret = devm_clk_rate_exclusive_get(&pdev->dev, sun20i_chip->clk_hosc);

Just ignoring for a bit that the 24 MHz oscillator is a fixed clock anyway, but why would we want exclusivity already at probe time? Isn't that too limiting, as no one might ever use any PWM channels, but it would still "belong to us"?

drivers/pwm/pwm-sun20i.c line 346

Quoted 10 lines

drivers/pwm/pwm-sun20i.c new file +10 -0

+     if (ret)
+             return dev_err_probe(&pdev->dev, ret,
+                                  "Failed to get hosc exclusive rate\n");
+
+     sun20i_chip->clk_apb = devm_clk_get_enabled(&pdev->dev, "apb");
+     if (IS_ERR(sun20i_chip->clk_apb))
+             return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_apb),
+                                  "Failed to get apb clock\n");
+
+     ret = devm_clk_rate_exclusive_get(&pdev->dev, sun20i_chip->clk_apb);

Just for the records: APB is practically also a fixed clock, set up once in firmware and never changed, since it drives a lot of other peripherals.
But same question as above, why do we lock its rate already here?

drivers/pwm/pwm-sun20i.c line 352

Quoted 7 lines

drivers/pwm/pwm-sun20i.c new file +6 -0

+     if (ret)
+             return dev_err_probe(&pdev->dev, ret,
+                                  "Failed to get apb exclusive rate\n");
+
+     if (clk_get_rate(sun20i_chip->clk_apb) <= clk_get_rate(sun20i_chip->clk_hosc))
+             dev_info(&pdev->dev, "APB clock must be greater than hosc clock");

Why this check? Does the code make any assumptions about this relation? If yes, we must surely deny this and bail out.
If not (and I feel we should handle it this way), we can just ignore this and not print anything.

Cheers,
Andre

Quoted 19 lines

drivers/pwm/pwm-sun20i.c new file +19 -0

+
+     rst = devm_reset_control_get_exclusive_deasserted(&pdev->dev, NULL);
+     if (IS_ERR(rst))
+             return dev_err_probe(&pdev->dev, PTR_ERR(rst),
+                                  "Failed to get reset control\n");
+
+     chip->ops = &sun20i_pwm_ops;
+
+     ret = devm_pwmchip_add(&pdev->dev, chip);
+     if (ret < 0)
+             return dev_err_probe(&pdev->dev, ret, "Failed to add PWM chip\n");
+
+     return 0;
+}
+
+static struct platform_driver sun20i_pwm_driver = {
+     .driver = {
+             .name = "sun20i-pwm",
+             .of_match_table = sun20i_pwm_dt_ids,

drivers/pwm/pwm-sun20i.c line 379

Quoted 8 lines

drivers/pwm/pwm-sun20i.c new file +8 -0

+     },
+     .probe = sun20i_pwm_probe,
+};
+module_platform_driver(sun20i_pwm_driver);
+
+MODULE_AUTHOR("Aleksandr Shubin <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun20i PWM driver");
+MODULE_LICENSE("GPL");

вт, 13 мая 2025 г. в 01:39, Andre Przywara <[email protected]>:

Quoted 24 lines

drivers/pwm/pwm-sun20i.c line 223

Quoted 8 lines · On Sun, 27 Apr 2025 17:24:54 +0300 Aleksandr Shubin <[email protected]>…

drivers/pwm/pwm-sun20i.c new file +8 -0

+              */
+             use_bus_clk = false;
+             val = mul_u64_u64_div_u64(state->period, hosc_rate, NSEC_PER_SEC);
+             /*
+              * If the calculated value is ≤ 1, the period is too short
+              * for proper PWM operation
+              */
+             if (val <= 1) {

So if I get the code correctly, it prefers HOSC over APB? Is that really the best way? Shouldn't it be the other way around: we use the faster clock, since this will not limit the sibling channel?

And another thing to consider are rounding errors due to integer
division: certain period rates might be better achievable with one or the other source clock: 3 MHz works best as 24MHz/8, 3.125MHz as 100MHz/32.
So shall we calculate the values and compare the errors instead? Oh, and also we need to consider bypassing, I feel like this should be checked first.

Quoted 2 lines

In any case I think there should be a comment describing the strategy and give some rationale, I think.

I like the idea of comparing the quantization error for each clock source (i.e. computing the actual period for both APB and HOSC and choosing whichever is closer to the requested period).
I can try to implement that error-minimization approach in the next series of patches and add a comment explaining the strategy.

Quoted 11 lines

Quoted 11 lines

drivers/pwm/pwm-sun20i.c new file +11 -0

+static int sun20i_pwm_probe(struct platform_device *pdev)
+{
+     struct pwm_chip *chip;
+     struct sun20i_pwm_chip *sun20i_chip;
+     struct clk *clk_bus;
+     struct reset_control *rst;
+     u32 npwm;
+     int ret;
+
+     ret = of_property_read_u32(pdev->dev.of_node, "allwinner,npwms", &npwm);
+     if (ret < 0)

Quoted 8 lines

drivers/pwm/pwm-sun20i.c line 313

Quoted 4 lines

drivers/pwm/pwm-sun20i.c new file +4 -0

+             npwm = 8; /* Default value */
+
+     if (npwm > 16) {
+             dev_info(&pdev->dev, "Limiting number of PWM lines from %u to 16", npwm);

I don't think we should proceed if the firmware information is clearly wrong. Just bail out with -EINVAL or so here, so that gets fixed in the DT.

Quoted 20 lines

Quoted 20 lines

drivers/pwm/pwm-sun20i.c new file +20 -0

+             npwm = 16;
+     }
+
+     chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*sun20i_chip));
+     if (IS_ERR(chip))
+             return PTR_ERR(chip);
+     sun20i_chip = to_sun20i_pwm_chip(chip);
+
+     sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(sun20i_chip->base))
+             return PTR_ERR(sun20i_chip->base);
+
+     clk_bus = devm_clk_get_enabled(&pdev->dev, "bus");
+     if (IS_ERR(clk_bus))
+             return dev_err_probe(&pdev->dev, PTR_ERR(clk_bus),
+                                  "Failed to get bus clock\n");
+
+     sun20i_chip->clk_hosc = devm_clk_get_enabled(&pdev->dev, "hosc");
+     if (IS_ERR(sun20i_chip->clk_hosc))
+             return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_hosc),

Quoted 8 lines

drivers/pwm/pwm-sun20i.c line 336

Quoted 3 lines

drivers/pwm/pwm-sun20i.c new file +3 -0

+                                  "Failed to get hosc clock\n");
+
+     ret = devm_clk_rate_exclusive_get(&pdev->dev, sun20i_chip->clk_hosc);

Just ignoring for a bit that the 24 MHz oscillator is a fixed clock anyway, but why would we want exclusivity already at probe time? Isn't that too limiting, as no one might ever use any PWM channels, but it would still "belong to us"?

Quoted 7 lines

Quoted 7 lines

drivers/pwm/pwm-sun20i.c new file +7 -0

+     if (ret)
+             return dev_err_probe(&pdev->dev, ret,
+                                  "Failed to get hosc exclusive rate\n");
+
+     sun20i_chip->clk_apb = devm_clk_get_enabled(&pdev->dev, "apb");
+     if (IS_ERR(sun20i_chip->clk_apb))
+             return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_apb),

Quoted 8 lines

drivers/pwm/pwm-sun20i.c line 346

Quoted 3 lines

drivers/pwm/pwm-sun20i.c new file +3 -0

+                                  "Failed to get apb clock\n");
+
+     ret = devm_clk_rate_exclusive_get(&pdev->dev, sun20i_chip->clk_apb);

Just for the records: APB is practically also a fixed clock, set up once in firmware and never changed, since it drives a lot of other peripherals.
But same question as above, why do we lock its rate already here?

That step was actually recommended by Uwe Kleine-König, so the decision on whether to keep or drop exclusive reservation is really a question for him—please coordinate with Uwe to agree on how best to proceed here.

Quoted 11 lines

drivers/pwm/pwm-sun20i.c line 352

Quoted 6 lines

drivers/pwm/pwm-sun20i.c new file +6 -0

+     if (ret)
+             return dev_err_probe(&pdev->dev, ret,
+                                  "Failed to get apb exclusive rate\n");
+
+     if (clk_get_rate(sun20i_chip->clk_apb) <= clk_get_rate(sun20i_chip->clk_hosc))
+             dev_info(&pdev->dev, "APB clock must be greater than hosc clock");

Why this check? Does the code make any assumptions about this relation? If yes, we must surely deny this and bail out.
If not (and I feel we should handle it this way), we can just ignore this and not print anything.

Quoted 25 lines

вт, 13 мая 2025 г. в 01:39, Andre Przywara <[email protected]>:

Quoted 24 lines

drivers/pwm/pwm-sun20i.c line 223

Quoted 8 lines · On Sun, 27 Apr 2025 17:24:54 +0300 Aleksandr Shubin <[email protected]>…

drivers/pwm/pwm-sun20i.c new file +8 -0

+              */
+             use_bus_clk = false;
+             val = mul_u64_u64_div_u64(state->period, hosc_rate, NSEC_PER_SEC);
+             /*
+              * If the calculated value is ≤ 1, the period is too short
+              * for proper PWM operation
+              */
+             if (val <= 1) {  

So if I get the code correctly, it prefers HOSC over APB? Is that really the best way? Shouldn't it be the other way around: we use the faster clock, since this will not limit the sibling channel?

And another thing to consider are rounding errors due to integer
division: certain period rates might be better achievable with one or the other source clock: 3 MHz works best as 24MHz/8, 3.125MHz as 100MHz/32.
So shall we calculate the values and compare the errors instead? Oh, and also we need to consider bypassing, I feel like this should be checked first.

Quoted 8 lines

Quoted 2 lines

In any case I think there should be a comment describing the strategy and give some rationale, I think.

I like the idea of comparing the quantization error for each clock source (i.e. computing the actual period for both APB and HOSC and choosing whichever is closer to the requested period).
I can try to implement that error-minimization approach in the next series of patches and add a comment explaining the strategy.

Consumers have different needs. Some might prefer a better match for period, but in my experience most would go for a fine-grained selection of duty_cycle, so prefering the faster clock sounds sane.

I don't say minimizing the error is wrong, but if it's unclear that this matches what a consumer wants I object to make the procedure to select the hardware settings considerably more complicated and run-time intensive.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/pwm/pwm-sun20i.c new file +11 -0

+static int sun20i_pwm_probe(struct platform_device *pdev)
+{
+     struct pwm_chip *chip;
+     struct sun20i_pwm_chip *sun20i_chip;
+     struct clk *clk_bus;
+     struct reset_control *rst;
+     u32 npwm;
+     int ret;
+
+     ret = of_property_read_u32(pdev->dev.of_node, "allwinner,npwms", &npwm);
+     if (ret < 0)

Quoted 8 lines

Quoted 8 lines

drivers/pwm/pwm-sun20i.c line 313

Quoted 4 lines

drivers/pwm/pwm-sun20i.c new file +4 -0

+             npwm = 8; /* Default value */
+
+     if (npwm > 16) {
+             dev_info(&pdev->dev, "Limiting number of PWM lines from %u to 16", npwm);  

I don't think we should proceed if the firmware information is clearly wrong. Just bail out with -EINVAL or so here, so that gets fixed in the DT.

To me it's not obvious that the "firmware information is clearly wrong". Maybe the next Allwinner SoC will have 24 outputs and the problem is only that this driver isn't prepared to cope for that number of outputs?

If that really happens it's arguable if it's better to refuse completely or just cope for the 16 outputs that the driver is able to. IMHO it's better to continue because a partially workable pwmchip is better than no chip at all. But I'd upgrade the message to dev_warn().

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

drivers/pwm/pwm-sun20i.c new file +20 -0

+             npwm = 16;
+     }
+
+     chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*sun20i_chip));
+     if (IS_ERR(chip))
+             return PTR_ERR(chip);
+     sun20i_chip = to_sun20i_pwm_chip(chip);
+
+     sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(sun20i_chip->base))
+             return PTR_ERR(sun20i_chip->base);
+
+     clk_bus = devm_clk_get_enabled(&pdev->dev, "bus");
+     if (IS_ERR(clk_bus))
+             return dev_err_probe(&pdev->dev, PTR_ERR(clk_bus),
+                                  "Failed to get bus clock\n");
+
+     sun20i_chip->clk_hosc = devm_clk_get_enabled(&pdev->dev, "hosc");
+     if (IS_ERR(sun20i_chip->clk_hosc))
+             return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_hosc),

Quoted 8 lines

Quoted 8 lines

drivers/pwm/pwm-sun20i.c line 336

Quoted 3 lines

drivers/pwm/pwm-sun20i.c new file +3 -0

+                                  "Failed to get hosc clock\n");
+
+     ret = devm_clk_rate_exclusive_get(&pdev->dev, sun20i_chip->clk_hosc);  

Just ignoring for a bit that the 24 MHz oscillator is a fixed clock anyway, but why would we want exclusivity already at probe time? Isn't that too limiting, as no one might ever use any PWM channels, but it would still "belong to us"?

That's a soft concept of "belong to us". Other consumers can still use it and even also call clk_rate_exclusive_get(). IMHO it's a good idea to call clk_rate_exclusive_get() for each clock that a driver relies on not to change. You could make the driver more flexible and only call that when the rate is actually relied on, but that's again a compromise with complexity of the driver. And if the clock rate is fixed anyhow, it doesn't hurt to do it here, right?

Quoted 16 lines

Quoted 15 lines

drivers/pwm/pwm-sun20i.c line 346

Quoted 10 lines

drivers/pwm/pwm-sun20i.c new file +10 -0

+     if (ret)
+             return dev_err_probe(&pdev->dev, ret,
+                                  "Failed to get hosc exclusive rate\n");
+
+     sun20i_chip->clk_apb = devm_clk_get_enabled(&pdev->dev, "apb");
+     if (IS_ERR(sun20i_chip->clk_apb))
+             return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_apb),
+                                  "Failed to get apb clock\n");
+
+     ret = devm_clk_rate_exclusive_get(&pdev->dev, sun20i_chip->clk_apb);  

Just for the records: APB is practically also a fixed clock, set up once in firmware and never changed, since it drives a lot of other peripherals.
But same question as above, why do we lock its rate already here?

Quoted 4 lines

That step was actually recommended by Uwe Kleine-König, so the decision on whether to keep or drop exclusive reservation is really a question for him—please coordinate with Uwe to agree on how best to proceed here.

Same as above. Iff the driver relies on the rate of this clock to keep constant, calling clk_rate_exclusive_get() is right.

Quoted 11 lines

Quoted 11 lines

drivers/pwm/pwm-sun20i.c line 352

Quoted 6 lines

drivers/pwm/pwm-sun20i.c new file +6 -0

+     if (ret)
+             return dev_err_probe(&pdev->dev, ret,
+                                  "Failed to get apb exclusive rate\n");
+
+     if (clk_get_rate(sun20i_chip->clk_apb) <= clk_get_rate(sun20i_chip->clk_hosc))
+             dev_info(&pdev->dev, "APB clock must be greater than hosc clock");  

Why this check? Does the code make any assumptions about this relation? If yes, we must surely deny this and bail out.
If not (and I feel we should handle it this way), we can just ignore this and not print anything.

ack.

Best regards
Uwe

Quoted 35 lines · On Wed, 28 May 2025 13:08:40 +0200 Uwe Kleine-König <[email protected]> wrote:

Quoted 25 lines · Александр Шубин

вт, 13 мая 2025 г. в 01:39, Andre Przywara <[email protected]>:

Quoted 24 lines

drivers/pwm/pwm-sun20i.c line 223

Quoted 8 lines · On Sun, 27 Apr 2025 17:24:54 +0300 Aleksandr Shubin <[email protected]>…

drivers/pwm/pwm-sun20i.c new file +8 -0

+              */
+             use_bus_clk = false;
+             val = mul_u64_u64_div_u64(state->period, hosc_rate, NSEC_PER_SEC);
+             /*
+              * If the calculated value is ≤ 1, the period is too short
+              * for proper PWM operation
+              */
+             if (val <= 1) {  

So if I get the code correctly, it prefers HOSC over APB? Is that really the best way? Shouldn't it be the other way around: we use the faster clock, since this will not limit the sibling channel?

And another thing to consider are rounding errors due to integer
division: certain period rates might be better achievable with one or the other source clock: 3 MHz works best as 24MHz/8, 3.125MHz as 100MHz/32.
So shall we calculate the values and compare the errors instead? Oh, and also we need to consider bypassing, I feel like this should be checked first.

Quoted 8 lines

Quoted 2 lines

In any case I think there should be a comment describing the strategy and give some rationale, I think.

I like the idea of comparing the quantization error for each clock source (i.e. computing the actual period for both APB and HOSC and choosing whichever is closer to the requested period).
I can try to implement that error-minimization approach in the next series of patches and add a comment explaining the strategy.

Quoted 8 lines

Consumers have different needs. Some might prefer a better match for period, but in my experience most would go for a fine-grained selection of duty_cycle, so prefering the faster clock sounds sane.

I don't say minimizing the error is wrong, but if it's unclear that this matches what a consumer wants I object to make the procedure to select the hardware settings considerably more complicated and run-time intensive.

Yes, I agree. There seems to be another use case here, which is to provide clocks on output pins. The PWM IP has a bypass switch (per channel, after the divider), and this feature is already required to supply the "internal" (co-packaged) Ethernet PHY on the Allwinner H616 with its clock. With the two possible input clocks and those pre-dividers there is actually quite a number of possible frequencies to deliver on output pins.

Since we need some algorithm to decide when we need to use the bypass mode, should we check for that if the duty cycle is 50%, to see if we can reach the frequency with just the pre-dividers?
Chances are we need this anyway, since for instance the 24MHz required for the PHY cannot be achieved otherwise.

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/pwm/pwm-sun20i.c new file +11 -0

+static int sun20i_pwm_probe(struct platform_device *pdev)
+{
+     struct pwm_chip *chip;
+     struct sun20i_pwm_chip *sun20i_chip;
+     struct clk *clk_bus;
+     struct reset_control *rst;
+     u32 npwm;
+     int ret;
+
+     ret = of_property_read_u32(pdev->dev.of_node, "allwinner,npwms", &npwm);
+     if (ret < 0)

Quoted 8 lines

Quoted 8 lines

drivers/pwm/pwm-sun20i.c line 313

Quoted 4 lines

drivers/pwm/pwm-sun20i.c new file +4 -0

+             npwm = 8; /* Default value */
+
+     if (npwm > 16) {
+             dev_info(&pdev->dev, "Limiting number of PWM lines from %u to 16", npwm);  

I don't think we should proceed if the firmware information is clearly wrong. Just bail out with -EINVAL or so here, so that gets fixed in the DT.

Quoted 3 lines

To me it's not obvious that the "firmware information is clearly wrong". Maybe the next Allwinner SoC will have 24 outputs and the problem is only that this driver isn't prepared to cope for that number of outputs?

But then it would be an error, regardless?
The MMIO register frame of this IP here has a hard limit on 16 channels, both by the bit assignments in each register (2 bits per channel in a 32-bit register), but also by the layout of the registers (max 8 registers, each for a pair of 2 PWM channels). So anything with more than 16 channels cannot be compatible with what this driver supports. So as this driver here stands right now, more than 16 channels is an error, simple as that. If we extend the driver later on, to cover more advanced IP, we would naturally amend this check, of course.

Aleksandr Shubin (3):
dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM
controller
pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
riscv: dts: allwinner: d1: Add pwm node

 .../bindings/pwm/allwinner,sun20i-pwm.yaml    |  84 ++++
 .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    |  12 +
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-sun20i.c                      | 379 ++++++++++++++++++
 5 files changed, 486 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
 create mode 100644 drivers/pwm/pwm-sun20i.c

Hi Aleksandr,

Quoted 6 lines · Aleksandr Shubin

Aleksandr Shubin (3):
dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM
controller
pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
riscv: dts: allwinner: d1: Add pwm node

Quoted 8 lines

 .../bindings/pwm/allwinner,sun20i-pwm.yaml    |  84 ++++
 .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    |  12 +
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-sun20i.c                      | 379 ++++++++++++++++++
 5 files changed, 486 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
 create mode 100644 drivers/pwm/pwm-sun20i.c

Thanks for your work on this driver!

For context, this PWM controller seems to be a second-generation design from Allwinner, which is found in the following chips: V5, A50, H616, V536, T7, A133, V833, R329, D1/T113, R128, V851, A523 and A733.

I've tested your driver on A133, which works fine too. It seems that H616 uses a similar (but slightly different) register layout.

In case you've missed it, there's a follow-up series adding H616 support at:
https://patchwork.ozlabs.org/project/linux-pwm/list/?series=409036&archive=both&state=*

And there's also a standalone effort (which I've redirected to your series) at:
https://patchwork.ozlabs.org/project/linux-pwm/list/?series=485644&archive=both&state=*

Now given that the new controller was introduced with the V5 (sun8iw12) from 2018, I think it would be a bit confusing to keep the sun20i-pwm name.

How about renaming the driver to sun8i-pwm instead? That would be more consistent with how other second generation designs from Allwinner are usually called in Linux and makes it more clear that it also targets sun8i and sun50i chips, in addition to sun20i.

All the best,

Paul

--
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.