[PATCH 0/2] Add fixes to iProc GPIO driver

[PATCH 0/2] Add fixes to iProc GPIO driver

[Srinath Mannam]

This patch series adds the following fixes to the iProc GPIO driver
 - Fix Warning message given for shared irqchip data structure
 - Fix pinconfig of pull-up/down and drive strength for AON/CRMU GPIOs

This patch set is based on Linux-5.2-rc4.

Changes from v1:
  - Add Fixes tags in both patches

Li Jin (1):
  gpio: iproc-gpio: Fix incorrect pinconf configurations

Rayagonda Kokatanur (1):
  gpio: iproc-gpio: Handle interrupts for multiple instances

 drivers/pinctrl/bcm/pinctrl-iproc-gpio.c | 117 +++++++++++++++++++++++--------
 1 file changed, 88 insertions(+), 29 deletions(-)

-- 
2.7.4

[PATCH 1/2] gpio: iproc-gpio: Fix incorrect pinconf configurations

[Srinath Mannam]

From: Li Jin <li.jin@broadcom.com>

Fix drive strength for AON/CRMU controller; fix pull-up/down setting
for CCM/CDRU controller.

Fixes: 616043d58a89 ("pinctrl: Rename gpio driver from cygnus to iproc")
Signed-off-by: Li Jin <li.jin@broadcom.com>
---
 drivers/pinctrl/bcm/pinctrl-iproc-gpio.c | 96 +++++++++++++++++++++++++-------
 1 file changed, 77 insertions(+), 19 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
index b70058c..20b9864 100644
--- a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
@@ -54,8 +54,12 @@
 /* drive strength control for ASIU GPIO */
 #define IPROC_GPIO_ASIU_DRV0_CTRL_OFFSET 0x58
 
-/* drive strength control for CCM/CRMU (AON) GPIO */
-#define IPROC_GPIO_DRV0_CTRL_OFFSET  0x00
+/* pinconf for CCM GPIO */
+#define IPROC_GPIO_PULL_DN_OFFSET   0x10
+#define IPROC_GPIO_PULL_UP_OFFSET   0x14
+
+/* pinconf for CRMU(aon) GPIO and CCM GPIO*/
+#define IPROC_GPIO_DRV_CTRL_OFFSET  0x00
 
 #define GPIO_BANK_SIZE 0x200
 #define NGPIOS_PER_BANK 32
@@ -76,6 +80,12 @@ enum iproc_pinconf_param {
 	IPROC_PINCON_MAX,
 };
 
+enum iproc_pinconf_ctrl_type {
+	IOCTRL_TYPE_AON = 1,
+	IOCTRL_TYPE_CDRU,
+	IOCTRL_TYPE_INVALID,
+};
+
 /*
  * Iproc GPIO core
  *
@@ -100,6 +110,7 @@ struct iproc_gpio {
 
 	void __iomem *base;
 	void __iomem *io_ctrl;
+	enum iproc_pinconf_ctrl_type io_ctrl_type;
 
 	raw_spinlock_t lock;
 
@@ -461,20 +472,44 @@ static const struct pinctrl_ops iproc_pctrl_ops = {
 static int iproc_gpio_set_pull(struct iproc_gpio *chip, unsigned gpio,
 				bool disable, bool pull_up)
 {
+	void __iomem *base;
 	unsigned long flags;
+	unsigned int shift;
+	u32 val_1, val_2;
 
 	raw_spin_lock_irqsave(&chip->lock, flags);
-
-	if (disable) {
-		iproc_set_bit(chip, IPROC_GPIO_RES_EN_OFFSET, gpio, false);
+	if (chip->io_ctrl_type == IOCTRL_TYPE_CDRU) {
+		base = chip->io_ctrl;
+		shift = IPROC_GPIO_SHIFT(gpio);
+
+		val_1 = readl(base + IPROC_GPIO_PULL_UP_OFFSET);
+		val_2 = readl(base + IPROC_GPIO_PULL_DN_OFFSET);
+		if (disable) {
+			/* no pull-up or pull-down */
+			val_1 &= ~BIT(shift);
+			val_2 &= ~BIT(shift);
+		} else if (pull_up) {
+			val_1 |= BIT(shift);
+			val_2 &= ~BIT(shift);
+		} else {
+			val_1 &= ~BIT(shift);
+			val_2 |= BIT(shift);
+		}
+		writel(val_1, base + IPROC_GPIO_PULL_UP_OFFSET);
+		writel(val_2, base + IPROC_GPIO_PULL_DN_OFFSET);
 	} else {
-		iproc_set_bit(chip, IPROC_GPIO_PAD_RES_OFFSET, gpio,
-			       pull_up);
-		iproc_set_bit(chip, IPROC_GPIO_RES_EN_OFFSET, gpio, true);
+		if (disable) {
+			iproc_set_bit(chip, IPROC_GPIO_RES_EN_OFFSET, gpio,
+				      false);
+		} else {
+			iproc_set_bit(chip, IPROC_GPIO_PAD_RES_OFFSET, gpio,
+				      pull_up);
+			iproc_set_bit(chip, IPROC_GPIO_RES_EN_OFFSET, gpio,
+				      true);
+		}
 	}
 
 	raw_spin_unlock_irqrestore(&chip->lock, flags);
-
 	dev_dbg(chip->dev, "gpio:%u set pullup:%d\n", gpio, pull_up);
 
 	return 0;
@@ -483,14 +518,35 @@ static int iproc_gpio_set_pull(struct iproc_gpio *chip, unsigned gpio,
 static void iproc_gpio_get_pull(struct iproc_gpio *chip, unsigned gpio,
 				 bool *disable, bool *pull_up)
 {
+	void __iomem *base;
 	unsigned long flags;
+	unsigned int shift;
+	u32 val_1, val_2;
 
 	raw_spin_lock_irqsave(&chip->lock, flags);
-	*disable = !iproc_get_bit(chip, IPROC_GPIO_RES_EN_OFFSET, gpio);
-	*pull_up = iproc_get_bit(chip, IPROC_GPIO_PAD_RES_OFFSET, gpio);
+	if (chip->io_ctrl_type == IOCTRL_TYPE_CDRU) {
+		base = chip->io_ctrl;
+		shift = IPROC_GPIO_SHIFT(gpio);
+
+		val_1 = readl(base + IPROC_GPIO_PULL_UP_OFFSET) & BIT(shift);
+		val_2 = readl(base + IPROC_GPIO_PULL_DN_OFFSET) & BIT(shift);
+
+		*pull_up = val_1 ? true : false;
+		*disable = (val_1 | val_2) ? false : true;
+
+	} else {
+		*disable = !iproc_get_bit(chip, IPROC_GPIO_RES_EN_OFFSET, gpio);
+		*pull_up = iproc_get_bit(chip, IPROC_GPIO_PAD_RES_OFFSET, gpio);
+	}
 	raw_spin_unlock_irqrestore(&chip->lock, flags);
 }
 
+#define DRV_STRENGTH_OFFSET(gpio, bit, type)  ((type) == IOCTRL_TYPE_AON ? \
+	((2 - (bit)) * 4 + IPROC_GPIO_DRV_CTRL_OFFSET) : \
+	((type) == IOCTRL_TYPE_CDRU) ? \
+	((bit) * 4 + IPROC_GPIO_DRV_CTRL_OFFSET) : \
+	((bit) * 4 + IPROC_GPIO_REG(gpio, IPROC_GPIO_ASIU_DRV0_CTRL_OFFSET)))
+
 static int iproc_gpio_set_strength(struct iproc_gpio *chip, unsigned gpio,
 				    unsigned strength)
 {
@@ -505,11 +561,8 @@ static int iproc_gpio_set_strength(struct iproc_gpio *chip, unsigned gpio,
 
 	if (chip->io_ctrl) {
 		base = chip->io_ctrl;
-		offset = IPROC_GPIO_DRV0_CTRL_OFFSET;
 	} else {
 		base = chip->base;
-		offset = IPROC_GPIO_REG(gpio,
-					 IPROC_GPIO_ASIU_DRV0_CTRL_OFFSET);
 	}
 
 	shift = IPROC_GPIO_SHIFT(gpio);
@@ -520,11 +573,11 @@ static int iproc_gpio_set_strength(struct iproc_gpio *chip, unsigned gpio,
 	raw_spin_lock_irqsave(&chip->lock, flags);
 	strength = (strength / 2) - 1;
 	for (i = 0; i < GPIO_DRV_STRENGTH_BITS; i++) {
+		offset = DRV_STRENGTH_OFFSET(gpio, i, chip->io_ctrl_type);
 		val = readl(base + offset);
 		val &= ~BIT(shift);
 		val |= ((strength >> i) & 0x1) << shift;
 		writel(val, base + offset);
-		offset += 4;
 	}
 	raw_spin_unlock_irqrestore(&chip->lock, flags);
 
@@ -541,11 +594,8 @@ static int iproc_gpio_get_strength(struct iproc_gpio *chip, unsigned gpio,
 
 	if (chip->io_ctrl) {
 		base = chip->io_ctrl;
-		offset = IPROC_GPIO_DRV0_CTRL_OFFSET;
 	} else {
 		base = chip->base;
-		offset = IPROC_GPIO_REG(gpio,
-					 IPROC_GPIO_ASIU_DRV0_CTRL_OFFSET);
 	}
 
 	shift = IPROC_GPIO_SHIFT(gpio);
@@ -553,10 +603,10 @@ static int iproc_gpio_get_strength(struct iproc_gpio *chip, unsigned gpio,
 	raw_spin_lock_irqsave(&chip->lock, flags);
 	*strength = 0;
 	for (i = 0; i < GPIO_DRV_STRENGTH_BITS; i++) {
+		offset = DRV_STRENGTH_OFFSET(gpio, i, chip->io_ctrl_type);
 		val = readl(base + offset) & BIT(shift);
 		val >>= shift;
 		*strength += (val << i);
-		offset += 4;
 	}
 
 	/* convert to mA */
@@ -734,6 +784,7 @@ static int iproc_gpio_probe(struct platform_device *pdev)
 	u32 ngpios, pinconf_disable_mask = 0;
 	int irq, ret;
 	bool no_pinconf = false;
+	enum iproc_pinconf_ctrl_type io_ctrl_type = IOCTRL_TYPE_INVALID;
 
 	/* NSP does not support drive strength config */
 	if (of_device_is_compatible(dev->of_node, "brcm,iproc-nsp-gpio"))
@@ -764,8 +815,15 @@ static int iproc_gpio_probe(struct platform_device *pdev)
 			dev_err(dev, "unable to map I/O memory\n");
 			return PTR_ERR(chip->io_ctrl);
 		}
+		if (of_device_is_compatible(dev->of_node,
+					    "brcm,cygnus-ccm-gpio"))
+			io_ctrl_type = IOCTRL_TYPE_CDRU;
+		else
+			io_ctrl_type = IOCTRL_TYPE_AON;
 	}
 
+	chip->io_ctrl_type = io_ctrl_type;
+
 	if (of_property_read_u32(dev->of_node, "ngpios", &ngpios)) {
 		dev_err(&pdev->dev, "missing ngpios DT property\n");
 		return -ENODEV;
-- 
2.7.4

[PATCH 2/2] gpio: iproc-gpio: Handle interrupts for multiple instances

[Srinath Mannam]

From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

When multiple instance of iproc-gpio chips are present, a fix up
message[1] is printed during the probe of second and later instances.

This issue is because driver sharing same irq_chip data structure
among multiple instances of driver.

Fix this by allocating irq_chip data structure per instance of
iproc-gpio.

[1] fix up message addressed by this patch
[  7.862208] gpio gpiochip2: (689d0000.gpio): detected irqchip that
   is shared with multiple gpiochips: please fix the driver.

Fixes: 616043d58a89 ("pinctrl: Rename gpio driver from cygnus to iproc")
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 drivers/pinctrl/bcm/pinctrl-iproc-gpio.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
index 20b9864..8729f47 100644
--- a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
@@ -114,6 +114,7 @@ struct iproc_gpio {
 
 	raw_spinlock_t lock;
 
+	struct irq_chip irqchip;
 	struct gpio_chip gc;
 	unsigned num_banks;
 
@@ -302,14 +303,6 @@ static int iproc_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
-static struct irq_chip iproc_gpio_irq_chip = {
-	.name = "bcm-iproc-gpio",
-	.irq_ack = iproc_gpio_irq_ack,
-	.irq_mask = iproc_gpio_irq_mask,
-	.irq_unmask = iproc_gpio_irq_unmask,
-	.irq_set_type = iproc_gpio_irq_set_type,
-};
-
 /*
  * Request the Iproc IOMUX pinmux controller to mux individual pins to GPIO
  */
@@ -875,14 +868,22 @@ static int iproc_gpio_probe(struct platform_device *pdev)
 	/* optional GPIO interrupt support */
 	irq = platform_get_irq(pdev, 0);
 	if (irq) {
-		ret = gpiochip_irqchip_add(gc, &iproc_gpio_irq_chip, 0,
+		chip->irqchip.name = "bcm-iproc-gpio";
+		chip->irqchip.irq_ack = iproc_gpio_irq_ack;
+		chip->irqchip.irq_mask = iproc_gpio_irq_mask;
+		chip->irqchip.irq_unmask = iproc_gpio_irq_unmask;
+		chip->irqchip.irq_set_type = iproc_gpio_irq_set_type;
+		chip->irqchip.irq_enable = iproc_gpio_irq_unmask;
+		chip->irqchip.irq_disable = iproc_gpio_irq_mask;
+
+		ret = gpiochip_irqchip_add(gc, &chip->irqchip, 0,
 					   handle_simple_irq, IRQ_TYPE_NONE);
 		if (ret) {
 			dev_err(dev, "no GPIO irqchip\n");
 			goto err_rm_gpiochip;
 		}
 
-		gpiochip_set_chained_irqchip(gc, &iproc_gpio_irq_chip, irq,
+		gpiochip_set_chained_irqchip(gc, &chip->irqchip, irq,
 					     iproc_gpio_irq_handler);
 	}
 
-- 
2.7.4

Re: [PATCH 1/2] gpio: iproc-gpio: Fix incorrect pinconf configurations

[Linus Walleij]

On Thu, Aug 29, 2019 at 5:52 AM Srinath Mannam
<srinath.mannam@broadcom.com> wrote:

> From: Li Jin <li.jin@broadcom.com>
>
> Fix drive strength for AON/CRMU controller; fix pull-up/down setting
> for CCM/CDRU controller.
>
> Fixes: 616043d58a89 ("pinctrl: Rename gpio driver from cygnus to iproc")
> Signed-off-by: Li Jin <li.jin@broadcom.com>

No response from maintainers for two weeks, so patch applied.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: iproc-gpio: Handle interrupts for multiple instances

[Linus Walleij]

On Thu, Aug 29, 2019 at 5:52 AM Srinath Mannam
<srinath.mannam@broadcom.com> wrote:

> From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>
> When multiple instance of iproc-gpio chips are present, a fix up
> message[1] is printed during the probe of second and later instances.
>
> This issue is because driver sharing same irq_chip data structure
> among multiple instances of driver.
>
> Fix this by allocating irq_chip data structure per instance of
> iproc-gpio.
>
> [1] fix up message addressed by this patch
> [  7.862208] gpio gpiochip2: (689d0000.gpio): detected irqchip that
>    is shared with multiple gpiochips: please fix the driver.
>
> Fixes: 616043d58a89 ("pinctrl: Rename gpio driver from cygnus to iproc")
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

Patch applied, I had to rewrite it a bit to fit the new code that
set up the irqchip when adding the gpio_chip, please check that
the result works.

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: iproc-gpio: Fix incorrect pinconf configurations

[Ray Jui]

On 9/11/19 2:34 AM, Linus Walleij wrote:
> On Thu, Aug 29, 2019 at 5:52 AM Srinath Mannam
> <srinath.mannam@broadcom.com> wrote:
> 
>> From: Li Jin <li.jin@broadcom.com>
>>
>> Fix drive strength for AON/CRMU controller; fix pull-up/down setting
>> for CCM/CDRU controller.
>>
>> Fixes: 616043d58a89 ("pinctrl: Rename gpio driver from cygnus to iproc")
>> Signed-off-by: Li Jin <li.jin@broadcom.com>
> 
> No response from maintainers for two weeks, so patch applied.
> 

These patches were actually all internally reviewed by Broadcom 
maintainers before sending out to the mailing list.

Obviously you wouldn't know about that, :)

One of us should have explicitly given our ACK, sorry...

Thanks,

Ray

> Yours,
> Linus Walleij
> 

Re: [PATCH 1/2] gpio: iproc-gpio: Fix incorrect pinconf configurations

[Linus Walleij]

On Wed, Sep 11, 2019 at 5:55 PM Ray Jui <ray.jui@broadcom.com> wrote:

> These patches were actually all internally reviewed by Broadcom
> maintainers before sending out to the mailing list.
>
> Obviously you wouldn't know about that, :)
>
> One of us should have explicitly given our ACK, sorry...

It's fine, the process is not perfect.

Thanks!
Linus Walleij

Re: [PATCH 2/2] gpio: iproc-gpio: Handle interrupts for multiple instances

[Srinath Mannam]

Hi Linus,

We have tested patch with your changes, it works fine.
Thanks a lot for all the help.

Regards,
Srinath.

On Wed, Sep 11, 2019 at 3:13 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Aug 29, 2019 at 5:52 AM Srinath Mannam
> <srinath.mannam@broadcom.com> wrote:
>
> > From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> >
> > When multiple instance of iproc-gpio chips are present, a fix up
> > message[1] is printed during the probe of second and later instances.
> >
> > This issue is because driver sharing same irq_chip data structure
> > among multiple instances of driver.
> >
> > Fix this by allocating irq_chip data structure per instance of
> > iproc-gpio.
> >
> > [1] fix up message addressed by this patch
> > [  7.862208] gpio gpiochip2: (689d0000.gpio): detected irqchip that
> >    is shared with multiple gpiochips: please fix the driver.
> >
> > Fixes: 616043d58a89 ("pinctrl: Rename gpio driver from cygnus to iproc")
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>
> Patch applied, I had to rewrite it a bit to fit the new code that
> set up the irqchip when adding the gpio_chip, please check that
> the result works.
>
> Yours,
> Linus Walleij