diff mbox series

dt-bindings: host1x: Add missing 'dma-coherent'

Message ID 20240409082324.9928-1-jonathanh@nvidia.com
State Accepted
Headers show
Series dt-bindings: host1x: Add missing 'dma-coherent' | expand

Commit Message

Jon Hunter April 9, 2024, 8:23 a.m. UTC
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(+)

Comments

Krzysztof Kozlowski April 10, 2024, 6:18 a.m. UTC | #1
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
Jon Hunter April 11, 2024, 10:09 a.m. UTC | #2
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
Krzysztof Kozlowski April 11, 2024, 10:43 a.m. UTC | #3
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
Rob Herring (Arm) April 11, 2024, 2:36 p.m. UTC | #4
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
Rob Herring (Arm) April 11, 2024, 2:43 p.m. UTC | #5
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
Krzysztof Kozlowski April 12, 2024, 5:25 a.m. UTC | #6
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
diff mbox series

Patch

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