Re: [RFC PATCH 1/9] dt-bindings: arm-smmu: Add missing Qualcomm SMMU compatibles

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 22/10/2022 05:17, Dmitry Baryshkov wrote:
> On 22/10/2022 03:59, Krzysztof Kozlowski wrote:
>> On 21/10/2022 12:55, Dmitry Baryshkov wrote:
>>> Add missing compatibles used for Adreno SMMU on sc7280 and sm8450
>>> platforms and for the Qualcomm v2 SMMU used on SDM630 platform.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>>> index 9066e6df1ba1..34ee33a62ba5 100644
>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>>> @@ -28,6 +28,7 @@ properties:
>>>             - enum:
>>>                 - qcom,msm8996-smmu-v2
>>>                 - qcom,msm8998-smmu-v2
>>> +              - qcom,sdm630-smmu-v2
>>
>> So qcom,adreno-smmu is not compatible with Adreno? See below.
>>
>>>             - const: qcom,smmu-v2
>>>   
>>>         - description: Qcom SoCs implementing "arm,mmu-500"
>>> @@ -48,10 +49,20 @@ properties:
>>>                 - qcom,sm8350-smmu-500
>>>                 - qcom,sm8450-smmu-500
>>>             - const: arm,mmu-500
>>> +
>>> +      - description: Qcom Adreno GPUs implementing "arm,smmu-500"
>>> +        items:
>>> +          - enum:
>>> +              - qcom,sc7280-smmu-500
>>> +              - qcom,sm8250-smmu-500
>>> +          - const: qcom,adreno-smmu
>>> +          - const: arm,mmu-500
>>>         - description: Qcom Adreno GPUs implementing "arm,smmu-v2"
>>>           items:
>>>             - enum:
>>> +              - qcom,msm8996-smmu-v2
>>>                 - qcom,sc7180-smmu-v2
>>> +              - qcom,sdm630-smmu-v2
>>
>> This does not look correct. The same compatible should not be present in
>> two different setups.
>>
>> If qcom,msm8996-smmu-v2 is compatible with qcom,adreno-smmu, then your
>> first hunk is not correct.
> 
> Currently the qcom,adreno-smmu compat string is used as a flag, telling 
> the kernel that this SMMU instance needs some special setup to work with 
> Adreno GPU driver

Indeed, I see the usage in DTS,

> 
> For example, we have the following compat lists in the existing DT files:
> - "qcom,msm8996-smmu-v2", "qcom,adreno-smmu", "qcom,smmu-v2"
> - "qcom,msm8996-smmu-v2", "qcom,smmu-v2" // not handled by arm-qcom-smmu
> 
> - "qcom,sdm630-smmu-v2", "qcom,adreno-smmu", "qcom,smmu-v2"
> - "qcom,sdm630-smmu-v2", "qcom,smmu-v2"
> 
> - "qcom,sdm845-smmu-v2", "qcom,adreno-smmu", "qcom,smmu-v2"
> - "qcom,sdm845-smmu-500", "arm,mmu-500"
> - "qcom,sdm845-smmu-v2", "qcom,smmu-v2" // special setup used on Cheza
> 
> - "qcom,sm8250-smmu-500", "qcom,adreno-smmu", "arm,mmu-500"
> - "qcom,sm8250-smmu-500", "arm,mmu-500"
> 
> 
> As we are trying to refactor the IOMMU bindings, what would be your 
> recommendation?
> 
> To introduce minimal changes, I wanted to have the following lists:
> - "qcom,SOC-smmu-500", "qcom,adreno-smmu", "qcom,smmu-500", "arm,mmu-500"
> 
> - "qcom,SOC-smmu-500", "qcom,smmu-500", "arm,mmu-500"
> 
> However maybe you would prefer the following model:
> 
> - "qcom,SOC-adreno-smmu-500", "qcom,adreno-smmu-500", "arm,mmu-500"
> - "qcom,SOC-smmu-500", "qcom,smmu-500", "arm,mmu-500"

If we started from scratch, I would prefer this one, however as DTSes
are already using your previous method, It's fine.

It's a bit confusing to have most specific compatible followed by
different fallbacks, but we already have few cases for this (e.g.
Renesas boards), so I guess it is fine here as well. At the end entire
compatible list uniquely describes the hardware.

> 
> 
> Or:
> - "qcom,SOC-smmu-500", "qcom,smmu-500", "arm,mmu-500" + 
> 'qcom,adreno-smmu' flag/property?
> 
> 
>>
>>>                 - qcom,sdm845-smmu-v2
>>>             - const: qcom,adreno-smmu
>>>             - const: qcom,smmu-v2
>>
>> Best regards,
>> Krzysztof
>>
> 

Best regards,
Krzysztof