[PATCH v9 0/1] Polarfire SoC GPIO support

[PATCH v9 0/1] Polarfire SoC GPIO support

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Hey all,

Lewis is no longer at Microchip, so I've taken over the GPIO controller
patchset that he had been working on prior to that:
https://lore.kernel.org/linux-gpio/20220815120834.1562544-1-lewis.hanly@microchip.com/

One thing that was wrong with Lewis' series was that it could only,
depending on the iteration of the series, support GPIOs that had their
interrupts muxed or GPIOs that had dedicated interrupts at the
parent interrupt controller. I found that to be problematic, because the
hardware itself always has a mix of muxed and dedicated interrupts and
so there was always a controller rendered unusable for interrupts.
I attempted to fix this by remodelling how the interrupt hierarchy in
the devicetree is described, with a mux added between the GPIO
controllers and the platform's interrupt controller. v7 introduced an
irqchip driver for the mux between the GPIO controllers and PLIC to handle
that problem.

After some discussion with Linus on v7, I've opted to strip out the
interrupt handling entirely, in order to upstream this piecemeal.
Interrupt controller support will be added at a later date, when I've
sorted out the bits that Thomas did not approve of.

The binding for this GPIO controller is already upstream, so there's
just one patch here now.

Cheers,
Conor.

v7: https://lore.kernel.org/linux-gpio/20240723-supervise-drown-d5d3b303e7fd@wendy/

CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: valentina.fernandezalanis@microchip.com
CC: Linus Walleij <linus.walleij@linaro.org>
CC: Bartosz Golaszewski <brgl@bgdev.pl>
CC: <linux-gpio@vger.kernel.org>

Lewis Hanly (1):
  gpio: mpfs: add polarfire soc gpio support

 drivers/gpio/Kconfig     |   6 ++
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-mpfs.c | 170 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 drivers/gpio/gpio-mpfs.c

-- 
2.45.2

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

[Conor Dooley]

From: Lewis Hanly <lewis.hanly@microchip.com>

Add a driver to support the Polarfire SoC gpio controller. Interrupt
controller support is unavailable for now and will be added at a later
date.

Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
v9:
- remove remove()
- drop OF_GPIO from kconfig
- add a prefix to MAX_NUM_GPIO

v8:
- drop interrupt support
- replace regular mmio acesses with regmap (nice complexity reduction)
- change mpfs_gpio_get() so that it can report non-zero when the line
  direction is output.
---
 drivers/gpio/Kconfig     |   6 ++
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-mpfs.c | 170 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 drivers/gpio/gpio-mpfs.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d93cd4f722b40..811263b033c89 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -549,6 +549,12 @@ config GPIO_PL061
 	help
 	  Say yes here to support the PrimeCell PL061 GPIO device.
 
+config GPIO_POLARFIRE_SOC
+	bool "Microchip FPGA GPIO support"
+	select REGMAP_MMIO
+	help
+	  Say yes here to support the GPIO controllers 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 1429e8c0229b9..fc66e6388c76c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -133,6 +133,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 0000000000000..e5017307b0fe6
--- /dev/null
+++ b/drivers/gpio/gpio-mpfs.c
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: (GPL-2.0)
+/*
+ * Microchip PolarFire SoC (MPFS) GPIO controller driver
+ *
+ * Copyright (c) 2018-2024 Microchip Technology Inc. and its subsidiaries
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+
+#define MPFS_GPIO_CTRL(i)		(0x4 * (i))
+#define MPFS_MAX_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_DIR_MASK		GENMASK(2, 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 {
+	struct clk *clk;
+	struct regmap *regs;
+	struct gpio_chip gc;
+};
+
+static const struct regmap_config mpfs_gpio_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+};
+
+static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+
+	regmap_update_bits(mpfs_gpio->regs, MPFS_GPIO_CTRL(gpio_index),
+			   MPFS_GPIO_DIR_MASK, MPFS_GPIO_EN_IN);
+
+	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);
+
+	regmap_update_bits(mpfs_gpio->regs, MPFS_GPIO_CTRL(gpio_index),
+			   MPFS_GPIO_DIR_MASK, MPFS_GPIO_EN_IN);
+	regmap_update_bits(mpfs_gpio->regs, MPFS_OUTP_REG, BIT(gpio_index),
+			   value << gpio_index);
+
+	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);
+	unsigned int gpio_cfg;
+
+	regmap_read(mpfs_gpio->regs, MPFS_GPIO_CTRL(gpio_index), &gpio_cfg);
+	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);
+
+	if (mpfs_gpio_get_direction(gc, gpio_index) == GPIO_LINE_DIRECTION_OUT)
+		return regmap_test_bits(mpfs_gpio->regs, MPFS_OUTP_REG, BIT(gpio_index));
+	else
+		return regmap_test_bits(mpfs_gpio->regs, 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);
+
+	mpfs_gpio_get(gc, gpio_index);
+
+	regmap_update_bits(mpfs_gpio->regs, MPFS_OUTP_REG, BIT(gpio_index),
+			   value << gpio_index);
+
+	mpfs_gpio_get(gc, gpio_index);
+}
+
+static int mpfs_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mpfs_gpio_chip *mpfs_gpio;
+	struct clk *clk;
+	void __iomem *base;
+	int ret, ngpios;
+
+	mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL);
+	if (!mpfs_gpio)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return dev_err_probe(dev, PTR_ERR(base), "failed to ioremap memory resource\n");
+
+	mpfs_gpio->regs = devm_regmap_init_mmio(dev, base, &mpfs_gpio_regmap_config);
+	if (IS_ERR(mpfs_gpio->regs))
+		return dev_err_probe(dev, PTR_ERR(mpfs_gpio->regs),
+				     "failed to initialise regmap\n");
+
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "failed to get and enable clock\n");
+
+	mpfs_gpio->clk = clk;
+
+	ngpios = MPFS_MAX_NUM_GPIO;
+	device_property_read_u32(dev, "ngpios", &ngpios);
+	if (ngpios > MPFS_MAX_NUM_GPIO)
+		ngpios = MPFS_MAX_NUM_GPIO;
+
+	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 = ngpios;
+	mpfs_gpio->gc.label = dev_name(dev);
+	mpfs_gpio->gc.parent = dev;
+	mpfs_gpio->gc.owner = THIS_MODULE;
+
+	ret = devm_gpiochip_add_data(dev, &mpfs_gpio->gc, mpfs_gpio);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, mpfs_gpio);
+
+	return 0;
+}
+
+static const struct of_device_id mpfs_gpio_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_gpio_of_ids,
+	},
+};
+builtin_platform_driver(mpfs_gpio_driver);
-- 
2.45.2

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

[Bartosz Golaszewski]

On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@kernel.org> wrote:
>
> From: Lewis Hanly <lewis.hanly@microchip.com>
>
> Add a driver to support the Polarfire SoC gpio controller. Interrupt
> controller support is unavailable for now and will be added at a later
> date.
>
> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> v9:
> - remove remove()
> - drop OF_GPIO from kconfig
> - add a prefix to MAX_NUM_GPIO
>
> v8:
> - drop interrupt support
> - replace regular mmio acesses with regmap (nice complexity reduction)
> - change mpfs_gpio_get() so that it can report non-zero when the line
>   direction is output.
> ---
>  drivers/gpio/Kconfig     |   6 ++
>  drivers/gpio/Makefile    |   1 +
>  drivers/gpio/gpio-mpfs.c | 170 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 177 insertions(+)
>  create mode 100644 drivers/gpio/gpio-mpfs.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index d93cd4f722b40..811263b033c89 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -549,6 +549,12 @@ config GPIO_PL061
>         help
>           Say yes here to support the PrimeCell PL061 GPIO device.
>
> +config GPIO_POLARFIRE_SOC
> +       bool "Microchip FPGA GPIO support"
> +       select REGMAP_MMIO
> +       help
> +         Say yes here to support the GPIO controllers 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 1429e8c0229b9..fc66e6388c76c 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -133,6 +133,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 0000000000000..e5017307b0fe6
> --- /dev/null
> +++ b/drivers/gpio/gpio-mpfs.c
> @@ -0,0 +1,170 @@
> +// SPDX-License-Identifier: (GPL-2.0)
> +/*
> + * Microchip PolarFire SoC (MPFS) GPIO controller driver
> + *
> + * Copyright (c) 2018-2024 Microchip Technology Inc. and its subsidiaries
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/init.h>
> +#include <linux/of.h>

Sorry for noticing it only now but you don't use any symbol from this
header so it can be dropped.

> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spinlock.h>
> +
> +#define MPFS_GPIO_CTRL(i)              (0x4 * (i))
> +#define MPFS_MAX_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_DIR_MASK             GENMASK(2, 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 {
> +       struct clk *clk;

After the initial clk_get() you never reference it again so there's no
need to store the address.

> +       struct regmap *regs;
> +       struct gpio_chip gc;
> +};
> +
> +static const struct regmap_config mpfs_gpio_regmap_config = {
> +       .reg_bits = 32,
> +       .reg_stride = 4,
> +       .val_bits = 32,
> +};
> +
> +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
> +{
> +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> +
> +       regmap_update_bits(mpfs_gpio->regs, MPFS_GPIO_CTRL(gpio_index),
> +                          MPFS_GPIO_DIR_MASK, MPFS_GPIO_EN_IN);
> +
> +       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);
> +
> +       regmap_update_bits(mpfs_gpio->regs, MPFS_GPIO_CTRL(gpio_index),
> +                          MPFS_GPIO_DIR_MASK, MPFS_GPIO_EN_IN);
> +       regmap_update_bits(mpfs_gpio->regs, MPFS_OUTP_REG, BIT(gpio_index),
> +                          value << gpio_index);
> +
> +       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);
> +       unsigned int gpio_cfg;
> +
> +       regmap_read(mpfs_gpio->regs, MPFS_GPIO_CTRL(gpio_index), &gpio_cfg);
> +       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);
> +
> +       if (mpfs_gpio_get_direction(gc, gpio_index) == GPIO_LINE_DIRECTION_OUT)
> +               return regmap_test_bits(mpfs_gpio->regs, MPFS_OUTP_REG, BIT(gpio_index));
> +       else
> +               return regmap_test_bits(mpfs_gpio->regs, 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);
> +
> +       mpfs_gpio_get(gc, gpio_index);
> +
> +       regmap_update_bits(mpfs_gpio->regs, MPFS_OUTP_REG, BIT(gpio_index),
> +                          value << gpio_index);
> +
> +       mpfs_gpio_get(gc, gpio_index);
> +}
> +
> +static int mpfs_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct mpfs_gpio_chip *mpfs_gpio;
> +       struct clk *clk;
> +       void __iomem *base;
> +       int ret, ngpios;
> +
> +       mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL);
> +       if (!mpfs_gpio)
> +               return -ENOMEM;
> +
> +       base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(base))
> +               return dev_err_probe(dev, PTR_ERR(base), "failed to ioremap memory resource\n");
> +
> +       mpfs_gpio->regs = devm_regmap_init_mmio(dev, base, &mpfs_gpio_regmap_config);
> +       if (IS_ERR(mpfs_gpio->regs))
> +               return dev_err_probe(dev, PTR_ERR(mpfs_gpio->regs),
> +                                    "failed to initialise regmap\n");
> +
> +       clk = devm_clk_get_enabled(dev, NULL);
> +       if (IS_ERR(clk))
> +               return dev_err_probe(dev, PTR_ERR(clk), "failed to get and enable clock\n");
> +
> +       mpfs_gpio->clk = clk;
> +
> +       ngpios = MPFS_MAX_NUM_GPIO;
> +       device_property_read_u32(dev, "ngpios", &ngpios);
> +       if (ngpios > MPFS_MAX_NUM_GPIO)
> +               ngpios = MPFS_MAX_NUM_GPIO;
> +
> +       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 = ngpios;

The "ngpios" property will be parsed by GPIO core so no need to set it.

> +       mpfs_gpio->gc.label = dev_name(dev);
> +       mpfs_gpio->gc.parent = dev;
> +       mpfs_gpio->gc.owner = THIS_MODULE;
> +
> +       ret = devm_gpiochip_add_data(dev, &mpfs_gpio->gc, mpfs_gpio);
> +       if (ret)
> +               return ret;
> +
> +       platform_set_drvdata(pdev, mpfs_gpio);

There's no platform_get_drvdata() in the driver so you can drop this too.

Bart

> +
> +       return 0;
> +}
> +
> +static const struct of_device_id mpfs_gpio_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_gpio_of_ids,
> +       },
> +};
> +builtin_platform_driver(mpfs_gpio_driver);
> --
> 2.45.2
>

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

On Thu, Oct 31, 2024 at 02:00:22PM +0100, Bartosz Golaszewski wrote:
> On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@kernel.org> wrote:

> > +       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 = ngpios;
> 
> The "ngpios" property will be parsed by GPIO core so no need to set it.

Are you sure it'll work here? I tried dropping the ngpios parsing, but I
get:
gpiochip_add_data_with_key: GPIOs 0..-1 (20122000.gpio) failed to register, -22

That's coming from the device_property_read_u32(dev, "ngpios", &ngpios);
in gpiochip_get_ngpios(). Checking against the bluefield driver and the
code in gpiochip_add_data_with_key(), it's not immediately obvious what
I am missing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Bartosz Golaszewski]

On Fri, Nov 1, 2024 at 1:17 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Oct 31, 2024 at 02:00:22PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@kernel.org> wrote:
>
> > > +       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 = ngpios;
> >
> > The "ngpios" property will be parsed by GPIO core so no need to set it.
>
> Are you sure it'll work here? I tried dropping the ngpios parsing, but I
> get:
> gpiochip_add_data_with_key: GPIOs 0..-1 (20122000.gpio) failed to register, -22
>
> That's coming from the device_property_read_u32(dev, "ngpios", &ngpios);
> in gpiochip_get_ngpios(). Checking against the bluefield driver and the
> code in gpiochip_add_data_with_key(), it's not immediately obvious what
> I am missing.

Does dev have an fwnode correctly assigned? What does dev_fwnode(dev) return?

Bart

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Fri, Nov 01, 2024 at 02:09:06PM +0100, Bartosz Golaszewski wrote:
> On Fri, Nov 1, 2024 at 1:17 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Oct 31, 2024 at 02:00:22PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > > > +       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 = ngpios;
> > >
> > > The "ngpios" property will be parsed by GPIO core so no need to set it.
> >
> > Are you sure it'll work here? I tried dropping the ngpios parsing, but I
> > get:
> > gpiochip_add_data_with_key: GPIOs 0..-1 (20122000.gpio) failed to register, -22
> >
> > That's coming from the device_property_read_u32(dev, "ngpios", &ngpios);
> > in gpiochip_get_ngpios(). Checking against the bluefield driver and the
> > code in gpiochip_add_data_with_key(), it's not immediately obvious what
> > I am missing.
> 
> Does dev have an fwnode correctly assigned? What does dev_fwnode(dev) return?

It's not a null pointer or something obviously wrong by virtue of being
null-adjacent, it is a virtual address but not one that %ps can identify.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Bartosz Golaszewski]

On Fri, Nov 1, 2024 at 2:37 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Nov 01, 2024 at 02:09:06PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Nov 1, 2024 at 1:17 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Thu, Oct 31, 2024 at 02:00:22PM +0100, Bartosz Golaszewski wrote:
> > > > On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > > > +       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 = ngpios;
> > > >
> > > > The "ngpios" property will be parsed by GPIO core so no need to set it.
> > >
> > > Are you sure it'll work here? I tried dropping the ngpios parsing, but I
> > > get:
> > > gpiochip_add_data_with_key: GPIOs 0..-1 (20122000.gpio) failed to register, -22
> > >
> > > That's coming from the device_property_read_u32(dev, "ngpios", &ngpios);
> > > in gpiochip_get_ngpios(). Checking against the bluefield driver and the
> > > code in gpiochip_add_data_with_key(), it's not immediately obvious what
> > > I am missing.
> >
> > Does dev have an fwnode correctly assigned? What does dev_fwnode(dev) return?
>
> It's not a null pointer or something obviously wrong by virtue of being
> null-adjacent, it is a virtual address but not one that %ps can identify.

This is an OF system right? If you do dev_of_node(dev), does the
returned node->name show the OF node you expect? Does it have the
"ngpios" property? Can you read it with of_property_read_u32()?

Bart

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

On Fri, Nov 01, 2024 at 02:47:51PM +0100, Bartosz Golaszewski wrote:
> On Fri, Nov 1, 2024 at 2:37 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Fri, Nov 01, 2024 at 02:09:06PM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Nov 1, 2024 at 1:17 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Thu, Oct 31, 2024 at 02:00:22PM +0100, Bartosz Golaszewski wrote:
> > > > > On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > > > +       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 = ngpios;
> > > > >
> > > > > The "ngpios" property will be parsed by GPIO core so no need to set it.
> > > >
> > > > Are you sure it'll work here? I tried dropping the ngpios parsing, but I
> > > > get:
> > > > gpiochip_add_data_with_key: GPIOs 0..-1 (20122000.gpio) failed to register, -22
> > > >
> > > > That's coming from the device_property_read_u32(dev, "ngpios", &ngpios);
> > > > in gpiochip_get_ngpios(). Checking against the bluefield driver and the
> > > > code in gpiochip_add_data_with_key(), it's not immediately obvious what
> > > > I am missing.
> > >
> > > Does dev have an fwnode correctly assigned? What does dev_fwnode(dev) return?
> >
> > It's not a null pointer or something obviously wrong by virtue of being
> > null-adjacent, it is a virtual address but not one that %ps can identify.
> 
> This is an OF system right? If you do dev_of_node(dev), does the
> returned node->name show the OF node you expect?

Yes.

> Does it have the
> "ngpios" property? Can you read it with of_property_read_u32()?

No, EINVAL there too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Bartosz Golaszewski]

On Fri, Nov 1, 2024 at 3:28 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Nov 01, 2024 at 02:47:51PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Nov 1, 2024 at 2:37 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Fri, Nov 01, 2024 at 02:09:06PM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Nov 1, 2024 at 1:17 PM Conor Dooley <conor@kernel.org> wrote:
> > > > >
> > > > > On Thu, Oct 31, 2024 at 02:00:22PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@kernel.org> wrote:
> > > > >
> > > > > > > +       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 = ngpios;
> > > > > >
> > > > > > The "ngpios" property will be parsed by GPIO core so no need to set it.
> > > > >
> > > > > Are you sure it'll work here? I tried dropping the ngpios parsing, but I
> > > > > get:
> > > > > gpiochip_add_data_with_key: GPIOs 0..-1 (20122000.gpio) failed to register, -22
> > > > >
> > > > > That's coming from the device_property_read_u32(dev, "ngpios", &ngpios);
> > > > > in gpiochip_get_ngpios(). Checking against the bluefield driver and the
> > > > > code in gpiochip_add_data_with_key(), it's not immediately obvious what
> > > > > I am missing.
> > > >
> > > > Does dev have an fwnode correctly assigned? What does dev_fwnode(dev) return?
> > >
> > > It's not a null pointer or something obviously wrong by virtue of being
> > > null-adjacent, it is a virtual address but not one that %ps can identify.
> >
> > This is an OF system right? If you do dev_of_node(dev), does the
> > returned node->name show the OF node you expect?
>
> Yes.

I mean inside gpiochip_get_ngpios(), sorry for confusion.

>
> > Does it have the
> > "ngpios" property? Can you read it with of_property_read_u32()?
>
> No, EINVAL there too.

I assume the node is not assigned correctly. What if in your probe you
do: gc->fwnode = dev_fwnode(dev)?

Bart

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

On Fri, Nov 01, 2024 at 03:32:16PM +0100, Bartosz Golaszewski wrote:
> On Fri, Nov 1, 2024 at 3:28 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Fri, Nov 01, 2024 at 02:47:51PM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Nov 1, 2024 at 2:37 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Fri, Nov 01, 2024 at 02:09:06PM +0100, Bartosz Golaszewski wrote:
> > > > > On Fri, Nov 1, 2024 at 1:17 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, Oct 31, 2024 at 02:00:22PM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > >
> > > > > > > > +       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 = ngpios;
> > > > > > >
> > > > > > > The "ngpios" property will be parsed by GPIO core so no need to set it.
> > > > > >
> > > > > > Are you sure it'll work here? I tried dropping the ngpios parsing, but I
> > > > > > get:
> > > > > > gpiochip_add_data_with_key: GPIOs 0..-1 (20122000.gpio) failed to register, -22
> > > > > >
> > > > > > That's coming from the device_property_read_u32(dev, "ngpios", &ngpios);
> > > > > > in gpiochip_get_ngpios(). Checking against the bluefield driver and the
> > > > > > code in gpiochip_add_data_with_key(), it's not immediately obvious what
> > > > > > I am missing.
> > > > >
> > > > > Does dev have an fwnode correctly assigned? What does dev_fwnode(dev) return?
> > > >
> > > > It's not a null pointer or something obviously wrong by virtue of being
> > > > null-adjacent, it is a virtual address but not one that %ps can identify.
> > >
> > > This is an OF system right? If you do dev_of_node(dev), does the
> > > returned node->name show the OF node you expect?
> >
> > Yes.
> 
> I mean inside gpiochip_get_ngpios(), sorry for confusion.

That is what I checked actually, didn't think you'd ask me to check the
one in probe that works.

> > > Does it have the
> > > "ngpios" property? Can you read it with of_property_read_u32()?
> >
> > No, EINVAL there too.
> 
> I assume the node is not assigned correctly. What if in your probe you
> do: gc->fwnode = dev_fwnode(dev)?

Makes no difference, same probe failure as before...


...but I just realised something: ngpios isn't a required property for
this device as it has a default of 32 and 32 is how many gpios this
controller has. Simple oversight, hours wasted. That's always the way of
it I suppose. The core code can't be used here I suppose, since ngpios
is optional.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Bartosz Golaszewski]

On Fri, Nov 1, 2024 at 4:16 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Nov 01, 2024 at 03:32:16PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Nov 1, 2024 at 3:28 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Fri, Nov 01, 2024 at 02:47:51PM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Nov 1, 2024 at 2:37 PM Conor Dooley <conor@kernel.org> wrote:
> > > > >
> > > > > On Fri, Nov 01, 2024 at 02:09:06PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Fri, Nov 1, 2024 at 1:17 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 31, 2024 at 02:00:22PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > >
> > > > > > > > > +       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 = ngpios;
> > > > > > > >
> > > > > > > > The "ngpios" property will be parsed by GPIO core so no need to set it.
> > > > > > >
> > > > > > > Are you sure it'll work here? I tried dropping the ngpios parsing, but I
> > > > > > > get:
> > > > > > > gpiochip_add_data_with_key: GPIOs 0..-1 (20122000.gpio) failed to register, -22
> > > > > > >
> > > > > > > That's coming from the device_property_read_u32(dev, "ngpios", &ngpios);
> > > > > > > in gpiochip_get_ngpios(). Checking against the bluefield driver and the
> > > > > > > code in gpiochip_add_data_with_key(), it's not immediately obvious what
> > > > > > > I am missing.
> > > > > >
> > > > > > Does dev have an fwnode correctly assigned? What does dev_fwnode(dev) return?
> > > > >
> > > > > It's not a null pointer or something obviously wrong by virtue of being
> > > > > null-adjacent, it is a virtual address but not one that %ps can identify.
> > > >
> > > > This is an OF system right? If you do dev_of_node(dev), does the
> > > > returned node->name show the OF node you expect?
> > >
> > > Yes.
> >
> > I mean inside gpiochip_get_ngpios(), sorry for confusion.
>
> That is what I checked actually, didn't think you'd ask me to check the
> one in probe that works.
>
> > > > Does it have the
> > > > "ngpios" property? Can you read it with of_property_read_u32()?
> > >
> > > No, EINVAL there too.
> >
> > I assume the node is not assigned correctly. What if in your probe you
> > do: gc->fwnode = dev_fwnode(dev)?
>
> Makes no difference, same probe failure as before...
>
>
> ...but I just realised something: ngpios isn't a required property for
> this device as it has a default of 32 and 32 is how many gpios this
> controller has. Simple oversight, hours wasted. That's always the way of
> it I suppose. The core code can't be used here I suppose, since ngpios
> is optional.

Ah right. If we get to gpiochip_get_ngpios() without a GPIO number
set, the property becomes mandatory.

Nevermind my comment from the review then.

Bart