| Message ID | 20200630001051.12350-3-vdumpa@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Nvidia Arm SMMUv2 Implementation | expand |
> Add binding for NVIDIA's Tegra194 SoC SMMU topology that is based on ARM > MMU-500. > > Signed-off-by: Krishna Reddy <vdumpa@nvidia.com> Reviewed-by: Pritesh Raithatha <praithatha@nvidia.com>
On 30/06/2020 01:10, Krishna Reddy wrote: > Add binding for NVIDIA's Tegra194 SoC SMMU topology that is based > on ARM MMU-500. > > Signed-off-by: Krishna Reddy <vdumpa@nvidia.com> > --- > Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > index d7ceb4c34423b..5b2586ac715ed 100644 > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > @@ -38,6 +38,11 @@ properties: > - qcom,sc7180-smmu-500 > - qcom,sdm845-smmu-500 > - const: arm,mmu-500 > + - description: NVIDIA SoCs that use more than one "arm,mmu-500" > + items: > + - enum: > + - nvdia,tegra194-smmu s/nvdia/nvidia Jon
On 2020-06-30 01:10, Krishna Reddy wrote: > Add binding for NVIDIA's Tegra194 SoC SMMU topology that is based > on ARM MMU-500. > > Signed-off-by: Krishna Reddy <vdumpa@nvidia.com> > --- > Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > index d7ceb4c34423b..5b2586ac715ed 100644 > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > @@ -38,6 +38,11 @@ properties: > - qcom,sc7180-smmu-500 > - qcom,sdm845-smmu-500 > - const: arm,mmu-500 > + - description: NVIDIA SoCs that use more than one "arm,mmu-500" Hmm, there must be a better way to word that to express that it only applies to the sets of SMMUs that must be programmed identically, and not any other independent MMU-500s that might also happen to be in the same SoC. > + items: > + - enum: > + - nvdia,tegra194-smmu > + - const: arm,mmu-500 Is the fallback compatible appropriate here? If software treats this as a standard MMU-500 it will only program the first instance (because the second isn't presented as a separate MMU-500) - is there any way that isn't going to blow up? Robin. > - items: > - const: arm,mmu-500 > - const: arm,smmu-v2 >
>> + - description: NVIDIA SoCs that use more than one "arm,mmu-500" > Hmm, there must be a better way to word that to express that it only applies to the sets of SMMUs that must be programmed identically, and not any other independent MMU-500s that might also happen to be in the same SoC. Let me reword it to "NVIDIA SoCs that must program multiple MMU-500s identically". >> + items: >> + - enum: >> + - nvdia,tegra194-smmu >> + - const: arm,mmu-500 >Is the fallback compatible appropriate here? If software treats this as a standard MMU-500 it will only program the first instance (because the second isn't presented as a separate MMU-500) - is there any way that isn't going to blow up? When compatible is set to both nvidia,tegra194-smmu and arm,mmu-500, implementation override ensure that both instances are programmed. Isn't it? I am not sure I follow your comment fully. -KR
On 01/07/2020 19:28, Krishna Reddy wrote: >>> + - description: NVIDIA SoCs that use more than one "arm,mmu-500" >> Hmm, there must be a better way to word that to express that it only applies to the sets of SMMUs that must be programmed identically, and not any other independent MMU-500s that might also happen to be in the same SoC. > > Let me reword it to "NVIDIA SoCs that must program multiple MMU-500s identically". > >>> + items: >>> + - enum: >>> + - nvdia,tegra194-smmu >>> + - const: arm,mmu-500 > >> Is the fallback compatible appropriate here? If software treats this as a standard MMU-500 it will only program the first instance (because the second isn't presented as a separate MMU-500) - is there any way that isn't going to blow up? > > When compatible is set to both nvidia,tegra194-smmu and arm,mmu-500, implementation override ensure that both instances are programmed. Isn't it? I am not sure I follow your comment fully. The problem is, if for some reason someone had a Tegra194, but only set the compatible string to 'arm,mmu-500' it would assume that it was a normal arm,mmu-500 and only one instance would be programmed. We always want at least 2 of the 3 instances programmed and so we should only match 'nvidia,tegra194-smmu'. In fact, I think that we also need to update the arm_smmu_of_match table to add 'nvidia,tegra194-smmu' with the data set to &arm_mmu500. Jon
>>>> + items: >>>> + - enum: >>>> + - nvdia,tegra194-smmu >>>> + - const: arm,mmu-500 > > >>> Is the fallback compatible appropriate here? If software treats this as a standard MMU-500 it will only program the first instance (because the second isn't presented as a separate MMU-500) - is there any way that isn't going to blow up? > > >> When compatible is set to both nvidia,tegra194-smmu and arm,mmu-500, implementation override ensure that both instances are programmed. Isn't it? I am not sure I follow your comment fully. >The problem is, if for some reason someone had a Tegra194, but only set the compatible string to 'arm,mmu-500' it would assume that it was a normal arm,mmu-500 and only one instance would be programmed. We always want at least 2 of the 3 instances >programmed and so we should only match 'nvidia,tegra194-smmu'. In fact, I think that we also need to update the arm_smmu_of_match table to add 'nvidia,tegra194-smmu' with the data set to &arm_mmu500. In that case, new binding "nvidia,smmu-v2" can be added with data set to &arm_mmu500 and enumeration would have nvidia,tegra194-smmu and another variant for next generation SoC in future. -KR -- nvpublic
On 2020-07-01 19:47, Jon Hunter wrote: > > On 01/07/2020 19:28, Krishna Reddy wrote: >>>> + - description: NVIDIA SoCs that use more than one "arm,mmu-500" >>> Hmm, there must be a better way to word that to express that it only applies to the sets of SMMUs that must be programmed identically, and not any other independent MMU-500s that might also happen to be in the same SoC. >> >> Let me reword it to "NVIDIA SoCs that must program multiple MMU-500s identically". >> >>>> + items: >>>> + - enum: >>>> + - nvdia,tegra194-smmu >>>> + - const: arm,mmu-500 >> >>> Is the fallback compatible appropriate here? If software treats this as a standard MMU-500 it will only program the first instance (because the second isn't presented as a separate MMU-500) - is there any way that isn't going to blow up? >> >> When compatible is set to both nvidia,tegra194-smmu and arm,mmu-500, implementation override ensure that both instances are programmed. Isn't it? I am not sure I follow your comment fully. > > The problem is, if for some reason someone had a Tegra194, but only set > the compatible string to 'arm,mmu-500' it would assume that it was a > normal arm,mmu-500 and only one instance would be programmed. We always > want at least 2 of the 3 instances programmed and so we should only > match 'nvidia,tegra194-smmu'. In fact, I think that we also need to > update the arm_smmu_of_match table to add 'nvidia,tegra194-smmu' with > the data set to &arm_mmu500. Right, the main concern is if the shiny new DT gets passed to an older kernel (or other OS entirely) which doesn't know the "nvdia,tegra194-smmu" compatible but would match on "arm,mmu-500" and bind a standard driver thinking it's going to work OK. The user would probably prefer that no driver matched and both instances were left turned off, than face the fallout of only one of them being set up. Robin.
On 01/07/2020 20:00, Krishna Reddy wrote: >>>>> + items: >>>>> + - enum: >>>>> + - nvdia,tegra194-smmu >>>>> + - const: arm,mmu-500 >>> >>>> Is the fallback compatible appropriate here? If software treats this as a standard MMU-500 it will only program the first instance (because the second isn't presented as a separate MMU-500) - is there any way that isn't going to blow up? >>> >>> When compatible is set to both nvidia,tegra194-smmu and arm,mmu-500, implementation override ensure that both instances are programmed. Isn't it? I am not sure I follow your comment fully. > >> The problem is, if for some reason someone had a Tegra194, but only set the compatible string to 'arm,mmu-500' it would assume that it was a normal arm,mmu-500 and only one instance would be programmed. We always want at least 2 of the 3 instances >programmed and so we should only match 'nvidia,tegra194-smmu'. In fact, I think that we also need to update the arm_smmu_of_match table to add 'nvidia,tegra194-smmu' with the data set to &arm_mmu500. > > In that case, new binding "nvidia,smmu-v2" can be added with data set to &arm_mmu500 and enumeration would have nvidia,tegra194-smmu and another variant for next generation SoC in future. I think you would be better off with nvidia,smmu-500 as smmu-v2 appears to be something different. I see others have a smmu-v2 but I am not sure if that is legacy. We have an smmu-500 and so that would seem more appropriate. Jon
On 01/07/2020 20:00, Krishna Reddy wrote: >>>>>> + items: >>>>>> + - enum: >>>>>> + - nvdia,tegra194-smmu >>>>>> + - const: arm,mmu-500 >>>> >>>>> Is the fallback compatible appropriate here? If software treats this as a standard MMU-500 it will only program the first instance (because the second isn't presented as a separate MMU-500) - is there any way that isn't going to blow up? >>>> >>>> When compatible is set to both nvidia,tegra194-smmu and arm,mmu-500, implementation override ensure that both instances are programmed. Isn't it? I am not sure I follow your comment fully. >> >>> The problem is, if for some reason someone had a Tegra194, but only set the compatible string to 'arm,mmu-500' it would assume that it was a normal arm,mmu-500 and only one instance would be programmed. We always want at least 2 of the 3 instances >>programmed and so we should only match 'nvidia,tegra194-smmu'. In fact, I think that we also need to update the arm_smmu_of_match table to add 'nvidia,tegra194-smmu' with the data set to &arm_mmu500. >> >> In that case, new binding "nvidia,smmu-v2" can be added with data set to &arm_mmu500 and enumeration would have nvidia,tegra194-smmu and another variant for next generation SoC in future. >I think you would be better off with nvidia,smmu-500 as smmu-v2 appears to be something different. I see others have a smmu-v2 but I am not sure if that is legacy. We have an smmu-500 and so that would seem more appropriate. I tried to use the binding synonymous to other vendors. V2 is the architecture version. MMU-500 is the actual implementation from ARM based on V2 arch. As we just use the MMU-500 IP as it is, It can be named as nvidia,smmu-500 or similar as well. Others probably having their own implementation based on V2 arch. KR -- nvpublic
On 2020-07-01 20:39, Krishna Reddy wrote: > On 01/07/2020 20:00, Krishna Reddy wrote: >>>>>>> + items: >>>>>>> + - enum: >>>>>>> + - nvdia,tegra194-smmu >>>>>>> + - const: arm,mmu-500 >>>>> >>>>>> Is the fallback compatible appropriate here? If software treats this as a standard MMU-500 it will only program the first instance (because the second isn't presented as a separate MMU-500) - is there any way that isn't going to blow up? >>>>> >>>>> When compatible is set to both nvidia,tegra194-smmu and arm,mmu-500, implementation override ensure that both instances are programmed. Isn't it? I am not sure I follow your comment fully. >>> >>>> The problem is, if for some reason someone had a Tegra194, but only set the compatible string to 'arm,mmu-500' it would assume that it was a normal arm,mmu-500 and only one instance would be programmed. We always want at least 2 of the 3 instances >>programmed and so we should only match 'nvidia,tegra194-smmu'. In fact, I think that we also need to update the arm_smmu_of_match table to add 'nvidia,tegra194-smmu' with the data set to &arm_mmu500. >>> >>> In that case, new binding "nvidia,smmu-v2" can be added with data set to &arm_mmu500 and enumeration would have nvidia,tegra194-smmu and another variant for next generation SoC in future. > >> I think you would be better off with nvidia,smmu-500 as smmu-v2 appears to be something different. I see others have a smmu-v2 but I am not sure if that is legacy. We have an smmu-500 and so that would seem more appropriate. > > I tried to use the binding synonymous to other vendors. > V2 is the architecture version. MMU-500 is the actual implementation from ARM based on V2 arch. As we just use the MMU-500 IP as it is, It can be named as nvidia,smmu-500 or similar as well. Yup, that sounds OK to me if you want a broader compatible to potentially match other future SoCs as well. > Others probably having their own implementation based on V2 arch. Exactly - "cavium,smmu-v2" and "qcom,smmu-v2" are their own in-house microarchitectures, not one of Arm's designs, so they don't really have a suitable 'product name' we could have used for the bindings. Robin.
diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml index d7ceb4c34423b..5b2586ac715ed 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml @@ -38,6 +38,11 @@ properties: - qcom,sc7180-smmu-500 - qcom,sdm845-smmu-500 - const: arm,mmu-500 + - description: NVIDIA SoCs that use more than one "arm,mmu-500" + items: + - enum: + - nvdia,tegra194-smmu + - const: arm,mmu-500 - items: - const: arm,mmu-500 - const: arm,smmu-v2
Add binding for NVIDIA's Tegra194 SoC SMMU topology that is based on ARM MMU-500. Signed-off-by: Krishna Reddy <vdumpa@nvidia.com> --- Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 5 +++++ 1 file changed, 5 insertions(+)