[PATCH 0/5] gpio: mvebu: Armada 8K/7K PWM support

[PATCH 0/5] gpio: mvebu: Armada 8K/7K PWM support

[Baruch Siach]

The gpio-mvebu driver supports the PWM functionality of the GPIO block for 
earlier Armada variants like XP, 370 and 38x. This series extends support to 
newer Armada variants that use CP11x and AP80x, like Armada 8K and 7K.

This series adds adds the 'pwm-offset' property to DT binding. 'pwm-offset' 
points to the base of A/B counter registers that determine the PWM period and 
duty cycle.

The existing PWM DT binding reflects an arbitrary decision to allocate the A 
counter to the first GPIO block, and B counter to the other one. In attempt to 
provide better future flexibility, the new 'pwm-offset' property always points 
to the base address of both A/B counters. The driver code still allocates the 
counters in the same way, but this might change in the future with no change to
the DT.

Tested AP806 and CP110 (both) on Armada 8040 based system.

Baruch Siach (5):
  gpio: mvebu: update Armada XP per-CPU comment
  gpio: mvebu: switch pwm duration registers to regmap
  gpio: mvebu: add pwm support for Armada 8K/7K
  arm64: dts: armada: add pwm offsets for ap/cp gpios
  dt-bindings: ap806: document gpio pwm-offset property

 .../arm/marvell/ap80x-system-controller.txt   |   8 +
 arch/arm64/boot/dts/marvell/armada-ap80x.dtsi |   3 +
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi |  10 ++
 drivers/gpio/gpio-mvebu.c                     | 165 +++++++++++-------
 4 files changed, 124 insertions(+), 62 deletions(-)

-- 
2.29.2

[PATCH 1/5] gpio: mvebu: update Armada XP per-CPU comment

[Baruch Siach]

Commit 2233bf7a92e ("gpio: mvebu: switch to regmap for register access")
introduced percpu_regs to replace percpu_membase. Update the comment to
match.

Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Fixes: 2233bf7a92e7 ("gpio: mvebu: switch to regmap for register access")
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 433e2c3f3fd5..bdc4d813a42e 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -78,8 +78,7 @@
 
 /*
  * The Armada XP has per-CPU registers for interrupt cause, interrupt
- * mask and interrupt level mask. Those are relative to the
- * percpu_membase.
+ * mask and interrupt level mask. Those are in percpu_regs range.
  */
 #define GPIO_EDGE_CAUSE_ARMADAXP_OFF(cpu) ((cpu) * 0x4)
 #define GPIO_EDGE_MASK_ARMADAXP_OFF(cpu)  (0x10 + (cpu) * 0x4)
-- 
2.29.2

Re: [PATCH 1/5] gpio: mvebu: update Armada XP per-CPU comment

[Andrew Lunn]

On Wed, Nov 18, 2020 at 12:30:42PM +0200, Baruch Siach wrote:
> Commit 2233bf7a92e ("gpio: mvebu: switch to regmap for register access")
> introduced percpu_regs to replace percpu_membase. Update the comment to
> match.
> 
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Fixes: 2233bf7a92e7 ("gpio: mvebu: switch to regmap for register access")
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH 2/5] gpio: mvebu: switch pwm duration registers to regmap

[Baruch Siach]

Commit 2233bf7a92e ("gpio: mvebu: switch to regmap for register access")
changed most readl/writel registers access calls to the regmap API in
preparation for Armada 7K/8K support. PWM duration registers were left using
readl/writel, as the driver does not support PWM for Armada 7K/8K.

Switch PWM duration registers to regmap as first step in adding Armada 7K/8K
PWM functionality support.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 68 +++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 32 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index bdc4d813a42e..946571e70928 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -92,7 +92,7 @@
 #define MVEBU_MAX_GPIO_PER_BANK		32
 
 struct mvebu_pwm {
-	void __iomem		*membase;
+	struct regmap		*regs;
 	unsigned long		 clk_rate;
 	struct gpio_desc	*gpiod;
 	struct pwm_chip		 chip;
@@ -278,17 +278,17 @@ mvebu_gpio_write_level_mask(struct mvebu_gpio_chip *mvchip, u32 val)
 }
 
 /*
- * Functions returning addresses of individual registers for a given
+ * Functions returning offsets of individual registers for a given
  * PWM controller.
  */
-static void __iomem *mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
+static unsigned int mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
 {
-	return mvpwm->membase + PWM_BLINK_ON_DURATION_OFF;
+	return PWM_BLINK_ON_DURATION_OFF;
 }
 
-static void __iomem *mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
+static unsigned int mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
 {
-	return mvpwm->membase + PWM_BLINK_OFF_DURATION_OFF;
+	return PWM_BLINK_OFF_DURATION_OFF;
 }
 
 /*
@@ -599,6 +599,13 @@ static void mvebu_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static const struct regmap_config mvebu_gpio_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+};
+
 /*
  * Functions implementing the pwm_chip methods
  */
@@ -659,9 +666,8 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 
 	spin_lock_irqsave(&mvpwm->lock, flags);
 
-	val = (unsigned long long)
-		readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm));
-	val *= NSEC_PER_SEC;
+	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm), &u);
+	val = (unsigned long long) u * NSEC_PER_SEC;
 	do_div(val, mvpwm->clk_rate);
 	if (val > UINT_MAX)
 		state->duty_cycle = UINT_MAX;
@@ -670,9 +676,8 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 	else
 		state->duty_cycle = 1;
 
-	val = (unsigned long long)
-		readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm));
-	val *= NSEC_PER_SEC;
+	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
+	val = (unsigned long long) u * NSEC_PER_SEC;
 	do_div(val, mvpwm->clk_rate);
 	if (val < state->duty_cycle) {
 		state->period = 1;
@@ -725,8 +730,8 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	spin_lock_irqsave(&mvpwm->lock, flags);
 
-	writel_relaxed(on, mvebu_pwmreg_blink_on_duration(mvpwm));
-	writel_relaxed(off, mvebu_pwmreg_blink_off_duration(mvpwm));
+	regmap_write(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm), on);
+	regmap_write(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), off);
 	if (state->enabled)
 		mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 1);
 	else
@@ -751,10 +756,10 @@ static void __maybe_unused mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
 
 	regmap_read(mvchip->regs, GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset,
 		    &mvpwm->blink_select);
-	mvpwm->blink_on_duration =
-		readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm));
-	mvpwm->blink_off_duration =
-		readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm));
+	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm),
+		    &mvpwm->blink_on_duration);
+	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm),
+		    &mvpwm->blink_off_duration);
 }
 
 static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
@@ -763,10 +768,10 @@ static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
 
 	regmap_write(mvchip->regs, GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset,
 		     mvpwm->blink_select);
-	writel_relaxed(mvpwm->blink_on_duration,
-		       mvebu_pwmreg_blink_on_duration(mvpwm));
-	writel_relaxed(mvpwm->blink_off_duration,
-		       mvebu_pwmreg_blink_off_duration(mvpwm));
+	regmap_write(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm),
+		     mvpwm->blink_on_duration);
+	regmap_write(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm),
+		     mvpwm->blink_off_duration);
 }
 
 static int mvebu_pwm_probe(struct platform_device *pdev,
@@ -775,6 +780,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 {
 	struct device *dev = &pdev->dev;
 	struct mvebu_pwm *mvpwm;
+	void __iomem *base;
 	u32 set;
 
 	if (!of_device_is_compatible(mvchip->chip.of_node,
@@ -812,9 +818,14 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 	mvchip->mvpwm = mvpwm;
 	mvpwm->mvchip = mvchip;
 
-	mvpwm->membase = devm_platform_ioremap_resource_byname(pdev, "pwm");
-	if (IS_ERR(mvpwm->membase))
-		return PTR_ERR(mvpwm->membase);
+	base = devm_platform_ioremap_resource_byname(pdev, "pwm");
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	mvpwm->regs = devm_regmap_init_mmio(&pdev->dev, base,
+					    &mvebu_gpio_regmap_config);
+	if (IS_ERR(mvpwm->regs))
+		return PTR_ERR(mvpwm->regs);
 
 	mvpwm->clk_rate = clk_get_rate(mvchip->clk);
 	if (!mvpwm->clk_rate) {
@@ -1021,13 +1032,6 @@ static int mvebu_gpio_resume(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct regmap_config mvebu_gpio_regmap_config = {
-	.reg_bits = 32,
-	.reg_stride = 4,
-	.val_bits = 32,
-	.fast_io = true,
-};
-
 static int mvebu_gpio_probe_raw(struct platform_device *pdev,
 				struct mvebu_gpio_chip *mvchip)
 {
-- 
2.29.2

Re: [PATCH 2/5] gpio: mvebu: switch pwm duration registers to regmap

[Andrew Lunn]

On Wed, Nov 18, 2020 at 12:30:43PM +0200, Baruch Siach wrote:
> Commit 2233bf7a92e ("gpio: mvebu: switch to regmap for register access")
> changed most readl/writel registers access calls to the regmap API in
> preparation for Armada 7K/8K support. PWM duration registers were left using
> readl/writel, as the driver does not support PWM for Armada 7K/8K.
> 
> Switch PWM duration registers to regmap as first step in adding Armada 7K/8K
> PWM functionality support.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH 3/5] gpio: mvebu: add pwm support for Armada 8K/7K

[Baruch Siach]

Use the pwm-offset DT property to store the location of PWM signal
duration registers.

Since we have more than two GPIO chips per system, we can't use the
alias id to differentiate between them. Use the offset value for that.

Move mvebu_pwm_probe() call before irq support code. The AP80x does not
provide irq support, but does provide PWM. Don't skip PWM probe because
of that.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 112 +++++++++++++++++++++++++-------------
 1 file changed, 75 insertions(+), 37 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 946571e70928..8602afd21673 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -70,7 +70,12 @@
  */
 #define PWM_BLINK_ON_DURATION_OFF	0x0
 #define PWM_BLINK_OFF_DURATION_OFF	0x4
+#define PWM_BLINK_COUNTER_B_OFF		0x8
 
+/* Armada 8k variant gpios register offsets */
+#define AP80X_GPIO0_OFF_A8K		0x1040
+#define CP11X_GPIO0_OFF_A8K		0x100
+#define CP11X_GPIO1_OFF_A8K		0x140
 
 /* The MV78200 has per-CPU registers for edge mask and level mask */
 #define GPIO_EDGE_MASK_MV78200_OFF(cpu)	  ((cpu) ? 0x30 : 0x18)
@@ -93,6 +98,7 @@
 
 struct mvebu_pwm {
 	struct regmap		*regs;
+	u32			 offset;
 	unsigned long		 clk_rate;
 	struct gpio_desc	*gpiod;
 	struct pwm_chip		 chip;
@@ -283,12 +289,12 @@ mvebu_gpio_write_level_mask(struct mvebu_gpio_chip *mvchip, u32 val)
  */
 static unsigned int mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
 {
-	return PWM_BLINK_ON_DURATION_OFF;
+	return mvpwm->offset + PWM_BLINK_ON_DURATION_OFF;
 }
 
 static unsigned int mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
 {
-	return PWM_BLINK_OFF_DURATION_OFF;
+	return mvpwm->offset + PWM_BLINK_OFF_DURATION_OFF;
 }
 
 /*
@@ -781,51 +787,80 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 	struct device *dev = &pdev->dev;
 	struct mvebu_pwm *mvpwm;
 	void __iomem *base;
+	u32 offset;
 	u32 set;
 
-	if (!of_device_is_compatible(mvchip->chip.of_node,
-				     "marvell,armada-370-gpio"))
-		return 0;
-
-	/*
-	 * There are only two sets of PWM configuration registers for
-	 * all the GPIO lines on those SoCs which this driver reserves
-	 * for the first two GPIO chips. So if the resource is missing
-	 * we can't treat it as an error.
-	 */
-	if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
+	if (of_device_is_compatible(mvchip->chip.of_node,
+				    "marvell,armada-370-gpio")) {
+		/*
+		 * There are only two sets of PWM configuration registers for
+		 * all the GPIO lines on those SoCs which this driver reserves
+		 * for the first two GPIO chips. So if the resource is missing
+		 * we can't treat it as an error.
+		 */
+		if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
+			return 0;
+		offset = 0;
+	} else if (mvchip->soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K) {
+		int ret = of_property_read_u32(dev->of_node, "pwm-offset",
+					       &offset);
+		if (ret < 0)
+			return 0;
+	} else {
 		return 0;
+	}
 
 	if (IS_ERR(mvchip->clk))
 		return PTR_ERR(mvchip->clk);
 
-	/*
-	 * Use set A for lines of GPIO chip with id 0, B for GPIO chip
-	 * with id 1. Don't allow further GPIO chips to be used for PWM.
-	 */
-	if (id == 0)
-		set = 0;
-	else if (id == 1)
-		set = U32_MAX;
-	else
-		return -EINVAL;
-	regmap_write(mvchip->regs,
-		     GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set);
-
 	mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
 	if (!mvpwm)
 		return -ENOMEM;
 	mvchip->mvpwm = mvpwm;
 	mvpwm->mvchip = mvchip;
+	mvpwm->offset = offset;
+
+	if (mvchip->soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K) {
+		mvpwm->regs = mvchip->regs;
+
+		switch (mvchip->offset) {
+		case AP80X_GPIO0_OFF_A8K:
+		case CP11X_GPIO0_OFF_A8K:
+			/* Blink counter A */
+			set = 0;
+			break;
+		case CP11X_GPIO1_OFF_A8K:
+			/* Blink counter B */
+			set = U32_MAX;
+			mvpwm->offset += PWM_BLINK_COUNTER_B_OFF;
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		base = devm_platform_ioremap_resource_byname(pdev, "pwm");
+		if (IS_ERR(base))
+			return PTR_ERR(base);
 
-	base = devm_platform_ioremap_resource_byname(pdev, "pwm");
-	if (IS_ERR(base))
-		return PTR_ERR(base);
+		mvpwm->regs = devm_regmap_init_mmio(&pdev->dev, base,
+						    &mvebu_gpio_regmap_config);
+		if (IS_ERR(mvpwm->regs))
+			return PTR_ERR(mvpwm->regs);
+
+		/*
+		 * Use set A for lines of GPIO chip with id 0, B for GPIO chip
+		 * with id 1. Don't allow further GPIO chips to be used for PWM.
+		 */
+		if (id == 0)
+			set = 0;
+		else if (id == 1)
+			set = U32_MAX;
+		else
+			return -EINVAL;
+	}
 
-	mvpwm->regs = devm_regmap_init_mmio(&pdev->dev, base,
-					    &mvebu_gpio_regmap_config);
-	if (IS_ERR(mvpwm->regs))
-		return PTR_ERR(mvpwm->regs);
+	regmap_write(mvchip->regs,
+		     GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set);
 
 	mvpwm->clk_rate = clk_get_rate(mvchip->clk);
 	if (!mvpwm->clk_rate) {
@@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 
 	devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
 
+	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
+	if (IS_ENABLED(CONFIG_PWM)) {
+		err = mvebu_pwm_probe(pdev, mvchip, id);
+		if (err)
+			return err;
+	}
+
 	/* Some gpio controllers do not provide irq support */
 	if (!have_irqs)
 		return 0;
@@ -1257,10 +1299,6 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 						 mvchip);
 	}
 
-	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
-	if (IS_ENABLED(CONFIG_PWM))
-		return mvebu_pwm_probe(pdev, mvchip, id);
-
 	return 0;
 
 err_domain:
-- 
2.29.2

Re: [PATCH 3/5] gpio: mvebu: add pwm support for Armada 8K/7K

[Andrew Lunn]

On Wed, Nov 18, 2020 at 12:30:44PM +0200, Baruch Siach wrote:
> Use the pwm-offset DT property to store the location of PWM signal
> duration registers.
> 
> Since we have more than two GPIO chips per system, we can't use the
> alias id to differentiate between them. Use the offset value for that.
> 
> Move mvebu_pwm_probe() call before irq support code. The AP80x does not
> provide irq support, but does provide PWM. Don't skip PWM probe because
> of that.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/gpio/gpio-mvebu.c | 112 +++++++++++++++++++++++++-------------
>  1 file changed, 75 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index 946571e70928..8602afd21673 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -70,7 +70,12 @@
>   */
>  #define PWM_BLINK_ON_DURATION_OFF	0x0
>  #define PWM_BLINK_OFF_DURATION_OFF	0x4
> +#define PWM_BLINK_COUNTER_B_OFF		0x8
>  
> +/* Armada 8k variant gpios register offsets */
> +#define AP80X_GPIO0_OFF_A8K		0x1040
> +#define CP11X_GPIO0_OFF_A8K		0x100
> +#define CP11X_GPIO1_OFF_A8K		0x140
>  
>  /* The MV78200 has per-CPU registers for edge mask and level mask */
>  #define GPIO_EDGE_MASK_MV78200_OFF(cpu)	  ((cpu) ? 0x30 : 0x18)
> @@ -93,6 +98,7 @@
>  
>  struct mvebu_pwm {
>  	struct regmap		*regs;
> +	u32			 offset;
>  	unsigned long		 clk_rate;
>  	struct gpio_desc	*gpiod;
>  	struct pwm_chip		 chip;
> @@ -283,12 +289,12 @@ mvebu_gpio_write_level_mask(struct mvebu_gpio_chip *mvchip, u32 val)
>   */
>  static unsigned int mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
>  {
> -	return PWM_BLINK_ON_DURATION_OFF;
> +	return mvpwm->offset + PWM_BLINK_ON_DURATION_OFF;
>  }
>  
>  static unsigned int mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
>  {
> -	return PWM_BLINK_OFF_DURATION_OFF;
> +	return mvpwm->offset + PWM_BLINK_OFF_DURATION_OFF;
>  }
>  
>  /*
> @@ -781,51 +787,80 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>  	struct device *dev = &pdev->dev;
>  	struct mvebu_pwm *mvpwm;
>  	void __iomem *base;
> +	u32 offset;
>  	u32 set;
>  
> -	if (!of_device_is_compatible(mvchip->chip.of_node,
> -				     "marvell,armada-370-gpio"))
> -		return 0;
> -
> -	/*
> -	 * There are only two sets of PWM configuration registers for
> -	 * all the GPIO lines on those SoCs which this driver reserves
> -	 * for the first two GPIO chips. So if the resource is missing
> -	 * we can't treat it as an error.
> -	 */
> -	if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
> +	if (of_device_is_compatible(mvchip->chip.of_node,
> +				    "marvell,armada-370-gpio")) {
> +		/*
> +		 * There are only two sets of PWM configuration registers for
> +		 * all the GPIO lines on those SoCs which this driver reserves
> +		 * for the first two GPIO chips. So if the resource is missing
> +		 * we can't treat it as an error.
> +		 */
> +		if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
> +			return 0;
> +		offset = 0;
> +	} else if (mvchip->soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K) {
> +		int ret = of_property_read_u32(dev->of_node, "pwm-offset",
> +					       &offset);
> +		if (ret < 0)
> +			return 0;

It would look more uniform if this was

	if (of_device_is_compatible(mvchip->chip.of_node,
				    "marvell,armada-8k-gpio")) {

> +	} else {
>  		return 0;
> +	}
>  
>  	if (IS_ERR(mvchip->clk))
>  		return PTR_ERR(mvchip->clk);
>  
> -	/*
> -	 * Use set A for lines of GPIO chip with id 0, B for GPIO chip
> -	 * with id 1. Don't allow further GPIO chips to be used for PWM.
> -	 */
> -	if (id == 0)
> -		set = 0;
> -	else if (id == 1)
> -		set = U32_MAX;
> -	else
> -		return -EINVAL;
> -	regmap_write(mvchip->regs,
> -		     GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set);
> -
>  	mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
>  	if (!mvpwm)
>  		return -ENOMEM;
>  	mvchip->mvpwm = mvpwm;
>  	mvpwm->mvchip = mvchip;
> +	mvpwm->offset = offset;
> +
> +	if (mvchip->soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K) {
> +		mvpwm->regs = mvchip->regs;
> +
> +		switch (mvchip->offset) {
> +		case AP80X_GPIO0_OFF_A8K:
> +		case CP11X_GPIO0_OFF_A8K:
> +			/* Blink counter A */
> +			set = 0;
> +			break;
> +		case CP11X_GPIO1_OFF_A8K:
> +			/* Blink counter B */
> +			set = U32_MAX;
> +			mvpwm->offset += PWM_BLINK_COUNTER_B_OFF;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	} else {
> +		base = devm_platform_ioremap_resource_byname(pdev, "pwm");
> +		if (IS_ERR(base))
> +			return PTR_ERR(base);
>  
> -	base = devm_platform_ioremap_resource_byname(pdev, "pwm");
> -	if (IS_ERR(base))
> -		return PTR_ERR(base);
> +		mvpwm->regs = devm_regmap_init_mmio(&pdev->dev, base,
> +						    &mvebu_gpio_regmap_config);
> +		if (IS_ERR(mvpwm->regs))
> +			return PTR_ERR(mvpwm->regs);
> +
> +		/*
> +		 * Use set A for lines of GPIO chip with id 0, B for GPIO chip
> +		 * with id 1. Don't allow further GPIO chips to be used for PWM.
> +		 */
> +		if (id == 0)
> +			set = 0;
> +		else if (id == 1)
> +			set = U32_MAX;
> +		else
> +			return -EINVAL;
> +	}
>  
> -	mvpwm->regs = devm_regmap_init_mmio(&pdev->dev, base,
> -					    &mvebu_gpio_regmap_config);
> -	if (IS_ERR(mvpwm->regs))
> -		return PTR_ERR(mvpwm->regs);
> +	regmap_write(mvchip->regs,
> +		     GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set);
>  
>  	mvpwm->clk_rate = clk_get_rate(mvchip->clk);
>  	if (!mvpwm->clk_rate) {
> @@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>  
>  	devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
>  
> +	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
> +	if (IS_ENABLED(CONFIG_PWM)) {
> +		err = mvebu_pwm_probe(pdev, mvchip, id);
> +		if (err)
> +			return err;
> +	}
> +

The existing error handling looks odd here. Why is there no goto
err_domain when probing the PWMs fails? I wonder if this a bug from me
from a long time again?

	Andrew

Re: [PATCH 3/5] gpio: mvebu: add pwm support for Armada 8K/7K

[Baruch Siach]

Hi Andrew,

On Thu, Nov 19 2020, Andrew Lunn wrote:
> On Wed, Nov 18, 2020 at 12:30:44PM +0200, Baruch Siach wrote:
>> Use the pwm-offset DT property to store the location of PWM signal
>> duration registers.
>> 
>> Since we have more than two GPIO chips per system, we can't use the
>> alias id to differentiate between them. Use the offset value for that.
>> 
>> Move mvebu_pwm_probe() call before irq support code. The AP80x does not
>> provide irq support, but does provide PWM. Don't skip PWM probe because
>> of that.
>> 
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

[snip]

>> @@ -781,51 +787,80 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>>  	struct device *dev = &pdev->dev;
>>  	struct mvebu_pwm *mvpwm;
>>  	void __iomem *base;
>> +	u32 offset;
>>  	u32 set;
>>  
>> -	if (!of_device_is_compatible(mvchip->chip.of_node,
>> -				     "marvell,armada-370-gpio"))
>> -		return 0;
>> -
>> -	/*
>> -	 * There are only two sets of PWM configuration registers for
>> -	 * all the GPIO lines on those SoCs which this driver reserves
>> -	 * for the first two GPIO chips. So if the resource is missing
>> -	 * we can't treat it as an error.
>> -	 */
>> -	if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
>> +	if (of_device_is_compatible(mvchip->chip.of_node,
>> +				    "marvell,armada-370-gpio")) {
>> +		/*
>> +		 * There are only two sets of PWM configuration registers for
>> +		 * all the GPIO lines on those SoCs which this driver reserves
>> +		 * for the first two GPIO chips. So if the resource is missing
>> +		 * we can't treat it as an error.
>> +		 */
>> +		if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
>> +			return 0;
>> +		offset = 0;
>> +	} else if (mvchip->soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K) {
>> +		int ret = of_property_read_u32(dev->of_node, "pwm-offset",
>> +					       &offset);
>> +		if (ret < 0)
>> +			return 0;
>
> It would look more uniform if this was
>
> 	if (of_device_is_compatible(mvchip->chip.of_node,
> 				    "marvell,armada-8k-gpio")) {

Right. However I use soc_variant again below. I think that
of_device_is_compatible is too verbose for that.

In fact, I'd rather use soc_variant for marvell,armada-370-gpio as
well. The trouble is that marvell,armada-370-gpio is not equivalent to
MVEBU_GPIO_SOC_VARIANT_ORION. Changing that is more intrusive.

>> +	} else {
>>  		return 0;
>> +	}

[snip]

>> @@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>>  
>>  	devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
>>  
>> +	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
>> +	if (IS_ENABLED(CONFIG_PWM)) {
>> +		err = mvebu_pwm_probe(pdev, mvchip, id);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>
> The existing error handling looks odd here. Why is there no goto
> err_domain when probing the PWMs fails? I wonder if this a bug from me
> from a long time again?

What would you release under the err_domain label? As far as I can see
all resources are allocated using devres, and released automatically on
failure exit.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH 3/5] gpio: mvebu: add pwm support for Armada 8K/7K

[Andrew Lunn]

> >> @@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> >>  
> >>  	devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
> >>  
> >> +	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
> >> +	if (IS_ENABLED(CONFIG_PWM)) {
> >> +		err = mvebu_pwm_probe(pdev, mvchip, id);
> >> +		if (err)
> >> +			return err;
> >> +	}
> >> +
> >
> > The existing error handling looks odd here. Why is there no goto
> > err_domain when probing the PWMs fails? I wonder if this a bug from me
> > from a long time again?
> 
> What would you release under the err_domain label? As far as I can see
> all resources are allocated using devres, and released automatically on
> failure exit.

The IRQ domain is still registers. So once the memory is automatically
freed, don't we have a potential use after free?

       Andrew

Re: [PATCH 3/5] gpio: mvebu: add pwm support for Armada 8K/7K

[Baruch Siach]

Hi Andrew,

On Thu, Nov 19 2020, Andrew Lunn wrote:
>> >> @@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>> >>  
>> >>  	devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
>> >>  
>> >> +	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
>> >> +	if (IS_ENABLED(CONFIG_PWM)) {
>> >> +		err = mvebu_pwm_probe(pdev, mvchip, id);
>> >> +		if (err)
>> >> +			return err;
>> >> +	}
>> >> +
>> >
>> > The existing error handling looks odd here. Why is there no goto
>> > err_domain when probing the PWMs fails? I wonder if this a bug from me
>> > from a long time again?
>> 
>> What would you release under the err_domain label? As far as I can see
>> all resources are allocated using devres, and released automatically on
>> failure exit.
>
> The IRQ domain is still registers. So once the memory is automatically
> freed, don't we have a potential use after free?

This patch moves PWM registration before IRQ domain registration for
another reason as mentioned in the commit log. So this might
incidentally fix the bug.

Would you prefer a separate patch for that with a 'Fixes:
757642f9a584e8' tag?

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH 3/5] gpio: mvebu: add pwm support for Armada 8K/7K

[Bartosz Golaszewski]

On Thu, Nov 19, 2020 at 2:47 PM Baruch Siach <baruch@tkos.co.il> wrote:
>
> Hi Andrew,
>
> On Thu, Nov 19 2020, Andrew Lunn wrote:
> >> >> @@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> >> >>
> >> >>   devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
> >> >>
> >> >> + /* Some MVEBU SoCs have simple PWM support for GPIO lines */
> >> >> + if (IS_ENABLED(CONFIG_PWM)) {
> >> >> +         err = mvebu_pwm_probe(pdev, mvchip, id);
> >> >> +         if (err)
> >> >> +                 return err;
> >> >> + }
> >> >> +
> >> >
> >> > The existing error handling looks odd here. Why is there no goto
> >> > err_domain when probing the PWMs fails? I wonder if this a bug from me
> >> > from a long time again?
> >>
> >> What would you release under the err_domain label? As far as I can see
> >> all resources are allocated using devres, and released automatically on
> >> failure exit.
> >
> > The IRQ domain is still registers. So once the memory is automatically
> > freed, don't we have a potential use after free?
>
> This patch moves PWM registration before IRQ domain registration for
> another reason as mentioned in the commit log. So this might
> incidentally fix the bug.
>
> Would you prefer a separate patch for that with a 'Fixes:
> 757642f9a584e8' tag?
>
> baruch
>

Baruch: does this series conflict with the fix you sent? I'm thinking
about how to take it through the next and fixes trees.

Bartosz

Re: [PATCH 3/5] gpio: mvebu: add pwm support for Armada 8K/7K

[Baruch Siach]

Hi Bartosz,

On Tue, Dec 01 2020, Bartosz Golaszewski wrote:
> On Thu, Nov 19, 2020 at 2:47 PM Baruch Siach <baruch@tkos.co.il> wrote:
>> On Thu, Nov 19 2020, Andrew Lunn wrote:
>> >> >> @@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>> >> >>
>> >> >>   devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
>> >> >>
>> >> >> + /* Some MVEBU SoCs have simple PWM support for GPIO lines */
>> >> >> + if (IS_ENABLED(CONFIG_PWM)) {
>> >> >> +         err = mvebu_pwm_probe(pdev, mvchip, id);
>> >> >> +         if (err)
>> >> >> +                 return err;
>> >> >> + }
>> >> >> +
>> >> >
>> >> > The existing error handling looks odd here. Why is there no goto
>> >> > err_domain when probing the PWMs fails? I wonder if this a bug from me
>> >> > from a long time again?
>> >>
>> >> What would you release under the err_domain label? As far as I can see
>> >> all resources are allocated using devres, and released automatically on
>> >> failure exit.
>> >
>> > The IRQ domain is still registers. So once the memory is automatically
>> > freed, don't we have a potential use after free?
>>
>> This patch moves PWM registration before IRQ domain registration for
>> another reason as mentioned in the commit log. So this might
>> incidentally fix the bug.
>>
>> Would you prefer a separate patch for that with a 'Fixes:
>> 757642f9a584e8' tag?
>
> Baruch: does this series conflict with the fix you sent? I'm thinking
> about how to take it through the next and fixes trees.

Yes, It conflicts.

I can send in a single series v3 of the fix along with the other patches
rebased on top. Would that work for you?

Thanks,
baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[PATCH 4/5] arm64: dts: armada: add pwm offsets for ap/cp gpios

[Baruch Siach]

The 'pwm-offset' property of both GPIO blocks (per CP component) point
to the same counter registers offset. The driver will decide how to use
counters A/B.

This is different from the convention of pwm on earlier Armada series
(370/38x). On those systems the assignment of A/B counters to GPIO
blocks is coded in both DT and the driver. The actual behaviour of the
current driver on Armada 8K/7K is the same as earlier systems.

Add also clock properties for base pwm frequency reference.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 arch/arm64/boot/dts/marvell/armada-ap80x.dtsi |  3 +++
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
index 12e477f1aeb9..7f8d589ed938 100644
--- a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
@@ -281,6 +281,9 @@ ap_gpio: gpio@1040 {
 					gpio-controller;
 					#gpio-cells = <2>;
 					gpio-ranges = <&ap_pinctrl 0 0 20>;
+					pwm-offset = <0x10c0>;
+					#pwm-cells = <2>;
+					clocks = <&ap_clk 3>;
 				};
 			};
 
diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
index 9dcf16beabf5..366daec416df 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
@@ -234,12 +234,17 @@ CP11X_LABEL(gpio1): gpio@100 {
 				gpio-controller;
 				#gpio-cells = <2>;
 				gpio-ranges = <&CP11X_LABEL(pinctrl) 0 0 32>;
+				pwm-offset = <0x1f0>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				interrupts = <86 IRQ_TYPE_LEVEL_HIGH>,
 					<85 IRQ_TYPE_LEVEL_HIGH>,
 					<84 IRQ_TYPE_LEVEL_HIGH>,
 					<83 IRQ_TYPE_LEVEL_HIGH>;
 				#interrupt-cells = <2>;
+				clock-names = "core", "axi";
+				clocks = <&CP11X_LABEL(clk) 1 21>,
+					 <&CP11X_LABEL(clk) 1 17>;
 				status = "disabled";
 			};
 
@@ -250,12 +255,17 @@ CP11X_LABEL(gpio2): gpio@140 {
 				gpio-controller;
 				#gpio-cells = <2>;
 				gpio-ranges = <&CP11X_LABEL(pinctrl) 0 32 31>;
+				pwm-offset = <0x1f0>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				interrupts = <82 IRQ_TYPE_LEVEL_HIGH>,
 					<81 IRQ_TYPE_LEVEL_HIGH>,
 					<80 IRQ_TYPE_LEVEL_HIGH>,
 					<79 IRQ_TYPE_LEVEL_HIGH>;
 				#interrupt-cells = <2>;
+				clock-names = "core", "axi";
+				clocks = <&CP11X_LABEL(clk) 1 21>,
+					 <&CP11X_LABEL(clk) 1 17>;
 				status = "disabled";
 			};
 		};
-- 
2.29.2

[PATCH 5/5] dt-bindings: ap806: document gpio pwm-offset property

[Baruch Siach]

Update the example as well. Add the '#pwm-cells' and 'clocks' properties
for a complete working example.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 .../bindings/arm/marvell/ap80x-system-controller.txt      | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt b/Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt
index e31511255d8e..a754e8992450 100644
--- a/Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt
+++ b/Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt
@@ -80,6 +80,11 @@ Required properties:
 
 - offset: offset address inside the syscon block
 
+Optional properties:
+
+- pwm-offset: offset address of PWM duration control registers inside the
+  syscon block
+
 Example:
 ap_syscon: system-controller@6f4000 {
 	compatible = "syscon", "simple-mfd";
@@ -101,6 +106,9 @@ ap_syscon: system-controller@6f4000 {
 		gpio-controller;
 		#gpio-cells = <2>;
 		gpio-ranges = <&ap_pinctrl 0 0 19>;
+		pwm-offset = <0x10c0>;
+		#pwm-cells = <2>;
+		clocks = <&ap_clk 3>;
 	};
 };
 
-- 
2.29.2

Re: [PATCH 0/5] gpio: mvebu: Armada 8K/7K PWM support

[Andrew Lunn]

On Wed, Nov 18, 2020 at 12:30:41PM +0200, Baruch Siach wrote:
> The gpio-mvebu driver supports the PWM functionality of the GPIO block for 
> earlier Armada variants like XP, 370 and 38x. This series extends support to 
> newer Armada variants that use CP11x and AP80x, like Armada 8K and 7K.
> 
> This series adds adds the 'pwm-offset' property to DT binding. 'pwm-offset' 

One adds is enough.

> points to the base of A/B counter registers that determine the PWM period and 
> duty cycle.
> 
> The existing PWM DT binding reflects an arbitrary decision to allocate the A 
> counter to the first GPIO block, and B counter to the other one.

It was not arbitrary. I decided on KISS. The few devices i've seen
using this have been for a single GPIO/PWN controlled fan. KISS was
sufficient for that, so why make it more complex?

	   Andrew

Re: [PATCH 0/5] gpio: mvebu: Armada 8K/7K PWM support

[Baruch Siach]

Hi Andrew,

Thanks for your review comments.

On Thu, Nov 19 2020, Andrew Lunn wrote:
> On Wed, Nov 18, 2020 at 12:30:41PM +0200, Baruch Siach wrote:
>> The gpio-mvebu driver supports the PWM functionality of the GPIO block for 
>> earlier Armada variants like XP, 370 and 38x. This series extends support to 
>> newer Armada variants that use CP11x and AP80x, like Armada 8K and 7K.
>> 
>> This series adds adds the 'pwm-offset' property to DT binding. 'pwm-offset' 
>
> One adds is enough.
>
>> points to the base of A/B counter registers that determine the PWM period and 
>> duty cycle.
>> 
>> The existing PWM DT binding reflects an arbitrary decision to allocate the A 
>> counter to the first GPIO block, and B counter to the other one.
>
> It was not arbitrary. I decided on KISS. The few devices i've seen
> using this have been for a single GPIO/PWN controlled fan. KISS was
> sufficient for that, so why make it more complex?

In saying "arbitrary" I don't mean to say it's not a good choice in the
context of the Linux PWM and GPIO subsystems. But this choice is still
arbitrary from hardware point of view. DT is meant to describe the
hardware. I think that coding the A/B counters allocation choice in DT
is not optimal in terms for hardware description. Both counters are
usable for both GPIO blocks.

I also don't see how this makes anything more complex. The driver code
is already aware of this A/B allocation (GPIO_BLINK_CNT_SELECT_OFF). My
suggestion is to keep this decision in the driver, and leave DT to
describe the hardware. This costs us a single code line (patch #3):

  mvpwm->offset += PWM_BLINK_COUNTER_B_OFF;

Does that make sense?

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -