[PATCH v2 0/3] GPIO: Add driver for ThunderX and OCTEON-TX SoCs

[PATCH v2 0/3] GPIO: Add driver for ThunderX and OCTEON-TX SoCs

[David Daney]

From: David Daney <david.daney@cavium.com>

The Cavium ThunderX and OCTEON-TX family of SoCs have on-chip GPIO
lines.  This patch set adds a driver for these.

Changes from v1:

 - in 1/3: Addressed Rob Harring's comments.

 - in 2/3: Trivial cleanups found in internal review + add some
   comments.

David Daney (3):
  dt-bindings: gpio: Add binding documentation for gpio-thunderx
  gpio: Add gpio driver support for ThunderX and OCTEON-TX
  MAINTAINERS: Add entry for THUNDERX GPIO Driver.

 .../devicetree/bindings/gpio/gpio-thunderx.txt     |  27 ++
 MAINTAINERS                                        |   5 +
 drivers/gpio/Kconfig                               |   8 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-thunderx.c                       | 487 +++++++++++++++++++++
 5 files changed, 528 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
 create mode 100644 drivers/gpio/gpio-thunderx.c

-- 
1.8.3.1

[PATCH v2 1/3] dt-bindings: gpio: Add binding documentation for gpio-thunderx

[David Daney]

From: David Daney <david.daney@cavium.com>

Signed-off-by: David Daney <david.daney@cavium.com>
---
 .../devicetree/bindings/gpio/gpio-thunderx.txt     | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-thunderx.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt b/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
new file mode 100644
index 0000000..3f883ae
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
@@ -0,0 +1,27 @@
+Cavium ThunderX/OCTEON-TX GPIO controller bindings
+
+Required Properties:
+- reg: The controller bus address.
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Must be 2.
+  - First cell is the GPIO pin number relative to the controller.
+  - Second cell is a standard generic flag bitfield as described in gpio.txt.
+
+Optional Properties:
+- compatible: "cavium,thunder-8890-gpio", unused as PCI driver binding is used.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Must be present and have value of 2 if
+                    "interrupt-controller" is present.
+  - First cell is the GPIO pin number relative to the controller.
+  - Second cell is triggering flags as defined in interrupts.txt.
+
+Example:
+
+gpio_6_0: gpio@6,0 {
+	compatible = "cavium,thunder-8890-gpio";
+	reg = <0x3000 0 0 0 0>; /*  DEVFN = 0x30 (6:0) */
+	gpio-controller;
+	#gpio-cells = <2>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+};
-- 
1.8.3.1

Re: [PATCH v2 1/3] dt-bindings: gpio: Add binding documentation for gpio-thunderx

[Linus Walleij]

On Sat, Jan 7, 2017 at 12:22 AM, David Daney <ddaney.cavm@gmail.com> wrote:

> From: David Daney <david.daney@cavium.com>
>
> Signed-off-by: David Daney <david.daney@cavium.com>
(...)

> +Optional Properties:
> +- compatible: "cavium,thunder-8890-gpio", unused as PCI driver binding is used.
> +- interrupt-controller: Marks the device node as an interrupt controller.
> +- #interrupt-cells: Must be present and have value of 2 if
> +                    "interrupt-controller" is present.
> +  - First cell is the GPIO pin number relative to the controller.
> +  - Second cell is triggering flags as defined in interrupts.txt.

AFAICT this device has an optional list of interrupts as well?
One per pin even?

Yours,
Linus Walleij

Re: [PATCH v2 1/3] dt-bindings: gpio: Add binding documentation for gpio-thunderx

[David Daney]

On 01/09/2017 11:36 AM, Linus Walleij wrote:
> On Sat, Jan 7, 2017 at 12:22 AM, David Daney <ddaney.cavm@gmail.com> wrote:
>
>> From: David Daney <david.daney@cavium.com>
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
> (...)
>
>> +Optional Properties:
>> +- compatible: "cavium,thunder-8890-gpio", unused as PCI driver binding is used.
>> +- interrupt-controller: Marks the device node as an interrupt controller.
>> +- #interrupt-cells: Must be present and have value of 2 if
>> +                    "interrupt-controller" is present.
>> +  - First cell is the GPIO pin number relative to the controller.
>> +  - Second cell is triggering flags as defined in interrupts.txt.
>
> AFAICT this device has an optional list of interrupts as well?
> One per pin even?

I'm not sure I understand your question.

The GPIO hardware supports an interrupt on each pin.  The underlying 
interrupt mechanism is via PCI MSI-X, which are fully discoverable by 
the driver, so lack of device tree binding for the these underlying 
MSI-X is fully appropriate.  On the other hand, users of the GPIO 
interrupt pins need this "interrupt-controller" and "#interrupt-cells" 
to be able to properly find and configure the proper interrupts.

I said the "interrupt-controller" property was optional, because some 
systems don't use GPIO interrupts and can function without specifying 
that it is also an interrupt controller.


>
> Yours,
> Linus Walleij
>

Re: [PATCH v2 1/3] dt-bindings: gpio: Add binding documentation for gpio-thunderx

[Linus Walleij]

On Mon, Jan 9, 2017 at 8:44 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 01/09/2017 11:36 AM, Linus Walleij wrote:

>>> +Optional Properties:
>>> +- compatible: "cavium,thunder-8890-gpio", unused as PCI driver binding
>>> is used.
>>> +- interrupt-controller: Marks the device node as an interrupt
>>> controller.
>>> +- #interrupt-cells: Must be present and have value of 2 if
>>> +                    "interrupt-controller" is present.
>>> +  - First cell is the GPIO pin number relative to the controller.
>>> +  - Second cell is triggering flags as defined in interrupts.txt.
>>
>>
>> AFAICT this device has an optional list of interrupts as well?
>> One per pin even?
>
> I'm not sure I understand your question.
>
> The GPIO hardware supports an interrupt on each pin.  The underlying
> interrupt mechanism is via PCI MSI-X, which are fully discoverable by the
> driver, so lack of device tree binding for the these underlying MSI-X is
> fully appropriate.

Sorry I guess I'm just ignorant about how PCI works, that has never
been my strongest subject admittedly.

So what you're saying is that PCI devices do not need specifying
interrupts not interrupt parents in the device tree?

That's fine then.

Yours,
Linus Walleij

Re: [PATCH v2 1/3] dt-bindings: gpio: Add binding documentation for gpio-thunderx

[Rob Herring]

On Fri, Jan 06, 2017 at 03:22:58PM -0800, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  .../devicetree/bindings/gpio/gpio-thunderx.txt     | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-thunderx.txt

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v2 2/3] gpio: Add gpio driver support for ThunderX and OCTEON-TX

[David Daney]

From: David Daney <david.daney@cavium.com>

Cavium ThunderX and OCTEON-TX are arm64 based SoCs.  Add driver for
the on-chip GPIO pins.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/gpio/Kconfig         |   8 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-thunderx.c | 487 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 496 insertions(+)
 create mode 100644 drivers/gpio/gpio-thunderx.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d5d3654..e691a5e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -421,6 +421,14 @@ config GPIO_TS4800
 	help
 	  This driver support TS-4800 FPGA GPIO controllers.
 
+config GPIO_THUNDERX
+	tristate "Cavium ThunderX/OCTEON-TX GPIO"
+	depends on ARCH_THUNDER || (64BIT && COMPILE_TEST)
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support the on-chip GPIO lines on the ThunderX
+	  and OCTEON-TX families of SoCs.
+
 config GPIO_TZ1090
 	bool "Toumaz Xenif TZ1090 GPIO support"
 	depends on SOC_TZ1090
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a7676b8..c62bc72 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -108,6 +108,7 @@ obj-$(CONFIG_GPIO_SYSCON)	+= gpio-syscon.o
 obj-$(CONFIG_GPIO_TB10X)	+= gpio-tb10x.o
 obj-$(CONFIG_GPIO_TC3589X)	+= gpio-tc3589x.o
 obj-$(CONFIG_GPIO_TEGRA)	+= gpio-tegra.o
+obj-$(CONFIG_GPIO_THUNDERX)	+= gpio-thunderx.o
 obj-$(CONFIG_GPIO_TIMBERDALE)	+= gpio-timberdale.o
 obj-$(CONFIG_GPIO_PALMAS)	+= gpio-palmas.o
 obj-$(CONFIG_GPIO_TPIC2810)	+= gpio-tpic2810.o
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
new file mode 100644
index 0000000..089d8d4
--- /dev/null
+++ b/drivers/gpio/gpio-thunderx.c
@@ -0,0 +1,487 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2016, 2017 Cavium Inc.
+ */
+
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/spinlock.h>
+
+
+#define GPIO_RX_DAT	0x0
+#define GPIO_TX_SET	0x8
+#define GPIO_TX_CLR	0x10
+#define GPIO_CONST	0x90
+#define  GPIO_CONST_GPIOS_MASK 0xff
+#define GPIO_BIT_CFG	0x400
+#define  GPIO_BIT_CFG_TX_OE		BIT(0)
+#define  GPIO_BIT_CFG_PIN_XOR		BIT(1)
+#define  GPIO_BIT_CFG_INT_EN		BIT(2)
+#define  GPIO_BIT_CFG_INT_TYPE		BIT(3)
+#define  GPIO_BIT_CFG_FIL_CNT_SHIFT	4
+#define  GPIO_BIT_CFG_FIL_SEL_SHIFT	8
+#define  GPIO_BIT_CFG_TX_OD		BIT(12)
+#define  GPIO_BIT_CFG_PIN_SEL_MASK	GENMASK(25, 16)
+#define GPIO_INTR	0x800
+#define  GPIO_INTR_INTR			BIT(0)
+#define  GPIO_INTR_INTR_W1S		BIT(1)
+#define  GPIO_INTR_ENA_W1C		BIT(2)
+#define  GPIO_INTR_ENA_W1S		BIT(3)
+#define GPIO_2ND_BANK	0x1400
+
+#define GLITCH_FILTER_400NS ((4ull << GPIO_BIT_CFG_FIL_SEL_SHIFT) | \
+			     (9ull << GPIO_BIT_CFG_FIL_CNT_SHIFT))
+
+static unsigned int bit_cfg_reg(unsigned int line)
+{
+	return 8 * line + GPIO_BIT_CFG;
+}
+
+static unsigned int intr_reg(unsigned int line)
+{
+	return 8 * line + GPIO_INTR;
+}
+
+struct thunderx_gpio;
+
+struct thunderx_irqdev {
+	struct thunderx_gpio	*gpio;
+	char			*name;
+	unsigned int		line;
+};
+
+struct thunderx_gpio {
+	struct gpio_chip	chip;
+	u8 __iomem		*register_base;
+	struct msix_entry	*msix_entries;
+	struct thunderx_irqdev	*irqdev_entries;
+	raw_spinlock_t		lock;
+	unsigned long		invert_mask[2];
+	unsigned long		od_mask[2];
+	int			base_msi;
+};
+
+
+/*
+ * Check (and WARN) that the pin is available for GPIO.  We will not
+ * allow modification of the state of non-GPIO pins from this driver.
+ */
+static bool thunderx_gpio_is_gpio(struct thunderx_gpio *gpio,
+				  unsigned int line)
+{
+	u64 bit_cfg = readq(gpio->register_base + bit_cfg_reg(line));
+	bool rv = (bit_cfg & GPIO_BIT_CFG_PIN_SEL_MASK) == 0;
+
+	WARN_RATELIMIT(!rv, "Pin %d not available for GPIO\n", line);
+
+	return rv;
+}
+
+static int thunderx_gpio_dir_in(struct gpio_chip *chip, unsigned int line)
+{
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+
+	if (!thunderx_gpio_is_gpio(gpio, line))
+		return -EIO;
+
+	raw_spin_lock(&gpio->lock);
+	clear_bit(line, gpio->invert_mask);
+	clear_bit(line, gpio->od_mask);
+	writeq(GLITCH_FILTER_400NS, gpio->register_base + bit_cfg_reg(line));
+	raw_spin_unlock(&gpio->lock);
+	return 0;
+}
+
+static void thunderx_gpio_set(struct gpio_chip *chip, unsigned int line,
+			      int value)
+{
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	int bank = line / 64;
+	int bank_bit = line % 64;
+
+	void __iomem *reg = gpio->register_base +
+		(bank * GPIO_2ND_BANK) + (value ? GPIO_TX_SET : GPIO_TX_CLR);
+
+	writeq(1ull << bank_bit, reg);
+}
+
+static int thunderx_gpio_dir_out(struct gpio_chip *chip, unsigned int line,
+				 int value)
+{
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	u64 bit_cfg = GPIO_BIT_CFG_TX_OE;
+
+	if (!thunderx_gpio_is_gpio(gpio, line))
+		return -EIO;
+
+	raw_spin_lock(&gpio->lock);
+
+	thunderx_gpio_set(chip, line, value);
+
+	if (test_bit(line, gpio->invert_mask))
+		bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
+
+	if (test_bit(line, gpio->od_mask))
+		bit_cfg |= GPIO_BIT_CFG_TX_OD;
+
+	writeq(bit_cfg, gpio->register_base + bit_cfg_reg(line));
+
+	raw_spin_unlock(&gpio->lock);
+	return 0;
+}
+
+/*
+ * Weird, setting open-drain mode causes signal inversion.  Note this
+ * so we can compensate in the dir_out function.
+ */
+static int thunderx_gpio_set_single_ended(struct gpio_chip *chip,
+					  unsigned int line,
+					  enum single_ended_mode mode)
+{
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+
+	if (mode == LINE_MODE_OPEN_SOURCE)
+		return -ENOTSUPP;
+
+	if (!thunderx_gpio_is_gpio(gpio, line))
+		return -EIO;
+
+	raw_spin_lock(&gpio->lock);
+	if (mode == LINE_MODE_OPEN_DRAIN) {
+		set_bit(line, gpio->invert_mask);
+		set_bit(line, gpio->od_mask);
+	} else {
+		clear_bit(line, gpio->invert_mask);
+		clear_bit(line, gpio->od_mask);
+	}
+	raw_spin_unlock(&gpio->lock);
+
+	return 0;
+}
+
+static int thunderx_gpio_get(struct gpio_chip *chip, unsigned int line)
+{
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	int bank = line / 64;
+	int bank_bit = line % 64;
+	u64 read_bits = readq(gpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_RX_DAT);
+
+	read_bits >>= bank_bit;
+
+	if (test_bit(line, gpio->invert_mask))
+		return !(read_bits & 1);
+	else
+		return read_bits & 1;
+}
+
+static void thunderx_gpio_set_multiple(struct gpio_chip *chip,
+				       unsigned long *mask,
+				       unsigned long *bits)
+{
+	int bank;
+	u64 set_bits, clear_bits;
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+
+	for (bank = 0; bank <= chip->ngpio / 64; bank++) {
+		set_bits = bits[bank] & mask[bank];
+		clear_bits = ~bits[bank] & mask[bank];
+		writeq(set_bits, gpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET);
+		writeq(clear_bits, gpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_CLR);
+	}
+}
+
+static irqreturn_t thunderx_gpio_chain_handler(int irq, void *dev)
+{
+	struct thunderx_irqdev *irqdev = dev;
+	int chained_irq;
+	int ret;
+
+	chained_irq = irq_find_mapping(irqdev->gpio->chip.irqdomain,
+				       irqdev->line);
+	if (!chained_irq)
+		return IRQ_NONE;
+
+	ret = generic_handle_irq(chained_irq);
+
+	return ret ? IRQ_NONE : IRQ_HANDLED;
+}
+
+static int thunderx_gpio_irq_request_resources(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	unsigned int line = data->hwirq;
+	struct thunderx_irqdev *irqdev;
+	int err;
+
+	if (!thunderx_gpio_is_gpio(gpio, line))
+		return -EIO;
+
+	irqdev = gpio->irqdev_entries + line;
+
+	irqdev->gpio = gpio;
+	irqdev->line = line;
+	irqdev->name = devm_kasprintf(chip->parent, GFP_KERNEL,
+				      "gpio-%d", line + chip->base);
+
+	writeq(GPIO_INTR_ENA_W1C, gpio->register_base + intr_reg(line));
+
+	err = devm_request_irq(chip->parent, gpio->msix_entries[line].vector,
+			       thunderx_gpio_chain_handler, IRQF_NO_THREAD, irqdev->name, irqdev);
+	return err;
+}
+
+static void thunderx_gpio_irq_release_resources(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	unsigned int line = data->hwirq;
+	struct thunderx_irqdev *irqdev;
+
+	irqdev = gpio->irqdev_entries + line;
+
+	/*
+	 * The request_resources/release_resources functions may be
+	 * called multiple times in the lifitime of the driver, so we
+	 * need to clean up the devm_* things to avoid a resource
+	 * leak.
+	 */
+	devm_free_irq(chip->parent, gpio->msix_entries[line].vector, irqdev);
+
+	writeq(GPIO_INTR_ENA_W1C, gpio->register_base + intr_reg(line));
+
+	devm_kfree(chip->parent, irqdev->name);
+}
+
+static void thunderx_gpio_irq_ack(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	unsigned int line = data->hwirq;
+
+	writeq(GPIO_INTR_INTR,
+	       gpio->register_base + intr_reg(line));
+}
+
+static void thunderx_gpio_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	unsigned int line = data->hwirq;
+
+	writeq(GPIO_INTR_ENA_W1C, gpio->register_base + intr_reg(line));
+}
+
+static void thunderx_gpio_irq_mask_ack(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	unsigned int line = data->hwirq;
+
+	writeq(GPIO_INTR_ENA_W1C | GPIO_INTR_INTR,
+	       gpio->register_base + intr_reg(line));
+}
+
+static void thunderx_gpio_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	unsigned int line = data->hwirq;
+
+	writeq(GPIO_INTR_ENA_W1S, gpio->register_base + intr_reg(line));
+}
+
+static int thunderx_gpio_irq_set_type(struct irq_data *data,
+				      unsigned int flow_type)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
+	unsigned int line = data->hwirq;
+	u64 bit_cfg;
+
+	irqd_set_trigger_type(data, flow_type);
+
+	bit_cfg = GLITCH_FILTER_400NS | GPIO_BIT_CFG_INT_EN;
+
+	if (flow_type & IRQ_TYPE_EDGE_BOTH) {
+		irq_set_handler_locked(data, handle_edge_irq);
+		bit_cfg |= GPIO_BIT_CFG_INT_TYPE;
+	} else {
+		irq_set_handler_locked(data, handle_level_irq);
+	}
+
+	raw_spin_lock(&gpio->lock);
+	if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW)) {
+		bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
+		set_bit(line, gpio->invert_mask);
+	} else {
+		clear_bit(line, gpio->invert_mask);
+	}
+	clear_bit(line, gpio->od_mask);
+	writeq(bit_cfg, gpio->register_base + bit_cfg_reg(line));
+	raw_spin_unlock(&gpio->lock);
+
+	return IRQ_SET_MASK_OK;
+}
+
+/*
+ * Interrupts are chained from underlying MSI-X vectors.  We have
+ * these irq_chip functions to be able to handle level triggering
+ * semantics and other acknowledgment tasks associated with the GPIO
+ * mechanism.
+ */
+static struct irq_chip thunderx_gpio_irq_chip = {
+	.name			= "GPIO",
+	.irq_enable		= thunderx_gpio_irq_unmask,
+	.irq_disable		= thunderx_gpio_irq_mask,
+	.irq_ack		= thunderx_gpio_irq_ack,
+	.irq_mask		= thunderx_gpio_irq_mask,
+	.irq_mask_ack		= thunderx_gpio_irq_mask_ack,
+	.irq_unmask		= thunderx_gpio_irq_unmask,
+	.irq_set_type		= thunderx_gpio_irq_set_type,
+	.irq_request_resources	= thunderx_gpio_irq_request_resources,
+	.irq_release_resources	= thunderx_gpio_irq_release_resources,
+	.flags			= IRQCHIP_SET_TYPE_MASKED
+};
+
+static int thunderx_gpio_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *id)
+{
+	void __iomem * const *tbl;
+	struct device *dev = &pdev->dev;
+	struct thunderx_gpio *gpio;
+	struct gpio_chip *chip;
+	int ngpio, i;
+	int err = 0;
+
+	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	raw_spin_lock_init(&gpio->lock);
+	chip = &gpio->chip;
+
+	pci_set_drvdata(pdev, gpio);
+
+	err = pcim_enable_device(pdev);
+	if (err) {
+		dev_err(dev, "Failed to enable PCI device: err %d\n", err);
+		goto out;
+	}
+
+	err = pcim_iomap_regions(pdev, 1 << 0, KBUILD_MODNAME);
+	if (err) {
+		dev_err(dev, "Failed to iomap PCI device: err %d\n", err);
+		goto out;
+	}
+
+	tbl = pcim_iomap_table(pdev);
+	gpio->register_base = tbl[0];
+	if (!gpio->register_base) {
+		dev_err(dev, "Cannot map PCI resource\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (pdev->subsystem_device == 0xa10a) {
+		/* CN88XX has no GPIO_CONST register*/
+		ngpio = 50;
+		gpio->base_msi = 48;
+	} else {
+		u64 c = readq(gpio->register_base + GPIO_CONST);
+
+		ngpio = c & GPIO_CONST_GPIOS_MASK;
+		gpio->base_msi = (c >> 8) & 0xff;
+	}
+
+	gpio->msix_entries = devm_kzalloc(dev,
+					  sizeof(struct msix_entry) * ngpio,
+					  GFP_KERNEL);
+	if (!gpio->msix_entries) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	gpio->irqdev_entries = devm_kzalloc(dev,
+					    sizeof(struct thunderx_irqdev) * ngpio,
+					    GFP_KERNEL);
+	if (!gpio->irqdev_entries) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < ngpio; i++)
+		gpio->msix_entries[i].entry = gpio->base_msi + (2 * i);
+
+	err = pci_enable_msix(pdev, gpio->msix_entries, ngpio);
+	if (err < 0)
+		goto out;
+
+	chip->label = KBUILD_MODNAME;
+	chip->parent = dev;
+	chip->owner = THIS_MODULE;
+	chip->base = -1; /* System allocated */
+	chip->can_sleep = false;
+	chip->ngpio = ngpio;
+	chip->direction_input = thunderx_gpio_dir_in;
+	chip->get = thunderx_gpio_get;
+	chip->direction_output = thunderx_gpio_dir_out;
+	chip->set = thunderx_gpio_set;
+	chip->set_multiple = thunderx_gpio_set_multiple;
+	chip->set_single_ended = thunderx_gpio_set_single_ended;
+	err = gpiochip_add(chip);
+	if (err)
+		goto out;
+
+	err = gpiochip_irqchip_add(chip, &thunderx_gpio_irq_chip, 0,
+				   handle_level_irq, IRQ_TYPE_NONE);
+	if (err) {
+		dev_err(dev, "gpiochip_irqchip_add failed: %d\n", err);
+		goto irqchip_out;
+	}
+
+	dev_info(dev, "ThunderX GPIO: %d lines with base %d.\n",
+		 ngpio, chip->base);
+	return 0;
+
+irqchip_out:
+	gpiochip_remove(chip);
+out:
+	pci_set_drvdata(pdev, NULL);
+	return err;
+}
+
+static void thunderx_gpio_remove(struct pci_dev *pdev)
+{
+	struct thunderx_gpio *gpio = pci_get_drvdata(pdev);
+
+	gpiochip_remove(&gpio->chip);
+	pci_set_drvdata(pdev, NULL);
+}
+
+static const struct pci_device_id thunderx_gpio_id_table[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xA00A) },
+	{ 0, }	/* end of table */
+};
+
+MODULE_DEVICE_TABLE(pci, thunderx_gpio_id_table);
+
+static struct pci_driver thunderx_gpio_driver = {
+	.name = KBUILD_MODNAME,
+	.id_table = thunderx_gpio_id_table,
+	.probe = thunderx_gpio_probe,
+	.remove = thunderx_gpio_remove,
+};
+
+module_pci_driver(thunderx_gpio_driver);
+
+MODULE_DESCRIPTION("Cavium Inc. ThunderX/OCTEON-TX GPIO Driver");
+MODULE_LICENSE("GPL");
-- 
1.8.3.1

Re: [PATCH v2 2/3] gpio: Add gpio driver support for ThunderX and OCTEON-TX

[Linus Walleij]

On Sat, Jan 7, 2017 at 12:22 AM, David Daney <ddaney.cavm@gmail.com> wrote:

> From: David Daney <david.daney@cavium.com>
>
> Cavium ThunderX and OCTEON-TX are arm64 based SoCs.  Add driver for
> the on-chip GPIO pins.
>
> Signed-off-by: David Daney <david.daney@cavium.com>

(...)
> +config GPIO_THUNDERX
> +       tristate "Cavium ThunderX/OCTEON-TX GPIO"

Do you really load this as module? OK then...

> +#include <linux/gpio.h>

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

Nothing else.

> +#define GLITCH_FILTER_400NS ((4ull << GPIO_BIT_CFG_FIL_SEL_SHIFT) | \
> +                            (9ull << GPIO_BIT_CFG_FIL_CNT_SHIFT))
> +
> +static unsigned int bit_cfg_reg(unsigned int line)
> +{
> +       return 8 * line + GPIO_BIT_CFG;
> +}
> +
> +static unsigned int intr_reg(unsigned int line)
> +{
> +       return 8 * line + GPIO_INTR;
> +}

This looks a bit overzealous, but OK.

> +struct thunderx_gpio;
> +
> +struct thunderx_irqdev {
> +       struct thunderx_gpio    *gpio;
> +       char                    *name;
> +       unsigned int            line;
> +};
> +
> +struct thunderx_gpio {
> +       struct gpio_chip        chip;
> +       u8 __iomem              *register_base;
> +       struct msix_entry       *msix_entries;
> +       struct thunderx_irqdev  *irqdev_entries;
> +       raw_spinlock_t          lock;
> +       unsigned long           invert_mask[2];
> +       unsigned long           od_mask[2];
> +       int                     base_msi;
> +};

Why can't you just move the thunderx_irqdev fields
into thunderx_gpio? It will save very little memory for
non-irq systems, I do not think footprint warrants it.

> +
> +static bool thunderx_gpio_is_gpio(struct thunderx_gpio *gpio,
> +                                 unsigned int line)
> +{
> +       u64 bit_cfg = readq(gpio->register_base + bit_cfg_reg(line));
> +       bool rv = (bit_cfg & GPIO_BIT_CFG_PIN_SEL_MASK) == 0;
> +
> +       WARN_RATELIMIT(!rv, "Pin %d not available for GPIO\n", line);
> +
> +       return rv;
> +}

Nifty. So this actually has a pin controller back-end?
I haven't seen the code for that yet, I think.

This seems like a cheap version of

/* External interface to pin control */
extern int pinctrl_request_gpio(unsigned gpio);
extern void pinctrl_free_gpio(unsigned gpio);
extern int pinctrl_gpio_direction_input(unsigned gpio);
extern int pinctrl_gpio_direction_output(unsigned gpio);

>From <linux/pinctrl/consumer.h>

So are you planning pin control support separately in drivers/pinctrl/*
in the future? Maybe some comment to replace this with proper pin control
in the future is warranted?

> +static int thunderx_gpio_dir_in(struct gpio_chip *chip, unsigned int line)
> +{
> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);

1. Please use gpiochip_get_data() instead of the container_of() construction,
   utilized devm_gpiochip_add_data() in your probe() call.

2. Do not call this local variable "gpio" that is superconfusing, at least call
    it txgpio or something.

1 & 2 applies EVERYWHERE in thid driver.

> +static void thunderx_gpio_set(struct gpio_chip *chip, unsigned int line,
> +                             int value)
> +{
> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
> +       int bank = line / 64;
> +       int bank_bit = line % 64;
> +
> +       void __iomem *reg = gpio->register_base +
> +               (bank * GPIO_2ND_BANK) + (value ? GPIO_TX_SET : GPIO_TX_CLR);
> +
> +       writeq(1ull << bank_bit, reg);

Use this:

#include <linus/bitops.h>

writeq(BIT(bank_bit), reg);

Applies EVERYWHERE you use (1ULL << n)

> +static int thunderx_gpio_set_single_ended(struct gpio_chip *chip,
> +                                         unsigned int line,
> +                                         enum single_ended_mode mode)

Thanks for implementing this properly.

> +       read_bits >>= bank_bit;
> +
> +       if (test_bit(line, gpio->invert_mask))
> +               return !(read_bits & 1);
> +       else
> +               return read_bits & 1;

This looks superconvoluted.

Can't you just:

if (test_bit(line, gpio->invert_mask))
   return !(read_bits & BIT(bank_bit));
else
   return !!(read_bits & BIT(bank_bit));

OK maybe not much clearer but seems clearer to me.

> +static int thunderx_gpio_irq_request_resources(struct irq_data *data)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
> +       unsigned int line = data->hwirq;
> +       struct thunderx_irqdev *irqdev;
> +       int err;
> +
> +       if (!thunderx_gpio_is_gpio(gpio, line))
> +               return -EIO;
> +
> +       irqdev = gpio->irqdev_entries + line;
> +
> +       irqdev->gpio = gpio;
> +       irqdev->line = line;
> +       irqdev->name = devm_kasprintf(chip->parent, GFP_KERNEL,
> +                                     "gpio-%d", line + chip->base);
> +
> +       writeq(GPIO_INTR_ENA_W1C, gpio->register_base + intr_reg(line));
> +
> +       err = devm_request_irq(chip->parent, gpio->msix_entries[line].vector,
> +                              thunderx_gpio_chain_handler, IRQF_NO_THREAD, irqdev->name, irqdev);
> +       return err;
> +}
> +
> +static void thunderx_gpio_irq_release_resources(struct irq_data *data)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
> +       unsigned int line = data->hwirq;
> +       struct thunderx_irqdev *irqdev;
> +
> +       irqdev = gpio->irqdev_entries + line;
> +
> +       /*
> +        * The request_resources/release_resources functions may be
> +        * called multiple times in the lifitime of the driver, so we
> +        * need to clean up the devm_* things to avoid a resource
> +        * leak.
> +        */
> +       devm_free_irq(chip->parent, gpio->msix_entries[line].vector, irqdev);
> +
> +       writeq(GPIO_INTR_ENA_W1C, gpio->register_base + intr_reg(line));
> +
> +       devm_kfree(chip->parent, irqdev->name);
> +}

Then just do not use the devm* variants. Explicitly allocate and free instead.

These callbacks should be called on all resources anyways.

> +static void thunderx_gpio_irq_unmask(struct irq_data *data)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
> +       unsigned int line = data->hwirq;
> +
> +       writeq(GPIO_INTR_ENA_W1S, gpio->register_base + intr_reg(line));
> +}
> +
> +static int thunderx_gpio_irq_set_type(struct irq_data *data,
> +                                     unsigned int flow_type)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
> +       unsigned int line = data->hwirq;
> +       u64 bit_cfg;
> +
> +       irqd_set_trigger_type(data, flow_type);
> +
> +       bit_cfg = GLITCH_FILTER_400NS | GPIO_BIT_CFG_INT_EN;
> +
> +       if (flow_type & IRQ_TYPE_EDGE_BOTH) {
> +               irq_set_handler_locked(data, handle_edge_irq);
> +               bit_cfg |= GPIO_BIT_CFG_INT_TYPE;
> +       } else {
> +               irq_set_handler_locked(data, handle_level_irq);
> +       }
> +
> +       raw_spin_lock(&gpio->lock);
> +       if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW)) {
> +               bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
> +               set_bit(line, gpio->invert_mask);
> +       } else {
> +               clear_bit(line, gpio->invert_mask);
> +       }
> +       clear_bit(line, gpio->od_mask);
> +       writeq(bit_cfg, gpio->register_base + bit_cfg_reg(line));
> +       raw_spin_unlock(&gpio->lock);
> +
> +       return IRQ_SET_MASK_OK;
> +}
> +
> +/*
> + * Interrupts are chained from underlying MSI-X vectors.  We have
> + * these irq_chip functions to be able to handle level triggering
> + * semantics and other acknowledgment tasks associated with the GPIO
> + * mechanism.
> + */
> +static struct irq_chip thunderx_gpio_irq_chip = {
> +       .name                   = "GPIO",
> +       .irq_enable             = thunderx_gpio_irq_unmask,
> +       .irq_disable            = thunderx_gpio_irq_mask,
> +       .irq_ack                = thunderx_gpio_irq_ack,
> +       .irq_mask               = thunderx_gpio_irq_mask,
> +       .irq_mask_ack           = thunderx_gpio_irq_mask_ack,
> +       .irq_unmask             = thunderx_gpio_irq_unmask,
> +       .irq_set_type           = thunderx_gpio_irq_set_type,
> +       .irq_request_resources  = thunderx_gpio_irq_request_resources,
> +       .irq_release_resources  = thunderx_gpio_irq_release_resources,
> +       .flags                  = IRQCHIP_SET_TYPE_MASKED
> +};

This looks wrong.

If you're calling devm_request_irq() on *every* *single* *irq* like you
do here, you are dealing with a hieararchical irqdomain, not a linear one,
and GPIOLIB_IRQCHIP may not be used.

Look at drivers/gpio/gpio-xgene-sb.c for inspiration. That is the only
hierarchical GPIO irqdomain I have right now.

Consult Marc Zyngier's IRQ domain lecture if you have time:
https://www.youtube.com/watch?v=YE8cRHVIM4E

If you have ideas how to combine hierarchical irqdomain and GPIOLIB_IRQCHIP
pls help out.

> +       gpio->irqdev_entries = devm_kzalloc(dev,
> +                                           sizeof(struct thunderx_irqdev) * ngpio,
> +                                           GFP_KERNEL);
> +       if (!gpio->irqdev_entries) {
> +               err = -ENOMEM;
> +               goto out;
> +       }

I think this is overkill. Use hierarchical irqdomain.

Yours,
Linus Walleij

Re: [PATCH v2 2/3] gpio: Add gpio driver support for ThunderX and OCTEON-TX

[David Daney]

On 01/09/2017 11:35 AM, Linus Walleij wrote:
> On Sat, Jan 7, 2017 at 12:22 AM, David Daney <ddaney.cavm@gmail.com> wrote:
>
>> From: David Daney <david.daney@cavium.com>
>>
>> Cavium ThunderX and OCTEON-TX are arm64 based SoCs.  Add driver for
>> the on-chip GPIO pins.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>
> (...)
>> +config GPIO_THUNDERX
>> +       tristate "Cavium ThunderX/OCTEON-TX GPIO"
>
> Do you really load this as module? OK then...

The driver does work when loaded as a module.  I have tested loading and 
unloading it many times.

>
>> +#include <linux/gpio.h>
>
> No.
> #include <linux/gpio/driver.h>
>
> Nothing else.

Right, I will fix that.

>
>> +#define GLITCH_FILTER_400NS ((4ull << GPIO_BIT_CFG_FIL_SEL_SHIFT) | \
>> +                            (9ull << GPIO_BIT_CFG_FIL_CNT_SHIFT))
>> +
>> +static unsigned int bit_cfg_reg(unsigned int line)
>> +{
>> +       return 8 * line + GPIO_BIT_CFG;
>> +}
>> +
>> +static unsigned int intr_reg(unsigned int line)
>> +{
>> +       return 8 * line + GPIO_INTR;
>> +}
>
> This looks a bit overzealous, but OK.
>
>> +struct thunderx_gpio;
>> +
>> +struct thunderx_irqdev {
>> +       struct thunderx_gpio    *gpio;
>> +       char                    *name;
>> +       unsigned int            line;
>> +};
>> +
>> +struct thunderx_gpio {
>> +       struct gpio_chip        chip;
>> +       u8 __iomem              *register_base;
>> +       struct msix_entry       *msix_entries;
>> +       struct thunderx_irqdev  *irqdev_entries;
>> +       raw_spinlock_t          lock;
>> +       unsigned long           invert_mask[2];
>> +       unsigned long           od_mask[2];
>> +       int                     base_msi;
>> +};
>
> Why can't you just move the thunderx_irqdev fields
> into thunderx_gpio?

There is a variable length array of them.  It is not a single instance 
of the structure.  Same for the msix_entries.

> It will save very little memory for
> non-irq systems, I do not think footprint warrants it.
>
>> +
>> +static bool thunderx_gpio_is_gpio(struct thunderx_gpio *gpio,
>> +                                 unsigned int line)
>> +{
>> +       u64 bit_cfg = readq(gpio->register_base + bit_cfg_reg(line));
>> +       bool rv = (bit_cfg & GPIO_BIT_CFG_PIN_SEL_MASK) == 0;
>> +
>> +       WARN_RATELIMIT(!rv, "Pin %d not available for GPIO\n", line);
>> +
>> +       return rv;
>> +}
>
> Nifty. So this actually has a pin controller back-end?

The hardware does function like a "pin controller"

> I haven't seen the code for that yet, I think.
>
> This seems like a cheap version of
>
> /* External interface to pin control */
> extern int pinctrl_request_gpio(unsigned gpio);
> extern void pinctrl_free_gpio(unsigned gpio);
> extern int pinctrl_gpio_direction_input(unsigned gpio);
> extern int pinctrl_gpio_direction_output(unsigned gpio);
>
> From <linux/pinctrl/consumer.h>
>
> So are you planning pin control support separately in drivers/pinctrl/*
> in the future?

Not at this time.  Currently early boot firmware is programming the pin 
functions, and we don't see a need to move the complexity of pinctrl 
into the kernel, especially as there is not really any ACPI support for 
pinctrl at this point, and we must also support ACPI.

> Maybe some comment to replace this with proper pin control
> in the future is warranted?
>

Good idea.  I will add a comment about this.


>> +static int thunderx_gpio_dir_in(struct gpio_chip *chip, unsigned int line)
>> +{
>> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
>
> 1. Please use gpiochip_get_data() instead of the container_of() construction,
>    utilized devm_gpiochip_add_data() in your probe() call.
>
> 2. Do not call this local variable "gpio" that is superconfusing, at least call
>     it txgpio or something.
>
> 1 & 2 applies EVERYWHERE in thid driver.

OK, I will make that change for the next revision of the patch set.

>
>> +static void thunderx_gpio_set(struct gpio_chip *chip, unsigned int line,
>> +                             int value)
>> +{
>> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
>> +       int bank = line / 64;
>> +       int bank_bit = line % 64;
>> +
>> +       void __iomem *reg = gpio->register_base +
>> +               (bank * GPIO_2ND_BANK) + (value ? GPIO_TX_SET : GPIO_TX_CLR);
>> +
>> +       writeq(1ull << bank_bit, reg);
>
> Use this:
>
> #include <linus/bitops.h>
>
> writeq(BIT(bank_bit), reg);
>
> Applies EVERYWHERE you use (1ULL << n)

OK.

>
>> +static int thunderx_gpio_set_single_ended(struct gpio_chip *chip,
>> +                                         unsigned int line,
>> +                                         enum single_ended_mode mode)
>
> Thanks for implementing this properly.
>
>> +       read_bits >>= bank_bit;
>> +
>> +       if (test_bit(line, gpio->invert_mask))
>> +               return !(read_bits & 1);
>> +       else
>> +               return read_bits & 1;
>
> This looks superconvoluted.
>
> Can't you just:
>
> if (test_bit(line, gpio->invert_mask))
>    return !(read_bits & BIT(bank_bit));
> else
>    return !!(read_bits & BIT(bank_bit));
>
> OK maybe not much clearer but seems clearer to me.

As I really dislike the "!!" idiom, would you settle for:

  if (test_bit(line, gpio->invert_mask))
     return (read_bits & BIT(bank_bit)) == 0;
  else
     return (read_bits & BIT(bank_bit)) != 0;

>
>> +static int thunderx_gpio_irq_request_resources(struct irq_data *data)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
>> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
>> +       unsigned int line = data->hwirq;
>> +       struct thunderx_irqdev *irqdev;
>> +       int err;
>> +
>> +       if (!thunderx_gpio_is_gpio(gpio, line))
>> +               return -EIO;
>> +
>> +       irqdev = gpio->irqdev_entries + line;
>> +
>> +       irqdev->gpio = gpio;
>> +       irqdev->line = line;
>> +       irqdev->name = devm_kasprintf(chip->parent, GFP_KERNEL,
>> +                                     "gpio-%d", line + chip->base);
>> +
>> +       writeq(GPIO_INTR_ENA_W1C, gpio->register_base + intr_reg(line));
>> +
>> +       err = devm_request_irq(chip->parent, gpio->msix_entries[line].vector,
>> +                              thunderx_gpio_chain_handler, IRQF_NO_THREAD, irqdev->name, irqdev);
>> +       return err;
>> +}
>> +
>> +static void thunderx_gpio_irq_release_resources(struct irq_data *data)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
>> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
>> +       unsigned int line = data->hwirq;
>> +       struct thunderx_irqdev *irqdev;
>> +
>> +       irqdev = gpio->irqdev_entries + line;
>> +
>> +       /*
>> +        * The request_resources/release_resources functions may be
>> +        * called multiple times in the lifitime of the driver, so we
>> +        * need to clean up the devm_* things to avoid a resource
>> +        * leak.
>> +        */
>> +       devm_free_irq(chip->parent, gpio->msix_entries[line].vector, irqdev);
>> +
>> +       writeq(GPIO_INTR_ENA_W1C, gpio->register_base + intr_reg(line));
>> +
>> +       devm_kfree(chip->parent, irqdev->name);
>> +}
>
> Then just do not use the devm* variants. Explicitly allocate and free instead.
>
> These callbacks should be called on all resources anyways.

Rithe.

>
>> +static void thunderx_gpio_irq_unmask(struct irq_data *data)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
>> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
>> +       unsigned int line = data->hwirq;
>> +
>> +       writeq(GPIO_INTR_ENA_W1S, gpio->register_base + intr_reg(line));
>> +}
>> +
>> +static int thunderx_gpio_irq_set_type(struct irq_data *data,
>> +                                     unsigned int flow_type)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
>> +       struct thunderx_gpio *gpio = container_of(chip, struct thunderx_gpio, chip);
>> +       unsigned int line = data->hwirq;
>> +       u64 bit_cfg;
>> +
>> +       irqd_set_trigger_type(data, flow_type);
>> +
>> +       bit_cfg = GLITCH_FILTER_400NS | GPIO_BIT_CFG_INT_EN;
>> +
>> +       if (flow_type & IRQ_TYPE_EDGE_BOTH) {
>> +               irq_set_handler_locked(data, handle_edge_irq);
>> +               bit_cfg |= GPIO_BIT_CFG_INT_TYPE;
>> +       } else {
>> +               irq_set_handler_locked(data, handle_level_irq);
>> +       }
>> +
>> +       raw_spin_lock(&gpio->lock);
>> +       if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW)) {
>> +               bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
>> +               set_bit(line, gpio->invert_mask);
>> +       } else {
>> +               clear_bit(line, gpio->invert_mask);
>> +       }
>> +       clear_bit(line, gpio->od_mask);
>> +       writeq(bit_cfg, gpio->register_base + bit_cfg_reg(line));
>> +       raw_spin_unlock(&gpio->lock);
>> +
>> +       return IRQ_SET_MASK_OK;
>> +}
>> +
>> +/*
>> + * Interrupts are chained from underlying MSI-X vectors.  We have
>> + * these irq_chip functions to be able to handle level triggering
>> + * semantics and other acknowledgment tasks associated with the GPIO
>> + * mechanism.
>> + */
>> +static struct irq_chip thunderx_gpio_irq_chip = {
>> +       .name                   = "GPIO",
>> +       .irq_enable             = thunderx_gpio_irq_unmask,
>> +       .irq_disable            = thunderx_gpio_irq_mask,
>> +       .irq_ack                = thunderx_gpio_irq_ack,
>> +       .irq_mask               = thunderx_gpio_irq_mask,
>> +       .irq_mask_ack           = thunderx_gpio_irq_mask_ack,
>> +       .irq_unmask             = thunderx_gpio_irq_unmask,
>> +       .irq_set_type           = thunderx_gpio_irq_set_type,
>> +       .irq_request_resources  = thunderx_gpio_irq_request_resources,
>> +       .irq_release_resources  = thunderx_gpio_irq_release_resources,
>> +       .flags                  = IRQCHIP_SET_TYPE_MASKED
>> +};
>
> This looks wrong.
>
> If you're calling devm_request_irq() on *every* *single* *irq* like you
> do here, you are dealing with a hieararchical irqdomain, not a linear one,
> and GPIOLIB_IRQCHIP may not be used.
>
> Look at drivers/gpio/gpio-xgene-sb.c for inspiration. That is the only
> hierarchical GPIO irqdomain I have right now.
>
> Consult Marc Zyngier's IRQ domain lecture if you have time:
> https://www.youtube.com/watch?v=YE8cRHVIM4E
>
> If you have ideas how to combine hierarchical irqdomain and GPIOLIB_IRQCHIP
> pls help out.
>
>> +       gpio->irqdev_entries = devm_kzalloc(dev,
>> +                                           sizeof(struct thunderx_irqdev) * ngpio,
>> +                                           GFP_KERNEL);
>> +       if (!gpio->irqdev_entries) {
>> +               err = -ENOMEM;
>> +               goto out;
>> +       }
>
> I think this is overkill. Use hierarchical irqdomain.

I will look into it.  I suspect it will require more lines of driver 
code to implement it than what I have here (that does actually work).

Thanks for looking at this,
David

Re: [PATCH v2 2/3] gpio: Add gpio driver support for ThunderX and OCTEON-TX

[Linus Walleij]

On Mon, Jan 9, 2017 at 9:02 PM, David Daney <ddaney@caviumnetworks.com> wrote:

>> if (test_bit(line, gpio->invert_mask))
>>    return !(read_bits & BIT(bank_bit));
>> else
>>    return !!(read_bits & BIT(bank_bit));
>>
>> OK maybe not much clearer but seems clearer to me.
>
> As I really dislike the "!!" idiom, would you settle for:
>
>  if (test_bit(line, gpio->invert_mask))
>     return (read_bits & BIT(bank_bit)) == 0;
>  else
>     return (read_bits & BIT(bank_bit)) != 0;

Not the biggest issue in the world. But I maintain a huge stack
of GPIO drivers and it drives me crazy that each one has to bear
the mark of the authors habits rather than mine.

>> I think this is overkill. Use hierarchical irqdomain.
>
> I will look into it.  I suspect it will require more lines of driver code to
> implement it than what I have here (that does actually work).

I understand. But at the same time, the kernel needs to have the
right idea of what it is dealing with here.

The generic IRQ handling code will take a shorter fastpath if
you are using hierarchical irqdomain (I think?) but I can't claim
to be an expert. When in doubt, consult Marc Z.

Yours,
Linus Walleij

[PATCH v2 3/3] MAINTAINERS: Add entry for THUNDERX GPIO Driver.

[David Daney]

From: David Daney <david.daney@cavium.com>

Signed-off-by: David Daney <david.daney@cavium.com>
---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 97d0b68..3d254e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10886,6 +10886,11 @@ M:	Andreas Noever <andreas.noever@gmail.com>
 S:	Maintained
 F:	drivers/thunderbolt/
 
+THUNDERX GPIO DRIVER
+M:	David Daney <david.daney@cavium.com>
+S:	Maintained
+F:	drivers/gpio/gpio-thunderx.c
+
 TI BQ27XXX POWER SUPPLY DRIVER
 R:	Andrew F. Davis <afd@ti.com>
 F:	include/linux/power/bq27xxx_battery.h
-- 
1.8.3.1