linux-edac.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups
@ 2022-09-29 23:26 Serge Semin
  2022-09-29 23:26 ` [PATCH RESEND v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin
                   ` (18 more replies)
  0 siblings, 19 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:26 UTC (permalink / raw)
  To: 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, 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/20220910195007.11027-1-Sergey.Semin@baikalelectronics.ru
[3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 support
Link: https://lore.kernel.org/linux-edac/20220910195659.11843-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 v2:
- 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

Link: https://lore.kernel.org/linux-edac/20220910194237.10142-1-Sergey.Semin@baikalelectronics.ru
Changelog v3:
- Drop the no longer used "priv" pointer from the mc_init() function.
  (@tbot)
- Drop the merged in patches:
[PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support
[PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name
  (@Krzysztof)

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+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.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 (17):
  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
  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

 MAINTAINERS                  |   1 +
 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     | 501 +++++++++++++++++++
 7 files changed, 927 insertions(+), 627 deletions(-)
 create mode 100644 drivers/edac/zynq_edac.c

-- 
2.37.3



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

* [PATCH RESEND v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
@ 2022-09-29 23:26 ` Serge Semin
  2022-10-31 17:24   ` Borislav Petkov
  2022-09-29 23:26 ` [PATCH RESEND v3 02/17] EDAC/synopsys: Fix generic device type detection procedure Serge Semin
                   ` (17 subsequent siblings)
  18 siblings, 1 reply; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:26 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 02/17] EDAC/synopsys: Fix generic device type detection procedure
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
  2022-09-29 23:26 ` [PATCH RESEND v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin
@ 2022-09-29 23:26 ` Serge Semin
  2022-09-29 23:26 ` [PATCH RESEND v3 03/17] EDAC/synopsys: Fix mci->scrub_cap field setting Serge Semin
                   ` (16 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:26 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 03/17] EDAC/synopsys: Fix mci->scrub_cap field setting
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
  2022-09-29 23:26 ` [PATCH RESEND v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin
  2022-09-29 23:26 ` [PATCH RESEND v3 02/17] EDAC/synopsys: Fix generic device type detection procedure Serge Semin
@ 2022-09-29 23:26 ` Serge Semin
  2022-09-29 23:26 ` [PATCH RESEND v3 04/17] EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse Serge Semin
                   ` (15 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:26 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 04/17] EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (2 preceding siblings ...)
  2022-09-29 23:26 ` [PATCH RESEND v3 03/17] EDAC/synopsys: Fix mci->scrub_cap field setting Serge Semin
@ 2022-09-29 23:26 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 05/17] EDAC/synopsys: Fix reading errors count before ECC status Serge Semin
                   ` (14 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:26 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 05/17] EDAC/synopsys: Fix reading errors count before ECC status
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (3 preceding siblings ...)
  2022-09-29 23:26 ` [PATCH RESEND v3 04/17] EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 06/17] EDAC/synopsys: Use platform device devm ioremap method Serge Semin
                   ` (13 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel, Shubhrajyoti Datta, 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>
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.3



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

* [PATCH RESEND v3 06/17] EDAC/synopsys: Use platform device devm ioremap method
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (4 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 05/17] EDAC/synopsys: Fix reading errors count before ECC status Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 07/17] EDAC/synopsys: Drop internal CE and UE counters Serge Semin
                   ` (12 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 07/17] EDAC/synopsys: Drop internal CE and UE counters
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (5 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 06/17] EDAC/synopsys: Use platform device devm ioremap method Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 08/17] EDAC/synopsys: Drop local to_mci macro implementation Serge Semin
                   ` (11 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 08/17] EDAC/synopsys: Drop local to_mci macro implementation
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (6 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 07/17] EDAC/synopsys: Drop internal CE and UE counters Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 09/17] EDAC/synopsys: Drop struct ecc_error_info.blknr field Serge Semin
                   ` (10 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 09/17] EDAC/synopsys: Drop struct ecc_error_info.blknr field
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (7 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 08/17] EDAC/synopsys: Drop local to_mci macro implementation Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 10/17] EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name Serge Semin
                   ` (9 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 10/17] EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (8 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 09/17] EDAC/synopsys: Drop struct ecc_error_info.blknr field Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 11/17] EDAC/synopsys: Drop redundant info from error message Serge Semin
                   ` (8 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 11/17] EDAC/synopsys: Drop redundant info from error message
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (9 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 10/17] EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 12/17] EDAC/mc: Init DIMM labels in MC registration method Serge Semin
                   ` (7 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 12/17] EDAC/mc: Init DIMM labels in MC registration method
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (10 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 11/17] EDAC/synopsys: Drop redundant info from error message Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure Serge Semin
                   ` (6 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (11 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 12/17] EDAC/mc: Init DIMM labels in MC registration method Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-10-12 17:29   ` Borislav Petkov
  2022-09-29 23:27 ` [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support Serge Semin
                   ` (5 subsequent siblings)
  18 siblings, 1 reply; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (12 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-30 14:42   ` Borislav Petkov
  2022-09-29 23:27 ` [PATCH RESEND v3 15/17] EDAC/synopsys: Drop unused platform-specific setup API Serge Semin
                   ` (4 subsequent siblings)
  18 siblings, 1 reply; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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>

---

Changelog v3:
- Drop the no longer used "priv" pointer from the zynq_mc_init() function.
  (@tbot)
---
 MAINTAINERS                  |   1 +
 drivers/edac/Kconfig         |   9 +-
 drivers/edac/Makefile        |   1 +
 drivers/edac/synopsys_edac.c | 271 +++----------------
 drivers/edac/zynq_edac.c     | 501 +++++++++++++++++++++++++++++++++++
 5 files changed, 544 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..0781e69e019c
--- /dev/null
+++ b/drivers/edac/zynq_edac.c
@@ -0,0 +1,501 @@
+// 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)
+{
+	mci->pdev = &pdev->dev;
+	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.3



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

* [PATCH RESEND v3 15/17] EDAC/synopsys: Drop unused platform-specific setup API
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (13 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 16/17] EDAC/synopsys: Unify the driver entities naming Serge Semin
                   ` (3 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 16/17] EDAC/synopsys: Unify the driver entities naming
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (14 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 15/17] EDAC/synopsys: Drop unused platform-specific setup API Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-29 23:27 ` [PATCH RESEND v3 17/17] EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros Serge Semin
                   ` (2 subsequent siblings)
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* [PATCH RESEND v3 17/17] EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (15 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 16/17] EDAC/synopsys: Unify the driver entities naming Serge Semin
@ 2022-09-29 23:27 ` Serge Semin
  2022-09-30 14:29 ` [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Borislav Petkov
  2023-08-18 23:06 ` Serge Semin
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-09-29 23:27 UTC (permalink / raw)
  To: 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, 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.3



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

* Re: [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (16 preceding siblings ...)
  2022-09-29 23:27 ` [PATCH RESEND v3 17/17] EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros Serge Semin
@ 2022-09-30 14:29 ` Borislav Petkov
  2022-10-06  7:13   ` Michal Simek
  2023-08-18 23:06 ` Serge Semin
  18 siblings, 1 reply; 38+ messages in thread
From: Borislav Petkov @ 2022-09-30 14:29 UTC (permalink / raw)
  To: Serge Semin, Michal Simek
  Cc: Mauro Carvalho Chehab, Tony Luck, Serge Semin, Alexey Malahov,
	Michail Ivanov, Pavel Parkhomenko, Punnaiah Choudary Kalluri,
	Manish Narani, Dinh Nguyen, James Morse, Robert Richter,
	Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Fri, Sep 30, 2022 at 02:26:55AM +0300, Serge Semin wrote:
> 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/20220910195007.11027-1-Sergey.Semin@baikalelectronics.ru
> [3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 support
> Link: https://lore.kernel.org/linux-edac/20220910195659.11843-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.

So I'd take a look slowly but I'd like for this driver's maintainer -
Michal Simek - to have a look first.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-09-29 23:27 ` [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support Serge Semin
@ 2022-09-30 14:42   ` Borislav Petkov
  2022-10-06 12:17     ` Serge Semin
  0 siblings, 1 reply; 38+ messages in thread
From: Borislav Petkov @ 2022-09-30 14:42 UTC (permalink / raw)
  To: Serge Semin
  Cc: Michal Simek, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Robert Richter, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Fri, Sep 30, 2022 at 02:27:09AM +0300, Serge Semin wrote:
> 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].

Yeah, no, you need to give more concrete details here.

Why exactly is this a problem?

Are you saying that if synopsys puts out 10 incompatible memory
controllers, we should have 10 separate EDAC drivers?

Hell no.

synopsys_edac.c is not a huge file and the probe logic which matches
which synps_platform_data to load is pretty straight-forward to me.

But maybe I'm missing something so please explain in detail what the
actual problems with this are?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups
  2022-09-30 14:29 ` [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Borislav Petkov
@ 2022-10-06  7:13   ` Michal Simek
  0 siblings, 0 replies; 38+ messages in thread
From: Michal Simek @ 2022-10-06  7:13 UTC (permalink / raw)
  To: Borislav Petkov, Serge Semin, Michal Simek, Shubhrajyoti Datta
  Cc: Mauro Carvalho Chehab, Tony Luck, Serge Semin, Alexey Malahov,
	Michail Ivanov, Pavel Parkhomenko, Punnaiah Choudary Kalluri,
	Manish Narani, Dinh Nguyen, James Morse, Robert Richter,
	Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

Hi Boris,

On 9/30/22 16:29, Borislav Petkov wrote:
> 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.
> 
> 
> On Fri, Sep 30, 2022 at 02:26:55AM +0300, Serge Semin wrote:
>> 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/20220910195007.11027-1-Sergey.Semin@baikalelectronics.ru
>> [3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 support
>> Link: https://lore.kernel.org/linux-edac/20220910195659.11843-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.
> 
> So I'd take a look slowly but I'd like for this driver's maintainer -
> Michal Simek - to have a look first.

It is assigned to my fragment for quite a long time. Origin author Punnaiah is 
no longer working on this driver. But we have new owner Shubhrajyoti who will 
take a look this patchset and test it on Xilinx HW.
Shubhrajyoti: please take a look at this series.

Thanks,
Michal


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

* Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-09-30 14:42   ` Borislav Petkov
@ 2022-10-06 12:17     ` Serge Semin
  2022-10-06 13:25       ` Borislav Petkov
  0 siblings, 1 reply; 38+ messages in thread
From: Serge Semin @ 2022-10-06 12:17 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Fri, Sep 30, 2022 at 04:42:31PM +0200, Borislav Petkov wrote:
> On Fri, Sep 30, 2022 at 02:27:09AM +0300, Serge Semin wrote:
> > 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].
> 

> Yeah, no, you need to give more concrete details here.
> 
> Why exactly is this a problem?

In general because it needlessly overcomplicates the driver, worsen
it scalability, maintainability and readability, makes it much harder
to add new controller features. Moreover even if you still able to add
some more features support the driver will get to be more and more messy
(as Michal correctly said in the original thread [1]). It will get to
be messy since you'll need to add more if-flag-else conditionals or
define new device-specific callbacks to select the right code path for
different controllers; you'll need to define some private data
specific to one controller and unused in another. All of that you
already have in the driver and all of that would be unneeded should the
driver author have followed the standard kernel bus-device-driver model.

Regarding this particular case. This series and two more patchsets
mentioned in the cover letter are about extending the Synopsys uMCTL2
DDRC controller functionality support in the driver. Here is just the
most significant part of the being added features:
1. DW uCMTL2 IP-core parameters auto-detection (DDR bus-width, frequency
ratio, burst-length, ECC-mode, etc).
2. Common SDRAM<->phys address translation.
3. Multi-ranked memory support.
4. ECC-scrubber and Scrubber IRQ support.
5. Individual IRQ signals support.
6. DFI alert_n IRQ support.
7. Add reference clock sources support.

None of the above can be utilized for the Zynq A05 DDR controller, but
is of the Synopsys uMCTL2 DDRC specific. Seeing the controllers are
not even of the same vendor and are absolutely incompatible,
considering all the new features, moving the Zynq A05 DDRC support
away to another driver was the best choice in order to create a
coherent and easy-to-read code, provide simple-enough patches for
review. The standard kernel bus-device-driver model is perfectly
suitable to differentiate these controllers. There is no point in
creating an additional abstraction.

> 
> Are you saying that if synopsys puts out 10 incompatible memory
> controllers, we should have 10 separate EDAC drivers?

I said "absolutely different controllers" in the patchlog. The level
of incompatibility can be different and can vary from case to case. In
this particular case there is nothing in common in Synopsys uMCTL2
DDRC and Zynq A05 DDRC. So if there are two absolutely different
devices at consideration then in general they should be handled in
different drivers as per the kernel bus-device-driver model.

Note having absolutely different devices detectable on the same
platform doesn't mean they should be handled by the same driver
otherwise the kernel would have had tons of common platform-drivers
instead of the device-specific ones. Even in current synopsys EDAC
driver state (without my patches applied) should you detach the Zynq
A05 DDRC support to another driver, the code would have got to be much
easier to read and maintain.

I one more time draw your attention to the fact that the original
reason of having both Synopsys uMCTL2 and Zynq A05 DDRC support in the
same driver has never been implemented [2]. See edac_mc_alloc()
is always called with index zero in the driver. Even despite of that I
still provide a way to solve the problem denoted in [2] in the patch
"EDAC/mc: Add MC unique index allocation procedure" in this patchset.

> 
> Hell no.
> 

> synopsys_edac.c is not a huge file and the probe logic which matches
> which synps_platform_data to load is pretty straight-forward to me.

The synps_platform_data interface is so abstractive so you could have
added any basic EDAC device support to the driver. It doesn't mean you
should even if the devices can be detected on the same platform,
especially if you expect the code to evolve further being extended with
new more complex features.

> 
> But maybe I'm missing something so please explain in detail what the
> actual problems with this are?

In this case I'd say the KISS principle applies. Simplification will
be reached by splitting the driver up only. I understand that it
couldn't have been done back then due to the original driver author
claiming the EDAC core to require the MC auto-index generation [2].
That's why I have added such functionality in the framework of this
series.

[1] https://lore.kernel.org/all/808655a9-77eb-4e3a-9781-2b059ad9517b@BN1AFFO11FD020.protection.gbl/
[2] https://lore.kernel.org/all/9dc2a947-d2ab-4f00-8ed3-d2499cb6fdfd@BN1BFFO11FD002.protection.gbl/

-Sergey

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-10-06 12:17     ` Serge Semin
@ 2022-10-06 13:25       ` Borislav Petkov
  2022-10-08  0:42         ` Serge Semin
  0 siblings, 1 reply; 38+ messages in thread
From: Borislav Petkov @ 2022-10-06 13:25 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Thu, Oct 06, 2022 at 03:17:40PM +0300, Serge Semin wrote:
> In general because it needlessly overcomplicates the driver, worsen
> it scalability, maintainability and readability, makes it much harder
> to add new controller features. Moreover even if you still able to add
> some more features support the driver will get to be more and more messy
> (as Michal correctly said in the original thread [1]).

Did you read that thread until the end?

> It will get to be messy since you'll need to add more if-flag-else
> conditionals or define new device-specific callbacks to select the
> right code path for different controllers; you'll need to define
> some private data specific to one controller and unused in another.
> All of that you already have in the driver and all of that would be
> unneeded should the driver author have followed the standard kernel
> bus-device-driver model.

So lemme ask this again because the last time it all sounded weird and I
don't think it got clarified fully: you cannot have more than one memory
controller type at the same time on those systems, can you?

Because if you can and you want to support that, the current EDAC
"design" allows to have only a single EDAC driver per system. So you
can't do two drivers now.

If your answer to that is

Subject: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure

then I'm sceptical but I'd need to look at that first.

And reading your commit messages, you're talking a lot about what you're
doing. But that should be visible from the patch. What I wanna read is
*why* you're doing it.

Like in this patch above, what's that "unique index allocation
procedure" for?

edac_mc_alloc() already gets a mc_num as the MC number.

And yes, if you want to do multiple driver instances like x86 does per
NUMA node instances, then that is done with edac_mc_alloc() which gives
you a memory controller descriptor and then you can deal with those.

From all the text it sounds to me you want to add a separate driver for
that Zynq A05 thing but I might still be missing something...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-10-06 13:25       ` Borislav Petkov
@ 2022-10-08  0:42         ` Serge Semin
  2022-10-12 17:28           ` Borislav Petkov
  0 siblings, 1 reply; 38+ messages in thread
From: Serge Semin @ 2022-10-08  0:42 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Thu, Oct 06, 2022 at 03:25:42PM +0200, Borislav Petkov wrote:
> On Thu, Oct 06, 2022 at 03:17:40PM +0300, Serge Semin wrote:
> > In general because it needlessly overcomplicates the driver, worsen
> > it scalability, maintainability and readability, makes it much harder
> > to add new controller features. Moreover even if you still able to add
> > some more features support the driver will get to be more and more messy
> > (as Michal correctly said in the original thread [1]).
> 

> Did you read that thread until the end?

Of course I did. Here is a short summary:

1. @Punnaiah described that he got a Zynq system with two different
DDR controllers. One of them was Synopsys DW uMCTL2 DDRC (ARM cortex A9
PS) and another one - Zynq A05 DDRC (Xilinx PL). All of these DDR
controllers could be probed in Linux. It was required to detect the ECC
errors for both of them.

2. @Punnaiah suggested that both of these controllers should have been
handled by two different drivers since the controllers were different.
But he was reasonably afraid that there could be a race condition for
the mc_num assignment since it was the drivers responsibility to
allocate one.

3. @Punnaiah suggested two solutions: 1) Keep two drivers in single file
and use static global variable for tracking the mc_num. 2) Let the
framework assign the mc_num to the edac driver.

4. @Punnaiah preferred the solution 2, but you said that the solution
1 would do it.

5. @Michal was against mixing up the support of two different devices
in a single driver. He reasonably assumed that the driver would get
to look messy. 

6. @Boris disagreed saying that it won't be messy and any
communication between the two memory controllers could be done
internally in that driver.

7. So you all decided to keep both controllers support in a single file,
but would get back to a discussion of having separate drivers for them
later.

But after all these years of the Synopsys EDAC driver living in kernel
we can conclude that combining the two different devices support in a
single driver was pointless. First by the driver design nobody ever
used the driver to handle both of them at the same time (MC index is
fixed to zero). Second there is no even a glimpse of communications
between two devices in the system. Third there are two different set
of macros and multiple platform-specific methods and if-flag-else
statements fully abstracting out the devices implementation from the
EDAC core.  All of that has made the driver overcomplicated in-vain
seeing none of the reasons of these complications have been realised.
Meanwhile extending further the Synopsys uMCTL2 DDR controller
functionality support would mean adding new macros, functions,
platform-specific flags, parameters and device-specific data. That
would have made the driver even more complicated. Dropping the
abstraction layer out and splitting up the drivers was the best choice
in the current circumstances especially seeing the driver author and
@Michal preferred that solution in the first place.

Moreover in order to cover a still possible case of having both
Synopsys uMCTL2 DDRC and Zynq A05 DDRC used on the same system in
future I have implemented the solution 2.

> 
> > It will get to be messy since you'll need to add more if-flag-else
> > conditionals or define new device-specific callbacks to select the
> > right code path for different controllers; you'll need to define
> > some private data specific to one controller and unused in another.
> > All of that you already have in the driver and all of that would be
> > unneeded should the driver author have followed the standard kernel
> > bus-device-driver model.
> 

> So lemme ask this again because the last time it all sounded weird and I
> don't think it got clarified fully: you cannot have more than one memory
> controller type at the same time on those systems, can you?

To be clear I don't work with the Xilinx systems. So I can only say based
on your discussion with @Punnaiah on the initial driver review.
@Punnaiah said they could have more than one memory controller type on
their systems. That's why he described the problem with the MC indexes
allocation and suggested to combine the devices support in a single
driver.

> 
> Because if you can and you want to support that, the current EDAC
> "design" allows to have only a single EDAC driver per system. So you
> can't do two drivers now.

I do understand that.

> 
> If your answer to that is
> 
> Subject: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
> 
> then I'm sceptical but I'd need to look at that first.

Em, if there is something else which makes the EDAC drivers to be
impossible to co-exist on the same system, then it greatly violates
the bus-device-driver model.

> 
> And reading your commit messages, you're talking a lot about what you're
> doing. But that should be visible from the patch. What I wanna read is
> *why* you're doing it.

Each patchlog has a thorough enough description of "why".

> 
> Like in this patch above, what's that "unique index allocation
> procedure" for?

Have you read the patchlog? 

> 
> edac_mc_alloc() already gets a mc_num as the MC number.
> 
> And yes, if you want to do multiple driver instances like x86 does per
> NUMA node instances, then that is done with edac_mc_alloc() which gives
> you a memory controller descriptor and then you can deal with those.
> 

See the "EDAC/mc: Add MC unique index allocation procedure" patch. It
also provides a way to assign the indexes based on the OF-aliases.

> From all the text it sounds to me you want to add a separate driver for
> that Zynq A05 thing but I might still be missing something...

I do want to detach the Zynq A05 device support from out of the
Synopsys uMCTL2 driver.

BTW have you read the cover letter? It contains a short summary of the
changes and their justification. It seems as if you started your
review straight from this patch.

-Sergey

> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-10-08  0:42         ` Serge Semin
@ 2022-10-12 17:28           ` Borislav Petkov
  2022-10-12 19:27             ` Serge Semin
  0 siblings, 1 reply; 38+ messages in thread
From: Borislav Petkov @ 2022-10-12 17:28 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Sat, Oct 08, 2022 at 03:42:24AM +0300, Serge Semin wrote:
> Of course I did. Here is a short summary:

You didn't have to do a short summary but sure, if you prefer...

> 7. So you all decided to keep both controllers support in a single file,
> but would get back to a discussion of having separate drivers for them
> later.

Yes, pretty much.

> But after all these years of the Synopsys EDAC driver living in kernel
> we can conclude that combining the two different devices support in a
> single driver was pointless. First by the driver design nobody ever
> used the driver to handle both of them at the same time (MC index is
> fixed to zero).

So how was this supposed to work on his system?

If you have a system with two different memory controllers and you want
to have two different EDAC drivers for each, then the whole EDAC core
code needs to be audited wrt concurrency and synchronizing access to
its facilities because I don't think it has ever supported more than a
single EDAC driver per system.

And it has never needed to, at least not on x86 land. Which is
currently changing because of CXL, because of accelerators needing
RAS, GPUs needing RAS and so on and so on. So eventually we'd have to
either put the new RAS functionality in the existing chipset-specific
driver or have to go the multiple EDAC drivers route. But that's only
tangential...

So first I'd like to hear what your use case is: single EDAC driver for
your particular Baical-T1 device or you need to support multiple EDAC
drivers.

If so, why?

> Moreover in order to cover a still possible case of having both
> Synopsys uMCTL2 DDRC and Zynq A05 DDRC used on the same system in
> future I have implemented the solution 2.

See above.

> Em, if there is something else which makes the EDAC drivers to be
> impossible to co-exist on the same system, then it greatly violates
> the bus-device-driver model.

If by that you mean the aspect of a driver associating with a device and
performing operations with it then why do you assume that EDAC drivers
have to adhere to that model?

> Have you read the patchlog?

Lemme reply to it directly.

> BTW have you read the cover letter? It contains a short summary of the
> changes and their justification.

Yes, I have read it and it contains a lot of unnecessary detail which
should be in the respective patches themselves. And I still don't know
exactly what *you* are trying to do, as I said above.

A cover letter should contain a short executive summary explaining only
the goal of the patchset and then you can go into details if you prefer.
A reviewer should not have to dig into patch management details to know
what this patchset is trying to do.

A possible structure could be:

Problem is A.

It happens because of B.

Fix it by doing C.

(Potentially do D).

Btw, when you're writing your commit messages, please use passive voice
in your commit message: no "we" or "I", etc, and describe your changes
in imperative mood.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
  2022-09-29 23:27 ` [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure Serge Semin
@ 2022-10-12 17:29   ` Borislav Petkov
  2022-10-12 20:01     ` Serge Semin
  0 siblings, 1 reply; 38+ messages in thread
From: Borislav Petkov @ 2022-10-12 17:29 UTC (permalink / raw)
  To: Serge Semin
  Cc: Michal Simek, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Robert Richter, Serge Semin, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Fri, Sep 30, 2022 at 02:27:08AM +0300, Serge Semin wrote:
> 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.

I guess this is trying to say that the current memory controller index
thing doesn't work. But why doesn't it work?

It works just fine with the x86 drivers - there the memory controller
number is the same as the node number where it is located so that works
just fine.

If that scheme cannot work on other systems, then I need to see an
explanation why it cannot work first.

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

This is talking about the "what" and not the "why".

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

This is also talking about the "what" and not the "why".

At best, what you're doing should be visible from the patch itself.

Here's a longer explanation of how a commit message should look like:

https://kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-10-12 17:28           ` Borislav Petkov
@ 2022-10-12 19:27             ` Serge Semin
  2022-10-12 19:44               ` Borislav Petkov
  0 siblings, 1 reply; 38+ messages in thread
From: Serge Semin @ 2022-10-12 19:27 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Wed, Oct 12, 2022 at 07:28:48PM +0200, Borislav Petkov wrote:
> On Sat, Oct 08, 2022 at 03:42:24AM +0300, Serge Semin wrote:
> > Of course I did. Here is a short summary:
> 
> You didn't have to do a short summary but sure, if you prefer...

It seemed to me as I should have so to make sure we are on the page of
the original discussion understanding.

> 
> > 7. So you all decided to keep both controllers support in a single file,
> > but would get back to a discussion of having separate drivers for them
> > later.
> 
> Yes, pretty much.
> 
> > But after all these years of the Synopsys EDAC driver living in kernel
> > we can conclude that combining the two different devices support in a
> > single driver was pointless. First by the driver design nobody ever
> > used the driver to handle both of them at the same time (MC index is
> > fixed to zero).
> 

> So how was this supposed to work on his system?

How am I supposed to know? You should have asked him at the time of
his patches review before accepting them. He (Punnaiah Choudary
Kalluri) said they had the system with two different DDR controllers.
Since the MC idx was supposed to be provided by the controller driver
he suggested to have both devices support implemented in the same
driver. You agreed. If no interaction intended between the two devices
why did he need to combine their support in the same driver then? He
could have as well just split the drivers up and didn't bother with
all the discussions.

> 
> If you have a system with two different memory controllers 

I don't. The Synopsys EDAC driver author (Punnaiah) did judging by
what he said in your discussion.

> and you want
> to have two different EDAC drivers for each, then the whole EDAC core
> code needs to be audited wrt concurrency and synchronizing access to
> its facilities because I don't think it has ever supported more than a
> single EDAC driver per system.

Once again. What is driver-depended do you have in the EDAC core?
Does it follow the bus-device-drivers model? I failed to find any
inter-driver dependency (what could be seen for instance in the
tty/serial subsystem). But I am not the subsystem maintainer after all.
I found the possible races in the MC index registration and fixed it
in the corresponding patch. But that was the just
registration-specifics behavior which could be easily fixed in the
same way as the most of the kernel subsystem do.

You are worried about the concurrencies. Does the EDAC core have
problems if there are several DDR devices of the same type probed?
AFAICS it doesn't as long as the indexes are properly allocated by the
driver. What is the problem with registering devices of different
types? The error counters are the MC-data-specific, so are the
sysfs-nodes. The EDAC MC error handler function doesn't touch any
inter-device parts of the core. The registration procedure is
protected by the mutex and RCU. So it seems as the EDAC core developer
thought about having the devices being registered concurrently.

> 
> And it has never needed to, at least not on x86 land. Which is
> currently changing because of CXL, because of accelerators needing
> RAS, GPUs needing RAS and so on and so on. So eventually we'd have to
> either put the new RAS functionality in the existing chipset-specific
> driver or have to go the multiple EDAC drivers route. But that's only
> tangential...

If it has never needed to, then please explain why did you let the
Synopsys EDAC driver being accepted like that then? 

> 
> So first I'd like to hear what your use case is: single EDAC driver for
> your particular Baical-T1 device or you need to support multiple EDAC
> drivers.

In my case it's a single EDAC driver per-chip. There can be several
DDR-controllers installed on the same SoC, but all of them of the same
type (Synopsys DW uMCTL2 v2.61a).

> 
> If so, why?

I don't have a system with different DDR controllers detected on the
same SoC but Punnaiah Choudary Kalluri, original Synopsys EDAC driver
developer, did.

> 
> > Moreover in order to cover a still possible case of having both
> > Synopsys uMCTL2 DDRC and Zynq A05 DDRC used on the same system in
> > future I have implemented the solution 2.
> 
> See above.
> 
> > Em, if there is something else which makes the EDAC drivers to be
> > impossible to co-exist on the same system, then it greatly violates
> > the bus-device-driver model.
> 
> If by that you mean the aspect of a driver associating with a device and
> performing operations with it then why do you assume that EDAC drivers
> have to adhere to that model?
> 
> > Have you read the patchlog?
> 
> Lemme reply to it directly.
> 
> > BTW have you read the cover letter? It contains a short summary of the
> > changes and their justification.
> 

> Yes, I have read it and it contains a lot of unnecessary detail which
> should be in the respective patches themselves. And I still don't know
> exactly what *you* are trying to do, as I said above.
> 
> A cover letter should contain a short executive summary explaining only
> the goal of the patchset and then you can go into details if you prefer.
> A reviewer should not have to dig into patch management details to know
> what this patchset is trying to do.

The main part of the letter starts exactly with the goals and then has
text with more details of what is done and why. It is enough to get a
notion regarding the patchset aim and content. Each patchlog starts
with the problem description, the suggested solution and some details
of the implementation I thought was required to add. Everything in
accordance with the kernel patches standards.

-Sergey

> 
> A possible structure could be:
> 
> Problem is A.
> 
> It happens because of B.
> 
> Fix it by doing C.
> 
> (Potentially do D).
> 
> Btw, when you're writing your commit messages, please use passive voice
> in your commit message: no "we" or "I", etc, and describe your changes
> in imperative mood.
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-10-12 19:27             ` Serge Semin
@ 2022-10-12 19:44               ` Borislav Petkov
  2022-10-12 20:55                 ` Serge Semin
  0 siblings, 1 reply; 38+ messages in thread
From: Borislav Petkov @ 2022-10-12 19:44 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Wed, Oct 12, 2022 at 10:27:43PM +0300, Serge Semin wrote:
> ... inter-device parts of the core. The registration procedure is
> protected by the mutex and RCU. So it seems as the EDAC core developer

Seems, schmeems. As I said already, EDAC has always had a single
chipset-specific driver. Period. So if one needs to run more than one
chipset-specific driver concurrently, then the whole code needs to be
audited because this hasn't been done before.

> If it has never needed to, then please explain why did you let the
> Synopsys EDAC driver being accepted like that then? 

I think I already did.

> In my case it's a single EDAC driver per-chip. There can be several
> DDR-controllers installed on the same SoC, but all of them of the same
> type (Synopsys DW uMCTL2 v2.61a).

Good.

I'll look at your patches as time allows.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
  2022-10-12 17:29   ` Borislav Petkov
@ 2022-10-12 20:01     ` Serge Semin
  2022-10-12 20:33       ` Borislav Petkov
  0 siblings, 1 reply; 38+ messages in thread
From: Serge Semin @ 2022-10-12 20:01 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Wed, Oct 12, 2022 at 07:29:22PM +0200, Borislav Petkov wrote:
> On Fri, Sep 30, 2022 at 02:27:08AM +0300, Serge Semin wrote:
> > 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.
> 

> I guess this is trying to say that the current memory controller index
> thing doesn't work. But why doesn't it work?

From what have you got this? I said that the current MC indexing
approach wasn't that optimal (always relying on the low-level driver
to allocate the index) because it caused having the same IDx
allocation pattern re-implemented in the drivers. It can be avoided by
the provided patch. The unified approach makes code indeed more
readable in the platform drivers and safer since they didn't have to
bother with more coding. See for instance the drivers with the
static variable-based IDs allocation. It doesn't seem like these
drivers bother with the detected DDR devices order. If so then the
automatic IDs allocation is perfect for them. Note the static variable
increment isn't atomic. Thus the ID allocation algorithm there is prone
to races should the devices probe is run concurrently.

> 
> It works just fine with the x86 drivers - there the memory controller
> number is the same as the node number where it is located so that works
> just fine.
> 
> If that scheme cannot work on other systems, then I need to see an
> explanation why it cannot work first.
> 
> > 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.
> 

> This is talking about the "what" and not the "why".
> 
> > 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.
> 
> This is also talking about the "what" and not the "why".
> 
> At best, what you're doing should be visible from the patch itself.
> 
> Here's a longer explanation of how a commit message should look like:
> 
> https://kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Have you read it yourself? Here is a short excerpt from there:
"Once the problem is established, describe what you are actually doing
about it in technical detail.  It's important to describe the change
in plain English for the reviewer to verify that the code is behaving
as you intend it to."

So the "problem" is described in the first paragraph and the technical
details in the later paragraphs.

-Sergey

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
  2022-10-12 20:01     ` Serge Semin
@ 2022-10-12 20:33       ` Borislav Petkov
  2022-10-12 20:44         ` Tony Luck
  2022-10-12 22:30         ` Serge Semin
  0 siblings, 2 replies; 38+ messages in thread
From: Borislav Petkov @ 2022-10-12 20:33 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Wed, Oct 12, 2022 at 11:01:54PM +0300, Serge Semin wrote:
> The unified approach makes code indeed more readable in the platform
> drivers and safer since they didn't have to bother with more coding.
> See for instance the drivers with the static variable-based IDs
> allocation.

Which drivers? Concrete examples please.

> Have you read it yourself? 

Yes. I even have improved it over the years.

> Here is a short excerpt from there:
> "Once the problem is established, describe what you are actually doing
> about it in technical detail.  It's important to describe the change
> in plain English for the reviewer to verify that the code is behaving
> as you intend it to."

Maybe that part can be misunderstood: "describe what you're doing about
it". That doesn't mean the text should explain what you're adding and
how stuff is defined: "It's defined by the EDAC_AUTO_MC_NUM macro." I
can see that from the diff.

So let me try to explain to you what I'm expecting from commit messages
in the EDAC tree:

The commit message should explain *why* a change is being done so that,
months, years from now, when you've gone on to do something else, people
doing git archeology can actually figure out *why* this change was done.

And the explanation in that commit message should be *complete* and
should contain *all* necessary information to understand why this change
was done.

Your commit message is not explaining the problem.

"In case of the unique index allocation it's not that optimal to always
rely on the low-level device drivers (platform drivers)"

That's your statement. That needs to have exact details so that people
can look at that commit message, look at the code which *you* point them
to in it and go, aha, that is the problem.

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

Who are they, exact pointers please.

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

That doesn't matter one bit for the change you're doing. You could have
added it under the "---" line.

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

Some of that paragraph should go over the function as a comment - not in
the commit message as it pertains to what the function does and it would
make a *lot* more sense there when someone tries to figure out what the
function does instead of in the commit message.

So, I'm still not convinced why do some EDAC drivers need unique MC
identifiers, why the current scheme doesn't work and where it doesn't
work.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
  2022-10-12 20:33       ` Borislav Petkov
@ 2022-10-12 20:44         ` Tony Luck
  2022-10-12 21:31           ` Serge Semin
  2022-10-12 22:30         ` Serge Semin
  1 sibling, 1 reply; 38+ messages in thread
From: Tony Luck @ 2022-10-12 20:44 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Serge Semin, Serge Semin, Michal Simek, Mauro Carvalho Chehab,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Wed, Oct 12, 2022 at 10:33:35PM +0200, Borislav Petkov wrote:
> On Wed, Oct 12, 2022 at 11:01:54PM +0300, Serge Semin wrote:

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

Just curious.If I have an EDAC driver using EDAC_AUTO_MC_NUM, and I
unload and reload the driver a bunch of times. Do I get the same memory
controller numbers each reload, or get an ever increasing set each time?

-Tony

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

* Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support
  2022-10-12 19:44               ` Borislav Petkov
@ 2022-10-12 20:55                 ` Serge Semin
  0 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-10-12 20:55 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Wed, Oct 12, 2022 at 09:44:00PM +0200, Borislav Petkov wrote:
> On Wed, Oct 12, 2022 at 10:27:43PM +0300, Serge Semin wrote:
> > ... inter-device parts of the core. The registration procedure is
> > protected by the mutex and RCU. So it seems as the EDAC core developer
> 
> Seems, schmeems. As I said already, EDAC has always had a single
> chipset-specific driver. Period. So if one needs to run more than one
> chipset-specific driver concurrently, then the whole code needs to be
> audited because this hasn't been done before.
> 
> > If it has never needed to, then please explain why did you let the
> > Synopsys EDAC driver being accepted like that then? 
> 
> I think I already did.

Kind of. What you didn't explain was the driver-specific problem in the
edac_mc core. What is the difference in the EDAC core handling
two devices (including of difference types) on the same platform and
handling the same devices each probed by two different drivers? (Consider
the drivers are designed thread-safe and we are talking about the EDAC
MC core.)

> 
> > In my case it's a single EDAC driver per-chip. There can be several
> > DDR-controllers installed on the same SoC, but all of them of the same
> > type (Synopsys DW uMCTL2 v2.61a).
> 
> Good.
> 
> I'll look at your patches as time allows.

Ok. Thanks in advance.

-Sergey

> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
  2022-10-12 20:44         ` Tony Luck
@ 2022-10-12 21:31           ` Serge Semin
  0 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2022-10-12 21:31 UTC (permalink / raw)
  To: Tony Luck
  Cc: Borislav Petkov, Serge Semin, Michal Simek,
	Mauro Carvalho Chehab, James Morse, Robert Richter,
	Alexey Malahov, Michail Ivanov, Pavel Parkhomenko,
	Punnaiah Choudary Kalluri, Manish Narani, Dinh Nguyen,
	Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Wed, Oct 12, 2022 at 01:44:59PM -0700, Tony Luck wrote:
> On Wed, Oct 12, 2022 at 10:33:35PM +0200, Borislav Petkov wrote:
> > On Wed, Oct 12, 2022 at 11:01:54PM +0300, Serge Semin wrote:
> 
> > "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."
> 

> Just curious.If I have an EDAC driver using EDAC_AUTO_MC_NUM, and I
> unload and reload the driver a bunch of times. Do I get the same memory
> controller numbers each reload, or get an ever increasing set each time?

You'll get the same numbers if no other device-driver allocated them
(which we already established isn't relevant to EDAC since only one
driver per system is supposed to work). In order to implement what you
said I should have called idr_alloc_cyclic() instead.

Here is what the suggested implementation implies:

1. If there is a particular ID assigned in the framework of the dts aliases
(i.e. mcX = &ddr0) then that value will be selected for the controller.
2. If no particular ID is assigned but there are mcX-like aliases
defined in DTS, then the next free id greater than the last mcX value
will be assigned.
3. If none if the above is relevant the first free ID in the pool will be
allocated.
4. On the driver unload the driver remove method will be called thus
the allocated ID will be freed on the MC-controller de-registration.
Thus on the next driver loading the freed IDs will be reused by the
idr_alloc() method design.

-Sergey

> 
> -Tony

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

* Re: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
  2022-10-12 20:33       ` Borislav Petkov
  2022-10-12 20:44         ` Tony Luck
@ 2022-10-12 22:30         ` Serge Semin
  2022-10-13  9:38           ` Borislav Petkov
  1 sibling, 1 reply; 38+ messages in thread
From: Serge Semin @ 2022-10-12 22:30 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Wed, Oct 12, 2022 at 10:33:35PM +0200, Borislav Petkov wrote:
> On Wed, Oct 12, 2022 at 11:01:54PM +0300, Serge Semin wrote:
> > The unified approach makes code indeed more readable in the platform
> > drivers and safer since they didn't have to bother with more coding.
> > See for instance the drivers with the static variable-based IDs
> > allocation.
> 
> Which drivers? Concrete examples please.

See below.

> 
> > Have you read it yourself? 
> 
> Yes. I even have improved it over the years.
> 
> > Here is a short excerpt from there:
> > "Once the problem is established, describe what you are actually doing
> > about it in technical detail.  It's important to describe the change
> > in plain English for the reviewer to verify that the code is behaving
> > as you intend it to."
> 
> Maybe that part can be misunderstood: "describe what you're doing about
> it". That doesn't mean the text should explain what you're adding and
> how stuff is defined: "It's defined by the EDAC_AUTO_MC_NUM macro." I
> can see that from the diff.
> 
> So let me try to explain to you what I'm expecting from commit messages
> in the EDAC tree:
> 

> The commit message should explain *why* a change is being done so that,
> months, years from now, when you've gone on to do something else, people
> doing git archeology can actually figure out *why* this change was done.
> 
> And the explanation in that commit message should be *complete* and
> should contain *all* necessary information to understand why this change
> was done.

A level of completeness can be relative to each person. For all the
years I've submitting the patches to the kernel I couldn't even
remember the last request to elaborate my logs. In no means I want to
say they were perfect. I could just be too immersed into the problem
so thought that the provided text was descriptive enough especially
for the subsystem maintainer. So to speak asking for more details
would be more than enough.

> 
> Your commit message is not explaining the problem.
> 
> "In case of the unique index allocation it's not that optimal to always
> rely on the low-level device drivers (platform drivers)"
> 

> That's your statement. That needs to have exact details so that people
> can look at that commit message, look at the code which *you* point them
> to in it and go, aha, that is the problem.
> 
> "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."
> 
> Who are they, exact pointers please.

So you need more details. You should have just asked. I can't read
your mind after all. IMO the description was detailed enough to
understand the problem. Anyway as I already said the current MC
indexing approach wasn't that optimal (always relying on the low-level
driver to allocate the index) because it caused having the same IDx
allocation pattern re-implemented in the drivers. The brightest
example is the drivers with the static variable-based IDs allocation.
It doesn't seem like these drivers bother with the detected DDR
devices order. If so then the automatic IDs allocation is perfect for
them. Instead they can just pass the EDAC_AUTO_MC_NUM id to the
edac_mc_alloc() method and drop the static-based pattern. Thus getting
smaller and more readable drivers code. Moreover the variable
increment isn't atomic. Thus the ID allocation algorithm there is
prone to races should the devices probe is run concurrently.

The last but not least there is no currently way to assign the
controllers ID by means of the DTS file. The suggested patch provides
such functionality by means of the DT aliases.

If it describes the problem better I'll add the text to the patchlog
on the next patchset re-spin.

> 
> "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."
> 
> That doesn't matter one bit for the change you're doing. You could have
> added it under the "---" line.

Ok. I'll drop it from the log.

> 
> "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."
>
 
> Some of that paragraph should go over the function as a comment - not in
> the commit message as it pertains to what the function does and it would
> make a *lot* more sense there when someone tries to figure out what the
> function does instead of in the commit message.
> 

IMO the function isn't that complex to add the comment there. The
semantic can be easily enough inferred from the implementation. 

> So, I'm still not convinced why do some EDAC drivers need unique MC
> identifiers, why the current scheme doesn't work and where it doesn't
> work.

Once again. I didn't say it didn't work. I said it wasn't optimal.
Though in some circumstance it can misbehave for some drivers.
Please see my response above.

-Sergey

> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
  2022-10-12 22:30         ` Serge Semin
@ 2022-10-13  9:38           ` Borislav Petkov
  0 siblings, 0 replies; 38+ messages in thread
From: Borislav Petkov @ 2022-10-13  9:38 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Michal Simek, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter, Alexey Malahov, Michail Ivanov,
	Pavel Parkhomenko, Punnaiah Choudary Kalluri, Manish Narani,
	Dinh Nguyen, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel

On Thu, Oct 13, 2022 at 01:30:39AM +0300, Serge Semin wrote:
> A level of completeness can be relative to each person. For all the
> years I've submitting the patches to the kernel I couldn't even
> remember the last request to elaborate my logs. In no means I want to
> say they were perfect. I could just be too immersed into the problem
> so thought that the provided text was descriptive enough especially
> for the subsystem maintainer. So to speak asking for more details
> would be more than enough.

Dude, are you even reading what I'm writing to you?!

I don't care how immersed you were in the problem and who asked or
didn't ask you to elaborate your logs. If you're submitting patches
to the EDAC tree, those logs need to be complete and explain things
sufficiently and exactly. Period.

> So you need more details. You should have just asked. I can't read
> your mind after all.

And I can't read yours too. And I asked like three times already. And
yet, you still are not giving me a concrete answer.

I said "exact pointers please". That means, you point me to a driver and
the *exact* *code* in there which you think is doing something which
needs fixing.

What you've given me again is the same spiel as before.

So let me save you and me some time: your patches are not going anywhere
until they explain the thing they're fixing properly and precisely. End
of story.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure
  2022-09-29 23:26 ` [PATCH RESEND v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin
@ 2022-10-31 17:24   ` Borislav Petkov
  2023-09-01 11:24     ` Serge Semin
  0 siblings, 1 reply; 38+ messages in thread
From: Borislav Petkov @ 2022-10-31 17:24 UTC (permalink / raw)
  To: Serge Semin
  Cc: Michal Simek, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Robert Richter, Dinh Nguyen, Serge Semin, Alexey Malahov,
	Michail Ivanov, Pavel Parkhomenko, Punnaiah Choudary Kalluri,
	Manish Narani, Rob Herring, Krzysztof Kozlowski, Rob Herring,
	Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-edac,
	linux-kernel, Sherry Sun, Shubhrajyoti Datta

On Fri, Sep 30, 2022 at 02:26:56AM +0300, Serge Semin wrote:
> 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.

Because?

Btw, for the future, you should make sure you add those commit authors
to Cc so that they can get a chance to comment. Adding the folks from
that commit to Cc.

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

What does that sentence mean exactly? The UE/CE auto clearing
functionality is not in that silicon?

> The IRQ status can be cleared by means of setting the ECCCLR/ECCCTL
> register self-cleared flags 0-3.

I'm guessing that's this reg:

/* ECC control register */
#define ECC_CTRL_OFST                   0xC4

?

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

Aah, you mean that the ->get_error_info() functions are doing the
clearing even if something should be doing self clearing. And that
DDR_ECC_INTR_SELF_CLEAR thing is queried when enabling the error
interrupt which is just bad naming because it looks like that quirk
controls what register to write/read.

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

So what is this commit message of

  4bcffe941758 ("EDAC/synopsys: Re-enable the error interrupts on v3 hw")

then talking about:

"Then the interrupt handler will be called only once."

How is that interrupt supposed to be reenabled?

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

Aha, that sounds like the right thing to toggle.

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

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to
do frotz", as if you are giving orders to the codebase to change its
behaviour."

In this case, pls formulate it something like this:

"So fix all these problems noted above: rename DDR_ECC_INTR_SELF_CLEAR
to SYNPS_ZYNQMP_IRQ_REGS to denote that, ..."

And so on.

> Its semantic is now the opposite: the quirk means having the ZynqMP
> IRQ CSRs available on the platform.

Yes, that makes more sense.

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

If those are not publicly accessible, then there's no point to put them
in here.

> Fixes: f7824ded4149 ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Does this need to go to stable@ and thus older kernels?

> ---
>  drivers/edac/synopsys_edac.c | 76 +++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 28 deletions(-)

> @@ -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;

That lock needs to be named properly and have a comment above it what it
protects.

>  	char message[SYNPS_EDAC_MSG_SIZE];
>  	struct synps_ecc_status stat;
>  	const struct synps_platform_data *p_data;

...

> @@ -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 */

How does this comment help here?

If it is available since a version number, why doesn't the below check a
version number? IOW, what is the relevance of that comment here?

In any case, I need to hear from this driver's maintainer too.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups
  2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
                   ` (17 preceding siblings ...)
  2022-09-30 14:29 ` [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Borislav Petkov
@ 2023-08-18 23:06 ` Serge Semin
  18 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2023-08-18 23:06 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Serge Semin, Alexander Stein, Michal Simek, Borislav Petkov,
	James Morse, Mauro Carvalho Chehab, Tony Luck, Robert Richter,
	Alexey Malahov, Shubhrajyoti Datta, Punnaiah Choudary Kalluri,
	Manish Narani, Dinh Nguyen, Rob Herring, Krzysztof Kozlowski,
	Rob Herring, Krzysztof Kozlowski, devicetree, linux-arm-kernel,
	linux-edac, linux-kernel

Hi Borislav

On Fri, Sep 30, 2022 at 02:26:55AM +0300, Serge Semin wrote:
> 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/20220910195007.11027-1-Sergey.Semin@baikalelectronics.ru
> [3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 support
> Link: https://lore.kernel.org/linux-edac/20220910195659.11843-1-Sergey.Semin@baikalelectronics.ru

I'm terribly sorry for a very long delay with addressing your comments
in this patchset. I have been very busy all that time with another
kernel drivers. Now I am finally able to get back to this series.

This patchset and another two ones have been successfully rebased onto
the latest kernel version - 6.5-rc5. No regression has been spotted in
my environment.

Regarding your comments to this series. I've got a general gist
concerning my patchlog messages. I'll convert all of them to using the
imperative mood and drop "what" description in v4.

Regarding the main issue of detaching the Zynq A05 DDRC part from the
Synopsys uMCTL2 driver. I see there is another ZynqMP mem-ctl driver
has been added recently. It's ZynqMP OCM. During its review you
settled that it's possible to have two different EDAC drivers executed
on the system.
https://lore.kernel.org/linux-edac/BY5PR12MB4258CB67B70D71F107EC1E9DDB3E9@BY5PR12MB4258.namprd12.prod.outlook.com/
So it's no longer issue in this series and the Zynq A05 DDRC part can
be freely moved to a separate driver as I originally intended.

Regarding the MC identifiers patch. I'll revise it one more time
whether it needs to be changed. In anyway I still find it very handy
to have a functionality of dynamic or alias-based MC identifier
allocation.

I'll respond to your comment sent to the patch 1 of this series. Then
I'll fix all the issues described above and resubmit v4. Then we'll be
able to get back to a standard review procedure.

Once again sorry for the huge delay with proceeding in this patchset
review.

Regards
-Serge(y)

> 
> 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 v2:
> - 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
> 
> Link: https://lore.kernel.org/linux-edac/20220910194237.10142-1-Sergey.Semin@baikalelectronics.ru
> Changelog v3:
> - Drop the no longer used "priv" pointer from the mc_init() function.
>   (@tbot)
> - Drop the merged in patches:
> [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support
> [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name
>   (@Krzysztof)
> 
> 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+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.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 (17):
>   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
>   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
> 
>  MAINTAINERS                  |   1 +
>  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     | 501 +++++++++++++++++++
>  7 files changed, 927 insertions(+), 627 deletions(-)
>  create mode 100644 drivers/edac/zynq_edac.c
> 
> -- 
> 2.37.3
> 
> 

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

* Re: [PATCH RESEND v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure
  2022-10-31 17:24   ` Borislav Petkov
@ 2023-09-01 11:24     ` Serge Semin
  0 siblings, 0 replies; 38+ messages in thread
From: Serge Semin @ 2023-09-01 11:24 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Serge Semin, Alexander Stein, Michal Simek, Michal Simek,
	Mauro Carvalho Chehab, Tony Luck, James Morse, Robert Richter,
	Dinh Nguyen, Alexey Malahov, Michail Ivanov, Pavel Parkhomenko,
	Punnaiah Choudary Kalluri, Manish Narani, Rob Herring,
	Krzysztof Kozlowski, Rob Herring, Krzysztof Kozlowski,
	devicetree, linux-arm-kernel, linux-edac, linux-kernel,
	Sherry Sun, Shubhrajyoti Datta

Hi Borislav

Thank you very much for all the useful notes. I've updated the series
in accordance with your comments. It will be submitted for review when the
merge window is open. Answers to some of your questions are below.

On Mon, Oct 31, 2022 at 06:24:52PM +0100, Borislav Petkov wrote:
> On Fri, Sep 30, 2022 at 02:26:56AM +0300, Serge Semin wrote:
> > 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.
> 
> Because?
> 
> Btw, for the future, you should make sure you add those commit authors
> to Cc so that they can get a chance to comment. Adding the folks from
> that commit to Cc.

At least the authors are Cc'ed actually.

> 
> > 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.
> 	    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> What does that sentence mean exactly? The UE/CE auto clearing
> functionality is not in that silicon?
> 
> > The IRQ status can be cleared by means of setting the ECCCLR/ECCCTL
> > register self-cleared flags 0-3.
> 
> I'm guessing that's this reg:
> 
> /* ECC control register */
> #define ECC_CTRL_OFST                   0xC4
> 
> ?
> 
> > 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".
> 
> Aah, you mean that the ->get_error_info() functions are doing the
> clearing even if something should be doing self clearing. And that
> DDR_ECC_INTR_SELF_CLEAR thing is queried when enabling the error
> interrupt which is just bad naming because it looks like that quirk
> controls what register to write/read.
> 
> > 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.)
> 
> So what is this commit message of
> 
>   4bcffe941758 ("EDAC/synopsys: Re-enable the error interrupts on v3 hw")
> 
> then talking about:
> 
> "Then the interrupt handler will be called only once."
> 
> How is that interrupt supposed to be reenabled?
> 
> > 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].
> 
> Aha, that sounds like the right thing to toggle.
> 
> > 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.
> 

Seeing you've got so many questions I guess the text has turned to be
a bit hard to comprehend. I'll update the patch log to having less
history insides but being more oriented towards the actual problem.
Sorry for the inconvenience.

> "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to
> do frotz", as if you are giving orders to the codebase to change its
> behaviour."
> 
> In this case, pls formulate it something like this:
> 
> "So fix all these problems noted above: rename DDR_ECC_INTR_SELF_CLEAR
> to SYNPS_ZYNQMP_IRQ_REGS to denote that, ..."
> 
> And so on.

Got it. The entire series of the patchsets will be fixed in accordance
with your request.

> 
> > Its semantic is now the opposite: the quirk means having the ZynqMP
> > IRQ CSRs available on the platform.
> 
> Yes, that makes more sense.

I'll move this change into another patch. The described problem can be
fixed with no need in this modification. Thus the patch will get to be
simpler for review. 

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

> If those are not publicly accessible, then there's no point to put them
> in here.

IMO it's better to have some links than not to have them at all and say
something like "Synopsys databook claims it differently". At least
they will be useful for the developers who have those documents.

> 
> > Fixes: f7824ded4149 ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR")
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> Does this need to go to stable@ and thus older kernels?

Yes it was the original intention - to get it backported, but it's up
to the stable tree maintainers to decide. The problem is real but very
much rare to be actually spotted especially seeing nobody has reported
about it yet.

> 
> > ---
> >  drivers/edac/synopsys_edac.c | 76 +++++++++++++++++++++++-------------
> >  1 file changed, 48 insertions(+), 28 deletions(-)
> 
> > @@ -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;
> 

> That lock needs to be named properly and have a comment above it what it
> protects.

I'll rename it to reglock. The structure already has it documented in
the above kdoc.

> 
> >  	char message[SYNPS_EDAC_MSG_SIZE];
> >  	struct synps_ecc_status stat;
> >  	const struct synps_platform_data *p_data;
> 
> ...
> 
> > @@ -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 */
> 

> How does this comment help here?
> 
> If it is available since a version number, why doesn't the below check a
> version number? IOW, what is the relevance of that comment here?

This comment will be added with a statement that the operation below
is "noop" for the older devices. Moreover the comment will be helpful
for the future driver hackers who would wish to add the DW uMCTL2
IP-core versions detected and utilized it in the driver. In none of
the my changes it was needed, so I didn't provide such functionality.

> 
> In any case, I need to hear from this driver's maintainer too.

I have much doubt we'll hear out anything from the original
driver authors. Hopefully Michal and Alexander (in Cc-list) will help
us out with reviews and testing.

-Serge(y)

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

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

end of thread, other threads:[~2023-09-01 11:24 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin
2022-09-29 23:26 ` [PATCH RESEND v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin
2022-10-31 17:24   ` Borislav Petkov
2023-09-01 11:24     ` Serge Semin
2022-09-29 23:26 ` [PATCH RESEND v3 02/17] EDAC/synopsys: Fix generic device type detection procedure Serge Semin
2022-09-29 23:26 ` [PATCH RESEND v3 03/17] EDAC/synopsys: Fix mci->scrub_cap field setting Serge Semin
2022-09-29 23:26 ` [PATCH RESEND v3 04/17] EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 05/17] EDAC/synopsys: Fix reading errors count before ECC status Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 06/17] EDAC/synopsys: Use platform device devm ioremap method Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 07/17] EDAC/synopsys: Drop internal CE and UE counters Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 08/17] EDAC/synopsys: Drop local to_mci macro implementation Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 09/17] EDAC/synopsys: Drop struct ecc_error_info.blknr field Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 10/17] EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 11/17] EDAC/synopsys: Drop redundant info from error message Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 12/17] EDAC/mc: Init DIMM labels in MC registration method Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure Serge Semin
2022-10-12 17:29   ` Borislav Petkov
2022-10-12 20:01     ` Serge Semin
2022-10-12 20:33       ` Borislav Petkov
2022-10-12 20:44         ` Tony Luck
2022-10-12 21:31           ` Serge Semin
2022-10-12 22:30         ` Serge Semin
2022-10-13  9:38           ` Borislav Petkov
2022-09-29 23:27 ` [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support Serge Semin
2022-09-30 14:42   ` Borislav Petkov
2022-10-06 12:17     ` Serge Semin
2022-10-06 13:25       ` Borislav Petkov
2022-10-08  0:42         ` Serge Semin
2022-10-12 17:28           ` Borislav Petkov
2022-10-12 19:27             ` Serge Semin
2022-10-12 19:44               ` Borislav Petkov
2022-10-12 20:55                 ` Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 15/17] EDAC/synopsys: Drop unused platform-specific setup API Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 16/17] EDAC/synopsys: Unify the driver entities naming Serge Semin
2022-09-29 23:27 ` [PATCH RESEND v3 17/17] EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros Serge Semin
2022-09-30 14:29 ` [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Borislav Petkov
2022-10-06  7:13   ` Michal Simek
2023-08-18 23:06 ` 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).