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
