This patch adds support for OPP to vote for the performance state of RPMH
power domain based upon GEN speed it PCIe got enumerated.
QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
maintains hardware state of a regulator by performing max aggregation of
the requests made by all of the processors.
PCIe controller can operate on different RPMh performance state of power
domain based up on the speed of the link. And this performance state varies
from target to target.
It is manadate to scale the performance state based up on the PCIe speed
link operates so that SoC can run under optimum power conditions.
Add Operating Performance Points(OPP) support to vote for RPMh state based
upon GEN speed link is operating.
Before link up PCIe driver will vote for the maximum performance state.
As now we are adding ICC BW vote in OPP, the ICC BW voting depends both
GEN speed and link width using opp-level to indicate the opp entry table
will be difficult.
In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
same icc bw if we use freq in the opp table to represent the PCIe Gen
speed number of PCIe entries can reduced.
So going back to use freq in the opp table instead of level.
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
Changes frm v5:
- Add ICC BW voting as part of OPP, rebase the latest kernel, and only
- either OPP or ICC BW voting will supported we removed the patch to
- return eror for icc opp update patch.
- As we added the icc bw voting in opp table I am not including reviewed
- by tags given in previous patch.
- Use opp freq to find opp entries as now we need to include pcie link
- also in to considerations.
- Add CPU-PCIe BW voting which is not present till now.
- Drop PCI: qcom: Return error from 'qcom_pcie_icc_update' as either opp or icc bw
- only one executes and there is no need to fail if opp or icc update fails.
- Link for v5: https://lore.kernel.org/linux-arm-msm/20231101063323.GH2897@thinkpad/T/
Changes from v4:
- Added a separate patch for returning error from the qcom_pcie_upadate
and moved opp update logic to icc_update and used a bool variable to
update the opp.
- Addressed comments made by pavan.
changes from v3:
- Removing the opp vote on suspend when the link is not up and link is not
up and add debug prints as suggested by pavan.
- Added dev_pm_opp_find_level_floor API to find the highest opp to vote.
changes from v2:
- Instead of using the freq based opp search use level based as suggested
by Dmitry Baryshkov.
Changes from v1:
- Addressed comments from Krzysztof Kozlowski.
- Added the rpmhpd_opp_xxx phandle as suggested by pavan.
- Added dev_pm_opp_set_opp API call which was missed on previous patch.
---
Krishna chaitanya chundru (6):
dt-bindings: PCI: qcom: Add interconnects path as required property
arm64: dts: qcom: sm8450: Add interconnect path to PCIe node
PCI: qcom: Add missing icc bandwidth vote for cpu to PCIe path
dt-bindings: pci: qcom: Add opp table
arm64: dts: qcom: sm8450: Add opp table support to PCIe
PCI: qcom: Add OPP support to scale performance state of power domain
.../devicetree/bindings/pci/qcom,pcie.yaml | 6 ++
arch/arm64/boot/dts/qcom/sm8450.dtsi | 82 +++++++++++++++
drivers/pci/controller/dwc/pcie-qcom.c | 114 ++++++++++++++++++---
3 files changed, 187 insertions(+), 15 deletions(-)
---
base-commit: 70d201a40823acba23899342d62bc2644051ad2e
change-id: 20240112-opp_support-3a1839c6a171
Best regards,
--
Krishna chaitanya chundru <[email protected]>
Add the interconnects path as required property for sm8450 platform.
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index eadba38171e1..bc28669f6fa0 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -777,6 +777,8 @@ allOf:
- qcom,pcie-sa8540p
- qcom,pcie-sa8775p
- qcom,pcie-sc8280xp
+ - qcom,pcie-sm8450-pcie0
+ - qcom,pcie-sm8450-pcie1
then:
required:
- interconnects
--
2.42.0
Add pcie-mem & cpu-pcie interconnect path to the PCIe nodes.
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
arch/arm64/boot/dts/qcom/sm8450.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 01e4dfc4babd..6b1d2e0d9d14 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -1781,6 +1781,10 @@ pcie0: pcie@1c00000 {
<0 0 0 3 &intc 0 0 0 151 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
<0 0 0 4 &intc 0 0 0 152 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+ interconnects = <&pcie_noc MASTER_PCIE_0 0 &mc_virt SLAVE_EBI1 0>,
+ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_PCIE_0 0>;
+ interconnect-names = "pcie-mem", "cpu-pcie";
+
clocks = <&gcc GCC_PCIE_0_PIPE_CLK>,
<&gcc GCC_PCIE_0_PIPE_CLK_SRC>,
<&pcie0_phy>,
@@ -1890,6 +1894,10 @@ pcie1: pcie@1c08000 {
<0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
<0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+ interconnects = <&pcie_noc MASTER_PCIE_1 0 &mc_virt SLAVE_EBI1 0>,
+ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_PCIE_1 0>;
+ interconnect-names = "pcie-mem", "cpu-pcie";
+
clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
<&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
<&pcie1_phy>,
--
2.42.0
CPU-PCIe path consits for registers PCIe BAR space, config space.
As there is less access on this path compared to pcie to mem path
add minimum vote i.e GEN1x1 bandwidth always.
In suspend remove the cpu vote after register space access is done.
Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
cc: [email protected]
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 11c80555d975..035953f0b6d8 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -240,6 +240,7 @@ struct qcom_pcie {
struct phy *phy;
struct gpio_desc *reset;
struct icc_path *icc_mem;
+ struct icc_path *icc_cpu;
const struct qcom_pcie_cfg *cfg;
struct dentry *debugfs;
bool suspended;
@@ -1372,6 +1373,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
if (IS_ERR(pcie->icc_mem))
return PTR_ERR(pcie->icc_mem);
+ pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
+ if (IS_ERR(pcie->icc_cpu))
+ return PTR_ERR(pcie->icc_cpu);
/*
* Some Qualcomm platforms require interconnect bandwidth constraints
* to be set before enabling interconnect clocks.
@@ -1381,7 +1385,18 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
*/
ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
if (ret) {
- dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+ dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
+ ret);
+ return ret;
+ }
+
+ /*
+ * The config space, BAR space and registers goes through cpu-pcie path.
+ * Set peak bandwidth to single-lane Gen1 for this path all the time.
+ */
+ ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
+ if (ret) {
+ dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n",
ret);
return ret;
}
@@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
*/
ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
if (ret) {
- dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
+ dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
return ret;
}
@@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
pcie->suspended = true;
}
+ /* Remove cpu path vote after all the register access is done */
+ ret = icc_set_bw(pcie->icc_cpu, 0, 0);
+ if (ret) {
+ dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
+ return ret;
+ }
return 0;
}
@@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
struct qcom_pcie *pcie = dev_get_drvdata(dev);
int ret;
+ ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
+ if (ret) {
+ dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
+ return ret;
+ }
+
if (pcie->suspended) {
ret = qcom_pcie_host_init(&pcie->pci->pp);
if (ret)
--
2.42.0
PCIe needs to choose the appropriate performance state of RPMH power
domain based upon the PCIe gen speed.
Adding the Operating Performance Points table allows to adjust power
domain performance state and icc peak bw, depending on the PCIe gen
speed and width.
Acked-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index bc28669f6fa0..a37b2ef7dbfc 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -122,6 +122,10 @@ properties:
description: GPIO controlled connection to WAKE# signal
maxItems: 1
+ operating-points-v2: true
+ opp-table:
+ type: object
+
required:
- compatible
- reg
--
2.42.0
PCIe needs to choose the appropriate performance state of RPMH power
domain and interconnect bandwidth based up on the PCIe gen speed.
Add the OPP table support to specify RPMH performance states and
interconnect peak bandwidth.
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
arch/arm64/boot/dts/qcom/sm8450.dtsi | 74 ++++++++++++++++++++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 6b1d2e0d9d14..eab85ecaeff0 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -1827,7 +1827,32 @@ pcie0: pcie@1c00000 {
pinctrl-names = "default";
pinctrl-0 = <&pcie0_default_state>;
+ operating-points-v2 = <&pcie0_opp_table>;
+
status = "disabled";
+
+ pcie0_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ opp-2500000 {
+ opp-hz = /bits/ 64 <2500000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <250000 250000>;
+ };
+
+ opp-5000000 {
+ opp-hz = /bits/ 64 <5000000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <500000 250000>;
+ };
+
+ opp-8000000 {
+ opp-hz = /bits/ 64 <8000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <984500 250000>;
+ };
+ };
+
};
pcie0_phy: phy@1c06000 {
@@ -1938,7 +1963,56 @@ pcie1: pcie@1c08000 {
pinctrl-names = "default";
pinctrl-0 = <&pcie1_default_state>;
+ operating-points-v2 = <&pcie1_opp_table>;
+
status = "disabled";
+
+ pcie1_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ /* GEN 1x1 */
+ opp-2500000 {
+ opp-hz = /bits/ 64 <2500000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <250000 250000>;
+ };
+
+ /* GEN 1x2 GEN 2x1 */
+ opp-5000000 {
+ opp-hz = /bits/ 64 <5000000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <500000 250000>;
+ };
+
+ /* GEN 2x2 */
+ opp-10000000 {
+ opp-hz = /bits/ 64 <10000000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <1000000 250000>;
+ };
+
+ /* GEN 3x1 */
+ opp-8000000 {
+ opp-hz = /bits/ 64 <8000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <984500 250000>;
+ };
+
+ /* GEN 3x2 GEN 4x1 */
+ opp-16000000 {
+ opp-hz = /bits/ 64 <16000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <1969000 250000>;
+ };
+
+ /* GEN 4x2 */
+ opp-32000000 {
+ opp-hz = /bits/ 64 <32000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <3938000 250000>;
+ };
+ };
+
};
pcie1_phy: phy@1c0e000 {
--
2.42.0
QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
maintains hardware state of a regulator by performing max aggregation of
the requests made by all of the processors.
PCIe controller can operate on different RPMh performance state of power
domain based up on the speed of the link. And this performance state varies
from target to target.
It is manadate to scale the performance state based up on the PCIe speed
link operates so that SoC can run under optimum power conditions.
Add Operating Performance Points(OPP) support to vote for RPMh state based
upon GEN speed link is operating.
OPP can handle ICC bw voting also, so move icc bw voting through opp
framework if opp entries are present.
In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
same icc bw and has frequency, so use frequency based search to reduce
number of entries in the opp table.
Don't initialize icc if opp is supported.
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 83 ++++++++++++++++++++++++++++------
1 file changed, 70 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 035953f0b6d8..31512dc9d6ff 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -22,6 +22,7 @@
#include <linux/of.h>
#include <linux/of_gpio.h>
#include <linux/pci.h>
+#include <linux/pm_opp.h>
#include <linux/pm_runtime.h>
#include <linux/platform_device.h>
#include <linux/phy/pcie.h>
@@ -244,6 +245,7 @@ struct qcom_pcie {
const struct qcom_pcie_cfg *cfg;
struct dentry *debugfs;
bool suspended;
+ bool opp_supported;
};
#define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
@@ -1404,16 +1406,14 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
return 0;
}
-static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
+static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
{
struct dw_pcie *pci = pcie->pci;
- u32 offset, status;
+ u32 offset, status, freq;
+ struct dev_pm_opp *opp;
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);
@@ -1424,11 +1424,42 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
- ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
- if (ret) {
- dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
- ret);
+ if (pcie->opp_supported) {
+ switch (speed) {
+ case 1:
+ freq = 2500000;
+ break;
+ case 2:
+ freq = 5000000;
+ break;
+ case 3:
+ freq = 8000000;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ fallthrough;
+ case 4:
+ freq = 16000000;
+ break;
+ }
+
+ opp = dev_pm_opp_find_freq_exact(pci->dev, freq * width, true);
+ if (!IS_ERR(opp)) {
+ ret = dev_pm_opp_set_opp(pci->dev, opp);
+ if (ret)
+ dev_err(pci->dev, "Failed to set opp: freq %ld ret %d\n",
+ dev_pm_opp_get_freq(opp), ret);
+ dev_pm_opp_put(opp);
+ }
+ } else {
+ ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
+ if (ret) {
+ dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
+ ret);
+ }
}
+
+ return;
}
static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
@@ -1471,8 +1502,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
static int qcom_pcie_probe(struct platform_device *pdev)
{
const struct qcom_pcie_cfg *pcie_cfg;
+ unsigned long max_freq = INT_MAX;
struct device *dev = &pdev->dev;
struct qcom_pcie *pcie;
+ struct dev_pm_opp *opp;
struct dw_pcie_rp *pp;
struct resource *res;
struct dw_pcie *pci;
@@ -1539,9 +1572,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
goto err_pm_runtime_put;
}
- ret = qcom_pcie_icc_init(pcie);
- if (ret)
+ /* OPP table is optional */
+ ret = devm_pm_opp_of_add_table(dev);
+ if (ret && ret != -ENODEV) {
+ dev_err_probe(dev, ret, "Failed to add OPP table\n");
goto err_pm_runtime_put;
+ }
+
+ /* vote for max freq in the opp table if opp table is present */
+ if (ret != -ENODEV) {
+ opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
+ if (!IS_ERR(opp)) {
+ ret = dev_pm_opp_set_opp(dev, opp);
+ if (ret)
+ dev_err_probe(pci->dev, ret,
+ "Failed to set opp: freq %ld\n",
+ dev_pm_opp_get_freq(opp));
+ dev_pm_opp_put(opp);
+ }
+ pcie->opp_supported = true;
+ }
+
+ /* Skip icc init if opp is supported as icc bw vote is handled by opp framework */
+ if (!pcie->opp_supported) {
+ ret = qcom_pcie_icc_init(pcie);
+ if (ret)
+ goto err_pm_runtime_put;
+ }
ret = pcie->cfg->ops->get_resources(pcie);
if (ret)
@@ -1561,7 +1618,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
goto err_phy_exit;
}
- qcom_pcie_icc_update(pcie);
+ qcom_pcie_icc_opp_update(pcie);
if (pcie->mhi)
qcom_pcie_init_debugfs(pcie);
@@ -1640,7 +1697,7 @@ static int qcom_pcie_resume_noirq(struct device *dev)
pcie->suspended = false;
}
- qcom_pcie_icc_update(pcie);
+ qcom_pcie_icc_opp_update(pcie);
return 0;
}
--
2.42.0
On 12/01/2024 14:22, Krishna chaitanya chundru wrote:
> CPU-PCIe path consits for registers PCIe BAR space, config space.
> As there is less access on this path compared to pcie to mem path
> add minimum vote i.e GEN1x1 bandwidth always.
>
> In suspend remove the cpu vote after register space access is done.
>
> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
If this patch is a Fixes then don't you need the accompanying dts change
as a parallel Fixes too ?
i.e. without the dts update - you won't have the nodes in the dts to
consume => applying this code to the stable kernel absent the dts will
result in no functional change and therefore no bugfix.
I'm not sure if you are asked to put a Fixes here but it seems to be it
should either be dropped or require a parallel Fixes: tag for the dts
and yaml changes.
What is the bug this change fixes in the backport ?
> cc: [email protected]
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
bod
On Fri, 12 Jan 2024 at 16:24, Krishna chaitanya chundru
<[email protected]> wrote:
>
> CPU-PCIe path consits for registers PCIe BAR space, config space.
> As there is less access on this path compared to pcie to mem path
> add minimum vote i.e GEN1x1 bandwidth always.
Is this BW amount a real requirement or just a random number? I mean,
the register space in my opinion consumes much less bandwidth compared
to Gen1 memory access.
>
> In suspend remove the cpu vote after register space access is done.
>
> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
> cc: [email protected]
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++--
> 1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 11c80555d975..035953f0b6d8 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -240,6 +240,7 @@ struct qcom_pcie {
> struct phy *phy;
> struct gpio_desc *reset;
> struct icc_path *icc_mem;
> + struct icc_path *icc_cpu;
> const struct qcom_pcie_cfg *cfg;
> struct dentry *debugfs;
> bool suspended;
> @@ -1372,6 +1373,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> if (IS_ERR(pcie->icc_mem))
> return PTR_ERR(pcie->icc_mem);
>
> + pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
> + if (IS_ERR(pcie->icc_cpu))
> + return PTR_ERR(pcie->icc_cpu);
> /*
> * Some Qualcomm platforms require interconnect bandwidth constraints
> * to be set before enabling interconnect clocks.
> @@ -1381,7 +1385,18 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> */
> ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> if (ret) {
> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
> + ret);
> + return ret;
> + }
> +
> + /*
> + * The config space, BAR space and registers goes through cpu-pcie path.
> + * Set peak bandwidth to single-lane Gen1 for this path all the time.
> + */
> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> + if (ret) {
> + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n",
> ret);
> return ret;
> }
> @@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> */
> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
> if (ret) {
> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
> + dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
> return ret;
> }
>
> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> pcie->suspended = true;
> }
>
> + /* Remove cpu path vote after all the register access is done */
> + ret = icc_set_bw(pcie->icc_cpu, 0, 0);
> + if (ret) {
> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
> + return ret;
> + }
> return 0;
> }
>
> @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> + if (ret) {
> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
> + return ret;
> + }
> +
> if (pcie->suspended) {
> ret = qcom_pcie_host_init(&pcie->pci->pp);
> if (ret)
>
> --
> 2.42.0
>
>
--
With best wishes
Dmitry
On Fri, 12 Jan 2024 at 16:25, Krishna chaitanya chundru
<[email protected]> wrote:
>
> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
> maintains hardware state of a regulator by performing max aggregation of
> the requests made by all of the processors.
>
> PCIe controller can operate on different RPMh performance state of power
> domain based up on the speed of the link. And this performance state varies
> from target to target.
>
> It is manadate to scale the performance state based up on the PCIe speed
> link operates so that SoC can run under optimum power conditions.
>
> Add Operating Performance Points(OPP) support to vote for RPMh state based
> upon GEN speed link is operating.
>
> OPP can handle ICC bw voting also, so move icc bw voting through opp
> framework if opp entries are present.
>
> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
> same icc bw and has frequency, so use frequency based search to reduce
> number of entries in the opp table.
>
> Don't initialize icc if opp is supported.
>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 83 ++++++++++++++++++++++++++++------
> 1 file changed, 70 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 035953f0b6d8..31512dc9d6ff 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -22,6 +22,7 @@
> #include <linux/of.h>
> #include <linux/of_gpio.h>
> #include <linux/pci.h>
> +#include <linux/pm_opp.h>
> #include <linux/pm_runtime.h>
> #include <linux/platform_device.h>
> #include <linux/phy/pcie.h>
> @@ -244,6 +245,7 @@ struct qcom_pcie {
> const struct qcom_pcie_cfg *cfg;
> struct dentry *debugfs;
> bool suspended;
> + bool opp_supported;
> };
>
> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
> @@ -1404,16 +1406,14 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> return 0;
> }
>
> -static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> +static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
> {
> struct dw_pcie *pci = pcie->pci;
> - u32 offset, status;
> + u32 offset, status, freq;
> + struct dev_pm_opp *opp;
> 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);
>
> @@ -1424,11 +1424,42 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
>
> - ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> - if (ret) {
> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> - ret);
> + if (pcie->opp_supported) {
> + switch (speed) {
> + case 1:
> + freq = 2500000;
> + break;
> + case 2:
> + freq = 5000000;
> + break;
> + case 3:
> + freq = 8000000;
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + fallthrough;
> + case 4:
> + freq = 16000000;
I expected that this kind of detail goes to the OPP table itself. Can
we index the table using the generation instead of frequency?
> + break;
> + }
> +
> + opp = dev_pm_opp_find_freq_exact(pci->dev, freq * width, true);
> + if (!IS_ERR(opp)) {
> + ret = dev_pm_opp_set_opp(pci->dev, opp);
> + if (ret)
> + dev_err(pci->dev, "Failed to set opp: freq %ld ret %d\n",
> + dev_pm_opp_get_freq(opp), ret);
> + dev_pm_opp_put(opp);
> + }
> + } else {
> + ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> + if (ret) {
> + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
> + ret);
> + }
> }
> +
> + return;
> }
>
> static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
> @@ -1471,8 +1502,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
> static int qcom_pcie_probe(struct platform_device *pdev)
> {
> const struct qcom_pcie_cfg *pcie_cfg;
> + unsigned long max_freq = INT_MAX;
> struct device *dev = &pdev->dev;
> struct qcom_pcie *pcie;
> + struct dev_pm_opp *opp;
> struct dw_pcie_rp *pp;
> struct resource *res;
> struct dw_pcie *pci;
> @@ -1539,9 +1572,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> goto err_pm_runtime_put;
> }
>
> - ret = qcom_pcie_icc_init(pcie);
> - if (ret)
> + /* OPP table is optional */
> + ret = devm_pm_opp_of_add_table(dev);
> + if (ret && ret != -ENODEV) {
> + dev_err_probe(dev, ret, "Failed to add OPP table\n");
> goto err_pm_runtime_put;
> + }
Can we initialise the table from the driver if it is not found? This
will help us by having the common code later on.
> +
> + /* vote for max freq in the opp table if opp table is present */
> + if (ret != -ENODEV) {
> + opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
> + if (!IS_ERR(opp)) {
> + ret = dev_pm_opp_set_opp(dev, opp);
> + if (ret)
> + dev_err_probe(pci->dev, ret,
> + "Failed to set opp: freq %ld\n",
> + dev_pm_opp_get_freq(opp));
> + dev_pm_opp_put(opp);
> + }
> + pcie->opp_supported = true;
> + }
> +
> + /* Skip icc init if opp is supported as icc bw vote is handled by opp framework */
> + if (!pcie->opp_supported) {
> + ret = qcom_pcie_icc_init(pcie);
> + if (ret)
> + goto err_pm_runtime_put;
> + }
>
> ret = pcie->cfg->ops->get_resources(pcie);
> if (ret)
> @@ -1561,7 +1618,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> goto err_phy_exit;
> }
>
> - qcom_pcie_icc_update(pcie);
> + qcom_pcie_icc_opp_update(pcie);
>
> if (pcie->mhi)
> qcom_pcie_init_debugfs(pcie);
> @@ -1640,7 +1697,7 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> pcie->suspended = false;
> }
>
> - qcom_pcie_icc_update(pcie);
> + qcom_pcie_icc_opp_update(pcie);
>
> return 0;
> }
>
> --
> 2.42.0
>
>
--
With best wishes
Dmitry
On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote:
> CPU-PCIe path consits for registers PCIe BAR space, config space.
consits?
> As there is less access on this path compared to pcie to mem path
> add minimum vote i.e GEN1x1 bandwidth always.
gen1 bandwidth can't be right.
> In suspend remove the cpu vote after register space access is done.
>
> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
> cc: [email protected]
This does not look like a fix so drop the above.
The commit you refer to explicitly left this path unconfigured for now
and only added support for the configuring the mem path as needed on
sc8280xp which otherwise would crash.
> @@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> */
> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
> if (ret) {
> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
> + dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
> return ret;
> }
>
> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> pcie->suspended = true;
> }
>
> + /* Remove cpu path vote after all the register access is done */
> + ret = icc_set_bw(pcie->icc_cpu, 0, 0);
I believe you should use icc_disable() here.
> + if (ret) {
> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
> + return ret;
And you need to unwind before returning on errors.
> + }
> return 0;
> }
>
> @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
icc_enable()
> + if (ret) {
> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
> + return ret;
> + }
Johan
Capitalize "ICC" and "CPU" to make the subject easier to read.
"Missing" might be superfluous in the subject? It would be nice to
have the ICC expansion once in the commit log as a hook for newbies
like me :)
On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote:
> CPU-PCIe path consits for registers PCIe BAR space, config space.
> As there is less access on this path compared to pcie to mem path
> add minimum vote i.e GEN1x1 bandwidth always.
"GEN1x1" is unnecessarily ambiguous, and the spec recommends avoiding
it (PCIe r6.0, sec 1.2). Use the actual bandwidth numbers instead.
"PCIe" to match above. Also below in comments and messages.
> In suspend remove the cpu vote after register space access is done.
"CPU" to match above.
> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
> cc: [email protected]
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++--
> 1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 11c80555d975..035953f0b6d8 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -240,6 +240,7 @@ struct qcom_pcie {
> struct phy *phy;
> struct gpio_desc *reset;
> struct icc_path *icc_mem;
> + struct icc_path *icc_cpu;
> const struct qcom_pcie_cfg *cfg;
> struct dentry *debugfs;
> bool suspended;
> @@ -1372,6 +1373,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> if (IS_ERR(pcie->icc_mem))
> return PTR_ERR(pcie->icc_mem);
>
> + pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
> + if (IS_ERR(pcie->icc_cpu))
> + return PTR_ERR(pcie->icc_cpu);
> /*
> * Some Qualcomm platforms require interconnect bandwidth constraints
> * to be set before enabling interconnect clocks.
> @@ -1381,7 +1385,18 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> */
> ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> if (ret) {
> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
> + ret);
> + return ret;
> + }
> +
> + /*
> + * The config space, BAR space and registers goes through cpu-pcie path.
> + * Set peak bandwidth to single-lane Gen1 for this path all the time.
Numbers instead of "Gen1".
> + */
> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> + if (ret) {
> + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n",
> ret);
> return ret;
> }
> @@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> */
> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
> if (ret) {
> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
> + dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
> return ret;
> }
>
> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> pcie->suspended = true;
> }
>
> + /* Remove cpu path vote after all the register access is done */
> + ret = icc_set_bw(pcie->icc_cpu, 0, 0);
> + if (ret) {
> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
> + return ret;
> + }
> return 0;
> }
>
> @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> + if (ret) {
> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
> + return ret;
> + }
> +
> if (pcie->suspended) {
> ret = qcom_pcie_host_init(&pcie->pci->pp);
> if (ret)
>
> --
> 2.42.0
>
On Fri, Jan 12, 2024 at 07:52:05PM +0530, Krishna chaitanya chundru wrote:
> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
> maintains hardware state of a regulator by performing max aggregation of
> the requests made by all of the processors.
>
> PCIe controller can operate on different RPMh performance state of power
> domain based up on the speed of the link. And this performance state varies
> from target to target.
>
> It is manadate to scale the performance state based up on the PCIe speed
> link operates so that SoC can run under optimum power conditions.
>
> Add Operating Performance Points(OPP) support to vote for RPMh state based
> upon GEN speed link is operating.
Thanks for this "OPP" expansion! Maybe "GEN" is unnecessary in this
sentence? And below, could be replaced with actual speeds?
> OPP can handle ICC bw voting also, so move icc bw voting through opp
> framework if opp entries are present.
s/opp/OPP/ to match
s/icc/ICC/ similarly (and perhaps expand once)
Also below in comments, etc.
> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
> same icc bw and has frequency, so use frequency based search to reduce
> number of entries in the opp table.
>
> Don't initialize icc if opp is supported.
Bjorn
On Fri, Jan 12, 2024 at 07:52:00PM +0530, Krishna chaitanya chundru wrote:
> Add the interconnects path as required property for sm8450 platform.
There's no explaination here as to why you need two different
compatibles for the instances on this device. Please add one.
Thanks,
Conor.
>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index eadba38171e1..bc28669f6fa0 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -777,6 +777,8 @@ allOf:
> - qcom,pcie-sa8540p
> - qcom,pcie-sa8775p
> - qcom,pcie-sc8280xp
> + - qcom,pcie-sm8450-pcie0
> + - qcom,pcie-sm8450-pcie1
> then:
> required:
> - interconnects
>
> --
> 2.42.0
>
On Fri, 12 Jan 2024 at 18:55, Conor Dooley <[email protected]> wrote:
>
> On Fri, Jan 12, 2024 at 07:52:00PM +0530, Krishna chaitanya chundru wrote:
> > Add the interconnects path as required property for sm8450 platform.
>
> There's no explaination here as to why you need two different
> compatibles for the instances on this device. Please add one.
Note, these are not new compatible strings. They are already defined
(separate because port0 and port1 have different sets of NoC clocks).
>
> Thanks,
> Conor.
>
> >
> > Signed-off-by: Krishna chaitanya chundru <[email protected]>
> > ---
> > Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > index eadba38171e1..bc28669f6fa0 100644
> > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > @@ -777,6 +777,8 @@ allOf:
> > - qcom,pcie-sa8540p
> > - qcom,pcie-sa8775p
> > - qcom,pcie-sc8280xp
> > + - qcom,pcie-sm8450-pcie0
> > + - qcom,pcie-sm8450-pcie1
> > then:
> > required:
> > - interconnects
> >
> > --
> > 2.42.0
> >
--
With best wishes
Dmitry
On Fri, Jan 12, 2024 at 07:12:01PM +0200, Dmitry Baryshkov wrote:
> On Fri, 12 Jan 2024 at 18:55, Conor Dooley <[email protected]> wrote:
> >
> > On Fri, Jan 12, 2024 at 07:52:00PM +0530, Krishna chaitanya chundru wrote:
> > > Add the interconnects path as required property for sm8450 platform.
> >
> > There's no explaination here as to why you need two different
> > compatibles for the instances on this device. Please add one.
>
> Note, these are not new compatible strings. They are already defined
> (separate because port0 and port1 have different sets of NoC clocks).
Ahh, my bad. My comment can be disregarded.
:wq
>
> >
> > Thanks,
> > Conor.
> >
> > >
> > > Signed-off-by: Krishna chaitanya chundru <[email protected]>
> > > ---
> > > Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > index eadba38171e1..bc28669f6fa0 100644
> > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > @@ -777,6 +777,8 @@ allOf:
> > > - qcom,pcie-sa8540p
> > > - qcom,pcie-sa8775p
> > > - qcom,pcie-sc8280xp
> > > + - qcom,pcie-sm8450-pcie0
> > > + - qcom,pcie-sm8450-pcie1
> > > then:
> > > required:
> > > - interconnects
> > >
> > > --
> > > 2.42.0
> > >
>
>
>
> --
> With best wishes
> Dmitry
On 12.01.2024 16:17, Bryan O'Donoghue wrote:
> On 12/01/2024 14:22, Krishna chaitanya chundru wrote:
>> CPU-PCIe path consits for registers PCIe BAR space, config space.
>> As there is less access on this path compared to pcie to mem path
>> add minimum vote i.e GEN1x1 bandwidth always.
>>
>> In suspend remove the cpu vote after register space access is done.
>>
>> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
>
> If this patch is a Fixes then don't you need the accompanying dts change as a parallel Fixes too ?
>
> i.e. without the dts update - you won't have the nodes in the dts to consume => applying this code to the stable kernel absent the dts will result in no functional change and therefore no bugfix.
The Fixes tag denotes a bug fix, its use for backport autosel is just
a nice "coincidence".
Fixing a lack of a required icc path and having to rely on BL leftovers
/ keepalive bus settings is definitely worth this tag in my eyes.
Konrad
On 12.01.2024 16:59, Johan Hovold wrote:
> On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote:
>> CPU-PCIe path consits for registers PCIe BAR space, config space.
>
> consits?
>
>> As there is less access on this path compared to pcie to mem path
>> add minimum vote i.e GEN1x1 bandwidth always.
>
> gen1 bandwidth can't be right.
>
>> In suspend remove the cpu vote after register space access is done.
>>
>> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
>> cc: [email protected]
>
> This does not look like a fix so drop the above.
>
> The commit you refer to explicitly left this path unconfigured for now
> and only added support for the configuring the mem path as needed on
> sc8280xp which otherwise would crash.
I only sorta agree. I'd include a fixes tag but point it to either 8450
addition or original driver introduction, as this is patching up a real
hole (see my reply to Bryan).
>
>> @@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> */
>> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>> if (ret) {
>> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
>> + dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
>> return ret;
>> }
>>
>> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> pcie->suspended = true;
>> }
>>
>> + /* Remove cpu path vote after all the register access is done */
>> + ret = icc_set_bw(pcie->icc_cpu, 0, 0);
>
> I believe you should use icc_disable() here.
Oh, TIL this exists!
Konrad
On 12.01.2024 15:22, Krishna chaitanya chundru wrote:
> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
> maintains hardware state of a regulator by performing max aggregation of
> the requests made by all of the processors.
>
> PCIe controller can operate on different RPMh performance state of power
> domain based up on the speed of the link. And this performance state varies
> from target to target.
>
> It is manadate to scale the performance state based up on the PCIe speed
> link operates so that SoC can run under optimum power conditions.
>
> Add Operating Performance Points(OPP) support to vote for RPMh state based
> upon GEN speed link is operating.
>
> OPP can handle ICC bw voting also, so move icc bw voting through opp
> framework if opp entries are present.
>
> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
> same icc bw and has frequency, so use frequency based search to reduce
> number of entries in the opp table.
>
> Don't initialize icc if opp is supported.
>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
[...]
>
> -static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> +static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
Or simply.. qcom_pcie_opp_update :) Especially with Dmitry's
suggestions
> {
> struct dw_pcie *pci = pcie->pci;
> - u32 offset, status;
> + u32 offset, status, freq;
> + struct dev_pm_opp *opp;
> 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);
>
> @@ -1424,11 +1424,42 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
>
> - ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> - if (ret) {
> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> - ret);
> + if (pcie->opp_supported) {
> + switch (speed) {
> + case 1:
> + freq = 2500000;
> + break;
> + case 2:
> + freq = 5000000;
> + break;
> + case 3:
> + freq = 8000000;
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + fallthrough;
> + case 4:
> + freq = 16000000;
> + break;
> + }
Might as well add gen5 and 6 rates of 3200.. and 6400.. since they're
hard-in-stone in the spec by now, AFAIK
Konrad
On 1/12/2024 8:47 PM, Bryan O'Donoghue wrote:
> On 12/01/2024 14:22, Krishna chaitanya chundru wrote:
>> CPU-PCIe path consits for registers PCIe BAR space, config space.
>> As there is less access on this path compared to pcie to mem path
>> add minimum vote i.e GEN1x1 bandwidth always.
>>
>> In suspend remove the cpu vote after register space access is done.
>>
>> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
>
> If this patch is a Fixes then don't you need the accompanying dts change
> as a parallel Fixes too ?
>
> i.e. without the dts update - you won't have the nodes in the dts to
> consume => applying this code to the stable kernel absent the dts will
> result in no functional change and therefore no bugfix.
>
> I'm not sure if you are asked to put a Fixes here but it seems to be it
> should either be dropped or require a parallel Fixes: tag for the dts
> and yaml changes.
>
> What is the bug this change fixes in the backport ?
>
There is no change required in the dts because the cpu-pcie path is
already present in the dts.
So till now driver is ignoring that path, that's why we tagged with
fixed.
-Krishna Chaitanya
>> cc: [email protected]
>> Signed-off-by: Krishna chaitanya chundru <[email protected]>
>
> ---
> bod
On 1/12/2024 9:00 PM, Dmitry Baryshkov wrote:
> On Fri, 12 Jan 2024 at 16:24, Krishna chaitanya chundru
> <[email protected]> wrote:
>>
>> CPU-PCIe path consits for registers PCIe BAR space, config space.
>> As there is less access on this path compared to pcie to mem path
>> add minimum vote i.e GEN1x1 bandwidth always.
>
> Is this BW amount a real requirement or just a random number? I mean,
> the register space in my opinion consumes much less bandwidth compared
> to Gen1 memory access.
>
Not register space right the BAR space and config space access from CPU
goes through this path only. There is no recommended value we need to
vote for this path. Keeping BAR space and config space we tried to vote
for GEN1x1.
Please suggest any recommended value, I will change that in the next
series.
- Krishna Chaitanya.
>>
>> In suspend remove the cpu vote after register space access is done.
>>
>> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
>> cc: [email protected]
>> Signed-off-by: Krishna chaitanya chundru <[email protected]>
>> ---
>> drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++--
>> 1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 11c80555d975..035953f0b6d8 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -240,6 +240,7 @@ struct qcom_pcie {
>> struct phy *phy;
>> struct gpio_desc *reset;
>> struct icc_path *icc_mem;
>> + struct icc_path *icc_cpu;
>> const struct qcom_pcie_cfg *cfg;
>> struct dentry *debugfs;
>> bool suspended;
>> @@ -1372,6 +1373,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>> if (IS_ERR(pcie->icc_mem))
>> return PTR_ERR(pcie->icc_mem);
>>
>> + pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
>> + if (IS_ERR(pcie->icc_cpu))
>> + return PTR_ERR(pcie->icc_cpu);
>> /*
>> * Some Qualcomm platforms require interconnect bandwidth constraints
>> * to be set before enabling interconnect clocks.
>> @@ -1381,7 +1385,18 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>> */
>> ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>> if (ret) {
>> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>> + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + /*
>> + * The config space, BAR space and registers goes through cpu-pcie path.
>> + * Set peak bandwidth to single-lane Gen1 for this path all the time.
>> + */
>> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>> + if (ret) {
>> + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n",
>> ret);
>> return ret;
>> }
>> @@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> */
>> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>> if (ret) {
>> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
>> + dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
>> return ret;
>> }
>>
>> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> pcie->suspended = true;
>> }
>>
>> + /* Remove cpu path vote after all the register access is done */
>> + ret = icc_set_bw(pcie->icc_cpu, 0, 0);
>> + if (ret) {
>> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
>> + return ret;
>> + }
>> return 0;
>> }
>>
>> @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>> struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> int ret;
>>
>> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>> + if (ret) {
>> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
>> + return ret;
>> + }
>> +
>> if (pcie->suspended) {
>> ret = qcom_pcie_host_init(&pcie->pci->pp);
>> if (ret)
>>
>> --
>> 2.42.0
>>
>>
>
>
On 1/12/2024 9:29 PM, Johan Hovold wrote:
> On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote:
>> CPU-PCIe path consits for registers PCIe BAR space, config space.
>
> consits?
>
>> As there is less access on this path compared to pcie to mem path
>> add minimum vote i.e GEN1x1 bandwidth always.
>
> gen1 bandwidth can't be right.
>
There is no recommended value we need vote for this path, as there is
BAR and config space in this path we are voting for GEN1x1.
Please suggest a recommended value for this path if the GEN1x1 is high.
>> In suspend remove the cpu vote after register space access is done.
>>
>> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
>> cc: [email protected]
>
> This does not look like a fix so drop the above.
>
> The commit you refer to explicitly left this path unconfigured for now
> and only added support for the configuring the mem path as needed on
> sc8280xp which otherwise would crash.
>
Without this path vote BAR and config space can result NOC timeout
errors, we are surviving because of other driver vote for this path.
For that reason we added a fix tag.
>> @@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> */
>> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>> if (ret) {
>> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
>> + dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
>> return ret;
>> }
>>
>> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> pcie->suspended = true;
>> }
>>
>> + /* Remove cpu path vote after all the register access is done */
>> + ret = icc_set_bw(pcie->icc_cpu, 0, 0);
>
> I believe you should use icc_disable() here.
>
>> + if (ret) {
>> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
>> + return ret;
>
> And you need to unwind before returning on errors.
>
>> + }
>> return 0;
>> }
>>
>> @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>> struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> int ret;
>>
>> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>
> icc_enable()
>
I was not aware of these API's, I will add them in next patch.
- Krishna Chaitanya.
>> + if (ret) {
>> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
>> + return ret;
>> + }
>
> Johan
On 1/12/2024 10:17 PM, Bjorn Helgaas wrote:
> Capitalize "ICC" and "CPU" to make the subject easier to read.
> "Missing" might be superfluous in the subject? It would be nice to
> have the ICC expansion once in the commit log as a hook for newbies
> like me :)
>
Sure I will change a suggested in next patch series.
> On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote:
>> CPU-PCIe path consits for registers PCIe BAR space, config space.
>> As there is less access on this path compared to pcie to mem path
>> add minimum vote i.e GEN1x1 bandwidth always.
>
> "GEN1x1" is unnecessarily ambiguous, and the spec recommends avoiding
> it (PCIe r6.0, sec 1.2). Use the actual bandwidth numbers instead.
>
> "PCIe" to match above. Also below in comments and messages.
>
ACK.
>> In suspend remove the cpu vote after register space access is done.
>
> "CPU" to match above.
>
ACK
>> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
>> cc: [email protected]
>> Signed-off-by: Krishna chaitanya chundru <[email protected]>
>> ---
>> drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++--
>> 1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 11c80555d975..035953f0b6d8 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -240,6 +240,7 @@ struct qcom_pcie {
>> struct phy *phy;
>> struct gpio_desc *reset;
>> struct icc_path *icc_mem;
>> + struct icc_path *icc_cpu;
>> const struct qcom_pcie_cfg *cfg;
>> struct dentry *debugfs;
>> bool suspended;
>> @@ -1372,6 +1373,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>> if (IS_ERR(pcie->icc_mem))
>> return PTR_ERR(pcie->icc_mem);
>>
>> + pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
>> + if (IS_ERR(pcie->icc_cpu))
>> + return PTR_ERR(pcie->icc_cpu);
>> /*
>> * Some Qualcomm platforms require interconnect bandwidth constraints
>> * to be set before enabling interconnect clocks.
>> @@ -1381,7 +1385,18 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>> */
>> ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>> if (ret) {
>> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>> + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + /*
>> + * The config space, BAR space and registers goes through cpu-pcie path.
>> + * Set peak bandwidth to single-lane Gen1 for this path all the time.
>
> Numbers instead of "Gen1".
>
ACK
-Krishna Chaitanya.
>> + */
>> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>> + if (ret) {
>> + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n",
>> ret);
>> return ret;
>> }
>> @@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> */
>> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>> if (ret) {
>> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
>> + dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
>> return ret;
>> }
>>
>> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> pcie->suspended = true;
>> }
>>
>> + /* Remove cpu path vote after all the register access is done */
>> + ret = icc_set_bw(pcie->icc_cpu, 0, 0);
>> + if (ret) {
>> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
>> + return ret;
>> + }
>> return 0;
>> }
>>
>> @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>> struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> int ret;
>>
>> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>> + if (ret) {
>> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
>> + return ret;
>> + }
>> +
>> if (pcie->suspended) {
>> ret = qcom_pcie_host_init(&pcie->pci->pp);
>> if (ret)
>>
>> --
>> 2.42.0
>>
On 1/12/2024 10:20 PM, Bjorn Helgaas wrote:
> On Fri, Jan 12, 2024 at 07:52:05PM +0530, Krishna chaitanya chundru wrote:
>> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
>> maintains hardware state of a regulator by performing max aggregation of
>> the requests made by all of the processors.
>>
>> PCIe controller can operate on different RPMh performance state of power
>> domain based up on the speed of the link. And this performance state varies
>> from target to target.
>>
>> It is manadate to scale the performance state based up on the PCIe speed
>> link operates so that SoC can run under optimum power conditions.
>>
>> Add Operating Performance Points(OPP) support to vote for RPMh state based
>> upon GEN speed link is operating.
>
> Thanks for this "OPP" expansion! Maybe "GEN" is unnecessary in this
> sentence? And below, could be replaced with actual speeds?
>
ACK
>> OPP can handle ICC bw voting also, so move icc bw voting through opp
>> framework if opp entries are present.
>
> s/opp/OPP/ to match
> s/icc/ICC/ similarly (and perhaps expand once)
> Also below in comments, etc.
>
ACK.
-Krishna Chaitanya.
>> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
>> same icc bw and has frequency, so use frequency based search to reduce
>> number of entries in the opp table.
>>
>> Don't initialize icc if opp is supported.
>
> Bjorn
On 1/12/2024 9:03 PM, Dmitry Baryshkov wrote:
> On Fri, 12 Jan 2024 at 16:25, Krishna chaitanya chundru
> <[email protected]> wrote:
>>
>> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
>> maintains hardware state of a regulator by performing max aggregation of
>> the requests made by all of the processors.
>>
>> PCIe controller can operate on different RPMh performance state of power
>> domain based up on the speed of the link. And this performance state varies
>> from target to target.
>>
>> It is manadate to scale the performance state based up on the PCIe speed
>> link operates so that SoC can run under optimum power conditions.
>>
>> Add Operating Performance Points(OPP) support to vote for RPMh state based
>> upon GEN speed link is operating.
>>
>> OPP can handle ICC bw voting also, so move icc bw voting through opp
>> framework if opp entries are present.
>>
>> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
>> same icc bw and has frequency, so use frequency based search to reduce
>> number of entries in the opp table.
>>
>> Don't initialize icc if opp is supported.
>>
>> Signed-off-by: Krishna chaitanya chundru <[email protected]>
>> ---
>> drivers/pci/controller/dwc/pcie-qcom.c | 83 ++++++++++++++++++++++++++++------
>> 1 file changed, 70 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 035953f0b6d8..31512dc9d6ff 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -22,6 +22,7 @@
>> #include <linux/of.h>
>> #include <linux/of_gpio.h>
>> #include <linux/pci.h>
>> +#include <linux/pm_opp.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/platform_device.h>
>> #include <linux/phy/pcie.h>
>> @@ -244,6 +245,7 @@ struct qcom_pcie {
>> const struct qcom_pcie_cfg *cfg;
>> struct dentry *debugfs;
>> bool suspended;
>> + bool opp_supported;
>> };
>>
>> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
>> @@ -1404,16 +1406,14 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>> return 0;
>> }
>>
>> -static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>> +static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
>> {
>> struct dw_pcie *pci = pcie->pci;
>> - u32 offset, status;
>> + u32 offset, status, freq;
>> + struct dev_pm_opp *opp;
>> 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);
>>
>> @@ -1424,11 +1424,42 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
>> width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
>>
>> - ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
>> - if (ret) {
>> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>> - ret);
>> + if (pcie->opp_supported) {
>> + switch (speed) {
>> + case 1:
>> + freq = 2500000;
>> + break;
>> + case 2:
>> + freq = 5000000;
>> + break;
>> + case 3:
>> + freq = 8000000;
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> + fallthrough;
>> + case 4:
>> + freq = 16000000;
>
> I expected that this kind of detail goes to the OPP table itself. Can
> we index the table using the generation instead of frequency?
>
In the previous patch also we tried to use index only, but problem using
index is we can define only GEN speed. As we are voting for the ICC BW
voting also we need to consider for lane width while configuring this
path. It is difficult to use index now as we need to consider both gen
speed and lane width.
For that reason we moved to frequencies to reduce number of entries in
OPP table.
for example if my controller supports GEN1 & GEN2 and MAX lane width is
2.
for GEN1x1
opp-2500000 {
};
for GEN2x1 & GEN1x2 as both use same frequiences and bandwidth.
opp-5000000 {
};
for GEN2x2
opp-10000000 {
};
- Krishna chaitanya.
>> + break;
>> + }
>> +
>> + opp = dev_pm_opp_find_freq_exact(pci->dev, freq * width, true);
>> + if (!IS_ERR(opp)) {
>> + ret = dev_pm_opp_set_opp(pci->dev, opp);
>> + if (ret)
>> + dev_err(pci->dev, "Failed to set opp: freq %ld ret %d\n",
>> + dev_pm_opp_get_freq(opp), ret);
>> + dev_pm_opp_put(opp);
>> + }
>> + } else {
>> + ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
>> + if (ret) {
>> + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
>> + ret);
>> + }
>> }
>> +
>> + return;
>> }
>>
>> static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
>> @@ -1471,8 +1502,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
>> static int qcom_pcie_probe(struct platform_device *pdev)
>> {
>> const struct qcom_pcie_cfg *pcie_cfg;
>> + unsigned long max_freq = INT_MAX;
>> struct device *dev = &pdev->dev;
>> struct qcom_pcie *pcie;
>> + struct dev_pm_opp *opp;
>> struct dw_pcie_rp *pp;
>> struct resource *res;
>> struct dw_pcie *pci;
>> @@ -1539,9 +1572,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>> goto err_pm_runtime_put;
>> }
>>
>> - ret = qcom_pcie_icc_init(pcie);
>> - if (ret)
>> + /* OPP table is optional */
>> + ret = devm_pm_opp_of_add_table(dev);
>> + if (ret && ret != -ENODEV) {
>> + dev_err_probe(dev, ret, "Failed to add OPP table\n");
>> goto err_pm_runtime_put;
>> + }
>
> Can we initialise the table from the driver if it is not found? This
> will help us by having the common code later on.
>
we already icc voting if there is no opp table present in the dts.
So I think this might not be needed.
Please let me know if you want to use for some other use case.
- Krishna Chaitanya.
>> +
>> + /* vote for max freq in the opp table if opp table is present */
>> + if (ret != -ENODEV) {
>> + opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
>> + if (!IS_ERR(opp)) {
>> + ret = dev_pm_opp_set_opp(dev, opp);
>> + if (ret)
>> + dev_err_probe(pci->dev, ret,
>> + "Failed to set opp: freq %ld\n",
>> + dev_pm_opp_get_freq(opp));
>> + dev_pm_opp_put(opp);
>> + }
>> + pcie->opp_supported = true;
>> + }
>> +
>> + /* Skip icc init if opp is supported as icc bw vote is handled by opp framework */
>> + if (!pcie->opp_supported) {
>> + ret = qcom_pcie_icc_init(pcie);
>> + if (ret)
>> + goto err_pm_runtime_put;
>> + }
>>
>> ret = pcie->cfg->ops->get_resources(pcie);
>> if (ret)
>> @@ -1561,7 +1618,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>> goto err_phy_exit;
>> }
>>
>> - qcom_pcie_icc_update(pcie);
>> + qcom_pcie_icc_opp_update(pcie);
>>
>> if (pcie->mhi)
>> qcom_pcie_init_debugfs(pcie);
>> @@ -1640,7 +1697,7 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>> pcie->suspended = false;
>> }
>>
>> - qcom_pcie_icc_update(pcie);
>> + qcom_pcie_icc_opp_update(pcie);
>>
>> return 0;
>> }
>>
>> --
>> 2.42.0
>>
>>
>
>
On 1/13/2024 4:14 AM, Konrad Dybcio wrote:
> On 12.01.2024 15:22, Krishna chaitanya chundru wrote:
>> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
>> maintains hardware state of a regulator by performing max aggregation of
>> the requests made by all of the processors.
>>
>> PCIe controller can operate on different RPMh performance state of power
>> domain based up on the speed of the link. And this performance state varies
>> from target to target.
>>
>> It is manadate to scale the performance state based up on the PCIe speed
>> link operates so that SoC can run under optimum power conditions.
>>
>> Add Operating Performance Points(OPP) support to vote for RPMh state based
>> upon GEN speed link is operating.
>>
>> OPP can handle ICC bw voting also, so move icc bw voting through opp
>> framework if opp entries are present.
>>
>> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
>> same icc bw and has frequency, so use frequency based search to reduce
>> number of entries in the opp table.
>>
>> Don't initialize icc if opp is supported.
>>
>> Signed-off-by: Krishna chaitanya chundru <[email protected]>
>> ---
>
> [...]
>
>>
>> -static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>> +static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
>
> Or simply.. qcom_pcie_opp_update :) Especially with Dmitry's
> suggestions
>
If OPP path is not present we are still voting through ICC, so it is
better to have name as it.
>> {
>> struct dw_pcie *pci = pcie->pci;
>> - u32 offset, status;
>> + u32 offset, status, freq;
>> + struct dev_pm_opp *opp;
>> 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);
>>
>> @@ -1424,11 +1424,42 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
>> width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
>>
>> - ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
>> - if (ret) {
>> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>> - ret);
>> + if (pcie->opp_supported) {
>> + switch (speed) {
>> + case 1:
>> + freq = 2500000;
>> + break;
>> + case 2:
>> + freq = 5000000;
>> + break;
>> + case 3:
>> + freq = 8000000;
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> + fallthrough;
>> + case 4:
>> + freq = 16000000;
>> + break;
>> + }
> Might as well add gen5 and 6 rates of 3200.. and 6400.. since they're
> hard-in-stone in the spec by now, AFAIK
>
> Konrad
ACK.
- Krishna Chaitanya.
On Tue, 16 Jan 2024 at 07:17, Krishna Chaitanya Chundru
<[email protected]> wrote:
>
>
>
> On 1/12/2024 9:03 PM, Dmitry Baryshkov wrote:
> > On Fri, 12 Jan 2024 at 16:25, Krishna chaitanya chundru
> > <[email protected]> wrote:
> >>
> >> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
> >> maintains hardware state of a regulator by performing max aggregation of
> >> the requests made by all of the processors.
> >>
> >> PCIe controller can operate on different RPMh performance state of power
> >> domain based up on the speed of the link. And this performance state varies
> >> from target to target.
> >>
> >> It is manadate to scale the performance state based up on the PCIe speed
> >> link operates so that SoC can run under optimum power conditions.
> >>
> >> Add Operating Performance Points(OPP) support to vote for RPMh state based
> >> upon GEN speed link is operating.
> >>
> >> OPP can handle ICC bw voting also, so move icc bw voting through opp
> >> framework if opp entries are present.
> >>
> >> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
> >> same icc bw and has frequency, so use frequency based search to reduce
> >> number of entries in the opp table.
> >>
> >> Don't initialize icc if opp is supported.
> >>
> >> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> >> ---
> >> drivers/pci/controller/dwc/pcie-qcom.c | 83 ++++++++++++++++++++++++++++------
> >> 1 file changed, 70 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >> index 035953f0b6d8..31512dc9d6ff 100644
> >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >> @@ -22,6 +22,7 @@
> >> #include <linux/of.h>
> >> #include <linux/of_gpio.h>
> >> #include <linux/pci.h>
> >> +#include <linux/pm_opp.h>
> >> #include <linux/pm_runtime.h>
> >> #include <linux/platform_device.h>
> >> #include <linux/phy/pcie.h>
> >> @@ -244,6 +245,7 @@ struct qcom_pcie {
> >> const struct qcom_pcie_cfg *cfg;
> >> struct dentry *debugfs;
> >> bool suspended;
> >> + bool opp_supported;
> >> };
> >>
> >> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
> >> @@ -1404,16 +1406,14 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> >> return 0;
> >> }
> >>
> >> -static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> >> +static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
> >> {
> >> struct dw_pcie *pci = pcie->pci;
> >> - u32 offset, status;
> >> + u32 offset, status, freq;
> >> + struct dev_pm_opp *opp;
> >> 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);
> >>
> >> @@ -1424,11 +1424,42 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> >> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> >> width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
> >>
> >> - ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> >> - if (ret) {
> >> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> >> - ret);
> >> + if (pcie->opp_supported) {
> >> + switch (speed) {
> >> + case 1:
> >> + freq = 2500000;
> >> + break;
> >> + case 2:
> >> + freq = 5000000;
> >> + break;
> >> + case 3:
> >> + freq = 8000000;
> >> + break;
> >> + default:
> >> + WARN_ON_ONCE(1);
> >> + fallthrough;
> >> + case 4:
> >> + freq = 16000000;
> >
> > I expected that this kind of detail goes to the OPP table itself. Can
> > we index the table using the generation instead of frequency?
> >
> In the previous patch also we tried to use index only, but problem using
> index is we can define only GEN speed. As we are voting for the ICC BW
> voting also we need to consider for lane width while configuring this
> path. It is difficult to use index now as we need to consider both gen
> speed and lane width.
> For that reason we moved to frequencies to reduce number of entries in
> OPP table.
> for example if my controller supports GEN1 & GEN2 and MAX lane width is
> 2.
>
> for GEN1x1
> opp-2500000 {
> };
>
> for GEN2x1 & GEN1x2 as both use same frequiences and bandwidth.
> opp-5000000 {
> };
>
> for GEN2x2
> opp-10000000 {
>
> };
Ack. It would be nice to add this as a comment. Something as simple as
'gen1x2 and gen2x1 share the bandwidth value and thus the opp entry'
>
> - Krishna chaitanya.
> >> + break;
> >> + }
> >> +
> >> + opp = dev_pm_opp_find_freq_exact(pci->dev, freq * width, true);
> >> + if (!IS_ERR(opp)) {
> >> + ret = dev_pm_opp_set_opp(pci->dev, opp);
> >> + if (ret)
> >> + dev_err(pci->dev, "Failed to set opp: freq %ld ret %d\n",
> >> + dev_pm_opp_get_freq(opp), ret);
> >> + dev_pm_opp_put(opp);
> >> + }
> >> + } else {
> >> + ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> >> + if (ret) {
> >> + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
> >> + ret);
> >> + }
> >> }
> >> +
> >> + return;
> >> }
> >>
> >> static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
> >> @@ -1471,8 +1502,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
> >> static int qcom_pcie_probe(struct platform_device *pdev)
> >> {
> >> const struct qcom_pcie_cfg *pcie_cfg;
> >> + unsigned long max_freq = INT_MAX;
> >> struct device *dev = &pdev->dev;
> >> struct qcom_pcie *pcie;
> >> + struct dev_pm_opp *opp;
> >> struct dw_pcie_rp *pp;
> >> struct resource *res;
> >> struct dw_pcie *pci;
> >> @@ -1539,9 +1572,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >> goto err_pm_runtime_put;
> >> }
> >>
> >> - ret = qcom_pcie_icc_init(pcie);
> >> - if (ret)
> >> + /* OPP table is optional */
> >> + ret = devm_pm_opp_of_add_table(dev);
> >> + if (ret && ret != -ENODEV) {
> >> + dev_err_probe(dev, ret, "Failed to add OPP table\n");
> >> goto err_pm_runtime_put;
> >> + }
> >
> > Can we initialise the table from the driver if it is not found? This
> > will help us by having the common code later on.
> >
> we already icc voting if there is no opp table present in the dts.
Yes. So later we have two different code paths: one for the OPP table
being present and another one for the absent OPP table. My suggestion
is to initialise minimal OPP table by hand and then have a common code
path in qcom_pcie_icc_update().
> So I think this might not be needed.
> Please let me know if you want to use for some other use case.
>
> - Krishna Chaitanya.
> >> +
> >> + /* vote for max freq in the opp table if opp table is present */
> >> + if (ret != -ENODEV) {
> >> + opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
> >> + if (!IS_ERR(opp)) {
> >> + ret = dev_pm_opp_set_opp(dev, opp);
> >> + if (ret)
> >> + dev_err_probe(pci->dev, ret,
> >> + "Failed to set opp: freq %ld\n",
> >> + dev_pm_opp_get_freq(opp));
> >> + dev_pm_opp_put(opp);
> >> + }
> >> + pcie->opp_supported = true;
> >> + }
> >> +
> >> + /* Skip icc init if opp is supported as icc bw vote is handled by opp framework */
> >> + if (!pcie->opp_supported) {
> >> + ret = qcom_pcie_icc_init(pcie);
> >> + if (ret)
> >> + goto err_pm_runtime_put;
> >> + }
> >>
> >> ret = pcie->cfg->ops->get_resources(pcie);
> >> if (ret)
> >> @@ -1561,7 +1618,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >> goto err_phy_exit;
> >> }
> >>
> >> - qcom_pcie_icc_update(pcie);
> >> + qcom_pcie_icc_opp_update(pcie);
> >>
> >> if (pcie->mhi)
> >> qcom_pcie_init_debugfs(pcie);
> >> @@ -1640,7 +1697,7 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> >> pcie->suspended = false;
> >> }
> >>
> >> - qcom_pcie_icc_update(pcie);
> >> + qcom_pcie_icc_opp_update(pcie);
> >>
> >> return 0;
> >> }
> >>
> >> --
> >> 2.42.0
> >>
> >>
> >
> >
--
With best wishes
Dmitry
On 16/01/2024 04:52, Krishna Chaitanya Chundru wrote:
>>
> There is no change required in the dts because the cpu-pcie path is
> already present in the dts.
Not at c4860af88d0cb1bb006df12615c5515ae509f73b its not, those dts
entries get added later.
But anyway re-reading your commit log "vote for minimum bandwidth as at
c4860af88d0cb1bb006df12615c5515ae509f73b" makes sense to me.
Reviewed-by: Bryan O'Donoghue <[email protected]>
On Tue, Jan 16, 2024 at 10:34:22AM +0530, Krishna Chaitanya Chundru wrote:
>
>
> On 1/12/2024 9:29 PM, Johan Hovold wrote:
> > On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote:
> >> CPU-PCIe path consits for registers PCIe BAR space, config space.
> >
> > consits?
> >
> >> As there is less access on this path compared to pcie to mem path
> >> add minimum vote i.e GEN1x1 bandwidth always.
> >
> > gen1 bandwidth can't be right.
> There is no recommended value we need vote for this path, as there is
> BAR and config space in this path we are voting for GEN1x1.
I can see that, but that does not explain why you used those seemingly
arbitrary numbers or why you think that's correct.
> Please suggest a recommended value for this path if the GEN1x1 is high.
No, you submitted the patch and you work for Qualcomm. You need to
figure out what the value should be. All I can say is that the gen1
value is likely not correct and therefore confusing.
> >> In suspend remove the cpu vote after register space access is done.
> >>
> >> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
> >> cc: [email protected]
> >
> > This does not look like a fix so drop the above.
> >
> > The commit you refer to explicitly left this path unconfigured for now
> > and only added support for the configuring the mem path as needed on
> > sc8280xp which otherwise would crash.
> Without this path vote BAR and config space can result NOC timeout
> errors, we are surviving because of other driver vote for this path.
> For that reason we added a fix tag.
Ok, then mention that in the commit message so that it becomes more
clear why this is needed and whether this should be considered a fix. As
it stands, the commit message makes this look like a new feature.
And the above Fixes tag is incorrect either way as that commit did not
introduce any issue.
Johan
On Fri, Jan 12, 2024 at 11:33:15PM +0100, Konrad Dybcio wrote:
> On 12.01.2024 16:17, Bryan O'Donoghue wrote:
> > On 12/01/2024 14:22, Krishna chaitanya chundru wrote:
> >> CPU-PCIe path consits for registers PCIe BAR space, config space.
> >> As there is less access on this path compared to pcie to mem path
> >> add minimum vote i.e GEN1x1 bandwidth always.
> >>
> >> In suspend remove the cpu vote after register space access is done.
> >>
> >> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
> >
> > If this patch is a Fixes then don't you need the accompanying dts change as a parallel Fixes too ?
> >
> > i.e. without the dts update - you won't have the nodes in the dts to consume => applying this code to the stable kernel absent the dts will result in no functional change and therefore no bugfix.
>
> The Fixes tag denotes a bug fix, its use for backport autosel is just
> a nice "coincidence".
>
> Fixing a lack of a required icc path and having to rely on BL leftovers
> / keepalive bus settings is definitely worth this tag in my eyes.
An incomplete implementation can sometimes be considered a bug, but not
always. If this is needed to enable a new use case, then it's hard to
argue that the original omission was a bug.
And as I just mentioned to Krishna, the above Fixes tag is not correct
as that commit did not *introduce* any issue. It solved the bit that was
strictly needed for sc8280xp, but now it seems you may need something
more for an even newer platform (even if no details and motivation was
included in the commit message as it should have been).
Johan
On Fri, Jan 12, 2024 at 11:37:03PM +0100, Konrad Dybcio wrote:
> On 12.01.2024 16:59, Johan Hovold wrote:
> > On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote:
> >> CPU-PCIe path consits for registers PCIe BAR space, config space.
> >
> > consits?
> >
> >> As there is less access on this path compared to pcie to mem path
> >> add minimum vote i.e GEN1x1 bandwidth always.
> >
> > gen1 bandwidth can't be right.
> >
> >> In suspend remove the cpu vote after register space access is done.
> >>
> >> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
> >> cc: [email protected]
> >
> > This does not look like a fix so drop the above.
> >
> > The commit you refer to explicitly left this path unconfigured for now
> > and only added support for the configuring the mem path as needed on
> > sc8280xp which otherwise would crash.
>
> I only sorta agree. I'd include a fixes tag but point it to either 8450
> addition or original driver introduction, as this is patching up a real
> hole (see my reply to Bryan).
Right, the above Fixes tag is not correct in any case.
And with a complete commit message it may be possible to tell whether
a Fixes tag is warranted or not.
Johan
Please, people, remember to trim unnecessary context from your replies
before hitting send!
This thread is barely readable currently, and leaving all context in
place also makes revisiting threads using the lore web interface a pain.
Johan
On Tue, Jan 16, 2024 at 10:27:23AM +0530, Krishna Chaitanya Chundru wrote:
>
>
> On 1/12/2024 9:00 PM, Dmitry Baryshkov wrote:
> > On Fri, 12 Jan 2024 at 16:24, Krishna chaitanya chundru
> > <[email protected]> wrote:
> > >
> > > CPU-PCIe path consits for registers PCIe BAR space, config space.
> > > As there is less access on this path compared to pcie to mem path
> > > add minimum vote i.e GEN1x1 bandwidth always.
> >
> > Is this BW amount a real requirement or just a random number? I mean,
> > the register space in my opinion consumes much less bandwidth compared
> > to Gen1 memory access.
> >
> Not register space right the BAR space and config space access from CPU
> goes through this path only. There is no recommended value we need to
> vote for this path. Keeping BAR space and config space we tried to vote
> for GEN1x1.
>
> Please suggest any recommended value, I will change that in the next
> series.
>
You should ask the HW folks on the recommended value to keep the reg access
clocking. We cannot suggest a value here.
If they say, "there is no recommended value", then ask them what would the
minimum value and use it here.
- Mani
> - Krishna Chaitanya.
> > >
> > > In suspend remove the cpu vote after register space access is done.
> > >
> > > Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
> > > cc: [email protected]
> > > Signed-off-by: Krishna chaitanya chundru <[email protected]>
> > > ---
> > > drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++--
> > > 1 file changed, 29 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 11c80555d975..035953f0b6d8 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -240,6 +240,7 @@ struct qcom_pcie {
> > > struct phy *phy;
> > > struct gpio_desc *reset;
> > > struct icc_path *icc_mem;
> > > + struct icc_path *icc_cpu;
> > > const struct qcom_pcie_cfg *cfg;
> > > struct dentry *debugfs;
> > > bool suspended;
> > > @@ -1372,6 +1373,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> > > if (IS_ERR(pcie->icc_mem))
> > > return PTR_ERR(pcie->icc_mem);
> > >
> > > + pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
> > > + if (IS_ERR(pcie->icc_cpu))
> > > + return PTR_ERR(pcie->icc_cpu);
> > > /*
> > > * Some Qualcomm platforms require interconnect bandwidth constraints
> > > * to be set before enabling interconnect clocks.
> > > @@ -1381,7 +1385,18 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> > > */
> > > ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> > > if (ret) {
> > > - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> > > + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
> > > + ret);
> > > + return ret;
> > > + }
> > > +
> > > + /*
> > > + * The config space, BAR space and registers goes through cpu-pcie path.
> > > + * Set peak bandwidth to single-lane Gen1 for this path all the time.
> > > + */
> > > + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> > > + if (ret) {
> > > + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n",
> > > ret);
> > > return ret;
> > > }
> > > @@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> > > */
> > > ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
> > > if (ret) {
> > > - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
> > > + dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
> > > return ret;
> > > }
> > >
> > > @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> > > pcie->suspended = true;
> > > }
> > >
> > > + /* Remove cpu path vote after all the register access is done */
> > > + ret = icc_set_bw(pcie->icc_cpu, 0, 0);
> > > + if (ret) {
> > > + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
> > > + return ret;
> > > + }
> > > return 0;
> > > }
> > >
> > > @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> > > struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > > int ret;
> > >
> > > + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> > > + if (ret) {
> > > + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > if (pcie->suspended) {
> > > ret = qcom_pcie_host_init(&pcie->pci->pp);
> > > if (ret)
> > >
> > > --
> > > 2.42.0
> > >
> > >
> >
> >
>
--
மணிவண்ணன் சதாசிவம்
On 1/16/24 11:52, Johan Hovold wrote:
> On Fri, Jan 12, 2024 at 11:33:15PM +0100, Konrad Dybcio wrote:
>> On 12.01.2024 16:17, Bryan O'Donoghue wrote:
>>> On 12/01/2024 14:22, Krishna chaitanya chundru wrote:
>>>> CPU-PCIe path consits for registers PCIe BAR space, config space.
>>>> As there is less access on this path compared to pcie to mem path
>>>> add minimum vote i.e GEN1x1 bandwidth always.
>>>>
>>>> In suspend remove the cpu vote after register space access is done.
>>>>
>>>> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
>>>
>>> If this patch is a Fixes then don't you need the accompanying dts change as a parallel Fixes too ?
>>>
>>> i.e. without the dts update - you won't have the nodes in the dts to consume => applying this code to the stable kernel absent the dts will result in no functional change and therefore no bugfix.
>>
>> The Fixes tag denotes a bug fix, its use for backport autosel is just
>> a nice "coincidence".
>>
>> Fixing a lack of a required icc path and having to rely on BL leftovers
>> / keepalive bus settings is definitely worth this tag in my eyes.
>
> An incomplete implementation can sometimes be considered a bug, but not
> always. If this is needed to enable a new use case, then it's hard to
> argue that the original omission was a bug.
>
> And as I just mentioned to Krishna, the above Fixes tag is not correct
> as that commit did not *introduce* any issue. It solved the bit that was
> strictly needed for sc8280xp, but now it seems you may need something
> more for an even newer platform (even if no details and motivation was
> included in the commit message as it should have been).
The PCIe hardware seems to be piggybacking off of others' bus
bandwidth requests and I think it's just been pure luck that
it didn't simply refuse to work on previous generations.
So indeed, the commit message seems incomplete in explaining
where the problem lies
Konrad
On Fri, 12 Jan 2024 19:52:00 +0530, Krishna chaitanya chundru wrote:
> Add the interconnects path as required property for sm8450 platform.
>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
Acked-by: Rob Herring <[email protected]>
On 1/17/2024 12:09 PM, Manivannan Sadhasivam wrote:
> On Tue, Jan 16, 2024 at 10:27:23AM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 1/12/2024 9:00 PM, Dmitry Baryshkov wrote:
>>> On Fri, 12 Jan 2024 at 16:24, Krishna chaitanya chundru
>>> <[email protected]> wrote:
>>>>
>>>> CPU-PCIe path consits for registers PCIe BAR space, config space.
>>>> As there is less access on this path compared to pcie to mem path
>>>> add minimum vote i.e GEN1x1 bandwidth always.
>>>
>>> Is this BW amount a real requirement or just a random number? I mean,
>>> the register space in my opinion consumes much less bandwidth compared
>>> to Gen1 memory access.
>>>
>> Not register space right the BAR space and config space access from CPU
>> goes through this path only. There is no recommended value we need to
>> vote for this path. Keeping BAR space and config space we tried to vote
>> for GEN1x1.
>>
>> Please suggest any recommended value, I will change that in the next
>> series.
>>
>
> You should ask the HW folks on the recommended value to keep the reg access
> clocking. We cannot suggest a value here.
>
> If they say, "there is no recommended value", then ask them what would the
> minimum value and use it here.
>
> - Mani
>
HW team suggested to use minimum value of 1Kbps for this path.
I will update the patches to use 1Kbps in the next series.
- Krishna Chaitanya.
>> - Krishna Chaitanya.
>>>>
>>>> In suspend remove the cpu vote after register space access is done.
>>>>
>>>> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support")
>>>> cc: [email protected]
>>>> Signed-off-by: Krishna chaitanya chundru <[email protected]>
>>>> ---
>>>> drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++--
>>>> 1 file changed, 29 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> index 11c80555d975..035953f0b6d8 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> @@ -240,6 +240,7 @@ struct qcom_pcie {
>>>> struct phy *phy;
>>>> struct gpio_desc *reset;
>>>> struct icc_path *icc_mem;
>>>> + struct icc_path *icc_cpu;
>>>> const struct qcom_pcie_cfg *cfg;
>>>> struct dentry *debugfs;
>>>> bool suspended;
>>>> @@ -1372,6 +1373,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>>>> if (IS_ERR(pcie->icc_mem))
>>>> return PTR_ERR(pcie->icc_mem);
>>>>
>>>> + pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
>>>> + if (IS_ERR(pcie->icc_cpu))
>>>> + return PTR_ERR(pcie->icc_cpu);
>>>> /*
>>>> * Some Qualcomm platforms require interconnect bandwidth constraints
>>>> * to be set before enabling interconnect clocks.
>>>> @@ -1381,7 +1385,18 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>>>> */
>>>> ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>>>> if (ret) {
>>>> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>>>> + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
>>>> + ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /*
>>>> + * The config space, BAR space and registers goes through cpu-pcie path.
>>>> + * Set peak bandwidth to single-lane Gen1 for this path all the time.
>>>> + */
>>>> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>>>> + if (ret) {
>>>> + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n",
>>>> ret);
>>>> return ret;
>>>> }
>>>> @@ -1573,7 +1588,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>>>> */
>>>> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>>>> if (ret) {
>>>> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
>>>> + dev_err(dev, "Failed to set interconnect bandwidth for pcie-mem: %d\n", ret);
>>>> return ret;
>>>> }
>>>>
>>>> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>>>> pcie->suspended = true;
>>>> }
>>>>
>>>> + /* Remove cpu path vote after all the register access is done */
>>>> + ret = icc_set_bw(pcie->icc_cpu, 0, 0);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> return 0;
>>>> }
>>>>
>>>> @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>>>> struct qcom_pcie *pcie = dev_get_drvdata(dev);
>>>> int ret;
>>>>
>>>> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> if (pcie->suspended) {
>>>> ret = qcom_pcie_host_init(&pcie->pci->pp);
>>>> if (ret)
>>>>
>>>> --
>>>> 2.42.0
>>>>
>>>>
>>>
>>>
>>
>
On Fri, Jan 12, 2024 at 07:52:00PM +0530, Krishna chaitanya chundru wrote:
> Add the interconnects path as required property for sm8450 platform.
>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index eadba38171e1..bc28669f6fa0 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -777,6 +777,8 @@ allOf:
> - qcom,pcie-sa8540p
> - qcom,pcie-sa8775p
> - qcom,pcie-sc8280xp
> + - qcom,pcie-sm8450-pcie0
> + - qcom,pcie-sm8450-pcie1
> then:
> required:
> - interconnects
>
> --
> 2.42.0
>
--
மணிவண்ணன் சதாசிவம்
On Fri, Jan 12, 2024 at 07:52:01PM +0530, Krishna chaitanya chundru wrote:
> Add pcie-mem & cpu-pcie interconnect path to the PCIe nodes.
>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
> arch/arm64/boot/dts/qcom/sm8450.dtsi | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 01e4dfc4babd..6b1d2e0d9d14 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -1781,6 +1781,10 @@ pcie0: pcie@1c00000 {
> <0 0 0 3 &intc 0 0 0 151 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> <0 0 0 4 &intc 0 0 0 152 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
>
> + interconnects = <&pcie_noc MASTER_PCIE_0 0 &mc_virt SLAVE_EBI1 0>,
> + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_PCIE_0 0>;
> + interconnect-names = "pcie-mem", "cpu-pcie";
> +
> clocks = <&gcc GCC_PCIE_0_PIPE_CLK>,
> <&gcc GCC_PCIE_0_PIPE_CLK_SRC>,
> <&pcie0_phy>,
> @@ -1890,6 +1894,10 @@ pcie1: pcie@1c08000 {
> <0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> <0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
>
> + interconnects = <&pcie_noc MASTER_PCIE_1 0 &mc_virt SLAVE_EBI1 0>,
> + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_PCIE_1 0>;
> + interconnect-names = "pcie-mem", "cpu-pcie";
> +
> clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
> <&pcie1_phy>,
>
> --
> 2.42.0
>
--
மணிவண்ணன் சதாசிவம்
On Fri, Jan 12, 2024 at 07:52:04PM +0530, Krishna chaitanya chundru wrote:
> PCIe needs to choose the appropriate performance state of RPMH power
> domain and interconnect bandwidth based up on the PCIe gen speed.
>
> Add the OPP table support to specify RPMH performance states and
> interconnect peak bandwidth.
>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sm8450.dtsi | 74 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 6b1d2e0d9d14..eab85ecaeff0 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -1827,7 +1827,32 @@ pcie0: pcie@1c00000 {
> pinctrl-names = "default";
> pinctrl-0 = <&pcie0_default_state>;
>
> + operating-points-v2 = <&pcie0_opp_table>;
> +
> status = "disabled";
> +
> + pcie0_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-2500000 {
> + opp-hz = /bits/ 64 <2500000>;
> + required-opps = <&rpmhpd_opp_low_svs>;
> + opp-peak-kBps = <250000 250000>;
This is a question for Viresh: We already have macros in the driver to derive
the bandwidth based on link speed. So if OPP core exposes a callback to allow
the consumers to set the bw on its own, we can get rid of this entry.
Similar to config_clks()/config_regulators(). Is that feasible?
- Mani
--
மணிவண்ணன் சதாசிவம்
On Fri, Jan 12, 2024 at 07:52:05PM +0530, Krishna chaitanya chundru wrote:
> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
> maintains hardware state of a regulator by performing max aggregation of
> the requests made by all of the processors.
>
s/processors/clients
> PCIe controller can operate on different RPMh performance state of power
> domain based up on the speed of the link. And this performance state varies
> from target to target.
>
> It is manadate to scale the performance state based up on the PCIe speed
> link operates so that SoC can run under optimum power conditions.
>
> Add Operating Performance Points(OPP) support to vote for RPMh state based
> upon GEN speed link is operating.
>
> OPP can handle ICC bw voting also, so move icc bw voting through opp
> framework if opp entries are present.
>
> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
> same icc bw and has frequency, so use frequency based search to reduce
> number of entries in the opp table.
>
> Don't initialize icc if opp is supported.
>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 83 ++++++++++++++++++++++++++++------
> 1 file changed, 70 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 035953f0b6d8..31512dc9d6ff 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -22,6 +22,7 @@
> #include <linux/of.h>
> #include <linux/of_gpio.h>
> #include <linux/pci.h>
> +#include <linux/pm_opp.h>
> #include <linux/pm_runtime.h>
> #include <linux/platform_device.h>
> #include <linux/phy/pcie.h>
> @@ -244,6 +245,7 @@ struct qcom_pcie {
> const struct qcom_pcie_cfg *cfg;
> struct dentry *debugfs;
> bool suspended;
> + bool opp_supported;
> };
>
> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
> @@ -1404,16 +1406,14 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> return 0;
> }
>
> -static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> +static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
> {
> struct dw_pcie *pci = pcie->pci;
> - u32 offset, status;
> + u32 offset, status, freq;
> + struct dev_pm_opp *opp;
> 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);
>
> @@ -1424,11 +1424,42 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
>
> - ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> - if (ret) {
> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> - ret);
> + if (pcie->opp_supported) {
> + switch (speed) {
> + case 1:
> + freq = 2500000;
> + break;
> + case 2:
> + freq = 5000000;
> + break;
> + case 3:
> + freq = 8000000;
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + fallthrough;
> + case 4:
> + freq = 16000000;
> + break;
> + }
This switch case is PCIe generic, so need to be moved to drivers/pci/pci.c.
There is already an API, pcie_link_speed_mbps() that returns the frequency in
MBps but uses the pcie_capability_read_word() API to read LNKSTA of the device.
But you can move the switch case inside that API to a separate function and
reuse that here.
> +
> + opp = dev_pm_opp_find_freq_exact(pci->dev, freq * width, true);
> + if (!IS_ERR(opp)) {
> + ret = dev_pm_opp_set_opp(pci->dev, opp);
> + if (ret)
> + dev_err(pci->dev, "Failed to set opp: freq %ld ret %d\n",
> + dev_pm_opp_get_freq(opp), ret);
> + dev_pm_opp_put(opp);
> + }
> + } else {
> + ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> + if (ret) {
> + dev_err(pci->dev, "failed to set interconnect bandwidth for pcie-mem: %d\n",
> + ret);
> + }
> }
> +
> + return;
> }
>
> static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
> @@ -1471,8 +1502,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
> static int qcom_pcie_probe(struct platform_device *pdev)
> {
> const struct qcom_pcie_cfg *pcie_cfg;
> + unsigned long max_freq = INT_MAX;
> struct device *dev = &pdev->dev;
> struct qcom_pcie *pcie;
> + struct dev_pm_opp *opp;
> struct dw_pcie_rp *pp;
> struct resource *res;
> struct dw_pcie *pci;
> @@ -1539,9 +1572,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> goto err_pm_runtime_put;
> }
>
> - ret = qcom_pcie_icc_init(pcie);
> - if (ret)
> + /* OPP table is optional */
> + ret = devm_pm_opp_of_add_table(dev);
> + if (ret && ret != -ENODEV) {
> + dev_err_probe(dev, ret, "Failed to add OPP table\n");
> goto err_pm_runtime_put;
> + }
> +
> + /* vote for max freq in the opp table if opp table is present */
/*
* Use highest OPP here if the OPP table is present. At the end of the probe(),
* OPP will be updated using qcom_pcie_icc_opp_update().
*/
- Mani
--
மணிவண்ணன் சதாசிவம்
On 29-01-24, 21:34, Manivannan Sadhasivam wrote:
> On Fri, Jan 12, 2024 at 07:52:04PM +0530, Krishna chaitanya chundru wrote:
> > PCIe needs to choose the appropriate performance state of RPMH power
> > domain and interconnect bandwidth based up on the PCIe gen speed.
> >
> > Add the OPP table support to specify RPMH performance states and
> > interconnect peak bandwidth.
> >
> > Signed-off-by: Krishna chaitanya chundru <[email protected]>
> > ---
> > arch/arm64/boot/dts/qcom/sm8450.dtsi | 74 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 74 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > index 6b1d2e0d9d14..eab85ecaeff0 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > @@ -1827,7 +1827,32 @@ pcie0: pcie@1c00000 {
> > pinctrl-names = "default";
> > pinctrl-0 = <&pcie0_default_state>;
> >
> > + operating-points-v2 = <&pcie0_opp_table>;
> > +
> > status = "disabled";
> > +
> > + pcie0_opp_table: opp-table {
> > + compatible = "operating-points-v2";
> > +
> > + opp-2500000 {
> > + opp-hz = /bits/ 64 <2500000>;
> > + required-opps = <&rpmhpd_opp_low_svs>;
> > + opp-peak-kBps = <250000 250000>;
>
> This is a question for Viresh: We already have macros in the driver to derive
> the bandwidth based on link speed. So if OPP core exposes a callback to allow
> the consumers to set the bw on its own, we can get rid of this entry.
>
> Similar to config_clks()/config_regulators(). Is that feasible?
I don't have any issues with a new callback for bw. But, AFAIU, the DT
is required to represent the hardware irrespective of what any OS
would do with it. So DT should ideally have these values here, right ?
Also, the driver has already moved away from using those macros now
and depend on the OPP core to do the right thing. It only uses the
macro for the cases where the DT OPP table isn't available. And as
said by few others as well already, the driver really should try to
add OPPs dynamically in that case to avoid multiple code paths and
stick to a single OPP based solution.
--
viresh
On Tue, Jan 30, 2024 at 11:41:11AM +0530, Viresh Kumar wrote:
> On 29-01-24, 21:34, Manivannan Sadhasivam wrote:
> > On Fri, Jan 12, 2024 at 07:52:04PM +0530, Krishna chaitanya chundru wrote:
> > > PCIe needs to choose the appropriate performance state of RPMH power
> > > domain and interconnect bandwidth based up on the PCIe gen speed.
> > >
> > > Add the OPP table support to specify RPMH performance states and
> > > interconnect peak bandwidth.
> > >
> > > Signed-off-by: Krishna chaitanya chundru <[email protected]>
> > > ---
> > > arch/arm64/boot/dts/qcom/sm8450.dtsi | 74 ++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 74 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > > index 6b1d2e0d9d14..eab85ecaeff0 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > > @@ -1827,7 +1827,32 @@ pcie0: pcie@1c00000 {
> > > pinctrl-names = "default";
> > > pinctrl-0 = <&pcie0_default_state>;
> > >
> > > + operating-points-v2 = <&pcie0_opp_table>;
> > > +
> > > status = "disabled";
> > > +
> > > + pcie0_opp_table: opp-table {
> > > + compatible = "operating-points-v2";
> > > +
> > > + opp-2500000 {
> > > + opp-hz = /bits/ 64 <2500000>;
> > > + required-opps = <&rpmhpd_opp_low_svs>;
> > > + opp-peak-kBps = <250000 250000>;
> >
> > This is a question for Viresh: We already have macros in the driver to derive
> > the bandwidth based on link speed. So if OPP core exposes a callback to allow
> > the consumers to set the bw on its own, we can get rid of this entry.
> >
> > Similar to config_clks()/config_regulators(). Is that feasible?
>
> I don't have any issues with a new callback for bw. But, AFAIU, the DT
> is required to represent the hardware irrespective of what any OS
> would do with it. So DT should ideally have these values here, right ?
>
Not necessarily. Because, right now the bandwidth values of the all peripherals
are encoded within the drivers. Only OPP has the requirement to define the
values in DT.
> Also, the driver has already moved away from using those macros now
> and depend on the OPP core to do the right thing. It only uses the
> macro for the cases where the DT OPP table isn't available. And as
> said by few others as well already, the driver really should try to
> add OPPs dynamically in that case to avoid multiple code paths and
> stick to a single OPP based solution.
>
Still I prefer to use OPP for bandwidth control because both the voltage and
bandwidth values need to be updated at the same time. My only point here is, if
OPP exposes a callback for bw, then we can keep the DT behavior consistent.
- Mani
> --
> viresh
--
மணிவண்ணன் சதாசிவம்
On 30-01-24, 12:44, Manivannan Sadhasivam wrote:
> On Tue, Jan 30, 2024 at 11:41:11AM +0530, Viresh Kumar wrote:
> > I don't have any issues with a new callback for bw. But, AFAIU, the DT
> > is required to represent the hardware irrespective of what any OS
> > would do with it. So DT should ideally have these values here, right ?
> >
>
> Not necessarily. Because, right now the bandwidth values of the all peripherals
> are encoded within the drivers. Only OPP has the requirement to define the
> values in DT.
I have a bit different argument here. I am saying that it doesn't
matter if we have OPP framework or something else using these values.
The hardware must be represented properly by the DT, so Linux or any
other firmware/OS can program the device. So DT should have bandwidth
values anyway. And that's the way we have designed things in Linux
now.
> > Also, the driver has already moved away from using those macros now
> > and depend on the OPP core to do the right thing. It only uses the
> > macro for the cases where the DT OPP table isn't available. And as
> > said by few others as well already, the driver really should try to
> > add OPPs dynamically in that case to avoid multiple code paths and
> > stick to a single OPP based solution.
> >
>
> Still I prefer to use OPP for bandwidth control because both the voltage and
> bandwidth values need to be updated at the same time. My only point here is, if
> OPP exposes a callback for bw, then we can keep the DT behavior consistent.
Feels like we are going a bit backward on this. The current view, as
per me, is that driver shouldn't need to micromanage all these
configurations and the OPP core should be able to handle them. That's
why we want to handle all configurations from there.
This also means that the DT needs to contain all this information and
drivers shouldn't use special math functions to calculate these
values. Drivers need to move away from them, instead of getting more
of those.
I don't see how a callback would be helpful here, if the driver relies
on DT values only. Or am I confusing things here ??
--
viresh
On 30-01-24, 15:18, Manivannan Sadhasivam wrote:
> So you are saying that the ICC core itself should get the bw values from DT
> instead of hardcoding in the driver? If so, I'd like to get the opinion from
> Georgi/Bjorn.
Not really. The drivers or the ICC core doesn't need to do anything I
guess. Since the values are coming via the OPP, we must just use it to
hide all these details.
Why is the ICC core required to get into this here ? ICC core should
be ready to get the information from DT (may or may not via the OPP
core), or from driver.
--
viresh
On Tue, Jan 30, 2024 at 02:06:19PM +0530, Viresh Kumar wrote:
> On 30-01-24, 12:44, Manivannan Sadhasivam wrote:
> > On Tue, Jan 30, 2024 at 11:41:11AM +0530, Viresh Kumar wrote:
> > > I don't have any issues with a new callback for bw. But, AFAIU, the DT
> > > is required to represent the hardware irrespective of what any OS
> > > would do with it. So DT should ideally have these values here, right ?
> > >
> >
> > Not necessarily. Because, right now the bandwidth values of the all peripherals
> > are encoded within the drivers. Only OPP has the requirement to define the
> > values in DT.
>
> I have a bit different argument here. I am saying that it doesn't
> matter if we have OPP framework or something else using these values.
> The hardware must be represented properly by the DT, so Linux or any
> other firmware/OS can program the device. So DT should have bandwidth
> values anyway. And that's the way we have designed things in Linux
> now.
>
So you are saying that the ICC core itself should get the bw values from DT
instead of hardcoding in the driver? If so, I'd like to get the opinion from
Georgi/Bjorn.
> > > Also, the driver has already moved away from using those macros now
> > > and depend on the OPP core to do the right thing. It only uses the
> > > macro for the cases where the DT OPP table isn't available. And as
> > > said by few others as well already, the driver really should try to
> > > add OPPs dynamically in that case to avoid multiple code paths and
> > > stick to a single OPP based solution.
> > >
> >
> > Still I prefer to use OPP for bandwidth control because both the voltage and
> > bandwidth values need to be updated at the same time. My only point here is, if
> > OPP exposes a callback for bw, then we can keep the DT behavior consistent.
>
> Feels like we are going a bit backward on this. The current view, as
> per me, is that driver shouldn't need to micromanage all these
> configurations and the OPP core should be able to handle them. That's
> why we want to handle all configurations from there.
>
> This also means that the DT needs to contain all this information and
> drivers shouldn't use special math functions to calculate these
> values. Drivers need to move away from them, instead of getting more
> of those.
>
> I don't see how a callback would be helpful here, if the driver relies
> on DT values only. Or am I confusing things here ??
>
No, there is no confusion here, but a difference in perspective. Let's get the
thoughts of Georgi/Bjorn on this. I just want to avoid the confusion in DT since
some peripherals with OPP support will have bw defined in DT, while rest of the
peripherals will have them in drivers.
- Mani
> --
> viresh
--
மணிவண்ணன் சதாசிவம்
On Tue, Jan 30, 2024 at 03:25:08PM +0530, Viresh Kumar wrote:
> On 30-01-24, 15:18, Manivannan Sadhasivam wrote:
> > So you are saying that the ICC core itself should get the bw values from DT
> > instead of hardcoding in the driver? If so, I'd like to get the opinion from
> > Georgi/Bjorn.
>
> Not really. The drivers or the ICC core doesn't need to do anything I
> guess. Since the values are coming via the OPP, we must just use it to
> hide all these details.
>
> Why is the ICC core required to get into this here ? ICC core should
> be ready to get the information from DT (may or may not via the OPP
> core), or from driver.
>
Agree. But what I'm saying is, right now there is no DT property in the
interconnect consumer nodes to specificy the bw requirements. This is all
hardcoded in the respective ICC consumer drivers.
But when we use OPP to control bw, the bw requirements come from DT. This is
what I see as a difference. Because, only nodes making use of OPP will specify
bw in DT and other nodes making use of just ICC will not.
Maybe I'm worrying too much about these details... But it looks like
inconsistency to me.
- Mani
--
மணிவண்ணன் சதாசிவம்
On 30-01-24, 18:46, Manivannan Sadhasivam wrote:
> Agree. But what I'm saying is, right now there is no DT property in the
> interconnect consumer nodes to specificy the bw requirements. This is all
> hardcoded in the respective ICC consumer drivers.
I thought there are a lot of users already in there..
$ git grep -i opp.*bps arch/arm64/boot/dts/ | wc -l
864
> But when we use OPP to control bw, the bw requirements come from DT. This is
> what I see as a difference. Because, only nodes making use of OPP will specify
> bw in DT and other nodes making use of just ICC will not.
>
> Maybe I'm worrying too much about these details... But it looks like
> inconsistency to me.
Right. So is there inconsistency right now ? Yes, there is.
The important question we need to answer is where do we want to see
all these drivers (specially new ones) in the future. What's the right
thing to do eventually ? Hardcode stuff ? Or Move it to DT ?
The answer is DT for me, so the code can be generic enough to be
reused. This is just one step in the right direction I guess.
Eventually the drivers must get simplified, which they are I guess.
--
viresh
On Wed, Jan 31, 2024 at 10:53:35AM +0530, Viresh Kumar wrote:
> On 30-01-24, 18:46, Manivannan Sadhasivam wrote:
> > Agree. But what I'm saying is, right now there is no DT property in the
> > interconnect consumer nodes to specificy the bw requirements. This is all
> > hardcoded in the respective ICC consumer drivers.
>
> I thought there are a lot of users already in there..
>
> $ git grep -i opp.*bps arch/arm64/boot/dts/ | wc -l
> 864
Most of the hits are from CPU nodes... For some reasons, peripheral drivers are
sticking to hardcoded values.
>
> > But when we use OPP to control bw, the bw requirements come from DT. This is
> > what I see as a difference. Because, only nodes making use of OPP will specify
> > bw in DT and other nodes making use of just ICC will not.
> >
> > Maybe I'm worrying too much about these details... But it looks like
> > inconsistency to me.
>
> Right. So is there inconsistency right now ? Yes, there is.
>
> The important question we need to answer is where do we want to see
> all these drivers (specially new ones) in the future. What's the right
> thing to do eventually ? Hardcode stuff ? Or Move it to DT ?
>
> The answer is DT for me, so the code can be generic enough to be
> reused. This is just one step in the right direction I guess.
> Eventually the drivers must get simplified, which they are I guess.
>
I completely agree that hardcoding the bw values is not the right thing, but was
worried about the inconsistency. But anyway, I hope either ICC will also move
towards DT for bw or we will convert all the drivers to use OPP in the future.
Thanks for the discussion so far! It clarified.
- Mani
--
மணிவண்ணன் சதாசிவம்
On 31-01-24, 14:16, Manivannan Sadhasivam wrote:
> Most of the hits are from CPU nodes... For some reasons, peripheral drivers are
> sticking to hardcoded values.
I guess the reason for this is that the OPP core wasn't used for non-CPU devices
until recently. And we are in a transition phase where few of the drivers will
migrate to using it and so will have DT based bw values.
--
viresh
On Tue, Jan 16, 2024 at 11:55:17AM +0200, Dmitry Baryshkov wrote:
> On Tue, 16 Jan 2024 at 07:17, Krishna Chaitanya Chundru
> <[email protected]> wrote:
> >
> >
> >
> > On 1/12/2024 9:03 PM, Dmitry Baryshkov wrote:
> > > On Fri, 12 Jan 2024 at 16:25, Krishna chaitanya chundru
> > > <[email protected]> wrote:
> > >>
> > >> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
> > >> maintains hardware state of a regulator by performing max aggregation of
> > >> the requests made by all of the processors.
> > >>
> > >> PCIe controller can operate on different RPMh performance state of power
> > >> domain based up on the speed of the link. And this performance state varies
> > >> from target to target.
> > >>
> > >> It is manadate to scale the performance state based up on the PCIe speed
> > >> link operates so that SoC can run under optimum power conditions.
> > >>
> > >> Add Operating Performance Points(OPP) support to vote for RPMh state based
> > >> upon GEN speed link is operating.
> > >>
> > >> OPP can handle ICC bw voting also, so move icc bw voting through opp
> > >> framework if opp entries are present.
> > >>
> > >> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
> > >> same icc bw and has frequency, so use frequency based search to reduce
> > >> number of entries in the opp table.
> > >>
> > >> Don't initialize icc if opp is supported.
> > >>
> > >> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> > >> ---
> > >> drivers/pci/controller/dwc/pcie-qcom.c | 83 ++++++++++++++++++++++++++++------
> > >> 1 file changed, 70 insertions(+), 13 deletions(-)
> > >>
> > >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > >> index 035953f0b6d8..31512dc9d6ff 100644
> > >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
[...]
> > >> static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
> > >> @@ -1471,8 +1502,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
> > >> static int qcom_pcie_probe(struct platform_device *pdev)
> > >> {
> > >> const struct qcom_pcie_cfg *pcie_cfg;
> > >> + unsigned long max_freq = INT_MAX;
> > >> struct device *dev = &pdev->dev;
> > >> struct qcom_pcie *pcie;
> > >> + struct dev_pm_opp *opp;
> > >> struct dw_pcie_rp *pp;
> > >> struct resource *res;
> > >> struct dw_pcie *pci;
> > >> @@ -1539,9 +1572,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > >> goto err_pm_runtime_put;
> > >> }
> > >>
> > >> - ret = qcom_pcie_icc_init(pcie);
> > >> - if (ret)
> > >> + /* OPP table is optional */
> > >> + ret = devm_pm_opp_of_add_table(dev);
> > >> + if (ret && ret != -ENODEV) {
> > >> + dev_err_probe(dev, ret, "Failed to add OPP table\n");
> > >> goto err_pm_runtime_put;
> > >> + }
> > >
> > > Can we initialise the table from the driver if it is not found? This
> > > will help us by having the common code later on.
> > >
> > we already icc voting if there is no opp table present in the dts.
>
> Yes. So later we have two different code paths: one for the OPP table
> being present and another one for the absent OPP table. My suggestion
> is to initialise minimal OPP table by hand and then have a common code
> path in qcom_pcie_icc_update().
>
Are you suggesting to duplicate DT in the driver?
- Mani
--
மணிவண்ணன் சதாசிவம்
On Thu, 1 Feb 2024 at 13:54, Manivannan Sadhasivam <[email protected]> wrote:
>
> On Tue, Jan 16, 2024 at 11:55:17AM +0200, Dmitry Baryshkov wrote:
> > On Tue, 16 Jan 2024 at 07:17, Krishna Chaitanya Chundru
> > <[email protected]> wrote:
> > >
> > >
> > >
> > > On 1/12/2024 9:03 PM, Dmitry Baryshkov wrote:
> > > > On Fri, 12 Jan 2024 at 16:25, Krishna chaitanya chundru
> > > > <[email protected]> wrote:
> > > >>
> > > >> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
> > > >> maintains hardware state of a regulator by performing max aggregation of
> > > >> the requests made by all of the processors.
> > > >>
> > > >> PCIe controller can operate on different RPMh performance state of power
> > > >> domain based up on the speed of the link. And this performance state varies
> > > >> from target to target.
> > > >>
> > > >> It is manadate to scale the performance state based up on the PCIe speed
> > > >> link operates so that SoC can run under optimum power conditions.
> > > >>
> > > >> Add Operating Performance Points(OPP) support to vote for RPMh state based
> > > >> upon GEN speed link is operating.
> > > >>
> > > >> OPP can handle ICC bw voting also, so move icc bw voting through opp
> > > >> framework if opp entries are present.
> > > >>
> > > >> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
> > > >> same icc bw and has frequency, so use frequency based search to reduce
> > > >> number of entries in the opp table.
> > > >>
> > > >> Don't initialize icc if opp is supported.
> > > >>
> > > >> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> > > >> ---
> > > >> drivers/pci/controller/dwc/pcie-qcom.c | 83 ++++++++++++++++++++++++++++------
> > > >> 1 file changed, 70 insertions(+), 13 deletions(-)
> > > >>
> > > >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > >> index 035953f0b6d8..31512dc9d6ff 100644
> > > >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>
> [...]
>
> > > >> static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
> > > >> @@ -1471,8 +1502,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
> > > >> static int qcom_pcie_probe(struct platform_device *pdev)
> > > >> {
> > > >> const struct qcom_pcie_cfg *pcie_cfg;
> > > >> + unsigned long max_freq = INT_MAX;
> > > >> struct device *dev = &pdev->dev;
> > > >> struct qcom_pcie *pcie;
> > > >> + struct dev_pm_opp *opp;
> > > >> struct dw_pcie_rp *pp;
> > > >> struct resource *res;
> > > >> struct dw_pcie *pci;
> > > >> @@ -1539,9 +1572,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >> goto err_pm_runtime_put;
> > > >> }
> > > >>
> > > >> - ret = qcom_pcie_icc_init(pcie);
> > > >> - if (ret)
> > > >> + /* OPP table is optional */
> > > >> + ret = devm_pm_opp_of_add_table(dev);
> > > >> + if (ret && ret != -ENODEV) {
> > > >> + dev_err_probe(dev, ret, "Failed to add OPP table\n");
> > > >> goto err_pm_runtime_put;
> > > >> + }
> > > >
> > > > Can we initialise the table from the driver if it is not found? This
> > > > will help us by having the common code later on.
> > > >
> > > we already icc voting if there is no opp table present in the dts.
> >
> > Yes. So later we have two different code paths: one for the OPP table
> > being present and another one for the absent OPP table. My suggestion
> > is to initialise minimal OPP table by hand and then have a common code
> > path in qcom_pcie_icc_update().
> >
>
> Are you suggesting to duplicate DT in the driver?
As a fallback for the cases when there is no OPP table in the driver
it might make sense. See
Otherwise the DT is still somewhat duplicated in the form of calling
icc functions directly.
--
With best wishes
Dmitry
On Thu, Feb 01, 2024 at 01:58:58PM +0200, Dmitry Baryshkov wrote:
> On Thu, 1 Feb 2024 at 13:54, Manivannan Sadhasivam <[email protected]> wrote:
> >
> > On Tue, Jan 16, 2024 at 11:55:17AM +0200, Dmitry Baryshkov wrote:
> > > On Tue, 16 Jan 2024 at 07:17, Krishna Chaitanya Chundru
> > > <[email protected]> wrote:
> > > >
> > > >
> > > >
> > > > On 1/12/2024 9:03 PM, Dmitry Baryshkov wrote:
> > > > > On Fri, 12 Jan 2024 at 16:25, Krishna chaitanya chundru
> > > > > <[email protected]> wrote:
> > > > >>
> > > > >> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
> > > > >> maintains hardware state of a regulator by performing max aggregation of
> > > > >> the requests made by all of the processors.
> > > > >>
> > > > >> PCIe controller can operate on different RPMh performance state of power
> > > > >> domain based up on the speed of the link. And this performance state varies
> > > > >> from target to target.
> > > > >>
> > > > >> It is manadate to scale the performance state based up on the PCIe speed
> > > > >> link operates so that SoC can run under optimum power conditions.
> > > > >>
> > > > >> Add Operating Performance Points(OPP) support to vote for RPMh state based
> > > > >> upon GEN speed link is operating.
> > > > >>
> > > > >> OPP can handle ICC bw voting also, so move icc bw voting through opp
> > > > >> framework if opp entries are present.
> > > > >>
> > > > >> In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
> > > > >> same icc bw and has frequency, so use frequency based search to reduce
> > > > >> number of entries in the opp table.
> > > > >>
> > > > >> Don't initialize icc if opp is supported.
> > > > >>
> > > > >> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> > > > >> ---
> > > > >> drivers/pci/controller/dwc/pcie-qcom.c | 83 ++++++++++++++++++++++++++++------
> > > > >> 1 file changed, 70 insertions(+), 13 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > >> index 035953f0b6d8..31512dc9d6ff 100644
> > > > >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >
> > [...]
> >
> > > > >> static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
> > > > >> @@ -1471,8 +1502,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
> > > > >> static int qcom_pcie_probe(struct platform_device *pdev)
> > > > >> {
> > > > >> const struct qcom_pcie_cfg *pcie_cfg;
> > > > >> + unsigned long max_freq = INT_MAX;
> > > > >> struct device *dev = &pdev->dev;
> > > > >> struct qcom_pcie *pcie;
> > > > >> + struct dev_pm_opp *opp;
> > > > >> struct dw_pcie_rp *pp;
> > > > >> struct resource *res;
> > > > >> struct dw_pcie *pci;
> > > > >> @@ -1539,9 +1572,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > > >> goto err_pm_runtime_put;
> > > > >> }
> > > > >>
> > > > >> - ret = qcom_pcie_icc_init(pcie);
> > > > >> - if (ret)
> > > > >> + /* OPP table is optional */
> > > > >> + ret = devm_pm_opp_of_add_table(dev);
> > > > >> + if (ret && ret != -ENODEV) {
> > > > >> + dev_err_probe(dev, ret, "Failed to add OPP table\n");
> > > > >> goto err_pm_runtime_put;
> > > > >> + }
> > > > >
> > > > > Can we initialise the table from the driver if it is not found? This
> > > > > will help us by having the common code later on.
> > > > >
> > > > we already icc voting if there is no opp table present in the dts.
> > >
> > > Yes. So later we have two different code paths: one for the OPP table
> > > being present and another one for the absent OPP table. My suggestion
> > > is to initialise minimal OPP table by hand and then have a common code
> > > path in qcom_pcie_icc_update().
> > >
> >
> > Are you suggesting to duplicate DT in the driver?
>
> As a fallback for the cases when there is no OPP table in the driver
> it might make sense. See
> Otherwise the DT is still somewhat duplicated in the form of calling
> icc functions directly.
>
No, DT is not duplicated. With this approach, we will end up hardcoding the DT
entries in the driver which sounds backwards to me. Even with 2 different code
paths, the hardware info will be left to the DT itself, so the driver just
consumes it.
So please, let's not do it.
- Mani
--
மணிவண்ணன் சதாசிவம்
On 31.01.2024 06:23, Viresh Kumar wrote:
> On 30-01-24, 18:46, Manivannan Sadhasivam wrote:
>> Agree. But what I'm saying is, right now there is no DT property in the
>> interconnect consumer nodes to specificy the bw requirements. This is all
>> hardcoded in the respective ICC consumer drivers.
>
> I thought there are a lot of users already in there..
>
> $ git grep -i opp.*bps arch/arm64/boot/dts/ | wc -l
> 864
>
>> But when we use OPP to control bw, the bw requirements come from DT. This is
>> what I see as a difference. Because, only nodes making use of OPP will specify
>> bw in DT and other nodes making use of just ICC will not.
>>
>> Maybe I'm worrying too much about these details... But it looks like
>> inconsistency to me.
>
> Right. So is there inconsistency right now ? Yes, there is.
>
> The important question we need to answer is where do we want to see
> all these drivers (specially new ones) in the future. What's the right
> thing to do eventually ? Hardcode stuff ? Or Move it to DT ?
>
> The answer is DT for me, so the code can be generic enough to be
> reused. This is just one step in the right direction I guess.
> Eventually the drivers must get simplified, which they are I guess.
I'm lukewarm on this.
A *lot* of hardware has more complex requirements than "x MBps at y MHz",
especially when performance counters come into the picture for dynamic
bw management.
OPP tables can't really handle this properly.
Konrad
On 01-02-24, 15:45, Konrad Dybcio wrote:
> I'm lukewarm on this.
>
> A *lot* of hardware has more complex requirements than "x MBps at y MHz",
> especially when performance counters come into the picture for dynamic
> bw management.
>
> OPP tables can't really handle this properly.
There was a similar concern for voltages earlier on and we added the capability
of adjusting the voltage for OPPs in the OPP core. Maybe something similar can
be done here ?
--
viresh
On 2.02.2024 08:33, Viresh Kumar wrote:
> On 01-02-24, 15:45, Konrad Dybcio wrote:
>> I'm lukewarm on this.
>>
>> A *lot* of hardware has more complex requirements than "x MBps at y MHz",
>> especially when performance counters come into the picture for dynamic
>> bw management.
>>
>> OPP tables can't really handle this properly.
>
> There was a similar concern for voltages earlier on and we added the capability
> of adjusting the voltage for OPPs in the OPP core. Maybe something similar can
> be done here ?
>
I really don't think it's fitting.. At any moment the device may require any
bandwidth value between 0 and MAX_BW_PER_LINK_GEN * LINK_WIDTH..
Konrad
On 2/10/2024 2:44 AM, Konrad Dybcio wrote:
> On 2.02.2024 08:33, Viresh Kumar wrote:
>> On 01-02-24, 15:45, Konrad Dybcio wrote:
>>> I'm lukewarm on this.
>>>
>>> A *lot* of hardware has more complex requirements than "x MBps at y MHz",
>>> especially when performance counters come into the picture for dynamic
>>> bw management.
>>>
>>> OPP tables can't really handle this properly.
>>
>> There was a similar concern for voltages earlier on and we added the capability
>> of adjusting the voltage for OPPs in the OPP core. Maybe something similar can
>> be done here ?
>>
> I really don't think it's fitting.. At any moment the device may require any
> bandwidth value between 0 and MAX_BW_PER_LINK_GEN * LINK_WIDTH..
>
> Konrad
Viresh & konrad can you both come to conclusion on this.
- Krishna Chaitanya.
On 09-02-24, 22:14, Konrad Dybcio wrote:
> On 2.02.2024 08:33, Viresh Kumar wrote:
> > On 01-02-24, 15:45, Konrad Dybcio wrote:
> >> I'm lukewarm on this.
> >>
> >> A *lot* of hardware has more complex requirements than "x MBps at y MHz",
> >> especially when performance counters come into the picture for dynamic
> >> bw management.
> >>
> >> OPP tables can't really handle this properly.
> >
> > There was a similar concern for voltages earlier on and we added the capability
> > of adjusting the voltage for OPPs in the OPP core. Maybe something similar can
> > be done here ?
> >
> I really don't think it's fitting.. At any moment the device may require any
> bandwidth value between 0 and MAX_BW_PER_LINK_GEN * LINK_WIDTH..
Okay, I leave it up to you guys to decide on how you want to do it. I still
believe getting the information via DT is the right thing, but maybe I still
don't understand the problem fully.
Thanks.
--
viresh
On Mon, Feb 19, 2024 at 03:58:34PM +0530, Viresh Kumar wrote:
> On 09-02-24, 22:14, Konrad Dybcio wrote:
> > On 2.02.2024 08:33, Viresh Kumar wrote:
> > > On 01-02-24, 15:45, Konrad Dybcio wrote:
> > >> I'm lukewarm on this.
> > >>
> > >> A *lot* of hardware has more complex requirements than "x MBps at y MHz",
> > >> especially when performance counters come into the picture for dynamic
> > >> bw management.
> > >>
> > >> OPP tables can't really handle this properly.
> > >
> > > There was a similar concern for voltages earlier on and we added the capability
> > > of adjusting the voltage for OPPs in the OPP core. Maybe something similar can
> > > be done here ?
> > >
> > I really don't think it's fitting.. At any moment the device may require any
> > bandwidth value between 0 and MAX_BW_PER_LINK_GEN * LINK_WIDTH..
>
> Okay, I leave it up to you guys to decide on how you want to do it. I still
> believe getting the information via DT is the right thing, but maybe I still
> don't understand the problem fully.
>
I argued for a different issue, but what Konrad pointed out is not a valid
concern to me. The driver may only require _fixed_ bandwidth between 0 and
(MAX_BW_PER_LINK_GEN * LINK_WIDTH) and DT can pass those bandwidth values.
Chaitanya pointed out that this may end up with long entries in DT once the PCIe
Gen versions start to increase (current Qcom platforms support upto Gen 4 only).
But that shouldn't be a real concern if we look at what DT has to provide.
- Mani
--
மணிவண்ணன் சதாசிவம்