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@vger.kernel.org> # 4.19
Fixes: 7e5700ae64f6 ("dt-bindings: Documentation for qcom, llcc")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
.../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
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sc8180x-llcc
+ - qcom,sc8280xp-llcc
+ then:
+ properties:
+ reg:
+ items:
+ - description: LLCC0 base register region
+ - description: LLCC1 base register region
+ - description: LLCC2 base register region
+ - description: LLCC3 base register region
+ - description: LLCC4 base register region
+ - description: LLCC5 base register region
+ - description: LLCC6 base register region
+ - description: LLCC7 base register region
+ - description: LLCC broadcast base register region
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sdm845-llcc
+ - qcom,sm8150-llcc
+ - qcom,sm8250-llcc
+ - qcom,sm8350-llcc
+ - qcom,sm8450-llcc
+ then:
+ properties:
+ reg:
+ items:
+ - description: LLCC0 base register region
+ - description: LLCC1 base register region
+ - description: LLCC2 base register region
+ - description: LLCC3 base register region
+ - description: LLCC broadcast base register region
additionalProperties: false
@@ -56,9 +119,15 @@ examples:
- |
#include <dt-bindings/interrupt-controller/arm-gic.h>
- system-cache-controller@1100000 {
- compatible = "qcom,sdm845-llcc";
- reg = <0x1100000 0x200000>, <0x1300000 0x50000> ;
- reg-names = "llcc_base", "llcc_broadcast_base";
- interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ system-cache-controller@1100000 {
+ compatible = "qcom,sdm845-llcc";
+ reg = <0 0x01100000 0 0x50000>, <0 0x01180000 0 0x50000>,
+ <0 0x01200000 0 0x50000>, <0 0x01280000 0 0x50000>,
+ <0 0x01300000 0 0x50000>;
+ interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
+ };
};
--
2.25.1On 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@vger.kernel.org> # 4.19
> Fixes: 7e5700ae64f6 ("dt-bindings: Documentation for qcom, llcc")
Also here there is no bug explained enough justifying backport of
bindings. Additionally, you effectively cannot backport bindings -
bindings from previous versions are still defining the ABI.
Best regards,
Krzysztof
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@vger.kernel.org> # 4.19
> Fixes: 7e5700ae64f6 ("dt-bindings: Documentation for qcom, llcc")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> .../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.
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - qcom,sc8180x-llcc
> + - qcom,sc8280xp-llcc
> + then:
> + properties:
> + reg:
> + items:
> + - description: LLCC0 base register region
> + - description: LLCC1 base register region
> + - description: LLCC2 base register region
> + - description: LLCC3 base register region
> + - description: LLCC4 base register region
> + - description: LLCC5 base register region
> + - description: LLCC6 base register region
> + - description: LLCC7 base register region
> + - description: LLCC broadcast base register region
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - qcom,sdm845-llcc
> + - qcom,sm8150-llcc
> + - qcom,sm8250-llcc
> + - qcom,sm8350-llcc
> + - qcom,sm8450-llcc
> + then:
> + properties:
> + reg:
> + items:
> + - description: LLCC0 base register region
> + - description: LLCC1 base register region
> + - description: LLCC2 base register region
> + - description: LLCC3 base register region
> + - description: LLCC broadcast base register region
>
> additionalProperties: false
>
> @@ -56,9 +119,15 @@ examples:
> - |
> #include <dt-bindings/interrupt-controller/arm-gic.h>
>
> - system-cache-controller@1100000 {
> - compatible = "qcom,sdm845-llcc";
> - reg = <0x1100000 0x200000>, <0x1300000 0x50000> ;
> - reg-names = "llcc_base", "llcc_broadcast_base";
> - interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + system-cache-controller@1100000 {
> + compatible = "qcom,sdm845-llcc";
Inconsistent indentation for DTS example. Use 4 spaces for it.
> + reg = <0 0x01100000 0 0x50000>, <0 0x01180000 0 0x50000>,
> + <0 0x01200000 0 0x50000>, <0 0x01280000 0 0x50000>,
> + <0 0x01300000 0 0x50000>;
> + interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
> + };
> };
Best regards,
Krzysztof
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@vger.kernel.org> # 4.19
> > Fixes: 7e5700ae64f6 ("dt-bindings: Documentation for qcom, llcc")
> > Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > .../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?
Thanks,
Mani
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,sc8180x-llcc
> > + - qcom,sc8280xp-llcc
> > + then:
> > + properties:
> > + reg:
> > + items:
> > + - description: LLCC0 base register region
> > + - description: LLCC1 base register region
> > + - description: LLCC2 base register region
> > + - description: LLCC3 base register region
> > + - description: LLCC4 base register region
> > + - description: LLCC5 base register region
> > + - description: LLCC6 base register region
> > + - description: LLCC7 base register region
> > + - description: LLCC broadcast base register region
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,sdm845-llcc
> > + - qcom,sm8150-llcc
> > + - qcom,sm8250-llcc
> > + - qcom,sm8350-llcc
> > + - qcom,sm8450-llcc
> > + then:
> > + properties:
> > + reg:
> > + items:
> > + - description: LLCC0 base register region
> > + - description: LLCC1 base register region
> > + - description: LLCC2 base register region
> > + - description: LLCC3 base register region
> > + - description: LLCC broadcast base register region
> >
> > additionalProperties: false
> >
> > @@ -56,9 +119,15 @@ examples:
> > - |
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> >
> > - system-cache-controller@1100000 {
> > - compatible = "qcom,sdm845-llcc";
> > - reg = <0x1100000 0x200000>, <0x1300000 0x50000> ;
> > - reg-names = "llcc_base", "llcc_broadcast_base";
> > - interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
> > + soc {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > +
> > + system-cache-controller@1100000 {
> > + compatible = "qcom,sdm845-llcc";
>
> Inconsistent indentation for DTS example. Use 4 spaces for it.
>
> > + reg = <0 0x01100000 0 0x50000>, <0 0x01180000 0 0x50000>,
> > + <0 0x01200000 0 0x50000>, <0 0x01280000 0 0x50000>,
> > + <0 0x01300000 0 0x50000>;
> > + interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
> > + };
> > };
>
> Best regards,
> Krzysztof
>
--
மணிவண்ணன் சதாசிவம்
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@vger.kernel.org> # 4.19
>>> Fixes: 7e5700ae64f6 ("dt-bindings: Documentation for qcom, llcc")
>>> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>> .../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
© 2016 - 2026 Red Hat, Inc.