[PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Johan Jonker]

Sync rk3066/rk3188 DT files from Linux.
This is the state as of linux-next v6.2-rc4.
New nfc node for MK808 rk3066a.
CRU nodes now have a clock property.
To prefend dtoc errors a fixed clock must also be
included for tpl/spl in the rk3xxx-u-boot.dtsi file.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 arch/arm/dts/rk3066a-mk808.dts    | 27 ++++++++++++++++++++++++++-
 arch/arm/dts/rk3066a.dtsi         |  3 ++-
 arch/arm/dts/rk3188-radxarock.dts | 24 +++++++++++++-----------
 arch/arm/dts/rk3188.dtsi          | 27 ++++++++++++++++-----------
 arch/arm/dts/rk3xxx-u-boot.dtsi   |  4 ++++
 arch/arm/dts/rk3xxx.dtsi          |  9 +++++++--
 6 files changed, 68 insertions(+), 26 deletions(-)

diff --git a/arch/arm/dts/rk3066a-mk808.dts b/arch/arm/dts/rk3066a-mk808.dts
index 667d57a4..06790f05 100644
--- a/arch/arm/dts/rk3066a-mk808.dts
+++ b/arch/arm/dts/rk3066a-mk808.dts
@@ -32,7 +32,7 @@
 		keyup-threshold-microvolt = <2500000>;
 		poll-interval = <100>;

-		recovery {
+		button-recovery {
 			label = "recovery";
 			linux,code = <KEY_VENDOR>;
 			press-threshold-microvolt = <0>;
@@ -157,7 +157,32 @@
 	pinctrl-0 = <&sd1_clk &sd1_cmd &sd1_bus4>;
 	pinctrl-names = "default";
 	vmmc-supply = <&vcc_wifi>;
+	#address-cells = <1>;
+	#size-cells = <0>;
 	status = "okay";
+
+	brcmf: wifi@1 {
+		compatible = "brcm,bcm4329-fmac";
+		reg = <1>;
+	};
+};
+
+&nfc {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	status = "okay";
+
+	nand@0 {
+		reg = <0>;
+		label = "rk-nand";
+		nand-bus-width = <8>;
+		nand-ecc-mode = "hw";
+		nand-ecc-step-size = <1024>;
+		nand-ecc-strength = <40>;
+		nand-is-boot-medium;
+		rockchip,boot-blks = <8>;
+		rockchip,boot-ecc-strength = <24>;
+	};
 };

 &pinctrl {
diff --git a/arch/arm/dts/rk3066a.dtsi b/arch/arm/dts/rk3066a.dtsi
index c25b9695..de9915d9 100644
--- a/arch/arm/dts/rk3066a.dtsi
+++ b/arch/arm/dts/rk3066a.dtsi
@@ -202,8 +202,9 @@
 	cru: clock-controller@20000000 {
 		compatible = "rockchip,rk3066a-cru";
 		reg = <0x20000000 0x1000>;
+		clocks = <&xin24m>;
+		clock-names = "xin24m";
 		rockchip,grf = <&grf>;
-
 		#clock-cells = <1>;
 		#reset-cells = <1>;
 		assigned-clocks = <&cru PLL_CPLL>, <&cru PLL_GPLL>,
diff --git a/arch/arm/dts/rk3188-radxarock.dts b/arch/arm/dts/rk3188-radxarock.dts
index e7138a4a..118deacd 100644
--- a/arch/arm/dts/rk3188-radxarock.dts
+++ b/arch/arm/dts/rk3188-radxarock.dts
@@ -6,7 +6,6 @@
 /dts-v1/;
 #include <dt-bindings/input/input.h>
 #include "rk3188.dtsi"
-#include "rk3188-radxarock-u-boot.dtsi"

 / {
 	model = "Radxa Rock";
@@ -25,7 +24,7 @@
 		compatible = "gpio-keys";
 		autorepeat;

-		power {
+		key-power {
 			gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
 			linux,code = <KEY_POWER>;
 			label = "GPIO Key Power";
@@ -72,7 +71,7 @@
 		#sound-dai-cells = <0>;
 	};

-	ir_recv: gpio-ir-receiver {
+	ir_recv: ir-receiver {
 		compatible = "gpio-ir-receiver";
 		gpios = <&gpio0 RK_PB2 GPIO_ACTIVE_LOW>;
 		pinctrl-names = "default";
@@ -127,18 +126,21 @@
 };

 &emac {
-	status = "okay";
-
+	phy = <&phy0>;
+	phy-supply = <&vcc_rmii>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&emac_xfer>, <&emac_mdio>, <&phy_int>;
+	status = "okay";

-	phy = <&phy0>;
-	phy-supply = <&vcc_rmii>;
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;

-	phy0: ethernet-phy@0 {
-		reg = <0>;
-		interrupt-parent = <&gpio3>;
-		interrupts = <RK_PD2 IRQ_TYPE_LEVEL_LOW>;
+		phy0: ethernet-phy@0 {
+			reg = <0>;
+			interrupt-parent = <&gpio3>;
+			interrupts = <RK_PD2 IRQ_TYPE_LEVEL_LOW>;
+		};
 	};
 };

diff --git a/arch/arm/dts/rk3188.dtsi b/arch/arm/dts/rk3188.dtsi
index 9a80f83a..44b54af0 100644
--- a/arch/arm/dts/rk3188.dtsi
+++ b/arch/arm/dts/rk3188.dtsi
@@ -54,7 +54,7 @@
 		};
 	};

-	cpu0_opp_table: opp_table0 {
+	cpu0_opp_table: opp-table-0 {
 		compatible = "operating-points-v2";
 		opp-shared;

@@ -195,8 +195,9 @@
 	cru: clock-controller@20000000 {
 		compatible = "rockchip,rk3188-cru";
 		reg = <0x20000000 0x1000>;
+		clocks = <&xin24m>;
+		clock-names = "xin24m";
 		rockchip,grf = <&grf>;
-
 		#clock-cells = <1>;
 		#reset-cells = <1>;
 	};
@@ -223,7 +224,7 @@
 		#size-cells = <1>;
 		ranges;

-		gpio0: gpio0@2000a000 {
+		gpio0: gpio@2000a000 {
 			compatible = "rockchip,rk3188-gpio-bank0";
 			reg = <0x2000a000 0x100>;
 			interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
@@ -236,7 +237,7 @@
 			#interrupt-cells = <2>;
 		};

-		gpio1: gpio1@2003c000 {
+		gpio1: gpio@2003c000 {
 			compatible = "rockchip,gpio-bank";
 			reg = <0x2003c000 0x100>;
 			interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
@@ -249,7 +250,7 @@
 			#interrupt-cells = <2>;
 		};

-		gpio2: gpio2@2003e000 {
+		gpio2: gpio@2003e000 {
 			compatible = "rockchip,gpio-bank";
 			reg = <0x2003e000 0x100>;
 			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
@@ -262,7 +263,7 @@
 			#interrupt-cells = <2>;
 		};

-		gpio3: gpio3@20080000 {
+		gpio3: gpio@20080000 {
 			compatible = "rockchip,gpio-bank";
 			reg = <0x20080000 0x100>;
 			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
@@ -275,15 +276,15 @@
 			#interrupt-cells = <2>;
 		};

-		pcfg_pull_up: pcfg_pull_up {
+		pcfg_pull_up: pcfg-pull-up {
 			bias-pull-up;
 		};

-		pcfg_pull_down: pcfg_pull_down {
+		pcfg_pull_down: pcfg-pull-down {
 			bias-pull-down;
 		};

-		pcfg_pull_none: pcfg_pull_none {
+		pcfg_pull_none: pcfg-pull-none {
 			bias-disable;
 		};

@@ -378,7 +379,7 @@
 				rockchip,pins = <2 RK_PD3 1 &pcfg_pull_none>;
 			};

-			lcdc1_rgb24: ldcd1-rgb24 {
+			lcdc1_rgb24: lcdc1-rgb24 {
 				rockchip,pins = <2 RK_PA0 1 &pcfg_pull_none>,
 						<2 RK_PA1 1 &pcfg_pull_none>,
 						<2 RK_PA2 1 &pcfg_pull_none>,
@@ -606,7 +607,6 @@

 &global_timer {
 	interrupts = <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>;
-	status = "disabled";
 };

 &local_timer {
@@ -641,6 +641,11 @@
 &grf {
 	compatible = "rockchip,rk3188-grf", "syscon", "simple-mfd";

+	io_domains: io-domains {
+		compatible = "rockchip,rk3188-io-voltage-domain";
+		status = "disabled";
+	};
+
 	usbphy: usbphy {
 		compatible = "rockchip,rk3188-usb-phy";
 		#address-cells = <1>;
diff --git a/arch/arm/dts/rk3xxx-u-boot.dtsi b/arch/arm/dts/rk3xxx-u-boot.dtsi
index e67432fb..c77d1fae 100644
--- a/arch/arm/dts/rk3xxx-u-boot.dtsi
+++ b/arch/arm/dts/rk3xxx-u-boot.dtsi
@@ -33,3 +33,7 @@
 &uart2 {
 	clock-frequency = <24000000>;
 };
+
+&xin24m {
+	u-boot,dm-pre-reloc;
+};
diff --git a/arch/arm/dts/rk3xxx.dtsi b/arch/arm/dts/rk3xxx.dtsi
index 616a828e..cb4e42ed 100644
--- a/arch/arm/dts/rk3xxx.dtsi
+++ b/arch/arm/dts/rk3xxx.dtsi
@@ -76,6 +76,13 @@
 		reg = <0x1013c200 0x20>;
 		interrupts = <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_EDGE_RISING)>;
 		clocks = <&cru CORE_PERI>;
+		status = "disabled";
+		/* The clock source and the sched_clock provided by the arm_global_timer
+		 * on Rockchip rk3066a/rk3188 are quite unstable because their rates
+		 * depend on the CPU frequency.
+		 * Keep the arm_global_timer disabled in order to have the
+		 * DW_APB_TIMER (rk3066a) or ROCKCHIP_TIMER (rk3188) selected by default.
+		 */
 	};

 	local_timer: local-timer@1013c600 {
@@ -186,8 +193,6 @@
 		compatible = "snps,arc-emac";
 		reg = <0x10204000 0x3c>;
 		interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
-		#address-cells = <1>;
-		#size-cells = <0>;

 		rockchip,grf = <&grf>;

--
2.20.1

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Quentin Schulz]

Hi Johan,

On 1/16/23 20:45, Johan Jonker wrote:
> Sync rk3066/rk3188 DT files from Linux.
> This is the state as of linux-next v6.2-rc4.
> New nfc node for MK808 rk3066a.
> CRU nodes now have a clock property.
> To prefend dtoc errors a fixed clock must also be
> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>   arch/arm/dts/rk3066a-mk808.dts    | 27 ++++++++++++++++++++++++++-
>   arch/arm/dts/rk3066a.dtsi         |  3 ++-
>   arch/arm/dts/rk3188-radxarock.dts | 24 +++++++++++++-----------
>   arch/arm/dts/rk3188.dtsi          | 27 ++++++++++++++++-----------
>   arch/arm/dts/rk3xxx-u-boot.dtsi   |  4 ++++
>   arch/arm/dts/rk3xxx.dtsi          |  9 +++++++--
>   6 files changed, 68 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/dts/rk3066a-mk808.dts b/arch/arm/dts/rk3066a-mk808.dts
> index 667d57a4..06790f05 100644
> --- a/arch/arm/dts/rk3066a-mk808.dts
> +++ b/arch/arm/dts/rk3066a-mk808.dts
> @@ -32,7 +32,7 @@
>   		keyup-threshold-microvolt = <2500000>;
>   		poll-interval = <100>;
> 
> -		recovery {
> +		button-recovery {
>   			label = "recovery";
>   			linux,code = <KEY_VENDOR>;
>   			press-threshold-microvolt = <0>;
> @@ -157,7 +157,32 @@
>   	pinctrl-0 = <&sd1_clk &sd1_cmd &sd1_bus4>;
>   	pinctrl-names = "default";
>   	vmmc-supply = <&vcc_wifi>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
>   	status = "okay";
> +
> +	brcmf: wifi@1 {
> +		compatible = "brcm,bcm4329-fmac";
> +		reg = <1>;
> +	};
> +};
> +
> +&nfc {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	status = "okay";
> +
> +	nand@0 {
> +		reg = <0>;
> +		label = "rk-nand";
> +		nand-bus-width = <8>;
> +		nand-ecc-mode = "hw";
> +		nand-ecc-step-size = <1024>;
> +		nand-ecc-strength = <40>;
> +		nand-is-boot-medium;
> +		rockchip,boot-blks = <8>;
> +		rockchip,boot-ecc-strength = <24>;
> +	};
>   };
> 
>   &pinctrl {
> diff --git a/arch/arm/dts/rk3066a.dtsi b/arch/arm/dts/rk3066a.dtsi
> index c25b9695..de9915d9 100644
> --- a/arch/arm/dts/rk3066a.dtsi
> +++ b/arch/arm/dts/rk3066a.dtsi
> @@ -202,8 +202,9 @@
>   	cru: clock-controller@20000000 {
>   		compatible = "rockchip,rk3066a-cru";
>   		reg = <0x20000000 0x1000>;
> +		clocks = <&xin24m>;
> +		clock-names = "xin24m";
>   		rockchip,grf = <&grf>;
> -
>   		#clock-cells = <1>;
>   		#reset-cells = <1>;
>   		assigned-clocks = <&cru PLL_CPLL>, <&cru PLL_GPLL>,
> diff --git a/arch/arm/dts/rk3188-radxarock.dts b/arch/arm/dts/rk3188-radxarock.dts
> index e7138a4a..118deacd 100644
> --- a/arch/arm/dts/rk3188-radxarock.dts
> +++ b/arch/arm/dts/rk3188-radxarock.dts
> @@ -6,7 +6,6 @@
>   /dts-v1/;
>   #include <dt-bindings/input/input.h>
>   #include "rk3188.dtsi"
> -#include "rk3188-radxarock-u-boot.dtsi"
> 
>   / {
>   	model = "Radxa Rock";
> @@ -25,7 +24,7 @@
>   		compatible = "gpio-keys";
>   		autorepeat;
> 
> -		power {
> +		key-power {
>   			gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>   			linux,code = <KEY_POWER>;
>   			label = "GPIO Key Power";
> @@ -72,7 +71,7 @@
>   		#sound-dai-cells = <0>;
>   	};
> 
> -	ir_recv: gpio-ir-receiver {
> +	ir_recv: ir-receiver {
>   		compatible = "gpio-ir-receiver";
>   		gpios = <&gpio0 RK_PB2 GPIO_ACTIVE_LOW>;
>   		pinctrl-names = "default";
> @@ -127,18 +126,21 @@
>   };
> 
>   &emac {
> -	status = "okay";
> -
> +	phy = <&phy0>;
> +	phy-supply = <&vcc_rmii>;
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&emac_xfer>, <&emac_mdio>, <&phy_int>;
> +	status = "okay";
> 
> -	phy = <&phy0>;
> -	phy-supply = <&vcc_rmii>;
> +	mdio {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> 
> -	phy0: ethernet-phy@0 {
> -		reg = <0>;
> -		interrupt-parent = <&gpio3>;
> -		interrupts = <RK_PD2 IRQ_TYPE_LEVEL_LOW>;
> +		phy0: ethernet-phy@0 {
> +			reg = <0>;
> +			interrupt-parent = <&gpio3>;
> +			interrupts = <RK_PD2 IRQ_TYPE_LEVEL_LOW>;
> +		};
>   	};
>   };
> 
> diff --git a/arch/arm/dts/rk3188.dtsi b/arch/arm/dts/rk3188.dtsi
> index 9a80f83a..44b54af0 100644
> --- a/arch/arm/dts/rk3188.dtsi
> +++ b/arch/arm/dts/rk3188.dtsi
> @@ -54,7 +54,7 @@
>   		};
>   	};
> 
> -	cpu0_opp_table: opp_table0 {
> +	cpu0_opp_table: opp-table-0 {
>   		compatible = "operating-points-v2";
>   		opp-shared;
> 
> @@ -195,8 +195,9 @@
>   	cru: clock-controller@20000000 {
>   		compatible = "rockchip,rk3188-cru";
>   		reg = <0x20000000 0x1000>;
> +		clocks = <&xin24m>;
> +		clock-names = "xin24m";
>   		rockchip,grf = <&grf>;
> -
>   		#clock-cells = <1>;
>   		#reset-cells = <1>;
>   	};
> @@ -223,7 +224,7 @@
>   		#size-cells = <1>;
>   		ranges;
> 
> -		gpio0: gpio0@2000a000 {
> +		gpio0: gpio@2000a000 {
>   			compatible = "rockchip,rk3188-gpio-bank0";
>   			reg = <0x2000a000 0x100>;
>   			interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> @@ -236,7 +237,7 @@
>   			#interrupt-cells = <2>;
>   		};
> 
> -		gpio1: gpio1@2003c000 {
> +		gpio1: gpio@2003c000 {

I believe this is the same issue as on RK3399 and PX30: this breaks our 
GPIO controller driver.

c.f. 
https://lore.kernel.org/u-boot/0ab9a600-b517-0ce5-d189-99fc8eddfa60@theobroma-systems.com/ 
and its answers.

Cheers,
Quentin

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Johan Jonker]

On 1/17/23 10:46, Quentin Schulz wrote:
> Hi Johan,
> 
> On 1/16/23 20:45, Johan Jonker wrote:
>> Sync rk3066/rk3188 DT files from Linux.
>> This is the state as of linux-next v6.2-rc4.
>> New nfc node for MK808 rk3066a.
>> CRU nodes now have a clock property.
>> To prefend dtoc errors a fixed clock must also be
>> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---

[..]

>> @@ -223,7 +224,7 @@
>>           #size-cells = <1>;
>>           ranges;
>>
>> -        gpio0: gpio0@2000a000 {
>> +        gpio0: gpio@2000a000 {
>>               compatible = "rockchip,rk3188-gpio-bank0";
>>               reg = <0x2000a000 0x100>;
>>               interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>> @@ -236,7 +237,7 @@
>>               #interrupt-cells = <2>;
>>           };
>>
>> -        gpio1: gpio1@2003c000 {

>> +        gpio1: gpio@2003c000 {

Hi,

LOL: I made that binding change on request from Linux DT maintainers.
Node names should generic.

===

My full u-boot is able to boot a Linux kernel for rk3066a.
Only when I give the command below it crashes:

gpio status -a

Could you confirm what other parts are effected?

If it's boots then it's good enough for me and move forward, so please merge.(Kever)

Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!

===

Using DT path or node name is wrong.

Comment by robh+dt:
/sys/bus/platform/devices/ paths are not an ABI. I'll consider
nodenames an ABI if a change is noticed, but not for sysfs path.

https://lore.kernel.org/linux-rockchip/CAL_JsqJGx_MwX9ynMiCUm_xP3t6xWbrtiUi-F+2ceXqZ2Su5tA@mail.gmail.com/

Make use of the properties and a compatible in a node instead.

===
A little test.
From: rockchip_gpio_probe():

int main() {
	//char dev_name[] = "gpio1@2003c000";
	char dev_name[] = "gpio@2003c000";
	char priv_name[2];
	int priv_bank;
	char *end;

	end = strrchr(dev_name, '@');
	priv_bank = trailing_strtoln(dev_name, end);

	priv_name[0] = 'A' + priv_bank;
	priv_name[1] = 0;

	printf("priv_bank: %d\n", priv_bank);
	printf("priv_name: %s\n", priv_name);
	return 0;
}

// priv_bank: 1
// priv_name: B

A change of node name gives:

// priv_bank: -1
// priv_name: @

===

Linux driver gpio-rockchip.c has an alias or else an increment id.

id = of_alias_get_id(np, "gpio");
	if (id < 0)
		id = gpio++;

Problem:
Probe order is not guarantied.
Aliases should go to board files and not in dtsi files anymore. 

===

The current compatible string is generic, but these strings should normally be SoC orientated.

Proposal:

replace:
compatible = "rockchip,gpio-bank";

by:
compatible = "rockchip,rk3188-gpio-bank";

With that known we can lookup the reg address in a lookup table and it's name for u-boot.

struct lookup_table rk_gpio_rk3188_data[] = {
	{0x2000a000, "A"},
	{0x2003c000, "B"},
	{0x2003e000, "C"},
	{0x20080000, "D"},
};


	{ .compatible = "rockchip,rk3188-gpio-bank", .data = &rk_gpio_rk3188_data },

===

If needed I can help with changes to rockchip,gpio-bank.yaml

Let me know if it's useful or that you have an other solution as long as we get forward here.

Johan

> 

> I believe this is the same issue as on RK3399 and PX30: this breaks our GPIO controller driver.

> 
> c.f. https://lore.kernel.org/u-boot/0ab9a600-b517-0ce5-d189-99fc8eddfa60@theobroma-systems.com/ and its answers.
> 
> Cheers,
> Quentin

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Johan Jonker]

On 1/17/23 10:46, Quentin Schulz wrote:
> Hi Johan,
> 
> On 1/16/23 20:45, Johan Jonker wrote:
>> Sync rk3066/rk3188 DT files from Linux.
>> This is the state as of linux-next v6.2-rc4.
>> New nfc node for MK808 rk3066a.
>> CRU nodes now have a clock property.
>> To prefend dtoc errors a fixed clock must also be
>> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---

[..]

>> @@ -223,7 +224,7 @@
>>           #size-cells = <1>;
>>           ranges;
>>
>> -        gpio0: gpio0@2000a000 {
>> +        gpio0: gpio@2000a000 {
>>               compatible = "rockchip,rk3188-gpio-bank0";
>>               reg = <0x2000a000 0x100>;
>>               interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>> @@ -236,7 +237,7 @@
>>               #interrupt-cells = <2>;
>>           };
>>
>> -        gpio1: gpio1@2003c000 {

>> +        gpio1: gpio@2003c000 {

Hi,

LOL: I made that binding change on request from Linux DT maintainers.
Node names should generic.

===

My full u-boot is able to boot a Linux kernel for rk3066a.
Only when I give the command below it crashes:

gpio status -a

Could you confirm what other parts are effected?

If it's boots then it's good enough for me and move forward, so please merge.(Kever)

Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!

===

Using DT path or node name is wrong.

Comment by robh+dt:
/sys/bus/platform/devices/ paths are not an ABI. I'll consider
nodenames an ABI if a change is noticed, but not for sysfs path.

https://lore.kernel.org/linux-rockchip/CAL_JsqJGx_MwX9ynMiCUm_xP3t6xWbrtiUi-F+2ceXqZ2Su5tA@mail.gmail.com/

Make use of the properties and a compatible in a node instead.

===
A little test.
From: rockchip_gpio_probe():

int main() {
	//char dev_name[] = "gpio1@2003c000";
	char dev_name[] = "gpio@2003c000";
	char priv_name[2];
	int priv_bank;
	char *end;

	end = strrchr(dev_name, '@');
	priv_bank = trailing_strtoln(dev_name, end);

	priv_name[0] = 'A' + priv_bank;
	priv_name[1] = 0;

	printf("priv_bank: %d\n", priv_bank);
	printf("priv_name: %s\n", priv_name);
	return 0;
}

// priv_bank: 1
// priv_name: B

A change of node name gives:

// priv_bank: -1
// priv_name: @

===

Linux driver gpio-rockchip.c has an alias or else an increment id.

id = of_alias_get_id(np, "gpio");
	if (id < 0)
		id = gpio++;

Problem:
Probe order is not guarantied.
Aliases should go to board files and not in dtsi files anymore. 

===

The current compatible string is generic, but these strings should normally be SoC orientated.

Proposal:

replace:
compatible = "rockchip,gpio-bank";

by:
compatible = "rockchip,rk3188-gpio-bank";

With that known we can lookup the reg address in a lookup table and it's name for u-boot.

struct lookup_table rk_gpio_rk3188_data[] = {
	{0x2000a000, "A"},
	{0x2003c000, "B"},
	{0x2003e000, "C"},
	{0x20080000, "D"},
};


	{ .compatible = "rockchip,rk3188-gpio-bank", .data = &rk_gpio_rk3188_data },

===

If needed I can help with changes to rockchip,gpio-bank.yaml

Let me know if it's useful or that you have an other solution as long as we get forward here.

Johan

> 

> I believe this is the same issue as on RK3399 and PX30: this breaks our GPIO controller driver.

> 
> c.f. https://lore.kernel.org/u-boot/0ab9a600-b517-0ce5-d189-99fc8eddfa60@theobroma-systems.com/ and its answers.
> 
> Cheers,
> Quentin

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[John Keeping]

Hi Johan,

On Tue, Jan 17, 2023 at 03:44:07PM +0100, Johan Jonker wrote:
> On 1/17/23 10:46, Quentin Schulz wrote:
> > On 1/16/23 20:45, Johan Jonker wrote:
> >> Sync rk3066/rk3188 DT files from Linux.
> >> This is the state as of linux-next v6.2-rc4.
> >> New nfc node for MK808 rk3066a.
> >> CRU nodes now have a clock property.
> >> To prefend dtoc errors a fixed clock must also be
> >> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
> >>
> >> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> >> ---
> 
> [..]
> 
> >> @@ -223,7 +224,7 @@
> >>           #size-cells = <1>;
> >>           ranges;
> >>
> >> -        gpio0: gpio0@2000a000 {
> >> +        gpio0: gpio@2000a000 {
> >>               compatible = "rockchip,rk3188-gpio-bank0";
> >>               reg = <0x2000a000 0x100>;
> >>               interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> >> @@ -236,7 +237,7 @@
> >>               #interrupt-cells = <2>;
> >>           };
> >>
> >> -        gpio1: gpio1@2003c000 {
> 
> >> +        gpio1: gpio@2003c000 {
> 
> Hi,
> 
> LOL: I made that binding change on request from Linux DT maintainers.
> Node names should generic.
> 
> ===
> 
> My full u-boot is able to boot a Linux kernel for rk3066a.
> Only when I give the command below it crashes:
> 
> gpio status -a
> 
> Could you confirm what other parts are effected?
> 
> If it's boots then it's good enough for me and move forward, so please merge.(Kever)
> 
> Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!

This reminded me of a patch that I never followed up with:

	https://lore.kernel.org/u-boot/20220726162509.1304234-1-john@metanate.com/

Can you test if that fixes `gpio status -a` for you?


John

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[John Keeping]

Hi Johan,

On Tue, Jan 17, 2023 at 03:44:07PM +0100, Johan Jonker wrote:
> On 1/17/23 10:46, Quentin Schulz wrote:
> > On 1/16/23 20:45, Johan Jonker wrote:
> >> Sync rk3066/rk3188 DT files from Linux.
> >> This is the state as of linux-next v6.2-rc4.
> >> New nfc node for MK808 rk3066a.
> >> CRU nodes now have a clock property.
> >> To prefend dtoc errors a fixed clock must also be
> >> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
> >>
> >> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> >> ---
> 
> [..]
> 
> >> @@ -223,7 +224,7 @@
> >>           #size-cells = <1>;
> >>           ranges;
> >>
> >> -        gpio0: gpio0@2000a000 {
> >> +        gpio0: gpio@2000a000 {
> >>               compatible = "rockchip,rk3188-gpio-bank0";
> >>               reg = <0x2000a000 0x100>;
> >>               interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> >> @@ -236,7 +237,7 @@
> >>               #interrupt-cells = <2>;
> >>           };
> >>
> >> -        gpio1: gpio1@2003c000 {
> 
> >> +        gpio1: gpio@2003c000 {
> 
> Hi,
> 
> LOL: I made that binding change on request from Linux DT maintainers.
> Node names should generic.
> 
> ===
> 
> My full u-boot is able to boot a Linux kernel for rk3066a.
> Only when I give the command below it crashes:
> 
> gpio status -a
> 
> Could you confirm what other parts are effected?
> 
> If it's boots then it's good enough for me and move forward, so please merge.(Kever)
> 
> Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!

This reminded me of a patch that I never followed up with:

	https://lore.kernel.org/u-boot/20220726162509.1304234-1-john@metanate.com/

Can you test if that fixes `gpio status -a` for you?


John

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Quentin Schulz]

Hi John,

On 1/17/23 16:20, John Keeping wrote:
> Hi Johan,
> 
> On Tue, Jan 17, 2023 at 03:44:07PM +0100, Johan Jonker wrote:
>> On 1/17/23 10:46, Quentin Schulz wrote:
>>> On 1/16/23 20:45, Johan Jonker wrote:
>>>> Sync rk3066/rk3188 DT files from Linux.
>>>> This is the state as of linux-next v6.2-rc4.
>>>> New nfc node for MK808 rk3066a.
>>>> CRU nodes now have a clock property.
>>>> To prefend dtoc errors a fixed clock must also be
>>>> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
>>>>
>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>> ---
>>
>> [..]
>>
>>>> @@ -223,7 +224,7 @@
>>>>            #size-cells = <1>;
>>>>            ranges;
>>>>
>>>> -        gpio0: gpio0@2000a000 {
>>>> +        gpio0: gpio@2000a000 {
>>>>                compatible = "rockchip,rk3188-gpio-bank0";
>>>>                reg = <0x2000a000 0x100>;
>>>>                interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>>>> @@ -236,7 +237,7 @@
>>>>                #interrupt-cells = <2>;
>>>>            };
>>>>
>>>> -        gpio1: gpio1@2003c000 {
>>
>>>> +        gpio1: gpio@2003c000 {
>>
>> Hi,
>>
>> LOL: I made that binding change on request from Linux DT maintainers.
>> Node names should generic.
>>
>> ===
>>
>> My full u-boot is able to boot a Linux kernel for rk3066a.
>> Only when I give the command below it crashes:
>>
>> gpio status -a
>>
>> Could you confirm what other parts are effected?
>>
>> If it's boots then it's good enough for me and move forward, so please merge.(Kever)
>>
>> Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!
> 
> This reminded me of a patch that I never followed up with:
> 
> 	https://urldefense.com/v3/__https://lore.kernel.org/u-boot/20220726162509.1304234-1-john@metanate.com/__;!!OOPJP91ZZw!lmqsst1SAzm_NO-LfRooukeMJOc5jP-Tqsm_DdWmyjnQc2VsP0D4TWJakw0qh5K-ENTe96HCtGp-MjKZtVrDM4Y8Tlg$
> 
> Can you test if that fixes `gpio status -a` for you?
> 

That's not enough I believe. You need CONFIG_DM_SEQ_ALIAS enabled, which 
isn't a given and we shouldn't rely on it. Also, there are currently no 
aliases for gpio controllers. And we shouldn't rely on u-boot.dtsi for 
those, because upstream might decide to have an alias gpio2 for gpio0 
for a board.

Cheers,
Quentin

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Quentin Schulz]

Hi John,

On 1/17/23 16:20, John Keeping wrote:
> Hi Johan,
> 
> On Tue, Jan 17, 2023 at 03:44:07PM +0100, Johan Jonker wrote:
>> On 1/17/23 10:46, Quentin Schulz wrote:
>>> On 1/16/23 20:45, Johan Jonker wrote:
>>>> Sync rk3066/rk3188 DT files from Linux.
>>>> This is the state as of linux-next v6.2-rc4.
>>>> New nfc node for MK808 rk3066a.
>>>> CRU nodes now have a clock property.
>>>> To prefend dtoc errors a fixed clock must also be
>>>> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
>>>>
>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>> ---
>>
>> [..]
>>
>>>> @@ -223,7 +224,7 @@
>>>>            #size-cells = <1>;
>>>>            ranges;
>>>>
>>>> -        gpio0: gpio0@2000a000 {
>>>> +        gpio0: gpio@2000a000 {
>>>>                compatible = "rockchip,rk3188-gpio-bank0";
>>>>                reg = <0x2000a000 0x100>;
>>>>                interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>>>> @@ -236,7 +237,7 @@
>>>>                #interrupt-cells = <2>;
>>>>            };
>>>>
>>>> -        gpio1: gpio1@2003c000 {
>>
>>>> +        gpio1: gpio@2003c000 {
>>
>> Hi,
>>
>> LOL: I made that binding change on request from Linux DT maintainers.
>> Node names should generic.
>>
>> ===
>>
>> My full u-boot is able to boot a Linux kernel for rk3066a.
>> Only when I give the command below it crashes:
>>
>> gpio status -a
>>
>> Could you confirm what other parts are effected?
>>
>> If it's boots then it's good enough for me and move forward, so please merge.(Kever)
>>
>> Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!
> 
> This reminded me of a patch that I never followed up with:
> 
> 	https://urldefense.com/v3/__https://lore.kernel.org/u-boot/20220726162509.1304234-1-john@metanate.com/__;!!OOPJP91ZZw!lmqsst1SAzm_NO-LfRooukeMJOc5jP-Tqsm_DdWmyjnQc2VsP0D4TWJakw0qh5K-ENTe96HCtGp-MjKZtVrDM4Y8Tlg$
> 
> Can you test if that fixes `gpio status -a` for you?
> 

That's not enough I believe. You need CONFIG_DM_SEQ_ALIAS enabled, which 
isn't a given and we shouldn't rely on it. Also, there are currently no 
aliases for gpio controllers. And we shouldn't rely on u-boot.dtsi for 
those, because upstream might decide to have an alias gpio2 for gpio0 
for a board.

Cheers,
Quentin

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Quentin Schulz]

Hi Johan,

On 1/17/23 15:44, Johan Jonker wrote:
> 
> 
> On 1/17/23 10:46, Quentin Schulz wrote:
>> Hi Johan,
>>
>> On 1/16/23 20:45, Johan Jonker wrote:
>>> Sync rk3066/rk3188 DT files from Linux.
>>> This is the state as of linux-next v6.2-rc4.
>>> New nfc node for MK808 rk3066a.
>>> CRU nodes now have a clock property.
>>> To prefend dtoc errors a fixed clock must also be
>>> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
>>>
>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>> ---
> 
> [..]
> 
>>> @@ -223,7 +224,7 @@
>>>            #size-cells = <1>;
>>>            ranges;
>>>
>>> -        gpio0: gpio0@2000a000 {
>>> +        gpio0: gpio@2000a000 {
>>>                compatible = "rockchip,rk3188-gpio-bank0";
>>>                reg = <0x2000a000 0x100>;
>>>                interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>>> @@ -236,7 +237,7 @@
>>>                #interrupt-cells = <2>;
>>>            };
>>>
>>> -        gpio1: gpio1@2003c000 {
> 
>>> +        gpio1: gpio@2003c000 {
> 
> Hi,
> 
> LOL: I made that binding change on request from Linux DT maintainers.
> Node names should generic.
> 

Yup I know. I tried to use a 5.19 DTS on a 5.10 kernel while debugging 
some stuff and was quite surprised to not even be able to reach userspace.

> ===
> 
> My full u-boot is able to boot a Linux kernel for rk3066a.
> Only when I give the command below it crashes:
> 
> gpio status -a
> 
> Could you confirm what other parts are effected?
> 

Well... Anything using GPIOs I believe? So Devices with a phandle to a 
gpio from an SoC GPIO controller, or some platform code getting a GPIO 
udevice to interact with some GPIO. Pinctrl shouldn't be affected 
AFAICT, so that might be the reason why most things seems to have worked 
fine for you?

> If it's boots then it's good enough for me and move forward, so please merge.(Kever)
> 

Since there's only one board using it, I guess that's your/Kever';s 
call, but that's not the case for PX30/RK3399 for example... where we'll 
need a similar change (though yes, my PX30 board also boots fine without 
and currently has this new DTSI, so broken GPIO driver).

> Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!
> 
> ===
> 
> Using DT path or node name is wrong.
> 
> Comment by robh+dt:
> /sys/bus/platform/devices/ paths are not an ABI. I'll consider
> nodenames an ABI if a change is noticed, but not for sysfs path.
> 
> https://urldefense.com/v3/__https://lore.kernel.org/linux-rockchip/CAL_JsqJGx_MwX9ynMiCUm_xP3t6xWbrtiUi-F*2ceXqZ2Su5tA@mail.gmail.com/__;Kw!!OOPJP91ZZw!iA_hrQyQy4KmdSwe46spmhBqsGQRRG4X_39kp9-JtCChxwMUdG22ICVmKLZHaIogqzTRzCZuHJ_rZ1hw5urOjubS9xJrRg$
> 

Yeah well... We weren't using the sysfs path but still have an issue :)

> Make use of the properties and a compatible in a node instead.
> 

That we can agree on.

> ===
> A little test.
> From: rockchip_gpio_probe():
> 
> int main() {
> 	//char dev_name[] = "gpio1@2003c000";
> 	char dev_name[] = "gpio@2003c000";
> 	char priv_name[2];
> 	int priv_bank;
> 	char *end;
> 
> 	end = strrchr(dev_name, '@');
> 	priv_bank = trailing_strtoln(dev_name, end);
> 
> 	priv_name[0] = 'A' + priv_bank;
> 	priv_name[1] = 0;
> 
> 	printf("priv_bank: %d\n", priv_bank);
> 	printf("priv_name: %s\n", priv_name);
> 	return 0;
> }
> 
> // priv_bank: 1
> // priv_name: B
> 
> A change of node name gives:
> 
> // priv_bank: -1
> // priv_name: @
> 
> ===
> 
> Linux driver gpio-rockchip.c has an alias or else an increment id.
> 
> id = of_alias_get_id(np, "gpio");
> 	if (id < 0)
> 		id = gpio++;
> 
> Problem:
> Probe order is not guarantied.
> Aliases should go to board files and not in dtsi files anymore.
> 

And also, nothing forbids any board user to have gpio2= <&gpio0> in 
their aliases. We shouldn't rely on aliases for anything else than how 
it;s exposed to userspace I believe. Here it's a mistake anyways.

> ===
> 
> The current compatible string is generic, but these strings should normally be SoC orientated.
> 
> Proposal:
> 
> replace:
> compatible = "rockchip,gpio-bank";
> 
> by:
> compatible = "rockchip,rk3188-gpio-bank";
> 
> With that known we can lookup the reg address in a lookup table and it's name for u-boot.
> 
> struct lookup_table rk_gpio_rk3188_data[] = {
> 	{0x2000a000, "A"},
> 	{0x2003c000, "B"},
> 	{0x2003e000, "C"},
> 	{0x20080000, "D"},
> };
> 
> 
> 	{ .compatible = "rockchip,rk3188-gpio-bank", .data = &rk_gpio_rk3188_data },
> 

Yes, that's an option too.

> ===
> 
> If needed I can help with changes to rockchip,gpio-bank.yaml
> 
> Let me know if it's useful or that you have an other solution as long as we get forward here.
> 

I believe Kever will want this to be upstreamed first in the kernel 
(though I think he doesn't mind if we sync stuff with linux-next, so 
that should make it faster to be merged in U-Boot).

I suggested:

rockchip,gpio-controller = <0>;
in gpio0,
rockchip,gpio-controller = <1>;
in gpio1, etc....

in the DT and reading this information from the driver directly. In 
which case, there's very little to do in the driver except getting this 
number and adding it to 'A'. We could also have a char there and not do 
this gymnastic with adding a number to the 'A' character.

Cheers,
Quentin

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Quentin Schulz]

Hi Johan,

On 1/17/23 15:44, Johan Jonker wrote:
> 
> 
> On 1/17/23 10:46, Quentin Schulz wrote:
>> Hi Johan,
>>
>> On 1/16/23 20:45, Johan Jonker wrote:
>>> Sync rk3066/rk3188 DT files from Linux.
>>> This is the state as of linux-next v6.2-rc4.
>>> New nfc node for MK808 rk3066a.
>>> CRU nodes now have a clock property.
>>> To prefend dtoc errors a fixed clock must also be
>>> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
>>>
>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>> ---
> 
> [..]
> 
>>> @@ -223,7 +224,7 @@
>>>            #size-cells = <1>;
>>>            ranges;
>>>
>>> -        gpio0: gpio0@2000a000 {
>>> +        gpio0: gpio@2000a000 {
>>>                compatible = "rockchip,rk3188-gpio-bank0";
>>>                reg = <0x2000a000 0x100>;
>>>                interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>>> @@ -236,7 +237,7 @@
>>>                #interrupt-cells = <2>;
>>>            };
>>>
>>> -        gpio1: gpio1@2003c000 {
> 
>>> +        gpio1: gpio@2003c000 {
> 
> Hi,
> 
> LOL: I made that binding change on request from Linux DT maintainers.
> Node names should generic.
> 

Yup I know. I tried to use a 5.19 DTS on a 5.10 kernel while debugging 
some stuff and was quite surprised to not even be able to reach userspace.

> ===
> 
> My full u-boot is able to boot a Linux kernel for rk3066a.
> Only when I give the command below it crashes:
> 
> gpio status -a
> 
> Could you confirm what other parts are effected?
> 

Well... Anything using GPIOs I believe? So Devices with a phandle to a 
gpio from an SoC GPIO controller, or some platform code getting a GPIO 
udevice to interact with some GPIO. Pinctrl shouldn't be affected 
AFAICT, so that might be the reason why most things seems to have worked 
fine for you?

> If it's boots then it's good enough for me and move forward, so please merge.(Kever)
> 

Since there's only one board using it, I guess that's your/Kever';s 
call, but that's not the case for PX30/RK3399 for example... where we'll 
need a similar change (though yes, my PX30 board also boots fine without 
and currently has this new DTSI, so broken GPIO driver).

> Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!
> 
> ===
> 
> Using DT path or node name is wrong.
> 
> Comment by robh+dt:
> /sys/bus/platform/devices/ paths are not an ABI. I'll consider
> nodenames an ABI if a change is noticed, but not for sysfs path.
> 
> https://urldefense.com/v3/__https://lore.kernel.org/linux-rockchip/CAL_JsqJGx_MwX9ynMiCUm_xP3t6xWbrtiUi-F*2ceXqZ2Su5tA@mail.gmail.com/__;Kw!!OOPJP91ZZw!iA_hrQyQy4KmdSwe46spmhBqsGQRRG4X_39kp9-JtCChxwMUdG22ICVmKLZHaIogqzTRzCZuHJ_rZ1hw5urOjubS9xJrRg$
> 

Yeah well... We weren't using the sysfs path but still have an issue :)

> Make use of the properties and a compatible in a node instead.
> 

That we can agree on.

> ===
> A little test.
> From: rockchip_gpio_probe():
> 
> int main() {
> 	//char dev_name[] = "gpio1@2003c000";
> 	char dev_name[] = "gpio@2003c000";
> 	char priv_name[2];
> 	int priv_bank;
> 	char *end;
> 
> 	end = strrchr(dev_name, '@');
> 	priv_bank = trailing_strtoln(dev_name, end);
> 
> 	priv_name[0] = 'A' + priv_bank;
> 	priv_name[1] = 0;
> 
> 	printf("priv_bank: %d\n", priv_bank);
> 	printf("priv_name: %s\n", priv_name);
> 	return 0;
> }
> 
> // priv_bank: 1
> // priv_name: B
> 
> A change of node name gives:
> 
> // priv_bank: -1
> // priv_name: @
> 
> ===
> 
> Linux driver gpio-rockchip.c has an alias or else an increment id.
> 
> id = of_alias_get_id(np, "gpio");
> 	if (id < 0)
> 		id = gpio++;
> 
> Problem:
> Probe order is not guarantied.
> Aliases should go to board files and not in dtsi files anymore.
> 

And also, nothing forbids any board user to have gpio2= <&gpio0> in 
their aliases. We shouldn't rely on aliases for anything else than how 
it;s exposed to userspace I believe. Here it's a mistake anyways.

> ===
> 
> The current compatible string is generic, but these strings should normally be SoC orientated.
> 
> Proposal:
> 
> replace:
> compatible = "rockchip,gpio-bank";
> 
> by:
> compatible = "rockchip,rk3188-gpio-bank";
> 
> With that known we can lookup the reg address in a lookup table and it's name for u-boot.
> 
> struct lookup_table rk_gpio_rk3188_data[] = {
> 	{0x2000a000, "A"},
> 	{0x2003c000, "B"},
> 	{0x2003e000, "C"},
> 	{0x20080000, "D"},
> };
> 
> 
> 	{ .compatible = "rockchip,rk3188-gpio-bank", .data = &rk_gpio_rk3188_data },
> 

Yes, that's an option too.

> ===
> 
> If needed I can help with changes to rockchip,gpio-bank.yaml
> 
> Let me know if it's useful or that you have an other solution as long as we get forward here.
> 

I believe Kever will want this to be upstreamed first in the kernel 
(though I think he doesn't mind if we sync stuff with linux-next, so 
that should make it faster to be merged in U-Boot).

I suggested:

rockchip,gpio-controller = <0>;
in gpio0,
rockchip,gpio-controller = <1>;
in gpio1, etc....

in the DT and reading this information from the driver directly. In 
which case, there's very little to do in the driver except getting this 
number and adding it to 'A'. We could also have a char there and not do 
this gymnastic with adding a number to the 'A' character.

Cheers,
Quentin

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[John Keeping]

Hi Quentin,

On Tue, Jan 17, 2023 at 04:58:54PM +0100, Quentin Schulz wrote:
> On 1/17/23 16:20, John Keeping wrote:
> > Hi Johan,
> > 
> > On Tue, Jan 17, 2023 at 03:44:07PM +0100, Johan Jonker wrote:
> > > On 1/17/23 10:46, Quentin Schulz wrote:
> > > > On 1/16/23 20:45, Johan Jonker wrote:
> > > > > Sync rk3066/rk3188 DT files from Linux.
> > > > > This is the state as of linux-next v6.2-rc4.
> > > > > New nfc node for MK808 rk3066a.
> > > > > CRU nodes now have a clock property.
> > > > > To prefend dtoc errors a fixed clock must also be
> > > > > included for tpl/spl in the rk3xxx-u-boot.dtsi file.
> > > > > 
> > > > > Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> > > > > ---
> > > 
> > > [..]
> > > 
> > > > > @@ -223,7 +224,7 @@
> > > > >            #size-cells = <1>;
> > > > >            ranges;
> > > > > 
> > > > > -        gpio0: gpio0@2000a000 {
> > > > > +        gpio0: gpio@2000a000 {
> > > > >                compatible = "rockchip,rk3188-gpio-bank0";
> > > > >                reg = <0x2000a000 0x100>;
> > > > >                interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> > > > > @@ -236,7 +237,7 @@
> > > > >                #interrupt-cells = <2>;
> > > > >            };
> > > > > 
> > > > > -        gpio1: gpio1@2003c000 {
> > > 
> > > > > +        gpio1: gpio@2003c000 {
> > > 
> > > Hi,
> > > 
> > > LOL: I made that binding change on request from Linux DT maintainers.
> > > Node names should generic.
> > > 
> > > ===
> > > 
> > > My full u-boot is able to boot a Linux kernel for rk3066a.
> > > Only when I give the command below it crashes:
> > > 
> > > gpio status -a
> > > 
> > > Could you confirm what other parts are effected?
> > > 
> > > If it's boots then it's good enough for me and move forward, so please merge.(Kever)
> > > 
> > > Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!
> > 
> > This reminded me of a patch that I never followed up with:
> > 
> > 	https://urldefense.com/v3/__https://lore.kernel.org/u-boot/20220726162509.1304234-1-john@metanate.com/__;!!OOPJP91ZZw!lmqsst1SAzm_NO-LfRooukeMJOc5jP-Tqsm_DdWmyjnQc2VsP0D4TWJakw0qh5K-ENTe96HCtGp-MjKZtVrDM4Y8Tlg$
> > 
> > Can you test if that fixes `gpio status -a` for you?
> > 
> 
> That's not enough I believe. You need CONFIG_DM_SEQ_ALIAS enabled, which
> isn't a given and we shouldn't rely on it. Also, there are currently no
> aliases for gpio controllers. And we shouldn't rely on u-boot.dtsi for
> those, because upstream might decide to have an alias gpio2 for gpio0 for a
> board.

If the aliases are set up in a non-standard way, then I think changing
the name here may be the right thing to do.

Any GPIOs referenced in the device tree will be by handle so the name is
irrelevant.  GPIO names are only used by board code or by the `gpio`
command, both of which are tightly tied to the device tree anyway.

If a stable name for a GPIO is needed, then the gpio-line-names DT
property exists and is supported by Linux.  The names here are purely a
U-Boot construction and effectively part of a "userspace" interface.

Just using dev_seq() unconditionally as Simon suggested in response to
my patch seems like the right answer (and then there is no dependency on
CONFIG_DM_SEQ_ALIAS although boards can require it and use aliases if
necessary).


John

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[John Keeping]

Hi Quentin,

On Tue, Jan 17, 2023 at 04:58:54PM +0100, Quentin Schulz wrote:
> On 1/17/23 16:20, John Keeping wrote:
> > Hi Johan,
> > 
> > On Tue, Jan 17, 2023 at 03:44:07PM +0100, Johan Jonker wrote:
> > > On 1/17/23 10:46, Quentin Schulz wrote:
> > > > On 1/16/23 20:45, Johan Jonker wrote:
> > > > > Sync rk3066/rk3188 DT files from Linux.
> > > > > This is the state as of linux-next v6.2-rc4.
> > > > > New nfc node for MK808 rk3066a.
> > > > > CRU nodes now have a clock property.
> > > > > To prefend dtoc errors a fixed clock must also be
> > > > > included for tpl/spl in the rk3xxx-u-boot.dtsi file.
> > > > > 
> > > > > Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> > > > > ---
> > > 
> > > [..]
> > > 
> > > > > @@ -223,7 +224,7 @@
> > > > >            #size-cells = <1>;
> > > > >            ranges;
> > > > > 
> > > > > -        gpio0: gpio0@2000a000 {
> > > > > +        gpio0: gpio@2000a000 {
> > > > >                compatible = "rockchip,rk3188-gpio-bank0";
> > > > >                reg = <0x2000a000 0x100>;
> > > > >                interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> > > > > @@ -236,7 +237,7 @@
> > > > >                #interrupt-cells = <2>;
> > > > >            };
> > > > > 
> > > > > -        gpio1: gpio1@2003c000 {
> > > 
> > > > > +        gpio1: gpio@2003c000 {
> > > 
> > > Hi,
> > > 
> > > LOL: I made that binding change on request from Linux DT maintainers.
> > > Node names should generic.
> > > 
> > > ===
> > > 
> > > My full u-boot is able to boot a Linux kernel for rk3066a.
> > > Only when I give the command below it crashes:
> > > 
> > > gpio status -a
> > > 
> > > Could you confirm what other parts are effected?
> > > 
> > > If it's boots then it's good enough for me and move forward, so please merge.(Kever)
> > > 
> > > Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!
> > 
> > This reminded me of a patch that I never followed up with:
> > 
> > 	https://urldefense.com/v3/__https://lore.kernel.org/u-boot/20220726162509.1304234-1-john@metanate.com/__;!!OOPJP91ZZw!lmqsst1SAzm_NO-LfRooukeMJOc5jP-Tqsm_DdWmyjnQc2VsP0D4TWJakw0qh5K-ENTe96HCtGp-MjKZtVrDM4Y8Tlg$
> > 
> > Can you test if that fixes `gpio status -a` for you?
> > 
> 
> That's not enough I believe. You need CONFIG_DM_SEQ_ALIAS enabled, which
> isn't a given and we shouldn't rely on it. Also, there are currently no
> aliases for gpio controllers. And we shouldn't rely on u-boot.dtsi for
> those, because upstream might decide to have an alias gpio2 for gpio0 for a
> board.

If the aliases are set up in a non-standard way, then I think changing
the name here may be the right thing to do.

Any GPIOs referenced in the device tree will be by handle so the name is
irrelevant.  GPIO names are only used by board code or by the `gpio`
command, both of which are tightly tied to the device tree anyway.

If a stable name for a GPIO is needed, then the gpio-line-names DT
property exists and is supported by Linux.  The names here are purely a
U-Boot construction and effectively part of a "userspace" interface.

Just using dev_seq() unconditionally as Simon suggested in response to
my patch seems like the right answer (and then there is no dependency on
CONFIG_DM_SEQ_ALIAS although boards can require it and use aliases if
necessary).


John

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Johan Jonker]

On 1/17/23 18:27, John Keeping wrote:
> Hi Quentin,
> 
> On Tue, Jan 17, 2023 at 04:58:54PM +0100, Quentin Schulz wrote:
>> On 1/17/23 16:20, John Keeping wrote:
>>> Hi Johan,
>>>
>>> On Tue, Jan 17, 2023 at 03:44:07PM +0100, Johan Jonker wrote:
>>>> On 1/17/23 10:46, Quentin Schulz wrote:
>>>>> On 1/16/23 20:45, Johan Jonker wrote:
>>>>>> Sync rk3066/rk3188 DT files from Linux.
>>>>>> This is the state as of linux-next v6.2-rc4.
>>>>>> New nfc node for MK808 rk3066a.
>>>>>> CRU nodes now have a clock property.
>>>>>> To prefend dtoc errors a fixed clock must also be
>>>>>> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
>>>>>>
>>>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>>>> ---
>>>>
>>>> [..]
>>>>
>>>>>> @@ -223,7 +224,7 @@
>>>>>>            #size-cells = <1>;
>>>>>>            ranges;
>>>>>>
>>>>>> -        gpio0: gpio0@2000a000 {
>>>>>> +        gpio0: gpio@2000a000 {
>>>>>>                compatible = "rockchip,rk3188-gpio-bank0";
>>>>>>                reg = <0x2000a000 0x100>;
>>>>>>                interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>>>>>> @@ -236,7 +237,7 @@
>>>>>>                #interrupt-cells = <2>;
>>>>>>            };
>>>>>>
>>>>>> -        gpio1: gpio1@2003c000 {
>>>>
>>>>>> +        gpio1: gpio@2003c000 {
>>>>
>>>> Hi,
>>>>
>>>> LOL: I made that binding change on request from Linux DT maintainers.
>>>> Node names should generic.
>>>>
>>>> ===
>>>>
>>>> My full u-boot is able to boot a Linux kernel for rk3066a.
>>>> Only when I give the command below it crashes:
>>>>
>>>> gpio status -a
>>>>
>>>> Could you confirm what other parts are effected?
>>>>
>>>> If it's boots then it's good enough for me and move forward, so please merge.(Kever)
>>>>
>>>> Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!
>>>
>>> This reminded me of a patch that I never followed up with:
>>>
>>> 	https://urldefense.com/v3/__https://lore.kernel.org/u-boot/20220726162509.1304234-1-john@metanate.com/__;!!OOPJP91ZZw!lmqsst1SAzm_NO-LfRooukeMJOc5jP-Tqsm_DdWmyjnQc2VsP0D4TWJakw0qh5K-ENTe96HCtGp-MjKZtVrDM4Y8Tlg$
>>>
>>> Can you test if that fixes `gpio status -a` for you?
>>>
>>
>> That's not enough I believe. You need CONFIG_DM_SEQ_ALIAS enabled, which
>> isn't a given and we shouldn't rely on it. Also, there are currently no
>> aliases for gpio controllers. And we shouldn't rely on u-boot.dtsi for
>> those, because upstream might decide to have an alias gpio2 for gpio0 for a
>> board.
> 
> If the aliases are set up in a non-standard way, then I think changing
> the name here may be the right thing to do.
> 
> Any GPIOs referenced in the device tree will be by handle so the name is
> irrelevant.  GPIO names are only used by board code or by the `gpio`
> command, both of which are tightly tied to the device tree anyway.
> 
> If a stable name for a GPIO is needed, then the gpio-line-names DT
> property exists and is supported by Linux.  The names here are purely a
> U-Boot construction and effectively part of a "userspace" interface.
> 

> Just using dev_seq() unconditionally as Simon suggested in response to
> my patch seems like the right answer (and then there is no dependency on
> CONFIG_DM_SEQ_ALIAS although boards can require it and use aliases if
> necessary).

I tested a bit with rk3066a mk808 with number gap between 4 and 6.
That letter is not consistent F vs G in u-boot with and without aliases.

My idea is that the use of aliases is more of something of an additional thing that we not always can count on that is there.
DTOC processing for dt-plat.c only looks at the nodes it self. Finding your ID then comes from the properties inside that node.
(ie: compatible + reg or separate id property)
Others can have other better ideas of course. Let us know.

Johan

> 
> 
> John

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Johan Jonker]

On 1/17/23 18:27, John Keeping wrote:
> Hi Quentin,
> 
> On Tue, Jan 17, 2023 at 04:58:54PM +0100, Quentin Schulz wrote:
>> On 1/17/23 16:20, John Keeping wrote:
>>> Hi Johan,
>>>
>>> On Tue, Jan 17, 2023 at 03:44:07PM +0100, Johan Jonker wrote:
>>>> On 1/17/23 10:46, Quentin Schulz wrote:
>>>>> On 1/16/23 20:45, Johan Jonker wrote:
>>>>>> Sync rk3066/rk3188 DT files from Linux.
>>>>>> This is the state as of linux-next v6.2-rc4.
>>>>>> New nfc node for MK808 rk3066a.
>>>>>> CRU nodes now have a clock property.
>>>>>> To prefend dtoc errors a fixed clock must also be
>>>>>> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
>>>>>>
>>>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>>>> ---
>>>>
>>>> [..]
>>>>
>>>>>> @@ -223,7 +224,7 @@
>>>>>>            #size-cells = <1>;
>>>>>>            ranges;
>>>>>>
>>>>>> -        gpio0: gpio0@2000a000 {
>>>>>> +        gpio0: gpio@2000a000 {
>>>>>>                compatible = "rockchip,rk3188-gpio-bank0";
>>>>>>                reg = <0x2000a000 0x100>;
>>>>>>                interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>>>>>> @@ -236,7 +237,7 @@
>>>>>>                #interrupt-cells = <2>;
>>>>>>            };
>>>>>>
>>>>>> -        gpio1: gpio1@2003c000 {
>>>>
>>>>>> +        gpio1: gpio@2003c000 {
>>>>
>>>> Hi,
>>>>
>>>> LOL: I made that binding change on request from Linux DT maintainers.
>>>> Node names should generic.
>>>>
>>>> ===
>>>>
>>>> My full u-boot is able to boot a Linux kernel for rk3066a.
>>>> Only when I give the command below it crashes:
>>>>
>>>> gpio status -a
>>>>
>>>> Could you confirm what other parts are effected?
>>>>
>>>> If it's boots then it's good enough for me and move forward, so please merge.(Kever)
>>>>
>>>> Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!
>>>
>>> This reminded me of a patch that I never followed up with:
>>>
>>> 	https://urldefense.com/v3/__https://lore.kernel.org/u-boot/20220726162509.1304234-1-john@metanate.com/__;!!OOPJP91ZZw!lmqsst1SAzm_NO-LfRooukeMJOc5jP-Tqsm_DdWmyjnQc2VsP0D4TWJakw0qh5K-ENTe96HCtGp-MjKZtVrDM4Y8Tlg$
>>>
>>> Can you test if that fixes `gpio status -a` for you?
>>>
>>
>> That's not enough I believe. You need CONFIG_DM_SEQ_ALIAS enabled, which
>> isn't a given and we shouldn't rely on it. Also, there are currently no
>> aliases for gpio controllers. And we shouldn't rely on u-boot.dtsi for
>> those, because upstream might decide to have an alias gpio2 for gpio0 for a
>> board.
> 
> If the aliases are set up in a non-standard way, then I think changing
> the name here may be the right thing to do.
> 
> Any GPIOs referenced in the device tree will be by handle so the name is
> irrelevant.  GPIO names are only used by board code or by the `gpio`
> command, both of which are tightly tied to the device tree anyway.
> 
> If a stable name for a GPIO is needed, then the gpio-line-names DT
> property exists and is supported by Linux.  The names here are purely a
> U-Boot construction and effectively part of a "userspace" interface.
> 

> Just using dev_seq() unconditionally as Simon suggested in response to
> my patch seems like the right answer (and then there is no dependency on
> CONFIG_DM_SEQ_ALIAS although boards can require it and use aliases if
> necessary).

I tested a bit with rk3066a mk808 with number gap between 4 and 6.
That letter is not consistent F vs G in u-boot with and without aliases.

My idea is that the use of aliases is more of something of an additional thing that we not always can count on that is there.
DTOC processing for dt-plat.c only looks at the nodes it self. Finding your ID then comes from the properties inside that node.
(ie: compatible + reg or separate id property)
Others can have other better ideas of course. Let us know.

Johan

> 
> 
> John