[PATCH 0/4] GPIO & Hierarchy IRQ support for HiFive Unleashed

[PATCH 0/4] GPIO & Hierarchy IRQ support for HiFive Unleashed

[Yash Shah]

This patch series adds GPIO drivers, DT documentation and DT nodes for
HiFive Unleashed board. The gpio patches are mostly based on Wesley's patch.
The patchset also adds hierarchy irq domain support as it is required by this
gpio driver.

This patchset is based on Linux 5.4-rc6 and tested on HiFive Unleashed board

Changes since RFC version:
Incorporated below changes as suggested by Linus Walleij on RFC version of this
patchset[0]
- Dropped PWM patches as they are already merged.
- Include "GPIO_GENERIC" and "REGMAP_MMIO" in Kconfig select option
- Remove unwanted inclusion of header files
- Use regmap MMIO instead of customised sifive_assign_bit()
- Use GPIOLIB_GENERIC and bgpio_init() to set up the accessors
- Use hierarchical irqdomain

[0] https://lore.kernel.org/linux-riscv/20181010123519.RVexDppaPFpIWl7QU_hpP8tc5qqWPJgeuLYn0FaGbeQ@z/

Yash Shah (4):
  irqchip: sifive: Support hierarchy irq domain
  gpio: sifive: Add DT documentation for SiFive GPIO
  gpio: sifive: Add GPIO driver for SiFive SoCs
  riscv: dts: Add DT support for SiFive FU540 GPIO driver

 .../devicetree/bindings/gpio/gpio-sifive.txt       |  33 +++
 arch/riscv/boot/dts/sifive/fu540-c000.dtsi         |  14 +-
 .../riscv/boot/dts/sifive/hifive-unleashed-a00.dts |   4 +
 drivers/gpio/Kconfig                               |   9 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-sifive.c                         | 255 +++++++++++++++++++++
 drivers/irqchip/Kconfig                            |   1 +
 drivers/irqchip/irq-sifive-plic.c                  |  41 +++-
 8 files changed, 353 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sifive.txt
 create mode 100644 drivers/gpio/gpio-sifive.c

-- 
2.7.4

[PATCH 1/4] irqchip: sifive: Support hierarchy irq domain

[Yash Shah]

Add support for hierarchy irq domains. This is needed as pre-requisite for
gpio-sifive driver.

Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 drivers/irqchip/Kconfig           |  1 +
 drivers/irqchip/irq-sifive-plic.c | 41 +++++++++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index ccbb897..a398552 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -488,6 +488,7 @@ endmenu
 config SIFIVE_PLIC
 	bool "SiFive Platform-Level Interrupt Controller"
 	depends on RISCV
+	select IRQ_DOMAIN_HIERARCHY
 	help
 	   This enables support for the PLIC chip found in SiFive (and
 	   potentially other) RISC-V systems.  The PLIC controls devices
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 7d0a12f..2fa1c84 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -154,15 +154,48 @@ static struct irq_chip plic_chip = {
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
 			      irq_hw_number_t hwirq)
 {
-	irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
-	irq_set_chip_data(irq, NULL);
+	irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
+			    handle_fasteoi_irq, NULL, NULL);
 	irq_set_noprobe(irq);
 	return 0;
 }
 
+static int plic_irq_domain_translate(struct irq_domain *d,
+				     struct irq_fwspec *fwspec,
+				     unsigned long *hwirq, unsigned int *type)
+{
+	if (WARN_ON(fwspec->param_count < 1))
+		return -EINVAL;
+	*hwirq = fwspec->param[0];
+	*type = IRQ_TYPE_NONE;
+	return 0;
+}
+
+static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				 unsigned int nr_irqs, void *arg)
+{
+	int i, ret;
+	irq_hw_number_t hwirq;
+	unsigned int type = IRQ_TYPE_NONE;
+	struct irq_fwspec *fwspec = arg;
+
+	ret = plic_irq_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nr_irqs; i++) {
+		ret = plic_irqdomain_map(domain, virq + i, hwirq + i);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct irq_domain_ops plic_irqdomain_ops = {
-	.map		= plic_irqdomain_map,
-	.xlate		= irq_domain_xlate_onecell,
+	.translate	= plic_irq_domain_translate,
+	.alloc		= plic_irq_domain_alloc,
+	.free		= irq_domain_free_irqs_top,
 };
 
 static struct irq_domain *plic_irqdomain;
-- 
2.7.4

Re: [PATCH 1/4] irqchip: sifive: Support hierarchy irq domain

[Marc Zyngier]

On 2019-11-12 13:21, Yash Shah wrote:
> Add support for hierarchy irq domains. This is needed as 
> pre-requisite for
> gpio-sifive driver.
>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
>  drivers/irqchip/Kconfig           |  1 +
>  drivers/irqchip/irq-sifive-plic.c | 41
> +++++++++++++++++++++++++++++++++++----
>  2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index ccbb897..a398552 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -488,6 +488,7 @@ endmenu
>  config SIFIVE_PLIC
>  	bool "SiFive Platform-Level Interrupt Controller"
>  	depends on RISCV
> +	select IRQ_DOMAIN_HIERARCHY
>  	help
>  	   This enables support for the PLIC chip found in SiFive (and
>  	   potentially other) RISC-V systems.  The PLIC controls devices
> diff --git a/drivers/irqchip/irq-sifive-plic.c
> b/drivers/irqchip/irq-sifive-plic.c
> index 7d0a12f..2fa1c84 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -154,15 +154,48 @@ static struct irq_chip plic_chip = {
>  static int plic_irqdomain_map(struct irq_domain *d, unsigned int 
> irq,
>  			      irq_hw_number_t hwirq)
>  {
> -	irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
> -	irq_set_chip_data(irq, NULL);
> +	irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> +			    handle_fasteoi_irq, NULL, NULL);
>  	irq_set_noprobe(irq);
>  	return 0;
>  }
>
> +static int plic_irq_domain_translate(struct irq_domain *d,
> +				     struct irq_fwspec *fwspec,
> +				     unsigned long *hwirq, unsigned int *type)
> +{
> +	if (WARN_ON(fwspec->param_count < 1))
> +		return -EINVAL;
> +	*hwirq = fwspec->param[0];
> +	*type = IRQ_TYPE_NONE;
> +	return 0;
> +}

This is actually what should be called irq_domain_translate_onecell().

Consider implementing that instead, and using it in this driver. I'm
pretty sure other drivers could use it (I spotted irq-nvic.c).

> +
> +static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned
> int virq,
> +				 unsigned int nr_irqs, void *arg)
> +{
> +	int i, ret;
> +	irq_hw_number_t hwirq;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	struct irq_fwspec *fwspec = arg;
> +
> +	ret = plic_irq_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		ret = plic_irqdomain_map(domain, virq + i, hwirq + i);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct irq_domain_ops plic_irqdomain_ops = {
> -	.map		= plic_irqdomain_map,
> -	.xlate		= irq_domain_xlate_onecell,
> +	.translate	= plic_irq_domain_translate,
> +	.alloc		= plic_irq_domain_alloc,
> +	.free		= irq_domain_free_irqs_top,
>  };
>
>  static struct irq_domain *plic_irqdomain;

Otherwise, looks OK.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH 1/4] irqchip: sifive: Support hierarchy irq domain

[Yash Shah]

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 12 November 2019 18:13
> 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 1/4] irqchip: sifive: Support hierarchy irq domain
> 
> On 2019-11-12 13:21, Yash Shah wrote:
> > Add support for hierarchy irq domains. This is needed as pre-requisite
> > for gpio-sifive driver.
> >
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> >  drivers/irqchip/Kconfig           |  1 +
> >  drivers/irqchip/irq-sifive-plic.c | 41
> > +++++++++++++++++++++++++++++++++++----
> >  2 files changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > ccbb897..a398552 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -488,6 +488,7 @@ endmenu
> >  config SIFIVE_PLIC
> >  	bool "SiFive Platform-Level Interrupt Controller"
> >  	depends on RISCV
> > +	select IRQ_DOMAIN_HIERARCHY
> >  	help
...
> >
> > +static int plic_irq_domain_translate(struct irq_domain *d,
> > +				     struct irq_fwspec *fwspec,
> > +				     unsigned long *hwirq, unsigned int *type)
> {
> > +	if (WARN_ON(fwspec->param_count < 1))
> > +		return -EINVAL;
> > +	*hwirq = fwspec->param[0];
> > +	*type = IRQ_TYPE_NONE;
> > +	return 0;
> > +}
> 
> This is actually what should be called irq_domain_translate_onecell().
> 
> Consider implementing that instead, and using it in this driver. I'm pretty sure
> other drivers could use it (I spotted irq-nvic.c).

Sure, will implement irq_domain_translate_onecell() and use that instead.
Thanks for your comments!

- Yash

> 
> >
> >  static struct irq_domain *plic_irqdomain;
> 
> Otherwise, looks OK.
> 
> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...

[PATCH 2/4] gpio: sifive: Add DT documentation for SiFive GPIO

[Yash Shah]

DT documentation for GPIO controller added.

Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
[Atish: Compatible string update]
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 .../devicetree/bindings/gpio/gpio-sifive.txt       | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sifive.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-sifive.txt b/Documentation/devicetree/bindings/gpio/gpio-sifive.txt
new file mode 100644
index 0000000..d3416d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-sifive.txt
@@ -0,0 +1,33 @@
+SiFive GPIO controller bindings
+
+Required properties:
+- compatible: Should be "sifive,<chip>-gpio" and "sifive,gpio<version>".
+  Supported compatible strings are: "sifive,fu540-c000-gpio" for the SiFive
+  GPIO v0 as integrated onto the SiFive FU540 chip, and "sifive,gpio0" for the
+  SiFive GPIO v0 IP block with no chip integration tweaks.
+  Please refer to sifive-blocks-ip-versioning.txt for details.
+- reg: Physical base address and length of the controller's registers.
+- clocks: Should contain a clock identifier for the GPIO's parent clock.
+- #gpio-cells : Should be 2
+  - The first cell is the GPIO offset number.
+  - The second cell indicates the polarity of the GPIO
+- gpio-controller : Marks the device node as a GPIO controller.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells : Should be 2.
+  - The first cell is the GPIO offset number within the GPIO controller.
+  - The second cell is the edge/level to use for interrupt generation.
+- interrupts: Specify the interrupts, one per GPIO
+
+Example:
+
+gpio: gpio@10060000 {
+	compatible = "sifive,fu540-c000-gpio","sifive,gpio0";
+	interrupt-parent = <&plic>;
+	interrupts = <7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22>;
+	reg = <0x0 0x10060000 0x0 0x1000>;
+	clocks = <&tlclk>;
+	gpio-controller;
+	#gpio-cells = <2>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+};
-- 
2.7.4

Re: [PATCH 2/4] gpio: sifive: Add DT documentation for SiFive GPIO

[Rob Herring]

On Tue, Nov 12, 2019 at 12:12:06PM +0000, Yash Shah wrote:
> DT documentation for GPIO controller added.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Compatible string update]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
>  .../devicetree/bindings/gpio/gpio-sifive.txt       | 33 ++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sifive.txt

Please make this a schema. See 
Documentation/devicetree/writing-schema.rst.

> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-sifive.txt b/Documentation/devicetree/bindings/gpio/gpio-sifive.txt
> new file mode 100644
> index 0000000..d3416d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-sifive.txt
> @@ -0,0 +1,33 @@
> +SiFive GPIO controller bindings
> +
> +Required properties:
> +- compatible: Should be "sifive,<chip>-gpio" and "sifive,gpio<version>".
> +  Supported compatible strings are: "sifive,fu540-c000-gpio" for the SiFive
> +  GPIO v0 as integrated onto the SiFive FU540 chip, and "sifive,gpio0" for the
> +  SiFive GPIO v0 IP block with no chip integration tweaks.
> +  Please refer to sifive-blocks-ip-versioning.txt for details.
> +- reg: Physical base address and length of the controller's registers.
> +- clocks: Should contain a clock identifier for the GPIO's parent clock.
> +- #gpio-cells : Should be 2
> +  - The first cell is the GPIO offset number.
> +  - The second cell indicates the polarity of the GPIO
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- interrupt-controller: Marks the device node as an interrupt controller.
> +- #interrupt-cells : Should be 2.
> +  - The first cell is the GPIO offset number within the GPIO controller.
> +  - The second cell is the edge/level to use for interrupt generation.
> +- interrupts: Specify the interrupts, one per GPIO

How many GPIOs?

> +
> +Example:
> +
> +gpio: gpio@10060000 {
> +	compatible = "sifive,fu540-c000-gpio","sifive,gpio0";

space                                         ^

> +	interrupt-parent = <&plic>;
> +	interrupts = <7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22>;
> +	reg = <0x0 0x10060000 0x0 0x1000>;
> +	clocks = <&tlclk>;
> +	gpio-controller;
> +	#gpio-cells = <2>;
> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +};
> -- 
> 2.7.4
> 

[PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Yash Shah]

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);
+	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)
-- 
2.7.4

Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Yash Shah]

> -----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

Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Bartosz Golaszewski]

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
>

RE: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Yash Shah]

> -----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/

Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Bartosz Golaszewski]

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/

Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Linus Walleij]

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

Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Bartosz Golaszewski]

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

Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Linus Walleij]

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

Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Bartosz Golaszewski]

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

RE: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

[Yash Shah]

> -----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 4/4] riscv: dts: Add DT support for SiFive FU540 GPIO driver

[Yash Shah]

Add the gpio DT node in SiFive FU540 soc-specific DT file.
Enable the gpio node in HiFive Unleashed board-specific DT file.

Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 arch/riscv/boot/dts/sifive/fu540-c000.dtsi          | 14 +++++++++++++-
 arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
index afa43c7..2d7c284 100644
--- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
@@ -246,6 +246,18 @@
 			#pwm-cells = <3>;
 			status = "disabled";
 		};
-
+		gpio: gpio@10060000 {
+			compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
+			interrupt-parent = <&plic0>;
+			interrupts = <7 8 9 10 11 12 13 14 15
+				      16 17 18 19 20 21 22>;
+			reg = <0x0 0x10060000 0x0 0x1000>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			clocks = <&prci PRCI_CLK_TLCLK>;
+			status = "disabled";
+		};
 	};
 };
diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
index 88cfcb9..609198c 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
@@ -94,3 +94,7 @@
 &pwm1 {
 	status = "okay";
 };
+
+&gpio {
+	status = "okay";
+};
-- 
2.7.4