Messages in this thread | | | Date | Mon, 25 May 2020 17:28:14 +0530 | From | Sai Prakash Ranjan <> | Subject | Re: [PATCH 2/6] arm64: dts: qcom: sm8250: add apps_smmu node |
| |
On 2020-05-25 17:23, Jonathan Marek wrote: > On 5/25/20 7:40 AM, Sai Prakash Ranjan wrote: >> On 2020-05-25 16:57, Jonathan Marek wrote: >>> On 5/25/20 7:17 AM, Sai Prakash Ranjan wrote: >>>> Hi, >>>> >>>> On 2020-05-25 16:38, Jonathan Marek wrote: >>>>> On 5/25/20 6:54 AM, Sai Prakash Ranjan wrote: >>>>>> On 2020-05-25 15:39, Jonathan Marek wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 5/25/20 5:42 AM, Sai Prakash Ranjan wrote: >>>>>>>> Hi Jonathan, >>>>>>>> >>>>>>>> On 2020-05-24 08:08, Jonathan Marek wrote: >>>>>>>>> Add the apps_smmu node for sm8250. Note that adding the iommus >>>>>>>>> field for >>>>>>>>> UFS is required because initializing the iommu removes the >>>>>>>>> bypass mapping >>>>>>>>> that created by the bootloader. >>>>>>>>> >>>>>>>> >>>>>>>> This statement doesn't seem right, you can just say since the >>>>>>>> bypass is disabled >>>>>>>> by default now, we need to add this property to enable >>>>>>>> translation and avoid global faults. >>>>>>>> >>>>>>> >>>>>>> If I use this patch [1] then the UFS iommu property isn't needed. >>>>>>> In >>>>>>> downstream, the identity (bypass?) stream mapping is inherited >>>>>>> from >>>>>>> the bootloader, and UFS is used without any iommu property. >>>>>>> Setting >>>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n doesn't make it work on its >>>>>>> own >>>>>>> (without the UFS iommu property), so there's more to it than just >>>>>>> "bypass is disabled by default now". >>>>>>> >>>>>>> https://patchwork.kernel.org/patch/11310757/ >>>>>>> >>>>>> >>>>>> "iommus" property is not about inheriting stream mapping from >>>>>> bootloader, >>>>>> it is used to enable SMMU address translation for the >>>>>> corresponding >>>>>> master when specified. So when you have disabled bypass, i.e., >>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y or via cmdline >>>>>> "arm-smmu.disable_bypass=1" >>>>>> and iommus property with SID and mask is not specified, then it >>>>>> will result >>>>>> in SMMU global faults. >>>>>> >>>>>> Downstream has bypass >>>>>> enabled(ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n),so you >>>>>> won't see any global faults if you do not have iommus property. >>>>>> >>>>>> Patch in your link is for display because of the usecase for >>>>>> splash screen >>>>>> on android and some other devices where the bootloader will >>>>>> configure SMMU, >>>>>> it has not yet merged and not likely to get merged in the current >>>>>> state. >>>>>> >>>>>> So yes "there is *not* much more to it than bypass is disabled by >>>>>> default now" >>>>>> and you have to specify "iommus" for the master devices or you >>>>>> should enable bypass, >>>>>> i.e., ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or >>>>>> arm-smmu.disable_bypass=n >>>>>> >>>>>> Try without the patch in the link and without iommus for UFS and >>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y and you will see. >>>>>> >>>>>> -Sai >>>>> >>>>> I know that the "iommus" property is not about inheriting stream >>>>> mapping. Probing the iommu removes the stream mapping created by >>>>> the >>>>> bootloader, the iommus property is added so that new mappings are >>>>> created to replace what was removed. >>>>> >>>>> You seem to be under the impression that the SM8150/SM8250 >>>>> bootloader >>>>> does not configure SMMU. It does, for both UFS and SDHC, just like >>>>> it >>>>> does for display/splash screen on some devices. >>>>> >>>> >>>> It could be that bootloader does configure SMMU for UFS and SDHC, >>>> but the >>>> upstream SMMU driver doesnt allow to inherit stream mapping from the >>>> bootloader >>>> yet, so adding iommus property based on the assumption that it is >>>> inherited seems >>>> wrong. >>>> >>> >>> I never said adding the iommus property is for inheriting stream >>> mapping. I mentioned inheriting to say UFS works without the iommus >>> property on downstream (it inherits a identity/bypass mapping). >>> >> >> Your commit description says "adding the iommus field for UFS is >> required >> because initializing the iommu removes the bypass mapping that created >> by the >> bootloader". So here it would mean like iommus property for UFS is not >> for >> enabling address translation by SMMU for UFS but to avoid removing >> mappings >> created by the bootloader which is not exactly what iommus property is >> for. >> > > I guess the commit message is ambiguous, that's not what I meant. Is > "Now that the kernel initializes the iommu, the bypass mappings set by > the bootloader are cleared. Adding the iommus property is required so > that new mappings are created for UFS." better? >
Yes looks good.
>>>>> With either value of ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT, it will >>>>> not >>>>> work without the iommus property. >>>> >>>> I'm pretty sure that if you have >>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n and >>>> without iommus, it should work. >>>> >>> >>> It doesn't work, with either ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or >>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y. >>> >> >> Ok since you are very sure about this, I will try with your patches on >> SM8150 MTP tomorrow since I do not have access to one now. >> Also just to make sure, please remove all the extra SMMU patches you >> have >> in your tree which are not yet merged or from downstream kernel. >> > > FYI, I have a branch [1] integrating patches for upstream. All the > patches up to 34fff8a519cc075933 ("arm64: dts: qcom: add sm8250 GPU > nodes") have been submitted (and the patches after that are > unnecessary for testing on sm8150). > > [1] https://github.com/flto/linux/commits/sm8x50-upstream >
Thanks, I will test this out.
-Sai -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |