[PATCH 1/2] gpio: Add a driver for Cadence GPIO controller

[PATCH 1/2] gpio: Add a driver for Cadence GPIO controller

[Boris Brezillon]

Add a driver for Cadence GPIO controller.
Even though this driver is pretty simple, I was not able to use the
generic GPIO infrastructure because it needs custom ->request()/->free()
implementation and ->direction_output() requires modifying 2 different
registers while the generic implementation only modifies the dirin (or
dirout) register. Other than that, the implementation is rather
straightforward.

Note that irq support has not been tested.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/gpio/Kconfig        |   7 +
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-cadence.c | 334 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+)
 create mode 100644 drivers/gpio/gpio-cadence.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d5d36549ecc1..3f5cf6670fd6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -143,6 +143,13 @@ config GPIO_BRCMSTB
 	help
 	  Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
 
+config GPIO_CADENCE
+	tristate "Cadence GPIO support"
+	depends on OF_GPIO
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to enable support for Cadence GPIO controller.
+
 config GPIO_CLPS711X
 	tristate "CLPS711X GPIO support"
 	depends on ARCH_CLPS711X || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a7676b82de6f..9c1ae8a30d3f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_GPIO_AXP209)	+= gpio-axp209.o
 obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
 obj-$(CONFIG_GPIO_BRCMSTB)	+= gpio-brcmstb.o
 obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
+obj-$(CONFIG_GPIO_CADENCE)	+= gpio-cadence.o
 obj-$(CONFIG_GPIO_CLPS711X)	+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
 obj-$(CONFIG_GPIO_CRYSTAL_COVE)	+= gpio-crystalcove.o
diff --git a/drivers/gpio/gpio-cadence.c b/drivers/gpio/gpio-cadence.c
new file mode 100644
index 000000000000..fbf323ad2eec
--- /dev/null
+++ b/drivers/gpio/gpio-cadence.c
@@ -0,0 +1,334 @@
+/*
+ * Copyright 2017 Cadence
+ *
+ * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define CDNS_GPIO_BYPASS_MODE		0x0
+#define CDNS_GPIO_DIRECTION_MODE	0x4
+#define CDNS_GPIO_OUTPUT_EN		0x8
+#define CDNS_GPIO_OUTPUT_VALUE		0xc
+#define CDNS_GPIO_INPUT_VALUE		0x10
+#define CDNS_GPIO_IRQ_MASK		0x14
+#define CDNS_GPIO_IRQ_EN		0x18
+#define CDNS_GPIO_IRQ_DIS		0x1c
+#define CDNS_GPIO_IRQ_STATUS		0x20
+#define CDNS_GPIO_IRQ_TYPE		0x24
+#define CDNS_GPIO_IRQ_VALUE		0x28
+#define CDNS_GPIO_IRQ_ANY_EDGE		0x2c
+
+struct cdns_gpio_chip {
+	struct gpio_chip base;
+	struct irq_chip irqchip;
+	struct clk *pclk;
+	void __iomem *regs;
+};
+
+static inline struct cdns_gpio_chip *to_cdns_gpio(struct gpio_chip *chip)
+{
+	return container_of(chip, struct cdns_gpio_chip, base);
+}
+
+static int cdns_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+
+	iowrite32(ioread32(cgpio->regs + CDNS_GPIO_BYPASS_MODE) & ~BIT(offset),
+		  cgpio->regs + CDNS_GPIO_BYPASS_MODE);
+
+	return 0;
+}
+
+static void cdns_gpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+
+	iowrite32(ioread32(cgpio->regs + CDNS_GPIO_BYPASS_MODE) | BIT(offset),
+		  cgpio->regs + CDNS_GPIO_BYPASS_MODE);
+}
+
+static int cdns_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+	u32 val = ioread32(cgpio->regs + CDNS_GPIO_DIRECTION_MODE);
+
+	return !!(val & BIT(offset));
+}
+
+static int cdns_gpio_direction_in(struct gpio_chip *chip, unsigned int offset)
+{
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+
+	iowrite32(ioread32(cgpio->regs + CDNS_GPIO_DIRECTION_MODE) |
+		  BIT(offset),
+		  cgpio->regs + CDNS_GPIO_DIRECTION_MODE);
+
+	return 0;
+}
+
+static int cdns_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+	u32 val;
+
+	if (ioread32(cgpio->regs + CDNS_GPIO_DIRECTION_MODE) & BIT(offset))
+		val = ioread32(cgpio->regs + CDNS_GPIO_INPUT_VALUE);
+	else
+		val = ioread32(cgpio->regs + CDNS_GPIO_OUTPUT_VALUE);
+
+	return !!(val & BIT(offset));
+}
+
+static void cdns_gpio_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+				   unsigned long *bits)
+{
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+	u32 val = ioread32(cgpio->regs + CDNS_GPIO_OUTPUT_VALUE);
+
+	val &= ~(*mask);
+	val |= (*bits) & (*mask);
+
+	iowrite32(val, cgpio->regs + CDNS_GPIO_OUTPUT_VALUE);
+}
+
+static void cdns_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			  int value)
+{
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+	u32 val = ioread32(cgpio->regs + CDNS_GPIO_OUTPUT_VALUE);
+
+	if (value)
+		val |= BIT(offset);
+	else
+		val &= ~BIT(offset);
+
+	iowrite32(val, cgpio->regs + CDNS_GPIO_OUTPUT_VALUE);
+}
+
+static int cdns_gpio_direction_out(struct gpio_chip *chip, unsigned int offset,
+				   int value)
+{
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+
+	iowrite32(ioread32(cgpio->regs + CDNS_GPIO_DIRECTION_MODE) &
+		  ~BIT(offset),
+		  cgpio->regs + CDNS_GPIO_DIRECTION_MODE);
+	cdns_gpio_set(chip, offset, value);
+	iowrite32(ioread32(cgpio->regs + CDNS_GPIO_OUTPUT_EN) | BIT(offset),
+		  cgpio->regs + CDNS_GPIO_DIRECTION_MODE);
+
+	return 0;
+}
+
+static void cdns_gpio_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+
+	iowrite32(d->hwirq, cgpio->regs + CDNS_GPIO_IRQ_DIS);
+}
+
+static void cdns_gpio_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+
+	iowrite32(BIT(d->hwirq), cgpio->regs + CDNS_GPIO_IRQ_EN);
+}
+
+static int cdns_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
+	u32 int_type = ioread32(cgpio->regs + CDNS_GPIO_IRQ_TYPE);
+	u32 int_value = ioread32(cgpio->regs + CDNS_GPIO_IRQ_VALUE);
+	u32 mask = BIT(d->hwirq);
+
+	int_type &= ~mask;
+	int_value &= ~mask;
+
+	if (type == IRQ_TYPE_LEVEL_HIGH) {
+		int_type |= mask;
+		int_value |= mask;
+	} else if (type == IRQ_TYPE_LEVEL_LOW) {
+		int_type |= mask;
+	} else if (type & IRQ_TYPE_EDGE_BOTH) {
+		u32 any_edge;
+
+		int_type &= ~mask;
+
+		any_edge = ioread32(cgpio->regs + CDNS_GPIO_IRQ_ANY_EDGE);
+		any_edge &= ~mask;
+
+		if (type == IRQ_TYPE_EDGE_BOTH)
+			any_edge |= mask;
+		else if (IRQ_TYPE_EDGE_RISING)
+			int_value |= mask;
+
+		iowrite32(any_edge, cgpio->regs + CDNS_GPIO_IRQ_ANY_EDGE);
+	} else {
+		return -EINVAL;
+	}
+
+	iowrite32(int_type, cgpio->regs + CDNS_GPIO_IRQ_TYPE);
+	iowrite32(int_value, cgpio->regs + CDNS_GPIO_IRQ_VALUE);
+
+	return 0;
+}
+
+static irqreturn_t cdns_gpio_irq_handler(int irq, void *dev)
+{
+	struct cdns_gpio_chip *cgpio = dev;
+	unsigned long status;
+	int hwirq;
+
+	/*
+	 * FIXME: If we have an edge irq that is masked we might lose it
+	 * since reading the STATUS register clears all IRQ flags.
+	 * We could store the status of all masked IRQ in the cdns_gpio_chip
+	 * struct but we then have no way to re-trigger the interrupt when
+	 * it is unmasked.
+	 */
+	status = ioread32(cgpio->regs + CDNS_GPIO_IRQ_STATUS) &
+		 ioread32(cgpio->regs + CDNS_GPIO_IRQ_MASK);
+
+	for_each_set_bit(hwirq, &status, 32) {
+		int irq = irq_find_mapping(cgpio->base.irqdomain, hwirq);
+
+		handle_nested_irq(irq);
+	}
+
+	return status ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int cdns_gpio_probe(struct platform_device *pdev)
+{
+	struct cdns_gpio_chip *cgpio;
+	struct resource *res;
+	int ret, irq;
+
+	cgpio = devm_kzalloc(&pdev->dev, sizeof(*cgpio), GFP_KERNEL);
+	if (!cgpio)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cgpio->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(cgpio->regs))
+		return PTR_ERR(cgpio->regs);
+
+	cgpio->base.label = dev_name(&pdev->dev);
+	cgpio->base.ngpio = 32;
+	cgpio->base.parent = &pdev->dev;
+	cgpio->base.base = -1;
+	cgpio->base.owner = THIS_MODULE;
+	cgpio->base.request = cdns_gpio_request;
+	cgpio->base.free = cdns_gpio_free;
+	cgpio->base.get_direction = cdns_gpio_get_direction;
+	cgpio->base.direction_input = cdns_gpio_direction_in;
+	cgpio->base.get = cdns_gpio_get;
+	cgpio->base.direction_output = cdns_gpio_direction_out;
+	cgpio->base.set = cdns_gpio_set;
+	cgpio->base.set_multiple = cdns_gpio_set_multiple;
+
+	cgpio->pclk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(cgpio->pclk)) {
+		ret = PTR_ERR(cgpio->pclk);
+		dev_err(&pdev->dev,
+			"Failed to retrieve peripheral clock, %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(cgpio->pclk);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Failed to enable the peripheral clock, %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &cgpio->base, cgpio);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+		goto err_disable_clk;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0) {
+		cgpio->irqchip.name = dev_name(&pdev->dev);
+		cgpio->irqchip.irq_mask = cdns_gpio_irq_mask;
+		cgpio->irqchip.irq_unmask = cdns_gpio_irq_unmask;
+		cgpio->irqchip.irq_set_type = cdns_gpio_irq_set_type;
+
+		ret = gpiochip_irqchip_add_nested(&cgpio->base,
+						  &cgpio->irqchip, 0,
+						  handle_simple_irq,
+						  IRQ_TYPE_NONE);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Could not connect irqchip to gpiochip, %d\n",
+				ret);
+			goto err_disable_clk;
+		}
+
+		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+						cdns_gpio_irq_handler,
+						IRQF_ONESHOT,
+						dev_name(&pdev->dev), cgpio);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"Failed to register irq handler, %d\n", ret);
+			goto err_disable_clk;
+		}
+	}
+
+	platform_set_drvdata(pdev, cgpio);
+
+	return 0;
+
+err_disable_clk:
+	clk_disable_unprepare(cgpio->pclk);
+
+	return 0;
+}
+
+static int cdns_gpio_remove(struct platform_device *pdev)
+{
+	struct cdns_gpio_chip *cgpio = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(cgpio->pclk);
+
+	return 0;
+}
+
+static const struct of_device_id cdns_of_ids[] = {
+	{ .compatible = "cdns,gpio-r1p02" },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver cdns_gpio_driver = {
+	.driver = {
+		.name = "cdns-gpio",
+		.of_match_table = cdns_of_ids,
+	},
+	.probe = cdns_gpio_probe,
+	.remove = cdns_gpio_remove,
+};
+module_platform_driver(cdns_gpio_driver);
+
+MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>");
+MODULE_DESCRIPTION("Cadence GPIO driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:cdns-gpio");
-- 
2.7.4

[PATCH 2/2] dt-bindings: gpio: Document Cadence GPIO controller bindings

[Boris Brezillon]

Document Cadence GPIO bindings.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../devicetree/bindings/gpio/cdns,gpio.txt         | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/cdns,gpio.txt

diff --git a/Documentation/devicetree/bindings/gpio/cdns,gpio.txt b/Documentation/devicetree/bindings/gpio/cdns,gpio.txt
new file mode 100644
index 000000000000..09fb2c8e5c9d
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/cdns,gpio.txt
@@ -0,0 +1,41 @@
+Cadence GPIO controller bindings
+
+Required properties:
+- compatible: should be "cdns,gpio-r1p02".
+- reg: the register base address and size.
+- #gpio-cells: should be 2.
+  * first cell is the GPIO number.
+  * second cell specifies the GPIO flags, as defined in
+    <dt-bindings/gpio/gpio.h>. Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
+    flags are supported.
+- gpio-controller: marks the device as a GPIO controller.
+- clocks: should contain one entry referencing the peripheral clock driving the
+  GPIO controller.
+
+Optional properties:
+- interrupt-parent: phandle of the parent interrupt controller.
+- interrupts: interrupt specifier for the controllers interrupt.
+- interrupt-controller: marks the device as an interrupt controller. When
+  defined, interrupts, interrupt-parent and #interrupt-cells are required.
+- interrupt-cells: should be 2.
+  * first cell is the GPIO number you want to use as an IRQ source.
+  * second cell specifies the IRQ type, as defined in
+    <dt-bindings/interrupt-controller/irq.h>.
+
+Example:
+
+	gpio-controller@d050000 {
+		compatible = "cdns,gpio";
+		reg = <0xd050000 0x10000>;
+
+		clocks = <&gpio_pclk>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
-- 
2.7.4

Re: [PATCH 2/2] dt-bindings: gpio: Document Cadence GPIO controller bindings

[Linus Walleij]

On Wed, Mar 29, 2017 at 6:04 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:

> Document Cadence GPIO bindings.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

That is a very terse commit message.

Other than that this looks good, just cutting the DT maintainers
some slack before applying it.

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: Add a driver for Cadence GPIO controller

[Linus Walleij]

On Wed, Mar 29, 2017 at 6:04 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:

> Add a driver for Cadence GPIO controller.

IIUC Cadence do a lot of things. Are there variants of this controller?
Thinking whether it should have several compatible strings, and
whether it needs SoC-specific bindings too.

> Even though this driver is pretty simple, I was not able to use the
> generic GPIO infrastructure because it needs custom ->request()/->free()
> implementation and ->direction_output() requires modifying 2 different
> registers while the generic implementation only modifies the dirin (or
> dirout) register. Other than that, the implementation is rather
> straightforward.
>
> Note that irq support has not been tested.

That's cool, kudos for doing it anyway, I will see if I can spot some
weirdness there.

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

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

It should work else something is wrong with the driver.

> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define CDNS_GPIO_BYPASS_MODE          0x0
> +#define CDNS_GPIO_DIRECTION_MODE       0x4
> +#define CDNS_GPIO_OUTPUT_EN            0x8
> +#define CDNS_GPIO_OUTPUT_VALUE         0xc
> +#define CDNS_GPIO_INPUT_VALUE          0x10
> +#define CDNS_GPIO_IRQ_MASK             0x14
> +#define CDNS_GPIO_IRQ_EN               0x18
> +#define CDNS_GPIO_IRQ_DIS              0x1c
> +#define CDNS_GPIO_IRQ_STATUS           0x20
> +#define CDNS_GPIO_IRQ_TYPE             0x24
> +#define CDNS_GPIO_IRQ_VALUE            0x28
> +#define CDNS_GPIO_IRQ_ANY_EDGE         0x2c

Seems simple.

> +struct cdns_gpio_chip {
> +       struct gpio_chip base;

-EALLYOURBASE;
That is a really confusing name for a GPIO chip. "base" is usually used
to name register base, i.e. what you call "regs".

Can you name it "chip" or "gc" or something?

> +static inline struct cdns_gpio_chip *to_cdns_gpio(struct gpio_chip *chip)
> +{
> +       return container_of(chip, struct cdns_gpio_chip, base);
> +}

Please use gpiochip_get_data(gc); instead at all sites, and use
devm_gpiochip_add() to get the data associated to the chip.

> +static int cdns_gpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
> +
> +       iowrite32(ioread32(cgpio->regs + CDNS_GPIO_BYPASS_MODE) & ~BIT(offset),
> +                 cgpio->regs + CDNS_GPIO_BYPASS_MODE);
> +
> +       return 0;
> +}

Two days ago I was adding exactly the same lines of code to
the Faraday driver (another IP vendor). So to me it seems you are doing
the right thing here. But add some comments as to what is going
on: is this as with the Faraday part: you disable any other
IO connected to the pad?

> +static void cdns_gpio_free(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
> +
> +       iowrite32(ioread32(cgpio->regs + CDNS_GPIO_BYPASS_MODE) | BIT(offset),
> +                 cgpio->regs + CDNS_GPIO_BYPASS_MODE);
> +}

Is it really correct to *force* the bypass mode when free:ing the GPIO?

Isn't it more appropriate to copy this bitmask into a state variable
and restore whatever mode it was in *before* you requested the
GPIO?

> +static int cdns_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +static int cdns_gpio_direction_in(struct gpio_chip *chip, unsigned int offset)
> +static int cdns_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +static void cdns_gpio_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +static void cdns_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +static int cdns_gpio_direction_out(struct gpio_chip *chip, unsigned int offset,

These are so simple. Which is why we have the generic GPIO driver :)

select GPIO_GENERIC in Kconfig

#

then in your dynamic gpiochip something like

        ret = bgpio_init(&g->gc, dev, 4,
                         g->base + GPIO_DATA_IN,
                         g->base + GPIO_DATA_SET,
                         g->base + GPIO_DATA_CLR,
                         g->base + GPIO_DIR,
                         NULL,
                         0);

There are flags and stuff for how  the bits get set and cleared
on various variants, check it out. It all comes in through
<linux/gpio/driver.h>.

You can still assign the .request and .free callbacks and
the irqchip after this, that is fine.
See drivers/gpio/gpio-gemini.c for my example

> +static void cdns_gpio_irq_mask(struct irq_data *d)
> +static void cdns_gpio_irq_unmask(struct irq_data *d)

All good.

> +static int cdns_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +       struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
> +       u32 int_type = ioread32(cgpio->regs + CDNS_GPIO_IRQ_TYPE);
> +       u32 int_value = ioread32(cgpio->regs + CDNS_GPIO_IRQ_VALUE);
> +       u32 mask = BIT(d->hwirq);
> +
> +       int_type &= ~mask;
> +       int_value &= ~mask;
> +
> +       if (type == IRQ_TYPE_LEVEL_HIGH) {
> +               int_type |= mask;
> +               int_value |= mask;
> +       } else if (type == IRQ_TYPE_LEVEL_LOW) {
> +               int_type |= mask;
> +       } else if (type & IRQ_TYPE_EDGE_BOTH) {
> +               u32 any_edge;
> +
> +               int_type &= ~mask;
> +
> +               any_edge = ioread32(cgpio->regs + CDNS_GPIO_IRQ_ANY_EDGE);
> +               any_edge &= ~mask;
> +
> +               if (type == IRQ_TYPE_EDGE_BOTH)
> +                       any_edge |= mask;
> +               else if (IRQ_TYPE_EDGE_RISING)
> +                       int_value |= mask;
> +
> +               iowrite32(any_edge, cgpio->regs + CDNS_GPIO_IRQ_ANY_EDGE);
> +       } else {
> +               return -EINVAL;
> +       }
> +
> +       iowrite32(int_type, cgpio->regs + CDNS_GPIO_IRQ_TYPE);
> +       iowrite32(int_value, cgpio->regs + CDNS_GPIO_IRQ_VALUE);
> +
> +       return 0;
> +}

I see that Cadence don't have a separare ACK register and instead
all IRQs get cleared when reading the status register. Not good,
so no separate edge and level handling. What were they thinking...
well not much to do about that.

> +static irqreturn_t cdns_gpio_irq_handler(int irq, void *dev)
> +{
> +       struct cdns_gpio_chip *cgpio = dev;
> +       unsigned long status;
> +       int hwirq;
> +
> +       /*
> +        * FIXME: If we have an edge irq that is masked we might lose it
> +        * since reading the STATUS register clears all IRQ flags.
> +        * We could store the status of all masked IRQ in the cdns_gpio_chip
> +        * struct but we then have no way to re-trigger the interrupt when
> +        * it is unmasked.
> +        */

It is marked FIXME but do you think it can even be fixed? It seems
like a hardware flaw. :(

Controllers that handle this properly have a separate ACK register,
usually.

> +       status = ioread32(cgpio->regs + CDNS_GPIO_IRQ_STATUS) &
> +                ioread32(cgpio->regs + CDNS_GPIO_IRQ_MASK);
> +
> +       for_each_set_bit(hwirq, &status, 32) {
> +               int irq = irq_find_mapping(cgpio->base.irqdomain, hwirq);
> +
> +               handle_nested_irq(irq);

I don't understand this nested business. Why are you making this
a nested IRQ when it seems to be just doing register writes into
IO space?

Why not use a chained interrupt handler?

Again look at the Gemini driver or pl061 for an example.

> +static int cdns_gpio_probe(struct platform_device *pdev)
> +{
> +       struct cdns_gpio_chip *cgpio;
> +       struct resource *res;
> +       int ret, irq;
> +
> +       cgpio = devm_kzalloc(&pdev->dev, sizeof(*cgpio), GFP_KERNEL);
> +       if (!cgpio)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       cgpio->regs = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(cgpio->regs))
> +               return PTR_ERR(cgpio->regs);
> +
> +       cgpio->base.label = dev_name(&pdev->dev);
> +       cgpio->base.ngpio = 32;
> +       cgpio->base.parent = &pdev->dev;
> +       cgpio->base.base = -1;
> +       cgpio->base.owner = THIS_MODULE;
> +       cgpio->base.request = cdns_gpio_request;
> +       cgpio->base.free = cdns_gpio_free;
> +       cgpio->base.get_direction = cdns_gpio_get_direction;
> +       cgpio->base.direction_input = cdns_gpio_direction_in;
> +       cgpio->base.get = cdns_gpio_get;
> +       cgpio->base.direction_output = cdns_gpio_direction_out;
> +       cgpio->base.set = cdns_gpio_set;
> +       cgpio->base.set_multiple = cdns_gpio_set_multiple;

So a bunch of these could be handled with generic GPIO so
we can focus on the meat.

> +       ret = devm_gpiochip_add_data(&pdev->dev, &cgpio->base, cgpio);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
> +               goto err_disable_clk;
> +       }

Hey you add the data, but don't get it with gpiochip_get_data().
Use the accessor, it's nice.

> +       irq = platform_get_irq(pdev, 0);
> +       if (irq >= 0) {
> +               cgpio->irqchip.name = dev_name(&pdev->dev);
> +               cgpio->irqchip.irq_mask = cdns_gpio_irq_mask;
> +               cgpio->irqchip.irq_unmask = cdns_gpio_irq_unmask;
> +               cgpio->irqchip.irq_set_type = cdns_gpio_irq_set_type;
> +
> +               ret = gpiochip_irqchip_add_nested(&cgpio->base,
> +                                                 &cgpio->irqchip, 0,
> +                                                 handle_simple_irq,
> +                                                 IRQ_TYPE_NONE);
> +               if (ret) {
> +                       dev_err(&pdev->dev,
> +                               "Could not connect irqchip to gpiochip, %d\n",
> +                               ret);
> +                       goto err_disable_clk;
> +               }
> +
> +               ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +                                               cdns_gpio_irq_handler,
> +                                               IRQF_ONESHOT,
> +                                               dev_name(&pdev->dev), cgpio);
> +               if (ret < 0) {
> +                       dev_err(&pdev->dev,
> +                               "Failed to register irq handler, %d\n", ret);
> +                       goto err_disable_clk;
> +               }
> +       }
>
And as mentioned I don't get this nested IRQ business.

Have you tried to use a chained interrupt? Like gpio-pl061 does?
You can just copy/paste and try it. Don't miss the chained_irq_enter()
and friends.

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: Add a driver for Cadence GPIO controller

[Boris Brezillon]

Hi Linus,

On Thu, 30 Mar 2017 11:03:45 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Wed, Mar 29, 2017 at 6:04 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> 
> > Add a driver for Cadence GPIO controller.  
> 
> IIUC Cadence do a lot of things. Are there variants of this controller?

I'll let Simon answer that one.

> Thinking whether it should have several compatible strings, and
> whether it needs SoC-specific bindings too.
> 
> > Even though this driver is pretty simple, I was not able to use the
> > generic GPIO infrastructure because it needs custom ->request()/->free()
> > implementation and ->direction_output() requires modifying 2 different
> > registers while the generic implementation only modifies the dirin (or
> > dirout) register. Other than that, the implementation is rather
> > straightforward.
> >
> > Note that irq support has not been tested.  
> 
> That's cool, kudos for doing it anyway, I will see if I can spot some
> weirdness there.
> 
> > +#include <linux/clk.h>
> > +#include <linux/gpio.h>  
> 
> Just
> #include <linux/gpio/driver.h>
> 
> It should work else something is wrong with the driver.

I'll try that.

> 
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#define CDNS_GPIO_BYPASS_MODE          0x0
> > +#define CDNS_GPIO_DIRECTION_MODE       0x4
> > +#define CDNS_GPIO_OUTPUT_EN            0x8
> > +#define CDNS_GPIO_OUTPUT_VALUE         0xc
> > +#define CDNS_GPIO_INPUT_VALUE          0x10
> > +#define CDNS_GPIO_IRQ_MASK             0x14
> > +#define CDNS_GPIO_IRQ_EN               0x18
> > +#define CDNS_GPIO_IRQ_DIS              0x1c
> > +#define CDNS_GPIO_IRQ_STATUS           0x20
> > +#define CDNS_GPIO_IRQ_TYPE             0x24
> > +#define CDNS_GPIO_IRQ_VALUE            0x28
> > +#define CDNS_GPIO_IRQ_ANY_EDGE         0x2c  
> 
> Seems simple.
> 
> > +struct cdns_gpio_chip {
> > +       struct gpio_chip base;  
> 
> -EALLYOURBASE;
> That is a really confusing name for a GPIO chip. "base" is usually used
> to name register base, i.e. what you call "regs".

I usually name a field base to show the inheritance, and regs for the
IP registers.

> 
> Can you name it "chip" or "gc" or something?

Sure, gc is fine.

> 
> > +static inline struct cdns_gpio_chip *to_cdns_gpio(struct gpio_chip *chip)
> > +{
> > +       return container_of(chip, struct cdns_gpio_chip, base);
> > +}  
> 
> Please use gpiochip_get_data(gc); instead at all sites, and use
> devm_gpiochip_add() to get the data associated to the chip.

Okay.

> 
> > +static int cdns_gpio_request(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
> > +
> > +       iowrite32(ioread32(cgpio->regs + CDNS_GPIO_BYPASS_MODE) & ~BIT(offset),
> > +                 cgpio->regs + CDNS_GPIO_BYPASS_MODE);
> > +
> > +       return 0;
> > +}  
> 
> Two days ago I was adding exactly the same lines of code to
> the Faraday driver (another IP vendor). So to me it seems you are doing
> the right thing here. But add some comments as to what is going
> on: is this as with the Faraday part: you disable any other
> IO connected to the pad?

AFAIU, bypass mode is here to let peripheral control the pins directly.
I guess what's behind the GPIO controller depends on the SoC, and most
of the time you'll have a pin controller to allow advanced pin-muxing
operations. 

> 
> > +static void cdns_gpio_free(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
> > +
> > +       iowrite32(ioread32(cgpio->regs + CDNS_GPIO_BYPASS_MODE) | BIT(offset),
> > +                 cgpio->regs + CDNS_GPIO_BYPASS_MODE);
> > +}  
> 
> Is it really correct to *force* the bypass mode when free:ing the GPIO?

I don't know. Again, it's likely to be SoC-specific.

> 
> Isn't it more appropriate to copy this bitmask into a state variable
> and restore whatever mode it was in *before* you requested the
> GPIO?

Yep, maybe. I can store the status at probe time and restore it when
->free() is called.

> 
> > +static int cdns_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> > +static int cdns_gpio_direction_in(struct gpio_chip *chip, unsigned int offset)
> > +static int cdns_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > +static void cdns_gpio_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +static void cdns_gpio_set(struct gpio_chip *chip, unsigned int offset,
> > +static int cdns_gpio_direction_out(struct gpio_chip *chip, unsigned int offset,  
> 
> These are so simple. Which is why we have the generic GPIO driver :)
> 
> select GPIO_GENERIC in Kconfig
> 
> #
> 
> then in your dynamic gpiochip something like
> 
>         ret = bgpio_init(&g->gc, dev, 4,
>                          g->base + GPIO_DATA_IN,
>                          g->base + GPIO_DATA_SET,
>                          g->base + GPIO_DATA_CLR,
>                          g->base + GPIO_DIR,
>                          NULL,
>                          0);

Actually, for ->direction_output() it's not working (see the
implementation: we need to change both CDNS_GPIO_DIRECTION_MODE and
CDNS_GPIO_OUTPUT_EN to enable the output mode).

I can modify generic GPIO code to handle this case, but I thought my
case was specific enough to not complexify the generic code with a case
that is unlikely to be re-used elsewhere. Maybe I was wrong.

> 
> There are flags and stuff for how  the bits get set and cleared
> on various variants, check it out. It all comes in through
> <linux/gpio/driver.h>.
> 
> You can still assign the .request and .free callbacks and
> the irqchip after this, that is fine.
> See drivers/gpio/gpio-gemini.c for my example

As said above, the generic ->direction_output() implementation does not
match the Cadence controller logic. It's almost possible to make it fit,
but it requires extending gpio-generic.c.

> 
> > +static void cdns_gpio_irq_mask(struct irq_data *d)
> > +static void cdns_gpio_irq_unmask(struct irq_data *d)  
> 
> All good.
> 
> > +static int cdns_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> > +       struct cdns_gpio_chip *cgpio = to_cdns_gpio(chip);
> > +       u32 int_type = ioread32(cgpio->regs + CDNS_GPIO_IRQ_TYPE);
> > +       u32 int_value = ioread32(cgpio->regs + CDNS_GPIO_IRQ_VALUE);
> > +       u32 mask = BIT(d->hwirq);
> > +
> > +       int_type &= ~mask;
> > +       int_value &= ~mask;
> > +
> > +       if (type == IRQ_TYPE_LEVEL_HIGH) {
> > +               int_type |= mask;
> > +               int_value |= mask;
> > +       } else if (type == IRQ_TYPE_LEVEL_LOW) {
> > +               int_type |= mask;
> > +       } else if (type & IRQ_TYPE_EDGE_BOTH) {
> > +               u32 any_edge;
> > +
> > +               int_type &= ~mask;
> > +
> > +               any_edge = ioread32(cgpio->regs + CDNS_GPIO_IRQ_ANY_EDGE);
> > +               any_edge &= ~mask;
> > +
> > +               if (type == IRQ_TYPE_EDGE_BOTH)
> > +                       any_edge |= mask;
> > +               else if (IRQ_TYPE_EDGE_RISING)
> > +                       int_value |= mask;
> > +
> > +               iowrite32(any_edge, cgpio->regs + CDNS_GPIO_IRQ_ANY_EDGE);
> > +       } else {
> > +               return -EINVAL;
> > +       }
> > +
> > +       iowrite32(int_type, cgpio->regs + CDNS_GPIO_IRQ_TYPE);
> > +       iowrite32(int_value, cgpio->regs + CDNS_GPIO_IRQ_VALUE);
> > +
> > +       return 0;
> > +}  
> 
> I see that Cadence don't have a separare ACK register and instead
> all IRQs get cleared when reading the status register. Not good,
> so no separate edge and level handling. What were they thinking...
> well not much to do about that.

Yep, unfortunately :-(.

> 
> > +static irqreturn_t cdns_gpio_irq_handler(int irq, void *dev)
> > +{
> > +       struct cdns_gpio_chip *cgpio = dev;
> > +       unsigned long status;
> > +       int hwirq;
> > +
> > +       /*
> > +        * FIXME: If we have an edge irq that is masked we might lose it
> > +        * since reading the STATUS register clears all IRQ flags.
> > +        * We could store the status of all masked IRQ in the cdns_gpio_chip
> > +        * struct but we then have no way to re-trigger the interrupt when
> > +        * it is unmasked.
> > +        */  
> 
> It is marked FIXME but do you think it can even be fixed? It seems
> like a hardware flaw. :(

Maybe not. Unless Simon comes up with a magic register to re-trigger an
interrupt :-).

> 
> Controllers that handle this properly have a separate ACK register,
> usually.

Yes.

> 
> > +       status = ioread32(cgpio->regs + CDNS_GPIO_IRQ_STATUS) &
> > +                ioread32(cgpio->regs + CDNS_GPIO_IRQ_MASK);
> > +
> > +       for_each_set_bit(hwirq, &status, 32) {
> > +               int irq = irq_find_mapping(cgpio->base.irqdomain, hwirq);
> > +
> > +               handle_nested_irq(irq);  
> 
> I don't understand this nested business. Why are you making this
> a nested IRQ when it seems to be just doing register writes into
> IO space?
> 
> Why not use a chained interrupt handler?

Indeed.

> 
> Again look at the Gemini driver or pl061 for an example.
> 
> > +static int cdns_gpio_probe(struct platform_device *pdev)
> > +{
> > +       struct cdns_gpio_chip *cgpio;
> > +       struct resource *res;
> > +       int ret, irq;
> > +
> > +       cgpio = devm_kzalloc(&pdev->dev, sizeof(*cgpio), GFP_KERNEL);
> > +       if (!cgpio)
> > +               return -ENOMEM;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       cgpio->regs = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(cgpio->regs))
> > +               return PTR_ERR(cgpio->regs);
> > +
> > +       cgpio->base.label = dev_name(&pdev->dev);
> > +       cgpio->base.ngpio = 32;
> > +       cgpio->base.parent = &pdev->dev;
> > +       cgpio->base.base = -1;
> > +       cgpio->base.owner = THIS_MODULE;
> > +       cgpio->base.request = cdns_gpio_request;
> > +       cgpio->base.free = cdns_gpio_free;
> > +       cgpio->base.get_direction = cdns_gpio_get_direction;
> > +       cgpio->base.direction_input = cdns_gpio_direction_in;
> > +       cgpio->base.get = cdns_gpio_get;
> > +       cgpio->base.direction_output = cdns_gpio_direction_out;
> > +       cgpio->base.set = cdns_gpio_set;
> > +       cgpio->base.set_multiple = cdns_gpio_set_multiple;  
> 
> So a bunch of these could be handled with generic GPIO so
> we can focus on the meat.

Could be, if we modify gpio-mmio.c to support my case. I have the
feeling that you'd prefer this solution, so I'll try to do that in my
v2 ;-).

Another solution would be to write 0xffffffff into CDNS_GPIO_OUTPUT_EN
at probe time so that each time CDNS_GPIO_DIRECTION_MODE is modified to
set a pin in output mode, the CDNS_GPIO_OUTPUT_EN is already correctly
configured.
Simon, would that work? Is there a good reason to keep a bit in
CDNS_GPIO_OUTPUT_EN set to 0 when the GPIO is in input mode (power
consumption?)?

> 
> > +       ret = devm_gpiochip_add_data(&pdev->dev, &cgpio->base, cgpio);
> > +       if (ret < 0) {
> > +               dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
> > +               goto err_disable_clk;
> > +       }  
> 
> Hey you add the data, but don't get it with gpiochip_get_data().
> Use the accessor, it's nice.

Sure. I'm so used to the container_of() trick that I sometime forget to
look at how the subsystem maintainer prefers to do it.

> 
> > +       irq = platform_get_irq(pdev, 0);
> > +       if (irq >= 0) {
> > +               cgpio->irqchip.name = dev_name(&pdev->dev);
> > +               cgpio->irqchip.irq_mask = cdns_gpio_irq_mask;
> > +               cgpio->irqchip.irq_unmask = cdns_gpio_irq_unmask;
> > +               cgpio->irqchip.irq_set_type = cdns_gpio_irq_set_type;
> > +
> > +               ret = gpiochip_irqchip_add_nested(&cgpio->base,
> > +                                                 &cgpio->irqchip, 0,
> > +                                                 handle_simple_irq,
> > +                                                 IRQ_TYPE_NONE);
> > +               if (ret) {
> > +                       dev_err(&pdev->dev,
> > +                               "Could not connect irqchip to gpiochip, %d\n",
> > +                               ret);
> > +                       goto err_disable_clk;
> > +               }
> > +
> > +               ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > +                                               cdns_gpio_irq_handler,
> > +                                               IRQF_ONESHOT,
> > +                                               dev_name(&pdev->dev), cgpio);
> > +               if (ret < 0) {
> > +                       dev_err(&pdev->dev,
> > +                               "Failed to register irq handler, %d\n", ret);
> > +                       goto err_disable_clk;
> > +               }
> > +       }
> >  
> And as mentioned I don't get this nested IRQ business.
> 
> Have you tried to use a chained interrupt? Like gpio-pl061 does?
> You can just copy/paste and try it. Don't miss the chained_irq_enter()
> and friends.

Well, yes. I'm always unsure when a nested IRQ should be used compared
to an irqchain (other drivers in the gpio subsystem are using the
nested IRQ approach, just grep for handle_nested_irq in drivers/gpio).
Maybe it has to be done this way when you use a threaded interrupt. I
looked at that a while ago, but I don't remember the differences and
when one should be used instead of the other.

Thanks for the review.

Boris

Re: [PATCH 1/2] gpio: Add a driver for Cadence GPIO controller

[Linus Walleij]

On Thu, Mar 30, 2017 at 1:29 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Linus Walleij <linus.walleij@linaro.org> wrote:

>> then in your dynamic gpiochip something like
>>
>>         ret = bgpio_init(&g->gc, dev, 4,
>>                          g->base + GPIO_DATA_IN,
>>                          g->base + GPIO_DATA_SET,
>>                          g->base + GPIO_DATA_CLR,
>>                          g->base + GPIO_DIR,
>>                          NULL,
>>                          0);
>
> Actually, for ->direction_output() it's not working (see the
> implementation: we need to change both CDNS_GPIO_DIRECTION_MODE and
> CDNS_GPIO_OUTPUT_EN to enable the output mode).
>
> I can modify generic GPIO code to handle this case, but I thought my
> case was specific enough to not complexify the generic code with a case
> that is unlikely to be re-used elsewhere. Maybe I was wrong.

You can also just override this one function after initializing
with bgpio_init? I don't know if it's OK to pass NULL
for the direction register.

Whatever becomes most elegant I guess?

>> You can still assign the .request and .free callbacks and
>> the irqchip after this, that is fine.
>> See drivers/gpio/gpio-gemini.c for my example
>
> As said above, the generic ->direction_output() implementation does not
> match the Cadence controller logic. It's almost possible to make it fit,
> but it requires extending gpio-generic.c.

OK I don't know if it's worth it. Make an educated decision,
I will not nix the patch if you keep with the custom implementation
for all functions.

> Could be, if we modify gpio-mmio.c to support my case. I have the
> feeling that you'd prefer this solution, so I'll try to do that in my
> v2 ;-).

If you think it just looks butt ugly then don't do it.

> Another solution would be to write 0xffffffff into CDNS_GPIO_OUTPUT_EN
> at probe time so that each time CDNS_GPIO_DIRECTION_MODE is modified to
> set a pin in output mode, the CDNS_GPIO_OUTPUT_EN is already correctly
> configured.
> Simon, would that work? Is there a good reason to keep a bit in
> CDNS_GPIO_OUTPUT_EN set to 0 when the GPIO is in input mode (power
> consumption?)?

Hm. Let's see.

> Sure. I'm so used to the container_of() trick that I sometime forget to
> look at how the subsystem maintainer prefers to do it.

I usually prefer it to use the associated data, but some still prefer
to use container_of() for type safety.

>> Have you tried to use a chained interrupt? Like gpio-pl061 does?
>> You can just copy/paste and try it. Don't miss the chained_irq_enter()
>> and friends.
>
> Well, yes. I'm always unsure when a nested IRQ should be used compared
> to an irqchain (other drivers in the gpio subsystem are using the
> nested IRQ approach, just grep for handle_nested_irq in drivers/gpio).
> Maybe it has to be done this way when you use a threaded interrupt. I
> looked at that a while ago, but I don't remember the differences and
> when one should be used instead of the other.

To me chained handlers are for fast things than can be handled entirely in
interrupt context, traversing the chain immediately when the IRQ
fires. Like this driver.

Nested threads are ideal for say GPIO expanders on I2S or SPI, where
you have to do a bunch of process context business to handle the
interrupt.

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: Add a driver for Cadence GPIO controller

[Simon Hatliff]

Hi Boris, Linus,

On 30/03/17 12:29, Boris Brezillon wrote:
> Hi Linus,
>
> On Thu, 30 Mar 2017 11:03:45 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> On Wed, Mar 29, 2017 at 6:04 PM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>>
>>> Add a driver for Cadence GPIO controller.
>> IIUC Cadence do a lot of things. Are there variants of this controller?
> I'll let Simon answer that one.
This is a controller that has been around since 2000.  It is not 
configurable in any way, so there are no variants.  Cadence do not offer 
any other GPIO controllers as IP.
>>> +static irqreturn_t cdns_gpio_irq_handler(int irq, void *dev)
>>> +{
>>> +       struct cdns_gpio_chip *cgpio = dev;
>>> +       unsigned long status;
>>> +       int hwirq;
>>> +
>>> +       /*
>>> +        * FIXME: If we have an edge irq that is masked we might lose it
>>> +        * since reading the STATUS register clears all IRQ flags.
>>> +        * We could store the status of all masked IRQ in the cdns_gpio_chip
>>> +        * struct but we then have no way to re-trigger the interrupt when
>>> +        * it is unmasked.
>>> +        */
>> It is marked FIXME but do you think it can even be fixed? It seems
>> like a hardware flaw. :(
> Maybe not. Unless Simon comes up with a magic register to re-trigger an
> interrupt :-).
There are no plans to update the controller.
> Another solution would be to write 0xffffffff into CDNS_GPIO_OUTPUT_EN
> at probe time so that each time CDNS_GPIO_DIRECTION_MODE is modified to
> set a pin in output mode, the CDNS_GPIO_OUTPUT_EN is already correctly
> configured.
> Simon, would that work? Is there a good reason to keep a bit in
> CDNS_GPIO_OUTPUT_EN set to 0 when the GPIO is in input mode (power
> consumption?)?
If direction_mode is set to input then output_en is ignored so this 
should work.  The hardware defaults to output mode, so as long as you 
set all pins to input mode before you set all output_en bits there 
should be no negative effect.

Cheers,
-- Simon

Re: [PATCH 1/2] gpio: Add a driver for Cadence GPIO controller

[Boris Brezillon]

On Thu, 30 Mar 2017 18:26:01 +0100
Simon Hatliff <hatliff@cadence.com> wrote:


> > Another solution would be to write 0xffffffff into CDNS_GPIO_OUTPUT_EN
> > at probe time so that each time CDNS_GPIO_DIRECTION_MODE is modified to
> > set a pin in output mode, the CDNS_GPIO_OUTPUT_EN is already correctly
> > configured.
> > Simon, would that work? Is there a good reason to keep a bit in
> > CDNS_GPIO_OUTPUT_EN set to 0 when the GPIO is in input mode (power
> > consumption?)?  
> If direction_mode is set to input then output_en is ignored so this 
> should work.  The hardware defaults to output mode, so as long as you 
> set all pins to input mode before you set all output_en bits there 
> should be no negative effect.

Okay, I'll try something like that, except I'll probably keep already
output-enabled in their existing state to avoid modifying bootloader's
GPIO settings.

Thanks for the feedback.

Boris

Re: [PATCH 1/2] gpio: Add a driver for Cadence GPIO controller

[Boris Brezillon]

On Thu, 30 Mar 2017 18:26:01 +0100
Simon Hatliff <hatliff@cadence.com> wrote:

> Hi Boris, Linus,
> 
> On 30/03/17 12:29, Boris Brezillon wrote:
> > Hi Linus,
> >
> > On Thu, 30 Mar 2017 11:03:45 +0200
> > Linus Walleij <linus.walleij@linaro.org> wrote:
> >  
> >> On Wed, Mar 29, 2017 at 6:04 PM, Boris Brezillon
> >> <boris.brezillon@free-electrons.com> wrote:
> >>  
> >>> Add a driver for Cadence GPIO controller.  
> >> IIUC Cadence do a lot of things. Are there variants of this controller?  
> > I'll let Simon answer that one.  
> This is a controller that has been around since 2000.  It is not 
> configurable in any way, so there are no variants.  Cadence do not offer 
> any other GPIO controllers as IP.
> >>> +static irqreturn_t cdns_gpio_irq_handler(int irq, void *dev)
> >>> +{
> >>> +       struct cdns_gpio_chip *cgpio = dev;
> >>> +       unsigned long status;
> >>> +       int hwirq;
> >>> +
> >>> +       /*
> >>> +        * FIXME: If we have an edge irq that is masked we might lose it
> >>> +        * since reading the STATUS register clears all IRQ flags.
> >>> +        * We could store the status of all masked IRQ in the cdns_gpio_chip
> >>> +        * struct but we then have no way to re-trigger the interrupt when
> >>> +        * it is unmasked.
> >>> +        */  
> >> It is marked FIXME but do you think it can even be fixed? It seems
> >> like a hardware flaw. :(  
> > Maybe not. Unless Simon comes up with a magic register to re-trigger an
> > interrupt :-).  
> There are no plans to update the controller.

Linus, what should I do here? Drop support for edge interrupts?

Re: [PATCH 1/2] gpio: Add a driver for Cadence GPIO controller

[Linus Walleij]

On Mon, Apr 10, 2017 at 3:26 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:

> Linus, what should I do here? Drop support for edge interrupts?

Do whatever makes most sense for your usecases.
I certainly allow dirty hacks if it is to be able to use the hardware
at all.

Yours,
Linus Walleij