[PATCH v4 0/3] arm64: qcom: sa8775p: add cache coherency support for SA8775P

[PATCH v4 0/3] arm64: qcom: sa8775p: add cache coherency support for SA8775P

[Mrinmay Sarkar]

In a multiprocessor system cache snooping maintains the consistency
of caches. Snooping logic is disabled from HW on this platform.
Cache coherency doesn’t work without enabling this logic.

This series is to enable cache snooping logic in both RC and EP
driver and add the "dma-coherent" property in dtsi to support
cache coherency in 8775 platform.

To verify this series we required [1]

[1] https://lore.kernel.org/all/1699669982-7691-1-git-send-email-quic_msarkar@quicinc.com/

v3 -> v4:
- added new cfg(cfg_1_34_0) for SA8775P in both RC and EP driver.
- populated a flag in the data structures instead of doing
  of_device_is_compatible() in both RC and EP patch.
- update commit mesaage and added reveiwed-by tag in commit message
  in dtsi patch.

v2 -> v3:
- update commit message(8755 -> 8775).

v1 -> v2:
- update cover letter with explanation.
- define each of these bits and ORing at usage time rather than
  directly writing value in register.

Mrinmay Sarkar (3):
  PCI: qcom: Enable cache coherency for SA8775P RC platform
  PCI: qcom-ep: Enable cache coherency for SA8775P EP
  arm64: dts: qcom: sa8775p: Mark PCIe EP controller as cache coherent

 arch/arm64/boot/dts/qcom/sa8775p.dtsi     |  1 +
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 23 ++++++++++++++++++++++-
 drivers/pci/controller/dwc/pcie-qcom.c    | 20 +++++++++++++++++++-
 3 files changed, 42 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v4 1/3] PCI: qcom: Enable cache coherency for SA8775P RC

[Mrinmay Sarkar]

In a multiprocessor system cache snooping maintains the consistency
of caches. Snooping logic is disabled from HW on this platform.
Cache coherency doesn’t work without enabling this logic.

8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for this
platform. Assign no_snoop_override flag into struct qcom_pcie_cfg and
set it true in cfg_1_34_0 and enable cache snooping if this particular
flag is true.

Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6902e97..76f03fc 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -51,6 +51,7 @@
 #define PARF_SID_OFFSET				0x234
 #define PARF_BDF_TRANSLATE_CFG			0x24c
 #define PARF_SLV_ADDR_SPACE_SIZE		0x358
+#define PCIE_PARF_NO_SNOOP_OVERIDE		0x3d4
 #define PARF_DEVICE_TYPE			0x1000
 #define PARF_BDF_TO_SID_TABLE_N			0x2000
 
@@ -117,6 +118,10 @@
 /* PARF_LTSSM register fields */
 #define LTSSM_EN				BIT(8)
 
+/* PARF_NO_SNOOP_OVERIDE register fields */
+#define WR_NO_SNOOP_OVERIDE_EN			BIT(1)
+#define RD_NO_SNOOP_OVERIDE_EN			BIT(3)
+
 /* PARF_DEVICE_TYPE register fields */
 #define DEVICE_TYPE_RC				0x4
 
@@ -229,6 +234,7 @@ struct qcom_pcie_ops {
 
 struct qcom_pcie_cfg {
 	const struct qcom_pcie_ops *ops;
+	bool no_snoop_overide;
 };
 
 struct qcom_pcie {
@@ -961,6 +967,13 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 
 static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
 {
+	const struct qcom_pcie_cfg *pcie_cfg = pcie->cfg;
+
+	/* Enable cache snooping for SA8775P */
+	if (pcie_cfg->no_snoop_overide)
+		writel(WR_NO_SNOOP_OVERIDE_EN | RD_NO_SNOOP_OVERIDE_EN,
+				pcie->parf + PCIE_PARF_NO_SNOOP_OVERIDE);
+
 	qcom_pcie_clear_hpc(pcie->pci);
 
 	return 0;
@@ -1331,6 +1344,11 @@ static const struct qcom_pcie_cfg cfg_1_9_0 = {
 	.ops = &ops_1_9_0,
 };
 
+static const struct qcom_pcie_cfg cfg_1_34_0 = {
+	.ops = &ops_1_9_0,
+	.no_snoop_overide = true,
+};
+
 static const struct qcom_pcie_cfg cfg_2_1_0 = {
 	.ops = &ops_2_1_0,
 };
@@ -1627,7 +1645,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
 	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
 	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
-	{ .compatible = "qcom,pcie-sa8775p", .data = &cfg_1_9_0},
+	{ .compatible = "qcom,pcie-sa8775p", .data = &cfg_1_34_0},
 	{ .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
 	{ .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 },
 	{ .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 },
-- 
2.7.4

[PATCH v4 2/3] PCI: qcom-ep: Enable cache coherency for SA8775P EP

[Mrinmay Sarkar]

In a multiprocessor system cache snooping maintains the consistency
of caches. Snooping logic is disabled from HW on this platform.
Cache coherency doesn’t work without enabling this logic.

8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for
this platform. Add struct qcom_pcie_ep_cfg as match data. Assign
no_snoop_override flag into struct qcom_pcie_ep_cfg and set it true
in cfg_1_34_0 and enable cache snooping if this particular flag is
true.

Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 3a53d97..2cf9a24 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -47,6 +47,7 @@
 #define PARF_DBI_BASE_ADDR_HI			0x354
 #define PARF_SLV_ADDR_SPACE_SIZE		0x358
 #define PARF_SLV_ADDR_SPACE_SIZE_HI		0x35c
+#define PCIE_PARF_NO_SNOOP_OVERIDE		0x3d4
 #define PARF_ATU_BASE_ADDR			0x634
 #define PARF_ATU_BASE_ADDR_HI			0x638
 #define PARF_SRIS_MODE				0x644
@@ -86,6 +87,10 @@
 #define PARF_DEBUG_INT_CFG_BUS_MASTER_EN	BIT(2)
 #define PARF_DEBUG_INT_RADM_PM_TURNOFF		BIT(3)
 
+/* PARF_NO_SNOOP_OVERIDE register fields */
+#define WR_NO_SNOOP_OVERIDE_EN                 BIT(1)
+#define RD_NO_SNOOP_OVERIDE_EN                 BIT(3)
+
 /* PARF_DEVICE_TYPE register fields */
 #define PARF_DEVICE_TYPE_EP			0x0
 
@@ -149,6 +154,10 @@ enum qcom_pcie_ep_link_status {
 	QCOM_PCIE_EP_LINK_DOWN,
 };
 
+struct qcom_pcie_ep_cfg {
+	bool no_snoop_overide;
+};
+
 /**
  * struct qcom_pcie_ep - Qualcomm PCIe Endpoint Controller
  * @pci: Designware PCIe controller struct
@@ -167,6 +176,7 @@ enum qcom_pcie_ep_link_status {
  * @num_clks: PCIe clocks count
  * @perst_en: Flag for PERST enable
  * @perst_sep_en: Flag for PERST separation enable
+ * @cfg: PCIe EP config struct
  * @link_status: PCIe Link status
  * @global_irq: Qualcomm PCIe specific Global IRQ
  * @perst_irq: PERST# IRQ
@@ -194,6 +204,7 @@ struct qcom_pcie_ep {
 	u32 perst_en;
 	u32 perst_sep_en;
 
+	const struct qcom_pcie_ep_cfg *cfg;
 	enum qcom_pcie_ep_link_status link_status;
 	int global_irq;
 	int perst_irq;
@@ -489,6 +500,11 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 	val |= BIT(8);
 	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
 
+	/* Enable cache snooping for SA8775P */
+	if (pcie_ep->cfg && pcie_ep->cfg->no_snoop_overide)
+		writel_relaxed(WR_NO_SNOOP_OVERIDE_EN | RD_NO_SNOOP_OVERIDE_EN,
+				pcie_ep->parf + PCIE_PARF_NO_SNOOP_OVERIDE);
+
 	return 0;
 
 err_disable_resources:
@@ -511,6 +527,10 @@ static void qcom_pcie_perst_assert(struct dw_pcie *pci)
 	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
 }
 
+static const struct qcom_pcie_ep_cfg cfg_1_34_0 = {
+	.no_snoop_overide = true,
+};
+
 /* Common DWC controller ops */
 static const struct dw_pcie_ops pci_ops = {
 	.link_up = qcom_pcie_dw_link_up,
@@ -817,6 +837,7 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
 	pcie_ep->pci.ep.ops = &pci_ep_ops;
 	pcie_ep->pci.edma.nr_irqs = 1;
 	platform_set_drvdata(pdev, pcie_ep);
+	pcie_ep->cfg = of_device_get_match_data(dev);
 
 	ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
 	if (ret)
@@ -875,7 +896,7 @@ static void qcom_pcie_ep_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_pcie_ep_match[] = {
-	{ .compatible = "qcom,sa8775p-pcie-ep", },
+	{ .compatible = "qcom,sa8775p-pcie-ep", .data = &cfg_1_34_0},
 	{ .compatible = "qcom,sdx55-pcie-ep", },
 	{ .compatible = "qcom,sm8450-pcie-ep", },
 	{ }
-- 
2.7.4

[PATCH v4 3/3] arm64: dts: qcom: sa8775p: Mark PCIe EP controller as cache coherent

[Mrinmay Sarkar]

The PCIe EP controller on SA8775P supports cache coherency, hence add
the "dma-coherent" property to mark it as such.

Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 7eab458..ab01efe 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -3620,6 +3620,7 @@
 				<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_PCIE_0 0>;
 		interconnect-names = "pcie-mem", "cpu-pcie";
 
+		dma-coherent;
 		iommus = <&pcie_smmu 0x0000 0x7f>;
 		resets = <&gcc GCC_PCIE_0_BCR>;
 		reset-names = "core";
-- 
2.7.4

Re: [PATCH v4 3/3] arm64: dts: qcom: sa8775p: Mark PCIe EP controller as cache coherent

[Johan Hovold]

On Tue, Nov 21, 2023 at 08:08:13PM +0530, Mrinmay Sarkar wrote:
> The PCIe EP controller on SA8775P supports cache coherency, hence add
> the "dma-coherent" property to mark it as such.
> 
> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> index 7eab458..ab01efe 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> @@ -3620,6 +3620,7 @@
>  				<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_PCIE_0 0>;
>  		interconnect-names = "pcie-mem", "cpu-pcie";
>  
> +		dma-coherent;
>  		iommus = <&pcie_smmu 0x0000 0x7f>;
>  		resets = <&gcc GCC_PCIE_0_BCR>;
>  		reset-names = "core";

What tree is this against? 

Both controllers are already marked as dma-coherent in mainline so this
patch makes no sense (and the context also looks wrong).

It was even you added them apparently:

	489f14be0e0a ("arm64: dts: qcom: sa8775p: Add pcie0 and pcie1 nodes")

Johan

Re: [PATCH v4 3/3] arm64: dts: qcom: sa8775p: Mark PCIe EP controller as cache coherent

[Mrinmay Sarkar]

On 11/21/2023 8:25 PM, Johan Hovold wrote:
> On Tue, Nov 21, 2023 at 08:08:13PM +0530, Mrinmay Sarkar wrote:
>> The PCIe EP controller on SA8775P supports cache coherency, hence add
>> the "dma-coherent" property to mark it as such.
>>
>> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> ---
>>   arch/arm64/boot/dts/qcom/sa8775p.dtsi | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> index 7eab458..ab01efe 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> @@ -3620,6 +3620,7 @@
>>   				<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_PCIE_0 0>;
>>   		interconnect-names = "pcie-mem", "cpu-pcie";
>>   
>> +		dma-coherent;
>>   		iommus = <&pcie_smmu 0x0000 0x7f>;
>>   		resets = <&gcc GCC_PCIE_0_BCR>;
>>   		reset-names = "core";
> What tree is this against?
>
> Both controllers are already marked as dma-coherent in mainline so this
> patch makes no sense (and the context also looks wrong).
>
> It was even you added them apparently:
>
> 	489f14be0e0a ("arm64: dts: qcom: sa8775p: Add pcie0 and pcie1 nodes")
>
> Johan
Hi Johan,

Yes both the RC controllers are dma-coherent and this change is for
PCIe EP controller and it is inside pcie0_ep node.
Actually the pcie0_ep node change is yet to apply on linux next.
I just made this change on top of that and the same I mentioned in
cover letter.

https://lore.kernel.org/all/1699669982-7691-6-git-send-email-quic_msarkar@quicinc.com/

Thanks,
Mrinmay

Re: [PATCH v4 3/3] arm64: dts: qcom: sa8775p: Mark PCIe EP controller as cache coherent

[Johan Hovold]

On Tue, Nov 21, 2023 at 09:02:41PM +0530, Mrinmay Sarkar wrote:
> 
> On 11/21/2023 8:25 PM, Johan Hovold wrote:
> > On Tue, Nov 21, 2023 at 08:08:13PM +0530, Mrinmay Sarkar wrote:
> >> The PCIe EP controller on SA8775P supports cache coherency, hence add
> >> the "dma-coherent" property to mark it as such.

> > What tree is this against?
> >
> > Both controllers are already marked as dma-coherent in mainline so this
> > patch makes no sense (and the context also looks wrong).

> Yes both the RC controllers are dma-coherent and this change is for
> PCIe EP controller and it is inside pcie0_ep node.
> Actually the pcie0_ep node change is yet to apply on linux next.
> I just made this change on top of that and the same I mentioned in
> cover letter.

Ah, thanks for clarifying, and sorry for the noise.

Johan

Re: [PATCH v4 1/3] PCI: qcom: Enable cache coherency for SA8775P RC

[Dmitry Baryshkov]

On Tue, 21 Nov 2023 at 16:38, Mrinmay Sarkar <quic_msarkar@quicinc.com> wrote:
>
> In a multiprocessor system cache snooping maintains the consistency
> of caches. Snooping logic is disabled from HW on this platform.
> Cache coherency doesn’t work without enabling this logic.
>
> 8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for this
> platform. Assign no_snoop_override flag into struct qcom_pcie_cfg and
> set it true in cfg_1_34_0 and enable cache snooping if this particular
> flag is true.

Thank you!

>
> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>



-- 
With best wishes
Dmitry

Re: [PATCH v4 0/3] arm64: qcom: sa8775p: add cache coherency support for SA8775P

[Konrad Dybcio]

On 11/21/23 15:38, Mrinmay Sarkar wrote:
> In a multiprocessor system cache snooping maintains the consistency
> of caches. Snooping logic is disabled from HW on this platform.
> Cache coherency doesn’t work without enabling this logic.
> 
> This series is to enable cache snooping logic in both RC and EP
> driver and add the "dma-coherent" property in dtsi to support
> cache coherency in 8775 platform.
> 
> To verify this series we required [1]
> 
> [1] https://lore.kernel.org/all/1699669982-7691-1-git-send-email-quic_msarkar@quicinc.com/
> 
> v3 -> v4:
> - added new cfg(cfg_1_34_0) for SA8775P in both RC and EP driver.
> - populated a flag in the data structures instead of doing
>    of_device_is_compatible() in both RC and EP patch.
> - update commit mesaage and added reveiwed-by tag in commit message
>    in dtsi patch.
This series looks good now, thanks!

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v4 1/3] PCI: qcom: Enable cache coherency for SA8775P RC

[Manivannan Sadhasivam]

On Tue, Nov 21, 2023 at 08:08:11PM +0530, Mrinmay Sarkar wrote:
> In a multiprocessor system cache snooping maintains the consistency
> of caches. Snooping logic is disabled from HW on this platform.
> Cache coherency doesn’t work without enabling this logic.
> 
> 8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for this
> platform. Assign no_snoop_override flag into struct qcom_pcie_cfg and
> set it true in cfg_1_34_0 and enable cache snooping if this particular
> flag is true.
> 

I just happen to check the internal register details of other platforms and I
see this PCIE_PARF_NO_SNOOP_OVERIDE register with the reset value of 0x0. So
going by the logic of this patch, this register needs to be configured for other
platforms as well to enable cache coherency, but it seems like not the case as
we never did and all are working fine (so far no issues reported).

So this gives me an impression that this patch is wrong or needs modification.
So,

Nacked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6902e97..76f03fc 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -51,6 +51,7 @@
>  #define PARF_SID_OFFSET				0x234
>  #define PARF_BDF_TRANSLATE_CFG			0x24c
>  #define PARF_SLV_ADDR_SPACE_SIZE		0x358
> +#define PCIE_PARF_NO_SNOOP_OVERIDE		0x3d4
>  #define PARF_DEVICE_TYPE			0x1000
>  #define PARF_BDF_TO_SID_TABLE_N			0x2000
>  
> @@ -117,6 +118,10 @@
>  /* PARF_LTSSM register fields */
>  #define LTSSM_EN				BIT(8)
>  
> +/* PARF_NO_SNOOP_OVERIDE register fields */
> +#define WR_NO_SNOOP_OVERIDE_EN			BIT(1)
> +#define RD_NO_SNOOP_OVERIDE_EN			BIT(3)
> +
>  /* PARF_DEVICE_TYPE register fields */
>  #define DEVICE_TYPE_RC				0x4
>  
> @@ -229,6 +234,7 @@ struct qcom_pcie_ops {
>  
>  struct qcom_pcie_cfg {
>  	const struct qcom_pcie_ops *ops;
> +	bool no_snoop_overide;

I'd suggest to name variables after their usecase and not the register. Like,

bool enable_cache_snoop;

>  };
>  
>  struct qcom_pcie {
> @@ -961,6 +967,13 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  
>  static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  {
> +	const struct qcom_pcie_cfg *pcie_cfg = pcie->cfg;
> +
> +	/* Enable cache snooping for SA8775P */

This comment doesn't belong here. It can be added while setting the flag in cfg.

> +	if (pcie_cfg->no_snoop_overide)
> +		writel(WR_NO_SNOOP_OVERIDE_EN | RD_NO_SNOOP_OVERIDE_EN,
> +				pcie->parf + PCIE_PARF_NO_SNOOP_OVERIDE);
> +
>  	qcom_pcie_clear_hpc(pcie->pci);
>  
>  	return 0;
> @@ -1331,6 +1344,11 @@ static const struct qcom_pcie_cfg cfg_1_9_0 = {
>  	.ops = &ops_1_9_0,
>  };
>  
> +static const struct qcom_pcie_cfg cfg_1_34_0 = {
> +	.ops = &ops_1_9_0,
> +	.no_snoop_overide = true,
> +};
> +
>  static const struct qcom_pcie_cfg cfg_2_1_0 = {
>  	.ops = &ops_2_1_0,
>  };
> @@ -1627,7 +1645,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>  	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
>  	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> -	{ .compatible = "qcom,pcie-sa8775p", .data = &cfg_1_9_0},
> +	{ .compatible = "qcom,pcie-sa8775p", .data = &cfg_1_34_0},
>  	{ .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
>  	{ .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 },
>  	{ .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 },
> -- 
> 2.7.4
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 1/3] PCI: qcom: Enable cache coherency for SA8775P RC

[Konrad Dybcio]

On 30.11.2023 06:21, Manivannan Sadhasivam wrote:
> On Tue, Nov 21, 2023 at 08:08:11PM +0530, Mrinmay Sarkar wrote:
>> In a multiprocessor system cache snooping maintains the consistency
>> of caches. Snooping logic is disabled from HW on this platform.
>> Cache coherency doesn’t work without enabling this logic.
>>
>> 8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for this
>> platform. Assign no_snoop_override flag into struct qcom_pcie_cfg and
>> set it true in cfg_1_34_0 and enable cache snooping if this particular
>> flag is true.
>>
> 
> I just happen to check the internal register details of other platforms and I
> see this PCIE_PARF_NO_SNOOP_OVERIDE register with the reset value of 0x0. So
> going by the logic of this patch, this register needs to be configured for other
> platforms as well to enable cache coherency, but it seems like not the case as
> we never did and all are working fine (so far no issues reported).

Guess we know that already [1]

The question is whether this override is necessary, or the default
internal state is OK on other platforms

Konrad

[1] https://lore.kernel.org/linux-arm-msm/cb4324aa-8035-ce6e-94ef-a31ed070225c@quicinc.com/

Re: [PATCH v4 1/3] PCI: qcom: Enable cache coherency for SA8775P RC

[Manivannan Sadhasivam]

On Thu, Nov 30, 2023 at 11:09:59AM +0100, Konrad Dybcio wrote:
> On 30.11.2023 06:21, Manivannan Sadhasivam wrote:
> > On Tue, Nov 21, 2023 at 08:08:11PM +0530, Mrinmay Sarkar wrote:
> >> In a multiprocessor system cache snooping maintains the consistency
> >> of caches. Snooping logic is disabled from HW on this platform.
> >> Cache coherency doesn’t work without enabling this logic.
> >>
> >> 8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for this
> >> platform. Assign no_snoop_override flag into struct qcom_pcie_cfg and
> >> set it true in cfg_1_34_0 and enable cache snooping if this particular
> >> flag is true.
> >>
> > 
> > I just happen to check the internal register details of other platforms and I
> > see this PCIE_PARF_NO_SNOOP_OVERIDE register with the reset value of 0x0. So
> > going by the logic of this patch, this register needs to be configured for other
> > platforms as well to enable cache coherency, but it seems like not the case as
> > we never did and all are working fine (so far no issues reported).
> 
> Guess we know that already [1]
> 

Bummer! I didn't look close into that reply :/

> The question is whether this override is necessary, or the default
> internal state is OK on other platforms
> 

I digged into it further...

The register description says "Enable this bit x to override no_snoop". So
NO_SNOOP is the default behavior unless bit x is set in this register.

This means if bit x is set, MRd and MWd TLPs originating from the desired PCIe
controller (Requester) will have the NO_SNOOP bit set in the header. So the
completer will not do any cache management for the transaction. But this also
requires that the address referenced by the TLP is not cacheable.

My guess here is that, hw designers have enabled the NO_SNOOP logic by default
and running into coherency issues on the completer side. Maybe due to the
addresses are cacheable always (?).

And the default value of this register has no impact on the NO_SNOOP attribute
unless specific bits are set.

But I need to confirm my above observations with HW team. Until then, I will
hold on to my Nack.

- Mani

> Konrad
> 
> [1] https://lore.kernel.org/linux-arm-msm/cb4324aa-8035-ce6e-94ef-a31ed070225c@quicinc.com/

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 1/3] PCI: qcom: Enable cache coherency for SA8775P RC\

[Manivannan Sadhasivam]

On Thu, Nov 30, 2023 at 04:39:09PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Nov 30, 2023 at 11:09:59AM +0100, Konrad Dybcio wrote:
> > On 30.11.2023 06:21, Manivannan Sadhasivam wrote:
> > > On Tue, Nov 21, 2023 at 08:08:11PM +0530, Mrinmay Sarkar wrote:
> > >> In a multiprocessor system cache snooping maintains the consistency
> > >> of caches. Snooping logic is disabled from HW on this platform.
> > >> Cache coherency doesn’t work without enabling this logic.
> > >>
> > >> 8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for this
> > >> platform. Assign no_snoop_override flag into struct qcom_pcie_cfg and
> > >> set it true in cfg_1_34_0 and enable cache snooping if this particular
> > >> flag is true.
> > >>
> > > 
> > > I just happen to check the internal register details of other platforms and I
> > > see this PCIE_PARF_NO_SNOOP_OVERIDE register with the reset value of 0x0. So
> > > going by the logic of this patch, this register needs to be configured for other
> > > platforms as well to enable cache coherency, but it seems like not the case as
> > > we never did and all are working fine (so far no issues reported).
> > 
> > Guess we know that already [1]
> > 
> 
> Bummer! I didn't look close into that reply :/
> 
> > The question is whether this override is necessary, or the default
> > internal state is OK on other platforms
> > 
> 
> I digged into it further...
> 
> The register description says "Enable this bit x to override no_snoop". So
> NO_SNOOP is the default behavior unless bit x is set in this register.
> 
> This means if bit x is set, MRd and MWd TLPs originating from the desired PCIe
> controller (Requester) will have the NO_SNOOP bit set in the header. So the
> completer will not do any cache management for the transaction. But this also
> requires that the address referenced by the TLP is not cacheable.
> 
> My guess here is that, hw designers have enabled the NO_SNOOP logic by default
> and running into coherency issues on the completer side. Maybe due to the
> addresses are cacheable always (?).
> 
> And the default value of this register has no impact on the NO_SNOOP attribute
> unless specific bits are set.
> 
> But I need to confirm my above observations with HW team. Until then, I will
> hold on to my Nack.
> 

I had some discussions with the hardware folks and clarified my concerns with
them. Here is the summary:

Due to some hardware changes, SA8775P has set the NO_SNOOP attribute in its TLP
for all the PCIe controllers. NO_SNOOP attribute when set, the requester is
indicating that there no cache coherency issues exit for the addressed memory
on the host i.e., memory is not cached. But in reality, requester cannot assume
this unless there is a complete control/visibility over the addressed memory on
the host.

And worst case, if the memory is cached on the host, it may lead to memory
corruption issues. It should be noted that the caching of memory on the host is
not solely dependent on the NO_SNOOP attribute in TLP.

So to avoid the corruption, this patch overrides the NO_SNOOP attribute by
setting the PCIE_PARF_NO_SNOOP_OVERIDE register. This patch is not needed for
other upstream supported platforms since they do not set NO_SNOOP attribute by
default.

Mrinmay, please add above information in the commit message while sending v2.
I'm taking by NACK back.

- Mani

-- 
மணிவண்ணன் சதாசிவம்