| Message ID | 20190203214205.13594-6-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: ixp4xx: Modernize and DT support | expand |
niedz., 3 lut 2019 o 22:42 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > This adds a driver for the IXP4xx GPIO block found in > the Intel XScale IXP4xx systems. > > The GPIO part of this block is pretty straight-forward and > just uses the generic MMIO GPIO library. > > The irqchip side of this driver is hierarchical where > the main irqchip will receive a processed level trigger > in response to the edge detector of the GPIO block, > so for this reason the v2 version of the irqdomain API > is used (as well as in the parent IXP4xx irqchip) and > masking, unmasking and setting up the type on IRQ > happens on several levels. > > Currently this GPIO controller will grab the parent > irqdomain using a special function, but as the platform > move toward device tree probing, this will not be needed: > we can just look up the parent irqdomain from the device > tree. > > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > Bartosz: looking for your ACK on this, it'd be good if > the other GPIO maintainer is aligned with my ideas here. > I intend to merge this through the ARM SoC tree. > --- Linus, My remarks are below. Mostly just nits (feel free to ignore) or questions that I'd like clarified. > MAINTAINERS | 1 + > drivers/gpio/Kconfig | 12 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-ixp4xx.c | 443 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 457 insertions(+) > create mode 100644 drivers/gpio/gpio-ixp4xx.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0d48faa3e635..6c161bd82238 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1651,6 +1651,7 @@ M: Krzysztof Halasa <khalasa@piap.pl> > L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) > S: Maintained > F: arch/arm/mach-ixp4xx/ > +F: drivers/gpio/gpio-ixp4xx.c > F: drivers/irqchip/irq-ixp4xx.c > F: include/linux/irqchip/irq-ixp4xx.h > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 699a8118c433..fe4a47e49a24 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -286,6 +286,18 @@ config GPIO_IOP > > If unsure, say N. > > +config GPIO_IXP4XX > + bool "Intel IXP4xx GPIO" > + depends on ARCH_IXP4XX || COMPILE_TEST > + select GPIO_GENERIC > + select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > + help > + Say yes here to support the GPIO functionality of a number of Intel > + IXP4xx series of chips. > + > + If unsure, say N. > + > config GPIO_LOONGSON > bool "Loongson-2/3 GPIO support" > depends on CPU_LOONGSON2 || CPU_LOONGSON3 > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index 0568bbe6fe68..cf66523b5eec 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -60,6 +60,7 @@ 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_IOP) += gpio-iop.o > +obj-$(CONFIG_GPIO_IXP4XX) += gpio-ixp4xx.o > obj-$(CONFIG_GPIO_IT87) += gpio-it87.o > obj-$(CONFIG_GPIO_JANZ_TTL) += gpio-janz-ttl.o > obj-$(CONFIG_GPIO_KEMPLD) += gpio-kempld.o > diff --git a/drivers/gpio/gpio-ixp4xx.c b/drivers/gpio/gpio-ixp4xx.c > new file mode 100644 > index 000000000000..44c24948379d > --- /dev/null > +++ b/drivers/gpio/gpio-ixp4xx.c > @@ -0,0 +1,443 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * IXP4 GPIO driver > + * Copyright (C) 2019 Linus Walleij <linus.walleij@linaro.org> > + * > + * based on previous work and know-how from: > + * Deepak Saxena <dsaxena@plexity.net> > + */ Just a nit, but I'd add a newline between the includes and the header. Also: C++ style comment for the header. > +#include <linux/gpio/driver.h> > +#include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/irqchip.h> > +#include <linux/platform_device.h> > +#include <linux/bitops.h> > +/* Include that go away with DT transition */ > +#include <linux/irqchip/irq-ixp4xx.h> I think a newline between linux/ and asm/ includes is a good rule. > +#include <asm/mach-types.h> > + > +#define IXP4XX_GPIO_GPOUTR 0x00 > +#define IXP4XX_GPIO_GPOER 0x04 > +#define IXP4XX_GPIO_GPINR 0x08 > +#define IXP4XX_GPIO_GPISR 0x0C > +#define IXP4XX_GPIO_GPIT1R 0x10 > +#define IXP4XX_GPIO_GPIT2R 0x14 > +#define IXP4XX_GPIO_GPCLKR 0x18 > +#define IXP4XX_GPIO_GPDBSELR 0x1C > + Since these are registers offsets - maybe the prefix could be IXP4XX_GPIO_REG_* or IXP4XX_GPIO_OFF_*? I think it's more readable. > +/* > + * The hardware uses 3 bits to indicate interrupt "style". > + * we clear and set these three bits accordingly. The lower 24 > + * bits in two registers (GPIT1R and GPIT2R) are used to set up > + * the style for 8 lines each for a total of 16 GPIO lines. > + */ > +#define IXP4XX_GPIO_STYLE_ACTIVE_HIGH 0x0 > +#define IXP4XX_GPIO_STYLE_ACTIVE_LOW 0x1 > +#define IXP4XX_GPIO_STYLE_RISING_EDGE 0x2 > +#define IXP4XX_GPIO_STYLE_FALLING_EDGE 0x3 > +#define IXP4XX_GPIO_STYLE_TRANSITIONAL 0x4 > +#define IXP4XX_GPIO_STYLE_MASK 0x7 I'd use GENMASK(2, 0) here. > +#define IXP4XX_GPIO_STYLE_SIZE 3 > + > +/** > + * struct ixp4xx_gpio - IXP4 GPIO state container > + * @dev: containing device for this instance > + * @fwnode: the fwnode for this GPIO chip > + * @gc: gpiochip for this instance > + * @domain: irqdomain for this chip instance > + * @base: remapped I/O-memory base > + * @irq_edge: Each bit represents an IRQ: 1: edge-triggered, > + * 0: level triggered > + */ > +struct ixp4xx_gpio { > + struct device *dev; > + struct fwnode_handle *fwnode; > + struct gpio_chip gc; > + struct irq_domain *domain; > + void __iomem *base; > + unsigned long long irq_edge; > +}; > + > +/** > + * struct ixp4xx_gpio_map - IXP4 GPIO to parent IRQ map > + * @gpio_offset: offset of the IXP4 GPIO line > + * @parent_hwirq: hwirq on the parent IRQ controller > + */ > +struct ixp4xx_gpio_map { > + int gpio_offset; > + int parent_hwirq; > +}; > + > +/* GPIO lines 0..12 have corresponding IRQs, GPIOs 13..15 have no IRQs */ > +const struct ixp4xx_gpio_map ixp4xx_gpiomap[] = { > + { .gpio_offset = 0, .parent_hwirq = 6 }, > + { .gpio_offset = 1, .parent_hwirq = 7 }, > + { .gpio_offset = 2, .parent_hwirq = 19 }, > + { .gpio_offset = 3, .parent_hwirq = 20 }, > + { .gpio_offset = 4, .parent_hwirq = 21 }, > + { .gpio_offset = 5, .parent_hwirq = 22 }, > + { .gpio_offset = 6, .parent_hwirq = 23 }, > + { .gpio_offset = 7, .parent_hwirq = 24 }, > + { .gpio_offset = 8, .parent_hwirq = 25 }, > + { .gpio_offset = 9, .parent_hwirq = 26 }, > + { .gpio_offset = 10, .parent_hwirq = 27 }, > + { .gpio_offset = 11, .parent_hwirq = 28 }, > + { .gpio_offset = 12, .parent_hwirq = 29 }, > +}; > + > +static void ixp4xx_gpio_irq_ack(struct irq_data *d) > +{ > + struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d); > + > + __raw_writel(BIT(d->hwirq), g->base + IXP4XX_GPIO_GPISR); > +} > + > +static void ixp4xx_gpio_irq_unmask(struct irq_data *d) > +{ > + struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d); > + > + /* ACK when unmasking if not edge-triggered */ > + if (!(g->irq_edge & BIT(d->hwirq))) > + ixp4xx_gpio_irq_ack(d); > + > + irq_chip_unmask_parent(d); > +} > + > +static int ixp4xx_gpio_irq_set_type(struct irq_data *d, unsigned int type) > +{ > + struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d); > + int line = d->hwirq; > + u32 int_style; > + unsigned long flags; > + u32 int_reg; > + u32 val; > + I'm personally a fan of putting variables of the same type in the same line and arranging them in reverse-christmas tree order, but feel free to ignore it. > + switch (type) { > + case IRQ_TYPE_EDGE_BOTH: > + irq_set_handler_locked(d, handle_edge_irq); > + int_style = IXP4XX_GPIO_STYLE_TRANSITIONAL; > + g->irq_edge |= BIT(d->hwirq); > + break; > + case IRQ_TYPE_EDGE_RISING: > + irq_set_handler_locked(d, handle_edge_irq); > + int_style = IXP4XX_GPIO_STYLE_RISING_EDGE; > + g->irq_edge |= BIT(d->hwirq); > + break; > + case IRQ_TYPE_EDGE_FALLING: > + irq_set_handler_locked(d, handle_edge_irq); > + int_style = IXP4XX_GPIO_STYLE_FALLING_EDGE; > + g->irq_edge |= BIT(d->hwirq); > + break; > + case IRQ_TYPE_LEVEL_HIGH: > + irq_set_handler_locked(d, handle_level_irq); > + int_style = IXP4XX_GPIO_STYLE_ACTIVE_HIGH; > + g->irq_edge &= ~BIT(d->hwirq); > + break; > + case IRQ_TYPE_LEVEL_LOW: > + irq_set_handler_locked(d, handle_level_irq); > + int_style = IXP4XX_GPIO_STYLE_ACTIVE_LOW; > + g->irq_edge &= ~BIT(d->hwirq); > + break; > + default: > + return -EINVAL; > + } > + > + if (line >= 8) { > + /* pins 8-15 */ > + line -= 8; > + int_reg = IXP4XX_GPIO_GPIT2R; > + } else { > + /* pins 0-7 */ > + int_reg = IXP4XX_GPIO_GPIT1R; > + } > + > + spin_lock_irqsave(&g->gc.bgpio_lock, flags); > + > + /* Clear the style for the appropriate pin */ > + val = __raw_readl(g->base + int_reg); > + val &= ~(IXP4XX_GPIO_STYLE_MASK << (line * IXP4XX_GPIO_STYLE_SIZE)); > + __raw_writel(val, g->base + int_reg); I know you're not using regmap, because this driver's based on gpio-mmio, but I'm wondering if you could maybe wrap those three operations in a helper wrapper e.g. ixp4xx_update_reg() or something to make it easier to read. Same below. > + > + __raw_writel(BIT(line), g->base + IXP4XX_GPIO_GPISR); > + > + /* Set the new style */ > + val = __raw_readl(g->base + int_reg); > + val |= (int_style << (line * IXP4XX_GPIO_STYLE_SIZE)); > + __raw_writel(val, g->base + int_reg); > + > + /* Force-configure this line as an input */ > + val = __raw_readl(g->base + IXP4XX_GPIO_GPOER); > + val |= BIT(d->hwirq); > + __raw_writel(val, g->base + IXP4XX_GPIO_GPOER); > + > + spin_unlock_irqrestore(&g->gc.bgpio_lock, flags); > + > + /* This parent only accept level high (asserted) */ > + return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH); > +} > + > +static struct irq_chip ixp4xx_gpio_irqchip = { > + .name = "IXP4GPIO", > + .irq_ack = ixp4xx_gpio_irq_ack, > + .irq_mask = irq_chip_mask_parent, > + .irq_unmask = ixp4xx_gpio_irq_unmask, > + .irq_set_type = ixp4xx_gpio_irq_set_type, > +}; I assume this device cannot have multiple instances, so it's safe to have a static irqchip? > + > +static int ixp4xx_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > +{ > + struct ixp4xx_gpio *g = gpiochip_get_data(gc); > + struct irq_fwspec fwspec; > + > + fwspec.fwnode = g->fwnode; > + fwspec.param_count = 2; > + fwspec.param[0] = offset; > + fwspec.param[1] = IRQ_TYPE_NONE; > + > + return irq_create_fwspec_mapping(&fwspec); > +} > + This is were I start to struggle. I'm still not very well versed in irq domain hierarchies yet. You already explain the concept in the commit message, but it would be great if you could add a comment describing the technical details of implementation here and in other related callbacks. The whole fwspec thingy is not very obvious and it would serve as an example for the future. > +static int ixp4xx_gpio_irq_domain_translate(struct irq_domain *domain, > + struct irq_fwspec *fwspec, > + unsigned long *hwirq, > + unsigned int *type) > +{ > + > + /* We support standard DT translation */ > + if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) { > + *hwirq = fwspec->param[0]; > + *type = fwspec->param[1]; > + return 0; > + } This logic is a bit non-intuitive - maybe you could first check for error conditions and then do any processing? > + > + /* This goes away when we transition to DT */ > + if (is_fwnode_irqchip(fwspec->fwnode)) { > + if (fwspec->param_count != 2) > + return -EINVAL; > + *hwirq = fwspec->param[0]; > + *type = fwspec->param[1]; > + WARN_ON(*type == IRQ_TYPE_NONE); > + return 0; > + } > + return -EINVAL; > +} > + > +static int ixp4xx_gpio_irq_domain_alloc(struct irq_domain *d, > + unsigned int irq, unsigned int nr_irqs, > + void *data) > +{ > + struct ixp4xx_gpio *g = d->host_data; > + irq_hw_number_t hwirq; > + unsigned int type = IRQ_TYPE_NONE; > + struct irq_fwspec *fwspec = data; > + int ret; > + int i; > + > + ret = ixp4xx_gpio_irq_domain_translate(d, fwspec, &hwirq, &type); > + if (ret) > + return ret; > + > + dev_dbg(g->dev, "allocate IRQ %d..%d, hwirq %lu..%lu\n", > + irq, irq + nr_irqs - 1, > + hwirq, hwirq + nr_irqs - 1); > + > + for (i = 0; i < nr_irqs; i++) { > + struct irq_fwspec parent_fwspec; > + const struct ixp4xx_gpio_map *map; > + int j; > + > + /* Not all lines support IRQs */ > + for (j = 0; j < ARRAY_SIZE(ixp4xx_gpiomap); j++) { > + map = &ixp4xx_gpiomap[j]; > + if (map->gpio_offset == hwirq) > + break; > + } > + if (j == ARRAY_SIZE(ixp4xx_gpiomap)) { > + dev_err(g->dev, "can't look up hwirq %lu\n", hwirq); > + return -EINVAL; > + } > + dev_dbg(g->dev, "found parent hwirq %u\n", map->parent_hwirq); > + > + /* > + * We set handle_bad_irq because the .set_type() should > + * always be invoked and set the right type of handler. > + */ > + irq_domain_set_info(d, > + irq + i, > + hwirq + i, > + &ixp4xx_gpio_irqchip, > + g, > + handle_bad_irq, > + NULL, NULL); > + irq_set_probe(irq + i); > + > + /* > + * Create a IRQ fwspec to send up to the parent irqdomain: > + * specify the hwirq we address on the parent and tie it > + * all together up the chain. > + */ > + parent_fwspec.fwnode = d->parent->fwnode; > + parent_fwspec.param_count = 2; > + parent_fwspec.param[0] = map->parent_hwirq; > + /* This parent only handles asserted level IRQs */ > + parent_fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH; > + dev_dbg(g->dev, "alloc_irqs_parent for %d parent hwirq %d\n", > + irq + i, map->parent_hwirq); > + ret = irq_domain_alloc_irqs_parent(d, irq + i, 1, > + &parent_fwspec); > + if (ret) > + dev_err(g->dev, > + "failed to allocate parent hwirq %d for hwirq %lu\n", > + map->parent_hwirq, hwirq); > + } > + > + return 0; > +} > + > +static const struct irq_domain_ops ixp4xx_gpio_irqdomain_ops = { > + .translate = ixp4xx_gpio_irq_domain_translate, > + .alloc = ixp4xx_gpio_irq_domain_alloc, > + .free = irq_domain_free_irqs_common, > +}; > + > +static int ixp4xx_gpio_probe(struct platform_device *pdev) > +{ > + unsigned long flags; > + struct device *dev = &pdev->dev; > + struct irq_domain *parent; > + struct resource *res; > + struct ixp4xx_gpio *g; > + int ret; > + int i; > + > + g = devm_kzalloc(dev, sizeof(*g), GFP_KERNEL); > + if (!g) > + return -ENOMEM; > + g->dev = dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + g->base = devm_ioremap_resource(dev, res); > + if (IS_ERR(g->base)) { > + dev_err(dev, "ioremap error\n"); > + return PTR_ERR(g->base); > + } > + > + /* > + * Make sure GPIO 14 and 15 are NOT used as clocks but GPIO on > + * specific machines. > + */ > + if (machine_is_dsmg600() || machine_is_nas100d()) > + __raw_writel(0x0, g->base + IXP4XX_GPIO_GPCLKR); > + > + /* > + * This is a very special big-endian ARM issue: when the IXP4xx is > + * run in big endian mode, all registers in the machine are switched > + * around to the CPU-native endianness. As you see mostly in the > + * driver we use __raw_readl()/__raw_writel() to access the registers > + * in the appropriate order. With the GPIO library we need to specify > + * byte order explicitly, so this flag needs to be set when compiling > + * for big endian. > + */ > +#if defined(CONFIG_CPU_BIG_ENDIAN) > + flags = BGPIOF_BIG_ENDIAN_BYTE_ORDER; > +#else > + flags = 0; > +#endif > + > + /* Populate and register gpio chip */ > + ret = bgpio_init(&g->gc, dev, 4, > + g->base + IXP4XX_GPIO_GPINR, > + g->base + IXP4XX_GPIO_GPOUTR, > + NULL, > + NULL, > + g->base + IXP4XX_GPIO_GPOER, > + flags); > + if (ret) { > + dev_err(dev, "unable to init generic GPIO\n"); > + return ret; > + } > + g->gc.to_irq = ixp4xx_gpio_to_irq; > + g->gc.ngpio = 16; > + g->gc.label = "IXP4XX_GPIO_CHIP"; > + /* > + * TODO: when we have migrated to device tree and all GPIOs > + * are fetched using phandles, set this to -1 to get rid of > + * the fixed gpiochip base. > + */ > + g->gc.base = 0; > + g->gc.parent = &pdev->dev; > + g->gc.owner = THIS_MODULE; > + > + ret = devm_gpiochip_add_data(dev, &g->gc, g); > + if (ret) { > + dev_err(dev, "failed to add SoC gpiochip\n"); > + return ret; > + } > + > + /* > + * When we convert to device tree we will simply look up the > + * parent irqdomain using irq_find_host(parent) as parent comes > + * from IRQCHIP_DECLARE(), then use of_node_to_fwnode() to get > + * the fwnode. For now we need this boardfile style code. > + */ > + parent = ixp4xx_get_irq_domain(); > + g->fwnode = irq_domain_alloc_fwnode(g->base); > + if (!g->fwnode) { > + dev_err(dev, "no domain base\n"); > + return -ENODEV; > + } > + g->domain = irq_domain_create_hierarchy(parent, > + IRQ_DOMAIN_FLAG_HIERARCHY, > + ARRAY_SIZE(ixp4xx_gpiomap), > + g->fwnode, > + &ixp4xx_gpio_irqdomain_ops, > + g); > + if (!g->domain) { > + irq_domain_free_fwnode(g->fwnode); > + dev_err(dev, "no hierarchical irq domain\n"); > + return ret; > + } > + > + /* > + * After adding OF support, this is no longer needed: irqs > + * will be allocated for the respective fwnodes. > + */ > + for (i = 0; i < ARRAY_SIZE(ixp4xx_gpiomap); i++) { > + const struct ixp4xx_gpio_map *map = &ixp4xx_gpiomap[i]; > + struct irq_fwspec fwspec; > + > + fwspec.fwnode = g->fwnode; > + /* This is the hwirq for the GPIO line side of things */ > + fwspec.param[0] = map->gpio_offset; > + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > + fwspec.param_count = 2; > + ret = __irq_domain_alloc_irqs(g->domain, > + -1, /* just pick something */ > + 1, > + NUMA_NO_NODE, > + &fwspec, > + false, > + NULL); > + if (ret < 0) { > + irq_domain_free_fwnode(g->fwnode); > + dev_err(dev, > + "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n", > + map->gpio_offset, map->parent_hwirq, ret); > + return ret; > + } > + } > + > + platform_set_drvdata(pdev, g); > + dev_info(dev, "IXP4 GPIO @%p registered\n", g->base); > + > + return 0; > +} > + Don't you need to dispose of the domain in the remove callback? They don't seem to have devm_ variants yet. > +static struct platform_driver ixp4xx_gpio_driver = { > + .driver = { > + .name = "ixp4xx-gpio", > + }, > + .probe = ixp4xx_gpio_probe, > +}; > +builtin_platform_driver(ixp4xx_gpio_driver); > -- > 2.20.1 > Best regards, Bartosz Golaszewski
Hi Bartosz, thanks for the review! On Wed, Feb 6, 2019 at 5:04 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > niedz., 3 lut 2019 o 22:42 Linus Walleij <linus.walleij@linaro.org> napisał(a): (...) > Just a nit, but I'd add a newline between the includes and the header. > Also: C++ style comment for the header. OK > > +#include <linux/irqchip/irq-ixp4xx.h> > > I think a newline between linux/ and asm/ includes is a good rule. OK > > +#define IXP4XX_GPIO_GPOUTR 0x00 > > +#define IXP4XX_GPIO_GPOER 0x04 > > +#define IXP4XX_GPIO_GPINR 0x08 > > +#define IXP4XX_GPIO_GPISR 0x0C > > +#define IXP4XX_GPIO_GPIT1R 0x10 > > +#define IXP4XX_GPIO_GPIT2R 0x14 > > +#define IXP4XX_GPIO_GPCLKR 0x18 > > +#define IXP4XX_GPIO_GPDBSELR 0x1C > > Since these are registers offsets - maybe the prefix could be > IXP4XX_GPIO_REG_* or IXP4XX_GPIO_OFF_*? I think it's more readable. OK I renamed them IXP4XX_REG_GPOUT etc. > > +#define IXP4XX_GPIO_STYLE_MASK 0x7 > > I'd use GENMASK(2, 0) here. OK > > + /* Clear the style for the appropriate pin */ > > + val = __raw_readl(g->base + int_reg); > > + val &= ~(IXP4XX_GPIO_STYLE_MASK << (line * IXP4XX_GPIO_STYLE_SIZE)); > > + __raw_writel(val, g->base + int_reg); > > I know you're not using regmap, because this driver's based on > gpio-mmio, but I'm wondering if you could maybe wrap those three > operations in a helper wrapper e.g. ixp4xx_update_reg() or something > to make it easier to read. Same below. I'm uncertain about that, I feel that kind of stuff creates extra runpaths that make the code hard to read, the compiler will of course inline it, but I like the read-modify-write explicit instead of wrapped in a function. > > +static struct irq_chip ixp4xx_gpio_irqchip = { > > + .name = "IXP4GPIO", > > + .irq_ack = ixp4xx_gpio_irq_ack, > > + .irq_mask = irq_chip_mask_parent, > > + .irq_unmask = ixp4xx_gpio_irq_unmask, > > + .irq_set_type = ixp4xx_gpio_irq_set_type, > > +}; > > I assume this device cannot have multiple instances, so it's safe to > have a static irqchip? Yes there can be only one. > > +static int ixp4xx_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct ixp4xx_gpio *g = gpiochip_get_data(gc); > > + struct irq_fwspec fwspec; > > + > > + fwspec.fwnode = g->fwnode; > > + fwspec.param_count = 2; > > + fwspec.param[0] = offset; > > + fwspec.param[1] = IRQ_TYPE_NONE; > > + > > + return irq_create_fwspec_mapping(&fwspec); > > +} > > + > > This is were I start to struggle. I'm still not very well versed in > irq domain hierarchies yet. You already explain the concept in the > commit message, but it would be great if you could add a comment > describing the technical details of implementation here and in other > related callbacks. The whole fwspec thingy is not very obvious and it > would serve as an example for the future. I wrote a paragraph of explanations but I realized that I was just writing documentation that should rather be in irqdomain. My plan is to update the GPIO driver documentation with a thorough description of hierarchical irqs instead. > > +static int ixp4xx_gpio_irq_domain_translate(struct irq_domain *domain, > > + struct irq_fwspec *fwspec, > > + unsigned long *hwirq, > > + unsigned int *type) > > +{ > > + > > + /* We support standard DT translation */ > > + if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) { > > + *hwirq = fwspec->param[0]; > > + *type = fwspec->param[1]; > > + return 0; > > + } > > This logic is a bit non-intuitive - maybe you could first check for > error conditions and then do any processing? Brian has created a generic twocell translation helper in irqchip for this, so I will refactor on top of that at some point and this complexity goes away. > Don't you need to dispose of the domain in the remove callback? They > don't seem to have devm_ variants yet. This is a bool Kconfig symbol so the driver is compiled into the kernel. Once probed, the irqchip/domain never goes away. If I try to remove it it would just take down the whole system with it, so even if I would implement it it would make no sense. There is something of a "hardness" in how tight an irqdomain/chip is coupled to a system, and on this system it is (currently) used for a lot of static board files and it just can't go away. (The opposite being e.g. a USB expander gpiochip which should me a module and can come and go as it please.) We could reconsider once all is moved over to device tree and more dynamic though. Yours, Linus Walleij
diff --git a/MAINTAINERS b/MAINTAINERS index 0d48faa3e635..6c161bd82238 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1651,6 +1651,7 @@ M: Krzysztof Halasa <khalasa@piap.pl> L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) S: Maintained F: arch/arm/mach-ixp4xx/ +F: drivers/gpio/gpio-ixp4xx.c F: drivers/irqchip/irq-ixp4xx.c F: include/linux/irqchip/irq-ixp4xx.h diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 699a8118c433..fe4a47e49a24 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -286,6 +286,18 @@ config GPIO_IOP If unsure, say N. +config GPIO_IXP4XX + bool "Intel IXP4xx GPIO" + depends on ARCH_IXP4XX || COMPILE_TEST + select GPIO_GENERIC + select IRQ_DOMAIN + select IRQ_DOMAIN_HIERARCHY + help + Say yes here to support the GPIO functionality of a number of Intel + IXP4xx series of chips. + + If unsure, say N. + config GPIO_LOONGSON bool "Loongson-2/3 GPIO support" depends on CPU_LOONGSON2 || CPU_LOONGSON3 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 0568bbe6fe68..cf66523b5eec 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -60,6 +60,7 @@ 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_IOP) += gpio-iop.o +obj-$(CONFIG_GPIO_IXP4XX) += gpio-ixp4xx.o obj-$(CONFIG_GPIO_IT87) += gpio-it87.o obj-$(CONFIG_GPIO_JANZ_TTL) += gpio-janz-ttl.o obj-$(CONFIG_GPIO_KEMPLD) += gpio-kempld.o diff --git a/drivers/gpio/gpio-ixp4xx.c b/drivers/gpio/gpio-ixp4xx.c new file mode 100644 index 000000000000..44c24948379d --- /dev/null +++ b/drivers/gpio/gpio-ixp4xx.c @@ -0,0 +1,443 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * IXP4 GPIO driver + * Copyright (C) 2019 Linus Walleij <linus.walleij@linaro.org> + * + * based on previous work and know-how from: + * Deepak Saxena <dsaxena@plexity.net> + */ +#include <linux/gpio/driver.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/irqchip.h> +#include <linux/platform_device.h> +#include <linux/bitops.h> +/* Include that go away with DT transition */ +#include <linux/irqchip/irq-ixp4xx.h> +#include <asm/mach-types.h> + +#define IXP4XX_GPIO_GPOUTR 0x00 +#define IXP4XX_GPIO_GPOER 0x04 +#define IXP4XX_GPIO_GPINR 0x08 +#define IXP4XX_GPIO_GPISR 0x0C +#define IXP4XX_GPIO_GPIT1R 0x10 +#define IXP4XX_GPIO_GPIT2R 0x14 +#define IXP4XX_GPIO_GPCLKR 0x18 +#define IXP4XX_GPIO_GPDBSELR 0x1C + +/* + * The hardware uses 3 bits to indicate interrupt "style". + * we clear and set these three bits accordingly. The lower 24 + * bits in two registers (GPIT1R and GPIT2R) are used to set up + * the style for 8 lines each for a total of 16 GPIO lines. + */ +#define IXP4XX_GPIO_STYLE_ACTIVE_HIGH 0x0 +#define IXP4XX_GPIO_STYLE_ACTIVE_LOW 0x1 +#define IXP4XX_GPIO_STYLE_RISING_EDGE 0x2 +#define IXP4XX_GPIO_STYLE_FALLING_EDGE 0x3 +#define IXP4XX_GPIO_STYLE_TRANSITIONAL 0x4 +#define IXP4XX_GPIO_STYLE_MASK 0x7 +#define IXP4XX_GPIO_STYLE_SIZE 3 + +/** + * struct ixp4xx_gpio - IXP4 GPIO state container + * @dev: containing device for this instance + * @fwnode: the fwnode for this GPIO chip + * @gc: gpiochip for this instance + * @domain: irqdomain for this chip instance + * @base: remapped I/O-memory base + * @irq_edge: Each bit represents an IRQ: 1: edge-triggered, + * 0: level triggered + */ +struct ixp4xx_gpio { + struct device *dev; + struct fwnode_handle *fwnode; + struct gpio_chip gc; + struct irq_domain *domain; + void __iomem *base; + unsigned long long irq_edge; +}; + +/** + * struct ixp4xx_gpio_map - IXP4 GPIO to parent IRQ map + * @gpio_offset: offset of the IXP4 GPIO line + * @parent_hwirq: hwirq on the parent IRQ controller + */ +struct ixp4xx_gpio_map { + int gpio_offset; + int parent_hwirq; +}; + +/* GPIO lines 0..12 have corresponding IRQs, GPIOs 13..15 have no IRQs */ +const struct ixp4xx_gpio_map ixp4xx_gpiomap[] = { + { .gpio_offset = 0, .parent_hwirq = 6 }, + { .gpio_offset = 1, .parent_hwirq = 7 }, + { .gpio_offset = 2, .parent_hwirq = 19 }, + { .gpio_offset = 3, .parent_hwirq = 20 }, + { .gpio_offset = 4, .parent_hwirq = 21 }, + { .gpio_offset = 5, .parent_hwirq = 22 }, + { .gpio_offset = 6, .parent_hwirq = 23 }, + { .gpio_offset = 7, .parent_hwirq = 24 }, + { .gpio_offset = 8, .parent_hwirq = 25 }, + { .gpio_offset = 9, .parent_hwirq = 26 }, + { .gpio_offset = 10, .parent_hwirq = 27 }, + { .gpio_offset = 11, .parent_hwirq = 28 }, + { .gpio_offset = 12, .parent_hwirq = 29 }, +}; + +static void ixp4xx_gpio_irq_ack(struct irq_data *d) +{ + struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d); + + __raw_writel(BIT(d->hwirq), g->base + IXP4XX_GPIO_GPISR); +} + +static void ixp4xx_gpio_irq_unmask(struct irq_data *d) +{ + struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d); + + /* ACK when unmasking if not edge-triggered */ + if (!(g->irq_edge & BIT(d->hwirq))) + ixp4xx_gpio_irq_ack(d); + + irq_chip_unmask_parent(d); +} + +static int ixp4xx_gpio_irq_set_type(struct irq_data *d, unsigned int type) +{ + struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d); + int line = d->hwirq; + u32 int_style; + unsigned long flags; + u32 int_reg; + u32 val; + + switch (type) { + case IRQ_TYPE_EDGE_BOTH: + irq_set_handler_locked(d, handle_edge_irq); + int_style = IXP4XX_GPIO_STYLE_TRANSITIONAL; + g->irq_edge |= BIT(d->hwirq); + break; + case IRQ_TYPE_EDGE_RISING: + irq_set_handler_locked(d, handle_edge_irq); + int_style = IXP4XX_GPIO_STYLE_RISING_EDGE; + g->irq_edge |= BIT(d->hwirq); + break; + case IRQ_TYPE_EDGE_FALLING: + irq_set_handler_locked(d, handle_edge_irq); + int_style = IXP4XX_GPIO_STYLE_FALLING_EDGE; + g->irq_edge |= BIT(d->hwirq); + break; + case IRQ_TYPE_LEVEL_HIGH: + irq_set_handler_locked(d, handle_level_irq); + int_style = IXP4XX_GPIO_STYLE_ACTIVE_HIGH; + g->irq_edge &= ~BIT(d->hwirq); + break; + case IRQ_TYPE_LEVEL_LOW: + irq_set_handler_locked(d, handle_level_irq); + int_style = IXP4XX_GPIO_STYLE_ACTIVE_LOW; + g->irq_edge &= ~BIT(d->hwirq); + break; + default: + return -EINVAL; + } + + if (line >= 8) { + /* pins 8-15 */ + line -= 8; + int_reg = IXP4XX_GPIO_GPIT2R; + } else { + /* pins 0-7 */ + int_reg = IXP4XX_GPIO_GPIT1R; + } + + spin_lock_irqsave(&g->gc.bgpio_lock, flags); + + /* Clear the style for the appropriate pin */ + val = __raw_readl(g->base + int_reg); + val &= ~(IXP4XX_GPIO_STYLE_MASK << (line * IXP4XX_GPIO_STYLE_SIZE)); + __raw_writel(val, g->base + int_reg); + + __raw_writel(BIT(line), g->base + IXP4XX_GPIO_GPISR); + + /* Set the new style */ + val = __raw_readl(g->base + int_reg); + val |= (int_style << (line * IXP4XX_GPIO_STYLE_SIZE)); + __raw_writel(val, g->base + int_reg); + + /* Force-configure this line as an input */ + val = __raw_readl(g->base + IXP4XX_GPIO_GPOER); + val |= BIT(d->hwirq); + __raw_writel(val, g->base + IXP4XX_GPIO_GPOER); + + spin_unlock_irqrestore(&g->gc.bgpio_lock, flags); + + /* This parent only accept level high (asserted) */ + return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH); +} + +static struct irq_chip ixp4xx_gpio_irqchip = { + .name = "IXP4GPIO", + .irq_ack = ixp4xx_gpio_irq_ack, + .irq_mask = irq_chip_mask_parent, + .irq_unmask = ixp4xx_gpio_irq_unmask, + .irq_set_type = ixp4xx_gpio_irq_set_type, +}; + +static int ixp4xx_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) +{ + struct ixp4xx_gpio *g = gpiochip_get_data(gc); + struct irq_fwspec fwspec; + + fwspec.fwnode = g->fwnode; + fwspec.param_count = 2; + fwspec.param[0] = offset; + fwspec.param[1] = IRQ_TYPE_NONE; + + return irq_create_fwspec_mapping(&fwspec); +} + +static int ixp4xx_gpio_irq_domain_translate(struct irq_domain *domain, + struct irq_fwspec *fwspec, + unsigned long *hwirq, + unsigned int *type) +{ + + /* We support standard DT translation */ + if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) { + *hwirq = fwspec->param[0]; + *type = fwspec->param[1]; + return 0; + } + + /* This goes away when we transition to DT */ + if (is_fwnode_irqchip(fwspec->fwnode)) { + if (fwspec->param_count != 2) + return -EINVAL; + *hwirq = fwspec->param[0]; + *type = fwspec->param[1]; + WARN_ON(*type == IRQ_TYPE_NONE); + return 0; + } + return -EINVAL; +} + +static int ixp4xx_gpio_irq_domain_alloc(struct irq_domain *d, + unsigned int irq, unsigned int nr_irqs, + void *data) +{ + struct ixp4xx_gpio *g = d->host_data; + irq_hw_number_t hwirq; + unsigned int type = IRQ_TYPE_NONE; + struct irq_fwspec *fwspec = data; + int ret; + int i; + + ret = ixp4xx_gpio_irq_domain_translate(d, fwspec, &hwirq, &type); + if (ret) + return ret; + + dev_dbg(g->dev, "allocate IRQ %d..%d, hwirq %lu..%lu\n", + irq, irq + nr_irqs - 1, + hwirq, hwirq + nr_irqs - 1); + + for (i = 0; i < nr_irqs; i++) { + struct irq_fwspec parent_fwspec; + const struct ixp4xx_gpio_map *map; + int j; + + /* Not all lines support IRQs */ + for (j = 0; j < ARRAY_SIZE(ixp4xx_gpiomap); j++) { + map = &ixp4xx_gpiomap[j]; + if (map->gpio_offset == hwirq) + break; + } + if (j == ARRAY_SIZE(ixp4xx_gpiomap)) { + dev_err(g->dev, "can't look up hwirq %lu\n", hwirq); + return -EINVAL; + } + dev_dbg(g->dev, "found parent hwirq %u\n", map->parent_hwirq); + + /* + * We set handle_bad_irq because the .set_type() should + * always be invoked and set the right type of handler. + */ + irq_domain_set_info(d, + irq + i, + hwirq + i, + &ixp4xx_gpio_irqchip, + g, + handle_bad_irq, + NULL, NULL); + irq_set_probe(irq + i); + + /* + * Create a IRQ fwspec to send up to the parent irqdomain: + * specify the hwirq we address on the parent and tie it + * all together up the chain. + */ + parent_fwspec.fwnode = d->parent->fwnode; + parent_fwspec.param_count = 2; + parent_fwspec.param[0] = map->parent_hwirq; + /* This parent only handles asserted level IRQs */ + parent_fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH; + dev_dbg(g->dev, "alloc_irqs_parent for %d parent hwirq %d\n", + irq + i, map->parent_hwirq); + ret = irq_domain_alloc_irqs_parent(d, irq + i, 1, + &parent_fwspec); + if (ret) + dev_err(g->dev, + "failed to allocate parent hwirq %d for hwirq %lu\n", + map->parent_hwirq, hwirq); + } + + return 0; +} + +static const struct irq_domain_ops ixp4xx_gpio_irqdomain_ops = { + .translate = ixp4xx_gpio_irq_domain_translate, + .alloc = ixp4xx_gpio_irq_domain_alloc, + .free = irq_domain_free_irqs_common, +}; + +static int ixp4xx_gpio_probe(struct platform_device *pdev) +{ + unsigned long flags; + struct device *dev = &pdev->dev; + struct irq_domain *parent; + struct resource *res; + struct ixp4xx_gpio *g; + int ret; + int i; + + g = devm_kzalloc(dev, sizeof(*g), GFP_KERNEL); + if (!g) + return -ENOMEM; + g->dev = dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + g->base = devm_ioremap_resource(dev, res); + if (IS_ERR(g->base)) { + dev_err(dev, "ioremap error\n"); + return PTR_ERR(g->base); + } + + /* + * Make sure GPIO 14 and 15 are NOT used as clocks but GPIO on + * specific machines. + */ + if (machine_is_dsmg600() || machine_is_nas100d()) + __raw_writel(0x0, g->base + IXP4XX_GPIO_GPCLKR); + + /* + * This is a very special big-endian ARM issue: when the IXP4xx is + * run in big endian mode, all registers in the machine are switched + * around to the CPU-native endianness. As you see mostly in the + * driver we use __raw_readl()/__raw_writel() to access the registers + * in the appropriate order. With the GPIO library we need to specify + * byte order explicitly, so this flag needs to be set when compiling + * for big endian. + */ +#if defined(CONFIG_CPU_BIG_ENDIAN) + flags = BGPIOF_BIG_ENDIAN_BYTE_ORDER; +#else + flags = 0; +#endif + + /* Populate and register gpio chip */ + ret = bgpio_init(&g->gc, dev, 4, + g->base + IXP4XX_GPIO_GPINR, + g->base + IXP4XX_GPIO_GPOUTR, + NULL, + NULL, + g->base + IXP4XX_GPIO_GPOER, + flags); + if (ret) { + dev_err(dev, "unable to init generic GPIO\n"); + return ret; + } + g->gc.to_irq = ixp4xx_gpio_to_irq; + g->gc.ngpio = 16; + g->gc.label = "IXP4XX_GPIO_CHIP"; + /* + * TODO: when we have migrated to device tree and all GPIOs + * are fetched using phandles, set this to -1 to get rid of + * the fixed gpiochip base. + */ + g->gc.base = 0; + g->gc.parent = &pdev->dev; + g->gc.owner = THIS_MODULE; + + ret = devm_gpiochip_add_data(dev, &g->gc, g); + if (ret) { + dev_err(dev, "failed to add SoC gpiochip\n"); + return ret; + } + + /* + * When we convert to device tree we will simply look up the + * parent irqdomain using irq_find_host(parent) as parent comes + * from IRQCHIP_DECLARE(), then use of_node_to_fwnode() to get + * the fwnode. For now we need this boardfile style code. + */ + parent = ixp4xx_get_irq_domain(); + g->fwnode = irq_domain_alloc_fwnode(g->base); + if (!g->fwnode) { + dev_err(dev, "no domain base\n"); + return -ENODEV; + } + g->domain = irq_domain_create_hierarchy(parent, + IRQ_DOMAIN_FLAG_HIERARCHY, + ARRAY_SIZE(ixp4xx_gpiomap), + g->fwnode, + &ixp4xx_gpio_irqdomain_ops, + g); + if (!g->domain) { + irq_domain_free_fwnode(g->fwnode); + dev_err(dev, "no hierarchical irq domain\n"); + return ret; + } + + /* + * After adding OF support, this is no longer needed: irqs + * will be allocated for the respective fwnodes. + */ + for (i = 0; i < ARRAY_SIZE(ixp4xx_gpiomap); i++) { + const struct ixp4xx_gpio_map *map = &ixp4xx_gpiomap[i]; + struct irq_fwspec fwspec; + + fwspec.fwnode = g->fwnode; + /* This is the hwirq for the GPIO line side of things */ + fwspec.param[0] = map->gpio_offset; + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; + fwspec.param_count = 2; + ret = __irq_domain_alloc_irqs(g->domain, + -1, /* just pick something */ + 1, + NUMA_NO_NODE, + &fwspec, + false, + NULL); + if (ret < 0) { + irq_domain_free_fwnode(g->fwnode); + dev_err(dev, + "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n", + map->gpio_offset, map->parent_hwirq, ret); + return ret; + } + } + + platform_set_drvdata(pdev, g); + dev_info(dev, "IXP4 GPIO @%p registered\n", g->base); + + return 0; +} + +static struct platform_driver ixp4xx_gpio_driver = { + .driver = { + .name = "ixp4xx-gpio", + }, + .probe = ixp4xx_gpio_probe, +}; +builtin_platform_driver(ixp4xx_gpio_driver);
This adds a driver for the IXP4xx GPIO block found in the Intel XScale IXP4xx systems. The GPIO part of this block is pretty straight-forward and just uses the generic MMIO GPIO library. The irqchip side of this driver is hierarchical where the main irqchip will receive a processed level trigger in response to the edge detector of the GPIO block, so for this reason the v2 version of the irqdomain API is used (as well as in the parent IXP4xx irqchip) and masking, unmasking and setting up the type on IRQ happens on several levels. Currently this GPIO controller will grab the parent irqdomain using a special function, but as the platform move toward device tree probing, this will not be needed: we can just look up the parent irqdomain from the device tree. Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Bartosz: looking for your ACK on this, it'd be good if the other GPIO maintainer is aligned with my ideas here. I intend to merge this through the ARM SoC tree. --- MAINTAINERS | 1 + drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-ixp4xx.c | 443 +++++++++++++++++++++++++++++++++++++ 4 files changed, 457 insertions(+) create mode 100644 drivers/gpio/gpio-ixp4xx.c