diff mbox

[RFC] edac: zynq: Added EDAC support for zynq ddr ecc controller

Message ID 4adb2b4b-2ad6-42b9-929e-cc062f817245@TX2EHSMHS021.ehs.local (mailing list archive)
State New, archived
Headers show

Commit Message

Punnaiah Choudary Kalluri March 9, 2014, 2:57 a.m. UTC
Added EDAC support for reporting the ecc errors of zynq ddr controller.
The ddr ecc controller corrects single bit errors and detects double bit
errors

Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
---
 .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
 drivers/edac/Kconfig                               |    7 +
 drivers/edac/Makefile                              |    1 +
 drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
 4 files changed, 639 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
 create mode 100644 drivers/edac/zynq_edac.c

Comments

Mark Rutland March 10, 2014, 10:56 a.m. UTC | #1
On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary Kalluri wrote:
> Added EDAC support for reporting the ecc errors of zynq ddr controller.
> The ddr ecc controller corrects single bit errors and detects double bit
> errors
> 
> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> ---
>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
>  drivers/edac/Kconfig                               |    7 +
>  drivers/edac/Makefile                              |    1 +
>  drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
>  4 files changed, 639 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
>  create mode 100644 drivers/edac/zynq_edac.c
> 
> diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt b/Documentation/devicetree/bindings/edac/zynq_edac.txt
> new file mode 100644
> index 0000000..c21ff83
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
> @@ -0,0 +1,18 @@
> +Zynq EDAC driver, it does reports the DDR ECC single bit errors that are
> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
> +
> +Required properties:
> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"

Is this an or or a xor?

For the former case, which is the most general, which can be used as a
fallback?

How about something like:

- compatible: should contain:
  * "xlnx,ps7-ddrc-1.00.a" for <DESCRIPTION HERE>
  * "xlnx,ps7-ddrc" for any compatible variant

Is there any need at all for the "xlnx,ps7-ddrc" string? The driver
doesn't care about it.

Cheers,
Mark.
Michal Simek March 10, 2014, 11:58 a.m. UTC | #2
On 03/10/2014 11:56 AM, Mark Rutland wrote:
> On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary Kalluri wrote:
>> Added EDAC support for reporting the ecc errors of zynq ddr controller.
>> The ddr ecc controller corrects single bit errors and detects double bit
>> errors
>>
>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>> ---
>>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
>>  drivers/edac/Kconfig                               |    7 +
>>  drivers/edac/Makefile                              |    1 +
>>  drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
>>  4 files changed, 639 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
>>  create mode 100644 drivers/edac/zynq_edac.c
>>
>> diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>> new file mode 100644
>> index 0000000..c21ff83
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>> @@ -0,0 +1,18 @@
>> +Zynq EDAC driver, it does reports the DDR ECC single bit errors that are
>> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
>> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
>> +
>> +Required properties:
>> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"
> 
> Is this an or or a xor?

Compatible string should be just xlnx,zynq-ddrc-1.00.a.
Nothing with ps7.

Thanks,
Michal
Soren Brinkmann March 10, 2014, 10:32 p.m. UTC | #3
On Mon, 2014-03-10 at 12:58PM +0100, Michal Simek wrote:
> On 03/10/2014 11:56 AM, Mark Rutland wrote:
> > On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary Kalluri wrote:
> >> Added EDAC support for reporting the ecc errors of zynq ddr controller.
> >> The ddr ecc controller corrects single bit errors and detects double bit
> >> errors
> >>
> >> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> >> ---
> >>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
> >>  drivers/edac/Kconfig                               |    7 +
> >>  drivers/edac/Makefile                              |    1 +
> >>  drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
> >>  4 files changed, 639 insertions(+), 0 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
> >>  create mode 100644 drivers/edac/zynq_edac.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt b/Documentation/devicetree/bindings/edac/zynq_edac.txt
> >> new file mode 100644
> >> index 0000000..c21ff83
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
> >> @@ -0,0 +1,18 @@
> >> +Zynq EDAC driver, it does reports the DDR ECC single bit errors that are
> >> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
> >> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
> >> +
> >> +Required properties:
> >> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"
> > 
> > Is this an or or a xor?
> 
> Compatible string should be just xlnx,zynq-ddrc-1.00.a.
> Nothing with ps7.

Isn't this vendor IP? IMHO, this should be something completely
different. Or if you want some Zynq-specific compat string it should
refer to an actual version string associated with Zynq. 1.00.a is not,
AFAIK.

	Sören
Punnaiah Choudary Kalluri March 11, 2014, 7:25 a.m. UTC | #4
>-----Original Message-----
>From: Michal Simek [mailto:monstr@monstr.eu]
>Sent: Monday, March 10, 2014 5:29 PM
>To: Mark Rutland
>Cc: Punnaiah Choudary Kalluri; dougthompson@xmission.com;
>devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
>edac@vger.kernel.org; Michal Simek; robh+dt@kernel.org; Pawel Moll;
>ijc+devicetree@hellion.org.uk; galak@codeaurora.org; rob@landley.net;
>kpc528@gmail.com; kalluripunnaiahchoudary@gmail.com; Punnaiah Choudary
>Kalluri
>Subject: Re: [RFC PATCH] edac: zynq: Added EDAC support for zynq ddr ecc
>controller
>
>On 03/10/2014 11:56 AM, Mark Rutland wrote:
>> On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary Kalluri
>wrote:
>>> Added EDAC support for reporting the ecc errors of zynq ddr controller.
>>> The ddr ecc controller corrects single bit errors and detects double
>>> bit errors
>>>
>>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>>> ---
>>>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
>>>  drivers/edac/Kconfig                               |    7 +
>>>  drivers/edac/Makefile                              |    1 +
>>>  drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
>>>  4 files changed, 639 insertions(+), 0 deletions(-)  create mode
>>> 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>  create mode 100644 drivers/edac/zynq_edac.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt
>>> b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>>> new file mode 100644
>>> index 0000000..c21ff83
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>>> @@ -0,0 +1,18 @@
>>> +Zynq EDAC driver, it does reports the DDR ECC single bit errors that
>>> +are corrected and double bit ecc errors that are detected by the DDR ECC
>controller.
>>> +ECC support for DDR is available in half-bus width(16 bit) configuration
>only.
>>> +
>>> +Required properties:
>>> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"
>>
>> Is this an or or a xor?
>
>Compatible string should be just xlnx,zynq-ddrc-1.00.a.
>Nothing with ps7.
Hi Mark and Michal,

I will correct the binding info and send the next version

Regards,
Punnaiah
>
>Thanks,
>Michal
>
>
>--
>Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
>w: www.monstr.eu p: +42-0-721842854
>Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
>Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT
>custodian and responsible for u-boot arm zynq platform
>



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
Michal Simek March 11, 2014, 8:32 a.m. UTC | #5
On 03/10/2014 11:32 PM, Sören Brinkmann wrote:
> On Mon, 2014-03-10 at 12:58PM +0100, Michal Simek wrote:
>> On 03/10/2014 11:56 AM, Mark Rutland wrote:
>>> On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary Kalluri wrote:
>>>> Added EDAC support for reporting the ecc errors of zynq ddr controller.
>>>> The ddr ecc controller corrects single bit errors and detects double bit
>>>> errors
>>>>
>>>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>>>> ---
>>>>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
>>>>  drivers/edac/Kconfig                               |    7 +
>>>>  drivers/edac/Makefile                              |    1 +
>>>>  drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
>>>>  4 files changed, 639 insertions(+), 0 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>>  create mode 100644 drivers/edac/zynq_edac.c
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>> new file mode 100644
>>>> index 0000000..c21ff83
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>> @@ -0,0 +1,18 @@
>>>> +Zynq EDAC driver, it does reports the DDR ECC single bit errors that are
>>>> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
>>>> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
>>>> +
>>>> +Required properties:
>>>> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"
>>>
>>> Is this an or or a xor?
>>
>> Compatible string should be just xlnx,zynq-ddrc-1.00.a.
>> Nothing with ps7.
> 
> Isn't this vendor IP? IMHO, this should be something completely
> different. Or if you want some Zynq-specific compat string it should
> refer to an actual version string associated with Zynq. 1.00.a is not,
> AFAIK.

I have checked with Punnaiah that this is Synopsys DDR memory controller.
Zynq is based on 1.4 version with some customization.
It means I think reasonable solution is
call it drivers/edac/synopsys_edac.c
rename zynq_ in driver to synopsys_ and
use xlnx,zynq-ddrc-1.00.a compatible string because zynq is not using
clean synopsys version.

I am not getting point why you don't like 1.00.a suffix here.
Because of historical point of view compatible strings should be different
for early silicon, silicon v1, silicon v2, silicon v3, etc
but we haven't used it at all that's why we can use xilinx scheme
which we are using for our soft IPs.

Does it sound reasonable?

Thanks,
Michal
Soren Brinkmann March 11, 2014, 3:50 p.m. UTC | #6
On Tue, 2014-03-11 at 09:32AM +0100, Michal Simek wrote:
> On 03/10/2014 11:32 PM, Sören Brinkmann wrote:
> > On Mon, 2014-03-10 at 12:58PM +0100, Michal Simek wrote:
> >> On 03/10/2014 11:56 AM, Mark Rutland wrote:
> >>> On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary Kalluri wrote:
> >>>> Added EDAC support for reporting the ecc errors of zynq ddr controller.
> >>>> The ddr ecc controller corrects single bit errors and detects double bit
> >>>> errors
> >>>>
> >>>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> >>>> ---
> >>>>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
> >>>>  drivers/edac/Kconfig                               |    7 +
> >>>>  drivers/edac/Makefile                              |    1 +
> >>>>  drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
> >>>>  4 files changed, 639 insertions(+), 0 deletions(-)
> >>>>  create mode 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
> >>>>  create mode 100644 drivers/edac/zynq_edac.c
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt b/Documentation/devicetree/bindings/edac/zynq_edac.txt
> >>>> new file mode 100644
> >>>> index 0000000..c21ff83
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
> >>>> @@ -0,0 +1,18 @@
> >>>> +Zynq EDAC driver, it does reports the DDR ECC single bit errors that are
> >>>> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
> >>>> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"
> >>>
> >>> Is this an or or a xor?
> >>
> >> Compatible string should be just xlnx,zynq-ddrc-1.00.a.
> >> Nothing with ps7.
> > 
> > Isn't this vendor IP? IMHO, this should be something completely
> > different. Or if you want some Zynq-specific compat string it should
> > refer to an actual version string associated with Zynq. 1.00.a is not,
> > AFAIK.
> 
> I have checked with Punnaiah that this is Synopsys DDR memory controller.
> Zynq is based on 1.4 version with some customization.
> It means I think reasonable solution is
> call it drivers/edac/synopsys_edac.c
> rename zynq_ in driver to synopsys_ and
> use xlnx,zynq-ddrc-1.00.a compatible string because zynq is not using
> clean synopsys version.
> 
> I am not getting point why you don't like 1.00.a suffix here.
> Because of historical point of view compatible strings should be different
> for early silicon, silicon v1, silicon v2, silicon v3, etc
> but we haven't used it at all that's why we can use xilinx scheme
> which we are using for our soft IPs.

Well, but you refer to Zynq which has silicon revisions you can use to
refer to a specific Zynq version. That matches data sheets. Zynq-1.00.a
is just completely made up and wouldn't be found in any data sheet.

	Sören
Michal Simek March 12, 2014, 10:27 a.m. UTC | #7
On 03/11/2014 04:50 PM, Sören Brinkmann wrote:
> On Tue, 2014-03-11 at 09:32AM +0100, Michal Simek wrote:
>> On 03/10/2014 11:32 PM, Sören Brinkmann wrote:
>>> On Mon, 2014-03-10 at 12:58PM +0100, Michal Simek wrote:
>>>> On 03/10/2014 11:56 AM, Mark Rutland wrote:
>>>>> On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary Kalluri wrote:
>>>>>> Added EDAC support for reporting the ecc errors of zynq ddr controller.
>>>>>> The ddr ecc controller corrects single bit errors and detects double bit
>>>>>> errors
>>>>>>
>>>>>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
>>>>>>  drivers/edac/Kconfig                               |    7 +
>>>>>>  drivers/edac/Makefile                              |    1 +
>>>>>>  drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
>>>>>>  4 files changed, 639 insertions(+), 0 deletions(-)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>>>>  create mode 100644 drivers/edac/zynq_edac.c
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..c21ff83
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>>>> @@ -0,0 +1,18 @@
>>>>>> +Zynq EDAC driver, it does reports the DDR ECC single bit errors that are
>>>>>> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
>>>>>> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"
>>>>>
>>>>> Is this an or or a xor?
>>>>
>>>> Compatible string should be just xlnx,zynq-ddrc-1.00.a.
>>>> Nothing with ps7.
>>>
>>> Isn't this vendor IP? IMHO, this should be something completely
>>> different. Or if you want some Zynq-specific compat string it should
>>> refer to an actual version string associated with Zynq. 1.00.a is not,
>>> AFAIK.
>>
>> I have checked with Punnaiah that this is Synopsys DDR memory controller.
>> Zynq is based on 1.4 version with some customization.
>> It means I think reasonable solution is
>> call it drivers/edac/synopsys_edac.c
>> rename zynq_ in driver to synopsys_ and
>> use xlnx,zynq-ddrc-1.00.a compatible string because zynq is not using
>> clean synopsys version.
>>
>> I am not getting point why you don't like 1.00.a suffix here.
>> Because of historical point of view compatible strings should be different
>> for early silicon, silicon v1, silicon v2, silicon v3, etc
>> but we haven't used it at all that's why we can use xilinx scheme
>> which we are using for our soft IPs.
> 
> Well, but you refer to Zynq which has silicon revisions you can use to
> refer to a specific Zynq version. That matches data sheets. Zynq-1.00.a
> is just completely made up and wouldn't be found in any data sheet.

I didn't reply this one but we discussed this with Soren over phone.
No problem to use Synopsys version A07 according TRM.

Then compatible string is
xlnx,zynq-ddrc-A07 and file name is synopsys_edac.c

Soren: please correct me if I DDRC version is wrong.

Regarding silicon revision in compatible string let's start new thread
out of this ddrc one.

Thanks,
Michal
Soren Brinkmann March 12, 2014, 4:13 p.m. UTC | #8
On Wed, 2014-03-12 at 11:27AM +0100, Michal Simek wrote:
> On 03/11/2014 04:50 PM, Sören Brinkmann wrote:
> > On Tue, 2014-03-11 at 09:32AM +0100, Michal Simek wrote:
> >> On 03/10/2014 11:32 PM, Sören Brinkmann wrote:
> >>> On Mon, 2014-03-10 at 12:58PM +0100, Michal Simek wrote:
> >>>> On 03/10/2014 11:56 AM, Mark Rutland wrote:
> >>>>> On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary Kalluri wrote:
> >>>>>> Added EDAC support for reporting the ecc errors of zynq ddr controller.
> >>>>>> The ddr ecc controller corrects single bit errors and detects double bit
> >>>>>> errors
> >>>>>>
> >>>>>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> >>>>>> ---
> >>>>>>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
> >>>>>>  drivers/edac/Kconfig                               |    7 +
> >>>>>>  drivers/edac/Makefile                              |    1 +
> >>>>>>  drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
> >>>>>>  4 files changed, 639 insertions(+), 0 deletions(-)
> >>>>>>  create mode 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
> >>>>>>  create mode 100644 drivers/edac/zynq_edac.c
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt b/Documentation/devicetree/bindings/edac/zynq_edac.txt
> >>>>>> new file mode 100644
> >>>>>> index 0000000..c21ff83
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
> >>>>>> @@ -0,0 +1,18 @@
> >>>>>> +Zynq EDAC driver, it does reports the DDR ECC single bit errors that are
> >>>>>> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
> >>>>>> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
> >>>>>> +
> >>>>>> +Required properties:
> >>>>>> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"
> >>>>>
> >>>>> Is this an or or a xor?
> >>>>
> >>>> Compatible string should be just xlnx,zynq-ddrc-1.00.a.
> >>>> Nothing with ps7.
> >>>
> >>> Isn't this vendor IP? IMHO, this should be something completely
> >>> different. Or if you want some Zynq-specific compat string it should
> >>> refer to an actual version string associated with Zynq. 1.00.a is not,
> >>> AFAIK.
> >>
> >> I have checked with Punnaiah that this is Synopsys DDR memory controller.
> >> Zynq is based on 1.4 version with some customization.
> >> It means I think reasonable solution is
> >> call it drivers/edac/synopsys_edac.c
> >> rename zynq_ in driver to synopsys_ and
> >> use xlnx,zynq-ddrc-1.00.a compatible string because zynq is not using
> >> clean synopsys version.
> >>
> >> I am not getting point why you don't like 1.00.a suffix here.
> >> Because of historical point of view compatible strings should be different
> >> for early silicon, silicon v1, silicon v2, silicon v3, etc
> >> but we haven't used it at all that's why we can use xilinx scheme
> >> which we are using for our soft IPs.
> > 
> > Well, but you refer to Zynq which has silicon revisions you can use to
> > refer to a specific Zynq version. That matches data sheets. Zynq-1.00.a
> > is just completely made up and wouldn't be found in any data sheet.
> 
> I didn't reply this one but we discussed this with Soren over phone.
> No problem to use Synopsys version A07 according TRM.
> 
> Then compatible string is
> xlnx,zynq-ddrc-A07 and file name is synopsys_edac.c

This seems to make sense. I'm never sure whether leading zeros are
actually needed and tend to remove them, but in general this looks like
a reasonable string to identify Zynq's DDR controller.

	Sören
Michal Simek March 12, 2014, 4:50 p.m. UTC | #9
On 03/12/2014 05:13 PM, Sören Brinkmann wrote:
> On Wed, 2014-03-12 at 11:27AM +0100, Michal Simek wrote:
>> On 03/11/2014 04:50 PM, Sören Brinkmann wrote:
>>> On Tue, 2014-03-11 at 09:32AM +0100, Michal Simek wrote:
>>>> On 03/10/2014 11:32 PM, Sören Brinkmann wrote:
>>>>> On Mon, 2014-03-10 at 12:58PM +0100, Michal Simek wrote:
>>>>>> On 03/10/2014 11:56 AM, Mark Rutland wrote:
>>>>>>> On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary Kalluri wrote:
>>>>>>>> Added EDAC support for reporting the ecc errors of zynq ddr controller.
>>>>>>>> The ddr ecc controller corrects single bit errors and detects double bit
>>>>>>>> errors
>>>>>>>>
>>>>>>>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>>>>>>>> ---
>>>>>>>>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +
>>>>>>>>  drivers/edac/Kconfig                               |    7 +
>>>>>>>>  drivers/edac/Makefile                              |    1 +
>>>>>>>>  drivers/edac/zynq_edac.c                           |  613 ++++++++++++++++++++
>>>>>>>>  4 files changed, 639 insertions(+), 0 deletions(-)
>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>>>>>>  create mode 100644 drivers/edac/zynq_edac.c
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..c21ff83
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
>>>>>>>> @@ -0,0 +1,18 @@
>>>>>>>> +Zynq EDAC driver, it does reports the DDR ECC single bit errors that are
>>>>>>>> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
>>>>>>>> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"
>>>>>>>
>>>>>>> Is this an or or a xor?
>>>>>>
>>>>>> Compatible string should be just xlnx,zynq-ddrc-1.00.a.
>>>>>> Nothing with ps7.
>>>>>
>>>>> Isn't this vendor IP? IMHO, this should be something completely
>>>>> different. Or if you want some Zynq-specific compat string it should
>>>>> refer to an actual version string associated with Zynq. 1.00.a is not,
>>>>> AFAIK.
>>>>
>>>> I have checked with Punnaiah that this is Synopsys DDR memory controller.
>>>> Zynq is based on 1.4 version with some customization.
>>>> It means I think reasonable solution is
>>>> call it drivers/edac/synopsys_edac.c
>>>> rename zynq_ in driver to synopsys_ and
>>>> use xlnx,zynq-ddrc-1.00.a compatible string because zynq is not using
>>>> clean synopsys version.
>>>>
>>>> I am not getting point why you don't like 1.00.a suffix here.
>>>> Because of historical point of view compatible strings should be different
>>>> for early silicon, silicon v1, silicon v2, silicon v3, etc
>>>> but we haven't used it at all that's why we can use xilinx scheme
>>>> which we are using for our soft IPs.
>>>
>>> Well, but you refer to Zynq which has silicon revisions you can use to
>>> refer to a specific Zynq version. That matches data sheets. Zynq-1.00.a
>>> is just completely made up and wouldn't be found in any data sheet.
>>
>> I didn't reply this one but we discussed this with Soren over phone.
>> No problem to use Synopsys version A07 according TRM.
>>
>> Then compatible string is
>> xlnx,zynq-ddrc-A07 and file name is synopsys_edac.c
> 
> This seems to make sense. I'm never sure whether leading zeros are
> actually needed and tend to remove them, but in general this looks like
> a reasonable string to identify Zynq's DDR controller.

A07 I found in Zynq TRM and probably has any conection to synopsys version scheme.

Thanks,
Michal
Punnaiah Choudary Kalluri March 13, 2014, 5:21 p.m. UTC | #10
>-----Original Message-----

>From: Michal Simek [mailto:michal.simek@xilinx.com]

>Sent: Wednesday, March 12, 2014 10:20 PM

>To: Soren Brinkmann

>Cc: Michal Simek; monstr@monstr.eu; Mark Rutland; Punnaiah Choudary

>Kalluri; dougthompson@xmission.com; devicetree@vger.kernel.org; linux-

>doc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-

>kernel@vger.kernel.org; linux-edac@vger.kernel.org; robh+dt@kernel.org;

>Pawel Moll; ijc+devicetree@hellion.org.uk; galak@codeaurora.org;

>rob@landley.net; kpc528@gmail.com; kalluripunnaiahchoudary@gmail.com;

>Punnaiah Choudary Kalluri

>Subject: Re: [RFC PATCH] edac: zynq: Added EDAC support for zynq ddr ecc

>controller

>

>On 03/12/2014 05:13 PM, Sören Brinkmann wrote:

>> On Wed, 2014-03-12 at 11:27AM +0100, Michal Simek wrote:

>>> On 03/11/2014 04:50 PM, Sören Brinkmann wrote:

>>>> On Tue, 2014-03-11 at 09:32AM +0100, Michal Simek wrote:

>>>>> On 03/10/2014 11:32 PM, Sören Brinkmann wrote:

>>>>>> On Mon, 2014-03-10 at 12:58PM +0100, Michal Simek wrote:

>>>>>>> On 03/10/2014 11:56 AM, Mark Rutland wrote:

>>>>>>>> On Sun, Mar 09, 2014 at 02:57:16AM +0000, Punnaiah Choudary

>Kalluri wrote:

>>>>>>>>> Added EDAC support for reporting the ecc errors of zynq ddr

>controller.

>>>>>>>>> The ddr ecc controller corrects single bit errors and detects double

>bit

>>>>>>>>> errors

>>>>>>>>>

>>>>>>>>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>

>>>>>>>>> ---

>>>>>>>>>  .../devicetree/bindings/edac/zynq_edac.txt         |   18 +

>>>>>>>>>  drivers/edac/Kconfig                               |    7 +

>>>>>>>>>  drivers/edac/Makefile                              |    1 +

>>>>>>>>>  drivers/edac/zynq_edac.c                           |  613

>++++++++++++++++++++

>>>>>>>>>  4 files changed, 639 insertions(+), 0 deletions(-)

>>>>>>>>>  create mode 100644

>Documentation/devicetree/bindings/edac/zynq_edac.txt

>>>>>>>>>  create mode 100644 drivers/edac/zynq_edac.c

>>>>>>>>>

>>>>>>>>> diff --git

>a/Documentation/devicetree/bindings/edac/zynq_edac.txt

>b/Documentation/devicetree/bindings/edac/zynq_edac.txt

>>>>>>>>> new file mode 100644

>>>>>>>>> index 0000000..c21ff83

>>>>>>>>> --- /dev/null

>>>>>>>>> +++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt

>>>>>>>>> @@ -0,0 +1,18 @@

>>>>>>>>> +Zynq EDAC driver, it does reports the DDR ECC single bit errors

>that are

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

>ECC controller.

>>>>>>>>> +ECC support for DDR is available in half-bus width(16 bit)

>configuration only.

>>>>>>>>> +

>>>>>>>>> +Required properties:

>>>>>>>>> +- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"

>>>>>>>>

>>>>>>>> Is this an or or a xor?

>>>>>>>

>>>>>>> Compatible string should be just xlnx,zynq-ddrc-1.00.a.

>>>>>>> Nothing with ps7.

>>>>>>

>>>>>> Isn't this vendor IP? IMHO, this should be something completely

>>>>>> different. Or if you want some Zynq-specific compat string it should

>>>>>> refer to an actual version string associated with Zynq. 1.00.a is not,

>>>>>> AFAIK.

>>>>>

>>>>> I have checked with Punnaiah that this is Synopsys DDR memory

>controller.

>>>>> Zynq is based on 1.4 version with some customization.

>>>>> It means I think reasonable solution is

>>>>> call it drivers/edac/synopsys_edac.c

>>>>> rename zynq_ in driver to synopsys_ and

>>>>> use xlnx,zynq-ddrc-1.00.a compatible string because zynq is not using

>>>>> clean synopsys version.

>>>>>

>>>>> I am not getting point why you don't like 1.00.a suffix here.

>>>>> Because of historical point of view compatible strings should be

>different

>>>>> for early silicon, silicon v1, silicon v2, silicon v3, etc

>>>>> but we haven't used it at all that's why we can use xilinx scheme

>>>>> which we are using for our soft IPs.

>>>>

>>>> Well, but you refer to Zynq which has silicon revisions you can use to

>>>> refer to a specific Zynq version. That matches data sheets. Zynq-1.00.a

>>>> is just completely made up and wouldn't be found in any data sheet.

>>>

>>> I didn't reply this one but we discussed this with Soren over phone.

>>> No problem to use Synopsys version A07 according TRM.

>>>

>>> Then compatible string is

>>> xlnx,zynq-ddrc-A07 and file name is synopsys_edac.c

>>

>> This seems to make sense. I'm never sure whether leading zeros are

>> actually needed and tend to remove them, but in general this looks like

>> a reasonable string to identify Zynq's DDR controller.

>

>A07 I found in Zynq TRM and probably has any conection to synopsys version

>scheme.

I have discussed with the design team and confirmed that the synopsis IP
version is 1.4 and looks like it is wrongly updated in TRM as A07.
So, I am going to use compatible string as xlnx,zynq-ddrc-1.4 and file name
as synopsys_edac.c and send you the updated patch soon.
Thanks for the review and suggestions

Regards,
Punnaiah
>

>Thanks,

>Michal

>

>

>




This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/edac/zynq_edac.txt b/Documentation/devicetree/bindings/edac/zynq_edac.txt
new file mode 100644
index 0000000..c21ff83
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/zynq_edac.txt
@@ -0,0 +1,18 @@ 
+Zynq EDAC driver, it does reports the DDR ECC single bit errors that are
+corrected and double bit ecc errors that are detected by the DDR ECC controller.
+ECC support for DDR is available in half-bus width(16 bit) configuration only.
+
+Required properties:
+- compatible: Should be "xlnx,ps7-ddrc" or "xlnx,ps7-ddrc-1.00.a"
+- reg: Should contain DDR controller registers location and length.
+
+Example:
+++++++++
+
+ps7_ddrc_0: ps7-ddrc@f8006000 {
+	compatible = "xlnx,ps7-ddrc-1.00.a", "xlnx,ps7-ddrc";
+	reg = <0xf8006000 0x1000>;
+};
+
+Zynq EDAC driver detects the DDR ECC enable state by reading the appropriate
+control register.
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..644539b 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -326,6 +326,13 @@  config EDAC_TILE
 	  Support for error detection and correction on the
 	  Tilera memory controller.
 
+config EDAC_ZYNQ
+	tristate "ZYNQ DDR Memory Controller"
+	depends on EDAC_MM_EDAC && ARCH_ZYNQ
+	help
+	  This enables support for EDAC on the ECC memory used
+	  with the ZYNQ DDR memory controller.
+
 config EDAC_HIGHBANK_MC
 	tristate "Highbank Memory Controller"
 	depends on EDAC_MM_EDAC && ARCH_HIGHBANK
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 4154ed6..b16e000 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -64,3 +64,4 @@  obj-$(CONFIG_EDAC_OCTEON_PC)		+= octeon_edac-pc.o
 obj-$(CONFIG_EDAC_OCTEON_L2C)		+= octeon_edac-l2c.o
 obj-$(CONFIG_EDAC_OCTEON_LMC)		+= octeon_edac-lmc.o
 obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
+obj-$(CONFIG_EDAC_ZYNQ)			+= zynq_edac.o
diff --git a/drivers/edac/zynq_edac.c b/drivers/edac/zynq_edac.c
new file mode 100644
index 0000000..b525db8
--- /dev/null
+++ b/drivers/edac/zynq_edac.c
@@ -0,0 +1,613 @@ 
+/*
+ * Xilinx Zynq DDR ECC Driver
+ * This driver is based on ppc4xx_edac.c drivers
+ *
+ * Copyright (C) 2012 - 2014 Xilinx, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/edac.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include "edac_core.h"
+
+/* Number of cs_rows needed per memory controller */
+#define ZYNQ_EDAC_NR_CSROWS	1
+
+/* Number of channels per memory controller */
+#define ZYNQ_EDAC_NR_CHANS	1
+
+/* Granularity of reported error in bytes */
+#define ZYNQ_EDAC_ERROR_GRAIN	1
+
+#define ZYNQ_EDAC_MESSAGE_SIZE	256
+
+/* Zynq DDR memory controller registers that are relevant to ECC */
+#define ZYNQ_DDRC_CONTROL_REG_OFFSET	0x0 /* Control regsieter */
+#define ZYNQ_DDRC_T_ZQ_REG_OFFSET	0xA4 /* ZQ register */
+
+/* ECC control register */
+#define ZYNQ_DDRC_ECC_CONTROL_REG_OFFSET	0xC4
+/* ECC log register */
+#define ZYNQ_DDRC_ECC_CE_LOG_REG_OFFSET		0xC8
+/* ECC address register */
+#define ZYNQ_DDRC_ECC_CE_ADDR_REG_OFFSET	0xCC
+/* ECC data[31:0] register */
+#define ZYNQ_DDRC_ECC_CE_DATA_31_0_REG_OFFSET	0xD0
+
+/* Uncorrectable error info regsisters */
+#define ZYNQ_DDRC_ECC_UE_LOG_REG_OFFSET		0xDC /* ECC control register */
+#define ZYNQ_DDRC_ECC_UE_ADDR_REG_OFFSET	0xE0 /* ECC log register */
+#define ZYNQ_DDRC_ECC_UE_DATA_31_0_REG_OFFSET	0xE4 /* ECC address register */
+
+#define ZYNQ_DDRC_ECC_STAT_REG_OFFSET	0xF0 /* ECC statistics register */
+#define ZYNQ_DDRC_ECC_SCRUB_REG_OFFSET	0xF4 /* ECC scrub register */
+
+/* Control regsiter bitfield definitions */
+#define ZYNQ_DDRC_CTRLREG_BUSWIDTH_MASK		0xC
+#define ZYNQ_DDRC_CTRLREG_BUSWIDTH_SHIFT	2
+
+#define ZYNQ_DDRCTL_WDTH_16	1
+#define ZYNQ_DDRCTL_WDTH_32	0
+
+/* ZQ register bitfield definitions */
+#define ZYNQ_DDRC_T_ZQ_REG_DDRMODE_MASK		0x2
+
+/* ECC control register bitfield definitions */
+#define ZYNQ_DDRC_ECCCTRL_CLR_CE_ERR	0x2
+#define ZYNQ_DDRC_ECCCTRL_CLR_UE_ERR	0x1
+
+/* ECC correctable/uncorrectable error log register definitions */
+#define ZYNQ_DDRC_ECC_CE_LOGREG_VALID		0x1
+#define ZYNQ_DDRC_ECC_CE_LOGREG_BITPOS_MASK	0xFE
+#define ZYNQ_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT	1
+
+/* ECC correctable/uncorrectable error address register definitions */
+#define ZYNQ_DDRC_ECC_ADDRREG_COL_MASK		0xFFF
+#define ZYNQ_DDRC_ECC_ADDRREG_ROW_MASK		0xFFFF000
+#define ZYNQ_DDRC_ECC_ADDRREG_ROW_SHIFT		12
+#define ZYNQ_DDRC_ECC_ADDRREG_BANK_MASK		0x70000000
+#define ZYNQ_DDRC_ECC_ADDRREG_BANK_SHIFT	28
+
+/* ECC statistic regsiter definitions */
+#define ZYNQ_DDRC_ECC_STATREG_UECOUNT_MASK	0xFF
+#define ZYNQ_DDRC_ECC_STATREG_CECOUNT_MASK	0xFF00
+#define ZYNQ_DDRC_ECC_STATREG_CECOUNT_SHIFT	8
+
+/* ECC scrub regsiter definitions */
+#define ZYNQ_DDRC_ECC_SCRUBREG_ECC_MODE_MASK	0x7
+#define ZYNQ_DDRC_ECC_SCRUBREG_ECCMODE_SECDED	0x4
+
+/**
+ * struct ecc_error_info - ECC error log information
+ * @row:	Row number
+ * @col:	Column number
+ * @bank:	Bank number
+ * @bitpos:	Bit position
+ * @data:	Data causing the error
+ */
+struct ecc_error_info {
+	u32 row;
+	u32 col;
+	u32 bank;
+	u32 bitpos;
+	u32 data;
+};
+
+/**
+ * struct zynq_ecc_status - ECC status information to report
+ * @ce_count:	Correctable error count
+ * @ue_count:	Uncorrectable error count
+ * @ceinfo:	Correctable error log information
+ * @ueinfo:	Uncorrectable error log information
+ */
+struct zynq_ecc_status {
+	u32 ce_count;
+	u32 ue_count;
+	struct ecc_error_info ceinfo;
+	struct ecc_error_info ueinfo;
+};
+
+/**
+ * struct zynq_edac_priv - Zynq DDR memory controller private instance data
+ * @baseaddr:		Base address of the DDR controller
+ * @ce_count:		Correctable Error count
+ * @ue_count:		Uncorrectable Error count
+ */
+struct zynq_edac_priv {
+	void __iomem *baseaddr;
+	u32 ce_count;
+	u32 ue_count;
+};
+
+/**
+ * zynq_edac_geterror_info - Get the current ecc error info
+ * @base:	Pointer to the base address of the ddr memory controller
+ * @perrstatus:	Pointer to the zynq ecc status structure
+ *
+ * This routine determines there is any ecc error or not
+ *
+ * Return: zero if there is no error otherwise returns 1
+ */
+static int zynq_edac_geterror_info(void __iomem *base,
+		struct zynq_ecc_status *perrstatus)
+{
+	u32 regval;
+	u32 clearval = 0;
+
+	regval = readl(base + ZYNQ_DDRC_ECC_STAT_REG_OFFSET) &
+			(ZYNQ_DDRC_ECC_STATREG_UECOUNT_MASK |
+			ZYNQ_DDRC_ECC_STATREG_CECOUNT_MASK);
+
+	if (regval == 0)
+		return 0;
+
+	memset(perrstatus, 0, sizeof(struct zynq_ecc_status));
+
+	perrstatus->ce_count = (regval & ZYNQ_DDRC_ECC_STATREG_CECOUNT_MASK) >>
+				ZYNQ_DDRC_ECC_STATREG_CECOUNT_SHIFT;
+	perrstatus->ue_count = (regval & ZYNQ_DDRC_ECC_STATREG_UECOUNT_MASK);
+
+	if (perrstatus->ce_count) {
+		regval = readl(base + ZYNQ_DDRC_ECC_CE_LOG_REG_OFFSET);
+		if (regval & ZYNQ_DDRC_ECC_CE_LOGREG_VALID) {
+			perrstatus->ceinfo.bitpos = (regval &
+				ZYNQ_DDRC_ECC_CE_LOGREG_BITPOS_MASK) >>
+				ZYNQ_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT;
+			regval = readl(base +
+					ZYNQ_DDRC_ECC_CE_ADDR_REG_OFFSET);
+			perrstatus->ceinfo.row = (regval &
+					ZYNQ_DDRC_ECC_ADDRREG_ROW_MASK) >>
+					ZYNQ_DDRC_ECC_ADDRREG_ROW_SHIFT;
+			perrstatus->ceinfo.col = (regval &
+					ZYNQ_DDRC_ECC_ADDRREG_COL_MASK);
+			perrstatus->ceinfo.bank = (regval &
+					ZYNQ_DDRC_ECC_ADDRREG_BANK_MASK) >>
+					ZYNQ_DDRC_ECC_ADDRREG_BANK_SHIFT;
+			perrstatus->ceinfo.data = readl(base +
+					ZYNQ_DDRC_ECC_CE_DATA_31_0_REG_OFFSET);
+			edac_dbg(3, "ce bitposition: %d data: %d\n",
+					perrstatus->ceinfo.bitpos,
+					perrstatus->ceinfo.data);
+		}
+		clearval = ZYNQ_DDRC_ECCCTRL_CLR_CE_ERR;
+	}
+
+	if (perrstatus->ue_count) {
+		regval = readl(base + ZYNQ_DDRC_ECC_UE_LOG_REG_OFFSET);
+		if (regval & ZYNQ_DDRC_ECC_CE_LOGREG_VALID) {
+			regval = readl(base +
+					ZYNQ_DDRC_ECC_UE_ADDR_REG_OFFSET);
+			perrstatus->ueinfo.row = (regval &
+					ZYNQ_DDRC_ECC_ADDRREG_ROW_MASK) >>
+					ZYNQ_DDRC_ECC_ADDRREG_ROW_SHIFT;
+			perrstatus->ueinfo.col = (regval &
+					ZYNQ_DDRC_ECC_ADDRREG_COL_MASK);
+			perrstatus->ueinfo.bank = (regval &
+					ZYNQ_DDRC_ECC_ADDRREG_BANK_MASK) >>
+					ZYNQ_DDRC_ECC_ADDRREG_BANK_SHIFT;
+			perrstatus->ueinfo.data = readl(base +
+					ZYNQ_DDRC_ECC_UE_DATA_31_0_REG_OFFSET);
+		}
+		clearval |= ZYNQ_DDRC_ECCCTRL_CLR_UE_ERR;
+	}
+
+	writel(clearval, base + ZYNQ_DDRC_ECC_CONTROL_REG_OFFSET);
+	writel(0x0, base + ZYNQ_DDRC_ECC_CONTROL_REG_OFFSET);
+
+	return 1;
+}
+
+/**
+ * zynq_edac_generate_message - Generate interpreted ECC status message
+ * @mci:	Pointer to the edac memory controller instance
+ * @perrstatus:	Pointer to the zynq ecc status structure
+ * @buffer:	Pointer to the buffer in which to generate the
+ *		message
+ * @size:	The size, in bytes, of space available in buffer
+ *
+ * This routine generates to the provided buffer the portion of the
+ * driver-unique report message associated with the ECC register of
+ * the specified ECC status.
+ */
+static void zynq_edac_generate_message(const struct mem_ctl_info *mci,
+			struct zynq_ecc_status *perrstatus, char *buffer,
+			size_t size)
+{
+	struct ecc_error_info *pinfo = NULL;
+
+	if (perrstatus->ce_count > 0)
+		pinfo = &perrstatus->ceinfo;
+	else
+		pinfo = &perrstatus->ueinfo;
+
+	snprintf(buffer, ZYNQ_EDAC_MESSAGE_SIZE,
+		 "DDR ECC error type :%s Row %d Bank %d Col %d ",
+		 (perrstatus->ce_count > 0) ? "CE" : "UE", pinfo->row,
+		 pinfo->bank, pinfo->col);
+}
+
+/**
+ * zynq_edac_handle_error - Handle controller error types CE and UE
+ * @mci:	Pointer to the edac memory controller instance
+ * @perrstatus:	Pointer to the zynq ecc status structure
+ *
+ * This routine handles an xilinx,ps7-ddrc controller ECC correctable error.
+ */
+static void zynq_edac_handle_error(struct mem_ctl_info *mci,
+			struct zynq_ecc_status *perrstatus)
+{
+	char message[ZYNQ_EDAC_MESSAGE_SIZE];
+
+	zynq_edac_generate_message(mci, perrstatus, &message[0],
+				   ZYNQ_EDAC_MESSAGE_SIZE);
+
+	if (perrstatus->ce_count)
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+				     perrstatus->ce_count, 0, 0, 0, 0, 0, -1,
+				     &message[0], "");
+	else
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+				     perrstatus->ue_count, 0, 0, 0, 0, 0, -1,
+				     &message[0], "");
+}
+
+/**
+ * zynq_edac_check - Check controller for ECC errors
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * This routine is used to check and post ECC errors and is called by
+ * the EDAC polling thread
+ */
+static void zynq_edac_check(struct mem_ctl_info *mci)
+{
+	struct zynq_edac_priv *priv = mci->pvt_info;
+	struct zynq_ecc_status errstatus;
+	int status;
+
+	status = zynq_edac_geterror_info(priv->baseaddr, &errstatus);
+	if (status) {
+		priv->ce_count += errstatus.ce_count;
+		priv->ue_count += errstatus.ue_count;
+
+		if (errstatus.ce_count) {
+			zynq_edac_handle_error(mci, &errstatus);
+			errstatus.ce_count = 0;
+		}
+		if (errstatus.ue_count) {
+			zynq_edac_handle_error(mci, &errstatus);
+			errstatus.ue_count = 0;
+		}
+		edac_dbg(3, "total error count ce %d ue %d\n",
+			 priv->ce_count, priv->ue_count);
+	}
+}
+
+/**
+ * zynq_edac_get_dtype - Return the controller memory width
+ * @base:	Pointer to the ddr memory contoller base address
+ *
+ * This routine returns the EDAC device type width appropriate for the
+ * current controller configuration.
+ *
+ * Return: a device type width enumeration.
+ */
+static enum dev_type zynq_edac_get_dtype(void __iomem *base)
+{
+	enum dev_type dt;
+	u32 width;
+
+	width = readl(base + ZYNQ_DDRC_CONTROL_REG_OFFSET);
+	width = (width & ZYNQ_DDRC_CTRLREG_BUSWIDTH_MASK) >>
+			ZYNQ_DDRC_CTRLREG_BUSWIDTH_SHIFT;
+
+	switch (width) {
+	case ZYNQ_DDRCTL_WDTH_16:
+		dt = DEV_X2;
+		break;
+	case ZYNQ_DDRCTL_WDTH_32:
+		dt = DEV_X4;
+		break;
+	default:
+		dt = DEV_UNKNOWN;
+	}
+
+	return dt;
+}
+
+/**
+ * zynq_edac_get_eccstate - Return the controller ecc enable/disable status
+ * @base:	Pointer to the ddr memory contoller base address
+ *
+ * This routine returns the ECC enable/diable status for the xlnx,ps7-ddrc
+ * controller
+ *
+ * Return: a ecc status boolean i.e true/false - enabled/disabled.
+ */
+static bool zynq_edac_get_eccstate(void __iomem *base)
+{
+	enum dev_type dt;
+	u32 ecctype;
+	bool state = false;
+
+	dt = zynq_edac_get_dtype(base);
+
+	ecctype = (readl(base + ZYNQ_DDRC_ECC_SCRUB_REG_OFFSET) &
+			ZYNQ_DDRC_ECC_SCRUBREG_ECC_MODE_MASK);
+
+	if ((ecctype == ZYNQ_DDRC_ECC_SCRUBREG_ECCMODE_SECDED)
+			&& (dt == DEV_X2)) {
+		state = true;
+		writel(0x0, base + ZYNQ_DDRC_ECC_CONTROL_REG_OFFSET);
+	} else {
+		state = false;
+	}
+
+	return state;
+}
+
+/**
+ * zynq_edac_get_memsize - reads the size of the attached memory device
+ *
+ * Return: the memory size in bytes
+ *
+ * This routine returns the size of the system memory by reading the sysinfo
+ * information
+ */
+static u32 zynq_edac_get_memsize(void)
+{
+	struct sysinfo inf;
+
+	/* Reading the system memory size from the global meminfo structure */
+	si_meminfo(&inf);
+
+	return inf.totalram * inf.mem_unit;
+}
+
+/**
+ * zynq_edac_get_mtype - Returns controller memory type
+ * @base:	pointer to the zynq ecc status structure
+ *
+ * This routine returns the EDAC memory type appropriate for the
+ * current controller configuration.
+ *
+ * Return: a memory type enumeration.
+ */
+static enum mem_type zynq_edac_get_mtype(void __iomem *base)
+{
+	enum mem_type mt;
+	u32 memtype;
+
+	memtype = readl(base + ZYNQ_DDRC_T_ZQ_REG_OFFSET);
+
+	if (memtype & ZYNQ_DDRC_T_ZQ_REG_DDRMODE_MASK)
+		mt = MEM_DDR3;
+	else
+		mt = MEM_DDR2;
+
+	return mt;
+}
+
+/**
+ * zynq_edac_init_csrows - Initialize the cs row data
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * This routine initializes the chip select rows associated
+ * with the EDAC memory controller instance
+ *
+ * Return: 0 if OK; otherwise, -EINVAL if the memory bank size
+ * configuration cannot be determined.
+ */
+static int zynq_edac_init_csrows(struct mem_ctl_info *mci)
+{
+	struct csrow_info *csi;
+	struct dimm_info *dimm;
+	struct zynq_edac_priv *priv = mci->pvt_info;
+	u32 size;
+	int row, j;
+
+	for (row = 0; row < mci->nr_csrows; row++) {
+		csi = mci->csrows[row];
+		size = zynq_edac_get_memsize();
+
+		for (j = 0; j < csi->nr_channels; j++) {
+			dimm = csi->channels[j]->dimm;
+			dimm->edac_mode = EDAC_FLAG_SECDED;
+			dimm->mtype = zynq_edac_get_mtype(priv->baseaddr);
+			dimm->nr_pages =
+			    (size >> PAGE_SHIFT) / csi->nr_channels;
+			dimm->grain = ZYNQ_EDAC_ERROR_GRAIN;
+			dimm->dtype = zynq_edac_get_dtype(priv->baseaddr);
+		}
+
+	}
+
+	return 0;
+}
+
+/**
+ * zynq_edac_mc_init - Initialize driver instance
+ * @mci:	Pointer to the edac memory controller instance
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * This routine performs initialization of the EDAC memory controller
+ * instance and related driver-private data associated with the
+ * xlnx,ps7-ddrc memory controller the instance is bound to.
+ *
+ * Return: 0 if OK; otherwise, < 0 on error.
+ */
+static int zynq_edac_mc_init(struct mem_ctl_info *mci,
+			struct platform_device *pdev)
+{
+	int status;
+	struct zynq_edac_priv *priv;
+
+	/* Initial driver pointers and private data */
+	mci->pdev = &pdev->dev;
+	priv = mci->pvt_info;
+	platform_set_drvdata(pdev, mci);
+
+	/* Initialize controller capabilities and configuration */
+	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
+	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+	mci->scrub_cap = SCRUB_HW_SRC;
+	/* Check the scrub setting from the controller */
+	mci->scrub_mode = SCRUB_NONE;
+
+	mci->edac_cap = EDAC_FLAG_SECDED;
+	/* Initialize strings */
+	mci->ctl_name = "zynq_ddr_controller";
+	mci->dev_name = dev_name(&pdev->dev);
+	mci->mod_name = "zynq_edac";
+	mci->mod_ver = "1";
+
+	/* Initialize callbacks */
+	edac_op_state = EDAC_OPSTATE_POLL;
+	mci->edac_check = zynq_edac_check;
+	mci->ctl_page_to_phys = NULL;
+
+	/*
+	 * Initialize the MC control structure 'csrows' table
+	 * with the mapping and control information.
+	 */
+	status = zynq_edac_init_csrows(mci);
+	if (status)
+		pr_err("Failed to initialize rows!\n");
+
+	return status;
+}
+
+/**
+ * zynq_edac_mc_probe - Check controller and bind driver
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * This routine probes a specific xilinx,ps7-ddrc controller
+ * instance for binding with the driver.
+ *
+ * Return: 0 if the controller instance was successfully bound to the
+ * driver; otherwise, < 0 on error.
+ */
+static int zynq_edac_mc_probe(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[2];
+	struct zynq_edac_priv *priv;
+	int rc;
+	struct resource *res;
+	void __iomem *baseaddr;
+
+	/* Get the data from the platform device */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	baseaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(baseaddr))
+		return PTR_ERR(baseaddr);
+
+	/* Check for the ecc enable status */
+	if (zynq_edac_get_eccstate(baseaddr) == false) {
+		dev_err(&pdev->dev, "ecc not enabled\n");
+		return -ENXIO;
+	}
+
+	/*
+	 * At this point, we know ECC is enabled, allocate an EDAC
+	 * controller instance and perform the appropriate
+	 * initialization.
+	 */
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = ZYNQ_EDAC_NR_CSROWS;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = ZYNQ_EDAC_NR_CHANS;
+	layers[1].is_virt_csrow = false;
+
+	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
+			    sizeof(struct zynq_edac_priv));
+	if (mci == NULL) {
+		pr_err("Failed memory allocation for mci instance!\n");
+		return -ENOMEM;
+	}
+
+	priv = mci->pvt_info;
+	priv->baseaddr = baseaddr;
+	rc = zynq_edac_mc_init(mci, pdev);
+	if (rc) {
+		pr_err("Failed to initialize instance!\n");
+		goto free_edac_mc;
+	}
+
+	/*
+	 * We have a valid, initialized EDAC instance bound to the
+	 * controller. Attempt to register it with the EDAC subsystem
+	 */
+	rc = edac_mc_add_mc(mci);
+	if (rc) {
+		dev_err(&pdev->dev, "failed to register with EDAC core\n");
+		goto del_edac_mc;
+	}
+
+	return rc;
+
+del_edac_mc:
+	edac_mc_del_mc(&pdev->dev);
+free_edac_mc:
+	edac_mc_free(mci);
+
+	return rc;
+}
+
+/**
+ * zynq_edac_mc_remove - Unbind driver from controller
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * This routine unbinds the EDAC memory controller instance associated
+ * with the specified xilinx,ps7-ddrc controller described by the
+ * OpenFirmware device tree node passed as a parameter.
+ *
+ * Return: Unconditionally 0
+ */
+static int zynq_edac_mc_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+
+	return 0;
+}
+
+/* Device tree node type and compatible tuples this driver can match on */
+static struct of_device_id zynq_edac_match[] = {
+	{ .compatible = "xlnx,zynq-ddrc-1.00.a", },
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, zynq_edac_match);
+
+static struct platform_driver zynq_edac_mc_driver = {
+	.driver = {
+		   .name = "zynq-edac",
+		   .owner = THIS_MODULE,
+		   .of_match_table = zynq_edac_match,
+		   },
+	.probe = zynq_edac_mc_probe,
+	.remove = zynq_edac_mc_remove,
+};
+
+module_platform_driver(zynq_edac_mc_driver);
+
+MODULE_AUTHOR("Xilinx, Inc.");
+MODULE_DESCRIPTION("Zynq DDR ECC driver");
+MODULE_LICENSE("GPL v2");