All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Add Realtek Otto GPIO support
@ 2021-03-15  8:23 Sander Vanheule
  2021-03-15  8:23 ` [PATCH 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
                   ` (6 more replies)
  0 siblings, 7 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-15  8:23 UTC (permalink / raw)
  To: linux-gpio, devicetree
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Thomas Gleixner,
	Marc Zyngier, linux-kernel, Sander Vanheule, Bert Vermeulen

Add support for the GPIO controller employed by Realtek in multiple series of
MIPS SoCs. These include the supported RTL838x and RTL839x series.
The register layout also matches the one found in GPIO controllers of
other (Lexra-based) SoCs such as RTL8196E, RTL8197D, and RTL8197F.

For the platform name 'otto', I am not aware of any official resources as to
what hardware this specifically applies to. However, in all of the GPL archives
we've received, from vendors using compatible SoCs in their design, the
platform under the MIPS architecture is referred to by this name.

The GPIO ports have been tested on a Zyxel GS1900-8 (RTL8380M), and
Zyxel GS1900-48 (RTL8393M). Furthermore, the GPIO ports and interrupt
controller have been tested on a Netgear GS110TPPv1 (RTL8381M).

Sander Vanheule (2):
  dt-bindings: gpio: Binding for Realtek Otto GPIO
  gpio: Add Realtek Otto GPIO support

 .../bindings/gpio/gpio-realtek-otto.yaml      |  80 +++++
 drivers/gpio/Kconfig                          |  12 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-realtek-otto.c              | 320 ++++++++++++++++++
 4 files changed, 413 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

-- 
2.30.2


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

* [PATCH 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO
  2021-03-15  8:23 [PATCH 0/2] Add Realtek Otto GPIO support Sander Vanheule
@ 2021-03-15  8:23 ` Sander Vanheule
  2021-03-15 15:01   ` Linus Walleij
  2021-03-15  8:23 ` [PATCH 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-15  8:23 UTC (permalink / raw)
  To: linux-gpio, devicetree
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Thomas Gleixner,
	Marc Zyngier, linux-kernel, Sander Vanheule, Bert Vermeulen

Add a binding description for Realtek's GPIO controller found on several
of their MIPS-based SoCs (codenamed Otto), such as the RTL838x and
RTL839x series of switch SoCs.

A fallback binding 'realtek,otto-gpio' is provided for cases where the
actual port ordering is not known yet, and enabling the interrupt
controller may result in uncaught interrupts.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 .../bindings/gpio/gpio-realtek-otto.yaml      | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml b/Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml
new file mode 100644
index 000000000000..3e8151e3a169
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-realtek-otto.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek Otto GPIO controller
+
+maintainers:
+  - Sander Vanheule <sander@svanheule.net>
+  - Bert Vermeulen <bert@biot.com>
+
+description: |
+  Realtek's GPIO controller on their MIPS switch SoCs (Otto platform) consists
+  of two banks of 32 GPIOs. These GPIOs can generate edge-triggered interrupts.
+  Each bank's interrupts are cascased into one interrupt line on the parent
+  interrupt controller, if provided.
+  This binding allows defining a single bank in the devicetree. The interrupt
+  controller is not supported on the fallback compatible name, which only
+  allows for GPIO port use.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - realtek,rtl8380-gpio
+              - realtek,rtl8390-gpio
+          - const: realtek,otto-gpio
+      - const: realtek,otto-gpio
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  ngpios:
+    minimum: 1
+    maximum: 32
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+dependencies:
+  interrupt-controller: [ interrupts ]
+
+examples:
+  - |
+      gpio@3500 {
+        compatible = "realtek,rtl8380-gpio", "realtek,otto-gpio";
+        reg = <0x3500 0x1c>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        ngpios = <24>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&rtlintc>;
+        interrupts = <23>;
+      };
+
+...
-- 
2.30.2


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

* [PATCH 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-15  8:23 [PATCH 0/2] Add Realtek Otto GPIO support Sander Vanheule
  2021-03-15  8:23 ` [PATCH 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
@ 2021-03-15  8:23 ` Sander Vanheule
  2021-03-15 15:10   ` Linus Walleij
  2021-03-15 19:08 ` [PATCH v2 0/2] " Sander Vanheule
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-15  8:23 UTC (permalink / raw)
  To: linux-gpio, devicetree
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Thomas Gleixner,
	Marc Zyngier, linux-kernel, Sander Vanheule, Bert Vermeulen

Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
64 GPIOs, divided over two banks. Each bank has a set of registers for
32 GPIOs, with support for edge-triggered interrupts.

Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
registers pack one bit per GPIO, except for the IMR register, which
packs two bits per GPIO (AB-CD).

Although the byte order is currently assumed to have port A..D at offset
0x0..0x3, this has been observed to be reversed on other, Lexra-based,
SoCs (e.g. RTL8196E/97D/97F).

Interrupt support is disabled for the fallback devicetree-compatible
'realtek,otto-gpio'. This allows for quick support of GPIO banks in
which the byte order would be unknown. In this case, the port ordering
in the IMR registers may not match the reversed order in the other
registers (DCBA, and BA-DC or DC-BA).

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/gpio/Kconfig             |  12 ++
 drivers/gpio/Makefile            |   1 +
 drivers/gpio/gpio-realtek-otto.c | 320 +++++++++++++++++++++++++++++++
 3 files changed, 333 insertions(+)
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..fedf1e49469e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -502,6 +502,18 @@ config GPIO_RDA
 	help
 	  Say Y here to support RDA Micro GPIO controller.
 
+config GPIO_REALTEK_OTTO
+	bool "Realtek Otto GPIO support"
+	depends on MACH_REALTEK_RTL
+	depends on OF_GPIO
+	default MACH_REALTEK_RTL
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	help
+	  The GPIO controller on the Otto MIPS platform supports up to two
+	  banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
+	  are grouped in four 8-bit wide ports.
+
 config GPIO_REG
 	bool
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..8ace5934e3c3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583)		+= gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RCAR)			+= gpio-rcar.o
 obj-$(CONFIG_GPIO_RDA)			+= gpio-rda.o
 obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
+obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
new file mode 100644
index 000000000000..04c11b2085f8
--- /dev/null
+++ b/drivers/gpio/gpio-realtek-otto.c
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/gpio/driver.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/swab.h>
+
+/*
+ * Total register block size is 0x1C for four ports.
+ * On the RTL8380/RLT8390 platforms port A, B, and C are implemented.
+ * RTL8389 and RTL8328 implement a second bank with ports E, F, G, and H.
+ *
+ * Port information is stored with the first port at offset 0, followed by the
+ * second, etc. Most registers store one bit per GPIO and should be read out in
+ * reversed endian order. The two interrupt mask registers store two bits per
+ * GPIO, and should be manipulated with swahw32, if required.
+ */
+
+/*
+ * Pin select: (0) "normal", (1) "dedicate peripheral"
+ * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits
+ * in the peripheral registers.
+ */
+#define REALTEK_GPIO_REG_CNR		0x00
+/* Clear bit (0) for input, set bit (1) for output */
+#define REALTEK_GPIO_REG_DIR		0x08
+#define REALTEK_GPIO_REG_DATA		0x0C
+/* Read bit for IRQ status, write 1 to clear IRQ */
+#define REALTEK_GPIO_REG_ISR		0x10
+/* Two bits per GPIO */
+#define REALTEK_GPIO_REG_IMR_AB		0x14
+#define REALTEK_GPIO_REG_IMR_CD		0x18
+#define REALTEK_GPIO_IRQ_EDGE_FALLING	1
+#define REALTEK_GPIO_IRQ_EDGE_RISING	2
+#define REALTEK_GPIO_IRQ_EDGE_BOTH	3
+
+#define REALTEK_GPIO_MAX		32
+
+/*
+ * Realtek GPIO driver data
+ * Because the interrupt mask register (IMR) combines the function of
+ * IRQ type selection and masking, two extra values are stored.
+ * intr_mask is used to mask/unmask the interrupts for certain GPIO,
+ * and intr_type is used to store the selected interrupt types. The
+ * logical AND of these values is written to IMR on changes.
+ *
+ * @dev Parent device
+ * @gc Associated gpio_chip instance
+ * @base Base address of the register block
+ * @lock Lock for accessing the IRQ registers and values
+ * @intr_mask Mask for GPIO interrupts
+ * @intr_type GPIO interrupt type selection
+ */
+struct realtek_gpio_ctrl {
+	struct device *dev;
+	struct gpio_chip gc;
+	void __iomem *base;
+	raw_spinlock_t lock;
+	u32 intr_mask[2];
+	u32 intr_type[2];
+};
+
+enum realtek_gpio_flags {
+	GPIO_INTERRUPTS = BIT(0),
+};
+
+static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+
+	return container_of(gc, struct realtek_gpio_ctrl, gc);
+}
+
+static inline u32 realtek_gpio_isr_read(struct realtek_gpio_ctrl *ctrl)
+{
+	return swab32(readl(ctrl->base + REALTEK_GPIO_REG_ISR));
+}
+
+static inline void realtek_gpio_isr_clear(struct realtek_gpio_ctrl *ctrl,
+	unsigned int pin_mask)
+{
+	writel(swab32(pin_mask), ctrl->base + REALTEK_GPIO_REG_ISR);
+}
+
+static inline void realtek_update_imr(struct realtek_gpio_ctrl *ctrl,
+	unsigned int offset, u32 type, u32 mask)
+{
+	unsigned int reg;
+
+	if (offset == 0)
+		reg = REALTEK_GPIO_REG_IMR_AB;
+	else
+		reg = REALTEK_GPIO_REG_IMR_CD;
+	writel(swahw32(type & mask), ctrl->base + reg);
+}
+
+static inline u32 realtek_gpio_imr_bits(unsigned int pin, u32 value)
+{
+	return ((value & 0x3) << 2*(pin % 16));
+}
+
+static void realtek_gpio_irq_ack(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	u32 pin = irqd_to_hwirq(data);
+
+	realtek_gpio_isr_clear(ctrl, BIT(pin));
+}
+
+static void realtek_gpio_irq_unmask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int pin = irqd_to_hwirq(data);
+	unsigned int offset = pin/16;
+	unsigned long flags;
+	u32 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[offset];
+	m |= realtek_gpio_imr_bits(pin, 3);
+	ctrl->intr_mask[offset] = m;
+	realtek_update_imr(ctrl, offset, ctrl->intr_type[offset], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static void realtek_gpio_irq_mask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int pin = irqd_to_hwirq(data);
+	unsigned int offset = pin/16;
+	unsigned long flags;
+	u32 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[offset];
+	m &= ~realtek_gpio_imr_bits(pin, 3);
+	ctrl->intr_mask[offset] = m;
+	realtek_update_imr(ctrl, offset, ctrl->intr_type[offset], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static int realtek_gpio_irq_set_type(struct irq_data *data,
+	unsigned int flow_type)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	irq_flow_handler_t handler;
+	unsigned int pin = irqd_to_hwirq(data);
+	unsigned int offset = pin/16;
+	unsigned long flags;
+	u32 type, t;
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_NONE:
+		type = 0;
+		handler = handle_bad_irq;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		type = REALTEK_GPIO_IRQ_EDGE_FALLING;
+		handler = handle_edge_irq;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		type = REALTEK_GPIO_IRQ_EDGE_RISING;
+		handler = handle_edge_irq;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		type = REALTEK_GPIO_IRQ_EDGE_BOTH;
+		handler = handle_edge_irq;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	irq_set_handler_locked(data, handler);
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	t = ctrl->intr_type[offset];
+	t &= ~realtek_gpio_imr_bits(pin, 3);
+	t |= realtek_gpio_imr_bits(pin, type);
+	ctrl->intr_type[offset] = t;
+	realtek_update_imr(ctrl, offset, t, ctrl->intr_mask[offset]);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static void realtek_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+	int offset;
+	unsigned long status;
+
+	chained_irq_enter(irq_chip, desc);
+
+	status = realtek_gpio_isr_read(ctrl);
+	for_each_set_bit(offset, &status, gc->ngpio) {
+		dev_dbg(ctrl->dev, "gpio irq %d\n", offset);
+		generic_handle_irq(irq_find_mapping(gc->irq.domain,
+						offset));
+		realtek_gpio_isr_clear(ctrl, BIT(offset));
+	}
+
+	chained_irq_exit(irq_chip, desc);
+}
+
+static struct irq_chip realtek_gpio_irq_chip = {
+	.name = "realtek-otto-gpio",
+	.irq_ack = realtek_gpio_irq_ack,
+	.irq_mask = realtek_gpio_irq_mask,
+	.irq_unmask = realtek_gpio_irq_unmask,
+	.irq_set_type = realtek_gpio_irq_set_type
+};
+
+static const struct of_device_id realtek_gpio_of_match[] = {
+	{ .compatible = "realtek,otto-gpio" },
+	{
+		.compatible = "realtek,rtl8380-gpio",
+		.data = (void *)GPIO_INTERRUPTS
+	},
+	{
+		.compatible = "realtek,rtl8390-gpio",
+		.data = (void *)GPIO_INTERRUPTS
+	},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
+
+static int realtek_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct gpio_irq_chip *girq;
+	struct realtek_gpio_ctrl *ctrl;
+	const struct of_device_id *match;
+	unsigned int match_flags;
+	u32 ngpios;
+	int err, irq;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	ctrl->dev = dev;
+
+	if (!np) {
+		dev_err(&pdev->dev, "no DT node found\n");
+		return -EINVAL;
+	}
+
+	match = of_match_node(realtek_gpio_of_match, np);
+	match_flags = (unsigned int) match->data;
+
+	if (of_property_read_u32(np, "ngpios", &ngpios) != 0)
+		ngpios = REALTEK_GPIO_MAX;
+
+	if (ngpios > REALTEK_GPIO_MAX) {
+		dev_err(&pdev->dev, "invalid ngpios (max. %d)\n",
+			REALTEK_GPIO_MAX);
+		return -EINVAL;
+	}
+
+	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ctrl->base))
+		return PTR_ERR(ctrl->base);
+
+	raw_spin_lock_init(&ctrl->lock);
+
+	err = bgpio_init(&ctrl->gc, dev, 4,
+		ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL,
+		ctrl->base + REALTEK_GPIO_REG_DIR, NULL,
+		BGPIOF_BIG_ENDIAN_BYTE_ORDER);
+	if (err) {
+		dev_err(dev, "unable to init generic GPIO");
+		return err;
+	}
+
+	ctrl->gc.ngpio = ngpios;
+	ctrl->gc.owner = THIS_MODULE;
+
+	irq = of_irq_get(np, 0);
+	if ((match_flags & GPIO_INTERRUPTS) && irq > 0) {
+		girq = &ctrl->gc.irq;
+		girq->chip = &realtek_gpio_irq_chip;
+		girq->parent_handler = realtek_gpio_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_bad_irq;
+		girq->parents[0] = irq;
+
+		/* Disable and clear all interrupts */
+		realtek_update_imr(ctrl, 0, 0, 0);
+		realtek_update_imr(ctrl, 1, 0, 0);
+		realtek_gpio_isr_clear(ctrl, BIT(ngpios)-1);
+	}
+
+	err = gpiochip_add_data(&ctrl->gc, ctrl);
+	return err;
+}
+
+static struct platform_driver realtek_gpio_driver = {
+	.driver = {
+		.name = "realtek-otto-gpio",
+		.of_match_table	= realtek_gpio_of_match,
+	},
+	.probe = realtek_gpio_probe,
+};
+
+builtin_platform_driver(realtek_gpio_driver);
+
+MODULE_DESCRIPTION("Realtek Otto GPIO support");
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2


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

* Re: [PATCH 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO
  2021-03-15  8:23 ` [PATCH 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
@ 2021-03-15 15:01   ` Linus Walleij
  0 siblings, 0 replies; 39+ messages in thread
From: Linus Walleij @ 2021-03-15 15:01 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	linux-kernel, Bert Vermeulen

On Mon, Mar 15, 2021 at 9:25 AM Sander Vanheule <sander@svanheule.net> wrote:

> Add a binding description for Realtek's GPIO controller found on several
> of their MIPS-based SoCs (codenamed Otto), such as the RTL838x and
> RTL839x series of switch SoCs.
>
> A fallback binding 'realtek,otto-gpio' is provided for cases where the
> actual port ordering is not known yet, and enabling the interrupt
> controller may result in uncaught interrupts.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>

These bindings look good to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

* Re: [PATCH 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-15  8:23 ` [PATCH 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
@ 2021-03-15 15:10   ` Linus Walleij
  2021-03-15 19:09     ` Sander Vanheule
  0 siblings, 1 reply; 39+ messages in thread
From: Linus Walleij @ 2021-03-15 15:10 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	linux-kernel, Bert Vermeulen

On Mon, Mar 15, 2021 at 9:26 AM Sander Vanheule <sander@svanheule.net> wrote:

> Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
> 64 GPIOs, divided over two banks. Each bank has a set of registers for
> 32 GPIOs, with support for edge-triggered interrupts.
>
> Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
> registers pack one bit per GPIO, except for the IMR register, which
> packs two bits per GPIO (AB-CD).
>
> Although the byte order is currently assumed to have port A..D at offset
> 0x0..0x3, this has been observed to be reversed on other, Lexra-based,
> SoCs (e.g. RTL8196E/97D/97F).
>
> Interrupt support is disabled for the fallback devicetree-compatible
> 'realtek,otto-gpio'. This allows for quick support of GPIO banks in
> which the byte order would be unknown. In this case, the port ordering
> in the IMR registers may not match the reversed order in the other
> registers (DCBA, and BA-DC or DC-BA).
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>

Overall this is a beautiful driver and it makes use of all the generic
frameworks I can think of. I don't see any reason not to merge
it so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

The following is some notes and nitpicks, nothing blocking any
merge, more like discussion.

> +enum realtek_gpio_flags {
> +       GPIO_INTERRUPTS = BIT(0),
> +};

I suppose this looks like this because more flags will be introduced
when you add more functionality to the driver. Otherwise it seems
like overkill so a bool would suffice.

I would add a comment /* TODO: this will be expanded */

> +static inline u32 realtek_gpio_imr_bits(unsigned int pin, u32 value)
> +{
> +       return ((value & 0x3) << 2*(pin % 16));
> +}

I would explain a bit about this, obviouslt it is two bit per
line, but it took me some time to parse, so a comment
about the bit layout would be nice.

> +       unsigned int offset = pin/16;

Here that number appears again.

The use of GPIO_GENERIC and GPIO irqchip is flawless
and first class.

Thanks!
Linus Walleij

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

* [PATCH v2 0/2] Add Realtek Otto GPIO support
  2021-03-15  8:23 [PATCH 0/2] Add Realtek Otto GPIO support Sander Vanheule
  2021-03-15  8:23 ` [PATCH 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
  2021-03-15  8:23 ` [PATCH 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
@ 2021-03-15 19:08 ` Sander Vanheule
  2021-03-15 19:08   ` [PATCH v2 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
  2021-03-15 19:08   ` [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
  2021-03-24 21:22 ` [PATCH v3 0/2] " Sander Vanheule
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-15 19:08 UTC (permalink / raw)
  To: linux-gpio, devicetree
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Thomas Gleixner,
	Marc Zyngier, linux-kernel, Sander Vanheule, bert

Add support for the GPIO controller employed by Realtek in multiple series of
MIPS SoCs. These include the supported RTL838x and RTL839x. The register layout
also matches the one found in the GPIO controller of other (Lexra-based) SoCs
such as RTL8196E, RTL8197D, and RTL8197F.

For the platform name 'otto', I am not aware of any official resources as to
what hardware this specifically applies to. However, in all of the GPL archives
we've received, from vendors using compatible SoCs in their design, the
platform under the MIPS architecture is referred to by this name.

The GPIO ports have been tested on a Zyxel GS1900-8 (RTL8380), and Zyxel
GS1900-48 (RTL8393). Furthermore, the GPIO ports and interrupt controller have
been tested on a Netgear GS110TPPv1 (RTL8381).

Changes in v2:
- Clarify structure and usage of IMR registers

Sander Vanheule (2):
  dt-bindings: gpio: Binding for Realtek Otto GPIO
  gpio: Add Realtek Otto GPIO support

 .../bindings/gpio/gpio-realtek-otto.yaml      |  80 +++++
 drivers/gpio/Kconfig                          |  12 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-realtek-otto.c              | 331 ++++++++++++++++++
 4 files changed, 424 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

-- 
2.30.2


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

* [PATCH v2 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO
  2021-03-15 19:08 ` [PATCH v2 0/2] " Sander Vanheule
@ 2021-03-15 19:08   ` Sander Vanheule
  2021-03-24 18:29     ` Rob Herring
  2021-03-15 19:08   ` [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
  1 sibling, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-15 19:08 UTC (permalink / raw)
  To: linux-gpio, devicetree
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Thomas Gleixner,
	Marc Zyngier, linux-kernel, Sander Vanheule, bert

Add a binding description for Realtek's GPIO controller found on several
of their MIPS-based SoCs (codenamed Otto), such as the RTL838x and
RTL839x series of switch SoCs.

A fallback binding 'realtek,otto-gpio' is provided for cases where the
actual port ordering is not known yet, and enabling the interrupt
controller may result in uncaught interrupts.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../bindings/gpio/gpio-realtek-otto.yaml      | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml b/Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml
new file mode 100644
index 000000000000..3e8151e3a169
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-realtek-otto.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek Otto GPIO controller
+
+maintainers:
+  - Sander Vanheule <sander@svanheule.net>
+  - Bert Vermeulen <bert@biot.com>
+
+description: |
+  Realtek's GPIO controller on their MIPS switch SoCs (Otto platform) consists
+  of two banks of 32 GPIOs. These GPIOs can generate edge-triggered interrupts.
+  Each bank's interrupts are cascased into one interrupt line on the parent
+  interrupt controller, if provided.
+  This binding allows defining a single bank in the devicetree. The interrupt
+  controller is not supported on the fallback compatible name, which only
+  allows for GPIO port use.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - realtek,rtl8380-gpio
+              - realtek,rtl8390-gpio
+          - const: realtek,otto-gpio
+      - const: realtek,otto-gpio
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  ngpios:
+    minimum: 1
+    maximum: 32
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+dependencies:
+  interrupt-controller: [ interrupts ]
+
+examples:
+  - |
+      gpio@3500 {
+        compatible = "realtek,rtl8380-gpio", "realtek,otto-gpio";
+        reg = <0x3500 0x1c>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        ngpios = <24>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&rtlintc>;
+        interrupts = <23>;
+      };
+
+...
-- 
2.30.2


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

* [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-15 19:08 ` [PATCH v2 0/2] " Sander Vanheule
  2021-03-15 19:08   ` [PATCH v2 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
@ 2021-03-15 19:08   ` Sander Vanheule
  2021-03-17 13:08     ` Andy Shevchenko
  1 sibling, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-15 19:08 UTC (permalink / raw)
  To: linux-gpio, devicetree
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Thomas Gleixner,
	Marc Zyngier, linux-kernel, Sander Vanheule, bert

Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
64 GPIOs, divided over two banks. Each bank has a set of registers for
32 GPIOs, with support for edge-triggered interrupts.

Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
registers pack one bit per GPIO, except for the IMR register, which
packs two bits per GPIO (AB-CD).

Although the byte order is currently assumed to have port A..D at offset
0x0..0x3, this has been observed to be reversed on other, Lexra-based,
SoCs (e.g. RTL8196E/97D/97F).

Interrupt support is disabled for the fallback devicetree-compatible
'realtek,otto-gpio'. This allows for quick support of GPIO banks in
which the byte order would be unknown. In this case, the port ordering
in the IMR registers may not match the reversed order in the other
registers (DCBA, and BA-DC or DC-BA).

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/Kconfig             |  12 ++
 drivers/gpio/Makefile            |   1 +
 drivers/gpio/gpio-realtek-otto.c | 331 +++++++++++++++++++++++++++++++
 3 files changed, 344 insertions(+)
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..fedf1e49469e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -502,6 +502,18 @@ config GPIO_RDA
 	help
 	  Say Y here to support RDA Micro GPIO controller.
 
+config GPIO_REALTEK_OTTO
+	bool "Realtek Otto GPIO support"
+	depends on MACH_REALTEK_RTL
+	depends on OF_GPIO
+	default MACH_REALTEK_RTL
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	help
+	  The GPIO controller on the Otto MIPS platform supports up to two
+	  banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
+	  are grouped in four 8-bit wide ports.
+
 config GPIO_REG
 	bool
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..8ace5934e3c3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583)		+= gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RCAR)			+= gpio-rcar.o
 obj-$(CONFIG_GPIO_RDA)			+= gpio-rda.o
 obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
+obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
new file mode 100644
index 000000000000..818412687346
--- /dev/null
+++ b/drivers/gpio/gpio-realtek-otto.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/gpio/driver.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/swab.h>
+
+/*
+ * Total register block size is 0x1C for four ports.
+ * On the RTL8380/RLT8390 platforms port A, B, and C are implemented.
+ * RTL8389 and RTL8328 implement a second bank with ports E, F, G, and H.
+ *
+ * Port information is stored with the first port at offset 0, followed by the
+ * second, etc. Most registers store one bit per GPIO and should be read out in
+ * reversed endian order. The two interrupt mask registers store two bits per
+ * GPIO, and should be manipulated with swahw32, if required.
+ */
+
+/*
+ * Pin select: (0) "normal", (1) "dedicate peripheral"
+ * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits
+ * in the peripheral registers.
+ */
+#define REALTEK_GPIO_REG_CNR		0x00
+/* Clear bit (0) for input, set bit (1) for output */
+#define REALTEK_GPIO_REG_DIR		0x08
+#define REALTEK_GPIO_REG_DATA		0x0C
+/* Read bit for IRQ status, write 1 to clear IRQ */
+#define REALTEK_GPIO_REG_ISR		0x10
+/* Two bits per GPIO */
+#define REALTEK_GPIO_REG_IMR_AB		0x14
+#define REALTEK_GPIO_REG_IMR_CD		0x18
+#define REALTEK_GPIO_IMR_LINE_MASK	3
+#define REALTEK_GPIO_IRQ_EDGE_FALLING	1
+#define REALTEK_GPIO_IRQ_EDGE_RISING	2
+#define REALTEK_GPIO_IRQ_EDGE_BOTH	3
+
+#define REALTEK_GPIO_MAX		32
+
+/*
+ * Realtek GPIO driver data
+ * Because the interrupt mask register (IMR) combines the function of
+ * IRQ type selection and masking, two extra values are stored.
+ * intr_mask is used to mask/unmask the interrupts for certain GPIO,
+ * and intr_type is used to store the selected interrupt types. The
+ * logical AND of these values is written to IMR on changes.
+ *
+ * @dev Parent device
+ * @gc Associated gpio_chip instance
+ * @base Base address of the register block
+ * @lock Lock for accessing the IRQ registers and values
+ * @intr_mask Mask for GPIO interrupts
+ * @intr_type GPIO interrupt type selection
+ */
+struct realtek_gpio_ctrl {
+	struct device *dev;
+	struct gpio_chip gc;
+	void __iomem *base;
+	raw_spinlock_t lock;
+	u32 intr_mask[2];
+	u32 intr_type[2];
+};
+
+enum realtek_gpio_flags {
+	GPIO_INTERRUPTS = BIT(0),
+};
+
+static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+
+	return container_of(gc, struct realtek_gpio_ctrl, gc);
+}
+
+static inline u32 realtek_gpio_isr_read(struct realtek_gpio_ctrl *ctrl)
+{
+	return swab32(readl(ctrl->base + REALTEK_GPIO_REG_ISR));
+}
+
+static inline void realtek_gpio_isr_clear(struct realtek_gpio_ctrl *ctrl,
+	unsigned int pin_mask)
+{
+	writel(swab32(pin_mask), ctrl->base + REALTEK_GPIO_REG_ISR);
+}
+
+static inline void realtek_gpio_update_imr(struct realtek_gpio_ctrl *ctrl,
+	unsigned int imr_offset, u32 type, u32 mask)
+{
+	unsigned int reg;
+
+	if (imr_offset == 0)
+		reg = REALTEK_GPIO_REG_IMR_AB;
+	else
+		reg = REALTEK_GPIO_REG_IMR_CD;
+	writel(swahw32(type & mask), ctrl->base + reg);
+}
+
+/*
+ * Since the IMRs contain two bits per GPIO, only 16 GPIO lines fit in a 32-bit
+ * register.
+ * Use realtek_gpio_pin_to_imr_offset() to select the correct IMR offset, and
+ * realtek_gpio_imr_bits() to put the GPIO line's new value in the right place.
+ */
+static inline u32 realtek_gpio_pin_to_imr_offset(unsigned int pin)
+{
+	return pin/16;
+}
+
+static inline u32 realtek_gpio_imr_bits(unsigned int pin, u32 value)
+{
+	return ((value & REALTEK_GPIO_IMR_LINE_MASK) << 2*(pin % 16));
+}
+
+static void realtek_gpio_irq_ack(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	u32 pin = irqd_to_hwirq(data);
+
+	realtek_gpio_isr_clear(ctrl, BIT(pin));
+}
+
+static void realtek_gpio_irq_unmask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int pin = irqd_to_hwirq(data);
+	unsigned int imr_offset = realtek_gpio_pin_to_imr_offset(pin);
+	unsigned long flags;
+	u32 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[imr_offset];
+	m |= realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[imr_offset] = m;
+	realtek_gpio_update_imr(ctrl, imr_offset, ctrl->intr_type[imr_offset], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static void realtek_gpio_irq_mask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int pin = irqd_to_hwirq(data);
+	unsigned int imr_offset = realtek_gpio_pin_to_imr_offset(pin);
+	unsigned long flags;
+	u32 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[imr_offset];
+	m &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[imr_offset] = m;
+	realtek_gpio_update_imr(ctrl, imr_offset, ctrl->intr_type[imr_offset], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static int realtek_gpio_irq_set_type(struct irq_data *data,
+	unsigned int flow_type)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	irq_flow_handler_t handler;
+	unsigned int pin = irqd_to_hwirq(data);
+	unsigned int imr_offset = realtek_gpio_pin_to_imr_offset(pin);
+	unsigned long flags;
+	u32 type, t;
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_NONE:
+		type = 0;
+		handler = handle_bad_irq;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		type = REALTEK_GPIO_IRQ_EDGE_FALLING;
+		handler = handle_edge_irq;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		type = REALTEK_GPIO_IRQ_EDGE_RISING;
+		handler = handle_edge_irq;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		type = REALTEK_GPIO_IRQ_EDGE_BOTH;
+		handler = handle_edge_irq;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	irq_set_handler_locked(data, handler);
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	t = ctrl->intr_type[imr_offset];
+	t &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
+	t |= realtek_gpio_imr_bits(pin, type);
+	ctrl->intr_type[imr_offset] = t;
+	realtek_gpio_update_imr(ctrl, imr_offset, t, ctrl->intr_mask[imr_offset]);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static void realtek_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+	int offset;
+	unsigned long status;
+
+	chained_irq_enter(irq_chip, desc);
+
+	status = realtek_gpio_isr_read(ctrl);
+	for_each_set_bit(offset, &status, gc->ngpio) {
+		dev_dbg(ctrl->dev, "gpio irq %d\n", offset);
+		generic_handle_irq(irq_find_mapping(gc->irq.domain, offset));
+		realtek_gpio_isr_clear(ctrl, BIT(offset));
+	}
+
+	chained_irq_exit(irq_chip, desc);
+}
+
+static struct irq_chip realtek_gpio_irq_chip = {
+	.name = "realtek-otto-gpio",
+	.irq_ack = realtek_gpio_irq_ack,
+	.irq_mask = realtek_gpio_irq_mask,
+	.irq_unmask = realtek_gpio_irq_unmask,
+	.irq_set_type = realtek_gpio_irq_set_type
+};
+
+static const struct of_device_id realtek_gpio_of_match[] = {
+	{ .compatible = "realtek,otto-gpio" },
+	{
+		.compatible = "realtek,rtl8380-gpio",
+		.data = (void *)GPIO_INTERRUPTS
+	},
+	{
+		.compatible = "realtek,rtl8390-gpio",
+		.data = (void *)GPIO_INTERRUPTS
+	},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
+
+static int realtek_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct gpio_irq_chip *girq;
+	struct realtek_gpio_ctrl *ctrl;
+	const struct of_device_id *match;
+	unsigned int match_flags;
+	u32 ngpios;
+	int err, irq;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	ctrl->dev = dev;
+
+	if (!np) {
+		dev_err(&pdev->dev, "no DT node found\n");
+		return -EINVAL;
+	}
+
+	match = of_match_node(realtek_gpio_of_match, np);
+	match_flags = (unsigned int) match->data;
+
+	if (of_property_read_u32(np, "ngpios", &ngpios) != 0)
+		ngpios = REALTEK_GPIO_MAX;
+
+	if (ngpios > REALTEK_GPIO_MAX) {
+		dev_err(&pdev->dev, "invalid ngpios (max. %d)\n",
+			REALTEK_GPIO_MAX);
+		return -EINVAL;
+	}
+
+	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ctrl->base))
+		return PTR_ERR(ctrl->base);
+
+	raw_spin_lock_init(&ctrl->lock);
+
+	err = bgpio_init(&ctrl->gc, dev, 4,
+		ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL,
+		ctrl->base + REALTEK_GPIO_REG_DIR, NULL,
+		BGPIOF_BIG_ENDIAN_BYTE_ORDER);
+	if (err) {
+		dev_err(dev, "unable to init generic GPIO");
+		return err;
+	}
+
+	ctrl->gc.ngpio = ngpios;
+	ctrl->gc.owner = THIS_MODULE;
+
+	irq = of_irq_get(np, 0);
+	if ((match_flags & GPIO_INTERRUPTS) && irq > 0) {
+		girq = &ctrl->gc.irq;
+		girq->chip = &realtek_gpio_irq_chip;
+		girq->parent_handler = realtek_gpio_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_bad_irq;
+		girq->parents[0] = irq;
+
+		/* Disable and clear all interrupts */
+		realtek_gpio_update_imr(ctrl, 0, 0, 0);
+		realtek_gpio_update_imr(ctrl, 1, 0, 0);
+		realtek_gpio_isr_clear(ctrl, BIT(ngpios)-1);
+	}
+
+	err = gpiochip_add_data(&ctrl->gc, ctrl);
+	return err;
+}
+
+static struct platform_driver realtek_gpio_driver = {
+	.driver = {
+		.name = "realtek-otto-gpio",
+		.of_match_table	= realtek_gpio_of_match,
+	},
+	.probe = realtek_gpio_probe,
+};
+
+builtin_platform_driver(realtek_gpio_driver);
+
+MODULE_DESCRIPTION("Realtek Otto GPIO support");
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2


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

* Re: [PATCH 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-15 15:10   ` Linus Walleij
@ 2021-03-15 19:09     ` Sander Vanheule
  0 siblings, 0 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-15 19:09 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	linux-kernel, Bert Vermeulen

On Mon, 2021-03-15 at 16:10 +0100, Linus Walleij wrote:
> On Mon, Mar 15, 2021 at 9:26 AM Sander Vanheule
> <sander@svanheule.net> wrote:
> 
> > Realtek MIPS SoCs (platform name Otto) have GPIO controllers with
> > up to
> > 64 GPIOs, divided over two banks. Each bank has a set of registers
> > for
> > 32 GPIOs, with support for edge-triggered interrupts.
> > 
> > Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH).
> > Most
> > registers pack one bit per GPIO, except for the IMR register, which
> > packs two bits per GPIO (AB-CD).
> > 
> > Although the byte order is currently assumed to have port A..D at
> > offset
> > 0x0..0x3, this has been observed to be reversed on other, Lexra-
> > based,
> > SoCs (e.g. RTL8196E/97D/97F).
> > 
> > Interrupt support is disabled for the fallback devicetree-
> > compatible
> > 'realtek,otto-gpio'. This allows for quick support of GPIO banks in
> > which the byte order would be unknown. In this case, the port
> > ordering
> > in the IMR registers may not match the reversed order in the other
> > registers (DCBA, and BA-DC or DC-BA).
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> 
> Overall this is a beautiful driver and it makes use of all the
> generic
> frameworks I can think of. I don't see any reason not to merge
> it so:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for the review and the kind comments!


> 
> The following is some notes and nitpicks, nothing blocking any
> merge, more like discussion.
> 
> > +enum realtek_gpio_flags {
> > +       GPIO_INTERRUPTS = BIT(0),
> > +};
> 
> I suppose this looks like this because more flags will be introduced
> when you add more functionality to the driver. Otherwise it seems
> like overkill so a bool would suffice.
> 
> I would add a comment /* TODO: this will be expanded */
> 

That's correct, I would like this to be extendable. Like the commit
message noted, some other SoC appear to have port order D-C-B-A. The
current driver only supports the A-B-C-D port order, so a flag could be
added to differentiate between A-first and D-first.

Another flag that will be added in the future, is one to indicate that
the GPIO block has extra interrupt control registers, located after the
second GPIO bank.

For example, the rtl9300-series appears to have both the reversed port
order, and an extra "interrupt enable" register. This is not yet
implemented, since I don't currently have a device with this type of
SoC.


> > +static inline u32 realtek_gpio_imr_bits(unsigned int pin, u32
> > value)
> > +{
> > +       return ((value & 0x3) << 2*(pin % 16));
> > +}
> 
> I would explain a bit about this, obviouslt it is two bit per
> line, but it took me some time to parse, so a comment
> about the bit layout would be nice.
> 
> > +       unsigned int offset = pin/16;
> 
> Here that number appears again.
> 

I've updated the patch (and added your Reviewed-by tags) for a v2.
Hopefully this is now more obvious from the code and comments.

Best,
Sander

> The use of GPIO_GENERIC and GPIO irqchip is flawless
> and first class.
> 
> Thanks!
> Linus Walleij



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

* Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-15 19:08   ` [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
@ 2021-03-17 13:08     ` Andy Shevchenko
  2021-03-19 15:51       ` Sander Vanheule
  0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2021-03-17 13:08 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: open list:GPIO SUBSYSTEM, devicetree, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	Linux Kernel Mailing List, Bert Vermeulen

On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule <sander@svanheule.net> wrote:
>
> Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
> 64 GPIOs, divided over two banks. Each bank has a set of registers for
> 32 GPIOs, with support for edge-triggered interrupts.
>
> Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
> registers pack one bit per GPIO, except for the IMR register, which
> packs two bits per GPIO (AB-CD).
>
> Although the byte order is currently assumed to have port A..D at offset
> 0x0..0x3, this has been observed to be reversed on other, Lexra-based,
> SoCs (e.g. RTL8196E/97D/97F).
>
> Interrupt support is disabled for the fallback devicetree-compatible
> 'realtek,otto-gpio'. This allows for quick support of GPIO banks in
> which the byte order would be unknown. In this case, the port ordering
> in the IMR registers may not match the reversed order in the other
> registers (DCBA, and BA-DC or DC-BA).

I have few comments.

> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/Kconfig             |  12 ++
>  drivers/gpio/Makefile            |   1 +
>  drivers/gpio/gpio-realtek-otto.c | 331 +++++++++++++++++++++++++++++++
>  3 files changed, 344 insertions(+)
>  create mode 100644 drivers/gpio/gpio-realtek-otto.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e3607ec4c2e8..fedf1e49469e 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -502,6 +502,18 @@ config GPIO_RDA
>         help
>           Say Y here to support RDA Micro GPIO controller.
>
> +config GPIO_REALTEK_OTTO
> +       bool "Realtek Otto GPIO support"
> +       depends on MACH_REALTEK_RTL

> +       depends on OF_GPIO

Don't see how it's used.

> +       default MACH_REALTEK_RTL
> +       select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP
> +       help
> +         The GPIO controller on the Otto MIPS platform supports up to two
> +         banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
> +         are grouped in four 8-bit wide ports.
> +
>  config GPIO_REG
>         bool
>         help
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c58a90a3c3b1..8ace5934e3c3 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583)          += gpio-rc5t583.o
>  obj-$(CONFIG_GPIO_RCAR)                        += gpio-rcar.o
>  obj-$(CONFIG_GPIO_RDA)                 += gpio-rda.o
>  obj-$(CONFIG_GPIO_RDC321X)             += gpio-rdc321x.o
> +obj-$(CONFIG_GPIO_REALTEK_OTTO)                += gpio-realtek-otto.o
>  obj-$(CONFIG_GPIO_REG)                 += gpio-reg.o
>  obj-$(CONFIG_ARCH_SA1100)              += gpio-sa1100.o
>  obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)       += gpio-sama5d2-piobu.o
> diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
> new file mode 100644
> index 000000000000..818412687346
> --- /dev/null
> +++ b/drivers/gpio/gpio-realtek-otto.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>

> +#include <linux/of_irq.h>

Why?
Perhaps what you need is property.h and mod_devicetable.h. See below.

> +#include <linux/platform_device.h>

> +#include <linux/swab.h>

Not sure why you need this? See below.

> +/*
> + * Total register block size is 0x1C for four ports.
> + * On the RTL8380/RLT8390 platforms port A, B, and C are implemented.
> + * RTL8389 and RTL8328 implement a second bank with ports E, F, G, and H.
> + *
> + * Port information is stored with the first port at offset 0, followed by the
> + * second, etc. Most registers store one bit per GPIO and should be read out in
> + * reversed endian order. The two interrupt mask registers store two bits per
> + * GPIO, and should be manipulated with swahw32, if required.
> + */
> +
> +/*
> + * Pin select: (0) "normal", (1) "dedicate peripheral"
> + * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits
> + * in the peripheral registers.
> + */
> +#define REALTEK_GPIO_REG_CNR           0x00
> +/* Clear bit (0) for input, set bit (1) for output */
> +#define REALTEK_GPIO_REG_DIR           0x08
> +#define REALTEK_GPIO_REG_DATA          0x0C
> +/* Read bit for IRQ status, write 1 to clear IRQ */
> +#define REALTEK_GPIO_REG_ISR           0x10
> +/* Two bits per GPIO */
> +#define REALTEK_GPIO_REG_IMR_AB                0x14
> +#define REALTEK_GPIO_REG_IMR_CD                0x18

> +#define REALTEK_GPIO_IMR_LINE_MASK     3

GENMASK() ?

> +#define REALTEK_GPIO_IRQ_EDGE_FALLING  1
> +#define REALTEK_GPIO_IRQ_EDGE_RISING   2
> +#define REALTEK_GPIO_IRQ_EDGE_BOTH     3
> +
> +#define REALTEK_GPIO_MAX               32
> +
> +/*
> + * Realtek GPIO driver data
> + * Because the interrupt mask register (IMR) combines the function of
> + * IRQ type selection and masking, two extra values are stored.
> + * intr_mask is used to mask/unmask the interrupts for certain GPIO,
> + * and intr_type is used to store the selected interrupt types. The
> + * logical AND of these values is written to IMR on changes.
> + *
> + * @dev Parent device
> + * @gc Associated gpio_chip instance
> + * @base Base address of the register block
> + * @lock Lock for accessing the IRQ registers and values
> + * @intr_mask Mask for GPIO interrupts
> + * @intr_type GPIO interrupt type selection
> + */
> +struct realtek_gpio_ctrl {
> +       struct device *dev;
> +       struct gpio_chip gc;
> +       void __iomem *base;
> +       raw_spinlock_t lock;
> +       u32 intr_mask[2];
> +       u32 intr_type[2];
> +};
> +
> +enum realtek_gpio_flags {
> +       GPIO_INTERRUPTS = BIT(0),
> +};
> +
> +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +
> +       return container_of(gc, struct realtek_gpio_ctrl, gc);
> +}
> +
> +static inline u32 realtek_gpio_isr_read(struct realtek_gpio_ctrl *ctrl)
> +{
> +       return swab32(readl(ctrl->base + REALTEK_GPIO_REG_ISR));

Why swab?! How is this supposed to work on BE CPUs?
Ditto for all swabXX() usage.

> +}
> +
> +static inline void realtek_gpio_isr_clear(struct realtek_gpio_ctrl *ctrl,
> +       unsigned int pin_mask)
> +{
> +       writel(swab32(pin_mask), ctrl->base + REALTEK_GPIO_REG_ISR);
> +}
> +
> +static inline void realtek_gpio_update_imr(struct realtek_gpio_ctrl *ctrl,
> +       unsigned int imr_offset, u32 type, u32 mask)
> +{
> +       unsigned int reg;
> +
> +       if (imr_offset == 0)
> +               reg = REALTEK_GPIO_REG_IMR_AB;
> +       else
> +               reg = REALTEK_GPIO_REG_IMR_CD;
> +       writel(swahw32(type & mask), ctrl->base + reg);
> +}
> +
> +/*
> + * Since the IMRs contain two bits per GPIO, only 16 GPIO lines fit in a 32-bit
> + * register.
> + * Use realtek_gpio_pin_to_imr_offset() to select the correct IMR offset, and
> + * realtek_gpio_imr_bits() to put the GPIO line's new value in the right place.
> + */
> +static inline u32 realtek_gpio_pin_to_imr_offset(unsigned int pin)
> +{
> +       return pin/16;

Too few spaces.

> +}
> +
> +static inline u32 realtek_gpio_imr_bits(unsigned int pin, u32 value)
> +{
> +       return ((value & REALTEK_GPIO_IMR_LINE_MASK) << 2*(pin % 16));

Too many parentheses.
Too few spaces.

> +}
> +
> +static void realtek_gpio_irq_ack(struct irq_data *data)
> +{
> +       struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> +       u32 pin = irqd_to_hwirq(data);

irq_hwnum_t (or how is it called? check the return type of
irqd_to_hwirq() function).

> +       realtek_gpio_isr_clear(ctrl, BIT(pin));
> +}
> +
> +static void realtek_gpio_irq_unmask(struct irq_data *data)
> +{
> +       struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> +       unsigned int pin = irqd_to_hwirq(data);
> +       unsigned int imr_offset = realtek_gpio_pin_to_imr_offset(pin);
> +       unsigned long flags;
> +       u32 m;
> +
> +       raw_spin_lock_irqsave(&ctrl->lock, flags);
> +       m = ctrl->intr_mask[imr_offset];
> +       m |= realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
> +       ctrl->intr_mask[imr_offset] = m;
> +       realtek_gpio_update_imr(ctrl, imr_offset, ctrl->intr_type[imr_offset], m);
> +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +}
> +
> +static void realtek_gpio_irq_mask(struct irq_data *data)
> +{
> +       struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> +       unsigned int pin = irqd_to_hwirq(data);
> +       unsigned int imr_offset = realtek_gpio_pin_to_imr_offset(pin);
> +       unsigned long flags;
> +       u32 m;
> +
> +       raw_spin_lock_irqsave(&ctrl->lock, flags);
> +       m = ctrl->intr_mask[imr_offset];
> +       m &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
> +       ctrl->intr_mask[imr_offset] = m;
> +       realtek_gpio_update_imr(ctrl, imr_offset, ctrl->intr_type[imr_offset], m);
> +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +}
> +
> +static int realtek_gpio_irq_set_type(struct irq_data *data,
> +       unsigned int flow_type)
> +{
> +       struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> +       irq_flow_handler_t handler;
> +       unsigned int pin = irqd_to_hwirq(data);
> +       unsigned int imr_offset = realtek_gpio_pin_to_imr_offset(pin);
> +       unsigned long flags;
> +       u32 type, t;
> +
> +       switch (flow_type & IRQ_TYPE_SENSE_MASK) {

> +       case IRQ_TYPE_NONE:
> +               type = 0;
> +               handler = handle_bad_irq;
> +               break;

Why is it here? Make it default like many other GPIO drivers do.

> +       case IRQ_TYPE_EDGE_FALLING:
> +               type = REALTEK_GPIO_IRQ_EDGE_FALLING;
> +               handler = handle_edge_irq;
> +               break;
> +       case IRQ_TYPE_EDGE_RISING:
> +               type = REALTEK_GPIO_IRQ_EDGE_RISING;
> +               handler = handle_edge_irq;
> +               break;
> +       case IRQ_TYPE_EDGE_BOTH:
> +               type = REALTEK_GPIO_IRQ_EDGE_BOTH;
> +               handler = handle_edge_irq;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       irq_set_handler_locked(data, handler);

handler is always the same. Use it directly here.

> +       raw_spin_lock_irqsave(&ctrl->lock, flags);
> +       t = ctrl->intr_type[imr_offset];
> +       t &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
> +       t |= realtek_gpio_imr_bits(pin, type);
> +       ctrl->intr_type[imr_offset] = t;
> +       realtek_gpio_update_imr(ctrl, imr_offset, t, ctrl->intr_mask[imr_offset]);
> +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +       return 0;
> +}
> +
> +static void realtek_gpio_irq_handler(struct irq_desc *desc)
> +{
> +       struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> +       struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +       struct irq_chip *irq_chip = irq_desc_get_chip(desc);
> +       int offset;
> +       unsigned long status;
> +
> +       chained_irq_enter(irq_chip, desc);
> +
> +       status = realtek_gpio_isr_read(ctrl);
> +       for_each_set_bit(offset, &status, gc->ngpio) {

> +               dev_dbg(ctrl->dev, "gpio irq %d\n", offset);

Why? Drop this development-phase line.

> +               generic_handle_irq(irq_find_mapping(gc->irq.domain, offset));
> +               realtek_gpio_isr_clear(ctrl, BIT(offset));
> +       }
> +
> +       chained_irq_exit(irq_chip, desc);
> +}
> +
> +static struct irq_chip realtek_gpio_irq_chip = {
> +       .name = "realtek-otto-gpio",
> +       .irq_ack = realtek_gpio_irq_ack,
> +       .irq_mask = realtek_gpio_irq_mask,
> +       .irq_unmask = realtek_gpio_irq_unmask,

> +       .irq_set_type = realtek_gpio_irq_set_type

+ comma

> +};
> +
> +static const struct of_device_id realtek_gpio_of_match[] = {
> +       { .compatible = "realtek,otto-gpio" },
> +       {
> +               .compatible = "realtek,rtl8380-gpio",
> +               .data = (void *)GPIO_INTERRUPTS
> +       },
> +       {
> +               .compatible = "realtek,rtl8390-gpio",
> +               .data = (void *)GPIO_INTERRUPTS
> +       },

> +       {},

Drop comma. They are not needed for terminator lines.

> +};
> +
> +MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
> +
> +static int realtek_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct gpio_irq_chip *girq;
> +       struct realtek_gpio_ctrl *ctrl;
> +       const struct of_device_id *match;
> +       unsigned int match_flags;
> +       u32 ngpios;
> +       int err, irq;
> +
> +       ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +       if (!ctrl)
> +               return -ENOMEM;
> +
> +       ctrl->dev = dev;
> +
> +       if (!np) {
> +               dev_err(&pdev->dev, "no DT node found\n");
> +               return -EINVAL;
> +       }
> +

> +       match = of_match_node(realtek_gpio_of_match, np);
> +       match_flags = (unsigned int) match->data;

device_get_match_data();

> +       if (of_property_read_u32(np, "ngpios", &ngpios) != 0)

' != 0' can be replaced by ''

device_property_read_u32()

> +               ngpios = REALTEK_GPIO_MAX;
> +
> +       if (ngpios > REALTEK_GPIO_MAX) {
> +               dev_err(&pdev->dev, "invalid ngpios (max. %d)\n",
> +                       REALTEK_GPIO_MAX);
> +               return -EINVAL;
> +       }
> +
> +       ctrl->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(ctrl->base))
> +               return PTR_ERR(ctrl->base);
> +
> +       raw_spin_lock_init(&ctrl->lock);
> +
> +       err = bgpio_init(&ctrl->gc, dev, 4,
> +               ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL,
> +               ctrl->base + REALTEK_GPIO_REG_DIR, NULL,
> +               BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> +       if (err) {
> +               dev_err(dev, "unable to init generic GPIO");
> +               return err;
> +       }
> +
> +       ctrl->gc.ngpio = ngpios;
> +       ctrl->gc.owner = THIS_MODULE;
> +
> +       irq = of_irq_get(np, 0);

Why not platform_get_irq_optional() ?

> +       if ((match_flags & GPIO_INTERRUPTS) && irq > 0) {
> +               girq = &ctrl->gc.irq;
> +               girq->chip = &realtek_gpio_irq_chip;
> +               girq->parent_handler = realtek_gpio_irq_handler;
> +               girq->num_parents = 1;
> +               girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),

1 -> girq->num_parent

> +                                       GFP_KERNEL);
> +               if (!girq->parents)
> +                       return -ENOMEM;
> +               girq->default_type = IRQ_TYPE_NONE;
> +               girq->handler = handle_bad_irq;
> +               girq->parents[0] = irq;
> +
> +               /* Disable and clear all interrupts */
> +               realtek_gpio_update_imr(ctrl, 0, 0, 0);
> +               realtek_gpio_update_imr(ctrl, 1, 0, 0);
> +               realtek_gpio_isr_clear(ctrl, BIT(ngpios)-1);
> +       }

> +       err = gpiochip_add_data(&ctrl->gc, ctrl);
> +       return err;

Fold it.

> +}
> +
> +static struct platform_driver realtek_gpio_driver = {
> +       .driver = {
> +               .name = "realtek-otto-gpio",
> +               .of_match_table = realtek_gpio_of_match,
> +       },
> +       .probe = realtek_gpio_probe,
> +};
> +
> +builtin_platform_driver(realtek_gpio_driver);
> +
> +MODULE_DESCRIPTION("Realtek Otto GPIO support");
> +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.30.2
>


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-17 13:08     ` Andy Shevchenko
@ 2021-03-19 15:51       ` Sander Vanheule
  2021-03-19 17:57         ` Andy Shevchenko
  0 siblings, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-19 15:51 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, devicetree, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	Linux Kernel Mailing List, Bert Vermeulen

Hi Andy,

Thanks for the review. I'll address the style comments in a v3. Some
further comments and discussion below.


On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote:
> On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule < 
> sander@svanheule.net> wrote:
> > +       depends on OF_GPIO
> 
> Don't see how it's used.

It isn't, so I'll remove it.


> > +#include <linux/of_irq.h>
> 
> Why?
> Perhaps what you need is property.h and mod_devicetable.h. See below.

With you suggestions, I was able to drop most explicit OF references.
Only of_device_id remains, for which I'll include mod_devicetable.h.


> > +#include <linux/swab.h>
> 
> Not sure why you need this? See below.

[snip]

> 
> > +
> > +static inline u32 realtek_gpio_isr_read(struct realtek_gpio_ctrl
> > *ctrl)
> > +{
> > +       return swab32(readl(ctrl->base + REALTEK_GPIO_REG_ISR));
> 
> Why swab?! How is this supposed to work on BE CPUs?
> Ditto for all swabXX() usage.

My use of swab32/swahw32 has little to do with the CPU being BE or LE,
but more with the register packing in the GPIO peripheral.

The supported SoCs have port layout A-B-C-D in the registers, where
firmware built with Realtek's SDK always denotes A0 as the first GPIO
line. So bit 24 in a register has the value for A0 (with the exception
of the IMR register).

I wrote these wrapper functions to be able to use the BIT() macro with
the GPIO line number, similar to how gpio-mmio uses ioread32be() when
the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used.

For the IMR register, port A again comes first, but is now 16 bits wide
instead of 8, with A0 at bits 16:17. That's why swahw32 is used for
this register.

On the currently unsupported RTL9300-series, the port layout is
reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping
functions won't be required. When support for this alternate port
layout is added, some code will need to be added to differentiate
between the two cases.


> > +}
> > +
> > +static inline void realtek_gpio_isr_clear(struct realtek_gpio_ctrl
> > *ctrl,
> > +       unsigned int pin_mask)
> > +{
> > +       writel(swab32(pin_mask), ctrl->base +
> > REALTEK_GPIO_REG_ISR);
> > +}
> > +
> > +static inline void realtek_gpio_update_imr(struct
> > realtek_gpio_ctrl *ctrl,
> > +       unsigned int imr_offset, u32 type, u32 mask)
> > +{
> > +       unsigned int reg;
> > +
> > +       if (imr_offset == 0)
> > +               reg = REALTEK_GPIO_REG_IMR_AB;
> > +       else
> > +               reg = REALTEK_GPIO_REG_IMR_CD;
> > +       writel(swahw32(type & mask), ctrl->base + reg);
> > +}

[snip]

> > +       switch (flow_type & IRQ_TYPE_SENSE_MASK) {
> 
> > +       case IRQ_TYPE_NONE:
> > +               type = 0;
> > +               handler = handle_bad_irq;
> > +               break;
> 
> Why is it here? Make it default like many other GPIO drivers do.
> 
> > +       case IRQ_TYPE_EDGE_FALLING:
> > +               type = REALTEK_GPIO_IRQ_EDGE_FALLING;
> > +               handler = handle_edge_irq;
> > +               break;
> > +       case IRQ_TYPE_EDGE_RISING:
> > +               type = REALTEK_GPIO_IRQ_EDGE_RISING;
> > +               handler = handle_edge_irq;
> > +               break;
> > +       case IRQ_TYPE_EDGE_BOTH:
> > +               type = REALTEK_GPIO_IRQ_EDGE_BOTH;
> > +               handler = handle_edge_irq;
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       irq_set_handler_locked(data, handler);
> 
> handler is always the same. Use it directly here.

I'll drop the IRQ_TYPE_NONE case. Do I understand it correctly, that
IRQ_TYPE_NONE should never be used as the new value, but only as the
default initial value?


Best,
Sander





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

* Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-19 15:51       ` Sander Vanheule
@ 2021-03-19 17:57         ` Andy Shevchenko
  2021-03-19 21:20           ` Sander Vanheule
  0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2021-03-19 17:57 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: open list:GPIO SUBSYSTEM, devicetree, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	Linux Kernel Mailing List, Bert Vermeulen

On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule <sander@svanheule.net> wrote:
> On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote:
> > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule <
> > sander@svanheule.net> wrote:

...

> > > +#include <linux/swab.h>
> >
> > Not sure why you need this? See below.

> > > +       return swab32(readl(ctrl->base + REALTEK_GPIO_REG_ISR));
> >
> > Why swab?! How is this supposed to work on BE CPUs?
> > Ditto for all swabXX() usage.
>
> My use of swab32/swahw32 has little to do with the CPU being BE or LE,
> but more with the register packing in the GPIO peripheral.
>
> The supported SoCs have port layout A-B-C-D in the registers, where
> firmware built with Realtek's SDK always denotes A0 as the first GPIO
> line. So bit 24 in a register has the value for A0 (with the exception
> of the IMR register).
>
> I wrote these wrapper functions to be able to use the BIT() macro with
> the GPIO line number, similar to how gpio-mmio uses ioread32be() when
> the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used.
>
> For the IMR register, port A again comes first, but is now 16 bits wide
> instead of 8, with A0 at bits 16:17. That's why swahw32 is used for
> this register.
>
> On the currently unsupported RTL9300-series, the port layout is
> reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping
> functions won't be required. When support for this alternate port
> layout is added, some code will need to be added to differentiate
> between the two cases.

Yes, you have different endianess on the hardware level, why not to
use the proper accessors (with or without utilization of the above
mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)?

...

> > > +       case IRQ_TYPE_NONE:
> > > +               type = 0;
> > > +               handler = handle_bad_irq;
> > > +               break;
> >
> > Why is it here? Make it default like many other GPIO drivers do.

> > > +       irq_set_handler_locked(data, handler);
> >
> > handler is always the same. Use it directly here.
>
> I'll drop the IRQ_TYPE_NONE case. Do I understand it correctly, that
> IRQ_TYPE_NONE should never be used as the new value, but only as the
> default initial value?

Initially you initialize the default handler to be "bad" (in order to
easily catch up issues with IRQ configurations).
When ->irq_set_type() is called, if everything is okay it will lock
the handler to the proper one.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-19 17:57         ` Andy Shevchenko
@ 2021-03-19 21:20           ` Sander Vanheule
  2021-03-19 21:24             ` Andy Shevchenko
  0 siblings, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-19 21:20 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, devicetree, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	Linux Kernel Mailing List, Bert Vermeulen

On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote:
> On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule
> <sander@svanheule.net> wrote:
> > On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule <
> > > sander@svanheule.net> wrote:
> 
> ...
> 
> > > > +#include <linux/swab.h>
> > > 
> > > Not sure why you need this? See below.
> 
> > > > +       return swab32(readl(ctrl->base +
> > > > REALTEK_GPIO_REG_ISR));
> > > 
> > > Why swab?! How is this supposed to work on BE CPUs?
> > > Ditto for all swabXX() usage.
> > 
> > My use of swab32/swahw32 has little to do with the CPU being BE or
> > LE,
> > but more with the register packing in the GPIO peripheral.
> > 
> > The supported SoCs have port layout A-B-C-D in the registers, where
> > firmware built with Realtek's SDK always denotes A0 as the first
> > GPIO
> > line. So bit 24 in a register has the value for A0 (with the
> > exception
> > of the IMR register).
> > 
> > I wrote these wrapper functions to be able to use the BIT() macro
> > with
> > the GPIO line number, similar to how gpio-mmio uses ioread32be()
> > when
> > the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used.
> > 
> > For the IMR register, port A again comes first, but is now 16 bits
> > wide
> > instead of 8, with A0 at bits 16:17. That's why swahw32 is used for
> > this register.
> > 
> > On the currently unsupported RTL9300-series, the port layout is
> > reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping
> > functions won't be required. When support for this alternate port
> > layout is added, some code will need to be added to differentiate
> > between the two cases.
> 
> Yes, you have different endianess on the hardware level, why not to
> use the proper accessors (with or without utilization of the above
> mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)?

The point I was trying to make, is that it isn't an endianess issue. I
shouldn't have used a register with single byte values to try to
illustrate that.

Consider instead the interrupt masking registers. To write the IMR bits
for port A (GPIO 0-7), a 16-bit value must be written. This value (e.g.
u16 port_a_imr) is always BE, independent of the packing order of the
ports in the registers:

   // On RTL8380: port A is in the upper word
   writew(port_a_imr, base + OFFSET_IMR_AB);
   
   // On RTL9300: port A is in the lower word
   writew(port_a_imr, base + OFFSET_IMR_AB + 2);

I want the low GPIO lines to be in the lower half-word, so I can
manipulate GPIO lines 0-15 with simple mask and shift operations.

It just so happens, that all registers needed by bgpio_init contain
single-byte values. With BGPIO_BIG_ENDIAN_BYTE_ORDER  the port order is
reversed as required, but it's a bit of a misnomer here.


Best,
Sander


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

* Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-19 21:20           ` Sander Vanheule
@ 2021-03-19 21:24             ` Andy Shevchenko
  2021-03-19 21:48               ` Sander Vanheule
  0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2021-03-19 21:24 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: open list:GPIO SUBSYSTEM, devicetree, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	Linux Kernel Mailing List, Bert Vermeulen

On Fri, Mar 19, 2021 at 11:20 PM Sander Vanheule <sander@svanheule.net> wrote:
> On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote:
> > On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule
> > <sander@svanheule.net> wrote:
> > > On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote:
> > > > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule <
> > > > sander@svanheule.net> wrote:

...

> > > > > +       return swab32(readl(ctrl->base +
> > > > > REALTEK_GPIO_REG_ISR));
> > > >
> > > > Why swab?! How is this supposed to work on BE CPUs?
> > > > Ditto for all swabXX() usage.
> > >
> > > My use of swab32/swahw32 has little to do with the CPU being BE or
> > > LE,
> > > but more with the register packing in the GPIO peripheral.
> > >
> > > The supported SoCs have port layout A-B-C-D in the registers, where
> > > firmware built with Realtek's SDK always denotes A0 as the first
> > > GPIO
> > > line. So bit 24 in a register has the value for A0 (with the
> > > exception
> > > of the IMR register).
> > >
> > > I wrote these wrapper functions to be able to use the BIT() macro
> > > with
> > > the GPIO line number, similar to how gpio-mmio uses ioread32be()
> > > when
> > > the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used.
> > >
> > > For the IMR register, port A again comes first, but is now 16 bits
> > > wide
> > > instead of 8, with A0 at bits 16:17. That's why swahw32 is used for
> > > this register.
> > >
> > > On the currently unsupported RTL9300-series, the port layout is
> > > reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping
> > > functions won't be required. When support for this alternate port
> > > layout is added, some code will need to be added to differentiate
> > > between the two cases.
> >
> > Yes, you have different endianess on the hardware level, why not to
> > use the proper accessors (with or without utilization of the above
> > mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)?
>
> The point I was trying to make, is that it isn't an endianess issue. I
> shouldn't have used a register with single byte values to try to
> illustrate that.
>
> Consider instead the interrupt masking registers. To write the IMR bits
> for port A (GPIO 0-7), a 16-bit value must be written. This value (e.g.
> u16 port_a_imr) is always BE, independent of the packing order of the
> ports in the registers:
>
>    // On RTL8380: port A is in the upper word
>    writew(port_a_imr, base + OFFSET_IMR_AB);
>
>    // On RTL9300: port A is in the lower word
>    writew(port_a_imr, base + OFFSET_IMR_AB + 2);
>
> I want the low GPIO lines to be in the lower half-word, so I can
> manipulate GPIO lines 0-15 with simple mask and shift operations.
>
> It just so happens, that all registers needed by bgpio_init contain
> single-byte values. With BGPIO_BIG_ENDIAN_BYTE_ORDER  the port order is
> reversed as required, but it's a bit of a misnomer here.

How many registers (per GPIO / port) do you have?
Can you list them and show endianess of the data for each of them and
for old and new hardware (something like a 3 column table)?

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-19 21:24             ` Andy Shevchenko
@ 2021-03-19 21:48               ` Sander Vanheule
  2021-03-22 13:16                 ` Andy Shevchenko
  0 siblings, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-19 21:48 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, devicetree, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	Linux Kernel Mailing List, Bert Vermeulen

On Fri, 2021-03-19 at 23:24 +0200, Andy Shevchenko wrote:
> On Fri, Mar 19, 2021 at 11:20 PM Sander Vanheule <
> sander@svanheule.net> wrote:
> > On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote:
> > > On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule
> > > <sander@svanheule.net> wrote:
> > > > On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote:
> > > > > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule <
> > > > > sander@svanheule.net> wrote:
> 
> ...
> 
> > > > > > +       return swab32(readl(ctrl->base +
> > > > > > REALTEK_GPIO_REG_ISR));
> > > > > 
> > > > > Why swab?! How is this supposed to work on BE CPUs?
> > > > > Ditto for all swabXX() usage.
> > > > 
> > > > My use of swab32/swahw32 has little to do with the CPU being BE
> > > > or
> > > > LE,
> > > > but more with the register packing in the GPIO peripheral.
> > > > 
> > > > The supported SoCs have port layout A-B-C-D in the registers,
> > > > where
> > > > firmware built with Realtek's SDK always denotes A0 as the first
> > > > GPIO
> > > > line. So bit 24 in a register has the value for A0 (with the
> > > > exception
> > > > of the IMR register).
> > > > 
> > > > I wrote these wrapper functions to be able to use the BIT() macro
> > > > with
> > > > the GPIO line number, similar to how gpio-mmio uses ioread32be()
> > > > when
> > > > the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used.
> > > > 
> > > > For the IMR register, port A again comes first, but is now 16
> > > > bits
> > > > wide
> > > > instead of 8, with A0 at bits 16:17. That's why swahw32 is used
> > > > for
> > > > this register.
> > > > 
> > > > On the currently unsupported RTL9300-series, the port layout is
> > > > reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping
> > > > functions won't be required. When support for this alternate port
> > > > layout is added, some code will need to be added to differentiate
> > > > between the two cases.
> > > 
> > > Yes, you have different endianess on the hardware level, why not to
> > > use the proper accessors (with or without utilization of the above
> > > mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)?
> > 
> > The point I was trying to make, is that it isn't an endianess issue.
> > I
> > shouldn't have used a register with single byte values to try to
> > illustrate that.
> > 
> > Consider instead the interrupt masking registers. To write the IMR
> > bits
> > for port A (GPIO 0-7), a 16-bit value must be written. This value
> > (e.g.
> > u16 port_a_imr) is always BE, independent of the packing order of the
> > ports in the registers:
> > 
> >    // On RTL8380: port A is in the upper word
> >    writew(port_a_imr, base + OFFSET_IMR_AB);
> > 
> >    // On RTL9300: port A is in the lower word
> >    writew(port_a_imr, base + OFFSET_IMR_AB + 2);
> > 
> > I want the low GPIO lines to be in the lower half-word, so I can
> > manipulate GPIO lines 0-15 with simple mask and shift operations.
> > 
> > It just so happens, that all registers needed by bgpio_init contain
> > single-byte values. With BGPIO_BIG_ENDIAN_BYTE_ORDER  the port order
> > is
> > reversed as required, but it's a bit of a misnomer here.
> 
> How many registers (per GPIO / port) do you have?
> Can you list them and show endianess of the data for each of them and
> for old and new hardware (something like a 3 column table)?

Each GPIO bank, with 32 GPIO lines, consists of four 8-line ports.
There are seven registers per port, but only five are used:

       |        | Data    | RTL8380    | RTL9300 
Reg    | Offset | type    | byte order | byte order
-------+--------+---------+------------+-----------
DIR    | 0x08   | 4 * u8  | A-B-C-D    | D-C-B-A
DATA   | 0x0C   | 4 * u8  | A-B-C-D    | D-C-B-A
ISR    | 0x10   | 4 * u8  | A-B-C-D    | D-C-B-A
IMR_AB | 0x14   | 2 * u16 | A-A-B-B    | B-B-A-A
IMR_CD | 0x18   | 2 * u16 | C-C-D-D    | D-D-C-C

The unused other registers are all 4*u8.

A-B-C-D means:  (A << 24) | (B << 16) | (C << 8) | D
A-A-B-B means:  (A << 16) | B

--
Best,
Sander


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

* Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-19 21:48               ` Sander Vanheule
@ 2021-03-22 13:16                 ` Andy Shevchenko
  0 siblings, 0 replies; 39+ messages in thread
From: Andy Shevchenko @ 2021-03-22 13:16 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: open list:GPIO SUBSYSTEM, devicetree, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Thomas Gleixner, Marc Zyngier,
	Linux Kernel Mailing List, Bert Vermeulen

On Fri, Mar 19, 2021 at 11:48 PM Sander Vanheule <sander@svanheule.net> wrote:
> On Fri, 2021-03-19 at 23:24 +0200, Andy Shevchenko wrote:
> > On Fri, Mar 19, 2021 at 11:20 PM Sander Vanheule <
> > sander@svanheule.net> wrote:
> > > On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote:

...

> > > The point I was trying to make, is that it isn't an endianess issue.
> > > I
> > > shouldn't have used a register with single byte values to try to
> > > illustrate that.
> > >
> > > Consider instead the interrupt masking registers. To write the IMR
> > > bits
> > > for port A (GPIO 0-7), a 16-bit value must be written. This value
> > > (e.g.
> > > u16 port_a_imr) is always BE, independent of the packing order of the
> > > ports in the registers:
> > >
> > >    // On RTL8380: port A is in the upper word
> > >    writew(port_a_imr, base + OFFSET_IMR_AB);
> > >
> > >    // On RTL9300: port A is in the lower word
> > >    writew(port_a_imr, base + OFFSET_IMR_AB + 2);
> > >
> > > I want the low GPIO lines to be in the lower half-word, so I can
> > > manipulate GPIO lines 0-15 with simple mask and shift operations.
> > >
> > > It just so happens, that all registers needed by bgpio_init contain
> > > single-byte values. With BGPIO_BIG_ENDIAN_BYTE_ORDER  the port order
> > > is
> > > reversed as required, but it's a bit of a misnomer here.
> >
> > How many registers (per GPIO / port) do you have?
> > Can you list them and show endianess of the data for each of them and
> > for old and new hardware (something like a 3 column table)?
>
> Each GPIO bank, with 32 GPIO lines, consists of four 8-line ports.
> There are seven registers per port, but only five are used:
>
>        |        | Data    | RTL8380    | RTL9300
> Reg    | Offset | type    | byte order | byte order
> -------+--------+---------+------------+-----------
> DIR    | 0x08   | 4 * u8  | A-B-C-D    | D-C-B-A
> DATA   | 0x0C   | 4 * u8  | A-B-C-D    | D-C-B-A
> ISR    | 0x10   | 4 * u8  | A-B-C-D    | D-C-B-A
> IMR_AB | 0x14   | 2 * u16 | A-A-B-B    | B-B-A-A
> IMR_CD | 0x18   | 2 * u16 | C-C-D-D    | D-D-C-C
>
> The unused other registers are all 4*u8.

You mean that they are following the same rules as DIR/DATA/ISR. right?

> A-B-C-D means:  (A << 24) | (B << 16) | (C << 8) | D
> A-A-B-B means:  (A << 16) | B

If the above is true for unused registers, it's clearly hardware endianness.

You need special treatment for IMR, but in general it follows the
logic behind the others.

So, you need some kind of I/O accessors like
 read_u8_reg()
 write_u8_reg()
 read_u16_reg()
 write_u16_reg()

And depending on endianess of hardware to call proper set of them.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO
  2021-03-15 19:08   ` [PATCH v2 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
@ 2021-03-24 18:29     ` Rob Herring
  0 siblings, 0 replies; 39+ messages in thread
From: Rob Herring @ 2021-03-24 18:29 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: linux-gpio, devicetree, Linus Walleij, Bartosz Golaszewski,
	Thomas Gleixner, Marc Zyngier, linux-kernel, bert

On Mon, Mar 15, 2021 at 08:08:05PM +0100, Sander Vanheule wrote:
> Add a binding description for Realtek's GPIO controller found on several
> of their MIPS-based SoCs (codenamed Otto), such as the RTL838x and
> RTL839x series of switch SoCs.
> 
> A fallback binding 'realtek,otto-gpio' is provided for cases where the
> actual port ordering is not known yet, and enabling the interrupt
> controller may result in uncaught interrupts.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../bindings/gpio/gpio-realtek-otto.yaml      | 80 +++++++++++++++++++

Use compatible string: realtek,otto-gpio.yaml

>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml b/Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml
> new file mode 100644
> index 000000000000..3e8151e3a169
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-realtek-otto.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-realtek-otto.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek Otto GPIO controller
> +
> +maintainers:
> +  - Sander Vanheule <sander@svanheule.net>
> +  - Bert Vermeulen <bert@biot.com>
> +
> +description: |
> +  Realtek's GPIO controller on their MIPS switch SoCs (Otto platform) consists
> +  of two banks of 32 GPIOs. These GPIOs can generate edge-triggered interrupts.
> +  Each bank's interrupts are cascased into one interrupt line on the parent
> +  interrupt controller, if provided.
> +  This binding allows defining a single bank in the devicetree. The interrupt
> +  controller is not supported on the fallback compatible name, which only
> +  allows for GPIO port use.
> +
> +properties:
> +  $nodename:
> +    pattern: "^gpio@[0-9a-f]+$"
> +
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - realtek,rtl8380-gpio
> +              - realtek,rtl8390-gpio
> +          - const: realtek,otto-gpio
> +      - const: realtek,otto-gpio

This should be dropped. You should always have an SoC specific 
compatible.

> +
> +  reg:
> +    maxItems: 1
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  ngpios:
> +    minimum: 1
> +    maximum: 32
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#gpio-cells"
> +  - gpio-controller
> +
> +additionalProperties: false
> +
> +dependencies:
> +  interrupt-controller: [ interrupts ]
> +
> +examples:
> +  - |
> +      gpio@3500 {
> +        compatible = "realtek,rtl8380-gpio", "realtek,otto-gpio";
> +        reg = <0x3500 0x1c>;
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        ngpios = <24>;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupt-parent = <&rtlintc>;
> +        interrupts = <23>;
> +      };
> +
> +...
> -- 
> 2.30.2
> 

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

* [PATCH v3 0/2] Add Realtek Otto GPIO support
  2021-03-15  8:23 [PATCH 0/2] Add Realtek Otto GPIO support Sander Vanheule
                   ` (2 preceding siblings ...)
  2021-03-15 19:08 ` [PATCH v2 0/2] " Sander Vanheule
@ 2021-03-24 21:22 ` Sander Vanheule
  2021-03-24 21:22   ` [PATCH v3 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
  2021-03-24 21:22   ` [PATCH v3 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
  2021-03-26 12:03 ` [PATCH v4 0/2] " Sander Vanheule
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-24 21:22 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: bert, bgolaszewski, linus.walleij, linux-kernel, maz, robh+dt,
	tglx, Sander Vanheule

Add support for the GPIO controller employed by Realtek in multiple series of
MIPS SoCs. These include the supported RTL838x and RTL839x. The register layout
also matches the one found in the GPIO controller of other (Lexra-based) SoCs
such as RTL8196E, RTL8197D, and RTL8197F.

For the platform name 'otto', I am not aware of any official resources as to
what hardware this specifically applies to. However, in all of the GPL archives
we've received, from vendors using compatible SoCs in their design, the
platform under the MIPS architecture is referred to by this name.

The GPIO ports have been tested on a Zyxel GS1900-8 (RTL8380), and Zyxel
GS1900-48 (RTL8393). Furthermore, the GPIO ports and interrupt controller have
been tested on a Netgear GS110TPPv1 (RTL8381).

Changes in v3:
- Remove OF dependencies in driver probe
- Don't accept IRQ_TYPE_NONE as a valid interrupt type
- Remove (now unused) dev property from control structure
- Use u8/u16 port registers, instead of raw u32 registers
- Use 'line' name for gpiochip, 'port' and 'pin' names for hardware
- Renamed DT bindings file
- Dropped fallback-only DT compatible
- Various code style clean-ups

Changes in v2:
- Clarify structure and usage of IMR registers
- Added Linus' Reviewed-by tags

Sander Vanheule (2):
  dt-bindings: gpio: Binding for Realtek Otto GPIO
  gpio: Add Realtek Otto GPIO support

 .../bindings/gpio/realtek,otto-gpio.yaml      |  78 ++++
 drivers/gpio/Kconfig                          |  11 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-realtek-otto.c              | 335 ++++++++++++++++++
 4 files changed, 425 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

-- 
2.30.2


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

* [PATCH v3 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO
  2021-03-24 21:22 ` [PATCH v3 0/2] " Sander Vanheule
@ 2021-03-24 21:22   ` Sander Vanheule
  2021-03-24 21:22   ` [PATCH v3 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
  1 sibling, 0 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-24 21:22 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: bert, bgolaszewski, linus.walleij, linux-kernel, maz, robh+dt,
	tglx, Sander Vanheule

Add a binding description for Realtek's GPIO controller found on several
of their MIPS-based SoCs (codenamed Otto), such as the RTL838x and
RTL839x series of switch SoCs.

A fallback binding 'realtek,otto-gpio' is provided for cases where the
actual port ordering is not known yet, and enabling the interrupt
controller may result in uncaught interrupts.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../bindings/gpio/realtek,otto-gpio.yaml      | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml b/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
new file mode 100644
index 000000000000..100f20cebd76
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/realtek,otto-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek Otto GPIO controller
+
+maintainers:
+  - Sander Vanheule <sander@svanheule.net>
+  - Bert Vermeulen <bert@biot.com>
+
+description: |
+  Realtek's GPIO controller on their MIPS switch SoCs (Otto platform) consists
+  of two banks of 32 GPIOs. These GPIOs can generate edge-triggered interrupts.
+  Each bank's interrupts are cascased into one interrupt line on the parent
+  interrupt controller, if provided.
+  This binding allows defining a single bank in the devicetree. The interrupt
+  controller is not supported on the fallback compatible name, which only
+  allows for GPIO port use.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    items:
+      - enum:
+          - realtek,rtl8380-gpio
+          - realtek,rtl8390-gpio
+      - const: realtek,otto-gpio
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  ngpios:
+    minimum: 1
+    maximum: 32
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+dependencies:
+  interrupt-controller: [ interrupts ]
+
+examples:
+  - |
+      gpio@3500 {
+        compatible = "realtek,rtl8380-gpio", "realtek,otto-gpio";
+        reg = <0x3500 0x1c>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        ngpios = <24>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&rtlintc>;
+        interrupts = <23>;
+      };
+
+...
-- 
2.30.2


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

* [PATCH v3 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-24 21:22 ` [PATCH v3 0/2] " Sander Vanheule
  2021-03-24 21:22   ` [PATCH v3 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
@ 2021-03-24 21:22   ` Sander Vanheule
  2021-03-24 21:29     ` Sander Vanheule
  1 sibling, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-24 21:22 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: bert, bgolaszewski, linus.walleij, linux-kernel, maz, robh+dt,
	tglx, Sander Vanheule

Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
64 GPIOs, divided over two banks. Each bank has a set of registers for
32 GPIOs, with support for edge-triggered interrupts.

Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
registers pack one bit per GPIO, except for the IMR register, which
packs two bits per GPIO (AB-CD).

Although the byte order is currently assumed to have port A..D at offset
0x0..0x3, this has been observed to be reversed on other, Lexra-based,
SoCs (e.g. RTL8196E/97D/97F).

Interrupt support is disabled for the fallback devicetree-compatible
'realtek,otto-gpio'. This allows for quick support of GPIO banks in
which the byte order would be unknown. In this case, the port ordering
in the IMR registers may not match the reversed order in the other
registers (DCBA, and BA-DC or DC-BA).

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/Kconfig             |  11 +
 drivers/gpio/Makefile            |   1 +
 drivers/gpio/gpio-realtek-otto.c | 335 +++++++++++++++++++++++++++++++
 3 files changed, 347 insertions(+)
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..d3be17812f94 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -502,6 +502,17 @@ config GPIO_RDA
 	help
 	  Say Y here to support RDA Micro GPIO controller.
 
+config GPIO_REALTEK_OTTO
+	bool "Realtek Otto GPIO support"
+	depends on MACH_REALTEK_RTL
+	default MACH_REALTEK_RTL
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	help
+	  The GPIO controller on the Otto MIPS platform supports up to two
+	  banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
+	  are grouped in four 8-bit wide ports.
+
 config GPIO_REG
 	bool
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..8ace5934e3c3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583)		+= gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RCAR)			+= gpio-rcar.o
 obj-$(CONFIG_GPIO_RDA)			+= gpio-rda.o
 obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
+obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
new file mode 100644
index 000000000000..0714d54e08d1
--- /dev/null
+++ b/drivers/gpio/gpio-realtek-otto.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/gpio/driver.h>
+#include <linux/irq.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/swab.h>
+
+/*
+ * Total register block size is 0x1C for four ports.
+ * On the RTL8380/RLT8390 platforms port A, B, and C are implemented.
+ * RTL8389 and RTL8328 implement a second bank with ports E, F, G, and H.
+ *
+ * Port information is stored with the first port at offset 0, followed by the
+ * second, etc. Most registers store one bit per GPIO and should be read out in
+ * reversed endian order. The two interrupt mask registers store two bits per
+ * GPIO, and should be manipulated with swahw32, if required.
+ */
+
+/*
+ * Pin select: (0) "normal", (1) "dedicate peripheral"
+ * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits
+ * in the peripheral registers.
+ */
+#define REALTEK_GPIO_REG_CNR		0x00
+/* Clear bit (0) for input, set bit (1) for output */
+#define REALTEK_GPIO_REG_DIR		0x08
+#define REALTEK_GPIO_REG_DATA		0x0C
+/* Read bit for IRQ status, write 1 to clear IRQ */
+#define REALTEK_GPIO_REG_ISR		0x10
+/* Two bits per GPIO in IMR registers */
+#define REALTEK_GPIO_REG_IMR		0x14
+#define REALTEK_GPIO_REG_IMR_AB		0x14
+#define REALTEK_GPIO_REG_IMR_CD		0x18
+#define REALTEK_GPIO_IMR_LINE_MASK	GENMASK(1, 0)
+#define REALTEK_GPIO_IRQ_EDGE_FALLING	1
+#define REALTEK_GPIO_IRQ_EDGE_RISING	2
+#define REALTEK_GPIO_IRQ_EDGE_BOTH	3
+
+#define REALTEK_GPIO_MAX		32
+#define REALTEK_GPIO_PORTS_PER_BANK	4
+
+/*
+ * Realtek GPIO driver data
+ * Because the interrupt mask register (IMR) combines the function of
+ * IRQ type selection and masking, two extra values are stored.
+ * intr_mask is used to mask/unmask the interrupts for certain GPIO,
+ * and intr_type is used to store the selected interrupt types. The
+ * logical AND of these values is written to IMR on changes.
+ *
+ * @gc Associated gpio_chip instance
+ * @base Base address of the register block
+ * @lock Lock for accessing the IRQ registers and values
+ * @intr_mask Mask for GPIO interrupts
+ * @intr_type GPIO interrupt type selection
+ */
+struct realtek_gpio_ctrl {
+	struct gpio_chip gc;
+	void __iomem *base;
+	raw_spinlock_t lock;
+	u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK];
+	u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK];
+};
+
+enum realtek_gpio_flags {
+	GPIO_INTERRUPTS = BIT(0),
+};
+
+static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+
+	return container_of(gc, struct realtek_gpio_ctrl, gc);
+}
+
+static inline unsigned int line_to_port(unsigned int line)
+{
+	return line / 8;
+}
+
+static inline unsigned int line_to_port_pin(unsigned int line)
+{
+	return line % 8;
+}
+
+static inline u8 read_u8_reg(void __iomem* reg, unsigned int port)
+{
+	return ioread8(reg + port);
+}
+
+static inline void write_u8_reg(void __iomem* reg, unsigned int port, u8 value)
+{
+	iowrite8(value, reg + port);
+}
+
+static inline u16 read_u16_reg(void __iomem* reg, unsigned int port)
+{
+	return ioread16(reg + 2 * port);
+}
+
+static inline void write_u16_reg(void __iomem* reg, unsigned int port, u16 value)
+{
+	iowrite16(value, reg + 2 * port);
+}
+
+/*
+ * Since the IMRs contain two bits per GPIO, only 16 GPIO lines fit in a 32-bit
+ * register. Use realtek_gpio_imr_bits() to put the GPIO line's new value in
+ * the right place.
+ */
+static inline u16 realtek_gpio_imr_bits(unsigned int pin, u16 value)
+{
+	return (value & REALTEK_GPIO_IMR_LINE_MASK) << 2 * pin;
+}
+
+static inline void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl,
+	unsigned int port, u16 irq_type, u16 irq_mask)
+{
+	write_u16_reg(ctrl->base + REALTEK_GPIO_REG_IMR, port,
+		   irq_type & irq_mask);
+}
+
+static void realtek_gpio_irq_ack(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	irq_hw_number_t line = irqd_to_hwirq(data);
+
+	write_u8_reg(ctrl->base + REALTEK_GPIO_REG_ISR, line_to_port(line),
+		BIT(line_to_port_pin(line)));
+}
+
+static void realtek_gpio_irq_unmask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line_to_port(line);
+	unsigned int pin = line_to_port_pin(line);
+	unsigned long flags;
+	u16 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[port];
+	m |= realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[port] = m;
+	realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static void realtek_gpio_irq_mask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line_to_port(line);
+	unsigned int pin = line_to_port_pin(line);
+	unsigned long flags;
+	u16 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[port];
+	m &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[port] = m;
+	realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static int realtek_gpio_irq_set_type(struct irq_data *data,
+	unsigned int flow_type)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line_to_port(line);
+	unsigned int pin = line_to_port_pin(line);
+	unsigned long flags;
+	u16 type, t;
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_FALLING:
+		type = REALTEK_GPIO_IRQ_EDGE_FALLING;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		type = REALTEK_GPIO_IRQ_EDGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		type = REALTEK_GPIO_IRQ_EDGE_BOTH;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	irq_set_handler_locked(data, handle_edge_irq);
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	t = ctrl->intr_type[port];
+	t &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
+	t |= realtek_gpio_imr_bits(pin, type);
+	ctrl->intr_type[port] = t;
+	realtek_gpio_write_imr(ctrl, port, t, ctrl->intr_mask[port]);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static void realtek_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+	void __iomem *reg_isr = ctrl->base + REALTEK_GPIO_REG_ISR;
+	unsigned int lines_done;
+	unsigned int port_pin_count;
+	unsigned int port;
+	unsigned int irq;
+	int offset;
+	unsigned long status;
+
+	chained_irq_enter(irq_chip, desc);
+
+	for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8) {
+		port = line_to_port(lines_done);
+		status = read_u8_reg(reg_isr, port);
+		port_pin_count = min(gc->ngpio - lines_done, (unsigned int) 8);
+		for_each_set_bit(offset, &status, port_pin_count) {
+			irq = irq_find_mapping(gc->irq.domain, offset);
+			generic_handle_irq(irq);
+			write_u8_reg(reg_isr, port, BIT(offset));
+		}
+	}
+
+	chained_irq_exit(irq_chip, desc);
+}
+
+static struct irq_chip realtek_gpio_irq_chip = {
+	.name = "realtek-otto-gpio",
+	.irq_ack = realtek_gpio_irq_ack,
+	.irq_mask = realtek_gpio_irq_mask,
+	.irq_unmask = realtek_gpio_irq_unmask,
+	.irq_set_type = realtek_gpio_irq_set_type,
+};
+
+static const struct of_device_id realtek_gpio_of_match[] = {
+	{ .compatible = "realtek,otto-gpio" },
+	{
+		.compatible = "realtek,rtl8380-gpio",
+		.data = (void *)GPIO_INTERRUPTS
+	},
+	{
+		.compatible = "realtek,rtl8390-gpio",
+		.data = (void *)GPIO_INTERRUPTS
+	},
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
+
+static int realtek_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int dev_flags;
+	struct gpio_irq_chip *girq;
+	struct realtek_gpio_ctrl *ctrl;
+	u32 ngpios;
+	int err, irq;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	dev_flags = (unsigned int) device_get_match_data(dev);
+
+	if (device_property_read_u32(dev, "ngpios", &ngpios))
+		ngpios = REALTEK_GPIO_MAX;
+
+	if (ngpios > REALTEK_GPIO_MAX) {
+		dev_err(&pdev->dev, "invalid ngpios (max. %d)\n",
+			REALTEK_GPIO_MAX);
+		return -EINVAL;
+	}
+
+	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ctrl->base))
+		return PTR_ERR(ctrl->base);
+
+	raw_spin_lock_init(&ctrl->lock);
+
+	err = bgpio_init(&ctrl->gc, dev, 4,
+		ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL,
+		ctrl->base + REALTEK_GPIO_REG_DIR, NULL,
+		BGPIOF_BIG_ENDIAN_BYTE_ORDER);
+	if (err) {
+		dev_err(dev, "unable to init generic GPIO");
+		return err;
+	}
+
+	ctrl->gc.ngpio = ngpios;
+	ctrl->gc.owner = THIS_MODULE;
+
+	irq = platform_get_irq_optional(pdev, 0);
+	if ((dev_flags & GPIO_INTERRUPTS) && irq > 0) {
+		girq = &ctrl->gc.irq;
+		girq->chip = &realtek_gpio_irq_chip;
+		girq->parent_handler = realtek_gpio_irq_handler;
+		girq->num_parents = 1;
+		girq->parents = devm_kcalloc(dev, girq->num_parents,
+					sizeof(*girq->parents),	GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_bad_irq;
+		girq->parents[0] = irq;
+
+		/* Disable and clear all interrupts */
+		iowrite32(0, ctrl->base + REALTEK_GPIO_REG_IMR_AB);
+		iowrite32(0, ctrl->base + REALTEK_GPIO_REG_IMR_CD);
+		iowrite32(GENMASK(31, 0), ctrl->base + REALTEK_GPIO_REG_ISR);
+	}
+
+	return gpiochip_add_data(&ctrl->gc, ctrl);
+}
+
+static struct platform_driver realtek_gpio_driver = {
+	.driver = {
+		.name = "realtek-otto-gpio",
+		.of_match_table	= realtek_gpio_of_match,
+	},
+	.probe = realtek_gpio_probe,
+};
+
+builtin_platform_driver(realtek_gpio_driver);
+
+MODULE_DESCRIPTION("Realtek Otto GPIO support");
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2


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

* Re: [PATCH v3 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-24 21:22   ` [PATCH v3 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
@ 2021-03-24 21:29     ` Sander Vanheule
  0 siblings, 0 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-24 21:29 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: bert, bgolaszewski, linus.walleij, linux-kernel, maz, robh+dt, tglx

On Wed, 2021-03-24 at 22:22 +0100, Sander Vanheule wrote:
> +static inline u8 read_u8_reg(void __iomem* reg, unsigned int port)
> +{
> +       return ioread8(reg + port);
> +}
> +
> +static inline void write_u8_reg(void __iomem* reg, unsigned int port,
> u8 value)
> +{
> +       iowrite8(value, reg + port);
> +}
> +
> +static inline u16 read_u16_reg(void __iomem* reg, unsigned int port)
> +{
> +       return ioread16(reg + 2 * port);
> +}
> +
> +static inline void write_u16_reg(void __iomem* reg, unsigned int
> port, u16 value)
> +{
> +       iowrite16(value, reg + 2 * port);
> +}

Of course I only noticed this after sending v3, but these functions
should have "void __iomem *reg" instead. I can fix this in a next
version.

Best,
Sander


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

* [PATCH v4 0/2] Add Realtek Otto GPIO support
  2021-03-15  8:23 [PATCH 0/2] Add Realtek Otto GPIO support Sander Vanheule
                   ` (3 preceding siblings ...)
  2021-03-24 21:22 ` [PATCH v3 0/2] " Sander Vanheule
@ 2021-03-26 12:03 ` Sander Vanheule
  2021-03-26 12:03   ` [PATCH v4 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
  2021-03-26 12:03   ` [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
  2021-03-30 15:26 ` [PATCH v5 0/2] " Sander Vanheule
  2021-03-30 17:48 ` [PATCH v6 0/2] " Sander Vanheule
  6 siblings, 2 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-26 12:03 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: bert, bgolaszewski, linus.walleij, linux-kernel, maz, robh+dt,
	tglx, andy.shevchenko, Sander Vanheule

Add support for the GPIO controller employed by Realtek in multiple series of
MIPS SoCs. These include the supported RTL838x and RTL839x. The register layout
also matches the one found in the GPIO controller of other (Lexra-based) SoCs
such as RTL8196E, RTL8197D, and RTL8197F.

For the platform name 'otto', I am not aware of any official resources as to
what hardware this specifically applies to. However, in all of the GPL archives
we've received, from vendors using compatible SoCs in their design, the
platform under the MIPS architecture is referred to by this name.

The GPIO ports have been tested on a Zyxel GS1900-8 (RTL8380), and Zyxel
GS1900-48 (RTL8393). Furthermore, the GPIO ports and interrupt controller have
been tested on a Netgear GS110TPPv1 (RTL8381).

Changes in v4:
- Fix pointer notation style
- Drop unused read_u16_reg() function
- Drop 'inline' specifier from functions

Changes in v3:
- Remove OF dependencies in driver probe
- Don't accept IRQ_TYPE_NONE as a valid interrupt type
- Remove (now unused) dev property from control structure
- Use u8/u16 port registers, instead of raw u32 registers
- Use 'line' name for gpiochip, 'port' and 'pin' names for hardware
- Renamed DT bindings file
- Dropped fallback-only DT compatible
- Various code style clean-ups

Changes in v2:
- Clarify structure and usage of IMR registers
- Added Linus' Reviewed-by tags

Sander Vanheule (2):
  dt-bindings: gpio: Binding for Realtek Otto GPIO
  gpio: Add Realtek Otto GPIO support

 .../bindings/gpio/realtek,otto-gpio.yaml      |  78 +++++
 drivers/gpio/Kconfig                          |  11 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-realtek-otto.c              | 330 ++++++++++++++++++
 4 files changed, 420 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

-- 
2.30.2


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

* [PATCH v4 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO
  2021-03-26 12:03 ` [PATCH v4 0/2] " Sander Vanheule
@ 2021-03-26 12:03   ` Sander Vanheule
  2021-03-27 18:16     ` Rob Herring
  2021-03-26 12:03   ` [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
  1 sibling, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-26 12:03 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: bert, bgolaszewski, linus.walleij, linux-kernel, maz, robh+dt,
	tglx, andy.shevchenko, Sander Vanheule

Add a binding description for Realtek's GPIO controller found on several
of their MIPS-based SoCs (codenamed Otto), such as the RTL838x and
RTL839x series of switch SoCs.

A fallback binding 'realtek,otto-gpio' is provided for cases where the
actual port ordering is not known yet, and enabling the interrupt
controller may result in uncaught interrupts.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../bindings/gpio/realtek,otto-gpio.yaml      | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml b/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
new file mode 100644
index 000000000000..100f20cebd76
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/realtek,otto-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek Otto GPIO controller
+
+maintainers:
+  - Sander Vanheule <sander@svanheule.net>
+  - Bert Vermeulen <bert@biot.com>
+
+description: |
+  Realtek's GPIO controller on their MIPS switch SoCs (Otto platform) consists
+  of two banks of 32 GPIOs. These GPIOs can generate edge-triggered interrupts.
+  Each bank's interrupts are cascased into one interrupt line on the parent
+  interrupt controller, if provided.
+  This binding allows defining a single bank in the devicetree. The interrupt
+  controller is not supported on the fallback compatible name, which only
+  allows for GPIO port use.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    items:
+      - enum:
+          - realtek,rtl8380-gpio
+          - realtek,rtl8390-gpio
+      - const: realtek,otto-gpio
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  ngpios:
+    minimum: 1
+    maximum: 32
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+dependencies:
+  interrupt-controller: [ interrupts ]
+
+examples:
+  - |
+      gpio@3500 {
+        compatible = "realtek,rtl8380-gpio", "realtek,otto-gpio";
+        reg = <0x3500 0x1c>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        ngpios = <24>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&rtlintc>;
+        interrupts = <23>;
+      };
+
+...
-- 
2.30.2


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

* [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-26 12:03 ` [PATCH v4 0/2] " Sander Vanheule
  2021-03-26 12:03   ` [PATCH v4 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
@ 2021-03-26 12:03   ` Sander Vanheule
  2021-03-26 18:19     ` Andy Shevchenko
  1 sibling, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-26 12:03 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: bert, bgolaszewski, linus.walleij, linux-kernel, maz, robh+dt,
	tglx, andy.shevchenko, Sander Vanheule

Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
64 GPIOs, divided over two banks. Each bank has a set of registers for
32 GPIOs, with support for edge-triggered interrupts.

Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
registers pack one bit per GPIO, except for the IMR register, which
packs two bits per GPIO (AB-CD).

Although the byte order is currently assumed to have port A..D at offset
0x0..0x3, this has been observed to be reversed on other, Lexra-based,
SoCs (e.g. RTL8196E/97D/97F).

Interrupt support is disabled for the fallback devicetree-compatible
'realtek,otto-gpio'. This allows for quick support of GPIO banks in
which the byte order would be unknown. In this case, the port ordering
in the IMR registers may not match the reversed order in the other
registers (DCBA, and BA-DC or DC-BA).

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/Kconfig             |  11 ++
 drivers/gpio/Makefile            |   1 +
 drivers/gpio/gpio-realtek-otto.c | 330 +++++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+)
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..d3be17812f94 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -502,6 +502,17 @@ config GPIO_RDA
 	help
 	  Say Y here to support RDA Micro GPIO controller.
 
+config GPIO_REALTEK_OTTO
+	bool "Realtek Otto GPIO support"
+	depends on MACH_REALTEK_RTL
+	default MACH_REALTEK_RTL
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	help
+	  The GPIO controller on the Otto MIPS platform supports up to two
+	  banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
+	  are grouped in four 8-bit wide ports.
+
 config GPIO_REG
 	bool
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..8ace5934e3c3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583)		+= gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RCAR)			+= gpio-rcar.o
 obj-$(CONFIG_GPIO_RDA)			+= gpio-rda.o
 obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
+obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
new file mode 100644
index 000000000000..07641a1686eb
--- /dev/null
+++ b/drivers/gpio/gpio-realtek-otto.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/gpio/driver.h>
+#include <linux/irq.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/swab.h>
+
+/*
+ * Total register block size is 0x1C for four ports.
+ * On the RTL8380/RLT8390 platforms port A, B, and C are implemented.
+ * RTL8389 and RTL8328 implement a second bank with ports E, F, G, and H.
+ *
+ * Port information is stored with the first port at offset 0, followed by the
+ * second, etc. Most registers store one bit per GPIO and should be read out in
+ * reversed endian order. The two interrupt mask registers store two bits per
+ * GPIO, and should be manipulated with swahw32, if required.
+ */
+
+/*
+ * Pin select: (0) "normal", (1) "dedicate peripheral"
+ * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits
+ * in the peripheral registers.
+ */
+#define REALTEK_GPIO_REG_CNR		0x00
+/* Clear bit (0) for input, set bit (1) for output */
+#define REALTEK_GPIO_REG_DIR		0x08
+#define REALTEK_GPIO_REG_DATA		0x0C
+/* Read bit for IRQ status, write 1 to clear IRQ */
+#define REALTEK_GPIO_REG_ISR		0x10
+/* Two bits per GPIO in IMR registers */
+#define REALTEK_GPIO_REG_IMR		0x14
+#define REALTEK_GPIO_REG_IMR_AB		0x14
+#define REALTEK_GPIO_REG_IMR_CD		0x18
+#define REALTEK_GPIO_IMR_LINE_MASK	GENMASK(1, 0)
+#define REALTEK_GPIO_IRQ_EDGE_FALLING	1
+#define REALTEK_GPIO_IRQ_EDGE_RISING	2
+#define REALTEK_GPIO_IRQ_EDGE_BOTH	3
+
+#define REALTEK_GPIO_MAX		32
+#define REALTEK_GPIO_PORTS_PER_BANK	4
+
+/*
+ * Realtek GPIO driver data
+ * Because the interrupt mask register (IMR) combines the function of
+ * IRQ type selection and masking, two extra values are stored.
+ * intr_mask is used to mask/unmask the interrupts for certain GPIO,
+ * and intr_type is used to store the selected interrupt types. The
+ * logical AND of these values is written to IMR on changes.
+ *
+ * @gc Associated gpio_chip instance
+ * @base Base address of the register block
+ * @lock Lock for accessing the IRQ registers and values
+ * @intr_mask Mask for GPIO interrupts
+ * @intr_type GPIO interrupt type selection
+ */
+struct realtek_gpio_ctrl {
+	struct gpio_chip gc;
+	void __iomem *base;
+	raw_spinlock_t lock;
+	u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK];
+	u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK];
+};
+
+enum realtek_gpio_flags {
+	GPIO_INTERRUPTS = BIT(0),
+};
+
+static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+
+	return container_of(gc, struct realtek_gpio_ctrl, gc);
+}
+
+static unsigned int line_to_port(unsigned int line)
+{
+	return line / 8;
+}
+
+static unsigned int line_to_port_pin(unsigned int line)
+{
+	return line % 8;
+}
+
+static u8 read_u8_reg(void __iomem *reg, unsigned int port)
+{
+	return ioread8(reg + port);
+}
+
+static void write_u8_reg(void __iomem *reg, unsigned int port, u8 value)
+{
+	iowrite8(value, reg + port);
+}
+
+static void write_u16_reg(void __iomem *reg, unsigned int port, u16 value)
+{
+	iowrite16(value, reg + 2 * port);
+}
+
+/*
+ * Since the IMRs contain two bits per GPIO, only 16 GPIO lines fit in a 32-bit
+ * register. Use realtek_gpio_imr_bits() to put the GPIO line's new value in
+ * the right place.
+ */
+static u16 realtek_gpio_imr_bits(unsigned int pin, u16 value)
+{
+	return (value & REALTEK_GPIO_IMR_LINE_MASK) << 2 * pin;
+}
+
+static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl,
+	unsigned int port, u16 irq_type, u16 irq_mask)
+{
+	write_u16_reg(ctrl->base + REALTEK_GPIO_REG_IMR, port,
+		   irq_type & irq_mask);
+}
+
+static void realtek_gpio_irq_ack(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	irq_hw_number_t line = irqd_to_hwirq(data);
+
+	write_u8_reg(ctrl->base + REALTEK_GPIO_REG_ISR, line_to_port(line),
+		BIT(line_to_port_pin(line)));
+}
+
+static void realtek_gpio_irq_unmask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line_to_port(line);
+	unsigned int pin = line_to_port_pin(line);
+	unsigned long flags;
+	u16 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[port];
+	m |= realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[port] = m;
+	realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static void realtek_gpio_irq_mask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line_to_port(line);
+	unsigned int pin = line_to_port_pin(line);
+	unsigned long flags;
+	u16 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[port];
+	m &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[port] = m;
+	realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static int realtek_gpio_irq_set_type(struct irq_data *data,
+	unsigned int flow_type)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line_to_port(line);
+	unsigned int pin = line_to_port_pin(line);
+	unsigned long flags;
+	u16 type, t;
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_FALLING:
+		type = REALTEK_GPIO_IRQ_EDGE_FALLING;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		type = REALTEK_GPIO_IRQ_EDGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		type = REALTEK_GPIO_IRQ_EDGE_BOTH;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	irq_set_handler_locked(data, handle_edge_irq);
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	t = ctrl->intr_type[port];
+	t &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
+	t |= realtek_gpio_imr_bits(pin, type);
+	ctrl->intr_type[port] = t;
+	realtek_gpio_write_imr(ctrl, port, t, ctrl->intr_mask[port]);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static void realtek_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+	void __iomem *reg_isr = ctrl->base + REALTEK_GPIO_REG_ISR;
+	unsigned int lines_done;
+	unsigned int port_pin_count;
+	unsigned int port;
+	unsigned int irq;
+	int offset;
+	unsigned long status;
+
+	chained_irq_enter(irq_chip, desc);
+
+	for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8) {
+		port = line_to_port(lines_done);
+		status = read_u8_reg(reg_isr, port);
+		port_pin_count = min(gc->ngpio - lines_done, 8U);
+		for_each_set_bit(offset, &status, port_pin_count) {
+			irq = irq_find_mapping(gc->irq.domain, offset);
+			generic_handle_irq(irq);
+			write_u8_reg(reg_isr, port, BIT(offset));
+		}
+	}
+
+	chained_irq_exit(irq_chip, desc);
+}
+
+static struct irq_chip realtek_gpio_irq_chip = {
+	.name = "realtek-otto-gpio",
+	.irq_ack = realtek_gpio_irq_ack,
+	.irq_mask = realtek_gpio_irq_mask,
+	.irq_unmask = realtek_gpio_irq_unmask,
+	.irq_set_type = realtek_gpio_irq_set_type,
+};
+
+static const struct of_device_id realtek_gpio_of_match[] = {
+	{ .compatible = "realtek,otto-gpio" },
+	{
+		.compatible = "realtek,rtl8380-gpio",
+		.data = (void *)GPIO_INTERRUPTS
+	},
+	{
+		.compatible = "realtek,rtl8390-gpio",
+		.data = (void *)GPIO_INTERRUPTS
+	},
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
+
+static int realtek_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int dev_flags;
+	struct gpio_irq_chip *girq;
+	struct realtek_gpio_ctrl *ctrl;
+	u32 ngpios;
+	int err, irq;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	dev_flags = (unsigned int) device_get_match_data(dev);
+
+	if (device_property_read_u32(dev, "ngpios", &ngpios))
+		ngpios = REALTEK_GPIO_MAX;
+
+	if (ngpios > REALTEK_GPIO_MAX) {
+		dev_err(&pdev->dev, "invalid ngpios (max. %d)\n",
+			REALTEK_GPIO_MAX);
+		return -EINVAL;
+	}
+
+	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ctrl->base))
+		return PTR_ERR(ctrl->base);
+
+	raw_spin_lock_init(&ctrl->lock);
+
+	err = bgpio_init(&ctrl->gc, dev, 4,
+		ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL,
+		ctrl->base + REALTEK_GPIO_REG_DIR, NULL,
+		BGPIOF_BIG_ENDIAN_BYTE_ORDER);
+	if (err) {
+		dev_err(dev, "unable to init generic GPIO");
+		return err;
+	}
+
+	ctrl->gc.ngpio = ngpios;
+	ctrl->gc.owner = THIS_MODULE;
+
+	irq = platform_get_irq_optional(pdev, 0);
+	if ((dev_flags & GPIO_INTERRUPTS) && irq > 0) {
+		girq = &ctrl->gc.irq;
+		girq->chip = &realtek_gpio_irq_chip;
+		girq->parent_handler = realtek_gpio_irq_handler;
+		girq->num_parents = 1;
+		girq->parents = devm_kcalloc(dev, girq->num_parents,
+					sizeof(*girq->parents),	GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_bad_irq;
+		girq->parents[0] = irq;
+
+		/* Disable and clear all interrupts */
+		iowrite32(0, ctrl->base + REALTEK_GPIO_REG_IMR_AB);
+		iowrite32(0, ctrl->base + REALTEK_GPIO_REG_IMR_CD);
+		iowrite32(GENMASK(31, 0), ctrl->base + REALTEK_GPIO_REG_ISR);
+	}
+
+	return gpiochip_add_data(&ctrl->gc, ctrl);
+}
+
+static struct platform_driver realtek_gpio_driver = {
+	.driver = {
+		.name = "realtek-otto-gpio",
+		.of_match_table	= realtek_gpio_of_match,
+	},
+	.probe = realtek_gpio_probe,
+};
+
+builtin_platform_driver(realtek_gpio_driver);
+
+MODULE_DESCRIPTION("Realtek Otto GPIO support");
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2


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

* Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-26 12:03   ` [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
@ 2021-03-26 18:19     ` Andy Shevchenko
  2021-03-26 21:11       ` Sander Vanheule
  0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2021-03-26 18:19 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: devicetree, open list:GPIO SUBSYSTEM, Bert Vermeulen,
	Bartosz Golaszewski, Linus Walleij, Linux Kernel Mailing List,
	Marc Zyngier, Rob Herring, Thomas Gleixner

On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <sander@svanheule.net> wrote:
>
> Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
> 64 GPIOs, divided over two banks. Each bank has a set of registers for
> 32 GPIOs, with support for edge-triggered interrupts.
>
> Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
> registers pack one bit per GPIO, except for the IMR register, which
> packs two bits per GPIO (AB-CD).
>
> Although the byte order is currently assumed to have port A..D at offset
> 0x0..0x3, this has been observed to be reversed on other, Lexra-based,
> SoCs (e.g. RTL8196E/97D/97F).
>
> Interrupt support is disabled for the fallback devicetree-compatible
> 'realtek,otto-gpio'. This allows for quick support of GPIO banks in
> which the byte order would be unknown. In this case, the port ordering
> in the IMR registers may not match the reversed order in the other
> registers (DCBA, and BA-DC or DC-BA).

Thanks for the update!
My comments below.

...

> +config GPIO_REALTEK_OTTO
> +       bool "Realtek Otto GPIO support"

Why not module?

> +       depends on MACH_REALTEK_RTL
> +       default MACH_REALTEK_RTL
> +       select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP

> +       help
> +         The GPIO controller on the Otto MIPS platform supports up to two
> +         banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
> +         are grouped in four 8-bit wide ports.

When allowing module build, here you may add what will be the name of it.

...

> +/*
> + * Total register block size is 0x1C for four ports.
> + * On the RTL8380/RLT8390 platforms port A, B, and C are implemented.

D?

> + * RTL8389 and RTL8328 implement a second bank with ports E, F, G, and H.
> + *
> + * Port information is stored with the first port at offset 0, followed by the
> + * second, etc. Most registers store one bit per GPIO and should be read out in
> + * reversed endian order. The two interrupt mask registers store two bits per
> + * GPIO, and should be manipulated with swahw32, if required.
> + */

...

> +/*

Seems like kernel doc format with missed ** header and properly formed
summary and description.

> + * Realtek GPIO driver data
> + * Because the interrupt mask register (IMR) combines the function of
> + * IRQ type selection and masking, two extra values are stored.
> + * intr_mask is used to mask/unmask the interrupts for certain GPIO,
> + * and intr_type is used to store the selected interrupt types. The
> + * logical AND of these values is written to IMR on changes.
> + *
> + * @gc Associated gpio_chip instance
> + * @base Base address of the register block
> + * @lock Lock for accessing the IRQ registers and values
> + * @intr_mask Mask for GPIO interrupts
> + * @intr_type GPIO interrupt type selection
> + */
> +struct realtek_gpio_ctrl {
> +       struct gpio_chip gc;
> +       void __iomem *base;
> +       raw_spinlock_t lock;
> +       u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK];
> +       u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK];
> +};
> +
> +enum realtek_gpio_flags {
> +       GPIO_INTERRUPTS = BIT(0),
> +};

...

> +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +
> +       return container_of(gc, struct realtek_gpio_ctrl, gc);
> +}

> +static unsigned int line_to_port(unsigned int line)
> +{
> +       return line / 8;
> +}
> +
> +static unsigned int line_to_port_pin(unsigned int line)
> +{
> +       return line % 8;
> +}

These are useless. Just use them inline.

...

> +static u8 read_u8_reg(void __iomem *reg, unsigned int port)
> +{
> +       return ioread8(reg + port);
> +}
> +
> +static void write_u8_reg(void __iomem *reg, unsigned int port, u8 value)
> +{
> +       iowrite8(value, reg + port);
> +}
> +
> +static void write_u16_reg(void __iomem *reg, unsigned int port, u16 value)
> +{
> +       iowrite16(value, reg + 2 * port);
> +}

What's the point? You better provide a controller structure as a
parameter. Look into other drivers. There are plenty of examples how
to provide IO accessors in smarter way.

...

> +static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl,
> +       unsigned int port, u16 irq_type, u16 irq_mask)
> +{
> +       write_u16_reg(ctrl->base + REALTEK_GPIO_REG_IMR, port,
> +                  irq_type & irq_mask);

Can be one line.

> +}

...

> +       write_u8_reg(ctrl->base + REALTEK_GPIO_REG_ISR, line_to_port(line),
> +               BIT(line_to_port_pin(line)));

line % 8 and line / 8 is much shorter. ANd then it becomes only one line.

...

> +static int realtek_gpio_irq_set_type(struct irq_data *data,
> +       unsigned int flow_type)

One line?

...

> +static void realtek_gpio_irq_handler(struct irq_desc *desc)
> +{
> +       struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> +       struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +       struct irq_chip *irq_chip = irq_desc_get_chip(desc);
> +       void __iomem *reg_isr = ctrl->base + REALTEK_GPIO_REG_ISR;
> +       unsigned int lines_done;
> +       unsigned int port_pin_count;
> +       unsigned int port;
> +       unsigned int irq;

> +       int offset;
> +       unsigned long status;

Rearrange them by swapping lines.

> +       chained_irq_enter(irq_chip, desc);
> +
> +       for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8) {
> +               port = line_to_port(lines_done);
> +               status = read_u8_reg(reg_isr, port);
> +               port_pin_count = min(gc->ngpio - lines_done, 8U);
> +               for_each_set_bit(offset, &status, port_pin_count) {
> +                       irq = irq_find_mapping(gc->irq.domain, offset);
> +                       generic_handle_irq(irq);

> +                       write_u8_reg(reg_isr, port, BIT(offset));

Shouldn't it be in the ->irq_ack() callback?

> +               }
> +       }

...

> +static const struct of_device_id realtek_gpio_of_match[] = {
> +       { .compatible = "realtek,otto-gpio" },
> +       {
> +               .compatible = "realtek,rtl8380-gpio",
> +               .data = (void *)GPIO_INTERRUPTS

Not sure why this flag is needed right now. Drop it completely for good.

> +       },
> +       {
> +               .compatible = "realtek,rtl8390-gpio",
> +               .data = (void *)GPIO_INTERRUPTS

Ditto.

> +       },
> +       {}
> +};

> +

Extra blank line.

> +MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);


...

> +               iowrite32(GENMASK(31, 0), ctrl->base + REALTEK_GPIO_REG_ISR);

This one perhaps needs a comment like "cleaning all IRQ states".
Note, we have a proper callback for this, i.e. hw_init. Consider to use it.

...

> +};

> +

Extra blank line.

> +builtin_platform_driver(realtek_gpio_driver);

...

So, looking into the code, I think you may easily get rid of 30-50 LOCs.
So, expecting <= 300 LOCs in v5.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-26 18:19     ` Andy Shevchenko
@ 2021-03-26 21:11       ` Sander Vanheule
  2021-03-29 10:26         ` Andy Shevchenko
  0 siblings, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-26 21:11 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: devicetree, open list:GPIO SUBSYSTEM, Bert Vermeulen,
	Bartosz Golaszewski, Linus Walleij, Linux Kernel Mailing List,
	Marc Zyngier, Rob Herring, Thomas Gleixner

Hi Andy,

Replies inline below.

On Fri, 2021-03-26 at 20:19 +0200, Andy Shevchenko wrote:
> On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <sander@svanheule.net>
> wrote:
> 
> > +config GPIO_REALTEK_OTTO
> > +       bool "Realtek Otto GPIO support"
> 
> Why not module?

This driver is only useful on a few specific MIPS SoCs, where this GPIO
peripheral is a part of that SoC. What would be the point of providing
this driver as a module?

> 
> > +       depends on MACH_REALTEK_RTL
> > +       default MACH_REALTEK_RTL
> > +       select GPIO_GENERIC
> > +       select GPIOLIB_IRQCHIP
> 
> > +       help
> > +         The GPIO controller on the Otto MIPS platform supports up
> > to two
> > +         banks of 32 GPIOs, with edge triggered interrupts. The 32
> > GPIOs
> > +         are grouped in four 8-bit wide ports.
> 
> When allowing module build, here you may add what will be the name of
> it.
> 
> ...
> 
> > +/*
> > + * Total register block size is 0x1C for four ports.
> > + * On the RTL8380/RLT8390 platforms port A, B, and C are
> > implemented.
> 
> D?

No port D on 8380/8390. Only 24 GPIO lines are present on these
platforms. I'll rephrase this comment.

> 
> > + * RTL8389 and RTL8328 implement a second bank with ports E, F, G,
> > and H.
> > + *
> > + * Port information is stored with the first port at offset 0,
> > followed by the
> > + * second, etc. Most registers store one bit per GPIO and should be
> > read out in
> > + * reversed endian order. The two interrupt mask registers store two
> > bits per
> > + * GPIO, and should be manipulated with swahw32, if required.
> > + */

This reference to swahw32 and the include of linux/swab.h will be
dropped.

> 
> > +/*
> 
> Seems like kernel doc format with missed ** header and properly formed
> summary and description.

I'll reformat.

> 
> > + * Realtek GPIO driver data
> > + * Because the interrupt mask register (IMR) combines the function
> > of
> > + * IRQ type selection and masking, two extra values are stored.
> > + * intr_mask is used to mask/unmask the interrupts for certain
> > GPIO,
> > + * and intr_type is used to store the selected interrupt types.
> > The
> > + * logical AND of these values is written to IMR on changes.
> > + *
> > + * @gc Associated gpio_chip instance
> > + * @base Base address of the register block
> > + * @lock Lock for accessing the IRQ registers and values
> > + * @intr_mask Mask for GPIO interrupts
> > + * @intr_type GPIO interrupt type selection
> > + */
> > +struct realtek_gpio_ctrl {
> > +       struct gpio_chip gc;
> > +       void __iomem *base;
> > +       raw_spinlock_t lock;
> > +       u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK];
> > +       u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK];
> > +};
> > +
> > +enum realtek_gpio_flags {
> > +       GPIO_INTERRUPTS = BIT(0),
> > +};
> 
> ...

See below. I'll add a comment.

> 
> > +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data
> > *data)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> > +
> > +       return container_of(gc, struct realtek_gpio_ctrl, gc);
> > +}
> 
> > +static unsigned int line_to_port(unsigned int line)
> > +{
> > +       return line / 8;
> > +}
> > +
> > +static unsigned int line_to_port_pin(unsigned int line)
> > +{
> > +       return line % 8;
> > +}
> 
> These are useless. Just use them inline.

I added these as the alternative of the /16 and %16 I had for the IMR
offsets in v2. The function names tell the reader _why_ I'm doing the
division and modulo operations, but I guess a properly named variable
would do the same.

> 
> > +static u8 read_u8_reg(void __iomem *reg, unsigned int port)
> > +{
> > +       return ioread8(reg + port);
> > +}
> > +
> > +static void write_u8_reg(void __iomem *reg, unsigned int port, u8
> > value)
> > +{
> > +       iowrite8(value, reg + port);
> > +}
> > +
> > +static void write_u16_reg(void __iomem *reg, unsigned int port, u16
> > value)
> > +{
> > +       iowrite16(value, reg + 2 * port);
> > +}
> 
> What's the point? You better provide a controller structure as a
> parameter. Look into other drivers. There are plenty of examples how
> to provide IO accessors in smarter way.

Since these are currently only really used for IMR and ISR, I'll fold
them into their accessor functions for v5.

> 
> > +static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl,
> > +       unsigned int port, u16 irq_type, u16 irq_mask)
> > +{
> > +       write_u16_reg(ctrl->base + REALTEK_GPIO_REG_IMR, port,
> > +                  irq_type & irq_mask);
> 
> Can be one line.
> 
> > +}
> 
> ...
> 
> > +static int realtek_gpio_irq_set_type(struct irq_data *data,
> > +       unsigned int flow_type)
> 
> One line?

I thought checkpatch.pl would complain, but it doesn't. Folded onto one
line.

> > +       chained_irq_enter(irq_chip, desc);
> > +
> > +       for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8)
> > {
> > +               port = line_to_port(lines_done);
> > +               status = read_u8_reg(reg_isr, port);
> > +               port_pin_count = min(gc->ngpio - lines_done, 8U);
> > +               for_each_set_bit(offset, &status, port_pin_count) {
> > +                       irq = irq_find_mapping(gc->irq.domain,
> > offset);
> > +                       generic_handle_irq(irq);
> 
> > +                       write_u8_reg(reg_isr, port, BIT(offset));
> 
> Shouldn't it be in the ->irq_ack() callback?

I think I added this line to deal with handle_bad_irq during
development. Like you say, handle_edge_irq() has it's specific ACK
logic, so this is probably even a bug. Will be removed.

> 
> > +               }
> > +       }
> 
> ...
> 
> > +static const struct of_device_id realtek_gpio_of_match[] = {
> > +       { .compatible = "realtek,otto-gpio" },
> > +       {
> > +               .compatible = "realtek,rtl8380-gpio",
> > +               .data = (void *)GPIO_INTERRUPTS
> 
> Not sure why this flag is needed right now. Drop it completely for
> good.
> > +       },
> > +       {
> > +               .compatible = "realtek,rtl8390-gpio",
> > +               .data = (void *)GPIO_INTERRUPTS
> 
> Ditto

Linus Walleij asked this question too after v1:
https://lore.kernel.org/linux-gpio/e9f0651e5fb52b7d56361ceb30b41759b6f2ec13.camel@svanheule.net/

Note that the fall-back compatible doesn't have this flag set.

> .
> 
> > +       },
> > +       {}
> > +};
> 
> > +
> 
> Extra blank line.

Add or drop? I see other drivers using no empty line between the
of_match table and the MODULE_DEVICE_TABLE macro.

> 
> > +MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
> 
> 
> ...
> 
> > +               iowrite32(GENMASK(31, 0), ctrl->base +
> > REALTEK_GPIO_REG_ISR);
> 
> This one perhaps needs a comment like "cleaning all IRQ states".
> Note, we have a proper callback for this, i.e. hw_init. Consider to use
> it.

Which "hw_init" are you referring too? I can't really find much, aside
from drivers implementing it themselves to differentiate between driver
and hardware set-up.

Since this is normally only called once, I can turn it into the more
readable:
	for (port = 0; (port * 8) < ngpios; port++) {
        	realtek_gpio_write_imr(ctrl, port, 0, 0);
                realtek_gpio_clear_isr(ctrl, port, GENMASK(7, 0));
        }

> 
> > +};
> 
> > +
> 
> Extra blank line.

I see the same use of one blank line in other drivers.


> > +builtin_platform_driver(realtek_gpio_driver);
> 
> ...
> 
> So, looking into the code, I think you may easily get rid of 30-50
> LOCs.
> So, expecting <= 300 LOCs in v5.

After trimming the file, sloccount puts me at 224, but the total line
count is still 310. :-)


Best,
Sander



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

* Re: [PATCH v4 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO
  2021-03-26 12:03   ` [PATCH v4 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
@ 2021-03-27 18:16     ` Rob Herring
  0 siblings, 0 replies; 39+ messages in thread
From: Rob Herring @ 2021-03-27 18:16 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: devicetree, maz, tglx, linus.walleij, robh+dt, bert,
	andy.shevchenko, bgolaszewski, linux-gpio, linux-kernel

On Fri, 26 Mar 2021 13:03:46 +0100, Sander Vanheule wrote:
> Add a binding description for Realtek's GPIO controller found on several
> of their MIPS-based SoCs (codenamed Otto), such as the RTL838x and
> RTL839x series of switch SoCs.
> 
> A fallback binding 'realtek,otto-gpio' is provided for cases where the
> actual port ordering is not known yet, and enabling the interrupt
> controller may result in uncaught interrupts.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../bindings/gpio/realtek,otto-gpio.yaml      | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

* Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-26 21:11       ` Sander Vanheule
@ 2021-03-29 10:26         ` Andy Shevchenko
  2021-03-29 17:28           ` Sander Vanheule
  0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2021-03-29 10:26 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: devicetree, open list:GPIO SUBSYSTEM, Bert Vermeulen,
	Bartosz Golaszewski, Linus Walleij, Linux Kernel Mailing List,
	Marc Zyngier, Rob Herring, Thomas Gleixner

On Fri, Mar 26, 2021 at 11:11 PM Sander Vanheule <sander@svanheule.net> wrote:
> On Fri, 2021-03-26 at 20:19 +0200, Andy Shevchenko wrote:
> > On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <sander@svanheule.net>
> > wrote:

...

> > > +       bool "Realtek Otto GPIO support"
> >
> > Why not module?
>
> This driver is only useful on a few specific MIPS SoCs, where this GPIO
> peripheral is a part of that SoC. What would be the point of providing
> this driver as a module?

If it's not critical for boot this makes the kernel smaller and loads
modules only on demand.
Also, (the main part) it allows to build multi-target kernels which
are in general smaller.

That said, you must provide quite a good justification why it's *not* a module.
Otherwise, fix the Kconfig and code accordingly.

...

> > > +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data
> > > *data)
> > > +{
> > > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> > > +
> > > +       return container_of(gc, struct realtek_gpio_ctrl, gc);
> > > +}
> >
> > > +static unsigned int line_to_port(unsigned int line)
> > > +{
> > > +       return line / 8;
> > > +}
> > > +
> > > +static unsigned int line_to_port_pin(unsigned int line)
> > > +{
> > > +       return line % 8;
> > > +}
> >
> > These are useless. Just use them inline.
>
> I added these as the alternative of the /16 and %16 I had for the IMR
> offsets in v2. The function names tell the reader _why_ I'm doing the
> division and modulo operations, but I guess a properly named variable
> would do the same.

Exactly! So, please use better variable names on stack.

...


> > > +static const struct of_device_id realtek_gpio_of_match[] = {
> > > +       { .compatible = "realtek,otto-gpio" },
> > > +       {
> > > +               .compatible = "realtek,rtl8380-gpio",
> > > +               .data = (void *)GPIO_INTERRUPTS
> >
> > Not sure why this flag is needed right now. Drop it completely for
> > good.
> > > +       },
> > > +       {
> > > +               .compatible = "realtek,rtl8390-gpio",
> > > +               .data = (void *)GPIO_INTERRUPTS
> >
> > Ditto
>
> Linus Walleij asked this question too after v1:
> https://lore.kernel.org/linux-gpio/e9f0651e5fb52b7d56361ceb30b41759b6f2ec13.camel@svanheule.net/
>
> Note that the fall-back compatible doesn't have this flag set.

AFAICS all, except one have this flag, I suggest you to do other way
around, i.e. check compatible string in the code. Or do something more
clever. What happens if you have this flag enabled for the fallback
node?

If two people ask the same, it might be a smoking gun.

...

> > > +};
> >
> > > +
> >
> > Extra blank line.
>
> Add or drop?

What do you think? :-)

> I see other drivers using no empty line between the
> of_match table and the MODULE_DEVICE_TABLE macro.

Yep, this is not a competition on amount of LOCs, actually, less LOCs
is better, if it keeps the same level of readability and
maintainability,

...

> > > +               iowrite32(GENMASK(31, 0), ctrl->base +
> > > REALTEK_GPIO_REG_ISR);
> >
> > This one perhaps needs a comment like "cleaning all IRQ states".
> > Note, we have a proper callback for this, i.e. hw_init. Consider to use
> > it.
>
> Which "hw_init" are you referring too? I can't really find much, aside
> from drivers implementing it themselves to differentiate between driver
> and hardware set-up.
>
> Since this is normally only called once, I can turn it into the more
> readable:
>         for (port = 0; (port * 8) < ngpios; port++) {
>                 realtek_gpio_write_imr(ctrl, port, 0, 0);
>                 realtek_gpio_clear_isr(ctrl, port, GENMASK(7, 0));
>         }

Good and move it to the callback.

->init_hw() in GPIO IRQ chip data structure.

...

> > > +};
> >
> > > +
> >
> > Extra blank line.
>
> I see the same use of one blank line in other drivers.

Same as above.

> > > +builtin_platform_driver(realtek_gpio_driver);

...

> > So, looking into the code, I think you may easily get rid of 30-50
> > LOCs.
> > So, expecting <= 300 LOCs in v5.
>
> After trimming the file, sloccount puts me at 224, but the total line
> count is still 310. :-)

I was referring to the LOCs, i.o.w. real code with comments :-)

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-29 10:26         ` Andy Shevchenko
@ 2021-03-29 17:28           ` Sander Vanheule
  2021-03-30 10:14             ` Andy Shevchenko
  0 siblings, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-29 17:28 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: devicetree, open list:GPIO SUBSYSTEM, Bert Vermeulen,
	Bartosz Golaszewski, Linus Walleij, Linux Kernel Mailing List,
	Marc Zyngier, Rob Herring, Thomas Gleixner

Hi Andy,

Thank you for clarifying your remarks. I'll support for building as a
module, and have implemented the gpio_irq_chip->init_hw() callback.

On Mon, 2021-03-29 at 13:26 +0300, Andy Shevchenko wrote:
> On Fri, Mar 26, 2021 at 11:11 PM Sander Vanheule <
> sander@svanheule.net> wrote:
> > On Fri, 2021-03-26 at 20:19 +0200, Andy Shevchenko wrote:
> > > On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <
> > > sander@svanheule.net>
> > > wrote:
> > > > +static const struct of_device_id realtek_gpio_of_match[] = {
> > > > +       { .compatible = "realtek,otto-gpio" },
> > > > +       {
> > > > +               .compatible = "realtek,rtl8380-gpio",
> > > > +               .data = (void *)GPIO_INTERRUPTS
> > > 
> > > Not sure why this flag is needed right now. Drop it completely for
> > > good.
> > > > +       },
> > > > +       {
> > > > +               .compatible = "realtek,rtl8390-gpio",
> > > > +               .data = (void *)GPIO_INTERRUPTS
> > > 
> > > Ditto
> > 
> > Linus Walleij asked this question too after v1:
> > https://lore.kernel.org/linux-gpio/e9f0651e5fb52b7d56361ceb30b41759b6f2ec13.camel@svanheule.net/
> > 
> > Note that the fall-back compatible doesn't have this flag set.
> 
> AFAICS all, except one have this flag, I suggest you to do other way
> around, i.e. check compatible string in the code. Or do something more
> clever. What happens if you have this flag enabled for the fallback
> node?
> 
> If two people ask the same, it might be a smoking gun.
> 

Testing for the fallback wouldn't work, since of_device_is_compatible()
would always match. Setting the (inverse) flag only on the fallback
would indeed reduce the clutter.

If the port order is reversed w.r.t. to the current implementation,
enabling a GPIO+IRQ would enable the same pin on a different port. I
don't think the result would be catastrophical, but it would result in
unexpected behaviour. When A0 and C0 are then enabled, A0 interrupts
would actually come from C0, and vice versa.

   Intended port | A | B | C | D
-----------------+---+---+---+---
Actual GPIO port | D | C | B | A
 Actual IRQ port | B | A | D | C

If only the actual GPIO ports change, at least you can still use a
modified GPIO line number and polling. The user could just leave out
the optional irq-controller from the devicetree, but I would rather
have it enforced in some way.


Best,
Sander


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

* Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-29 17:28           ` Sander Vanheule
@ 2021-03-30 10:14             ` Andy Shevchenko
  0 siblings, 0 replies; 39+ messages in thread
From: Andy Shevchenko @ 2021-03-30 10:14 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: devicetree, open list:GPIO SUBSYSTEM, Bert Vermeulen,
	Bartosz Golaszewski, Linus Walleij, Linux Kernel Mailing List,
	Marc Zyngier, Rob Herring, Thomas Gleixner

On Mon, Mar 29, 2021 at 8:28 PM Sander Vanheule <sander@svanheule.net> wrote:
> On Mon, 2021-03-29 at 13:26 +0300, Andy Shevchenko wrote:
> > On Fri, Mar 26, 2021 at 11:11 PM Sander Vanheule <
> > sander@svanheule.net> wrote:

...

> > AFAICS all, except one have this flag, I suggest you to do other way
> > around, i.e. check compatible string in the code. Or do something more
> > clever. What happens if you have this flag enabled for the fallback
> > node?
> >
> > If two people ask the same, it might be a smoking gun.
> >
>
> Testing for the fallback wouldn't work, since of_device_is_compatible()
> would always match. Setting the (inverse) flag only on the fallback
> would indeed reduce the clutter.
>
> If the port order is reversed w.r.t. to the current implementation,
> enabling a GPIO+IRQ would enable the same pin on a different port. I
> don't think the result would be catastrophical, but it would result in
> unexpected behaviour. When A0 and C0 are then enabled, A0 interrupts
> would actually come from C0, and vice versa.
>
>    Intended port | A | B | C | D
> -----------------+---+---+---+---
> Actual GPIO port | D | C | B | A
>  Actual IRQ port | B | A | D | C
>
> If only the actual GPIO ports change, at least you can still use a
> modified GPIO line number and polling. The user could just leave out
> the optional irq-controller from the devicetree, but I would rather
> have it enforced in some way.

OK! Thanks for clarification.

-- 
With Best Regards,
Andy Shevchenko

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

* [PATCH v5 0/2] Add Realtek Otto GPIO support
  2021-03-15  8:23 [PATCH 0/2] Add Realtek Otto GPIO support Sander Vanheule
                   ` (4 preceding siblings ...)
  2021-03-26 12:03 ` [PATCH v4 0/2] " Sander Vanheule
@ 2021-03-30 15:26 ` Sander Vanheule
  2021-03-30 15:26   ` [PATCH v5 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
  2021-03-30 15:26   ` [PATCH v5 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
  2021-03-30 17:48 ` [PATCH v6 0/2] " Sander Vanheule
  6 siblings, 2 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-30 15:26 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: andy.shevchenko, bert, bgolaszewski, linus.walleij, linux-kernel,
	maz, robh+dt, Sander Vanheule

Add support for the GPIO controller employed by Realtek in multiple series of
MIPS SoCs. These include the supported RTL838x and RTL839x. The register layout
also matches the one found in the GPIO controller of other (Lexra-based) SoCs
such as RTL8196E, RTL8197D, and RTL8197F.

For the platform name 'otto', I am not aware of any official resources as to
what hardware this specifically applies to. However, in all of the GPL archives
we've received, from vendors using compatible SoCs in their design, the
platform under the MIPS architecture is referred to by this name.

The GPIO ports have been tested on a Zyxel GS1900-8 (RTL8380), and Zyxel
GS1900-48 (RTL8393). Furthermore, the GPIO ports and interrupt controller have
been tested on a Netgear GS110TPPv1 (RTL8381).

Changes in v5:
- Edited code comments
- Fold functions that were used only once or twice (ISR/IMR accessors)
- Drop trivial functions for line to port/pin calculations
- Use gpio_irq_chip->init_hw() to initialise IRQ registers
- Invert GPIO_INTERRUPTS flag to GPIO_INTERRUPTS_DISABLED
- Support building as module
- Add Rob's Reviewed-by tag

Changes in v4:
- Fix pointer notation style
- Drop unused read_u16_reg() function
- Drop 'inline' specifier from functions

Changes in v3:
- Remove OF dependencies in driver probe
- Don't accept IRQ_TYPE_NONE as a valid interrupt type
- Remove (now unused) dev property from control structure
- Use u8/u16 port registers, instead of raw u32 registers
- Use 'line' name for gpiochip, 'port' and 'pin' names for hardware
- Renamed DT bindings file
- Dropped fallback-only DT compatible
- Various code style clean-ups

Changes in v2:
- Clarify structure and usage of IMR registers
- Added Linus' Reviewed-by tags

Sander Vanheule (2):
  dt-bindings: gpio: Binding for Realtek Otto GPIO
  gpio: Add Realtek Otto GPIO support

 .../bindings/gpio/realtek,otto-gpio.yaml      |  78 +++++
 drivers/gpio/Kconfig                          |  13 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-realtek-otto.c              | 326 ++++++++++++++++++
 4 files changed, 418 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

-- 
2.30.2


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

* [PATCH v5 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO
  2021-03-30 15:26 ` [PATCH v5 0/2] " Sander Vanheule
@ 2021-03-30 15:26   ` Sander Vanheule
  2021-03-30 15:26   ` [PATCH v5 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
  1 sibling, 0 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-30 15:26 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: andy.shevchenko, bert, bgolaszewski, linus.walleij, linux-kernel,
	maz, robh+dt, Sander Vanheule, Rob Herring

Add a binding description for Realtek's GPIO controller found on several
of their MIPS-based SoCs (codenamed Otto), such as the RTL838x and
RTL839x series of switch SoCs.

A fallback binding 'realtek,otto-gpio' is provided for cases where the
actual port ordering is not known yet, and enabling the interrupt
controller may result in uncaught interrupts.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/gpio/realtek,otto-gpio.yaml      | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml b/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
new file mode 100644
index 000000000000..100f20cebd76
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/realtek,otto-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek Otto GPIO controller
+
+maintainers:
+  - Sander Vanheule <sander@svanheule.net>
+  - Bert Vermeulen <bert@biot.com>
+
+description: |
+  Realtek's GPIO controller on their MIPS switch SoCs (Otto platform) consists
+  of two banks of 32 GPIOs. These GPIOs can generate edge-triggered interrupts.
+  Each bank's interrupts are cascased into one interrupt line on the parent
+  interrupt controller, if provided.
+  This binding allows defining a single bank in the devicetree. The interrupt
+  controller is not supported on the fallback compatible name, which only
+  allows for GPIO port use.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    items:
+      - enum:
+          - realtek,rtl8380-gpio
+          - realtek,rtl8390-gpio
+      - const: realtek,otto-gpio
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  ngpios:
+    minimum: 1
+    maximum: 32
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+dependencies:
+  interrupt-controller: [ interrupts ]
+
+examples:
+  - |
+      gpio@3500 {
+        compatible = "realtek,rtl8380-gpio", "realtek,otto-gpio";
+        reg = <0x3500 0x1c>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        ngpios = <24>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&rtlintc>;
+        interrupts = <23>;
+      };
+
+...
-- 
2.30.2


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

* [PATCH v5 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-30 15:26 ` [PATCH v5 0/2] " Sander Vanheule
  2021-03-30 15:26   ` [PATCH v5 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
@ 2021-03-30 15:26   ` Sander Vanheule
  2021-03-30 16:43     ` Andy Shevchenko
  1 sibling, 1 reply; 39+ messages in thread
From: Sander Vanheule @ 2021-03-30 15:26 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: andy.shevchenko, bert, bgolaszewski, linus.walleij, linux-kernel,
	maz, robh+dt, Sander Vanheule

Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
64 GPIOs, divided over two banks. Each bank has a set of registers for
32 GPIOs, with support for edge-triggered interrupts.

Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
registers pack one bit per GPIO, except for the IMR register, which
packs two bits per GPIO (AB-CD).

Although the byte order is currently assumed to have port A..D at offset
0x0..0x3, this has been observed to be reversed on other, Lexra-based,
SoCs (e.g. RTL8196E/97D/97F).

Interrupt support is disabled for the fallback devicetree-compatible
'realtek,otto-gpio'. This allows for quick support of GPIO banks in
which the byte order would be unknown. In this case, the port ordering
in the IMR registers may not match the reversed order in the other
registers (DCBA, and BA-DC or DC-BA).

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/Kconfig             |  13 ++
 drivers/gpio/Makefile            |   1 +
 drivers/gpio/gpio-realtek-otto.c | 326 +++++++++++++++++++++++++++++++
 3 files changed, 340 insertions(+)
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..6fb13d6507db 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -502,6 +502,19 @@ config GPIO_RDA
 	help
 	  Say Y here to support RDA Micro GPIO controller.
 
+config GPIO_REALTEK_OTTO
+	tristate "Realtek Otto GPIO support"
+	depends on MACH_REALTEK_RTL
+	default MACH_REALTEK_RTL
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	help
+	  The GPIO controller on the Otto MIPS platform supports up to two
+	  banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
+	  are grouped in four 8-bit wide ports.
+
+	  When built as a module, the module will be called realtek_otto_gpio.
+
 config GPIO_REG
 	bool
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..8ace5934e3c3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583)		+= gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RCAR)			+= gpio-rcar.o
 obj-$(CONFIG_GPIO_RDA)			+= gpio-rda.o
 obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
+obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
new file mode 100644
index 000000000000..05ce5d48e121
--- /dev/null
+++ b/drivers/gpio/gpio-realtek-otto.c
@@ -0,0 +1,326 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/gpio/driver.h>
+#include <linux/irq.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+/*
+ * Total register block size is 0x1C for one bank of four ports (A, B, C, D).
+ * An optional second bank, with ports E, F, G, and H, may be present, starting
+ * at register offset 0x1C.
+ */
+
+/*
+ * Pin select: (0) "normal", (1) "dedicate peripheral"
+ * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits
+ * in the peripheral registers.
+ */
+#define REALTEK_GPIO_REG_CNR		0x00
+/* Clear bit (0) for input, set bit (1) for output */
+#define REALTEK_GPIO_REG_DIR		0x08
+#define REALTEK_GPIO_REG_DATA		0x0C
+/* Read bit for IRQ status, write 1 to clear IRQ */
+#define REALTEK_GPIO_REG_ISR		0x10
+/* Two bits per GPIO in IMR registers */
+#define REALTEK_GPIO_REG_IMR		0x14
+#define REALTEK_GPIO_REG_IMR_AB		0x14
+#define REALTEK_GPIO_REG_IMR_CD		0x18
+#define REALTEK_GPIO_IMR_LINE_MASK	GENMASK(1, 0)
+#define REALTEK_GPIO_IRQ_EDGE_FALLING	1
+#define REALTEK_GPIO_IRQ_EDGE_RISING	2
+#define REALTEK_GPIO_IRQ_EDGE_BOTH	3
+
+#define REALTEK_GPIO_MAX		32
+#define REALTEK_GPIO_PORTS_PER_BANK	4
+
+/**
+ * realtek_gpio_ctrl - Realtek Otto GPIO driver data
+ *
+ * @gc: Associated gpio_chip instance
+ * @base: Base address of the register block for a GPIO bank
+ * @lock: Lock for accessing the IRQ registers and values
+ * @intr_mask: Mask for interrupts lines
+ * @intr_type: Interrupt type selection
+ *
+ * Because the interrupt mask register (IMR) combines the function of IRQ type
+ * selection and masking, two extra values are stored. @intr_mask is used to
+ * mask/unmask the interrupts for a GPIO port, and @intr_type is used to store
+ * the selected interrupt types. The logical AND of these values is written to
+ * IMR on changes.
+ */
+struct realtek_gpio_ctrl {
+	struct gpio_chip gc;
+	void __iomem *base;
+	raw_spinlock_t lock;
+	u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK];
+	u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK];
+};
+
+/* Expand with more flags as devices with other quirks are added */
+enum realtek_gpio_flags {
+	/*
+	 * Allow disabling interrupts, for cases where the port order is
+	 * unknown. This may result in a port mismatch between ISR and IMR.
+	 * An interrupt would appear to come from a different line than the
+	 * line the IRQ handler was assigned to, causing uncaught interrupts.
+	 */
+	GPIO_INTERRUPTS_DISABLED = BIT(0),
+};
+
+static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+
+	return container_of(gc, struct realtek_gpio_ctrl, gc);
+}
+
+/*
+ * Normal port order register access
+ *
+ * Port information is stored with the first port at offset 0, followed by the
+ * second, etc. Most registers store one bit per GPIO and use a u8 value per
+ * port. The two interrupt mask registers store two bits per GPIO, so use u16
+ * values.
+ */
+static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl,
+	unsigned int port, u16 irq_type, u16 irq_mask)
+{
+	iowrite16(irq_type & irq_mask, ctrl->base + REALTEK_GPIO_REG_IMR + 2 * port);
+}
+
+static void realtek_gpio_clear_isr(struct realtek_gpio_ctrl *ctrl,
+	unsigned int port, u8 mask)
+{
+	iowrite8(mask, ctrl->base + REALTEK_GPIO_REG_ISR + port);
+}
+
+static u8 realtek_gpio_read_isr(struct realtek_gpio_ctrl *ctrl,
+	unsigned int port)
+{
+	return ioread8(ctrl->base + REALTEK_GPIO_REG_ISR + port);
+}
+
+/* Set the rising and falling edge mask bits for a GPIO port pin */
+static u16 realtek_gpio_imr_bits(unsigned int pin, u16 value)
+{
+	return (value & REALTEK_GPIO_IMR_LINE_MASK) << 2 * pin;
+}
+
+static void realtek_gpio_irq_ack(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	irq_hw_number_t line = irqd_to_hwirq(data);
+	unsigned int port = line / 8;
+	unsigned int port_pin = line % 8;
+
+	realtek_gpio_clear_isr(ctrl, port, BIT(port_pin));
+}
+
+static void realtek_gpio_irq_unmask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line / 8;
+	unsigned int port_pin = line % 8;
+	unsigned long flags;
+	u16 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[port];
+	m |= realtek_gpio_imr_bits(port_pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[port] = m;
+	realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static void realtek_gpio_irq_mask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line / 8;
+	unsigned int port_pin = line % 8;
+	unsigned long flags;
+	u16 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[port];
+	m &= ~realtek_gpio_imr_bits(port_pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[port] = m;
+	realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static int realtek_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line / 8;
+	unsigned int port_pin = line % 8;
+	unsigned long flags;
+	u16 type, t;
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_FALLING:
+		type = REALTEK_GPIO_IRQ_EDGE_FALLING;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		type = REALTEK_GPIO_IRQ_EDGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		type = REALTEK_GPIO_IRQ_EDGE_BOTH;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	irq_set_handler_locked(data, handle_edge_irq);
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	t = ctrl->intr_type[port];
+	t &= ~realtek_gpio_imr_bits(port_pin, REALTEK_GPIO_IMR_LINE_MASK);
+	t |= realtek_gpio_imr_bits(port_pin, type);
+	ctrl->intr_type[port] = t;
+	realtek_gpio_write_imr(ctrl, port, t, ctrl->intr_mask[port]);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static void realtek_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+	unsigned int lines_done;
+	unsigned int port_pin_count;
+	unsigned int irq;
+	unsigned long status;
+	int offset;
+
+	chained_irq_enter(irq_chip, desc);
+
+	for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8) {
+		status = realtek_gpio_read_isr(ctrl, lines_done / 8);
+		port_pin_count = min(gc->ngpio - lines_done, 8U);
+		for_each_set_bit(offset, &status, port_pin_count) {
+			irq = irq_find_mapping(gc->irq.domain, offset);
+			generic_handle_irq(irq);
+		}
+	}
+
+	chained_irq_exit(irq_chip, desc);
+}
+
+static int realtek_gpio_irq_init(struct gpio_chip *gc)
+{
+	struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	unsigned int port;
+
+	for (port = 0; (port * 8) < gc->ngpio; port++) {
+		realtek_gpio_write_imr(ctrl, port, 0, 0);
+		realtek_gpio_clear_isr(ctrl, port, GENMASK(7, 0));
+	}
+
+	return 0;
+}
+
+static struct irq_chip realtek_gpio_irq_chip = {
+	.name = "realtek-otto-gpio",
+	.irq_ack = realtek_gpio_irq_ack,
+	.irq_mask = realtek_gpio_irq_mask,
+	.irq_unmask = realtek_gpio_irq_unmask,
+	.irq_set_type = realtek_gpio_irq_set_type,
+};
+
+static const struct of_device_id realtek_gpio_of_match[] = {
+	{
+		.compatible = "realtek,otto-gpio",
+		.data = (void *)GPIO_INTERRUPTS_DISABLED,
+	},
+	{
+		.compatible = "realtek,rtl8380-gpio",
+	},
+	{
+		.compatible = "realtek,rtl8390-gpio",
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
+
+static int realtek_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int dev_flags;
+	struct gpio_irq_chip *girq;
+	struct realtek_gpio_ctrl *ctrl;
+	u32 ngpios;
+	int err, irq;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	dev_flags = (unsigned int) device_get_match_data(dev);
+
+	if (device_property_read_u32(dev, "ngpios", &ngpios))
+		ngpios = REALTEK_GPIO_MAX;
+
+	if (ngpios > REALTEK_GPIO_MAX) {
+		dev_err(&pdev->dev, "invalid ngpios (max. %d)\n",
+			REALTEK_GPIO_MAX);
+		return -EINVAL;
+	}
+
+	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ctrl->base))
+		return PTR_ERR(ctrl->base);
+
+	raw_spin_lock_init(&ctrl->lock);
+
+	err = bgpio_init(&ctrl->gc, dev, 4,
+		ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL,
+		ctrl->base + REALTEK_GPIO_REG_DIR, NULL,
+		BGPIOF_BIG_ENDIAN_BYTE_ORDER);
+	if (err) {
+		dev_err(dev, "unable to init generic GPIO");
+		return err;
+	}
+
+	ctrl->gc.ngpio = ngpios;
+	ctrl->gc.owner = THIS_MODULE;
+
+	irq = platform_get_irq_optional(pdev, 0);
+	if (!(dev_flags & GPIO_INTERRUPTS_DISABLED) && irq > 0) {
+		girq = &ctrl->gc.irq;
+		girq->chip = &realtek_gpio_irq_chip;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_bad_irq;
+		girq->parent_handler = realtek_gpio_irq_handler;
+		girq->num_parents = 1;
+		girq->parents = devm_kcalloc(dev, girq->num_parents,
+					sizeof(*girq->parents),	GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+		girq->parents[0] = irq;
+		girq->init_hw = realtek_gpio_irq_init;
+	}
+
+	return gpiochip_add_data(&ctrl->gc, ctrl);
+}
+
+static struct platform_driver realtek_gpio_driver = {
+	.driver = {
+		.name = "realtek-otto-gpio",
+		.of_match_table	= realtek_gpio_of_match,
+	},
+	.probe = realtek_gpio_probe,
+};
+module_platform_driver(realtek_gpio_driver);
+
+MODULE_DESCRIPTION("Realtek Otto GPIO support");
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2


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

* Re: [PATCH v5 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-30 15:26   ` [PATCH v5 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
@ 2021-03-30 16:43     ` Andy Shevchenko
  0 siblings, 0 replies; 39+ messages in thread
From: Andy Shevchenko @ 2021-03-30 16:43 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: devicetree, open list:GPIO SUBSYSTEM, Bert Vermeulen,
	Bartosz Golaszewski, Linus Walleij, Linux Kernel Mailing List,
	Marc Zyngier, Rob Herring

On Tue, Mar 30, 2021 at 6:27 PM Sander Vanheule <sander@svanheule.net> wrote:
>
> Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
> 64 GPIOs, divided over two banks. Each bank has a set of registers for
> 32 GPIOs, with support for edge-triggered interrupts.
>
> Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
> registers pack one bit per GPIO, except for the IMR register, which
> packs two bits per GPIO (AB-CD).
>
> Although the byte order is currently assumed to have port A..D at offset
> 0x0..0x3, this has been observed to be reversed on other, Lexra-based,
> SoCs (e.g. RTL8196E/97D/97F).
>
> Interrupt support is disabled for the fallback devicetree-compatible
> 'realtek,otto-gpio'. This allows for quick support of GPIO banks in
> which the byte order would be unknown. In this case, the port ordering
> in the IMR registers may not match the reversed order in the other
> registers (DCBA, and BA-DC or DC-BA).

Thanks for an update. Much better now!
My comments below.

After addressing them,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/Kconfig             |  13 ++
>  drivers/gpio/Makefile            |   1 +
>  drivers/gpio/gpio-realtek-otto.c | 326 +++++++++++++++++++++++++++++++
>  3 files changed, 340 insertions(+)
>  create mode 100644 drivers/gpio/gpio-realtek-otto.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e3607ec4c2e8..6fb13d6507db 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -502,6 +502,19 @@ config GPIO_RDA
>         help
>           Say Y here to support RDA Micro GPIO controller.
>
> +config GPIO_REALTEK_OTTO
> +       tristate "Realtek Otto GPIO support"
> +       depends on MACH_REALTEK_RTL
> +       default MACH_REALTEK_RTL
> +       select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP
> +       help
> +         The GPIO controller on the Otto MIPS platform supports up to two
> +         banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
> +         are grouped in four 8-bit wide ports.
> +
> +         When built as a module, the module will be called realtek_otto_gpio.
> +
>  config GPIO_REG
>         bool
>         help
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c58a90a3c3b1..8ace5934e3c3 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583)          += gpio-rc5t583.o
>  obj-$(CONFIG_GPIO_RCAR)                        += gpio-rcar.o
>  obj-$(CONFIG_GPIO_RDA)                 += gpio-rda.o
>  obj-$(CONFIG_GPIO_RDC321X)             += gpio-rdc321x.o
> +obj-$(CONFIG_GPIO_REALTEK_OTTO)                += gpio-realtek-otto.o
>  obj-$(CONFIG_GPIO_REG)                 += gpio-reg.o
>  obj-$(CONFIG_ARCH_SA1100)              += gpio-sa1100.o
>  obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)       += gpio-sama5d2-piobu.o
> diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
> new file mode 100644
> index 000000000000..05ce5d48e121
> --- /dev/null
> +++ b/drivers/gpio/gpio-realtek-otto.c
> @@ -0,0 +1,326 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/irq.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>

In sorter order this goes before module.h.

> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +/*
> + * Total register block size is 0x1C for one bank of four ports (A, B, C, D).
> + * An optional second bank, with ports E, F, G, and H, may be present, starting
> + * at register offset 0x1C.
> + */
> +
> +/*
> + * Pin select: (0) "normal", (1) "dedicate peripheral"
> + * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits
> + * in the peripheral registers.
> + */
> +#define REALTEK_GPIO_REG_CNR           0x00
> +/* Clear bit (0) for input, set bit (1) for output */
> +#define REALTEK_GPIO_REG_DIR           0x08
> +#define REALTEK_GPIO_REG_DATA          0x0C
> +/* Read bit for IRQ status, write 1 to clear IRQ */
> +#define REALTEK_GPIO_REG_ISR           0x10
> +/* Two bits per GPIO in IMR registers */
> +#define REALTEK_GPIO_REG_IMR           0x14
> +#define REALTEK_GPIO_REG_IMR_AB                0x14
> +#define REALTEK_GPIO_REG_IMR_CD                0x18
> +#define REALTEK_GPIO_IMR_LINE_MASK     GENMASK(1, 0)
> +#define REALTEK_GPIO_IRQ_EDGE_FALLING  1
> +#define REALTEK_GPIO_IRQ_EDGE_RISING   2
> +#define REALTEK_GPIO_IRQ_EDGE_BOTH     3
> +
> +#define REALTEK_GPIO_MAX               32
> +#define REALTEK_GPIO_PORTS_PER_BANK    4
> +
> +/**
> + * realtek_gpio_ctrl - Realtek Otto GPIO driver data
> + *
> + * @gc: Associated gpio_chip instance
> + * @base: Base address of the register block for a GPIO bank
> + * @lock: Lock for accessing the IRQ registers and values
> + * @intr_mask: Mask for interrupts lines
> + * @intr_type: Interrupt type selection
> + *
> + * Because the interrupt mask register (IMR) combines the function of IRQ type
> + * selection and masking, two extra values are stored. @intr_mask is used to
> + * mask/unmask the interrupts for a GPIO port, and @intr_type is used to store
> + * the selected interrupt types. The logical AND of these values is written to
> + * IMR on changes.
> + */
> +struct realtek_gpio_ctrl {
> +       struct gpio_chip gc;
> +       void __iomem *base;
> +       raw_spinlock_t lock;
> +       u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK];
> +       u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK];
> +};
> +
> +/* Expand with more flags as devices with other quirks are added */
> +enum realtek_gpio_flags {
> +       /*
> +        * Allow disabling interrupts, for cases where the port order is
> +        * unknown. This may result in a port mismatch between ISR and IMR.
> +        * An interrupt would appear to come from a different line than the
> +        * line the IRQ handler was assigned to, causing uncaught interrupts.
> +        */
> +       GPIO_INTERRUPTS_DISABLED = BIT(0),
> +};
> +
> +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +
> +       return container_of(gc, struct realtek_gpio_ctrl, gc);
> +}
> +
> +/*
> + * Normal port order register access
> + *
> + * Port information is stored with the first port at offset 0, followed by the
> + * second, etc. Most registers store one bit per GPIO and use a u8 value per
> + * port. The two interrupt mask registers store two bits per GPIO, so use u16
> + * values.
> + */
> +static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl,
> +       unsigned int port, u16 irq_type, u16 irq_mask)

Okay, you use 1 TAB as indentation. I prefer a different style: under
the first character after (, but it's up to you. I'm fine with either.

> +{
> +       iowrite16(irq_type & irq_mask, ctrl->base + REALTEK_GPIO_REG_IMR + 2 * port);
> +}
> +
> +static void realtek_gpio_clear_isr(struct realtek_gpio_ctrl *ctrl,
> +       unsigned int port, u8 mask)
> +{
> +       iowrite8(mask, ctrl->base + REALTEK_GPIO_REG_ISR + port);
> +}
> +
> +static u8 realtek_gpio_read_isr(struct realtek_gpio_ctrl *ctrl,
> +       unsigned int port)
> +{
> +       return ioread8(ctrl->base + REALTEK_GPIO_REG_ISR + port);
> +}
> +
> +/* Set the rising and falling edge mask bits for a GPIO port pin */
> +static u16 realtek_gpio_imr_bits(unsigned int pin, u16 value)
> +{
> +       return (value & REALTEK_GPIO_IMR_LINE_MASK) << 2 * pin;
> +}
> +
> +static void realtek_gpio_irq_ack(struct irq_data *data)
> +{
> +       struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> +       irq_hw_number_t line = irqd_to_hwirq(data);
> +       unsigned int port = line / 8;
> +       unsigned int port_pin = line % 8;
> +
> +       realtek_gpio_clear_isr(ctrl, port, BIT(port_pin));
> +}
> +
> +static void realtek_gpio_irq_unmask(struct irq_data *data)
> +{
> +       struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> +       unsigned int line = irqd_to_hwirq(data);
> +       unsigned int port = line / 8;
> +       unsigned int port_pin = line % 8;
> +       unsigned long flags;
> +       u16 m;
> +
> +       raw_spin_lock_irqsave(&ctrl->lock, flags);
> +       m = ctrl->intr_mask[port];
> +       m |= realtek_gpio_imr_bits(port_pin, REALTEK_GPIO_IMR_LINE_MASK);
> +       ctrl->intr_mask[port] = m;
> +       realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
> +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +}
> +
> +static void realtek_gpio_irq_mask(struct irq_data *data)
> +{
> +       struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> +       unsigned int line = irqd_to_hwirq(data);
> +       unsigned int port = line / 8;
> +       unsigned int port_pin = line % 8;
> +       unsigned long flags;
> +       u16 m;
> +
> +       raw_spin_lock_irqsave(&ctrl->lock, flags);
> +       m = ctrl->intr_mask[port];
> +       m &= ~realtek_gpio_imr_bits(port_pin, REALTEK_GPIO_IMR_LINE_MASK);
> +       ctrl->intr_mask[port] = m;
> +       realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
> +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +}
> +
> +static int realtek_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type)
> +{
> +       struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> +       unsigned int line = irqd_to_hwirq(data);
> +       unsigned int port = line / 8;
> +       unsigned int port_pin = line % 8;
> +       unsigned long flags;
> +       u16 type, t;
> +
> +       switch (flow_type & IRQ_TYPE_SENSE_MASK) {
> +       case IRQ_TYPE_EDGE_FALLING:
> +               type = REALTEK_GPIO_IRQ_EDGE_FALLING;
> +               break;
> +       case IRQ_TYPE_EDGE_RISING:
> +               type = REALTEK_GPIO_IRQ_EDGE_RISING;
> +               break;
> +       case IRQ_TYPE_EDGE_BOTH:
> +               type = REALTEK_GPIO_IRQ_EDGE_BOTH;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       irq_set_handler_locked(data, handle_edge_irq);
> +
> +       raw_spin_lock_irqsave(&ctrl->lock, flags);
> +       t = ctrl->intr_type[port];
> +       t &= ~realtek_gpio_imr_bits(port_pin, REALTEK_GPIO_IMR_LINE_MASK);
> +       t |= realtek_gpio_imr_bits(port_pin, type);
> +       ctrl->intr_type[port] = t;
> +       realtek_gpio_write_imr(ctrl, port, t, ctrl->intr_mask[port]);
> +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +       return 0;
> +}
> +
> +static void realtek_gpio_irq_handler(struct irq_desc *desc)
> +{
> +       struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> +       struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +       struct irq_chip *irq_chip = irq_desc_get_chip(desc);
> +       unsigned int lines_done;
> +       unsigned int port_pin_count;
> +       unsigned int irq;
> +       unsigned long status;
> +       int offset;
> +
> +       chained_irq_enter(irq_chip, desc);
> +
> +       for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8) {
> +               status = realtek_gpio_read_isr(ctrl, lines_done / 8);
> +               port_pin_count = min(gc->ngpio - lines_done, 8U);
> +               for_each_set_bit(offset, &status, port_pin_count) {
> +                       irq = irq_find_mapping(gc->irq.domain, offset);
> +                       generic_handle_irq(irq);
> +               }
> +       }
> +
> +       chained_irq_exit(irq_chip, desc);
> +}
> +
> +static int realtek_gpio_irq_init(struct gpio_chip *gc)
> +{
> +       struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +       unsigned int port;
> +
> +       for (port = 0; (port * 8) < gc->ngpio; port++) {
> +               realtek_gpio_write_imr(ctrl, port, 0, 0);
> +               realtek_gpio_clear_isr(ctrl, port, GENMASK(7, 0));
> +       }
> +
> +       return 0;
> +}
> +
> +static struct irq_chip realtek_gpio_irq_chip = {
> +       .name = "realtek-otto-gpio",
> +       .irq_ack = realtek_gpio_irq_ack,
> +       .irq_mask = realtek_gpio_irq_mask,
> +       .irq_unmask = realtek_gpio_irq_unmask,
> +       .irq_set_type = realtek_gpio_irq_set_type,
> +};
> +
> +static const struct of_device_id realtek_gpio_of_match[] = {
> +       {
> +               .compatible = "realtek,otto-gpio",
> +               .data = (void *)GPIO_INTERRUPTS_DISABLED,
> +       },
> +       {
> +               .compatible = "realtek,rtl8380-gpio",
> +       },
> +       {
> +               .compatible = "realtek,rtl8390-gpio",
> +       },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
> +
> +static int realtek_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       unsigned int dev_flags;
> +       struct gpio_irq_chip *girq;
> +       struct realtek_gpio_ctrl *ctrl;
> +       u32 ngpios;
> +       int err, irq;
> +
> +       ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +       if (!ctrl)
> +               return -ENOMEM;
> +
> +       dev_flags = (unsigned int) device_get_match_data(dev);
> +
> +       if (device_property_read_u32(dev, "ngpios", &ngpios))
> +               ngpios = REALTEK_GPIO_MAX;

You may actually drop the conditional and do like this:

       ngpios = REALTEK_GPIO_MAX;
       device_property_read_u32(dev, "ngpios", &ngpios);
       if (ngpios > ...)

> +       if (ngpios > REALTEK_GPIO_MAX) {
> +               dev_err(&pdev->dev, "invalid ngpios (max. %d)\n",
> +                       REALTEK_GPIO_MAX);
> +               return -EINVAL;
> +       }
> +
> +       ctrl->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(ctrl->base))
> +               return PTR_ERR(ctrl->base);
> +
> +       raw_spin_lock_init(&ctrl->lock);
> +
> +       err = bgpio_init(&ctrl->gc, dev, 4,
> +               ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL,
> +               ctrl->base + REALTEK_GPIO_REG_DIR, NULL,
> +               BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> +       if (err) {
> +               dev_err(dev, "unable to init generic GPIO");
> +               return err;
> +       }
> +
> +       ctrl->gc.ngpio = ngpios;
> +       ctrl->gc.owner = THIS_MODULE;
> +
> +       irq = platform_get_irq_optional(pdev, 0);
> +       if (!(dev_flags & GPIO_INTERRUPTS_DISABLED) && irq > 0) {
> +               girq = &ctrl->gc.irq;
> +               girq->chip = &realtek_gpio_irq_chip;
> +               girq->default_type = IRQ_TYPE_NONE;
> +               girq->handler = handle_bad_irq;
> +               girq->parent_handler = realtek_gpio_irq_handler;
> +               girq->num_parents = 1;
> +               girq->parents = devm_kcalloc(dev, girq->num_parents,
> +                                       sizeof(*girq->parents), GFP_KERNEL);
> +               if (!girq->parents)
> +                       return -ENOMEM;
> +               girq->parents[0] = irq;
> +               girq->init_hw = realtek_gpio_irq_init;
> +       }
> +
> +       return gpiochip_add_data(&ctrl->gc, ctrl);

Either you have to have ->remove() callback defined or use
devm_gpiochip_add_data() here.

> +}
> +
> +static struct platform_driver realtek_gpio_driver = {
> +       .driver = {
> +               .name = "realtek-otto-gpio",
> +               .of_match_table = realtek_gpio_of_match,
> +       },
> +       .probe = realtek_gpio_probe,
> +};
> +module_platform_driver(realtek_gpio_driver);
> +
> +MODULE_DESCRIPTION("Realtek Otto GPIO support");
> +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.30.2
>


-- 
With Best Regards,
Andy Shevchenko

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

* [PATCH v6 0/2] Add Realtek Otto GPIO support
  2021-03-15  8:23 [PATCH 0/2] Add Realtek Otto GPIO support Sander Vanheule
                   ` (5 preceding siblings ...)
  2021-03-30 15:26 ` [PATCH v5 0/2] " Sander Vanheule
@ 2021-03-30 17:48 ` Sander Vanheule
  2021-03-30 17:48   ` [PATCH v6 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
                     ` (2 more replies)
  6 siblings, 3 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-30 17:48 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: andy.shevchenko, bert, bgolaszewski, linus.walleij, linux-kernel,
	maz, robh+dt, Sander Vanheule

Add support for the GPIO controller employed by Realtek in multiple series of
MIPS SoCs. These include the supported RTL838x and RTL839x. The register layout
also matches the one found in the GPIO controller of other (Lexra-based) SoCs
such as RTL8196E, RTL8197D, and RTL8197F.

For the platform name 'otto', I am not aware of any official resources as to
what hardware this specifically applies to. However, in all of the GPL archives
we've received, from vendors using compatible SoCs in their design, the
platform under the MIPS architecture is referred to by this name.

The GPIO ports have been tested on a Zyxel GS1900-8 (RTL8380), and Zyxel
GS1900-48 (RTL8393). Furthermore, the GPIO ports and interrupt controller have
been tested on a Netgear GS110TPPv1 (RTL8381).

Changes in v6:
- Use devm_gpiochip_add_data()
- Code style for reading ngpios, header order
- Add Andy's Reviewed-by tag

Changes in v5:
- Edited code comments
- Fold functions that were used only once or twice (ISR/IMR accessors)
- Drop trivial functions for line to port/pin calculations
- Use gpio_irq_chip->init_hw() to initialise IRQ registers
- Invert GPIO_INTERRUPTS flag to GPIO_INTERRUPTS_DISABLED
- Support building as module
- Add Rob's Reviewed-by tag

Changes in v4:
- Fix pointer notation style
- Drop unused read_u16_reg() function
- Drop 'inline' specifier from functions

Changes in v3:
- Remove OF dependencies in driver probe
- Don't accept IRQ_TYPE_NONE as a valid interrupt type
- Remove (now unused) dev property from control structure
- Use u8/u16 port registers, instead of raw u32 registers
- Use 'line' name for gpiochip, 'port' and 'pin' names for hardware
- Renamed DT bindings file
- Dropped fallback-only DT compatible
- Various code style clean-ups

Changes in v2:
- Clarify structure and usage of IMR registers
- Added Linus' Reviewed-by tags

Sander Vanheule (2):
  dt-bindings: gpio: Binding for Realtek Otto GPIO
  gpio: Add Realtek Otto GPIO support

 .../bindings/gpio/realtek,otto-gpio.yaml      |  78 +++++
 drivers/gpio/Kconfig                          |  13 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-realtek-otto.c              | 325 ++++++++++++++++++
 4 files changed, 417 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

-- 
2.30.2


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

* [PATCH v6 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO
  2021-03-30 17:48 ` [PATCH v6 0/2] " Sander Vanheule
@ 2021-03-30 17:48   ` Sander Vanheule
  2021-03-30 17:48   ` [PATCH v6 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
  2021-03-31  7:49   ` [PATCH v6 0/2] " Bartosz Golaszewski
  2 siblings, 0 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-30 17:48 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: andy.shevchenko, bert, bgolaszewski, linus.walleij, linux-kernel,
	maz, robh+dt, Sander Vanheule, Rob Herring

Add a binding description for Realtek's GPIO controller found on several
of their MIPS-based SoCs (codenamed Otto), such as the RTL838x and
RTL839x series of switch SoCs.

A fallback binding 'realtek,otto-gpio' is provided for cases where the
actual port ordering is not known yet, and enabling the interrupt
controller may result in uncaught interrupts.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/gpio/realtek,otto-gpio.yaml      | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml b/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
new file mode 100644
index 000000000000..100f20cebd76
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/realtek,otto-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek Otto GPIO controller
+
+maintainers:
+  - Sander Vanheule <sander@svanheule.net>
+  - Bert Vermeulen <bert@biot.com>
+
+description: |
+  Realtek's GPIO controller on their MIPS switch SoCs (Otto platform) consists
+  of two banks of 32 GPIOs. These GPIOs can generate edge-triggered interrupts.
+  Each bank's interrupts are cascased into one interrupt line on the parent
+  interrupt controller, if provided.
+  This binding allows defining a single bank in the devicetree. The interrupt
+  controller is not supported on the fallback compatible name, which only
+  allows for GPIO port use.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    items:
+      - enum:
+          - realtek,rtl8380-gpio
+          - realtek,rtl8390-gpio
+      - const: realtek,otto-gpio
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  ngpios:
+    minimum: 1
+    maximum: 32
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+dependencies:
+  interrupt-controller: [ interrupts ]
+
+examples:
+  - |
+      gpio@3500 {
+        compatible = "realtek,rtl8380-gpio", "realtek,otto-gpio";
+        reg = <0x3500 0x1c>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        ngpios = <24>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&rtlintc>;
+        interrupts = <23>;
+      };
+
+...
-- 
2.30.2


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

* [PATCH v6 2/2] gpio: Add Realtek Otto GPIO support
  2021-03-30 17:48 ` [PATCH v6 0/2] " Sander Vanheule
  2021-03-30 17:48   ` [PATCH v6 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
@ 2021-03-30 17:48   ` Sander Vanheule
  2021-03-31  7:49   ` [PATCH v6 0/2] " Bartosz Golaszewski
  2 siblings, 0 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-30 17:48 UTC (permalink / raw)
  To: devicetree, linux-gpio
  Cc: andy.shevchenko, bert, bgolaszewski, linus.walleij, linux-kernel,
	maz, robh+dt, Sander Vanheule

Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
64 GPIOs, divided over two banks. Each bank has a set of registers for
32 GPIOs, with support for edge-triggered interrupts.

Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
registers pack one bit per GPIO, except for the IMR register, which
packs two bits per GPIO (AB-CD).

Although the byte order is currently assumed to have port A..D at offset
0x0..0x3, this has been observed to be reversed on other, Lexra-based,
SoCs (e.g. RTL8196E/97D/97F).

Interrupt support is disabled for the fallback devicetree-compatible
'realtek,otto-gpio'. This allows for quick support of GPIO banks in
which the byte order would be unknown. In this case, the port ordering
in the IMR registers may not match the reversed order in the other
registers (DCBA, and BA-DC or DC-BA).

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/gpio/Kconfig             |  13 ++
 drivers/gpio/Makefile            |   1 +
 drivers/gpio/gpio-realtek-otto.c | 325 +++++++++++++++++++++++++++++++
 3 files changed, 339 insertions(+)
 create mode 100644 drivers/gpio/gpio-realtek-otto.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..6fb13d6507db 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -502,6 +502,19 @@ config GPIO_RDA
 	help
 	  Say Y here to support RDA Micro GPIO controller.
 
+config GPIO_REALTEK_OTTO
+	tristate "Realtek Otto GPIO support"
+	depends on MACH_REALTEK_RTL
+	default MACH_REALTEK_RTL
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	help
+	  The GPIO controller on the Otto MIPS platform supports up to two
+	  banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
+	  are grouped in four 8-bit wide ports.
+
+	  When built as a module, the module will be called realtek_otto_gpio.
+
 config GPIO_REG
 	bool
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..8ace5934e3c3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583)		+= gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RCAR)			+= gpio-rcar.o
 obj-$(CONFIG_GPIO_RDA)			+= gpio-rda.o
 obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
+obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
new file mode 100644
index 000000000000..cb64fb5a51aa
--- /dev/null
+++ b/drivers/gpio/gpio-realtek-otto.c
@@ -0,0 +1,325 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/gpio/driver.h>
+#include <linux/irq.h>
+#include <linux/minmax.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+/*
+ * Total register block size is 0x1C for one bank of four ports (A, B, C, D).
+ * An optional second bank, with ports E, F, G, and H, may be present, starting
+ * at register offset 0x1C.
+ */
+
+/*
+ * Pin select: (0) "normal", (1) "dedicate peripheral"
+ * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits
+ * in the peripheral registers.
+ */
+#define REALTEK_GPIO_REG_CNR		0x00
+/* Clear bit (0) for input, set bit (1) for output */
+#define REALTEK_GPIO_REG_DIR		0x08
+#define REALTEK_GPIO_REG_DATA		0x0C
+/* Read bit for IRQ status, write 1 to clear IRQ */
+#define REALTEK_GPIO_REG_ISR		0x10
+/* Two bits per GPIO in IMR registers */
+#define REALTEK_GPIO_REG_IMR		0x14
+#define REALTEK_GPIO_REG_IMR_AB		0x14
+#define REALTEK_GPIO_REG_IMR_CD		0x18
+#define REALTEK_GPIO_IMR_LINE_MASK	GENMASK(1, 0)
+#define REALTEK_GPIO_IRQ_EDGE_FALLING	1
+#define REALTEK_GPIO_IRQ_EDGE_RISING	2
+#define REALTEK_GPIO_IRQ_EDGE_BOTH	3
+
+#define REALTEK_GPIO_MAX		32
+#define REALTEK_GPIO_PORTS_PER_BANK	4
+
+/**
+ * realtek_gpio_ctrl - Realtek Otto GPIO driver data
+ *
+ * @gc: Associated gpio_chip instance
+ * @base: Base address of the register block for a GPIO bank
+ * @lock: Lock for accessing the IRQ registers and values
+ * @intr_mask: Mask for interrupts lines
+ * @intr_type: Interrupt type selection
+ *
+ * Because the interrupt mask register (IMR) combines the function of IRQ type
+ * selection and masking, two extra values are stored. @intr_mask is used to
+ * mask/unmask the interrupts for a GPIO port, and @intr_type is used to store
+ * the selected interrupt types. The logical AND of these values is written to
+ * IMR on changes.
+ */
+struct realtek_gpio_ctrl {
+	struct gpio_chip gc;
+	void __iomem *base;
+	raw_spinlock_t lock;
+	u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK];
+	u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK];
+};
+
+/* Expand with more flags as devices with other quirks are added */
+enum realtek_gpio_flags {
+	/*
+	 * Allow disabling interrupts, for cases where the port order is
+	 * unknown. This may result in a port mismatch between ISR and IMR.
+	 * An interrupt would appear to come from a different line than the
+	 * line the IRQ handler was assigned to, causing uncaught interrupts.
+	 */
+	GPIO_INTERRUPTS_DISABLED = BIT(0),
+};
+
+static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+
+	return container_of(gc, struct realtek_gpio_ctrl, gc);
+}
+
+/*
+ * Normal port order register access
+ *
+ * Port information is stored with the first port at offset 0, followed by the
+ * second, etc. Most registers store one bit per GPIO and use a u8 value per
+ * port. The two interrupt mask registers store two bits per GPIO, so use u16
+ * values.
+ */
+static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl,
+	unsigned int port, u16 irq_type, u16 irq_mask)
+{
+	iowrite16(irq_type & irq_mask, ctrl->base + REALTEK_GPIO_REG_IMR + 2 * port);
+}
+
+static void realtek_gpio_clear_isr(struct realtek_gpio_ctrl *ctrl,
+	unsigned int port, u8 mask)
+{
+	iowrite8(mask, ctrl->base + REALTEK_GPIO_REG_ISR + port);
+}
+
+static u8 realtek_gpio_read_isr(struct realtek_gpio_ctrl *ctrl, unsigned int port)
+{
+	return ioread8(ctrl->base + REALTEK_GPIO_REG_ISR + port);
+}
+
+/* Set the rising and falling edge mask bits for a GPIO port pin */
+static u16 realtek_gpio_imr_bits(unsigned int pin, u16 value)
+{
+	return (value & REALTEK_GPIO_IMR_LINE_MASK) << 2 * pin;
+}
+
+static void realtek_gpio_irq_ack(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	irq_hw_number_t line = irqd_to_hwirq(data);
+	unsigned int port = line / 8;
+	unsigned int port_pin = line % 8;
+
+	realtek_gpio_clear_isr(ctrl, port, BIT(port_pin));
+}
+
+static void realtek_gpio_irq_unmask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line / 8;
+	unsigned int port_pin = line % 8;
+	unsigned long flags;
+	u16 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[port];
+	m |= realtek_gpio_imr_bits(port_pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[port] = m;
+	realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static void realtek_gpio_irq_mask(struct irq_data *data)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line / 8;
+	unsigned int port_pin = line % 8;
+	unsigned long flags;
+	u16 m;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	m = ctrl->intr_mask[port];
+	m &= ~realtek_gpio_imr_bits(port_pin, REALTEK_GPIO_IMR_LINE_MASK);
+	ctrl->intr_mask[port] = m;
+	realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static int realtek_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
+	unsigned int line = irqd_to_hwirq(data);
+	unsigned int port = line / 8;
+	unsigned int port_pin = line % 8;
+	unsigned long flags;
+	u16 type, t;
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_FALLING:
+		type = REALTEK_GPIO_IRQ_EDGE_FALLING;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		type = REALTEK_GPIO_IRQ_EDGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		type = REALTEK_GPIO_IRQ_EDGE_BOTH;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	irq_set_handler_locked(data, handle_edge_irq);
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	t = ctrl->intr_type[port];
+	t &= ~realtek_gpio_imr_bits(port_pin, REALTEK_GPIO_IMR_LINE_MASK);
+	t |= realtek_gpio_imr_bits(port_pin, type);
+	ctrl->intr_type[port] = t;
+	realtek_gpio_write_imr(ctrl, port, t, ctrl->intr_mask[port]);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static void realtek_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+	unsigned int lines_done;
+	unsigned int port_pin_count;
+	unsigned int irq;
+	unsigned long status;
+	int offset;
+
+	chained_irq_enter(irq_chip, desc);
+
+	for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8) {
+		status = realtek_gpio_read_isr(ctrl, lines_done / 8);
+		port_pin_count = min(gc->ngpio - lines_done, 8U);
+		for_each_set_bit(offset, &status, port_pin_count) {
+			irq = irq_find_mapping(gc->irq.domain, offset);
+			generic_handle_irq(irq);
+		}
+	}
+
+	chained_irq_exit(irq_chip, desc);
+}
+
+static int realtek_gpio_irq_init(struct gpio_chip *gc)
+{
+	struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	unsigned int port;
+
+	for (port = 0; (port * 8) < gc->ngpio; port++) {
+		realtek_gpio_write_imr(ctrl, port, 0, 0);
+		realtek_gpio_clear_isr(ctrl, port, GENMASK(7, 0));
+	}
+
+	return 0;
+}
+
+static struct irq_chip realtek_gpio_irq_chip = {
+	.name = "realtek-otto-gpio",
+	.irq_ack = realtek_gpio_irq_ack,
+	.irq_mask = realtek_gpio_irq_mask,
+	.irq_unmask = realtek_gpio_irq_unmask,
+	.irq_set_type = realtek_gpio_irq_set_type,
+};
+
+static const struct of_device_id realtek_gpio_of_match[] = {
+	{
+		.compatible = "realtek,otto-gpio",
+		.data = (void *)GPIO_INTERRUPTS_DISABLED,
+	},
+	{
+		.compatible = "realtek,rtl8380-gpio",
+	},
+	{
+		.compatible = "realtek,rtl8390-gpio",
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
+
+static int realtek_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int dev_flags;
+	struct gpio_irq_chip *girq;
+	struct realtek_gpio_ctrl *ctrl;
+	u32 ngpios;
+	int err, irq;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	dev_flags = (unsigned int) device_get_match_data(dev);
+
+	ngpios = REALTEK_GPIO_MAX;
+	device_property_read_u32(dev, "ngpios", &ngpios);
+
+	if (ngpios > REALTEK_GPIO_MAX) {
+		dev_err(&pdev->dev, "invalid ngpios (max. %d)\n",
+			REALTEK_GPIO_MAX);
+		return -EINVAL;
+	}
+
+	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ctrl->base))
+		return PTR_ERR(ctrl->base);
+
+	raw_spin_lock_init(&ctrl->lock);
+
+	err = bgpio_init(&ctrl->gc, dev, 4,
+		ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL,
+		ctrl->base + REALTEK_GPIO_REG_DIR, NULL,
+		BGPIOF_BIG_ENDIAN_BYTE_ORDER);
+	if (err) {
+		dev_err(dev, "unable to init generic GPIO");
+		return err;
+	}
+
+	ctrl->gc.ngpio = ngpios;
+	ctrl->gc.owner = THIS_MODULE;
+
+	irq = platform_get_irq_optional(pdev, 0);
+	if (!(dev_flags & GPIO_INTERRUPTS_DISABLED) && irq > 0) {
+		girq = &ctrl->gc.irq;
+		girq->chip = &realtek_gpio_irq_chip;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_bad_irq;
+		girq->parent_handler = realtek_gpio_irq_handler;
+		girq->num_parents = 1;
+		girq->parents = devm_kcalloc(dev, girq->num_parents,
+					sizeof(*girq->parents),	GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+		girq->parents[0] = irq;
+		girq->init_hw = realtek_gpio_irq_init;
+	}
+
+	return devm_gpiochip_add_data(dev, &ctrl->gc, ctrl);
+}
+
+static struct platform_driver realtek_gpio_driver = {
+	.driver = {
+		.name = "realtek-otto-gpio",
+		.of_match_table	= realtek_gpio_of_match,
+	},
+	.probe = realtek_gpio_probe,
+};
+module_platform_driver(realtek_gpio_driver);
+
+MODULE_DESCRIPTION("Realtek Otto GPIO support");
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2


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

* Re: [PATCH v6 0/2] Add Realtek Otto GPIO support
  2021-03-30 17:48 ` [PATCH v6 0/2] " Sander Vanheule
  2021-03-30 17:48   ` [PATCH v6 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
  2021-03-30 17:48   ` [PATCH v6 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
@ 2021-03-31  7:49   ` Bartosz Golaszewski
  2021-03-31  8:11     ` Sander Vanheule
  2 siblings, 1 reply; 39+ messages in thread
From: Bartosz Golaszewski @ 2021-03-31  7:49 UTC (permalink / raw)
  To: Sander Vanheule
  Cc: linux-devicetree, linux-gpio, Andy Shevchenko, Bert Vermeulen,
	Linus Walleij, LKML, Marc Zyngier, Rob Herring

On Tue, Mar 30, 2021 at 7:48 PM Sander Vanheule <sander@svanheule.net> wrote:
>
> Add support for the GPIO controller employed by Realtek in multiple series of
> MIPS SoCs. These include the supported RTL838x and RTL839x. The register layout
> also matches the one found in the GPIO controller of other (Lexra-based) SoCs
> such as RTL8196E, RTL8197D, and RTL8197F.
>
> For the platform name 'otto', I am not aware of any official resources as to
> what hardware this specifically applies to. However, in all of the GPL archives
> we've received, from vendors using compatible SoCs in their design, the
> platform under the MIPS architecture is referred to by this name.
>
> The GPIO ports have been tested on a Zyxel GS1900-8 (RTL8380), and Zyxel
> GS1900-48 (RTL8393). Furthermore, the GPIO ports and interrupt controller have
> been tested on a Netgear GS110TPPv1 (RTL8381).
>
> Changes in v6:
> - Use devm_gpiochip_add_data()
> - Code style for reading ngpios, header order
> - Add Andy's Reviewed-by tag
>
> Changes in v5:
> - Edited code comments
> - Fold functions that were used only once or twice (ISR/IMR accessors)
> - Drop trivial functions for line to port/pin calculations
> - Use gpio_irq_chip->init_hw() to initialise IRQ registers
> - Invert GPIO_INTERRUPTS flag to GPIO_INTERRUPTS_DISABLED
> - Support building as module
> - Add Rob's Reviewed-by tag
>
> Changes in v4:
> - Fix pointer notation style
> - Drop unused read_u16_reg() function
> - Drop 'inline' specifier from functions
>
> Changes in v3:
> - Remove OF dependencies in driver probe
> - Don't accept IRQ_TYPE_NONE as a valid interrupt type
> - Remove (now unused) dev property from control structure
> - Use u8/u16 port registers, instead of raw u32 registers
> - Use 'line' name for gpiochip, 'port' and 'pin' names for hardware
> - Renamed DT bindings file
> - Dropped fallback-only DT compatible
> - Various code style clean-ups
>
> Changes in v2:
> - Clarify structure and usage of IMR registers
> - Added Linus' Reviewed-by tags
>
> Sander Vanheule (2):
>   dt-bindings: gpio: Binding for Realtek Otto GPIO
>   gpio: Add Realtek Otto GPIO support
>
>  .../bindings/gpio/realtek,otto-gpio.yaml      |  78 +++++
>  drivers/gpio/Kconfig                          |  13 +
>  drivers/gpio/Makefile                         |   1 +
>  drivers/gpio/gpio-realtek-otto.c              | 325 ++++++++++++++++++
>  4 files changed, 417 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
>  create mode 100644 drivers/gpio/gpio-realtek-otto.c
>
> --
> 2.30.2
>

Series applied, thanks!

Bartosz

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

* Re: [PATCH v6 0/2] Add Realtek Otto GPIO support
  2021-03-31  7:49   ` [PATCH v6 0/2] " Bartosz Golaszewski
@ 2021-03-31  8:11     ` Sander Vanheule
  0 siblings, 0 replies; 39+ messages in thread
From: Sander Vanheule @ 2021-03-31  8:11 UTC (permalink / raw)
  To: Bartosz Golaszewski, Andy Shevchenko, Linus Walleij, Rob Herring
  Cc: linux-devicetree, linux-gpio, Bert Vermeulen, LKML, Marc Zyngier

On Wed, 2021-03-31 at 09:49 +0200, Bartosz Golaszewski wrote:
> On Tue, Mar 30, 2021 at 7:48 PM Sander Vanheule
> <sander@svanheule.net> wrote:
> > 
> > Add support for the GPIO controller employed by Realtek in multiple
> > series of MIPS SoCs. These include the supported RTL838x and
> > RTL839x. The register layout also matches the one found in the GPIO
> > controller of other (Lexra-based) SoCs such as RTL8196E, RTL8197D,
> > and RTL8197F.
> 
> Series applied, thanks!

Thanks for merging, and thanks for the discussion everyone!

Best,
Sander



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

end of thread, other threads:[~2021-03-31  8:12 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15  8:23 [PATCH 0/2] Add Realtek Otto GPIO support Sander Vanheule
2021-03-15  8:23 ` [PATCH 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
2021-03-15 15:01   ` Linus Walleij
2021-03-15  8:23 ` [PATCH 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
2021-03-15 15:10   ` Linus Walleij
2021-03-15 19:09     ` Sander Vanheule
2021-03-15 19:08 ` [PATCH v2 0/2] " Sander Vanheule
2021-03-15 19:08   ` [PATCH v2 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
2021-03-24 18:29     ` Rob Herring
2021-03-15 19:08   ` [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
2021-03-17 13:08     ` Andy Shevchenko
2021-03-19 15:51       ` Sander Vanheule
2021-03-19 17:57         ` Andy Shevchenko
2021-03-19 21:20           ` Sander Vanheule
2021-03-19 21:24             ` Andy Shevchenko
2021-03-19 21:48               ` Sander Vanheule
2021-03-22 13:16                 ` Andy Shevchenko
2021-03-24 21:22 ` [PATCH v3 0/2] " Sander Vanheule
2021-03-24 21:22   ` [PATCH v3 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
2021-03-24 21:22   ` [PATCH v3 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
2021-03-24 21:29     ` Sander Vanheule
2021-03-26 12:03 ` [PATCH v4 0/2] " Sander Vanheule
2021-03-26 12:03   ` [PATCH v4 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
2021-03-27 18:16     ` Rob Herring
2021-03-26 12:03   ` [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
2021-03-26 18:19     ` Andy Shevchenko
2021-03-26 21:11       ` Sander Vanheule
2021-03-29 10:26         ` Andy Shevchenko
2021-03-29 17:28           ` Sander Vanheule
2021-03-30 10:14             ` Andy Shevchenko
2021-03-30 15:26 ` [PATCH v5 0/2] " Sander Vanheule
2021-03-30 15:26   ` [PATCH v5 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
2021-03-30 15:26   ` [PATCH v5 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
2021-03-30 16:43     ` Andy Shevchenko
2021-03-30 17:48 ` [PATCH v6 0/2] " Sander Vanheule
2021-03-30 17:48   ` [PATCH v6 1/2] dt-bindings: gpio: Binding for Realtek Otto GPIO Sander Vanheule
2021-03-30 17:48   ` [PATCH v6 2/2] gpio: Add Realtek Otto GPIO support Sander Vanheule
2021-03-31  7:49   ` [PATCH v6 0/2] " Bartosz Golaszewski
2021-03-31  8:11     ` Sander Vanheule

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.