[PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Johan Jonker]

The current divider to calculate the bank ID can change.
Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 drivers/gpio/rk_gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
index f7ad4d68..0a2acf18 100644
--- a/drivers/gpio/rk_gpio.c
+++ b/drivers/gpio/rk_gpio.c
@@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
 					     0, &args);
 	if (!ret || ret != -ENOENT) {
 		uc_priv->gpio_count = args.args[2];
-		priv->bank = args.args[1] / args.args[2];
+		priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
 	} else {
 		uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
 		end = strrchr(dev->name, '@');
--
2.20.1

[PATCH v1 2/6] arm: dts: rockchip: rk3066a-u-boot: add gpio-ranges

[Johan Jonker]

The gpio node names are made generic, but without
gpio bank ID. Add gpio-ranges to rk3066a-u-boot.dtsi
for now till a better method is found.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 arch/arm/dts/rk3066a-u-boot.dtsi | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/dts/rk3066a-u-boot.dtsi b/arch/arm/dts/rk3066a-u-boot.dtsi
index bc6e609d..8d4e1ad2 100644
--- a/arch/arm/dts/rk3066a-u-boot.dtsi
+++ b/arch/arm/dts/rk3066a-u-boot.dtsi
@@ -2,3 +2,28 @@

 #include "rockchip-u-boot.dtsi"
 #include "rk3xxx-u-boot.dtsi"
+
+&gpio0 {
+	gpio-ranges = <&pinctrl 0 0 32>;
+};
+
+&gpio1 {
+	gpio-ranges = <&pinctrl 0 32 32>;
+};
+
+&gpio2 {
+	gpio-ranges = <&pinctrl 0 64 32>;
+};
+
+&gpio3 {
+	gpio-ranges = <&pinctrl 0 96 32>;
+};
+
+&gpio4 {
+	gpio-ranges = <&pinctrl 0 128 32>;
+};
+
+&gpio6 {
+	gpio-ranges = <&pinctrl 0 192 32>;
+};
+
--
2.20.1

[PATCH v1 3/6] arm: dts: rockchip: rk3188-u-boot: add gpio-ranges

[Johan Jonker]

The gpio node names are made generic, but without
gpio bank ID. Add gpio-ranges to rk3188-u-boot.dtsi
for now till a better method is found.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 arch/arm/dts/rk3188-u-boot.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/dts/rk3188-u-boot.dtsi b/arch/arm/dts/rk3188-u-boot.dtsi
index 735776c1..176f9e65 100644
--- a/arch/arm/dts/rk3188-u-boot.dtsi
+++ b/arch/arm/dts/rk3188-u-boot.dtsi
@@ -12,6 +12,19 @@

 &gpio0 {
 	compatible = "rockchip,gpio-bank";
+	gpio-ranges = <&pinctrl 0 0 32>;
+};
+
+&gpio1 {
+	gpio-ranges = <&pinctrl 0 32 32>;
+};
+
+&gpio2 {
+	gpio-ranges = <&pinctrl 0 64 32>;
+};
+
+&gpio3 {
+	gpio-ranges = <&pinctrl 0 96 32>;
 };

 &pmu {
--
2.20.1

[PATCH v1 4/6] 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

[PATCH v1 5/6] rockchip: configs: mk808: change CONFIG_TPL_TEXT_BASE

[Johan Jonker]

Currently the Rockchip rk3066a u-boot-tpl.bin file needs
to add the characters "RK30", while the other SoCs replace
the first 4 bytes. Bring this in line with the rest by
lowering CONFIG_TPL_TEXT_BASE and update rockchip.rst
instructions.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 configs/mk808_defconfig         | 2 +-
 doc/board/rockchip/rockchip.rst | 7 +++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/configs/mk808_defconfig b/configs/mk808_defconfig
index c080706d..548df9c5 100644
--- a/configs/mk808_defconfig
+++ b/configs/mk808_defconfig
@@ -18,7 +18,7 @@ CONFIG_SPL_TEXT_BASE=0x60000000
 CONFIG_DM_RESET=y
 CONFIG_ROCKCHIP_RK3066=y
 # CONFIG_ROCKCHIP_STIMER is not set
-CONFIG_TPL_TEXT_BASE=0x10080C04
+CONFIG_TPL_TEXT_BASE=0x10080C00
 CONFIG_TPL_STACK=0x1008FFFF
 CONFIG_TARGET_MK808=y
 CONFIG_SPL_STACK_R_ADDR=0x70000000
diff --git a/doc/board/rockchip/rockchip.rst b/doc/board/rockchip/rockchip.rst
index b5563b8f..7d903946 100644
--- a/doc/board/rockchip/rockchip.rst
+++ b/doc/board/rockchip/rockchip.rst
@@ -380,9 +380,8 @@ Program with commands in a bash script ./flash.sh:

         #!/bin/sh

-        printf "RK30" > tplspl.bin
-        dd if=u-boot-tpl.bin >> tplspl.bin
-        truncate -s %2048 tplspl.bin
+        printf "RK30" | dd conv=notrunc bs=4 count=1 of=u-boot-tpl.bin
+        truncate -s %2048 u-boot-tpl.bin
         truncate -s %2048 u-boot-spl.bin
         ../tools/boot_merger --verbose config-flash.ini
         ../tools/upgrade_tool ul ./RK30xxLoader_uboot.bin
@@ -406,7 +405,7 @@ config-flash.ini:
         NUM=2
         LOADER1=FlashData
         LOADER2=FlashBoot
-        FlashData=tplspl.bin
+        FlashData=u-boot-tpl.bin
         FlashBoot=u-boot-spl.bin
         [OUTPUT]
         PATH=RK30xxLoader_uboot.bin
--
2.20.1

[PATCH v1 6/6] rockchip: configs: mk808: enable usb support

[Johan Jonker]

Enable usb support in the mk808_defconfig.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 configs/mk808_defconfig | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/configs/mk808_defconfig b/configs/mk808_defconfig
index 548df9c5..3e9bdb14 100644
--- a/configs/mk808_defconfig
+++ b/configs/mk808_defconfig
@@ -53,6 +53,9 @@ CONFIG_SYS_PBSIZE=276
 # CONFIG_BOOTM_VXWORKS is not set
 CONFIG_CMD_GPT=y
 CONFIG_CMD_MMC=y
+CONFIG_CMD_USB=y
+CONFIG_CMD_ROCKUSB=y
+CONFIG_CMD_USB_MASS_STORAGE=y
 # CONFIG_CMD_SETEXPR is not set
 CONFIG_CMD_CACHE=y
 CONFIG_CMD_TIME=y
@@ -78,6 +81,7 @@ CONFIG_TPL_SYSCON=y
 CONFIG_CLK=y
 CONFIG_SPL_CLK=y
 CONFIG_TPL_CLK=y
+CONFIG_FASTBOOT_BUF_ADDR=0x80000000
 CONFIG_ROCKCHIP_GPIO=y
 # CONFIG_SPL_DM_I2C is not set
 CONFIG_LED=y
@@ -106,6 +110,12 @@ CONFIG_TIMER=y
 CONFIG_SPL_TIMER=y
 CONFIG_TPL_TIMER=y
 CONFIG_DESIGNWARE_APB_TIMER=y
+CONFIG_USB=y
+CONFIG_USB_DWC2=y
+CONFIG_ROCKCHIP_USB2_PHY=y
+CONFIG_USB_GADGET=y
+CONFIG_USB_GADGET_DWC2_OTG=y
+CONFIG_USB_FUNCTION_ROCKUSB=y
 CONFIG_SPL_TINY_MEMSET=y
 CONFIG_ERRNO_STR=y
 # CONFIG_TPL_OF_LIBFDT is not set
--
2.20.1

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

[Simon Glass]

On Thu, 16 Mar 2023 at 10:47, Johan Jonker <jbx6244@gmail.com> 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(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v1 5/6] rockchip: configs: mk808: change CONFIG_TPL_TEXT_BASE

[Simon Glass]

On Thu, 16 Mar 2023 at 10:48, Johan Jonker <jbx6244@gmail.com> wrote:
>
> Currently the Rockchip rk3066a u-boot-tpl.bin file needs
> to add the characters "RK30", while the other SoCs replace
> the first 4 bytes. Bring this in line with the rest by
> lowering CONFIG_TPL_TEXT_BASE and update rockchip.rst
> instructions.
>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  configs/mk808_defconfig         | 2 +-
>  doc/board/rockchip/rockchip.rst | 7 +++----
>  2 files changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/configs/mk808_defconfig b/configs/mk808_defconfig
> index c080706d..548df9c5 100644
> --- a/configs/mk808_defconfig
> +++ b/configs/mk808_defconfig
> @@ -18,7 +18,7 @@ CONFIG_SPL_TEXT_BASE=0x60000000
>  CONFIG_DM_RESET=y
>  CONFIG_ROCKCHIP_RK3066=y
>  # CONFIG_ROCKCHIP_STIMER is not set
> -CONFIG_TPL_TEXT_BASE=0x10080C04
> +CONFIG_TPL_TEXT_BASE=0x10080C00

lower-case hex is better!

Re: [PATCH v1 6/6] rockchip: configs: mk808: enable usb support

[Simon Glass]

On Thu, 16 Mar 2023 at 10:48, Johan Jonker <jbx6244@gmail.com> wrote:
>
> Enable usb support in the mk808_defconfig.
>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  configs/mk808_defconfig | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Simon Glass]

Hi Johan,

On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@gmail.com> wrote:
>
> The current divider to calculate the bank ID can change.
> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.

What is the motivation for this patch?

>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  drivers/gpio/rk_gpio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
> index f7ad4d68..0a2acf18 100644
> --- a/drivers/gpio/rk_gpio.c
> +++ b/drivers/gpio/rk_gpio.c
> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>                                              0, &args);
>         if (!ret || ret != -ENOENT) {
>                 uc_priv->gpio_count = args.args[2];
> -               priv->bank = args.args[1] / args.args[2];
> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>         } else {
>                 uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>                 end = strrchr(dev->name, '@');
> --
> 2.20.1
>

Regards,
SImon

Re: [PATCH v1 2/6] arm: dts: rockchip: rk3066a-u-boot: add gpio-ranges

[Simon Glass]

On Thu, 16 Mar 2023 at 10:47, Johan Jonker <jbx6244@gmail.com> wrote:
>
> The gpio node names are made generic, but without
> gpio bank ID. Add gpio-ranges to rk3066a-u-boot.dtsi
> for now till a better method is found.
>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  arch/arm/dts/rk3066a-u-boot.dtsi | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v1 3/6] arm: dts: rockchip: rk3188-u-boot: add gpio-ranges

[Simon Glass]

On Thu, 16 Mar 2023 at 10:47, Johan Jonker <jbx6244@gmail.com> wrote:
>
> The gpio node names are made generic, but without
> gpio bank ID. Add gpio-ranges to rk3188-u-boot.dtsi
> for now till a better method is found.
>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  arch/arm/dts/rk3188-u-boot.dtsi | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Johan Jonker]

On 3/18/23 21:20, Simon Glass wrote:
> Hi Johan,
> 
> On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@gmail.com> wrote:
>>
>> The current divider to calculate the bank ID can change.
>> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
> 

> What is the motivation for this patch?

The gpio-ranges property format:

gpio-ranges = <[pin controller phandle], [GPIO controller offset],
                [pin controller offset], [number of pins]>;

1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.
2: The "gpio-ranges" syntax allows multiple items with variable number of pins.

===

Theoretical example:

gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1

vs.

gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
              <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1

Both descriptions are valid.
The number of pins in the second example is reduced to 16 per item.
Using that as divider will give a wrong bank number.
Use a constant instead.
For the Rockchip situation simple parsing the first item is enough the know it's bank number for now.
To do it correct one could parse a list of gpio-range items, but that makes it more complicated.

From gpio.txt:
Each offset runs from 0 to N. It is perfectly fine to pile any number of
ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
in practice these ranges are often lumped in discrete sets.

> 
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>  drivers/gpio/rk_gpio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
>>
>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>> index f7ad4d68..0a2acf18 100644
>> --- a/drivers/gpio/rk_gpio.c
>> +++ b/drivers/gpio/rk_gpio.c
>> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>                                              0, &args);
>>         if (!ret || ret != -ENOENT) {
>>                 uc_priv->gpio_count = args.args[2];
>> -               priv->bank = args.args[1] / args.args[2];
>> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>>         } else {
>>                 uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>                 end = strrchr(dev->name, '@');
>> --
>> 2.20.1
>>
> 
> Regards,
> SImon

Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Jonas Karlman]

Hi Johan,
On 2023-03-19 12:34, Johan Jonker wrote:
> 
> 
> On 3/18/23 21:20, Simon Glass wrote:
>> Hi Johan,
>>
>> On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@gmail.com> wrote:
>>>
>>> The current divider to calculate the bank ID can change.
>>> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
>>
> 
>> What is the motivation for this patch?
> 
> The gpio-ranges property format:
> 
> gpio-ranges = <[pin controller phandle], [GPIO controller offset],
>                 [pin controller offset], [number of pins]>;
> 
> 1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.
> 2: The "gpio-ranges" syntax allows multiple items with variable number of pins.

Is there a reason why gpio-ranges is used to determine bank id?

In the series at [1], add support for rk35xx gpio banks, sent yesterday
I changed this to rely on the dev alias id, same as in the linux driver.

Any reason why using the device alias id won't work for the same purpose
in u-boot?

[1] https://patchwork.ozlabs.org/project/uboot/patch/20230318235651.826148-3-jonas@kwiboo.se/

Regards,
Jonas

> 
> ===
> 
> Theoretical example:
> 
> gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1
> 
> vs.
> 
> gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
>               <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1
> 
> Both descriptions are valid.
> The number of pins in the second example is reduced to 16 per item.
> Using that as divider will give a wrong bank number.
> Use a constant instead.
> For the Rockchip situation simple parsing the first item is enough the know it's bank number for now.
> To do it correct one could parse a list of gpio-range items, but that makes it more complicated.
> 
> From gpio.txt:
> Each offset runs from 0 to N. It is perfectly fine to pile any number of
> ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
> in practice these ranges are often lumped in discrete sets.
> 
>>
>>>
>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>> ---
>>>  drivers/gpio/rk_gpio.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>>>
>>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>>> index f7ad4d68..0a2acf18 100644
>>> --- a/drivers/gpio/rk_gpio.c
>>> +++ b/drivers/gpio/rk_gpio.c
>>> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>                                              0, &args);
>>>         if (!ret || ret != -ENOENT) {
>>>                 uc_priv->gpio_count = args.args[2];
>>> -               priv->bank = args.args[1] / args.args[2];
>>> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>>>         } else {
>>>                 uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>>                 end = strrchr(dev->name, '@');
>>> --
>>> 2.20.1
>>>
>>
>> Regards,
>> SImon

Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Johan Jonker]

On 3/19/23 13:20, Jonas Karlman wrote:
> Hi Johan,
> On 2023-03-19 12:34, Johan Jonker wrote:
>>
>>
>> On 3/18/23 21:20, Simon Glass wrote:
>>> Hi Johan,
>>>
>>> On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@gmail.com> wrote:
>>>>
>>>> The current divider to calculate the bank ID can change.
>>>> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
>>>
>>
>>> What is the motivation for this patch?
>>
>> The gpio-ranges property format:
>>
>> gpio-ranges = <[pin controller phandle], [GPIO controller offset],
>>                 [pin controller offset], [number of pins]>;
>>
>> 1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.
>> 2: The "gpio-ranges" syntax allows multiple items with variable number of pins.
> 
> Is there a reason why gpio-ranges is used to determine bank id?
> 
> In the series at [1], add support for rk35xx gpio banks, sent yesterday
> I changed this to rely on the dev alias id, same as in the linux driver.
>
> Any reason why using the device alias id won't work for the same purpose
> in u-boot?

Aliases are board/user orientated based on availability.
See discussion elsewhere(by Arnd Bergmann and others, see lore link unknown for now)
All aliases should be moved out of the dtsi files, so there's no guaranty that they are there anymore.
Aliases should follow the Rockchip TRM naming to prevent confusion.(by Heiko)
Special rules apply to mmc aliases: based on availability, reg order and without number gap. 

U-Boot/bootloader specific:
Aliases get lost with reduced DT's and are problematic with overlays,etc(ie. it needs special handling on top of that).
Aliases are not in the dtb platdata structure made by dtoc for TPL/SPL.(current drivers don't work yet, but in the future they may)

Best is to have a property inside the node for parsing and reduced nodes.
gpio-ranges is the only option currently available.
(not saying that is perfect, but at least don't invent new properties that needs to be continuously supported as legacy)


> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230318235651.826148-3-jonas@kwiboo.se/
> 
> Regards,
> Jonas
> 
>>
>> ===
>>
>> Theoretical example:
>>
>> gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1
>>
>> vs.
>>
>> gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
>>               <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1
>>
>> Both descriptions are valid.
>> The number of pins in the second example is reduced to 16 per item.
>> Using that as divider will give a wrong bank number.
>> Use a constant instead.
>> For the Rockchip situation simple parsing the first item is enough the know it's bank number for now.
>> To do it correct one could parse a list of gpio-range items, but that makes it more complicated.
>>
>> From gpio.txt:
>> Each offset runs from 0 to N. It is perfectly fine to pile any number of
>> ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
>> in practice these ranges are often lumped in discrete sets.
>>
>>>
>>>>
>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>> ---
>>>>  drivers/gpio/rk_gpio.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>>>
>>>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>>>> index f7ad4d68..0a2acf18 100644
>>>> --- a/drivers/gpio/rk_gpio.c
>>>> +++ b/drivers/gpio/rk_gpio.c
>>>> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>>                                              0, &args);
>>>>         if (!ret || ret != -ENOENT) {
>>>>                 uc_priv->gpio_count = args.args[2];
>>>> -               priv->bank = args.args[1] / args.args[2];
>>>> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>>>>         } else {
>>>>                 uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>>>                 end = strrchr(dev->name, '@');
>>>> --
>>>> 2.20.1
>>>>
>>>
>>> Regards,
>>> SImon
> 

Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Jonas Karlman]

On 2023-03-19 13:55, Johan Jonker wrote:
> 
> 
> On 3/19/23 13:20, Jonas Karlman wrote:
>> Hi Johan,
>> On 2023-03-19 12:34, Johan Jonker wrote:
>>>
>>>
>>> On 3/18/23 21:20, Simon Glass wrote:
>>>> Hi Johan,
>>>>
>>>> On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@gmail.com> wrote:
>>>>>
>>>>> The current divider to calculate the bank ID can change.
>>>>> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
>>>>
>>>
>>>> What is the motivation for this patch?
>>>
>>> The gpio-ranges property format:
>>>
>>> gpio-ranges = <[pin controller phandle], [GPIO controller offset],
>>>                 [pin controller offset], [number of pins]>;
>>>
>>> 1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.
>>> 2: The "gpio-ranges" syntax allows multiple items with variable number of pins.
>>
>> Is there a reason why gpio-ranges is used to determine bank id?
>>
>> In the series at [1], add support for rk35xx gpio banks, sent yesterday
>> I changed this to rely on the dev alias id, same as in the linux driver.
>>
>> Any reason why using the device alias id won't work for the same purpose
>> in u-boot?
> 
> Aliases are board/user orientated based on availability.
> See discussion elsewhere(by Arnd Bergmann and others, see lore link unknown for now)
> All aliases should be moved out of the dtsi files, so there's no guaranty that they are there anymore.
> Aliases should follow the Rockchip TRM naming to prevent confusion.(by Heiko)
> Special rules apply to mmc aliases: based on availability, reg order and without number gap. 

Thanks for this information.

> 
> U-Boot/bootloader specific:
> Aliases get lost with reduced DT's and are problematic with overlays,etc(ie. it needs special handling on top of that).
> Aliases are not in the dtb platdata structure made by dtoc for TPL/SPL.(current drivers don't work yet, but in the future they may)

Aliases seem to be included in the reduced DT compiled for SPL for my
Quartz64-A board, see below. For dtb platdata I can see this as a problem,
yet the rk gpio driver does not support dtb platdata and it has spl
helper functions a board can use if SPL_OF_REAL is not an option.
Do you have plans to add platdata support to the rk gpio driver?

  model = "Pine64 RK3566 Quartz64-A Board";

  aliases {
    gpio0 = "/pinctrl/gpio@fdd60000";
    gpio3 = "/pinctrl/gpio@fe760000";
    serial2 = "/serial@fe660000";
    spi0 = "/spi@fe300000";
    mmc0 = "/mmc@fe310000";
    mmc1 = "/mmc@fe2b0000";
  };

I am fairly new to DT overlays, how is alias problematic with overlays?

> 
> Best is to have a property inside the node for parsing and reduced nodes.
> gpio-ranges is the only option currently available.
> (not saying that is perfect, but at least don't invent new properties that needs to be continuously supported as legacy)
> 

My understanding is that this is also just abusing the gpio-ranges prop.
Still not understanding the advantage compare to using the aliases.

gpio-ranges advantages:
- prop in node that gets included in dtb platdata

aliases advantages:
- already (ab)used by linux driver
- fewer lines of code in driver and u-boot.dtsi

Both options abuse the device tree, at least with aliases we abuse it
in the same way as the linux driver. Or are there patches to change
this behavior also for the linux driver?

Regards,
Jonas

> 
>>
>> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230318235651.826148-3-jonas@kwiboo.se/
>>
>> Regards,
>> Jonas
>>
>>>
>>> ===
>>>
>>> Theoretical example:
>>>
>>> gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1
>>>
>>> vs.
>>>
>>> gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
>>>               <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1
>>>
>>> Both descriptions are valid.
>>> The number of pins in the second example is reduced to 16 per item.
>>> Using that as divider will give a wrong bank number.
>>> Use a constant instead.
>>> For the Rockchip situation simple parsing the first item is enough the know it's bank number for now.
>>> To do it correct one could parse a list of gpio-range items, but that makes it more complicated.
>>>
>>> From gpio.txt:
>>> Each offset runs from 0 to N. It is perfectly fine to pile any number of
>>> ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
>>> in practice these ranges are often lumped in discrete sets.
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>>> ---
>>>>>  drivers/gpio/rk_gpio.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>
>>>>>
>>>>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>>>>> index f7ad4d68..0a2acf18 100644
>>>>> --- a/drivers/gpio/rk_gpio.c
>>>>> +++ b/drivers/gpio/rk_gpio.c
>>>>> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>>>                                              0, &args);
>>>>>         if (!ret || ret != -ENOENT) {
>>>>>                 uc_priv->gpio_count = args.args[2];
>>>>> -               priv->bank = args.args[1] / args.args[2];
>>>>> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>>>>>         } else {
>>>>>                 uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>>>>                 end = strrchr(dev->name, '@');
>>>>> --
>>>>> 2.20.1
>>>>>
>>>>
>>>> Regards,
>>>> SImon
>>

Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Jonas Karlman]

On 2023-03-19 14:51, Jonas Karlman wrote:
> On 2023-03-19 13:55, Johan Jonker wrote:
>>
>>
>> On 3/19/23 13:20, Jonas Karlman wrote:
>>> Hi Johan,
>>> On 2023-03-19 12:34, Johan Jonker wrote:
>>>>
>>>>
>>>> On 3/18/23 21:20, Simon Glass wrote:
>>>>> Hi Johan,
>>>>>
>>>>> On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@gmail.com> wrote:
>>>>>>
>>>>>> The current divider to calculate the bank ID can change.
>>>>>> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
>>>>>
>>>>
>>>>> What is the motivation for this patch?
>>>>
>>>> The gpio-ranges property format:
>>>>
>>>> gpio-ranges = <[pin controller phandle], [GPIO controller offset],
>>>>                 [pin controller offset], [number of pins]>;
>>>>
>>>> 1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.
>>>> 2: The "gpio-ranges" syntax allows multiple items with variable number of pins.
>>>
>>> Is there a reason why gpio-ranges is used to determine bank id?
>>>
>>> In the series at [1], add support for rk35xx gpio banks, sent yesterday
>>> I changed this to rely on the dev alias id, same as in the linux driver.
>>>
>>> Any reason why using the device alias id won't work for the same purpose
>>> in u-boot?
>>
>> Aliases are board/user orientated based on availability.
>> See discussion elsewhere(by Arnd Bergmann and others, see lore link unknown for now)
>> All aliases should be moved out of the dtsi files, so there's no guaranty that they are there anymore.
>> Aliases should follow the Rockchip TRM naming to prevent confusion.(by Heiko)
>> Special rules apply to mmc aliases: based on availability, reg order and without number gap. 
> 
> Thanks for this information.
> 
>>
>> U-Boot/bootloader specific:
>> Aliases get lost with reduced DT's and are problematic with overlays,etc(ie. it needs special handling on top of that).
>> Aliases are not in the dtb platdata structure made by dtoc for TPL/SPL.(current drivers don't work yet, but in the future they may)
> 
> Aliases seem to be included in the reduced DT compiled for SPL for my
> Quartz64-A board, see below. For dtb platdata I can see this as a problem,
> yet the rk gpio driver does not support dtb platdata and it has spl
> helper functions a board can use if SPL_OF_REAL is not an option.
> Do you have plans to add platdata support to the rk gpio driver?
> 
>   model = "Pine64 RK3566 Quartz64-A Board";
> 
>   aliases {
>     gpio0 = "/pinctrl/gpio@fdd60000";
>     gpio3 = "/pinctrl/gpio@fe760000";
>     serial2 = "/serial@fe660000";
>     spi0 = "/spi@fe300000";
>     mmc0 = "/mmc@fe310000";
>     mmc1 = "/mmc@fe2b0000";
>   };
> 
> I am fairly new to DT overlays, how is alias problematic with overlays?
> 
>>
>> Best is to have a property inside the node for parsing and reduced nodes.
>> gpio-ranges is the only option currently available.
>> (not saying that is perfect, but at least don't invent new properties that needs to be continuously supported as legacy)
>>
> 
> My understanding is that this is also just abusing the gpio-ranges prop.
> Still not understanding the advantage compare to using the aliases.
> 
> gpio-ranges advantages:
> - prop in node that gets included in dtb platdata
> 
> aliases advantages:
> - already (ab)used by linux driver
> - fewer lines of code in driver and u-boot.dtsi
> 
> Both options abuse the device tree, at least with aliases we abuse it
> in the same way as the linux driver. Or are there patches to change
> this behavior also for the linux driver?

I have now found your latest linux series at [2].

After reading a little bit more I can better understand the using of
gpio-ranges and that there is an end goal of moving the gpio bank nodes
outside the pinctrl node. Together with the HW limitation that a gpio
bank only can handle up to 32 pins, using the gpio-ranges makes more sense.

I will send a v2 of my series with minor changes to the probe function.

[2] https://patchwork.ozlabs.org/project/linux-gpio/list/?series=343421

Regards,
Jonas

> 
> Regards,
> Jonas
> 
>>
>>>
>>> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230318235651.826148-3-jonas@kwiboo.se/
>>>
>>> Regards,
>>> Jonas
>>>
>>>>
>>>> ===
>>>>
>>>> Theoretical example:
>>>>
>>>> gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1
>>>>
>>>> vs.
>>>>
>>>> gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
>>>>               <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1
>>>>
>>>> Both descriptions are valid.
>>>> The number of pins in the second example is reduced to 16 per item.
>>>> Using that as divider will give a wrong bank number.
>>>> Use a constant instead.
>>>> For the Rockchip situation simple parsing the first item is enough the know it's bank number for now.
>>>> To do it correct one could parse a list of gpio-range items, but that makes it more complicated.
>>>>
>>>> From gpio.txt:
>>>> Each offset runs from 0 to N. It is perfectly fine to pile any number of
>>>> ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
>>>> in practice these ranges are often lumped in discrete sets.
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>>>> ---
>>>>>>  drivers/gpio/rk_gpio.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>
>>>>>>
>>>>>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>>>>>> index f7ad4d68..0a2acf18 100644
>>>>>> --- a/drivers/gpio/rk_gpio.c
>>>>>> +++ b/drivers/gpio/rk_gpio.c
>>>>>> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>>>>                                              0, &args);
>>>>>>         if (!ret || ret != -ENOENT) {
>>>>>>                 uc_priv->gpio_count = args.args[2];
>>>>>> -               priv->bank = args.args[1] / args.args[2];
>>>>>> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>>>>>>         } else {
>>>>>>                 uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>>>>>                 end = strrchr(dev->name, '@');
>>>>>> --
>>>>>> 2.20.1
>>>>>>
>>>>>
>>>>> Regards,
>>>>> SImon
>>>
> 

Re: [PATCH v1 2/6] arm: dts: rockchip: rk3066a-u-boot: add gpio-ranges

[Jonas Karlman]

Hi Johan,

On 2023-03-16 17:47, Johan Jonker wrote:
> The gpio node names are made generic, but without
> gpio bank ID. Add gpio-ranges to rk3066a-u-boot.dtsi
> for now till a better method is found.
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  arch/arm/dts/rk3066a-u-boot.dtsi | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm/dts/rk3066a-u-boot.dtsi b/arch/arm/dts/rk3066a-u-boot.dtsi
> index bc6e609d..8d4e1ad2 100644
> --- a/arch/arm/dts/rk3066a-u-boot.dtsi
> +++ b/arch/arm/dts/rk3066a-u-boot.dtsi
> @@ -2,3 +2,28 @@
> 
>  #include "rockchip-u-boot.dtsi"
>  #include "rk3xxx-u-boot.dtsi"
> +
> +&gpio0 {
> +	gpio-ranges = <&pinctrl 0 0 32>;
> +};
> +
> +&gpio1 {
> +	gpio-ranges = <&pinctrl 0 32 32>;
> +};
> +
> +&gpio2 {
> +	gpio-ranges = <&pinctrl 0 64 32>;
> +};
> +
> +&gpio3 {
> +	gpio-ranges = <&pinctrl 0 96 32>;
> +};
> +
> +&gpio4 {
> +	gpio-ranges = <&pinctrl 0 128 32>;
> +};
> +
> +&gpio6 {
> +	gpio-ranges = <&pinctrl 0 192 32>;

I guess technically this should be <&pinctrl 0 160 16> to correctly
describe the hardware, i.e. pin 160-175 is controlled by the "gpio6"
controller. The name gpio6 should probably only be used in aliases.

I see no reason why the gpio + pinctrl drivers can't call it bank 5
instead of 6. And if that is the case I think our intended use of
gpio-ranges should be acceptable in linux and should also remove the
need for adding a new rockchip,gpio-controller prop.

Because of limitation in HW, 32 pin blocks, any other use of gpio-ranges
other than for a full 32 pin block is invalid. The gpio driver should fail
when [GPIO controller offset] != 0 or [pin controller offset] % 32 != 0
or when multiple ranges is specified.

Regards,
Jonas

> +};
> +
> --
> 2.20.1
> 

Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Kever Yang]

Hi Johan,

On 2023/3/19 19:34, Johan Jonker wrote:
>
> On 3/18/23 21:20, Simon Glass wrote:
>> Hi Johan,
>>
>> On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@gmail.com> wrote:
>>> The current divider to calculate the bank ID can change.
>>> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
>> What is the motivation for this patch?
> The gpio-ranges property format:
>
> gpio-ranges = <[pin controller phandle], [GPIO controller offset],
>                  [pin controller offset], [number of pins]>;
>
> 1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.

Could you share which TRM did you find gpio-banks is not 32 pins?

The design should be 32 pins per bank for all the SoCs, although some 
banks may not have full 32 pins,

but all the pin ID should follow the rules with 32pin per bank.


Thanks,

- Kever

> 2: The "gpio-ranges" syntax allows multiple items with variable number of pins.
>
> ===
>
> Theoretical example:
>
> gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1
>
> vs.
>
> gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
>                <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1
>
> Both descriptions are valid.
> The number of pins in the second example is reduced to 16 per item.
> Using that as divider will give a wrong bank number.
> Use a constant instead.
> For the Rockchip situation simple parsing the first item is enough the know it's bank number for now.
> To do it correct one could parse a list of gpio-range items, but that makes it more complicated.
>
> >From gpio.txt:
> Each offset runs from 0 to N. It is perfectly fine to pile any number of
> ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
> in practice these ranges are often lumped in discrete sets.
>
>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>> ---
>>>   drivers/gpio/rk_gpio.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>>> index f7ad4d68..0a2acf18 100644
>>> --- a/drivers/gpio/rk_gpio.c
>>> +++ b/drivers/gpio/rk_gpio.c
>>> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>                                               0, &args);
>>>          if (!ret || ret != -ENOENT) {
>>>                  uc_priv->gpio_count = args.args[2];
>>> -               priv->bank = args.args[1] / args.args[2];
>>> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>>>          } else {
>>>                  uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>>                  end = strrchr(dev->name, '@');
>>> --
>>> 2.20.1
>>>
>> Regards,
>> SImon

Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Johan Jonker]

On 3/20/23 02:32, Kever Yang wrote:
> Hi Johan,
> 
> On 2023/3/19 19:34, Johan Jonker wrote:
>>
>> On 3/18/23 21:20, Simon Glass wrote:
>>> Hi Johan,
>>>
>>> On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@gmail.com> wrote:
>>>> The current divider to calculate the bank ID can change.
>>>> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
>>> What is the motivation for this patch?
>> The gpio-ranges property format:
>>
>> gpio-ranges = <[pin controller phandle], [GPIO controller offset],
>>                  [pin controller offset], [number of pins]>;
>>
>> 1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.
> 

> Could you share which TRM did you find gpio-banks is not 32 pins?

px2==rk3066:
Rockchip PX2 TRM V1.0.pdf page 20
GPIO
6 groups of GPIO (GPIO0~GPIO4, GPIO6) , 32 GPIOs per group in
GPIO0~GPIO4, and 16 GPIOs in GPIO6, totally have 176 GPIOs

gpio6 is incomplete, mostly only A+B registers but never 32 pin.

===
rk3288-chapter-01-introduction.pdf page 15
GPIO
Totally 160 GPIOs

gpio0 only A+B+C 

gpio8 only A+B

===
rk3066 :
16 : https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pinctrl/rockchip/pinctrl-rk3066.c#L83

rk3288:
24: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pinctrl/rockchip/pinctrl-rk3288.c#L185
16: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pinctrl/rockchip/pinctrl-rk3288.c#L213
> 
> The design should be 32 pins per bank for all the SoCs, although some banks may not have full 32 pins,


> 
> but all the pin ID should follow the rules with 32pin per bank.

That's correct. This patch does not break that formula. Just don't use args[2].

number of pins != divider

> 
> 
> Thanks,
> 
> - Kever
> 
>> 2: The "gpio-ranges" syntax allows multiple items with variable number of pins.
>>
>> ===
>>
>> Theoretical example:
>>
>> gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1
>>
>> vs.
>>
>> gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
>>                <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1
>>
>> Both descriptions are valid.
>> The number of pins in the second example is reduced to 16 per item.
>> Using that as divider will give a wrong bank number.
>> Use a constant instead.
>> For the Rockchip situation simple parsing the first item is enough the know it's bank number for now.
>> To do it correct one could parse a list of gpio-range items, but that makes it more complicated.
>>
>> >From gpio.txt:
>> Each offset runs from 0 to N. It is perfectly fine to pile any number of
>> ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
>> in practice these ranges are often lumped in discrete sets.
>>
>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>> ---
>>>>   drivers/gpio/rk_gpio.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>>>> index f7ad4d68..0a2acf18 100644
>>>> --- a/drivers/gpio/rk_gpio.c
>>>> +++ b/drivers/gpio/rk_gpio.c
>>>> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>>                                               0, &args);
>>>>          if (!ret || ret != -ENOENT) {
>>>>                  uc_priv->gpio_count = args.args[2];
>>>> -               priv->bank = args.args[1] / args.args[2];
>>>> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>>>>          } else {
>>>>                  uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>>>                  end = strrchr(dev->name, '@');
>>>> -- 
>>>> 2.20.1
>>>>
>>> Regards,
>>> SImon