[PATCH v2 0/5] Add GPIO support for MStar/SigmaStar ARMv7

[PATCH v2 0/5] Add GPIO support for MStar/SigmaStar ARMv7

[Daniel Palmer]

At the moment the MStar/SigmaStar support is only really
capable of shell from an initramfs and not much else.

Most of the interesting drivers are blocked on clock and pinctrl
drivers and those are going to take me a little while to get cleaned
up.

Clock and pinctrl aren't needed for basic GPIO to work (all pins
start off as GPIOs..) and it makes it possible to actually do something
so this series adds everything that is needed for the main GPIO
block in these chips.

Changes since v1:

- Moves the binding header commit before the yaml commit
  
- Fixes the license on the binding header to include BSD-2-Clause

- The driver has been reworked to use the gpiolib irqchip functionality
  as suggested by Linus[0]. I think I got this right. The gpio controller
  doesn't actually do anything with interrupts itself.. It just happens
  to have 4 lines that are also wired to lines on one of the interrupt
  controllers.

- Now that the driver is an interrupt controller in it's own right for
  the gpio lines that have associated interrupts the binding description
  has been updated to add the interrupt-controller bits and remove the
  description of the interrupt-names that described how the interrupts
  used to be passed in.

Daniel Palmer (5):
  dt-bindings: gpio: Add a binding header for the MSC313 GPIO driver
  dt-bindings: gpio: Binding for MStar MSC313 GPIO controller
  gpio: msc313: MStar MSC313 GPIO driver
  ARM: mstar: Add gpio controller to MStar base dtsi
  ARM: mstar: Fill in GPIO controller properties for infinity

 .../bindings/gpio/mstar,msc313-gpio.yaml      |  61 +++
 MAINTAINERS                                   |   3 +
 arch/arm/boot/dts/mstar-infinity.dtsi         |   7 +
 arch/arm/boot/dts/mstar-v7.dtsi               |  10 +
 drivers/gpio/Kconfig                          |   9 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-msc313.c                    | 406 ++++++++++++++++++
 include/dt-bindings/gpio/msc313-gpio.h        |  95 ++++
 8 files changed, 592 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
 create mode 100644 drivers/gpio/gpio-msc313.c
 create mode 100644 include/dt-bindings/gpio/msc313-gpio.h

-- 
2.28.0

[PATCH v2 1/5] dt-bindings: gpio: Add a binding header for the MSC313 GPIO driver

[Daniel Palmer]

Header adds defines for the gpio number of each pin
from the driver view. The gpio block seems to support 128 lines
but what line is mapped to a physical pin depends on the chip.
The driver itself uses the index of a pin's offset in an array
of the possible offsets for a chip as the gpio number.

The defines remove the need to work out that index to consume
a pin in the device tree.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 MAINTAINERS                            |  1 +
 include/dt-bindings/gpio/msc313-gpio.h | 95 ++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)
 create mode 100644 include/dt-bindings/gpio/msc313-gpio.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 3f345f36c22c..a188fae8c04e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2132,6 +2132,7 @@ W:	http://linux-chenxing.org/
 F:	Documentation/devicetree/bindings/arm/mstar/*
 F:	arch/arm/boot/dts/mstar-*
 F:	arch/arm/mach-mstar/
+F:	include/dt-bindings/gpio/msc313-gpio.h
 
 ARM/NEC MOBILEPRO 900/c MACHINE SUPPORT
 M:	Michael Petchkovsky <mkpetch@internode.on.net>
diff --git a/include/dt-bindings/gpio/msc313-gpio.h b/include/dt-bindings/gpio/msc313-gpio.h
new file mode 100644
index 000000000000..9b8cd6ffb7c4
--- /dev/null
+++ b/include/dt-bindings/gpio/msc313-gpio.h
@@ -0,0 +1,95 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * GPIO definitions for MStar/SigmaStar MSC313 and later SoCs
+ *
+ * Copyright (C) 2020 Daniel Palmer <daniel@thingy.jp>
+ */
+
+#ifndef _DT_BINDINGS_MSC313_GPIO_H
+#define _DT_BINDINGS_MSC313_GPIO_H
+
+/* pin names for fuart, same for all SoCs so far */
+#define MSC313_PINNAME_FUART_RX		"fuart_rx"
+#define MSC313_PINNAME_FUART_TX		"fuart_tx"
+#define MSC313_PINNAME_FUART_CTS	"fuart_cts"
+#define MSC313_PINNAME_FUART_RTS	"fuart_rts"
+
+/* pin names for sr, mercury5 is different */
+#define MSC313_PINNAME_SR_IO2		"sr_io2"
+#define MSC313_PINNAME_SR_IO3		"sr_io3"
+#define MSC313_PINNAME_SR_IO4		"sr_io4"
+#define MSC313_PINNAME_SR_IO5		"sr_io5"
+#define MSC313_PINNAME_SR_IO6		"sr_io6"
+#define MSC313_PINNAME_SR_IO7		"sr_io7"
+#define MSC313_PINNAME_SR_IO8		"sr_io8"
+#define MSC313_PINNAME_SR_IO9		"sr_io9"
+#define MSC313_PINNAME_SR_IO10		"sr_io10"
+#define MSC313_PINNAME_SR_IO11		"sr_io11"
+#define MSC313_PINNAME_SR_IO12		"sr_io12"
+#define MSC313_PINNAME_SR_IO13		"sr_io13"
+#define MSC313_PINNAME_SR_IO14		"sr_io14"
+#define MSC313_PINNAME_SR_IO15		"sr_io15"
+#define MSC313_PINNAME_SR_IO16		"sr_io16"
+#define MSC313_PINNAME_SR_IO17		"sr_io17"
+
+/* pin names for sd, same for all SoCs so far */
+#define MSC313_PINNAME_SD_CLK		"sd_clk"
+#define MSC313_PINNAME_SD_CMD		"sd_cmd"
+#define MSC313_PINNAME_SD_D0		"sd_d0"
+#define MSC313_PINNAME_SD_D1		"sd_d1"
+#define MSC313_PINNAME_SD_D2		"sd_d2"
+#define MSC313_PINNAME_SD_D3		"sd_d3"
+
+/* pin names for i2c1, same for all SoCs so for */
+#define MSC313_PINNAME_I2C1_SCL		"i2c1_scl"
+#define MSC313_PINNAME_I2C1_SCA		"i2c1_sda"
+
+/* pin names for spi0, same for all SoCs so far */
+#define MSC313_PINNAME_SPI0_CZ		"spi0_cz"
+#define MSC313_PINNAME_SPI0_CK		"spi0_ck"
+#define MSC313_PINNAME_SPI0_DI		"spi0_di"
+#define MSC313_PINNAME_SPI0_DO		"spi0_do"
+
+#define MSC313_GPIO_FUART	0
+#define MSC313_GPIO_FUART_RX	(MSC313_GPIO_FUART + 0)
+#define MSC313_GPIO_FUART_TX	(MSC313_GPIO_FUART + 1)
+#define MSC313_GPIO_FUART_CTS	(MSC313_GPIO_FUART + 2)
+#define MSC313_GPIO_FUART_RTS	(MSC313_GPIO_FUART + 3)
+
+#define MSC313_GPIO_SR		(MSC313_GPIO_FUART_RTS + 1)
+#define MSC313_GPIO_SR_IO2	(MSC313_GPIO_SR + 0)
+#define MSC313_GPIO_SR_IO3	(MSC313_GPIO_SR + 1)
+#define MSC313_GPIO_SR_IO4	(MSC313_GPIO_SR + 2)
+#define MSC313_GPIO_SR_IO5	(MSC313_GPIO_SR + 3)
+#define MSC313_GPIO_SR_IO6	(MSC313_GPIO_SR + 4)
+#define MSC313_GPIO_SR_IO7	(MSC313_GPIO_SR + 5)
+#define MSC313_GPIO_SR_IO8	(MSC313_GPIO_SR + 6)
+#define MSC313_GPIO_SR_IO9	(MSC313_GPIO_SR + 7)
+#define MSC313_GPIO_SR_IO10	(MSC313_GPIO_SR + 8)
+#define MSC313_GPIO_SR_IO11	(MSC313_GPIO_SR + 9)
+#define MSC313_GPIO_SR_IO12	(MSC313_GPIO_SR + 10)
+#define MSC313_GPIO_SR_IO13	(MSC313_GPIO_SR + 11)
+#define MSC313_GPIO_SR_IO14	(MSC313_GPIO_SR + 12)
+#define MSC313_GPIO_SR_IO15	(MSC313_GPIO_SR + 13)
+#define MSC313_GPIO_SR_IO16	(MSC313_GPIO_SR + 14)
+#define MSC313_GPIO_SR_IO17	(MSC313_GPIO_SR + 15)
+
+#define MSC313_GPIO_SD		(MSC313_GPIO_SR_IO17 + 1)
+#define MSC313_GPIO_SD_CLK	(MSC313_GPIO_SD + 0)
+#define MSC313_GPIO_SD_CMD	(MSC313_GPIO_SD + 1)
+#define MSC313_GPIO_SD_D0	(MSC313_GPIO_SD + 2)
+#define MSC313_GPIO_SD_D1	(MSC313_GPIO_SD + 3)
+#define MSC313_GPIO_SD_D2	(MSC313_GPIO_SD + 4)
+#define MSC313_GPIO_SD_D3	(MSC313_GPIO_SD + 5)
+
+#define MSC313_GPIO_I2C1	(MSC313_GPIO_SD_D3 + 1)
+#define MSC313_GPIO_I2C1_SCL	(MSC313_GPIO_I2C1 + 0)
+#define MSC313_GPIO_I2C1_SDA	(MSC313_GPIO_I2C1 + 1)
+
+#define MSC313_GPIO_SPI0	(MSC313_GPIO_I2C1_SDA + 1)
+#define MSC313_GPIO_SPI0_CZ	(MSC313_GPIO_SPI0 + 0)
+#define MSC313_GPIO_SPI0_CK	(MSC313_GPIO_SPI0 + 1)
+#define MSC313_GPIO_SPI0_DI	(MSC313_GPIO_SPI0 + 2)
+#define MSC313_GPIO_SPI0_DO	(MSC313_GPIO_SPI0 + 3)
+
+#endif /* _DT_BINDINGS_MSC313_GPIO_H */
-- 
2.28.0

[PATCH v2 2/5] dt-bindings: gpio: Binding for MStar MSC313 GPIO controller

[Daniel Palmer]

Add a binding description for the MStar/SigmaStar GPIO controller
found in the MSC313 and later ARMv7 SoCs.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 .../bindings/gpio/mstar,msc313-gpio.yaml      | 61 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml b/Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
new file mode 100644
index 000000000000..8c69153ac27e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/mstar,msc313-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MStar/SigmaStar GPIO controller
+
+maintainers:
+  - Daniel Palmer <daniel@thingy.jp>
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    const: mstar,msc313-gpio
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  gpio-ranges-group-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+  - interrupt-controller
+  - "#interrupt-cells"
+
+examples:
+  - |
+    #include <dt-bindings/gpio/msc313-gpio.h>
+
+    gpio: gpio@207800 {
+      compatible = "mstar,msc313e-gpio";
+      #gpio-cells = <2>;
+      reg = <0x207800 0x200>;
+      gpio-controller;
+      gpio-ranges = <&pinctrl 0 36 22>,
+                    <&pinctrl 22 63 4>,
+                    <&pinctrl 26 68 6>;
+      #interrupt-cells = <2>;
+      interrupt-controller;
+      interrupt-parent = <&intc_fiq>;
+      status = "okay";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index a188fae8c04e..102aedca81dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2130,6 +2130,7 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 W:	http://linux-chenxing.org/
 F:	Documentation/devicetree/bindings/arm/mstar/*
+F:	Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
 F:	arch/arm/boot/dts/mstar-*
 F:	arch/arm/mach-mstar/
 F:	include/dt-bindings/gpio/msc313-gpio.h
-- 
2.28.0

[PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Daniel Palmer]

This adds a driver that supports the GPIO block found in
MStar/SigmaStar ARMv7 SoCs.

The controller seems to support 128 lines but where they
are wired up differs between chips and no currently known
chip uses anywhere near 128 lines so there needs to be some
per-chip data to collect together what lines actually have
physical pins attached and map the right names to them.

The core peripherals seem to use the same lines on the
currently known chips but the lines used for the sensor
interface, lcd controller etc pins seem to be totally
different between the infinity and mercury chips

The code tries to collect all of the re-usable names,
offsets etc together so that it's easy to build the extra
per-chip data for other chips in the future.

So far this only supports the MSC313 and MSC313E chips.

Support for the SSC8336N (mercury5) is trivial to add once
all of the lines have been mapped out.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 MAINTAINERS                |   1 +
 drivers/gpio/Kconfig       |   9 +
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-msc313.c | 406 +++++++++++++++++++++++++++++++++++++
 4 files changed, 417 insertions(+)
 create mode 100644 drivers/gpio/gpio-msc313.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 102aedca81dc..86b16452c505 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2133,6 +2133,7 @@ F:	Documentation/devicetree/bindings/arm/mstar/*
 F:	Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
 F:	arch/arm/boot/dts/mstar-*
 F:	arch/arm/mach-mstar/
+F:	drivers/gpio/gpio-msc313.c
 F:	include/dt-bindings/gpio/msc313-gpio.h
 
 ARM/NEC MOBILEPRO 900/c MACHINE SUPPORT
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5d4de5cd6759..64160cc0a477 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -737,6 +737,15 @@ config GPIO_AMD_FCH
 	  Note: This driver doesn't registers itself automatically, as it
 	  needs to be provided with platform specific configuration.
 	  (See eg. CONFIG_PCENGINES_APU2.)
+
+config GPIO_MSC313
+	bool "MStar MSC313 GPIO support"
+	default y if ARCH_MSTARV7
+	depends on ARCH_MSTARV7
+	select GPIOLIB_IRQCHIP
+	help
+	  Say Y here to support GPIO on MStar MSC313 and later SoCs.
+
 endmenu
 
 menu "Port-mapped I/O GPIO drivers"
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 09dada80ac34..b6c116a7c785 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_GPIO_MOCKUP)		+= gpio-mockup.o
 obj-$(CONFIG_GPIO_MOXTET)		+= gpio-moxtet.o
 obj-$(CONFIG_GPIO_MPC5200)		+= gpio-mpc5200.o
 obj-$(CONFIG_GPIO_MPC8XXX)		+= gpio-mpc8xxx.o
+obj-$(CONFIG_GPIO_MSC313)		+= gpio-msc313.o
 obj-$(CONFIG_GPIO_MSIC)			+= gpio-msic.o
 obj-$(CONFIG_GPIO_MT7621)		+= gpio-mt7621.o
 obj-$(CONFIG_GPIO_MVEBU)		+= gpio-mvebu.o
diff --git a/drivers/gpio/gpio-msc313.c b/drivers/gpio/gpio-msc313.c
new file mode 100644
index 000000000000..2751ee520b86
--- /dev/null
+++ b/drivers/gpio/gpio-msc313.c
@@ -0,0 +1,406 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Daniel Palmer<daniel@thingy.jp>
+ */
+
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/gpio/driver.h>
+
+#include <dt-bindings/gpio/msc313-gpio.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define DRIVER_NAME "gpio-msc313"
+
+#define MSC313_GPIO_IN  BIT(0)
+#define MSC313_GPIO_OUT BIT(4)
+#define MSC313_GPIO_OEN BIT(5)
+
+/* These bits need to be saved to correctly restore the
+ * gpio state when resuming from suspend to memory.
+ */
+#define MSC313_GPIO_BITSTOSAVE (MSC313_GPIO_OUT | MSC313_GPIO_OEN)
+
+#define FUART_NAMES			\
+	MSC313_PINNAME_FUART_RX,	\
+	MSC313_PINNAME_FUART_TX,	\
+	MSC313_PINNAME_FUART_CTS,	\
+	MSC313_PINNAME_FUART_RTS
+
+#define OFF_FUART_RX	0x50
+#define OFF_FUART_TX	0x54
+#define OFF_FUART_CTS	0x58
+#define OFF_FUART_RTS	0x5c
+
+#define FUART_OFFSETS	\
+	OFF_FUART_RX,	\
+	OFF_FUART_TX,	\
+	OFF_FUART_CTS,	\
+	OFF_FUART_RTS
+
+#define SR_NAMES		\
+	MSC313_PINNAME_SR_IO2,	\
+	MSC313_PINNAME_SR_IO3,	\
+	MSC313_PINNAME_SR_IO4,	\
+	MSC313_PINNAME_SR_IO5,	\
+	MSC313_PINNAME_SR_IO6,	\
+	MSC313_PINNAME_SR_IO7,	\
+	MSC313_PINNAME_SR_IO8,	\
+	MSC313_PINNAME_SR_IO9,	\
+	MSC313_PINNAME_SR_IO10,	\
+	MSC313_PINNAME_SR_IO11,	\
+	MSC313_PINNAME_SR_IO12,	\
+	MSC313_PINNAME_SR_IO13,	\
+	MSC313_PINNAME_SR_IO14,	\
+	MSC313_PINNAME_SR_IO15,	\
+	MSC313_PINNAME_SR_IO16,	\
+	MSC313_PINNAME_SR_IO17
+
+#define OFF_SR_IO2	0x88
+#define OFF_SR_IO3	0x8c
+#define OFF_SR_IO4	0x90
+#define OFF_SR_IO5	0x94
+#define OFF_SR_IO6	0x98
+#define OFF_SR_IO7	0x9c
+#define OFF_SR_IO8	0xa0
+#define OFF_SR_IO9	0xa4
+#define OFF_SR_IO10	0xa8
+#define OFF_SR_IO11	0xac
+#define OFF_SR_IO12	0xb0
+#define OFF_SR_IO13	0xb4
+#define OFF_SR_IO14	0xb8
+#define OFF_SR_IO15	0xbc
+#define OFF_SR_IO16	0xc0
+#define OFF_SR_IO17	0xc4
+
+#define SR_OFFSETS	\
+	OFF_SR_IO2,	\
+	OFF_SR_IO3,	\
+	OFF_SR_IO4,	\
+	OFF_SR_IO5,	\
+	OFF_SR_IO6,	\
+	OFF_SR_IO7,	\
+	OFF_SR_IO8,	\
+	OFF_SR_IO9,	\
+	OFF_SR_IO10,	\
+	OFF_SR_IO11,	\
+	OFF_SR_IO12,	\
+	OFF_SR_IO13,	\
+	OFF_SR_IO14,	\
+	OFF_SR_IO15,	\
+	OFF_SR_IO16,	\
+	OFF_SR_IO17
+
+#define SD_NAMES		\
+	MSC313_PINNAME_SD_CLK,	\
+	MSC313_PINNAME_SD_CMD,	\
+	MSC313_PINNAME_SD_D0,	\
+	MSC313_PINNAME_SD_D1,	\
+	MSC313_PINNAME_SD_D2,	\
+	MSC313_PINNAME_SD_D3
+
+#define OFF_SD_CLK	0x140
+#define OFF_SD_CMD	0x144
+#define OFF_SD_D0	0x148
+#define OFF_SD_D1	0x14c
+#define OFF_SD_D2	0x150
+#define OFF_SD_D3	0x154
+
+#define SD_OFFSETS	\
+	OFF_SD_CLK,	\
+	OFF_SD_CMD,	\
+	OFF_SD_D0,	\
+	OFF_SD_D1,	\
+	OFF_SD_D2,	\
+	OFF_SD_D3
+
+#define I2C1_NAMES			\
+	MSC313_PINNAME_I2C1_SCL,	\
+	MSC313_PINNAME_I2C1_SCA
+
+#define OFF_I2C1_SCL	0x188
+#define OFF_I2C1_SCA	0x18c
+
+#define I2C1_OFFSETS	\
+	OFF_I2C1_SCL,	\
+	OFF_I2C1_SCA
+
+#define SPI0_NAMES		\
+	MSC313_PINNAME_SPI0_CZ,	\
+	MSC313_PINNAME_SPI0_CK,	\
+	MSC313_PINNAME_SPI0_DI,	\
+	MSC313_PINNAME_SPI0_DO
+
+#define OFF_SPI0_CZ	0x1c0
+#define OFF_SPI0_CK	0x1c4
+#define OFF_SPI0_DI	0x1c8
+#define OFF_SPI0_DO	0x1cc
+
+#define SPI0_OFFSETS	\
+	OFF_SPI0_CZ,	\
+	OFF_SPI0_CK,	\
+	OFF_SPI0_DI,	\
+	OFF_SPI0_DO
+
+struct msc313_gpio_data {
+	const char * const *names;
+	const unsigned int *offsets;
+	const unsigned int num;
+};
+
+#define MSC313_GPIO_CHIPDATA(_chip) \
+static const struct msc313_gpio_data _chip##_data = { \
+	.names = _chip##_names, \
+	.offsets = _chip##_offsets, \
+	.num = ARRAY_SIZE(_chip##_offsets), \
+}
+
+#ifdef CONFIG_MACH_INFINITY
+static const char * const msc313_names[] = {
+	FUART_NAMES,
+	SR_NAMES,
+	SD_NAMES,
+	I2C1_NAMES,
+	SPI0_NAMES,
+};
+
+static const unsigned int msc313_offsets[] = {
+	FUART_OFFSETS,
+	SR_OFFSETS,
+	SD_OFFSETS,
+	I2C1_OFFSETS,
+	SPI0_OFFSETS,
+};
+
+MSC313_GPIO_CHIPDATA(msc313);
+#endif
+
+struct msc313_gpio {
+	void __iomem *base;
+	const struct msc313_gpio_data *gpio_data;
+	u8 *saved;
+};
+
+static void msc313_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct msc313_gpio *gpio = gpiochip_get_data(chip);
+	u8 gpioreg = readb_relaxed(gpio->base + gpio->gpio_data->offsets[offset]);
+
+	if (value)
+		gpioreg |= MSC313_GPIO_OUT;
+	else
+		gpioreg &= ~MSC313_GPIO_OUT;
+
+	writeb_relaxed(gpioreg, gpio->base + gpio->gpio_data->offsets[offset]);
+}
+
+static int msc313_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct msc313_gpio *gpio = gpiochip_get_data(chip);
+
+	return readb_relaxed(gpio->base + gpio->gpio_data->offsets[offset])
+			& MSC313_GPIO_IN;
+}
+
+static int msc313_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct msc313_gpio *gpio = gpiochip_get_data(chip);
+	u8 gpioreg = readb_relaxed(gpio->base + gpio->gpio_data->offsets[offset]);
+
+	gpioreg |= MSC313_GPIO_OEN;
+	writeb_relaxed(gpioreg, gpio->base + gpio->gpio_data->offsets[offset]);
+
+	return 0;
+}
+
+static int msc313_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct msc313_gpio *gpio = gpiochip_get_data(chip);
+	u8 gpioreg = readb_relaxed(gpio->base + gpio->gpio_data->offsets[offset]);
+
+	gpioreg &= ~MSC313_GPIO_OEN;
+	if (value)
+		gpioreg |= MSC313_GPIO_OUT;
+	else
+		gpioreg &= ~MSC313_GPIO_OUT;
+	writeb_relaxed(gpioreg, gpio->base + gpio->gpio_data->offsets[offset]);
+
+	return 0;
+}
+
+/*
+ * The interrupt handling happens in the parent interrupt controller,
+ * we don't do anything here.
+ */
+static struct irq_chip msc313_gpio_irqchip = {
+	.name = "GPIO",
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_set_type = irq_chip_set_type_parent,
+};
+
+/* The parent interrupt controller needs the GIC interrupt type set to GIC_SPI
+ * so we need to provide the fwspec. Essentially gpiochip_populate_parent_fwspec_twocell
+ * that puts GIC_SPI into the first cell.
+ */
+static void *msc313_gpio_populate_parent_fwspec(struct gpio_chip *gc,
+					     unsigned int parent_hwirq,
+					     unsigned int parent_type)
+{
+	struct irq_fwspec *fwspec;
+
+	fwspec = kmalloc(sizeof(*fwspec), GFP_KERNEL);
+	if (!fwspec)
+		return NULL;
+
+	fwspec->fwnode = gc->irq.parent_domain->fwnode;
+	fwspec->param_count = 3;
+	fwspec->param[0] = GIC_SPI;
+	fwspec->param[1] = parent_hwirq;
+	fwspec->param[2] = parent_type;
+
+	return fwspec;
+}
+
+static int msc313e_gpio_child_to_parent_hwirq(struct gpio_chip *chip,
+					     unsigned int child,
+					     unsigned int child_type,
+					     unsigned int *parent,
+					     unsigned int *parent_type)
+{
+	struct msc313_gpio *priv = gpiochip_get_data(chip);
+	unsigned int offset = priv->gpio_data->offsets[child];
+	int ret = -EINVAL;
+
+	/* only the spi0 pins have interrupts on the parent
+	 * on all of the known chips and so far they are all
+	 * mapped to the same place
+	 */
+	if (offset >= OFF_SPI0_CZ && offset <= OFF_SPI0_DO) {
+		*parent_type = child_type;
+		*parent = ((offset - OFF_SPI0_CZ) >> 2) + 28;
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static int msc313_gpio_probe(struct platform_device *pdev)
+{
+	const struct msc313_gpio_data *match_data;
+	struct msc313_gpio *gpio;
+	struct gpio_chip *gpiochip;
+	struct gpio_irq_chip *gpioirqchip;
+	struct resource *res;
+	struct irq_domain *parent_domain;
+	struct device_node *parent_node;
+	int ret;
+
+	match_data = of_device_get_match_data(&pdev->dev);
+	if (!match_data)
+		return -EINVAL;
+
+	parent_node = of_irq_find_parent(pdev->dev.of_node);
+	if (!parent_node)
+		return -ENODEV;
+
+	parent_domain = irq_find_host(parent_node);
+	if (!parent_domain)
+		return -ENODEV;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->gpio_data = match_data;
+
+	gpio->saved = devm_kzalloc(&pdev->dev, gpio->gpio_data->num * sizeof(*gpio->saved), GFP_KERNEL);
+	if (!gpio->saved)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, gpio);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	gpio->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio->base))
+		return PTR_ERR(gpio->base);
+
+	gpiochip = devm_kzalloc(&pdev->dev, sizeof(*gpiochip), GFP_KERNEL);
+	if (!gpiochip)
+		return -ENOMEM;
+
+	gpiochip->label = DRIVER_NAME;
+	gpiochip->parent = &pdev->dev;
+	gpiochip->request = gpiochip_generic_request;
+	gpiochip->free = gpiochip_generic_free;
+	gpiochip->direction_input = msc313_gpio_direction_input;
+	gpiochip->direction_output = msc313_gpio_direction_output;
+	gpiochip->get = msc313_gpio_get;
+	gpiochip->set = msc313_gpio_set;
+	gpiochip->base = -1;
+	gpiochip->ngpio = gpio->gpio_data->num;
+	gpiochip->names = gpio->gpio_data->names;
+
+	gpioirqchip = &gpiochip->irq;
+	gpioirqchip->chip = &msc313_gpio_irqchip;
+	gpioirqchip->fwnode = of_node_to_fwnode(pdev->dev.of_node);
+	gpioirqchip->parent_domain = parent_domain;
+	gpioirqchip->child_to_parent_hwirq = msc313e_gpio_child_to_parent_hwirq;
+	gpioirqchip->populate_parent_alloc_arg = msc313_gpio_populate_parent_fwspec;
+	gpioirqchip->handler = handle_bad_irq;
+	gpioirqchip->default_type = IRQ_TYPE_NONE;
+
+	ret = gpiochip_add_data(gpiochip, gpio);
+	return ret;
+}
+
+static const struct of_device_id msc313_gpio_of_match[] = {
+#ifdef CONFIG_MACH_INFINITY
+	{
+		.compatible = "mstar,msc313-gpio",
+		.data = &msc313_data,
+	},
+#endif
+	{ }
+};
+
+/* The GPIO controller loses the state of the registers when the
+ * SoC goes into suspend to memory mode so we need to save some
+ * of the register bits before suspending and put it back when resuming
+ */
+
+static int __maybe_unused msc313_gpio_suspend(struct device *dev)
+{
+	struct msc313_gpio *gpio = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < gpio->gpio_data->num; i++)
+		gpio->saved[i] = readb_relaxed(gpio->base + gpio->gpio_data->offsets[i]) & MSC313_GPIO_BITSTOSAVE;
+
+	return 0;
+}
+
+static int __maybe_unused msc313_gpio_resume(struct device *dev)
+{
+	struct msc313_gpio *gpio = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < gpio->gpio_data->num; i++)
+		writeb_relaxed(gpio->saved[i], gpio->base + gpio->gpio_data->offsets[i]);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(msc313_gpio_ops, msc313_gpio_suspend, msc313_gpio_resume);
+
+static struct platform_driver msc313_gpio_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = msc313_gpio_of_match,
+		.pm = &msc313_gpio_ops,
+	},
+	.probe = msc313_gpio_probe,
+};
+
+builtin_platform_driver(msc313_gpio_driver);
-- 
2.28.0

[PATCH v2 4/5] ARM: mstar: Add gpio controller to MStar base dtsi

[Daniel Palmer]

The GPIO controller is at the same address in all of the
currently known chips so create a node for it in the base
dtsi.

Some extra properties are needed to actually use it so
disable it by default.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 arch/arm/boot/dts/mstar-v7.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-v7.dtsi b/arch/arm/boot/dts/mstar-v7.dtsi
index f07880561e11..81369bc07f78 100644
--- a/arch/arm/boot/dts/mstar-v7.dtsi
+++ b/arch/arm/boot/dts/mstar-v7.dtsi
@@ -109,6 +109,16 @@ l3bridge: l3bridge@204400 {
 				reg = <0x204400 0x200>;
 			};
 
+			gpio: gpio@207800 {
+				#gpio-cells = <2>;
+				reg = <0x207800 0x200>;
+				gpio-controller;
+				#interrupt-cells = <2>;
+				interrupt-controller;
+				interrupt-parent = <&intc_fiq>;
+				status = "disabled";
+			};
+
 			pm_uart: uart@221000 {
 				compatible = "ns16550a";
 				reg = <0x221000 0x100>;
-- 
2.28.0

[PATCH v2 5/5] ARM: mstar: Fill in GPIO controller properties for infinity

[Daniel Palmer]

Fill in the properties needed to use the GPIO controller
in the infinity and infinity3 chips.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 arch/arm/boot/dts/mstar-infinity.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-infinity.dtsi b/arch/arm/boot/dts/mstar-infinity.dtsi
index cd911adef014..0bee517797f4 100644
--- a/arch/arm/boot/dts/mstar-infinity.dtsi
+++ b/arch/arm/boot/dts/mstar-infinity.dtsi
@@ -6,6 +6,13 @@
 
 #include "mstar-v7.dtsi"
 
+#include <dt-bindings/gpio/msc313-gpio.h>
+
 &imi {
 	reg = <0xa0000000 0x16000>;
 };
+
+&gpio {
+	compatible = "mstar,msc313-gpio";
+	status = "okay";
+};
-- 
2.28.0

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Andy Shevchenko]

On Mon, Oct 19, 2020 at 5:11 PM Daniel Palmer <daniel@0x0f.com> wrote:
>
> This adds a driver that supports the GPIO block found in
> MStar/SigmaStar ARMv7 SoCs.
>
> The controller seems to support 128 lines but where they
> are wired up differs between chips and no currently known
> chip uses anywhere near 128 lines so there needs to be some
> per-chip data to collect together what lines actually have
> physical pins attached and map the right names to them.
>
> The core peripherals seem to use the same lines on the
> currently known chips but the lines used for the sensor
> interface, lcd controller etc pins seem to be totally
> different between the infinity and mercury chips
>
> The code tries to collect all of the re-usable names,
> offsets etc together so that it's easy to build the extra
> per-chip data for other chips in the future.
>
> So far this only supports the MSC313 and MSC313E chips.
>
> Support for the SSC8336N (mercury5) is trivial to add once
> all of the lines have been mapped out.

...

> +config GPIO_MSC313
> +       bool "MStar MSC313 GPIO support"

Why boolean?

> +       default y if ARCH_MSTARV7

Simply
       default ARCH_MSTARV7
should work as well.

Are you planning to extend this to other boards?

> +       depends on ARCH_MSTARV7
> +       select GPIOLIB_IRQCHIP
> +       help
> +         Say Y here to support GPIO on MStar MSC313 and later SoCs.

Please, be more specific. Also it's recommended to have a module name
to be included (but let's understand first why it's not a module)

...

> +/*
> + * Copyright (C) 2020 Daniel Palmer<daniel@thingy.jp>
> + */

One line.

...

> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/gpio/driver.h>

I believe this should be reworked.
For example, it misses mod_devicetable.h, bits.h, io.h, types.h, etc, but has

...

> +/* These bits need to be saved to correctly restore the
> + * gpio state when resuming from suspend to memory.
> + */

/*
 * For this subsystem the comment style for multi-line
 * like this.
 */

...

> +#ifdef CONFIG_MACH_INFINITY

Does it make any sense?

> +#endif

...

> +       return readb_relaxed(gpio->base + gpio->gpio_data->offsets[offset])
> +                       & MSC313_GPIO_IN;

Usual pattern is to leave operators on the previous line.

...

> +static struct irq_chip msc313_gpio_irqchip = {
> +       .name = "GPIO",

Is this name good enough?

> +       .irq_eoi = irq_chip_eoi_parent,
> +       .irq_mask = irq_chip_mask_parent,
> +       .irq_unmask = irq_chip_unmask_parent,
> +       .irq_set_type = irq_chip_set_type_parent,
> +};

...

> +static int msc313e_gpio_child_to_parent_hwirq(struct gpio_chip *chip,
> +                                            unsigned int child,
> +                                            unsigned int child_type,
> +                                            unsigned int *parent,
> +                                            unsigned int *parent_type)
> +{
> +       struct msc313_gpio *priv = gpiochip_get_data(chip);
> +       unsigned int offset = priv->gpio_data->offsets[child];
> +       int ret = -EINVAL;
> +
> +       /* only the spi0 pins have interrupts on the parent
> +        * on all of the known chips and so far they are all
> +        * mapped to the same place
> +        */

Comment style!

> +       if (offset >= OFF_SPI0_CZ && offset <= OFF_SPI0_DO) {
> +               *parent_type = child_type;
> +               *parent = ((offset - OFF_SPI0_CZ) >> 2) + 28;
> +               ret = 0;
> +       }
> +
> +       return ret;

Oh, can, for a starter, we use a regular (not-so-twisted) pattern

  if (...)
    return -EINVAL;
  ...
  return 0;

?

> +}

...

> +       gpio->saved = devm_kzalloc(&pdev->dev, gpio->gpio_data->num * sizeof(*gpio->saved), GFP_KERNEL);

devm_kcalloc()

> +       if (!gpio->saved)
> +               return -ENOMEM;

...

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       gpio->base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()

> +       if (IS_ERR(gpio->base))
> +               return PTR_ERR(gpio->base);

...

> +       gpiochip->label = DRIVER_NAME;

Not good. When you use user space how do you distinguish if more than
one chip appears in the system?

...

> +       ret = gpiochip_add_data(gpiochip, gpio);
> +       return ret;

return ...(...);

Why not devm_gpiochip_add_data() ?

...

> +static const struct of_device_id msc313_gpio_of_match[] = {

> +#ifdef CONFIG_MACH_INFINITY

To me this makes no sense.

> +       {
> +               .compatible = "mstar,msc313-gpio",
> +               .data = &msc313_data,
> +       },
> +#endif
> +       { }
> +};

...

> +/* The GPIO controller loses the state of the registers when the
> + * SoC goes into suspend to memory mode so we need to save some
> + * of the register bits before suspending and put it back when resuming
> + */

Comment style!

> +

Redundant blank line.

...

> +static int __maybe_unused msc313_gpio_resume(struct device *dev)
> +{

> +}

> +

Redundant blank line.

> +static SIMPLE_DEV_PM_OPS(msc313_gpio_ops, msc313_gpio_suspend, msc313_gpio_resume);

...

> +static struct platform_driver msc313_gpio_driver = {
> +       .driver = {
> +               .name = DRIVER_NAME,
> +               .of_match_table = msc313_gpio_of_match,
> +               .pm = &msc313_gpio_ops,

You still allow to unbind.

> +       },
> +       .probe = msc313_gpio_probe,
> +};

> +

Redundant blank line.

> +builtin_platform_driver(msc313_gpio_driver);

Why?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Daniel Palmer]

Hi Andy,

On Tue, 20 Oct 2020 at 20:59, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > +config GPIO_MSC313
> > +       bool "MStar MSC313 GPIO support"
>
> Why boolean?

Because it's a built in driver. I could change it to tristate/a module
but I didn't think it needed to be one.
The machines this is used on generally only have 64 or 128MB of RAM so
the kernel is usually built without modules and only the totally
required stuff built in.

> > +       default y if ARCH_MSTARV7
>
> Simply
>        default ARCH_MSTARV7
> should work as well.
>
> Are you planning to extend this to other boards?

I think I copy/pasted the block above there. I'll fix this up.

As for other boards. I think this GPIO controller is only present in
MStar's SoCs and some MediaTek SoCs that inherited parts from MStar
after MediaTek bought them. Like the MStar interrupt controller is
present in some MediaTek TV chips.

>
> > +       depends on ARCH_MSTARV7
> > +       select GPIOLIB_IRQCHIP
> > +       help
> > +         Say Y here to support GPIO on MStar MSC313 and later SoCs.
>
> Please, be more specific. Also it's recommended to have a module name
> to be included (but let's understand first why it's not a module)

Ok. I'll rework that. As for it not being a module. I can make it
possible to build it
as a module. I just didn't really think it needed to be one.

> > +#include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/gpio/driver.h>
>
> I believe this should be reworked.
> For example, it misses mod_devicetable.h, bits.h, io.h, types.h, etc, but has

I'll look at this again.

> > +/* These bits need to be saved to correctly restore the
> > + * gpio state when resuming from suspend to memory.
> > + */
>
> /*
>  * For this subsystem the comment style for multi-line
>  * like this.
>  */

Sorry, I'll fix these up.

> > +#ifdef CONFIG_MACH_INFINITY
>
> Does it make any sense?
>
> > +#endif

It doesn't make a lot of sense right now but it makes a bit more sense
when the support for the mercury chips is added. They have their own
set of offsets
for the used pins. I cut that out of this series because I haven't
fully reverse engineered all of the pins for those chips yet so the
support for them has a lot of guesses and notes in the code.

Anyhow, with only 64MB of RAM I thought it made sense to not compile
in the mercury tables when support for those machines isn't enabled.
I'll drop the if/defs for the next version.

> > +static struct irq_chip msc313_gpio_irqchip = {
> > +       .name = "GPIO",
>
> Is this name good enough?
>

There is only one GPIO block in the chips this is for so I think it's
unique at least.

> > +       gpiochip->label = DRIVER_NAME;
>
> Not good. When you use user space how do you distinguish if more than
> one chip appears in the system?

So far there is only ever one of these GPIO controller for the whole system.
There is another GPIO block in the system but it uses another driver
as the register layout is totally different.

Thank you for all of the comments. Sorry about the multiple style issues.
I thought checkpatch had my back on that.

Thanks,

Daniel

Re: [PATCH v2 1/5] dt-bindings: gpio: Add a binding header for the MSC313 GPIO driver

[Rob Herring]

On Mon, Oct 19, 2020 at 11:10:04PM +0900, Daniel Palmer wrote:
> Header adds defines for the gpio number of each pin
> from the driver view. The gpio block seems to support 128 lines
> but what line is mapped to a physical pin depends on the chip.
> The driver itself uses the index of a pin's offset in an array
> of the possible offsets for a chip as the gpio number.
> 
> The defines remove the need to work out that index to consume
> a pin in the device tree.

I'd expect the DT to have 0-127 numbering... If you need to map that to 
another number, then an array property in DT could handle that.

> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> ---
>  MAINTAINERS                            |  1 +
>  include/dt-bindings/gpio/msc313-gpio.h | 95 ++++++++++++++++++++++++++
>  2 files changed, 96 insertions(+)
>  create mode 100644 include/dt-bindings/gpio/msc313-gpio.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3f345f36c22c..a188fae8c04e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2132,6 +2132,7 @@ W:	http://linux-chenxing.org/
>  F:	Documentation/devicetree/bindings/arm/mstar/*
>  F:	arch/arm/boot/dts/mstar-*
>  F:	arch/arm/mach-mstar/
> +F:	include/dt-bindings/gpio/msc313-gpio.h
>  
>  ARM/NEC MOBILEPRO 900/c MACHINE SUPPORT
>  M:	Michael Petchkovsky <mkpetch@internode.on.net>
> diff --git a/include/dt-bindings/gpio/msc313-gpio.h b/include/dt-bindings/gpio/msc313-gpio.h
> new file mode 100644
> index 000000000000..9b8cd6ffb7c4
> --- /dev/null
> +++ b/include/dt-bindings/gpio/msc313-gpio.h
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +/*
> + * GPIO definitions for MStar/SigmaStar MSC313 and later SoCs
> + *
> + * Copyright (C) 2020 Daniel Palmer <daniel@thingy.jp>
> + */
> +
> +#ifndef _DT_BINDINGS_MSC313_GPIO_H
> +#define _DT_BINDINGS_MSC313_GPIO_H
> +
> +/* pin names for fuart, same for all SoCs so far */
> +#define MSC313_PINNAME_FUART_RX		"fuart_rx"
> +#define MSC313_PINNAME_FUART_TX		"fuart_tx"
> +#define MSC313_PINNAME_FUART_CTS	"fuart_cts"
> +#define MSC313_PINNAME_FUART_RTS	"fuart_rts"

Defines for strings serves no purpose.

> +
> +/* pin names for sr, mercury5 is different */
> +#define MSC313_PINNAME_SR_IO2		"sr_io2"
> +#define MSC313_PINNAME_SR_IO3		"sr_io3"
> +#define MSC313_PINNAME_SR_IO4		"sr_io4"
> +#define MSC313_PINNAME_SR_IO5		"sr_io5"
> +#define MSC313_PINNAME_SR_IO6		"sr_io6"
> +#define MSC313_PINNAME_SR_IO7		"sr_io7"
> +#define MSC313_PINNAME_SR_IO8		"sr_io8"
> +#define MSC313_PINNAME_SR_IO9		"sr_io9"
> +#define MSC313_PINNAME_SR_IO10		"sr_io10"
> +#define MSC313_PINNAME_SR_IO11		"sr_io11"
> +#define MSC313_PINNAME_SR_IO12		"sr_io12"
> +#define MSC313_PINNAME_SR_IO13		"sr_io13"
> +#define MSC313_PINNAME_SR_IO14		"sr_io14"
> +#define MSC313_PINNAME_SR_IO15		"sr_io15"
> +#define MSC313_PINNAME_SR_IO16		"sr_io16"
> +#define MSC313_PINNAME_SR_IO17		"sr_io17"
> +
> +/* pin names for sd, same for all SoCs so far */
> +#define MSC313_PINNAME_SD_CLK		"sd_clk"
> +#define MSC313_PINNAME_SD_CMD		"sd_cmd"
> +#define MSC313_PINNAME_SD_D0		"sd_d0"
> +#define MSC313_PINNAME_SD_D1		"sd_d1"
> +#define MSC313_PINNAME_SD_D2		"sd_d2"
> +#define MSC313_PINNAME_SD_D3		"sd_d3"
> +
> +/* pin names for i2c1, same for all SoCs so for */
> +#define MSC313_PINNAME_I2C1_SCL		"i2c1_scl"
> +#define MSC313_PINNAME_I2C1_SCA		"i2c1_sda"
> +
> +/* pin names for spi0, same for all SoCs so far */
> +#define MSC313_PINNAME_SPI0_CZ		"spi0_cz"
> +#define MSC313_PINNAME_SPI0_CK		"spi0_ck"
> +#define MSC313_PINNAME_SPI0_DI		"spi0_di"
> +#define MSC313_PINNAME_SPI0_DO		"spi0_do"
> +
> +#define MSC313_GPIO_FUART	0
> +#define MSC313_GPIO_FUART_RX	(MSC313_GPIO_FUART + 0)
> +#define MSC313_GPIO_FUART_TX	(MSC313_GPIO_FUART + 1)
> +#define MSC313_GPIO_FUART_CTS	(MSC313_GPIO_FUART + 2)
> +#define MSC313_GPIO_FUART_RTS	(MSC313_GPIO_FUART + 3)

We don't normally have defines for GPIO numbers either. 

> +
> +#define MSC313_GPIO_SR		(MSC313_GPIO_FUART_RTS + 1)
> +#define MSC313_GPIO_SR_IO2	(MSC313_GPIO_SR + 0)
> +#define MSC313_GPIO_SR_IO3	(MSC313_GPIO_SR + 1)
> +#define MSC313_GPIO_SR_IO4	(MSC313_GPIO_SR + 2)
> +#define MSC313_GPIO_SR_IO5	(MSC313_GPIO_SR + 3)
> +#define MSC313_GPIO_SR_IO6	(MSC313_GPIO_SR + 4)
> +#define MSC313_GPIO_SR_IO7	(MSC313_GPIO_SR + 5)
> +#define MSC313_GPIO_SR_IO8	(MSC313_GPIO_SR + 6)
> +#define MSC313_GPIO_SR_IO9	(MSC313_GPIO_SR + 7)
> +#define MSC313_GPIO_SR_IO10	(MSC313_GPIO_SR + 8)
> +#define MSC313_GPIO_SR_IO11	(MSC313_GPIO_SR + 9)
> +#define MSC313_GPIO_SR_IO12	(MSC313_GPIO_SR + 10)
> +#define MSC313_GPIO_SR_IO13	(MSC313_GPIO_SR + 11)
> +#define MSC313_GPIO_SR_IO14	(MSC313_GPIO_SR + 12)
> +#define MSC313_GPIO_SR_IO15	(MSC313_GPIO_SR + 13)
> +#define MSC313_GPIO_SR_IO16	(MSC313_GPIO_SR + 14)
> +#define MSC313_GPIO_SR_IO17	(MSC313_GPIO_SR + 15)
> +
> +#define MSC313_GPIO_SD		(MSC313_GPIO_SR_IO17 + 1)
> +#define MSC313_GPIO_SD_CLK	(MSC313_GPIO_SD + 0)
> +#define MSC313_GPIO_SD_CMD	(MSC313_GPIO_SD + 1)
> +#define MSC313_GPIO_SD_D0	(MSC313_GPIO_SD + 2)
> +#define MSC313_GPIO_SD_D1	(MSC313_GPIO_SD + 3)
> +#define MSC313_GPIO_SD_D2	(MSC313_GPIO_SD + 4)
> +#define MSC313_GPIO_SD_D3	(MSC313_GPIO_SD + 5)
> +
> +#define MSC313_GPIO_I2C1	(MSC313_GPIO_SD_D3 + 1)
> +#define MSC313_GPIO_I2C1_SCL	(MSC313_GPIO_I2C1 + 0)
> +#define MSC313_GPIO_I2C1_SDA	(MSC313_GPIO_I2C1 + 1)
> +
> +#define MSC313_GPIO_SPI0	(MSC313_GPIO_I2C1_SDA + 1)
> +#define MSC313_GPIO_SPI0_CZ	(MSC313_GPIO_SPI0 + 0)
> +#define MSC313_GPIO_SPI0_CK	(MSC313_GPIO_SPI0 + 1)
> +#define MSC313_GPIO_SPI0_DI	(MSC313_GPIO_SPI0 + 2)
> +#define MSC313_GPIO_SPI0_DO	(MSC313_GPIO_SPI0 + 3)
> +
> +#endif /* _DT_BINDINGS_MSC313_GPIO_H */
> -- 
> 2.28.0
> 

Re: [PATCH v2 2/5] dt-bindings: gpio: Binding for MStar MSC313 GPIO controller

[Rob Herring]

On Mon, Oct 19, 2020 at 11:10:05PM +0900, Daniel Palmer wrote:
> Add a binding description for the MStar/SigmaStar GPIO controller
> found in the MSC313 and later ARMv7 SoCs.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> ---
>  .../bindings/gpio/mstar,msc313-gpio.yaml      | 61 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml b/Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
> new file mode 100644
> index 000000000000..8c69153ac27e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/mstar,msc313-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MStar/SigmaStar GPIO controller
> +
> +maintainers:
> +  - Daniel Palmer <daniel@thingy.jp>
> +
> +properties:
> +  $nodename:
> +    pattern: "^gpio@[0-9a-f]+$"
> +
> +  compatible:
> +    const: mstar,msc313-gpio
> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-ranges: true
> +
> +  gpio-ranges-group-names:
> +    $ref: /schemas/types.yaml#/definitions/string-array

The strings in your header should be defined here.

> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - interrupt-controller
> +  - "#interrupt-cells"

additionalProperties: false

> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/msc313-gpio.h>
> +
> +    gpio: gpio@207800 {
> +      compatible = "mstar,msc313e-gpio";
> +      #gpio-cells = <2>;
> +      reg = <0x207800 0x200>;
> +      gpio-controller;
> +      gpio-ranges = <&pinctrl 0 36 22>,
> +                    <&pinctrl 22 63 4>,
> +                    <&pinctrl 26 68 6>;
> +      #interrupt-cells = <2>;
> +      interrupt-controller;
> +      interrupt-parent = <&intc_fiq>;
> +      status = "okay";

Don't show status in examples.

> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a188fae8c04e..102aedca81dc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2130,6 +2130,7 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>  S:	Maintained
>  W:	http://linux-chenxing.org/
>  F:	Documentation/devicetree/bindings/arm/mstar/*
> +F:	Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
>  F:	arch/arm/boot/dts/mstar-*
>  F:	arch/arm/mach-mstar/
>  F:	include/dt-bindings/gpio/msc313-gpio.h
> -- 
> 2.28.0
> 

Re: [PATCH v2 1/5] dt-bindings: gpio: Add a binding header for the MSC313 GPIO driver

[Daniel Palmer]

Hi Rob,

On Mon, 26 Oct 2020 at 22:46, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Oct 19, 2020 at 11:10:04PM +0900, Daniel Palmer wrote:
> > Header adds defines for the gpio number of each pin
> > from the driver view. The gpio block seems to support 128 lines
> > but what line is mapped to a physical pin depends on the chip.
> > The driver itself uses the index of a pin's offset in an array
> > of the possible offsets for a chip as the gpio number.
> >
> > The defines remove the need to work out that index to consume
> > a pin in the device tree.
>
> I'd expect the DT to have 0-127 numbering... If you need to map that to
> another number, then an array property in DT could handle that.
>

Thank you for the comments on this header and the binding description.

Thinking about this again I'm thinking about having the GPIO numbers
be 0-127 like you say but supplying the valid offsets for that
specific chip and the pad/pin names to make visible to the user via an
array/arrays that contains the pin register offsets and the pin names.
Basically my per-chip table moves out of the driver and into the DT.
Does that sound acceptable? The main thing I want to avoid is
presenting the user with 128 gpios when the actually chip only has <10
of them wired up.

Thanks,

Daniel

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Linus Walleij]

On Tue, Oct 20, 2020 at 1:59 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Oct 19, 2020 at 5:11 PM Daniel Palmer <daniel@0x0f.com> wrote:

> > +config GPIO_MSC313
> > +       bool "MStar MSC313 GPIO support"

> > +       default y if ARCH_MSTARV7

I think it is possible to write:

default ARCH_MSTARV7

For this.

> Why boolean?

I answered this question in some other mail but there is usually somewhat
a good reason for this but let's discuss it a bit.

The following usecase:

- You have a generic distribution such as Android

- The generic distribution does not use initramfs to support basic
  drivers (does Android? Nowadays?)

- So all modules would need to load from actual storage media.

- The storage media is an SD card.

- The SD card reader has a card detect line connected to one
  of the GPIO lines.

- Now we have catch-22 because there is no way to load the
  GPIO driver modules from the storage media because the
  driver is needed to mount the storage media.

I do not know if this "never happens" because every generic
distribution "should" be using initramfs for their drivers. But it
provides a convenient way for users to shoot themselves in the
foot and be frustrated about that their root filesystem is not
mounting.

I do not think this is limited to GPIO card detects but it is a very
immediate example.

What is the consensus here?

Yours,
Linus Walleij

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Linus Walleij]

On Mon, Oct 19, 2020 at 4:10 PM Daniel Palmer <daniel@0x0f.com> wrote:

> This adds a driver that supports the GPIO block found in
> MStar/SigmaStar ARMv7 SoCs.
>
> The controller seems to support 128 lines but where they
> are wired up differs between chips and no currently known
> chip uses anywhere near 128 lines so there needs to be some
> per-chip data to collect together what lines actually have
> physical pins attached and map the right names to them.
>
> The core peripherals seem to use the same lines on the
> currently known chips but the lines used for the sensor
> interface, lcd controller etc pins seem to be totally
> different between the infinity and mercury chips
>
> The code tries to collect all of the re-usable names,
> offsets etc together so that it's easy to build the extra
> per-chip data for other chips in the future.
>
> So far this only supports the MSC313 and MSC313E chips.
>
> Support for the SSC8336N (mercury5) is trivial to add once
> all of the lines have been mapped out.
>
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>

This looks really nice, the generic hierarchical IRQchip does
its job very well here.

I saw you would send another version but this already looks
like merge material.

Certainly any remaining issues can be ironed out in-tree.

> +config GPIO_MSC313
> +       bool "MStar MSC313 GPIO support"
> +       default y if ARCH_MSTARV7
> +       depends on ARCH_MSTARV7
> +       select GPIOLIB_IRQCHIP

select IRQ_DOMAIN_HIERARCHY

since you are dependent on this.

(Else people will soon report problems with randconfig...)

> +/* The parent interrupt controller needs the GIC interrupt type set to GIC_SPI
> + * so we need to provide the fwspec. Essentially gpiochip_populate_parent_fwspec_twocell
> + * that puts GIC_SPI into the first cell.
> + */
> +static void *msc313_gpio_populate_parent_fwspec(struct gpio_chip *gc,
> +                                            unsigned int parent_hwirq,
> +                                            unsigned int parent_type)
> +{
> +       struct irq_fwspec *fwspec;
> +
> +       fwspec = kmalloc(sizeof(*fwspec), GFP_KERNEL);
> +       if (!fwspec)
> +               return NULL;
> +
> +       fwspec->fwnode = gc->irq.parent_domain->fwnode;
> +       fwspec->param_count = 3;
> +       fwspec->param[0] = GIC_SPI;
> +       fwspec->param[1] = parent_hwirq;
> +       fwspec->param[2] = parent_type;
> +
> +       return fwspec;
> +}

Clever. Looping in Marc Z so he can say if this looks allright to him.

> +static int msc313e_gpio_child_to_parent_hwirq(struct gpio_chip *chip,
> +                                            unsigned int child,
> +                                            unsigned int child_type,
> +                                            unsigned int *parent,
> +                                            unsigned int *parent_type)
> +{
> +       struct msc313_gpio *priv = gpiochip_get_data(chip);
> +       unsigned int offset = priv->gpio_data->offsets[child];
> +       int ret = -EINVAL;
> +
> +       /* only the spi0 pins have interrupts on the parent
> +        * on all of the known chips and so far they are all
> +        * mapped to the same place
> +        */
> +       if (offset >= OFF_SPI0_CZ && offset <= OFF_SPI0_DO) {
> +               *parent_type = child_type;
> +               *parent = ((offset - OFF_SPI0_CZ) >> 2) + 28;
> +               ret = 0;
> +       }
> +
> +       return ret;
> +}

Neat!

> +static int msc313_gpio_probe(struct platform_device *pdev)
> +{
> +       const struct msc313_gpio_data *match_data;
> +       struct msc313_gpio *gpio;
> +       struct gpio_chip *gpiochip;
> +       struct gpio_irq_chip *gpioirqchip;
> +       struct resource *res;
> +       struct irq_domain *parent_domain;
> +       struct device_node *parent_node;
> +       int ret;
> +
> +       match_data = of_device_get_match_data(&pdev->dev);

There is a lot of referencing &pdev->dev.

I would add a local variable like this:

struct device *dev = &pdev->dev

and replace all &pdev->dev with that. It will make the code more
compact and easier to read.

Yours,
Linus Walleij

Re: [PATCH v2 4/5] ARM: mstar: Add gpio controller to MStar base dtsi

[Linus Walleij]

On Mon, Oct 19, 2020 at 4:10 PM Daniel Palmer <daniel@0x0f.com> wrote:

> The GPIO controller is at the same address in all of the
> currently known chips so create a node for it in the base
> dtsi.
>
> Some extra properties are needed to actually use it so
> disable it by default.
>
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>

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

Eventually this should go through the ARM SoC tree, but
it is fine to send with the rest of the patches, I will simply
just apply 1-3.

Yours,
Linus Walleij

Re: [PATCH v2 5/5] ARM: mstar: Fill in GPIO controller properties for infinity

[Linus Walleij]

On Mon, Oct 19, 2020 at 4:10 PM Daniel Palmer <daniel@0x0f.com> wrote:

> Fill in the properties needed to use the GPIO controller
> in the infinity and infinity3 chips.
>
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>

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

Yours,
Linus Walleij

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Marc Zyngier]

On 2020-11-05 09:40, Linus Walleij wrote:
> On Mon, Oct 19, 2020 at 4:10 PM Daniel Palmer <daniel@0x0f.com> wrote:

[...]

>> +/* The parent interrupt controller needs the GIC interrupt type set 
>> to GIC_SPI
>> + * so we need to provide the fwspec. Essentially 
>> gpiochip_populate_parent_fwspec_twocell
>> + * that puts GIC_SPI into the first cell.
>> + */

nit: comment style.

>> +static void *msc313_gpio_populate_parent_fwspec(struct gpio_chip *gc,
>> +                                            unsigned int 
>> parent_hwirq,
>> +                                            unsigned int parent_type)
>> +{
>> +       struct irq_fwspec *fwspec;
>> +
>> +       fwspec = kmalloc(sizeof(*fwspec), GFP_KERNEL);
>> +       if (!fwspec)
>> +               return NULL;
>> +
>> +       fwspec->fwnode = gc->irq.parent_domain->fwnode;
>> +       fwspec->param_count = 3;
>> +       fwspec->param[0] = GIC_SPI;
>> +       fwspec->param[1] = parent_hwirq;
>> +       fwspec->param[2] = parent_type;
>> +
>> +       return fwspec;
>> +}
> 
> Clever. Looping in Marc Z so he can say if this looks allright to him.

Yup, this looks correct. However, looking at the bit of the patch that
isn't quoted here, I see that msc313_gpio_irqchip doesn't have a
.irq_set_affinity callback. Is this system UP only?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Daniel Palmer]

Hi Marc,

On Thu, 5 Nov 2020 at 21:08, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-11-05 09:40, Linus Walleij wrote:
> > On Mon, Oct 19, 2020 at 4:10 PM Daniel Palmer <daniel@0x0f.com> wrote:
>
> [...]
>
> >> +/* The parent interrupt controller needs the GIC interrupt type set
> >> to GIC_SPI
> >> + * so we need to provide the fwspec. Essentially
> >> gpiochip_populate_parent_fwspec_twocell
> >> + * that puts GIC_SPI into the first cell.
> >> + */
>
> nit: comment style.

I've fixed these and some other bits for the v3.
I've held off on pushing that until the rest of it seemed right.

> >> +static void *msc313_gpio_populate_parent_fwspec(struct gpio_chip *gc,
> >> +                                            unsigned int
> >> parent_hwirq,
> >> +                                            unsigned int parent_type)
> >> +{
> >> +       struct irq_fwspec *fwspec;
> >> +
> >> +       fwspec = kmalloc(sizeof(*fwspec), GFP_KERNEL);
> >> +       if (!fwspec)
> >> +               return NULL;
> >> +
> >> +       fwspec->fwnode = gc->irq.parent_domain->fwnode;
> >> +       fwspec->param_count = 3;
> >> +       fwspec->param[0] = GIC_SPI;
> >> +       fwspec->param[1] = parent_hwirq;
> >> +       fwspec->param[2] = parent_type;
> >> +
> >> +       return fwspec;
> >> +}
> >
> > Clever. Looping in Marc Z so he can say if this looks allright to him.
>
> Yup, this looks correct. However, looking at the bit of the patch that
> isn't quoted here, I see that msc313_gpio_irqchip doesn't have a
> .irq_set_affinity callback. Is this system UP only?

What is in mainline right now is UP only but there are chips with a
second cortex A7 that I have working in my tree.
So I will add that in for v3 if I can work out what I should actually
do there. :)

Thanks,

Daniel

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Marc Zyngier]

On 2020-11-05 15:23, Daniel Palmer wrote:
> Hi Marc,
> 
> On Thu, 5 Nov 2020 at 21:08, Marc Zyngier <maz@kernel.org> wrote:
>> 
>> On 2020-11-05 09:40, Linus Walleij wrote:
>> > On Mon, Oct 19, 2020 at 4:10 PM Daniel Palmer <daniel@0x0f.com> wrote:
>> 
>> [...]
>> 
>> >> +/* The parent interrupt controller needs the GIC interrupt type set
>> >> to GIC_SPI
>> >> + * so we need to provide the fwspec. Essentially
>> >> gpiochip_populate_parent_fwspec_twocell
>> >> + * that puts GIC_SPI into the first cell.
>> >> + */
>> 
>> nit: comment style.
> 
> I've fixed these and some other bits for the v3.
> I've held off on pushing that until the rest of it seemed right.
> 
>> >> +static void *msc313_gpio_populate_parent_fwspec(struct gpio_chip *gc,
>> >> +                                            unsigned int
>> >> parent_hwirq,
>> >> +                                            unsigned int parent_type)
>> >> +{
>> >> +       struct irq_fwspec *fwspec;
>> >> +
>> >> +       fwspec = kmalloc(sizeof(*fwspec), GFP_KERNEL);
>> >> +       if (!fwspec)
>> >> +               return NULL;
>> >> +
>> >> +       fwspec->fwnode = gc->irq.parent_domain->fwnode;
>> >> +       fwspec->param_count = 3;
>> >> +       fwspec->param[0] = GIC_SPI;
>> >> +       fwspec->param[1] = parent_hwirq;
>> >> +       fwspec->param[2] = parent_type;
>> >> +
>> >> +       return fwspec;
>> >> +}
>> >
>> > Clever. Looping in Marc Z so he can say if this looks allright to him.
>> 
>> Yup, this looks correct. However, looking at the bit of the patch that
>> isn't quoted here, I see that msc313_gpio_irqchip doesn't have a
>> .irq_set_affinity callback. Is this system UP only?
> 
> What is in mainline right now is UP only but there are chips with a
> second cortex A7 that I have working in my tree.
> So I will add that in for v3 if I can work out what I should actually
> do there. :)

Probably nothing more than setting the callback to 
irq_chip_set_affinity_parent,
I'd expect.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Linus Walleij]

On Thu, Nov 5, 2020 at 4:43 PM Marc Zyngier <maz@kernel.org> wrote:
> On 2020-11-05 15:23, Daniel Palmer wrote:
> > On Thu, 5 Nov 2020 at 21:08, Marc Zyngier <maz@kernel.org> wrote:

> > >  I see that msc313_gpio_irqchip doesn't have a
> >> .irq_set_affinity callback. Is this system UP only?
> >
> > What is in mainline right now is UP only but there are chips with a
> > second cortex A7 that I have working in my tree.
> > So I will add that in for v3 if I can work out what I should actually
> > do there. :)
>
> Probably nothing more than setting the callback to
> irq_chip_set_affinity_parent,

Hm, is this something all GPIO irqchips used on SMP systems
should be doing? Or just hierarchical ones?

Yours,
Linus Walleij

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Marc Zyngier]

On 2020-11-10 14:02, Linus Walleij wrote:
> On Thu, Nov 5, 2020 at 4:43 PM Marc Zyngier <maz@kernel.org> wrote:
>> On 2020-11-05 15:23, Daniel Palmer wrote:
>> > On Thu, 5 Nov 2020 at 21:08, Marc Zyngier <maz@kernel.org> wrote:
> 
>> > >  I see that msc313_gpio_irqchip doesn't have a
>> >> .irq_set_affinity callback. Is this system UP only?
>> >
>> > What is in mainline right now is UP only but there are chips with a
>> > second cortex A7 that I have working in my tree.
>> > So I will add that in for v3 if I can work out what I should actually
>> > do there. :)
>> 
>> Probably nothing more than setting the callback to
>> irq_chip_set_affinity_parent,
> 
> Hm, is this something all GPIO irqchips used on SMP systems
> should be doing? Or just hierarchical ones?

Probably only the hierarchical ones. I'd expect the non-hierarchical
GPIOs to be muxed behind a single interrupt, which makes it impossible
to move a single GPIO around, and moving the mux interrupt would break
userspace's expectations that interrupts move independently of each 
others.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Linus Walleij]

On Tue, Nov 10, 2020 at 3:19 PM Marc Zyngier <maz@kernel.org> wrote:
> On 2020-11-10 14:02, Linus Walleij wrote:

> >> Probably nothing more than setting the callback to
> >> irq_chip_set_affinity_parent,
> >
> > Hm, is this something all GPIO irqchips used on SMP systems
> > should be doing? Or just hierarchical ones?
>
> Probably only the hierarchical ones. I'd expect the non-hierarchical
> GPIOs to be muxed behind a single interrupt, which makes it impossible
> to move a single GPIO around, and moving the mux interrupt would break
> userspace's expectations that interrupts move independently of each
> others.

I found two suspects and sent patches. I think I might have some
more candidates down in pinctrl. I do have some hierarchical IRQ
that is on UP systems, I suppose these are not affected.

Yours,
Linus Walleij

Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Marc Zyngier]

On 2020-11-11 14:09, Linus Walleij wrote:
> On Tue, Nov 10, 2020 at 3:19 PM Marc Zyngier <maz@kernel.org> wrote:
>> On 2020-11-10 14:02, Linus Walleij wrote:
> 
>> >> Probably nothing more than setting the callback to
>> >> irq_chip_set_affinity_parent,
>> >
>> > Hm, is this something all GPIO irqchips used on SMP systems
>> > should be doing? Or just hierarchical ones?
>> 
>> Probably only the hierarchical ones. I'd expect the non-hierarchical
>> GPIOs to be muxed behind a single interrupt, which makes it impossible
>> to move a single GPIO around, and moving the mux interrupt would break
>> userspace's expectations that interrupts move independently of each
>> others.
> 
> I found two suspects and sent patches. I think I might have some
> more candidates down in pinctrl. I do have some hierarchical IRQ
> that is on UP systems, I suppose these are not affected.

Yup, they look good. Feel free to add my Ack to them.
And yes, UP systems are naturally oblivious of interrupt affinity.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...