| Message ID | 20210426095426.118356-1-tsbogend@alpha.franken.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] gpio: Add support for IDT 79RC3243x GPIO controller | expand |
On Mon, Apr 26, 2021 at 12:55 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. 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@alpha.franken.de> > --- > 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@alpha.franken.de>"); > +MODULE_LICENSE("GPL"); > -- > 2.29.2 >
Hi, Am 2021-04-26 12:29, schrieb Andy Shevchenko: > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > <tsbogend@alpha.franken.de> wrote: > > 2) there is gpio-regmap generic code, that may be worth > considering. This driver uses memory mapped registers. While that is also possible with gpio-regmap, there is one drawback: it assumes gpiochip->can_sleep = true for now, see [1]. Unfortunately, there is no easy way to ask the regmap if its mmio/fastio. -michael [1] https://elixir.bootlin.com/linux/v5.12/source/drivers/gpio/gpio-regmap.c#L257
On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: > Am 2021-04-26 12:29, schrieb Andy Shevchenko: > > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > > <tsbogend@alpha.franken.de> wrote: > > > > 2) there is gpio-regmap generic code, that may be worth > > considering. > > This driver uses memory mapped registers. While that is > also possible with gpio-regmap, there is one drawback: > it assumes gpiochip->can_sleep = true for now, see [1]. > Unfortunately, there is no easy way to ask the regmap > if its mmio/fastio. I don't see how it is an impediment. Prerequisite patch? > [1] > https://elixir.bootlin.com/linux/v5.12/source/drivers/gpio/gpio-regmap.c#L257
Am 2021-04-28 13:07, schrieb Andy Shevchenko: > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer >> > <tsbogend@alpha.franken.de> wrote: >> > >> > 2) there is gpio-regmap generic code, that may be worth >> > considering. >> >> This driver uses memory mapped registers. While that is >> also possible with gpio-regmap, there is one drawback: >> it assumes gpiochip->can_sleep = true for now, see [1]. >> Unfortunately, there is no easy way to ask the regmap >> if its mmio/fastio. > > I don't see how it is an impediment. You'd have to use the *_cansleep() variants with the gpios, which cannot be used everywhere, no? -michael
On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: > > Am 2021-04-28 13:07, schrieb Andy Shevchenko: > > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: > >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: > >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > >> > <tsbogend@alpha.franken.de> wrote: > >> > > >> > 2) there is gpio-regmap generic code, that may be worth > >> > considering. > >> > >> This driver uses memory mapped registers. While that is > >> also possible with gpio-regmap, there is one drawback: > >> it assumes gpiochip->can_sleep = true for now, see [1]. > >> Unfortunately, there is no easy way to ask the regmap > >> if its mmio/fastio. > > > > I don't see how it is an impediment. > > You'd have to use the *_cansleep() variants with the gpios, > which cannot be used everywhere, no? *can* sleep means that it requires a sleeping context to run, if your controller is fine with that, there are no worries. OTOH if you want to run this in an atomic context, then consumers can't do with that kind of controller. What I meant above (and you stripped it here) is to add a patch that will fix that and set it based on gpio_regmap_config.
Am 2021-04-28 15:44, schrieb Andy Shevchenko: > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer >> >> > <tsbogend@alpha.franken.de> wrote: >> >> > >> >> > 2) there is gpio-regmap generic code, that may be worth >> >> > considering. >> >> >> >> This driver uses memory mapped registers. While that is >> >> also possible with gpio-regmap, there is one drawback: >> >> it assumes gpiochip->can_sleep = true for now, see [1]. >> >> Unfortunately, there is no easy way to ask the regmap >> >> if its mmio/fastio. >> > >> > I don't see how it is an impediment. >> >> You'd have to use the *_cansleep() variants with the gpios, >> which cannot be used everywhere, no? > > *can* sleep means that it requires a sleeping context to run, if your > controller is fine with that, there are no worries. OTOH if you want > to run this in an atomic context, then consumers can't do with that > kind of controller. Ok, then we are on the same track. > What I meant above (and you stripped it here) is > to add a patch that will fix that and set it based on > gpio_regmap_config. Yes, but ideally, it would ask the regmap. Otherwise that information is redundant and might mismatch, i.e. gpio_regmap_config tell can_sleep=false but the regmap is an I2C type for example. Also if a driver wants to support both regmap types, we are no step further. -michael
On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <michael@walle.cc> wrote: > Am 2021-04-28 15:44, schrieb Andy Shevchenko: > > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: > >> > >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: > >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: > >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: > >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > >> >> > <tsbogend@alpha.franken.de> wrote: > >> >> > > >> >> > 2) there is gpio-regmap generic code, that may be worth > >> >> > considering. > >> >> > >> >> This driver uses memory mapped registers. While that is > >> >> also possible with gpio-regmap, there is one drawback: > >> >> it assumes gpiochip->can_sleep = true for now, see [1]. > >> >> Unfortunately, there is no easy way to ask the regmap > >> >> if its mmio/fastio. > >> > > >> > I don't see how it is an impediment. > >> > >> You'd have to use the *_cansleep() variants with the gpios, > >> which cannot be used everywhere, no? > > > > *can* sleep means that it requires a sleeping context to run, if your > > controller is fine with that, there are no worries. OTOH if you want > > to run this in an atomic context, then consumers can't do with that > > kind of controller. > > Ok, then we are on the same track. > > > What I meant above (and you stripped it here) is > > to add a patch that will fix that and set it based on > > gpio_regmap_config. > > Yes, but ideally, it would ask the regmap. Otherwise that > information is redundant and might mismatch, i.e. gpio_regmap_config > tell can_sleep=false but the regmap is an I2C type for example. Also > if a driver wants to support both regmap types, we are no step > further. Yeah, I agree that is a band aid, but you are free to fix it actually on regmap level. I don't think it will require an enormous amount of work there. We have time :-)
[Adding Mark here, too] Am 2021-04-28 16:32, schrieb Andy Shevchenko: > On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <michael@walle.cc> wrote: >> Am 2021-04-28 15:44, schrieb Andy Shevchenko: >> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: >> >> >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: >> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: >> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: >> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer >> >> >> > <tsbogend@alpha.franken.de> wrote: >> >> >> > >> >> >> > 2) there is gpio-regmap generic code, that may be worth >> >> >> > considering. >> >> >> >> >> >> This driver uses memory mapped registers. While that is >> >> >> also possible with gpio-regmap, there is one drawback: >> >> >> it assumes gpiochip->can_sleep = true for now, see [1]. >> >> >> Unfortunately, there is no easy way to ask the regmap >> >> >> if its mmio/fastio. >> >> > >> >> > I don't see how it is an impediment. >> >> >> >> You'd have to use the *_cansleep() variants with the gpios, >> >> which cannot be used everywhere, no? >> > >> > *can* sleep means that it requires a sleeping context to run, if your >> > controller is fine with that, there are no worries. OTOH if you want >> > to run this in an atomic context, then consumers can't do with that >> > kind of controller. >> >> Ok, then we are on the same track. >> >> > What I meant above (and you stripped it here) is >> > to add a patch that will fix that and set it based on >> > gpio_regmap_config. >> >> Yes, but ideally, it would ask the regmap. Otherwise that >> information is redundant and might mismatch, i.e. gpio_regmap_config >> tell can_sleep=false but the regmap is an I2C type for example. Also >> if a driver wants to support both regmap types, we are no step >> further. > > Yeah, I agree that is a band aid, but you are free to fix it actually > on regmap level. > I don't think it will require an enormous amount of work there. I'd love to fix that, but Mark was against exposing that property outside of regmap. So it it what it is for now ;) Maybe he'll change his mind or someone has another idea. -michael
On Wed, Apr 28, 2021 at 5:48 PM Michael Walle <michael@walle.cc> wrote: > > [Adding Mark here, too] > > Am 2021-04-28 16:32, schrieb Andy Shevchenko: > > On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <michael@walle.cc> wrote: > >> Am 2021-04-28 15:44, schrieb Andy Shevchenko: > >> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: > >> >> > >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: > >> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: > >> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: > >> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > >> >> >> > <tsbogend@alpha.franken.de> wrote: > >> >> >> > > >> >> >> > 2) there is gpio-regmap generic code, that may be worth > >> >> >> > considering. > >> >> >> > >> >> >> This driver uses memory mapped registers. While that is > >> >> >> also possible with gpio-regmap, there is one drawback: > >> >> >> it assumes gpiochip->can_sleep = true for now, see [1]. > >> >> >> Unfortunately, there is no easy way to ask the regmap > >> >> >> if its mmio/fastio. > >> >> > > >> >> > I don't see how it is an impediment. > >> >> > >> >> You'd have to use the *_cansleep() variants with the gpios, > >> >> which cannot be used everywhere, no? > >> > > >> > *can* sleep means that it requires a sleeping context to run, if your > >> > controller is fine with that, there are no worries. OTOH if you want > >> > to run this in an atomic context, then consumers can't do with that > >> > kind of controller. > >> > >> Ok, then we are on the same track. > >> > >> > What I meant above (and you stripped it here) is > >> > to add a patch that will fix that and set it based on > >> > gpio_regmap_config. > >> > >> Yes, but ideally, it would ask the regmap. Otherwise that > >> information is redundant and might mismatch, i.e. gpio_regmap_config > >> tell can_sleep=false but the regmap is an I2C type for example. Also > >> if a driver wants to support both regmap types, we are no step > >> further. > > > > Yeah, I agree that is a band aid, but you are free to fix it actually > > on regmap level. > > I don't think it will require an enormous amount of work there. > > I'd love to fix that, but Mark was against exposing that property > outside of regmap. So it it what it is for now ;) Maybe he'll change > his mind or someone has another idea. Then let's go to ugly variant with duplicating it in gpio-regmap config. with a FIXME note or so. I don't think we should allow new drivers be based on bgpio_init().
Am 2021-04-28 17:02, schrieb Andy Shevchenko: > On Wed, Apr 28, 2021 at 5:48 PM Michael Walle <michael@walle.cc> wrote: >> >> [Adding Mark here, too] >> >> Am 2021-04-28 16:32, schrieb Andy Shevchenko: >> > On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <michael@walle.cc> wrote: >> >> Am 2021-04-28 15:44, schrieb Andy Shevchenko: >> >> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: >> >> >> >> >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: >> >> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: >> >> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: >> >> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer >> >> >> >> > <tsbogend@alpha.franken.de> wrote: >> >> >> >> > >> >> >> >> > 2) there is gpio-regmap generic code, that may be worth >> >> >> >> > considering. >> >> >> >> >> >> >> >> This driver uses memory mapped registers. While that is >> >> >> >> also possible with gpio-regmap, there is one drawback: >> >> >> >> it assumes gpiochip->can_sleep = true for now, see [1]. >> >> >> >> Unfortunately, there is no easy way to ask the regmap >> >> >> >> if its mmio/fastio. >> >> >> > >> >> >> > I don't see how it is an impediment. >> >> >> >> >> >> You'd have to use the *_cansleep() variants with the gpios, >> >> >> which cannot be used everywhere, no? >> >> > >> >> > *can* sleep means that it requires a sleeping context to run, if your >> >> > controller is fine with that, there are no worries. OTOH if you want >> >> > to run this in an atomic context, then consumers can't do with that >> >> > kind of controller. >> >> >> >> Ok, then we are on the same track. >> >> >> >> > What I meant above (and you stripped it here) is >> >> > to add a patch that will fix that and set it based on >> >> > gpio_regmap_config. >> >> >> >> Yes, but ideally, it would ask the regmap. Otherwise that >> >> information is redundant and might mismatch, i.e. gpio_regmap_config >> >> tell can_sleep=false but the regmap is an I2C type for example. Also >> >> if a driver wants to support both regmap types, we are no step >> >> further. >> > >> > Yeah, I agree that is a band aid, but you are free to fix it actually >> > on regmap level. >> > I don't think it will require an enormous amount of work there. >> >> I'd love to fix that, but Mark was against exposing that property >> outside of regmap. So it it what it is for now ;) Maybe he'll change >> his mind or someone has another idea. > > Then let's go to ugly variant with duplicating it in gpio-regmap > config. with a FIXME note or so. I don't think we should allow new > drivers be based on bgpio_init(). Agreed, given that a possible fix should be easy enough later. -michael
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 */ + +#include <linux/gpio/driver.h> +#include <linux/irq.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/platform_device.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; + 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) + 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@alpha.franken.de>"); +MODULE_LICENSE("GPL");
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 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 | 209 +++++++++++++++++++++++++++++++++++ 3 files changed, 222 insertions(+) create mode 100644 drivers/gpio/gpio-idt3243x.c