Re: [PATCH 2/6] arm64: dts: qcom: sm8250: add apps_smmu node

[Jonathan Marek <jonathan@marek.ca>]

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).

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

> -Sai
>