* [PATCH v3 0/1] Add Polarfire SoC GPIO support @ 2022-07-16 7:11 lewis.hanly 2022-07-16 7:11 ` [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support lewis.hanly 0 siblings, 1 reply; 12+ messages in thread From: lewis.hanly @ 2022-07-16 7:11 UTC (permalink / raw) To: linux-gpio, linux-riscv, linus.walleij, brgl, linux-kernel, palmer, maz Cc: conor.dooley, daire.mcnamara, lewis.hanly From: Lewis Hanly <lewis.hanly@microchip.com> Add a driver to support the Polarfire SoC gpio controller. Tested with 5.19-rc5 MPFS gpio interrupts can be configured as direct or non direct connections to the PLIC (Platform Level Interrupt Controller). GPIO_INTERRUPT_FAB_CR(31:0) system register will enable GPIO2(31:0) corresponding interrupt on PLIC. e.g. If GPIO_INTERRUPT_FAB_CR bit0 is set then GPIO2 bit0 interrupt is available on the direct input pin on the PLIC. Changes in v3: Changed order in kconfig. Removed blank lines in driver header/source file. Removed BYTE_BOUNDARY variable and use macro to do *4. mpfs_gpio_assign_bit parameter uses macro instead of (i * BYTE_BOUNDARY). Add correct definitions for direction. Change order of variables in mpfs_gpio_irq_set_type function. Return dev_err_probe instead of dev_err. Remove noise of dev_inf. Avoid using of_match_ptr. use devm_gpiochip_add_data(..) Update mpfs_gpio_remove. Changes in v2: Use raw_spinlock. Use __assign_bit() to assign bit, added a bool variable for value. Remove unnecessary checking gpio_index. Remove default from switch statement. Use const for irq_chip, name updated and use mask/unmask. Use latest kernel api irq set_chip. Implemented hierarchical interrupt chip support, although suggested to use chained interrupt flow I believe this fits better. Lewis Hanly (1): gpio: mpfs: add polarfire soc gpio support drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-mpfs.c | 361 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 371 insertions(+) create mode 100644 drivers/gpio/gpio-mpfs.c -- 2.25.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 7:11 [PATCH v3 0/1] Add Polarfire SoC GPIO support lewis.hanly @ 2022-07-16 7:11 ` lewis.hanly 2022-07-16 10:33 ` Marc Zyngier 2022-07-16 10:44 ` Marc Zyngier 0 siblings, 2 replies; 12+ messages in thread From: lewis.hanly @ 2022-07-16 7:11 UTC (permalink / raw) To: linux-gpio, linux-riscv, linus.walleij, brgl, linux-kernel, palmer, maz Cc: conor.dooley, daire.mcnamara, lewis.hanly From: Lewis Hanly <lewis.hanly@microchip.com> Add a driver to support the Polarfire SoC gpio controller. Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> --- drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-mpfs.c | 361 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 371 insertions(+) create mode 100644 drivers/gpio/gpio-mpfs.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index b01961999ced..86b1e5557482 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -490,6 +490,15 @@ config GPIO_PMIC_EIC_SPRD help Say yes here to support Spreadtrum PMIC EIC device. +config GPIO_POLARFIRE_SOC + bool "Microchip FPGA GPIO support" + depends on OF_GPIO + select IRQ_DOMAIN_HIERARCHY + select GPIOLIB_IRQCHIP + select GPIO_GENERIC + help + Say yes here to support the GPIO device on Microchip FPGAs. + config GPIO_PXA bool "PXA GPIO support" depends on ARCH_PXA || ARCH_MMP || COMPILE_TEST diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 14352f6dfe8e..3b8b6703e593 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -119,6 +119,7 @@ obj-$(CONFIG_GPIO_PCI_IDIO_16) += gpio-pci-idio-16.o obj-$(CONFIG_GPIO_PISOSR) += gpio-pisosr.o obj-$(CONFIG_GPIO_PL061) += gpio-pl061.o obj-$(CONFIG_GPIO_PMIC_EIC_SPRD) += gpio-pmic-eic-sprd.o +obj-$(CONFIG_GPIO_POLARFIRE_SOC) += gpio-mpfs.o obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RASPBERRYPI_EXP) += gpio-raspberrypi-exp.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o diff --git a/drivers/gpio/gpio-mpfs.c b/drivers/gpio/gpio-mpfs.c new file mode 100644 index 000000000000..5806abc5cfb8 --- /dev/null +++ b/drivers/gpio/gpio-mpfs.c @@ -0,0 +1,361 @@ +// SPDX-License-Identifier: (GPL-2.0) +/* + * Microchip PolarFire SoC (MPFS) GPIO controller driver + * + * Copyright (c) 2018-2022 Microchip Technology Inc. and its subsidiaries + * + * Author: Lewis Hanly <lewis.hanly@microchip.com> + */ + +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/gpio/driver.h> +#include <linux/errno.h> +#include <linux/init.h> +#include <linux/irq.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/mod_devicetable.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> + +#define MPFS_GPIO_CTRL(i) (0x4 * (i)) +#define NUM_GPIO 32 +#define MPFS_GPIO_EN_INT 3 +#define MPFS_GPIO_EN_OUT_BUF BIT(2) +#define MPFS_GPIO_EN_IN BIT(1) +#define MPFS_GPIO_EN_OUT BIT(0) + +#define MPFS_GPIO_TYPE_INT_EDGE_BOTH 0x80 +#define MPFS_GPIO_TYPE_INT_EDGE_NEG 0x60 +#define MPFS_GPIO_TYPE_INT_EDGE_POS 0x40 +#define MPFS_GPIO_TYPE_INT_LEVEL_LOW 0x20 +#define MPFS_GPIO_TYPE_INT_LEVEL_HIGH 0x00 +#define MPFS_GPIO_TYPE_INT_MASK GENMASK(7, 5) +#define MPFS_IRQ_REG 0x80 +#define MPFS_INP_REG 0x84 +#define MPFS_OUTP_REG 0x88 + +struct mpfs_gpio_chip { + void __iomem *base; + struct clk *clk; + raw_spinlock_t lock; + struct gpio_chip gc; + unsigned int irq_number[NUM_GPIO]; +}; + +static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, bool value) +{ + unsigned long reg = readl(addr); + + __assign_bit(bit_offset, ®, value); + writel(reg, addr); +} + +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + u32 gpio_cfg; + unsigned long flags; + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + + gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index)); + gpio_cfg |= MPFS_GPIO_EN_IN; + gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF); + writel(gpio_cfg, mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index)); + + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); + + return 0; +} + +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + u32 gpio_cfg; + unsigned long flags; + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + + gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index)); + gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF; + gpio_cfg &= ~MPFS_GPIO_EN_IN; + writel(gpio_cfg, mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index)); + + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG, gpio_index, value); + + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); + + return 0; +} + +static int mpfs_gpio_get_direction(struct gpio_chip *gc, + unsigned int gpio_index) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + u32 gpio_cfg; + + gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index)); + if (gpio_cfg & MPFS_GPIO_EN_IN) + return GPIO_LINE_DIRECTION_IN; + + return GPIO_LINE_DIRECTION_OUT; +} + +static int mpfs_gpio_get(struct gpio_chip *gc, + unsigned int gpio_index) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + + return !!(readl(mpfs_gpio->base + MPFS_INP_REG) & BIT(gpio_index)); +} + +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + unsigned long flags; + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG, + gpio_index, value); + + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); +} + +static int mpfs_gpio_irq_set_type(struct irq_data *data, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + int gpio_index = irqd_to_hwirq(data); + u32 interrupt_type; + u32 gpio_cfg; + unsigned long flags; + + switch (type) { + case IRQ_TYPE_EDGE_BOTH: + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH; + break; + case IRQ_TYPE_EDGE_FALLING: + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_NEG; + break; + case IRQ_TYPE_EDGE_RISING: + interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_POS; + break; + case IRQ_TYPE_LEVEL_HIGH: + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_HIGH; + break; + case IRQ_TYPE_LEVEL_LOW: + interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW; + break; + } + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + + gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index)); + gpio_cfg &= ~MPFS_GPIO_TYPE_INT_MASK; + gpio_cfg |= interrupt_type; + writel(gpio_cfg, mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index)); + + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); + + return 0; +} + +static void mpfs_gpio_irq_enable(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(data); + int gpio_index = hwirq % NUM_GPIO; + + gpiochip_enable_irq(gc, hwirq); + irq_chip_enable_parent(data); + + mpfs_gpio_direction_input(gc, gpio_index); + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1); + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index), + MPFS_GPIO_EN_INT, 1); +} + +static void mpfs_gpio_irq_disable(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(data); + int gpio_index = hwirq % NUM_GPIO; + + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1); + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index), + MPFS_GPIO_EN_INT, 0); + + irq_chip_disable_parent(data); + gpiochip_disable_irq(gc, hwirq); +} + +static void mpfs_gpio_irq_eoi(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(data) % NUM_GPIO; + unsigned long flags; + + raw_spin_lock_irqsave(&mpfs_gpio->lock, flags); + /* Clear pending interrupt */ + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1); + raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags); + + irq_chip_eoi_parent(data); +} + +static int mpfs_gpio_irq_set_affinity(struct irq_data *data, + const struct cpumask *dest, + bool force) +{ + if (data->parent_data) + return irq_chip_set_affinity_parent(data, dest, force); + + return -EINVAL; +} + +static const struct irq_chip mpfs_gpio_irqchip = { + .name = "mpfs", + .irq_set_type = mpfs_gpio_irq_set_type, + .irq_mask = irq_chip_mask_parent, + .irq_unmask = irq_chip_unmask_parent, + .irq_enable = mpfs_gpio_irq_enable, + .irq_disable = mpfs_gpio_irq_disable, + .irq_eoi = mpfs_gpio_irq_eoi, + .irq_set_affinity = mpfs_gpio_irq_set_affinity, + .flags = IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc, + unsigned int child, + unsigned int child_type, + unsigned int *parent, + unsigned int *parent_type) +{ + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); + struct irq_data *d = irq_get_irq_data(mpfs_gpio->irq_number[child]); + *parent_type = IRQ_TYPE_NONE; + *parent = irqd_to_hwirq(d); + + return 0; +} + +static int mpfs_gpio_probe(struct platform_device *pdev) +{ + struct clk *clk; + struct device *dev = &pdev->dev; + struct device_node *node = pdev->dev.of_node; + struct device_node *irq_parent; + struct gpio_irq_chip *girq; + struct irq_domain *parent; + struct mpfs_gpio_chip *mpfs_gpio; + int i, ret, ngpio; + + mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL); + if (!mpfs_gpio) + return -ENOMEM; + + mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(mpfs_gpio->base)) + return dev_err_probe(dev, PTR_ERR(mpfs_gpio->clk), "input clock not found.\n"); + + clk = devm_clk_get(dev, NULL); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), "devm_clk_get failed\n"); + + ret = clk_prepare_enable(clk); + if (ret) + return dev_err_probe(dev, ret, "failed to enable clock\n"); + + mpfs_gpio->clk = clk; + + ngpio = of_irq_count(node); + if (ngpio > NUM_GPIO) { + ret = -ENXIO; + goto cleanup_clock; + } + + irq_parent = of_irq_find_parent(node); + if (!irq_parent) { + ret = -ENODEV; + goto cleanup_clock; + } + parent = irq_find_host(irq_parent); + if (!parent) { + ret = -ENODEV; + goto cleanup_clock; + } + + /* Get the interrupt numbers. */ + /* Clear/Disable All interrupts before enabling parent interrupts. */ + for (i = 0; i < ngpio; i++) { + mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i); + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, i, 1); + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_GPIO_CTRL(i), + MPFS_GPIO_EN_INT, 0); + } + + raw_spin_lock_init(&mpfs_gpio->lock); + + mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input; + mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output; + mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction; + mpfs_gpio->gc.get = mpfs_gpio_get; + mpfs_gpio->gc.set = mpfs_gpio_set; + mpfs_gpio->gc.base = -1; + mpfs_gpio->gc.ngpio = ngpio; + mpfs_gpio->gc.label = dev_name(dev); + mpfs_gpio->gc.parent = dev; + mpfs_gpio->gc.owner = THIS_MODULE; + + girq = &mpfs_gpio->gc.irq; + gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip); + girq->fwnode = of_node_to_fwnode(node); + girq->parent_domain = parent; + girq->child_to_parent_hwirq = mpfs_gpio_child_to_parent_hwirq; + girq->handler = handle_bad_irq; + girq->default_type = IRQ_TYPE_NONE; + + ret = devm_gpiochip_add_data(dev, &mpfs_gpio->gc, mpfs_gpio); + if (ret) + goto cleanup_clock; + + platform_set_drvdata(pdev, mpfs_gpio); + + return 0; + +cleanup_clock: + clk_disable_unprepare(mpfs_gpio->clk); + return ret; +} + +static int mpfs_gpio_remove(struct platform_device *pdev) +{ + struct mpfs_gpio_chip *mpfs_gpio = platform_get_drvdata(pdev); + + clk_disable_unprepare(mpfs_gpio->clk); + return 0; +} + +static const struct of_device_id mpfs_of_ids[] = { + { .compatible = "microchip,mpfs-gpio", }, + { /* end of list */ } +}; + +static struct platform_driver mpfs_gpio_driver = { + .probe = mpfs_gpio_probe, + .driver = { + .name = "microchip,mpfs-gpio", + .of_match_table = mpfs_of_ids, + }, + .remove = mpfs_gpio_remove, +}; +builtin_platform_driver(mpfs_gpio_driver); -- 2.25.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 7:11 ` [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support lewis.hanly @ 2022-07-16 10:33 ` Marc Zyngier 2022-07-16 15:21 ` Lewis.Hanly 2022-07-31 8:56 ` Lewis.Hanly 2022-07-16 10:44 ` Marc Zyngier 1 sibling, 2 replies; 12+ messages in thread From: Marc Zyngier @ 2022-07-16 10:33 UTC (permalink / raw) To: lewis.hanly Cc: linux-gpio, linux-riscv, linus.walleij, brgl, linux-kernel, palmer, conor.dooley, daire.mcnamara On Sat, 16 Jul 2022 08:11:13 +0100, <lewis.hanly@microchip.com> wrote: > > From: Lewis Hanly <lewis.hanly@microchip.com> > > Add a driver to support the Polarfire SoC gpio controller. > > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> [...] > +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc, > + unsigned int child, > + unsigned int child_type, > + unsigned int *parent, > + unsigned int *parent_type) > +{ > + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); > + struct irq_data *d = irq_get_irq_data(mpfs_gpio->irq_number[child]); This looks totally wrong. It means that you have already instantiated part of the hierarchy, and it is likely that you will get multiple hierarchy sharing some levels, which isn't intended. > + *parent_type = IRQ_TYPE_NONE; > + *parent = irqd_to_hwirq(d); > + > + return 0; > +} > + > +static int mpfs_gpio_probe(struct platform_device *pdev) > +{ > + struct clk *clk; > + struct device *dev = &pdev->dev; > + struct device_node *node = pdev->dev.of_node; > + struct device_node *irq_parent; > + struct gpio_irq_chip *girq; > + struct irq_domain *parent; > + struct mpfs_gpio_chip *mpfs_gpio; > + int i, ret, ngpio; > + > + mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL); > + if (!mpfs_gpio) > + return -ENOMEM; > + > + mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(mpfs_gpio->base)) > + return dev_err_probe(dev, PTR_ERR(mpfs_gpio->clk), "input clock not found.\n"); > + > + clk = devm_clk_get(dev, NULL); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), "devm_clk_get failed\n"); > + > + ret = clk_prepare_enable(clk); > + if (ret) > + return dev_err_probe(dev, ret, "failed to enable clock\n"); > + > + mpfs_gpio->clk = clk; > + > + ngpio = of_irq_count(node); > + if (ngpio > NUM_GPIO) { > + ret = -ENXIO; > + goto cleanup_clock; > + } > + > + irq_parent = of_irq_find_parent(node); > + if (!irq_parent) { > + ret = -ENODEV; > + goto cleanup_clock; > + } > + parent = irq_find_host(irq_parent); > + if (!parent) { > + ret = -ENODEV; > + goto cleanup_clock; > + } > + > + /* Get the interrupt numbers. */ > + /* Clear/Disable All interrupts before enabling parent interrupts. */ > + for (i = 0; i < ngpio; i++) { > + mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i); Bingo. You are allocating the interrupt for the level below. You really shouldn't do that. If you need to retrieve the *hwirq* for the level below, you need to parse the DT without triggering an IRQ allocation (of_irq_parse_one() and co). M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 10:33 ` Marc Zyngier @ 2022-07-16 15:21 ` Lewis.Hanly 2022-07-16 17:52 ` Marc Zyngier 2022-07-31 8:56 ` Lewis.Hanly 1 sibling, 1 reply; 12+ messages in thread From: Lewis.Hanly @ 2022-07-16 15:21 UTC (permalink / raw) To: maz Cc: linux-riscv, Conor.Dooley, brgl, linux-gpio, linus.walleij, palmer, linux-kernel, Daire.McNamara Thanks Marc, On Sat, 2022-07-16 at 11:33 +0100, Marc Zyngier wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Sat, 16 Jul 2022 08:11:13 +0100, > <lewis.hanly@microchip.com> wrote: > > From: Lewis Hanly <lewis.hanly@microchip.com> > > > > Add a driver to support the Polarfire SoC gpio controller. > > > > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> > > [...] > > > +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc, > > + unsigned int child, > > + unsigned int child_type, > > + unsigned int *parent, > > + unsigned int *parent_type) > > +{ > > + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); > > + struct irq_data *d = irq_get_irq_data(mpfs_gpio- > > >irq_number[child]); > > This looks totally wrong. It means that you have already instantiated > part of the hierarchy, and it is likely that you will get multiple > hierarchy sharing some levels, which isn't intended. Some background why I use the above. We need to support both direct and non-direct IRQ connections to the PLIC. In direct mode the GPIO IRQ's are connected directly to the PLIC and certainly no need for the above. GPIO's can also be configured in non- direct, which means they use a shared IRQ, hence the above. > > > + *parent_type = IRQ_TYPE_NONE; > > + *parent = irqd_to_hwirq(d); > > + > > + return 0; > > +} > > + > > +static int mpfs_gpio_probe(struct platform_device *pdev) > > +{ > > + struct clk *clk; > > + struct device *dev = &pdev->dev; > > + struct device_node *node = pdev->dev.of_node; > > + struct device_node *irq_parent; > > + struct gpio_irq_chip *girq; > > + struct irq_domain *parent; > > + struct mpfs_gpio_chip *mpfs_gpio; > > + int i, ret, ngpio; > > + > > + mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), > > GFP_KERNEL); > > + if (!mpfs_gpio) > > + return -ENOMEM; > > + > > + mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(mpfs_gpio->base)) > > + return dev_err_probe(dev, PTR_ERR(mpfs_gpio->clk), > > "input clock not found.\n"); > > + > > + clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(clk)) > > + return dev_err_probe(dev, PTR_ERR(clk), "devm_clk_get > > failed\n"); > > + > > + ret = clk_prepare_enable(clk); > > + if (ret) > > + return dev_err_probe(dev, ret, "failed to enable > > clock\n"); > > + > > + mpfs_gpio->clk = clk; > > + > > + ngpio = of_irq_count(node); > > + if (ngpio > NUM_GPIO) { > > + ret = -ENXIO; > > + goto cleanup_clock; > > + } > > + > > + irq_parent = of_irq_find_parent(node); > > + if (!irq_parent) { > > + ret = -ENODEV; > > + goto cleanup_clock; > > + } > > + parent = irq_find_host(irq_parent); > > + if (!parent) { > > + ret = -ENODEV; > > + goto cleanup_clock; > > + } > > + > > + /* Get the interrupt numbers. */ > > + /* Clear/Disable All interrupts before enabling parent > > interrupts. */ > > + for (i = 0; i < ngpio; i++) { > > + mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i); > > Bingo. You are allocating the interrupt for the level below. You > really shouldn't do that. > > If you need to retrieve the *hwirq* for the level below, you need to > parse the DT without triggering an IRQ allocation (of_irq_parse_one() > and co). > > M. > > -- > Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 15:21 ` Lewis.Hanly @ 2022-07-16 17:52 ` Marc Zyngier 2022-07-16 18:32 ` Conor.Dooley 0 siblings, 1 reply; 12+ messages in thread From: Marc Zyngier @ 2022-07-16 17:52 UTC (permalink / raw) To: Lewis.Hanly Cc: linux-riscv, Conor.Dooley, brgl, linux-gpio, linus.walleij, palmer, linux-kernel, Daire.McNamara On Sat, 16 Jul 2022 16:21:48 +0100, <Lewis.Hanly@microchip.com> wrote: > > Thanks Marc, > > On Sat, 2022-07-16 at 11:33 +0100, Marc Zyngier wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > On Sat, 16 Jul 2022 08:11:13 +0100, > > <lewis.hanly@microchip.com> wrote: > > > From: Lewis Hanly <lewis.hanly@microchip.com> > > > > > > Add a driver to support the Polarfire SoC gpio controller. > > > > > > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> > > > > [...] > > > > > +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc, > > > + unsigned int child, > > > + unsigned int child_type, > > > + unsigned int *parent, > > > + unsigned int *parent_type) > > > +{ > > > + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); > > > + struct irq_data *d = irq_get_irq_data(mpfs_gpio- > > > >irq_number[child]); > > > > This looks totally wrong. It means that you have already instantiated > > part of the hierarchy, and it is likely that you will get multiple > > hierarchy sharing some levels, which isn't intended. > > Some background why I use the above. > We need to support both direct and non-direct IRQ connections to the > PLIC. > In direct mode the GPIO IRQ's are connected directly to the PLIC and > certainly no need for the above. GPIO's can also be configured in non- > direct, which means they use a shared IRQ, hence the above. That's unfortunately not acceptable. You need to distinguish which one is which, and separate them. Your non-direct mode certainly requires special handling, and is not fit for a hierarchical mode. M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 17:52 ` Marc Zyngier @ 2022-07-16 18:32 ` Conor.Dooley 2022-07-17 15:10 ` Marc Zyngier 0 siblings, 1 reply; 12+ messages in thread From: Conor.Dooley @ 2022-07-16 18:32 UTC (permalink / raw) To: maz, Lewis.Hanly Cc: linux-riscv, Conor.Dooley, brgl, linux-gpio, linus.walleij, palmer, linux-kernel, Daire.McNamara On 16/07/2022 18:52, Marc Zyngier wrote: > On Sat, 16 Jul 2022 16:21:48 +0100, > <Lewis.Hanly@microchip.com> wrote: >> >> Thanks Marc, >> >> On Sat, 2022-07-16 at 11:33 +0100, Marc Zyngier wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>> know the content is safe >>> >>> On Sat, 16 Jul 2022 08:11:13 +0100, >>> <lewis.hanly@microchip.com> wrote: >>>> From: Lewis Hanly <lewis.hanly@microchip.com> >>>> >>>> Add a driver to support the Polarfire SoC gpio controller. >>>> >>>> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> >>> >>> [...] >>> >>>> +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc, >>>> + unsigned int child, >>>> + unsigned int child_type, >>>> + unsigned int *parent, >>>> + unsigned int *parent_type) >>>> +{ >>>> + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); >>>> + struct irq_data *d = irq_get_irq_data(mpfs_gpio- >>>>> irq_number[child]); >>> >>> This looks totally wrong. It means that you have already instantiated >>> part of the hierarchy, and it is likely that you will get multiple >>> hierarchy sharing some levels, which isn't intended. >> >> Some background why I use the above. >> We need to support both direct and non-direct IRQ connections to the >> PLIC. >> In direct mode the GPIO IRQ's are connected directly to the PLIC and >> certainly no need for the above. GPIO's can also be configured in non- >> direct, which means they use a shared IRQ, hence the above. > > That's unfortunately not acceptable. You need to distinguish which one > is which, and separate them. Your non-direct mode certainly requires > special handling, and is not fit for a hierarchical mode. Unfortunately, the configuration is not fixed on the silicon level. The SoC has 3 GPIOs (with 32 lines each). The interrupt configuration looks something like the below: GPIO# width IRQ# ================================== gpio0/2 14 [26:13] gpio1/2 24 [50:27] gpio0_non_direct 1 51 gpio1_non_direct 1 52 gpio2_non_direct 1 53 Depending on what the bootloader/firmware does, these can be configured differently (done prior to linux starting). By default, 14 GPIOs from GPIO0 are fed into their own interrupt lines & ditto for 24 from GPIO1. The remaining GPIO0 & GPIO1 lines go into the corresponding non-direct interrupt. If they bootloader/firmware configures something different, a "direct" interrupt line can be switched to a GPIO2 line instead. Something like the following (the interrupts are offset by 13 here, as the global interrupts feed into the PLIC at an offset): * global int GPIO_INTERRUPT_FAB_CR 0 1 0 GPIO0 bit 0 GPIO2 bit 0 1 GPIO0 bit 1 GPIO2 bit 1 . . 12 GPIO0 bit 12 GPIO2 bit 12 13 GPIO0 bit 13 GPIO2 bit 13 14 GPIO1 bit 0 GPIO2 bit 14 15 GPIO1 bit 1 GPIO2 bit 15 . . . 30 GPIO1 bit 16 GPIO2 bit 30 31 GPIO1 bit 17 GPIO2 bit 31 32 GPIO1 bit 18 33 GPIO1 bit 19 34 GPIO1 bit 20 35 GPIO1 bit 21 36 GPIO1 bit 22 37 GPIO1 bit 23 38 Or of all GPIO0 interrupts who do not have a direct connection enabled 39 Or of all GPIO1 interrupts who do not have a direct connection enabled 40 Or of all GPIO2 interrupts who do not have a direct connection enabled Since we can tell based on the interrupt number in the device tree whether a line is in direct mode - can you suggest what the most appropriate irq structure for the driver? Although for extending this driver to the "soft" IP core, it may be easier to just create a "microchip,gpio-direct-mode-mask" property or similar and use that to figure out what configuration a line is in. Thanks, Conor. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 18:32 ` Conor.Dooley @ 2022-07-17 15:10 ` Marc Zyngier 2022-07-17 15:46 ` Conor.Dooley 0 siblings, 1 reply; 12+ messages in thread From: Marc Zyngier @ 2022-07-17 15:10 UTC (permalink / raw) To: Conor.Dooley Cc: Lewis.Hanly, linux-riscv, brgl, linux-gpio, linus.walleij, palmer, linux-kernel, Daire.McNamara On Sat, 16 Jul 2022 19:32:20 +0100, <Conor.Dooley@microchip.com> wrote: > > On 16/07/2022 18:52, Marc Zyngier wrote: > > On Sat, 16 Jul 2022 16:21:48 +0100, > > <Lewis.Hanly@microchip.com> wrote: > >> > >> Thanks Marc, > >> > >> On Sat, 2022-07-16 at 11:33 +0100, Marc Zyngier wrote: > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you > >>> know the content is safe > >>> > >>> On Sat, 16 Jul 2022 08:11:13 +0100, > >>> <lewis.hanly@microchip.com> wrote: > >>>> From: Lewis Hanly <lewis.hanly@microchip.com> > >>>> > >>>> Add a driver to support the Polarfire SoC gpio controller. > >>>> > >>>> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> > >>> > >>> [...] > >>> > >>>> +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc, > >>>> + unsigned int child, > >>>> + unsigned int child_type, > >>>> + unsigned int *parent, > >>>> + unsigned int *parent_type) > >>>> +{ > >>>> + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); > >>>> + struct irq_data *d = irq_get_irq_data(mpfs_gpio- > >>>>> irq_number[child]); > >>> > >>> This looks totally wrong. It means that you have already instantiated > >>> part of the hierarchy, and it is likely that you will get multiple > >>> hierarchy sharing some levels, which isn't intended. > >> > >> Some background why I use the above. > >> We need to support both direct and non-direct IRQ connections to the > >> PLIC. > >> In direct mode the GPIO IRQ's are connected directly to the PLIC and > >> certainly no need for the above. GPIO's can also be configured in non- > >> direct, which means they use a shared IRQ, hence the above. > > > > That's unfortunately not acceptable. You need to distinguish which one > > is which, and separate them. Your non-direct mode certainly requires > > special handling, and is not fit for a hierarchical mode. > > Unfortunately, the configuration is not fixed on the silicon level. The > SoC has 3 GPIOs (with 32 lines each). The interrupt configuration looks Let's start with a bit of terminology so that we can understand each other: - GPIO: a single piece of wire - GPIO block: a set of wires with a common programming interface As I understand it, you have 3 GPIO blocks, each with 32 GPIOs, for a total of 96 external lines. Correct? > something like the below: > GPIO# width IRQ# > ================================== > gpio0/2 14 [26:13] > gpio1/2 24 [50:27] > gpio0_non_direct 1 51 > gpio1_non_direct 1 52 > gpio2_non_direct 1 53 > > Depending on what the bootloader/firmware does, these can be configured > differently (done prior to linux starting). By default, 14 GPIOs from > GPIO0 are fed into their own interrupt lines & ditto for 24 from GPIO1. > The remaining GPIO0 & GPIO1 lines go into the corresponding non-direct > interrupt. If they bootloader/firmware configures something different, > a "direct" interrupt line can be switched to a GPIO2 line instead. What does non-direct mean? Multiplexing inputs into a single output? Can you individually mask/unmask the input lines that are in this mode (the kernel calls this a "chained irqchip")? How does this switch between direct and non-direct happen? Do you have some sort of external pad to GPIO line routing? It would really help if you could point people at an actual specification for these blocks rather than paraphrasing things. > > Something like the following (the interrupts are offset by 13 here, as > the global interrupts feed into the PLIC at an offset): > > * global int GPIO_INTERRUPT_FAB_CR > 0 1 > 0 GPIO0 bit 0 GPIO2 bit 0 > 1 GPIO0 bit 1 GPIO2 bit 1 > . > . > 12 GPIO0 bit 12 GPIO2 bit 12 > 13 GPIO0 bit 13 GPIO2 bit 13 > 14 GPIO1 bit 0 GPIO2 bit 14 > 15 GPIO1 bit 1 GPIO2 bit 15 > . > . > . > 30 GPIO1 bit 16 GPIO2 bit 30 > 31 GPIO1 bit 17 GPIO2 bit 31 > 32 GPIO1 bit 18 > 33 GPIO1 bit 19 > 34 GPIO1 bit 20 > 35 GPIO1 bit 21 > 36 GPIO1 bit 22 > 37 GPIO1 bit 23 > 38 Or of all GPIO0 interrupts who do not have a direct connection enabled > 39 Or of all GPIO1 interrupts who do not have a direct connection enabled > 40 Or of all GPIO2 interrupts who do not have a direct connection enabled > > Since we can tell based on the interrupt number in the device tree > whether a line is in direct mode - can you suggest what the most > appropriate irq structure for the driver? The topology must be described in DT one way or another, and I don't really want to rely on a fixed interrupt number that will change from one version to another. In any case: - direct interrupts should be handled as a hierarchy, mostly like the code currently does, but definitely without the probing hack. - muxed interrupts (non-direct?) should be handled via a chained irqchip, using a different irqdomain, as the topology is radically different. > Although for extending this driver to the "soft" IP core, it may be easier > to just create a "microchip,gpio-direct-mode-mask" property or similar and > use that to figure out what configuration a line is in. My guts feeling is that this will eventually end-up biting you, as people will want to change the direct/non-direct status of an interrupt at boot time, without depending on the FW to do that on their behalf. I'm not necessarily advocating for this as this is a lot more code and it could totally invalidate the existing binding, but this is worth keeping in mind. In any case, this driver needs some serious rewriting. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-17 15:10 ` Marc Zyngier @ 2022-07-17 15:46 ` Conor.Dooley 0 siblings, 0 replies; 12+ messages in thread From: Conor.Dooley @ 2022-07-17 15:46 UTC (permalink / raw) To: maz, Conor.Dooley Cc: Lewis.Hanly, linux-riscv, brgl, linux-gpio, linus.walleij, palmer, linux-kernel, Daire.McNamara On 17/07/2022 16:10, Marc Zyngier wrote: > On Sat, 16 Jul 2022 19:32:20 +0100, > <Conor.Dooley@microchip.com> wrote: >> >> On 16/07/2022 18:52, Marc Zyngier wrote: >>> On Sat, 16 Jul 2022 16:21:48 +0100, >>> <Lewis.Hanly@microchip.com> wrote: >>>> >>>> Thanks Marc, >>>> >>>> On Sat, 2022-07-16 at 11:33 +0100, Marc Zyngier wrote: >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>>> know the content is safe >>>>> >>>>> On Sat, 16 Jul 2022 08:11:13 +0100, >>>>> <lewis.hanly@microchip.com> wrote: >>>>>> From: Lewis Hanly <lewis.hanly@microchip.com> >>>>>> >>>>>> Add a driver to support the Polarfire SoC gpio controller. >>>>>> >>>>>> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> >>>>> >>>>> [...] >>>>> >>>>>> +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc, >>>>>> + unsigned int child, >>>>>> + unsigned int child_type, >>>>>> + unsigned int *parent, >>>>>> + unsigned int *parent_type) >>>>>> +{ >>>>>> + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); >>>>>> + struct irq_data *d = irq_get_irq_data(mpfs_gpio- >>>>>>> irq_number[child]); >>>>> >>>>> This looks totally wrong. It means that you have already instantiated >>>>> part of the hierarchy, and it is likely that you will get multiple >>>>> hierarchy sharing some levels, which isn't intended. >>>> >>>> Some background why I use the above. >>>> We need to support both direct and non-direct IRQ connections to the >>>> PLIC. >>>> In direct mode the GPIO IRQ's are connected directly to the PLIC and >>>> certainly no need for the above. GPIO's can also be configured in non- >>>> direct, which means they use a shared IRQ, hence the above. >>> >>> That's unfortunately not acceptable. You need to distinguish which one >>> is which, and separate them. Your non-direct mode certainly requires >>> special handling, and is not fit for a hierarchical mode. >> >> Unfortunately, the configuration is not fixed on the silicon level. The >> SoC has 3 GPIOs (with 32 lines each). The interrupt configuration looks > > Let's start with a bit of terminology so that we can understand each > other: > - GPIO: a single piece of wire > - GPIO block: a set of wires with a common programming interface > > As I understand it, you have 3 GPIO blocks, each with 32 GPIOs, for a > total of 96 external lines. Correct? Correct. > >> something like the below: >> GPIO# width IRQ# >> ================================== >> gpio0/2 14 [26:13] >> gpio1/2 24 [50:27] >> gpio0_non_direct 1 51 >> gpio1_non_direct 1 52 >> gpio2_non_direct 1 53 >> >> Depending on what the bootloader/firmware does, these can be configured >> differently (done prior to linux starting). By default, 14 GPIOs from >> GPIO0 are fed into their own interrupt lines & ditto for 24 from GPIO1. >> The remaining GPIO0 & GPIO1 lines go into the corresponding non-direct >> interrupt. If they bootloader/firmware configures something different, >> a "direct" interrupt line can be switched to a GPIO2 line instead. > > What does non-direct mean? Multiplexing inputs into a single output? non-direct being "not directly connected to the PLIC", so yes. The interrupts without a direct connection are muxed into a per GPIO block interrupt. > Can you individually mask/unmask the input lines that are in this mode > (the kernel calls this a "chained irqchip")? Each GPIO line has an interrupt enable bit in it's config register. > > How does this switch between direct and non-direct happen? Do you have > some sort of external pad to GPIO line routing? It would really help > if you could point people at an actual specification for these blocks > rather than paraphrasing things. GPIO is discussed on page 85 of the TRM: https://www.microsemi.com/document-portal/doc_download/1245725-polarfire-soc-fpga-mss-technical-reference-manual The register map is here: https://ww1.microchip.com/downloads/aemDocuments/documents/FPGA/ProductDocuments/SupportingCollateral/V1_4_Register_Map.zip PFSOC_MSS_TOP_SYSREG/GPIO_INTERRUPT_FAB_CR is the register that can switch a line between direct/non-direct mode. GPIO_*_LO are as you might expect are the GPIO block registers > >> >> Something like the following (the interrupts are offset by 13 here, as >> the global interrupts feed into the PLIC at an offset): >> >> * global int GPIO_INTERRUPT_FAB_CR >> 0 1 >> 0 GPIO0 bit 0 GPIO2 bit 0 >> 1 GPIO0 bit 1 GPIO2 bit 1 >> . >> . >> 12 GPIO0 bit 12 GPIO2 bit 12 >> 13 GPIO0 bit 13 GPIO2 bit 13 >> 14 GPIO1 bit 0 GPIO2 bit 14 >> 15 GPIO1 bit 1 GPIO2 bit 15 >> . >> . >> . >> 30 GPIO1 bit 16 GPIO2 bit 30 >> 31 GPIO1 bit 17 GPIO2 bit 31 >> 32 GPIO1 bit 18 >> 33 GPIO1 bit 19 >> 34 GPIO1 bit 20 >> 35 GPIO1 bit 21 >> 36 GPIO1 bit 22 >> 37 GPIO1 bit 23 >> 38 Or of all GPIO0 interrupts who do not have a direct connection enabled >> 39 Or of all GPIO1 interrupts who do not have a direct connection enabled >> 40 Or of all GPIO2 interrupts who do not have a direct connection enabled >> >> Since we can tell based on the interrupt number in the device tree >> whether a line is in direct mode - can you suggest what the most >> appropriate irq structure for the driver? > > The topology must be described in DT one way or another, and I don't > really want to rely on a fixed interrupt number that will change from > one version to another. Yeah, that was my gut feeling too. > > In any case: > > - direct interrupts should be handled as a hierarchy, mostly like the > code currently does, but definitely without the probing hack. > > - muxed interrupts (non-direct?) should be handled via a chained > irqchip, using a different irqdomain, as the topology is radically > different. Thanks. > >> Although for extending this driver to the "soft" IP core, it may be easier >> to just create a "microchip,gpio-direct-mode-mask" property or similar and >> use that to figure out what configuration a line is in. > > My guts feeling is that this will eventually end-up biting you, as > people will want to change the direct/non-direct status of an > interrupt at boot time, without depending on the FW to do that on > their behalf. I am inclined to not allow that in all honesty. Because this is an FPGA, there's configuration based on the contents of the FPGA fabric that has to be done by something before an OS can be brought up & we have to draw a line somewhere for what's reasonable for Linux to do. > > In any case, this driver needs some serious rewriting. Thanks for your help Marc. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 10:33 ` Marc Zyngier 2022-07-16 15:21 ` Lewis.Hanly @ 2022-07-31 8:56 ` Lewis.Hanly 1 sibling, 0 replies; 12+ messages in thread From: Lewis.Hanly @ 2022-07-31 8:56 UTC (permalink / raw) To: maz Cc: linux-riscv, Conor.Dooley, brgl, linux-gpio, linus.walleij, palmer, linux-kernel, Daire.McNamara On Sat, 2022-07-16 at 11:33 +0100, Marc Zyngier wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Sat, 16 Jul 2022 08:11:13 +0100, > <lewis.hanly@microchip.com> wrote: > > From: Lewis Hanly <lewis.hanly@microchip.com> > > > > Add a driver to support the Polarfire SoC gpio controller. > > > > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> > > [...] > > > +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc, > > + unsigned int child, > > + unsigned int child_type, > > + unsigned int *parent, > > + unsigned int *parent_type) > > +{ > > + struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc); > > + struct irq_data *d = irq_get_irq_data(mpfs_gpio- > > >irq_number[child]); > > This looks totally wrong. It means that you have already instantiated > part of the hierarchy, and it is likely that you will get multiple > hierarchy sharing some levels, which isn't intended. Thanks Marc, as you have pointed out the hierarchical interrupt flow is not fitting our hw. I am in the process of reworking the driver to implement chained interrupt flow as you pointed out before. > > > + *parent_type = IRQ_TYPE_NONE; > > + *parent = irqd_to_hwirq(d); > > + > > + return 0; > > +} > > + > > +static int mpfs_gpio_probe(struct platform_device *pdev) > > +{ > > + struct clk *clk; > > + struct device *dev = &pdev->dev; > > + struct device_node *node = pdev->dev.of_node; > > + struct device_node *irq_parent; > > + struct gpio_irq_chip *girq; > > + struct irq_domain *parent; > > + struct mpfs_gpio_chip *mpfs_gpio; > > + int i, ret, ngpio; > > + > > + mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), > > GFP_KERNEL); > > + if (!mpfs_gpio) > > + return -ENOMEM; > > + > > + mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(mpfs_gpio->base)) > > + return dev_err_probe(dev, PTR_ERR(mpfs_gpio->clk), > > "input clock not found.\n"); > > + > > + clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(clk)) > > + return dev_err_probe(dev, PTR_ERR(clk), "devm_clk_get > > failed\n"); > > + > > + ret = clk_prepare_enable(clk); > > + if (ret) > > + return dev_err_probe(dev, ret, "failed to enable > > clock\n"); > > + > > + mpfs_gpio->clk = clk; > > + > > + ngpio = of_irq_count(node); > > + if (ngpio > NUM_GPIO) { > > + ret = -ENXIO; > > + goto cleanup_clock; > > + } > > + > > + irq_parent = of_irq_find_parent(node); > > + if (!irq_parent) { > > + ret = -ENODEV; > > + goto cleanup_clock; > > + } > > + parent = irq_find_host(irq_parent); > > + if (!parent) { > > + ret = -ENODEV; > > + goto cleanup_clock; > > + } > > + > > + /* Get the interrupt numbers. */ > > + /* Clear/Disable All interrupts before enabling parent > > interrupts. */ > > + for (i = 0; i < ngpio; i++) { > > + mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i); > > Bingo. You are allocating the interrupt for the level below. You > really shouldn't do that. > > If you need to retrieve the *hwirq* for the level below, you need to > parse the DT without triggering an IRQ allocation (of_irq_parse_one() > and co). > > M. > > -- > Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 7:11 ` [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support lewis.hanly 2022-07-16 10:33 ` Marc Zyngier @ 2022-07-16 10:44 ` Marc Zyngier 2022-07-16 12:17 ` Lewis.Hanly 2022-07-16 12:20 ` Conor.Dooley 1 sibling, 2 replies; 12+ messages in thread From: Marc Zyngier @ 2022-07-16 10:44 UTC (permalink / raw) To: lewis.hanly Cc: linux-gpio, linux-riscv, linus.walleij, brgl, linux-kernel, palmer, conor.dooley, daire.mcnamara On Sat, 16 Jul 2022 08:11:13 +0100, <lewis.hanly@microchip.com> wrote: > > From: Lewis Hanly <lewis.hanly@microchip.com> > > Add a driver to support the Polarfire SoC gpio controller. > > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> > --- > drivers/gpio/Kconfig | 9 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-mpfs.c | 361 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 371 insertions(+) > create mode 100644 drivers/gpio/gpio-mpfs.c A couple of other nits: > +static const struct of_device_id mpfs_of_ids[] = { > + { .compatible = "microchip,mpfs-gpio", }, Where is the DT binding for this? > + { /* end of list */ } > +}; > + > +static struct platform_driver mpfs_gpio_driver = { > + .probe = mpfs_gpio_probe, > + .driver = { > + .name = "microchip,mpfs-gpio", > + .of_match_table = mpfs_of_ids, > + }, > + .remove = mpfs_gpio_remove, No, please. You cannot enforce that there are no interrupts being used (and nothing checks for this), and you're pretty much guaranteed that the system will catch fire on the first interrupt being delivered. Moreover, your "remove" callback only turns the clock off, which is yet another nail on that coffin. M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 10:44 ` Marc Zyngier @ 2022-07-16 12:17 ` Lewis.Hanly 2022-07-16 12:20 ` Conor.Dooley 1 sibling, 0 replies; 12+ messages in thread From: Lewis.Hanly @ 2022-07-16 12:17 UTC (permalink / raw) To: maz Cc: linux-gpio, linux-riscv, linus.walleij, brgl, linux-kernel, palmer, Conor.Dooley, Daire.McNamara On 16/07/2022 11:44, Marc Zyngier wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Sat, 16 Jul 2022 08:11:13 +0100, > <lewis.hanly@microchip.com> wrote: >> >> From: Lewis Hanly <lewis.hanly@microchip.com> >> >> Add a driver to support the Polarfire SoC gpio controller. >> >> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com> >> --- >> drivers/gpio/Kconfig | 9 + >> drivers/gpio/Makefile | 1 + >> drivers/gpio/gpio-mpfs.c | 361 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 371 insertions(+) >> create mode 100644 drivers/gpio/gpio-mpfs.c > > A couple of other nits: > >> +static const struct of_device_id mpfs_of_ids[] = { >> + { .compatible = "microchip,mpfs-gpio", }, > > Where is the DT binding for this? Already upstream, Documentation/devicetree/bindings/gpio/microchip%2Cmpfs-gpio.yaml > >> + { /* end of list */ } >> +}; >> + >> +static struct platform_driver mpfs_gpio_driver = { >> + .probe = mpfs_gpio_probe, >> + .driver = { >> + .name = "microchip,mpfs-gpio", >> + .of_match_table = mpfs_of_ids, >> + }, >> + .remove = mpfs_gpio_remove, > > No, please. You cannot enforce that there are no interrupts being used > (and nothing checks for this), and you're pretty much guaranteed that > the system will catch fire on the first interrupt being delivered. > Moreover, your "remove" callback only turns the clock off, which is > yet another nail on that coffin. Will remove. > > M. > > -- > Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support 2022-07-16 10:44 ` Marc Zyngier 2022-07-16 12:17 ` Lewis.Hanly @ 2022-07-16 12:20 ` Conor.Dooley 1 sibling, 0 replies; 12+ messages in thread From: Conor.Dooley @ 2022-07-16 12:20 UTC (permalink / raw) To: maz, Lewis.Hanly Cc: linux-gpio, linux-riscv, linus.walleij, brgl, linux-kernel, palmer, Daire.McNamara On 16/07/2022 11:44, Marc Zyngier wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > A couple of other nits: > >> +static const struct of_device_id mpfs_of_ids[] = { >> + { .compatible = "microchip,mpfs-gpio", }, > > Where is the DT binding for this? > Upstreamed with the device tree entry in 5.18: Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml Lewis: might be worth mentioning that in your cover letters. Thanks, Conor. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-07-31 8:57 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-16 7:11 [PATCH v3 0/1] Add Polarfire SoC GPIO support lewis.hanly 2022-07-16 7:11 ` [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support lewis.hanly 2022-07-16 10:33 ` Marc Zyngier 2022-07-16 15:21 ` Lewis.Hanly 2022-07-16 17:52 ` Marc Zyngier 2022-07-16 18:32 ` Conor.Dooley 2022-07-17 15:10 ` Marc Zyngier 2022-07-17 15:46 ` Conor.Dooley 2022-07-31 8:56 ` Lewis.Hanly 2022-07-16 10:44 ` Marc Zyngier 2022-07-16 12:17 ` Lewis.Hanly 2022-07-16 12:20 ` Conor.Dooley
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).