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

[Mrinmay Sarkar <quic_msarkar@quicinc.com>]

On 2/24/2024 4:24 AM, Bjorn Helgaas wrote:
> On Fri, Feb 23, 2024 at 07:33:38PM +0530, Mrinmay Sarkar wrote:
>> 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.
> s/that there no/that no/
> s/exit/exist/
>
> Forgive my ignorance here.  It sounds like the cache coherency issue
> would refer to system memory, so the relevant No Snoop attribute would
> be in DMA transactions, i.e., Memory Reads or Writes initiated by PCIe
> Endpoints.  But it looks like this patch would affect TLPs initiated
> by the Root Complex, not those from Endpoints, so I'm confused about
> how this works.
>
> If this were in the qcom-ep driver, it would make sense that setting
> No Snoop in the TLPs initiated by the Endpoint could be a problem, but
> that doesn't seem to be what this patch is concerned with.
I think in multiprocessor system cache coherency issue might occur.
and RC as well needs to snoop cache to avoid coherency as it is not
enable by default.

and we are enabling this feature for qcom-ep driver as well.
it is in patch2.

Thanks
Mrinmay

>> 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.
>>
>> 8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for this
>> platform. Assign enable_cache_snoop 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.
> s/intruduce/introduce/
>
> Bjorn