Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

From: Andy Shevchenko
Date: Mon Apr 26 2021 - 06:29:59 EST

Next message: Tomasz Duszynski: "Re: [PATCH 2/3] iio: sps30: add support for serial interface"
Previous message: Michal Hocko: "Re: [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation"
In reply to: Rob Herring: "Re: [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller"
Next in thread: Michael Walle: "Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
<tsbogend@xxxxxxxxxxxxxxxx> wrote:
>
> IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> to 32 gpios. All gpios could be used as an interrupt source.

Thank you!

Honestly speaking, I was about to give a tag but realized 1) we missed
v5.13 anyway, and 2) there is gpio-regmap generic code, that may be
worth considering. Otherwise this is in a pretty good shape.

My comments below.

> Signed-off-by: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
> ---
> Changes in v4:

> - added spinlock to serialize access to irq registers

I'm not sure it's enough to have separated locks for these registers
versus direction / value ones.
Can't you reuse bgpio_lock?

Looking into bgpio code, I think it has issues with locking in some
cases (it does two or more operations each of them serialized, but not
together, it means there is a window where another I/O may happen and
potentially screw up the GPIO state.

Dunno if gpio-regmap has this solved (I suggest to look into it as
well, at least regmap API provides locking by default).

> - 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 | 209 +++++++++++++++++++++++++++++++++++
> 3 files changed, 222 insertions(+)
> create mode 100644 drivers/gpio/gpio-idt3243x.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e3607ec4c2e8..90543a95dbb8 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 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 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..62e5643a0228
> --- /dev/null
> +++ b/drivers/gpio/gpio-idt3243x.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Driver for IDT/Renesas 79RC3243x Interrupt Controller */
> +

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

+ spinlock.h (but see above)

> +
> +#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;
> + spinlock_t irq_lock; /* serialize access to irq registers */
> +};
> +
> +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_EDGE_BOTH)
> + return -EINVAL;
> +
> + if (sense == 0 || sense == IRQ_TYPE_LEVEL_MASK)

0 => IRQ_TYPE_NONE

Now I have got the below exit. You need to check here for EDGE

So,

if (sense == IRQ_TYPE_NONE || (sense & IRQ_TYPE_EDGE_BOTH)

And setting LEVEL_HIGH + LEVEL_LOW shouldn't stop you here. It's fine,
just declared HIGH as a winner.

> + return -EINVAL;
> +
> + spin_lock_irqsave(&ctrl->irq_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(&ctrl->irq_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(&ctrl->irq_lock, flags);
> +
> + ctrl->mask_cache |= BIT(d->hwirq);
> + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
> +
> + spin_unlock_irqrestore(&ctrl->irq_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(&ctrl->irq_lock, flags);
> +
> + ctrl->mask_cache &= ~BIT(d->hwirq);
> + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
> +
> + spin_unlock_irqrestore(&ctrl->irq_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;
> +
> + 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 = 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;
> +
> + spin_lock_init(&ctrl->irq_lock);
> +
> + 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@xxxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 2.29.2
>

--
With Best Regards,
Andy Shevchenko

Next message: Tomasz Duszynski: "Re: [PATCH 2/3] iio: sps30: add support for serial interface"
Previous message: Michal Hocko: "Re: [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation"
In reply to: Rob Herring: "Re: [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller"
Next in thread: Michael Walle: "Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]