linux-edac.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups
@ 2022-09-10 19:42 Serge Semin
  2022-09-10 19:42 ` [PATCH v2 01/19] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin
                   ` (19 more replies)
  0 siblings, 20 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, James Morse, Robert Richter, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

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: In-progress] EDAC/synopsys: Add generic DDRC info and address mapping
Link: https://lore.kernel.org/linux-edac/20220822191427.27969-1-Sergey.Semin@baikalelectronics.ru
[3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 support
Link: https://lore.kernel.org/linux-edac/20220822191957.28546-1-Sergey.Semin@baikalelectronics.ru

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.

Link: https://lore.kernel.org/linux-edac/20220822190730.27277-1-Sergey.Semin@baikalelectronics.ru/
Changelog 2:
- Move Synopsys DW uMCTL2 DDRC bindings file renaming to a separate patch.
  (@Krzysztof)
- Introduce a new compatible string "snps,dw-umctl2-ddrc" matching the new
  DT-schema name.
- Forgot to fix some of the prefix of the SYNPS_ZYNQMP_IRQ_REGS macro
  in several places. (@tbot)
- Drop the no longer used "priv" pointer from the mc_init() function.
  (@tbot)
- Include "linux/bitfield.h" header file to get the FIELD_GET macro
  definition. (@tbot)
- Drop the already merged in patches:
[PATCH 12/20] EDAC/mc: Replace spaces with tabs in memtype flags definition
[PATCH 13/20] EDAC/mc: Drop duplicated dimm->nr_pages debug printout

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Michail Ivanov <Michail.Ivanov@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xilinx.com>
Cc: Manish Narani <manish.narani@xilinx.com>
Cc: Dinh Nguyen <dinguyen@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Robert Richter <rric@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (19):
  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: Init DIMM labels in MC registration method
  EDAC/mc: Add MC unique index allocation procedure
  dt-bindings: memory: snps: Detach Zynq DDRC controller support
  dt-bindings: memory: snps: Use more descriptive device name
  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                  |  56 ++
 .../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                        | 135 ++-
 drivers/edac/edac_mc.h                        |   4 +
 drivers/edac/synopsys_edac.c                  | 903 ++++++------------
 drivers/edac/zynq_edac.c                      | 504 ++++++++++
 10 files changed, 1026 insertions(+), 703 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.37.2


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 01/19] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 02/19] EDAC/synopsys: Fix generic device type detection procedure Serge Semin
                   ` (18 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter,
	Dinh Nguyen
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Rob Herring, Krzysztof Kozlowski, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel, Borislav Petkov

The generic DW uMCTL2 DDRC v3.x support was added in commit f7824ded4149
("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR"). It
hasn't been done quiet well there with respect to the IRQs handling
procedure. An attempt to fix that was introduced in the recent commit
4bcffe941758 ("EDAC/synopsys: Re-enable the error interrupts on v3 hw").
Alas again it didn't provide quite complete solution.

First of all the commit f7824ded4149 ("EDAC/synopsys: Add support for
version 3 of the Synopsys EDAC DDR") log says that v3.80a "has UE/CE auto
cleared". They aren't in none of the IP-core versions. The IRQ status can
be cleared by means of setting the ECCCLR/ECCCTL register self-cleared
flags 0-3. The pending IRQ clearance is done in the respective
get_error_info() method of the driver. Thus defining a quirk flag with the
"DDR_ECC_INTR_SELF_CLEAR" name was at least very inaccurate if not to say
misleading. So was adding the comments about the "ce/ue bits automatically
cleared".

Second, disabling the being handled IRQ in the handler doesn't make sense
in Linux since the IC line is masked during that procedure anyway. So
disabling the IRQ in one part of the handler and enabling it at the end of
the method is simply redundant. (See, the ZynqMP-specific code with the
QoS IRQ CSR didn't do that originally.)

Finally calling the zynqmp_get_error_info() method concurrently with the
enable_irq()/disable_irq() functions causes the IRQs mask state race
condition. Starting from DW uMCTL2 DDRC IP-core v3.10a [1] the ECCCLR
register has been renamed to ECCCTL and has been equipped with CE/UE IRQs
enable/disable flags [2]. So the CSR now serves for the IRQ status and
control functions used concurrently during the IRQ handling and the IRQ
disabling/enabling. Thus the corresponding critical section must be
protected with the IRQ-safe spin-lock.

So let's fix all the problems noted above. First the
DDR_ECC_INTR_SELF_CLEAR flag is renamed to SYNPS_ZYNQMP_IRQ_REGS. Its
semantic is now the opposite: the quirk means having the ZynqMP IRQ CSRs
available on the platform. Second the DDR_UE_MASK and DDR_CE_MASK macros
are renamed to imply being used in the framework of the ECCCLR/ECCCTL CSRs
accesses. Third all the misleading comments are removed. Finally the
ECC_CLR_OFST register IOs are now protected with the IRQ-safe spin-lock
taken in order to prevent the IRQ status clearance and IRQ enable/disable
race condition.

[1] DesignWare Cores Enhanced Universal DDR Memory and Protocol
Controllers (uMCTL2/uPCTL2), Release Notes, Version 3.91a, October 2020,
p. 27.
[2] DesignWare® Cores Enhanced Universal DDR Memory Controller (uMCTL2),
Databook Version 3.91a, October 2020, p.818-819.

Fixes: f7824ded4149 ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/edac/synopsys_edac.c | 76 +++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 28 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index f7d37c282819..c78fb5781ff9 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -9,6 +9,7 @@
 #include <linux/edac.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -88,7 +89,7 @@
 /* DDR ECC Quirks */
 #define DDR_ECC_INTR_SUPPORT		BIT(0)
 #define DDR_ECC_DATA_POISON_SUPPORT	BIT(1)
-#define DDR_ECC_INTR_SELF_CLEAR		BIT(2)
+#define SYNPS_ZYNQMP_IRQ_REGS		BIT(2)
 
 /* ZynqMP Enhanced DDR memory controller registers that are relevant to ECC */
 /* ECC Configuration Registers */
@@ -133,11 +134,13 @@
 
 #define ECC_ADDRMAP0_OFFSET		0x200
 
-/* Control register bitfield definitions */
+/* 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 Control Register width definitions  */
 #define DDRCTL_EWDTH_16			2
@@ -164,10 +167,6 @@
 #define DDR_QOS_IRQ_EN_OFST		0x20208
 #define DDR_QOS_IRQ_DB_OFST		0x2020C
 
-/* DDR QOS Interrupt register definitions */
-#define DDR_UE_MASK			BIT(9)
-#define DDR_CE_MASK			BIT(8)
-
 /* ECC Corrected Error Register Mask and Shifts*/
 #define ECC_CEADDR0_RW_MASK		0x3FFFF
 #define ECC_CEADDR0_RNK_MASK		BIT(24)
@@ -300,6 +299,7 @@ struct synps_ecc_status {
 /**
  * struct synps_edac_priv - DDR memory controller private instance data.
  * @baseaddr:		Base address of the DDR controller.
+ * @lock:		Concurrent CSRs access lock.
  * @message:		Buffer for framing the event specific info.
  * @stat:		ECC status information.
  * @p_data:		Platform data.
@@ -314,6 +314,7 @@ struct synps_ecc_status {
  */
 struct synps_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;
@@ -409,7 +410,8 @@ static int zynq_get_error_info(struct synps_edac_priv *priv)
 static int zynqmp_get_error_info(struct synps_edac_priv *priv)
 {
 	struct synps_ecc_status *p;
-	u32 regval, clearval = 0;
+	u32 regval, clearval;
+	unsigned long flags;
 	void __iomem *base;
 
 	base = priv->baseaddr;
@@ -452,11 +454,16 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
 					ECC_CEADDR1_BNKNR_SHIFT;
 	p->ueinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
 	p->ueinfo.data = readl(base + ECC_UESYND0_OFST);
+
 out:
-	clearval = ECC_CTRL_CLR_CE_ERR | ECC_CTRL_CLR_CE_ERRCNT;
-	clearval |= ECC_CTRL_CLR_UE_ERR | ECC_CTRL_CLR_UE_ERRCNT;
+	spin_lock_irqsave(&priv->lock, flags);
+
+	clearval = readl(base + ECC_CLR_OFST) |
+		   ECC_CTRL_CLR_CE_ERR | ECC_CTRL_CLR_CE_ERRCNT |
+		   ECC_CTRL_CLR_UE_ERR | ECC_CTRL_CLR_UE_ERRCNT;
 	writel(clearval, base + ECC_CLR_OFST);
-	writel(0x0, base + ECC_CLR_OFST);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
 
 	return 0;
 }
@@ -516,24 +523,42 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
 
 static void enable_intr(struct synps_edac_priv *priv)
 {
+	unsigned long flags;
+
 	/* Enable UE/CE Interrupts */
-	if (priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR)
-		writel(DDR_UE_MASK | DDR_CE_MASK,
-		       priv->baseaddr + ECC_CLR_OFST);
-	else
+	if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS) {
 		writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
 		       priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
 
+		return;
+	}
+
+	/* IRQs Enable/Disable feature has been available since v3.10a */
+	spin_lock_irqsave(&priv->lock, flags);
+
+	writel(ECC_CTRL_EN_CE_IRQ | ECC_CTRL_EN_UE_IRQ,
+	       priv->baseaddr + ECC_CLR_OFST);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 static void disable_intr(struct synps_edac_priv *priv)
 {
+	unsigned long flags;
+
 	/* Disable UE/CE Interrupts */
-	if (priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR)
-		writel(0x0, priv->baseaddr + ECC_CLR_OFST);
-	else
+	if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS) {
 		writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
 		       priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
+
+		return;
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	writel(0, priv->baseaddr + ECC_CLR_OFST);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 /**
@@ -553,11 +578,7 @@ static irqreturn_t intr_handler(int irq, void *dev_id)
 	priv = mci->pvt_info;
 	p_data = priv->p_data;
 
-	/*
-	 * v3.0 of the controller has the ce/ue bits cleared automatically,
-	 * so this condition does not apply.
-	 */
-	if (!(priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR)) {
+	if (priv->p_data->quirks & SYNPS_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))
@@ -574,11 +595,9 @@ static irqreturn_t intr_handler(int irq, void *dev_id)
 
 	edac_dbg(3, "Total error count CE %d UE %d\n",
 		 priv->ce_cnt, priv->ue_cnt);
-	/* v3.0 of the controller does not have this register */
-	if (!(priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR))
+
+	if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS)
 		writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
-	else
-		enable_intr(priv);
 
 	return IRQ_HANDLED;
 }
@@ -900,7 +919,7 @@ static const struct synps_platform_data zynqmp_edac_def = {
 	.get_mtype	= zynqmp_get_mtype,
 	.get_dtype	= zynqmp_get_dtype,
 	.get_ecc_state	= zynqmp_get_ecc_state,
-	.quirks         = (DDR_ECC_INTR_SUPPORT
+	.quirks         = (DDR_ECC_INTR_SUPPORT | SYNPS_ZYNQMP_IRQ_REGS
 #ifdef CONFIG_EDAC_DEBUG
 			  | DDR_ECC_DATA_POISON_SUPPORT
 #endif
@@ -912,7 +931,7 @@ static const struct synps_platform_data synopsys_edac_def = {
 	.get_mtype	= zynqmp_get_mtype,
 	.get_dtype	= zynqmp_get_dtype,
 	.get_ecc_state	= zynqmp_get_ecc_state,
-	.quirks         = (DDR_ECC_INTR_SUPPORT | DDR_ECC_INTR_SELF_CLEAR
+	.quirks         = (DDR_ECC_INTR_SUPPORT
 #ifdef CONFIG_EDAC_DEBUG
 			  | DDR_ECC_DATA_POISON_SUPPORT
 #endif
@@ -1360,6 +1379,7 @@ static int mc_probe(struct platform_device *pdev)
 	priv = mci->pvt_info;
 	priv->baseaddr = baseaddr;
 	priv->p_data = p_data;
+	spin_lock_init(&priv->lock);
 
 	mc_init(mci, pdev);
 
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 02/19] EDAC/synopsys: Fix generic device type detection procedure
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
  2022-09-10 19:42 ` [PATCH v2 01/19] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 03/19] EDAC/synopsys: Fix mci->scrub_cap field setting Serge Semin
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter,
	Manish Narani
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Dinh Nguyen,
	Rob Herring, Krzysztof Kozlowski, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel, Borislav Petkov

First of all the enum dev_type constants define the Memory devices, i.e.
DRAM chips, DQ-bus width (see the enumberation kdoc for details). So what
is returned from the zynqmp_get_dtype() procedure is definitely wrong.
Secondly the DRAM chips type has nothing to do with the data bus width
specified in the MSTR.data_bus_width CSR field. The later one just
determines the part of the whole DQ-bus used to access the data from the
all DRAM chips. So it doesn't indicate the individual chips type. Thirdly
the DRAM chips type can be determined only in case of the DDR4 protocol by
means of the MSTR.device_config field state (it is supposed to be set by
the system firmware). Finally the DW uMCTL2 DDRC ECC capability doesn't
depend on the memory chips type. Moreover it doesn't depend on the data
bus width in runtime either. The IP-core reference manual says in [1,2]
that the ECC support can't be enabled during the IP-core synthesizes for
the DRAM data bus widths other than 16, 32 or 64. At the same time the
bus width mode (MSTR.data_bus_width) doesn't change the ECC feature
availability. Thus it was wrong to determine the ECC state with respect to
the DQ-bus width mode.

Let's fix all of the mistakes above in the zynqmp_get_dtype() and
zynqmp_get_ecc_state() methods. In accordance with the DW uMCTL2 DDRC
nature the DRAM chips type in most of the cases will be unknown except
when DDR4 protocol is utilized. ECC availability will be determined by the
ECCCFG0.ecc_mode field state only.

[1] DesignWare® Cores Enhanced Universal DDR Memory Controller (uMCTL2)
Databook, Version 3.91a, October 2020, p. 421.
[2] DesignWare® Cores Enhanced Universal DDR Memory Controller (uMCTL2)
Databook, Version 3.91a, October 2020, p. 633.

Fixes: b500b4a029d5 ("EDAC, synopsys: Add ECC support for ZynqMP DDR controller")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Changelog v2:
- Include "linux/bitfield.h" header file to get the FIELD_GET macro
  definition. (@tbot)
---
 drivers/edac/synopsys_edac.c | 57 +++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index c78fb5781ff9..17960f7ca29b 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2012 - 2014 Xilinx, Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/edac.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -142,7 +143,12 @@
 #define ECC_CTRL_EN_CE_IRQ		BIT(8)
 #define ECC_CTRL_EN_UE_IRQ		BIT(9)
 
-/* DDR Control Register width definitions  */
+/* DDR Master Register 0 definitions */
+#define DDR_MSTR_DEV_CFG_MASK		GENMASK(31, 30)
+#define DDR_MSTR_DEV_X4			0x0
+#define DDR_MSTR_DEV_X8			0x1
+#define DDR_MSTR_DEV_X16		0x2
+#define DDR_MSTR_DEV_X32		0x3
 #define DDRCTL_EWDTH_16			2
 #define DDRCTL_EWDTH_32			1
 #define DDRCTL_EWDTH_64			0
@@ -671,26 +677,25 @@ static enum dev_type zynq_get_dtype(const void __iomem *base)
  */
 static enum dev_type zynqmp_get_dtype(const void __iomem *base)
 {
-	enum dev_type dt;
-	u32 width;
-
-	width = readl(base + CTRL_OFST);
-	width = (width & ECC_CTRL_BUSWIDTH_MASK) >> ECC_CTRL_BUSWIDTH_SHIFT;
-	switch (width) {
-	case DDRCTL_EWDTH_16:
-		dt = DEV_X2;
-		break;
-	case DDRCTL_EWDTH_32:
-		dt = DEV_X4;
-		break;
-	case DDRCTL_EWDTH_64:
-		dt = DEV_X8;
-		break;
-	default:
-		dt = DEV_UNKNOWN;
+	u32 regval;
+
+	regval = readl(base + DDR_MSTR_OFST);
+	if (!(regval & MEM_TYPE_DDR4))
+		return DEV_UNKNOWN;
+
+	regval = FIELD_GET(DDR_MSTR_DEV_CFG_MASK, regval);
+	switch (regval) {
+	case DDR_MSTR_DEV_X4:
+		return DEV_X4;
+	case DDR_MSTR_DEV_X8:
+		return DEV_X8;
+	case DDR_MSTR_DEV_X16:
+		return DEV_X16;
+	case DDR_MSTR_DEV_X32:
+		return DEV_X32;
 	}
 
-	return dt;
+	return DEV_UNKNOWN;
 }
 
 /**
@@ -727,19 +732,11 @@ static bool zynq_get_ecc_state(void __iomem *base)
  */
 static bool zynqmp_get_ecc_state(void __iomem *base)
 {
-	enum dev_type dt;
-	u32 ecctype;
+	u32 regval;
 
-	dt = zynqmp_get_dtype(base);
-	if (dt == DEV_UNKNOWN)
-		return false;
+	regval = readl(base + ECC_CFG0_OFST) & SCRUB_MODE_MASK;
 
-	ecctype = readl(base + ECC_CFG0_OFST) & SCRUB_MODE_MASK;
-	if ((ecctype == SCRUB_MODE_SECDED) &&
-	    ((dt == DEV_X2) || (dt == DEV_X4) || (dt == DEV_X8)))
-		return true;
-
-	return false;
+	return (regval == SCRUB_MODE_SECDED);
 }
 
 /**
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 03/19] EDAC/synopsys: Fix mci->scrub_cap field setting
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
  2022-09-10 19:42 ` [PATCH v2 01/19] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin
  2022-09-10 19:42 ` [PATCH v2 02/19] EDAC/synopsys: Fix generic device type detection procedure Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 04/19] EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse Serge Semin
                   ` (16 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter,
	Punnaiah Choudary Kalluri
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Manish Narani, Dinh Nguyen, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel, Borislav Petkov

The mem_ctl_info.scrub_cap field is supposed to be set with the ECC
scrub-related flags while the driver initializes it with the SCRUB_HW_SRC
flag ID. It's definitely wrong, though hasn't caused any problem since the
structure field isn't used by the EDAC core. Let's fix it anyway by using
the SCRUB_FLAG_HW_SRC macro to initialize the field.

Fixes: ae9b56e3996d ("EDAC, synps: Add EDAC support for zynq ddr ecc controller")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/edac/synopsys_edac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 17960f7ca29b..c6f3b383e5ff 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -858,7 +858,7 @@ static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
 	/* Initialize controller capabilities and configuration */
 	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
-	mci->scrub_cap = SCRUB_HW_SRC;
+	mci->scrub_cap = SCRUB_FLAG_HW_SRC;
 	mci->scrub_mode = SCRUB_NONE;
 
 	mci->edac_cap = EDAC_FLAG_SECDED;
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 04/19] EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (2 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 03/19] EDAC/synopsys: Fix mci->scrub_cap field setting Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 05/19] EDAC/synopsys: Fix reading errors count before ECC status Serge Semin
                   ` (15 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter,
	Manish Narani
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Dinh Nguyen,
	Rob Herring, Krzysztof Kozlowski, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel, Borislav Petkov

Currently the ADDRMAP4.addrmap_col_b10 field gets to be parsed in case of
the LPDDR3 memory and Quarter DQ bus width mode. It's wrong since that
field is marked as unused for that mode in all the available DW uMCTL2
DDRC releases (up to IP-core v3.91a). Most likely the field parsing has
been added by mistake as a result of the copy-paste from the Half DW bus
width mode part of the same function. Drop it from the driver for good
then.

Fixes: 1a81361f75d8 ("EDAC, synopsys: Add Error Injection support for ZynqMP DDR controller")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/edac/synopsys_edac.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index c6f3b383e5ff..da1d90a87778 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -1239,10 +1239,6 @@ static void setup_column_address_map(struct synps_edac_priv *priv, u32 *addrmap)
 				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
 				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK) +
 				 COL_B9_BASE);
-			priv->col_shift[13] = ((addrmap[4] &
-				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
-				((addrmap[4] & COL_MAX_VAL_MASK) +
-				 COL_B10_BASE);
 		} else {
 			priv->col_shift[11] = (((addrmap[3] >> 16) &
 				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 05/19] EDAC/synopsys: Fix reading errors count before ECC status
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (3 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 04/19] EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-12  5:26   ` Datta, Shubhrajyoti
  2022-09-10 19:42 ` [PATCH v2 06/19] EDAC/synopsys: Use platform device devm ioremap method Serge Semin
                   ` (14 subsequent siblings)
  19 siblings, 1 reply; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter,
	Shubhrajyoti Datta
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel, Borislav Petkov

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 <Sergey.Semin@baikalelectronics.ru>
---
 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 da1d90a87778..558d3b3e6864 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -423,18 +423,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.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 06/19] EDAC/synopsys: Use platform device devm ioremap method
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (4 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 05/19] EDAC/synopsys: Fix reading errors count before ECC status Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 07/19] EDAC/synopsys: Drop internal CE and UE counters Serge Semin
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>
---
 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 558d3b3e6864..e9002d9b3f09 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -1337,11 +1337,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.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 07/19] EDAC/synopsys: Drop internal CE and UE counters
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (5 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 06/19] EDAC/synopsys: Use platform device devm ioremap method Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 08/19] EDAC/synopsys: Drop local to_mci macro implementation Serge Semin
                   ` (12 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

First of all they aren't exposed anyhow by the driver. Secondly the EDAC
core already tracks the total amount of correctable and uncorrectable
errors (see mem_ctl_info.{ce_mc,ue_mc} fields usage). Let's drop the
useless internal counters then for good.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/edac/synopsys_edac.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index e9002d9b3f09..592c7753184f 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -309,8 +309,6 @@ struct synps_ecc_status {
  * @message:		Buffer for framing the event specific info.
  * @stat:		ECC status information.
  * @p_data:		Platform data.
- * @ce_cnt:		Correctable Error count.
- * @ue_cnt:		Uncorrectable Error count.
  * @poison_addr:	Data poison address.
  * @row_shift:		Bit shifts for row bit.
  * @col_shift:		Bit shifts for column bit.
@@ -324,8 +322,6 @@ struct synps_edac_priv {
 	char message[SYNPS_EDAC_MSG_SIZE];
 	struct synps_ecc_status stat;
 	const struct synps_platform_data *p_data;
-	u32 ce_cnt;
-	u32 ue_cnt;
 #ifdef CONFIG_EDAC_DEBUG
 	ulong poison_addr;
 	u32 row_shift[18];
@@ -595,12 +591,8 @@ static irqreturn_t intr_handler(int irq, void *dev_id)
 	if (status)
 		return IRQ_NONE;
 
-	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);
 
 	if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS)
 		writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 08/19] EDAC/synopsys: Drop local to_mci macro implementation
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (6 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 07/19] EDAC/synopsys: Drop internal CE and UE counters Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 09/19] EDAC/synopsys: Drop struct ecc_error_info.blknr field Serge Semin
                   ` (11 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

The to_mci macro was added in commit 1a81361f75d8 ("EDAC, synopsys: Add
Error Injection support for ZynqMP DDR controller") together with the
errors injection debug feature. It turns our the macro with the same
semantic and name has already been defined in the edac_mc.h (former
edac_core.h) header file. No idea why it was needed to have a local
version with the same semantic, but now there is no point in that. Drop
the local implementation for good then.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/edac/synopsys_edac.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 592c7753184f..9a039aa0c308 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -949,7 +949,6 @@ static const struct of_device_id synps_edac_match[] = {
 MODULE_DEVICE_TABLE(of, synps_edac_match);
 
 #ifdef CONFIG_EDAC_DEBUG
-#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
 
 /**
  * ddr_poison_setup -	Update poison registers.
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 09/19] EDAC/synopsys: Drop struct ecc_error_info.blknr field
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (7 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 08/19] EDAC/synopsys: Drop local to_mci macro implementation Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 10/19] EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name Serge Semin
                   ` (10 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

Even though the ECC(C|U)ADDR1 CSR description indeed says it's a "Block
number" in the DW uMCTL2 DDRC hw reference manuals, the corresponding
register field name (ECC(C|U)ADDR1.ecc_(un)corr_col) and the rest of the
hw documentation refer to the field as the SDRAM address column. Thus
let's use the already available ecc_error_info.col field to read the
column number to and drop the questionable ecc_error_info.blknr field for
good.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/edac/synopsys_edac.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 9a039aa0c308..3a863c012eb6 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -178,7 +178,7 @@
 #define ECC_CEADDR0_RNK_MASK		BIT(24)
 #define ECC_CEADDR1_BNKGRP_MASK		0x3000000
 #define ECC_CEADDR1_BNKNR_MASK		0x70000
-#define ECC_CEADDR1_BLKNR_MASK		0xFFF
+#define ECC_CEADDR1_COL_MASK		0xFFF
 #define ECC_CEADDR1_BNKGRP_SHIFT	24
 #define ECC_CEADDR1_BNKNR_SHIFT		16
 
@@ -276,7 +276,6 @@
  * @bitpos:	Bit position.
  * @data:	Data causing the error.
  * @bankgrpnr:	Bank group number.
- * @blknr:	Block number.
  */
 struct ecc_error_info {
 	u32 row;
@@ -285,7 +284,6 @@ struct ecc_error_info {
 	u32 bitpos;
 	u32 data;
 	u32 bankgrpnr;
-	u32 blknr;
 };
 
 /**
@@ -438,7 +436,7 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
 					ECC_CEADDR1_BNKNR_SHIFT;
 	p->ceinfo.bankgrpnr = (regval &	ECC_CEADDR1_BNKGRP_MASK) >>
 					ECC_CEADDR1_BNKGRP_SHIFT;
-	p->ceinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
+	p->ceinfo.col = (regval & ECC_CEADDR1_COL_MASK);
 	p->ceinfo.data = readl(base + ECC_CSYND0_OFST);
 	edac_dbg(2, "ECCCSYN0: 0x%08X ECCCSYN1: 0x%08X ECCCSYN2: 0x%08X\n",
 		 readl(base + ECC_CSYND0_OFST), readl(base + ECC_CSYND1_OFST),
@@ -454,7 +452,7 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
 					ECC_CEADDR1_BNKGRP_SHIFT;
 	p->ueinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
 					ECC_CEADDR1_BNKNR_SHIFT;
-	p->ueinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
+	p->ueinfo.col = (regval & ECC_CEADDR1_COL_MASK);
 	p->ueinfo.data = readl(base + ECC_UESYND0_OFST);
 
 out:
@@ -486,10 +484,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 Bank %d BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x",
-				 "CE", pinf->row, pinf->bank,
-				 pinf->bankgrpnr, pinf->blknr,
-				 pinf->bitpos, pinf->data);
+				 "DDR ECC error type:%s Row %d Col %d Bank %d BankGroup Number %d Bit Position: %d Data: 0x%08x",
+				 "CE", pinf->row, pinf->col, pinf->bank,
+				 pinf->bankgrpnr, 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",
@@ -506,9 +503,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 Bank %d BankGroup Number %d Block Number %d",
-				 "UE", pinf->row, pinf->bank,
-				 pinf->bankgrpnr, pinf->blknr);
+				 "DDR ECC error type :%s Row %d Col %d Bank %d BankGroup Number %d",
+				 "UE", pinf->row, pinf->col, pinf->bank,
+				 pinf->bankgrpnr);
 		} else {
 			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
 				 "DDR ECC error type :%s Row %d Bank %d Col %d ",
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 10/19] EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (8 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 09/19] EDAC/synopsys: Drop struct ecc_error_info.blknr field Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 11/19] EDAC/synopsys: Drop redundant info from error message Serge Semin
                   ` (9 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>
---
 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 3a863c012eb6..2740f6c8c249 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -273,17 +273,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;
 };
 
 /**
@@ -434,7 +434,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);
@@ -448,7 +448,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;
@@ -484,9 +484,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",
@@ -503,9 +503,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.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 11/19] EDAC/synopsys: Drop redundant info from error message
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (9 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 10/19] EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 12/19] EDAC/mc: Init DIMM labels in MC registration method Serge Semin
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

Currently the custom error message is too long so the corresponding log
messages printed in several lines. There is some duplicated/redundant
information we can remove from it. First of all we can shorten it out by
dropping the message prefix "DDR ECC error type:%s". Indeed the text
printed to the console later by the edac_mc_printk() method will contain
the error type and the memory controller id referring to the device
detected the error. So having the same info in the custom part of the
same message is useless especially seeing the string has got too long
already. Secondly referring to the corrected bit as "Bit Position" is
redundant since saying just "Bit" is not that less descriptive. Let's
drop all of these parts of the error message then so to have a shorter
but still informative enough log message.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/edac/synopsys_edac.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 2740f6c8c249..5088634bc213 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -484,13 +484,13 @@ 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 Bank Group %d Bit Position: %d Data: 0x%08x",
-				 "CE", pinf->row, pinf->col, pinf->bank,
-				 pinf->bankgrp, pinf->bitpos, pinf->data);
+				 "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,
-				 "DDR ECC error type:%s Row %d Bank %d Col %d Bit Position: %d Data: 0x%08x",
-				 "CE", pinf->row, pinf->bank, pinf->col,
+				 "Row %d Bank %d Col %d Bit: %d Data: 0x%08x",
+				 pinf->row, pinf->bank, pinf->col,
 				 pinf->bitpos, pinf->data);
 		}
 
@@ -503,13 +503,12 @@ 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 Bank Group %d",
-				 "UE", pinf->row, pinf->col, pinf->bank,
-				 pinf->bankgrp);
+				 "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,
-				 "DDR ECC error type :%s Row %d Bank %d Col %d ",
-				 "UE", pinf->row, pinf->bank, pinf->col);
+				 "Row %d Bank %d Col %d",
+				 pinf->row, pinf->bank, pinf->col);
 		}
 
 		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 12/19] EDAC/mc: Init DIMM labels in MC registration method
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (10 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 11/19] EDAC/synopsys: Drop redundant info from error message Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 13/19] EDAC/mc: Add MC unique index allocation procedure Serge Semin
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

As a preparation before adding the unique MC index allocation procedure we
need to move the DIMM labels initialization to the memory controller
registration method. It's required because the labels contain the MC index
as the "mc%u" part of the string, which in case of the auto-generated
index isn't available at the moment of the MCI/csrow/dimms descriptor
allocation.

So nothing complex is done here. Just move the labels initialization from
edac_mc_alloc_dimms() to the dedicated method edac_mc_init_labels() and
call it from edac_mc_add_mc_with_groups().

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/edac/edac_mc.c | 48 +++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 6faeb2ab3960..24814839d885 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -256,7 +256,6 @@ static int edac_mc_alloc_dimms(struct mem_ctl_info *mci)
 	unsigned int pos[EDAC_MAX_LAYERS];
 	unsigned int row, chn, idx;
 	int layer;
-	void *p;
 
 	/*
 	 * Allocate and fill the dimm structs
@@ -271,7 +270,6 @@ static int edac_mc_alloc_dimms(struct mem_ctl_info *mci)
 	for (idx = 0; idx < mci->tot_dimms; idx++) {
 		struct dimm_info *dimm;
 		struct rank_info *chan;
-		int n, len;
 
 		chan = mci->csrows[row]->channels[chn];
 
@@ -282,22 +280,9 @@ static int edac_mc_alloc_dimms(struct mem_ctl_info *mci)
 		dimm->mci = mci;
 		dimm->idx = idx;
 
-		/*
-		 * Copy DIMM location and initialize it.
-		 */
-		len = sizeof(dimm->label);
-		p = dimm->label;
-		n = scnprintf(p, len, "mc#%u", mci->mc_idx);
-		p += n;
-		len -= n;
-		for (layer = 0; layer < mci->n_layers; layer++) {
-			n = scnprintf(p, len, "%s#%u",
-				      edac_layer_name[mci->layers[layer].type],
-				      pos[layer]);
-			p += n;
-			len -= n;
+		/* Copy DIMM location */
+		for (layer = 0; layer < mci->n_layers; layer++)
 			dimm->location[layer] = pos[layer];
-		}
 
 		/* Link it to the csrows old API data */
 		chan->dimm = dimm;
@@ -510,6 +495,33 @@ void edac_mc_reset_delay_period(unsigned long value)
 
 
 
+/**
+ * edac_mc_init_labels() - Initialize DIMM labels
+ *
+ * @mci: pointer to the mci structure which DIMM labels need to be initialized
+ *
+ * .. note::
+ *	locking model: must be called with the mem_ctls_mutex lock held
+ */
+static void edac_mc_init_labels(struct mem_ctl_info *mci)
+{
+	int n, len, layer;
+	unsigned int idx;
+	char *p;
+
+	for (idx = 0; idx < mci->tot_dimms; idx++) {
+		len = sizeof(mci->dimms[idx]->label);
+		p = mci->dimms[idx]->label;
+
+		n = scnprintf(p, len, "mc#%u", mci->mc_idx);
+		for (layer = 0; layer < mci->n_layers; layer++) {
+			n += scnprintf(p + n, len - n, "%s#%u",
+				      edac_layer_name[mci->layers[layer].type],
+				      mci->dimms[idx]->location[layer]);
+		}
+	}
+}
+
 /* Return 0 on success, 1 on failure.
  * Before calling this function, caller must
  * assign a unique value to mci->mc_idx.
@@ -637,6 +649,8 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 		goto fail0;
 	}
 
+	edac_mc_init_labels(mci);
+
 	if (add_mc_to_global_list(mci))
 		goto fail0;
 
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 13/19] EDAC/mc: Add MC unique index allocation procedure
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (11 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 12/19] EDAC/mc: Init DIMM labels in MC registration method Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support Serge Semin
                   ` (6 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>

---

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.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (12 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 13/19] EDAC/mc: Add MC unique index allocation procedure Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-16 18:44   ` Rob Herring
  2022-09-21 18:32   ` (subset) " Krzysztof Kozlowski
  2022-09-10 19:42 ` [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name Serge Semin
                   ` (5 subsequent siblings)
  19 siblings, 2 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, Krzysztof Kozlowski,
	Manish Narani
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Dinh Nguyen,
	James Morse, Robert Richter, Rob Herring, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel, Krzysztof Kozlowski

The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
the CSRs layout is absolutely different and it doesn't support IRQs 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 in the same bindings.
Let's split the DT-schema up.

Note since the synopsys,ddrc-ecc.yaml schema describes the Synopsys DW
uMCTL2 DDR controller only, we need to accordingly fix the device
descriptions.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Changelog v2:
- Move Synopsys DW uMCTL2 DDRC bindings file renaming to a separate
  patch. (@Krzysztof)
---
 .../memory-controllers/synopsys,ddrc-ecc.yaml | 63 ++++++-------------
 .../xlnx,zynq-ddrc-a05.yaml                   | 38 +++++++++++
 MAINTAINERS                                   |  1 +
 3 files changed, 59 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml

diff --git a/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml b/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
index f46e95704f53..0be8ecc73d1a 100644
--- a/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
@@ -4,7 +4,7 @@
 $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
+title: Synopsys DesignWare Universal Multi-Protocol Memory Controller
 
 maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
@@ -12,21 +12,22 @@ maintainers:
   - Michal Simek <michal.simek@xilinx.com>
 
 description: |
-  The ZynqMP DDR ECC controller has an optional ECC support in 64-bit and
-  32-bit bus width configurations.
+  Synopsys DesignWare Enhanced uMCTL2 DDR Memory Controller is capable of
+  working with the memory devices supporting 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 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.
+  For instance the ZynqMP DDR controller is based on the DW uMCTL2 v2.40a
+  controller. It has an optional SEC/DEC ECC support in 64- and 32-bits
+  bus width configurations.
 
 properties:
   compatible:
-    enum:
-      - snps,ddrc-3.80a
-      - xlnx,zynq-ddrc-a05
-      - xlnx,zynqmp-ddrc-2.40a
+    oneOf:
+      - description: Synopsys DW uMCTL2 DDR controller v3.80a
+        const: snps,ddrc-3.80a
+      - description: Xilinx ZynqMP DDR controller v2.40a
+        const: xlnx,zynqmp-ddrc-2.40a
 
   interrupts:
     maxItems: 1
@@ -37,40 +38,16 @@ properties:
 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
+  - interrupts
 
 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>;
-        };
+    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/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 <krzk@kernel.org>
+  - Manish Narani <manish.narani@xilinx.com>
+  - Michal Simek <michal.simek@xilinx.com>
+
+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..40e1a146ca61 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3087,6 +3087,7 @@ 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/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.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (13 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-16 18:46   ` Rob Herring
  2022-09-21 18:32   ` (subset) " Krzysztof Kozlowski
  2022-09-10 19:42 ` [PATCH v2 16/19] EDAC/synopsys: Detach Zynq DDRC controller support Serge Semin
                   ` (4 subsequent siblings)
  19 siblings, 2 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, Krzysztof Kozlowski,
	Manish Narani
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Dinh Nguyen,
	James Morse, Robert Richter, Rob Herring, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel, Krzysztof Kozlowski

The DT-schema name and the corresponding generic compatible string look
inappropriate in the current DW uMCTL2 DDRC DT-bindings:
1. DT-schema name contains undefined vendor-prefix. It's supposed to be
"snps", not "synopsys".
2. DT-schema name has "ecc" suffix. That is a device property, and has
nothing to do with the controller actual name.
3. The controller name is different. It's DW uMCTL2 DDRC. Just DDRC
doesn't identify the IP-core in subject.
4. There is no much point in using the IP-core version in the device name
since it can be retrieved from the corresponding device CSR. Moreover the
DW uMCTL2 DDRC driver doesn't differentiate the IP-core version at the
current state.

In order to fix all the inconsistencies described above we suggest to
rename the DT-schema to "snps,dw-umctl2-ddrc.yaml", deprecate the
compatible string "snps,ddrc-3.80a" and define a new generic device
name as "snps,dw-umctl2-ddrc".

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Changelog v2:
- This is a new patch created on v2 by detaching the DT-schema renaming
  from the previous patch in the series. (@Krzysztof)
- Fix the compatible string name so one would match the new DT-schema
  name.
---
 .../{synopsys,ddrc-ecc.yaml => snps,dw-umctl2-ddrc.yaml}   | 7 +++++--
 MAINTAINERS                                                | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)
 rename Documentation/devicetree/bindings/memory-controllers/{synopsys,ddrc-ecc.yaml => snps,dw-umctl2-ddrc.yaml} (83%)

diff --git a/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
similarity index 83%
rename from Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
rename to Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
index 0be8ecc73d1a..9212dfe6e956 100644
--- a/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/memory-controllers/synopsys,ddrc-ecc.yaml#
+$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
@@ -24,8 +24,11 @@ description: |
 properties:
   compatible:
     oneOf:
-      - description: Synopsys DW uMCTL2 DDR controller v3.80a
+      - deprecated: true
+        description: Synopsys DW uMCTL2 DDR controller v3.80a
         const: snps,ddrc-3.80a
+      - description: Synopsys DW uMCTL2 DDR controller
+        const: snps,dw-umctl2-ddrc
       - description: Xilinx ZynqMP DDR controller v2.40a
         const: xlnx,zynqmp-ddrc-2.40a
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 40e1a146ca61..357230710ed9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3087,6 +3087,7 @@ 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/i2c/xlnx,xps-iic-2.00.a.yaml
+F:	Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.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/
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 16/19] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (14 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-11  1:03   ` kernel test robot
  2022-09-10 19:42 ` [PATCH v2 17/19] EDAC/synopsys: Drop unused platform-specific setup API Serge Semin
                   ` (3 subsequent siblings)
  19 siblings, 1 reply; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>
---
 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 357230710ed9..551ae8910021 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 5088634bc213..f48b3a2938f7 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -31,68 +31,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
@@ -133,15 +81,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)
@@ -149,10 +93,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
@@ -160,6 +110,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
@@ -201,21 +159,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
@@ -346,61 +294,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.
@@ -623,37 +516,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.
@@ -686,30 +548,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.
@@ -722,9 +560,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);
 }
 
 /**
@@ -741,30 +579,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.
@@ -779,7 +593,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;
@@ -891,14 +705,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,
@@ -925,10 +731,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
@@ -1054,12 +856,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;
 }
@@ -1150,8 +952,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;
@@ -1292,7 +1094,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);
 	}
 
@@ -1346,7 +1148,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,
@@ -1388,13 +1190,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.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 17/19] EDAC/synopsys: Drop unused platform-specific setup API
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (15 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 16/19] EDAC/synopsys: Detach Zynq DDRC controller support Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 18/19] EDAC/synopsys: Unify the driver entities naming Serge Semin
                   ` (2 subsequent siblings)
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>

---

Changelog v2:
- Drop the no longer used "priv" pointer from the mc_init() function.
  (@tbot)
---
 drivers/edac/synopsys_edac.c | 197 +++++++++--------------------------
 1 file changed, 51 insertions(+), 146 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index f48b3a2938f7..26694f4fa162 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -32,9 +32,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 */
 
@@ -279,28 +277,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;
@@ -375,17 +365,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,
@@ -394,15 +378,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,
@@ -461,13 +440,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);
@@ -476,7 +453,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;
 
@@ -490,34 +467,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
@@ -525,7 +475,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;
 
@@ -549,14 +499,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;
 
@@ -580,7 +530,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
@@ -588,7 +538,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;
@@ -617,14 +567,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();
@@ -632,10 +579,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);
 		}
 	}
 }
@@ -651,10 +598,7 @@ static void init_csrows(struct mem_ctl_info *mci)
  */
 static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
 {
-	struct synps_edac_priv *priv;
-
 	mci->pdev = &pdev->dev;
-	priv = mci->pvt_info;
 	platform_set_drvdata(pdev, mci);
 
 	/* Initialize controller capabilities and configuration */
@@ -668,12 +612,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;
 
@@ -705,47 +644,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
 
 /**
@@ -1136,7 +1034,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;
 	}
@@ -1163,11 +1061,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) {
@@ -1177,17 +1073,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;
@@ -1209,12 +1101,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);
@@ -1223,6 +1113,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.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 18/19] EDAC/synopsys: Unify the driver entities naming
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (16 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 17/19] EDAC/synopsys: Drop unused platform-specific setup API Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2022-09-10 19:42 ` [PATCH v2 19/19] EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros Serge Semin
  2023-05-25  6:35 ` [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Alexander Stein
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>

---

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.

Changelog v2:
- Forgot to fix some of the SYNPS_ZYNQMP_IRQ_REGS macro utilizations.
  (@tbot)
---
 drivers/edac/synopsys_edac.c | 240 +++++++++++++++++------------------
 1 file changed, 120 insertions(+), 120 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 26694f4fa162..49bb28af448b 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.
@@ -18,23 +18,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
@@ -215,7 +215,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.
@@ -223,7 +223,7 @@
  * @bitpos:	Bit position.
  * @data:	Data causing the error.
  */
-struct ecc_error_info {
+struct snps_ecc_error_info {
 	u32 row;
 	u32 col;
 	u32 bank;
@@ -233,21 +233,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.
@@ -260,12 +260,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];
@@ -277,22 +277,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;
@@ -352,21 +352,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);
@@ -379,7 +379,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);
 
@@ -391,12 +391,12 @@ 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;
 
 	/* Enable UE/CE Interrupts */
-	if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS) {
+	if (priv->p_data->quirks & SNPS_ZYNQMP_IRQ_REGS) {
 		writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
 		       priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
 
@@ -412,12 +412,12 @@ 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;
 
 	/* Disable UE/CE Interrupts */
-	if (priv->p_data->quirks & SYNPS_ZYNQMP_IRQ_REGS) {
+	if (priv->p_data->quirks & SNPS_ZYNQMP_IRQ_REGS) {
 		writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
 		       priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
 
@@ -432,42 +432,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
@@ -475,7 +475,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;
 
@@ -499,14 +499,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;
 
@@ -516,11 +516,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;
 
@@ -530,7 +530,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
@@ -538,7 +538,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;
@@ -558,15 +558,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;
@@ -574,21 +574,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.
  *
@@ -596,7 +596,7 @@ 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)
 {
 	mci->pdev = &pdev->dev;
 	platform_set_drvdata(pdev, mci);
@@ -608,21 +608,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);
@@ -632,14 +633,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;
 }
@@ -647,13 +648,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;
@@ -711,7 +712,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",
@@ -725,12 +726,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;
 }
@@ -740,7 +741,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)
@@ -752,7 +753,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)
@@ -767,7 +768,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;
 
@@ -780,13 +781,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;
@@ -845,7 +846,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;
@@ -947,7 +948,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) &
@@ -959,7 +960,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;
@@ -969,7 +970,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] &
@@ -977,14 +978,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;
@@ -996,20 +997,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.
@@ -1017,11 +1018,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;
@@ -1034,20 +1035,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");
@@ -1059,9 +1060,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;
 
@@ -1073,13 +1074,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;
@@ -1091,20 +1092,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);
@@ -1113,32 +1114,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.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 19/19] EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (17 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 18/19] EDAC/synopsys: Unify the driver entities naming Serge Semin
@ 2022-09-10 19:42 ` Serge Semin
  2023-05-25  6:35 ` [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Alexander Stein
  19 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2022-09-10 19:42 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Michal Simek, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, devicetree,
	linux-arm-kernel, linux-edac, linux-kernel

Instead of using the very handy helpers denoted in the subject the driver
has been created with the open-coded {mask,shift} statements. It makes the
code bulky, prone to mistakes and much harder to read. Seeing there are
many places in the driver implementing the CSR fields get/set pattern
let's use the FIELD_GET()/FIELD_PREP() macros introduced in the kernel
specifically for that case. In addition we suggest to use the BIT() and
GENMASK() macros to generate the CSR flags/masks. While at it unify the
row, column, rank, bank and bank group macros names to be looking in the
same way as the fields of the snps_ecc_error_info structure.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/edac/synopsys_edac.c | 123 +++++++++++++++++------------------
 1 file changed, 60 insertions(+), 63 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 49bb28af448b..7833bcff3e2e 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/edac.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -91,22 +92,19 @@
 #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 DDR_MSTR_BUSWIDTH_MASK		GENMASK(13, 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_MASK		GENMASK(2, 0)
 #define ECC_CFG0_MODE_SECDED		0x4
 
 /* ECC status register definitions */
-#define ECC_STAT_UECNT_MASK		0xF0000
-#define ECC_STAT_UECNT_SHIFT		16
-#define ECC_STAT_CECNT_MASK		0xF00
-#define ECC_STAT_CECNT_SHIFT		8
-#define ECC_STAT_BITNUM_MASK		0x7F
+#define ECC_STAT_UE_MASK		GENMASK(23, 16)
+#define ECC_STAT_CE_MASK		GENMASK(15, 8)
+#define ECC_STAT_BITNUM_MASK		GENMASK(6, 0)
 
 /* ECC control/clear register definitions */
 #define ECC_CTRL_CLR_CE_ERR		BIT(0)
@@ -117,49 +115,41 @@
 #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
-#define ECC_ERRCNT_CECNT_MASK		0xFFFF
+#define ECC_ERRCNT_UECNT_MASK		GENMASK(31, 16)
+#define ECC_ERRCNT_CECNT_MASK		GENMASK(15, 0)
 
 /* DDR QOS Interrupt register definitions */
 #define DDR_QOS_IRQ_STAT_OFST		0x20200
-#define DDR_QOSUE_MASK			0x4
-#define	DDR_QOSCE_MASK			0x2
-#define	ECC_CE_UE_INTR_MASK		0x6
+#define DDR_QOSUE_MASK			BIT(2)
+#define DDR_QOSCE_MASK			BIT(1)
+#define ECC_CE_UE_INTR_MASK		(DDR_QOSUE_MASK | DDR_QOSCE_MASK)
 #define DDR_QOS_IRQ_EN_OFST		0x20208
 #define DDR_QOS_IRQ_DB_OFST		0x2020C
 
 /* ECC Corrected Error Register Mask and Shifts*/
-#define ECC_CEADDR0_RW_MASK		0x3FFFF
-#define ECC_CEADDR0_RNK_MASK		BIT(24)
-#define ECC_CEADDR1_BNKGRP_MASK		0x3000000
-#define ECC_CEADDR1_BNKNR_MASK		0x70000
-#define ECC_CEADDR1_COL_MASK		0xFFF
-#define ECC_CEADDR1_BNKGRP_SHIFT	24
-#define ECC_CEADDR1_BNKNR_SHIFT		16
+#define ECC_CEADDR0_RANK_MASK		GENMASK(27, 24)
+#define ECC_CEADDR0_ROW_MASK		GENMASK(17, 0)
+#define ECC_CEADDR1_BANKGRP_MASK	GENMASK(25, 24)
+#define ECC_CEADDR1_BANK_MASK		GENMASK(23, 16)
+#define ECC_CEADDR1_COL_MASK		GENMASK(11, 0)
 
 /* ECC Poison register shifts */
-#define ECC_POISON0_RANK_SHIFT		24
-#define ECC_POISON0_RANK_MASK		BIT(24)
-#define ECC_POISON0_COLUMN_SHIFT	0
-#define ECC_POISON0_COLUMN_MASK		0xFFF
-#define ECC_POISON1_BG_SHIFT		28
-#define ECC_POISON1_BG_MASK		0x30000000
-#define ECC_POISON1_BANKNR_SHIFT	24
-#define ECC_POISON1_BANKNR_MASK		0x7000000
-#define ECC_POISON1_ROW_SHIFT		0
-#define ECC_POISON1_ROW_MASK		0x3FFFF
+#define ECC_POISON0_RANK_MASK		GENMASK(27, 24)
+#define ECC_POISON0_COL_MASK		GENMASK(11, 0)
+#define ECC_POISON1_BANKGRP_MASK	GENMASK(29, 28)
+#define ECC_POISON1_BANK_MASK		GENMASK(26, 24)
+#define ECC_POISON1_ROW_MASK		GENMASK(17, 0)
 
 /* DDR Memory type defines */
-#define MEM_TYPE_DDR3			0x1
-#define MEM_TYPE_LPDDR3			0x8
-#define MEM_TYPE_DDR2			0x4
-#define MEM_TYPE_DDR4			0x10
-#define MEM_TYPE_LPDDR4			0x20
+#define MEM_TYPE_DDR3			BIT(0)
+#define MEM_TYPE_DDR2			BIT(2)
+#define MEM_TYPE_LPDDR3			BIT(3)
+#define MEM_TYPE_DDR4			BIT(4)
+#define MEM_TYPE_LPDDR4			BIT(5)
 
 /* DDRC ECC CE & UE poison mask */
-#define ECC_CEPOISON_MASK		0x3
-#define ECC_UEPOISON_MASK		0x1
+#define ECC_CEPOISON_MASK		GENMASK(1, 0)
+#define ECC_UEPOISON_MASK		BIT(0)
 
 /* DDRC Device config shifts/masks */
 #define DDR_MAX_ROW_SHIFT		18
@@ -304,38 +294,40 @@ static int snps_get_error_info(struct snps_edac_priv *priv)
 	if (!regval)
 		return 1;
 
-	p->ceinfo.bitpos = (regval & ECC_STAT_BITNUM_MASK);
+	p->ceinfo.bitpos = FIELD_GET(ECC_STAT_BITNUM_MASK, regval);
 
 	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;
+	p->ce_cnt = FIELD_GET(ECC_ERRCNT_CECNT_MASK, regval);
+	p->ue_cnt = FIELD_GET(ECC_ERRCNT_UECNT_MASK, regval);
 	if (!p->ce_cnt)
 		goto ue_err;
 
 	regval = readl(base + ECC_CEADDR0_OFST);
-	p->ceinfo.row = (regval & ECC_CEADDR0_RW_MASK);
+	p->ceinfo.row = FIELD_GET(ECC_CEADDR0_ROW_MASK, regval);
+
 	regval = readl(base + ECC_CEADDR1_OFST);
-	p->ceinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
-					ECC_CEADDR1_BNKNR_SHIFT;
-	p->ceinfo.bankgrp = (regval & ECC_CEADDR1_BNKGRP_MASK) >>
-					ECC_CEADDR1_BNKGRP_SHIFT;
-	p->ceinfo.col = (regval & ECC_CEADDR1_COL_MASK);
+	p->ceinfo.bank = FIELD_GET(ECC_CEADDR1_BANK_MASK, regval);
+	p->ceinfo.bankgrp = FIELD_GET(ECC_CEADDR1_BANKGRP_MASK, regval);
+	p->ceinfo.col = FIELD_GET(ECC_CEADDR1_COL_MASK, regval);
+
 	p->ceinfo.data = readl(base + ECC_CSYND0_OFST);
+
 	edac_dbg(2, "ECCCSYN0: 0x%08X ECCCSYN1: 0x%08X ECCCSYN2: 0x%08X\n",
 		 readl(base + ECC_CSYND0_OFST), readl(base + ECC_CSYND1_OFST),
 		 readl(base + ECC_CSYND2_OFST));
+
 ue_err:
 	if (!p->ue_cnt)
 		goto out;
 
 	regval = readl(base + ECC_UEADDR0_OFST);
-	p->ueinfo.row = (regval & ECC_CEADDR0_RW_MASK);
+	p->ueinfo.row = FIELD_GET(ECC_CEADDR0_ROW_MASK, regval);
+
 	regval = readl(base + ECC_UEADDR1_OFST);
-	p->ueinfo.bankgrp = (regval & ECC_CEADDR1_BNKGRP_MASK) >>
-					ECC_CEADDR1_BNKGRP_SHIFT;
-	p->ueinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
-					ECC_CEADDR1_BNKNR_SHIFT;
-	p->ueinfo.col = (regval & ECC_CEADDR1_COL_MASK);
+	p->ueinfo.bankgrp = FIELD_GET(ECC_CEADDR1_BANKGRP_MASK, regval);
+	p->ueinfo.bank = FIELD_GET(ECC_CEADDR1_BANK_MASK, regval);
+	p->ueinfo.col = FIELD_GET(ECC_CEADDR1_COL_MASK, regval);
+
 	p->ueinfo.data = readl(base + ECC_UESYND0_OFST);
 
 out:
@@ -510,7 +502,8 @@ static bool snps_get_ecc_state(void __iomem *base)
 {
 	u32 regval;
 
-	regval = readl(base + ECC_CFG0_OFST) & ECC_CFG0_MODE_MASK;
+	regval = readl(base + ECC_CFG0_OFST);
+	regval = FIELD_GET(ECC_CFG0_MODE_MASK, regval);
 
 	return (regval == ECC_CFG0_MODE_SECDED);
 }
@@ -697,13 +690,13 @@ static void snps_data_poison_setup(struct snps_edac_priv *priv)
 	if (priv->rank_shift[0])
 		rank = (hif_addr >> priv->rank_shift[0]) & BIT(0);
 
-	regval = (rank << ECC_POISON0_RANK_SHIFT) & ECC_POISON0_RANK_MASK;
-	regval |= (col << ECC_POISON0_COLUMN_SHIFT) & ECC_POISON0_COLUMN_MASK;
+	regval = FIELD_PREP(ECC_POISON0_RANK_MASK, rank) |
+		 FIELD_PREP(ECC_POISON0_COL_MASK, col);
 	writel(regval, priv->baseaddr + ECC_POISON0_OFST);
 
-	regval = (bankgrp << ECC_POISON1_BG_SHIFT) & ECC_POISON1_BG_MASK;
-	regval |= (bank << ECC_POISON1_BANKNR_SHIFT) & ECC_POISON1_BANKNR_MASK;
-	regval |= (row << ECC_POISON1_ROW_SHIFT) & ECC_POISON1_ROW_MASK;
+	regval = FIELD_PREP(ECC_POISON1_BANKGRP_MASK, bankgrp) |
+		 FIELD_PREP(ECC_POISON1_BANK_MASK, bank) |
+		 FIELD_PREP(ECC_POISON1_ROW_MASK, row);
 	writel(regval, priv->baseaddr + ECC_POISON1_OFST);
 }
 
@@ -742,10 +735,14 @@ static ssize_t inject_data_poison_show(struct device *dev,
 {
 	struct mem_ctl_info *mci = to_mci(dev);
 	struct snps_edac_priv *priv = mci->pvt_info;
+	const char *errstr;
+	u32 regval;
+
+	regval = readl(priv->baseaddr + ECC_CFG1_OFST);
+	errstr = FIELD_GET(ECC_CEPOISON_MASK, regval) == ECC_CEPOISON_MASK ?
+		 "Correctable Error" : "UnCorrectable Error";
 
-	return sprintf(data, "Data Poisoning: %s\n\r",
-			(((readl(priv->baseaddr + ECC_CFG1_OFST)) & 0x3) == 0x3)
-			? ("Correctable Error") : ("UnCorrectable Error"));
+	return sprintf(data, "Data Poisoning: %s\n\r", errstr);
 }
 
 static ssize_t inject_data_poison_store(struct device *dev,
@@ -852,7 +849,7 @@ static void snps_setup_column_address_map(struct snps_edac_priv *priv, u32 *addr
 	int index;
 
 	memtype = readl(priv->baseaddr + DDR_MSTR_OFST);
-	width = (memtype & DDR_MSTR_BUSWIDTH_MASK) >> DDR_MSTR_BUSWIDTH_SHIFT;
+	width = FIELD_GET(DDR_MSTR_BUSWIDTH_MASK, memtype);
 
 	priv->col_shift[0] = 0;
 	priv->col_shift[1] = 1;
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 16/19] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-09-10 19:42 ` [PATCH v2 16/19] EDAC/synopsys: Detach Zynq DDRC controller support Serge Semin
@ 2022-09-11  1:03   ` kernel test robot
  0 siblings, 0 replies; 31+ messages in thread
From: kernel test robot @ 2022-09-11  1:03 UTC (permalink / raw)
  To: Serge Semin, Rob Herring, Krzysztof Kozlowski, Michal Simek,
	Borislav Petkov, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Robert Richter
  Cc: kbuild-all, linux-media, Serge Semin, Alexey Malahov,
	Michail Ivanov, Pavel Parkhomenko, Punnaiah Choudary Kalluri,
	Manish Narani, Dinh Nguyen, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel

Hi Serge,

I love your patch! Perhaps something to improve:

[auto build test WARNING on ras/edac-for-next]
[also build test WARNING on krzk-mem-ctrl/for-next linus/master]
[cannot apply to xilinx-xlnx/master]
[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/20220911-034806
base:   https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git edac-for-next
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20220911/202209110838.nMRJWlc2-lkp@intel.com/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/f9c107260e46195172faf58ccea98f7de56eae08
        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/20220911-034806
        git checkout f9c107260e46195172faf58ccea98f7de56eae08
        # 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 <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/edac/zynq_edac.c: In function 'zynq_mc_init':
>> drivers/edac/zynq_edac.c:378:32: warning: variable 'priv' set but not used [-Wunused-but-set-variable]
     378 |         struct zynq_edac_priv *priv;
         |                                ^~~~
--
>> drivers/edac/zynq_edac.c:194: warning: expecting prototype for handle_error(). Prototype was for zynq_handle_error() instead
>> drivers/edac/zynq_edac.c:233: warning: expecting prototype for check_errors(). Prototype was for zynq_check_errors() instead


vim +/priv +378 drivers/edac/zynq_edac.c

   185	
   186	/**
   187	 * handle_error - Handle Correctable and Uncorrectable errors.
   188	 * @mci:	EDAC memory controller instance.
   189	 * @p:		Zynq ECC status structure.
   190	 *
   191	 * Handles ECC correctable and uncorrectable errors.
   192	 */
   193	static void zynq_handle_error(struct mem_ctl_info *mci, struct zynq_ecc_status *p)
 > 194	{
   195		struct zynq_edac_priv *priv = mci->pvt_info;
   196		struct zynq_ecc_error_info *pinf;
   197	
   198		if (p->ce_cnt) {
   199			pinf = &p->ceinfo;
   200	
   201			snprintf(priv->message, ZYNQ_EDAC_MSG_SIZE,
   202				 "Row %d Bank %d Col %d Bit %d Data 0x%08x",
   203				 pinf->row, pinf->bank, pinf->col,
   204				 pinf->bitpos, pinf->data);
   205	
   206			edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
   207					     p->ce_cnt, 0, 0, 0, 0, 0, -1,
   208					     priv->message, "");
   209		}
   210	
   211		if (p->ue_cnt) {
   212			pinf = &p->ueinfo;
   213	
   214			snprintf(priv->message, ZYNQ_EDAC_MSG_SIZE,
   215				 "Row %d Bank %d Col %d",
   216				 pinf->row, pinf->bank, pinf->col);
   217	
   218			edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
   219					     p->ue_cnt, 0, 0, 0, 0, 0, -1,
   220					     priv->message, "");
   221		}
   222	
   223		memset(p, 0, sizeof(*p));
   224	}
   225	
   226	/**
   227	 * check_errors - Check controller for ECC errors.
   228	 * @mci:	EDAC memory controller instance.
   229	 *
   230	 * Check and post ECC errors. Called by the polling thread.
   231	 */
   232	static void zynq_check_errors(struct mem_ctl_info *mci)
 > 233	{
   234		struct zynq_edac_priv *priv = mci->pvt_info;
   235		int status;
   236	
   237		status = zynq_get_error_info(priv);
   238		if (status)
   239			return;
   240	
   241		zynq_handle_error(mci, &priv->stat);
   242	}
   243	
   244	/**
   245	 * zynq_get_dtype - Return the controller memory width.
   246	 * @base:	DDR memory controller base address.
   247	 *
   248	 * Get the EDAC device type width appropriate for the current controller
   249	 * configuration.
   250	 *
   251	 * Return: a device type width enumeration.
   252	 */
   253	static enum dev_type zynq_get_dtype(const void __iomem *base)
   254	{
   255		enum dev_type dt;
   256		u32 width;
   257	
   258		width = readl(base + ZYNQ_CTRL_OFST);
   259		width = (width & ZYNQ_CTRL_BW_MASK) >> ZYNQ_CTRL_BW_SHIFT;
   260	
   261		switch (width) {
   262		case ZYNQ_DDRCTL_WDTH_16:
   263			dt = DEV_X2;
   264			break;
   265		case ZYNQ_DDRCTL_WDTH_32:
   266			dt = DEV_X4;
   267			break;
   268		default:
   269			dt = DEV_UNKNOWN;
   270		}
   271	
   272		return dt;
   273	}
   274	
   275	/**
   276	 * zynq_get_ecc_state - Return the controller ECC enable/disable status.
   277	 * @base:	DDR memory controller base address.
   278	 *
   279	 * Get the ECC enable/disable status of the controller.
   280	 *
   281	 * Return: true if enabled, otherwise false.
   282	 */
   283	static bool zynq_get_ecc_state(void __iomem *base)
   284	{
   285		enum dev_type dt;
   286		u32 ecctype;
   287	
   288		dt = zynq_get_dtype(base);
   289		if (dt == DEV_UNKNOWN)
   290			return false;
   291	
   292		ecctype = readl(base + ZYNQ_SCRUB_OFST) & ZYNQ_SCRUB_MODE_MASK;
   293		if ((ecctype == ZYNQ_SCRUB_MODE_SECDED) && (dt == DEV_X2))
   294			return true;
   295	
   296		return false;
   297	}
   298	
   299	/**
   300	 * zynq_get_memsize - Read the size of the attached memory device.
   301	 *
   302	 * Return: the memory size in bytes.
   303	 */
   304	static u32 zynq_get_memsize(void)
   305	{
   306		struct sysinfo inf;
   307	
   308		si_meminfo(&inf);
   309	
   310		return inf.totalram * inf.mem_unit;
   311	}
   312	
   313	/**
   314	 * zynq_get_mtype - Return the controller memory type.
   315	 * @base:	Zynq ECC status structure.
   316	 *
   317	 * Get the EDAC memory type appropriate for the current controller
   318	 * configuration.
   319	 *
   320	 * Return: a memory type enumeration.
   321	 */
   322	static enum mem_type zynq_get_mtype(const void __iomem *base)
   323	{
   324		enum mem_type mt;
   325		u32 memtype;
   326	
   327		memtype = readl(base + ZYNQ_T_ZQ_OFST);
   328	
   329		if (memtype & ZYNQ_T_ZQ_DDRMODE_MASK)
   330			mt = MEM_DDR3;
   331		else
   332			mt = MEM_DDR2;
   333	
   334		return mt;
   335	}
   336	
   337	/**
   338	 * zynq_init_csrows - Initialize the csrow data.
   339	 * @mci:	EDAC memory controller instance.
   340	 *
   341	 * Initialize the chip select rows associated with the EDAC memory
   342	 * controller instance.
   343	 */
   344	static void zynq_init_csrows(struct mem_ctl_info *mci)
   345	{
   346		struct zynq_edac_priv *priv = mci->pvt_info;
   347		struct csrow_info *csi;
   348		struct dimm_info *dimm;
   349		u32 size, row;
   350		int j;
   351	
   352		for (row = 0; row < mci->nr_csrows; row++) {
   353			csi = mci->csrows[row];
   354			size = zynq_get_memsize();
   355	
   356			for (j = 0; j < csi->nr_channels; j++) {
   357				dimm		= csi->channels[j]->dimm;
   358				dimm->edac_mode	= EDAC_SECDED;
   359				dimm->mtype	= zynq_get_mtype(priv->baseaddr);
   360				dimm->nr_pages	= (size >> PAGE_SHIFT) / csi->nr_channels;
   361				dimm->grain	= ZYNQ_EDAC_ERR_GRAIN;
   362				dimm->dtype	= zynq_get_dtype(priv->baseaddr);
   363			}
   364		}
   365	}
   366	
   367	/**
   368	 * zynq_mc_init - Initialize one driver instance.
   369	 * @mci:	EDAC memory controller instance.
   370	 * @pdev:	platform device.
   371	 *
   372	 * Perform initialization of the EDAC memory controller instance and
   373	 * related driver-private data associated with the memory controller the
   374	 * instance is bound to.
   375	 */
   376	static void zynq_mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
   377	{
 > 378		struct zynq_edac_priv *priv;
   379	
   380		mci->pdev = &pdev->dev;
   381		priv = mci->pvt_info;
   382		platform_set_drvdata(pdev, mci);
   383	
   384		/* Initialize controller capabilities and configuration */
   385		mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
   386		mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
   387		mci->scrub_cap = SCRUB_FLAG_HW_SRC;
   388		mci->scrub_mode = SCRUB_NONE;
   389	
   390		mci->edac_cap = EDAC_FLAG_SECDED;
   391		mci->ctl_name = "zynq_ddr_controller";
   392		mci->dev_name = ZYNQ_EDAC_MOD_STRING;
   393		mci->mod_name = ZYNQ_EDAC_MOD_VER;
   394	
   395		edac_op_state = EDAC_OPSTATE_POLL;
   396		mci->edac_check = zynq_check_errors;
   397	
   398		mci->ctl_page_to_phys = NULL;
   399	
   400		zynq_init_csrows(mci);
   401	}
   402	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH v2 05/19] EDAC/synopsys: Fix reading errors count before ECC status
  2022-09-10 19:42 ` [PATCH v2 05/19] EDAC/synopsys: Fix reading errors count before ECC status Serge Semin
@ 2022-09-12  5:26   ` Datta, Shubhrajyoti
  0 siblings, 0 replies; 31+ messages in thread
From: Datta, Shubhrajyoti @ 2022-09-12  5:26 UTC (permalink / raw)
  To: Serge Semin, Rob Herring, Krzysztof Kozlowski, Michal Simek,
	Borislav Petkov, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Robert Richter, Shubhrajyoti Datta
  Cc: Serge Semin, Alexey Malahov, Michail Ivanov, Pavel Parkhomenko,
	Punnaiah Choudary Kalluri, Manish Narani, Dinh Nguyen,
	Rob Herring, Krzysztof Kozlowski, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel, Borislav Petkov

[AMD Official Use Only - General]



> -----Original Message-----
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Sent: Sunday, September 11, 2022 1:12 AM
> To: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Michal Simek
> <michal.simek@xilinx.com>; Borislav Petkov <bp@alien8.de>; Mauro
> Carvalho Chehab <mchehab@kernel.org>; Tony Luck
> <tony.luck@intel.com>; James Morse <james.morse@arm.com>; Robert
> Richter <rric@kernel.org>; Shubhrajyoti Datta
> <shubhrajyoti.datta@xilinx.com>
> Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>; Serge Semin
> <fancer.lancer@gmail.com>; Alexey Malahov
> <Alexey.Malahov@baikalelectronics.ru>; Michail Ivanov
> <Michail.Ivanov@baikalelectronics.ru>; Pavel Parkhomenko
> <Pavel.Parkhomenko@baikalelectronics.ru>; Punnaiah Choudary Kalluri
> <punnaiah.choudary.kalluri@xilinx.com>; Manish Narani
> <manish.narani@xilinx.com>; Dinh Nguyen <dinguyen@kernel.org>; Rob
> Herring <robh@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org>; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-edac@vger.kernel.org; linux-
> kernel@vger.kernel.org; Borislav Petkov <bp@suse.de>
> Subject: [PATCH v2 05/19] EDAC/synopsys: Fix reading errors count before
> ECC status
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> 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 <Sergey.Semin@baikalelectronics.ru>


Reviewed-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

> ---
>  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 da1d90a87778..558d3b3e6864 100644
> --- a/drivers/edac/synopsys_edac.c
> +++ b/drivers/edac/synopsys_edac.c
> @@ -423,18 +423,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.37.2

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support
  2022-09-10 19:42 ` [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support Serge Semin
@ 2022-09-16 18:44   ` Rob Herring
  2022-09-21 18:32   ` (subset) " Krzysztof Kozlowski
  1 sibling, 0 replies; 31+ messages in thread
From: Rob Herring @ 2022-09-16 18:44 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Krzysztof Kozlowski, Robert Richter, Michal Simek,
	Rob Herring, linux-edac, Manish Narani, Krzysztof Kozlowski,
	Mauro Carvalho Chehab, devicetree, Punnaiah Choudary Kalluri,
	Pavel Parkhomenko, Michail Ivanov, linux-arm-kernel, Dinh Nguyen,
	Borislav Petkov, Alexey Malahov, Tony Luck, Krzysztof Kozlowski,
	linux-kernel, James Morse

On Sat, 10 Sep 2022 22:42:32 +0300, 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 support IRQs 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 in the same bindings.
> Let's split the DT-schema up.
> 
> Note since the synopsys,ddrc-ecc.yaml schema describes the Synopsys DW
> uMCTL2 DDR controller only, we need to accordingly fix the device
> descriptions.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ---
> 
> Changelog v2:
> - Move Synopsys DW uMCTL2 DDRC bindings file renaming to a separate
>   patch. (@Krzysztof)
> ---
>  .../memory-controllers/synopsys,ddrc-ecc.yaml | 63 ++++++-------------
>  .../xlnx,zynq-ddrc-a05.yaml                   | 38 +++++++++++
>  MAINTAINERS                                   |  1 +
>  3 files changed, 59 insertions(+), 43 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name
  2022-09-10 19:42 ` [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name Serge Semin
@ 2022-09-16 18:46   ` Rob Herring
  2022-09-21 18:32   ` (subset) " Krzysztof Kozlowski
  1 sibling, 0 replies; 31+ messages in thread
From: Rob Herring @ 2022-09-16 18:46 UTC (permalink / raw)
  To: Serge Semin
  Cc: Krzysztof Kozlowski, Rob Herring, Serge Semin, linux-arm-kernel,
	Dinh Nguyen, Manish Narani, linux-kernel,
	Punnaiah Choudary Kalluri, Krzysztof Kozlowski, Borislav Petkov,
	Michail Ivanov, Tony Luck, James Morse, Alexey Malahov,
	Pavel Parkhomenko, linux-edac, Krzysztof Kozlowski,
	Mauro Carvalho Chehab, Michal Simek, devicetree, Robert Richter

On Sat, 10 Sep 2022 22:42:33 +0300, Serge Semin wrote:
> The DT-schema name and the corresponding generic compatible string look
> inappropriate in the current DW uMCTL2 DDRC DT-bindings:
> 1. DT-schema name contains undefined vendor-prefix. It's supposed to be
> "snps", not "synopsys".
> 2. DT-schema name has "ecc" suffix. That is a device property, and has
> nothing to do with the controller actual name.
> 3. The controller name is different. It's DW uMCTL2 DDRC. Just DDRC
> doesn't identify the IP-core in subject.
> 4. There is no much point in using the IP-core version in the device name
> since it can be retrieved from the corresponding device CSR. Moreover the
> DW uMCTL2 DDRC driver doesn't differentiate the IP-core version at the
> current state.
> 
> In order to fix all the inconsistencies described above we suggest to
> rename the DT-schema to "snps,dw-umctl2-ddrc.yaml", deprecate the
> compatible string "snps,ddrc-3.80a" and define a new generic device
> name as "snps,dw-umctl2-ddrc".
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ---
> 
> Changelog v2:
> - This is a new patch created on v2 by detaching the DT-schema renaming
>   from the previous patch in the series. (@Krzysztof)
> - Fix the compatible string name so one would match the new DT-schema
>   name.
> ---
>  .../{synopsys,ddrc-ecc.yaml => snps,dw-umctl2-ddrc.yaml}   | 7 +++++--
>  MAINTAINERS                                                | 1 +
>  2 files changed, 6 insertions(+), 2 deletions(-)
>  rename Documentation/devicetree/bindings/memory-controllers/{synopsys,ddrc-ecc.yaml => snps,dw-umctl2-ddrc.yaml} (83%)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: (subset) [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support
  2022-09-10 19:42 ` [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support Serge Semin
  2022-09-16 18:44   ` Rob Herring
@ 2022-09-21 18:32   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 31+ messages in thread
From: Krzysztof Kozlowski @ 2022-09-21 18:32 UTC (permalink / raw)
  To: mchehab, robh+dt, Tony Luck, michal.simek, Sergey.Semin,
	manish.narani, krzysztof.kozlowski+dt, bp
  Cc: Krzysztof Kozlowski, linux-arm-kernel, punnaiah.choudary.kalluri,
	krzk, linux-edac, linux-kernel, rric, james.morse,
	Alexey.Malahov, dinguyen, robh, Michail.Ivanov, devicetree,
	Pavel.Parkhomenko, fancer.lancer

On Sat, 10 Sep 2022 22:42:32 +0300, 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 support IRQs 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 in the same bindings.
> Let's split the DT-schema up.
> 
> Note since the synopsys,ddrc-ecc.yaml schema describes the Synopsys DW
> uMCTL2 DDR controller only, we need to accordingly fix the device
> descriptions.
> 
> [...]

Applied, thanks!

[14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support
        https://git.kernel.org/krzk/linux-mem-ctrl/c/845081313632b6a27dff576cf102b4aecb4654cf

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: (subset) [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name
  2022-09-10 19:42 ` [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name Serge Semin
  2022-09-16 18:46   ` Rob Herring
@ 2022-09-21 18:32   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 31+ messages in thread
From: Krzysztof Kozlowski @ 2022-09-21 18:32 UTC (permalink / raw)
  To: mchehab, robh+dt, michal.simek, Tony Luck, Sergey.Semin,
	manish.narani, krzysztof.kozlowski+dt, bp
  Cc: Krzysztof Kozlowski, linux-arm-kernel, punnaiah.choudary.kalluri,
	krzk, linux-edac, linux-kernel, rric, james.morse,
	Alexey.Malahov, dinguyen, robh, Michail.Ivanov, devicetree,
	Pavel.Parkhomenko, fancer.lancer

On Sat, 10 Sep 2022 22:42:33 +0300, Serge Semin wrote:
> The DT-schema name and the corresponding generic compatible string look
> inappropriate in the current DW uMCTL2 DDRC DT-bindings:
> 1. DT-schema name contains undefined vendor-prefix. It's supposed to be
> "snps", not "synopsys".
> 2. DT-schema name has "ecc" suffix. That is a device property, and has
> nothing to do with the controller actual name.
> 3. The controller name is different. It's DW uMCTL2 DDRC. Just DDRC
> doesn't identify the IP-core in subject.
> 4. There is no much point in using the IP-core version in the device name
> since it can be retrieved from the corresponding device CSR. Moreover the
> DW uMCTL2 DDRC driver doesn't differentiate the IP-core version at the
> current state.
> 
> [...]

Applied, thanks!

[15/19] dt-bindings: memory: snps: Use more descriptive device name
        https://git.kernel.org/krzk/linux-mem-ctrl/c/9f60675a0f2e72f7967cc534f1c97f6da3b47392

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups
  2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (18 preceding siblings ...)
  2022-09-10 19:42 ` [PATCH v2 19/19] EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros Serge Semin
@ 2023-05-25  6:35 ` Alexander Stein
  2023-05-25 10:24   ` Serge Semin
  19 siblings, 1 reply; 31+ messages in thread
From: Alexander Stein @ 2023-05-25  6:35 UTC (permalink / raw)
  To: Michal Simek, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	Serge Semin
  Cc: Rob Herring, Krzysztof Kozlowski, Serge Semin, Serge Semin,
	Alexey Malahov, Michail Ivanov, Pavel Parkhomenko,
	Punnaiah Choudary Kalluri, Manish Narani, Dinh Nguyen,
	James Morse, Robert Richter, Rob Herring, Krzysztof Kozlowski,
	devicetree, linux-arm-kernel, linux-edac, linux-kernel

Hi,

Am Samstag, 10. September 2022, 21:42:18 CEST schrieb Serge Semin:
> 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: In-progress] EDAC/synopsys: Add generic DDRC info and address mapping
> Link:
> https://lore.kernel.org/linux-edac/20220822191427.27969-1-Sergey.Semin@baik
> alelectronics.ru [3: In-progress] EDAC/synopsys: Add generic resources and
> Baikal-T1 support Link:
> https://lore.kernel.org/linux-edac/20220822191957.28546-1-Sergey.Semin@baik
> alelectronics.ru
> 
> 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.

What is the state of this/these series? AFAICS only the DT patches got 
applied.
The synopsys driver got refactored quite a lot, so adding proper support for 
imx8mp from current state will conflict quite a lot.
It's a Synopsys V3.70a (without HW poisoning support!), refer to commit 
68b7cf5d91d4c ("arm64: dts: imx8mp: add ddr controller node to support EDAC on 
imx8mp").

Best regards,
Alexander

> 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.
> 
> Link:
> https://lore.kernel.org/linux-edac/20220822190730.27277-1-Sergey.Semin@baik
> alelectronics.ru/ Changelog 2:
> - Move Synopsys DW uMCTL2 DDRC bindings file renaming to a separate patch.
>   (@Krzysztof)
> - Introduce a new compatible string "snps,dw-umctl2-ddrc" matching the new
>   DT-schema name.
> - Forgot to fix some of the prefix of the SYNPS_ZYNQMP_IRQ_REGS macro
>   in several places. (@tbot)
> - Drop the no longer used "priv" pointer from the mc_init() function.
>   (@tbot)
> - Include "linux/bitfield.h" header file to get the FIELD_GET macro
>   definition. (@tbot)
> - Drop the already merged in patches:
> [PATCH 12/20] EDAC/mc: Replace spaces with tabs in memtype flags definition
> [PATCH 13/20] EDAC/mc: Drop duplicated dimm->nr_pages debug printout
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Michail Ivanov <Michail.Ivanov@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xilinx.com>
> Cc: Manish Narani <manish.narani@xilinx.com>
> Cc: Dinh Nguyen <dinguyen@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Robert Richter <rric@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-edac@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Serge Semin (19):
>   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: Init DIMM labels in MC registration method
>   EDAC/mc: Add MC unique index allocation procedure
>   dt-bindings: memory: snps: Detach Zynq DDRC controller support
>   dt-bindings: memory: snps: Use more descriptive device name
>   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                  |  56 ++
>  .../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                        | 135 ++-
>  drivers/edac/edac_mc.h                        |   4 +
>  drivers/edac/synopsys_edac.c                  | 903 ++++++------------
>  drivers/edac/zynq_edac.c                      | 504 ++++++++++
>  10 files changed, 1026 insertions(+), 703 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.ya
> ml delete mode 100644
> Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
> create mode 100644
> Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yam
> l create mode 100644 drivers/edac/zynq_edac.c


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups
  2023-05-25  6:35 ` [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Alexander Stein
@ 2023-05-25 10:24   ` Serge Semin
  2023-07-03 11:58     ` Serge Semin
  0 siblings, 1 reply; 31+ messages in thread
From: Serge Semin @ 2023-05-25 10:24 UTC (permalink / raw)
  To: Alexander Stein
  Cc: Michal Simek, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	Serge Semin, Rob Herring, Krzysztof Kozlowski, Alexey Malahov,
	Michail Ivanov, Pavel Parkhomenko, Punnaiah Choudary Kalluri,
	Manish Narani, Dinh Nguyen, James Morse, Robert Richter,
	Rob Herring, Krzysztof Kozlowski, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel

Hi Alexander,

On Thu, May 25, 2023 at 08:35:59AM +0200, Alexander Stein wrote:
> Hi,
> 
> Am Samstag, 10. September 2022, 21:42:18 CEST schrieb Serge Semin:
> > 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: In-progress] EDAC/synopsys: Add generic DDRC info and address mapping
> > Link:
> > https://lore.kernel.org/linux-edac/20220822191427.27969-1-Sergey.Semin@baik
> > alelectronics.ru [3: In-progress] EDAC/synopsys: Add generic resources and
> > Baikal-T1 support Link:
> > https://lore.kernel.org/linux-edac/20220822191957.28546-1-Sergey.Semin@baik
> > alelectronics.ru
> > 
> > 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.
> 
> What is the state of this/these series? AFAICS only the DT patches got 
> applied.
> The synopsys driver got refactored quite a lot, so adding proper support for 
> imx8mp from current state will conflict quite a lot.
> It's a Synopsys V3.70a (without HW poisoning support!), refer to commit 
> 68b7cf5d91d4c ("arm64: dts: imx8mp: add ddr controller node to support EDAC on 
> imx8mp").

I has been quite busy lately in DW PCIe RP/EP/eDMA driver and my own
deeds. But I am going to get back to this series within a month.
Could you meanwhile have a look at it (review and tests are very
welcome) and if possible start adding the imx8mp support based on the
suggested patchsets (see the lore links above)?

The main goal of my changes is to generalize the driver code and make
it working for the original Synopsys DW uMCTL2 device only with as much
compatibility as possible with the various IP-core configs. Most
likely adding your imx8mp DW uMCTL2 V3.70a on top of my changes will
be much easier than writing your own changes.

Note recently I've rebased my patches on top of the latest kernel
(6.4-rc1). Can't remember any major conflict so most likely it won't
cause much difficulties for you too. The resultant driver works well
for my system: DDR-phys memory space back-and-forth mapping, errors
poisoning scrubbing, etc.

-Serge(y)

> 
> Best regards,
> Alexander
> 
> > 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.
> > 
> > Link:
> > https://lore.kernel.org/linux-edac/20220822190730.27277-1-Sergey.Semin@baik
> > alelectronics.ru/ Changelog 2:
> > - Move Synopsys DW uMCTL2 DDRC bindings file renaming to a separate patch.
> >   (@Krzysztof)
> > - Introduce a new compatible string "snps,dw-umctl2-ddrc" matching the new
> >   DT-schema name.
> > - Forgot to fix some of the prefix of the SYNPS_ZYNQMP_IRQ_REGS macro
> >   in several places. (@tbot)
> > - Drop the no longer used "priv" pointer from the mc_init() function.
> >   (@tbot)
> > - Include "linux/bitfield.h" header file to get the FIELD_GET macro
> >   definition. (@tbot)
> > - Drop the already merged in patches:
> > [PATCH 12/20] EDAC/mc: Replace spaces with tabs in memtype flags definition
> > [PATCH 13/20] EDAC/mc: Drop duplicated dimm->nr_pages debug printout
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Michail Ivanov <Michail.Ivanov@baikalelectronics.ru>
> > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> > Cc: Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xilinx.com>
> > Cc: Manish Narani <manish.narani@xilinx.com>
> > Cc: Dinh Nguyen <dinguyen@kernel.org>
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Robert Richter <rric@kernel.org>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-edac@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > 
> > Serge Semin (19):
> >   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: Init DIMM labels in MC registration method
> >   EDAC/mc: Add MC unique index allocation procedure
> >   dt-bindings: memory: snps: Detach Zynq DDRC controller support
> >   dt-bindings: memory: snps: Use more descriptive device name
> >   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                  |  56 ++
> >  .../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                        | 135 ++-
> >  drivers/edac/edac_mc.h                        |   4 +
> >  drivers/edac/synopsys_edac.c                  | 903 ++++++------------
> >  drivers/edac/zynq_edac.c                      | 504 ++++++++++
> >  10 files changed, 1026 insertions(+), 703 deletions(-)
> >  create mode 100644
> > Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.ya
> > ml delete mode 100644
> > Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
> > create mode 100644
> > Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yam
> > l create mode 100644 drivers/edac/zynq_edac.c
> 
> 
> -- 
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
> 
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups
  2023-05-25 10:24   ` Serge Semin
@ 2023-07-03 11:58     ` Serge Semin
  2023-07-24 15:24       ` Serge Semin
  0 siblings, 1 reply; 31+ messages in thread
From: Serge Semin @ 2023-07-03 11:58 UTC (permalink / raw)
  To: Alexander Stein
  Cc: Michal Simek, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	Serge Semin, Rob Herring, Krzysztof Kozlowski, Alexey Malahov,
	Michail Ivanov, Pavel Parkhomenko, Punnaiah Choudary Kalluri,
	Manish Narani, Dinh Nguyen, James Morse, Robert Richter,
	Rob Herring, Krzysztof Kozlowski, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel

Hi Alexander

On Thu, May 25, 2023 at 01:24:37PM +0300, Serge Semin wrote:
> Hi Alexander,
> 
> On Thu, May 25, 2023 at 08:35:59AM +0200, Alexander Stein wrote:
> > Hi,
> > 
> > Am Samstag, 10. September 2022, 21:42:18 CEST schrieb Serge Semin:
> > > 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: In-progress] EDAC/synopsys: Add generic DDRC info and address mapping
> > > Link:
> > > https://lore.kernel.org/linux-edac/20220822191427.27969-1-Sergey.Semin@baik
> > > alelectronics.ru [3: In-progress] EDAC/synopsys: Add generic resources and
> > > Baikal-T1 support Link:
> > > https://lore.kernel.org/linux-edac/20220822191957.28546-1-Sergey.Semin@baik
> > > alelectronics.ru
> > > 
> > > 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.
> > 
> > What is the state of this/these series? AFAICS only the DT patches got 
> > applied.
> > The synopsys driver got refactored quite a lot, so adding proper support for 
> > imx8mp from current state will conflict quite a lot.
> > It's a Synopsys V3.70a (without HW poisoning support!), refer to commit 
> > 68b7cf5d91d4c ("arm64: dts: imx8mp: add ddr controller node to support EDAC on 
> > imx8mp").
> 
> I has been quite busy lately in DW PCIe RP/EP/eDMA driver and my own
> deeds. But I am going to get back to this series within a month.
> Could you meanwhile have a look at it (review and tests are very
> welcome) and if possible start adding the imx8mp support based on the
> suggested patchsets (see the lore links above)?
> 
> The main goal of my changes is to generalize the driver code and make
> it working for the original Synopsys DW uMCTL2 device only with as much
> compatibility as possible with the various IP-core configs. Most
> likely adding your imx8mp DW uMCTL2 V3.70a on top of my changes will
> be much easier than writing your own changes.
> 
> Note recently I've rebased my patches on top of the latest kernel
> (6.4-rc1). Can't remember any major conflict so most likely it won't
> cause much difficulties for you too. The resultant driver works well
> for my system: DDR-phys memory space back-and-forth mapping, errors
> poisoning scrubbing, etc.
> 
> -Serge(y)

Just to let you know. I need two more weeks to settle down my current
work and then I am getting back to this series. Once again sorry for
the delay.

-Serge(y)

> 
> > 
> > Best regards,
> > Alexander
> > 
> > > 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.
> > > 
> > > Link:
> > > https://lore.kernel.org/linux-edac/20220822190730.27277-1-Sergey.Semin@baik
> > > alelectronics.ru/ Changelog 2:
> > > - Move Synopsys DW uMCTL2 DDRC bindings file renaming to a separate patch.
> > >   (@Krzysztof)
> > > - Introduce a new compatible string "snps,dw-umctl2-ddrc" matching the new
> > >   DT-schema name.
> > > - Forgot to fix some of the prefix of the SYNPS_ZYNQMP_IRQ_REGS macro
> > >   in several places. (@tbot)
> > > - Drop the no longer used "priv" pointer from the mc_init() function.
> > >   (@tbot)
> > > - Include "linux/bitfield.h" header file to get the FIELD_GET macro
> > >   definition. (@tbot)
> > > - Drop the already merged in patches:
> > > [PATCH 12/20] EDAC/mc: Replace spaces with tabs in memtype flags definition
> > > [PATCH 13/20] EDAC/mc: Drop duplicated dimm->nr_pages debug printout
> > > 
> > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > > Cc: Michail Ivanov <Michail.Ivanov@baikalelectronics.ru>
> > > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> > > Cc: Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xilinx.com>
> > > Cc: Manish Narani <manish.narani@xilinx.com>
> > > Cc: Dinh Nguyen <dinguyen@kernel.org>
> > > Cc: James Morse <james.morse@arm.com>
> > > Cc: Robert Richter <rric@kernel.org>
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > Cc: devicetree@vger.kernel.org
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: linux-edac@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > 
> > > Serge Semin (19):
> > >   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: Init DIMM labels in MC registration method
> > >   EDAC/mc: Add MC unique index allocation procedure
> > >   dt-bindings: memory: snps: Detach Zynq DDRC controller support
> > >   dt-bindings: memory: snps: Use more descriptive device name
> > >   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                  |  56 ++
> > >  .../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                        | 135 ++-
> > >  drivers/edac/edac_mc.h                        |   4 +
> > >  drivers/edac/synopsys_edac.c                  | 903 ++++++------------
> > >  drivers/edac/zynq_edac.c                      | 504 ++++++++++
> > >  10 files changed, 1026 insertions(+), 703 deletions(-)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.ya
> > > ml delete mode 100644
> > > Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
> > > create mode 100644
> > > Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yam
> > > l create mode 100644 drivers/edac/zynq_edac.c
> > 
> > 
> > -- 
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > http://www.tq-group.com/
> > 
> > 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups
  2023-07-03 11:58     ` Serge Semin
@ 2023-07-24 15:24       ` Serge Semin
  2023-08-16 18:48         ` Serge Semin
  0 siblings, 1 reply; 31+ messages in thread
From: Serge Semin @ 2023-07-24 15:24 UTC (permalink / raw)
  To: Alexander Stein
  Cc: Michal Simek, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	Serge Semin, Rob Herring, Krzysztof Kozlowski, Alexey Malahov,
	Michail Ivanov, Pavel Parkhomenko, Punnaiah Choudary Kalluri,
	Manish Narani, Dinh Nguyen, James Morse, Robert Richter,
	Rob Herring, Krzysztof Kozlowski, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel

Hello Alexander

On Mon, Jul 03, 2023 at 02:58:06PM +0300, Serge Semin wrote:
> Hi Alexander
> 
> On Thu, May 25, 2023 at 01:24:37PM +0300, Serge Semin wrote:
> > Hi Alexander,
> > 
> > On Thu, May 25, 2023 at 08:35:59AM +0200, Alexander Stein wrote:
> > > Hi,
> > > 
> > > Am Samstag, 10. September 2022, 21:42:18 CEST schrieb Serge Semin:
> > > > 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: In-progress] EDAC/synopsys: Add generic DDRC info and address mapping
> > > > Link:
> > > > https://lore.kernel.org/linux-edac/20220822191427.27969-1-Sergey.Semin@baik
> > > > alelectronics.ru [3: In-progress] EDAC/synopsys: Add generic resources and
> > > > Baikal-T1 support Link:
> > > > https://lore.kernel.org/linux-edac/20220822191957.28546-1-Sergey.Semin@baik
> > > > alelectronics.ru
> > > > 
> > > > 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.
> > > 
> > > What is the state of this/these series? AFAICS only the DT patches got 
> > > applied.
> > > The synopsys driver got refactored quite a lot, so adding proper support for 
> > > imx8mp from current state will conflict quite a lot.
> > > It's a Synopsys V3.70a (without HW poisoning support!), refer to commit 
> > > 68b7cf5d91d4c ("arm64: dts: imx8mp: add ddr controller node to support EDAC on 
> > > imx8mp").
> > 
> > I has been quite busy lately in DW PCIe RP/EP/eDMA driver and my own
> > deeds. But I am going to get back to this series within a month.
> > Could you meanwhile have a look at it (review and tests are very
> > welcome) and if possible start adding the imx8mp support based on the
> > suggested patchsets (see the lore links above)?
> > 
> > The main goal of my changes is to generalize the driver code and make
> > it working for the original Synopsys DW uMCTL2 device only with as much
> > compatibility as possible with the various IP-core configs. Most
> > likely adding your imx8mp DW uMCTL2 V3.70a on top of my changes will
> > be much easier than writing your own changes.
> > 
> > Note recently I've rebased my patches on top of the latest kernel
> > (6.4-rc1). Can't remember any major conflict so most likely it won't
> > cause much difficulties for you too. The resultant driver works well
> > for my system: DDR-phys memory space back-and-forth mapping, errors
> > poisoning scrubbing, etc.
> > 
> > -Serge(y)
> 
> Just to let you know. I need two more weeks to settle down my current
> work and then I am getting back to this series. Once again sorry for
> the delay.

Terribly sorry for the delay again. But I need two more weeks to
finish my work. Then I'll finally get back to this patchset.

-Serge(y)

> 
> -Serge(y)
> 
> > 
> > > 
> > > Best regards,
> > > Alexander
> > > 
> > > > 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.
> > > > 
> > > > Link:
> > > > https://lore.kernel.org/linux-edac/20220822190730.27277-1-Sergey.Semin@baik
> > > > alelectronics.ru/ Changelog 2:
> > > > - Move Synopsys DW uMCTL2 DDRC bindings file renaming to a separate patch.
> > > >   (@Krzysztof)
> > > > - Introduce a new compatible string "snps,dw-umctl2-ddrc" matching the new
> > > >   DT-schema name.
> > > > - Forgot to fix some of the prefix of the SYNPS_ZYNQMP_IRQ_REGS macro
> > > >   in several places. (@tbot)
> > > > - Drop the no longer used "priv" pointer from the mc_init() function.
> > > >   (@tbot)
> > > > - Include "linux/bitfield.h" header file to get the FIELD_GET macro
> > > >   definition. (@tbot)
> > > > - Drop the already merged in patches:
> > > > [PATCH 12/20] EDAC/mc: Replace spaces with tabs in memtype flags definition
> > > > [PATCH 13/20] EDAC/mc: Drop duplicated dimm->nr_pages debug printout
> > > > 
> > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > > > Cc: Michail Ivanov <Michail.Ivanov@baikalelectronics.ru>
> > > > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> > > > Cc: Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xilinx.com>
> > > > Cc: Manish Narani <manish.narani@xilinx.com>
> > > > Cc: Dinh Nguyen <dinguyen@kernel.org>
> > > > Cc: James Morse <james.morse@arm.com>
> > > > Cc: Robert Richter <rric@kernel.org>
> > > > Cc: Rob Herring <robh@kernel.org>
> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > Cc: devicetree@vger.kernel.org
> > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > Cc: linux-edac@vger.kernel.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > > 
> > > > Serge Semin (19):
> > > >   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: Init DIMM labels in MC registration method
> > > >   EDAC/mc: Add MC unique index allocation procedure
> > > >   dt-bindings: memory: snps: Detach Zynq DDRC controller support
> > > >   dt-bindings: memory: snps: Use more descriptive device name
> > > >   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                  |  56 ++
> > > >  .../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                        | 135 ++-
> > > >  drivers/edac/edac_mc.h                        |   4 +
> > > >  drivers/edac/synopsys_edac.c                  | 903 ++++++------------
> > > >  drivers/edac/zynq_edac.c                      | 504 ++++++++++
> > > >  10 files changed, 1026 insertions(+), 703 deletions(-)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.ya
> > > > ml delete mode 100644
> > > > Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
> > > > create mode 100644
> > > > Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yam
> > > > l create mode 100644 drivers/edac/zynq_edac.c
> > > 
> > > 
> > > -- 
> > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > Amtsgericht München, HRB 105018
> > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > http://www.tq-group.com/
> > > 
> > > 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups
  2023-07-24 15:24       ` Serge Semin
@ 2023-08-16 18:48         ` Serge Semin
  0 siblings, 0 replies; 31+ messages in thread
From: Serge Semin @ 2023-08-16 18:48 UTC (permalink / raw)
  To: Alexander Stein
  Cc: Michal Simek, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	Serge Semin, Rob Herring, Krzysztof Kozlowski, Alexey Malahov,
	Michail Ivanov, Pavel Parkhomenko, Punnaiah Choudary Kalluri,
	Manish Narani, Dinh Nguyen, James Morse, Robert Richter,
	Rob Herring, Krzysztof Kozlowski, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel

Hi Alexander

On Mon, Jul 24, 2023 at 06:24:20PM +0300, Serge Semin wrote:
> Hello Alexander
> 
> On Mon, Jul 03, 2023 at 02:58:06PM +0300, Serge Semin wrote:
> > Hi Alexander
> > 
> > On Thu, May 25, 2023 at 01:24:37PM +0300, Serge Semin wrote:
> > > Hi Alexander,
> > > 
> > > On Thu, May 25, 2023 at 08:35:59AM +0200, Alexander Stein wrote:
> > > > Hi,
> > > > 
> > > > Am Samstag, 10. September 2022, 21:42:18 CEST schrieb Serge Semin:
> > > > > 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: In-progress] EDAC/synopsys: Add generic DDRC info and address mapping
> > > > > Link:
> > > > > https://lore.kernel.org/linux-edac/20220822191427.27969-1-Sergey.Semin@baik
> > > > > alelectronics.ru [3: In-progress] EDAC/synopsys: Add generic resources and
> > > > > Baikal-T1 support Link:
> > > > > https://lore.kernel.org/linux-edac/20220822191957.28546-1-Sergey.Semin@baik
> > > > > alelectronics.ru
> > > > > 
> > > > > 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.
> > > > 
> > > > What is the state of this/these series? AFAICS only the DT patches got 
> > > > applied.
> > > > The synopsys driver got refactored quite a lot, so adding proper support for 
> > > > imx8mp from current state will conflict quite a lot.
> > > > It's a Synopsys V3.70a (without HW poisoning support!), refer to commit 
> > > > 68b7cf5d91d4c ("arm64: dts: imx8mp: add ddr controller node to support EDAC on 
> > > > imx8mp").
> > > 
> > > I has been quite busy lately in DW PCIe RP/EP/eDMA driver and my own
> > > deeds. But I am going to get back to this series within a month.
> > > Could you meanwhile have a look at it (review and tests are very
> > > welcome) and if possible start adding the imx8mp support based on the
> > > suggested patchsets (see the lore links above)?
> > > 
> > > The main goal of my changes is to generalize the driver code and make
> > > it working for the original Synopsys DW uMCTL2 device only with as much
> > > compatibility as possible with the various IP-core configs. Most
> > > likely adding your imx8mp DW uMCTL2 V3.70a on top of my changes will
> > > be much easier than writing your own changes.
> > > 
> > > Note recently I've rebased my patches on top of the latest kernel
> > > (6.4-rc1). Can't remember any major conflict so most likely it won't
> > > cause much difficulties for you too. The resultant driver works well
> > > for my system: DDR-phys memory space back-and-forth mapping, errors
> > > poisoning scrubbing, etc.
> > > 
> > > -Serge(y)
> > 
> > Just to let you know. I need two more weeks to settle down my current
> > work and then I am getting back to this series. Once again sorry for
> > the delay.
> 
> Terribly sorry for the delay again. But I need two more weeks to
> finish my work. Then I'll finally get back to this patchset.

I am finally partly done with my current tasks and able to get back to
this patchset. I'll add you to the Cc-list of the series so you would
be aware of the progress. This and another my Synopsys EDAC-related
patchsets have been successfully rebased onto the latest kernel source
tree (it's 6.5-rc6). All my smoke tests didn't reveal any problem.

What is next. I'll answer to the latest Borislav comments, then fix the
issues he raised there and resend my patchsets. Hopefully I'll manage to
do that on this week. Then the standard kernel patches review cycle shall
proceed.

Sorry once again for the huge delay and thanks for your patience.

-Serge(y)

> 
> -Serge(y)
> 
> > 
> > -Serge(y)
> > 
> > > 
> > > > 
> > > > Best regards,
> > > > Alexander
> > > > 
> > > > > 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.
> > > > > 
> > > > > Link:
> > > > > https://lore.kernel.org/linux-edac/20220822190730.27277-1-Sergey.Semin@baik
> > > > > alelectronics.ru/ Changelog 2:
> > > > > - Move Synopsys DW uMCTL2 DDRC bindings file renaming to a separate patch.
> > > > >   (@Krzysztof)
> > > > > - Introduce a new compatible string "snps,dw-umctl2-ddrc" matching the new
> > > > >   DT-schema name.
> > > > > - Forgot to fix some of the prefix of the SYNPS_ZYNQMP_IRQ_REGS macro
> > > > >   in several places. (@tbot)
> > > > > - Drop the no longer used "priv" pointer from the mc_init() function.
> > > > >   (@tbot)
> > > > > - Include "linux/bitfield.h" header file to get the FIELD_GET macro
> > > > >   definition. (@tbot)
> > > > > - Drop the already merged in patches:
> > > > > [PATCH 12/20] EDAC/mc: Replace spaces with tabs in memtype flags definition
> > > > > [PATCH 13/20] EDAC/mc: Drop duplicated dimm->nr_pages debug printout
> > > > > 
> > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > > > > Cc: Michail Ivanov <Michail.Ivanov@baikalelectronics.ru>
> > > > > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> > > > > Cc: Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xilinx.com>
> > > > > Cc: Manish Narani <manish.narani@xilinx.com>
> > > > > Cc: Dinh Nguyen <dinguyen@kernel.org>
> > > > > Cc: James Morse <james.morse@arm.com>
> > > > > Cc: Robert Richter <rric@kernel.org>
> > > > > Cc: Rob Herring <robh@kernel.org>
> > > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > > Cc: devicetree@vger.kernel.org
> > > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > > Cc: linux-edac@vger.kernel.org
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > 
> > > > > Serge Semin (19):
> > > > >   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: Init DIMM labels in MC registration method
> > > > >   EDAC/mc: Add MC unique index allocation procedure
> > > > >   dt-bindings: memory: snps: Detach Zynq DDRC controller support
> > > > >   dt-bindings: memory: snps: Use more descriptive device name
> > > > >   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                  |  56 ++
> > > > >  .../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                        | 135 ++-
> > > > >  drivers/edac/edac_mc.h                        |   4 +
> > > > >  drivers/edac/synopsys_edac.c                  | 903 ++++++------------
> > > > >  drivers/edac/zynq_edac.c                      | 504 ++++++++++
> > > > >  10 files changed, 1026 insertions(+), 703 deletions(-)
> > > > >  create mode 100644
> > > > > Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.ya
> > > > > ml delete mode 100644
> > > > > Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
> > > > > create mode 100644
> > > > > Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yam
> > > > > l create mode 100644 drivers/edac/zynq_edac.c
> > > > 
> > > > 
> > > > -- 
> > > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > > Amtsgericht München, HRB 105018
> > > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > > http://www.tq-group.com/
> > > > 
> > > > 

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2023-08-16 18:49 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-10 19:42 [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
2022-09-10 19:42 ` [PATCH v2 01/19] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin
2022-09-10 19:42 ` [PATCH v2 02/19] EDAC/synopsys: Fix generic device type detection procedure Serge Semin
2022-09-10 19:42 ` [PATCH v2 03/19] EDAC/synopsys: Fix mci->scrub_cap field setting Serge Semin
2022-09-10 19:42 ` [PATCH v2 04/19] EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse Serge Semin
2022-09-10 19:42 ` [PATCH v2 05/19] EDAC/synopsys: Fix reading errors count before ECC status Serge Semin
2022-09-12  5:26   ` Datta, Shubhrajyoti
2022-09-10 19:42 ` [PATCH v2 06/19] EDAC/synopsys: Use platform device devm ioremap method Serge Semin
2022-09-10 19:42 ` [PATCH v2 07/19] EDAC/synopsys: Drop internal CE and UE counters Serge Semin
2022-09-10 19:42 ` [PATCH v2 08/19] EDAC/synopsys: Drop local to_mci macro implementation Serge Semin
2022-09-10 19:42 ` [PATCH v2 09/19] EDAC/synopsys: Drop struct ecc_error_info.blknr field Serge Semin
2022-09-10 19:42 ` [PATCH v2 10/19] EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name Serge Semin
2022-09-10 19:42 ` [PATCH v2 11/19] EDAC/synopsys: Drop redundant info from error message Serge Semin
2022-09-10 19:42 ` [PATCH v2 12/19] EDAC/mc: Init DIMM labels in MC registration method Serge Semin
2022-09-10 19:42 ` [PATCH v2 13/19] EDAC/mc: Add MC unique index allocation procedure Serge Semin
2022-09-10 19:42 ` [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support Serge Semin
2022-09-16 18:44   ` Rob Herring
2022-09-21 18:32   ` (subset) " Krzysztof Kozlowski
2022-09-10 19:42 ` [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name Serge Semin
2022-09-16 18:46   ` Rob Herring
2022-09-21 18:32   ` (subset) " Krzysztof Kozlowski
2022-09-10 19:42 ` [PATCH v2 16/19] EDAC/synopsys: Detach Zynq DDRC controller support Serge Semin
2022-09-11  1:03   ` kernel test robot
2022-09-10 19:42 ` [PATCH v2 17/19] EDAC/synopsys: Drop unused platform-specific setup API Serge Semin
2022-09-10 19:42 ` [PATCH v2 18/19] EDAC/synopsys: Unify the driver entities naming Serge Semin
2022-09-10 19:42 ` [PATCH v2 19/19] EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros Serge Semin
2023-05-25  6:35 ` [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups Alexander Stein
2023-05-25 10:24   ` Serge Semin
2023-07-03 11:58     ` Serge Semin
2023-07-24 15:24       ` Serge Semin
2023-08-16 18:48         ` Serge Semin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).