2020-12-11 10:43:07

by Nobuhiro Iwamatsu

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

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


2020-12-11 10:43:24

by Nobuhiro Iwamatsu

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

Add bindings for the Toshiba Visconti GPIO Controller.

Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Punit Agrawal <[email protected]>
---
.../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 <[email protected]>
+
+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

2020-12-11 16:02:47

by Nobuhiro Iwamatsu

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

Add the GPIO driver for Toshiba Visconti ARM SoCs.

Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
Reviewed-by: Punit Agrawal <[email protected]>
---
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 <[email protected]>
+ */
+
+#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 <[email protected]>");
+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

2020-12-13 05:45:34

by Linus Walleij

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

Looping in Marc here:

On Fri, Dec 11, 2020 at 1:43 AM Nobuhiro Iwamatsu
<[email protected]> wrote:

> Add bindings for the Toshiba Visconti GPIO Controller.
>
> Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> Reviewed-by: Punit Agrawal <[email protected]>
(...)
> + 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

2020-12-13 05:48:36

by Linus Walleij

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

On Fri, Dec 11, 2020 at 1:43 AM Nobuhiro Iwamatsu
<[email protected]> 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 <[email protected]>
> Reviewed-by: Punit Agrawal <[email protected]>
(...)

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

2020-12-17 05:14:18

by Nobuhiro Iwamatsu

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

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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>