All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips
@ 2020-12-24 11:22 Nikita Shubin
  2020-12-24 11:22 ` [RFC PATCH 1/3] " Nikita Shubin
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Nikita Shubin @ 2020-12-24 11:22 UTC (permalink / raw)
  Cc: Nikita Shubin, Linus Walleij, Bartosz Golaszewski, linux-gpio,
	linux-kernel

I was lucky enough to became an owner of some splendid piece's of
antiques called ts7250 based on the top of Cirrus Logic EP9302.

I don't know what fate expects this hardware (it's not EOL it's just Not
recommended for new designs) but i wanted to share fixes in ep93xx gpio area.

It seems ep93xx is deadly broken at current state usage of AB gpiochips
interrupts leads to deadlocks coused by irq_unmask/irq_mask recursions.

Port F is not working at all:

-bash-5.0# gpio-event-mon -n gpiochip5 -o 0 -r -f
------------[ cut here ]------------
kernel BUG at drivers/gpio/gpio-ep93xx.c:64!
Internal error: Oops - BUG: 0 [#1] ARM
Modules linked in:
CPU: 0 PID: 403 Comm: gpio-event-mon Not tainted 5.9.10-00011-ge93e9618628b-dirty #19
Hardware name: Technologic Systems TS-72xx SBC
PC is at ep93xx_gpio_update_int_params+0x1c/0x80
LR is at ep93xx_gpio_update_int_params+0x14/0x80
pc : [<c03abc44>]    lr : [<c03abc3c>]    psr: 20000093
sp : c158de00  ip : 00000000  fp : 00000001
r10: c44154d4  r9 : 00000000  r8 : c4415020
r7 : c04ef884  r6 : c051c842  r5 : c4415020  r4 : 00000005
r3 : 00000000  r2 : 00000000  r1 : c04eb768  r0 : 00000008
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 0000717f  Table: 01684000  DAC: 00000051
Process gpio-event-mon (pid: 403, stack limit = 0x(ptrval))
Stack: (0xc158de00 to 0xc158e000)
de00: 00000005 00000002 c051c842 c0238dc0 c0238c98 c0238c98 c04ef874 00000000
de20: 00000003 c04fcfcc 60000013 c04ef910 c04ef8d4 c00456f0 c04ef874 c15f1e00
de40: 00000000 00000000 00000001 c0045d40 c15f1e00 c4400160 c0044ca8 c04ef8a8
de60: 60000013 00000000 c15f1e00 c04ef874 c04ef884 00000001 c0235d70 c158b800
de80: be825f0f c0045ec8 00000003 c158b800 c440aa00 be825bc8 00000003 00000001
dea0: 00000000 c0236f00 c44ed3a0 c158b800 c45c2015 00000000 00000001 00000003
dec0: 6f697067 6576652d 6d2d746e 00006e6f 00000000 00000000 00000000 00000000
dee0: be825df4 c00abb0c c440c500 c00aabd4 c440c500 c528b840 c45c2010 c04e1228
df00: 00000ff0 c4478d28 c030b404 be825bc8 c1550e20 00000003 c1550e20 c00c3388
df20: c4478d28 c00c3d48 be825f0f c00abd00 c45c2000 c45c2000 c1550e20 c00bfea8
df40: 00000003 c00b0714 00000000 c4450000 00000004 00000100 00000001 c04e1228
df60: c158dfb0 ffffff9c 000231f8 00000003 00000142 c00b085c 00000000 c04e1228
df80: 00000000 be825f0f 00000003 00000003 00000036 c00083c4 c158c000 00000000
dfa0: be825f0f c00081e0 be825f0f 00000003 00000003 c030b404 be825bc8 00000000
dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f
dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec 60000010 00000003 00000000 00000000
[<c03abc44>] (ep93xx_gpio_update_int_params) from [<c0238dc0>] (ep93xx_gpio_irq_type+0x128/0x1c0)
[<c0238dc0>] (ep93xx_gpio_irq_type) from [<c00456f0>] (__irq_set_trigger+0x6c/0x128)
[<c00456f0>] (__irq_set_trigger) from [<c0045d40>] (__setup_irq+0x594/0x678)
[<c0045d40>] (__setup_irq) from [<c0045ec8>] (request_threaded_irq+0xa4/0x128)
[<c0045ec8>] (request_threaded_irq) from [<c0236f00>] (gpio_ioctl+0x300/0x4d8)
[<c0236f00>] (gpio_ioctl) from [<c00c3388>] (vfs_ioctl+0x24/0x3c)
[<c00c3388>] (vfs_ioctl) from [<c00c3d48>] (sys_ioctl+0xbc/0x768)
[<c00c3d48>] (sys_ioctl) from [<c00081e0>] (ret_fast_syscall+0x0/0x50)
Exception stack(0xc158dfa8 to 0xc158dff0)
dfa0:                   be825f0f 00000003 00000003 c030b404 be825bc8 00000000
dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f
dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec
Code: e59f0060 ebfff3e1 e3540002 9a000000 (e7f001f2)
---[ end trace 3f6544e133e9f5ae ]---

These change requires your judgment.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 1/3] gpio: ep93xx: convert to multi irqchips
  2020-12-24 11:22 [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Nikita Shubin
@ 2020-12-24 11:22 ` Nikita Shubin
  2020-12-26 17:52   ` Andy Shevchenko
  2020-12-24 11:22 ` [RFC PATCH 2/3] gpio: ep93xx: drop to_irq binding Nikita Shubin
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Nikita Shubin @ 2020-12-24 11:22 UTC (permalink / raw)
  Cc: Nikita Shubin, Linus Walleij, Bartosz Golaszewski, linux-gpio,
	linux-kernel

Since gpiolib requires having separate irqchips for each gpiochip, we
need to add some we definetly need a separate one for F port, and we
could combine gpiochip A and B into one - but this will break namespace
and logick.

So despite 3 irqchips is a bit beefy we need a separate irqchip for each
interrupt capable port.

- added separate irqchip for each iterrupt capable gpiochip
- dropped ep93xx_gpio_port (it wasn't working correct for port F anyway)
- moved irq registers into separate struct ep93xx_irq_chip, togather
  with regs current state
- reworked irq handle for ab gpiochips (through bit not tottaly sure this
  is a correct thing to do)
- dropped has_irq and has_hierarchical_irq and added a simple index
  which we rely on when adding irq's to gpiochip's

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 407 +++++++++++++++++++++----------------
 1 file changed, 227 insertions(+), 180 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 226da8df6f10..d6db0ff16581 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irq.h>
 #include <linux/slab.h>
@@ -34,103 +35,83 @@
  */
 #define EP93XX_GPIO_F_IRQ_BASE 80
 
+struct ep93xx_irq_chip {
+	void __iomem	*int_type1;
+	void __iomem	*int_type2;
+	void __iomem	*eoi;
+	void __iomem	*en;
+	void __iomem	*debounce;
+	void __iomem	*status;
+	u8		gpio_int_unmasked;
+	u8		gpio_int_enabled;
+	u8		gpio_int_type1;
+	u8		gpio_int_type2;
+	u8		gpio_int_debounce;
+	struct	irq_chip chip;
+};
+
+#define to_ep93xx_irq_chip(x) container_of(x, struct ep93xx_irq_chip, chip)
+
 struct ep93xx_gpio {
-	void __iomem		*base;
-	struct gpio_chip	gc[8];
+	void __iomem			*base;
+	struct ep93xx_irq_chip		ic[3];
+	struct gpio_chip		gc[8];
 };
 
 /*************************************************************************
  * Interrupt handling for EP93xx on-chip GPIOs
  *************************************************************************/
-static unsigned char gpio_int_unmasked[3];
-static unsigned char gpio_int_enabled[3];
-static unsigned char gpio_int_type1[3];
-static unsigned char gpio_int_type2[3];
-static unsigned char gpio_int_debounce[3];
-
 /* Port ordering is: A B F */
+static const char *irq_chip_names[3]		= {"gpio-irq-a", 
+						"gpio-irq-b", 
+						"gpio-irq-f"};
 static const u8 int_type1_register_offset[3]	= { 0x90, 0xac, 0x4c };
 static const u8 int_type2_register_offset[3]	= { 0x94, 0xb0, 0x50 };
 static const u8 eoi_register_offset[3]		= { 0x98, 0xb4, 0x54 };
 static const u8 int_en_register_offset[3]	= { 0x9c, 0xb8, 0x58 };
 static const u8 int_debounce_register_offset[3]	= { 0xa8, 0xc4, 0x64 };
+static const u8 int_status_register_offset[3]	= { 0xa0, 0xbc, 0x5c };
 
-static void ep93xx_gpio_update_int_params(struct ep93xx_gpio *epg, unsigned port)
-{
-	BUG_ON(port > 2);
-
-	writeb_relaxed(0, epg->base + int_en_register_offset[port]);
-
-	writeb_relaxed(gpio_int_type2[port],
-		       epg->base + int_type2_register_offset[port]);
-
-	writeb_relaxed(gpio_int_type1[port],
-		       epg->base + int_type1_register_offset[port]);
-
-	writeb(gpio_int_unmasked[port] & gpio_int_enabled[port],
-	       epg->base + int_en_register_offset[port]);
-}
-
-static int ep93xx_gpio_port(struct gpio_chip *gc)
+static void ep93xx_gpio_update_int_params(struct ep93xx_irq_chip *eic)
 {
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = 0;
-
-	while (port < ARRAY_SIZE(epg->gc) && gc != &epg->gc[port])
-		port++;
-
-	/* This should not happen but is there as a last safeguard */
-	if (port == ARRAY_SIZE(epg->gc)) {
-		pr_crit("can't find the GPIO port\n");
-		return 0;
-	}
-
-	return port;
+	writeb_relaxed(0, eic->en); // disable port irqs
+	writeb_relaxed(eic->gpio_int_type2, eic->int_type2);
+	writeb_relaxed(eic->gpio_int_type1, eic->int_type1);
+	writeb(eic->gpio_int_unmasked, eic->en); // enable port irqs
 }
 
-static void ep93xx_gpio_int_debounce(struct gpio_chip *gc,
+static void ep93xx_gpio_int_debounce(struct ep93xx_irq_chip *eic,
 				     unsigned int offset, bool enable)
 {
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
 	int port_mask = BIT(offset);
 
 	if (enable)
-		gpio_int_debounce[port] |= port_mask;
+		eic->gpio_int_debounce |= port_mask;
 	else
-		gpio_int_debounce[port] &= ~port_mask;
+		eic->gpio_int_debounce &= ~port_mask;
 
-	writeb(gpio_int_debounce[port],
-	       epg->base + int_debounce_register_offset[port]);
+	writeb(eic->gpio_int_debounce, eic->debounce);
 }
 
-static void ep93xx_gpio_ab_irq_handler(struct irq_desc *desc)
+static u32 ep93xx_gpio_ab_irq_handler(struct gpio_chip *gc)
 {
-	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+	struct irq_chip *ic = gc->irq.chip;
+	struct ep93xx_irq_chip *eic = to_ep93xx_irq_chip(ic);
 	unsigned long stat;
 	int offset;
 
-	chained_irq_enter(irqchip, desc);
+	stat = readb(eic->status);
 
-	/*
-	 * Dispatch the IRQs to the irqdomain of each A and B
-	 * gpiochip irqdomains depending on what has fired.
-	 * The tricky part is that the IRQ line is shared
-	 * between bank A and B and each has their own gpiochip.
-	 */
-	stat = readb(epg->base + EP93XX_GPIO_A_INT_STATUS);
 	for_each_set_bit(offset, &stat, 8)
-		generic_handle_irq(irq_find_mapping(epg->gc[0].irq.domain,
+		generic_handle_irq(irq_find_mapping(gc->irq.domain,
 						    offset));
 
-	stat = readb(epg->base + EP93XX_GPIO_B_INT_STATUS);
-	for_each_set_bit(offset, &stat, 8)
-		generic_handle_irq(irq_find_mapping(epg->gc[1].irq.domain,
-						    offset));
+	return stat;
+}
 
-	chained_irq_exit(irqchip, desc);
+static irqreturn_t ep93xx_ab_irq_handler(int irq, void *dev_id)
+{
+	return IRQ_RETVAL(ep93xx_gpio_ab_irq_handler(dev_id));
 }
 
 static void ep93xx_gpio_f_irq_handler(struct irq_desc *desc)
@@ -152,53 +133,50 @@ static void ep93xx_gpio_f_irq_handler(struct irq_desc *desc)
 
 static void ep93xx_gpio_irq_ack(struct irq_data *d)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	struct irq_chip *ic = irq_data_get_irq_chip(d);
+	struct ep93xx_irq_chip *eic = to_ep93xx_irq_chip(ic);
 	int port_mask = BIT(d->irq & 7);
 
 	if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) {
-		gpio_int_type2[port] ^= port_mask; /* switch edge direction */
-		ep93xx_gpio_update_int_params(epg, port);
+		eic->gpio_int_type2 ^= port_mask; /* switch edge direction */
+		ep93xx_gpio_update_int_params(eic);
 	}
 
-	writeb(port_mask, epg->base + eoi_register_offset[port]);
+	writeb(port_mask, eic->eoi);
 }
 
 static void ep93xx_gpio_irq_mask_ack(struct irq_data *d)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	struct irq_chip *ic = irq_data_get_irq_chip(d);
+	struct ep93xx_irq_chip *eic = to_ep93xx_irq_chip(ic);
+
 	int port_mask = BIT(d->irq & 7);
 
 	if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH)
-		gpio_int_type2[port] ^= port_mask; /* switch edge direction */
+		eic->gpio_int_type2 ^= port_mask; /* switch edge direction */
 
-	gpio_int_unmasked[port] &= ~port_mask;
-	ep93xx_gpio_update_int_params(epg, port);
+	eic->gpio_int_unmasked &= ~port_mask;
+	ep93xx_gpio_update_int_params(eic);
 
-	writeb(port_mask, epg->base + eoi_register_offset[port]);
+	writeb(port_mask, eic->eoi);
 }
 
 static void ep93xx_gpio_irq_mask(struct irq_data *d)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	struct irq_chip *ic = irq_data_get_irq_chip(d);
+	struct ep93xx_irq_chip *eic = to_ep93xx_irq_chip(ic);
 
-	gpio_int_unmasked[port] &= ~BIT(d->irq & 7);
-	ep93xx_gpio_update_int_params(epg, port);
+	eic->gpio_int_unmasked &= ~BIT(d->irq & 7);
+	ep93xx_gpio_update_int_params(eic);
 }
 
 static void ep93xx_gpio_irq_unmask(struct irq_data *d)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	struct irq_chip *ic = irq_data_get_irq_chip(d);
+	struct ep93xx_irq_chip *eic = to_ep93xx_irq_chip(ic);
 
-	gpio_int_unmasked[port] |= BIT(d->irq & 7);
-	ep93xx_gpio_update_int_params(epg, port);
+	eic->gpio_int_unmasked |= BIT(d->irq & 7);
+	ep93xx_gpio_update_int_params(eic);
 }
 
 /*
@@ -209,8 +187,8 @@ static void ep93xx_gpio_irq_unmask(struct irq_data *d)
 static int ep93xx_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	struct irq_chip *ic = irq_data_get_irq_chip(d);
+	struct ep93xx_irq_chip *eic = to_ep93xx_irq_chip(ic);
 	int offset = d->irq & 7;
 	int port_mask = BIT(offset);
 	irq_flow_handler_t handler;
@@ -219,32 +197,32 @@ static int ep93xx_gpio_irq_type(struct irq_data *d, unsigned int type)
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
-		gpio_int_type1[port] |= port_mask;
-		gpio_int_type2[port] |= port_mask;
+		eic->gpio_int_type1 |= port_mask;
+		eic->gpio_int_type2 |= port_mask;
 		handler = handle_edge_irq;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		gpio_int_type1[port] |= port_mask;
-		gpio_int_type2[port] &= ~port_mask;
+		eic->gpio_int_type1 |= port_mask;
+		eic->gpio_int_type2 &= ~port_mask;
 		handler = handle_edge_irq;
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		gpio_int_type1[port] &= ~port_mask;
-		gpio_int_type2[port] |= port_mask;
+		eic->gpio_int_type1 &= ~port_mask;
+		eic->gpio_int_type2 |= port_mask;
 		handler = handle_level_irq;
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		gpio_int_type1[port] &= ~port_mask;
-		gpio_int_type2[port] &= ~port_mask;
+		eic->gpio_int_type1 &= ~port_mask;
+		eic->gpio_int_type2 &= ~port_mask;
 		handler = handle_level_irq;
 		break;
 	case IRQ_TYPE_EDGE_BOTH:
-		gpio_int_type1[port] |= port_mask;
+		eic->gpio_int_type1 |= port_mask;
 		/* set initial polarity based on current input level */
 		if (gc->get(gc, offset))
-			gpio_int_type2[port] &= ~port_mask; /* falling */
+			eic->gpio_int_type2 &= ~port_mask; /* falling */
 		else
-			gpio_int_type2[port] |= port_mask; /* rising */
+			eic->gpio_int_type2 |= port_mask; /* rising */
 		handler = handle_edge_irq;
 		break;
 	default:
@@ -253,70 +231,96 @@ static int ep93xx_gpio_irq_type(struct irq_data *d, unsigned int type)
 
 	irq_set_handler_locked(d, handler);
 
-	gpio_int_enabled[port] |= port_mask;
+	eic->gpio_int_enabled |= port_mask;
 
-	ep93xx_gpio_update_int_params(epg, port);
+	ep93xx_gpio_update_int_params(eic);
 
 	return 0;
 }
 
-static struct irq_chip ep93xx_gpio_irq_chip = {
-	.name		= "GPIO",
-	.irq_ack	= ep93xx_gpio_irq_ack,
-	.irq_mask_ack	= ep93xx_gpio_irq_mask_ack,
-	.irq_mask	= ep93xx_gpio_irq_mask,
-	.irq_unmask	= ep93xx_gpio_irq_unmask,
-	.irq_set_type	= ep93xx_gpio_irq_type,
-};
+static void ep93xx_init_irq_chips(struct ep93xx_gpio *egp)
+{
+	int i;
+	struct ep93xx_irq_chip *eic;
+	struct irq_chip *ic;
+
+	for (i = 0; i < ARRAY_SIZE(egp->ic); i++) {
+		eic = &egp->ic[i];
+		ic = &eic->chip;
+
+		eic->int_type1 = egp->base + int_type1_register_offset[i];
+		eic->int_type2 = egp->base + int_type2_register_offset[i];
+		eic->eoi = egp->base + eoi_register_offset[i];
+		eic->en = egp->base + int_en_register_offset[i];
+		eic->debounce = egp->base + int_debounce_register_offset[i];
+		eic->status = egp->base + int_status_register_offset[i];
+
+		/* maybe read from regs ?  */
+		eic->gpio_int_unmasked = 0;
+		eic->gpio_int_enabled = 0;
+		eic->gpio_int_type1 = 0;
+		eic->gpio_int_type2 = 0;
+		eic->gpio_int_debounce = 0;
+
+		ic->name = irq_chip_names[i];
+		ic->irq_ack	= ep93xx_gpio_irq_ack;
+		ic->irq_mask_ack	= ep93xx_gpio_irq_mask_ack;
+		ic->irq_mask	= ep93xx_gpio_irq_mask;
+		ic->irq_unmask	= ep93xx_gpio_irq_unmask;
+		ic->irq_set_type	= ep93xx_gpio_irq_type;
+	}
+}
 
 /*************************************************************************
  * gpiolib interface for EP93xx on-chip GPIOs
  *************************************************************************/
+
+
 struct ep93xx_gpio_bank {
+	u8		idx;
 	const char	*label;
 	int		data;
 	int		dir;
 	int		base;
-	bool		has_irq;
-	bool		has_hierarchical_irq;
 	unsigned int	irq_base;
 };
 
-#define EP93XX_GPIO_BANK(_label, _data, _dir, _base, _has_irq, _has_hier, _irq_base) \
-	{							\
-		.label		= _label,			\
-		.data		= _data,			\
-		.dir		= _dir,				\
-		.base		= _base,			\
-		.has_irq	= _has_irq,			\
-		.has_hierarchical_irq = _has_hier,		\
-		.irq_base	= _irq_base,			\
+#define EP93XX_GPIO_BANK(_idx, _label, _data, _dir, _base, _irq_base)	\
+	{								\
+		.idx		= _idx,					\
+		.label		= _label,				\
+		.data		= _data,				\
+		.dir		= _dir,					\
+		.base		= _base,				\
+		.irq_base	= _irq_base,				\
 	}
 
 static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
 	/* Bank A has 8 IRQs */
-	EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, 64),
+	EP93XX_GPIO_BANK(0, "A", 0x00, 0x10, 0, 64),
 	/* Bank B has 8 IRQs */
-	EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, 72),
-	EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0),
-	EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0),
-	EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0),
+	EP93XX_GPIO_BANK(1, "B", 0x04, 0x14, 8, 72),
+	EP93XX_GPIO_BANK(2, "C", 0x08, 0x18, 40, 0),
+	EP93XX_GPIO_BANK(3, "D", 0x0c, 0x1c, 24, 0),
+	EP93XX_GPIO_BANK(4, "E", 0x20, 0x24, 32, 0),
 	/* Bank F has 8 IRQs */
-	EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, 0),
-	EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
-	EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
+	EP93XX_GPIO_BANK(5, "F", 0x30, 0x34, 16, 0),
+	EP93XX_GPIO_BANK(6, "G", 0x38, 0x3c, 48, 0),
+	EP93XX_GPIO_BANK(7, "H", 0x40, 0x44, 56, 0),
 };
 
 static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned offset,
 				  unsigned long config)
 {
+	struct irq_chip *ic = gc->irq.chip;
+	struct ep93xx_irq_chip *eic = to_ep93xx_irq_chip(ic);
 	u32 debounce;
 
 	if (pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE)
 		return -ENOTSUPP;
 
 	debounce = pinconf_to_config_argument(config);
-	ep93xx_gpio_int_debounce(gc, offset, debounce ? true : false);
+	ep93xx_gpio_int_debounce(eic, offset, debounce ? true : false);
 
 	return 0;
 }
@@ -326,6 +330,88 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned offset)
 	return EP93XX_GPIO_F_IRQ_BASE + offset;
 }
 
+static int ep93xx_gpio_add_ab_irq_chip(struct gpio_chip *gc,
+					struct platform_device *pdev,
+					struct ep93xx_irq_chip *eic,
+					unsigned int irq_base)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *girq = &gc->irq;
+	int ab_parent_irq;
+	int err;
+
+	gc->set_config = ep93xx_gpio_set_config;
+	girq->chip = &eic->chip;
+
+	ab_parent_irq = platform_get_irq(pdev, 0);
+
+	err = devm_request_irq(dev, ab_parent_irq,
+			ep93xx_ab_irq_handler,
+			IRQF_SHARED, eic->chip.name, gc);
+
+	if (err) {
+		dev_err(dev, "error requesting IRQ : %d\n", ab_parent_irq);
+		return err;
+	}
+
+	girq->num_parents = 1;
+	girq->parents = devm_kcalloc(dev, 1,
+				sizeof(*girq->parents),
+				GFP_KERNEL);
+	if (!girq->parents)
+		return -ENOMEM;
+
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+	girq->parents[0] = ab_parent_irq;
+	girq->first = irq_base;
+
+	return 0;
+}
+
+static int ep93xx_gpio_add_f_irq_chip(struct gpio_chip *gc,
+				      struct platform_device *pdev,
+				      struct ep93xx_irq_chip *eic)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *girq = &gc->irq;
+	int gpio_irq;
+	int i;
+
+	gc->set_config = ep93xx_gpio_set_config;
+	girq->chip = &eic->chip;
+
+	/*
+	 * FIXME: convert this to use hierarchical IRQ support!
+	 * this requires fixing the root irqchip to be hierarchial.
+	 */
+	girq->parent_handler = ep93xx_gpio_f_irq_handler;
+	girq->num_parents = 8;
+	girq->parents = devm_kcalloc(dev, 8,
+				     sizeof(*girq->parents),
+				     GFP_KERNEL);
+
+	if (!girq->parents)
+		return -ENOMEM;
+
+	/* Pick resources 1..8 for these IRQs */
+	for (i = 1; i <= 8; i++)
+		girq->parents[i - 1] = platform_get_irq(pdev, i);
+	for (i = 0; i < 8; i++) {
+		gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
+		irq_set_chip_data(gpio_irq, gc);
+		irq_set_chip_and_handler(gpio_irq,
+					girq->chip,
+					handle_level_irq);
+		irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST);
+	}
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+	gc->to_irq = ep93xx_gpio_f_to_irq;
+
+	return 0;
+}
+
 static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 				struct platform_device *pdev,
 				struct ep93xx_gpio *epg,
@@ -334,7 +420,6 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 	void __iomem *data = epg->base + bank->data;
 	void __iomem *dir = epg->base + bank->dir;
 	struct device *dev = &pdev->dev;
-	struct gpio_irq_chip *girq;
 	int err;
 
 	err = bgpio_init(gc, dev, 1, data, NULL, NULL, dir, NULL, 0);
@@ -344,58 +429,17 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 	gc->label = bank->label;
 	gc->base = bank->base;
 
-	girq = &gc->irq;
-	if (bank->has_irq || bank->has_hierarchical_irq) {
-		gc->set_config = ep93xx_gpio_set_config;
-		girq->chip = &ep93xx_gpio_irq_chip;
-	}
-
-	if (bank->has_irq) {
-		int ab_parent_irq = platform_get_irq(pdev, 0);
-
-		girq->parent_handler = ep93xx_gpio_ab_irq_handler;
-		girq->num_parents = 1;
-		girq->parents = devm_kcalloc(dev, 1,
-					     sizeof(*girq->parents),
-					     GFP_KERNEL);
-		if (!girq->parents)
-			return -ENOMEM;
-		girq->default_type = IRQ_TYPE_NONE;
-		girq->handler = handle_level_irq;
-		girq->parents[0] = ab_parent_irq;
-		girq->first = bank->irq_base;
+	if (bank->idx == 0 || bank->idx == 1) {
+		err = ep93xx_gpio_add_ab_irq_chip(gc, pdev, &epg->ic[bank->idx], bank->irq_base);
+		if (err)
+			return err;
 	}
 
-	/* Only bank F has especially funky IRQ handling */
-	if (bank->has_hierarchical_irq) {
-		int gpio_irq;
-		int i;
-
-		/*
-		 * FIXME: convert this to use hierarchical IRQ support!
-		 * this requires fixing the root irqchip to be hierarchial.
-		 */
-		girq->parent_handler = ep93xx_gpio_f_irq_handler;
-		girq->num_parents = 8;
-		girq->parents = devm_kcalloc(dev, 8,
-					     sizeof(*girq->parents),
-					     GFP_KERNEL);
-		if (!girq->parents)
-			return -ENOMEM;
-		/* Pick resources 1..8 for these IRQs */
-		for (i = 1; i <= 8; i++)
-			girq->parents[i - 1] = platform_get_irq(pdev, i);
-		for (i = 0; i < 8; i++) {
-			gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
-			irq_set_chip_data(gpio_irq, &epg->gc[5]);
-			irq_set_chip_and_handler(gpio_irq,
-						 &ep93xx_gpio_irq_chip,
-						 handle_level_irq);
-			irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST);
-		}
-		girq->default_type = IRQ_TYPE_NONE;
-		girq->handler = handle_level_irq;
-		gc->to_irq = ep93xx_gpio_f_to_irq;
+	if (bank->idx == 5) {
+		/* Only bank F has especially funky IRQ handling */
+		err = ep93xx_gpio_add_f_irq_chip(gc, pdev, &epg->ic[2]);
+		if (err)
+			return err;
 	}
 
 	return devm_gpiochip_add_data(dev, gc, epg);
@@ -414,6 +458,9 @@ static int ep93xx_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(epg->base))
 		return PTR_ERR(epg->base);
 
+	/* init irq chips */
+	ep93xx_init_irq_chips(epg);
+
 	for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_banks); i++) {
 		struct gpio_chip *gc = &epg->gc[i];
 		struct ep93xx_gpio_bank *bank = &ep93xx_gpio_banks[i];
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 2/3] gpio: ep93xx: drop to_irq binding
  2020-12-24 11:22 [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Nikita Shubin
  2020-12-24 11:22 ` [RFC PATCH 1/3] " Nikita Shubin
@ 2020-12-24 11:22 ` Nikita Shubin
  2020-12-27 21:21   ` Linus Walleij
  2020-12-24 11:22 ` [RFC PATCH 3/3] gpio: ep93xx: specify gpio_irq_chip->first Nikita Shubin
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Nikita Shubin @ 2020-12-24 11:22 UTC (permalink / raw)
  Cc: Nikita Shubin, Linus Walleij, Bartosz Golaszewski, linux-gpio,
	linux-kernel

As ->to_irq is redefined in gpiochip_add_irqchip, having it defined in
driver is useless, so let's drop it.

Also i think it is worth to give a gentle warning in
gpiochip_add_irqchip, to prevent people relying on to_irq.

For example

WARN_ON_ONCE(gc->to_irq,
"to_irq is redefined in gpiochip_add_irqchip" \
"and you shouldn't rely on it\n");

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index d6db0ff16581..90afe07213ce 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -325,11 +325,6 @@ static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned offset,
 	return 0;
 }
 
-static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned offset)
-{
-	return EP93XX_GPIO_F_IRQ_BASE + offset;
-}
-
 static int ep93xx_gpio_add_ab_irq_chip(struct gpio_chip *gc,
 					struct platform_device *pdev,
 					struct ep93xx_irq_chip *eic,
@@ -407,7 +402,6 @@ static int ep93xx_gpio_add_f_irq_chip(struct gpio_chip *gc,
 	}
 	girq->default_type = IRQ_TYPE_NONE;
 	girq->handler = handle_level_irq;
-	gc->to_irq = ep93xx_gpio_f_to_irq;
 
 	return 0;
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 3/3] gpio: ep93xx: specify gpio_irq_chip->first
  2020-12-24 11:22 [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Nikita Shubin
  2020-12-24 11:22 ` [RFC PATCH 1/3] " Nikita Shubin
  2020-12-24 11:22 ` [RFC PATCH 2/3] gpio: ep93xx: drop to_irq binding Nikita Shubin
@ 2020-12-24 11:22 ` Nikita Shubin
  2020-12-27 21:22   ` Linus Walleij
  2020-12-26 17:34 ` [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Andy Shevchenko
  2020-12-27 13:55 ` Linus Walleij
  4 siblings, 1 reply; 14+ messages in thread
From: Nikita Shubin @ 2020-12-24 11:22 UTC (permalink / raw)
  Cc: Nikita Shubin, Linus Walleij, Bartosz Golaszewski, linux-gpio,
	linux-kernel

Port F irq's should be statically mapped to EP93XX_GPIO_F_IRQ_BASE.

So we need to specify girq->first otherwise:

"If device tree is used, then first_irq will be 0 and
irqs get mapped dynamically on the fly"

And that's not the thing we want.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 90afe07213ce..a321a7441294 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -402,6 +402,7 @@ static int ep93xx_gpio_add_f_irq_chip(struct gpio_chip *gc,
 	}
 	girq->default_type = IRQ_TYPE_NONE;
 	girq->handler = handle_level_irq;
+	girq->first = EP93XX_GPIO_F_IRQ_BASE;
 
 	return 0;
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips
  2020-12-24 11:22 [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Nikita Shubin
                   ` (2 preceding siblings ...)
  2020-12-24 11:22 ` [RFC PATCH 3/3] gpio: ep93xx: specify gpio_irq_chip->first Nikita Shubin
@ 2020-12-26 17:34 ` Andy Shevchenko
  2020-12-27 14:00   ` Linus Walleij
  2020-12-27 13:55 ` Linus Walleij
  4 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2020-12-26 17:34 UTC (permalink / raw)
  To: Nikita Shubin
  Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

On Thu, Dec 24, 2020 at 1:24 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> I was lucky enough to became an owner of some splendid piece's of
> antiques called ts7250 based on the top of Cirrus Logic EP9302.
>
> I don't know what fate expects this hardware (it's not EOL it's just Not
> recommended for new designs) but i wanted to share fixes in ep93xx gpio area.
>
> It seems ep93xx is deadly broken at current state usage of AB gpiochips
> interrupts leads to deadlocks coused by irq_unmask/irq_mask recursions.

I personally consider it quite nice to have support even for outdated
hardware (reminds me about a recent LWN article about 32-bit
architectures and a comment there how it could affect the environment
if we drop them from being supported).

So I fully support this series.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 1/3] gpio: ep93xx: convert to multi irqchips
  2020-12-24 11:22 ` [RFC PATCH 1/3] " Nikita Shubin
@ 2020-12-26 17:52   ` Andy Shevchenko
  2020-12-28 15:05     ` nikita.shubin
  0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2020-12-26 17:52 UTC (permalink / raw)
  To: Nikita Shubin
  Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

On Thu, Dec 24, 2020 at 1:23 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> Since gpiolib requires having separate irqchips for each gpiochip, we
> need to add some we definetly need a separate one for F port, and we

definitely

> could combine gpiochip A and B into one - but this will break namespace
> and logick.
>
> So despite 3 irqchips is a bit beefy we need a separate irqchip for each

is a -> being a

> interrupt capable port.
>
> - added separate irqchip for each iterrupt capable gpiochip

interrupt

> - dropped ep93xx_gpio_port (it wasn't working correct for port F anyway)
> - moved irq registers into separate struct ep93xx_irq_chip, togather

irq -> IRQ (everywhere)

together

>   with regs current state
> - reworked irq handle for ab gpiochips (through bit not tottaly sure this
>   is a correct thing to do)

ab -> AB ?

In the parentheses something like "I'm not totally sure that this is a
correct thing to do, though".

> - dropped has_irq and has_hierarchical_irq and added a simple index
>   which we rely on when adding irq's to gpiochip's

IRQs to GPIO chips

(It would be nice if you can spell check and proofread  commit
messages and comments in the code.

...

> +struct ep93xx_irq_chip {
> +       void __iomem    *int_type1;
> +       void __iomem    *int_type2;
> +       void __iomem    *eoi;
> +       void __iomem    *en;
> +       void __iomem    *debounce;
> +       void __iomem    *status;

This is a bit... overcomplicated.
Can we rather use regmap API?

> +       u8              gpio_int_unmasked;
> +       u8              gpio_int_enabled;
> +       u8              gpio_int_type1;
> +       u8              gpio_int_type2;
> +       u8              gpio_int_debounce;
> +       struct  irq_chip chip;
> +};

...

>  /* Port ordering is: A B F */
> +static const char *irq_chip_names[3]           = {"gpio-irq-a",
> +                                               "gpio-irq-b",
> +                                               "gpio-irq-f"};

Can you use better pattern, ie.
static const char * const foo[] = {
  ...
};

(there are two things: splitting per lines and additional const)?

...

> +       ab_parent_irq = platform_get_irq(pdev, 0);

Error check, please?
Also, if it's an optional resource, use platform_get_irq_optional().

> +       err = devm_request_irq(dev, ab_parent_irq,
> +                       ep93xx_ab_irq_handler,
> +                       IRQF_SHARED, eic->chip.name, gc);

> +

Redundant blank line.

> +       if (err) {
> +               dev_err(dev, "error requesting IRQ : %d\n", ab_parent_irq);
> +               return err;
> +       }

...

> +       girq->num_parents = 1;
> +       girq->parents = devm_kcalloc(dev, 1,
> +                               sizeof(*girq->parents),
> +                               GFP_KERNEL);

Can be squeezed to less amount of LOCs. Also consider to use
girq->num_parents as a parameter to devm_kcalloc().

> +       if (!girq->parents)
> +               return -ENOMEM;

...

> +       girq->handler = handle_level_irq;

Don't we want to mark them as bad by using handle_bad_irq() as default handler?

...

> +       /*
> +        * FIXME: convert this to use hierarchical IRQ support!
> +        * this requires fixing the root irqchip to be hierarchial.

hierarchical

> +        */

...

> +       girq->num_parents = 8;
> +       girq->parents = devm_kcalloc(dev, 8,
> +                                    sizeof(*girq->parents),
> +                                    GFP_KERNEL);

As per above.

> +

Redundant blank line.

> +       if (!girq->parents)
> +               return -ENOMEM;

...

> +       /* Pick resources 1..8 for these IRQs */
> +       for (i = 1; i <= 8; i++)
> +               girq->parents[i - 1] = platform_get_irq(pdev, i);

I would rather like to see i + 1 as a parameter which is much easier
to read and understand.

> +       for (i = 0; i < 8; i++) {

Also in both cases replace 8 by ARRAY_SIZE() or predefined constant.

> +               gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
> +               irq_set_chip_data(gpio_irq, gc);
> +               irq_set_chip_and_handler(gpio_irq,
> +                                       girq->chip,
> +                                       handle_level_irq);
> +               irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST);
> +       }

Okay, I see that this is in the original code. Consider them as
suggestions for additional changes.

And briefly looking into the rest of the code the recommendation is to
split this and perhaps other patches to smaller logical pieces.

Also, try to organize your series in groups each of them respectively
represents the following
1) fixes (can be backported, usually contain Fixes tag to the culprit commit);
2) preparatory refactoring patches / cleanups;
3) new features.


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips
  2020-12-24 11:22 [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Nikita Shubin
                   ` (3 preceding siblings ...)
  2020-12-26 17:34 ` [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Andy Shevchenko
@ 2020-12-27 13:55 ` Linus Walleij
  2020-12-28 15:14   ` nikita.shubin
  4 siblings, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2020-12-27 13:55 UTC (permalink / raw)
  To: Nikita Shubin, Hartley Sweeten
  Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, linux-kernel

Paging Hartley who I think uses this chip.

On Thu, Dec 24, 2020 at 12:22 PM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:

> I was lucky enough to became an owner of some splendid piece's of
> antiques called ts7250 based on the top of Cirrus Logic EP9302.
>
> I don't know what fate expects this hardware (it's not EOL it's just Not
> recommended for new designs) but i wanted to share fixes in ep93xx gpio area.
>
> It seems ep93xx is deadly broken at current state usage of AB gpiochips
> interrupts leads to deadlocks coused by irq_unmask/irq_mask recursions.
>
> Port F is not working at all:

Yeah when I converted this gpio driver I did not have access to a hardware
which made use of port F :(

I used the SIM.One board but they all died on me, then I acquired another
EP93xx board that I just didn't have time to test.

The long term plan is to convert these boards to use device tree and
multiplatform. Interested in the job? ;)

Thanks for fixing up the GPIO chip, let's get the patches in shape and
merge as fixes. Bartosz is handling the fixes right now, I'll try to review too!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips
  2020-12-26 17:34 ` [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Andy Shevchenko
@ 2020-12-27 14:00   ` Linus Walleij
  0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2020-12-27 14:00 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Nikita Shubin, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

On Sat, Dec 26, 2020 at 6:34 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> I personally consider it quite nice to have support even for outdated
> hardware (reminds me about a recent LWN article about 32-bit
> architectures and a comment there how it could affect the environment
> if we drop them from being supported).
>
> So I fully support this series.

I had a lecture on the subject actually where EP93xx came up:
https://docs.google.com/presentation/d/1pv9MTCDpb0EgzrzicA9JF3pdUL6EcUmKhTavk5_hOHs

As seen it is considered a "fully developed product" and it would not
surprise me if Cirrus is taping out new EP93xx chips even.

This SoC is definitely on my shortlist of stuff we need to support.
(I wish Cirrus would provide some manpower but ...)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/3] gpio: ep93xx: drop to_irq binding
  2020-12-24 11:22 ` [RFC PATCH 2/3] gpio: ep93xx: drop to_irq binding Nikita Shubin
@ 2020-12-27 21:21   ` Linus Walleij
  2021-01-04 14:17     ` Bartosz Golaszewski
  0 siblings, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2020-12-27 21:21 UTC (permalink / raw)
  To: Nikita Shubin; +Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, linux-kernel

On Thu, Dec 24, 2020 at 12:22 PM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:

> As ->to_irq is redefined in gpiochip_add_irqchip, having it defined in
> driver is useless, so let's drop it.
>
> Also i think it is worth to give a gentle warning in
> gpiochip_add_irqchip, to prevent people relying on to_irq.
>
> For example
>
> WARN_ON_ONCE(gc->to_irq,
> "to_irq is redefined in gpiochip_add_irqchip" \
> "and you shouldn't rely on it\n");
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Take out the suggestion in the commit message and implement it,
but I think WARN_ON is too nasty, just use dev_err().

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 3/3] gpio: ep93xx: specify gpio_irq_chip->first
  2020-12-24 11:22 ` [RFC PATCH 3/3] gpio: ep93xx: specify gpio_irq_chip->first Nikita Shubin
@ 2020-12-27 21:22   ` Linus Walleij
  0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2020-12-27 21:22 UTC (permalink / raw)
  To: Nikita Shubin; +Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, linux-kernel

On Thu, Dec 24, 2020 at 12:22 PM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:

> Port F irq's should be statically mapped to EP93XX_GPIO_F_IRQ_BASE.
>
> So we need to specify girq->first otherwise:
>
> "If device tree is used, then first_irq will be 0 and
> irqs get mapped dynamically on the fly"
>
> And that's not the thing we want.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

We can only fix this properly once we convert the platform
to device tree. (Along with making the irqchip hierarchical.)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 1/3] gpio: ep93xx: convert to multi irqchips
  2020-12-26 17:52   ` Andy Shevchenko
@ 2020-12-28 15:05     ` nikita.shubin
  0 siblings, 0 replies; 14+ messages in thread
From: nikita.shubin @ 2020-12-28 15:05 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

26.12.2020, 20:52, "Andy Shevchenko" <andy.shevchenko@gmail.com>:
> On Thu, Dec 24, 2020 at 1:23 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>>  Since gpiolib requires having separate irqchips for each gpiochip, we
>>  need to add some we definetly need a separate one for F port, and we
>
> definitely
>
>>  could combine gpiochip A and B into one - but this will break namespace
>>  and logick.
>>
>>  So despite 3 irqchips is a bit beefy we need a separate irqchip for each
>
> is a -> being a
>
>>  interrupt capable port.
>>
>>  - added separate irqchip for each iterrupt capable gpiochip
>
> interrupt
>
>>  - dropped ep93xx_gpio_port (it wasn't working correct for port F anyway)
>>  - moved irq registers into separate struct ep93xx_irq_chip, togather
>
> irq -> IRQ (everywhere)
>
> together
>
>>    with regs current state
>>  - reworked irq handle for ab gpiochips (through bit not tottaly sure this
>>    is a correct thing to do)
>
> ab -> AB ?
>
> In the parentheses something like "I'm not totally sure that this is a
> correct thing to do, though".
>
>>  - dropped has_irq and has_hierarchical_irq and added a simple index
>>    which we rely on when adding irq's to gpiochip's
>
> IRQs to GPIO chips
>
> (It would be nice if you can spell check and proofread commit
> messages and comments in the code.
>
> ...
>
>>  +struct ep93xx_irq_chip {
>>  + void __iomem *int_type1;
>>  + void __iomem *int_type2;
>>  + void __iomem *eoi;
>>  + void __iomem *en;
>>  + void __iomem *debounce;
>>  + void __iomem *status;
>
> This is a bit... overcomplicated.
> Can we rather use regmap API?
>
>>  + u8 gpio_int_unmasked;
>>  + u8 gpio_int_enabled;
>>  + u8 gpio_int_type1;
>>  + u8 gpio_int_type2;
>>  + u8 gpio_int_debounce;
>>  + struct irq_chip chip;
>>  +};
>
> ...
>
>>   /* Port ordering is: A B F */
>>  +static const char *irq_chip_names[3] = {"gpio-irq-a",
>>  + "gpio-irq-b",
>>  + "gpio-irq-f"};
>
> Can you use better pattern, ie.
> static const char * const foo[] = {
>   ...
> };
>
> (there are two things: splitting per lines and additional const)?
>
> ...
>
>>  + ab_parent_irq = platform_get_irq(pdev, 0);
>
> Error check, please?
> Also, if it's an optional resource, use platform_get_irq_optional().
>
>>  + err = devm_request_irq(dev, ab_parent_irq,
>>  + ep93xx_ab_irq_handler,
>>  + IRQF_SHARED, eic->chip.name, gc);
>
>>  +
>
> Redundant blank line.
>
>>  + if (err) {
>>  + dev_err(dev, "error requesting IRQ : %d\n", ab_parent_irq);
>>  + return err;
>>  + }
>
> ...
>
>>  + girq->num_parents = 1;
>>  + girq->parents = devm_kcalloc(dev, 1,
>>  + sizeof(*girq->parents),
>>  + GFP_KERNEL);
>
> Can be squeezed to less amount of LOCs. Also consider to use
> girq->num_parents as a parameter to devm_kcalloc().
>
>>  + if (!girq->parents)
>>  + return -ENOMEM;
>
> ...
>
>>  + girq->handler = handle_level_irq;
>
> Don't we want to mark them as bad by using handle_bad_irq() as default handler?
>
> ...
>
>>  + /*
>>  + * FIXME: convert this to use hierarchical IRQ support!
>>  + * this requires fixing the root irqchip to be hierarchial.
>
> hierarchical
>
>>  + */
>
> ...
>
>>  + girq->num_parents = 8;
>>  + girq->parents = devm_kcalloc(dev, 8,
>>  + sizeof(*girq->parents),
>>  + GFP_KERNEL);
>
> As per above.
>
>>  +
>
> Redundant blank line.
>
>>  + if (!girq->parents)
>>  + return -ENOMEM;
>
> ...
>
>>  + /* Pick resources 1..8 for these IRQs */
>>  + for (i = 1; i <= 8; i++)
>>  + girq->parents[i - 1] = platform_get_irq(pdev, i);
>
> I would rather like to see i + 1 as a parameter which is much easier
> to read and understand.
>
>>  + for (i = 0; i < 8; i++) {
>
> Also in both cases replace 8 by ARRAY_SIZE() or predefined constant.
>
>>  + gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
>>  + irq_set_chip_data(gpio_irq, gc);
>>  + irq_set_chip_and_handler(gpio_irq,
>>  + girq->chip,
>>  + handle_level_irq);
>>  + irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST);
>>  + }
>
> Okay, I see that this is in the original code. Consider them as
> suggestions for additional changes.
>
> And briefly looking into the rest of the code the recommendation is to
> split this and perhaps other patches to smaller logical pieces.
>
> Also, try to organize your series in groups each of them respectively
> represents the following
> 1) fixes (can be backported, usually contain Fixes tag to the culprit commit);
> 2) preparatory refactoring patches / cleanups;
> 3) new features.
>
> --
> With Best Regards,
> Andy Shevchenko

Thank you, Andy, i ll rework my RFC patch according to your advices and resubmit.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips
  2020-12-27 13:55 ` Linus Walleij
@ 2020-12-28 15:14   ` nikita.shubin
  2020-12-28 20:41     ` Linus Walleij
  0 siblings, 1 reply; 14+ messages in thread
From: nikita.shubin @ 2020-12-28 15:14 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, linux-kernel,
	Hartley Sweeten

27.12.2020, 16:55, "Linus Walleij" <linus.walleij@linaro.org>:
> Paging Hartley who I think uses this chip.
>
> On Thu, Dec 24, 2020 at 12:22 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
>
>>  I was lucky enough to became an owner of some splendid piece's of
>>  antiques called ts7250 based on the top of Cirrus Logic EP9302.
>>
>>  I don't know what fate expects this hardware (it's not EOL it's just Not
>>  recommended for new designs) but i wanted to share fixes in ep93xx gpio area.
>>
>>  It seems ep93xx is deadly broken at current state usage of AB gpiochips
>>  interrupts leads to deadlocks coused by irq_unmask/irq_mask recursions.
>>
>>  Port F is not working at all:
>
> Yeah when I converted this gpio driver I did not have access to a hardware
> which made use of port F :(
>
> I used the SIM.One board but they all died on me, then I acquired another
> EP93xx board that I just didn't have time to test.
>
> The long term plan is to convert these boards to use device tree and
> multiplatform. Interested in the job? ;)
>

I had such a thought, but I was stopped by the delusion that no one is using this 
hardware anymore.

Not promising anything right now - but such a job sounds like a real fun.

> Thanks for fixing up the GPIO chip, let's get the patches in shape and
> merge as fixes. Bartosz is handling the fixes right now, I'll try to review too!

I am glad that this work is interesting not only for me. For this, I'm willing to put in 
a little effort to make the proposed changes look decent.

>
> Yours,
> Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips
  2020-12-28 15:14   ` nikita.shubin
@ 2020-12-28 20:41     ` Linus Walleij
  0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2020-12-28 20:41 UTC (permalink / raw)
  To: Nikita Shubin
  Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, linux-kernel,
	Hartley Sweeten

On Mon, Dec 28, 2020 at 4:14 PM <nikita.shubin@maquefel.me> wrote:

> I had such a thought, but I was stopped by the delusion that no one is using this
> hardware anymore.

It is definately *used* a lot. It is dubious if it is used with the
latest kernel.
Hartley was/is using it on engraving machinery and using the latest
kernel, I think his setup is pretty large.

The number of deployed machines using EP93xx is likely large, but
more importantly, as with MOXA Art it is likely to be internet connected
and managing vital control systems.

In short: nobody may be upgrading these kernels, but the definately
*should* be upgrading them. For security reasons.

So as a community hat I have concluded it would be socially unacceptable
to delete it and not offer an upgrade path for these systems the
day they realize "oh s.... that is an internet connected pressure boiler".

So if possible it should be modernized.

> Not promising anything right now - but such a job sounds like a real fun.

I would be grateful.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/3] gpio: ep93xx: drop to_irq binding
  2020-12-27 21:21   ` Linus Walleij
@ 2021-01-04 14:17     ` Bartosz Golaszewski
  0 siblings, 0 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2021-01-04 14:17 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Nikita Shubin, open list:GPIO SUBSYSTEM, linux-kernel

On Sun, Dec 27, 2020 at 10:21 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Dec 24, 2020 at 12:22 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
>
> > As ->to_irq is redefined in gpiochip_add_irqchip, having it defined in
> > driver is useless, so let's drop it.
> >
> > Also i think it is worth to give a gentle warning in
> > gpiochip_add_irqchip, to prevent people relying on to_irq.
> >
> > For example
> >
> > WARN_ON_ONCE(gc->to_irq,
> > "to_irq is redefined in gpiochip_add_irqchip" \
> > "and you shouldn't rely on it\n");
> >
> > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Take out the suggestion in the commit message and implement it,
> but I think WARN_ON is too nasty, just use dev_err().
>
> Yours,
> Linus Walleij

This depends on patch 1/3 so I'll wait until you resend with issues addressed.

Bartosz

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-01-04 14:18 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-24 11:22 [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Nikita Shubin
2020-12-24 11:22 ` [RFC PATCH 1/3] " Nikita Shubin
2020-12-26 17:52   ` Andy Shevchenko
2020-12-28 15:05     ` nikita.shubin
2020-12-24 11:22 ` [RFC PATCH 2/3] gpio: ep93xx: drop to_irq binding Nikita Shubin
2020-12-27 21:21   ` Linus Walleij
2021-01-04 14:17     ` Bartosz Golaszewski
2020-12-24 11:22 ` [RFC PATCH 3/3] gpio: ep93xx: specify gpio_irq_chip->first Nikita Shubin
2020-12-27 21:22   ` Linus Walleij
2020-12-26 17:34 ` [RFC PATCH 0/3] gpio: ep93xx: convert to multi irqchips Andy Shevchenko
2020-12-27 14:00   ` Linus Walleij
2020-12-27 13:55 ` Linus Walleij
2020-12-28 15:14   ` nikita.shubin
2020-12-28 20:41     ` Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.