diff mbox series

[RFC,v7,4/6] gpio: mpfs: add polarfire soc gpio support

Message ID 20240723-underage-wheat-7dd65c2158e7@wendy
State New
Headers show
Series PolarFire SoC GPIO support | expand

Commit Message

Conor Dooley July 23, 2024, 11:27 a.m. UTC
From: Lewis Hanly <lewis.hanly@microchip.com>

Add a driver to support the Polarfire SoC gpio controller

Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Unchanged from last list submission, I'm looking for comments on the
other patches in the series at the moment.
---
 drivers/gpio/Kconfig     |   7 +
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-mpfs.c | 320 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 328 insertions(+)
 create mode 100644 drivers/gpio/gpio-mpfs.c

Comments

Linus Walleij Aug. 5, 2024, 8 a.m. UTC | #1
On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:

> From: Lewis Hanly <lewis.hanly@microchip.com>
>
> Add a driver to support the Polarfire SoC gpio controller
>
> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Looks good to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Linus Walleij Aug. 5, 2024, 8:04 a.m. UTC | #2
On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:


> From: Lewis Hanly <lewis.hanly@microchip.com>
>
> Add a driver to support the Polarfire SoC gpio controller
>
> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Just a comment on second thought:

> +config GPIO_POLARFIRE_SOC
> +       bool "Microchip FPGA GPIO support"
> +       depends on OF_GPIO
> +       select GPIOLIB_IRQCHIP

select GPIO_GENERIC?

> +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
> +{
> +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> +       u32 gpio_cfg;
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> +
> +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> +       gpio_cfg |= MPFS_GPIO_EN_IN;
> +       gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);

OK this part is unique...

> +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
> +{
> +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> +       u32 gpio_cfg;
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> +
> +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> +       gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;

Also here

> +static int mpfs_gpio_get_direction(struct gpio_chip *gc,
> +                                  unsigned int gpio_index)
> +static int mpfs_gpio_get(struct gpio_chip *gc,
> +                        unsigned int gpio_index)
> +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)

But these are just MMIO functions.

Is it possible to use augmented generic MMIO, i.e just override these
two functions that
need special handling?

Yours,
Linus Walleij
Conor Dooley Aug. 6, 2024, 5:18 p.m. UTC | #3
On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:
> On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> 
> 
> > From: Lewis Hanly <lewis.hanly@microchip.com>
> >
> > Add a driver to support the Polarfire SoC gpio controller
> >
> > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Just a comment on second thought:
> 
> > +config GPIO_POLARFIRE_SOC
> > +       bool "Microchip FPGA GPIO support"
> > +       depends on OF_GPIO
> > +       select GPIOLIB_IRQCHIP
> 
> select GPIO_GENERIC?
> 
> > +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
> > +{
> > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > +       u32 gpio_cfg;
> > +       unsigned long flags;
> > +
> > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > +       gpio_cfg |= MPFS_GPIO_EN_IN;
> > +       gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);
> 
> OK this part is unique...
> 
> > +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
> > +{
> > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > +       u32 gpio_cfg;
> > +       unsigned long flags;
> > +
> > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > +       gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;
> 
> Also here
> 
> > +static int mpfs_gpio_get_direction(struct gpio_chip *gc,
> > +                                  unsigned int gpio_index)
> > +static int mpfs_gpio_get(struct gpio_chip *gc,
> > +                        unsigned int gpio_index)
> > +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)
> 
> But these are just MMIO functions.
> 
> Is it possible to use augmented generic MMIO, i.e just override these
> two functions that
> need special handling?

I'll look into it - as I mentioned under the --- line, I really didn't
touch most of the driver and there's comments from Lewis' submission
that still apply.

Cheers,
Conor.
Linus Walleij Aug. 7, 2024, 4:55 p.m. UTC | #4
On Tue, Aug 6, 2024 at 7:18 PM Conor Dooley <conor@kernel.org> wrote:
> On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:

> > Is it possible to use augmented generic MMIO, i.e just override these
> > two functions that
> > need special handling?
>
> I'll look into it - as I mentioned under the --- line, I really didn't
> touch most of the driver and there's comments from Lewis' submission
> that still apply.

Thanks Conor, thanks for taking this over, too many patch sets fall
on the floor. I'm mostly fine merging it like this and then improving
it in-tree as well, I'm not as insistent on things being perfect before
merging as long as they are maintained.

Yours,
Linus Walleij
Conor Dooley Aug. 7, 2024, 5:22 p.m. UTC | #5
On Wed, Aug 07, 2024 at 06:55:58PM +0200, Linus Walleij wrote:
> On Tue, Aug 6, 2024 at 7:18 PM Conor Dooley <conor@kernel.org> wrote:
> > On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:
> 
> > > Is it possible to use augmented generic MMIO, i.e just override these
> > > two functions that
> > > need special handling?
> >
> > I'll look into it - as I mentioned under the --- line, I really didn't
> > touch most of the driver and there's comments from Lewis' submission
> > that still apply.
> 
> Thanks Conor, thanks for taking this over, too many patch sets fall
> on the floor.

Meh, it always bugged me that upstreaming driver was given up on because
fixing the interrupts was "hard". It'd be a poor example to others if I
le the driver sit downstream as a result.

> I'm mostly fine merging it like this and then improving
> it in-tree as well, I'm not as insistent on things being perfect before
> merging as long as they are maintained.

The gpio side of things might not be too bad, but the irqchip side is
crap (it has an of_iomap() in it for example*), and if the irqchip driver
needs work it feels sensible to improve on both before merging anything.
Unless of course, you think it would be reasonable to rip the interrupt
support out of the gpio driver, merge that and incrementally add it
while also improving the things you and the earlier review mentioned
w.r.t. regmap?

Cheers,
Conor.

* Getting rid of the of_iomap() needs me to rewrite parts of the clock
driver, which seemed overkill until I knew whether or not the approach
was permitted.
Conor Dooley Oct. 16, 2024, 9:56 a.m. UTC | #6
On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:
> On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> 
> 
> > From: Lewis Hanly <lewis.hanly@microchip.com>
> >
> > Add a driver to support the Polarfire SoC gpio controller
> >
> > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Just a comment on second thought:
> 
> > +config GPIO_POLARFIRE_SOC
> > +       bool "Microchip FPGA GPIO support"
> > +       depends on OF_GPIO
> > +       select GPIOLIB_IRQCHIP
> 
> select GPIO_GENERIC?
> 
> > +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
> > +{
> > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > +       u32 gpio_cfg;
> > +       unsigned long flags;
> > +
> > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > +       gpio_cfg |= MPFS_GPIO_EN_IN;
> > +       gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);
> 
> OK this part is unique...
> 
> > +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
> > +{
> > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > +       u32 gpio_cfg;
> > +       unsigned long flags;
> > +
> > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > +       gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;
> 
> Also here
> 
> > +static int mpfs_gpio_get_direction(struct gpio_chip *gc,
> > +                                  unsigned int gpio_index)
> > +static int mpfs_gpio_get(struct gpio_chip *gc,
> > +                        unsigned int gpio_index)
> > +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)
> 
> But these are just MMIO functions.
> 
> Is it possible to use augmented generic MMIO, i.e just override these
> two functions that
> need special handling?

So, I've been looking into this again (finally), with an eye to stripping
the interrupt handling bits out, and trying to upstream this in pieces.
I dunno if I'm making a mistake here, but I don't know if there's much
value in implementing this suggestion - as far as I can tell only the
get()/set() functions can be replaced by what's provided by gpio-mmio.c.
There are no controller wide registers that control direction and so
bgpio_get_dir() can't be used - direction is read from the same
mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index) registers that it is set
using. Adding bgpio stuff, to just go ahead and overwrite it, to save on
trivial get()/set() implementations seems to me like adding complication
rather than removing it. What am I missing here?

Cheers,
Conor.
Conor Dooley Oct. 16, 2024, 10:29 a.m. UTC | #7
On Wed, Oct 16, 2024 at 10:56:32AM +0100, Conor Dooley wrote:
> On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:
> > On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> > 
> > 
> > > From: Lewis Hanly <lewis.hanly@microchip.com>
> > >
> > > Add a driver to support the Polarfire SoC gpio controller
> > >
> > > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > 
> > Just a comment on second thought:
> > 
> > > +config GPIO_POLARFIRE_SOC
> > > +       bool "Microchip FPGA GPIO support"
> > > +       depends on OF_GPIO
> > > +       select GPIOLIB_IRQCHIP
> > 
> > select GPIO_GENERIC?
> > 
> > > +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
> > > +{
> > > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > > +       u32 gpio_cfg;
> > > +       unsigned long flags;
> > > +
> > > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > > +
> > > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > > +       gpio_cfg |= MPFS_GPIO_EN_IN;
> > > +       gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);
> > 
> > OK this part is unique...
> > 
> > > +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
> > > +{
> > > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > > +       u32 gpio_cfg;
> > > +       unsigned long flags;
> > > +
> > > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > > +
> > > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > > +       gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;
> > 
> > Also here
> > 
> > > +static int mpfs_gpio_get_direction(struct gpio_chip *gc,
> > > +                                  unsigned int gpio_index)
> > > +static int mpfs_gpio_get(struct gpio_chip *gc,
> > > +                        unsigned int gpio_index)
> > > +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)
> > 
> > But these are just MMIO functions.
> > 
> > Is it possible to use augmented generic MMIO, i.e just override these
> > two functions that
> > need special handling?
> 
> So, I've been looking into this again (finally), with an eye to stripping
> the interrupt handling bits out, and trying to upstream this in pieces.
> I dunno if I'm making a mistake here, but I don't know if there's much
> value in implementing this suggestion - as far as I can tell only the
> get()/set() functions can be replaced by what's provided by gpio-mmio.c.
> There are no controller wide registers that control direction and so
> bgpio_get_dir() can't be used - direction is read from the same
> mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index) registers that it is set
> using. Adding bgpio stuff, to just go ahead and overwrite it, to save on
> trivial get()/set() implementations seems to me like adding complication
> rather than removing it. What am I missing here?

What does bring a nice simplification though, IMO, is regmap. I am
pretty sure that using it was one of the suggestions made last time
Lewis submitted this - so I think I'm going to do that instead.
Linus Walleij Oct. 16, 2024, 7:25 p.m. UTC | #8
On Wed, Oct 16, 2024 at 11:56 AM Conor Dooley <conor@kernel.org> wrote:

> I dunno if I'm making a mistake here, but I don't know if there's much
> value in implementing this suggestion - as far as I can tell only the
> get()/set() functions can be replaced by what's provided by gpio-mmio.c.

You're not making any mistake, you know the driver and hw better
than me.

Just skip the GPIO_MMIO if you think the result would look ugly.

You can add my:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Linus Walleij Oct. 16, 2024, 7:26 p.m. UTC | #9
On Wed, Oct 16, 2024 at 12:29 PM Conor Dooley <conor@kernel.org> wrote:

> What does bring a nice simplification though, IMO, is regmap. I am
> pretty sure that using it was one of the suggestions made last time
> Lewis submitted this - so I think I'm going to do that instead.

If you have the time. Using GPIO_REGMAP for MMIO is not that
common and I think the driver is pretty neat as it stands.

Yours,
Linus Walleij
Conor Dooley Oct. 16, 2024, 7:42 p.m. UTC | #10
On Wed, Oct 16, 2024 at 09:26:13PM +0200, Linus Walleij wrote:
> On Wed, Oct 16, 2024 at 12:29 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > What does bring a nice simplification though, IMO, is regmap. I am
> > pretty sure that using it was one of the suggestions made last time
> > Lewis submitted this - so I think I'm going to do that instead.
> 
> If you have the time. Using GPIO_REGMAP for MMIO is not that
> common and I think the driver is pretty neat as it stands.

As with using the common MMIO stuff, I don't think GPIO_REGMAP provides
that much value as I cannot use the direction stuff from it. I was
thinking of using regmap directly, like:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=gpio-no-irq&id=c8933e1e3600e3fa29efe28fbb2e343e133f9d67
which I think reduces how ugly the two direction functions look.
Conor Dooley Oct. 22, 2024, 4:28 p.m. UTC | #11
On Wed, Oct 16, 2024 at 08:42:51PM +0100, Conor Dooley wrote:
> On Wed, Oct 16, 2024 at 09:26:13PM +0200, Linus Walleij wrote:
> > On Wed, Oct 16, 2024 at 12:29 PM Conor Dooley <conor@kernel.org> wrote:
> > 
> > > What does bring a nice simplification though, IMO, is regmap. I am
> > > pretty sure that using it was one of the suggestions made last time
> > > Lewis submitted this - so I think I'm going to do that instead.
> > 
> > If you have the time. Using GPIO_REGMAP for MMIO is not that
> > common and I think the driver is pretty neat as it stands.
> 
> As with using the common MMIO stuff, I don't think GPIO_REGMAP provides
> that much value as I cannot use the direction stuff from it. I was
> thinking of using regmap directly, like:
> https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=gpio-no-irq&id=c8933e1e3600e3fa29efe28fbb2e343e133f9d67
> which I think reduces how ugly the two direction functions look.

Sorry to bother you Linus, but I was hoping to see some sort of comment
here before I squash this stuff and submit a new version. Is something
like what I linked above acceptable?
Linus Walleij Oct. 23, 2024, 9:58 a.m. UTC | #12
On Tue, Oct 22, 2024 at 6:28 PM Conor Dooley <conor@kernel.org> wrote:
> On Wed, Oct 16, 2024 at 08:42:51PM +0100, Conor Dooley wrote:
> > On Wed, Oct 16, 2024 at 09:26:13PM +0200, Linus Walleij wrote:
> > > On Wed, Oct 16, 2024 at 12:29 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > > What does bring a nice simplification though, IMO, is regmap. I am
> > > > pretty sure that using it was one of the suggestions made last time
> > > > Lewis submitted this - so I think I'm going to do that instead.
> > >
> > > If you have the time. Using GPIO_REGMAP for MMIO is not that
> > > common and I think the driver is pretty neat as it stands.
> >
> > As with using the common MMIO stuff, I don't think GPIO_REGMAP provides
> > that much value as I cannot use the direction stuff from it. I was
> > thinking of using regmap directly, like:
> > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=gpio-no-irq&id=c8933e1e3600e3fa29efe28fbb2e343e133f9d67
> > which I think reduces how ugly the two direction functions look.
>
> Sorry to bother you Linus, but I was hoping to see some sort of comment
> here before I squash this stuff and submit a new version. Is something
> like what I linked above acceptable?

I pretty much trust your judgement on this, I'm fine with either solution,
the patch is also perfectly fine already as it is unless you want to polish it
further.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3dbddec070281..78fe494e3722c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -549,6 +549,13 @@  config GPIO_PL061
 	help
 	  Say yes here to support the PrimeCell PL061 GPIO device.
 
+config GPIO_POLARFIRE_SOC
+	bool "Microchip FPGA GPIO support"
+	depends on OF_GPIO
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support the GPIO device on Microchip FPGAs.
+
 config GPIO_PXA
 	bool "PXA GPIO support"
 	depends on ARCH_PXA || ARCH_MMP || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index e2a53013780e5..dd6ba21bce76e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -132,6 +132,7 @@  obj-$(CONFIG_GPIO_PCI_IDIO_16)		+= gpio-pci-idio-16.o
 obj-$(CONFIG_GPIO_PISOSR)		+= gpio-pisosr.o
 obj-$(CONFIG_GPIO_PL061)		+= gpio-pl061.o
 obj-$(CONFIG_GPIO_PMIC_EIC_SPRD)	+= gpio-pmic-eic-sprd.o
+obj-$(CONFIG_GPIO_POLARFIRE_SOC)	+= gpio-mpfs.o
 obj-$(CONFIG_GPIO_PXA)			+= gpio-pxa.o
 obj-$(CONFIG_GPIO_RASPBERRYPI_EXP)	+= gpio-raspberrypi-exp.o
 obj-$(CONFIG_GPIO_RC5T583)		+= gpio-rc5t583.o
diff --git a/drivers/gpio/gpio-mpfs.c b/drivers/gpio/gpio-mpfs.c
new file mode 100644
index 0000000000000..1ac0526ba1029
--- /dev/null
+++ b/drivers/gpio/gpio-mpfs.c
@@ -0,0 +1,320 @@ 
+// SPDX-License-Identifier: (GPL-2.0)
+/*
+ * Microchip PolarFire SoC (MPFS) GPIO controller driver
+ *
+ * Copyright (c) 2018-2022 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Lewis Hanly <lewis.hanly@microchip.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define MPFS_GPIO_CTRL(i)		(0x4 * (i))
+#define MAX_NUM_GPIO			32
+#define MPFS_GPIO_EN_INT		3
+#define MPFS_GPIO_EN_OUT_BUF		BIT(2)
+#define MPFS_GPIO_EN_IN			BIT(1)
+#define MPFS_GPIO_EN_OUT		BIT(0)
+
+#define MPFS_GPIO_TYPE_INT_EDGE_BOTH	0x80
+#define MPFS_GPIO_TYPE_INT_EDGE_NEG	0x60
+#define MPFS_GPIO_TYPE_INT_EDGE_POS	0x40
+#define MPFS_GPIO_TYPE_INT_LEVEL_LOW	0x20
+#define MPFS_GPIO_TYPE_INT_LEVEL_HIGH	0x00
+#define MPFS_GPIO_TYPE_INT_MASK		GENMASK(7, 5)
+#define MPFS_IRQ_REG			0x80
+#define MPFS_INP_REG			0x84
+#define MPFS_OUTP_REG			0x88
+
+struct mpfs_gpio_chip {
+	void __iomem *base;
+	struct clk *clk;
+	raw_spinlock_t	lock;
+	struct gpio_chip gc;
+};
+
+static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, bool value)
+{
+	unsigned long reg = readl(addr);
+
+	__assign_bit(bit_offset, &reg, value);
+	writel(reg, addr);
+}
+
+static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	u32 gpio_cfg;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+	gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+	gpio_cfg |= MPFS_GPIO_EN_IN;
+	gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);
+	writel(gpio_cfg, mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+
+	raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+	return 0;
+}
+
+static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	u32 gpio_cfg;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+	gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+	gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;
+	gpio_cfg &= ~MPFS_GPIO_EN_IN;
+	writel(gpio_cfg, mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG, gpio_index, value);
+
+	raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+	return 0;
+}
+
+static int mpfs_gpio_get_direction(struct gpio_chip *gc,
+				   unsigned int gpio_index)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	u32 gpio_cfg;
+
+	gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+	if (gpio_cfg & MPFS_GPIO_EN_IN)
+		return GPIO_LINE_DIRECTION_IN;
+
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static int mpfs_gpio_get(struct gpio_chip *gc,
+			 unsigned int gpio_index)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+
+	return !!(readl(mpfs_gpio->base + MPFS_INP_REG) & BIT(gpio_index));
+}
+
+static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG,
+			     gpio_index, value);
+
+	raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+}
+
+static int mpfs_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	int gpio_index = irqd_to_hwirq(data);
+	u32 interrupt_type;
+	u32 gpio_cfg;
+	unsigned long flags;
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_BOTH:
+		interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_NEG;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_POS;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_HIGH;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
+		break;
+	}
+
+	raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+	gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+	gpio_cfg &= ~MPFS_GPIO_TYPE_INT_MASK;
+	gpio_cfg |= interrupt_type;
+	writel(gpio_cfg, mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+
+	raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+	return 0;
+}
+
+static void mpfs_gpio_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	int gpio_index = irqd_to_hwirq(data);
+
+	gpiochip_enable_irq(gc, gpio_index);
+	mpfs_gpio_direction_input(gc, gpio_index);
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1);
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index),
+			     MPFS_GPIO_EN_INT, 1);
+}
+
+static void mpfs_gpio_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	int gpio_index = irqd_to_hwirq(data);
+
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1);
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index),
+			     MPFS_GPIO_EN_INT, 0);
+	gpiochip_disable_irq(gc, gpio_index);
+}
+
+static const struct irq_chip mpfs_gpio_irqchip = {
+	.name = "mpfs",
+	.irq_set_type = mpfs_gpio_irq_set_type,
+	.irq_mask	= mpfs_gpio_irq_mask,
+	.irq_unmask	= mpfs_gpio_irq_unmask,
+	.flags = IRQCHIP_IMMUTABLE | IRQCHIP_MASK_ON_SUSPEND,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static void mpfs_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+	struct mpfs_gpio_chip *mpfs_gpio =
+		gpiochip_get_data(irq_desc_get_handler_data(desc));
+	unsigned long status;
+	int offset;
+
+	chained_irq_enter(irqchip, desc);
+
+	status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
+	for_each_set_bit(offset, &status, mpfs_gpio->gc.ngpio) {
+		mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1);
+		generic_handle_irq(irq_find_mapping(mpfs_gpio->gc.irq.domain, offset));
+	}
+
+	chained_irq_exit(irqchip, desc);
+}
+
+static int mpfs_gpio_probe(struct platform_device *pdev)
+{
+	struct clk *clk;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = pdev->dev.of_node;
+	struct mpfs_gpio_chip *mpfs_gpio;
+	struct gpio_irq_chip *girq;
+	int i, ret, ngpios, nirqs;
+
+	mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL);
+	if (!mpfs_gpio)
+		return -ENOMEM;
+
+	mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mpfs_gpio->base))
+		return dev_err_probe(dev, PTR_ERR(mpfs_gpio->base), "failed to ioremap memory resource\n");
+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "devm_clk_get failed\n");
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable clock\n");
+
+	mpfs_gpio->clk = clk;
+
+	ngpios = MAX_NUM_GPIO;
+	device_property_read_u32(dev, "ngpios", &ngpios);
+	if (ngpios > MAX_NUM_GPIO)
+		ngpios = MAX_NUM_GPIO;
+
+	raw_spin_lock_init(&mpfs_gpio->lock);
+	mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input;
+	mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output;
+	mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction;
+	mpfs_gpio->gc.get = mpfs_gpio_get;
+	mpfs_gpio->gc.set = mpfs_gpio_set;
+	mpfs_gpio->gc.base = -1;
+	mpfs_gpio->gc.ngpio = ngpios;
+	mpfs_gpio->gc.label = dev_name(dev);
+	mpfs_gpio->gc.parent = dev;
+	mpfs_gpio->gc.owner = THIS_MODULE;
+
+	nirqs = of_irq_count(node);
+	if (nirqs > MAX_NUM_GPIO) {
+		ret = -ENXIO;
+		goto cleanup_clock;
+	}
+	girq = &mpfs_gpio->gc.irq;
+	gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip);
+	girq->handler = handle_simple_irq;
+	girq->parent_handler = mpfs_gpio_irq_handler;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->num_parents = nirqs;
+	girq->parents = devm_kcalloc(&pdev->dev, nirqs,
+				     sizeof(*girq->parents), GFP_KERNEL);
+	if (!girq->parents) {
+		ret = -ENOMEM;
+		goto cleanup_clock;
+	}
+	for (i = 0; i < nirqs; i++)
+		girq->parents[i] = platform_get_irq(pdev, i);
+
+	ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio);
+	if (ret)
+		goto cleanup_clock;
+
+	platform_set_drvdata(pdev, mpfs_gpio);
+
+	return 0;
+
+cleanup_clock:
+	clk_disable_unprepare(mpfs_gpio->clk);
+	return ret;
+}
+
+static int mpfs_gpio_remove(struct platform_device *pdev)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = platform_get_drvdata(pdev);
+
+	gpiochip_remove(&mpfs_gpio->gc);
+	clk_disable_unprepare(mpfs_gpio->clk);
+
+	return 0;
+}
+
+static const struct of_device_id mpfs_gpio_of_ids[] = {
+	{ .compatible = "microchip,mpfs-gpio", },
+	{ /* end of list */ }
+};
+
+static struct platform_driver mpfs_gpio_driver = {
+	.probe = mpfs_gpio_probe,
+	.driver = {
+		.name = "microchip,mpfs-gpio",
+		.of_match_table = mpfs_gpio_of_ids,
+	},
+	.remove = mpfs_gpio_remove,
+};
+builtin_platform_driver(mpfs_gpio_driver);