[PATCH v2 0/2] Meson8b: fix requesting GPIOs greater than GPIOZ_3

[PATCH v2 0/2] Meson8b: fix requesting GPIOs greater than GPIOZ_3

[Martin Blumenstingl]

GPIOs greater GPIOZ_3 (whose ID is 83 - as a reminder: this is exactly
the number of actual GPIO lines on Meson8b) could not be requested. Using
CARD_6 (ID 100, "base of the GPIO controller is 382) as an example:
$ echo 482 > /sys/class/gpio/export
export_store: invalid GPIO 482

This was discovered by Linus L?ssing when he was adding SD card support
on the Odroid-C1.


changes since RFC v1 at [0]:
- dropped RFC prefix
- instead of adding a hack to override the "ngpio" field this now splits
  the register definitions as suggested by Jerome (and based on the
  discussion of RFC v1). see the description of PATCH #1 for a more
  detailed explanation


@Kevin: PATCH #2 is based on my other patch from [1]:
"ARM: dts: meson8b: add the I2C clocks"


[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006259.html
[1] http://lists.infradead.org/pipermail/linux-amlogic/2018-February/006473.html


Martin Blumenstingl (2):
  pinctrl: meson: meson8b: fix requesting GPIOs greater than GPIOZ_3
  ARM: dts: meson8b: the CBUS GPIO controller only has 83 GPIOs

 arch/arm/boot/dts/meson8b.dtsi          |   2 +-
 drivers/pinctrl/meson/pinctrl-meson8b.c |  20 +++---
 include/dt-bindings/gpio/meson8b-gpio.h | 121 ++++++++++++++++++++++++++++----
 3 files changed, 121 insertions(+), 22 deletions(-)

-- 
2.16.2

[PATCH] Meson8b GPIO fix - WiP

[Martin Blumenstingl]

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8b.dtsi          |   2 +-
 drivers/pinctrl/meson/pinctrl-meson8b.c |  20 +++---
 include/dt-bindings/gpio/meson8b-gpio.h | 121 ++++++++++++++++++++++++++++----
 3 files changed, 121 insertions(+), 22 deletions(-)

diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index 1a7c16640ea5..144dc3b750b6 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -183,7 +183,7 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
-			gpio-ranges = <&pinctrl_cbus 0 0 130>;
+			gpio-ranges = <&pinctrl_cbus 0 0 83>;
 		};
 
 		eth_rgmii_pins: eth-rgmii {
diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c
index 5bd808dc81e1..bb2a30964fc6 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8b.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8b.c
@@ -884,20 +884,24 @@ static struct meson_pmx_func meson8b_aobus_functions[] = {
 };
 
 static struct meson_bank meson8b_cbus_banks[] = {
-	/*   name    first              last        irq      pullen  pull    dir     out     in  */
-	BANK("X",    GPIOX_0,		GPIOX_21,   97, 118, 4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
-	BANK("Y",    GPIOY_0,		GPIOY_14,   80,  96, 3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
-	BANK("DV",   GPIODV_9,		GPIODV_29,  59,  79, 0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
-	BANK("H",    GPIOH_0,		GPIOH_9,    14,  23, 1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
-	BANK("CARD", CARD_0,		CARD_6,     43,  49, 2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
-	BANK("BOOT", BOOT_0,		BOOT_18,    24,  42, 2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
+	/*   name        first          last        irq       pullen   pull     dir      out      in   */
+	BANK("X0..11",	 GPIOX_0,	GPIOX_11,   97, 108,  4,  0,   4,  0,   0,  0,   1,  0,   2,  0),
+	BANK("X16..21",	 GPIOX_16,	GPIOX_21,  113, 118,  4, 16,   4, 16,   0, 16,   1, 16,   2, 16),
+	BANK("Y0..1",	 GPIOY_0,	GPIOY_1,    80,  81,  3,  0,   3,  0,   3,  0,   4,  0,   5,  0),
+	BANK("Y3",	 GPIOY_3,	GPIOY_3,    83,  83,  3,  3,   3,  3,   3,  3,   4,  3,   5,  3),
+	BANK("Y6..14",	 GPIOY_6,	GPIOY_14,   86,  94,  3,  6,   3,  6,   3,  6,   4,  6,   5,  6),
+	BANK("DV9",	 GPIODV_9,	GPIODV_9,   59,  59,  0,  9,   0,  9,   7,  9,   8,  9,   9,  9),
+	BANK("DV24..29", GPIODV_24,	GPIODV_29,  74,  79,  0, 24,   0, 24,   7, 24,   8, 24,   9, 24),
+	BANK("H",	 GPIOH_0,	GPIOH_9,    14,  23,  1, 16,   1, 16,   9, 19,  10, 19,  11, 19),
+	BANK("CARD",	 CARD_0,	CARD_6,     43,  49,  2, 20,   2, 20,   0, 22,   1, 22,   2, 22),
+	BANK("BOOT",	 BOOT_0,	BOOT_18,    24,  42,  2,  0,   2,  0,   9,  0,  10,  0,  11,  0),
 
 	/*
 	 * The following bank is not mentionned in the public datasheet
 	 * There is no information whether it can be used with the gpio
 	 * interrupt controller
 	 */
-	BANK("DIF",  DIF_0_P,		DIF_4_N,    -1,  -1, 5,  8,  5,  8, 12, 12, 13, 12, 14, 12),
+	BANK("DIF",	 DIF_0_P,	DIF_4_N,    -1,  -1,  5,  8,   5,  8,  12, 12,  13, 12,  14, 12),
 };
 
 static struct meson_bank meson8b_aobus_banks[] = {
diff --git a/include/dt-bindings/gpio/meson8b-gpio.h b/include/dt-bindings/gpio/meson8b-gpio.h
index c38cb20d7182..bf0d76fa0e7b 100644
--- a/include/dt-bindings/gpio/meson8b-gpio.h
+++ b/include/dt-bindings/gpio/meson8b-gpio.h
@@ -15,18 +15,113 @@
 #ifndef _DT_BINDINGS_MESON8B_GPIO_H
 #define _DT_BINDINGS_MESON8B_GPIO_H
 
-#include <dt-bindings/gpio/meson8-gpio.h>
-
-/* GPIO Bank DIF */
-#define DIF_0_P		120
-#define DIF_0_N		121
-#define DIF_1_P		122
-#define DIF_1_N		123
-#define DIF_2_P		124
-#define DIF_2_N		125
-#define DIF_3_P		126
-#define DIF_3_N		127
-#define DIF_4_P		128
-#define DIF_4_N		129
+/* EE (CBUS) GPIO chip */
+#define GPIOX_0		0
+#define GPIOX_1		1
+#define GPIOX_2		2
+#define GPIOX_3		3
+#define GPIOX_4		4
+#define GPIOX_5		5
+#define GPIOX_6		6
+#define GPIOX_7		7
+#define GPIOX_8		8
+#define GPIOX_9		9
+#define GPIOX_10	10
+#define GPIOX_11	11
+#define GPIOX_16	12
+#define GPIOX_17	13
+#define GPIOX_18	14
+#define GPIOX_19	15
+#define GPIOX_20	16
+#define GPIOX_21	17
+
+#define GPIOY_0		18
+#define GPIOY_1		19
+#define GPIOY_3		20
+#define GPIOY_6		21
+#define GPIOY_7		22
+#define GPIOY_8		23
+#define GPIOY_9		24
+#define GPIOY_10	25
+#define GPIOY_11	26
+#define GPIOY_12	27
+#define GPIOY_13	28
+#define GPIOY_14	29
+
+#define GPIODV_9	30
+#define GPIODV_24	31
+#define GPIODV_25	32
+#define GPIODV_26	33
+#define GPIODV_27	34
+#define GPIODV_28	35
+#define GPIODV_29	36
+
+#define GPIOH_0		37
+#define GPIOH_1		38
+#define GPIOH_2		39
+#define GPIOH_3		40
+#define GPIOH_4		41
+#define GPIOH_5		42
+#define GPIOH_6		43
+#define GPIOH_7		44
+#define GPIOH_8		45
+#define GPIOH_9		46
+
+#define CARD_0		47
+#define CARD_1		48
+#define CARD_2		49
+#define CARD_3		50
+#define CARD_4		51
+#define CARD_5		52
+#define CARD_6		53
+
+#define BOOT_0		54
+#define BOOT_1		55
+#define BOOT_2		56
+#define BOOT_3		57
+#define BOOT_4		58
+#define BOOT_5		59
+#define BOOT_6		60
+#define BOOT_7		61
+#define BOOT_8		62
+#define BOOT_9		63
+#define BOOT_10		64
+#define BOOT_11		65
+#define BOOT_12		66
+#define BOOT_13		67
+#define BOOT_14		68
+#define BOOT_15		69
+#define BOOT_16		70
+#define BOOT_17		71
+#define BOOT_18		72
+
+#define DIF_0_P		73
+#define DIF_0_N		74
+#define DIF_1_P		75
+#define DIF_1_N		76
+#define DIF_2_P		77
+#define DIF_2_N		78
+#define DIF_3_P		79
+#define DIF_3_N		80
+#define DIF_4_P		81
+#define DIF_4_N		82
+
+/* AO GPIO chip */
+#define GPIOAO_0	0
+#define GPIOAO_1	1
+#define GPIOAO_2	2
+#define GPIOAO_3	3
+#define GPIOAO_4	4
+#define GPIOAO_5	5
+#define GPIOAO_6	6
+#define GPIOAO_7	7
+#define GPIOAO_8	8
+#define GPIOAO_9	9
+#define GPIOAO_10	10
+#define GPIOAO_11	11
+#define GPIOAO_12	12
+#define GPIOAO_13	13
+#define GPIO_BSD_EN	14
+#define GPIO_TEST_N	15
 
 #endif /* _DT_BINDINGS_MESON8B_GPIO_H */
-- 
2.16.2

[PATCH] Meson8b GPIO fix - WiP

[Martin Blumenstingl]

On Sun, Feb 25, 2018 at 12:38 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  arch/arm/boot/dts/meson8b.dtsi          |   2 +-
>  drivers/pinctrl/meson/pinctrl-meson8b.c |  20 +++---
>  include/dt-bindings/gpio/meson8b-gpio.h | 121 ++++++++++++++++++++++++++++----
>  3 files changed, 121 insertions(+), 22 deletions(-)
please ignore this patch, I accidentally sent this
the actual result of this patch is split into two patches and each of
them contains a proper patch description: [0]

sorry for the noise


[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-February/006546.html

[PATCH v2 1/2] pinctrl: meson: meson8b: fix requesting GPIOs greater than GPIOZ_3

[Martin Blumenstingl]

Meson8b is a cost reduced variant of the Meson8 SoC. It's package size
is smaller than Meson8.
Unfortunately there are a few key differences which cannot be seen
without close inspection of the code and the public S805 datasheet:
- the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and
  GPIOX_15 GPIOs
- the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and
  GPIOY_16 GPIOs
- the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24,
  GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29
- the GPIOZ bank is missing completely
- there is a new GPIO bank called "DIF"

This means that Meson8b only has 83 actual GPIO lines. Without any holes
there would be 130 GPIO lines in total (120 are inherited from Meson8
plus 10 new from the DIF bank).

GPIOs greater GPIOZ_3 (whose ID is 83 - as a reminder: this is exactly
the number of actual GPIO lines on Meson8b and also the value of
meson8b_cbus_pinctrl_data.num_pins) cannot berequested. Using CARD_6
(which used ID 100 prior to this patch, "base of the GPIO controller was
382) as an example:
$ echo 482 > /sys/class/gpio/export
export_store: invalid GPIO 482

This removes all non-existing pins from to dt-bindings header file
(include/dt-bindings/gpio/meson8b-gpio.h). This allows us to have a
consecutive numbering for the GPIO #defines (GPIOY_2 doesn't exist for
example, so previously the GPIOY_3 ID was "GPIOY_1 + 2", after this
patch it is "GPIOY_1 + 1"). As a nice side-effect this means that we get
compile-time (instead of runtime) errors if Meson8b .dts uses a pin that
only exists on Meson8.

Additionally the pinctrl-meson8b driver has to be updated to handle this
new GPIO numbering. By default a struct meson_bank only handles GPIO
banks where the pins are numbered consecutively because it calculates
the bit offsets based on the GPIO IDs.
This is solved by  taking the original BANK() definition and splitting it
into consecutive subsets (X0..11 and X16..21). The bit offsets for each
new bank includes the skipped GPIOs (the definition of the "X0..11" bank
is identical to the old "X" bank apart from the "last IRQ" field, the
definition of the new, split "X16..21" bank takes the original "X" bank
and adds 16 - the start of the new split bank - to the "first IRQ",
pullen bit, pull bit, dir bit, out bit and in bit).

Commit 984cffdeaeb7ea ("pinctrl: Fix gpio/pin mapping for Meson8b")
fixed the same issue by setting "ngpio" (of the gpio_chip) to 130.
Unfortunately this broke in db80f0e158e621 ("pinctrl: meson: get rid of
unneeded domain structures").
The solution from this patch was considered to be better than the
previous attempt at fixing this because it provides compile-time error
checking for the GPIOs that exist on Meson8 but don't exist on Meson8b.

The following pins were tested on an Odroid-C1 using the sysfs GPIO
interface checking that their value (high or low) could be read:
- GPIOX_0, GPIOX_1, GPIOX_2, GPIOX_3, GPIOX_4, GPIOX_5, GPIOX_6,
  GPIOX_7, GPIOX_8, GPIOX_9, GPIOX_10, GPIOX_11, GPIOX_18, GPIOX_19,
  GPIOX_20, GPIOX_21
- GPIOY_3, GPIOY_7, GPIOY_8
(some of these had to be pulled up because they were low by default,
others were high by default so these had to be pulled down)

Reported-by: Linus L?ssing <linus.luessing@c0d3.blue>
Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pinctrl/meson/pinctrl-meson8b.c |  20 +++---
 include/dt-bindings/gpio/meson8b-gpio.h | 121 ++++++++++++++++++++++++++++----
 2 files changed, 120 insertions(+), 21 deletions(-)

diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c
index 5bd808dc81e1..bb2a30964fc6 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8b.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8b.c
@@ -884,20 +884,24 @@ static struct meson_pmx_func meson8b_aobus_functions[] = {
 };
 
 static struct meson_bank meson8b_cbus_banks[] = {
-	/*   name    first              last        irq      pullen  pull    dir     out     in  */
-	BANK("X",    GPIOX_0,		GPIOX_21,   97, 118, 4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
-	BANK("Y",    GPIOY_0,		GPIOY_14,   80,  96, 3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
-	BANK("DV",   GPIODV_9,		GPIODV_29,  59,  79, 0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
-	BANK("H",    GPIOH_0,		GPIOH_9,    14,  23, 1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
-	BANK("CARD", CARD_0,		CARD_6,     43,  49, 2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
-	BANK("BOOT", BOOT_0,		BOOT_18,    24,  42, 2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
+	/*   name        first          last        irq       pullen   pull     dir      out      in   */
+	BANK("X0..11",	 GPIOX_0,	GPIOX_11,   97, 108,  4,  0,   4,  0,   0,  0,   1,  0,   2,  0),
+	BANK("X16..21",	 GPIOX_16,	GPIOX_21,  113, 118,  4, 16,   4, 16,   0, 16,   1, 16,   2, 16),
+	BANK("Y0..1",	 GPIOY_0,	GPIOY_1,    80,  81,  3,  0,   3,  0,   3,  0,   4,  0,   5,  0),
+	BANK("Y3",	 GPIOY_3,	GPIOY_3,    83,  83,  3,  3,   3,  3,   3,  3,   4,  3,   5,  3),
+	BANK("Y6..14",	 GPIOY_6,	GPIOY_14,   86,  94,  3,  6,   3,  6,   3,  6,   4,  6,   5,  6),
+	BANK("DV9",	 GPIODV_9,	GPIODV_9,   59,  59,  0,  9,   0,  9,   7,  9,   8,  9,   9,  9),
+	BANK("DV24..29", GPIODV_24,	GPIODV_29,  74,  79,  0, 24,   0, 24,   7, 24,   8, 24,   9, 24),
+	BANK("H",	 GPIOH_0,	GPIOH_9,    14,  23,  1, 16,   1, 16,   9, 19,  10, 19,  11, 19),
+	BANK("CARD",	 CARD_0,	CARD_6,     43,  49,  2, 20,   2, 20,   0, 22,   1, 22,   2, 22),
+	BANK("BOOT",	 BOOT_0,	BOOT_18,    24,  42,  2,  0,   2,  0,   9,  0,  10,  0,  11,  0),
 
 	/*
 	 * The following bank is not mentionned in the public datasheet
 	 * There is no information whether it can be used with the gpio
 	 * interrupt controller
 	 */
-	BANK("DIF",  DIF_0_P,		DIF_4_N,    -1,  -1, 5,  8,  5,  8, 12, 12, 13, 12, 14, 12),
+	BANK("DIF",	 DIF_0_P,	DIF_4_N,    -1,  -1,  5,  8,   5,  8,  12, 12,  13, 12,  14, 12),
 };
 
 static struct meson_bank meson8b_aobus_banks[] = {
diff --git a/include/dt-bindings/gpio/meson8b-gpio.h b/include/dt-bindings/gpio/meson8b-gpio.h
index c38cb20d7182..bf0d76fa0e7b 100644
--- a/include/dt-bindings/gpio/meson8b-gpio.h
+++ b/include/dt-bindings/gpio/meson8b-gpio.h
@@ -15,18 +15,113 @@
 #ifndef _DT_BINDINGS_MESON8B_GPIO_H
 #define _DT_BINDINGS_MESON8B_GPIO_H
 
-#include <dt-bindings/gpio/meson8-gpio.h>
-
-/* GPIO Bank DIF */
-#define DIF_0_P		120
-#define DIF_0_N		121
-#define DIF_1_P		122
-#define DIF_1_N		123
-#define DIF_2_P		124
-#define DIF_2_N		125
-#define DIF_3_P		126
-#define DIF_3_N		127
-#define DIF_4_P		128
-#define DIF_4_N		129
+/* EE (CBUS) GPIO chip */
+#define GPIOX_0		0
+#define GPIOX_1		1
+#define GPIOX_2		2
+#define GPIOX_3		3
+#define GPIOX_4		4
+#define GPIOX_5		5
+#define GPIOX_6		6
+#define GPIOX_7		7
+#define GPIOX_8		8
+#define GPIOX_9		9
+#define GPIOX_10	10
+#define GPIOX_11	11
+#define GPIOX_16	12
+#define GPIOX_17	13
+#define GPIOX_18	14
+#define GPIOX_19	15
+#define GPIOX_20	16
+#define GPIOX_21	17
+
+#define GPIOY_0		18
+#define GPIOY_1		19
+#define GPIOY_3		20
+#define GPIOY_6		21
+#define GPIOY_7		22
+#define GPIOY_8		23
+#define GPIOY_9		24
+#define GPIOY_10	25
+#define GPIOY_11	26
+#define GPIOY_12	27
+#define GPIOY_13	28
+#define GPIOY_14	29
+
+#define GPIODV_9	30
+#define GPIODV_24	31
+#define GPIODV_25	32
+#define GPIODV_26	33
+#define GPIODV_27	34
+#define GPIODV_28	35
+#define GPIODV_29	36
+
+#define GPIOH_0		37
+#define GPIOH_1		38
+#define GPIOH_2		39
+#define GPIOH_3		40
+#define GPIOH_4		41
+#define GPIOH_5		42
+#define GPIOH_6		43
+#define GPIOH_7		44
+#define GPIOH_8		45
+#define GPIOH_9		46
+
+#define CARD_0		47
+#define CARD_1		48
+#define CARD_2		49
+#define CARD_3		50
+#define CARD_4		51
+#define CARD_5		52
+#define CARD_6		53
+
+#define BOOT_0		54
+#define BOOT_1		55
+#define BOOT_2		56
+#define BOOT_3		57
+#define BOOT_4		58
+#define BOOT_5		59
+#define BOOT_6		60
+#define BOOT_7		61
+#define BOOT_8		62
+#define BOOT_9		63
+#define BOOT_10		64
+#define BOOT_11		65
+#define BOOT_12		66
+#define BOOT_13		67
+#define BOOT_14		68
+#define BOOT_15		69
+#define BOOT_16		70
+#define BOOT_17		71
+#define BOOT_18		72
+
+#define DIF_0_P		73
+#define DIF_0_N		74
+#define DIF_1_P		75
+#define DIF_1_N		76
+#define DIF_2_P		77
+#define DIF_2_N		78
+#define DIF_3_P		79
+#define DIF_3_N		80
+#define DIF_4_P		81
+#define DIF_4_N		82
+
+/* AO GPIO chip */
+#define GPIOAO_0	0
+#define GPIOAO_1	1
+#define GPIOAO_2	2
+#define GPIOAO_3	3
+#define GPIOAO_4	4
+#define GPIOAO_5	5
+#define GPIOAO_6	6
+#define GPIOAO_7	7
+#define GPIOAO_8	8
+#define GPIOAO_9	9
+#define GPIOAO_10	10
+#define GPIOAO_11	11
+#define GPIOAO_12	12
+#define GPIOAO_13	13
+#define GPIO_BSD_EN	14
+#define GPIO_TEST_N	15
 
 #endif /* _DT_BINDINGS_MESON8B_GPIO_H */
-- 
2.16.2

[PATCH v2 1/2] pinctrl: meson: meson8b: fix requesting GPIOs greater than GPIOZ_3

[Jerome Brunet]

On Sun, 2018-02-25 at 12:38 +0100, Martin Blumenstingl wrote:
> Reported-by: Linus L?ssing <linus.luessing@c0d3.blue>
> Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Well done !
Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>


> ---
>  drivers/pinctrl/meson/pinctrl-meson8b.c |  20 +++---
>  include/dt-bindings/gpio/meson8b-gpio.h | 121 ++++++++++++++++++++++++++++----
>  2 files changed, 120 insertions(+), 21 deletions(-)

[PATCH v2 1/2] pinctrl: meson: meson8b: fix requesting GPIOs greater than GPIOZ_3

[Linus Walleij]

On Sun, Feb 25, 2018 at 12:38 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:

> Meson8b is a cost reduced variant of the Meson8 SoC. It's package size
> is smaller than Meson8.
> Unfortunately there are a few key differences which cannot be seen
> without close inspection of the code and the public S805 datasheet:
> - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and
>   GPIOX_15 GPIOs
> - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and
>   GPIOY_16 GPIOs
> - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24,
>   GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29
> - the GPIOZ bank is missing completely
> - there is a new GPIO bank called "DIF"
>
> This means that Meson8b only has 83 actual GPIO lines. Without any holes
> there would be 130 GPIO lines in total (120 are inherited from Meson8
> plus 10 new from the DIF bank).
>
> GPIOs greater GPIOZ_3 (whose ID is 83 - as a reminder: this is exactly
> the number of actual GPIO lines on Meson8b and also the value of
> meson8b_cbus_pinctrl_data.num_pins) cannot berequested. Using CARD_6
> (which used ID 100 prior to this patch, "base of the GPIO controller was
> 382) as an example:
> $ echo 482 > /sys/class/gpio/export
> export_store: invalid GPIO 482
>
> This removes all non-existing pins from to dt-bindings header file
> (include/dt-bindings/gpio/meson8b-gpio.h). This allows us to have a
> consecutive numbering for the GPIO #defines (GPIOY_2 doesn't exist for
> example, so previously the GPIOY_3 ID was "GPIOY_1 + 2", after this
> patch it is "GPIOY_1 + 1"). As a nice side-effect this means that we get
> compile-time (instead of runtime) errors if Meson8b .dts uses a pin that
> only exists on Meson8.
>
> Additionally the pinctrl-meson8b driver has to be updated to handle this
> new GPIO numbering. By default a struct meson_bank only handles GPIO
> banks where the pins are numbered consecutively because it calculates
> the bit offsets based on the GPIO IDs.
> This is solved by  taking the original BANK() definition and splitting it
> into consecutive subsets (X0..11 and X16..21). The bit offsets for each
> new bank includes the skipped GPIOs (the definition of the "X0..11" bank
> is identical to the old "X" bank apart from the "last IRQ" field, the
> definition of the new, split "X16..21" bank takes the original "X" bank
> and adds 16 - the start of the new split bank - to the "first IRQ",
> pullen bit, pull bit, dir bit, out bit and in bit).
>
> Commit 984cffdeaeb7ea ("pinctrl: Fix gpio/pin mapping for Meson8b")
> fixed the same issue by setting "ngpio" (of the gpio_chip) to 130.
> Unfortunately this broke in db80f0e158e621 ("pinctrl: meson: get rid of
> unneeded domain structures").
> The solution from this patch was considered to be better than the
> previous attempt at fixing this because it provides compile-time error
> checking for the GPIOs that exist on Meson8 but don't exist on Meson8b.
>
> The following pins were tested on an Odroid-C1 using the sysfs GPIO
> interface checking that their value (high or low) could be read:
> - GPIOX_0, GPIOX_1, GPIOX_2, GPIOX_3, GPIOX_4, GPIOX_5, GPIOX_6,
>   GPIOX_7, GPIOX_8, GPIOX_9, GPIOX_10, GPIOX_11, GPIOX_18, GPIOX_19,
>   GPIOX_20, GPIOX_21
> - GPIOY_3, GPIOY_7, GPIOY_8
> (some of these had to be pulled up because they were low by default,
> others were high by default so these had to be pulled down)
>
> Reported-by: Linus L?ssing <linus.luessing@c0d3.blue>
> Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Patch applied with Jerome's review tag.

Do I need to queue this for v4.16 fixes or is v4.17 good
enough?

Should it even be tagged for stable?

Yours,
Linus Walleij

[PATCH v2 1/2] pinctrl: meson: meson8b: fix requesting GPIOs greater than GPIOZ_3

[Martin Blumenstingl]

Hi Linus,

On Fri, Mar 2, 2018 at 10:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Feb 25, 2018 at 12:38 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>
>> Meson8b is a cost reduced variant of the Meson8 SoC. It's package size
>> is smaller than Meson8.
>> Unfortunately there are a few key differences which cannot be seen
>> without close inspection of the code and the public S805 datasheet:
>> - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and
>>   GPIOX_15 GPIOs
>> - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and
>>   GPIOY_16 GPIOs
>> - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24,
>>   GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29
>> - the GPIOZ bank is missing completely
>> - there is a new GPIO bank called "DIF"
>>
>> This means that Meson8b only has 83 actual GPIO lines. Without any holes
>> there would be 130 GPIO lines in total (120 are inherited from Meson8
>> plus 10 new from the DIF bank).
>>
>> GPIOs greater GPIOZ_3 (whose ID is 83 - as a reminder: this is exactly
>> the number of actual GPIO lines on Meson8b and also the value of
>> meson8b_cbus_pinctrl_data.num_pins) cannot berequested. Using CARD_6
>> (which used ID 100 prior to this patch, "base of the GPIO controller was
>> 382) as an example:
>> $ echo 482 > /sys/class/gpio/export
>> export_store: invalid GPIO 482
>>
>> This removes all non-existing pins from to dt-bindings header file
>> (include/dt-bindings/gpio/meson8b-gpio.h). This allows us to have a
>> consecutive numbering for the GPIO #defines (GPIOY_2 doesn't exist for
>> example, so previously the GPIOY_3 ID was "GPIOY_1 + 2", after this
>> patch it is "GPIOY_1 + 1"). As a nice side-effect this means that we get
>> compile-time (instead of runtime) errors if Meson8b .dts uses a pin that
>> only exists on Meson8.
>>
>> Additionally the pinctrl-meson8b driver has to be updated to handle this
>> new GPIO numbering. By default a struct meson_bank only handles GPIO
>> banks where the pins are numbered consecutively because it calculates
>> the bit offsets based on the GPIO IDs.
>> This is solved by  taking the original BANK() definition and splitting it
>> into consecutive subsets (X0..11 and X16..21). The bit offsets for each
>> new bank includes the skipped GPIOs (the definition of the "X0..11" bank
>> is identical to the old "X" bank apart from the "last IRQ" field, the
>> definition of the new, split "X16..21" bank takes the original "X" bank
>> and adds 16 - the start of the new split bank - to the "first IRQ",
>> pullen bit, pull bit, dir bit, out bit and in bit).
>>
>> Commit 984cffdeaeb7ea ("pinctrl: Fix gpio/pin mapping for Meson8b")
>> fixed the same issue by setting "ngpio" (of the gpio_chip) to 130.
>> Unfortunately this broke in db80f0e158e621 ("pinctrl: meson: get rid of
>> unneeded domain structures").
>> The solution from this patch was considered to be better than the
>> previous attempt at fixing this because it provides compile-time error
>> checking for the GPIOs that exist on Meson8 but don't exist on Meson8b.
>>
>> The following pins were tested on an Odroid-C1 using the sysfs GPIO
>> interface checking that their value (high or low) could be read:
>> - GPIOX_0, GPIOX_1, GPIOX_2, GPIOX_3, GPIOX_4, GPIOX_5, GPIOX_6,
>>   GPIOX_7, GPIOX_8, GPIOX_9, GPIOX_10, GPIOX_11, GPIOX_18, GPIOX_19,
>>   GPIOX_20, GPIOX_21
>> - GPIOY_3, GPIOY_7, GPIOY_8
>> (some of these had to be pulled up because they were low by default,
>> others were high by default so these had to be pulled down)
>>
>> Reported-by: Linus L?ssing <linus.luessing@c0d3.blue>
>> Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> Patch applied with Jerome's review tag.
thank you!

> Do I need to queue this for v4.16 fixes or is v4.17 good
> enough?
>
> Should it even be tagged for stable?
in my opinion v4.17 is good

the CARD, BOOT and DIF banks are affected (and only when trying to use
them as GPIO, pinctrl still works without this patch).
as far as I can see only CARD_6 (SD card detection) and BOOT_9 (eMMC
reset) are used as GPIOs on Odroid-C1 (I assume that other boards only
use these too)

this issue only came up when adding SD card support for Odroid-C1
it went unnoticed for ~1.5 years, so I believe it won't hurt anybody
if it "only" makes it into v4.17


Regards
Martin

[PATCH v2 2/2] ARM: dts: meson8b: the CBUS GPIO controller only has 83 GPIOs

[Martin Blumenstingl]

Update the "gpio-ranges" property of the CBUS GPIO controller on Meson8b
because it only provides 83 GPIOs.
The GPIO definitions in include/dt-bindings/gpio/meson8b-gpio.h
inherited all GPIOs from Meson8 until recently. However, Meson8b does
not support all GPIOs which are supported by Meson8 (Meson8b doesn't
have a GPIOZ bank, most of the pins from the GPIODV bank are missing on
Meson8b - just to name a few differences).

The actual number of GPIOs is only 83, instead of 120 from Meson8 plus
the 10 GPIOs from the DIF bank on Meson8b.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8b.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index 5f7841b2d163..9fa77975354b 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -183,7 +183,7 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
-			gpio-ranges = <&pinctrl_cbus 0 0 130>;
+			gpio-ranges = <&pinctrl_cbus 0 0 83>;
 		};
 
 		eth_rgmii_pins: eth-rgmii {
-- 
2.16.2

[PATCH v2 2/2] ARM: dts: meson8b: the CBUS GPIO controller only has 83 GPIOs

[Jerome Brunet]

On Sun, 2018-02-25 at 12:38 +0100, Martin Blumenstingl wrote:
> Update the "gpio-ranges" property of the CBUS GPIO controller on Meson8b
> because it only provides 83 GPIOs.
> The GPIO definitions in include/dt-bindings/gpio/meson8b-gpio.h
> inherited all GPIOs from Meson8 until recently. However, Meson8b does
> not support all GPIOs which are supported by Meson8 (Meson8b doesn't
> have a GPIOZ bank, most of the pins from the GPIODV bank are missing on
> Meson8b - just to name a few differences).
> 
> The actual number of GPIOs is only 83, instead of 120 from Meson8 plus
> the 10 GPIOs from the DIF bank on Meson8b.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

> ---
>  arch/arm/boot/dts/meson8b.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
> index 5f7841b2d163..9fa77975354b 100644
> --- a/arch/arm/boot/dts/meson8b.dtsi
> +++ b/arch/arm/boot/dts/meson8b.dtsi
> @@ -183,7 +183,7 @@
>  			reg-names = "mux", "pull", "pull-enable", "gpio";
>  			gpio-controller;
>  			#gpio-cells = <2>;
> -			gpio-ranges = <&pinctrl_cbus 0 0 130>;
> +			gpio-ranges = <&pinctrl_cbus 0 0 83>;
>  		};
>  
>  		eth_rgmii_pins: eth-rgmii {

[PATCH v2 2/2] ARM: dts: meson8b: the CBUS GPIO controller only has 83 GPIOs

[Linus Walleij]

On Sun, Feb 25, 2018 at 12:38 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:

> Update the "gpio-ranges" property of the CBUS GPIO controller on Meson8b
> because it only provides 83 GPIOs.
> The GPIO definitions in include/dt-bindings/gpio/meson8b-gpio.h
> inherited all GPIOs from Meson8 until recently. However, Meson8b does
> not support all GPIOs which are supported by Meson8 (Meson8b doesn't
> have a GPIOZ bank, most of the pins from the GPIODV bank are missing on
> Meson8b - just to name a few differences).
>
> The actual number of GPIOs is only 83, instead of 120 from Meson8 plus
> the 10 GPIOs from the DIF bank on Meson8b.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

Please funnel this through the ARM SoC tree. If it is closesly
associated with patch 1/2 and need to go in together with it,
I can merge it if some ARM SoC maintainer can provide an
ACK.

Yours,
Linus Walleij