Re: [PATCH v2 02/13] dt-bindings: arm: msm: Fix register regions used for LLCC banks

[Krzysztof Kozlowski]

On 13/12/2022 18:30, Manivannan Sadhasivam wrote:
> On Tue, Dec 13, 2022 at 05:24:45PM +0100, Krzysztof Kozlowski wrote:
>> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
>>> Register regions of the LLCC banks are located at separate addresses.
>>> Currently, the binding just lists the LLCC0 base address and specifies
>>> the size to cover all banks. This is not the correct approach since,
>>> there are holes and other registers located in between.
>>>
>>> So let's specify the base address of each LLCC bank and get rid of
>>> reg-names property as it is not needed anymore. It should be noted that
>>> the bank count differs for each SoC, so that also needs to be taken into
>>> account in the binding.
>>>
>>> Cc: <stable@xxxxxxxxxxxxxxx> # 4.19
>>> Fixes: 7e5700ae64f6 ("dt-bindings: Documentation for qcom, llcc")
>>> Reported-by: Parikshit Pareek <quic_ppareek@xxxxxxxxxxx>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
>>> ---
>>>  .../bindings/arm/msm/qcom,llcc.yaml           | 97 ++++++++++++++++---
>>>  1 file changed, 83 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
>>> index d1df49ffcc1b..260bc87629a7 100644
>>> --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
>>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
>>> @@ -33,14 +33,8 @@ properties:
>>>        - qcom,sm8550-llcc
>>>  
>>>    reg:
>>> -    items:
>>> -      - description: LLCC base register region
>>> -      - description: LLCC broadcast base register region
>>> -
>>> -  reg-names:
>>> -    items:
>>> -      - const: llcc_base
>>> -      - const: llcc_broadcast_base
>>> +    minItems: 2
>>> +    maxItems: 9
>>>  
>>>    interrupts:
>>>      maxItems: 1
>>> @@ -48,7 +42,76 @@ properties:
>>>  required:
>>>    - compatible
>>>    - reg
>>> -  - reg-names
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,sc7180-llcc
>>> +              - qcom,sm6350-llcc
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          items:
>>> +            - description: LLCC0 base register region
>>> +            - description: LLCC broadcast base register region
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,sc7280-llcc
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          items:
>>> +            - description: LLCC0 base register region
>>> +            - description: LLCC1 base register region
>>> +            - description: LLCC broadcast base register region
>>
>> This will break all existing users (all systems, bootloaders/firmwares),
>> so you need to explain that in commit msg - why breaking is allowed, who
>> is or is not going to be affected etc. Otherwise judging purely by
>> bindings this is an ABI break.
>>
>> Reason "This is not the correct approach since, there are holes and
>> other registers located in between." is not enough, because this
>> suggests previous approach was just not the best and you have something
>> better. Better is not a reason for ABI break.
>>
> 
> Maybe I need to reword the commit message a bit. But clearly the binding was
> wrong for rest of the SoCs other than SDM845 as the total size of the LLCC
> region includes registers of other peripherals like memory controller.
> 
> In that case, will you let the binding to be wrong or fix it?

Sure it needs fixing, but as I said you need to explain why breaking ABI
is okay and who/where is going to be affected.

Best regards,
Krzysztof