[v3,1/3] dt-bindings: interconnect: Remove required reg field

Message ID 20221026190520.4004264-2-quic_molvera@quicinc.com
State New
Headers
Series Add interconnect support for QDU1000/QRU1000 SoCs |

Commit Message

Melody Olvera Oct. 26, 2022, 7:05 p.m. UTC
  Many of the *-virt compatible devices do not have a reg field
so remove it as required from the bindings.

Signed-off-by: Melody Olvera <quic_molvera@quicinc.com>
---
 Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml | 1 -
 1 file changed, 1 deletion(-)
  

Comments

Krzysztof Kozlowski Oct. 27, 2022, 3:29 p.m. UTC | #1
On 26/10/2022 15:05, Melody Olvera wrote:
> Many of the *-virt compatible devices do not have a reg field
> so remove it as required from the bindings.

and some virt have it... This should be probably separate binding or if
the list is small - allOf:if:then.

Anyway you need to resend everything to Cc all maintainers, not some subset.

Best regards,
Krzysztof
  
Melody Olvera Oct. 31, 2022, 11:29 p.m. UTC | #2
On 10/27/2022 8:29 AM, Krzysztof Kozlowski wrote:
> On 26/10/2022 15:05, Melody Olvera wrote:
>> Many of the *-virt compatible devices do not have a reg field
>> so remove it as required from the bindings.
> and some virt have it... This should be probably separate binding or if
> the list is small - allOf:if:then.
I attempted this; however I'm still seeing failures in dtb_check. I've added this
to the binding; does this look correct?
 allOf:
   - $ref: qcom,rpmh-common.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,qdu1000-clk-virt
+              - qcom,qdu1000-mc-virt
+
+    then:
+      required:
+        - compatible

>
> Anyway you need to resend everything to Cc all maintainers, not some subset.
Discussed earlier.

Thanks,
Melody
  
Krzysztof Kozlowski Nov. 2, 2022, 9:11 p.m. UTC | #3
On 31/10/2022 19:29, Melody Olvera wrote:
> 
> 
> On 10/27/2022 8:29 AM, Krzysztof Kozlowski wrote:
>> On 26/10/2022 15:05, Melody Olvera wrote:
>>> Many of the *-virt compatible devices do not have a reg field
>>> so remove it as required from the bindings.
>> and some virt have it... This should be probably separate binding or if
>> the list is small - allOf:if:then.
> I attempted this; however I'm still seeing failures in dtb_check. I've added this
> to the binding; does this look correct?
>  allOf:
>    - $ref: qcom,rpmh-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,qdu1000-clk-virt
> +              - qcom,qdu1000-mc-virt
> +
> +    then:
> +      required:
> +        - compatible

No, because we talk about reg, not compatible. You should not require
reg instead for some compatibles... but then the schema is getting
complicated.

It's difficult to give you recommendation because I do not know what are
all these "virt" interconnects. Why some have unit address, why some do not?

Best regards,
Krzysztof
  
Georgi Djakov Nov. 7, 2022, 2:36 p.m. UTC | #4
Hi,

On 2.11.22 23:11, Krzysztof Kozlowski wrote:
> On 31/10/2022 19:29, Melody Olvera wrote:
>>
>>
>> On 10/27/2022 8:29 AM, Krzysztof Kozlowski wrote:
>>> On 26/10/2022 15:05, Melody Olvera wrote:
>>>> Many of the *-virt compatible devices do not have a reg field
>>>> so remove it as required from the bindings.
>>> and some virt have it... This should be probably separate binding or if
>>> the list is small - allOf:if:then.
>> I attempted this; however I'm still seeing failures in dtb_check. I've added this
>> to the binding; does this look correct?
>>   allOf:
>>     - $ref: qcom,rpmh-common.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,qdu1000-clk-virt
>> +              - qcom,qdu1000-mc-virt
>> +
>> +    then:
>> +      required:
>> +        - compatible
> 
> No, because we talk about reg, not compatible. You should not require
> reg instead for some compatibles... but then the schema is getting
> complicated.
> 
> It's difficult to give you recommendation because I do not know what are
> all these "virt" interconnects. Why some have unit address, why some do not?

My understanding is that the "reg" property is required for the NoCs that have
registers for controlling the QoS settings for the ports from Linux side.
Other NoCs might be controlled by some remote processor and direct access from
Linux may not be possible, so they do not have unit address and are outside of
the soc DT node.
Do we need to strictly define when exactly the "reg" property is required,
can't we just mark it as optional?

Thanks,
Georgi
  
Krzysztof Kozlowski Nov. 7, 2022, 6:28 p.m. UTC | #5
On 07/11/2022 15:36, Georgi Djakov wrote:
> Hi,
> 
> On 2.11.22 23:11, Krzysztof Kozlowski wrote:
>> On 31/10/2022 19:29, Melody Olvera wrote:
>>>
>>>
>>> On 10/27/2022 8:29 AM, Krzysztof Kozlowski wrote:
>>>> On 26/10/2022 15:05, Melody Olvera wrote:
>>>>> Many of the *-virt compatible devices do not have a reg field
>>>>> so remove it as required from the bindings.
>>>> and some virt have it... This should be probably separate binding or if
>>>> the list is small - allOf:if:then.
>>> I attempted this; however I'm still seeing failures in dtb_check. I've added this
>>> to the binding; does this look correct?
>>>   allOf:
>>>     - $ref: qcom,rpmh-common.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,qdu1000-clk-virt
>>> +              - qcom,qdu1000-mc-virt
>>> +
>>> +    then:
>>> +      required:
>>> +        - compatible
>>
>> No, because we talk about reg, not compatible. You should not require
>> reg instead for some compatibles... but then the schema is getting
>> complicated.
>>
>> It's difficult to give you recommendation because I do not know what are
>> all these "virt" interconnects. Why some have unit address, why some do not?
> 
> My understanding is that the "reg" property is required for the NoCs that have
> registers for controlling the QoS settings for the ports from Linux side.
> Other NoCs might be controlled by some remote processor and direct access from
> Linux may not be possible, so they do not have unit address and are outside of
> the soc DT node.
> Do we need to strictly define when exactly the "reg" property is required,
> can't we just mark it as optional?

It's preferred to make it strictly required or not allowed, so the
bindings are specific. This also allows to validate for mistakes. It
would be a bit different case if such test for req would make the
bindings complicated. I think it's not the case because we could just
split the bindings into two files:
1. One for controlled by AP, with reg.
2. One for controller by remote processors, without reg.

Best regards,
Krzysztof
  
Melody Olvera Nov. 7, 2022, 10:46 p.m. UTC | #6
On 11/7/2022 10:28 AM, Krzysztof Kozlowski wrote:
> On 07/11/2022 15:36, Georgi Djakov wrote:
>> Hi,
>>
>> On 2.11.22 23:11, Krzysztof Kozlowski wrote:
>>> On 31/10/2022 19:29, Melody Olvera wrote:
>>>>
>>>> On 10/27/2022 8:29 AM, Krzysztof Kozlowski wrote:
>>>>> On 26/10/2022 15:05, Melody Olvera wrote:
>>>>>> Many of the *-virt compatible devices do not have a reg field
>>>>>> so remove it as required from the bindings.
>>>>> and some virt have it... This should be probably separate binding or if
>>>>> the list is small - allOf:if:then.
>>>> I attempted this; however I'm still seeing failures in dtb_check. I've added this
>>>> to the binding; does this look correct?
>>>>   allOf:
>>>>     - $ref: qcom,rpmh-common.yaml#
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - qcom,qdu1000-clk-virt
>>>> +              - qcom,qdu1000-mc-virt
>>>> +
>>>> +    then:
>>>> +      required:
>>>> +        - compatible
>>> No, because we talk about reg, not compatible. You should not require
>>> reg instead for some compatibles... but then the schema is getting
>>> complicated.
>>>
>>> It's difficult to give you recommendation because I do not know what are
>>> all these "virt" interconnects. Why some have unit address, why some do not?
>> My understanding is that the "reg" property is required for the NoCs that have
>> registers for controlling the QoS settings for the ports from Linux side.
>> Other NoCs might be controlled by some remote processor and direct access from
>> Linux may not be possible, so they do not have unit address and are outside of
>> the soc DT node.
>> Do we need to strictly define when exactly the "reg" property is required,
>> can't we just mark it as optional?
> It's preferred to make it strictly required or not allowed, so the
> bindings are specific. This also allows to validate for mistakes. It
> would be a bit different case if such test for req would make the
> bindings complicated. I think it's not the case because we could just
> split the bindings into two files:
> 1. One for controlled by AP, with reg.
> 2. One for controller by remote processors, without reg.
>
Sounds good. Will drop this change and add a new binding document.

Thanks,
Melody
  

Patch

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
index a429a1ed1006..d9cdba15b4f4 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
@@ -137,7 +137,6 @@  properties:
 
 required:
   - compatible
-  - reg
 
 unevaluatedProperties: false