This patchset is a first one in the series created in the framework of
my Baikal-T1 DDRC-related work:
[1: In-progress] EDAC/mc/synopsys: Various fixes and cleanups
Link: ---you are looking at it---
[2: To be submitted] EDAC/synopsys: Add generic DDRC info and address mapping
Link:
[3: To be submitted] EDAC/synopsys: Add generic resources and Baikal-T1 support
Link:
Note the patchsets above must be merged in the same order as they are
placed in the list in order to prevent conflicts. Nothing prevents them
from being reviewed synchronously though. Any tests are very welcome.
Thanks in advance.
Regarding this series content. It's an initial patchset which
traditionally provides various fixes, cleanups and modifications required
for the more comfortable further features development. The main goal of it
though is to detach the Xilinx Zynq A05 DDRC related code into the
dedicated driver since first it has nothing to do with the Synopsys DW
uMCTL2 DDR controller and second it will be a great deal obstacle on the
way of extending the Synopsys-part functionality.
The series starts with fixes patches, which in short concern the next
aspects: touching the ZynqMP-specific CSRs on the Xilinx ZinqMP platform
only, serializing an access to the ECCCLR register, adding correct memory
devices type detection, setting a correct value to the
mem_ctl_info.scrub_cap field, dropping an erroneous ADDRMAP[4] parsing and
getting back a correct order of the ECC errors info detection procedure.
Afterwards the patchset provides several cleanup patches required for the
more coherent code splitting up (Xilinx Zynq A05 and Synopsys DW uMCTL2)
so the provided modifications would be useful in both drivers. First we
get to replace the platform resource manual IO-remapping with the
devm_platform_ioremap_resource() method call. Secondly we suggest to drop:
internal CE/UE errors counters, local to_mci() macros definition, some
redundant ecc_error_info structure fields and redundant info from the
error message, duplicated dimm->nr_pages debug printout and spaces from
the MEM_TYPE flags declarations. (The later two updates concern the MCI
core part.) Thirdly before splitting up the driver we need to add an
unique MC index allocation infrastructure to the MCI core. It's required
since after splitting the driver up we'll need to make sure both device
types could be correctly probed on the same platform. Finally the Xilinx
Zynq A05 part of the driver is moved out to a dedicated driver where it
should been originally placed. After that the platform-specific setups API
is removed from the Synopsys DW uMCTL2 DDRC driver since it's no longer
required.
Finally as the cherry on the cake we suggest to unify the DW uMCTL2 DDRC
driver entities naming and replace the open-coded "shift/mask" patter with
the kernel helpers like BIT/GENMASK/FIELD_x in there. It shall
significantly improve the code readability.
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Michail Ivanov <[email protected]>
Cc: Pavel Parkhomenko <[email protected]>
Cc: Punnaiah Choudary Kalluri <[email protected]>
Cc: Manish Narani <[email protected]>
Cc: Dinh Nguyen <[email protected]>
Cc: James Morse <[email protected]>
Cc: Robert Richter <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Serge Semin (20):
EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure
EDAC/synopsys: Fix generic device type detection procedure
EDAC/synopsys: Fix mci->scrub_cap field setting
EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse
EDAC/synopsys: Fix reading errors count before ECC status
EDAC/synopsys: Use platform device devm ioremap method
EDAC/synopsys: Drop internal CE and UE counters
EDAC/synopsys: Drop local to_mci macro implementation
EDAC/synopsys: Drop struct ecc_error_info.blknr field
EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name
EDAC/synopsys: Drop redundant info from error message
EDAC/mc: Replace spaces with tabs in memtype flags definition
EDAC/mc: Drop duplicated dimm->nr_pages debug printout
EDAC/mc: Init DIMM labels in MC registration method
EDAC/mc: Add MC unique index allocation procedure
dt-bindings: memory: snps: Detach Zynq DDRC controller support
EDAC/synopsys: Detach Zynq DDRC controller support
EDAC/synopsys: Drop unused platform-specific setup API
EDAC/synopsys: Unify the driver entities naming
EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros
.../snps,dw-umctl2-ddrc.yaml | 51 +
.../memory-controllers/synopsys,ddrc-ecc.yaml | 76 --
.../xlnx,zynq-ddrc-a05.yaml | 38 +
MAINTAINERS | 3 +
drivers/edac/Kconfig | 9 +-
drivers/edac/Makefile | 1 +
drivers/edac/edac_mc.c | 136 ++-
drivers/edac/edac_mc.h | 4 +
drivers/edac/synopsys_edac.c | 902 ++++++------------
drivers/edac/zynq_edac.c | 504 ++++++++++
include/linux/edac.h | 30 +-
11 files changed, 1037 insertions(+), 717 deletions(-)
create mode 100644 Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
delete mode 100644 Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
create mode 100644 Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml
create mode 100644 drivers/edac/zynq_edac.c
--
2.35.1
None of the ecc_error_info structure fields has "nr" suffix even though
each of them do re-present some number (row number, column number, bank
number). Let's drop the suffix from the bankgrpnr field name for the sake
of unification. While at it drop the word "Number" from the CE/UE error
messages too since it doesn't give any helpful info there.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/edac/synopsys_edac.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 843d2717c72b..63b3d11e3904 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -272,17 +272,17 @@
* @row: Row number.
* @col: Column number.
* @bank: Bank number.
+ * @bankgrp: Bank group number.
* @bitpos: Bit position.
* @data: Data causing the error.
- * @bankgrpnr: Bank group number.
*/
struct ecc_error_info {
u32 row;
u32 col;
u32 bank;
+ u32 bankgrp;
u32 bitpos;
u32 data;
- u32 bankgrpnr;
};
/**
@@ -433,7 +433,7 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
regval = readl(base + ECC_CEADDR1_OFST);
p->ceinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
ECC_CEADDR1_BNKNR_SHIFT;
- p->ceinfo.bankgrpnr = (regval & ECC_CEADDR1_BNKGRP_MASK) >>
+ p->ceinfo.bankgrp = (regval & ECC_CEADDR1_BNKGRP_MASK) >>
ECC_CEADDR1_BNKGRP_SHIFT;
p->ceinfo.col = (regval & ECC_CEADDR1_COL_MASK);
p->ceinfo.data = readl(base + ECC_CSYND0_OFST);
@@ -447,7 +447,7 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
regval = readl(base + ECC_UEADDR0_OFST);
p->ueinfo.row = (regval & ECC_CEADDR0_RW_MASK);
regval = readl(base + ECC_UEADDR1_OFST);
- p->ueinfo.bankgrpnr = (regval & ECC_CEADDR1_BNKGRP_MASK) >>
+ p->ueinfo.bankgrp = (regval & ECC_CEADDR1_BNKGRP_MASK) >>
ECC_CEADDR1_BNKGRP_SHIFT;
p->ueinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
ECC_CEADDR1_BNKNR_SHIFT;
@@ -483,9 +483,9 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
pinf = &p->ceinfo;
if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
- "DDR ECC error type:%s Row %d Col %d Bank %d BankGroup Number %d Bit Position: %d Data: 0x%08x",
+ "DDR ECC error type:%s Row %d Col %d Bank %d Bank Group %d Bit Position: %d Data: 0x%08x",
"CE", pinf->row, pinf->col, pinf->bank,
- pinf->bankgrpnr, pinf->bitpos, pinf->data);
+ pinf->bankgrp, pinf->bitpos, pinf->data);
} else {
snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
"DDR ECC error type:%s Row %d Bank %d Col %d Bit Position: %d Data: 0x%08x",
@@ -502,9 +502,9 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
pinf = &p->ueinfo;
if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
- "DDR ECC error type :%s Row %d Col %d Bank %d BankGroup Number %d",
+ "DDR ECC error type :%s Row %d Col %d Bank %d Bank Group %d",
"UE", pinf->row, pinf->col, pinf->bank,
- pinf->bankgrpnr);
+ pinf->bankgrp);
} else {
snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
"DDR ECC error type :%s Row %d Bank %d Col %d ",
--
2.35.1
Currently the driver naming schema is kind of random. There are structures
and methods with the "synps" prefix, there are structures and methods
with no driver-specific prefix, there are methods with the "edac" prefix,
there are structure instances with "zynqmp" and "synopsys" prefixes, there
are macros with "SYNPS", "ECC" and "DDR" prefixes. Moreover some time ago
some of function names were shortened out by completely removing the
vendor-specific prefixes thus leaving the driver with no strict entities
naming convention (see commit bb894bc46ed0 ("EDAC, synopsys: Shorten
static function names")).
All of that makes the code much harder to read for no much reason (except
shorter names utilization) since there is no easy way to distinguish now
the local, EDAC core and global name spaces right from the code context.
Similarly the kernel code index services (like elixir) gets to find the
different functions with the same name, which harden the kernel hacking.
Let's fix all of that by unifying the driver local entity names like
functions, structures and non-CSR-related macros especially seeing the
same approach has been used in the most of the EDAC LLDD. We suggest to
use the "snps" prefix here as the shortest version of the controller
vendor name. While at it add a more detailed controller name (DW uMCTL2
DDRC) to the driver comments and string literals where it's appropriate.
Signed-off-by: Serge Semin <[email protected]>
---
Note "dw" prefix would be even shorter alternative. But we decided to
stick with "snps" since "synopsys" has already been used in the module
name.
---
drivers/edac/synopsys_edac.c | 238 +++++++++++++++++------------------
1 file changed, 119 insertions(+), 119 deletions(-)
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index e171c8d96274..9ac0c8c4e3b8 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Synopsys DDR ECC Driver
+ * Synopsys DW uMCTL2 DDR ECC Driver
* This driver is based on ppc4xx_edac.c drivers
*
* Copyright (C) 2012 - 2014 Xilinx, Inc.
@@ -17,23 +17,23 @@
#include "edac_module.h"
/* Number of cs_rows needed per memory controller */
-#define SYNPS_EDAC_NR_CSROWS 1
+#define SNPS_EDAC_NR_CSROWS 1
/* Number of channels per memory controller */
-#define SYNPS_EDAC_NR_CHANS 1
+#define SNPS_EDAC_NR_CHANS 1
/* Granularity of reported error in bytes */
-#define SYNPS_EDAC_ERR_GRAIN 1
+#define SNPS_EDAC_ERR_GRAIN 1
-#define SYNPS_EDAC_MSG_SIZE 256
+#define SNPS_EDAC_MSG_SIZE 256
-#define SYNPS_EDAC_MOD_STRING "synps_edac"
-#define SYNPS_EDAC_MOD_VER "1"
+#define SNPS_EDAC_MOD_STRING "snps_edac"
+#define SNPS_EDAC_MOD_VER "1"
/* DDR ECC Quirks */
-#define SYNPS_ZYNQMP_IRQ_REGS BIT(0)
+#define SNPS_ZYNQMP_IRQ_REGS BIT(0)
-/* Synopsys DDR memory controller registers that are relevant to ECC */
+/* Synopsys uMCTL2 DDR controller registers that are relevant to ECC */
/* DDRC master0 Register */
#define DDR_MSTR_OFST 0x0
@@ -214,7 +214,7 @@
#define RANK_B0_BASE 6
/**
- * struct ecc_error_info - ECC error log information.
+ * struct snps_ecc_error_info - ECC error log information.
* @row: Row number.
* @col: Column number.
* @bank: Bank number.
@@ -222,7 +222,7 @@
* @bitpos: Bit position.
* @data: Data causing the error.
*/
-struct ecc_error_info {
+struct snps_ecc_error_info {
u32 row;
u32 col;
u32 bank;
@@ -232,21 +232,21 @@ struct ecc_error_info {
};
/**
- * struct synps_ecc_status - ECC status information to report.
+ * struct snps_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 synps_ecc_status {
+struct snps_ecc_status {
u32 ce_cnt;
u32 ue_cnt;
- struct ecc_error_info ceinfo;
- struct ecc_error_info ueinfo;
+ struct snps_ecc_error_info ceinfo;
+ struct snps_ecc_error_info ueinfo;
};
/**
- * struct synps_edac_priv - DDR memory controller private instance data.
+ * struct snps_edac_priv - DDR memory controller private data.
* @baseaddr: Base address of the DDR controller.
* @lock: Concurrent CSRs access lock.
* @message: Buffer for framing the event specific info.
@@ -259,12 +259,12 @@ struct synps_ecc_status {
* @bankgrp_shift: Bit shifts for bank group bit.
* @rank_shift: Bit shifts for rank bit.
*/
-struct synps_edac_priv {
+struct snps_edac_priv {
void __iomem *baseaddr;
spinlock_t lock;
- char message[SYNPS_EDAC_MSG_SIZE];
- struct synps_ecc_status stat;
- const struct synps_platform_data *p_data;
+ char message[SNPS_EDAC_MSG_SIZE];
+ struct snps_ecc_status stat;
+ const struct snps_platform_data *p_data;
#ifdef CONFIG_EDAC_DEBUG
ulong poison_addr;
u32 row_shift[18];
@@ -276,22 +276,22 @@ struct synps_edac_priv {
};
/**
- * struct synps_platform_data - Synopsys uMCTL2 DDRC platform data.
+ * struct snps_platform_data - Synopsys uMCTL2 DDRC platform data.
* @quirks: IP-core specific quirks.
*/
-struct synps_platform_data {
+struct snps_platform_data {
u32 quirks;
};
/**
- * synps_get_error_info - Get the current ECC error info.
+ * snps_get_error_info - Get the current ECC error info.
* @priv: DDR memory controller private instance data.
*
* Return: one if there is no error otherwise returns zero.
*/
-static int synps_get_error_info(struct synps_edac_priv *priv)
+static int snps_get_error_info(struct snps_edac_priv *priv)
{
- struct synps_ecc_status *p;
+ struct snps_ecc_status *p;
u32 regval, clearval;
unsigned long flags;
void __iomem *base;
@@ -351,21 +351,21 @@ static int synps_get_error_info(struct synps_edac_priv *priv)
}
/**
- * handle_error - Handle Correctable and Uncorrectable errors.
+ * snps_handle_error - Handle Correctable and Uncorrectable errors.
* @mci: EDAC memory controller instance.
* @p: Synopsys ECC status structure.
*
* Handles ECC correctable and uncorrectable errors.
*/
-static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
+static void snps_handle_error(struct mem_ctl_info *mci, struct snps_ecc_status *p)
{
- struct synps_edac_priv *priv = mci->pvt_info;
- struct ecc_error_info *pinf;
+ struct snps_edac_priv *priv = mci->pvt_info;
+ struct snps_ecc_error_info *pinf;
if (p->ce_cnt) {
pinf = &p->ceinfo;
- snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
+ snprintf(priv->message, SNPS_EDAC_MSG_SIZE,
"Row %d Col %d Bank %d Bank Group %d Bit %d Data 0x%08x",
pinf->row, pinf->col, pinf->bank, pinf->bankgrp,
pinf->bitpos, pinf->data);
@@ -378,7 +378,7 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
if (p->ue_cnt) {
pinf = &p->ueinfo;
- snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
+ snprintf(priv->message, SNPS_EDAC_MSG_SIZE,
"Row %d Col %d Bank %d Bank Group %d",
pinf->row, pinf->col, pinf->bank, pinf->bankgrp);
@@ -390,7 +390,7 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
memset(p, 0, sizeof(*p));
}
-static void enable_intr(struct synps_edac_priv *priv)
+static void snps_enable_irq(struct snps_edac_priv *priv)
{
unsigned long flags;
@@ -411,7 +411,7 @@ static void enable_intr(struct synps_edac_priv *priv)
spin_unlock_irqrestore(&priv->lock, flags);
}
-static void disable_intr(struct synps_edac_priv *priv)
+static void snps_disable_irq(struct snps_edac_priv *priv)
{
unsigned long flags;
@@ -431,42 +431,42 @@ static void disable_intr(struct synps_edac_priv *priv)
}
/**
- * intr_handler - Interrupt Handler for ECC interrupts.
+ * snps_irq_handler - Interrupt Handler for ECC interrupts.
* @irq: IRQ number.
* @dev_id: Device ID.
*
* Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise.
*/
-static irqreturn_t intr_handler(int irq, void *dev_id)
+static irqreturn_t snps_irq_handler(int irq, void *dev_id)
{
struct mem_ctl_info *mci = dev_id;
- struct synps_edac_priv *priv;
+ struct snps_edac_priv *priv;
int status, regval;
priv = mci->pvt_info;
- if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS) {
+ if (priv->p_data->quirks & SNPS_ZYNQMP_IRQ_REGS) {
regval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
regval &= (DDR_QOSCE_MASK | DDR_QOSUE_MASK);
if (!(regval & ECC_CE_UE_INTR_MASK))
return IRQ_NONE;
}
- status = synps_get_error_info(priv);
+ status = snps_get_error_info(priv);
if (status)
return IRQ_NONE;
- handle_error(mci, &priv->stat);
+ snps_handle_error(mci, &priv->stat);
- if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS)
+ if (priv->p_data->quirks & SNPS_ZYNQMP_IRQ_REGS)
writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
return IRQ_HANDLED;
}
/**
- * synps_get_dtype - Return the controller memory width.
+ * snps_get_dtype - Return the controller memory width.
* @base: DDR memory controller base address.
*
* Get the EDAC device type width appropriate for the current controller
@@ -474,7 +474,7 @@ static irqreturn_t intr_handler(int irq, void *dev_id)
*
* Return: a device type width enumeration.
*/
-static enum dev_type synps_get_dtype(const void __iomem *base)
+static enum dev_type snps_get_dtype(const void __iomem *base)
{
u32 regval;
@@ -498,14 +498,14 @@ static enum dev_type synps_get_dtype(const void __iomem *base)
}
/**
- * synps_get_ecc_state - Return the controller ECC enable/disable status.
+ * snps_get_ecc_state - Return the controller ECC enable/disable status.
* @base: DDR memory controller base address.
*
* Get the ECC enable/disable status for the controller.
*
* Return: a ECC status boolean i.e true/false - enabled/disabled.
*/
-static bool synps_get_ecc_state(void __iomem *base)
+static bool snps_get_ecc_state(void __iomem *base)
{
u32 regval;
@@ -515,11 +515,11 @@ static bool synps_get_ecc_state(void __iomem *base)
}
/**
- * get_memsize - Read the size of the attached memory device.
+ * snps_get_memsize - Read the size of the attached memory device.
*
* Return: the memory size in bytes.
*/
-static u32 get_memsize(void)
+static u32 snps_get_memsize(void)
{
struct sysinfo inf;
@@ -529,7 +529,7 @@ static u32 get_memsize(void)
}
/**
- * synps_get_mtype - Returns controller memory type.
+ * snps_get_mtype - Returns controller memory type.
* @base: Synopsys ECC status structure.
*
* Get the EDAC memory type appropriate for the current controller
@@ -537,7 +537,7 @@ static u32 get_memsize(void)
*
* Return: a memory type enumeration.
*/
-static enum mem_type synps_get_mtype(const void __iomem *base)
+static enum mem_type snps_get_mtype(const void __iomem *base)
{
enum mem_type mt;
u32 memtype;
@@ -557,15 +557,15 @@ static enum mem_type synps_get_mtype(const void __iomem *base)
}
/**
- * init_csrows - Initialize the csrow data.
+ * snps_init_csrows - Initialize the csrow data.
* @mci: EDAC memory controller instance.
*
* Initialize the chip select rows associated with the EDAC memory
* controller instance.
*/
-static void init_csrows(struct mem_ctl_info *mci)
+static void snps_init_csrows(struct mem_ctl_info *mci)
{
- struct synps_edac_priv *priv = mci->pvt_info;
+ struct snps_edac_priv *priv = mci->pvt_info;
struct csrow_info *csi;
struct dimm_info *dimm;
u32 size, row;
@@ -573,21 +573,21 @@ static void init_csrows(struct mem_ctl_info *mci)
for (row = 0; row < mci->nr_csrows; row++) {
csi = mci->csrows[row];
- size = get_memsize();
+ size = snps_get_memsize();
for (j = 0; j < csi->nr_channels; j++) {
dimm = csi->channels[j]->dimm;
dimm->edac_mode = EDAC_SECDED;
- dimm->mtype = synps_get_mtype(priv->baseaddr);
+ dimm->mtype = snps_get_mtype(priv->baseaddr);
dimm->nr_pages = (size >> PAGE_SHIFT) / csi->nr_channels;
- dimm->grain = SYNPS_EDAC_ERR_GRAIN;
- dimm->dtype = synps_get_dtype(priv->baseaddr);
+ dimm->grain = SNPS_EDAC_ERR_GRAIN;
+ dimm->dtype = snps_get_dtype(priv->baseaddr);
}
}
}
/**
- * mc_init - Initialize one driver instance.
+ * snps_mc_init - Initialize one driver instance.
* @mci: EDAC memory controller instance.
* @pdev: platform device.
*
@@ -595,9 +595,9 @@ static void init_csrows(struct mem_ctl_info *mci)
* related driver-private data associated with the memory controller the
* instance is bound to.
*/
-static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
+static void snps_mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
{
- struct synps_edac_priv *priv;
+ struct snps_edac_priv *priv;
mci->pdev = &pdev->dev;
priv = mci->pvt_info;
@@ -610,21 +610,22 @@ static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
mci->scrub_mode = SCRUB_NONE;
mci->edac_cap = EDAC_FLAG_SECDED;
- mci->ctl_name = "synps_ddr_controller";
- mci->dev_name = SYNPS_EDAC_MOD_STRING;
- mci->mod_name = SYNPS_EDAC_MOD_VER;
+ mci->ctl_name = "snps_umctl2_ddrc";
+ mci->dev_name = SNPS_EDAC_MOD_STRING;
+ mci->mod_name = SNPS_EDAC_MOD_VER;
edac_op_state = EDAC_OPSTATE_INT;
mci->ctl_page_to_phys = NULL;
- init_csrows(mci);
+ snps_init_csrows(mci);
}
-static int setup_irq(struct mem_ctl_info *mci,
- struct platform_device *pdev)
+
+
+static int snps_setup_irq(struct mem_ctl_info *mci, struct platform_device *pdev)
{
- struct synps_edac_priv *priv = mci->pvt_info;
+ struct snps_edac_priv *priv = mci->pvt_info;
int ret, irq;
irq = platform_get_irq(pdev, 0);
@@ -634,14 +635,14 @@ static int setup_irq(struct mem_ctl_info *mci,
return irq;
}
- ret = devm_request_irq(&pdev->dev, irq, intr_handler,
+ ret = devm_request_irq(&pdev->dev, irq, snps_irq_handler,
0, dev_name(&pdev->dev), mci);
if (ret < 0) {
edac_printk(KERN_ERR, EDAC_MC, "Failed to request IRQ\n");
return ret;
}
- enable_intr(priv);
+ snps_enable_irq(priv);
return 0;
}
@@ -649,13 +650,13 @@ static int setup_irq(struct mem_ctl_info *mci,
#ifdef CONFIG_EDAC_DEBUG
/**
- * ddr_poison_setup - Update poison registers.
+ * snps_data_poison_setup - Update poison registers.
* @priv: DDR memory controller private instance data.
*
* Update poison registers as per DDR mapping.
* Return: none.
*/
-static void ddr_poison_setup(struct synps_edac_priv *priv)
+static void snps_data_poison_setup(struct snps_edac_priv *priv)
{
int col = 0, row = 0, bank = 0, bankgrp = 0, rank = 0, regval;
int index;
@@ -713,7 +714,7 @@ static ssize_t inject_data_error_show(struct device *dev,
char *data)
{
struct mem_ctl_info *mci = to_mci(dev);
- struct synps_edac_priv *priv = mci->pvt_info;
+ struct snps_edac_priv *priv = mci->pvt_info;
return sprintf(data, "Poison0 Addr: 0x%08x\n\rPoison1 Addr: 0x%08x\n\r"
"Error injection Address: 0x%lx\n\r",
@@ -727,12 +728,12 @@ static ssize_t inject_data_error_store(struct device *dev,
const char *data, size_t count)
{
struct mem_ctl_info *mci = to_mci(dev);
- struct synps_edac_priv *priv = mci->pvt_info;
+ struct snps_edac_priv *priv = mci->pvt_info;
if (kstrtoul(data, 0, &priv->poison_addr))
return -EINVAL;
- ddr_poison_setup(priv);
+ snps_data_poison_setup(priv);
return count;
}
@@ -742,7 +743,7 @@ static ssize_t inject_data_poison_show(struct device *dev,
char *data)
{
struct mem_ctl_info *mci = to_mci(dev);
- struct synps_edac_priv *priv = mci->pvt_info;
+ struct snps_edac_priv *priv = mci->pvt_info;
return sprintf(data, "Data Poisoning: %s\n\r",
(((readl(priv->baseaddr + ECC_CFG1_OFST)) & 0x3) == 0x3)
@@ -754,7 +755,7 @@ static ssize_t inject_data_poison_store(struct device *dev,
const char *data, size_t count)
{
struct mem_ctl_info *mci = to_mci(dev);
- struct synps_edac_priv *priv = mci->pvt_info;
+ struct snps_edac_priv *priv = mci->pvt_info;
writel(0, priv->baseaddr + DDR_SWCTL);
if (strncmp(data, "CE", 2) == 0)
@@ -769,7 +770,7 @@ static ssize_t inject_data_poison_store(struct device *dev,
static DEVICE_ATTR_RW(inject_data_error);
static DEVICE_ATTR_RW(inject_data_poison);
-static int edac_create_sysfs_attributes(struct mem_ctl_info *mci)
+static int snps_create_sysfs_attributes(struct mem_ctl_info *mci)
{
int rc;
@@ -782,13 +783,13 @@ static int edac_create_sysfs_attributes(struct mem_ctl_info *mci)
return 0;
}
-static void edac_remove_sysfs_attributes(struct mem_ctl_info *mci)
+static void snps_remove_sysfs_attributes(struct mem_ctl_info *mci)
{
device_remove_file(&mci->dev, &dev_attr_inject_data_error);
device_remove_file(&mci->dev, &dev_attr_inject_data_poison);
}
-static void setup_row_address_map(struct synps_edac_priv *priv, u32 *addrmap)
+static void snps_setup_row_address_map(struct snps_edac_priv *priv, u32 *addrmap)
{
u32 addrmap_row_b2_10;
int index;
@@ -847,7 +848,7 @@ static void setup_row_address_map(struct synps_edac_priv *priv, u32 *addrmap)
ROW_MAX_VAL_MASK) + ROW_B17_BASE);
}
-static void setup_column_address_map(struct synps_edac_priv *priv, u32 *addrmap)
+static void snps_setup_column_address_map(struct snps_edac_priv *priv, u32 *addrmap)
{
u32 width, memtype;
int index;
@@ -949,7 +950,7 @@ static void setup_column_address_map(struct synps_edac_priv *priv, u32 *addrmap)
}
-static void setup_bank_address_map(struct synps_edac_priv *priv, u32 *addrmap)
+static void snps_setup_bank_address_map(struct snps_edac_priv *priv, u32 *addrmap)
{
priv->bank_shift[0] = (addrmap[1] & BANK_MAX_VAL_MASK) + BANK_B0_BASE;
priv->bank_shift[1] = ((addrmap[1] >> 8) &
@@ -961,7 +962,7 @@ static void setup_bank_address_map(struct synps_edac_priv *priv, u32 *addrmap)
}
-static void setup_bg_address_map(struct synps_edac_priv *priv, u32 *addrmap)
+static void snps_setup_bg_address_map(struct snps_edac_priv *priv, u32 *addrmap)
{
priv->bankgrp_shift[0] = (addrmap[8] &
BANKGRP_MAX_VAL_MASK) + BANKGRP_B0_BASE;
@@ -971,7 +972,7 @@ static void setup_bg_address_map(struct synps_edac_priv *priv, u32 *addrmap)
}
-static void setup_rank_address_map(struct synps_edac_priv *priv, u32 *addrmap)
+static void snps_setup_rank_address_map(struct snps_edac_priv *priv, u32 *addrmap)
{
priv->rank_shift[0] = ((addrmap[0] & RANK_MAX_VAL_MASK) ==
RANK_MAX_VAL_MASK) ? 0 : ((addrmap[0] &
@@ -979,14 +980,14 @@ static void setup_rank_address_map(struct synps_edac_priv *priv, u32 *addrmap)
}
/**
- * setup_address_map - Set Address Map by querying ADDRMAP registers.
+ * snps_setup_address_map - Set Address Map by querying ADDRMAP registers.
* @priv: DDR memory controller private instance data.
*
* Set Address Map by querying ADDRMAP registers.
*
* Return: none.
*/
-static void setup_address_map(struct synps_edac_priv *priv)
+static void snps_setup_address_map(struct snps_edac_priv *priv)
{
u32 addrmap[12];
int index;
@@ -998,20 +999,20 @@ static void setup_address_map(struct synps_edac_priv *priv)
addrmap[index] = readl(priv->baseaddr + addrmap_offset);
}
- setup_row_address_map(priv, addrmap);
+ snps_setup_row_address_map(priv, addrmap);
- setup_column_address_map(priv, addrmap);
+ snps_setup_column_address_map(priv, addrmap);
- setup_bank_address_map(priv, addrmap);
+ snps_setup_bank_address_map(priv, addrmap);
- setup_bg_address_map(priv, addrmap);
+ snps_setup_bg_address_map(priv, addrmap);
- setup_rank_address_map(priv, addrmap);
+ snps_setup_rank_address_map(priv, addrmap);
}
#endif /* CONFIG_EDAC_DEBUG */
/**
- * mc_probe - Check controller and bind driver.
+ * snps_mc_probe - Check controller and bind driver.
* @pdev: platform device.
*
* Probe a specific controller instance for binding with the driver.
@@ -1019,11 +1020,11 @@ static void setup_address_map(struct synps_edac_priv *priv)
* Return: 0 if the controller instance was successfully bound to the
* driver; otherwise, < 0 on error.
*/
-static int mc_probe(struct platform_device *pdev)
+static int snps_mc_probe(struct platform_device *pdev)
{
- const struct synps_platform_data *p_data;
+ const struct snps_platform_data *p_data;
struct edac_mc_layer layers[2];
- struct synps_edac_priv *priv;
+ struct snps_edac_priv *priv;
struct mem_ctl_info *mci;
void __iomem *baseaddr;
int rc;
@@ -1036,20 +1037,20 @@ static int mc_probe(struct platform_device *pdev)
if (!p_data)
return -ENODEV;
- if (!synps_get_ecc_state(baseaddr)) {
+ if (!snps_get_ecc_state(baseaddr)) {
edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
return -ENXIO;
}
layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
- layers[0].size = SYNPS_EDAC_NR_CSROWS;
+ layers[0].size = SNPS_EDAC_NR_CSROWS;
layers[0].is_virt_csrow = true;
layers[1].type = EDAC_MC_LAYER_CHANNEL;
- layers[1].size = SYNPS_EDAC_NR_CHANS;
+ layers[1].size = SNPS_EDAC_NR_CHANS;
layers[1].is_virt_csrow = false;
mci = edac_mc_alloc(EDAC_AUTO_MC_NUM, ARRAY_SIZE(layers), layers,
- sizeof(struct synps_edac_priv));
+ sizeof(struct snps_edac_priv));
if (!mci) {
edac_printk(KERN_ERR, EDAC_MC,
"Failed memory allocation for mc instance\n");
@@ -1061,9 +1062,9 @@ static int mc_probe(struct platform_device *pdev)
priv->p_data = p_data;
spin_lock_init(&priv->lock);
- mc_init(mci, pdev);
+ snps_mc_init(mci, pdev);
- rc = setup_irq(mci, pdev);
+ rc = snps_setup_irq(mci, pdev);
if (rc)
goto free_edac_mc;
@@ -1075,13 +1076,13 @@ static int mc_probe(struct platform_device *pdev)
}
#ifdef CONFIG_EDAC_DEBUG
- rc = edac_create_sysfs_attributes(mci);
+ rc = snps_create_sysfs_attributes(mci);
if (rc) {
edac_printk(KERN_ERR, EDAC_MC, "Failed to create sysfs entries\n");
goto free_edac_mc;
}
- setup_address_map(priv);
+ snps_setup_address_map(priv);
#endif
return rc;
@@ -1093,20 +1094,20 @@ static int mc_probe(struct platform_device *pdev)
}
/**
- * mc_remove - Unbind driver from controller.
+ * snps_mc_remove - Unbind driver from device.
* @pdev: Platform device.
*
* Return: Unconditionally 0
*/
-static int mc_remove(struct platform_device *pdev)
+static int snps_mc_remove(struct platform_device *pdev)
{
struct mem_ctl_info *mci = platform_get_drvdata(pdev);
- struct synps_edac_priv *priv = mci->pvt_info;
+ struct snps_edac_priv *priv = mci->pvt_info;
- disable_intr(priv);
+ snps_disable_irq(priv);
#ifdef CONFIG_EDAC_DEBUG
- edac_remove_sysfs_attributes(mci);
+ snps_remove_sysfs_attributes(mci);
#endif
edac_mc_del_mc(&pdev->dev);
@@ -1115,32 +1116,31 @@ static int mc_remove(struct platform_device *pdev)
return 0;
}
-static const struct synps_platform_data zynqmp_edac_def = {
- .quirks = SYNPS_ZYNQMP_IRQ_REGS,
+static const struct snps_platform_data zynqmp_edac_def = {
+ .quirks = SNPS_ZYNQMP_IRQ_REGS,
};
-static const struct synps_platform_data synopsys_edac_def = {
+static const struct snps_platform_data snps_edac_def = {
.quirks = 0,
};
-static const struct of_device_id synps_edac_match[] = {
+static const struct of_device_id snps_edac_match[] = {
{ .compatible = "xlnx,zynqmp-ddrc-2.40a", .data = &zynqmp_edac_def },
- { .compatible = "snps,ddrc-3.80a", .data = &synopsys_edac_def },
+ { .compatible = "snps,ddrc-3.80a", .data = &snps_edac_def },
{ }
};
-MODULE_DEVICE_TABLE(of, synps_edac_match);
+MODULE_DEVICE_TABLE(of, snps_edac_match);
-static struct platform_driver synps_edac_mc_driver = {
+static struct platform_driver snps_edac_mc_driver = {
.driver = {
- .name = "synopsys-edac",
- .of_match_table = synps_edac_match,
+ .name = "snps-edac",
+ .of_match_table = snps_edac_match,
},
- .probe = mc_probe,
- .remove = mc_remove,
+ .probe = snps_mc_probe,
+ .remove = snps_mc_remove,
};
-
-module_platform_driver(synps_edac_mc_driver);
+module_platform_driver(snps_edac_mc_driver);
MODULE_AUTHOR("Xilinx Inc");
-MODULE_DESCRIPTION("Synopsys DDR ECC driver");
+MODULE_DESCRIPTION("Synopsys uMCTL2 DDR ECC driver");
MODULE_LICENSE("GPL v2");
--
2.35.1
The driver now works with the Synopys DW uMCTL2 DDR IP-core only (Xilinx
Zynq A05 DDRc support has been moved to the dedicated driver). Pretty much
all the currently available IP-core revisions have got the same ECC and
main DDR-config CSRs map. Thus there is no point in supporting the no
longer used platform-specific API like the callbacks responsible for
getting the ECC errors info, memory and device types, ECC state. All of
that data can be retrieved in the same way on all the Synopys DW uMCTL2
DDR controller versions. Similarly there is no longer need in the
DDR_ECC_INTR_SUPPORT and DDR_ECC_DATA_POISON_SUPPORT quirk flags since DW
uMCTL2 always supports IRQs and data poisoning. Let's drop that
infrastructure for good then.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/edac/synopsys_edac.c | 194 +++++++++--------------------------
1 file changed, 51 insertions(+), 143 deletions(-)
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 0cb84f183aed..e171c8d96274 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -31,9 +31,7 @@
#define SYNPS_EDAC_MOD_VER "1"
/* DDR ECC Quirks */
-#define DDR_ECC_INTR_SUPPORT BIT(0)
-#define DDR_ECC_DATA_POISON_SUPPORT BIT(1)
-#define SYNPS_ZYNQMP_IRQ_REGS BIT(2)
+#define SYNPS_ZYNQMP_IRQ_REGS BIT(0)
/* Synopsys DDR memory controller registers that are relevant to ECC */
@@ -278,28 +276,20 @@ struct synps_edac_priv {
};
/**
- * struct synps_platform_data - synps platform data structure.
- * @get_error_info: Get EDAC error info.
- * @get_mtype: Get mtype.
- * @get_dtype: Get dtype.
- * @get_ecc_state: Get ECC state.
- * @quirks: To differentiate IPs.
+ * struct synps_platform_data - Synopsys uMCTL2 DDRC platform data.
+ * @quirks: IP-core specific quirks.
*/
struct synps_platform_data {
- int (*get_error_info)(struct synps_edac_priv *priv);
- enum mem_type (*get_mtype)(const void __iomem *base);
- enum dev_type (*get_dtype)(const void __iomem *base);
- bool (*get_ecc_state)(void __iomem *base);
- int quirks;
+ u32 quirks;
};
/**
- * zynqmp_get_error_info - Get the current ECC error info.
+ * synps_get_error_info - Get the current ECC error info.
* @priv: DDR memory controller private instance data.
*
* Return: one if there is no error otherwise returns zero.
*/
-static int zynqmp_get_error_info(struct synps_edac_priv *priv)
+static int synps_get_error_info(struct synps_edac_priv *priv)
{
struct synps_ecc_status *p;
u32 regval, clearval;
@@ -374,17 +364,11 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
if (p->ce_cnt) {
pinf = &p->ceinfo;
- if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
- snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
- "Row %d Col %d Bank %d Bank Group %d Bit %d Data 0x%08x",
- pinf->row, pinf->col, pinf->bank, pinf->bankgrp,
- pinf->bitpos, pinf->data);
- } else {
- snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
- "Row %d Bank %d Col %d Bit: %d Data: 0x%08x",
- pinf->row, pinf->bank, pinf->col,
- pinf->bitpos, pinf->data);
- }
+
+ snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
+ "Row %d Col %d Bank %d Bank Group %d Bit %d Data 0x%08x",
+ pinf->row, pinf->col, pinf->bank, pinf->bankgrp,
+ pinf->bitpos, pinf->data);
edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
p->ce_cnt, 0, 0, 0, 0, 0, -1,
@@ -393,15 +377,10 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
if (p->ue_cnt) {
pinf = &p->ueinfo;
- if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
- snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
- "Row %d Col %d Bank %d Bank Group %d",
- pinf->row, pinf->col, pinf->bank, pinf->bankgrp);
- } else {
- snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
- "Row %d Bank %d Col %d",
- pinf->row, pinf->bank, pinf->col);
- }
+
+ snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
+ "Row %d Col %d Bank %d Bank Group %d",
+ pinf->row, pinf->col, pinf->bank, pinf->bankgrp);
edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
p->ue_cnt, 0, 0, 0, 0, 0, -1,
@@ -460,13 +439,11 @@ static void disable_intr(struct synps_edac_priv *priv)
*/
static irqreturn_t intr_handler(int irq, void *dev_id)
{
- const struct synps_platform_data *p_data;
struct mem_ctl_info *mci = dev_id;
struct synps_edac_priv *priv;
int status, regval;
priv = mci->pvt_info;
- p_data = priv->p_data;
if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS) {
regval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
@@ -475,7 +452,7 @@ static irqreturn_t intr_handler(int irq, void *dev_id)
return IRQ_NONE;
}
- status = p_data->get_error_info(priv);
+ status = synps_get_error_info(priv);
if (status)
return IRQ_NONE;
@@ -489,34 +466,7 @@ static irqreturn_t intr_handler(int irq, void *dev_id)
}
/**
- * check_errors - Check controller for ECC errors.
- * @mci: EDAC memory controller instance.
- *
- * Check and post ECC errors. Called by the polling thread.
- */
-static void check_errors(struct mem_ctl_info *mci)
-{
- const struct synps_platform_data *p_data;
- struct synps_edac_priv *priv;
- int status;
-
- priv = mci->pvt_info;
- p_data = priv->p_data;
-
- status = p_data->get_error_info(priv);
- if (status)
- return;
-
- priv->ce_cnt += priv->stat.ce_cnt;
- priv->ue_cnt += priv->stat.ue_cnt;
- handle_error(mci, &priv->stat);
-
- edac_dbg(3, "Total error count CE %d UE %d\n",
- priv->ce_cnt, priv->ue_cnt);
-}
-
-/**
- * zynqmp_get_dtype - Return the controller memory width.
+ * synps_get_dtype - Return the controller memory width.
* @base: DDR memory controller base address.
*
* Get the EDAC device type width appropriate for the current controller
@@ -524,7 +474,7 @@ static void check_errors(struct mem_ctl_info *mci)
*
* Return: a device type width enumeration.
*/
-static enum dev_type zynqmp_get_dtype(const void __iomem *base)
+static enum dev_type synps_get_dtype(const void __iomem *base)
{
u32 regval;
@@ -548,14 +498,14 @@ static enum dev_type zynqmp_get_dtype(const void __iomem *base)
}
/**
- * zynqmp_get_ecc_state - Return the controller ECC enable/disable status.
+ * synps_get_ecc_state - Return the controller ECC enable/disable status.
* @base: DDR memory controller base address.
*
* Get the ECC enable/disable status for the controller.
*
* Return: a ECC status boolean i.e true/false - enabled/disabled.
*/
-static bool zynqmp_get_ecc_state(void __iomem *base)
+static bool synps_get_ecc_state(void __iomem *base)
{
u32 regval;
@@ -579,7 +529,7 @@ static u32 get_memsize(void)
}
/**
- * zynqmp_get_mtype - Returns controller memory type.
+ * synps_get_mtype - Returns controller memory type.
* @base: Synopsys ECC status structure.
*
* Get the EDAC memory type appropriate for the current controller
@@ -587,7 +537,7 @@ static u32 get_memsize(void)
*
* Return: a memory type enumeration.
*/
-static enum mem_type zynqmp_get_mtype(const void __iomem *base)
+static enum mem_type synps_get_mtype(const void __iomem *base)
{
enum mem_type mt;
u32 memtype;
@@ -616,14 +566,11 @@ static enum mem_type zynqmp_get_mtype(const void __iomem *base)
static void init_csrows(struct mem_ctl_info *mci)
{
struct synps_edac_priv *priv = mci->pvt_info;
- const struct synps_platform_data *p_data;
struct csrow_info *csi;
struct dimm_info *dimm;
u32 size, row;
int j;
- p_data = priv->p_data;
-
for (row = 0; row < mci->nr_csrows; row++) {
csi = mci->csrows[row];
size = get_memsize();
@@ -631,10 +578,10 @@ static void init_csrows(struct mem_ctl_info *mci)
for (j = 0; j < csi->nr_channels; j++) {
dimm = csi->channels[j]->dimm;
dimm->edac_mode = EDAC_SECDED;
- dimm->mtype = p_data->get_mtype(priv->baseaddr);
+ dimm->mtype = synps_get_mtype(priv->baseaddr);
dimm->nr_pages = (size >> PAGE_SHIFT) / csi->nr_channels;
dimm->grain = SYNPS_EDAC_ERR_GRAIN;
- dimm->dtype = p_data->get_dtype(priv->baseaddr);
+ dimm->dtype = synps_get_dtype(priv->baseaddr);
}
}
}
@@ -667,12 +614,7 @@ static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
mci->dev_name = SYNPS_EDAC_MOD_STRING;
mci->mod_name = SYNPS_EDAC_MOD_VER;
- if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
- edac_op_state = EDAC_OPSTATE_INT;
- } else {
- edac_op_state = EDAC_OPSTATE_POLL;
- mci->edac_check = check_errors;
- }
+ edac_op_state = EDAC_OPSTATE_INT;
mci->ctl_page_to_phys = NULL;
@@ -704,47 +646,6 @@ static int setup_irq(struct mem_ctl_info *mci,
return 0;
}
-static const struct synps_platform_data zynqmp_edac_def = {
- .get_error_info = zynqmp_get_error_info,
- .get_mtype = zynqmp_get_mtype,
- .get_dtype = zynqmp_get_dtype,
- .get_ecc_state = zynqmp_get_ecc_state,
- .quirks = (DDR_ECC_INTR_SUPPORT | SYNPS_ZYNQMP_IRQ_REGS
-#ifdef CONFIG_EDAC_DEBUG
- | DDR_ECC_DATA_POISON_SUPPORT
-#endif
- ),
-};
-
-static const struct synps_platform_data synopsys_edac_def = {
- .get_error_info = zynqmp_get_error_info,
- .get_mtype = zynqmp_get_mtype,
- .get_dtype = zynqmp_get_dtype,
- .get_ecc_state = zynqmp_get_ecc_state,
- .quirks = (DDR_ECC_INTR_SUPPORT
-#ifdef CONFIG_EDAC_DEBUG
- | DDR_ECC_DATA_POISON_SUPPORT
-#endif
- ),
-};
-
-
-static const struct of_device_id synps_edac_match[] = {
- {
- .compatible = "xlnx,zynqmp-ddrc-2.40a",
- .data = (void *)&zynqmp_edac_def
- },
- {
- .compatible = "snps,ddrc-3.80a",
- .data = (void *)&synopsys_edac_def
- },
- {
- /* end of table */
- }
-};
-
-MODULE_DEVICE_TABLE(of, synps_edac_match);
-
#ifdef CONFIG_EDAC_DEBUG
/**
@@ -1135,7 +1036,7 @@ static int mc_probe(struct platform_device *pdev)
if (!p_data)
return -ENODEV;
- if (!p_data->get_ecc_state(baseaddr)) {
+ if (!synps_get_ecc_state(baseaddr)) {
edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
return -ENXIO;
}
@@ -1162,11 +1063,9 @@ static int mc_probe(struct platform_device *pdev)
mc_init(mci, pdev);
- if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
- rc = setup_irq(mci, pdev);
- if (rc)
- goto free_edac_mc;
- }
+ rc = setup_irq(mci, pdev);
+ if (rc)
+ goto free_edac_mc;
rc = edac_mc_add_mc(mci);
if (rc) {
@@ -1176,17 +1075,13 @@ static int mc_probe(struct platform_device *pdev)
}
#ifdef CONFIG_EDAC_DEBUG
- if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT) {
- rc = edac_create_sysfs_attributes(mci);
- if (rc) {
- edac_printk(KERN_ERR, EDAC_MC,
- "Failed to create sysfs entries\n");
- goto free_edac_mc;
- }
+ rc = edac_create_sysfs_attributes(mci);
+ if (rc) {
+ edac_printk(KERN_ERR, EDAC_MC, "Failed to create sysfs entries\n");
+ goto free_edac_mc;
}
- if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT)
- setup_address_map(priv);
+ setup_address_map(priv);
#endif
return rc;
@@ -1208,12 +1103,10 @@ static int mc_remove(struct platform_device *pdev)
struct mem_ctl_info *mci = platform_get_drvdata(pdev);
struct synps_edac_priv *priv = mci->pvt_info;
- if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT)
- disable_intr(priv);
+ disable_intr(priv);
#ifdef CONFIG_EDAC_DEBUG
- if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT)
- edac_remove_sysfs_attributes(mci);
+ edac_remove_sysfs_attributes(mci);
#endif
edac_mc_del_mc(&pdev->dev);
@@ -1222,6 +1115,21 @@ static int mc_remove(struct platform_device *pdev)
return 0;
}
+static const struct synps_platform_data zynqmp_edac_def = {
+ .quirks = SYNPS_ZYNQMP_IRQ_REGS,
+};
+
+static const struct synps_platform_data synopsys_edac_def = {
+ .quirks = 0,
+};
+
+static const struct of_device_id synps_edac_match[] = {
+ { .compatible = "xlnx,zynqmp-ddrc-2.40a", .data = &zynqmp_edac_def },
+ { .compatible = "snps,ddrc-3.80a", .data = &synopsys_edac_def },
+ { }
+};
+MODULE_DEVICE_TABLE(of, synps_edac_match);
+
static struct platform_driver synps_edac_mc_driver = {
.driver = {
.name = "synopsys-edac",
--
2.35.1
It was a bad idea in the first place to combine two absolutely different
controllers support in a single driver [1]. It caused having an additional
level of abstraction, which obviously have needlessly overcomplicated the
driver and as such caused many problems in the new main controller
features support implementation. The solution looks even more unreasonable
now seeing the justification of having both controllers support in a
single driver hasn't been implemented by the original code author [2].
Anyway since the Zynq A05 DDRC controller has nothing in common with DW
uMCTL2 DDRC (even the CSRs layout is absolutely different, not to say it
doesn't support IRQs), since the EDAC MC core driver now support MC index
auto-generation and since we are going to extend the DW uMCTL2 EDAC driver
functionality (having an incompatible code support will greatly complicate
the resultant driver and the upcoming patches) let's move the Xilinx Zynq
DDRC support to another driver - zynq_edac.c. Basically it means to simply
detach the Zynq-specific callbacks, init/probe/remove methods and move
them in the new driver. The resultant driver will mostly look similar to
the code submitted in the initial commit ae9b56e3996d ("EDAC, synps: Add
EDAC support for zynq ddr ecc controller") except a few fixes added
afterwards. The Zynq-specific code and macros are removed from the
original module of course thus leaving the DWC uMCTL2 DDRC driver purely
synopsys DDRC specific.
Note 1. In order to improve the new driver code readability all the
internal entities have been equipped with the vendor-specific prefix. Thus
the local, global and EDAC-core specific names will be explicitly
distinguishable right from the place they are called.
Note 2. Some of the Zynq-specific macros have been used in the framework
of the DW uMCTL2 DDRC-specific functions. In most of the cases it was
wrong even though hasn't caused any problem since the macro substituted
with correct values (for instance, CTRL_OFST macro usage where
DDR_MSTR_OFST should have been utilized). Anyway since these macros are
now moved to another driver let's fix the places where the macros have
been improperly used.
[1] Link: https://lore.kernel.org/all/808655a9-77eb-4e3a-9781-2b059ad9517b@BN1AFFO11FD020.protection.gbl/
[2] Link: https://lore.kernel.org/all/9dc2a947-d2ab-4f00-8ed3-d2499cb6fdfd@BN1BFFO11FD002.protection.gbl/
Signed-off-by: Serge Semin <[email protected]>
---
MAINTAINERS | 1 +
drivers/edac/Kconfig | 9 +-
drivers/edac/Makefile | 1 +
drivers/edac/synopsys_edac.c | 271 +++----------------
drivers/edac/zynq_edac.c | 504 +++++++++++++++++++++++++++++++++++
5 files changed, 547 insertions(+), 239 deletions(-)
create mode 100644 drivers/edac/zynq_edac.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 17f23f810ee4..27b6bb41e401 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3094,6 +3094,7 @@ F: arch/arm/mach-zynq/
F: drivers/clocksource/timer-cadence-ttc.c
F: drivers/cpuidle/cpuidle-zynq.c
F: drivers/edac/synopsys_edac.c
+F: drivers/edac/zynq_edac.c
F: drivers/i2c/busses/i2c-cadence.c
F: drivers/i2c/busses/i2c-xiic.c
F: drivers/mmc/host/sdhci-of-arasan.c
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 17562cf1fe97..98bcdadf4143 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -486,7 +486,7 @@ config EDAC_ARMADA_XP
config EDAC_SYNOPSYS
tristate "Synopsys DDR Memory Controller"
- depends on ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_INTEL_SOCFPGA || ARCH_MXC
+ depends on ARCH_ZYNQMP || ARCH_INTEL_SOCFPGA || ARCH_MXC
help
Support for error detection and correction on the Synopsys DDR
memory controller.
@@ -541,4 +541,11 @@ config EDAC_DMC520
Support for error detection and correction on the
SoCs with ARM DMC-520 DRAM controller.
+config EDAC_ZYNQ
+ tristate "Xilinx Zynq A05 DDR Memory Controller"
+ depends on ARCH_ZYNQ
+ help
+ Support for error detection and correction on the Xilinx Zynq A05
+ DDR memory controller.
+
endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 2d1641a27a28..83e063f53b22 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_ZYNQ) += zynq_edac.o
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 379a668295cc..0cb84f183aed 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -30,68 +30,16 @@
#define SYNPS_EDAC_MOD_STRING "synps_edac"
#define SYNPS_EDAC_MOD_VER "1"
-/* Synopsys DDR memory controller registers that are relevant to ECC */
-#define CTRL_OFST 0x0
-#define T_ZQ_OFST 0xA4
-
-/* ECC control register */
-#define ECC_CTRL_OFST 0xC4
-/* ECC log register */
-#define CE_LOG_OFST 0xC8
-/* ECC address register */
-#define CE_ADDR_OFST 0xCC
-/* ECC data[31:0] register */
-#define CE_DATA_31_0_OFST 0xD0
-
-/* Uncorrectable error info registers */
-#define UE_LOG_OFST 0xDC
-#define UE_ADDR_OFST 0xE0
-#define UE_DATA_31_0_OFST 0xE4
-
-#define STAT_OFST 0xF0
-#define SCRUB_OFST 0xF4
-
-/* Control register bit field definitions */
-#define CTRL_BW_MASK 0xC
-#define CTRL_BW_SHIFT 2
-
-#define DDRCTL_WDTH_16 1
-#define DDRCTL_WDTH_32 0
-
-/* ZQ register bit field definitions */
-#define T_ZQ_DDRMODE_MASK 0x2
-
-/* ECC control register bit field definitions */
-#define ECC_CTRL_CLR_CE_ERR 0x2
-#define ECC_CTRL_CLR_UE_ERR 0x1
-
-/* ECC correctable/uncorrectable error log register definitions */
-#define LOG_VALID 0x1
-#define CE_LOG_BITPOS_MASK 0xFE
-#define CE_LOG_BITPOS_SHIFT 1
-
-/* ECC correctable/uncorrectable error address register definitions */
-#define ADDR_COL_MASK 0xFFF
-#define ADDR_ROW_MASK 0xFFFF000
-#define ADDR_ROW_SHIFT 12
-#define ADDR_BANK_MASK 0x70000000
-#define ADDR_BANK_SHIFT 28
-
-/* ECC statistic register definitions */
-#define STAT_UECNT_MASK 0xFF
-#define STAT_CECNT_MASK 0xFF00
-#define STAT_CECNT_SHIFT 8
-
-/* ECC scrub register definitions */
-#define SCRUB_MODE_MASK 0x7
-#define SCRUB_MODE_SECDED 0x4
-
/* DDR ECC Quirks */
#define DDR_ECC_INTR_SUPPORT BIT(0)
#define DDR_ECC_DATA_POISON_SUPPORT BIT(1)
#define SYNPS_ZYNQMP_IRQ_REGS BIT(2)
-/* ZynqMP Enhanced DDR memory controller registers that are relevant to ECC */
+/* Synopsys DDR memory controller registers that are relevant to ECC */
+
+/* DDRC master0 Register */
+#define DDR_MSTR_OFST 0x0
+
/* ECC Configuration Registers */
#define ECC_CFG0_OFST 0x70
#define ECC_CFG1_OFST 0x74
@@ -132,15 +80,11 @@
#define ECC_POISON0_OFST 0xB8
#define ECC_POISON1_OFST 0xBC
-#define ECC_ADDRMAP0_OFFSET 0x200
+/* DDR Address map0 Registers */
+#define ECC_ADDRMAP0_OFST 0x200
-/* ECC control/clear register definitions */
-#define ECC_CTRL_BUSWIDTH_MASK 0x3000
-#define ECC_CTRL_BUSWIDTH_SHIFT 12
-#define ECC_CTRL_CLR_CE_ERRCNT BIT(2)
-#define ECC_CTRL_CLR_UE_ERRCNT BIT(3)
-#define ECC_CTRL_EN_CE_IRQ BIT(8)
-#define ECC_CTRL_EN_UE_IRQ BIT(9)
+/* DDR Software control register */
+#define DDR_SWCTL 0x320
/* DDR Master Register 0 definitions */
#define DDR_MSTR_DEV_CFG_MASK GENMASK(31, 30)
@@ -148,10 +92,16 @@
#define DDR_MSTR_DEV_X8 0x1
#define DDR_MSTR_DEV_X16 0x2
#define DDR_MSTR_DEV_X32 0x3
+#define DDR_MSTR_BUSWIDTH_MASK 0x3000
+#define DDR_MSTR_BUSWIDTH_SHIFT 12
#define DDRCTL_EWDTH_16 2
#define DDRCTL_EWDTH_32 1
#define DDRCTL_EWDTH_64 0
+/* ECC CFG0 register definitions */
+#define ECC_CFG0_MODE_MASK 0x7
+#define ECC_CFG0_MODE_SECDED 0x4
+
/* ECC status register definitions */
#define ECC_STAT_UECNT_MASK 0xF0000
#define ECC_STAT_UECNT_SHIFT 16
@@ -159,6 +109,14 @@
#define ECC_STAT_CECNT_SHIFT 8
#define ECC_STAT_BITNUM_MASK 0x7F
+/* ECC control/clear register definitions */
+#define ECC_CTRL_CLR_CE_ERR BIT(0)
+#define ECC_CTRL_CLR_UE_ERR BIT(1)
+#define ECC_CTRL_CLR_CE_ERRCNT BIT(2)
+#define ECC_CTRL_CLR_UE_ERRCNT BIT(3)
+#define ECC_CTRL_EN_CE_IRQ BIT(8)
+#define ECC_CTRL_EN_UE_IRQ BIT(9)
+
/* ECC error count register definitions */
#define ECC_ERRCNT_UECNT_MASK 0xFFFF0000
#define ECC_ERRCNT_UECNT_SHIFT 16
@@ -200,21 +158,11 @@
#define MEM_TYPE_DDR4 0x10
#define MEM_TYPE_LPDDR4 0x20
-/* DDRC Software control register */
-#define DDRC_SWCTL 0x320
-
/* DDRC ECC CE & UE poison mask */
#define ECC_CEPOISON_MASK 0x3
#define ECC_UEPOISON_MASK 0x1
-/* DDRC Device config masks */
-#define DDRC_MSTR_CFG_MASK 0xC0000000
-#define DDRC_MSTR_CFG_SHIFT 30
-#define DDRC_MSTR_CFG_X4_MASK 0x0
-#define DDRC_MSTR_CFG_X8_MASK 0x1
-#define DDRC_MSTR_CFG_X16_MASK 0x2
-#define DDRC_MSTR_CFG_X32_MASK 0x3
-
+/* DDRC Device config shifts/masks */
#define DDR_MAX_ROW_SHIFT 18
#define DDR_MAX_COL_SHIFT 14
#define DDR_MAX_BANK_SHIFT 3
@@ -345,61 +293,6 @@ struct synps_platform_data {
int quirks;
};
-/**
- * zynq_get_error_info - Get the current ECC error info.
- * @priv: DDR memory controller private instance data.
- *
- * Return: one if there is no error, otherwise zero.
- */
-static int zynq_get_error_info(struct synps_edac_priv *priv)
-{
- struct synps_ecc_status *p;
- u32 regval, clearval = 0;
- void __iomem *base;
-
- base = priv->baseaddr;
- p = &priv->stat;
-
- regval = readl(base + STAT_OFST);
- if (!regval)
- return 1;
-
- p->ce_cnt = (regval & STAT_CECNT_MASK) >> STAT_CECNT_SHIFT;
- p->ue_cnt = regval & STAT_UECNT_MASK;
-
- regval = readl(base + CE_LOG_OFST);
- if (!(p->ce_cnt && (regval & LOG_VALID)))
- goto ue_err;
-
- p->ceinfo.bitpos = (regval & CE_LOG_BITPOS_MASK) >> CE_LOG_BITPOS_SHIFT;
- regval = readl(base + CE_ADDR_OFST);
- p->ceinfo.row = (regval & ADDR_ROW_MASK) >> ADDR_ROW_SHIFT;
- p->ceinfo.col = regval & ADDR_COL_MASK;
- p->ceinfo.bank = (regval & ADDR_BANK_MASK) >> ADDR_BANK_SHIFT;
- p->ceinfo.data = readl(base + CE_DATA_31_0_OFST);
- edac_dbg(3, "CE bit position: %d data: %d\n", p->ceinfo.bitpos,
- p->ceinfo.data);
- clearval = ECC_CTRL_CLR_CE_ERR;
-
-ue_err:
- regval = readl(base + UE_LOG_OFST);
- if (!(p->ue_cnt && (regval & LOG_VALID)))
- goto out;
-
- regval = readl(base + UE_ADDR_OFST);
- p->ueinfo.row = (regval & ADDR_ROW_MASK) >> ADDR_ROW_SHIFT;
- p->ueinfo.col = regval & ADDR_COL_MASK;
- p->ueinfo.bank = (regval & ADDR_BANK_MASK) >> ADDR_BANK_SHIFT;
- p->ueinfo.data = readl(base + UE_DATA_31_0_OFST);
- clearval |= ECC_CTRL_CLR_UE_ERR;
-
-out:
- writel(clearval, base + ECC_CTRL_OFST);
- writel(0x0, base + ECC_CTRL_OFST);
-
- return 0;
-}
-
/**
* zynqmp_get_error_info - Get the current ECC error info.
* @priv: DDR memory controller private instance data.
@@ -622,37 +515,6 @@ static void check_errors(struct mem_ctl_info *mci)
priv->ce_cnt, priv->ue_cnt);
}
-/**
- * zynq_get_dtype - Return the controller memory width.
- * @base: DDR memory controller base address.
- *
- * Get the EDAC device type width appropriate for the current controller
- * configuration.
- *
- * Return: a device type width enumeration.
- */
-static enum dev_type zynq_get_dtype(const void __iomem *base)
-{
- enum dev_type dt;
- u32 width;
-
- width = readl(base + CTRL_OFST);
- width = (width & CTRL_BW_MASK) >> CTRL_BW_SHIFT;
-
- switch (width) {
- case DDRCTL_WDTH_16:
- dt = DEV_X2;
- break;
- case DDRCTL_WDTH_32:
- dt = DEV_X4;
- break;
- default:
- dt = DEV_UNKNOWN;
- }
-
- return dt;
-}
-
/**
* zynqmp_get_dtype - Return the controller memory width.
* @base: DDR memory controller base address.
@@ -685,30 +547,6 @@ static enum dev_type zynqmp_get_dtype(const void __iomem *base)
return DEV_UNKNOWN;
}
-/**
- * zynq_get_ecc_state - Return the controller ECC enable/disable status.
- * @base: DDR memory controller base address.
- *
- * Get the ECC enable/disable status of the controller.
- *
- * Return: true if enabled, otherwise false.
- */
-static bool zynq_get_ecc_state(void __iomem *base)
-{
- enum dev_type dt;
- u32 ecctype;
-
- dt = zynq_get_dtype(base);
- if (dt == DEV_UNKNOWN)
- return false;
-
- ecctype = readl(base + SCRUB_OFST) & SCRUB_MODE_MASK;
- if ((ecctype == SCRUB_MODE_SECDED) && (dt == DEV_X2))
- return true;
-
- return false;
-}
-
/**
* zynqmp_get_ecc_state - Return the controller ECC enable/disable status.
* @base: DDR memory controller base address.
@@ -721,9 +559,9 @@ static bool zynqmp_get_ecc_state(void __iomem *base)
{
u32 regval;
- regval = readl(base + ECC_CFG0_OFST) & SCRUB_MODE_MASK;
+ regval = readl(base + ECC_CFG0_OFST) & ECC_CFG0_MODE_MASK;
- return (regval == SCRUB_MODE_SECDED);
+ return (regval == ECC_CFG0_MODE_SECDED);
}
/**
@@ -740,30 +578,6 @@ static u32 get_memsize(void)
return inf.totalram * inf.mem_unit;
}
-/**
- * zynq_get_mtype - Return the controller memory type.
- * @base: Synopsys ECC status structure.
- *
- * Get the EDAC memory type appropriate for the current controller
- * configuration.
- *
- * Return: a memory type enumeration.
- */
-static enum mem_type zynq_get_mtype(const void __iomem *base)
-{
- enum mem_type mt;
- u32 memtype;
-
- memtype = readl(base + T_ZQ_OFST);
-
- if (memtype & T_ZQ_DDRMODE_MASK)
- mt = MEM_DDR3;
- else
- mt = MEM_DDR2;
-
- return mt;
-}
-
/**
* zynqmp_get_mtype - Returns controller memory type.
* @base: Synopsys ECC status structure.
@@ -778,7 +592,7 @@ static enum mem_type zynqmp_get_mtype(const void __iomem *base)
enum mem_type mt;
u32 memtype;
- memtype = readl(base + CTRL_OFST);
+ memtype = readl(base + DDR_MSTR_OFST);
if ((memtype & MEM_TYPE_DDR3) || (memtype & MEM_TYPE_LPDDR3))
mt = MEM_DDR3;
@@ -890,14 +704,6 @@ static int setup_irq(struct mem_ctl_info *mci,
return 0;
}
-static const struct synps_platform_data zynq_edac_def = {
- .get_error_info = zynq_get_error_info,
- .get_mtype = zynq_get_mtype,
- .get_dtype = zynq_get_dtype,
- .get_ecc_state = zynq_get_ecc_state,
- .quirks = 0,
-};
-
static const struct synps_platform_data zynqmp_edac_def = {
.get_error_info = zynqmp_get_error_info,
.get_mtype = zynqmp_get_mtype,
@@ -924,10 +730,6 @@ static const struct synps_platform_data synopsys_edac_def = {
static const struct of_device_id synps_edac_match[] = {
- {
- .compatible = "xlnx,zynq-ddrc-a05",
- .data = (void *)&zynq_edac_def
- },
{
.compatible = "xlnx,zynqmp-ddrc-2.40a",
.data = (void *)&zynqmp_edac_def
@@ -1053,12 +855,12 @@ static ssize_t inject_data_poison_store(struct device *dev,
struct mem_ctl_info *mci = to_mci(dev);
struct synps_edac_priv *priv = mci->pvt_info;
- writel(0, priv->baseaddr + DDRC_SWCTL);
+ writel(0, priv->baseaddr + DDR_SWCTL);
if (strncmp(data, "CE", 2) == 0)
writel(ECC_CEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
else
writel(ECC_UEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
- writel(1, priv->baseaddr + DDRC_SWCTL);
+ writel(1, priv->baseaddr + DDR_SWCTL);
return count;
}
@@ -1149,8 +951,8 @@ static void setup_column_address_map(struct synps_edac_priv *priv, u32 *addrmap)
u32 width, memtype;
int index;
- memtype = readl(priv->baseaddr + CTRL_OFST);
- width = (memtype & ECC_CTRL_BUSWIDTH_MASK) >> ECC_CTRL_BUSWIDTH_SHIFT;
+ memtype = readl(priv->baseaddr + DDR_MSTR_OFST);
+ width = (memtype & DDR_MSTR_BUSWIDTH_MASK) >> DDR_MSTR_BUSWIDTH_SHIFT;
priv->col_shift[0] = 0;
priv->col_shift[1] = 1;
@@ -1291,7 +1093,7 @@ static void setup_address_map(struct synps_edac_priv *priv)
for (index = 0; index < 12; index++) {
u32 addrmap_offset;
- addrmap_offset = ECC_ADDRMAP0_OFFSET + (index * 4);
+ addrmap_offset = ECC_ADDRMAP0_OFST + (index * 4);
addrmap[index] = readl(priv->baseaddr + addrmap_offset);
}
@@ -1345,7 +1147,7 @@ static int mc_probe(struct platform_device *pdev)
layers[1].size = SYNPS_EDAC_NR_CHANS;
layers[1].is_virt_csrow = false;
- mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
+ mci = edac_mc_alloc(EDAC_AUTO_MC_NUM, ARRAY_SIZE(layers), layers,
sizeof(struct synps_edac_priv));
if (!mci) {
edac_printk(KERN_ERR, EDAC_MC,
@@ -1387,13 +1189,6 @@ static int mc_probe(struct platform_device *pdev)
setup_address_map(priv);
#endif
- /*
- * Start capturing the correctable and uncorrectable errors. A write of
- * 0 starts the counters.
- */
- if (!(priv->p_data->quirks & DDR_ECC_INTR_SUPPORT))
- writel(0x0, baseaddr + ECC_CTRL_OFST);
-
return rc;
free_edac_mc:
diff --git a/drivers/edac/zynq_edac.c b/drivers/edac/zynq_edac.c
new file mode 100644
index 000000000000..66a2dedddfef
--- /dev/null
+++ b/drivers/edac/zynq_edac.c
@@ -0,0 +1,504 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Zynq DDR ECC Driver
+ * This driver is based on ppc4xx_edac.c drivers
+ *
+ * Copyright (C) 2012 - 2014 Xilinx, Inc.
+ */
+
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "edac_module.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_ERR_GRAIN 1
+
+#define ZYNQ_EDAC_MSG_SIZE 256
+
+#define ZYNQ_EDAC_MOD_STRING "zynq_edac"
+#define ZYNQ_EDAC_MOD_VER "1"
+
+/* Zynq DDR memory controller ECC registers */
+#define ZYNQ_CTRL_OFST 0x0
+#define ZYNQ_T_ZQ_OFST 0xA4
+
+/* ECC control register */
+#define ZYNQ_ECC_CTRL_OFST 0xC4
+/* ECC log register */
+#define ZYNQ_CE_LOG_OFST 0xC8
+/* ECC address register */
+#define ZYNQ_CE_ADDR_OFST 0xCC
+/* ECC data[31:0] register */
+#define ZYNQ_CE_DATA_31_0_OFST 0xD0
+
+/* Uncorrectable error info registers */
+#define ZYNQ_UE_LOG_OFST 0xDC
+#define ZYNQ_UE_ADDR_OFST 0xE0
+#define ZYNQ_UE_DATA_31_0_OFST 0xE4
+
+#define ZYNQ_STAT_OFST 0xF0
+#define ZYNQ_SCRUB_OFST 0xF4
+
+/* Control register bit field definitions */
+#define ZYNQ_CTRL_BW_MASK 0xC
+#define ZYNQ_CTRL_BW_SHIFT 2
+
+#define ZYNQ_DDRCTL_WDTH_16 1
+#define ZYNQ_DDRCTL_WDTH_32 0
+
+/* ZQ register bit field definitions */
+#define ZYNQ_T_ZQ_DDRMODE_MASK 0x2
+
+/* ECC control register bit field definitions */
+#define ZYNQ_ECC_CTRL_CLR_CE_ERR 0x2
+#define ZYNQ_ECC_CTRL_CLR_UE_ERR 0x1
+
+/* ECC correctable/uncorrectable error log register definitions */
+#define ZYNQ_LOG_VALID 0x1
+#define ZYNQ_CE_LOG_BITPOS_MASK 0xFE
+#define ZYNQ_CE_LOG_BITPOS_SHIFT 1
+
+/* ECC correctable/uncorrectable error address register definitions */
+#define ZYNQ_ADDR_COL_MASK 0xFFF
+#define ZYNQ_ADDR_ROW_MASK 0xFFFF000
+#define ZYNQ_ADDR_ROW_SHIFT 12
+#define ZYNQ_ADDR_BANK_MASK 0x70000000
+#define ZYNQ_ADDR_BANK_SHIFT 28
+
+/* ECC statistic register definitions */
+#define ZYNQ_STAT_UECNT_MASK 0xFF
+#define ZYNQ_STAT_CECNT_MASK 0xFF00
+#define ZYNQ_STAT_CECNT_SHIFT 8
+
+/* ECC scrub register definitions */
+#define ZYNQ_SCRUB_MODE_MASK 0x7
+#define ZYNQ_SCRUB_MODE_SECDED 0x4
+
+/**
+ * struct zynq_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 zynq_ecc_error_info {
+ u32 row;
+ u32 col;
+ u32 bank;
+ u32 bitpos;
+ u32 data;
+};
+
+/**
+ * struct zynq_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 zynq_ecc_status {
+ u32 ce_cnt;
+ u32 ue_cnt;
+ struct zynq_ecc_error_info ceinfo;
+ struct zynq_ecc_error_info ueinfo;
+};
+
+/**
+ * struct zynq_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.
+ */
+struct zynq_edac_priv {
+ void __iomem *baseaddr;
+ char message[ZYNQ_EDAC_MSG_SIZE];
+ struct zynq_ecc_status stat;
+};
+
+/**
+ * zynq_get_error_info - Get the current ECC error info.
+ * @priv: DDR memory controller private instance data.
+ *
+ * Return: one if there is no error, otherwise zero.
+ */
+static int zynq_get_error_info(struct zynq_edac_priv *priv)
+{
+ struct zynq_ecc_status *p;
+ u32 regval, clearval = 0;
+ void __iomem *base;
+
+ base = priv->baseaddr;
+ p = &priv->stat;
+
+ regval = readl(base + ZYNQ_STAT_OFST);
+ if (!regval)
+ return 1;
+
+ p->ce_cnt = (regval & ZYNQ_STAT_CECNT_MASK) >> ZYNQ_STAT_CECNT_SHIFT;
+ p->ue_cnt = regval & ZYNQ_STAT_UECNT_MASK;
+
+ regval = readl(base + ZYNQ_CE_LOG_OFST);
+ if (!(p->ce_cnt && (regval & ZYNQ_LOG_VALID)))
+ goto ue_err;
+
+ p->ceinfo.bitpos = (regval & ZYNQ_CE_LOG_BITPOS_MASK) >> ZYNQ_CE_LOG_BITPOS_SHIFT;
+ regval = readl(base + ZYNQ_CE_ADDR_OFST);
+ p->ceinfo.row = (regval & ZYNQ_ADDR_ROW_MASK) >> ZYNQ_ADDR_ROW_SHIFT;
+ p->ceinfo.col = regval & ZYNQ_ADDR_COL_MASK;
+ p->ceinfo.bank = (regval & ZYNQ_ADDR_BANK_MASK) >> ZYNQ_ADDR_BANK_SHIFT;
+ p->ceinfo.data = readl(base + ZYNQ_CE_DATA_31_0_OFST);
+ edac_dbg(3, "CE bit position: %d data: %d\n", p->ceinfo.bitpos,
+ p->ceinfo.data);
+ clearval = ZYNQ_ECC_CTRL_CLR_CE_ERR;
+
+ue_err:
+ regval = readl(base + ZYNQ_UE_LOG_OFST);
+ if (!(p->ue_cnt && (regval & ZYNQ_LOG_VALID)))
+ goto out;
+
+ regval = readl(base + ZYNQ_UE_ADDR_OFST);
+ p->ueinfo.row = (regval & ZYNQ_ADDR_ROW_MASK) >> ZYNQ_ADDR_ROW_SHIFT;
+ p->ueinfo.col = regval & ZYNQ_ADDR_COL_MASK;
+ p->ueinfo.bank = (regval & ZYNQ_ADDR_BANK_MASK) >> ZYNQ_ADDR_BANK_SHIFT;
+ p->ueinfo.data = readl(base + ZYNQ_UE_DATA_31_0_OFST);
+ clearval |= ZYNQ_ECC_CTRL_CLR_UE_ERR;
+
+out:
+ writel(clearval, base + ZYNQ_ECC_CTRL_OFST);
+ writel(0x0, base + ZYNQ_ECC_CTRL_OFST);
+
+ return 0;
+}
+
+/**
+ * handle_error - Handle Correctable and Uncorrectable errors.
+ * @mci: EDAC memory controller instance.
+ * @p: Zynq ECC status structure.
+ *
+ * Handles ECC correctable and uncorrectable errors.
+ */
+static void zynq_handle_error(struct mem_ctl_info *mci, struct zynq_ecc_status *p)
+{
+ struct zynq_edac_priv *priv = mci->pvt_info;
+ struct zynq_ecc_error_info *pinf;
+
+ if (p->ce_cnt) {
+ pinf = &p->ceinfo;
+
+ snprintf(priv->message, ZYNQ_EDAC_MSG_SIZE,
+ "Row %d Bank %d Col %d Bit %d Data 0x%08x",
+ pinf->row, pinf->bank, pinf->col,
+ pinf->bitpos, pinf->data);
+
+ edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+ p->ce_cnt, 0, 0, 0, 0, 0, -1,
+ priv->message, "");
+ }
+
+ if (p->ue_cnt) {
+ pinf = &p->ueinfo;
+
+ snprintf(priv->message, ZYNQ_EDAC_MSG_SIZE,
+ "Row %d Bank %d Col %d",
+ pinf->row, pinf->bank, pinf->col);
+
+ edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+ p->ue_cnt, 0, 0, 0, 0, 0, -1,
+ priv->message, "");
+ }
+
+ memset(p, 0, sizeof(*p));
+}
+
+/**
+ * check_errors - Check controller for ECC errors.
+ * @mci: EDAC memory controller instance.
+ *
+ * Check and post ECC errors. Called by the polling thread.
+ */
+static void zynq_check_errors(struct mem_ctl_info *mci)
+{
+ struct zynq_edac_priv *priv = mci->pvt_info;
+ int status;
+
+ status = zynq_get_error_info(priv);
+ if (status)
+ return;
+
+ zynq_handle_error(mci, &priv->stat);
+}
+
+/**
+ * zynq_get_dtype - Return the controller memory width.
+ * @base: DDR memory controller base address.
+ *
+ * Get the EDAC device type width appropriate for the current controller
+ * configuration.
+ *
+ * Return: a device type width enumeration.
+ */
+static enum dev_type zynq_get_dtype(const void __iomem *base)
+{
+ enum dev_type dt;
+ u32 width;
+
+ width = readl(base + ZYNQ_CTRL_OFST);
+ width = (width & ZYNQ_CTRL_BW_MASK) >> ZYNQ_CTRL_BW_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_get_ecc_state - Return the controller ECC enable/disable status.
+ * @base: DDR memory controller base address.
+ *
+ * Get the ECC enable/disable status of the controller.
+ *
+ * Return: true if enabled, otherwise false.
+ */
+static bool zynq_get_ecc_state(void __iomem *base)
+{
+ enum dev_type dt;
+ u32 ecctype;
+
+ dt = zynq_get_dtype(base);
+ if (dt == DEV_UNKNOWN)
+ return false;
+
+ ecctype = readl(base + ZYNQ_SCRUB_OFST) & ZYNQ_SCRUB_MODE_MASK;
+ if ((ecctype == ZYNQ_SCRUB_MODE_SECDED) && (dt == DEV_X2))
+ return true;
+
+ return false;
+}
+
+/**
+ * zynq_get_memsize - Read the size of the attached memory device.
+ *
+ * Return: the memory size in bytes.
+ */
+static u32 zynq_get_memsize(void)
+{
+ struct sysinfo inf;
+
+ si_meminfo(&inf);
+
+ return inf.totalram * inf.mem_unit;
+}
+
+/**
+ * zynq_get_mtype - Return the controller memory type.
+ * @base: Zynq ECC status structure.
+ *
+ * Get the EDAC memory type appropriate for the current controller
+ * configuration.
+ *
+ * Return: a memory type enumeration.
+ */
+static enum mem_type zynq_get_mtype(const void __iomem *base)
+{
+ enum mem_type mt;
+ u32 memtype;
+
+ memtype = readl(base + ZYNQ_T_ZQ_OFST);
+
+ if (memtype & ZYNQ_T_ZQ_DDRMODE_MASK)
+ mt = MEM_DDR3;
+ else
+ mt = MEM_DDR2;
+
+ return mt;
+}
+
+/**
+ * zynq_init_csrows - Initialize the csrow data.
+ * @mci: EDAC memory controller instance.
+ *
+ * Initialize the chip select rows associated with the EDAC memory
+ * controller instance.
+ */
+static void zynq_init_csrows(struct mem_ctl_info *mci)
+{
+ struct zynq_edac_priv *priv = mci->pvt_info;
+ struct csrow_info *csi;
+ struct dimm_info *dimm;
+ u32 size, row;
+ int j;
+
+ for (row = 0; row < mci->nr_csrows; row++) {
+ csi = mci->csrows[row];
+ size = zynq_get_memsize();
+
+ for (j = 0; j < csi->nr_channels; j++) {
+ dimm = csi->channels[j]->dimm;
+ dimm->edac_mode = EDAC_SECDED;
+ dimm->mtype = zynq_get_mtype(priv->baseaddr);
+ dimm->nr_pages = (size >> PAGE_SHIFT) / csi->nr_channels;
+ dimm->grain = ZYNQ_EDAC_ERR_GRAIN;
+ dimm->dtype = zynq_get_dtype(priv->baseaddr);
+ }
+ }
+}
+
+/**
+ * zynq_mc_init - Initialize one driver instance.
+ * @mci: EDAC memory controller instance.
+ * @pdev: platform device.
+ *
+ * Perform initialization of the EDAC memory controller instance and
+ * related driver-private data associated with the memory controller the
+ * instance is bound to.
+ */
+static void zynq_mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
+{
+ struct zynq_edac_priv *priv;
+
+ 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_FLAG_HW_SRC;
+ mci->scrub_mode = SCRUB_NONE;
+
+ mci->edac_cap = EDAC_FLAG_SECDED;
+ mci->ctl_name = "zynq_ddr_controller";
+ mci->dev_name = ZYNQ_EDAC_MOD_STRING;
+ mci->mod_name = ZYNQ_EDAC_MOD_VER;
+
+ edac_op_state = EDAC_OPSTATE_POLL;
+ mci->edac_check = zynq_check_errors;
+
+ mci->ctl_page_to_phys = NULL;
+
+ zynq_init_csrows(mci);
+}
+
+/**
+ * zynq_mc_probe - Check controller and bind driver.
+ * @pdev: platform device.
+ *
+ * Probe 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 zynq_mc_probe(struct platform_device *pdev)
+{
+ struct edac_mc_layer layers[2];
+ struct zynq_edac_priv *priv;
+ struct mem_ctl_info *mci;
+ void __iomem *baseaddr;
+ int rc;
+
+ baseaddr = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(baseaddr))
+ return PTR_ERR(baseaddr);
+
+ if (!zynq_get_ecc_state(baseaddr)) {
+ edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
+ return -ENXIO;
+ }
+
+ 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(EDAC_AUTO_MC_NUM, ARRAY_SIZE(layers), layers,
+ sizeof(struct zynq_edac_priv));
+ if (!mci) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Failed memory allocation for mc instance\n");
+ return -ENOMEM;
+ }
+
+ priv = mci->pvt_info;
+ priv->baseaddr = baseaddr;
+
+ zynq_mc_init(mci, pdev);
+
+ rc = edac_mc_add_mc(mci);
+ if (rc) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Failed to register with EDAC core\n");
+ goto free_edac_mc;
+ }
+
+ /*
+ * Start capturing the correctable and uncorrectable errors. A write of
+ * 0 starts the counters.
+ */
+ writel(0x0, baseaddr + ZYNQ_ECC_CTRL_OFST);
+
+ return 0;
+
+free_edac_mc:
+ edac_mc_free(mci);
+
+ return rc;
+}
+
+/**
+ * zynq_mc_remove - Unbind driver from controller.
+ * @pdev: Platform device.
+ *
+ * Return: Unconditionally 0
+ */
+static int zynq_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;
+}
+
+static const struct of_device_id zynq_edac_match[] = {
+ { .compatible = "xlnx,zynq-ddrc-a05" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, zynq_edac_match);
+
+static struct platform_driver zynq_edac_mc_driver = {
+ .driver = {
+ .name = "zynq-edac",
+ .of_match_table = zynq_edac_match,
+ },
+ .probe = zynq_mc_probe,
+ .remove = zynq_mc_remove,
+};
+module_platform_driver(zynq_edac_mc_driver);
+
+MODULE_AUTHOR("Xilinx Inc");
+MODULE_DESCRIPTION("Zynq DDR ECC driver");
+MODULE_LICENSE("GPL v2");
--
2.35.1
DW DDRs CSRs resource descriptor is used by the devm_ioremap_resource()
function invocation only in the driver probe method. Thus we can freely
convert the platform_get_resource() and devm_ioremap_resource() couple to
just a single devm_platform_ioremap_resource() method call.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/edac/synopsys_edac.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 14653b799901..f38c326f2cf5 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -1336,11 +1336,9 @@ static int mc_probe(struct platform_device *pdev)
struct synps_edac_priv *priv;
struct mem_ctl_info *mci;
void __iomem *baseaddr;
- struct resource *res;
int rc;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- baseaddr = devm_ioremap_resource(&pdev->dev, res);
+ baseaddr = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(baseaddr))
return PTR_ERR(baseaddr);
--
2.35.1
In case of the unique index allocation it's not that optimal to always
rely on the low-level device drivers (platform drivers), because they get
to start to implement either the same design pattern (for instance global
static MC counter) or may end-up with having non-unique index eventually
at runtime. Needless to say that having a generic unique index
allocation/tracking procedure will make code more readable and safer.
The suggested implementation is based on the kernel IDA infrastructure
exposed by the lib/idr.c driver with API described in linux/idr.h header
file. It's used to create an ID resource descriptor "mc_idr", which then
is utilized either to track the custom MC idx specified by EDAC LLDDs or
to allocate the next-free MC idx.
A new special MC index is introduced here. It's defined by the
EDAC_AUTO_MC_NUM macro with a value specifically chosen as the least
probable value used for the real MC index. In case if the EDAC_AUTO_MC_NUM
index is specified by the EDAC LLDD, the MC index will be either retrieved
from the MC device OF-node alias index ("mc[:number:]") or automatically
generated as the next-free MC index found by the ID allocation procedure.
Signed-off-by: Serge Semin <[email protected]>
---
Note the approach implemented here has been partly ported from the SPI
core driver using IDA to track/allocate SPI bus numbers.
Link: https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi.c#L2957
---
drivers/edac/edac_mc.c | 89 +++++++++++++++++++++++++++++++++++++++---
drivers/edac/edac_mc.h | 4 ++
2 files changed, 87 insertions(+), 6 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 24814839d885..634c41ea7804 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -29,6 +29,9 @@
#include <linux/edac.h>
#include <linux/bitops.h>
#include <linux/uaccess.h>
+#include <linux/idr.h>
+#include <linux/of.h>
+
#include <asm/page.h>
#include "edac_mc.h"
#include "edac_module.h"
@@ -46,6 +49,7 @@ EXPORT_SYMBOL_GPL(edac_op_state);
/* lock to memory controller's control array */
static DEFINE_MUTEX(mem_ctls_mutex);
static LIST_HEAD(mc_devices);
+static DEFINE_IDR(mc_idr);
/*
* Used to lock EDAC MC to just one module, avoiding two drivers e. g.
@@ -493,7 +497,64 @@ void edac_mc_reset_delay_period(unsigned long value)
mutex_unlock(&mem_ctls_mutex);
}
+/**
+ * edac_mc_alloc_id() - Allocate unique Memory Controller identifier
+ *
+ * @mci: pointer to the mci structure to allocate ID for
+ *
+ * Use edac_mc_free_id() to coherently free the MC identifier.
+ *
+ * .. note::
+ * locking model: must be called with the mem_ctls_mutex lock held
+ *
+ * Returns:
+ * 0 on Success, or an error code on failure
+ */
+static int edac_mc_alloc_id(struct mem_ctl_info *mci)
+{
+ struct device_node *np = dev_of_node(mci->pdev);
+ int ret, min, max;
+
+ if (mci->mc_idx == EDAC_AUTO_MC_NUM) {
+ ret = of_alias_get_id(np, "mc");
+ if (ret >= 0) {
+ min = ret;
+ max = ret + 1;
+ } else {
+ min = of_alias_get_highest_id("mc");
+ if (min >= 0)
+ min++;
+ else
+ min = 0;
+
+ max = 0;
+ }
+ } else {
+ min = mci->mc_idx;
+ max = mci->mc_idx + 1;
+ }
+
+ ret = idr_alloc(&mc_idr, mci, min, max, GFP_KERNEL);
+ if (ret < 0)
+ return ret == -ENOSPC ? -EBUSY : ret;
+
+ mci->mc_idx = ret;
+
+ return 0;
+}
+/**
+ * edac_mc_free_id() - Free Memory Controller identifier
+ *
+ * @mci: pointer to the mci structure to free ID from
+ *
+ * .. note::
+ * locking model: must be called with the mem_ctls_mutex lock held
+ */
+static void edac_mc_free_id(struct mem_ctl_info *mci)
+{
+ idr_remove(&mc_idr, mci->mc_idx);
+}
/**
* edac_mc_init_labels() - Initialize DIMM labels
@@ -612,7 +673,8 @@ EXPORT_SYMBOL_GPL(edac_get_owner);
int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
const struct attribute_group **groups)
{
- int ret = -EINVAL;
+ int ret;
+
edac_dbg(0, "\n");
#ifdef CONFIG_EDAC_DEBUG
@@ -649,20 +711,30 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
goto fail0;
}
+ ret = edac_mc_alloc_id(mci);
+ if (ret) {
+ edac_printk(KERN_ERR, EDAC_MC, "failed to allocate MC idx %u\n",
+ mci->mc_idx);
+ goto fail0;
+ }
+
edac_mc_init_labels(mci);
- if (add_mc_to_global_list(mci))
- goto fail0;
+ if (add_mc_to_global_list(mci)) {
+ ret = -EINVAL;
+ goto fail1;
+ }
/* set load time so that error rate can be tracked */
mci->start_time = jiffies;
mci->bus = edac_get_sysfs_subsys();
- if (edac_create_sysfs_mci_device(mci, groups)) {
+ ret = edac_create_sysfs_mci_device(mci, groups);
+ if (ret) {
edac_mc_printk(mci, KERN_WARNING,
"failed to create sysfs device\n");
- goto fail1;
+ goto fail2;
}
if (mci->edac_check) {
@@ -686,9 +758,12 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
mutex_unlock(&mem_ctls_mutex);
return 0;
-fail1:
+fail2:
del_mc_from_global_list(mci);
+fail1:
+ edac_mc_free_id(mci);
+
fail0:
mutex_unlock(&mem_ctls_mutex);
return ret;
@@ -716,6 +791,8 @@ struct mem_ctl_info *edac_mc_del_mc(struct device *dev)
if (del_mc_from_global_list(mci))
edac_mc_owner = NULL;
+ edac_mc_free_id(mci);
+
mutex_unlock(&mem_ctls_mutex);
if (mci->edac_check)
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index 881b00eadf7a..4b6676235b1b 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -23,6 +23,7 @@
#define _EDAC_MC_H_
#include <linux/kernel.h>
+#include <linux/limits.h>
#include <linux/types.h>
#include <linux/module.h>
#include <linux/spinlock.h>
@@ -37,6 +38,9 @@
#include <linux/workqueue.h>
#include <linux/edac.h>
+/* Generate MC identifier automatically */
+#define EDAC_AUTO_MC_NUM UINT_MAX
+
#if PAGE_SHIFT < 20
#define PAGES_TO_MiB(pages) ((pages) >> (20 - PAGE_SHIFT))
#define MiB_TO_PAGES(mb) ((mb) << (20 - PAGE_SHIFT))
--
2.35.1
The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
no any reason to have these controllers described by the same bindings.
Thus let's split them up.
While at it rename the original Synopsys uMCTL2 DT-schema file to a more
descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
description of the device bindings.
Signed-off-by: Serge Semin <[email protected]>
---
.../snps,dw-umctl2-ddrc.yaml | 51 +++++++++++++
.../memory-controllers/synopsys,ddrc-ecc.yaml | 76 -------------------
.../xlnx,zynq-ddrc-a05.yaml | 38 ++++++++++
MAINTAINERS | 2 +
4 files changed, 91 insertions(+), 76 deletions(-)
create mode 100644 Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
delete mode 100644 Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
create mode 100644 Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml
diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
new file mode 100644
index 000000000000..787d91d64eee
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DesignWare Universal Multi-Protocol Memory Controller
+
+maintainers:
+ - Krzysztof Kozlowski <[email protected]>
+ - Manish Narani <[email protected]>
+ - Michal Simek <[email protected]>
+
+description: |
+ Synopsys DesignWare Enhanced uMCTL2 DDR Memory Controller is cappable of
+ working with DDR devices up to (LP)DDR4 protocol. It can be equipped
+ with SEC/DEC ECC feature if DRAM data bus width is either 16-bits or
+ 32-bits or 64-bits wide.
+
+ The ZynqMP DDR controller is based on the DW uMCTL2 v2.40a controller.
+ It has an optional SEC/DEC ECC support in 64-bit and 32-bit bus width
+ configurations.
+
+properties:
+ compatible:
+ enum:
+ - snps,ddrc-3.80a
+ - xlnx,zynqmp-ddrc-2.40a
+
+ interrupts:
+ maxItems: 1
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ memory-controller@fd070000 {
+ compatible = "xlnx,zynqmp-ddrc-2.40a";
+ reg = <0xfd070000 0x30000>;
+ interrupt-parent = <&gic>;
+ interrupts = <0 112 4>;
+ };
+...
diff --git a/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml b/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
deleted file mode 100644
index f46e95704f53..000000000000
--- a/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
+++ /dev/null
@@ -1,76 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0-only
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/memory-controllers/synopsys,ddrc-ecc.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: Synopsys IntelliDDR Multi Protocol memory controller
-
-maintainers:
- - Krzysztof Kozlowski <[email protected]>
- - Manish Narani <[email protected]>
- - Michal Simek <[email protected]>
-
-description: |
- The ZynqMP DDR ECC controller has an optional ECC support in 64-bit and
- 32-bit bus width configurations.
-
- The Zynq DDR ECC controller has an optional ECC support in half-bus width
- (16-bit) configuration.
-
- These both ECC controllers correct single bit ECC errors and detect double bit
- ECC errors.
-
-properties:
- compatible:
- enum:
- - snps,ddrc-3.80a
- - xlnx,zynq-ddrc-a05
- - xlnx,zynqmp-ddrc-2.40a
-
- interrupts:
- maxItems: 1
-
- reg:
- maxItems: 1
-
-required:
- - compatible
- - reg
-
-allOf:
- - if:
- properties:
- compatible:
- contains:
- enum:
- - snps,ddrc-3.80a
- - xlnx,zynqmp-ddrc-2.40a
- then:
- required:
- - interrupts
- else:
- properties:
- interrupts: false
-
-additionalProperties: false
-
-examples:
- - |
- memory-controller@f8006000 {
- compatible = "xlnx,zynq-ddrc-a05";
- reg = <0xf8006000 0x1000>;
- };
-
- - |
- axi {
- #address-cells = <2>;
- #size-cells = <2>;
-
- memory-controller@fd070000 {
- compatible = "xlnx,zynqmp-ddrc-2.40a";
- reg = <0x0 0xfd070000 0x0 0x30000>;
- interrupt-parent = <&gic>;
- interrupts = <0 112 4>;
- };
- };
diff --git a/Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml b/Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml
new file mode 100644
index 000000000000..8f72e2f8588a
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/xlnx,zynq-ddrc-a05.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Zynq A05 DDR Memory Controller
+
+maintainers:
+ - Krzysztof Kozlowski <[email protected]>
+ - Manish Narani <[email protected]>
+ - Michal Simek <[email protected]>
+
+description:
+ The Zynq DDR ECC controller has an optional ECC support in half-bus width
+ (16-bit) configuration. It is cappable of correcting single bit ECC errors
+ and detecting double bit ECC errors.
+
+properties:
+ compatible:
+ const: xlnx,zynq-ddrc-a05
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ memory-controller@f8006000 {
+ compatible = "xlnx,zynq-ddrc-a05";
+ reg = <0xf8006000 0x1000>;
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 9d7f64dc0efe..17f23f810ee4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3086,7 +3086,9 @@ S: Supported
W: http://wiki.xilinx.com
T: git https://github.com/Xilinx/linux-xlnx.git
F: Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
+F: Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
F: Documentation/devicetree/bindings/i2c/xlnx,xps-iic-2.00.a.yaml
+F: Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml
F: Documentation/devicetree/bindings/spi/xlnx,zynq-qspi.yaml
F: arch/arm/mach-zynq/
F: drivers/clocksource/timer-cadence-ttc.c
--
2.35.1
The duplicated edac_dbg()-based dimm->nr_pages field was introduced in
commit 6e84d359b2be ("edac_mc: Cleanup per-dimm_info debug messages"). You
can find the duplicated line even in the commit message text:
> [ 1011.380101] EDAC DEBUG: edac_mc_dump_dimm: dimm->nr_pages = 0x40000
> [ 1011.380103] EDAC DEBUG: edac_mc_dump_dimm: dimm->grain = 8
> [ 1011.380104] EDAC DEBUG: edac_mc_dump_dimm: dimm->nr_pages = 0x40000
Let's drop the corresponding edac_dbg() method call then.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/edac/edac_mc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index eb58644bb019..6faeb2ab3960 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -103,7 +103,6 @@ static void edac_mc_dump_dimm(struct dimm_info *dimm)
edac_dbg(4, " dimm->label = '%s'\n", dimm->label);
edac_dbg(4, " dimm->nr_pages = 0x%x\n", dimm->nr_pages);
edac_dbg(4, " dimm->grain = %d\n", dimm->grain);
- edac_dbg(4, " dimm->nr_pages = 0x%x\n", dimm->nr_pages);
}
static void edac_mc_dump_csrow(struct csrow_info *csrow)
--
2.35.1
Currently the memory type macros are partly defined with the multiple
spaces between the macro name and its definition. Let's replace the spaces
with tabs as the kernel coding style requires.
Signed-off-by: Serge Semin <[email protected]>
---
include/linux/edac.h | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/include/linux/edac.h b/include/linux/edac.h
index e730b3468719..fa4bda2a70f6 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -231,21 +231,21 @@ enum mem_type {
#define MEM_FLAG_DDR BIT(MEM_DDR)
#define MEM_FLAG_RDDR BIT(MEM_RDDR)
#define MEM_FLAG_RMBS BIT(MEM_RMBS)
-#define MEM_FLAG_DDR2 BIT(MEM_DDR2)
-#define MEM_FLAG_FB_DDR2 BIT(MEM_FB_DDR2)
-#define MEM_FLAG_RDDR2 BIT(MEM_RDDR2)
-#define MEM_FLAG_XDR BIT(MEM_XDR)
-#define MEM_FLAG_DDR3 BIT(MEM_DDR3)
-#define MEM_FLAG_RDDR3 BIT(MEM_RDDR3)
-#define MEM_FLAG_LPDDR3 BIT(MEM_LPDDR3)
-#define MEM_FLAG_DDR4 BIT(MEM_DDR4)
-#define MEM_FLAG_RDDR4 BIT(MEM_RDDR4)
-#define MEM_FLAG_LRDDR4 BIT(MEM_LRDDR4)
-#define MEM_FLAG_LPDDR4 BIT(MEM_LPDDR4)
-#define MEM_FLAG_DDR5 BIT(MEM_DDR5)
-#define MEM_FLAG_RDDR5 BIT(MEM_RDDR5)
-#define MEM_FLAG_LRDDR5 BIT(MEM_LRDDR5)
-#define MEM_FLAG_NVDIMM BIT(MEM_NVDIMM)
+#define MEM_FLAG_DDR2 BIT(MEM_DDR2)
+#define MEM_FLAG_FB_DDR2 BIT(MEM_FB_DDR2)
+#define MEM_FLAG_RDDR2 BIT(MEM_RDDR2)
+#define MEM_FLAG_XDR BIT(MEM_XDR)
+#define MEM_FLAG_DDR3 BIT(MEM_DDR3)
+#define MEM_FLAG_RDDR3 BIT(MEM_RDDR3)
+#define MEM_FLAG_LPDDR3 BIT(MEM_LPDDR3)
+#define MEM_FLAG_DDR4 BIT(MEM_DDR4)
+#define MEM_FLAG_RDDR4 BIT(MEM_RDDR4)
+#define MEM_FLAG_LRDDR4 BIT(MEM_LRDDR4)
+#define MEM_FLAG_LPDDR4 BIT(MEM_LPDDR4)
+#define MEM_FLAG_DDR5 BIT(MEM_DDR5)
+#define MEM_FLAG_RDDR5 BIT(MEM_RDDR5)
+#define MEM_FLAG_LRDDR5 BIT(MEM_LRDDR5)
+#define MEM_FLAG_NVDIMM BIT(MEM_NVDIMM)
#define MEM_FLAG_WIO2 BIT(MEM_WIO2)
#define MEM_FLAG_HBM2 BIT(MEM_HBM2)
--
2.35.1
Aside with fixing the errors count CSR usage the commit e2932d1f6f05
("EDAC/synopsys: Read the error count from the correct register") all of
the sudden has also changed the order of the errors status check
procedure. So now the errors handler method first reads the number of CE
and UE and only then makes sure that any of these errors have actually
happened. It doesn't make much sense. Let's fix that by getting back the
procedures order: first check the ECC status, then read the number of
errors.
Fixes: e2932d1f6f05 ("EDAC/synopsys: Read the error count from the correct register")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/edac/synopsys_edac.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 873dbc684fe6..14653b799901 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -422,18 +422,18 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
base = priv->baseaddr;
p = &priv->stat;
- regval = readl(base + ECC_ERRCNT_OFST);
- p->ce_cnt = regval & ECC_ERRCNT_CECNT_MASK;
- p->ue_cnt = (regval & ECC_ERRCNT_UECNT_MASK) >> ECC_ERRCNT_UECNT_SHIFT;
- if (!p->ce_cnt)
- goto ue_err;
-
regval = readl(base + ECC_STAT_OFST);
if (!regval)
return 1;
p->ceinfo.bitpos = (regval & ECC_STAT_BITNUM_MASK);
+ regval = readl(base + ECC_ERRCNT_OFST);
+ p->ce_cnt = regval & ECC_ERRCNT_CECNT_MASK;
+ p->ue_cnt = (regval & ECC_ERRCNT_UECNT_MASK) >> ECC_ERRCNT_UECNT_SHIFT;
+ if (!p->ce_cnt)
+ goto ue_err;
+
regval = readl(base + ECC_CEADDR0_OFST);
p->ceinfo.row = (regval & ECC_CEADDR0_RW_MASK);
regval = readl(base + ECC_CEADDR1_OFST);
--
2.35.1
On 22/08/2022 22:07, Serge Semin wrote:
> -
> -allOf:
> - - if:
> - properties:
> - compatible:
> - contains:
> - enum:
> - - snps,ddrc-3.80a
> - - xlnx,zynqmp-ddrc-2.40a
> - then:
> - required:
> - - interrupts
> - else:
> - properties:
> - interrupts: false
This is not a pure "rename"... Organize your patches so only one thing
is done at a time. Rename does not change the functional parts of the
binding...
Best regards,
Krzysztof
On 23/08/2022 11:27, Serge Semin wrote:
> On Tue, Aug 23, 2022 at 11:22:08AM +0300, Krzysztof Kozlowski wrote:
>> On 23/08/2022 11:17, Krzysztof Kozlowski wrote:
>>> On 22/08/2022 22:07, Serge Semin wrote:
>>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
>>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
>>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
>>>> no any reason to have these controllers described by the same bindings.
>>>> Thus let's split them up.
>>>>
>>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
>>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
>>>> description of the device bindings.
>>>
>>> Filename should be based on compatible, so if renaming then
>>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
>>> filename anyway. Therefore nack for rename.
>>>
>>> BTW, if you perform renames, generate patches with proper -M/-C/-B
>>> arguments so this is detected.
>>>
>>>
>>>>
>>>> Signed-off-by: Serge Semin <[email protected]>
>>>> ---> .../snps,dw-umctl2-ddrc.yaml | 51 +++++++++++++
>>>
>>> This is a mess. I did not get any cover letters, any other patches any
>>> description of relation between this and your other one.
>>>
>>> It seems you make independent and conflicting changes to the same file,
>>> so this has to be properly organized.
>>>
>>> Send entire patchset with cover letter with description of all
>>> dependencies to all maintainers.
>>>
>>> This is unreviewable now, so a no.
>>
>
>> And also untestable by Rob's bot, so will have to wait.
>
> For what reason it's untestable? The patch has no dependencies from
> any other patchset.
This one is testable, but the next one is not, because it depends on
something. I don't see the reason to split the bindings between
different patchsets.
Best regards,
Krzysztof
On 23/08/2022 11:17, Krzysztof Kozlowski wrote:
> On 22/08/2022 22:07, Serge Semin wrote:
>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
>> no any reason to have these controllers described by the same bindings.
>> Thus let's split them up.
>>
>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
>> description of the device bindings.
>
> Filename should be based on compatible, so if renaming then
> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> filename anyway. Therefore nack for rename.
>
> BTW, if you perform renames, generate patches with proper -M/-C/-B
> arguments so this is detected.
>
>
>>
>> Signed-off-by: Serge Semin <[email protected]>
>> ---> .../snps,dw-umctl2-ddrc.yaml | 51 +++++++++++++
>
> This is a mess. I did not get any cover letters, any other patches any
> description of relation between this and your other one.
>
> It seems you make independent and conflicting changes to the same file,
> so this has to be properly organized.
>
> Send entire patchset with cover letter with description of all
> dependencies to all maintainers.
>
> This is unreviewable now, so a no.
And also untestable by Rob's bot, so will have to wait.
I found the cover letters, somehow they got buried in inbox.
Best regards,
Krzysztof
On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
> On 22/08/2022 22:07, Serge Semin wrote:
> > The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> > the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> > uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> > no any reason to have these controllers described by the same bindings.
> > Thus let's split them up.
> >
> > While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> > descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> > description of the device bindings.
>
> Filename should be based on compatible, so if renaming then
> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> filename anyway. Therefore nack for rename.
New requirement? I've submitted not a single patch to the DT-bindings
sources and didn't get any comment from Rob about that. In addition
There are DT bindings with names different from what is defined in the
compatible name. Moreover there are tons of bindings with various
compatible names. What name to choose then? Finally the current name
is too generic to use for actual DW uMCTL2 DDRC controller.
-Sergey
>
> BTW, if you perform renames, generate patches with proper -M/-C/-B
> arguments so this is detected.
>
>
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > ---> .../snps,dw-umctl2-ddrc.yaml | 51 +++++++++++++
>
> This is a mess. I did not get any cover letters, any other patches any
> description of relation between this and your other one.
>
> It seems you make independent and conflicting changes to the same file,
> so this has to be properly organized.
>
> Send entire patchset with cover letter with description of all
> dependencies to all maintainers.
>
> This is unreviewable now, so a no.
>
> Best regards,
> Krzysztof
On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
> On 22/08/2022 22:07, Serge Semin wrote:
> > The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> > the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> > uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> > no any reason to have these controllers described by the same bindings.
> > Thus let's split them up.
> >
> > While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> > descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> > description of the device bindings.
>
> Filename should be based on compatible, so if renaming then
> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> filename anyway. Therefore nack for rename.
>
> BTW, if you perform renames, generate patches with proper -M/-C/-B
> arguments so this is detected.
>
>
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > ---> .../snps,dw-umctl2-ddrc.yaml | 51 +++++++++++++
>
> This is a mess. I did not get any cover letters, any other patches any
> description of relation between this and your other one.
>
> It seems you make independent and conflicting changes to the same file,
> so this has to be properly organized.
Don't hurry with the judgement. Have a better look at your DT-related
inbox.
Link: https://lore.kernel.org/linux-devicetree/[email protected]/
-Sergey
>
> Send entire patchset with cover letter with description of all
> dependencies to all maintainers.
>
> This is unreviewable now, so a no.
>
> Best regards,
> Krzysztof
On 22/08/2022 22:07, Serge Semin wrote:
> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> no any reason to have these controllers described by the same bindings.
> Thus let's split them up.
>
> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> description of the device bindings.
Filename should be based on compatible, so if renaming then
snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
filename anyway. Therefore nack for rename.
BTW, if you perform renames, generate patches with proper -M/-C/-B
arguments so this is detected.
>
> Signed-off-by: Serge Semin <[email protected]>
> ---> .../snps,dw-umctl2-ddrc.yaml | 51 +++++++++++++
This is a mess. I did not get any cover letters, any other patches any
description of relation between this and your other one.
It seems you make independent and conflicting changes to the same file,
so this has to be properly organized.
Send entire patchset with cover letter with description of all
dependencies to all maintainers.
This is unreviewable now, so a no.
Best regards,
Krzysztof
On 23/08/2022 11:32, Serge Semin wrote:
> On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
>> On 22/08/2022 22:07, Serge Semin wrote:
>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
>>> no any reason to have these controllers described by the same bindings.
>>> Thus let's split them up.
>>>
>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
>>> description of the device bindings.
>>
>
>> Filename should be based on compatible, so if renaming then
>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
>> filename anyway. Therefore nack for rename.
>
> New requirement? I've submitted not a single patch to the DT-bindings
> sources and didn't get any comment from Rob about that.
This is not a new requirement. It has been since some time and Rob gave
such reviews.
https://lore.kernel.org/linux-devicetree/[email protected]/
For devices with multiple compatibles that's a bit tricky, but assuming
the bindings describe both original design from Synopsys and it's
implementations, then something closer to Synopsys makes sense.
> In addition
> There are DT bindings with names different from what is defined in the
> compatible name. Moreover there are tons of bindings with various
> compatible names. What name to choose then? Finally the current name
> is too generic to use for actual DW uMCTL2 DDRC controller.
There are thousands of bugs, inconsistencies, naming differences in
kernel. I don't find these as arguments to repeat the practice...so the
bindings file name should be based on the compatible.
Best regards,
Krzysztof
On Tue, Aug 23, 2022 at 11:22:08AM +0300, Krzysztof Kozlowski wrote:
> On 23/08/2022 11:17, Krzysztof Kozlowski wrote:
> > On 22/08/2022 22:07, Serge Semin wrote:
> >> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> >> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> >> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> >> no any reason to have these controllers described by the same bindings.
> >> Thus let's split them up.
> >>
> >> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> >> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> >> description of the device bindings.
> >
> > Filename should be based on compatible, so if renaming then
> > snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> > filename anyway. Therefore nack for rename.
> >
> > BTW, if you perform renames, generate patches with proper -M/-C/-B
> > arguments so this is detected.
> >
> >
> >>
> >> Signed-off-by: Serge Semin <[email protected]>
> >> ---> .../snps,dw-umctl2-ddrc.yaml | 51 +++++++++++++
> >
> > This is a mess. I did not get any cover letters, any other patches any
> > description of relation between this and your other one.
> >
> > It seems you make independent and conflicting changes to the same file,
> > so this has to be properly organized.
> >
> > Send entire patchset with cover letter with description of all
> > dependencies to all maintainers.
> >
> > This is unreviewable now, so a no.
>
> And also untestable by Rob's bot, so will have to wait.
For what reason it's untestable? The patch has no dependencies from
any other patchset.
-Sergey
>
> I found the cover letters, somehow they got buried in inbox.
>
> Best regards,
> Krzysztof
On Tue, Aug 23, 2022 at 11:30:22AM +0300, Krzysztof Kozlowski wrote:
> On 23/08/2022 11:27, Serge Semin wrote:
> > On Tue, Aug 23, 2022 at 11:22:08AM +0300, Krzysztof Kozlowski wrote:
> >> On 23/08/2022 11:17, Krzysztof Kozlowski wrote:
> >>> On 22/08/2022 22:07, Serge Semin wrote:
> >>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> >>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> >>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> >>>> no any reason to have these controllers described by the same bindings.
> >>>> Thus let's split them up.
> >>>>
> >>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> >>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> >>>> description of the device bindings.
> >>>
> >>> Filename should be based on compatible, so if renaming then
> >>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> >>> filename anyway. Therefore nack for rename.
> >>>
> >>> BTW, if you perform renames, generate patches with proper -M/-C/-B
> >>> arguments so this is detected.
> >>>
> >>>
> >>>>
> >>>> Signed-off-by: Serge Semin <[email protected]>
> >>>> ---> .../snps,dw-umctl2-ddrc.yaml | 51 +++++++++++++
> >>>
> >>> This is a mess. I did not get any cover letters, any other patches any
> >>> description of relation between this and your other one.
> >>>
> >>> It seems you make independent and conflicting changes to the same file,
> >>> so this has to be properly organized.
> >>>
> >>> Send entire patchset with cover letter with description of all
> >>> dependencies to all maintainers.
> >>>
> >>> This is unreviewable now, so a no.
> >>
> >
> >> And also untestable by Rob's bot, so will have to wait.
> >
> > For what reason it's untestable? The patch has no dependencies from
> > any other patchset.
>
> This one is testable, but the next one is not, because it depends on
> something. I don't see the reason to split the bindings between
> different patchsets.
Really, do you want me to collect all 55 patches in a single patchset?
Please read the cover letter more carefully. And please don't hurry
with your judgement before nacking here and there.
-Sergey
>
> Best regards,
> Krzysztof
On 23/08/2022 14:45, Serge Semin wrote:
> On Tue, Aug 23, 2022 at 11:44:16AM +0300, Krzysztof Kozlowski wrote:
>> On 23/08/2022 11:32, Serge Semin wrote:
>>> On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
>>>> On 22/08/2022 22:07, Serge Semin wrote:
>>>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
>>>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
>>>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
>>>>> no any reason to have these controllers described by the same bindings.
>>>>> Thus let's split them up.
>>>>>
>>>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
>>>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
>>>>> description of the device bindings.
>>>>
>>>
>
>>>> Filename should be based on compatible, so if renaming then
>>>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
>>>> filename anyway. Therefore nack for rename.
>
> Original name was synopsys,ddrc-ecc.yaml which doesn't match any of
> the compatible strings.
>
>>>
>>> New requirement? I've submitted not a single patch to the DT-bindings
>>> sources and didn't get any comment from Rob about that.
>>
>
>> This is not a new requirement. It has been since some time and Rob gave
>> such reviews.
>>
>> https://lore.kernel.org/linux-devicetree/[email protected]/
>
> April 2022. So it's new. It would be nice to have it defined somewhere
> in docs (writing-bindings.rst?). So does the compatibles order (this
> was surprising to me too).
>
>>
>> For devices with multiple compatibles that's a bit tricky, but assuming
>> the bindings describe both original design from Synopsys and it's
>> implementations, then something closer to Synopsys makes sense.
>
> The closest name would be snps,dw-umctl2-ddrc.yaml. snps,ddrc is too
> generic especially for the IP-cores vendor. It doesn't have a
> reference to the actual IP-core the device in subject is based on.
You are aware that in such case mistake is not in the file name but in
the compatible?
>
>>
>>
>>> In addition
>>> There are DT bindings with names different from what is defined in the
>>> compatible name. Moreover there are tons of bindings with various
>>> compatible names. What name to choose then? Finally the current name
>>> is too generic to use for actual DW uMCTL2 DDRC controller.
>>
>
>> There are thousands of bugs, inconsistencies, naming differences in
>> kernel. I don't find these as arguments to repeat the practice...so the
>> bindings file name should be based on the compatible.
>
> Did I ask for an exception? I justified why the renaming was necessary. You
> said it goes against the practice of having the DT-schema named as the
> device compatible strings and just nacked. But above in this message you said
>
>> "assuming the bindings describe both original design from Synopsys
>> and it's implementations, then something closer to Synopsys makes sense"
>
> What I suggest makes more sense than some abstract Synopsys DDRC,
> which may refer to a Synopsys DDR controller other than the subject
> one. So I see two solutions here:
> 1. Adding a new generic compatible string like "snps,dw-umctl2-ddrc"
> and deprecate the "snps,ddrc-3.80a".
This might be good idea, although unfortunately replacing compatibles
takes quite a lot of time if you do not want to break any out-of-tree
users (read: other users of bindings).
> It gets to be even more justified
> seeing the Synopsys IP-core version has been exported in the device
> CSRs since IP-core v3.20a. So having the version attached to the
> compatible string was absolutely redundant.
The version might have sense in a way to differentiate from some older
versions, pre 3.20a. The binding was probably incomplete anyway.
> 2. Just deprecate the generic compatible string, the new compatible
> devices will be supposed to use a vendor-specific compatible strings,
> but still rename the DT-bindings file. This makes sense since the
> current generic name isn't quiet well structured. It' prefix-part is
> too generic and at the same time it refers to a device reversion for
> no much reason.
You mean disallow entirely "snps,ddrc-3.80a" and expect everyone to use
device/implementation specific compatible? I guess this depends whether
this custom block can be used without vendor specific addons. For
several other DW blocks we expect to have the generic snsp fallback and
this generic fallback can be used alone in Linux (pcie-designware-plat.c
binds to it).
Here the Linux driver also binds to generic synopsys compatible, so I
would assume it has a meaning and use case on its own.
>
> What do you think?
>
> * Note I've got it you'd prefer the renaming being performed in a
> separate patch.
The rename could be in the split patch as here, but then I assume the
rename part to be detected by git and be a pure rename. However:
1. The git did not mark it as rename (you might need to use custom
arguments to -M/-B/-C),
2. There were also changes in the process (allOf:if:then).
Best regards,
Krzysztof
On Tue, Aug 23, 2022 at 11:44:16AM +0300, Krzysztof Kozlowski wrote:
> On 23/08/2022 11:32, Serge Semin wrote:
> > On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
> >> On 22/08/2022 22:07, Serge Semin wrote:
> >>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> >>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> >>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> >>> no any reason to have these controllers described by the same bindings.
> >>> Thus let's split them up.
> >>>
> >>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> >>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> >>> description of the device bindings.
> >>
> >
> >> Filename should be based on compatible, so if renaming then
> >> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> >> filename anyway. Therefore nack for rename.
Original name was synopsys,ddrc-ecc.yaml which doesn't match any of
the compatible strings.
> >
> > New requirement? I've submitted not a single patch to the DT-bindings
> > sources and didn't get any comment from Rob about that.
>
> This is not a new requirement. It has been since some time and Rob gave
> such reviews.
>
> https://lore.kernel.org/linux-devicetree/[email protected]/
April 2022. So it's new. It would be nice to have it defined somewhere
in docs (writing-bindings.rst?). So does the compatibles order (this
was surprising to me too).
>
> For devices with multiple compatibles that's a bit tricky, but assuming
> the bindings describe both original design from Synopsys and it's
> implementations, then something closer to Synopsys makes sense.
The closest name would be snps,dw-umctl2-ddrc.yaml. snps,ddrc is too
generic especially for the IP-cores vendor. It doesn't have a
reference to the actual IP-core the device in subject is based on.
>
>
> > In addition
> > There are DT bindings with names different from what is defined in the
> > compatible name. Moreover there are tons of bindings with various
> > compatible names. What name to choose then? Finally the current name
> > is too generic to use for actual DW uMCTL2 DDRC controller.
>
> There are thousands of bugs, inconsistencies, naming differences in
> kernel. I don't find these as arguments to repeat the practice...so the
> bindings file name should be based on the compatible.
Did I ask for an exception? I justified why the renaming was necessary. You
said it goes against the practice of having the DT-schema named as the
device compatible strings and just nacked. But above in this message you said
> "assuming the bindings describe both original design from Synopsys
> and it's implementations, then something closer to Synopsys makes sense"
What I suggest makes more sense than some abstract Synopsys DDRC,
which may refer to a Synopsys DDR controller other than the subject
one. So I see two solutions here:
1. Adding a new generic compatible string like "snps,dw-umctl2-ddrc"
and deprecate the "snps,ddrc-3.80a". It gets to be even more justified
seeing the Synopsys IP-core version has been exported in the device
CSRs since IP-core v3.20a. So having the version attached to the
compatible string was absolutely redundant.
2. Just deprecate the generic compatible string, the new compatible
devices will be supposed to use a vendor-specific compatible strings,
but still rename the DT-bindings file. This makes sense since the
current generic name isn't quiet well structured. It' prefix-part is
too generic and at the same time it refers to a device reversion for
no much reason.
What do you think?
* Note I've got it you'd prefer the renaming being performed in a
separate patch.
-Sergey
>
> Best regards,
> Krzysztof
Hi Serge,
I love your patch! Perhaps something to improve:
[auto build test WARNING on ras/edac-for-next]
[also build test WARNING on robh/for-next linus/master v6.0-rc2 next-20220823]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/EDAC-mc-synopsys-Various-fixes-and-cleanups/20220823-031020
base: https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git edac-for-next
config: arm-buildonly-randconfig-r006-20220823 (https://download.01.org/0day-ci/archive/20220824/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/ec4a1d5280959bf5c6ff8d63006c62d54e01f9cc
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Serge-Semin/EDAC-mc-synopsys-Various-fixes-and-cleanups/20220823-031020
git checkout ec4a1d5280959bf5c6ff8d63006c62d54e01f9cc
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/edac/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/edac/synopsys_edac.c: In function 'synps_get_dtype':
drivers/edac/synopsys_edac.c:485:18: error: implicit declaration of function 'FIELD_GET'; did you mean 'FOLL_GET'? [-Werror=implicit-function-declaration]
485 | regval = FIELD_GET(DDR_MSTR_DEV_CFG_MASK, regval);
| ^~~~~~~~~
| FOLL_GET
drivers/edac/synopsys_edac.c: In function 'mc_init':
>> drivers/edac/synopsys_edac.c:600:33: warning: variable 'priv' set but not used [-Wunused-but-set-variable]
600 | struct synps_edac_priv *priv;
| ^~~~
cc1: some warnings being treated as errors
vim +/priv +600 drivers/edac/synopsys_edac.c
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 588
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 589 /**
225af74d6312b2a Manish Narani 2018-10-04 590 * mc_init - Initialize one driver instance.
225af74d6312b2a Manish Narani 2018-10-04 591 * @mci: EDAC memory controller instance.
225af74d6312b2a Manish Narani 2018-10-04 592 * @pdev: platform device.
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 593 *
225af74d6312b2a Manish Narani 2018-10-04 594 * Perform initialization of the EDAC memory controller instance and
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 595 * related driver-private data associated with the memory controller the
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 596 * instance is bound to.
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 597 */
fa9f6b9e1cf9620 Manish Narani 2018-10-04 598 static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 599 {
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 @600 struct synps_edac_priv *priv;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 601
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 602 mci->pdev = &pdev->dev;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 603 priv = mci->pvt_info;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 604 platform_set_drvdata(pdev, mci);
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 605
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 606 /* Initialize controller capabilities and configuration */
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 607 mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 608 mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
5cfa57cc8481722 Serge Semin 2022-08-22 609 mci->scrub_cap = SCRUB_FLAG_HW_SRC;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 610 mci->scrub_mode = SCRUB_NONE;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 611
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 612 mci->edac_cap = EDAC_FLAG_SECDED;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 613 mci->ctl_name = "synps_ddr_controller";
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 614 mci->dev_name = SYNPS_EDAC_MOD_STRING;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 615 mci->mod_name = SYNPS_EDAC_MOD_VER;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 616
b500b4a029d577c Manish Narani 2018-10-25 617 edac_op_state = EDAC_OPSTATE_INT;
b500b4a029d577c Manish Narani 2018-10-25 618
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 619 mci->ctl_page_to_phys = NULL;
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 620
fa9f6b9e1cf9620 Manish Narani 2018-10-04 621 init_csrows(mci);
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 622 }
ae9b56e3996dadb Punnaiah Choudary Kalluri 2015-01-06 623
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Serge,
I love your patch! Yet something to improve:
[auto build test ERROR on ras/edac-for-next]
[also build test ERROR on robh/for-next linus/master v6.0-rc2 next-20220823]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/EDAC-mc-synopsys-Various-fixes-and-cleanups/20220823-031020
base: https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git edac-for-next
config: arm-buildonly-randconfig-r006-20220823 (https://download.01.org/0day-ci/archive/20220824/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/9c500fa8dbd08021742d8f5a48403c8f6292741c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Serge-Semin/EDAC-mc-synopsys-Various-fixes-and-cleanups/20220823-031020
git checkout 9c500fa8dbd08021742d8f5a48403c8f6292741c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/edac/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/edac/synopsys_edac.c: In function 'snps_enable_irq':
>> drivers/edac/synopsys_edac.c:398:36: error: 'SYNPS_ZYNQMP_IRQ_REGS' undeclared (first use in this function); did you mean 'SNPS_ZYNQMP_IRQ_REGS'?
398 | if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS) {
| ^~~~~~~~~~~~~~~~~~~~~
| SNPS_ZYNQMP_IRQ_REGS
drivers/edac/synopsys_edac.c:398:36: note: each undeclared identifier is reported only once for each function it appears in
drivers/edac/synopsys_edac.c: In function 'snps_disable_irq':
drivers/edac/synopsys_edac.c:419:36: error: 'SYNPS_ZYNQMP_IRQ_REGS' undeclared (first use in this function); did you mean 'SNPS_ZYNQMP_IRQ_REGS'?
419 | if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS) {
| ^~~~~~~~~~~~~~~~~~~~~
| SNPS_ZYNQMP_IRQ_REGS
drivers/edac/synopsys_edac.c: In function 'snps_get_dtype':
drivers/edac/synopsys_edac.c:485:18: error: implicit declaration of function 'FIELD_GET'; did you mean 'FOLL_GET'? [-Werror=implicit-function-declaration]
485 | regval = FIELD_GET(DDR_MSTR_DEV_CFG_MASK, regval);
| ^~~~~~~~~
| FOLL_GET
drivers/edac/synopsys_edac.c: In function 'snps_mc_init':
drivers/edac/synopsys_edac.c:600:32: warning: variable 'priv' set but not used [-Wunused-but-set-variable]
600 | struct snps_edac_priv *priv;
| ^~~~
cc1: some warnings being treated as errors
vim +398 drivers/edac/synopsys_edac.c
ae9b56e3996dad Punnaiah Choudary Kalluri 2015-01-06 392
9c500fa8dbd080 Serge Semin 2022-08-22 393 static void snps_enable_irq(struct snps_edac_priv *priv)
4bcffe941758ee Sherry Sun 2022-04-27 394 {
b8ee875dfc73be Serge Semin 2022-08-22 395 unsigned long flags;
b8ee875dfc73be Serge Semin 2022-08-22 396
4bcffe941758ee Sherry Sun 2022-04-27 397 /* Enable UE/CE Interrupts */
b8ee875dfc73be Serge Semin 2022-08-22 @398 if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS) {
4bcffe941758ee Sherry Sun 2022-04-27 399 writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
4bcffe941758ee Sherry Sun 2022-04-27 400 priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
4bcffe941758ee Sherry Sun 2022-04-27 401
b8ee875dfc73be Serge Semin 2022-08-22 402 return;
b8ee875dfc73be Serge Semin 2022-08-22 403 }
b8ee875dfc73be Serge Semin 2022-08-22 404
b8ee875dfc73be Serge Semin 2022-08-22 405 /* IRQs Enable/Disable feature has been available since v3.10a */
b8ee875dfc73be Serge Semin 2022-08-22 406 spin_lock_irqsave(&priv->lock, flags);
b8ee875dfc73be Serge Semin 2022-08-22 407
b8ee875dfc73be Serge Semin 2022-08-22 408 writel(ECC_CTRL_EN_CE_IRQ | ECC_CTRL_EN_UE_IRQ,
b8ee875dfc73be Serge Semin 2022-08-22 409 priv->baseaddr + ECC_CLR_OFST);
b8ee875dfc73be Serge Semin 2022-08-22 410
b8ee875dfc73be Serge Semin 2022-08-22 411 spin_unlock_irqrestore(&priv->lock, flags);
4bcffe941758ee Sherry Sun 2022-04-27 412 }
4bcffe941758ee Sherry Sun 2022-04-27 413
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, Aug 23, 2022 at 03:03:51PM +0300, Krzysztof Kozlowski wrote:
> On 23/08/2022 14:45, Serge Semin wrote:
> > On Tue, Aug 23, 2022 at 11:44:16AM +0300, Krzysztof Kozlowski wrote:
> >> On 23/08/2022 11:32, Serge Semin wrote:
> >>> On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
> >>>> On 22/08/2022 22:07, Serge Semin wrote:
> >>>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> >>>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> >>>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> >>>>> no any reason to have these controllers described by the same bindings.
> >>>>> Thus let's split them up.
> >>>>>
> >>>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> >>>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> >>>>> description of the device bindings.
> >>>>
> >>>
> >
> >>>> Filename should be based on compatible, so if renaming then
> >>>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> >>>> filename anyway. Therefore nack for rename.
> >
> > Original name was synopsys,ddrc-ecc.yaml which doesn't match any of
> > the compatible strings.
> >
> >>>
> >>> New requirement? I've submitted not a single patch to the DT-bindings
> >>> sources and didn't get any comment from Rob about that.
> >>
> >
> >> This is not a new requirement. It has been since some time and Rob gave
> >> such reviews.
> >>
> >> https://lore.kernel.org/linux-devicetree/[email protected]/
> >
> > April 2022. So it's new. It would be nice to have it defined somewhere
> > in docs (writing-bindings.rst?). So does the compatibles order (this
> > was surprising to me too).
> >
> >>
> >> For devices with multiple compatibles that's a bit tricky, but assuming
> >> the bindings describe both original design from Synopsys and it's
> >> implementations, then something closer to Synopsys makes sense.
> >
> > The closest name would be snps,dw-umctl2-ddrc.yaml. snps,ddrc is too
> > generic especially for the IP-cores vendor. It doesn't have a
> > reference to the actual IP-core the device in subject is based on.
>
> You are aware that in such case mistake is not in the file name but in
> the compatible?
Let's compare, what is defined for Synopsys DW uMCTL2 DDRC in the
DT-bindings now:
compatible string: snps,ddrc-3.80a
file name: synopsys,ddrc-ecc.yaml
First of all they don't match at all. Secondly none of the names refer
to the actual IP-core the DT-bindings file is defined for. So in this
case the problem is in both:
1. file name has undefined vendor-prefix. It's supposed to be "snps".
2. file name has "ecc" suffix, which is a device property, but has
nothing to do with the device actual name.
2. actual device name is different. It's DW uMCTL2 DDRC. Just DDRC
doesn't identify the IP-core in subject.
>
> >
> >>
> >>
> >>> In addition
> >>> There are DT bindings with names different from what is defined in the
> >>> compatible name. Moreover there are tons of bindings with various
> >>> compatible names. What name to choose then? Finally the current name
> >>> is too generic to use for actual DW uMCTL2 DDRC controller.
> >>
> >
> >> There are thousands of bugs, inconsistencies, naming differences in
> >> kernel. I don't find these as arguments to repeat the practice...so the
> >> bindings file name should be based on the compatible.
> >
> > Did I ask for an exception? I justified why the renaming was necessary. You
> > said it goes against the practice of having the DT-schema named as the
> > device compatible strings and just nacked. But above in this message you said
> >
> >> "assuming the bindings describe both original design from Synopsys
> >> and it's implementations, then something closer to Synopsys makes sense"
> >
> > What I suggest makes more sense than some abstract Synopsys DDRC,
> > which may refer to a Synopsys DDR controller other than the subject
> > one. So I see two solutions here:
> > 1. Adding a new generic compatible string like "snps,dw-umctl2-ddrc"
> > and deprecate the "snps,ddrc-3.80a".
>
> This might be good idea, although unfortunately replacing compatibles
> takes quite a lot of time if you do not want to break any out-of-tree
> users (read: other users of bindings).
Well, I didn't imply the replacement, but "deprecation". It means to
just mark the "snps,ddrc-3.80a" compatible string as deprecated in the
bindings file and define a new one "snps,dw-umctl2-ddrc" which wouldn't have
the "deprecated: true" property set. It shall at least implicitly warn
people not to add new DTS-files with the deprecated device name. As I
see it eventually the dtbs-check tool will be updated to auto-detect
such attempts and, for instance, print a warning.
>
>
> > It gets to be even more justified
> > seeing the Synopsys IP-core version has been exported in the device
> > CSRs since IP-core v3.20a. So having the version attached to the
> > compatible string was absolutely redundant.
>
> The version might have sense in a way to differentiate from some older
> versions, pre 3.20a.
I've almost fully refactored the device driver, and can say for sure
that the driver doesn't implement any Synopsys IP-core versions
specifics:
1. "xlnx,zynq-ddrc-a05" - has absolutely nothing in common with the
Synopsys DW uMCTL2 DDR controller (see the patch log for details).
That's why I've split the driver and DT-bindings up.
2. "xlnx,zynqmp-ddrc-2.40a" - ZynqMP DDR controller based on the
Synopsys uMCTL2 DDRC v2.40a. The device name is vendor-specific
anyway. So seeing there is no any other ZynqMP DDR controller added,
having IP-core version attached to the compatible string has been
redundant in the first place. Anyway the driver implements only the
ZynqMP-platform specifics irrespective to the IP-core version.
3. "snps,ddrc-3.80a" - without my patchsets this represents a
Synopsys DW uMCTL2 DDR controller synthesized with 64-bit DQ-bus and
8xSDRAM words burst config. The later settings have nothing to do with
the IP-core version, meanwhile what is really v3.x-specific isn't
implemented in the driver.
> The binding was probably incomplete anyway.
At the current state it's incomplete indeed. After applying all my
DT-related patches it will get to be almost complete, at least in the
CSRs, IRQs, clocks and resets resources part. There is always room for
the device-specific properties though, but judging by my experience caught
from Rob' reviews such properties aren't welcome if we have the IP-core
version detectable and if we can add the platform-specific compatible
string. The later one could be used to define the platform-specific
parameters right in the driver so the DT-bindings could be kept
relatively simple.
>
>
> > 2. Just deprecate the generic compatible string, the new compatible
> > devices will be supposed to use a vendor-specific compatible strings,
> > but still rename the DT-bindings file. This makes sense since the
> > current generic name isn't quiet well structured. It' prefix-part is
> > too generic and at the same time it refers to a device reversion for
> > no much reason.
>
> You mean disallow entirely "snps,ddrc-3.80a" and expect everyone to use
> device/implementation specific compatible?
Yes as an alternative to the solution 1. described above.
> I guess this depends whether
> this custom block can be used without vendor specific addons. For
> several other DW blocks we expect to have the generic snsp fallback and
> this generic fallback can be used alone in Linux (pcie-designware-plat.c
> binds to it).
Just recently I've got a very long discussion with Rob regarding the
generic fallback compatible string. What he said "drop the generic
compatible fallback string. It proved to be useless." So I had to drop
the generic fallback compatible string from the nodes of my local DTS
files where it was appropriate and update all my DT-related patches to
disallow having vendor-specific and generic compatible strings specified
together.
Note what Rob said concerned the generic compatible "fallback" case,
not the generic compatible string in general. It's ok to have a
generic device name defined irrespective to the platform vendor.
Moreover it's applicable in case of the DW uMCTL2 DDRC IP-core since
first IP-core version is auto-detectable starting from v3.20a and
second I managed to implement auto-detection solutions for almost
all the DDR/ECC-specific parameters. So I am more inclined to the
solution 1) suggested by me in the previous email message:
- deprecate "snps,ddrc-3.80a" string.
- add new generic "snps,dw-umctl2-ddrc" compatible string.
- rename the DT-bindings file.
>
> Here the Linux driver also binds to generic synopsys compatible, so I
> would assume it has a meaning and use case on its own.
Please see my messages above regarding the current Synopsys DW uMCTL2
EDAC driver implementation.
>
> >
> > What do you think?
> >
> > * Note I've got it you'd prefer the renaming being performed in a
> > separate patch.
>
> The rename could be in the split patch as here, but then I assume the
> rename part to be detected by git and be a pure rename. However:
> 1. The git did not mark it as rename (you might need to use custom
> arguments to -M/-B/-C),
Of course git hasn't detected it as rename, because aside with renaming
I've split the bindings up. Splitting these two updates up into two
patches will give us what you said. So to speak I suggest the next
updates for v2:
PATCH X. Detach the Zynq A05 DDRC DT-bindings to a separate schema.
PATCH X + 1. Rename the Synopsys DW uMCTL2 DDRC bindings file and add a more
descriptive generic compatible string name.
What do you think?
> 2. There were also changes in the process (allOf:if:then).
Right. But this is in another patchset. I'll address your notes in there.
-Sergey
>
>
> Best regards,
> Krzysztof
On 24/08/2022 20:27, Serge Semin wrote:
>
> Note what Rob said concerned the generic compatible "fallback" case,
> not the generic compatible string in general. It's ok to have a
> generic device name defined irrespective to the platform vendor.
> Moreover it's applicable in case of the DW uMCTL2 DDRC IP-core since
> first IP-core version is auto-detectable starting from v3.20a and
> second I managed to implement auto-detection solutions for almost
> all the DDR/ECC-specific parameters. So I am more inclined to the
> solution 1) suggested by me in the previous email message:
> - deprecate "snps,ddrc-3.80a" string.
> - add new generic "snps,dw-umctl2-ddrc" compatible string.
> - rename the DT-bindings file.
Sounds ok.
>
>>
>> Here the Linux driver also binds to generic synopsys compatible, so I
>> would assume it has a meaning and use case on its own.
>
> Please see my messages above regarding the current Synopsys DW uMCTL2
> EDAC driver implementation.
>
>>
>>>
>>> What do you think?
>>>
>>> * Note I've got it you'd prefer the renaming being performed in a
>>> separate patch.
>>
>> The rename could be in the split patch as here, but then I assume the
>> rename part to be detected by git and be a pure rename. However:
>> 1. The git did not mark it as rename (you might need to use custom
>> arguments to -M/-B/-C),
>
> Of course git hasn't detected it as rename, because aside with renaming
> I've split the bindings up. Splitting these two updates up into two
> patches will give us what you said. So to speak I suggest the next
> updates for v2:
> PATCH X. Detach the Zynq A05 DDRC DT-bindings to a separate schema.
> PATCH X + 1. Rename the Synopsys DW uMCTL2 DDRC bindings file and add a more
> descriptive generic compatible string name.
>
> What do you think?
Regardless of the split the rename can be and should be detected by Git.
That's why we have these options. If it is not detected, you changed too
much during rename, so it is not a rename anymore. Relatively small
amount of changes would still be detected.
>
>> 2. There were also changes in the process (allOf:if:then).
>
> Right. But this is in another patchset. I'll address your notes in there.
Best regards,
Krzysztof
On Thu, Aug 25, 2022 at 09:06:42AM +0300, Krzysztof Kozlowski wrote:
> On 24/08/2022 20:27, Serge Semin wrote:
>
> >
> > Note what Rob said concerned the generic compatible "fallback" case,
> > not the generic compatible string in general. It's ok to have a
> > generic device name defined irrespective to the platform vendor.
> > Moreover it's applicable in case of the DW uMCTL2 DDRC IP-core since
> > first IP-core version is auto-detectable starting from v3.20a and
> > second I managed to implement auto-detection solutions for almost
> > all the DDR/ECC-specific parameters. So I am more inclined to the
> > solution 1) suggested by me in the previous email message:
> > - deprecate "snps,ddrc-3.80a" string.
> > - add new generic "snps,dw-umctl2-ddrc" compatible string.
> > - rename the DT-bindings file.
>
> Sounds ok.
Agreed then.
>
> >
> >>
> >> Here the Linux driver also binds to generic synopsys compatible, so I
> >> would assume it has a meaning and use case on its own.
> >
> > Please see my messages above regarding the current Synopsys DW uMCTL2
> > EDAC driver implementation.
> >
> >>
> >>>
> >>> What do you think?
> >>>
> >>> * Note I've got it you'd prefer the renaming being performed in a
> >>> separate patch.
> >>
> >> The rename could be in the split patch as here, but then I assume the
> >> rename part to be detected by git and be a pure rename. However:
> >> 1. The git did not mark it as rename (you might need to use custom
> >> arguments to -M/-B/-C),
> >
> > Of course git hasn't detected it as rename, because aside with renaming
> > I've split the bindings up. Splitting these two updates up into two
> > patches will give us what you said. So to speak I suggest the next
> > updates for v2:
> > PATCH X. Detach the Zynq A05 DDRC DT-bindings to a separate schema.
> > PATCH X + 1. Rename the Synopsys DW uMCTL2 DDRC bindings file and add a more
> > descriptive generic compatible string name.
> >
> > What do you think?
>
> Regardless of the split the rename can be and should be detected by Git.
> That's why we have these options. If it is not detected, you changed too
> much during rename, so it is not a rename anymore. Relatively small
> amount of changes would still be detected.
Right. I'll make sure the renaming is detected.
-Sergey
>
> >
> >> 2. There were also changes in the process (allOf:if:then).
> >
> > Right. But this is in another patchset. I'll address your notes in there.
>
>
> Best regards,
> Krzysztof
On Mon, Aug 22, 2022 at 10:07:23PM +0300, Serge Semin wrote:
> The duplicated edac_dbg()-based dimm->nr_pages field was introduced in
> commit 6e84d359b2be ("edac_mc: Cleanup per-dimm_info debug messages"). You
> can find the duplicated line even in the commit message text:
> > [ 1011.380101] EDAC DEBUG: edac_mc_dump_dimm: dimm->nr_pages = 0x40000
> > [ 1011.380103] EDAC DEBUG: edac_mc_dump_dimm: dimm->grain = 8
> > [ 1011.380104] EDAC DEBUG: edac_mc_dump_dimm: dimm->nr_pages = 0x40000
> Let's drop the corresponding edac_dbg() method call then.
>
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/edac/edac_mc.c | 1 -
> 1 file changed, 1 deletion(-)
Applied, thanks.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Aug 22, 2022 at 10:07:22PM +0300, Serge Semin wrote:
> Currently the memory type macros are partly defined with the multiple
> spaces between the macro name and its definition. Let's replace the spaces
> with tabs as the kernel coding style requires.
>
> Signed-off-by: Serge Semin <[email protected]>
> ---
> include/linux/edac.h | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
Applied, thanks.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette