[v3,06/13] rockchip: rk3399: Add Orangepi RK3399 support

Message ID 20190425173427.13445-7-jagan@amarulasolutions.com
State New
Headers show
Series
  • rockchip: Add new rk3399 boards
Related show

Commit Message

Jagan Teki April 25, 2019, 5:34 p.m. UTC
Add initial support for Orangepi RK3399 board.

Specification
- Rockchip RK3399
- 2GB/4GB DDR3
- 16GB eMMC
- SD card slot
- RTL8211E 1Gbps
- AP6356S WiFI/BT
- HDMI In/Out, DP, MIPI DSI/CSI
- Mini PCIe
- Sensors, Keys etc
- DC12V-2A and DC5V-2A

Commit details about Linux DTS sync:
"arm64: dts: rockchip: Add support for the Orange Pi RK3399"
(sha1: d3e71487a790979057c0fdbf32f85033639c16e6)

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 arch/arm/dts/Makefile                    |   1 +
 arch/arm/dts/rk3399-orangepi-u-boot.dtsi |   7 +
 arch/arm/dts/rk3399-orangepi.dts         | 771 +++++++++++++++++++++++
 board/rockchip/evb_rk3399/MAINTAINERS    |   7 +
 configs/orangepi-rk3399_defconfig        |  58 ++
 5 files changed, 844 insertions(+)
 create mode 100644 arch/arm/dts/rk3399-orangepi-u-boot.dtsi
 create mode 100644 arch/arm/dts/rk3399-orangepi.dts
 create mode 100644 configs/orangepi-rk3399_defconfig

Comments

Paul Kocialkowski April 26, 2019, 2:17 p.m. UTC | #1
Hi,

On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> Add initial support for Orangepi RK3399 board.
> 
> Specification
> - Rockchip RK3399
> - 2GB/4GB DDR3
> - 16GB eMMC
> - SD card slot

Looks like you're missing u-boot,dm-pre-reloc to have it working, which
will need to be introduced when moving to rk3399-u-boot.dtsi.

Cheers,

Paul

> - RTL8211E 1Gbps
> - AP6356S WiFI/BT
> - HDMI In/Out, DP, MIPI DSI/CSI
> - Mini PCIe
> - Sensors, Keys etc
> - DC12V-2A and DC5V-2A
> 
> Commit details about Linux DTS sync:
> "arm64: dts: rockchip: Add support for the Orange Pi RK3399"
> (sha1: d3e71487a790979057c0fdbf32f85033639c16e6)
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  arch/arm/dts/Makefile                    |   1 +
>  arch/arm/dts/rk3399-orangepi-u-boot.dtsi |   7 +
>  arch/arm/dts/rk3399-orangepi.dts         | 771 +++++++++++++++++++++++
>  board/rockchip/evb_rk3399/MAINTAINERS    |   7 +
>  configs/orangepi-rk3399_defconfig        |  58 ++
>  5 files changed, 844 insertions(+)
>  create mode 100644 arch/arm/dts/rk3399-orangepi-u-boot.dtsi
>  create mode 100644 arch/arm/dts/rk3399-orangepi.dts
>  create mode 100644 configs/orangepi-rk3399_defconfig
> 
> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> index 0e2ffdb87f..6d55b0caf8 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -87,6 +87,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += \
>  	rk3399-evb.dtb \
>  	rk3399-firefly.dtb \
>  	rk3399-gru-bob.dtb \
> +	rk3399-orangepi.dtb \
>  	rk3399-puma-ddr1333.dtb \
>  	rk3399-puma-ddr1600.dtb \
>  	rk3399-puma-ddr1866.dtb \
> diff --git a/arch/arm/dts/rk3399-orangepi-u-boot.dtsi b/arch/arm/dts/rk3399-orangepi-u-boot.dtsi
> new file mode 100644
> index 0000000000..236b61d78d
> --- /dev/null
> +++ b/arch/arm/dts/rk3399-orangepi-u-boot.dtsi
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Jagan Teki <jagan@amarulasolutions.com>
> + */
> +
> +#include "rk3399-u-boot.dtsi"
> +#include "rk3399-sdram-ddr3-1333.dtsi"
> diff --git a/arch/arm/dts/rk3399-orangepi.dts b/arch/arm/dts/rk3399-orangepi.dts
> new file mode 100644
> index 0000000000..cf37b96a6b
> --- /dev/null
> +++ b/arch/arm/dts/rk3399-orangepi.dts
> @@ -0,0 +1,771 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2017 Fuzhou Rockchip Electronics Co., Ltd.
> + */
> +
> +/dts-v1/;
> +
> +#include "dt-bindings/pwm/pwm.h"
> +#include "dt-bindings/input/input.h"
> +#include "rk3399.dtsi"
> +#include "rk3399-opp.dtsi"
> +
> +/ {
> +	model = "Orange Pi RK3399 Board";
> +	compatible = "rockchip,rk3399-orangepi", "rockchip,rk3399";
> +
> +	chosen {
> +		stdout-path = "serial2:1500000n8";
> +	};
> +
> +	clkin_gmac: external-gmac-clock {
> +		compatible = "fixed-clock";
> +		clock-frequency = <125000000>;
> +		clock-output-names = "clkin_gmac";
> +		#clock-cells = <0>;
> +	};
> +
> +	adc-keys {
> +		compatible = "adc-keys";
> +		io-channels = <&saradc 1>;
> +		io-channel-names = "buttons";
> +		keyup-threshold-microvolt = <1800000>;
> +		poll-interval = <100>;
> +
> +		button-up {
> +			label = "Volume Up";
> +			linux,code = <KEY_VOLUMEUP>;
> +			press-threshold-microvolt = <100000>;
> +		};
> +
> +		button-down {
> +			label = "Volume Down";
> +			linux,code = <KEY_VOLUMEDOWN>;
> +			press-threshold-microvolt = <300000>;
> +		};
> +
> +		back {
> +			label = "Back";
> +			linux,code = <KEY_BACK>;
> +			press-threshold-microvolt = <985000>;
> +		};
> +
> +		menu {
> +			label = "Menu";
> +			linux,code = <KEY_MENU>;
> +			press-threshold-microvolt = <1314000>;
> +		};
> +	};
> +
> +	dc_12v: dc-12v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "dc_12v";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +	};
> +
> +	keys: gpio-keys {
> +		compatible = "gpio-keys";
> +		autorepeat;
> +
> +		power {
> +			debounce-interval = <100>;
> +			gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>;
> +			label = "GPIO Power";
> +			linux,code = <KEY_POWER>;
> +			linux,input-type = <1>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pwr_btn>;
> +			wakeup-source;
> +		};
> +	};
> +
> +	sdio_pwrseq: sdio-pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		clocks = <&rk808 1>;
> +		clock-names = "ext_clock";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wifi_reg_on_h>;
> +		reset-gpios = <&gpio0 RK_PB2 GPIO_ACTIVE_LOW>;
> +	};
> +
> +	/* switched by pmic_sleep */
> +	vcc1v8_s3: vcca1v8_s3: vcc1v8-s3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc1v8_s3";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		vin-supply = <&vcc_1v8>;
> +	};
> +
> +	vcc3v0_sd: vcc3v0-sd {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sdmmc0_pwr_h>;
> +		regulator-boot-on;
> +		regulator-max-microvolt = <3000000>;
> +		regulator-min-microvolt = <3000000>;
> +		regulator-name = "vcc3v0_sd";
> +		vin-supply = <&vcc3v3_sys>;
> +	};
> +
> +	vcc3v3_sys: vcc3v3-sys {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v3_sys";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vcc_sys>;
> +	};
> +
> +	vcc5v0_host: vcc5v0-host-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio4 RK_PD1 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vcc5v0_host_en>;
> +		regulator-name = "vcc5v0_host";
> +		regulator-always-on;
> +		vin-supply = <&vcc_sys>;
> +	};
> +
> +	vcc5v0_typec0: vcc5v0-typec0-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio1 RK_PA3 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vcc5v0_typec0_en>;
> +		regulator-name = "vcc5v0_typec0";
> +		vin-supply = <&vcc_sys>;
> +	};
> +
> +	vcc_sys: vcc-sys {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc_sys";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&dc_12v>;
> +	};
> +
> +	vdd_log: vdd-log {
> +		compatible = "pwm-regulator";
> +		pwms = <&pwm2 0 25000 1>;
> +		regulator-name = "vdd_log";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <800000>;
> +		regulator-max-microvolt = <1400000>;
> +		vin-supply = <&vcc_sys>;
> +	};
> +};
> +
> +&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>;
> +};
> +
> +&cpu_b0 {
> +	cpu-supply = <&vdd_cpu_b>;
> +};
> +
> +&cpu_b1 {
> +	cpu-supply = <&vdd_cpu_b>;
> +};
> +
> +&emmc_phy {
> +	status = "okay";
> +};
> +
> +&gmac {
> +	assigned-clocks = <&cru SCLK_RMII_SRC>;
> +	assigned-clock-parents = <&clkin_gmac>;
> +	clock_in_out = "input";
> +	phy-supply = <&vcc3v3_s3>;
> +	phy-mode = "rgmii";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&rgmii_pins>;
> +	snps,reset-gpio = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>;
> +	snps,reset-active-low;
> +	snps,reset-delays-us = <0 10000 50000>;
> +	tx_delay = <0x28>;
> +	rx_delay = <0x11>;
> +	status = "okay";
> +};
> +
> +&gpu {
> +	mali-supply = <&vdd_gpu>;
> +	status = "okay";
> +};
> +
> +&hdmi {
> +	ddc-i2c-bus = <&i2c3>;
> +	status = "okay";
> +};
> +
> +&hdmi_sound {
> +	status = "okay";
> +};
> +
> +&i2c0 {
> +	clock-frequency = <400000>;
> +	i2c-scl-rising-time-ns = <168>;
> +	i2c-scl-falling-time-ns = <4>;
> +	status = "okay";
> +
> +	rk808: pmic@1b {
> +		compatible = "rockchip,rk808";
> +		reg = <0x1b>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <21 IRQ_TYPE_LEVEL_LOW>;
> +		#clock-cells = <1>;
> +		clock-output-names = "rtc_clko_soc", "rtc_clko_wifi";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pmic_int_l>;
> +		rockchip,system-power-controller;
> +		wakeup-source;
> +
> +		vcc1-supply = <&vcc3v3_sys>;
> +		vcc2-supply = <&vcc3v3_sys>;
> +		vcc3-supply = <&vcc3v3_sys>;
> +		vcc4-supply = <&vcc3v3_sys>;
> +		vcc6-supply = <&vcc3v3_sys>;
> +		vcc7-supply = <&vcc3v3_sys>;
> +		vcc8-supply = <&vcc3v3_sys>;
> +		vcc9-supply = <&vcc3v3_sys>;
> +		vcc10-supply = <&vcc3v3_sys>;
> +		vcc11-supply = <&vcc3v3_sys>;
> +		vcc12-supply = <&vcc3v3_sys>;
> +		vddio-supply = <&vcc_3v0>;
> +
> +		regulators {
> +			vdd_center: DCDC_REG1 {
> +				regulator-name = "vdd_center";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <700000>;
> +				regulator-max-microvolt = <1500000>;
> +				regulator-ramp-delay = <6001>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};
> +			};
> +
> +			vdd_cpu_l: DCDC_REG2 {
> +				regulator-name = "vdd_cpu_l";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <700000>;
> +				regulator-max-microvolt = <1500000>;
> +				regulator-ramp-delay = <6001>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};
> +			};
> +
> +			vcc_ddr: DCDC_REG3 {
> +				regulator-name = "vcc_ddr";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +				};
> +			};
> +
> +			vcc_1v8: DCDC_REG4 {
> +				regulator-name = "vcc_1v8";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1800000>;
> +				};
> +			};
> +
> +			vcc1v8_dvp: LDO_REG1 {
> +				regulator-name = "vcc1v8_dvp";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3400000>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};
> +			};
> +
> +			vcc3v0_tp: LDO_REG2 {
> +				regulator-name = "vcc3v0_tp";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3400000>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};
> +			};
> +
> +			vcc1v8_pmupll: LDO_REG3 {
> +				regulator-name = "vcc1v8_pmupll";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <800000>;
> +				regulator-max-microvolt = <2500000>;
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1800000>;
> +				};
> +			};
> +
> +			vcc_sdio: LDO_REG4 {
> +				regulator-name = "vcc_sdio";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3400000>;
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <3000000>;
> +				};
> +			};
> +
> +			vcca3v0_codec: LDO_REG5 {
> +				regulator-name = "vcca3v0_codec";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3400000>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};
> +			};
> +
> +			vcc_1v5: LDO_REG6 {
> +				regulator-name = "vcc_1v5";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <800000>;
> +				regulator-max-microvolt = <2500000>;
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1500000>;
> +				};
> +			};
> +
> +			vcca1v8_codec: LDO_REG7 {
> +				regulator-name = "vcca1v8_codec";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <800000>;
> +				regulator-max-microvolt = <2500000>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};
> +			};
> +
> +			vcc_3v0: LDO_REG8 {
> +				regulator-name = "vcc_3v0";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3400000>;
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <3000000>;
> +				};
> +			};
> +
> +			vcc3v3_s3: SWITCH_REG1 {
> +				regulator-name = "vcc3v3_s3";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};
> +			};
> +
> +			vcc3v3_s0: SWITCH_REG2 {
> +				regulator-name = "vcc3v3_s0";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};
> +			};
> +		};
> +	};
> +
> +	vdd_cpu_b: regulator@40 {
> +		compatible = "silergy,syr827";
> +		reg = <0x40>;
> +		fcs,suspend-voltage-selector = <1>;
> +		regulator-name = "vdd_cpu_b";
> +		regulator-min-microvolt = <712500>;
> +		regulator-max-microvolt = <1500000>;
> +		regulator-ramp-delay = <1000>;
> +		regulator-always-on;
> +		regulator-boot-on;
> +		vin-supply = <&vcc3v3_sys>;
> +
> +		regulator-state-mem {
> +			regulator-off-in-suspend;
> +		};
> +	};
> +
> +	vdd_gpu: regulator@41 {
> +		compatible = "silergy,syr828";
> +		reg = <0x41>;
> +		fcs,suspend-voltage-selector = <1>;
> +		regulator-name = "vdd_gpu";
> +		regulator-min-microvolt = <712500>;
> +		regulator-max-microvolt = <1500000>;
> +		regulator-ramp-delay = <1000>;
> +		regulator-always-on;
> +		regulator-boot-on;
> +		vin-supply = <&vcc3v3_sys>;
> +
> +		regulator-state-mem {
> +			regulator-off-in-suspend;
> +		};
> +	};
> +};
> +
> +&i2c1 {
> +	i2c-scl-rising-time-ns = <450>;
> +	i2c-scl-falling-time-ns = <15>;
> +	status = "okay";
> +};
> +
> +&i2c3 {
> +	i2c-scl-rising-time-ns = <450>;
> +	i2c-scl-falling-time-ns = <15>;
> +	status = "okay";
> +};
> +
> +&i2c4 {
> +	clock-frequency = <400000>;
> +	i2c-scl-rising-time-ns = <450>;
> +	i2c-scl-falling-time-ns = <15>;
> +	status = "okay";
> +
> +	ak09911@c {
> +		compatible = "asahi-kasei,ak09911";
> +		reg = <0x0c>;
> +		vdd-supply = <&vcc3v3_s3>;
> +	};
> +
> +	mpu6500@68 {
> +		compatible = "invensense,mpu6500";
> +		reg = <0x68>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <RK_PC6 IRQ_TYPE_EDGE_RISING>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&gsensor_int_l>;
> +		vddio-supply = <&vcc3v3_s3>;
> +	};
> +
> +	lsm6ds3@6a {
> +		compatible = "st,lsm6ds3";
> +		reg = <0x6a>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <RK_PD0 IRQ_TYPE_EDGE_RISING>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&gyr_int_l>;
> +		vdd-supply = <&vcc3v3_s3>;
> +		vddio-supply = <&vcc3v3_s3>;
> +	};
> +
> +	cm32181@10 {
> +		compatible = "capella,cm32181";
> +		reg = <0x10>;
> +		interrupt-parent = <&gpio4>;
> +		interrupts = <RK_PD0 IRQ_TYPE_EDGE_RISING>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&light_int_l>;
> +		vdd-supply = <&vcc3v3_s3>;
> +	};
> +};
> +
> +&io_domains {
> +	status = "okay";
> +	bt656-supply = <&vcc_3v0>;
> +	audio-supply = <&vcca1v8_codec>;
> +	sdmmc-supply = <&vcc_sdio>;
> +	gpio1830-supply = <&vcc_3v0>;
> +};
> +
> +&pmu_io_domains {
> +	status = "okay";
> +	pmu1830-supply = <&vcc_3v0>;
> +};
> +
> +&pinctrl {
> +	buttons {
> +		pwr_btn: pwr-btn {
> +			rockchip,pins = <0 RK_PA5 RK_FUNC_GPIO &pcfg_pull_up>;
> +		};
> +	};
> +
> +	pmic {
> +		pmic_int_l: pmic-int-l {
> +			rockchip,pins =
> +				<1 RK_PC5 RK_FUNC_GPIO &pcfg_pull_up>;
> +		};
> +	};
> +
> +	sd {
> +		sdmmc0_pwr_h: sdmmc0-pwr-h {
> +			rockchip,pins =
> +				<RK_GPIO0 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	usb2 {
> +		vcc5v0_host_en: vcc5v0-host-en {
> +			rockchip,pins =
> +				<4 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +
> +		vcc5v0_typec0_en: vcc5v0-typec0-en {
> +			rockchip,pins =
> +				<1 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	sdio-pwrseq {
> +		wifi_reg_on_h: wifi-reg-on-h {
> +			rockchip,pins = <0 RK_PB2 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	wifi {
> +		wifi_host_wake_l: wifi-host-wake-l {
> +			rockchip,pins = <0 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	bluetooth {
> +		bt_reg_on_h: bt-enable-h {
> +			rockchip,pins = <0 RK_PB1 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +
> +		bt_host_wake_l: bt-host-wake-l {
> +			rockchip,pins = <0 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +
> +		bt_wake_l: bt-wake-l {
> +			rockchip,pins = <2 RK_PD2 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	mpu6500 {
> +		gsensor_int_l: gsensor-int-l {
> +			rockchip,pins = <1 RK_PC6 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	lsm6ds3 {
> +		gyr_int_l: gyr-int-l {
> +			rockchip,pins = <1 RK_PD0 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	cm32181 {
> +		light_int_l: light-int-l {
> +			rockchip,pins = <4 RK_PD0 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +};
> +
> +&pwm0 {
> +	status = "okay";
> +};
> +
> +&pwm2 {
> +	status = "okay";
> +};
> +
> +&saradc {
> +	vref-supply = <&vcca1v8_s3>;
> +	status = "okay";
> +};
> +
> +&sdhci {
> +	bus-width = <8>;
> +	mmc-hs400-1_8v;
> +	mmc-hs400-enhanced-strobe;
> +	non-removable;
> +	status = "okay";
> +};
> +
> +&sdio0 {
> +	bus-width = <4>;
> +	cap-sd-highspeed;
> +	cap-sdio-irq;
> +	clock-frequency = <50000000>;
> +	disable-wp;
> +	keep-power-in-suspend;
> +	max-frequency = <50000000>;
> +	mmc-pwrseq = <&sdio_pwrseq>;
> +	non-removable;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sdio0_bus4 &sdio0_cmd &sdio0_clk>;
> +	sd-uhs-sdr104;
> +	status = "okay";
> +
> +	brcmf: wifi@1 {
> +		compatible = "brcm,bcm4329-fmac";
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <RK_PA3 GPIO_ACTIVE_HIGH>;
> +		interrupt-names = "host-wake";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wifi_host_wake_l>;
> +	};
> +};
> +
> +&sdmmc {
> +	bus-width = <4>;
> +	cap-mmc-highspeed;
> +	cap-sd-highspeed;
> +	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
> +	clock-frequency = <150000000>;
> +	disable-wp;
> +	max-frequency = <150000000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
> +	vmmc-supply = <&vcc3v0_sd>;
> +	vqmmc-supply = <&vcc_sdio>;
> +	status = "okay";
> +};
> +
> +&tcphy0 {
> +	status = "okay";
> +};
> +
> +&tcphy1 {
> +	status = "okay";
> +};
> +
> +&tsadc {
> +	rockchip,hw-tshut-mode = <1>;
> +	rockchip,hw-tshut-polarity = <1>;
> +	status = "okay";
> +};
> +
> +&u2phy0 {
> +	status = "okay";
> +
> +	u2phy0_otg: otg-port {
> +		phy-supply = <&vcc5v0_typec0>;
> +		status = "okay";
> +	};
> +
> +	u2phy0_host: host-port {
> +		phy-supply = <&vcc5v0_host>;
> +		status = "okay";
> +	};
> +};
> +
> +&u2phy1 {
> +	status = "okay";
> +
> +	u2phy1_otg: otg-port {
> +		status = "okay";
> +	};
> +
> +	u2phy1_host: host-port {
> +		phy-supply = <&vcc5v0_host>;
> +		status = "okay";
> +	};
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>;
> +	status = "okay";
> +
> +	bluetooth {
> +		compatible = "brcm,bcm43438-bt";
> +		clocks = <&rk808 1>;
> +		clock-names = "ext_clock";
> +		device-wakeup-gpios = <&gpio2 RK_PD2 GPIO_ACTIVE_HIGH>;
> +		host-wakeup-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_HIGH>;
> +		shutdown-gpios = <&gpio0 RK_PB1 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&bt_host_wake_l &bt_wake_l &bt_reg_on_h>;
> +	};
> +};
> +
> +&uart2 {
> +	status = "okay";
> +};
> +
> +&usb_host0_ehci {
> +	status = "okay";
> +};
> +
> +&usb_host0_ohci {
> +	status = "okay";
> +};
> +
> +&usb_host1_ehci {
> +	status = "okay";
> +};
> +
> +&usb_host1_ohci {
> +	status = "okay";
> +};
> +
> +&usbdrd3_0 {
> +	status = "okay";
> +};
> +
> +&usbdrd_dwc3_0 {
> +	status = "okay";
> +	dr_mode = "otg";
> +};
> +
> +&usbdrd3_1 {
> +	status = "okay";
> +};
> +
> +&usbdrd_dwc3_1 {
> +	status = "okay";
> +	dr_mode = "host";
> +};
> +
> +&vopb {
> +	status = "okay";
> +};
> +
> +&vopb_mmu {
> +	status = "okay";
> +};
> +
> +&vopl {
> +	status = "okay";
> +};
> +
> +&vopl_mmu {
> +	status = "okay";
> +};
> diff --git a/board/rockchip/evb_rk3399/MAINTAINERS b/board/rockchip/evb_rk3399/MAINTAINERS
> index caad30641e..07ee8ce92c 100644
> --- a/board/rockchip/evb_rk3399/MAINTAINERS
> +++ b/board/rockchip/evb_rk3399/MAINTAINERS
> @@ -5,3 +5,10 @@ F:      board/rockchip/evb_rk3399
>  F:      include/configs/evb_rk3399.h
>  F:      configs/evb-rk3399_defconfig
>  F:      configs/firefly-rk3399_defconfig
> +
> +ORANGEPI-RK3399
> +M:	Jagan Teki <jagan@amarulasolutions.com>
> +S:	Maintained
> +F:	configs/orangepi-rk3399_defconfig
> +F:	arch/arm/dts/rk3399-u-boot.dtsi
> +F:	arch/arm/dts/rk3399-orangepi-u-boot.dtsi
> diff --git a/configs/orangepi-rk3399_defconfig b/configs/orangepi-rk3399_defconfig
> new file mode 100644
> index 0000000000..deb7bc1388
> --- /dev/null
> +++ b/configs/orangepi-rk3399_defconfig
> @@ -0,0 +1,58 @@
> +CONFIG_ARM=y
> +CONFIG_ARCH_ROCKCHIP=y
> +CONFIG_SYS_TEXT_BASE=0x00200000
> +CONFIG_SPL_LIBCOMMON_SUPPORT=y
> +CONFIG_SPL_LIBGENERIC_SUPPORT=y
> +CONFIG_SYS_MALLOC_F_LEN=0x4000
> +CONFIG_ROCKCHIP_RK3399=y
> +CONFIG_ROCKCHIP_SPL_RESERVE_IRAM=0x4000
> +CONFIG_DEBUG_UART_BASE=0xFF1A0000
> +CONFIG_DEBUG_UART_CLOCK=24000000
> +CONFIG_SPL_STACK_R_ADDR=0x80000
> +CONFIG_DEBUG_UART=y
> +CONFIG_NR_DRAM_BANKS=1
> +CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-orangepi.dtb"
> +# CONFIG_DISPLAY_CPUINFO is not set
> +CONFIG_DISPLAY_BOARDINFO_LATE=y
> +CONFIG_SPL_STACK_R=y
> +CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x4000
> +CONFIG_CMD_BOOTZ=y
> +CONFIG_CMD_GPT=y
> +CONFIG_CMD_MMC=y
> +CONFIG_CMD_SF=y
> +CONFIG_CMD_USB=y
> +# CONFIG_CMD_SETEXPR is not set
> +CONFIG_CMD_TIME=y
> +CONFIG_SPL_OF_CONTROL=y
> +CONFIG_DEFAULT_DEVICE_TREE="rk3399-orangepi"
> +CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> +CONFIG_ENV_IS_IN_MMC=y
> +CONFIG_ROCKCHIP_GPIO=y
> +CONFIG_SYS_I2C_ROCKCHIP=y
> +CONFIG_MMC_DW=y
> +CONFIG_MMC_DW_ROCKCHIP=y
> +CONFIG_MMC_SDHCI=y
> +CONFIG_MMC_SDHCI_ROCKCHIP=y
> +CONFIG_DM_ETH=y
> +CONFIG_ETH_DESIGNWARE=y
> +CONFIG_GMAC_ROCKCHIP=y
> +CONFIG_PMIC_RK8XX=y
> +CONFIG_REGULATOR_PWM=y
> +CONFIG_REGULATOR_RK8XX=y
> +CONFIG_PWM_ROCKCHIP=y
> +CONFIG_BAUDRATE=1500000
> +CONFIG_DEBUG_UART_SHIFT=2
> +CONFIG_SYSRESET=y
> +CONFIG_USB=y
> +CONFIG_USB_XHCI_HCD=y
> +CONFIG_USB_XHCI_DWC3=y
> +CONFIG_USB_EHCI_HCD=y
> +CONFIG_USB_EHCI_GENERIC=y
> +CONFIG_USB_HOST_ETHER=y
> +CONFIG_USB_ETHER_ASIX=y
> +CONFIG_USB_ETHER_ASIX88179=y
> +CONFIG_USB_ETHER_MCS7830=y
> +CONFIG_USB_ETHER_RTL8152=y
> +CONFIG_USB_ETHER_SMSC95XX=y
> +CONFIG_USE_TINY_PRINTF=y
> +CONFIG_ERRNO_STR=y
Jagan Teki April 26, 2019, 2:27 p.m. UTC | #2
On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > Add initial support for Orangepi RK3399 board.
> >
> > Specification
> > - Rockchip RK3399
> > - 2GB/4GB DDR3
> > - 16GB eMMC
> > - SD card slot
>
> Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> will need to be introduced when moving to rk3399-u-boot.dtsi.

Look like you are confused or doesn't check the patch. This patch have
rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
u-boot,dm-pre-reloc for sdmmc.
Paul Kocialkowski April 26, 2019, 2:34 p.m. UTC | #3
Hi,

On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > Add initial support for Orangepi RK3399 board.
> > > 
> > > Specification
> > > - Rockchip RK3399
> > > - 2GB/4GB DDR3
> > > - 16GB eMMC
> > > - SD card slot
> > 
> > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > will need to be introduced when moving to rk3399-u-boot.dtsi.
> 
> Look like you are confused or doesn't check the patch. This patch have
> rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> u-boot,dm-pre-reloc for sdmmc.

Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
it in your second series when you add it back to rk3399-u-boot.dtsi.

It's not okay to submit the board with broken MMC support and fix it in
a subsequent series.

Cheers,

Paul
Jagan Teki April 26, 2019, 2:45 p.m. UTC | #4
On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi,
> > >
> > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > Add initial support for Orangepi RK3399 board.
> > > >
> > > > Specification
> > > > - Rockchip RK3399
> > > > - 2GB/4GB DDR3
> > > > - 16GB eMMC
> > > > - SD card slot
> > >
> > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> >
> > Look like you are confused or doesn't check the patch. This patch have
> > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > u-boot,dm-pre-reloc for sdmmc.
>
> Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> it in your second series when you add it back to rk3399-u-boot.dtsi.

Which u-boot,dm-pre-reloc are you taking about?

v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
which were used by subsequent boards on the same series.

The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
existing board dts files thought that it will include
rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1

>
> It's not okay to submit the board with broken MMC support and fix it in
> a subsequent series.

Again, nothing broken. existing boards or dts(i) files are untouched.
Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
node to make use of new boards.  and the same reused by next series
so-that I can add binman global to all rk3399 boards.

[1] https://patchwork.ozlabs.org/patch/1091534/
Paul Kocialkowski April 26, 2019, 2:54 p.m. UTC | #5
Hi,

On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > > Hi,
> > > > 
> > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > Add initial support for Orangepi RK3399 board.
> > > > > 
> > > > > Specification
> > > > > - Rockchip RK3399
> > > > > - 2GB/4GB DDR3
> > > > > - 16GB eMMC
> > > > > - SD card slot
> > > > 
> > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > 
> > > Look like you are confused or doesn't check the patch. This patch have
> > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > u-boot,dm-pre-reloc for sdmmc.
> > 
> > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > it in your second series when you add it back to rk3399-u-boot.dtsi.
> 
> Which u-boot,dm-pre-reloc are you taking about?
> 
> v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> which were used by subsequent boards on the same series.
> 
> The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> existing board dts files thought that it will include
> rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> 
> > It's not okay to submit the board with broken MMC support and fix it in
> > a subsequent series.
> 
> Again, nothing broken. existing boards or dts(i) files are untouched.
> Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> node to make use of new boards.  and the same reused by next series
> so-that I can add binman global to all rk3399 boards.

Okay I think I'm getting there. So the Orangepi uses the new scheme
(including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
other boards are still using the previous scheme after the series.

This is very confusing and I really think you should keep u-boot,dm-
pre-reloc in the orange pi dts file and then make the transition with
all the other boards. You're mixing multiple logical steps which makes
it really hard to understand what's going on.

More to that, introducing the rk3399-u-boot.dtsi is a logical step that
should be grouped with your second series, not the first one. In your
first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.

Could you respin the two series to group changes by logic changes
instead of the current state of interleaved changes?

Mixing logical changes (not to mention spreading them accross series)
is really a no-go and makes reviewing the patches very difficult, as
this thread perfectly illustrates.

It's really not against you personally, but there are such rules that
need to be followed in upstream contributions. They are at least as
essential as the underlying technical work.

Cheers,

Paul

> [1] https://patchwork.ozlabs.org/patch/1091534/
Jagan Teki April 26, 2019, 3:01 p.m. UTC | #6
On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> > On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi,
> > >
> > > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > Hi,
> > > > >
> > > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > > Add initial support for Orangepi RK3399 board.
> > > > > >
> > > > > > Specification
> > > > > > - Rockchip RK3399
> > > > > > - 2GB/4GB DDR3
> > > > > > - 16GB eMMC
> > > > > > - SD card slot
> > > > >
> > > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > >
> > > > Look like you are confused or doesn't check the patch. This patch have
> > > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > > u-boot,dm-pre-reloc for sdmmc.
> > >
> > > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > > it in your second series when you add it back to rk3399-u-boot.dtsi.
> >
> > Which u-boot,dm-pre-reloc are you taking about?
> >
> > v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> > which were used by subsequent boards on the same series.
> >
> > The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> > existing board dts files thought that it will include
> > rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> >
> > > It's not okay to submit the board with broken MMC support and fix it in
> > > a subsequent series.
> >
> > Again, nothing broken. existing boards or dts(i) files are untouched.
> > Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> > node to make use of new boards.  and the same reused by next series
> > so-that I can add binman global to all rk3399 boards.
>
> Okay I think I'm getting there. So the Orangepi uses the new scheme
> (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
> other boards are still using the previous scheme after the series.
>
> This is very confusing and I really think you should keep u-boot,dm-
> pre-reloc in the orange pi dts file and then make the transition with
> all the other boards. You're mixing multiple logical steps which makes
> it really hard to understand what's going on.
>
> More to that, introducing the rk3399-u-boot.dtsi is a logical step that
> should be grouped with your second series, not the first one. In your
> first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
>
> Could you respin the two series to group changes by logic changes
> instead of the current state of interleaved changes?

Okay, to make it clear I can send v4 with v3.1 included and updated
commit log. but I the new series about binman remains same since the
changes on first patch on the series are relevant.

Jagan.
Paul Kocialkowski April 26, 2019, 3:12 p.m. UTC | #7
Hi,

On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
> On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> > > On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > > Hi,
> > > > 
> > > > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > > > Add initial support for Orangepi RK3399 board.
> > > > > > > 
> > > > > > > Specification
> > > > > > > - Rockchip RK3399
> > > > > > > - 2GB/4GB DDR3
> > > > > > > - 16GB eMMC
> > > > > > > - SD card slot
> > > > > > 
> > > > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > > > 
> > > > > Look like you are confused or doesn't check the patch. This patch have
> > > > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > > > u-boot,dm-pre-reloc for sdmmc.
> > > > 
> > > > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > > > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > > > it in your second series when you add it back to rk3399-u-boot.dtsi.
> > > 
> > > Which u-boot,dm-pre-reloc are you taking about?
> > > 
> > > v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> > > which were used by subsequent boards on the same series.
> > > 
> > > The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> > > existing board dts files thought that it will include
> > > rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> > > 
> > > > It's not okay to submit the board with broken MMC support and fix it in
> > > > a subsequent series.
> > > 
> > > Again, nothing broken. existing boards or dts(i) files are untouched.
> > > Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> > > node to make use of new boards.  and the same reused by next series
> > > so-that I can add binman global to all rk3399 boards.
> > 
> > Okay I think I'm getting there. So the Orangepi uses the new scheme
> > (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
> > other boards are still using the previous scheme after the series.
> > 
> > This is very confusing and I really think you should keep u-boot,dm-
> > pre-reloc in the orange pi dts file and then make the transition with
> > all the other boards. You're mixing multiple logical steps which makes
> > it really hard to understand what's going on.
> > 
> > More to that, introducing the rk3399-u-boot.dtsi is a logical step that
> > should be grouped with your second series, not the first one. In your
> > first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
> > 
> > Could you respin the two series to group changes by logic changes
> > instead of the current state of interleaved changes?
> 
> Okay, to make it clear I can send v4 with v3.1 included and updated
> commit log. but I the new series about binman remains same since the
> changes on first patch on the series are relevant.

I think it makes no sense to introduce rk3399-u-boot.dts as part of the
series currently in v3. You need to remove the patch (currently v3.1)
from this series and group it with the new series (binman-related).

Otherwise, you're mixing two unrelated logical changes across two
series.

Cheers,

Paul
Jagan Teki April 26, 2019, 3:18 p.m. UTC | #8
On Fri, Apr 26, 2019 at 8:42 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
> > On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi,
> > >
> > > On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> > > > On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > Hi,
> > > > >
> > > > > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > > > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > > > > Add initial support for Orangepi RK3399 board.
> > > > > > > >
> > > > > > > > Specification
> > > > > > > > - Rockchip RK3399
> > > > > > > > - 2GB/4GB DDR3
> > > > > > > > - 16GB eMMC
> > > > > > > > - SD card slot
> > > > > > >
> > > > > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > > > >
> > > > > > Look like you are confused or doesn't check the patch. This patch have
> > > > > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > > > > u-boot,dm-pre-reloc for sdmmc.
> > > > >
> > > > > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > > > > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > > > > it in your second series when you add it back to rk3399-u-boot.dtsi.
> > > >
> > > > Which u-boot,dm-pre-reloc are you taking about?
> > > >
> > > > v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> > > > which were used by subsequent boards on the same series.
> > > >
> > > > The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> > > > existing board dts files thought that it will include
> > > > rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> > > >
> > > > > It's not okay to submit the board with broken MMC support and fix it in
> > > > > a subsequent series.
> > > >
> > > > Again, nothing broken. existing boards or dts(i) files are untouched.
> > > > Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> > > > node to make use of new boards.  and the same reused by next series
> > > > so-that I can add binman global to all rk3399 boards.
> > >
> > > Okay I think I'm getting there. So the Orangepi uses the new scheme
> > > (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
> > > other boards are still using the previous scheme after the series.
> > >
> > > This is very confusing and I really think you should keep u-boot,dm-
> > > pre-reloc in the orange pi dts file and then make the transition with
> > > all the other boards. You're mixing multiple logical steps which makes
> > > it really hard to understand what's going on.
> > >
> > > More to that, introducing the rk3399-u-boot.dtsi is a logical step that
> > > should be grouped with your second series, not the first one. In your
> > > first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
> > >
> > > Could you respin the two series to group changes by logic changes
> > > instead of the current state of interleaved changes?
> >
> > Okay, to make it clear I can send v4 with v3.1 included and updated
> > commit log. but I the new series about binman remains same since the
> > changes on first patch on the series are relevant.
>
> I think it makes no sense to introduce rk3399-u-boot.dts as part of the
> series currently in v3. You need to remove the patch (currently v3.1)
> from this series and group it with the new series (binman-related).
>
> Otherwise, you're mixing two unrelated logical changes across two
> series.

rk3399-u-boot.dtsi is require for sdboot on that boards, whole idea is
to untouch Linux dts files and get introduced initial
rk3399-u-boot.dtsi which doesn't effect any board dts files but use
new boards.

rk3399-u-boot.dtsi is required for binman for adding binman changes,
and indeed binman series has some issues which can take time to merge
and other one can do it before.
Paul Kocialkowski April 26, 2019, 3:38 p.m. UTC | #9
Hi,

Le vendredi 26 avril 2019 à 20:48 +0530, Jagan Teki a écrit :
> On Fri, Apr 26, 2019 at 8:42 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
> > > On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > > Hi,
> > > > 
> > > > On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> > > > > On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > > > > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > > > > > Add initial support for Orangepi RK3399 board.
> > > > > > > > > 
> > > > > > > > > Specification
> > > > > > > > > - Rockchip RK3399
> > > > > > > > > - 2GB/4GB DDR3
> > > > > > > > > - 16GB eMMC
> > > > > > > > > - SD card slot
> > > > > > > > 
> > > > > > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > > > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > > > > > 
> > > > > > > Look like you are confused or doesn't check the patch. This patch have
> > > > > > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > > > > > u-boot,dm-pre-reloc for sdmmc.
> > > > > > 
> > > > > > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > > > > > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > > > > > it in your second series when you add it back to rk3399-u-boot.dtsi.
> > > > > 
> > > > > Which u-boot,dm-pre-reloc are you taking about?
> > > > > 
> > > > > v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> > > > > which were used by subsequent boards on the same series.
> > > > > 
> > > > > The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> > > > > existing board dts files thought that it will include
> > > > > rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> > > > > 
> > > > > > It's not okay to submit the board with broken MMC support and fix it in
> > > > > > a subsequent series.
> > > > > 
> > > > > Again, nothing broken. existing boards or dts(i) files are untouched.
> > > > > Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> > > > > node to make use of new boards.  and the same reused by next series
> > > > > so-that I can add binman global to all rk3399 boards.
> > > > 
> > > > Okay I think I'm getting there. So the Orangepi uses the new scheme
> > > > (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
> > > > other boards are still using the previous scheme after the series.
> > > > 
> > > > This is very confusing and I really think you should keep u-boot,dm-
> > > > pre-reloc in the orange pi dts file and then make the transition with
> > > > all the other boards. You're mixing multiple logical steps which makes
> > > > it really hard to understand what's going on.
> > > > 
> > > > More to that, introducing the rk3399-u-boot.dtsi is a logical step that
> > > > should be grouped with your second series, not the first one. In your
> > > > first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
> > > > 
> > > > Could you respin the two series to group changes by logic changes
> > > > instead of the current state of interleaved changes?
> > > 
> > > Okay, to make it clear I can send v4 with v3.1 included and updated
> > > commit log. but I the new series about binman remains same since the
> > > changes on first patch on the series are relevant.
> > 
> > I think it makes no sense to introduce rk3399-u-boot.dts as part of the
> > series currently in v3. You need to remove the patch (currently v3.1)
> > from this series and group it with the new series (binman-related).
> > 
> > Otherwise, you're mixing two unrelated logical changes across two
> > series.
> 
> rk3399-u-boot.dtsi is require for sdboot on that boards, whole idea is
> to untouch Linux dts files and get introduced initial
> rk3399-u-boot.dtsi which doesn't effect any board dts files but use
> new boards.

Yes I think I properly understand that now. The problem is that
currently, rk3399 boards do not use rk3399-u-boot.dtsi. In the first
series, you are introducing the file and including it in the newly-
introduced devices, but not in the previous ones. This is inconsistent.

Then, in the second series, you are moving existing boards to using
rk3399-u-boot.dtsi, except for the boards you introduced in the first
series. This is inconsistent.

Do you see the problem? I feel like you are missing it and only
focusing on the fact that the series works in practice, which is not
the issue.

> rk3399-u-boot.dtsi is required for binman for adding binman changes,
> and indeed binman series has some issues which can take time to merge
> and other one can do it before.

Yes, it can be done before in the first series (so things will not
break), but it should not be done that way for the reason I'm talking
about. Again, the way changes are split and organized is as import as
the technical changes. It's not sufficient that the series works, it
also has to make sense by grouping logical changes.

Cheers,

Paul
Jagan Teki April 26, 2019, 3:48 p.m. UTC | #10
On Fri, Apr 26, 2019 at 9:08 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> Le vendredi 26 avril 2019 à 20:48 +0530, Jagan Teki a écrit :
> > On Fri, Apr 26, 2019 at 8:42 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi,
> > >
> > > On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
> > > > On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
> > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > Hi,
> > > > >
> > > > > On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> > > > > > On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > > > > > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > > > > > > Add initial support for Orangepi RK3399 board.
> > > > > > > > > >
> > > > > > > > > > Specification
> > > > > > > > > > - Rockchip RK3399
> > > > > > > > > > - 2GB/4GB DDR3
> > > > > > > > > > - 16GB eMMC
> > > > > > > > > > - SD card slot
> > > > > > > > >
> > > > > > > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > > > > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > > > > > >
> > > > > > > > Look like you are confused or doesn't check the patch. This patch have
> > > > > > > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > > > > > > u-boot,dm-pre-reloc for sdmmc.
> > > > > > >
> > > > > > > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > > > > > > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > > > > > > it in your second series when you add it back to rk3399-u-boot.dtsi.
> > > > > >
> > > > > > Which u-boot,dm-pre-reloc are you taking about?
> > > > > >
> > > > > > v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> > > > > > which were used by subsequent boards on the same series.
> > > > > >
> > > > > > The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> > > > > > existing board dts files thought that it will include
> > > > > > rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> > > > > >
> > > > > > > It's not okay to submit the board with broken MMC support and fix it in
> > > > > > > a subsequent series.
> > > > > >
> > > > > > Again, nothing broken. existing boards or dts(i) files are untouched.
> > > > > > Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> > > > > > node to make use of new boards.  and the same reused by next series
> > > > > > so-that I can add binman global to all rk3399 boards.
> > > > >
> > > > > Okay I think I'm getting there. So the Orangepi uses the new scheme
> > > > > (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
> > > > > other boards are still using the previous scheme after the series.
> > > > >
> > > > > This is very confusing and I really think you should keep u-boot,dm-
> > > > > pre-reloc in the orange pi dts file and then make the transition with
> > > > > all the other boards. You're mixing multiple logical steps which makes
> > > > > it really hard to understand what's going on.
> > > > >
> > > > > More to that, introducing the rk3399-u-boot.dtsi is a logical step that
> > > > > should be grouped with your second series, not the first one. In your
> > > > > first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
> > > > >
> > > > > Could you respin the two series to group changes by logic changes
> > > > > instead of the current state of interleaved changes?
> > > >
> > > > Okay, to make it clear I can send v4 with v3.1 included and updated
> > > > commit log. but I the new series about binman remains same since the
> > > > changes on first patch on the series are relevant.
> > >
> > > I think it makes no sense to introduce rk3399-u-boot.dts as part of the
> > > series currently in v3. You need to remove the patch (currently v3.1)
> > > from this series and group it with the new series (binman-related).
> > >
> > > Otherwise, you're mixing two unrelated logical changes across two
> > > series.
> >
> > rk3399-u-boot.dtsi is require for sdboot on that boards, whole idea is
> > to untouch Linux dts files and get introduced initial
> > rk3399-u-boot.dtsi which doesn't effect any board dts files but use
> > new boards.
>
> Yes I think I properly understand that now. The problem is that
> currently, rk3399 boards do not use rk3399-u-boot.dtsi. In the first
> series, you are introducing the file and including it in the newly-
> introduced devices, but not in the previous ones. This is inconsistent.
>
> Then, in the second series, you are moving existing boards to using
> rk3399-u-boot.dtsi, except for the boards you introduced in the first
> series. This is inconsistent.
>
> Do you see the problem? I feel like you are missing it and only
> focusing on the fact that the series works in practice, which is not
> the issue.

Not again.

First series, I get introduced rk3399-u-boot.dtsi, and only the new
boards are using this file and next series rest of the boards are
using which indeed a valid step. what is inconsistent here, I don't
understand.

>
> > rk3399-u-boot.dtsi is required for binman for adding binman changes,
> > and indeed binman series has some issues which can take time to merge
> > and other one can do it before.
>
> Yes, it can be done before in the first series (so things will not
> break), but it should not be done that way for the reason I'm talking
> about. Again, the way changes are split and organized is as import as
> the technical changes. It's not sufficient that the series works, it
> also has to make sense by grouping logical changes.

Don't agree your point, let me send v4 will see what Philipp and other
can think off.

thanks for your comments,
Jagan.
Paul Kocialkowski April 26, 2019, 4:08 p.m. UTC | #11
Hi,

Le vendredi 26 avril 2019 à 21:18 +0530, Jagan Teki a écrit :
> On Fri, Apr 26, 2019 at 9:08 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > Le vendredi 26 avril 2019 à 20:48 +0530, Jagan Teki a écrit :
> > > On Fri, Apr 26, 2019 at 8:42 PM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > > Hi,
> > > > 
> > > > On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
> > > > > On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
> > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> > > > > > > On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > > > > > > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > > > > > > > Add initial support for Orangepi RK3399 board.
> > > > > > > > > > > 
> > > > > > > > > > > Specification
> > > > > > > > > > > - Rockchip RK3399
> > > > > > > > > > > - 2GB/4GB DDR3
> > > > > > > > > > > - 16GB eMMC
> > > > > > > > > > > - SD card slot
> > > > > > > > > > 
> > > > > > > > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > > > > > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > > > > > > > 
> > > > > > > > > Look like you are confused or doesn't check the patch. This patch have
> > > > > > > > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > > > > > > > u-boot,dm-pre-reloc for sdmmc.
> > > > > > > > 
> > > > > > > > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > > > > > > > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > > > > > > > it in your second series when you add it back to rk3399-u-boot.dtsi.
> > > > > > > 
> > > > > > > Which u-boot,dm-pre-reloc are you taking about?
> > > > > > > 
> > > > > > > v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> > > > > > > which were used by subsequent boards on the same series.
> > > > > > > 
> > > > > > > The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> > > > > > > existing board dts files thought that it will include
> > > > > > > rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> > > > > > > 
> > > > > > > > It's not okay to submit the board with broken MMC support and fix it in
> > > > > > > > a subsequent series.
> > > > > > > 
> > > > > > > Again, nothing broken. existing boards or dts(i) files are untouched.
> > > > > > > Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> > > > > > > node to make use of new boards.  and the same reused by next series
> > > > > > > so-that I can add binman global to all rk3399 boards.
> > > > > > 
> > > > > > Okay I think I'm getting there. So the Orangepi uses the new scheme
> > > > > > (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
> > > > > > other boards are still using the previous scheme after the series.
> > > > > > 
> > > > > > This is very confusing and I really think you should keep u-boot,dm-
> > > > > > pre-reloc in the orange pi dts file and then make the transition with
> > > > > > all the other boards. You're mixing multiple logical steps which makes
> > > > > > it really hard to understand what's going on.
> > > > > > 
> > > > > > More to that, introducing the rk3399-u-boot.dtsi is a logical step that
> > > > > > should be grouped with your second series, not the first one. In your
> > > > > > first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
> > > > > > 
> > > > > > Could you respin the two series to group changes by logic changes
> > > > > > instead of the current state of interleaved changes?
> > > > > 
> > > > > Okay, to make it clear I can send v4 with v3.1 included and updated
> > > > > commit log. but I the new series about binman remains same since the
> > > > > changes on first patch on the series are relevant.
> > > > 
> > > > I think it makes no sense to introduce rk3399-u-boot.dts as part of the
> > > > series currently in v3. You need to remove the patch (currently v3.1)
> > > > from this series and group it with the new series (binman-related).
> > > > 
> > > > Otherwise, you're mixing two unrelated logical changes across two
> > > > series.
> > > 
> > > rk3399-u-boot.dtsi is require for sdboot on that boards, whole idea is
> > > to untouch Linux dts files and get introduced initial
> > > rk3399-u-boot.dtsi which doesn't effect any board dts files but use
> > > new boards.
> > 
> > Yes I think I properly understand that now. The problem is that
> > currently, rk3399 boards do not use rk3399-u-boot.dtsi. In the first
> > series, you are introducing the file and including it in the newly-
> > introduced devices, but not in the previous ones. This is inconsistent.
> > 
> > Then, in the second series, you are moving existing boards to using
> > rk3399-u-boot.dtsi, except for the boards you introduced in the first
> > series. This is inconsistent.
> > 
> > Do you see the problem? I feel like you are missing it and only
> > focusing on the fact that the series works in practice, which is not
> > the issue.
> 
> Not again.
> 
> First series, I get introduced rk3399-u-boot.dtsi, and only the new
> boards are using this file and next series rest of the boards are
> using which indeed a valid step. what is inconsistent here, I don't
> understand.

Yes, what you are describing is exactly the issue. It does not make
sense to introduce a new common u-boot dtsi for rk3399 and add new
devices that use this file *before* switching existing devices to the
new common u-boot dtsi for rk3399 in the same series.

Globally with your series, you are doing 3 things:
- Introducing a common RK3399 u-boot dtsi with the required pre-reloc
entries;
- Adding new RK3399 devices and syncing the dtsi files from Linux;
- Introducing u-boot-rockchip-with-spl.bin.

So it should be 3 distinct series, not two. As you can see, you can
either do step 1 and then step 2 or step 2 and then step 1, but you
cannot mix parts of step 1 into step 2 and vice-versa.

Is it clearer explained this way?

Cheers,

Paul

> > > rk3399-u-boot.dtsi is required for binman for adding binman changes,
> > > and indeed binman series has some issues which can take time to merge
> > > and other one can do it before.
> > 
> > Yes, it can be done before in the first series (so things will not
> > break), but it should not be done that way for the reason I'm talking
> > about. Again, the way changes are split and organized is as import as
> > the technical changes. It's not sufficient that the series works, it
> > also has to make sense by grouping logical changes.
> 
> Don't agree your point, let me send v4 will see what Philipp and other
> can think off.
> 
> thanks for your comments,
> Jagan.
Philipp Tomsich April 26, 2019, 4:09 p.m. UTC | #12
> On 26.04.2019, at 18:08, Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:
> 
> Hi,
> 
> Le vendredi 26 avril 2019 à 21:18 +0530, Jagan Teki a écrit :
>> On Fri, Apr 26, 2019 at 9:08 PM Paul Kocialkowski
>> <paul.kocialkowski@bootlin.com> wrote:
>>> Hi,
>>> 
>>> Le vendredi 26 avril 2019 à 20:48 +0530, Jagan Teki a écrit :
>>>> On Fri, Apr 26, 2019 at 8:42 PM Paul Kocialkowski
>>>> <paul.kocialkowski@bootlin.com> wrote:
>>>>> Hi,
>>>>> 
>>>>> On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
>>>>>> On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
>>>>>> <paul.kocialkowski@bootlin.com> wrote:
>>>>>>> Hi,
>>>>>>> 
>>>>>>> On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
>>>>>>>> On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
>>>>>>>> <paul.kocialkowski@bootlin.com> wrote:
>>>>>>>>> Hi,
>>>>>>>>> 
>>>>>>>>> On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
>>>>>>>>>> On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
>>>>>>>>>> <paul.kocialkowski@bootlin.com> wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>> 
>>>>>>>>>>> On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
>>>>>>>>>>>> Add initial support for Orangepi RK3399 board.
>>>>>>>>>>>> 
>>>>>>>>>>>> Specification
>>>>>>>>>>>> - Rockchip RK3399
>>>>>>>>>>>> - 2GB/4GB DDR3
>>>>>>>>>>>> - 16GB eMMC
>>>>>>>>>>>> - SD card slot
>>>>>>>>>>> 
>>>>>>>>>>> Looks like you're missing u-boot,dm-pre-reloc to have it working, which
>>>>>>>>>>> will need to be introduced when moving to rk3399-u-boot.dtsi.
>>>>>>>>>> 
>>>>>>>>>> Look like you are confused or doesn't check the patch. This patch have
>>>>>>>>>> rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
>>>>>>>>>> u-boot,dm-pre-reloc for sdmmc.
>>>>>>>>> 
>>>>>>>>> Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
>>>>>>>>> rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
>>>>>>>>> it in your second series when you add it back to rk3399-u-boot.dtsi.
>>>>>>>> 
>>>>>>>> Which u-boot,dm-pre-reloc are you taking about?
>>>>>>>> 
>>>>>>>> v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
>>>>>>>> which were used by subsequent boards on the same series.
>>>>>>>> 
>>>>>>>> The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
>>>>>>>> existing board dts files thought that it will include
>>>>>>>> rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
>>>>>>>> 
>>>>>>>>> It's not okay to submit the board with broken MMC support and fix it in
>>>>>>>>> a subsequent series.
>>>>>>>> 
>>>>>>>> Again, nothing broken. existing boards or dts(i) files are untouched.
>>>>>>>> Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
>>>>>>>> node to make use of new boards.  and the same reused by next series
>>>>>>>> so-that I can add binman global to all rk3399 boards.
>>>>>>> 
>>>>>>> Okay I think I'm getting there. So the Orangepi uses the new scheme
>>>>>>> (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
>>>>>>> other boards are still using the previous scheme after the series.
>>>>>>> 
>>>>>>> This is very confusing and I really think you should keep u-boot,dm-
>>>>>>> pre-reloc in the orange pi dts file and then make the transition with
>>>>>>> all the other boards. You're mixing multiple logical steps which makes
>>>>>>> it really hard to understand what's going on.
>>>>>>> 
>>>>>>> More to that, introducing the rk3399-u-boot.dtsi is a logical step that
>>>>>>> should be grouped with your second series, not the first one. In your
>>>>>>> first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
>>>>>>> 
>>>>>>> Could you respin the two series to group changes by logic changes
>>>>>>> instead of the current state of interleaved changes?
>>>>>> 
>>>>>> Okay, to make it clear I can send v4 with v3.1 included and updated
>>>>>> commit log. but I the new series about binman remains same since the
>>>>>> changes on first patch on the series are relevant.
>>>>> 
>>>>> I think it makes no sense to introduce rk3399-u-boot.dts as part of the
>>>>> series currently in v3. You need to remove the patch (currently v3.1)
>>>>> from this series and group it with the new series (binman-related).
>>>>> 
>>>>> Otherwise, you're mixing two unrelated logical changes across two
>>>>> series.
>>>> 
>>>> rk3399-u-boot.dtsi is require for sdboot on that boards, whole idea is
>>>> to untouch Linux dts files and get introduced initial
>>>> rk3399-u-boot.dtsi which doesn't effect any board dts files but use
>>>> new boards.
>>> 
>>> Yes I think I properly understand that now. The problem is that
>>> currently, rk3399 boards do not use rk3399-u-boot.dtsi. In the first
>>> series, you are introducing the file and including it in the newly-
>>> introduced devices, but not in the previous ones. This is inconsistent.
>>> 
>>> Then, in the second series, you are moving existing boards to using
>>> rk3399-u-boot.dtsi, except for the boards you introduced in the first
>>> series. This is inconsistent.
>>> 
>>> Do you see the problem? I feel like you are missing it and only
>>> focusing on the fact that the series works in practice, which is not
>>> the issue.
>> 
>> Not again.
>> 
>> First series, I get introduced rk3399-u-boot.dtsi, and only the new
>> boards are using this file and next series rest of the boards are
>> using which indeed a valid step. what is inconsistent here, I don't
>> understand.
> 
> Yes, what you are describing is exactly the issue. It does not make
> sense to introduce a new common u-boot dtsi for rk3399 and add new
> devices that use this file *before* switching existing devices to the
> new common u-boot dtsi for rk3399 in the same series.

I am with Paul on this one.

Thanks,
Philipp.

> 
> Globally with your series, you are doing 3 things:
> - Introducing a common RK3399 u-boot dtsi with the required pre-reloc
> entries;
> - Adding new RK3399 devices and syncing the dtsi files from Linux;
> - Introducing u-boot-rockchip-with-spl.bin.
> 
> So it should be 3 distinct series, not two. As you can see, you can
> either do step 1 and then step 2 or step 2 and then step 1, but you
> cannot mix parts of step 1 into step 2 and vice-versa.
> 
> Is it clearer explained this way?
> 
> Cheers,
> 
> Paul
> 
>>>> rk3399-u-boot.dtsi is required for binman for adding binman changes,
>>>> and indeed binman series has some issues which can take time to merge
>>>> and other one can do it before.
>>> 
>>> Yes, it can be done before in the first series (so things will not
>>> break), but it should not be done that way for the reason I'm talking
>>> about. Again, the way changes are split and organized is as import as
>>> the technical changes. It's not sufficient that the series works, it
>>> also has to make sense by grouping logical changes.
>> 
>> Don't agree your point, let me send v4 will see what Philipp and other
>> can think off.
>> 
>> thanks for your comments,
>> Jagan.
Jagan Teki April 26, 2019, 4:16 p.m. UTC | #13
On Fri, Apr 26, 2019 at 9:38 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> Le vendredi 26 avril 2019 à 21:18 +0530, Jagan Teki a écrit :
> > On Fri, Apr 26, 2019 at 9:08 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi,
> > >
> > > Le vendredi 26 avril 2019 à 20:48 +0530, Jagan Teki a écrit :
> > > > On Fri, Apr 26, 2019 at 8:42 PM Paul Kocialkowski
> > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > Hi,
> > > > >
> > > > > On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
> > > > > > On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
> > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> > > > > > > > On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> > > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > > > > > > > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > > > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > > > > > > > > Add initial support for Orangepi RK3399 board.
> > > > > > > > > > > >
> > > > > > > > > > > > Specification
> > > > > > > > > > > > - Rockchip RK3399
> > > > > > > > > > > > - 2GB/4GB DDR3
> > > > > > > > > > > > - 16GB eMMC
> > > > > > > > > > > > - SD card slot
> > > > > > > > > > >
> > > > > > > > > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > > > > > > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > > > > > > > >
> > > > > > > > > > Look like you are confused or doesn't check the patch. This patch have
> > > > > > > > > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > > > > > > > > u-boot,dm-pre-reloc for sdmmc.
> > > > > > > > >
> > > > > > > > > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > > > > > > > > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > > > > > > > > it in your second series when you add it back to rk3399-u-boot.dtsi.
> > > > > > > >
> > > > > > > > Which u-boot,dm-pre-reloc are you taking about?
> > > > > > > >
> > > > > > > > v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> > > > > > > > which were used by subsequent boards on the same series.
> > > > > > > >
> > > > > > > > The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> > > > > > > > existing board dts files thought that it will include
> > > > > > > > rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> > > > > > > >
> > > > > > > > > It's not okay to submit the board with broken MMC support and fix it in
> > > > > > > > > a subsequent series.
> > > > > > > >
> > > > > > > > Again, nothing broken. existing boards or dts(i) files are untouched.
> > > > > > > > Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> > > > > > > > node to make use of new boards.  and the same reused by next series
> > > > > > > > so-that I can add binman global to all rk3399 boards.
> > > > > > >
> > > > > > > Okay I think I'm getting there. So the Orangepi uses the new scheme
> > > > > > > (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
> > > > > > > other boards are still using the previous scheme after the series.
> > > > > > >
> > > > > > > This is very confusing and I really think you should keep u-boot,dm-
> > > > > > > pre-reloc in the orange pi dts file and then make the transition with
> > > > > > > all the other boards. You're mixing multiple logical steps which makes
> > > > > > > it really hard to understand what's going on.
> > > > > > >
> > > > > > > More to that, introducing the rk3399-u-boot.dtsi is a logical step that
> > > > > > > should be grouped with your second series, not the first one. In your
> > > > > > > first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
> > > > > > >
> > > > > > > Could you respin the two series to group changes by logic changes
> > > > > > > instead of the current state of interleaved changes?
> > > > > >
> > > > > > Okay, to make it clear I can send v4 with v3.1 included and updated
> > > > > > commit log. but I the new series about binman remains same since the
> > > > > > changes on first patch on the series are relevant.
> > > > >
> > > > > I think it makes no sense to introduce rk3399-u-boot.dts as part of the
> > > > > series currently in v3. You need to remove the patch (currently v3.1)
> > > > > from this series and group it with the new series (binman-related).
> > > > >
> > > > > Otherwise, you're mixing two unrelated logical changes across two
> > > > > series.
> > > >
> > > > rk3399-u-boot.dtsi is require for sdboot on that boards, whole idea is
> > > > to untouch Linux dts files and get introduced initial
> > > > rk3399-u-boot.dtsi which doesn't effect any board dts files but use
> > > > new boards.
> > >
> > > Yes I think I properly understand that now. The problem is that
> > > currently, rk3399 boards do not use rk3399-u-boot.dtsi. In the first
> > > series, you are introducing the file and including it in the newly-
> > > introduced devices, but not in the previous ones. This is inconsistent.
> > >
> > > Then, in the second series, you are moving existing boards to using
> > > rk3399-u-boot.dtsi, except for the boards you introduced in the first
> > > series. This is inconsistent.
> > >
> > > Do you see the problem? I feel like you are missing it and only
> > > focusing on the fact that the series works in practice, which is not
> > > the issue.
> >
> > Not again.
> >
> > First series, I get introduced rk3399-u-boot.dtsi, and only the new
> > boards are using this file and next series rest of the boards are
> > using which indeed a valid step. what is inconsistent here, I don't
> > understand.
>
> Yes, what you are describing is exactly the issue. It does not make
> sense to introduce a new common u-boot dtsi for rk3399 and add new
> devices that use this file *before* switching existing devices to the
> new common u-boot dtsi for rk3399 in the same series.

How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
patch says it is an initial one and it is bit hard to add all at once.
ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
unaffected to any of the existing dts files.
Paul Kocialkowski April 26, 2019, 4:40 p.m. UTC | #14
Hi,

Le vendredi 26 avril 2019 à 21:46 +0530, Jagan Teki a écrit :
> On Fri, Apr 26, 2019 at 9:38 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > Le vendredi 26 avril 2019 à 21:18 +0530, Jagan Teki a écrit :
> > > On Fri, Apr 26, 2019 at 9:08 PM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > > Hi,
> > > > 
> > > > Le vendredi 26 avril 2019 à 20:48 +0530, Jagan Teki a écrit :
> > > > > On Fri, Apr 26, 2019 at 8:42 PM Paul Kocialkowski
> > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
> > > > > > > On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
> > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> > > > > > > > > On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> > > > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > > > > > > > > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > > > > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > > 
> > > > > > > > > > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > > > > > > > > > Add initial support for Orangepi RK3399 board.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Specification
> > > > > > > > > > > > > - Rockchip RK3399
> > > > > > > > > > > > > - 2GB/4GB DDR3
> > > > > > > > > > > > > - 16GB eMMC
> > > > > > > > > > > > > - SD card slot
> > > > > > > > > > > > 
> > > > > > > > > > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > > > > > > > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > > > > > > > > > 
> > > > > > > > > > > Look like you are confused or doesn't check the patch. This patch have
> > > > > > > > > > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > > > > > > > > > u-boot,dm-pre-reloc for sdmmc.
> > > > > > > > > > 
> > > > > > > > > > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > > > > > > > > > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > > > > > > > > > it in your second series when you add it back to rk3399-u-boot.dtsi.
> > > > > > > > > 
> > > > > > > > > Which u-boot,dm-pre-reloc are you taking about?
> > > > > > > > > 
> > > > > > > > > v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> > > > > > > > > which were used by subsequent boards on the same series.
> > > > > > > > > 
> > > > > > > > > The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> > > > > > > > > existing board dts files thought that it will include
> > > > > > > > > rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> > > > > > > > > 
> > > > > > > > > > It's not okay to submit the board with broken MMC support and fix it in
> > > > > > > > > > a subsequent series.
> > > > > > > > > 
> > > > > > > > > Again, nothing broken. existing boards or dts(i) files are untouched.
> > > > > > > > > Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> > > > > > > > > node to make use of new boards.  and the same reused by next series
> > > > > > > > > so-that I can add binman global to all rk3399 boards.
> > > > > > > > 
> > > > > > > > Okay I think I'm getting there. So the Orangepi uses the new scheme
> > > > > > > > (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
> > > > > > > > other boards are still using the previous scheme after the series.
> > > > > > > > 
> > > > > > > > This is very confusing and I really think you should keep u-boot,dm-
> > > > > > > > pre-reloc in the orange pi dts file and then make the transition with
> > > > > > > > all the other boards. You're mixing multiple logical steps which makes
> > > > > > > > it really hard to understand what's going on.
> > > > > > > > 
> > > > > > > > More to that, introducing the rk3399-u-boot.dtsi is a logical step that
> > > > > > > > should be grouped with your second series, not the first one. In your
> > > > > > > > first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
> > > > > > > > 
> > > > > > > > Could you respin the two series to group changes by logic changes
> > > > > > > > instead of the current state of interleaved changes?
> > > > > > > 
> > > > > > > Okay, to make it clear I can send v4 with v3.1 included and updated
> > > > > > > commit log. but I the new series about binman remains same since the
> > > > > > > changes on first patch on the series are relevant.
> > > > > > 
> > > > > > I think it makes no sense to introduce rk3399-u-boot.dts as part of the
> > > > > > series currently in v3. You need to remove the patch (currently v3.1)
> > > > > > from this series and group it with the new series (binman-related).
> > > > > > 
> > > > > > Otherwise, you're mixing two unrelated logical changes across two
> > > > > > series.
> > > > > 
> > > > > rk3399-u-boot.dtsi is require for sdboot on that boards, whole idea is
> > > > > to untouch Linux dts files and get introduced initial
> > > > > rk3399-u-boot.dtsi which doesn't effect any board dts files but use
> > > > > new boards.
> > > > 
> > > > Yes I think I properly understand that now. The problem is that
> > > > currently, rk3399 boards do not use rk3399-u-boot.dtsi. In the first
> > > > series, you are introducing the file and including it in the newly-
> > > > introduced devices, but not in the previous ones. This is inconsistent.
> > > > 
> > > > Then, in the second series, you are moving existing boards to using
> > > > rk3399-u-boot.dtsi, except for the boards you introduced in the first
> > > > series. This is inconsistent.
> > > > 
> > > > Do you see the problem? I feel like you are missing it and only
> > > > focusing on the fact that the series works in practice, which is not
> > > > the issue.
> > > 
> > > Not again.
> > > 
> > > First series, I get introduced rk3399-u-boot.dtsi, and only the new
> > > boards are using this file and next series rest of the boards are
> > > using which indeed a valid step. what is inconsistent here, I don't
> > > understand.
> > 
> > Yes, what you are describing is exactly the issue. It does not make
> > sense to introduce a new common u-boot dtsi for rk3399 and add new
> > devices that use this file *before* switching existing devices to the
> > new common u-boot dtsi for rk3399 in the same series.
> 
> How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
> patch says it is an initial one and it is bit hard to add all at once.
> ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
> unaffected to any of the existing dts files.

Again, it's not a technical issue. Your proposal works and has no
technical issue. The issue is in how the commits are grouped together.
Patch series need to make logical sense. One patch series should
accomplish one change, and each patch represents a step of that change.

This is how upstream contributions work, and it's a powerful way to
allow both efficient code review and good code quality. We want to keep
things as simple, explicit and well-described as possible, so that
things are easy for reviewers and as many people as possible can
understand the issue and share their thoughts about it.

It is all part of communication with others as part of a community.
It is definitely an implicit rule that is not written down somewhere
precisely, but that's the social contract between developers that work
on a common software project.

In that, contributing to upstream is different than baseline tech
company standards, because you have to take extra steps to describe
your work and explain it to others. You must make sure you give them
all the comfort they need to painlessly understand what you did.

It plays a great deal in having series accepted without going through
very long discussions and numerous iterations. The less questions
maintainers will have about your work, the better. They may disagree
with the decisions you took, but these discussions are purely technical
and can be resolved quickly.

It's basically that: you have to make sure everything is explained and
easy to understand so that the discussions will only be about the
choices you made and not trying to understand why you had to make these
choices.

Cheers,

Paul
Jagan Teki April 26, 2019, 4:53 p.m. UTC | #15
On Fri, Apr 26, 2019 at 10:10 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> Le vendredi 26 avril 2019 à 21:46 +0530, Jagan Teki a écrit :
> > On Fri, Apr 26, 2019 at 9:38 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi,
> > >
> > > Le vendredi 26 avril 2019 à 21:18 +0530, Jagan Teki a écrit :
> > > > On Fri, Apr 26, 2019 at 9:08 PM Paul Kocialkowski
> > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > Hi,
> > > > >
> > > > > Le vendredi 26 avril 2019 à 20:48 +0530, Jagan Teki a écrit :
> > > > > > On Fri, Apr 26, 2019 at 8:42 PM Paul Kocialkowski
> > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
> > > > > > > > On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
> > > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
> > > > > > > > > > On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
> > > > > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
> > > > > > > > > > > > On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
> > > > > > > > > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
> > > > > > > > > > > > > > Add initial support for Orangepi RK3399 board.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Specification
> > > > > > > > > > > > > > - Rockchip RK3399
> > > > > > > > > > > > > > - 2GB/4GB DDR3
> > > > > > > > > > > > > > - 16GB eMMC
> > > > > > > > > > > > > > - SD card slot
> > > > > > > > > > > > >
> > > > > > > > > > > > > Looks like you're missing u-boot,dm-pre-reloc to have it working, which
> > > > > > > > > > > > > will need to be introduced when moving to rk3399-u-boot.dtsi.
> > > > > > > > > > > >
> > > > > > > > > > > > Look like you are confused or doesn't check the patch. This patch have
> > > > > > > > > > > > rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
> > > > > > > > > > > > u-boot,dm-pre-reloc for sdmmc.
> > > > > > > > > > >
> > > > > > > > > > > Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
> > > > > > > > > > > rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
> > > > > > > > > > > it in your second series when you add it back to rk3399-u-boot.dtsi.
> > > > > > > > > >
> > > > > > > > > > Which u-boot,dm-pre-reloc are you taking about?
> > > > > > > > > >
> > > > > > > > > > v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
> > > > > > > > > > which were used by subsequent boards on the same series.
> > > > > > > > > >
> > > > > > > > > > The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
> > > > > > > > > > existing board dts files thought that it will include
> > > > > > > > > > rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
> > > > > > > > > >
> > > > > > > > > > > It's not okay to submit the board with broken MMC support and fix it in
> > > > > > > > > > > a subsequent series.
> > > > > > > > > >
> > > > > > > > > > Again, nothing broken. existing boards or dts(i) files are untouched.
> > > > > > > > > > Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
> > > > > > > > > > node to make use of new boards.  and the same reused by next series
> > > > > > > > > > so-that I can add binman global to all rk3399 boards.
> > > > > > > > >
> > > > > > > > > Okay I think I'm getting there. So the Orangepi uses the new scheme
> > > > > > > > > (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
> > > > > > > > > other boards are still using the previous scheme after the series.
> > > > > > > > >
> > > > > > > > > This is very confusing and I really think you should keep u-boot,dm-
> > > > > > > > > pre-reloc in the orange pi dts file and then make the transition with
> > > > > > > > > all the other boards. You're mixing multiple logical steps which makes
> > > > > > > > > it really hard to understand what's going on.
> > > > > > > > >
> > > > > > > > > More to that, introducing the rk3399-u-boot.dtsi is a logical step that
> > > > > > > > > should be grouped with your second series, not the first one. In your
> > > > > > > > > first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
> > > > > > > > >
> > > > > > > > > Could you respin the two series to group changes by logic changes
> > > > > > > > > instead of the current state of interleaved changes?
> > > > > > > >
> > > > > > > > Okay, to make it clear I can send v4 with v3.1 included and updated
> > > > > > > > commit log. but I the new series about binman remains same since the
> > > > > > > > changes on first patch on the series are relevant.
> > > > > > >
> > > > > > > I think it makes no sense to introduce rk3399-u-boot.dts as part of the
> > > > > > > series currently in v3. You need to remove the patch (currently v3.1)
> > > > > > > from this series and group it with the new series (binman-related).
> > > > > > >
> > > > > > > Otherwise, you're mixing two unrelated logical changes across two
> > > > > > > series.
> > > > > >
> > > > > > rk3399-u-boot.dtsi is require for sdboot on that boards, whole idea is
> > > > > > to untouch Linux dts files and get introduced initial
> > > > > > rk3399-u-boot.dtsi which doesn't effect any board dts files but use
> > > > > > new boards.
> > > > >
> > > > > Yes I think I properly understand that now. The problem is that
> > > > > currently, rk3399 boards do not use rk3399-u-boot.dtsi. In the first
> > > > > series, you are introducing the file and including it in the newly-
> > > > > introduced devices, but not in the previous ones. This is inconsistent.
> > > > >
> > > > > Then, in the second series, you are moving existing boards to using
> > > > > rk3399-u-boot.dtsi, except for the boards you introduced in the first
> > > > > series. This is inconsistent.
> > > > >
> > > > > Do you see the problem? I feel like you are missing it and only
> > > > > focusing on the fact that the series works in practice, which is not
> > > > > the issue.
> > > >
> > > > Not again.
> > > >
> > > > First series, I get introduced rk3399-u-boot.dtsi, and only the new
> > > > boards are using this file and next series rest of the boards are
> > > > using which indeed a valid step. what is inconsistent here, I don't
> > > > understand.
> > >
> > > Yes, what you are describing is exactly the issue. It does not make
> > > sense to introduce a new common u-boot dtsi for rk3399 and add new
> > > devices that use this file *before* switching existing devices to the
> > > new common u-boot dtsi for rk3399 in the same series.
> >
> > How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
> > patch says it is an initial one and it is bit hard to add all at once.
> > ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
> > unaffected to any of the existing dts files.
>
> Again, it's not a technical issue. Your proposal works and has no
> technical issue. The issue is in how the commits are grouped together.
> Patch series need to make logical sense. One patch series should
> accomplish one change, and each patch represents a step of that change.

It's about how you think. say if I wouldn't send the binman, I'm sure
this kind of discussion may not happen. In first mail you said
"something broken and how it can repair next" that indeed doesn't make
any sense of without understanding the whole series of changes.

Any new changes would come up with initial version, that what we
usually supported, ie what we did -u-boot.dtsi changes to i.MX6 and
developers are using the same is adding one after another.

>
> This is how upstream contributions work, and it's a powerful way to
> allow both efficient code review and good code quality. We want to keep
> things as simple, explicit and well-described as possible, so that
> things are easy for reviewers and as many people as possible can
> understand the issue and share their thoughts about it.

Yes, we adopt these principles and that what we are really working.

>
> It is all part of communication with others as part of a community.
> It is definitely an implicit rule that is not written down somewhere
> precisely, but that's the social contract between developers that work
> on a common software project.
>
> In that, contributing to upstream is different than baseline tech
> company standards, because you have to take extra steps to describe
> your work and explain it to others. You must make sure you give them
> all the comfort they need to painlessly understand what you did.

I hope all my communication was relevant to the topics, I can even
given the example how things were moved.

>
> It plays a great deal in having series accepted without going through
> very long discussions and numerous iterations. The less questions
> maintainers will have about your work, the better. They may disagree
> with the decisions you took, but these discussions are purely technical
> and can be resolved quickly.

Pure technical, yes ie what I thought off. no problem for me for long
discussion atleast people can understand how things went.

Jagan.
Paul Kocialkowski April 26, 2019, 5:08 p.m. UTC | #16
Hi,

Le vendredi 26 avril 2019 à 22:23 +0530, Jagan Teki a écrit :
> > > > > First series, I get introduced rk3399-u-boot.dtsi, and only the new
> > > > > boards are using this file and next series rest of the boards are
> > > > > using which indeed a valid step. what is inconsistent here, I don't
> > > > > understand.
> > > > 
> > > > Yes, what you are describing is exactly the issue. It does not make
> > > > sense to introduce a new common u-boot dtsi for rk3399 and add new
> > > > devices that use this file *before* switching existing devices to the
> > > > new common u-boot dtsi for rk3399 in the same series.
> > > 
> > > How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
> > > patch says it is an initial one and it is bit hard to add all at once.
> > > ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
> > > unaffected to any of the existing dts files.
> > 
> > Again, it's not a technical issue. Your proposal works and has no
> > technical issue. The issue is in how the commits are grouped together.
> > Patch series need to make logical sense. One patch series should
> > accomplish one change, and each patch represents a step of that change.
> 
> It's about how you think. say if I wouldn't send the binman, I'm sure
> this kind of discussion may not happen. In first mail you said
> "something broken and how it can repair next" that indeed doesn't make
> any sense of without understanding the whole series of changes.

One part to that is that I had misunderstood a few things, but I really
should not have to look at each patch before I can have an idea of what
the series does. Your series is called "rockchip: Add new rk3399
boards" and with the v3.1 change you made, the title is no longer true.
You are doing that + introducing a new rk3399 u-boot dtsi without
switching existing boards to it before adding new ones.

From that moment, you needed to split your changes into two series.
Instead of that, you made another series with mostly unrelated changes
where you included an unrelated commit adding the pre-reloc entries to
the dtsi created in the previous series.

That's an issue for communication with the community.

> Any new changes would come up with initial version, that what we
> usually supported, ie what we did -u-boot.dtsi changes to i.MX6 and
> developers are using the same is adding one after another.

But you already have boards in the tree that needs the tree. You can
make individual commits for switching boards to the new dtsi, but you
need to do that in the same series as introducing the dtsi if you are
submitting both things at the same time. If you want to have them
merged separately, then you can send device dtsi patches individually
for that, but not as part of another unrelated series.

> > This is how upstream contributions work, and it's a powerful way to
> > allow both efficient code review and good code quality. We want to keep
> > things as simple, explicit and well-described as possible, so that
> > things are easy for reviewers and as many people as possible can
> > understand the issue and share their thoughts about it.
> 
> Yes, we adopt these principles and that what we are really working.
> 
> > It is all part of communication with others as part of a community.
> > It is definitely an implicit rule that is not written down somewhere
> > precisely, but that's the social contract between developers that work
> > on a common software project.
> > 
> > In that, contributing to upstream is different than baseline tech
> > company standards, because you have to take extra steps to describe
> > your work and explain it to others. You must make sure you give them
> > all the comfort they need to painlessly understand what you did.
> 
> I hope all my communication was relevant to the topics, I can even
> given the example how things were moved.

Clearly there was a major communication issue between us. You only
answered on technical topics and never about how your patch series is
organized and what logical changes you are making.

To be honest with you, I feel like we have a general communication
issue where you only seem to focus on technical aspects, when the
issues are about the meta-data of the changes such as commit messages,
code comments and how changes are organized together.

> > It plays a great deal in having series accepted without going through
> > very long discussions and numerous iterations. The less questions
> > maintainers will have about your work, the better. They may disagree
> > with the decisions you took, but these discussions are purely technical
> > and can be resolved quickly.
> 
> Pure technical, yes ie what I thought off. no problem for me for long
> discussion atleast people can understand how things went.

Community discussions can not, and must not be purely technical. This
is not a personal opinion of mine, this is how all free software
communities work. Some care more than others about it, but in very
technical projects like U-Boot and Linux, people need to care about it
a lot.

The more technical the things you are dealing with are, the more you
need to do on the non-technical side to make sure what you are doing is
clear, understood by everyone and makes sense on its own.

Cheers,

Paul
Jagan Teki April 26, 2019, 5:21 p.m. UTC | #17
On Fri, Apr 26, 2019 at 10:38 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> Le vendredi 26 avril 2019 à 22:23 +0530, Jagan Teki a écrit :
> > > > > > First series, I get introduced rk3399-u-boot.dtsi, and only the new
> > > > > > boards are using this file and next series rest of the boards are
> > > > > > using which indeed a valid step. what is inconsistent here, I don't
> > > > > > understand.
> > > > >
> > > > > Yes, what you are describing is exactly the issue. It does not make
> > > > > sense to introduce a new common u-boot dtsi for rk3399 and add new
> > > > > devices that use this file *before* switching existing devices to the
> > > > > new common u-boot dtsi for rk3399 in the same series.
> > > >
> > > > How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
> > > > patch says it is an initial one and it is bit hard to add all at once.
> > > > ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
> > > > unaffected to any of the existing dts files.
> > >
> > > Again, it's not a technical issue. Your proposal works and has no
> > > technical issue. The issue is in how the commits are grouped together.
> > > Patch series need to make logical sense. One patch series should
> > > accomplish one change, and each patch represents a step of that change.
> >
> > It's about how you think. say if I wouldn't send the binman, I'm sure
> > this kind of discussion may not happen. In first mail you said
> > "something broken and how it can repair next" that indeed doesn't make
> > any sense of without understanding the whole series of changes.
>
> One part to that is that I had misunderstood a few things, but I really
> should not have to look at each patch before I can have an idea of what
> the series does. Your series is called "rockchip: Add new rk3399
> boards" and with the v3.1 change you made, the title is no longer true.
> You are doing that + introducing a new rk3399 u-boot dtsi without
> switching existing boards to it before adding new ones.
>
> From that moment, you needed to split your changes into two series.
> Instead of that, you made another series with mostly unrelated changes
> where you included an unrelated commit adding the pre-reloc entries to
> the dtsi created in the previous series.
>
> That's an issue for communication with the community.
>
> > Any new changes would come up with initial version, that what we
> > usually supported, ie what we did -u-boot.dtsi changes to i.MX6 and
> > developers are using the same is adding one after another.
>
> But you already have boards in the tree that needs the tree. You can
> make individual commits for switching boards to the new dtsi, but you
> need to do that in the same series as introducing the dtsi if you are
> submitting both things at the same time. If you want to have them
> merged separately, then you can send device dtsi patches individually
> for that, but not as part of another unrelated series.
>
> > > This is how upstream contributions work, and it's a powerful way to
> > > allow both efficient code review and good code quality. We want to keep
> > > things as simple, explicit and well-described as possible, so that
> > > things are easy for reviewers and as many people as possible can
> > > understand the issue and share their thoughts about it.
> >
> > Yes, we adopt these principles and that what we are really working.
> >
> > > It is all part of communication with others as part of a community.
> > > It is definitely an implicit rule that is not written down somewhere
> > > precisely, but that's the social contract between developers that work
> > > on a common software project.
> > >
> > > In that, contributing to upstream is different than baseline tech
> > > company standards, because you have to take extra steps to describe
> > > your work and explain it to others. You must make sure you give them
> > > all the comfort they need to painlessly understand what you did.
> >
> > I hope all my communication was relevant to the topics, I can even
> > given the example how things were moved.
>
> Clearly there was a major communication issue between us. You only
> answered on technical topics and never about how your patch series is
> organized and what logical changes you are making.
>
> To be honest with you, I feel like we have a general communication
> issue where you only seem to focus on technical aspects, when the
> issues are about the meta-data of the changes such as commit messages,
> code comments and how changes are organized together.

No one is better in all aspects, and things were improving no one cam
make global statements like this though he can find something gaps on
on previous one. You would better suggest on the commit where it
confused can make me better understanding.

>
> > > It plays a great deal in having series accepted without going through
> > > very long discussions and numerous iterations. The less questions
> > > maintainers will have about your work, the better. They may disagree
> > > with the decisions you took, but these discussions are purely technical
> > > and can be resolved quickly.
> >
> > Pure technical, yes ie what I thought off. no problem for me for long
> > discussion atleast people can understand how things went.
>
> Community discussions can not, and must not be purely technical. This
> is not a personal opinion of mine, this is how all free software
> communities work. Some care more than others about it, but in very
> technical projects like U-Boot and Linux, people need to care about it
> a lot.
>
> The more technical the things you are dealing with are, the more you
> need to do on the non-technical side to make sure what you are doing is
> clear, understood by everyone and makes sense on its own.

True, this is what I'm trying to work always, thanks.
Paul Kocialkowski April 26, 2019, 5:37 p.m. UTC | #18
Hi,

Le vendredi 26 avril 2019 à 22:51 +0530, Jagan Teki a écrit :
> On Fri, Apr 26, 2019 at 10:38 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > Le vendredi 26 avril 2019 à 22:23 +0530, Jagan Teki a écrit :
> > > > > > > First series, I get introduced rk3399-u-boot.dtsi, and only the new
> > > > > > > boards are using this file and next series rest of the boards are
> > > > > > > using which indeed a valid step. what is inconsistent here, I don't
> > > > > > > understand.
> > > > > > 
> > > > > > Yes, what you are describing is exactly the issue. It does not make
> > > > > > sense to introduce a new common u-boot dtsi for rk3399 and add new
> > > > > > devices that use this file *before* switching existing devices to the
> > > > > > new common u-boot dtsi for rk3399 in the same series.
> > > > > 
> > > > > How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
> > > > > patch says it is an initial one and it is bit hard to add all at once.
> > > > > ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
> > > > > unaffected to any of the existing dts files.
> > > > 
> > > > Again, it's not a technical issue. Your proposal works and has no
> > > > technical issue. The issue is in how the commits are grouped together.
> > > > Patch series need to make logical sense. One patch series should
> > > > accomplish one change, and each patch represents a step of that change.
> > > 
> > > It's about how you think. say if I wouldn't send the binman, I'm sure
> > > this kind of discussion may not happen. In first mail you said
> > > "something broken and how it can repair next" that indeed doesn't make
> > > any sense of without understanding the whole series of changes.
> > 
> > One part to that is that I had misunderstood a few things, but I really
> > should not have to look at each patch before I can have an idea of what
> > the series does. Your series is called "rockchip: Add new rk3399
> > boards" and with the v3.1 change you made, the title is no longer true.
> > You are doing that + introducing a new rk3399 u-boot dtsi without
> > switching existing boards to it before adding new ones.
> > 
> > From that moment, you needed to split your changes into two series.
> > Instead of that, you made another series with mostly unrelated changes
> > where you included an unrelated commit adding the pre-reloc entries to
> > the dtsi created in the previous series.
> > 
> > That's an issue for communication with the community.
> > 
> > > Any new changes would come up with initial version, that what we
> > > usually supported, ie what we did -u-boot.dtsi changes to i.MX6 and
> > > developers are using the same is adding one after another.
> > 
> > But you already have boards in the tree that needs the tree. You can
> > make individual commits for switching boards to the new dtsi, but you
> > need to do that in the same series as introducing the dtsi if you are
> > submitting both things at the same time. If you want to have them
> > merged separately, then you can send device dtsi patches individually
> > for that, but not as part of another unrelated series.
> > 
> > > > This is how upstream contributions work, and it's a powerful way to
> > > > allow both efficient code review and good code quality. We want to keep
> > > > things as simple, explicit and well-described as possible, so that
> > > > things are easy for reviewers and as many people as possible can
> > > > understand the issue and share their thoughts about it.
> > > 
> > > Yes, we adopt these principles and that what we are really working.
> > > 
> > > > It is all part of communication with others as part of a community.
> > > > It is definitely an implicit rule that is not written down somewhere
> > > > precisely, but that's the social contract between developers that work
> > > > on a common software project.
> > > > 
> > > > In that, contributing to upstream is different than baseline tech
> > > > company standards, because you have to take extra steps to describe
> > > > your work and explain it to others. You must make sure you give them
> > > > all the comfort they need to painlessly understand what you did.
> > > 
> > > I hope all my communication was relevant to the topics, I can even
> > > given the example how things were moved.
> > 
> > Clearly there was a major communication issue between us. You only
> > answered on technical topics and never about how your patch series is
> > organized and what logical changes you are making.
> > 
> > To be honest with you, I feel like we have a general communication
> > issue where you only seem to focus on technical aspects, when the
> > issues are about the meta-data of the changes such as commit messages,
> > code comments and how changes are organized together.
> 
> No one is better in all aspects, and things were improving no one cam
> make global statements like this though he can find something gaps on
> on previous one. You would better suggest on the commit where it
> confused can make me better understanding.

I am making such a global statement because I've had this issue with
you a few times before already, and from what I could see about sunxi-
related discussions, it seems that others have had similar issues
before too. Each time, it feels like you are not discussing the issue
and answering on mostly-unrelated technical topics, exactly like it's
happening now.

Now this is really nothing personal against you, I have no interest in
pointing out things that need to be worked on in the work you submit in
order to make you feel bad or inferior. But at this point, I feel like
you are not understanding the global issue so I am bringing the
discussion to more general conclusions, since we are talking about
communication issues more generally.

Everyone is trying to improve and it's perfectly fine to have flaws.
For that matter, the situation was already reversed a few times when
you reviewed some of my sunxi patches saying that the commit log was
unclear, and I gladly accepted the criticism to make a better next
revision. I also want to add that you make significant important
contributions to lots of projects I really care about. This thread is a
perfect example of that too, since I plan on using a RK3399 boards you
are introducing for my personal use. So I am more than happy to see you
do all that hard work and I truly value your contributions.

Really, I'm mostly trying to help here and collaborate towards
resolving the situation.

So could you please send 3 distinct series that deal with each one of
the aspects from the list that follows?

- Introducing a common RK3399 u-boot dtsi with the required pre-reloc
entries;
- Adding new RK3399 devices and syncing the dtsi files from Linux;
- Introducing u-boot-rockchip-with-spl.bin.

Thanks,

Paul

> > > > It plays a great deal in having series accepted without going through
> > > > very long discussions and numerous iterations. The less questions
> > > > maintainers will have about your work, the better. They may disagree
> > > > with the decisions you took, but these discussions are purely technical
> > > > and can be resolved quickly.
> > > 
> > > Pure technical, yes ie what I thought off. no problem for me for long
> > > discussion atleast people can understand how things went.
> > 
> > Community discussions can not, and must not be purely technical. This
> > is not a personal opinion of mine, this is how all free software
> > communities work. Some care more than others about it, but in very
> > technical projects like U-Boot and Linux, people need to care about it
> > a lot.
> > 
> > The more technical the things you are dealing with are, the more you
> > need to do on the non-technical side to make sure what you are doing is
> > clear, understood by everyone and makes sense on its own.
> 
> True, this is what I'm trying to work always, thanks.
Jagan Teki April 26, 2019, 5:42 p.m. UTC | #19
On Fri, Apr 26, 2019 at 11:07 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> Le vendredi 26 avril 2019 à 22:51 +0530, Jagan Teki a écrit :
> > On Fri, Apr 26, 2019 at 10:38 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi,
> > >
> > > Le vendredi 26 avril 2019 à 22:23 +0530, Jagan Teki a écrit :
> > > > > > > > First series, I get introduced rk3399-u-boot.dtsi, and only the new
> > > > > > > > boards are using this file and next series rest of the boards are
> > > > > > > > using which indeed a valid step. what is inconsistent here, I don't
> > > > > > > > understand.
> > > > > > >
> > > > > > > Yes, what you are describing is exactly the issue. It does not make
> > > > > > > sense to introduce a new common u-boot dtsi for rk3399 and add new
> > > > > > > devices that use this file *before* switching existing devices to the
> > > > > > > new common u-boot dtsi for rk3399 in the same series.
> > > > > >
> > > > > > How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
> > > > > > patch says it is an initial one and it is bit hard to add all at once.
> > > > > > ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
> > > > > > unaffected to any of the existing dts files.
> > > > >
> > > > > Again, it's not a technical issue. Your proposal works and has no
> > > > > technical issue. The issue is in how the commits are grouped together.
> > > > > Patch series need to make logical sense. One patch series should
> > > > > accomplish one change, and each patch represents a step of that change.
> > > >
> > > > It's about how you think. say if I wouldn't send the binman, I'm sure
> > > > this kind of discussion may not happen. In first mail you said
> > > > "something broken and how it can repair next" that indeed doesn't make
> > > > any sense of without understanding the whole series of changes.
> > >
> > > One part to that is that I had misunderstood a few things, but I really
> > > should not have to look at each patch before I can have an idea of what
> > > the series does. Your series is called "rockchip: Add new rk3399
> > > boards" and with the v3.1 change you made, the title is no longer true.
> > > You are doing that + introducing a new rk3399 u-boot dtsi without
> > > switching existing boards to it before adding new ones.
> > >
> > > From that moment, you needed to split your changes into two series.
> > > Instead of that, you made another series with mostly unrelated changes
> > > where you included an unrelated commit adding the pre-reloc entries to
> > > the dtsi created in the previous series.
> > >
> > > That's an issue for communication with the community.
> > >
> > > > Any new changes would come up with initial version, that what we
> > > > usually supported, ie what we did -u-boot.dtsi changes to i.MX6 and
> > > > developers are using the same is adding one after another.
> > >
> > > But you already have boards in the tree that needs the tree. You can
> > > make individual commits for switching boards to the new dtsi, but you
> > > need to do that in the same series as introducing the dtsi if you are
> > > submitting both things at the same time. If you want to have them
> > > merged separately, then you can send device dtsi patches individually
> > > for that, but not as part of another unrelated series.
> > >
> > > > > This is how upstream contributions work, and it's a powerful way to
> > > > > allow both efficient code review and good code quality. We want to keep
> > > > > things as simple, explicit and well-described as possible, so that
> > > > > things are easy for reviewers and as many people as possible can
> > > > > understand the issue and share their thoughts about it.
> > > >
> > > > Yes, we adopt these principles and that what we are really working.
> > > >
> > > > > It is all part of communication with others as part of a community.
> > > > > It is definitely an implicit rule that is not written down somewhere
> > > > > precisely, but that's the social contract between developers that work
> > > > > on a common software project.
> > > > >
> > > > > In that, contributing to upstream is different than baseline tech
> > > > > company standards, because you have to take extra steps to describe
> > > > > your work and explain it to others. You must make sure you give them
> > > > > all the comfort they need to painlessly understand what you did.
> > > >
> > > > I hope all my communication was relevant to the topics, I can even
> > > > given the example how things were moved.
> > >
> > > Clearly there was a major communication issue between us. You only
> > > answered on technical topics and never about how your patch series is
> > > organized and what logical changes you are making.
> > >
> > > To be honest with you, I feel like we have a general communication
> > > issue where you only seem to focus on technical aspects, when the
> > > issues are about the meta-data of the changes such as commit messages,
> > > code comments and how changes are organized together.
> >
> > No one is better in all aspects, and things were improving no one cam
> > make global statements like this though he can find something gaps on
> > on previous one. You would better suggest on the commit where it
> > confused can make me better understanding.
>
> I am making such a global statement because I've had this issue with
> you a few times before already, and from what I could see about sunxi-
> related discussions, it seems that others have had similar issues
> before too. Each time, it feels like you are not discussing the issue
> and answering on mostly-unrelated technical topics, exactly like it's
> happening now.
>
> Now this is really nothing personal against you, I have no interest in
> pointing out things that need to be worked on in the work you submit in
> order to make you feel bad or inferior. But at this point, I feel like
> you are not understanding the global issue so I am bringing the
> discussion to more general conclusions, since we are talking about
> communication issues more generally.
>
> Everyone is trying to improve and it's perfectly fine to have flaws.
> For that matter, the situation was already reversed a few times when
> you reviewed some of my sunxi patches saying that the commit log was
> unclear, and I gladly accepted the criticism to make a better next
> revision. I also want to add that you make significant important
> contributions to lots of projects I really care about. This thread is a
> perfect example of that too, since I plan on using a RK3399 boards you
> are introducing for my personal use. So I am more than happy to see you
> do all that hard work and I truly value your contributions.
>
> Really, I'm mostly trying to help here and collaborate towards
> resolving the situation.

Thanks.

>
> So could you please send 3 distinct series that deal with each one of
> the aspects from the list that follows?

Just now I sent v5, which would resolve what you are thinking of,
please have a look.
Paul Kocialkowski April 26, 2019, 6:02 p.m. UTC | #20
Hi,

Le vendredi 26 avril 2019 à 23:12 +0530, Jagan Teki a écrit :
> On Fri, Apr 26, 2019 at 11:07 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > Le vendredi 26 avril 2019 à 22:51 +0530, Jagan Teki a écrit :
> > > On Fri, Apr 26, 2019 at 10:38 PM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > > Hi,
> > > > 
> > > > Le vendredi 26 avril 2019 à 22:23 +0530, Jagan Teki a écrit :
> > > > > > > > > First series, I get introduced rk3399-u-boot.dtsi, and only the new
> > > > > > > > > boards are using this file and next series rest of the boards are
> > > > > > > > > using which indeed a valid step. what is inconsistent here, I don't
> > > > > > > > > understand.
> > > > > > > > 
> > > > > > > > Yes, what you are describing is exactly the issue. It does not make
> > > > > > > > sense to introduce a new common u-boot dtsi for rk3399 and add new
> > > > > > > > devices that use this file *before* switching existing devices to the
> > > > > > > > new common u-boot dtsi for rk3399 in the same series.
> > > > > > > 
> > > > > > > How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
> > > > > > > patch says it is an initial one and it is bit hard to add all at once.
> > > > > > > ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
> > > > > > > unaffected to any of the existing dts files.
> > > > > > 
> > > > > > Again, it's not a technical issue. Your proposal works and has no
> > > > > > technical issue. The issue is in how the commits are grouped together.
> > > > > > Patch series need to make logical sense. One patch series should
> > > > > > accomplish one change, and each patch represents a step of that change.
> > > > > 
> > > > > It's about how you think. say if I wouldn't send the binman, I'm sure
> > > > > this kind of discussion may not happen. In first mail you said
> > > > > "something broken and how it can repair next" that indeed doesn't make
> > > > > any sense of without understanding the whole series of changes.
> > > > 
> > > > One part to that is that I had misunderstood a few things, but I really
> > > > should not have to look at each patch before I can have an idea of what
> > > > the series does. Your series is called "rockchip: Add new rk3399
> > > > boards" and with the v3.1 change you made, the title is no longer true.
> > > > You are doing that + introducing a new rk3399 u-boot dtsi without
> > > > switching existing boards to it before adding new ones.
> > > > 
> > > > From that moment, you needed to split your changes into two series.
> > > > Instead of that, you made another series with mostly unrelated changes
> > > > where you included an unrelated commit adding the pre-reloc entries to
> > > > the dtsi created in the previous series.
> > > > 
> > > > That's an issue for communication with the community.
> > > > 
> > > > > Any new changes would come up with initial version, that what we
> > > > > usually supported, ie what we did -u-boot.dtsi changes to i.MX6 and
> > > > > developers are using the same is adding one after another.
> > > > 
> > > > But you already have boards in the tree that needs the tree. You can
> > > > make individual commits for switching boards to the new dtsi, but you
> > > > need to do that in the same series as introducing the dtsi if you are
> > > > submitting both things at the same time. If you want to have them
> > > > merged separately, then you can send device dtsi patches individually
> > > > for that, but not as part of another unrelated series.
> > > > 
> > > > > > This is how upstream contributions work, and it's a powerful way to
> > > > > > allow both efficient code review and good code quality. We want to keep
> > > > > > things as simple, explicit and well-described as possible, so that
> > > > > > things are easy for reviewers and as many people as possible can
> > > > > > understand the issue and share their thoughts about it.
> > > > > 
> > > > > Yes, we adopt these principles and that what we are really working.
> > > > > 
> > > > > > It is all part of communication with others as part of a community.
> > > > > > It is definitely an implicit rule that is not written down somewhere
> > > > > > precisely, but that's the social contract between developers that work
> > > > > > on a common software project.
> > > > > > 
> > > > > > In that, contributing to upstream is different than baseline tech
> > > > > > company standards, because you have to take extra steps to describe
> > > > > > your work and explain it to others. You must make sure you give them
> > > > > > all the comfort they need to painlessly understand what you did.
> > > > > 
> > > > > I hope all my communication was relevant to the topics, I can even
> > > > > given the example how things were moved.
> > > > 
> > > > Clearly there was a major communication issue between us. You only
> > > > answered on technical topics and never about how your patch series is
> > > > organized and what logical changes you are making.
> > > > 
> > > > To be honest with you, I feel like we have a general communication
> > > > issue where you only seem to focus on technical aspects, when the
> > > > issues are about the meta-data of the changes such as commit messages,
> > > > code comments and how changes are organized together.
> > > 
> > > No one is better in all aspects, and things were improving no one cam
> > > make global statements like this though he can find something gaps on
> > > on previous one. You would better suggest on the commit where it
> > > confused can make me better understanding.
> > 
> > I am making such a global statement because I've had this issue with
> > you a few times before already, and from what I could see about sunxi-
> > related discussions, it seems that others have had similar issues
> > before too. Each time, it feels like you are not discussing the issue
> > and answering on mostly-unrelated technical topics, exactly like it's
> > happening now.
> > 
> > Now this is really nothing personal against you, I have no interest in
> > pointing out things that need to be worked on in the work you submit in
> > order to make you feel bad or inferior. But at this point, I feel like
> > you are not understanding the global issue so I am bringing the
> > discussion to more general conclusions, since we are talking about
> > communication issues more generally.
> > 
> > Everyone is trying to improve and it's perfectly fine to have flaws.
> > For that matter, the situation was already reversed a few times when
> > you reviewed some of my sunxi patches saying that the commit log was
> > unclear, and I gladly accepted the criticism to make a better next
> > revision. I also want to add that you make significant important
> > contributions to lots of projects I really care about. This thread is a
> > perfect example of that too, since I plan on using a RK3399 boards you
> > are introducing for my personal use. So I am more than happy to see you
> > do all that hard work and I truly value your contributions.
> > 
> > Really, I'm mostly trying to help here and collaborate towards
> > resolving the situation.
> 
> Thanks.
> 
> > So could you please send 3 distinct series that deal with each one of
> > the aspects from the list that follows?
> 
> Just now I sent v5, which would resolve what you are thinking of,
> please have a look.

It seems that we're not quite there yet and you still need to split the
series. Looking forward to that!

Cheers,

Paul

Patch

diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index 0e2ffdb87f..6d55b0caf8 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -87,6 +87,7 @@  dtb-$(CONFIG_ARCH_ROCKCHIP) += \
 	rk3399-evb.dtb \
 	rk3399-firefly.dtb \
 	rk3399-gru-bob.dtb \
+	rk3399-orangepi.dtb \
 	rk3399-puma-ddr1333.dtb \
 	rk3399-puma-ddr1600.dtb \
 	rk3399-puma-ddr1866.dtb \
diff --git a/arch/arm/dts/rk3399-orangepi-u-boot.dtsi b/arch/arm/dts/rk3399-orangepi-u-boot.dtsi
new file mode 100644
index 0000000000..236b61d78d
--- /dev/null
+++ b/arch/arm/dts/rk3399-orangepi-u-boot.dtsi
@@ -0,0 +1,7 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Jagan Teki <jagan@amarulasolutions.com>
+ */
+
+#include "rk3399-u-boot.dtsi"
+#include "rk3399-sdram-ddr3-1333.dtsi"
diff --git a/arch/arm/dts/rk3399-orangepi.dts b/arch/arm/dts/rk3399-orangepi.dts
new file mode 100644
index 0000000000..cf37b96a6b
--- /dev/null
+++ b/arch/arm/dts/rk3399-orangepi.dts
@@ -0,0 +1,771 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2017 Fuzhou Rockchip Electronics Co., Ltd.
+ */
+
+/dts-v1/;
+
+#include "dt-bindings/pwm/pwm.h"
+#include "dt-bindings/input/input.h"
+#include "rk3399.dtsi"
+#include "rk3399-opp.dtsi"
+
+/ {
+	model = "Orange Pi RK3399 Board";
+	compatible = "rockchip,rk3399-orangepi", "rockchip,rk3399";
+
+	chosen {
+		stdout-path = "serial2:1500000n8";
+	};
+
+	clkin_gmac: external-gmac-clock {
+		compatible = "fixed-clock";
+		clock-frequency = <125000000>;
+		clock-output-names = "clkin_gmac";
+		#clock-cells = <0>;
+	};
+
+	adc-keys {
+		compatible = "adc-keys";
+		io-channels = <&saradc 1>;
+		io-channel-names = "buttons";
+		keyup-threshold-microvolt = <1800000>;
+		poll-interval = <100>;
+
+		button-up {
+			label = "Volume Up";
+			linux,code = <KEY_VOLUMEUP>;
+			press-threshold-microvolt = <100000>;
+		};
+
+		button-down {
+			label = "Volume Down";
+			linux,code = <KEY_VOLUMEDOWN>;
+			press-threshold-microvolt = <300000>;
+		};
+
+		back {
+			label = "Back";
+			linux,code = <KEY_BACK>;
+			press-threshold-microvolt = <985000>;
+		};
+
+		menu {
+			label = "Menu";
+			linux,code = <KEY_MENU>;
+			press-threshold-microvolt = <1314000>;
+		};
+	};
+
+	dc_12v: dc-12v {
+		compatible = "regulator-fixed";
+		regulator-name = "dc_12v";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+	};
+
+	keys: gpio-keys {
+		compatible = "gpio-keys";
+		autorepeat;
+
+		power {
+			debounce-interval = <100>;
+			gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>;
+			label = "GPIO Power";
+			linux,code = <KEY_POWER>;
+			linux,input-type = <1>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pwr_btn>;
+			wakeup-source;
+		};
+	};
+
+	sdio_pwrseq: sdio-pwrseq {
+		compatible = "mmc-pwrseq-simple";
+		clocks = <&rk808 1>;
+		clock-names = "ext_clock";
+		pinctrl-names = "default";
+		pinctrl-0 = <&wifi_reg_on_h>;
+		reset-gpios = <&gpio0 RK_PB2 GPIO_ACTIVE_LOW>;
+	};
+
+	/* switched by pmic_sleep */
+	vcc1v8_s3: vcca1v8_s3: vcc1v8-s3 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc1v8_s3";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		vin-supply = <&vcc_1v8>;
+	};
+
+	vcc3v0_sd: vcc3v0-sd {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&sdmmc0_pwr_h>;
+		regulator-boot-on;
+		regulator-max-microvolt = <3000000>;
+		regulator-min-microvolt = <3000000>;
+		regulator-name = "vcc3v0_sd";
+		vin-supply = <&vcc3v3_sys>;
+	};
+
+	vcc3v3_sys: vcc3v3-sys {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v3_sys";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		vin-supply = <&vcc_sys>;
+	};
+
+	vcc5v0_host: vcc5v0-host-regulator {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&gpio4 RK_PD1 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&vcc5v0_host_en>;
+		regulator-name = "vcc5v0_host";
+		regulator-always-on;
+		vin-supply = <&vcc_sys>;
+	};
+
+	vcc5v0_typec0: vcc5v0-typec0-regulator {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&gpio1 RK_PA3 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&vcc5v0_typec0_en>;
+		regulator-name = "vcc5v0_typec0";
+		vin-supply = <&vcc_sys>;
+	};
+
+	vcc_sys: vcc-sys {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc_sys";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		vin-supply = <&dc_12v>;
+	};
+
+	vdd_log: vdd-log {
+		compatible = "pwm-regulator";
+		pwms = <&pwm2 0 25000 1>;
+		regulator-name = "vdd_log";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <800000>;
+		regulator-max-microvolt = <1400000>;
+		vin-supply = <&vcc_sys>;
+	};
+};
+
+&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>;
+};
+
+&cpu_b0 {
+	cpu-supply = <&vdd_cpu_b>;
+};
+
+&cpu_b1 {
+	cpu-supply = <&vdd_cpu_b>;
+};
+
+&emmc_phy {
+	status = "okay";
+};
+
+&gmac {
+	assigned-clocks = <&cru SCLK_RMII_SRC>;
+	assigned-clock-parents = <&clkin_gmac>;
+	clock_in_out = "input";
+	phy-supply = <&vcc3v3_s3>;
+	phy-mode = "rgmii";
+	pinctrl-names = "default";
+	pinctrl-0 = <&rgmii_pins>;
+	snps,reset-gpio = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>;
+	snps,reset-active-low;
+	snps,reset-delays-us = <0 10000 50000>;
+	tx_delay = <0x28>;
+	rx_delay = <0x11>;
+	status = "okay";
+};
+
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
+
+&hdmi {
+	ddc-i2c-bus = <&i2c3>;
+	status = "okay";
+};
+
+&hdmi_sound {
+	status = "okay";
+};
+
+&i2c0 {
+	clock-frequency = <400000>;
+	i2c-scl-rising-time-ns = <168>;
+	i2c-scl-falling-time-ns = <4>;
+	status = "okay";
+
+	rk808: pmic@1b {
+		compatible = "rockchip,rk808";
+		reg = <0x1b>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <21 IRQ_TYPE_LEVEL_LOW>;
+		#clock-cells = <1>;
+		clock-output-names = "rtc_clko_soc", "rtc_clko_wifi";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pmic_int_l>;
+		rockchip,system-power-controller;
+		wakeup-source;
+
+		vcc1-supply = <&vcc3v3_sys>;
+		vcc2-supply = <&vcc3v3_sys>;
+		vcc3-supply = <&vcc3v3_sys>;
+		vcc4-supply = <&vcc3v3_sys>;
+		vcc6-supply = <&vcc3v3_sys>;
+		vcc7-supply = <&vcc3v3_sys>;
+		vcc8-supply = <&vcc3v3_sys>;
+		vcc9-supply = <&vcc3v3_sys>;
+		vcc10-supply = <&vcc3v3_sys>;
+		vcc11-supply = <&vcc3v3_sys>;
+		vcc12-supply = <&vcc3v3_sys>;
+		vddio-supply = <&vcc_3v0>;
+
+		regulators {
+			vdd_center: DCDC_REG1 {
+				regulator-name = "vdd_center";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <700000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-ramp-delay = <6001>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vdd_cpu_l: DCDC_REG2 {
+				regulator-name = "vdd_cpu_l";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <700000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-ramp-delay = <6001>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc_ddr: DCDC_REG3 {
+				regulator-name = "vcc_ddr";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc_1v8: DCDC_REG4 {
+				regulator-name = "vcc_1v8";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vcc1v8_dvp: LDO_REG1 {
+				regulator-name = "vcc1v8_dvp";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc3v0_tp: LDO_REG2 {
+				regulator-name = "vcc3v0_tp";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc1v8_pmupll: LDO_REG3 {
+				regulator-name = "vcc1v8_pmupll";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <800000>;
+				regulator-max-microvolt = <2500000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vcc_sdio: LDO_REG4 {
+				regulator-name = "vcc_sdio";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3000000>;
+				};
+			};
+
+			vcca3v0_codec: LDO_REG5 {
+				regulator-name = "vcca3v0_codec";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc_1v5: LDO_REG6 {
+				regulator-name = "vcc_1v5";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <800000>;
+				regulator-max-microvolt = <2500000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1500000>;
+				};
+			};
+
+			vcca1v8_codec: LDO_REG7 {
+				regulator-name = "vcca1v8_codec";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <800000>;
+				regulator-max-microvolt = <2500000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc_3v0: LDO_REG8 {
+				regulator-name = "vcc_3v0";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3000000>;
+				};
+			};
+
+			vcc3v3_s3: SWITCH_REG1 {
+				regulator-name = "vcc3v3_s3";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc3v3_s0: SWITCH_REG2 {
+				regulator-name = "vcc3v3_s0";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+		};
+	};
+
+	vdd_cpu_b: regulator@40 {
+		compatible = "silergy,syr827";
+		reg = <0x40>;
+		fcs,suspend-voltage-selector = <1>;
+		regulator-name = "vdd_cpu_b";
+		regulator-min-microvolt = <712500>;
+		regulator-max-microvolt = <1500000>;
+		regulator-ramp-delay = <1000>;
+		regulator-always-on;
+		regulator-boot-on;
+		vin-supply = <&vcc3v3_sys>;
+
+		regulator-state-mem {
+			regulator-off-in-suspend;
+		};
+	};
+
+	vdd_gpu: regulator@41 {
+		compatible = "silergy,syr828";
+		reg = <0x41>;
+		fcs,suspend-voltage-selector = <1>;
+		regulator-name = "vdd_gpu";
+		regulator-min-microvolt = <712500>;
+		regulator-max-microvolt = <1500000>;
+		regulator-ramp-delay = <1000>;
+		regulator-always-on;
+		regulator-boot-on;
+		vin-supply = <&vcc3v3_sys>;
+
+		regulator-state-mem {
+			regulator-off-in-suspend;
+		};
+	};
+};
+
+&i2c1 {
+	i2c-scl-rising-time-ns = <450>;
+	i2c-scl-falling-time-ns = <15>;
+	status = "okay";
+};
+
+&i2c3 {
+	i2c-scl-rising-time-ns = <450>;
+	i2c-scl-falling-time-ns = <15>;
+	status = "okay";
+};
+
+&i2c4 {
+	clock-frequency = <400000>;
+	i2c-scl-rising-time-ns = <450>;
+	i2c-scl-falling-time-ns = <15>;
+	status = "okay";
+
+	ak09911@c {
+		compatible = "asahi-kasei,ak09911";
+		reg = <0x0c>;
+		vdd-supply = <&vcc3v3_s3>;
+	};
+
+	mpu6500@68 {
+		compatible = "invensense,mpu6500";
+		reg = <0x68>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <RK_PC6 IRQ_TYPE_EDGE_RISING>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&gsensor_int_l>;
+		vddio-supply = <&vcc3v3_s3>;
+	};
+
+	lsm6ds3@6a {
+		compatible = "st,lsm6ds3";
+		reg = <0x6a>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <RK_PD0 IRQ_TYPE_EDGE_RISING>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&gyr_int_l>;
+		vdd-supply = <&vcc3v3_s3>;
+		vddio-supply = <&vcc3v3_s3>;
+	};
+
+	cm32181@10 {
+		compatible = "capella,cm32181";
+		reg = <0x10>;
+		interrupt-parent = <&gpio4>;
+		interrupts = <RK_PD0 IRQ_TYPE_EDGE_RISING>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&light_int_l>;
+		vdd-supply = <&vcc3v3_s3>;
+	};
+};
+
+&io_domains {
+	status = "okay";
+	bt656-supply = <&vcc_3v0>;
+	audio-supply = <&vcca1v8_codec>;
+	sdmmc-supply = <&vcc_sdio>;
+	gpio1830-supply = <&vcc_3v0>;
+};
+
+&pmu_io_domains {
+	status = "okay";
+	pmu1830-supply = <&vcc_3v0>;
+};
+
+&pinctrl {
+	buttons {
+		pwr_btn: pwr-btn {
+			rockchip,pins = <0 RK_PA5 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+	};
+
+	pmic {
+		pmic_int_l: pmic-int-l {
+			rockchip,pins =
+				<1 RK_PC5 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+	};
+
+	sd {
+		sdmmc0_pwr_h: sdmmc0-pwr-h {
+			rockchip,pins =
+				<RK_GPIO0 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	usb2 {
+		vcc5v0_host_en: vcc5v0-host-en {
+			rockchip,pins =
+				<4 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+
+		vcc5v0_typec0_en: vcc5v0-typec0-en {
+			rockchip,pins =
+				<1 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	sdio-pwrseq {
+		wifi_reg_on_h: wifi-reg-on-h {
+			rockchip,pins = <0 RK_PB2 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	wifi {
+		wifi_host_wake_l: wifi-host-wake-l {
+			rockchip,pins = <0 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	bluetooth {
+		bt_reg_on_h: bt-enable-h {
+			rockchip,pins = <0 RK_PB1 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+
+		bt_host_wake_l: bt-host-wake-l {
+			rockchip,pins = <0 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+
+		bt_wake_l: bt-wake-l {
+			rockchip,pins = <2 RK_PD2 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	mpu6500 {
+		gsensor_int_l: gsensor-int-l {
+			rockchip,pins = <1 RK_PC6 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	lsm6ds3 {
+		gyr_int_l: gyr-int-l {
+			rockchip,pins = <1 RK_PD0 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	cm32181 {
+		light_int_l: light-int-l {
+			rockchip,pins = <4 RK_PD0 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+};
+
+&pwm0 {
+	status = "okay";
+};
+
+&pwm2 {
+	status = "okay";
+};
+
+&saradc {
+	vref-supply = <&vcca1v8_s3>;
+	status = "okay";
+};
+
+&sdhci {
+	bus-width = <8>;
+	mmc-hs400-1_8v;
+	mmc-hs400-enhanced-strobe;
+	non-removable;
+	status = "okay";
+};
+
+&sdio0 {
+	bus-width = <4>;
+	cap-sd-highspeed;
+	cap-sdio-irq;
+	clock-frequency = <50000000>;
+	disable-wp;
+	keep-power-in-suspend;
+	max-frequency = <50000000>;
+	mmc-pwrseq = <&sdio_pwrseq>;
+	non-removable;
+	pinctrl-names = "default";
+	pinctrl-0 = <&sdio0_bus4 &sdio0_cmd &sdio0_clk>;
+	sd-uhs-sdr104;
+	status = "okay";
+
+	brcmf: wifi@1 {
+		compatible = "brcm,bcm4329-fmac";
+		interrupt-parent = <&gpio0>;
+		interrupts = <RK_PA3 GPIO_ACTIVE_HIGH>;
+		interrupt-names = "host-wake";
+		pinctrl-names = "default";
+		pinctrl-0 = <&wifi_host_wake_l>;
+	};
+};
+
+&sdmmc {
+	bus-width = <4>;
+	cap-mmc-highspeed;
+	cap-sd-highspeed;
+	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
+	clock-frequency = <150000000>;
+	disable-wp;
+	max-frequency = <150000000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
+	vmmc-supply = <&vcc3v0_sd>;
+	vqmmc-supply = <&vcc_sdio>;
+	status = "okay";
+};
+
+&tcphy0 {
+	status = "okay";
+};
+
+&tcphy1 {
+	status = "okay";
+};
+
+&tsadc {
+	rockchip,hw-tshut-mode = <1>;
+	rockchip,hw-tshut-polarity = <1>;
+	status = "okay";
+};
+
+&u2phy0 {
+	status = "okay";
+
+	u2phy0_otg: otg-port {
+		phy-supply = <&vcc5v0_typec0>;
+		status = "okay";
+	};
+
+	u2phy0_host: host-port {
+		phy-supply = <&vcc5v0_host>;
+		status = "okay";
+	};
+};
+
+&u2phy1 {
+	status = "okay";
+
+	u2phy1_otg: otg-port {
+		status = "okay";
+	};
+
+	u2phy1_host: host-port {
+		phy-supply = <&vcc5v0_host>;
+		status = "okay";
+	};
+};
+
+&uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>;
+	status = "okay";
+
+	bluetooth {
+		compatible = "brcm,bcm43438-bt";
+		clocks = <&rk808 1>;
+		clock-names = "ext_clock";
+		device-wakeup-gpios = <&gpio2 RK_PD2 GPIO_ACTIVE_HIGH>;
+		host-wakeup-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_HIGH>;
+		shutdown-gpios = <&gpio0 RK_PB1 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&bt_host_wake_l &bt_wake_l &bt_reg_on_h>;
+	};
+};
+
+&uart2 {
+	status = "okay";
+};
+
+&usb_host0_ehci {
+	status = "okay";
+};
+
+&usb_host0_ohci {
+	status = "okay";
+};
+
+&usb_host1_ehci {
+	status = "okay";
+};
+
+&usb_host1_ohci {
+	status = "okay";
+};
+
+&usbdrd3_0 {
+	status = "okay";
+};
+
+&usbdrd_dwc3_0 {
+	status = "okay";
+	dr_mode = "otg";
+};
+
+&usbdrd3_1 {
+	status = "okay";
+};
+
+&usbdrd_dwc3_1 {
+	status = "okay";
+	dr_mode = "host";
+};
+
+&vopb {
+	status = "okay";
+};
+
+&vopb_mmu {
+	status = "okay";
+};
+
+&vopl {
+	status = "okay";
+};
+
+&vopl_mmu {
+	status = "okay";
+};
diff --git a/board/rockchip/evb_rk3399/MAINTAINERS b/board/rockchip/evb_rk3399/MAINTAINERS
index caad30641e..07ee8ce92c 100644
--- a/board/rockchip/evb_rk3399/MAINTAINERS
+++ b/board/rockchip/evb_rk3399/MAINTAINERS
@@ -5,3 +5,10 @@  F:      board/rockchip/evb_rk3399
 F:      include/configs/evb_rk3399.h
 F:      configs/evb-rk3399_defconfig
 F:      configs/firefly-rk3399_defconfig
+
+ORANGEPI-RK3399
+M:	Jagan Teki <jagan@amarulasolutions.com>
+S:	Maintained
+F:	configs/orangepi-rk3399_defconfig
+F:	arch/arm/dts/rk3399-u-boot.dtsi
+F:	arch/arm/dts/rk3399-orangepi-u-boot.dtsi
diff --git a/configs/orangepi-rk3399_defconfig b/configs/orangepi-rk3399_defconfig
new file mode 100644
index 0000000000..deb7bc1388
--- /dev/null
+++ b/configs/orangepi-rk3399_defconfig
@@ -0,0 +1,58 @@ 
+CONFIG_ARM=y
+CONFIG_ARCH_ROCKCHIP=y
+CONFIG_SYS_TEXT_BASE=0x00200000
+CONFIG_SPL_LIBCOMMON_SUPPORT=y
+CONFIG_SPL_LIBGENERIC_SUPPORT=y
+CONFIG_SYS_MALLOC_F_LEN=0x4000
+CONFIG_ROCKCHIP_RK3399=y
+CONFIG_ROCKCHIP_SPL_RESERVE_IRAM=0x4000
+CONFIG_DEBUG_UART_BASE=0xFF1A0000
+CONFIG_DEBUG_UART_CLOCK=24000000
+CONFIG_SPL_STACK_R_ADDR=0x80000
+CONFIG_DEBUG_UART=y
+CONFIG_NR_DRAM_BANKS=1
+CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-orangepi.dtb"
+# CONFIG_DISPLAY_CPUINFO is not set
+CONFIG_DISPLAY_BOARDINFO_LATE=y
+CONFIG_SPL_STACK_R=y
+CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x4000
+CONFIG_CMD_BOOTZ=y
+CONFIG_CMD_GPT=y
+CONFIG_CMD_MMC=y
+CONFIG_CMD_SF=y
+CONFIG_CMD_USB=y
+# CONFIG_CMD_SETEXPR is not set
+CONFIG_CMD_TIME=y
+CONFIG_SPL_OF_CONTROL=y
+CONFIG_DEFAULT_DEVICE_TREE="rk3399-orangepi"
+CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
+CONFIG_ENV_IS_IN_MMC=y
+CONFIG_ROCKCHIP_GPIO=y
+CONFIG_SYS_I2C_ROCKCHIP=y
+CONFIG_MMC_DW=y
+CONFIG_MMC_DW_ROCKCHIP=y
+CONFIG_MMC_SDHCI=y
+CONFIG_MMC_SDHCI_ROCKCHIP=y
+CONFIG_DM_ETH=y
+CONFIG_ETH_DESIGNWARE=y
+CONFIG_GMAC_ROCKCHIP=y
+CONFIG_PMIC_RK8XX=y
+CONFIG_REGULATOR_PWM=y
+CONFIG_REGULATOR_RK8XX=y
+CONFIG_PWM_ROCKCHIP=y
+CONFIG_BAUDRATE=1500000
+CONFIG_DEBUG_UART_SHIFT=2
+CONFIG_SYSRESET=y
+CONFIG_USB=y
+CONFIG_USB_XHCI_HCD=y
+CONFIG_USB_XHCI_DWC3=y
+CONFIG_USB_EHCI_HCD=y
+CONFIG_USB_EHCI_GENERIC=y
+CONFIG_USB_HOST_ETHER=y
+CONFIG_USB_ETHER_ASIX=y
+CONFIG_USB_ETHER_ASIX88179=y
+CONFIG_USB_ETHER_MCS7830=y
+CONFIG_USB_ETHER_RTL8152=y
+CONFIG_USB_ETHER_SMSC95XX=y
+CONFIG_USE_TINY_PRINTF=y
+CONFIG_ERRNO_STR=y