[PATCH 0/2] edac: Add support for Xilinx ZynqMP OCM EDAC

[PATCH 0/2] edac: Add support for Xilinx ZynqMP OCM EDAC

[Sai Krishna Potthuri]

This patch series does following
- Add bindings for Xilinx ZynqMP OCM EDAC.
- Add EDAC driver support for ZynqMP OCM controller.

Sai Krishna Potthuri (1):
  edac: zynqmp_ocm: Add EDAC support for ZynqMP OCM

Shubhrajyoti Datta (1):
  dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

 .../bindings/edac/xlnx,zynqmp-ocmc.yaml       |  41 ++
 MAINTAINERS                                   |   7 +
 drivers/edac/Kconfig                          |   9 +
 drivers/edac/Makefile                         |   1 +
 drivers/edac/zynqmp_ocm_edac.c                | 643 ++++++++++++++++++
 5 files changed, 701 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
 create mode 100644 drivers/edac/zynqmp_ocm_edac.c

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/2] edac: Add support for Xilinx ZynqMP OCM EDAC

[Sai Krishna Potthuri]

This patch series does following
- Add bindings for Xilinx ZynqMP OCM EDAC.
- Add EDAC driver support for ZynqMP OCM controller.

Sai Krishna Potthuri (1):
  edac: zynqmp_ocm: Add EDAC support for ZynqMP OCM

Shubhrajyoti Datta (1):
  dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

 .../bindings/edac/xlnx,zynqmp-ocmc.yaml       |  41 ++
 MAINTAINERS                                   |   7 +
 drivers/edac/Kconfig                          |   9 +
 drivers/edac/Makefile                         |   1 +
 drivers/edac/zynqmp_ocm_edac.c                | 643 ++++++++++++++++++
 5 files changed, 701 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
 create mode 100644 drivers/edac/zynqmp_ocm_edac.c

-- 
2.17.1

[PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

[Sai Krishna Potthuri]

From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>

Add bindings for Xilinx ZynqMP OCM controller.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
---
 .../bindings/edac/xlnx,zynqmp-ocmc.yaml       | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml

diff --git a/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
new file mode 100644
index 000000000000..9bcecaccade2
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/edac/xlnx,zynqmp-ocmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx Zynqmp OCM EDAC driver
+
+maintainers:
+  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
+  - Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
+
+description: |
+  Xilinx ZynqMP OCM EDAC driver, it does reports the OCM ECC single bit errors
+  that are corrected and double bit ecc errors that are detected by the OCM
+  ECC controller.
+
+properties:
+  compatible:
+    const: xlnx,zynqmp-ocmc-1.0
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    memory-controller@ff960000 {
+      compatible = "xlnx,zynqmp-ocmc-1.0";
+      reg = <0xff960000 0x1000>;
+      interrupts = <0 10 4>;
+    };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

[Sai Krishna Potthuri]

From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>

Add bindings for Xilinx ZynqMP OCM controller.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
---
 .../bindings/edac/xlnx,zynqmp-ocmc.yaml       | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml

diff --git a/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
new file mode 100644
index 000000000000..9bcecaccade2
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/edac/xlnx,zynqmp-ocmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx Zynqmp OCM EDAC driver
+
+maintainers:
+  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
+  - Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
+
+description: |
+  Xilinx ZynqMP OCM EDAC driver, it does reports the OCM ECC single bit errors
+  that are corrected and double bit ecc errors that are detected by the OCM
+  ECC controller.
+
+properties:
+  compatible:
+    const: xlnx,zynqmp-ocmc-1.0
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    memory-controller@ff960000 {
+      compatible = "xlnx,zynqmp-ocmc-1.0";
+      reg = <0xff960000 0x1000>;
+      interrupts = <0 10 4>;
+    };
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

[Krzysztof Kozlowski]

On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> 
> Add bindings for Xilinx ZynqMP OCM controller.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> ---
>  .../bindings/edac/xlnx,zynqmp-ocmc.yaml       | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> new file mode 100644
> index 000000000000..9bcecaccade2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/edac/xlnx,zynqmp-ocmc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Zynqmp OCM EDAC driver

s/EDAC driver//
Is it a memory controller?

> +
> +maintainers:
> +  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> +  - Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> +
> +description: |
> +  Xilinx ZynqMP OCM EDAC driver, it does reports the OCM ECC single bit errors

The same. Describe the hardware, not the Linux driver or its subsystem.

> +  that are corrected and double bit ecc errors that are detected by the OCM

s/ecc/ECC/

> +  ECC controller.
> +
> +properties:
> +  compatible:
> +    const: xlnx,zynqmp-ocmc-1.0
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    memory-controller@ff960000 {
> +      compatible = "xlnx,zynqmp-ocmc-1.0";
> +      reg = <0xff960000 0x1000>;
> +      interrupts = <0 10 4>;

Isn't the interrupt using common flags? If so, use proper defines.

> +    };


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

[Krzysztof Kozlowski]

On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> 
> Add bindings for Xilinx ZynqMP OCM controller.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> ---
>  .../bindings/edac/xlnx,zynqmp-ocmc.yaml       | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> new file mode 100644
> index 000000000000..9bcecaccade2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/edac/xlnx,zynqmp-ocmc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Zynqmp OCM EDAC driver

s/EDAC driver//
Is it a memory controller?

> +
> +maintainers:
> +  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> +  - Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> +
> +description: |
> +  Xilinx ZynqMP OCM EDAC driver, it does reports the OCM ECC single bit errors

The same. Describe the hardware, not the Linux driver or its subsystem.

> +  that are corrected and double bit ecc errors that are detected by the OCM

s/ecc/ECC/

> +  ECC controller.
> +
> +properties:
> +  compatible:
> +    const: xlnx,zynqmp-ocmc-1.0
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    memory-controller@ff960000 {
> +      compatible = "xlnx,zynqmp-ocmc-1.0";
> +      reg = <0xff960000 0x1000>;
> +      interrupts = <0 10 4>;

Isn't the interrupt using common flags? If so, use proper defines.

> +    };


Best regards,
Krzysztof

RE: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

[Potthuri, Sai Krishna]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, August 16, 2022 1:29 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Michal Simek
> <michal.simek@xilinx.com>; Borislav Petkov <bp@alien8.de>; Mauro
> Carvalho Chehab <mchehab@kernel.org>; Tony Luck <tony.luck@intel.com>;
> James Morse <james.morse@arm.com>; Robert Richter <rric@kernel.org>
> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> saikrishna12468@gmail.com; git (AMD-Xilinx) <git@amd.com>; Shubhrajyoti
> Datta <shubhrajyoti.datta@xilinx.com>
> Subject: Re: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP
> OCM
> 
> On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
> > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> >
> > Add bindings for Xilinx ZynqMP OCM controller.
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> > Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > ---
> >  .../bindings/edac/xlnx,zynqmp-ocmc.yaml       | 41 +++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> > b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> > new file mode 100644
> > index 000000000000..9bcecaccade2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> > @@ -0,0 +1,41 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/edac/xlnx,zynqmp-ocmc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Xilinx Zynqmp OCM EDAC driver
> 
> s/EDAC driver//
> Is it a memory controller?
This driver is about Error Detection and Correction feature for OCM (On Chip
Memory) controller which supports ECC functionality.
> 
> > +
> > +maintainers:
> > +  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> > +  - Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > +
> > +description: |
> > +  Xilinx ZynqMP OCM EDAC driver, it does reports the OCM ECC single
> > +bit errors
> 
> The same. Describe the hardware, not the Linux driver or its subsystem.
I will fix in v2.
> 
> > +  that are corrected and double bit ecc errors that are detected by
> > + the OCM
> 
> s/ecc/ECC/
I will fix in v2.
> 
> > +  ECC controller.
> > +
> > +properties:
> > +  compatible:
> > +    const: xlnx,zynqmp-ocmc-1.0
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    memory-controller@ff960000 {
> > +      compatible = "xlnx,zynqmp-ocmc-1.0";
> > +      reg = <0xff960000 0x1000>;
> > +      interrupts = <0 10 4>;
> 
> Isn't the interrupt using common flags? If so, use proper defines.
I will fix in v2.

Regards
Sai Krishna
> 
> > +    };
> 
> 
> Best regards,
> Krzysztof

RE: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

[Potthuri, Sai Krishna]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, August 16, 2022 1:29 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Michal Simek
> <michal.simek@xilinx.com>; Borislav Petkov <bp@alien8.de>; Mauro
> Carvalho Chehab <mchehab@kernel.org>; Tony Luck <tony.luck@intel.com>;
> James Morse <james.morse@arm.com>; Robert Richter <rric@kernel.org>
> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> saikrishna12468@gmail.com; git (AMD-Xilinx) <git@amd.com>; Shubhrajyoti
> Datta <shubhrajyoti.datta@xilinx.com>
> Subject: Re: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP
> OCM
> 
> On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
> > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> >
> > Add bindings for Xilinx ZynqMP OCM controller.
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> > Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > ---
> >  .../bindings/edac/xlnx,zynqmp-ocmc.yaml       | 41 +++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> > b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> > new file mode 100644
> > index 000000000000..9bcecaccade2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> > @@ -0,0 +1,41 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/edac/xlnx,zynqmp-ocmc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Xilinx Zynqmp OCM EDAC driver
> 
> s/EDAC driver//
> Is it a memory controller?
This driver is about Error Detection and Correction feature for OCM (On Chip
Memory) controller which supports ECC functionality.
> 
> > +
> > +maintainers:
> > +  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> > +  - Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > +
> > +description: |
> > +  Xilinx ZynqMP OCM EDAC driver, it does reports the OCM ECC single
> > +bit errors
> 
> The same. Describe the hardware, not the Linux driver or its subsystem.
I will fix in v2.
> 
> > +  that are corrected and double bit ecc errors that are detected by
> > + the OCM
> 
> s/ecc/ECC/
I will fix in v2.
> 
> > +  ECC controller.
> > +
> > +properties:
> > +  compatible:
> > +    const: xlnx,zynqmp-ocmc-1.0
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    memory-controller@ff960000 {
> > +      compatible = "xlnx,zynqmp-ocmc-1.0";
> > +      reg = <0xff960000 0x1000>;
> > +      interrupts = <0 10 4>;
> 
> Isn't the interrupt using common flags? If so, use proper defines.
I will fix in v2.

Regards
Sai Krishna
> 
> > +    };
> 
> 
> Best regards,
> Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

[Krzysztof Kozlowski]

On 16/08/2022 15:43, Potthuri, Sai Krishna wrote:
> Hi Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Tuesday, August 16, 2022 1:29 PM
>> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Rob Herring
>> <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Michal Simek
>> <michal.simek@xilinx.com>; Borislav Petkov <bp@alien8.de>; Mauro
>> Carvalho Chehab <mchehab@kernel.org>; Tony Luck <tony.luck@intel.com>;
>> James Morse <james.morse@arm.com>; Robert Richter <rric@kernel.org>
>> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; linux-edac@vger.kernel.org;
>> saikrishna12468@gmail.com; git (AMD-Xilinx) <git@amd.com>; Shubhrajyoti
>> Datta <shubhrajyoti.datta@xilinx.com>
>> Subject: Re: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP
>> OCM
>>
>> On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
>>> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
>>>
>>> Add bindings for Xilinx ZynqMP OCM controller.
>>>
>>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
>>> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
>>> ---
>>>  .../bindings/edac/xlnx,zynqmp-ocmc.yaml       | 41 +++++++++++++++++++
>>>  1 file changed, 41 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
>>> b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
>>> new file mode 100644
>>> index 000000000000..9bcecaccade2
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
>>> @@ -0,0 +1,41 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/edac/xlnx,zynqmp-ocmc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Xilinx Zynqmp OCM EDAC driver
>>
>> s/EDAC driver//
>> Is it a memory controller?
> This driver is about Error Detection and Correction feature for OCM (On Chip
> Memory) controller which supports ECC functionality.

I am not talking about driver. What is the hardware exactly? On Chip
Memory Controller sounds like Memory Controller, so use this instead of
EDAC.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP OCM

[Krzysztof Kozlowski]

On 16/08/2022 15:43, Potthuri, Sai Krishna wrote:
> Hi Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Tuesday, August 16, 2022 1:29 PM
>> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Rob Herring
>> <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Michal Simek
>> <michal.simek@xilinx.com>; Borislav Petkov <bp@alien8.de>; Mauro
>> Carvalho Chehab <mchehab@kernel.org>; Tony Luck <tony.luck@intel.com>;
>> James Morse <james.morse@arm.com>; Robert Richter <rric@kernel.org>
>> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; linux-edac@vger.kernel.org;
>> saikrishna12468@gmail.com; git (AMD-Xilinx) <git@amd.com>; Shubhrajyoti
>> Datta <shubhrajyoti.datta@xilinx.com>
>> Subject: Re: [PATCH 1/2] dt-bindings: edac: Add bindings for Xilinx ZynqMP
>> OCM
>>
>> On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
>>> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
>>>
>>> Add bindings for Xilinx ZynqMP OCM controller.
>>>
>>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
>>> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
>>> ---
>>>  .../bindings/edac/xlnx,zynqmp-ocmc.yaml       | 41 +++++++++++++++++++
>>>  1 file changed, 41 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
>>> b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
>>> new file mode 100644
>>> index 000000000000..9bcecaccade2
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
>>> @@ -0,0 +1,41 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/edac/xlnx,zynqmp-ocmc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Xilinx Zynqmp OCM EDAC driver
>>
>> s/EDAC driver//
>> Is it a memory controller?
> This driver is about Error Detection and Correction feature for OCM (On Chip
> Memory) controller which supports ECC functionality.

I am not talking about driver. What is the hardware exactly? On Chip
Memory Controller sounds like Memory Controller, so use this instead of
EDAC.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP OCM

[Sai Krishna Potthuri]

Add EDAC support for Xilinx ZynqMP OCM Controller, this driver
reports CE and UE errors based on the interrupts, and also creates ue/ce
sysfs entries for error injection.

Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---
 MAINTAINERS                    |   7 +
 drivers/edac/Kconfig           |   9 +
 drivers/edac/Makefile          |   1 +
 drivers/edac/zynqmp_ocm_edac.c | 643 +++++++++++++++++++++++++++++++++
 4 files changed, 660 insertions(+)
 create mode 100644 drivers/edac/zynqmp_ocm_edac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index edc96cdb85e8..cd4c6c90bca3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21692,6 +21692,13 @@ F:	Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dpdma.yaml
 F:	drivers/dma/xilinx/xilinx_dpdma.c
 F:	include/dt-bindings/dma/xlnx-zynqmp-dpdma.h
 
+XILINX ZYNQMP OCM EDAC DRIVER
+M:	Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
+M:	Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
+F:	drivers/edac/zynqmp_ocm_edac.c
+
 XILINX ZYNQMP PSGTR PHY DRIVER
 M:	Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
 M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 58ab63642e72..fece60f586af 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -539,4 +539,13 @@ config EDAC_DMC520
 	  Support for error detection and correction on the
 	  SoCs with ARM DMC-520 DRAM controller.
 
+config EDAC_ZYNQMP_OCM
+	tristate "Xilinx ZynqMP OCM Controller"
+	depends on ARCH_ZYNQMP
+	help
+	  Support for error detection and correction on the xilinx ZynqMP OCM
+	  controller.
+	  This driver can also be built as a module. If so, the module
+	  will be called zynqmp_ocm_edac.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 2d1641a27a28..634c1cec1588 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -84,3 +84,4 @@ obj-$(CONFIG_EDAC_QCOM)			+= qcom_edac.o
 obj-$(CONFIG_EDAC_ASPEED)		+= aspeed_edac.o
 obj-$(CONFIG_EDAC_BLUEFIELD)		+= bluefield_edac.o
 obj-$(CONFIG_EDAC_DMC520)		+= dmc520_edac.o
+obj-$(CONFIG_EDAC_ZYNQMP_OCM)		+= zynqmp_ocm_edac.o
diff --git a/drivers/edac/zynqmp_ocm_edac.c b/drivers/edac/zynqmp_ocm_edac.c
new file mode 100644
index 000000000000..ee094e983d9b
--- /dev/null
+++ b/drivers/edac/zynqmp_ocm_edac.c
@@ -0,0 +1,643 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx ZynqMP OCM ECC Driver
+ *
+ * Copyright (C) 2022 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#include "edac_module.h"
+
+#define ZYNQMP_OCM_EDAC_MSG_SIZE	256
+
+#define ZYNQMP_OCM_EDAC_STRING	"zynqmp_ocm"
+
+/* Controller registers */
+#define CTRL_OFST			0x0
+#define OCM_ISR_OFST		0x04
+#define OCM_IMR_OFST		0x08
+#define OCM_IEN_OFST		0x0C
+#define OCM_IDS_OFST		0x10
+
+/* ECC control register */
+#define ECC_CTRL_OFST		0x14
+
+/* Correctable error info registers */
+#define CE_FFA_OFST			0x1C
+#define CE_FFD0_OFST		0x20
+#define CE_FFD1_OFST		0x24
+#define CE_FFD2_OFST		0x28
+#define CE_FFD3_OFST		0x2C
+#define CE_FFE_OFST			0x30
+
+/* Uncorrectable error info registers */
+#define UE_FFA_OFST			0x34
+#define UE_FFD0_OFST		0x38
+#define UE_FFD1_OFST		0x3C
+#define UE_FFD2_OFST		0x40
+#define UE_FFD3_OFST		0x44
+#define UE_FFE_OFST			0x48
+
+/* ECC control register bit field definitions */
+#define ECC_CTRL_CLR_CE_ERR	0x40
+#define ECC_CTRL_CLR_UE_ERR	0x80
+
+/* Fault injection data and count registers */
+#define OCM_FID0_OFST		0x4C
+#define OCM_FID1_OFST		0x50
+#define OCM_FID2_OFST		0x54
+#define OCM_FID3_OFST		0x58
+#define OCM_FIC_OFST		0x74
+
+#define UE_MAX_BITPOS_LOWER	31
+#define UE_MIN_BITPOS_UPPER	32
+#define UE_MAX_BITPOS_UPPER	63
+
+/* Interrupt masks */
+#define OCM_CEINTR_MASK		BIT(6)
+#define OCM_UEINTR_MASK		BIT(7)
+#define OCM_ECC_ENABLE_MASK	BIT(0)
+#define OCM_CEUE_MASK		GENMASK(7, 6)
+
+#define OCM_FICOUNT_MASK	GENMASK(23, 0)
+#define OCM_BASEVAL			0xFFFC0000
+#define EDAC_DEVICE			"ZynqMP-OCM"
+
+/**
+ * struct ecc_error_info - ECC error log information
+ * @addr:	Fault generated at this address
+ * @data0:	Generated fault data
+ * @data1:	Generated fault data
+ */
+struct ecc_error_info {
+	u32 addr;
+	u32 data0;
+	u32 data1;
+};
+
+/**
+ * struct zynqmp_ocm_ecc_status - ECC status information to report
+ * @ce_cnt:	Correctable error count
+ * @ue_cnt:	Uncorrectable error count
+ * @ceinfo:	Correctable error log information
+ * @ueinfo:	Uncorrectable error log information
+ */
+struct zynqmp_ocm_ecc_status {
+	u32 ce_cnt;
+	u32 ue_cnt;
+	struct ecc_error_info ceinfo;
+	struct ecc_error_info ueinfo;
+};
+
+/**
+ * struct zynqmp_ocm_edac_priv - DDR memory controller private instance data
+ * @baseaddr:	Base address of the DDR controller
+ * @message:	Buffer for framing the event specific info
+ * @stat:	ECC status information
+ * @p_data:	Pointer to platform data
+ * @ce_cnt:	Correctable Error count
+ * @ue_cnt:	Uncorrectable Error count
+ * @ce_bitpos:	Bit position for Correctable Error
+ * @ue_bitpos0:	First bit position for Uncorrectable Error
+ * @ue_bitpos1:	Second bit position for Uncorrectable Error
+ */
+struct zynqmp_ocm_edac_priv {
+	void __iomem *baseaddr;
+	char message[ZYNQMP_OCM_EDAC_MSG_SIZE];
+	struct zynqmp_ocm_ecc_status stat;
+	const struct zynqmp_ocm_platform_data *p_data;
+	u32 ce_cnt;
+	u32 ue_cnt;
+	u8 ce_bitpos;
+	u8 ue_bitpos0;
+	u8 ue_bitpos1;
+};
+
+/**
+ * zynqmp_ocm_edac_geterror_info - Get the current ecc error info
+ * @base:	Pointer to the base address of the ddr memory controller
+ * @p:		Pointer to the ocm ecc status structure
+ * @mask:	Status register mask value
+ *
+ * Determines there is any ecc error or not
+ *
+ */
+static void zynqmp_ocm_edac_geterror_info(void __iomem *base,
+					  struct zynqmp_ocm_ecc_status *p, int mask)
+{
+	if (mask & OCM_CEINTR_MASK) {
+		p->ce_cnt++;
+		p->ceinfo.data0 = readl(base + CE_FFD0_OFST);
+		p->ceinfo.data1 = readl(base + CE_FFD1_OFST);
+		p->ceinfo.addr = (OCM_BASEVAL | readl(base + CE_FFA_OFST));
+		writel(ECC_CTRL_CLR_CE_ERR, base + OCM_ISR_OFST);
+	} else if (mask & OCM_UEINTR_MASK) {
+		p->ue_cnt++;
+		p->ueinfo.data0 = readl(base + UE_FFD0_OFST);
+		p->ueinfo.data1 = readl(base + UE_FFD1_OFST);
+		p->ueinfo.addr = (OCM_BASEVAL | readl(base + UE_FFA_OFST));
+		writel(ECC_CTRL_CLR_UE_ERR, base + OCM_ISR_OFST);
+	}
+}
+
+/**
+ * zynqmp_ocm_edac_handle_error - Handle controller error types CE and UE
+ * @dci:	Pointer to the edac device controller instance
+ * @p:		Pointer to the ocm ecc status structure
+ *
+ * Handles the controller ECC correctable and uncorrectable error.
+ */
+static void zynqmp_ocm_edac_handle_error(struct edac_device_ctl_info *dci,
+					 struct zynqmp_ocm_ecc_status *p)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+	struct ecc_error_info *pinf;
+
+	if (p->ce_cnt) {
+		pinf = &p->ceinfo;
+		snprintf(priv->message, ZYNQMP_OCM_EDAC_MSG_SIZE,
+			 "\n\rOCM ECC error type :%s\n\r"
+			 "Addr: [0x%X]\n\rFault Data[31:0]: [0x%X]\n\r"
+			 "Fault Data[63:32]: [0x%X]",
+			 "CE", pinf->addr, pinf->data0, pinf->data1);
+		edac_device_handle_ce(dci, 0, 0, priv->message);
+	}
+
+	if (p->ue_cnt) {
+		pinf = &p->ueinfo;
+		snprintf(priv->message, ZYNQMP_OCM_EDAC_MSG_SIZE,
+			 "\n\rOCM ECC error type :%s\n\r"
+			 "Addr: [0x%X]\n\rFault Data[31:0]: [0x%X]\n\r"
+			 "Fault Data[63:32]: [0x%X]",
+			 "UE", pinf->addr, pinf->data0, pinf->data1);
+		edac_device_handle_ue(dci, 0, 0, priv->message);
+	}
+
+	memset(p, 0, sizeof(*p));
+}
+
+/**
+ * zynqmp_ocm_edac_intr_handler - isr routine
+ * @irq:        irq number
+ * @dev_id:     device id pointer
+ *
+ * This is the ISR routine called by edac core interrupt thread.
+ * Used to check and post ECC errors.
+ *
+ * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise
+ */
+static irqreturn_t zynqmp_ocm_edac_intr_handler(int irq, void *dev_id)
+{
+	struct edac_device_ctl_info *dci = dev_id;
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+	int regval;
+
+	regval = readl(priv->baseaddr + OCM_ISR_OFST);
+	if (!(regval & OCM_CEUE_MASK))
+		return IRQ_NONE;
+
+	zynqmp_ocm_edac_geterror_info(priv->baseaddr,
+				      &priv->stat, regval);
+
+	priv->ce_cnt += priv->stat.ce_cnt;
+	priv->ue_cnt += priv->stat.ue_cnt;
+	zynqmp_ocm_edac_handle_error(dci, &priv->stat);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * zynqmp_ocm_edac_get_eccstate - Return the controller ecc status
+ * @base:	Pointer to the ddr memory controller base address
+ *
+ * Get the ECC enable/disable status for the controller
+ *
+ * Return: ecc status 0/1.
+ */
+static bool zynqmp_ocm_edac_get_eccstate(void __iomem *base)
+{
+	return readl(base + ECC_CTRL_OFST) & OCM_ECC_ENABLE_MASK;
+}
+
+static const struct of_device_id zynqmp_ocm_edac_match[] = {
+	{ .compatible = "xlnx,zynqmp-ocmc-1.0"},
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, zynqmp_ocm_edac_match);
+
+/**
+ * zynqmp_ocm_edac_inject_fault_count_show - Shows fault injection count
+ * @dci:        Pointer to the edac device struct
+ * @data:       Pointer to user data
+ *
+ * Shows the fault injection count, once the counter reaches
+ * zero, it injects errors
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_fault_count_show(struct edac_device_ctl_info *dci,
+						       char *data)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	return sprintf(data, "FIC: 0x%x\n\r",
+			readl(priv->baseaddr + OCM_FIC_OFST));
+}
+
+/**
+ * zynqmp_ocm_edac_inject_fault_count_store - write fi count
+ * @dci:	Pointer to the edac device struct
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Update the fault injection count register, once the counter reaches
+ * zero, it injects errors
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_fault_count_store(struct edac_device_ctl_info *dci,
+							const char *data, size_t count)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+	u32 ficount;
+
+	if (!data)
+		return -EFAULT;
+
+	if (kstrtouint(data, 0, &ficount))
+		return -EINVAL;
+
+	ficount &= OCM_FICOUNT_MASK;
+	writel(ficount, priv->baseaddr + OCM_FIC_OFST);
+
+	return count;
+}
+
+/**
+ * zynqmp_ocm_edac_inject_cebitpos_show - Shows CE bit position
+ * @dci:        Pointer to the edac device struct
+ * @data:       Pointer to user data
+ *
+ * Shows the Correctable error bit position,
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_cebitpos_show(struct edac_device_ctl_info
+							*dci, char *data)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (priv->ce_bitpos <= UE_MAX_BITPOS_LOWER)
+		return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID0_OFST))));
+
+	return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID1_OFST))));
+}
+
+/**
+ * zynqmp_ocm_edac_inject_cebitpos_store - Set CE bit position
+ * @dci:	Pointer to the edac device struct
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Set any one bit to inject CE error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_cebitpos_store(struct edac_device_ctl_info *dci,
+						     const char *data, size_t count)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (!data)
+		return -EFAULT;
+
+	if (kstrtou8(data, 0, &priv->ce_bitpos))
+		return -EINVAL;
+
+	if (priv->ce_bitpos <= UE_MAX_BITPOS_LOWER) {
+		writel(1 << priv->ce_bitpos, priv->baseaddr + OCM_FID0_OFST);
+		writel(0, priv->baseaddr + OCM_FID1_OFST);
+	} else if (priv->ce_bitpos <= UE_MAX_BITPOS_UPPER) {
+		writel(1 << (priv->ce_bitpos - UE_MIN_BITPOS_UPPER),
+		       priv->baseaddr + OCM_FID1_OFST);
+		writel(0, priv->baseaddr + OCM_FID0_OFST);
+	} else {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Bit number > 64 is not valid\n");
+	}
+
+	return count;
+}
+
+/**
+ * zynqmp_ocm_edac_inject_uebitpos0_show - Shows UE bit postion0
+ * @dci:        Pointer to the edac device struct
+ * @data:       Pointer to user data
+ *
+ * Shows the one of bit position for UE error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_uebitpos0_show(struct edac_device_ctl_info *dci,
+						     char *data)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (priv->ue_bitpos0 <= UE_MAX_BITPOS_LOWER)
+		return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID0_OFST))));
+
+	return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID1_OFST))));
+}
+
+/**
+ * zynqmp_ocm_edac_inject_uebitpos0_store - set UE bit position0
+ * @dci:	Pointer to the edac device struct
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Set the first bit position for UE Error generation,we need to configure
+ * any two bit positions to inject UE Error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_uebitpos0_store(struct edac_device_ctl_info *dci,
+						      const char *data, size_t count)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (!data)
+		return -EFAULT;
+
+	if (kstrtou8(data, 0, &priv->ue_bitpos0))
+		return -EINVAL;
+
+	if (priv->ue_bitpos0 <= UE_MAX_BITPOS_LOWER)
+		writel(1 << priv->ue_bitpos0, priv->baseaddr + OCM_FID0_OFST);
+	else if (priv->ue_bitpos0 <= UE_MAX_BITPOS_UPPER)
+		writel(1 << (priv->ue_bitpos0 - UE_MIN_BITPOS_UPPER),
+		       priv->baseaddr + OCM_FID1_OFST);
+	else
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Bit position > 64 is not valid\n");
+	edac_printk(KERN_INFO, EDAC_DEVICE,
+		    "Set another bit position for UE\n");
+
+	return count;
+}
+
+/**
+ * zynqmp_ocm_edac_inject_uebitpos1_show - Shows UE bit postion1
+ * @dci:        Pointer to the edac device struct
+ * @data:       Pointer to user data
+ *
+ * Shows the second bit position configured for UE error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_uebitpos1_show(struct edac_device_ctl_info *dci,
+						     char *data)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (priv->ue_bitpos1 <= UE_MAX_BITPOS_LOWER)
+		return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID0_OFST))));
+
+	return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID1_OFST))));
+}
+
+/**
+ * zynqmp_ocm_edac_inject_uebitpos1_store - Set UE second bit position
+ * @dci:	Pointer to the edac device struct
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Set the second bit position for UE Error generation,we need to configure
+ * any two bit positions to inject UE Error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_uebitpos1_store(struct edac_device_ctl_info *dci,
+						      const char *data, size_t count)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+	u32 mask;
+
+	if (!data)
+		return -EFAULT;
+
+	if (kstrtou8(data, 0, &priv->ue_bitpos1))
+		return -EINVAL;
+
+	if (priv->ue_bitpos0 == priv->ue_bitpos1) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Bit positions should not be equal\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * If both bit positions are referring to 32 bit data, then configure
+	 * only FID0 register or if it is 64 bit data, then configure only
+	 * FID1 register.
+	 */
+	if (priv->ue_bitpos0 <= UE_MAX_BITPOS_LOWER &&
+	    priv->ue_bitpos1 <= UE_MAX_BITPOS_LOWER) {
+		mask = (1 << priv->ue_bitpos0);
+		mask |= (1 << priv->ue_bitpos1);
+		writel(mask, priv->baseaddr + OCM_FID0_OFST);
+		writel(0, priv->baseaddr + OCM_FID1_OFST);
+	} else if ((priv->ue_bitpos0 >= UE_MIN_BITPOS_UPPER &&
+		    priv->ue_bitpos0 <= UE_MAX_BITPOS_UPPER) &&
+		   (priv->ue_bitpos1 >= UE_MIN_BITPOS_UPPER &&
+		    priv->ue_bitpos1 <= UE_MAX_BITPOS_UPPER)) {
+		mask = (1 << (priv->ue_bitpos0 - UE_MIN_BITPOS_UPPER));
+		mask |= (1 << (priv->ue_bitpos1 - UE_MIN_BITPOS_UPPER));
+		writel(mask, priv->baseaddr + OCM_FID1_OFST);
+		writel(0, priv->baseaddr + OCM_FID0_OFST);
+	}
+
+	/*
+	 * If one bit position is referring a bit in 32 bit data and other in
+	 * 64 bit data, just configure FID0/FID1 based on uebitpos1.
+	 */
+	if (priv->ue_bitpos0 <= UE_MAX_BITPOS_LOWER &&
+	    (priv->ue_bitpos1 >= UE_MIN_BITPOS_UPPER &&
+	     priv->ue_bitpos1 <= UE_MAX_BITPOS_UPPER)) {
+		writel(1 << (priv->ue_bitpos1 - UE_MIN_BITPOS_UPPER),
+		       priv->baseaddr + OCM_FID1_OFST);
+	} else if ((priv->ue_bitpos0 >= UE_MIN_BITPOS_UPPER &&
+		    priv->ue_bitpos0 <= UE_MAX_BITPOS_UPPER) &&
+		   (priv->ue_bitpos1 <= UE_MAX_BITPOS_LOWER)) {
+		writel(1 << priv->ue_bitpos1,
+		       priv->baseaddr + OCM_FID0_OFST);
+	} else {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Bit position > 64 is not valid, Valid bits:[63:0]\n");
+	}
+
+	edac_printk(KERN_INFO, EDAC_DEVICE,
+		    "UE at Bit Position0: %d Bit Position1: %d\n",
+		    priv->ue_bitpos0, priv->ue_bitpos1);
+
+	return count;
+}
+
+static struct edac_dev_sysfs_attribute zynqmp_ocm_edac_sysfs_attributes[] = {
+	{
+		.attr = {
+			.name = "inject_cebitpos",
+			.mode = (0644)
+		},
+		.show = zynqmp_ocm_edac_inject_cebitpos_show,
+		.store = zynqmp_ocm_edac_inject_cebitpos_store},
+	{
+		.attr = {
+			.name = "inject_uebitpos0",
+			.mode = (0644)
+		},
+		.show = zynqmp_ocm_edac_inject_uebitpos0_show,
+		.store = zynqmp_ocm_edac_inject_uebitpos0_store},
+	{
+		.attr = {
+			.name = "inject_uebitpos1",
+			.mode = (0644)
+		},
+		.show = zynqmp_ocm_edac_inject_uebitpos1_show,
+		.store = zynqmp_ocm_edac_inject_uebitpos1_store},
+	{
+		.attr = {
+			.name = "inject_fault_count",
+			.mode = (0644)
+		},
+		.show = zynqmp_ocm_edac_inject_fault_count_show,
+		.store = zynqmp_ocm_edac_inject_fault_count_store},
+	/* End of list */
+	{
+		.attr = {.name = NULL}
+	}
+};
+
+/**
+ * zynqmp_set_ocm_edac_sysfs_attributes - create sysfs attributes
+ * @edac_dev:	Pointer to the edac device struct
+ *
+ * Creates sysfs entries for error injection
+ */
+static void zynqmp_set_ocm_edac_sysfs_attributes(struct edac_device_ctl_info
+						*edac_dev)
+{
+	edac_dev->sysfs_attributes = zynqmp_ocm_edac_sysfs_attributes;
+}
+
+/**
+ * zynqmp_ocm_edac_probe - Check controller and bind driver
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * Probes a specific controller instance for binding with the driver.
+ *
+ * Return: 0 if the controller instance was successfully bound to the
+ * driver; otherwise error code.
+ */
+static int zynqmp_ocm_edac_probe(struct platform_device *pdev)
+{
+	struct zynqmp_ocm_edac_priv *priv;
+	struct edac_device_ctl_info *dci;
+	void __iomem *baseaddr;
+	struct resource *res;
+	int irq, ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	baseaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(baseaddr))
+		return PTR_ERR(baseaddr);
+
+	if (!zynqmp_ocm_edac_get_eccstate(baseaddr)) {
+		edac_printk(KERN_INFO, EDAC_DEVICE,
+			    "ECC not enabled - Disabling EDAC driver\n");
+		return -ENXIO;
+	}
+
+	dci = edac_device_alloc_ctl_info(sizeof(*priv), ZYNQMP_OCM_EDAC_STRING,
+					 1, ZYNQMP_OCM_EDAC_STRING, 1, 0, NULL, 0,
+					 edac_device_alloc_index());
+	if (!dci) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Unable to allocate EDAC device\n");
+		return -ENOMEM;
+	}
+
+	priv = dci->pvt_info;
+	platform_set_drvdata(pdev, dci);
+	dci->dev = &pdev->dev;
+	priv->baseaddr = baseaddr;
+	dci->mod_name = pdev->dev.driver->name;
+	dci->ctl_name = ZYNQMP_OCM_EDAC_STRING;
+	dci->dev_name = dev_name(&pdev->dev);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "No irq %d in DT\n", irq);
+		ret = irq;
+		goto free_dev_ctl;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq,
+			       zynqmp_ocm_edac_intr_handler,
+			       0, dev_name(&pdev->dev), dci);
+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_DEVICE, "Failed to request Irq\n");
+		goto free_dev_ctl;
+	}
+
+	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IEN_OFST);
+
+	zynqmp_set_ocm_edac_sysfs_attributes(dci);
+	ret = edac_device_add_device(dci);
+	if (ret)
+		goto free_dev_ctl;
+
+	return 0;
+
+free_dev_ctl:
+	edac_device_free_ctl_info(dci);
+
+	return ret;
+}
+
+/**
+ * zynqmp_ocm_edac_remove - Unbind driver from controller
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * Return: Unconditionally 0
+ */
+static int zynqmp_ocm_edac_remove(struct platform_device *pdev)
+{
+	struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IDS_OFST);
+	edac_device_del_device(&pdev->dev);
+	edac_device_free_ctl_info(dci);
+
+	return 0;
+}
+
+static struct platform_driver zynqmp_ocm_edac_driver = {
+	.driver = {
+		   .name = "zynqmp-ocm-edac",
+		   .of_match_table = zynqmp_ocm_edac_match,
+		   },
+	.probe = zynqmp_ocm_edac_probe,
+	.remove = zynqmp_ocm_edac_remove,
+};
+
+module_platform_driver(zynqmp_ocm_edac_driver);
+
+MODULE_AUTHOR("Advanced Micro Devices, Inc");
+MODULE_DESCRIPTION("ZynqMP OCM ECC driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP OCM

[Sai Krishna Potthuri]

Add EDAC support for Xilinx ZynqMP OCM Controller, this driver
reports CE and UE errors based on the interrupts, and also creates ue/ce
sysfs entries for error injection.

Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---
 MAINTAINERS                    |   7 +
 drivers/edac/Kconfig           |   9 +
 drivers/edac/Makefile          |   1 +
 drivers/edac/zynqmp_ocm_edac.c | 643 +++++++++++++++++++++++++++++++++
 4 files changed, 660 insertions(+)
 create mode 100644 drivers/edac/zynqmp_ocm_edac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index edc96cdb85e8..cd4c6c90bca3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21692,6 +21692,13 @@ F:	Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dpdma.yaml
 F:	drivers/dma/xilinx/xilinx_dpdma.c
 F:	include/dt-bindings/dma/xlnx-zynqmp-dpdma.h
 
+XILINX ZYNQMP OCM EDAC DRIVER
+M:	Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
+M:	Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
+F:	drivers/edac/zynqmp_ocm_edac.c
+
 XILINX ZYNQMP PSGTR PHY DRIVER
 M:	Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
 M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 58ab63642e72..fece60f586af 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -539,4 +539,13 @@ config EDAC_DMC520
 	  Support for error detection and correction on the
 	  SoCs with ARM DMC-520 DRAM controller.
 
+config EDAC_ZYNQMP_OCM
+	tristate "Xilinx ZynqMP OCM Controller"
+	depends on ARCH_ZYNQMP
+	help
+	  Support for error detection and correction on the xilinx ZynqMP OCM
+	  controller.
+	  This driver can also be built as a module. If so, the module
+	  will be called zynqmp_ocm_edac.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 2d1641a27a28..634c1cec1588 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -84,3 +84,4 @@ obj-$(CONFIG_EDAC_QCOM)			+= qcom_edac.o
 obj-$(CONFIG_EDAC_ASPEED)		+= aspeed_edac.o
 obj-$(CONFIG_EDAC_BLUEFIELD)		+= bluefield_edac.o
 obj-$(CONFIG_EDAC_DMC520)		+= dmc520_edac.o
+obj-$(CONFIG_EDAC_ZYNQMP_OCM)		+= zynqmp_ocm_edac.o
diff --git a/drivers/edac/zynqmp_ocm_edac.c b/drivers/edac/zynqmp_ocm_edac.c
new file mode 100644
index 000000000000..ee094e983d9b
--- /dev/null
+++ b/drivers/edac/zynqmp_ocm_edac.c
@@ -0,0 +1,643 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx ZynqMP OCM ECC Driver
+ *
+ * Copyright (C) 2022 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#include "edac_module.h"
+
+#define ZYNQMP_OCM_EDAC_MSG_SIZE	256
+
+#define ZYNQMP_OCM_EDAC_STRING	"zynqmp_ocm"
+
+/* Controller registers */
+#define CTRL_OFST			0x0
+#define OCM_ISR_OFST		0x04
+#define OCM_IMR_OFST		0x08
+#define OCM_IEN_OFST		0x0C
+#define OCM_IDS_OFST		0x10
+
+/* ECC control register */
+#define ECC_CTRL_OFST		0x14
+
+/* Correctable error info registers */
+#define CE_FFA_OFST			0x1C
+#define CE_FFD0_OFST		0x20
+#define CE_FFD1_OFST		0x24
+#define CE_FFD2_OFST		0x28
+#define CE_FFD3_OFST		0x2C
+#define CE_FFE_OFST			0x30
+
+/* Uncorrectable error info registers */
+#define UE_FFA_OFST			0x34
+#define UE_FFD0_OFST		0x38
+#define UE_FFD1_OFST		0x3C
+#define UE_FFD2_OFST		0x40
+#define UE_FFD3_OFST		0x44
+#define UE_FFE_OFST			0x48
+
+/* ECC control register bit field definitions */
+#define ECC_CTRL_CLR_CE_ERR	0x40
+#define ECC_CTRL_CLR_UE_ERR	0x80
+
+/* Fault injection data and count registers */
+#define OCM_FID0_OFST		0x4C
+#define OCM_FID1_OFST		0x50
+#define OCM_FID2_OFST		0x54
+#define OCM_FID3_OFST		0x58
+#define OCM_FIC_OFST		0x74
+
+#define UE_MAX_BITPOS_LOWER	31
+#define UE_MIN_BITPOS_UPPER	32
+#define UE_MAX_BITPOS_UPPER	63
+
+/* Interrupt masks */
+#define OCM_CEINTR_MASK		BIT(6)
+#define OCM_UEINTR_MASK		BIT(7)
+#define OCM_ECC_ENABLE_MASK	BIT(0)
+#define OCM_CEUE_MASK		GENMASK(7, 6)
+
+#define OCM_FICOUNT_MASK	GENMASK(23, 0)
+#define OCM_BASEVAL			0xFFFC0000
+#define EDAC_DEVICE			"ZynqMP-OCM"
+
+/**
+ * struct ecc_error_info - ECC error log information
+ * @addr:	Fault generated at this address
+ * @data0:	Generated fault data
+ * @data1:	Generated fault data
+ */
+struct ecc_error_info {
+	u32 addr;
+	u32 data0;
+	u32 data1;
+};
+
+/**
+ * struct zynqmp_ocm_ecc_status - ECC status information to report
+ * @ce_cnt:	Correctable error count
+ * @ue_cnt:	Uncorrectable error count
+ * @ceinfo:	Correctable error log information
+ * @ueinfo:	Uncorrectable error log information
+ */
+struct zynqmp_ocm_ecc_status {
+	u32 ce_cnt;
+	u32 ue_cnt;
+	struct ecc_error_info ceinfo;
+	struct ecc_error_info ueinfo;
+};
+
+/**
+ * struct zynqmp_ocm_edac_priv - DDR memory controller private instance data
+ * @baseaddr:	Base address of the DDR controller
+ * @message:	Buffer for framing the event specific info
+ * @stat:	ECC status information
+ * @p_data:	Pointer to platform data
+ * @ce_cnt:	Correctable Error count
+ * @ue_cnt:	Uncorrectable Error count
+ * @ce_bitpos:	Bit position for Correctable Error
+ * @ue_bitpos0:	First bit position for Uncorrectable Error
+ * @ue_bitpos1:	Second bit position for Uncorrectable Error
+ */
+struct zynqmp_ocm_edac_priv {
+	void __iomem *baseaddr;
+	char message[ZYNQMP_OCM_EDAC_MSG_SIZE];
+	struct zynqmp_ocm_ecc_status stat;
+	const struct zynqmp_ocm_platform_data *p_data;
+	u32 ce_cnt;
+	u32 ue_cnt;
+	u8 ce_bitpos;
+	u8 ue_bitpos0;
+	u8 ue_bitpos1;
+};
+
+/**
+ * zynqmp_ocm_edac_geterror_info - Get the current ecc error info
+ * @base:	Pointer to the base address of the ddr memory controller
+ * @p:		Pointer to the ocm ecc status structure
+ * @mask:	Status register mask value
+ *
+ * Determines there is any ecc error or not
+ *
+ */
+static void zynqmp_ocm_edac_geterror_info(void __iomem *base,
+					  struct zynqmp_ocm_ecc_status *p, int mask)
+{
+	if (mask & OCM_CEINTR_MASK) {
+		p->ce_cnt++;
+		p->ceinfo.data0 = readl(base + CE_FFD0_OFST);
+		p->ceinfo.data1 = readl(base + CE_FFD1_OFST);
+		p->ceinfo.addr = (OCM_BASEVAL | readl(base + CE_FFA_OFST));
+		writel(ECC_CTRL_CLR_CE_ERR, base + OCM_ISR_OFST);
+	} else if (mask & OCM_UEINTR_MASK) {
+		p->ue_cnt++;
+		p->ueinfo.data0 = readl(base + UE_FFD0_OFST);
+		p->ueinfo.data1 = readl(base + UE_FFD1_OFST);
+		p->ueinfo.addr = (OCM_BASEVAL | readl(base + UE_FFA_OFST));
+		writel(ECC_CTRL_CLR_UE_ERR, base + OCM_ISR_OFST);
+	}
+}
+
+/**
+ * zynqmp_ocm_edac_handle_error - Handle controller error types CE and UE
+ * @dci:	Pointer to the edac device controller instance
+ * @p:		Pointer to the ocm ecc status structure
+ *
+ * Handles the controller ECC correctable and uncorrectable error.
+ */
+static void zynqmp_ocm_edac_handle_error(struct edac_device_ctl_info *dci,
+					 struct zynqmp_ocm_ecc_status *p)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+	struct ecc_error_info *pinf;
+
+	if (p->ce_cnt) {
+		pinf = &p->ceinfo;
+		snprintf(priv->message, ZYNQMP_OCM_EDAC_MSG_SIZE,
+			 "\n\rOCM ECC error type :%s\n\r"
+			 "Addr: [0x%X]\n\rFault Data[31:0]: [0x%X]\n\r"
+			 "Fault Data[63:32]: [0x%X]",
+			 "CE", pinf->addr, pinf->data0, pinf->data1);
+		edac_device_handle_ce(dci, 0, 0, priv->message);
+	}
+
+	if (p->ue_cnt) {
+		pinf = &p->ueinfo;
+		snprintf(priv->message, ZYNQMP_OCM_EDAC_MSG_SIZE,
+			 "\n\rOCM ECC error type :%s\n\r"
+			 "Addr: [0x%X]\n\rFault Data[31:0]: [0x%X]\n\r"
+			 "Fault Data[63:32]: [0x%X]",
+			 "UE", pinf->addr, pinf->data0, pinf->data1);
+		edac_device_handle_ue(dci, 0, 0, priv->message);
+	}
+
+	memset(p, 0, sizeof(*p));
+}
+
+/**
+ * zynqmp_ocm_edac_intr_handler - isr routine
+ * @irq:        irq number
+ * @dev_id:     device id pointer
+ *
+ * This is the ISR routine called by edac core interrupt thread.
+ * Used to check and post ECC errors.
+ *
+ * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise
+ */
+static irqreturn_t zynqmp_ocm_edac_intr_handler(int irq, void *dev_id)
+{
+	struct edac_device_ctl_info *dci = dev_id;
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+	int regval;
+
+	regval = readl(priv->baseaddr + OCM_ISR_OFST);
+	if (!(regval & OCM_CEUE_MASK))
+		return IRQ_NONE;
+
+	zynqmp_ocm_edac_geterror_info(priv->baseaddr,
+				      &priv->stat, regval);
+
+	priv->ce_cnt += priv->stat.ce_cnt;
+	priv->ue_cnt += priv->stat.ue_cnt;
+	zynqmp_ocm_edac_handle_error(dci, &priv->stat);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * zynqmp_ocm_edac_get_eccstate - Return the controller ecc status
+ * @base:	Pointer to the ddr memory controller base address
+ *
+ * Get the ECC enable/disable status for the controller
+ *
+ * Return: ecc status 0/1.
+ */
+static bool zynqmp_ocm_edac_get_eccstate(void __iomem *base)
+{
+	return readl(base + ECC_CTRL_OFST) & OCM_ECC_ENABLE_MASK;
+}
+
+static const struct of_device_id zynqmp_ocm_edac_match[] = {
+	{ .compatible = "xlnx,zynqmp-ocmc-1.0"},
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, zynqmp_ocm_edac_match);
+
+/**
+ * zynqmp_ocm_edac_inject_fault_count_show - Shows fault injection count
+ * @dci:        Pointer to the edac device struct
+ * @data:       Pointer to user data
+ *
+ * Shows the fault injection count, once the counter reaches
+ * zero, it injects errors
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_fault_count_show(struct edac_device_ctl_info *dci,
+						       char *data)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	return sprintf(data, "FIC: 0x%x\n\r",
+			readl(priv->baseaddr + OCM_FIC_OFST));
+}
+
+/**
+ * zynqmp_ocm_edac_inject_fault_count_store - write fi count
+ * @dci:	Pointer to the edac device struct
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Update the fault injection count register, once the counter reaches
+ * zero, it injects errors
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_fault_count_store(struct edac_device_ctl_info *dci,
+							const char *data, size_t count)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+	u32 ficount;
+
+	if (!data)
+		return -EFAULT;
+
+	if (kstrtouint(data, 0, &ficount))
+		return -EINVAL;
+
+	ficount &= OCM_FICOUNT_MASK;
+	writel(ficount, priv->baseaddr + OCM_FIC_OFST);
+
+	return count;
+}
+
+/**
+ * zynqmp_ocm_edac_inject_cebitpos_show - Shows CE bit position
+ * @dci:        Pointer to the edac device struct
+ * @data:       Pointer to user data
+ *
+ * Shows the Correctable error bit position,
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_cebitpos_show(struct edac_device_ctl_info
+							*dci, char *data)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (priv->ce_bitpos <= UE_MAX_BITPOS_LOWER)
+		return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID0_OFST))));
+
+	return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID1_OFST))));
+}
+
+/**
+ * zynqmp_ocm_edac_inject_cebitpos_store - Set CE bit position
+ * @dci:	Pointer to the edac device struct
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Set any one bit to inject CE error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_cebitpos_store(struct edac_device_ctl_info *dci,
+						     const char *data, size_t count)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (!data)
+		return -EFAULT;
+
+	if (kstrtou8(data, 0, &priv->ce_bitpos))
+		return -EINVAL;
+
+	if (priv->ce_bitpos <= UE_MAX_BITPOS_LOWER) {
+		writel(1 << priv->ce_bitpos, priv->baseaddr + OCM_FID0_OFST);
+		writel(0, priv->baseaddr + OCM_FID1_OFST);
+	} else if (priv->ce_bitpos <= UE_MAX_BITPOS_UPPER) {
+		writel(1 << (priv->ce_bitpos - UE_MIN_BITPOS_UPPER),
+		       priv->baseaddr + OCM_FID1_OFST);
+		writel(0, priv->baseaddr + OCM_FID0_OFST);
+	} else {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Bit number > 64 is not valid\n");
+	}
+
+	return count;
+}
+
+/**
+ * zynqmp_ocm_edac_inject_uebitpos0_show - Shows UE bit postion0
+ * @dci:        Pointer to the edac device struct
+ * @data:       Pointer to user data
+ *
+ * Shows the one of bit position for UE error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_uebitpos0_show(struct edac_device_ctl_info *dci,
+						     char *data)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (priv->ue_bitpos0 <= UE_MAX_BITPOS_LOWER)
+		return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID0_OFST))));
+
+	return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID1_OFST))));
+}
+
+/**
+ * zynqmp_ocm_edac_inject_uebitpos0_store - set UE bit position0
+ * @dci:	Pointer to the edac device struct
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Set the first bit position for UE Error generation,we need to configure
+ * any two bit positions to inject UE Error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_uebitpos0_store(struct edac_device_ctl_info *dci,
+						      const char *data, size_t count)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (!data)
+		return -EFAULT;
+
+	if (kstrtou8(data, 0, &priv->ue_bitpos0))
+		return -EINVAL;
+
+	if (priv->ue_bitpos0 <= UE_MAX_BITPOS_LOWER)
+		writel(1 << priv->ue_bitpos0, priv->baseaddr + OCM_FID0_OFST);
+	else if (priv->ue_bitpos0 <= UE_MAX_BITPOS_UPPER)
+		writel(1 << (priv->ue_bitpos0 - UE_MIN_BITPOS_UPPER),
+		       priv->baseaddr + OCM_FID1_OFST);
+	else
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Bit position > 64 is not valid\n");
+	edac_printk(KERN_INFO, EDAC_DEVICE,
+		    "Set another bit position for UE\n");
+
+	return count;
+}
+
+/**
+ * zynqmp_ocm_edac_inject_uebitpos1_show - Shows UE bit postion1
+ * @dci:        Pointer to the edac device struct
+ * @data:       Pointer to user data
+ *
+ * Shows the second bit position configured for UE error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_uebitpos1_show(struct edac_device_ctl_info *dci,
+						     char *data)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	if (priv->ue_bitpos1 <= UE_MAX_BITPOS_LOWER)
+		return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID0_OFST))));
+
+	return sprintf(data, "Fault Injection Data Reg: [0x%x]\n\r",
+			((readl(priv->baseaddr + OCM_FID1_OFST))));
+}
+
+/**
+ * zynqmp_ocm_edac_inject_uebitpos1_store - Set UE second bit position
+ * @dci:	Pointer to the edac device struct
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Set the second bit position for UE Error generation,we need to configure
+ * any two bit positions to inject UE Error
+ * Return: Number of bytes copied on success else error code.
+ */
+static ssize_t zynqmp_ocm_edac_inject_uebitpos1_store(struct edac_device_ctl_info *dci,
+						      const char *data, size_t count)
+{
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+	u32 mask;
+
+	if (!data)
+		return -EFAULT;
+
+	if (kstrtou8(data, 0, &priv->ue_bitpos1))
+		return -EINVAL;
+
+	if (priv->ue_bitpos0 == priv->ue_bitpos1) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Bit positions should not be equal\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * If both bit positions are referring to 32 bit data, then configure
+	 * only FID0 register or if it is 64 bit data, then configure only
+	 * FID1 register.
+	 */
+	if (priv->ue_bitpos0 <= UE_MAX_BITPOS_LOWER &&
+	    priv->ue_bitpos1 <= UE_MAX_BITPOS_LOWER) {
+		mask = (1 << priv->ue_bitpos0);
+		mask |= (1 << priv->ue_bitpos1);
+		writel(mask, priv->baseaddr + OCM_FID0_OFST);
+		writel(0, priv->baseaddr + OCM_FID1_OFST);
+	} else if ((priv->ue_bitpos0 >= UE_MIN_BITPOS_UPPER &&
+		    priv->ue_bitpos0 <= UE_MAX_BITPOS_UPPER) &&
+		   (priv->ue_bitpos1 >= UE_MIN_BITPOS_UPPER &&
+		    priv->ue_bitpos1 <= UE_MAX_BITPOS_UPPER)) {
+		mask = (1 << (priv->ue_bitpos0 - UE_MIN_BITPOS_UPPER));
+		mask |= (1 << (priv->ue_bitpos1 - UE_MIN_BITPOS_UPPER));
+		writel(mask, priv->baseaddr + OCM_FID1_OFST);
+		writel(0, priv->baseaddr + OCM_FID0_OFST);
+	}
+
+	/*
+	 * If one bit position is referring a bit in 32 bit data and other in
+	 * 64 bit data, just configure FID0/FID1 based on uebitpos1.
+	 */
+	if (priv->ue_bitpos0 <= UE_MAX_BITPOS_LOWER &&
+	    (priv->ue_bitpos1 >= UE_MIN_BITPOS_UPPER &&
+	     priv->ue_bitpos1 <= UE_MAX_BITPOS_UPPER)) {
+		writel(1 << (priv->ue_bitpos1 - UE_MIN_BITPOS_UPPER),
+		       priv->baseaddr + OCM_FID1_OFST);
+	} else if ((priv->ue_bitpos0 >= UE_MIN_BITPOS_UPPER &&
+		    priv->ue_bitpos0 <= UE_MAX_BITPOS_UPPER) &&
+		   (priv->ue_bitpos1 <= UE_MAX_BITPOS_LOWER)) {
+		writel(1 << priv->ue_bitpos1,
+		       priv->baseaddr + OCM_FID0_OFST);
+	} else {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Bit position > 64 is not valid, Valid bits:[63:0]\n");
+	}
+
+	edac_printk(KERN_INFO, EDAC_DEVICE,
+		    "UE at Bit Position0: %d Bit Position1: %d\n",
+		    priv->ue_bitpos0, priv->ue_bitpos1);
+
+	return count;
+}
+
+static struct edac_dev_sysfs_attribute zynqmp_ocm_edac_sysfs_attributes[] = {
+	{
+		.attr = {
+			.name = "inject_cebitpos",
+			.mode = (0644)
+		},
+		.show = zynqmp_ocm_edac_inject_cebitpos_show,
+		.store = zynqmp_ocm_edac_inject_cebitpos_store},
+	{
+		.attr = {
+			.name = "inject_uebitpos0",
+			.mode = (0644)
+		},
+		.show = zynqmp_ocm_edac_inject_uebitpos0_show,
+		.store = zynqmp_ocm_edac_inject_uebitpos0_store},
+	{
+		.attr = {
+			.name = "inject_uebitpos1",
+			.mode = (0644)
+		},
+		.show = zynqmp_ocm_edac_inject_uebitpos1_show,
+		.store = zynqmp_ocm_edac_inject_uebitpos1_store},
+	{
+		.attr = {
+			.name = "inject_fault_count",
+			.mode = (0644)
+		},
+		.show = zynqmp_ocm_edac_inject_fault_count_show,
+		.store = zynqmp_ocm_edac_inject_fault_count_store},
+	/* End of list */
+	{
+		.attr = {.name = NULL}
+	}
+};
+
+/**
+ * zynqmp_set_ocm_edac_sysfs_attributes - create sysfs attributes
+ * @edac_dev:	Pointer to the edac device struct
+ *
+ * Creates sysfs entries for error injection
+ */
+static void zynqmp_set_ocm_edac_sysfs_attributes(struct edac_device_ctl_info
+						*edac_dev)
+{
+	edac_dev->sysfs_attributes = zynqmp_ocm_edac_sysfs_attributes;
+}
+
+/**
+ * zynqmp_ocm_edac_probe - Check controller and bind driver
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * Probes a specific controller instance for binding with the driver.
+ *
+ * Return: 0 if the controller instance was successfully bound to the
+ * driver; otherwise error code.
+ */
+static int zynqmp_ocm_edac_probe(struct platform_device *pdev)
+{
+	struct zynqmp_ocm_edac_priv *priv;
+	struct edac_device_ctl_info *dci;
+	void __iomem *baseaddr;
+	struct resource *res;
+	int irq, ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	baseaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(baseaddr))
+		return PTR_ERR(baseaddr);
+
+	if (!zynqmp_ocm_edac_get_eccstate(baseaddr)) {
+		edac_printk(KERN_INFO, EDAC_DEVICE,
+			    "ECC not enabled - Disabling EDAC driver\n");
+		return -ENXIO;
+	}
+
+	dci = edac_device_alloc_ctl_info(sizeof(*priv), ZYNQMP_OCM_EDAC_STRING,
+					 1, ZYNQMP_OCM_EDAC_STRING, 1, 0, NULL, 0,
+					 edac_device_alloc_index());
+	if (!dci) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Unable to allocate EDAC device\n");
+		return -ENOMEM;
+	}
+
+	priv = dci->pvt_info;
+	platform_set_drvdata(pdev, dci);
+	dci->dev = &pdev->dev;
+	priv->baseaddr = baseaddr;
+	dci->mod_name = pdev->dev.driver->name;
+	dci->ctl_name = ZYNQMP_OCM_EDAC_STRING;
+	dci->dev_name = dev_name(&pdev->dev);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "No irq %d in DT\n", irq);
+		ret = irq;
+		goto free_dev_ctl;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq,
+			       zynqmp_ocm_edac_intr_handler,
+			       0, dev_name(&pdev->dev), dci);
+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_DEVICE, "Failed to request Irq\n");
+		goto free_dev_ctl;
+	}
+
+	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IEN_OFST);
+
+	zynqmp_set_ocm_edac_sysfs_attributes(dci);
+	ret = edac_device_add_device(dci);
+	if (ret)
+		goto free_dev_ctl;
+
+	return 0;
+
+free_dev_ctl:
+	edac_device_free_ctl_info(dci);
+
+	return ret;
+}
+
+/**
+ * zynqmp_ocm_edac_remove - Unbind driver from controller
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * Return: Unconditionally 0
+ */
+static int zynqmp_ocm_edac_remove(struct platform_device *pdev)
+{
+	struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IDS_OFST);
+	edac_device_del_device(&pdev->dev);
+	edac_device_free_ctl_info(dci);
+
+	return 0;
+}
+
+static struct platform_driver zynqmp_ocm_edac_driver = {
+	.driver = {
+		   .name = "zynqmp-ocm-edac",
+		   .of_match_table = zynqmp_ocm_edac_match,
+		   },
+	.probe = zynqmp_ocm_edac_probe,
+	.remove = zynqmp_ocm_edac_remove,
+};
+
+module_platform_driver(zynqmp_ocm_edac_driver);
+
+MODULE_AUTHOR("Advanced Micro Devices, Inc");
+MODULE_DESCRIPTION("ZynqMP OCM ECC driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP OCM

[Krzysztof Kozlowski]

On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
> Add EDAC support for Xilinx ZynqMP OCM Controller, this driver
> reports CE and UE errors based on the interrupts, and also creates ue/ce
> sysfs entries for error injection.
> 
> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

A bit confusing SoB order, although sometimes rational. Are you sure
about authorship here?

> ---
>  MAINTAINERS                    |   7 +
>  drivers/edac/Kconfig           |   9 +
>  drivers/edac/Makefile          |   1 +
>  drivers/edac/zynqmp_ocm_edac.c | 643 +++++++++++++++++++++++++++++++++
>  4 files changed, 660 insertions(+)
>  create mode 100644 drivers/edac/zynqmp_ocm_edac.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index edc96cdb85e8..cd4c6c90bca3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21692,6 +21692,13 @@ F:	Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dpdma.yaml
>  F:	drivers/dma/xilinx/xilinx_dpdma.c
>  F:	include/dt-bindings/dma/xlnx-zynqmp-dpdma.h
>  
> +XILINX ZYNQMP OCM EDAC DRIVER
> +M:	Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> +M:	Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> +F:	drivers/edac/zynqmp_ocm_edac.c
> +
>  XILINX ZYNQMP PSGTR PHY DRIVER
>  M:	Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>  M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 58ab63642e72..fece60f586af 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -539,4 +539,13 @@ config EDAC_DMC520
>  	  Support for error detection and correction on the
>  	  SoCs with ARM DMC-520 DRAM controller.
>  
> +config EDAC_ZYNQMP_OCM
> +	tristate "Xilinx ZynqMP OCM Controller"
> +	depends on ARCH_ZYNQMP

|| COMPILE_TEST


> +	help
> +	  Support for error de


> +/**
> + * zynqmp_ocm_edac_get_eccstate - Return the controller ecc status
> + * @base:	Pointer to the ddr memory controller base address
> + *
> + * Get the ECC enable/disable status for the controller
> + *
> + * Return: ecc status 0/1.
> + */
> +static bool zynqmp_ocm_edac_get_eccstate(void __iomem *base)
> +{
> +	return readl(base + ECC_CTRL_OFST) & OCM_ECC_ENABLE_MASK;
> +}
> +
> +static const struct of_device_id zynqmp_ocm_edac_match[] = {
> +	{ .compatible = "xlnx,zynqmp-ocmc-1.0"},
> +	{ /* end of table */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, zynqmp_ocm_edac_match);

This goes to the end. Do not embed static variables in the middle of the
code.


> +
> +/**
> + * zynqmp_set_ocm_edac_sysfs_attributes - create sysfs attributes
> + * @edac_dev:	Pointer to the edac device struct
> + *
> + * Creates sysfs entries for error injection
> + */
> +static void zynqmp_set_ocm_edac_sysfs_attributes(struct edac_device_ctl_info
> +						*edac_dev)
> +{
> +	edac_dev->sysfs_attributes = zynqmp_ocm_edac_sysfs_attributes;
> +}
> +
> +/**
> + * zynqmp_ocm_edac_probe - Check controller and bind driver
> + * @pdev:	Pointer to the platform_device struct
> + *
> + * Probes a specific controller instance for binding with the driver.
> + *
> + * Return: 0 if the controller instance was successfully bound to the
> + * driver; otherwise error code.
> + */

Drop the kerneldoc for probe(). It's obvious and exactly the same
everywhere. You could keep it if you write something different than theh
same message for 1000 other probes.

> +static int zynqmp_ocm_edac_probe(struct platform_device *pdev)
> +{
> +	struct zynqmp_ocm_edac_priv *priv;
> +	struct edac_device_ctl_info *dci;
> +	void __iomem *baseaddr;
> +	struct resource *res;
> +	int irq, ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	baseaddr = devm_ioremap_resource(&pdev->dev, res);

There is a wrapper for these.

> +	if (IS_ERR(baseaddr))
> +		return PTR_ERR(baseaddr);
> +
> +	if (!zynqmp_ocm_edac_get_eccstate(baseaddr)) {
> +		edac_printk(KERN_INFO, EDAC_DEVICE,
> +			    "ECC not enabled - Disabling EDAC driver\n");

How do you disable the driver? What if there are two devices - how does
this disables the driver for second device?

> +		return -ENXIO;
> +	}
> +
> +	dci = edac_device_alloc_ctl_info(sizeof(*priv), ZYNQMP_OCM_EDAC_STRING,
> +					 1, ZYNQMP_OCM_EDAC_STRING, 1, 0, NULL, 0,
> +					 edac_device_alloc_index());
> +	if (!dci) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE,
> +			    "Unable to allocate EDAC device\n");

No ENOMEM prints.

> +		return -ENOMEM;
> +	}
> +
> +	priv = dci->pvt_info;
> +	platform_set_drvdata(pdev, dci);
> +	dci->dev = &pdev->dev;
> +	priv->baseaddr = baseaddr;
> +	dci->mod_name = pdev->dev.driver->name;
> +	dci->ctl_name = ZYNQMP_OCM_EDAC_STRING;
> +	dci->dev_name = dev_name(&pdev->dev);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE,
> +			    "No irq %d in DT\n", irq);

The same, no need for printks. Core does it.

> +		ret = irq;
> +		goto free_dev_ctl;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq,
> +			       zynqmp_ocm_edac_intr_handler,
> +			       0, dev_name(&pdev->dev), dci);
> +	if (ret) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE, "Failed to request Irq\n");
> +		goto free_dev_ctl;
> +	}
> +
> +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IEN_OFST);
> +
> +	zynqmp_set_ocm_edac_sysfs_attributes(dci);
> +	ret = edac_device_add_device(dci);
> +	if (ret)
> +		goto free_dev_ctl;
> +
> +	return 0;
> +
> +free_dev_ctl:
> +	edac_device_free_ctl_info(dci);
> +
> +	return ret;
> +}
> +
> +/**
> + * zynqmp_ocm_edac_remove - Unbind driver from controller
> + * @pdev:	Pointer to the platform_device struct
> + *
> + * Return: Unconditionally 0
> + */

Same comment for kerneldoc.

> +static int zynqmp_ocm_edac_remove(struct platform_device *pdev)
> +{
> +	struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
> +	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
> +
> +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IDS_OFST);
> +	edac_device_del_device(&pdev->dev);
> +	edac_device_free_ctl_info(dci);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver zynqmp_ocm_edac_driver = {
> +	.driver = {
> +		   .name = "zynqmp-ocm-edac",
> +		   .of_match_table = zynqmp_ocm_edac_match,
> +		   },
> +	.probe = zynqmp_ocm_edac_probe,
> +	.remove = zynqmp_ocm_edac_remove,
> +};
> +
> +module_platform_driver(zynqmp_ocm_edac_driver);
> +
> +MODULE_AUTHOR("Advanced Micro Devices, Inc");
> +MODULE_DESCRIPTION("ZynqMP OCM ECC driver");
> +MODULE_LICENSE("GPL");


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP OCM

[Krzysztof Kozlowski]

On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
> Add EDAC support for Xilinx ZynqMP OCM Controller, this driver
> reports CE and UE errors based on the interrupts, and also creates ue/ce
> sysfs entries for error injection.
> 
> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

A bit confusing SoB order, although sometimes rational. Are you sure
about authorship here?

> ---
>  MAINTAINERS                    |   7 +
>  drivers/edac/Kconfig           |   9 +
>  drivers/edac/Makefile          |   1 +
>  drivers/edac/zynqmp_ocm_edac.c | 643 +++++++++++++++++++++++++++++++++
>  4 files changed, 660 insertions(+)
>  create mode 100644 drivers/edac/zynqmp_ocm_edac.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index edc96cdb85e8..cd4c6c90bca3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21692,6 +21692,13 @@ F:	Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dpdma.yaml
>  F:	drivers/dma/xilinx/xilinx_dpdma.c
>  F:	include/dt-bindings/dma/xlnx-zynqmp-dpdma.h
>  
> +XILINX ZYNQMP OCM EDAC DRIVER
> +M:	Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> +M:	Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> +F:	drivers/edac/zynqmp_ocm_edac.c
> +
>  XILINX ZYNQMP PSGTR PHY DRIVER
>  M:	Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>  M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 58ab63642e72..fece60f586af 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -539,4 +539,13 @@ config EDAC_DMC520
>  	  Support for error detection and correction on the
>  	  SoCs with ARM DMC-520 DRAM controller.
>  
> +config EDAC_ZYNQMP_OCM
> +	tristate "Xilinx ZynqMP OCM Controller"
> +	depends on ARCH_ZYNQMP

|| COMPILE_TEST


> +	help
> +	  Support for error de


> +/**
> + * zynqmp_ocm_edac_get_eccstate - Return the controller ecc status
> + * @base:	Pointer to the ddr memory controller base address
> + *
> + * Get the ECC enable/disable status for the controller
> + *
> + * Return: ecc status 0/1.
> + */
> +static bool zynqmp_ocm_edac_get_eccstate(void __iomem *base)
> +{
> +	return readl(base + ECC_CTRL_OFST) & OCM_ECC_ENABLE_MASK;
> +}
> +
> +static const struct of_device_id zynqmp_ocm_edac_match[] = {
> +	{ .compatible = "xlnx,zynqmp-ocmc-1.0"},
> +	{ /* end of table */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, zynqmp_ocm_edac_match);

This goes to the end. Do not embed static variables in the middle of the
code.


> +
> +/**
> + * zynqmp_set_ocm_edac_sysfs_attributes - create sysfs attributes
> + * @edac_dev:	Pointer to the edac device struct
> + *
> + * Creates sysfs entries for error injection
> + */
> +static void zynqmp_set_ocm_edac_sysfs_attributes(struct edac_device_ctl_info
> +						*edac_dev)
> +{
> +	edac_dev->sysfs_attributes = zynqmp_ocm_edac_sysfs_attributes;
> +}
> +
> +/**
> + * zynqmp_ocm_edac_probe - Check controller and bind driver
> + * @pdev:	Pointer to the platform_device struct
> + *
> + * Probes a specific controller instance for binding with the driver.
> + *
> + * Return: 0 if the controller instance was successfully bound to the
> + * driver; otherwise error code.
> + */

Drop the kerneldoc for probe(). It's obvious and exactly the same
everywhere. You could keep it if you write something different than theh
same message for 1000 other probes.

> +static int zynqmp_ocm_edac_probe(struct platform_device *pdev)
> +{
> +	struct zynqmp_ocm_edac_priv *priv;
> +	struct edac_device_ctl_info *dci;
> +	void __iomem *baseaddr;
> +	struct resource *res;
> +	int irq, ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	baseaddr = devm_ioremap_resource(&pdev->dev, res);

There is a wrapper for these.

> +	if (IS_ERR(baseaddr))
> +		return PTR_ERR(baseaddr);
> +
> +	if (!zynqmp_ocm_edac_get_eccstate(baseaddr)) {
> +		edac_printk(KERN_INFO, EDAC_DEVICE,
> +			    "ECC not enabled - Disabling EDAC driver\n");

How do you disable the driver? What if there are two devices - how does
this disables the driver for second device?

> +		return -ENXIO;
> +	}
> +
> +	dci = edac_device_alloc_ctl_info(sizeof(*priv), ZYNQMP_OCM_EDAC_STRING,
> +					 1, ZYNQMP_OCM_EDAC_STRING, 1, 0, NULL, 0,
> +					 edac_device_alloc_index());
> +	if (!dci) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE,
> +			    "Unable to allocate EDAC device\n");

No ENOMEM prints.

> +		return -ENOMEM;
> +	}
> +
> +	priv = dci->pvt_info;
> +	platform_set_drvdata(pdev, dci);
> +	dci->dev = &pdev->dev;
> +	priv->baseaddr = baseaddr;
> +	dci->mod_name = pdev->dev.driver->name;
> +	dci->ctl_name = ZYNQMP_OCM_EDAC_STRING;
> +	dci->dev_name = dev_name(&pdev->dev);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE,
> +			    "No irq %d in DT\n", irq);

The same, no need for printks. Core does it.

> +		ret = irq;
> +		goto free_dev_ctl;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq,
> +			       zynqmp_ocm_edac_intr_handler,
> +			       0, dev_name(&pdev->dev), dci);
> +	if (ret) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE, "Failed to request Irq\n");
> +		goto free_dev_ctl;
> +	}
> +
> +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IEN_OFST);
> +
> +	zynqmp_set_ocm_edac_sysfs_attributes(dci);
> +	ret = edac_device_add_device(dci);
> +	if (ret)
> +		goto free_dev_ctl;
> +
> +	return 0;
> +
> +free_dev_ctl:
> +	edac_device_free_ctl_info(dci);
> +
> +	return ret;
> +}
> +
> +/**
> + * zynqmp_ocm_edac_remove - Unbind driver from controller
> + * @pdev:	Pointer to the platform_device struct
> + *
> + * Return: Unconditionally 0
> + */

Same comment for kerneldoc.

> +static int zynqmp_ocm_edac_remove(struct platform_device *pdev)
> +{
> +	struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
> +	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
> +
> +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IDS_OFST);
> +	edac_device_del_device(&pdev->dev);
> +	edac_device_free_ctl_info(dci);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver zynqmp_ocm_edac_driver = {
> +	.driver = {
> +		   .name = "zynqmp-ocm-edac",
> +		   .of_match_table = zynqmp_ocm_edac_match,
> +		   },
> +	.probe = zynqmp_ocm_edac_probe,
> +	.remove = zynqmp_ocm_edac_remove,
> +};
> +
> +module_platform_driver(zynqmp_ocm_edac_driver);
> +
> +MODULE_AUTHOR("Advanced Micro Devices, Inc");
> +MODULE_DESCRIPTION("ZynqMP OCM ECC driver");
> +MODULE_LICENSE("GPL");


Best regards,
Krzysztof

RE: [PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP OCM

[Potthuri, Sai Krishna]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, August 16, 2022 1:36 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Michal Simek
> <michal.simek@xilinx.com>; Borislav Petkov <bp@alien8.de>; Mauro
> Carvalho Chehab <mchehab@kernel.org>; Tony Luck <tony.luck@intel.com>;
> James Morse <james.morse@arm.com>; Robert Richter <rric@kernel.org>
> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> saikrishna12468@gmail.com; git (AMD-Xilinx) <git@amd.com>; Datta,
> Shubhrajyoti <shubhrajyoti.datta@amd.com>
> Subject: Re: [PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP
> OCM
> 
> On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
> > Add EDAC support for Xilinx ZynqMP OCM Controller, this driver reports
> > CE and UE errors based on the interrupts, and also creates ue/ce sysfs
> > entries for error injection.
> >
> > Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> 
> A bit confusing SoB order, although sometimes rational. Are you sure about
> authorship here?
Yes, I am the author of this file and Shubhrajyoti contributed to this.
> 
> > ---
> >  MAINTAINERS                    |   7 +
> >  drivers/edac/Kconfig           |   9 +
> >  drivers/edac/Makefile          |   1 +
> >  drivers/edac/zynqmp_ocm_edac.c | 643
> > +++++++++++++++++++++++++++++++++
> >  4 files changed, 660 insertions(+)
> >  create mode 100644 drivers/edac/zynqmp_ocm_edac.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > edc96cdb85e8..cd4c6c90bca3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -21692,6 +21692,13 @@ F:
> 	Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-
> dpdma.yaml
> >  F:	drivers/dma/xilinx/xilinx_dpdma.c
> >  F:	include/dt-bindings/dma/xlnx-zynqmp-dpdma.h
> >
> > +XILINX ZYNQMP OCM EDAC DRIVER
> > +M:	Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> > +M:	Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> > +F:	drivers/edac/zynqmp_ocm_edac.c
> > +
> >  XILINX ZYNQMP PSGTR PHY DRIVER
> >  M:	Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> >  M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index
> > 58ab63642e72..fece60f586af 100644
> > --- a/drivers/edac/Kconfig
> > +++ b/drivers/edac/Kconfig
> > @@ -539,4 +539,13 @@ config EDAC_DMC520
> >  	  Support for error detection and correction on the
> >  	  SoCs with ARM DMC-520 DRAM controller.
> >
> > +config EDAC_ZYNQMP_OCM
> > +	tristate "Xilinx ZynqMP OCM Controller"
> > +	depends on ARCH_ZYNQMP
> 
> || COMPILE_TEST
I will fix in v2.
> 
> 
> > +	help
> > +	  Support for error de
> 
> 
> > +/**
> > + * zynqmp_ocm_edac_get_eccstate - Return the controller ecc status
> > + * @base:	Pointer to the ddr memory controller base address
> > + *
> > + * Get the ECC enable/disable status for the controller
> > + *
> > + * Return: ecc status 0/1.
> > + */
> > +static bool zynqmp_ocm_edac_get_eccstate(void __iomem *base) {
> > +	return readl(base + ECC_CTRL_OFST) & OCM_ECC_ENABLE_MASK; }
> > +
> > +static const struct of_device_id zynqmp_ocm_edac_match[] = {
> > +	{ .compatible = "xlnx,zynqmp-ocmc-1.0"},
> > +	{ /* end of table */ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, zynqmp_ocm_edac_match);
> 
> This goes to the end. Do not embed static variables in the middle of the code.
I will fix in v2.
> 
> 
> > +
> > +/**
> > + * zynqmp_set_ocm_edac_sysfs_attributes - create sysfs attributes
> > + * @edac_dev:	Pointer to the edac device struct
> > + *
> > + * Creates sysfs entries for error injection  */ static void
> > +zynqmp_set_ocm_edac_sysfs_attributes(struct edac_device_ctl_info
> > +						*edac_dev)
> > +{
> > +	edac_dev->sysfs_attributes = zynqmp_ocm_edac_sysfs_attributes; }
> > +
> > +/**
> > + * zynqmp_ocm_edac_probe - Check controller and bind driver
> > + * @pdev:	Pointer to the platform_device struct
> > + *
> > + * Probes a specific controller instance for binding with the driver.
> > + *
> > + * Return: 0 if the controller instance was successfully bound to the
> > + * driver; otherwise error code.
> > + */
> 
> Drop the kerneldoc for probe(). It's obvious and exactly the same
> everywhere. You could keep it if you write something different than theh
> same message for 1000 other probes.
I will fix in v2.
> 
> > +static int zynqmp_ocm_edac_probe(struct platform_device *pdev) {
> > +	struct zynqmp_ocm_edac_priv *priv;
> > +	struct edac_device_ctl_info *dci;
> > +	void __iomem *baseaddr;
> > +	struct resource *res;
> > +	int irq, ret;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	baseaddr = devm_ioremap_resource(&pdev->dev, res);
> 
> There is a wrapper for these.
I will fix in v2.
> 
> > +	if (IS_ERR(baseaddr))
> > +		return PTR_ERR(baseaddr);
> > +
> > +	if (!zynqmp_ocm_edac_get_eccstate(baseaddr)) {
> > +		edac_printk(KERN_INFO, EDAC_DEVICE,
> > +			    "ECC not enabled - Disabling EDAC driver\n");
> 
> How do you disable the driver? What if there are two devices - how does this
> disables the driver for second device?
Here I am checking the ECC capability of the controller, if ecc is enabled then
i will proceed with the probe otherwise return from here.
If there are two devices, then probe will be called twice, and each device has its
own capabilities.
"Disabling EDAC driver" statement might creating the confusion, i will remove
that statement.
> 
> > +		return -ENXIO;
> > +	}
> > +
> > +	dci = edac_device_alloc_ctl_info(sizeof(*priv),
> ZYNQMP_OCM_EDAC_STRING,
> > +					 1, ZYNQMP_OCM_EDAC_STRING, 1,
> 0, NULL, 0,
> > +					 edac_device_alloc_index());
> > +	if (!dci) {
> > +		edac_printk(KERN_ERR, EDAC_DEVICE,
> > +			    "Unable to allocate EDAC device\n");
> 
> No ENOMEM prints.
I will fix in v2.
> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	priv = dci->pvt_info;
> > +	platform_set_drvdata(pdev, dci);
> > +	dci->dev = &pdev->dev;
> > +	priv->baseaddr = baseaddr;
> > +	dci->mod_name = pdev->dev.driver->name;
> > +	dci->ctl_name = ZYNQMP_OCM_EDAC_STRING;
> > +	dci->dev_name = dev_name(&pdev->dev);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		edac_printk(KERN_ERR, EDAC_DEVICE,
> > +			    "No irq %d in DT\n", irq);
> 
> The same, no need for printks. Core does it.
I will fix in v2.
> 
> > +		ret = irq;
> > +		goto free_dev_ctl;
> > +	}
> > +
> > +	ret = devm_request_irq(&pdev->dev, irq,
> > +			       zynqmp_ocm_edac_intr_handler,
> > +			       0, dev_name(&pdev->dev), dci);
> > +	if (ret) {
> > +		edac_printk(KERN_ERR, EDAC_DEVICE, "Failed to request
> Irq\n");
> > +		goto free_dev_ctl;
> > +	}
> > +
> > +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IEN_OFST);
> > +
> > +	zynqmp_set_ocm_edac_sysfs_attributes(dci);
> > +	ret = edac_device_add_device(dci);
> > +	if (ret)
> > +		goto free_dev_ctl;
> > +
> > +	return 0;
> > +
> > +free_dev_ctl:
> > +	edac_device_free_ctl_info(dci);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_ocm_edac_remove - Unbind driver from controller
> > + * @pdev:	Pointer to the platform_device struct
> > + *
> > + * Return: Unconditionally 0
> > + */
> 
> Same comment for kerneldoc.
I will fix in v2.

Regards
Sai Krishna
> 
> > +static int zynqmp_ocm_edac_remove(struct platform_device *pdev) {
> > +	struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
> > +	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
> > +
> > +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IDS_OFST);
> > +	edac_device_del_device(&pdev->dev);
> > +	edac_device_free_ctl_info(dci);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver zynqmp_ocm_edac_driver = {
> > +	.driver = {
> > +		   .name = "zynqmp-ocm-edac",
> > +		   .of_match_table = zynqmp_ocm_edac_match,
> > +		   },
> > +	.probe = zynqmp_ocm_edac_probe,
> > +	.remove = zynqmp_ocm_edac_remove,
> > +};
> > +
> > +module_platform_driver(zynqmp_ocm_edac_driver);
> > +
> > +MODULE_AUTHOR("Advanced Micro Devices, Inc");
> > +MODULE_DESCRIPTION("ZynqMP OCM ECC driver");
> MODULE_LICENSE("GPL");
> 
> 
> Best regards,
> Krzysztof

RE: [PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP OCM

[Potthuri, Sai Krishna]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, August 16, 2022 1:36 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Michal Simek
> <michal.simek@xilinx.com>; Borislav Petkov <bp@alien8.de>; Mauro
> Carvalho Chehab <mchehab@kernel.org>; Tony Luck <tony.luck@intel.com>;
> James Morse <james.morse@arm.com>; Robert Richter <rric@kernel.org>
> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> saikrishna12468@gmail.com; git (AMD-Xilinx) <git@amd.com>; Datta,
> Shubhrajyoti <shubhrajyoti.datta@amd.com>
> Subject: Re: [PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP
> OCM
> 
> On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
> > Add EDAC support for Xilinx ZynqMP OCM Controller, this driver reports
> > CE and UE errors based on the interrupts, and also creates ue/ce sysfs
> > entries for error injection.
> >
> > Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> 
> A bit confusing SoB order, although sometimes rational. Are you sure about
> authorship here?
Yes, I am the author of this file and Shubhrajyoti contributed to this.
> 
> > ---
> >  MAINTAINERS                    |   7 +
> >  drivers/edac/Kconfig           |   9 +
> >  drivers/edac/Makefile          |   1 +
> >  drivers/edac/zynqmp_ocm_edac.c | 643
> > +++++++++++++++++++++++++++++++++
> >  4 files changed, 660 insertions(+)
> >  create mode 100644 drivers/edac/zynqmp_ocm_edac.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > edc96cdb85e8..cd4c6c90bca3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -21692,6 +21692,13 @@ F:
> 	Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-
> dpdma.yaml
> >  F:	drivers/dma/xilinx/xilinx_dpdma.c
> >  F:	include/dt-bindings/dma/xlnx-zynqmp-dpdma.h
> >
> > +XILINX ZYNQMP OCM EDAC DRIVER
> > +M:	Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> > +M:	Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> > +F:	drivers/edac/zynqmp_ocm_edac.c
> > +
> >  XILINX ZYNQMP PSGTR PHY DRIVER
> >  M:	Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> >  M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index
> > 58ab63642e72..fece60f586af 100644
> > --- a/drivers/edac/Kconfig
> > +++ b/drivers/edac/Kconfig
> > @@ -539,4 +539,13 @@ config EDAC_DMC520
> >  	  Support for error detection and correction on the
> >  	  SoCs with ARM DMC-520 DRAM controller.
> >
> > +config EDAC_ZYNQMP_OCM
> > +	tristate "Xilinx ZynqMP OCM Controller"
> > +	depends on ARCH_ZYNQMP
> 
> || COMPILE_TEST
I will fix in v2.
> 
> 
> > +	help
> > +	  Support for error de
> 
> 
> > +/**
> > + * zynqmp_ocm_edac_get_eccstate - Return the controller ecc status
> > + * @base:	Pointer to the ddr memory controller base address
> > + *
> > + * Get the ECC enable/disable status for the controller
> > + *
> > + * Return: ecc status 0/1.
> > + */
> > +static bool zynqmp_ocm_edac_get_eccstate(void __iomem *base) {
> > +	return readl(base + ECC_CTRL_OFST) & OCM_ECC_ENABLE_MASK; }
> > +
> > +static const struct of_device_id zynqmp_ocm_edac_match[] = {
> > +	{ .compatible = "xlnx,zynqmp-ocmc-1.0"},
> > +	{ /* end of table */ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, zynqmp_ocm_edac_match);
> 
> This goes to the end. Do not embed static variables in the middle of the code.
I will fix in v2.
> 
> 
> > +
> > +/**
> > + * zynqmp_set_ocm_edac_sysfs_attributes - create sysfs attributes
> > + * @edac_dev:	Pointer to the edac device struct
> > + *
> > + * Creates sysfs entries for error injection  */ static void
> > +zynqmp_set_ocm_edac_sysfs_attributes(struct edac_device_ctl_info
> > +						*edac_dev)
> > +{
> > +	edac_dev->sysfs_attributes = zynqmp_ocm_edac_sysfs_attributes; }
> > +
> > +/**
> > + * zynqmp_ocm_edac_probe - Check controller and bind driver
> > + * @pdev:	Pointer to the platform_device struct
> > + *
> > + * Probes a specific controller instance for binding with the driver.
> > + *
> > + * Return: 0 if the controller instance was successfully bound to the
> > + * driver; otherwise error code.
> > + */
> 
> Drop the kerneldoc for probe(). It's obvious and exactly the same
> everywhere. You could keep it if you write something different than theh
> same message for 1000 other probes.
I will fix in v2.
> 
> > +static int zynqmp_ocm_edac_probe(struct platform_device *pdev) {
> > +	struct zynqmp_ocm_edac_priv *priv;
> > +	struct edac_device_ctl_info *dci;
> > +	void __iomem *baseaddr;
> > +	struct resource *res;
> > +	int irq, ret;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	baseaddr = devm_ioremap_resource(&pdev->dev, res);
> 
> There is a wrapper for these.
I will fix in v2.
> 
> > +	if (IS_ERR(baseaddr))
> > +		return PTR_ERR(baseaddr);
> > +
> > +	if (!zynqmp_ocm_edac_get_eccstate(baseaddr)) {
> > +		edac_printk(KERN_INFO, EDAC_DEVICE,
> > +			    "ECC not enabled - Disabling EDAC driver\n");
> 
> How do you disable the driver? What if there are two devices - how does this
> disables the driver for second device?
Here I am checking the ECC capability of the controller, if ecc is enabled then
i will proceed with the probe otherwise return from here.
If there are two devices, then probe will be called twice, and each device has its
own capabilities.
"Disabling EDAC driver" statement might creating the confusion, i will remove
that statement.
> 
> > +		return -ENXIO;
> > +	}
> > +
> > +	dci = edac_device_alloc_ctl_info(sizeof(*priv),
> ZYNQMP_OCM_EDAC_STRING,
> > +					 1, ZYNQMP_OCM_EDAC_STRING, 1,
> 0, NULL, 0,
> > +					 edac_device_alloc_index());
> > +	if (!dci) {
> > +		edac_printk(KERN_ERR, EDAC_DEVICE,
> > +			    "Unable to allocate EDAC device\n");
> 
> No ENOMEM prints.
I will fix in v2.
> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	priv = dci->pvt_info;
> > +	platform_set_drvdata(pdev, dci);
> > +	dci->dev = &pdev->dev;
> > +	priv->baseaddr = baseaddr;
> > +	dci->mod_name = pdev->dev.driver->name;
> > +	dci->ctl_name = ZYNQMP_OCM_EDAC_STRING;
> > +	dci->dev_name = dev_name(&pdev->dev);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		edac_printk(KERN_ERR, EDAC_DEVICE,
> > +			    "No irq %d in DT\n", irq);
> 
> The same, no need for printks. Core does it.
I will fix in v2.
> 
> > +		ret = irq;
> > +		goto free_dev_ctl;
> > +	}
> > +
> > +	ret = devm_request_irq(&pdev->dev, irq,
> > +			       zynqmp_ocm_edac_intr_handler,
> > +			       0, dev_name(&pdev->dev), dci);
> > +	if (ret) {
> > +		edac_printk(KERN_ERR, EDAC_DEVICE, "Failed to request
> Irq\n");
> > +		goto free_dev_ctl;
> > +	}
> > +
> > +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IEN_OFST);
> > +
> > +	zynqmp_set_ocm_edac_sysfs_attributes(dci);
> > +	ret = edac_device_add_device(dci);
> > +	if (ret)
> > +		goto free_dev_ctl;
> > +
> > +	return 0;
> > +
> > +free_dev_ctl:
> > +	edac_device_free_ctl_info(dci);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_ocm_edac_remove - Unbind driver from controller
> > + * @pdev:	Pointer to the platform_device struct
> > + *
> > + * Return: Unconditionally 0
> > + */
> 
> Same comment for kerneldoc.
I will fix in v2.

Regards
Sai Krishna
> 
> > +static int zynqmp_ocm_edac_remove(struct platform_device *pdev) {
> > +	struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
> > +	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
> > +
> > +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IDS_OFST);
> > +	edac_device_del_device(&pdev->dev);
> > +	edac_device_free_ctl_info(dci);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver zynqmp_ocm_edac_driver = {
> > +	.driver = {
> > +		   .name = "zynqmp-ocm-edac",
> > +		   .of_match_table = zynqmp_ocm_edac_match,
> > +		   },
> > +	.probe = zynqmp_ocm_edac_probe,
> > +	.remove = zynqmp_ocm_edac_remove,
> > +};
> > +
> > +module_platform_driver(zynqmp_ocm_edac_driver);
> > +
> > +MODULE_AUTHOR("Advanced Micro Devices, Inc");
> > +MODULE_DESCRIPTION("ZynqMP OCM ECC driver");
> MODULE_LICENSE("GPL");
> 
> 
> Best regards,
> Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel