| Message ID | 20200524023815.21789-3-jonathan@marek.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: smmu/USB nodes and HDK855/HDK865 dts | expand |
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. > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > --- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 107 +++++++++++++++++++++++++++ > 1 file changed, 107 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi > b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 2f99c350c287..43c5e48c15e2 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -323,6 +323,8 @@ ufs_mem_hc: ufshc@1d84000 { > > power-domains = <&gcc UFS_PHY_GDSC>; > > + iommus = <&apps_smmu 0x0e0 0>, <&apps_smmu 0x4e0 0>; > + > clock-names = > "core_clk", > "bus_aggr_clk", > @@ -428,6 +430,111 @@ tlmm: pinctrl@f100000 { > wakeup-parent = <&pdc>; > }; > > + apps_smmu: iommu@15000000 { > + compatible = "qcom,sdm845-smmu-500", "arm,mmu-500"; This should be qcom,sm8250-smmu-500 and also you need to update the arm-smmu binding with this compatible in a separate patch. -Sai
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/ >> Signed-off-by: Jonathan Marek <jonathan@marek.ca> >> --- >> arch/arm64/boot/dts/qcom/sm8250.dtsi | 107 +++++++++++++++++++++++++++ >> 1 file changed, 107 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi >> b/arch/arm64/boot/dts/qcom/sm8250.dtsi >> index 2f99c350c287..43c5e48c15e2 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi >> @@ -323,6 +323,8 @@ ufs_mem_hc: ufshc@1d84000 { >> >> power-domains = <&gcc UFS_PHY_GDSC>; >> >> + iommus = <&apps_smmu 0x0e0 0>, <&apps_smmu 0x4e0 0>; >> + >> clock-names = >> "core_clk", >> "bus_aggr_clk", >> @@ -428,6 +430,111 @@ tlmm: pinctrl@f100000 { >> wakeup-parent = <&pdc>; >> }; >> >> + apps_smmu: iommu@15000000 { >> + compatible = "qcom,sdm845-smmu-500", "arm,mmu-500"; > > This should be qcom,sm8250-smmu-500 and also you need to update the > arm-smmu > binding with this compatible in a separate patch. > > -Sai >
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
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. With either value of ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT, it will not work without the iommus property.
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. > 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. -Sai
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 >
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. >>> 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. -Sai
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? >>>> 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 > -Sai >
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
On Mon 25 May 04:53 PDT 2020, Jonathan Marek wrote: [..] > 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? > This looks better, but it's actually not a bypass mapping that we inherit from the bootloader, it's the stream mapping pointing to a disabled (~ARM_SMMU_SCTLR_M) context bank. So when we wipe the stream mappings we will fault on the unmatched stream - which secure world "handles" for us... As such, I think you should replace "bypass" with "stream". Regards, Bjorn
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi index 2f99c350c287..43c5e48c15e2 100644 --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi @@ -323,6 +323,8 @@ ufs_mem_hc: ufshc@1d84000 { power-domains = <&gcc UFS_PHY_GDSC>; + iommus = <&apps_smmu 0x0e0 0>, <&apps_smmu 0x4e0 0>; + clock-names = "core_clk", "bus_aggr_clk", @@ -428,6 +430,111 @@ tlmm: pinctrl@f100000 { wakeup-parent = <&pdc>; }; + apps_smmu: iommu@15000000 { + compatible = "qcom,sdm845-smmu-500", "arm,mmu-500"; + reg = <0 0x15000000 0 0x100000>; + #iommu-cells = <2>; + #global-interrupts = <2>; + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 182 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 185 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 187 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 315 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 318 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 319 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 320 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 321 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 322 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 323 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 324 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 325 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 326 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 327 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 328 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 329 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 331 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 332 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 333 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 334 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 335 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 336 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 337 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 338 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 339 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 340 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 341 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 342 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 343 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 344 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 345 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 395 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 396 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 397 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 399 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 401 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 402 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 407 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 408 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 409 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 412 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 418 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 419 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 421 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 423 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 424 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 690 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 691 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 692 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 693 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 694 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 695 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 696 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 697 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 707 IRQ_TYPE_LEVEL_HIGH>; + }; + intc: interrupt-controller@17a00000 { compatible = "arm,gic-v3"; #interrupt-cells = <3>;
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. Signed-off-by: Jonathan Marek <jonathan@marek.ca> --- arch/arm64/boot/dts/qcom/sm8250.dtsi | 107 +++++++++++++++++++++++++++ 1 file changed, 107 insertions(+)