[PATCH v2 0/4] gpio: aspeed: Fixes and support for sharing with co-processor

[PATCH v2 0/4] gpio: aspeed: Fixes and support for sharing with co-processor

[Benjamin Herrenschmidt]

This series cleans up the register accessors a bit, adds missing ones,
fixes the access to the write latch and finally adds an interface
that a co-processor driver can use to change the owner of some of
the GPIO lines and arbitrate access to shared GPIO banks.

v2: - Address Joel comments

NOTE: I haven't changed the inclusion of gpio/consumer.h and gpiolib.h
      as, according to my replies, I do actually need them. I'm happy
      to use an alternative if somebody can suggest a reasonable one.

[PATCH v2 1/4] gpio: aspeed: Rework register type accessors

[Benjamin Herrenschmidt]

Use a single accessor function for all register types instead
of several spread around. This will make it easier/cleaner
to introduce new registers and keep the mechanism in one
place.

The big switch/case is optimized at compile time since the
switch value is a constant.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/gpio/gpio-aspeed.c | 118 ++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 49 deletions(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index b31ae16170e7..c9baeeb7f0cc 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -127,12 +127,21 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	},
 };
 
-#define GPIO_BANK(x)	((x) >> 5)
-#define GPIO_OFFSET(x)	((x) & 0x1f)
-#define GPIO_BIT(x)	BIT(GPIO_OFFSET(x))
+enum aspeed_gpio_reg {
+	reg_val,
+	reg_dir,
+	reg_irq_enable,
+	reg_irq_type0,
+	reg_irq_type1,
+	reg_irq_type2,
+	reg_irq_status,
+	reg_debounce_sel1,
+	reg_debounce_sel2,
+	reg_tolerance,
+};
 
-#define GPIO_DATA	0x00
-#define GPIO_DIR	0x04
+#define GPIO_VAL_VALUE	0x00
+#define GPIO_VAL_DIR	0x04
 
 #define GPIO_IRQ_ENABLE	0x00
 #define GPIO_IRQ_TYPE0	0x04
@@ -143,6 +152,40 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 #define GPIO_DEBOUNCE_SEL1 0x00
 #define GPIO_DEBOUNCE_SEL2 0x04
 
+/* This will be resolved at compile time */
+static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
+				     const struct aspeed_gpio_bank *bank,
+				     const enum aspeed_gpio_reg reg)
+{
+	switch (reg) {
+	case reg_val:
+		return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
+	case reg_dir:
+		return gpio->base + bank->val_regs + GPIO_VAL_DIR;
+	case reg_irq_enable:
+		return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
+	case reg_irq_type0:
+		return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
+	case reg_irq_type1:
+		return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
+	case reg_irq_type2:
+		return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
+	case reg_irq_status:
+		return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
+	case reg_debounce_sel1:
+		return gpio->base + bank->debounce_regs + GPIO_DEBOUNCE_SEL1;
+	case reg_debounce_sel2:
+		return gpio->base + bank->debounce_regs + GPIO_DEBOUNCE_SEL2;
+	case reg_tolerance:
+		return gpio->base + bank->tolerance_regs;
+	}
+	BUG_ON(1);
+}
+
+#define GPIO_BANK(x)	((x) >> 5)
+#define GPIO_OFFSET(x)	((x) & 0x1f)
+#define GPIO_BIT(x)	BIT(GPIO_OFFSET(x))
+
 #define _GPIO_SET_DEBOUNCE(t, o, i) ((!!((t) & BIT(i))) << GPIO_OFFSET(o))
 #define GPIO_SET_DEBOUNCE1(t, o) _GPIO_SET_DEBOUNCE(t, o, 1)
 #define GPIO_SET_DEBOUNCE2(t, o) _GPIO_SET_DEBOUNCE(t, o, 0)
@@ -201,27 +244,12 @@ static inline bool have_output(struct aspeed_gpio *gpio, unsigned int offset)
 	return !props || (props->output & GPIO_BIT(offset));
 }
 
-static void __iomem *bank_val_reg(struct aspeed_gpio *gpio,
-		const struct aspeed_gpio_bank *bank,
-		unsigned int reg)
-{
-	return gpio->base + bank->val_regs + reg;
-}
-
-static void __iomem *bank_irq_reg(struct aspeed_gpio *gpio,
-		const struct aspeed_gpio_bank *bank,
-		unsigned int reg)
-{
-	return gpio->base + bank->irq_regs + reg;
-}
-
 static int aspeed_gpio_get(struct gpio_chip *gc, unsigned int offset)
 {
 	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
 	const struct aspeed_gpio_bank *bank = to_bank(offset);
 
-	return !!(ioread32(bank_val_reg(gpio, bank, GPIO_DATA))
-			& GPIO_BIT(offset));
+	return !!(ioread32(bank_reg(gpio, bank, reg_val)) & GPIO_BIT(offset));
 }
 
 static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
@@ -232,7 +260,7 @@ static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
 	void __iomem *addr;
 	u32 reg;
 
-	addr = bank_val_reg(gpio, bank, GPIO_DATA);
+	addr = bank_reg(gpio, bank, reg_val);
 	reg = gpio->dcache[GPIO_BANK(offset)];
 
 	if (val)
@@ -269,8 +297,8 @@ static int aspeed_gpio_dir_in(struct gpio_chip *gc, unsigned int offset)
 
 	spin_lock_irqsave(&gpio->lock, flags);
 
-	reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR));
-	iowrite32(reg & ~GPIO_BIT(offset), bank_val_reg(gpio, bank, GPIO_DIR));
+	reg = ioread32(bank_reg(gpio, bank, reg_dir));
+	iowrite32(reg & ~GPIO_BIT(offset), bank_reg(gpio, bank, reg_dir));
 
 	spin_unlock_irqrestore(&gpio->lock, flags);
 
@@ -291,8 +319,8 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
 	spin_lock_irqsave(&gpio->lock, flags);
 
 	__aspeed_gpio_set(gc, offset, val);
-	reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR));
-	iowrite32(reg | GPIO_BIT(offset), bank_val_reg(gpio, bank, GPIO_DIR));
+	reg = ioread32(bank_reg(gpio, bank, reg_dir));
+	iowrite32(reg | GPIO_BIT(offset), bank_reg(gpio, bank, reg_dir));
 
 	spin_unlock_irqrestore(&gpio->lock, flags);
 
@@ -314,7 +342,7 @@ static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
 
 	spin_lock_irqsave(&gpio->lock, flags);
 
-	val = ioread32(bank_val_reg(gpio, bank, GPIO_DIR)) & GPIO_BIT(offset);
+	val = ioread32(bank_reg(gpio, bank, reg_dir)) & GPIO_BIT(offset);
 
 	spin_unlock_irqrestore(&gpio->lock, flags);
 
@@ -358,7 +386,7 @@ static void aspeed_gpio_irq_ack(struct irq_data *d)
 	if (rc)
 		return;
 
-	status_addr = bank_irq_reg(gpio, bank, GPIO_IRQ_STATUS);
+	status_addr = bank_reg(gpio, bank, reg_irq_status);
 
 	spin_lock_irqsave(&gpio->lock, flags);
 	iowrite32(bit, status_addr);
@@ -378,7 +406,7 @@ static void aspeed_gpio_irq_set_mask(struct irq_data *d, bool set)
 	if (rc)
 		return;
 
-	addr = bank_irq_reg(gpio, bank, GPIO_IRQ_ENABLE);
+	addr = bank_reg(gpio, bank, reg_irq_enable);
 
 	spin_lock_irqsave(&gpio->lock, flags);
 
@@ -442,17 +470,17 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
 
 	spin_lock_irqsave(&gpio->lock, flags);
 
-	addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE0);
+	addr = bank_reg(gpio, bank, reg_irq_type0);
 	reg = ioread32(addr);
 	reg = (reg & ~bit) | type0;
 	iowrite32(reg, addr);
 
-	addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE1);
+	addr = bank_reg(gpio, bank, reg_irq_type1);
 	reg = ioread32(addr);
 	reg = (reg & ~bit) | type1;
 	iowrite32(reg, addr);
 
-	addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE2);
+	addr = bank_reg(gpio, bank, reg_irq_type2);
 	reg = ioread32(addr);
 	reg = (reg & ~bit) | type2;
 	iowrite32(reg, addr);
@@ -477,7 +505,7 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
 	for (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) {
 		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
 
-		reg = ioread32(bank_irq_reg(data, bank, GPIO_IRQ_STATUS));
+		reg = ioread32(bank_reg(data, bank, reg_irq_status));
 
 		for_each_set_bit(p, &reg, 32) {
 			girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
@@ -549,21 +577,21 @@ static int aspeed_gpio_reset_tolerance(struct gpio_chip *chip,
 					unsigned int offset, bool enable)
 {
 	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
-	const struct aspeed_gpio_bank *bank;
 	unsigned long flags;
+	void __iomem *treg;
 	u32 val;
 
-	bank = to_bank(offset);
+	treg = bank_reg(gpio, to_bank(offset), reg_tolerance);
 
 	spin_lock_irqsave(&gpio->lock, flags);
-	val = readl(gpio->base + bank->tolerance_regs);
+	val = readl(treg);
 
 	if (enable)
 		val |= GPIO_BIT(offset);
 	else
 		val &= ~GPIO_BIT(offset);
 
-	writel(val, gpio->base + bank->tolerance_regs);
+	writel(val, treg);
 	spin_unlock_irqrestore(&gpio->lock, flags);
 
 	return 0;
@@ -582,13 +610,6 @@ static void aspeed_gpio_free(struct gpio_chip *chip, unsigned int offset)
 	pinctrl_gpio_free(chip->base + offset);
 }
 
-static inline void __iomem *bank_debounce_reg(struct aspeed_gpio *gpio,
-		const struct aspeed_gpio_bank *bank,
-		unsigned int reg)
-{
-	return gpio->base + bank->debounce_regs + reg;
-}
-
 static int usecs_to_cycles(struct aspeed_gpio *gpio, unsigned long usecs,
 		u32 *cycles)
 {
@@ -666,11 +687,11 @@ static void configure_timer(struct aspeed_gpio *gpio, unsigned int offset,
 	void __iomem *addr;
 	u32 val;
 
-	addr = bank_debounce_reg(gpio, bank, GPIO_DEBOUNCE_SEL1);
+	addr = bank_reg(gpio, bank, reg_debounce_sel1);
 	val = ioread32(addr);
 	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE1(timer, offset), addr);
 
-	addr = bank_debounce_reg(gpio, bank, GPIO_DEBOUNCE_SEL2);
+	addr = bank_reg(gpio, bank, reg_debounce_sel2);
 	val = ioread32(addr);
 	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE2(timer, offset), addr);
 }
@@ -904,9 +925,8 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
 
 	/* Populate it with initial values read from the HW */
 	for (i = 0; i < banks; i++) {
-		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
-		gpio->dcache[i] = ioread32(gpio->base + bank->val_regs +
-					   GPIO_DATA);
+		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_val);
+		gpio->dcache[i] = ioread32(addr);
 	}
 
 	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
-- 
2.17.1

[PATCH v2 1/4] gpio: aspeed: Rework register type accessors

[Joel Stanley]

On 18 June 2018 at 14:23, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> Use a single accessor function for all register types instead
> of several spread around. This will make it easier/cleaner
> to introduce new registers and keep the mechanism in one
> place.
>
> The big switch/case is optimized at compile time since the
> switch value is a constant.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

[PATCH v2 1/4] gpio: aspeed: Rework register type accessors

[Andrew Jeffery]

On Mon, 18 Jun 2018, at 14:23, Benjamin Herrenschmidt wrote:
> Use a single accessor function for all register types instead
> of several spread around. This will make it easier/cleaner
> to introduce new registers and keep the mechanism in one
> place.
> 
> The big switch/case is optimized at compile time since the
> switch value is a constant.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/gpio/gpio-aspeed.c | 118 ++++++++++++++++++++++---------------
>  1 file changed, 69 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index b31ae16170e7..c9baeeb7f0cc 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -127,12 +127,21 @@ static const struct aspeed_gpio_bank 
> aspeed_gpio_banks[] = {
>  	},
>  };
>  
> -#define GPIO_BANK(x)	((x) >> 5)
> -#define GPIO_OFFSET(x)	((x) & 0x1f)
> -#define GPIO_BIT(x)	BIT(GPIO_OFFSET(x))
> +enum aspeed_gpio_reg {
> +	reg_val,
> +	reg_dir,
> +	reg_irq_enable,
> +	reg_irq_type0,
> +	reg_irq_type1,
> +	reg_irq_type2,
> +	reg_irq_status,
> +	reg_debounce_sel1,
> +	reg_debounce_sel2,
> +	reg_tolerance,
> +};
>  
> -#define GPIO_DATA	0x00
> -#define GPIO_DIR	0x04
> +#define GPIO_VAL_VALUE	0x00
> +#define GPIO_VAL_DIR	0x04
>  
>  #define GPIO_IRQ_ENABLE	0x00
>  #define GPIO_IRQ_TYPE0	0x04
> @@ -143,6 +152,40 @@ static const struct aspeed_gpio_bank 
> aspeed_gpio_banks[] = {
>  #define GPIO_DEBOUNCE_SEL1 0x00
>  #define GPIO_DEBOUNCE_SEL2 0x04
>  
> +/* This will be resolved at compile time */
> +static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
> +				     const struct aspeed_gpio_bank *bank,
> +				     const enum aspeed_gpio_reg reg)
> +{
> +	switch (reg) {
> +	case reg_val:
> +		return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> +	case reg_dir:
> +		return gpio->base + bank->val_regs + GPIO_VAL_DIR;
> +	case reg_irq_enable:
> +		return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> +	case reg_irq_type0:
> +		return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> +	case reg_irq_type1:
> +		return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> +	case reg_irq_type2:
> +		return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> +	case reg_irq_status:
> +		return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> +	case reg_debounce_sel1:
> +		return gpio->base + bank->debounce_regs + GPIO_DEBOUNCE_SEL1;
> +	case reg_debounce_sel2:
> +		return gpio->base + bank->debounce_regs + GPIO_DEBOUNCE_SEL2;
> +	case reg_tolerance:
> +		return gpio->base + bank->tolerance_regs;
> +	}
> +	BUG_ON(1);
> +}
> +
> +#define GPIO_BANK(x)	((x) >> 5)
> +#define GPIO_OFFSET(x)	((x) & 0x1f)
> +#define GPIO_BIT(x)	BIT(GPIO_OFFSET(x))
> +
>  #define _GPIO_SET_DEBOUNCE(t, o, i) ((!!((t) & BIT(i))) << 
> GPIO_OFFSET(o))
>  #define GPIO_SET_DEBOUNCE1(t, o) _GPIO_SET_DEBOUNCE(t, o, 1)
>  #define GPIO_SET_DEBOUNCE2(t, o) _GPIO_SET_DEBOUNCE(t, o, 0)
> @@ -201,27 +244,12 @@ static inline bool have_output(struct aspeed_gpio 
> *gpio, unsigned int offset)
>  	return !props || (props->output & GPIO_BIT(offset));
>  }
>  
> -static void __iomem *bank_val_reg(struct aspeed_gpio *gpio,
> -		const struct aspeed_gpio_bank *bank,
> -		unsigned int reg)
> -{
> -	return gpio->base + bank->val_regs + reg;
> -}
> -
> -static void __iomem *bank_irq_reg(struct aspeed_gpio *gpio,
> -		const struct aspeed_gpio_bank *bank,
> -		unsigned int reg)
> -{
> -	return gpio->base + bank->irq_regs + reg;
> -}
> -
>  static int aspeed_gpio_get(struct gpio_chip *gc, unsigned int offset)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
>  	const struct aspeed_gpio_bank *bank = to_bank(offset);
>  
> -	return !!(ioread32(bank_val_reg(gpio, bank, GPIO_DATA))
> -			& GPIO_BIT(offset));
> +	return !!(ioread32(bank_reg(gpio, bank, reg_val)) & GPIO_BIT(offset));
>  }
>  
>  static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int 
> offset,
> @@ -232,7 +260,7 @@ static void __aspeed_gpio_set(struct gpio_chip *gc, 
> unsigned int offset,
>  	void __iomem *addr;
>  	u32 reg;
>  
> -	addr = bank_val_reg(gpio, bank, GPIO_DATA);
> +	addr = bank_reg(gpio, bank, reg_val);
>  	reg = gpio->dcache[GPIO_BANK(offset)];
>  
>  	if (val)
> @@ -269,8 +297,8 @@ static int aspeed_gpio_dir_in(struct gpio_chip *gc, 
> unsigned int offset)
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
>  
> -	reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR));
> -	iowrite32(reg & ~GPIO_BIT(offset), bank_val_reg(gpio, bank, GPIO_DIR));
> +	reg = ioread32(bank_reg(gpio, bank, reg_dir));
> +	iowrite32(reg & ~GPIO_BIT(offset), bank_reg(gpio, bank, reg_dir));
>  
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  
> @@ -291,8 +319,8 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
>  	spin_lock_irqsave(&gpio->lock, flags);
>  
>  	__aspeed_gpio_set(gc, offset, val);
> -	reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR));
> -	iowrite32(reg | GPIO_BIT(offset), bank_val_reg(gpio, bank, GPIO_DIR));
> +	reg = ioread32(bank_reg(gpio, bank, reg_dir));
> +	iowrite32(reg | GPIO_BIT(offset), bank_reg(gpio, bank, reg_dir));
>  
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  
> @@ -314,7 +342,7 @@ static int aspeed_gpio_get_direction(struct 
> gpio_chip *gc, unsigned int offset)
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
>  
> -	val = ioread32(bank_val_reg(gpio, bank, GPIO_DIR)) & GPIO_BIT(offset);
> +	val = ioread32(bank_reg(gpio, bank, reg_dir)) & GPIO_BIT(offset);
>  
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  
> @@ -358,7 +386,7 @@ static void aspeed_gpio_irq_ack(struct irq_data *d)
>  	if (rc)
>  		return;
>  
> -	status_addr = bank_irq_reg(gpio, bank, GPIO_IRQ_STATUS);
> +	status_addr = bank_reg(gpio, bank, reg_irq_status);
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
>  	iowrite32(bit, status_addr);
> @@ -378,7 +406,7 @@ static void aspeed_gpio_irq_set_mask(struct irq_data 
> *d, bool set)
>  	if (rc)
>  		return;
>  
> -	addr = bank_irq_reg(gpio, bank, GPIO_IRQ_ENABLE);
> +	addr = bank_reg(gpio, bank, reg_irq_enable);
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
>  
> @@ -442,17 +470,17 @@ static int aspeed_gpio_set_type(struct irq_data 
> *d, unsigned int type)
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
>  
> -	addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE0);
> +	addr = bank_reg(gpio, bank, reg_irq_type0);
>  	reg = ioread32(addr);
>  	reg = (reg & ~bit) | type0;
>  	iowrite32(reg, addr);
>  
> -	addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE1);
> +	addr = bank_reg(gpio, bank, reg_irq_type1);
>  	reg = ioread32(addr);
>  	reg = (reg & ~bit) | type1;
>  	iowrite32(reg, addr);
>  
> -	addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE2);
> +	addr = bank_reg(gpio, bank, reg_irq_type2);
>  	reg = ioread32(addr);
>  	reg = (reg & ~bit) | type2;
>  	iowrite32(reg, addr);
> @@ -477,7 +505,7 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
>  	for (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) {
>  		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
>  
> -		reg = ioread32(bank_irq_reg(data, bank, GPIO_IRQ_STATUS));
> +		reg = ioread32(bank_reg(data, bank, reg_irq_status));
>  
>  		for_each_set_bit(p, &reg, 32) {
>  			girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> @@ -549,21 +577,21 @@ static int aspeed_gpio_reset_tolerance(struct 
> gpio_chip *chip,
>  					unsigned int offset, bool enable)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
> -	const struct aspeed_gpio_bank *bank;
>  	unsigned long flags;
> +	void __iomem *treg;
>  	u32 val;
>  
> -	bank = to_bank(offset);
> +	treg = bank_reg(gpio, to_bank(offset), reg_tolerance);
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
> -	val = readl(gpio->base + bank->tolerance_regs);
> +	val = readl(treg);
>  
>  	if (enable)
>  		val |= GPIO_BIT(offset);
>  	else
>  		val &= ~GPIO_BIT(offset);
>  
> -	writel(val, gpio->base + bank->tolerance_regs);
> +	writel(val, treg);
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  
>  	return 0;
> @@ -582,13 +610,6 @@ static void aspeed_gpio_free(struct gpio_chip 
> *chip, unsigned int offset)
>  	pinctrl_gpio_free(chip->base + offset);
>  }
>  
> -static inline void __iomem *bank_debounce_reg(struct aspeed_gpio *gpio,
> -		const struct aspeed_gpio_bank *bank,
> -		unsigned int reg)
> -{
> -	return gpio->base + bank->debounce_regs + reg;
> -}
> -
>  static int usecs_to_cycles(struct aspeed_gpio *gpio, unsigned long 
> usecs,
>  		u32 *cycles)
>  {
> @@ -666,11 +687,11 @@ static void configure_timer(struct aspeed_gpio 
> *gpio, unsigned int offset,
>  	void __iomem *addr;
>  	u32 val;
>  
> -	addr = bank_debounce_reg(gpio, bank, GPIO_DEBOUNCE_SEL1);
> +	addr = bank_reg(gpio, bank, reg_debounce_sel1);
>  	val = ioread32(addr);
>  	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE1(timer, offset), addr);
>  
> -	addr = bank_debounce_reg(gpio, bank, GPIO_DEBOUNCE_SEL2);
> +	addr = bank_reg(gpio, bank, reg_debounce_sel2);
>  	val = ioread32(addr);
>  	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE2(timer, offset), addr);
>  }
> @@ -904,9 +925,8 @@ static int __init aspeed_gpio_probe(struct 
> platform_device *pdev)
>  
>  	/* Populate it with initial values read from the HW */
>  	for (i = 0; i < banks; i++) {
> -		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
> -		gpio->dcache[i] = ioread32(gpio->base + bank->val_regs +
> -					   GPIO_DATA);
> +		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_val);
> +		gpio->dcache[i] = ioread32(addr);
>  	}
>  
>  	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> -- 
> 2.17.1
> 

[PATCH v2 2/4] gpio: aspeed: Add "Read Data" register to read the write latch

[Benjamin Herrenschmidt]

The Aspeed GPIO hardware has a quirk: the value register, for an
output GPIO, doesn't contain the last value written (the write
latch content) but the sampled input value.

This means that when reading back shortly after writing, you can
get an incorrect value as the input value is delayed by a few
synchronizers.

The HW supports a separate read-only register "Data Read Register"
which allows you to read the write latch instead.

This adds the definition for it, and uses it for the initial
population of the GPIO value cache. It will be used more in
subsequent patches.
can be

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/gpio/gpio-aspeed.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index c9baeeb7f0cc..a5ded50c6db0 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -59,18 +59,33 @@ struct aspeed_gpio {
 };
 
 struct aspeed_gpio_bank {
-	uint16_t	val_regs;
+	uint16_t	val_regs;	/* +0: Rd: read input value, Wr: set write latch
+					 * +4: Rd/Wr: Direction (0=in, 1=out)
+					 */
+	uint16_t	rdata_reg;	/*     Rd: read write latch, Wr: <none>  */
 	uint16_t	irq_regs;
 	uint16_t	debounce_regs;
 	uint16_t	tolerance_regs;
 	const char	names[4][3];
 };
 
+/*
+ * Note: The "value" register returns the input value sampled on the
+ *       line even when the GPIO is configured as an output. Since
+ *       that input goes through synchronizers, writing, then reading
+ *       back may not return the written value right away.
+ *
+ *       The "rdata" register returns the content of the write latch
+ *       and thus can be used to read back what was last written
+ *       reliably.
+ */
+
 static const int debounce_timers[4] = { 0x00, 0x50, 0x54, 0x58 };
 
 static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	{
 		.val_regs = 0x0000,
+		.rdata_reg = 0x00c0,
 		.irq_regs = 0x0008,
 		.debounce_regs = 0x0040,
 		.tolerance_regs = 0x001c,
@@ -78,6 +93,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	},
 	{
 		.val_regs = 0x0020,
+		.rdata_reg = 0x00c4,
 		.irq_regs = 0x0028,
 		.debounce_regs = 0x0048,
 		.tolerance_regs = 0x003c,
@@ -85,6 +101,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	},
 	{
 		.val_regs = 0x0070,
+		.rdata_reg = 0x00c8,
 		.irq_regs = 0x0098,
 		.debounce_regs = 0x00b0,
 		.tolerance_regs = 0x00ac,
@@ -92,6 +109,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	},
 	{
 		.val_regs = 0x0078,
+		.rdata_reg = 0x00cc,
 		.irq_regs = 0x00e8,
 		.debounce_regs = 0x0100,
 		.tolerance_regs = 0x00fc,
@@ -99,6 +117,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	},
 	{
 		.val_regs = 0x0080,
+		.rdata_reg = 0x00d0,
 		.irq_regs = 0x0118,
 		.debounce_regs = 0x0130,
 		.tolerance_regs = 0x012c,
@@ -106,6 +125,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	},
 	{
 		.val_regs = 0x0088,
+		.rdata_reg = 0x00d4,
 		.irq_regs = 0x0148,
 		.debounce_regs = 0x0160,
 		.tolerance_regs = 0x015c,
@@ -113,6 +133,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	},
 	{
 		.val_regs = 0x01E0,
+		.rdata_reg = 0x00d8,
 		.irq_regs = 0x0178,
 		.debounce_regs = 0x0190,
 		.tolerance_regs = 0x018c,
@@ -120,6 +141,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	},
 	{
 		.val_regs = 0x01e8,
+		.rdata_reg = 0x00dc,
 		.irq_regs = 0x01a8,
 		.debounce_regs = 0x01c0,
 		.tolerance_regs = 0x01bc,
@@ -129,6 +151,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 
 enum aspeed_gpio_reg {
 	reg_val,
+	reg_rdata,
 	reg_dir,
 	reg_irq_enable,
 	reg_irq_type0,
@@ -160,6 +183,8 @@ static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
 	switch (reg) {
 	case reg_val:
 		return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
+	case reg_rdata:
+		return gpio->base + bank->rdata_reg;
 	case reg_dir:
 		return gpio->base + bank->val_regs + GPIO_VAL_DIR;
 	case reg_irq_enable:
@@ -925,7 +950,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
 
 	/* Populate it with initial values read from the HW */
 	for (i = 0; i < banks; i++) {
-		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_val);
+		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_rdata);
 		gpio->dcache[i] = ioread32(addr);
 	}
 
-- 
2.17.1

[PATCH v2 2/4] gpio: aspeed: Add "Read Data" register to read the write latch

[Joel Stanley]

On 18 June 2018 at 14:23, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> The Aspeed GPIO hardware has a quirk: the value register, for an
> output GPIO, doesn't contain the last value written (the write
> latch content) but the sampled input value.
>
> This means that when reading back shortly after writing, you can
> get an incorrect value as the input value is delayed by a few
> synchronizers.
>
> The HW supports a separate read-only register "Data Read Register"
> which allows you to read the write latch instead.
>
> This adds the definition for it, and uses it for the initial
> population of the GPIO value cache. It will be used more in
> subsequent patches.
> can be
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

[PATCH v2 2/4] gpio: aspeed: Add "Read Data" register to read the write latch

[Andrew Jeffery]

On Mon, 18 Jun 2018, at 14:23, Benjamin Herrenschmidt wrote:
> The Aspeed GPIO hardware has a quirk: the value register, for an
> output GPIO, doesn't contain the last value written (the write
> latch content) but the sampled input value.
> 
> This means that when reading back shortly after writing, you can
> get an incorrect value as the input value is delayed by a few
> synchronizers.
> 
> The HW supports a separate read-only register "Data Read Register"
> which allows you to read the write latch instead.
> 
> This adds the definition for it, and uses it for the initial
> population of the GPIO value cache. It will be used more in
> subsequent patches.
> can be
> 

Stray "can be"?

LGTM otherwise.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/gpio/gpio-aspeed.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index c9baeeb7f0cc..a5ded50c6db0 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -59,18 +59,33 @@ struct aspeed_gpio {
>  };
>  
>  struct aspeed_gpio_bank {
> -	uint16_t	val_regs;
> +	uint16_t	val_regs;	/* +0: Rd: read input value, Wr: set write latch
> +					 * +4: Rd/Wr: Direction (0=in, 1=out)
> +					 */
> +	uint16_t	rdata_reg;	/*     Rd: read write latch, Wr: <none>  */
>  	uint16_t	irq_regs;
>  	uint16_t	debounce_regs;
>  	uint16_t	tolerance_regs;
>  	const char	names[4][3];
>  };
>  
> +/*
> + * Note: The "value" register returns the input value sampled on the
> + *       line even when the GPIO is configured as an output. Since
> + *       that input goes through synchronizers, writing, then reading
> + *       back may not return the written value right away.
> + *
> + *       The "rdata" register returns the content of the write latch
> + *       and thus can be used to read back what was last written
> + *       reliably.
> + */
> +
>  static const int debounce_timers[4] = { 0x00, 0x50, 0x54, 0x58 };
>  
>  static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  	{
>  		.val_regs = 0x0000,
> +		.rdata_reg = 0x00c0,
>  		.irq_regs = 0x0008,
>  		.debounce_regs = 0x0040,
>  		.tolerance_regs = 0x001c,
> @@ -78,6 +93,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  	},
>  	{
>  		.val_regs = 0x0020,
> +		.rdata_reg = 0x00c4,
>  		.irq_regs = 0x0028,
>  		.debounce_regs = 0x0048,
>  		.tolerance_regs = 0x003c,
> @@ -85,6 +101,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  	},
>  	{
>  		.val_regs = 0x0070,
> +		.rdata_reg = 0x00c8,
>  		.irq_regs = 0x0098,
>  		.debounce_regs = 0x00b0,
>  		.tolerance_regs = 0x00ac,
> @@ -92,6 +109,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  	},
>  	{
>  		.val_regs = 0x0078,
> +		.rdata_reg = 0x00cc,
>  		.irq_regs = 0x00e8,
>  		.debounce_regs = 0x0100,
>  		.tolerance_regs = 0x00fc,
> @@ -99,6 +117,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  	},
>  	{
>  		.val_regs = 0x0080,
> +		.rdata_reg = 0x00d0,
>  		.irq_regs = 0x0118,
>  		.debounce_regs = 0x0130,
>  		.tolerance_regs = 0x012c,
> @@ -106,6 +125,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  	},
>  	{
>  		.val_regs = 0x0088,
> +		.rdata_reg = 0x00d4,
>  		.irq_regs = 0x0148,
>  		.debounce_regs = 0x0160,
>  		.tolerance_regs = 0x015c,
> @@ -113,6 +133,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  	},
>  	{
>  		.val_regs = 0x01E0,
> +		.rdata_reg = 0x00d8,
>  		.irq_regs = 0x0178,
>  		.debounce_regs = 0x0190,
>  		.tolerance_regs = 0x018c,
> @@ -120,6 +141,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  	},
>  	{
>  		.val_regs = 0x01e8,
> +		.rdata_reg = 0x00dc,
>  		.irq_regs = 0x01a8,
>  		.debounce_regs = 0x01c0,
>  		.tolerance_regs = 0x01bc,
> @@ -129,6 +151,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  
>  enum aspeed_gpio_reg {
>  	reg_val,
> +	reg_rdata,
>  	reg_dir,
>  	reg_irq_enable,
>  	reg_irq_type0,
> @@ -160,6 +183,8 @@ static inline void __iomem *bank_reg(struct 
> aspeed_gpio *gpio,
>  	switch (reg) {
>  	case reg_val:
>  		return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> +	case reg_rdata:
> +		return gpio->base + bank->rdata_reg;
>  	case reg_dir:
>  		return gpio->base + bank->val_regs + GPIO_VAL_DIR;
>  	case reg_irq_enable:
> @@ -925,7 +950,7 @@ static int __init aspeed_gpio_probe(struct 
> platform_device *pdev)
>  
>  	/* Populate it with initial values read from the HW */
>  	for (i = 0; i < banks; i++) {
> -		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_val);
> +		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_rdata);
>  		gpio->dcache[i] = ioread32(addr);
>  	}
>  
> -- 
> 2.17.1
> 

[PATCH v2 2/4] gpio: aspeed: Add "Read Data" register to read the write latch

[Benjamin Herrenschmidt]

On Mon, 2018-06-18 at 23:08 +0930, Andrew Jeffery wrote:
> On Mon, 18 Jun 2018, at 14:23, Benjamin Herrenschmidt wrote:
> > The Aspeed GPIO hardware has a quirk: the value register, for an
> > output GPIO, doesn't contain the last value written (the write
> > latch content) but the sampled input value.
> > 
> > This means that when reading back shortly after writing, you can
> > get an incorrect value as the input value is delayed by a few
> > synchronizers.
> > 
> > The HW supports a separate read-only register "Data Read Register"
> > which allows you to read the write latch instead.
> > 
> > This adds the definition for it, and uses it for the initial
> > population of the GPIO value cache. It will be used more in
> > subsequent patches.
> > can be
> > 
> 
> Stray "can be"?

heh yup, no idea where it comes from

> LGTM otherwise.

Ok. I'll wait for Linus W. reactions, and repost a hopefully final v3
with all the acks folded in.

> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >  drivers/gpio/gpio-aspeed.c | 29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> > index c9baeeb7f0cc..a5ded50c6db0 100644
> > --- a/drivers/gpio/gpio-aspeed.c
> > +++ b/drivers/gpio/gpio-aspeed.c
> > @@ -59,18 +59,33 @@ struct aspeed_gpio {
> >  };
> >  
> >  struct aspeed_gpio_bank {
> > -	uint16_t	val_regs;
> > +	uint16_t	val_regs;	/* +0: Rd: read input value, Wr: set write latch
> > +					 * +4: Rd/Wr: Direction (0=in, 1=out)
> > +					 */
> > +	uint16_t	rdata_reg;	/*     Rd: read write latch, Wr: <none>  */
> >  	uint16_t	irq_regs;
> >  	uint16_t	debounce_regs;
> >  	uint16_t	tolerance_regs;
> >  	const char	names[4][3];
> >  };
> >  
> > +/*
> > + * Note: The "value" register returns the input value sampled on the
> > + *       line even when the GPIO is configured as an output. Since
> > + *       that input goes through synchronizers, writing, then reading
> > + *       back may not return the written value right away.
> > + *
> > + *       The "rdata" register returns the content of the write latch
> > + *       and thus can be used to read back what was last written
> > + *       reliably.
> > + */
> > +
> >  static const int debounce_timers[4] = { 0x00, 0x50, 0x54, 0x58 };
> >  
> >  static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
> >  	{
> >  		.val_regs = 0x0000,
> > +		.rdata_reg = 0x00c0,
> >  		.irq_regs = 0x0008,
> >  		.debounce_regs = 0x0040,
> >  		.tolerance_regs = 0x001c,
> > @@ -78,6 +93,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
> >  	},
> >  	{
> >  		.val_regs = 0x0020,
> > +		.rdata_reg = 0x00c4,
> >  		.irq_regs = 0x0028,
> >  		.debounce_regs = 0x0048,
> >  		.tolerance_regs = 0x003c,
> > @@ -85,6 +101,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
> >  	},
> >  	{
> >  		.val_regs = 0x0070,
> > +		.rdata_reg = 0x00c8,
> >  		.irq_regs = 0x0098,
> >  		.debounce_regs = 0x00b0,
> >  		.tolerance_regs = 0x00ac,
> > @@ -92,6 +109,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
> >  	},
> >  	{
> >  		.val_regs = 0x0078,
> > +		.rdata_reg = 0x00cc,
> >  		.irq_regs = 0x00e8,
> >  		.debounce_regs = 0x0100,
> >  		.tolerance_regs = 0x00fc,
> > @@ -99,6 +117,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
> >  	},
> >  	{
> >  		.val_regs = 0x0080,
> > +		.rdata_reg = 0x00d0,
> >  		.irq_regs = 0x0118,
> >  		.debounce_regs = 0x0130,
> >  		.tolerance_regs = 0x012c,
> > @@ -106,6 +125,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
> >  	},
> >  	{
> >  		.val_regs = 0x0088,
> > +		.rdata_reg = 0x00d4,
> >  		.irq_regs = 0x0148,
> >  		.debounce_regs = 0x0160,
> >  		.tolerance_regs = 0x015c,
> > @@ -113,6 +133,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
> >  	},
> >  	{
> >  		.val_regs = 0x01E0,
> > +		.rdata_reg = 0x00d8,
> >  		.irq_regs = 0x0178,
> >  		.debounce_regs = 0x0190,
> >  		.tolerance_regs = 0x018c,
> > @@ -120,6 +141,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
> >  	},
> >  	{
> >  		.val_regs = 0x01e8,
> > +		.rdata_reg = 0x00dc,
> >  		.irq_regs = 0x01a8,
> >  		.debounce_regs = 0x01c0,
> >  		.tolerance_regs = 0x01bc,
> > @@ -129,6 +151,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
> >  
> >  enum aspeed_gpio_reg {
> >  	reg_val,
> > +	reg_rdata,
> >  	reg_dir,
> >  	reg_irq_enable,
> >  	reg_irq_type0,
> > @@ -160,6 +183,8 @@ static inline void __iomem *bank_reg(struct 
> > aspeed_gpio *gpio,
> >  	switch (reg) {
> >  	case reg_val:
> >  		return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> > +	case reg_rdata:
> > +		return gpio->base + bank->rdata_reg;
> >  	case reg_dir:
> >  		return gpio->base + bank->val_regs + GPIO_VAL_DIR;
> >  	case reg_irq_enable:
> > @@ -925,7 +950,7 @@ static int __init aspeed_gpio_probe(struct 
> > platform_device *pdev)
> >  
> >  	/* Populate it with initial values read from the HW */
> >  	for (i = 0; i < banks; i++) {
> > -		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_val);
> > +		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_rdata);
> >  		gpio->dcache[i] = ioread32(addr);
> >  	}
> >  
> > -- 
> > 2.17.1
> > 

[PATCH v2 3/4] gpio: aspeed: Add command source registers

[Benjamin Herrenschmidt]

This adds the definitions for the command source registers
and a helper to set them.

Those registers allow to control which bus master on the
SoC is allowed to modify a given bank of GPIOs and will
be used by subsequent patches.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/gpio/gpio-aspeed.c | 54 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index a5ded50c6db0..b3968f66b1d2 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -66,6 +66,7 @@ struct aspeed_gpio_bank {
 	uint16_t	irq_regs;
 	uint16_t	debounce_regs;
 	uint16_t	tolerance_regs;
+	uint16_t	cmdsrc_regs;
 	const char	names[4][3];
 };
 
@@ -89,6 +90,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.irq_regs = 0x0008,
 		.debounce_regs = 0x0040,
 		.tolerance_regs = 0x001c,
+		.cmdsrc_regs = 0x0060,
 		.names = { "A", "B", "C", "D" },
 	},
 	{
@@ -97,6 +99,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.irq_regs = 0x0028,
 		.debounce_regs = 0x0048,
 		.tolerance_regs = 0x003c,
+		.cmdsrc_regs = 0x0068,
 		.names = { "E", "F", "G", "H" },
 	},
 	{
@@ -105,6 +108,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.irq_regs = 0x0098,
 		.debounce_regs = 0x00b0,
 		.tolerance_regs = 0x00ac,
+		.cmdsrc_regs = 0x0090,
 		.names = { "I", "J", "K", "L" },
 	},
 	{
@@ -113,6 +117,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.irq_regs = 0x00e8,
 		.debounce_regs = 0x0100,
 		.tolerance_regs = 0x00fc,
+		.cmdsrc_regs = 0x00e0,
 		.names = { "M", "N", "O", "P" },
 	},
 	{
@@ -121,6 +126,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.irq_regs = 0x0118,
 		.debounce_regs = 0x0130,
 		.tolerance_regs = 0x012c,
+		.cmdsrc_regs = 0x0110,
 		.names = { "Q", "R", "S", "T" },
 	},
 	{
@@ -129,6 +135,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.irq_regs = 0x0148,
 		.debounce_regs = 0x0160,
 		.tolerance_regs = 0x015c,
+		.cmdsrc_regs = 0x0140,
 		.names = { "U", "V", "W", "X" },
 	},
 	{
@@ -137,6 +144,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.irq_regs = 0x0178,
 		.debounce_regs = 0x0190,
 		.tolerance_regs = 0x018c,
+		.cmdsrc_regs = 0x0170,
 		.names = { "Y", "Z", "AA", "AB" },
 	},
 	{
@@ -145,6 +153,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.irq_regs = 0x01a8,
 		.debounce_regs = 0x01c0,
 		.tolerance_regs = 0x01bc,
+		.cmdsrc_regs = 0x01a0,
 		.names = { "AC", "", "", "" },
 	},
 };
@@ -161,6 +170,8 @@ enum aspeed_gpio_reg {
 	reg_debounce_sel1,
 	reg_debounce_sel2,
 	reg_tolerance,
+	reg_cmdsrc0,
+	reg_cmdsrc1,
 };
 
 #define GPIO_VAL_VALUE	0x00
@@ -175,6 +186,13 @@ enum aspeed_gpio_reg {
 #define GPIO_DEBOUNCE_SEL1 0x00
 #define GPIO_DEBOUNCE_SEL2 0x04
 
+#define GPIO_CMDSRC_0	0x00
+#define GPIO_CMDSRC_1	0x04
+#define  GPIO_CMDSRC_ARM		0
+#define  GPIO_CMDSRC_LPC		1
+#define  GPIO_CMDSRC_COLDFIRE		2
+#define  GPIO_CMDSRC_RESERVED		3
+
 /* This will be resolved at compile time */
 static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
 				     const struct aspeed_gpio_bank *bank,
@@ -203,6 +221,10 @@ static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
 		return gpio->base + bank->debounce_regs + GPIO_DEBOUNCE_SEL2;
 	case reg_tolerance:
 		return gpio->base + bank->tolerance_regs;
+	case reg_cmdsrc0:
+		return gpio->base + bank->cmdsrc_regs + GPIO_CMDSRC_0;
+	case reg_cmdsrc1:
+		return gpio->base + bank->cmdsrc_regs + GPIO_CMDSRC_1;
 	}
 	BUG_ON(1);
 }
@@ -269,6 +291,38 @@ static inline bool have_output(struct aspeed_gpio *gpio, unsigned int offset)
 	return !props || (props->output & GPIO_BIT(offset));
 }
 
+static void aspeed_gpio_change_cmd_source(struct aspeed_gpio *gpio,
+					  const struct aspeed_gpio_bank *bank,
+					  int bindex, int cmdsrc)
+{
+	void __iomem *c0 = bank_reg(gpio, bank, reg_cmdsrc0);
+	void __iomem *c1 = bank_reg(gpio, bank, reg_cmdsrc1);
+	u32 bit, reg;
+
+	/*
+	 * Each register controls 4 banks, so take the bottom 2
+	 * bits of the bank index, and use them to select the
+	 * right control bit (0, 8, 16 or 24).
+	 */
+	bit = BIT((bindex & 3) << 3);
+
+	/* Source 1 first to avoid illegal 11 combination */
+	reg = ioread32(c1);
+	if (cmdsrc & 2)
+		reg |= bit;
+	else
+		reg &= ~bit;
+	iowrite32(reg, c1);
+
+	/* Then Source 0 */
+	reg = ioread32(c0);
+	if (cmdsrc & 1)
+		reg |= bit;
+	else
+		reg &= ~bit;
+	iowrite32(reg, c0);
+}
+
 static int aspeed_gpio_get(struct gpio_chip *gc, unsigned int offset)
 {
 	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
-- 
2.17.1

[PATCH v2 3/4] gpio: aspeed: Add command source registers

[Joel Stanley]

On 18 June 2018 at 14:23, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> This adds the definitions for the command source registers
> and a helper to set them.
>
> Those registers allow to control which bus master on the
> SoC is allowed to modify a given bank of GPIOs and will
> be used by subsequent patches.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

>
> +static void aspeed_gpio_change_cmd_source(struct aspeed_gpio *gpio,
> +                                         const struct aspeed_gpio_bank *bank,
> +                                         int bindex, int cmdsrc)
> +{
> +       void __iomem *c0 = bank_reg(gpio, bank, reg_cmdsrc0);
> +       void __iomem *c1 = bank_reg(gpio, bank, reg_cmdsrc1);
> +       u32 bit, reg;
> +
> +       /*
> +        * Each register controls 4 banks, so take the bottom 2
> +        * bits of the bank index, and use them to select the
> +        * right control bit (0, 8, 16 or 24).
> +        */
> +       bit = BIT((bindex & 3) << 3);

This is still hard to understand (it looks like a mistake at first
glance). I don't have any suggestions other than changing  << 3 to *
8.

The comment does explain it, so I'm fine with it going in.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> +
> +       /* Source 1 first to avoid illegal 11 combination */
> +       reg = ioread32(c1);
> +       if (cmdsrc & 2)
> +               reg |= bit;
> +       else
> +               reg &= ~bit;
> +       iowrite32(reg, c1);
> +
> +       /* Then Source 0 */
> +       reg = ioread32(c0);
> +       if (cmdsrc & 1)
> +               reg |= bit;
> +       else
> +               reg &= ~bit;
> +       iowrite32(reg, c0);
> +}
> +
>  static int aspeed_gpio_get(struct gpio_chip *gc, unsigned int offset)
>  {
>         struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> --
> 2.17.1
>

[PATCH v2 3/4] gpio: aspeed: Add command source registers

[Andrew Jeffery]

On Mon, 18 Jun 2018, at 14:23, Benjamin Herrenschmidt wrote:
> This adds the definitions for the command source registers
> and a helper to set them.
> 
> Those registers allow to control which bus master on the
> SoC is allowed to modify a given bank of GPIOs and will
> be used by subsequent patches.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/gpio/gpio-aspeed.c | 54 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index a5ded50c6db0..b3968f66b1d2 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -66,6 +66,7 @@ struct aspeed_gpio_bank {
>  	uint16_t	irq_regs;
>  	uint16_t	debounce_regs;
>  	uint16_t	tolerance_regs;
> +	uint16_t	cmdsrc_regs;
>  	const char	names[4][3];
>  };
>  
> @@ -89,6 +90,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  		.irq_regs = 0x0008,
>  		.debounce_regs = 0x0040,
>  		.tolerance_regs = 0x001c,
> +		.cmdsrc_regs = 0x0060,
>  		.names = { "A", "B", "C", "D" },
>  	},
>  	{
> @@ -97,6 +99,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  		.irq_regs = 0x0028,
>  		.debounce_regs = 0x0048,
>  		.tolerance_regs = 0x003c,
> +		.cmdsrc_regs = 0x0068,
>  		.names = { "E", "F", "G", "H" },
>  	},
>  	{
> @@ -105,6 +108,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  		.irq_regs = 0x0098,
>  		.debounce_regs = 0x00b0,
>  		.tolerance_regs = 0x00ac,
> +		.cmdsrc_regs = 0x0090,
>  		.names = { "I", "J", "K", "L" },
>  	},
>  	{
> @@ -113,6 +117,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  		.irq_regs = 0x00e8,
>  		.debounce_regs = 0x0100,
>  		.tolerance_regs = 0x00fc,
> +		.cmdsrc_regs = 0x00e0,
>  		.names = { "M", "N", "O", "P" },
>  	},
>  	{
> @@ -121,6 +126,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  		.irq_regs = 0x0118,
>  		.debounce_regs = 0x0130,
>  		.tolerance_regs = 0x012c,
> +		.cmdsrc_regs = 0x0110,
>  		.names = { "Q", "R", "S", "T" },
>  	},
>  	{
> @@ -129,6 +135,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  		.irq_regs = 0x0148,
>  		.debounce_regs = 0x0160,
>  		.tolerance_regs = 0x015c,
> +		.cmdsrc_regs = 0x0140,
>  		.names = { "U", "V", "W", "X" },
>  	},
>  	{
> @@ -137,6 +144,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  		.irq_regs = 0x0178,
>  		.debounce_regs = 0x0190,
>  		.tolerance_regs = 0x018c,
> +		.cmdsrc_regs = 0x0170,
>  		.names = { "Y", "Z", "AA", "AB" },
>  	},
>  	{
> @@ -145,6 +153,7 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  		.irq_regs = 0x01a8,
>  		.debounce_regs = 0x01c0,
>  		.tolerance_regs = 0x01bc,
> +		.cmdsrc_regs = 0x01a0,
>  		.names = { "AC", "", "", "" },
>  	},
>  };
> @@ -161,6 +170,8 @@ enum aspeed_gpio_reg {
>  	reg_debounce_sel1,
>  	reg_debounce_sel2,
>  	reg_tolerance,
> +	reg_cmdsrc0,
> +	reg_cmdsrc1,
>  };
>  
>  #define GPIO_VAL_VALUE	0x00
> @@ -175,6 +186,13 @@ enum aspeed_gpio_reg {
>  #define GPIO_DEBOUNCE_SEL1 0x00
>  #define GPIO_DEBOUNCE_SEL2 0x04
>  
> +#define GPIO_CMDSRC_0	0x00
> +#define GPIO_CMDSRC_1	0x04
> +#define  GPIO_CMDSRC_ARM		0
> +#define  GPIO_CMDSRC_LPC		1
> +#define  GPIO_CMDSRC_COLDFIRE		2
> +#define  GPIO_CMDSRC_RESERVED		3
> +
>  /* This will be resolved at compile time */
>  static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
>  				     const struct aspeed_gpio_bank *bank,
> @@ -203,6 +221,10 @@ static inline void __iomem *bank_reg(struct 
> aspeed_gpio *gpio,
>  		return gpio->base + bank->debounce_regs + GPIO_DEBOUNCE_SEL2;
>  	case reg_tolerance:
>  		return gpio->base + bank->tolerance_regs;
> +	case reg_cmdsrc0:
> +		return gpio->base + bank->cmdsrc_regs + GPIO_CMDSRC_0;
> +	case reg_cmdsrc1:
> +		return gpio->base + bank->cmdsrc_regs + GPIO_CMDSRC_1;
>  	}
>  	BUG_ON(1);
>  }
> @@ -269,6 +291,38 @@ static inline bool have_output(struct aspeed_gpio 
> *gpio, unsigned int offset)
>  	return !props || (props->output & GPIO_BIT(offset));
>  }
>  
> +static void aspeed_gpio_change_cmd_source(struct aspeed_gpio *gpio,
> +					  const struct aspeed_gpio_bank *bank,
> +					  int bindex, int cmdsrc)
> +{
> +	void __iomem *c0 = bank_reg(gpio, bank, reg_cmdsrc0);
> +	void __iomem *c1 = bank_reg(gpio, bank, reg_cmdsrc1);
> +	u32 bit, reg;
> +
> +	/*
> +	 * Each register controls 4 banks, so take the bottom 2
> +	 * bits of the bank index, and use them to select the
> +	 * right control bit (0, 8, 16 or 24).
> +	 */
> +	bit = BIT((bindex & 3) << 3);
> +
> +	/* Source 1 first to avoid illegal 11 combination */
> +	reg = ioread32(c1);
> +	if (cmdsrc & 2)
> +		reg |= bit;
> +	else
> +		reg &= ~bit;
> +	iowrite32(reg, c1);
> +
> +	/* Then Source 0 */
> +	reg = ioread32(c0);
> +	if (cmdsrc & 1)
> +		reg |= bit;
> +	else
> +		reg &= ~bit;
> +	iowrite32(reg, c0);

This sequence is very similar to the debounce configuration. I think the patch is fine, but I wonder if in the future it's worth trying to refactor things a little to provide a common implementation. It may be more trouble than it's worth though given corner cases like the reserved value here.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> +}
> +
>  static int aspeed_gpio_get(struct gpio_chip *gc, unsigned int offset)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> -- 
> 2.17.1
> 

[PATCH v2 4/4] gpio: aspeed: Add interfaces for co-processor to grab GPIOs

[Benjamin Herrenschmidt]

On the Aspeed chip, the GPIOs can be under control of the ARM
chip or of the ColdFire coprocessor. (There's a third command
source, the LPC bus, which we don't use or support yet).

The control of which master is allowed to modify a given
GPIO is per-bank (8 GPIOs).

Unfortunately, systems already exist for which we want to
use GPIOs of both sources in the same bank.

This provides an API exported by the gpio-aspeed driver
that an aspeed coprocessor driver can use to "grab" some
GPIOs for use by the coprocessor, and allow the coprocessor
driver to provide callbacks for arbitrating access.

Once at least one GPIO of a given bank has been "grabbed"
by the coprocessor, the entire bank is marked as being
under coprocessor control. It's command source is switched
to the coprocessor.

If the ARM then tries to write to a GPIO in such a marked bank,
the provided callbacks are used to request access from the
coprocessor driver, which is responsible to doing whatever
is necessary to "pause" the coprocessor or prevent it from
trying to use the GPIOs while the ARM is doing its accesses.

During that time, the command source for the bank is temporarily
switched back to the ARM.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/gpio/gpio-aspeed.c  | 235 +++++++++++++++++++++++++++++++++---
 include/linux/gpio/aspeed.h |  14 +++
 2 files changed, 229 insertions(+), 20 deletions(-)
 create mode 100644 include/linux/gpio/aspeed.h

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index b3968f66b1d2..6660c1889f8b 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -12,6 +12,8 @@
 #include <asm/div64.h>
 #include <linux/clk.h>
 #include <linux/gpio/driver.h>
+#include <linux/gpio/aspeed.h>
+#include <linux/gpio/consumer.h>
 #include <linux/hashtable.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -22,6 +24,8 @@
 #include <linux/spinlock.h>
 #include <linux/string.h>
 
+#include "gpiolib.h"
+
 struct aspeed_bank_props {
 	unsigned int bank;
 	u32 input;
@@ -56,6 +60,7 @@ struct aspeed_gpio {
 	struct clk *clk;
 
 	u32 *dcache;
+	u8 *cf_copro_bankmap;
 };
 
 struct aspeed_gpio_bank {
@@ -83,6 +88,9 @@ struct aspeed_gpio_bank {
 
 static const int debounce_timers[4] = { 0x00, 0x50, 0x54, 0x58 };
 
+static const struct aspeed_gpio_copro_ops *copro_ops;
+static void *copro_data;
+
 static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 	{
 		.val_regs = 0x0000,
@@ -323,6 +331,50 @@ static void aspeed_gpio_change_cmd_source(struct aspeed_gpio *gpio,
 	iowrite32(reg, c0);
 }
 
+static bool aspeed_gpio_copro_request(struct aspeed_gpio *gpio,
+				      unsigned int offset)
+{
+	const struct aspeed_gpio_bank *bank = to_bank(offset);
+
+	if (!copro_ops || !gpio->cf_copro_bankmap)
+		return false;
+	if (!gpio->cf_copro_bankmap[offset >> 3])
+		return false;
+	if (!copro_ops->request_access)
+		return false;
+
+	/* Pause the coprocessor */
+	copro_ops->request_access(copro_data);
+
+	/* Change command source back to ARM */
+	aspeed_gpio_change_cmd_source(gpio, bank, offset >> 3, GPIO_CMDSRC_ARM);
+
+	/* Update cache */
+	gpio->dcache[GPIO_BANK(offset)] = ioread32(bank_reg(gpio, bank, reg_rdata));
+
+	return true;
+}
+
+static void aspeed_gpio_copro_release(struct aspeed_gpio *gpio,
+				      unsigned int offset)
+{
+	const struct aspeed_gpio_bank *bank = to_bank(offset);
+
+	if (!copro_ops || !gpio->cf_copro_bankmap)
+		return;
+	if (!gpio->cf_copro_bankmap[offset >> 3])
+		return;
+	if (!copro_ops->release_access)
+		return;
+
+	/* Change command source back to ColdFire */
+	aspeed_gpio_change_cmd_source(gpio, bank, offset >> 3,
+				      GPIO_CMDSRC_COLDFIRE);
+
+	/* Restart the coprocessor */
+	copro_ops->release_access(copro_data);
+}
+
 static int aspeed_gpio_get(struct gpio_chip *gc, unsigned int offset)
 {
 	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
@@ -356,11 +408,15 @@ static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
 {
 	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
 	unsigned long flags;
+	bool copro;
 
 	spin_lock_irqsave(&gpio->lock, flags);
+	copro = aspeed_gpio_copro_request(gpio, offset);
 
 	__aspeed_gpio_set(gc, offset, val);
 
+	if (copro)
+		aspeed_gpio_copro_release(gpio, offset);
 	spin_unlock_irqrestore(&gpio->lock, flags);
 }
 
@@ -368,7 +424,9 @@ static int aspeed_gpio_dir_in(struct gpio_chip *gc, unsigned int offset)
 {
 	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
 	const struct aspeed_gpio_bank *bank = to_bank(offset);
+	void __iomem *addr = bank_reg(gpio, bank, reg_dir);
 	unsigned long flags;
+	bool copro;
 	u32 reg;
 
 	if (!have_input(gpio, offset))
@@ -376,8 +434,13 @@ static int aspeed_gpio_dir_in(struct gpio_chip *gc, unsigned int offset)
 
 	spin_lock_irqsave(&gpio->lock, flags);
 
-	reg = ioread32(bank_reg(gpio, bank, reg_dir));
-	iowrite32(reg & ~GPIO_BIT(offset), bank_reg(gpio, bank, reg_dir));
+	reg = ioread32(addr);
+	reg &= ~GPIO_BIT(offset);
+
+	copro = aspeed_gpio_copro_request(gpio, offset);
+	iowrite32(reg, addr);
+	if (copro)
+		aspeed_gpio_copro_release(gpio, offset);
 
 	spin_unlock_irqrestore(&gpio->lock, flags);
 
@@ -389,7 +452,9 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
 {
 	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
 	const struct aspeed_gpio_bank *bank = to_bank(offset);
+	void __iomem *addr = bank_reg(gpio, bank, reg_dir);
 	unsigned long flags;
+	bool copro;
 	u32 reg;
 
 	if (!have_output(gpio, offset))
@@ -397,10 +462,15 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
 
 	spin_lock_irqsave(&gpio->lock, flags);
 
+	reg = ioread32(addr);
+	reg |= GPIO_BIT(offset);
+
+	copro = aspeed_gpio_copro_request(gpio, offset);
 	__aspeed_gpio_set(gc, offset, val);
-	reg = ioread32(bank_reg(gpio, bank, reg_dir));
-	iowrite32(reg | GPIO_BIT(offset), bank_reg(gpio, bank, reg_dir));
+	iowrite32(reg, addr);
 
+	if (copro)
+		aspeed_gpio_copro_release(gpio, offset);
 	spin_unlock_irqrestore(&gpio->lock, flags);
 
 	return 0;
@@ -430,24 +500,23 @@ static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
 }
 
 static inline int irqd_to_aspeed_gpio_data(struct irq_data *d,
-		struct aspeed_gpio **gpio,
-		const struct aspeed_gpio_bank **bank,
-		u32 *bit)
+					   struct aspeed_gpio **gpio,
+					   const struct aspeed_gpio_bank **bank,
+					   u32 *bit, int *offset)
 {
-	int offset;
 	struct aspeed_gpio *internal;
 
-	offset = irqd_to_hwirq(d);
+	*offset = irqd_to_hwirq(d);
 
 	internal = irq_data_get_irq_chip_data(d);
 
 	/* This might be a bit of a questionable place to check */
-	if (!have_irq(internal, offset))
+	if (!have_irq(internal, *offset))
 		return -ENOTSUPP;
 
 	*gpio = internal;
-	*bank = to_bank(offset);
-	*bit = GPIO_BIT(offset);
+	*bank = to_bank(*offset);
+	*bit = GPIO_BIT(*offset);
 
 	return 0;
 }
@@ -458,17 +527,23 @@ static void aspeed_gpio_irq_ack(struct irq_data *d)
 	struct aspeed_gpio *gpio;
 	unsigned long flags;
 	void __iomem *status_addr;
+	int rc, offset;
+	bool copro;
 	u32 bit;
-	int rc;
 
-	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
+	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
 	if (rc)
 		return;
 
 	status_addr = bank_reg(gpio, bank, reg_irq_status);
 
 	spin_lock_irqsave(&gpio->lock, flags);
+	copro = aspeed_gpio_copro_request(gpio, offset);
+
 	iowrite32(bit, status_addr);
+
+	if (copro)
+		aspeed_gpio_copro_release(gpio, offset);
 	spin_unlock_irqrestore(&gpio->lock, flags);
 }
 
@@ -479,15 +554,17 @@ static void aspeed_gpio_irq_set_mask(struct irq_data *d, bool set)
 	unsigned long flags;
 	u32 reg, bit;
 	void __iomem *addr;
-	int rc;
+	int rc, offset;
+	bool copro;
 
-	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
+	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
 	if (rc)
 		return;
 
 	addr = bank_reg(gpio, bank, reg_irq_enable);
 
 	spin_lock_irqsave(&gpio->lock, flags);
+	copro = aspeed_gpio_copro_request(gpio, offset);
 
 	reg = ioread32(addr);
 	if (set)
@@ -496,6 +573,8 @@ static void aspeed_gpio_irq_set_mask(struct irq_data *d, bool set)
 		reg &= ~bit;
 	iowrite32(reg, addr);
 
+	if (copro)
+		aspeed_gpio_copro_release(gpio, offset);
 	spin_unlock_irqrestore(&gpio->lock, flags);
 }
 
@@ -520,9 +599,10 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
 	struct aspeed_gpio *gpio;
 	unsigned long flags;
 	void __iomem *addr;
-	int rc;
+	int rc, offset;
+	bool copro;
 
-	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
+	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
 	if (rc)
 		return -EINVAL;
 
@@ -548,6 +628,7 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
 	}
 
 	spin_lock_irqsave(&gpio->lock, flags);
+	copro = aspeed_gpio_copro_request(gpio, offset);
 
 	addr = bank_reg(gpio, bank, reg_irq_type0);
 	reg = ioread32(addr);
@@ -564,6 +645,8 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
 	reg = (reg & ~bit) | type2;
 	iowrite32(reg, addr);
 
+	if (copro)
+		aspeed_gpio_copro_release(gpio, offset);
 	spin_unlock_irqrestore(&gpio->lock, flags);
 
 	irq_set_handler_locked(d, handler);
@@ -658,11 +741,14 @@ static int aspeed_gpio_reset_tolerance(struct gpio_chip *chip,
 	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
 	unsigned long flags;
 	void __iomem *treg;
+	bool copro;
 	u32 val;
 
 	treg = bank_reg(gpio, to_bank(offset), reg_tolerance);
 
 	spin_lock_irqsave(&gpio->lock, flags);
+	copro = aspeed_gpio_copro_request(gpio, offset);
+
 	val = readl(treg);
 
 	if (enable)
@@ -671,6 +757,9 @@ static int aspeed_gpio_reset_tolerance(struct gpio_chip *chip,
 		val &= ~GPIO_BIT(offset);
 
 	writel(val, treg);
+
+	if (copro)
+		aspeed_gpio_copro_release(gpio, offset);
 	spin_unlock_irqrestore(&gpio->lock, flags);
 
 	return 0;
@@ -766,6 +855,9 @@ static void configure_timer(struct aspeed_gpio *gpio, unsigned int offset,
 	void __iomem *addr;
 	u32 val;
 
+	/* Note: Debounce timer isn't under control of the command
+	 * source registers, so no need to sync with the coprocessor
+	 */
 	addr = bank_reg(gpio, bank, reg_debounce_sel1);
 	val = ioread32(addr);
 	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE1(timer, offset), addr);
@@ -912,6 +1004,101 @@ static int aspeed_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 	return -ENOTSUPP;
 }
 
+/**
+ * aspeed_gpio_copro_set_ops - Sets the callbacks used for handhsaking with
+ *                             the coprocessor for shared GPIO banks
+ * @ops: The callbacks
+ * @data: Pointer passed back to the callbacks
+ */
+int aspeed_gpio_copro_set_ops(const struct aspeed_gpio_copro_ops *ops, void *data)
+{
+	copro_data = data;
+	copro_ops = ops;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(aspeed_gpio_copro_set_ops);
+
+/**
+ * aspeed_gpio_copro_grab_gpio - Mark a GPIO used by the coprocessor. The entire
+ *                               bank gets marked and any access from the ARM will
+ *                               result in handshaking via callbacks.
+ * @desc: The GPIO to be marked
+ */
+int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc)
+{
+	struct gpio_chip *chip = gpiod_to_chip(desc);
+	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
+	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
+	const struct aspeed_gpio_bank *bank = to_bank(offset);
+	unsigned long flags;
+
+	if (!gpio->cf_copro_bankmap)
+		gpio->cf_copro_bankmap = kzalloc(gpio->config->nr_gpios >> 3, GFP_KERNEL);
+	if (!gpio->cf_copro_bankmap)
+		return -ENOMEM;
+	if (offset < 0 || offset > gpio->config->nr_gpios)
+		return -EINVAL;
+	bindex = offset >> 3;
+
+	spin_lock_irqsave(&gpio->lock, flags);
+
+	/* Sanity check, this shouldn't happen */
+	if (gpio->cf_copro_bankmap[bindex] == 0xff) {
+		rc = -EIO;
+		goto bail;
+	}
+	gpio->cf_copro_bankmap[bindex]++;
+
+	/* Switch command source */
+	if (gpio->cf_copro_bankmap[bindex] == 1)
+		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
+					      GPIO_CMDSRC_COLDFIRE);
+
+ bail:
+	spin_unlock_irqrestore(&gpio->lock, flags);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(aspeed_gpio_copro_grab_gpio);
+
+/**
+ * aspeed_gpio_copro_release_gpio - Unmark a GPIO used by the coprocessor.
+ * @desc: The GPIO to be marked
+ */
+int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc)
+{
+	struct gpio_chip *chip = gpiod_to_chip(desc);
+	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
+	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
+	const struct aspeed_gpio_bank *bank = to_bank(offset);
+	unsigned long flags;
+
+	if (!gpio->cf_copro_bankmap)
+		return -ENXIO;
+
+	if (offset < 0 || offset > gpio->config->nr_gpios)
+		return -EINVAL;
+	bindex = offset >> 3;
+
+	spin_lock_irqsave(&gpio->lock, flags);
+
+	/* Sanity check, this shouldn't happen */
+	if (gpio->cf_copro_bankmap[bindex] == 0) {
+		rc = -EIO;
+		goto bail;
+	}
+	gpio->cf_copro_bankmap[bindex]--;
+
+	/* Switch command source */
+	if (gpio->cf_copro_bankmap[bindex] == 0)
+		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
+					      GPIO_CMDSRC_ARM);
+ bail:
+	spin_unlock_irqrestore(&gpio->lock, flags);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(aspeed_gpio_copro_release_gpio);
+
 /*
  * Any banks not specified in a struct aspeed_bank_props array are assumed to
  * have the properties:
@@ -1002,10 +1189,18 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
 	if (!gpio->dcache)
 		return -ENOMEM;
 
-	/* Populate it with initial values read from the HW */
+	/*
+	 * Populate it with initial values read from the HW and switch
+	 * all command sources to the ARM by default
+	 */
 	for (i = 0; i < banks; i++) {
-		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_rdata);
+		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
+		void __iomem *addr = bank_reg(gpio, bank, reg_rdata);
 		gpio->dcache[i] = ioread32(addr);
+		aspeed_gpio_change_cmd_source(gpio, bank, 0, GPIO_CMDSRC_ARM);
+		aspeed_gpio_change_cmd_source(gpio, bank, 1, GPIO_CMDSRC_ARM);
+		aspeed_gpio_change_cmd_source(gpio, bank, 2, GPIO_CMDSRC_ARM);
+		aspeed_gpio_change_cmd_source(gpio, bank, 3, GPIO_CMDSRC_ARM);
 	}
 
 	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
diff --git a/include/linux/gpio/aspeed.h b/include/linux/gpio/aspeed.h
new file mode 100644
index 000000000000..b6871c9b71f7
--- /dev/null
+++ b/include/linux/gpio/aspeed.h
@@ -0,0 +1,14 @@
+#ifndef __GPIO_ASPEED_H
+#define __GPIO_ASPEED_H
+
+struct aspeed_gpio_copro_ops {
+	int (*request_access)(void *data);
+	int (*release_access)(void *data);
+};
+
+int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc);
+int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc);
+int aspeed_gpio_copro_set_ops(const struct aspeed_gpio_copro_ops *ops, void *data);
+
+
+#endif /* __GPIO_ASPEED_H */
-- 
2.17.1

[PATCH v2 4/4] gpio: aspeed: Add interfaces for co-processor to grab GPIOs

[Joel Stanley]

On 18 June 2018 at 14:23, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On the Aspeed chip, the GPIOs can be under control of the ARM
> chip or of the ColdFire coprocessor. (There's a third command
> source, the LPC bus, which we don't use or support yet).
>
> The control of which master is allowed to modify a given
> GPIO is per-bank (8 GPIOs).
>
> Unfortunately, systems already exist for which we want to
> use GPIOs of both sources in the same bank.
>
> This provides an API exported by the gpio-aspeed driver
> that an aspeed coprocessor driver can use to "grab" some
> GPIOs for use by the coprocessor, and allow the coprocessor
> driver to provide callbacks for arbitrating access.
>
> Once at least one GPIO of a given bank has been "grabbed"
> by the coprocessor, the entire bank is marked as being
> under coprocessor control. It's command source is switched
> to the coprocessor.
>
> If the ARM then tries to write to a GPIO in such a marked bank,
> the provided callbacks are used to request access from the
> coprocessor driver, which is responsible to doing whatever
> is necessary to "pause" the coprocessor or prevent it from
> trying to use the GPIOs while the ARM is doing its accesses.
>
> During that time, the command source for the bank is temporarily
> switched back to the ARM.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

I did some playing around with the consumer.h include and I couldn't
see an easy way out of this either.

Reviewed-by: Joel Stanley <joel@jms.id.au>

[PATCH v2 4/4] gpio: aspeed: Add interfaces for co-processor to grab GPIOs

[Andrew Jeffery]

On Mon, 18 Jun 2018, at 14:23, Benjamin Herrenschmidt wrote:
> On the Aspeed chip, the GPIOs can be under control of the ARM
> chip or of the ColdFire coprocessor. (There's a third command
> source, the LPC bus, which we don't use or support yet).
> 
> The control of which master is allowed to modify a given
> GPIO is per-bank (8 GPIOs).
> 
> Unfortunately, systems already exist for which we want to
> use GPIOs of both sources in the same bank.
> 
> This provides an API exported by the gpio-aspeed driver
> that an aspeed coprocessor driver can use to "grab" some
> GPIOs for use by the coprocessor, and allow the coprocessor
> driver to provide callbacks for arbitrating access.
> 
> Once at least one GPIO of a given bank has been "grabbed"
> by the coprocessor, the entire bank is marked as being
> under coprocessor control. It's command source is switched
> to the coprocessor.
> 
> If the ARM then tries to write to a GPIO in such a marked bank,
> the provided callbacks are used to request access from the
> coprocessor driver, which is responsible to doing whatever
> is necessary to "pause" the coprocessor or prevent it from
> trying to use the GPIOs while the ARM is doing its accesses.
> 
> During that time, the command source for the bank is temporarily
> switched back to the ARM.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/gpio/gpio-aspeed.c  | 235 +++++++++++++++++++++++++++++++++---
>  include/linux/gpio/aspeed.h |  14 +++
>  2 files changed, 229 insertions(+), 20 deletions(-)
>  create mode 100644 include/linux/gpio/aspeed.h
> 
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index b3968f66b1d2..6660c1889f8b 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -12,6 +12,8 @@
>  #include <asm/div64.h>
>  #include <linux/clk.h>
>  #include <linux/gpio/driver.h>
> +#include <linux/gpio/aspeed.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/hashtable.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> @@ -22,6 +24,8 @@
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
>  
> +#include "gpiolib.h"
> +
>  struct aspeed_bank_props {
>  	unsigned int bank;
>  	u32 input;
> @@ -56,6 +60,7 @@ struct aspeed_gpio {
>  	struct clk *clk;
>  
>  	u32 *dcache;
> +	u8 *cf_copro_bankmap;
>  };
>  
>  struct aspeed_gpio_bank {
> @@ -83,6 +88,9 @@ struct aspeed_gpio_bank {
>  
>  static const int debounce_timers[4] = { 0x00, 0x50, 0x54, 0x58 };
>  
> +static const struct aspeed_gpio_copro_ops *copro_ops;
> +static void *copro_data;
> +
>  static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
>  	{
>  		.val_regs = 0x0000,
> @@ -323,6 +331,50 @@ static void aspeed_gpio_change_cmd_source(struct 
> aspeed_gpio *gpio,
>  	iowrite32(reg, c0);
>  }
>  
> +static bool aspeed_gpio_copro_request(struct aspeed_gpio *gpio,
> +				      unsigned int offset)
> +{
> +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +
> +	if (!copro_ops || !gpio->cf_copro_bankmap)
> +		return false;
> +	if (!gpio->cf_copro_bankmap[offset >> 3])
> +		return false;
> +	if (!copro_ops->request_access)
> +		return false;
> +
> +	/* Pause the coprocessor */
> +	copro_ops->request_access(copro_data);
> +
> +	/* Change command source back to ARM */
> +	aspeed_gpio_change_cmd_source(gpio, bank, offset >> 3, 
> GPIO_CMDSRC_ARM);
> +
> +	/* Update cache */
> +	gpio->dcache[GPIO_BANK(offset)] = ioread32(bank_reg(gpio, bank, 
> reg_rdata));
> +
> +	return true;
> +}
> +
> +static void aspeed_gpio_copro_release(struct aspeed_gpio *gpio,
> +				      unsigned int offset)
> +{
> +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +
> +	if (!copro_ops || !gpio->cf_copro_bankmap)
> +		return;
> +	if (!gpio->cf_copro_bankmap[offset >> 3])
> +		return;
> +	if (!copro_ops->release_access)
> +		return;
> +
> +	/* Change command source back to ColdFire */
> +	aspeed_gpio_change_cmd_source(gpio, bank, offset >> 3,
> +				      GPIO_CMDSRC_COLDFIRE);
> +
> +	/* Restart the coprocessor */
> +	copro_ops->release_access(copro_data);
> +}
> +
>  static int aspeed_gpio_get(struct gpio_chip *gc, unsigned int offset)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> @@ -356,11 +408,15 @@ static void aspeed_gpio_set(struct gpio_chip *gc, 
> unsigned int offset,
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
>  	unsigned long flags;
> +	bool copro;
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
> +	copro = aspeed_gpio_copro_request(gpio, offset);
>  
>  	__aspeed_gpio_set(gc, offset, val);
>  
> +	if (copro)
> +		aspeed_gpio_copro_release(gpio, offset);
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  }
>  
> @@ -368,7 +424,9 @@ static int aspeed_gpio_dir_in(struct gpio_chip *gc, 
> unsigned int offset)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
>  	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +	void __iomem *addr = bank_reg(gpio, bank, reg_dir);
>  	unsigned long flags;
> +	bool copro;
>  	u32 reg;
>  
>  	if (!have_input(gpio, offset))
> @@ -376,8 +434,13 @@ static int aspeed_gpio_dir_in(struct gpio_chip *gc, 
> unsigned int offset)
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
>  
> -	reg = ioread32(bank_reg(gpio, bank, reg_dir));
> -	iowrite32(reg & ~GPIO_BIT(offset), bank_reg(gpio, bank, reg_dir));
> +	reg = ioread32(addr);
> +	reg &= ~GPIO_BIT(offset);
> +
> +	copro = aspeed_gpio_copro_request(gpio, offset);
> +	iowrite32(reg, addr);
> +	if (copro)
> +		aspeed_gpio_copro_release(gpio, offset);
>  
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  
> @@ -389,7 +452,9 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
>  	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +	void __iomem *addr = bank_reg(gpio, bank, reg_dir);
>  	unsigned long flags;
> +	bool copro;
>  	u32 reg;
>  
>  	if (!have_output(gpio, offset))
> @@ -397,10 +462,15 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
>  
> +	reg = ioread32(addr);
> +	reg |= GPIO_BIT(offset);
> +
> +	copro = aspeed_gpio_copro_request(gpio, offset);
>  	__aspeed_gpio_set(gc, offset, val);
> -	reg = ioread32(bank_reg(gpio, bank, reg_dir));
> -	iowrite32(reg | GPIO_BIT(offset), bank_reg(gpio, bank, reg_dir));
> +	iowrite32(reg, addr);
>  
> +	if (copro)
> +		aspeed_gpio_copro_release(gpio, offset);
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  
>  	return 0;
> @@ -430,24 +500,23 @@ static int aspeed_gpio_get_direction(struct 
> gpio_chip *gc, unsigned int offset)
>  }
>  
>  static inline int irqd_to_aspeed_gpio_data(struct irq_data *d,
> -		struct aspeed_gpio **gpio,
> -		const struct aspeed_gpio_bank **bank,
> -		u32 *bit)
> +					   struct aspeed_gpio **gpio,
> +					   const struct aspeed_gpio_bank **bank,
> +					   u32 *bit, int *offset)
>  {
> -	int offset;
>  	struct aspeed_gpio *internal;
>  
> -	offset = irqd_to_hwirq(d);
> +	*offset = irqd_to_hwirq(d);

Nit: Did you intend to set this out parameter before potentially returning an error? I had tried to avoid that up until now.

>  
>  	internal = irq_data_get_irq_chip_data(d);
>  
>  	/* This might be a bit of a questionable place to check */
> -	if (!have_irq(internal, offset))
> +	if (!have_irq(internal, *offset))
>  		return -ENOTSUPP;
>  
>  	*gpio = internal;
> -	*bank = to_bank(offset);
> -	*bit = GPIO_BIT(offset);
> +	*bank = to_bank(*offset);
> +	*bit = GPIO_BIT(*offset);
>  
>  	return 0;
>  }
> @@ -458,17 +527,23 @@ static void aspeed_gpio_irq_ack(struct irq_data *d)
>  	struct aspeed_gpio *gpio;
>  	unsigned long flags;
>  	void __iomem *status_addr;
> +	int rc, offset;
> +	bool copro;
>  	u32 bit;
> -	int rc;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
>  	if (rc)
>  		return;
>  
>  	status_addr = bank_reg(gpio, bank, reg_irq_status);
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
> +	copro = aspeed_gpio_copro_request(gpio, offset);
> +
>  	iowrite32(bit, status_addr);
> +
> +	if (copro)
> +		aspeed_gpio_copro_release(gpio, offset);
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  }
>  
> @@ -479,15 +554,17 @@ static void aspeed_gpio_irq_set_mask(struct 
> irq_data *d, bool set)
>  	unsigned long flags;
>  	u32 reg, bit;
>  	void __iomem *addr;
> -	int rc;
> +	int rc, offset;
> +	bool copro;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
>  	if (rc)
>  		return;
>  
>  	addr = bank_reg(gpio, bank, reg_irq_enable);
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
> +	copro = aspeed_gpio_copro_request(gpio, offset);
>  
>  	reg = ioread32(addr);
>  	if (set)
> @@ -496,6 +573,8 @@ static void aspeed_gpio_irq_set_mask(struct irq_data 
> *d, bool set)
>  		reg &= ~bit;
>  	iowrite32(reg, addr);
>  
> +	if (copro)
> +		aspeed_gpio_copro_release(gpio, offset);
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  }
>  
> @@ -520,9 +599,10 @@ static int aspeed_gpio_set_type(struct irq_data *d, 
> unsigned int type)
>  	struct aspeed_gpio *gpio;
>  	unsigned long flags;
>  	void __iomem *addr;
> -	int rc;
> +	int rc, offset;
> +	bool copro;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
>  	if (rc)
>  		return -EINVAL;
>  
> @@ -548,6 +628,7 @@ static int aspeed_gpio_set_type(struct irq_data *d, 
> unsigned int type)
>  	}
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
> +	copro = aspeed_gpio_copro_request(gpio, offset);
>  
>  	addr = bank_reg(gpio, bank, reg_irq_type0);
>  	reg = ioread32(addr);
> @@ -564,6 +645,8 @@ static int aspeed_gpio_set_type(struct irq_data *d, 
> unsigned int type)
>  	reg = (reg & ~bit) | type2;
>  	iowrite32(reg, addr);
>  
> +	if (copro)
> +		aspeed_gpio_copro_release(gpio, offset);
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  
>  	irq_set_handler_locked(d, handler);
> @@ -658,11 +741,14 @@ static int aspeed_gpio_reset_tolerance(struct 
> gpio_chip *chip,
>  	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
>  	unsigned long flags;
>  	void __iomem *treg;
> +	bool copro;
>  	u32 val;
>  
>  	treg = bank_reg(gpio, to_bank(offset), reg_tolerance);
>  
>  	spin_lock_irqsave(&gpio->lock, flags);
> +	copro = aspeed_gpio_copro_request(gpio, offset);
> +
>  	val = readl(treg);
>  
>  	if (enable)
> @@ -671,6 +757,9 @@ static int aspeed_gpio_reset_tolerance(struct 
> gpio_chip *chip,
>  		val &= ~GPIO_BIT(offset);
>  
>  	writel(val, treg);
> +
> +	if (copro)
> +		aspeed_gpio_copro_release(gpio, offset);
>  	spin_unlock_irqrestore(&gpio->lock, flags);
>  
>  	return 0;
> @@ -766,6 +855,9 @@ static void configure_timer(struct aspeed_gpio 
> *gpio, unsigned int offset,
>  	void __iomem *addr;
>  	u32 val;
>  
> +	/* Note: Debounce timer isn't under control of the command
> +	 * source registers, so no need to sync with the coprocessor
> +	 */
>  	addr = bank_reg(gpio, bank, reg_debounce_sel1);
>  	val = ioread32(addr);
>  	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE1(timer, offset), addr);
> @@ -912,6 +1004,101 @@ static int aspeed_gpio_set_config(struct 
> gpio_chip *chip, unsigned int offset,
>  	return -ENOTSUPP;
>  }
>  
> +/**
> + * aspeed_gpio_copro_set_ops - Sets the callbacks used for handhsaking 
> with
> + *                             the coprocessor for shared GPIO banks
> + * @ops: The callbacks
> + * @data: Pointer passed back to the callbacks
> + */
> +int aspeed_gpio_copro_set_ops(const struct aspeed_gpio_copro_ops *ops, 
> void *data)
> +{
> +	copro_data = data;
> +	copro_ops = ops;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(aspeed_gpio_copro_set_ops);
> +
> +/**
> + * aspeed_gpio_copro_grab_gpio - Mark a GPIO used by the coprocessor. 
> The entire
> + *                               bank gets marked and any access from 
> the ARM will
> + *                               result in handshaking via callbacks.
> + * @desc: The GPIO to be marked
> + */
> +int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc)
> +{
> +	struct gpio_chip *chip = gpiod_to_chip(desc);
> +	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
> +	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
> +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +	unsigned long flags;
> +
> +	if (!gpio->cf_copro_bankmap)
> +		gpio->cf_copro_bankmap = kzalloc(gpio->config->nr_gpios >> 3, 
> GFP_KERNEL);
> +	if (!gpio->cf_copro_bankmap)
> +		return -ENOMEM;
> +	if (offset < 0 || offset > gpio->config->nr_gpios)
> +		return -EINVAL;
> +	bindex = offset >> 3;
> +
> +	spin_lock_irqsave(&gpio->lock, flags);
> +
> +	/* Sanity check, this shouldn't happen */
> +	if (gpio->cf_copro_bankmap[bindex] == 0xff) {
> +		rc = -EIO;
> +		goto bail;
> +	}
> +	gpio->cf_copro_bankmap[bindex]++;
> +
> +	/* Switch command source */
> +	if (gpio->cf_copro_bankmap[bindex] == 1)
> +		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
> +					      GPIO_CMDSRC_COLDFIRE);
> +
> + bail:
> +	spin_unlock_irqrestore(&gpio->lock, flags);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(aspeed_gpio_copro_grab_gpio);
> +
> +/**
> + * aspeed_gpio_copro_release_gpio - Unmark a GPIO used by the 
> coprocessor.
> + * @desc: The GPIO to be marked
> + */
> +int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc)
> +{
> +	struct gpio_chip *chip = gpiod_to_chip(desc);
> +	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
> +	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
> +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +	unsigned long flags;
> +
> +	if (!gpio->cf_copro_bankmap)
> +		return -ENXIO;
> +
> +	if (offset < 0 || offset > gpio->config->nr_gpios)
> +		return -EINVAL;
> +	bindex = offset >> 3;
> +
> +	spin_lock_irqsave(&gpio->lock, flags);
> +
> +	/* Sanity check, this shouldn't happen */
> +	if (gpio->cf_copro_bankmap[bindex] == 0) {
> +		rc = -EIO;
> +		goto bail;
> +	}
> +	gpio->cf_copro_bankmap[bindex]--;
> +
> +	/* Switch command source */
> +	if (gpio->cf_copro_bankmap[bindex] == 0)
> +		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
> +					      GPIO_CMDSRC_ARM);
> + bail:
> +	spin_unlock_irqrestore(&gpio->lock, flags);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(aspeed_gpio_copro_release_gpio);
> +
>  /*
>   * Any banks not specified in a struct aspeed_bank_props array are 
> assumed to
>   * have the properties:
> @@ -1002,10 +1189,18 @@ static int __init aspeed_gpio_probe(struct 
> platform_device *pdev)
>  	if (!gpio->dcache)
>  		return -ENOMEM;
>  
> -	/* Populate it with initial values read from the HW */
> +	/*
> +	 * Populate it with initial values read from the HW and switch
> +	 * all command sources to the ARM by default
> +	 */
>  	for (i = 0; i < banks; i++) {
> -		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_rdata);
> +		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
> +		void __iomem *addr = bank_reg(gpio, bank, reg_rdata);
>  		gpio->dcache[i] = ioread32(addr);
> +		aspeed_gpio_change_cmd_source(gpio, bank, 0, GPIO_CMDSRC_ARM);
> +		aspeed_gpio_change_cmd_source(gpio, bank, 1, GPIO_CMDSRC_ARM);
> +		aspeed_gpio_change_cmd_source(gpio, bank, 2, GPIO_CMDSRC_ARM);
> +		aspeed_gpio_change_cmd_source(gpio, bank, 3, GPIO_CMDSRC_ARM);
>  	}
>  
>  	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> diff --git a/include/linux/gpio/aspeed.h b/include/linux/gpio/aspeed.h
> new file mode 100644
> index 000000000000..b6871c9b71f7
> --- /dev/null
> +++ b/include/linux/gpio/aspeed.h
> @@ -0,0 +1,14 @@
> +#ifndef __GPIO_ASPEED_H
> +#define __GPIO_ASPEED_H
> +
> +struct aspeed_gpio_copro_ops {
> +	int (*request_access)(void *data);
> +	int (*release_access)(void *data);
> +};
> +
> +int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc);
> +int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc);
> +int aspeed_gpio_copro_set_ops(const struct aspeed_gpio_copro_ops *ops, 
> void *data);
> +
> +
> +#endif /* __GPIO_ASPEED_H */
> -- 
> 2.17.1
> 

LGTM otherwise, so I'll send a r-b tag if we can live with that one nit-pick.

[PATCH v2 4/4] gpio: aspeed: Add interfaces for co-processor to grab GPIOs

[Benjamin Herrenschmidt]

On Mon, 2018-06-18 at 23:53 +0930, Andrew Jeffery wrote:
> 
> >  static inline int irqd_to_aspeed_gpio_data(struct irq_data *d,
> > -		struct aspeed_gpio **gpio,
> > -		const struct aspeed_gpio_bank **bank,
> > -		u32 *bit)
> > +					   struct aspeed_gpio **gpio,
> > +					   const struct aspeed_gpio_bank **bank,
> > +					   u32 *bit, int *offset)
> >  {
> > -	int offset;
> >  	struct aspeed_gpio *internal;
> >  
> > -	offset = irqd_to_hwirq(d);
> > +	*offset = irqd_to_hwirq(d);
> 
> Nit: Did you intend to set this out parameter before potentially returning an error? I had tried to avoid that up until now.

Yeah it's constant-ish, I don't see why not
 
> >  	internal = irq_data_get_irq_chip_data(d);
> >  
> >  	/* This might be a bit of a questionable place to check */
> > -	if (!have_irq(internal, offset))
> > +	if (!have_irq(internal, *offset))
> >  		return -ENOTSUPP;
> >  
> >  	*gpio = internal;
> > -	*bank = to_bank(offset);
> > -	*bit = GPIO_BIT(offset);
> > +	*bank = to_bank(*offset);
> > +	*bit = GPIO_BIT(*offset);
> >  
> >  	return 0;
> >  }
> > @@ -458,17 +527,23 @@ static void aspeed_gpio_irq_ack(struct irq_data *d)
> >  	struct aspeed_gpio *gpio;
> >  	unsigned long flags;
> >  	void __iomem *status_addr;
> > +	int rc, offset;
> > +	bool copro;
> >  	u32 bit;
> > -	int rc;
> >  
> > -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
> > +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> >  	if (rc)
> >  		return;
> >  
> >  	status_addr = bank_reg(gpio, bank, reg_irq_status);
> >  
> >  	spin_lock_irqsave(&gpio->lock, flags);
> > +	copro = aspeed_gpio_copro_request(gpio, offset);
> > +
> >  	iowrite32(bit, status_addr);
> > +
> > +	if (copro)
> > +		aspeed_gpio_copro_release(gpio, offset);
> >  	spin_unlock_irqrestore(&gpio->lock, flags);
> >  }
> >  
> > @@ -479,15 +554,17 @@ static void aspeed_gpio_irq_set_mask(struct 
> > irq_data *d, bool set)
> >  	unsigned long flags;
> >  	u32 reg, bit;
> >  	void __iomem *addr;
> > -	int rc;
> > +	int rc, offset;
> > +	bool copro;
> >  
> > -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
> > +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> >  	if (rc)
> >  		return;
> >  
> >  	addr = bank_reg(gpio, bank, reg_irq_enable);
> >  
> >  	spin_lock_irqsave(&gpio->lock, flags);
> > +	copro = aspeed_gpio_copro_request(gpio, offset);
> >  
> >  	reg = ioread32(addr);
> >  	if (set)
> > @@ -496,6 +573,8 @@ static void aspeed_gpio_irq_set_mask(struct irq_data 
> > *d, bool set)
> >  		reg &= ~bit;
> >  	iowrite32(reg, addr);
> >  
> > +	if (copro)
> > +		aspeed_gpio_copro_release(gpio, offset);
> >  	spin_unlock_irqrestore(&gpio->lock, flags);
> >  }
> >  
> > @@ -520,9 +599,10 @@ static int aspeed_gpio_set_type(struct irq_data *d, 
> > unsigned int type)
> >  	struct aspeed_gpio *gpio;
> >  	unsigned long flags;
> >  	void __iomem *addr;
> > -	int rc;
> > +	int rc, offset;
> > +	bool copro;
> >  
> > -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit);
> > +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> >  	if (rc)
> >  		return -EINVAL;
> >  
> > @@ -548,6 +628,7 @@ static int aspeed_gpio_set_type(struct irq_data *d, 
> > unsigned int type)
> >  	}
> >  
> >  	spin_lock_irqsave(&gpio->lock, flags);
> > +	copro = aspeed_gpio_copro_request(gpio, offset);
> >  
> >  	addr = bank_reg(gpio, bank, reg_irq_type0);
> >  	reg = ioread32(addr);
> > @@ -564,6 +645,8 @@ static int aspeed_gpio_set_type(struct irq_data *d, 
> > unsigned int type)
> >  	reg = (reg & ~bit) | type2;
> >  	iowrite32(reg, addr);
> >  
> > +	if (copro)
> > +		aspeed_gpio_copro_release(gpio, offset);
> >  	spin_unlock_irqrestore(&gpio->lock, flags);
> >  
> >  	irq_set_handler_locked(d, handler);
> > @@ -658,11 +741,14 @@ static int aspeed_gpio_reset_tolerance(struct 
> > gpio_chip *chip,
> >  	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
> >  	unsigned long flags;
> >  	void __iomem *treg;
> > +	bool copro;
> >  	u32 val;
> >  
> >  	treg = bank_reg(gpio, to_bank(offset), reg_tolerance);
> >  
> >  	spin_lock_irqsave(&gpio->lock, flags);
> > +	copro = aspeed_gpio_copro_request(gpio, offset);
> > +
> >  	val = readl(treg);
> >  
> >  	if (enable)
> > @@ -671,6 +757,9 @@ static int aspeed_gpio_reset_tolerance(struct 
> > gpio_chip *chip,
> >  		val &= ~GPIO_BIT(offset);
> >  
> >  	writel(val, treg);
> > +
> > +	if (copro)
> > +		aspeed_gpio_copro_release(gpio, offset);
> >  	spin_unlock_irqrestore(&gpio->lock, flags);
> >  
> >  	return 0;
> > @@ -766,6 +855,9 @@ static void configure_timer(struct aspeed_gpio 
> > *gpio, unsigned int offset,
> >  	void __iomem *addr;
> >  	u32 val;
> >  
> > +	/* Note: Debounce timer isn't under control of the command
> > +	 * source registers, so no need to sync with the coprocessor
> > +	 */
> >  	addr = bank_reg(gpio, bank, reg_debounce_sel1);
> >  	val = ioread32(addr);
> >  	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE1(timer, offset), addr);
> > @@ -912,6 +1004,101 @@ static int aspeed_gpio_set_config(struct 
> > gpio_chip *chip, unsigned int offset,
> >  	return -ENOTSUPP;
> >  }
> >  
> > +/**
> > + * aspeed_gpio_copro_set_ops - Sets the callbacks used for handhsaking 
> > with
> > + *                             the coprocessor for shared GPIO banks
> > + * @ops: The callbacks
> > + * @data: Pointer passed back to the callbacks
> > + */
> > +int aspeed_gpio_copro_set_ops(const struct aspeed_gpio_copro_ops *ops, 
> > void *data)
> > +{
> > +	copro_data = data;
> > +	copro_ops = ops;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(aspeed_gpio_copro_set_ops);
> > +
> > +/**
> > + * aspeed_gpio_copro_grab_gpio - Mark a GPIO used by the coprocessor. 
> > The entire
> > + *                               bank gets marked and any access from 
> > the ARM will
> > + *                               result in handshaking via callbacks.
> > + * @desc: The GPIO to be marked
> > + */
> > +int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc)
> > +{
> > +	struct gpio_chip *chip = gpiod_to_chip(desc);
> > +	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
> > +	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
> > +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> > +	unsigned long flags;
> > +
> > +	if (!gpio->cf_copro_bankmap)
> > +		gpio->cf_copro_bankmap = kzalloc(gpio->config->nr_gpios >> 3, 
> > GFP_KERNEL);
> > +	if (!gpio->cf_copro_bankmap)
> > +		return -ENOMEM;
> > +	if (offset < 0 || offset > gpio->config->nr_gpios)
> > +		return -EINVAL;
> > +	bindex = offset >> 3;
> > +
> > +	spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +	/* Sanity check, this shouldn't happen */
> > +	if (gpio->cf_copro_bankmap[bindex] == 0xff) {
> > +		rc = -EIO;
> > +		goto bail;
> > +	}
> > +	gpio->cf_copro_bankmap[bindex]++;
> > +
> > +	/* Switch command source */
> > +	if (gpio->cf_copro_bankmap[bindex] == 1)
> > +		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
> > +					      GPIO_CMDSRC_COLDFIRE);
> > +
> > + bail:
> > +	spin_unlock_irqrestore(&gpio->lock, flags);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(aspeed_gpio_copro_grab_gpio);
> > +
> > +/**
> > + * aspeed_gpio_copro_release_gpio - Unmark a GPIO used by the 
> > coprocessor.
> > + * @desc: The GPIO to be marked
> > + */
> > +int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc)
> > +{
> > +	struct gpio_chip *chip = gpiod_to_chip(desc);
> > +	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
> > +	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
> > +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> > +	unsigned long flags;
> > +
> > +	if (!gpio->cf_copro_bankmap)
> > +		return -ENXIO;
> > +
> > +	if (offset < 0 || offset > gpio->config->nr_gpios)
> > +		return -EINVAL;
> > +	bindex = offset >> 3;
> > +
> > +	spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +	/* Sanity check, this shouldn't happen */
> > +	if (gpio->cf_copro_bankmap[bindex] == 0) {
> > +		rc = -EIO;
> > +		goto bail;
> > +	}
> > +	gpio->cf_copro_bankmap[bindex]--;
> > +
> > +	/* Switch command source */
> > +	if (gpio->cf_copro_bankmap[bindex] == 0)
> > +		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
> > +					      GPIO_CMDSRC_ARM);
> > + bail:
> > +	spin_unlock_irqrestore(&gpio->lock, flags);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(aspeed_gpio_copro_release_gpio);
> > +
> >  /*
> >   * Any banks not specified in a struct aspeed_bank_props array are 
> > assumed to
> >   * have the properties:
> > @@ -1002,10 +1189,18 @@ static int __init aspeed_gpio_probe(struct 
> > platform_device *pdev)
> >  	if (!gpio->dcache)
> >  		return -ENOMEM;
> >  
> > -	/* Populate it with initial values read from the HW */
> > +	/*
> > +	 * Populate it with initial values read from the HW and switch
> > +	 * all command sources to the ARM by default
> > +	 */
> >  	for (i = 0; i < banks; i++) {
> > -		void __iomem *addr = bank_reg(gpio, &aspeed_gpio_banks[i], reg_rdata);
> > +		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
> > +		void __iomem *addr = bank_reg(gpio, bank, reg_rdata);
> >  		gpio->dcache[i] = ioread32(addr);
> > +		aspeed_gpio_change_cmd_source(gpio, bank, 0, GPIO_CMDSRC_ARM);
> > +		aspeed_gpio_change_cmd_source(gpio, bank, 1, GPIO_CMDSRC_ARM);
> > +		aspeed_gpio_change_cmd_source(gpio, bank, 2, GPIO_CMDSRC_ARM);
> > +		aspeed_gpio_change_cmd_source(gpio, bank, 3, GPIO_CMDSRC_ARM);
> >  	}
> >  
> >  	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> > diff --git a/include/linux/gpio/aspeed.h b/include/linux/gpio/aspeed.h
> > new file mode 100644
> > index 000000000000..b6871c9b71f7
> > --- /dev/null
> > +++ b/include/linux/gpio/aspeed.h
> > @@ -0,0 +1,14 @@
> > +#ifndef __GPIO_ASPEED_H
> > +#define __GPIO_ASPEED_H
> > +
> > +struct aspeed_gpio_copro_ops {
> > +	int (*request_access)(void *data);
> > +	int (*release_access)(void *data);
> > +};
> > +
> > +int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc);
> > +int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc);
> > +int aspeed_gpio_copro_set_ops(const struct aspeed_gpio_copro_ops *ops, 
> > void *data);
> > +
> > +
> > +#endif /* __GPIO_ASPEED_H */
> > -- 
> > 2.17.1
> > 
> 
> LGTM otherwise, so I'll send a r-b tag if we can live with that one nit-pick.

[PATCH v2 4/4] gpio: aspeed: Add interfaces for co-processor to grab GPIOs

[Benjamin Herrenschmidt]

On Tue, 2018-06-19 at 00:38 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2018-06-18 at 23:53 +0930, Andrew Jeffery wrote:
> > 
> > >   static inline int irqd_to_aspeed_gpio_data(struct irq_data *d,
> > > -           struct aspeed_gpio **gpio,
> > > -           const struct aspeed_gpio_bank **bank,
> > > -           u32 *bit)
> > > +                                      struct aspeed_gpio **gpio,
> > > +                                      const struct aspeed_gpio_bank **bank,
> > > +                                      u32 *bit, int *offset)
> > >   {
> > > -   int offset;
> > >      struct aspeed_gpio *internal;
> > >   
> > > -   offset = irqd_to_hwirq(d);
> > > +   *offset = irqd_to_hwirq(d);
> > 
> > Nit: Did you intend to set this out parameter before potentially returning an error? I had tried to avoid that up until now.
> 
> Yeah it's constant-ish, I don't see why not

I mean it's a pure function of the input, it's not like there's any
special side effect that one may want to avoid in case of error.

Cheers,
Ben.

[PATCH v2 4/4] gpio: aspeed: Add interfaces for co-processor to grab GPIOs

[Andrew Jeffery]

On Tue, 19 Jun 2018, at 10:06, Benjamin Herrenschmidt wrote:
> On Tue, 2018-06-19 at 00:38 +1000, Benjamin Herrenschmidt wrote:
> > On Mon, 2018-06-18 at 23:53 +0930, Andrew Jeffery wrote:
> > > 
> > > >   static inline int irqd_to_aspeed_gpio_data(struct irq_data *d,
> > > > -           struct aspeed_gpio **gpio,
> > > > -           const struct aspeed_gpio_bank **bank,
> > > > -           u32 *bit)
> > > > +                                      struct aspeed_gpio **gpio,
> > > > +                                      const struct aspeed_gpio_bank **bank,
> > > > +                                      u32 *bit, int *offset)
> > > >   {
> > > > -   int offset;
> > > >      struct aspeed_gpio *internal;
> > > >   
> > > > -   offset = irqd_to_hwirq(d);
> > > > +   *offset = irqd_to_hwirq(d);
> > > 
> > > Nit: Did you intend to set this out parameter before potentially returning an error? I had tried to avoid that up until now.
> > 
> > Yeah it's constant-ish, I don't see why not
> 
> I mean it's a pure function of the input, it's not like there's any
> special side effect that one may want to avoid in case of error.

Right. Thanks for clarifying. Seems reasonable.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>