[PATCH 0/3] Add support for PWM Configure and stablize for PWM kona

[PATCH 0/3] Add support for PWM Configure and stablize for PWM kona

[Sheetal Tigadoli]

Hi,
	This patchset contain support to make PWM changes configure
	and stablize
	Following are brief changes done
	a. Add support for version2 compatible string
	b. Change PWM config and stablize delay in PWM Kona

Praveen Kumar B (3):
  dt-bindings: pwm: kona: Add new compatible for new version pwm-kona
  drivers: pwm: pwm-bcm-kona: Add pwm-kona-v2 support
  ARM: dts: cygnus: Change pwm compatible to new version

 .../devicetree/bindings/pwm/brcm,kona-pwm.txt      |   2 +-
 arch/arm/boot/dts/bcm-cygnus.dtsi                  |   2 +-
 drivers/pwm/pwm-bcm-kona.c                         | 128 ++++++++++++++++-----
 3 files changed, 100 insertions(+), 32 deletions(-)

-- 
1.9.1


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

[PATCH 1/3] dt-bindings: pwm: kona: Add new compatible for new version pwm-kona

[Sheetal Tigadoli]

From: Praveen Kumar B <praveen.b@broadcom.com>

Add new compatible string for new version of pwm-kona

Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
---
 Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
index 8eae9fe..d37f697 100644
--- a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
@@ -3,7 +3,7 @@ Broadcom Kona PWM controller device tree bindings
 This controller has 6 channels.
 
 Required Properties :
-- compatible: should contain "brcm,kona-pwm"
+- compatible: should contain "brcm,kona-pwm" or "brcm,kona-pwm-v2"
 - reg: physical base address and length of the controller's registers
 - clocks: phandle + clock specifier pair for the external clock
 - #pwm-cells: Should be 3. See pwm.txt in this directory for a
-- 
1.9.1


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

[PATCH 2/3] drivers: pwm: pwm-bcm-kona: Add pwm-kona-v2 support

[Sheetal Tigadoli]

From: Praveen Kumar B <praveen.b@broadcom.com>

Add support for new version of pwm-kona.
Add support to make PWM changes configured and stable.

Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c | 128 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 98 insertions(+), 30 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 09a95ae..2b44ad8 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -45,30 +45,39 @@
  *    high or low depending on its state at that exact instant.
  */
 
-#define PWM_CONTROL_OFFSET			(0x00000000)
+#define PWM_CONTROL_OFFSET			0x00000000
 #define PWM_CONTROL_SMOOTH_SHIFT(chan)		(24 + (chan))
 #define PWM_CONTROL_TYPE_SHIFT(chan)		(16 + (chan))
 #define PWM_CONTROL_POLARITY_SHIFT(chan)	(8 + (chan))
 #define PWM_CONTROL_TRIGGER_SHIFT(chan)		(chan)
 
-#define PRESCALE_OFFSET				(0x00000004)
+#define PRESCALE_OFFSET				0x00000004
 #define PRESCALE_SHIFT(chan)			((chan) << 2)
 #define PRESCALE_MASK(chan)			(0x7 << PRESCALE_SHIFT(chan))
-#define PRESCALE_MIN				(0x00000000)
-#define PRESCALE_MAX				(0x00000007)
+#define PRESCALE_MIN				0x00000000
+#define PRESCALE_MAX				0x00000007
 
 #define PERIOD_COUNT_OFFSET(chan)		(0x00000008 + ((chan) << 3))
-#define PERIOD_COUNT_MIN			(0x00000002)
-#define PERIOD_COUNT_MAX			(0x00ffffff)
+#define PERIOD_COUNT_MIN			0x00000002
+#define PERIOD_COUNT_MAX			0x00ffffff
 
 #define DUTY_CYCLE_HIGH_OFFSET(chan)		(0x0000000c + ((chan) << 3))
-#define DUTY_CYCLE_HIGH_MIN			(0x00000000)
-#define DUTY_CYCLE_HIGH_MAX			(0x00ffffff)
+#define DUTY_CYCLE_HIGH_MIN			0x00000000
+#define DUTY_CYCLE_HIGH_MAX			0x00ffffff
+
+#define PWM_MONITOR_OFFSET			0xb0
+#define PWM_MONITOR_TIMEOUT_US			5
+
+enum kona_pwmc_ver {
+	KONA_PWM_V1 = 1,
+	KONA_PWM_V2
+};
 
 struct kona_pwmc {
 	struct pwm_chip chip;
 	void __iomem *base;
 	struct clk *clk;
+	enum kona_pwmc_ver version;
 };
 
 static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
@@ -76,11 +85,40 @@ static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
 	return container_of(_chip, struct kona_pwmc, chip);
 }
 
+static int kona_pwmc_wait_stable(struct pwm_chip *chip, unsigned int chan,
+				 unsigned int kona_ver)
+{
+	struct kona_pwmc *kp = to_kona_pwmc(chip);
+	unsigned int value;
+	unsigned int count = PWM_MONITOR_TIMEOUT_US * 1000;
+
+	switch (kona_ver) {
+	case KONA_PWM_V1:
+		/*
+		 * There must be a min 400ns delay between clearing trigger and
+		 * settingit. Failing to do this may result in no PWM signal.
+		 */
+		ndelay(400);
+		return 0;
+	case KONA_PWM_V2:
+		do {
+			value = readl(kp->base + PWM_MONITOR_OFFSET);
+			if (!(value & (BIT(chan))))
+				return 0;
+			ndelay(1);
+		} while (count--);
+
+		return -ETIMEDOUT;
+	default:
+		return -ENODEV;
+	}
+}
+
 /*
  * Clear trigger bit but set smooth bit to maintain old output.
  */
-static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
-	unsigned int chan)
+static int kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+					  unsigned int chan)
 {
 	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
@@ -88,14 +126,10 @@ static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
 	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-	/*
-	 * There must be a min 400ns delay between clearing trigger and setting
-	 * it. Failing to do this may result in no PWM signal.
-	 */
-	ndelay(400);
+	return kona_pwmc_wait_stable(&kp->chip, chan, kp->version);
 }
 
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+static int kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 {
 	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
@@ -104,8 +138,7 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-	/* Trigger bit must be held high for at least 400 ns. */
-	ndelay(400);
+	return kona_pwmc_wait_stable(&kp->chip, chan, kp->version);
 }
 
 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -115,6 +148,7 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u64 val, div, rate;
 	unsigned long prescale = PRESCALE_MIN, pc, dc;
 	unsigned int value, chan = pwm->hwpwm;
+	int ret;
 
 	/*
 	 * Find period count, duty count and prescale to suit duty_ns and
@@ -156,7 +190,12 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * validated immediately instead of on enable.
 	 */
 	if (pwm_is_enabled(pwm)) {
-		kona_pwmc_prepare_for_settings(kp, chan);
+		ret = kona_pwmc_prepare_for_settings(kp, chan);
+		if (ret < 0) {
+			dev_err(chip->dev, "failed to prepare pwm settings: %d\n",
+				ret);
+			return ret;
+		}
 
 		value = readl(kp->base + PRESCALE_OFFSET);
 		value &= ~PRESCALE_MASK(chan);
@@ -167,7 +206,12 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
 
-		kona_pwmc_apply_settings(kp, chan);
+		ret = kona_pwmc_apply_settings(kp, chan);
+		if (ret < 0) {
+			dev_err(chip->dev, "failed to apply settings: %d\n",
+				ret);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -187,7 +231,11 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 		return ret;
 	}
 
-	kona_pwmc_prepare_for_settings(kp, chan);
+	ret = kona_pwmc_prepare_for_settings(kp, chan);
+	if (ret < 0) {
+		dev_err(chip->dev, "failed to prepare pwm settings: %d\n", ret);
+		return ret;
+	}
 
 	value = readl(kp->base + PWM_CONTROL_OFFSET);
 
@@ -198,7 +246,11 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-	kona_pwmc_apply_settings(kp, chan);
+	ret = kona_pwmc_apply_settings(kp, chan);
+	if (ret < 0) {
+		dev_err(chip->dev, "failed to apply pwm settings: %d\n", ret);
+		return ret;
+	}
 
 	clk_disable_unprepare(kp->clk);
 
@@ -231,8 +283,13 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct kona_pwmc *kp = to_kona_pwmc(chip);
 	unsigned int chan = pwm->hwpwm;
 	unsigned int value;
+	int ret;
 
-	kona_pwmc_prepare_for_settings(kp, chan);
+	ret = kona_pwmc_prepare_for_settings(kp, chan);
+	if (ret < 0) {
+		dev_err(chip->dev, "failed to prepare pwm settings: %d\n", ret);
+		return;
+	}
 
 	/* Simulate a disable by configuring for zero duty */
 	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
@@ -243,7 +300,11 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	value &= ~PRESCALE_MASK(chan);
 	writel(value, kp->base + PRESCALE_OFFSET);
 
-	kona_pwmc_apply_settings(kp, chan);
+	ret = kona_pwmc_apply_settings(kp, chan);
+	if (ret < 0) {
+		dev_err(chip->dev, "failed to prepare pwm settings: %d\n", ret);
+		return;
+	}
 
 	clk_disable_unprepare(kp->clk);
 }
@@ -256,14 +317,26 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	.owner = THIS_MODULE,
 };
 
+static const struct of_device_id bcm_kona_pwmc_dt[] = {
+	{ .compatible = "brcm,kona-pwm", .data = (void *)KONA_PWM_V1},
+	{ .compatible = "brcm,kona-pwm-v2", .data = (void *)KONA_PWM_V2},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
+
 static int kona_pwmc_probe(struct platform_device *pdev)
 {
 	struct kona_pwmc *kp;
 	struct resource *res;
+	const struct of_device_id *of_id;
 	unsigned int chan;
 	unsigned int value = 0;
 	int ret = 0;
 
+	of_id = of_match_node(bcm_kona_pwmc_dt, pdev->dev.of_node);
+	if (!of_id)
+		return -ENODEV;
+
 	kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
 	if (kp == NULL)
 		return -ENOMEM;
@@ -276,6 +349,7 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 	kp->chip.npwm = 6;
 	kp->chip.of_xlate = of_pwm_xlate_with_flags;
 	kp->chip.of_pwm_n_cells = 3;
+	kp->version = (enum kona_pwmc_ver)of_id->data;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	kp->base = devm_ioremap_resource(&pdev->dev, res);
@@ -322,12 +396,6 @@ static int kona_pwmc_remove(struct platform_device *pdev)
 	return pwmchip_remove(&kp->chip);
 }
 
-static const struct of_device_id bcm_kona_pwmc_dt[] = {
-	{ .compatible = "brcm,kona-pwm" },
-	{ },
-};
-MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
-
 static struct platform_driver kona_pwmc_driver = {
 	.driver = {
 		.name = "bcm-kona-pwm",
-- 
1.9.1


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

[PATCH 3/3] ARM: dts: cygnus: Change pwm compatible to new version

[Sheetal Tigadoli]

From: Praveen Kumar B <praveen.b@broadcom.com>

Change pwm compatible to new version of pwm-kona
Add new compatible to check pwm configure status

Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
---
 arch/arm/boot/dts/bcm-cygnus.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 253df71..2a433e7 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -595,7 +595,7 @@
 		};
 
 		pwm: pwm@180aa500 {
-			compatible = "brcm,kona-pwm";
+			compatible = "brcm,kona-pwm-v2";
 			reg = <0x180aa500 0xc4>;
 			#pwm-cells = <3>;
 			clocks = <&asiu_clks BCM_CYGNUS_ASIU_PWM_CLK>;
-- 
1.9.1


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

Re: [PATCH 2/3] drivers: pwm: pwm-bcm-kona: Add pwm-kona-v2 support

[Andrew Lunn]

On Fri, Jan 11, 2019 at 10:51:15AM +0530, Sheetal Tigadoli wrote:
> From: Praveen Kumar B <praveen.b@broadcom.com>
> 
> Add support for new version of pwm-kona.
> Add support to make PWM changes configured and stable.
> 
> Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
> ---
>  drivers/pwm/pwm-bcm-kona.c | 128 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 98 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 09a95ae..2b44ad8 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -45,30 +45,39 @@
>   *    high or low depending on its state at that exact instant.
>   */
>  
> -#define PWM_CONTROL_OFFSET			(0x00000000)
> +#define PWM_CONTROL_OFFSET			0x00000000
>  #define PWM_CONTROL_SMOOTH_SHIFT(chan)		(24 + (chan))
>  #define PWM_CONTROL_TYPE_SHIFT(chan)		(16 + (chan))
>  #define PWM_CONTROL_POLARITY_SHIFT(chan)	(8 + (chan))
>  #define PWM_CONTROL_TRIGGER_SHIFT(chan)		(chan)
>  
> -#define PRESCALE_OFFSET				(0x00000004)
> +#define PRESCALE_OFFSET				0x00000004
>  #define PRESCALE_SHIFT(chan)			((chan) << 2)
>  #define PRESCALE_MASK(chan)			(0x7 << PRESCALE_SHIFT(chan))
> -#define PRESCALE_MIN				(0x00000000)
> -#define PRESCALE_MAX				(0x00000007)
> +#define PRESCALE_MIN				0x00000000
> +#define PRESCALE_MAX				0x00000007

Hi Praveen

These changes are unrelated to adding support for a new PWM. So
ideally they should be in a separate patch.

> +static int kona_pwmc_wait_stable(struct pwm_chip *chip, unsigned int chan,
> +				 unsigned int kona_ver)
> +{
> +	struct kona_pwmc *kp = to_kona_pwmc(chip);
> +	unsigned int value;
> +	unsigned int count = PWM_MONITOR_TIMEOUT_US * 1000;
> +
> +	switch (kona_ver) {
> +	case KONA_PWM_V1:
> +		/*
> +		 * There must be a min 400ns delay between clearing trigger and
> +		 * settingit. Failing to do this may result in no PWM signal.
> +		 */
> +		ndelay(400);
> +		return 0;
> +	case KONA_PWM_V2:
> +		do {
> +			value = readl(kp->base + PWM_MONITOR_OFFSET);
> +			if (!(value & (BIT(chan))))
> +				return 0;
> +			ndelay(1);
> +		} while (count--);

You can probably use readl_poll_timeout() here.

    Andrew

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

Re: [PATCH 1/3] dt-bindings: pwm: kona: Add new compatible for new version pwm-kona

[Uwe Kleine-König]

On Fri, Jan 11, 2019 at 10:51:14AM +0530, Sheetal Tigadoli wrote:
> From: Praveen Kumar B <praveen.b@broadcom.com>
> 
> Add new compatible string for new version of pwm-kona
> 
> Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
> ---
>  Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
> index 8eae9fe..d37f697 100644
> --- a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
> @@ -3,7 +3,7 @@ Broadcom Kona PWM controller device tree bindings
>  This controller has 6 channels.
>  
>  Required Properties :
> -- compatible: should contain "brcm,kona-pwm"
> +- compatible: should contain "brcm,kona-pwm" or "brcm,kona-pwm-v2"

Is v2 used on a newer generation of kona SoCs? On i.MX these variants
are usually named after the first SoC that came with the new variant. Is
this sensible here, too?

Best regards
Uwe

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

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

Re: [PATCH 2/3] drivers: pwm: pwm-bcm-kona: Add pwm-kona-v2 support

[Uwe Kleine-König]

Hello,

On Fri, Jan 11, 2019 at 10:51:15AM +0530, Sheetal Tigadoli wrote:
> From: Praveen Kumar B <praveen.b@broadcom.com>
> 
> Add support for new version of pwm-kona.
> Add support to make PWM changes configured and stable.
> 
> Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
> ---
>  drivers/pwm/pwm-bcm-kona.c | 128 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 98 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 09a95ae..2b44ad8 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -45,30 +45,39 @@
>   *    high or low depending on its state at that exact instant.
>   */
>  
> -#define PWM_CONTROL_OFFSET			(0x00000000)
> +#define PWM_CONTROL_OFFSET			0x00000000
>  #define PWM_CONTROL_SMOOTH_SHIFT(chan)		(24 + (chan))
>  #define PWM_CONTROL_TYPE_SHIFT(chan)		(16 + (chan))
>  #define PWM_CONTROL_POLARITY_SHIFT(chan)	(8 + (chan))
>  #define PWM_CONTROL_TRIGGER_SHIFT(chan)		(chan)
>  
> -#define PRESCALE_OFFSET				(0x00000004)
> +#define PRESCALE_OFFSET				0x00000004
>  #define PRESCALE_SHIFT(chan)			((chan) << 2)
>  #define PRESCALE_MASK(chan)			(0x7 << PRESCALE_SHIFT(chan))
> -#define PRESCALE_MIN				(0x00000000)
> -#define PRESCALE_MAX				(0x00000007)
> +#define PRESCALE_MIN				0x00000000
> +#define PRESCALE_MAX				0x00000007
>  
>  #define PERIOD_COUNT_OFFSET(chan)		(0x00000008 + ((chan) << 3))
> -#define PERIOD_COUNT_MIN			(0x00000002)
> -#define PERIOD_COUNT_MAX			(0x00ffffff)
> +#define PERIOD_COUNT_MIN			0x00000002
> +#define PERIOD_COUNT_MAX			0x00ffffff
>  
>  #define DUTY_CYCLE_HIGH_OFFSET(chan)		(0x0000000c + ((chan) << 3))
> -#define DUTY_CYCLE_HIGH_MIN			(0x00000000)
> -#define DUTY_CYCLE_HIGH_MAX			(0x00ffffff)
> +#define DUTY_CYCLE_HIGH_MIN			0x00000000
> +#define DUTY_CYCLE_HIGH_MAX			0x00ffffff

All these are unrelated changes.

> +#define PWM_MONITOR_OFFSET			0xb0
> +#define PWM_MONITOR_TIMEOUT_US			5
> +
> +enum kona_pwmc_ver {
> +	KONA_PWM_V1 = 1,
> +	KONA_PWM_V2
> +};
>  
>  struct kona_pwmc {
>  	struct pwm_chip chip;
>  	void __iomem *base;
>  	struct clk *clk;
> +	enum kona_pwmc_ver version;
>  };
>  
>  static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
> @@ -76,11 +85,40 @@ static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
>  	return container_of(_chip, struct kona_pwmc, chip);
>  }
>  
> +static int kona_pwmc_wait_stable(struct pwm_chip *chip, unsigned int chan,
> +				 unsigned int kona_ver)
> +{
> +	struct kona_pwmc *kp = to_kona_pwmc(chip);
> +	unsigned int value;
> +	unsigned int count = PWM_MONITOR_TIMEOUT_US * 1000;
> +
> +	switch (kona_ver) {
> +	case KONA_PWM_V1:
> +		/*
> +		 * There must be a min 400ns delay between clearing trigger and
> +		 * settingit. Failing to do this may result in no PWM signal.

Space missing.

> +		 */
> +		ndelay(400);
> +		return 0;
> +	case KONA_PWM_V2:
> +		do {
> +			value = readl(kp->base + PWM_MONITOR_OFFSET);
> +			if (!(value & (BIT(chan))))
> +				return 0;
> +			ndelay(1);
> +		} while (count--);
> +
> +		return -ETIMEDOUT;
> +	default:
> +		return -ENODEV;
> +	}

I wonder if v1 and v2 are different enough to justify a separate driver
for v2.

> +}
> +
>  /*
>   * Clear trigger bit but set smooth bit to maintain old output.
>   */
> -static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
> -	unsigned int chan)
> +static int kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
> +					  unsigned int chan)
>  {
>  	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>  
> @@ -88,14 +126,10 @@ static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
>  	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>  	writel(value, kp->base + PWM_CONTROL_OFFSET);
>  
> -	/*
> -	 * There must be a min 400ns delay between clearing trigger and setting
> -	 * it. Failing to do this may result in no PWM signal.
> -	 */
> -	ndelay(400);
> +	return kona_pwmc_wait_stable(&kp->chip, chan, kp->version);
>  }
>  
> -static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +static int kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>  {
>  	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>  
> @@ -104,8 +138,7 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>  	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
>  	writel(value, kp->base + PWM_CONTROL_OFFSET);
>  
> -	/* Trigger bit must be held high for at least 400 ns. */
> -	ndelay(400);
> +	return kona_pwmc_wait_stable(&kp->chip, chan, kp->version);
>  }
>  
>  static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> @@ -115,6 +148,7 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,

It would be great if you converted the driver to new-style (that is
implementing .apply instead of .config, .enable, .disable and
.set_polarity). This usually simplifies the driver and maybe adding
support for v2 would be easier then, too.

>  	u64 val, div, rate;
>  	unsigned long prescale = PRESCALE_MIN, pc, dc;
>  	unsigned int value, chan = pwm->hwpwm;
> +	int ret;
>  
>  	/*
>  	 * Find period count, duty count and prescale to suit duty_ns and
> @@ -156,7 +190,12 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * validated immediately instead of on enable.
>  	 */
>  	if (pwm_is_enabled(pwm)) {
> -		kona_pwmc_prepare_for_settings(kp, chan);
> +		ret = kona_pwmc_prepare_for_settings(kp, chan);
> +		if (ret < 0) {
> +			dev_err(chip->dev, "failed to prepare pwm settings: %d\n",
> +				ret);
> +			return ret;
> +		}
>  
>  		value = readl(kp->base + PRESCALE_OFFSET);
>  		value &= ~PRESCALE_MASK(chan);
> @@ -167,7 +206,12 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  		writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
>  
> -		kona_pwmc_apply_settings(kp, chan);
> +		ret = kona_pwmc_apply_settings(kp, chan);
> +		if (ret < 0) {
> +			dev_err(chip->dev, "failed to apply settings: %d\n",
> +				ret);
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
> @@ -187,7 +231,11 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return ret;
>  	}
>  
> -	kona_pwmc_prepare_for_settings(kp, chan);
> +	ret = kona_pwmc_prepare_for_settings(kp, chan);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to prepare pwm settings: %d\n", ret);
> +		return ret;
> +	}
>  
>  	value = readl(kp->base + PWM_CONTROL_OFFSET);
>  
> @@ -198,7 +246,11 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	writel(value, kp->base + PWM_CONTROL_OFFSET);
>  
> -	kona_pwmc_apply_settings(kp, chan);
> +	ret = kona_pwmc_apply_settings(kp, chan);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to apply pwm settings: %d\n", ret);
> +		return ret;
> +	}
>  
>  	clk_disable_unprepare(kp->clk);
>  
> @@ -231,8 +283,13 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	struct kona_pwmc *kp = to_kona_pwmc(chip);
>  	unsigned int chan = pwm->hwpwm;
>  	unsigned int value;
> +	int ret;
>  
> -	kona_pwmc_prepare_for_settings(kp, chan);
> +	ret = kona_pwmc_prepare_for_settings(kp, chan);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to prepare pwm settings: %d\n", ret);
> +		return;
> +	}
>  
>  	/* Simulate a disable by configuring for zero duty */
>  	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> @@ -243,7 +300,11 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	value &= ~PRESCALE_MASK(chan);
>  	writel(value, kp->base + PRESCALE_OFFSET);
>  
> -	kona_pwmc_apply_settings(kp, chan);
> +	ret = kona_pwmc_apply_settings(kp, chan);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to prepare pwm settings: %d\n", ret);
> +		return;
> +	}
>  
>  	clk_disable_unprepare(kp->clk);
>  }
> @@ -256,14 +317,26 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	.owner = THIS_MODULE,
>  };
>  
> +static const struct of_device_id bcm_kona_pwmc_dt[] = {
> +	{ .compatible = "brcm,kona-pwm", .data = (void *)KONA_PWM_V1},
> +	{ .compatible = "brcm,kona-pwm-v2", .data = (void *)KONA_PWM_V2},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
> +
>  static int kona_pwmc_probe(struct platform_device *pdev)
>  {
>  	struct kona_pwmc *kp;
>  	struct resource *res;
> +	const struct of_device_id *of_id;
>  	unsigned int chan;
>  	unsigned int value = 0;
>  	int ret = 0;
>  
> +	of_id = of_match_node(bcm_kona_pwmc_dt, pdev->dev.of_node);
> +	if (!of_id)
> +		return -ENODEV;
> +
>  	kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
>  	if (kp == NULL)
>  		return -ENOMEM;
> @@ -276,6 +349,7 @@ static int kona_pwmc_probe(struct platform_device *pdev)
>  	kp->chip.npwm = 6;
>  	kp->chip.of_xlate = of_pwm_xlate_with_flags;
>  	kp->chip.of_pwm_n_cells = 3;
> +	kp->version = (enum kona_pwmc_ver)of_id->data;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	kp->base = devm_ioremap_resource(&pdev->dev, res);
> @@ -322,12 +396,6 @@ static int kona_pwmc_remove(struct platform_device *pdev)
>  	return pwmchip_remove(&kp->chip);
>  }
>  
> -static const struct of_device_id bcm_kona_pwmc_dt[] = {
> -	{ .compatible = "brcm,kona-pwm" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
> -
>  static struct platform_driver kona_pwmc_driver = {
>  	.driver = {
>  		.name = "bcm-kona-pwm",
> -- 
> 1.9.1
> 
> 

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

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

Re: [PATCH 1/3] dt-bindings: pwm: kona: Add new compatible for new version pwm-kona

[Scott Branden]

Hi Uwe

On 2019-01-11 12:48 p.m., Uwe Kleine-König wrote:
> On Fri, Jan 11, 2019 at 10:51:14AM +0530, Sheetal Tigadoli wrote:
>> From: Praveen Kumar B <praveen.b@broadcom.com>
>>
>> Add new compatible string for new version of pwm-kona
>>
>> Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
>> ---
>>   Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
>> index 8eae9fe..d37f697 100644
>> --- a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
>> +++ b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
>> @@ -3,7 +3,7 @@ Broadcom Kona PWM controller device tree bindings
>>   This controller has 6 channels.
>>   
>>   Required Properties :
>> -- compatible: should contain "brcm,kona-pwm"
>> +- compatible: should contain "brcm,kona-pwm" or "brcm,kona-pwm-v2"
> Is v2 used on a newer generation of kona SoCs? On i.MX these variants
> are usually named after the first SoC that came with the new variant. Is
> this sensible here, too?

It doesn't make as much sense here as different revs of the IP block are 
picked up based on various decisions.

A new SoC could decide to use an old version.

>
> Best regards
> Uwe
>

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

Re: [PATCH 1/3] dt-bindings: pwm: kona: Add new compatible for new version pwm-kona

[Uwe Kleine-König]

Hello Scott,

On Fri, Jan 11, 2019 at 01:28:45PM -0800, Scott Branden wrote:
> On 2019-01-11 12:48 p.m., Uwe Kleine-König wrote:
> > On Fri, Jan 11, 2019 at 10:51:14AM +0530, Sheetal Tigadoli wrote:
> > > From: Praveen Kumar B <praveen.b@broadcom.com>
> > > 
> > > Add new compatible string for new version of pwm-kona
> > > 
> > > Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
> > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> > > Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
> > > ---
> > >   Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
> > > index 8eae9fe..d37f697 100644
> > > --- a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
> > > +++ b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
> > > @@ -3,7 +3,7 @@ Broadcom Kona PWM controller device tree bindings
> > >   This controller has 6 channels.
> > >   Required Properties :
> > > -- compatible: should contain "brcm,kona-pwm"
> > > +- compatible: should contain "brcm,kona-pwm" or "brcm,kona-pwm-v2"
> > Is v2 used on a newer generation of kona SoCs? On i.MX these variants
> > are usually named after the first SoC that came with the new variant. Is
> > this sensible here, too?
> 
> It doesn't make as much sense here as different revs of the IP block are
> picked up based on various decisions.
> 
> A new SoC could decide to use an old version.

IMHO this is no reason to not use the name of the oldest SoC with this
variant. I don't know how the SoC names are in the broadcom family, but
if they were (in order of age, oldest first):

	ant
	bear
	crocodile

and ant and crocodile use the same IP block we would have

a) with v1, v2:

	ant:
		compatible = "brcm,kona-pwm-v1";
	bear:
		compatible = "brcm,kona-pwm-v2";
	crocodile:
		compatible = "brcm,kona-pwm-v1";

; and 

b) with the SoC naming:

	ant:
		compatible = "brcm,kona-ant-pwm";
	bear:
		compatible = "brcm,kona-bear-pwm";
	crocodile:
		compatible = "brcm,kona-crocodile-pwm", "brcm,kona-ant-pwm";

(If you want, drop "brcm,kona-crocodile-pwm", but keeping it is more
defensive.) 

I like b) (with "...-crocodile-...") better than a). crocodile using
"...-ant-..." is not more ugly than crocodile using "...-v1". This is
also a tad more robust because if broadcom releases kona-dolphin and
someone finds a minor difference between the IPs used on ant and
crocodile it depends on the order of these events who gets v3, while
with the SoC naming the result is clear.

(OK, and given that "brcm,kona-pwm" is already fixed, both approaches
need slight adaption, but I guess you still get what I meant.)

Best regards
Uwe


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

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

Re: [PATCH 1/3] dt-bindings: pwm: kona: Add new compatible for new version pwm-kona

[Scott Branden]

Hi Uwe,

On 2019-01-12 7:05 a.m., Uwe Kleine-König wrote:
> Hello Scott,
>
> On Fri, Jan 11, 2019 at 01:28:45PM -0800, Scott Branden wrote:
>> On 2019-01-11 12:48 p.m., Uwe Kleine-König wrote:
>>> On Fri, Jan 11, 2019 at 10:51:14AM +0530, Sheetal Tigadoli wrote:
>>>> From: Praveen Kumar B <praveen.b@broadcom.com>
>>>>
>>>> Add new compatible string for new version of pwm-kona
>>>>
>>>> Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
>>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>>> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
>>>> index 8eae9fe..d37f697 100644
>>>> --- a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
>>>> +++ b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
>>>> @@ -3,7 +3,7 @@ Broadcom Kona PWM controller device tree bindings
>>>>    This controller has 6 channels.
>>>>    Required Properties :
>>>> -- compatible: should contain "brcm,kona-pwm"
>>>> +- compatible: should contain "brcm,kona-pwm" or "brcm,kona-pwm-v2"
>>> Is v2 used on a newer generation of kona SoCs? On i.MX these variants
>>> are usually named after the first SoC that came with the new variant. Is
>>> this sensible here, too?
>> It doesn't make as much sense here as different revs of the IP block are
>> picked up based on various decisions.
>>
>> A new SoC could decide to use an old version.
> IMHO this is no reason to not use the name of the oldest SoC with this
> variant. I don't know how the SoC names are in the broadcom family, but
> if they were (in order of age, oldest first):
>
> 	ant
> 	bear
> 	crocodile
>
> and ant and crocodile use the same IP block we would have
>
> a) with v1, v2:
>
> 	ant:
> 		compatible = "brcm,kona-pwm-v1";
> 	bear:
> 		compatible = "brcm,kona-pwm-v2";
> 	crocodile:
> 		compatible = "brcm,kona-pwm-v1";
>
> ; and
>
> b) with the SoC naming:
>
> 	ant:
> 		compatible = "brcm,kona-ant-pwm";
> 	bear:
> 		compatible = "brcm,kona-bear-pwm";
> 	crocodile:
> 		compatible = "brcm,kona-crocodile-pwm", "brcm,kona-ant-pwm";
>
> (If you want, drop "brcm,kona-crocodile-pwm", but keeping it is more
> defensive.)
>
> I like b) (with "...-crocodile-...") better than a). crocodile using
> "...-ant-..." is not more ugly than crocodile using "...-v1". This is
> also a tad more robust because if broadcom releases kona-dolphin and
> someone finds a minor difference between the IPs used on ant and
> crocodile it depends on the order of these events who gets v3, while
> with the SoC naming the result is clear.
>
> (OK, and given that "brcm,kona-pwm" is already fixed, both approaches
> need slight adaption, but I guess you still get what I meant.)

Thanks for your thoughts and explanation.

It is unfortunate devicetree has no proper guidelines or documentation on

binding naming.  In the interest of getting this upstream we can name it

"brcm, omega-pwm".  We can drop kona from the binding name as that 
architecture

is really no more - only IP derived from it is - hence the name kona-v2 
previously.

>
> Best regards
> Uwe
>
>
Cheers,
Scott

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

Re: [PATCH 1/3] dt-bindings: pwm: kona: Add new compatible for new version pwm-kona

[Rob Herring]

On Tue, Jan 15, 2019 at 04:14:21PM -0800, Scott Branden wrote:
> Hi Uwe,
> 
> On 2019-01-12 7:05 a.m., Uwe Kleine-König wrote:
> > Hello Scott,
> > 
> > On Fri, Jan 11, 2019 at 01:28:45PM -0800, Scott Branden wrote:
> > > On 2019-01-11 12:48 p.m., Uwe Kleine-König wrote:
> > > > On Fri, Jan 11, 2019 at 10:51:14AM +0530, Sheetal Tigadoli wrote:
> > > > > From: Praveen Kumar B <praveen.b@broadcom.com>
> > > > > 
> > > > > Add new compatible string for new version of pwm-kona
> > > > > 
> > > > > Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
> > > > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > > > Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> > > > > Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
> > > > > ---
> > > > >    Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
> > > > > index 8eae9fe..d37f697 100644
> > > > > --- a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
> > > > > +++ b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
> > > > > @@ -3,7 +3,7 @@ Broadcom Kona PWM controller device tree bindings
> > > > >    This controller has 6 channels.
> > > > >    Required Properties :
> > > > > -- compatible: should contain "brcm,kona-pwm"
> > > > > +- compatible: should contain "brcm,kona-pwm" or "brcm,kona-pwm-v2"
> > > > Is v2 used on a newer generation of kona SoCs? On i.MX these variants
> > > > are usually named after the first SoC that came with the new variant. Is
> > > > this sensible here, too?
> > > It doesn't make as much sense here as different revs of the IP block are
> > > picked up based on various decisions.
> > > 
> > > A new SoC could decide to use an old version.
> > IMHO this is no reason to not use the name of the oldest SoC with this
> > variant. I don't know how the SoC names are in the broadcom family, but
> > if they were (in order of age, oldest first):
> > 
> > 	ant
> > 	bear
> > 	crocodile
> > 
> > and ant and crocodile use the same IP block we would have
> > 
> > a) with v1, v2:
> > 
> > 	ant:
> > 		compatible = "brcm,kona-pwm-v1";
> > 	bear:
> > 		compatible = "brcm,kona-pwm-v2";
> > 	crocodile:
> > 		compatible = "brcm,kona-pwm-v1";

Version numbers can be fine, but generally only as fallbacks as even the 
same IP version can be integrated into an SoC differently.

The other issue with versions is they should be meanful such as 
corresponding to version tags in IP repos. Often, I'd guess anything 
with a 'v1' is just what some s/w person made up. Of course, we only 
can really know that for opensource IP or programmable logic IP.

If you do use versions, document what the versioning scheme is.

> > 
> > ; and
> > 
> > b) with the SoC naming:
> > 
> > 	ant:
> > 		compatible = "brcm,kona-ant-pwm";
> > 	bear:
> > 		compatible = "brcm,kona-bear-pwm";
> > 	crocodile:
> > 		compatible = "brcm,kona-crocodile-pwm", "brcm,kona-ant-pwm";

This is the recommended practice.

> > 
> > (If you want, drop "brcm,kona-crocodile-pwm", but keeping it is more
> > defensive.)

Generally, you should have "brcm,kona-crocodile-pwm" in case there's 
some difference found later. Then you can support the bug or feature 
without a DT change.

> > 
> > I like b) (with "...-crocodile-...") better than a). crocodile using
> > "...-ant-..." is not more ugly than crocodile using "...-v1". This is
> > also a tad more robust because if broadcom releases kona-dolphin and
> > someone finds a minor difference between the IPs used on ant and
> > crocodile it depends on the order of these events who gets v3, while
> > with the SoC naming the result is clear.
> > 
> > (OK, and given that "brcm,kona-pwm" is already fixed, both approaches
> > need slight adaption, but I guess you still get what I meant.)
> 
> Thanks for your thoughts and explanation.
> 
> It is unfortunate devicetree has no proper guidelines or documentation on
> 
> binding naming.  In the interest of getting this upstream we can name it

Surely we've captured that somewhere...

> 
> "brcm, omega-pwm".  We can drop kona from the binding name as that
> architecture
> 
> is really no more - only IP derived from it is - hence the name kona-v2
> previously.
> 
> > 
> > Best regards
> > Uwe
> > 
> > 
> Cheers,
> Scott

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

Re: [PATCH 1/3] dt-bindings: pwm: kona: Add new compatible for new version pwm-kona

[Scott Branden]

Hi Rob,

On 2019-01-21 3:11 p.m., Rob Herring wrote:
> On Tue, Jan 15, 2019 at 04:14:21PM -0800, Scott Branden wrote:
>> Hi Uwe,
>>
>> On 2019-01-12 7:05 a.m., Uwe Kleine-König wrote:
>>> Hello Scott,
>>>
>>> On Fri, Jan 11, 2019 at 01:28:45PM -0800, Scott Branden wrote:
>>>> On 2019-01-11 12:48 p.m., Uwe Kleine-König wrote:
>>>>> On Fri, Jan 11, 2019 at 10:51:14AM +0530, Sheetal Tigadoli wrote:
>>>>>> From: Praveen Kumar B <praveen.b@broadcom.com>
>>>>>>
>>>>>> Add new compatible string for new version of pwm-kona
>>>>>>
>>>>>> Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
>>>>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>>>>> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
>>>>>> ---
>>>>>>     Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
>>>>>> index 8eae9fe..d37f697 100644
>>>>>> --- a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
>>>>>> +++ b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt
>>>>>> @@ -3,7 +3,7 @@ Broadcom Kona PWM controller device tree bindings
>>>>>>     This controller has 6 channels.
>>>>>>     Required Properties :
>>>>>> -- compatible: should contain "brcm,kona-pwm"
>>>>>> +- compatible: should contain "brcm,kona-pwm" or "brcm,kona-pwm-v2"
>>>>> Is v2 used on a newer generation of kona SoCs? On i.MX these variants
>>>>> are usually named after the first SoC that came with the new variant. Is
>>>>> this sensible here, too?
>>>> It doesn't make as much sense here as different revs of the IP block are
>>>> picked up based on various decisions.
>>>>
>>>> A new SoC could decide to use an old version.
>>> IMHO this is no reason to not use the name of the oldest SoC with this
>>> variant. I don't know how the SoC names are in the broadcom family, but
>>> if they were (in order of age, oldest first):
>>>
>>> 	ant
>>> 	bear
>>> 	crocodile
>>>
>>> and ant and crocodile use the same IP block we would have
>>>
>>> a) with v1, v2:
>>>
>>> 	ant:
>>> 		compatible = "brcm,kona-pwm-v1";
>>> 	bear:
>>> 		compatible = "brcm,kona-pwm-v2";
>>> 	crocodile:
>>> 		compatible = "brcm,kona-pwm-v1";
> Version numbers can be fine, but generally only as fallbacks as even the
> same IP version can be integrated into an SoC differently.
>
> The other issue with versions is they should be meanful such as
> corresponding to version tags in IP repos. Often, I'd guess anything
> with a 'v1' is just what some s/w person made up. Of course, we only
> can really know that for opensource IP or programmable logic IP.
>
> If you do use versions, document what the versioning scheme is.
>
>>> ; and
>>>
>>> b) with the SoC naming:
>>>
>>> 	ant:
>>> 		compatible = "brcm,kona-ant-pwm";
>>> 	bear:
>>> 		compatible = "brcm,kona-bear-pwm";
>>> 	crocodile:
>>> 		compatible = "brcm,kona-crocodile-pwm", "brcm,kona-ant-pwm";
> This is the recommended practice.
>
>>> (If you want, drop "brcm,kona-crocodile-pwm", but keeping it is more
>>> defensive.)
> Generally, you should have "brcm,kona-crocodile-pwm" in case there's
> some difference found later. Then you can support the bug or feature
> without a DT change.

No DT change would be necessary in any case.

A check against the SOC type in the driver without additional DT 
compatibility strings could be done.

>
>>> I like b) (with "...-crocodile-...") better than a). crocodile using
>>> "...-ant-..." is not more ugly than crocodile using "...-v1". This is
>>> also a tad more robust because if broadcom releases kona-dolphin and
>>> someone finds a minor difference between the IPs used on ant and
>>> crocodile it depends on the order of these events who gets v3, while
>>> with the SoC naming the result is clear.
>>>
>>> (OK, and given that "brcm,kona-pwm" is already fixed, both approaches
>>> need slight adaption, but I guess you still get what I meant.)
>> Thanks for your thoughts and explanation.
>>
>> It is unfortunate devicetree has no proper guidelines or documentation on
>>
>> binding naming.  In the interest of getting this upstream we can name it
> Surely we've captured that somewhere...

Please point me at such documentation.

There is no consistency in kernel drivers from what I have seen.

>
>> "brcm, omega-pwm".  We can drop kona from the binding name as that
>> architecture
>>
>> is really no more - only IP derived from it is - hence the name kona-v2
>> previously.
>>
>>> Best regards
>>> Uwe
>>>
>>>
>> Cheers,
>> Scott

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