| Message ID | 20241031-appease-purse-55145b5dfba4@spud |
|---|---|
| State | New |
| Headers | show |
| Series | Polarfire SoC GPIO support | expand |
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 >
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.
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
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.
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
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.
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
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.
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
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);