[PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[Ziyang Huang]

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@outlook.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@1000000 {
 			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;
 			};
 		};
 
-- 
2.40.1

Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[Bryan O'Donoghue]

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@outlook.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@1000000 {
>   			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@78af000

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

Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[Konrad Dybcio]

On 1.09.2023 16: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).
Surely only one pair of wires is connected? Why is there an "OR"?

Konrad

Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[Ziyang Huang]

在 2023/9/1 23:04, Bryan O'Donoghue 写道:
> <...>
> 
> 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@78af000
> 
> 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
> 

No, please read my commit message carefully.

The Y of pinctrl is the index of pinmux config. So it can't be used in 
the serial node definition.

Please note that the physical port of first serial is configurable. It 
can use gpio20, gpio21 or/and gpio28,29. All of these pins are for the 
first serial.

Let's take the second serial as an example. It has 3 configurable 
physical port groups - "blsp1_uart0" (pinconfig name, use GPIO 
10,11,12,13), "blsp1_uart1" (gpio 31,32,33,34), "blsp1_uart2" (gpio 
23,24,25,26).

But the dts name of the second serial definition is "blsp1_uart2". 
Because it the second serial of the first BLSP block.

Same logic. The dts name of the first serial definition is 
"blsp1_uart1". Because it the first serial of the first BLSP block.

I think I need to introduce the architecture of these SoC. It has two 
BLSP block. Each BLSP block has several uart port.

So the dts name of serial contains the BLSP index and the serial index 
inside BLSP. But pinconf name doesn't care about it. So it use global 
index. And due to the physical ports are configurable, it need pinmux index.

The equation will be like this:

dts name of serial definition: "blspX_uartY"
pinconf name: "blspU_uartV"
U = (uart_number_inside_each_blsp * (X - 1)) + (Y - 1)

Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[Ziyang Huang]

在 2023/9/2 20:22, Konrad Dybcio 写道:
> On 1.09.2023 16: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).
> Surely only one pair of wires is connected? Why is there an "OR"?
> 
> Konrad

Because it is configurable. Please read my previous email.

And these two groups can be connected at same time. This is what u-boot 
does. It just like the parallel circuit. So there can be "or/and". lol...

Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[Krzysztof Kozlowski]

On 01/09/2023 16: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@outlook.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@1000000 {
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
>  
> -			uart1_pins: uart1-state {
> -				pins = "gpio31", "gpio32", "gpio33", "gpio34";
> -				function = "blsp1_uart1";
> +			uart1_pins_a: uart1@0 {

Regardless of other topics, change to @0 is neither correct, nor
explained in commit msg.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof

Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[Bryan O'Donoghue]

On 03/09/2023 14:02, Ziyang Huang wrote:
> 在 2023/9/1 23:04, Bryan O'Donoghue 写道:
>> <...>
>>
>> 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@78af000
>>
>> 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
>>
> 
> No, please read my commit message carefully.
> 
> The Y of pinctrl is the index of pinmux config. So it can't be used in 
> the serial node definition.
> 
> Please note that the physical port of first serial is configurable. It 
> can use gpio20, gpio21 or/and gpio28,29. All of these pins are for the 
> first serial.
> 
> Let's take the second serial as an example. It has 3 configurable 
> physical port groups - "blsp1_uart0" (pinconfig name, use GPIO 
> 10,11,12,13), "blsp1_uart1" (gpio 31,32,33,34), "blsp1_uart2" (gpio 
> 23,24,25,26).
> 
> But the dts name of the second serial definition is "blsp1_uart2". 
> Because it the second serial of the first BLSP block.
> 
> Same logic. The dts name of the first serial definition is 
> "blsp1_uart1". Because it the first serial of the first BLSP block.
> 
> I think I need to introduce the architecture of these SoC. It has two 
> BLSP block. Each BLSP block has several uart port.
> 
> So the dts name of serial contains the BLSP index and the serial index 
> inside BLSP. But pinconf name doesn't care about it. So it use global 
> index. And due to the physical ports are configurable, it need pinmux 
> index.
> 
> The equation will be like this:
> 
> dts name of serial definition: "blspX_uartY"
> pinconf name: "blspU_uartV"
> U = (uart_number_inside_each_blsp * (X - 1)) + (Y - 1)

I've checked the documentation for this chip.

gpio20, gpio21 = blsp0_uart0
gpio28, gpio29 = blsp0_uart0

These pins are muxed to UART0, I agree, the u-boot dts also indicates 
this also.

If we open the documentation further we see

0x78AF000 = BLSP1_BLSP_UART0
0x79b0000 = BLSP1_BLSP_UART1

So for starters the dtsi has the _wrong_ label.

Here/anseo

grep uart0: arch/arm64/boot/dts/qcom/*
arch/arm64/boot/dts/qcom/ipq5332.dtsi:		blsp1_uart0: serial@78af000 {
arch/arm64/boot/dts/qcom/ipq9574.dtsi:		blsp1_uart0: serial@78af000 {

That's how that label ought to be the main hint something is askance is 
assigning a pin named "blsp0_uart0" to a dts entry named "blsp1_uart1".

Please update the label in your next revision.

---
bod

Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[Sricharan Ramabadhran]

On 9/4/2023 6:27 AM, Bryan O'Donoghue wrote:
> On 03/09/2023 14:02, Ziyang Huang wrote:
>> 在 2023/9/1 23:04, Bryan O'Donoghue 写道:
>>> <...>
>>>
>>> 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@78af000
>>>
>>> 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
>>>
>>
>> No, please read my commit message carefully.
>>
>> The Y of pinctrl is the index of pinmux config. So it can't be used in 
>> the serial node definition.
>>
>> Please note that the physical port of first serial is configurable. It 
>> can use gpio20, gpio21 or/and gpio28,29. All of these pins are for the 
>> first serial.
>>
>> Let's take the second serial as an example. It has 3 configurable 
>> physical port groups - "blsp1_uart0" (pinconfig name, use GPIO 
>> 10,11,12,13), "blsp1_uart1" (gpio 31,32,33,34), "blsp1_uart2" (gpio 
>> 23,24,25,26).
>>
>> But the dts name of the second serial definition is "blsp1_uart2". 
>> Because it the second serial of the first BLSP block.
>>
>> Same logic. The dts name of the first serial definition is 
>> "blsp1_uart1". Because it the first serial of the first BLSP block.
>>
>> I think I need to introduce the architecture of these SoC. It has two 
>> BLSP block. Each BLSP block has several uart port.
>>
>> So the dts name of serial contains the BLSP index and the serial index 
>> inside BLSP. But pinconf name doesn't care about it. So it use global 
>> index. And due to the physical ports are configurable, it need pinmux 
>> index.
>>
>> The equation will be like this:
>>
>> dts name of serial definition: "blspX_uartY"
>> pinconf name: "blspU_uartV"
>> U = (uart_number_inside_each_blsp * (X - 1)) + (Y - 1)
> 
> I've checked the documentation for this chip.
> 
> gpio20, gpio21 = blsp0_uart0
> gpio28, gpio29 = blsp0_uart0
> 
> These pins are muxed to UART0, I agree, the u-boot dts also indicates 
> this also.
> 
> If we open the documentation further we see
> 
> 0x78AF000 = BLSP1_BLSP_UART0
> 0x79b0000 = BLSP1_BLSP_UART1
> 
> So for starters the dtsi has the _wrong_ label.
> 
> Here/anseo
> 
> grep uart0: arch/arm64/boot/dts/qcom/*
> arch/arm64/boot/dts/qcom/ipq5332.dtsi:        blsp1_uart0: serial@78af000 {
> arch/arm64/boot/dts/qcom/ipq9574.dtsi:        blsp1_uart0: serial@78af000 {
> 
> That's how that label ought to be the main hint something is askance is 
> assigning a pin named "blsp0_uart0" to a dts entry named "blsp1_uart1".
> 
> Please update the label in your next revision.
> 

   Agree here, both label (to blsp1_uart0) and pins needs to be updated
    (as in this patch).

Regards,
  Sricharan

Re: [PATCH v3] arm64: dts: ipq5018: Correct uart1_pins pinconf

[Ziyang Huang]

在 2023/9/4 8:57, Bryan O'Donoghue 写道:
> <...>
> 
> I've checked the documentation for this chip.
> 
> gpio20, gpio21 = blsp0_uart0
> gpio28, gpio29 = blsp0_uart0
> 
> These pins are muxed to UART0, I agree, the u-boot dts also indicates 
> this also.
> 
> If we open the documentation further we see
> 
> 0x78AF000 = BLSP1_BLSP_UART0
> 0x79b0000 = BLSP1_BLSP_UART1
> 
> So for starters the dtsi has the _wrong_ label.
> 
> Here/anseo
> 
> grep uart0: arch/arm64/boot/dts/qcom/*
> arch/arm64/boot/dts/qcom/ipq5332.dtsi:        blsp1_uart0: serial@78af000 {
> arch/arm64/boot/dts/qcom/ipq9574.dtsi:        blsp1_uart0: serial@78af000 {
> 
> That's how that label ought to be the main hint something is askance is 
> assigning a pin named "blsp0_uart0" to a dts entry named "blsp1_uart1".
> 
> Please update the label in your next revision.
> 
> ---
> bod

I think the root cause is the confused name in pinctrl. I will update 
the mux index to alphabetical order in next patch.

By the way, can you find out the documents about the pinmux map. For 
example, the code of pinctrl only show that GPIO20,21 are for UART0. But 
which pin is TX and which is RX? And yes, because of UART, it's easy to 
find out.

But what I want to known is "blsp2_spi". It has 3 pinmux configs - 
"blsp2_spi" (GPIO27), "blsp2_spi0" (GPIO31,32,33,34) and "blsp2_spi1" 
(GPIO23,24,25,26). What "blsp2_spi" (GPIO27) for?