[PATCH 0/5] Add support for Allwinner H616 PWM

[PATCH 0/5] Add support for Allwinner H616 PWM

[Hironori KIKUCHI]

Add support for the Allwinner H616 PWM, building on top of Aleksandr's
Allwinner D1 PWM driver v9.

Additionally, the 4th and 5th patches implement the proposed method
for delegating the clock source and DIV_M selection to the Device Tree.
While it works well without these patches with the original behavior,
applying them enables fine-grained control of PWM resolution and
prevents non-deterministic behavior dependent on the enabling order.

I have only been able to test on H700 (H616 variant) using an
oscilloscope. I would greatly appreciate it if someone could test
this patch series on the D1 or other models.

Regards,
kikuchan.

Hironori KIKUCHI (5):
  pwm: sun20i: Use devm_pwmchip_alloc() helper
  pwm: sun20i: Add support for Allwinner H616 PWM
  dt-bindings: pwm: sun20i: Add compatible string for Allwinner H616 PWM
  pwm: sun20i: Delegating the clock source and DIV_M to the Device Tree
  dt-bindings: pwm: sun20i: Add options to select a clock source and
    DIV_M

 .../bindings/pwm/allwinner,sun20i-pwm.yaml    |  20 ++
 drivers/pwm/pwm-sun20i.c                      | 326 ++++++++++--------
 2 files changed, 201 insertions(+), 145 deletions(-)

-- 
2.45.1

[PATCH 1/5] pwm: sun20i: Use devm_pwmchip_alloc() helper

[Hironori KIKUCHI]

This patch fixes a compile error by using the devm_pwmchip_alloc() helper
function along the way.

Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
---
 drivers/pwm/pwm-sun20i.c | 45 ++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
index 3e3b5b138b3..93782023af6 100644
--- a/drivers/pwm/pwm-sun20i.c
+++ b/drivers/pwm/pwm-sun20i.c
@@ -102,7 +102,7 @@ struct sun20i_pwm_chip {
 
 static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
 {
-	return container_of(chip, struct sun20i_pwm_chip, chip);
+	return pwmchip_get_drvdata(chip);
 }
 
 static inline u32 sun20i_pwm_readl(struct sun20i_pwm_chip *chip,
@@ -308,12 +308,31 @@ static void sun20i_pwm_reset_ctrl_release(void *data)
 
 static int sun20i_pwm_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct sun20i_pwm_chip *sun20i_chip;
+	const struct sun20i_pwm_data *data;
+	u32 npwm;
 	int ret;
 
-	sun20i_chip = devm_kzalloc(&pdev->dev, sizeof(*sun20i_chip), GFP_KERNEL);
-	if (!sun20i_chip)
-		return -ENOMEM;
+	data = of_device_get_match_data(&pdev->dev);
+	if (!data)
+		return -ENODEV;
+
+	ret = of_property_read_u32(pdev->dev.of_node, "allwinner,pwm-channels", &npwm);
+	if (ret)
+		npwm = 8;
+
+	if (npwm > 16) {
+		dev_info(&pdev->dev, "Limiting number of PWM lines from %u to 16", npwm);
+		npwm = 16;
+	}
+
+	chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*sun20i_chip));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	sun20i_chip = to_sun20i_pwm_chip(chip);
+
+	sun20i_chip->data = data;
 
 	sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(sun20i_chip->base))
@@ -339,17 +358,6 @@ static int sun20i_pwm_probe(struct platform_device *pdev)
 		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;
-
-	if (sun20i_chip->chip.npwm > 16) {
-		dev_info(&pdev->dev, "Limiting number of PWM lines from %u to 16",
-			 sun20i_chip->chip.npwm);
-		sun20i_chip->chip.npwm = 16;
-	}
-
 	/* Deassert reset */
 	ret = reset_control_deassert(sun20i_chip->rst);
 	if (ret)
@@ -359,17 +367,14 @@ static int sun20i_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	sun20i_chip->chip.dev = &pdev->dev;
-	sun20i_chip->chip.ops = &sun20i_pwm_ops;
+	chip->ops = &sun20i_pwm_ops;
 
 	mutex_init(&sun20i_chip->mutex);
 
-	ret = devm_pwmchip_add(&pdev->dev, &sun20i_chip->chip);
+	ret = devm_pwmchip_add(&pdev->dev, chip);
 	if (ret < 0)
 		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
 
-	platform_set_drvdata(pdev, sun20i_chip);
-
 	return 0;
 }
 
-- 
2.45.1

[PATCH 2/5] pwm: sun20i: Add support for Allwinner H616 PWM

[Hironori KIKUCHI]

Allwinner H616 SoC has a PWM controller similar to the one
in the D1, which is supported by the pwm-sun20i driver.

This patch adds support for the Allwinner H616 PWM.
The main difference is in the register layout. Specifically, the
GATING flag is placed in the PCCR register instead of the
individual PCGR register. Thus, it must be handled properly.

Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
---
 drivers/pwm/pwm-sun20i.c | 109 ++++++++++++++++++++++++++++++---------
 1 file changed, 86 insertions(+), 23 deletions(-)

diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
index 93782023af6..d07ce0ebd2a 100644
--- a/drivers/pwm/pwm-sun20i.c
+++ b/drivers/pwm/pwm-sun20i.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
+ * PWM Controller Driver for sunxi platforms (D1, T113-S3, R329 and H616)
  *
  * Limitations:
  * - When the parameters change, current running period will not be completed
@@ -20,8 +20,17 @@
 #include <linux/pwm.h>
 #include <linux/reset.h>
 
+#define SUN20I_PWM_REG_OFFSET_PER_D1		(0x0080)
+#define SUN20I_PWM_REG_OFFSET_PCR_D1		(0x0100 + 0x0000)
+#define SUN20I_PWM_REG_OFFSET_PPR_D1		(0x0100 + 0x0004)
+#define SUN20I_PWM_REG_OFFSET_PER_H616		(0x0040)
+#define SUN20I_PWM_REG_OFFSET_PCR_H616		(0x0060 + 0x0000)
+#define SUN20I_PWM_REG_OFFSET_PPR_H616		(0x0060 + 0x0004)
+
 #define SUN20I_PWM_CLK_CFG(chan)		(0x20 + (((chan) >> 1) * 0x4))
 #define SUN20I_PWM_CLK_CFG_SRC			GENMASK(8, 7)
+#define SUN20I_PWM_CLK_CFG_BYPASS(chan)		BIT(5 + ((chan) & 1))
+#define SUN20I_PWM_CLK_CFG_GATING		BIT(4)
 #define SUN20I_PWM_CLK_CFG_DIV_M		GENMASK(3, 0)
 #define SUN20I_PWM_CLK_DIV_M_MAX		8
 
@@ -29,15 +38,15 @@
 #define SUN20I_PWM_CLK_GATE_BYPASS(chan)	BIT((chan) + 16)
 #define SUN20I_PWM_CLK_GATE_GATING(chan)	BIT(chan)
 
-#define SUN20I_PWM_ENABLE			0x80
+#define SUN20I_PWM_ENABLE(chip)			((chip)->data->reg_per)
 #define SUN20I_PWM_ENABLE_EN(chan)		BIT(chan)
 
-#define SUN20I_PWM_CTL(chan)			(0x100 + (chan) * 0x20)
+#define SUN20I_PWM_CTL(chip, chan)		((chip)->data->reg_pcr + (chan) * 0x20)
 #define SUN20I_PWM_CTL_ACT_STA			BIT(8)
 #define SUN20I_PWM_CTL_PRESCAL_K		GENMASK(7, 0)
 #define SUN20I_PWM_CTL_PRESCAL_K_MAX		field_max(SUN20I_PWM_CTL_PRESCAL_K)
 
-#define SUN20I_PWM_PERIOD(chan)			(0x104 + (chan) * 0x20)
+#define SUN20I_PWM_PERIOD(chip, chan)		((chip)->data->reg_ppr + (chan) * 0x20)
 #define SUN20I_PWM_PERIOD_ENTIRE_CYCLE		GENMASK(31, 16)
 #define SUN20I_PWM_PERIOD_ACT_CYCLE		GENMASK(15, 0)
 
@@ -91,6 +100,13 @@
  */
 #define SUN20I_PWM_MAGIC			(255 * 65537 + 2 * 65536 + 1)
 
+struct sun20i_pwm_data {
+	unsigned long reg_per;
+	unsigned long reg_pcr;
+	unsigned long reg_ppr;
+	bool has_pcgr;
+};
+
 struct sun20i_pwm_chip {
 	struct clk *clk_bus, *clk_hosc, *clk_apb;
 	struct reset_control *rst;
@@ -98,6 +114,7 @@ struct sun20i_pwm_chip {
 	void __iomem *base;
 	/* Mutex to protect pwm apply state */
 	struct mutex mutex;
+	const struct sun20i_pwm_data *data;
 };
 
 static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
@@ -139,16 +156,16 @@ static int sun20i_pwm_get_state(struct pwm_chip *chip,
 	else
 		clk_rate = clk_get_rate(sun20i_chip->clk_apb);
 
-	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
+	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(sun20i_chip, pwm->hwpwm));
 	state->polarity = (SUN20I_PWM_CTL_ACT_STA & val) ?
 			   PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
 
 	prescale_k = FIELD_GET(SUN20I_PWM_CTL_PRESCAL_K, val) + 1;
 
-	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
+	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE(sun20i_chip));
 	state->enabled = (SUN20I_PWM_ENABLE_EN(pwm->hwpwm) & val) ? true : false;
 
-	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_PERIOD(pwm->hwpwm));
+	val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_PERIOD(sun20i_chip, pwm->hwpwm));
 
 	mutex_unlock(&sun20i_chip->mutex);
 
@@ -187,23 +204,32 @@ static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	mutex_lock(&sun20i_chip->mutex);
 
-	pwm_en = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
+	pwm_en = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE(sun20i_chip));
 
-	if (state->enabled != pwm->state.enabled) {
-		clk_gate = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_GATE);
-
-		if (!state->enabled) {
+	if (state->enabled != pwm->state.enabled && !state->enabled) {
+		if (sun20i_chip->data->has_pcgr) {
+			/* Disabling the gate via PWM Clock Gating Register */
+			clk_gate = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_GATE);
 			clk_gate &= ~SUN20I_PWM_CLK_GATE_GATING(pwm->hwpwm);
-			pwm_en &= ~SUN20I_PWM_ENABLE_EN(pwm->hwpwm);
-			sun20i_pwm_writel(sun20i_chip, pwm_en, SUN20I_PWM_ENABLE);
 			sun20i_pwm_writel(sun20i_chip, clk_gate, SUN20I_PWM_CLK_GATE);
+		} else if (!(pwm_en & SUN20I_PWM_ENABLE_EN(pwm->hwpwm ^ 1))) {
+			/*
+			 * Disabling the gate via PWM Clock Configuration Register
+			 * if and only if the counterpart channel is disabled
+			 */
+			clk_cfg = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
+			clk_cfg &= ~SUN20I_PWM_CLK_CFG_GATING;
+			sun20i_pwm_writel(sun20i_chip, clk_cfg, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
 		}
+
+		pwm_en &= ~SUN20I_PWM_ENABLE_EN(pwm->hwpwm);
+		sun20i_pwm_writel(sun20i_chip, pwm_en, sun20i_chip->data->reg_per);
 	}
 
 	if (state->polarity != pwm->state.polarity ||
 	    state->duty_cycle != pwm->state.duty_cycle ||
 	    state->period != pwm->state.period) {
-		ctl = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
+		ctl = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(sun20i_chip, pwm->hwpwm));
 		clk_cfg = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
 		hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
 		bus_rate = clk_get_rate(sun20i_chip->clk_apb);
@@ -234,7 +260,8 @@ static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			}
 
 			/* set up the CLK_DIV_M and clock CLK_SRC */
-			clk_cfg = FIELD_PREP(SUN20I_PWM_CLK_CFG_DIV_M, div_m);
+			clk_cfg &= ~(SUN20I_PWM_CLK_CFG_DIV_M | SUN20I_PWM_CLK_CFG_SRC);
+			clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_DIV_M, div_m);
 			clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_SRC, use_bus_clk);
 
 			sun20i_pwm_writel(sun20i_chip, clk_cfg, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
@@ -265,21 +292,33 @@ static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		 * Duty-cycle = T high-level / T period
 		 */
 		reg_period |= FIELD_PREP(SUN20I_PWM_PERIOD_ACT_CYCLE, act_cycle);
-		sun20i_pwm_writel(sun20i_chip, reg_period, SUN20I_PWM_PERIOD(pwm->hwpwm));
+		sun20i_pwm_writel(sun20i_chip, reg_period,
+			SUN20I_PWM_PERIOD(sun20i_chip, pwm->hwpwm));
 
 		ctl = FIELD_PREP(SUN20I_PWM_CTL_PRESCAL_K, prescale_k);
 		if (state->polarity == PWM_POLARITY_NORMAL)
 			ctl |= SUN20I_PWM_CTL_ACT_STA;
 
-		sun20i_pwm_writel(sun20i_chip, ctl, SUN20I_PWM_CTL(pwm->hwpwm));
+		sun20i_pwm_writel(sun20i_chip, ctl, SUN20I_PWM_CTL(sun20i_chip, pwm->hwpwm));
 	}
 
 	if (state->enabled != pwm->state.enabled && state->enabled) {
-		clk_gate &= ~SUN20I_PWM_CLK_GATE_BYPASS(pwm->hwpwm);
-		clk_gate |= SUN20I_PWM_CLK_GATE_GATING(pwm->hwpwm);
+		if (sun20i_chip->data->has_pcgr) {
+			/* Enabling the gate via PWM Clock Gating Register */
+			clk_gate = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_GATE);
+			clk_gate &= ~SUN20I_PWM_CLK_GATE_BYPASS(pwm->hwpwm);
+			clk_gate |= SUN20I_PWM_CLK_GATE_GATING(pwm->hwpwm);
+			sun20i_pwm_writel(sun20i_chip, clk_gate, SUN20I_PWM_CLK_GATE);
+		} else {
+			/* Enabling the gate via PWM Clock Configuration Register */
+			clk_cfg = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
+			clk_cfg &= ~SUN20I_PWM_CLK_CFG_BYPASS(pwm->hwpwm);
+			clk_cfg |= SUN20I_PWM_CLK_CFG_GATING;
+			sun20i_pwm_writel(sun20i_chip, clk_cfg, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
+		}
+
 		pwm_en |= SUN20I_PWM_ENABLE_EN(pwm->hwpwm);
-		sun20i_pwm_writel(sun20i_chip, pwm_en, SUN20I_PWM_ENABLE);
-		sun20i_pwm_writel(sun20i_chip, clk_gate, SUN20I_PWM_CLK_GATE);
+		sun20i_pwm_writel(sun20i_chip, pwm_en, SUN20I_PWM_ENABLE(sun20i_chip));
 	}
 
 unlock_mutex:
@@ -293,8 +332,29 @@ static const struct pwm_ops sun20i_pwm_ops = {
 	.get_state = sun20i_pwm_get_state,
 };
 
+static const struct sun20i_pwm_data sun20i_d1_pwm_data = {
+	.reg_per = SUN20I_PWM_REG_OFFSET_PER_D1,
+	.reg_pcr = SUN20I_PWM_REG_OFFSET_PCR_D1,
+	.reg_ppr = SUN20I_PWM_REG_OFFSET_PPR_D1,
+	.has_pcgr = true,
+};
+
+static const struct sun20i_pwm_data sun50i_h616_pwm_data = {
+	.reg_per = SUN20I_PWM_REG_OFFSET_PER_H616,
+	.reg_pcr = SUN20I_PWM_REG_OFFSET_PCR_H616,
+	.reg_ppr = SUN20I_PWM_REG_OFFSET_PPR_H616,
+	.has_pcgr = false,
+};
+
 static const struct of_device_id sun20i_pwm_dt_ids[] = {
-	{ .compatible = "allwinner,sun20i-d1-pwm" },
+	{
+		.compatible = "allwinner,sun20i-d1-pwm",
+		.data = &sun20i_d1_pwm_data
+	},
+	{
+		.compatible = "allwinner,sun50i-h616-pwm",
+		.data = &sun50i_h616_pwm_data
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, sun20i_pwm_dt_ids);
@@ -338,6 +398,8 @@ static int sun20i_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(sun20i_chip->base))
 		return PTR_ERR(sun20i_chip->base);
 
+	sun20i_chip->data = data;
+
 	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),
@@ -388,5 +450,6 @@ static struct platform_driver sun20i_pwm_driver = {
 module_platform_driver(sun20i_pwm_driver);
 
 MODULE_AUTHOR("Aleksandr Shubin <privatesub2@gmail.com>");
+MODULE_AUTHOR("Hironori KIKUCHI <kikuchan98@gmail.com>");
 MODULE_DESCRIPTION("Allwinner sun20i PWM driver");
 MODULE_LICENSE("GPL");
-- 
2.45.1

Re: [PATCH 2/5] pwm: sun20i: Add support for Allwinner H616 PWM

[Uwe Kleine-König]

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

Hello,

On Fri, May 31, 2024 at 11:11:34PM +0900, Hironori KIKUCHI wrote:
> @@ -20,8 +20,17 @@
>  #include <linux/pwm.h>
>  #include <linux/reset.h>
>  
> +#define SUN20I_PWM_REG_OFFSET_PER_D1		(0x0080)
> +#define SUN20I_PWM_REG_OFFSET_PCR_D1		(0x0100 + 0x0000)
> +#define SUN20I_PWM_REG_OFFSET_PPR_D1		(0x0100 + 0x0004)
> +#define SUN20I_PWM_REG_OFFSET_PER_H616		(0x0040)
> +#define SUN20I_PWM_REG_OFFSET_PCR_H616		(0x0060 + 0x0000)
> +#define SUN20I_PWM_REG_OFFSET_PPR_H616		(0x0060 + 0x0004)

Instead of having a conditional for each register, it would be easier
to do:

	#define SUN20I_PWM_CHANBASE_D1		0x80
	#define SUN20I_PWM_CHANBASE_H616	0x40

(maybe with a more suitable name) and then do:

	#define SUN20I_PWM_PER(sun20i_chip)		((sun20i_chip)->chanbase + 0)
	#define SUN20I_PWM_PCR(sun20i_chip)		((sun20i_chip)->chanbase + 0x20)
	#define SUN20I_PWM_PPR(sun20i_chip)		((sun20i_chip)->chanbase + 0x24)

I would expect these definitions to appear in the order of register
addresses, that is below SUN20I_PWM_CLK_CFG. This reduces the size of
the private data struct, is easier to understnad to a human (I think)
and I claim this results in more compact code (without having it
verified).

Best regards
Uwe

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

[PATCH 3/5] dt-bindings: pwm: sun20i: Add compatible string for Allwinner H616 PWM

[Hironori KIKUCHI]

Allwinner H616 SoC has a PWM controller similar to the one
in the D1, which is supported by the pwm-sun20i driver.

The main difference is in the register layout. Specifically, the
GATING flag is placed in the PCCR register instead of the
individual PCGR register.

Add a compatible string to distinguish them.

Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
---
 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
index 89cebf7841a..b9b6d7e7c87 100644
--- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
@@ -17,6 +17,7 @@ properties:
       - items:
           - const: allwinner,sun50i-r329-pwm
           - const: allwinner,sun20i-d1-pwm
+      - const: allwinner,sun50i-h616-pwm
 
   reg:
     maxItems: 1
-- 
2.45.1

Re: [PATCH 3/5] dt-bindings: pwm: sun20i: Add compatible string for Allwinner H616 PWM

[Krzysztof Kozlowski]

On 31/05/2024 16:11, Hironori KIKUCHI wrote:
> Allwinner H616 SoC has a PWM controller similar to the one
> in the D1, which is supported by the pwm-sun20i driver.
> 
> The main difference is in the register layout. Specifically, the
> GATING flag is placed in the PCCR register instead of the
> individual PCGR register.
> 
> Add a compatible string to distinguish them.
> 
> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> ---
>  Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml | 1 +

There is no such file and there is no dependency (lore link) neither
here nor in cover letter.

Best regards,
Krzysztof

Re: [PATCH 3/5] dt-bindings: pwm: sun20i: Add compatible string for Allwinner H616 PWM

[Hironori KIKUCHI]

Hello Krzysztof,
Thank you for your reply.

> There is no such file and there is no dependency (lore link) neither
> here nor in cover letter.

I'm so sorry, it's based on the patch series:
https://lore.kernel.org/linux-sunxi/20240520184227.120956-1-privatesub2@gmail.com/T/#u

Best regards,
kikuchan.

[PATCH 4/5] pwm: sun20i: Delegating the clock source and DIV_M to the Device Tree

[Hironori KIKUCHI]

This patch removes the SUN20I_PWM_MAGIC macro by delegating the clock
source and DIV_M selection to the Device Tree.
This change addresses the issue of resolution discrepancies that arise
from the enabling order of PWM channels which are coupled.

Additionally, this patch clarifies and corrects the calculations for
the period and duty cycle. By using DIV_ROUND_CLOSEST(), it minimizes
the errors between the configured and actual values.

Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
---
 drivers/pwm/pwm-sun20i.c | 190 ++++++++++++++++-----------------------
 1 file changed, 79 insertions(+), 111 deletions(-)

diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
index d07ce0ebd2a..4bf8a67df38 100644
--- a/drivers/pwm/pwm-sun20i.c
+++ b/drivers/pwm/pwm-sun20i.c
@@ -52,53 +52,13 @@
 
 #define SUN20I_PWM_PCNTR_SIZE			BIT(16)
 
-/*
- * SUN20I_PWM_MAGIC is used to quickly compute the values of the clock dividers
- * div_m (SUN20I_PWM_CLK_CFG_DIV_M) & prescale_k (SUN20I_PWM_CTL_PRESCAL_K)
- * without using a loop. These dividers limit the # of cycles in a period
- * to SUN20I_PWM_PCNTR_SIZE by applying a scaling factor of
- * 1/(div_m * (prescale_k + 1)) to the clock source.
- *
- * SUN20I_PWM_MAGIC is derived by solving for div_m and prescale_k
- * such that for a given requested period,
- *
- * i) div_m is minimized for any prescale_k ≤ SUN20I_PWM_CTL_PRESCAL_K_MAX,
- * ii) prescale_k is minimized.
- *
- * The derivation proceeds as follows, with val = # of cycles for requested
- * period:
- *
- * for a given value of div_m we want the smallest prescale_k such that
- *
- * (val >> div_m) // (prescale_k + 1) ≤ 65536 (SUN20I_PWM_PCNTR_SIZE)
- *
- * This is equivalent to:
- *
- * (val >> div_m) ≤ 65536 * (prescale_k + 1) + prescale_k
- * ⟺ (val >> div_m) ≤ 65537 * prescale_k + 65536
- * ⟺ (val >> div_m) - 65536 ≤ 65537 * prescale_k
- * ⟺ ((val >> div_m) - 65536) / 65537 ≤ prescale_k
- *
- * As prescale_k is integer, this becomes
- *
- * ((val >> div_m) - 65536) // 65537 ≤ prescale_k
- *
- * And is minimized at
- *
- * ((val >> div_m) - 65536) // 65537
- *
- * Now we pick the smallest div_m that satifies prescale_k ≤ 255
- * (i.e SUN20I_PWM_CTL_PRESCAL_K_MAX),
- *
- * ((val >> div_m) - 65536) // 65537 ≤ 255
- * ⟺ (val >> div_m) - 65536 ≤ 255 * 65537 + 65536
- * ⟺ val >> div_m ≤ 255 * 65537 + 2 * 65536
- * ⟺ val >> div_m < (255 * 65537 + 2 * 65536 + 1)
- * ⟺ div_m = fls((val) / (255 * 65537 + 2 * 65536 + 1))
- *
- * Suggested by Uwe Kleine-König
- */
-#define SUN20I_PWM_MAGIC			(255 * 65537 + 2 * 65536 + 1)
+#define SUN20I_PWM_CLOCK_SRC_HOSC		(0)
+#define SUN20I_PWM_CLOCK_SRC_APB		(1)
+#define SUN20I_PWM_CLOCK_SRC_DEFAULT		SUN20I_PWM_CLOCK_SRC_HOSC
+#define SUN20I_PWM_DIV_M_SHIFT_DEFAULT		(0)
+
+#define SUN20I_PWM_CHANNELS_MAX			(16)
+#define SUN20I_PWM_ENTIRE_CYCLE_MAX		(0xffff)
 
 struct sun20i_pwm_data {
 	unsigned long reg_per;
@@ -115,6 +75,9 @@ struct sun20i_pwm_chip {
 	/* Mutex to protect pwm apply state */
 	struct mutex mutex;
 	const struct sun20i_pwm_data *data;
+
+	u32 clk_src_reg[(SUN20I_PWM_CHANNELS_MAX + 1) / 2];
+	u32 div_m_shift_reg[(SUN20I_PWM_CHANNELS_MAX + 1) / 2];
 };
 
 static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
@@ -139,7 +102,8 @@ static int sun20i_pwm_get_state(struct pwm_chip *chip,
 				struct pwm_state *state)
 {
 	struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
-	u16 ent_cycle, act_cycle, prescale_k;
+	u32 ent_cycle, act_cycle;
+	u16 prescale_k;
 	u64 clk_rate, tmp;
 	u8 div_m;
 	u32 val;
@@ -170,7 +134,7 @@ static int sun20i_pwm_get_state(struct pwm_chip *chip,
 	mutex_unlock(&sun20i_chip->mutex);
 
 	act_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ACT_CYCLE, val);
-	ent_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, val);
+	ent_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, val) + 1;
 
 	/*
 	 * The duration of the active phase should not be longer
@@ -196,9 +160,9 @@ static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
 	struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
-	u64 bus_rate, hosc_rate, val, ent_cycle, act_cycle;
-	u32 clk_gate, clk_cfg, pwm_en, ctl, reg_period;
-	u32 prescale_k, div_m;
+	u64 bus_rate, hosc_rate, ent_cycle, act_cycle;
+	u32 clk_gate, clk_cfg, pwm_en, ctl, reg_period, clk_rate;
+	u32 prescale_k, div_m, div_m_shift;
 	bool use_bus_clk;
 	int ret = 0;
 
@@ -229,76 +193,49 @@ static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (state->polarity != pwm->state.polarity ||
 	    state->duty_cycle != pwm->state.duty_cycle ||
 	    state->period != pwm->state.period) {
-		ctl = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(sun20i_chip, pwm->hwpwm));
-		clk_cfg = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
+		int idx = pwm->hwpwm / 2;
+
 		hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
 		bus_rate = clk_get_rate(sun20i_chip->clk_apb);
-		if (pwm_en & SUN20I_PWM_ENABLE_EN(pwm->hwpwm ^ 1)) {
-			/* if the neighbor channel is enable, check period only */
-			use_bus_clk = FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, clk_cfg) != 0;
-			val = mul_u64_u64_div_u64(state->period,
-						  (use_bus_clk ? bus_rate : hosc_rate),
-						  NSEC_PER_SEC);
 
-			div_m = FIELD_GET(SUN20I_PWM_CLK_CFG_DIV_M, clk_cfg);
-		} else {
-			/* check period and select clock source */
-			use_bus_clk = false;
-			val = mul_u64_u64_div_u64(state->period, hosc_rate, NSEC_PER_SEC);
-			if (val <= 1) {
-				use_bus_clk = true;
-				val = mul_u64_u64_div_u64(state->period, bus_rate, NSEC_PER_SEC);
-				if (val <= 1) {
-					ret = -EINVAL;
-					goto unlock_mutex;
-				}
-			}
-			div_m = fls(DIV_ROUND_DOWN_ULL(val, SUN20I_PWM_MAGIC));
-			if (div_m > SUN20I_PWM_CLK_DIV_M_MAX) {
-				ret = -EINVAL;
-				goto unlock_mutex;
-			}
+		use_bus_clk = sun20i_chip->clk_src_reg[idx] == SUN20I_PWM_CLOCK_SRC_APB;
+		clk_rate = use_bus_clk ? bus_rate : hosc_rate;
+		div_m_shift = sun20i_chip->div_m_shift_reg[idx];
+		div_m = 1 << div_m_shift;
 
-			/* set up the CLK_DIV_M and clock CLK_SRC */
-			clk_cfg &= ~(SUN20I_PWM_CLK_CFG_DIV_M | SUN20I_PWM_CLK_CFG_SRC);
-			clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_DIV_M, div_m);
-			clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_SRC, use_bus_clk);
-
-			sun20i_pwm_writel(sun20i_chip, clk_cfg, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
+		if (state->period > U64_MAX / clk_rate || state->duty_cycle > state->period) {
+			ret = -EINVAL;
+			goto unlock_mutex;
 		}
+		ent_cycle = DIV_ROUND_CLOSEST(state->period * clk_rate, NSEC_PER_SEC * div_m);
+		act_cycle =
+			min(DIV_ROUND_CLOSEST(state->duty_cycle * clk_rate, NSEC_PER_SEC * div_m),
+			    ent_cycle);
+		if (ent_cycle == 0 ||
+		    ent_cycle > SUN20I_PWM_ENTIRE_CYCLE_MAX * SUN20I_PWM_CTL_PRESCAL_K_MAX) {
+			ret = -EINVAL;
+			goto unlock_mutex;
+		}
+		prescale_k = clamp(DIV_ROUND_UP_ULL(ent_cycle, SUN20I_PWM_ENTIRE_CYCLE_MAX), 1,
+				   SUN20I_PWM_CTL_PRESCAL_K_MAX);
+		ent_cycle = clamp(DIV_ROUND_CLOSEST_ULL(ent_cycle, prescale_k), 1,
+				  SUN20I_PWM_ENTIRE_CYCLE_MAX);
+		act_cycle = clamp(DIV_ROUND_CLOSEST_ULL(act_cycle, prescale_k), 0, ent_cycle);
 
-		/* calculate prescale_k, PWM entire cycle */
-		ent_cycle = val >> div_m;
-		prescale_k = DIV_ROUND_DOWN_ULL(ent_cycle, 65537);
-		if (prescale_k > SUN20I_PWM_CTL_PRESCAL_K_MAX)
-			prescale_k = SUN20I_PWM_CTL_PRESCAL_K_MAX;
+		clk_cfg = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
+		clk_cfg &= ~(SUN20I_PWM_CLK_CFG_DIV_M | SUN20I_PWM_CLK_CFG_SRC);
+		clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_DIV_M, div_m_shift);
+		clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_SRC, use_bus_clk);
+		sun20i_pwm_writel(sun20i_chip, clk_cfg, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
 
-		do_div(ent_cycle, prescale_k + 1);
-
-		/* for N cycles, PPRx.PWM_ENTIRE_CYCLE = (N-1) */
 		reg_period = FIELD_PREP(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, ent_cycle - 1);
-
-		/* set duty cycle */
-		val = mul_u64_u64_div_u64(state->duty_cycle,
-					  (use_bus_clk ? bus_rate : hosc_rate),
-					  NSEC_PER_SEC);
-		act_cycle = val >> div_m;
-		do_div(act_cycle, prescale_k + 1);
-
-		/*
-		 * The formula of the output period and the duty-cycle for PWM are as follows.
-		 * T period = (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
-		 */
 		reg_period |= FIELD_PREP(SUN20I_PWM_PERIOD_ACT_CYCLE, act_cycle);
 		sun20i_pwm_writel(sun20i_chip, reg_period,
 			SUN20I_PWM_PERIOD(sun20i_chip, pwm->hwpwm));
 
-		ctl = FIELD_PREP(SUN20I_PWM_CTL_PRESCAL_K, prescale_k);
+		ctl = FIELD_PREP(SUN20I_PWM_CTL_PRESCAL_K, prescale_k - 1);
 		if (state->polarity == PWM_POLARITY_NORMAL)
 			ctl |= SUN20I_PWM_CTL_ACT_STA;
-
 		sun20i_pwm_writel(sun20i_chip, ctl, SUN20I_PWM_CTL(sun20i_chip, pwm->hwpwm));
 	}
 
@@ -382,9 +319,10 @@ static int sun20i_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		npwm = 8;
 
-	if (npwm > 16) {
-		dev_info(&pdev->dev, "Limiting number of PWM lines from %u to 16", npwm);
-		npwm = 16;
+	if (npwm > SUN20I_PWM_CHANNELS_MAX) {
+		dev_info(&pdev->dev, "Limiting number of PWM lines from %u to %u", npwm,
+			 SUN20I_PWM_CHANNELS_MAX);
+		npwm = SUN20I_PWM_CHANNELS_MAX;
 	}
 
 	chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*sun20i_chip));
@@ -420,6 +358,36 @@ static int sun20i_pwm_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->rst),
 				     "failed to get bus reset\n");
 
+	for (int i = 0; i < (npwm + 1) / 2; i++) {
+		const char *source;
+		u32 div_m;
+
+		sun20i_chip->clk_src_reg[i] = SUN20I_PWM_CLOCK_SRC_DEFAULT;
+		sun20i_chip->div_m_shift_reg[i] = SUN20I_PWM_DIV_M_SHIFT_DEFAULT;
+
+		ret = of_property_read_string_index(pdev->dev.of_node,
+						    "allwinner,pwm-pair-clock-sources", i, &source);
+		if (!ret) {
+			if (!strcasecmp(source, "hosc"))
+				sun20i_chip->clk_src_reg[i] = SUN20I_PWM_CLOCK_SRC_HOSC;
+			else if (!strcasecmp(source, "apb"))
+				sun20i_chip->clk_src_reg[i] = SUN20I_PWM_CLOCK_SRC_APB;
+			else
+				return dev_err_probe(&pdev->dev, -EINVAL,
+						     "Unknown clock source: %s\n", source);
+		}
+
+		ret = of_property_read_u32_index(pdev->dev.of_node,
+						 "allwinner,pwm-pair-clock-prescales", i, &div_m);
+		if (!ret) {
+			if (div_m <= SUN20I_PWM_CLK_DIV_M_MAX)
+				sun20i_chip->div_m_shift_reg[i] = div_m;
+			else
+				return dev_err_probe(&pdev->dev, -EINVAL,
+						     "Invalid prescale value: %u\n", div_m);
+		}
+	}
+
 	/* Deassert reset */
 	ret = reset_control_deassert(sun20i_chip->rst);
 	if (ret)
-- 
2.45.1

[PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV_M

[Hironori KIKUCHI]

This patch adds new options to select a clock source and DIV_M register
value for each coupled PWM channels.

Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
---
 .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
index b9b6d7e7c87..436a1d344ab 100644
--- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
@@ -45,6 +45,25 @@ properties:
     description: The number of PWM channels configured for this instance
     enum: [6, 9]
 
+  allwinner,pwm-pair-clock-sources:
+    description: The clock source names for each PWM pair
+    items:
+      enum: [hosc, apb]
+    minItems: 1
+    maxItems: 8
+
+  allwinner,pwm-pair-clock-prescales:
+    description: The prescale (DIV_M register) values for each PWM pair
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    items:
+      items:
+        minimum: 0
+        maximum: 8
+      minItems: 1
+      maxItems: 1
+    minItems: 1
+    maxItems: 8
+
 allOf:
   - $ref: pwm.yaml#
 
-- 
2.45.1

Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV_M

[Krzysztof Kozlowski]

On 31/05/2024 16:11, Hironori KIKUCHI wrote:
> This patch adds new options to select a clock source and DIV_M register
> value for each coupled PWM channels.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

Bindings are before their users. This should not be last patch, because
this implies there is no user.

This applies to all variants? Or the one you add? Confused...

> 
> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> ---
>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> index b9b6d7e7c87..436a1d344ab 100644
> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> @@ -45,6 +45,25 @@ properties:
>      description: The number of PWM channels configured for this instance
>      enum: [6, 9]
>  
> +  allwinner,pwm-pair-clock-sources:
> +    description: The clock source names for each PWM pair
> +    items:
> +      enum: [hosc, apb]
> +    minItems: 1
> +    maxItems: 8

Missing type... and add 8 of such items to your example to make it complete.

> +
> +  allwinner,pwm-pair-clock-prescales:
> +    description: The prescale (DIV_M register) values for each PWM pair
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    items:
> +      items:
> +        minimum: 0
> +        maximum: 8
> +      minItems: 1
> +      maxItems: 1
> +    minItems: 1
> +    maxItems: 8

This does not look like matrix but array.

Why clock DIV cannot be deduced from typical PWM attributes + clock
frequency?

Best regards,
Krzysztof

Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV_M

[Hironori KIKUCHI]

Hello,

> > This patch adds new options to select a clock source and DIV_M register
> > value for each coupled PWM channels.
>
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> Bindings are before their users. This should not be last patch, because
> this implies there is no user.

I'm sorry, I'll fix them.

> This applies to all variants? Or the one you add? Confused...

Apologies for confusing you. This applies to all variants.

>
> >
> > Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> > ---
> >  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > index b9b6d7e7c87..436a1d344ab 100644
> > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > @@ -45,6 +45,25 @@ properties:
> >      description: The number of PWM channels configured for this instance
> >      enum: [6, 9]
> >
> > +  allwinner,pwm-pair-clock-sources:
> > +    description: The clock source names for each PWM pair
> > +    items:
> > +      enum: [hosc, apb]
> > +    minItems: 1
> > +    maxItems: 8
>
> Missing type... and add 8 of such items to your example to make it complete.

Thank you. I'll fix it.

>
> > +
> > +  allwinner,pwm-pair-clock-prescales:
> > +    description: The prescale (DIV_M register) values for each PWM pair
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    items:
> > +      items:
> > +        minimum: 0
> > +        maximum: 8
> > +      minItems: 1
> > +      maxItems: 1
> > +    minItems: 1
> > +    maxItems: 8
>
> This does not look like matrix but array.

I wanted to specify values like this:

    allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
    allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":

These should correspond to each PWM pair.
This way, I thought we might be able to visually understand the relationship
between prescalers and sources, like clock-names and clocks.

Is this notation uncommon, perhaps?

>
> Why clock DIV cannot be deduced from typical PWM attributes + clock
> frequency?

This SoC's PWM system has one shared prescaler and clock source for each pair
of PWM channels. I should have noted this earlier, sorry.

Actually, the original v9 patch automatically deduced the DIV value
from the frequency.
However, because the two channels share a single prescaler, once one channel is
enabled, it affects and restricts the DIV value for the other channel
in the pair.
This introduces a problem of determining which channel should set the shared DIV
value. The original behavior was that the first channel enabled would win.

Instead, this patch try to resolve the issue by specifying these
values for each PWM
pairs deterministically.
That's why it requires the new options.

>
> Best regards,
> Krzysztof

Best regards,
kikuchan.

Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV_M

[Krzysztof Kozlowski]

On 31/05/2024 19:57, Hironori KIKUCHI wrote:
> Hello,
> 
>>> This patch adds new options to select a clock source and DIV_M register
>>> value for each coupled PWM channels.
>>
>> Please do not use "This commit/patch/change", but imperative mood. See
>> longer explanation here:
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>> Bindings are before their users. This should not be last patch, because
>> this implies there is no user.
> 
> I'm sorry, I'll fix them.
> 
>> This applies to all variants? Or the one you add? Confused...
> 
> Apologies for confusing you. This applies to all variants.
> 
>>
>>>
>>> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
>>> ---
>>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
>>> index b9b6d7e7c87..436a1d344ab 100644
>>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
>>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
>>> @@ -45,6 +45,25 @@ properties:
>>>      description: The number of PWM channels configured for this instance
>>>      enum: [6, 9]
>>>
>>> +  allwinner,pwm-pair-clock-sources:
>>> +    description: The clock source names for each PWM pair
>>> +    items:
>>> +      enum: [hosc, apb]
>>> +    minItems: 1
>>> +    maxItems: 8
>>
>> Missing type... and add 8 of such items to your example to make it complete.
> 
> Thank you. I'll fix it.
> 
>>
>>> +
>>> +  allwinner,pwm-pair-clock-prescales:
>>> +    description: The prescale (DIV_M register) values for each PWM pair
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>> +    items:
>>> +      items:
>>> +        minimum: 0
>>> +        maximum: 8
>>> +      minItems: 1
>>> +      maxItems: 1
>>> +    minItems: 1
>>> +    maxItems: 8
>>
>> This does not look like matrix but array.
> 
> I wanted to specify values like this:
> 
>     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
>     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> 
> These should correspond to each PWM pair.
> This way, I thought we might be able to visually understand the relationship
> between prescalers and sources, like clock-names and clocks.
> 
> Is this notation uncommon, perhaps?

It's still an array.

> 
>>
>> Why clock DIV cannot be deduced from typical PWM attributes + clock
>> frequency?
> 
> This SoC's PWM system has one shared prescaler and clock source for each pair
> of PWM channels. I should have noted this earlier, sorry.
> 
> Actually, the original v9 patch automatically deduced the DIV value
> from the frequency.
> However, because the two channels share a single prescaler, once one channel is
> enabled, it affects and restricts the DIV value for the other channel
> in the pair.
> This introduces a problem of determining which channel should set the shared DIV
> value. The original behavior was that the first channel enabled would win.

There's nothing bad in this.

> 
> Instead, this patch try to resolve the issue by specifying these
> values for each PWM
> pairs deterministically.
> That's why it requires the new options.

This does not solve that wrong divider can be programmed for second
channel in each pair.

Best regards,
Krzysztof

Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV_M

[Hironori KIKUCHI]

Hi Krzysztof,

> On 31/05/2024 19:57, Hironori KIKUCHI wrote:
> > Hello,
> >
> >>> This patch adds new options to select a clock source and DIV_M register
> >>> value for each coupled PWM channels.
> >>
> >> Please do not use "This commit/patch/change", but imperative mood. See
> >> longer explanation here:
> >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> >>
> >> Bindings are before their users. This should not be last patch, because
> >> this implies there is no user.
> >
> > I'm sorry, I'll fix them.
> >
> >> This applies to all variants? Or the one you add? Confused...
> >
> > Apologies for confusing you. This applies to all variants.
> >
> >>
> >>>
> >>> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> >>> ---
> >>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> >>>  1 file changed, 19 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> >>> index b9b6d7e7c87..436a1d344ab 100644
> >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> >>> @@ -45,6 +45,25 @@ properties:
> >>>      description: The number of PWM channels configured for this instance
> >>>      enum: [6, 9]
> >>>
> >>> +  allwinner,pwm-pair-clock-sources:
> >>> +    description: The clock source names for each PWM pair
> >>> +    items:
> >>> +      enum: [hosc, apb]
> >>> +    minItems: 1
> >>> +    maxItems: 8
> >>
> >> Missing type... and add 8 of such items to your example to make it complete.
> >
> > Thank you. I'll fix it.
> >
> >>
> >>> +
> >>> +  allwinner,pwm-pair-clock-prescales:
> >>> +    description: The prescale (DIV_M register) values for each PWM pair
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> >>> +    items:
> >>> +      items:
> >>> +        minimum: 0
> >>> +        maximum: 8
> >>> +      minItems: 1
> >>> +      maxItems: 1
> >>> +    minItems: 1
> >>> +    maxItems: 8
> >>
> >> This does not look like matrix but array.
> >
> > I wanted to specify values like this:
> >
> >     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
> >     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> >
> > These should correspond to each PWM pair.
> > This way, I thought we might be able to visually understand the relationship
> > between prescalers and sources, like clock-names and clocks.
> >
> > Is this notation uncommon, perhaps?
>
> It's still an array.

Oh I understood and clear. Thank you.

> >> Why clock DIV cannot be deduced from typical PWM attributes + clock
> >> frequency?
> >
> > This SoC's PWM system has one shared prescaler and clock source for each pair
> > of PWM channels. I should have noted this earlier, sorry.
> >
> > Actually, the original v9 patch automatically deduced the DIV value
> > from the frequency.
> > However, because the two channels share a single prescaler, once one channel is
> > enabled, it affects and restricts the DIV value for the other channel
> > in the pair.
> > This introduces a problem of determining which channel should set the shared DIV
> > value. The original behavior was that the first channel enabled would win.
>
> There's nothing bad in this.
>
> >
> > Instead, this patch try to resolve the issue by specifying these
> > values for each PWM
> > pairs deterministically.
> > That's why it requires the new options.
>
> This does not solve that wrong divider can be programmed for second
> channel in each pair.
>

Let me illustrate the connection of a paired PWM channels to be sure.

.    +------+                   +--------------+  +------+
.    + HOSC +--+             +--+ prescale_k 0 +--+ PWM0 |
.    +------+  |  +-------+  |  +--------------+  +------+
.              +--+ DIV_M +--+
.    +------+  |  +-------+  |  +--------------+  +------+
.    + APBx +--+             +--+ prescale_k 1 +--+ PWM1 |
.    +------+                   +--------------+  +------+
.          CLK_SRC

The PWM0 and PWM1 share DIV_M and CLK_SRC for them, and (not
illustrated) PWM2 and PWM3 share another DIV_M and another CLK_SRC for
them, and so on.
The DIV_M ranges from 0 to 8 and is used as a 1 / 2^DIV_M prescaler,
prescale_k ranges from 0 to 255 and is used as a 1 / (prescale_k + 1)
prescaler.

In the original v9 patch, enabling PWM0 determines CLK_SRC and
calculates DIV_M from the period that is going to be set.
Once the CLK_SRC and DIV_M are fixed, they cannot be changed until
both channels are disabled, unless PWM0 is the only enabled channel.

Looks good so far, but there is a pitfall.

Selecting CLK_SRC and DIV_M means it defines the PWM resolution of the
period and duty cycle for the pair of the PWM channels.
In other words, the resolution is determined by the (most likely the
very first) period, which can be arbitrary.

Consider an application that uses PWM channels to generate a square
wave in stereo.
The very first musical note played defines the entire resolution for
the subsequent notes.
The music quality depends on the first note.

The problem is, there is NO way to fixate the resolution to be used.

The proposed method provides a simple way to deterministically fixate
the resolution.
(ofcourse, prescale_k is still calculated by period to be set)

> Best regards,
> Krzysztof

Best regards,
kikuchan.

Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV_M

[Andre Przywara]

On Sun, 2 Jun 2024 15:15:13 +0900
Hironori KIKUCHI <kikuchan98@gmail.com> wrote:

Hi Kikuchan,

> Hi Krzysztof,
> 
> > On 31/05/2024 19:57, Hironori KIKUCHI wrote:  
> > > Hello,
> > >  
> > >>> This patch adds new options to select a clock source and DIV_M register
> > >>> value for each coupled PWM channels.  
> > >>
> > >> Please do not use "This commit/patch/change", but imperative mood. See
> > >> longer explanation here:
> > >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> > >>
> > >> Bindings are before their users. This should not be last patch, because
> > >> this implies there is no user.  
> > >
> > > I'm sorry, I'll fix them.
> > >  
> > >> This applies to all variants? Or the one you add? Confused...  
> > >
> > > Apologies for confusing you. This applies to all variants.
> > >  
> > >>  
> > >>>
> > >>> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> > >>> ---
> > >>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> > >>>  1 file changed, 19 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > >>> index b9b6d7e7c87..436a1d344ab 100644
> > >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > >>> @@ -45,6 +45,25 @@ properties:
> > >>>      description: The number of PWM channels configured for this instance
> > >>>      enum: [6, 9]
> > >>>
> > >>> +  allwinner,pwm-pair-clock-sources:
> > >>> +    description: The clock source names for each PWM pair
> > >>> +    items:
> > >>> +      enum: [hosc, apb]
> > >>> +    minItems: 1
> > >>> +    maxItems: 8  
> > >>
> > >> Missing type... and add 8 of such items to your example to make it complete.  
> > >
> > > Thank you. I'll fix it.
> > >  
> > >>  
> > >>> +
> > >>> +  allwinner,pwm-pair-clock-prescales:
> > >>> +    description: The prescale (DIV_M register) values for each PWM pair
> > >>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > >>> +    items:
> > >>> +      items:
> > >>> +        minimum: 0
> > >>> +        maximum: 8
> > >>> +      minItems: 1
> > >>> +      maxItems: 1
> > >>> +    minItems: 1
> > >>> +    maxItems: 8  
> > >>
> > >> This does not look like matrix but array.  
> > >
> > > I wanted to specify values like this:
> > >
> > >     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
> > >     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> > >
> > > These should correspond to each PWM pair.
> > > This way, I thought we might be able to visually understand the relationship
> > > between prescalers and sources, like clock-names and clocks.
> > >
> > > Is this notation uncommon, perhaps?  
> >
> > It's still an array.  
> 
> Oh I understood and clear. Thank you.
> 
> > >> Why clock DIV cannot be deduced from typical PWM attributes + clock
> > >> frequency?  
> > >
> > > This SoC's PWM system has one shared prescaler and clock source for each pair
> > > of PWM channels. I should have noted this earlier, sorry.
> > >
> > > Actually, the original v9 patch automatically deduced the DIV value
> > > from the frequency.
> > > However, because the two channels share a single prescaler, once one channel is
> > > enabled, it affects and restricts the DIV value for the other channel
> > > in the pair.
> > > This introduces a problem of determining which channel should set the shared DIV
> > > value. The original behavior was that the first channel enabled would win.  
> >
> > There's nothing bad in this.
> >  
> > >
> > > Instead, this patch try to resolve the issue by specifying these
> > > values for each PWM
> > > pairs deterministically.
> > > That's why it requires the new options.  
> >
> > This does not solve that wrong divider can be programmed for second
> > channel in each pair.
> >  
> 
> Let me illustrate the connection of a paired PWM channels to be sure.
> 
> .    +------+                   +--------------+  +------+
> .    + HOSC +--+             +--+ prescale_k 0 +--+ PWM0 |
> .    +------+  |  +-------+  |  +--------------+  +------+
> .              +--+ DIV_M +--+
> .    +------+  |  +-------+  |  +--------------+  +------+
> .    + APBx +--+             +--+ prescale_k 1 +--+ PWM1 |
> .    +------+                   +--------------+  +------+
> .          CLK_SRC
> 
> The PWM0 and PWM1 share DIV_M and CLK_SRC for them, and (not
> illustrated) PWM2 and PWM3 share another DIV_M and another CLK_SRC for
> them, and so on.
> The DIV_M ranges from 0 to 8 and is used as a 1 / 2^DIV_M prescaler,
> prescale_k ranges from 0 to 255 and is used as a 1 / (prescale_k + 1)
> prescaler.
> 
> In the original v9 patch, enabling PWM0 determines CLK_SRC and
> calculates DIV_M from the period that is going to be set.
> Once the CLK_SRC and DIV_M are fixed, they cannot be changed until
> both channels are disabled, unless PWM0 is the only enabled channel.
> 
> Looks good so far, but there is a pitfall.
> 
> Selecting CLK_SRC and DIV_M means it defines the PWM resolution of the
> period and duty cycle for the pair of the PWM channels.
> In other words, the resolution is determined by the (most likely the
> very first) period, which can be arbitrary.

So I understand the problem, but I don't think expressing this in the
devicetree is the right solution. It seems like a tempting pragmatical
approach, but it sounds like the wrong way: this is not a hardware
*description* of any kind, but rather a way to describe a certain user
intention or a configuration. So this looks like a rather embedded
approach to me, where you have a certain fixed register setup in mind,
and want to somehow force this to the hardware.
Another problem with this approach is that it doesn't really cover the
sysfs interface, which is very dynamic by nature.

I have some questions / ideas, and would love to hear some feedback on
them:
- If some PWM channels are "linked", I don't think there is much we can
  do about it: it's a hardware limitation. The details of that is
  already "encoded" in the compatible string, I'd say, so there is no
  need for further description in the devicetree. Any PWM user on those
  boards would probably need to know about the shortcomings, and either
  use different channels for wildly different PWM setups, or accept
  that there are actually only three freely programmable PWM channels.
- Does the PWM subsystem already have a way to model linked channels?
  Maybe that problem is solved already elsewhere?
- Previous Allwinner PWM IP was restricted to use the 24 MHz
  oscillator only, and people seem to have survived with that. Can we
  not just restrict ourselves to one clock source for those linked
  channels? I would assume that the PWM frequency is less important
  than the duty cycle? 
- Can't we just return an error if some conflicting setup requests are
  made? At the expense of this seeming somewhat random to the user,
  because it depends on the order of requests? But people could then
  react on the returned error value?

In general, I wonder what the real use cases are, maybe it's not a
problem in real life? Do you have a concrete issue at hand, or is this
just thinking about all potential use cases - which is honourable, but
maybe a bit over the top here?

Cheers,
Andre

> Consider an application that uses PWM channels to generate a square
> wave in stereo.
> The very first musical note played defines the entire resolution for
> the subsequent notes.
> The music quality depends on the first note.
> 
> The problem is, there is NO way to fixate the resolution to be used.
> 
> The proposed method provides a simple way to deterministically fixate
> the resolution.
> (ofcourse, prescale_k is still calculated by period to be set)
> 
> > Best regards,
> > Krzysztof  
> 
> Best regards,
> kikuchan.
> 

Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV_M

[Hironori KIKUCHI]

Hi Andre,

Thank you for your reply.

2024年6月3日(月) 9:10 Andre Przywara <andre.przywara@arm.com>:
>
> On Sun, 2 Jun 2024 15:15:13 +0900
> Hironori KIKUCHI <kikuchan98@gmail.com> wrote:
>
> Hi Kikuchan,
>
> > Hi Krzysztof,
> >
> > > On 31/05/2024 19:57, Hironori KIKUCHI wrote:
> > > > Hello,
> > > >
> > > >>> This patch adds new options to select a clock source and DIV_M register
> > > >>> value for each coupled PWM channels.
> > > >>
> > > >> Please do not use "This commit/patch/change", but imperative mood. See
> > > >> longer explanation here:
> > > >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> > > >>
> > > >> Bindings are before their users. This should not be last patch, because
> > > >> this implies there is no user.
> > > >
> > > > I'm sorry, I'll fix them.
> > > >
> > > >> This applies to all variants? Or the one you add? Confused...
> > > >
> > > > Apologies for confusing you. This applies to all variants.
> > > >
> > > >>
> > > >>>
> > > >>> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> > > >>> ---
> > > >>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> > > >>>  1 file changed, 19 insertions(+)
> > > >>>
> > > >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > >>> index b9b6d7e7c87..436a1d344ab 100644
> > > >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > >>> @@ -45,6 +45,25 @@ properties:
> > > >>>      description: The number of PWM channels configured for this instance
> > > >>>      enum: [6, 9]
> > > >>>
> > > >>> +  allwinner,pwm-pair-clock-sources:
> > > >>> +    description: The clock source names for each PWM pair
> > > >>> +    items:
> > > >>> +      enum: [hosc, apb]
> > > >>> +    minItems: 1
> > > >>> +    maxItems: 8
> > > >>
> > > >> Missing type... and add 8 of such items to your example to make it complete.
> > > >
> > > > Thank you. I'll fix it.
> > > >
> > > >>
> > > >>> +
> > > >>> +  allwinner,pwm-pair-clock-prescales:
> > > >>> +    description: The prescale (DIV_M register) values for each PWM pair
> > > >>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > >>> +    items:
> > > >>> +      items:
> > > >>> +        minimum: 0
> > > >>> +        maximum: 8
> > > >>> +      minItems: 1
> > > >>> +      maxItems: 1
> > > >>> +    minItems: 1
> > > >>> +    maxItems: 8
> > > >>
> > > >> This does not look like matrix but array.
> > > >
> > > > I wanted to specify values like this:
> > > >
> > > >     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
> > > >     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> > > >
> > > > These should correspond to each PWM pair.
> > > > This way, I thought we might be able to visually understand the relationship
> > > > between prescalers and sources, like clock-names and clocks.
> > > >
> > > > Is this notation uncommon, perhaps?
> > >
> > > It's still an array.
> >
> > Oh I understood and clear. Thank you.
> >
> > > >> Why clock DIV cannot be deduced from typical PWM attributes + clock
> > > >> frequency?
> > > >
> > > > This SoC's PWM system has one shared prescaler and clock source for each pair
> > > > of PWM channels. I should have noted this earlier, sorry.
> > > >
> > > > Actually, the original v9 patch automatically deduced the DIV value
> > > > from the frequency.
> > > > However, because the two channels share a single prescaler, once one channel is
> > > > enabled, it affects and restricts the DIV value for the other channel
> > > > in the pair.
> > > > This introduces a problem of determining which channel should set the shared DIV
> > > > value. The original behavior was that the first channel enabled would win.
> > >
> > > There's nothing bad in this.
> > >
> > > >
> > > > Instead, this patch try to resolve the issue by specifying these
> > > > values for each PWM
> > > > pairs deterministically.
> > > > That's why it requires the new options.
> > >
> > > This does not solve that wrong divider can be programmed for second
> > > channel in each pair.
> > >
> >
> > Let me illustrate the connection of a paired PWM channels to be sure.
> >
> > .    +------+                   +--------------+  +------+
> > .    + HOSC +--+             +--+ prescale_k 0 +--+ PWM0 |
> > .    +------+  |  +-------+  |  +--------------+  +------+
> > .              +--+ DIV_M +--+
> > .    +------+  |  +-------+  |  +--------------+  +------+
> > .    + APBx +--+             +--+ prescale_k 1 +--+ PWM1 |
> > .    +------+                   +--------------+  +------+
> > .          CLK_SRC
> >
> > The PWM0 and PWM1 share DIV_M and CLK_SRC for them, and (not
> > illustrated) PWM2 and PWM3 share another DIV_M and another CLK_SRC for
> > them, and so on.
> > The DIV_M ranges from 0 to 8 and is used as a 1 / 2^DIV_M prescaler,
> > prescale_k ranges from 0 to 255 and is used as a 1 / (prescale_k + 1)
> > prescaler.
> >
> > In the original v9 patch, enabling PWM0 determines CLK_SRC and
> > calculates DIV_M from the period that is going to be set.
> > Once the CLK_SRC and DIV_M are fixed, they cannot be changed until
> > both channels are disabled, unless PWM0 is the only enabled channel.
> >
> > Looks good so far, but there is a pitfall.
> >
> > Selecting CLK_SRC and DIV_M means it defines the PWM resolution of the
> > period and duty cycle for the pair of the PWM channels.
> > In other words, the resolution is determined by the (most likely the
> > very first) period, which can be arbitrary.
>
> So I understand the problem, but I don't think expressing this in the
> devicetree is the right solution. It seems like a tempting pragmatical
> approach, but it sounds like the wrong way: this is not a hardware
> *description* of any kind, but rather a way to describe a certain user
> intention or a configuration. So this looks like a rather embedded
> approach to me, where you have a certain fixed register setup in mind,
> and want to somehow force this to the hardware.
> Another problem with this approach is that it doesn't really cover the
> sysfs interface, which is very dynamic by nature.

... Indeed. You're right.
Now I've realized it was a bad idea.
It should be done in sysfs or sysctl perhaps.

>
> I have some questions / ideas, and would love to hear some feedback on
> them:
> - If some PWM channels are "linked", I don't think there is much we can
>   do about it: it's a hardware limitation. The details of that is
>   already "encoded" in the compatible string, I'd say, so there is no
>   need for further description in the devicetree. Any PWM user on those
>   boards would probably need to know about the shortcomings, and either
>   use different channels for wildly different PWM setups, or accept
>   that there are actually only three freely programmable PWM channels.
> - Does the PWM subsystem already have a way to model linked channels?
>   Maybe that problem is solved already elsewhere?
> - Previous Allwinner PWM IP was restricted to use the 24 MHz
>   oscillator only, and people seem to have survived with that. Can we
>   not just restrict ourselves to one clock source for those linked
>   channels? I would assume that the PWM frequency is less important
>   than the duty cycle?
> - Can't we just return an error if some conflicting setup requests are
>   made? At the expense of this seeming somewhat random to the user,
>   because it depends on the order of requests? But people could then
>   react on the returned error value?
>
> In general, I wonder what the real use cases are, maybe it's not a
> problem in real life? Do you have a concrete issue at hand, or is this
> just thinking about all potential use cases - which is honourable, but
> maybe a bit over the top here?

IMHO, it is sufficient to use fixed CLK_SRC and DIV_M values for this
driver, since the default values (CLK_SRC == hosc and DIV_M == 0)
already provide enough range in real life.

What I really care about is minimizing complexity and avoiding surprises.
Although the original method enables an incredibly wide range of the
period, it introduces unpredictability in resolution and inequity in a
pair due to a race in the order of enabling, as a drawback.

If the primary concern is achieving such a wide range, then I think
the original method is the most suitable option.

>
> Cheers,
> Andre
>

Best regards,
kikuchan.

Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV_M

[Uwe Kleine-König]

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

Hello,

On Mon, Jun 03, 2024 at 05:42:08PM +0900, Hironori KIKUCHI wrote:
> 2024年6月3日(月) 9:10 Andre Przywara <andre.przywara@arm.com>:
> >
> > On Sun, 2 Jun 2024 15:15:13 +0900
> > Hironori KIKUCHI <kikuchan98@gmail.com> wrote:
> >
> > > > On 31/05/2024 19:57, Hironori KIKUCHI wrote:
> > > > >>> ---
> > > > >>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> > > > >>>  1 file changed, 19 insertions(+)
> > > > >>>
> > > > >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > > >>> index b9b6d7e7c87..436a1d344ab 100644
> > > > >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > > >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > > >>> @@ -45,6 +45,25 @@ properties:
> > > > >>>      description: The number of PWM channels configured for this instance
> > > > >>>      enum: [6, 9]
> > > > >>>
> > > > >>> +  allwinner,pwm-pair-clock-sources:
> > > > >>> +    description: The clock source names for each PWM pair
> > > > >>> +    items:
> > > > >>> +      enum: [hosc, apb]
> > > > >>> +    minItems: 1
> > > > >>> +    maxItems: 8
> > > > >>
> > > > >> Missing type... and add 8 of such items to your example to make it complete.
> > > > >
> > > > > Thank you. I'll fix it.
> > > > >
> > > > >>
> > > > >>> +
> > > > >>> +  allwinner,pwm-pair-clock-prescales:
> > > > >>> +    description: The prescale (DIV_M register) values for each PWM pair
> > > > >>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > > >>> +    items:
> > > > >>> +      items:
> > > > >>> +        minimum: 0
> > > > >>> +        maximum: 8
> > > > >>> +      minItems: 1
> > > > >>> +      maxItems: 1
> > > > >>> +    minItems: 1
> > > > >>> +    maxItems: 8
> > > > >>
> > > > >> This does not look like matrix but array.
> > > > >
> > > > > I wanted to specify values like this:
> > > > >
> > > > >     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
> > > > >     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> > > > >
> > > > > These should correspond to each PWM pair.
> > > > > This way, I thought we might be able to visually understand the relationship
> > > > > between prescalers and sources, like clock-names and clocks.
> > > > >
> > > > > Is this notation uncommon, perhaps?
> > > >
> > > > It's still an array.
> > >
> > > Oh I understood and clear. Thank you.

For clocks there is already a binding to assign a working configuration.
assigned-clocks, assigned-clock-rates and assigned-clock-parents are the
relevant properties. If you create a clk from the parent clock selector
and mdiv, you can stick to the existing bindings.

> > > [...]
> >
> > So I understand the problem, but I don't think expressing this in the
> > devicetree is the right solution. It seems like a tempting pragmatical
> > approach, but it sounds like the wrong way: this is not a hardware
> > *description* of any kind, but rather a way to describe a certain user
> > intention or a configuration. So this looks like a rather embedded
> > approach to me, where you have a certain fixed register setup in mind,
> > and want to somehow force this to the hardware.
> > Another problem with this approach is that it doesn't really cover the
> > sysfs interface, which is very dynamic by nature.
> 
> ... Indeed. You're right.
> Now I've realized it was a bad idea.
> It should be done in sysfs or sysctl perhaps.

I don't think a driver specific knob somewhere is a practical solution.

Best regards
Uwe

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

Re: [PATCH 0/5] Add support for Allwinner H616 PWM

[Uwe Kleine-König]

On Fri, May 31, 2024 at 11:11:32PM +0900, Hironori KIKUCHI wrote:
> Add support for the Allwinner H616 PWM, building on top of Aleksandr's
> Allwinner D1 PWM driver v9.

It would be great if you could arrange with Aleksandr to maybe put your
efforts into a single series. I think this would simplify reviewing and
overall handling of your series to me.

Your first patch should for sure be squashed into Aleksandr's patch #2.

Best regards
Uwe