[PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's

[PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's

[Yendapally Reddy Dhananjaya Reddy]

This patchset contains the pwm support for the Broadcom's iProc SoC's.

The first patch provides the documentation details and the
second patch contains the controller support details. The third patch
contains the enable method for Northstar Plus SoC.

This patch series has been tested on NSP bcm958625HR board with oscilloscope.
The pwm is not enabled for this board as the pins are open and not used.

This patch series is based on v4.6.0-rc1 and is available from github
repo: https://github.com/Broadcom/cygnus-linux.git
branch: iproc-pwm-v1

This patch series was originally sent on 03/01/16.
See https://lkml.org/lkml/2016/3/1/487

Yendapally Reddy Dhananjaya Reddy (3):
  Documentation: dt: Add Broadcom iproc pwm controller binding
  pwm: kona: Add support for Broadcom iproc pwm controller
  ARM: dts: NSP: Add PWM Support to DT

 .../devicetree/bindings/pwm/brcm,kona-pwm.txt      |   2 +-
 arch/arm/boot/dts/bcm-nsp.dtsi                     |   8 +
 drivers/pwm/Kconfig                                |   6 +-
 drivers/pwm/pwm-bcm-kona.c                         | 183 ++++++++++++++++++---
 4 files changed, 172 insertions(+), 27 deletions(-)

-- 
2.1.0

[PATCH RESEND 1/3] Documentation: dt: Add Broadcom iproc pwm controller binding

[Yendapally Reddy Dhananjaya Reddy]

Add a binding for Broadcom iproc pwm controller

Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 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..5036f63 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,iproc-pwm"
 - 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
-- 
2.1.0

[PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller

[Yendapally Reddy Dhananjaya Reddy]

Update the kona driver to support Broadcom iproc pwm controller

Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
---
 drivers/pwm/Kconfig        |   6 +-
 drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 163 insertions(+), 26 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c182efc..e45ea33 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -76,9 +76,11 @@ config PWM_ATMEL_TCB
 
 config PWM_BCM_KONA
 	tristate "Kona PWM support"
-	depends on ARCH_BCM_MOBILE
+	depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC
+	default ARCH_BCM_IPROC
 	help
-	  Generic PWM framework driver for Broadcom Kona PWM block.
+	  Generic PWM framework driver for Broadcom Kona PWM block. The
+	  same block is also used in Broadcom iProc SoC's.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-bcm-kona.
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index c634183..ef152e3a 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -19,6 +19,7 @@
 #include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
@@ -47,30 +48,90 @@
 
 #define PWM_CONTROL_OFFSET			(0x00000000)
 #define PWM_CONTROL_SMOOTH_SHIFT(chan)		(24 + (chan))
-#define PWM_CONTROL_TYPE_SHIFT(chan)		(16 + (chan))
+#define PWM_CONTROL_TYPE_SHIFT(shift, chan)	(shift + chan)
 #define PWM_CONTROL_POLARITY_SHIFT(chan)	(8 + (chan))
 #define PWM_CONTROL_TRIGGER_SHIFT(chan)		(chan)
 
 #define PRESCALE_OFFSET				(0x00000004)
-#define PRESCALE_SHIFT(chan)			((chan) << 2)
-#define PRESCALE_MASK(chan)			(0x7 << PRESCALE_SHIFT(chan))
+#define PRESCALE_SHIFT				(0x00000004)
+#define PRESCALE_MASK				(0x00000007)
 #define PRESCALE_MIN				(0x00000000)
 #define PRESCALE_MAX				(0x00000007)
 
-#define PERIOD_COUNT_OFFSET(chan)		(0x00000008 + ((chan) << 3))
+#define PERIOD_COUNT_OFFSET(offset, chan)	(offset + (chan << 3))
 #define PERIOD_COUNT_MIN			(0x00000002)
 #define PERIOD_COUNT_MAX			(0x00ffffff)
+#define KONA_PERIOD_COUNT_OFFSET		(0x00000008)
 
-#define DUTY_CYCLE_HIGH_OFFSET(chan)		(0x0000000c + ((chan) << 3))
+#define DUTY_CYCLE_HIGH_OFFSET(offset, chan)	(offset + (chan << 3))
 #define DUTY_CYCLE_HIGH_MIN			(0x00000000)
 #define DUTY_CYCLE_HIGH_MAX			(0x00ffffff)
+#define KONA_DUTY_CYCLE_HIGH_OFFSET		(0x0000000c)
+
+#define PWM_CHANNEL_CNT				(0x00000006)
+#define SIGNAL_PUSH_PULL			(0x00000001)
+#define PWMOUT_TYPE_SHIFT			(0x00000010)
+
+#define IPROC_PRESCALE_OFFSET			(0x00000024)
+#define IPROC_PRESCALE_SHIFT			(0x00000006)
+#define IPROC_PRESCALE_MAX			(0x0000003f)
+
+#define IPROC_PERIOD_COUNT_OFFSET		(0x00000004)
+#define IPROC_PERIOD_COUNT_MIN			(0x00000002)
+#define IPROC_PERIOD_COUNT_MAX			(0x0000ffff)
+
+#define IPROC_DUTY_CYCLE_HIGH_OFFSET		(0x00000008)
+#define IPROC_DUTY_CYCLE_HIGH_MIN		(0x00000000)
+#define IPROC_DUTY_CYCLE_HIGH_MAX		(0x0000ffff)
+
+#define IPROC_PWM_CHANNEL_CNT			(0x00000004)
+#define IPROC_SIGNAL_PUSH_PULL			(0x00000000)
+#define IPROC_PWMOUT_TYPE_SHIFT			(0x0000000f)
+
+/*
+ * pwm controller reg structure
+ *
+ * @prescale_offset: prescale register offset
+ * @period_offset: period register offset
+ * @duty_offset: duty register offset
+ * @no_of_channels: number of channels
+ * @out_type_shift: out type shift in the register
+ * @signal_type: push-pull or open drain
+ * @prescale_max: prescale max
+ * @prescale_shift: prescale shift in register
+ * @prescale_ch_ascending: prescale ch order in prescale register
+ * @duty_cycle_max: value of max duty cycle
+ * @duty_cycle_min: value of min duty cycle
+ * @period_count_max: max period count val
+ * @period_count_min: min period count val
+ * @smooth_output_support: pwm smooth output support
+ */
+struct kona_pwmc_reg {
+	u32 prescale_offset;
+	u32 period_offset;
+	u32 duty_offset;
+	u32 no_of_channels;
+	u32 out_type_shift;
+	u32 signal_type;
+	u32 prescale_max;
+	u32 prescale_shift;
+	bool prescale_ch_ascending;
+	u32 duty_cycle_max;
+	u32 duty_cycle_min;
+	u32 period_count_max;
+	u32 period_count_min;
+	bool smooth_output_support;
+};
 
 struct kona_pwmc {
 	struct pwm_chip chip;
 	void __iomem *base;
 	struct clk *clk;
+	const struct kona_pwmc_reg *reg;
 };
 
+static const struct of_device_id bcm_kona_pwmc_dt[];
+
 static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
 {
 	return container_of(_chip, struct kona_pwmc, chip);
@@ -84,7 +145,9 @@ static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
 {
 	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+	if (kp->reg->smooth_output_support)
+		value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+
 	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
@@ -100,7 +163,9 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
 	/* Set trigger bit and clear smooth bit to apply new settings */
-	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+	if (kp->reg->smooth_output_support)
+		value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+
 	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
@@ -138,15 +203,17 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		dc = div64_u64(val, div);
 
 		/* If duty_ns or period_ns are not achievable then return */
-		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+		if (pc < kp->reg->period_count_min ||
+						dc < kp->reg->duty_cycle_min)
 			return -EINVAL;
 
 		/* If pc and dc are in bounds, the calculation is done */
-		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
+		if (pc <= kp->reg->period_count_max &&
+						dc <= kp->reg->duty_cycle_max)
 			break;
 
 		/* Otherwise, increase prescale and recalculate pc and dc */
-		if (++prescale > PRESCALE_MAX)
+		if (++prescale > kp->reg->prescale_max)
 			return -EINVAL;
 	}
 
@@ -156,16 +223,30 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * validated immediately instead of on enable.
 	 */
 	if (pwm_is_enabled(pwm)) {
+		u32 ch_pre_shift = kp->reg->prescale_shift;
+
 		kona_pwmc_prepare_for_settings(kp, chan);
 
-		value = readl(kp->base + PRESCALE_OFFSET);
-		value &= ~PRESCALE_MASK(chan);
-		value |= prescale << PRESCALE_SHIFT(chan);
-		writel(value, kp->base + PRESCALE_OFFSET);
+		if (kp->reg->prescale_ch_ascending)
+			/*
+			 * The prescale bits mask is in ascending
+			 * order in the register
+			 * "ch(n-2)bits..ch(n-1)bits..ch(n)bits".
+			 */
+			ch_pre_shift *= ((kp->reg->no_of_channels - 1) - chan);
+		else
+			ch_pre_shift *= chan;
 
-		writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+		value = readl(kp->base + kp->reg->prescale_offset);
+		value &= ~(kp->reg->prescale_max << ch_pre_shift);
+		value |= prescale << ch_pre_shift;
+		writel(value, kp->base + kp->reg->prescale_offset);
 
-		writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+		writel(pc, kp->base +
+			PERIOD_COUNT_OFFSET(kp->reg->period_offset, chan));
+
+		writel(dc, kp->base +
+			DUTY_CYCLE_HIGH_OFFSET(kp->reg->duty_offset, chan));
 
 		kona_pwmc_apply_settings(kp, chan);
 	}
@@ -231,17 +312,29 @@ 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;
+	u32 ch_pre_shift = kp->reg->prescale_shift;
+
+	if (kp->reg->prescale_ch_ascending)
+		/*
+		 * The prescale bits mask is in ascending order
+		 * in the register "ch(n-2)bits..ch(n-1)bits..ch(n)bits".
+		 */
+		ch_pre_shift *= ((kp->reg->no_of_channels - 1) - chan);
+	else
+		ch_pre_shift *= chan;
 
 	kona_pwmc_prepare_for_settings(kp, chan);
 
 	/* Simulate a disable by configuring for zero duty */
-	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
-	writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
+	writel(0, kp->base +
+			DUTY_CYCLE_HIGH_OFFSET(kp->reg->duty_offset, chan));
+	writel(0, kp->base +
+			PERIOD_COUNT_OFFSET(kp->reg->period_offset, chan));
 
 	/* Set prescale to 0 for this channel */
-	value = readl(kp->base + PRESCALE_OFFSET);
-	value &= ~PRESCALE_MASK(chan);
-	writel(value, kp->base + PRESCALE_OFFSET);
+	value = readl(kp->base + kp->reg->prescale_offset);
+	value &= ~(kp->reg->prescale_max << ch_pre_shift);
+	writel(value, kp->base + kp->reg->prescale_offset);
 
 	kona_pwmc_apply_settings(kp, chan);
 
@@ -263,17 +356,23 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 	unsigned int chan;
 	unsigned int value = 0;
 	int ret = 0;
+	const struct of_device_id *match;
 
 	kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
 	if (kp == NULL)
 		return -ENOMEM;
 
+	match = of_match_device(bcm_kona_pwmc_dt, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+
+	kp->reg = (struct kona_pwmc_reg *)match->data;
 	platform_set_drvdata(pdev, kp);
 
 	kp->chip.dev = &pdev->dev;
 	kp->chip.ops = &kona_pwm_ops;
 	kp->chip.base = -1;
-	kp->chip.npwm = 6;
+	kp->chip.npwm = kp->reg->no_of_channels;
 	kp->chip.of_xlate = of_pwm_xlate_with_flags;
 	kp->chip.of_pwm_n_cells = 3;
 	kp->chip.can_sleep = true;
@@ -298,7 +397,8 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 
 	/* Set push/pull for all channels */
 	for (chan = 0; chan < kp->chip.npwm; chan++)
-		value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
+		value |= (kp->reg->signal_type <<
+			PWM_CONTROL_TYPE_SHIFT(kp->reg->out_type_shift, chan));
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
@@ -323,8 +423,43 @@ static int kona_pwmc_remove(struct platform_device *pdev)
 	return pwmchip_remove(&kp->chip);
 }
 
+static const struct kona_pwmc_reg kona_pwmc_reg_data = {
+	.prescale_offset = PRESCALE_OFFSET,
+	.period_offset = KONA_PERIOD_COUNT_OFFSET,
+	.duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET,
+	.no_of_channels = PWM_CHANNEL_CNT,
+	.out_type_shift = PWMOUT_TYPE_SHIFT,
+	.signal_type = SIGNAL_PUSH_PULL,
+	.prescale_max = PRESCALE_MAX,
+	.prescale_shift = PRESCALE_SHIFT,
+	.prescale_ch_ascending = false,
+	.duty_cycle_max = DUTY_CYCLE_HIGH_MAX,
+	.duty_cycle_min = DUTY_CYCLE_HIGH_MIN,
+	.period_count_max = PERIOD_COUNT_MAX,
+	.period_count_min = PERIOD_COUNT_MIN,
+	.smooth_output_support = true,
+};
+
+static const struct kona_pwmc_reg iproc_pwmc_reg_data = {
+	.prescale_offset = IPROC_PRESCALE_OFFSET,
+	.period_offset = IPROC_PERIOD_COUNT_OFFSET,
+	.duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET,
+	.no_of_channels = IPROC_PWM_CHANNEL_CNT,
+	.out_type_shift = IPROC_PWMOUT_TYPE_SHIFT,
+	.signal_type = IPROC_SIGNAL_PUSH_PULL,
+	.prescale_max = IPROC_PRESCALE_MAX,
+	.prescale_shift = IPROC_PRESCALE_SHIFT,
+	.prescale_ch_ascending = true,
+	.duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX,
+	.duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN,
+	.period_count_max = IPROC_PERIOD_COUNT_MAX,
+	.period_count_min = IPROC_PERIOD_COUNT_MIN,
+	.smooth_output_support = false,
+};
+
 static const struct of_device_id bcm_kona_pwmc_dt[] = {
-	{ .compatible = "brcm,kona-pwm" },
+	{ .compatible = "brcm,kona-pwm", .data = &kona_pwmc_reg_data},
+	{ .compatible = "brcm,iproc-pwm", .data = &iproc_pwmc_reg_data},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
-- 
2.1.0

Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller

[Thierry Reding]

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

On Tue, Mar 29, 2016 at 10:22:29AM -0400, Yendapally Reddy Dhananjaya Reddy wrote:
> Update the kona driver to support Broadcom iproc pwm controller
> 
> Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
> ---
>  drivers/pwm/Kconfig        |   6 +-
>  drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 163 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index c182efc..e45ea33 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB
>  
>  config PWM_BCM_KONA
>  	tristate "Kona PWM support"
> -	depends on ARCH_BCM_MOBILE
> +	depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC
> +	default ARCH_BCM_IPROC

Why the default? Typically you'd enable this in one or more of the
default configurations. default ARCH_* is really only useful if the
driver is essential. PWM doesn't usually fall into this category.

> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index c634183..ef152e3a 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -19,6 +19,7 @@
>  #include <linux/math64.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  #include <linux/slab.h>
> @@ -47,30 +48,90 @@
>  
>  #define PWM_CONTROL_OFFSET			(0x00000000)
>  #define PWM_CONTROL_SMOOTH_SHIFT(chan)		(24 + (chan))
> -#define PWM_CONTROL_TYPE_SHIFT(chan)		(16 + (chan))
> +#define PWM_CONTROL_TYPE_SHIFT(shift, chan)	(shift + chan)

You need to put the parameters into parentheses to avoid expansion from
potentially messing up the expression.

>  #define PWM_CONTROL_POLARITY_SHIFT(chan)	(8 + (chan))
>  #define PWM_CONTROL_TRIGGER_SHIFT(chan)		(chan)
>  
>  #define PRESCALE_OFFSET				(0x00000004)
> -#define PRESCALE_SHIFT(chan)			((chan) << 2)
> -#define PRESCALE_MASK(chan)			(0x7 << PRESCALE_SHIFT(chan))
> +#define PRESCALE_SHIFT				(0x00000004)
> +#define PRESCALE_MASK				(0x00000007)

Hmm... this looks odd. Why are you dropping the chan parameter here?

>  #define PRESCALE_MIN				(0x00000000)
>  #define PRESCALE_MAX				(0x00000007)
>  
> -#define PERIOD_COUNT_OFFSET(chan)		(0x00000008 + ((chan) << 3))
> +#define PERIOD_COUNT_OFFSET(offset, chan)	(offset + (chan << 3))

Need parentheses here as well.

>  #define PERIOD_COUNT_MIN			(0x00000002)
>  #define PERIOD_COUNT_MAX			(0x00ffffff)
> +#define KONA_PERIOD_COUNT_OFFSET		(0x00000008)
>  
> -#define DUTY_CYCLE_HIGH_OFFSET(chan)		(0x0000000c + ((chan) << 3))
> +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan)	(offset + (chan << 3))
>  #define DUTY_CYCLE_HIGH_MIN			(0x00000000)
>  #define DUTY_CYCLE_HIGH_MAX			(0x00ffffff)
> +#define KONA_DUTY_CYCLE_HIGH_OFFSET		(0x0000000c)
> +
> +#define PWM_CHANNEL_CNT				(0x00000006)
> +#define SIGNAL_PUSH_PULL			(0x00000001)
> +#define PWMOUT_TYPE_SHIFT			(0x00000010)
> +
> +#define IPROC_PRESCALE_OFFSET			(0x00000024)
> +#define IPROC_PRESCALE_SHIFT			(0x00000006)
> +#define IPROC_PRESCALE_MAX			(0x0000003f)
> +
> +#define IPROC_PERIOD_COUNT_OFFSET		(0x00000004)
> +#define IPROC_PERIOD_COUNT_MIN			(0x00000002)
> +#define IPROC_PERIOD_COUNT_MAX			(0x0000ffff)
> +
> +#define IPROC_DUTY_CYCLE_HIGH_OFFSET		(0x00000008)
> +#define IPROC_DUTY_CYCLE_HIGH_MIN		(0x00000000)
> +#define IPROC_DUTY_CYCLE_HIGH_MAX		(0x0000ffff)
> +
> +#define IPROC_PWM_CHANNEL_CNT			(0x00000004)
> +#define IPROC_SIGNAL_PUSH_PULL			(0x00000000)
> +#define IPROC_PWMOUT_TYPE_SHIFT			(0x0000000f)
> +
> +/*
> + * pwm controller reg structure
> + *
> + * @prescale_offset: prescale register offset
> + * @period_offset: period register offset
> + * @duty_offset: duty register offset
> + * @no_of_channels: number of channels
> + * @out_type_shift: out type shift in the register
> + * @signal_type: push-pull or open drain
> + * @prescale_max: prescale max
> + * @prescale_shift: prescale shift in register
> + * @prescale_ch_ascending: prescale ch order in prescale register
> + * @duty_cycle_max: value of max duty cycle
> + * @duty_cycle_min: value of min duty cycle
> + * @period_count_max: max period count val
> + * @period_count_min: min period count val
> + * @smooth_output_support: pwm smooth output support
> + */
> +struct kona_pwmc_reg {
> +	u32 prescale_offset;
> +	u32 period_offset;
> +	u32 duty_offset;
> +	u32 no_of_channels;
> +	u32 out_type_shift;
> +	u32 signal_type;
> +	u32 prescale_max;
> +	u32 prescale_shift;
> +	bool prescale_ch_ascending;
> +	u32 duty_cycle_max;
> +	u32 duty_cycle_min;
> +	u32 period_count_max;
> +	u32 period_count_min;
> +	bool smooth_output_support;
> +};

This is rather tedious. It looks to me like this isn't very similar to
the existing driver. Register offsets move around, bitfield positions
change, feature set is different. Might be better off turning this into
a separate driver after all.

> +static const struct kona_pwmc_reg kona_pwmc_reg_data = {
> +	.prescale_offset = PRESCALE_OFFSET,
> +	.period_offset = KONA_PERIOD_COUNT_OFFSET,
> +	.duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET,
> +	.no_of_channels = PWM_CHANNEL_CNT,
> +	.out_type_shift = PWMOUT_TYPE_SHIFT,
> +	.signal_type = SIGNAL_PUSH_PULL,
> +	.prescale_max = PRESCALE_MAX,
> +	.prescale_shift = PRESCALE_SHIFT,
> +	.prescale_ch_ascending = false,
> +	.duty_cycle_max = DUTY_CYCLE_HIGH_MAX,
> +	.duty_cycle_min = DUTY_CYCLE_HIGH_MIN,
> +	.period_count_max = PERIOD_COUNT_MAX,
> +	.period_count_min = PERIOD_COUNT_MIN,
> +	.smooth_output_support = true,
> +};
> +
> +static const struct kona_pwmc_reg iproc_pwmc_reg_data = {
> +	.prescale_offset = IPROC_PRESCALE_OFFSET,
> +	.period_offset = IPROC_PERIOD_COUNT_OFFSET,
> +	.duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET,
> +	.no_of_channels = IPROC_PWM_CHANNEL_CNT,
> +	.out_type_shift = IPROC_PWMOUT_TYPE_SHIFT,
> +	.signal_type = IPROC_SIGNAL_PUSH_PULL,
> +	.prescale_max = IPROC_PRESCALE_MAX,
> +	.prescale_shift = IPROC_PRESCALE_SHIFT,
> +	.prescale_ch_ascending = true,
> +	.duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX,
> +	.duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN,
> +	.period_count_max = IPROC_PERIOD_COUNT_MAX,
> +	.period_count_min = IPROC_PERIOD_COUNT_MIN,
> +	.smooth_output_support = false,
> +};

This looks like you could possible support a lot more hardware with this
driver because it's now almost completely parameterized.

I don't see much sense in keeping this in the same driver and I think
it'd be better to write a new one from scratch, even if that means
slight duplication.

Or you'll have to make a very compelling argument as to why this is the
better option.

Thierry

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

Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller

[Boris Brezillon]

Hi,

On Tue, 10 May 2016 16:40:24 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Mar 29, 2016 at 10:22:29AM -0400, Yendapally Reddy Dhananjaya Reddy wrote:
> > Update the kona driver to support Broadcom iproc pwm controller
> > 
> > Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
> > ---
> >  drivers/pwm/Kconfig        |   6 +-
> >  drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 163 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index c182efc..e45ea33 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB
> >  
> >  config PWM_BCM_KONA
> >  	tristate "Kona PWM support"
> > -	depends on ARCH_BCM_MOBILE
> > +	depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC
> > +	default ARCH_BCM_IPROC  
> 
> Why the default? Typically you'd enable this in one or more of the
> default configurations. default ARCH_* is really only useful if the
> driver is essential. PWM doesn't usually fall into this category.
> 
> > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> > index c634183..ef152e3a 100644
> > --- a/drivers/pwm/pwm-bcm-kona.c
> > +++ b/drivers/pwm/pwm-bcm-kona.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/math64.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pwm.h>
> >  #include <linux/slab.h>
> > @@ -47,30 +48,90 @@
> >  
> >  #define PWM_CONTROL_OFFSET			(0x00000000)
> >  #define PWM_CONTROL_SMOOTH_SHIFT(chan)		(24 + (chan))
> > -#define PWM_CONTROL_TYPE_SHIFT(chan)		(16 + (chan))
> > +#define PWM_CONTROL_TYPE_SHIFT(shift, chan)	(shift + chan)  
> 
> You need to put the parameters into parentheses to avoid expansion from
> potentially messing up the expression.
> 
> >  #define PWM_CONTROL_POLARITY_SHIFT(chan)	(8 + (chan))
> >  #define PWM_CONTROL_TRIGGER_SHIFT(chan)		(chan)
> >  
> >  #define PRESCALE_OFFSET				(0x00000004)
> > -#define PRESCALE_SHIFT(chan)			((chan) << 2)
> > -#define PRESCALE_MASK(chan)			(0x7 << PRESCALE_SHIFT(chan))
> > +#define PRESCALE_SHIFT				(0x00000004)
> > +#define PRESCALE_MASK				(0x00000007)  
> 
> Hmm... this looks odd. Why are you dropping the chan parameter here?
> 
> >  #define PRESCALE_MIN				(0x00000000)
> >  #define PRESCALE_MAX				(0x00000007)
> >  
> > -#define PERIOD_COUNT_OFFSET(chan)		(0x00000008 + ((chan) << 3))
> > +#define PERIOD_COUNT_OFFSET(offset, chan)	(offset + (chan << 3))  
> 
> Need parentheses here as well.
> 
> >  #define PERIOD_COUNT_MIN			(0x00000002)
> >  #define PERIOD_COUNT_MAX			(0x00ffffff)
> > +#define KONA_PERIOD_COUNT_OFFSET		(0x00000008)
> >  
> > -#define DUTY_CYCLE_HIGH_OFFSET(chan)		(0x0000000c + ((chan) << 3))
> > +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan)	(offset + (chan << 3))
> >  #define DUTY_CYCLE_HIGH_MIN			(0x00000000)
> >  #define DUTY_CYCLE_HIGH_MAX			(0x00ffffff)
> > +#define KONA_DUTY_CYCLE_HIGH_OFFSET		(0x0000000c)
> > +
> > +#define PWM_CHANNEL_CNT				(0x00000006)
> > +#define SIGNAL_PUSH_PULL			(0x00000001)
> > +#define PWMOUT_TYPE_SHIFT			(0x00000010)
> > +
> > +#define IPROC_PRESCALE_OFFSET			(0x00000024)
> > +#define IPROC_PRESCALE_SHIFT			(0x00000006)
> > +#define IPROC_PRESCALE_MAX			(0x0000003f)
> > +
> > +#define IPROC_PERIOD_COUNT_OFFSET		(0x00000004)
> > +#define IPROC_PERIOD_COUNT_MIN			(0x00000002)
> > +#define IPROC_PERIOD_COUNT_MAX			(0x0000ffff)
> > +
> > +#define IPROC_DUTY_CYCLE_HIGH_OFFSET		(0x00000008)
> > +#define IPROC_DUTY_CYCLE_HIGH_MIN		(0x00000000)
> > +#define IPROC_DUTY_CYCLE_HIGH_MAX		(0x0000ffff)
> > +
> > +#define IPROC_PWM_CHANNEL_CNT			(0x00000004)
> > +#define IPROC_SIGNAL_PUSH_PULL			(0x00000000)
> > +#define IPROC_PWMOUT_TYPE_SHIFT			(0x0000000f)
> > +
> > +/*
> > + * pwm controller reg structure
> > + *
> > + * @prescale_offset: prescale register offset
> > + * @period_offset: period register offset
> > + * @duty_offset: duty register offset
> > + * @no_of_channels: number of channels
> > + * @out_type_shift: out type shift in the register
> > + * @signal_type: push-pull or open drain
> > + * @prescale_max: prescale max
> > + * @prescale_shift: prescale shift in register
> > + * @prescale_ch_ascending: prescale ch order in prescale register
> > + * @duty_cycle_max: value of max duty cycle
> > + * @duty_cycle_min: value of min duty cycle
> > + * @period_count_max: max period count val
> > + * @period_count_min: min period count val
> > + * @smooth_output_support: pwm smooth output support
> > + */
> > +struct kona_pwmc_reg {
> > +	u32 prescale_offset;
> > +	u32 period_offset;
> > +	u32 duty_offset;
> > +	u32 no_of_channels;
> > +	u32 out_type_shift;
> > +	u32 signal_type;
> > +	u32 prescale_max;
> > +	u32 prescale_shift;
> > +	bool prescale_ch_ascending;
> > +	u32 duty_cycle_max;
> > +	u32 duty_cycle_min;
> > +	u32 period_count_max;
> > +	u32 period_count_min;
> > +	bool smooth_output_support;
> > +};  
> 
> This is rather tedious. It looks to me like this isn't very similar to
> the existing driver. Register offsets move around, bitfield positions
> change, feature set is different. Might be better off turning this into
> a separate driver after all.
> 
> > +static const struct kona_pwmc_reg kona_pwmc_reg_data = {
> > +	.prescale_offset = PRESCALE_OFFSET,
> > +	.period_offset = KONA_PERIOD_COUNT_OFFSET,
> > +	.duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET,
> > +	.no_of_channels = PWM_CHANNEL_CNT,
> > +	.out_type_shift = PWMOUT_TYPE_SHIFT,
> > +	.signal_type = SIGNAL_PUSH_PULL,
> > +	.prescale_max = PRESCALE_MAX,
> > +	.prescale_shift = PRESCALE_SHIFT,
> > +	.prescale_ch_ascending = false,
> > +	.duty_cycle_max = DUTY_CYCLE_HIGH_MAX,
> > +	.duty_cycle_min = DUTY_CYCLE_HIGH_MIN,
> > +	.period_count_max = PERIOD_COUNT_MAX,
> > +	.period_count_min = PERIOD_COUNT_MIN,
> > +	.smooth_output_support = true,
> > +};
> > +
> > +static const struct kona_pwmc_reg iproc_pwmc_reg_data = {
> > +	.prescale_offset = IPROC_PRESCALE_OFFSET,
> > +	.period_offset = IPROC_PERIOD_COUNT_OFFSET,
> > +	.duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET,
> > +	.no_of_channels = IPROC_PWM_CHANNEL_CNT,
> > +	.out_type_shift = IPROC_PWMOUT_TYPE_SHIFT,
> > +	.signal_type = IPROC_SIGNAL_PUSH_PULL,
> > +	.prescale_max = IPROC_PRESCALE_MAX,
> > +	.prescale_shift = IPROC_PRESCALE_SHIFT,
> > +	.prescale_ch_ascending = true,
> > +	.duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX,
> > +	.duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN,
> > +	.period_count_max = IPROC_PERIOD_COUNT_MAX,
> > +	.period_count_min = IPROC_PERIOD_COUNT_MIN,
> > +	.smooth_output_support = false,
> > +};  
> 
> This looks like you could possible support a lot more hardware with this
> driver because it's now almost completely parameterized.
> 
> I don't see much sense in keeping this in the same driver and I think
> it'd be better to write a new one from scratch, even if that means
> slight duplication.
> 
> Or you'll have to make a very compelling argument as to why this is the
> better option.

Sorry to enter the discussion just now (Brian CCed me in an answer he
made to v4 of this series).

Honestly, I'd have the exact opposite argument: if you have a look at
newer versions of this patch, you'll see that more than 90% of the code
is duplicated, and re-using the same logic with different field
positions is quite common in other drivers. Actually the counter
argument to your suggestion are bug fixes: when you find a bug, you'll
have to remember fixing it for all implementations. Having a common
code base has IMO more pros than cons.

BTW, if you switch to regmap, you have this field-position
customization for free (see reg_field).

Regards,

Boris

Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller

[Thierry Reding]

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

On Thu, Jun 23, 2016 at 10:32:52PM +0200, Boris Brezillon wrote:
> Hi,
> 
> On Tue, 10 May 2016 16:40:24 +0200
> Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On Tue, Mar 29, 2016 at 10:22:29AM -0400, Yendapally Reddy Dhananjaya Reddy wrote:
> > > Update the kona driver to support Broadcom iproc pwm controller
> > > 
> > > Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
> > > ---
> > >  drivers/pwm/Kconfig        |   6 +-
> > >  drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------
> > >  2 files changed, 163 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index c182efc..e45ea33 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB
> > >  
> > >  config PWM_BCM_KONA
> > >  	tristate "Kona PWM support"
> > > -	depends on ARCH_BCM_MOBILE
> > > +	depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC
> > > +	default ARCH_BCM_IPROC  
> > 
> > Why the default? Typically you'd enable this in one or more of the
> > default configurations. default ARCH_* is really only useful if the
> > driver is essential. PWM doesn't usually fall into this category.
> > 
> > > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> > > index c634183..ef152e3a 100644
> > > --- a/drivers/pwm/pwm-bcm-kona.c
> > > +++ b/drivers/pwm/pwm-bcm-kona.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/math64.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > > +#include <linux/of_device.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pwm.h>
> > >  #include <linux/slab.h>
> > > @@ -47,30 +48,90 @@
> > >  
> > >  #define PWM_CONTROL_OFFSET			(0x00000000)
> > >  #define PWM_CONTROL_SMOOTH_SHIFT(chan)		(24 + (chan))
> > > -#define PWM_CONTROL_TYPE_SHIFT(chan)		(16 + (chan))
> > > +#define PWM_CONTROL_TYPE_SHIFT(shift, chan)	(shift + chan)  
> > 
> > You need to put the parameters into parentheses to avoid expansion from
> > potentially messing up the expression.
> > 
> > >  #define PWM_CONTROL_POLARITY_SHIFT(chan)	(8 + (chan))
> > >  #define PWM_CONTROL_TRIGGER_SHIFT(chan)		(chan)
> > >  
> > >  #define PRESCALE_OFFSET				(0x00000004)
> > > -#define PRESCALE_SHIFT(chan)			((chan) << 2)
> > > -#define PRESCALE_MASK(chan)			(0x7 << PRESCALE_SHIFT(chan))
> > > +#define PRESCALE_SHIFT				(0x00000004)
> > > +#define PRESCALE_MASK				(0x00000007)  
> > 
> > Hmm... this looks odd. Why are you dropping the chan parameter here?
> > 
> > >  #define PRESCALE_MIN				(0x00000000)
> > >  #define PRESCALE_MAX				(0x00000007)
> > >  
> > > -#define PERIOD_COUNT_OFFSET(chan)		(0x00000008 + ((chan) << 3))
> > > +#define PERIOD_COUNT_OFFSET(offset, chan)	(offset + (chan << 3))  
> > 
> > Need parentheses here as well.
> > 
> > >  #define PERIOD_COUNT_MIN			(0x00000002)
> > >  #define PERIOD_COUNT_MAX			(0x00ffffff)
> > > +#define KONA_PERIOD_COUNT_OFFSET		(0x00000008)
> > >  
> > > -#define DUTY_CYCLE_HIGH_OFFSET(chan)		(0x0000000c + ((chan) << 3))
> > > +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan)	(offset + (chan << 3))
> > >  #define DUTY_CYCLE_HIGH_MIN			(0x00000000)
> > >  #define DUTY_CYCLE_HIGH_MAX			(0x00ffffff)
> > > +#define KONA_DUTY_CYCLE_HIGH_OFFSET		(0x0000000c)
> > > +
> > > +#define PWM_CHANNEL_CNT				(0x00000006)
> > > +#define SIGNAL_PUSH_PULL			(0x00000001)
> > > +#define PWMOUT_TYPE_SHIFT			(0x00000010)
> > > +
> > > +#define IPROC_PRESCALE_OFFSET			(0x00000024)
> > > +#define IPROC_PRESCALE_SHIFT			(0x00000006)
> > > +#define IPROC_PRESCALE_MAX			(0x0000003f)
> > > +
> > > +#define IPROC_PERIOD_COUNT_OFFSET		(0x00000004)
> > > +#define IPROC_PERIOD_COUNT_MIN			(0x00000002)
> > > +#define IPROC_PERIOD_COUNT_MAX			(0x0000ffff)
> > > +
> > > +#define IPROC_DUTY_CYCLE_HIGH_OFFSET		(0x00000008)
> > > +#define IPROC_DUTY_CYCLE_HIGH_MIN		(0x00000000)
> > > +#define IPROC_DUTY_CYCLE_HIGH_MAX		(0x0000ffff)
> > > +
> > > +#define IPROC_PWM_CHANNEL_CNT			(0x00000004)
> > > +#define IPROC_SIGNAL_PUSH_PULL			(0x00000000)
> > > +#define IPROC_PWMOUT_TYPE_SHIFT			(0x0000000f)
> > > +
> > > +/*
> > > + * pwm controller reg structure
> > > + *
> > > + * @prescale_offset: prescale register offset
> > > + * @period_offset: period register offset
> > > + * @duty_offset: duty register offset
> > > + * @no_of_channels: number of channels
> > > + * @out_type_shift: out type shift in the register
> > > + * @signal_type: push-pull or open drain
> > > + * @prescale_max: prescale max
> > > + * @prescale_shift: prescale shift in register
> > > + * @prescale_ch_ascending: prescale ch order in prescale register
> > > + * @duty_cycle_max: value of max duty cycle
> > > + * @duty_cycle_min: value of min duty cycle
> > > + * @period_count_max: max period count val
> > > + * @period_count_min: min period count val
> > > + * @smooth_output_support: pwm smooth output support
> > > + */
> > > +struct kona_pwmc_reg {
> > > +	u32 prescale_offset;
> > > +	u32 period_offset;
> > > +	u32 duty_offset;
> > > +	u32 no_of_channels;
> > > +	u32 out_type_shift;
> > > +	u32 signal_type;
> > > +	u32 prescale_max;
> > > +	u32 prescale_shift;
> > > +	bool prescale_ch_ascending;
> > > +	u32 duty_cycle_max;
> > > +	u32 duty_cycle_min;
> > > +	u32 period_count_max;
> > > +	u32 period_count_min;
> > > +	bool smooth_output_support;
> > > +};  
> > 
> > This is rather tedious. It looks to me like this isn't very similar to
> > the existing driver. Register offsets move around, bitfield positions
> > change, feature set is different. Might be better off turning this into
> > a separate driver after all.
> > 
> > > +static const struct kona_pwmc_reg kona_pwmc_reg_data = {
> > > +	.prescale_offset = PRESCALE_OFFSET,
> > > +	.period_offset = KONA_PERIOD_COUNT_OFFSET,
> > > +	.duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET,
> > > +	.no_of_channels = PWM_CHANNEL_CNT,
> > > +	.out_type_shift = PWMOUT_TYPE_SHIFT,
> > > +	.signal_type = SIGNAL_PUSH_PULL,
> > > +	.prescale_max = PRESCALE_MAX,
> > > +	.prescale_shift = PRESCALE_SHIFT,
> > > +	.prescale_ch_ascending = false,
> > > +	.duty_cycle_max = DUTY_CYCLE_HIGH_MAX,
> > > +	.duty_cycle_min = DUTY_CYCLE_HIGH_MIN,
> > > +	.period_count_max = PERIOD_COUNT_MAX,
> > > +	.period_count_min = PERIOD_COUNT_MIN,
> > > +	.smooth_output_support = true,
> > > +};
> > > +
> > > +static const struct kona_pwmc_reg iproc_pwmc_reg_data = {
> > > +	.prescale_offset = IPROC_PRESCALE_OFFSET,
> > > +	.period_offset = IPROC_PERIOD_COUNT_OFFSET,
> > > +	.duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET,
> > > +	.no_of_channels = IPROC_PWM_CHANNEL_CNT,
> > > +	.out_type_shift = IPROC_PWMOUT_TYPE_SHIFT,
> > > +	.signal_type = IPROC_SIGNAL_PUSH_PULL,
> > > +	.prescale_max = IPROC_PRESCALE_MAX,
> > > +	.prescale_shift = IPROC_PRESCALE_SHIFT,
> > > +	.prescale_ch_ascending = true,
> > > +	.duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX,
> > > +	.duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN,
> > > +	.period_count_max = IPROC_PERIOD_COUNT_MAX,
> > > +	.period_count_min = IPROC_PERIOD_COUNT_MIN,
> > > +	.smooth_output_support = false,
> > > +};  
> > 
> > This looks like you could possible support a lot more hardware with this
> > driver because it's now almost completely parameterized.
> > 
> > I don't see much sense in keeping this in the same driver and I think
> > it'd be better to write a new one from scratch, even if that means
> > slight duplication.
> > 
> > Or you'll have to make a very compelling argument as to why this is the
> > better option.
> 
> Sorry to enter the discussion just now (Brian CCed me in an answer he
> made to v4 of this series).
> 
> Honestly, I'd have the exact opposite argument: if you have a look at
> newer versions of this patch, you'll see that more than 90% of the code
> is duplicated, and re-using the same logic with different field
> positions is quite common in other drivers. Actually the counter
> argument to your suggestion are bug fixes: when you find a bug, you'll
> have to remember fixing it for all implementations. Having a common
> code base has IMO more pros than cons.
> 
> BTW, if you switch to regmap, you have this field-position
> customization for free (see reg_field).

My point was that with the above structures you have an almost
completely parameterized driver. There's probably a number of other PWM
controllers that you could support using those structure, or by adding
to them. The question then becomes: where do we draw the line? When is
a new driver warranted, and when is it not.

Your argument regarding bug fixes has some weight, but these drivers are
really trivial, so bug fixes will be equally trivial to port between
them.

There's also the issue about errata and quirks that may apply to one but
not all supported versions. The outcome of supporting all those
combinations can be completely unmaintainable, so I am willing to take a
bit of duplication if it means that in the end it will make things
easier for me.

Thierry

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

Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller

[Boris Brezillon]

Hi Thierry,

This is my last answer to this thread, after that I'll stop bothering
you: after all, you're the PWM subsystem maintainer, and I have nothing
to say about how you want to maintain your subsytem ;-).

On Tue, 28 Jun 2016 14:52:23 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Thu, Jun 23, 2016 at 10:32:52PM +0200, Boris Brezillon wrote:
> > Hi,
> > 
> > On Tue, 10 May 2016 16:40:24 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >   
> > > On Tue, Mar 29, 2016 at 10:22:29AM -0400, Yendapally Reddy Dhananjaya Reddy wrote:  
> > > > Update the kona driver to support Broadcom iproc pwm controller
> > > > 
> > > > Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
> > > > ---
> > > >  drivers/pwm/Kconfig        |   6 +-
> > > >  drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------
> > > >  2 files changed, 163 insertions(+), 26 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > index c182efc..e45ea33 100644
> > > > --- a/drivers/pwm/Kconfig
> > > > +++ b/drivers/pwm/Kconfig
> > > > @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB
> > > >  
> > > >  config PWM_BCM_KONA
> > > >  	tristate "Kona PWM support"
> > > > -	depends on ARCH_BCM_MOBILE
> > > > +	depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC
> > > > +	default ARCH_BCM_IPROC    
> > > 
> > > Why the default? Typically you'd enable this in one or more of the
> > > default configurations. default ARCH_* is really only useful if the
> > > driver is essential. PWM doesn't usually fall into this category.
> > >   
> > > > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> > > > index c634183..ef152e3a 100644
> > > > --- a/drivers/pwm/pwm-bcm-kona.c
> > > > +++ b/drivers/pwm/pwm-bcm-kona.c
> > > > @@ -19,6 +19,7 @@
> > > >  #include <linux/math64.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/pwm.h>
> > > >  #include <linux/slab.h>
> > > > @@ -47,30 +48,90 @@
> > > >  
> > > >  #define PWM_CONTROL_OFFSET			(0x00000000)
> > > >  #define PWM_CONTROL_SMOOTH_SHIFT(chan)		(24 + (chan))
> > > > -#define PWM_CONTROL_TYPE_SHIFT(chan)		(16 + (chan))
> > > > +#define PWM_CONTROL_TYPE_SHIFT(shift, chan)	(shift + chan)    
> > > 
> > > You need to put the parameters into parentheses to avoid expansion from
> > > potentially messing up the expression.
> > >   
> > > >  #define PWM_CONTROL_POLARITY_SHIFT(chan)	(8 + (chan))
> > > >  #define PWM_CONTROL_TRIGGER_SHIFT(chan)		(chan)
> > > >  
> > > >  #define PRESCALE_OFFSET				(0x00000004)
> > > > -#define PRESCALE_SHIFT(chan)			((chan) << 2)
> > > > -#define PRESCALE_MASK(chan)			(0x7 << PRESCALE_SHIFT(chan))
> > > > +#define PRESCALE_SHIFT				(0x00000004)
> > > > +#define PRESCALE_MASK				(0x00000007)    
> > > 
> > > Hmm... this looks odd. Why are you dropping the chan parameter here?
> > >   
> > > >  #define PRESCALE_MIN				(0x00000000)
> > > >  #define PRESCALE_MAX				(0x00000007)
> > > >  
> > > > -#define PERIOD_COUNT_OFFSET(chan)		(0x00000008 + ((chan) << 3))
> > > > +#define PERIOD_COUNT_OFFSET(offset, chan)	(offset + (chan << 3))    
> > > 
> > > Need parentheses here as well.
> > >   
> > > >  #define PERIOD_COUNT_MIN			(0x00000002)
> > > >  #define PERIOD_COUNT_MAX			(0x00ffffff)
> > > > +#define KONA_PERIOD_COUNT_OFFSET		(0x00000008)
> > > >  
> > > > -#define DUTY_CYCLE_HIGH_OFFSET(chan)		(0x0000000c + ((chan) << 3))
> > > > +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan)	(offset + (chan << 3))
> > > >  #define DUTY_CYCLE_HIGH_MIN			(0x00000000)
> > > >  #define DUTY_CYCLE_HIGH_MAX			(0x00ffffff)
> > > > +#define KONA_DUTY_CYCLE_HIGH_OFFSET		(0x0000000c)
> > > > +
> > > > +#define PWM_CHANNEL_CNT				(0x00000006)
> > > > +#define SIGNAL_PUSH_PULL			(0x00000001)
> > > > +#define PWMOUT_TYPE_SHIFT			(0x00000010)
> > > > +
> > > > +#define IPROC_PRESCALE_OFFSET			(0x00000024)
> > > > +#define IPROC_PRESCALE_SHIFT			(0x00000006)
> > > > +#define IPROC_PRESCALE_MAX			(0x0000003f)
> > > > +
> > > > +#define IPROC_PERIOD_COUNT_OFFSET		(0x00000004)
> > > > +#define IPROC_PERIOD_COUNT_MIN			(0x00000002)
> > > > +#define IPROC_PERIOD_COUNT_MAX			(0x0000ffff)
> > > > +
> > > > +#define IPROC_DUTY_CYCLE_HIGH_OFFSET		(0x00000008)
> > > > +#define IPROC_DUTY_CYCLE_HIGH_MIN		(0x00000000)
> > > > +#define IPROC_DUTY_CYCLE_HIGH_MAX		(0x0000ffff)
> > > > +
> > > > +#define IPROC_PWM_CHANNEL_CNT			(0x00000004)
> > > > +#define IPROC_SIGNAL_PUSH_PULL			(0x00000000)
> > > > +#define IPROC_PWMOUT_TYPE_SHIFT			(0x0000000f)
> > > > +
> > > > +/*
> > > > + * pwm controller reg structure
> > > > + *
> > > > + * @prescale_offset: prescale register offset
> > > > + * @period_offset: period register offset
> > > > + * @duty_offset: duty register offset
> > > > + * @no_of_channels: number of channels
> > > > + * @out_type_shift: out type shift in the register
> > > > + * @signal_type: push-pull or open drain
> > > > + * @prescale_max: prescale max
> > > > + * @prescale_shift: prescale shift in register
> > > > + * @prescale_ch_ascending: prescale ch order in prescale register
> > > > + * @duty_cycle_max: value of max duty cycle
> > > > + * @duty_cycle_min: value of min duty cycle
> > > > + * @period_count_max: max period count val
> > > > + * @period_count_min: min period count val
> > > > + * @smooth_output_support: pwm smooth output support
> > > > + */
> > > > +struct kona_pwmc_reg {
> > > > +	u32 prescale_offset;
> > > > +	u32 period_offset;
> > > > +	u32 duty_offset;
> > > > +	u32 no_of_channels;
> > > > +	u32 out_type_shift;
> > > > +	u32 signal_type;
> > > > +	u32 prescale_max;
> > > > +	u32 prescale_shift;
> > > > +	bool prescale_ch_ascending;
> > > > +	u32 duty_cycle_max;
> > > > +	u32 duty_cycle_min;
> > > > +	u32 period_count_max;
> > > > +	u32 period_count_min;
> > > > +	bool smooth_output_support;
> > > > +};    
> > > 
> > > This is rather tedious. It looks to me like this isn't very similar to
> > > the existing driver. Register offsets move around, bitfield positions
> > > change, feature set is different. Might be better off turning this into
> > > a separate driver after all.
> > >   
> > > > +static const struct kona_pwmc_reg kona_pwmc_reg_data = {
> > > > +	.prescale_offset = PRESCALE_OFFSET,
> > > > +	.period_offset = KONA_PERIOD_COUNT_OFFSET,
> > > > +	.duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET,
> > > > +	.no_of_channels = PWM_CHANNEL_CNT,
> > > > +	.out_type_shift = PWMOUT_TYPE_SHIFT,
> > > > +	.signal_type = SIGNAL_PUSH_PULL,
> > > > +	.prescale_max = PRESCALE_MAX,
> > > > +	.prescale_shift = PRESCALE_SHIFT,
> > > > +	.prescale_ch_ascending = false,
> > > > +	.duty_cycle_max = DUTY_CYCLE_HIGH_MAX,
> > > > +	.duty_cycle_min = DUTY_CYCLE_HIGH_MIN,
> > > > +	.period_count_max = PERIOD_COUNT_MAX,
> > > > +	.period_count_min = PERIOD_COUNT_MIN,
> > > > +	.smooth_output_support = true,
> > > > +};
> > > > +
> > > > +static const struct kona_pwmc_reg iproc_pwmc_reg_data = {
> > > > +	.prescale_offset = IPROC_PRESCALE_OFFSET,
> > > > +	.period_offset = IPROC_PERIOD_COUNT_OFFSET,
> > > > +	.duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET,
> > > > +	.no_of_channels = IPROC_PWM_CHANNEL_CNT,
> > > > +	.out_type_shift = IPROC_PWMOUT_TYPE_SHIFT,
> > > > +	.signal_type = IPROC_SIGNAL_PUSH_PULL,
> > > > +	.prescale_max = IPROC_PRESCALE_MAX,
> > > > +	.prescale_shift = IPROC_PRESCALE_SHIFT,
> > > > +	.prescale_ch_ascending = true,
> > > > +	.duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX,
> > > > +	.duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN,
> > > > +	.period_count_max = IPROC_PERIOD_COUNT_MAX,
> > > > +	.period_count_min = IPROC_PERIOD_COUNT_MIN,
> > > > +	.smooth_output_support = false,
> > > > +};    
> > > 
> > > This looks like you could possible support a lot more hardware with this
> > > driver because it's now almost completely parameterized.
> > > 
> > > I don't see much sense in keeping this in the same driver and I think
> > > it'd be better to write a new one from scratch, even if that means
> > > slight duplication.
> > > 
> > > Or you'll have to make a very compelling argument as to why this is the
> > > better option.  
> > 
> > Sorry to enter the discussion just now (Brian CCed me in an answer he
> > made to v4 of this series).
> > 
> > Honestly, I'd have the exact opposite argument: if you have a look at
> > newer versions of this patch, you'll see that more than 90% of the code
> > is duplicated, and re-using the same logic with different field
> > positions is quite common in other drivers. Actually the counter
> > argument to your suggestion are bug fixes: when you find a bug, you'll
> > have to remember fixing it for all implementations. Having a common
> > code base has IMO more pros than cons.
> > 
> > BTW, if you switch to regmap, you have this field-position
> > customization for free (see reg_field).  
> 
> My point was that with the above structures you have an almost
> completely parameterized driver. There's probably a number of other PWM
> controllers that you could support using those structure, or by adding
> to them. The question then becomes: where do we draw the line? When is
> a new driver warranted, and when is it not.

Well, there's a difference between trying to provide a completely
generic PWM driver and trying to factorize code for 2 almost identical
IPs (only the field position and width changes here).
The generic PWM driver approach is clearly an utopia because all PWM
IPs have their own requirements in term of clock source selection,
PWM configuration and sequencing constraints.

I agree that the decision to factorize common code is subjective, and
that the maintainer (you in this case) will always have the final word.

> 
> Your argument regarding bug fixes has some weight, but these drivers are
> really trivial, so bug fixes will be equally trivial to port between
> them.

Well, it's not only about bug fixes, but driver maintenance in general.
Let's take the recent atomic PWM rework: we're now recommending to
implement ->apply() instead of ->enable()/disable()/config(), and Brian
asked Yendapally to rework his implementation to follow this
recommendation. If we had chosen for the kona driver enhancement
instead of duplicating the code in a new driver we would have the
atomic update for the kona IP for free...

> 
> There's also the issue about errata and quirks that may apply to one but
> not all supported versions. The outcome of supporting all those
> combinations can be completely unmaintainable, so I am willing to take a
> bit of duplication if it means that in the end it will make things
> easier for me.

With only 2 revisions/versions supported, I don't expect to see a lot
of different errata, but that's true in general.

[PATCH RESEND 3/3] ARM: dts: NSP: Add PWM Support to DT

[Yendapally Reddy Dhananjaya Reddy]

Add PWM support to the device tree for the Broadcom Northstar Plus SoC.

Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
---
 arch/arm/boot/dts/bcm-nsp.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
index def9e78..f9c8341 100644
--- a/arch/arm/boot/dts/bcm-nsp.dtsi
+++ b/arch/arm/boot/dts/bcm-nsp.dtsi
@@ -206,6 +206,14 @@
 			brcm,nand-has-wp;
 		};
 
+		pwm: pwm@31000 {
+			compatible = "brcm,iproc-pwm";
+			reg = <0x31000 0x28>;
+			clocks = <&osc>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		ccbtimer0: timer@34000 {
 			compatible = "arm,sp804";
 			reg = <0x34000 0x1000>;
-- 
2.1.0

Re: [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's

[Yendapally Reddy Dhananjaya Reddy]

Hi Thierry,

  Can you please provide review comments for this patch set.

Thanks
Dhananjay

On Tue, Mar 29, 2016 at 7:52 PM, Yendapally Reddy Dhananjaya Reddy
<yendapally.reddy@broadcom.com> wrote:
> This patchset contains the pwm support for the Broadcom's iProc SoC's.
>
> The first patch provides the documentation details and the
> second patch contains the controller support details. The third patch
> contains the enable method for Northstar Plus SoC.
>
> This patch series has been tested on NSP bcm958625HR board with oscilloscope.
> The pwm is not enabled for this board as the pins are open and not used.
>
> This patch series is based on v4.6.0-rc1 and is available from github
> repo: https://github.com/Broadcom/cygnus-linux.git
> branch: iproc-pwm-v1
>
> This patch series was originally sent on 03/01/16.
> See https://lkml.org/lkml/2016/3/1/487
>
> Yendapally Reddy Dhananjaya Reddy (3):
>   Documentation: dt: Add Broadcom iproc pwm controller binding
>   pwm: kona: Add support for Broadcom iproc pwm controller
>   ARM: dts: NSP: Add PWM Support to DT
>
>  .../devicetree/bindings/pwm/brcm,kona-pwm.txt      |   2 +-
>  arch/arm/boot/dts/bcm-nsp.dtsi                     |   8 +
>  drivers/pwm/Kconfig                                |   6 +-
>  drivers/pwm/pwm-bcm-kona.c                         | 183 ++++++++++++++++++---
>  4 files changed, 172 insertions(+), 27 deletions(-)
>
> --
> 2.1.0
>