diff mbox series

[v2] gpio: tqmx86: Add GPIO from for this IO controller

Message ID 1547250512-17430-1-git-send-email-andrew@lunn.ch
State New
Headers show
Series [v2] gpio: tqmx86: Add GPIO from for this IO controller | expand

Commit Message

Andrew Lunn Jan. 11, 2019, 11:48 p.m. UTC 
Some TQ-Systems ComExpress modules contain an IO controller with 8
GPIO lines.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2

Make use of GPIO_IRQCHIP
Replace magic numbers with #define
Support both edges for interrupts
Make use of BIT macro
Sort include files
Use linux/gpio/driver.h
Rename interrupt handler function
Use devm_gpiochip_add_data()
Change MODULE_AUTHOR to me and give credit for vendor author in header
---
 drivers/gpio/Kconfig       |   7 +
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-tqmx86.c | 339 +++++++++++++++++++++++++++++++++++++
 3 files changed, 347 insertions(+)
 create mode 100644 drivers/gpio/gpio-tqmx86.c

2.20.1

Comments

Linus Walleij Jan. 14, 2019, 10:09 a.m. UTC | #1 
Hi Andrew,

(Cc Julia C for the RT spinlock question.)

thanks for the updated v2 patch! It's almost perfect. But I ran into some
snags.

Overall it is pretty much an MMIO driver, just that it uses ioread()/iowrite()
and we really need to get around to fixing up the gpio-mmio.c to
support x86 style io with some flag. But it is a bit much to ask for
a simple driver (I might send or ask for patches later to convert it.)

On Sat, Jan 12, 2019 at 12:49 AM Andrew Lunn <andrew@lunn.ch> wrote:

> Some TQ-Systems ComExpress modules contain an IO controller with 8
> GPIO lines.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> v2

> diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> new file mode 100644
> index 000000000000..f626b716f30c
> --- /dev/null
> +++ b/drivers/gpio/gpio-tqmx86.c

Strangely this does not apply in my tree!

$ git am --signoff lunn1.patch
Applying: gpio: tqmx86: Add GPIO from for this IO controller
error: new file drivers/gpio/gpio-tqmx86.c depends on old contents
Patch failed at 0001 gpio: tqmx86: Add GPIO from for this IO controller

I don't know if the problem is on my side :/

I guess I can figure it out if it fails again.

> +struct tqmx86_gpio_data {
> +       struct gpio_chip        chip;
> +       struct irq_chip         irq_chip;
> +       void __iomem            *io_base;
> +       int                     irq;
> +       spinlock_t              spinlock;

I am not an expert in RT but I think this needs to be a
raw_spinlock_t (and use raw accessors) to work with realtime.

But maybe that just apply to chained IRQ handlers?

Julia do you have a definitive answer to this?

Yours,
Linus Walleij
Andrew Lunn Jan. 14, 2019, 1:39 p.m. UTC | #2 
On Mon, Jan 14, 2019 at 11:09:07AM +0100, Linus Walleij wrote:
> Hi Andrew,
> 
> (Cc Julia C for the RT spinlock question.)
> 
> thanks for the updated v2 patch! It's almost perfect. But I ran into some
> snags.
> 
> Overall it is pretty much an MMIO driver, just that it uses ioread()/iowrite()
> and we really need to get around to fixing up the gpio-mmio.c to
> support x86 style io with some flag. But it is a bit much to ask for
> a simple driver (I might send or ask for patches later to convert it.)

I consider it, but never made the jump to actually do it. gpio-sch.c
might benefit.  gpio-ts5500.c, gpio-gpio-mm.c, gpio-104-idio-16.c look
like they could be converted to central accessors.

gpio-kempld.c, gpio-sch311x.c gpio-it87.c, gpio-f7188x.c,
gpio-104-idio-16.c use inb() outb(), but are not straight memory
mapped. They have an extra level of indirection. But that indirection
looks similar for them all, so maybe some shared accessors could also
be added.

I would be happy to test patches for this driver, and i also have a
board using the gpio-kempld.d driver.

 
> On Sat, Jan 12, 2019 at 12:49 AM Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > Some TQ-Systems ComExpress modules contain an IO controller with 8
> > GPIO lines.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> > v2
> 
> > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> > new file mode 100644
> > index 000000000000..f626b716f30c
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-tqmx86.c
> 
> Strangely this does not apply in my tree!
> 
> $ git am --signoff lunn1.patch
> Applying: gpio: tqmx86: Add GPIO from for this IO controller
> error: new file drivers/gpio/gpio-tqmx86.c depends on old contents
> Patch failed at 0001 gpio: tqmx86: Add GPIO from for this IO controller

Ah, sorry about that. I have drivers for GPIO, MFD, I2C and platform
in the same branch. In order to make that work, i'm using plain -rc1,
not a maintainer specific tree. That could be the issue.

When i resend, i can cherry-pick it to your tree.

> > +struct tqmx86_gpio_data {
> > +       struct gpio_chip        chip;
> > +       struct irq_chip         irq_chip;
> > +       void __iomem            *io_base;
> > +       int                     irq;
> > +       spinlock_t              spinlock;
> 
> I am not an expert in RT but I think this needs to be a
> raw_spinlock_t (and use raw accessors) to work with realtime.
> 
> But maybe that just apply to chained IRQ handlers?

Not my area of expertise also.

I just read Documentation/driver-api/gpio/driver.rst.

That suggests i should use raw spinlocks. This could be used as a fast
chained IRQ, in that it is hanging off the main x86 interrupt
controller, and accessing the GPIO interrupt registers are also fast,
non-blocking operations.

However, my use case don't require this. I'm only using one pin as an
interrupt. That interrupt is itself just another link in an interrupt
chain, in that i have an Ethernet switch's interrupt pin connected to
the GPIO.  I need to read switch registers to read the switches
interrupt controller register. And that is done over a bitbanging MDIO
bus, implemented by pins on this GPIO controller. So that is all done
in a threaded interrupt handler, and all very slow.

However, this is a generic driver, and used in generic x86 hardware
often used in embedded systems. So other users might have RT use
cases.

Thanks
	Andrew
Julia Cartwright Jan. 14, 2019, 9:45 p.m. UTC | #3 
On Mon, Jan 14, 2019 at 02:39:52PM +0100, Andrew Lunn wrote:
> On Mon, Jan 14, 2019 at 11:09:07AM +0100, Linus Walleij wrote:
> > Hi Andrew,
> >
> > (Cc Julia C for the RT spinlock question.)
> >
[..]
> > > +struct tqmx86_gpio_data {
> > > +       struct gpio_chip        chip;
> > > +       struct irq_chip         irq_chip;
> > > +       void __iomem            *io_base;
> > > +       int                     irq;
> > > +       spinlock_t              spinlock;
> > 
> > I am not an expert in RT but I think this needs to be a
> > raw_spinlock_t (and use raw accessors) to work with realtime.
> > 
> > But maybe that just apply to chained IRQ handlers?
> 
> Not my area of expertise also.
> 
> I just read Documentation/driver-api/gpio/driver.rst.
>
> That suggests i should use raw spinlocks.

Because many of the struct irq_chip operations are invoked under struct
irq_desc::lock, which is raw, there will be issues on RT unless you also
make use of raw_spinlock_t here.

Converting to using raw_spinlock_t here doesn't look like it's going to
be a problem, seeing as this driver already does very little under the
lock.

> This could be used as a fast chained IRQ, in that it is hanging off
> the main x86 interrupt controller, and accessing the GPIO interrupt
> registers are also fast, non-blocking operations.

The implementation, as it stands, will also fall over w/ forced
irqthreading.

In particular, the tqmx86_gpio_irq_handler will be force threaded, and
executed with interrupts enabled, which will be problematic when
dispatching to downstream handlers..

This should work fine if you use the chained irq approach, even in the
force irqthreading case.

   Julia
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b5a2845347ec..4b31bb2940e9 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1190,6 +1190,13 @@  config GPIO_TPS68470
 	  of the TPS68470 must be available before dependent
 	  drivers are loaded.
 
+config GPIO_TQMX86
+	tristate "TQ-Systems QTMX86 GPIO"
+	depends on MFD_TQMX86
+	select GPIOLIB_IRQCHIP
+	help
+	  This driver supports GPIO on the TQMX86 IO controller.
+
 config GPIO_TWL4030
 	tristate "TWL4030, TWL5030, and TPS659x0 GPIOs"
 	depends on TWL4030_CORE
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 37628f8dbf70..89cfad015687 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -135,6 +135,7 @@  obj-$(CONFIG_GPIO_TPS6586X)	+= gpio-tps6586x.o
 obj-$(CONFIG_GPIO_TPS65910)	+= gpio-tps65910.o
 obj-$(CONFIG_GPIO_TPS65912)	+= gpio-tps65912.o
 obj-$(CONFIG_GPIO_TPS68470)	+= gpio-tps68470.o
+obj-$(CONFIG_GPIO_TQMX86)	+= gpio-tqmx86.o
 obj-$(CONFIG_GPIO_TS4800)	+= gpio-ts4800.o
 obj-$(CONFIG_GPIO_TS4900)	+= gpio-ts4900.o
 obj-$(CONFIG_GPIO_TS5500)	+= gpio-ts5500.o
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
new file mode 100644
index 000000000000..f626b716f30c
--- /dev/null
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -0,1 +1,339 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TQ-Systems TQMx86 PLD GPIO driver
+ *
+ * Based on vendor driver by:
+ *   Vadim V.Vlasov <vvlasov@dev.rtsoft.ru>
+ */
+
+#include <linux/bitops.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#define TQMX86_NGPIO	8
+#define TQMX86_NGPO	4	/* 0-3 - output */
+#define TQMX86_NGPI	4	/* 4-7 - input */
+#define TQMX86_DIR_INPUT_MASK	0xf0	/* 0-3 - output, 4-7 - input */
+
+#define TQMX86_GPIODD	0	/* GPIO Data Direction Register */
+#define TQMX86_GPIOD	1	/* GPIO Data Register */
+#define TQMX86_GPIIC	3	/* GPI Interrupt Configuration Register */
+#define TQMX86_GPIIS	4	/* GPI Interrupt Status Register */
+
+#define TQMX86_GPII_FALLING	BIT(0)
+#define TQMX86_GPII_RISING	BIT(1)
+#define TQMX86_GPII_MASK	(BIT(0) | BIT(1))
+#define TQMX86_GPII_BITS	2
+
+struct tqmx86_gpio_data {
+	struct gpio_chip	chip;
+	struct irq_chip		irq_chip;
+	void __iomem		*io_base;
+	int			irq;
+	spinlock_t		spinlock;
+	u8			irq_type[TQMX86_NGPI];
+};
+
+static u8 tqmx86_gpio_read(struct tqmx86_gpio_data *gd, unsigned int reg)
+{
+	return ioread8(gd->io_base + reg);
+}
+
+static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, u8 val,
+			      unsigned int reg)
+{
+	iowrite8(val, gd->io_base + reg);
+}
+
+static int tqmx86_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
+
+	return !!(tqmx86_gpio_read(gpio, TQMX86_GPIOD) & BIT(offset));
+}
+
+static void tqmx86_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			    int value)
+{
+	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
+	unsigned long flags;
+	u8 val;
+
+	spin_lock_irqsave(&gpio->spinlock, flags);
+	val = tqmx86_gpio_read(gpio, TQMX86_GPIOD);
+	if (value)
+		val |= BIT(offset);
+	else
+		val &= ~BIT(offset);
+	tqmx86_gpio_write(gpio, val, TQMX86_GPIOD);
+	spin_unlock_irqrestore(&gpio->spinlock, flags);
+}
+
+static int tqmx86_gpio_direction_input(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	/* Direction cannot be changed. Validate is an input. */
+	if (BIT(offset) & TQMX86_DIR_INPUT_MASK)
+		return 0;
+	else
+		return -EINVAL;
+}
+
+static int tqmx86_gpio_direction_output(struct gpio_chip *chip,
+					unsigned int offset,
+					int value)
+{
+	/* Direction cannot be changed, validate is an output */
+	if (BIT(offset) & TQMX86_DIR_INPUT_MASK)
+		return -EINVAL;
+	else
+		return 0;
+}
+
+static int tqmx86_gpio_get_direction(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	return !!(TQMX86_DIR_INPUT_MASK & BIT(offset));
+}
+
+static void tqmx86_gpio_irq_mask(struct irq_data *data)
+{
+	unsigned int offset = (data->hwirq - TQMX86_NGPO);
+	struct tqmx86_gpio_data *gpio = gpiochip_get_data(
+		irq_data_get_irq_chip_data(data));
+	unsigned long flags;
+	u8 gpiic, mask;
+
+	mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS);
+
+	spin_lock_irqsave(&gpio->spinlock, flags);
+	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
+	gpiic &= ~mask;
+	tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
+	spin_unlock_irqrestore(&gpio->spinlock, flags);
+}
+
+static void tqmx86_gpio_irq_unmask(struct irq_data *data)
+{
+	unsigned int offset = (data->hwirq - TQMX86_NGPO);
+	struct tqmx86_gpio_data *gpio = gpiochip_get_data(
+		irq_data_get_irq_chip_data(data));
+	unsigned long flags;
+	u8 gpiic, mask;
+
+	mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS);
+
+	spin_lock_irqsave(&gpio->spinlock, flags);
+	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
+	gpiic &= ~mask;
+	gpiic |= gpio->irq_type[offset] << (offset * TQMX86_GPII_BITS);
+	tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
+	spin_unlock_irqrestore(&gpio->spinlock, flags);
+}
+
+static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct tqmx86_gpio_data *gpio = gpiochip_get_data(
+		irq_data_get_irq_chip_data(data));
+	unsigned int offset = (data->hwirq - TQMX86_NGPO);
+	unsigned int edge_type = type & IRQF_TRIGGER_MASK;
+	unsigned long flags;
+	u8 new_type, gpiic;
+
+	switch (edge_type) {
+	case IRQ_TYPE_EDGE_RISING:
+		new_type = TQMX86_GPII_RISING;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		new_type = TQMX86_GPII_FALLING;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		new_type = TQMX86_GPII_FALLING | TQMX86_GPII_RISING;
+		break;
+	default:
+		return -EINVAL; /* not supported */
+	}
+
+	gpio->irq_type[offset] = new_type;
+
+	spin_lock_irqsave(&gpio->spinlock, flags);
+	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
+	gpiic &= ~((TQMX86_GPII_MASK) << (offset * TQMX86_GPII_BITS));
+	gpiic |= new_type << (offset * TQMX86_GPII_BITS);
+	tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
+	spin_unlock_irqrestore(&gpio->spinlock, flags);
+
+	return 0;
+}
+
+static irqreturn_t tqmx86_gpio_irq_handler(int irq, void *data)
+{
+	struct tqmx86_gpio_data *gpio = data;
+	unsigned long irq_bits;
+	int i = 0, child_irq;
+	u8 irq_status;
+
+	irq_status = tqmx86_gpio_read(gpio, TQMX86_GPIIS);
+
+	if (!irq_status)
+		return IRQ_HANDLED;
+
+	tqmx86_gpio_write(gpio, irq_status, TQMX86_GPIIS);
+
+	irq_bits = irq_status;
+	for_each_set_bit(i, &irq_bits, TQMX86_NGPI) {
+		child_irq = irq_find_mapping(gpio->chip.irq.domain,
+					     i + TQMX86_NGPO);
+		generic_handle_irq(child_irq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/* Minimal runtime PM is needed by the IRQ subsystem */
+static int __maybe_unused tqmx86_gpio_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int __maybe_unused tqmx86_gpio_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+static const struct dev_pm_ops tqmx86_gpio_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(tqmx86_gpio_runtime_suspend,
+			   tqmx86_gpio_runtime_resume, NULL)
+};
+
+static int tqmx86_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct tqmx86_gpio_data *gpio;
+	struct gpio_chip *chip;
+	void __iomem *io_base;
+	struct resource *res;
+	int ret, irq;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Cannot get I/O\n");
+		return -ENODEV;
+	}
+
+	io_base = devm_ioport_map(&pdev->dev, res->start, resource_size(res));
+	if (!io_base)
+		return -ENOMEM;
+
+	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	spin_lock_init(&gpio->spinlock);
+	gpio->io_base = io_base;
+
+	tqmx86_gpio_write(gpio, (u8)~TQMX86_DIR_INPUT_MASK, TQMX86_GPIODD);
+
+	platform_set_drvdata(pdev, gpio);
+
+	chip = &gpio->chip;
+	chip->label = "gpio-tqmx86";
+	chip->owner = THIS_MODULE;
+	chip->can_sleep = false;
+	chip->base = -1;
+	chip->direction_input = tqmx86_gpio_direction_input;
+	chip->direction_output = tqmx86_gpio_direction_output;
+	chip->get_direction = tqmx86_gpio_get_direction;
+	chip->get = tqmx86_gpio_get;
+	chip->set = tqmx86_gpio_set;
+	chip->ngpio = TQMX86_NGPIO;
+	chip->irq.need_valid_mask = true;
+	chip->parent = pdev->dev.parent;
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = devm_gpiochip_add_data(dev, chip, gpio);
+	if (ret) {
+		dev_err(dev, "Could not register GPIO chip\n");
+		goto out_pm_dis;
+	}
+
+	if (irq) {
+		struct irq_chip *irq_chip = &gpio->irq_chip;
+		u8 irq_status;
+
+		irq_chip->name = chip->label;
+		irq_chip->parent_device = &pdev->dev;
+		irq_chip->irq_mask = tqmx86_gpio_irq_mask;
+		irq_chip->irq_unmask = tqmx86_gpio_irq_unmask;
+		irq_chip->irq_set_type = tqmx86_gpio_irq_set_type;
+
+		/* Mask all interrupts */
+		tqmx86_gpio_write(gpio, 0, TQMX86_GPIIC);
+
+		/* Clear all pending interrupts */
+		irq_status = tqmx86_gpio_read(gpio, TQMX86_GPIIS);
+		tqmx86_gpio_write(gpio, irq_status, TQMX86_GPIIS);
+
+		ret = devm_request_irq(dev, irq, tqmx86_gpio_irq_handler,
+				       IRQF_TRIGGER_NONE,
+				       dev_name(dev), gpio);
+
+		if (ret) {
+			dev_err(dev, "Could not request irq.\n");
+			goto out_remove;
+		}
+		ret = gpiochip_irqchip_add(chip, irq_chip,
+					   0, handle_simple_irq,
+					   IRQ_TYPE_EDGE_BOTH);
+		if (ret) {
+			dev_err(dev, "Could not add irq chip\n");
+			goto out_remove;
+		}
+
+	}
+
+	/* Only GPIOs 4-7 are valid for interrupts. Clear the others */
+	clear_bit(0, chip->irq.valid_mask);
+	clear_bit(1, chip->irq.valid_mask);
+	clear_bit(2, chip->irq.valid_mask);
+	clear_bit(3, chip->irq.valid_mask);
+
+	dev_info(dev, "GPIO functionality initialized with %d pins\n",
+		 chip->ngpio);
+
+	return 0;
+
+out_remove:
+	gpiochip_remove(&gpio->chip);
+out_pm_dis:
+	pm_runtime_disable(&pdev->dev);
+
+	return ret;
+}
+
+static struct platform_driver tqmx86_gpio_driver = {
+	.driver = {
+		.name = "tqmx86-gpio",
+		.pm = &tqmx86_gpio_dev_pm_ops,
+	},
+	.probe		= tqmx86_gpio_probe,
+};
+
+module_platform_driver(tqmx86_gpio_driver);
+
+MODULE_DESCRIPTION("TQMx86 PLD GPIO Driver");
+MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:tqmx86-gpio");
--