Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\))
Subject: Re: [PATCH] ARM64: dts: rockchip: add some pins to rk3399
From:   klaus.goger@theobroma-systems.com
In-Reply-To: <20180612152544.3812-1-ayaka@soulik.info>
Date:   Tue, 12 Jun 2018 20:21:06 +0200
Cc:     linux-rockchip@lists.infradead.org,
        Mark Rutland <mark.rutland@arm.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, Heiko Stuebner <heiko@sntech.de>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will.deacon@arm.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        Shawn Lin <shawn.lin@rock-chips.com>
Content-Transfer-Encoding: 8BIT
Message-Id: <B649911B-28E0-4466-B3F0-15875FFB9D43@theobroma-systems.com>
References: <20180612152544.3812-1-ayaka@soulik.info>
To:     Randy Li <ayaka@soulik.info>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Randy,

> On 12.06.2018, at 17:25, Randy Li <ayaka@soulik.info> wrote:
> 
> Those pins would be used by many boards.
> 
> Signed-off-by: Randy Li <ayaka@soulik.info>
> ---
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 97 +++++++++++++++++++++++++++-----
> 1 file changed, 83 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index e0040b648f43..9fa629857929 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -160,7 +160,7 @@
> 		};
> 	};
> 
> -	display-subsystem {
> +	display_subsystem: display-subsystem {

nitpick: that change is not pin related

> 		compatible = "rockchip,display-subsystem";
> 		ports = <&vopl_out>, <&vopb_out>;
> 	};
> @@ -1936,19 +1936,49 @@
> 			drive-strength = <12>;
> 		};
> 
> +		pcfg_pull_none_13ma: pcfg-pull-none-13ma {
> +			bias-disable;
> +			drive-strength = <13>;
> +		};
> +
> +		pcfg_pull_none_18ma: pcfg-pull-none-18ma {
> +			bias-disable;
> +			drive-strength = <18>;
> +		};
> +
> +		pcfg_pull_none_20ma: pcfg-pull-none-20ma {
> +			bias-disable;
> +			drive-strength = <20>;
> +		};
> +
> +		pcfg_pull_up_2ma: pcfg-pull-up-2ma {
> +			bias-pull-up;
> +			drive-strength = <2>;
> +		};
> +
> 		pcfg_pull_up_8ma: pcfg-pull-up-8ma {
> 			bias-pull-up;
> 			drive-strength = <8>;
> 		};
> 
> +		pcfg_pull_up_18ma: pcfg-pull-up-18ma {
> +			bias-pull-up;
> +			drive-strength = <18>;
> +		};
> +
> +		pcfg_pull_up_20ma: pcfg-pull-up-20ma {
> +			bias-pull-up;
> +			drive-strength = <20>;
> +		};
> +
> 		pcfg_pull_down_4ma: pcfg-pull-down-4ma {
> 			bias-pull-down;
> 			drive-strength = <4>;
> 		};
> 
> -		pcfg_pull_up_2ma: pcfg-pull-up-2ma {
> -			bias-pull-up;
> -			drive-strength = <2>;
> +		pcfg_pull_down_8ma: pcfg-pull-down-8ma {
> +			bias-pull-down;
> +			drive-strength = <8>;
> 		};
> 
> 		pcfg_pull_down_12ma: pcfg-pull-down-12ma {
> @@ -1956,9 +1986,22 @@
> 			drive-strength = <12>;
> 		};
> 
> -		pcfg_pull_none_13ma: pcfg-pull-none-13ma {
> -			bias-disable;
> -			drive-strength = <13>;
> +		pcfg_pull_down_18ma: pcfg-pull-down-18ma {
> +			bias-pull-down;
> +			drive-strength = <18>;
> +		};
> +
> +		pcfg_pull_down_20ma: pcfg-pull-down-20ma {
> +			bias-pull-down;
> +			drive-strength = <18>;

drive-strength = <20>?

> +		};
> +
> +		pcfg_output_high: pcfg-output-high {
> +			output-high;	

Trailing whitespace

> +		};
> +
> +		pcfg_output_low: pcfg-output-low {
> +			output-low;	

Trailing whitespace

> 		};
> 
> 		clock {
> @@ -2484,10 +2527,21 @@
> 					<4 18 RK_FUNC_1 &pcfg_pull_none>;
> 			};
> 
> +			pwm0_pin_pull_down: pwm0-pin-pull-down {
> +				rockchip,pins =
> +					<4 18 RK_FUNC_1 &pcfg_pull_down>;
> +			};
> +
> 			vop0_pwm_pin: vop0-pwm-pin {
> 				rockchip,pins =
> 					<4 18 RK_FUNC_2 &pcfg_pull_none>;
> 			};
> +
> +			vop1_pwm_pin: vop1-pwm-pin {
> +				rockchip,pins =
> +					<4 18 RK_FUNC_3 &pcfg_pull_none>;
> +			};
> +
> 		};
> 
> 		pwm1 {
> @@ -2496,9 +2550,9 @@
> 					<4 22 RK_FUNC_1 &pcfg_pull_none>;
> 			};
> 
> -			vop1_pwm_pin: vop1-pwm-pin {
> +			pwm1_pin_pull_down: pwm1-pin-pull-down {
> 				rockchip,pins =
> -					<4 18 RK_FUNC_3 &pcfg_pull_none>;
> +					<4 22 RK_FUNC_1 &pcfg_pull_down>;
> 			};
> 		};
> 
> @@ -2507,6 +2561,11 @@
> 				rockchip,pins =
> 					<1 19 RK_FUNC_1 &pcfg_pull_none>;
> 			};
> +
> +			pwm2_pin_pull_down: pwm2-pin-pull-down {
> +				rockchip,pins =
> +					<1 19 RK_FUNC_1 &pcfg_pull_none>;
> +			};

&pcfg_pull_down? 

> 		};
> 
> 		pwm3a {
> @@ -2526,25 +2585,35 @@
> 		hdmi {
> 			hdmi_i2c_xfer: hdmi-i2c-xfer {
> 				rockchip,pins =
> -					<4 RK_PC1 RK_FUNC_3 &pcfg_pull_none>,
> -					<4 RK_PC0 RK_FUNC_3 &pcfg_pull_none>;
> +					<4 17 RK_FUNC_3 &pcfg_pull_none>,
> +					<4 16 RK_FUNC_3 &pcfg_pull_none>;
> 			};
> 

Please keep the RK_Pxx macros.

> 			hdmi_cec: hdmi-cec {
> 				rockchip,pins =
> -					<4 RK_PC7 RK_FUNC_1 &pcfg_pull_none>;
> +					<4 23 RK_FUNC_1 &pcfg_pull_none>;
> 			};

Same

> 		};
> 
> 		pcie {
> +			pcie_clkreqn: pci-clkreqn {
> +				rockchip,pins =
> +					<2 26 RK_FUNC_2 &pcfg_pull_none>;
> +			};
> +
> +			pcie_clkreqnb: pci-clkreqnb {
> +				rockchip,pins =
> +					<4 24 RK_FUNC_1 &pcfg_pull_none>;
> +			};
> +

I’m not sure if pci-clkreqn is functional at all. If not I’m not sure if we should add it to the dtsi.
Shawn may know more about it.

> 			pcie_clkreqn_cpm: pci-clkreqn-cpm {
> 				rockchip,pins =
> -					<2 RK_PD2 RK_FUNC_GPIO &pcfg_pull_none>;
> +					<2 26 RK_FUNC_GPIO &pcfg_pull_none>;
> 			};
> 
> 			pcie_clkreqnb_cpm: pci-clkreqnb-cpm {
> 				rockchip,pins =
> -					<4 RK_PD0 RK_FUNC_GPIO &pcfg_pull_none>;
> +					<4 24 RK_FUNC_GPIO &pcfg_pull_none>;
> 			};
> 		};
> 
> -- 
> 2.14.4


Could we actually use RK_Pxx for all new pin definitions? Would increase readability a lot.

Thanks,
Klaus