Re: [PATCH v8 1/3] arm64: dts: qcom: sm8750: Add USB support to SM8750 SoCs

[Krishna Kurapati PSSNV <krishna.kurapati@oss.qualcomm.com>]

On 10/23/2025 2:03 AM, Bjorn Andersson wrote:
> On Wed, Oct 22, 2025 at 02:10:50PM +0530, Krishna Kurapati wrote:
>> From: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
>>
>> Add the base USB devicetree definitions for SM8750 platforms.  The overall
> 
> Please start your commit message with the problem description and leave
> the description of the "solution" to later.
> 
> If you replace "overall" with "SM8750" the second sentence is a good
> start.
> 
>> chipset contains a single DWC3 USB3 controller (rev. 200a), SS QMP PHY
>> (rev. v8) and M31 eUSB2 PHY.  The major difference for SM8750 is the
> 
> "The major difference from previous SoCs is the..."
> 
>> transition to using the M31 eUSB2 PHY compared to previous SoCs.
>>
>> Enable USB support on SM8750 MTP and QRD variants. SM8750 has a QMP combo
>> PHY for the SSUSB path, and a M31 eUSB2 PHY for the HSUSB path.
>>
>> Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
>> Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> This means "Konrad suggested that I implement this patch".
> 
>> [Konrad: Suggestion to flatten DT]
> 
> This syntax is for "patch was originally authored by above, but "name"
> changed it in so-and-so way".
> 
> In other words, while the gesture of giving Konrad credit for his
> suggestion during review is nice, you should omit the Suggested-by and
> you should cover bigger things you changed since Wesley wrote the patch,
> i.e. say:
> 
> [krishna: Flattened dwc3 node]
> 

My bad.

Actually Wesley implemented the original patch.
Konrad provided a diff to flatten it.
I sent it.

Let me rephrase the [] block as follows:
[Konrad: Flattened the dwc3 node]

Hope that would be clear.

>> Signed-off-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8750.dtsi | 158 +++++++++++++++++++++++++++
>>   1 file changed, 158 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>> index a82d9867c7cb..d933c378bd8d 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>> @@ -12,6 +12,7 @@
>>   #include <dt-bindings/interconnect/qcom,sm8750-rpmh.h>
>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>   #include <dt-bindings/mailbox/qcom-ipcc.h>
>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
>>   #include <dt-bindings/power/qcom,rpmhpd.h>
>>   #include <dt-bindings/power/qcom-rpmpd.h>
>>   #include <dt-bindings/soc/qcom,gpr.h>
>> @@ -2581,6 +2582,163 @@ data-pins {
>>   			};
>>   		};
>>   
>> +		usb_1_hsphy: phy@88e3000 {
>> +			compatible = "qcom,sm8750-m31-eusb2-phy";
>> +			reg = <0x0 0x88e3000 0x0 0x29c>;
>> +
>> +			clocks = <&tcsrcc TCSR_USB2_CLKREF_EN>;
>> +			clock-names = "ref";
>> +
>> +			resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>;
>> +
>> +			#phy-cells = <0>;
>> +
>> +			status = "disabled";
>> +		};
>> +
>> +		usb_dp_qmpphy: phy@88e8000 {
>> +			compatible = "qcom,sm8750-qmp-usb3-dp-phy";
>> +			reg = <0x0 0x088e8000 0x0 0x4000>;
>> +
>> +			clocks = <&gcc GCC_USB3_PRIM_PHY_AUX_CLK>,
>> +				 <&tcsrcc TCSR_USB3_CLKREF_EN>,
>> +				 <&gcc GCC_USB3_PRIM_PHY_COM_AUX_CLK>,
>> +				 <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
>> +			clock-names = "aux",
>> +				      "ref",
>> +				      "com_aux",
>> +				      "usb3_pipe";
>> +
>> +			resets = <&gcc GCC_USB3_PHY_PRIM_BCR>,
>> +				 <&gcc GCC_USB3_DP_PHY_PRIM_BCR>;
>> +			reset-names = "phy",
>> +				      "common";
>> +
>> +			power-domains = <&gcc GCC_USB3_PHY_GDSC>;
>> +
>> +			#clock-cells = <1>;
>> +			#phy-cells = <1>;
>> +
>> +			orientation-switch;
>> +
>> +			status = "disabled";
>> +
>> +			ports {
>> +				#address-cells = <1>;
>> +				#size-cells = <0>;
>> +
>> +				port@0 {
>> +					reg = <0>;
>> +
>> +					usb_dp_qmpphy_out: endpoint {
>> +					};
>> +				};
>> +
>> +				port@1 {
>> +					reg = <1>;
>> +
>> +					usb_dp_qmpphy_usb_ss_in: endpoint {
>> +						remote-endpoint = <&usb_1_dwc3_ss>;
>> +					};
>> +				};
>> +
>> +				port@2 {
>> +					reg = <2>;
>> +
>> +					usb_dp_qmpphy_dp_in: endpoint {
>> +					};
>> +				};
>> +			};
>> +		};
>> +
>> +		usb_1: usb@a600000 {
> 
> Commit message says there's a single USB controller, so why does it need
> a _1 suffix? (Same with usb_1_hsphy above)
> 

ACK.
Will use just "usb" as node name.

Regards,
Krishna,