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

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

v2:
 - fix dt-bindings
 - fix a remark in the driver

v3:
 - fix dt-bindings
 - fix sunxi-d1s-t113.dtsi

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

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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.

Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>
---
 .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 86 +++++++++++++++++++
 1 file changed, 86 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..4e6eaa18f342
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
@@ -0,0 +1,86 @@
+# 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 <privatesub2@gmail.com>
+
+properties:
+  compatible:
+    oneOf:
+      - const: allwinner,sun20i-d1-pwm
+      - items:
+          - const: allwinner,sun20i-r329-pwm
+          - 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:
+    maxItems: 1
+
+  allwinner,pwm-channels:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: The number of PWM channels configured for this instance
+    enum: [6, 9]
+
+allOf:
+  - $ref: pwm.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: allwinner,sun20i-r329-pwm
+
+    then:
+      required:
+        - allwinner,pwm-channels
+
+    else:
+      not:
+        required:
+          - allwinner,pwm-channels
+
+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 = <&dcxo>, <&ccu CLK_BUS_PWM>;
+      clock-names = "hosc", "bus";
+      resets = <&ccu RST_BUS_PWM>;
+      #pwm-cells = <0x3>;
+    };
+
+...
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 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      |  10 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 333 insertions(+)
 create mode 100644 drivers/pwm/pwm-sun20i.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8df861b1f4a3..05c48a36969e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -594,6 +594,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
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..63e9c64e0e18
--- /dev/null
+++ b/drivers/pwm/pwm-sun20i.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
+ *
+ * Limitations:
+ * - When the parameters change, current running period will not be completed
+ *   and run new settings immediately.
+ * - It output HIGH-Z state when PWM channel disabled.
+ *
+ * Copyright (c) 2023 Aleksandr Shubin <privatesub2@gmail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+
+#define PWM_CLK_CFG_REG(chan)		(0x20 + (((chan) >> 1) * 0x4))
+#define PWM_CLK_SRC			GENMASK(8, 7)
+#define PWM_CLK_DIV_M			GENMASK(3, 0)
+
+#define PWM_CLK_GATE_REG		0x40
+#define PWM_CLK_BYPASS(chan)		BIT((chan) - 16)
+#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			GENMASK(7, 0)
+
+#define PWM_PERIOD_REG(chan)		(0x104 + (chan) * 0x20)
+#define PWM_ENTIRE_CYCLE		GENMASK(31, 16)
+#define PWM_ACT_CYCLE			GENMASK(15, 0)
+
+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 ent_cycle, act_cycle;
+	u8 prescal, div_id;
+
+	mutex_lock(&sun20i_chip->mutex);
+
+	val = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
+	div_id = FIELD_GET(PWM_CLK_DIV_M, val);
+	if (FIELD_GET(PWM_CLK_SRC, val) == 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));
+	state->polarity = (PWM_ACT_STA & val) ? PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
+
+	prescal = FIELD_GET(PWM_PRESCAL_K, val) + 1;
+
+	val = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
+	state->enabled = (PWM_EN(pwm->hwpwm) & val) ? true : false;
+
+	val = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
+	act_cycle = FIELD_GET(PWM_ACT_CYCLE, val);
+	ent_cycle = FIELD_GET(PWM_ENTIRE_CYCLE, val);
+	if (act_cycle > ent_cycle)
+		act_cycle = ent_cycle;
+
+	tmp = (u64)(act_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
+	state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
+	tmp = (u64)(ent_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
+	state->period = DIV_ROUND_UP_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)
+{
+	int ret = 0;
+	u32 clk_gate, clk_cfg, pwm_en, ctl, period;
+	u64 bus_rate, hosc_rate, clk_div, val;
+	u16 prescaler, div_m;
+	bool use_bus_clk, calc_div_m;
+	struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
+
+	mutex_lock(&sun20i_chip->mutex);
+
+	pwm_en = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
+
+	if (state->enabled != pwm->state.enabled)
+		clk_gate = sun20i_pwm_readl(sun20i_chip, PWM_CLK_GATE_REG);
+
+	if (state->enabled != pwm->state.enabled && !state->enabled) {
+		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);
+	}
+
+	if (state->polarity != pwm->state.polarity ||
+	    state->duty_cycle != pwm->state.duty_cycle ||
+	    state->period != pwm->state.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)) {
+			/* if the neighbor channel is enable, check period only */
+			use_bus_clk = FIELD_GET(PWM_CLK_SRC, clk_cfg) != 0;
+			if (use_bus_clk)
+				val = state->period * bus_rate;
+			else
+				val = state->period * hosc_rate;
+			do_div(val, NSEC_PER_SEC);
+
+			div_m = FIELD_GET(PWM_CLK_DIV_M, clk_cfg);
+			calc_div_m = false;
+		} 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) {
+					ret = -EINVAL;
+					goto unlock_mutex;
+				}
+			}
+			div_m = 0;
+			calc_div_m = true;
+
+			/* set up the CLK_DIV_M and clock CLK_SRC */
+			clk_cfg = FIELD_PREP(PWM_CLK_DIV_M, div_m);
+			clk_cfg |= FIELD_PREP(PWM_CLK_SRC, use_bus_clk ? 1 : 0);
+
+			sun20i_pwm_writel(sun20i_chip, clk_cfg, PWM_CLK_CFG_REG(pwm->hwpwm));
+		}
+
+		/* calculate prescaler, M factor, PWM entire cycle */
+		clk_div = val;
+		for (prescaler = 0;; prescaler++) {
+			if (prescaler >= 256) {
+				if (calc_div_m) {
+					prescaler = 0;
+					div_m++;
+					if (div_m >= 9) {
+						ret = -EINVAL;
+						goto unlock_mutex;
+					}
+				} else {
+					ret = -EINVAL;
+					goto unlock_mutex;
+				}
+			}
+
+			clk_div = val >> div_m;
+			do_div(clk_div, prescaler + 1);
+			if (clk_div <= 65534)
+				break;
+		}
+
+		period = FIELD_PREP(PWM_ENTIRE_CYCLE, clk_div);
+
+		/* set duty cycle */
+		if (use_bus_clk)
+			val = state->duty_cycle * bus_rate;
+		else
+			val = state->duty_cycle * hosc_rate;
+		do_div(val, NSEC_PER_SEC);
+		clk_div = val >> div_m;
+		do_div(clk_div, prescaler + 1);
+
+		if (state->duty_cycle == state->period)
+			clk_div++;
+		period |= FIELD_PREP(PWM_ACT_CYCLE, clk_div);
+		sun20i_pwm_writel(sun20i_chip, period, PWM_PERIOD_REG(pwm->hwpwm));
+
+		ctl = FIELD_PREP(PWM_PRESCAL_K, prescaler);
+		if (state->polarity == PWM_POLARITY_NORMAL)
+			ctl |= PWM_ACT_STA;
+
+		sun20i_pwm_writel(sun20i_chip, ctl, PWM_CTL_REG(pwm->hwpwm));
+	}
+
+	if (state->enabled != pwm->state.enabled && state->enabled) {
+		clk_gate &= ~PWM_CLK_BYPASS(pwm->hwpwm);
+		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_enabled(&pdev->dev, "bus");
+	if (IS_ERR(sun20i_chip->clk_bus))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->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");
+
+	sun20i_chip->rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(sun20i_chip->rst))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->rst),
+				     "failed to get bus reset\n");
+
+	ret = of_property_read_u32(pdev->dev.of_node, "allwinner,pwm-channels",
+				   &sun20i_chip->chip.npwm);
+	if (ret)
+		sun20i_chip->chip.npwm = 8;
+
+	/* Deassert reset */
+	ret = reset_control_deassert(sun20i_chip->rst);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to deassert reset\n");
+
+	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) {
+		reset_control_assert(sun20i_chip->rst);
+		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+	}
+
+	platform_set_drvdata(pdev, sun20i_chip);
+
+	return 0;
+}
+
+static void sun20i_pwm_remove(struct platform_device *pdev)
+{
+	struct sun20i_pwm_chip *sun20i_chip = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&sun20i_chip->chip);
+
+	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 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 | 11 +++++++++++
 1 file changed, 11 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..e24543b6aff7 100644
--- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
@@ -127,6 +127,17 @@ 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>;
+			status = "disabled";
+			#pwm-cells = <0x3>;
+		};
+
 		ccu: clock-controller@2001000 {
 			compatible = "allwinner,sun20i-d1-ccu";
 			reg = <0x2001000 0x1000>;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 949 bytes --]

On Tue, Jun 27, 2023 at 11:23:24AM +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.
> 
> 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.
> 
> Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>

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

Cheers,
Conor.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Palmer Dabbelt]

On Tue, 27 Jun 2023 01:23:23 PDT (-0700), privatesub2@gmail.com wrote:
> 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.
>
> v2:
>  - fix dt-bindings
>  - fix a remark in the driver
>
> v3:
>  - fix dt-bindings
>  - fix sunxi-d1s-t113.dtsi
>
> 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    |  86 +++++
>  .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    |  11 +
>  drivers/pwm/Kconfig                           |  10 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-sun20i.c                      | 322 ++++++++++++++++++
>  5 files changed, 430 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
>  create mode 100644 drivers/pwm/pwm-sun20i.c

Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

I'm not sure if this ended up somewhere, but I'm assuming it's not aimed 
at my tree.  LMK if you guys want me to pick it up, I'm dropping it from 
patchwork.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1718 bytes --]

Hello,

On Thu, Jul 06, 2023 at 10:08:47AM -0700, Palmer Dabbelt wrote:
> On Tue, 27 Jun 2023 01:23:23 PDT (-0700), privatesub2@gmail.com wrote:
> > 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.
> > 
> > v2:
> >  - fix dt-bindings
> >  - fix a remark in the driver
> > 
> > v3:
> >  - fix dt-bindings
> >  - fix sunxi-d1s-t113.dtsi
> > 
> > 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    |  86 +++++
> >  .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    |  11 +
> >  drivers/pwm/Kconfig                           |  10 +
> >  drivers/pwm/Makefile                          |   1 +
> >  drivers/pwm/pwm-sun20i.c                      | 322 ++++++++++++++++++
> >  5 files changed, 430 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> >  create mode 100644 drivers/pwm/pwm-sun20i.c
> 
> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
> 
> I'm not sure if this ended up somewhere, but I'm assuming it's not aimed at
> my tree.  LMK if you guys want me to pick it up, I'm dropping it from
> patchwork.

At least patch #2 should go via the pwm tree. It's on my list to review.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Philipp Zabel]

On Di, 2023-06-27 at 11:23 +0300, Aleksandr Shubin wrote:
[...]
> +static int sun20i_pwm_probe(struct platform_device *pdev)
> +{
[...]
> +	sun20i_chip->rst = devm_reset_control_get(&pdev->dev, NULL);

Please use devm_reset_control_get_exclusive() directly.

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 11269 bytes --]

Hello,

On Tue, Jun 27, 2023 at 11:23:25AM +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      |  10 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 333 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sun20i.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8df861b1f4a3..05c48a36969e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -594,6 +594,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
> 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..63e9c64e0e18
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun20i.c
> @@ -0,0 +1,322 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
> + *
> + * Limitations:
> + * - When the parameters change, current running period will not be completed
> + *   and run new settings immediately.
> + * - It output HIGH-Z state when PWM channel disabled.
> + *
> + * Copyright (c) 2023 Aleksandr Shubin <privatesub2@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +
> +#define PWM_CLK_CFG_REG(chan)		(0x20 + (((chan) >> 1) * 0x4))
> +#define PWM_CLK_SRC			GENMASK(8, 7)
> +#define PWM_CLK_DIV_M			GENMASK(3, 0)
> +
> +#define PWM_CLK_GATE_REG		0x40
> +#define PWM_CLK_BYPASS(chan)		BIT((chan) - 16)
> +#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			GENMASK(7, 0)
> +
> +#define PWM_PERIOD_REG(chan)		(0x104 + (chan) * 0x20)
> +#define PWM_ENTIRE_CYCLE		GENMASK(31, 16)
> +#define PWM_ACT_CYCLE			GENMASK(15, 0)

Can you please adapt the register field names to include the register
name? I'd use:

#define PWM_CTL(chan)		(0x100 + (chan) * 0x20)
#define PWM_CTL_ACT_STA			BIT(8)
#define PWM_CTL_PRESCAL_K		GENMASK(7, 0)

then you get a chance to spot when PWM_CLK_BYPASS(x) is written to
PWM_CLK_CFG.


> +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 ent_cycle, act_cycle;
> +	u8 prescal, div_id;
> +
> +	mutex_lock(&sun20i_chip->mutex);
> +
> +	val = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> +	div_id = FIELD_GET(PWM_CLK_DIV_M, val);
> +	if (FIELD_GET(PWM_CLK_SRC, val) == 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));
> +	state->polarity = (PWM_ACT_STA & val) ? PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
> +
> +	prescal = FIELD_GET(PWM_PRESCAL_K, val) + 1;

If PWM_PRESCAL_K is 0xff, prescal ends up being 0. This isn't right, is
it?

> +	val = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> +	state->enabled = (PWM_EN(pwm->hwpwm) & val) ? true : false;
> +
> +	val = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
> +	act_cycle = FIELD_GET(PWM_ACT_CYCLE, val);
> +	ent_cycle = FIELD_GET(PWM_ENTIRE_CYCLE, val);
> +	if (act_cycle > ent_cycle)
> +		act_cycle = ent_cycle;
> +

A comment that with the width of the used factors this cannot overflow
would be nice here.

> +	tmp = (u64)(act_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;

Can be simplified to:

	tmp = (u64)act_cycle * prescal << div_id * NSEC_PER_SEC;

> +	state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
> +	tmp = (u64)(ent_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> +	state->period = DIV_ROUND_UP_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)
> +{
> +	int ret = 0;
> +	u32 clk_gate, clk_cfg, pwm_en, ctl, period;
> +	u64 bus_rate, hosc_rate, clk_div, val;
> +	u16 prescaler, div_m;
> +	bool use_bus_clk, calc_div_m;
> +	struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> +
> +	mutex_lock(&sun20i_chip->mutex);
> +
> +	pwm_en = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> +
> +	if (state->enabled != pwm->state.enabled)
> +		clk_gate = sun20i_pwm_readl(sun20i_chip, PWM_CLK_GATE_REG);
> +
> +	if (state->enabled != pwm->state.enabled && !state->enabled) {
> +		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);
> +	}
> +
> +	if (state->polarity != pwm->state.polarity ||
> +	    state->duty_cycle != pwm->state.duty_cycle ||
> +	    state->period != pwm->state.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)) {
> +			/* if the neighbor channel is enable, check period only */
> +			use_bus_clk = FIELD_GET(PWM_CLK_SRC, clk_cfg) != 0;
> +			if (use_bus_clk)
> +				val = state->period * bus_rate;
> +			else
> +				val = state->period * hosc_rate;
> +			do_div(val, NSEC_PER_SEC);
> +
> +			div_m = FIELD_GET(PWM_CLK_DIV_M, clk_cfg);
> +			calc_div_m = false;
> +		} 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) {
> +					ret = -EINVAL;
> +					goto unlock_mutex;
> +				}
> +			}
> +			div_m = 0;
> +			calc_div_m = true;
> +
> +			/* set up the CLK_DIV_M and clock CLK_SRC */
> +			clk_cfg = FIELD_PREP(PWM_CLK_DIV_M, div_m);
> +			clk_cfg |= FIELD_PREP(PWM_CLK_SRC, use_bus_clk ? 1 : 0);
> +
> +			sun20i_pwm_writel(sun20i_chip, clk_cfg, PWM_CLK_CFG_REG(pwm->hwpwm));
> +		}
> +
> +		/* calculate prescaler, M factor, PWM entire cycle */
> +		clk_div = val;

This assignment is useless as it is overwritten in the loop below, isn't
it?

> +		for (prescaler = 0;; prescaler++) {
> +			if (prescaler >= 256) {
> +				if (calc_div_m) {
> +					prescaler = 0;
> +					div_m++;
> +					if (div_m >= 9) {
> +						ret = -EINVAL;
> +						goto unlock_mutex;
> +					}
> +				} else {
> +					ret = -EINVAL;
> +					goto unlock_mutex;
> +				}
> +			}
> +
> +			clk_div = val >> div_m;
> +			do_div(clk_div, prescaler + 1);
> +			if (clk_div <= 65534)
> +				break;

This can be calculated without a loop.

> +		}
> +
> +		period = FIELD_PREP(PWM_ENTIRE_CYCLE, clk_div);
> +
> +		/* set duty cycle */
> +		if (use_bus_clk)
> +			val = state->duty_cycle * bus_rate;
> +		else
> +			val = state->duty_cycle * hosc_rate;
> +		do_div(val, NSEC_PER_SEC);
> +		clk_div = val >> div_m;
> +		do_div(clk_div, prescaler + 1);
> +
> +		if (state->duty_cycle == state->period)
> +			clk_div++;

I don't understand that one. Can you explain that in a comment please?

> +		period |= FIELD_PREP(PWM_ACT_CYCLE, clk_div);
> +		sun20i_pwm_writel(sun20i_chip, period, PWM_PERIOD_REG(pwm->hwpwm));
> +
> +		ctl = FIELD_PREP(PWM_PRESCAL_K, prescaler);
> +		if (state->polarity == PWM_POLARITY_NORMAL)
> +			ctl |= PWM_ACT_STA;
> +
> +		sun20i_pwm_writel(sun20i_chip, ctl, PWM_CTL_REG(pwm->hwpwm));

Is this racy? I.e. does the write to PWM_PERIOD_REG(pwm->hwpwm) above
already has an effect before PWM_CTL_REG(pwm->hwpwm) is written?

> +	}
> +
> +	if (state->enabled != pwm->state.enabled && state->enabled) {
> +		clk_gate &= ~PWM_CLK_BYPASS(pwm->hwpwm);
> +		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);

This is (I guess) racy. If your PWM is running with

	.period = 10000
	.duty_cyle = 0
	.enabled = true

and you configure it to

	.period = 10000
	.duty_cyle = 10000
	.enabled = false

you get a short spike. For a enabled=true -> enabled=false transition
you should disable first before configuring duty+period (or skip the
latter completely).

> +	}
> +
> +unlock_mutex:
> +	mutex_unlock(&sun20i_chip->mutex);
> +
> +	return ret;
> +}

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jernej Škrabec]

Dne četrtek, 06. julij 2023 ob 22:20:57 CEST je Uwe Kleine-König napisal(a):
> Hello,
> 
> On Thu, Jul 06, 2023 at 10:08:47AM -0700, Palmer Dabbelt wrote:
> > On Tue, 27 Jun 2023 01:23:23 PDT (-0700), privatesub2@gmail.com wrote:
> > > 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.
> > > 
> > > v2:
> > >  - fix dt-bindings
> > >  - fix a remark in the driver
> > > 
> > > v3:
> > >  - fix dt-bindings
> > >  - fix sunxi-d1s-t113.dtsi
> > > 
> > > 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    |  86 +++++
> > >  .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    |  11 +
> > >  drivers/pwm/Kconfig                           |  10 +
> > >  drivers/pwm/Makefile                          |   1 +
> > >  drivers/pwm/pwm-sun20i.c                      | 322 ++++++++++++++++++
> > >  5 files changed, 430 insertions(+)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml create
> > >  mode 100644 drivers/pwm/pwm-sun20i.c
> > 
> > Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
> > 
> > I'm not sure if this ended up somewhere, but I'm assuming it's not aimed
> > at
> > my tree.  LMK if you guys want me to pick it up, I'm dropping it from
> > patchwork.
> 
> At least patch #2 should go via the pwm tree. It's on my list to review.

Rest will go through sunxi tree, although DT bindings patches are usually 
picked together with driver patches. I'm fine either way.

Best regards,
Jernej





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

Hi Uwe,
thanks for having a look!
Sorry for replying so late.

пн, 10 июл. 2023 г. в 12:14, Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>
> Hello,
>
> On Tue, Jun 27, 2023 at 11:23:25AM +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      |  10 ++
> >  drivers/pwm/Makefile     |   1 +
> >  drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 333 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-sun20i.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 8df861b1f4a3..05c48a36969e 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -594,6 +594,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
> > 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..63e9c64e0e18
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sun20i.c
> > @@ -0,0 +1,322 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
> > + *
> > + * Limitations:
> > + * - When the parameters change, current running period will not be completed
> > + *   and run new settings immediately.
> > + * - It output HIGH-Z state when PWM channel disabled.
> > + *
> > + * Copyright (c) 2023 Aleksandr Shubin <privatesub2@gmail.com>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/reset.h>
> > +
> > +#define PWM_CLK_CFG_REG(chan)                (0x20 + (((chan) >> 1) * 0x4))
> > +#define PWM_CLK_SRC                  GENMASK(8, 7)
> > +#define PWM_CLK_DIV_M                        GENMASK(3, 0)
> > +
> > +#define PWM_CLK_GATE_REG             0x40
> > +#define PWM_CLK_BYPASS(chan)         BIT((chan) - 16)
> > +#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                        GENMASK(7, 0)
> > +
> > +#define PWM_PERIOD_REG(chan)         (0x104 + (chan) * 0x20)
> > +#define PWM_ENTIRE_CYCLE             GENMASK(31, 16)
> > +#define PWM_ACT_CYCLE                        GENMASK(15, 0)
>
> Can you please adapt the register field names to include the register
> name? I'd use:
>
> #define PWM_CTL(chan)           (0x100 + (chan) * 0x20)
> #define PWM_CTL_ACT_STA                 BIT(8)
> #define PWM_CTL_PRESCAL_K               GENMASK(7, 0)
>
> then you get a chance to spot when PWM_CLK_BYPASS(x) is written to
> PWM_CLK_CFG.
>
>
> > +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 ent_cycle, act_cycle;
> > +     u8 prescal, div_id;
> > +
> > +     mutex_lock(&sun20i_chip->mutex);
> > +
> > +     val = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> > +     div_id = FIELD_GET(PWM_CLK_DIV_M, val);
> > +     if (FIELD_GET(PWM_CLK_SRC, val) == 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));
> > +     state->polarity = (PWM_ACT_STA & val) ? PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
> > +
> > +     prescal = FIELD_GET(PWM_PRESCAL_K, val) + 1;
>
> If PWM_PRESCAL_K is 0xff, prescal ends up being 0. This isn't right, is
> it?
>
> > +     val = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > +     state->enabled = (PWM_EN(pwm->hwpwm) & val) ? true : false;
> > +
> > +     val = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
> > +     act_cycle = FIELD_GET(PWM_ACT_CYCLE, val);
> > +     ent_cycle = FIELD_GET(PWM_ENTIRE_CYCLE, val);
> > +     if (act_cycle > ent_cycle)
> > +             act_cycle = ent_cycle;
> > +
>
> A comment that with the width of the used factors this cannot overflow
> would be nice here.
>
> > +     tmp = (u64)(act_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
>
> Can be simplified to:
>
>         tmp = (u64)act_cycle * prescal << div_id * NSEC_PER_SEC;
>
> > +     state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
> > +     tmp = (u64)(ent_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> > +     state->period = DIV_ROUND_UP_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)
> > +{
> > +     int ret = 0;
> > +     u32 clk_gate, clk_cfg, pwm_en, ctl, period;
> > +     u64 bus_rate, hosc_rate, clk_div, val;
> > +     u16 prescaler, div_m;
> > +     bool use_bus_clk, calc_div_m;
> > +     struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> > +
> > +     mutex_lock(&sun20i_chip->mutex);
> > +
> > +     pwm_en = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > +
> > +     if (state->enabled != pwm->state.enabled)
> > +             clk_gate = sun20i_pwm_readl(sun20i_chip, PWM_CLK_GATE_REG);
> > +
> > +     if (state->enabled != pwm->state.enabled && !state->enabled) {
> > +             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);
> > +     }
> > +
> > +     if (state->polarity != pwm->state.polarity ||
> > +         state->duty_cycle != pwm->state.duty_cycle ||
> > +         state->period != pwm->state.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)) {
> > +                     /* if the neighbor channel is enable, check period only */
> > +                     use_bus_clk = FIELD_GET(PWM_CLK_SRC, clk_cfg) != 0;
> > +                     if (use_bus_clk)
> > +                             val = state->period * bus_rate;
> > +                     else
> > +                             val = state->period * hosc_rate;
> > +                     do_div(val, NSEC_PER_SEC);
> > +
> > +                     div_m = FIELD_GET(PWM_CLK_DIV_M, clk_cfg);
> > +                     calc_div_m = false;
> > +             } 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) {
> > +                                     ret = -EINVAL;
> > +                                     goto unlock_mutex;
> > +                             }
> > +                     }
> > +                     div_m = 0;
> > +                     calc_div_m = true;
> > +
> > +                     /* set up the CLK_DIV_M and clock CLK_SRC */
> > +                     clk_cfg = FIELD_PREP(PWM_CLK_DIV_M, div_m);
> > +                     clk_cfg |= FIELD_PREP(PWM_CLK_SRC, use_bus_clk ? 1 : 0);
> > +
> > +                     sun20i_pwm_writel(sun20i_chip, clk_cfg, PWM_CLK_CFG_REG(pwm->hwpwm));
> > +             }
> > +
> > +             /* calculate prescaler, M factor, PWM entire cycle */
> > +             clk_div = val;
>
> This assignment is useless as it is overwritten in the loop below, isn't
> it?
>
> > +             for (prescaler = 0;; prescaler++) {
> > +                     if (prescaler >= 256) {
> > +                             if (calc_div_m) {
> > +                                     prescaler = 0;
> > +                                     div_m++;
> > +                                     if (div_m >= 9) {
> > +                                             ret = -EINVAL;
> > +                                             goto unlock_mutex;
> > +                                     }
> > +                             } else {
> > +                                     ret = -EINVAL;
> > +                                     goto unlock_mutex;
> > +                             }
> > +                     }
> > +
> > +                     clk_div = val >> div_m;
> > +                     do_div(clk_div, prescaler + 1);
> > +                     if (clk_div <= 65534)
> > +                             break;
>
> This can be calculated without a loop.

Point me please where can I see the calculation of two divisors without a loop?
>
> > +             }
> > +
> > +             period = FIELD_PREP(PWM_ENTIRE_CYCLE, clk_div);
> > +
> > +             /* set duty cycle */
> > +             if (use_bus_clk)
> > +                     val = state->duty_cycle * bus_rate;
> > +             else
> > +                     val = state->duty_cycle * hosc_rate;
> > +             do_div(val, NSEC_PER_SEC);
> > +             clk_div = val >> div_m;
> > +             do_div(clk_div, prescaler + 1);
> > +
> > +             if (state->duty_cycle == state->period)
> > +                     clk_div++;
>
> I don't understand that one. Can you explain that in a comment please?

The formula of the output period and the duty-cycle for PWM are as follows.
T period = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * (PPR0.PWM_ENTIRE_CYCLE + 1)
T high-level = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * PPR0.PWM_ACT_CYCLE
Duty-cycle = T high-level / T period
In accordance with this formula, in order to set the duty-cycle to 100%,
it is necessary that PWM_ACT_CYCLE >= PWM_ENTIRE_CYCLE + 1
>
> > +             period |= FIELD_PREP(PWM_ACT_CYCLE, clk_div);
> > +             sun20i_pwm_writel(sun20i_chip, period, PWM_PERIOD_REG(pwm->hwpwm));
> > +
> > +             ctl = FIELD_PREP(PWM_PRESCAL_K, prescaler);
> > +             if (state->polarity == PWM_POLARITY_NORMAL)
> > +                     ctl |= PWM_ACT_STA;
> > +
> > +             sun20i_pwm_writel(sun20i_chip, ctl, PWM_CTL_REG(pwm->hwpwm));
>
> Is this racy? I.e. does the write to PWM_PERIOD_REG(pwm->hwpwm) above
> already has an effect before PWM_CTL_REG(pwm->hwpwm) is written?
>
> > +     }
> > +
> > +     if (state->enabled != pwm->state.enabled && state->enabled) {
> > +             clk_gate &= ~PWM_CLK_BYPASS(pwm->hwpwm);
> > +             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);
>
> This is (I guess) racy. If your PWM is running with
>
>         .period = 10000
>         .duty_cyle = 0
>         .enabled = true
>
> and you configure it to
>
>         .period = 10000
>         .duty_cyle = 10000
>         .enabled = false
>
> you get a short spike. For a enabled=true -> enabled=false transition
> you should disable first before configuring duty+period (or skip the
> latter completely).

When switching enabled=true -> enabled=false then before setting the period,
the enable register will be written false and the pvm will become inactive.
this is the place:
if (state->enabled != pwm->state.enabled && !state->enabled) {
>
> > +     }
> > +
> > +unlock_mutex:
> > +     mutex_unlock(&sun20i_chip->mutex);
> > +
> > +     return ret;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Cheers,
Aleksandr

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 17262 bytes --]

Hello,

On Fri, Aug 04, 2023 at 10:19:26AM +0300, Александр Шубин wrote:
> пн, 10 июл. 2023 г. в 12:14, Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Tue, Jun 27, 2023 at 11:23:25AM +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      |  10 ++
> > >  drivers/pwm/Makefile     |   1 +
> > >  drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 333 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-sun20i.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 8df861b1f4a3..05c48a36969e 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -594,6 +594,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
> > > 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..63e9c64e0e18
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sun20i.c
> > > @@ -0,0 +1,322 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
> > > + *
> > > + * Limitations:
> > > + * - When the parameters change, current running period will not be completed
> > > + *   and run new settings immediately.
> > > + * - It output HIGH-Z state when PWM channel disabled.
> > > + *
> > > + * Copyright (c) 2023 Aleksandr Shubin <privatesub2@gmail.com>
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/reset.h>
> > > +
> > > +#define PWM_CLK_CFG_REG(chan)                (0x20 + (((chan) >> 1) * 0x4))
> > > +#define PWM_CLK_SRC                  GENMASK(8, 7)
> > > +#define PWM_CLK_DIV_M                        GENMASK(3, 0)
> > > +
> > > +#define PWM_CLK_GATE_REG             0x40
> > > +#define PWM_CLK_BYPASS(chan)         BIT((chan) - 16)
> > > +#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                        GENMASK(7, 0)
> > > +
> > > +#define PWM_PERIOD_REG(chan)         (0x104 + (chan) * 0x20)
> > > +#define PWM_ENTIRE_CYCLE             GENMASK(31, 16)
> > > +#define PWM_ACT_CYCLE                        GENMASK(15, 0)
> >
> > Can you please adapt the register field names to include the register
> > name? I'd use:
> >
> > #define PWM_CTL(chan)           (0x100 + (chan) * 0x20)
> > #define PWM_CTL_ACT_STA                 BIT(8)
> > #define PWM_CTL_PRESCAL_K               GENMASK(7, 0)
> >
> > then you get a chance to spot when PWM_CLK_BYPASS(x) is written to
> > PWM_CLK_CFG.
> >
> >
> > > +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 ent_cycle, act_cycle;
> > > +     u8 prescal, div_id;
> > > +
> > > +     mutex_lock(&sun20i_chip->mutex);
> > > +
> > > +     val = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > +     div_id = FIELD_GET(PWM_CLK_DIV_M, val);
> > > +     if (FIELD_GET(PWM_CLK_SRC, val) == 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));
> > > +     state->polarity = (PWM_ACT_STA & val) ? PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
> > > +
> > > +     prescal = FIELD_GET(PWM_PRESCAL_K, val) + 1;
> >
> > If PWM_PRESCAL_K is 0xff, prescal ends up being 0. This isn't right, is
> > it?
> >
> > > +     val = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > > +     state->enabled = (PWM_EN(pwm->hwpwm) & val) ? true : false;
> > > +
> > > +     val = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
> > > +     act_cycle = FIELD_GET(PWM_ACT_CYCLE, val);
> > > +     ent_cycle = FIELD_GET(PWM_ENTIRE_CYCLE, val);
> > > +     if (act_cycle > ent_cycle)
> > > +             act_cycle = ent_cycle;
> > > +
> >
> > A comment that with the width of the used factors this cannot overflow
> > would be nice here.
> >
> > > +     tmp = (u64)(act_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> >
> > Can be simplified to:
> >
> >         tmp = (u64)act_cycle * prescal << div_id * NSEC_PER_SEC;
> >
> > > +     state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
> > > +     tmp = (u64)(ent_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> > > +     state->period = DIV_ROUND_UP_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)
> > > +{
> > > +     int ret = 0;
> > > +     u32 clk_gate, clk_cfg, pwm_en, ctl, period;
> > > +     u64 bus_rate, hosc_rate, clk_div, val;
> > > +     u16 prescaler, div_m;
> > > +     bool use_bus_clk, calc_div_m;
> > > +     struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> > > +
> > > +     mutex_lock(&sun20i_chip->mutex);
> > > +
> > > +     pwm_en = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > > +
> > > +     if (state->enabled != pwm->state.enabled)
> > > +             clk_gate = sun20i_pwm_readl(sun20i_chip, PWM_CLK_GATE_REG);
> > > +
> > > +     if (state->enabled != pwm->state.enabled && !state->enabled) {
> > > +             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);
> > > +     }
> > > +
> > > +     if (state->polarity != pwm->state.polarity ||
> > > +         state->duty_cycle != pwm->state.duty_cycle ||
> > > +         state->period != pwm->state.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)) {
> > > +                     /* if the neighbor channel is enable, check period only */
> > > +                     use_bus_clk = FIELD_GET(PWM_CLK_SRC, clk_cfg) != 0;
> > > +                     if (use_bus_clk)
> > > +                             val = state->period * bus_rate;
> > > +                     else
> > > +                             val = state->period * hosc_rate;
> > > +                     do_div(val, NSEC_PER_SEC);
> > > +
> > > +                     div_m = FIELD_GET(PWM_CLK_DIV_M, clk_cfg);
> > > +                     calc_div_m = false;
> > > +             } 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) {
> > > +                                     ret = -EINVAL;
> > > +                                     goto unlock_mutex;
> > > +                             }
> > > +                     }
> > > +                     div_m = 0;
> > > +                     calc_div_m = true;
> > > +
> > > +                     /* set up the CLK_DIV_M and clock CLK_SRC */
> > > +                     clk_cfg = FIELD_PREP(PWM_CLK_DIV_M, div_m);
> > > +                     clk_cfg |= FIELD_PREP(PWM_CLK_SRC, use_bus_clk ? 1 : 0);
> > > +
> > > +                     sun20i_pwm_writel(sun20i_chip, clk_cfg, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > +             }
> > > +
> > > +             /* calculate prescaler, M factor, PWM entire cycle */
> > > +             clk_div = val;
> >
> > This assignment is useless as it is overwritten in the loop below, isn't
> > it?
> >
> > > +             for (prescaler = 0;; prescaler++) {
> > > +                     if (prescaler >= 256) {
> > > +                             if (calc_div_m) {
> > > +                                     prescaler = 0;
> > > +                                     div_m++;
> > > +                                     if (div_m >= 9) {
> > > +                                             ret = -EINVAL;
> > > +                                             goto unlock_mutex;
> > > +                                     }
> > > +                             } else {
> > > +                                     ret = -EINVAL;
> > > +                                     goto unlock_mutex;
> > > +                             }
> > > +                     }
> > > +
> > > +                     clk_div = val >> div_m;
> > > +                     do_div(clk_div, prescaler + 1);
> > > +                     if (clk_div <= 65534)
> > > +                             break;
> >
> > This can be calculated without a loop.
> 
> Point me please where can I see the calculation of two divisors without a loop?

for a given value of div_m you search the smallest prescaler such that

	(val >> div_m) // (prescaler + 1) ≤ 65534

(Using Python syntax where // denotes the usual
round-down-to-next-integer division and / normal exact division.)

This is equivalent to:

	  (val >> div_m) // (prescaler + 1) ≤ 65534
	⟺ (val >> div_m) ≤ 65534 * (prescaler + 1) + prescaler
	⟺ (val >> div_m) ≤ 65535 * prescaler + 65534
⟺ (val >> div_m) - 65534 ≤ 65535 * prescaler
	⟺ ((val >> div_m) - 65534) / 65535 ≤ prescaler

And as prescaler is integer, this is

	  ...
	⟺ ((val >> div_m) - 65534) // 65535 ≤ prescaler

So the prescaler value you're looking for is:

	 ((val >> div_m) - 65534) // 65535

And then you have to pick the smallest div_m such that prescaler ≤ 255:

	  ((val >> div_m) - 65534) // 65535 ≤ 255
	⟺ (val >> div_m) - 65534 ≤ 255 * 65535 + 65534
	⟺ val >> div_m ≤ 255 * 65535 + 2 * 65534
	⟺ val >> div_m ≤ 16842493
	⟺ val >> div_m < 16842494

so div_m is fls((val) / 16842494).

You might want to double check this and explain the algorithm in a
comment similar to the above calculation.

> > > +             }
> > > +
> > > +             period = FIELD_PREP(PWM_ENTIRE_CYCLE, clk_div);
> > > +
> > > +             /* set duty cycle */
> > > +             if (use_bus_clk)
> > > +                     val = state->duty_cycle * bus_rate;
> > > +             else
> > > +                     val = state->duty_cycle * hosc_rate;

maybe better use:

	if (use_bus_clk)
		rate = bus_rate;
	else
		rate = hosc_rate;

	val = state->duty_cycle * rate

> > > +             do_div(val, NSEC_PER_SEC);
> > > +             clk_div = val >> div_m;
> > > +             do_div(clk_div, prescaler + 1);
> > > +
> > > +             if (state->duty_cycle == state->period)
> > > +                     clk_div++;
> >
> > I don't understand that one. Can you explain that in a comment please?
> 
> The formula of the output period and the duty-cycle for PWM are as follows.
> T period = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * (PPR0.PWM_ENTIRE_CYCLE + 1)

That can be simpler written as:

	Tperiod = PWM0_PRESCALE_K / PWM01_CLK * (PPR0.PWM_ENTIRE_CYCLE + 1)

right?

> T high-level = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * PPR0.PWM_ACT_CYCLE

simlar:

	Thigh = PWM0_PRESCALE_K / PWM01_CLK * PPR0.PWM_ACT_CYCLE

> Duty-cycle = T high-level / T period
> In accordance with this formula, in order to set the duty-cycle to 100%,
> it is necessary that PWM_ACT_CYCLE >= PWM_ENTIRE_CYCLE + 1

The +1 is also relevant for duty_cycles other than state->period, right?

> > > +             period |= FIELD_PREP(PWM_ACT_CYCLE, clk_div);
> > > +             sun20i_pwm_writel(sun20i_chip, period, PWM_PERIOD_REG(pwm->hwpwm));
> > > +
> > > +             ctl = FIELD_PREP(PWM_PRESCAL_K, prescaler);
> > > +             if (state->polarity == PWM_POLARITY_NORMAL)
> > > +                     ctl |= PWM_ACT_STA;
> > > +
> > > +             sun20i_pwm_writel(sun20i_chip, ctl, PWM_CTL_REG(pwm->hwpwm));
> >
> > Is this racy? I.e. does the write to PWM_PERIOD_REG(pwm->hwpwm) above
> > already has an effect before PWM_CTL_REG(pwm->hwpwm) is written?
> >
> > > +     }
> > > +
> > > +     if (state->enabled != pwm->state.enabled && state->enabled) {
> > > +             clk_gate &= ~PWM_CLK_BYPASS(pwm->hwpwm);
> > > +             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);
> >
> > This is (I guess) racy. If your PWM is running with
> >
> >         .period = 10000
> >         .duty_cyle = 0
> >         .enabled = true
> >
> > and you configure it to
> >
> >         .period = 10000
> >         .duty_cyle = 10000
> >         .enabled = false
> >
> > you get a short spike. For a enabled=true -> enabled=false transition
> > you should disable first before configuring duty+period (or skip the
> > latter completely).
> 
> When switching enabled=true -> enabled=false then before setting the period,
> the enable register will be written false and the pvm will become inactive.
> this is the place:
> if (state->enabled != pwm->state.enabled && !state->enabled) {

Ah, it seems I missed that (or I just don't understand any more what
meant back then :-)

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel