arm64: dts: qcom: sc7180: Mark SCM as dma-coherent for trogdor

  Trogdor devices use firmware backed by TF-A instead of Qualcomm's
normal TZ. On TF-A we end up mapping memory as cachable. Specifically,
you can see in Trogdor's TF-A code [1] in qti_sip_mem_assign() that we
call qti_mmap_add_dynamic_region() with MT_RO_DATA. This translates
down to MT_MEMORY instead of MT_NON_CACHEABLE or MT_DEVICE. Apparently
Qualcomm's normal TZ implementation maps the memory as non-cachable.

Let's add the "dma-coherent" attribute to the SCM for trogdor.

Adding "dma-coherent" like this fixes WiFi on sc7180-trogdor
devices. WiFi was broken as of commit 7bd6680b47fa ("Revert "Revert
"arm64: dma: Drop cache invalidation from
arch_dma_prep_coherent()"""). Specifically at bootup we'd get:

 qcom_scm firmware:scm: Assign memory protection call failed -22
 qcom_rmtfs_mem 94600000.memory: assign memory failed
 qcom_rmtfs_mem: probe of 94600000.memory failed with error -22

From discussion on the mailing lists [2] and over IRC [3], it was
determined that we should always have been tagging the SCM as
dma-coherent on trogdor but that the old "invalidate" happened to make
things work most of the time. Tagging it properly like this is a much
more robust solution.

[1] https://chromium.googlesource.com/chromiumos/third_party/arm-trusted-firmware/+/refs/heads/firmware-trogdor-13577.B/plat/qti/common/src/qti_syscall.c
[2] https://lore.kernel.org/r/20230614165904.1.I279773c37e2c1ed8fbb622ca6d1397aea0023526@changeid
[3] https://oftc.irclog.whitequark.org/linux-msm/2023-06-15

Fixes: 7bd6680b47fa ("Revert "Revert "arm64: dma: Drop cache invalidation from arch_dma_prep_coherent()""")
Fixes: 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial trogdor and lazor dt")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 4 ++++
 arch/arm64/boot/dts/qcom/sc7180.dtsi         | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)
  

Hi,

On Thu, Jun 15, 2023 at 2:59 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Trogdor devices use firmware backed by TF-A instead of Qualcomm's
> normal TZ. On TF-A we end up mapping memory as cachable. Specifically,
> you can see in Trogdor's TF-A code [1] in qti_sip_mem_assign() that we
> call qti_mmap_add_dynamic_region() with MT_RO_DATA. This translates
> down to MT_MEMORY instead of MT_NON_CACHEABLE or MT_DEVICE. Apparently
> Qualcomm's normal TZ implementation maps the memory as non-cachable.
>
> Let's add the "dma-coherent" attribute to the SCM for trogdor.
>
> Adding "dma-coherent" like this fixes WiFi on sc7180-trogdor
> devices. WiFi was broken as of commit 7bd6680b47fa ("Revert "Revert
> "arm64: dma: Drop cache invalidation from
> arch_dma_prep_coherent()"""). Specifically at bootup we'd get:
>
>  qcom_scm firmware:scm: Assign memory protection call failed -22
>  qcom_rmtfs_mem 94600000.memory: assign memory failed
>  qcom_rmtfs_mem: probe of 94600000.memory failed with error -22
>
> From discussion on the mailing lists [2] and over IRC [3], it was
> determined that we should always have been tagging the SCM as
> dma-coherent on trogdor but that the old "invalidate" happened to make
> things work most of the time. Tagging it properly like this is a much
> more robust solution.
>
> [1] https://chromium.googlesource.com/chromiumos/third_party/arm-trusted-firmware/+/refs/heads/firmware-trogdor-13577.B/plat/qti/common/src/qti_syscall.c
> [2] https://lore.kernel.org/r/20230614165904.1.I279773c37e2c1ed8fbb622ca6d1397aea0023526@changeid
> [3] https://oftc.irclog.whitequark.org/linux-msm/2023-06-15
>
> Fixes: 7bd6680b47fa ("Revert "Revert "arm64: dma: Drop cache invalidation from arch_dma_prep_coherent()""")
> Fixes: 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial trogdor and lazor dt")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 4 ++++
>  arch/arm64/boot/dts/qcom/sc7180.dtsi         | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)

Shoot. I just realized I probably need a bindings update too. If this
looks good other than that, I'll post a v2 tomorrow.

-Doug
  

On 15.06.2023 23:52, Douglas Anderson wrote:
> Trogdor devices use firmware backed by TF-A instead of Qualcomm's
> normal TZ. On TF-A we end up mapping memory as cachable. Specifically,
> you can see in Trogdor's TF-A code [1] in qti_sip_mem_assign() that we
> call qti_mmap_add_dynamic_region() with MT_RO_DATA. This translates
> down to MT_MEMORY instead of MT_NON_CACHEABLE or MT_DEVICE. Apparently
> Qualcomm's normal TZ implementation maps the memory as non-cachable.
> 
> Let's add the "dma-coherent" attribute to the SCM for trogdor.
> 
> Adding "dma-coherent" like this fixes WiFi on sc7180-trogdor
> devices. WiFi was broken as of commit 7bd6680b47fa ("Revert "Revert
> "arm64: dma: Drop cache invalidation from
> arch_dma_prep_coherent()"""). Specifically at bootup we'd get:
> 
>  qcom_scm firmware:scm: Assign memory protection call failed -22
>  qcom_rmtfs_mem 94600000.memory: assign memory failed
>  qcom_rmtfs_mem: probe of 94600000.memory failed with error -22
> 
> From discussion on the mailing lists [2] and over IRC [3], it was
> determined that we should always have been tagging the SCM as
> dma-coherent on trogdor but that the old "invalidate" happened to make
> things work most of the time. Tagging it properly like this is a much
> more robust solution.
> 
> [1] https://chromium.googlesource.com/chromiumos/third_party/arm-trusted-firmware/+/refs/heads/firmware-trogdor-13577.B/plat/qti/common/src/qti_syscall.c
> [2] https://lore.kernel.org/r/20230614165904.1.I279773c37e2c1ed8fbb622ca6d1397aea0023526@changeid
> [3] https://oftc.irclog.whitequark.org/linux-msm/2023-06-15
> 
> Fixes: 7bd6680b47fa ("Revert "Revert "arm64: dma: Drop cache invalidation from arch_dma_prep_coherent()""")
> Fixes: 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial trogdor and lazor dt")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
Please also add a comment in the dt

With that:

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

Konrad
> 
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 4 ++++
>  arch/arm64/boot/dts/qcom/sc7180.dtsi         | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index ca6920de7ea8..5e1e7c5bd474 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -1194,6 +1194,10 @@ &pm6150l_gpios {
>  			  "";
>  };
>  
> +&scm {
> +	dma-coherent;
> +};
> +
>  &tlmm {
>  	/*
>  	 * pinctrl settings for pins that have no real owners.
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index f479cab8ab45..a65be760d1a7 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -369,7 +369,7 @@ CLUSTER_SLEEP_0: cluster-sleep-0 {
>  	};
>  
>  	firmware {
> -		scm {
> +		scm: scm {
>  			compatible = "qcom,scm-sc7180", "qcom,scm";
>  		};
>  	};
  

On Thu, Jun 15, 2023 at 02:52:54PM -0700, Douglas Anderson wrote:
> Trogdor devices use firmware backed by TF-A instead of Qualcomm's
> normal TZ. On TF-A we end up mapping memory as cachable. Specifically,
> you can see in Trogdor's TF-A code [1] in qti_sip_mem_assign() that we
> call qti_mmap_add_dynamic_region() with MT_RO_DATA. This translates
> down to MT_MEMORY instead of MT_NON_CACHEABLE or MT_DEVICE.
>
> **Apparently Qualcomm's normal TZ implementation maps the memory as
> non-cachable.**

Are you sure about this? From the discussion in the chat the conclusion
was that we can check easily for TF-A, but we have absolutely no idea
what Qualcomm's firmware implementation does. It might be "broken" the
same way and we just have not noticed it yet.

I would perhaps just omit this sentence so we don't risk misleading
someone with information we're not sure about. :)

> 
> Let's add the "dma-coherent" attribute to the SCM for trogdor.
> 

What about sc7280? I guess they use largely the same TF-A firmware?

Thanks,
Stephan
  

On 16.06.2023 09:10, Stephan Gerhold wrote:
> On Thu, Jun 15, 2023 at 02:52:54PM -0700, Douglas Anderson wrote:
>> Trogdor devices use firmware backed by TF-A instead of Qualcomm's
>> normal TZ. On TF-A we end up mapping memory as cachable. Specifically,
>> you can see in Trogdor's TF-A code [1] in qti_sip_mem_assign() that we
>> call qti_mmap_add_dynamic_region() with MT_RO_DATA. This translates
>> down to MT_MEMORY instead of MT_NON_CACHEABLE or MT_DEVICE.
>>
>> **Apparently Qualcomm's normal TZ implementation maps the memory as
>> non-cachable.**
> 
> Are you sure about this? From the discussion in the chat the conclusion
> was that we can check easily for TF-A, but we have absolutely no idea
> what Qualcomm's firmware implementation does. It might be "broken" the
> same way and we just have not noticed it yet.
Nikita (+CC) was able to boot Venus (which uses that memory reservation
call, I believe) on next-20230615 with a WP firmware, so it should be okay..

Konrad
> 
> I would perhaps just omit this sentence so we don't risk misleading
> someone with information we're not sure about. :)
> 
>>
>> Let's add the "dma-coherent" attribute to the SCM for trogdor.
>>
> 
> What about sc7280? I guess they use largely the same TF-A firmware?
> 
> Thanks,
> Stephan
  

On Fri, Jun 16, 2023 at 01:20:09PM +0200, Konrad Dybcio wrote:
> On 16.06.2023 09:10, Stephan Gerhold wrote:
> > On Thu, Jun 15, 2023 at 02:52:54PM -0700, Douglas Anderson wrote:
> >> Trogdor devices use firmware backed by TF-A instead of Qualcomm's
> >> normal TZ. On TF-A we end up mapping memory as cachable. Specifically,
> >> you can see in Trogdor's TF-A code [1] in qti_sip_mem_assign() that we
> >> call qti_mmap_add_dynamic_region() with MT_RO_DATA. This translates
> >> down to MT_MEMORY instead of MT_NON_CACHEABLE or MT_DEVICE.
> >>
> >> **Apparently Qualcomm's normal TZ implementation maps the memory as
> >> non-cachable.**
> > 
> > Are you sure about this? From the discussion in the chat the conclusion
> > was that we can check easily for TF-A, but we have absolutely no idea
> > what Qualcomm's firmware implementation does. It might be "broken" the
> > same way and we just have not noticed it yet.
> Nikita (+CC) was able to boot Venus (which uses that memory reservation
> call, I believe) on next-20230615 with a WP firmware, so it should be okay..

Unfortunately we cannot draw any conclusions from a working case. Doug
mentioned this happens only with CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y [1].
But even with that in theory there could be side effects that invalidate
or evict the cache line, making it look like it's described correctly
but it will just explode at some later point.

It's more reliable to draw conclusions from a failing case: I asked
Nikita to test with "dma-coherent" added to SCM on the WoA firmware.
This fails with similar errors Doug had without the property ("Assign
memory protection call failed -22"). Clearly the firmware did not read
the proper values were just written into the cache. So it's indeed
reasonable to assume that Qcom's implementation maps the memory as
non-cacheable.

Feel free to disregard my original comment then and keep the sentence. :)

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/20230614165904.1.I279773c37e2c1ed8fbb622ca6d1397aea0023526@changeid/