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

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

[Manivannan Sadhasivam]

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.1

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

[Krzysztof Kozlowski]

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

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

[Manivannan Sadhasivam]

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
> 

-- 
மணிவண்ணன் சதாசிவம்

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@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

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

[Krzysztof Kozlowski]

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")

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

[PATCH v2 03/13] arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks

[Manivannan Sadhasivam]

The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

Also, let's get rid of reg-names property as it is not needed anymore.
The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 5.4
Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 65032b94b46d..683b861e060d 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2132,8 +2132,9 @@ uart15: serial@a9c000 {
 
 		llcc: system-cache-controller@1100000 {
 			compatible = "qcom,sdm845-llcc";
-			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
+			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.1

Re: [PATCH v2 03/13] arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks

[Sai Prakash Ranjan]

On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.4
> Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 65032b94b46d..683b861e060d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2132,8 +2132,9 @@ uart15: serial@a9c000 {
>   
>   		llcc: system-cache-controller@1100000 {
>   			compatible = "qcom,sdm845-llcc";
> -			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
> +			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>;
>   		};
>   

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

Re: [PATCH v2 03/13] arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks

[Krzysztof Kozlowski]

On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.4

No, you cannot backport it. You will break users.

> Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 65032b94b46d..683b861e060d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2132,8 +2132,9 @@ uart15: serial@a9c000 {
>  
>  		llcc: system-cache-controller@1100000 {
>  			compatible = "qcom,sdm845-llcc";
> -			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";

Once property was made required, you cannot remove it. What if other
bindings user depends on it?

Please instead keep/update the reg-names and/or mark it as deprecated.
It must stay in DTS for some time.

Best regards,
Krzysztof

Re: [PATCH v2 03/13] arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks

[Manivannan Sadhasivam]

On Tue, Dec 13, 2022 at 05:27:45PM +0100, Krzysztof Kozlowski wrote:
> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> > The LLCC block has several banks each with a different base address
> > and holes in between. So it is not a correct approach to cover these
> > banks with a single offset/size. Instead, the individual bank's base
> > address needs to be specified in devicetree with the exact size.
> > 
> > Also, let's get rid of reg-names property as it is not needed anymore.
> > The driver is expected to parse the reg field based on index to get the
> > addresses of each LLCC banks.
> > 
> > Cc: <stable@vger.kernel.org> # 5.4
> 
> No, you cannot backport it. You will break users.
> 

If the driver change gets backported, it will break users, isn't it?

> > Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
> > Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 65032b94b46d..683b861e060d 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -2132,8 +2132,9 @@ uart15: serial@a9c000 {
> >  
> >  		llcc: system-cache-controller@1100000 {
> >  			compatible = "qcom,sdm845-llcc";
> > -			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
> > -			reg-names = "llcc_base", "llcc_broadcast_base";
> 
> Once property was made required, you cannot remove it. What if other
> bindings user depends on it?
> 
> Please instead keep/update the reg-names and/or mark it as deprecated.
> It must stay in DTS for some time.
> 

Fair enough. I will mark it as deprecated in binding and will keep it in dts.

Thanks,
Mani

> Best regards,
> Krzysztof
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 03/13] arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks

[Krzysztof Kozlowski]

On 13/12/2022 18:13, Manivannan Sadhasivam wrote:
> On Tue, Dec 13, 2022 at 05:27:45PM +0100, Krzysztof Kozlowski wrote:
>> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
>>> The LLCC block has several banks each with a different base address
>>> and holes in between. So it is not a correct approach to cover these
>>> banks with a single offset/size. Instead, the individual bank's base
>>> address needs to be specified in devicetree with the exact size.
>>>
>>> Also, let's get rid of reg-names property as it is not needed anymore.
>>> The driver is expected to parse the reg field based on index to get the
>>> addresses of each LLCC banks.
>>>
>>> Cc: <stable@vger.kernel.org> # 5.4
>>
>> No, you cannot backport it. You will break users.
>>
> 
> If the driver change gets backported, it will break users, isn't it?

Whether driver change gets backported or not - all out of tree kernel
users, other systems, firmwares/bootloaders are broken and backporting
driver piece will not fix it.

By this backport you mean that the change can go alone to v5.4 kernel
(you did not write here dependency on other backport) and I wonder if
v5.4 kernel works with this patch...

> 
>>> Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
>>> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> index 65032b94b46d..683b861e060d 100644
>>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> @@ -2132,8 +2132,9 @@ uart15: serial@a9c000 {
>>>  
>>>  		llcc: system-cache-controller@1100000 {
>>>  			compatible = "qcom,sdm845-llcc";
>>> -			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
>>> -			reg-names = "llcc_base", "llcc_broadcast_base";
>>
>> Once property was made required, you cannot remove it. What if other
>> bindings user depends on it?
>>
>> Please instead keep/update the reg-names and/or mark it as deprecated.
>> It must stay in DTS for some time.
>>
> 
> Fair enough. I will mark it as deprecated in binding and will keep it in dts.

Best regards,
Krzysztof

[PATCH v2 04/13] arm64: dts: qcom: sc7180: Remove reg-names property from LLCC node

[Manivannan Sadhasivam]

The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

On SC7180, there is only one LLCC bank available. So only change needed is
to remove the reg-names property from LLCC node to conform to the binding.

The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 5.6
Fixes: c831fa299996 ("arm64: dts: qcom: sc7180: Add Last level cache controller node")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index f71cf21a8dd8..b0d524bbf051 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -2759,7 +2759,6 @@ dc_noc: interconnect@9160000 {
 		system-cache-controller@9200000 {
 			compatible = "qcom,sc7180-llcc";
 			reg = <0 0x09200000 0 0x50000>, <0 0x09600000 0 0x50000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
 			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
-- 
2.25.1

Re: [PATCH v2 04/13] arm64: dts: qcom: sc7180: Remove reg-names property from LLCC node

[Sai Prakash Ranjan]

On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> On SC7180, there is only one LLCC bank available. So only change needed is
> to remove the reg-names property from LLCC node to conform to the binding.
> 
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.6
> Fixes: c831fa299996 ("arm64: dts: qcom: sc7180: Add Last level cache controller node")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sc7180.dtsi | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index f71cf21a8dd8..b0d524bbf051 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -2759,7 +2759,6 @@ dc_noc: interconnect@9160000 {
>   		system-cache-controller@9200000 {
>   			compatible = "qcom,sc7180-llcc";
>   			reg = <0 0x09200000 0 0x50000>, <0 0x09600000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
>   			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>   		};
>   

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

Re: [PATCH v2 04/13] arm64: dts: qcom: sc7180: Remove reg-names property from LLCC node

[Krzysztof Kozlowski]

On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> On SC7180, there is only one LLCC bank available. So only change needed is
> to remove the reg-names property from LLCC node to conform to the binding.
> 
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.6

Oh, no, there is no single bug here. Binding from v5.6+ (which cannot be
changed) required/defined such reg-names. This is neither a bug nor
possible to backport.

> Fixes: c831fa299996 ("arm64: dts: qcom: sc7180: Add Last level cache controller node")

Drop.

> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index f71cf21a8dd8..b0d524bbf051 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -2759,7 +2759,6 @@ dc_noc: interconnect@9160000 {
>  		system-cache-controller@9200000 {
>  			compatible = "qcom,sc7180-llcc";
>  			reg = <0 0x09200000 0 0x50000>, <0 0x09600000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";

That's an ABI break...

>  			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>  		};
>  

Best regards,
Krzysztof

Re: [PATCH v2 04/13] arm64: dts: qcom: sc7180: Remove reg-names property from LLCC node

[Manivannan Sadhasivam]

On Tue, Dec 13, 2022 at 05:30:09PM +0100, Krzysztof Kozlowski wrote:
> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> > The LLCC block has several banks each with a different base address
> > and holes in between. So it is not a correct approach to cover these
> > banks with a single offset/size. Instead, the individual bank's base
> > address needs to be specified in devicetree with the exact size.
> > 
> > On SC7180, there is only one LLCC bank available. So only change needed is
> > to remove the reg-names property from LLCC node to conform to the binding.
> > 
> > The driver is expected to parse the reg field based on index to get the
> > addresses of each LLCC banks.
> > 
> > Cc: <stable@vger.kernel.org> # 5.6
> 
> Oh, no, there is no single bug here. Binding from v5.6+ (which cannot be
> changed) required/defined such reg-names. This is neither a bug nor
> possible to backport.
> 
> > Fixes: c831fa299996 ("arm64: dts: qcom: sc7180: Add Last level cache controller node")
> 
> Drop.
> 
> > Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sc7180.dtsi | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > index f71cf21a8dd8..b0d524bbf051 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > @@ -2759,7 +2759,6 @@ dc_noc: interconnect@9160000 {
> >  		system-cache-controller@9200000 {
> >  			compatible = "qcom,sc7180-llcc";
> >  			reg = <0 0x09200000 0 0x50000>, <0 0x09600000 0 0x50000>;
> > -			reg-names = "llcc_base", "llcc_broadcast_base";
> 
> That's an ABI break...
> 

As agreed, I will keep reg-names in dts for now.

Thanks,
Mani

> >  			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
> >  		};
> >  
> 
> Best regards,
> Krzysztof
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v2 05/13] arm64: dts: qcom: sc7280: Fix the base addresses of LLCC banks

[Manivannan Sadhasivam]

The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

While at it, let's also fix the size of the llcc_broadcast_base to cover
the whole region.

Also, let's get rid of reg-names property as it is not needed anymore.
The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 5.13
Fixes: 0392968dbe09 ("arm64: dts: qcom: sc7280: Add device tree node for LLCC")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 0adf13399e64..90e11cbbaf88 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3579,8 +3579,8 @@ gem_noc: interconnect@9100000 {
 
 		system-cache-controller@9200000 {
 			compatible = "qcom,sc7280-llcc";
-			reg = <0 0x09200000 0 0xd0000>, <0 0x09600000 0 0x50000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
+			reg = <0 0x09200000 0 0x58000>, <0 0x09280000 0 0x58000>,
+			      <0 0x09600000 0 0x58000>;
 			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
-- 
2.25.1

Re: [PATCH v2 05/13] arm64: dts: qcom: sc7280: Fix the base addresses of LLCC banks

[Sai Prakash Ranjan]

On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> While at it, let's also fix the size of the llcc_broadcast_base to cover
> the whole region.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.13
> Fixes: 0392968dbe09 ("arm64: dts: qcom: sc7280: Add device tree node for LLCC")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 0adf13399e64..90e11cbbaf88 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3579,8 +3579,8 @@ gem_noc: interconnect@9100000 {
>   
>   		system-cache-controller@9200000 {
>   			compatible = "qcom,sc7280-llcc";
> -			reg = <0 0x09200000 0 0xd0000>, <0 0x09600000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
> +			reg = <0 0x09200000 0 0x58000>, <0 0x09280000 0 0x58000>,
> +			      <0 0x09600000 0 0x58000>;
>   			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>   		};
>   

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

Re: [PATCH v2 05/13] arm64: dts: qcom: sc7280: Fix the base addresses of LLCC banks

[Krzysztof Kozlowski]

On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> While at it, let's also fix the size of the llcc_broadcast_base to cover
> the whole region.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.13
> Fixes: 0392968dbe09 ("arm64: dts: qcom: sc7280: Add device tree node for LLCC")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>

Same comments here and in all further patches.

Best regards,
Krzysztof

[PATCH v2 06/13] arm64: dts: qcom: sc8280xp: Fix the base addresses of LLCC banks

[Manivannan Sadhasivam]

The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

Also, let's get rid of reg-names property as it is not needed anymore.
The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 6.0
Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 109c9d2b684d..f8731cbccce9 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -1856,8 +1856,11 @@ opp-6 {
 
 		system-cache-controller@9200000 {
 			compatible = "qcom,sc8280xp-llcc";
-			reg = <0 0x09200000 0 0x58000>, <0 0x09600000 0 0x58000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
+			reg = <0 0x09200000 0 0x58000>, <0 0x09280000 0 0x58000>,
+			      <0 0x09300000 0 0x58000>, <0 0x09380000 0 0x58000>,
+			      <0 0x09400000 0 0x58000>, <0 0x09480000 0 0x58000>,
+			      <0 0x09500000 0 0x58000>, <0 0x09580000 0 0x58000>,
+			      <0 0x09600000 0 0x58000>;
 			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
-- 
2.25.1

Re: [PATCH v2 06/13] arm64: dts: qcom: sc8280xp: Fix the base addresses of LLCC banks

[Sai Prakash Ranjan]

On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 6.0
> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 109c9d2b684d..f8731cbccce9 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -1856,8 +1856,11 @@ opp-6 {
>   
>   		system-cache-controller@9200000 {
>   			compatible = "qcom,sc8280xp-llcc";
> -			reg = <0 0x09200000 0 0x58000>, <0 0x09600000 0 0x58000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
> +			reg = <0 0x09200000 0 0x58000>, <0 0x09280000 0 0x58000>,
> +			      <0 0x09300000 0 0x58000>, <0 0x09380000 0 0x58000>,
> +			      <0 0x09400000 0 0x58000>, <0 0x09480000 0 0x58000>,
> +			      <0 0x09500000 0 0x58000>, <0 0x09580000 0 0x58000>,
> +			      <0 0x09600000 0 0x58000>;
>   			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>   		};
>   

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

[PATCH v2 07/13] arm64: dts: qcom: sm8150: Fix the base addresses of LLCC banks

[Manivannan Sadhasivam]

The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

Also, let's get rid of reg-names property as it is not needed anymore.
The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 5.11
Fixes: bb1f7cf68a2d ("arm64: dts: qcom: sm8150: Add LLC support for sm8150")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8150.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index a0c57fb798d3..958655bf5b1a 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -1762,8 +1762,9 @@ mmss_noc: interconnect@1740000 {
 
 		system-cache-controller@9200000 {
 			compatible = "qcom,sm8150-llcc";
-			reg = <0 0x09200000 0 0x200000>, <0 0x09600000 0 0x50000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
+			reg = <0 0x09200000 0 0x50000>, <0 0x09280000 0 0x50000>,
+			      <0 0x09300000 0 0x50000>, <0 0x09380000 0 0x50000>,
+			      <0 0x09600000 0 0x50000>;
 			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
-- 
2.25.1

Re: [PATCH v2 07/13] arm64: dts: qcom: sm8150: Fix the base addresses of LLCC banks

[Sai Prakash Ranjan]

On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.11
> Fixes: bb1f7cf68a2d ("arm64: dts: qcom: sm8150: Add LLC support for sm8150")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8150.dtsi | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index a0c57fb798d3..958655bf5b1a 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> @@ -1762,8 +1762,9 @@ mmss_noc: interconnect@1740000 {
>   
>   		system-cache-controller@9200000 {
>   			compatible = "qcom,sm8150-llcc";
> -			reg = <0 0x09200000 0 0x200000>, <0 0x09600000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
> +			reg = <0 0x09200000 0 0x50000>, <0 0x09280000 0 0x50000>,
> +			      <0 0x09300000 0 0x50000>, <0 0x09380000 0 0x50000>,
> +			      <0 0x09600000 0 0x50000>;
>   			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>   		};
>   

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

[PATCH v2 08/13] arm64: dts: qcom: sm8250: Fix the base addresses of LLCC banks

[Manivannan Sadhasivam]

The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

Also, let's get rid of reg-names property as it is not needed anymore.
The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 5.12
Fixes: 0085a33a25cc ("arm64: dts: qcom: sm8250: Add support for LLCC block")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index dab5579946f3..439ca5f6000e 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -3545,8 +3545,9 @@ usb_1_dwc3: usb@a600000 {
 
 		system-cache-controller@9200000 {
 			compatible = "qcom,sm8250-llcc";
-			reg = <0 0x09200000 0 0x1d0000>, <0 0x09600000 0 0x50000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
+			reg = <0 0x09200000 0 0x50000>, <0 0x09280000 0 0x50000>,
+			      <0 0x09300000 0 0x50000>, <0 0x09380000 0 0x50000>,
+			      <0 0x09600000 0 0x50000>;
 		};
 
 		usb_2: usb@a8f8800 {
-- 
2.25.1

Re: [PATCH v2 08/13] arm64: dts: qcom: sm8250: Fix the base addresses of LLCC banks

[Sai Prakash Ranjan]

On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.12
> Fixes: 0085a33a25cc ("arm64: dts: qcom: sm8250: Add support for LLCC block")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8250.dtsi | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index dab5579946f3..439ca5f6000e 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -3545,8 +3545,9 @@ usb_1_dwc3: usb@a600000 {
>   
>   		system-cache-controller@9200000 {
>   			compatible = "qcom,sm8250-llcc";
> -			reg = <0 0x09200000 0 0x1d0000>, <0 0x09600000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
> +			reg = <0 0x09200000 0 0x50000>, <0 0x09280000 0 0x50000>,
> +			      <0 0x09300000 0 0x50000>, <0 0x09380000 0 0x50000>,
> +			      <0 0x09600000 0 0x50000>;
>   		};
>   
>   		usb_2: usb@a8f8800 {

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

[PATCH v2 09/13] arm64: dts: qcom: sm8350: Fix the base addresses of LLCC banks

[Manivannan Sadhasivam]

The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

Also, let's get rid of reg-names property as it is not needed anymore.
The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 5.17
Fixes: 9ac8999e8d6c ("arm64: dts: qcom: sm8350: Add LLCC node")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8350.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
index 245dce24ec59..7f76778e1bc8 100644
--- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
@@ -2513,8 +2513,9 @@ gem_noc: interconnect@9100000 {
 
 		system-cache-controller@9200000 {
 			compatible = "qcom,sm8350-llcc";
-			reg = <0 0x09200000 0 0x1d0000>, <0 0x09600000 0 0x50000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
+			reg = <0 0x09200000 0 0x58000>, <0 0x09280000 0 0x58000>,
+			      <0 0x09300000 0 0x58000>, <0 0x09380000 0 0x58000>,
+			      <0 0x09600000 0 0x58000>;
 		};
 
 		usb_1: usb@a6f8800 {
-- 
2.25.1

Re: [PATCH v2 09/13] arm64: dts: qcom: sm8350: Fix the base addresses of LLCC banks

[Sai Prakash Ranjan]

On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.17
> Fixes: 9ac8999e8d6c ("arm64: dts: qcom: sm8350: Add LLCC node")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8350.dtsi | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> index 245dce24ec59..7f76778e1bc8 100644
> --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> @@ -2513,8 +2513,9 @@ gem_noc: interconnect@9100000 {
>   
>   		system-cache-controller@9200000 {
>   			compatible = "qcom,sm8350-llcc";
> -			reg = <0 0x09200000 0 0x1d0000>, <0 0x09600000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
> +			reg = <0 0x09200000 0 0x58000>, <0 0x09280000 0 0x58000>,
> +			      <0 0x09300000 0 0x58000>, <0 0x09380000 0 0x58000>,
> +			      <0 0x09600000 0 0x58000>;
>   		};
>   
>   		usb_1: usb@a6f8800 {

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

[PATCH v2 10/13] arm64: dts: qcom: sm8450: Fix the base addresses of LLCC banks

[Manivannan Sadhasivam]

The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

Also, let's get rid of reg-names property as it is not needed anymore.
The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 5.18
Fixes: 1dc3e50eb680 ("arm64: dts: qcom: sm8450: Add LLCC/system-cache-controller node")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 570475040d95..30685857021a 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -3640,8 +3640,9 @@ gem_noc: interconnect@19100000 {
 
 		system-cache-controller@19200000 {
 			compatible = "qcom,sm8450-llcc";
-			reg = <0 0x19200000 0 0x580000>, <0 0x19a00000 0 0x80000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
+			reg = <0 0x19200000 0 0x80000>, <0 0x19600000 0 0x80000>,
+			      <0 0x19300000 0 0x80000>, <0 0x19700000 0 0x80000>,
+			      <0 0x19a00000 0 0x80000>;
 			interrupts = <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
-- 
2.25.1

Re: [PATCH v2 10/13] arm64: dts: qcom: sm8450: Fix the base addresses of LLCC banks

[Sai Prakash Ranjan]

On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.18
> Fixes: 1dc3e50eb680 ("arm64: dts: qcom: sm8450: Add LLCC/system-cache-controller node")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8450.dtsi | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 570475040d95..30685857021a 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -3640,8 +3640,9 @@ gem_noc: interconnect@19100000 {
>   
>   		system-cache-controller@19200000 {
>   			compatible = "qcom,sm8450-llcc";
> -			reg = <0 0x19200000 0 0x580000>, <0 0x19a00000 0 0x80000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
> +			reg = <0 0x19200000 0 0x80000>, <0 0x19600000 0 0x80000>,
> +			      <0 0x19300000 0 0x80000>, <0 0x19700000 0 0x80000>,
> +			      <0 0x19a00000 0 0x80000>;
>   			interrupts = <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>;
>   		};
>   

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

[PATCH v2 11/13] arm64: dts: qcom: sm6350: Remove reg-names property from LLCC node

[Manivannan Sadhasivam]

The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

On SM6350, there is only one LLCC bank available. So only change needed is
to remove the reg-names property from LLCC node to conform to the binding.

The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 5.16
Fixes: ced2f0d75e13 ("arm64: dts: qcom: sm6350: Add LLCC node")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm6350.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
index 43324bf291c3..1f39627cd7c6 100644
--- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
@@ -1174,7 +1174,6 @@ dc_noc: interconnect@9160000 {
 		system-cache-controller@9200000 {
 			compatible = "qcom,sm6350-llcc";
 			reg = <0 0x09200000 0 0x50000>, <0 0x09600000 0 0x50000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
 		};
 
 		gem_noc: interconnect@9680000 {
-- 
2.25.1

Re: [PATCH v2 11/13] arm64: dts: qcom: sm6350: Remove reg-names property from LLCC node

[Sai Prakash Ranjan]

On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> On SM6350, there is only one LLCC bank available. So only change needed is
> to remove the reg-names property from LLCC node to conform to the binding.
> 
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.16
> Fixes: ced2f0d75e13 ("arm64: dts: qcom: sm6350: Add LLCC node")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm6350.dtsi | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> index 43324bf291c3..1f39627cd7c6 100644
> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> @@ -1174,7 +1174,6 @@ dc_noc: interconnect@9160000 {
>   		system-cache-controller@9200000 {
>   			compatible = "qcom,sm6350-llcc";
>   			reg = <0 0x09200000 0 0x50000>, <0 0x09600000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
>   		};
>   
>   		gem_noc: interconnect@9680000 {

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

Re: [PATCH v2 11/13] arm64: dts: qcom: sm6350: Remove reg-names property from LLCC node

[Krzysztof Kozlowski]

On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> On SM6350, there is only one LLCC bank available. So only change needed is
> to remove the reg-names property from LLCC node to conform to the binding.
> 
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.16
> Fixes: ced2f0d75e13 ("arm64: dts: qcom: sm6350: Add LLCC node")

This is a definitive no go. There is no bug here and such change cannot
be backported.

> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>

What is the bug here which deserves a credit? reg-names in v5.16 were
perfectly correct.

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> index 43324bf291c3..1f39627cd7c6 100644
> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> @@ -1174,7 +1174,6 @@ dc_noc: interconnect@9160000 {
>  		system-cache-controller@9200000 {
>  			compatible = "qcom,sm6350-llcc";
>  			reg = <0 0x09200000 0 0x50000>, <0 0x09600000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
>  		};
>  
>  		gem_noc: interconnect@9680000 {

Best regards,
Krzysztof

Re: [PATCH v2 11/13] arm64: dts: qcom: sm6350: Remove reg-names property from LLCC node

[Manivannan Sadhasivam]

On Tue, Dec 13, 2022 at 05:31:40PM +0100, Krzysztof Kozlowski wrote:
> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> > The LLCC block has several banks each with a different base address
> > and holes in between. So it is not a correct approach to cover these
> > banks with a single offset/size. Instead, the individual bank's base
> > address needs to be specified in devicetree with the exact size.
> > 
> > On SM6350, there is only one LLCC bank available. So only change needed is
> > to remove the reg-names property from LLCC node to conform to the binding.
> > 
> > The driver is expected to parse the reg field based on index to get the
> > addresses of each LLCC banks.
> > 
> > Cc: <stable@vger.kernel.org> # 5.16
> > Fixes: ced2f0d75e13 ("arm64: dts: qcom: sm6350: Add LLCC node")
> 
> This is a definitive no go. There is no bug here and such change cannot
> be backported.
> 
> > Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> 
> What is the bug here which deserves a credit? reg-names in v5.16 were
> perfectly correct.
> 

If the driver gets backported to v5.16, it won't. But anyway, this property
will stay in dts.

Thanks,
Mani

> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sm6350.dtsi | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > index 43324bf291c3..1f39627cd7c6 100644
> > --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > @@ -1174,7 +1174,6 @@ dc_noc: interconnect@9160000 {
> >  		system-cache-controller@9200000 {
> >  			compatible = "qcom,sm6350-llcc";
> >  			reg = <0 0x09200000 0 0x50000>, <0 0x09600000 0 0x50000>;
> > -			reg-names = "llcc_base", "llcc_broadcast_base";
> >  		};
> >  
> >  		gem_noc: interconnect@9680000 {
> 
> Best regards,
> Krzysztof
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v2 12/13] qcom: llcc/edac: Fix the base address used for accessing LLCC banks

[Manivannan Sadhasivam]

The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
accessing the (Control and Status Registers) CSRs of each LLCC bank.
This stride only works for some SoCs like SDM845 for which driver
support was initially added.

But the later SoCs use different register stride that vary between the
banks with holes in-between. So it is not possible to use a single register
stride for accessing the CSRs of each bank. By doing so could result in a
crash.

For fixing this issue, let's obtain the base address of each LLCC bank from
devicetree and get rid of the fixed stride. This also means, we no longer
need to rely on reg-names property and get the base addresses using index.

First index is LLCC bank 0 and last index is LLCC broadcast. If the SoC
supports more than one bank, then those needs to be defined in devicetree
for index from 1..N-1.

Cc: <stable@vger.kernel.org> # 4.20
Fixes: a3134fb09e0b ("drivers: soc: Add LLCC driver")
Fixes: 27450653f1db ("drivers: edac: Add EDAC driver support for QCOM SoCs")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/edac/qcom_edac.c           | 14 +++---
 drivers/soc/qcom/llcc-qcom.c       | 72 +++++++++++++++++-------------
 include/linux/soc/qcom/llcc-qcom.h |  6 +--
 3 files changed, 48 insertions(+), 44 deletions(-)

diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
index 97a27e42dd61..5be93577fc03 100644
--- a/drivers/edac/qcom_edac.c
+++ b/drivers/edac/qcom_edac.c
@@ -213,7 +213,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
 
 	for (i = 0; i < reg_data.reg_cnt; i++) {
 		synd_reg = reg_data.synd_reg + (i * 4);
-		ret = regmap_read(drv->regmap, drv->offsets[bank] + synd_reg,
+		ret = regmap_read(drv->regmaps[bank], synd_reg,
 				  &synd_val);
 		if (ret)
 			goto clear;
@@ -222,8 +222,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
 			    reg_data.name, i, synd_val);
 	}
 
-	ret = regmap_read(drv->regmap,
-			  drv->offsets[bank] + reg_data.count_status_reg,
+	ret = regmap_read(drv->regmaps[bank], reg_data.count_status_reg,
 			  &err_cnt);
 	if (ret)
 		goto clear;
@@ -233,8 +232,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
 	edac_printk(KERN_CRIT, EDAC_LLCC, "%s: Error count: 0x%4x\n",
 		    reg_data.name, err_cnt);
 
-	ret = regmap_read(drv->regmap,
-			  drv->offsets[bank] + reg_data.ways_status_reg,
+	ret = regmap_read(drv->regmaps[bank], reg_data.ways_status_reg,
 			  &err_ways);
 	if (ret)
 		goto clear;
@@ -296,8 +294,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
 
 	/* Iterate over the banks and look for Tag RAM or Data RAM errors */
 	for (i = 0; i < drv->num_banks; i++) {
-		ret = regmap_read(drv->regmap,
-				  drv->offsets[i] + DRP_INTERRUPT_STATUS,
+		ret = regmap_read(drv->regmaps[i], DRP_INTERRUPT_STATUS,
 				  &drp_error);
 
 		if (!ret && (drp_error & SB_ECC_ERROR)) {
@@ -312,8 +309,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
 		if (!ret)
 			irq_rc = IRQ_HANDLED;
 
-		ret = regmap_read(drv->regmap,
-				  drv->offsets[i] + TRP_INTERRUPT_0_STATUS,
+		ret = regmap_read(drv->regmaps[i], TRP_INTERRUPT_0_STATUS,
 				  &trp_error);
 
 		if (!ret && (trp_error & SB_ECC_ERROR)) {
diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 23ce2f78c4ed..a29f22dad7fa 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -62,8 +62,6 @@
 #define LLCC_TRP_WRSC_CACHEABLE_EN    0x21f2c
 #define LLCC_TRP_ALGO_CFG8	      0x21f30
 
-#define BANK_OFFSET_STRIDE	      0x80000
-
 #define LLCC_VERSION_2_0_0_0          0x02000000
 #define LLCC_VERSION_2_1_0_0          0x02010000
 #define LLCC_VERSION_4_1_0_0          0x04010000
@@ -898,8 +896,8 @@ static int qcom_llcc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
-		const char *name)
+static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev, u8 index,
+					  const char *name)
 {
 	void __iomem *base;
 	struct regmap_config llcc_regmap_config = {
@@ -909,7 +907,7 @@ static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
 		.fast_io = true,
 	};
 
-	base = devm_platform_ioremap_resource_byname(pdev, name);
+	base = devm_platform_ioremap_resource(pdev, index);
 	if (IS_ERR(base))
 		return ERR_CAST(base);
 
@@ -927,6 +925,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 	const struct llcc_slice_config *llcc_cfg;
 	u32 sz;
 	u32 version;
+	struct regmap *regmap;
 
 	drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
 	if (!drv_data) {
@@ -934,21 +933,51 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	drv_data->regmap = qcom_llcc_init_mmio(pdev, "llcc_base");
-	if (IS_ERR(drv_data->regmap)) {
-		ret = PTR_ERR(drv_data->regmap);
+	/* Initialize the first LLCC bank regmap */
+	regmap = qcom_llcc_init_mmio(pdev, i, "llcc0_base");
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
 		goto err;
 	}
 
-	drv_data->bcast_regmap =
-		qcom_llcc_init_mmio(pdev, "llcc_broadcast_base");
+	cfg = of_device_get_match_data(&pdev->dev);
+
+	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
+	if (ret)
+		goto err;
+
+	num_banks &= LLCC_LB_CNT_MASK;
+	num_banks >>= LLCC_LB_CNT_SHIFT;
+	drv_data->num_banks = num_banks;
+
+	drv_data->regmaps = devm_kcalloc(dev, num_banks, sizeof(*drv_data->regmaps), GFP_KERNEL);
+	if (!drv_data->regmaps) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	drv_data->regmaps[0] = regmap;
+
+	/* Initialize rest of LLCC bank regmaps */
+	for (i = 1; i < num_banks; i++) {
+		char *base = kasprintf(GFP_KERNEL, "llcc%d_base", i);
+
+		drv_data->regmaps[i] = qcom_llcc_init_mmio(pdev, i, base);
+		if (IS_ERR(drv_data->regmaps[i])) {
+			ret = PTR_ERR(drv_data->regmaps[i]);
+			kfree(base);
+			goto err;
+		}
+
+		kfree(base);
+	}
+
+	drv_data->bcast_regmap = qcom_llcc_init_mmio(pdev, i, "llcc_broadcast_base");
 	if (IS_ERR(drv_data->bcast_regmap)) {
 		ret = PTR_ERR(drv_data->bcast_regmap);
 		goto err;
 	}
 
-	cfg = of_device_get_match_data(&pdev->dev);
-
 	/* Extract version of the IP */
 	ret = regmap_read(drv_data->bcast_regmap, cfg->reg_offset[LLCC_COMMON_HW_INFO],
 			  &version);
@@ -957,15 +986,6 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 
 	drv_data->version = version;
 
-	ret = regmap_read(drv_data->regmap, cfg->reg_offset[LLCC_COMMON_STATUS0],
-			  &num_banks);
-	if (ret)
-		goto err;
-
-	num_banks &= LLCC_LB_CNT_MASK;
-	num_banks >>= LLCC_LB_CNT_SHIFT;
-	drv_data->num_banks = num_banks;
-
 	llcc_cfg = cfg->sct_data;
 	sz = cfg->size;
 
@@ -973,16 +993,6 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 		if (llcc_cfg[i].slice_id > drv_data->max_slices)
 			drv_data->max_slices = llcc_cfg[i].slice_id;
 
-	drv_data->offsets = devm_kcalloc(dev, num_banks, sizeof(u32),
-							GFP_KERNEL);
-	if (!drv_data->offsets) {
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	for (i = 0; i < num_banks; i++)
-		drv_data->offsets[i] = i * BANK_OFFSET_STRIDE;
-
 	drv_data->bitmap = devm_bitmap_zalloc(dev, drv_data->max_slices,
 					      GFP_KERNEL);
 	if (!drv_data->bitmap) {
diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
index ad1fd718169d..423220e66026 100644
--- a/include/linux/soc/qcom/llcc-qcom.h
+++ b/include/linux/soc/qcom/llcc-qcom.h
@@ -120,7 +120,7 @@ struct llcc_edac_reg_offset {
 
 /**
  * struct llcc_drv_data - Data associated with the llcc driver
- * @regmap: regmap associated with the llcc device
+ * @regmaps: regmaps associated with the llcc device
  * @bcast_regmap: regmap associated with llcc broadcast offset
  * @cfg: pointer to the data structure for slice configuration
  * @edac_reg_offset: Offset of the LLCC EDAC registers
@@ -129,12 +129,11 @@ struct llcc_edac_reg_offset {
  * @max_slices: max slices as read from device tree
  * @num_banks: Number of llcc banks
  * @bitmap: Bit map to track the active slice ids
- * @offsets: Pointer to the bank offsets array
  * @ecc_irq: interrupt for llcc cache error detection and reporting
  * @version: Indicates the LLCC version
  */
 struct llcc_drv_data {
-	struct regmap *regmap;
+	struct regmap **regmaps;
 	struct regmap *bcast_regmap;
 	const struct llcc_slice_config *cfg;
 	const struct llcc_edac_reg_offset *edac_reg_offset;
@@ -143,7 +142,6 @@ struct llcc_drv_data {
 	u32 max_slices;
 	u32 num_banks;
 	unsigned long *bitmap;
-	u32 *offsets;
 	int ecc_irq;
 	u32 version;
 };
-- 
2.25.1

Re: [PATCH v2 12/13] qcom: llcc/edac: Fix the base address used for accessing LLCC banks

[Krzysztof Kozlowski]

On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> accessing the (Control and Status Registers) CSRs of each LLCC bank.
> This stride only works for some SoCs like SDM845 for which driver
> support was initially added.
> 
> But the later SoCs use different register stride that vary between the
> banks with holes in-between. So it is not possible to use a single register
> stride for accessing the CSRs of each bank. By doing so could result in a
> crash.
> 
> For fixing this issue, let's obtain the base address of each LLCC bank from
> devicetree and get rid of the fixed stride. This also means, we no longer
> need to rely on reg-names property and get the base addresses using index.
> 
> First index is LLCC bank 0 and last index is LLCC broadcast. If the SoC
> supports more than one bank, then those needs to be defined in devicetree
> for index from 1..N-1.
> 
> Cc: <stable@vger.kernel.org> # 4.20
> Fixes: a3134fb09e0b ("drivers: soc: Add LLCC driver")
> Fixes: 27450653f1db ("drivers: edac: Add EDAC driver support for QCOM SoCs")

Your previous patches in the series had incorrect CC-stable/Fixes tags,
thus I have doubts also here.

Can you confirm, that this patch alone (alone! Without DTS patches) when
backported to v4.20, still works perfectly fine for sdm845?

> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/edac/qcom_edac.c           | 14 +++---
>  drivers/soc/qcom/llcc-qcom.c       | 72 +++++++++++++++++-------------
>  include/linux/soc/qcom/llcc-qcom.h |  6 +--
>  3 files changed, 48 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> index 97a27e42dd61..5be93577fc03 100644
> --- a/drivers/edac/qcom_edac.c
> +++ b/drivers/edac/qcom_edac.c
> @@ -213,7 +213,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
>  
>  	for (i = 0; i < reg_data.reg_cnt; i++) {
>  		synd_reg = reg_data.synd_reg + (i * 4);
> -		ret = regmap_read(drv->regmap, drv->offsets[bank] + synd_reg,
> +		ret = regmap_read(drv->regmaps[bank], synd_reg,
>  				  &synd_val);
>  		if (ret)
>  			goto clear;
> @@ -222,8 +222,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
>  			    reg_data.name, i, synd_val);
>  	}
>  
> -	ret = regmap_read(drv->regmap,
> -			  drv->offsets[bank] + reg_data.count_status_reg,
> +	ret = regmap_read(drv->regmaps[bank], reg_data.count_status_reg,
>  			  &err_cnt);
>  	if (ret)
>  		goto clear;
> @@ -233,8 +232,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
>  	edac_printk(KERN_CRIT, EDAC_LLCC, "%s: Error count: 0x%4x\n",
>  		    reg_data.name, err_cnt);
>  
> -	ret = regmap_read(drv->regmap,
> -			  drv->offsets[bank] + reg_data.ways_status_reg,
> +	ret = regmap_read(drv->regmaps[bank], reg_data.ways_status_reg,
>  			  &err_ways);
>  	if (ret)
>  		goto clear;
> @@ -296,8 +294,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
>  
>  	/* Iterate over the banks and look for Tag RAM or Data RAM errors */
>  	for (i = 0; i < drv->num_banks; i++) {
> -		ret = regmap_read(drv->regmap,
> -				  drv->offsets[i] + DRP_INTERRUPT_STATUS,
> +		ret = regmap_read(drv->regmaps[i], DRP_INTERRUPT_STATUS,
>  				  &drp_error);
>  
>  		if (!ret && (drp_error & SB_ECC_ERROR)) {
> @@ -312,8 +309,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
>  		if (!ret)
>  			irq_rc = IRQ_HANDLED;
>  
> -		ret = regmap_read(drv->regmap,
> -				  drv->offsets[i] + TRP_INTERRUPT_0_STATUS,
> +		ret = regmap_read(drv->regmaps[i], TRP_INTERRUPT_0_STATUS,
>  				  &trp_error);
>  
>  		if (!ret && (trp_error & SB_ECC_ERROR)) {
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 23ce2f78c4ed..a29f22dad7fa 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -62,8 +62,6 @@
>  #define LLCC_TRP_WRSC_CACHEABLE_EN    0x21f2c
>  #define LLCC_TRP_ALGO_CFG8	      0x21f30
>  
> -#define BANK_OFFSET_STRIDE	      0x80000
> -
>  #define LLCC_VERSION_2_0_0_0          0x02000000
>  #define LLCC_VERSION_2_1_0_0          0x02010000
>  #define LLCC_VERSION_4_1_0_0          0x04010000
> @@ -898,8 +896,8 @@ static int qcom_llcc_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
> -		const char *name)
> +static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev, u8 index,
> +					  const char *name)
>  {
>  	void __iomem *base;
>  	struct regmap_config llcc_regmap_config = {
> @@ -909,7 +907,7 @@ static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
>  		.fast_io = true,
>  	};
>  
> -	base = devm_platform_ioremap_resource_byname(pdev, name);
> +	base = devm_platform_ioremap_resource(pdev, index);
>  	if (IS_ERR(base))
>  		return ERR_CAST(base);
>  
> @@ -927,6 +925,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  	const struct llcc_slice_config *llcc_cfg;
>  	u32 sz;
>  	u32 version;
> +	struct regmap *regmap;
>  
>  	drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>  	if (!drv_data) {
> @@ -934,21 +933,51 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	drv_data->regmap = qcom_llcc_init_mmio(pdev, "llcc_base");
> -	if (IS_ERR(drv_data->regmap)) {
> -		ret = PTR_ERR(drv_data->regmap);
> +	/* Initialize the first LLCC bank regmap */
> +	regmap = qcom_llcc_init_mmio(pdev, i, "llcc0_base");

What is the value of "i" here? Looks like not initialized in my next.

> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
>  		goto err;
>  	}
>  
> -	drv_data->bcast_regmap =
> -		qcom_llcc_init_mmio(pdev, "llcc_broadcast_base");
> +	cfg = of_device_get_match_data(&pdev->dev);
> +
> +	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
> +	if (ret)
> +		goto err;
> +
> +	num_banks &= LLCC_LB_CNT_MASK;
> +	num_banks >>= LLCC_LB_CNT_SHIFT;
> +	drv_data->num_banks = num_banks;
> +
> +	drv_data->regmaps = devm_kcalloc(dev, num_banks, sizeof(*drv_data->regmaps), GFP_KERNEL);
> +	if (!drv_data->regmaps) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	drv_data->regmaps[0] = regmap;
> +
> +	/* Initialize rest of LLCC bank regmaps */
> +	for (i = 1; i < num_banks; i++) {
> +		char *base = kasprintf(GFP_KERNEL, "llcc%d_base", i);
> +
> +		drv_data->regmaps[i] = qcom_llcc_init_mmio(pdev, i, base);
> +		if (IS_ERR(drv_data->regmaps[i])) {
> +			ret = PTR_ERR(drv_data->regmaps[i]);
> +			kfree(base);
> +			goto err;

This looks like the ABI break so:
1. Existing users are broken,
2. It cannot be backported.


> +		}
> +
> +		kfree(base);
> +	}
> +

Best regards,
Krzysztof

Re: [PATCH v2 12/13] qcom: llcc/edac: Fix the base address used for accessing LLCC banks

[Manivannan Sadhasivam]

On Tue, Dec 13, 2022 at 05:37:37PM +0100, Krzysztof Kozlowski wrote:
> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> > The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> > accessing the (Control and Status Registers) CSRs of each LLCC bank.
> > This stride only works for some SoCs like SDM845 for which driver
> > support was initially added.
> > 
> > But the later SoCs use different register stride that vary between the
> > banks with holes in-between. So it is not possible to use a single register
> > stride for accessing the CSRs of each bank. By doing so could result in a
> > crash.
> > 
> > For fixing this issue, let's obtain the base address of each LLCC bank from
> > devicetree and get rid of the fixed stride. This also means, we no longer
> > need to rely on reg-names property and get the base addresses using index.
> > 
> > First index is LLCC bank 0 and last index is LLCC broadcast. If the SoC
> > supports more than one bank, then those needs to be defined in devicetree
> > for index from 1..N-1.
> > 
> > Cc: <stable@vger.kernel.org> # 4.20
> > Fixes: a3134fb09e0b ("drivers: soc: Add LLCC driver")
> > Fixes: 27450653f1db ("drivers: edac: Add EDAC driver support for QCOM SoCs")
> 
> Your previous patches in the series had incorrect CC-stable/Fixes tags,
> thus I have doubts also here.
> 

Sorry I do not agree with you. I wanted to backport binding, dts and driver
patches to possible LTS kernels together and that's why I tagged stable list.

Either all goes to stable or none. If your question is more towards what if one
goes before the other, then in that case I may need to specify the dependency
of commits but that will look messy. I took the gamble because, the driver is
already broken in stable kernels.

> Can you confirm, that this patch alone (alone! Without DTS patches) when
> backported to v4.20, still works perfectly fine for sdm845?
> 

It won't and that's why I also tagged dts patches for backporting.

> > Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/edac/qcom_edac.c           | 14 +++---
> >  drivers/soc/qcom/llcc-qcom.c       | 72 +++++++++++++++++-------------
> >  include/linux/soc/qcom/llcc-qcom.h |  6 +--
> >  3 files changed, 48 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> > index 97a27e42dd61..5be93577fc03 100644
> > --- a/drivers/edac/qcom_edac.c
> > +++ b/drivers/edac/qcom_edac.c
> > @@ -213,7 +213,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
> >  
> >  	for (i = 0; i < reg_data.reg_cnt; i++) {
> >  		synd_reg = reg_data.synd_reg + (i * 4);
> > -		ret = regmap_read(drv->regmap, drv->offsets[bank] + synd_reg,
> > +		ret = regmap_read(drv->regmaps[bank], synd_reg,
> >  				  &synd_val);
> >  		if (ret)
> >  			goto clear;
> > @@ -222,8 +222,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
> >  			    reg_data.name, i, synd_val);
> >  	}
> >  
> > -	ret = regmap_read(drv->regmap,
> > -			  drv->offsets[bank] + reg_data.count_status_reg,
> > +	ret = regmap_read(drv->regmaps[bank], reg_data.count_status_reg,
> >  			  &err_cnt);
> >  	if (ret)
> >  		goto clear;
> > @@ -233,8 +232,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
> >  	edac_printk(KERN_CRIT, EDAC_LLCC, "%s: Error count: 0x%4x\n",
> >  		    reg_data.name, err_cnt);
> >  
> > -	ret = regmap_read(drv->regmap,
> > -			  drv->offsets[bank] + reg_data.ways_status_reg,
> > +	ret = regmap_read(drv->regmaps[bank], reg_data.ways_status_reg,
> >  			  &err_ways);
> >  	if (ret)
> >  		goto clear;
> > @@ -296,8 +294,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
> >  
> >  	/* Iterate over the banks and look for Tag RAM or Data RAM errors */
> >  	for (i = 0; i < drv->num_banks; i++) {
> > -		ret = regmap_read(drv->regmap,
> > -				  drv->offsets[i] + DRP_INTERRUPT_STATUS,
> > +		ret = regmap_read(drv->regmaps[i], DRP_INTERRUPT_STATUS,
> >  				  &drp_error);
> >  
> >  		if (!ret && (drp_error & SB_ECC_ERROR)) {
> > @@ -312,8 +309,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
> >  		if (!ret)
> >  			irq_rc = IRQ_HANDLED;
> >  
> > -		ret = regmap_read(drv->regmap,
> > -				  drv->offsets[i] + TRP_INTERRUPT_0_STATUS,
> > +		ret = regmap_read(drv->regmaps[i], TRP_INTERRUPT_0_STATUS,
> >  				  &trp_error);
> >  
> >  		if (!ret && (trp_error & SB_ECC_ERROR)) {
> > diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> > index 23ce2f78c4ed..a29f22dad7fa 100644
> > --- a/drivers/soc/qcom/llcc-qcom.c
> > +++ b/drivers/soc/qcom/llcc-qcom.c
> > @@ -62,8 +62,6 @@
> >  #define LLCC_TRP_WRSC_CACHEABLE_EN    0x21f2c
> >  #define LLCC_TRP_ALGO_CFG8	      0x21f30
> >  
> > -#define BANK_OFFSET_STRIDE	      0x80000
> > -
> >  #define LLCC_VERSION_2_0_0_0          0x02000000
> >  #define LLCC_VERSION_2_1_0_0          0x02010000
> >  #define LLCC_VERSION_4_1_0_0          0x04010000
> > @@ -898,8 +896,8 @@ static int qcom_llcc_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > -static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
> > -		const char *name)
> > +static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev, u8 index,
> > +					  const char *name)
> >  {
> >  	void __iomem *base;
> >  	struct regmap_config llcc_regmap_config = {
> > @@ -909,7 +907,7 @@ static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
> >  		.fast_io = true,
> >  	};
> >  
> > -	base = devm_platform_ioremap_resource_byname(pdev, name);
> > +	base = devm_platform_ioremap_resource(pdev, index);
> >  	if (IS_ERR(base))
> >  		return ERR_CAST(base);
> >  
> > @@ -927,6 +925,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> >  	const struct llcc_slice_config *llcc_cfg;
> >  	u32 sz;
> >  	u32 version;
> > +	struct regmap *regmap;
> >  
> >  	drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
> >  	if (!drv_data) {
> > @@ -934,21 +933,51 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> >  		goto err;
> >  	}
> >  
> > -	drv_data->regmap = qcom_llcc_init_mmio(pdev, "llcc_base");
> > -	if (IS_ERR(drv_data->regmap)) {
> > -		ret = PTR_ERR(drv_data->regmap);
> > +	/* Initialize the first LLCC bank regmap */
> > +	regmap = qcom_llcc_init_mmio(pdev, i, "llcc0_base");
> 
> What is the value of "i" here? Looks like not initialized in my next.
> 

Yes, this was a mistake and been reported by kernel bot. It will be fixed in
next version.

> > +	if (IS_ERR(regmap)) {
> > +		ret = PTR_ERR(regmap);
> >  		goto err;
> >  	}
> >  
> > -	drv_data->bcast_regmap =
> > -		qcom_llcc_init_mmio(pdev, "llcc_broadcast_base");
> > +	cfg = of_device_get_match_data(&pdev->dev);
> > +
> > +	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
> > +	if (ret)
> > +		goto err;
> > +
> > +	num_banks &= LLCC_LB_CNT_MASK;
> > +	num_banks >>= LLCC_LB_CNT_SHIFT;
> > +	drv_data->num_banks = num_banks;
> > +
> > +	drv_data->regmaps = devm_kcalloc(dev, num_banks, sizeof(*drv_data->regmaps), GFP_KERNEL);
> > +	if (!drv_data->regmaps) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	drv_data->regmaps[0] = regmap;
> > +
> > +	/* Initialize rest of LLCC bank regmaps */
> > +	for (i = 1; i < num_banks; i++) {
> > +		char *base = kasprintf(GFP_KERNEL, "llcc%d_base", i);
> > +
> > +		drv_data->regmaps[i] = qcom_llcc_init_mmio(pdev, i, base);
> > +		if (IS_ERR(drv_data->regmaps[i])) {
> > +			ret = PTR_ERR(drv_data->regmaps[i]);
> > +			kfree(base);
> > +			goto err;
> 
> This looks like the ABI break so:
> 1. Existing users are broken,

I fixed the dts for all affected SoCs, then who are all other existing users?

> 2. It cannot be backported.
> 

This is a bug fix and clearly needs to be backported along with the dts
changes. For this purpose only I have tagged both dts and driver patches for
backporting. Am I missing anything here?

Thanks,
Mani

> 
> > +		}
> > +
> > +		kfree(base);
> > +	}
> > +
> 
> Best regards,
> Krzysztof
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 12/13] qcom: llcc/edac: Fix the base address used for accessing LLCC banks

[Krzysztof Kozlowski]

On 13/12/2022 18:44, Manivannan Sadhasivam wrote:
> On Tue, Dec 13, 2022 at 05:37:37PM +0100, Krzysztof Kozlowski wrote:
>> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
>>> The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
>>> accessing the (Control and Status Registers) CSRs of each LLCC bank.
>>> This stride only works for some SoCs like SDM845 for which driver
>>> support was initially added.
>>>
>>> But the later SoCs use different register stride that vary between the
>>> banks with holes in-between. So it is not possible to use a single register
>>> stride for accessing the CSRs of each bank. By doing so could result in a
>>> crash.
>>>
>>> For fixing this issue, let's obtain the base address of each LLCC bank from
>>> devicetree and get rid of the fixed stride. This also means, we no longer
>>> need to rely on reg-names property and get the base addresses using index.
>>>
>>> First index is LLCC bank 0 and last index is LLCC broadcast. If the SoC
>>> supports more than one bank, then those needs to be defined in devicetree
>>> for index from 1..N-1.
>>>
>>> Cc: <stable@vger.kernel.org> # 4.20
>>> Fixes: a3134fb09e0b ("drivers: soc: Add LLCC driver")
>>> Fixes: 27450653f1db ("drivers: edac: Add EDAC driver support for QCOM SoCs")
>>
>> Your previous patches in the series had incorrect CC-stable/Fixes tags,
>> thus I have doubts also here.
>>
> 
> Sorry I do not agree with you. I wanted to backport binding, dts and driver
> patches to possible LTS kernels together and that's why I tagged stable list.
> 
> Either all goes to stable or none. If your question is more towards what if one
> goes before the other, then in that case I may need to specify the dependency
> of commits but that will look messy. I took the gamble because, the driver is
> already broken in stable kernels.

I understand the intention. Assuming SDM845 was working, by backporting
it you affect all stable users while fixing other SoCs. Therefore the
patch would be worth backports only if other users with SDM845 were not
affected.

> 
>>> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>>  drivers/edac/qcom_edac.c           | 14 +++---
>>>  drivers/soc/qcom/llcc-qcom.c       | 72 +++++++++++++++++-------------
>>>  include/linux/soc/qcom/llcc-qcom.h |  6 +--
>>>  3 files changed, 48 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
>>> index 97a27e42dd61..5be93577fc03 100644
>>> --- a/drivers/edac/qcom_edac.c
>>> +++ b/drivers/edac/qcom_edac.c
>>> @@ -213,7 +213,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
>>>  
>>>  	for (i = 0; i < reg_data.reg_cnt; i++) {
>>>  		synd_reg = reg_data.synd_reg + (i * 4);
>>> -		ret = regmap_read(drv->regmap, drv->offsets[bank] + synd_reg,
>>> +		ret = regmap_read(drv->regmaps[bank], synd_reg,
>>>  				  &synd_val);
>>>  		if (ret)
>>>  			goto clear;
>>> @@ -222,8 +222,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
>>>  			    reg_data.name, i, synd_val);
>>>  	}
>>>  
>>> -	ret = regmap_read(drv->regmap,
>>> -			  drv->offsets[bank] + reg_data.count_status_reg,
>>> +	ret = regmap_read(drv->regmaps[bank], reg_data.count_status_reg,
>>>  			  &err_cnt);
>>>  	if (ret)
>>>  		goto clear;
>>> @@ -233,8 +232,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
>>>  	edac_printk(KERN_CRIT, EDAC_LLCC, "%s: Error count: 0x%4x\n",
>>>  		    reg_data.name, err_cnt);
>>>  
>>> -	ret = regmap_read(drv->regmap,
>>> -			  drv->offsets[bank] + reg_data.ways_status_reg,
>>> +	ret = regmap_read(drv->regmaps[bank], reg_data.ways_status_reg,
>>>  			  &err_ways);
>>>  	if (ret)
>>>  		goto clear;
>>> @@ -296,8 +294,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
>>>  
>>>  	/* Iterate over the banks and look for Tag RAM or Data RAM errors */
>>>  	for (i = 0; i < drv->num_banks; i++) {
>>> -		ret = regmap_read(drv->regmap,
>>> -				  drv->offsets[i] + DRP_INTERRUPT_STATUS,
>>> +		ret = regmap_read(drv->regmaps[i], DRP_INTERRUPT_STATUS,
>>>  				  &drp_error);
>>>  
>>>  		if (!ret && (drp_error & SB_ECC_ERROR)) {
>>> @@ -312,8 +309,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
>>>  		if (!ret)
>>>  			irq_rc = IRQ_HANDLED;
>>>  
>>> -		ret = regmap_read(drv->regmap,
>>> -				  drv->offsets[i] + TRP_INTERRUPT_0_STATUS,
>>> +		ret = regmap_read(drv->regmaps[i], TRP_INTERRUPT_0_STATUS,
>>>  				  &trp_error);
>>>  
>>>  		if (!ret && (trp_error & SB_ECC_ERROR)) {
>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>> index 23ce2f78c4ed..a29f22dad7fa 100644
>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>> @@ -62,8 +62,6 @@
>>>  #define LLCC_TRP_WRSC_CACHEABLE_EN    0x21f2c
>>>  #define LLCC_TRP_ALGO_CFG8	      0x21f30
>>>  
>>> -#define BANK_OFFSET_STRIDE	      0x80000
>>> -
>>>  #define LLCC_VERSION_2_0_0_0          0x02000000
>>>  #define LLCC_VERSION_2_1_0_0          0x02010000
>>>  #define LLCC_VERSION_4_1_0_0          0x04010000
>>> @@ -898,8 +896,8 @@ static int qcom_llcc_remove(struct platform_device *pdev)
>>>  	return 0;
>>>  }
>>>  
>>> -static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
>>> -		const char *name)
>>> +static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev, u8 index,
>>> +					  const char *name)
>>>  {
>>>  	void __iomem *base;
>>>  	struct regmap_config llcc_regmap_config = {
>>> @@ -909,7 +907,7 @@ static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
>>>  		.fast_io = true,
>>>  	};
>>>  
>>> -	base = devm_platform_ioremap_resource_byname(pdev, name);
>>> +	base = devm_platform_ioremap_resource(pdev, index);
>>>  	if (IS_ERR(base))
>>>  		return ERR_CAST(base);
>>>  
>>> @@ -927,6 +925,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>  	const struct llcc_slice_config *llcc_cfg;
>>>  	u32 sz;
>>>  	u32 version;
>>> +	struct regmap *regmap;
>>>  
>>>  	drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>>>  	if (!drv_data) {
>>> @@ -934,21 +933,51 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>  		goto err;
>>>  	}
>>>  
>>> -	drv_data->regmap = qcom_llcc_init_mmio(pdev, "llcc_base");
>>> -	if (IS_ERR(drv_data->regmap)) {
>>> -		ret = PTR_ERR(drv_data->regmap);
>>> +	/* Initialize the first LLCC bank regmap */
>>> +	regmap = qcom_llcc_init_mmio(pdev, i, "llcc0_base");
>>
>> What is the value of "i" here? Looks like not initialized in my next.
>>
> 
> Yes, this was a mistake and been reported by kernel bot. It will be fixed in
> next version.
> 
>>> +	if (IS_ERR(regmap)) {
>>> +		ret = PTR_ERR(regmap);
>>>  		goto err;
>>>  	}
>>>  
>>> -	drv_data->bcast_regmap =
>>> -		qcom_llcc_init_mmio(pdev, "llcc_broadcast_base");
>>> +	cfg = of_device_get_match_data(&pdev->dev);
>>> +
>>> +	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
>>> +	if (ret)
>>> +		goto err;
>>> +
>>> +	num_banks &= LLCC_LB_CNT_MASK;
>>> +	num_banks >>= LLCC_LB_CNT_SHIFT;
>>> +	drv_data->num_banks = num_banks;
>>> +
>>> +	drv_data->regmaps = devm_kcalloc(dev, num_banks, sizeof(*drv_data->regmaps), GFP_KERNEL);
>>> +	if (!drv_data->regmaps) {
>>> +		ret = -ENOMEM;
>>> +		goto err;
>>> +	}
>>> +
>>> +	drv_data->regmaps[0] = regmap;
>>> +
>>> +	/* Initialize rest of LLCC bank regmaps */
>>> +	for (i = 1; i < num_banks; i++) {
>>> +		char *base = kasprintf(GFP_KERNEL, "llcc%d_base", i);
>>> +
>>> +		drv_data->regmaps[i] = qcom_llcc_init_mmio(pdev, i, base);
>>> +		if (IS_ERR(drv_data->regmaps[i])) {
>>> +			ret = PTR_ERR(drv_data->regmaps[i]);
>>> +			kfree(base);
>>> +			goto err;
>>
>> This looks like the ABI break so:
>> 1. Existing users are broken,
> 
> I fixed the dts for all affected SoCs, then who are all other existing users?

In the case of the bindings and DTS the other users are:
All DTS in the wild (out-of-tree customers, forks), all other operating
systems (BSD), firmware and bootloaders.

In the case of the driver we talk about all out of tree DTS, which you
did not fix. Kernel must work with old or any other DTB which conforms
to the ABI (bindings). You cannot change ABI to work around that...

> 
>> 2. It cannot be backported.
>>
> 
> This is a bug fix and clearly needs to be backported along with the dts
> changes. For this purpose only I have tagged both dts and driver patches for
> backporting. Am I missing anything here?

You cannot backport ABI break for Linux. We assume ABI should not be
broken for current/future releases, but this we sometimes ignore. There
are reasons. However for the past and stable releases - it's a no go...
unless it never, never worked.

What you can do, is to have patch which does not break ABI for working
platforms (SDM845?). Such patch can be backported.


Best regards,
Krzysztof