dt-bindings: edac: Add DT bindings for Kryo EDAC
Subsystems (3)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/2 dt-bindings: edac: Add DT bindings for Kryo EDAC
This adds DT bindings for Kryo EDAC implemented with RAS extensions on KRYO{3,4}XX CPU cores for reporting of cache errors.
--- .../bindings/edac/qcom-kryo-edac.yaml | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +67 -0
@@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/edac/qcom-kryo-edac.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Kryo Error Detection and Correction(EDAC) + +maintainers: + - Sai Prakash Ranjan <[email protected]> + +description: | + Kryo EDAC is defined to describe on-chip error detection and correction + for the Kryo CPU cores which implement RAS extensions. It will report + all Single Bit Errors and Double Bit Errors found in L1/L2 caches in + in two registers ERXSTATUS_EL1 and ERXMISC0_EL1. L3-SCU cache errors + are reported in ERR1STATUS and ERR1MISC0 registers. + ERXSTATUS_EL1 - Selected Error Record Primary Status Register, EL1 + ERXMISC0_EL1 - Selected Error Record Miscellaneous Register 0, EL1 + ERR1STATUS - Error Record Primary Status Register + ERR1MISC0 - Error Record Miscellaneous Register 0 + Current implementation of Kryo ECC(Error Correcting Code) mechanism is + based on interrupts. + +properties: + compatible: + enum: + - qcom,kryo-edac + + interrupts: + minItems: 1 + maxItems: 4 + items: + - description: l1-l2 cache faultirq interrupt + - description: l1-l2 cache errirq interrupt + - description: l3-scu cache errirq interrupt + - description: l3-scu cache faultirq interrupt + + interrupt-names: + minItems: 1 + maxItems: 4 + items: + - const: l1-l2-faultirq + - const: l1-l2-errirq + - const: l3-scu-errirq + - const: l3-scu-faultirq + +required: + - compatible + - interrupts + - interrupt-names + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + kryo_edac { + compatible = "qcom,kryo-edac"; + interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>, + <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "l1-l2-faultirq", + "l1-l2-errirq", + "l3-scu-errirq", + "l3-scu-faultirq"; + };
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Rob Herring
maintainerQuoted 47 lines · Sai Prakash Ranjan
This adds DT bindings for Kryo EDAC implemented with RAS extensions on KRYO{3,4}XX CPU cores for reporting of cache errors.
Signed-off-by Sai Prakash Ranjan <[email protected]>--- .../bindings/edac/qcom-kryo-edac.yaml | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/edac/qcom-kryo-edac.yamlDocumentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +31
-0@@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/edac/qcom-kryo-edac.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Kryo Error Detection and Correction(EDAC) + +maintainers: + - Sai Prakash Ranjan <[email protected]> + +description: | + Kryo EDAC is defined to describe on-chip error detection and correction + for the Kryo CPU cores which implement RAS extensions. It will report + all Single Bit Errors and Double Bit Errors found in L1/L2 caches in + in two registers ERXSTATUS_EL1 and ERXMISC0_EL1. L3-SCU cache errors + are reported in ERR1STATUS and ERR1MISC0 registers. + ERXSTATUS_EL1 - Selected Error Record Primary Status Register, EL1 + ERXMISC0_EL1 - Selected Error Record Miscellaneous Register 0, EL1 + ERR1STATUS - Error Record Primary Status Register + ERR1MISC0 - Error Record Miscellaneous Register 0 + Current implementation of Kryo ECC(Error Correcting Code) mechanism is + based on interrupts. + +properties: + compatible: + enum: + - qcom,kryo-edac + + interrupts: + minItems: 1
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml line 41
Quoted 10 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +10
-0+ maxItems: 4 + items: + - description: l1-l2 cache faultirq interrupt + - description: l1-l2 cache errirq interrupt + - description: l3-scu cache errirq interrupt + - description: l3-scu cache faultirq interrupt + + interrupt-names: + minItems: 1 + maxItems: 4
You are saying only these combinations are valid:
l1-l2-faultirq
l1-l2-faultirq
l1-l2-errirq
l1-l2-faultirq
l1-l2-errirq
l3-scu-errirq
l1-l2-faultirq
l1-l2-errirq
l3-scu-errirq
l3-scu-faultirq
Is that your intent?
Quoted 30 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +26
-0+ items: + - const: l1-l2-faultirq + - const: l1-l2-errirq + - const: l3-scu-errirq + - const: l3-scu-faultirq + +required: + - compatible + - interrupts + - interrupt-names + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + kryo_edac { + compatible = "qcom,kryo-edac"; + interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>, + <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "l1-l2-faultirq", + "l1-l2-errirq", + "l3-scu-errirq", + "l3-scu-faultirq"; + };Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Rob,
Quoted 79 lines · Rob Herring
Quoted 56 lines · Sai Prakash Ranjan
This adds DT bindings for Kryo EDAC implemented with RAS extensions on KRYO{3,4}XX CPU cores for reporting of cache errors.
Signed-off-by Sai Prakash Ranjan <[email protected]>--- .../bindings/edac/qcom-kryo-edac.yaml | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yamlDocumentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +13
-0@@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/edac/qcom-kryo-edac.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Kryo Error Detection and Correction(EDAC) + +maintainers: + - Sai Prakash Ranjan <[email protected]> + +description: | + Kryo EDAC is defined to describe on-chip error detection andcorrection
+ for the Kryo CPU cores which implement RAS extensions. It will report
+ all Single Bit Errors and Double Bit Errors found in L1/L2 caches in
+ in two registersERXSTATUS_EL1andERXMISC0_EL1. L3-SCU cache errorsDocumentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +2
-0+ are reported in ERR1STATUS and ERR1MISC0 registers. + ERXSTATUS_EL1 - Selected Error Record Primary Status Register,EL1
+ERXMISC0_EL1- Selected Error Record Miscellaneous Register 0,
EL1Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +3
-0+ ERR1STATUS - Error Record Primary Status Register + ERR1MISC0 - Error Record Miscellaneous Register 0 + Current implementation of Kryo ECC(Error Correcting Code) mechanismis
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +9
-0+ based on interrupts. + +properties: + compatible: + enum: + - qcom,kryo-edac + + interrupts: + minItems: 1Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml line 41
Quoted 10 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +10
-0+ maxItems: 4 + items: + - description: l1-l2 cache faultirq interrupt + - description: l1-l2 cache errirq interrupt + - description: l3-scu cache errirq interrupt + - description: l3-scu cache faultirq interrupt + + interrupt-names: + minItems: 1 + maxItems: 4You are saying only these combinations are valid:
l1-l2-faultirq
l1-l2-faultirq
l1-l2-errirql1-l2-faultirq
l1-l2-errirq
l3-scu-errirq
Quoted 7 lines
l1-l2-faultirq
l1-l2-errirq
l3-scu-errirq
l3-scu-faultirqIs that your intent?
No, I want any combination of interrupts to be valid with atleast one interrupt as mandatory. I thought specifying minItems as 1 and maxItems as 4 will take care of this, am I doing something wrong?
Thanks,
Sai
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Rob Herring
maintainerQuoted 90 lines · Sai Prakash Ranjan
Hi Rob,
Quoted 86 lines · Rob Herring
Quoted 58 lines · Sai Prakash Ranjan
This adds DT bindings for Kryo EDAC implemented with RAS extensions on KRYO{3,4}XX CPU cores for reporting of cache errors.
Signed-off-by Sai Prakash Ranjan <[email protected]>--- .../bindings/edac/qcom-kryo-edac.yaml | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yamldiff --git
a/Documentation/devicetree/bindings/edac/qcom-kryo-edac.yamlb/Documentation/devicetree/bindings/edac/qcom-kryo-edac.yamlnew file mode 100644
index 000000000000..1a39429a73b4Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +13
-0@@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/edac/qcom-kryo-edac.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Kryo Error Detection and Correction(EDAC) + +maintainers: + - Sai Prakash Ranjan <[email protected]> + +description: | + Kryo EDAC is defined to describe on-chip error detection andcorrection
+ for the Kryo CPU cores which implement RAS extensions. It will report
+ all Single Bit Errors and Double Bit Errors found in L1/L2 caches in
+ in two registersERXSTATUS_EL1andERXMISC0_EL1. L3-SCU cache errorsDocumentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +2
-0+ are reported in ERR1STATUS and ERR1MISC0 registers. + ERXSTATUS_EL1 - Selected Error Record Primary Status Register,EL1
+ERXMISC0_EL1- Selected Error Record Miscellaneous Register 0,
EL1Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +3
-0+ ERR1STATUS - Error Record Primary Status Register + ERR1MISC0 - Error Record Miscellaneous Register 0 + Current implementation of Kryo ECC(Error Correcting Code) mechanismis
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +9
-0+ based on interrupts. + +properties: + compatible: + enum: + - qcom,kryo-edac + + interrupts: + minItems: 1Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml line 41
Quoted 10 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +10
-0+ maxItems: 4 + items: + - description: l1-l2 cache faultirq interrupt + - description: l1-l2 cache errirq interrupt + - description: l3-scu cache errirq interrupt + - description: l3-scu cache faultirq interrupt + + interrupt-names: + minItems: 1 + maxItems: 4You are saying only these combinations are valid:
l1-l2-faultirq
l1-l2-faultirq
l1-l2-errirql1-l2-faultirq
l1-l2-errirq
l3-scu-errirql1-l2-faultirq
l1-l2-errirq
l3-scu-errirq
l3-scu-faultirq
Quoted 7 lines
Quoted 2 lines
Is that your intent?
No, I want any combination of interrupts to be valid with atleast one interrupt as mandatory.
I thought specifying minItems as 1 and maxItems as 4 will take care of this, am I doing something wrong?
Interrupts (really all properties) have a defined order in DT and an 'items' list defines both the order and index. You'll need to use oneOf and list out the possibilities. Stick to ones you actually need.
Rob
Quoted 12 lines · Rob Herring
Quoted 7 lines
Quoted 2 lines
Is that your intent?
No, I want any combination of interrupts to be valid with atleast one interrupt as mandatory.
I thought specifying minItems as 1 and maxItems as 4 will take care of this, am I doing something wrong?Interrupts (really all properties) have a defined order in DT and an 'items' list defines both the order and index. You'll need to use oneOf and list out the possibilities. Stick to ones you actually need.
Thanks, I will make the change in the next spin.
-Sai
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Sai,
(CC: +Tyler)
Quoted 3 lines · Sai Prakash Ranjan
This adds DT bindings for Kryo EDAC implemented with RAS extensions on KRYO{3,4}XX CPU cores for reporting of cache errors.
KRYO{3,4}XX isn't the only SoC with the RAS extensions. The DT needs to convey the range of ways this armv8 RAS extensions stuff can be wired up.
The folk who look after the ACPI specs have made a start:
https://static.docs.arm.com/den0085/a/DEN0085_RAS_ACPI_1.0_BETA_1.pdf
(I suspect that isn't the latest version, I'll try and find out)
I'd like the ACPI table and DT to convey the same information so that we don't need to convert or infer things in the driver. If something is missing, we should get it added!
Quoted 10 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +4
-0@@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/edac/qcom-kryo-edac.yaml#
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml line 14
Quoted 10 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +10
-0+$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Kryo Error Detection and Correction(EDAC) + +maintainers: + - Sai Prakash Ranjan <[email protected]> + +description: | + Kryo EDAC is defined to describe on-chip error detection and correction + for the Kryo CPU cores which implement RAS extensions.
Please don't make this Kryo specific, otherwise this binding becomes an extra thing we need to support with a 'v8.2 RAS' driver.
What I'd like is a single 'armv82_ras' edac driver that handles faults and errors reported by interrupts, and interacts with the arch code's handling of 'external aborts'. This should work for all platforms using v8.2 RAS and later.
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +10
-0+ It will report + all Single Bit Errors and Double Bit Errors found in L1/L2 caches in + in two registers ERXSTATUS_EL1 and ERXMISC0_EL1. L3-SCU cache errors + are reported in ERR1STATUS and ERR1MISC0 registers. + ERXSTATUS_EL1 - Selected Error Record Primary Status Register, EL1 + ERXMISC0_EL1 - Selected Error Record Miscellaneous Register 0, EL1 + ERR1STATUS - Error Record Primary Status Register + ERR1MISC0 - Error Record Miscellaneous Register 0 + Current implementation of Kryo ECC(Error Correcting Code) mechanism is + based on interrupts.
Your SoC picked the system registers as the interface to these component's registers. The binding would need to specify which index the 'l1-l2' records start at, and how many there are. The same for the 'l3-scu'. You can't hard code these, they are different on other platforms.
There is also an MMIO interface which needs a base address, along with the index and ranges. (which may be different). The same component may use both the system register and the MMIO interface.
This stuff is likely to vary on big/little systems, so you need a way of describing which CPUs the settings refer to. This probably isn't something the ACPI tables capture as ACPI machines are typically homogenous.
Thanks,
James
Hi James,
Quoted 4 lines · James Morse
Hi Sai,
(CC: +Tyler)
Quoted 9 lines
Quoted 3 lines · Sai Prakash Ranjan
This adds DT bindings for Kryo EDAC implemented with RAS extensions on KRYO{3,4}XX CPU cores for reporting of cache errors.
KRYO{3,4}XX isn't the only SoC with the RAS extensions. The DT needs to convey the range
of ways this armv8 RAS extensions stuff can be wired up.
Right, but I was going for Kryo specific implementation and hence the binding as such.
Quoted 5 lines
The folk who look after the ACPI specs have made a start:
https://static.docs.arm.com/den0085/a/DEN0085_RAS_ACPI_1.0_BETA_1.pdf(I suspect that isn't the latest version, I'll try and find out)
That would be helpful, thanks.
Quoted 5 lines
I'd like the ACPI table and DT to convey the same information so that we don't need to
convert or infer things in the driver. If something is missing, we should get it added!
Sure, I think it is decided now that kernel first RAS implementation will be generic.
Quoted 27 lines
Quoted 20 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +13
-0@@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/edac/qcom-kryo-edac.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Kryo Error Detection and Correction(EDAC) + +maintainers: + - Sai Prakash Ranjan <[email protected]> + +description: | + Kryo EDAC is defined to describe on-chip error detection andQuoted 2 lines
correction
+ for the Kryo CPU cores which implement RAS extensions.Please don't make this Kryo specific, otherwise this binding becomes an extra thing we
need to support with a 'v8.2 RAS' driver.
Quoted 7 lines
What I'd like is a single '
armv82_ras' edac driver that handles faults and errors reported
by interrupts, and interacts with the arch code's handling of 'external aborts'. This
should work for all platforms using v8.2 RAS and later.
Ok sure.
Quoted 16 lines
Quoted 13 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +2
-0+ It will report + all Single Bit Errors and Double Bit Errors found in L1/L2 cachesin
+ in two registersERXSTATUS_EL1andERXMISC0_EL1. L3-SCU cache errorsDocumentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +2
-0+ are reported in ERR1STATUS and ERR1MISC0 registers. + ERXSTATUS_EL1 - Selected Error Record Primary Status Register,EL1
+ERXMISC0_EL1- Selected Error Record Miscellaneous Register 0,
EL1Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +3
-0+ ERR1STATUS - Error Record Primary Status Register + ERR1MISC0 - Error Record Miscellaneous Register 0 + Current implementation of Kryo ECC(Error Correcting Code) mechanismQuoted 2 lines
is
+ based on interrupts.
Quoted 8 lines
Your SoC picked the system registers as the interface to these component's registers.
The binding would need to specify which index the 'l1-l2' records start at, and how many
there are. The same for the 'l3-scu'. You can't hard code these, they are different on
other platforms.
Ok will keep this in mind for the next version.
Quoted 6 lines
There is also an MMIO interface which needs a base address, along with the index and
ranges. (which may be different). The same component may use both the system register and
the MMIO interface.
I have some doubts here, Where do I get this info? Will this be implementation specific?
Quoted 6 lines
This stuff is likely to vary on big/little systems, so you need a way of describing which
CPUs the settings refer to. This probably isn't something the ACPI tables capture as ACPI
machines are typically homogenous.
Our SoCs are based on big.LITTLE arch, so this will be needed.
Thanks,
Sai
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Sai,
Quoted 18 lines · Sai Prakash Ranjan
Quoted 5 lines · James Morse
Quoted 3 lines · Sai Prakash Ranjan
This adds DT bindings for Kryo EDAC implemented with RAS extensions on KRYO{3,4}XX CPU cores for reporting of cache errors.
Quoted 11 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml
Quoted 6 lines
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +0
-0There is also an MMIO interface which needs a base address, along with the index and
ranges. (which may be different). The same component may use both the system register and the MMIO interface.
Quoted 1 line
I have some doubts here, Where do I get this info? Will this be implementation specific?
It will be implementation specific. The ACPI spec folk have gathered some of the range of ways people are putting this together. We should take that into account with the binding, otherwise we end up with a 'v1' and 'v2' of the binding and have to support both.
There is a 'Beta 2' of that ACPI document. It should appear on the website at some point. Qualcomm should have this somewhere, its called 'DEN0085_RAS_ACPI_1.0_RELEASE_BETA2.pdf.
Thanks,
James
PATCH 1/2 dt-bindings: edac: Add DT bindings for Kryo EDAC
In reply to a message not in the archiveThis adds DT bindings for Kryo EDAC implemented with RAS extensions on KRYO{3,4}XX CPU cores for reporting of cache errors.
--- .../bindings/edac/qcom-kryo-edac.yaml | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml
Documentation/devicetree/bindings/edac/qcom-kryo-edac.yaml new file +67 -0
@@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/edac/qcom-kryo-edac.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Kryo Error Detection and Correction(EDAC) + +maintainers: + - Sai Prakash Ranjan <[email protected]> + +description: | + Kryo EDAC is defined to describe on-chip error detection and correction + for the Kryo CPU cores which implement RAS extensions. It will report + all Single Bit Errors and Double Bit Errors found in L1/L2 caches in + in two registers ERXSTATUS_EL1 and ERXMISC0_EL1. L3-SCU cache errors + are reported in ERR1STATUS and ERR1MISC0 registers. + ERXSTATUS_EL1 - Selected Error Record Primary Status Register, EL1 + ERXMISC0_EL1 - Selected Error Record Miscellaneous Register 0, EL1 + ERR1STATUS - Error Record Primary Status Register + ERR1MISC0 - Error Record Miscellaneous Register 0 + Current implementation of Kryo ECC(Error Correcting Code) mechanism is + based on interrupts. + +properties: + compatible: + enum: + - qcom,kryo-edac + + interrupts: + minItems: 1 + maxItems: 4 + items: + - description: l1-l2 cache faultirq interrupt + - description: l1-l2 cache errirq interrupt + - description: l3-scu cache errirq interrupt + - description: l3-scu cache faultirq interrupt + + interrupt-names: + minItems: 1 + maxItems: 4 + items: + - const: l1-l2-faultirq + - const: l1-l2-errirq + - const: l3-scu-errirq + - const: l3-scu-faultirq + +required: + - compatible + - interrupts + - interrupt-names + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + kryo_edac { + compatible = "qcom,kryo-edac"; + interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>, + <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "l1-l2-faultirq", + "l1-l2-errirq", + "l3-scu-errirq", + "l3-scu-faultirq"; + };
Signature · 8 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 2/2 drivers: edac: Add EDAC support for Kryo CPU caches
In reply to a message not in the archiveKryo{3,4}XX CPU cores implement RAS extensions to support Error Correcting Code(ECC). Currently all Kryo{3,4}XX CPU cores (gold/silver a.k.a big/LITTLE) support ECC via RAS. This adds an interrupt based driver for those CPUs and provides an optional polling of error recording system registers.
--- MAINTAINERS | 7 + drivers/edac/Kconfig | 20 + drivers/edac/Makefile | 1 + drivers/edac/qcom_kryo_edac.c | 679 ++++++++++++++++++++++++++++++++++ 4 files changed, 707 insertions(+) create mode 100644 drivers/edac/qcom_kryo_edac.c
MAINTAINERS +7 -0
@@ -6049,6 +6049,13 @@ L: [email protected] S: Maintained F: drivers/edac/qcom_edac.c +EDAC-KRYO-QCOM +M: Sai Prakash Ranjan <[email protected]> +L: [email protected] +L: [email protected] +S: Maintained +F: drivers/edac/qcom_kryo_edac.c + EDIROL UA-101/UA-1000 DRIVER M: Clemens Ladisch <[email protected]> L: [email protected] (moderated for non-subscribers)
drivers/edac/Kconfig +20 -0
@@ -508,6 +508,26 @@ config EDAC_QCOM For debugging issues having to do with stability and overall system health, you should probably say 'Y' here. +config EDAC_QCOM_KRYO + tristate "QCOM Kryo EDAC for CPU L1/L2/L3-SCU caches" + depends on ARCH_QCOM && ARM64_RAS_EXTN + help + Support for Error detection and correction on Kryo Gold and Silver CPU + cores with RAS extensions. Currently it detects and reports all Single + Bit Errors (SBEs) and Double Bit Errors (DBEs). + + For debugging issues having to do with stability and overall system + health, you should probably say 'Y' here. + +config EDAC_QCOM_KRYO_POLL + depends on EDAC_QCOM_KRYO + bool "Poll on Kryo ECC registers" + help + This option chooses whether or not you want to poll on the Kryo ECC + registers. When this is enabled, the polling rate can be set as a + module parameter. By default, it will call the polling function every + second. + config EDAC_ASPEED tristate "Aspeed AST 2500 SoC" depends on MACH_ASPEED_G5
drivers/edac/Makefile +1 -0
@@ -85,5 +85,6 @@ obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o obj-$(CONFIG_EDAC_TI) += ti_edac.o obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o +obj-$(CONFIG_EDAC_QCOM_KRYO) += qcom_kryo_edac.o obj-$(CONFIG_EDAC_ASPEED) += aspeed_edac.o obj-$(CONFIG_EDAC_BLUEFIELD) += bluefield_edac.o
drivers/edac/qcom_kryo_edac.c new file +679 -0
679 lines not on this page · raw message
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 2/2 drivers: edac: Add EDAC support for Kryo CPU caches
In reply to a message not in the archiveKryo{3,4}XX CPU cores implement RAS extensions to support Error Correcting Code(ECC). Currently all Kryo{3,4}XX CPU cores (gold/silver a.k.a big/LITTLE) support ECC via RAS. This adds an interrupt based driver for those CPUs and provides an optional polling of error recording system registers.
--- MAINTAINERS | 7 + drivers/edac/Kconfig | 20 + drivers/edac/Makefile | 1 + drivers/edac/qcom_kryo_edac.c | 679 ++++++++++++++++++++++++++++++++++ 4 files changed, 707 insertions(+) create mode 100644 drivers/edac/qcom_kryo_edac.c
MAINTAINERS +7 -0
@@ -6049,6 +6049,13 @@ L: [email protected] S: Maintained F: drivers/edac/qcom_edac.c +EDAC-KRYO-QCOM +M: Sai Prakash Ranjan <[email protected]> +L: [email protected] +L: [email protected] +S: Maintained +F: drivers/edac/qcom_kryo_edac.c + EDIROL UA-101/UA-1000 DRIVER M: Clemens Ladisch <[email protected]> L: [email protected] (moderated for non-subscribers)
drivers/edac/Kconfig +20 -0
@@ -508,6 +508,26 @@ config EDAC_QCOM For debugging issues having to do with stability and overall system health, you should probably say 'Y' here. +config EDAC_QCOM_KRYO + tristate "QCOM Kryo EDAC for CPU L1/L2/L3-SCU caches" + depends on ARCH_QCOM && ARM64_RAS_EXTN + help + Support for Error detection and correction on Kryo Gold and Silver CPU + cores with RAS extensions. Currently it detects and reports all Single + Bit Errors (SBEs) and Double Bit Errors (DBEs). + + For debugging issues having to do with stability and overall system + health, you should probably say 'Y' here. + +config EDAC_QCOM_KRYO_POLL + depends on EDAC_QCOM_KRYO + bool "Poll on Kryo ECC registers" + help + This option chooses whether or not you want to poll on the Kryo ECC + registers. When this is enabled, the polling rate can be set as a + module parameter. By default, it will call the polling function every + second. + config EDAC_ASPEED tristate "Aspeed AST 2500 SoC" depends on MACH_ASPEED_G5
drivers/edac/Makefile +1 -0
@@ -85,5 +85,6 @@ obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o obj-$(CONFIG_EDAC_TI) += ti_edac.o obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o +obj-$(CONFIG_EDAC_QCOM_KRYO) += qcom_kryo_edac.o obj-$(CONFIG_EDAC_ASPEED) += aspeed_edac.o obj-$(CONFIG_EDAC_BLUEFIELD) += bluefield_edac.o
drivers/edac/qcom_kryo_edac.c new file +679 -0
679 lines not on this page · raw message
Signature · 8 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
A message that is not in the archive (sent off-list or not yet received): 0101016ed57a6311-e815485c-4b77-4342-a3de-203673941602-000000@us-west-2.amazonses.com
Re: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches
Hi Sai,
Quoted 181 lines · Sai Prakash Ranjan
Kryo{3,4}XX CPU cores implement RAS extensions to support Error Correcting Code(ECC). Currently all Kryo{3,4}XX CPU cores (gold/silver a.k.a big/LITTLE) support ECC via RAS. This adds an interrupt based driver for those CPUs and provides an optional polling of error recording system registers.
Signed-off-by Sai Prakash Ranjan <[email protected]>--- MAINTAINERS | 7 + drivers/edac/Kconfig | 20 + drivers/edac/Makefile | 1 + drivers/edac/qcom_kryo_edac.c | 679 ++++++++++++++++++++++++++++++++++ 4 files changed, 707 insertions(+) create mode 100644 drivers/edac/qcom_kryo_edac.cMAINTAINERS +7
-0@@ -6049,6 +6049,13 @@ L: [email protected] S: Maintained F: drivers/edac/qcom_edac.c +EDAC-KRYO-QCOM +M: Sai Prakash Ranjan <[email protected]> +L: [email protected] +L: [email protected] +S: Maintained +F: drivers/edac/qcom_kryo_edac.c + EDIROL UA-101/UA-1000 DRIVER M: Clemens Ladisch <[email protected]> L: [email protected] (moderated for non-subscribers)drivers/edac/Kconfig +20
-0@@ -508,6 +508,26 @@ config EDAC_QCOM For debugging issues having to do with stability and overall system health, you should probably say 'Y' here. +config EDAC_QCOM_KRYO + tristate "QCOM Kryo EDAC for CPU L1/L2/L3-SCU caches" + depends on ARCH_QCOM && ARM64_RAS_EXTN + help + Support for Error detection and correction on Kryo Gold and Silver CPU + cores with RAS extensions. Currently it detects and reports all Single + Bit Errors (SBEs) and Double Bit Errors (DBEs). + + For debugging issues having to do with stability and overall system + health, you should probably say 'Y' here. + +config EDAC_QCOM_KRYO_POLL + depends on EDAC_QCOM_KRYO + bool "Poll on Kryo ECC registers" + help + This option chooses whether or not you want to poll on the Kryo ECC + registers. When this is enabled, the polling rate can be set as a + module parameter. By default, it will call the polling function every + second. + config EDAC_ASPEED tristate "Aspeed AST 2500 SoC" depends on MACH_ASPEED_G5drivers/edac/Makefile +1
-0@@ -85,5 +85,6 @@ obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o obj-$(CONFIG_EDAC_TI) += ti_edac.o obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o +obj-$(CONFIG_EDAC_QCOM_KRYO) += qcom_kryo_edac.o obj-$(CONFIG_EDAC_ASPEED) += aspeed_edac.o obj-$(CONFIG_EDAC_BLUEFIELD) += bluefield_edac.odrivers/edac/qcom_kryo_edac.c new file +98
-0@@ -0,0 +1,679 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2019, The Linux Foundation. All rights reserved. +*/ + +#include <linux/bitfield.h> +#include <linux/cpu_pm.h> +#include <linux/of_irq.h> +#include <linux/smp.h> + +#include <asm/cputype.h> +#include <asm/sysreg.h> + +#include "edac_device.h" +#include "edac_mc.h" + +#define DRV_NAME "qcom_kryo_edac" + +/* +* ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.3 +* ARM DSU TRM Chapter B2.3 +* CFI = Corrected Fault Handling interrupt, FI = Fault handling interrupt +* UI = Uncorrected error recovery interrupt, ED = Error Detection +*/ +#define KRYO_ERRXCTLR_ED BIT(0) +#define KRYO_ERRXCTLR_UI BIT(2) +#define KRYO_ERRXCTLR_FI BIT(3) +#define KRYO_ERRXCTLR_CFI BIT(8) +#define KRYO_ERRXCTLR_ENABLE (KRYO_ERRXCTLR_CFI | KRYO_ERRXCTLR_FI | \ + KRYO_ERRXCTLR_UI | KRYO_ERRXCTLR_ED) + +/* +* ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.4 +* ARM DSU TRM Chapter B2.4 +*/ +#define KRYO_ERRXFR_ED GENMASK(1, 0) +#define KRYO_ERRXFR_DE GENMASK(3, 2) +#define KRYO_ERRXFR_UI GENMASK(5, 4) +#define KRYO_ERRXFR_FI GENMASK(7, 6) +#define KRYO_ERRXFR_UE GENMASK(9, 8) +#define KRYO_ERRXFR_CFI GENMASK(11, 10) +#define KRYO_ERRXFR_CEC GENMASK(14, 12) +#define KRYO_ERRXFR_RP BIT(15) +#define KRYO_ERRXFR_SUPPORTED (KRYO_ERRXFR_ED | KRYO_ERRXFR_DE | \ + KRYO_ERRXFR_UI | KRYO_ERRXFR_FI | \ + KRYO_ERRXFR_UE | KRYO_ERRXFR_CFI | \ + KRYO_ERRXFR_CEC | KRYO_ERRXFR_RP) + +/* +* ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.5 +* ARM DSU TRM Chapter B2.5 +*/ +#define KRYO_ERRXMISC0_CECR GENMASK_ULL(38, 32) +#define KRYO_ERRXMISC0_CECO GENMASK_ULL(46, 40) + +/* ARM Cortex-A76 TRM Chapter B3.5 */ +#define KRYO_ERRXMISC0_UNIT GENMASK(3, 0) +#define KRYO_ERRXMISC0_LVL GENMASK(3, 1) + +/* ARM Cortex-A76 TRM Chapter B3.10 has SERR bitfields 4:0 +* but Cortex-A55, Cortex-A75 and DSU TRM has SERR bitfields 7:0. +* Since max error record is 21, we can use bitfields 4:0 for +* Kryo{3,4}XX CPUs. +*/ +#define KRYO_ERRXSTATUS_SERR GENMASK(4, 0) +#define KRYO_ERRXSTATUS_DE BIT(23) +#define KRYO_ERRXSTATUS_CE GENMASK(25, 24) +#define KRYO_ERRXSTATUS_MV BIT(26) +#define KRYO_ERRXSTATUS_UE BIT(29) +#define KRYO_ERRXSTATUS_VALID BIT(30) + +/* ARM Cortex-A76 TRM Chapter B3.5 +* IC = Instruction Cache, DC = Data Cache +*/ +#define KRYO_L1_UNIT_IC 0x1 +#define KRYO_L2_UNIT_TLB 0x2 +#define KRYO_L1_UNIT_DC 0x4 +#define KRYO_L2_UNIT 0x8 + +/* +* ARM Cortex-A55 TRM Chapter B2.36 +* ARM Cortex-A75, Cortex-A76 TRM Chapter B2.37 +*/ +#define KRYO_ERR_RECORD_L1_L2 0x0 +#define KRYO_ERR_RECORD_L3 0x1 + +/* ARM DSU TRM Chapter B2.10 */ +#define BUS_ERROR 0x12 + +/* QCOM Kryo CPU part numbers */ +#define KRYO3XX_GOLD 0x802 +#define KRYO4XX_GOLD 0x804 +#define KRYO4XX_SILVER_V1 0x803 +#define KRYO4XX_SILVER_V2 0x805 + +#define KRYO_EDAC_MSG_MAX 256 + +static int poll_msec =1000 ;
drivers/edac/qcom_kryo_edac.c line 108
Quoted 10 lines
drivers/edac/qcom_kryo_edac.c new file +10
-0+module_param (poll_msec, int,0444 ); + +enum { +KRYO_L1 =0 , +KRYO_L2 , +KRYO_L3 , +}; + +/* CE = Corrected Error, UE = Uncorrected Error, DE = Deferred Error */ +enum {
No name?
Quoted 38 lines
drivers/edac/qcom_kryo_edac.c new file +38
-0+KRYO_L1_CE =0 , +KRYO_L1_UE , +KRYO_L1_DE , +KRYO_L2_CE , +KRYO_L2_UE , +KRYO_L2_DE , +KRYO_L3_CE , +KRYO_L3_UE , + KRYO_L3_DE, +}; + +struct error_record { +u32 error_code; +const char *error_msg; +}; + +struct error_type { +void (*fn)(struct edac_device_ctl_info *edev_ctl, +int inst_nr,int block_nr,const char *msg); +const char *msg; +}; + +/* +* ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.10 +* ARM DSU TRM Chapter B2.10 +*/ +static const struct error_record serror_record[] = { + {0x1 ,"Errors due to fault injection" }, + {0x2 ,"ECC error from internal data buffer" }, + {0x6 ,"ECC error on cache data RAM" }, + {0x7 ,"ECC error on cache tag or dirty RAM" }, + {0x8 ,"Parity error on TLB data RAM" }, + {0x9 ,"Parity error on TLB tag RAM" }, + {0x12 ,"Error response for a cache copyback" }, + {0x15 ,"Deferred error not supported" }, +}; + +static const struct error_type err_type[] = {
drivers/edac/qcom_kryo_edac.c line 156
Quoted 10 lines
drivers/edac/qcom_kryo_edac.c new file +10
-0+ { edac_device_handle_ce, "Kryo L1 Corrected Error" }, + { edac_device_handle_ue, "Kryo L1 Uncorrected Error" }, + { edac_device_handle_ue, "Kryo L1 Deferred Error" }, + { edac_device_handle_ce, "Kryo L2 Corrected Error" }, + { edac_device_handle_ue, "Kryo L2 Uncorrected Error" }, + { edac_device_handle_ue, "Kryo L2 Deferred Error" }, + { edac_device_handle_ce, "L3 Corrected Error" }, + { edac_device_handle_ue, "L3 Uncorrected Error" }, + { edac_device_handle_ue, "L3 Deferred Error" }, +};
A comment is warranted to indicate that err_type is indexed by the enum, as this would be easy to mess up in later changes.
drivers/edac/qcom_kryo_edac.c line 168
Quoted 12 lines
drivers/edac/qcom_kryo_edac.c new file +12
-0+ +static struct edac_device_ctl_info __percpu *edac_dev; +static struct edac_device_ctl_info *drv_edev_ctl; + +static const char *get_error_msg (u64 errxstatus) +{ +const struct error_record *rec; +u32 errxstatus_serr; + + errxstatus_serr =FIELD_GET (KRYO_ERRXSTATUS_SERR , errxstatus); + +for (rec = serror_record; rec->error_code; rec++) {
It looks like you expect the table to be zero terminated, but it's not. Add the missing zero entry.
Quoted 78 lines
drivers/edac/qcom_kryo_edac.c new file +78
-0+if (errxstatus_serr == rec->error_code) +return rec->error_msg; + } + +return NULL ; +} + +static void dump_syndrome_reg (int error_type,int level, +u64 errxstatus,u64 errxmisc, +struct edac_device_ctl_info *edev_ctl) +{ +char msg[KRYO_EDAC_MSG_MAX ]; +const char *error_msg; +int cpu; + + cpu =raw_smp_processor_id (); + + error_msg =get_error_msg (errxstatus); +if (!error_msg) +return ; + +snprintf (msg,KRYO_EDAC_MSG_MAX , +"CPU%d: %s, ERRXSTATUS_EL1:%#llx ERRXMISC0_EL1:%#llx, %s" , + cpu, err_type[error_type].msg, errxstatus, errxmisc, + error_msg); + + err_type[error_type].fn (edev_ctl,0 , level, msg); +} + +static void kryo_check_err_type (u64 errxstatus,u64 errxmisc, +struct edac_device_ctl_info *edev_ctl, +int level) +{ +u32 errxstatus_ue, errxstatus_ce, errxstatus_de; + + errxstatus_ce =FIELD_GET (KRYO_ERRXSTATUS_CE , errxstatus); + errxstatus_ue =FIELD_GET (KRYO_ERRXSTATUS_UE , errxstatus); + errxstatus_de =FIELD_GET (KRYO_ERRXSTATUS_DE , errxstatus); + +switch (level) { +case KRYO_L1 : +if (errxstatus_ce) +dump_syndrome_reg (KRYO_L1_CE , level, errxstatus, + errxmisc, edev_ctl); +else if (errxstatus_ue) +dump_syndrome_reg (KRYO_L1_UE , level, errxstatus, + errxmisc, edev_ctl); +else if (errxstatus_de) +dump_syndrome_reg (KRYO_L1_DE , level, errxstatus, + errxmisc, edev_ctl); +else +edac_printk (KERN_ERR ,DRV_NAME ,"Unknown error\n" ); +break ; +case KRYO_L2 : +if (errxstatus_ce) +dump_syndrome_reg (KRYO_L2_CE , level, errxstatus, + errxmisc, edev_ctl); +else if (errxstatus_ue) +dump_syndrome_reg (KRYO_L2_UE , level, errxstatus, + errxmisc, edev_ctl); +else if (errxstatus_de) +dump_syndrome_reg (KRYO_L2_DE , level, errxstatus, + errxmisc, edev_ctl); +else +edac_printk (KERN_ERR ,DRV_NAME ,"Unknown error\n" ); +break ; +case KRYO_L3 : +if (errxstatus_ce) +dump_syndrome_reg (KRYO_L3_CE , level, errxstatus, + errxmisc, edev_ctl); +else if (errxstatus_ue) +dump_syndrome_reg (KRYO_L3_UE , level, errxstatus, + errxmisc, edev_ctl); +else if (errxstatus_de) +dump_syndrome_reg (KRYO_L3_DE , level, errxstatus, + errxmisc, edev_ctl); +else +edac_printk (KERN_ERR ,DRV_NAME ,"Unknown error\n" );
drivers/edac/qcom_kryo_edac.c line 256
Quoted 10 lines
drivers/edac/qcom_kryo_edac.c new file +10
-0+break ; +default : +edac_printk (KERN_ERR ,DRV_NAME ,"Unknown level\n" ); + } +} + +static inline void kryo_clear_error (u64 errxstatus) +{ +write_sysreg_s (errxstatus,SYS_ERXSTATUS_EL1 ); +isb ();
Is the isb() necessary? If so, why not a dsb as well?
Quoted 65 lines
drivers/edac/qcom_kryo_edac.c new file +65
-0+} + +static void kryo_parse_l1_l2_cache_error (u64 errxstatus,u64 errxmisc, +struct edac_device_ctl_info *edev_ctl, +int cpu) +{ +u32 part_num =read_cpuid_part_number (); + +switch (part_num) { +/* Kryo3XX gold CPU cores do not have a UNIT bitfield */ +case KRYO3XX_GOLD : +case KRYO4XX_SILVER_V1 : +case KRYO4XX_SILVER_V2 : +switch (FIELD_GET (KRYO_ERRXMISC0_LVL , errxmisc)) { +case KRYO_L1 : +kryo_check_err_type (errxstatus, errxmisc, + edev_ctl,KRYO_L1 ); +break ; +case KRYO_L2 : +kryo_check_err_type (errxstatus, errxmisc, + edev_ctl,KRYO_L2 ); +break ; +default : +edac_printk (KERN_ERR ,DRV_NAME , +"silver cpu:%d unknown error: %lu\n" , cpu, +FIELD_GET (KRYO_ERRXMISC0_LVL , errxmisc)); + } +break ; +/* Kryo4XX gold CPU cores have a UNIT bitfield to identify levels */ +case KRYO4XX_GOLD : +switch (FIELD_GET (KRYO_ERRXMISC0_UNIT , errxmisc)) { +case KRYO_L1_UNIT_DC : +case KRYO_L1_UNIT_IC : +kryo_check_err_type (errxstatus, errxmisc, + edev_ctl,KRYO_L1 ); +break ; +case KRYO_L2_UNIT : +case KRYO_L2_UNIT_TLB : +kryo_check_err_type (errxstatus, errxmisc, + edev_ctl,KRYO_L2 ); +break ; +default : +edac_printk (KERN_ERR ,DRV_NAME , +"gold cpu:%d unknown error: %lu\n" , cpu, +FIELD_GET (KRYO_ERRXMISC0_UNIT , errxmisc)); + } +break ; +default : +edac_printk (KERN_ERR ,DRV_NAME , +"Error in matching cpu%d with part num:%u\n" , + cpu, part_num); + } +} + +static inline bool kryo_check_regs_valid (u64 errxstatus) +{ +/* Check if status and misc regs are valid */ +if (!(FIELD_GET (KRYO_ERRXSTATUS_VALID , errxstatus)) || + !(FIELD_GET (KRYO_ERRXSTATUS_MV , errxstatus))) +return false; + +return true; +} + +static void kryo_check_l1_l2_ecc (void *info)
drivers/edac/qcom_kryo_edac.c line 331
Quoted 10 lines
drivers/edac/qcom_kryo_edac.c new file +10
-0+{ +struct edac_device_ctl_info *edev_ctl = info; +u64 errxstatus; +u64 errxmisc; +int cpu; + + cpu =smp_processor_id (); +/* We know record 0 is L1 and L2 */ +write_sysreg_s (0 ,SYS_ERRSELR_EL1 ); +isb ();
Another isb I'm not sure about. Is this meant to provide a barrier between ERRSELR and ERXSTATUS? Wouldn't that be dsb, not isb?
Quoted 15 lines
drivers/edac/qcom_kryo_edac.c new file +15
-0+ + errxstatus =read_sysreg_s (SYS_ERXSTATUS_EL1 ); +if (!kryo_check_regs_valid (errxstatus)) +return ; + + errxmisc =read_sysreg_s (SYS_ERXMISC0_EL1 ); +/* Check if L1/L2 error */ +if (!(FIELD_GET (KRYO_ERRXMISC0_LVL , errxmisc) ==KRYO_L1 ) && + !(FIELD_GET (KRYO_ERRXMISC0_LVL , errxmisc) ==KRYO_L2 )) +return ; + +kryo_parse_l1_l2_cache_error (errxstatus, errxmisc, edev_ctl, cpu); +kryo_clear_error (errxstatus); +} +
Re: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches
Quoting Evan Green (2019-12-11 11:32:37)
Quoted 10 lines
Hi Sai,
Quoted 6 lines · Sai Prakash Ranjan
drivers/edac/qcom_kryo_edac.c new file +0
-0@@ -0,0 +1,679 @@
[...]
Quoted 17 lines
Quoted 17 lines
drivers/edac/qcom_kryo_edac.c new file +17
-0+static const struct error_record serror_record[] = { + {0x1 ,"Errors due to fault injection" }, + {0x2 ,"ECC error from internal data buffer" }, + {0x6 ,"ECC error on cache data RAM" }, + {0x7 ,"ECC error on cache tag or dirty RAM" }, + {0x8 ,"Parity error on TLB data RAM" }, + {0x9 ,"Parity error on TLB tag RAM" }, + {0x12 ,"Error response for a cache copyback" }, + {0x15 ,"Deferred error not supported" }, +}; + +static const struct error_type err_type[] = { + { edac_device_handle_ce,"Kryo L1 Corrected Error" }, + { edac_device_handle_ue,"Kryo L1 Uncorrected Error" }, + { edac_device_handle_ue,"Kryo L1 Deferred Error" }, + { edac_device_handle_ce,"Kryo L2 Corrected Error" }, + { edac_device_handle_ue,"Kryo L2 Uncorrected Error" },
Quoted 8 lines
drivers/edac/qcom_kryo_edac.c
Quoted 5 lines
drivers/edac/qcom_kryo_edac.c new file +5
-0+ { edac_device_handle_ue, "Kryo L2 Deferred Error" }, + { edac_device_handle_ce, "L3 Corrected Error" }, + { edac_device_handle_ue, "L3 Uncorrected Error" }, + { edac_device_handle_ue, "L3 Deferred Error" }, +};A comment is warranted to indicate that
err_typeis indexed by the enum, as this would be easy to mess up in later changes.
Instead of a comment please use array indexing.
[KRYO_L1_CE] = { edac_device_handle_ce, "Kryo L1..." },
..._______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches
Quoted 38 lines · Stephen Boyd
Quoting Evan Green (2019-12-11 11:32:37)
Quoted 10 lines
Hi Sai,
Quoted 6 lines · Sai Prakash Ranjan
drivers/edac/qcom_kryo_edac.c new file +0
-0@@ -0,0 +1,679 @@[...]
Quoted 17 lines
Quoted 17 lines
drivers/edac/qcom_kryo_edac.c new file +17
-0+static const struct error_record serror_record[] = { + {0x1 ,"Errors due to fault injection" }, + {0x2 ,"ECC error from internal data buffer" }, + {0x6 ,"ECC error on cache data RAM" }, + {0x7 ,"ECC error on cache tag or dirty RAM" }, + {0x8 ,"Parity error on TLB data RAM" }, + {0x9 ,"Parity error on TLB tag RAM" }, + {0x12 ,"Error response for a cache copyback" }, + {0x15 ,"Deferred error not supported" }, +}; + +static const struct error_type err_type[] = { + { edac_device_handle_ce,"Kryo L1 Corrected Error" }, + { edac_device_handle_ue,"Kryo L1 Uncorrected Error" }, + { edac_device_handle_ue,"Kryo L1 Deferred Error" }, + { edac_device_handle_ce,"Kryo L2 Corrected Error" }, + { edac_device_handle_ue,"Kryo L2 Uncorrected Error" },Quoted 8 lines
drivers/edac/qcom_kryo_edac.c
Quoted 5 lines
drivers/edac/qcom_kryo_edac.c new file +5
-0+ { edac_device_handle_ue, "Kryo L2 Deferred Error" }, + { edac_device_handle_ce, "L3 Corrected Error" }, + { edac_device_handle_ue, "L3 Uncorrected Error" }, + { edac_device_handle_ue, "L3 Deferred Error" }, +};A comment is warranted to indicate that
err_typeis indexed by the enum, as this would be easy to mess up in later changes.
Quoted 4 lines
Instead of a comment please use array indexing.
[KRYO_L1_CE] = { edac_device_handle_ce, "Kryo L1..." }, ...
Will do this in the next spin.
-Sai
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
A message that is not in the archive (sent off-list or not yet received): 0101016ed57a6559-46c6c649-db28-4945-a11c-7441b8e9ac5b-000000@us-west-2.amazonses.com
Borislav Petkov
maintainerRe: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches
Quoted 3 lines · Sai Prakash Ranjan
Kryo{3,4}XX CPU cores implement RAS extensions to support Error Correcting Code(ECC). Currently all Kryo{3,4}XX CPU cores (gold/silver a.k.a big/LITTLE) support ECC via RAS.
via RAS what? ARM64_RAS_EXTN?
In any case, this needs James to look at and especially if there's some ARM-generic functionality in there which should be shared, of course.
Quoted 1 line
This adds an interrupt based driver for those CPUs and
s/This adds/Add/
Quoted 47 lines
provides an optional polling of error recording system registers.
Signed-off-by Sai Prakash Ranjan <[email protected]>--- MAINTAINERS | 7 + drivers/edac/Kconfig | 20 + drivers/edac/Makefile | 1 + drivers/edac/qcom_kryo_edac.c | 679 ++++++++++++++++++++++++++++++++++ 4 files changed, 707 insertions(+) create mode 100644 drivers/edac/qcom_kryo_edac.cMAINTAINERS +7
-0@@ -6049,6 +6049,13 @@ L: [email protected] S: Maintained F: drivers/edac/qcom_edac.c +EDAC-KRYO-QCOM +M: Sai Prakash Ranjan <[email protected]> +L: [email protected] +L: [email protected] +S: Maintained +F: drivers/edac/qcom_kryo_edac.c + EDIROL UA-101/UA-1000 DRIVER M: Clemens Ladisch <[email protected]> L: [email protected] (moderated for non-subscribers)drivers/edac/Kconfig +9
-0@@ -508,6 +508,26 @@ config EDAC_QCOM For debugging issues having to do with stability and overall system health, you should probably say 'Y' here. +config EDAC_QCOM_KRYO + tristate "QCOM Kryo EDAC for CPU L1/L2/L3-SCU caches" + depends on ARCH_QCOM && ARM64_RAS_EXTN + help + Support for Error detection and correction on Kryo Gold and Silver CPU + cores with RAS extensions. Currently it detects and reports all Single + Bit Errors (SBEs) and Double Bit Errors (DBEs). + + For debugging issues having to do with stability and overall system
drivers/edac/Kconfig config EDAC_QCOM · line 529
Quoted 10 lines
drivers/edac/Kconfig +10
-0+ health, you should probably say 'Y' here. + +config EDAC_QCOM_KRYO_POLL + depends on EDAC_QCOM_KRYO + bool "Poll on Kryo ECC registers" + help + This option chooses whether or not you want to poll on the Kryo ECC + registers. When this is enabled, the polling rate can be set as a + module parameter. By default, it will call the polling function every + second.
Why is this a separate option and why should people use that?
Can the polling/irq be switched automatically?
drivers/edac/Makefile obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o · line 88
Quoted 13 lines
drivers/edac/Kconfig +1
-0+ config EDAC_ASPEED tristate "Aspeed AST 2500 SoC" depends on MACH_ASPEED_G5drivers/edac/Makefile +1
-0@@ -85,5 +85,6 @@ obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o obj-$(CONFIG_EDAC_TI) += ti_edac.o obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o +obj-$(CONFIG_EDAC_QCOM_KRYO) += qcom_kryo_edac.o
What is the difference between this new driver and the qcom_edac one? Can functionality be shared?
Should this new one be called simply kryo_edac instead?
Quoted 18 lines
drivers/edac/Makefile +0
-0obj-$(CONFIG_EDAC_ASPEED) += aspeed_edac.o obj-$(CONFIG_EDAC_BLUEFIELD) += bluefield_edac.odrivers/edac/qcom_kryo_edac.c new file +10
-0@@ -0,0 +1,679 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2019, The Linux Foundation. All rights reserved. +*/ + +#include <linux/bitfield.h> +#include <linux/cpu_pm.h> +#include <linux/of_irq.h> +#include <linux/smp.h> +
drivers/edac/qcom_kryo_edac.c line 20
Quoted 10 lines
drivers/edac/qcom_kryo_edac.c new file +10
-0+#include <asm/cputype.h> +#include <asm/sysreg.h> + +#include "edac_device.h" +#include "edac_mc.h" + +#define DRV_NAME "qcom_kryo_edac" + +/* +* ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.3
Chapter? Where? URL?
Quoted 127 lines
drivers/edac/qcom_kryo_edac.c new file +127
-0+* ARM DSU TRM Chapter B2.3 +* CFI = Corrected Fault Handling interrupt, FI = Fault handling interrupt +* UI = Uncorrected error recovery interrupt, ED = Error Detection +*/ +#define KRYO_ERRXCTLR_ED BIT(0) +#define KRYO_ERRXCTLR_UI BIT(2) +#define KRYO_ERRXCTLR_FI BIT(3) +#define KRYO_ERRXCTLR_CFI BIT(8) +#define KRYO_ERRXCTLR_ENABLE (KRYO_ERRXCTLR_CFI | KRYO_ERRXCTLR_FI | \ + KRYO_ERRXCTLR_UI | KRYO_ERRXCTLR_ED) + +/* +* ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.4 +* ARM DSU TRM Chapter B2.4 +*/ +#define KRYO_ERRXFR_ED GENMASK(1, 0) +#define KRYO_ERRXFR_DE GENMASK(3, 2) +#define KRYO_ERRXFR_UI GENMASK(5, 4) +#define KRYO_ERRXFR_FI GENMASK(7, 6) +#define KRYO_ERRXFR_UE GENMASK(9, 8) +#define KRYO_ERRXFR_CFI GENMASK(11, 10) +#define KRYO_ERRXFR_CEC GENMASK(14, 12) +#define KRYO_ERRXFR_RP BIT(15) +#define KRYO_ERRXFR_SUPPORTED (KRYO_ERRXFR_ED | KRYO_ERRXFR_DE | \ + KRYO_ERRXFR_UI | KRYO_ERRXFR_FI | \ + KRYO_ERRXFR_UE | KRYO_ERRXFR_CFI | \ + KRYO_ERRXFR_CEC | KRYO_ERRXFR_RP) + +/* +* ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.5 +* ARM DSU TRM Chapter B2.5 +*/ +#define KRYO_ERRXMISC0_CECR GENMASK_ULL(38, 32) +#define KRYO_ERRXMISC0_CECO GENMASK_ULL(46, 40) + +/* ARM Cortex-A76 TRM Chapter B3.5 */ +#define KRYO_ERRXMISC0_UNIT GENMASK(3, 0) +#define KRYO_ERRXMISC0_LVL GENMASK(3, 1) + +/* ARM Cortex-A76 TRM Chapter B3.10 has SERR bitfields 4:0 +* but Cortex-A55, Cortex-A75 and DSU TRM has SERR bitfields 7:0. +* Since max error record is 21, we can use bitfields 4:0 for +* Kryo{3,4}XX CPUs. +*/ +#define KRYO_ERRXSTATUS_SERR GENMASK(4, 0) +#define KRYO_ERRXSTATUS_DE BIT(23) +#define KRYO_ERRXSTATUS_CE GENMASK(25, 24) +#define KRYO_ERRXSTATUS_MV BIT(26) +#define KRYO_ERRXSTATUS_UE BIT(29) +#define KRYO_ERRXSTATUS_VALID BIT(30) + +/* ARM Cortex-A76 TRM Chapter B3.5 +* IC = Instruction Cache, DC = Data Cache +*/ +#define KRYO_L1_UNIT_IC 0x1 +#define KRYO_L2_UNIT_TLB 0x2 +#define KRYO_L1_UNIT_DC 0x4 +#define KRYO_L2_UNIT 0x8 + +/* +* ARM Cortex-A55 TRM Chapter B2.36 +* ARM Cortex-A75, Cortex-A76 TRM Chapter B2.37 +*/ +#define KRYO_ERR_RECORD_L1_L2 0x0 +#define KRYO_ERR_RECORD_L3 0x1 + +/* ARM DSU TRM Chapter B2.10 */ +#define BUS_ERROR 0x12 + +/* QCOM Kryo CPU part numbers */ +#define KRYO3XX_GOLD 0x802 +#define KRYO4XX_GOLD 0x804 +#define KRYO4XX_SILVER_V1 0x803 +#define KRYO4XX_SILVER_V2 0x805 + +#define KRYO_EDAC_MSG_MAX 256 + +static int poll_msec =1000 ; +module_param (poll_msec, int,0444 ); + +enum { +KRYO_L1 =0 , +KRYO_L2 , +KRYO_L3 , +}; + +/* CE = Corrected Error, UE = Uncorrected Error, DE = Deferred Error */ +enum { +KRYO_L1_CE =0 , +KRYO_L1_UE , +KRYO_L1_DE , +KRYO_L2_CE , +KRYO_L2_UE , +KRYO_L2_DE , +KRYO_L3_CE , +KRYO_L3_UE , +KRYO_L3_DE , +}; + +struct error_record { +u32 error_code; +const char *error_msg; +}; + +struct error_type { +void (*fn)(struct edac_device_ctl_info *edev_ctl, +int inst_nr,int block_nr,const char *msg); +const char *msg; +}; + +/* +* ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.10 +* ARM DSU TRM Chapter B2.10 +*/ +static const struct error_record serror_record[] = { + {0x1 ,"Errors due to fault injection" }, + {0x2 ,"ECC error from internal data buffer" }, + {0x6 ,"ECC error on cache data RAM" }, + {0x7 ,"ECC error on cache tag or dirty RAM" }, + {0x8 ,"Parity error on TLB data RAM" }, + {0x9 ,"Parity error on TLB tag RAM" }, + {0x12 ,"Error response for a cache copyback" }, + {0x15 ,"Deferred error not supported" }, +}; + +static const struct error_type err_type[] = { + { edac_device_handle_ce,"Kryo L1 Corrected Error" },
drivers/edac/qcom_kryo_edac.c line 157
Quoted 10 lines
drivers/edac/qcom_kryo_edac.c new file +10
-0+ { edac_device_handle_ue, "Kryo L1 Uncorrected Error" }, + { edac_device_handle_ue, "Kryo L1 Deferred Error" }, + { edac_device_handle_ce, "Kryo L2 Corrected Error" }, + { edac_device_handle_ue, "Kryo L2 Uncorrected Error" }, + { edac_device_handle_ue, "Kryo L2 Deferred Error" }, + { edac_device_handle_ce, "L3 Corrected Error" }, + { edac_device_handle_ue, "L3 Uncorrected Error" }, + { edac_device_handle_ue, "L3 Deferred Error" }, +}; +
All that is not really needed - you can put the whole error type detection and dumping in kryo_check_err_type() in nicely readable switch-case statement. No need for the function pointers and special structs.
Quoted 17 lines
drivers/edac/qcom_kryo_edac.c new file +17
-0+static struct edac_device_ctl_info __percpu *edac_dev; +static struct edac_device_ctl_info *drv_edev_ctl; + +static const char *get_error_msg (u64 errxstatus) +{ +const struct error_record *rec; +u32 errxstatus_serr; + + errxstatus_serr =FIELD_GET (KRYO_ERRXSTATUS_SERR , errxstatus); + +for (rec = serror_record; rec->error_code; rec++) { +if (errxstatus_serr == rec->error_code) +return rec->error_msg; + } + +return NULL ; +}
drivers/edac/qcom_kryo_edac.c line 184
Quoted 10 lines
drivers/edac/qcom_kryo_edac.c new file +10
-0+ +static void dump_syndrome_reg (int error_type,int level, +u64 errxstatus,u64 errxmisc, +struct edac_device_ctl_info *edev_ctl) +{ +char msg[KRYO_EDAC_MSG_MAX ]; +const char *error_msg; +int cpu; + + cpu =raw_smp_processor_id ();
Why raw_?
Quoted 12 lines
drivers/edac/qcom_kryo_edac.c new file +12
-0+ + error_msg =get_error_msg (errxstatus); +if (!error_msg) +return ; + +snprintf (msg,KRYO_EDAC_MSG_MAX , +"CPU%d: %s, ERRXSTATUS_EL1:%#llx ERRXMISC0_EL1:%#llx, %s" , + cpu, err_type[error_type].msg, errxstatus, errxmisc, + error_msg); + + err_type[error_type].fn (edev_ctl,0 , level, msg); +}
...
Quoted 4 lines
drivers/edac/qcom_kryo_edac.c new file +4
-0+static int kryo_l1_l2_setup_irq (struct platform_device *pdev, +struct edac_device_ctl_info *edev_ctl) +{ +int cpu, errirq, faultirq, ret;
drivers/edac/qcom_kryo_edac.c
Quoted 10 lines
drivers/edac/qcom_kryo_edac.c new file +10
-0+ + edac_dev =devm_alloc_percpu (&pdev->dev, *edac_dev); +if (!edac_dev) +return -ENOMEM ; + +for_each_possible_cpu (cpu) { +preempt_disable (); +per_cpu (edac_dev, cpu) = edev_ctl; +preempt_enable (); + }
That sillyness doesn't belong here, if at all.
...
drivers/edac/qcom_kryo_edac.c
Quoted 4 lines
drivers/edac/qcom_kryo_edac.c new file +4
-0+static void kryo_poll_cache_error (struct edac_device_ctl_info *edev_ctl) +{ +if (!edev_ctl) + edev_ctl = drv_edev_ctl;
That's silly.
Quoted 4 lines
drivers/edac/qcom_kryo_edac.c new file +4
-0+ +on_each_cpu (kryo_check_l1_l2_ecc, edev_ctl,1 ); +kryo_check_l3_scu_ecc (edev_ctl); +}
...
drivers/edac/qcom_kryo_edac.c
Quoted 7 lines
drivers/edac/qcom_kryo_edac.c new file +7
-0+static int qcom_kryo_edac_probe (struct platform_device *pdev) +{ +struct edac_device_ctl_info *edev_ctl; +struct device *dev = &pdev->dev; +int ret; + +qcom_kryo_edac_setup ();
This function needs to have a return value saying whether it did setup the hw properly or not and the probe function needs to return here if not.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches
Hi Boris,
Thanks for the review comments.
Quoted 10 lines · Borislav Petkov
Quoted 3 lines · Sai Prakash Ranjan
Kryo{3,4}XX CPU cores implement RAS extensions to support Error Correcting Code(ECC). Currently all Kryo{3,4}XX CPU cores (gold/silver a.k.a big/LITTLE) support ECC via RAS.
via RAS what?
ARM64_RAS_EXTN?In any case, this needs James to look at and especially if there's some ARM-generic functionality in there which should be shared, of course.
Yes it is ARM64_RAS_EXTN and I have been hoping if James can provide the feedback, it has been some time now since I posted this out.
Quoted 4 lines
Quoted 1 line
This adds an interrupt based driver for those CPUs and
s/This adds/Add/
Will correct.
Quoted 14 lines
Review
Quoted 11 lines
(quoted diff) +6
-0+ +config EDAC_QCOM_KRYO_POLL + depends on EDAC_QCOM_KRYO + bool "Poll on Kryo ECC registers" + help + This option chooses whether or not you want to poll on the KryoECC
(quoted diff) +2
-0+ registers. When this is enabled, the polling rate can be set as a + module parameter. By default, it will call the polling functionevery
(quoted diff) +1
-0+ second.Why is this a separate option and why should people use that?
Quoted 2 lines
Can the polling/irq be switched automatically?
No it cannot be switched automatically. It is used in case some SoCs do not support an irq based mechanism for EDAC. But I am contradicting myself because I am telling that atleast one interrupt should be specified in bindings, so it is best if I drop this polling option for now.
Quoted 17 lines
drivers/edac/Makefile obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o · line 88
Quoted 13 lines
(quoted diff) +1
-0+ config EDAC_ASPEED tristate "Aspeed AST 2500 SoC" depends on MACH_ASPEED_G5drivers/edac/Makefile +1
-0@@ -85,5 +85,6 @@ obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o obj-$(CONFIG_EDAC_TI) += ti_edac.o obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o +obj-$(CONFIG_EDAC_QCOM_KRYO) += qcom_kryo_edac.oWhat is the difference between this new driver and the
qcom_edacone? Can functionality be shared?
Quoted 2 lines
Should this new one be called simply
kryo_edacinstead?
qcom_edac driver is for QCOM system cache(last level cache), it should be renamed to qcom_llcc_edac.c. This new driver is for QCOM Kryo CPU core caches(L1,L2,L3).
Functionality cannot be shared as these two are different IP blocks and best kept separate.
Quoted 8 lines
drivers/edac/Makefile obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o
Quoted 5 lines
drivers/edac/Makefile +5
-0+ +#define DRV_NAME "qcom_kryo_edac" + +/* + * ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.3Chapter? Where? URL?
I chose this because these TRMs are openly available and if you search for these above terms like "Cortex-A76 TRM Chapter B3.3" in google, then the first search result will be the TRM pdf, otherwise I would have to specify the long URL for the pdf and we do not know how long that URL link will be active.
Quoted 10 lines
Quoted 10 lines
drivers/edac/Makefile +10
-0+ +static const struct error_type err_type[] = { + { edac_device_handle_ce, "Kryo L1 Corrected Error" }, + { edac_device_handle_ue, "Kryo L1 Uncorrected Error" }, + { edac_device_handle_ue, "Kryo L1 Deferred Error" }, + { edac_device_handle_ce, "Kryo L2 Corrected Error" }, + { edac_device_handle_ue, "Kryo L2 Uncorrected Error" }, + { edac_device_handle_ue, "Kryo L2 Deferred Error" }, + { edac_device_handle_ce, "L3 Corrected Error" }, + { edac_device_handle_ue, "L3 Uncorrected Error" },
Quoted 9 lines
drivers/edac/Makefile
Quoted 3 lines
drivers/edac/Makefile +3
-0+ { edac_device_handle_ue, "L3 Deferred Error" }, +}; +All that is not really needed - you can put the whole error type detection and dumping in
kryo_check_err_type()in nicely readable switch-case statement. No need for the function pointers and special structs.
How is this not easily readable? If I put this in kryo_check_err_type, then there will be nested switch which I think is not so great in terms of readability since it will not fit in one screen and is just more lines of code.
Quoted 21 lines
Quoted 21 lines
drivers/edac/Makefile +21
-0+static struct edac_device_ctl_info __percpu *edac_dev; +static struct edac_device_ctl_info *drv_edev_ctl; + +static const char *get_error_msg(u64 errxstatus) +{ + const struct error_record *rec; + u32 errxstatus_serr; + + errxstatus_serr = FIELD_GET(KRYO_ERRXSTATUS_SERR, errxstatus); + + for (rec = serror_record; rec->error_code; rec++) { + if (errxstatus_serr == rec->error_code) + return rec->error_msg; + } + + return NULL; +} + +static void dump_syndrome_reg(int error_type, int level, + u64 errxstatus, u64 errxmisc, + struct edac_device_ctl_info *edev_ctl)
Quoted 9 lines
drivers/edac/Makefile
Quoted 6 lines
drivers/edac/Makefile +6
-0+{ + char msg[KRYO_EDAC_MSG_MAX]; + const char *error_msg; + int cpu; + + cpu = raw_smp_processor_id();Why
raw_?
Because we will be calling smp_processor_id in preemptible context and if we enable CONFIG_DEBUG_PREEMPT, we would get a nice backtrace.
[ 3.747468] BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1 [ 3.755527] caller is qcom_kryo_edac_probe+0x138/0x2b8 [ 3.760819] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S 5.4.0-rc7-next-20191113-00009-g8666855d6a5b-dirty #107 [ 3.772323] Hardware name: Qualcomm Technologies, Inc. SM8150 MTP (DT) [ 3.779030] Call trace: [ 3.781556] dump_backtrace+0x0/0x158 [ 3.785331] show_stack+0x14/0x20 [ 3.788741] dump_stack+0xb0/0xf4 [ 3.792164] debug_smp_processor_id+0xd8/0xe0 [ 3.796639] qcom_kryo_edac_probe+0x138/0x2b8 [ 3.801116] platform_drv_probe+0x50/0xa8 [ 3.805236] really_probe+0x108/0x360 [ 3.808999] driver_probe_device+0x58/0x100 [ 3.813304] device_driver_attach+0x6c/0x78 [ 3.817606] __driver_attach+0xb0/0xf0 [ 3.821459] bus_for_each_dev+0x68/0xc8 [ 3.825407] driver_attach+0x20/0x28 [ 3.829083] bus_add_driver+0x160/0x1f0 [ 3.833030] driver_register+0x60/0x110 [ 3.836976] __platform_driver_register+0x40/0x48 [ 3.841813] qcom_kryo_edac_driver_init+0x18/0x20 [ 3.846645] do_one_initcall+0x58/0x1a0 [ 3.850596] kernel_init_freeable+0x19c/0x240 [ 3.855075] kernel_init+0x10/0x108 [ 3.858665] ret_from_fork+0x10/0x1c
Quoted 8 lines
Quoted 8 lines
drivers/edac/Makefile +8
-0+static int kryo_l1_l2_setup_irq(struct platform_device *pdev, + struct edac_device_ctl_info *edev_ctl) +{ + int cpu, errirq, faultirq, ret; + + edac_dev = devm_alloc_percpu(&pdev->dev, *edac_dev); + if (!edac_dev) + return -ENOMEM;
Quoted 9 lines
drivers/edac/Makefile
Quoted 6 lines
drivers/edac/Makefile +6
-0+ + for_each_possible_cpu(cpu) { + preempt_disable(); + per_cpu(edac_dev, cpu) = edev_ctl; + preempt_enable(); + }That sillyness doesn't belong here, if at all.
Sorry but I do not understand the sillyness here. Could you please explain?
Quoted 10 lines
...
drivers/edac/Makefile
Quoted 5 lines
+static void
kryo_poll_cache_error(struct edac_device_ctl_info *edev_ctl)drivers/edac/Makefile +3
-0+{ + if (!edev_ctl) + edev_ctl = drv_edev_ctl;That's silly.
Actually its not silly. In case, polling is enabled and on PM exit edev_ctl could be NULL.
Quoted 7 lines
Quoted 4 lines
drivers/edac/Makefile +4
-0+ + on_each_cpu(kryo_check_l1_l2_ecc, edev_ctl, 1); + kryo_check_l3_scu_ecc(edev_ctl); +}...
Quoted 11 lines
drivers/edac/Makefile
Quoted 7 lines
drivers/edac/Makefile +7
-0+static int qcom_kryo_edac_probe(struct platform_device *pdev) +{ + struct edac_device_ctl_info *edev_ctl; + struct device *dev = &pdev->dev; + int ret; + + qcom_kryo_edac_setup();This function needs to have a return value saying whether it did setup the hw properly or not and the probe function needs to return here if not.
Ok will add a return check.
Thanks,
Sai
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches
Hi guys,
(CC: +Tyler)
Quoted 6 lines · Sai Prakash Ranjan
Quoted 5 lines · Borislav Petkov
Quoted 3 lines · Sai Prakash Ranjan
Kryo{3,4}XX CPU cores implement RAS extensions to support Error Correcting Code(ECC). Currently all Kryo{3,4}XX CPU cores (gold/silver a.k.a big/LITTLE) support ECC via RAS.
Quoted 7 lines
Quoted 4 lines
via RAS what?
ARM64_RAS_EXTN?In any case, this needs James to look at and especially if there's some ARM-generic functionality in there which should be shared, of course.
Yes it is
ARM64_RAS_EXTNand I have been hoping if James can provide the feedback, it has been some time now since I posted this out.
Sorry, I was out of the office for most of November/December, and I'm slowly catching up...
Quoted 12 lines
Quoted 12 lines
Review
Quoted 9 lines
(quoted diff) +9
-0+ +config EDAC_QCOM_KRYO_POLL + depends on EDAC_QCOM_KRYO + bool "Poll on Kryo ECC registers" + help + This option chooses whether or not you want to poll on the Kryo ECC + registers. When this is enabled, the polling rate can be set as a + module parameter. By default, it will call the polling function every + second.Why is this a separate option and why should people use that?
Quoted 7 lines
Quoted 1 line
Can the polling/irq be switched automatically?
No it cannot be switched automatically. It is used in case some SoCs do not support an irq based mechanism for EDAC.
But I am contradicting myself because I am telling that atleast one interrupt should be specified in bindings,
so it is best if I drop this polling option for now.
For now, sure. But I think this will come back for systems with embarrassing amounts of RAM that would rather scrub the errors than take a flood of IRQs. I'd like this to be controllable from user-space.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/edac/Makefile +0
-0
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
drivers/edac/Makefile +0
-0@@ -85,5 +85,6 @@ obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.oobj-$(CONFIG_EDAC_XGENE) += xgene_edac.o obj-$(CONFIG_EDAC_TI) += ti_edac.o obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o +obj-$(CONFIG_EDAC_QCOM_KRYO) += qcom_kryo_edac.o
What is the difference between this new driver and the
qcom_edacone? Can functionality be shared?
High-level story time:
Until the 'v8.2' revision of the 'v8' Arm-architecture (the 64bit one), arm didn't describe how RAS should work. Partners implemented what they needed, and we ended up with this collection of drivers because they were all different.
v8.2 fixed all this, the good news is once its done, we should never need another edac driver. (at least, not for SoCs built for v8.2). The downside is there is quite a lot in there, and we need to cover ACPI machines as well as DT.
Quoted 5 lines
qcom_edacdriver is for QCOM system cache(last level cache), it should be renamed toqcom_llcc_edac.c.
This new driver is for QCOM Kryo CPU corecaches(L1,L2,L3).Functionality cannot be shared as these two are different IP blocks and best kept separate.
The qcom_edac will be Qualcomm's pre-v8.2 support.
This series is about the v8.2 support which all looks totally different to Linux.
Quoted 11 lines
Quoted 4 lines
Quoted 1 line
+ * ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.3
Chapter? Where? URL?
I chose this because these TRMs are openly available and if you search for these above terms like
"Cortex-A76 TRM Chapter B3.3" in google, then the first search result will be the TRM pdf, otherwise
I would have to specify the long URL for the pdf and we do not know how long that URL link will be active.
These are SoC/CPU specific. Using these we can't solve the whole problem.
The architecture all those should fit into is here:
https://static.docs.arm.com/ddi0587/cb/2019_07_05_DD_0587_C_b.pdf
(or https://developer.arm.com/docs/ and look for 'RAS')
... and the arm-arm.
Quoted 35 lines
Quoted 12 lines
drivers/edac/Makefile
Quoted 9 lines
drivers/edac/Makefile +9
-0+static void dump_syndrome_reg(int error_type, int level, + u64 errxstatus, u64 errxmisc, + struct edac_device_ctl_info *edev_ctl) +{ + char msg[KRYO_EDAC_MSG_MAX]; + const char *error_msg; + int cpu; + + cpu = raw_smp_processor_id();Why
raw_?Because we will be calling
smp_processor_idin preemptible context and if we enableCONFIG_DEBUG_PREEMPT,
we would get a nice backtrace.[ 3.747468] BUG: using
smp_processor_id()in preemptible [00000000] code: swapper/0/1 [ 3.755527] caller isqcom_kryo_edac_probe+0x138/0x2b8
[ 3.760819] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S 5.4.0-rc7-next-20191113-00009-g8666855d6a5b-dirty #107
[ 3.772323] Hardware name: Qualcomm Technologies, Inc. SM8150 MTP (DT) [ 3.779030] Call trace:
[ 3.781556]dump_backtrace+0x0/0x158
[ 3.785331]show_stack+0x14/0x20
[ 3.788741]dump_stack+0xb0/0xf4
[ 3.792164]debug_smp_processor_id+0xd8/0xe0
[ 3.796639]qcom_kryo_edac_probe+0x138/0x2b8
[ 3.801116]platform_drv_probe+0x50/0xa8
[ 3.805236]really_probe+0x108/0x360
[ 3.808999]driver_probe_device+0x58/0x100
[ 3.813304]device_driver_attach+0x6c/0x78
[ 3.817606]__driver_attach+0xb0/0xf0
[ 3.821459]bus_for_each_dev+0x68/0xc8
[ 3.825407]driver_attach+0x20/0x28
Quoted 8 lines
[ 3.829083]
bus_add_driver+0x160/0x1f0
[ 3.833030]driver_register+0x60/0x110
[ 3.836976]__platform_driver_register+0x40/0x48[ 3.841813]qcom_kryo_edac_driver_init+0x18/0x20[ 3.846645]do_one_initcall+0x58/0x1a0
[ 3.850596]kernel_init_freeable+0x19c/0x240[ 3.855075]kernel_init+0x10/0x108
[ 3.858665]ret_from_fork+0x10/0x1c
and raw_ stops the backtrace? You are still preemptible. The problem still exists, you've just suppressed the warning.
At any time in dump_syndrome_reg(), you could get an interrupt and another task gets scheduled. Later your thread is started on another cpu... but not the one whose cpu number you read from smp_processor_id(). Whatever you needed it for, might have the wrong value.
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/edac/Makefile +8
-0+static int kryo_l1_l2_setup_irq(struct platform_device *pdev, + struct edac_device_ctl_info *edev_ctl) +{ + int cpu, errirq, faultirq, ret; + + edac_dev = devm_alloc_percpu(&pdev->dev, *edac_dev); + if (!edac_dev) + return -ENOMEM;Quoted 8 lines
Quoted 6 lines
+
+for_each_possible_cpu(cpu){
+preempt_disable();
+per_cpu(edac_dev, cpu)=edev_ctl;
+preempt_enable();
+ }That sillyness doesn't belong here, if at all.
Quoted 1 line
Sorry but I do not understand the sillyness here. Could you please explain?
preempt_disable() prevents another task being scheduled instead of you, avoiding the risk that you get scheduled on another cpu. In this case it doesn't matter which cpu you are running on as you aren't accessing _this_ cpu's edac_dev, you are accessing each one in a loop.
Thanks,
James
Re: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches
Hi James,
Quoted 46 lines · James Morse
Hi guys,
(CC: +Tyler)
Quoted 6 lines · Sai Prakash Ranjan
Quoted 5 lines · Borislav Petkov
Quoted 3 lines · Sai Prakash Ranjan
Kryo{3,4}XX CPU cores implement RAS extensions to support Error Correcting Code(ECC). Currently all Kryo{3,4}XX CPU cores (gold/silver a.k.a big/LITTLE) support ECC via RAS.
Quoted 7 lines
Quoted 4 lines
via RAS what?
ARM64_RAS_EXTN?In any case, this needs James to look at and especially if there's some ARM-generic functionality in there which should be shared, of course.
Yes it is
ARM64_RAS_EXTNand I have been hoping if James can provide the feedback, it has been some time now since I posted this out.Sorry, I was out of the office for most of November/December, and I'm slowly catching up...
Quoted 15 lines
Quoted 15 lines
Quoted 6 lines
(quoted diff) +6
-0+ +config EDAC_QCOM_KRYO_POLL + depends on EDAC_QCOM_KRYO + bool "Poll on Kryo ECC registers" + help + This option chooses whether or not you want to poll on theQuoted 6 lines
Kryo ECC
+ registers. When this is enabled, the polling rate can be set as a
+ module parameter. By default, it will call the polling function every
+ second.Why is this a separate option and why should people use that?
Quoted 7 lines
Quoted 1 line
Can the polling/irq be switched automatically?
No it cannot be switched automatically. It is used in case some SoCs do not support an irq based mechanism for EDAC.
But I am contradicting myself because I am telling that atleast one interrupt should be specified in bindings,
so it is best if I drop this polling option for now.
Quoted 6 lines
For now, sure. But I think this will come back for systems with embarrassing amounts of
RAM that would rather scrub the errors than take a flood of IRQs. I'd like this to be
controllable from user-space.
Ok so we should have an option to switch between polling and irq.
Quoted 22 lines
Quoted 14 lines
Quoted 14 lines
Quoted 11 lines
drivers/edac/Makefile +0
-0@@ -85,5 +85,6 @@ obj-$(CONFIG_EDAC_SYNOPSYS) +=synopsys_edac.o
obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o obj-$(CONFIG_EDAC_TI) += ti_edac.o obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o +obj-$(CONFIG_EDAC_QCOM_KRYO) += qcom_kryo_edac.oWhat is the difference between this new driver and the
qcom_edacone? Can functionality be shared?High-level story time:
Until the 'v8.2' revision of the 'v8' Arm-architecture (the 64bit one), arm didn't
describe how RAS should work. Partners implemented what they needed, and we ended up with
this collection of drivers because they were all different.
Quoted 6 lines
v8.2 fixed all this, the good news is once its done, we should never need another edac
driver. (at least, not for SoCs built for v8.2). The downside is there is quite a lot in
there, and we need to cover ACPI machines as well as DT.
That is true but the qcom_edac one which is merged is for LLC(system cache) which is a QCOM IP.
Quoted 10 lines
Quoted 5 lines
qcom_edacdriver is for QCOM system cache(last level cache), it should be renamed toqcom_llcc_edac.c.
This new driver is for QCOM Kryo CPU corecaches(L1,L2,L3).Functionality cannot be shared as these two are different IP blocks and best kept separate.
The
qcom_edacwill be Qualcomm's pre-v8.2 support. This series is about the v8.2 support which all looks totally different to Linux.
As said before qcom_edac is for LLC which is not available on all SoCs. QCOM's pre v8.2 support is not upstreamed.
Quoted 13 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
+ * ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.3
Chapter? Where? URL?
Quoted 6 lines
I chose this because these TRMs are openly available and if you search for these above terms like
"Cortex-A76 TRM Chapter B3.3" in google, then the first search result will be the TRM pdf, otherwise
I would have to specify the long URL for the pdf and we do not know how long that URL link will be active.
Quoted 8 lines
These are SoC/CPU specific. Using these we can't solve the whole problem.
The architecture all those should fit into is here:
https://static.docs.arm.com/ddi0587/cb/2019_07_05_DD_0587_C_b.pdf
(or https://developer.arm.com/docs/ and look for 'RAS')... and the arm-arm.
Thanks for the link.
Quoted 49 lines
Quoted 36 lines
Quoted 13 lines
drivers/edac/Makefile
Quoted 10 lines
drivers/edac/Makefile +9
-0+static void dump_syndrome_reg(int error_type, int level, + u64 errxstatus, u64 errxmisc, + struct edac_device_ctl_info *edev_ctl) +{ + char msg[KRYO_EDAC_MSG_MAX]; + const char *error_msg; + int cpu; + + cpu = raw_smp_processor_id();Why
raw_?Because we will be calling
smp_processor_idin preemptible context and if we enableCONFIG_DEBUG_PREEMPT,
we would get a nice backtrace.[ 3.747468] BUG: using
smp_processor_id()in preemptible [00000000] code: swapper/0/1 [ 3.755527] caller isqcom_kryo_edac_probe+0x138/0x2b8
[ 3.760819] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S 5.4.0-rc7-next-20191113-00009-g8666855d6a5b-dirty #107
[ 3.772323] Hardware name: Qualcomm Technologies, Inc. SM8150 MTP (DT) [ 3.779030] Call trace:
[ 3.781556]dump_backtrace+0x0/0x158
[ 3.785331]show_stack+0x14/0x20
[ 3.788741]dump_stack+0xb0/0xf4
[ 3.792164]debug_smp_processor_id+0xd8/0xe0
[ 3.796639]qcom_kryo_edac_probe+0x138/0x2b8
[ 3.801116]platform_drv_probe+0x50/0xa8
[ 3.805236]really_probe+0x108/0x360
[ 3.808999]driver_probe_device+0x58/0x100
[ 3.813304]device_driver_attach+0x6c/0x78
[ 3.817606]__driver_attach+0xb0/0xf0
[ 3.821459]bus_for_each_dev+0x68/0xc8
[ 3.825407]driver_attach+0x20/0x28Quoted 8 lines
[ 3.829083]
bus_add_driver+0x160/0x1f0
[ 3.833030]driver_register+0x60/0x110
[ 3.836976]__platform_driver_register+0x40/0x48[ 3.841813]qcom_kryo_edac_driver_init+0x18/0x20[ 3.846645]do_one_initcall+0x58/0x1a0
[ 3.850596]kernel_init_freeable+0x19c/0x240[ 3.855075]kernel_init+0x10/0x108
[ 3.858665]ret_from_fork+0x10/0x1cand
raw_stops the backtrace? You are still preemptible. The problem still exists, you've
just suppressed the warning.
Quoted 7 lines
At any time in
dump_syndrome_reg(), you could get an interrupt and another task gets
scheduled. Later your thread is started on another cpu... but not the one whose cpu number
you read fromsmp_processor_id(). Whatever you needed it for, might have the wrong value.
Ok will correct this.
Quoted 20 lines
Quoted 18 lines
Quoted 17 lines
Quoted 5 lines
drivers/edac/Makefile +4
-0+static int kryo_l1_l2_setup_irq(struct platform_device *pdev, + struct edac_device_ctl_info *edev_ctl) +{ + int cpu, errirq, faultirq, ret;drivers/edac/Makefile
Quoted 10 lines
drivers/edac/Makefile +10
-0+ + edac_dev = devm_alloc_percpu(&pdev->dev, *edac_dev); + if (!edac_dev) + return -ENOMEM; + + for_each_possible_cpu(cpu) { + preempt_disable(); + per_cpu(edac_dev, cpu) = edev_ctl; + preempt_enable(); + }That sillyness doesn't belong here, if at all.
Quoted 1 line
Sorry but I do not understand the sillyness here. Could you please explain?
Quoted 8 lines
preempt_disable()prevents another task being scheduled instead of you, avoiding the risk
that you get scheduled on another cpu. In this case it doesn't matter which cpu you are
running on as you aren't accessing _this_ cpu'sedac_dev, you are accessing each one in a
loop.
Thanks for the explanation James, now I get the sillyness.
Thanks,
Sai
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation