Re: [RFC PATCH v2] edac: synopsys: Added EDAC support for zynq ddr ecc controller

[punnaiah choudary kalluri]

On Wed, Apr 9, 2014 at 2:10 AM, Borislav Petkov <bp@xxxxxxxxx> wrote:
> On Mon, Mar 17, 2014 at 10:53:44AM +0530, Punnaiah Choudary Kalluri wrote:
>> Added EDAC support for reporting the ecc errors of synopsys ddr controller.
>> The ddr ecc controller corrects single bit errors and detects double bit
>> errors
>>
>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xxxxxxxxxx>
>> ---
>> Changes for v2:
>> - Updated the commit header and message
>> - Renamed the filenames to synopsys_edac
>> - Corrected the compatilble string, commnets
>> - Renamed the macros,fucntions and data structures
>> ---
>>  .../devicetree/bindings/edac/synopsys_edac.txt     |   18 +
>>  drivers/edac/Kconfig                               |    7 +
>>  drivers/edac/Makefile                              |    1 +
>>  drivers/edac/synopsys_edac.c                       |  614 ++++++++++++++++++++
>>  4 files changed, 640 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/edac/synopsys_edac.txt
>>  create mode 100644 drivers/edac/synopsys_edac.c
>>
>> diff --git a/Documentation/devicetree/bindings/edac/synopsys_edac.txt b/Documentation/devicetree/bindings/edac/synopsys_edac.txt
>> new file mode 100644
>> index 0000000..c4a559b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/edac/synopsys_edac.txt
>> @@ -0,0 +1,18 @@
>> +Synopsys 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,zynq-ddrc-1.04"
>> +- reg: Should contain DDR controller registers location and length.
>> +
>> +Example:
>> +++++++++
>> +
>> +ddrc0: ddrc@f8006000 {
>> +     compatible = "xlnx,zynq-ddrc-1.04";
>> +     reg = <0xf8006000 0x1000>;
>> +};
>> +
>> +Synopsys 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..58b69b1 100644
>> --- a/drivers/edac/Kconfig
>> +++ b/drivers/edac/Kconfig
>> @@ -368,4 +368,11 @@ config EDAC_OCTEON_PCI
>>         Support for error detection and correction on the
>>         Cavium Octeon family of SOCs.
>>
>> +config EDAC_SYNOPSYS
>> +     tristate "Synopsys DDR Memory Controller"
>> +     depends on EDAC_MM_EDAC && ARCH_ZYNQ
>> +     help
>> +       This enables support for EDAC on the ECC memory used
>> +       with the Synopsys DDR memory controller.
>
> s/This enables s/S/
>
>> +
>>  endif # EDAC
>> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
>> index 4154ed6..5628a6f 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_SYNOPSYS)          += synopsys_edac.o
>> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
>> new file mode 100644
>> index 0000000..7cec331
>> --- /dev/null
>> +++ b/drivers/edac/synopsys_edac.c
>> @@ -0,0 +1,614 @@
>> +/*
>> + * Synopsys DDR ECC Driver
>> + * This driver is based on ppc4xx_edac.c drivers
>> + *
>> + * Copyright (C) 2012 - 2014 Xilinx, Inc.
>
> The same question to you: are you going to maintain this driver? If so,
> please consider adding yourself to the MAINTAINERS file so that people
> reporting issues with it can send you a note.

Let me check internally with team and get back to you on this.
>
>> + *
>> + * 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/>.
>> + */
>
> Please drop the GPL boilerplate - simply refer to the COPYING file in
> the kernel source repository instead.
>
>> +
>> +#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 SYNOPSYS_EDAC_NR_CSROWS      1
>> +
>> +/* Number of channels per memory controller */
>> +#define SYNOPSYS_EDAC_NR_CHANS       1
>> +
>> +/* Granularity of reported error in bytes */
>> +#define SYNOPSYS_EDAC_ERROR_GRAIN    1
>> +
>> +#define SYNOPSYS_EDAC_MESSAGE_SIZE   256
>> +
>> +/* Synopsys DDR memory controller registers that are relevant to ECC */
>> +#define SYNOPSYS_DDRC_CONTROL_REG_OFFSET     0x0 /* Control regsieter */
>> +#define SYNOPSYS_DDRC_T_ZQ_REG_OFFSET        0xA4 /* ZQ register */
>> +
>> +/* ECC control register */
>> +#define SYNOPSYS_DDRC_ECC_CONTROL_REG_OFFSET 0xC4
>> +/* ECC log register */
>> +#define SYNOPSYS_DDRC_ECC_CE_LOG_REG_OFFSET          0xC8
>> +/* ECC address register */
>> +#define SYNOPSYS_DDRC_ECC_CE_ADDR_REG_OFFSET 0xCC
>> +/* ECC data[31:0] register */
>> +#define SYNOPSYS_DDRC_ECC_CE_DATA_31_0_REG_OFFSET    0xD0
>> +
>> +/* Uncorrectable error info regsisters */
>> +#define SYNOPSYS_DDRC_ECC_UE_LOG_REG_OFFSET  0xDC /* ECC log register */
>> +#define SYNOPSYS_DDRC_ECC_UE_ADDR_REG_OFFSET 0xE0 /* ECC address register */
>> +#define SYNOPSYS_DDRC_ECC_UE_DATA_31_0_REG_OFFSET    0xE4 /* ECC data reg */
>> +
>> +#define SYNOPSYS_DDRC_ECC_STAT_REG_OFFSET    0xF0 /* ECC stats register */
>> +#define SYNOPSYS_DDRC_ECC_SCRUB_REG_OFFSET   0xF4 /* ECC scrub register */
>> +
>> +/* Control regsiter bitfield definitions */
>> +#define SYNOPSYS_DDRC_CTRLREG_BUSWIDTH_MASK  0xC
>> +#define SYNOPSYS_DDRC_CTRLREG_BUSWIDTH_SHIFT 2
>> +
>> +#define SYNOPSYS_DDRCTL_WDTH_16      1
>> +#define SYNOPSYS_DDRCTL_WDTH_32      0
>> +
>> +/* ZQ register bitfield definitions */
>> +#define SYNOPSYS_DDRC_T_ZQ_REG_DDRMODE_MASK  0x2
>> +
>> +/* ECC control register bitfield definitions */
>> +#define SYNOPSYS_DDRC_ECCCTRL_CLR_CE_ERR     0x2
>> +#define SYNOPSYS_DDRC_ECCCTRL_CLR_UE_ERR     0x1
>> +
>> +/* ECC correctable/uncorrectable error log register definitions */
>> +#define SYNOPSYS_DDRC_ECC_CE_LOGREG_VALID            0x1
>> +#define SYNOPSYS_DDRC_ECC_CE_LOGREG_BITPOS_MASK              0xFE
>> +#define SYNOPSYS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT     1
>> +
>> +/* ECC correctable/uncorrectable error address register definitions */
>> +#define SYNOPSYS_DDRC_ECC_ADDRREG_COL_MASK           0xFFF
>> +#define SYNOPSYS_DDRC_ECC_ADDRREG_ROW_MASK           0xFFFF000
>> +#define SYNOPSYS_DDRC_ECC_ADDRREG_ROW_SHIFT          12
>> +#define SYNOPSYS_DDRC_ECC_ADDRREG_BANK_MASK          0x70000000
>> +#define SYNOPSYS_DDRC_ECC_ADDRREG_BANK_SHIFT 28
>> +
>> +/* ECC statistic regsiter definitions */
>> +#define SYNOPSYS_DDRC_ECC_STATREG_UECOUNT_MASK       0xFF
>> +#define SYNOPSYS_DDRC_ECC_STATREG_CECOUNT_MASK       0xFF00
>> +#define SYNOPSYS_DDRC_ECC_STATREG_CECOUNT_SHIFT      8
>> +
>> +/* ECC scrub regsiter definitions */
>> +#define SYNOPSYS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK     0x7
>> +#define SYNOPSYS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED    0x4
>
> Those defines and their values could use better alignment. Also see
> below the note about slimming down those names in general.
>
>> +/**
>> + * 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 synopsys_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 synopsys_ecc_status {
>> +     u32 ce_count;
>> +     u32 ue_count;
>> +     struct ecc_error_info ceinfo;
>> +     struct ecc_error_info ueinfo;
>> +};
>> +
>> +/**
>> + * struct synopsys_edac_priv - DDR memory controller private instance data
>> + * @baseaddr:                Base address of the DDR controller
>> + * @ce_count:                Correctable Error count
>> + * @ue_count:                Uncorrectable Error count
>> + */
>> +struct synopsys_edac_priv {
>> +     void __iomem *baseaddr;
>> +     u32 ce_count;
>> +     u32 ue_count;
>> +};
>> +
>> +/**
>
> Why do we need the kernel-doc annotation for all those static functions?

Since it is recommended in Documentation/kernel-doc-nano-HOWTO.txt
but also said it is low priority and at the discretion of the MAINTAINER of
that kernel source file

So, if you recommend not use kernel-doc annotation then i will take care
in next version.
>
> Also, drop the "synopsys_edac_" prefix of all static functions - that'll
> slim up the code even further.

Ok. it means using geterr_info is sufficient than synopsys_edac_geterror_info
and applicable to other static functions
>
>> + * synopsys_edac_geterror_info - Get the current ecc error info
>> + * @base:    Pointer to the base address of the ddr memory controller
>> + * @perrstatus:      Pointer to the synopsys 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 synopsys_edac_geterror_info(void __iomem *base,
>
> That base is an arg to readl, so it should be "const volatile void
> __iomem *base", no?

Yes. I will fix
>
>> +             struct synopsys_ecc_status *perrstatus)
>
> Please align function args at the opening brace "("
>
>> +{
>> +     u32 regval;
>> +     u32 clearval = 0;
>> +
>> +     regval = readl(base + SYNOPSYS_DDRC_ECC_STAT_REG_OFFSET) &
>> +                     (SYNOPSYS_DDRC_ECC_STATREG_UECOUNT_MASK |
>> +                     SYNOPSYS_DDRC_ECC_STATREG_CECOUNT_MASK);
>
> All your macro definitions start with "SYNOPSYS_DDRC" even though
> they're only local to this file. Which makes the code very unreadable
> due to their size.
>
> You could slim them up by dropping the prefix and doing:
>
>         regval = readl(base + STAT_REG_OFFSET) &
>                         (STATREG_UECOUNT_MASK | STATREG_CECOUNT_MASK);
>
> which is much better IMO.
>
> The same is true for struct names like synopsys_ecc_status and the like.
> Shortening them too would slim down your code considerably.
>
>> +
>> +     if (regval == 0)
>> +             return 0;
>
>         if (!regval)
>
>> +
>> +     memset(perrstatus, 0, sizeof(struct synopsys_ecc_status));
>
> That perrstatus could also be shortened to errstat and make the whole
> code shorter:
>
>         memset(errstat, 0, sizeof(*errstat));
>
> and so on...
>
>> +
>> +     perrstatus->ce_count =
>> +                        (regval & SYNOPSYS_DDRC_ECC_STATREG_CECOUNT_MASK) >>
>> +                         SYNOPSYS_DDRC_ECC_STATREG_CECOUNT_SHIFT;
>> +     perrstatus->ue_count =
>> +                        (regval & SYNOPSYS_DDRC_ECC_STATREG_UECOUNT_MASK);
>> +
>> +     if (perrstatus->ce_count) {
>> +             regval = readl(base + SYNOPSYS_DDRC_ECC_CE_LOG_REG_OFFSET);
>> +             if (regval & SYNOPSYS_DDRC_ECC_CE_LOGREG_VALID) {
>> +                     perrstatus->ceinfo.bitpos = (regval &
>> +                             SYNOPSYS_DDRC_ECC_CE_LOGREG_BITPOS_MASK) >>
>> +                             SYNOPSYS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT;
>> +                     regval = readl(base +
>> +                                     SYNOPSYS_DDRC_ECC_CE_ADDR_REG_OFFSET);
>> +                     perrstatus->ceinfo.row = (regval &
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_ROW_MASK) >>
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_ROW_SHIFT;
>> +                     perrstatus->ceinfo.col = (regval &
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_COL_MASK);
>> +                     perrstatus->ceinfo.bank = (regval &
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_BANK_MASK) >>
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_BANK_SHIFT;
>> +                     perrstatus->ceinfo.data = readl(base +
>> +                             SYNOPSYS_DDRC_ECC_CE_DATA_31_0_REG_OFFSET);
>> +                     edac_dbg(3, "ce bitposition: %d data: %d\n",
>> +                                     perrstatus->ceinfo.bitpos,
>> +                                     perrstatus->ceinfo.data);
>> +             }
>> +             clearval = SYNOPSYS_DDRC_ECCCTRL_CLR_CE_ERR;
>> +     }
>> +
>> +     if (perrstatus->ue_count) {
>> +             regval = readl(base + SYNOPSYS_DDRC_ECC_UE_LOG_REG_OFFSET);
>> +             if (regval & SYNOPSYS_DDRC_ECC_CE_LOGREG_VALID) {
>> +                     regval = readl(base +
>> +                                     SYNOPSYS_DDRC_ECC_UE_ADDR_REG_OFFSET);
>> +                     perrstatus->ueinfo.row = (regval &
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_ROW_MASK) >>
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_ROW_SHIFT;
>> +                     perrstatus->ueinfo.col = (regval &
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_COL_MASK);
>> +                     perrstatus->ueinfo.bank = (regval &
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_BANK_MASK) >>
>> +                                     SYNOPSYS_DDRC_ECC_ADDRREG_BANK_SHIFT;
>> +                     perrstatus->ueinfo.data = readl(base +
>> +                                 SYNOPSYS_DDRC_ECC_UE_DATA_31_0_REG_OFFSET);
>
> All those assignments could be much shorter:
>
>                         p->ueinfo.row = (regval & ADDRREG_ROW_MASK) >> ADDRREG_ROW_SHIFT;
>
> Also, you can save yourself two indentation levels by flipping the
> logic:
>
>         if (!(p->ce_count && (regval & CE_LOGREG_VALID)))
>                 goto ue_error;
>
>         /* CE assignments */
>
>         ...
>
>  ue_error:
>
>         if (!(p->ue_count && (regval & UE_LOGREG_VALID)))
>                 goto out;
>
>         /* UE assignments */
>
>         ...
>
>  out:
>
>         writel(...);
>         writel(...);
>
>
>> +             }
>> +             clearval |= SYNOPSYS_DDRC_ECCCTRL_CLR_UE_ERR;
>> +     }
>> +
>> +     writel(clearval, base + SYNOPSYS_DDRC_ECC_CONTROL_REG_OFFSET);
>> +     writel(0x0, base + SYNOPSYS_DDRC_ECC_CONTROL_REG_OFFSET);
>> +
>> +     return 1;
>> +}
>> +
>> +/**
>> + * synopsys_edac_generate_message - Generate interpreted ECC status message
>> + * @mci:     Pointer to the edac memory controller instance
>> + * @perrstatus:      Pointer to the synopsys 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 synopsys_edac_generate_message(const struct mem_ctl_info *mci,
>> +                     struct synopsys_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, SYNOPSYS_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);
>> +}
>
> This function can be part of synopsys_edac_handle_error() below instead
> as it is simple enough and called only once there - no need for a
> separate function.
>
>> +
>> +/**
>> + * synopsys_edac_handle_error - Handle controller error types CE and UE
>> + * @mci:     Pointer to the edac memory controller instance
>> + * @perrstatus:      Pointer to the synopsys ecc status structure
>> + *
>> + * This routine handles the controller ECC correctable error.
>
> It handles also UC errors.
>
>> + */
>> +static void synopsys_edac_handle_error(struct mem_ctl_info *mci,
>> +                     struct synopsys_ecc_status *perrstatus)
>> +{
>> +     char message[SYNOPSYS_EDAC_MESSAGE_SIZE];
>> +
>> +     synopsys_edac_generate_message(mci, perrstatus, &message[0],
>> +                                SYNOPSYS_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], "");
>
> string_array == &string_array[0]
>
>> +     else
>> +             edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
>> +                                  perrstatus->ue_count, 0, 0, 0, 0, 0, -1,
>> +                                  &message[0], "");
>
> Ditto.
>
>> +}
>> +
>> +/**
>> + * synopsys_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 synopsys_edac_check(struct mem_ctl_info *mci)
>> +{
>> +     struct synopsys_edac_priv *priv = mci->pvt_info;
>> +     struct synopsys_ecc_status errstatus;
>> +     int status;
>> +
>> +     status = synopsys_edac_geterror_info(priv->baseaddr, &errstatus);
>> +     if (status) {
>
> Save an indentation level:
>
>         if (!status)
>                 return;
>
>         priv->ce_count += ...
>         ...
>
>> +             priv->ce_count += errstatus.ce_count;
>> +             priv->ue_count += errstatus.ue_count;
>> +
>> +             if (errstatus.ce_count) {
>> +                     synopsys_edac_handle_error(mci, &errstatus);
>> +                     errstatus.ce_count = 0;
>> +             }
>> +             if (errstatus.ue_count) {
>> +                     synopsys_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);
>> +     }
>> +}
>> +
>> +/**
>> + * synopsys_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 synopsys_edac_get_dtype(void __iomem *base)
>> +{
>> +     enum dev_type dt;
>> +     u32 width;
>> +
>> +     width = readl(base + SYNOPSYS_DDRC_CONTROL_REG_OFFSET);
>> +     width = (width & SYNOPSYS_DDRC_CTRLREG_BUSWIDTH_MASK) >>
>> +                     SYNOPSYS_DDRC_CTRLREG_BUSWIDTH_SHIFT;
>> +
>> +     switch (width) {
>> +     case SYNOPSYS_DDRCTL_WDTH_16:
>> +             dt = DEV_X2;
>> +             break;
>> +     case SYNOPSYS_DDRCTL_WDTH_32:
>> +             dt = DEV_X4;
>> +             break;
>> +     default:
>> +             dt = DEV_UNKNOWN;
>> +     }
>> +
>> +     return dt;
>> +}
>> +
>> +/**
>> + * synopsys_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 controller
>> + *
>> + * Return: a ecc status boolean i.e true/false - enabled/disabled.
>> + */
>> +static bool synopsys_edac_get_eccstate(void __iomem *base)
>> +{
>> +     enum dev_type dt;
>> +     u32 ecctype;
>> +     bool state = false;
>> +
>> +     dt = synopsys_edac_get_dtype(base);
>
> Shouldn't you handle the error case where it returns DEV_UNKNOWN type?
>
>> +
>> +     ecctype = (readl(base + SYNOPSYS_DDRC_ECC_SCRUB_REG_OFFSET) &
>> +                     SYNOPSYS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK);
>> +
>> +     if ((ecctype == SYNOPSYS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED)
>> +                     && (dt == DEV_X2)) {
>> +             state = true;
>> +             writel(0x0, base + SYNOPSYS_DDRC_ECC_CONTROL_REG_OFFSET);
>> +     } else {
>> +             state = false;
>
> Why that else branch? state is already initialized to false at function
> entry time.
>
>> +     }
>> +
>> +     return state;
>> +}
>> +
>> +/**
>> + * synopsys_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
>> + */
>
> No need for that comment.
>
>> +static u32 synopsys_edac_get_memsize(void)
>> +{
>> +     struct sysinfo inf;
>> +
>> +     /* Reading the system memory size from the global meminfo structure */
>
> Same here.
>
> For simple functions where it is paramount what the function does, you
> don't need to comment every line.
>
>> +     si_meminfo(&inf);
>> +
>> +     return inf.totalram * inf.mem_unit;
>> +}
>> +
>> +/**
>> + * synopsys_edac_get_mtype - Returns controller memory type
>> + * @base:    pointer to the synopsys 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 synopsys_edac_get_mtype(void __iomem *base)
>> +{
>> +     enum mem_type mt;
>> +     u32 memtype;
>> +
>> +     memtype = readl(base + SYNOPSYS_DDRC_T_ZQ_REG_OFFSET);
>> +
>> +     if (memtype & SYNOPSYS_DDRC_T_ZQ_REG_DDRMODE_MASK)
>> +             mt = MEM_DDR3;
>> +     else
>> +             mt = MEM_DDR2;
>> +
>> +     return mt;
>> +}
>> +
>> +/**
>> + * synopsys_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
>
> Where is that -EINVAL being returned?
>
>> + * configuration cannot be determined.
>> + */
>> +static int synopsys_edac_init_csrows(struct mem_ctl_info *mci)
>> +{
>> +     struct csrow_info *csi;
>> +     struct dimm_info *dimm;
>> +     struct synopsys_edac_priv *priv = mci->pvt_info;
>> +     u32 size;
>> +     int row, j;
>> +
>> +     for (row = 0; row < mci->nr_csrows; row++) {
>> +             csi = mci->csrows[row];
>> +             size = synopsys_edac_get_memsize();
>> +
>> +             for (j = 0; j < csi->nr_channels; j++) {
>> +                     dimm = csi->channels[j]->dimm;
>> +                     dimm->edac_mode = EDAC_FLAG_SECDED;
>> +                     dimm->mtype = synopsys_edac_get_mtype(priv->baseaddr);
>> +                     dimm->nr_pages =
>> +                         (size >> PAGE_SHIFT) / csi->nr_channels;
>> +                     dimm->grain = SYNOPSYS_EDAC_ERROR_GRAIN;
>> +                     dimm->dtype = synopsys_edac_get_dtype(priv->baseaddr);
>> +             }
>> +
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * synopsys_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
>> + * memory controller the instance is bound to.
>> + *
>> + * Return: 0 if OK; otherwise, < 0 on error.
>> + */
>> +static int synopsys_edac_mc_init(struct mem_ctl_info *mci,
>> +                     struct platform_device *pdev)
>> +{
>> +     int status;
>> +     struct synopsys_edac_priv *priv;
>> +
>> +     /* Initial driver pointers and private data */
>
> Useless comment.
>
>> +     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 */
>
> Useless comment.
>
>> +     mci->scrub_mode = SCRUB_NONE;
>> +
>> +     mci->edac_cap = EDAC_FLAG_SECDED;
>> +     /* Initialize strings */
>
> Useless comment.
>
>> +     mci->ctl_name = "synopsys_ddr_controller";
>> +     mci->dev_name = dev_name(&pdev->dev);
>> +     mci->mod_name = "synopsys_edac";
>
> Do a
>
> #define EDAC_MOD_STR    "synopsys_edac"
>
> like the other drivers do and use that here and below.
>
>> +     mci->mod_ver = "1";
>> +
>> +     /* Initialize callbacks */
>
> Useless comment.
>
>> +     edac_op_state = EDAC_OPSTATE_POLL;
>> +     mci->edac_check = synopsys_edac_check;
>> +     mci->ctl_page_to_phys = NULL;
>> +
>> +     /*
>> +      * Initialize the MC control structure 'csrows' table
>> +      * with the mapping and control information.
>> +      */
>> +     status = synopsys_edac_init_csrows(mci);
>> +     if (status)
>> +             pr_err("Failed to initialize rows!\n");
>
> edac_printk
>
>> +
>> +     return status;
>> +}
>> +
>> +/**
>> + * synopsys_edac_mc_probe - Check controller and bind driver
>> + * @pdev:    Pointer to the platform_device struct
>> + *
>> + * This routine probes a specific 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 synopsys_edac_mc_probe(struct platform_device *pdev)
>> +{
>> +     struct mem_ctl_info *mci;
>> +     struct edac_mc_layer layers[2];
>> +     struct synopsys_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);
>
> You're not handling the case where this function returns NULL.

devm_ioremap_resource function will check this condition, so not
checking for the NULL explicitly
>
>> +     baseaddr = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(baseaddr))
>> +             return PTR_ERR(baseaddr);
>> +
>> +     /* Check for the ecc enable status */
>> +     if (synopsys_edac_get_eccstate(baseaddr) == false) {
>
>         if (!get_eccstate(...))
>
>> +             dev_err(&pdev->dev, "ecc not enabled\n");
>
> edac_printk pls.
>
>> +             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 = SYNOPSYS_EDAC_NR_CSROWS;
>> +     layers[0].is_virt_csrow = true;
>> +     layers[1].type = EDAC_MC_LAYER_CHANNEL;
>> +     layers[1].size = SYNOPSYS_EDAC_NR_CHANS;
>> +     layers[1].is_virt_csrow = false;
>> +
>> +     mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
>> +                         sizeof(struct synopsys_edac_priv));
>> +     if (mci == NULL) {
>
>         if (!mci)
>
>> +             pr_err("Failed memory allocation for mci instance!\n");
>
> edac_printk
>
> Before you return, devm_iounmap(... baseaddr). Just do it with nice goto
> labels.

Since the probe returns error, it it that the devm_ framework will clean these
resources automatically?
>
>> +             return -ENOMEM;
>> +     }
>> +
>> +     priv = mci->pvt_info;
>> +     priv->baseaddr = baseaddr;
>> +     rc = synopsys_edac_mc_init(mci, pdev);
>> +     if (rc) {
>> +             pr_err("Failed to initialize instance!\n");
>
> edac_printk
>
>> +             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");
>
> Ditto.
>
>> +             goto del_edac_mc;
>> +     }
>> +
>> +     return rc;
>> +
>> +del_edac_mc:
>> +     edac_mc_del_mc(&pdev->dev);
>> +free_edac_mc:
>> +     edac_mc_free(mci);
>> +
>> +     return rc;
>> +}
>> +
>> +/**
>> + * synopsys_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 controller described by the
>> + * OpenFirmware device tree node passed as a parameter.
>> + *
>> + * Return: Unconditionally 0
>> + */
>
> No need for that whole comment at all.
>
>> +static int synopsys_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 */
>
> Ditto.
>
>> +static struct of_device_id synopsys_edac_match[] = {
>> +     { .compatible = "xlnx,zynq-ddrc-1.04", },
>> +     { /* end of table */ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, synopsys_edac_match);
>> +
>> +static struct platform_driver synopsys_edac_mc_driver = {
>> +     .driver = {
>> +                .name = "synopsys-edac",
>> +                .owner = THIS_MODULE,
>> +                .of_match_table = synopsys_edac_match,
>> +                },
>> +     .probe = synopsys_edac_mc_probe,
>> +     .remove = synopsys_edac_mc_remove,
>> +};
>> +
>> +module_platform_driver(synopsys_edac_mc_driver);
>> +
>> +MODULE_AUTHOR("Xilinx, Inc.");
>> +MODULE_DESCRIPTION("Synopsys DDR ECC driver");
>> +MODULE_LICENSE("GPL v2");
>
> Ok, that should be it so far, more review with the next submission.

Thanks for the detailed review and i will implement all other comments and will
send the next version once we conclude the resolutions for my above concerns
and comments.

Regrads,
Punnaiah
>
> --
> Regards/Gruss,
>     Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/