[[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Ben Dooks]

Add documentation for the bindings for Synopsys' DesignWare PWM block
as we will be adding DT/platform support to the Linux driver soon.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
--
v2:
- fix #pwm-cells to be 3
- fix indentation and ordering issues
---
 .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
new file mode 100644
index 000000000000..594085e5e26f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 SiFive, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/snps,pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys PWM controller
+
+maintainers:
+  - Ben Dooks <ben.dooks@sifive.com>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: snps,pwm
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    items:
+      - description: Interface bus clock
+      - description: PWM reference clock
+
+  clock-names:
+    items:
+      - const: bus
+      - const: timer
+
+required:
+  - "#pwm-cells"
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
-- 
2.35.1

[[PATCH v2] 2/9] dt-bindings: pwm: snps,pwm add pwm number

[Ben Dooks]

Add snps,pwm-number property to indicate if the block does not have
all 8 of the PWM blocks.

Not sure if this should be a general PWM property consider optional
for all PWM types, so have added a specific one here (there is only
one other controller with a property for PWM count at the moment)

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 Documentation/devicetree/bindings/pwm/snps,pwm.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
index 594085e5e26f..e95f518b3974 100644
--- a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
@@ -30,11 +30,16 @@ properties:
       - const: bus
       - const: timer
 
+  snps,pwm-number:
+    $ref: '/schemas/types.yaml#/definitions/uint32'
+    description: u32 value representing the number of PWM devices
+
 required:
   - "#pwm-cells"
   - compatible
   - reg
   - clocks
   - clock-names
+  - snps,pwm-number
 
 additionalProperties: false
-- 
2.35.1

Re: [[PATCH v2] 2/9] dt-bindings: pwm: snps,pwm add pwm number

[Krzysztof Kozlowski]

On 25/07/2022 23:21, Ben Dooks wrote:
> Add snps,pwm-number property to indicate if the block does not have
> all 8 of the PWM blocks.
> 
> Not sure if this should be a general PWM property consider optional
> for all PWM types, so have added a specific one here (there is only
> one other controller with a property for PWM count at the moment)
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>  Documentation/devicetree/bindings/pwm/snps,pwm.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
> index 594085e5e26f..e95f518b3974 100644
> --- a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
> @@ -30,11 +30,16 @@ properties:
>        - const: bus
>        - const: timer
>  
> +  snps,pwm-number:
> +    $ref: '/schemas/types.yaml#/definitions/uint32'

1. No quotes
2. Add minimum and maximum (it looks like there are such).

> +    description: u32 value representing the number of PWM devices


Best regards,
Krzysztof

Re: [[PATCH v2] 2/9] dt-bindings: pwm: snps,pwm add pwm number

[Ben Dooks]

On 26/07/2022 11:08, Krzysztof Kozlowski wrote:
> On 25/07/2022 23:21, Ben Dooks wrote:
>> Add snps,pwm-number property to indicate if the block does not have
>> all 8 of the PWM blocks.
>>
>> Not sure if this should be a general PWM property consider optional
>> for all PWM types, so have added a specific one here (there is only
>> one other controller with a property for PWM count at the moment)
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>>   Documentation/devicetree/bindings/pwm/snps,pwm.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>> index 594085e5e26f..e95f518b3974 100644
>> --- a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>> @@ -30,11 +30,16 @@ properties:
>>         - const: bus
>>         - const: timer
>>   
>> +  snps,pwm-number:
>> +    $ref: '/schemas/types.yaml#/definitions/uint32'
> 
> 1. No quotes
> 2. Add minimum and maximum (it looks like there are such).
> 
>> +    description: u32 value representing the number of PWM devices
> 

ok, thanks.

[[PATCH v2] 3/9] pwm: change &pci->dev to dev in probe

[Ben Dooks]

The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
&pci->dev throughout the function. Change these all to the be
'dev' variable to make lines shorter.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-dwc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 7568300bb11e..c706ef9a7ba1 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -202,14 +202,13 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	struct dwc_pwm *dwc;
 	int ret;
 
-	dwc = devm_kzalloc(&pci->dev, sizeof(*dwc), GFP_KERNEL);
+	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
 	if (!dwc)
 		return -ENOMEM;
 
 	ret = pcim_enable_device(pci);
 	if (ret) {
-		dev_err(&pci->dev,
-			"Failed to enable device (%pe)\n", ERR_PTR(ret));
+		dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret));
 		return ret;
 	}
 
@@ -217,14 +216,13 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 
 	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
 	if (ret) {
-		dev_err(&pci->dev,
-			"Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
+		dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
 		return ret;
 	}
 
 	dwc->base = pcim_iomap_table(pci)[0];
 	if (!dwc->base) {
-		dev_err(&pci->dev, "Base address missing\n");
+		dev_err(dev, "Base address missing\n");
 		return -ENOMEM;
 	}
 
-- 
2.35.1

[[PATCH v2] 4/9] pwm: move dwc memory alloc to own function

[Ben Dooks]

In preparation for adding other bus support, move the allocation
of the pwm struct out of the main driver code.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/pwm-dwc.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index c706ef9a7ba1..61f11e0a9319 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -196,13 +196,29 @@ static const struct pwm_ops dwc_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
+{
+	struct dwc_pwm *dwc;
+
+	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
+	if (!dwc)
+		return NULL;
+
+	dwc->chip.dev = dev;
+	dwc->chip.ops = &dwc_pwm_ops;
+	dwc->chip.npwm = DWC_TIMERS_TOTAL;
+
+	dev_set_drvdata(dev, dwc);
+	return dwc;
+}
+
 static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 {
 	struct device *dev = &pci->dev;
 	struct dwc_pwm *dwc;
 	int ret;
 
-	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
+	dwc = dwc_pwm_alloc(dev);
 	if (!dwc)
 		return -ENOMEM;
 
@@ -226,12 +242,6 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return -ENOMEM;
 	}
 
-	pci_set_drvdata(pci, dwc);
-
-	dwc->chip.dev = dev;
-	dwc->chip.ops = &dwc_pwm_ops;
-	dwc->chip.npwm = DWC_TIMERS_TOTAL;
-
 	ret = pwmchip_add(&dwc->chip);
 	if (ret)
 		return ret;
-- 
2.35.1

[[PATCH v2] 5/9] pwm: dwc: add of/platform support

[Ben Dooks]

The dwc pwm controller can be used in non-PCI systems, so allow
either platform or OF based probing.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/Kconfig   |  5 ++--
 drivers/pwm/pwm-dwc.c | 53 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 904de8d61828..e1aa645c1084 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -166,9 +166,10 @@ config PWM_CROS_EC
 
 config PWM_DWC
 	tristate "DesignWare PWM Controller"
-	depends on PCI
+	depends on PCI || OF
 	help
-	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
+	  PWM driver for Synopsys DWC PWM Controller attached to either a
+	  PCI or platform bus.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-dwc.
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 61f11e0a9319..235cb730c888 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
 
@@ -319,6 +320,58 @@ static struct pci_driver dwc_pwm_driver = {
 
 module_pci_driver(dwc_pwm_driver);
 
+#ifdef CONFIG_OF
+static int dwc_pwm_plat_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dwc_pwm *dwc;
+	int ret;
+
+	dwc = dwc_pwm_alloc(dev);
+	if (!dwc)
+		return -ENOMEM;
+
+	dwc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(dwc->base))
+		return dev_err_probe(dev, PTR_ERR(dwc->base),
+				     "failed to map IO\n");
+
+	ret = pwmchip_add(&dwc->chip);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int dwc_pwm_plat_remove(struct platform_device *pdev)
+{
+	struct dwc_pwm *dwc = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&dwc->chip);
+	return 0;
+}
+
+static const struct of_device_id dwc_pwm_dt_ids[] = {
+	{ .compatible = "snps,pwm" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
+
+static struct platform_driver dwc_pwm_plat_driver = {
+	.driver = {
+		.name		= "dwc-pwm",
+		.of_match_table  = dwc_pwm_dt_ids,
+	},
+	.probe	= dwc_pwm_plat_probe,
+	.remove	= dwc_pwm_plat_remove,
+};
+
+module_platform_driver(dwc_pwm_plat_driver);
+
+MODULE_ALIAS("platform:dwc-pwm");
+#endif /* CONFIG_OF */
+
+
 MODULE_AUTHOR("Felipe Balbi (Intel)");
 MODULE_AUTHOR("Jarkko Nikula <jarkko.nikula@linux.intel.com>");
 MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
-- 
2.35.1

[[PATCH v2] 6/9] pwm: dwc: allow driver to be built with COMPILE_TEST

[Ben Dooks]

Allow dwc driver to be built with COMPILE_TEST should allow
better coverage when build testing.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e1aa645c1084..2ba248aa2ff9 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -166,7 +166,8 @@ config PWM_CROS_EC
 
 config PWM_DWC
 	tristate "DesignWare PWM Controller"
-	depends on PCI || OF
+	depends on PCI || OF || COMPILE_TEST
+	depends on HAS_IOMEM
 	help
 	  PWM driver for Synopsys DWC PWM Controller attached to either a
 	  PCI or platform bus.
-- 
2.35.1

[[PATCH v2] 7/9] pwm: dwc: add timer clock

[Ben Dooks]

Add a configurable clock base rate for the pwm as when being built
for non-PCI the block may be sourced from an internal clock.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
--
v2:
  - removed the ifdef and merged the other clock patch in here
---
 drivers/pwm/pwm-dwc.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 235cb730c888..9067f32869f8 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
@@ -35,7 +36,6 @@
 #define DWC_TIMERS_COMP_VERSION	0xac
 
 #define DWC_TIMERS_TOTAL	8
-#define DWC_CLK_PERIOD_NS	10
 
 /* Timer Control Register */
 #define DWC_TIM_CTRL_EN		BIT(0)
@@ -54,6 +54,8 @@ struct dwc_pwm_ctx {
 struct dwc_pwm {
 	struct pwm_chip chip;
 	void __iomem *base;
+	struct clk *clk;
+	unsigned int clk_ns;
 	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
 };
 #define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
@@ -96,13 +98,13 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
 	 * periods and check are the result within HW limits between 1 and
 	 * 2^32 periods.
 	 */
-	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, DWC_CLK_PERIOD_NS);
+	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	low = tmp - 1;
 
 	tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
-				    DWC_CLK_PERIOD_NS);
+				    dwc->clk_ns);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	high = tmp - 1;
@@ -177,12 +179,12 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
 	duty += 1;
-	duty *= DWC_CLK_PERIOD_NS;
+	duty *= dwc->clk_ns;
 	state->duty_cycle = duty;
 
 	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
 	period += 1;
-	period *= DWC_CLK_PERIOD_NS;
+	period *= dwc->clk_ns;
 	period += duty;
 	state->period = period;
 
@@ -205,6 +207,7 @@ static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
 	if (!dwc)
 		return NULL;
 
+	dwc->clk_ns = 10;
 	dwc->chip.dev = dev;
 	dwc->chip.ops = &dwc_pwm_ops;
 	dwc->chip.npwm = DWC_TIMERS_TOTAL;
@@ -336,6 +339,14 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(dwc->base),
 				     "failed to map IO\n");
 
+	dwc->clk = devm_clk_get(dev, "timer");
+	if (IS_ERR(dwc->clk))
+		return dev_err_probe(dev, PTR_ERR(dwc->clk),
+				     "failed to get timer clock\n");
+
+	clk_prepare_enable(dwc->clk);
+	dwc->clk_ns = 1000000000 / clk_get_rate(dwc->clk);
+
 	ret = pwmchip_add(&dwc->chip);
 	if (ret)
 		return ret;
@@ -347,6 +358,7 @@ static int dwc_pwm_plat_remove(struct platform_device *pdev)
 {
 	struct dwc_pwm *dwc = platform_get_drvdata(pdev);
 
+	clk_disable_unprepare(dwc->clk);
 	pwmchip_remove(&dwc->chip);
 	return 0;
 }
-- 
2.35.1

[[PATCH v2] 8/9] pwm: dwc: add snps,pwm-number to limit pwm count

[Ben Dooks]

Add snps,pwm-number property to indicate if the block does not have
all 8 of the PWM blocks.

Not sure if this should be a general PWM property consider optional
for all PWM types, so have added a specific one here (there is only
one other controller with a property for PWM count at the moment)

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/pwm-dwc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 9067f32869f8..da325133d297 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -328,12 +328,20 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dwc_pwm *dwc;
+	u32 nr_pwm;
 	int ret;
 
 	dwc = dwc_pwm_alloc(dev);
 	if (!dwc)
 		return -ENOMEM;
 
+	if (!device_property_read_u32(dev, "snps,pwm-number", &nr_pwm)) {
+		if (nr_pwm > DWC_TIMERS_TOTAL)
+			dev_err(dev, "too many PWMs specified (%d)\n", nr_pwm);
+		else
+			dwc->chip.npwm = nr_pwm;
+	}
+
 	dwc->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(dwc->base))
 		return dev_err_probe(dev, PTR_ERR(dwc->base),
-- 
2.35.1

[[PATCH v2] 9/9] pwm: dwc: add PWM bit unset in get_state call

[Ben Dooks]

If we are not in PWM mode, then the output is technically a 50%
output based on a single timer instead of the high-low based on
the two counters. Add a check for the PWM mode in dwc_pwm_get_state()
and if DWC_TIM_CTRL_PWM is not set, then return a 50% cycle.

This may only be an issue on initialisation, as the rest of the
code currently assumes we're always going to have the extended
PWM mode using two counters.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/pwm-dwc.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index da325133d297..f3723c4d1e59 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -171,23 +171,35 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct dwc_pwm *dwc = to_dwc_pwm(chip);
 	u64 duty, period;
+	u32 ctrl, ld, ld2;
 
 	pm_runtime_get_sync(chip->dev);
 
-	state->enabled = !!(dwc_pwm_readl(dwc,
-				DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
+	ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
+	ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
+	ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
 
-	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
-	duty += 1;
-	duty *= dwc->clk_ns;
-	state->duty_cycle = duty;
+	state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
 
-	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
-	period += 1;
-	period *= dwc->clk_ns;
-	period += duty;
-	state->period = period;
+	/* If we're not in PWM, technically the output is a 50-50
+	 * based on the timer load-count only.
+	 */
+	if (ctrl & DWC_TIM_CTRL_PWM) {
+		duty = ld;
+		duty += 1;
+		duty *= dwc->clk_ns;
+
+		period = ld2;
+		period += 1;
+		period *= dwc->clk_ns;
+		period += duty;
+	} else {
+		duty = (ld + 1) * dwc->clk_ns;
+		period = duty * 2;
+	}
 
+	state->period = period;
+	state->duty_cycle = duty;
 	state->polarity = PWM_POLARITY_INVERSED;
 
 	pm_runtime_put_sync(chip->dev);
-- 
2.35.1

Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Ben Dooks]

On 25/07/2022 22:21, Ben Dooks wrote:

Argh, forgot the cover note on this:

Second version of updates for the DesignWare PWM driver to move it
from PCI only to both PCI and OF.

Fixes:

v2:
- fix #pwm-cells count to be 3
- fix indetation
- merge the two clock patches
- add HAS_IOMEM as a config dependency

Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Krzysztof Kozlowski]

On 25/07/2022 23:21, Ben Dooks wrote:
> Add documentation for the bindings for Synopsys' DesignWare PWM block
> as we will be adding DT/platform support to the Linux driver soon.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> --

This is not proper delimiter and causes the changelog to end up in commit.

Correct also wrong formatting of subject PATCH.

> v2:
> - fix #pwm-cells to be 3
> - fix indentation and ordering issues
> ---
>  .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
> new file mode 100644
> index 000000000000..594085e5e26f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2022 SiFive, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/snps,pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys PWM controller
> +
> +maintainers:
> +  - Ben Dooks <ben.dooks@sifive.com>
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    const: snps,pwm

This is very generic compatible. I doubt that you cover here all
Synopsys PWM designs, past and future. You need a specific compatible.

> +
> +  "#pwm-cells":
> +    const: 3
> +
> +  clocks:
> +    items:
> +      - description: Interface bus clock
> +      - description: PWM reference clock
> +
> +  clock-names:
> +    items:
> +      - const: bus
> +      - const: timer
> +
> +required:
> +  - "#pwm-cells"
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false

Missing example.


Best regards,
Krzysztof

Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Ben Dooks]

On 26/07/2022 11:05, Krzysztof Kozlowski wrote:
> On 25/07/2022 23:21, Ben Dooks wrote:
>> Add documentation for the bindings for Synopsys' DesignWare PWM block
>> as we will be adding DT/platform support to the Linux driver soon.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> --
> 
> This is not proper delimiter and causes the changelog to end up in commit.
> 
> Correct also wrong formatting of subject PATCH.

I realised that once sent and forgot the cover letter.
Maybe I'll try some more post covid recovery.

>> v2:
>> - fix #pwm-cells to be 3
>> - fix indentation and ordering issues
>> ---
>>   .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>> new file mode 100644
>> index 000000000000..594085e5e26f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>> @@ -0,0 +1,40 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2022 SiFive, Inc.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/snps,pwm.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Synopsys PWM controller
>> +
>> +maintainers:
>> +  - Ben Dooks <ben.dooks@sifive.com>
>> +
>> +allOf:
>> +  - $ref: pwm.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: snps,pwm
> 
> This is very generic compatible. I doubt that you cover here all
> Synopsys PWM designs, past and future. You need a specific compatible.

 From what I can get from the documentation (2.13a) there hasn't been
a huge external interface change and what has been added is all part
of synthesis time options.

>> +
>> +  "#pwm-cells":
>> +    const: 3
>> +
>> +  clocks:
>> +    items:
>> +      - description: Interface bus clock
>> +      - description: PWM reference clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: bus
>> +      - const: timer
>> +
>> +required:
>> +  - "#pwm-cells"
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
> 
> Missing example.

ok, thanks.

Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Krzysztof Kozlowski]

On 26/07/2022 12:12, Ben Dooks wrote:
> On 26/07/2022 11:05, Krzysztof Kozlowski wrote:
>> On 25/07/2022 23:21, Ben Dooks wrote:
>>> Add documentation for the bindings for Synopsys' DesignWare PWM block
>>> as we will be adding DT/platform support to the Linux driver soon.
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>> --
>>
>> This is not proper delimiter and causes the changelog to end up in commit.
>>
>> Correct also wrong formatting of subject PATCH.
> 
> I realised that once sent and forgot the cover letter.
> Maybe I'll try some more post covid recovery.
> 
>>> v2:
>>> - fix #pwm-cells to be 3
>>> - fix indentation and ordering issues
>>> ---
>>>   .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
>>>   1 file changed, 40 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>> new file mode 100644
>>> index 000000000000..594085e5e26f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>> @@ -0,0 +1,40 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +# Copyright (C) 2022 SiFive, Inc.
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pwm/snps,pwm.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Synopsys PWM controller
>>> +
>>> +maintainers:
>>> +  - Ben Dooks <ben.dooks@sifive.com>
>>> +
>>> +allOf:
>>> +  - $ref: pwm.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: snps,pwm
>>
>> This is very generic compatible. I doubt that you cover here all
>> Synopsys PWM designs, past and future. You need a specific compatible.
> 
>  From what I can get from the documentation (2.13a) there hasn't been
> a huge external interface change and what has been added is all part
> of synthesis time options.

But you have some specific version, right? Usually these blocks are
versioned, so you must include it. I would even argue that such generic
compatible should not be used as fallback at all, because it is simply
to generic (PWM is not some model name but common acronym),


Best regards,
Krzysztof

Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Ben Dooks]

On 26/07/2022 12:05, Krzysztof Kozlowski wrote:
> On 26/07/2022 12:12, Ben Dooks wrote:
>> On 26/07/2022 11:05, Krzysztof Kozlowski wrote:
>>> On 25/07/2022 23:21, Ben Dooks wrote:
>>>> Add documentation for the bindings for Synopsys' DesignWare PWM block
>>>> as we will be adding DT/platform support to the Linux driver soon.
>>>>
>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>>> --
>>>
>>> This is not proper delimiter and causes the changelog to end up in commit.
>>>
>>> Correct also wrong formatting of subject PATCH.
>>
>> I realised that once sent and forgot the cover letter.
>> Maybe I'll try some more post covid recovery.
>>
>>>> v2:
>>>> - fix #pwm-cells to be 3
>>>> - fix indentation and ordering issues
>>>> ---
>>>>    .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
>>>>    1 file changed, 40 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>> new file mode 100644
>>>> index 000000000000..594085e5e26f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>> @@ -0,0 +1,40 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +# Copyright (C) 2022 SiFive, Inc.
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/pwm/snps,pwm.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Synopsys PWM controller
>>>> +
>>>> +maintainers:
>>>> +  - Ben Dooks <ben.dooks@sifive.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: pwm.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: snps,pwm
>>>
>>> This is very generic compatible. I doubt that you cover here all
>>> Synopsys PWM designs, past and future. You need a specific compatible.
>>
>>   From what I can get from the documentation (2.13a) there hasn't been
>> a huge external interface change and what has been added is all part
>> of synthesis time options.
> 
> But you have some specific version, right? Usually these blocks are
> versioned, so you must include it. I would even argue that such generic
> compatible should not be used as fallback at all, because it is simply
> to generic (PWM is not some model name but common acronym),

I suppose dw-apb-timers is the actual document name, but that's already
been used for the timer mode in a number of SoCs so probably isn't going
to be useful. dw-apb-timers-pwm might be a better prefix if snps,pwm is
not going to be acceptable. (Yes, the block can be built as either a
PWM or a generic interrupt generating timer at IP generation time)

As for the version numbers, we could have the -v.vv suffix for these
blocks, but the v2.xx log has 22 entries already and only one feature
for programming (which is also a configurable one so can't be just
enabled by default - it's the 0/100 mode flag in the control registers).

I'm not sure what the v1.xx timers had, but I don't have access to this
information and we're getting these documents as second-generation so I
am not sure if we can get a v1.xx at-all (I suspect this is also going
to have a number of revisions and about 1 useful register api change
which would be the "new mode" double counter method which we currently
rely on having being implicitly enabled by the IP builder (again this
feature is still something that can be configured on IP genaration))

Given the configurability of the core, the version numbers might be
usable at some point, but it does seem to be a lot of churn for what
currently can be described by one boolean for the 0/100 feature that
might-be available. Is there a way of saying the compatible string
can be dw-apb-timers-pwm-2.[0-9][0-9][a-z] ?

Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Krzysztof Kozlowski]

On 27/07/2022 12:32, Ben Dooks wrote:
> On 26/07/2022 12:05, Krzysztof Kozlowski wrote:
>> On 26/07/2022 12:12, Ben Dooks wrote:
>>> On 26/07/2022 11:05, Krzysztof Kozlowski wrote:
>>>> On 25/07/2022 23:21, Ben Dooks wrote:
>>>>> Add documentation for the bindings for Synopsys' DesignWare PWM block
>>>>> as we will be adding DT/platform support to the Linux driver soon.
>>>>>
>>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>>>> --
>>>>
>>>> This is not proper delimiter and causes the changelog to end up in commit.
>>>>
>>>> Correct also wrong formatting of subject PATCH.
>>>
>>> I realised that once sent and forgot the cover letter.
>>> Maybe I'll try some more post covid recovery.
>>>
>>>>> v2:
>>>>> - fix #pwm-cells to be 3
>>>>> - fix indentation and ordering issues
>>>>> ---
>>>>>    .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
>>>>>    1 file changed, 40 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..594085e5e26f
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>> @@ -0,0 +1,40 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +# Copyright (C) 2022 SiFive, Inc.
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/pwm/snps,pwm.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Synopsys PWM controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Ben Dooks <ben.dooks@sifive.com>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: pwm.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: snps,pwm
>>>>
>>>> This is very generic compatible. I doubt that you cover here all
>>>> Synopsys PWM designs, past and future. You need a specific compatible.
>>>
>>>   From what I can get from the documentation (2.13a) there hasn't been
>>> a huge external interface change and what has been added is all part
>>> of synthesis time options.
>>
>> But you have some specific version, right? Usually these blocks are
>> versioned, so you must include it. I would even argue that such generic
>> compatible should not be used as fallback at all, because it is simply
>> to generic (PWM is not some model name but common acronym),
> 
> I suppose dw-apb-timers is the actual document name, but that's already
> been used for the timer mode in a number of SoCs so probably isn't going
> to be useful. dw-apb-timers-pwm might be a better prefix if snps,pwm is
> not going to be acceptable. (Yes, the block can be built as either a
> PWM or a generic interrupt generating timer at IP generation time)
> 
> As for the version numbers, we could have the -v.vv suffix for these
> blocks, but the v2.xx log has 22 entries already and only one feature
> for programming (which is also a configurable one so can't be just
> enabled by default - it's the 0/100 mode flag in the control registers).
> 
> I'm not sure what the v1.xx timers had, but I don't have access to this
> information and we're getting these documents as second-generation so I
> am not sure if we can get a v1.xx at-all (I suspect this is also going
> to have a number of revisions and about 1 useful register api change
> which would be the "new mode" double counter method which we currently
> rely on having being implicitly enabled by the IP builder (again this
> feature is still something that can be configured on IP genaration))

But why would you need v1.xx documentation?

> 
> Given the configurability of the core, the version numbers might be
> usable at some point, but it does seem to be a lot of churn for what
> currently can be described by one boolean for the 0/100 feature that
> might-be available. Is there a way of saying the compatible string
> can be dw-apb-timers-pwm-2.[0-9][0-9][a-z] ?

I don't understand why. Aren't you documenting here only v2.13a version?

Best regards,
Krzysztof

Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Ben Dooks]

On 27/07/2022 13:02, Krzysztof Kozlowski wrote:
> On 27/07/2022 12:32, Ben Dooks wrote:
>> On 26/07/2022 12:05, Krzysztof Kozlowski wrote:
>>> On 26/07/2022 12:12, Ben Dooks wrote:
>>>> On 26/07/2022 11:05, Krzysztof Kozlowski wrote:
>>>>> On 25/07/2022 23:21, Ben Dooks wrote:
>>>>>> Add documentation for the bindings for Synopsys' DesignWare PWM block
>>>>>> as we will be adding DT/platform support to the Linux driver soon.
>>>>>>
>>>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>>>>> --
>>>>>
>>>>> This is not proper delimiter and causes the changelog to end up in commit.
>>>>>
>>>>> Correct also wrong formatting of subject PATCH.
>>>>
>>>> I realised that once sent and forgot the cover letter.
>>>> Maybe I'll try some more post covid recovery.
>>>>
>>>>>> v2:
>>>>>> - fix #pwm-cells to be 3
>>>>>> - fix indentation and ordering issues
>>>>>> ---
>>>>>>     .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
>>>>>>     1 file changed, 40 insertions(+)
>>>>>>     create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..594085e5e26f
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>>> @@ -0,0 +1,40 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +# Copyright (C) 2022 SiFive, Inc.
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/pwm/snps,pwm.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Synopsys PWM controller
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Ben Dooks <ben.dooks@sifive.com>
>>>>>> +
>>>>>> +allOf:
>>>>>> +  - $ref: pwm.yaml#
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: snps,pwm
>>>>>
>>>>> This is very generic compatible. I doubt that you cover here all
>>>>> Synopsys PWM designs, past and future. You need a specific compatible.
>>>>
>>>>    From what I can get from the documentation (2.13a) there hasn't been
>>>> a huge external interface change and what has been added is all part
>>>> of synthesis time options.
>>>
>>> But you have some specific version, right? Usually these blocks are
>>> versioned, so you must include it. I would even argue that such generic
>>> compatible should not be used as fallback at all, because it is simply
>>> to generic (PWM is not some model name but common acronym),
>>
>> I suppose dw-apb-timers is the actual document name, but that's already
>> been used for the timer mode in a number of SoCs so probably isn't going
>> to be useful. dw-apb-timers-pwm might be a better prefix if snps,pwm is
>> not going to be acceptable. (Yes, the block can be built as either a
>> PWM or a generic interrupt generating timer at IP generation time)

The first thing I'd like to get sorted is should we rename this to
snps,dw-apb-timers-pwm so we can rename the file and the compatible
that goes with it.

>> As for the version numbers, we could have the -v.vv suffix for these
>> blocks, but the v2.xx log has 22 entries already and only one feature
>> for programming (which is also a configurable one so can't be just
>> enabled by default - it's the 0/100 mode flag in the control registers).
>>
>> I'm not sure what the v1.xx timers had, but I don't have access to this
>> information and we're getting these documents as second-generation so I
>> am not sure if we can get a v1.xx at-all (I suspect this is also going
>> to have a number of revisions and about 1 useful register api change
>> which would be the "new mode" double counter method which we currently
>> rely on having being implicitly enabled by the IP builder (again this
>> feature is still something that can be configured on IP genaration))
> 
> But why would you need v1.xx documentation?

I believe the driver should cover a large part of the v1.xx cores
as well, we just don't have any documentation for these to verify
this.

>>
>> Given the configurability of the core, the version numbers might be
>> usable at some point, but it does seem to be a lot of churn for what
>> currently can be described by one boolean for the 0/100 feature that
>> might-be available. Is there a way of saying the compatible string
>> can be dw-apb-timers-pwm-2.[0-9][0-9][a-z] ?
> 
> I don't understand why. Aren't you documenting here only v2.13a version?

The document as-such should cover everything I have a log for, we've not
had time to test the extension for 0or100% which was introduced in 2.11a
spec. The earliest history I have is 2.02d. I will go and see if I can
find someone who can go look for anything earlier.

As a note, it does look like all the v2.xx cores have the IP version
register in them so we can auto-detect the version from that, at least
for the DT/platform case.

> Best regards,
> Krzysztof

Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Krzysztof Kozlowski]

On 27/07/2022 15:21, Ben Dooks wrote:
> On 27/07/2022 13:02, Krzysztof Kozlowski wrote:
>> On 27/07/2022 12:32, Ben Dooks wrote:
>>> On 26/07/2022 12:05, Krzysztof Kozlowski wrote:
>>>> On 26/07/2022 12:12, Ben Dooks wrote:
>>>>> On 26/07/2022 11:05, Krzysztof Kozlowski wrote:
>>>>>> On 25/07/2022 23:21, Ben Dooks wrote:
>>>>>>> Add documentation for the bindings for Synopsys' DesignWare PWM block
>>>>>>> as we will be adding DT/platform support to the Linux driver soon.
>>>>>>>
>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>>>>>> --
>>>>>>
>>>>>> This is not proper delimiter and causes the changelog to end up in commit.
>>>>>>
>>>>>> Correct also wrong formatting of subject PATCH.
>>>>>
>>>>> I realised that once sent and forgot the cover letter.
>>>>> Maybe I'll try some more post covid recovery.
>>>>>
>>>>>>> v2:
>>>>>>> - fix #pwm-cells to be 3
>>>>>>> - fix indentation and ordering issues
>>>>>>> ---
>>>>>>>     .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
>>>>>>>     1 file changed, 40 insertions(+)
>>>>>>>     create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..594085e5e26f
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>>>> @@ -0,0 +1,40 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +# Copyright (C) 2022 SiFive, Inc.
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/pwm/snps,pwm.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Synopsys PWM controller
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Ben Dooks <ben.dooks@sifive.com>
>>>>>>> +
>>>>>>> +allOf:
>>>>>>> +  - $ref: pwm.yaml#
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: snps,pwm
>>>>>>
>>>>>> This is very generic compatible. I doubt that you cover here all
>>>>>> Synopsys PWM designs, past and future. You need a specific compatible.
>>>>>
>>>>>    From what I can get from the documentation (2.13a) there hasn't been
>>>>> a huge external interface change and what has been added is all part
>>>>> of synthesis time options.
>>>>
>>>> But you have some specific version, right? Usually these blocks are
>>>> versioned, so you must include it. I would even argue that such generic
>>>> compatible should not be used as fallback at all, because it is simply
>>>> to generic (PWM is not some model name but common acronym),
>>>
>>> I suppose dw-apb-timers is the actual document name, but that's already
>>> been used for the timer mode in a number of SoCs so probably isn't going
>>> to be useful. dw-apb-timers-pwm might be a better prefix if snps,pwm is
>>> not going to be acceptable. (Yes, the block can be built as either a
>>> PWM or a generic interrupt generating timer at IP generation time)
> 
> The first thing I'd like to get sorted is should we rename this to
> snps,dw-apb-timers-pwm so we can rename the file and the compatible
> that goes with it.

I don't have the datasheets/spec/manual for this, so I have no clue what
is it. I know though that calling it generic "pwm" is a bit too generic.

For example "snps,dw-apb-timer" is not called "snps,timer" but
DesignWare APB Timer.

>>> As for the version numbers, we could have the -v.vv suffix for these
>>> blocks, but the v2.xx log has 22 entries already and only one feature
>>> for programming (which is also a configurable one so can't be just
>>> enabled by default - it's the 0/100 mode flag in the control registers).
>>>
>>> I'm not sure what the v1.xx timers had, but I don't have access to this
>>> information and we're getting these documents as second-generation so I
>>> am not sure if we can get a v1.xx at-all (I suspect this is also going
>>> to have a number of revisions and about 1 useful register api change
>>> which would be the "new mode" double counter method which we currently
>>> rely on having being implicitly enabled by the IP builder (again this
>>> feature is still something that can be configured on IP genaration))
>>
>> But why would you need v1.xx documentation?
> 
> I believe the driver should cover a large part of the v1.xx cores
> as well, we just don't have any documentation for these to verify
> this.

Yeah, but I still don't understand what is the problem to solve in
bindings for that.

>>> Given the configurability of the core, the version numbers might be
>>> usable at some point, but it does seem to be a lot of churn for what
>>> currently can be described by one boolean for the 0/100 feature that
>>> might-be available. Is there a way of saying the compatible string
>>> can be dw-apb-timers-pwm-2.[0-9][0-9][a-z] ?
>>
>> I don't understand why. Aren't you documenting here only v2.13a version?
> 
> The document as-such should cover everything I have a log for, we've not
> had time to test the extension for 0or100% which was introduced in 2.11a
> spec. The earliest history I have is 2.02d. I will go and see if I can
> find someone who can go look for anything earlier.

Several of them might be actually compatible, so you might not need 100
different compatibles. Patterns are not allowed.

I doubt that PWM block is much more complicated than for example DW MAC,
which somehow can exist with few versions defined...

> 
> As a note, it does look like all the v2.xx cores have the IP version
> register in them so we can auto-detect the version from that, at least
> for the DT/platform case.

Auto-detection is then preferred, so just call it -v2.02d which will
cover all known v2 for you and the rest is done via autodetection.

Best regards,
Krzysztof

Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm

[Ben Dooks]

On 26/07/2022 12:05, Krzysztof Kozlowski wrote:
> On 26/07/2022 12:12, Ben Dooks wrote:
>> On 26/07/2022 11:05, Krzysztof Kozlowski wrote:
>>> On 25/07/2022 23:21, Ben Dooks wrote:
>>>> Add documentation for the bindings for Synopsys' DesignWare PWM block
>>>> as we will be adding DT/platform support to the Linux driver soon.
>>>>
>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>>> --
>>>
>>> This is not proper delimiter and causes the changelog to end up in commit.
>>>
>>> Correct also wrong formatting of subject PATCH.
>>
>> I realised that once sent and forgot the cover letter.
>> Maybe I'll try some more post covid recovery.
>>
>>>> v2:
>>>> - fix #pwm-cells to be 3
>>>> - fix indentation and ordering issues
>>>> ---
>>>>    .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
>>>>    1 file changed, 40 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>> new file mode 100644
>>>> index 000000000000..594085e5e26f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>> @@ -0,0 +1,40 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +# Copyright (C) 2022 SiFive, Inc.
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/pwm/snps,pwm.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Synopsys PWM controller
>>>> +
>>>> +maintainers:
>>>> +  - Ben Dooks <ben.dooks@sifive.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: pwm.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: snps,pwm
>>>
>>> This is very generic compatible. I doubt that you cover here all
>>> Synopsys PWM designs, past and future. You need a specific compatible.
>>
>>   From what I can get from the documentation (2.13a) there hasn't been
>> a huge external interface change and what has been added is all part
>> of synthesis time options.
> 
> But you have some specific version, right? Usually these blocks are
> versioned, so you must include it. I would even argue that such generic
> compatible should not be used as fallback at all, because it is simply
> to generic (PWM is not some model name but common acronym),

Thank you for the feedback, forgot to say that on the original reply.

-- 
Ben