* [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path
[not found] <1686154687-29356-1-git-send-email-quic_krichai@quicinc.com>
@ 2023-06-07 16:18 ` Krishna chaitanya chundru
2023-06-07 17:21 ` Rob Herring
2023-06-08 15:27 ` Rob Herring
2023-06-07 16:18 ` [PATCH v2 2/3] arm: dts: qcom: sdx55: Add interconnect path Krishna chaitanya chundru
2023-06-07 16:18 ` [PATCH v2 3/3] PCI: qcom-ep: Add ICC bandwidth voting support Krishna chaitanya chundru
2 siblings, 2 replies; 14+ messages in thread
From: Krishna chaitanya chundru @ 2023-06-07 16:18 UTC (permalink / raw)
To: manivannan.sadhasivam
Cc: quic_vbadigan, quic_ramkri, Krishna chaitanya chundru, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
open list:ARM/QUALCOMM SUPPORT,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
Add the "pcie-mem" interconnect path to the bindings.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
index b3c22eb..6fc5440 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
@@ -70,6 +70,13 @@ properties:
description: GPIO used as WAKE# output signal
maxItems: 1
+ interconnects:
+ maxItems: 1
+
+ interconnect-names:
+ items:
+ - const: pcie-mem
+
resets:
maxItems: 1
@@ -97,6 +104,8 @@ required:
- interrupts
- interrupt-names
- reset-gpios
+ - interconnects
+ - interconnect-names
- resets
- reset-names
- power-domains
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] arm: dts: qcom: sdx55: Add interconnect path
[not found] <1686154687-29356-1-git-send-email-quic_krichai@quicinc.com>
2023-06-07 16:18 ` [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path Krishna chaitanya chundru
@ 2023-06-07 16:18 ` Krishna chaitanya chundru
2023-06-08 16:00 ` Manivannan Sadhasivam
2023-06-09 11:09 ` kernel test robot
2023-06-07 16:18 ` [PATCH v2 3/3] PCI: qcom-ep: Add ICC bandwidth voting support Krishna chaitanya chundru
2 siblings, 2 replies; 14+ messages in thread
From: Krishna chaitanya chundru @ 2023-06-07 16:18 UTC (permalink / raw)
To: manivannan.sadhasivam
Cc: quic_vbadigan, quic_ramkri, Krishna chaitanya chundru, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, open list:ARM/QUALCOMM SUPPORT,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
Add pcie-mem interconnect path to sdx55 target.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
arch/arm/boot/dts/qcom-sdx55.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-sdx55.dtsi b/arch/arm/boot/dts/qcom-sdx55.dtsi
index 342c3d1..e9f8bfe 100644
--- a/arch/arm/boot/dts/qcom-sdx55.dtsi
+++ b/arch/arm/boot/dts/qcom-sdx55.dtsi
@@ -421,6 +421,10 @@
<GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "global",
"doorbell";
+
+ interconnects = <&system_noc MASTER_PCIE_0 0 &mc_virt SLAVE_EBI1 0>;
+ interconnect-names = "pci-mem";
+
resets = <&gcc GCC_PCIE_BCR>;
reset-names = "core";
power-domains = <&gcc PCIE_GDSC>;
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] PCI: qcom-ep: Add ICC bandwidth voting support
[not found] <1686154687-29356-1-git-send-email-quic_krichai@quicinc.com>
2023-06-07 16:18 ` [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path Krishna chaitanya chundru
2023-06-07 16:18 ` [PATCH v2 2/3] arm: dts: qcom: sdx55: Add interconnect path Krishna chaitanya chundru
@ 2023-06-07 16:18 ` Krishna chaitanya chundru
2023-06-07 16:43 ` Bjorn Helgaas
2 siblings, 1 reply; 14+ messages in thread
From: Krishna chaitanya chundru @ 2023-06-07 16:18 UTC (permalink / raw)
To: manivannan.sadhasivam
Cc: quic_vbadigan, quic_ramkri, Krishna chaitanya chundru,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM, open list
Add support to vote for ICC bandwidth based on the link
speed and width.
This patch is inspired from pcie-qcom driver to add basic
interconnect support.
Link:https://patchwork.kernel.org/project/linux-pci/patch/20221102090705.23634-3-johan+linaro@kernel.org/
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
drivers/pci/controller/dwc/pcie-qcom-ep.c | 68 +++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 19b3283..5f9139d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -13,6 +13,7 @@
#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
+#include <linux/interconnect.h>
#include <linux/mfd/syscon.h>
#include <linux/phy/pcie.h>
#include <linux/phy/phy.h>
@@ -28,6 +29,7 @@
#define PARF_SYS_CTRL 0x00
#define PARF_DB_CTRL 0x10
#define PARF_PM_CTRL 0x20
+#define PARF_PM_STTS 0x24
#define PARF_MHI_CLOCK_RESET_CTRL 0x174
#define PARF_MHI_BASE_ADDR_LOWER 0x178
#define PARF_MHI_BASE_ADDR_UPPER 0x17c
@@ -128,6 +130,9 @@
/* DBI register fields */
#define DBI_CON_STATUS_POWER_STATE_MASK GENMASK(1, 0)
+#define DBI_LINKCTRLSTATUS 0x80
+#define DBI_LINKCTRLSTATUS_SHIFT 16
+
#define XMLH_LINK_UP 0x400
#define CORE_RESET_TIME_US_MIN 1000
#define CORE_RESET_TIME_US_MAX 1005
@@ -178,6 +183,8 @@ struct qcom_pcie_ep {
struct phy *phy;
struct dentry *debugfs;
+ struct icc_path *icc;
+
struct clk_bulk_data *clks;
int num_clks;
@@ -253,9 +260,51 @@ static void qcom_pcie_dw_stop_link(struct dw_pcie *pci)
disable_irq(pcie_ep->perst_irq);
}
+static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
+{
+ struct dw_pcie *pci = &pcie_ep->pci;
+ int speed, width;
+ u32 val, bw;
+ int ret;
+
+ if (!pcie_ep->icc)
+ return;
+
+ val = dw_pcie_readl_dbi(pci, DBI_LINKCTRLSTATUS);
+ val = val >> DBI_LINKCTRLSTATUS_SHIFT;
+
+ speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
+ width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
+
+ switch (speed) {
+ case 1:
+ bw = MBps_to_icc(250); /* BW for GEN1 per lane: 250MBps */
+ break;
+ case 2:
+ bw = MBps_to_icc(500); /* BW for GEN2 per lane: 500MBps */
+ break;
+ case 3:
+ bw = MBps_to_icc(985); /* BW for GEN3 per lane: 985MBps */
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ fallthrough;
+ case 4:
+ bw = MBps_to_icc(1969); /* BW for GEN4 per lane: 985MBps */
+ break;
+ }
+
+ ret = icc_set_bw(pcie_ep->icc, 0, width * bw);
+ if (ret) {
+ dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+ ret);
+ }
+}
+
static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
{
int ret;
+ struct dw_pcie *pci = &pcie_ep->pci;
ret = clk_bulk_prepare_enable(pcie_ep->num_clks, pcie_ep->clks);
if (ret)
@@ -277,6 +326,20 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
if (ret)
goto err_phy_exit;
+ /*
+ * Some Qualcomm platforms require interconnect bandwidth constraints
+ * to be set before enabling interconnect clocks.
+ *
+ * Set an initial average bandwidth corresponding to single-lane Gen 1
+ * for the pcie to mem path.
+ */
+ ret = icc_set_bw(pcie_ep->icc, 0, MBps_to_icc(250)); /* BW for GEN1 per lane: 250MBps */
+ if (ret) {
+ dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+ ret);
+ goto err_phy_exit;
+ }
+
return 0;
err_phy_exit:
@@ -550,6 +613,10 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
if (IS_ERR(pcie_ep->phy))
ret = PTR_ERR(pcie_ep->phy);
+ pcie_ep->icc = devm_of_icc_get(dev, "pcie-mem");
+ if (IS_ERR(pcie_ep->icc))
+ ret = PTR_ERR(pcie_ep->icc);
+
return ret;
}
@@ -572,6 +639,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data)
} else if (FIELD_GET(PARF_INT_ALL_BME, status)) {
dev_dbg(dev, "Received BME event. Link is enabled!\n");
pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
+ qcom_pcie_ep_icc_update(pcie_ep);
} else if (FIELD_GET(PARF_INT_ALL_PM_TURNOFF, status)) {
dev_dbg(dev, "Received PM Turn-off event! Entering L23\n");
val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] PCI: qcom-ep: Add ICC bandwidth voting support
2023-06-07 16:18 ` [PATCH v2 3/3] PCI: qcom-ep: Add ICC bandwidth voting support Krishna chaitanya chundru
@ 2023-06-07 16:43 ` Bjorn Helgaas
2023-06-09 11:50 ` Krishna Chaitanya Chundru
2023-06-09 11:52 ` Krishna Chaitanya Chundru
0 siblings, 2 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2023-06-07 16:43 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: manivannan.sadhasivam, quic_vbadigan, quic_ramkri,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM, open list
On Wed, Jun 07, 2023 at 09:48:07PM +0530, Krishna chaitanya chundru wrote:
> Add support to vote for ICC bandwidth based on the link
> speed and width.
>
> This patch is inspired from pcie-qcom driver to add basic
> interconnect support.
Wrap to fill 75 columns like the rest of the git history.
> Link:https://patchwork.kernel.org/project/linux-pci/patch/20221102090705.23634-3-johan+linaro@kernel.org/
Add space after "Link:"
Probably better to use a lore link
(https://lore.kernel.org/r/20221102090705.23634-3-johan+linaro@kernel.org/)
because I think lore is more general-purpose than patchwork and we
don't get any benefit from the patchwork features here.
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> drivers/pci/controller/dwc/pcie-qcom-ep.c | 68 +++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 19b3283..5f9139d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -13,6 +13,7 @@
> #include <linux/debugfs.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> +#include <linux/interconnect.h>
> #include <linux/mfd/syscon.h>
> #include <linux/phy/pcie.h>
> #include <linux/phy/phy.h>
> @@ -28,6 +29,7 @@
> #define PARF_SYS_CTRL 0x00
> #define PARF_DB_CTRL 0x10
> #define PARF_PM_CTRL 0x20
> +#define PARF_PM_STTS 0x24
> #define PARF_MHI_CLOCK_RESET_CTRL 0x174
> #define PARF_MHI_BASE_ADDR_LOWER 0x178
> #define PARF_MHI_BASE_ADDR_UPPER 0x17c
> @@ -128,6 +130,9 @@
> /* DBI register fields */
> #define DBI_CON_STATUS_POWER_STATE_MASK GENMASK(1, 0)
>
> +#define DBI_LINKCTRLSTATUS 0x80
> +#define DBI_LINKCTRLSTATUS_SHIFT 16
> +
> #define XMLH_LINK_UP 0x400
> #define CORE_RESET_TIME_US_MIN 1000
> #define CORE_RESET_TIME_US_MAX 1005
> @@ -178,6 +183,8 @@ struct qcom_pcie_ep {
> struct phy *phy;
> struct dentry *debugfs;
>
> + struct icc_path *icc;
Seems gratuitously different from the "icc_mem" name used in
pcie-qcom. Use the same name unless there's a reason to be different.
> struct clk_bulk_data *clks;
> int num_clks;
>
> @@ -253,9 +260,51 @@ static void qcom_pcie_dw_stop_link(struct dw_pcie *pci)
> disable_irq(pcie_ep->perst_irq);
> }
>
> +static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
> +{
> + struct dw_pcie *pci = &pcie_ep->pci;
> + int speed, width;
> + u32 val, bw;
> + int ret;
> +
> + if (!pcie_ep->icc)
> + return;
> +
> + val = dw_pcie_readl_dbi(pci, DBI_LINKCTRLSTATUS);
> + val = val >> DBI_LINKCTRLSTATUS_SHIFT;
> +
> + speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
This whole function is basically identical to qcom_pcie_icc_update().
Could a single implementation be shared? qcom_pcie_icc_update() uses
dw_pcie_find_capability(pci, PCI_CAP_ID_EXP) instead of the hard-coded
DBI_LINKCTRLSTATUS, but there are other instances of
dw_pcie_find_capability() in pcie-qcom-ep.c, so it seems possible that
the same code could be used both places.
> + switch (speed) {
> + case 1:
> + bw = MBps_to_icc(250); /* BW for GEN1 per lane: 250MBps */
> + break;
> + case 2:
> + bw = MBps_to_icc(500); /* BW for GEN2 per lane: 500MBps */
> + break;
> + case 3:
> + bw = MBps_to_icc(985); /* BW for GEN3 per lane: 985MBps */
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + fallthrough;
> + case 4:
> + bw = MBps_to_icc(1969); /* BW for GEN4 per lane: 985MBps */
> + break;
> + }
> +
> + ret = icc_set_bw(pcie_ep->icc, 0, width * bw);
> + if (ret) {
> + dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> + ret);
> + }
> +}
> +
> static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
> {
> int ret;
> + struct dw_pcie *pci = &pcie_ep->pci;
>
> ret = clk_bulk_prepare_enable(pcie_ep->num_clks, pcie_ep->clks);
> if (ret)
> @@ -277,6 +326,20 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
> if (ret)
> goto err_phy_exit;
>
> + /*
> + * Some Qualcomm platforms require interconnect bandwidth constraints
> + * to be set before enabling interconnect clocks.
> + *
> + * Set an initial average bandwidth corresponding to single-lane Gen 1
> + * for the pcie to mem path.
> + */
> + ret = icc_set_bw(pcie_ep->icc, 0, MBps_to_icc(250)); /* BW for GEN1 per lane: 250MBps */
Reformat the comment so this all fits in 80 columns like the rest of
the file.
> + if (ret) {
> + dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> + ret);
> + goto err_phy_exit;
> + }
This plus the pcie_ep->icc init below is basically identical to
qcom_pcie_icc_init() in pcie_qcom.c. Why not use the same structure
here, with a qcom_pcie_icc_init() function? It's better to be the
same than different (when possible, of course).
> return 0;
>
> err_phy_exit:
> @@ -550,6 +613,10 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
> if (IS_ERR(pcie_ep->phy))
> ret = PTR_ERR(pcie_ep->phy);
>
> + pcie_ep->icc = devm_of_icc_get(dev, "pcie-mem");
> + if (IS_ERR(pcie_ep->icc))
> + ret = PTR_ERR(pcie_ep->icc);
> +
> return ret;
> }
>
> @@ -572,6 +639,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data)
> } else if (FIELD_GET(PARF_INT_ALL_BME, status)) {
> dev_dbg(dev, "Received BME event. Link is enabled!\n");
> pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
> + qcom_pcie_ep_icc_update(pcie_ep);
> } else if (FIELD_GET(PARF_INT_ALL_PM_TURNOFF, status)) {
> dev_dbg(dev, "Received PM Turn-off event! Entering L23\n");
> val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path
2023-06-07 16:18 ` [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path Krishna chaitanya chundru
@ 2023-06-07 17:21 ` Rob Herring
2023-06-09 11:55 ` Krishna Chaitanya Chundru
2023-06-08 15:27 ` Rob Herring
1 sibling, 1 reply; 14+ messages in thread
From: Rob Herring @ 2023-06-07 17:21 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: quic_vbadigan, Lorenzo Pieralisi, quic_ramkri, Konrad Dybcio,
Conor Dooley, devicetree, linux-kernel, Krzysztof Wilczyński,
linux-pci, Bjorn Helgaas, linux-arm-msm, Bjorn Andersson,
manivannan.sadhasivam, Krzysztof Kozlowski, Manivannan Sadhasivam,
Andy Gross
On Wed, 07 Jun 2023 21:48:05 +0530, Krishna chaitanya chundru wrote:
> Add the "pcie-mem" interconnect path to the bindings.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb: pcie-ep@1c00000: 'interconnects' is a required property
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb: pcie-ep@1c00000: 'interconnect-names' is a required property
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1686154687-29356-2-git-send-email-quic_krichai@quicinc.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path
2023-06-07 16:18 ` [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path Krishna chaitanya chundru
2023-06-07 17:21 ` Rob Herring
@ 2023-06-08 15:27 ` Rob Herring
2023-06-08 15:52 ` Manivannan Sadhasivam
1 sibling, 1 reply; 14+ messages in thread
From: Rob Herring @ 2023-06-08 15:27 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: manivannan.sadhasivam, quic_vbadigan, quic_ramkri, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, open list:ARM/QUALCOMM SUPPORT,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
On Wed, Jun 07, 2023 at 09:48:05PM +0530, Krishna chaitanya chundru wrote:
> Add the "pcie-mem" interconnect path to the bindings.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> index b3c22eb..6fc5440 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> @@ -70,6 +70,13 @@ properties:
> description: GPIO used as WAKE# output signal
> maxItems: 1
>
> + interconnects:
> + maxItems: 1
> +
> + interconnect-names:
> + items:
> + - const: pcie-mem
> +
> resets:
> maxItems: 1
>
> @@ -97,6 +104,8 @@ required:
> - interrupts
> - interrupt-names
> - reset-gpios
> + - interconnects
> + - interconnect-names
You can't add required properties. That's an ABI break. Up to the
platform whether that's acceptible, but you have to explain all this in
the commmit msg.
> - resets
> - reset-names
> - power-domains
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path
2023-06-08 15:27 ` Rob Herring
@ 2023-06-08 15:52 ` Manivannan Sadhasivam
0 siblings, 0 replies; 14+ messages in thread
From: Manivannan Sadhasivam @ 2023-06-08 15:52 UTC (permalink / raw)
To: Rob Herring
Cc: Krishna chaitanya chundru, manivannan.sadhasivam, quic_vbadigan,
quic_ramkri, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, open list:ARM/QUALCOMM SUPPORT,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
On Thu, Jun 08, 2023 at 09:27:59AM -0600, Rob Herring wrote:
> On Wed, Jun 07, 2023 at 09:48:05PM +0530, Krishna chaitanya chundru wrote:
> > Add the "pcie-mem" interconnect path to the bindings.
> >
> > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > ---
> > Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> > index b3c22eb..6fc5440 100644
> > --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> > @@ -70,6 +70,13 @@ properties:
> > description: GPIO used as WAKE# output signal
> > maxItems: 1
> >
> > + interconnects:
> > + maxItems: 1
> > +
> > + interconnect-names:
> > + items:
> > + - const: pcie-mem
> > +
> > resets:
> > maxItems: 1
> >
> > @@ -97,6 +104,8 @@ required:
> > - interrupts
> > - interrupt-names
> > - reset-gpios
> > + - interconnects
> > + - interconnect-names
>
> You can't add required properties. That's an ABI break. Up to the
> platform whether that's acceptible, but you have to explain all this in
> the commmit msg.
>
Some platforms may not boot if a device driver doesn't initialize the
interconnect path. Mostly it is all handled by the bootloader but we have
starting to see cases where bootloader simply ignores them.
So I'd say that these need to be made required (should've been from the start
but I take the blame). And yes, this info should be part of the commit message.
- Mani
> > - resets
> > - reset-names
> > - power-domains
> > --
> > 2.7.4
> >
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] arm: dts: qcom: sdx55: Add interconnect path
2023-06-07 16:18 ` [PATCH v2 2/3] arm: dts: qcom: sdx55: Add interconnect path Krishna chaitanya chundru
@ 2023-06-08 16:00 ` Manivannan Sadhasivam
2023-06-09 11:09 ` kernel test robot
1 sibling, 0 replies; 14+ messages in thread
From: Manivannan Sadhasivam @ 2023-06-08 16:00 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: manivannan.sadhasivam, quic_vbadigan, quic_ramkri, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, open list:ARM/QUALCOMM SUPPORT,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
On Wed, Jun 07, 2023 at 09:48:06PM +0530, Krishna chaitanya chundru wrote:
> Add pcie-mem interconnect path to sdx55 target.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
- Mani
> ---
> arch/arm/boot/dts/qcom-sdx55.dtsi | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/boot/dts/qcom-sdx55.dtsi b/arch/arm/boot/dts/qcom-sdx55.dtsi
> index 342c3d1..e9f8bfe 100644
> --- a/arch/arm/boot/dts/qcom-sdx55.dtsi
> +++ b/arch/arm/boot/dts/qcom-sdx55.dtsi
> @@ -421,6 +421,10 @@
> <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
> interrupt-names = "global",
> "doorbell";
> +
> + interconnects = <&system_noc MASTER_PCIE_0 0 &mc_virt SLAVE_EBI1 0>;
> + interconnect-names = "pci-mem";
> +
> resets = <&gcc GCC_PCIE_BCR>;
> reset-names = "core";
> power-domains = <&gcc PCIE_GDSC>;
> --
> 2.7.4
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] arm: dts: qcom: sdx55: Add interconnect path
2023-06-07 16:18 ` [PATCH v2 2/3] arm: dts: qcom: sdx55: Add interconnect path Krishna chaitanya chundru
2023-06-08 16:00 ` Manivannan Sadhasivam
@ 2023-06-09 11:09 ` kernel test robot
1 sibling, 0 replies; 14+ messages in thread
From: kernel test robot @ 2023-06-09 11:09 UTC (permalink / raw)
To: Krishna chaitanya chundru, manivannan.sadhasivam; +Cc: linux-kernel
Hi Krishna,
kernel test robot noticed the following build errors:
[auto build test ERROR on pci/for-linus]
[also build test ERROR on robh/for-next linus/master v6.4-rc5 next-20230608]
[cannot apply to pci/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/arm-dts-qcom-sdx55-Add-interconnect-path/20230608-011823
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git for-linus
patch link: https://lore.kernel.org/r/1686154687-29356-3-git-send-email-quic_krichai%40quicinc.com
patch subject: [PATCH v2 2/3] arm: dts: qcom: sdx55: Add interconnect path
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20230609/202306091848.ra1frGj3-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git remote add pci https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git
git fetch pci for-linus
git checkout pci/for-linus
b4 shazam https://lore.kernel.org/r/1686154687-29356-3-git-send-email-quic_krichai@quicinc.com
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306091848.ra1frGj3-lkp@intel.com/
All errors (new ones prefixed by >>):
>> Error: arch/arm/boot/dts/qcom-sdx55.dtsi:425.33-34 syntax error
FATAL ERROR: Unable to parse input tree
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] PCI: qcom-ep: Add ICC bandwidth voting support
2023-06-07 16:43 ` Bjorn Helgaas
@ 2023-06-09 11:50 ` Krishna Chaitanya Chundru
2023-06-09 11:52 ` Krishna Chaitanya Chundru
1 sibling, 0 replies; 14+ messages in thread
From: Krishna Chaitanya Chundru @ 2023-06-09 11:50 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: manivannan.sadhasivam, quic_vbadigan, quic_ramkri,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM, open list
On 6/7/2023 10:13 PM, Bjorn Helgaas wrote:
> On Wed, Jun 07, 2023 at 09:48:07PM +0530, Krishna chaitanya chundru wrote:
>> Add support to vote for ICC bandwidth based on the link
>> speed and width.
>>
>> This patch is inspired from pcie-qcom driver to add basic
>> interconnect support.
> Wrap to fill 75 columns like the rest of the git history.
>
>> Link:https://patchwork.kernel.org/project/linux-pci/patch/20221102090705.23634-3-johan+linaro@kernel.org/
> Add space after "Link:"
>
> Probably better to use a lore link
> (https://lore.kernel.org/r/20221102090705.23634-3-johan+linaro@kernel.org/)
> because I think lore is more general-purpose than patchwork and we
> don't get any benefit from the patchwork features here.
done
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>> drivers/pci/controller/dwc/pcie-qcom-ep.c | 68 +++++++++++++++++++++++++++++++
>> 1 file changed, 68 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> index 19b3283..5f9139d 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> @@ -13,6 +13,7 @@
>> #include <linux/debugfs.h>
>> #include <linux/delay.h>
>> #include <linux/gpio/consumer.h>
>> +#include <linux/interconnect.h>
>> #include <linux/mfd/syscon.h>
>> #include <linux/phy/pcie.h>
>> #include <linux/phy/phy.h>
>> @@ -28,6 +29,7 @@
>> #define PARF_SYS_CTRL 0x00
>> #define PARF_DB_CTRL 0x10
>> #define PARF_PM_CTRL 0x20
>> +#define PARF_PM_STTS 0x24
>> #define PARF_MHI_CLOCK_RESET_CTRL 0x174
>> #define PARF_MHI_BASE_ADDR_LOWER 0x178
>> #define PARF_MHI_BASE_ADDR_UPPER 0x17c
>> @@ -128,6 +130,9 @@
>> /* DBI register fields */
>> #define DBI_CON_STATUS_POWER_STATE_MASK GENMASK(1, 0)
>>
>> +#define DBI_LINKCTRLSTATUS 0x80
>> +#define DBI_LINKCTRLSTATUS_SHIFT 16
>> +
>> #define XMLH_LINK_UP 0x400
>> #define CORE_RESET_TIME_US_MIN 1000
>> #define CORE_RESET_TIME_US_MAX 1005
>> @@ -178,6 +183,8 @@ struct qcom_pcie_ep {
>> struct phy *phy;
>> struct dentry *debugfs;
>>
>> + struct icc_path *icc;
> Seems gratuitously different from the "icc_mem" name used in
> pcie-qcom. Use the same name unless there's a reason to be different.
done
>> struct clk_bulk_data *clks;
>> int num_clks;
>>
>> @@ -253,9 +260,51 @@ static void qcom_pcie_dw_stop_link(struct dw_pcie *pci)
>> disable_irq(pcie_ep->perst_irq);
>> }
>>
>> +static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
>> +{
>> + struct dw_pcie *pci = &pcie_ep->pci;
>> + int speed, width;
>> + u32 val, bw;
>> + int ret;
>> +
>> + if (!pcie_ep->icc)
>> + return;
>> +
>> + val = dw_pcie_readl_dbi(pci, DBI_LINKCTRLSTATUS);
>> + val = val >> DBI_LINKCTRLSTATUS_SHIFT;
>> +
>> + speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
>> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
> This whole function is basically identical to qcom_pcie_icc_update().
> Could a single implementation be shared? qcom_pcie_icc_update() uses
> dw_pcie_find_capability(pci, PCI_CAP_ID_EXP) instead of the hard-coded
> DBI_LINKCTRLSTATUS, but there are other instances of
> dw_pcie_find_capability() in pcie-qcom-ep.c, so it seems possible that
> the same code could be used both places.
done
>> + switch (speed) {
>> + case 1:
>> + bw = MBps_to_icc(250); /* BW for GEN1 per lane: 250MBps */
>> + break;
>> + case 2:
>> + bw = MBps_to_icc(500); /* BW for GEN2 per lane: 500MBps */
>> + break;
>> + case 3:
>> + bw = MBps_to_icc(985); /* BW for GEN3 per lane: 985MBps */
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> + fallthrough;
>> + case 4:
>> + bw = MBps_to_icc(1969); /* BW for GEN4 per lane: 985MBps */
>> + break;
>> + }
>> +
>> + ret = icc_set_bw(pcie_ep->icc, 0, width * bw);
>> + if (ret) {
>> + dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>> + ret);
>> + }
>> +}
>> +
>> static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
>> {
>> int ret;
>> + struct dw_pcie *pci = &pcie_ep->pci;
>>
>> ret = clk_bulk_prepare_enable(pcie_ep->num_clks, pcie_ep->clks);
>> if (ret)
>> @@ -277,6 +326,20 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
>> if (ret)
>> goto err_phy_exit;
>>
>> + /*
>> + * Some Qualcomm platforms require interconnect bandwidth constraints
>> + * to be set before enabling interconnect clocks.
>> + *
>> + * Set an initial average bandwidth corresponding to single-lane Gen 1
>> + * for the pcie to mem path.
>> + */
>> + ret = icc_set_bw(pcie_ep->icc, 0, MBps_to_icc(250)); /* BW for GEN1 per lane: 250MBps */
> Reformat the comment so this all fits in 80 columns like the rest of
> the file.
done
>> + if (ret) {
>> + dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>> + ret);
>> + goto err_phy_exit;
>> + }
> This plus the pcie_ep->icc init below is basically identical to
> qcom_pcie_icc_init() in pcie_qcom.c. Why not use the same structure
> here, with a qcom_pcie_icc_init() function? It's better to be the
> same than different (when possible, of course).
In pcie_qcom.c driver the resources will be turned on the probe itself.
But in pcie-qcom-ep.c driver will wait for the host to toggle the perst
to enable all the resources
that is reason it is different here.
- Krishna chaitanya.
>> return 0;
>>
>> err_phy_exit:
>> @@ -550,6 +613,10 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
>> if (IS_ERR(pcie_ep->phy))
>> ret = PTR_ERR(pcie_ep->phy);
>>
>> + pcie_ep->icc = devm_of_icc_get(dev, "pcie-mem");
>> + if (IS_ERR(pcie_ep->icc))
>> + ret = PTR_ERR(pcie_ep->icc);
>> +
>> return ret;
>> }
>>
>> @@ -572,6 +639,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data)
>> } else if (FIELD_GET(PARF_INT_ALL_BME, status)) {
>> dev_dbg(dev, "Received BME event. Link is enabled!\n");
>> pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
>> + qcom_pcie_ep_icc_update(pcie_ep);
>> } else if (FIELD_GET(PARF_INT_ALL_PM_TURNOFF, status)) {
>> dev_dbg(dev, "Received PM Turn-off event! Entering L23\n");
>> val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] PCI: qcom-ep: Add ICC bandwidth voting support
2023-06-07 16:43 ` Bjorn Helgaas
2023-06-09 11:50 ` Krishna Chaitanya Chundru
@ 2023-06-09 11:52 ` Krishna Chaitanya Chundru
2023-06-09 16:58 ` Bjorn Helgaas
1 sibling, 1 reply; 14+ messages in thread
From: Krishna Chaitanya Chundru @ 2023-06-09 11:52 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: manivannan.sadhasivam, quic_vbadigan, quic_ramkri,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM, open list
On 6/7/2023 10:13 PM, Bjorn Helgaas wrote:
> On Wed, Jun 07, 2023 at 09:48:07PM +0530, Krishna chaitanya chundru wrote:
>> Add support to vote for ICC bandwidth based on the link
>> speed and width.
>>
>> This patch is inspired from pcie-qcom driver to add basic
>> interconnect support.
> Wrap to fill 75 columns like the rest of the git history.
>
>> Link:https://patchwork.kernel.org/project/linux-pci/patch/20221102090705.23634-3-johan+linaro@kernel.org/
> Add space after "Link:"
>
> Probably better to use a lore link
> (https://lore.kernel.org/r/20221102090705.23634-3-johan+linaro@kernel.org/)
> because I think lore is more general-purpose than patchwork and we
> don't get any benefit from the patchwork features here.
done
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>> drivers/pci/controller/dwc/pcie-qcom-ep.c | 68 +++++++++++++++++++++++++++++++
>> 1 file changed, 68 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> index 19b3283..5f9139d 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> @@ -13,6 +13,7 @@
>> #include <linux/debugfs.h>
>> #include <linux/delay.h>
>> #include <linux/gpio/consumer.h>
>> +#include <linux/interconnect.h>
>> #include <linux/mfd/syscon.h>
>> #include <linux/phy/pcie.h>
>> #include <linux/phy/phy.h>
>> @@ -28,6 +29,7 @@
>> #define PARF_SYS_CTRL 0x00
>> #define PARF_DB_CTRL 0x10
>> #define PARF_PM_CTRL 0x20
>> +#define PARF_PM_STTS 0x24
>> #define PARF_MHI_CLOCK_RESET_CTRL 0x174
>> #define PARF_MHI_BASE_ADDR_LOWER 0x178
>> #define PARF_MHI_BASE_ADDR_UPPER 0x17c
>> @@ -128,6 +130,9 @@
>> /* DBI register fields */
>> #define DBI_CON_STATUS_POWER_STATE_MASK GENMASK(1, 0)
>>
>> +#define DBI_LINKCTRLSTATUS 0x80
>> +#define DBI_LINKCTRLSTATUS_SHIFT 16
>> +
>> #define XMLH_LINK_UP 0x400
>> #define CORE_RESET_TIME_US_MIN 1000
>> #define CORE_RESET_TIME_US_MAX 1005
>> @@ -178,6 +183,8 @@ struct qcom_pcie_ep {
>> struct phy *phy;
>> struct dentry *debugfs;
>>
>> + struct icc_path *icc;
> Seems gratuitously different from the "icc_mem" name used in
> pcie-qcom. Use the same name unless there's a reason to be different.
done
>> struct clk_bulk_data *clks;
>> int num_clks;
>>
>> @@ -253,9 +260,51 @@ static void qcom_pcie_dw_stop_link(struct dw_pcie *pci)
>> disable_irq(pcie_ep->perst_irq);
>> }
>>
>> +static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
>> +{
>> + struct dw_pcie *pci = &pcie_ep->pci;
>> + int speed, width;
>> + u32 val, bw;
>> + int ret;
>> +
>> + if (!pcie_ep->icc)
>> + return;
>> +
>> + val = dw_pcie_readl_dbi(pci, DBI_LINKCTRLSTATUS);
>> + val = val >> DBI_LINKCTRLSTATUS_SHIFT;
>> +
>> + speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
>> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
> This whole function is basically identical to qcom_pcie_icc_update().
> Could a single implementation be shared? qcom_pcie_icc_update() uses
> dw_pcie_find_capability(pci, PCI_CAP_ID_EXP) instead of the hard-coded
> DBI_LINKCTRLSTATUS, but there are other instances of
> dw_pcie_find_capability() in pcie-qcom-ep.c, so it seems possible that
> the same code could be used both places.
done
>> + switch (speed) {
>> + case 1:
>> + bw = MBps_to_icc(250); /* BW for GEN1 per lane: 250MBps */
>> + break;
>> + case 2:
>> + bw = MBps_to_icc(500); /* BW for GEN2 per lane: 500MBps */
>> + break;
>> + case 3:
>> + bw = MBps_to_icc(985); /* BW for GEN3 per lane: 985MBps */
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> + fallthrough;
>> + case 4:
>> + bw = MBps_to_icc(1969); /* BW for GEN4 per lane: 985MBps */
>> + break;
>> + }
>> +
>> + ret = icc_set_bw(pcie_ep->icc, 0, width * bw);
>> + if (ret) {
>> + dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>> + ret);
>> + }
>> +}
>> +
>> static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
>> {
>> int ret;
>> + struct dw_pcie *pci = &pcie_ep->pci;
>>
>> ret = clk_bulk_prepare_enable(pcie_ep->num_clks, pcie_ep->clks);
>> if (ret)
>> @@ -277,6 +326,20 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
>> if (ret)
>> goto err_phy_exit;
>>
>> + /*
>> + * Some Qualcomm platforms require interconnect bandwidth constraints
>> + * to be set before enabling interconnect clocks.
>> + *
>> + * Set an initial average bandwidth corresponding to single-lane Gen 1
>> + * for the pcie to mem path.
>> + */
>> + ret = icc_set_bw(pcie_ep->icc, 0, MBps_to_icc(250)); /* BW for GEN1 per lane: 250MBps */
> Reformat the comment so this all fits in 80 columns like the rest of
> the file.
done
>> + if (ret) {
>> + dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>> + ret);
>> + goto err_phy_exit;
>> + }
> This plus the pcie_ep->icc init below is basically identical to
> qcom_pcie_icc_init() in pcie_qcom.c. Why not use the same structure
> here, with a qcom_pcie_icc_init() function? It's better to be the
> same than different (when possible, of course).
In pcie_qcom.c driver the resources will be turned on the probe itself.
But in pcie-qcom-ep.c driver will wait for the host to toggle the perst
to enable all the resources
that is reason it is different here.
- Krishna chaitanya.
>> return 0;
>>
>> err_phy_exit:
>> @@ -550,6 +613,10 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
>> if (IS_ERR(pcie_ep->phy))
>> ret = PTR_ERR(pcie_ep->phy);
>>
>> + pcie_ep->icc = devm_of_icc_get(dev, "pcie-mem");
>> + if (IS_ERR(pcie_ep->icc))
>> + ret = PTR_ERR(pcie_ep->icc);
>> +
>> return ret;
>> }
>>
>> @@ -572,6 +639,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data)
>> } else if (FIELD_GET(PARF_INT_ALL_BME, status)) {
>> dev_dbg(dev, "Received BME event. Link is enabled!\n");
>> pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
>> + qcom_pcie_ep_icc_update(pcie_ep);
>> } else if (FIELD_GET(PARF_INT_ALL_PM_TURNOFF, status)) {
>> dev_dbg(dev, "Received PM Turn-off event! Entering L23\n");
>> val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path
2023-06-07 17:21 ` Rob Herring
@ 2023-06-09 11:55 ` Krishna Chaitanya Chundru
2023-06-09 15:34 ` Krzysztof Kozlowski
0 siblings, 1 reply; 14+ messages in thread
From: Krishna Chaitanya Chundru @ 2023-06-09 11:55 UTC (permalink / raw)
To: Rob Herring
Cc: quic_vbadigan, Lorenzo Pieralisi, quic_ramkri, Konrad Dybcio,
Conor Dooley, devicetree, linux-kernel, Krzysztof Wilczyński,
linux-pci, Bjorn Helgaas, linux-arm-msm, Bjorn Andersson,
manivannan.sadhasivam, Krzysztof Kozlowski, Manivannan Sadhasivam,
Andy Gross
On 6/7/2023 10:51 PM, Rob Herring wrote:
> On Wed, 07 Jun 2023 21:48:05 +0530, Krishna chaitanya chundru wrote:
>> Add the "pcie-mem" interconnect path to the bindings.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>> Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb: pcie-ep@1c00000: 'interconnects' is a required property
> From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb: pcie-ep@1c00000: 'interconnect-names' is a required property
> From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1686154687-29356-2-git-send-email-quic_krichai@quicinc.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>
Fixed the errors.
- Krishna chaitanya
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path
2023-06-09 11:55 ` Krishna Chaitanya Chundru
@ 2023-06-09 15:34 ` Krzysztof Kozlowski
0 siblings, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-09 15:34 UTC (permalink / raw)
To: Krishna Chaitanya Chundru, Rob Herring
Cc: quic_vbadigan, Lorenzo Pieralisi, quic_ramkri, Konrad Dybcio,
Conor Dooley, devicetree, linux-kernel, Krzysztof Wilczyński,
linux-pci, Bjorn Helgaas, linux-arm-msm, Bjorn Andersson,
manivannan.sadhasivam, Krzysztof Kozlowski, Manivannan Sadhasivam,
Andy Gross
On 09/06/2023 13:55, Krishna Chaitanya Chundru wrote:
>>
>> Please check and re-submit after running the above command yourself. Note
>> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
>> your schema. However, it must be unset to test all examples with your schema.
>>
> Fixed the errors.
Then please test it before sending.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] PCI: qcom-ep: Add ICC bandwidth voting support
2023-06-09 11:52 ` Krishna Chaitanya Chundru
@ 2023-06-09 16:58 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2023-06-09 16:58 UTC (permalink / raw)
To: Krishna Chaitanya Chundru
Cc: manivannan.sadhasivam, quic_vbadigan, quic_ramkri,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM,
open list:PCIE ENDPOINT DRIVER FOR QUALCOMM, open list
On Fri, Jun 09, 2023 at 05:22:00PM +0530, Krishna Chaitanya Chundru wrote:
> On 6/7/2023 10:13 PM, Bjorn Helgaas wrote:
> > On Wed, Jun 07, 2023 at 09:48:07PM +0530, Krishna chaitanya chundru wrote:
> > > Add support to vote for ICC bandwidth based on the link
> > > speed and width.
> > This plus the pcie_ep->icc init below is basically identical to
> > qcom_pcie_icc_init() in pcie_qcom.c. Why not use the same structure
> > here, with a qcom_pcie_icc_init() function? It's better to be the
> > same than different (when possible, of course).
>
> In pcie_qcom.c driver the resources will be turned on the probe itself.
>
> But in pcie-qcom-ep.c driver will wait for the host to toggle the perst to
> enable all the resources
>
> that is reason it is different here.
Understood, differences like this do make sense. It's not always
possible, but when it is, it's helpful to use similar structure and
function names because it makes it easier to understand and easier to
see opportunities for refactoring.
Bjorn
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-06-09 16:58 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1686154687-29356-1-git-send-email-quic_krichai@quicinc.com>
2023-06-07 16:18 ` [PATCH v2 1/3] dt-bindings: PCI: qcom: ep: Add interconnects path Krishna chaitanya chundru
2023-06-07 17:21 ` Rob Herring
2023-06-09 11:55 ` Krishna Chaitanya Chundru
2023-06-09 15:34 ` Krzysztof Kozlowski
2023-06-08 15:27 ` Rob Herring
2023-06-08 15:52 ` Manivannan Sadhasivam
2023-06-07 16:18 ` [PATCH v2 2/3] arm: dts: qcom: sdx55: Add interconnect path Krishna chaitanya chundru
2023-06-08 16:00 ` Manivannan Sadhasivam
2023-06-09 11:09 ` kernel test robot
2023-06-07 16:18 ` [PATCH v2 3/3] PCI: qcom-ep: Add ICC bandwidth voting support Krishna chaitanya chundru
2023-06-07 16:43 ` Bjorn Helgaas
2023-06-09 11:50 ` Krishna Chaitanya Chundru
2023-06-09 11:52 ` Krishna Chaitanya Chundru
2023-06-09 16:58 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).