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@xxxxxxxxxxx/

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