[PATCH v3 0/1] Add Polarfire SoC GPIO support

[PATCH v3 0/1] Add Polarfire SoC GPIO support

[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

[PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[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, &reg, 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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Marc Zyngier]

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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Marc Zyngier]

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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Lewis.Hanly]

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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Conor.Dooley]

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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Lewis.Hanly]

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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Marc Zyngier]

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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Conor.Dooley]

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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Marc Zyngier]

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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Conor.Dooley]

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

Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Lewis.Hanly]

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