diff mbox series

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

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

Commit Message

Thomas Bogendoerfer April 22, 2021, 3:20 p.m. UTC 
IDT 79RC3243x SoCs integrated a gpio controller, which handles up
to 32 gpios. All gpios could be used as interrupt source.

Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
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 | 198 +++++++++++++++++++++++++++++++++++
 3 files changed, 211 insertions(+)
 create mode 100644 drivers/gpio/gpio-idt3243x.c

Comments

Thomas Bogendoerfer April 24, 2021, 10:35 a.m. UTC | #1 
On Fri, Apr 23, 2021 at 06:37:41PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 22, 2021 at 6:21 PM Thomas Bogendoerfer

> <tsbogend@alpha.franken.de> wrote:

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

> 

> Is it guaranteed to be linear always?


yes

> > +               if (virq)

> > +                       generic_handle_irq(virq);

> > +       }

> > +

> > +       chained_irq_exit(host_chip, desc);

> > +}

> 

> ...

> 

> > +       if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))

> 

> There is a _BOTH variant.


that's IRQ_TYPE_EDGE_BOTH. LEVEL_BOTH would be an interesing concept.

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

> 

> > +       else

> > +               return -EINVAL;

> 

> Is it a double check of the above?


no, the above test is for anything not LEVEL and this now takes care
to be at least LEVEL_LOW or LEVEL_HIGH. This doesn't check for LOW|HIGH,
which I assumed nobody tries to set...

> > +       ctrl->gc.parent = dev;

> 

> Wondering if it's already done by GPIO library.


no it uses it:

        if (gc->parent) {
                gdev->dev.parent = gc->parent;
                gdev->dev.of_node = gc->parent->of_node;
        }

> ...

> 

> > +       ctrl->gc.ngpio = ngpios;

> 

> Shouldn't you do this before calling for bgpio_init()?


no, bgpio_init() initializes ngpios to size of register width, which is
32 for this hardware. And this statement restricts it to the real available
number of gpios.

> ...

> 

> > +       parent_irq = irq_of_parse_and_map(pdev->dev.of_node, 0);

> 

> platform_get_irq() ?..


yes, looks better :-)

> > +       /* Mask interrupts. */

> > +       ctrl->mask_cache = 0xffffffff;

> > +       writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);

> 

> What about using ->init_hw() call back?


sure, doesn't look like it's worth the effort, but I changed it.

> > +       girq->handler = handle_level_irq;

> 

> handle_bad_irq()


the hardware only supports level interrupts. That's also why there is
no handler change in idt_gpio_irq_set_type.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]
Andy Shevchenko April 24, 2021, 11:35 a.m. UTC | #2 
On Sat, Apr 24, 2021 at 1:47 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
> On Fri, Apr 23, 2021 at 06:37:41PM +0300, Andy Shevchenko wrote:

> > On Thu, Apr 22, 2021 at 6:21 PM Thomas Bogendoerfer

> > <tsbogend@alpha.franken.de> wrote:


...

> > > +               virq = irq_linear_revmap(gc->irq.domain, bit);

> >

> > Is it guaranteed to be linear always?

>

> yes


OK.

...

> > > +       if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))

> >

> > There is a _BOTH variant.

>

> that's IRQ_TYPE_EDGE_BOTH. LEVEL_BOTH would be an interesing concept.


Sorry, I meant _MASK in case of level. No need to open code the
existing definition.

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

> >

> > > +       else

> > > +               return -EINVAL;

> >

> > Is it a double check of the above?

>

> no, the above test is for anything not LEVEL and this now takes care

> to be at least LEVEL_LOW or LEVEL_HIGH. This doesn't check for LOW|HIGH,

> which I assumed nobody tries to set...


And? Seems you have it as a dead code.
In your case HIGH is the winner anyway.

...

> > > +       /* Mask interrupts. */

> > > +       ctrl->mask_cache = 0xffffffff;

> > > +       writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);

> >

> > What about using ->init_hw() call back?

>

> sure, doesn't look like it's worth the effort, but I changed it.


The problem here (which you may not notice from day 1) is the
ordering. We carefully put the ->init_hw() call in the proper place
and time.

...

> > > +       girq->handler = handle_level_irq;

> >

> > handle_bad_irq()

>

> the hardware only supports level interrupts. That's also why there is

> no handler change in idt_gpio_irq_set_type.


After I fixed a nasty issue in the pca953x related to Intel Galileo
platform, I may tell that setting the handler here is equal to putting
a mine for the future blown. When you set it in BAD here it will
reveal issues, if any, sooner than later.

-- 
With Best Regards,
Andy Shevchenko
Thomas Bogendoerfer April 26, 2021, 9:33 a.m. UTC | #3 
On Fri, Apr 23, 2021 at 06:56:23PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 22, 2021 at 6:21 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 interrupt source.

> 

> One missed question: No I/O serialization in the entire driver? Is it

> taken care of by bgpio wrapper or ...?


for gpios bggpio takes care of it, for irqs I've added them in coming v4.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..2948eb4ab8a5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -770,6 +770,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 SoC devices.
+
+	  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 c58a90a3c3b1..75dd9c5665c5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -67,6 +67,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..1e609bc4d839
--- /dev/null
+++ b/drivers/gpio/gpio-idt3243x.c
@@ -0,0 +1,198 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for IDT/Renesas 79RC3243x Interrupt Controller.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.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;
+	u32 ilevel;
+
+	if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+		return -EINVAL;
+
+	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);
+	else
+		return -EINVAL;
+
+	writel(ilevel, ctrl->gpio + IDT_GPIO_ILEVEL);
+	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);
+
+	ctrl->mask_cache |= BIT(d->hwirq);
+	writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+}
+
+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);
+
+	ctrl->mask_cache &= ~BIT(d->hwirq);
+	writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+}
+
+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;
+
+	ret = device_property_read_u32(dev, "ngpios", &ngpios);
+	if (ret) {
+		dev_err(dev, "ngpios property is not valid\n");
+		return 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;
+	}
+	ctrl->gc.ngpio = ngpios;
+
+	ctrl->pic = devm_platform_ioremap_resource_byname(pdev, "pic");
+	if (!ctrl->pic)
+		return -ENOMEM;
+
+	parent_irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (!parent_irq) {
+		dev_err(&pdev->dev, "Failed to map parent IRQ!\n");
+		return -EINVAL;
+	}
+
+	/* Mask interrupts. */
+	ctrl->mask_cache = 0xffffffff;
+	writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+
+	girq = &ctrl->gc.irq;
+	girq->chip = &idt_gpio_irqchip;
+	girq->parent_handler = idt_gpio_dispatch;
+	girq->num_parents = 1;
+	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
+				     GFP_KERNEL);
+	if (!girq->parents) {
+		ret = -ENOMEM;
+		goto out_unmap_irq;
+	}
+	girq->parents[0] = parent_irq;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl);
+	if (ret)
+		goto out_unmap_irq;
+
+	return 0;
+
+out_unmap_irq:
+	irq_dispose_mapping(parent_irq);
+	return ret;
+}
+
+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");