diff mbox series

[v5,1/2] gpio: Add support for IDT 79RC3243x GPIO controller

Message ID 20210514123309.134048-1-tsbogend@alpha.franken.de
State New
Headers show
Series [v5,1/2] gpio: Add support for IDT 79RC3243x GPIO controller | expand

Commit Message

Thomas Bogendoerfer May 14, 2021, 12:33 p.m. UTC 
IDT 79RC3243x SoCs integrated a gpio controller, which handles up
to 32 gpios. All gpios could be used as an interrupt source.

Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
Changes in v5:
 - use bgpio spinlock
 - made interrupt controller optional
 - made ngpios optional

Changes in v4:
 - added spinlock to serialize access to irq registers
 - reworked checking of irq sense bits
 - start with handle_bad_irq and set handle_level_irq in idt_gpio_irq_set_type
 - cleaned up #includes
 - use platform_get_irq

Changes in v3:
 - changed compatible string to idt,32434-gpio
 - registers now start with gpio direction register and leaves
   out alternate function register for pinmux/pinctrl driver

Changes in v2:
 - made driver buildable as module
 - use for_each_set_bit() in irq dispatch handler
 - use gpiochip_get_data instead of own container_of helper
 - use module_platform_driver() instead of arch_initcall
 - don't default y for Mikrotik RB532

 drivers/gpio/Kconfig         |  12 ++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-idt3243x.c | 206 +++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)
 create mode 100644 drivers/gpio/gpio-idt3243x.c

Comments

Linus Walleij May 18, 2021, 11:50 p.m. UTC | #1 
Hi Thomas,

thanks for your patch!

I can see this is starting to look really good.

There is one thing that confuses me:

On Fri, May 14, 2021 at 2:33 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:

> IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> to 32 gpios. All gpios could be used as an interrupt source.
>
> Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> ---
> Changes in v5:
(...)

> +static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type)
> +{
(...)
> +       /* hardware only supports level triggered */
> +       if (sense == IRQ_TYPE_NONE || (sense & IRQ_TYPE_EDGE_BOTH))
> +               return -EINVAL;
(...)
> +       irq_set_handler_locked(d, handle_level_irq);

But:

> +static void idt_gpio_ack(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +
> +       writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT);
> +}
(...)
> +       .irq_ack = idt_gpio_ack,

Correct me if I'm wrong but I thing .irq_ack() is only called
from handle_edge_irq ... so never in this case.

Can this ACK just be deleted?

The code in the ACK callback also looks really weird:
write all bits except for the current IRQ into the status
register? It's usually the other way around with these
things. That really makes me suspect it is unused.

Yours,
Linus Walleij
Thomas Bogendoerfer May 19, 2021, 6:20 a.m. UTC | #2 
On Wed, May 19, 2021 at 01:50:39AM +0200, Linus Walleij wrote:
> Hi Thomas,
> 
> thanks for your patch!
> 
> I can see this is starting to look really good.
> 
> There is one thing that confuses me:
> 
> On Fri, May 14, 2021 at 2:33 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> 
> > IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> > to 32 gpios. All gpios could be used as an interrupt source.
> >
> > Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > ---
> > Changes in v5:
> (...)
> 
> > +static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type)
> > +{
> (...)
> > +       /* hardware only supports level triggered */
> > +       if (sense == IRQ_TYPE_NONE || (sense & IRQ_TYPE_EDGE_BOTH))
> > +               return -EINVAL;
> (...)
> > +       irq_set_handler_locked(d, handle_level_irq);
> 
> But:
> 
> > +static void idt_gpio_ack(struct irq_data *d)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +       struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> > +
> > +       writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT);
> > +}
> (...)
> > +       .irq_ack = idt_gpio_ack,
> 
> Correct me if I'm wrong but I thing .irq_ack() is only called
> from handle_edge_irq ... so never in this case.

handle_level_irq() does a mask_ack_irq() and this uses mask_irq() and
desc->irq_data.chip->irq_ack(), if there is no irq_mask_ack function.

> Can this ACK just be deleted?

no without it interrupts won't go away.

> The code in the ACK callback also looks really weird:
> write all bits except for the current IRQ into the status
> register? It's usually the other way around with these
> things. That really makes me suspect it is unused.

interrupts are acked by writing a 0 to the bit position. I know it's
unusal...

Thomas.
Thomas Bogendoerfer June 4, 2021, 12:22 p.m. UTC | #3 
On Fri, May 14, 2021 at 02:33:07PM +0200, Thomas Bogendoerfer wrote:
> IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> to 32 gpios. All gpios could be used as an interrupt source.
> 
> Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> ---
> Changes in v5:
>  - use bgpio spinlock
>  - made interrupt controller optional
>  - made ngpios optional
> 
> Changes in v4:
>  - added spinlock to serialize access to irq registers
>  - reworked checking of irq sense bits
>  - start with handle_bad_irq and set handle_level_irq in idt_gpio_irq_set_type
>  - cleaned up #includes
>  - use platform_get_irq
> 
> Changes in v3:
>  - changed compatible string to idt,32434-gpio
>  - registers now start with gpio direction register and leaves
>    out alternate function register for pinmux/pinctrl driver
> 
> Changes in v2:
>  - made driver buildable as module
>  - use for_each_set_bit() in irq dispatch handler
>  - use gpiochip_get_data instead of own container_of helper
>  - use module_platform_driver() instead of arch_initcall
>  - don't default y for Mikrotik RB532
> 
>  drivers/gpio/Kconfig         |  12 ++
>  drivers/gpio/Makefile        |   1 +
>  drivers/gpio/gpio-idt3243x.c | 206 +++++++++++++++++++++++++++++++++++
>  3 files changed, 219 insertions(+)
>  create mode 100644 drivers/gpio/gpio-idt3243x.c
> [...]

is there anything a still need to do to get this integrated for v5.14 ?

Thomas.
Linus Walleij June 4, 2021, 10:03 p.m. UTC | #4 
On Fri, Jun 4, 2021 at 2:22 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:

> is there anything a still need to do to get this integrated for v5.14 ?

IMO not really:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Bartosz is collecting the patches for v5.14.

Yours,
Linus Walleij
Bartosz Golaszewski June 7, 2021, 2 p.m. UTC | #5 
On Sat, Jun 5, 2021 at 12:04 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Jun 4, 2021 at 2:22 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
>
> > is there anything a still need to do to get this integrated for v5.14 ?
>
> IMO not really:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Bartosz is collecting the patches for v5.14.
>
> Yours,
> Linus Walleij

Sorry for the delay, I'm currently overwhelmed with a house renovation
and the approaching move. Now applied (together with the bindings),
thanks!

Bartosz
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 1dd0ec6727fd..ae2721967191 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -782,6 +782,18 @@  config GPIO_MSC313
 	  Say Y here to support the main GPIO block on MStar/SigmaStar
 	  ARMv7 based SoCs.
 
+config GPIO_IDT3243X
+	tristate "IDT 79RC3243X GPIO support"
+	depends on MIKROTIK_RB532 || COMPILE_TEST
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	help
+	  Select this option to enable GPIO driver for
+	  IDT 79RC3243X based devices like Mikrotik RB532.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gpio-idt3243x.
+
 endmenu
 
 menu "Port-mapped I/O GPIO drivers"
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d7c81e1611a4..32a32659866a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -68,6 +68,7 @@  obj-$(CONFIG_GPIO_HISI)                 += gpio-hisi.o
 obj-$(CONFIG_GPIO_HLWD)			+= gpio-hlwd.o
 obj-$(CONFIG_HTC_EGPIO)			+= gpio-htc-egpio.o
 obj-$(CONFIG_GPIO_ICH)			+= gpio-ich.o
+obj-$(CONFIG_GPIO_IDT3243X)		+= gpio-idt3243x.o
 obj-$(CONFIG_GPIO_IOP)			+= gpio-iop.o
 obj-$(CONFIG_GPIO_IT87)			+= gpio-it87.o
 obj-$(CONFIG_GPIO_IXP4XX)		+= gpio-ixp4xx.o
diff --git a/drivers/gpio/gpio-idt3243x.c b/drivers/gpio/gpio-idt3243x.c
new file mode 100644
index 000000000000..e961acee1571
--- /dev/null
+++ b/drivers/gpio/gpio-idt3243x.c
@@ -0,0 +1,206 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for IDT/Renesas 79RC3243x Interrupt Controller  */
+
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define IDT_PIC_IRQ_PEND	0x00
+#define IDT_PIC_IRQ_MASK	0x08
+
+#define IDT_GPIO_DIR		0x00
+#define IDT_GPIO_DATA		0x04
+#define IDT_GPIO_ILEVEL		0x08
+#define IDT_GPIO_ISTAT		0x0C
+
+struct idt_gpio_ctrl {
+	struct gpio_chip gc;
+	void __iomem *pic;
+	void __iomem *gpio;
+	u32 mask_cache;
+};
+
+static void idt_gpio_dispatch(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	struct irq_chip *host_chip = irq_desc_get_chip(desc);
+	unsigned int bit, virq;
+	unsigned long pending;
+
+	chained_irq_enter(host_chip, desc);
+
+	pending = readl(ctrl->pic + IDT_PIC_IRQ_PEND);
+	pending &= ~ctrl->mask_cache;
+	for_each_set_bit(bit, &pending, gc->ngpio) {
+		virq = irq_linear_revmap(gc->irq.domain, bit);
+		if (virq)
+			generic_handle_irq(virq);
+	}
+
+	chained_irq_exit(host_chip, desc);
+}
+
+static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	unsigned int sense = flow_type & IRQ_TYPE_SENSE_MASK;
+	unsigned long flags;
+	u32 ilevel;
+
+	/* hardware only supports level triggered */
+	if (sense == IRQ_TYPE_NONE || (sense & IRQ_TYPE_EDGE_BOTH))
+		return -EINVAL;
+
+	spin_lock_irqsave(&gc->bgpio_lock, flags);
+
+	ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL);
+	if (sense & IRQ_TYPE_LEVEL_HIGH)
+		ilevel |= BIT(d->hwirq);
+	else if (sense & IRQ_TYPE_LEVEL_LOW)
+		ilevel &= ~BIT(d->hwirq);
+
+	writel(ilevel, ctrl->gpio + IDT_GPIO_ILEVEL);
+	irq_set_handler_locked(d, handle_level_irq);
+
+	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
+	return 0;
+}
+
+static void idt_gpio_ack(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+
+	writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT);
+}
+
+static void idt_gpio_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&gc->bgpio_lock, flags);
+
+	ctrl->mask_cache |= BIT(d->hwirq);
+	writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+
+	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
+}
+
+static void idt_gpio_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&gc->bgpio_lock, flags);
+
+	ctrl->mask_cache &= ~BIT(d->hwirq);
+	writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+
+	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
+}
+
+static int idt_gpio_irq_init_hw(struct gpio_chip *gc)
+{
+	struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+
+	/* Mask interrupts. */
+	ctrl->mask_cache = 0xffffffff;
+	writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+
+	return 0;
+}
+
+static struct irq_chip idt_gpio_irqchip = {
+	.name = "IDTGPIO",
+	.irq_mask = idt_gpio_mask,
+	.irq_ack = idt_gpio_ack,
+	.irq_unmask = idt_gpio_unmask,
+	.irq_set_type = idt_gpio_irq_set_type
+};
+
+static int idt_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *girq;
+	struct idt_gpio_ctrl *ctrl;
+	unsigned int parent_irq;
+	int ngpios;
+	int ret;
+
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	ctrl->gpio = devm_platform_ioremap_resource_byname(pdev, "gpio");
+	if (!ctrl->gpio)
+		return -ENOMEM;
+
+	ctrl->gc.parent = dev;
+
+	ret = bgpio_init(&ctrl->gc, &pdev->dev, 4, ctrl->gpio + IDT_GPIO_DATA,
+			 NULL, NULL, ctrl->gpio + IDT_GPIO_DIR, NULL, 0);
+	if (ret) {
+		dev_err(dev, "bgpio_init failed\n");
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev, "ngpios", &ngpios);
+	if (!ret)
+		ctrl->gc.ngpio = ngpios;
+
+	if (device_property_read_bool(dev, "interrupt-controller")) {
+		ctrl->pic = devm_platform_ioremap_resource_byname(pdev, "pic");
+		if (!ctrl->pic)
+			return -ENOMEM;
+
+		parent_irq = platform_get_irq(pdev, 0);
+		if (!parent_irq)
+			return -EINVAL;
+
+		girq = &ctrl->gc.irq;
+		girq->chip = &idt_gpio_irqchip;
+		girq->init_hw = idt_gpio_irq_init_hw;
+		girq->parent_handler = idt_gpio_dispatch;
+		girq->num_parents = 1;
+		girq->parents = devm_kcalloc(dev, girq->num_parents,
+					     sizeof(*girq->parents),
+					     GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+
+		girq->parents[0] = parent_irq;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_bad_irq;
+	}
+
+	return devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl);
+}
+
+static const struct of_device_id idt_gpio_of_match[] = {
+	{ .compatible = "idt,32434-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, idt_gpio_of_match);
+
+static struct platform_driver idt_gpio_driver = {
+	.probe = idt_gpio_probe,
+	.driver = {
+		.name = "idt3243x-gpio",
+		.of_match_table = idt_gpio_of_match,
+	},
+};
+module_platform_driver(idt_gpio_driver);
+
+MODULE_DESCRIPTION("IDT 79RC3243x GPIO/PIC Driver");
+MODULE_AUTHOR("Thomas Bogendoerfer <tsbogend@alpha.franken.de>");
+MODULE_LICENSE("GPL");