| Message ID | 20201211094138.2863677-1-nobuhiro1.iwamatsu@toshiba.co.jp |
|---|---|
| Headers | show |
| Series | gpio: visconti: Add Toshiba Visconti GPIO support | expand |
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
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
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...
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 >
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