| Message ID | cover.1614332994.git.saiprakash.ranjan@codeaurora.org |
|---|---|
| Headers | show |
| Series | iommu/arm-smmu-qcom: Add SC7280 support | expand |
On Fri, Feb 26, 2021 at 03:25:40PM +0530, Sai Prakash Ranjan wrote: > Adreno(GPU) SMMU and APSS(Application Processor SubSystem) SMMU > both implement "arm,mmu-500" in some QTI SoCs and to run through > adreno smmu specific implementation such as enabling split pagetables > support, we need to match the "qcom,adreno-smmu" compatible first > before apss smmu or else we will be running apps smmu implementation > for adreno smmu and the additional features for adreno smmu is never > set. For ex: we have "qcom,sc7280-smmu-500" compatible for both apps > and adreno smmu implementing "arm,mmu-500", so the adreno smmu > implementation is never reached because the current sequence checks > for apps smmu compatible(qcom,sc7280-smmu-500) first and runs that > specific impl and we never reach adreno smmu specific implementation. > > Suggested-by: Akhil P Oommen <akhilpo@codeaurora.org> Acked-by: Jordan Crouse <jordan@cosmicpenguin.net> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> > --- > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > index bea3ee0dabc2..03f048aebb80 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > @@ -345,11 +345,17 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu) > { > const struct device_node *np = smmu->dev->of_node; > > - if (of_match_node(qcom_smmu_impl_of_match, np)) > - return qcom_smmu_create(smmu, &qcom_smmu_impl); > - > + /* > + * Do not change this order of implementation, i.e., first adreno > + * smmu impl and then apss smmu since we can have both implementing > + * arm,mmu-500 in which case we will miss setting adreno smmu specific > + * features if the order is changed. > + */ > if (of_device_is_compatible(np, "qcom,adreno-smmu")) > return qcom_smmu_create(smmu, &qcom_adreno_smmu_impl); > > + if (of_match_node(qcom_smmu_impl_of_match, np)) > + return qcom_smmu_create(smmu, &qcom_smmu_impl); > + > return smmu; > } > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Fri, Feb 26, 2021 at 9:24 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Fri 26 Feb 03:55 CST 2021, Sai Prakash Ranjan wrote: > > > Adreno(GPU) SMMU and APSS(Application Processor SubSystem) SMMU > > both implement "arm,mmu-500" in some QTI SoCs and to run through > > adreno smmu specific implementation such as enabling split pagetables > > support, we need to match the "qcom,adreno-smmu" compatible first > > before apss smmu or else we will be running apps smmu implementation > > for adreno smmu and the additional features for adreno smmu is never > > set. For ex: we have "qcom,sc7280-smmu-500" compatible for both apps > > and adreno smmu implementing "arm,mmu-500", so the adreno smmu > > implementation is never reached because the current sequence checks > > for apps smmu compatible(qcom,sc7280-smmu-500) first and runs that > > specific impl and we never reach adreno smmu specific implementation. > > > > So you're saying that you have a single SMMU instance that's compatible > with both an entry in qcom_smmu_impl_of_match[] and "qcom,adreno-smmu"? > > Per your proposed change we will pick the adreno ops _only_ for this > component, essentially disabling the non-Adreno quirks selected by the > qcom impl. As such keeping the non-adreno compatible in the > qcom_smmu_impl_init[] seems to only serve to obfuscate the situation. > > Don't we somehow need the combined set of quirks? (At least if we're > running this with a standard UEFI based boot flow?) > are you thinking of the apps-smmu handover of display context bank? That shouldn't change, the only thing that changes is that gpu-smmu becomes an mmu-500, whereas previously only apps-smmu was.. BR, -R
On Fri, Feb 26, 2021 at 11:24:52AM -0600, Bjorn Andersson wrote: > On Fri 26 Feb 03:55 CST 2021, Sai Prakash Ranjan wrote: > > > Adreno(GPU) SMMU and APSS(Application Processor SubSystem) SMMU > > both implement "arm,mmu-500" in some QTI SoCs and to run through > > adreno smmu specific implementation such as enabling split pagetables > > support, we need to match the "qcom,adreno-smmu" compatible first > > before apss smmu or else we will be running apps smmu implementation > > for adreno smmu and the additional features for adreno smmu is never > > set. For ex: we have "qcom,sc7280-smmu-500" compatible for both apps > > and adreno smmu implementing "arm,mmu-500", so the adreno smmu > > implementation is never reached because the current sequence checks > > for apps smmu compatible(qcom,sc7280-smmu-500) first and runs that > > specific impl and we never reach adreno smmu specific implementation. > > > > So you're saying that you have a single SMMU instance that's compatible > with both an entry in qcom_smmu_impl_of_match[] and "qcom,adreno-smmu"? > > Per your proposed change we will pick the adreno ops _only_ for this > component, essentially disabling the non-Adreno quirks selected by the > qcom impl. As such keeping the non-adreno compatible in the > qcom_smmu_impl_init[] seems to only serve to obfuscate the situation. > > Don't we somehow need the combined set of quirks? (At least if we're > running this with a standard UEFI based boot flow?) We *do* need the combined set of quirks, so there has to be an adreno-smmu impelmentation that matches the "generic" implementation with a few extra function hooks added on. I'm not sure if there is a clever way to figure out how to meld the implementation hooks at runtime but the alternative is to just make sure that the adreno-smmu static struct calls the same quirks as its generic partner. Jordan > > Suggested-by: Akhil P Oommen <akhilpo@codeaurora.org> > > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> > > --- > > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > > index bea3ee0dabc2..03f048aebb80 100644 > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > > @@ -345,11 +345,17 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu) > > { > > const struct device_node *np = smmu->dev->of_node; > > > > - if (of_match_node(qcom_smmu_impl_of_match, np)) > > - return qcom_smmu_create(smmu, &qcom_smmu_impl); > > - > > + /* > > + * Do not change this order of implementation, i.e., first adreno > > + * smmu impl and then apss smmu since we can have both implementing > > + * arm,mmu-500 in which case we will miss setting adreno smmu specific > > + * features if the order is changed. > > + */ > > if (of_device_is_compatible(np, "qcom,adreno-smmu")) > > return qcom_smmu_create(smmu, &qcom_adreno_smmu_impl); > > > > + if (of_match_node(qcom_smmu_impl_of_match, np)) > > + return qcom_smmu_create(smmu, &qcom_smmu_impl); > > + > > return smmu; > > } > > -- > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > > of Code Aurora Forum, hosted by The Linux Foundation > > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Fri, Feb 26, 2021 at 11:48:13AM -0700, Jordan Crouse wrote: > On Fri, Feb 26, 2021 at 11:24:52AM -0600, Bjorn Andersson wrote: > > On Fri 26 Feb 03:55 CST 2021, Sai Prakash Ranjan wrote: > > > > > Adreno(GPU) SMMU and APSS(Application Processor SubSystem) SMMU > > > both implement "arm,mmu-500" in some QTI SoCs and to run through > > > adreno smmu specific implementation such as enabling split pagetables > > > support, we need to match the "qcom,adreno-smmu" compatible first > > > before apss smmu or else we will be running apps smmu implementation > > > for adreno smmu and the additional features for adreno smmu is never > > > set. For ex: we have "qcom,sc7280-smmu-500" compatible for both apps > > > and adreno smmu implementing "arm,mmu-500", so the adreno smmu > > > implementation is never reached because the current sequence checks > > > for apps smmu compatible(qcom,sc7280-smmu-500) first and runs that > > > specific impl and we never reach adreno smmu specific implementation. > > > > > > > So you're saying that you have a single SMMU instance that's compatible > > with both an entry in qcom_smmu_impl_of_match[] and "qcom,adreno-smmu"? > > > > Per your proposed change we will pick the adreno ops _only_ for this > > component, essentially disabling the non-Adreno quirks selected by the > > qcom impl. As such keeping the non-adreno compatible in the > > qcom_smmu_impl_init[] seems to only serve to obfuscate the situation. > > > > Don't we somehow need the combined set of quirks? (At least if we're > > running this with a standard UEFI based boot flow?) > > We *do* need the combined set of quirks, so there has to be an adreno-smmu > impelmentation that matches the "generic" implementation with a few extra > function hooks added on. I'm not sure if there is a clever way to figure out how > to meld the implementation hooks at runtime but the alternative is to just make > sure that the adreno-smmu static struct calls the same quirks as its generic > partner. To clarify, the gpu-smmu doesn't strictly need the s2cr handoff or the cfg_probe though it wouldn't hurt to have them since they would be essentially passthroughs for the GPU. We do need to capture errata like the sdm845_smmu500_reset which is already part of the upstream adreno implementation. I think the main takeaway is that if a new errata or quirk is added for main mmu500 it needs to be considered for adreno-smmu too. Jordan > > > Suggested-by: Akhil P Oommen <akhilpo@codeaurora.org> > > > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> > > > --- > > > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > > > index bea3ee0dabc2..03f048aebb80 100644 > > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > > > @@ -345,11 +345,17 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu) > > > { > > > const struct device_node *np = smmu->dev->of_node; > > > > > > - if (of_match_node(qcom_smmu_impl_of_match, np)) > > > - return qcom_smmu_create(smmu, &qcom_smmu_impl); > > > - > > > + /* > > > + * Do not change this order of implementation, i.e., first adreno > > > + * smmu impl and then apss smmu since we can have both implementing > > > + * arm,mmu-500 in which case we will miss setting adreno smmu specific > > > + * features if the order is changed. > > > + */ > > > if (of_device_is_compatible(np, "qcom,adreno-smmu")) > > > return qcom_smmu_create(smmu, &qcom_adreno_smmu_impl); > > > > > > + if (of_match_node(qcom_smmu_impl_of_match, np)) > > > + return qcom_smmu_create(smmu, &qcom_smmu_impl); > > > + > > > return smmu; > > > } > > > -- > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > > > of Code Aurora Forum, hosted by The Linux Foundation > > > > > _______________________________________________ > > iommu mailing list > > iommu@lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/iommu