NetBSD Problem Report #57597

From cypheon@beltix.localdomain  Sat Sep  2 11:32:13 2023
Return-Path: <cypheon@beltix.localdomain>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 35CE51A923A
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  2 Sep 2023 11:32:13 +0000 (UTC)
Message-Id: <20230902113204.96E95109E7@beltix.localdomain>
Date: Sat,  2 Sep 2023 13:32:04 +0200 (CEST)
From: johann@sinyax.net
Reply-To: johann@sinyax.net
To: gnats-bugs@NetBSD.org
Subject: [PATCH] rk_gpio: Implement GPIO v2
X-Send-Pr-Version: 3.95

>Number:         57597
>Category:       port-evbarm
>Synopsis:       rk_gpio: Implement GPIO v2 (on rk3588)
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    tnn
>State:          closed
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Sat Sep 02 11:35:00 +0000 2023
>Closed-Date:    Tue Oct 17 17:33:59 +0000 2023
>Last-Modified:  Tue Oct 17 17:35:01 +0000 2023
>Originator:     Johann Rudloff
>Release:        NetBSD 10.99.7, code from 2023-08-23
>Organization:
>Environment:
System: NetBSD arm64 10.99.7 NetBSD 10.99.7 (ROCK5B) #221: Thu Aug 31 21:25:39 CEST 2023  cypheon@beltix:/home/cypheon/obj-aarch64/sys/arch/evbarm/compile/ROCK5B evbarm
Architecture: aarch64
Machine: evbarm

>Description:
Rockchip SoCs rk3568, rk3588, and probably others contain a slightly
different GPIO controller.  The main difference is, that half of each
(32-bit) DR/DDR register is used by the write-enable flags.
This means that:
 a) The 32 pins are spread over 2 registers,
 b) Read-modify-write cycle (and thus locking) is no longer needed.

With this commit, the applicable version is read from a dedicated
register at attach time.  In case of an unrecognized version a warning
is printed, but otherwise behaviour is unchanged ("version 1" mode is
used).

>How-To-Repeat:
It is currently difficult to test this patch in isolation on the rk3588, as
overall rk3588 support in HEAD is not that far.  In my current development
setup this code is able to switch on USB host power on the ROCK 5B to enable
USB accessories (together with other patches).

To prevent regressions, I tested the behaviour on a ROCK64 (rk3328) to make
sure the correct "v1" path is selected by checking dmesg.  Additionally
verified by using `gpioctl` to manually toggle a GPIO pin and checking with a
connected voltmeter.

>Fix:
URL: https://git.sr.ht/~cypheon/NetBSD/commit/922ac9659892b492e5f9560b1e7181d371e26b43.patch

 sys/arch/arm/rockchip/rk_gpio.c | 150 +++++++++++++++++++++++++-------
 1 file changed, 120 insertions(+), 30 deletions(-)

diff --git a/sys/arch/arm/rockchip/rk_gpio.c b/sys/arch/arm/rockchip/rk_gpio.c
index edd3e138fb14..9f5e6a803093 100644
--- a/sys/arch/arm/rockchip/rk_gpio.c
+++ b/sys/arch/arm/rockchip/rk_gpio.c
@@ -55,12 +55,38 @@ __KERNEL_RCSID(0, "$NetBSD: rk_gpio.c,v 1.5 2021/08/07 16:18:45 thorpej Exp $");
 #define	GPIO_PORTA_EOI_REG		0x004c
 #define	GPIO_EXT_PORTA_REG		0x0050
 #define	GPIO_LS_SYNC_REG		0x0060
+#define	GPIO_VER_ID_REG			0x0078
+#define	GPIO_VER_ID_GPIOV2		0x0101157c
+
+
+/*
+ * In "version 2" GPIO controllers, half of each register is used by the
+ * write_enable mask, so the 32 pins are spread over two registers.
+ *
+ * pins  0 - 15 go into the GPIO_SWPORT_*_L register
+ * pins 16 - 31 go into the GPIO_SWPORT_*_H register
+ */
+#define GPIOV2_SWPORT_DR_BASE		0x0000
+#define GPIOV2_SWPORT_DR_REG(pin)	(\
+	GPIOV2_SWPORT_DR_BASE + GPIOV2_REG_OFFSET(pin))
+#define GPIOV2_SWPORT_DDR_BASE		0x0008
+#define GPIOV2_SWPORT_DDR_REG(pin)	(\
+	GPIOV2_SWPORT_DDR_BASE + GPIOV2_REG_OFFSET(pin))
+#define	GPIOV2_EXT_PORT_REG		0x0070
+#define GPIOV2_REG_OFFSET(pin)		(4 * ((pin) / 16))
+#define GPIOV2_DATA_MASK(pin)		(__BIT((pin) % 16))
+#define GPIOV2_WRITE_MASK(pin)		(__BIT(((pin) % 16) + 16))

 static const struct device_compatible_entry compat_data[] = {
 	{ .compat = "rockchip,gpio-bank" },
 	DEVICE_COMPAT_EOL
 };

+enum rk_gpio_version {
+	GPIOV1 = 1,
+	GPIOV2
+};
+
 struct rk_gpio_softc {
 	device_t sc_dev;
 	bus_space_tag_t sc_bst;
@@ -70,6 +96,7 @@ struct rk_gpio_softc {
 	struct gpio_chipset_tag sc_gp;
 	gpio_pin_t sc_pins[32];
 	device_t sc_gpiodev;
+	enum rk_gpio_version sc_version;
 };

 struct rk_gpio_pin {
@@ -93,18 +120,37 @@ CFATTACH_DECL_NEW(rk_gpio, sizeof(struct rk_gpio_softc),
 static int
 rk_gpio_ctl(struct rk_gpio_softc *sc, u_int pin, int flags)
 {
+	uint32_t reg, write_mask;
 	uint32_t ddr;

 	KASSERT(mutex_owned(&sc->sc_lock));

-	ddr = RD4(sc, GPIO_SWPORTA_DDR_REG);
-	if (flags & GPIO_PIN_INPUT)
-		ddr &= ~__BIT(pin);
-	else if (flags & GPIO_PIN_OUTPUT)
-		ddr |= __BIT(pin);
-	WR4(sc, GPIO_SWPORTA_DDR_REG, ddr);
+	if (sc->sc_version == GPIOV1) {
+		ddr = RD4(sc, GPIO_SWPORTA_DDR_REG);
+		if (flags & GPIO_PIN_INPUT)
+			ddr &= ~__BIT(pin);
+		else if (flags & GPIO_PIN_OUTPUT)
+			ddr |= __BIT(pin);
+		WR4(sc, GPIO_SWPORTA_DDR_REG, ddr);
+
+		return 0;
+	} else if (sc->sc_version == GPIOV2) {
+		reg = GPIOV2_SWPORT_DDR_REG(pin);
+		write_mask = GPIOV2_WRITE_MASK(pin);
+
+		if (flags & GPIO_PIN_INPUT)
+			ddr = 0;
+		else if (flags & GPIO_PIN_OUTPUT)
+			ddr = GPIOV2_DATA_MASK(pin);
+		else
+			return EINVAL;
+
+		WR4(sc, reg, write_mask | ddr);
+
+		return 0;
+	}

-	return 0;
+	return ENXIO;
 }

 static void *
@@ -158,15 +204,22 @@ rk_gpio_read(device_t dev, void *priv, bool raw)
 {
 	struct rk_gpio_softc * const sc = device_private(dev);
 	struct rk_gpio_pin *pin = priv;
-	uint32_t data;
+	uint32_t data, reg;
 	int val;

 	KASSERT(sc == pin->pin_sc);

 	const uint32_t data_mask = __BIT(pin->pin_nr);

+	if (sc->sc_version == GPIOV1)
+		reg = GPIO_EXT_PORTA_REG;
+	else if (sc->sc_version == GPIOV2)
+		reg = GPIOV2_EXT_PORT_REG;
+	else
+		return 0;
+
 	/* No lock required for reads */
-	data = RD4(sc, GPIO_EXT_PORTA_REG);
+	data = RD4(sc, reg);
 	val = __SHIFTOUT(data, data_mask);
 	if (!raw && pin->pin_actlo)
 		val = !val;
@@ -179,7 +232,7 @@ rk_gpio_write(device_t dev, void *priv, int val, bool raw)
 {
 	struct rk_gpio_softc * const sc = device_private(dev);
 	struct rk_gpio_pin *pin = priv;
-	uint32_t data;
+	uint32_t data, reg, write_mask;

 	KASSERT(sc == pin->pin_sc);

@@ -188,14 +241,22 @@ rk_gpio_write(device_t dev, void *priv, int val, bool raw)
 	if (!raw && pin->pin_actlo)
 		val = !val;

-	mutex_enter(&sc->sc_lock);
-	data = RD4(sc, GPIO_SWPORTA_DR_REG);
-	if (val)
-		data |= data_mask;
-	else
-		data &= ~data_mask;
-	WR4(sc, GPIO_SWPORTA_DR_REG, data);
-	mutex_exit(&sc->sc_lock);
+
+	if (sc->sc_version == GPIOV1) {
+		mutex_enter(&sc->sc_lock);
+		data = RD4(sc, GPIO_SWPORTA_DR_REG);
+		if (val)
+			data |= data_mask;
+		else
+			data &= ~data_mask;
+		WR4(sc, GPIO_SWPORTA_DR_REG, data);
+		mutex_exit(&sc->sc_lock);
+	} else if (sc->sc_version == GPIOV2) {
+		reg = GPIOV2_SWPORT_DR_REG(pin->pin_nr);
+		write_mask = GPIOV2_WRITE_MASK(pin->pin_nr);
+		data = val ? GPIOV2_DATA_MASK(pin->pin_nr) : 0;
+		WR4(sc, reg, write_mask | data);
+	}
 }

 static struct fdtbus_gpio_controller_func rk_gpio_funcs = {
@@ -209,15 +270,22 @@ static int
 rk_gpio_pin_read(void *priv, int pin)
 {
 	struct rk_gpio_softc * const sc = priv;
-	uint32_t data;
+	uint32_t data, reg;
 	int val;

 	KASSERT(pin < __arraycount(sc->sc_pins));

 	const uint32_t data_mask = __BIT(pin);

+	if (sc->sc_version == GPIOV1)
+		reg = GPIO_EXT_PORTA_REG;
+	else if (sc->sc_version == GPIOV2)
+		reg = GPIOV2_EXT_PORT_REG;
+	else
+		return 0;
+
 	/* No lock required for reads */
-	data = RD4(sc, GPIO_EXT_PORTA_REG);
+	data = RD4(sc, reg);
 	val = __SHIFTOUT(data, data_mask);

 	return val;
@@ -227,20 +295,27 @@ static void
 rk_gpio_pin_write(void *priv, int pin, int val)
 {
 	struct rk_gpio_softc * const sc = priv;
-	uint32_t data;
+	uint32_t data, reg, write_mask;

 	KASSERT(pin < __arraycount(sc->sc_pins));

 	const uint32_t data_mask = __BIT(pin);

-	mutex_enter(&sc->sc_lock);
-	data = RD4(sc, GPIO_SWPORTA_DR_REG);
-	if (val)
-		data |= data_mask;
-	else
-		data &= ~data_mask;
-	WR4(sc, GPIO_SWPORTA_DR_REG, data);
-	mutex_exit(&sc->sc_lock);
+	if (sc->sc_version == GPIOV1) {
+		mutex_enter(&sc->sc_lock);
+		data = RD4(sc, GPIO_SWPORTA_DR_REG);
+		if (val)
+			data |= data_mask;
+		else
+			data &= ~data_mask;
+		WR4(sc, GPIO_SWPORTA_DR_REG, data);
+		mutex_exit(&sc->sc_lock);
+	} else if (sc->sc_version == GPIOV2) {
+		reg = GPIOV2_SWPORT_DR_REG(pin);
+		write_mask = GPIOV2_WRITE_MASK(pin);
+		data = val ? GPIOV2_DATA_MASK(pin) : 0;
+		WR4(sc, reg, write_mask | data);
+	}
 }

 static void
@@ -294,6 +369,7 @@ rk_gpio_attach(device_t parent, device_t self, void *aux)
 	struct rk_gpio_softc * const sc = device_private(self);
 	struct fdt_attach_args * const faa = aux;
 	const int phandle = faa->faa_phandle;
+	uint32_t ver_id;
 	struct clk *clk;
 	bus_addr_t addr;
 	bus_size_t size;
@@ -314,10 +390,24 @@ rk_gpio_attach(device_t parent, device_t self, void *aux)
 		aprint_error(": couldn't map registers\n");
 		return;
 	}
+
+	ver_id = RD4(sc, GPIO_VER_ID_REG);
+	/* In version 1, the register is undocumented, but reads as 0. */
+	if (ver_id == 0) {
+		sc->sc_version = GPIOV1;
+	} else if (ver_id == GPIO_VER_ID_GPIOV2) {
+		sc->sc_version = GPIOV2;
+	} else {
+		aprint_normal("warning: unknown version ID: %08" PRIx32
+				", treating as v1\n", ver_id);
+		sc->sc_version = GPIOV1;
+	}
+
 	mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_VM);

 	aprint_naive("\n");
-	aprint_normal(": GPIO (%s)\n", fdtbus_get_string(phandle, "name"));
+	aprint_normal(": GPIO v%d (%s)\n", sc->sc_version,
+			fdtbus_get_string(phandle, "name"));

 	fdtbus_register_gpio_controller(self, phandle, &rk_gpio_funcs);

-- 
2.40.1

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: port-evbarm-maintainer->tnn
Responsible-Changed-By: tnn@NetBSD.org
Responsible-Changed-When: Mon, 16 Oct 2023 21:35:45 +0000
Responsible-Changed-Why:
take


State-Changed-From-To: open->closed
State-Changed-By: tnn@NetBSD.org
State-Changed-When: Tue, 17 Oct 2023 17:33:59 +0000
State-Changed-Why:
Committed with some cosmetic changes. Thanks!


From: "Tobias Nygren" <tnn@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57597 CVS commit: src/sys/arch/arm/rockchip
Date: Tue, 17 Oct 2023 17:31:12 +0000

 Module Name:	src
 Committed By:	tnn
 Date:		Tue Oct 17 17:31:12 UTC 2023

 Modified Files:
 	src/sys/arch/arm/rockchip: rk_gpio.c

 Log Message:
 rk_gpio: add support for version 2 controller

 Based on PR 57597 from Johann Rudloff.


 To generate a diff of this commit:
 cvs rdiff -u -r1.6 -r1.7 src/sys/arch/arm/rockchip/rk_gpio.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

Home	PR Database Search