[PATCH v1 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

[PATCH v1 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

[Aleksandr Shubin]

Hi,

This series adds support for PWM controller on new
Allwinner's SoCs, such as D1, T113s and R329. The implemented driver
provides basic functionality for control PWM channels.

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    |  70 ++++
 .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    |  12 +
 drivers/pwm/Kconfig                           |  12 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-sun20i.c                      | 364 ++++++++++++++++++
 5 files changed, 459 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
 create mode 100644 drivers/pwm/pwm-sun20i.c

-- 
2.25.1

[PATCH v1 1/3] dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM controller

[Aleksandr Shubin]

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

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.

Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>
---
 .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
new file mode 100644
index 000000000000..e5f9cb2d5c4f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
@@ -0,0 +1,70 @@
+# 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:
+  - Chen-Yu Tsai <wens@csie.org>
+  - Maxime Ripard <mripard@kernel.org>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: allwinner,sun20i-d1-pwm
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    items:
+      - description: 24 MHz oscillator
+      - description: Bus Clock
+
+  clock-names:
+    items:
+      - const: hosc
+      - const: bus
+
+  resets:
+    items:
+      - description: module reset
+
+  allwinner,pwm-channels:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: The number of PWM channels configured for this instance
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+  - clocks
+  - clock-names
+  - resets
+  - allwinner,pwm-channels
+
+additionalProperties: false
+
+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 = <&dcxo>, <&ccu CLK_BUS_PWM>;
+      clock-names = "hosc", "bus";
+      resets = <&ccu RST_BUS_PWM>;
+      allwinner,pwm-channels = <8>;
+      #pwm-cells = <0x3>;
+    };
+
+...
-- 
2.25.1

[PATCH v1 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support

[Aleksandr Shubin]

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.

Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>
---
 drivers/pwm/Kconfig      |  12 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-sun20i.c | 364 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/pwm/pwm-sun20i.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8df861b1f4a3..b435e50fbd3e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -594,6 +594,18 @@ 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.
+	  The main difference between these SoCs is the number of
+	  channels defined by the DT property.
+
+	  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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 19899b912e00..cea872e22c78 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -55,6 +55,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
diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
new file mode 100644
index 000000000000..100b0f3bcec0
--- /dev/null
+++ b/drivers/pwm/pwm-sun20i.c
@@ -0,0 +1,364 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
+ *
+ * Copyright (c) 2023 Aleksandr Shubin <privatesub2@gmail.com>
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pwm.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+
+#define PWM_CLK_CFG_REG(chan)		(0x20 + (((chan) >> 1) * 0x4))
+#define PWM_CLK_SRC			7
+#define PWM_CLK_SRC_MASK		GENMASK(8, PWM_CLK_SRC)
+#define PWM_CLK_DIV_M			0
+#define PWM_CLK_DIV_M_MASK		GENMASK(3, PWM_CLK_DIV_M)
+
+#define PWM_CLK_GATE_REG		0x40
+#define PWM_CLK_GATING(chan)		BIT(chan)
+
+#define PWM_ENABLE_REG			0x80
+#define PWM_EN(chan)			BIT(chan)
+
+#define PWM_CTL_REG(chan)		(0x100 + (chan) * 0x20)
+#define PWM_ACT_STA			BIT(8)
+#define PWM_PRESCAL_K			0
+#define PWM_PRESCAL_K_MASK		GENMASK(7, PWM_PRESCAL_K)
+
+#define PWM_PERIOD_REG(chan)		(0x104 + (chan) * 0x20)
+#define PWM_ENTIRE_CYCLE		16
+#define PWM_ENTIRE_CYCLE_MASK		GENMASK(31, PWM_ENTIRE_CYCLE)
+#define PWM_ACT_CYCLE			0
+#define PWM_ACT_CYCLE_MASK		GENMASK(15, PWM_ACT_CYCLE)
+
+#define SET_VALUE(reg_val, val, name) \
+		 (reg_val = (((reg_val) & ~name##_MASK) | ((val) << (name))))
+#define GET_VALUE(reg_val, name) \
+		 (((reg_val) & ~name##_MASK) >> (name))
+
+struct sun20i_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk_bus, *clk_hosc;
+	struct reset_control *rst;
+	void __iomem *base;
+	/* Mutex to protect pwm apply state */
+	struct mutex mutex;
+};
+
+static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct sun20i_pwm_chip, 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);
+	u64 clk_rate, tmp;
+	u32 val;
+	u16 clk_div, act_cycle;
+	u8 prescal, div_id;
+	u8 chn = pwm->hwpwm;
+
+	mutex_lock(&sun20i_chip->mutex);
+
+	val = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
+	div_id = GET_VALUE(val, PWM_CLK_DIV_M);
+	if (GET_VALUE(val, PWM_CLK_SRC) == 0)
+		clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
+	else
+		clk_rate = clk_get_rate(sun20i_chip->clk_bus);
+
+	val = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
+	if (PWM_ACT_STA & val)
+		state->polarity = PWM_POLARITY_NORMAL;
+	else
+		state->polarity = PWM_POLARITY_INVERSED;
+
+	prescal = PWM_PRESCAL_K & val;
+
+	val = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
+	if (PWM_EN(chn) & val)
+		state->enabled = true;
+	else
+		state->enabled = false;
+
+	val = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
+	act_cycle = GET_VALUE(val, PWM_ACT_CYCLE);
+	clk_div = GET_VALUE(val, PWM_ENTIRE_CYCLE);
+
+	tmp = act_cycle * prescal * (1U << div_id) * NSEC_PER_SEC;
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+	tmp = clk_div * prescal * (1U << div_id) * NSEC_PER_SEC;
+	state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+
+	mutex_unlock(&sun20i_chip->mutex);
+
+	return 0;
+}
+
+static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct pwm_state curstate;
+	int ret = 0;
+	u32 clk_gate, clk_cfg, pwm_en, ctl, period;
+	u64 bus_rate, hosc_rate, clk_div, val, clk_rate;
+	u16 prescaler, div_m;
+	bool use_bus_clk;
+	struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
+
+	mutex_lock(&sun20i_chip->mutex);
+	pwm_get_state(pwm, &curstate);
+
+	pwm_en = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
+	if (state->polarity != curstate.polarity ||
+	    state->duty_cycle != curstate.duty_cycle ||
+	    state->period != curstate.period) {
+		ctl = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
+		clk_cfg = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
+		hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
+		bus_rate = clk_get_rate(sun20i_chip->clk_bus);
+
+		if (pwm_en & PWM_EN(pwm->hwpwm ^ 1)) {
+			/* check period only */
+			if (GET_VALUE(clk_cfg, PWM_CLK_SRC) == 0)
+				clk_rate = hosc_rate;
+			else
+				clk_rate = bus_rate;
+
+			val = state->period * clk_rate;
+			do_div(val, NSEC_PER_SEC);
+
+			div_m = GET_VALUE(clk_cfg, PWM_CLK_DIV_M);
+
+			/* calculate and set prescaler, PWM entire cycle */
+			clk_div = val;
+			for (prescaler = 0; clk_div > 65535; prescaler++) {
+				if (prescaler >= 256) {
+					dev_err(sun20i_chip->chip.dev, "Period is too long\n");
+					ret = -EINVAL;
+					goto unlock_mutex;
+				}
+
+				clk_div = val;
+				do_div(clk_div, 1U << div_m);
+				do_div(clk_div, prescaler + 1);
+			}
+		} else {
+			/* check period and select clock source */
+			use_bus_clk = false;
+			val = state->period * hosc_rate;
+			do_div(val, NSEC_PER_SEC);
+			if (val <= 1) {
+				use_bus_clk = true;
+				val = state->period * bus_rate;
+				do_div(val, NSEC_PER_SEC);
+				if (val <= 1) {
+					dev_err(sun20i_chip->chip.dev, "Period is too small\n");
+					ret = -EINVAL;
+					goto unlock_mutex;
+				}
+			}
+
+			if (use_bus_clk)
+				SET_VALUE(clk_cfg, 1, PWM_CLK_SRC);
+			else
+				SET_VALUE(clk_cfg, 0, PWM_CLK_SRC);
+
+			/* calculate and set prescaler, M factor, PWM entire cycle */
+			clk_div = val;
+			for (prescaler = div_m = 0; clk_div > 65535; prescaler++) {
+				if (prescaler >= 256) {
+					prescaler = 0;
+					div_m++;
+					if (div_m >= 9) {
+						dev_err(sun20i_chip->chip.dev, "Period is too long\n");
+						ret = -EINVAL;
+						goto unlock_mutex;
+					}
+				}
+
+				clk_div = val;
+				do_div(clk_div, 1U << div_m);
+				do_div(clk_div, prescaler + 1);
+			}
+
+			/* set up the M factor */
+			SET_VALUE(clk_cfg, div_m, PWM_CLK_DIV_M);
+
+			sun20i_pwm_writel(sun20i_chip, clk_cfg, PWM_CLK_CFG_REG(pwm->hwpwm));
+		}
+
+		period = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
+
+		SET_VALUE(period, clk_div, PWM_ENTIRE_CYCLE);
+		SET_VALUE(ctl, prescaler, PWM_PRESCAL_K);
+
+		/* set duty cycle */
+		val = state->period;
+		do_div(val, clk_div);
+		clk_div = state->duty_cycle;
+		do_div(clk_div, val);
+		if (clk_div > 65535)
+			clk_div = 65535;
+
+		SET_VALUE(period, clk_div, PWM_ACT_CYCLE);
+		sun20i_pwm_writel(sun20i_chip, period, PWM_PERIOD_REG(pwm->hwpwm));
+
+		if (state->polarity == PWM_POLARITY_NORMAL)
+			ctl |= PWM_ACT_STA;
+		else
+			ctl &= ~PWM_ACT_STA;
+
+		sun20i_pwm_writel(sun20i_chip, ctl, PWM_CTL_REG(pwm->hwpwm));
+	}
+
+	if (state->enabled != curstate.enabled) {
+		clk_gate = sun20i_pwm_readl(sun20i_chip, PWM_CLK_GATE_REG);
+
+		if (state->enabled) {
+			clk_gate |= PWM_CLK_GATING(pwm->hwpwm);
+			pwm_en |= PWM_EN(pwm->hwpwm);
+		} else {
+			clk_gate &= ~PWM_CLK_GATING(pwm->hwpwm);
+			pwm_en &= ~PWM_EN(pwm->hwpwm);
+		}
+		sun20i_pwm_writel(sun20i_chip, pwm_en, PWM_ENABLE_REG);
+		sun20i_pwm_writel(sun20i_chip, clk_gate, PWM_CLK_GATE_REG);
+	}
+
+unlock_mutex:
+	mutex_unlock(&sun20i_chip->mutex);
+
+	return ret;
+}
+
+static const struct pwm_ops sun20i_pwm_ops = {
+	.get_state = sun20i_pwm_get_state,
+	.apply = sun20i_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+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 sun20i_pwm_chip *sun20i_chip;
+	int ret;
+
+	sun20i_chip = devm_kzalloc(&pdev->dev, sizeof(*sun20i_chip), GFP_KERNEL);
+	if (!sun20i_chip)
+		return -ENOMEM;
+
+	sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(sun20i_chip->base))
+		return PTR_ERR(sun20i_chip->base);
+
+	sun20i_chip->clk_bus = devm_clk_get(&pdev->dev, "bus");
+	if (IS_ERR(sun20i_chip->clk_bus)) {
+		dev_err(&pdev->dev, "Failed to get bus clock\n");
+		return PTR_ERR(sun20i_chip->clk_bus);
+	}
+
+	sun20i_chip->clk_hosc = devm_clk_get(&pdev->dev, "hosc");
+	if (IS_ERR(sun20i_chip->clk_hosc)) {
+		dev_err(&pdev->dev, "Failed to get hosc clock\n");
+		return PTR_ERR(sun20i_chip->clk_hosc);
+	}
+
+	sun20i_chip->rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(sun20i_chip->rst)) {
+		dev_err(&pdev->dev, "Failed to get bus reset\n");
+		return PTR_ERR(sun20i_chip->rst);
+	}
+
+	/* Deassert reset */
+	ret = reset_control_deassert(sun20i_chip->rst);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to deassert reset\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(sun20i_chip->clk_bus);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to ungate bus clock\n");
+		goto err_bus;
+	}
+
+	ret = of_property_read_u32(pdev->dev.of_node,
+				   "allwinner,pwm-channels",
+				   &sun20i_chip->chip.npwm);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't get pwm-channels\n");
+		goto err_pwm_add;
+	}
+
+	sun20i_chip->chip.dev = &pdev->dev;
+	sun20i_chip->chip.ops = &sun20i_pwm_ops;
+
+	mutex_init(&sun20i_chip->mutex);
+
+	ret = pwmchip_add(&sun20i_chip->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to add PWM chip: %d\n", ret);
+		goto err_pwm_add;
+	}
+
+	platform_set_drvdata(pdev, sun20i_chip);
+
+	return 0;
+
+err_pwm_add:
+	clk_disable_unprepare(sun20i_chip->clk_bus);
+err_bus:
+	reset_control_assert(sun20i_chip->rst);
+	return ret;
+}
+
+static void sun20i_pwm_remove(struct platform_device *pdev)
+{
+	struct sun20i_pwm_chip *sun20i_chip = platform_get_drvdata(pdev);
+	int ret;
+
+	pwmchip_remove(&sun20i_chip->chip);
+
+	clk_disable_unprepare(sun20i_chip->clk_bus);
+	reset_control_assert(sun20i_chip->rst);
+}
+
+static struct platform_driver sun20i_pwm_driver = {
+	.driver = {
+		.name = "sun20i-pwm",
+		.of_match_table = sun20i_pwm_dt_ids,
+	},
+	.probe = sun20i_pwm_probe,
+	.remove_new = sun20i_pwm_remove,
+};
+module_platform_driver(sun20i_pwm_driver);
+
+MODULE_AUTHOR("Aleksandr Shubin <privatesub2@gmail.com>");
+MODULE_DESCRIPTION("Allwinner sun20i PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH v1 3/3] riscv: dts: allwinner: d1: Add pwm node

[Aleksandr Shubin]

D1 and T113s contain a pwm controller with 8 channels.
This controller is supported by the sun20i-pwm driver.

Add a device tree node for it.

Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>
---
 arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
index 922e8e0e2c09..50f0f761527b 100644
--- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
@@ -127,6 +127,18 @@ uart3_pb_pins: uart3-pb-pins {
 			};
 		};
 
+		pwm: pwm@2000c00 {
+			compatible = "allwinner,sun20i-d1-pwm";
+			reg = <0x02000c00 0x400>;
+			clocks = <&dcxo>,
+				 <&ccu CLK_BUS_PWM>;
+			clock-names = "hosc", "bus";
+			resets = <&ccu RST_BUS_PWM>;
+			allwinner,pwm-channels = <8>;
+			status = "disabled";
+			#pwm-cells = <0x3>;
+		};
+
 		ccu: clock-controller@2001000 {
 			compatible = "allwinner,sun20i-d1-ccu";
 			reg = <0x2001000 0x1000>;
-- 
2.25.1

Re: [PATCH v1 1/3] dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM controller

[Conor Dooley]

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

Hey Aleksandr,

On Thu, Jun 15, 2023 at 05:43:56PM +0300, Aleksandr Shubin wrote:
> Allwinner's D1, T113-S3 and R329 SoCs have a new pwm
> controller witch is different from the previous pwm-sun4i.
> 
> 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).

It would be good to note that 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. It would be fine to use the current compatible for
the D1 as a fallback.
The allwinner,pwm-channels property should probably only be allowed on
the R329 and only allow the values of 6 & 9.

> Add a device tree binding for them.
> 
> Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>
> ---
>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> new file mode 100644
> index 000000000000..e5f9cb2d5c4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> @@ -0,0 +1,70 @@
> +# 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:
> +  - Chen-Yu Tsai <wens@csie.org>
> +  - Maxime Ripard <mripard@kernel.org>

BTW, this should probably be you, since you have the hardware with this
SoC.

> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    const: allwinner,sun20i-d1-pwm
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 3
> +
> +  clocks:
> +    items:
> +      - description: 24 MHz oscillator
> +      - description: Bus Clock
> +
> +  clock-names:
> +    items:
> +      - const: hosc
> +      - const: bus
> +
> +  resets:
> +    items:
> +      - description: module reset

Don't need the items here, because there is only one. You can just do
"maxItems: 1" instead.

> +  allwinner,pwm-channels:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: The number of PWM channels configured for this instance
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#pwm-cells"
> +  - clocks
> +  - clock-names
> +  - resets
> +  - allwinner,pwm-channels

Cheers,
Conor.

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

Re: [PATCH v1 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support

[Uwe Kleine-König]

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

Hello,

this isn't a complete review, just a first quick look. Up to you if you
follow up with a v2 fixing the things here. If you don't I will take a
deeper look at a later time.

On Thu, Jun 15, 2023 at 05:43:57PM +0300, Aleksandr Shubin wrote:
> 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.
> 
> Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>
> ---
>  drivers/pwm/Kconfig      |  12 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-sun20i.c | 364 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 377 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sun20i.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8df861b1f4a3..b435e50fbd3e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -594,6 +594,18 @@ 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.
> +	  The main difference between these SoCs is the number of
> +	  channels defined by the DT property.

That is a detail that better fits into a code comment in the driver. The
user choosing if they should enable PWM_SUN20I or not, don't need to
know that.

> +	  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
> [...]
> diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
> new file mode 100644
> index 000000000000..100b0f3bcec0
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun20i.c
> @@ -0,0 +1,364 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
> + *
> + * Copyright (c) 2023 Aleksandr Shubin <privatesub2@gmail.com>
> + */

Please add a "Limitations" section here answering the following
questions:

 - Is a currently running period completed when a new pwm_state is
   applied that has .enabled = true? What about .enabled = false?
 - How does the PWM behave when disabled? Typical behaviours are: drives
   to inactive level, just freezes or goes HIGH-Z.

Stick to the format that is used in other drivers to make this easily
grepable.

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/clk.h>
> +#include <linux/reset.h>

alphabetic ordering please

> +#define PWM_CLK_CFG_REG(chan)		(0x20 + (((chan) >> 1) * 0x4))
> +#define PWM_CLK_SRC			7
> +#define PWM_CLK_SRC_MASK		GENMASK(8, PWM_CLK_SRC)
> +#define PWM_CLK_DIV_M			0
> +#define PWM_CLK_DIV_M_MASK		GENMASK(3, PWM_CLK_DIV_M)
> +
> +#define PWM_CLK_GATE_REG		0x40
> +#define PWM_CLK_GATING(chan)		BIT(chan)
> +
> +#define PWM_ENABLE_REG			0x80
> +#define PWM_EN(chan)			BIT(chan)
> +
> +#define PWM_CTL_REG(chan)		(0x100 + (chan) * 0x20)
> +#define PWM_ACT_STA			BIT(8)
> +#define PWM_PRESCAL_K			0
> +#define PWM_PRESCAL_K_MASK		GENMASK(7, PWM_PRESCAL_K)
> +
> +#define PWM_PERIOD_REG(chan)		(0x104 + (chan) * 0x20)
> +#define PWM_ENTIRE_CYCLE		16
> +#define PWM_ENTIRE_CYCLE_MASK		GENMASK(31, PWM_ENTIRE_CYCLE)
> +#define PWM_ACT_CYCLE			0
> +#define PWM_ACT_CYCLE_MASK		GENMASK(15, PWM_ACT_CYCLE)
> +
> +#define SET_VALUE(reg_val, val, name) \
> +		 (reg_val = (((reg_val) & ~name##_MASK) | ((val) << (name))))
> +#define GET_VALUE(reg_val, name) \
> +		 (((reg_val) & ~name##_MASK) >> (name))

Please use FIELD_GET here instead. This works nice with dropping
PWM_PRESCAL_K and renaming PWM_PRESCAL_K_MASK to PWM_PRESCAL_K.

> [...]
> +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);
> +	u64 clk_rate, tmp;
> +	u32 val;
> +	u16 clk_div, act_cycle;
> +	u8 prescal, div_id;
> +	u8 chn = pwm->hwpwm;
> +
> +	mutex_lock(&sun20i_chip->mutex);
> +
> +	val = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> +	div_id = GET_VALUE(val, PWM_CLK_DIV_M);
> +	if (GET_VALUE(val, PWM_CLK_SRC) == 0)
> +		clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
> +	else
> +		clk_rate = clk_get_rate(sun20i_chip->clk_bus);
> +
> +	val = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
> +	if (PWM_ACT_STA & val)
> +		state->polarity = PWM_POLARITY_NORMAL;
> +	else
> +		state->polarity = PWM_POLARITY_INVERSED;
> +
> +	prescal = PWM_PRESCAL_K & val;
> +
> +	val = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> +	if (PWM_EN(chn) & val)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;
> +
> +	val = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
> +	act_cycle = GET_VALUE(val, PWM_ACT_CYCLE);
> +	clk_div = GET_VALUE(val, PWM_ENTIRE_CYCLE);
> +
> +	tmp = act_cycle * prescal * (1U << div_id) * NSEC_PER_SEC;
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
> +	tmp = clk_div * prescal * (1U << div_id) * NSEC_PER_SEC;
> +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);

You have to use ROUND_UP here to make

	.get_state(mypwm, &state);
	.apply(mypwm, &state);

idempotent. Test your driver with PWM_DEBUG enabled. With an appropriate
test sequence this should tell you such things. Test with something
like:

	for i in some_range:
		pwm_apply_state(mypwm, { .enabled = true, .period = i, .duty_cycle = 0 })
	for i in reversed(some_range):
		pwm_apply_state(mypwm, { .enabled = true, .period = i, .duty_cycle = 0 })

	for i in [0, someperiod]:
		pwm_apply_state(mypwm, { .enabled = true, .period = someperiod, .duty_cycle = i })
	for i in reversed([0, someperiod]):
		pwm_apply_state(mypwm, { .enabled = true, .period = someperiod, .duty_cycle = i })

> +	mutex_unlock(&sun20i_chip->mutex);
> +
> +	return 0;
> +}
> +
> +static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct pwm_state curstate;
> +	int ret = 0;
> +	u32 clk_gate, clk_cfg, pwm_en, ctl, period;
> +	u64 bus_rate, hosc_rate, clk_div, val, clk_rate;
> +	u16 prescaler, div_m;
> +	bool use_bus_clk;
> +	struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> +
> +	mutex_lock(&sun20i_chip->mutex);
> +	pwm_get_state(pwm, &curstate);

Don't call pwm API functions in lowlevel driver. Even less while holding
a lock. If you need the previous state, using pwm->state is fine.

> +	pwm_en = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> +	if (state->polarity != curstate.polarity ||
> +	    state->duty_cycle != curstate.duty_cycle ||
> +	    state->period != curstate.period) {
> +		ctl = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
> +		clk_cfg = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> +		hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
> +		bus_rate = clk_get_rate(sun20i_chip->clk_bus);
> +
> +		if (pwm_en & PWM_EN(pwm->hwpwm ^ 1)) {
> +			/* check period only */
> +			if (GET_VALUE(clk_cfg, PWM_CLK_SRC) == 0)
> +				clk_rate = hosc_rate;
> +			else
> +				clk_rate = bus_rate;
> +
> +			val = state->period * clk_rate;

This might overflow.

> +			do_div(val, NSEC_PER_SEC);
> +
> +			div_m = GET_VALUE(clk_cfg, PWM_CLK_DIV_M);
> +
> +			/* calculate and set prescaler, PWM entire cycle */
> +			clk_div = val;
> +			for (prescaler = 0; clk_div > 65535; prescaler++) {
> +				if (prescaler >= 256) {
> +					dev_err(sun20i_chip->chip.dev, "Period is too long\n");

dev_err in .apply() isn't a good idea. Please fail silently.

Also you should configure the biggest period the hardware can support
that isn't bigger than state->period. So the requested period cannot be
too long.

> +					ret = -EINVAL;
> +					goto unlock_mutex;
> +				}
> +
> +				clk_div = val;
> +				do_div(clk_div, 1U << div_m);

This can be simplified to clk_div >> div_m.

> +				do_div(clk_div, prescaler + 1);
> +			}

I wonder if prescaler can be calculated without a loop.

> +		} else {
> +			/* check period and select clock source */
> +			use_bus_clk = false;
> +			val = state->period * hosc_rate;
> +			do_div(val, NSEC_PER_SEC);
> +			if (val <= 1) {
> +				use_bus_clk = true;
> +				val = state->period * bus_rate;
> +				do_div(val, NSEC_PER_SEC);
> +				if (val <= 1) {
> +					dev_err(sun20i_chip->chip.dev, "Period is too small\n");
> +					ret = -EINVAL;
> +					goto unlock_mutex;
> +				}
> +			}
> +
> +			if (use_bus_clk)
> +				SET_VALUE(clk_cfg, 1, PWM_CLK_SRC);
> +			else
> +				SET_VALUE(clk_cfg, 0, PWM_CLK_SRC);
> +
> +			/* calculate and set prescaler, M factor, PWM entire cycle */
> +			clk_div = val;
> +			for (prescaler = div_m = 0; clk_div > 65535; prescaler++) {
> +				if (prescaler >= 256) {
> +					prescaler = 0;
> +					div_m++;
> +					if (div_m >= 9) {
> +						dev_err(sun20i_chip->chip.dev, "Period is too long\n");
> +						ret = -EINVAL;
> +						goto unlock_mutex;
> +					}
> +				}
> +
> +				clk_div = val;
> +				do_div(clk_div, 1U << div_m);
> +				do_div(clk_div, prescaler + 1);
> +			}
> +
> +			/* set up the M factor */
> +			SET_VALUE(clk_cfg, div_m, PWM_CLK_DIV_M);
> +
> +			sun20i_pwm_writel(sun20i_chip, clk_cfg, PWM_CLK_CFG_REG(pwm->hwpwm));
> +		}
> +
> +		period = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
> +
> +		SET_VALUE(period, clk_div, PWM_ENTIRE_CYCLE);
> +		SET_VALUE(ctl, prescaler, PWM_PRESCAL_K);

Are there any fields in PWM_PERIOD_REG(pwm->hwpwm) you're not
recalculating? If not, you can skip reading the register and use
FIELD_PREP instead of SET_VALUE.

> +
> +		/* set duty cycle */
> +		val = state->period;
> +		do_div(val, clk_div);
> +		clk_div = state->duty_cycle;
> +		do_div(clk_div, val);
> +		if (clk_div > 65535)
> +			clk_div = 65535;
> +
> +		SET_VALUE(period, clk_div, PWM_ACT_CYCLE);
> +		sun20i_pwm_writel(sun20i_chip, period, PWM_PERIOD_REG(pwm->hwpwm));
> +
> +		if (state->polarity == PWM_POLARITY_NORMAL)
> +			ctl |= PWM_ACT_STA;
> +		else
> +			ctl &= ~PWM_ACT_STA;
> +
> +		sun20i_pwm_writel(sun20i_chip, ctl, PWM_CTL_REG(pwm->hwpwm));
> +	}
> +
> +	if (state->enabled != curstate.enabled) {
> +		clk_gate = sun20i_pwm_readl(sun20i_chip, PWM_CLK_GATE_REG);
> +
> +		if (state->enabled) {
> +			clk_gate |= PWM_CLK_GATING(pwm->hwpwm);
> +			pwm_en |= PWM_EN(pwm->hwpwm);
> +		} else {
> +			clk_gate &= ~PWM_CLK_GATING(pwm->hwpwm);
> +			pwm_en &= ~PWM_EN(pwm->hwpwm);
> +		}

Disabling (probably) needs to be handled earlier. Consider your PWM is
enabled with say .period = 5000 and .duty_cycle = 0. If now

	pwm_apply_state(&mypwm, &(struct pwm_state){ .period = 5000, .duty_cycle = 5000, .enabled = false })

is called, the output might become active before gating becomes
effective. It's not always possible to prevent such spikes, but if it
is, please do.

> +		sun20i_pwm_writel(sun20i_chip, pwm_en, PWM_ENABLE_REG);
> +		sun20i_pwm_writel(sun20i_chip, clk_gate, PWM_CLK_GATE_REG);
> +	}
> +
> +unlock_mutex:
> +	mutex_unlock(&sun20i_chip->mutex);
> +
> +	return ret;
> +}
> [...]
> +static int sun20i_pwm_probe(struct platform_device *pdev)
> +{
> +	struct sun20i_pwm_chip *sun20i_chip;
> +	int ret;
> +
> +	sun20i_chip = devm_kzalloc(&pdev->dev, sizeof(*sun20i_chip), GFP_KERNEL);
> +	if (!sun20i_chip)
> +		return -ENOMEM;
> +
> +	sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(sun20i_chip->base))
> +		return PTR_ERR(sun20i_chip->base);
> +
> +	sun20i_chip->clk_bus = devm_clk_get(&pdev->dev, "bus");

consider using devm_clk_get_enabled()

> +	if (IS_ERR(sun20i_chip->clk_bus)) {
> +		dev_err(&pdev->dev, "Failed to get bus clock\n");
> +		return PTR_ERR(sun20i_chip->clk_bus);

Please make this:

	return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_bus),
			     "Failed to get bus clock\n");

ditto for the other error paths.

> +	}
> +
> +	sun20i_chip->clk_hosc = devm_clk_get(&pdev->dev, "hosc");
> +	if (IS_ERR(sun20i_chip->clk_hosc)) {
> +		dev_err(&pdev->dev, "Failed to get hosc clock\n");
> +		return PTR_ERR(sun20i_chip->clk_hosc);
> +	}
> +
> +	sun20i_chip->rst = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(sun20i_chip->rst)) {
> +		dev_err(&pdev->dev, "Failed to get bus reset\n");
> +		return PTR_ERR(sun20i_chip->rst);
> +	}
> +
> +	/* Deassert reset */
> +	ret = reset_control_deassert(sun20i_chip->rst);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to deassert reset\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(sun20i_chip->clk_bus);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to ungate bus clock\n");
> +		goto err_bus;
> +	}
> +
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   "allwinner,pwm-channels",
> +				   &sun20i_chip->chip.npwm);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't get pwm-channels\n");
> +		goto err_pwm_add;
> +	}
> +
> +	sun20i_chip->chip.dev = &pdev->dev;
> +	sun20i_chip->chip.ops = &sun20i_pwm_ops;
> +
> +	mutex_init(&sun20i_chip->mutex);

If the hardware is enabled, please make sure that the needed clocks are
not disabled.

> +	ret = pwmchip_add(&sun20i_chip->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to add PWM chip: %d\n", ret);
> +		goto err_pwm_add;
> +	}
> +
> +	platform_set_drvdata(pdev, sun20i_chip);
> +
> +	return 0;
> +
> +err_pwm_add:
> +	clk_disable_unprepare(sun20i_chip->clk_bus);
> +err_bus:
> +	reset_control_assert(sun20i_chip->rst);
> +	return ret;
> +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v1 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support

[Александр Шубин]

Hi Uwe,

пт, 16 июн. 2023 г. в 11:03, Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>
> Hello,
>
> this isn't a complete review, just a first quick look. Up to you if you
> follow up with a v2 fixing the things here. If you don't I will take a
> deeper look at a later time.

Thank you for your review. I will take your remarks into account when
creating the v2.

Re: [PATCH v1 1/3] dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM controller

[Conor Dooley]

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

On Fri, Jun 16, 2023 at 11:02:12AM +0300, Александр Шубин wrote:
> чт, 15 июн. 2023 г. в 19:37, Conor Dooley <conor@kernel.org>:
> > On Thu, Jun 15, 2023 at 05:43:56PM +0300, Aleksandr Shubin wrote:
> > > Allwinner's D1, T113-S3 and R329 SoCs have a new pwm
> > > controller witch is different from the previous pwm-sun4i.
> > >
> > > 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).
> >
> > It would be good to note that 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. It would be fine to use the current compatible for
> > the D1 as a fallback.
> > The allwinner,pwm-channels property should probably only be allowed on
> > the R329 and only allow the values of 6 & 9.
> 
> 
> Maybe would it be better to allow only 8 channels for D1 and only 6 and 9
> channels for R329?

If you're on a D1, you don't need a dt property to tell you the number
of channels, you already know it to be 8. Couple ways driver could
implement that, for example: if the dt-binding requires the property on
the R329, you could set 8 as the default and overwrite it if the
property exists - which would only be permitted on the R329.
Up to you.

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