Message-ID: <9c731c93-772e-409e-b7e5-ae36af402c76@kernel.org>
Date: Sat, 17 Aug 2024 13:32:11 +0200
From: Konrad Dybcio <konradybcio@...nel.org>
To: Song Xue <quic_songxue@...cinc.com>,
 Konrad Dybcio <konradybcio@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Wesley Cheng <quic_wcheng@...cinc.com>,
 Bjorn Andersson <andersson@...nel.org>
Cc: Marijn Suijten <marijn.suijten@...ainline.org>,
 linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Krishna Kurapati <quic_kriskura@...cinc.com>,
 Konrad Dybcio <quic_kdybcio@...cinc.com>
Subject: Re: [PATCH 2/2] arm64: dts: qcom: x1e80100: Add USB Multiport
 controller

On 14.08.2024 1:56 PM, Song Xue wrote:
> 
> 
> On 8/14/2024 6:24 PM, Konrad Dybcio wrote:
>> On 14.08.2024 12:08 PM, Song Xue wrote:
>>>
>>> On 8/9/2024 9:18 PM, Konrad Dybcio wrote:
>>>> X1E80100 has a multiport controller with 2 HS (eUSB) and 2 SS PHYs
>>>> attached to it. It's commonly used for USB-A ports and internally
>>>> routed devices. Configure it to support such functionality.
>>>>
>>>> Signed-off-by: Konrad Dybcio<konrad.dybcio@...aro.org>
>>>> ---
>>
>> [...]
>>
>>>> +
>>>> +                phys = <&usb_mp_hsphy0>, <&usb_mp_qmpphy0>,
>>>> +                       <&usb_mp_hsphy1>, <&usb_mp_qmpphy1>;
>>>> +                phy-names = "usb2-0", "usb3-0",
>>>> +                        "usb2-1", "usb3-1";
>>>> +                dr_mode = "host";
>>>
>>> Why do we add the dr_mode definition in dtsi file rather than in corresponding board dts file?  Could we follow the node "usb_1_ss1_dwc3"  in x1e80100-crd.dtsi?
>>
>> That is because the MP controller is host-only and it doesn't make sense
>> to ensure the OS of that in each board file separately. That's also how
>> it's done on other platforms with a MP controller description.
>>
>>>
>>> BTW, how do we verify the function of  multiport controller？From my test on x1e80100-crd,  the eusb6 which is from usb_mp_hsphy1 attaches the third-party repeater, do we need a new repeater node/driver to verify the function of eusb6?
>>
>> I have a X1E Surface Laptop 7 with a USB-A port with a NXP PTN3222 in
>> front of it. Tested with a smoke test, with both SS and HS USB-A devices.
>>
> What is detailed information on smoke test.
> From my end, I also have two questions.
> 1. I found the usb_mp_hsphy1 is using the driver "phy-qcom-snps-eusb2". However, the driver requires a repeater node from DT. At present, we don't have the node or driver for NXP repeater and it is not working on eusb6 to detect the NXP repeater. So, is it possible for us to have complete function involving with MP DT and repeater node for CRD board, and then we push patches together?

I believe you're a bit confused about the upstreaming process. Describing
hardware in Device Tree vs doing the same plus enabling it on some upstream
board are of equal value, and this patch is very much in the spirit of
"release early, release often".

There's no need to delay patches that are correct within their own
confinement (which they should be [1]) just so that the series is bigger.
That may even be discouraged by some folks..

> 2. The usb_mp_dwc3 node has four phys. When enabling the driver for the node, we must need enable all four phys in borad's DT. Howerver, if the board is only using one phy like eusb6, is it suitable to enable other three phys?

Yes, they will simply be registered, configured and since there won't
be any interrupts (as the pins are N/C, it will not do much). But
these PHYs are physically on the SoC regardless of them being
connected, so I see no issue.

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes

Konrad

Powered by blists - more mailing lists