[PATCH v3 0/3] PCI: qcom: sc7280: add missing aggre0, aggre1 and ddrs sf tbu clocks

[PATCH v3 0/3] PCI: qcom: sc7280: add missing aggre0, aggre1 and ddrs sf tbu clocks

[Krishna chaitanya chundru]

Add missing aggre0, aggre1 and ddrs sf tbu clocks supports to PCIe node.

Without voting these clocks, PCIe link is going down when system is
suspended as these clocks can get turned off as no-one is voting for them.

Krishna chaitanya chundru (3):
  PCI: qcom: Add sc7280 aggre0, aggre1 and ddr sf tbu clocks in PCIe
    driver
  dt-bindings: pci: QCOM Add missing sc7280 aggre0, aggre1 clocks
  arm64: dts: qcom: sc7280: Add missing aggre0, aggre1 clocks

 Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++----
 arch/arm64/boot/dts/qcom/sc7280.dtsi                 |  4 ++++
 drivers/pci/controller/dwc/pcie-qcom.c               |  3 +++
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] PCI: qcom: Add sc7280 aggre0, aggre1 and ddr sf tbu clocks in PCIe driver

[Krishna chaitanya chundru]

Add missing aggre0, aggre1 and ddrs sf tbu clocks in PCIe driver.

If these clocks are not presenti, the PCIe link goes down in system suspend
and resume.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 2ea1375..a7202f0 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1548,7 +1548,10 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
 static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
 	.has_tbu_clk = true,
+	.has_ddrss_sf_tbu_clk = true,
 	.pipe_clk_need_muxing = true,
+	.has_aggre0_clk = true,
+	.has_aggre1_clk = true,
 };
 
 static const struct qcom_pcie_cfg sc8180x_cfg = {
-- 
2.7.4

Re: [PATCH v3 1/3] PCI: qcom: Add sc7280 aggre0, aggre1 and ddr sf tbu clocks in PCIe driver

[Dmitry Baryshkov]

On Sat, 3 Sept 2022 at 05:23, Krishna chaitanya chundru
<quic_krichai@quicinc.com> wrote:
>
> Add missing aggre0, aggre1 and ddrs sf tbu clocks in PCIe driver.
>
> If these clocks are not presenti, the PCIe link goes down in system suspend
> and resume.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>

This does not apply onto the linux-next. If this is a fix intended for
the current master or for the linux-stable please state so.

> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 2ea1375..a7202f0 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1548,7 +1548,10 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
>  static const struct qcom_pcie_cfg sc7280_cfg = {
>         .ops = &ops_1_9_0,
>         .has_tbu_clk = true,
> +       .has_ddrss_sf_tbu_clk = true,
>         .pipe_clk_need_muxing = true,
> +       .has_aggre0_clk = true,
> +       .has_aggre1_clk = true,
>  };
>
>  static const struct qcom_pcie_cfg sc8180x_cfg = {
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 1/3] PCI: qcom: Add sc7280 aggre0, aggre1 and ddr sf tbu clocks in PCIe driver

[Bjorn Helgaas]

On Sat, Sep 03, 2022 at 07:43:02AM +0530, Krishna chaitanya chundru wrote:
> Add missing aggre0, aggre1 and ddrs sf tbu clocks in PCIe driver.
> 
> If these clocks are not presenti, the PCIe link goes down in system suspend
> and resume.

s/presenti/present/

But the hardware clocks are present regardless of this driver change.

I suspect the point of this is really that if the driver doesn't
clk_get() these clocks to increase the reference count, we don't know
that the clocks are in use, and since they appear unused, they get
turned off during suspend.

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 2ea1375..a7202f0 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1548,7 +1548,10 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
>  static const struct qcom_pcie_cfg sc7280_cfg = {
>  	.ops = &ops_1_9_0,
>  	.has_tbu_clk = true,
> +	.has_ddrss_sf_tbu_clk = true,
>  	.pipe_clk_need_muxing = true,
> +	.has_aggre0_clk = true,
> +	.has_aggre1_clk = true,
>  };
>  
>  static const struct qcom_pcie_cfg sc8180x_cfg = {
> -- 
> 2.7.4
> 

Re: [PATCH v3 1/3] PCI: qcom: Add sc7280 aggre0, aggre1 and ddr sf tbu clocks in PCIe driver

[Krishna Chaitanya Chundru]

On 9/8/2022 12:14 AM, Bjorn Helgaas wrote:
> On Sat, Sep 03, 2022 at 07:43:02AM +0530, Krishna chaitanya chundru wrote:
>> Add missing aggre0, aggre1 and ddrs sf tbu clocks in PCIe driver.
>>
>> If these clocks are not presenti, the PCIe link goes down in system suspend
>> and resume.
> s/presenti/present/
>
> But the hardware clocks are present regardless of this driver change.
>
> I suspect the point of this is really that if the driver doesn't
> clk_get() these clocks to increase the reference count, we don't know
> that the clocks are in use, and since they appear unused, they get
> turned off during suspend.

Yes, these are present in the hardware. As we are not voting for these 
clocks from

our driver in the system suspend these clocks can be turn off.

>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 2ea1375..a7202f0 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1548,7 +1548,10 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
>>   static const struct qcom_pcie_cfg sc7280_cfg = {
>>   	.ops = &ops_1_9_0,
>>   	.has_tbu_clk = true,
>> +	.has_ddrss_sf_tbu_clk = true,
>>   	.pipe_clk_need_muxing = true,
>> +	.has_aggre0_clk = true,
>> +	.has_aggre1_clk = true,
>>   };
>>   
>>   static const struct qcom_pcie_cfg sc8180x_cfg = {
>> -- 
>> 2.7.4
>>

[PATCH v3 2/3] dt-bindings: pci: QCOM Add missing sc7280 aggre0, aggre1 clocks

[Krishna chaitanya chundru]

Add missing aggre0 and aggre1 clocks.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
changes since v2:
	- Increase the max items of clock's in common properties.
---
 Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index 0b69b12..b759465 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -53,11 +53,11 @@ properties:
   # Platform constraints are described later.
   clocks:
     minItems: 3
-    maxItems: 12
+    maxItems: 13
 
   clock-names:
     minItems: 3
-    maxItems: 12
+    maxItems: 13
 
   resets:
     minItems: 1
@@ -423,8 +423,8 @@ allOf:
     then:
       properties:
         clocks:
-          minItems: 11
-          maxItems: 11
+          minItems: 13
+          maxItems: 13
         clock-names:
           items:
             - const: pipe # PIPE clock
@@ -437,6 +437,8 @@ allOf:
             - const: bus_slave # Slave AXI clock
             - const: slave_q2a # Slave Q2A clock
             - const: tbu # PCIe TBU clock
+            - const: aggre0 # Aggre NoC PCIe CENTER SF AXI clock
+            - const: aggre1 # Aggre NoC PCIe1 AXI clock
             - const: ddrss_sf_tbu # PCIe SF TBU clock
         resets:
           maxItems: 1
-- 
2.7.4

Re: [PATCH v3 2/3] dt-bindings: pci: QCOM Add missing sc7280 aggre0, aggre1 clocks

[Krzysztof Kozlowski]

On 03/09/2022 04:13, Krishna chaitanya chundru wrote:
> Add missing aggre0 and aggre1 clocks.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> changes since v2:
> 	- Increase the max items of clock's in common properties.
> ---
>  Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index 0b69b12..b759465 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -53,11 +53,11 @@ properties:
>    # Platform constraints are described later.
>    clocks:
>      minItems: 3
> -    maxItems: 12
> +    maxItems: 13
>  
>    clock-names:
>      minItems: 3
> -    maxItems: 12
> +    maxItems: 13
>  
>    resets:
>      minItems: 1
> @@ -423,8 +423,8 @@ allOf:
>      then:
>        properties:
>          clocks:
> -          minItems: 11
> -          maxItems: 11
> +          minItems: 13
> +          maxItems: 13
>          clock-names:
>            items:
>              - const: pipe # PIPE clock
> @@ -437,6 +437,8 @@ allOf:
>              - const: bus_slave # Slave AXI clock
>              - const: slave_q2a # Slave Q2A clock
>              - const: tbu # PCIe TBU clock
> +            - const: aggre0 # Aggre NoC PCIe CENTER SF AXI clock
> +            - const: aggre1 # Aggre NoC PCIe1 AXI clock
>              - const: ddrss_sf_tbu # PCIe SF TBU clock

Why adding them in the middle, not at the end of list? It does not match
other variants and affects the DTB ABI (order is strict).

Best regards,
Krzysztof

Re: [PATCH v3 2/3] dt-bindings: pci: QCOM Add missing sc7280 aggre0, aggre1 clocks

[Krishna Chaitanya Chundru]

On 9/7/2022 5:52 PM, Krzysztof Kozlowski wrote:
> On 03/09/2022 04:13, Krishna chaitanya chundru wrote:
>> Add missing aggre0 and aggre1 clocks.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>> changes since v2:
>> 	- Increase the max items of clock's in common properties.
>> ---
>>   Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> index 0b69b12..b759465 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> @@ -53,11 +53,11 @@ properties:
>>     # Platform constraints are described later.
>>     clocks:
>>       minItems: 3
>> -    maxItems: 12
>> +    maxItems: 13
>>   
>>     clock-names:
>>       minItems: 3
>> -    maxItems: 12
>> +    maxItems: 13
>>   
>>     resets:
>>       minItems: 1
>> @@ -423,8 +423,8 @@ allOf:
>>       then:
>>         properties:
>>           clocks:
>> -          minItems: 11
>> -          maxItems: 11
>> +          minItems: 13
>> +          maxItems: 13
>>           clock-names:
>>             items:
>>               - const: pipe # PIPE clock
>> @@ -437,6 +437,8 @@ allOf:
>>               - const: bus_slave # Slave AXI clock
>>               - const: slave_q2a # Slave Q2A clock
>>               - const: tbu # PCIe TBU clock
>> +            - const: aggre0 # Aggre NoC PCIe CENTER SF AXI clock
>> +            - const: aggre1 # Aggre NoC PCIe1 AXI clock
>>               - const: ddrss_sf_tbu # PCIe SF TBU clock
> Why adding them in the middle, not at the end of list? It does not match
> other variants and affects the DTB ABI (order is strict).
>
> Best regards,
> Krzysztof
Ok I will change the order as suggested.

[PATCH v3 3/3] arm64: dts: qcom: sc7280: Add missing aggre0, aggre1 clocks

[Krishna chaitanya chundru]

Add missing aggre0, aggre1 clocks.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index e66fc67..a5ce095 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2043,6 +2043,8 @@
 				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
 				 <&gcc GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
 				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
+				 <&gcc GCC_AGGRE_NOC_PCIE_CENTER_SF_AXI_CLK>,
+				 <&gcc GCC_AGGRE_NOC_PCIE_1_AXI_CLK>,
 				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
 
 			clock-names = "pipe",
@@ -2055,6 +2057,8 @@
 				      "bus_slave",
 				      "slave_q2a",
 				      "tbu",
+				      "aggre0",
+				      "aggre1",
 				      "ddrss_sf_tbu";
 
 			assigned-clocks = <&gcc GCC_PCIE_1_AUX_CLK>;
-- 
2.7.4

Re: [PATCH v3 3/3] arm64: dts: qcom: sc7280: Add missing aggre0, aggre1 clocks

[Krzysztof Kozlowski]

On 03/09/2022 04:13, Krishna chaitanya chundru wrote:
> Add missing aggre0, aggre1 clocks.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index e66fc67..a5ce095 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -2043,6 +2043,8 @@
>  				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
>  				 <&gcc GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
>  				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
> +				 <&gcc GCC_AGGRE_NOC_PCIE_CENTER_SF_AXI_CLK>,
> +				 <&gcc GCC_AGGRE_NOC_PCIE_1_AXI_CLK>,
>  				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
>  
>  			clock-names = "pipe",
> @@ -2055,6 +2057,8 @@
>  				      "bus_slave",
>  				      "slave_q2a",
>  				      "tbu",
> +				      "aggre0",
> +				      "aggre1",
>  				      "ddrss_sf_tbu";
>  

Same as binding - adding entries in the middle causes ABI issues.

Best regards,
Krzysztof

Re: [PATCH v3 3/3] arm64: dts: qcom: sc7280: Add missing aggre0, aggre1 clocks

[Krishna Chaitanya Chundru]

On 9/7/2022 5:52 PM, Krzysztof Kozlowski wrote:
> On 03/09/2022 04:13, Krishna chaitanya chundru wrote:
>> Add missing aggre0, aggre1 clocks.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index e66fc67..a5ce095 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -2043,6 +2043,8 @@
>>   				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
>>   				 <&gcc GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
>>   				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
>> +				 <&gcc GCC_AGGRE_NOC_PCIE_CENTER_SF_AXI_CLK>,
>> +				 <&gcc GCC_AGGRE_NOC_PCIE_1_AXI_CLK>,
>>   				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
>>   
>>   			clock-names = "pipe",
>> @@ -2055,6 +2057,8 @@
>>   				      "bus_slave",
>>   				      "slave_q2a",
>>   				      "tbu",
>> +				      "aggre0",
>> +				      "aggre1",
>>   				      "ddrss_sf_tbu";
>>   
> Same as binding - adding entries in the middle causes ABI issues.
>
> Best regards,
> Krzysztof
Ok I will change the order as suggested.