Re: [PATCH V2] dt-bindings: clock: qcom: Remove required-opps from required list on SM8650

[Jagadeesh Kona <quic_jkona@quicinc.com>]

On 7/26/2024 8:28 PM, Krzysztof Kozlowski wrote:
> On 26/07/2024 16:30, Jagadeesh Kona wrote:
>>
>>
>> On 7/24/2024 1:38 PM, Krzysztof Kozlowski wrote:
>>> On 20/07/2024 07:28, Jagadeesh Kona wrote:
>>>> On SM8650, the minimum voltage corner supported on MMCX from cmd-db is
>>>> sufficient for clock controllers to operate and there is no need to specify
>>>> the required-opps. Hence remove the required-opps property from the list of
>>>> required properties for SM8650 camcc and videocc bindings.
>>>>
>>>> This fixes:
>>>> arch/arm64/boot/dts/qcom/sm8650-mtp.dtb: clock-controller@aaf0000:
>>>> 'required-opps' is a required property
>>>>
>>>> arch/arm64/boot/dts/qcom/sm8650-mtp.dtb: clock-controller@ade0000:
>>>> 'required-opps' is a required property
>>>>
>>>> Fixes: a6a61b9701d1 ("dt-bindings: clock: qcom: Add SM8650 video clock controller")
>>>> Fixes: 1ae3f0578e0e ("dt-bindings: clock: qcom: Add SM8650 camera clock controller")
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202407070147.C9c3oTqS-lkp@intel.com/
>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>> ---
>>>> Changes in V2:
>>>>    - Made required: conditional and dropped required-opps from it only for SM8650 platform
>>>>    - Dropped Krzysztof Acked-by tag due to above changes
>>>>    - Link to V1: https://lore.kernel.org/all/20240708130836.19273-1-quic_jkona@quicinc.com/#r
>>>>
>>>> .../bindings/clock/qcom,sm8450-camcc.yaml     | 26 +++++++++++++------
>>>>    .../bindings/clock/qcom,sm8450-videocc.yaml   | 25 +++++++++++++-----
>>>>    2 files changed, 36 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
>>>> index f58edfc10f4c..8698c801ed11 100644
>>>> --- a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
>>>> +++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
>>>> @@ -21,9 +21,6 @@ description: |
>>>>        include/dt-bindings/clock/qcom,sm8650-camcc.h
>>>>        include/dt-bindings/clock/qcom,x1e80100-camcc.h
>>>>    
>>>> -allOf:
>>>> -  - $ref: qcom,gcc.yaml#
>>>> -
>>>>    properties:
>>>>      compatible:
>>>>        enum:
>>>> @@ -53,11 +50,24 @@ properties:
>>>>      reg:
>>>>        maxItems: 1
>>>>    
>>>> -required:
>>>
>>> You cannot remove required block.
>>>
>>>> -  - compatible
>>>> -  - clocks
>>>> -  - power-domains
>>>> -  - required-opps
>>>> +allOf:
>>>> +  - $ref: qcom,gcc.yaml#
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            const: qcom,sm8650-camcc
>>>> +    then:
>>>> +      required:
>>>> +        - compatible
>>>> +        - clocks
>>>> +        - power-domains
>>>> +    else:
>>>> +      required:
>>>> +        - compatible
>>>> +        - clocks
>>>> +        - power-domains
>>>> +        - required-opps
>>>>    
>>>>    unevaluatedProperties: false
>>>>    
>>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>>>> index b2792b4bb554..2e5a061f33d6 100644
>>>> --- a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>>>> +++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>>>> @@ -40,15 +40,26 @@ properties:
>>>>        description:
>>>>          A phandle to an OPP node describing required MMCX performance point.
>>>>    
>>>> -required:
>>>
>>> No, you cannot remove required block.
>>>
>>> To clarify: there is almost no single binding using your style. Even if
>>> there is one, then 99 others are using it differently. Do not implement
>>> things entirely different than everyone else. This is the same for C
>>> code you send upstream. No difference here...
>>>
>>
>> Thanks Krzysztof for the explanation.
>>
>> Hi Dmitry,
>>
>> As we discussed during SM8650 camcc and videocc changes, the MMCX rail's
>> minimum voltage level from cmd-db is adequate for these clock
>> controllers to operate on SM8650. So, we removed the 'required-opps'
>> property from their DT nodes.
> 
> Not sure with whom you discuss. With Dmitry or me. Anyway, I said
> nothing about required-opps, but the "required:" block.
> 
>>
>> Although 'required-opps' will remain in the properties list, it’s not
>> mandatory to be present in 'required:' list, as it is dependent on
>> cmd-db minimum level. So, can I please go ahead and update these
>> bindings to remove 'required-opps' from the 'required:' list, as done in
>> v1 of this series.
>>
>> It seems unconventional to make 'required:' conditional based on the
>> platform type.
>>
> 
> Obviously. But nothing stops you - and there are plenty of examples - of
> requiring one particular property based on the variant.
> 

Thanks Krzysztof for the info.

I will update the change to remove only 'required-opps' from the 
required list and add it back wherever required based on the variant.

Thanks,
Jagadeesh

> Best regards,
> Krzysztof
>