| Message ID | 1700577493-18538-2-git-send-email-quic_msarkar@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | arm64: qcom: sa8775p: add cache coherency support for SA8775P | expand |
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>
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 > >
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/
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/
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
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 },
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(-)