diff mbox series

[07/16] arm64: dts: qcom: sm8550-aim300: add PCIe0

Message ID 20231117101817.4401-8-quic_tengfan@quicinc.com (mailing list archive)
State Changes Requested
Headers show
Series arm64: qcom: add sm8550-aim300 board support | expand

Commit Message

Tengfei Fan Nov. 17, 2023, 10:18 a.m. UTC
Add PCIe0 nodes used with WCN7851 device.  The PCIe1 is not connected,
thus skip pcie_1_phy_aux_clk input clock to GCC.

Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8550-aim300.dts | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Dmitry Baryshkov Nov. 17, 2023, 10:29 a.m. UTC | #1
On 17/11/2023 12:18, Tengfei Fan wrote:
> Add PCIe0 nodes used with WCN7851 device.  The PCIe1 is not connected,
> thus skip pcie_1_phy_aux_clk input clock to GCC.
> 
> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/sm8550-aim300.dts | 32 ++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
> index 202b979da8ca..3aca0a433a00 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
> @@ -393,6 +393,38 @@
>   	};
>   };
>   
> +&gcc {
> +	clocks = <&bi_tcxo_div2>, <&sleep_clk>,
> +		 <&pcie0_phy>,
> +		 <&pcie1_phy>,
> +		 <0>,
> +		 <&ufs_mem_phy 0>,
> +		 <&ufs_mem_phy 1>,
> +		 <&ufs_mem_phy 2>,
> +		 <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
> +};

NAK, this should go to sm8550.dtsi unless there is a good reason.

> +
> +&pcie_1_phy_aux_clk {
> +	status = "disabled";
> +};
> +
> +&pcie0 {
> +	perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
> +	wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
> +
> +	pinctrl-0 = <&pcie0_default_state>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +};
> +
> +&pcie0_phy {
> +	vdda-phy-supply = <&vreg_l1e_0p88>;
> +	vdda-pll-supply = <&vreg_l3e_1p2>;
> +
> +	status = "okay";
> +};
> +
>   &pm8550b_eusb2_repeater {
>   	vdd18-supply = <&vreg_l15b_1p8>;
>   	vdd3-supply = <&vreg_l5b_3p1>;
Krzysztof Kozlowski Nov. 17, 2023, 10:30 a.m. UTC | #2
On 17/11/2023 11:18, Tengfei Fan wrote:
> Add PCIe0 nodes used with WCN7851 device.  The PCIe1 is not connected,
> thus skip pcie_1_phy_aux_clk input clock to GCC.
> 
> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>

You just added this board. Does it mean you added incomplete and wrong DTSU?

Best regards,
Krzysztof
Neil Armstrong Nov. 17, 2023, 10:41 a.m. UTC | #3
On 17/11/2023 11:29, Dmitry Baryshkov wrote:
> On 17/11/2023 12:18, Tengfei Fan wrote:
>> Add PCIe0 nodes used with WCN7851 device.  The PCIe1 is not connected,
>> thus skip pcie_1_phy_aux_clk input clock to GCC.
>>
>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8550-aim300.dts | 32 ++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>> index 202b979da8ca..3aca0a433a00 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>> +++ b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>> @@ -393,6 +393,38 @@
>>       };
>>   };
>> +&gcc {
>> +    clocks = <&bi_tcxo_div2>, <&sleep_clk>,
>> +         <&pcie0_phy>,
>> +         <&pcie1_phy>,
>> +         <0>,
>> +         <&ufs_mem_phy 0>,
>> +         <&ufs_mem_phy 1>,
>> +         <&ufs_mem_phy 2>,
>> +         <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
>> +};
> 
> NAK, this should go to sm8550.dtsi unless there is a good reason.

Actually this is how QRD8550 was designed, so it's fine to mimic.

Neil

> 
>> +
>> +&pcie_1_phy_aux_clk {
>> +    status = "disabled";
>> +};
>> +
>> +&pcie0 {
>> +    perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
>> +    wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
>> +
>> +    pinctrl-0 = <&pcie0_default_state>;
>> +    pinctrl-names = "default";
>> +
>> +    status = "okay";
>> +};
>> +
>> +&pcie0_phy {
>> +    vdda-phy-supply = <&vreg_l1e_0p88>;
>> +    vdda-pll-supply = <&vreg_l3e_1p2>;
>> +
>> +    status = "okay";
>> +};
>> +
>>   &pm8550b_eusb2_repeater {
>>       vdd18-supply = <&vreg_l15b_1p8>;
>>       vdd3-supply = <&vreg_l5b_3p1>;
>
Konrad Dybcio Nov. 18, 2023, 12:08 a.m. UTC | #4
On 17.11.2023 11:41, neil.armstrong@linaro.org wrote:
> On 17/11/2023 11:29, Dmitry Baryshkov wrote:
>> On 17/11/2023 12:18, Tengfei Fan wrote:
>>> Add PCIe0 nodes used with WCN7851 device.  The PCIe1 is not connected,
>>> thus skip pcie_1_phy_aux_clk input clock to GCC.
>>>
>>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/sm8550-aim300.dts | 32 ++++++++++++++++++++++
>>>   1 file changed, 32 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>>> index 202b979da8ca..3aca0a433a00 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>>> @@ -393,6 +393,38 @@
>>>       };
>>>   };
>>> +&gcc {
>>> +    clocks = <&bi_tcxo_div2>, <&sleep_clk>,
>>> +         <&pcie0_phy>,
>>> +         <&pcie1_phy>,
>>> +         <0>,
>>> +         <&ufs_mem_phy 0>,
>>> +         <&ufs_mem_phy 1>,
>>> +         <&ufs_mem_phy 2>,
>>> +         <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
>>> +};
>>
>> NAK, this should go to sm8550.dtsi unless there is a good reason.
> 
> Actually this is how QRD8550 was designed, so it's fine to mimic.
Does CCF not handle this gracefully?

Konrad
Neil Armstrong Nov. 19, 2023, 5:59 p.m. UTC | #5
Le 18/11/2023 à 01:08, Konrad Dybcio a écrit :
> On 17.11.2023 11:41, neil.armstrong@linaro.org wrote:
>> On 17/11/2023 11:29, Dmitry Baryshkov wrote:
>>> On 17/11/2023 12:18, Tengfei Fan wrote:
>>>> Add PCIe0 nodes used with WCN7851 device.  The PCIe1 is not connected,
>>>> thus skip pcie_1_phy_aux_clk input clock to GCC.
>>>>
>>>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/sm8550-aim300.dts | 32 ++++++++++++++++++++++
>>>>    1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>>>> index 202b979da8ca..3aca0a433a00 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>>>> +++ b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>>>> @@ -393,6 +393,38 @@
>>>>        };
>>>>    };
>>>> +&gcc {
>>>> +    clocks = <&bi_tcxo_div2>, <&sleep_clk>,
>>>> +         <&pcie0_phy>,
>>>> +         <&pcie1_phy>,
>>>> +         <0>,
>>>> +         <&ufs_mem_phy 0>,
>>>> +         <&ufs_mem_phy 1>,
>>>> +         <&ufs_mem_phy 2>,
>>>> +         <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
>>>> +};
>>>
>>> NAK, this should go to sm8550.dtsi unless there is a good reason.
>>
>> Actually this is how QRD8550 was designed, so it's fine to mimic.
> Does CCF not handle this gracefully?

CCF handles this very gracefully and it's a perfectly valid DT in regard
to the bindings...

neil

> 
> Konrad
Tengfei Fan Nov. 21, 2023, 12:40 a.m. UTC | #6
在 11/20/2023 1:59 AM, Neil Armstrong 写道:
> Le 18/11/2023 à 01:08, Konrad Dybcio a écrit :
>> On 17.11.2023 11:41, neil.armstrong@linaro.org wrote:
>>> On 17/11/2023 11:29, Dmitry Baryshkov wrote:
>>>> On 17/11/2023 12:18, Tengfei Fan wrote:
>>>>> Add PCIe0 nodes used with WCN7851 device.  The PCIe1 is not connected,
>>>>> thus skip pcie_1_phy_aux_clk input clock to GCC.
>>>>>
>>>>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/qcom/sm8550-aim300.dts | 32 
>>>>> ++++++++++++++++++++++
>>>>>    1 file changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts 
>>>>> b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>>>>> index 202b979da8ca..3aca0a433a00 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
>>>>> @@ -393,6 +393,38 @@
>>>>>        };
>>>>>    };
>>>>> +&gcc {
>>>>> +    clocks = <&bi_tcxo_div2>, <&sleep_clk>,
>>>>> +         <&pcie0_phy>,
>>>>> +         <&pcie1_phy>,
>>>>> +         <0>,
>>>>> +         <&ufs_mem_phy 0>,
>>>>> +         <&ufs_mem_phy 1>,
>>>>> +         <&ufs_mem_phy 2>,
>>>>> +         <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
>>>>> +};
>>>>
>>>> NAK, this should go to sm8550.dtsi unless there is a good reason.
>>>
>>> Actually this is how QRD8550 was designed, so it's fine to mimic.
>> Does CCF not handle this gracefully?
> 
> CCF handles this very gracefully and it's a perfectly valid DT in regard
> to the bindings...
> 
> neil
> 
>>
>> Konrad
> 
Thanks Konrad and Neil comments and disscusion this patch, I also will 
confirm this with internal team.
Tengfei Fan Nov. 21, 2023, 12:51 a.m. UTC | #7
在 11/17/2023 6:30 PM, Krzysztof Kozlowski 写道:
> On 17/11/2023 11:18, Tengfei Fan wrote:
>> Add PCIe0 nodes used with WCN7851 device.  The PCIe1 is not connected,
>> thus skip pcie_1_phy_aux_clk input clock to GCC.
>>
>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
> 
> You just added this board. Does it mean you added incomplete and wrong DTSU?
> 
> Best regards,
> Krzysztof
> 

Hi Krzysztof,
I will drop PCIe1 setting in dts file because of PCIe1 still have not 
enable in dts file.
Another I understand what your comments means is I should combine all 
the functions which should be implemented together and submit as a 
complete patch, right?
I will combine all the functions patch to a total patch when I do next 
version patch series, because there is another your comments also want 
to me do as so.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
index 202b979da8ca..3aca0a433a00 100644
--- a/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
+++ b/arch/arm64/boot/dts/qcom/sm8550-aim300.dts
@@ -393,6 +393,38 @@ 
 	};
 };
 
+&gcc {
+	clocks = <&bi_tcxo_div2>, <&sleep_clk>,
+		 <&pcie0_phy>,
+		 <&pcie1_phy>,
+		 <0>,
+		 <&ufs_mem_phy 0>,
+		 <&ufs_mem_phy 1>,
+		 <&ufs_mem_phy 2>,
+		 <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
+};
+
+&pcie_1_phy_aux_clk {
+	status = "disabled";
+};
+
+&pcie0 {
+	perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
+	wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
+
+	pinctrl-0 = <&pcie0_default_state>;
+	pinctrl-names = "default";
+
+	status = "okay";
+};
+
+&pcie0_phy {
+	vdda-phy-supply = <&vreg_l1e_0p88>;
+	vdda-pll-supply = <&vreg_l3e_1p2>;
+
+	status = "okay";
+};
+
 &pm8550b_eusb2_repeater {
 	vdd18-supply = <&vreg_l15b_1p8>;
 	vdd3-supply = <&vreg_l5b_3p1>;