linux-kernel - Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b6fa8337-a5c0-172a-a41b-ab18de3f4f72@linaro.org>
Date:   Fri, 1 Sep 2023 16:04:06 +0100
From:   Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To:     Ziyang Huang <hzyitc@...look.com>, agross@...nel.org
Cc:     andersson@...nel.org, konrad.dybcio@...aro.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        quic_gokulsri@...cinc.com, quic_srichara@...cinc.com,
        quic_varada@...cinc.com, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

On 01/09/2023 15:10, Ziyang Huang wrote:
> In pinctrl, the pinconfigs for uart are named "blspX_uartY".
>    X is the UART ID. Starts from 1.
>      1-6 are in BLSP Block 1.
>      7-12 are in BLSP Block 2.
>    Y is the index of mux config. Starts from 0.
> 
> In dts, the serials are also named "blspX_uartY", but with different logic.
>    X is the BLSP Block ID. Starts from 1.
>    Y is the uart id inside block.
>      In "ipq6018.dtsi" and "ipq8074.dtsi", it starts from 1.
>      But in "ipq5332.dtsi" and "ipq9574.dtsi", it starts from 0.
> 
> +-----------------+-----------------+-------------+-----------------+
> |     Block ID    | ID inside Block |  dts name   | pinconfig name  |
> | (Starts from 1) | (Starts from 1) |             |                 |
> +-----------------+-----------------+-------------+-----------------+
> |        1        |        1        | blsp1_uart1 |   blsp0_uartY   |
> |        1        |        2        | blsp1_uart2 |   blsp1_uartY   |
> |        1        |        6        | blsp1_uart6 |   blsp5_uartY   |
> |        2        |        1        | blsp2_uart1 |   blsp6_uartY   |
> |        2        |        6        | blsp2_uart6 |   blsp12_uartY  |
> +-----------------+-----------------+-------------+-----------------+
> 
> In "ipq5018.dts", "blsp1_uart1" (dts name) is the first serial (confimed
> by the address), So its pinconfig should be "blsp0_uart0" (pinconfig name,
> use GPIO 20 and 21) or "blsp0_uart1" (pinconfig name, use GPIO 28 and 29).
> 
> Fixes: 570006756a16 ("arm64: dts: Add ipq5018 SoC and rdp432-c2 board support")
> Signed-off-by: Ziyang Huang <hzyitc@...look.com>
> ---
> Changes since v1:
> - Use corrent name in From
> 
> Changes since v2:
> - Define 2 pinconfs for uart1 in ipq5018.dtsi
> - rdp432-c2 use uart1_pins_a
> 
>   arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts |  2 +-
>   arch/arm64/boot/dts/qcom/ipq5018.dtsi          | 15 +++++++++++----
>   2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> index e636a1cb9b77..e83d1863e89c 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> @@ -23,7 +23,7 @@ chosen {
>   };
>   
>   &blsp1_uart1 {
> -	pinctrl-0 = <&uart1_pins>;
> +	pinctrl-0 = <&uart1_pins_a>;
>   	pinctrl-names = "default";
>   	status = "okay";
>   };
> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> index 9f13d2dcdfd5..50b4a2bd6fd3 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> @@ -103,11 +103,18 @@ tlmm: pinctrl@...0000 {
>   			interrupt-controller;
>   			#interrupt-cells = <2>;
>   
> -			uart1_pins: uart1-state {
> -				pins = "gpio31", "gpio32", "gpio33", "gpio34";
> -				function = "blsp1_uart1";
> +			uart1_pins_a: uart1@0 {
> +				pins = "gpio20", "gpio21";
> +				function = "blsp0_uart0";
>   				drive-strength = <8>;
> -				bias-pull-down;
> +				bias-disabled;
> +			};
> +
> +			uart1_pins_b: uart1@1 {
> +				pins = "gpio28", "gpio29";
> +				function = "blsp0_uart1";
> +				drive-strength = <8>;
> +				bias-disabled;
>   			};
>   		};
>   

The assignment of pins 20 and 21 to blsp1_uart1 is not correct.

The blspX_uartY in pinctrl should match what is in the dtsi so assigning 
pins_a above to blsp1_uart1 is not right. The dts name and pinctrl name 
should be the same.

Your console is on blsp0_uart0.

https://git.codelinaro.org/clo/qsdk/oss/boot/u-boot-2016/-/blob/5343739b4070bcec2fecd72f758c16adc31a3083/arch/arm/dts/ipq5018-mp03.3.dts#L33

So roughly speaking

arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts

aliases {
	serial0 = &blsp0_uart0;
};

chosen {
	stdout-path = "serial0:115200n8";
};

&blsp0_uart0 {
         pinctrl-0 = <&uart0_pins>;
         pinctrl-names = "default";
         status = "okay";
};


arch/arm64/boot/dts/qcom/ipq5018.dtsi

blsp0_uart0: serial@...f000

either that or  blsp0_uart1 for pins28 and pins29 - you seem to indicate 
pins_1 => blsp0_uart0.

The two roots of the problem are

1. Mislabeling of the uart block in the dtsi
2. Invalid miscongiruation of pins for that misnamed block

The fix should be

1. Fix the labeling of uart in the dtsi
2. Decide on which pins gpio20, gpio21 ? are the right ones to configure

I thought you said in a previous email if you changed pins gpio28 and 
gpio29 that the UART would fail if so that implies blsp0_uart1.

Either way the pinctrl and dts should agree.

---
bod