[PATCH v3 0/6] Add Aspeed G7 gpio support

[PATCH v3 0/6] Add Aspeed G7 gpio support

[Billy Tsai]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="yes", Size: 1584 bytes --]

The Aspeed 7th generation SoC features two GPIO controllers: one with 12
GPIO pins and another with 216 GPIO pins. The main difference from the
previous generation is that the control logic has been updated to support
per-pin control, allowing each pin to have its own 32-bit register for
configuring value, direction, interrupt type, and more.
This patch serial also add low-level operations (llops) to abstract the
register access for GPIO registers and the coprocessor request/release in
gpio-aspeed.c making it easier to extend the driver to support different
hardware register layouts.

Change since v2:
- Correct minItems for gpio-line names
- Remove the example for ast2700, because it’s the same as the AST2600
- Fix the sparse warning which is reported by the test robot
- Remove the version and use the match data to replace it.
- Add another two patches one for deferred probe one for flush write.

Changes since v1:
- Merge the gpio-aspeed-g7.c into the gpio-aspeed.c.
- Create the llops in gpio-aspeed.c for flexibility.

Billy Tsai (6):
  dt-bindings: gpio: aspeed,ast2400-gpio: Support ast2700
  gpio: aspeed: Remove the name for bank array
  gpio: aspeed: Create llops to handle hardware access
  gpio: aspeed: Support G7 Aspeed gpio controller
  gpio: aspeed: Change the macro to support deferred probe
  gpio: aspeed: Add the flush write to ensure the write complete.

 .../bindings/gpio/aspeed,ast2400-gpio.yaml    |  19 +-
 drivers/gpio/gpio-aspeed.c                    | 498 +++++++++++-------
 2 files changed, 313 insertions(+), 204 deletions(-)

-- 
2.25.1

[PATCH v3 1/6] dt-bindings: gpio: aspeed,ast2400-gpio: Support ast2700

[Billy Tsai]

The AST2700 is the 7th generation SoC from Aspeed, featuring two GPIO
controllers: one with 12 GPIO pins and another with 216 GPIO pins.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../bindings/gpio/aspeed,ast2400-gpio.yaml    | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
index cf11aa7ec8c7..b9afd07a9d24 100644
--- a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
@@ -15,6 +15,7 @@ properties:
       - aspeed,ast2400-gpio
       - aspeed,ast2500-gpio
       - aspeed,ast2600-gpio
+      - aspeed,ast2700-gpio
 
   reg:
     maxItems: 1
@@ -25,7 +26,7 @@ properties:
 
   gpio-controller: true
   gpio-line-names:
-    minItems: 36
+    minItems: 12
     maxItems: 232
 
   gpio-ranges: true
@@ -42,7 +43,7 @@ properties:
     const: 2
 
   ngpios:
-    minimum: 36
+    minimum: 12
     maximum: 232
 
 required:
@@ -93,6 +94,20 @@ allOf:
           enum: [ 36, 208 ]
       required:
         - ngpios
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2700-gpio
+    then:
+      properties:
+        gpio-line-names:
+          minItems: 12
+          maxItems: 216
+        ngpios:
+          enum: [ 12, 216 ]
+      required:
+        - ngpios
 
 additionalProperties: false
 
-- 
2.25.1

Re: [PATCH v3 1/6] dt-bindings: gpio: aspeed,ast2400-gpio: Support ast2700

[Conor Dooley]

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

On Fri, Sep 13, 2024 at 03:43:20PM +0800, Billy Tsai wrote:
> The AST2700 is the 7th generation SoC from Aspeed, featuring two GPIO
> controllers: one with 12 GPIO pins and another with 216 GPIO pins.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

[PATCH v3 2/6] gpio: aspeed: Remove the name for bank array

[Billy Tsai]

The bank array name is only used to determine if the GPIO offset is valid,
and this condition can be replaced by checking if the offset exceeds the
ngpio property.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/gpio/gpio-aspeed.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 04c03402db6d..d20e15b2079d 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -77,7 +77,6 @@ struct aspeed_gpio_bank {
 	uint16_t	debounce_regs;
 	uint16_t	tolerance_regs;
 	uint16_t	cmdsrc_regs;
-	const char	names[4][3];
 };
 
 /*
@@ -104,7 +103,6 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.debounce_regs = 0x0040,
 		.tolerance_regs = 0x001c,
 		.cmdsrc_regs = 0x0060,
-		.names = { "A", "B", "C", "D" },
 	},
 	{
 		.val_regs = 0x0020,
@@ -113,7 +111,6 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.debounce_regs = 0x0048,
 		.tolerance_regs = 0x003c,
 		.cmdsrc_regs = 0x0068,
-		.names = { "E", "F", "G", "H" },
 	},
 	{
 		.val_regs = 0x0070,
@@ -122,7 +119,6 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.debounce_regs = 0x00b0,
 		.tolerance_regs = 0x00ac,
 		.cmdsrc_regs = 0x0090,
-		.names = { "I", "J", "K", "L" },
 	},
 	{
 		.val_regs = 0x0078,
@@ -131,7 +127,6 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.debounce_regs = 0x0100,
 		.tolerance_regs = 0x00fc,
 		.cmdsrc_regs = 0x00e0,
-		.names = { "M", "N", "O", "P" },
 	},
 	{
 		.val_regs = 0x0080,
@@ -140,7 +135,6 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.debounce_regs = 0x0130,
 		.tolerance_regs = 0x012c,
 		.cmdsrc_regs = 0x0110,
-		.names = { "Q", "R", "S", "T" },
 	},
 	{
 		.val_regs = 0x0088,
@@ -149,7 +143,6 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.debounce_regs = 0x0160,
 		.tolerance_regs = 0x015c,
 		.cmdsrc_regs = 0x0140,
-		.names = { "U", "V", "W", "X" },
 	},
 	{
 		.val_regs = 0x01E0,
@@ -158,7 +151,6 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.debounce_regs = 0x0190,
 		.tolerance_regs = 0x018c,
 		.cmdsrc_regs = 0x0170,
-		.names = { "Y", "Z", "AA", "AB" },
 	},
 	{
 		.val_regs = 0x01e8,
@@ -167,7 +159,6 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.debounce_regs = 0x01c0,
 		.tolerance_regs = 0x01bc,
 		.cmdsrc_regs = 0x01a0,
-		.names = { "AC", "", "", "" },
 	},
 };
 
@@ -280,11 +271,11 @@ static inline const struct aspeed_bank_props *find_bank_props(
 static inline bool have_gpio(struct aspeed_gpio *gpio, unsigned int offset)
 {
 	const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
-	const struct aspeed_gpio_bank *bank = to_bank(offset);
-	unsigned int group = GPIO_OFFSET(offset) / 8;
 
-	return bank->names[group][0] != '\0' &&
-		(!props || ((props->input | props->output) & GPIO_BIT(offset)));
+	if (offset >= gpio->chip.ngpio)
+		return false;
+
+	return (!props || ((props->input | props->output) & GPIO_BIT(offset)));
 }
 
 static inline bool have_input(struct aspeed_gpio *gpio, unsigned int offset)
-- 
2.25.1

Re: [PATCH v3 2/6] gpio: aspeed: Remove the name for bank array

[Andrew Jeffery]

On Fri, 2024-09-13 at 15:43 +0800, Billy Tsai wrote:
> The bank array name is only used to determine if the GPIO offset is valid,
> and this condition can be replaced by checking if the offset exceeds the
> ngpio property.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

[PATCH v3 3/6] gpio: aspeed: Create llops to handle hardware access

[Billy Tsai]

Add low-level operations (llops) to abstract the register access for GPIO
registers and the coprocessor request/release. With this abstraction
layer, the driver can separate the hardware and software logic, making it
easier to extend the driver to support different hardware register
layouts.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/gpio/gpio-aspeed.c | 366 ++++++++++++++++++-------------------
 1 file changed, 178 insertions(+), 188 deletions(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index d20e15b2079d..40ca308054c6 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -39,6 +39,10 @@ struct aspeed_bank_props {
 struct aspeed_gpio_config {
 	unsigned int nr_gpios;
 	const struct aspeed_bank_props *props;
+	const struct aspeed_gpio_llops *llops;
+	const int *debounce_timers_array;
+	int debounce_timers_num;
+	bool cmd_source_supoort;
 };
 
 /*
@@ -178,6 +182,15 @@ enum aspeed_gpio_reg {
 	reg_cmdsrc1,
 };
 
+struct aspeed_gpio_llops {
+	bool (*copro_request)(struct aspeed_gpio *gpio, unsigned int offset);
+	void (*copro_release)(struct aspeed_gpio *gpio, unsigned int offset);
+	void (*reg_bits_set)(struct aspeed_gpio *gpio, unsigned int offset,
+			     const enum aspeed_gpio_reg reg, u32 val);
+	u32 (*reg_bits_read)(struct aspeed_gpio *gpio, unsigned int offset,
+			     const enum aspeed_gpio_reg reg);
+};
+
 #define GPIO_VAL_VALUE	0x00
 #define GPIO_VAL_DIR	0x04
 
@@ -237,10 +250,6 @@ static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
 #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)
-
 static const struct aspeed_gpio_bank *to_bank(unsigned int offset)
 {
 	unsigned int bank = GPIO_BANK(offset);
@@ -296,107 +305,56 @@ static inline bool have_output(struct aspeed_gpio *gpio, unsigned int offset)
 }
 
 static void aspeed_gpio_change_cmd_source(struct aspeed_gpio *gpio,
-					  const struct aspeed_gpio_bank *bank,
-					  int bindex, int cmdsrc)
+					  unsigned int offset,
+					  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).
+	 * The command source register is only valid in bits 0, 8, 16, and 24, so we use
+	 * (offset & ~(0x7)) to ensure that reg_bits_set always targets a valid bit.
 	 */
-	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);
-
+	gpio->config->llops->reg_bits_set(gpio, offset & ~(0x7), reg_cmdsrc1, !!(cmdsrc & BIT(1)));
 	/* Then Source 0 */
-	reg = ioread32(c0);
-	if (cmdsrc & 1)
-		reg |= bit;
-	else
-		reg &= ~bit;
-	iowrite32(reg, c0);
+	gpio->config->llops->reg_bits_set(gpio, offset & ~(0x7), reg_cmdsrc0, !!(cmdsrc & BIT(0)));
 }
 
 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;
+	if (gpio->config->llops->copro_request)
+		return gpio->config->llops->copro_request(gpio, offset);
 
-	/* 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;
+	return false;
 }
 
 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);
+	if (gpio->config->llops->copro_release)
+		gpio->config->llops->copro_release(gpio, offset);
 }
 
 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_reg(gpio, bank, reg_val)) & GPIO_BIT(offset));
+	return gpio->config->llops->reg_bits_read(gpio, offset, reg_val);
 }
 
 static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
 			      int val)
 {
 	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
-	const struct aspeed_gpio_bank *bank = to_bank(offset);
-	void __iomem *addr;
 	u32 reg;
 
-	addr = bank_reg(gpio, bank, reg_val);
 	reg = gpio->dcache[GPIO_BANK(offset)];
-
 	if (val)
 		reg |= GPIO_BIT(offset);
 	else
 		reg &= ~GPIO_BIT(offset);
 	gpio->dcache[GPIO_BANK(offset)] = reg;
 
-	iowrite32(reg, addr);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_val, val);
 }
 
 static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
@@ -404,7 +362,7 @@ 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;
+	bool copro = 0;
 
 	raw_spin_lock_irqsave(&gpio->lock, flags);
 	copro = aspeed_gpio_copro_request(gpio, offset);
@@ -419,22 +377,16 @@ static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
 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;
+	bool copro = 0;
 
 	if (!have_input(gpio, offset))
 		return -ENOTSUPP;
 
 	raw_spin_lock_irqsave(&gpio->lock, flags);
 
-	reg = ioread32(addr);
-	reg &= ~GPIO_BIT(offset);
-
 	copro = aspeed_gpio_copro_request(gpio, offset);
-	iowrite32(reg, addr);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_dir, 0);
 	if (copro)
 		aspeed_gpio_copro_release(gpio, offset);
 
@@ -447,23 +399,17 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
 			       unsigned int offset, int val)
 {
 	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;
+	bool copro = 0;
 
 	if (!have_output(gpio, offset))
 		return -ENOTSUPP;
 
 	raw_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);
-	iowrite32(reg, addr);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_dir, 1);
 
 	if (copro)
 		aspeed_gpio_copro_release(gpio, offset);
@@ -475,7 +421,6 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
 static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
 {
 	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
-	const struct aspeed_gpio_bank *bank = to_bank(offset);
 	unsigned long flags;
 	u32 val;
 
@@ -487,7 +432,7 @@ static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
 
 	raw_spin_lock_irqsave(&gpio->lock, flags);
 
-	val = ioread32(bank_reg(gpio, bank, reg_dir)) & GPIO_BIT(offset);
+	val = gpio->config->llops->reg_bits_read(gpio, offset, reg_dir);
 
 	raw_spin_unlock_irqrestore(&gpio->lock, flags);
 
@@ -496,8 +441,7 @@ 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, int *offset)
+					   int *offset)
 {
 	struct aspeed_gpio *internal;
 
@@ -510,32 +454,25 @@ static inline int irqd_to_aspeed_gpio_data(struct irq_data *d,
 		return -ENOTSUPP;
 
 	*gpio = internal;
-	*bank = to_bank(*offset);
-	*bit = GPIO_BIT(*offset);
 
 	return 0;
 }
 
 static void aspeed_gpio_irq_ack(struct irq_data *d)
 {
-	const struct aspeed_gpio_bank *bank;
 	struct aspeed_gpio *gpio;
 	unsigned long flags;
-	void __iomem *status_addr;
 	int rc, offset;
-	bool copro;
-	u32 bit;
+	bool copro = 0;
 
-	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
+	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
 	if (rc)
 		return;
 
-	status_addr = bank_reg(gpio, bank, reg_irq_status);
-
 	raw_spin_lock_irqsave(&gpio->lock, flags);
 	copro = aspeed_gpio_copro_request(gpio, offset);
 
-	iowrite32(bit, status_addr);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_status, 1);
 
 	if (copro)
 		aspeed_gpio_copro_release(gpio, offset);
@@ -544,20 +481,15 @@ static void aspeed_gpio_irq_ack(struct irq_data *d)
 
 static void aspeed_gpio_irq_set_mask(struct irq_data *d, bool set)
 {
-	const struct aspeed_gpio_bank *bank;
 	struct aspeed_gpio *gpio;
 	unsigned long flags;
-	u32 reg, bit;
-	void __iomem *addr;
 	int rc, offset;
-	bool copro;
+	bool copro = 0;
 
-	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
+	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
 	if (rc)
 		return;
 
-	addr = bank_reg(gpio, bank, reg_irq_enable);
-
 	/* Unmasking the IRQ */
 	if (set)
 		gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(d));
@@ -565,12 +497,7 @@ static void aspeed_gpio_irq_set_mask(struct irq_data *d, bool set)
 	raw_spin_lock_irqsave(&gpio->lock, flags);
 	copro = aspeed_gpio_copro_request(gpio, offset);
 
-	reg = ioread32(addr);
-	if (set)
-		reg |= bit;
-	else
-		reg &= ~bit;
-	iowrite32(reg, addr);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_enable, set);
 
 	if (copro)
 		aspeed_gpio_copro_release(gpio, offset);
@@ -596,34 +523,31 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
 	u32 type0 = 0;
 	u32 type1 = 0;
 	u32 type2 = 0;
-	u32 bit, reg;
-	const struct aspeed_gpio_bank *bank;
 	irq_flow_handler_t handler;
 	struct aspeed_gpio *gpio;
 	unsigned long flags;
-	void __iomem *addr;
 	int rc, offset;
-	bool copro;
+	bool copro = 0;
 
-	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
+	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
 	if (rc)
 		return -EINVAL;
 
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_EDGE_BOTH:
-		type2 |= bit;
+		type2 = 1;
 		fallthrough;
 	case IRQ_TYPE_EDGE_RISING:
-		type0 |= bit;
+		type0 = 1;
 		fallthrough;
 	case IRQ_TYPE_EDGE_FALLING:
 		handler = handle_edge_irq;
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		type0 |= bit;
+		type0 = 1;
 		fallthrough;
 	case IRQ_TYPE_LEVEL_LOW:
-		type1 |= bit;
+		type1 = 1;
 		handler = handle_level_irq;
 		break;
 	default:
@@ -633,20 +557,9 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
 	raw_spin_lock_irqsave(&gpio->lock, flags);
 	copro = aspeed_gpio_copro_request(gpio, offset);
 
-	addr = bank_reg(gpio, bank, reg_irq_type0);
-	reg = ioread32(addr);
-	reg = (reg & ~bit) | type0;
-	iowrite32(reg, addr);
-
-	addr = bank_reg(gpio, bank, reg_irq_type1);
-	reg = ioread32(addr);
-	reg = (reg & ~bit) | type1;
-	iowrite32(reg, addr);
-
-	addr = bank_reg(gpio, bank, reg_irq_type2);
-	reg = ioread32(addr);
-	reg = (reg & ~bit) | type2;
-	iowrite32(reg, addr);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_type0, type0);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_type1, type1);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_type2, type2);
 
 	if (copro)
 		aspeed_gpio_copro_release(gpio, offset);
@@ -661,7 +574,6 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
 {
 	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
 	struct irq_chip *ic = irq_desc_get_chip(desc);
-	struct aspeed_gpio *data = gpiochip_get_data(gc);
 	unsigned int i, p, banks;
 	unsigned long reg;
 	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
@@ -670,9 +582,7 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
 
 	banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
 	for (i = 0; i < banks; i++) {
-		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
-
-		reg = ioread32(bank_reg(data, bank, reg_irq_status));
+		reg = gpio->config->llops->reg_bits_read(gpio, i, reg_irq_status);
 
 		for_each_set_bit(p, &reg, 32)
 			generic_handle_domain_irq(gc->irq.domain, i * 32 + p);
@@ -711,23 +621,12 @@ 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);
+	bool copro = 0;
 
 	raw_spin_lock_irqsave(&gpio->lock, flags);
 	copro = aspeed_gpio_copro_request(gpio, offset);
 
-	val = readl(treg);
-
-	if (enable)
-		val |= GPIO_BIT(offset);
-	else
-		val &= ~GPIO_BIT(offset);
-
-	writel(val, treg);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_tolerance, enable);
 
 	if (copro)
 		aspeed_gpio_copro_release(gpio, offset);
@@ -821,21 +720,11 @@ static inline bool timer_allocation_registered(struct aspeed_gpio *gpio,
 static void configure_timer(struct aspeed_gpio *gpio, unsigned int offset,
 		unsigned int timer)
 {
-	const struct aspeed_gpio_bank *bank = to_bank(offset);
-	const u32 mask = GPIO_BIT(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);
-
-	addr = bank_reg(gpio, bank, reg_debounce_sel2);
-	val = ioread32(addr);
-	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE2(timer, offset), addr);
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_debounce_sel1, !!(timer & BIT(1)));
+	gpio->config->llops->reg_bits_set(gpio, offset, reg_debounce_sel2, !!(timer & BIT(0)));
 }
 
 static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
@@ -866,15 +755,15 @@ static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
 	}
 
 	/* Try to find a timer already configured for the debounce period */
-	for (i = 1; i < ARRAY_SIZE(debounce_timers); i++) {
+	for (i = 1; i < gpio->config->debounce_timers_num; i++) {
 		u32 cycles;
 
-		cycles = ioread32(gpio->base + debounce_timers[i]);
+		cycles = ioread32(gpio->base + gpio->config->debounce_timers_array[i]);
 		if (requested_cycles == cycles)
 			break;
 	}
 
-	if (i == ARRAY_SIZE(debounce_timers)) {
+	if (i == gpio->config->debounce_timers_num) {
 		int j;
 
 		/*
@@ -888,8 +777,8 @@ static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
 
 		if (j == ARRAY_SIZE(gpio->timer_users)) {
 			dev_warn(chip->parent,
-					"Debounce timers exhausted, cannot debounce for period %luus\n",
-					usecs);
+				 "Debounce timers exhausted, cannot debounce for period %luus\n",
+				 usecs);
 
 			rc = -EPERM;
 
@@ -905,7 +794,7 @@ static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
 
 		i = j;
 
-		iowrite32(requested_cycles, gpio->base + debounce_timers[i]);
+		iowrite32(requested_cycles, gpio->base + gpio->config->debounce_timers_array[i]);
 	}
 
 	if (WARN(i == 0, "Cannot register index of disabled timer\n")) {
@@ -1008,6 +897,9 @@ int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc,
 	const struct aspeed_gpio_bank *bank = to_bank(offset);
 	unsigned long flags;
 
+	if (!gpio->config->cmd_source_supoort)
+		return -EOPNOTSUPP;
+
 	if (!gpio->cf_copro_bankmap)
 		gpio->cf_copro_bankmap = kzalloc(gpio->chip.ngpio >> 3, GFP_KERNEL);
 	if (!gpio->cf_copro_bankmap)
@@ -1027,7 +919,7 @@ int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc,
 
 	/* Switch command source */
 	if (gpio->cf_copro_bankmap[bindex] == 1)
-		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
+		aspeed_gpio_change_cmd_source(gpio, offset,
 					      GPIO_CMDSRC_COLDFIRE);
 
 	if (vreg_offset)
@@ -1051,9 +943,11 @@ 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->config->cmd_source_supoort)
+		return -EOPNOTSUPP;
+
 	if (!gpio->cf_copro_bankmap)
 		return -ENXIO;
 
@@ -1072,7 +966,7 @@ int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc)
 
 	/* Switch command source */
 	if (gpio->cf_copro_bankmap[bindex] == 0)
-		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
+		aspeed_gpio_change_cmd_source(gpio, offset,
 					      GPIO_CMDSRC_ARM);
  bail:
 	raw_spin_unlock_irqrestore(&gpio->lock, flags);
@@ -1082,12 +976,10 @@ EXPORT_SYMBOL_GPL(aspeed_gpio_copro_release_gpio);
 
 static void aspeed_gpio_irq_print_chip(struct irq_data *d, struct seq_file *p)
 {
-	const struct aspeed_gpio_bank *bank;
 	struct aspeed_gpio *gpio;
-	u32 bit;
 	int rc, offset;
 
-	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
+	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
 	if (rc)
 		return;
 
@@ -1104,6 +996,78 @@ static const struct irq_chip aspeed_gpio_irq_chip = {
 	GPIOCHIP_IRQ_RESOURCE_HELPERS,
 };
 
+static void aspeed_g4_reg_bits_set(struct aspeed_gpio *gpio, unsigned int offset,
+				   const enum aspeed_gpio_reg reg, u32 val)
+{
+	const struct aspeed_gpio_bank *bank = to_bank(offset);
+	void __iomem *addr = bank_reg(gpio, bank, reg);
+	u32 temp;
+
+	temp = ioread32(addr);
+	if (val)
+		temp |= GPIO_BIT(offset);
+	else
+		temp &= ~GPIO_BIT(offset);
+
+	iowrite32(temp, addr);
+}
+
+static u32 aspeed_g4_reg_bits_read(struct aspeed_gpio *gpio, unsigned int offset,
+				   const enum aspeed_gpio_reg reg)
+{
+	const struct aspeed_gpio_bank *bank = to_bank(offset);
+	void __iomem *addr = bank_reg(gpio, bank, reg);
+
+	if (reg == reg_rdata)
+		return ioread32(addr);
+	return !!(ioread32(addr) & GPIO_BIT(offset));
+}
+
+static bool aspeed_g4_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, offset, GPIO_CMDSRC_ARM);
+
+	/* Update cache */
+	gpio->dcache[GPIO_BANK(offset)] = ioread32(bank_reg(gpio, bank, reg_rdata));
+
+	return true;
+}
+
+static void aspeed_g4_copro_release(struct aspeed_gpio *gpio, unsigned int 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, offset, GPIO_CMDSRC_COLDFIRE);
+
+	/* Restart the coprocessor */
+	copro_ops->release_access(copro_data);
+}
+
+static const struct aspeed_gpio_llops aspeed_g4_llops = {
+	.copro_request = aspeed_g4_copro_request,
+	.copro_release = aspeed_g4_copro_release,
+	.reg_bits_set = aspeed_g4_reg_bits_set,
+	.reg_bits_read = aspeed_g4_reg_bits_read,
+};
 /*
  * Any banks not specified in a struct aspeed_bank_props array are assumed to
  * have the properties:
@@ -1120,7 +1084,14 @@ static const struct aspeed_bank_props ast2400_bank_props[] = {
 
 static const struct aspeed_gpio_config ast2400_config =
 	/* 220 for simplicity, really 216 with two 4-GPIO holes, four at end */
-	{ .nr_gpios = 220, .props = ast2400_bank_props, };
+	{
+		.nr_gpios = 220,
+		.props = ast2400_bank_props,
+		.llops = &aspeed_g4_llops,
+		.debounce_timers_array = debounce_timers,
+		.debounce_timers_num = ARRAY_SIZE(debounce_timers),
+		.cmd_source_supoort = true,
+	};
 
 static const struct aspeed_bank_props ast2500_bank_props[] = {
 	/*     input	  output   */
@@ -1132,7 +1103,14 @@ static const struct aspeed_bank_props ast2500_bank_props[] = {
 
 static const struct aspeed_gpio_config ast2500_config =
 	/* 232 for simplicity, actual number is 228 (4-GPIO hole in GPIOAB) */
-	{ .nr_gpios = 232, .props = ast2500_bank_props, };
+	{
+		.nr_gpios = 232,
+		.props = ast2500_bank_props,
+		.llops = &aspeed_g4_llops,
+		.debounce_timers_array = debounce_timers,
+		.debounce_timers_num = ARRAY_SIZE(debounce_timers),
+		.cmd_source_supoort = true,
+	};
 
 static const struct aspeed_bank_props ast2600_bank_props[] = {
 	/*     input	  output   */
@@ -1148,7 +1126,14 @@ static const struct aspeed_gpio_config ast2600_config =
 	 * We expect ngpio being set in the device tree and this is a fallback
 	 * option.
 	 */
-	{ .nr_gpios = 208, .props = ast2600_bank_props, };
+	{
+		.nr_gpios = 208,
+		.props = ast2600_bank_props,
+		.llops = &aspeed_g4_llops,
+		.debounce_timers_array = debounce_timers,
+		.debounce_timers_num = ARRAY_SIZE(debounce_timers),
+		.cmd_source_supoort = true,
+	};
 
 static const struct of_device_id aspeed_gpio_of_table[] = {
 	{ .compatible = "aspeed,ast2400-gpio", .data = &ast2400_config, },
@@ -1191,6 +1176,9 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
 
 	gpio->config = gpio_id->data;
 
+	if (!gpio->config->llops->reg_bits_set || !gpio->config->llops->reg_bits_read)
+		return -EOPNOTSUPP;
+
 	gpio->chip.parent = &pdev->dev;
 	err = of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpio);
 	gpio->chip.ngpio = (u16) ngpio;
@@ -1218,15 +1206,17 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
 	 * 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++) {
-		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);
-	}
+	if (gpio->config->cmd_source_supoort)
+		for (i = 0; i < banks; i++) {
+			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, i * 8 + 0, GPIO_CMDSRC_ARM);
+			aspeed_gpio_change_cmd_source(gpio, i * 8 + 8, GPIO_CMDSRC_ARM);
+			aspeed_gpio_change_cmd_source(gpio, i * 8 + 16, GPIO_CMDSRC_ARM);
+			aspeed_gpio_change_cmd_source(gpio, i * 8 + 24, GPIO_CMDSRC_ARM);
+		}
 
 	/* Set up an irqchip */
 	irq = platform_get_irq(pdev, 0);
-- 
2.25.1

Re: [PATCH v3 3/6] gpio: aspeed: Create llops to handle hardware access

[Andrew Jeffery]

On Fri, 2024-09-13 at 15:43 +0800, Billy Tsai wrote:
> Add low-level operations (llops) to abstract the register access for GPIO
> registers and the coprocessor request/release. With this abstraction
> layer, the driver can separate the hardware and software logic, making it
> easier to extend the driver to support different hardware register
> layouts.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  drivers/gpio/gpio-aspeed.c | 366 ++++++++++++++++++-------------------
>  1 file changed, 178 insertions(+), 188 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index d20e15b2079d..40ca308054c6 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -39,6 +39,10 @@ struct aspeed_bank_props {
>  struct aspeed_gpio_config {
>  	unsigned int nr_gpios;
>  	const struct aspeed_bank_props *props;
> +	const struct aspeed_gpio_llops *llops;
> +	const int *debounce_timers_array;
> +	int debounce_timers_num;
> +	bool cmd_source_supoort;

Typo: s/cmd_source_supoort/cmd_source_support/

However, this is a derived value. It can be inferred like:

   static bool aspeed_gpio_supports_cmd_source(const struct aspeed_gpio_config *config)
   {
       return config->llops->copro_request && config->llops->coproc_release;
   }

Can we do that instead? That removes the opportunity for the member
values to become unsynchronised in bad ways.

>  };
>  
>  /*
> @@ -178,6 +182,15 @@ enum aspeed_gpio_reg {
>  	reg_cmdsrc1,
>  };
>  
> +struct aspeed_gpio_llops {
> +	bool (*copro_request)(struct aspeed_gpio *gpio, unsigned int offset);
> +	void (*copro_release)(struct aspeed_gpio *gpio, unsigned int offset);
> +	void (*reg_bits_set)(struct aspeed_gpio *gpio, unsigned int offset,
> +			     const enum aspeed_gpio_reg reg, u32 val);
> +	u32 (*reg_bits_read)(struct aspeed_gpio *gpio, unsigned int offset,

Guess I should have commented on this on v2, but maybe call this
`reg_bits_get()` for symmetry with `reg_bits_set()`?

I also have a comment on the use of "bits" in the member names below.

> +			     const enum aspeed_gpio_reg reg);
> +};
> +
>  #define GPIO_VAL_VALUE	0x00
>  #define GPIO_VAL_DIR	0x04
>  
> @@ -237,10 +250,6 @@ static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
>  #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)
> -
>  static const struct aspeed_gpio_bank *to_bank(unsigned int offset)
>  {
>  	unsigned int bank = GPIO_BANK(offset);
> @@ -296,107 +305,56 @@ static inline bool have_output(struct aspeed_gpio *gpio, unsigned int offset)
>  }
>  
>  static void aspeed_gpio_change_cmd_source(struct aspeed_gpio *gpio,
> -					  const struct aspeed_gpio_bank *bank,
> -					  int bindex, int cmdsrc)
> +					  unsigned int offset,
> +					  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).
> +	 * The command source register is only valid in bits 0, 8, 16, and 24, so we use
> +	 * (offset & ~(0x7)) to ensure that reg_bits_set always targets a valid bit.
>  	 */
> -	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);
> -
> +	gpio->config->llops->reg_bits_set(gpio, offset & ~(0x7), reg_cmdsrc1, !!(cmdsrc & BIT(1)));
>  	/* Then Source 0 */
> -	reg = ioread32(c0);
> -	if (cmdsrc & 1)
> -		reg |= bit;
> -	else
> -		reg &= ~bit;
> -	iowrite32(reg, c0);
> +	gpio->config->llops->reg_bits_set(gpio, offset & ~(0x7), reg_cmdsrc0, !!(cmdsrc & BIT(0)));
>  }
>  
>  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;
> +	if (gpio->config->llops->copro_request)
> +		return gpio->config->llops->copro_request(gpio, offset);
>  
> -	/* 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;
> +	return false;
>  }
>  
>  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);
> +	if (gpio->config->llops->copro_release)
> +		gpio->config->llops->copro_release(gpio, offset);
>  }
>  
>  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_reg(gpio, bank, reg_val)) & GPIO_BIT(offset));
> +	return gpio->config->llops->reg_bits_read(gpio, offset, reg_val);
>  }
>  
>  static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
>  			      int val)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
> -	void __iomem *addr;
>  	u32 reg;
>  
> -	addr = bank_reg(gpio, bank, reg_val);
>  	reg = gpio->dcache[GPIO_BANK(offset)];
> -
>  	if (val)
>  		reg |= GPIO_BIT(offset);
>  	else
>  		reg &= ~GPIO_BIT(offset);
>  	gpio->dcache[GPIO_BANK(offset)] = reg;
>  
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_val, val);
>  }
>  
>  static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
> @@ -404,7 +362,7 @@ 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;
> +	bool copro = 0;

Isn't this a dead store? If it's not, can we use `false` instead to
align with the `bool` type?

If it is a dead store can you please address the rest of the instances
throughout the patch?

>  
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
> @@ -419,22 +377,16 @@ static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
>  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;
> +	bool copro = 0;
>  
>  	if (!have_input(gpio, offset))
>  		return -ENOTSUPP;
>  
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  
> -	reg = ioread32(addr);
> -	reg &= ~GPIO_BIT(offset);
> -
>  	copro = aspeed_gpio_copro_request(gpio, offset);
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_dir, 0);
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
>  
> @@ -447,23 +399,17 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
>  			       unsigned int offset, int val)
>  {
>  	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;
> +	bool copro = 0;
>  
>  	if (!have_output(gpio, offset))
>  		return -ENOTSUPP;
>  
>  	raw_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);
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_dir, 1);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -475,7 +421,6 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
>  static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
>  	unsigned long flags;
>  	u32 val;
>  
> @@ -487,7 +432,7 @@ static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
>  
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  
> -	val = ioread32(bank_reg(gpio, bank, reg_dir)) & GPIO_BIT(offset);
> +	val = gpio->config->llops->reg_bits_read(gpio, offset, reg_dir);
>  
>  	raw_spin_unlock_irqrestore(&gpio->lock, flags);
>  
> @@ -496,8 +441,7 @@ 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, int *offset)
> +					   int *offset)
>  {
>  	struct aspeed_gpio *internal;
>  
> @@ -510,32 +454,25 @@ static inline int irqd_to_aspeed_gpio_data(struct irq_data *d,
>  		return -ENOTSUPP;
>  
>  	*gpio = internal;
> -	*bank = to_bank(*offset);
> -	*bit = GPIO_BIT(*offset);
>  
>  	return 0;
>  }
>  
>  static void aspeed_gpio_irq_ack(struct irq_data *d)
>  {
> -	const struct aspeed_gpio_bank *bank;
>  	struct aspeed_gpio *gpio;
>  	unsigned long flags;
> -	void __iomem *status_addr;
>  	int rc, offset;
> -	bool copro;
> -	u32 bit;
> +	bool copro = 0;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
>  	if (rc)
>  		return;
>  
> -	status_addr = bank_reg(gpio, bank, reg_irq_status);
> -
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
>  
> -	iowrite32(bit, status_addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_status, 1);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -544,20 +481,15 @@ static void aspeed_gpio_irq_ack(struct irq_data *d)
>  
>  static void aspeed_gpio_irq_set_mask(struct irq_data *d, bool set)
>  {
> -	const struct aspeed_gpio_bank *bank;
>  	struct aspeed_gpio *gpio;
>  	unsigned long flags;
> -	u32 reg, bit;
> -	void __iomem *addr;
>  	int rc, offset;
> -	bool copro;
> +	bool copro = 0;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
>  	if (rc)
>  		return;
>  
> -	addr = bank_reg(gpio, bank, reg_irq_enable);
> -
>  	/* Unmasking the IRQ */
>  	if (set)
>  		gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(d));
> @@ -565,12 +497,7 @@ static void aspeed_gpio_irq_set_mask(struct irq_data *d, bool set)
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
>  
> -	reg = ioread32(addr);
> -	if (set)
> -		reg |= bit;
> -	else
> -		reg &= ~bit;
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_enable, set);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -596,34 +523,31 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
>  	u32 type0 = 0;
>  	u32 type1 = 0;
>  	u32 type2 = 0;
> -	u32 bit, reg;
> -	const struct aspeed_gpio_bank *bank;
>  	irq_flow_handler_t handler;
>  	struct aspeed_gpio *gpio;
>  	unsigned long flags;
> -	void __iomem *addr;
>  	int rc, offset;
> -	bool copro;
> +	bool copro = 0;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
>  	if (rc)
>  		return -EINVAL;
>  
>  	switch (type & IRQ_TYPE_SENSE_MASK) {
>  	case IRQ_TYPE_EDGE_BOTH:
> -		type2 |= bit;
> +		type2 = 1;
>  		fallthrough;
>  	case IRQ_TYPE_EDGE_RISING:
> -		type0 |= bit;
> +		type0 = 1;
>  		fallthrough;
>  	case IRQ_TYPE_EDGE_FALLING:
>  		handler = handle_edge_irq;
>  		break;
>  	case IRQ_TYPE_LEVEL_HIGH:
> -		type0 |= bit;
> +		type0 = 1;
>  		fallthrough;
>  	case IRQ_TYPE_LEVEL_LOW:
> -		type1 |= bit;
> +		type1 = 1;
>  		handler = handle_level_irq;
>  		break;
>  	default:
> @@ -633,20 +557,9 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
>  
> -	addr = bank_reg(gpio, bank, reg_irq_type0);
> -	reg = ioread32(addr);
> -	reg = (reg & ~bit) | type0;
> -	iowrite32(reg, addr);
> -
> -	addr = bank_reg(gpio, bank, reg_irq_type1);
> -	reg = ioread32(addr);
> -	reg = (reg & ~bit) | type1;
> -	iowrite32(reg, addr);
> -
> -	addr = bank_reg(gpio, bank, reg_irq_type2);
> -	reg = ioread32(addr);
> -	reg = (reg & ~bit) | type2;
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_type0, type0);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_type1, type1);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_type2, type2);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -661,7 +574,6 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
>  {
>  	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
>  	struct irq_chip *ic = irq_desc_get_chip(desc);
> -	struct aspeed_gpio *data = gpiochip_get_data(gc);
>  	unsigned int i, p, banks;
>  	unsigned long reg;
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> @@ -670,9 +582,7 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
>  
>  	banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
>  	for (i = 0; i < banks; i++) {
> -		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
> -
> -		reg = ioread32(bank_reg(data, bank, reg_irq_status));
> +		reg = gpio->config->llops->reg_bits_read(gpio, i, reg_irq_status);
>  
>  		for_each_set_bit(p, &reg, 32)
>  			generic_handle_domain_irq(gc->irq.domain, i * 32 + p);
> @@ -711,23 +621,12 @@ 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);
> +	bool copro = 0;
>  
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
>  
> -	val = readl(treg);
> -
> -	if (enable)
> -		val |= GPIO_BIT(offset);
> -	else
> -		val &= ~GPIO_BIT(offset);
> -
> -	writel(val, treg);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_tolerance, enable);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -821,21 +720,11 @@ static inline bool timer_allocation_registered(struct aspeed_gpio *gpio,
>  static void configure_timer(struct aspeed_gpio *gpio, unsigned int offset,
>  		unsigned int timer)
>  {
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
> -	const u32 mask = GPIO_BIT(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);
> -
> -	addr = bank_reg(gpio, bank, reg_debounce_sel2);
> -	val = ioread32(addr);
> -	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE2(timer, offset), addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_debounce_sel1, !!(timer & BIT(1)));
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_debounce_sel2, !!(timer & BIT(0)));
>  }
>  
>  static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
> @@ -866,15 +755,15 @@ static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
>  	}
>  
>  	/* Try to find a timer already configured for the debounce period */
> -	for (i = 1; i < ARRAY_SIZE(debounce_timers); i++) {
> +	for (i = 1; i < gpio->config->debounce_timers_num; i++) {
>  		u32 cycles;
>  
> -		cycles = ioread32(gpio->base + debounce_timers[i]);
> +		cycles = ioread32(gpio->base + gpio->config->debounce_timers_array[i]);
>  		if (requested_cycles == cycles)
>  			break;
>  	}
>  
> -	if (i == ARRAY_SIZE(debounce_timers)) {
> +	if (i == gpio->config->debounce_timers_num) {
>  		int j;
>  
>  		/*
> @@ -888,8 +777,8 @@ static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
>  
>  		if (j == ARRAY_SIZE(gpio->timer_users)) {
>  			dev_warn(chip->parent,
> -					"Debounce timers exhausted, cannot debounce for period %luus\n",
> -					usecs);
> +				 "Debounce timers exhausted, cannot debounce for period %luus\n",
> +				 usecs);
>  
>  			rc = -EPERM;
>  
> @@ -905,7 +794,7 @@ static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
>  
>  		i = j;
>  
> -		iowrite32(requested_cycles, gpio->base + debounce_timers[i]);
> +		iowrite32(requested_cycles, gpio->base + gpio->config->debounce_timers_array[i]);
>  	}
>  
>  	if (WARN(i == 0, "Cannot register index of disabled timer\n")) {
> @@ -1008,6 +897,9 @@ int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc,
>  	const struct aspeed_gpio_bank *bank = to_bank(offset);
>  	unsigned long flags;
>  
> +	if (!gpio->config->cmd_source_supoort)
> +		return -EOPNOTSUPP;
> +
>  	if (!gpio->cf_copro_bankmap)
>  		gpio->cf_copro_bankmap = kzalloc(gpio->chip.ngpio >> 3, GFP_KERNEL);
>  	if (!gpio->cf_copro_bankmap)
> @@ -1027,7 +919,7 @@ int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc,
>  
>  	/* Switch command source */
>  	if (gpio->cf_copro_bankmap[bindex] == 1)
> -		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
> +		aspeed_gpio_change_cmd_source(gpio, offset,
>  					      GPIO_CMDSRC_COLDFIRE);
>  
>  	if (vreg_offset)
> @@ -1051,9 +943,11 @@ 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->config->cmd_source_supoort)
> +		return -EOPNOTSUPP;
> +
>  	if (!gpio->cf_copro_bankmap)
>  		return -ENXIO;
>  
> @@ -1072,7 +966,7 @@ int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc)
>  
>  	/* Switch command source */
>  	if (gpio->cf_copro_bankmap[bindex] == 0)
> -		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
> +		aspeed_gpio_change_cmd_source(gpio, offset,
>  					      GPIO_CMDSRC_ARM);
>   bail:
>  	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> @@ -1082,12 +976,10 @@ EXPORT_SYMBOL_GPL(aspeed_gpio_copro_release_gpio);
>  
>  static void aspeed_gpio_irq_print_chip(struct irq_data *d, struct seq_file *p)
>  {
> -	const struct aspeed_gpio_bank *bank;
>  	struct aspeed_gpio *gpio;
> -	u32 bit;
>  	int rc, offset;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
>  	if (rc)
>  		return;
>  
> @@ -1104,6 +996,78 @@ static const struct irq_chip aspeed_gpio_irq_chip = {
>  	GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>  
> +static void aspeed_g4_reg_bits_set(struct aspeed_gpio *gpio, unsigned int offset,
> +				   const enum aspeed_gpio_reg reg, u32 val)
> +{
> +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +	void __iomem *addr = bank_reg(gpio, bank, reg);
> +	u32 temp;
> +
> +	temp = ioread32(addr);
> +	if (val)
> +		temp |= GPIO_BIT(offset);
> +	else
> +		temp &= ~GPIO_BIT(offset);

Perhaps the type of `val` should be `bool` for now to indicate it can
only handle a single bit? Maybe "_bits_" in the function names is
unhelpful too, and that should be just "_bit_"?

> +
> +	iowrite32(temp, addr);
> +}
> +
> +static u32 aspeed_g4_reg_bits_read(struct aspeed_gpio *gpio, unsigned int offset,
> +				   const enum aspeed_gpio_reg reg)
> +{
> +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +	void __iomem *addr = bank_reg(gpio, bank, reg);
> +
> +	if (reg == reg_rdata)
> +		return ioread32(addr);
> +	return !!(ioread32(addr) & GPIO_BIT(offset));
> +}
> +
> +static bool aspeed_g4_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, offset, GPIO_CMDSRC_ARM);
> +
> +	/* Update cache */
> +	gpio->dcache[GPIO_BANK(offset)] = ioread32(bank_reg(gpio, bank, reg_rdata));
> +
> +	return true;
> +}
> +
> +static void aspeed_g4_copro_release(struct aspeed_gpio *gpio, unsigned int 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, offset, GPIO_CMDSRC_COLDFIRE);
> +
> +	/* Restart the coprocessor */
> +	copro_ops->release_access(copro_data);
> +}
> +
> +static const struct aspeed_gpio_llops aspeed_g4_llops = {
> +	.copro_request = aspeed_g4_copro_request,
> +	.copro_release = aspeed_g4_copro_release,
> +	.reg_bits_set = aspeed_g4_reg_bits_set,
> +	.reg_bits_read = aspeed_g4_reg_bits_read,
> +};
>  /*
>   * Any banks not specified in a struct aspeed_bank_props array are assumed to
>   * have the properties:
> @@ -1120,7 +1084,14 @@ static const struct aspeed_bank_props ast2400_bank_props[] = {
>  
>  static const struct aspeed_gpio_config ast2400_config =
>  	/* 220 for simplicity, really 216 with two 4-GPIO holes, four at end */
> -	{ .nr_gpios = 220, .props = ast2400_bank_props, };
> +	{
> +		.nr_gpios = 220,
> +		.props = ast2400_bank_props,
> +		.llops = &aspeed_g4_llops,
> +		.debounce_timers_array = debounce_timers,
> +		.debounce_timers_num = ARRAY_SIZE(debounce_timers),
> +		.cmd_source_supoort = true,
> +	};
>  
>  static const struct aspeed_bank_props ast2500_bank_props[] = {
>  	/*     input	  output   */
> @@ -1132,7 +1103,14 @@ static const struct aspeed_bank_props ast2500_bank_props[] = {
>  
>  static const struct aspeed_gpio_config ast2500_config =
>  	/* 232 for simplicity, actual number is 228 (4-GPIO hole in GPIOAB) */
> -	{ .nr_gpios = 232, .props = ast2500_bank_props, };
> +	{
> +		.nr_gpios = 232,
> +		.props = ast2500_bank_props,
> +		.llops = &aspeed_g4_llops,
> +		.debounce_timers_array = debounce_timers,
> +		.debounce_timers_num = ARRAY_SIZE(debounce_timers),
> +		.cmd_source_supoort = true,
> +	};
>  
>  static const struct aspeed_bank_props ast2600_bank_props[] = {
>  	/*     input	  output   */
> @@ -1148,7 +1126,14 @@ static const struct aspeed_gpio_config ast2600_config =
>  	 * We expect ngpio being set in the device tree and this is a fallback
>  	 * option.
>  	 */
> -	{ .nr_gpios = 208, .props = ast2600_bank_props, };
> +	{
> +		.nr_gpios = 208,
> +		.props = ast2600_bank_props,
> +		.llops = &aspeed_g4_llops,
> +		.debounce_timers_array = debounce_timers,
> +		.debounce_timers_num = ARRAY_SIZE(debounce_timers),
> +		.cmd_source_supoort = true,
> +	};
>  
>  static const struct of_device_id aspeed_gpio_of_table[] = {
>  	{ .compatible = "aspeed,ast2400-gpio", .data = &ast2400_config, },
> @@ -1191,6 +1176,9 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
>  
>  	gpio->config = gpio_id->data;
>  
> +	if (!gpio->config->llops->reg_bits_set || !gpio->config->llops->reg_bits_read)
> +		return -EOPNOTSUPP;
> +

Hmm, not quite sure about -EOPNOTSUPP. -EINVAL might be better. Given
it's a programming error I think it deserves a log message as well, as
ultimately it should never be hit, and if it is we want to clear up the
confusion quickly.

Andrew

Re: [PATCH v3 3/6] gpio: aspeed: Create llops to handle hardware access

[Andrew Jeffery]

On Fri, 2024-09-13 at 15:43 +0800, Billy Tsai wrote:
> 
> @@ -1218,15 +1206,17 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
>  	 * 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++) {
> -		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);
> -	}
> +	if (gpio->config->cmd_source_supoort)

This test only reflects the level of effort put into supporting the
AST2700 GPIO controllers so far, it doesn't reflect the actual
capability of the hardware...

> +		for (i = 0; i < banks; i++) {
> +			const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
> +			void __iomem *addr = bank_reg(gpio, bank, reg_rdata);

... and I don' think this is correct if/when the command source support
is implemented for the AST2700. Can you please add a comment about the
bank/address calculation, and change the condition above to account for
this implementation being specific to SoCs earlier than the AST2700?

Andrew

> +
> +			gpio->dcache[i] = ioread32(addr);
> +			aspeed_gpio_change_cmd_source(gpio, i * 8 + 0, GPIO_CMDSRC_ARM);
> +			aspeed_gpio_change_cmd_source(gpio, i * 8 + 8, GPIO_CMDSRC_ARM);
> +			aspeed_gpio_change_cmd_source(gpio, i * 8 + 16, GPIO_CMDSRC_ARM);
> +			aspeed_gpio_change_cmd_source(gpio, i * 8 + 24, GPIO_CMDSRC_ARM);
> +		}
>  
>  	/* Set up an irqchip */
>  	irq = platform_get_irq(pdev, 0);

Re: [PATCH v3 3/6] gpio: aspeed: Create llops to handle hardware access

[Billy Tsai]

Hi Andrew,

I will add the following change on the next patch:

1. Add `privilege_ctrl` and `privilege_init` callback
2. Use `bool aspeed_gpio_support_copro()` api to replace the `cmd_source_supoort` flag
3. Add the `dcache_require` flag and move the dcache usage into the `reg_bit_set` callback
4. `reg_bits_set` -> `reg_bit_set` and `reg_bits_read` -> `reg_bits_get`
5. `bool copro = 0` -> `bool copro = false`
6. if (!gpio->config->llops->reg_bit_set || !gpio->config->llops->reg_bits_get) return -EINVAL;

Thanks for your comments.

[PATCH v3 4/6] gpio: aspeed: Support G7 Aspeed gpio controller

[Billy Tsai]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="yes", Size: 5394 bytes --]

In the 7th generation of the SoC from Aspeed, the control logic of the
GPIO controller has been updated to support per-pin control. Each pin now
has its own 32-bit register, allowing for individual control of the pin’s
value, direction, interrupt type, and other settings. The permission for
coprocessor access is supported by the hardware but hasn’t been
implemented in the current patch.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/gpio/gpio-aspeed.c | 110 +++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 40ca308054c6..13495442e1aa 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -30,6 +30,23 @@
 #include <linux/gpio/consumer.h>
 #include "gpiolib.h"
 
+#define GPIO_G7_IRQ_STS_BASE 0x100
+#define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
+#define GPIO_G7_CTRL_REG_BASE 0x180
+#define GPIO_G7_CTRL_REG_OFFSET(x) (GPIO_G7_CTRL_REG_BASE + (x) * 0x4)
+#define GPIO_G7_CTRL_OUT_DATA BIT(0)
+#define GPIO_G7_CTRL_DIR BIT(1)
+#define GPIO_G7_CTRL_IRQ_EN BIT(2)
+#define GPIO_G7_CTRL_IRQ_TYPE0 BIT(3)
+#define GPIO_G7_CTRL_IRQ_TYPE1 BIT(4)
+#define GPIO_G7_CTRL_IRQ_TYPE2 BIT(5)
+#define GPIO_G7_CTRL_RST_TOLERANCE BIT(6)
+#define GPIO_G7_CTRL_DEBOUNCE_SEL2 BIT(7)
+#define GPIO_G7_CTRL_DEBOUNCE_SEL1 BIT(8)
+#define GPIO_G7_CTRL_INPUT_MASK BIT(9)
+#define GPIO_G7_CTRL_IRQ_STS BIT(12)
+#define GPIO_G7_CTRL_IN_DATA BIT(13)
+
 struct aspeed_bank_props {
 	unsigned int bank;
 	u32 input;
@@ -95,6 +112,7 @@ struct aspeed_gpio_bank {
  */
 
 static const int debounce_timers[4] = { 0x00, 0x50, 0x54, 0x58 };
+static const int g7_debounce_timers[4] = { 0x00, 0x04, 0x00, 0x08 };
 
 static const struct aspeed_gpio_copro_ops *copro_ops;
 static void *copro_data;
@@ -246,6 +264,39 @@ static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
 	BUG();
 }
 
+static inline u32 reg_mask(const enum aspeed_gpio_reg reg)
+{
+	switch (reg) {
+	case reg_val:
+		return GPIO_G7_CTRL_OUT_DATA;
+	case reg_dir:
+		return GPIO_G7_CTRL_DIR;
+	case reg_irq_enable:
+		return GPIO_G7_CTRL_IRQ_EN;
+	case reg_irq_type0:
+		return GPIO_G7_CTRL_IRQ_TYPE0;
+	case reg_irq_type1:
+		return GPIO_G7_CTRL_IRQ_TYPE1;
+	case reg_irq_type2:
+		return GPIO_G7_CTRL_IRQ_TYPE2;
+	case reg_tolerance:
+		return GPIO_G7_CTRL_RST_TOLERANCE;
+	case reg_debounce_sel1:
+		return GPIO_G7_CTRL_DEBOUNCE_SEL1;
+	case reg_debounce_sel2:
+		return GPIO_G7_CTRL_DEBOUNCE_SEL2;
+	case reg_rdata:
+		return GPIO_G7_CTRL_OUT_DATA;
+	case reg_irq_status:
+		return GPIO_G7_CTRL_IRQ_STS;
+	case reg_cmdsrc0:
+	case reg_cmdsrc1:
+	default:
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+}
+
 #define GPIO_BANK(x)	((x) >> 5)
 #define GPIO_OFFSET(x)	((x) & 0x1f)
 #define GPIO_BIT(x)	BIT(GPIO_OFFSET(x))
@@ -1068,6 +1119,41 @@ static const struct aspeed_gpio_llops aspeed_g4_llops = {
 	.reg_bits_set = aspeed_g4_reg_bits_set,
 	.reg_bits_read = aspeed_g4_reg_bits_read,
 };
+
+static void aspeed_g7_reg_bits_set(struct aspeed_gpio *gpio, unsigned int offset,
+				   const enum aspeed_gpio_reg reg, u32 val)
+{
+	u32 mask = reg_mask(reg);
+	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+	u32 write_val = (ioread32(addr) & ~(mask)) | (((val) << (ffs(mask) - 1)) & (mask));
+
+	iowrite32(write_val, addr);
+}
+
+static u32 aspeed_g7_reg_bits_read(struct aspeed_gpio *gpio, unsigned int offset,
+				   const enum aspeed_gpio_reg reg)
+{
+	u32 mask = reg_mask(reg);
+	void __iomem *addr;
+
+	if (reg == reg_irq_status) {
+		addr = gpio->base + GPIO_G7_IRQ_STS_OFFSET(offset);
+		return ioread32(addr);
+	}
+	addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+	if (reg == reg_val)
+		mask = GPIO_G7_CTRL_IN_DATA;
+
+	return (((ioread32(addr)) & (mask)) >> (ffs(mask) - 1));
+}
+
+static const struct aspeed_gpio_llops aspeed_g7_llops = {
+	.copro_request = NULL,
+	.copro_release = NULL,
+	.reg_bits_set = aspeed_g7_reg_bits_set,
+	.reg_bits_read = aspeed_g7_reg_bits_read,
+};
+
 /*
  * Any banks not specified in a struct aspeed_bank_props array are assumed to
  * have the properties:
@@ -1135,10 +1221,34 @@ static const struct aspeed_gpio_config ast2600_config =
 		.cmd_source_supoort = true,
 	};
 
+static const struct aspeed_bank_props ast2700_bank_props[] = {
+	/*     input	  output   */
+	{ 1, 0x0fffffff, 0x0fffffff }, /* E/F/G/H, 4-GPIO hole */
+	{ 6, 0x00ffffff, 0x00ffffff }, /* Y/Z/AA */
+	{},
+};
+
+static const struct aspeed_gpio_config ast2700_config =
+	/*
+	 * ast2700 has two controllers one with 212 GPIOs and one with 16 GPIOs.
+	 * 216 for simplicity, actual number is 212 (4-GPIO hole in GPIOH)
+	 * We expect ngpio being set in the device tree and this is a fallback
+	 * option.
+	 */
+	{
+		.nr_gpios = 216,
+		.props = ast2700_bank_props,
+		.llops = &aspeed_g7_llops,
+		.debounce_timers_array = g7_debounce_timers,
+		.debounce_timers_num = ARRAY_SIZE(g7_debounce_timers),
+		.cmd_source_supoort = false,
+	};
+
 static const struct of_device_id aspeed_gpio_of_table[] = {
 	{ .compatible = "aspeed,ast2400-gpio", .data = &ast2400_config, },
 	{ .compatible = "aspeed,ast2500-gpio", .data = &ast2500_config, },
 	{ .compatible = "aspeed,ast2600-gpio", .data = &ast2600_config, },
+	{ .compatible = "aspeed,ast2700-gpio", .data = &ast2700_config, },
 	{}
 };
 MODULE_DEVICE_TABLE(of, aspeed_gpio_of_table);
-- 
2.25.1

[PATCH v3 5/6] gpio: aspeed: Change the macro to support deferred probe

[Billy Tsai]

Use module_platform_driver() to replace module_platform_driver_probe().
The former utilizes platform_driver_register(), which allows the driver to
defer probing when it doesn't acquire the necessary resources due to probe
order. In contrast, the latter uses __platform_driver_probe(), which
includes the comment "Note that this is incompatible with deferred
probing." Since our GPIO driver requires access to the clock resource, the
former is more suitable.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/gpio/gpio-aspeed.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 13495442e1aa..d322c03481a8 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -1359,13 +1359,14 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
 }
 
 static struct platform_driver aspeed_gpio_driver = {
+	.probe = aspeed_gpio_probe,
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.of_match_table = aspeed_gpio_of_table,
 	},
 };
 
-module_platform_driver_probe(aspeed_gpio_driver, aspeed_gpio_probe);
+module_platform_driver(aspeed_gpio_driver);
 
 MODULE_DESCRIPTION("Aspeed GPIO Driver");
 MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v3 5/6] gpio: aspeed: Change the macro to support deferred probe

[kernel test robot]

Hi Billy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on linus/master v6.11-rc7 next-20240913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Billy-Tsai/dt-bindings-gpio-aspeed-ast2400-gpio-Support-ast2700/20240913-154911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240913074325.239390-6-billy_tsai%40aspeedtech.com
patch subject: [PATCH v3 5/6] gpio: aspeed: Change the macro to support deferred probe
config: arc-randconfig-001-20240914 (https://download.01.org/0day-ci/archive/20240914/202409141654.wHuMQLLU-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409141654.wHuMQLLU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409141654.wHuMQLLU-lkp@intel.com/

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> WARNING: modpost: vmlinux: section mismatch in reference: aspeed_gpio_driver+0x0 (section: .data) -> aspeed_gpio_probe (section: .init.text)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 5/6] gpio: aspeed: Change the macro to support deferred probe

[kernel test robot]

Hi Billy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on linus/master v6.11-rc7 next-20240913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Billy-Tsai/dt-bindings-gpio-aspeed-ast2400-gpio-Support-ast2700/20240913-154911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240913074325.239390-6-billy_tsai%40aspeedtech.com
patch subject: [PATCH v3 5/6] gpio: aspeed: Change the macro to support deferred probe
config: arm-randconfig-003-20240914 (https://download.01.org/0day-ci/archive/20240914/202409142334.UEVN1oRP-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409142334.UEVN1oRP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409142334.UEVN1oRP-lkp@intel.com/

All warnings (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/locking/test-ww_mutex.o
>> WARNING: modpost: drivers/gpio/gpio-aspeed: section mismatch in reference: aspeed_gpio_driver+0x0 (section: .data) -> aspeed_gpio_probe (section: .init.text)
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/serial/qcaux.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/serial/symbolserial.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-hub.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-aspeed.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-ast-cf.o

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 6/6] gpio: aspeed: Add the flush write to ensure the write complete.

[Billy Tsai]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="yes", Size: 1251 bytes --]

Performing a dummy read ensures that the register write operation is fully
completed, mitigating any potential bus delays that could otherwise impact
the frequency of bitbang usage. E.g., if the JTAG application uses GPIO to
control the JTAG pins (TCK, TMS, TDI, TDO, and TRST), and the application
sets the TCK clock to 1 MHz, the GPIO’s high/low transitions will rely on
a delay function to ensure the clock frequency does not exceed 1 MHz.
However, this can lead to rapid toggling of the GPIO because the write
operation is POSTed and does not wait for a bus acknowledgment.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/gpio/gpio-aspeed.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index d322c03481a8..d1e61ee48da2 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -406,6 +406,8 @@ static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
 	gpio->dcache[GPIO_BANK(offset)] = reg;
 
 	gpio->config->llops->reg_bits_set(gpio, offset, reg_val, val);
+	// flush write
+	gpio->config->llops->reg_bits_read(gpio, offset, reg_val);
 }
 
 static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
-- 
2.25.1

Re: [PATCH v3 6/6] gpio: aspeed: Add the flush write to ensure the write complete.

[Andrew Jeffery]

On Fri, 2024-09-13 at 15:43 +0800, Billy Tsai wrote:
> Performing a dummy read ensures that the register write operation is fully
> completed, mitigating any potential bus delays that could otherwise impact
> the frequency of bitbang usage. E.g., if the JTAG application uses GPIO to
> control the JTAG pins (TCK, TMS, TDI, TDO, and TRST), and the application
> sets the TCK clock to 1 MHz, the GPIO’s high/low transitions will rely on
> a delay function to ensure the clock frequency does not exceed 1 MHz.
> However, this can lead to rapid toggling of the GPIO because the write
> operation is POSTed and does not wait for a bus acknowledgment.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

... are you aware of any other driver concerns of a similar nature wrt
the architecture of the SoCs?

Re: [PATCH v3 6/6] gpio: aspeed: Add the flush write to ensure the write complete.

[Billy Tsai]

> > Performing a dummy read ensures that the register write operation is fully
> > completed, mitigating any potential bus delays that could otherwise impact
> > the frequency of bitbang usage. E.g., if the JTAG application uses GPIO to
> > control the JTAG pins (TCK, TMS, TDI, TDO, and TRST), and the application
> > sets the TCK clock to 1 MHz, the GPIO’s high/low transitions will rely on
> > a delay function to ensure the clock frequency does not exceed 1 MHz.
> > However, this can lead to rapid toggling of the GPIO because the write
> > operation is POSTed and does not wait for a bus acknowledgment.
> >
> > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

> ... are you aware of any other driver concerns of a similar nature wrt
> the architecture of the SoCs?

No, we are only aware of this issue with the GPIO controller, which affects
the output pin behavior immediately after register write.