[PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Manivannan Sadhasivam]

The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
This offset 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 with the current drivers. So far this crash is not reported since
EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
driver extensively by triggering the EDAC IRQ (that's where each bank
CSRs are accessed).
    
For fixing this issue, let's obtain the base address of each LLCC bank from
devicetree and get rid of the fixed stride.

This series affects multiple platforms but I have only tested this on
SM8250 and SM8450. Testing on other platforms is welcomed.

Thanks,
Mani

Changes in v2:

* Removed reg-names property and used index of reg property to parse LLCC
  bank base address (Bjorn)
* Collected Ack from Sai for binding
* Added a new patch for polling mode (Luca)
* Renamed subject of patches targeting SC7180 and SM6350

Manivannan Sadhasivam (13):
  dt-bindings: arm: msm: Update the maintainers for LLCC
  dt-bindings: arm: msm: Fix register regions used for LLCC banks
  arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks
  arm64: dts: qcom: sc7180: Remove reg-names property from LLCC node
  arm64: dts: qcom: sc7280: Fix the base addresses of LLCC banks
  arm64: dts: qcom: sc8280xp: Fix the base addresses of LLCC banks
  arm64: dts: qcom: sm8150: Fix the base addresses of LLCC banks
  arm64: dts: qcom: sm8250: Fix the base addresses of LLCC banks
  arm64: dts: qcom: sm8350: Fix the base addresses of LLCC banks
  arm64: dts: qcom: sm8450: Fix the base addresses of LLCC banks
  arm64: dts: qcom: sm6350: Remove reg-names property from LLCC node
  qcom: llcc/edac: Fix the base address used for accessing LLCC banks
  qcom: llcc/edac: Support polling mode for ECC handling

 .../bindings/arm/msm/qcom,llcc.yaml           | 100 +++++++++++++++---
 arch/arm64/boot/dts/qcom/sc7180.dtsi          |   1 -
 arch/arm64/boot/dts/qcom/sc7280.dtsi          |   4 +-
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi        |   7 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |   5 +-
 arch/arm64/boot/dts/qcom/sm6350.dtsi          |   1 -
 arch/arm64/boot/dts/qcom/sm8150.dtsi          |   5 +-
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |   5 +-
 arch/arm64/boot/dts/qcom/sm8350.dtsi          |   5 +-
 arch/arm64/boot/dts/qcom/sm8450.dtsi          |   5 +-
 drivers/edac/qcom_edac.c                      |  51 +++++----
 drivers/soc/qcom/llcc-qcom.c                  |  85 ++++++++-------
 include/linux/soc/qcom/llcc-qcom.h            |   6 +-
 13 files changed, 186 insertions(+), 94 deletions(-)

-- 
2.25.1

[PATCH v2 01/13] dt-bindings: arm: msm: Update the maintainers for LLCC

[Manivannan Sadhasivam]

Rishabh Bhatnagar has left Qualcomm, and there is no evidence of him
maintaining with a new identity. So his entry needs to be removed.

Also, Sai Prakash Ranjan's email address should be updated to use
quicinc domain.

Cc: Sai Prakash Ranjan <quic_saipraka@quicinc.com>
Acked-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
index 38efcad56dbd..d1df49ffcc1b 100644
--- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
@@ -7,8 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: Last Level Cache Controller
 
 maintainers:
-  - Rishabh Bhatnagar <rishabhb@codeaurora.org>
-  - Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
+  - Sai Prakash Ranjan <quic_saipraka@quicinc.com>
 
 description: |
   LLCC (Last Level Cache Controller) provides last level of cache memory in SoC,
-- 
2.25.1

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[PATCH v2 13/13] qcom: llcc/edac: Support polling mode for ECC handling

[Manivannan Sadhasivam]

Not all Qcom platforms support IRQ mode for ECC handling. For those
platforms, the current EDAC driver will not be probed due to missing ECC
IRQ in devicetree.

So add support for polling mode so that the EDAC driver can be used on all
Qcom platforms supporting LLCC.

The polling delay of 5000ms is chosed based on Qcom downstream/vendor
driver.

Reported-by: Luca Weiss <luca.weiss@fairphone.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/edac/qcom_edac.c     | 37 +++++++++++++++++++++++++-----------
 drivers/soc/qcom/llcc-qcom.c | 13 ++++++-------
 2 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
index 5be93577fc03..f7afb5375293 100644
--- a/drivers/edac/qcom_edac.c
+++ b/drivers/edac/qcom_edac.c
@@ -76,6 +76,8 @@
 #define DRP0_INTERRUPT_ENABLE           BIT(6)
 #define SB_DB_DRP_INTERRUPT_ENABLE      0x3
 
+#define ECC_POLL_MSEC			5000
+
 enum {
 	LLCC_DRAM_CE = 0,
 	LLCC_DRAM_UE,
@@ -283,8 +285,7 @@ dump_syn_reg(struct edac_device_ctl_info *edev_ctl, int err_type, u32 bank)
 	return ret;
 }
 
-static irqreturn_t
-llcc_ecc_irq_handler(int irq, void *edev_ctl)
+static irqreturn_t llcc_ecc_irq_handler(int irq, void *edev_ctl)
 {
 	struct edac_device_ctl_info *edac_dev_ctl = edev_ctl;
 	struct llcc_drv_data *drv = edac_dev_ctl->pvt_info;
@@ -328,6 +329,11 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
 	return irq_rc;
 }
 
+static void llcc_ecc_check(struct edac_device_ctl_info *edev_ctl)
+{
+	llcc_ecc_irq_handler(0, edev_ctl);
+}
+
 static int qcom_llcc_edac_probe(struct platform_device *pdev)
 {
 	struct llcc_drv_data *llcc_driv_data = pdev->dev.platform_data;
@@ -356,22 +362,31 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
 	edev_ctl->panic_on_ue = LLCC_ERP_PANIC_ON_UE;
 	edev_ctl->pvt_info = llcc_driv_data;
 
+	/* Check if LLCC driver has passed ECC IRQ */
+	ecc_irq = llcc_driv_data->ecc_irq;
+	if (ecc_irq > 0) {
+		/* Use interrupt mode if IRQ is available */
+		edac_op_state = EDAC_OPSTATE_INT;
+	} else {
+		/* Fall back to polling mode otherwise */
+		edac_op_state = EDAC_OPSTATE_POLL;
+		edev_ctl->poll_msec = ECC_POLL_MSEC;
+		edev_ctl->edac_check = llcc_ecc_check;
+	}
+
 	rc = edac_device_add_device(edev_ctl);
 	if (rc)
 		goto out_mem;
 
 	platform_set_drvdata(pdev, edev_ctl);
 
-	/* Request for ecc irq */
-	ecc_irq = llcc_driv_data->ecc_irq;
-	if (ecc_irq < 0) {
-		rc = -ENODEV;
-		goto out_dev;
-	}
-	rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
+	/* Request ECC IRQ if available */
+	if (ecc_irq > 0) {
+		rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
 			      IRQF_TRIGGER_HIGH, "llcc_ecc", edev_ctl);
-	if (rc)
-		goto out_dev;
+		if (rc)
+			goto out_dev;
+	}
 
 	return rc;
 
diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index a29f22dad7fa..e044e6756415 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -1011,13 +1011,12 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 		goto err;
 
 	drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
-	if (drv_data->ecc_irq >= 0) {
-		llcc_edac = platform_device_register_data(&pdev->dev,
-						"qcom_llcc_edac", -1, drv_data,
-						sizeof(*drv_data));
-		if (IS_ERR(llcc_edac))
-			dev_err(dev, "Failed to register llcc edac driver\n");
-	}
+
+	llcc_edac = platform_device_register_data(&pdev->dev,
+					"qcom_llcc_edac", -1, drv_data,
+					sizeof(*drv_data));
+	if (IS_ERR(llcc_edac))
+		dev_err(dev, "Failed to register llcc edac driver\n");
 
 	return 0;
 err:
-- 
2.25.1

Re: [PATCH v2 13/13] qcom: llcc/edac: Support polling mode for ECC handling

[Luca Weiss]

Hi Manivannan,

On Mon Dec 12, 2022 at 1:33 PM CET, Manivannan Sadhasivam wrote:
> Not all Qcom platforms support IRQ mode for ECC handling. For those
> platforms, the current EDAC driver will not be probed due to missing ECC
> IRQ in devicetree.
>
> So add support for polling mode so that the EDAC driver can be used on all
> Qcom platforms supporting LLCC.
>
> The polling delay of 5000ms is chosed based on Qcom downstream/vendor
> driver.

I think it does work for me on SM6350, I get this in dmesg:

[    0.054608] EDAC MC: Ver: 3.0.0
[    0.273913] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (POLLED)

What I've noticed though is that the 5000ms poll you defined in the
driver doesn't seem to be reflected at runtime? Or am I looking at
different things?

/ # cat /sys/devices/system/edac/qcom-llcc/poll_msec 
1000

Regards
Luca

>
> Reported-by: Luca Weiss <luca.weiss@fairphone.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/edac/qcom_edac.c     | 37 +++++++++++++++++++++++++-----------
>  drivers/soc/qcom/llcc-qcom.c | 13 ++++++-------
>  2 files changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> index 5be93577fc03..f7afb5375293 100644
> --- a/drivers/edac/qcom_edac.c
> +++ b/drivers/edac/qcom_edac.c
> @@ -76,6 +76,8 @@
>  #define DRP0_INTERRUPT_ENABLE           BIT(6)
>  #define SB_DB_DRP_INTERRUPT_ENABLE      0x3
>  
> +#define ECC_POLL_MSEC			5000
> +
>  enum {
>  	LLCC_DRAM_CE = 0,
>  	LLCC_DRAM_UE,
> @@ -283,8 +285,7 @@ dump_syn_reg(struct edac_device_ctl_info *edev_ctl, int err_type, u32 bank)
>  	return ret;
>  }
>  
> -static irqreturn_t
> -llcc_ecc_irq_handler(int irq, void *edev_ctl)
> +static irqreturn_t llcc_ecc_irq_handler(int irq, void *edev_ctl)
>  {
>  	struct edac_device_ctl_info *edac_dev_ctl = edev_ctl;
>  	struct llcc_drv_data *drv = edac_dev_ctl->pvt_info;
> @@ -328,6 +329,11 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
>  	return irq_rc;
>  }
>  
> +static void llcc_ecc_check(struct edac_device_ctl_info *edev_ctl)
> +{
> +	llcc_ecc_irq_handler(0, edev_ctl);
> +}
> +
>  static int qcom_llcc_edac_probe(struct platform_device *pdev)
>  {
>  	struct llcc_drv_data *llcc_driv_data = pdev->dev.platform_data;
> @@ -356,22 +362,31 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
>  	edev_ctl->panic_on_ue = LLCC_ERP_PANIC_ON_UE;
>  	edev_ctl->pvt_info = llcc_driv_data;
>  
> +	/* Check if LLCC driver has passed ECC IRQ */
> +	ecc_irq = llcc_driv_data->ecc_irq;
> +	if (ecc_irq > 0) {
> +		/* Use interrupt mode if IRQ is available */
> +		edac_op_state = EDAC_OPSTATE_INT;
> +	} else {
> +		/* Fall back to polling mode otherwise */
> +		edac_op_state = EDAC_OPSTATE_POLL;
> +		edev_ctl->poll_msec = ECC_POLL_MSEC;
> +		edev_ctl->edac_check = llcc_ecc_check;
> +	}
> +
>  	rc = edac_device_add_device(edev_ctl);
>  	if (rc)
>  		goto out_mem;
>  
>  	platform_set_drvdata(pdev, edev_ctl);
>  
> -	/* Request for ecc irq */
> -	ecc_irq = llcc_driv_data->ecc_irq;
> -	if (ecc_irq < 0) {
> -		rc = -ENODEV;
> -		goto out_dev;
> -	}
> -	rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
> +	/* Request ECC IRQ if available */
> +	if (ecc_irq > 0) {
> +		rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
>  			      IRQF_TRIGGER_HIGH, "llcc_ecc", edev_ctl);
> -	if (rc)
> -		goto out_dev;
> +		if (rc)
> +			goto out_dev;
> +	}
>  
>  	return rc;
>  
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index a29f22dad7fa..e044e6756415 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -1011,13 +1011,12 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  		goto err;
>  
>  	drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
> -	if (drv_data->ecc_irq >= 0) {
> -		llcc_edac = platform_device_register_data(&pdev->dev,
> -						"qcom_llcc_edac", -1, drv_data,
> -						sizeof(*drv_data));
> -		if (IS_ERR(llcc_edac))
> -			dev_err(dev, "Failed to register llcc edac driver\n");
> -	}
> +
> +	llcc_edac = platform_device_register_data(&pdev->dev,
> +					"qcom_llcc_edac", -1, drv_data,
> +					sizeof(*drv_data));
> +	if (IS_ERR(llcc_edac))
> +		dev_err(dev, "Failed to register llcc edac driver\n");
>  
>  	return 0;
>  err:
> -- 
> 2.25.1

Re: [PATCH v2 13/13] qcom: llcc/edac: Support polling mode for ECC handling

[Manivannan Sadhasivam]

On Mon, Dec 12, 2022 at 04:53:49PM +0100, Luca Weiss wrote:
> Hi Manivannan,
> 
> On Mon Dec 12, 2022 at 1:33 PM CET, Manivannan Sadhasivam wrote:
> > Not all Qcom platforms support IRQ mode for ECC handling. For those
> > platforms, the current EDAC driver will not be probed due to missing ECC
> > IRQ in devicetree.
> >
> > So add support for polling mode so that the EDAC driver can be used on all
> > Qcom platforms supporting LLCC.
> >
> > The polling delay of 5000ms is chosed based on Qcom downstream/vendor
> > driver.
> 
> I think it does work for me on SM6350, I get this in dmesg:
> 
> [    0.054608] EDAC MC: Ver: 3.0.0
> [    0.273913] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (POLLED)
> 
> What I've noticed though is that the 5000ms poll you defined in the
> driver doesn't seem to be reflected at runtime? Or am I looking at
> different things?
> 
> / # cat /sys/devices/system/edac/qcom-llcc/poll_msec 
> 1000
> 

Oops... Looks like the interval is hardcoded in edac_device driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/edac/edac_device.c#n449

This clearly needs to be fixed. Will do so in next version.

Thanks a lot for testing the series. I will also consider this as your t-b tag
for SM6350 and driver patches.

Thanks,
Mani

> Regards
> Luca
> 
> >
> > Reported-by: Luca Weiss <luca.weiss@fairphone.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/edac/qcom_edac.c     | 37 +++++++++++++++++++++++++-----------
> >  drivers/soc/qcom/llcc-qcom.c | 13 ++++++-------
> >  2 files changed, 32 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> > index 5be93577fc03..f7afb5375293 100644
> > --- a/drivers/edac/qcom_edac.c
> > +++ b/drivers/edac/qcom_edac.c
> > @@ -76,6 +76,8 @@
> >  #define DRP0_INTERRUPT_ENABLE           BIT(6)
> >  #define SB_DB_DRP_INTERRUPT_ENABLE      0x3
> >  
> > +#define ECC_POLL_MSEC			5000
> > +
> >  enum {
> >  	LLCC_DRAM_CE = 0,
> >  	LLCC_DRAM_UE,
> > @@ -283,8 +285,7 @@ dump_syn_reg(struct edac_device_ctl_info *edev_ctl, int err_type, u32 bank)
> >  	return ret;
> >  }
> >  
> > -static irqreturn_t
> > -llcc_ecc_irq_handler(int irq, void *edev_ctl)
> > +static irqreturn_t llcc_ecc_irq_handler(int irq, void *edev_ctl)
> >  {
> >  	struct edac_device_ctl_info *edac_dev_ctl = edev_ctl;
> >  	struct llcc_drv_data *drv = edac_dev_ctl->pvt_info;
> > @@ -328,6 +329,11 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
> >  	return irq_rc;
> >  }
> >  
> > +static void llcc_ecc_check(struct edac_device_ctl_info *edev_ctl)
> > +{
> > +	llcc_ecc_irq_handler(0, edev_ctl);
> > +}
> > +
> >  static int qcom_llcc_edac_probe(struct platform_device *pdev)
> >  {
> >  	struct llcc_drv_data *llcc_driv_data = pdev->dev.platform_data;
> > @@ -356,22 +362,31 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
> >  	edev_ctl->panic_on_ue = LLCC_ERP_PANIC_ON_UE;
> >  	edev_ctl->pvt_info = llcc_driv_data;
> >  
> > +	/* Check if LLCC driver has passed ECC IRQ */
> > +	ecc_irq = llcc_driv_data->ecc_irq;
> > +	if (ecc_irq > 0) {
> > +		/* Use interrupt mode if IRQ is available */
> > +		edac_op_state = EDAC_OPSTATE_INT;
> > +	} else {
> > +		/* Fall back to polling mode otherwise */
> > +		edac_op_state = EDAC_OPSTATE_POLL;
> > +		edev_ctl->poll_msec = ECC_POLL_MSEC;
> > +		edev_ctl->edac_check = llcc_ecc_check;
> > +	}
> > +
> >  	rc = edac_device_add_device(edev_ctl);
> >  	if (rc)
> >  		goto out_mem;
> >  
> >  	platform_set_drvdata(pdev, edev_ctl);
> >  
> > -	/* Request for ecc irq */
> > -	ecc_irq = llcc_driv_data->ecc_irq;
> > -	if (ecc_irq < 0) {
> > -		rc = -ENODEV;
> > -		goto out_dev;
> > -	}
> > -	rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
> > +	/* Request ECC IRQ if available */
> > +	if (ecc_irq > 0) {
> > +		rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
> >  			      IRQF_TRIGGER_HIGH, "llcc_ecc", edev_ctl);
> > -	if (rc)
> > -		goto out_dev;
> > +		if (rc)
> > +			goto out_dev;
> > +	}
> >  
> >  	return rc;
> >  
> > diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> > index a29f22dad7fa..e044e6756415 100644
> > --- a/drivers/soc/qcom/llcc-qcom.c
> > +++ b/drivers/soc/qcom/llcc-qcom.c
> > @@ -1011,13 +1011,12 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> >  		goto err;
> >  
> >  	drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
> > -	if (drv_data->ecc_irq >= 0) {
> > -		llcc_edac = platform_device_register_data(&pdev->dev,
> > -						"qcom_llcc_edac", -1, drv_data,
> > -						sizeof(*drv_data));
> > -		if (IS_ERR(llcc_edac))
> > -			dev_err(dev, "Failed to register llcc edac driver\n");
> > -	}
> > +
> > +	llcc_edac = platform_device_register_data(&pdev->dev,
> > +					"qcom_llcc_edac", -1, drv_data,
> > +					sizeof(*drv_data));
> > +	if (IS_ERR(llcc_edac))
> > +		dev_err(dev, "Failed to register llcc edac driver\n");
> >  
> >  	return 0;
> >  err:
> > -- 
> > 2.25.1
> 

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

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Andrew Halaney]

On Mon, Dec 12, 2022 at 06:02:58PM +0530, Manivannan Sadhasivam wrote:
> The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
> This offset 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 with the current drivers. So far this crash is not reported since
> EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
> driver extensively by triggering the EDAC IRQ (that's where each bank
> CSRs are accessed).
>
> For fixing this issue, let's obtain the base address of each LLCC bank from
> devicetree and get rid of the fixed stride.
>
> This series affects multiple platforms but I have only tested this on
> SM8250 and SM8450. Testing on other platforms is welcomed.
>

Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride

I took this for a quick spin on the qdrive3 I've got access to without
any issue:

    [root@localhost ~]# modprobe qcom_edac
    [root@localhost ~]# dmesg | grep -i edac
    [    0.620723] EDAC MC: Ver: 3.0.0
    [    1.165417] ghes_edac: GHES probing device list is empty
    [  594.688103] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (INTERRUPT)
    [root@localhost ~]# cat /proc/interrupts | grep ecc
    174:          0          0          0          0          0          0          0          0     GICv3 614 Level     llcc_ecc
    [root@localhost ~]#

Potentially stupid question, but are users expected to manually load the
driver as I did? I don't see how it would be loaded automatically in the
current state, but thought it was funny that I needed to modprobe
myself.

Please let me know if you want me to do any more further testing!

Thanks,
Andrew

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

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>

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>

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>

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>

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 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Manivannan Sadhasivam]

On Mon, Dec 12, 2022 at 01:23:40PM -0600, Andrew Halaney wrote:
> On Mon, Dec 12, 2022 at 06:02:58PM +0530, Manivannan Sadhasivam wrote:
> > The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> > accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
> > This offset 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 with the current drivers. So far this crash is not reported since
> > EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
> > driver extensively by triggering the EDAC IRQ (that's where each bank
> > CSRs are accessed).
> >
> > For fixing this issue, let's obtain the base address of each LLCC bank from
> > devicetree and get rid of the fixed stride.
> >
> > This series affects multiple platforms but I have only tested this on
> > SM8250 and SM8450. Testing on other platforms is welcomed.
> >
> 
> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
> 

Thanks!

> I took this for a quick spin on the qdrive3 I've got access to without
> any issue:
> 
>     [root@localhost ~]# modprobe qcom_edac
>     [root@localhost ~]# dmesg | grep -i edac
>     [    0.620723] EDAC MC: Ver: 3.0.0
>     [    1.165417] ghes_edac: GHES probing device list is empty
>     [  594.688103] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (INTERRUPT)
>     [root@localhost ~]# cat /proc/interrupts | grep ecc
>     174:          0          0          0          0          0          0          0          0     GICv3 614 Level     llcc_ecc
>     [root@localhost ~]#
> 
> Potentially stupid question, but are users expected to manually load the
> driver as I did? I don't see how it would be loaded automatically in the
> current state, but thought it was funny that I needed to modprobe
> myself.
> 
> Please let me know if you want me to do any more further testing!
> 

Well, I always ended up using the driver as a built-in. I do make it module for
build test but never really used it as a module, so didn't catch this issue.

This is due to the module alias not exported by the qcom_edac driver. Below
diff allows kernel to autoload it:

diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
index f7afb5375293..13919d01c22d 100644
--- a/drivers/edac/qcom_edac.c
+++ b/drivers/edac/qcom_edac.c
@@ -419,3 +419,4 @@ module_platform_driver(qcom_llcc_edac_driver);
 
 MODULE_DESCRIPTION("QCOM EDAC driver");
 MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom_llcc_edac");

Please test and let me know. I will add this as a new patch in next version.

Thanks,
Mani

> Thanks,
> Andrew
> 

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

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Andrew Halaney]

On Tue, Dec 13, 2022 at 10:58:02AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Dec 12, 2022 at 01:23:40PM -0600, Andrew Halaney wrote:
> > On Mon, Dec 12, 2022 at 06:02:58PM +0530, Manivannan Sadhasivam wrote:
> > > The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> > > accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
> > > This offset 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 with the current drivers. So far this crash is not reported since
> > > EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
> > > driver extensively by triggering the EDAC IRQ (that's where each bank
> > > CSRs are accessed).
> > >
> > > For fixing this issue, let's obtain the base address of each LLCC bank from
> > > devicetree and get rid of the fixed stride.
> > >
> > > This series affects multiple platforms but I have only tested this on
> > > SM8250 and SM8450. Testing on other platforms is welcomed.
> > >
> > 
> > Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
> > 
> 
> Thanks!
> 
> > I took this for a quick spin on the qdrive3 I've got access to without
> > any issue:
> > 
> >     [root@localhost ~]# modprobe qcom_edac
> >     [root@localhost ~]# dmesg | grep -i edac
> >     [    0.620723] EDAC MC: Ver: 3.0.0
> >     [    1.165417] ghes_edac: GHES probing device list is empty
> >     [  594.688103] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (INTERRUPT)
> >     [root@localhost ~]# cat /proc/interrupts | grep ecc
> >     174:          0          0          0          0          0          0          0          0     GICv3 614 Level     llcc_ecc
> >     [root@localhost ~]#
> > 
> > Potentially stupid question, but are users expected to manually load the
> > driver as I did? I don't see how it would be loaded automatically in the
> > current state, but thought it was funny that I needed to modprobe
> > myself.
> > 
> > Please let me know if you want me to do any more further testing!
> > 
> 
> Well, I always ended up using the driver as a built-in. I do make it module for
> build test but never really used it as a module, so didn't catch this issue.
> 
> This is due to the module alias not exported by the qcom_edac driver. Below
> diff allows kernel to autoload it:
> 
> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> index f7afb5375293..13919d01c22d 100644
> --- a/drivers/edac/qcom_edac.c
> +++ b/drivers/edac/qcom_edac.c
> @@ -419,3 +419,4 @@ module_platform_driver(qcom_llcc_edac_driver);
>  
>  MODULE_DESCRIPTION("QCOM EDAC driver");
>  MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom_llcc_edac");
> 
> Please test and let me know. I will add this as a new patch in next version.
> 

Thanks Mani, that gets things working for me. For that patch:

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
Tested-by: Andrew Halaney <ahalaney@redhat.com>

My personal opinion, but that probably deserves a Fixes: tag too!

Re: [PATCH v2 01/13] dt-bindings: arm: msm: Update the maintainers for LLCC

[Krzysztof Kozlowski]

On 12/12/2022 13:32, Manivannan Sadhasivam wrote:
> Rishabh Bhatnagar has left Qualcomm, and there is no evidence of him
> maintaining with a new identity. So his entry needs to be removed.
> 
> Also, Sai Prakash Ranjan's email address should be updated to use
> quicinc domain.
> 

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

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

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

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 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 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Krzysztof Kozlowski]

On 13/12/2022 06:28, Manivannan Sadhasivam wrote:
> On Mon, Dec 12, 2022 at 01:23:40PM -0600, Andrew Halaney wrote:
>> On Mon, Dec 12, 2022 at 06:02:58PM +0530, Manivannan Sadhasivam wrote:
>>> The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
>>> accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
>>> This offset 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 with the current drivers. So far this crash is not reported since
>>> EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
>>> driver extensively by triggering the EDAC IRQ (that's where each bank
>>> CSRs are accessed).
>>>
>>> For fixing this issue, let's obtain the base address of each LLCC bank from
>>> devicetree and get rid of the fixed stride.
>>>
>>> This series affects multiple platforms but I have only tested this on
>>> SM8250 and SM8450. Testing on other platforms is welcomed.
>>>
>>
>> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
>>
> 
> Thanks!
> 
>> I took this for a quick spin on the qdrive3 I've got access to without
>> any issue:
>>
>>     [root@localhost ~]# modprobe qcom_edac
>>     [root@localhost ~]# dmesg | grep -i edac
>>     [    0.620723] EDAC MC: Ver: 3.0.0
>>     [    1.165417] ghes_edac: GHES probing device list is empty
>>     [  594.688103] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (INTERRUPT)
>>     [root@localhost ~]# cat /proc/interrupts | grep ecc
>>     174:          0          0          0          0          0          0          0          0     GICv3 614 Level     llcc_ecc
>>     [root@localhost ~]#
>>
>> Potentially stupid question, but are users expected to manually load the
>> driver as I did? I don't see how it would be loaded automatically in the
>> current state, but thought it was funny that I needed to modprobe
>> myself.
>>
>> Please let me know if you want me to do any more further testing!
>>
> 
> Well, I always ended up using the driver as a built-in. I do make it module for
> build test but never really used it as a module, so didn't catch this issue.
> 
> This is due to the module alias not exported by the qcom_edac driver. Below
> diff allows kernel to autoload it:
> 
> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> index f7afb5375293..13919d01c22d 100644
> --- a/drivers/edac/qcom_edac.c
> +++ b/drivers/edac/qcom_edac.c
> @@ -419,3 +419,4 @@ module_platform_driver(qcom_llcc_edac_driver);
>  
>  MODULE_DESCRIPTION("QCOM EDAC driver");
>  MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom_llcc_edac");

While this is a way to fix it, but instead of creating aliases for wrong
names, either a correct name should be used or driver should receive ID
table.

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

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

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
> 

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

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 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 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Manivannan Sadhasivam]

On Tue, Dec 13, 2022 at 05:54:56PM +0100, Krzysztof Kozlowski wrote:
> On 13/12/2022 06:28, Manivannan Sadhasivam wrote:
> > On Mon, Dec 12, 2022 at 01:23:40PM -0600, Andrew Halaney wrote:
> >> On Mon, Dec 12, 2022 at 06:02:58PM +0530, Manivannan Sadhasivam wrote:
> >>> The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> >>> accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
> >>> This offset 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 with the current drivers. So far this crash is not reported since
> >>> EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
> >>> driver extensively by triggering the EDAC IRQ (that's where each bank
> >>> CSRs are accessed).
> >>>
> >>> For fixing this issue, let's obtain the base address of each LLCC bank from
> >>> devicetree and get rid of the fixed stride.
> >>>
> >>> This series affects multiple platforms but I have only tested this on
> >>> SM8250 and SM8450. Testing on other platforms is welcomed.
> >>>
> >>
> >> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
> >>
> > 
> > Thanks!
> > 
> >> I took this for a quick spin on the qdrive3 I've got access to without
> >> any issue:
> >>
> >>     [root@localhost ~]# modprobe qcom_edac
> >>     [root@localhost ~]# dmesg | grep -i edac
> >>     [    0.620723] EDAC MC: Ver: 3.0.0
> >>     [    1.165417] ghes_edac: GHES probing device list is empty
> >>     [  594.688103] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (INTERRUPT)
> >>     [root@localhost ~]# cat /proc/interrupts | grep ecc
> >>     174:          0          0          0          0          0          0          0          0     GICv3 614 Level     llcc_ecc
> >>     [root@localhost ~]#
> >>
> >> Potentially stupid question, but are users expected to manually load the
> >> driver as I did? I don't see how it would be loaded automatically in the
> >> current state, but thought it was funny that I needed to modprobe
> >> myself.
> >>
> >> Please let me know if you want me to do any more further testing!
> >>
> > 
> > Well, I always ended up using the driver as a built-in. I do make it module for
> > build test but never really used it as a module, so didn't catch this issue.
> > 
> > This is due to the module alias not exported by the qcom_edac driver. Below
> > diff allows kernel to autoload it:
> > 
> > diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> > index f7afb5375293..13919d01c22d 100644
> > --- a/drivers/edac/qcom_edac.c
> > +++ b/drivers/edac/qcom_edac.c
> > @@ -419,3 +419,4 @@ module_platform_driver(qcom_llcc_edac_driver);
> >  
> >  MODULE_DESCRIPTION("QCOM EDAC driver");
> >  MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:qcom_llcc_edac");
> 
> While this is a way to fix it, but instead of creating aliases for wrong
> names, either a correct name should be used or driver should receive ID
> table.
> 

I'm not sure how you'd fix it with a _correct_ name here. Also, the id table is
an overkill since there is only one driver that is making use of it. And
moreover, there is no definite ID to use.

Thanks,
Mani

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

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

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Krzysztof Kozlowski]

On 13/12/2022 18:57, Manivannan Sadhasivam wrote:
> On Tue, Dec 13, 2022 at 05:54:56PM +0100, Krzysztof Kozlowski wrote:
>> On 13/12/2022 06:28, Manivannan Sadhasivam wrote:
>>> On Mon, Dec 12, 2022 at 01:23:40PM -0600, Andrew Halaney wrote:
>>>> On Mon, Dec 12, 2022 at 06:02:58PM +0530, Manivannan Sadhasivam wrote:
>>>>> The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
>>>>> accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
>>>>> This offset 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 with the current drivers. So far this crash is not reported since
>>>>> EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
>>>>> driver extensively by triggering the EDAC IRQ (that's where each bank
>>>>> CSRs are accessed).
>>>>>
>>>>> For fixing this issue, let's obtain the base address of each LLCC bank from
>>>>> devicetree and get rid of the fixed stride.
>>>>>
>>>>> This series affects multiple platforms but I have only tested this on
>>>>> SM8250 and SM8450. Testing on other platforms is welcomed.
>>>>>
>>>>
>>>> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
>>>>
>>>
>>> Thanks!
>>>
>>>> I took this for a quick spin on the qdrive3 I've got access to without
>>>> any issue:
>>>>
>>>>     [root@localhost ~]# modprobe qcom_edac
>>>>     [root@localhost ~]# dmesg | grep -i edac
>>>>     [    0.620723] EDAC MC: Ver: 3.0.0
>>>>     [    1.165417] ghes_edac: GHES probing device list is empty
>>>>     [  594.688103] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (INTERRUPT)
>>>>     [root@localhost ~]# cat /proc/interrupts | grep ecc
>>>>     174:          0          0          0          0          0          0          0          0     GICv3 614 Level     llcc_ecc
>>>>     [root@localhost ~]#
>>>>
>>>> Potentially stupid question, but are users expected to manually load the
>>>> driver as I did? I don't see how it would be loaded automatically in the
>>>> current state, but thought it was funny that I needed to modprobe
>>>> myself.
>>>>
>>>> Please let me know if you want me to do any more further testing!
>>>>
>>>
>>> Well, I always ended up using the driver as a built-in. I do make it module for
>>> build test but never really used it as a module, so didn't catch this issue.
>>>
>>> This is due to the module alias not exported by the qcom_edac driver. Below
>>> diff allows kernel to autoload it:
>>>
>>> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
>>> index f7afb5375293..13919d01c22d 100644
>>> --- a/drivers/edac/qcom_edac.c
>>> +++ b/drivers/edac/qcom_edac.c
>>> @@ -419,3 +419,4 @@ module_platform_driver(qcom_llcc_edac_driver);
>>>  
>>>  MODULE_DESCRIPTION("QCOM EDAC driver");
>>>  MODULE_LICENSE("GPL v2");
>>> +MODULE_ALIAS("platform:qcom_llcc_edac");
>>
>> While this is a way to fix it, but instead of creating aliases for wrong
>> names, either a correct name should be used or driver should receive ID
>> table.
>>
> 
> I'm not sure how you'd fix it with a _correct_ name here. 

Hm, I assumed that it would be enough if driver name would match device
name. Currently these two are not in sync. Maybe it's not enough when
built as module?

> Also, the id table is
> an overkill since there is only one driver that is making use of it. And
> moreover, there is no definite ID to use.

Every driver with a single device support has usually ID table and it's
not a problem...

Best regards,
Krzysztof

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Manivannan Sadhasivam]

On Tue, Dec 13, 2022 at 07:47:17PM +0100, Krzysztof Kozlowski wrote:
> On 13/12/2022 18:57, Manivannan Sadhasivam wrote:
> > On Tue, Dec 13, 2022 at 05:54:56PM +0100, Krzysztof Kozlowski wrote:
> >> On 13/12/2022 06:28, Manivannan Sadhasivam wrote:
> >>> On Mon, Dec 12, 2022 at 01:23:40PM -0600, Andrew Halaney wrote:
> >>>> On Mon, Dec 12, 2022 at 06:02:58PM +0530, Manivannan Sadhasivam wrote:
> >>>>> The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> >>>>> accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
> >>>>> This offset 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 with the current drivers. So far this crash is not reported since
> >>>>> EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
> >>>>> driver extensively by triggering the EDAC IRQ (that's where each bank
> >>>>> CSRs are accessed).
> >>>>>
> >>>>> For fixing this issue, let's obtain the base address of each LLCC bank from
> >>>>> devicetree and get rid of the fixed stride.
> >>>>>
> >>>>> This series affects multiple platforms but I have only tested this on
> >>>>> SM8250 and SM8450. Testing on other platforms is welcomed.
> >>>>>
> >>>>
> >>>> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
> >>>>
> >>>
> >>> Thanks!
> >>>
> >>>> I took this for a quick spin on the qdrive3 I've got access to without
> >>>> any issue:
> >>>>
> >>>>     [root@localhost ~]# modprobe qcom_edac
> >>>>     [root@localhost ~]# dmesg | grep -i edac
> >>>>     [    0.620723] EDAC MC: Ver: 3.0.0
> >>>>     [    1.165417] ghes_edac: GHES probing device list is empty
> >>>>     [  594.688103] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (INTERRUPT)
> >>>>     [root@localhost ~]# cat /proc/interrupts | grep ecc
> >>>>     174:          0          0          0          0          0          0          0          0     GICv3 614 Level     llcc_ecc
> >>>>     [root@localhost ~]#
> >>>>
> >>>> Potentially stupid question, but are users expected to manually load the
> >>>> driver as I did? I don't see how it would be loaded automatically in the
> >>>> current state, but thought it was funny that I needed to modprobe
> >>>> myself.
> >>>>
> >>>> Please let me know if you want me to do any more further testing!
> >>>>
> >>>
> >>> Well, I always ended up using the driver as a built-in. I do make it module for
> >>> build test but never really used it as a module, so didn't catch this issue.
> >>>
> >>> This is due to the module alias not exported by the qcom_edac driver. Below
> >>> diff allows kernel to autoload it:
> >>>
> >>> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> >>> index f7afb5375293..13919d01c22d 100644
> >>> --- a/drivers/edac/qcom_edac.c
> >>> +++ b/drivers/edac/qcom_edac.c
> >>> @@ -419,3 +419,4 @@ module_platform_driver(qcom_llcc_edac_driver);
> >>>  
> >>>  MODULE_DESCRIPTION("QCOM EDAC driver");
> >>>  MODULE_LICENSE("GPL v2");
> >>> +MODULE_ALIAS("platform:qcom_llcc_edac");
> >>
> >> While this is a way to fix it, but instead of creating aliases for wrong
> >> names, either a correct name should be used or driver should receive ID
> >> table.
> >>
> > 
> > I'm not sure how you'd fix it with a _correct_ name here. 
> 
> Hm, I assumed that it would be enough if driver name would match device
> name. Currently these two are not in sync. Maybe it's not enough when
> built as module?
> 

Right, for module it is not enough and that's why we need id_table/alias.

> > Also, the id table is
> > an overkill since there is only one driver that is making use of it. And
> > moreover, there is no definite ID to use.
> 
> Every driver with a single device support has usually ID table and it's
> not a problem...
> 

Are you referring to OF/ACPI ID table? Or something else?

Thanks,
Mani

> Best regards,
> Krzysztof
> 

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

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Krzysztof Kozlowski]

On 19/12/2022 14:50, Manivannan Sadhasivam wrote:
> 
>>> Also, the id table is
>>> an overkill since there is only one driver that is making use of it. And
>>> moreover, there is no definite ID to use.
>>
>> Every driver with a single device support has usually ID table and it's
>> not a problem...
>>
> 
> Are you referring to OF/ACPI ID table? Or something else?

No, I refer to the driver ID table (I2C, platform whatever the driver is).

Best regards,
Krzysztof

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Manivannan Sadhasivam]

On Mon, Dec 19, 2022 at 03:11:36PM +0100, Krzysztof Kozlowski wrote:
> On 19/12/2022 14:50, Manivannan Sadhasivam wrote:
> > 
> >>> Also, the id table is
> >>> an overkill since there is only one driver that is making use of it. And
> >>> moreover, there is no definite ID to use.
> >>
> >> Every driver with a single device support has usually ID table and it's
> >> not a problem...
> >>
> > 
> > Are you referring to OF/ACPI ID table? Or something else?
> 
> No, I refer to the driver ID table (I2C, platform whatever the driver is).
> 

Yeah, that's what I wanted to avoid here. The ID table makes sense if you have
a bus like I2C or a separate subsystem but here LLCC is an individual driver.
So creating a separate ID table is an overkill IMO.

Thanks,
Mani

> Best regards,
> Krzysztof
> 

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

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Krzysztof Kozlowski]

On 19/12/2022 15:16, Manivannan Sadhasivam wrote:
> On Mon, Dec 19, 2022 at 03:11:36PM +0100, Krzysztof Kozlowski wrote:
>> On 19/12/2022 14:50, Manivannan Sadhasivam wrote:
>>>
>>>>> Also, the id table is
>>>>> an overkill since there is only one driver that is making use of it. And
>>>>> moreover, there is no definite ID to use.
>>>>
>>>> Every driver with a single device support has usually ID table and it's
>>>> not a problem...
>>>>
>>>
>>> Are you referring to OF/ACPI ID table? Or something else?
>>
>> No, I refer to the driver ID table (I2C, platform whatever the driver is).
>>
> 
> Yeah, that's what I wanted to avoid here. The ID table makes sense if you have
> a bus like I2C or a separate subsystem but here LLCC is an individual driver.
> So creating a separate ID table is an overkill IMO.

Why this is an overkill? Just few lines and many, many drivers have it.
Even duplicated (for legacy reasons) with OF tables.

ALIAS is not the way to go around ID table because essentially you are
re-implementing it.

Best regards,
Krzysztof

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Dmitry Baryshkov]

On Mon, 19 Dec 2022 at 16:17, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Mon, Dec 19, 2022 at 03:11:36PM +0100, Krzysztof Kozlowski wrote:
> > On 19/12/2022 14:50, Manivannan Sadhasivam wrote:
> > >
> > >>> Also, the id table is
> > >>> an overkill since there is only one driver that is making use of it. And
> > >>> moreover, there is no definite ID to use.
> > >>
> > >> Every driver with a single device support has usually ID table and it's
> > >> not a problem...
> > >>
> > >
> > > Are you referring to OF/ACPI ID table? Or something else?
> >
> > No, I refer to the driver ID table (I2C, platform whatever the driver is).
> >
>
> Yeah, that's what I wanted to avoid here. The ID table makes sense if you have
> a bus like I2C or a separate subsystem but here LLCC is an individual driver.
> So creating a separate ID table is an overkill IMO.

Well, struct platform_device_id is used quite a lot together with the
MODULE_DEVICE_TABLE(platform, _ids);

On the other hand:

$ git grep MODULE_ALIAS.*platform: | wc -l
1308
$ git grep MODULE_DEVICE_TABLE.*platform | wc -l
236

-- 
With best wishes
Dmitry

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Manivannan Sadhasivam]

On Mon, Dec 19, 2022 at 06:49:39PM +0200, Dmitry Baryshkov wrote:
> On Mon, 19 Dec 2022 at 16:17, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Mon, Dec 19, 2022 at 03:11:36PM +0100, Krzysztof Kozlowski wrote:
> > > On 19/12/2022 14:50, Manivannan Sadhasivam wrote:
> > > >
> > > >>> Also, the id table is
> > > >>> an overkill since there is only one driver that is making use of it. And
> > > >>> moreover, there is no definite ID to use.
> > > >>
> > > >> Every driver with a single device support has usually ID table and it's
> > > >> not a problem...
> > > >>
> > > >
> > > > Are you referring to OF/ACPI ID table? Or something else?
> > >
> > > No, I refer to the driver ID table (I2C, platform whatever the driver is).
> > >
> >
> > Yeah, that's what I wanted to avoid here. The ID table makes sense if you have
> > a bus like I2C or a separate subsystem but here LLCC is an individual driver.
> > So creating a separate ID table is an overkill IMO.
> 
> Well, struct platform_device_id is used quite a lot together with the
> MODULE_DEVICE_TABLE(platform, _ids);
> 
> On the other hand:
> 
> $ git grep MODULE_ALIAS.*platform: | wc -l
> 1308
> $ git grep MODULE_DEVICE_TABLE.*platform | wc -l
> 236
> 

Hmm. I think I will just go with platform_device_id in the next version.

Thanks,
Mani

> -- 
> With best wishes
> Dmitry

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

Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC banks

[Manivannan Sadhasivam]

Hi Andrew,

On Mon, Dec 12, 2022 at 01:23:40PM -0600, Andrew Halaney wrote:
> On Mon, Dec 12, 2022 at 06:02:58PM +0530, Manivannan Sadhasivam wrote:
> > The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> > accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
> > This offset 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 with the current drivers. So far this crash is not reported since
> > EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
> > driver extensively by triggering the EDAC IRQ (that's where each bank
> > CSRs are accessed).
> >
> > For fixing this issue, let's obtain the base address of each LLCC bank from
> > devicetree and get rid of the fixed stride.
> >
> > This series affects multiple platforms but I have only tested this on
> > SM8250 and SM8450. Testing on other platforms is welcomed.
> >
> 
> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
> 

I dropped your tested-by tag in v3 as some of the patch content have been
changed. Please test v3 and share your feedback.

Thanks,
Mani

> I took this for a quick spin on the qdrive3 I've got access to without
> any issue:
> 
>     [root@localhost ~]# modprobe qcom_edac
>     [root@localhost ~]# dmesg | grep -i edac
>     [    0.620723] EDAC MC: Ver: 3.0.0
>     [    1.165417] ghes_edac: GHES probing device list is empty
>     [  594.688103] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (INTERRUPT)
>     [root@localhost ~]# cat /proc/interrupts | grep ecc
>     174:          0          0          0          0          0          0          0          0     GICv3 614 Level     llcc_ecc
>     [root@localhost ~]#
> 
> Potentially stupid question, but are users expected to manually load the
> driver as I did? I don't see how it would be loaded automatically in the
> current state, but thought it was funny that I needed to modprobe
> myself.
> 
> Please let me know if you want me to do any more further testing!
> 
> Thanks,
> Andrew
> 

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