| Message ID | 20200817220238.603465-11-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/arm-smmu + drm/msm: per-process GPU pgtables | expand |
Hi, On Mon, Aug 17, 2020 at 3:03 PM Rob Clark <robdclark@gmail.com> wrote: > > From: Jordan Crouse <jcrouse@codeaurora.org> > > Every Qcom Adreno GPU has an embedded SMMU for its own use. These > devices depend on unique features such as split pagetables, > different stall/halt requirements and other settings. Identify them > with a compatible string so that they can be identified in the > arm-smmu implementation specific code. > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > index 503160a7b9a0..5ec5d0d691f6 100644 > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > @@ -40,6 +40,10 @@ properties: > - qcom,sm8150-smmu-500 > - qcom,sm8250-smmu-500 > - const: arm,mmu-500 > + - description: Qcom Adreno GPUs implementing "arm,smmu-v2" > + items: > + - const: qcom,adreno-smmu > + - const: qcom,smmu-v2 I know I'm kinda late to the game, but this seems weird to me, especially given the later patches in the series like: https://lore.kernel.org/r/20200817220238.603465-19-robdclark@gmail.com Specifically in that patch you can see that this IOMMU already had a compatible string and we're changing it and throwing away the model-specific string? I'm guessing that you're just trying to make it easier for code to identify the adreno iommu, but it seems like a better way would have been to just add the adreno compatible in the middle, like: - description: Qcom Adreno GPUs implementing "arm,smmu-v2" items: - enum: - qcom,msm8996-smmu-v2 - qcom,msm8998-smmu-v2 - qcom,sc7180-smmu-v2 - qcom,sdm845-smmu-v2 - const: qcom,adreno-smmu - const: qcom,smmu-v2 Then we still have the SoC-specific compatible string in case we need it but we also have the generic one? It also means that we're not deleting the old compatible string... -Doug > - description: Marvell SoCs implementing "arm,mmu-500" > items: > - const: marvell,ap806-smmu-500 > -- > 2.26.2 >
On Wed, Aug 19, 2020 at 10:03 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Aug 17, 2020 at 3:03 PM Rob Clark <robdclark@gmail.com> wrote: > > > > From: Jordan Crouse <jcrouse@codeaurora.org> > > > > Every Qcom Adreno GPU has an embedded SMMU for its own use. These > > devices depend on unique features such as split pagetables, > > different stall/halt requirements and other settings. Identify them > > with a compatible string so that they can be identified in the > > arm-smmu implementation specific code. > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > index 503160a7b9a0..5ec5d0d691f6 100644 > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > @@ -40,6 +40,10 @@ properties: > > - qcom,sm8150-smmu-500 > > - qcom,sm8250-smmu-500 > > - const: arm,mmu-500 > > + - description: Qcom Adreno GPUs implementing "arm,smmu-v2" > > + items: > > + - const: qcom,adreno-smmu > > + - const: qcom,smmu-v2 > > I know I'm kinda late to the game, but this seems weird to me, > especially given the later patches in the series like: > > https://lore.kernel.org/r/20200817220238.603465-19-robdclark@gmail.com > > Specifically in that patch you can see that this IOMMU already had a > compatible string and we're changing it and throwing away the > model-specific string? I'm guessing that you're just trying to make > it easier for code to identify the adreno iommu, but it seems like a > better way would have been to just add the adreno compatible in the > middle, like: > > - description: Qcom Adreno GPUs implementing "arm,smmu-v2" > items: > - enum: > - qcom,msm8996-smmu-v2 > - qcom,msm8998-smmu-v2 > - qcom,sc7180-smmu-v2 > - qcom,sdm845-smmu-v2 > - const: qcom,adreno-smmu > - const: qcom,smmu-v2 > > Then we still have the SoC-specific compatible string in case we need > it but we also have the generic one? It also means that we're not > deleting the old compatible string... I did bring up the thing about removing the compat string in an earlier revision of the series.. but then we realized that qcom,sc7180-smmu-v2 was never actually used anywhere. But I guess we could: compatible = "qcom,sc7180-smmu-v2", "qcom,adreno-smmu", "qcom,smmu-v2"; BR, -R > > -Doug > > > > - description: Marvell SoCs implementing "arm,mmu-500" > > items: > > - const: marvell,ap806-smmu-500 > > -- > > 2.26.2 > >
Hi, On Wed, Aug 19, 2020 at 10:36 AM Rob Clark <robdclark@gmail.com> wrote: > > On Wed, Aug 19, 2020 at 10:03 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Mon, Aug 17, 2020 at 3:03 PM Rob Clark <robdclark@gmail.com> wrote: > > > > > > From: Jordan Crouse <jcrouse@codeaurora.org> > > > > > > Every Qcom Adreno GPU has an embedded SMMU for its own use. These > > > devices depend on unique features such as split pagetables, > > > different stall/halt requirements and other settings. Identify them > > > with a compatible string so that they can be identified in the > > > arm-smmu implementation specific code. > > > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > > --- > > > Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > > index 503160a7b9a0..5ec5d0d691f6 100644 > > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > > @@ -40,6 +40,10 @@ properties: > > > - qcom,sm8150-smmu-500 > > > - qcom,sm8250-smmu-500 > > > - const: arm,mmu-500 > > > + - description: Qcom Adreno GPUs implementing "arm,smmu-v2" > > > + items: > > > + - const: qcom,adreno-smmu > > > + - const: qcom,smmu-v2 > > > > I know I'm kinda late to the game, but this seems weird to me, > > especially given the later patches in the series like: > > > > https://lore.kernel.org/r/20200817220238.603465-19-robdclark@gmail.com > > > > Specifically in that patch you can see that this IOMMU already had a > > compatible string and we're changing it and throwing away the > > model-specific string? I'm guessing that you're just trying to make > > it easier for code to identify the adreno iommu, but it seems like a > > better way would have been to just add the adreno compatible in the > > middle, like: > > > > - description: Qcom Adreno GPUs implementing "arm,smmu-v2" > > items: > > - enum: > > - qcom,msm8996-smmu-v2 > > - qcom,msm8998-smmu-v2 > > - qcom,sc7180-smmu-v2 > > - qcom,sdm845-smmu-v2 > > - const: qcom,adreno-smmu > > - const: qcom,smmu-v2 > > > > Then we still have the SoC-specific compatible string in case we need > > it but we also have the generic one? It also means that we're not > > deleting the old compatible string... > > I did bring up the thing about removing the compat string in an > earlier revision of the series.. but then we realized that > qcom,sc7180-smmu-v2 was never actually used anywhere. Right, so at least there's not going to be weird issues where landing the dts before the code change will break anything. > But I guess we could: compatible = "qcom,sc7180-smmu-v2", > "qcom,adreno-smmu", "qcom,smmu-v2"; Yeah, that was what I was suggesting. -Doug
On Wed, Aug 19, 2020 at 10:36:38AM -0700, Rob Clark wrote: > On Wed, Aug 19, 2020 at 10:03 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Mon, Aug 17, 2020 at 3:03 PM Rob Clark <robdclark@gmail.com> wrote: > > > > > > From: Jordan Crouse <jcrouse@codeaurora.org> > > > > > > Every Qcom Adreno GPU has an embedded SMMU for its own use. These > > > devices depend on unique features such as split pagetables, > > > different stall/halt requirements and other settings. Identify them > > > with a compatible string so that they can be identified in the > > > arm-smmu implementation specific code. > > > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > > --- > > > Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > > index 503160a7b9a0..5ec5d0d691f6 100644 > > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > > > @@ -40,6 +40,10 @@ properties: > > > - qcom,sm8150-smmu-500 > > > - qcom,sm8250-smmu-500 > > > - const: arm,mmu-500 > > > + - description: Qcom Adreno GPUs implementing "arm,smmu-v2" > > > + items: > > > + - const: qcom,adreno-smmu > > > + - const: qcom,smmu-v2 > > > > I know I'm kinda late to the game, but this seems weird to me, > > especially given the later patches in the series like: > > > > https://lore.kernel.org/r/20200817220238.603465-19-robdclark@gmail.com > > > > Specifically in that patch you can see that this IOMMU already had a > > compatible string and we're changing it and throwing away the > > model-specific string? I'm guessing that you're just trying to make > > it easier for code to identify the adreno iommu, but it seems like a > > better way would have been to just add the adreno compatible in the > > middle, like: > > > > - description: Qcom Adreno GPUs implementing "arm,smmu-v2" > > items: > > - enum: > > - qcom,msm8996-smmu-v2 > > - qcom,msm8998-smmu-v2 > > - qcom,sc7180-smmu-v2 > > - qcom,sdm845-smmu-v2 > > - const: qcom,adreno-smmu > > - const: qcom,smmu-v2 > > > > Then we still have the SoC-specific compatible string in case we need > > it but we also have the generic one? It also means that we're not > > deleting the old compatible string... > > I did bring up the thing about removing the compat string in an > earlier revision of the series.. but then we realized that > qcom,sc7180-smmu-v2 was never actually used anywhere. > > But I guess we could: compatible = "qcom,sc7180-smmu-v2", > "qcom,adreno-smmu", "qcom,smmu-v2"; I think the SoC specific string is intended for the "other" SMMU that everybody else uses. Rarely would a workaround for that SMMU affect the GPU and vice versa. Since these are the bindings it doesn't hurt to allow for the possibility but I would be surprised if the occasion presented itself. Jordan > BR, > -R > > > > > > > > -Doug > > > > > > > - description: Marvell SoCs implementing "arm,mmu-500" > > > items: > > > - const: marvell,ap806-smmu-500 > > > -- > > > 2.26.2 > > >
diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml index 503160a7b9a0..5ec5d0d691f6 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml @@ -40,6 +40,10 @@ properties: - qcom,sm8150-smmu-500 - qcom,sm8250-smmu-500 - const: arm,mmu-500 + - description: Qcom Adreno GPUs implementing "arm,smmu-v2" + items: + - const: qcom,adreno-smmu + - const: qcom,smmu-v2 - description: Marvell SoCs implementing "arm,mmu-500" items: - const: marvell,ap806-smmu-500