[3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
diff mbox series

Message ID 1573560684-48104-4-git-send-email-yash.shah@sifive.com
State New
Headers show
Series
  • GPIO & Hierarchy IRQ support for HiFive Unleashed
Related show

Commit Message

Yash Shah Nov. 12, 2019, 12:12 p.m. UTC
Adds the GPIO driver for SiFive RISC-V SoCs.

Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
[Atish: Various fixes and code cleanup]
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 drivers/gpio/Kconfig       |   9 ++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-sifive.c | 255 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 265 insertions(+)
 create mode 100644 drivers/gpio/gpio-sifive.c

Comments

Marc Zyngier Nov. 12, 2019, 12:58 p.m. UTC | #1
On 2019-11-12 13:21, Yash Shah wrote:
> Adds the GPIO driver for SiFive RISC-V SoCs.
>
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>

[...]

> +static int sifive_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> +					     unsigned int child,
> +					     unsigned int child_type,
> +					     unsigned int *parent,
> +					     unsigned int *parent_type)
> +{
> +	/* All these interrupts are level high in the CPU */
> +	*parent_type = IRQ_TYPE_LEVEL_HIGH;

It is bizare that you enforce LEVEL_HIGH here, while setting it to NONE
in the PLIC driver. These things should be consistent.

> +	*parent = child + 7;

Irk, magic numbers...

         M.
Bartosz Golaszewski Nov. 13, 2019, 1:10 p.m. UTC | #2
wt., 12 lis 2019 o 13:12 Yash Shah <yash.shah@sifive.com> napisał(a):
>
> Adds the GPIO driver for SiFive RISC-V SoCs.
>
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
>  drivers/gpio/Kconfig       |   9 ++
>  drivers/gpio/Makefile      |   1 +
>  drivers/gpio/gpio-sifive.c | 255 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 265 insertions(+)
>  create mode 100644 drivers/gpio/gpio-sifive.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 38e096e..05e8a41 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -453,6 +453,15 @@ config GPIO_SAMA5D2_PIOBU
>           The difference from regular GPIOs is that they
>           maintain their value during backup/self-refresh.
>
> +config GPIO_SIFIVE
> +       bool "SiFive GPIO support"
> +       depends on OF_GPIO
> +       select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP
> +       select REGMAP_MMIO
> +       help
> +         Say yes here to support the GPIO device on SiFive SoCs.
> +
>  config GPIO_SIOX
>         tristate "SIOX GPIO support"
>         depends on SIOX
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index d2fd19c..bf7984e 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -121,6 +121,7 @@ obj-$(CONFIG_ARCH_SA1100)           += gpio-sa1100.o
>  obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)       += gpio-sama5d2-piobu.o
>  obj-$(CONFIG_GPIO_SCH311X)             += gpio-sch311x.o
>  obj-$(CONFIG_GPIO_SCH)                 += gpio-sch.o
> +obj-$(CONFIG_GPIO_SIFIVE)              += gpio-sifive.o
>  obj-$(CONFIG_GPIO_SIOX)                        += gpio-siox.o
>  obj-$(CONFIG_GPIO_SODAVILLE)           += gpio-sodaville.o
>  obj-$(CONFIG_GPIO_SPEAR_SPICS)         += gpio-spear-spics.o
> diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
> new file mode 100644
> index 0000000..abdf839
> --- /dev/null
> +++ b/drivers/gpio/gpio-sifive.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 SiFive
> + */
> +#include <linux/bitops.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/of_irq.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/regmap.h>
> +
> +#define GPIO_INPUT_VAL 0x00
> +#define GPIO_INPUT_EN  0x04
> +#define GPIO_OUTPUT_EN 0x08
> +#define GPIO_OUTPUT_VAL        0x0C
> +#define GPIO_RISE_IE   0x18
> +#define GPIO_RISE_IP   0x1C
> +#define GPIO_FALL_IE   0x20
> +#define GPIO_FALL_IP   0x24
> +#define GPIO_HIGH_IE   0x28
> +#define GPIO_HIGH_IP   0x2C
> +#define GPIO_LOW_IE    0x30
> +#define GPIO_LOW_IP    0x34
> +#define GPIO_OUTPUT_XOR        0x40
> +
> +#define MAX_GPIO       32
> +
> +struct sifive_gpio {
> +       raw_spinlock_t          lock;
> +       void __iomem            *base;
> +       struct gpio_chip        gc;
> +       struct regmap           *regs;
> +       u32                     enabled;
> +       unsigned int            trigger[MAX_GPIO];
> +       unsigned int            irq_parent[MAX_GPIO];
> +};
> +
> +static void sifive_set_ie(struct sifive_gpio *chip, unsigned int offset)
> +{
> +       unsigned long flags;
> +       unsigned int trigger;
> +
> +       raw_spin_lock_irqsave(&chip->lock, flags);
> +       trigger = (chip->enabled & BIT(offset)) ? chip->trigger[offset] : 0;
> +       regmap_update_bits(chip->regs, GPIO_RISE_IE, BIT(offset),
> +                          (trigger & IRQ_TYPE_EDGE_RISING) ? BIT(offset) : 0);
> +       regmap_update_bits(chip->regs, GPIO_FALL_IE, BIT(offset),
> +                          (trigger & IRQ_TYPE_EDGE_FALLING) ? BIT(offset) : 0);
> +       regmap_update_bits(chip->regs, GPIO_HIGH_IE, BIT(offset),
> +                          (trigger & IRQ_TYPE_LEVEL_HIGH) ? BIT(offset) : 0);
> +       regmap_update_bits(chip->regs, GPIO_LOW_IE, BIT(offset),
> +                          (trigger & IRQ_TYPE_LEVEL_LOW) ? BIT(offset) : 0);
> +       raw_spin_unlock_irqrestore(&chip->lock, flags);
> +}
> +
> +static int sifive_irq_set_type(struct irq_data *d, unsigned int trigger)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct sifive_gpio *chip = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(d);
> +
> +       if (offset < 0 || offset >= gc->ngpio)
> +               return -EINVAL;
> +
> +       chip->trigger[offset] = trigger;
> +       sifive_set_ie(chip, offset);
> +       return 0;
> +}
> +
> +static void sifive_irq_enable(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct sifive_gpio *chip = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(d) % MAX_GPIO;
> +       u32 bit = BIT(offset);
> +
> +       irq_chip_enable_parent(d);
> +
> +       /* Switch to input */
> +       gc->direction_input(gc, offset);
> +
> +       /* Clear any sticky pending interrupts */
> +       iowrite32(bit, chip->base + GPIO_RISE_IP);
> +       iowrite32(bit, chip->base + GPIO_FALL_IP);
> +       iowrite32(bit, chip->base + GPIO_HIGH_IP);
> +       iowrite32(bit, chip->base + GPIO_LOW_IP);
> +
> +       /* Enable interrupts */
> +       assign_bit(offset, (unsigned long *)&chip->enabled, 1);
> +       sifive_set_ie(chip, offset);
> +}
> +
> +static void sifive_irq_disable(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct sifive_gpio *chip = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(d) % MAX_GPIO;
> +
> +       assign_bit(offset, (unsigned long *)&chip->enabled, 0);
> +       sifive_set_ie(chip, offset);
> +       irq_chip_disable_parent(d);
> +}
> +
> +static void sifive_irq_eoi(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct sifive_gpio *chip = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(d) % MAX_GPIO;
> +       u32 bit = BIT(offset);
> +
> +       /* Clear all pending interrupts */
> +       iowrite32(bit, chip->base + GPIO_RISE_IP);
> +       iowrite32(bit, chip->base + GPIO_FALL_IP);
> +       iowrite32(bit, chip->base + GPIO_HIGH_IP);
> +       iowrite32(bit, chip->base + GPIO_LOW_IP);
> +
> +       irq_chip_eoi_parent(d);
> +}
> +
> +static struct irq_chip sifive_irqchip = {
> +       .name           = "sifive-gpio",
> +       .irq_set_type   = sifive_irq_set_type,
> +       .irq_mask       = irq_chip_mask_parent,
> +       .irq_unmask     = irq_chip_unmask_parent,
> +       .irq_enable     = sifive_irq_enable,
> +       .irq_disable    = sifive_irq_disable,
> +       .irq_eoi        = sifive_irq_eoi,
> +};
> +
> +static int sifive_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> +                                            unsigned int child,
> +                                            unsigned int child_type,
> +                                            unsigned int *parent,
> +                                            unsigned int *parent_type)
> +{
> +       /* All these interrupts are level high in the CPU */
> +       *parent_type = IRQ_TYPE_LEVEL_HIGH;
> +       *parent = child + 7;
> +       return 0;
> +}
> +
> +static const struct regmap_config sifive_gpio_regmap_config = {
> +       .reg_bits = 32,
> +       .reg_stride = 4,
> +       .val_bits = 32,
> +       .fast_io = true,
> +};
> +
> +static int sifive_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *node = pdev->dev.of_node;
> +       struct device_node *irq_parent;
> +       struct irq_domain *parent;
> +       struct gpio_irq_chip *girq;
> +       struct sifive_gpio *chip;
> +       struct resource *res;
> +       int ret, ngpio;
> +
> +       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +       if (!chip)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       chip->base = devm_ioremap_resource(dev, res);

Use devm_platform_ioremap_resource() and drop the res variable.

> +       if (IS_ERR(chip->base)) {
> +               dev_err(dev, "failed to allocate device memory\n");
> +               return PTR_ERR(chip->base);
> +       }
> +
> +       chip->regs = devm_regmap_init_mmio(dev, chip->base,
> +                                          &sifive_gpio_regmap_config);

Why do you need this regmap here? You initialize a new regmap, then
use your own locking despite not having disabled the internal locking
in regmap, and then you initialize the mmio generic GPIO code which
will use yet another lock to operate on the same registers and in the
end you write to those registers without taking any lock anyway.
Doesn't make much sense to me.

> +       if (IS_ERR(chip->regs))
> +               return PTR_ERR(chip->regs);
> +
> +       ngpio = of_irq_count(node);
> +       if (ngpio >= MAX_GPIO) {
> +               dev_err(dev, "Too many GPIO interrupts (max=%d)\n", MAX_GPIO);
> +               return -ENXIO;
> +       }
> +
> +       irq_parent = of_irq_find_parent(node);
> +       if (!irq_parent) {
> +               dev_err(dev, "no IRQ parent node\n");
> +               return -ENODEV;
> +       }
> +       parent = irq_find_host(irq_parent);
> +       if (!parent) {
> +               dev_err(dev, "no IRQ parent domain\n");
> +               return -ENODEV;
> +       }
> +
> +       ret = bgpio_init(&chip->gc, dev, 4,
> +                        chip->base + GPIO_INPUT_VAL,
> +                        chip->base + GPIO_OUTPUT_VAL,
> +                        NULL,
> +                        chip->base + GPIO_OUTPUT_EN,
> +                        chip->base + GPIO_INPUT_EN,
> +                        0);
> +       if (ret) {
> +               dev_err(dev, "unable to init generic GPIO\n");
> +               return ret;
> +       }
> +
> +       /* Disable all GPIO interrupts before enabling parent interrupts */
> +       iowrite32(0, chip->base + GPIO_RISE_IE);
> +       iowrite32(0, chip->base + GPIO_FALL_IE);
> +       iowrite32(0, chip->base + GPIO_HIGH_IE);
> +       iowrite32(0, chip->base + GPIO_LOW_IE);
> +       chip->enabled = 0;
> +
> +       raw_spin_lock_init(&chip->lock);
> +       chip->gc.base = -1;
> +       chip->gc.ngpio = ngpio;
> +       chip->gc.label = dev_name(dev);
> +       chip->gc.parent = dev;
> +       chip->gc.owner = THIS_MODULE;
> +       girq = &chip->gc.irq;
> +       girq->chip = &sifive_irqchip;
> +       girq->fwnode = of_node_to_fwnode(node);
> +       girq->parent_domain = parent;
> +       girq->child_to_parent_hwirq = sifive_gpio_child_to_parent_hwirq;
> +       girq->handler = handle_bad_irq;
> +       girq->default_type = IRQ_TYPE_NONE;
> +
> +       ret = gpiochip_add_data(&chip->gc, chip);
> +       if (ret)
> +               return ret;
> +
> +       platform_set_drvdata(pdev, chip);
> +       dev_info(dev, "SiFive GPIO chip registered %d GPIOs\n", ngpio);

Core gpio library emits a very similar debug message from
gpiochip_setup_dev(), I think you can drop it and directly return
gpiochip_add_data().

Bartosz

> +
> +       return 0;
> +}
> +
> +static const struct of_device_id sifive_gpio_match[] = {
> +       { .compatible = "sifive,gpio0" },
> +       { .compatible = "sifive,fu540-c000-gpio" },
> +       { },
> +};
> +
> +static struct platform_driver sifive_gpio_driver = {
> +       .probe          = sifive_gpio_probe,
> +       .driver = {
> +               .name   = "sifive_gpio",
> +               .of_match_table = of_match_ptr(sifive_gpio_match),
> +       },
> +};
> +builtin_platform_driver(sifive_gpio_driver)
> --
> 2.7.4
>
Yash Shah Nov. 18, 2019, 7:50 a.m. UTC | #3
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 12 November 2019 18:28
> To: Yash Shah <yash.shah@sifive.com>
> Cc: linus.walleij@linaro.org; bgolaszewski@baylibre.com;
> robh+dt@kernel.org; mark.rutland@arm.com; palmer@dabbelt.com; Paul
> Walmsley ( Sifive) <paul.walmsley@sifive.com>; aou@eecs.berkeley.edu;
> tglx@linutronix.de; jason@lakedaemon.net; bmeng.cn@gmail.com;
> atish.patra@wdc.com; Sagar Kadam <sagar.kadam@sifive.com>; linux-
> gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> riscv@lists.infradead.org; linux-kernel@vger.kernel.org; Sachin Ghadi
> <sachin.ghadi@sifive.com>
> Subject: Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
> 
> On 2019-11-12 13:21, Yash Shah wrote:
> > Adds the GPIO driver for SiFive RISC-V SoCs.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> 
> [...]
> 
> > +static int sifive_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> > +					     unsigned int child,
> > +					     unsigned int child_type,
> > +					     unsigned int *parent,
> > +					     unsigned int *parent_type)
> > +{
> > +	/* All these interrupts are level high in the CPU */
> > +	*parent_type = IRQ_TYPE_LEVEL_HIGH;
> 
> It is bizare that you enforce LEVEL_HIGH here, while setting it to NONE in the
> PLIC driver. These things should be consistent.

Will change this to IRQ_TYPE_NONE.

> 
> > +	*parent = child + 7;
> 
> Irk, magic numbers...

This is the offset for GPIO IRQs. Will add a macro for this.
Thanks for your comments!

- Yash
Yash Shah Nov. 18, 2019, 10:03 a.m. UTC | #4
> -----Original Message-----
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Sent: 13 November 2019 18:41
> To: Yash Shah <yash.shah@sifive.com>
> Cc: linus.walleij@linaro.org; robh+dt@kernel.org; mark.rutland@arm.com;
> palmer@dabbelt.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
> aou@eecs.berkeley.edu; tglx@linutronix.de; jason@lakedaemon.net;
> maz@kernel.org; bmeng.cn@gmail.com; atish.patra@wdc.com; Sagar Kadam
> <sagar.kadam@sifive.com>; linux-gpio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-riscv@lists.infradead.org; linux-
> kernel@vger.kernel.org; Sachin Ghadi <sachin.ghadi@sifive.com>
> Subject: Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
> 
> wt., 12 lis 2019 o 13:12 Yash Shah <yash.shah@sifive.com> napisał(a):
> >
> > Adds the GPIO driver for SiFive RISC-V SoCs.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>

[...]

> > +
> > +static int sifive_gpio_probe(struct platform_device *pdev) {
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *node = pdev->dev.of_node;
> > +       struct device_node *irq_parent;
> > +       struct irq_domain *parent;
> > +       struct gpio_irq_chip *girq;
> > +       struct sifive_gpio *chip;
> > +       struct resource *res;
> > +       int ret, ngpio;
> > +
> > +       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> > +       if (!chip)
> > +               return -ENOMEM;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       chip->base = devm_ioremap_resource(dev, res);
> 
> Use devm_platform_ioremap_resource() and drop the res variable.
> 

Sure, will do that.

> > +       if (IS_ERR(chip->base)) {
> > +               dev_err(dev, "failed to allocate device memory\n");
> > +               return PTR_ERR(chip->base);
> > +       }
> > +
> > +       chip->regs = devm_regmap_init_mmio(dev, chip->base,
> > +
> > + &sifive_gpio_regmap_config);
> 
> Why do you need this regmap here? You initialize a new regmap, then use
> your own locking despite not having disabled the internal locking in regmap,
> and then you initialize the mmio generic GPIO code which will use yet
> another lock to operate on the same registers and in the end you write to
> those registers without taking any lock anyway.
> Doesn't make much sense to me.
> 

As suggested in the comments received on the RFC version of this patch[0], I am trying to use regmap MMIO by looking at gpio-mvebu.c. I got your point regarding the usage of own locks is not making any sense.
Here is what I will do in v2:
1. drop the usage of own locks
2. consistently use regmap_* apis for register access (replace all iowrites).
Does this make sense now?

> > +       if (IS_ERR(chip->regs))
> > +               return PTR_ERR(chip->regs);
> > +

[...]

> > +
> > +       ret = gpiochip_add_data(&chip->gc, chip);
> > +       if (ret)
> > +               return ret;
> > +
> > +       platform_set_drvdata(pdev, chip);
> > +       dev_info(dev, "SiFive GPIO chip registered %d GPIOs\n",
> > + ngpio);
> 
> Core gpio library emits a very similar debug message from
> gpiochip_setup_dev(), I think you can drop it and directly return
> gpiochip_add_data().
> 
> Bartosz

Ok. Will directly return gpiochip_add_data().
Thanks for your comments!

- Yash

[0] https://lore.kernel.org/linux-riscv/20181010123519.RVexDppaPFpIWl7QU_hpP8tc5qqWPJgeuLYn0FaGbeQ@z/
Bartosz Golaszewski Nov. 18, 2019, 10:15 a.m. UTC | #5
pon., 18 lis 2019 o 11:03 Yash Shah <yash.shah@sifive.com> napisał(a):
>
> > -----Original Message-----
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > Sent: 13 November 2019 18:41
> > To: Yash Shah <yash.shah@sifive.com>
> > Cc: linus.walleij@linaro.org; robh+dt@kernel.org; mark.rutland@arm.com;
> > palmer@dabbelt.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
> > aou@eecs.berkeley.edu; tglx@linutronix.de; jason@lakedaemon.net;
> > maz@kernel.org; bmeng.cn@gmail.com; atish.patra@wdc.com; Sagar Kadam
> > <sagar.kadam@sifive.com>; linux-gpio@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-riscv@lists.infradead.org; linux-
> > kernel@vger.kernel.org; Sachin Ghadi <sachin.ghadi@sifive.com>
> > Subject: Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
> >
> > wt., 12 lis 2019 o 13:12 Yash Shah <yash.shah@sifive.com> napisał(a):
> > >
> > > Adds the GPIO driver for SiFive RISC-V SoCs.
> > >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
>
> [...]
>
> > > +
> > > +static int sifive_gpio_probe(struct platform_device *pdev) {
> > > +       struct device *dev = &pdev->dev;
> > > +       struct device_node *node = pdev->dev.of_node;
> > > +       struct device_node *irq_parent;
> > > +       struct irq_domain *parent;
> > > +       struct gpio_irq_chip *girq;
> > > +       struct sifive_gpio *chip;
> > > +       struct resource *res;
> > > +       int ret, ngpio;
> > > +
> > > +       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> > > +       if (!chip)
> > > +               return -ENOMEM;
> > > +
> > > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +       chip->base = devm_ioremap_resource(dev, res);
> >
> > Use devm_platform_ioremap_resource() and drop the res variable.
> >
>
> Sure, will do that.
>
> > > +       if (IS_ERR(chip->base)) {
> > > +               dev_err(dev, "failed to allocate device memory\n");
> > > +               return PTR_ERR(chip->base);
> > > +       }
> > > +
> > > +       chip->regs = devm_regmap_init_mmio(dev, chip->base,
> > > +
> > > + &sifive_gpio_regmap_config);
> >
> > Why do you need this regmap here? You initialize a new regmap, then use
> > your own locking despite not having disabled the internal locking in regmap,
> > and then you initialize the mmio generic GPIO code which will use yet
> > another lock to operate on the same registers and in the end you write to
> > those registers without taking any lock anyway.
> > Doesn't make much sense to me.
> >
>
> As suggested in the comments received on the RFC version of this patch[0], I am trying to use regmap MMIO by looking at gpio-mvebu.c. I got your point regarding the usage of own locks is not making any sense.
> Here is what I will do in v2:
> 1. drop the usage of own locks
> 2. consistently use regmap_* apis for register access (replace all iowrites).
> Does this make sense now?

The thing is: the gpio-mmio code you're (correctly) reusing uses a
different lock - namely: bgpio_lock in struct gpio_chip. If you want
to use regmap for register operations, then you need to set
disable_locking in regmap_config to true and then take this lock
manually on every access.

Bart

>
> > > +       if (IS_ERR(chip->regs))
> > > +               return PTR_ERR(chip->regs);
> > > +
>
> [...]
>
> > > +
> > > +       ret = gpiochip_add_data(&chip->gc, chip);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       platform_set_drvdata(pdev, chip);
> > > +       dev_info(dev, "SiFive GPIO chip registered %d GPIOs\n",
> > > + ngpio);
> >
> > Core gpio library emits a very similar debug message from
> > gpiochip_setup_dev(), I think you can drop it and directly return
> > gpiochip_add_data().
> >
> > Bartosz
>
> Ok. Will directly return gpiochip_add_data().
> Thanks for your comments!
>
> - Yash
>
> [0] https://lore.kernel.org/linux-riscv/20181010123519.RVexDppaPFpIWl7QU_hpP8tc5qqWPJgeuLYn0FaGbeQ@z/
Linus Walleij Nov. 19, 2019, 3:02 p.m. UTC | #6
On Mon, Nov 18, 2019 at 11:15 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> pon., 18 lis 2019 o 11:03 Yash Shah <yash.shah@sifive.com> napisał(a):

> > As suggested in the comments received on the RFC version of this patch[0], I am trying to use regmap MMIO by looking at gpio-mvebu.c. I got your point regarding the usage of own locks is not making any sense.
> > Here is what I will do in v2:
> > 1. drop the usage of own locks
> > 2. consistently use regmap_* apis for register access (replace all iowrites).
> > Does this make sense now?
>
> The thing is: the gpio-mmio code you're (correctly) reusing uses a
> different lock - namely: bgpio_lock in struct gpio_chip. If you want
> to use regmap for register operations, then you need to set
> disable_locking in regmap_config to true and then take this lock
> manually on every access.

Is it really so? The bgpio_lock does protect the registers used
by regmap-mmio but unless the interrupt code is also using the
same registers it is fine to have a different lock for those.

Is the interrupt code really poking into the very same registers
as passed to bgpio_init()?

Of course it could be seen as a bit dirty to poke around in the
same memory space with regmap and the bgpio_* accessors
but in practice it's no problem if they never touch the same
things.

Yours,
Linus Walleij
Bartosz Golaszewski Nov. 19, 2019, 4:41 p.m. UTC | #7
wt., 19 lis 2019 o 16:03 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Mon, Nov 18, 2019 at 11:15 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > pon., 18 lis 2019 o 11:03 Yash Shah <yash.shah@sifive.com> napisał(a):
>
> > > As suggested in the comments received on the RFC version of this patch[0], I am trying to use regmap MMIO by looking at gpio-mvebu.c. I got your point regarding the usage of own locks is not making any sense.
> > > Here is what I will do in v2:
> > > 1. drop the usage of own locks
> > > 2. consistently use regmap_* apis for register access (replace all iowrites).
> > > Does this make sense now?
> >
> > The thing is: the gpio-mmio code you're (correctly) reusing uses a
> > different lock - namely: bgpio_lock in struct gpio_chip. If you want
> > to use regmap for register operations, then you need to set
> > disable_locking in regmap_config to true and then take this lock
> > manually on every access.
>
> Is it really so? The bgpio_lock does protect the registers used
> by regmap-mmio but unless the interrupt code is also using the
> same registers it is fine to have a different lock for those.
>
> Is the interrupt code really poking into the very same registers
> as passed to bgpio_init()?
>
> Of course it could be seen as a bit dirty to poke around in the
> same memory space with regmap and the bgpio_* accessors
> but in practice it's no problem if they never touch the same
> things.
>
> Yours,
> Linus Walleij

I'm wondering if it won't cause any inconsistencies when for example
interrupts are being triggered on input lines while we're also reading
their values? Seems to me it's just more clear to use a single lock
for a register range. Most drivers using gpio-mmio do just that in
their irq-related routines.

Anyway: even without using bgpio_lock this code is inconsistent: if
we're using regmap for interrupt registers, we should either decide to
rely on locking provided by regmap or disable it and use a locally
defined lock. Also: if we're using regmap, then let's use it
everywhere, not only when it's convenient for updating registers.

Bart
Linus Walleij Nov. 22, 2019, 12:28 p.m. UTC | #8
On Tue, Nov 19, 2019 at 5:42 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> wt., 19 lis 2019 o 16:03 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> > On Mon, Nov 18, 2019 at 11:15 AM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:

> > > pon., 18 lis 2019 o 11:03 Yash Shah <yash.shah@sifive.com> napisał(a):
> > Is it really so? The bgpio_lock does protect the registers used
> > by regmap-mmio but unless the interrupt code is also using the
> > same registers it is fine to have a different lock for those.
> >
> > Is the interrupt code really poking into the very same registers
> > as passed to bgpio_init()?
> >
> > Of course it could be seen as a bit dirty to poke around in the
> > same memory space with regmap and the bgpio_* accessors
> > but in practice it's no problem if they never touch the same
> > things.
> >
> > Yours,
> > Linus Walleij
>
> I'm wondering if it won't cause any inconsistencies when for example
> interrupts are being triggered on input lines while we're also reading
> their values? Seems to me it's just more clear to use a single lock
> for a register range. Most drivers using gpio-mmio do just that in
> their irq-related routines.

OK good point. Just one lock for the whole thing is likely
more maintainable even if it works with two different locks.

> Anyway: even without using bgpio_lock this code is inconsistent: if
> we're using regmap for interrupt registers, we should either decide to
> rely on locking provided by regmap or disable it and use a locally
> defined lock.

OK makes sense, let's say we use the bgpio_lock everywhere
for this.

Yash: are you OK with this? (Haven't read the new patch set
yet, maybe it is already fixed...)

> Also: if we're using regmap, then let's use it
> everywhere, not only when it's convenient for updating registers.

I think what you are saying is that we should extend gpio-mmio.c
with some optional regmap API (or create a separate MMIO library
for regmap consumers) which makes sense, but it feels a bit
heavy task to toss at contributors.

We could add it to the TODO file, where I already have some
item like this for port-mapped I/O.

Yours,
Linus Walleij
Bartosz Golaszewski Nov. 22, 2019, 12:39 p.m. UTC | #9
pt., 22 lis 2019 o 13:28 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Tue, Nov 19, 2019 at 5:42 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > wt., 19 lis 2019 o 16:03 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> > > On Mon, Nov 18, 2019 at 11:15 AM Bartosz Golaszewski
> > > <bgolaszewski@baylibre.com> wrote:
>
> > > > pon., 18 lis 2019 o 11:03 Yash Shah <yash.shah@sifive.com> napisał(a):
> > > Is it really so? The bgpio_lock does protect the registers used
> > > by regmap-mmio but unless the interrupt code is also using the
> > > same registers it is fine to have a different lock for those.
> > >
> > > Is the interrupt code really poking into the very same registers
> > > as passed to bgpio_init()?
> > >
> > > Of course it could be seen as a bit dirty to poke around in the
> > > same memory space with regmap and the bgpio_* accessors
> > > but in practice it's no problem if they never touch the same
> > > things.
> > >
> > > Yours,
> > > Linus Walleij
> >
> > I'm wondering if it won't cause any inconsistencies when for example
> > interrupts are being triggered on input lines while we're also reading
> > their values? Seems to me it's just more clear to use a single lock
> > for a register range. Most drivers using gpio-mmio do just that in
> > their irq-related routines.
>
> OK good point. Just one lock for the whole thing is likely
> more maintainable even if it works with two different locks.
>
> > Anyway: even without using bgpio_lock this code is inconsistent: if
> > we're using regmap for interrupt registers, we should either decide to
> > rely on locking provided by regmap or disable it and use a locally
> > defined lock.
>
> OK makes sense, let's say we use the bgpio_lock everywhere
> for this.
>
> Yash: are you OK with this? (Haven't read the new patch set
> yet, maybe it is already fixed...)
>
> > Also: if we're using regmap, then let's use it
> > everywhere, not only when it's convenient for updating registers.
>
> I think what you are saying is that we should extend gpio-mmio.c
> with some optional regmap API (or create a separate MMIO library
> for regmap consumers) which makes sense, but it feels a bit
> heavy task to toss at contributors.
>
> We could add it to the TODO file, where I already have some
> item like this for port-mapped I/O.
>

It's been on my personal TODO list too for some time as it seems it
could benefit some i2c drivers too. Also: I think such a helper could
eventually completely replace the generic drivers such as gpio-mmio
and gpio-reg.

In other words: good idea to put it into the TODO. If there are no
objections I was thinking about starting the work soon aiming at v5.6,
as soon as we get the recent changes in uAPI out of the way.

Bart

> Yours,
> Linus Walleij
Yash Shah Nov. 25, 2019, 4:54 a.m. UTC | #10
> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: 22 November 2019 17:58
> To: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Yash Shah <yash.shah@sifive.com>; robh+dt@kernel.org;
> mark.rutland@arm.com; palmer@dabbelt.com; Paul Walmsley ( Sifive)
> <paul.walmsley@sifive.com>; aou@eecs.berkeley.edu; tglx@linutronix.de;
> jason@lakedaemon.net; maz@kernel.org; bmeng.cn@gmail.com;
> atish.patra@wdc.com; Sagar Kadam <sagar.kadam@sifive.com>; linux-
> gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> riscv@lists.infradead.org; linux-kernel@vger.kernel.org; Sachin Ghadi
> <sachin.ghadi@sifive.com>
> Subject: Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
> 
> On Tue, Nov 19, 2019 at 5:42 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > wt., 19 lis 2019 o 16:03 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> > > On Mon, Nov 18, 2019 at 11:15 AM Bartosz Golaszewski
> > > <bgolaszewski@baylibre.com> wrote:
> 
> > > > pon., 18 lis 2019 o 11:03 Yash Shah <yash.shah@sifive.com> napisał(a):
> > > Is it really so? The bgpio_lock does protect the registers used by
> > > regmap-mmio but unless the interrupt code is also using the same
> > > registers it is fine to have a different lock for those.
> > >
> > > Is the interrupt code really poking into the very same registers as
> > > passed to bgpio_init()?
> > >
> > > Of course it could be seen as a bit dirty to poke around in the same
> > > memory space with regmap and the bgpio_* accessors but in practice
> > > it's no problem if they never touch the same things.
> > >
> > > Yours,
> > > Linus Walleij
> >
> > I'm wondering if it won't cause any inconsistencies when for example
> > interrupts are being triggered on input lines while we're also reading
> > their values? Seems to me it's just more clear to use a single lock
> > for a register range. Most drivers using gpio-mmio do just that in
> > their irq-related routines.
> 
> OK good point. Just one lock for the whole thing is likely more maintainable
> even if it works with two different locks.
> 
> > Anyway: even without using bgpio_lock this code is inconsistent: if
> > we're using regmap for interrupt registers, we should either decide to
> > rely on locking provided by regmap or disable it and use a locally
> > defined lock.
> 
> OK makes sense, let's say we use the bgpio_lock everywhere for this.
> 
> Yash: are you OK with this? (Haven't read the new patch set yet, maybe it is
> already fixed...)

Yes, I am ok with this. I will be sending v3 soon with this change.

- Yash

Patch
diff mbox series

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 38e096e..05e8a41 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -453,6 +453,15 @@  config GPIO_SAMA5D2_PIOBU
 	  The difference from regular GPIOs is that they
 	  maintain their value during backup/self-refresh.
 
+config GPIO_SIFIVE
+	bool "SiFive GPIO support"
+	depends on OF_GPIO
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	select REGMAP_MMIO
+	help
+	  Say yes here to support the GPIO device on SiFive SoCs.
+
 config GPIO_SIOX
 	tristate "SIOX GPIO support"
 	depends on SIOX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d2fd19c..bf7984e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -121,6 +121,7 @@  obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
+obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
 obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
 obj-$(CONFIG_GPIO_SPEAR_SPICS)		+= gpio-spear-spics.o
diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
new file mode 100644
index 0000000..abdf839
--- /dev/null
+++ b/drivers/gpio/gpio-sifive.c
@@ -0,0 +1,255 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 SiFive
+ */
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/of_irq.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/regmap.h>
+
+#define GPIO_INPUT_VAL	0x00
+#define GPIO_INPUT_EN	0x04
+#define GPIO_OUTPUT_EN	0x08
+#define GPIO_OUTPUT_VAL	0x0C
+#define GPIO_RISE_IE	0x18
+#define GPIO_RISE_IP	0x1C
+#define GPIO_FALL_IE	0x20
+#define GPIO_FALL_IP	0x24
+#define GPIO_HIGH_IE	0x28
+#define GPIO_HIGH_IP	0x2C
+#define GPIO_LOW_IE	0x30
+#define GPIO_LOW_IP	0x34
+#define GPIO_OUTPUT_XOR	0x40
+
+#define MAX_GPIO	32
+
+struct sifive_gpio {
+	raw_spinlock_t		lock;
+	void __iomem		*base;
+	struct gpio_chip	gc;
+	struct regmap		*regs;
+	u32			enabled;
+	unsigned int		trigger[MAX_GPIO];
+	unsigned int		irq_parent[MAX_GPIO];
+};
+
+static void sifive_set_ie(struct sifive_gpio *chip, unsigned int offset)
+{
+	unsigned long flags;
+	unsigned int trigger;
+
+	raw_spin_lock_irqsave(&chip->lock, flags);
+	trigger = (chip->enabled & BIT(offset)) ? chip->trigger[offset] : 0;
+	regmap_update_bits(chip->regs, GPIO_RISE_IE, BIT(offset),
+			   (trigger & IRQ_TYPE_EDGE_RISING) ? BIT(offset) : 0);
+	regmap_update_bits(chip->regs, GPIO_FALL_IE, BIT(offset),
+			   (trigger & IRQ_TYPE_EDGE_FALLING) ? BIT(offset) : 0);
+	regmap_update_bits(chip->regs, GPIO_HIGH_IE, BIT(offset),
+			   (trigger & IRQ_TYPE_LEVEL_HIGH) ? BIT(offset) : 0);
+	regmap_update_bits(chip->regs, GPIO_LOW_IE, BIT(offset),
+			   (trigger & IRQ_TYPE_LEVEL_LOW) ? BIT(offset) : 0);
+	raw_spin_unlock_irqrestore(&chip->lock, flags);
+}
+
+static int sifive_irq_set_type(struct irq_data *d, unsigned int trigger)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct sifive_gpio *chip = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(d);
+
+	if (offset < 0 || offset >= gc->ngpio)
+		return -EINVAL;
+
+	chip->trigger[offset] = trigger;
+	sifive_set_ie(chip, offset);
+	return 0;
+}
+
+static void sifive_irq_enable(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct sifive_gpio *chip = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(d) % MAX_GPIO;
+	u32 bit = BIT(offset);
+
+	irq_chip_enable_parent(d);
+
+	/* Switch to input */
+	gc->direction_input(gc, offset);
+
+	/* Clear any sticky pending interrupts */
+	iowrite32(bit, chip->base + GPIO_RISE_IP);
+	iowrite32(bit, chip->base + GPIO_FALL_IP);
+	iowrite32(bit, chip->base + GPIO_HIGH_IP);
+	iowrite32(bit, chip->base + GPIO_LOW_IP);
+
+	/* Enable interrupts */
+	assign_bit(offset, (unsigned long *)&chip->enabled, 1);
+	sifive_set_ie(chip, offset);
+}
+
+static void sifive_irq_disable(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct sifive_gpio *chip = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(d) % MAX_GPIO;
+
+	assign_bit(offset, (unsigned long *)&chip->enabled, 0);
+	sifive_set_ie(chip, offset);
+	irq_chip_disable_parent(d);
+}
+
+static void sifive_irq_eoi(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct sifive_gpio *chip = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(d) % MAX_GPIO;
+	u32 bit = BIT(offset);
+
+	/* Clear all pending interrupts */
+	iowrite32(bit, chip->base + GPIO_RISE_IP);
+	iowrite32(bit, chip->base + GPIO_FALL_IP);
+	iowrite32(bit, chip->base + GPIO_HIGH_IP);
+	iowrite32(bit, chip->base + GPIO_LOW_IP);
+
+	irq_chip_eoi_parent(d);
+}
+
+static struct irq_chip sifive_irqchip = {
+	.name		= "sifive-gpio",
+	.irq_set_type	= sifive_irq_set_type,
+	.irq_mask	= irq_chip_mask_parent,
+	.irq_unmask	= irq_chip_unmask_parent,
+	.irq_enable	= sifive_irq_enable,
+	.irq_disable	= sifive_irq_disable,
+	.irq_eoi	= sifive_irq_eoi,
+};
+
+static int sifive_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
+					     unsigned int child,
+					     unsigned int child_type,
+					     unsigned int *parent,
+					     unsigned int *parent_type)
+{
+	/* All these interrupts are level high in the CPU */
+	*parent_type = IRQ_TYPE_LEVEL_HIGH;
+	*parent = child + 7;
+	return 0;
+}
+
+static const struct regmap_config sifive_gpio_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+};
+
+static int sifive_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = pdev->dev.of_node;
+	struct device_node *irq_parent;
+	struct irq_domain *parent;
+	struct gpio_irq_chip *girq;
+	struct sifive_gpio *chip;
+	struct resource *res;
+	int ret, ngpio;
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	chip->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(chip->base)) {
+		dev_err(dev, "failed to allocate device memory\n");
+		return PTR_ERR(chip->base);
+	}
+
+	chip->regs = devm_regmap_init_mmio(dev, chip->base,
+					   &sifive_gpio_regmap_config);
+	if (IS_ERR(chip->regs))
+		return PTR_ERR(chip->regs);
+
+	ngpio = of_irq_count(node);
+	if (ngpio >= MAX_GPIO) {
+		dev_err(dev, "Too many GPIO interrupts (max=%d)\n", MAX_GPIO);
+		return -ENXIO;
+	}
+
+	irq_parent = of_irq_find_parent(node);
+	if (!irq_parent) {
+		dev_err(dev, "no IRQ parent node\n");
+		return -ENODEV;
+	}
+	parent = irq_find_host(irq_parent);
+	if (!parent) {
+		dev_err(dev, "no IRQ parent domain\n");
+		return -ENODEV;
+	}
+
+	ret = bgpio_init(&chip->gc, dev, 4,
+			 chip->base + GPIO_INPUT_VAL,
+			 chip->base + GPIO_OUTPUT_VAL,
+			 NULL,
+			 chip->base + GPIO_OUTPUT_EN,
+			 chip->base + GPIO_INPUT_EN,
+			 0);
+	if (ret) {
+		dev_err(dev, "unable to init generic GPIO\n");
+		return ret;
+	}
+
+	/* Disable all GPIO interrupts before enabling parent interrupts */
+	iowrite32(0, chip->base + GPIO_RISE_IE);
+	iowrite32(0, chip->base + GPIO_FALL_IE);
+	iowrite32(0, chip->base + GPIO_HIGH_IE);
+	iowrite32(0, chip->base + GPIO_LOW_IE);
+	chip->enabled = 0;
+
+	raw_spin_lock_init(&chip->lock);
+	chip->gc.base = -1;
+	chip->gc.ngpio = ngpio;
+	chip->gc.label = dev_name(dev);
+	chip->gc.parent = dev;
+	chip->gc.owner = THIS_MODULE;
+	girq = &chip->gc.irq;
+	girq->chip = &sifive_irqchip;
+	girq->fwnode = of_node_to_fwnode(node);
+	girq->parent_domain = parent;
+	girq->child_to_parent_hwirq = sifive_gpio_child_to_parent_hwirq;
+	girq->handler = handle_bad_irq;
+	girq->default_type = IRQ_TYPE_NONE;
+
+	ret = gpiochip_add_data(&chip->gc, chip);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, chip);
+	dev_info(dev, "SiFive GPIO chip registered %d GPIOs\n", ngpio);
+
+	return 0;
+}
+
+static const struct of_device_id sifive_gpio_match[] = {
+	{ .compatible = "sifive,gpio0" },
+	{ .compatible = "sifive,fu540-c000-gpio" },
+	{ },
+};
+
+static struct platform_driver sifive_gpio_driver = {
+	.probe		= sifive_gpio_probe,
+	.driver = {
+		.name	= "sifive_gpio",
+		.of_match_table = of_match_ptr(sifive_gpio_match),
+	},
+};
+builtin_platform_driver(sifive_gpio_driver)