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 quic_krichai@quicinc.com --- 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,
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: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru quic_krichai@quicinc.com --- 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)
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: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru quic_krichai@quicinc.com
--- bod
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 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 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 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: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru quic_krichai@quicinc.com
bod
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 bryan.odonoghue@linaro.org
On Fri, 12 Jan 2024 at 16:24, Krishna chaitanya chundru quic_krichai@quicinc.com 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: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru quic_krichai@quicinc.com
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:00 PM, Dmitry Baryshkov wrote:
On Fri, 12 Jan 2024 at 16:24, Krishna chaitanya chundru quic_krichai@quicinc.com 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: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru quic_krichai@quicinc.com
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 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 quic_krichai@quicinc.com 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: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru quic_krichai@quicinc.com
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/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 quic_krichai@quicinc.com 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: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru quic_krichai@quicinc.com
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: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: stable@vger.kernel.org
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);@@ -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
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: stable@vger.kernel.org
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);@@ -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 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: stable@vger.kernel.org
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
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: stable@vger.kernel.org
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);@@ -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 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: stable@vger.kernel.org
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
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: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru quic_krichai@quicinc.com
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);@@ -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);@@ -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: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: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru quic_krichai@quicinc.com
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);@@ -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);@@ -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
linux-stable-mirror@lists.linaro.org
-
Bjorn Helgaas -
Bryan O'Donoghue -
Dmitry Baryshkov -
Johan Hovold -
Konrad Dybcio -
Krishna Chaitanya Chundru -
Krishna chaitanya chundru
-
Manivannan Sadhasivam