PCI: qcom: Add basic interconnect support
Subsystems (5)
cover PCI: qcom: Add basic interconnect support
On Qualcomm platforms like SC8280XP and SA8540P interconnect bandwidth must be requested before enabling interconnect clocks.
Add basic support for managing an optional "pcie-mem" interconnect path by setting a low constraint before enabling clocks and updating it after the link is up.
This is specifically needed to prevent a crash on SC8280XP/SA8540P when the interconnect constraints are enforced during boot.
As support for these platforms was added in 6.1-rc1 it would be nice to have this merged as a fix for 6.1, but deferring for 6.2 works as well.
Johan
Johan Hovold (2):
dt-bindings: PCI: qcom: Add SC8280XP/SA8540P interconnects
PCI: qcom: Add basic interconnect support
.../devicetree/bindings/pci/qcom,pcie.yaml | 25 ++++++ drivers/pci/controller/dwc/pcie-qcom.c | 76 +++++++++++++++++++ 2 files changed, 101 insertions(+)
Signature · 1 line
2.37.3
PATCH 1/2 dt-bindings: PCI: qcom: Add SC8280XP/SA8540P interconnects
Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect paths to the bindings.
--- .../devicetree/bindings/pci/qcom,pcie.yaml | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/pci/qcom,pcie.yaml +25 -0
@@ -62,6 +62,12 @@ properties: minItems: 3 maxItems: 12 + interconnects: + maxItems: 2 + + interconnect-names: + maxItems: 2 + resets: minItems: 1 maxItems: 12
@@ -629,6 +635,25 @@ allOf: items: - const: pci # PCIe core reset + - if: + properties: + compatible: + contains: + enum: + - qcom,pcie-sa8540p + - qcom,pcie-sc8280xp + then: + properties: + interconnects: + maxItems: 2 + interconnect-names: + items: + - const: pcie-mem + - const: cpu-pcie + required: + - interconnects + - interconnect-names + - if: not: properties:
Signature · 1 line
2.37.3
Re: [PATCH 1/2] dt-bindings: PCI: qcom: Add SC8280XP/SA8540P interconnects
Quoted 32 lines · Johan Hovold
Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect paths to the bindings.
Fixes 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")Fixes 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")Signed-off-by Johan Hovold <[email protected]>--- .../devicetree/bindings/pci/qcom,pcie.yaml | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+)Documentation/devicetree/bindings/pci/qcom,pcie.yaml +7
-0@@ -62,6 +62,12 @@ properties: minItems: 3 maxItems: 12 + interconnects: + maxItems: 2 + + interconnect-names: + maxItems: 2 + resets: minItems: 1 maxItems: 12@@ -629,6 +635,25 @@ allOf: items: - const: pci # PCIe core reset + - if:
Documentation/devicetree/bindings/pci/qcom,pcie.yaml allOf: · line 648
Quoted 10 lines
Documentation/devicetree/bindings/pci/qcom,pcie.yaml +10
-0+ properties: + compatible: + contains: + enum: + - qcom,pcie-sa8540p + - qcom,pcie-sc8280xp + then: + properties: + interconnects: + maxItems: 2
No need for this.
Documentation/devicetree/bindings/pci/qcom,pcie.yaml allOf: · line 655
Quoted 7 lines
Documentation/devicetree/bindings/pci/qcom,pcie.yaml +7
-0+ interconnect-names: + items: + - const: pcie-mem + - const: cpu-pcie + required: + - interconnects + - interconnect-names
else:
??
Otherwise, you allow any names for other variants.
Best regards,
Krzysztof
Re: [PATCH 1/2] dt-bindings: PCI: qcom: Add SC8280XP/SA8540P interconnects
Quoted 54 lines · Krzysztof Kozlowski
Quoted 32 lines · Johan Hovold
Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect paths to the bindings.
Fixes 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")Fixes 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")Signed-off-by Johan Hovold <[email protected]>--- .../devicetree/bindings/pci/qcom,pcie.yaml | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+)Documentation/devicetree/bindings/pci/qcom,pcie.yaml +7
-0@@ -62,6 +62,12 @@ properties: minItems: 3 maxItems: 12 + interconnects: + maxItems: 2 + + interconnect-names: + maxItems: 2 + resets: minItems: 1 maxItems: 12@@ -629,6 +635,25 @@ allOf: items: - const: pci # PCIe core reset + - if:Documentation/devicetree/bindings/pci/qcom,pcie.yaml allOf: · line 648
Quoted 10 lines
Documentation/devicetree/bindings/pci/qcom,pcie.yaml +10
-0+ properties: + compatible: + contains: + enum: + - qcom,pcie-sa8540p + - qcom,pcie-sc8280xp + then: + properties: + interconnects: + maxItems: 2No need for this.
Documentation/devicetree/bindings/pci/qcom,pcie.yaml allOf: · line 655
Quoted 7 lines
Documentation/devicetree/bindings/pci/qcom,pcie.yaml +7
-0+ interconnect-names: + items: + - const: pcie-mem + - const: cpu-pcie + required: + - interconnects + - interconnect-names
Quoted 4 lines
else:
??Otherwise, you allow any names for other variants.
Are you suggesting something like moving the names to the common constraints for now:
interconnects:
maxItems: 2
interconnect-names:
items:
- const: pcie-mem
- const: cpu-pcie
and then in the allOf:
- if:
properties:
compatible:
contains:
enum:
- qcom,pcie-sa8540p
- qcom,pcie-sc8280xp
then:
required:
- interconnects
- interconnect-names
else:
properties:
interconnects: false
interconnect-names: false
This way we'd catch anyone adding interconnects to a DTS without first updating the bindings, but it also seems to go against the idea of bindings fully describing the hardware by saying that no other platforms have interconnects (when they actually do even if we don't describe it just yet).
Or should we do the above but without the else clause to have some constraints in place on the names at least?
Johan
Re: [PATCH 1/2] dt-bindings: PCI: qcom: Add SC8280XP/SA8540P interconnects
Quoted 89 lines · Johan Hovold
Quoted 54 lines · Krzysztof Kozlowski
Quoted 32 lines · Johan Hovold
Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect paths to the bindings.
Fixes 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")Fixes 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")Signed-off-by Johan Hovold <[email protected]>--- .../devicetree/bindings/pci/qcom,pcie.yaml | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+)Documentation/devicetree/bindings/pci/qcom,pcie.yaml +7
-0@@ -62,6 +62,12 @@ properties: minItems: 3 maxItems: 12 + interconnects: + maxItems: 2 + + interconnect-names: + maxItems: 2 + resets: minItems: 1 maxItems: 12@@ -629,6 +635,25 @@ allOf: items: - const: pci # PCIe core reset + - if:Documentation/devicetree/bindings/pci/qcom,pcie.yaml allOf: · line 648
Quoted 10 lines
Documentation/devicetree/bindings/pci/qcom,pcie.yaml +10
-0+ properties: + compatible: + contains: + enum: + - qcom,pcie-sa8540p + - qcom,pcie-sc8280xp + then: + properties: + interconnects: + maxItems: 2No need for this.
Documentation/devicetree/bindings/pci/qcom,pcie.yaml allOf: · line 655
Quoted 7 lines
Documentation/devicetree/bindings/pci/qcom,pcie.yaml +7
-0+ interconnect-names: + items: + - const: pcie-mem + - const: cpu-pcie + required: + - interconnects + - interconnect-namesQuoted 4 lines
else:
??Otherwise, you allow any names for other variants.
Are you suggesting something like moving the names to the common constraints for now:
interconnects:
maxItems: 2interconnect-names:
items:
- const: pcie-mem
- const: cpu-pcieand then in the allOf:
- if:
properties:
compatible:
contains:
enum:
- qcom,pcie-sa8540p
- qcom,pcie-sc8280xp
then:
required:
- interconnects
- interconnect-names
else:
properties:
interconnects: false
interconnect-names: false
Quoted 5 lines
This way we'd catch anyone adding interconnects to a DTS without first updating the bindings, but it also seems to go against the idea of bindings fully describing the hardware by saying that no other platforms have interconnects (when they actually do even if we don't describe it just yet).
You can add a comment to the else like "TODO: Not described yet". I would prefer to have specific but incomplete bindings, instead of loose one which later might cause people adding whatever names they like.
Quoted 2 lines
Or should we do the above but without the else clause to have some constraints in place on the names at least?
This would work as well if you think the names are applicable for other devices.
Best regards,
Krzysztof
Re: [PATCH 1/2] dt-bindings: PCI: qcom: Add SC8280XP/SA8540P interconnects
Quoted 57 lines · Krzysztof Kozlowski
Quoted 46 lines · Johan Hovold
Quoted 11 lines · Krzysztof Kozlowski
Quoted 10 lines · Johan Hovold
Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect paths to the bindings.
Fixes 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")Fixes 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")Signed-off-by Johan Hovold <[email protected]>--- .../devicetree/bindings/pci/qcom,pcie.yaml | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+)Quoted 4 lines
Documentation/devicetree/bindings/pci/qcom,pcie.yaml
Quoted 4 lines
Documentation/devicetree/bindings/pci/qcom,pcie.yaml +0
-0Are you suggesting something like moving the names to the common constraints for now:
interconnects:
maxItems: 2interconnect-names:
items:
- const: pcie-mem
- const: cpu-pcieand then in the allOf:
- if:
properties:
compatible:
contains:
enum:
- qcom,pcie-sa8540p
- qcom,pcie-sc8280xp
then:
required:
- interconnects
- interconnect-names
else:
properties:
interconnects: false
interconnect-names: falseQuoted 5 lines
This way we'd catch anyone adding interconnects to a DTS without first updating the bindings, but it also seems to go against the idea of bindings fully describing the hardware by saying that no other platforms have interconnects (when they actually do even if we don't describe it just yet).
You can add a comment to the else like "TODO: Not described yet". I would prefer to have specific but incomplete bindings, instead of loose one which later might cause people adding whatever names they like.
Quoted 5 lines
Quoted 2 lines
Or should we do the above but without the else clause to have some constraints in place on the names at least?
This would work as well if you think the names are applicable for other devices.
I think that's a reasonable assumption so I'll go with this alternative.
Thanks!
Johan
PATCH 2/2 PCI: qcom: Add basic interconnect support
In reply to Johan HovoldOn Qualcomm platforms like SC8280XP and SA8540P interconnect bandwidth must be requested before enabling interconnect clocks.
Add basic support for managing an optional "pcie-mem" interconnect path by setting a low constraint before enabling clocks and updating it after the link is up.
Note that it is not possible for a controller driver to set anything but a maximum peak bandwidth as expected average bandwidth will vary with use case and actual use (and power policy?). This very much remains an unresolved problem with the interconnect framework.
Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie" path for now as it is not clear what an appropriate constraint would be (and the system does not crash when left unspecified currently).
--- drivers/pci/controller/dwc/pcie-qcom.c | 76 ++++++++++++++++++++++++++ 1 file changed, 76 insertions(+)
drivers/pci/controller/dwc/pcie-qcom.c +76 -0
@@ -12,6 +12,7 @@#include <linux/crc8.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> +#include <linux/interconnect.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/iopoll.h>
@@ -224,6 +225,7 @@ struct qcom_pcie {union qcom_pcie_resources res;struct phy *phy;struct gpio_desc *reset; +struct icc_path *icc_mem;const struct qcom_pcie_cfg *cfg; };
@@ -1644,6 +1646,74 @@ static const struct dw_pcie_ops dw_pcie_ops = { .start_link = qcom_pcie_start_link, }; +static int qcom_pcie_icc_init (struct qcom_pcie *pcie) +{ +struct dw_pcie *pci = pcie->pci; +int ret; + + pcie->icc_mem =devm_of_icc_get (pci->dev,"pcie-mem" ); +if (IS_ERR (pcie->icc_mem)) { + ret =PTR_ERR (pcie->icc_mem); +return ret; + } + +/* +* Some Qualcomm platforms require interconnect bandwidth constraints +* to be set before enabling interconnect clocks. +* +* Set an initial peak bandwidth corresponding to single-lane Gen 1 +* for the pcie-mem path. +*/ + ret =icc_set_bw (pcie->icc_mem,0 ,MBps_to_icc (250 )); +if (ret) { +dev_err (pci->dev,"failed to set interconnect bandwidth: %d\n" , + ret); +return ret; + } + +return 0 ; +} + +static void qcom_pcie_icc_update (struct qcom_pcie *pcie) +{ +struct dw_pcie *pci = pcie->pci; +u32 offset, status, bw; +int speed, width; +int ret; + +if (!pcie->icc_mem) +return ; + + offset =dw_pcie_find_capability (pci,PCI_CAP_ID_EXP ); + status =readw (pci->dbi_base + offset +PCI_EXP_LNKSTA ); + +/* Only update constraints if link is up. */ +if (!(status &PCI_EXP_LNKSTA_DLLLA )) +return ; + + speed =FIELD_GET (PCI_EXP_LNKSTA_CLS , status); + width =FIELD_GET (PCI_EXP_LNKSTA_NLW , status); + +switch (speed) { +case 1 : + bw =MBps_to_icc (250 ); +break ; +case 2 : + bw =MBps_to_icc (500 ); +break ; +default : +case 3 : + bw =MBps_to_icc (985 ); +break ; + } + + ret =icc_set_bw (pcie->icc_mem,0 , width * bw); +if (ret) { +dev_err (pci->dev,"failed to set interconnect bandwidth: %d\n" , + ret); + } +} +static int qcom_pcie_probe (struct platform_device *pdev) {struct device *dev = &pdev->dev;
@@ -1704,6 +1774,10 @@ static int qcom_pcie_probe(struct platform_device *pdev) goto err_pm_runtime_put; } + ret =qcom_pcie_icc_init (pcie); +if (ret) + goto err_pm_runtime_put; + ret = pcie->cfg->ops->get_resources (pcie);if (ret) goto err_pm_runtime_put;
@@ -1722,6 +1796,8 @@ static int qcom_pcie_probe(struct platform_device *pdev) goto err_phy_exit; } +qcom_pcie_icc_update (pcie); +return 0 ; err_phy_exit:
Signature · 1 line
2.37.3
Quoted 8 lines · Johan Hovold
drivers/pci/controller/dwc/pcie-qcom.c +8
-0+/* +* Some Qualcomm platforms require interconnect bandwidth constraints +* to be set before enabling interconnect clocks. +* +* Set an initial peak bandwidth corresponding to single-lane Gen 1 +* for the pcie-mem path. +*/ + ret =icc_set_bw (pcie->icc_mem,0 ,MBps_to_icc (250 ));
[snip]
Quoted 5 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-0+ speed =FIELD_GET (PCI_EXP_LNKSTA_CLS , status); + width =FIELD_GET (PCI_EXP_LNKSTA_NLW , status); + +switch (speed) { +case 1 :
drivers/pci/controller/dwc/pcie-qcom.c static const struct dw_pcie_ops dw_pcie_ops = { · line 1708
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-0+ bw =MBps_to_icc (250 ); +break ; +case 2 : + bw =MBps_to_icc (500 ); +break ; +default : +case 3 : + bw =MBps_to_icc (985 ); +break ; + }
Just curious: These platforms have a 4 lane PCIe bus. Why use 985 instead of 1000 for the maximum?
Brian
Quoted 22 lines · Brian Masney
Quoted 8 lines · Johan Hovold
drivers/pci/controller/dwc/pcie-qcom.c +8
-0+/* +* Some Qualcomm platforms require interconnect bandwidth constraints +* to be set before enabling interconnect clocks. +* +* Set an initial peak bandwidth corresponding to single-lane Gen 1 +* for the pcie-mem path. +*/ + ret =icc_set_bw (pcie->icc_mem,0 ,MBps_to_icc (250 ));[snip]
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-0+ speed =FIELD_GET (PCI_EXP_LNKSTA_CLS , status); + width =FIELD_GET (PCI_EXP_LNKSTA_NLW , status); + +switch (speed) { +case 1 : + bw =MBps_to_icc (250 ); +break ; +case 2 : + bw =MBps_to_icc (500 ); +break ;
Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c static const struct dw_pcie_ops dw_pcie_ops = { · line 1708
Quoted 5 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-0+default : +case 3 : + bw =MBps_to_icc (985 ); +break ; + }Just curious: These platforms have a 4 lane PCIe bus. Why use 985 instead of 1000 for the maximum?
This is the per-lane bandwidth that depends on encoding. The four-lane peak throughput would be about 3.94 GB/s.
Johan
Quoted 12 lines · Johan Hovold
On Qualcomm platforms like SC8280XP and SA8540P interconnect bandwidth must be requested before enabling interconnect clocks.
Add basic support for managing an optional "pcie-mem" interconnect path by setting a low constraint before enabling clocks and updating it after the link is up.
Note that it is not possible for a controller driver to set anything but a maximum peak bandwidth as expected average bandwidth will vary with use case and actual use (and power policy?). This very much remains an unresolved problem with the interconnect framework.
Quoted 6 lines
Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie" path for now as it is not clear what an appropriate constraint would be (and the system does not crash when left unspecified currently).
Fixes 70574511f3fc ("PCI: qcom: Add support for SC8280XP")Signed-off-by Johan Hovold <[email protected]>