| Message ID | 1573560684-48104-4-git-send-email-yash.shah@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | GPIO & Hierarchy IRQ support for HiFive Unleashed | expand |
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.
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 >
> -----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
> -----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/
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/
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
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
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
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
> -----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
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)