[PATCH v3 0/2] arm64: Add basic support for Kobol's Helios64

[PATCH v3 0/2] arm64: Add basic support for Kobol's Helios64

[Uwe Kleine-König]

Hello,

compared to v2 of this series (starting with Message-Id:
20201012204317.1581752-1-uwe@kleine-koenig.org) I sorted various things
as pointed out by Johan Jonker, dropped and improved a few things. I
hope I got the sorting right now.

This time I also Cc:d lkml (even though I don't think to attract someone
interested in this patch series there).

Uwe Kleine-König (2):
  dt-bindings: vendor-prefixes: Add kobol prefix
  arm64: dts: rockchip: Add basic support for Kobol's Helios64

 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 arch/arm64/boot/dts/rockchip/Makefile         |   1 +
 .../dts/rockchip/rk3399-kobol-helios64.dts    | 378 ++++++++++++++++++
 3 files changed, 381 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts

-- 
2.28.0

[PATCH v3 1/2] dt-bindings: vendor-prefixes: Add kobol prefix

[Uwe Kleine-König]

The prefix is already used in arm/armada-388-helios4.dts.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 63996ab03521..b2cbdad45846 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -545,6 +545,8 @@ patternProperties:
     description: Kionix, Inc.
   "^kobo,.*":
     description: Rakuten Kobo Inc.
+  "^kobol,.*":
+    description: Kobol Innovations Pte. Ltd.
   "^koe,.*":
     description: Kaohsiung Opto-Electronics Inc.
   "^kontron,.*":
-- 
2.28.0

[PATCH v3 2/2] arm64: dts: rockchip: Add basic support for Kobol's Helios64

[Uwe Kleine-König]

The hardware is described in detail on Kobol's wiki at
https://wiki.kobol.io/helios64/intro/.

Up to now the following peripherals are working:

 - UART
 - Micro-SD card
 - eMMC
 - ethernet port 1
 - status LED
 - temperature sensor on i2c bus 2

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 arch/arm64/boot/dts/rockchip/Makefile         |   1 +
 .../dts/rockchip/rk3399-kobol-helios64.dts    | 378 ++++++++++++++++++
 2 files changed, 379 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index b87b1f773083..ddf07c5e5f7c 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -24,6 +24,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-hugsun-x99.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-khadas-edge.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-khadas-edge-captain.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-khadas-edge-v.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-kobol-helios64.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-leez-p710.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts b/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts
new file mode 100644
index 000000000000..98a2bbed8c14
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts
@@ -0,0 +1,378 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2020 Aditya Prayoga <aditya@kobol.io>
+ */
+
+/*
+ * The Kobol Helios64 is a board designed to operate as a NAS and optionally
+ * ships with an enclosing that can host five 2.5" hard disks.
+ *
+ * See https://wiki.kobol.io/helios64/intro/ for further details.
+ */
+
+/dts-v1/;
+#include "rk3399.dtsi"
+#include "rk3399-opp.dtsi"
+
+/ {
+	model = "Kobol Helios64";
+	compatible = "kobol,helios64", "rockchip,rk3399";
+
+	avdd_1v8_s0: avdd-1v8-s0 {
+		compatible = "regulator-fixed";
+		regulator-name = "avdd_1v8_s0";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		vin-supply = <&vcc3v3_sys_s3>;
+	};
+
+	clkin_gmac: external-gmac-clock {
+		compatible = "fixed-clock";
+		clock-frequency = <125000000>;
+		clock-output-names = "clkin_gmac";
+		#clock-cells = <0>;
+	};
+
+	leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&fault_led &status_led>;
+
+		fault-led {
+			label = "helios64:red:fault";
+			gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
+			default-state = "keep";
+		};
+
+		status-led {
+			label = "helios64:green:status";
+			gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "none";
+			default-state = "on";
+		};
+	};
+
+	vcc1v8_sys_s0: vcc1v8-sys-s0 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc1v8_sys_s0";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		vin-supply = <&vcc1v8_sys_s3>;
+	};
+
+	vcc3v0_sd: vcc3v0-sd {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v0_sd";
+		regulator-boot-on;
+		regulator-min-microvolt = <3000000>;
+		regulator-max-microvolt = <3000000>;
+		vin-supply = <&vcc3v3_sys_s3>;
+	};
+
+	vcc3v3_sys_s3: vcc_lan: vcc3v3-sys-s3 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v3_sys_s3";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		vin-supply = <&vcc5v0_sys>;
+
+		regulator-state-mem {
+			regulator-on-in-suspend;
+		};
+	};
+
+	vcc5v0_sys: vcc5v0-sys {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc5v0_sys";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		vin-supply = <&vcc12v_dcin_bkup>;
+
+		regulator-state-mem {
+			regulator-on-in-suspend;
+		};
+	};
+
+	vcc12v_dcin: vcc12v-dcin {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc12v_dcin";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+	};
+
+	vcc12v_dcin_bkup: vcc12v-dcin-bkup {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc12v_dcin_bkup";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		vin-supply = <&vcc12v_dcin>;
+	};
+};
+
+/*
+ * The system doesn't run stable with cpu freq enabled, so disallow the lower
+ * frequencies until this problem is properly understood and resolved.
+ */
+&cluster0_opp {
+        /delete-node/ opp00;
+        /delete-node/ opp01;
+        /delete-node/ opp02;
+        /delete-node/ opp03;
+        /delete-node/ opp04;
+};
+&cluster1_opp {
+        /delete-node/ opp00;
+        /delete-node/ opp01;
+        /delete-node/ opp02;
+        /delete-node/ opp03;
+        /delete-node/ opp04;
+        /delete-node/ opp05;
+        /delete-node/ opp06;
+};
+
+&cpu_b0 {
+	cpu-supply = <&vdd_cpu_b>;
+};
+
+&cpu_b1 {
+	cpu-supply = <&vdd_cpu_b>;
+};
+
+&cpu_l0 {
+	cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_l1 {
+	cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_l2 {
+	cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_l3 {
+	cpu-supply = <&vdd_cpu_l>;
+};
+
+&emmc_phy {
+	status = "okay";
+};
+
+&gmac {
+	assigned-clock-parents = <&clkin_gmac>;
+	assigned-clocks = <&cru SCLK_RMII_SRC>;
+	clock_in_out = "input";
+	phy-mode = "rgmii";
+	phy-supply = <&vcc_lan>;
+	pinctrl-0 = <&rgmii_pins &rgmii_phy_reset>;
+	pinctrl-names = "default";
+	rx_delay = <0x20>;
+	snps,reset-active-low;
+	snps,reset-delays-us = <0 10000 50000>;
+	snps,reset-gpio = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>;
+	tx_delay = <0x28>;
+	status = "okay";
+};
+
+&i2c0 {
+	clock-frequency = <400000>;
+	i2c-scl-falling-time-ns = <4>;
+	i2c-scl-rising-time-ns = <168>;
+	status = "okay";
+
+	rk808: pmic@1b {
+		compatible = "rockchip,rk808";
+		reg = <0x1b>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
+		clock-output-names = "xin32k", "rk808-clkout2";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pmic_int_l>;
+		vcc1-supply = <&vcc5v0_sys>;
+		vcc2-supply = <&vcc5v0_sys>;
+		vcc3-supply = <&vcc5v0_sys>;
+		vcc4-supply = <&vcc5v0_sys>;
+		vcc6-supply = <&vcc5v0_sys>;
+		vcc7-supply = <&vcc5v0_sys>;
+		vcc8-supply = <&vcc3v3_sys_s3>;
+		vcc9-supply = <&vcc5v0_sys>;
+		vcc10-supply = <&vcc5v0_sys>;
+		vcc11-supply = <&vcc5v0_sys>;
+		vcc12-supply = <&vcc3v3_sys_s3>;
+		vddio-supply = <&vcc3v0_s3>;
+		wakeup-source;
+
+		#clock-cells = <1>;
+
+		regulators {
+			vdd_cpu_l: DCDC_REG2 {
+				regulator-name = "vdd_cpu_l";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-max-microvolt = <1350000>;
+				regulator-min-microvolt = <750000>;
+				regulator-ramp-delay = <6001>;
+
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc1v8_sys_s3: DCDC_REG4 {
+				regulator-name = "vcc1v8_sys_s3";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-max-microvolt = <1800000>;
+				regulator-min-microvolt = <1800000>;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vcc_sdio_s0: LDO_REG4 {
+				regulator-name = "vcc_sdio_s0";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3000000>;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3000000>;
+				};
+			};
+
+			vcc3v0_s3: LDO_REG8 {
+				regulator-name = "vcc3v0_s3";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-max-microvolt = <3000000>;
+				regulator-min-microvolt = <3000000>;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3000000>;
+				};
+			};
+		};
+	};
+
+	vdd_cpu_b: regulator@40 {
+		compatible = "silergy,syr827";
+		reg = <0x40>;
+		fcs,suspend-voltage-selector = <1>;
+		pinctrl-0 = <&vsel1_gpio>;
+		pinctrl-names = "default";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-max-microvolt = <1500000>;
+		regulator-min-microvolt = <712500>;
+		regulator-name = "vdd_cpu_b";
+		regulator-ramp-delay = <1000>;
+		vin-supply = <&vcc5v0_sys>;
+
+		regulator-state-mem {
+			regulator-off-in-suspend;
+		};
+	};
+};
+
+&i2c2 {
+	clock-frequency = <400000>;
+	i2c-scl-rising-time-ns = <160>;
+	i2c-scl-falling-time-ns = <30>;
+	status = "okay";
+
+	temp@4c {
+		compatible = "national,lm75";
+		reg = <0x4c>;
+	};
+};
+
+&io_domains {
+	audio-supply = <&vcc1v8_sys_s0>;
+	bt656-supply = <&vcc1v8_sys_s0>;
+	gpio1830-supply = <&vcc3v0_s3>;
+	sdmmc-supply = <&vcc_sdio_s0>;
+	status = "okay";
+};
+
+&pinctrl {
+	gmac {
+		rgmii_phy_reset: rgmii-phy-reset {
+			rockchip,pins =
+				<3 RK_PB7 RK_FUNC_GPIO &pcfg_output_low>;
+		};
+	};
+
+	leds {
+		fault_led: fault-led {
+			rockchip,pins =
+				<0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		status_led: status-led {
+			rockchip,pins =
+				<0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_down>,
+		};
+	};
+
+	pmic {
+		pmic_int_l: pmic-int-l {
+			rockchip,pins = <0 RK_PB2 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+
+		vsel1_gpio: vsel1-gpio {
+			rockchip,pins = <1 RK_PC1 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		vsel2_gpio: vsel2-gpio {
+			rockchip,pins = <1 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+	};
+};
+
+&pmu_io_domains {
+	pmu1830-supply = <&vcc3v0_s3>;
+	status = "okay";
+};
+
+&sdhci {
+	bus-width = <8>;
+	disable-wp;
+	mmc-hs200-1_8v;
+	non-removable;
+	supports-emmc;
+	vqmmc-supply = <&vcc1v8_sys_s0>;
+	status = "okay";
+};
+
+&sdmmc {
+	bus-width = <4>;
+	cap-sd-highspeed;
+	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
+	disable-wp;
+	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
+	pinctrl-names = "default";
+	vmmc-supply = <&vcc3v0_sd>;
+	vqmmc-supply = <&vcc_sdio_s0>;
+	status = "okay";
+};
+
+&uart2 {
+	status = "okay";
+};
-- 
2.28.0

Re: [PATCH v3 2/2] arm64: dts: rockchip: Add basic support for Kobol's Helios64

[Johan Jonker]

Hi Uwe,

Part 1 of 2 missing here.
Submit all patches to all maintainers and mail lists.
Don't forget robh+dt !

./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback
--nogit <patch 1> <patch2>

git send-email --suppress-cc all --annotate --to <..> --cc <..> <patch
1> <patch2>

On 10/13/20 6:13 PM, Uwe Kleine-König wrote:
> The hardware is described in detail on Kobol's wiki at
> https://wiki.kobol.io/helios64/intro/.
> 
> Up to now the following peripherals are working:
> 
>  - UART
>  - Micro-SD card
>  - eMMC
>  - ethernet port 1
>  - status LED
>  - temperature sensor on i2c bus 2
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---

Add a little change log here.

>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
>  .../dts/rockchip/rk3399-kobol-helios64.dts    | 378 ++++++++++++++++++
>  2 files changed, 379 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index b87b1f773083..ddf07c5e5f7c 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -24,6 +24,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-hugsun-x99.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-khadas-edge.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-khadas-edge-captain.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-khadas-edge-v.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-kobol-helios64.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-leez-p710.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts b/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts
> new file mode 100644
> index 000000000000..98a2bbed8c14
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts
> @@ -0,0 +1,378 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2020 Aditya Prayoga <aditya@kobol.io>
> + */
> +
> +/*
> + * The Kobol Helios64 is a board designed to operate as a NAS and optionally
> + * ships with an enclosing that can host five 2.5" hard disks.
> + *
> + * See https://wiki.kobol.io/helios64/intro/ for further details.
> + */
> +
> +/dts-v1/;
> +#include "rk3399.dtsi"
> +#include "rk3399-opp.dtsi"
> +
> +/ {
> +	model = "Kobol Helios64";
> +	compatible = "kobol,helios64", "rockchip,rk3399";
> +
> +	avdd_1v8_s0: avdd-1v8-s0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "avdd_1v8_s0";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		vin-supply = <&vcc3v3_sys_s3>;
> +	};
> +
> +	clkin_gmac: external-gmac-clock {
> +		compatible = "fixed-clock";
> +		clock-frequency = <125000000>;
> +		clock-output-names = "clkin_gmac";
> +		#clock-cells = <0>;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&fault_led &status_led>;

sort

> +
> +		fault-led {
fault_led: led-0 {}

My fault.
Change ones more...
  # The first form is preferred, but fall back to just 'led' anywhere in the
  # node name to at least catch some child nodes.
  "(^led-[0-9a-f]$|led)":

> +			label = "helios64:red:fault";
> +			gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> +			default-state = "keep";
> +		};
> +

> +		status-led {

status_led: led-1 {}

> +			label = "helios64:green:status";
> +			gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;

> +			linux,default-trigger = "none";

Don't use 'none' for mainline.

> +			default-state = "on";
> +		};
> +	};
> +
> +	vcc1v8_sys_s0: vcc1v8-sys-s0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc1v8_sys_s0";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		vin-supply = <&vcc1v8_sys_s3>;
> +	};
> +
> +	vcc3v0_sd: vcc3v0-sd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v0_sd";

Doesn't a sd card need a on/off gpio?
Could you check the schematics?

> +		regulator-boot-on;
> +		regulator-min-microvolt = <3000000>;
> +		regulator-max-microvolt = <3000000>;
> +		vin-supply = <&vcc3v3_sys_s3>;
> +	};
> +
> +	vcc3v3_sys_s3: vcc_lan: vcc3v3-sys-s3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v3_sys_s3";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vcc5v0_sys>;
> +
> +		regulator-state-mem {
> +			regulator-on-in-suspend;
> +		};
> +	};
> +
> +	vcc5v0_sys: vcc5v0-sys {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc5v0_sys";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc12v_dcin_bkup>;
> +
> +		regulator-state-mem {
> +			regulator-on-in-suspend;
> +		};
> +	};
> +
> +	vcc12v_dcin: vcc12v-dcin {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc12v_dcin";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +	};
> +
> +	vcc12v_dcin_bkup: vcc12v-dcin-bkup {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc12v_dcin_bkup";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +		vin-supply = <&vcc12v_dcin>;
> +	};
> +};
> +
> +/*
> + * The system doesn't run stable with cpu freq enabled, so disallow the lower
> + * frequencies until this problem is properly understood and resolved.
> + */
> +&cluster0_opp {
> +        /delete-node/ opp00;
> +        /delete-node/ opp01;
> +        /delete-node/ opp02;
> +        /delete-node/ opp03;
> +        /delete-node/ opp04;
> +};
> +&cluster1_opp {
> +        /delete-node/ opp00;
> +        /delete-node/ opp01;
> +        /delete-node/ opp02;
> +        /delete-node/ opp03;
> +        /delete-node/ opp04;
> +        /delete-node/ opp05;
> +        /delete-node/ opp06;
> +};
> +
> +&cpu_b0 {
> +	cpu-supply = <&vdd_cpu_b>;
> +};
> +
> +&cpu_b1 {
> +	cpu-supply = <&vdd_cpu_b>;
> +};
> +
> +&cpu_l0 {
> +	cpu-supply = <&vdd_cpu_l>;
> +};
> +
> +&cpu_l1 {
> +	cpu-supply = <&vdd_cpu_l>;
> +};
> +
> +&cpu_l2 {
> +	cpu-supply = <&vdd_cpu_l>;
> +};
> +
> +&cpu_l3 {
> +	cpu-supply = <&vdd_cpu_l>;
> +};
> +
> +&emmc_phy {
> +	status = "okay";
> +};
> +
> +&gmac {
> +	assigned-clock-parents = <&clkin_gmac>;
> +	assigned-clocks = <&cru SCLK_RMII_SRC>;
> +	clock_in_out = "input";
> +	phy-mode = "rgmii";
> +	phy-supply = <&vcc_lan>;
> +	pinctrl-0 = <&rgmii_pins &rgmii_phy_reset>;
> +	pinctrl-names = "default";
> +	rx_delay = <0x20>;
> +	snps,reset-active-low;
> +	snps,reset-delays-us = <0 10000 50000>;
> +	snps,reset-gpio = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>;
> +	tx_delay = <0x28>;
> +	status = "okay";
> +};
> +
> +&i2c0 {
> +	clock-frequency = <400000>;
> +	i2c-scl-falling-time-ns = <4>;
> +	i2c-scl-rising-time-ns = <168>;
> +	status = "okay";
> +
> +	rk808: pmic@1b {
> +		compatible = "rockchip,rk808";
> +		reg = <0x1b>;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
> +		clock-output-names = "xin32k", "rk808-clkout2";

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pmic_int_l>;

sort

> +		vcc1-supply = <&vcc5v0_sys>;
> +		vcc2-supply = <&vcc5v0_sys>;
> +		vcc3-supply = <&vcc5v0_sys>;
> +		vcc4-supply = <&vcc5v0_sys>;
> +		vcc6-supply = <&vcc5v0_sys>;
> +		vcc7-supply = <&vcc5v0_sys>;
> +		vcc8-supply = <&vcc3v3_sys_s3>;
> +		vcc9-supply = <&vcc5v0_sys>;
> +		vcc10-supply = <&vcc5v0_sys>;
> +		vcc11-supply = <&vcc5v0_sys>;
> +		vcc12-supply = <&vcc3v3_sys_s3>;
> +		vddio-supply = <&vcc3v0_s3>;
> +		wakeup-source;

> +

remove empty line

> +		#clock-cells = <1>;
> +
> +		regulators {
> +			vdd_cpu_l: DCDC_REG2 {
> +				regulator-name = "vdd_cpu_l";
> +				regulator-always-on;
> +				regulator-boot-on;

> +				regulator-max-microvolt = <1350000>;
> +				regulator-min-microvolt = <750000>;


The rest has min above max.
Exception to the sort rule, not sure what Heiko wants, but keep it every
where the same.


> +				regulator-ramp-delay = <6001>;
> +
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};
> +			};
> +
> +			vcc1v8_sys_s3: DCDC_REG4 {
> +				regulator-name = "vcc1v8_sys_s3";
> +				regulator-always-on;
> +				regulator-boot-on;

> +				regulator-max-microvolt = <1800000>;
> +				regulator-min-microvolt = <1800000>;

idem

> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1800000>;
> +				};
> +			};
> +
> +			vcc_sdio_s0: LDO_REG4 {
> +				regulator-name = "vcc_sdio_s0";
> +				regulator-always-on;
> +				regulator-boot-on;

> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3000000>;

keep

> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <3000000>;
> +				};
> +			};
> +
> +			vcc3v0_s3: LDO_REG8 {
> +				regulator-name = "vcc3v0_s3";
> +				regulator-always-on;
> +				regulator-boot-on;

> +				regulator-max-microvolt = <3000000>;
> +				regulator-min-microvolt = <3000000>;

change

> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <3000000>;
> +				};
> +			};
> +		};
> +	};
> +
> +	vdd_cpu_b: regulator@40 {
> +		compatible = "silergy,syr827";
> +		reg = <0x40>;
> +		fcs,suspend-voltage-selector = <1>;
> +		pinctrl-0 = <&vsel1_gpio>;
> +		pinctrl-names = "default";
> +		regulator-always-on;
> +		regulator-boot-on;

> +		regulator-max-microvolt = <1500000>;
> +		regulator-min-microvolt = <712500>;

change

> +		regulator-name = "vdd_cpu_b";
> +		regulator-ramp-delay = <1000>;
> +		vin-supply = <&vcc5v0_sys>;
> +
> +		regulator-state-mem {
> +			regulator-off-in-suspend;
> +		};
> +	};
> +};
> +
> +&i2c2 {
> +	clock-frequency = <400000>;

> +	i2c-scl-rising-time-ns = <160>;
> +	i2c-scl-falling-time-ns = <30>;

sort

> +	status = "okay";
> +
> +	temp@4c {
> +		compatible = "national,lm75";
> +		reg = <0x4c>;
> +	};
> +};
> +
> +&io_domains {
> +	audio-supply = <&vcc1v8_sys_s0>;
> +	bt656-supply = <&vcc1v8_sys_s0>;
> +	gpio1830-supply = <&vcc3v0_s3>;
> +	sdmmc-supply = <&vcc_sdio_s0>;
> +	status = "okay";
> +};
> +
> +&pinctrl {
> +	gmac {
> +		rgmii_phy_reset: rgmii-phy-reset {
> +			rockchip,pins =

> +				<3 RK_PB7 RK_FUNC_GPIO &pcfg_output_low>;

align on the same line similar to pmic

> +		};
> +	};
> +
> +	leds {
> +		fault_led: fault-led {
> +			rockchip,pins =

> +				<0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;

align

> +		};
> +
> +		status_led: status-led {
> +			rockchip,pins =

> +				<0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_down>,

align

> +		};
> +	};
> +
> +	pmic {
> +		pmic_int_l: pmic-int-l {
> +			rockchip,pins = <0 RK_PB2 RK_FUNC_GPIO &pcfg_pull_up>;
> +		};
> +
> +		vsel1_gpio: vsel1-gpio {
> +			rockchip,pins = <1 RK_PC1 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
> +
> +		vsel2_gpio: vsel2-gpio {
> +			rockchip,pins = <1 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
> +	};
> +};
> +
> +&pmu_io_domains {
> +	pmu1830-supply = <&vcc3v0_s3>;
> +	status = "okay";
> +};
> +
> +&sdhci {
> +	bus-width = <8>;

> +	disable-wp;

remove
not used with emmc

> +	mmc-hs200-1_8v;
> +	non-removable;

> +	supports-emmc;

remove
not a valid property for mainline

> +	vqmmc-supply = <&vcc1v8_sys_s0>;
> +	status = "okay";
> +};
> +
> +&sdmmc {
> +	bus-width = <4>;
> +	cap-sd-highspeed;

> +	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;

see regulator?

> +	disable-wp;
> +	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
> +	pinctrl-names = "default";
> +	vmmc-supply = <&vcc3v0_sd>;
> +	vqmmc-supply = <&vcc_sdio_s0>;
> +	status = "okay";
> +};
> +
> +&uart2 {
> +	status = "okay";
> +};
> 

Re: [PATCH v3 2/2] arm64: dts: rockchip: Add basic support for Kobol's Helios64

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3444 bytes --]

Hello Johan,

On 10/13/20 7:34 PM, Johan Jonker wrote:
> Part 1 of 2 missing here.

Please complain to gmail then, given that patch 1 can be found on
https://lore.kernel.org/linux-arm-kernel/20201013161340.720403-2-uwe@kleine-koenig.org/.

> Submit all patches to all maintainers and mail lists.
> Don't forget robh+dt !

I'm really surprised you insist here. In my experience Rob (with his
dt-hat on) is not interested in specifics of the machine files and he
already acked patch 1.

> Add a little change log here.

I assume you also didn't get the cover letter?

>> +		fault-led {
> fault_led: led-0 {}
> 
> My fault.
> Change ones more...
>   # The first form is preferred, but fall back to just 'led' anywhere in the
>   # node name to at least catch some child nodes.
>   "(^led-[0-9a-f]$|led)":

ok, the label isn't necessary, is it?

>> +			label = "helios64:green:status";
>> +			gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
> 
>> +			linux,default-trigger = "none";
> 
> Don't use 'none' for mainline.

Oh, I missed that. Thanks for your persistence.

>> +			default-state = "on";
>> +		};
>> +	};
>> +
>> +	vcc1v8_sys_s0: vcc1v8-sys-s0 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc1v8_sys_s0";
>> +		regulator-always-on;
>> +		regulator-boot-on;
>> +		regulator-min-microvolt = <1800000>;
>> +		regulator-max-microvolt = <1800000>;
>> +		vin-supply = <&vcc1v8_sys_s3>;
>> +	};
>> +
>> +	vcc3v0_sd: vcc3v0-sd {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc3v0_sd";
> 
> Doesn't a sd card need a on/off gpio?
> Could you check the schematics?

Hmm, there is a GPIO. I didn't consider a problem there given that the
machine logs

	[   31.708706] vcc3v0_sd: disabling

when there is no SD-card in the slot. Will investigate further.

>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pmic_int_l>;
> 
> sort

I would expect this is an exception from the sorting rule.

$ git grep pinctrl-names linus/master:arch/arm64/boot/dts/ | wc -l
1905

$ git grep -A1 pinctrl-names linus/master:arch/arm64/boot/dts/ | \
  grep pinctrl-0 | wc -l
1412

When grepping over arch/arm64/boot/dts/rockchip the numbers are
453 and 445 respectively.

Will use pinctrl-names above pinctrl-0 consistently.

>> +				regulator-max-microvolt = <1350000>;
>> +				regulator-min-microvolt = <750000>;
> 
> 
> The rest has min above max.
> Exception to the sort rule, not sure what Heiko wants, but keep it every
> where the same.

OK, most rockchip dts have min before max, will fix up.

>> +	i2c-scl-rising-time-ns = <160>;
>> +	i2c-scl-falling-time-ns = <30>;
> 
> sort

I consider it logical to have rise before fall. In 43 of 59 cases in
arch/arm64/boot/dts/rockchip/ it is done this way.

>> +	vqmmc-supply = <&vcc1v8_sys_s0>;
>> +	status = "okay";
>> +};
>> +
>> +&sdmmc {
>> +	bus-width = <4>;
>> +	cap-sd-highspeed;
> 
>> +	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
> 
> see regulator?

GPIO0_A7 is the card detect line. I don't understand your question. Is
it the same as above, i.e. that it should be possible that the SD
regulator can be disabled?

>> +	disable-wp;
>> +	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
>> +	pinctrl-names = "default";
>> +	vmmc-supply = <&vcc3v0_sd>;
>> +	vqmmc-supply = <&vcc_sdio_s0>;
>> +	status = "okay";
>> +};

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] arm64: dts: rockchip: Add basic support for Kobol's Helios64

[Johan Jonker]

Hi Uwe,

On 10/13/20 10:22 PM, Uwe Kleine-König wrote:
> Hello Johan,
>
> On 10/13/20 7:34 PM, Johan Jonker wrote:
>> Part 1 of 2 missing here.
>
> Please complain to gmail then, given that patch 1 can be found on
>
https://lore.kernel.org/linux-arm-kernel/20201013161340.720403-2-uwe@kleine-koenig.org/.

Will check my spam.

>
>> Submit all patches to all maintainers and mail lists.
>> Don't forget robh+dt !
>
> I'm really surprised you insist here. In my experience Rob (with his
> dt-hat on) is not interested in specifics of the machine files and he
> already acked patch 1.

As rule of the dumb. Keep everybody in that list informed till the last
version. What they do with is not our problem or concern.

>
>> Add a little change log here.
>
> I assume you also didn't get the cover letter?
>
>>> +		fault-led {
>> fault_led: led-0 {}
>>
>> My fault.
>> Change ones more...
>>   # The first form is preferred, but fall back to just 'led' anywhere
in the
>>   # node name to at least catch some child nodes.
>>   "(^led-[0-9a-f]$|led)":
>
> ok, the label isn't necessary, is it?

Not really, for legacy and readability reasons keep it.
Without label both in nodename and as property one has to compare pin
numbers in pinctrl to see which node belong to each other.
Some users may want to add there own functions. With already available
label they can do so.

>
>>> +			label = "helios64:green:status";
>>> +			gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
>>
>>> +			linux,default-trigger = "none";
>>
>> Don't use 'none' for mainline.
>
> Oh, I missed that. Thanks for your persistence.

No thanks.

>
>>> +			default-state = "on";
>>> +		};
>>> +	};
>>> +
>>> +	vcc1v8_sys_s0: vcc1v8-sys-s0 {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "vcc1v8_sys_s0";
>>> +		regulator-always-on;
>>> +		regulator-boot-on;
>>> +		regulator-min-microvolt = <1800000>;
>>> +		regulator-max-microvolt = <1800000>;
>>> +		vin-supply = <&vcc1v8_sys_s3>;
>>> +	};
>>> +
>>> +	vcc3v0_sd: vcc3v0-sd {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "vcc3v0_sd";
>>
>> Doesn't a sd card need a on/off gpio?
>> Could you check the schematics?
>
> Hmm, there is a GPIO. I didn't consider a problem there given that the
> machine logs
>
> 	[   31.708706] vcc3v0_sd: disabling
>
> when there is no SD-card in the slot. Will investigate further.

Let us know...

>
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&pmic_int_l>;
>>
>> sort
>
> I would expect this is an exception from the sorting rule.
>
> $ git grep pinctrl-names linus/master:arch/arm64/boot/dts/ | wc -l
> 1905
>
> $ git grep -A1 pinctrl-names linus/master:arch/arm64/boot/dts/ | \
>   grep pinctrl-0 | wc -l
> 1412
>
> When grepping over arch/arm64/boot/dts/rockchip the numbers are
> 453 and 445 respectively.
>
> Will use pinctrl-names above pinctrl-0 consistently.

That people in the past blindly followed the manufacturer dts doesn't
make it an exception to the sort rule. Whatever you do, do it consistent.

>
>>> +				regulator-max-microvolt = <1350000>;
>>> +				regulator-min-microvolt = <750000>;
>>
>>
>> The rest has min above max.
>> Exception to the sort rule, not sure what Heiko wants, but keep it every
>> where the same.
>
> OK, most rockchip dts have min before max, will fix up.
>
>>> +	i2c-scl-rising-time-ns = <160>;
>>> +	i2c-scl-falling-time-ns = <30>;
>>
>> sort
>
> I consider it logical to have rise before fall. In 43 of 59 cases in
> arch/arm64/boot/dts/rockchip/ it is done this way.

That people in the past blindly followed the manufacturer dts doesn't
make it an exception to the sort rule. Whatever you do, do it consistent.

>
>>> +	vqmmc-supply = <&vcc1v8_sys_s0>;
>>> +	status = "okay";
>>> +};
>>> +
>>> +&sdmmc {
>>> +	bus-width = <4>;
>>> +	cap-sd-highspeed;
>>
>>> +	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
>>
>> see regulator?

Little longer version:
See my question at the sd regulator.
We have a pin to detect, but not to switch the regulator.
Haven't looked at the schematics. Just a reminder to check if it's
correct. Let us know..

>
> GPIO0_A7 is the card detect line. I don't understand your question. Is
> it the same as above, i.e. that it should be possible that the SD
> regulator can be disabled?
>
>>> +	disable-wp;
>>> +	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
>>> +	pinctrl-names = "default";
>>> +	vmmc-supply = <&vcc3v0_sd>;
>>> +	vqmmc-supply = <&vcc_sdio_s0>;
>>> +	status = "okay";
>>> +};
>
> Best regards
> Uwe
>

Re: [PATCH v3 2/2] arm64: dts: rockchip: Add basic support for Kobol's Helios64

[Robin Murphy]

On 2020-10-13 23:41, Johan Jonker wrote:
> Hi Uwe,
> 
> On 10/13/20 10:22 PM, Uwe Kleine-König wrote:
>> Hello Johan,
>>
>> On 10/13/20 7:34 PM, Johan Jonker wrote:
>>> Part 1 of 2 missing here.
>>
>> Please complain to gmail then, given that patch 1 can be found on
>>
> https://lore.kernel.org/linux-arm-kernel/20201013161340.720403-2-uwe@kleine-koenig.org/.
> 
> Will check my spam.
> 
>>
>>> Submit all patches to all maintainers and mail lists.
>>> Don't forget robh+dt !
>>
>> I'm really surprised you insist here. In my experience Rob (with his
>> dt-hat on) is not interested in specifics of the machine files and he
>> already acked patch 1.
> 
> As rule of the dumb. Keep everybody in that list informed till the last
> version. What they do with is not our problem or concern.

Note that get_maintainer.pl is hardly perfect, and using a bit of 
judgement is a far better idea than zealously trusting it to be the one 
true CC list and spamming people with things they really aren't going to 
care about. I mean, I *still* occasionally get CCed on random PowerPC 
patches because I once touched a couple of couple of unrelated lines in 
one of their arch headers...

Even if you're perfectly happy to spend time scanning and deleting 
hundreds of irrelevant emails from your inbox every day, don't assume 
that everyone else feels the same way ;)

FWIW as far as lists go, if I have a good idea of who the target 
audience for a patch is then I'll only CC LKML for the sake of archiving 
if there aren't at least two other relevant lists on CC.

>>
>>> Add a little change log here.
>>
>> I assume you also didn't get the cover letter?
>>
>>>> +		fault-led {
>>> fault_led: led-0 {}
>>>
>>> My fault.
>>> Change ones more...
>>>    # The first form is preferred, but fall back to just 'led' anywhere
> in the
>>>    # node name to at least catch some child nodes.
>>>    "(^led-[0-9a-f]$|led)":
>>
>> ok, the label isn't necessary, is it?
> 
> Not really, for legacy and readability reasons keep it.
> Without label both in nodename and as property one has to compare pin
> numbers in pinctrl to see which node belong to each other.
> Some users may want to add there own functions. With already available
> label they can do so.

The LED's "label" property already canonically describes what it is. An 
unused DTS label just adds noise in the source and bloats DTBs built 
with symbols. If there really are users out there who want to use 
overlays to set a custom trigger or state for the fraction of boot time 
between the LED driver probing and when a udev rule could do it, I can't 
imagine they'd mind adding the couple of extra lines to specify the full 
node path in their overlay.

>>
>>>> +			label = "helios64:green:status";
>>>> +			gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
>>>
>>>> +			linux,default-trigger = "none";
>>>
>>> Don't use 'none' for mainline.
>>
>> Oh, I missed that. Thanks for your persistence.
> 
> No thanks.
> 
>>
>>>> +			default-state = "on";
>>>> +		};
>>>> +	};
>>>> +
>>>> +	vcc1v8_sys_s0: vcc1v8-sys-s0 {
>>>> +		compatible = "regulator-fixed";
>>>> +		regulator-name = "vcc1v8_sys_s0";
>>>> +		regulator-always-on;
>>>> +		regulator-boot-on;
>>>> +		regulator-min-microvolt = <1800000>;
>>>> +		regulator-max-microvolt = <1800000>;
>>>> +		vin-supply = <&vcc1v8_sys_s3>;
>>>> +	};
>>>> +
>>>> +	vcc3v0_sd: vcc3v0-sd {
>>>> +		compatible = "regulator-fixed";
>>>> +		regulator-name = "vcc3v0_sd";
>>>
>>> Doesn't a sd card need a on/off gpio?
>>> Could you check the schematics?
>>
>> Hmm, there is a GPIO. I didn't consider a problem there given that the
>> machine logs
>>
>> 	[   31.708706] vcc3v0_sd: disabling
>>
>> when there is no SD-card in the slot. Will investigate further.

Looking at how the regulator core handles enable pins, I'd assume that a 
regulator can always be *logically* enabled and disabled regardless of 
whether doing so will actually physically switch it. (And I guess I've 
finally disproved my own thought about whether "regulator-always-on" is 
redundant for non-switchable regulators...)

It's not absolutely *mandatory* for VMMC to be switchable - some low-end 
boards simply take it off any old 3V3 rail, however that does prevent 
power-cycling the card to reset it if it gets into a bad state, so I'd 
tend to expect a well-designed board would carry the extra few cents of 
BOM cost for a switchable regulator.

> Let us know...
> 
>>
>>>> +		pinctrl-names = "default";
>>>> +		pinctrl-0 = <&pmic_int_l>;
>>>
>>> sort
>>
>> I would expect this is an exception from the sorting rule.
>>
>> $ git grep pinctrl-names linus/master:arch/arm64/boot/dts/ | wc -l
>> 1905
>>
>> $ git grep -A1 pinctrl-names linus/master:arch/arm64/boot/dts/ | \
>>    grep pinctrl-0 | wc -l
>> 1412
>>
>> When grepping over arch/arm64/boot/dts/rockchip the numbers are
>> 453 and 445 respectively.
>>
>> Will use pinctrl-names above pinctrl-0 consistently.
> 
> That people in the past blindly followed the manufacturer dts doesn't
> make it an exception to the sort rule. Whatever you do, do it consistent.
> 
>>
>>>> +				regulator-max-microvolt = <1350000>;
>>>> +				regulator-min-microvolt = <750000>;
>>>
>>>
>>> The rest has min above max.
>>> Exception to the sort rule, not sure what Heiko wants, but keep it every
>>> where the same.
>>
>> OK, most rockchip dts have min before max, will fix up.
>>
>>>> +	i2c-scl-rising-time-ns = <160>;
>>>> +	i2c-scl-falling-time-ns = <30>;
>>>
>>> sort
>>
>> I consider it logical to have rise before fall. In 43 of 59 cases in
>> arch/arm64/boot/dts/rockchip/ it is done this way.
> 
> That people in the past blindly followed the manufacturer dts doesn't
> make it an exception to the sort rule. Whatever you do, do it consistent.
> 
>>
>>>> +	vqmmc-supply = <&vcc1v8_sys_s0>;
>>>> +	status = "okay";
>>>> +};
>>>> +
>>>> +&sdmmc {
>>>> +	bus-width = <4>;
>>>> +	cap-sd-highspeed;
>>>
>>>> +	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
>>>
>>> see regulator?
> 
> Little longer version:
> See my question at the sd regulator.
> We have a pin to detect, but not to switch the regulator.
> Haven't looked at the schematics. Just a reminder to check if it's
> correct. Let us know..

I share Uwe's befuddlement here - VMMC and CD are pretty much entirely 
unrelated. If the dedicated CD function is used its internal pullup 
depends on VQMMC, but that means you can't power down the I/O when no 
card is present, because it then can't generate an interrupt when one 
*is* inserted (or like RK3288 the CD line just floats and fires 
interrupts randomly). At least on RK3399 they moved the CD pin to a 
different I/O bank where the GPIO function is always-on regardless of 
VQMMC, so using the pin as a GPIO here allows the whole SDMMC block to 
be fully powered down when idle. None of which has anything to do with 
VMMC, which is the main power supply to the card itself and doesn't go 
anywhere near the SoC...

Robin.

>>
>> GPIO0_A7 is the card detect line. I don't understand your question. Is
>> it the same as above, i.e. that it should be possible that the SD
>> regulator can be disabled?
>>
>>>> +	disable-wp;
>>>> +	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
>>>> +	pinctrl-names = "default";
>>>> +	vmmc-supply = <&vcc3v0_sd>;
>>>> +	vqmmc-supply = <&vcc_sdio_s0>;
>>>> +	status = "okay";
>>>> +};
>>
>> Best regards
>> Uwe
>>
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>