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: <7355fb0a-f2a7-1af6-3369-a5996610e9cc@quicinc.com>
Date:   Tue, 5 Sep 2023 16:49:28 +0530
From:   Sricharan Ramabadhran <quic_srichara@...cinc.com>
To:     Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Ziyang Huang <hzyitc@...look.com>
CC:     <agross@...nel.org>, <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_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 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@...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
>>>
>>
>> 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@...f000 {
> arch/arm64/boot/dts/qcom/ipq9574.dtsi:        blsp1_uart0: serial@...f000 {
> 
> 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