[PATCH v2 0/5] Add sdmmc UHS support to ROC-RK3328-CC board.

[PATCH v2 0/5] Add sdmmc UHS support to ROC-RK3328-CC board.

[djw]

From: Levin Du <djw@t-chip.com.cn>


Hi all, this is an attemp to add sdmmc UHS support to the
ROC-RK3328-CC board.

This patch series adds a new compatible `rockchip,gpio-syscon` to
the gpio-syscon driver for general Rockchip SoC usage.

A new gpio controller named `gpio_mute` is defined in
rk3328.dtsi so that all rk3328 boards has access to it.

The ROC-RK3328-CC board use the new gpio <&gpio_mute 1> in
gpio-regulator to control the signal voltage of the sdmmc.
It is essential for UHS support which requires 1.8V signal voltage.

Many thanks to Heiko's great advice!

Changes in v2:
- Rename gpio_syscon10 to gpio_mute in doc
- Rename gpio_syscon10 to gpio_mute in rk3328.dtsi
- Rename gpio_syscon10 to gpio_mute in rk3328-roc-cc.dts

Changes in v1:
- New: allow fetching syscon from parent node in gpio-syscon driver
- Refactured for general gpio-syscon usage for Rockchip SoCs.
- Add doc rockchip,gpio-syscon.txt
- Split from V0 into small patches
- Sort dts properties in sdmmc node

Heiko Stuebner (1):
  gpio: syscon: allow fetching syscon from parent node

Levin Du (4):
  gpio: syscon: Add gpio-syscon for rockchip
  arm64: dts: rockchip: Add gpio-mute to rk3328
  arm64: dts: rockchip: Add io-domain to roc-rk3328-cc
  arm64: dts: rockchip: Add sdmmc UHS support for roc-rk3328-cc

 .../bindings/gpio/rockchip,gpio-syscon.txt         | 41 ++++++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts     | 30 ++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328.dtsi           |  7 ++++
 drivers/gpio/gpio-syscon.c                         | 32 +++++++++++++++++
 4 files changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

-- 
2.7.4

[PATCH v2 0/5] Add sdmmc UHS support to ROC-RK3328-CC board.

[djw]

From: Levin Du <djw@t-chip.com.cn>


Hi all, this is an attemp to add sdmmc UHS support to the
ROC-RK3328-CC board.

This patch series adds a new compatible `rockchip,gpio-syscon` to
the gpio-syscon driver for general Rockchip SoC usage.

A new gpio controller named `gpio_mute` is defined in
rk3328.dtsi so that all rk3328 boards has access to it.

The ROC-RK3328-CC board use the new gpio <&gpio_mute 1> in
gpio-regulator to control the signal voltage of the sdmmc.
It is essential for UHS support which requires 1.8V signal voltage.

Many thanks to Heiko's great advice!

Changes in v2:
- Rename gpio_syscon10 to gpio_mute in doc
- Rename gpio_syscon10 to gpio_mute in rk3328.dtsi
- Rename gpio_syscon10 to gpio_mute in rk3328-roc-cc.dts

Changes in v1:
- New: allow fetching syscon from parent node in gpio-syscon driver
- Refactured for general gpio-syscon usage for Rockchip SoCs.
- Add doc rockchip,gpio-syscon.txt
- Split from V0 into small patches
- Sort dts properties in sdmmc node

Heiko Stuebner (1):
  gpio: syscon: allow fetching syscon from parent node

Levin Du (4):
  gpio: syscon: Add gpio-syscon for rockchip
  arm64: dts: rockchip: Add gpio-mute to rk3328
  arm64: dts: rockchip: Add io-domain to roc-rk3328-cc
  arm64: dts: rockchip: Add sdmmc UHS support for roc-rk3328-cc

 .../bindings/gpio/rockchip,gpio-syscon.txt         | 41 ++++++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts     | 30 ++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328.dtsi           |  7 ++++
 drivers/gpio/gpio-syscon.c                         | 32 +++++++++++++++++
 4 files changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

-- 
2.7.4

[PATCH v2 0/5] Add sdmmc UHS support to ROC-RK3328-CC board.

[djw at t-chip.com.cn]

From: Levin Du <djw@t-chip.com.cn>


Hi all, this is an attemp to add sdmmc UHS support to the
ROC-RK3328-CC board.

This patch series adds a new compatible `rockchip,gpio-syscon` to
the gpio-syscon driver for general Rockchip SoC usage.

A new gpio controller named `gpio_mute` is defined in
rk3328.dtsi so that all rk3328 boards has access to it.

The ROC-RK3328-CC board use the new gpio <&gpio_mute 1> in
gpio-regulator to control the signal voltage of the sdmmc.
It is essential for UHS support which requires 1.8V signal voltage.

Many thanks to Heiko's great advice!

Changes in v2:
- Rename gpio_syscon10 to gpio_mute in doc
- Rename gpio_syscon10 to gpio_mute in rk3328.dtsi
- Rename gpio_syscon10 to gpio_mute in rk3328-roc-cc.dts

Changes in v1:
- New: allow fetching syscon from parent node in gpio-syscon driver
- Refactured for general gpio-syscon usage for Rockchip SoCs.
- Add doc rockchip,gpio-syscon.txt
- Split from V0 into small patches
- Sort dts properties in sdmmc node

Heiko Stuebner (1):
  gpio: syscon: allow fetching syscon from parent node

Levin Du (4):
  gpio: syscon: Add gpio-syscon for rockchip
  arm64: dts: rockchip: Add gpio-mute to rk3328
  arm64: dts: rockchip: Add io-domain to roc-rk3328-cc
  arm64: dts: rockchip: Add sdmmc UHS support for roc-rk3328-cc

 .../bindings/gpio/rockchip,gpio-syscon.txt         | 41 ++++++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts     | 30 ++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328.dtsi           |  7 ++++
 drivers/gpio/gpio-syscon.c                         | 32 +++++++++++++++++
 4 files changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

-- 
2.7.4

[PATCH v2 1/5] gpio: syscon: allow fetching syscon from parent node

[djw]

From: Heiko Stuebner <heiko@sntech.de>

Syscon nodes can be a simple-mfd and the syscon-users then be declared
as children of this node. That way the parent-child structure can be
better represented for devices that are fully embedded in the syscon.

Therefore allow getting the syscon from the parent if neither
a special compatible nor a gpio,syscon-dev property is defined.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Levin Du <djw@t-chip.com.cn>
---

Changes in v2: None
Changes in v1:
- New: allow fetching syscon from parent node in gpio-syscon driver

 drivers/gpio/gpio-syscon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 537cec7..7325b86 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -205,6 +205,8 @@ static int syscon_gpio_probe(struct platform_device *pdev)
 	} else {
 		priv->syscon =
 			syscon_regmap_lookup_by_phandle(np, "gpio,syscon-dev");
+		if (IS_ERR(priv->syscon) && np->parent)
+			priv->syscon = syscon_node_to_regmap(np->parent);
 		if (IS_ERR(priv->syscon))
 			return PTR_ERR(priv->syscon);
 
-- 
2.7.4

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[djw]

From: Levin Du <djw@t-chip.com.cn>

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du <djw@t-chip.com.cn>

---

Changes in v2:
- Rename gpio_syscon10 to gpio_mute in doc

Changes in v1:
- Refactured for general gpio-syscon usage for Rockchip SoCs.
- Add doc rockchip,gpio-syscon.txt

 .../bindings/gpio/rockchip,gpio-syscon.txt         | 41 ++++++++++++++++++++++
 drivers/gpio/gpio-syscon.c                         | 30 ++++++++++++++++
 2 files changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 0000000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+    0 = Active high,
+    1 = Active low.
+- gpio,syscon-dev: Should contain <grf_phandle syscon_offset 0>.
+  If declared as child of the grf node, the grf_phandle can be 0.
+
+Example:
+
+1. As child of grf node:
+
+	grf: syscon@ff100000 {
+		compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+
+		gpio_mute: gpio-mute {
+			compatible = "rockchip,gpio-syscon";
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio,syscon-dev = <0 0x0428 0>;
+		};
+	};
+
+
+2. Not child of grf node:
+
+	grf: syscon@ff100000 {
+		compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+		//...
+	};
+
+	gpio_mute: gpio-mute {
+		compatible = "rockchip,gpio-syscon";
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio,syscon-dev = <&grf 0x0428 0>;
+	};
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 7325b86..e24b408 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -135,6 +135,32 @@ static const struct syscon_gpio_data clps711x_mctrl_gpio = {
 	.dat_bit_offset	= 0x40 * 8 + 8,
 };
 
+static void rockchip_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			      int val)
+{
+	struct syscon_gpio_priv *priv = gpiochip_get_data(chip);
+	unsigned int offs;
+	u8 bit;
+	u32 data;
+	int ret;
+
+	offs = priv->dreg_offset + priv->data->dat_bit_offset + offset;
+	bit = offs % SYSCON_REG_BITS;
+	data = (val ? BIT(bit) : 0) | BIT(bit + 16);
+	ret = regmap_write(priv->syscon,
+			   (offs / SYSCON_REG_BITS) * SYSCON_REG_SIZE,
+			   data);
+	if (ret < 0)
+		dev_err(chip->parent, "gpio write failed ret(%d)\n", ret);
+}
+
+static const struct syscon_gpio_data rockchip_gpio_syscon = {
+	/* Rockchip GRF_SOC_CON Bits 0-15 */
+	.flags		= GPIO_SYSCON_FEAT_OUT,
+	.bit_count	= 16,
+	.set		= rockchip_gpio_set,
+};
+
 #define KEYSTONE_LOCK_BIT BIT(0)
 
 static void keystone_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
@@ -175,6 +201,10 @@ static const struct of_device_id syscon_gpio_ids[] = {
 		.compatible	= "ti,keystone-dsp-gpio",
 		.data		= &keystone_dsp_gpio,
 	},
+	{
+		.compatible	= "rockchip,gpio-syscon",
+		.data		= &rockchip_gpio_syscon,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, syscon_gpio_ids);
-- 
2.7.4

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[djw at t-chip.com.cn]

From: Levin Du <djw@t-chip.com.cn>

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du <djw@t-chip.com.cn>

---

Changes in v2:
- Rename gpio_syscon10 to gpio_mute in doc

Changes in v1:
- Refactured for general gpio-syscon usage for Rockchip SoCs.
- Add doc rockchip,gpio-syscon.txt

 .../bindings/gpio/rockchip,gpio-syscon.txt         | 41 ++++++++++++++++++++++
 drivers/gpio/gpio-syscon.c                         | 30 ++++++++++++++++
 2 files changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 0000000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+    0 = Active high,
+    1 = Active low.
+- gpio,syscon-dev: Should contain <grf_phandle syscon_offset 0>.
+  If declared as child of the grf node, the grf_phandle can be 0.
+
+Example:
+
+1. As child of grf node:
+
+	grf: syscon at ff100000 {
+		compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+
+		gpio_mute: gpio-mute {
+			compatible = "rockchip,gpio-syscon";
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio,syscon-dev = <0 0x0428 0>;
+		};
+	};
+
+
+2. Not child of grf node:
+
+	grf: syscon at ff100000 {
+		compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+		//...
+	};
+
+	gpio_mute: gpio-mute {
+		compatible = "rockchip,gpio-syscon";
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio,syscon-dev = <&grf 0x0428 0>;
+	};
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 7325b86..e24b408 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -135,6 +135,32 @@ static const struct syscon_gpio_data clps711x_mctrl_gpio = {
 	.dat_bit_offset	= 0x40 * 8 + 8,
 };
 
+static void rockchip_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			      int val)
+{
+	struct syscon_gpio_priv *priv = gpiochip_get_data(chip);
+	unsigned int offs;
+	u8 bit;
+	u32 data;
+	int ret;
+
+	offs = priv->dreg_offset + priv->data->dat_bit_offset + offset;
+	bit = offs % SYSCON_REG_BITS;
+	data = (val ? BIT(bit) : 0) | BIT(bit + 16);
+	ret = regmap_write(priv->syscon,
+			   (offs / SYSCON_REG_BITS) * SYSCON_REG_SIZE,
+			   data);
+	if (ret < 0)
+		dev_err(chip->parent, "gpio write failed ret(%d)\n", ret);
+}
+
+static const struct syscon_gpio_data rockchip_gpio_syscon = {
+	/* Rockchip GRF_SOC_CON Bits 0-15 */
+	.flags		= GPIO_SYSCON_FEAT_OUT,
+	.bit_count	= 16,
+	.set		= rockchip_gpio_set,
+};
+
 #define KEYSTONE_LOCK_BIT BIT(0)
 
 static void keystone_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
@@ -175,6 +201,10 @@ static const struct of_device_id syscon_gpio_ids[] = {
 		.compatible	= "ti,keystone-dsp-gpio",
 		.data		= &keystone_dsp_gpio,
 	},
+	{
+		.compatible	= "rockchip,gpio-syscon",
+		.data		= &rockchip_gpio_syscon,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, syscon_gpio_ids);
-- 
2.7.4

[PATCH v2 3/5] arm64: dts: rockchip: Add gpio-mute to rk3328

From: Levin Du <djw@t-chip.com.cn>

Adding a new gpio controller named "gpio-mute" to rk3328, providing
access to the GPIO_MUTE pin defined in the syscon GRF_SOC_CON10.

The GPIO_MUTE pin is referred to as <&gpio-mute 1>.

Signed-off-by: Levin Du <djw@t-chip.com.cn>

---

Changes in v2:
- Rename gpio_syscon10 to gpio_mute in rk3328.dtsi

Changes in v1:
- Split from V0 and add to rk3328.dtsi for general use.

 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index b8e9da1..5ba29d3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -309,6 +309,13 @@
 			mode-loader = <BOOT_BL_DOWNLOAD>;
 		};
 
+		/* The GPIO_MUTE pin is referred to as <&gpio-mute 1>.*/
+		gpio_mute: gpio-mute {
+			compatible = "rockchip,gpio-syscon";
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio,syscon-dev = <0 0x0428 0>;
+		};
 	};
 
 	uart0: serial@ff110000 {
-- 
2.7.4

[PATCH v2 3/5] arm64: dts: rockchip: Add gpio-mute to rk3328

[djw at t-chip.com.cn]

From: Levin Du <djw@t-chip.com.cn>

Adding a new gpio controller named "gpio-mute" to rk3328, providing
access to the GPIO_MUTE pin defined in the syscon GRF_SOC_CON10.

The GPIO_MUTE pin is referred to as <&gpio-mute 1>.

Signed-off-by: Levin Du <djw@t-chip.com.cn>

---

Changes in v2:
- Rename gpio_syscon10 to gpio_mute in rk3328.dtsi

Changes in v1:
- Split from V0 and add to rk3328.dtsi for general use.

 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index b8e9da1..5ba29d3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -309,6 +309,13 @@
 			mode-loader = <BOOT_BL_DOWNLOAD>;
 		};
 
+		/* The GPIO_MUTE pin is referred to as <&gpio-mute 1>.*/
+		gpio_mute: gpio-mute {
+			compatible = "rockchip,gpio-syscon";
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio,syscon-dev = <0 0x0428 0>;
+		};
 	};
 
 	uart0: serial at ff110000 {
-- 
2.7.4

[PATCH v2 4/5] arm64: dts: rockchip: Add io-domain to roc-rk3328-cc

[djw]

From: Levin Du <djw@t-chip.com.cn>

It is necessary for the io domain setting of the SoC to match
the voltage supplied by the regulators.

Signed-off-by: Levin Du <djw@t-chip.com.cn>

---

Changes in v2: None
Changes in v1:
- Split from V0.

 arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
index 246c317..b983abd 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
@@ -208,6 +208,18 @@
 	};
 };
 
+&io_domains {
+	status = "okay";
+
+	vccio1-supply = <&vcc_io>;
+	vccio2-supply = <&vcc18_emmc>;
+	vccio3-supply = <&vcc_io>;
+	vccio4-supply = <&vcc_18>;
+	vccio5-supply = <&vcc_io>;
+	vccio6-supply = <&vcc_io>;
+	pmuio-supply = <&vcc_io>;
+};
+
 &pinctrl {
 	pmic {
 		pmic_int_l: pmic-int-l {
-- 
2.7.4

[PATCH v2 4/5] arm64: dts: rockchip: Add io-domain to roc-rk3328-cc

[djw at t-chip.com.cn]

From: Levin Du <djw@t-chip.com.cn>

It is necessary for the io domain setting of the SoC to match
the voltage supplied by the regulators.

Signed-off-by: Levin Du <djw@t-chip.com.cn>

---

Changes in v2: None
Changes in v1:
- Split from V0.

 arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
index 246c317..b983abd 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
@@ -208,6 +208,18 @@
 	};
 };
 
+&io_domains {
+	status = "okay";
+
+	vccio1-supply = <&vcc_io>;
+	vccio2-supply = <&vcc18_emmc>;
+	vccio3-supply = <&vcc_io>;
+	vccio4-supply = <&vcc_18>;
+	vccio5-supply = <&vcc_io>;
+	vccio6-supply = <&vcc_io>;
+	pmuio-supply = <&vcc_io>;
+};
+
 &pinctrl {
 	pmic {
 		pmic_int_l: pmic-int-l {
-- 
2.7.4

[PATCH v2 5/5] arm64: dts: rockchip: Add sdmmc UHS support for roc-rk3328-cc

[djw]

From: Levin Du <djw@t-chip.com.cn>

In roc-rk3328-cc board, the signal voltage of sdmmc is supplied by
the vcc_sdio regulator, which is a mux between 1.8V and 3.3V,
controlled by a special output only gpio pin labeled
"gpiomut_pmuio_iout", corresponding bit 1 of the syscon GRF_SOC_CON10.

This special pin can now be reference as <&gpio_mute 1>, thanks
to the gpio-syscon driver, which makes writing regulator-gpio possible.

If the signal voltage changes, the io domain needs to change
correspondingly.

To use this feature, the following options are required in kernel config:
 - CONFIG_GPIO_SYSCON=y
 - CONFIG_POWER_AVS=y
 - CONFIG_ROCKCHIP_IODOMAIN=y

Signed-off-by: Levin Du <djw@t-chip.com.cn>

---

Changes in v2:
- Rename gpio_syscon10 to gpio_mute in rk3328-roc-cc.dts

Changes in v1:
- Split into small patches
- Sort dts properties in sdmmc node

 arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
index b983abd..e3162bb 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
@@ -41,6 +41,19 @@
 		vin-supply = <&vcc_io>;
 	};
 
+	vcc_sdio: sdmmcio-regulator {
+		compatible = "regulator-gpio";
+		gpios = <&gpio_mute 1 GPIO_ACTIVE_HIGH>;
+		states = <1800000 0x1
+			  3300000 0x0>;
+		regulator-name = "vcc_sdio";
+		regulator-type = "voltage";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-always-on;
+		vin-supply = <&vcc_sys>;
+	};
+
 	vcc_host1_5v: vcc_otg_5v: vcc-host1-5v-regulator {
 		compatible = "regulator-fixed";
 		enable-active-high;
@@ -213,7 +226,7 @@
 
 	vccio1-supply = <&vcc_io>;
 	vccio2-supply = <&vcc18_emmc>;
-	vccio3-supply = <&vcc_io>;
+	vccio3-supply = <&vcc_sdio>;
 	vccio4-supply = <&vcc_18>;
 	vccio5-supply = <&vcc_io>;
 	vccio6-supply = <&vcc_io>;
@@ -242,7 +255,12 @@
 	max-frequency = <150000000>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&sdmmc0_clk &sdmmc0_cmd &sdmmc0_dectn &sdmmc0_bus4>;
+	sd-uhs-sdr12;
+	sd-uhs-sdr25;
+	sd-uhs-sdr50;
+	sd-uhs-sdr104;
 	vmmc-supply = <&vcc_sd>;
+	vqmmc-supply = <&vcc_sdio>;
 	status = "okay";
 };
 
-- 
2.7.4

[PATCH v2 5/5] arm64: dts: rockchip: Add sdmmc UHS support for roc-rk3328-cc

[djw at t-chip.com.cn]

From: Levin Du <djw@t-chip.com.cn>

In roc-rk3328-cc board, the signal voltage of sdmmc is supplied by
the vcc_sdio regulator, which is a mux between 1.8V and 3.3V,
controlled by a special output only gpio pin labeled
"gpiomut_pmuio_iout", corresponding bit 1 of the syscon GRF_SOC_CON10.

This special pin can now be reference as <&gpio_mute 1>, thanks
to the gpio-syscon driver, which makes writing regulator-gpio possible.

If the signal voltage changes, the io domain needs to change
correspondingly.

To use this feature, the following options are required in kernel config:
 - CONFIG_GPIO_SYSCON=y
 - CONFIG_POWER_AVS=y
 - CONFIG_ROCKCHIP_IODOMAIN=y

Signed-off-by: Levin Du <djw@t-chip.com.cn>

---

Changes in v2:
- Rename gpio_syscon10 to gpio_mute in rk3328-roc-cc.dts

Changes in v1:
- Split into small patches
- Sort dts properties in sdmmc node

 arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
index b983abd..e3162bb 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
@@ -41,6 +41,19 @@
 		vin-supply = <&vcc_io>;
 	};
 
+	vcc_sdio: sdmmcio-regulator {
+		compatible = "regulator-gpio";
+		gpios = <&gpio_mute 1 GPIO_ACTIVE_HIGH>;
+		states = <1800000 0x1
+			  3300000 0x0>;
+		regulator-name = "vcc_sdio";
+		regulator-type = "voltage";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-always-on;
+		vin-supply = <&vcc_sys>;
+	};
+
 	vcc_host1_5v: vcc_otg_5v: vcc-host1-5v-regulator {
 		compatible = "regulator-fixed";
 		enable-active-high;
@@ -213,7 +226,7 @@
 
 	vccio1-supply = <&vcc_io>;
 	vccio2-supply = <&vcc18_emmc>;
-	vccio3-supply = <&vcc_io>;
+	vccio3-supply = <&vcc_sdio>;
 	vccio4-supply = <&vcc_18>;
 	vccio5-supply = <&vcc_io>;
 	vccio6-supply = <&vcc_io>;
@@ -242,7 +255,12 @@
 	max-frequency = <150000000>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&sdmmc0_clk &sdmmc0_cmd &sdmmc0_dectn &sdmmc0_bus4>;
+	sd-uhs-sdr12;
+	sd-uhs-sdr25;
+	sd-uhs-sdr50;
+	sd-uhs-sdr104;
 	vmmc-supply = <&vcc_sd>;
+	vqmmc-supply = <&vcc_sdio>;
 	status = "okay";
 };
 
-- 
2.7.4

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
> From: Levin Du <djw@t-chip.com.cn>
> 
> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> which do not belong to the general pinctrl.
> 
> Adding gpio-syscon support makes controlling regulator or
> LED using these special pins very easy by reusing existing
> drivers, such as gpio-regulator and led-gpio.
> 
> Signed-off-by: Levin Du <djw@t-chip.com.cn>
> 
> ---
> 
> Changes in v2:
> - Rename gpio_syscon10 to gpio_mute in doc
> 
> Changes in v1:
> - Refactured for general gpio-syscon usage for Rockchip SoCs.
> - Add doc rockchip,gpio-syscon.txt
> 
>  .../bindings/gpio/rockchip,gpio-syscon.txt         | 41 ++++++++++++++++++++++
>  drivers/gpio/gpio-syscon.c                         | 30 ++++++++++++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> new file mode 100644
> index 0000000..b1b2a67
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> @@ -0,0 +1,41 @@
> +* Rockchip GPIO support for GRF_SOC_CON registers
> +
> +Required properties:
> +- compatible: Should contain "rockchip,gpio-syscon".
> +- gpio-controller: Marks the device node as a gpio controller.
> +- #gpio-cells: Should be two. The first cell is the pin number and
> +  the second cell is used to specify the gpio polarity:
> +    0 = Active high,
> +    1 = Active low.

There's no need for this child node. Just make the parent node a gpio 
controller.

Rob

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
> From: Levin Du <djw@t-chip.com.cn>
> 
> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> which do not belong to the general pinctrl.
> 
> Adding gpio-syscon support makes controlling regulator or
> LED using these special pins very easy by reusing existing
> drivers, such as gpio-regulator and led-gpio.
> 
> Signed-off-by: Levin Du <djw@t-chip.com.cn>
> 
> ---
> 
> Changes in v2:
> - Rename gpio_syscon10 to gpio_mute in doc
> 
> Changes in v1:
> - Refactured for general gpio-syscon usage for Rockchip SoCs.
> - Add doc rockchip,gpio-syscon.txt
> 
>  .../bindings/gpio/rockchip,gpio-syscon.txt         | 41 ++++++++++++++++++++++
>  drivers/gpio/gpio-syscon.c                         | 30 ++++++++++++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> new file mode 100644
> index 0000000..b1b2a67
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> @@ -0,0 +1,41 @@
> +* Rockchip GPIO support for GRF_SOC_CON registers
> +
> +Required properties:
> +- compatible: Should contain "rockchip,gpio-syscon".
> +- gpio-controller: Marks the device node as a gpio controller.
> +- #gpio-cells: Should be two. The first cell is the pin number and
> +  the second cell is used to specify the gpio polarity:
> +    0 = Active high,
> +    1 = Active low.

There's no need for this child node. Just make the parent node a gpio 
controller.

Rob

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin Du]

On 2018-05-23 2:02 AM, Rob Herring wrote:
> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
>> From: Levin Du <djw@t-chip.com.cn>
>>
>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> which do not belong to the general pinctrl.
>>
>> Adding gpio-syscon support makes controlling regulator or
>> LED using these special pins very easy by reusing existing
>> drivers, such as gpio-regulator and led-gpio.
>>
>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>
>> ---
>>
>> Changes in v2:
>> - Rename gpio_syscon10 to gpio_mute in doc
>>
>> Changes in v1:
>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> - Add doc rockchip,gpio-syscon.txt
>>
>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41 ++++++++++++++++++++++
>>   drivers/gpio/gpio-syscon.c                         | 30 ++++++++++++++++
>>   2 files changed, 71 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> new file mode 100644
>> index 0000000..b1b2a67
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> @@ -0,0 +1,41 @@
>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> +
>> +Required properties:
>> +- compatible: Should contain "rockchip,gpio-syscon".
>> +- gpio-controller: Marks the device node as a gpio controller.
>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> +  the second cell is used to specify the gpio polarity:
>> +    0 = Active high,
>> +    1 = Active low.
> There's no need for this child node. Just make the parent node a gpio
> controller.
>
> Rob
Hi Rob, it is not clear to me. Do you suggest that the grf node should 
be a gpio controller,
like below?

+    grf: syscon at ff100000 {
+        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", 
"syscon", "simple-mfd";
+        //...
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio,syscon-dev = <&grf 0x0428 0>;
+    };

or just reserve the following case in the doc?

+    grf: syscon at ff100000 {
+        compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+        //...
+    };
+
+    gpio_mute: gpio-mute {
+        compatible = "rockchip,gpio-syscon";
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio,syscon-dev = <&grf 0x0428 0>;
+    };


Thanks
Levin

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin Du]

On 2018-05-23 2:02 AM, Rob Herring wrote:
> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
>> From: Levin Du <djw@t-chip.com.cn>
>>
>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> which do not belong to the general pinctrl.
>>
>> Adding gpio-syscon support makes controlling regulator or
>> LED using these special pins very easy by reusing existing
>> drivers, such as gpio-regulator and led-gpio.
>>
>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>
>> ---
>>
>> Changes in v2:
>> - Rename gpio_syscon10 to gpio_mute in doc
>>
>> Changes in v1:
>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> - Add doc rockchip,gpio-syscon.txt
>>
>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41 ++++++++++++++++++++++
>>   drivers/gpio/gpio-syscon.c                         | 30 ++++++++++++++++
>>   2 files changed, 71 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> new file mode 100644
>> index 0000000..b1b2a67
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> @@ -0,0 +1,41 @@
>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> +
>> +Required properties:
>> +- compatible: Should contain "rockchip,gpio-syscon".
>> +- gpio-controller: Marks the device node as a gpio controller.
>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> +  the second cell is used to specify the gpio polarity:
>> +    0 = Active high,
>> +    1 = Active low.
> There's no need for this child node. Just make the parent node a gpio
> controller.
>
> Rob
Hi Rob, it is not clear to me. Do you suggest that the grf node should 
be a gpio controller,
like below?

+??? grf: syscon at ff100000 {
+??? ??? compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", 
"syscon", "simple-mfd";
+??? ??? //...
+??? ??? gpio-controller;
+??? ??? #gpio-cells = <2>;
+??? ??? gpio,syscon-dev = <&grf 0x0428 0>;
+??? };

or just reserve the following case in the doc?

+??? grf: syscon at ff100000 {
+??? ??? compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+??? ??? //...
+??? };
+
+??? gpio_mute: gpio-mute {
+??? ??? compatible = "rockchip,gpio-syscon";
+??? ??? gpio-controller;
+??? ??? #gpio-cells = <2>;
+??? ??? gpio,syscon-dev = <&grf 0x0428 0>;
+??? };


Thanks
Levin

Re: [PATCH v2 1/5] gpio: syscon: allow fetching syscon from parent node

[Linus Walleij]

On Fri, May 18, 2018 at 5:52 AM,  <djw@t-chip.com.cn> wrote:

> From: Heiko Stuebner <heiko@sntech.de>
>
> Syscon nodes can be a simple-mfd and the syscon-users then be declared
> as children of this node. That way the parent-child structure can be
> better represented for devices that are fully embedded in the syscon.
>
> Therefore allow getting the syscon from the parent if neither
> a special compatible nor a gpio,syscon-dev property is defined.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Levin Du <djw@t-chip.com.cn>
> ---
>
> Changes in v2: None
> Changes in v1:
> - New: allow fetching syscon from parent node in gpio-syscon driver

Regardless of what happens with the rest of the patches this
looks sane and generally useful, so patch applied!

Yours,
Linus Walleij

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
> On 2018-05-23 2:02 AM, Rob Herring wrote:
>>
>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
>>>
>>> From: Levin Du <djw@t-chip.com.cn>
>>>
>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>>> which do not belong to the general pinctrl.
>>>
>>> Adding gpio-syscon support makes controlling regulator or
>>> LED using these special pins very easy by reusing existing
>>> drivers, such as gpio-regulator and led-gpio.
>>>
>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - Rename gpio_syscon10 to gpio_mute in doc
>>>
>>> Changes in v1:
>>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>>> - Add doc rockchip,gpio-syscon.txt
>>>
>>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>>> ++++++++++++++++++++++
>>>   drivers/gpio/gpio-syscon.c                         | 30
>>> ++++++++++++++++
>>>   2 files changed, 71 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> new file mode 100644
>>> index 0000000..b1b2a67
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> @@ -0,0 +1,41 @@
>>> +* Rockchip GPIO support for GRF_SOC_CON registers
>>> +
>>> +Required properties:
>>> +- compatible: Should contain "rockchip,gpio-syscon".
>>> +- gpio-controller: Marks the device node as a gpio controller.
>>> +- #gpio-cells: Should be two. The first cell is the pin number and
>>> +  the second cell is used to specify the gpio polarity:
>>> +    0 = Active high,
>>> +    1 = Active low.
>>
>> There's no need for this child node. Just make the parent node a gpio
>> controller.
>>
>> Rob
>
> Hi Rob, it is not clear to me. Do you suggest that the grf node should be a
> gpio controller,
> like below?
>
> +    grf: syscon at ff100000 {
> +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> "syscon", "simple-mfd";

Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

> +        //...
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio,syscon-dev = <&grf 0x0428 0>;

And drop this. It may be used in the kernel, but it is not a
documented property.

> +    };
>
> or just reserve the following case in the doc?
>
> +    grf: syscon at ff100000 {
> +        compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> +        //...
> +    };
> +
> +    gpio_mute: gpio-mute {
> +        compatible = "rockchip,gpio-syscon";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio,syscon-dev = <&grf 0x0428 0>;
> +    };
>
>
> Thanks
> Levin
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
> On 2018-05-23 2:02 AM, Rob Herring wrote:
>>
>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
>>>
>>> From: Levin Du <djw@t-chip.com.cn>
>>>
>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>>> which do not belong to the general pinctrl.
>>>
>>> Adding gpio-syscon support makes controlling regulator or
>>> LED using these special pins very easy by reusing existing
>>> drivers, such as gpio-regulator and led-gpio.
>>>
>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - Rename gpio_syscon10 to gpio_mute in doc
>>>
>>> Changes in v1:
>>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>>> - Add doc rockchip,gpio-syscon.txt
>>>
>>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>>> ++++++++++++++++++++++
>>>   drivers/gpio/gpio-syscon.c                         | 30
>>> ++++++++++++++++
>>>   2 files changed, 71 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> new file mode 100644
>>> index 0000000..b1b2a67
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> @@ -0,0 +1,41 @@
>>> +* Rockchip GPIO support for GRF_SOC_CON registers
>>> +
>>> +Required properties:
>>> +- compatible: Should contain "rockchip,gpio-syscon".
>>> +- gpio-controller: Marks the device node as a gpio controller.
>>> +- #gpio-cells: Should be two. The first cell is the pin number and
>>> +  the second cell is used to specify the gpio polarity:
>>> +    0 = Active high,
>>> +    1 = Active low.
>>
>> There's no need for this child node. Just make the parent node a gpio
>> controller.
>>
>> Rob
>
> Hi Rob, it is not clear to me. Do you suggest that the grf node should be a
> gpio controller,
> like below?
>
> +    grf: syscon at ff100000 {
> +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> "syscon", "simple-mfd";

Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

> +        //...
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio,syscon-dev = <&grf 0x0428 0>;

And drop this. It may be used in the kernel, but it is not a
documented property.

> +    };
>
> or just reserve the following case in the doc?
>
> +    grf: syscon at ff100000 {
> +        compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> +        //...
> +    };
> +
> +    gpio_mute: gpio-mute {
> +        compatible = "rockchip,gpio-syscon";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio,syscon-dev = <&grf 0x0428 0>;
> +    };
>
>
> Thanks
> Levin
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stübner]

Hi Rob, Levin,

sorry for being late to the party.

Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
> > On 2018-05-23 2:02 AM, Rob Herring wrote:
> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
> >>> From: Levin Du <djw@t-chip.com.cn>
> >>> 
> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >>> which do not belong to the general pinctrl.
> >>> 
> >>> Adding gpio-syscon support makes controlling regulator or
> >>> LED using these special pins very easy by reusing existing
> >>> drivers, such as gpio-regulator and led-gpio.
> >>> 
> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
> >>> 
> >>> ---
> >>> 
> >>> Changes in v2:
> >>> - Rename gpio_syscon10 to gpio_mute in doc
> >>> 
> >>> Changes in v1:
> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
> >>> - Add doc rockchip,gpio-syscon.txt
> >>> 
> >>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
> >>> 
> >>> ++++++++++++++++++++++
> >>> 
> >>>   drivers/gpio/gpio-syscon.c                         | 30
> >>> 
> >>> ++++++++++++++++
> >>> 
> >>>   2 files changed, 71 insertions(+)
> >>>   create mode 100644
> >>> 
> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> 
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> new file mode 100644
> >>> index 0000000..b1b2a67
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> @@ -0,0 +1,41 @@
> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >>> +
> >>> +Required properties:
> >>> +- compatible: Should contain "rockchip,gpio-syscon".
> >>> +- gpio-controller: Marks the device node as a gpio controller.
> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >>> +  the second cell is used to specify the gpio polarity:
> >>> +    0 = Active high,
> >>> +    1 = Active low.
> >> 
> >> There's no need for this child node. Just make the parent node a gpio
> >> controller.
> >> 
> >> Rob
> > 
> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> > a
> > gpio controller,
> > like below?
> > 
> > +    grf: syscon at ff100000 {
> > +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> > "syscon", "simple-mfd";
> 
> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

I would disagree quite a bit here. The grf are the "general register files",
a bunch of registers used for quite a lot of things, and so it seems
among other users, also a gpio-controller for some more random pins
not controlled through the regular gpio controllers.

For a more fully stocked grf, please see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338

So the gpio controller should definitly also be a subnode.

The gpio in question is called "mute", so I'd think the gpio-syscon driver
should just define a "rockchip,rk3328-gpio-mute" compatible and contain
all the register voodoo in the driver itself and not define it in the dt.

So it should probably look like

grf: syscon at ff100000 {
        compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";

	[all the other syscon sub-devices]

	gpio_mute: gpio-mute {
		compatible = "rockchip,rk3328-gpio-mute";
		gpio-controller;
		#gpio-cells = <2>;
	};

	[more other syscon sub-devices]
};


Heiko	
	



> > +        //...
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio,syscon-dev = <&grf 0x0428 0>;
> 
> And drop this. It may be used in the kernel, but it is not a
> documented property.
> 
> > +    };
> > 
> > or just reserve the following case in the doc?
> > 
> > +    grf: syscon at ff100000 {
> > +        compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> > +        //...
> > +    };
> > +
> > +    gpio_mute: gpio-mute {
> > +        compatible = "rockchip,gpio-syscon";
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio,syscon-dev = <&grf 0x0428 0>;
> > +    };
> > 
> > 
> > Thanks
> > Levin
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stübner]

Hi Rob, Levin,

sorry for being late to the party.

Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
> > On 2018-05-23 2:02 AM, Rob Herring wrote:
> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
> >>> From: Levin Du <djw@t-chip.com.cn>
> >>> 
> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >>> which do not belong to the general pinctrl.
> >>> 
> >>> Adding gpio-syscon support makes controlling regulator or
> >>> LED using these special pins very easy by reusing existing
> >>> drivers, such as gpio-regulator and led-gpio.
> >>> 
> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
> >>> 
> >>> ---
> >>> 
> >>> Changes in v2:
> >>> - Rename gpio_syscon10 to gpio_mute in doc
> >>> 
> >>> Changes in v1:
> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
> >>> - Add doc rockchip,gpio-syscon.txt
> >>> 
> >>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
> >>> 
> >>> ++++++++++++++++++++++
> >>> 
> >>>   drivers/gpio/gpio-syscon.c                         | 30
> >>> 
> >>> ++++++++++++++++
> >>> 
> >>>   2 files changed, 71 insertions(+)
> >>>   create mode 100644
> >>> 
> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> 
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> new file mode 100644
> >>> index 0000000..b1b2a67
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> @@ -0,0 +1,41 @@
> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >>> +
> >>> +Required properties:
> >>> +- compatible: Should contain "rockchip,gpio-syscon".
> >>> +- gpio-controller: Marks the device node as a gpio controller.
> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >>> +  the second cell is used to specify the gpio polarity:
> >>> +    0 = Active high,
> >>> +    1 = Active low.
> >> 
> >> There's no need for this child node. Just make the parent node a gpio
> >> controller.
> >> 
> >> Rob
> > 
> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> > a
> > gpio controller,
> > like below?
> > 
> > +    grf: syscon at ff100000 {
> > +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> > "syscon", "simple-mfd";
> 
> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

I would disagree quite a bit here. The grf are the "general register files",
a bunch of registers used for quite a lot of things, and so it seems
among other users, also a gpio-controller for some more random pins
not controlled through the regular gpio controllers.

For a more fully stocked grf, please see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338

So the gpio controller should definitly also be a subnode.

The gpio in question is called "mute", so I'd think the gpio-syscon driver
should just define a "rockchip,rk3328-gpio-mute" compatible and contain
all the register voodoo in the driver itself and not define it in the dt.

So it should probably look like

grf: syscon at ff100000 {
        compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";

	[all the other syscon sub-devices]

	gpio_mute: gpio-mute {
		compatible = "rockchip,rk3328-gpio-mute";
		gpio-controller;
		#gpio-cells = <2>;
	};

	[more other syscon sub-devices]
};


Heiko	
	



> > +        //...
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio,syscon-dev = <&grf 0x0428 0>;
> 
> And drop this. It may be used in the kernel, but it is not a
> documented property.
> 
> > +    };
> > 
> > or just reserve the following case in the doc?
> > 
> > +    grf: syscon at ff100000 {
> > +        compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> > +        //...
> > +    };
> > +
> > +    gpio_mute: gpio-mute {
> > +        compatible = "rockchip,gpio-syscon";
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio,syscon-dev = <&grf 0x0428 0>;
> > +    };
> > 
> > 
> > Thanks
> > Levin
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote:
> Hi Rob, Levin,
>
> sorry for being late to the party.
>
> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
>> >>> From: Levin Du <djw@t-chip.com.cn>
>> >>>
>> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >>> which do not belong to the general pinctrl.
>> >>>
>> >>> Adding gpio-syscon support makes controlling regulator or
>> >>> LED using these special pins very easy by reusing existing
>> >>> drivers, such as gpio-regulator and led-gpio.
>> >>>
>> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>> >>>
>> >>> ---
>> >>>
>> >>> Changes in v2:
>> >>> - Rename gpio_syscon10 to gpio_mute in doc
>> >>>
>> >>> Changes in v1:
>> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> >>> - Add doc rockchip,gpio-syscon.txt
>> >>>
>> >>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>> >>>
>> >>> ++++++++++++++++++++++
>> >>>
>> >>>   drivers/gpio/gpio-syscon.c                         | 30
>> >>>
>> >>> ++++++++++++++++
>> >>>
>> >>>   2 files changed, 71 insertions(+)
>> >>>   create mode 100644
>> >>>
>> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>>
>> >>> diff --git
>> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> new file mode 100644
>> >>> index 0000000..b1b2a67
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> @@ -0,0 +1,41 @@
>> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >>> +
>> >>> +Required properties:
>> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >>> +  the second cell is used to specify the gpio polarity:
>> >>> +    0 = Active high,
>> >>> +    1 = Active low.
>> >>
>> >> There's no need for this child node. Just make the parent node a gpio
>> >> controller.
>> >>
>> >> Rob
>> >
>> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>> > a
>> > gpio controller,
>> > like below?
>> >
>> > +    grf: syscon at ff100000 {
>> > +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> > "syscon", "simple-mfd";
>>
>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>
> I would disagree quite a bit here. The grf are the "general register files",
> a bunch of registers used for quite a lot of things, and so it seems
> among other users, also a gpio-controller for some more random pins
> not controlled through the regular gpio controllers.
>
> For a more fully stocked grf, please see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>
> So the gpio controller should definitly also be a subnode.

Sigh, yes, if there are a bunch of functions needing subnodes like the
above, then yes that makes sense. But that's not what has been
presented. Please make some attempt at defining *all* the functions.
An actual binding would be nice, but I'll settle for just a list of
things. The list should have functions that have DT dependencies (like
clocks for phys in the above) because until you do, you don't need
child nodes.

> The gpio in question is called "mute", so I'd think the gpio-syscon driver
> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> all the register voodoo in the driver itself and not define it in the dt.

Is there really just one GPIO? If it has a defined function, then is
it really GP? Can you control direction? I know Linus W doesn't like
that kind of abuse of GPIO.

> So it should probably look like
>
> grf: syscon at ff100000 {
>         compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>
>         [all the other syscon sub-devices]
>
>         gpio_mute: gpio-mute {
>                 compatible = "rockchip,rk3328-gpio-mute";
>                 gpio-controller;
>                 #gpio-cells = <2>;
>         };
>
>         [more other syscon sub-devices]
> };

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Wed, May 23, 2018 at 10:12 AM, Heiko St?bner <heiko@sntech.de> wrote:
> Hi Rob, Levin,
>
> sorry for being late to the party.
>
> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
>> >>> From: Levin Du <djw@t-chip.com.cn>
>> >>>
>> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >>> which do not belong to the general pinctrl.
>> >>>
>> >>> Adding gpio-syscon support makes controlling regulator or
>> >>> LED using these special pins very easy by reusing existing
>> >>> drivers, such as gpio-regulator and led-gpio.
>> >>>
>> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>> >>>
>> >>> ---
>> >>>
>> >>> Changes in v2:
>> >>> - Rename gpio_syscon10 to gpio_mute in doc
>> >>>
>> >>> Changes in v1:
>> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> >>> - Add doc rockchip,gpio-syscon.txt
>> >>>
>> >>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>> >>>
>> >>> ++++++++++++++++++++++
>> >>>
>> >>>   drivers/gpio/gpio-syscon.c                         | 30
>> >>>
>> >>> ++++++++++++++++
>> >>>
>> >>>   2 files changed, 71 insertions(+)
>> >>>   create mode 100644
>> >>>
>> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>>
>> >>> diff --git
>> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> new file mode 100644
>> >>> index 0000000..b1b2a67
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> @@ -0,0 +1,41 @@
>> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >>> +
>> >>> +Required properties:
>> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >>> +  the second cell is used to specify the gpio polarity:
>> >>> +    0 = Active high,
>> >>> +    1 = Active low.
>> >>
>> >> There's no need for this child node. Just make the parent node a gpio
>> >> controller.
>> >>
>> >> Rob
>> >
>> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>> > a
>> > gpio controller,
>> > like below?
>> >
>> > +    grf: syscon at ff100000 {
>> > +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> > "syscon", "simple-mfd";
>>
>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>
> I would disagree quite a bit here. The grf are the "general register files",
> a bunch of registers used for quite a lot of things, and so it seems
> among other users, also a gpio-controller for some more random pins
> not controlled through the regular gpio controllers.
>
> For a more fully stocked grf, please see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>
> So the gpio controller should definitly also be a subnode.

Sigh, yes, if there are a bunch of functions needing subnodes like the
above, then yes that makes sense. But that's not what has been
presented. Please make some attempt at defining *all* the functions.
An actual binding would be nice, but I'll settle for just a list of
things. The list should have functions that have DT dependencies (like
clocks for phys in the above) because until you do, you don't need
child nodes.

> The gpio in question is called "mute", so I'd think the gpio-syscon driver
> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> all the register voodoo in the driver itself and not define it in the dt.

Is there really just one GPIO? If it has a defined function, then is
it really GP? Can you control direction? I know Linus W doesn't like
that kind of abuse of GPIO.

> So it should probably look like
>
> grf: syscon at ff100000 {
>         compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>
>         [all the other syscon sub-devices]
>
>         gpio_mute: gpio-mute {
>                 compatible = "rockchip,rk3328-gpio-mute";
>                 gpio-controller;
>                 #gpio-cells = <2>;
>         };
>
>         [more other syscon sub-devices]
> };

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin Du]

Hi all, I'd like to quote reply of Robin Murphy at
  http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html

>
> I would suggest s/pin number/bit number in the associated GRF register/
> here. At least in this RK3328 case there's only one pin, which isn't
> numbered, and if you naively considered it pin 0 of this 'bank' you'd
> already have the wrong number. Since we're dealing with the "random
> SoC-specific controls" region of the GRF as opposed to the
> relatively-consistent and organised pinmux parts, I don't think we
> should rely on any assumptions about how things are laid out.
>
> I was initially going to suggest a more specific compatible string as
> well, but on reflection I think the generic "rockchip,gpio-syscon" for
> basic "flip this single GRF bit" functionality actually is the right way
> to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
> total related to this pin - the enable, value, and some pull controls
> (which I assume apply when the output is disabled) - if at some point in
> future we *did* want to start explicitly controlling the rest of them
> too, then would be a good time to define a separate
> "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
> for that specialised functionality, independently of this basic one.


Shall we go the generic "rockchip,gpio-syscon" way, or the specific
  "rockchip,rk3328-gpio-mute" way? I prefer the former one.

The property of "gpio,syscon-dev" in gpio-syscon driver should be 
documented.
Since the gpio controller is defined in the dtsi file, which inevitably 
contains voodoo
register addresses. But at the board level dts file, there won't be more 
register
addresses.

On 2018-05-24 3:53 AM, Rob Herring wrote:
> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote:
>> Hi Rob, Levin,
>>
>> sorry for being late to the party.
>>
>> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>>>> On 2018-05-23 2:02 AM, Rob Herring wrote:
>>>>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
>>>>>> From: Levin Du <djw@t-chip.com.cn>
>>>>>>
>>>>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>>>>>> which do not belong to the general pinctrl.
>>>>>>
>>>>>> Adding gpio-syscon support makes controlling regulator or
>>>>>> LED using these special pins very easy by reusing existing
>>>>>> drivers, such as gpio-regulator and led-gpio.
>>>>>>
>>>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Rename gpio_syscon10 to gpio_mute in doc
>>>>>>
>>>>>> Changes in v1:
>>>>>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>>>>>> - Add doc rockchip,gpio-syscon.txt
>>>>>>
>>>>>>    .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>>>>>>
>>>>>> ++++++++++++++++++++++
>>>>>>
>>>>>>    drivers/gpio/gpio-syscon.c                         | 30
>>>>>>
>>>>>> ++++++++++++++++
>>>>>>
>>>>>>    2 files changed, 71 insertions(+)
>>>>>>    create mode 100644
>>>>>>
>>>>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..b1b2a67
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>> @@ -0,0 +1,41 @@
>>>>>> +* Rockchip GPIO support for GRF_SOC_CON registers
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: Should contain "rockchip,gpio-syscon".
>>>>>> +- gpio-controller: Marks the device node as a gpio controller.
>>>>>> +- #gpio-cells: Should be two. The first cell is the pin number and
>>>>>> +  the second cell is used to specify the gpio polarity:
>>>>>> +    0 = Active high,
>>>>>> +    1 = Active low.
>>>>> There's no need for this child node. Just make the parent node a gpio
>>>>> controller.
>>>>>
>>>>> Rob
>>>> Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>>>> a
>>>> gpio controller,
>>>> like below?
>>>>
>>>> +    grf: syscon at ff100000 {
>>>> +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>>>> "syscon", "simple-mfd";
>>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>> I would disagree quite a bit here. The grf are the "general register files",
>> a bunch of registers used for quite a lot of things, and so it seems
>> among other users, also a gpio-controller for some more random pins
>> not controlled through the regular gpio controllers.
>>
>> For a more fully stocked grf, please see
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>>
>> So the gpio controller should definitly also be a subnode.
> Sigh, yes, if there are a bunch of functions needing subnodes like the
> above, then yes that makes sense. But that's not what has been
> presented. Please make some attempt at defining *all* the functions.
> An actual binding would be nice, but I'll settle for just a list of
> things. The list should have functions that have DT dependencies (like
> clocks for phys in the above) because until you do, you don't need
> child nodes.

In rk3328.dtsi file, there are lots of line "rockchip,grf = <&grf>;" in 
various nodes,
such as tsadc,  cru, gmac2io, gmac2phy, and also pinctrl, which are not 
sub nodes of
`grf`, but for reference only. The gpio-syscon node should also have 
similar behavior.
  They are not strongly coupled. The gpio-syscon node should be defined 
outside of the
`grf` node.


>
>> The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> all the register voodoo in the driver itself and not define it in the dt.
> Is there really just one GPIO? If it has a defined function, then is
> it really GP? Can you control direction? I know Linus W doesn't like
> that kind of abuse of GPIO.

The "mute" pin is a output only GPIO, which is already supported by 
setting flags in the gpio-syscon
  driver. And yes, this pin has a defined function, but can also be used 
for general purpose operation.


Thanks
Levin

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin Du]

Hi all, I'd like to quote reply of Robin Murphy at
 ?http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html

>
> I would suggest s/pin number/bit number in the associated GRF register/
> here. At least in this RK3328 case there's only one pin, which isn't
> numbered, and if you naively considered it pin 0 of this 'bank' you'd
> already have the wrong number. Since we're dealing with the "random
> SoC-specific controls" region of the GRF as opposed to the
> relatively-consistent and organised pinmux parts, I don't think we
> should rely on any assumptions about how things are laid out.
>
> I was initially going to suggest a more specific compatible string as
> well, but on reflection I think the generic "rockchip,gpio-syscon" for
> basic "flip this single GRF bit" functionality actually is the right way
> to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
> total related to this pin - the enable, value, and some pull controls
> (which I assume apply when the output is disabled) - if at some point in
> future we *did* want to start explicitly controlling the rest of them
> too, then would be a good time to define a separate
> "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
> for that specialised functionality, independently of this basic one.


Shall we go the generic "rockchip,gpio-syscon" way, or the specific
 ?"rockchip,rk3328-gpio-mute" way? I prefer the former one.

The property of "gpio,syscon-dev" in gpio-syscon driver should be 
documented.
Since the gpio controller is defined in the dtsi file, which inevitably 
contains voodoo
register addresses. But at the board level dts file, there won't be more 
register
addresses.

On 2018-05-24 3:53 AM, Rob Herring wrote:
> On Wed, May 23, 2018 at 10:12 AM, Heiko St?bner <heiko@sntech.de> wrote:
>> Hi Rob, Levin,
>>
>> sorry for being late to the party.
>>
>> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>>>> On 2018-05-23 2:02 AM, Rob Herring wrote:
>>>>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
>>>>>> From: Levin Du <djw@t-chip.com.cn>
>>>>>>
>>>>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>>>>>> which do not belong to the general pinctrl.
>>>>>>
>>>>>> Adding gpio-syscon support makes controlling regulator or
>>>>>> LED using these special pins very easy by reusing existing
>>>>>> drivers, such as gpio-regulator and led-gpio.
>>>>>>
>>>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Rename gpio_syscon10 to gpio_mute in doc
>>>>>>
>>>>>> Changes in v1:
>>>>>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>>>>>> - Add doc rockchip,gpio-syscon.txt
>>>>>>
>>>>>>    .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>>>>>>
>>>>>> ++++++++++++++++++++++
>>>>>>
>>>>>>    drivers/gpio/gpio-syscon.c                         | 30
>>>>>>
>>>>>> ++++++++++++++++
>>>>>>
>>>>>>    2 files changed, 71 insertions(+)
>>>>>>    create mode 100644
>>>>>>
>>>>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..b1b2a67
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>> @@ -0,0 +1,41 @@
>>>>>> +* Rockchip GPIO support for GRF_SOC_CON registers
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: Should contain "rockchip,gpio-syscon".
>>>>>> +- gpio-controller: Marks the device node as a gpio controller.
>>>>>> +- #gpio-cells: Should be two. The first cell is the pin number and
>>>>>> +  the second cell is used to specify the gpio polarity:
>>>>>> +    0 = Active high,
>>>>>> +    1 = Active low.
>>>>> There's no need for this child node. Just make the parent node a gpio
>>>>> controller.
>>>>>
>>>>> Rob
>>>> Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>>>> a
>>>> gpio controller,
>>>> like below?
>>>>
>>>> +    grf: syscon at ff100000 {
>>>> +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>>>> "syscon", "simple-mfd";
>>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>> I would disagree quite a bit here. The grf are the "general register files",
>> a bunch of registers used for quite a lot of things, and so it seems
>> among other users, also a gpio-controller for some more random pins
>> not controlled through the regular gpio controllers.
>>
>> For a more fully stocked grf, please see
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>>
>> So the gpio controller should definitly also be a subnode.
> Sigh, yes, if there are a bunch of functions needing subnodes like the
> above, then yes that makes sense. But that's not what has been
> presented. Please make some attempt at defining *all* the functions.
> An actual binding would be nice, but I'll settle for just a list of
> things. The list should have functions that have DT dependencies (like
> clocks for phys in the above) because until you do, you don't need
> child nodes.

In rk3328.dtsi file, there are lots of line "rockchip,grf = <&grf>;" in 
various nodes,
such as tsadc,? cru, gmac2io, gmac2phy, and also pinctrl, which are not 
sub nodes of
`grf`, but for reference only. The gpio-syscon node should also have 
similar behavior.
 ?They are not strongly coupled. The gpio-syscon node should be defined 
outside of the
`grf` node.


>
>> The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> all the register voodoo in the driver itself and not define it in the dt.
> Is there really just one GPIO? If it has a defined function, then is
> it really GP? Can you control direction? I know Linus W doesn't like
> that kind of abuse of GPIO.

The "mute" pin is a output only GPIO, which is already supported by 
setting flags in the gpio-syscon
 ?driver. And yes, this pin has a defined function, but can also be used 
for general purpose operation.


Thanks
Levin

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Linus Walleij]

On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner <heiko@sntech.de> wrote:

> So the gpio controller should definitly also be a subnode.
>
> The gpio in question is called "mute", so I'd think the gpio-syscon driver
> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> all the register voodoo in the driver itself and not define it in the dt.
>
> So it should probably look like
>
> grf: syscon at ff100000 {
>         compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>
>         [all the other syscon sub-devices]
>
>         gpio_mute: gpio-mute {
>                 compatible = "rockchip,rk3328-gpio-mute";
>                 gpio-controller;
>                 #gpio-cells = <2>;
>         };

I'm sceptic.

That doesn't sound like "general purpose input output" at all.

It sounds like special purpose, for a mute button.

Does it use IRQ? I would recommend implementing
drivers/input/keyboard/syscon-keys.c in the same vein
as drivers/leds/leds-syscon.c so you can avoid indirection
through GPIO for no good reason at all.

I already have other good uses for such a generic
input driver.

Yours,
Linus Walleij

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Linus Walleij]

On Wed, May 23, 2018 at 5:12 PM, Heiko St?bner <heiko@sntech.de> wrote:

> So the gpio controller should definitly also be a subnode.
>
> The gpio in question is called "mute", so I'd think the gpio-syscon driver
> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> all the register voodoo in the driver itself and not define it in the dt.
>
> So it should probably look like
>
> grf: syscon at ff100000 {
>         compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>
>         [all the other syscon sub-devices]
>
>         gpio_mute: gpio-mute {
>                 compatible = "rockchip,rk3328-gpio-mute";
>                 gpio-controller;
>                 #gpio-cells = <2>;
>         };

I'm sceptic.

That doesn't sound like "general purpose input output" at all.

It sounds like special purpose, for a mute button.

Does it use IRQ? I would recommend implementing
drivers/input/keyboard/syscon-keys.c in the same vein
as drivers/leds/leds-syscon.c so you can avoid indirection
through GPIO for no good reason at all.

I already have other good uses for such a generic
input driver.

Yours,
Linus Walleij

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stübner]

Hi Linus,

Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij:
> On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner <heiko@sntech.de> wrote:
> > So the gpio controller should definitly also be a subnode.
> > 
> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> > all the register voodoo in the driver itself and not define it in the dt.
> > 
> > So it should probably look like
> > 
> > grf: syscon at ff100000 {
> > 
> >         compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> >         
> >         [all the other syscon sub-devices]
> >         
> >         gpio_mute: gpio-mute {
> >         
> >                 compatible = "rockchip,rk3328-gpio-mute";
> >                 gpio-controller;
> >                 #gpio-cells = <2>;
> >         
> >         };
> 
> I'm sceptic.
> 
> That doesn't sound like "general purpose input output" at all.
> 
> It sounds like special purpose, for a mute button.
> 
> Does it use IRQ? I would recommend implementing
> drivers/input/keyboard/syscon-keys.c in the same vein
> as drivers/leds/leds-syscon.c so you can avoid indirection
> through GPIO for no good reason at all.

To quote Levin from the other mail:
--------
The "mute" pin is a output only GPIO, which is already supported by 
setting flags in the gpio-syscon
  driver. And yes, this pin has a defined function, but can also be used 
for general purpose operation.
--------

So to summarize, the documentation calls it "mute", but it is usable as
a general pin, which is the reason Levin is working on it - because on his
board this pin is used to switch between two voltages (aka a gpio-regulator)
for the sdmmc controller [3.3V + 1.8V].

Available pin settings are output-enable + of course the high/low setting
and I think I remember there is even a pull setting for it in the GRF 
somewhere - but my memory might be fuzzy here.


Heiko

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stübner]

Hi Linus,

Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij:
> On Wed, May 23, 2018 at 5:12 PM, Heiko St?bner <heiko@sntech.de> wrote:
> > So the gpio controller should definitly also be a subnode.
> > 
> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> > all the register voodoo in the driver itself and not define it in the dt.
> > 
> > So it should probably look like
> > 
> > grf: syscon at ff100000 {
> > 
> >         compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> >         
> >         [all the other syscon sub-devices]
> >         
> >         gpio_mute: gpio-mute {
> >         
> >                 compatible = "rockchip,rk3328-gpio-mute";
> >                 gpio-controller;
> >                 #gpio-cells = <2>;
> >         
> >         };
> 
> I'm sceptic.
> 
> That doesn't sound like "general purpose input output" at all.
> 
> It sounds like special purpose, for a mute button.
> 
> Does it use IRQ? I would recommend implementing
> drivers/input/keyboard/syscon-keys.c in the same vein
> as drivers/leds/leds-syscon.c so you can avoid indirection
> through GPIO for no good reason at all.

To quote Levin from the other mail:
--------
The "mute" pin is a output only GPIO, which is already supported by 
setting flags in the gpio-syscon
  driver. And yes, this pin has a defined function, but can also be used 
for general purpose operation.
--------

So to summarize, the documentation calls it "mute", but it is usable as
a general pin, which is the reason Levin is working on it - because on his
board this pin is used to switch between two voltages (aka a gpio-regulator)
for the sdmmc controller [3.3V + 1.8V].

Available pin settings are output-enable + of course the high/low setting
and I think I remember there is even a pull setting for it in the GRF 
somewhere - but my memory might be fuzzy here.


Heiko

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Linus Walleij]

On Thu, May 24, 2018 at 10:35 AM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij:
>> On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner <heiko@sntech.de> wrote:
>> > So the gpio controller should definitly also be a subnode.
>> >
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>> >
>> > So it should probably look like
>> >
>> > grf: syscon at ff100000 {
>> >
>> >         compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>> >
>> >         [all the other syscon sub-devices]
>> >
>> >         gpio_mute: gpio-mute {
>> >
>> >                 compatible = "rockchip,rk3328-gpio-mute";
>> >                 gpio-controller;
>> >                 #gpio-cells = <2>;
>> >
>> >         };
>>
>> I'm sceptic.
>>
>> That doesn't sound like "general purpose input output" at all.
>>
>> It sounds like special purpose, for a mute button.
>>
>> Does it use IRQ? I would recommend implementing
>> drivers/input/keyboard/syscon-keys.c in the same vein
>> as drivers/leds/leds-syscon.c so you can avoid indirection
>> through GPIO for no good reason at all.
>
> To quote Levin from the other mail:
> --------
> The "mute" pin is a output only GPIO, which is already supported by
> setting flags in the gpio-syscon
>   driver. And yes, this pin has a defined function, but can also be used
> for general purpose operation.
> --------
>
> So to summarize, the documentation calls it "mute", but it is usable as
> a general pin, which is the reason Levin is working on it - because on his
> board this pin is used to switch between two voltages (aka a gpio-regulator)
> for the sdmmc controller [3.3V + 1.8V].

OK then, I was wrong! :)

Go ahead with this, sorry for the fuzz.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Linus Walleij]

On Thu, May 24, 2018 at 10:35 AM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij:
>> On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner <heiko@sntech.de> wrote:
>> > So the gpio controller should definitly also be a subnode.
>> >
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>> >
>> > So it should probably look like
>> >
>> > grf: syscon at ff100000 {
>> >
>> >         compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>> >
>> >         [all the other syscon sub-devices]
>> >
>> >         gpio_mute: gpio-mute {
>> >
>> >                 compatible = "rockchip,rk3328-gpio-mute";
>> >                 gpio-controller;
>> >                 #gpio-cells = <2>;
>> >
>> >         };
>>
>> I'm sceptic.
>>
>> That doesn't sound like "general purpose input output" at all.
>>
>> It sounds like special purpose, for a mute button.
>>
>> Does it use IRQ? I would recommend implementing
>> drivers/input/keyboard/syscon-keys.c in the same vein
>> as drivers/leds/leds-syscon.c so you can avoid indirection
>> through GPIO for no good reason at all.
>
> To quote Levin from the other mail:
> --------
> The "mute" pin is a output only GPIO, which is already supported by
> setting flags in the gpio-syscon
>   driver. And yes, this pin has a defined function, but can also be used
> for general purpose operation.
> --------
>
> So to summarize, the documentation calls it "mute", but it is usable as
> a general pin, which is the reason Levin is working on it - because on his
> board this pin is used to switch between two voltages (aka a gpio-regulator)
> for the sdmmc controller [3.3V + 1.8V].

OK then, I was wrong! :)

Go ahead with this, sorry for the fuzz.

Yours,
Linus Walleij

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Linus Walleij]

On Thu, May 24, 2018 at 10:35 AM, Heiko St?bner <heiko@sntech.de> wrote:
> Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij:
>> On Wed, May 23, 2018 at 5:12 PM, Heiko St?bner <heiko@sntech.de> wrote:
>> > So the gpio controller should definitly also be a subnode.
>> >
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>> >
>> > So it should probably look like
>> >
>> > grf: syscon at ff100000 {
>> >
>> >         compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>> >
>> >         [all the other syscon sub-devices]
>> >
>> >         gpio_mute: gpio-mute {
>> >
>> >                 compatible = "rockchip,rk3328-gpio-mute";
>> >                 gpio-controller;
>> >                 #gpio-cells = <2>;
>> >
>> >         };
>>
>> I'm sceptic.
>>
>> That doesn't sound like "general purpose input output" at all.
>>
>> It sounds like special purpose, for a mute button.
>>
>> Does it use IRQ? I would recommend implementing
>> drivers/input/keyboard/syscon-keys.c in the same vein
>> as drivers/leds/leds-syscon.c so you can avoid indirection
>> through GPIO for no good reason at all.
>
> To quote Levin from the other mail:
> --------
> The "mute" pin is a output only GPIO, which is already supported by
> setting flags in the gpio-syscon
>   driver. And yes, this pin has a defined function, but can also be used
> for general purpose operation.
> --------
>
> So to summarize, the documentation calls it "mute", but it is usable as
> a general pin, which is the reason Levin is working on it - because on his
> board this pin is used to switch between two voltages (aka a gpio-regulator)
> for the sdmmc controller [3.3V + 1.8V].

OK then, I was wrong! :)

Go ahead with this, sorry for the fuzz.

Yours,
Linus Walleij

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stuebner]

Hi Rob,

Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote:
> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
> >> >>> From: Levin Du <djw@t-chip.com.cn>
> >> >>>
> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >> >>> which do not belong to the general pinctrl.
> >> >>>
> >> >>> Adding gpio-syscon support makes controlling regulator or
> >> >>> LED using these special pins very easy by reusing existing
> >> >>> drivers, such as gpio-regulator and led-gpio.
> >> >>>
> >> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
> >> >>>
> >> >>> ---
> >> >>>
> >> >>> Changes in v2:
> >> >>> - Rename gpio_syscon10 to gpio_mute in doc
> >> >>>
> >> >>> Changes in v1:
> >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
> >> >>> - Add doc rockchip,gpio-syscon.txt
> >> >>>
> >> >>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
> >> >>>
> >> >>> ++++++++++++++++++++++
> >> >>>
> >> >>>   drivers/gpio/gpio-syscon.c                         | 30
> >> >>>
> >> >>> ++++++++++++++++
> >> >>>
> >> >>>   2 files changed, 71 insertions(+)
> >> >>>   create mode 100644
> >> >>>
> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>>
> >> >>> diff --git
> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> new file mode 100644
> >> >>> index 0000000..b1b2a67
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> @@ -0,0 +1,41 @@
> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >> >>> +
> >> >>> +Required properties:
> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >> >>> +  the second cell is used to specify the gpio polarity:
> >> >>> +    0 = Active high,
> >> >>> +    1 = Active low.
> >> >>
> >> >> There's no need for this child node. Just make the parent node a gpio
> >> >> controller.
> >> >>
> >> >> Rob
> >> >
> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> >> > a
> >> > gpio controller,
> >> > like below?
> >> >
> >> > +    grf: syscon at ff100000 {
> >> > +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> >> > "syscon", "simple-mfd";
> >>
> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
> >
> > I would disagree quite a bit here. The grf are the "general register files",
> > a bunch of registers used for quite a lot of things, and so it seems
> > among other users, also a gpio-controller for some more random pins
> > not controlled through the regular gpio controllers.
> >
> > For a more fully stocked grf, please see
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
> >
> > So the gpio controller should definitly also be a subnode.
> 
> Sigh, yes, if there are a bunch of functions needing subnodes like the
> above, then yes that makes sense. But that's not what has been
> presented. Please make some attempt at defining *all* the functions.
> An actual binding would be nice, but I'll settle for just a list of
> things. The list should have functions that have DT dependencies (like
> clocks for phys in the above) because until you do, you don't need
> child nodes.

That's the problem with the Rockchip-GRF, you only realize its content
when implementing specific features. 

Like on the rk3399 the table of the register-list of the GRF alone is 11
pages long with the register details tables taking up another 230 pages.
And functional description is often somewhat thin.

So I'm not sure I fully understand what you're asking, but in general
we define the bindings for sub-devices when tackling these individual
components, see for example
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8

which also has a real phy-driver behind it and binding against that
subnode of the GRF simple-mfd.

These are real IP blocks somewhere on the socs, with regular supplies
like resets, clocks etc in most cases. Only their controlling registers
got dumped into the GRF for some reason.

And in retrospect it really looks like we're doing something right,
because it seems these bindings seem quite stable over time.


> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> > all the register voodoo in the driver itself and not define it in the dt.
> 
> Is there really just one GPIO? If it has a defined function, then is
> it really GP? Can you control direction? I know Linus W doesn't like
> that kind of abuse of GPIO.

looks like I convinced Linus that we're not abusing anything with this :-) .


Heiko

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stuebner]

Hi Rob,

Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
> On Wed, May 23, 2018 at 10:12 AM, Heiko St?bner <heiko@sntech.de> wrote:
> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
> >> >>> From: Levin Du <djw@t-chip.com.cn>
> >> >>>
> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >> >>> which do not belong to the general pinctrl.
> >> >>>
> >> >>> Adding gpio-syscon support makes controlling regulator or
> >> >>> LED using these special pins very easy by reusing existing
> >> >>> drivers, such as gpio-regulator and led-gpio.
> >> >>>
> >> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
> >> >>>
> >> >>> ---
> >> >>>
> >> >>> Changes in v2:
> >> >>> - Rename gpio_syscon10 to gpio_mute in doc
> >> >>>
> >> >>> Changes in v1:
> >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
> >> >>> - Add doc rockchip,gpio-syscon.txt
> >> >>>
> >> >>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
> >> >>>
> >> >>> ++++++++++++++++++++++
> >> >>>
> >> >>>   drivers/gpio/gpio-syscon.c                         | 30
> >> >>>
> >> >>> ++++++++++++++++
> >> >>>
> >> >>>   2 files changed, 71 insertions(+)
> >> >>>   create mode 100644
> >> >>>
> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>>
> >> >>> diff --git
> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> new file mode 100644
> >> >>> index 0000000..b1b2a67
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> @@ -0,0 +1,41 @@
> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >> >>> +
> >> >>> +Required properties:
> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >> >>> +  the second cell is used to specify the gpio polarity:
> >> >>> +    0 = Active high,
> >> >>> +    1 = Active low.
> >> >>
> >> >> There's no need for this child node. Just make the parent node a gpio
> >> >> controller.
> >> >>
> >> >> Rob
> >> >
> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> >> > a
> >> > gpio controller,
> >> > like below?
> >> >
> >> > +    grf: syscon at ff100000 {
> >> > +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> >> > "syscon", "simple-mfd";
> >>
> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
> >
> > I would disagree quite a bit here. The grf are the "general register files",
> > a bunch of registers used for quite a lot of things, and so it seems
> > among other users, also a gpio-controller for some more random pins
> > not controlled through the regular gpio controllers.
> >
> > For a more fully stocked grf, please see
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
> >
> > So the gpio controller should definitly also be a subnode.
> 
> Sigh, yes, if there are a bunch of functions needing subnodes like the
> above, then yes that makes sense. But that's not what has been
> presented. Please make some attempt at defining *all* the functions.
> An actual binding would be nice, but I'll settle for just a list of
> things. The list should have functions that have DT dependencies (like
> clocks for phys in the above) because until you do, you don't need
> child nodes.

That's the problem with the Rockchip-GRF, you only realize its content
when implementing specific features. 

Like on the rk3399 the table of the register-list of the GRF alone is 11
pages long with the register details tables taking up another 230 pages.
And functional description is often somewhat thin.

So I'm not sure I fully understand what you're asking, but in general
we define the bindings for sub-devices when tackling these individual
components, see for example
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8

which also has a real phy-driver behind it and binding against that
subnode of the GRF simple-mfd.

These are real IP blocks somewhere on the socs, with regular supplies
like resets, clocks etc in most cases. Only their controlling registers
got dumped into the GRF for some reason.

And in retrospect it really looks like we're doing something right,
because it seems these bindings seem quite stable over time.


> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> > all the register voodoo in the driver itself and not define it in the dt.
> 
> Is there really just one GPIO? If it has a defined function, then is
> it really GP? Can you control direction? I know Linus W doesn't like
> that kind of abuse of GPIO.

looks like I convinced Linus that we're not abusing anything with this :-) .


Heiko

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stuebner]

Hi Levin,

Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du:
> Hi all, I'd like to quote reply of Robin Murphy at
>   http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html
> 
> >
> > I would suggest s/pin number/bit number in the associated GRF register/
> > here. At least in this RK3328 case there's only one pin, which isn't
> > numbered, and if you naively considered it pin 0 of this 'bank' you'd
> > already have the wrong number. Since we're dealing with the "random
> > SoC-specific controls" region of the GRF as opposed to the
> > relatively-consistent and organised pinmux parts, I don't think we
> > should rely on any assumptions about how things are laid out.
> >
> > I was initially going to suggest a more specific compatible string as
> > well, but on reflection I think the generic "rockchip,gpio-syscon" for
> > basic "flip this single GRF bit" functionality actually is the right way
> > to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
> > total related to this pin - the enable, value, and some pull controls
> > (which I assume apply when the output is disabled) - if at some point in
> > future we *did* want to start explicitly controlling the rest of them
> > too, then would be a good time to define a separate
> > "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
> > for that specialised functionality, independently of this basic one.
> 
> 
> Shall we go the generic "rockchip,gpio-syscon" way, or the specific
>   "rockchip,rk3328-gpio-mute" way? I prefer the former one.
> 
> The property of "gpio,syscon-dev" in gpio-syscon driver should be 
> documented.
> Since the gpio controller is defined in the dtsi file, which inevitably 
> contains voodoo
> register addresses. But at the board level dts file, there won't be more 
> register
> addresses.

Past experience shows that the GRF is not really suitable for
generalization, as it's more of a dumping ground where chip designers
can put everything that's left over. This is especially true for
GRF_SOC_CONx registers, that really only contain pretty random bits.

So personally, I'd really prefer soc-specific compatibles as everywhere
else, instead of trying to push stuff into the devicetree that won't hold
up on future socs.


> On 2018-05-24 3:53 AM, Rob Herring wrote:
> > On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote:
> >> Hi Rob, Levin,
> >>
> >> sorry for being late to the party.
> >>
> >> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> >>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
> >>>> On 2018-05-23 2:02 AM, Rob Herring wrote:
> >>>>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
> >>>>>> From: Levin Du <djw@t-chip.com.cn>
> >>>>>>
> >>>>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >>>>>> which do not belong to the general pinctrl.
> >>>>>>
> >>>>>> Adding gpio-syscon support makes controlling regulator or
> >>>>>> LED using these special pins very easy by reusing existing
> >>>>>> drivers, such as gpio-regulator and led-gpio.
> >>>>>>
> >>>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> Changes in v2:
> >>>>>> - Rename gpio_syscon10 to gpio_mute in doc
> >>>>>>
> >>>>>> Changes in v1:
> >>>>>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
> >>>>>> - Add doc rockchip,gpio-syscon.txt
> >>>>>>
> >>>>>>    .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
> >>>>>>
> >>>>>> ++++++++++++++++++++++
> >>>>>>
> >>>>>>    drivers/gpio/gpio-syscon.c                         | 30
> >>>>>>
> >>>>>> ++++++++++++++++
> >>>>>>
> >>>>>>    2 files changed, 71 insertions(+)
> >>>>>>    create mode 100644
> >>>>>>
> >>>>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>>>>>
> >>>>>> diff --git
> >>>>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>>>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>>>>> new file mode 100644
> >>>>>> index 0000000..b1b2a67
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>>>>> @@ -0,0 +1,41 @@
> >>>>>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >>>>>> +
> >>>>>> +Required properties:
> >>>>>> +- compatible: Should contain "rockchip,gpio-syscon".
> >>>>>> +- gpio-controller: Marks the device node as a gpio controller.
> >>>>>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >>>>>> +  the second cell is used to specify the gpio polarity:
> >>>>>> +    0 = Active high,
> >>>>>> +    1 = Active low.
> >>>>> There's no need for this child node. Just make the parent node a gpio
> >>>>> controller.
> >>>>>
> >>>>> Rob
> >>>> Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> >>>> a
> >>>> gpio controller,
> >>>> like below?
> >>>>
> >>>> +    grf: syscon at ff100000 {
> >>>> +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> >>>> "syscon", "simple-mfd";
> >>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
> >> I would disagree quite a bit here. The grf are the "general register files",
> >> a bunch of registers used for quite a lot of things, and so it seems
> >> among other users, also a gpio-controller for some more random pins
> >> not controlled through the regular gpio controllers.
> >>
> >> For a more fully stocked grf, please see
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
> >>
> >> So the gpio controller should definitly also be a subnode.
> > Sigh, yes, if there are a bunch of functions needing subnodes like the
> > above, then yes that makes sense. But that's not what has been
> > presented. Please make some attempt at defining *all* the functions.
> > An actual binding would be nice, but I'll settle for just a list of
> > things. The list should have functions that have DT dependencies (like
> > clocks for phys in the above) because until you do, you don't need
> > child nodes.
> 
> In rk3328.dtsi file, there are lots of line "rockchip,grf = <&grf>;" in 
> various nodes,
> such as tsadc,  cru, gmac2io, gmac2phy, and also pinctrl, which are not 
> sub nodes of
> `grf`, but for reference only. The gpio-syscon node should also have 
> similar behavior.
>   They are not strongly coupled. The gpio-syscon node should be defined 
> outside of the
> `grf` node.

Not necessarily.

I.e. things like the tsadc, cru etc have their own register space and only
some minor additional bits inside the GRF.

Other things like some phys and your mute-gpio are _fully embedded_ inside
the GRF and thus become child devices. This describes the hardware layout
way better, helps unclutter the devicetree and also shows this distinction
between "additional bits" and "embedded" clearly.


Heiko

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stuebner]

Hi Levin,

Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du:
> Hi all, I'd like to quote reply of Robin Murphy at
>   http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html
> 
> >
> > I would suggest s/pin number/bit number in the associated GRF register/
> > here. At least in this RK3328 case there's only one pin, which isn't
> > numbered, and if you naively considered it pin 0 of this 'bank' you'd
> > already have the wrong number. Since we're dealing with the "random
> > SoC-specific controls" region of the GRF as opposed to the
> > relatively-consistent and organised pinmux parts, I don't think we
> > should rely on any assumptions about how things are laid out.
> >
> > I was initially going to suggest a more specific compatible string as
> > well, but on reflection I think the generic "rockchip,gpio-syscon" for
> > basic "flip this single GRF bit" functionality actually is the right way
> > to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
> > total related to this pin - the enable, value, and some pull controls
> > (which I assume apply when the output is disabled) - if at some point in
> > future we *did* want to start explicitly controlling the rest of them
> > too, then would be a good time to define a separate
> > "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
> > for that specialised functionality, independently of this basic one.
> 
> 
> Shall we go the generic "rockchip,gpio-syscon" way, or the specific
>   "rockchip,rk3328-gpio-mute" way? I prefer the former one.
> 
> The property of "gpio,syscon-dev" in gpio-syscon driver should be 
> documented.
> Since the gpio controller is defined in the dtsi file, which inevitably 
> contains voodoo
> register addresses. But at the board level dts file, there won't be more 
> register
> addresses.

Past experience shows that the GRF is not really suitable for
generalization, as it's more of a dumping ground where chip designers
can put everything that's left over. This is especially true for
GRF_SOC_CONx registers, that really only contain pretty random bits.

So personally, I'd really prefer soc-specific compatibles as everywhere
else, instead of trying to push stuff into the devicetree that won't hold
up on future socs.


> On 2018-05-24 3:53 AM, Rob Herring wrote:
> > On Wed, May 23, 2018 at 10:12 AM, Heiko St?bner <heiko@sntech.de> wrote:
> >> Hi Rob, Levin,
> >>
> >> sorry for being late to the party.
> >>
> >> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> >>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
> >>>> On 2018-05-23 2:02 AM, Rob Herring wrote:
> >>>>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
> >>>>>> From: Levin Du <djw@t-chip.com.cn>
> >>>>>>
> >>>>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >>>>>> which do not belong to the general pinctrl.
> >>>>>>
> >>>>>> Adding gpio-syscon support makes controlling regulator or
> >>>>>> LED using these special pins very easy by reusing existing
> >>>>>> drivers, such as gpio-regulator and led-gpio.
> >>>>>>
> >>>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> Changes in v2:
> >>>>>> - Rename gpio_syscon10 to gpio_mute in doc
> >>>>>>
> >>>>>> Changes in v1:
> >>>>>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
> >>>>>> - Add doc rockchip,gpio-syscon.txt
> >>>>>>
> >>>>>>    .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
> >>>>>>
> >>>>>> ++++++++++++++++++++++
> >>>>>>
> >>>>>>    drivers/gpio/gpio-syscon.c                         | 30
> >>>>>>
> >>>>>> ++++++++++++++++
> >>>>>>
> >>>>>>    2 files changed, 71 insertions(+)
> >>>>>>    create mode 100644
> >>>>>>
> >>>>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>>>>>
> >>>>>> diff --git
> >>>>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>>>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>>>>> new file mode 100644
> >>>>>> index 0000000..b1b2a67
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>>>>> @@ -0,0 +1,41 @@
> >>>>>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >>>>>> +
> >>>>>> +Required properties:
> >>>>>> +- compatible: Should contain "rockchip,gpio-syscon".
> >>>>>> +- gpio-controller: Marks the device node as a gpio controller.
> >>>>>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >>>>>> +  the second cell is used to specify the gpio polarity:
> >>>>>> +    0 = Active high,
> >>>>>> +    1 = Active low.
> >>>>> There's no need for this child node. Just make the parent node a gpio
> >>>>> controller.
> >>>>>
> >>>>> Rob
> >>>> Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> >>>> a
> >>>> gpio controller,
> >>>> like below?
> >>>>
> >>>> +    grf: syscon at ff100000 {
> >>>> +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> >>>> "syscon", "simple-mfd";
> >>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
> >> I would disagree quite a bit here. The grf are the "general register files",
> >> a bunch of registers used for quite a lot of things, and so it seems
> >> among other users, also a gpio-controller for some more random pins
> >> not controlled through the regular gpio controllers.
> >>
> >> For a more fully stocked grf, please see
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
> >>
> >> So the gpio controller should definitly also be a subnode.
> > Sigh, yes, if there are a bunch of functions needing subnodes like the
> > above, then yes that makes sense. But that's not what has been
> > presented. Please make some attempt at defining *all* the functions.
> > An actual binding would be nice, but I'll settle for just a list of
> > things. The list should have functions that have DT dependencies (like
> > clocks for phys in the above) because until you do, you don't need
> > child nodes.
> 
> In rk3328.dtsi file, there are lots of line "rockchip,grf = <&grf>;" in 
> various nodes,
> such as tsadc,  cru, gmac2io, gmac2phy, and also pinctrl, which are not 
> sub nodes of
> `grf`, but for reference only. The gpio-syscon node should also have 
> similar behavior.
>   They are not strongly coupled. The gpio-syscon node should be defined 
> outside of the
> `grf` node.

Not necessarily.

I.e. things like the tsadc, cru etc have their own register space and only
some minor additional bits inside the GRF.

Other things like some phys and your mute-gpio are _fully embedded_ inside
the GRF and thus become child devices. This describes the hardware layout
way better, helps unclutter the devicetree and also shows this distinction
between "additional bits" and "embedded" clearly.


Heiko

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Thu, May 24, 2018 at 7:07 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> Hi Rob,
>
> Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
>> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote:
>> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
>> >> >>> From: Levin Du <djw@t-chip.com.cn>
>> >> >>>
>> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >> >>> which do not belong to the general pinctrl.
>> >> >>>
>> >> >>> Adding gpio-syscon support makes controlling regulator or
>> >> >>> LED using these special pins very easy by reusing existing
>> >> >>> drivers, such as gpio-regulator and led-gpio.
>> >> >>>
>> >> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>> >> >>>
>> >> >>> ---
>> >> >>>
>> >> >>> Changes in v2:
>> >> >>> - Rename gpio_syscon10 to gpio_mute in doc
>> >> >>>
>> >> >>> Changes in v1:
>> >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> >> >>> - Add doc rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>> >> >>>
>> >> >>> ++++++++++++++++++++++
>> >> >>>
>> >> >>>   drivers/gpio/gpio-syscon.c                         | 30
>> >> >>>
>> >> >>> ++++++++++++++++
>> >> >>>
>> >> >>>   2 files changed, 71 insertions(+)
>> >> >>>   create mode 100644
>> >> >>>
>> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>> diff --git
>> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> new file mode 100644
>> >> >>> index 0000000..b1b2a67
>> >> >>> --- /dev/null
>> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> @@ -0,0 +1,41 @@
>> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >> >>> +
>> >> >>> +Required properties:
>> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >> >>> +  the second cell is used to specify the gpio polarity:
>> >> >>> +    0 = Active high,
>> >> >>> +    1 = Active low.
>> >> >>
>> >> >> There's no need for this child node. Just make the parent node a gpio
>> >> >> controller.
>> >> >>
>> >> >> Rob
>> >> >
>> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>> >> > a
>> >> > gpio controller,
>> >> > like below?
>> >> >
>> >> > +    grf: syscon at ff100000 {
>> >> > +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> >> > "syscon", "simple-mfd";
>> >>
>> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>> >
>> > I would disagree quite a bit here. The grf are the "general register files",
>> > a bunch of registers used for quite a lot of things, and so it seems
>> > among other users, also a gpio-controller for some more random pins
>> > not controlled through the regular gpio controllers.
>> >
>> > For a more fully stocked grf, please see
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>> >
>> > So the gpio controller should definitly also be a subnode.
>>
>> Sigh, yes, if there are a bunch of functions needing subnodes like the
>> above, then yes that makes sense. But that's not what has been
>> presented. Please make some attempt at defining *all* the functions.
>> An actual binding would be nice, but I'll settle for just a list of
>> things. The list should have functions that have DT dependencies (like
>> clocks for phys in the above) because until you do, you don't need
>> child nodes.
>
> That's the problem with the Rockchip-GRF, you only realize its content
> when implementing specific features.
>
> Like on the rk3399 the table of the register-list of the GRF alone is 11
> pages long with the register details tables taking up another 230 pages.
> And functional description is often somewhat thin.

But surely one can scan thru it and have some clue what functions
there are. For example, does this chip have phy registers in GRF?

> So I'm not sure I fully understand what you're asking, but in general
> we define the bindings for sub-devices when tackling these individual
> components, see for example
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8

Yes, and in that case it makes sense. The individual functions
themselves have resources defined in DT like clocks. What I don't want
to see are child nodes defining *only* a compatible and any provider
properties (e.g. #gpio-cells). The only reason to do that is to make
Linux bind a driver, but DT is not a list of drivers to bind.

This is what I don't want to see:

syscon {
  compatible = "foo,soc-sysctrl", "syscon", "simple-mfd";
  reg = <...>;
  clock-controller {
    compatible = "foo,soc-sysctrl-clocks";
    #clock-cells = <1>;
  };
  reset-controller {
    compatible = "foo,soc-sysctrl-resets";
    #reset-cells = <1>;
  };
  gpio {
    compatible = "foo,soc-sysctrl-gpios";
    #gpio-cells = <2>;
    gpio-controller;
  };
};

But rather:

syscon {
  compatible = "foo,soc-sysctrl";
  reg = <...>;
  #clock-cells = <1>;
  #reset-cells = <1>;
  #gpio-cells = <2>;
  gpio-controller;
};

> which also has a real phy-driver behind it and binding against that
> subnode of the GRF simple-mfd.
>
> These are real IP blocks somewhere on the socs, with regular supplies
> like resets, clocks etc in most cases. Only their controlling registers
> got dumped into the GRF for some reason.

I can tell that from your examples, but I can't tell that with this
binding. For this binding, it looks like you are adding a sub-node for
1 register bit. That wouldn't scale if you have 11 page register list.

> And in retrospect it really looks like we're doing something right,
> because it seems these bindings seem quite stable over time.
>
>
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>>
>> Is there really just one GPIO? If it has a defined function, then is
>> it really GP? Can you control direction? I know Linus W doesn't like
>> that kind of abuse of GPIO.
>
> looks like I convinced Linus that we're not abusing anything with this :-) .

Okay, but still my question remains: is it really only 1 GPIO?
Dropping "-mute" would be more future proof if not.

Rob

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Thu, May 24, 2018 at 7:07 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> Hi Rob,
>
> Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
>> On Wed, May 23, 2018 at 10:12 AM, Heiko St?bner <heiko@sntech.de> wrote:
>> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
>> >> >>> From: Levin Du <djw@t-chip.com.cn>
>> >> >>>
>> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >> >>> which do not belong to the general pinctrl.
>> >> >>>
>> >> >>> Adding gpio-syscon support makes controlling regulator or
>> >> >>> LED using these special pins very easy by reusing existing
>> >> >>> drivers, such as gpio-regulator and led-gpio.
>> >> >>>
>> >> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>> >> >>>
>> >> >>> ---
>> >> >>>
>> >> >>> Changes in v2:
>> >> >>> - Rename gpio_syscon10 to gpio_mute in doc
>> >> >>>
>> >> >>> Changes in v1:
>> >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> >> >>> - Add doc rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>> >> >>>
>> >> >>> ++++++++++++++++++++++
>> >> >>>
>> >> >>>   drivers/gpio/gpio-syscon.c                         | 30
>> >> >>>
>> >> >>> ++++++++++++++++
>> >> >>>
>> >> >>>   2 files changed, 71 insertions(+)
>> >> >>>   create mode 100644
>> >> >>>
>> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>> diff --git
>> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> new file mode 100644
>> >> >>> index 0000000..b1b2a67
>> >> >>> --- /dev/null
>> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> @@ -0,0 +1,41 @@
>> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >> >>> +
>> >> >>> +Required properties:
>> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >> >>> +  the second cell is used to specify the gpio polarity:
>> >> >>> +    0 = Active high,
>> >> >>> +    1 = Active low.
>> >> >>
>> >> >> There's no need for this child node. Just make the parent node a gpio
>> >> >> controller.
>> >> >>
>> >> >> Rob
>> >> >
>> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>> >> > a
>> >> > gpio controller,
>> >> > like below?
>> >> >
>> >> > +    grf: syscon at ff100000 {
>> >> > +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> >> > "syscon", "simple-mfd";
>> >>
>> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>> >
>> > I would disagree quite a bit here. The grf are the "general register files",
>> > a bunch of registers used for quite a lot of things, and so it seems
>> > among other users, also a gpio-controller for some more random pins
>> > not controlled through the regular gpio controllers.
>> >
>> > For a more fully stocked grf, please see
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>> >
>> > So the gpio controller should definitly also be a subnode.
>>
>> Sigh, yes, if there are a bunch of functions needing subnodes like the
>> above, then yes that makes sense. But that's not what has been
>> presented. Please make some attempt at defining *all* the functions.
>> An actual binding would be nice, but I'll settle for just a list of
>> things. The list should have functions that have DT dependencies (like
>> clocks for phys in the above) because until you do, you don't need
>> child nodes.
>
> That's the problem with the Rockchip-GRF, you only realize its content
> when implementing specific features.
>
> Like on the rk3399 the table of the register-list of the GRF alone is 11
> pages long with the register details tables taking up another 230 pages.
> And functional description is often somewhat thin.

But surely one can scan thru it and have some clue what functions
there are. For example, does this chip have phy registers in GRF?

> So I'm not sure I fully understand what you're asking, but in general
> we define the bindings for sub-devices when tackling these individual
> components, see for example
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8

Yes, and in that case it makes sense. The individual functions
themselves have resources defined in DT like clocks. What I don't want
to see are child nodes defining *only* a compatible and any provider
properties (e.g. #gpio-cells). The only reason to do that is to make
Linux bind a driver, but DT is not a list of drivers to bind.

This is what I don't want to see:

syscon {
  compatible = "foo,soc-sysctrl", "syscon", "simple-mfd";
  reg = <...>;
  clock-controller {
    compatible = "foo,soc-sysctrl-clocks";
    #clock-cells = <1>;
  };
  reset-controller {
    compatible = "foo,soc-sysctrl-resets";
    #reset-cells = <1>;
  };
  gpio {
    compatible = "foo,soc-sysctrl-gpios";
    #gpio-cells = <2>;
    gpio-controller;
  };
};

But rather:

syscon {
  compatible = "foo,soc-sysctrl";
  reg = <...>;
  #clock-cells = <1>;
  #reset-cells = <1>;
  #gpio-cells = <2>;
  gpio-controller;
};

> which also has a real phy-driver behind it and binding against that
> subnode of the GRF simple-mfd.
>
> These are real IP blocks somewhere on the socs, with regular supplies
> like resets, clocks etc in most cases. Only their controlling registers
> got dumped into the GRF for some reason.

I can tell that from your examples, but I can't tell that with this
binding. For this binding, it looks like you are adding a sub-node for
1 register bit. That wouldn't scale if you have 11 page register list.

> And in retrospect it really looks like we're doing something right,
> because it seems these bindings seem quite stable over time.
>
>
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>>
>> Is there really just one GPIO? If it has a defined function, then is
>> it really GP? Can you control direction? I know Linus W doesn't like
>> that kind of abuse of GPIO.
>
> looks like I convinced Linus that we're not abusing anything with this :-) .

Okay, but still my question remains: is it really only 1 GPIO?
Dropping "-mute" would be more future proof if not.

Rob

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin]

On 2018-05-24 8:18 PM, Heiko Stuebner wrote:
> Hi Levin,
>
> Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du:
>> Hi all, I'd like to quote reply of Robin Murphy at
>>    http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html
>>
>>> I would suggest s/pin number/bit number in the associated GRF register/
>>> here. At least in this RK3328 case there's only one pin, which isn't
>>> numbered, and if you naively considered it pin 0 of this 'bank' you'd
>>> already have the wrong number. Since we're dealing with the "random
>>> SoC-specific controls" region of the GRF as opposed to the
>>> relatively-consistent and organised pinmux parts, I don't think we
>>> should rely on any assumptions about how things are laid out.
>>>
>>> I was initially going to suggest a more specific compatible string as
>>> well, but on reflection I think the generic "rockchip,gpio-syscon" for
>>> basic "flip this single GRF bit" functionality actually is the right way
>>> to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
>>> total related to this pin - the enable, value, and some pull controls
>>> (which I assume apply when the output is disabled) - if at some point in
>>> future we *did* want to start explicitly controlling the rest of them
>>> too, then would be a good time to define a separate
>>> "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
>>> for that specialised functionality, independently of this basic one.
>>
>> Shall we go the generic "rockchip,gpio-syscon" way, or the specific
>>    "rockchip,rk3328-gpio-mute" way? I prefer the former one.
>>
>> The property of "gpio,syscon-dev" in gpio-syscon driver should be
>> documented.
>> Since the gpio controller is defined in the dtsi file, which inevitably
>> contains voodoo
>> register addresses. But at the board level dts file, there won't be more
>> register
>> addresses.
> Past experience shows that the GRF is not really suitable for
> generalization, as it's more of a dumping ground where chip designers
> can put everything that's left over. This is especially true for
> GRF_SOC_CONx registers, that really only contain pretty random bits.
>
> So personally, I'd really prefer soc-specific compatibles as everywhere
> else, instead of trying to push stuff into the devicetree that won't hold
> up on future socs.
>
>
>> On 2018-05-24 3:53 AM, Rob Herring wrote:
>>> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote:
>>>> Hi Rob, Levin,
>>>>
>>>> sorry for being late to the party.
>>>>
>>>> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>>>>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>>>>>> On 2018-05-23 2:02 AM, Rob Herring wrote:
>>>>>>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
>>>>>>>> From: Levin Du <djw@t-chip.com.cn>
>>>>>>>>
>>>>>>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>>>>>>>> which do not belong to the general pinctrl.
>>>>>>>>
>>>>>>>> Adding gpio-syscon support makes controlling regulator or
>>>>>>>> LED using these special pins very easy by reusing existing
>>>>>>>> drivers, such as gpio-regulator and led-gpio.
>>>>>>>>
>>>>>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>> - Rename gpio_syscon10 to gpio_mute in doc
>>>>>>>>
>>>>>>>> Changes in v1:
>>>>>>>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>>>>>>>> - Add doc rockchip,gpio-syscon.txt
>>>>>>>>
>>>>>>>>     .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>>>>>>>>
>>>>>>>> ++++++++++++++++++++++
>>>>>>>>
>>>>>>>>     drivers/gpio/gpio-syscon.c                         | 30
>>>>>>>>
>>>>>>>> ++++++++++++++++
>>>>>>>>
>>>>>>>>     2 files changed, 71 insertions(+)
>>>>>>>>     create mode 100644
>>>>>>>>
>>>>>>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..b1b2a67
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>>> @@ -0,0 +1,41 @@
>>>>>>>> +* Rockchip GPIO support for GRF_SOC_CON registers
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible: Should contain "rockchip,gpio-syscon".
>>>>>>>> +- gpio-controller: Marks the device node as a gpio controller.
>>>>>>>> +- #gpio-cells: Should be two. The first cell is the pin number and
>>>>>>>> +  the second cell is used to specify the gpio polarity:
>>>>>>>> +    0 = Active high,
>>>>>>>> +    1 = Active low.
>>>>>>> There's no need for this child node. Just make the parent node a gpio
>>>>>>> controller.
>>>>>>>
>>>>>>> Rob
>>>>>> Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>>>>>> a
>>>>>> gpio controller,
>>>>>> like below?
>>>>>>
>>>>>> +    grf: syscon at ff100000 {
>>>>>> +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>>>>>> "syscon", "simple-mfd";
>>>>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>>>> I would disagree quite a bit here. The grf are the "general register files",
>>>> a bunch of registers used for quite a lot of things, and so it seems
>>>> among other users, also a gpio-controller for some more random pins
>>>> not controlled through the regular gpio controllers.
>>>>
>>>> For a more fully stocked grf, please see
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>>>>
>>>> So the gpio controller should definitly also be a subnode.
>>> Sigh, yes, if there are a bunch of functions needing subnodes like the
>>> above, then yes that makes sense. But that's not what has been
>>> presented. Please make some attempt at defining *all* the functions.
>>> An actual binding would be nice, but I'll settle for just a list of
>>> things. The list should have functions that have DT dependencies (like
>>> clocks for phys in the above) because until you do, you don't need
>>> child nodes.
>> In rk3328.dtsi file, there are lots of line "rockchip,grf = <&grf>;" in
>> various nodes,
>> such as tsadc,  cru, gmac2io, gmac2phy, and also pinctrl, which are not
>> sub nodes of
>> `grf`, but for reference only. The gpio-syscon node should also have
>> similar behavior.
>>    They are not strongly coupled. The gpio-syscon node should be defined
>> outside of the
>> `grf` node.
> Not necessarily.
>
> I.e. things like the tsadc, cru etc have their own register space and only
> some minor additional bits inside the GRF.
>
> Other things like some phys and your mute-gpio are _fully embedded_ inside
> the GRF and thus become child devices. This describes the hardware layout
> way better, helps unclutter the devicetree and also shows this distinction
> between "additional bits" and "embedded" clearly.
>
>
> Heiko

Your good point convinced me. I'd like to discuss the V3 patch here.

Since there's only one GPIO pin (the GPIO_MUTE pin) in GRF_SOC_CON10 
register,
the GPIO controller is named `gpio-mute` and has only one GPIO pin which is
referred to as `<&gpio-mute 0>`:

In rk3328.dtsi:

     grf: syscon@ff100000 {
         //...
         /* The GPIO_MUTE pin is referred to as <&gpio-mute 0>.*/
         gpio_mute: gpio-mute {
             compatible = "rockchip,rk3328-mute-gpio";
             gpio-controller;
             #gpio-cells = <2>;
         };
     };


In gpio-syscon.c:

   static const struct syscon_gpio_data rockchip_rk3328_mute_gpio = {
          /* rk3328 mute gpio is an output only pin at GRF_SOC_CON10[1] */
         .flags          = GPIO_SYSCON_FEAT_OUT,
         .bit_count      = 1,
         .dat_bit_offset = 0x0428 * 8 + 1,
         .set            = rockchip_gpio_set,
   };

   static const struct of_device_id syscon_gpio_ids[] = {
     //...
     {
         .compatible    = "rockchip,rk3328-mute-gpio",
         .data        = &rockchip_rk3328_mute_gpio,
     },
     {}
   };

Compared to V0 patch, the bit_count changes from 2 to 1 and the 
dat_bit_offset
increases 1. Therefore the GPIO_MUTE pin is now referred to as
`<&gpio-mute 0>`. IMHO it is better than `<&gpio-mute 1>` in the V0 patch.
In V0, `1` is the physical offset of the output pin in register and 
<&gpio-mute 0>
is an invalid GPIO.

Thanks
Levin

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin]

On 2018-05-24 8:18 PM, Heiko Stuebner wrote:
> Hi Levin,
>
> Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du:
>> Hi all, I'd like to quote reply of Robin Murphy at
>>    http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html
>>
>>> I would suggest s/pin number/bit number in the associated GRF register/
>>> here. At least in this RK3328 case there's only one pin, which isn't
>>> numbered, and if you naively considered it pin 0 of this 'bank' you'd
>>> already have the wrong number. Since we're dealing with the "random
>>> SoC-specific controls" region of the GRF as opposed to the
>>> relatively-consistent and organised pinmux parts, I don't think we
>>> should rely on any assumptions about how things are laid out.
>>>
>>> I was initially going to suggest a more specific compatible string as
>>> well, but on reflection I think the generic "rockchip,gpio-syscon" for
>>> basic "flip this single GRF bit" functionality actually is the right way
>>> to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
>>> total related to this pin - the enable, value, and some pull controls
>>> (which I assume apply when the output is disabled) - if at some point in
>>> future we *did* want to start explicitly controlling the rest of them
>>> too, then would be a good time to define a separate
>>> "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
>>> for that specialised functionality, independently of this basic one.
>>
>> Shall we go the generic "rockchip,gpio-syscon" way, or the specific
>>    "rockchip,rk3328-gpio-mute" way? I prefer the former one.
>>
>> The property of "gpio,syscon-dev" in gpio-syscon driver should be
>> documented.
>> Since the gpio controller is defined in the dtsi file, which inevitably
>> contains voodoo
>> register addresses. But at the board level dts file, there won't be more
>> register
>> addresses.
> Past experience shows that the GRF is not really suitable for
> generalization, as it's more of a dumping ground where chip designers
> can put everything that's left over. This is especially true for
> GRF_SOC_CONx registers, that really only contain pretty random bits.
>
> So personally, I'd really prefer soc-specific compatibles as everywhere
> else, instead of trying to push stuff into the devicetree that won't hold
> up on future socs.
>
>
>> On 2018-05-24 3:53 AM, Rob Herring wrote:
>>> On Wed, May 23, 2018 at 10:12 AM, Heiko St?bner <heiko@sntech.de> wrote:
>>>> Hi Rob, Levin,
>>>>
>>>> sorry for being late to the party.
>>>>
>>>> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>>>>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>>>>>> On 2018-05-23 2:02 AM, Rob Herring wrote:
>>>>>>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
>>>>>>>> From: Levin Du <djw@t-chip.com.cn>
>>>>>>>>
>>>>>>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>>>>>>>> which do not belong to the general pinctrl.
>>>>>>>>
>>>>>>>> Adding gpio-syscon support makes controlling regulator or
>>>>>>>> LED using these special pins very easy by reusing existing
>>>>>>>> drivers, such as gpio-regulator and led-gpio.
>>>>>>>>
>>>>>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>> - Rename gpio_syscon10 to gpio_mute in doc
>>>>>>>>
>>>>>>>> Changes in v1:
>>>>>>>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>>>>>>>> - Add doc rockchip,gpio-syscon.txt
>>>>>>>>
>>>>>>>>     .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>>>>>>>>
>>>>>>>> ++++++++++++++++++++++
>>>>>>>>
>>>>>>>>     drivers/gpio/gpio-syscon.c                         | 30
>>>>>>>>
>>>>>>>> ++++++++++++++++
>>>>>>>>
>>>>>>>>     2 files changed, 71 insertions(+)
>>>>>>>>     create mode 100644
>>>>>>>>
>>>>>>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..b1b2a67
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>>>>>> @@ -0,0 +1,41 @@
>>>>>>>> +* Rockchip GPIO support for GRF_SOC_CON registers
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible: Should contain "rockchip,gpio-syscon".
>>>>>>>> +- gpio-controller: Marks the device node as a gpio controller.
>>>>>>>> +- #gpio-cells: Should be two. The first cell is the pin number and
>>>>>>>> +  the second cell is used to specify the gpio polarity:
>>>>>>>> +    0 = Active high,
>>>>>>>> +    1 = Active low.
>>>>>>> There's no need for this child node. Just make the parent node a gpio
>>>>>>> controller.
>>>>>>>
>>>>>>> Rob
>>>>>> Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>>>>>> a
>>>>>> gpio controller,
>>>>>> like below?
>>>>>>
>>>>>> +    grf: syscon at ff100000 {
>>>>>> +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>>>>>> "syscon", "simple-mfd";
>>>>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>>>> I would disagree quite a bit here. The grf are the "general register files",
>>>> a bunch of registers used for quite a lot of things, and so it seems
>>>> among other users, also a gpio-controller for some more random pins
>>>> not controlled through the regular gpio controllers.
>>>>
>>>> For a more fully stocked grf, please see
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>>>>
>>>> So the gpio controller should definitly also be a subnode.
>>> Sigh, yes, if there are a bunch of functions needing subnodes like the
>>> above, then yes that makes sense. But that's not what has been
>>> presented. Please make some attempt at defining *all* the functions.
>>> An actual binding would be nice, but I'll settle for just a list of
>>> things. The list should have functions that have DT dependencies (like
>>> clocks for phys in the above) because until you do, you don't need
>>> child nodes.
>> In rk3328.dtsi file, there are lots of line "rockchip,grf = <&grf>;" in
>> various nodes,
>> such as tsadc,  cru, gmac2io, gmac2phy, and also pinctrl, which are not
>> sub nodes of
>> `grf`, but for reference only. The gpio-syscon node should also have
>> similar behavior.
>>    They are not strongly coupled. The gpio-syscon node should be defined
>> outside of the
>> `grf` node.
> Not necessarily.
>
> I.e. things like the tsadc, cru etc have their own register space and only
> some minor additional bits inside the GRF.
>
> Other things like some phys and your mute-gpio are _fully embedded_ inside
> the GRF and thus become child devices. This describes the hardware layout
> way better, helps unclutter the devicetree and also shows this distinction
> between "additional bits" and "embedded" clearly.
>
>
> Heiko

Your good point convinced me. I'd like to discuss the V3 patch here.

Since there's only one GPIO pin (the GPIO_MUTE pin) in GRF_SOC_CON10 
register,
the GPIO controller is named `gpio-mute` and has only one GPIO pin which is
referred to as `<&gpio-mute 0>`:

In rk3328.dtsi:

 ??? grf: syscon at ff100000 {
 ??????? //...
 ??? ??? /* The GPIO_MUTE pin is referred to as <&gpio-mute 0>.*/
 ??? ??? gpio_mute: gpio-mute {
 ??? ??? ??? compatible = "rockchip,rk3328-mute-gpio";
 ??? ??? ??? gpio-controller;
 ??? ??? ??? #gpio-cells = <2>;
 ??? ??? };
 ??? };


In gpio-syscon.c:

 ? static const struct syscon_gpio_data rockchip_rk3328_mute_gpio = {
 ? ? ???? /* rk3328 mute gpio is an output only pin at GRF_SOC_CON10[1] */
 ??????? .flags????????? = GPIO_SYSCON_FEAT_OUT,
 ??????? .bit_count????? = 1,
 ??????? .dat_bit_offset = 0x0428 * 8 + 1,
 ??????? .set??????????? = rockchip_gpio_set,
 ? };

 ? static const struct of_device_id syscon_gpio_ids[] = {
 ??? //...
 ??? {
 ??? ??? .compatible??? = "rockchip,rk3328-mute-gpio",
 ??? ??? .data??? ??? = &rockchip_rk3328_mute_gpio,
 ??? },
 ??? {}
 ? };

Compared to V0 patch, the bit_count changes from 2 to 1 and the 
dat_bit_offset
increases 1. Therefore the GPIO_MUTE pin is now referred to as
`<&gpio-mute 0>`. IMHO it is better than `<&gpio-mute 1>` in the V0 patch.
In V0, `1` is the physical offset of the output pin in register and 
<&gpio-mute 0>
is an invalid GPIO.

Thanks
Levin