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

[PATCH v3 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,rk3328-gpio-mute` to
the gpio-syscon driver for the access of the GPIO_MUTE pin in rk3328.

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

The ROC-RK3328-CC board use the new gpio <&gpio_mute 0> 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 the Linux people!

Changes in v3:
- Change from general gpio-syscon to specific rk3328-gpio-mute
- Use dedicated "rockchip,rk3328-gpio-mute" driver
- Use <&gpio_mute 0> instead of <&gpio_mute 1> to refer to the GPIO_MUTE pin.

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 and add to rk3328.dtsi for general use.
- Split from V0.
- Split 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: rockchip: add GPIO_MUTE support for rk3328
  arm64: dts: rockchip: Add GPIO_MUTE pin support 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,rk3328-gpio-mute.txt    | 28 ++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts     | 34 ++++++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328.dtsi           |  7 +++++
 drivers/gpio/gpio-syscon.c                         | 33 +++++++++++++++++++++
 4 files changed, 102 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt

-- 
2.7.4

[PATCH v3 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 v3: None
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

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

[Linus Walleij]

On Thu, May 31, 2018 at 5:27 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>

This patch has been sent upstream for v4.18.

Yours,
Linus Walleij

[PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[djw]

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

In Rockchip RK3328, the output only GPIO_MUTE pin, originally for codec
mute control, can also be used for general purpose. It is manipulated by
the GRF_SOC_CON10 register.

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

---

Changes in v3:
- Change from general gpio-syscon to specific rk3328-gpio-mute

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,rk3328-gpio-mute.txt    | 28 +++++++++++++++++++
 drivers/gpio/gpio-syscon.c                         | 31 ++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt

diff --git a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
new file mode 100644
index 0000000..10bc632
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
@@ -0,0 +1,28 @@
+Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE pin.
+
+In Rockchip RK3328, the output only GPIO_MUTE pin, originally for codec mute
+control, can also be used for general purpose. It is manipulated by the
+GRF_SOC_CON10 register.
+
+Required properties:
+- compatible: Should contain "rockchip,rk3328-gpio-mute".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be 2. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+    0 = Active high,
+    1 = Active low.
+
+Example:
+
+	grf: syscon@ff100000 {
+		compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+
+		gpio_mute: gpio-mute {
+			compatible = "rockchip,rk3328-gpio-mute";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
+
+Note: The gpio_mute node should be declared as the child of the GRF (General
+Register File) node. The GPIO_MUTE pin is referred to as <&gpio_mute 0>.
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 7325b86..49a142a 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -135,6 +135,33 @@ 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_rk3328_gpio_mute = {
+	/* RK3328 GPIO_MUTE 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,
+};
+
 #define KEYSTONE_LOCK_BIT BIT(0)
 
 static void keystone_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
@@ -175,6 +202,10 @@ static const struct of_device_id syscon_gpio_ids[] = {
 		.compatible	= "ti,keystone-dsp-gpio",
 		.data		= &keystone_dsp_gpio,
 	},
+	{
+		.compatible	= "rockchip,rk3328-gpio-mute",
+		.data		= &rockchip_rk3328_gpio_mute,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, syscon_gpio_ids);
-- 
2.7.4

Re: [PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[Rob Herring]

On Wed, May 30, 2018 at 10:27 PM,  <djw@t-chip.com.cn> wrote:
> From: Levin Du <djw@t-chip.com.cn>
>
> In Rockchip RK3328, the output only GPIO_MUTE pin, originally for codec
> mute control, can also be used for general purpose. It is manipulated by
> the GRF_SOC_CON10 register.
>
> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>
> ---
>
> Changes in v3:
> - Change from general gpio-syscon to specific rk3328-gpio-mute
>
> 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,rk3328-gpio-mute.txt    | 28 +++++++++++++++++++
>  drivers/gpio/gpio-syscon.c                         | 31 ++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> new file mode 100644
> index 0000000..10bc632
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> @@ -0,0 +1,28 @@
> +Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE pin.
> +
> +In Rockchip RK3328, the output only GPIO_MUTE pin, originally for codec mute
> +control, can also be used for general purpose. It is manipulated by the
> +GRF_SOC_CON10 register.
> +
> +Required properties:
> +- compatible: Should contain "rockchip,rk3328-gpio-mute".
> +- gpio-controller: Marks the device node as a gpio controller.
> +- #gpio-cells: Should be 2. The first cell is the pin number and
> +  the second cell is used to specify the gpio polarity:
> +    0 = Active high,
> +    1 = Active low.
> +
> +Example:
> +
> +       grf: syscon@ff100000 {
> +               compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> +
> +               gpio_mute: gpio-mute {

Node names should be generic:

gpio {

This also means you can't add another GPIO node in the future and
you'll have to live with "rockchip,rk3328-gpio-mute" covering more
than 1 GPIO if you do need to add more GPIOs.

> +                       compatible = "rockchip,rk3328-gpio-mute";
> +                       gpio-controller;
> +                       #gpio-cells = <2>;
> +               };
> +       };
> +
> +Note: The gpio_mute node should be declared as the child of the GRF (General
> +Register File) node. The GPIO_MUTE pin is referred to as <&gpio_mute 0>.

This is wrong because you should have 2 cells. The phandle doesn't
count as a cell.

Rob

Re: [PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[Levin]

Hi Rob,

On 2018-05-31 10:45 PM, Rob Herring wrote:
> On Wed, May 30, 2018 at 10:27 PM,  <djw@t-chip.com.cn> wrote:
>> From: Levin Du <djw@t-chip.com.cn>
>>
>> In Rockchip RK3328, the output only GPIO_MUTE pin, originally for codec
>> mute control, can also be used for general purpose. It is manipulated by
>> the GRF_SOC_CON10 register.
>>
>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>
>> ---
>>
>> Changes in v3:
>> - Change from general gpio-syscon to specific rk3328-gpio-mute
>>
>> 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,rk3328-gpio-mute.txt    | 28 +++++++++++++++++++
>>   drivers/gpio/gpio-syscon.c                         | 31 ++++++++++++++++++++++
>>   2 files changed, 59 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>> new file mode 100644
>> index 0000000..10bc632
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>> @@ -0,0 +1,28 @@
>> +Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE pin.
>> +
>> +In Rockchip RK3328, the output only GPIO_MUTE pin, originally for codec mute
>> +control, can also be used for general purpose. It is manipulated by the
>> +GRF_SOC_CON10 register.
>> +
>> +Required properties:
>> +- compatible: Should contain "rockchip,rk3328-gpio-mute".
>> +- gpio-controller: Marks the device node as a gpio controller.
>> +- #gpio-cells: Should be 2. The first cell is the pin number and
>> +  the second cell is used to specify the gpio polarity:
>> +    0 = Active high,
>> +    1 = Active low.
>> +
>> +Example:
>> +
>> +       grf: syscon@ff100000 {
>> +               compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>> +
>> +               gpio_mute: gpio-mute {
> Node names should be generic:
>
> gpio {
>
> This also means you can't add another GPIO node in the future and
> you'll have to live with "rockchip,rk3328-gpio-mute" covering more
> than 1 GPIO if you do need to add more GPIOs.

As the first line describes, this GPIO controller is dedicated for the 
GPIO_MUTE pin.
There's only one GPIO pin in the GRF_SOC_CON10 register. Therefore the 
gpio_mute
name is proper IMHO.

>> +                       compatible = "rockchip,rk3328-gpio-mute";
>> +                       gpio-controller;
>> +                       #gpio-cells = <2>;
>> +               };
>> +       };
>> +
>> +Note: The gpio_mute node should be declared as the child of the GRF (General
>> +Register File) node. The GPIO_MUTE pin is referred to as <&gpio_mute 0>.
> This is wrong because you should have 2 cells. The phandle doesn't
> count as a cell.
>
> Rob
>
Thanks for pointing that out. So it should be:

    The GPIO_MUTE pin is referred to as <&gpio_mute 0 POLARITY>.


Thanks,
Levin

Re: [PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[Rob Herring]

On Thu, May 31, 2018 at 9:05 PM, Levin <djw@t-chip.com.cn> wrote:
> Hi Rob,
>
>
> On 2018-05-31 10:45 PM, Rob Herring wrote:
>>
>> On Wed, May 30, 2018 at 10:27 PM,  <djw@t-chip.com.cn> wrote:
>>>
>>> From: Levin Du <djw@t-chip.com.cn>
>>>
>>> In Rockchip RK3328, the output only GPIO_MUTE pin, originally for codec
>>> mute control, can also be used for general purpose. It is manipulated by
>>> the GRF_SOC_CON10 register.
>>>
>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>>
>>> ---
>>>
>>> Changes in v3:
>>> - Change from general gpio-syscon to specific rk3328-gpio-mute
>>>
>>> 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,rk3328-gpio-mute.txt    | 28
>>> +++++++++++++++++++
>>>   drivers/gpio/gpio-syscon.c                         | 31
>>> ++++++++++++++++++++++
>>>   2 files changed, 59 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>> b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>> new file mode 100644
>>> index 0000000..10bc632
>>> --- /dev/null
>>> +++
>>> b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>> @@ -0,0 +1,28 @@
>>> +Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE pin.
>>> +
>>> +In Rockchip RK3328, the output only GPIO_MUTE pin, originally for codec
>>> mute
>>> +control, can also be used for general purpose. It is manipulated by the
>>> +GRF_SOC_CON10 register.
>>> +
>>> +Required properties:
>>> +- compatible: Should contain "rockchip,rk3328-gpio-mute".
>>> +- gpio-controller: Marks the device node as a gpio controller.
>>> +- #gpio-cells: Should be 2. The first cell is the pin number and
>>> +  the second cell is used to specify the gpio polarity:
>>> +    0 = Active high,
>>> +    1 = Active low.
>>> +
>>> +Example:
>>> +
>>> +       grf: syscon@ff100000 {
>>> +               compatible = "rockchip,rk3328-grf", "syscon",
>>> "simple-mfd";
>>> +
>>> +               gpio_mute: gpio-mute {
>>
>> Node names should be generic:
>>
>> gpio {
>>
>> This also means you can't add another GPIO node in the future and
>> you'll have to live with "rockchip,rk3328-gpio-mute" covering more
>> than 1 GPIO if you do need to add more GPIOs.
>
>
> As the first line describes, this GPIO controller is dedicated for the
> GPIO_MUTE pin.
> There's only one GPIO pin in the GRF_SOC_CON10 register. Therefore the
> gpio_mute
> name is proper IMHO.

It's how many GPIOs in the GRF, not this register. What I'm saying is
when you come along later to add another GPIO in the GRF, you had
better just add it to this same node. I'm not going to accept another
GPIO controller node within the GRF. You have the cells to support
more than 1, so it would only be a driver change. The compatible
string would then not be ideally named at that point. But compatible
strings are just unique identifiers, so it doesn't really matter what
the string is.

I'm being told both "this is the only GPIO" and "the GRF has too many
different functions for us to tell you what they all are". So which is
it?

Rob

Re: [PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[Levin Du]

Rob Herring <robh+dt@kernel.org> writes:

> On Thu, May 31, 2018 at 9:05 PM, Levin <djw@t-chip.com.cn> 
> wrote:
>> Hi Rob,
>>
>>
>> On 2018-05-31 10:45 PM, Rob Herring wrote:
>>>
>>> On Wed, May 30, 2018 at 10:27 PM,  <djw@t-chip.com.cn> wrote:
>>>>
>>>> From: Levin Du <djw@t-chip.com.cn>
>>>>
>>>> In Rockchip RK3328, the output only GPIO_MUTE pin, originally 
>>>> for codec
>>>> mute control, can also be used for general purpose. It is 
>>>> manipulated by
>>>> the GRF_SOC_CON10 register.
>>>>
>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>>>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - Change from general gpio-syscon to specific 
>>>> rk3328-gpio-mute
>>>>
>>>> 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,rk3328-gpio-mute.txt    | 28
>>>> +++++++++++++++++++
>>>>   drivers/gpio/gpio-syscon.c                         | 31
>>>> ++++++++++++++++++++++
>>>>   2 files changed, 59 insertions(+)
>>>>   create mode 100644
>>>> Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>>> b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>>> new file mode 100644
>>>> index 0000000..10bc632
>>>> --- /dev/null
>>>> +++
>>>> b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>>> @@ -0,0 +1,28 @@
>>>> +Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE 
>>>> pin.
>>>> +
>>>> +In Rockchip RK3328, the output only GPIO_MUTE pin, 
>>>> originally for codec
>>>> mute
>>>> +control, can also be used for general purpose. It is 
>>>> manipulated by the
>>>> +GRF_SOC_CON10 register.
>>>> +
>>>> +Required properties:
>>>> +- compatible: Should contain "rockchip,rk3328-gpio-mute".
>>>> +- gpio-controller: Marks the device node as a gpio 
>>>> controller.
>>>> +- #gpio-cells: Should be 2. The first cell is the pin number 
>>>> and
>>>> +  the second cell is used to specify the gpio polarity:
>>>> +    0 = Active high,
>>>> +    1 = Active low.
>>>> +
>>>> +Example:
>>>> +
>>>> +       grf: syscon@ff100000 {
>>>> +               compatible = "rockchip,rk3328-grf", "syscon",
>>>> "simple-mfd";
>>>> +
>>>> +               gpio_mute: gpio-mute {
>>>
>>> Node names should be generic:
>>>
>>> gpio {
>>>
>>> This also means you can't add another GPIO node in the future 
>>> and
>>> you'll have to live with "rockchip,rk3328-gpio-mute" covering 
>>> more
>>> than 1 GPIO if you do need to add more GPIOs.
>>
>>
>> As the first line describes, this GPIO controller is dedicated 
>> for the
>> GPIO_MUTE pin.
>> There's only one GPIO pin in the GRF_SOC_CON10 register. 
>> Therefore the
>> gpio_mute
>> name is proper IMHO.
>
> It's how many GPIOs in the GRF, not this register. What I'm 
> saying is
> when you come along later to add another GPIO in the GRF, you 
> had
> better just add it to this same node. I'm not going to accept 
> another
> GPIO controller node within the GRF. You have the cells to 
> support
> more than 1, so it would only be a driver change. The compatible
> string would then not be ideally named at that point. But 
> compatible
> strings are just unique identifiers, so it doesn't really matter 
> what
> the string is.
>

I'll try my best to introduce the situation here. The GRF, 
GPIO0~GPIO3
are register blocks in the RK3328 Soc. The GPIO0~GPIO3 contain 
registers
for GPIO operations like reading/writing data, setting direction,
interruption etc, which corresponds to the GPIO banks 
(gpio0~gpio3)
defined in rk3328.dtsi:

	pinctrl: pinctrl {
		compatible = "rockchip,rk3328-pinctrl";
		rockchip,grf = <&grf>;
		#address-cells = <2>;
		#size-cells = <2>;
		ranges;

		gpio0: gpio0@ff210000 {
			compatible = "rockchip,gpio-bank";
			reg = <0x0 0xff210000 0x0 0x100>;
			interrupts = <GIC_SPI 51 
			IRQ_TYPE_LEVEL_HIGH>;
			clocks = <&cru PCLK_GPIO0>;

			gpio-controller;
			#gpio-cells = <2>;

			interrupt-controller;
			#interrupt-cells = <2>;
		};

		gpio1: gpio1@ff220000 {
                //...
		};

		gpio2: gpio2@ff230000 {
                //...
		};

		gpio3: gpio3@ff240000 {
                //...
		};
         }

However, these general GPIO pins has multiplexed functions and 
their
pull up/down and driving strength can also be configured. These 
settings
are manipulated by the GRF registers in pinctrl driver. Quoted 
from the
TRM, the GRF has the following function:

 - IOMUX control
 - Control the state of GPIO in power-down mode
 - GPIO PAD pull down and pull up control
 - Used for common system control
 - Used to record the system state

Therefore the functions of the GRF are messy and scattered in 
different
nodes. The so-called GPIO_MUTE does not belong to GPIO0~GPIO3. It 
is
manipulated by the GRF_SOC_CON10 register in the GRF block.

> I'm being told both "this is the only GPIO" and "the GRF has too 
> many
> different functions for us to tell you what they all are". So 
> which is
> it?
>
> Rob

They are both true, but lack of context. See the above 
description.

Thanks,
Levin

Re: [PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[Rob Herring]

On Sat, Jun 02, 2018 at 04:40:09PM +0800, Levin Du wrote:
> 
> Rob Herring <robh+dt@kernel.org> writes:
> 
> > On Thu, May 31, 2018 at 9:05 PM, Levin <djw@t-chip.com.cn> wrote:
> > > Hi Rob,
> > > 
> > > 
> > > On 2018-05-31 10:45 PM, Rob Herring wrote:
> > > > 
> > > > On Wed, May 30, 2018 at 10:27 PM,  <djw@t-chip.com.cn> wrote:
> > > > > 
> > > > > From: Levin Du <djw@t-chip.com.cn>
> > > > > 
> > > > > In Rockchip RK3328, the output only GPIO_MUTE pin,
> > > > > originally for codec
> > > > > mute control, can also be used for general purpose. It is
> > > > > manipulated by
> > > > > the GRF_SOC_CON10 register.
> > > > > 
> > > > > Signed-off-by: Levin Du <djw@t-chip.com.cn>
> > > > > 
> > > > > ---
> > > > > 
> > > > > Changes in v3:
> > > > > - Change from general gpio-syscon to specific
> > > > > rk3328-gpio-mute
> > > > > 
> > > > > 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,rk3328-gpio-mute.txt    | 28
> > > > > +++++++++++++++++++
> > > > >   drivers/gpio/gpio-syscon.c                         | 31
> > > > > ++++++++++++++++++++++
> > > > >   2 files changed, 59 insertions(+)
> > > > >   create mode 100644
> > > > > Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> > > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> > > > > new file mode 100644
> > > > > index 0000000..10bc632
> > > > > --- /dev/null
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> > > > > @@ -0,0 +1,28 @@
> > > > > +Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE
> > > > > pin.
> > > > > +
> > > > > +In Rockchip RK3328, the output only GPIO_MUTE pin,
> > > > > originally for codec
> > > > > mute
> > > > > +control, can also be used for general purpose. It is
> > > > > manipulated by the
> > > > > +GRF_SOC_CON10 register.
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible: Should contain "rockchip,rk3328-gpio-mute".
> > > > > +- gpio-controller: Marks the device node as a gpio
> > > > > controller.
> > > > > +- #gpio-cells: Should be 2. The first cell is the pin
> > > > > number and
> > > > > +  the second cell is used to specify the gpio polarity:
> > > > > +    0 = Active high,
> > > > > +    1 = Active low.
> > > > > +
> > > > > +Example:
> > > > > +
> > > > > +       grf: syscon@ff100000 {
> > > > > +               compatible = "rockchip,rk3328-grf", "syscon",
> > > > > "simple-mfd";
> > > > > +
> > > > > +               gpio_mute: gpio-mute {
> > > > 
> > > > Node names should be generic:
> > > > 
> > > > gpio {
> > > > 
> > > > This also means you can't add another GPIO node in the future
> > > > and
> > > > you'll have to live with "rockchip,rk3328-gpio-mute" covering
> > > > more
> > > > than 1 GPIO if you do need to add more GPIOs.
> > > 
> > > 
> > > As the first line describes, this GPIO controller is dedicated for
> > > the
> > > GPIO_MUTE pin.
> > > There's only one GPIO pin in the GRF_SOC_CON10 register. Therefore
> > > the
> > > gpio_mute
> > > name is proper IMHO.
> > 
> > It's how many GPIOs in the GRF, not this register. What I'm saying is
> > when you come along later to add another GPIO in the GRF, you had
> > better just add it to this same node. I'm not going to accept another
> > GPIO controller node within the GRF. You have the cells to support
> > more than 1, so it would only be a driver change. The compatible
> > string would then not be ideally named at that point. But compatible
> > strings are just unique identifiers, so it doesn't really matter what
> > the string is.
> > 
> 
> I'll try my best to introduce the situation here. The GRF, GPIO0~GPIO3
> are register blocks in the RK3328 Soc. The GPIO0~GPIO3 contain registers
> for GPIO operations like reading/writing data, setting direction,
> interruption etc, which corresponds to the GPIO banks (gpio0~gpio3)
> defined in rk3328.dtsi:

I'm only talking about GRF functions, not "regular" GPIOs.

> 	pinctrl: pinctrl {
> 		compatible = "rockchip,rk3328-pinctrl";
> 		rockchip,grf = <&grf>;
> 		#address-cells = <2>;
> 		#size-cells = <2>;
> 		ranges;
> 
> 		gpio0: gpio0@ff210000 {
> 			compatible = "rockchip,gpio-bank";
> 			reg = <0x0 0xff210000 0x0 0x100>;
> 			interrupts = <GIC_SPI 51 			IRQ_TYPE_LEVEL_HIGH>;
> 			clocks = <&cru PCLK_GPIO0>;
> 
> 			gpio-controller;
> 			#gpio-cells = <2>;
> 
> 			interrupt-controller;
> 			#interrupt-cells = <2>;
> 		};
> 
> 		gpio1: gpio1@ff220000 {
>                //...
> 		};
> 
> 		gpio2: gpio2@ff230000 {
>                //...
> 		};
> 
> 		gpio3: gpio3@ff240000 {
>                //...
> 		};
>         }
> 
> However, these general GPIO pins has multiplexed functions and their
> pull up/down and driving strength can also be configured. These settings
> are manipulated by the GRF registers in pinctrl driver. Quoted from the
> TRM, the GRF has the following function:
> 
> - IOMUX control
> - Control the state of GPIO in power-down mode
> - GPIO PAD pull down and pull up control
> - Used for common system control
> - Used to record the system state
> 
> Therefore the functions of the GRF are messy and scattered in different
> nodes. The so-called GPIO_MUTE does not belong to GPIO0~GPIO3. It is
> manipulated by the GRF_SOC_CON10 register in the GRF block.
> 
> > I'm being told both "this is the only GPIO" and "the GRF has too many
> > different functions for us to tell you what they all are". So which is
> > it?
> > 
> > Rob
> 
> They are both true, but lack of context. See the above description.

What I meant was "only GPIO in GRF registers"...

Rob

Re: [PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[Levin]

Rob Herring <robh@kernel.org> writes:

> On Sat, Jun 02, 2018 at 04:40:09PM +0800, Levin Du wrote:
>> 
>> Rob Herring <robh+dt@kernel.org> writes:
>> 
>> > On Thu, May 31, 2018 at 9:05 PM, Levin <djw@t-chip.com.cn> wrote:
>> > > Hi Rob,
>> > > 
>> > > 
>> > > On 2018-05-31 10:45 PM, Rob Herring wrote:
>> > > > 
>> > > > On Wed, May 30, 2018 at 10:27 PM,  <djw@t-chip.com.cn> wrote:
>> > > > > 
>> > > > > From: Levin Du <djw@t-chip.com.cn>
>> > > > > 
>> > > > > In Rockchip RK3328, the output only GPIO_MUTE pin,
>> > > > > originally for codec
>> > > > > mute control, can also be used for general purpose. It is
>> > > > > manipulated by
>> > > > > the GRF_SOC_CON10 register.
>> > > > > 
>> > > > > Signed-off-by: Levin Du <djw@t-chip.com.cn>
>> > > > > 
>> > > > > ---
>> > > > > 
>> > > > > Changes in v3:
>> > > > > - Change from general gpio-syscon to specific
>> > > > > rk3328-gpio-mute
>> > > > > 
>> > > > > 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,rk3328-gpio-mute.txt    | 28
>> > > > > +++++++++++++++++++
>> > > > >   drivers/gpio/gpio-syscon.c                         | 31
>> > > > > ++++++++++++++++++++++
>> > > > >   2 files changed, 59 insertions(+)
>> > > > >   create mode 100644
>> > > > > Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>> > > > > 
>> > > > > diff --git
>> > > > > a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>> > > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>> > > > > new file mode 100644
>> > > > > index 0000000..10bc632
>> > > > > --- /dev/null
>> > > > > +++
>> > > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>> > > > > @@ -0,0 +1,28 @@
>> > > > > +Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE
>> > > > > pin.
>> > > > > +
>> > > > > +In Rockchip RK3328, the output only GPIO_MUTE pin,
>> > > > > originally for codec
>> > > > > mute
>> > > > > +control, can also be used for general purpose. It is
>> > > > > manipulated by the
>> > > > > +GRF_SOC_CON10 register.
>> > > > > +
>> > > > > +Required properties:
>> > > > > +- compatible: Should contain "rockchip,rk3328-gpio-mute".
>> > > > > +- gpio-controller: Marks the device node as a gpio
>> > > > > controller.
>> > > > > +- #gpio-cells: Should be 2. The first cell is the pin
>> > > > > number and
>> > > > > +  the second cell is used to specify the gpio polarity:
>> > > > > +    0 = Active high,
>> > > > > +    1 = Active low.
>> > > > > +
>> > > > > +Example:
>> > > > > +
>> > > > > +       grf: syscon@ff100000 {
>> > > > > +               compatible = "rockchip,rk3328-grf", "syscon",
>> > > > > "simple-mfd";
>> > > > > +
>> > > > > +               gpio_mute: gpio-mute {
>> > > > 
>> > > > Node names should be generic:
>> > > > 
>> > > > gpio {
>> > > > 
>> > > > This also means you can't add another GPIO node in the future
>> > > > and
>> > > > you'll have to live with "rockchip,rk3328-gpio-mute" covering
>> > > > more
>> > > > than 1 GPIO if you do need to add more GPIOs.
>> > > 
>> > > 
>> > > As the first line describes, this GPIO controller is dedicated for
>> > > the
>> > > GPIO_MUTE pin.
>> > > There's only one GPIO pin in the GRF_SOC_CON10 register. Therefore
>> > > the
>> > > gpio_mute
>> > > name is proper IMHO.
>> > 
>> > It's how many GPIOs in the GRF, not this register. What I'm saying is
>> > when you come along later to add another GPIO in the GRF, you had
>> > better just add it to this same node. I'm not going to accept another
>> > GPIO controller node within the GRF. You have the cells to support
>> > more than 1, so it would only be a driver change. The compatible
>> > string would then not be ideally named at that point. But compatible
>> > strings are just unique identifiers, so it doesn't really matter what
>> > the string is.
>> > 
>> 
>> I'll try my best to introduce the situation here. The GRF, GPIO0~GPIO3
>> are register blocks in the RK3328 Soc. The GPIO0~GPIO3 contain registers
>> for GPIO operations like reading/writing data, setting direction,
>> interruption etc, which corresponds to the GPIO banks (gpio0~gpio3)
>> defined in rk3328.dtsi:
>
> I'm only talking about GRF functions, not "regular" GPIOs.
>
>> 	pinctrl: pinctrl {
>> 		compatible = "rockchip,rk3328-pinctrl";
>> 		rockchip,grf = <&grf>;
>> 		#address-cells = <2>;
>> 		#size-cells = <2>;
>> 		ranges;
>> 
>> 		gpio0: gpio0@ff210000 {
>> 			compatible = "rockchip,gpio-bank";
>> 			reg = <0x0 0xff210000 0x0 0x100>;
>> 			interrupts = <GIC_SPI 51 			IRQ_TYPE_LEVEL_HIGH>;
>> 			clocks = <&cru PCLK_GPIO0>;
>> 
>> 			gpio-controller;
>> 			#gpio-cells = <2>;
>> 
>> 			interrupt-controller;
>> 			#interrupt-cells = <2>;
>> 		};
>> 
>> 		gpio1: gpio1@ff220000 {
>>                //...
>> 		};
>> 
>> 		gpio2: gpio2@ff230000 {
>>                //...
>> 		};
>> 
>> 		gpio3: gpio3@ff240000 {
>>                //...
>> 		};
>>         }
>> 
>> However, these general GPIO pins has multiplexed functions and their
>> pull up/down and driving strength can also be configured. These settings
>> are manipulated by the GRF registers in pinctrl driver. Quoted from the
>> TRM, the GRF has the following function:
>> 
>> - IOMUX control
>> - Control the state of GPIO in power-down mode
>> - GPIO PAD pull down and pull up control
>> - Used for common system control
>> - Used to record the system state
>> 
>> Therefore the functions of the GRF are messy and scattered in different
>> nodes. The so-called GPIO_MUTE does not belong to GPIO0~GPIO3. It is
>> manipulated by the GRF_SOC_CON10 register in the GRF block.
>> 
>> > I'm being told both "this is the only GPIO" and "the GRF has too many
>> > different functions for us to tell you what they all are". So which is
>> > it?
>> > 
>> > Rob
>> 
>> They are both true, but lack of context. See the above description.
>
> What I meant was "only GPIO in GRF registers"...
>
> Rob

I check the TRM and schematic once again. In GRF resters, there are also
HDMI GPIOs, which are already covered by the HDMI driver. Aside from
those, MUTE_GPIO is the only GPIO.

Levin

Re: [PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[djw]

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

> Rob Herring <robh@kernel.org> writes:
>
>> On Sat, Jun 02, 2018 at 04:40:09PM +0800, Levin Du wrote:
>>> 
>>> Rob Herring <robh+dt@kernel.org> writes:
>>> 
>>> > On Thu, May 31, 2018 at 9:05 PM, Levin <djw@t-chip.com.cn> wrote:
>>> > > Hi Rob,
>>> > > 
>>> > > 
>>> > > On 2018-05-31 10:45 PM, Rob Herring wrote:
>>> > > > 
>>> > > > On Wed, May 30, 2018 at 10:27 PM,  <djw@t-chip.com.cn> wrote:
>>> > > > > 
>>> > > > > From: Levin Du <djw@t-chip.com.cn>
>>> > > > > 
>>> > > > > In Rockchip RK3328, the output only GPIO_MUTE pin,
>>> > > > > originally for codec
>>> > > > > mute control, can also be used for general purpose. It is
>>> > > > > manipulated by
>>> > > > > the GRF_SOC_CON10 register.
>>> > > > > 
>>> > > > > Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>> > > > > 
>>> > > > > ---
>>> > > > > 
>>> > > > > Changes in v3:
>>> > > > > - Change from general gpio-syscon to specific
>>> > > > > rk3328-gpio-mute
>>> > > > > 
>>> > > > > 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,rk3328-gpio-mute.txt    | 28
>>> > > > > +++++++++++++++++++
>>> > > > >   drivers/gpio/gpio-syscon.c                         | 31
>>> > > > > ++++++++++++++++++++++
>>> > > > >   2 files changed, 59 insertions(+)
>>> > > > >   create mode 100644
>>> > > > > Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>> > > > > 
>>> > > > > diff --git
>>> > > > > a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>> > > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>> > > > > new file mode 100644
>>> > > > > index 0000000..10bc632
>>> > > > > --- /dev/null
>>> > > > > +++
>>> > > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
>>> > > > > @@ -0,0 +1,28 @@
>>> > > > > +Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE
>>> > > > > pin.
>>> > > > > +
>>> > > > > +In Rockchip RK3328, the output only GPIO_MUTE pin,
>>> > > > > originally for codec
>>> > > > > mute
>>> > > > > +control, can also be used for general purpose. It is
>>> > > > > manipulated by the
>>> > > > > +GRF_SOC_CON10 register.
>>> > > > > +
>>> > > > > +Required properties:
>>> > > > > +- compatible: Should contain "rockchip,rk3328-gpio-mute".
>>> > > > > +- gpio-controller: Marks the device node as a gpio
>>> > > > > controller.
>>> > > > > +- #gpio-cells: Should be 2. The first cell is the pin
>>> > > > > number and
>>> > > > > +  the second cell is used to specify the gpio polarity:
>>> > > > > +    0 = Active high,
>>> > > > > +    1 = Active low.
>>> > > > > +
>>> > > > > +Example:
>>> > > > > +
>>> > > > > +       grf: syscon@ff100000 {
>>> > > > > +               compatible = "rockchip,rk3328-grf", "syscon",
>>> > > > > "simple-mfd";
>>> > > > > +
>>> > > > > +               gpio_mute: gpio-mute {
>>> > > > 
>>> > > > Node names should be generic:
>>> > > > 
>>> > > > gpio {
>>> > > > 
>>> > > > This also means you can't add another GPIO node in the future
>>> > > > and
>>> > > > you'll have to live with "rockchip,rk3328-gpio-mute" covering
>>> > > > more
>>> > > > than 1 GPIO if you do need to add more GPIOs.
>>> > > 
>>> > > 
>>> > > As the first line describes, this GPIO controller is dedicated for
>>> > > the
>>> > > GPIO_MUTE pin.
>>> > > There's only one GPIO pin in the GRF_SOC_CON10 register. Therefore
>>> > > the
>>> > > gpio_mute
>>> > > name is proper IMHO.
>>> > 
>>> > It's how many GPIOs in the GRF, not this register. What I'm saying is
>>> > when you come along later to add another GPIO in the GRF, you had
>>> > better just add it to this same node. I'm not going to accept another
>>> > GPIO controller node within the GRF. You have the cells to support
>>> > more than 1, so it would only be a driver change. The compatible
>>> > string would then not be ideally named at that point. But compatible
>>> > strings are just unique identifiers, so it doesn't really matter what
>>> > the string is.
>>> > 
>>> 
>>> I'll try my best to introduce the situation here. The GRF, GPIO0~GPIO3
>>> are register blocks in the RK3328 Soc. The GPIO0~GPIO3 contain registers
>>> for GPIO operations like reading/writing data, setting direction,
>>> interruption etc, which corresponds to the GPIO banks (gpio0~gpio3)
>>> defined in rk3328.dtsi:
>>
>> I'm only talking about GRF functions, not "regular" GPIOs.
>>
>>> 	pinctrl: pinctrl {
>>> 		compatible = "rockchip,rk3328-pinctrl";
>>> 		rockchip,grf = <&grf>;
>>> 		#address-cells = <2>;
>>> 		#size-cells = <2>;
>>> 		ranges;
>>> 
>>> 		gpio0: gpio0@ff210000 {
>>> 			compatible = "rockchip,gpio-bank";
>>> 			reg = <0x0 0xff210000 0x0 0x100>;
>>> 			interrupts = <GIC_SPI 51 			IRQ_TYPE_LEVEL_HIGH>;
>>> 			clocks = <&cru PCLK_GPIO0>;
>>> 
>>> 			gpio-controller;
>>> 			#gpio-cells = <2>;
>>> 
>>> 			interrupt-controller;
>>> 			#interrupt-cells = <2>;
>>> 		};
>>> 
>>> 		gpio1: gpio1@ff220000 {
>>>                //...
>>> 		};
>>> 
>>> 		gpio2: gpio2@ff230000 {
>>>                //...
>>> 		};
>>> 
>>> 		gpio3: gpio3@ff240000 {
>>>                //...
>>> 		};
>>>         }
>>> 
>>> However, these general GPIO pins has multiplexed functions and their
>>> pull up/down and driving strength can also be configured. These settings
>>> are manipulated by the GRF registers in pinctrl driver. Quoted from the
>>> TRM, the GRF has the following function:
>>> 
>>> - IOMUX control
>>> - Control the state of GPIO in power-down mode
>>> - GPIO PAD pull down and pull up control
>>> - Used for common system control
>>> - Used to record the system state
>>> 
>>> Therefore the functions of the GRF are messy and scattered in different
>>> nodes. The so-called GPIO_MUTE does not belong to GPIO0~GPIO3. It is
>>> manipulated by the GRF_SOC_CON10 register in the GRF block.
>>> 
>>> > I'm being told both "this is the only GPIO" and "the GRF has too many
>>> > different functions for us to tell you what they all are". So which is
>>> > it?
>>> > 
>>> > Rob
>>> 
>>> They are both true, but lack of context. See the above description.
>>
>> What I meant was "only GPIO in GRF registers"...
>>
>> Rob
>
> I check the TRM and schematic once again. In GRF resters, there are also
> HDMI GPIOs, which are already covered by the HDMI driver. Aside from
> those, MUTE_GPIO is the only GPIO.
>
> Levin

Hi Rob,

Is there anything I can do to move forward? I know that this patch is
far from a perfect solution. But it can be refactored later on.

Best Regards,
Levin

Re: [PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[Rob Herring]

On Thu, Jun 28, 2018 at 1:22 AM
<djw@archiso.i-did-not-set--mail-host-address--so-tickle-me> wrote:
>
> Levin <djw@t-chip.com.cn> writes:
>
> > Rob Herring <robh@kernel.org> writes:
> >
> >> On Sat, Jun 02, 2018 at 04:40:09PM +0800, Levin Du wrote:
> >>>
> >>> Rob Herring <robh+dt@kernel.org> writes:
> >>>
> >>> > On Thu, May 31, 2018 at 9:05 PM, Levin <djw@t-chip.com.cn> wrote:
> >>> > > Hi Rob,
> >>> > >
> >>> > >
> >>> > > On 2018-05-31 10:45 PM, Rob Herring wrote:
> >>> > > >
> >>> > > > On Wed, May 30, 2018 at 10:27 PM,  <djw@t-chip.com.cn> wrote:
> >>> > > > >
> >>> > > > > From: Levin Du <djw@t-chip.com.cn>
> >>> > > > >
> >>> > > > > In Rockchip RK3328, the output only GPIO_MUTE pin,
> >>> > > > > originally for codec
> >>> > > > > mute control, can also be used for general purpose. It is
> >>> > > > > manipulated by
> >>> > > > > the GRF_SOC_CON10 register.
> >>> > > > >
> >>> > > > > Signed-off-by: Levin Du <djw@t-chip.com.cn>
> >>> > > > >
> >>> > > > > ---
> >>> > > > >
> >>> > > > > Changes in v3:
> >>> > > > > - Change from general gpio-syscon to specific
> >>> > > > > rk3328-gpio-mute
> >>> > > > >
> >>> > > > > 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,rk3328-gpio-mute.txt    | 28
> >>> > > > > +++++++++++++++++++
> >>> > > > >   drivers/gpio/gpio-syscon.c                         | 31
> >>> > > > > ++++++++++++++++++++++
> >>> > > > >   2 files changed, 59 insertions(+)
> >>> > > > >   create mode 100644
> >>> > > > > Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> >>> > > > >
> >>> > > > > diff --git
> >>> > > > > a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> >>> > > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> >>> > > > > new file mode 100644
> >>> > > > > index 0000000..10bc632
> >>> > > > > --- /dev/null
> >>> > > > > +++
> >>> > > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> >>> > > > > @@ -0,0 +1,28 @@
> >>> > > > > +Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE
> >>> > > > > pin.
> >>> > > > > +
> >>> > > > > +In Rockchip RK3328, the output only GPIO_MUTE pin,
> >>> > > > > originally for codec
> >>> > > > > mute
> >>> > > > > +control, can also be used for general purpose. It is
> >>> > > > > manipulated by the
> >>> > > > > +GRF_SOC_CON10 register.
> >>> > > > > +
> >>> > > > > +Required properties:
> >>> > > > > +- compatible: Should contain "rockchip,rk3328-gpio-mute".
> >>> > > > > +- gpio-controller: Marks the device node as a gpio
> >>> > > > > controller.
> >>> > > > > +- #gpio-cells: Should be 2. The first cell is the pin
> >>> > > > > number and
> >>> > > > > +  the second cell is used to specify the gpio polarity:
> >>> > > > > +    0 = Active high,
> >>> > > > > +    1 = Active low.
> >>> > > > > +
> >>> > > > > +Example:
> >>> > > > > +
> >>> > > > > +       grf: syscon@ff100000 {
> >>> > > > > +               compatible = "rockchip,rk3328-grf", "syscon",
> >>> > > > > "simple-mfd";
> >>> > > > > +
> >>> > > > > +               gpio_mute: gpio-mute {
> >>> > > >
> >>> > > > Node names should be generic:
> >>> > > >
> >>> > > > gpio {
> >>> > > >
> >>> > > > This also means you can't add another GPIO node in the future
> >>> > > > and
> >>> > > > you'll have to live with "rockchip,rk3328-gpio-mute" covering
> >>> > > > more
> >>> > > > than 1 GPIO if you do need to add more GPIOs.
> >>> > >
> >>> > >
> >>> > > As the first line describes, this GPIO controller is dedicated for
> >>> > > the
> >>> > > GPIO_MUTE pin.
> >>> > > There's only one GPIO pin in the GRF_SOC_CON10 register. Therefore
> >>> > > the
> >>> > > gpio_mute
> >>> > > name is proper IMHO.
> >>> >
> >>> > It's how many GPIOs in the GRF, not this register. What I'm saying is
> >>> > when you come along later to add another GPIO in the GRF, you had
> >>> > better just add it to this same node. I'm not going to accept another
> >>> > GPIO controller node within the GRF. You have the cells to support
> >>> > more than 1, so it would only be a driver change. The compatible
> >>> > string would then not be ideally named at that point. But compatible
> >>> > strings are just unique identifiers, so it doesn't really matter what
> >>> > the string is.
> >>> >
> >>>
> >>> I'll try my best to introduce the situation here. The GRF, GPIO0~GPIO3
> >>> are register blocks in the RK3328 Soc. The GPIO0~GPIO3 contain registers
> >>> for GPIO operations like reading/writing data, setting direction,
> >>> interruption etc, which corresponds to the GPIO banks (gpio0~gpio3)
> >>> defined in rk3328.dtsi:
> >>
> >> I'm only talking about GRF functions, not "regular" GPIOs.
> >>
> >>>     pinctrl: pinctrl {
> >>>             compatible = "rockchip,rk3328-pinctrl";
> >>>             rockchip,grf = <&grf>;
> >>>             #address-cells = <2>;
> >>>             #size-cells = <2>;
> >>>             ranges;
> >>>
> >>>             gpio0: gpio0@ff210000 {
> >>>                     compatible = "rockchip,gpio-bank";
> >>>                     reg = <0x0 0xff210000 0x0 0x100>;
> >>>                     interrupts = <GIC_SPI 51                        IRQ_TYPE_LEVEL_HIGH>;
> >>>                     clocks = <&cru PCLK_GPIO0>;
> >>>
> >>>                     gpio-controller;
> >>>                     #gpio-cells = <2>;
> >>>
> >>>                     interrupt-controller;
> >>>                     #interrupt-cells = <2>;
> >>>             };
> >>>
> >>>             gpio1: gpio1@ff220000 {
> >>>                //...
> >>>             };
> >>>
> >>>             gpio2: gpio2@ff230000 {
> >>>                //...
> >>>             };
> >>>
> >>>             gpio3: gpio3@ff240000 {
> >>>                //...
> >>>             };
> >>>         }
> >>>
> >>> However, these general GPIO pins has multiplexed functions and their
> >>> pull up/down and driving strength can also be configured. These settings
> >>> are manipulated by the GRF registers in pinctrl driver. Quoted from the
> >>> TRM, the GRF has the following function:
> >>>
> >>> - IOMUX control
> >>> - Control the state of GPIO in power-down mode
> >>> - GPIO PAD pull down and pull up control
> >>> - Used for common system control
> >>> - Used to record the system state
> >>>
> >>> Therefore the functions of the GRF are messy and scattered in different
> >>> nodes. The so-called GPIO_MUTE does not belong to GPIO0~GPIO3. It is
> >>> manipulated by the GRF_SOC_CON10 register in the GRF block.
> >>>
> >>> > I'm being told both "this is the only GPIO" and "the GRF has too many
> >>> > different functions for us to tell you what they all are". So which is
> >>> > it?
> >>> >
> >>> > Rob
> >>>
> >>> They are both true, but lack of context. See the above description.
> >>
> >> What I meant was "only GPIO in GRF registers"...
> >>
> >> Rob
> >
> > I check the TRM and schematic once again. In GRF resters, there are also
> > HDMI GPIOs, which are already covered by the HDMI driver. Aside from
> > those, MUTE_GPIO is the only GPIO.

So if some board uses the HDMI GPIOs for some other function, then you
would need to model them as GPIOs. Presumably there's just a syscon
phandle and the HDMI driver touches those registers directly which is
not ideal.

> > Levin
>
> Hi Rob,
>
> Is there anything I can do to move forward? I know that this patch is
> far from a perfect solution. But it can be refactored later on.

But you can't refactor bindings later on. Extend yes, refactor no.
That's what I'm trying to make sure you avoid.

So if you don't want to be stuck with the "mute" name later on, call
this "...-grf-gpio" to make it future proof.

[PATCH v3 3/5] arm64: dts: rockchip: Add GPIO_MUTE pin support to rk3328

[djw]

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, which is manupulated by the GRF_SOC_CON10
register.

The GPIO_MUTE pin is referred to as <&gpio_mute 0>.

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

---

Changes in v3:
- Use dedicated "rockchip,rk3328-gpio-mute" driver

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 be2bfbc..2ee0fa3 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>;
 		};
 
+		/* Use <&gpio_mute 0> to refer to GPIO_MUTE pin */
+		gpio_mute: gpio-mute {
+			compatible = "rockchip,rk3328-gpio-mute";
+			gpio-controller;
+			#gpio-cells = <2>;
+			status = "disabled";
+		};
 	};
 
 	uart0: serial@ff110000 {
-- 
2.7.4

[PATCH v3 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 v3: None
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 v3 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 0>, 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 v3:
- Use <&gpio_mute 0> instead of <&gpio_mute 1> to refer to the GPIO_MUTE pin.

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 | 24 +++++++++++++++++++++++-
 1 file changed, 23 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..25c1111 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 0 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";
 };
 
@@ -277,3 +295,7 @@
 &usb_host0_ohci {
 	status = "okay";
 };
+
+&gpio_mute {
+	status = "okay";
+};
-- 
2.7.4