gpio: mvebu: Armada 8K/7K PWM support

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 <[email protected]>

Fixes 2233bf7a92e7 ("gpio: mvebu: switch to regmap for register access")

Signed-off-by Baruch Siach <[email protected]>

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 <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpio-mvebu.c | 112 +++++++++++++++++++++++++-------------
 1 file changed, 75 insertions(+), 37 deletions(-)

drivers/gpio/gpio-mvebu.c +15 -13

index 946571e70928..8602afd21673 100644
@@ -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

drivers/gpio/gpio-mvebu.c +43 -20

+	} 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) {

Quoted 11 lines · 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 <[email protected]>

Quoted 24 lines

drivers/gpio/gpio-mvebu.c +7 -11

@@ -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

drivers/gpio/gpio-mvebu.c static int mvebu_pwm_probe(struct platform_device *pdev, · line 808

Quoted 10 lines

drivers/gpio/gpio-mvebu.c +10 -0

+		 * 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")) {

Quoted 3 lines

drivers/gpio/gpio-mvebu.c +2 -0

+	} else {
 		return 0;
+	}

Quoted 4 lines

drivers/gpio/gpio-mvebu.c +0 -0

@@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 
 	devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
 

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1244

Quoted 7 lines

drivers/gpio/gpio-mvebu.c +7 -0

+	/* 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?

Quoted 4 lines

Quoted 4 lines

drivers/gpio/gpio-mvebu.c +0 -0

@@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 
 	devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
 

Quoted 11 lines

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1244

Quoted 7 lines

drivers/gpio/gpio-mvebu.c +7 -0

+	/* 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.

Quoted 16 lines

Quoted 15 lines

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1244

Quoted 11 lines

drivers/gpio/gpio-mvebu.c +7 -0

@@ -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?

Quoted 3 lines

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?

Hi Andrew,

Quoted 16 lines · Andrew Lunn

Quoted 16 lines

Quoted 15 lines

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1244

Quoted 11 lines

drivers/gpio/gpio-mvebu.c +7 -0

@@ -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?

Quoted 6 lines

Quoted 3 lines

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

Quoted 28 lines · Baruch Siach

Quoted 16 lines · Andrew Lunn

Quoted 16 lines

Quoted 15 lines

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1244

Quoted 11 lines

drivers/gpio/gpio-mvebu.c +7 -0

@@ -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?

Quoted 6 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

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.

Quoted 5 lines · 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'

One adds is enough.

Quoted 3 lines

points to the base of A/B counter registers that determine the PWM period and duty cycle.

Quoted 2 lines

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?