[PATCH v4 0/4] gpio: visconti: Add Toshiba Visconti GPIO support

[PATCH v4 0/4] gpio: visconti: Add Toshiba Visconti GPIO support

[Nobuhiro Iwamatsu]

Hi,

This series is the GPIO driver for Toshiba's ARM SoC, Visconti[0].
This provides DT binding documentation, device driver, MAINTAINER files, and updates to DT files.

Best regards,
  Nobuhiro

[0]: https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-recognition-processors-visconti.html

dt-bindings: gpio: Add bindings for Toshiba Visconti GPIO Controller:
  v3 -> v4: Add Reviewed-by tag.
  v2 -> v3: Fix dtschema/dtc warnings.
    dtschema/dtc warnings/errors:
      Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.example.dt.yaml: gpio@28020000: interrupts: [[0, 24, 4], [0, 25, 4], [0, 26, 4], [0, 27, 4], [0, 28, 4], [0, 29, 4], [0, 30, 4], [0, 31, 4], [0, 32, 4], [0, 33, 4], [0, 34, 4], [0, 35, 4], [0, 36, 4], [0, 37, 4], [0, 38, 4], [0, 39, 4]] is too short
	From schema: Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml
  v1 -> v2: Fix typo.

gpio: visoconti: Add Toshiba Visconti GPIO support:
  v3 -> v4: Drop VISCONTI_GPIO_NR.
            Fix return code of platform_irq_count.
            Fix coprytight header.
            Add Reviewed-by tag.
  v2 -> v3: Add select GPIO_GENERIC
            Use genric MMIO GPIO library
            Use bgpio_init() as initialized the generic helpers.
            Use irqchip template instead of gpiochip_irqchip_add().
  v1 -> v2: No update

MAINTAINERS: Add entries for Toshiba Visconti GPIO controller:
  v3 -> v4: No update
  v2 -> v3: No update
  v1 -> v2: No update

arm: dts: visconti: Add DT support for Toshiba Visconti5 GPIO driver:
  v3 -> v4: Add Reviewed-by tag.
  v2 -> v3: Fix compatible string.
  v1 -> v2: No update

Nobuhiro Iwamatsu (4):
  dt-bindings: gpio: Add bindings for Toshiba Visconti GPIO Controller
  gpio: visconti: Add Toshiba Visconti GPIO support
  MAINTAINERS: Add entries for Toshiba Visconti GPIO controller
  arm: dts: visconti: Add DT support for Toshiba Visconti5 GPIO driver

 .../bindings/gpio/toshiba,gpio-visconti.yaml  |  85 +++++++
 MAINTAINERS                                   |   2 +
 .../boot/dts/toshiba/tmpv7708-rm-mbrc.dts     |   4 +
 arch/arm64/boot/dts/toshiba/tmpv7708.dtsi     |  27 ++
 drivers/gpio/Kconfig                          |   9 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-visconti.c                  | 230 ++++++++++++++++++
 drivers/pinctrl/visconti/pinctrl-common.c     |  23 ++
 8 files changed, 381 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml
 create mode 100644 drivers/gpio/gpio-visconti.c

-- 
2.29.2

[PATCH v4 1/4] dt-bindings: gpio: Add bindings for Toshiba Visconti GPIO Controller

[Nobuhiro Iwamatsu]

Add bindings for the Toshiba Visconti GPIO Controller.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
---
 .../bindings/gpio/toshiba,gpio-visconti.yaml  | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml

diff --git a/Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml b/Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml
new file mode 100644
index 000000000000..5168a15b90e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml
@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/toshiba,gpio-visconti.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toshiba Visconti ARM SoCs GPIO controller
+
+maintainers:
+  - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
+
+properties:
+  compatible:
+    items:
+      - const: toshiba,gpio-tmpv7708
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  gpio-controller: true
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    description:
+      interrupt mapping one per GPIO.
+    minItems: 16
+    maxItems: 16
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-ranges
+  - gpio-controller
+  - interrupt-controller
+  - "#interrupt-cells"
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+      #include <dt-bindings/interrupt-controller/irq.h>
+      #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+      soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        gpio: gpio@28020000 {
+          compatible = "toshiba,gpio-tmpv7708";
+          reg = <0 0x28020000 0 0x1000>;
+          #gpio-cells = <0x2>;
+          gpio-ranges = <&pmux 0 0 32>;
+          gpio-controller;
+          interrupt-controller;
+          #interrupt-cells = <2>;
+          interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
+              <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
+        };
+      };
+...
-- 
2.29.2

[PATCH v4 2/4] gpio: visconti: Add Toshiba Visconti GPIO support

[Nobuhiro Iwamatsu]

Add the GPIO driver for Toshiba Visconti ARM SoCs.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
---
 drivers/gpio/Kconfig                      |   9 +
 drivers/gpio/Makefile                     |   1 +
 drivers/gpio/gpio-visconti.c              | 230 ++++++++++++++++++++++
 drivers/pinctrl/visconti/pinctrl-common.c |  23 +++
 4 files changed, 263 insertions(+)
 create mode 100644 drivers/gpio/gpio-visconti.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5d4de5cd6759..85d0f4499d63 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -631,6 +631,15 @@ config GPIO_VF610
 	help
 	  Say yes here to support Vybrid vf610 GPIOs.
 
+config GPIO_VISCONTI
+	tristate "Toshiba Visconti GPIO support"
+	depends on ARCH_VISCONTI || COMPILE_TEST
+	depends on OF_GPIO
+	select GPIOLIB_IRQCHIP
+	select GPIO_GENERIC
+	help
+	  Say yes here to support GPIO on Tohisba Visconti.
+
 config GPIO_VR41XX
 	tristate "NEC VR4100 series General-purpose I/O Uint support"
 	depends on CPU_VR41XX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 09dada80ac34..02c9d8d83a54 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -162,6 +162,7 @@ obj-$(CONFIG_GPIO_UCB1400)		+= gpio-ucb1400.o
 obj-$(CONFIG_GPIO_UNIPHIER)		+= gpio-uniphier.o
 obj-$(CONFIG_GPIO_VF610)		+= gpio-vf610.o
 obj-$(CONFIG_GPIO_VIPERBOARD)		+= gpio-viperboard.o
+obj-$(CONFIG_GPIO_VISCONTI)		+= gpio-visconti.o
 obj-$(CONFIG_GPIO_VR41XX)		+= gpio-vr41xx.o
 obj-$(CONFIG_GPIO_VX855)		+= gpio-vx855.o
 obj-$(CONFIG_GPIO_WCD934X)		+= gpio-wcd934x.o
diff --git a/drivers/gpio/gpio-visconti.c b/drivers/gpio/gpio-visconti.c
new file mode 100644
index 000000000000..8a3081bf67cf
--- /dev/null
+++ b/drivers/gpio/gpio-visconti.c
@@ -0,0 +1,230 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Toshiba Visconti GPIO Support
+ *
+ * (C) Copyright 2020 Toshiba Electronic Devices & Storage Corporation
+ * (C) Copyright 2020 TOSHIBA CORPORATION
+ *
+ * Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
+ */
+
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/driver.h>
+#include <linux/of.h>
+#include <linux/bitops.h>
+
+/* register offset */
+#define GPIO_DIR	0x00
+#define GPIO_IDATA	0x08
+#define GPIO_ODATA	0x10
+#define GPIO_OSET	0x18
+#define GPIO_OCLR	0x20
+#define GPIO_INTMODE	0x30
+
+struct visconti_gpio {
+	void __iomem *base;
+	int *irq;
+	spinlock_t lock; /* protect gpio register */
+	struct device *dev;
+	struct gpio_chip gpio_chip;
+	struct irq_chip irq_chip;
+};
+
+static void visconti_gpio_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct visconti_gpio *priv = gpiochip_get_data(gc);
+
+	disable_irq_nosync(priv->irq[irqd_to_hwirq(d)]);
+}
+
+static void visconti_gpio_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct visconti_gpio *priv = gpiochip_get_data(gc);
+
+	enable_irq(priv->irq[irqd_to_hwirq(d)]);
+}
+
+static int visconti_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct visconti_gpio *priv = gpiochip_get_data(gc);
+	u32 offset = irqd_to_hwirq(d);
+	u32 bit = BIT(offset);
+	u32 intc_type = IRQ_TYPE_EDGE_RISING;
+	u32 intmode, odata;
+	int ret = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	odata = readl(priv->base + GPIO_ODATA);
+	intmode = readl(priv->base + GPIO_INTMODE);
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		odata &= ~bit;
+		intmode &= ~bit;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		odata |= bit;
+		intmode &= ~bit;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		intmode |= bit;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		intc_type = IRQ_TYPE_LEVEL_HIGH;
+		odata &= ~bit;
+		intmode &= ~bit;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		intc_type = IRQ_TYPE_LEVEL_HIGH;
+		odata |= bit;
+		intmode &= ~bit;
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+	writel(odata, priv->base + GPIO_ODATA);
+	writel(intmode, priv->base + GPIO_INTMODE);
+	irq_set_irq_type(priv->irq[offset], intc_type);
+err:
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return ret;
+}
+
+static irqreturn_t visconti_gpio_irq_handler(int irq, void *dev_id)
+{
+	struct visconti_gpio *priv = dev_id;
+	u32 offset = irq - priv->irq[0];
+
+	generic_handle_irq(irq_find_mapping(priv->gpio_chip.irq.domain, offset));
+
+	return IRQ_HANDLED;
+}
+
+static void visconti_init_irq_valid_mask(struct gpio_chip *chip, unsigned long *valid_mask,
+					 unsigned int ngpios)
+{
+	int i;
+
+	/* Exclude GPIO pins 16-31 from irq */
+	for (i = 16; i < ngpios; i++)
+		clear_bit(i, valid_mask);
+}
+
+static int visconti_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct visconti_gpio *priv;
+	struct irq_chip *irq_chip;
+	struct irq_desc *desc;
+	struct gpio_irq_chip *girq;
+	const char *name = dev_name(dev);
+	int i, ret, num_irq;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	spin_lock_init(&priv->lock);
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	ret = platform_irq_count(pdev);
+	if (ret < 0)
+		return ret;
+
+	num_irq = ret;
+
+	priv->irq = devm_kcalloc(dev, num_irq, sizeof(priv->irq), GFP_KERNEL);
+	if (!priv->irq)
+		return -ENOMEM;
+
+	for (i = 0; i < num_irq; i++) {
+		priv->irq[i] = platform_get_irq(pdev, i);
+		if (priv->irq[i] < 0) {
+			dev_err(dev, "invalid IRQ[%d]\n", i);
+			return priv->irq[i];
+		}
+	}
+
+	ret = bgpio_init(&priv->gpio_chip, dev, 4,
+			 priv->base + GPIO_IDATA,
+			 priv->base + GPIO_OSET,
+			 priv->base + GPIO_OCLR,
+			 priv->base + GPIO_DIR,
+			 NULL,
+			 0);
+	if (ret) {
+		dev_err(dev, "unable to init generic GPIO\n");
+		return ret;
+	}
+
+	priv->gpio_chip.irq.init_valid_mask = visconti_init_irq_valid_mask;
+
+	irq_chip = &priv->irq_chip;
+	irq_chip->name = "gpio-visconti";
+	irq_chip->irq_mask = visconti_gpio_irq_mask;
+	irq_chip->irq_unmask = visconti_gpio_irq_unmask;
+	irq_chip->irq_set_type = visconti_gpio_irq_set_type;
+	irq_chip->flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_MASK_ON_SUSPEND;
+
+	girq = &priv->gpio_chip.irq;
+	girq->chip = irq_chip;
+	/* This will let us handle the parent IRQ in the driver */
+	girq->parent_handler = NULL;
+	girq->num_parents = 0;
+	girq->parents = NULL;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+
+	ret = devm_gpiochip_add_data(dev, &priv->gpio_chip, priv);
+	if (ret) {
+		dev_err(dev, "failed to add GPIO chip\n");
+		return ret;
+	}
+
+	for (i = 0; i < num_irq; i++) {
+		desc = irq_to_desc(priv->irq[i]);
+		desc->status_use_accessors |= IRQ_NOAUTOEN;
+		if (devm_request_irq(dev, priv->irq[i],
+				     visconti_gpio_irq_handler, 0, name, priv)) {
+			dev_err(dev, "failed to request IRQ[%d]\n", i);
+			return -ENOENT;
+		}
+	}
+
+	return ret;
+}
+
+static const struct of_device_id visconti_gpio_of_match[] = {
+	{ .compatible = "toshiba,gpio-tmpv7708", },
+	{ /* end of table */ }
+};
+MODULE_DEVICE_TABLE(of, visconti_gpio_of_match);
+
+static struct platform_driver visconti_gpio_driver = {
+	.probe		= visconti_gpio_probe,
+	.driver		= {
+		.name	= "visconti_gpio",
+		.of_match_table = of_match_ptr(visconti_gpio_of_match),
+	}
+};
+module_platform_driver(visconti_gpio_driver);
+
+MODULE_AUTHOR("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>");
+MODULE_DESCRIPTION("Toshiba Visconti GPIO Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/pinctrl/visconti/pinctrl-common.c b/drivers/pinctrl/visconti/pinctrl-common.c
index 0cb10b7b4430..21c7e0d18fea 100644
--- a/drivers/pinctrl/visconti/pinctrl-common.c
+++ b/drivers/pinctrl/visconti/pinctrl-common.c
@@ -245,11 +245,34 @@ static int visconti_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int visconti_gpio_request_enable(struct pinctrl_dev *pctldev,
+				      struct pinctrl_gpio_range *range,
+				      unsigned int pin)
+{
+	struct visconti_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
+	const struct visconti_mux *gpio_mux = &priv->devdata->gpio_mux[pin];
+	unsigned long flags;
+	unsigned int val;
+
+	dev_dbg(priv->dev, "%s: pin = %d\n", __func__, pin);
+
+	/* update mux */
+	spin_lock_irqsave(&priv->lock, flags);
+	val = readl(priv->base + gpio_mux->offset);
+	val &= ~gpio_mux->mask;
+	val |= gpio_mux->val;
+	writel(val, priv->base + gpio_mux->offset);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return 0;
+}
+
 static const struct pinmux_ops visconti_pinmux_ops = {
 	.get_functions_count	= visconti_get_functions_count,
 	.get_function_name	= visconti_get_function_name,
 	.get_function_groups	= visconti_get_function_groups,
 	.set_mux		= visconti_set_mux,
+	.gpio_request_enable	= visconti_gpio_request_enable,
 	.strict			= true,
 };
 
-- 
2.29.2

[PATCH v4 3/4] MAINTAINERS: Add entries for Toshiba Visconti GPIO controller

[Nobuhiro Iwamatsu]

Add entries for Toshiba Visconti GPIO Controller binding and driver.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 50fbbfffb921..8565272d95c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2636,8 +2636,10 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Supported
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/iwamatsu/linux-visconti.git
 F:	Documentation/devicetree/bindings/arm/toshiba.yaml
+F:	Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml
 F:	Documentation/devicetree/bindings/pinctrl/toshiba,tmpv7700-pinctrl.yaml
 F:	arch/arm64/boot/dts/toshiba/
+F:	drivers/gpio/gpio-visconti.c
 F:	drivers/pinctrl/visconti/
 N:	visconti
 
-- 
2.29.2

[PATCH v4 4/4] arm: dts: visconti: Add DT support for Toshiba Visconti5 GPIO driver

[Nobuhiro Iwamatsu]

Add the GPIO node in Toshiba Visconti5 SoC-specific DT file.
And enable the GPIO node in TMPV7708 RM main board's board-specific DT file.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
---
 .../boot/dts/toshiba/tmpv7708-rm-mbrc.dts     |  4 +++
 arch/arm64/boot/dts/toshiba/tmpv7708.dtsi     | 27 +++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/arch/arm64/boot/dts/toshiba/tmpv7708-rm-mbrc.dts b/arch/arm64/boot/dts/toshiba/tmpv7708-rm-mbrc.dts
index ed0bf7f13f54..950010a290f0 100644
--- a/arch/arm64/boot/dts/toshiba/tmpv7708-rm-mbrc.dts
+++ b/arch/arm64/boot/dts/toshiba/tmpv7708-rm-mbrc.dts
@@ -41,3 +41,7 @@ &uart1 {
 	clocks = <&uart_clk>;
 	clock-names = "apb_pclk";
 };
+
+&gpio {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/toshiba/tmpv7708.dtsi b/arch/arm64/boot/dts/toshiba/tmpv7708.dtsi
index 242f25f4e12a..ac9bddb35b0a 100644
--- a/arch/arm64/boot/dts/toshiba/tmpv7708.dtsi
+++ b/arch/arm64/boot/dts/toshiba/tmpv7708.dtsi
@@ -157,6 +157,33 @@ pmux: pmux@24190000 {
 			reg = <0 0x24190000 0 0x10000>;
 		};
 
+		gpio: gpio@28020000 {
+			compatible = "toshiba,gpio-tmpv7708";
+			reg = <0 0x28020000 0 0x1000>;
+			#gpio-cells = <0x2>;
+			gpio-ranges = <&pmux 0 0 32>;
+			gpio-controller;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			interrupts =
+				<GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
 		uart0: serial@28200000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0 0x28200000 0 0x1000>;
-- 
2.29.2

Re: [PATCH v4 1/4] dt-bindings: gpio: Add bindings for Toshiba Visconti GPIO Controller

[Linus Walleij]

Looping in Marc here:

On Fri, Dec 11, 2020 at 1:43 AM Nobuhiro Iwamatsu
<nobuhiro1.iwamatsu@toshiba.co.jp> wrote:

> Add bindings for the Toshiba Visconti GPIO Controller.
>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
(...)
> +          interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
> +              <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;

This is an hierarchical IRQ controller. (These IRQs are mapped 1-to-1
to IRQ lines.)
I was under the impression that we don't encode interrupts into the GPIO
controller like this when we have that.

Instead, hardcode these into the driver. The compatible string gives
away how the
local offsets map to the GIC IRQs.

Add no interrupts to the node but make sure that the GIC is the parent.
(Should be default.)

Compare e.g.
Documentation/devicetree/bindings/gpio/intel,ixp4xx-gpio.txt

Which has a similar "some hierarchical IRQs" setup.

Yours,
Linus Walleij

Re: [PATCH v4 2/4] gpio: visconti: Add Toshiba Visconti GPIO support

[Linus Walleij]

On Fri, Dec 11, 2020 at 1:43 AM Nobuhiro Iwamatsu
<nobuhiro1.iwamatsu@toshiba.co.jp> wrote:

This iteration is looking really good, but we are not quite there yet,
because now that the driver looks so much better I can see that it
is a hierarchical interrupt controller.

> Add the GPIO driver for Toshiba Visconti ARM SoCs.
>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
(...)

> +config GPIO_VISCONTI
> +       tristate "Toshiba Visconti GPIO support"
> +       depends on ARCH_VISCONTI || COMPILE_TEST
> +       depends on OF_GPIO
> +       select GPIOLIB_IRQCHIP
> +       select GPIO_GENERIC
> +       help
> +         Say yes here to support GPIO on Tohisba Visconti.

Add
select IRQ_DOMAIN_HIERARCHY

> +struct visconti_gpio {
> +       void __iomem *base;
> +       int *irq;

Don't keep these irqs around.

> +       ret = platform_irq_count(pdev);
> +       if (ret < 0)
> +               return ret;
> +
> +       num_irq = ret;
> +
> +       priv->irq = devm_kcalloc(dev, num_irq, sizeof(priv->irq), GFP_KERNEL);
> +       if (!priv->irq)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < num_irq; i++) {
> +               priv->irq[i] = platform_get_irq(pdev, i);
> +               if (priv->irq[i] < 0) {
> +                       dev_err(dev, "invalid IRQ[%d]\n", i);
> +                       return priv->irq[i];
> +               }
> +       }

Instead of doing this, look in for example
drivers/gpio/gpio-ixp4xx.c

You need:

> +       girq = &priv->gpio_chip.irq;
> +       girq->chip = irq_chip;

girq->fwnode = fwnode;
girq->parent_domain = parent;
girq->child_to_parent_hwirq = visconti_gpio_child_to_parent_hwirq;

The mapping function will be something like this:

static int visconti_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
                                             unsigned int child,
                                             unsigned int child_type,
                                             unsigned int *parent,
                                             unsigned int *parent_type)
{
        /* Interrupts 0..15 mapped to interrupts 24..39 on the GIC */
        if (child < 16) {
            /* All these interrupts are level high in the CPU */
            *parent_type = IRQ_TYPE_LEVEL_HIGH;
            *parent = child + 24;
            return 0;
        }
        return -EINVAL;
}

> +       priv->gpio_chip.irq.init_valid_mask = visconti_init_irq_valid_mask;

This will be set up by gpiolib when using hierarchical irqs.

> +       /* This will let us handle the parent IRQ in the driver */
> +       girq->parent_handler = NULL;
> +       girq->num_parents = 0;
> +       girq->parents = NULL;

You don't need this.

> +       girq->default_type = IRQ_TYPE_NONE;
> +       girq->handler = handle_level_irq;

But this stays.

> +       for (i = 0; i < num_irq; i++) {
> +               desc = irq_to_desc(priv->irq[i]);
> +               desc->status_use_accessors |= IRQ_NOAUTOEN;
> +               if (devm_request_irq(dev, priv->irq[i],
> +                                    visconti_gpio_irq_handler, 0, name, priv)) {
> +                       dev_err(dev, "failed to request IRQ[%d]\n", i);
> +                       return -ENOENT;
> +               }
> +       }

This should not be needed either when using hiearchical IRQs,
also the irqchip maintainers will beat us up for poking around in the
descs like this.

The rest looks solid!

Yours,
Linus Walleij

Re: [PATCH v4 2/4] gpio: visconti: Add Toshiba Visconti GPIO support

[Nobuhiro Iwamatsu]

Hi,

Thanks for your review.

On Sat, Dec 12, 2020 at 12:20:47AM +0100, Linus Walleij wrote:
> On Fri, Dec 11, 2020 at 1:43 AM Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp> wrote:
> 
> This iteration is looking really good, but we are not quite there yet,
> because now that the driver looks so much better I can see that it
> is a hierarchical interrupt controller.

As you pointed out, this GPIO interrupt is directly linked to the GIC
interrupt.
As a function of the GPIO driver, there is a function (IRQ_DOMAIN_HIERARCHY)
that can handle these as one-to-one, so it is pointed out that it is better
to use this. Is this correct?

> 
> > Add the GPIO driver for Toshiba Visconti ARM SoCs.
> >
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
> (...)
> 
> > +config GPIO_VISCONTI
> > +       tristate "Toshiba Visconti GPIO support"
> > +       depends on ARCH_VISCONTI || COMPILE_TEST
> > +       depends on OF_GPIO
> > +       select GPIOLIB_IRQCHIP
> > +       select GPIO_GENERIC
> > +       help
> > +         Say yes here to support GPIO on Tohisba Visconti.
> 
> Add
> select IRQ_DOMAIN_HIERARCHY

OK, I will add this.

> 
> > +struct visconti_gpio {
> > +       void __iomem *base;
> > +       int *irq;
> 
> Don't keep these irqs around.
>

OK, I will remove this
.
> > +       ret = platform_irq_count(pdev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       num_irq = ret;
> > +
> > +       priv->irq = devm_kcalloc(dev, num_irq, sizeof(priv->irq), GFP_KERNEL);
> > +       if (!priv->irq)
> > +               return -ENOMEM;
> > +
> > +       for (i = 0; i < num_irq; i++) {
> > +               priv->irq[i] = platform_get_irq(pdev, i);
> > +               if (priv->irq[i] < 0) {
> > +                       dev_err(dev, "invalid IRQ[%d]\n", i);
> > +                       return priv->irq[i];
> > +               }
> > +       }
> 
> Instead of doing this, look in for example
> drivers/gpio/gpio-ixp4xx.c
> 
> You need:
> 
> > +       girq = &priv->gpio_chip.irq;
> > +       girq->chip = irq_chip;
> 
> girq->fwnode = fwnode;
> girq->parent_domain = parent;
> girq->child_to_parent_hwirq = visconti_gpio_child_to_parent_hwirq;
> 


I understood that the irq_domain specified by girq->parent_domain will be the
GIC. Is this correct?


> The mapping function will be something like this:
> 
> static int visconti_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
>                                              unsigned int child,
>                                              unsigned int child_type,
>                                              unsigned int *parent,
>                                              unsigned int *parent_type)
> {
>         /* Interrupts 0..15 mapped to interrupts 24..39 on the GIC */
>         if (child < 16) {
>             /* All these interrupts are level high in the CPU */
>             *parent_type = IRQ_TYPE_LEVEL_HIGH;
>             *parent = child + 24;
>             return 0;
>         }
>         return -EINVAL;
> }
>

I see, I will add this function.

> > +       priv->gpio_chip.irq.init_valid_mask = visconti_init_irq_valid_mask;
> 
> This will be set up by gpiolib when using hierarchical irqs.
> 

OK.

> > +       /* This will let us handle the parent IRQ in the driver */
> > +       girq->parent_handler = NULL;
> > +       girq->num_parents = 0;
> > +       girq->parents = NULL;
> 
> You don't need this.
> 
> > +       girq->default_type = IRQ_TYPE_NONE;
> > +       girq->handler = handle_level_irq;
> 
> But this stays.
> 

OK, I will update to these.

> > +       for (i = 0; i < num_irq; i++) {
> > +               desc = irq_to_desc(priv->irq[i]);
> > +               desc->status_use_accessors |= IRQ_NOAUTOEN;
> > +               if (devm_request_irq(dev, priv->irq[i],
> > +                                    visconti_gpio_irq_handler, 0, name, priv)) {
> > +                       dev_err(dev, "failed to request IRQ[%d]\n", i);
> > +                       return -ENOENT;
> > +               }
> > +       }
> 
> This should not be needed either when using hiearchical IRQs,
> also the irqchip maintainers will beat us up for poking around in the
> descs like this.

I understand that the processing equivalent to request_irq() is processed
by the irqchip frame work (or GIC driver). Is this correct?


> 
> The rest looks solid!
> 

Thank you.
I will apply your point into the driver.

Best regards,
  Nobuhiro

Re: [PATCH v4 2/4] gpio: visconti: Add Toshiba Visconti GPIO support

[Marc Zyngier]

On 2020-12-16 09:11, Nobuhiro Iwamatsu wrote:

[...]

>> > +       for (i = 0; i < num_irq; i++) {
>> > +               desc = irq_to_desc(priv->irq[i]);
>> > +               desc->status_use_accessors |= IRQ_NOAUTOEN;
>> > +               if (devm_request_irq(dev, priv->irq[i],
>> > +                                    visconti_gpio_irq_handler, 0, name, priv)) {
>> > +                       dev_err(dev, "failed to request IRQ[%d]\n", i);
>> > +                       return -ENOENT;
>> > +               }
>> > +       }
>> 
>> This should not be needed either when using hiearchical IRQs,
>> also the irqchip maintainers will beat us up for poking around in the
>> descs like this.
> 
> I understand that the processing equivalent to request_irq() is 
> processed
> by the irqchip frame work (or GIC driver). Is this correct?

request_irq() is reserved to endpoint drivers (the driver for the device
driving the IRQ line). If this is indeed a hierarchical irqchip, the
line allocation will be driven from the GPIO framework, and 
request_irq()
will perform the activation. There isn't anything that this driver 
should
do directly other than configuring its own state and passing the request
along to the parent controller.

And yes, mucking with the irq descriptor will get you in massive 
trouble,
never do that.

Thanks,

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

Re: [PATCH v4 2/4] gpio: visconti: Add Toshiba Visconti GPIO support

[Nobuhiro Iwamatsu]

Hi,

On Wed, Dec 16, 2020 at 09:36:17AM +0000, Marc Zyngier wrote:
> On 2020-12-16 09:11, Nobuhiro Iwamatsu wrote:
> 
> [...]
> 
> > > > +       for (i = 0; i < num_irq; i++) {
> > > > +               desc = irq_to_desc(priv->irq[i]);
> > > > +               desc->status_use_accessors |= IRQ_NOAUTOEN;
> > > > +               if (devm_request_irq(dev, priv->irq[i],
> > > > +                                    visconti_gpio_irq_handler, 0, name, priv)) {
> > > > +                       dev_err(dev, "failed to request IRQ[%d]\n", i);
> > > > +                       return -ENOENT;
> > > > +               }
> > > > +       }
> > > 
> > > This should not be needed either when using hiearchical IRQs,
> > > also the irqchip maintainers will beat us up for poking around in the
> > > descs like this.
> > 
> > I understand that the processing equivalent to request_irq() is
> > processed
> > by the irqchip frame work (or GIC driver). Is this correct?
> 
> request_irq() is reserved to endpoint drivers (the driver for the device
> driving the IRQ line). If this is indeed a hierarchical irqchip, the
> line allocation will be driven from the GPIO framework, and request_irq()
> will perform the activation. There isn't anything that this driver should
> do directly other than configuring its own state and passing the request
> along to the parent controller.
> 
> And yes, mucking with the irq descriptor will get you in massive trouble,
> never do that.
> 

I see. Thank you for the explanation.
I got a better understanding.

Best regards,
  Nobuhiro

> -- 
> Jazz is not dead. It just smells funny...
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>