* [PATCH 0/2] gpio: brcm: XGS iProc GPIO driver @ 2019-10-04 1:25 Chris Packham 2019-10-04 1:25 ` [PATCH 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc Chris Packham 2019-10-04 1:25 ` [PATCH 2/2] gpio: Add xgs-iproc driver Chris Packham 0 siblings, 2 replies; 9+ messages in thread From: Chris Packham @ 2019-10-04 1:25 UTC (permalink / raw) To: linus.walleij, bgolaszewski, robh+dt, mark.rutland, rjui, sbranden, bcm-kernel-feedback-list, f.fainelli, richard.laing Cc: linux-gpio, Chris Packham, linux-kernel, linux-arm-kernel, devicetree This is ported this from Broadcom's XLDK. There seem to be 3 different IP blocks for 3 separate banks of GPIOs in the iProc chips. I've dropped everything except support for the Chip Common A GPIO controller because the other blocks actually seem to be supportable with other drivers. The driver itself is halfway between pinctrl-nsp-gpio.c and pinctrl-iproc-gpio.c. Chris Packham (2): dt-bindings: gpio: brcm: Add bindings for xgs-iproc gpio: Add xgs-iproc driver .../bindings/gpio/brcm,xgs-iproc.txt | 41 ++ drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xgs-iproc.c | 422 ++++++++++++++++++ 4 files changed, 473 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.txt create mode 100644 drivers/gpio/gpio-xgs-iproc.c -- 2.23.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc 2019-10-04 1:25 [PATCH 0/2] gpio: brcm: XGS iProc GPIO driver Chris Packham @ 2019-10-04 1:25 ` Chris Packham 2019-10-15 19:23 ` Rob Herring 2019-10-04 1:25 ` [PATCH 2/2] gpio: Add xgs-iproc driver Chris Packham 1 sibling, 1 reply; 9+ messages in thread From: Chris Packham @ 2019-10-04 1:25 UTC (permalink / raw) To: linus.walleij, bgolaszewski, robh+dt, mark.rutland, rjui, sbranden, bcm-kernel-feedback-list, f.fainelli, richard.laing Cc: linux-gpio, Chris Packham, linux-kernel, linux-arm-kernel, devicetree This GPIO controller is present on a number of Broadcom switch ASICs with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio blocks but different enough to require a separate driver. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- .../bindings/gpio/brcm,xgs-iproc.txt | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.txt diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.txt b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.txt new file mode 100644 index 000000000000..328b844c82dc --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.txt @@ -0,0 +1,41 @@ +Broadcom XGS iProc GPIO controller + +This controller is the Chip Common A GPIO present on a number of Broadcom +switch ASICs with integrated SoCs. + +Required properties: +- compatible: + Must be "brcm,iproc-gpio-cca" + +- reg: + The first region defines the base I/O address containing + the GPIO controller registers. The second region defines + the I/O address containing the Chip Common A interrupt + registers. + +Optional properties: + +- interrupts: + The interrupt shared by all GPIO lines for this controller. + +- #interrupt-cells: + Should be <2>. The first cell is the GPIO number, the second should specify + flags. + + See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt + +- interrupt-controller: + Marks the device node as an interrupt controller + +Example: + gpioa: gpio@18000060 { + compatible = "brcm,iproc-gpio-cca"; + #gpio-cells = <2>; + reg = <0x18000060 0x50>, + <0x18000000 0x50>; + ngpios = <12>; + gpio-controller; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>; + }; -- 2.23.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc 2019-10-04 1:25 ` [PATCH 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc Chris Packham @ 2019-10-15 19:23 ` Rob Herring 0 siblings, 0 replies; 9+ messages in thread From: Rob Herring @ 2019-10-15 19:23 UTC (permalink / raw) To: Chris Packham Cc: mark.rutland, devicetree, f.fainelli, sbranden, linux-gpio, rjui, linus.walleij, linux-kernel, richard.laing, bgolaszewski, bcm-kernel-feedback-list, linux-arm-kernel On Fri, Oct 04, 2019 at 02:25:24PM +1300, Chris Packham wrote: > This GPIO controller is present on a number of Broadcom switch ASICs > with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio > blocks but different enough to require a separate driver. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > .../bindings/gpio/brcm,xgs-iproc.txt | 41 +++++++++++++++++++ > 1 file changed, 41 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.txt Please make this a DT schema. > > diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.txt b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.txt > new file mode 100644 > index 000000000000..328b844c82dc > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.txt > @@ -0,0 +1,41 @@ > +Broadcom XGS iProc GPIO controller > + > +This controller is the Chip Common A GPIO present on a number of Broadcom > +switch ASICs with integrated SoCs. > + > +Required properties: > +- compatible: > + Must be "brcm,iproc-gpio-cca" > + > +- reg: > + The first region defines the base I/O address containing > + the GPIO controller registers. The second region defines > + the I/O address containing the Chip Common A interrupt > + registers. > + > +Optional properties: > + > +- interrupts: > + The interrupt shared by all GPIO lines for this controller. > + > +- #interrupt-cells: > + Should be <2>. The first cell is the GPIO number, the second should specify > + flags. > + > + See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > + > +- interrupt-controller: > + Marks the device node as an interrupt controller > + > +Example: > + gpioa: gpio@18000060 { > + compatible = "brcm,iproc-gpio-cca"; > + #gpio-cells = <2>; Not documented... > + reg = <0x18000060 0x50>, > + <0x18000000 0x50>; > + ngpios = <12>; Not documented. > + gpio-controller; Not documented. > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>; > + }; > -- > 2.23.0 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] gpio: Add xgs-iproc driver 2019-10-04 1:25 [PATCH 0/2] gpio: brcm: XGS iProc GPIO driver Chris Packham 2019-10-04 1:25 ` [PATCH 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc Chris Packham @ 2019-10-04 1:25 ` Chris Packham 2019-10-04 3:01 ` kbuild test robot ` (2 more replies) 1 sibling, 3 replies; 9+ messages in thread From: Chris Packham @ 2019-10-04 1:25 UTC (permalink / raw) To: linus.walleij, bgolaszewski, robh+dt, mark.rutland, rjui, sbranden, bcm-kernel-feedback-list, f.fainelli, richard.laing Cc: linux-gpio, Chris Packham, linux-kernel, linux-arm-kernel, devicetree This driver supports the Chip Common A GPIO controller present on a number of Broadcom switch ASICs with integrated SoCs. The controller is similar to the pinctrl-nsp-gpio and pinctrl-iproc-gpio blocks but different enough that a separate driver is required. This has been ported from Broadcom's XLDK 5.0.3 retaining only the CCA support (pinctrl-iproc-gpio covers CCB). Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xgs-iproc.c | 422 ++++++++++++++++++++++++++++++++++ 3 files changed, 432 insertions(+) create mode 100644 drivers/gpio/gpio-xgs-iproc.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 38e096e6925f..4b3c0f8397d7 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -156,6 +156,15 @@ config GPIO_BRCMSTB help Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs. +config GPIO_XGS_IPROC + tristate "BRCM XGS iProc GPIO support" + depends on OF_GPIO && (ARCH_BCM_IPROC || COMPILE_TEST) + select GPIO_GENERIC + select GPIOLIB_IRQCHIP + default ARCH_BCM_IPROC + help + Say yes here to enable GPIO support for Broadcom XGS iProc SoCs. + config GPIO_CADENCE tristate "Cadence GPIO support" depends on OF_GPIO diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index d2fd19c15bae..3783c3d43fbe 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -37,6 +37,7 @@ obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o obj-$(CONFIG_GPIO_BD70528) += gpio-bd70528.o obj-$(CONFIG_GPIO_BD9571MWV) += gpio-bd9571mwv.o obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o +obj-$(CONFIG_GPIO_XGS_IPROC) += gpio-xgs-iproc.o obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o obj-$(CONFIG_GPIO_CADENCE) += gpio-cadence.o obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o diff --git a/drivers/gpio/gpio-xgs-iproc.c b/drivers/gpio/gpio-xgs-iproc.c new file mode 100644 index 000000000000..12656ca7b9d4 --- /dev/null +++ b/drivers/gpio/gpio-xgs-iproc.c @@ -0,0 +1,422 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2017 Broadcom Corporation + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/irq.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/ioport.h> +#include <linux/of_device.h> +#include <linux/of_address.h> +#include <linux/of_gpio.h> + +#define CCA_INT_F_GPIOINT BIT(0) +#define CCA_INT_STS 0x20 +#define CCA_INT_MASK 0x24 + +#define GPIO_CCA_DIN 0x0 +#define GPIO_CCA_DOUT 0x4 +#define GPIO_CCA_OUT_EN 0x8 +#define GPIO_CCA_INT_LEVEL 0x10 +#define GPIO_CCA_INT_LEVEL_MASK 0x14 +#define GPIO_CCA_INT_EVENT 0x18 +#define GPIO_CCA_INT_EVENT_MASK 0x1C +#define GPIO_CCA_INT_EDGE 0x24 + +struct iproc_gpio_chip { + struct device *dev; + void __iomem *base; + void __iomem *intr; + struct gpio_chip gc; + struct irq_chip irqchip; + spinlock_t lock; + struct irq_domain *irq_domain; +}; + +static u32 iproc_gpio_readl(struct iproc_gpio_chip *chip, int reg) +{ + return readl(chip->base + reg); +} + +static void iproc_gpio_writel(struct iproc_gpio_chip *chip, u32 val, int reg) +{ + writel(val, chip->base + reg); +} + +/* returns the corresponding gpio register bit */ +static int iproc_irq_to_gpio(struct iproc_gpio_chip *chip, u32 irq) +{ + struct irq_data *data = irq_domain_get_irq_data(chip->irq_domain, irq); + + return data->hwirq; +} + +static void iproc_gpio_irq_ack(struct irq_data *d) +{ + u32 irq = d->irq; + struct iproc_gpio_chip *chip = irq_get_chip_data(irq); + int pin; + u32 irq_type, event_status = 0; + + pin = iproc_irq_to_gpio(chip, d->irq); + irq_type = irq_get_trigger_type(irq); + if (irq_type & IRQ_TYPE_EDGE_BOTH) { + event_status |= BIT(pin); + iproc_gpio_writel(chip, event_status, GPIO_CCA_INT_EVENT); + } +} + +static void iproc_gpio_irq_unmask(struct irq_data *d) +{ + u32 irq = d->irq; + struct iproc_gpio_chip *chip = irq_get_chip_data(irq); + int pin; + u32 int_mask, irq_type, event_mask; + + pin = iproc_irq_to_gpio(chip, irq); + irq_type = irq_get_trigger_type(irq); + event_mask = iproc_gpio_readl(chip, GPIO_CCA_INT_EVENT_MASK); + int_mask = iproc_gpio_readl(chip, GPIO_CCA_INT_LEVEL_MASK); + + if (irq_type & IRQ_TYPE_EDGE_BOTH) { + event_mask |= 1 << pin; + iproc_gpio_writel(chip, event_mask, GPIO_CCA_INT_EVENT_MASK); + } else { + int_mask |= 1 << pin; + iproc_gpio_writel(chip, int_mask, GPIO_CCA_INT_LEVEL_MASK); + } +} + +static void iproc_gpio_irq_mask(struct irq_data *d) +{ + u32 irq = d->irq; + struct iproc_gpio_chip *chip = irq_get_chip_data(irq); + int pin; + u32 irq_type, int_mask, event_mask; + + pin = iproc_irq_to_gpio(chip, irq); + irq_type = irq_get_trigger_type(irq); + event_mask = iproc_gpio_readl(chip, GPIO_CCA_INT_EVENT_MASK); + int_mask = iproc_gpio_readl(chip, GPIO_CCA_INT_LEVEL_MASK); + + if (irq_type & IRQ_TYPE_EDGE_BOTH) { + event_mask &= ~BIT(pin); + iproc_gpio_writel(chip, event_mask, GPIO_CCA_INT_EVENT_MASK); + } else { + int_mask &= ~BIT(pin); + iproc_gpio_writel(chip, int_mask, GPIO_CCA_INT_LEVEL_MASK); + } +} + + +static int iproc_gpio_irq_set_type(struct irq_data *d, u32 type) +{ + u32 irq = d->irq; + struct iproc_gpio_chip *chip = irq_get_chip_data(irq); + int pin; + u32 event_pol, int_pol; + + pin = iproc_irq_to_gpio(chip, irq); + + switch (type & IRQ_TYPE_SENSE_MASK) { + case IRQ_TYPE_EDGE_RISING: + event_pol = iproc_gpio_readl(chip, GPIO_CCA_INT_EDGE); + event_pol &= ~BIT(pin); + iproc_gpio_writel(chip, event_pol, GPIO_CCA_INT_EDGE); + break; + case IRQ_TYPE_EDGE_FALLING: + event_pol = iproc_gpio_readl(chip, GPIO_CCA_INT_EDGE); + event_pol |= BIT(pin); + iproc_gpio_writel(chip, event_pol, GPIO_CCA_INT_EDGE); + break; + case IRQ_TYPE_LEVEL_HIGH: + int_pol = iproc_gpio_readl(chip, GPIO_CCA_INT_LEVEL); + int_pol &= ~BIT(pin); + iproc_gpio_writel(chip, int_pol, GPIO_CCA_INT_LEVEL); + break; + case IRQ_TYPE_LEVEL_LOW: + int_pol = iproc_gpio_readl(chip, GPIO_CCA_INT_LEVEL); + int_pol |= BIT(pin); + iproc_gpio_writel(chip, int_pol, GPIO_CCA_INT_LEVEL); + break; + default: + /* should not come here */ + return -EINVAL; + } + + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH)) + irq_set_handler_locked(irq_get_irq_data(irq), handle_level_irq); + else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)) + irq_set_handler_locked(irq_get_irq_data(irq), handle_edge_irq); + + return 0; +} + +static irqreturn_t iproc_gpio_irq_handler(int irq, void *data) +{ + struct iproc_gpio_chip *chip = (struct iproc_gpio_chip *)data; + struct gpio_chip gc = chip->gc; + int bit; + unsigned long int_bits = 0; + u32 int_status; + + /* go through the entire GPIOs and handle all interrupts */ + int_status = readl(chip->intr + CCA_INT_STS); + if (int_status & CCA_INT_F_GPIOINT) { + u32 event, level; + + /* Get level and edge interrupts */ + event = readl(chip->base + GPIO_CCA_INT_EVENT_MASK); + event &= readl(chip->base + GPIO_CCA_INT_EVENT); + level = readl(chip->base + GPIO_CCA_DIN); + level ^= readl(chip->base + GPIO_CCA_INT_LEVEL); + level &= readl(chip->base + GPIO_CCA_INT_LEVEL_MASK); + int_bits = level | event; + + for_each_set_bit(bit, &int_bits, gc.ngpio) + generic_handle_irq( + irq_linear_revmap(chip->irq_domain, bit)); + } + + return int_bits ? IRQ_HANDLED : IRQ_NONE; +} + +static int iproc_gpiolib_input(struct gpio_chip *gc, u32 gpio) +{ + struct iproc_gpio_chip *chip = gpiochip_get_data(gc); + unsigned long flags; + u32 val; + + spin_lock_irqsave(&chip->lock, flags); + + val = iproc_gpio_readl(chip, GPIO_CCA_OUT_EN); + val &= ~BIT(gpio); + iproc_gpio_writel(chip, val, GPIO_CCA_OUT_EN); + + spin_unlock_irqrestore(&chip->lock, flags); + + return 0; +} + +static int iproc_gpiolib_output(struct gpio_chip *gc, u32 gpio, int value) +{ + struct iproc_gpio_chip *chip = gpiochip_get_data(gc); + unsigned long flags, val; + + spin_lock_irqsave(&chip->lock, flags); + + val = iproc_gpio_readl(chip, GPIO_CCA_OUT_EN); + val |= BIT(gpio); + iproc_gpio_writel(chip, val, GPIO_CCA_OUT_EN); + + spin_unlock_irqrestore(&chip->lock, flags); + + return 0; +} + +static void iproc_gpiolib_set(struct gpio_chip *gc, u32 gpio, int value) +{ + struct iproc_gpio_chip *chip = gpiochip_get_data(gc); + unsigned long flags, val; + + spin_lock_irqsave(&chip->lock, flags); + + val = iproc_gpio_readl(chip, GPIO_CCA_OUT_EN); + val &= BIT(gpio); + + /* this function only applies to output pin */ + if (!val) + return; + + val = iproc_gpio_readl(chip, GPIO_CCA_DOUT); + if (value == 0) + /* Set the pin to zero */ + val &= ~BIT(gpio); + else + /* Set the pin to 1 */ + val |= BIT(gpio); + + iproc_gpio_writel(chip, val, GPIO_CCA_DOUT); + + spin_unlock_irqrestore(&chip->lock, flags); +} + +static int iproc_gpiolib_get(struct gpio_chip *gc, u32 gpio) +{ + struct iproc_gpio_chip *chip = gpiochip_get_data(gc); + unsigned long flags; + u32 val, is_out; + /* GPIO register bit */ + + spin_lock_irqsave(&chip->lock, flags); + + /* determine the GPIO pin direction */ + is_out = iproc_gpio_readl(chip, GPIO_CCA_OUT_EN); + is_out &= BIT(gpio); + + if (is_out) + val = iproc_gpio_readl(chip, GPIO_CCA_DOUT); + else + val = iproc_gpio_readl(chip, GPIO_CCA_DIN); + + spin_unlock_irqrestore(&chip->lock, flags); + + return !!(val & BIT(gpio)); +} + +static int iproc_gpiolib_to_irq(struct gpio_chip *gc, u32 offset) +{ + struct iproc_gpio_chip *chip = gpiochip_get_data(gc); + + return irq_linear_revmap(chip->irq_domain, offset); +} + +static int iproc_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *dn = pdev->dev.of_node; + struct iproc_gpio_chip *chip; + u32 num_gpios; + int irq, ret; + + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + chip->dev = dev; + platform_set_drvdata(pdev, chip); + + chip->gc.label = dev_name(dev); + + chip->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(chip->base)) + return PTR_ERR(chip->base); + + /* Get number of GPIO pin */ + if (of_property_read_u32(dn, "ngpios", &num_gpios)) { + dev_err(dev, "missing ngpios DT property\n"); + return -EINVAL; + } + chip->gc.ngpio = num_gpios; + chip->gc.parent = dev; + chip->gc.of_node = dn; + chip->gc.direction_input = iproc_gpiolib_input; + chip->gc.direction_output = iproc_gpiolib_output; + chip->gc.set = iproc_gpiolib_set; + chip->gc.get = iproc_gpiolib_get; + chip->gc.to_irq = iproc_gpiolib_to_irq; + + ret = gpiochip_add_data(&chip->gc, chip); + if (ret) { + dev_err(dev, "unable to add GPIO chip\n"); + return ret; + } + + irq = platform_get_irq(pdev, 0); + if (irq > 0) { + u32 val, count; + struct irq_chip *irqc; + + irqc = &chip->irqchip; + irqc->name = dev_name(dev); + irqc->irq_ack = iproc_gpio_irq_ack; + irqc->irq_mask = iproc_gpio_irq_mask; + irqc->irq_unmask = iproc_gpio_irq_unmask; + irqc->irq_set_type = iproc_gpio_irq_set_type; + irqc->irq_enable = iproc_gpio_irq_unmask; + irqc->irq_disable = iproc_gpio_irq_mask; + + chip->intr = devm_platform_ioremap_resource(pdev, 1); + if (IS_ERR(chip->intr)) + return PTR_ERR(chip->intr); + + /* Create irq domain */ + chip->irq_domain = irq_domain_add_linear(dn, num_gpios, + &irq_domain_simple_ops, chip); + + if (!chip->irq_domain) { + dev_err(dev, "Couldn't allocate IRQ domain\n"); + ret = -ENODEV; + goto err_irq_domain; + } + + /* Map each gpio pin to an IRQ and set the handler */ + for (count = 0; count < num_gpios; count++) { + int irq; + + irq = irq_create_mapping(chip->irq_domain, count); + irq_set_chip_and_handler(irq, irqc, handle_simple_irq); + irq_set_chip_data(irq, chip); + } + + /* Enable GPIO interrupts for CCA GPIO */ + val = readl(chip->intr + CCA_INT_MASK); + val |= CCA_INT_F_GPIOINT; + writel(val, chip->intr + CCA_INT_MASK); + + /* Install ISR for this GPIO controller */ + ret = devm_request_irq(dev, irq, iproc_gpio_irq_handler, + IRQF_SHARED, chip->gc.label, chip); + if (ret) { + dev_err(dev, "Fail to request IRQ%d: %d\n", irq, ret); + goto err_irq_request; + } + } + + return 0; + +err_irq_request: + irq_domain_remove(chip->irq_domain); + chip->irq_domain = NULL; + +err_irq_domain: + gpiochip_remove(&chip->gc); + + return ret; +} + +static int __exit iproc_gpio_remove(struct platform_device *pdev) +{ + struct iproc_gpio_chip *chip; + + chip = platform_get_drvdata(pdev); + if (!chip) + return -ENODEV; + + if (chip->intr) { + u32 val; + + val = readl(chip->intr + CCA_INT_MASK); + val &= ~CCA_INT_F_GPIOINT; + writel(val, chip->intr + CCA_INT_MASK); + } + + gpiochip_remove(&chip->gc); + + return 0; +} + +static const struct of_device_id bcm_iproc_gpio_of_match[] __initconst = { + { .compatible = "brcm,iproc-gpio-cca" }, + {} +}; +MODULE_DEVICE_TABLE(of, bcm_iproc_gpio_of_match); + +static struct platform_driver bcm_iproc_gpio_driver = { + .driver = { + .name = "iproc-xgs-gpio", + .owner = THIS_MODULE, + .of_match_table = bcm_iproc_gpio_of_match, + }, + .probe = iproc_gpio_probe, + .remove = iproc_gpio_remove, +}; + +module_platform_driver(bcm_iproc_gpio_driver); + +MODULE_DESCRIPTION("XGS IPROC GPIO driver"); +MODULE_LICENSE("GPL v2"); -- 2.23.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] gpio: Add xgs-iproc driver 2019-10-04 1:25 ` [PATCH 2/2] gpio: Add xgs-iproc driver Chris Packham @ 2019-10-04 3:01 ` kbuild test robot 2019-10-04 3:30 ` kbuild test robot 2019-10-11 7:43 ` Linus Walleij 2 siblings, 0 replies; 9+ messages in thread From: kbuild test robot @ 2019-10-04 3:01 UTC (permalink / raw) To: Chris Packham Cc: mark.rutland, devicetree, f.fainelli, sbranden, linux-gpio, rjui, linus.walleij, linux-kernel, richard.laing, bgolaszewski, robh+dt, bcm-kernel-feedback-list, kbuild-all, Chris Packham, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 1296 bytes --] Hi Chris, Thank you for the patch! Yet something to improve: [auto build test ERROR on gpio/for-next] [cannot apply to v5.4-rc1 next-20191003] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Chris-Packham/gpio-brcm-XGS-iProc-GPIO-driver/20191004-093320 base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next config: ia64-allmodconfig (attached as .config) compiler: ia64-linux-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=ia64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> ERROR: "ia64_delay_loop" undefined! >> ERROR: "ia64_delay_loop" undefined! --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 54589 bytes --] [-- Attachment #3: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] gpio: Add xgs-iproc driver 2019-10-04 1:25 ` [PATCH 2/2] gpio: Add xgs-iproc driver Chris Packham 2019-10-04 3:01 ` kbuild test robot @ 2019-10-04 3:30 ` kbuild test robot 2019-10-11 7:43 ` Linus Walleij 2 siblings, 0 replies; 9+ messages in thread From: kbuild test robot @ 2019-10-04 3:30 UTC (permalink / raw) To: Chris Packham Cc: mark.rutland, devicetree, f.fainelli, sbranden, linux-gpio, rjui, linus.walleij, linux-kernel, richard.laing, bgolaszewski, robh+dt, bcm-kernel-feedback-list, kbuild-all, Chris Packham, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 1627 bytes --] Hi Chris, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on gpio/for-next] [cannot apply to v5.4-rc1 next-20191003] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Chris-Packham/gpio-brcm-XGS-iProc-GPIO-driver/20191004-093320 base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=m68k If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> WARNING: drivers/gpio/gpio-xgs-iproc.o(.data+0x2a): Section mismatch in reference from the variable bcm_iproc_gpio_driver to the variable .init.rodata:bcm_iproc_gpio_of_match The variable bcm_iproc_gpio_driver references the variable __initconst bcm_iproc_gpio_of_match If the reference is valid then annotate the variable with or __refdata (see linux/init.h) or name the variable: --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 50932 bytes --] [-- Attachment #3: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] gpio: Add xgs-iproc driver 2019-10-04 1:25 ` [PATCH 2/2] gpio: Add xgs-iproc driver Chris Packham 2019-10-04 3:01 ` kbuild test robot 2019-10-04 3:30 ` kbuild test robot @ 2019-10-11 7:43 ` Linus Walleij 2019-10-13 22:08 ` Chris Packham 2 siblings, 1 reply; 9+ messages in thread From: Linus Walleij @ 2019-10-11 7:43 UTC (permalink / raw) To: Chris Packham Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Florian Fainelli, Scott Branden, open list:GPIO SUBSYSTEM, Ray Jui, linux-kernel, richard.laing, Bartosz Golaszewski, Rob Herring, bcm-kernel-feedback-list, Linux ARM Hi Chris! Thanks for your patch! On Fri, Oct 4, 2019 at 3:25 AM Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > This driver supports the Chip Common A GPIO controller present on a > number of Broadcom switch ASICs with integrated SoCs. The controller is > similar to the pinctrl-nsp-gpio and pinctrl-iproc-gpio blocks but > different enough that a separate driver is required. > > This has been ported from Broadcom's XLDK 5.0.3 retaining only the CCA > support (pinctrl-iproc-gpio covers CCB). > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> (...) > +config GPIO_XGS_IPROC > + tristate "BRCM XGS iProc GPIO support" > + depends on OF_GPIO && (ARCH_BCM_IPROC || COMPILE_TEST) > + select GPIO_GENERIC > + select GPIOLIB_IRQCHIP Nice reuse of abstractions. > +static u32 iproc_gpio_readl(struct iproc_gpio_chip *chip, int reg) > +{ > + return readl(chip->base + reg); > +} > + > +static void iproc_gpio_writel(struct iproc_gpio_chip *chip, u32 val, int reg) > +{ > + writel(val, chip->base + reg); > +} These wrappers don't really add anything do they? Just inline the direct readl()/writel() to base + reg everywhere instead. > +/* returns the corresponding gpio register bit */ > +static int iproc_irq_to_gpio(struct iproc_gpio_chip *chip, u32 irq) > +{ > + struct irq_data *data = irq_domain_get_irq_data(chip->irq_domain, irq); > + > + return data->hwirq; > +} I would name it something clearer since "gpio" is pretty ambigous. Like iproc_irq_to_gpio_offset() Maybe this is also a bit of unnecessary wrapper? > +static irqreturn_t iproc_gpio_irq_handler(int irq, void *data) > +{ > + struct iproc_gpio_chip *chip = (struct iproc_gpio_chip *)data; > + struct gpio_chip gc = chip->gc; > + int bit; > + unsigned long int_bits = 0; > + u32 int_status; > + > + /* go through the entire GPIOs and handle all interrupts */ > + int_status = readl(chip->intr + CCA_INT_STS); > + if (int_status & CCA_INT_F_GPIOINT) { > + u32 event, level; > + > + /* Get level and edge interrupts */ > + event = readl(chip->base + GPIO_CCA_INT_EVENT_MASK); > + event &= readl(chip->base + GPIO_CCA_INT_EVENT); > + level = readl(chip->base + GPIO_CCA_DIN); > + level ^= readl(chip->base + GPIO_CCA_INT_LEVEL); > + level &= readl(chip->base + GPIO_CCA_INT_LEVEL_MASK); > + int_bits = level | event; > + > + for_each_set_bit(bit, &int_bits, gc.ngpio) > + generic_handle_irq( > + irq_linear_revmap(chip->irq_domain, bit)); > + } > + > + return int_bits ? IRQ_HANDLED : IRQ_NONE; > +} I think this should be a chained interrupt handler (see below how to register it). See e.g. drivers/gpio/gpio-ftgpio010.c for an example: change function prototype, no return value, use chained_irq_enter/exit(irqchip, desc); etc. > +static int iproc_gpiolib_input(struct gpio_chip *gc, u32 gpio) > +static int iproc_gpiolib_output(struct gpio_chip *gc, u32 gpio, int value) > +static void iproc_gpiolib_set(struct gpio_chip *gc, u32 gpio, int value) > +static int iproc_gpiolib_get(struct gpio_chip *gc, u32 gpio) These callbacks seems to reimplement parts of GPIO_GENERIC that you should already be using. Again look at drivers/gpio/gpio-ftgpio010.c() use bgpio_init() to set up the library callbacks, look in drivers/gpio/gpio-mmio.c for kerneldoc on the function. > +static int iproc_gpiolib_to_irq(struct gpio_chip *gc, u32 offset) > +{ > + struct iproc_gpio_chip *chip = gpiochip_get_data(gc); > + > + return irq_linear_revmap(chip->irq_domain, offset); > +} GPIOLIB_IRQCHIP provides a .to_irq() implementation so drop this and let the library handle this. > +static int iproc_gpio_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *dn = pdev->dev.of_node; > + struct iproc_gpio_chip *chip; > + u32 num_gpios; > + int irq, ret; > + > + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + chip->dev = dev; > + platform_set_drvdata(pdev, chip); > + > + chip->gc.label = dev_name(dev); > + > + chip->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(chip->base)) > + return PTR_ERR(chip->base); > + > + /* Get number of GPIO pin */ > + if (of_property_read_u32(dn, "ngpios", &num_gpios)) { > + dev_err(dev, "missing ngpios DT property\n"); > + return -EINVAL; > + } Maybe provide a sensible default? > + chip->gc.ngpio = num_gpios; > + chip->gc.parent = dev; > + chip->gc.of_node = dn; > + chip->gc.direction_input = iproc_gpiolib_input; > + chip->gc.direction_output = iproc_gpiolib_output; > + chip->gc.set = iproc_gpiolib_set; > + chip->gc.get = iproc_gpiolib_get; Drop this and call bgpio_init() to set up the callbacks instead. However, set up .ngpio *after* calling bgpio_init() so users can't access the nonexisting gpios. > + chip->gc.to_irq = iproc_gpiolib_to_irq; Drop this and use the GPIOLIB_IRQCHIP. > + ret = gpiochip_add_data(&chip->gc, chip); > + if (ret) { > + dev_err(dev, "unable to add GPIO chip\n"); > + return ret; > + } Why not use devm_gpiochip_add_data()? > + irq = platform_get_irq(pdev, 0); > + if (irq > 0) { > + u32 val, count; > + struct irq_chip *irqc; > + > + irqc = &chip->irqchip; > + irqc->name = dev_name(dev); > + irqc->irq_ack = iproc_gpio_irq_ack; > + irqc->irq_mask = iproc_gpio_irq_mask; > + irqc->irq_unmask = iproc_gpio_irq_unmask; > + irqc->irq_set_type = iproc_gpio_irq_set_type; > + irqc->irq_enable = iproc_gpio_irq_unmask; > + irqc->irq_disable = iproc_gpio_irq_mask; > + chip->intr = devm_platform_ioremap_resource(pdev, 1); > + if (IS_ERR(chip->intr)) > + return PTR_ERR(chip->intr); Move all this above the [devm_]gpiochip_add_data() call and add: struct gpio_irq_chip *girq; girq = &chip->gc.irq; girq->chip = irqc; etc follow the pattern from other drivers like the mentioned ftgpio010.c. > + /* Create irq domain */ > + chip->irq_domain = irq_domain_add_linear(dn, num_gpios, > + &irq_domain_simple_ops, chip); > + > + if (!chip->irq_domain) { > + dev_err(dev, "Couldn't allocate IRQ domain\n"); > + ret = -ENODEV; > + goto err_irq_domain; > + } > + > + /* Map each gpio pin to an IRQ and set the handler */ > + for (count = 0; count < num_gpios; count++) { > + int irq; > + > + irq = irq_create_mapping(chip->irq_domain, count); > + irq_set_chip_and_handler(irq, irqc, handle_simple_irq); > + irq_set_chip_data(irq, chip); > + } Drop this and let the generic GPIO irqchip handle the domain. > + /* Enable GPIO interrupts for CCA GPIO */ > + val = readl(chip->intr + CCA_INT_MASK); > + val |= CCA_INT_F_GPIOINT; > + writel(val, chip->intr + CCA_INT_MASK); Move this before registering the chip. > + /* Install ISR for this GPIO controller */ > + ret = devm_request_irq(dev, irq, iproc_gpio_irq_handler, > + IRQF_SHARED, chip->gc.label, chip); > + if (ret) { > + dev_err(dev, "Fail to request IRQ%d: %d\n", irq, ret); > + goto err_irq_request; > + } Drop this and use the gpiolib irqchip library. Yours, Linus Walleij _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] gpio: Add xgs-iproc driver 2019-10-11 7:43 ` Linus Walleij @ 2019-10-13 22:08 ` Chris Packham 2019-10-16 12:53 ` Linus Walleij 0 siblings, 1 reply; 9+ messages in thread From: Chris Packham @ 2019-10-13 22:08 UTC (permalink / raw) To: linus.walleij Cc: mark.rutland, devicetree, f.fainelli, sbranden, bgolaszewski, rjui, linux-kernel, Richard Laing, linux-gpio, robh+dt, bcm-kernel-feedback-list, linux-arm-kernel On Fri, 2019-10-11 at 09:43 +0200, Linus Walleij wrote: > Hi Chris! > > Thanks for your patch! > > On Fri, Oct 4, 2019 at 3:25 AM Chris Packham > <chris.packham@alliedtelesis.co.nz> wrote: > > > This driver supports the Chip Common A GPIO controller present on a > > number of Broadcom switch ASICs with integrated SoCs. The controller is > > similar to the pinctrl-nsp-gpio and pinctrl-iproc-gpio blocks but > > different enough that a separate driver is required. > > > > This has been ported from Broadcom's XLDK 5.0.3 retaining only the CCA > > support (pinctrl-iproc-gpio covers CCB). > > > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > (...) > > > +config GPIO_XGS_IPROC > > + tristate "BRCM XGS iProc GPIO support" > > + depends on OF_GPIO && (ARCH_BCM_IPROC || COMPILE_TEST) > > + select GPIO_GENERIC > > + select GPIOLIB_IRQCHIP > > Nice reuse of abstractions. > > > +static u32 iproc_gpio_readl(struct iproc_gpio_chip *chip, int reg) > > +{ > > + return readl(chip->base + reg); > > +} > > + > > +static void iproc_gpio_writel(struct iproc_gpio_chip *chip, u32 val, int reg) > > +{ > > + writel(val, chip->base + reg); > > +} > > These wrappers don't really add anything do they? Just inline the > direct readl()/writel() to base + reg everywhere instead. > Will do. > > +/* returns the corresponding gpio register bit */ > > +static int iproc_irq_to_gpio(struct iproc_gpio_chip *chip, u32 irq) > > +{ > > + struct irq_data *data = irq_domain_get_irq_data(chip->irq_domain, irq); > > + > > + return data->hwirq; > > +} > > I would name it something clearer since "gpio" is pretty ambigous. > > Like iproc_irq_to_gpio_offset() > > Maybe this is also a bit of unnecessary wrapper? > I'll look into it. We might already have the irq_data we need so the callers could use "pin = d->hwirq;". > > +static irqreturn_t iproc_gpio_irq_handler(int irq, void *data) > > +{ > > + struct iproc_gpio_chip *chip = (struct iproc_gpio_chip *)data; > > + struct gpio_chip gc = chip->gc; > > + int bit; > > + unsigned long int_bits = 0; > > + u32 int_status; > > + > > + /* go through the entire GPIOs and handle all interrupts */ > > + int_status = readl(chip->intr + CCA_INT_STS); > > + if (int_status & CCA_INT_F_GPIOINT) { > > + u32 event, level; > > + > > + /* Get level and edge interrupts */ > > + event = readl(chip->base + GPIO_CCA_INT_EVENT_MASK); > > + event &= readl(chip->base + GPIO_CCA_INT_EVENT); > > + level = readl(chip->base + GPIO_CCA_DIN); > > + level ^= readl(chip->base + GPIO_CCA_INT_LEVEL); > > + level &= readl(chip->base + GPIO_CCA_INT_LEVEL_MASK); > > + int_bits = level | event; > > + > > + for_each_set_bit(bit, &int_bits, gc.ngpio) > > + generic_handle_irq( > > + irq_linear_revmap(chip->irq_domain, bit)); > > + } > > + > > + return int_bits ? IRQ_HANDLED : IRQ_NONE; > > +} > > I think this should be a chained interrupt handler (see below how to > register it). > > See e.g. drivers/gpio/gpio-ftgpio010.c for an example: > change function prototype, no return value, use > chained_irq_enter/exit(irqchip, desc); etc. > I don't think a chained interrupt handler can work. The problem is that the parent irq on the SoC is shared between the gpio and uart0 (why it's this way with two IP blocks in the same SoC I'll never know). When a chained interrupt handler is registered I lose the serial interrupts. Please correct me if there is some way to make the chained handlers deal with sharing interrupts. > > +static int iproc_gpiolib_input(struct gpio_chip *gc, u32 gpio) > > +static int iproc_gpiolib_output(struct gpio_chip *gc, u32 gpio, int value) > > +static void iproc_gpiolib_set(struct gpio_chip *gc, u32 gpio, int value) > > +static int iproc_gpiolib_get(struct gpio_chip *gc, u32 gpio) > > These callbacks seems to reimplement parts of GPIO_GENERIC > that you should already be using. > > Again look at drivers/gpio/gpio-ftgpio010.c() use bgpio_init() > to set up the library callbacks, look in > drivers/gpio/gpio-mmio.c for kerneldoc on the function. > Will look into it. > > +static int iproc_gpiolib_to_irq(struct gpio_chip *gc, u32 offset) > > +{ > > + struct iproc_gpio_chip *chip = gpiochip_get_data(gc); > > + > > + return irq_linear_revmap(chip->irq_domain, offset); > > +} > > GPIOLIB_IRQCHIP provides a .to_irq() implementation > so drop this and let the library handle this. > Will that work if I've squirreled the irq_domain away in struct iproc_gpio_chip? See above as to why I'm not using the embedded gpio_irq_chip. > > +static int iproc_gpio_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *dn = pdev->dev.of_node; > > + struct iproc_gpio_chip *chip; > > + u32 num_gpios; > > + int irq, ret; > > + > > + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > > + if (!chip) > > + return -ENOMEM; > > + > > + chip->dev = dev; > > + platform_set_drvdata(pdev, chip); > > + > > + chip->gc.label = dev_name(dev); > > + > > + chip->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(chip->base)) > > + return PTR_ERR(chip->base); > > + > > + /* Get number of GPIO pin */ > > + if (of_property_read_u32(dn, "ngpios", &num_gpios)) { > > + dev_err(dev, "missing ngpios DT property\n"); > > + return -EINVAL; > > + } > > Maybe provide a sensible default? > I don't know that there is one. On the particular SoC I have the number is 12 but the datasheets I have access to are really hard to follow (the SoC part is tacked on to the end of the datasheet for the switch ASIC). 32 would be vaguely sane since they're all 32-bit wide registers. > > + chip->gc.ngpio = num_gpios; > > + chip->gc.parent = dev; > > + chip->gc.of_node = dn; > > + chip->gc.direction_input = iproc_gpiolib_input; > > + chip->gc.direction_output = iproc_gpiolib_output; > > + chip->gc.set = iproc_gpiolib_set; > > + chip->gc.get = iproc_gpiolib_get; > > Drop this and call bgpio_init() to set up the callbacks > instead. However, set up .ngpio *after* calling > bgpio_init() so users can't access the nonexisting > gpios. > > > + chip->gc.to_irq = iproc_gpiolib_to_irq; > > Drop this and use the GPIOLIB_IRQCHIP. > > > + ret = gpiochip_add_data(&chip->gc, chip); > > + if (ret) { > > + dev_err(dev, "unable to add GPIO chip\n"); > > + return ret; > > + } > > Why not use devm_gpiochip_add_data()? > > > + irq = platform_get_irq(pdev, 0); > > + if (irq > 0) { > > + u32 val, count; > > + struct irq_chip *irqc; > > + > > + irqc = &chip->irqchip; > > + irqc->name = dev_name(dev); > > + irqc->irq_ack = iproc_gpio_irq_ack; > > + irqc->irq_mask = iproc_gpio_irq_mask; > > + irqc->irq_unmask = iproc_gpio_irq_unmask; > > + irqc->irq_set_type = iproc_gpio_irq_set_type; > > + irqc->irq_enable = iproc_gpio_irq_unmask; > > + irqc->irq_disable = iproc_gpio_irq_mask; > > + chip->intr = devm_platform_ioremap_resource(pdev, 1); > > + if (IS_ERR(chip->intr)) > > + return PTR_ERR(chip->intr); > > Move all this above the [devm_]gpiochip_add_data() > call and add: > > struct gpio_irq_chip *girq; > > girq = &chip->gc.irq; > girq->chip = irqc; > > etc follow the pattern from other drivers like the > mentioned ftgpio010.c. > > > + /* Create irq domain */ > > + chip->irq_domain = irq_domain_add_linear(dn, num_gpios, > > + &irq_domain_simple_ops, chip); > > + > > + if (!chip->irq_domain) { > > + dev_err(dev, "Couldn't allocate IRQ domain\n"); > > + ret = -ENODEV; > > + goto err_irq_domain; > > + } > > + > > + /* Map each gpio pin to an IRQ and set the handler */ > > + for (count = 0; count < num_gpios; count++) { > > + int irq; > > + > > + irq = irq_create_mapping(chip->irq_domain, count); > > + irq_set_chip_and_handler(irq, irqc, handle_simple_irq); > > + irq_set_chip_data(irq, chip); > > + } > > Drop this and let the generic GPIO irqchip handle > the domain. > > > + /* Enable GPIO interrupts for CCA GPIO */ > > + val = readl(chip->intr + CCA_INT_MASK); > > + val |= CCA_INT_F_GPIOINT; > > + writel(val, chip->intr + CCA_INT_MASK); > > Move this before registering the chip. > > > + /* Install ISR for this GPIO controller */ > > + ret = devm_request_irq(dev, irq, iproc_gpio_irq_handler, > > + IRQF_SHARED, chip->gc.label, chip); > > + if (ret) { > > + dev_err(dev, "Fail to request IRQ%d: %d\n", irq, ret); > > + goto err_irq_request; > > + } > > Drop this and use the gpiolib irqchip library. As I've said above I'm not sure I can due to sharing interrupts with uart0. I'd be happy to be proven wrong as the generic irq code removes a great deal of boiler plate for me. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] gpio: Add xgs-iproc driver 2019-10-13 22:08 ` Chris Packham @ 2019-10-16 12:53 ` Linus Walleij 0 siblings, 0 replies; 9+ messages in thread From: Linus Walleij @ 2019-10-16 12:53 UTC (permalink / raw) To: Chris Packham Cc: mark.rutland, devicetree, f.fainelli, sbranden, bgolaszewski, rjui, linux-kernel, Richard Laing, linux-gpio, robh+dt, bcm-kernel-feedback-list, linux-arm-kernel On Mon, Oct 14, 2019 at 12:08 AM Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote: Me: > > I think this should be a chained interrupt handler (see below how to > > register it). > > > > See e.g. drivers/gpio/gpio-ftgpio010.c for an example: > > change function prototype, no return value, use > > chained_irq_enter/exit(irqchip, desc); etc. > > > > I don't think a chained interrupt handler can work. The problem is that > the parent irq on the SoC is shared between the gpio and uart0 (why > it's this way with two IP blocks in the same SoC I'll never know). When > a chained interrupt handler is registered I lose the serial interrupts. > Please correct me if there is some way to make the chained handlers > deal with sharing interrupts. Aha I see. Look at: drivers/gpio/gpio-mt7621.c And how that driver sets the parent handler to NULL in order to still exploit the core helpers. I will refactor this to some more elegant API at some point when I get there, for now follow the example of mt7621. Yours, Linus Walleij _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-10-16 12:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-04 1:25 [PATCH 0/2] gpio: brcm: XGS iProc GPIO driver Chris Packham 2019-10-04 1:25 ` [PATCH 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc Chris Packham 2019-10-15 19:23 ` Rob Herring 2019-10-04 1:25 ` [PATCH 2/2] gpio: Add xgs-iproc driver Chris Packham 2019-10-04 3:01 ` kbuild test robot 2019-10-04 3:30 ` kbuild test robot 2019-10-11 7:43 ` Linus Walleij 2019-10-13 22:08 ` Chris Packham 2019-10-16 12:53 ` Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).