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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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