| Message ID | 20200704122809.73794-1-konradybcio@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] iommu/arm-smmu: Implement qcom,skip-init | expand |
[Adding Bjorn, Jordan and John because I really don't want a bunch of different ways to tell the driver that the firmware is screwing things up] On Sat, Jul 04, 2020 at 02:28:09PM +0200, Konrad Dybcio wrote: > This adds the downstream property required to support > SMMUs on SDM630 and other platforms (the need for it > most likely depends on firmware configuration). > > Signed-off-by: Konrad Dybcio <konradybcio@gmail.com> > --- > .../devicetree/bindings/iommu/arm,smmu.yaml | 10 ++++++++++ > drivers/iommu/arm-smmu.c | 15 +++++++++------ > 2 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > index d7ceb4c34423..9abd6d41a32c 100644 > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > @@ -102,6 +102,16 @@ properties: > access to SMMU configuration registers. In this case non-secure aliases of > secure registers have to be used during SMMU configuration. > > + qcom,skip-init: > + description: | > + Disable resetting configuration for all context banks > + during device reset. This is useful for targets where > + some context banks are dedicated to other execution > + environments outside of Linux and those other EEs are > + programming their own stream match tables, SCTLR, etc. > + Without setting this option we will trample on their > + configuration. It would probably be better to know _which_ context banks we shouldn't touch, no? Otherwise what happens to the others? > + > stream-match-mask: > $ref: /schemas/types.yaml#/definitions/uint32 > description: | > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 243bc4cb2705..a5c623d4caf9 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -1655,13 +1655,16 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) > * Reset stream mapping groups: Initial values mark all SMRn as > * invalid and all S2CRn as bypass unless overridden. > */ > - for (i = 0; i < smmu->num_mapping_groups; ++i) > - arm_smmu_write_sme(smmu, i); > > - /* Make sure all context banks are disabled and clear CB_FSR */ > - for (i = 0; i < smmu->num_context_banks; ++i) { > - arm_smmu_write_context_bank(smmu, i); > - arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT); > + if (!of_find_property(smmu->dev->of_node, "qcom,skip-init", NULL)) { > + for (i = 0; i < smmu->num_mapping_groups; ++i) > + arm_smmu_write_sme(smmu, i); > + > + /* Make sure all context banks are disabled and clear CB_FSR */ > + for (i = 0; i < smmu->num_context_banks; ++i) { > + arm_smmu_write_context_bank(smmu, i); > + arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT); > + } > } Do we not need to worry about the SMRs as well? Will
> It would probably be better to know _which_ context banks we shouldn't > touch, no? Otherwise what happens to the others? > Do we not need to worry about the SMRs as well? This was mimicked from CAF (think [1]) and the SMMUs don't make the hypervisor angry anymore, so I wouldn't be too picky on that if it works.. [1] https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.7.1.r1/drivers/iommu/arm-smmu.c#L4104-L4109 Regards Konrad
On Sat 04 Jul 06:09 PDT 2020, Will Deacon wrote: > [Adding Bjorn, Jordan and John because I really don't want a bunch of > different ways to tell the driver that the firmware is screwing things up] > Thanks Will. > On Sat, Jul 04, 2020 at 02:28:09PM +0200, Konrad Dybcio wrote: > > This adds the downstream property required to support > > SMMUs on SDM630 and other platforms (the need for it > > most likely depends on firmware configuration). > > > > Signed-off-by: Konrad Dybcio <konradybcio@gmail.com> > > --- > > .../devicetree/bindings/iommu/arm,smmu.yaml | 10 ++++++++++ > > drivers/iommu/arm-smmu.c | 15 +++++++++------ > > 2 files changed, 19 insertions(+), 6 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > index d7ceb4c34423..9abd6d41a32c 100644 > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > @@ -102,6 +102,16 @@ properties: > > access to SMMU configuration registers. In this case non-secure aliases of > > secure registers have to be used during SMMU configuration. > > > > + qcom,skip-init: > > + description: | > > + Disable resetting configuration for all context banks > > + during device reset. This is useful for targets where > > + some context banks are dedicated to other execution > > + environments outside of Linux and those other EEs are > > + programming their own stream match tables, SCTLR, etc. > > + Without setting this option we will trample on their > > + configuration. > > It would probably be better to know _which_ context banks we shouldn't > touch, no? Otherwise what happens to the others? > From my investigations of the issue of maintaining the boot display through the initialization of arm-smmu I assume the reason for skipping this step don't want to flush out the SMR, S2CR and context bank initialization because it would disrupt the display hardware's access to memory. And in itself I believe that this is quite certainly going to work - until you start attaching devices. Because in itself this does nothing to ensure that the driver won't overwrite stream mapping or context bank configuration as devices are attached. So on e.g. SDM845 we need to ensure that the driver doesn't stomp over the display mapping left by the bootloader. Further more, on platforms such as QCS405 (which are derived from platforms supported by qcom_iommu today), the stream mapping registers (SMR and S2CR) are write ignore, which means that without knowledge about the existing mappings the hardware and driver will be out of sync. NB. Compared to the platforms that is supported by qcom_iommu, the stream mapping registers are readable on these newer platforms, while on e.g. MSM8916 we get an access violation by attempting to read SMR/S2CR. > > + > > stream-match-mask: > > $ref: /schemas/types.yaml#/definitions/uint32 > > description: | > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > > index 243bc4cb2705..a5c623d4caf9 100644 > > --- a/drivers/iommu/arm-smmu.c > > +++ b/drivers/iommu/arm-smmu.c > > @@ -1655,13 +1655,16 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) > > * Reset stream mapping groups: Initial values mark all SMRn as > > * invalid and all S2CRn as bypass unless overridden. > > */ > > - for (i = 0; i < smmu->num_mapping_groups; ++i) > > - arm_smmu_write_sme(smmu, i); > > > > - /* Make sure all context banks are disabled and clear CB_FSR */ > > - for (i = 0; i < smmu->num_context_banks; ++i) { > > - arm_smmu_write_context_bank(smmu, i); > > - arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT); > > + if (!of_find_property(smmu->dev->of_node, "qcom,skip-init", NULL)) { > > + for (i = 0; i < smmu->num_mapping_groups; ++i) > > + arm_smmu_write_sme(smmu, i); > > + > > + /* Make sure all context banks are disabled and clear CB_FSR */ > > + for (i = 0; i < smmu->num_context_banks; ++i) { > > + arm_smmu_write_context_bank(smmu, i); > > + arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT); > > + } > > } > > Do we not need to worry about the SMRs as well? > I don't think we should skip the actual initialization, because to avoid strange side effects we need to ensure that the driver and hardware are in sync (either for specific streams/banks or for all of them). I've continued my work on supporting boot display on e.g. SDM845, based on Thierry's patches, but still have some unresolved corner cases to fully resolve - e.g. how to ensure that the display hardware's stream mapping survives the probe deferral of the display driver. Hopefully I will be able to post something in a few days. That said, there's a generation of platforms between MSM8916 (which we support using qcom_iommu) and SDM845 (which can run with arm-smmu). AngeloGioacchino proposed a series last year to extend the qcom_iommu to support these [1]. If SD630 falls in this category, or in the newer SDM845/SM8150 category I don't know. It would be quite interesting to hear more about the exact behaviors seems on SDM630, to see how we can support this as well. [1] https://lore.kernel.org/linux-arm-msm/20191001155641.37117-1-kholk11@gmail.com/ Regards, Bjorn
So.. is this a no-no? I of course would like to omit this entirely, but SMMUs on sdm630 and friends are REALLY picky.. What seems to happen is that when the driver tries to do things the "standard" way, hypervisor decides to hang the platform or force a reboot. Not very usable. This thing is needed for the platform to even boot properly and one more [1] is required to make mdss work with video mode panels (the fact that CMD-mode panels work is kinda hilarious to me). To be honest, there are even more qcom quirks (of which at least qcom,dynamic and qcom-use-3-lvl-tables are used on 630).. [2] Looking forward to your answers and possibly better solutions. [1] https://github.com/konradybcio/linux/commit/83ac38af259968f92b6a8b7eab90096c78469f87 [2] https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.7.1.r1/drivers/iommu/arm-smmu.c#L404-L415 Regards Konrad
On Tue, Jul 21, 2020 at 05:04:11PM +0200, Konrad Dybcio wrote: > So.. is this a no-no? > > I of course would like to omit this entirely, but SMMUs on sdm630 and > friends are REALLY picky.. What seems to happen is that when the > driver tries to do things the "standard" way, hypervisor decides to > hang the platform or force a reboot. Not very usable. > > > This thing is needed for the platform to even boot properly and one > more [1] is required to make mdss work with video mode panels (the > fact that CMD-mode panels work is kinda hilarious to me). > > To be honest, there are even more qcom quirks (of which at least > qcom,dynamic and qcom-use-3-lvl-tables are used on 630).. [2] > > Looking forward to your answers and possibly better solutions. Nobody is disputing that the qcom SMMUs don't have their share of quirks but it seems that the community has mostly settled on the agreement that there are better ways to solve this than a handful of device tree properties. The current focus has been on moving more of the SMMU specific bits into the arm-smmu-qcom implementation [1] and I think that is the right way to go. As for the other quirks we can probably discuss those on a case by case basis. I doubt you will find much enthusiasm for qcom,use-3-lvl-tables and I've been working on replacing qcom,dynamic with something much better [2]. [1] https://lists.linuxfoundation.org/pipermail/iommu/2020-July/046304.html [2] https://lists.linuxfoundation.org/pipermail/iommu/2020-July/046756.html Jordan > [1] https://github.com/konradybcio/linux/commit/83ac38af259968f92b6a8b7eab90096c78469f87 > [2] https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.7.1.r1/drivers/iommu/arm-smmu.c#L404-L415 > > Regards > Konrad
>The current >focus has been on moving more of the SMMU specific bits into the arm-smmu-qcom >implementation [1] and I think that is the right way to go. Pardon if I overlooked something obvious, but I can't seem to find a clean way for implementing qcom,skip-init in arm-smmu-qcom, as neither the arm_smmu_test_smr_masks nor the probe function seem to be alterable with arm_smmu_impl. I'm open to your ideas guys. Konrad
On Tue 21 Jul 09:20 PDT 2020, Konrad Dybcio wrote: > >The current > >focus has been on moving more of the SMMU specific bits into the arm-smmu-qcom > >implementation [1] and I think that is the right way to go. > > Pardon if I overlooked something obvious, but I can't seem to find a > clean way for implementing qcom,skip-init in arm-smmu-qcom, as neither > the arm_smmu_test_smr_masks nor the probe function seem to be > alterable with arm_smmu_impl. I'm open to your ideas guys. > Is the problem on SDM630 that when you write to SMR/S2CR the device reboots? Or that when you start writing out the context bank configuration that trips the display and the device reboots? Regards, Bjorn
>Is the problem on SDM630 that when you write to SMR/S2CR the device >reboots? Or that when you start writing out the context bank >configuration that trips the display and the device reboots? I added some debug prints and the phone hangs after reaching the seventh CB (with i=6) at arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT); line in arm_smmu_device_reset. Konrad
On Wed 22 Jul 13:11 PDT 2020, Konrad Dybcio wrote: > >Is the problem on SDM630 that when you write to SMR/S2CR the device > >reboots? Or that when you start writing out the context bank > >configuration that trips the display and the device reboots? > > I added some debug prints and the phone hangs after reaching the > seventh CB (with i=6) at > > arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT); > > line in arm_smmu_device_reset. > Sounds like things are progressing nicely for a while there, presumably until the next time the display is being refreshed. Would you be willing to try out the following work in progress: https://lore.kernel.org/linux-arm-msm/20200717001619.325317-1-bjorn.andersson@linaro.org/ You need to adjust drivers/iommu/arm-smmu-impl.c so that arm_smmu_impl_init() will invoke qcom_smmu_impl_init() as it spots your apps smmu. Regards, Bjorn
> Sounds like things are progressing nicely for a while there, presumably > until the next time the display is being refreshed. > > Would you be willing to try out the following work in progress: > https://lore.kernel.org/linux-arm-msm/20200717001619.325317-1-bjorn.andersson@linaro.org/ I sure would like to if you could be kind enough to tell me which tree I should apply it against. Latest -next brought some changes to drivers/iommu/ structure which makes this not apply at all :/ Konrad
diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml index d7ceb4c34423..9abd6d41a32c 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml @@ -102,6 +102,16 @@ properties: access to SMMU configuration registers. In this case non-secure aliases of secure registers have to be used during SMMU configuration. + qcom,skip-init: + description: | + Disable resetting configuration for all context banks + during device reset. This is useful for targets where + some context banks are dedicated to other execution + environments outside of Linux and those other EEs are + programming their own stream match tables, SCTLR, etc. + Without setting this option we will trample on their + configuration. + stream-match-mask: $ref: /schemas/types.yaml#/definitions/uint32 description: | diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 243bc4cb2705..a5c623d4caf9 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1655,13 +1655,16 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) * Reset stream mapping groups: Initial values mark all SMRn as * invalid and all S2CRn as bypass unless overridden. */ - for (i = 0; i < smmu->num_mapping_groups; ++i) - arm_smmu_write_sme(smmu, i); - /* Make sure all context banks are disabled and clear CB_FSR */ - for (i = 0; i < smmu->num_context_banks; ++i) { - arm_smmu_write_context_bank(smmu, i); - arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT); + if (!of_find_property(smmu->dev->of_node, "qcom,skip-init", NULL)) { + for (i = 0; i < smmu->num_mapping_groups; ++i) + arm_smmu_write_sme(smmu, i); + + /* Make sure all context banks are disabled and clear CB_FSR */ + for (i = 0; i < smmu->num_context_banks; ++i) { + arm_smmu_write_context_bank(smmu, i); + arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT); + } } /* Invalidate the TLB, just in case */
This adds the downstream property required to support SMMUs on SDM630 and other platforms (the need for it most likely depends on firmware configuration). Signed-off-by: Konrad Dybcio <konradybcio@gmail.com> --- .../devicetree/bindings/iommu/arm,smmu.yaml | 10 ++++++++++ drivers/iommu/arm-smmu.c | 15 +++++++++------ 2 files changed, 19 insertions(+), 6 deletions(-)