| Message ID | 20230615145253.1.Ic62daa649b47b656b313551d646c4de9a7da4bd4@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: qcom: sc7180: Mark SCM as dma-coherent for trogdor | expand |
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/
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"; }; };
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(-)