[PATCH] dt-bindings: host1x: Add missing 'dma-coherent'

[PATCH] dt-bindings: host1x: Add missing 'dma-coherent'

[Jon Hunter]

The dtbs_check reports that the 'dma-coherent' property is "unevaluated
and invalid" for the host1x@13e00000 device on Tegra194 and Tegra234
platforms. Fix this by updating the dt-binding document for host1x to
add the 'dma-coherent' property for these devices.

Fixes: 361238cdc525 ("arm64: tegra: Mark host1x as dma-coherent on Tegra194/234")
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 .../bindings/display/tegra/nvidia,tegra20-host1x.yaml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
index 94c5242c03b2..3563378a01af 100644
--- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
+++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
@@ -177,6 +177,15 @@ allOf:
 
       required:
         - reg-names
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - nvidia,tegra194-host1x
+    then:
+      properties:
+        dma-coherent: true
   - if:
       properties:
         compatible:
@@ -226,6 +235,8 @@ allOf:
             use. Should be a mapping of IDs 0..n to IOMMU entries corresponding to
             usable stream IDs.
 
+        dma-coherent: true
+
       required:
         - reg-names
 
-- 
2.34.1

Re: [PATCH] dt-bindings: host1x: Add missing 'dma-coherent'

[Krzysztof Kozlowski]

On 09/04/2024 10:23, Jon Hunter wrote:
> The dtbs_check reports that the 'dma-coherent' property is "unevaluated
> and invalid" for the host1x@13e00000 device on Tegra194 and Tegra234
> platforms. Fix this by updating the dt-binding document for host1x to
> add the 'dma-coherent' property for these devices.

That's not really proper reason. What if DTS is wrong? The reason could
be if device is actually DMA coherent...

> 
> Fixes: 361238cdc525 ("arm64: tegra: Mark host1x as dma-coherent on Tegra194/234")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  .../bindings/display/tegra/nvidia,tegra20-host1x.yaml | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> index 94c5242c03b2..3563378a01af 100644
> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> @@ -177,6 +177,15 @@ allOf:
>  
>        required:
>          - reg-names
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra194-host1x
> +    then:
> +      properties:
> +        dma-coherent: true

Do not define properties in allOf. Put it in top-level. If not all
devices are DMA coherent, you can disallow it for certain variants (:false).

Best regards,
Krzysztof

Re: [PATCH] dt-bindings: host1x: Add missing 'dma-coherent'

[Jon Hunter]

On 10/04/2024 07:18, Krzysztof Kozlowski wrote:
> On 09/04/2024 10:23, Jon Hunter wrote:
>> The dtbs_check reports that the 'dma-coherent' property is "unevaluated
>> and invalid" for the host1x@13e00000 device on Tegra194 and Tegra234
>> platforms. Fix this by updating the dt-binding document for host1x to
>> add the 'dma-coherent' property for these devices.
> 
> That's not really proper reason. What if DTS is wrong? The reason could
> be if device is actually DMA coherent...

In this case the DTS is correct. I guess I should have been more 
explicit about that.

>> Fixes: 361238cdc525 ("arm64: tegra: Mark host1x as dma-coherent on Tegra194/234")
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>   .../bindings/display/tegra/nvidia,tegra20-host1x.yaml | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
>> index 94c5242c03b2..3563378a01af 100644
>> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
>> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
>> @@ -177,6 +177,15 @@ allOf:
>>   
>>         required:
>>           - reg-names
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - nvidia,tegra194-host1x
>> +    then:
>> +      properties:
>> +        dma-coherent: true
> 
> Do not define properties in allOf. Put it in top-level. If not all
> devices are DMA coherent, you can disallow it for certain variants (:false).


So for host1x we currently have the following devices supported ...

properties:
   compatible:
     oneOf:
       - enum:
           - nvidia,tegra20-host1x
           - nvidia,tegra30-host1x
           - nvidia,tegra114-host1x
           - nvidia,tegra124-host1x
           - nvidia,tegra210-host1x
           - nvidia,tegra186-host1x
           - nvidia,tegra194-host1x
           - nvidia,tegra234-host1x

       - items:
           - const: nvidia,tegra132-host1x
           - const: nvidia,tegra124-host1x


Now only the Tegra194 and Tegra234 are coherent (which are the latest 
devices). So rather than add this to the top and then filter out all 
those that are not supported, I opted for the above because there is 
only 2 devices that need this. Admittedly, as much as I like the yaml 
bindings, for things like this, it is not really clear which is the best 
way to handle, so appreciate the guidance.

Jon

-- 
nvpublic

Re: [PATCH] dt-bindings: host1x: Add missing 'dma-coherent'

[Krzysztof Kozlowski]

On 11/04/2024 12:09, Jon Hunter wrote:
> 
> On 10/04/2024 07:18, Krzysztof Kozlowski wrote:
>> On 09/04/2024 10:23, Jon Hunter wrote:
>>> The dtbs_check reports that the 'dma-coherent' property is "unevaluated
>>> and invalid" for the host1x@13e00000 device on Tegra194 and Tegra234
>>> platforms. Fix this by updating the dt-binding document for host1x to
>>> add the 'dma-coherent' property for these devices.
>>
>> That's not really proper reason. What if DTS is wrong? The reason could
>> be if device is actually DMA coherent...
> 
> In this case the DTS is correct. I guess I should have been more 
> explicit about that.
> 
>>> Fixes: 361238cdc525 ("arm64: tegra: Mark host1x as dma-coherent on Tegra194/234")
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>   .../bindings/display/tegra/nvidia,tegra20-host1x.yaml | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
>>> index 94c5242c03b2..3563378a01af 100644
>>> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
>>> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
>>> @@ -177,6 +177,15 @@ allOf:
>>>   
>>>         required:
>>>           - reg-names
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - nvidia,tegra194-host1x
>>> +    then:
>>> +      properties:
>>> +        dma-coherent: true
>>
>> Do not define properties in allOf. Put it in top-level. If not all
>> devices are DMA coherent, you can disallow it for certain variants (:false).
> 
> 
> So for host1x we currently have the following devices supported ...
> 
> properties:
>    compatible:
>      oneOf:
>        - enum:
>            - nvidia,tegra20-host1x
>            - nvidia,tegra30-host1x
>            - nvidia,tegra114-host1x
>            - nvidia,tegra124-host1x
>            - nvidia,tegra210-host1x
>            - nvidia,tegra186-host1x
>            - nvidia,tegra194-host1x
>            - nvidia,tegra234-host1x
> 
>        - items:
>            - const: nvidia,tegra132-host1x
>            - const: nvidia,tegra124-host1x
> 
> 
> Now only the Tegra194 and Tegra234 are coherent (which are the latest 
> devices). So rather than add this to the top and then filter out all 
> those that are not supported, I opted for the above because there is 
> only 2 devices that need this. Admittedly, as much as I like the yaml 
> bindings, for things like this, it is not really clear which is the best 
> way to handle, so appreciate the guidance.

The way to handle is that you must define properties top-level.

For simplification you could keep the if which duplicates the
dma-coherent:true but add else: which forbids it

if:
 ...
  then:
    properties:
      dma-coherent: true
  else:
    properties:
      dma-coherent: false

Or just ignore the problem. Binding is not 1-to-1 with DTS.

Best regards,
Krzysztof

Re: [PATCH] dt-bindings: host1x: Add missing 'dma-coherent'

[Rob Herring]

On Thu, Apr 11, 2024 at 5:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 11/04/2024 12:09, Jon Hunter wrote:
> >
> > On 10/04/2024 07:18, Krzysztof Kozlowski wrote:
> >> On 09/04/2024 10:23, Jon Hunter wrote:
> >>> The dtbs_check reports that the 'dma-coherent' property is "unevaluated
> >>> and invalid" for the host1x@13e00000 device on Tegra194 and Tegra234
> >>> platforms. Fix this by updating the dt-binding document for host1x to
> >>> add the 'dma-coherent' property for these devices.
> >>
> >> That's not really proper reason. What if DTS is wrong? The reason could
> >> be if device is actually DMA coherent...
> >
> > In this case the DTS is correct. I guess I should have been more
> > explicit about that.
> >
> >>> Fixes: 361238cdc525 ("arm64: tegra: Mark host1x as dma-coherent on Tegra194/234")
> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> >>> ---
> >>>   .../bindings/display/tegra/nvidia,tegra20-host1x.yaml | 11 +++++++++++
> >>>   1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> >>> index 94c5242c03b2..3563378a01af 100644
> >>> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> >>> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> >>> @@ -177,6 +177,15 @@ allOf:
> >>>
> >>>         required:
> >>>           - reg-names
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            enum:
> >>> +              - nvidia,tegra194-host1x
> >>> +    then:
> >>> +      properties:
> >>> +        dma-coherent: true
> >>
> >> Do not define properties in allOf. Put it in top-level. If not all
> >> devices are DMA coherent, you can disallow it for certain variants (:false).
> >
> >
> > So for host1x we currently have the following devices supported ...
> >
> > properties:
> >    compatible:
> >      oneOf:
> >        - enum:
> >            - nvidia,tegra20-host1x
> >            - nvidia,tegra30-host1x
> >            - nvidia,tegra114-host1x
> >            - nvidia,tegra124-host1x
> >            - nvidia,tegra210-host1x
> >            - nvidia,tegra186-host1x
> >            - nvidia,tegra194-host1x
> >            - nvidia,tegra234-host1x
> >
> >        - items:
> >            - const: nvidia,tegra132-host1x
> >            - const: nvidia,tegra124-host1x
> >
> >
> > Now only the Tegra194 and Tegra234 are coherent (which are the latest
> > devices). So rather than add this to the top and then filter out all
> > those that are not supported, I opted for the above because there is
> > only 2 devices that need this. Admittedly, as much as I like the yaml
> > bindings, for things like this, it is not really clear which is the best
> > way to handle, so appreciate the guidance.
>
> The way to handle is that you must define properties top-level.
>
> For simplification you could keep the if which duplicates the
> dma-coherent:true but add else: which forbids it
>
> if:
>  ...
>   then:
>     properties:
>       dma-coherent: true

I think just 'then: true' will work here.

Technically, json-schema allows:

if:
  ...
else:
  ...

Actually any combination of the 3 keywords present is valid, but we've
disallowed the above and other nonsense ones in the meta-schema. We
can always relax things if there's a need.

Rob

Re: [PATCH] dt-bindings: host1x: Add missing 'dma-coherent'

[Rob Herring]

On Thu, Apr 11, 2024 at 5:09 AM Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 10/04/2024 07:18, Krzysztof Kozlowski wrote:
> > On 09/04/2024 10:23, Jon Hunter wrote:
> >> The dtbs_check reports that the 'dma-coherent' property is "unevaluated
> >> and invalid" for the host1x@13e00000 device on Tegra194 and Tegra234
> >> platforms. Fix this by updating the dt-binding document for host1x to
> >> add the 'dma-coherent' property for these devices.
> >
> > That's not really proper reason. What if DTS is wrong? The reason could
> > be if device is actually DMA coherent...
>
> In this case the DTS is correct. I guess I should have been more
> explicit about that.
>
> >> Fixes: 361238cdc525 ("arm64: tegra: Mark host1x as dma-coherent on Tegra194/234")
> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> >> ---
> >>   .../bindings/display/tegra/nvidia,tegra20-host1x.yaml | 11 +++++++++++
> >>   1 file changed, 11 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> >> index 94c5242c03b2..3563378a01af 100644
> >> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> >> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> >> @@ -177,6 +177,15 @@ allOf:
> >>
> >>         required:
> >>           - reg-names
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            enum:
> >> +              - nvidia,tegra194-host1x
> >> +    then:
> >> +      properties:
> >> +        dma-coherent: true
> >
> > Do not define properties in allOf. Put it in top-level. If not all
> > devices are DMA coherent, you can disallow it for certain variants (:false).
>
>
> So for host1x we currently have the following devices supported ...
>
> properties:
>    compatible:
>      oneOf:
>        - enum:
>            - nvidia,tegra20-host1x
>            - nvidia,tegra30-host1x
>            - nvidia,tegra114-host1x
>            - nvidia,tegra124-host1x
>            - nvidia,tegra210-host1x
>            - nvidia,tegra186-host1x
>            - nvidia,tegra194-host1x
>            - nvidia,tegra234-host1x
>
>        - items:
>            - const: nvidia,tegra132-host1x
>            - const: nvidia,tegra124-host1x
>
>
> Now only the Tegra194 and Tegra234 are coherent (which are the latest
> devices). So rather than add this to the top and then filter out all
> those that are not supported, I opted for the above because there is
> only 2 devices that need this. Admittedly, as much as I like the yaml
> bindings, for things like this, it is not really clear which is the best
> way to handle, so appreciate the guidance.

I would say how you have it is fine, but that would only be for common
boolean properties with no possible constraints. Having that exception
would make the preferences less clear I think.

Rob

Re: [PATCH] dt-bindings: host1x: Add missing 'dma-coherent'

[Krzysztof Kozlowski]

On 09/04/2024 10:23, Jon Hunter wrote:
> The dtbs_check reports that the 'dma-coherent' property is "unevaluated
> and invalid" for the host1x@13e00000 device on Tegra194 and Tegra234
> platforms. Fix this by updating the dt-binding document for host1x to
> add the 'dma-coherent' property for these devices.
> 
> Fixes: 361238cdc525 ("arm64: tegra: Mark host1x as dma-coherent on Tegra194/234")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---

With mentioning in commit msg that these devices are actually
dma-coherent (and after Rob's explanation):

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof