[v5,2/2] gpio: aspeed: Add SGPIO driver
diff mbox series

Message ID 1563564291-9692-3-git-send-email-hongweiz@ami.com
State New
Headers show
Series
  • gpio: aspeed: Add SGPIO driver
Related show

Commit Message

Hongwei Zhang July 19, 2019, 7:24 p.m. UTC
Add SGPIO driver support for Aspeed AST2500 SoC.

Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
 drivers/gpio/sgpio-aspeed.c | 522 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 522 insertions(+)
 create mode 100644 drivers/gpio/sgpio-aspeed.c

Comments

Linus Walleij July 20, 2019, 7:36 a.m. UTC | #1
Hi Hongwei,

thanks for your patch!

some comments and nitpicking below:

On Fri, Jul 19, 2019 at 9:25 PM Hongwei Zhang <hongweiz@ami.com> wrote:

> Add SGPIO driver support for Aspeed AST2500 SoC.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>

> +// SPDX-License-Identifier: GPL-2.0+

I think the SPDX people prefer GPL-2.0-or-later

> +#include <linux/gpio.h>

Do not include this header in any new code using or
providing GPIOs.

> +#include <linux/gpio/driver.h>

This should be enough.

> +/*
> + * Note: The "value" register returns the input value when the GPIO is
> + *      configured as an input.
> + *
> + *      The "rdata" register returns the output value when the GPIO is
> + *      configured as an output.
> + */
> +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> +       {
> +               .val_regs = 0x0000,
> +               .rdata_reg = 0x0070,
> +               .irq_regs = 0x0004,
> +               .names = { "A", "B", "C", "D" },
> +       },
> +       {
> +               .val_regs = 0x001C,
> +               .rdata_reg = 0x0074,
> +               .irq_regs = 0x0020,
> +               .names = { "E", "F", "G", "H" },
> +       },
> +       {
> +               .val_regs = 0x0038,
> +               .rdata_reg = 0x0078,
> +               .irq_regs = 0x003C,
> +               .names = { "I", "J" },
> +       },
> +};

I guess you have been over the reasons why this is one big GPIO
chip instead of  10 individual gpio_chips?

It is usally better to have the individual chips, because it is easier
to just cut down the code to handle one instance and not having
to offset around the different address ranges.

Even if they all have the same clock, the clocks are reference
counted so it will just be referenced 10 times at most.

If they share a few common registers it is not good to split it
though. So there may be a compelling argument for keeping them
all together.

> +/* This will be resolved at compile time */

I don't see why that matters.

> +static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
> +                                    const struct aspeed_sgpio_bank *bank,
> +                                    const enum aspeed_sgpio_reg reg)

You don't need inline. The compiler will inline it anyway if it
see the need for it.

The only time we really use inline is in header files, where we
want to point out that this function will be inlined as there is no
compiled code in header files.

> +#define GPIO_BANK(x)    ((x) >> 5)
> +#define GPIO_OFFSET(x)  ((x) & 0x1f)
> +#define GPIO_BIT(x)     BIT(GPIO_OFFSET(x))

OK seems fairly standard.

> +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
> +static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
> +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)

These are fairly standard.

> +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> +       struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gpio->lock, flags);
> +       gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
> +       spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +       return 0;
> +}

There is a bug here. You fail to write the "val" to the output
line, which is the expected semantic of this call.

> +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)

These are all very simple MMIO accessors.

If you made one gpio_chip per bank, you could just use gpio-mmio.c
to control the lines by

select GPIO_GENERIC

        ret = bgpio_init(chip, dev, 4,
                         base + GPIO_VAL_VALUE ,
                         NULL,
                         NULL,
                         NULL,
                         NULL,
                         0);

The MMIO gpio library takes care of shadowing the direction and all.
It also will implement get/set_multiple() for you for free.

So seriously consider making one gpio_chip per bank.

> +static inline void irqd_to_aspeed_sgpio_data(struct irq_data *d,
> +static void aspeed_sgpio_irq_ack(struct irq_data *d)
> +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
> +static void aspeed_sgpio_irq_mask(struct irq_data *d)
> +static void aspeed_sgpio_irq_unmask(struct irq_data *d)
> +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
> +static void aspeed_sgpio_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_sgpio *data = gpiochip_get_data(gc);
> +       unsigned int i, p, girq;
> +       unsigned long reg;
> +
> +       chained_irq_enter(ic, desc);
> +
> +       for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> +               const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> +
> +               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);
> +                       generic_handle_irq(girq);
> +               }
> +
> +       }

This also gets really complex with one driver for all the banks.

> +       /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
> +       for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {

(...)
> +static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> +{
> +       struct aspeed_sgpio *gpio;
> +       u32 nr_gpios, sgpio_freq, sgpio_clk_div;
> +       int rc;
> +       unsigned long apb_freq;
> +
> +       /* initialize allocated memory with zeros */

No need for this comment, developers know what "kzalloc" means.

> +       rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
> +       if (rc < 0) {
> +               dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> +               return -EINVAL;
> +       }
> +
> +       gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(gpio->pclk)) {
> +               dev_err(&pdev->dev, "devm_clk_get failed\n");
> +               return PTR_ERR(gpio->pclk);
> +       }
> +
> +       apb_freq = clk_get_rate(gpio->pclk);
> +
> +       /*
> +        * From the datasheet,
> +        *      SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
> +        *      period = 2 * (GPIO254[31:16] + 1) / PCLK
> +        *      frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
> +        *      frequency = PCLK / (2 * (GPIO254[31:16] + 1))
> +        *      frequency * 2 * (GPIO254[31:16] + 1) = PCLK
> +        *      GPIO254[31:16] = PCLK / (frequency * 2) - 1
> +        */
> +       if (sgpio_freq == 0)
> +               return -EINVAL;
> +
> +       sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
> +
> +       if (sgpio_clk_div > (1 << 16) - 1)
> +               return -EINVAL;
> +
> +       iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
> +                 FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
> +                 ASPEED_SGPIO_ENABLE,
> +                 gpio->base + ASPEED_SGPIO_CTRL);

This is a separate clock driver.

Break this out as a separate clk device that the other GPIOs
grab.

Put this in drivers/clk/clk-aspeed-gpio.c or wherever appropriate
with some
reg = <0xnnnnnn54 4>;

Then let the GPIO driver grab this clock. This makes it possible
to use a per-gpio-bank split of the GPIO chips.

It looks a bit complicated but this will work so much better because
the clock code is in the clock subsystem and the GPIO is split up
and becomes a very small driver since it can use gpio MMIO.

Yours,
Linus Walleij
Hongwei Zhang July 22, 2019, 8:36 p.m. UTC | #2
Hello Linus,

Thanks for your reviewing, please find my inline comment on why we group the 
("A", "B", "C", "D") etc. together at below. We will address other concerns 
separately.
--Hongwei

> From:	Linus Walleij <linus.walleij@linaro.org>
> Sent:	Saturday, July 20, 2019 3:37 AM
> To:	Hongwei Zhang
> Cc:	Andrew Jeffery; open list:GPIO SUBSYSTEM; Joel Stanley; linux-aspeed@lists.ozlabs.org; Bartosz 
> Golaszewski; linux-kernel@vger.kernel.org; Linux ARM
> Subject:	Re: [v5 2/2] gpio: aspeed: Add SGPIO driver
> 
> Hi Hongwei,
> 
> thanks for your patch!
> 
> some comments and nitpicking below:
> 
> On Fri, Jul 19, 2019 at 9:25 PM Hongwei Zhang <hongweiz@ami.com> wrote:
> 
> > Add SGPIO driver support for Aspeed AST2500 SoC.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> 
> > +// SPDX-License-Identifier: GPL-2.0+
> 
> I think the SPDX people prefer GPL-2.0-or-later
> 
> > +#include <linux/gpio.h>
> 
> Do not include this header in any new code using or providing GPIOs.
> 
> > +#include <linux/gpio/driver.h>
> 
> This should be enough.
> 
> > +/*
> > + * Note: The "value" register returns the input value when the GPIO is
> > + *      configured as an input.
> > + *
> > + *      The "rdata" register returns the output value when the GPIO is
> > + *      configured as an output.
> > + */
> > +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> > +       {
> > +               .val_regs = 0x0000,
> > +               .rdata_reg = 0x0070,
> > +               .irq_regs = 0x0004,
> > +               .names = { "A", "B", "C", "D" },
> > +       },
> > +       {
> > +               .val_regs = 0x001C,
> > +               .rdata_reg = 0x0074,
> > +               .irq_regs = 0x0020,
> > +               .names = { "E", "F", "G", "H" },
> > +       },
> > +       {
> > +               .val_regs = 0x0038,
> > +               .rdata_reg = 0x0078,
> > +               .irq_regs = 0x003C,
> > +               .names = { "I", "J" },
> > +       },
> > +};
> 
> I guess you have been over the reasons why this is one big GPIO chip instead of  10 individual gpio_chips?
> 
> It is usally better to have the individual chips, because it is easier to just cut down the code to handle 
> one instance and not having to offset around the different address ranges.
> 
> Even if they all have the same clock, the clocks are reference counted so it will just be referenced 10 
> times at most.
> 
> If they share a few common registers it is not good to split it though. So there may be a compelling 
> argument for keeping them all together.
> 

As you suspected it correctly, AST2500 utilizes all the 32 bits of the registers 
(data value, interrupt, etc...), such that using 8-bit bands
[7:0]/[15:8]/23:16]/[31:24] of GPIO_200H for SGPIO_A/B/C/D . 
so registering 10 gpiochip drivers separately will make code more 
complicated, for example gpio_200 register (data_value reg) has to be 
shared by 4 gpiochip instances, and the same is true for gpio204 (interrupt reg), 
and other more registers.
So we would prefer to keeping current implementation.

> > +/* This will be resolved at compile time */
> 
> I don't see why that matters.
> 
> > +static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
> > +                                    const struct aspeed_sgpio_bank *bank,
> > +                                    const enum aspeed_sgpio_reg reg)
> 
> You don't need inline. The compiler will inline it anyway if it see the need for it.
> 
> The only time we really use inline is in header files, where we want to point out that this function will be 
> inlined as there is no compiled code in header files.
> 
> > +#define GPIO_BANK(x)    ((x) >> 5)
> > +#define GPIO_OFFSET(x)  ((x) & 0x1f)
> > +#define GPIO_BIT(x)     BIT(GPIO_OFFSET(x))
> 
> OK seems fairly standard.
> 
> > +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int 
> > +offset) static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned 
> > +int offset, int val) static int aspeed_sgpio_dir_in(struct gpio_chip 
> > +*gc, unsigned int offset)
> 
> These are fairly standard.
> 
> > +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int 
> > +offset, int val) {
> > +       struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&gpio->lock, flags);
> > +       gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
> > +       spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +       return 0;
> > +}
> 
> There is a bug here. You fail to write the "val" to the output line, which is the expected semantic of this 
> call.
> 
> > +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned 
> > +int offset)
> 
> These are all very simple MMIO accessors.
> 
> If you made one gpio_chip per bank, you could just use gpio-mmio.c to control the lines by
> 
> select GPIO_GENERIC
> 
>         ret = bgpio_init(chip, dev, 4,
>                          base + GPIO_VAL_VALUE ,
>                          NULL,
>                          NULL,
>                          NULL,
>                          NULL,
>                          0);
> 
> The MMIO gpio library takes care of shadowing the direction and all.
> It also will implement get/set_multiple() for you for free.
> 
> So seriously consider making one gpio_chip per bank.
> 
> > +static inline void irqd_to_aspeed_sgpio_data(struct irq_data *d, 
> > +static void aspeed_sgpio_irq_ack(struct irq_data *d) static void 
> > +aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set) static void 
> > +aspeed_sgpio_irq_mask(struct irq_data *d) static void 
> > +aspeed_sgpio_irq_unmask(struct irq_data *d) static int 
> > +aspeed_sgpio_set_type(struct irq_data *d, unsigned int type) static 
> > +void aspeed_sgpio_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_sgpio *data = gpiochip_get_data(gc);
> > +       unsigned int i, p, girq;
> > +       unsigned long reg;
> > +
> > +       chained_irq_enter(ic, desc);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > +               const struct aspeed_sgpio_bank *bank = 
> > + &aspeed_sgpio_banks[i];
> > +
> > +               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);
> > +                       generic_handle_irq(girq);
> > +               }
> > +
> > +       }
> 
> This also gets really complex with one driver for all the banks.
> 
> > +       /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
> > +       for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> 
> (...)
> > +static int __init aspeed_sgpio_probe(struct platform_device *pdev) {
> > +       struct aspeed_sgpio *gpio;
> > +       u32 nr_gpios, sgpio_freq, sgpio_clk_div;
> > +       int rc;
> > +       unsigned long apb_freq;
> > +
> > +       /* initialize allocated memory with zeros */
> 
> No need for this comment, developers know what "kzalloc" means.
> 
> > +       rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
> > +       if (rc < 0) {
> > +               dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> > +       if (IS_ERR(gpio->pclk)) {
> > +               dev_err(&pdev->dev, "devm_clk_get failed\n");
> > +               return PTR_ERR(gpio->pclk);
> > +       }
> > +
> > +       apb_freq = clk_get_rate(gpio->pclk);
> > +
> > +       /*
> > +        * From the datasheet,
> > +        *      SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
> > +        *      period = 2 * (GPIO254[31:16] + 1) / PCLK
> > +        *      frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
> > +        *      frequency = PCLK / (2 * (GPIO254[31:16] + 1))
> > +        *      frequency * 2 * (GPIO254[31:16] + 1) = PCLK
> > +        *      GPIO254[31:16] = PCLK / (frequency * 2) - 1
> > +        */
> > +       if (sgpio_freq == 0)
> > +               return -EINVAL;
> > +
> > +       sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
> > +
> > +       if (sgpio_clk_div > (1 << 16) - 1)
> > +               return -EINVAL;
> > +
> > +       iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
> > +                 FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
> > +                 ASPEED_SGPIO_ENABLE,
> > +                 gpio->base + ASPEED_SGPIO_CTRL);
> 
> This is a separate clock driver.
> 
> Break this out as a separate clk device that the other GPIOs grab.
> 
> Put this in drivers/clk/clk-aspeed-gpio.c or wherever appropriate with some reg = <0xnnnnnn54 4>;
> 
> Then let the GPIO driver grab this clock. This makes it possible to use a per-gpio-bank split of the GPIO 
> chips.
> 
> It looks a bit complicated but this will work so much better because the clock code is in the clock 
> subsystem and the GPIO is split up and becomes a very small driver since it can use gpio MMIO.
> 
> Yours,
> Linus Walleij
Linus Walleij July 28, 2019, 11:37 p.m. UTC | #3
On Mon, Jul 22, 2019 at 10:37 PM Hongwei Zhang <hongweiz@ami.com> wrote:

> As you suspected it correctly, AST2500 utilizes all the 32 bits of the registers
> (data value, interrupt, etc...), such that using 8-bit bands
> [7:0]/[15:8]/23:16]/[31:24] of GPIO_200H for SGPIO_A/B/C/D .
> so registering 10 gpiochip drivers separately will make code more
> complicated, for example gpio_200 register (data_value reg) has to be
> shared by 4 gpiochip instances, and the same is true for gpio204 (interrupt reg),
> and other more registers.
> So we would prefer to keeping current implementation.

OK this is a pretty good argument. My review assumed one
32-bit register was not shared between banks but it is,
I see.

The above situation can be managed by regmap, but that will
just a different complexity so go with this approach then.

Yours,
Linus Walleij

Patch
diff mbox series

diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
new file mode 100644
index 0000000..c024c0a
--- /dev/null
+++ b/drivers/gpio/sgpio-aspeed.c
@@ -0,0 +1,522 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019 American Megatrends International LLC.
+ *
+ * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
+#include <linux/hashtable.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+
+#define MAX_NR_SGPIO			80
+
+#define ASPEED_SGPIO_CTRL		0x54
+
+#define ASPEED_SGPIO_PINS_MASK		GENMASK(9, 6)
+#define ASPEED_SGPIO_CLK_DIV_MASK	GENMASK(31, 16)
+#define ASPEED_SGPIO_ENABLE		BIT(0)
+
+struct aspeed_sgpio {
+	struct gpio_chip chip;
+	struct clk *pclk;
+	spinlock_t lock;
+	void __iomem *base;
+	uint32_t dir_in[3];
+	int irq;
+};
+
+struct aspeed_sgpio_bank {
+	uint16_t    val_regs;
+	uint16_t    rdata_reg;
+	uint16_t    irq_regs;
+	const char  names[4][3];
+};
+
+/*
+ * Note: The "value" register returns the input value when the GPIO is
+ *	 configured as an input.
+ *
+ *	 The "rdata" register returns the output value when the GPIO is
+ *	 configured as an output.
+ */
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+	{
+		.val_regs = 0x0000,
+		.rdata_reg = 0x0070,
+		.irq_regs = 0x0004,
+		.names = { "A", "B", "C", "D" },
+	},
+	{
+		.val_regs = 0x001C,
+		.rdata_reg = 0x0074,
+		.irq_regs = 0x0020,
+		.names = { "E", "F", "G", "H" },
+	},
+	{
+		.val_regs = 0x0038,
+		.rdata_reg = 0x0078,
+		.irq_regs = 0x003C,
+		.names = { "I", "J" },
+	},
+};
+
+enum aspeed_sgpio_reg {
+	reg_val,
+	reg_rdata,
+	reg_irq_enable,
+	reg_irq_type0,
+	reg_irq_type1,
+	reg_irq_type2,
+	reg_irq_status,
+};
+
+#define GPIO_VAL_VALUE      0x00
+#define GPIO_IRQ_ENABLE     0x00
+#define GPIO_IRQ_TYPE0      0x04
+#define GPIO_IRQ_TYPE1      0x08
+#define GPIO_IRQ_TYPE2      0x0C
+#define GPIO_IRQ_STATUS     0x10
+
+/* This will be resolved at compile time */
+static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
+				     const struct aspeed_sgpio_bank *bank,
+				     const enum aspeed_sgpio_reg reg)
+{
+	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_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;
+	default:
+		/* acturally if code runs to here, it's an error case */
+		BUG_ON(1);
+	}
+}
+
+#define GPIO_BANK(x)    ((x) >> 5)
+#define GPIO_OFFSET(x)  ((x) & 0x1f)
+#define GPIO_BIT(x)     BIT(GPIO_OFFSET(x))
+
+static const struct aspeed_sgpio_bank *to_bank(unsigned int offset)
+{
+	unsigned int bank = GPIO_BANK(offset);
+
+	WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
+	return &aspeed_sgpio_banks[bank];
+}
+
+static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+	const struct aspeed_sgpio_bank *bank = to_bank(offset);
+	unsigned long flags;
+	enum aspeed_sgpio_reg reg;
+	bool is_input;
+	int rc = 0;
+
+	spin_lock_irqsave(&gpio->lock, flags);
+
+	is_input = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
+	reg = is_input ? reg_val : reg_rdata;
+	rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset));
+
+	spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return rc;
+}
+
+static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+	const struct aspeed_sgpio_bank *bank = to_bank(offset);
+	unsigned long flags;
+	void __iomem *addr;
+	u32 reg = 0;
+
+	spin_lock_irqsave(&gpio->lock, flags);
+
+	addr = bank_reg(gpio, bank, reg_val);
+
+	if (val)
+		reg |= GPIO_BIT(offset);
+	else
+		reg &= ~GPIO_BIT(offset);
+
+	iowrite32(reg, addr);
+
+	spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)
+{
+	struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&gpio->lock, flags);
+	gpio->dir_in[GPIO_BANK(offset)] |= GPIO_BIT(offset);
+	spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return 0;
+}
+
+static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&gpio->lock, flags);
+	gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
+	spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return 0;
+}
+
+static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	int dir_status;
+	struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&gpio->lock, flags);
+	dir_status = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
+	spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return dir_status;
+
+}
+
+static inline void irqd_to_aspeed_sgpio_data(struct irq_data *d,
+					     struct aspeed_sgpio **gpio,
+					     const struct aspeed_sgpio_bank **bank,
+					     u32 *bit, int *offset)
+{
+	struct aspeed_sgpio *internal;
+
+	*offset = irqd_to_hwirq(d);
+	internal = irq_data_get_irq_chip_data(d);
+	WARN_ON(!internal);
+
+	*gpio = internal;
+	*bank = to_bank(*offset);
+	*bit = GPIO_BIT(*offset);
+}
+
+static void aspeed_sgpio_irq_ack(struct irq_data *d)
+{
+	const struct aspeed_sgpio_bank *bank;
+	struct aspeed_sgpio *gpio;
+	unsigned long flags;
+	void __iomem *status_addr;
+	int offset;
+	u32 bit;
+
+	irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+	status_addr = bank_reg(gpio, bank, reg_irq_status);
+
+	spin_lock_irqsave(&gpio->lock, flags);
+
+	iowrite32(bit, status_addr);
+
+	spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
+{
+	const struct aspeed_sgpio_bank *bank;
+	struct aspeed_sgpio *gpio;
+	unsigned long flags;
+	u32 reg, bit;
+	void __iomem *addr;
+	int offset;
+
+	irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+	addr = bank_reg(gpio, bank, reg_irq_enable);
+
+	spin_lock_irqsave(&gpio->lock, flags);
+
+	reg = ioread32(addr);
+	if (set)
+		reg |= bit;
+	else
+		reg &= ~bit;
+
+	iowrite32(reg, addr);
+
+	spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_mask(struct irq_data *d)
+{
+	aspeed_sgpio_irq_set_mask(d, false);
+}
+
+static void aspeed_sgpio_irq_unmask(struct irq_data *d)
+{
+	aspeed_sgpio_irq_set_mask(d, true);
+}
+
+static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
+{
+	u32 type0 = 0;
+	u32 type1 = 0;
+	u32 type2 = 0;
+	u32 bit, reg;
+	const struct aspeed_sgpio_bank *bank;
+	irq_flow_handler_t handler;
+	struct aspeed_sgpio *gpio;
+	unsigned long flags;
+	void __iomem *addr;
+	int offset;
+
+	irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_BOTH:
+		type2 |= bit;
+		/* fall through */
+	case IRQ_TYPE_EDGE_RISING:
+		type0 |= bit;
+		/* fall through */
+	case IRQ_TYPE_EDGE_FALLING:
+		handler = handle_edge_irq;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		type0 |= bit;
+		/* fall through */
+	case IRQ_TYPE_LEVEL_LOW:
+		type1 |= bit;
+		handler = handle_level_irq;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&gpio->lock, flags);
+
+	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);
+
+	spin_unlock_irqrestore(&gpio->lock, flags);
+
+	irq_set_handler_locked(d, handler);
+
+	return 0;
+}
+
+static void aspeed_sgpio_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_sgpio *data = gpiochip_get_data(gc);
+	unsigned int i, p, girq;
+	unsigned long reg;
+
+	chained_irq_enter(ic, desc);
+
+	for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+		const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
+
+		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);
+			generic_handle_irq(girq);
+		}
+
+	}
+
+	chained_irq_exit(ic, desc);
+}
+
+static struct irq_chip aspeed_sgpio_irqchip = {
+	.name       = "aspeed-sgpio",
+	.irq_ack    = aspeed_sgpio_irq_ack,
+	.irq_mask   = aspeed_sgpio_irq_mask,
+	.irq_unmask = aspeed_sgpio_irq_unmask,
+	.irq_set_type   = aspeed_sgpio_set_type,
+};
+
+static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
+				   struct platform_device *pdev)
+{
+	int rc, i;
+	const struct aspeed_sgpio_bank *bank;
+
+	rc = platform_get_irq(pdev, 0);
+	if (rc < 0)
+		return rc;
+
+	gpio->irq = rc;
+
+	/* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
+	for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+		bank =  &aspeed_sgpio_banks[i];
+		/* disable irq enable bits */
+		iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
+		/* clear status bits */
+		iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
+	}
+
+	rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
+				  0, handle_bad_irq, IRQ_TYPE_NONE);
+	if (rc) {
+		dev_info(&pdev->dev, "Could not add irqchip\n");
+		return rc;
+	}
+
+	gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
+				     gpio->irq, aspeed_sgpio_irq_handler);
+
+	/* set IRQ settings and Enable Interrupt */
+	for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+		bank = &aspeed_sgpio_banks[i];
+		/* set falling or level-low irq */
+		iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
+		/* trigger type is edge */
+		iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
+		/* dual edge trigger mode. */
+		iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
+		/* enable irq */
+		iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
+	}
+
+	return 0;
+}
+
+static const struct of_device_id aspeed_sgpio_of_table[] = {
+	{ .compatible = "aspeed,ast2400-sgpio" },
+	{ .compatible = "aspeed,ast2500-sgpio" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
+
+static int __init aspeed_sgpio_probe(struct platform_device *pdev)
+{
+	struct aspeed_sgpio *gpio;
+	u32 nr_gpios, sgpio_freq, sgpio_clk_div;
+	int rc;
+	unsigned long apb_freq;
+
+	/* initialize allocated memory with zeros */
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(gpio->base))
+		return PTR_ERR(gpio->base);
+
+	rc = of_property_read_u32(pdev->dev.of_node, "ngpios", &nr_gpios);
+	if (rc < 0) {
+		dev_err(&pdev->dev, "Could not read ngpios property\n");
+		return -EINVAL;
+	} else if (nr_gpios > MAX_NR_SGPIO) {
+		dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: %d\n",
+			MAX_NR_SGPIO, nr_gpios);
+		return -EINVAL;
+	}
+
+	rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
+	if (rc < 0) {
+		dev_err(&pdev->dev, "Could not read bus-frequency property\n");
+		return -EINVAL;
+	}
+
+	gpio->pclk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(gpio->pclk)) {
+		dev_err(&pdev->dev, "devm_clk_get failed\n");
+		return PTR_ERR(gpio->pclk);
+	}
+
+	apb_freq = clk_get_rate(gpio->pclk);
+
+	/*
+	 * From the datasheet,
+	 *	SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
+	 *	period = 2 * (GPIO254[31:16] + 1) / PCLK
+	 *	frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
+	 *	frequency = PCLK / (2 * (GPIO254[31:16] + 1))
+	 *	frequency * 2 * (GPIO254[31:16] + 1) = PCLK
+	 *	GPIO254[31:16] = PCLK / (frequency * 2) - 1
+	 */
+	if (sgpio_freq == 0)
+		return -EINVAL;
+
+	sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
+
+	if (sgpio_clk_div > (1 << 16) - 1)
+		return -EINVAL;
+
+	iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
+		  FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
+		  ASPEED_SGPIO_ENABLE,
+		  gpio->base + ASPEED_SGPIO_CTRL);
+
+	spin_lock_init(&gpio->lock);
+
+	gpio->chip.parent = &pdev->dev;
+	gpio->chip.ngpio = nr_gpios;
+	gpio->chip.direction_input = aspeed_sgpio_dir_in;
+	gpio->chip.direction_output = aspeed_sgpio_dir_out;
+	gpio->chip.get_direction = aspeed_sgpio_get_direction;
+	gpio->chip.request = NULL;
+	gpio->chip.free = NULL;
+	gpio->chip.get = aspeed_sgpio_get;
+	gpio->chip.set = aspeed_sgpio_set;
+	gpio->chip.set_config = NULL;
+	gpio->chip.label = dev_name(&pdev->dev);
+	gpio->chip.base = -1;
+
+	/* set all SGPIO pins as input (1). */
+	memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in));
+
+	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+	if (rc < 0)
+		return rc;
+
+	return aspeed_sgpio_setup_irqs(gpio, pdev);
+}
+
+static struct platform_driver aspeed_sgpio_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = aspeed_sgpio_of_table,
+	},
+};
+
+module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
+MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
+MODULE_LICENSE("GPL");