Message-ID: <20200705033511.GR388985@builder.lan>
Date:   Sat, 4 Jul 2020 20:35:11 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Will Deacon <will@...nel.org>
Cc:     Konrad Dybcio <konradybcio@...il.com>, skrzynka@...radybcio.pl,
        Robin Murphy <robin.murphy@....com>,
        Joerg Roedel <joro@...tes.org>,
        Rob Herring <robh+dt@...nel.org>,
        linux-arm-kernel@...ts.infradead.org,
        iommu@...ts.linux-foundation.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, jcrouse@...eaurora.org,
        john.stultz@...aro.org
Subject: Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

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@...il.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

Powered by blists - more mailing lists