[PATCH 00/13] EDAC/synopsys: Add generic resources and Baikal-T1 support

[PATCH 00/13] EDAC/synopsys: Add generic resources and Baikal-T1 support

[Serge Semin]

This patchset is a third 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: https://lore.kernel.org/linux-edac/20220822190730.27277-1-Sergey.Semin@baikalelectronics.ru/
[2: In-progress] EDAC/synopsys: Add generic DDRC info and address mapping
Link: https://lore.kernel.org/linux-edac/20220822191427.27969-1-Sergey.Semin@baikalelectronics.ru/
[3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 support
Link: ---you are looking at it---

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.

This is a final patchset in the framework of my Synopsys DW uMCTL2 DDRC
work, which completes the driver updates with the new functionality and
at the closure introduces the Baikal-T1 DDRC support.

The series starts from extending the Synopsys DW uMCTL2 DDRC DT-schema
with the controller specific IRQs, clocks and resets properties. In
addition to the Baikal-T1 DDRC is added to the DT-bindings since it's
based on the DW uMCTL2 DDRC v2.61a.

After that we suggest to finally inform the MCI core with the detected
SDRAM ranks and make sure the detected errors are reported to the
corresponding rank. Then we extend the DDRC capabilities with optional
Scrub functionality. It's indeed possible to have the DW uMCTL2 controller
with no HW-accelerated Scrub support (no RMW engine). In that case the MCI
core is supposed to perform the erroneous location ECC update by means of
the platform-specific scrub method.

Then we get to fix the error-injection functionality a bit. First since
the driver now has the Sys<->SDRAM address translation infrastructure we
can use it to convert the supplied poisonous system address to the SDRAM
one. Thus there is no longer need in preserving the address in the device
private data. Second we suggest to add a DebuFS node-based command to
disable the error-injection feature (no idea why it hasn't been done in
the first place).

Afterwards a series of the IRQ-related patches goes. First we introduce the
individual DDRC event IRQs support in accordance with what has been added
to the DT-bindings and what the native DW uMCTL2 DDR controller actually
provides. Then aside to the ECC CE/UE errors detection we suggest to the
DFI/SDRAM CRC/Parity errors report. It specifically useful for the DDR4
memory which has dedicated ALARM_n signal, but can be still utilized in
the framework of the older protocols if the device DFI-PHY calculates
the HIF-interface signals parity. Third after adding the platform
clock/resets request procedure we introduce the HW-accelerated Scrubber
support. Its performance can be tuned by means of the sdram_scrub_rate
SysFS node and the Core clock rate. Note it is possible to one-time-run
the Scrubber in the back-to-back mode so to perform a burst-like scan of
the whole SDRAM memory.

At the patchset closure we finally fix the DW uMCTL2 DDRC kernel config to
be available not only on the Xilinx, Intel and MXC platforms and add the
Baikal-T1 DDRC support which the whole work has been dedicated for in the
first place.

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: 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 (13):
  dt-bindings: memory: snps: Extend schema with IRQs/resets/clocks props
  dt-bindings: memory: snps: Add Baikal-T1 DDRC support
  EDAC/synopsys: Add multi-ranked memory support
  EDAC/synopsys: Add optional ECC Scrub support
  EDAC/synopsys: Drop ECC poison address from private data
  EDAC/synopsys: Add data poisoning disable support
  EDAC/synopsys: Split up ECC UE/CE IRQs handler
  EDAC/synopsys: Add individual named ECC IRQs support
  EDAC/synopsys: Add DFI alert_n IRQ support
  EDAC/synopsys: Add reference clocks support
  EDAC/synopsys: Add ECC Scrubber support
  EDAC/synopsys: Drop vendor-specific arch dependency
  EDAC/synopsys: Add Baikal-T1 DDRC support

 .../snps,dw-umctl2-ddrc.yaml                  |  75 +-
 drivers/edac/Kconfig                          |   1 -
 drivers/edac/synopsys_edac.c                  | 952 ++++++++++++++----
 3 files changed, 830 insertions(+), 198 deletions(-)

-- 
2.35.1

[PATCH 01/13] dt-bindings: memory: snps: Extend schema with IRQs/resets/clocks props

[Serge Semin]

First of all the DW uMCTL2 DDRC IP-core supports the individual IRQ lines
for each standard event: ECC Corrected Error, ECC Uncorrected Error, ECC
Address Protection, Scrubber-Done signal, DFI Parity/CRC Error. It's
possible that the platform engineers merge them up in the IRQ controller
level. So let's add both configuration support to the DT-schema.

Secondly each IP-core interface is supplied with a clock source like APB
reference clock, AXI-ports clock, main DDRC core reference clock and
Scrubber low-power clock. In addition to that each clock domain can have a
dedicated reset signal. Let's add the properties for at least the denoted
clock sources and the corresponding reset controls.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../snps,dw-umctl2-ddrc.yaml                  | 65 +++++++++++++++++--
 1 file changed, 60 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
index 787d91d64eee..8db92210cfe1 100644
--- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
@@ -13,13 +13,13 @@ maintainers:
 
 description: |
   Synopsys DesignWare Enhanced uMCTL2 DDR Memory Controller is cappable of
-  working with DDR devices up to (LP)DDR4 protocol. It can be equipped
+  working with DDR devices upporting to (LP)DDR4 protocol. It can be equipped
   with SEC/DEC ECC feature if DRAM data bus width is either 16-bits or
   32-bits or 64-bits wide.
 
-  The ZynqMP DDR controller is based on the DW uMCTL2 v2.40a controller.
-  It has an optional SEC/DEC ECC support in 64-bit and 32-bit bus width
-  configurations.
+  For instance the ZynqMP DDR controller is based on the DW uMCTL2 v2.40a
+  controller. It has an optional SEC/DEC ECC support in 64-bit and 32-bit
+  bus width configurations.
 
 properties:
   compatible:
@@ -28,11 +28,55 @@ properties:
       - xlnx,zynqmp-ddrc-2.40a
 
   interrupts:
-    maxItems: 1
+    description:
+      DW uMCTL2 DDRC IP-core provides individual IRQ signal for each event":"
+      ECC Corrected Error, ECC Uncorrected Error, ECC Address Protection,
+      Scrubber-Done signal, DFI Parity/CRC Error. Some platforms may have the
+      signals merged before they reach the IRQ controller or have some of them
+      absent in case if the corresponding feature is unavailable/disabled.
+    minItems: 1
+    maxItems: 5
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 5
+    oneOf:
+      - description: Common ECC CE/UE/Scrubber/DFI Errors IRQ
+        items:
+          - const: ecc
+      - description: Individual ECC CE/UE/Scrubber/DFI Errors IRQs
+        items:
+          enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ]
 
   reg:
     maxItems: 1
 
+  clocks:
+    description:
+      A standard set of the clock sources contains CSRs bus clock, AXI-ports
+      reference clock, DDRC core clock, Scrubber standalone clock
+      (synchronous to the DDRC clock).
+    minItems: 1
+    maxItems: 4
+
+  clock-names:
+    minItems: 1
+    maxItems: 4
+    items:
+      enum: [ pclk, aclk, core, sbr ]
+
+  resets:
+    description:
+      Each clock domain can have separate reset signal.
+    minItems: 1
+    maxItems: 4
+
+  reset-names:
+    minItems: 1
+    maxItems: 4
+    items:
+      enum: [ prst, arst, core, sbr ]
+
 required:
   - compatible
   - reg
@@ -48,4 +92,15 @@ examples:
       interrupt-parent = <&gic>;
       interrupts = <0 112 4>;
     };
+  - |
+    memory-controller@fd070000 {
+      compatible = "snps,ddrc-3.80a";
+      reg = <0x3d400000 0x400000>;
+
+      interrupts = <0 147 4>, <0 148 4>, <0 149 4>, <0 150 4>;
+      interrupt-names = "ecc_ce", "ecc_ue", "ecc_sbr", "dfi_e";
+
+      clocks = <&rcu 0>, <&rcu 5>, <&rcu 6>, <&rcu 7>;
+      clock-names = "pclk", "aclk", "core", "sbr";
+    };
 ...
-- 
2.35.1

[PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Serge Semin]

Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
are individual IRQs for each ECC and DFI events.The dedicated scrubber
clock source is absent since it's fully synchronous to the core clock.
In addition to that the DFI-DDR PHY CSRs can be accessed via a separate
registers space.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../memory-controllers/snps,dw-umctl2-ddrc.yaml        | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
index 8db92210cfe1..899a6c5f9806 100644
--- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
@@ -26,6 +26,7 @@ properties:
     enum:
       - snps,ddrc-3.80a
       - xlnx,zynqmp-ddrc-2.40a
+      - baikal,bt1-ddrc
 
   interrupts:
     description:
@@ -49,7 +50,14 @@ properties:
           enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ]
 
   reg:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
+
+  reg-names:
+    minItems: 1
+    items:
+      - const: umctl2
+      - const: phy
 
   clocks:
     description:
-- 
2.35.1

[PATCH 03/13] EDAC/synopsys: Add multi-ranked memory support

[Serge Semin]

DW uMCTL2 DDRC supports multi-rank memory attached to the controller. If
so the MSTR.active_ranks field will be set with the populated ranks
bitfield. It is permitted to have one, two or four ranks activated at a
time [1]. Since we now have the number of ranks determined in the
controller configuration detection procedure, it can be easily used for
accordingly extending the MCI chip-select layer. In case of the ECC errors
the affected rank will be read from the CE/UE address CSRs [2].

Note we need to drop the ranks from the total memory size calculation
since multiple ranks are taken into account by means of the layer[0]
definition.

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

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 90b57986a9b5..872ad9a164a7 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -23,9 +23,6 @@
 
 #include "edac_module.h"
 
-/* Number of cs_rows needed per memory controller */
-#define SNPS_EDAC_NR_CSROWS		1
-
 /* Number of channels per memory controller */
 #define SNPS_EDAC_NR_CHANS		1
 
@@ -795,7 +792,7 @@ static void snps_handle_error(struct mem_ctl_info *mci, struct snps_ecc_status *
 
 		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, p->ce_cnt,
 				     PHYS_PFN(sys), offset_in_page(sys),
-				     pinf->syndrome, 0, 0, -1,
+				     pinf->syndrome, pinf->sdram.rank, 0, -1,
 				     priv->message, "");
 	}
 
@@ -812,7 +809,7 @@ static void snps_handle_error(struct mem_ctl_info *mci, struct snps_ecc_status *
 
 		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, p->ue_cnt,
 				     PHYS_PFN(sys), offset_in_page(sys),
-				     pinf->syndrome, 0, 0, -1,
+				     pinf->syndrome, pinf->sdram.rank, 0, -1,
 				     priv->message, "");
 	}
 
@@ -1411,10 +1408,7 @@ static u64 snps_get_sdram_size(struct snps_edac_priv *priv)
 			size++;
 	}
 
-	for (i = 0; i < DDR_MAX_RANK_WIDTH; i++) {
-		if (map->rank[i] != DDR_ADDRMAP_UNUSED)
-			size++;
-	}
+	/* Skip the ranks since the multi-rankness is determined by layer[0] */
 
 	return 1ULL << (size + priv->info.dq_width);
 }
@@ -1468,7 +1462,7 @@ static struct mem_ctl_info *snps_mc_create(struct snps_edac_priv *priv)
 	struct mem_ctl_info *mci;
 
 	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
-	layers[0].size = SNPS_EDAC_NR_CSROWS;
+	layers[0].size = priv->info.ranks;
 	layers[0].is_virt_csrow = true;
 	layers[1].type = EDAC_MC_LAYER_CHANNEL;
 	layers[1].size = SNPS_EDAC_NR_CHANS;
-- 
2.35.1

[PATCH 04/13] EDAC/synopsys: Add optional ECC Scrub support

[Serge Semin]

DW uMCTL2 DDRC ECC has a so called ECC Scrub feature in case if an
single-bit error is detected. The scrub is executed as a new RMW operation
to the location that resulted in a single-bit error thus fixing the ECC
code preserved in the SDRAM. But that feature not only optional, but also
runtime switchable. So there can be platforms with DW uMCTL2 DDRC not
supporting hardware-base scrub. In those cases the single-bit errors will
still be detected but won't be fixed until the next SDRAM write commands
to the erroneous location. Since the ECC Scrub feature availability is
detectable by means of the ECCCFG0.dis_scrub flag state we can use it to
tune the MCI core up so one would automatically execute the
platform-specific the platform-specific scrubbing to the affected SDRAM
location. It's now possible to be done since the DW uMCTL2 DDRC driver
supports the actual system address reported to the MCI core. The only
thing left to do is to auto-detect the ECC Scrub feature availability and
set the mem_ctl.info.scrub_mode mode with SCRUB_SW_SRC if the feature is
unavailable. The rest will be done by the MCI core when the single-bit
errors happen.

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 872ad9a164a7..2b8de7e8fae1 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -32,6 +32,7 @@
 #define SNPS_EDAC_MOD_VER		"1"
 
 /* DDR capabilities */
+#define SNPS_CAP_ECC_SCRUB		BIT(0)
 #define SNPS_CAP_ZYNQMP			BIT(31)
 
 /* Synopsys uMCTL2 DDR controller registers that are relevant to ECC */
@@ -114,6 +115,7 @@
 #define DDR_MSTR_MEM_LPDDR4		BIT(5)
 
 /* ECC CFG0 register definitions */
+#define ECC_CFG0_DIS_SCRUB		BIT(4)
 #define ECC_CFG0_MODE_MASK		GENMASK(2, 0)
 
 /* ECC status register definitions */
@@ -1008,6 +1010,10 @@ static int snps_get_ddrc_info(struct snps_edac_priv *priv)
 		return -ENXIO;
 	}
 
+	/* Assume HW-src scrub is always available if it isn't disabled */
+	if (!(regval & ECC_CFG0_DIS_SCRUB))
+		priv->info.caps |= SNPS_CAP_ECC_SCRUB;
+
 	/* Auto-detect the basic HIF/SDRAM bus parameters */
 	regval = readl(priv->baseaddr + DDR_MSTR_OFST);
 
@@ -1484,8 +1490,14 @@ static struct mem_ctl_info *snps_mc_create(struct snps_edac_priv *priv)
 			 MEM_FLAG_DDR3 | MEM_FLAG_LPDDR3 |
 			 MEM_FLAG_DDR4 | MEM_FLAG_LPDDR4;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
-	mci->scrub_cap = SCRUB_FLAG_HW_SRC;
-	mci->scrub_mode = SCRUB_NONE;
+
+	if (priv->info.caps & SNPS_CAP_ECC_SCRUB) {
+		mci->scrub_mode = SCRUB_HW_SRC;
+		mci->scrub_cap = SCRUB_FLAG_HW_SRC;
+	} else {
+		mci->scrub_mode = SCRUB_SW_SRC;
+		mci->scrub_cap = SCRUB_FLAG_SW_SRC;
+	}
 
 	mci->edac_cap = EDAC_FLAG_SECDED;
 	mci->ctl_name = "snps_umctl2_ddrc";
@@ -1578,6 +1590,8 @@ static int snps_ddrc_info_show(struct seq_file *s, void *data)
 
 	seq_puts(s, "Caps:");
 	if (priv->info.caps) {
+		if (priv->info.caps & SNPS_CAP_ECC_SCRUB)
+			seq_puts(s, " +Scrub");
 		if (priv->info.caps & SNPS_CAP_ZYNQMP)
 			seq_puts(s, " +ZynqMP");
 	} else {
-- 
2.35.1

[PATCH 05/13] EDAC/synopsys: Drop ECC poison address from private data

[Serge Semin]

Since the driver now has the generic Sys/SDRAM address translation
interface there is no need in preserving the poisonous address in the
driver private data especially seeing it is used in the framework of the
DebugFS node anyway. So let's drop the snps_edac_priv.poison_addr field
and just perform Sys/SDRAM back and forth address translation right in
place of the "inject_data_error" node accessors.

It causes a bit more modifications than a simple field removal. Since the
poisonous address is not preserved now there is no point in having the
snps_data_poison_setup() method so its content can be moved right into the
"inject_data_error" write operation. For the same reason there is no point
in printing the ECCPOISONADDR{0,1} registers content in the
"inject_data_error" read operation. Since the CSRs content is now parsed
anyway let's print the SDRAM address instead.

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 2b8de7e8fae1..05201f5a284e 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -407,7 +407,6 @@ struct snps_ecc_status {
  * @lock:		Concurrent CSRs access lock.
  * @message:		Buffer for framing the event specific info.
  * @stat:		ECC status information.
- * @poison_addr:	Data poison address.
  */
 struct snps_edac_priv {
 	struct snps_ddrc_info info;
@@ -418,9 +417,6 @@ struct snps_edac_priv {
 	spinlock_t lock;
 	char message[SNPS_EDAC_MSG_SIZE];
 	struct snps_ecc_status stat;
-#ifdef CONFIG_EDAC_DEBUG
-	ulong poison_addr;
-#endif
 };
 
 /**
@@ -1713,44 +1709,32 @@ static int snps_hif_sdram_map_show(struct seq_file *s, void *data)
 
 DEFINE_SHOW_ATTRIBUTE(snps_hif_sdram_map);
 
-/**
- * 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 snps_data_poison_setup(struct snps_edac_priv *priv)
-{
-	struct snps_sdram_addr sdram;
-	u32 regval;
-
-	snps_map_sys_to_sdram(priv, priv->poison_addr, &sdram);
-
-	regval = FIELD_PREP(ECC_POISON0_RANK_MASK, sdram.rank) |
-		 FIELD_PREP(ECC_POISON0_COL_MASK, sdram.col);
-	writel(regval, priv->baseaddr + ECC_POISON0_OFST);
-
-	regval = FIELD_PREP(ECC_POISON1_BANKGRP_MASK, sdram.bankgrp) |
-		 FIELD_PREP(ECC_POISON1_BANK_MASK, sdram.bank) |
-		 FIELD_PREP(ECC_POISON1_ROW_MASK, sdram.row);
-	writel(regval, priv->baseaddr + ECC_POISON1_OFST);
-}
-
 static ssize_t snps_inject_data_error_read(struct file *filep, char __user *ubuf,
 					   size_t size, loff_t *offp)
 {
 	struct mem_ctl_info *mci = filep->private_data;
 	struct snps_edac_priv *priv = mci->pvt_info;
+	struct snps_sdram_addr sdram;
 	char buf[SNPS_DBGFS_BUF_LEN];
+	dma_addr_t sys;
+	u32 regval;
 	int pos;
 
-	pos = scnprintf(buf, sizeof(buf), "Poison0 Addr: 0x%08x\n\r",
-			readl(priv->baseaddr + ECC_POISON0_OFST));
-	pos += scnprintf(buf + pos, sizeof(buf) - pos, "Poison1 Addr: 0x%08x\n\r",
-			 readl(priv->baseaddr + ECC_POISON1_OFST));
-	pos += scnprintf(buf + pos, sizeof(buf) - pos, "Error injection Address: 0x%lx\n\r",
-			 priv->poison_addr);
+	regval = readl(priv->baseaddr + ECC_POISON0_OFST);
+	sdram.rank = FIELD_GET(ECC_POISON0_RANK_MASK, regval);
+	sdram.col = FIELD_GET(ECC_POISON0_COL_MASK, regval);
+
+	regval = readl(priv->baseaddr + ECC_POISON1_OFST);
+	sdram.bankgrp = FIELD_PREP(ECC_POISON1_BANKGRP_MASK, regval);
+	sdram.bank = FIELD_PREP(ECC_POISON1_BANK_MASK, regval);
+	sdram.row = FIELD_PREP(ECC_POISON1_ROW_MASK, regval);
+
+	snps_map_sdram_to_sys(priv, &sdram, &sys);
+
+	pos = scnprintf(buf, sizeof(buf),
+			"%pad: Row %hu Rank %hu Bank %hhu Bank Group %hhu Rank %hhu\n",
+			&sys, sdram.row, sdram.col, sdram.bank, sdram.bankgrp,
+			sdram.rank);
 
 	return simple_read_from_buffer(ubuf, size, offp, buf, pos);
 }
@@ -1760,13 +1744,25 @@ static ssize_t snps_inject_data_error_write(struct file *filep, const char __use
 {
 	struct mem_ctl_info *mci = filep->private_data;
 	struct snps_edac_priv *priv = mci->pvt_info;
+	struct snps_sdram_addr sdram;
+	u32 regval;
+	u64 sys;
 	int rc;
 
-	rc = kstrtoul_from_user(ubuf, size, 0, &priv->poison_addr);
+	rc = kstrtou64_from_user(ubuf, size, 0, &sys);
 	if (rc)
 		return rc;
 
-	snps_data_poison_setup(priv);
+	snps_map_sys_to_sdram(priv, sys, &sdram);
+
+	regval = FIELD_PREP(ECC_POISON0_RANK_MASK, sdram.rank) |
+		 FIELD_PREP(ECC_POISON0_COL_MASK, sdram.col);
+	writel(regval, priv->baseaddr + ECC_POISON0_OFST);
+
+	regval = FIELD_PREP(ECC_POISON1_BANKGRP_MASK, sdram.bankgrp) |
+		 FIELD_PREP(ECC_POISON1_BANK_MASK, sdram.bank) |
+		 FIELD_PREP(ECC_POISON1_ROW_MASK, sdram.row);
+	writel(regval, priv->baseaddr + ECC_POISON1_OFST);
 
 	return size;
 }
-- 
2.35.1

[PATCH 06/13] EDAC/synopsys: Add data poisoning disable support

[Serge Semin]

Even though being a pure-debug feature currently the data poison can't be
disabled once it has been initialized and enabled. Irrespective to the way
the feature has been implemented it doesn't seem right since the system
may print false ECC errors in case if the poisoned address is accessed by
the kernel or by the user-space applications. It's possible since the
poisoned address isn't reserved in any kernel mm subsystems. Even though
that doesn't seem right either at least it's tolerable since the ECC data
poison is supposed to be utilized in the framework of the EDAC driver
debugging, but having the feature unswitchable can't be justified that
easy especially seeing it's not that hard to implement.

So in order to have the ECC data poison switchable we suggest to define
three possible values acceptable by the "inject_data_poison" DebugFS node:
1. "CE" - emit correctable error (as before).
2. "UE" - emit uncorrectable error (used to be any non-"CE" value).
3. Any other value - disable data poison feature.

Note we have to redefine the macros describing the data poison-related
fields of the ECC_CFG0 register in a way so they would be used to
separately switch the feature on/off and to select the type of the ECC
error. As a result the suggest solution turns into a proper ECC_CFG0 CSRs
fields setup based on the value written to the "inject_data_poison"
DebugFS node.

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 05201f5a284e..028a9ad70d49 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -118,6 +118,10 @@
 #define ECC_CFG0_DIS_SCRUB		BIT(4)
 #define ECC_CFG0_MODE_MASK		GENMASK(2, 0)
 
+/* ECC CFG1 register definitions */
+#define ECC_CFG1_POISON_BIT		BIT(1)
+#define ECC_CFG1_POISON_EN		BIT(0)
+
 /* ECC status register definitions */
 #define ECC_STAT_UE_MASK		GENMASK(23, 16)
 #define ECC_STAT_CE_MASK		GENMASK(15, 8)
@@ -157,10 +161,6 @@
 #define ECC_POISON1_BANK_MASK		GENMASK(26, 24)
 #define ECC_POISON1_ROW_MASK		GENMASK(17, 0)
 
-/* DDRC ECC CE & UE poison mask */
-#define ECC_CEPOISON_MASK		GENMASK(1, 0)
-#define ECC_UEPOISON_MASK		BIT(0)
-
 /* DDRC address mapping parameters */
 #define DDR_ADDRMAP_NREGS		12
 
@@ -1781,10 +1781,14 @@ static ssize_t snps_inject_data_poison_read(struct file *filep, char __user *ubu
 	int pos;
 
 	regval = readl(priv->baseaddr + ECC_CFG1_OFST);
-	errstr = FIELD_GET(ECC_CEPOISON_MASK, regval) == ECC_CEPOISON_MASK ?
-		 "Correctable Error" : "UnCorrectable Error";
+	if (!(regval & ECC_CFG1_POISON_EN))
+		errstr = "Off";
+	else if (regval & ECC_CFG1_POISON_BIT)
+		errstr = "CE";
+	else
+		errstr = "UE";
 
-	pos = scnprintf(buf, sizeof(buf), "Data Poisoning: %s\n\r", errstr);
+	pos = scnprintf(buf, sizeof(buf), "%s\n", errstr);
 
 	return simple_read_from_buffer(ubuf, size, offp, buf, pos);
 }
@@ -1795,6 +1799,7 @@ static ssize_t snps_inject_data_poison_write(struct file *filep, const char __us
 	struct mem_ctl_info *mci = filep->private_data;
 	struct snps_edac_priv *priv = mci->pvt_info;
 	char buf[SNPS_DBGFS_BUF_LEN];
+	u32 regval;
 	int rc;
 
 	rc = simple_write_to_buffer(buf, sizeof(buf), offp, ubuf, size);
@@ -1802,10 +1807,16 @@ static ssize_t snps_inject_data_poison_write(struct file *filep, const char __us
 		return rc;
 
 	writel(0, priv->baseaddr + DDR_SWCTL);
+
+	regval = readl(priv->baseaddr + ECC_CFG1_OFST);
 	if (strncmp(buf, "CE", 2) == 0)
-		writel(ECC_CEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
+		regval |= ECC_CFG1_POISON_BIT | ECC_CFG1_POISON_EN;
+	else if (strncmp(buf, "UE", 2) == 0)
+		regval = (regval & ~ECC_CFG1_POISON_BIT) | ECC_CFG1_POISON_EN;
 	else
-		writel(ECC_UEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
+		regval &= ~ECC_CFG1_POISON_EN;
+	writel(regval, priv->baseaddr + ECC_CFG1_OFST);
+
 	writel(1, priv->baseaddr + DDR_SWCTL);
 
 	return size;
-- 
2.35.1

[PATCH 07/13] EDAC/synopsys: Split up ECC UE/CE IRQs handler

[Serge Semin]

DW uMCTL2 DDRC IP-core doesn't have common IRQ line. Instead it provides
individual IRQ output signals for each controller event like: corrected
error, uncorrected error, DFI parity error, address protection, scrubber
done, and so on. So the common IRQ handler implemented in the Synopsys
EDAC driver isn't device-specific but is a particular platform specific.
Obviously it won't be suitable for the generic devices which are added to
the platforms with the original individual IRQs as it has happened in our
case.

So let's split up the common IRQ handler into two ones handling ECC
corrected and uncorrected errors. It won't be that hard since both
sub-methods it calls are already logically divided into two CE/UE parts.
What we need to do is to move these parts into the dedicated methods and
redefine the local variables a bit. The new methods will be simply called
from the common IRQs handler if one is utilized on the particular
platform. Otherwise each new IRQ handler will be called on particular
interrupt request (the IRQ handlers registration will be added a bit
later). Note we now can discard the snps_ecc_status structure as unneeded
since the error data is collected and reported now within a single method.

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 028a9ad70d49..ac0123ff4595 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -143,7 +143,6 @@
 #define DDR_QOS_IRQ_STAT_OFST		0x20200
 #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
 
@@ -372,31 +371,19 @@ struct snps_sdram_addr {
 /**
  * struct snps_ecc_error_info - ECC error log information.
  * @sdram:	SDRAM address.
+ * @ecnt:	Number of detected errors.
  * @bitpos:	Bit position.
  * @data:	Data causing the error.
  * @syndrome:	Erroneous data syndrome.
  */
 struct snps_ecc_error_info {
 	struct snps_sdram_addr sdram;
+	u16 ecnt;
 	u32 bitpos;
 	u64 data;
 	u32 syndrome;
 };
 
-/**
- * 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 snps_ecc_status {
-	u32 ce_cnt;
-	u32 ue_cnt;
-	struct snps_ecc_error_info ceinfo;
-	struct snps_ecc_error_info ueinfo;
-};
-
 /**
  * struct snps_edac_priv - DDR memory controller private data.
  * @info:		DDR controller config info.
@@ -406,7 +393,6 @@ struct snps_ecc_status {
  * @baseaddr:		Base address of the DDR controller.
  * @lock:		Concurrent CSRs access lock.
  * @message:		Buffer for framing the event specific info.
- * @stat:		ECC status information.
  */
 struct snps_edac_priv {
 	struct snps_ddrc_info info;
@@ -416,7 +402,6 @@ struct snps_edac_priv {
 	void __iomem *baseaddr;
 	spinlock_t lock;
 	char message[SNPS_EDAC_MSG_SIZE];
-	struct snps_ecc_status stat;
 };
 
 /**
@@ -688,130 +673,178 @@ static inline u32 snps_get_bitpos(u32 bitnum, enum snps_dq_width dq_width)
 }
 
 /**
- * snps_get_error_info - Get the current ECC error info.
- * @priv:	DDR memory controller private instance data.
+ * snps_ce_irq_handler - Corrected error interrupt handler.
+ * @irq:        IRQ number.
+ * @dev_id:     Device ID.
  *
- * Return: one if there is no error otherwise returns zero.
+ * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise.
  */
-static int snps_get_error_info(struct snps_edac_priv *priv)
+static irqreturn_t snps_ce_irq_handler(int irq, void *dev_id)
 {
-	struct snps_ecc_status *p;
-	u32 regval, clearval;
+	struct mem_ctl_info *mci = dev_id;
+	struct snps_edac_priv *priv = mci->pvt_info;
+	struct snps_ecc_error_info einfo;
 	unsigned long flags;
-	void __iomem *base;
+	u32 qosval, regval;
+	dma_addr_t sys;
 
-	base = priv->baseaddr;
-	p = &priv->stat;
+	/* Make sure IRQ is caused by a corrected ECC error */
+	if (priv->info.caps & SNPS_CAP_ZYNQMP) {
+		qosval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
+		if (!(qosval & DDR_QOSCE_MASK))
+			return IRQ_NONE;
 
-	regval = readl(base + ECC_STAT_OFST);
-	if (!regval)
-		return 1;
+		qosval &= DDR_QOSCE_MASK;
+	}
 
-	p->ceinfo.bitpos = FIELD_GET(ECC_STAT_BITNUM_MASK, regval);
+	regval = readl(priv->baseaddr + ECC_STAT_OFST);
+	if (!FIELD_GET(ECC_STAT_CE_MASK, regval))
+		return IRQ_NONE;
 
-	regval = readl(base + ECC_ERRCNT_OFST);
-	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;
+	/* Read error info like bit position, SDRAM address, data, syndrome */
+	einfo.bitpos = FIELD_GET(ECC_STAT_BITNUM_MASK, regval);
+	einfo.bitpos = snps_get_bitpos(einfo.bitpos, priv->info.dq_width);
 
-	p->ceinfo.bitpos = snps_get_bitpos(p->ceinfo.bitpos, priv->info.dq_width);
+	regval = readl(priv->baseaddr + ECC_ERRCNT_OFST);
+	einfo.ecnt = FIELD_GET(ECC_ERRCNT_CECNT_MASK, regval);
 
-	regval = readl(base + ECC_CEADDR0_OFST);
-	p->ceinfo.sdram.rank = FIELD_GET(ECC_CEADDR0_RANK_MASK, regval);
-	p->ceinfo.sdram.row = FIELD_GET(ECC_CEADDR0_ROW_MASK, regval);
+	regval = readl(priv->baseaddr + ECC_CEADDR0_OFST);
+	einfo.sdram.rank = FIELD_GET(ECC_CEADDR0_RANK_MASK, regval);
+	einfo.sdram.row = FIELD_GET(ECC_CEADDR0_ROW_MASK, regval);
 
-	regval = readl(base + ECC_CEADDR1_OFST);
-	p->ceinfo.sdram.bankgrp = FIELD_GET(ECC_CEADDR1_BANKGRP_MASK, regval);
-	p->ceinfo.sdram.bank = FIELD_GET(ECC_CEADDR1_BANK_MASK, regval);
-	p->ceinfo.sdram.col = FIELD_GET(ECC_CEADDR1_COL_MASK, regval);
+	regval = readl(priv->baseaddr + ECC_CEADDR1_OFST);
+	einfo.sdram.bankgrp = FIELD_GET(ECC_CEADDR1_BANKGRP_MASK, regval);
+	einfo.sdram.bank = FIELD_GET(ECC_CEADDR1_BANK_MASK, regval);
+	einfo.sdram.col = FIELD_GET(ECC_CEADDR1_COL_MASK, regval);
 
-	p->ceinfo.data = readl(base + ECC_CSYND0_OFST);
+	einfo.data = readl(priv->baseaddr + ECC_CSYND0_OFST);
 	if (priv->info.dq_width == SNPS_DQ_64)
-		p->ceinfo.data |= (u64)readl(base + ECC_CSYND1_OFST) << 32;
-
-	p->ceinfo.syndrome = readl(base + ECC_CSYND2_OFST);
+		einfo.data |= (u64)readl(priv->baseaddr + ECC_CSYND1_OFST) << 32;
 
-ue_err:
-	if (!p->ue_cnt)
-		goto out;
+	einfo.syndrome = readl(priv->baseaddr + ECC_CSYND2_OFST);
 
-	regval = readl(base + ECC_UEADDR0_OFST);
-	p->ueinfo.sdram.rank = FIELD_GET(ECC_CEADDR0_RANK_MASK, regval);
-	p->ueinfo.sdram.row = FIELD_GET(ECC_CEADDR0_ROW_MASK, regval);
+	/* Report the detected errors with the corresponding sys address */
+	snps_map_sdram_to_sys(priv, &einfo.sdram, &sys);
 
-	regval = readl(base + ECC_UEADDR1_OFST);
-	p->ueinfo.sdram.bankgrp = FIELD_GET(ECC_CEADDR1_BANKGRP_MASK, regval);
-	p->ueinfo.sdram.bank = FIELD_GET(ECC_CEADDR1_BANK_MASK, regval);
-	p->ueinfo.sdram.col = FIELD_GET(ECC_CEADDR1_COL_MASK, regval);
+	snprintf(priv->message, SNPS_EDAC_MSG_SIZE,
+		 "Row %hu Col %hu Bank %hhu Bank Group %hhu Rank %hhu Bit %d Data 0x%08llx",
+		 einfo.sdram.row, einfo.sdram.col, einfo.sdram.bank,
+		 einfo.sdram.bankgrp, einfo.sdram.rank,
+		 einfo.bitpos, einfo.data);
 
-	p->ueinfo.data = readl(base + ECC_UESYND0_OFST);
-	if (priv->info.dq_width == SNPS_DQ_64)
-		p->ueinfo.data |= (u64)readl(base + ECC_UESYND1_OFST) << 32;
-
-	p->ueinfo.syndrome = readl(base + ECC_UESYND2_OFST);
+	edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, einfo.ecnt,
+			     PHYS_PFN(sys), offset_in_page(sys),
+			     einfo.syndrome, einfo.sdram.rank, 0, -1,
+			     priv->message, "");
 
-out:
+	/* Make sure the CE IRQ status is cleared */
 	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);
+	regval = readl(priv->baseaddr + ECC_CLR_OFST) |
+		 ECC_CTRL_CLR_CE_ERR | ECC_CTRL_CLR_CE_ERRCNT;
+	writel(regval, priv->baseaddr + ECC_CLR_OFST);
 
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-	return 0;
+	if (priv->info.caps & SNPS_CAP_ZYNQMP)
+		writel(qosval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
+
+	return IRQ_HANDLED;
 }
 
 /**
- * snps_handle_error - Handle Correctable and Uncorrectable errors.
- * @mci:	EDAC memory controller instance.
- * @p:		Synopsys ECC status structure.
+ * snps_ue_irq_handler - Uncorrected error interrupt handler.
+ * @irq:        IRQ number.
+ * @dev_id:     Device ID.
  *
- * Handles ECC correctable and uncorrectable errors.
+ * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise.
  */
-static void snps_handle_error(struct mem_ctl_info *mci, struct snps_ecc_status *p)
+static irqreturn_t snps_ue_irq_handler(int irq, void *dev_id)
 {
+	struct mem_ctl_info *mci = dev_id;
 	struct snps_edac_priv *priv = mci->pvt_info;
-	struct snps_ecc_error_info *pinf;
+	struct snps_ecc_error_info einfo;
+	unsigned long flags;
+	u32 qosval, regval;
 	dma_addr_t sys;
 
-	if (p->ce_cnt) {
-		pinf = &p->ceinfo;
+	/* Make sure IRQ is caused by an uncorrected ECC error */
+	if (priv->info.caps & SNPS_CAP_ZYNQMP) {
+		qosval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
+		if (!(regval & DDR_QOSUE_MASK))
+			return IRQ_NONE;
+
+		qosval &= DDR_QOSUE_MASK;
+	}
 
-		snprintf(priv->message, SNPS_EDAC_MSG_SIZE,
-			 "Row %hu Col %hu Bank %hhu Bank Group %hhu Rank %hhu Bit %d Data 0x%08llx",
-			 pinf->sdram.row, pinf->sdram.col, pinf->sdram.bank,
-			 pinf->sdram.bankgrp, pinf->sdram.rank,
-			 pinf->bitpos, pinf->data);
+	regval = readl(priv->baseaddr + ECC_STAT_OFST);
+	if (!FIELD_GET(ECC_STAT_UE_MASK, regval))
+		return IRQ_NONE;
 
-		snps_map_sdram_to_sys(priv, &pinf->sdram, &sys);
+	/* Read error info like SDRAM address, data and syndrome */
+	regval = readl(priv->baseaddr + ECC_ERRCNT_OFST);
+	einfo.ecnt = FIELD_GET(ECC_ERRCNT_UECNT_MASK, regval);
 
-		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, p->ce_cnt,
-				     PHYS_PFN(sys), offset_in_page(sys),
-				     pinf->syndrome, pinf->sdram.rank, 0, -1,
-				     priv->message, "");
-	}
+	regval = readl(priv->baseaddr + ECC_UEADDR0_OFST);
+	einfo.sdram.rank = FIELD_GET(ECC_CEADDR0_RANK_MASK, regval);
+	einfo.sdram.row = FIELD_GET(ECC_CEADDR0_ROW_MASK, regval);
 
-	if (p->ue_cnt) {
-		pinf = &p->ueinfo;
+	regval = readl(priv->baseaddr + ECC_UEADDR1_OFST);
+	einfo.sdram.bankgrp = FIELD_GET(ECC_CEADDR1_BANKGRP_MASK, regval);
+	einfo.sdram.bank = FIELD_GET(ECC_CEADDR1_BANK_MASK, regval);
+	einfo.sdram.col = FIELD_GET(ECC_CEADDR1_COL_MASK, regval);
 
-		snprintf(priv->message, SNPS_EDAC_MSG_SIZE,
-			 "Row %hu Col %hu Bank %hhu Bank Group %hhu Rank %hhu Data 0x%08llx",
-			 pinf->sdram.row, pinf->sdram.col, pinf->sdram.bank,
-			 pinf->sdram.bankgrp, pinf->sdram.rank,
-			 pinf->data);
+	einfo.data = readl(priv->baseaddr + ECC_UESYND0_OFST);
+	if (priv->info.dq_width == SNPS_DQ_64)
+		einfo.data |= (u64)readl(priv->baseaddr + ECC_UESYND1_OFST) << 32;
 
-		snps_map_sdram_to_sys(priv, &pinf->sdram, &sys);
+	einfo.syndrome = readl(priv->baseaddr + ECC_UESYND2_OFST);
 
-		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, p->ue_cnt,
-				     PHYS_PFN(sys), offset_in_page(sys),
-				     pinf->syndrome, pinf->sdram.rank, 0, -1,
-				     priv->message, "");
-	}
+	/* Report the detected errors with the corresponding sys address */
+	snps_map_sdram_to_sys(priv, &einfo.sdram, &sys);
+
+	snprintf(priv->message, SNPS_EDAC_MSG_SIZE,
+		 "Row %hu Col %hu Bank %hhu Bank Group %hhu Rank %hhu Data 0x%08llx",
+		 einfo.sdram.row, einfo.sdram.col, einfo.sdram.bank,
+		 einfo.sdram.bankgrp, einfo.sdram.rank,
+		 einfo.data);
+
+	edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, einfo.ecnt,
+			     PHYS_PFN(sys), offset_in_page(sys),
+			     einfo.syndrome, einfo.sdram.rank, 0, -1,
+			     priv->message, "");
+
+	/* Make sure the UE IRQ status is cleared */
+	spin_lock_irqsave(&priv->lock, flags);
+
+	regval = readl(priv->baseaddr + ECC_CLR_OFST) |
+		 ECC_CTRL_CLR_UE_ERR | ECC_CTRL_CLR_UE_ERRCNT;
+	writel(regval, priv->baseaddr + ECC_CLR_OFST);
 
-	memset(p, 0, sizeof(*p));
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	if (priv->info.caps & SNPS_CAP_ZYNQMP)
+		writel(qosval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * snps_com_irq_handler - Interrupt IRQ signal handler.
+ * @irq:        IRQ number.
+ * @dev_id:     Device ID.
+ *
+ * Return: IRQ_NONE, if interrupts not set or IRQ_HANDLED otherwise.
+ */
+static irqreturn_t snps_com_irq_handler(int irq, void *dev_id)
+{
+	irqreturn_t rc = IRQ_NONE;
+
+	rc |= snps_ce_irq_handler(irq, dev_id);
+
+	rc |= snps_ue_irq_handler(irq, dev_id);
+
+	return rc;
 }
 
 static void snps_enable_irq(struct snps_edac_priv *priv)
@@ -854,41 +887,6 @@ static void snps_disable_irq(struct snps_edac_priv *priv)
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
-/**
- * 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 snps_irq_handler(int irq, void *dev_id)
-{
-	struct mem_ctl_info *mci = dev_id;
-	struct snps_edac_priv *priv;
-	int status, regval;
-
-	priv = mci->pvt_info;
-
-	if (priv->info.caps & SNPS_CAP_ZYNQMP) {
-		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 = snps_get_error_info(priv);
-	if (status)
-		return IRQ_NONE;
-
-	snps_handle_error(mci, &priv->stat);
-
-
-	if (priv->info.caps & SNPS_CAP_ZYNQMP)
-		writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
-
-	return IRQ_HANDLED;
-}
-
 /**
  * snps_create_data - Create private data.
  * @pdev:	platform device.
@@ -1538,7 +1536,7 @@ static int snps_setup_irq(struct mem_ctl_info *mci)
 		return irq;
 	}
 
-	ret = devm_request_irq(&priv->pdev->dev, irq, snps_irq_handler,
+	ret = devm_request_irq(&priv->pdev->dev, irq, snps_com_irq_handler,
 			       0, dev_name(&priv->pdev->dev), mci);
 	if (ret < 0) {
 		edac_printk(KERN_ERR, EDAC_MC, "Failed to request IRQ\n");
-- 
2.35.1

[PATCH 08/13] EDAC/synopsys: Add individual named ECC IRQs support

[Serge Semin]

Currently the DW uMCTL2 DDRC EDAC driver supports a common unnamed IRQ
only. It isn't suitable for our platform which has the individual IRQ
lines for each DDRC event (ECC UE, ECC CE, DFI parity error, Scrubber
done, etc).  Moreover the DW uMCTL2 DDRC IP-core doesn't have an option to
be configured with a common interrupts output line. So in order to have
the generic DW uMCTL2 DDR controller and our platform supported by the
driver we need to add the individual, per DDRC event, IRQs request
support. There is not much to do really since the common IRQs handler has
already been split up into the sub-handlers. So the only thing we need to
do is first try to request the individual IRQs, if failed then fallback to
the common IRQ. The IRQ names are used in accordance with the DW uMCTL2
DDRC DT-bindings.

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index ac0123ff4595..e7d59238c5ca 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -1524,25 +1524,96 @@ static void snps_mc_free(struct mem_ctl_info *mci)
 	edac_mc_free(mci);
 }
 
-static int snps_setup_irq(struct mem_ctl_info *mci)
+/**
+ * snps_request_ind_irq - Request individual DDRC IRQs.
+ * @mci:	EDAC memory controller instance.
+ *
+ * Return: 0 if the IRQs were successfully requested, 1 if the individual IRQs
+ * are unavailable, otherwise negative errno.
+ */
+static int snps_request_ind_irq(struct mem_ctl_info *mci)
 {
 	struct snps_edac_priv *priv = mci->pvt_info;
-	int ret, irq;
+	struct device *dev = &priv->pdev->dev;
+	int rc, irq;
 
-	irq = platform_get_irq(priv->pdev, 0);
-	if (irq < 0) {
-		edac_printk(KERN_ERR, EDAC_MC,
-			    "No IRQ %d in DT\n", irq);
+	irq = platform_get_irq_byname_optional(priv->pdev, "ecc_ce");
+	if (irq == -ENXIO)
+		return 1;
+	if (irq < 0)
+		return irq;
+
+	rc = devm_request_irq(dev, irq, snps_ce_irq_handler, 0, "ecc_ce", mci);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC, "Failed to request ECC CE IRQ\n");
+		return rc;
+	}
+
+	irq = platform_get_irq_byname(priv->pdev, "ecc_ue");
+	if (irq < 0)
 		return irq;
+
+	rc = devm_request_irq(dev, irq, snps_ue_irq_handler, 0, "ecc_ue", mci);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC, "Failed to request ECC UE IRQ\n");
+		return rc;
+	}
+
+	return 0;
+}
+
+/**
+ * snps_request_com_irq - Request common DDRC IRQ.
+ * @mci:	EDAC memory controller instance.
+ *
+ * It first attempts to get the named IRQ. If failed the method fallbacks
+ * to first available one.
+ *
+ * Return: 0 if the IRQ was successfully requested otherwise negative errno.
+ */
+static int snps_request_com_irq(struct mem_ctl_info *mci)
+{
+	struct snps_edac_priv *priv = mci->pvt_info;
+	struct device *dev = &priv->pdev->dev;
+	int rc, irq;
+
+	irq = platform_get_irq_byname_optional(priv->pdev, "ecc");
+	if (irq < 0) {
+		irq = platform_get_irq(priv->pdev, 0);
+		if (irq < 0)
+			return irq;
 	}
 
-	ret = devm_request_irq(&priv->pdev->dev, irq, snps_com_irq_handler,
-			       0, dev_name(&priv->pdev->dev), mci);
-	if (ret < 0) {
+	rc = devm_request_irq(dev, irq, snps_com_irq_handler, 0, "ecc", mci);
+	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC, "Failed to request IRQ\n");
-		return ret;
+		return rc;
 	}
 
+	return 0;
+}
+
+/**
+ * snps_setup_irq - Request and enable DDRC IRQs.
+ * @mci:	EDAC memory controller instance.
+ *
+ * It first tries to get and request individual IRQs. If failed the method
+ * fallbacks to the common IRQ line case. The IRQs will be enabled only if
+ * some of these requests have been successful.
+ *
+ * Return: 0 if IRQs were successfully setup otherwise negative errno.
+ */
+static int snps_setup_irq(struct mem_ctl_info *mci)
+{
+	struct snps_edac_priv *priv = mci->pvt_info;
+	int rc;
+
+	rc = snps_request_ind_irq(mci);
+	if (rc > 0)
+		rc = snps_request_com_irq(mci);
+	if (rc)
+		return rc;
+
 	snps_enable_irq(priv);
 
 	return 0;
-- 
2.35.1

[PATCH 09/13] EDAC/synopsys: Add DFI alert_n IRQ support

[Serge Semin]

In accordance with [1] DW uMCTL2 DDR controller can generate an IRQ in
case if an attached SDRAM detects a CRC/Parity error. That capability is
mainly applicable for the DDR4 memory which has an additional signals
PARITY/ALERT_n indicating the even SDRAM address/command parity signal and
alert if the parity turns to be not even. But in accordance with [1] at
least the SDRAM address/command parity is calculated irrespective of the
memory protocol and then sent out by means of the dfi_parity_n signal
further to the DDR PHY. So depending on the DDR protocol and the DDR PHY
implementation the CRC/Parity error can be checked at some point
independently from the DDR devices type and then signaled via the
dfi_alert_n line. In anycase it would be very much useful to catch the
event and at least warn the user about problems with the DFI/SDRAM signals
integrity.

So here we suggest to add the DFI CRC/Parity IRQs handling support. First
the IRQ line is requested by the name "dfi_e" (defined in the DT-bindings)
and register its handler in case of the platform with the individual DW
uMCTL2 DDRC IRQs. If individual IRQs are unavailable the common IRQ
handler will call the DFI CRC/Parity event handler. Note the handler
doesn't do much. It just checks the IRQ status, reads the number of
errors, reports the fatal error to the MCI core and clears the IRQ status.
Alas neither the erroneous SDRAM address nor the executed command are
available in this case. Secondly the DFI CRC/Parity IRQ is
enabled/disabled together with the ECC CE/UE interrupts in the controller
probe procedure.  Finally the CRC/Parity capability is advertised by the
EDAC controller capabilities flags.

[1] DesignWare® Cores Enhanced Universal DDR Memory Controller (uMCTL2)
    Databook, Version 3.91a, October 2020, p.131-132

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index e7d59238c5ca..cdfa0e16bc5c 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -80,6 +80,12 @@
 #define ECC_POISON0_OFST		0xB8
 #define ECC_POISON1_OFST		0xBC
 
+/* DDR CRC/Parity register */
+#define DDR_CRCPARCTL0_OFST		0xC0
+#define DDR_CRCPARCTL1_OFST		0xC4
+#define DDR_CRCPARCTL2_OFST		0xC8
+#define DDR_CRCPARSTAT_OFST		0xCC
+
 /* DDR Address map0 Registers */
 #define DDR_ADDRMAP0_OFST		0x200
 
@@ -153,6 +159,13 @@
 #define ECC_CEADDR1_BANK_MASK		GENMASK(23, 16)
 #define ECC_CEADDR1_COL_MASK		GENMASK(11, 0)
 
+/* DDR CRC/Parity register definitions */
+#define DDR_CRCPARCTL0_CLR_ALRT_ERRCNT	BIT(2)
+#define DDR_CRCPARCTL0_CLR_ALRT_ERR	BIT(1)
+#define DDR_CRCPARCTL0_EN_ALRT_IRQ	BIT(0)
+#define DDR_CRCPARSTAT_ALRT_ERR		BIT(16)
+#define DDR_CRCPARSTAT_ALRT_CNT_MASK	GENMASK(15, 0)
+
 /* ECC Poison register shifts */
 #define ECC_POISON0_RANK_MASK		GENMASK(27, 24)
 #define ECC_POISON0_COL_MASK		GENMASK(11, 0)
@@ -829,6 +842,48 @@ static irqreturn_t snps_ue_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/**
+ * snps_dfi_irq_handler - DFI CRC/Parity error interrupt handler.
+ * @irq:        IRQ number.
+ * @dev_id:     Device ID.
+ *
+ * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise.
+ */
+static irqreturn_t snps_dfi_irq_handler(int irq, void *dev_id)
+{
+	struct mem_ctl_info *mci = dev_id;
+	struct snps_edac_priv *priv = mci->pvt_info;
+	unsigned long flags;
+	u32 regval;
+	u16 ecnt;
+
+	/* Make sure IRQ is caused by an DFI alert error */
+	regval = readl(priv->baseaddr + DDR_CRCPARSTAT_OFST);
+	if (!(regval & DDR_CRCPARSTAT_ALRT_ERR))
+		return IRQ_NONE;
+
+	/* Just a number of CRC/Parity errors is available */
+	ecnt = FIELD_GET(DDR_CRCPARSTAT_ALRT_CNT_MASK, regval);
+
+	/* Report the detected errors with just the custom message */
+	snprintf(priv->message, SNPS_EDAC_MSG_SIZE,
+		 "DFI CRC/Parity error detected on dfi_alert_n");
+
+	edac_mc_handle_error(HW_EVENT_ERR_FATAL, mci, ecnt,
+			     0, 0, 0, 0, 0, -1, priv->message, "");
+
+	/* Make sure the DFI alert IRQ status is cleared */
+	spin_lock_irqsave(&priv->lock, flags);
+
+	regval = readl(priv->baseaddr + DDR_CRCPARCTL0_OFST) |
+		 DDR_CRCPARCTL0_CLR_ALRT_ERR | DDR_CRCPARCTL0_CLR_ALRT_ERRCNT;
+	writel(regval, priv->baseaddr + DDR_CRCPARCTL0_OFST);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
 /**
  * snps_com_irq_handler - Interrupt IRQ signal handler.
  * @irq:        IRQ number.
@@ -844,6 +899,8 @@ static irqreturn_t snps_com_irq_handler(int irq, void *dev_id)
 
 	rc |= snps_ue_irq_handler(irq, dev_id);
 
+	rc |= snps_dfi_irq_handler(irq, dev_id);
+
 	return rc;
 }
 
@@ -859,11 +916,16 @@ static void snps_enable_irq(struct snps_edac_priv *priv)
 		return;
 	}
 
-	/* IRQs Enable/Disable feature has been available since v3.10a */
+	/*
+	 * ECC IRQs Enable/Disable feature has been available since v3.10a,
+	 * while CRC/Parity interrupts control - since v2.10a.
+	 */
 	spin_lock_irqsave(&priv->lock, flags);
 
 	writel(ECC_CTRL_EN_CE_IRQ | ECC_CTRL_EN_UE_IRQ,
 	       priv->baseaddr + ECC_CLR_OFST);
+	writel(DDR_CRCPARCTL0_EN_ALRT_IRQ,
+	       priv->baseaddr + DDR_CRCPARCTL0_OFST);
 
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
@@ -883,6 +945,7 @@ static void snps_disable_irq(struct snps_edac_priv *priv)
 	spin_lock_irqsave(&priv->lock, flags);
 
 	writel(0, priv->baseaddr + ECC_CLR_OFST);
+	writel(0, priv->baseaddr + DDR_CRCPARCTL0_OFST);
 
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
@@ -1483,7 +1546,8 @@ static struct mem_ctl_info *snps_mc_create(struct snps_edac_priv *priv)
 	mci->mtype_cap = MEM_FLAG_LPDDR | MEM_FLAG_DDR2 | MEM_FLAG_LPDDR2 |
 			 MEM_FLAG_DDR3 | MEM_FLAG_LPDDR3 |
 			 MEM_FLAG_DDR4 | MEM_FLAG_LPDDR4;
-	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED | EDAC_FLAG_PARITY;
+	mci->edac_cap = mci->edac_ctl_cap;
 
 	if (priv->info.caps & SNPS_CAP_ECC_SCRUB) {
 		mci->scrub_mode = SCRUB_HW_SRC;
@@ -1493,7 +1557,6 @@ static struct mem_ctl_info *snps_mc_create(struct snps_edac_priv *priv)
 		mci->scrub_cap = SCRUB_FLAG_SW_SRC;
 	}
 
-	mci->edac_cap = EDAC_FLAG_SECDED;
 	mci->ctl_name = "snps_umctl2_ddrc";
 	mci->dev_name = SNPS_EDAC_MOD_STRING;
 	mci->mod_name = SNPS_EDAC_MOD_VER;
@@ -1559,6 +1622,15 @@ static int snps_request_ind_irq(struct mem_ctl_info *mci)
 		return rc;
 	}
 
+	irq = platform_get_irq_byname_optional(priv->pdev, "dfi_e");
+	if (irq > 0) {
+		rc = devm_request_irq(dev, irq, snps_dfi_irq_handler, 0, "dfi_e", mci);
+		if (rc) {
+			edac_printk(KERN_ERR, EDAC_MC, "Failed to request DFI IRQ\n");
+			return rc;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.35.1

[PATCH 10/13] EDAC/synopsys: Add reference clocks support

[Serge Semin]

Currently the driver doesn't support any clock-related resources request
and handling, fairly assuming that all of them are supposed to be enabled
anyway in order for the system to work correctly. It's true for the Core
and AXI Ports reference clocks, but the CSR (APB) and Scrubber clocks
might still be disabled in case if the system firmware doesn't imply any
other software touching the DDR controller internals. Since the DW uMCTL2
DDRC driver does access the controller registers at the very least we need
to make sure the APB clock is enabled. Let's add the reference clocks
support then. First of all the driver will request all the clocks possibly
defined for the controller (Core, AXI, APB and Scrubber). Secondly the APB
clock will be enabled/disabled only since the Scrubber is currently
unsupported by the driver, and the Core and AXI clocks feed the critical
system parts so we need to avoid touching them with a risk to de-stabilize
the system memory. Please note the clocks connection IDs have been chosen
in accordance with the DT-bindings.

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index cdfa0e16bc5c..d6d5dfabddf5 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -8,6 +8,7 @@
 
 #include <linux/bits.h>
 #include <linux/bitfield.h>
+#include <linux/clk.h>
 #include <linux/edac.h>
 #include <linux/fs.h>
 #include <linux/log2.h>
@@ -301,6 +302,25 @@ enum snps_ecc_mode {
 	SNPS_ECC_ADVX4X8 = 5,
 };
 
+/**
+ * enum snps_ref_clk - DW uMCTL2 DDR controller clocks.
+ * @SNPS_CSR_CLK:	CSR/APB interface clock.
+ * @SNPS_AXI_CLK:	AXI (AHB) Port reference clock.
+ * @SNPS_CORE_CLK:	DDR controller (including DFI) clock. SDRAM clock
+ *			matches runs with this freq in 1:1 ratio mode and
+ *			with twice of this freq in case of 1:2 ratio mode.
+ * @SNPS_SBR_CLK:	Scrubber port reference clock (synchronous to
+ *			the core clock).
+ * @SNPS_MAX_NCLK:	Total number of clocks.
+ */
+enum snps_ref_clk {
+	SNPS_CSR_CLK,
+	SNPS_AXI_CLK,
+	SNPS_CORE_CLK,
+	SNPS_SBR_CLK,
+	SNPS_MAX_NCLK
+};
+
 /**
  * struct snps_ddrc_info - DDR controller platform parameters.
  * @caps:		DDR controller capabilities.
@@ -405,6 +425,7 @@ struct snps_ecc_error_info {
  * @pdev:		Platform device.
  * @baseaddr:		Base address of the DDR controller.
  * @lock:		Concurrent CSRs access lock.
+ * @clks:		Controller reference clocks.
  * @message:		Buffer for framing the event specific info.
  */
 struct snps_edac_priv {
@@ -414,6 +435,7 @@ struct snps_edac_priv {
 	struct platform_device *pdev;
 	void __iomem *baseaddr;
 	spinlock_t lock;
+	struct clk_bulk_data clks[SNPS_MAX_NCLK];
 	char message[SNPS_EDAC_MSG_SIZE];
 };
 
@@ -974,6 +996,60 @@ static struct snps_edac_priv *snps_create_data(struct platform_device *pdev)
 	return priv;
 }
 
+/**
+ * snps_get_res - Get platform device resources.
+ * @priv:	DDR memory controller private instance data.
+ *
+ * It's supposed to request all the controller resources available for the
+ * particular platform and enable all the required for the driver normal
+ * work. Note only the CSR and Scrubber clocks are supposed to be switched
+ * on/off by the driver.
+ *
+ * Return: negative errno if failed to get the resources, otherwise - zero.
+ */
+static int snps_get_res(struct snps_edac_priv *priv)
+{
+	const char * const ids[] = {
+		[SNPS_CSR_CLK] = "pclk",
+		[SNPS_AXI_CLK] = "aclk",
+		[SNPS_CORE_CLK] = "core",
+		[SNPS_SBR_CLK] = "sbr",
+	};
+	int i, rc;
+
+	for (i = 0; i < SNPS_MAX_NCLK; i++)
+		priv->clks[i].id = ids[i];
+
+	rc = devm_clk_bulk_get_optional(&priv->pdev->dev, SNPS_MAX_NCLK,
+					priv->clks);
+	if (rc) {
+		edac_printk(KERN_INFO, EDAC_MC, "Failed to get ref clocks\n");
+		return rc;
+	}
+
+	/*
+	 * Don't touch the Core and AXI clocks since they are critical for the
+	 * stable system functioning and are supposed to have been enabled
+	 * anyway.
+	 */
+	rc = clk_prepare_enable(priv->clks[SNPS_CSR_CLK].clk);
+	if (rc) {
+		edac_printk(KERN_INFO, EDAC_MC, "Couldn't enable CSR clock\n");
+		return rc;
+	}
+
+	return 0;
+}
+
+/**
+ * snps_put_res - Put platform device resources.
+ * @priv:	DDR memory controller private instance data.
+ */
+static void snps_put_res(struct snps_edac_priv *priv)
+{
+	clk_disable_unprepare(priv->clks[SNPS_CSR_CLK].clk);
+}
+
 /*
  * zynqmp_init_plat - ZynqMP-specific platform initialization.
  * @priv:	DDR memory controller private data.
@@ -1707,9 +1783,17 @@ static int snps_ddrc_info_show(struct seq_file *s, void *data)
 {
 	struct mem_ctl_info *mci = s->private;
 	struct snps_edac_priv *priv = mci->pvt_info;
+	unsigned long rate;
 
 	seq_printf(s, "SDRAM: %s\n", edac_mem_types[priv->info.sdram_mode]);
 
+	rate = clk_get_rate(priv->clks[SNPS_CORE_CLK].clk);
+	if (rate) {
+		rate = rate / HZ_PER_MHZ;
+		seq_printf(s, "Clock: Core %luMHz SDRAM %luMHz\n",
+			   rate, priv->info.freq_ratio * rate);
+	}
+
 	seq_printf(s, "DQ bus: %u/%s\n", (BITS_PER_BYTE << priv->info.dq_width),
 		   priv->info.dq_mode == SNPS_DQ_FULL ? "Full" :
 		   priv->info.dq_mode == SNPS_DQ_HALF ? "Half" :
@@ -2018,15 +2102,21 @@ static int snps_mc_probe(struct platform_device *pdev)
 	if (IS_ERR(priv))
 		return PTR_ERR(priv);
 
-	rc = snps_get_ddrc_info(priv);
+	rc = snps_get_res(priv);
 	if (rc)
 		return rc;
 
+	rc = snps_get_ddrc_info(priv);
+	if (rc)
+		goto put_res;
+
 	snps_get_addr_map(priv);
 
 	mci = snps_mc_create(priv);
-	if (IS_ERR(mci))
-		return PTR_ERR(mci);
+	if (IS_ERR(mci)) {
+		rc = PTR_ERR(mci);
+		goto put_res;
+	}
 
 	rc = snps_setup_irq(mci);
 	if (rc)
@@ -2046,6 +2136,9 @@ static int snps_mc_probe(struct platform_device *pdev)
 free_edac_mc:
 	snps_mc_free(mci);
 
+put_res:
+	snps_put_res(priv);
+
 	return rc;
 }
 
@@ -2066,6 +2159,8 @@ static int snps_mc_remove(struct platform_device *pdev)
 
 	snps_mc_free(mci);
 
+	snps_put_res(priv);
+
 	return 0;
 }
 
-- 
2.35.1

[PATCH 11/13] EDAC/synopsys: Add ECC Scrubber support

[Serge Semin]

DW uMCTL2 DDR controller IP-core can by synthesized with an embedded
Scrubber engine. The ECC Scrubber (SBR) is a block which initiates
periodic background burst read commands to the DDRC and further towards
the DDR memory in an attempt to trigger Correctable or Uncorrectable
errors. If a Correctable error is detected the ECC Scrub feature will
execute the Read-Modify-Write (RMW) procedure in order to fix the ECC. In
case of the Uncorrectable error it will be just reported as the
corresponding IRQ event. So it's definitely very useful feature. Let's add
it to the driver then especially seeing the MCI core already has some
infrastructure for it.

First of all even though the Core clock rate is only used for the Scrub
rate calculations we need to have the Scrubber clock enabled if one is
supplied otherwise the engine won't work.

Secondly the Scrubber engine support needs to be detected. Alas there is
no any special CSR indicating whether the DW uMCTL2 DDRC IP-core has been
synthesized with one embedded. Instead we suggest to implement the
detection procedure based on the Scrubber-specific CSRs writability. So if
the SBRWDATA0 CSR is writable then the CSR exists, which means the
Scrubber is available, otherwise the capability will be considered as
absent.

Thirdly the MCI core provides two callbacks utilized for the Scrubber
tuning: set the Scrubber bandwidth in bytes, which can also be used to
disable the periodic scrubbing, and get the Scrubber bandwidth (zero if
disabled). We can implement both of them by using the Scrubber CSRs the
controller provides. In particular aside with the back-to-back periodic
reads the Scrubber provides a way to delay the next read command for the
predefined set of 512's Core/Scrubber clock cycles. It can be used to
change the Scrubber bandwidth from the DDR maximal bandwidth (no delay) to
up to (0x1FFF * 512) Core/Scrubber clock cycles (see the inline comments
for details and utilized formulae). Note the Scrubber clock must be
synchronous to the Core clock by the controller design so here we get to
use the Core clock rate for the calculations. Pleas also note if no Core
clock specified the Scrubber will still be supported, but the bandwidth
will be used directly to calculate the Scrubber reads interval. The
back-to-back reads mode in this case will be indicated by the INT_MAX
bandwidth.

Fourthly the back-to-back scrubbing most likely will cause the significant
system performance drop. The manual says that it has been added to the
controller for the initial SDRAM initialization and the fast SDRAM
scrubbing after getting out of the low-power state. In anyway it is
supposed to be enabled only for a single SDRAM pass. We get to preserve
that semantic here so the back-to-back scrubbing will be disabled in the
Scrubber Done IRQ handler.

Finally the denoted scrub-rate callbacks and the SCRUB_FLAG_HW_PROG and
SCRUB_FLAG_HW_TUN flags will set to the MCI descriptor based on the
detected Scrubber capability. So no capability - no flags and no
callbacks.

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index d6d5dfabddf5..f86d1be2702a 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -18,6 +18,7 @@
 #include <linux/seq_file.h>
 #include <linux/sizes.h>
 #include <linux/spinlock.h>
+#include <linux/units.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -34,6 +35,7 @@
 
 /* DDR capabilities */
 #define SNPS_CAP_ECC_SCRUB		BIT(0)
+#define SNPS_CAP_ECC_SCRUBBER		BIT(1)
 #define SNPS_CAP_ZYNQMP			BIT(31)
 
 /* Synopsys uMCTL2 DDR controller registers that are relevant to ECC */
@@ -102,6 +104,12 @@
 #define DDR_SARBASE0_OFST		0xF04
 #define DDR_SARSIZE0_OFST		0xF08
 
+/* ECC Scrubber registers */
+#define ECC_SBRCTL_OFST			0xF24
+#define ECC_SBRSTAT_OFST		0xF28
+#define ECC_SBRWDATA0_OFST		0xF2C
+#define ECC_SBRWDATA1_OFST		0xF30
+
 /* DDR Master Register 0 definitions */
 #define DDR_MSTR_DEV_CFG_MASK		GENMASK(31, 30)
 #define DDR_MSTR_DEV_X4			0x0
@@ -244,6 +252,18 @@
 #define DDR_MAX_NSAR			4
 #define DDR_MIN_SARSIZE			SZ_256M
 
+/* ECC Scrubber registers definitions */
+#define ECC_SBRCTL_SCRUB_INTERVAL	GENMASK(20, 8)
+#define ECC_SBRCTL_INTERVAL_STEP	512
+#define ECC_SBRCTL_INTERVAL_MIN		0
+#define ECC_SBRCTL_INTERVAL_SAFE	1
+#define ECC_SBRCTL_INTERVAL_MAX		(ECC_SBRCTL_SCRUB_INTERVAL >> 8)
+#define ECC_SBRCTL_SCRUB_BURST		GENMASK(6, 4)
+#define ECC_SBRCTL_SCRUB_MODE_WR	BIT(2)
+#define ECC_SBRCTL_SCRUB_EN		BIT(0)
+#define ECC_SBRSTAT_SCRUB_DONE		BIT(1)
+#define ECC_SBRSTAT_SCRUB_BUSY		BIT(0)
+
 /**
  * enum snps_dq_width - SDRAM DQ bus width (ECC capable).
  * SNPS_DQ_32:	32-bit memory data width.
@@ -906,6 +926,47 @@ static irqreturn_t snps_dfi_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/**
+ * snps_sbr_irq_handler - Scrubber Done interrupt handler.
+ * @irq:        IRQ number.
+ * @dev_id:     Device ID.
+ *
+ * It just checks whether the IRQ has been caused by the Scrubber Done event
+ * and disables the back-to-back scrubbing by falling back to the smallest
+ * delay between the Scrubber read commands.
+ *
+ * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise.
+ */
+static irqreturn_t snps_sbr_irq_handler(int irq, void *dev_id)
+{
+	struct mem_ctl_info *mci = dev_id;
+	struct snps_edac_priv *priv = mci->pvt_info;
+	unsigned long flags;
+	u32 regval, en;
+
+	/* Make sure IRQ is caused by the Scrubber Done event */
+	regval = readl(priv->baseaddr + ECC_SBRSTAT_OFST);
+	if (!(regval & ECC_SBRSTAT_SCRUB_DONE))
+		return IRQ_NONE;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	regval = readl(priv->baseaddr + ECC_SBRCTL_OFST);
+	en = regval & ECC_SBRCTL_SCRUB_EN;
+	writel(regval & ~en, priv->baseaddr + ECC_SBRCTL_OFST);
+
+	regval = FIELD_PREP(ECC_SBRCTL_SCRUB_INTERVAL, ECC_SBRCTL_INTERVAL_SAFE);
+	writel(regval, priv->baseaddr + ECC_SBRCTL_OFST);
+
+	writel(regval | en, priv->baseaddr + ECC_SBRCTL_OFST);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	edac_mc_printk(mci, KERN_WARNING, "Back-to-back scrubbing disabled\n");
+
+	return IRQ_HANDLED;
+}
+
 /**
  * snps_com_irq_handler - Interrupt IRQ signal handler.
  * @irq:        IRQ number.
@@ -915,6 +976,8 @@ static irqreturn_t snps_dfi_irq_handler(int irq, void *dev_id)
  */
 static irqreturn_t snps_com_irq_handler(int irq, void *dev_id)
 {
+	struct mem_ctl_info *mci = dev_id;
+	struct snps_edac_priv *priv = mci->pvt_info;
 	irqreturn_t rc = IRQ_NONE;
 
 	rc |= snps_ce_irq_handler(irq, dev_id);
@@ -923,6 +986,9 @@ static irqreturn_t snps_com_irq_handler(int irq, void *dev_id)
 
 	rc |= snps_dfi_irq_handler(irq, dev_id);
 
+	if (priv->info.caps & SNPS_CAP_ECC_SCRUBBER)
+		rc |= snps_sbr_irq_handler(irq, dev_id);
+
 	return rc;
 }
 
@@ -972,6 +1038,205 @@ static void snps_disable_irq(struct snps_edac_priv *priv)
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
+/**
+ * snps_get_sdram_bw - Get SDRAM bandwidth.
+ * @priv:	DDR memory controller private instance data.
+ *
+ * The SDRAM interface bandwidth is calculated based on the DDRC Core clock rate
+ * and the DW uMCTL2 IP-core parameters like DQ-bus width and mode and
+ * Core/SDRAM clocks frequency ratio. Note it returns the theoretical bandwidth
+ * which in reality is hardly possible to reach.
+ *
+ * Return: SDRAM bandwidth or zero if no Core clock specified.
+ */
+static u64 snps_get_sdram_bw(struct snps_edac_priv *priv)
+{
+	unsigned long rate;
+
+	/*
+	 * Depending on the ratio mode the SDRAM clock either matches the Core
+	 * clock or runs with the twice its frequency.
+	 */
+	rate = clk_get_rate(priv->clks[SNPS_CORE_CLK].clk);
+	rate *= priv->info.freq_ratio;
+
+	/*
+	 * Scale up by 2 since it's DDR (Double Data Rate) and subtract the
+	 * DQ-mode since in non-Full mode only a part of the DQ-bus is utilised
+	 * on each SDRAM clock edge.
+	 */
+	return (2U << (priv->info.dq_width - priv->info.dq_mode)) * (u64)rate;
+}
+
+/**
+ * snps_get_scrub_bw - Get Scrubber bandwidth.
+ * @priv:	DDR memory controller private instance data.
+ * @interval:	Scrub interval.
+ *
+ * DW uMCTL2 DDRC Scrubber performs periodical progressive burst reads (RMW if
+ * ECC CE is detected) commands from the whole memory space. The read commands
+ * can be delayed by means of the SBRCTL.scrub_interval field. The Scrubber
+ * cycles look as follows:
+ *
+ * |---HIF-burst-read---|-------delay-------|-HIF-burst-read-| etc
+ *
+ * Tb = Bl*[DQ]/Bw[RAM] Td = 512*interval/Fc - periods of the stages, where
+ * Bl - HIF burst length, [DQ] - Full DQ-bus width, Bw[RAM] - SDRAM bandwidth,
+ * Fc - Core clock frequency (Scrubber and Core clocks are synchronous).
+ *
+ * After some simple calculations the expressions above can be used to get the
+ * next Scrubber bandwidth formulae:
+ *
+ * Bw[Sbr] = Bw[RAM] / (1 + F * interval), where
+ * F = 2 * 512 * Fr * Fc * [DQ]e - interval scale factor with
+ * Fr - HIF/SDRAM clock frequency ratio (1 or 2), [DQ]e - DQ-bus width mode.
+ *
+ * Return: Scrubber bandwidth or zero if no Core clock specified.
+ */
+static u64 snps_get_scrub_bw(struct snps_edac_priv *priv, u32 interval)
+{
+	unsigned long fac;
+	u64 bw_ram;
+
+	fac = (2 * ECC_SBRCTL_INTERVAL_STEP * priv->info.freq_ratio) /
+	      (priv->info.hif_burst_len * (1UL << priv->info.dq_mode));
+
+	bw_ram = snps_get_sdram_bw(priv);
+
+	do_div(bw_ram, 1 + fac * interval);
+
+	return bw_ram;
+}
+
+/**
+ * snps_get_scrub_interval - Get Scrubber delay interval.
+ * @priv:	DDR memory controller private instance data.
+ * @bw:		Scrubber bandwidth.
+ *
+ * Similarly to the Scrubber bandwidth the interval formulae can be inferred
+ * from the same expressions:
+ *
+ * interval = (Bw[RAM] - Bw[Sbr]) / (F * Bw[Sbr])
+ *
+ * Return: Scrubber delay interval or zero if no Core clock specified.
+ */
+static u32 snps_get_scrub_interval(struct snps_edac_priv *priv, u32 bw)
+{
+	unsigned long fac;
+	u64 bw_ram;
+
+	fac = (2 * priv->info.freq_ratio * ECC_SBRCTL_INTERVAL_STEP) /
+	      (priv->info.hif_burst_len * (1UL << priv->info.dq_mode));
+
+	bw_ram = snps_get_sdram_bw(priv);
+
+	/* Divide twice so not to cause the integer overflow in (fac * bw) */
+	bw_ram -= bw;
+	do_div(bw_ram, bw);
+	do_div(bw_ram, fac);
+
+	return bw_ram;
+}
+
+/**
+ * snps_set_sdram_scrub_rate - Set the Scrubber bandwidth.
+ * @mci:	EDAC memory controller instance.
+ * @bw:		Bandwidth.
+ *
+ * It calculates the delay between the Scrubber read commands based on the
+ * specified bandwidth and the Core clock rate. If the Core clock is unavailable
+ * the passed bandwidth will be directly used as the interval value.
+ *
+ * Note the method warns about the back-to-back scrubbing since it may
+ * significantly degrade the system performance. This mode is supposed to be
+ * used for a single SDRAM scrubbing pass only. So it will be turned off in the
+ * Scrubber Done IRQ handler.
+ *
+ * Return: Actually set bandwidth (interval-based approximated bandwidth if the
+ * Core clock is unavailable) or zero if the Scrubber was disabled.
+ */
+static int snps_set_sdram_scrub_rate(struct mem_ctl_info *mci, u32 bw)
+{
+	struct snps_edac_priv *priv = mci->pvt_info;
+	u32 regval, interval;
+	unsigned long flags;
+	u64 bw_min, bw_max;
+
+	/* Don't bother with the calculations just disable and return. */
+	if (!bw) {
+		spin_lock_irqsave(&priv->lock, flags);
+
+		regval = readl(priv->baseaddr + ECC_SBRCTL_OFST);
+		regval &= ~ECC_SBRCTL_SCRUB_EN;
+		writel(regval, priv->baseaddr + ECC_SBRCTL_OFST);
+
+		spin_unlock_irqrestore(&priv->lock, flags);
+
+		return 0;
+	}
+
+	/* If no Core clock specified fallback to the direct interval setup. */
+	bw_max = snps_get_scrub_bw(priv, ECC_SBRCTL_INTERVAL_MIN);
+	if (bw_max) {
+		bw_min = snps_get_scrub_bw(priv, ECC_SBRCTL_INTERVAL_MAX);
+		bw = clamp_t(u64, bw, bw_min, bw_max);
+
+		interval = snps_get_scrub_interval(priv, bw);
+	} else {
+		bw = clamp_val(bw, ECC_SBRCTL_INTERVAL_MIN, ECC_SBRCTL_INTERVAL_MAX);
+
+		interval = ECC_SBRCTL_INTERVAL_MAX - bw;
+	}
+
+	/*
+	 * SBRCTL.scrub_en bitfield must be accessed separately from the other
+	 * CSR bitfields. It means the flag must be set/clear with no updates
+	 * to the rest of the fields.
+	 */
+	spin_lock_irqsave(&priv->lock, flags);
+
+	regval = FIELD_PREP(ECC_SBRCTL_SCRUB_INTERVAL, interval);
+	writel(regval, priv->baseaddr + ECC_SBRCTL_OFST);
+
+	writel(regval | ECC_SBRCTL_SCRUB_EN, priv->baseaddr + ECC_SBRCTL_OFST);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	if (!interval)
+		edac_mc_printk(mci, KERN_WARNING, "Back-to-back scrubbing enabled\n");
+
+	if (!bw_max)
+		return interval ? bw : INT_MAX;
+
+	return snps_get_scrub_bw(priv, interval);
+}
+
+/**
+ * snps_get_sdram_scrub_rate - Get the Scrubber bandwidth.
+ * @mci:	EDAC memory controller instance.
+ *
+ * Return: Scrubber bandwidth (interval-based approximated bandwidth if the
+ * Core clock is unavailable) or zero if the Scrubber was disabled.
+ */
+static int snps_get_sdram_scrub_rate(struct mem_ctl_info *mci)
+{
+	struct snps_edac_priv *priv = mci->pvt_info;
+	u32 regval;
+	u64 bw;
+
+	regval = readl(priv->baseaddr + ECC_SBRCTL_OFST);
+	if (!(regval & ECC_SBRCTL_SCRUB_EN))
+		return 0;
+
+	regval = FIELD_GET(ECC_SBRCTL_SCRUB_INTERVAL, regval);
+
+	bw = snps_get_scrub_bw(priv, regval);
+	if (!bw)
+		return regval ? ECC_SBRCTL_INTERVAL_MAX - regval : INT_MAX;
+
+	return bw;
+}
+
 /**
  * snps_create_data - Create private data.
  * @pdev:	platform device.
@@ -1038,7 +1303,18 @@ static int snps_get_res(struct snps_edac_priv *priv)
 		return rc;
 	}
 
+	rc = clk_prepare_enable(priv->clks[SNPS_SBR_CLK].clk);
+	if (rc) {
+		edac_printk(KERN_INFO, EDAC_MC, "Couldn't enable Scrubber clock\n");
+		goto err_disable_pclk;
+	}
+
 	return 0;
+
+err_disable_pclk:
+	clk_disable_unprepare(priv->clks[SNPS_CSR_CLK].clk);
+
+	return rc;
 }
 
 /**
@@ -1047,6 +1323,8 @@ static int snps_get_res(struct snps_edac_priv *priv)
  */
 static void snps_put_res(struct snps_edac_priv *priv)
 {
+	clk_disable_unprepare(priv->clks[SNPS_SBR_CLK].clk);
+
 	clk_disable_unprepare(priv->clks[SNPS_CSR_CLK].clk);
 }
 
@@ -1147,6 +1425,14 @@ static int snps_get_ddrc_info(struct snps_edac_priv *priv)
 	if (!(regval & ECC_CFG0_DIS_SCRUB))
 		priv->info.caps |= SNPS_CAP_ECC_SCRUB;
 
+	/* Auto-detect the scrubber by writing to the SBRWDATA0 CSR */
+	regval = readl(priv->baseaddr + ECC_SBRWDATA0_OFST);
+	writel(~regval, priv->baseaddr + ECC_SBRWDATA0_OFST);
+	if (regval != readl(priv->baseaddr + ECC_SBRWDATA0_OFST)) {
+		priv->info.caps |= SNPS_CAP_ECC_SCRUBBER;
+		writel(regval, priv->baseaddr + ECC_SBRWDATA0_OFST);
+	}
+
 	/* Auto-detect the basic HIF/SDRAM bus parameters */
 	regval = readl(priv->baseaddr + DDR_MSTR_OFST);
 
@@ -1633,6 +1919,12 @@ static struct mem_ctl_info *snps_mc_create(struct snps_edac_priv *priv)
 		mci->scrub_cap = SCRUB_FLAG_SW_SRC;
 	}
 
+	if (priv->info.caps & SNPS_CAP_ECC_SCRUBBER) {
+		mci->scrub_cap |= SCRUB_FLAG_HW_PROG | SCRUB_FLAG_HW_TUN;
+		mci->set_sdram_scrub_rate = snps_set_sdram_scrub_rate;
+		mci->get_sdram_scrub_rate = snps_get_sdram_scrub_rate;
+	}
+
 	mci->ctl_name = "snps_umctl2_ddrc";
 	mci->dev_name = SNPS_EDAC_MOD_STRING;
 	mci->mod_name = SNPS_EDAC_MOD_VER;
@@ -1707,6 +1999,16 @@ static int snps_request_ind_irq(struct mem_ctl_info *mci)
 		}
 	}
 
+	irq = platform_get_irq_byname_optional(priv->pdev, "ecc_sbr");
+	if (irq > 0) {
+		rc = devm_request_irq(dev, irq, snps_sbr_irq_handler, 0, "ecc_sbr", mci);
+		if (rc) {
+			edac_printk(KERN_ERR, EDAC_MC, "Failed to request Sbr IRQ\n");
+			return rc;
+		}
+	}
+
+
 	return 0;
 }
 
@@ -1813,6 +2115,8 @@ static int snps_ddrc_info_show(struct seq_file *s, void *data)
 	if (priv->info.caps) {
 		if (priv->info.caps & SNPS_CAP_ECC_SCRUB)
 			seq_puts(s, " +Scrub");
+		if (priv->info.caps & SNPS_CAP_ECC_SCRUBBER)
+			seq_puts(s, " +Scrubber");
 		if (priv->info.caps & SNPS_CAP_ZYNQMP)
 			seq_puts(s, " +ZynqMP");
 	} else {
-- 
2.35.1

[PATCH 12/13] EDAC/synopsys: Drop vendor-specific arch dependency

[Serge Semin]

DW uMCTL2 DDRC EDAC driver is no longer specific to particular DDRC
versions. It's generic in the most of the aspects now. So set its kernel
config independently from the ZynqMP/IntelFPAG/MXC platforms.

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

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 98bcdadf4143..6aa59a0bacf1 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -486,7 +486,6 @@ config EDAC_ARMADA_XP
 
 config EDAC_SYNOPSYS
 	tristate "Synopsys DDR Memory Controller"
-	depends on ARCH_ZYNQMP || ARCH_INTEL_SOCFPGA || ARCH_MXC
 	help
 	  Support for error detection and correction on the Synopsys DDR
 	  memory controller.
-- 
2.35.1

[PATCH 13/13] EDAC/synopsys: Add Baikal-T1 DDRC support

[Serge Semin]

Baikal-T1 SoC is equipped with the DW uMCTl2 DDRC of v2.61a with 32-bit
DQ-bus accepting DDR2/DDR3 SDRAMs of up to 2 ranks, 1:2 HIF/SDRAM clocks
rate ratio, HIF interface burst length of 8 Full DQ-bus words, 40-bit
System/Application address width and 128-bits data width, 3 System address
regions with block size 256MB. There is SEC/DED ECC capability with Scrub
(RMW) and Scrubber features.

Since the Baikal-T1 DDR controller is capable of the ECC let's add it to
the DW uMCTL2 DDRC EDAC driver. The most of the parameters above will be
autodetected except HIF burst length and SAR block size, which will be set
by means of the Baikal-T1-specific initialization method. The controller
compatible string "baikal,bt1-ddrc" will be used to attach the driver to
the kernel device. It's chosen in accordance with the just updated
DT-bindings.

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

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index f86d1be2702a..9780f61ac84c 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -1342,6 +1342,20 @@ static int zynqmp_init_plat(struct snps_edac_priv *priv)
 	return 0;
 }
 
+/*
+ * bt1_init_plat - Baikal-T1-specific platform initialization.
+ * @priv:	DDR memory controller private data.
+ *
+ * Return: always zero.
+ */
+static int bt1_init_plat(struct snps_edac_priv *priv)
+{
+	priv->info.hif_burst_len = SNPS_DDR_BL8;
+	priv->sys_app_map.minsize = DDR_MIN_SARSIZE;
+
+	return 0;
+}
+
 /**
  * snps_get_dtype - Return the controller memory width.
  * @mstr:	Master CSR value.
@@ -2470,6 +2484,7 @@ static int snps_mc_remove(struct platform_device *pdev)
 
 static const struct of_device_id snps_edac_match[] = {
 	{ .compatible = "xlnx,zynqmp-ddrc-2.40a", .data = zynqmp_init_plat },
+	{ .compatible = "baikal,bt1-ddrc", .data = bt1_init_plat },
 	{ .compatible = "snps,ddrc-3.80a" },
 	{ }
 };
-- 
2.35.1

Re: [PATCH 01/13] dt-bindings: memory: snps: Extend schema with IRQs/resets/clocks props

[Krzysztof Kozlowski]

On 22/08/2022 22:19, Serge Semin wrote:
> First of all the DW uMCTL2 DDRC IP-core supports the individual IRQ lines
> for each standard event: ECC Corrected Error, ECC Uncorrected Error, ECC
> Address Protection, Scrubber-Done signal, DFI Parity/CRC Error. It's
> possible that the platform engineers merge them up in the IRQ controller
> level. So let's add both configuration support to the DT-schema.
> 
> Secondly each IP-core interface is supplied with a clock source like APB
> reference clock, AXI-ports clock, main DDRC core reference clock and
> Scrubber low-power clock. In addition to that each clock domain can have a
> dedicated reset signal. Let's add the properties for at least the denoted
> clock sources and the corresponding reset controls.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  .../snps,dw-umctl2-ddrc.yaml                  | 65 +++++++++++++++++--
>  1 file changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> index 787d91d64eee..8db92210cfe1 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> @@ -13,13 +13,13 @@ maintainers:
>  
>  description: |
>    Synopsys DesignWare Enhanced uMCTL2 DDR Memory Controller is cappable of

Typo in original text: capable

> -  working with DDR devices up to (LP)DDR4 protocol. It can be equipped
> +  working with DDR devices upporting to (LP)DDR4 protocol. It can be equipped

Typo - supporting?

>    with SEC/DEC ECC feature if DRAM data bus width is either 16-bits or
>    32-bits or 64-bits wide.
>  
> -  The ZynqMP DDR controller is based on the DW uMCTL2 v2.40a controller.
> -  It has an optional SEC/DEC ECC support in 64-bit and 32-bit bus width
> -  configurations.
> +  For instance the ZynqMP DDR controller is based on the DW uMCTL2 v2.40a
> +  controller. It has an optional SEC/DEC ECC support in 64-bit and 32-bit
> +  bus width configurations.

These changes do not look related to your patch, so split them.

>  
>  properties:
>    compatible:
> @@ -28,11 +28,55 @@ properties:
>        - xlnx,zynqmp-ddrc-2.40a
>  
>    interrupts:
> -    maxItems: 1
> +    description:
> +      DW uMCTL2 DDRC IP-core provides individual IRQ signal for each event":"
> +      ECC Corrected Error, ECC Uncorrected Error, ECC Address Protection,
> +      Scrubber-Done signal, DFI Parity/CRC Error. Some platforms may have the
> +      signals merged before they reach the IRQ controller or have some of them
> +      absent in case if the corresponding feature is unavailable/disabled.
> +    minItems: 1
> +    maxItems: 5

List has to be strictly ordered, so instead list and describe the
items... unless you are sure that any of these interrupt lines can be
merged into any other one?

> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 5
> +    oneOf:
> +      - description: Common ECC CE/UE/Scrubber/DFI Errors IRQ
> +        items:
> +          - const: ecc
> +      - description: Individual ECC CE/UE/Scrubber/DFI Errors IRQs
> +        items:
> +          enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ]
>  
>    reg:
>      maxItems: 1
>  
> +  clocks:
> +    description:
> +      A standard set of the clock sources contains CSRs bus clock, AXI-ports
> +      reference clock, DDRC core clock, Scrubber standalone clock
> +      (synchronous to the DDRC clock).
> +    minItems: 1
> +    maxItems: 4

I expect list to be strictly defined, not flexible.

> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 4
> +    items:
> +      enum: [ pclk, aclk, core, sbr ]
> +
> +  resets:
> +    description:
> +      Each clock domain can have separate reset signal.
> +    minItems: 1
> +    maxItems: 4
> +
> +  reset-names:
> +    minItems: 1
> +    maxItems: 4
> +    items:
> +      enum: [ prst, arst, core, sbr ]

The same.

> +
>  required:
>    - compatible
>    - reg
> @@ -48,4 +92,15 @@ examples:
>        interrupt-parent = <&gic>;
>        interrupts = <0 112 4>;
>      };
> +  - |
> +    memory-controller@fd070000 {
> +      compatible = "snps,ddrc-3.80a";
> +      reg = <0x3d400000 0x400000>;
> +
> +      interrupts = <0 147 4>, <0 148 4>, <0 149 4>, <0 150 4>;

Use proper defines.

> +      interrupt-names = "ecc_ce", "ecc_ue", "ecc_sbr", "dfi_e";
> +
> +      clocks = <&rcu 0>, <&rcu 5>, <&rcu 6>, <&rcu 7>;
> +      clock-names = "pclk", "aclk", "core", "sbr";
> +    };
>  ...


Best regards,
Krzysztof

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Krzysztof Kozlowski]

On 22/08/2022 22:19, Serge Semin wrote:
> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
> are individual IRQs for each ECC and DFI events.The dedicated scrubber

Missing space before "The".

> clock source is absent since it's fully synchronous to the core clock.

You need allOf:if-then restricting this per variant.

> In addition to that the DFI-DDR PHY CSRs can be accessed via a separate
> registers space.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  .../memory-controllers/snps,dw-umctl2-ddrc.yaml        | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> index 8db92210cfe1..899a6c5f9806 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> @@ -26,6 +26,7 @@ properties:
>      enum:
>        - snps,ddrc-3.80a
>        - xlnx,zynqmp-ddrc-2.40a
> +      - baikal,bt1-ddrc

Messed order. Don't add stuff at the end, but in alphabetical order.

>  
>    interrupts:
>      description:
> @@ -49,7 +50,14 @@ properties:
>            enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ]
>  
>    reg:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
> +
> +  reg-names:
> +    minItems: 1
> +    items:
> +      - const: umctl2
> +      - const: phy

You need allOf:if-then restricting this per variant.

Best regards,
Krzysztof

Re: [PATCH 01/13] dt-bindings: memory: snps: Extend schema with IRQs/resets/clocks props

[Serge Semin]

On Tue, Aug 23, 2022 at 11:11:08AM +0300, Krzysztof Kozlowski wrote:
> On 22/08/2022 22:19, Serge Semin wrote:
> > First of all the DW uMCTL2 DDRC IP-core supports the individual IRQ lines
> > for each standard event: ECC Corrected Error, ECC Uncorrected Error, ECC
> > Address Protection, Scrubber-Done signal, DFI Parity/CRC Error. It's
> > possible that the platform engineers merge them up in the IRQ controller
> > level. So let's add both configuration support to the DT-schema.
> > 
> > Secondly each IP-core interface is supplied with a clock source like APB
> > reference clock, AXI-ports clock, main DDRC core reference clock and
> > Scrubber low-power clock. In addition to that each clock domain can have a
> > dedicated reset signal. Let's add the properties for at least the denoted
> > clock sources and the corresponding reset controls.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  .../snps,dw-umctl2-ddrc.yaml                  | 65 +++++++++++++++++--
> >  1 file changed, 60 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> > index 787d91d64eee..8db92210cfe1 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> > @@ -13,13 +13,13 @@ maintainers:
> >  
> >  description: |
> >    Synopsys DesignWare Enhanced uMCTL2 DDR Memory Controller is cappable of
> 

> Typo in original text: capable
> 
> > -  working with DDR devices up to (LP)DDR4 protocol. It can be equipped
> > +  working with DDR devices upporting to (LP)DDR4 protocol. It can be equipped
> 
> Typo - supporting?
> 
> >    with SEC/DEC ECC feature if DRAM data bus width is either 16-bits or
> >    32-bits or 64-bits wide.
> >  
> > -  The ZynqMP DDR controller is based on the DW uMCTL2 v2.40a controller.
> > -  It has an optional SEC/DEC ECC support in 64-bit and 32-bit bus width
> > -  configurations.
> > +  For instance the ZynqMP DDR controller is based on the DW uMCTL2 v2.40a
> > +  controller. It has an optional SEC/DEC ECC support in 64-bit and 32-bit
> > +  bus width configurations.
> 
> These changes do not look related to your patch, so split them.

Right. Sorry for the confusing change. Indeed this update belongs to a
different patch. I'll move it to the patchset #0 to the patch with the
Zynq DT-bindings detachment.

> 
> >  
> >  properties:
> >    compatible:
> > @@ -28,11 +28,55 @@ properties:
> >        - xlnx,zynqmp-ddrc-2.40a
> >  
> >    interrupts:
> > -    maxItems: 1
> > +    description:
> > +      DW uMCTL2 DDRC IP-core provides individual IRQ signal for each event":"
> > +      ECC Corrected Error, ECC Uncorrected Error, ECC Address Protection,
> > +      Scrubber-Done signal, DFI Parity/CRC Error. Some platforms may have the
> > +      signals merged before they reach the IRQ controller or have some of them
> > +      absent in case if the corresponding feature is unavailable/disabled.
> > +    minItems: 1
> > +    maxItems: 5
> 

> List has to be strictly ordered, so instead list and describe the
> items... unless you are sure that any of these interrupt lines can be
> merged into any other one?

That's what I noted in the property description. Anyway please see the
interrupt-names property for the possible interrupts setup. To sum up
some of the IRQs might be absent or some of them merged into a single
signal.

> 
> > +
> > +  interrupt-names:
> > +    minItems: 1
> > +    maxItems: 5
> > +    oneOf:
> > +      - description: Common ECC CE/UE/Scrubber/DFI Errors IRQ
> > +        items:
> > +          - const: ecc
> > +      - description: Individual ECC CE/UE/Scrubber/DFI Errors IRQs
> > +        items:
> > +          enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ]
> >  
> >    reg:
> >      maxItems: 1
> >  
> > +  clocks:
> > +    description:
> > +      A standard set of the clock sources contains CSRs bus clock, AXI-ports
> > +      reference clock, DDRC core clock, Scrubber standalone clock
> > +      (synchronous to the DDRC clock).
> > +    minItems: 1
> > +    maxItems: 4
> 

> I expect list to be strictly defined, not flexible.

Some of the clock sources might be absent or tied up to another one
(for instance pclk, aclk and sbr can be clocked from a single core
clock source). It depends on the IP-core synthesize parameters.

> 
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 4
> > +    items:
> > +      enum: [ pclk, aclk, core, sbr ]
> > +
> > +  resets:
> > +    description:
> > +      Each clock domain can have separate reset signal.
> > +    minItems: 1
> > +    maxItems: 4
> > +
> > +  reset-names:
> > +    minItems: 1
> > +    maxItems: 4
> > +    items:
> > +      enum: [ prst, arst, core, sbr ]
> 

> The same.

The same as for the clock.

> 
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -48,4 +92,15 @@ examples:
> >        interrupt-parent = <&gic>;
> >        interrupts = <0 112 4>;
> >      };
> > +  - |
> > +    memory-controller@fd070000 {
> > +      compatible = "snps,ddrc-3.80a";
> > +      reg = <0x3d400000 0x400000>;
> > +
> > +      interrupts = <0 147 4>, <0 148 4>, <0 149 4>, <0 150 4>;
> 

> Use proper defines.

What do you mean? Which defines do you think would be proper? If you
meant the IRQ DT-bindings macros, then what difference does it make
for a generic device in the DT-binding example? Note since the device
is defined as generic it can be placed on different platforms with
different interrupt controller requirements. So what do you mean by
"proper" in this case?

-Serge

> 
> > +      interrupt-names = "ecc_ce", "ecc_ue", "ecc_sbr", "dfi_e";
> > +
> > +      clocks = <&rcu 0>, <&rcu 5>, <&rcu 6>, <&rcu 7>;
> > +      clock-names = "pclk", "aclk", "core", "sbr";
> > +    };
> >  ...
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Serge Semin]

On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote:
> On 22/08/2022 22:19, Serge Semin wrote:
> > Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
> > with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
> > are individual IRQs for each ECC and DFI events.The dedicated scrubber
> 

> Missing space before "The".

Ok. Thanks.

> 
> > clock source is absent since it's fully synchronous to the core clock.
> 

> You need allOf:if-then restricting this per variant.

I really don't like the allOf-if-if-etc pattern because it gets to be
very bulky if all the vendor-specific and generic platform
peculiarities are placed in there. I am more keen of having a
generic DT-schema which would be then allOf-ed by the vendor-specific
device bindings. What do you think I'd provide such design in this
case too?

But I'll need to move the compatible property definition to the
"select" property. Like this:

Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml:
+[...]
+# Please create a separate DT-schema for your DW uMCTL2 DDR controller
+# and make sure it's assigned with the vendor-specific compatible string.
+select:
+  properties:
+    compatible:
+      oneOf:
+        - deprecated: true
+          description: Synopsys DW uMCTL2 DDR controller v3.80a
+          const: snps,ddrc-3.80a
+        - description: Synopsys DW uMCTL2 DDR controller
+          const: snps,dw-umctl2-ddrc
+        - description: Xilinx ZynqMP DDR controller v2.40a
+          const: xlnx,zynqmp-ddrc-2.40a
+  required:
+    - compatible
+
+properties:
+  compatible: true
+[...]
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: true

After that the "snps,dw-umctl2-ddrc.yaml" schema can be referenced in the
allOf composition. Like this:

Documentation/devicetree/bindings/memory-controllers/baikal,bt1-ddrc.yaml:
+[...]
+allOf:
+  - $ref: /schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml#
+[...]

At the same time the generic DT-schema will be used to evaluate the
"snps,ddrc-3.80a", "snps,dw-umctl2-ddrc" and "xlnx,zynqmp-ddrc-2.40a"
device nodes as before. What do you think about that?

One big positive side of this that even though the generic schema
can't define the IRQ/resets/clocks phandlers order because various
platforms may have different external signals setup, the
vendor-specific schema can and should. So I'll be able to describe the
Baikal-T1 DDRC specific properties (clocks, clock-names, interrupts,
interrupt-names, etc) in much more details including the reference
signals order what you asked in the previous patch review.

-Sergey

> 
> > In addition to that the DFI-DDR PHY CSRs can be accessed via a separate
> > registers space.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  .../memory-controllers/snps,dw-umctl2-ddrc.yaml        | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> > index 8db92210cfe1..899a6c5f9806 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> > @@ -26,6 +26,7 @@ properties:
> >      enum:
> >        - snps,ddrc-3.80a
> >        - xlnx,zynqmp-ddrc-2.40a
> > +      - baikal,bt1-ddrc
>
 
> Messed order. Don't add stuff at the end, but in alphabetical order.

Ok. But could you please give me a reference with this requirement
documented? I've submitted many DT-bindings patches with the
compatible property updates and none of them received such comment
from Rob.

> 
> >  
> >    interrupts:
> >      description:
> > @@ -49,7 +50,14 @@ properties:
> >            enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ]
> >  
> >    reg:
> > -    maxItems: 1
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  reg-names:
> > +    minItems: 1
> > +    items:
> > +      - const: umctl2
> > +      - const: phy
> 

> You need allOf:if-then restricting this per variant.

Please see my comment above regarding possible solution of this.

-Sergey

> 
> Best regards,
> Krzysztof

Re: [PATCH 01/13] dt-bindings: memory: snps: Extend schema with IRQs/resets/clocks props

[Krzysztof Kozlowski]

On 26/08/2022 11:47, Serge Semin wrote:

>>
>>> +
>>> +  interrupt-names:
>>> +    minItems: 1
>>> +    maxItems: 5
>>> +    oneOf:
>>> +      - description: Common ECC CE/UE/Scrubber/DFI Errors IRQ
>>> +        items:
>>> +          - const: ecc
>>> +      - description: Individual ECC CE/UE/Scrubber/DFI Errors IRQs
>>> +        items:
>>> +          enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ]
>>>  
>>>    reg:
>>>      maxItems: 1
>>>  
>>> +  clocks:
>>> +    description:
>>> +      A standard set of the clock sources contains CSRs bus clock, AXI-ports
>>> +      reference clock, DDRC core clock, Scrubber standalone clock
>>> +      (synchronous to the DDRC clock).
>>> +    minItems: 1
>>> +    maxItems: 4
>>
> 
>> I expect list to be strictly defined, not flexible.
> 
> Some of the clock sources might be absent or tied up to another one
> (for instance pclk, aclk and sbr can be clocked from a single core
> clock source). It depends on the IP-core synthesize parameters.

Yet still your device has clock lines - clock inputs, right? Therefore
still 4 clocks will be going there or you want to say that the pin is
not connected (or pulled down)?

> 
>>
>>> +
>>> +  clock-names:
>>> +    minItems: 1
>>> +    maxItems: 4
>>> +    items:
>>> +      enum: [ pclk, aclk, core, sbr ]
>>> +
>>> +  resets:
>>> +    description:
>>> +      Each clock domain can have separate reset signal.
>>> +    minItems: 1
>>> +    maxItems: 4
>>> +
>>> +  reset-names:
>>> +    minItems: 1
>>> +    maxItems: 4
>>> +    items:
>>> +      enum: [ prst, arst, core, sbr ]
>>
> 
>> The same.
> 
> The same as for the clock.
> 
>>
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>> @@ -48,4 +92,15 @@ examples:
>>>        interrupt-parent = <&gic>;
>>>        interrupts = <0 112 4>;
>>>      };
>>> +  - |
>>> +    memory-controller@fd070000 {
>>> +      compatible = "snps,ddrc-3.80a";
>>> +      reg = <0x3d400000 0x400000>;
>>> +
>>> +      interrupts = <0 147 4>, <0 148 4>, <0 149 4>, <0 150 4>;
>>
> 
>> Use proper defines.
> 
> What do you mean? Which defines do you think would be proper? If you
> meant the IRQ DT-bindings macros, then what difference does it make
> for a generic device in the DT-binding example? 

The macros/defines representing these numbers.


> Note since the device
> is defined as generic it can be placed on different platforms with
> different interrupt controller requirements. So what do you mean by
> "proper" in this case?

Proper means text instead of hard-coded number. This piece of code has
meaning in a specific context, because you used interrupts matching some
specific interrupt controllers. In that controller context, the "4" has
a meaning. Just like "0". You cannot say that this piece of code is
interrupt-controller-independent, because it is not. 4 has meaning.

If you want it to be independent, drop all the flags... If you use flags
from a specific implementation, then use proper defines matching them,
not hard-coded numbers.

Best regards,
Krzysztof

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Rob Herring]

On Mon, Aug 22, 2022 at 10:19:45PM +0300, Serge Semin wrote:
> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
> are individual IRQs for each ECC and DFI events.The dedicated scrubber
> clock source is absent since it's fully synchronous to the core clock.
> In addition to that the DFI-DDR PHY CSRs can be accessed via a separate
> registers space.

Are you sure the phy and dfi irq shouldn't be a separate device?

Rob

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Krzysztof Kozlowski]

On 26/08/2022 11:54, Serge Semin wrote:
> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote:
>> On 22/08/2022 22:19, Serge Semin wrote:
>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber
>>
> 
>> Missing space before "The".
> 
> Ok. Thanks.
> 
>>
>>> clock source is absent since it's fully synchronous to the core clock.
>>
> 
>> You need allOf:if-then restricting this per variant.
> 
> I really don't like the allOf-if-if-etc pattern because it gets to be
> very bulky if all the vendor-specific and generic platform
> peculiarities are placed in there. I am more keen of having a
> generic DT-schema which would be then allOf-ed by the vendor-specific
> device bindings. What do you think I'd provide such design in this
> case too?

Sure, it would work.

> 
> But I'll need to move the compatible property definition to the
> "select" property. Like this:
> 
> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml:
> +[...]
> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller
> +# and make sure it's assigned with the vendor-specific compatible string.
> +select:
> +  properties:
> +    compatible:
> +      oneOf:
> +        - deprecated: true
> +          description: Synopsys DW uMCTL2 DDR controller v3.80a
> +          const: snps,ddrc-3.80a
> +        - description: Synopsys DW uMCTL2 DDR controller
> +          const: snps,dw-umctl2-ddrc
> +        - description: Xilinx ZynqMP DDR controller v2.40a
> +          const: xlnx,zynqmp-ddrc-2.40a
> +  required:
> +    - compatible

Not entirely. If you need select, then add it with compatibles, but all
descriptions and deprecated are staying in properties.


> +
> +properties:
> +  compatible: true
> +[...]
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: true
> 
> After that the "snps,dw-umctl2-ddrc.yaml" schema can be referenced in the
> allOf composition. Like this:
> 
> Documentation/devicetree/bindings/memory-controllers/baikal,bt1-ddrc.yaml:
> +[...]
> +allOf:
> +  - $ref: /schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml#
> +[...]
> 
> At the same time the generic DT-schema will be used to evaluate the
> "snps,ddrc-3.80a", "snps,dw-umctl2-ddrc" and "xlnx,zynqmp-ddrc-2.40a"
> device nodes as before. What do you think about that?
> 
> One big positive side of this that even though the generic schema
> can't define the IRQ/resets/clocks phandlers order because various
> platforms may have different external signals setup, the
> vendor-specific schema can and should. So I'll be able to describe the
> Baikal-T1 DDRC specific properties (clocks, clock-names, interrupts,
> interrupt-names, etc) in much more details including the reference
> signals order what you asked in the previous patch review.

It's ok. You need then second schema for your device, because something
must end with additional/unevaluatedProperties false.

Best regards,
Krzysztof

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Serge Semin]

On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote:
> On 26/08/2022 11:54, Serge Semin wrote:
> > On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote:
> >> On 22/08/2022 22:19, Serge Semin wrote:
> >>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
> >>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
> >>> are individual IRQs for each ECC and DFI events.The dedicated scrubber
> >>
> > 
> >> Missing space before "The".
> > 
> > Ok. Thanks.
> > 
> >>
> >>> clock source is absent since it's fully synchronous to the core clock.
> >>
> > 
> >> You need allOf:if-then restricting this per variant.
> > 
> > I really don't like the allOf-if-if-etc pattern because it gets to be
> > very bulky if all the vendor-specific and generic platform
> > peculiarities are placed in there. I am more keen of having a
> > generic DT-schema which would be then allOf-ed by the vendor-specific
> > device bindings. What do you think I'd provide such design in this
> > case too?
> 
> Sure, it would work.
> 
> > 
> > But I'll need to move the compatible property definition to the
> > "select" property. Like this:
> > 
> > Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml:
> > +[...]
> > +# Please create a separate DT-schema for your DW uMCTL2 DDR controller
> > +# and make sure it's assigned with the vendor-specific compatible string.
> > +select:
> > +  properties:
> > +    compatible:
> > +      oneOf:
> > +        - deprecated: true
> > +          description: Synopsys DW uMCTL2 DDR controller v3.80a
> > +          const: snps,ddrc-3.80a
> > +        - description: Synopsys DW uMCTL2 DDR controller
> > +          const: snps,dw-umctl2-ddrc
> > +        - description: Xilinx ZynqMP DDR controller v2.40a
> > +          const: xlnx,zynqmp-ddrc-2.40a
> > +  required:
> > +    - compatible
> 

> Not entirely. If you need select, then add it with compatibles, but all
> descriptions and deprecated are staying in properties.

Ok. But note in such case the compatible string constraints will get
to be opened for any non-common string. Like this:

+ properties:
+   compatible:
+     oneOf:
+       - const: snps,ddrc-3.80a
+       - {}

It's required for the DT-schemas referencing the common one, otherwise
they will fail DT-nodes evaluation due to the "compatible" property
missing the vendor-specific string.

> 
> 
> > +
> > +properties:
> > +  compatible: true
> > +[...]
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +additionalProperties: true
> > 
> > After that the "snps,dw-umctl2-ddrc.yaml" schema can be referenced in the
> > allOf composition. Like this:
> > 
> > Documentation/devicetree/bindings/memory-controllers/baikal,bt1-ddrc.yaml:
> > +[...]
> > +allOf:
> > +  - $ref: /schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml#
> > +[...]
> > 
> > At the same time the generic DT-schema will be used to evaluate the
> > "snps,ddrc-3.80a", "snps,dw-umctl2-ddrc" and "xlnx,zynqmp-ddrc-2.40a"
> > device nodes as before. What do you think about that?
> > 
> > One big positive side of this that even though the generic schema
> > can't define the IRQ/resets/clocks phandlers order because various
> > platforms may have different external signals setup, the
> > vendor-specific schema can and should. So I'll be able to describe the
> > Baikal-T1 DDRC specific properties (clocks, clock-names, interrupts,
> > interrupt-names, etc) in much more details including the reference
> > signals order what you asked in the previous patch review.
> 

> It's ok. You need then second schema for your device, because something
> must end with additional/unevaluatedProperties false.

Right. I'll add a separate DT-schema for my device.

-Sergey

> 
> Best regards,
> Krzysztof

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Krzysztof Kozlowski]

On 08/09/2022 11:46, Serge Semin wrote:
> On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote:
>> On 26/08/2022 11:54, Serge Semin wrote:
>>> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote:
>>>> On 22/08/2022 22:19, Serge Semin wrote:
>>>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
>>>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
>>>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber
>>>>
>>>
>>>> Missing space before "The".
>>>
>>> Ok. Thanks.
>>>
>>>>
>>>>> clock source is absent since it's fully synchronous to the core clock.
>>>>
>>>
>>>> You need allOf:if-then restricting this per variant.
>>>
>>> I really don't like the allOf-if-if-etc pattern because it gets to be
>>> very bulky if all the vendor-specific and generic platform
>>> peculiarities are placed in there. I am more keen of having a
>>> generic DT-schema which would be then allOf-ed by the vendor-specific
>>> device bindings. What do you think I'd provide such design in this
>>> case too?
>>
>> Sure, it would work.
>>
>>>
>>> But I'll need to move the compatible property definition to the
>>> "select" property. Like this:
>>>
>>> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml:
>>> +[...]
>>> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller
>>> +# and make sure it's assigned with the vendor-specific compatible string.
>>> +select:
>>> +  properties:
>>> +    compatible:
>>> +      oneOf:
>>> +        - deprecated: true
>>> +          description: Synopsys DW uMCTL2 DDR controller v3.80a
>>> +          const: snps,ddrc-3.80a
>>> +        - description: Synopsys DW uMCTL2 DDR controller
>>> +          const: snps,dw-umctl2-ddrc
>>> +        - description: Xilinx ZynqMP DDR controller v2.40a
>>> +          const: xlnx,zynqmp-ddrc-2.40a
>>> +  required:
>>> +    - compatible
>>
> 
>> Not entirely. If you need select, then add it with compatibles, but all
>> descriptions and deprecated are staying in properties.
> 
> Ok. But note in such case the compatible string constraints will get
> to be opened for any non-common string. Like this:
> 
> + properties:
> +   compatible:
> +     oneOf:
> +       - const: snps,ddrc-3.80a
> +       - {}

Not really. If you define here specific device compatibles in select,
they must be here as well.

> 
> It's required for the DT-schemas referencing the common one, otherwise
> they will fail DT-nodes evaluation due to the "compatible" property
> missing the vendor-specific string.

o you probably mix here purposes. Either you define common schema or
device specific one. If you define common, usually it does not enforce
any compatibles. You do not need select, no need for compatibles either,
although you can add above syntax if it is valid. If you write here
specific device bindings, then compatibles should be listed. Judging
from what you wrote it's neither this nor that...

Best regards,
Krzysztof

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Serge Semin]

On Thu, Sep 08, 2022 at 11:58:50AM +0200, Krzysztof Kozlowski wrote:
> On 08/09/2022 11:46, Serge Semin wrote:
> > On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote:
> >> On 26/08/2022 11:54, Serge Semin wrote:
> >>> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote:
> >>>> On 22/08/2022 22:19, Serge Semin wrote:
> >>>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
> >>>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
> >>>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber
> >>>>
> >>>
> >>>> Missing space before "The".
> >>>
> >>> Ok. Thanks.
> >>>
> >>>>
> >>>>> clock source is absent since it's fully synchronous to the core clock.
> >>>>
> >>>
> >>>> You need allOf:if-then restricting this per variant.
> >>>
> >>> I really don't like the allOf-if-if-etc pattern because it gets to be
> >>> very bulky if all the vendor-specific and generic platform
> >>> peculiarities are placed in there. I am more keen of having a
> >>> generic DT-schema which would be then allOf-ed by the vendor-specific
> >>> device bindings. What do you think I'd provide such design in this
> >>> case too?
> >>
> >> Sure, it would work.
> >>
> >>>
> >>> But I'll need to move the compatible property definition to the
> >>> "select" property. Like this:
> >>>
> >>> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml:
> >>> +[...]
> >>> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller
> >>> +# and make sure it's assigned with the vendor-specific compatible string.
> >>> +select:
> >>> +  properties:
> >>> +    compatible:
> >>> +      oneOf:
> >>> +        - deprecated: true
> >>> +          description: Synopsys DW uMCTL2 DDR controller v3.80a
> >>> +          const: snps,ddrc-3.80a
> >>> +        - description: Synopsys DW uMCTL2 DDR controller
> >>> +          const: snps,dw-umctl2-ddrc
> >>> +        - description: Xilinx ZynqMP DDR controller v2.40a
> >>> +          const: xlnx,zynqmp-ddrc-2.40a
> >>> +  required:
> >>> +    - compatible
> >>
> > 
> >> Not entirely. If you need select, then add it with compatibles, but all
> >> descriptions and deprecated are staying in properties.
> > 
> > Ok. But note in such case the compatible string constraints will get
> > to be opened for any non-common string. Like this:
> > 
> > + properties:
> > +   compatible:
> > +     oneOf:
> > +       - const: snps,ddrc-3.80a
> > +       - {}
> 
> Not really. If you define here specific device compatibles in select,
> they must be here as well.
> 
> > 
> > It's required for the DT-schemas referencing the common one, otherwise
> > they will fail DT-nodes evaluation due to the "compatible" property
> > missing the vendor-specific string.
> 

> o you probably mix here purposes. Either you define common schema or
> device specific one. If you define common, usually it does not enforce
> any compatibles. You do not need select, no need for compatibles either,
> although you can add above syntax if it is valid. If you write here
> specific device bindings, then compatibles should be listed. Judging
> from what you wrote it's neither this nor that...

My idea was to provide both the common DT-schema and the
vendor-specific ones. But the later one would refer to the common
schema in the framework of the "allOf:" composition. Like this:

snps,dw-umctl2-ddrc.yaml:
+ [...]
+ select:
+   properties:
+     compatible:
+       enum:
+         - snps,ddrc-3.80a
+ [...]
+ properties:
+   compatible:
+     oneOf:
+       - const: snps,ddrc-3.80a
+       - {}
+   interrupts:
+   [...]
+   interrupt-names:
+   [...]
+ additionalProperties: false
+ [...]

baikal,bt1-ddrc.yaml:
+ [...]
+ allOf:
+   - "schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml:#"
+ [...]
+ unevaluateProperties: false
+ [...]

Thus the common schema as before would provide the widest set of the
properties and their constraints, while the vendor-specific one would
be !obligated! to follow the common schema conventions, but provide a
more specific set of the properties and constraints. A similar
approach is implemented for instance in the DW USB3 DT-schemas, but
with the generic compatible string fallback. In this case we don't
need the fallback string, but in order for the common schema being
applicable for both the common and vendor-specific DT-nodes the
compatible property constraints need to be designed as is provided in
the example above.

Alternatively we can split the snps,dw-umctl2-ddrc.yaml schema up into
the two ones:
snps,dw-umctl2-ddrc-common.yaml
and
snps,dw-umctl2-ddrc.yaml
So the first schema would contain all the common properties definition
and would be only used for being referenced in the particular device
DT-bindings (select: false). The snps,dw-umctl2-ddrc.yaml and
vendor-specific DT-schemas would just have it "allOf:"ed.

Personally I'd prefer the design pattern with the always-true
compatible property constraint as in the example above since it seems
easier to maintain than having the common and generic device
DT-schemas.

Note having a common DT-schema and a vendor-specific one referencing
the common schema is very much useful practice for the devices based
on the same IP-core. Vendor-device driver authors tend to create their
own bindings for the same clocks/resets/phys/etc thus making the
drivers much harder to maintain (for a bright example see what has
been done for the DW PCIe RP/EP IP-core). The worst part of it is that
ones the DT-bindings interface is implemented it can't be changed
since the kernel needs to be backward compatible with the DT-sources
compiled before. So the main goal of the common DT-schema is to fix
the common DT interface and make sure the new vendor-specific device
drivers would at least consider either to follow the IP-core DT
convention or to widen it up if required.

-Sergey

> 
> Best regards,
> Krzysztof

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Krzysztof Kozlowski]

On 08/09/2022 17:08, Serge Semin wrote:
> On Thu, Sep 08, 2022 at 11:58:50AM +0200, Krzysztof Kozlowski wrote:
>> On 08/09/2022 11:46, Serge Semin wrote:
>>> On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote:
>>>> On 26/08/2022 11:54, Serge Semin wrote:
>>>>> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote:
>>>>>> On 22/08/2022 22:19, Serge Semin wrote:
>>>>>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
>>>>>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
>>>>>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber
>>>>>>
>>>>>
>>>>>> Missing space before "The".
>>>>>
>>>>> Ok. Thanks.
>>>>>
>>>>>>
>>>>>>> clock source is absent since it's fully synchronous to the core clock.
>>>>>>
>>>>>
>>>>>> You need allOf:if-then restricting this per variant.
>>>>>
>>>>> I really don't like the allOf-if-if-etc pattern because it gets to be
>>>>> very bulky if all the vendor-specific and generic platform
>>>>> peculiarities are placed in there. I am more keen of having a
>>>>> generic DT-schema which would be then allOf-ed by the vendor-specific
>>>>> device bindings. What do you think I'd provide such design in this
>>>>> case too?
>>>>
>>>> Sure, it would work.
>>>>
>>>>>
>>>>> But I'll need to move the compatible property definition to the
>>>>> "select" property. Like this:
>>>>>
>>>>> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml:
>>>>> +[...]
>>>>> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller
>>>>> +# and make sure it's assigned with the vendor-specific compatible string.
>>>>> +select:
>>>>> +  properties:
>>>>> +    compatible:
>>>>> +      oneOf:
>>>>> +        - deprecated: true
>>>>> +          description: Synopsys DW uMCTL2 DDR controller v3.80a
>>>>> +          const: snps,ddrc-3.80a
>>>>> +        - description: Synopsys DW uMCTL2 DDR controller
>>>>> +          const: snps,dw-umctl2-ddrc
>>>>> +        - description: Xilinx ZynqMP DDR controller v2.40a
>>>>> +          const: xlnx,zynqmp-ddrc-2.40a
>>>>> +  required:
>>>>> +    - compatible
>>>>
>>>
>>>> Not entirely. If you need select, then add it with compatibles, but all
>>>> descriptions and deprecated are staying in properties.
>>>
>>> Ok. But note in such case the compatible string constraints will get
>>> to be opened for any non-common string. Like this:
>>>
>>> + properties:
>>> +   compatible:
>>> +     oneOf:
>>> +       - const: snps,ddrc-3.80a
>>> +       - {}
>>
>> Not really. If you define here specific device compatibles in select,
>> they must be here as well.
>>
>>>
>>> It's required for the DT-schemas referencing the common one, otherwise
>>> they will fail DT-nodes evaluation due to the "compatible" property
>>> missing the vendor-specific string.
>>
> 
>> o you probably mix here purposes. Either you define common schema or
>> device specific one. If you define common, usually it does not enforce
>> any compatibles. You do not need select, no need for compatibles either,
>> although you can add above syntax if it is valid. If you write here
>> specific device bindings, then compatibles should be listed. Judging
>> from what you wrote it's neither this nor that...
> 
> My idea was to provide both the common DT-schema and the
> vendor-specific ones. But the later one would refer to the common
> schema in the framework of the "allOf:" composition. Like this:
> 
> snps,dw-umctl2-ddrc.yaml:
> + [...]
> + select:
> +   properties:
> +     compatible:
> +       enum:
> +         - snps,ddrc-3.80a
> + [...]
> + properties:
> +   compatible:
> +     oneOf:
> +       - const: snps,ddrc-3.80a
> +       - {}

This is not the approach snps,dwc3.yaml is doing. It is closer to
snps,dw-pcie.yaml, but that one ends with additionalProperties:true so
it is expected to be constrained by something else.

You can choose either way, but what you wrote before looked different -
basically loosing the compatibles documentation.

> +   interrupts:
> +   [...]
> +   interrupt-names:
> +   [...]
> + additionalProperties: false
> + [...]
> 
> baikal,bt1-ddrc.yaml:
> + [...]
> + allOf:
> +   - "schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml:#"
> + [...]
> + unevaluateProperties: false
> + [...]
> 
> Thus the common schema as before would provide the widest set of the
> properties and their constraints, while the vendor-specific one would
> be !obligated! to follow the common schema conventions, but provide a
> more specific set of the properties and constraints. A similar
> approach is implemented for instance in the DW USB3 DT-schemas, but
> with the generic compatible string fallback. In this case we don't
> need the fallback string, but in order for the common schema being
> applicable for both the common and vendor-specific DT-nodes the
> compatible property constraints need to be designed as is provided in
> the example above.

Anyway, it's getting complicated so I am not sure about which option now
we discuss. You cannot have deprecated entries in select and compatibles
described there, without describing them in properties.

> 
> Alternatively we can split the snps,dw-umctl2-ddrc.yaml schema up into
> the two ones:
> snps,dw-umctl2-ddrc-common.yaml
> and
> snps,dw-umctl2-ddrc.yaml
> So the first schema would contain all the common properties definition
> and would be only used for being referenced in the particular device
> DT-bindings (select: false). The snps,dw-umctl2-ddrc.yaml and
> vendor-specific DT-schemas would just have it "allOf:"ed.
> 
> Personally I'd prefer the design pattern with the always-true
> compatible property constraint as in the example above since it seems
> easier to maintain than having the common and generic device
> DT-schemas.
> 
> Note having a common DT-schema and a vendor-specific one referencing
> the common schema is very much useful practice for the devices based
> on the same IP-core. Vendor-device driver authors tend to create their
> own bindings for the same clocks/resets/phys/etc thus making the
> drivers much harder to maintain (for a bright example see what has
> been done for the DW PCIe RP/EP IP-core). The worst part of it is that
> ones the DT-bindings interface is implemented it can't be changed
> since the kernel needs to be backward compatible with the DT-sources
> compiled before. So the main goal of the common DT-schema is to fix
> the common DT interface and make sure the new vendor-specific device
> drivers would at least consider either to follow the IP-core DT
> convention or to widen it up if required.


Best regards,
Krzysztof

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Serge Semin]

On Thu, Sep 08, 2022 at 05:27:30PM +0200, Krzysztof Kozlowski wrote:
> On 08/09/2022 17:08, Serge Semin wrote:
> > On Thu, Sep 08, 2022 at 11:58:50AM +0200, Krzysztof Kozlowski wrote:
> >> On 08/09/2022 11:46, Serge Semin wrote:
> >>> On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 26/08/2022 11:54, Serge Semin wrote:
> >>>>> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote:
> >>>>>> On 22/08/2022 22:19, Serge Semin wrote:
> >>>>>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
> >>>>>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
> >>>>>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber
> >>>>>>
> >>>>>
> >>>>>> Missing space before "The".
> >>>>>
> >>>>> Ok. Thanks.
> >>>>>
> >>>>>>
> >>>>>>> clock source is absent since it's fully synchronous to the core clock.
> >>>>>>
> >>>>>
> >>>>>> You need allOf:if-then restricting this per variant.
> >>>>>
> >>>>> I really don't like the allOf-if-if-etc pattern because it gets to be
> >>>>> very bulky if all the vendor-specific and generic platform
> >>>>> peculiarities are placed in there. I am more keen of having a
> >>>>> generic DT-schema which would be then allOf-ed by the vendor-specific
> >>>>> device bindings. What do you think I'd provide such design in this
> >>>>> case too?
> >>>>
> >>>> Sure, it would work.
> >>>>
> >>>>>
> >>>>> But I'll need to move the compatible property definition to the
> >>>>> "select" property. Like this:
> >>>>>
> >>>>> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml:
> >>>>> +[...]
> >>>>> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller
> >>>>> +# and make sure it's assigned with the vendor-specific compatible string.
> >>>>> +select:
> >>>>> +  properties:
> >>>>> +    compatible:
> >>>>> +      oneOf:
> >>>>> +        - deprecated: true
> >>>>> +          description: Synopsys DW uMCTL2 DDR controller v3.80a
> >>>>> +          const: snps,ddrc-3.80a
> >>>>> +        - description: Synopsys DW uMCTL2 DDR controller
> >>>>> +          const: snps,dw-umctl2-ddrc
> >>>>> +        - description: Xilinx ZynqMP DDR controller v2.40a
> >>>>> +          const: xlnx,zynqmp-ddrc-2.40a
> >>>>> +  required:
> >>>>> +    - compatible
> >>>>
> >>>
> >>>> Not entirely. If you need select, then add it with compatibles, but all
> >>>> descriptions and deprecated are staying in properties.
> >>>
> >>> Ok. But note in such case the compatible string constraints will get
> >>> to be opened for any non-common string. Like this:
> >>>
> >>> + properties:
> >>> +   compatible:
> >>> +     oneOf:
> >>> +       - const: snps,ddrc-3.80a
> >>> +       - {}
> >>
> >> Not really. If you define here specific device compatibles in select,
> >> they must be here as well.
> >>
> >>>
> >>> It's required for the DT-schemas referencing the common one, otherwise
> >>> they will fail DT-nodes evaluation due to the "compatible" property
> >>> missing the vendor-specific string.
> >>
> > 
> >> o you probably mix here purposes. Either you define common schema or
> >> device specific one. If you define common, usually it does not enforce
> >> any compatibles. You do not need select, no need for compatibles either,
> >> although you can add above syntax if it is valid. If you write here
> >> specific device bindings, then compatibles should be listed. Judging
> >> from what you wrote it's neither this nor that...
> > 
> > My idea was to provide both the common DT-schema and the
> > vendor-specific ones. But the later one would refer to the common
> > schema in the framework of the "allOf:" composition. Like this:
> > 
> > snps,dw-umctl2-ddrc.yaml:
> > + [...]
> > + select:
> > +   properties:
> > +     compatible:
> > +       enum:
> > +         - snps,ddrc-3.80a
> > + [...]
> > + properties:
> > +   compatible:
> > +     oneOf:
> > +       - const: snps,ddrc-3.80a
> > +       - {}
> 

> This is not the approach snps,dwc3.yaml is doing. It is closer to
> snps,dw-pcie.yaml, but that one ends with additionalProperties:true so
> it is expected to be constrained by something else.

Right. I should have used the "additionalProperties: true" instead in
the common DT-schema below.

> 
> You can choose either way, but what you wrote before looked different -
> basically loosing the compatibles documentation.

Ok. I'll make sure the compatibles documentation won't be lost.

> 
> > +   interrupts:
> > +   [...]
> > +   interrupt-names:
> > +   [...]

> > + additionalProperties: false

* here should have been "true" of course.

> > + [...]
> > 
> > baikal,bt1-ddrc.yaml:
> > + [...]
> > + allOf:
> > +   - "schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml:#"
> > + [...]
> > + unevaluateProperties: false
> > + [...]
> > 
> > Thus the common schema as before would provide the widest set of the
> > properties and their constraints, while the vendor-specific one would
> > be !obligated! to follow the common schema conventions, but provide a
> > more specific set of the properties and constraints. A similar
> > approach is implemented for instance in the DW USB3 DT-schemas, but
> > with the generic compatible string fallback. In this case we don't
> > need the fallback string, but in order for the common schema being
> > applicable for both the common and vendor-specific DT-nodes the
> > compatible property constraints need to be designed as is provided in
> > the example above.
> 

> Anyway, it's getting complicated so I am not sure about which option now
> we discuss. You cannot have deprecated entries in select and compatibles
> described there, without describing them in properties.

Ok. That's what I intended in the later approach. I'll resend the
series with the updated bindings. We can continue the discussion after
that in the new emails thread.

-Sergey

> 
> > 
> > Alternatively we can split the snps,dw-umctl2-ddrc.yaml schema up into
> > the two ones:
> > snps,dw-umctl2-ddrc-common.yaml
> > and
> > snps,dw-umctl2-ddrc.yaml
> > So the first schema would contain all the common properties definition
> > and would be only used for being referenced in the particular device
> > DT-bindings (select: false). The snps,dw-umctl2-ddrc.yaml and
> > vendor-specific DT-schemas would just have it "allOf:"ed.
> > 
> > Personally I'd prefer the design pattern with the always-true
> > compatible property constraint as in the example above since it seems
> > easier to maintain than having the common and generic device
> > DT-schemas.
> > 
> > Note having a common DT-schema and a vendor-specific one referencing
> > the common schema is very much useful practice for the devices based
> > on the same IP-core. Vendor-device driver authors tend to create their
> > own bindings for the same clocks/resets/phys/etc thus making the
> > drivers much harder to maintain (for a bright example see what has
> > been done for the DW PCIe RP/EP IP-core). The worst part of it is that
> > ones the DT-bindings interface is implemented it can't be changed
> > since the kernel needs to be backward compatible with the DT-sources
> > compiled before. So the main goal of the common DT-schema is to fix
> > the common DT interface and make sure the new vendor-specific device
> > drivers would at least consider either to follow the IP-core DT
> > convention or to widen it up if required.
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 01/13] dt-bindings: memory: snps: Extend schema with IRQs/resets/clocks props

[Serge Semin]

On Tue, Aug 30, 2022 at 06:01:56PM +0300, Krzysztof Kozlowski wrote:
> On 26/08/2022 11:47, Serge Semin wrote:
> 
> >>
> >>> +
> >>> +  interrupt-names:
> >>> +    minItems: 1
> >>> +    maxItems: 5
> >>> +    oneOf:
> >>> +      - description: Common ECC CE/UE/Scrubber/DFI Errors IRQ
> >>> +        items:
> >>> +          - const: ecc
> >>> +      - description: Individual ECC CE/UE/Scrubber/DFI Errors IRQs
> >>> +        items:
> >>> +          enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ]
> >>>  
> >>>    reg:
> >>>      maxItems: 1
> >>>  
> >>> +  clocks:
> >>> +    description:
> >>> +      A standard set of the clock sources contains CSRs bus clock, AXI-ports
> >>> +      reference clock, DDRC core clock, Scrubber standalone clock
> >>> +      (synchronous to the DDRC clock).
> >>> +    minItems: 1
> >>> +    maxItems: 4
> >>
> > 
> >> I expect list to be strictly defined, not flexible.
> > 

> > Some of the clock sources might be absent or tied up to another one
> > (for instance pclk, aclk and sbr can be clocked from a single core
> > clock source). It depends on the IP-core synthesize parameters.
> 
> Yet still your device has clock lines - clock inputs, right? Therefore
> still 4 clocks will be going there or you want to say that the pin is
> not connected (or pulled down)?

As we agreed my device will have dedicated DT-schema referencing this
one. The DT-bindings in the subject is the generic IP-core bindings.
So some of the lines indeed may be absent depending on the synthesize
parameters or the particular platform specifics. My device DT-schema
will contain more specific clocks/resets/irqs properties constraints.

> 
> > 
> >>
> >>> +
> >>> +  clock-names:
> >>> +    minItems: 1
> >>> +    maxItems: 4
> >>> +    items:
> >>> +      enum: [ pclk, aclk, core, sbr ]
> >>> +
> >>> +  resets:
> >>> +    description:
> >>> +      Each clock domain can have separate reset signal.
> >>> +    minItems: 1
> >>> +    maxItems: 4
> >>> +
> >>> +  reset-names:
> >>> +    minItems: 1
> >>> +    maxItems: 4
> >>> +    items:
> >>> +      enum: [ prst, arst, core, sbr ]
> >>
> > 
> >> The same.
> > 
> > The same as for the clock.
> > 
> >>
> >>> +
> >>>  required:
> >>>    - compatible
> >>>    - reg
> >>> @@ -48,4 +92,15 @@ examples:
> >>>        interrupt-parent = <&gic>;
> >>>        interrupts = <0 112 4>;
> >>>      };
> >>> +  - |
> >>> +    memory-controller@fd070000 {
> >>> +      compatible = "snps,ddrc-3.80a";
> >>> +      reg = <0x3d400000 0x400000>;
> >>> +
> >>> +      interrupts = <0 147 4>, <0 148 4>, <0 149 4>, <0 150 4>;
> >>
> > 
> >> Use proper defines.
> > 
> > What do you mean? Which defines do you think would be proper? If you
> > meant the IRQ DT-bindings macros, then what difference does it make
> > for a generic device in the DT-binding example? 
> 

> The macros/defines representing these numbers.
> 
> 
> > Note since the device
> > is defined as generic it can be placed on different platforms with
> > different interrupt controller requirements. So what do you mean by
> > "proper" in this case?
> 
> Proper means text instead of hard-coded number. This piece of code has
> meaning in a specific context, because you used interrupts matching some
> specific interrupt controllers. In that controller context, the "4" has
> a meaning. Just like "0". You cannot say that this piece of code is
> interrupt-controller-independent, because it is not. 4 has meaning.
> 
> If you want it to be independent, drop all the flags... If you use flags
> from a specific implementation, then use proper defines matching them,
> not hard-coded numbers.

I'll replace the number 4 with the IRQ-level triggered macro and drop
the flag 0 then.

-Sergey

> 
> Best regards,
> Krzysztof

Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Serge Semin]

Hi Rob,
Sorry for missing your message. It has kind of got lost among the
Krzysztof' comments.

On Tue, Aug 30, 2022 at 01:00:28PM -0500, Rob Herring wrote:
> On Mon, Aug 22, 2022 at 10:19:45PM +0300, Serge Semin wrote:
> > Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
> > with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
> > are individual IRQs for each ECC and DFI events.The dedicated scrubber
> > clock source is absent since it's fully synchronous to the core clock.
> > In addition to that the DFI-DDR PHY CSRs can be accessed via a separate
> > registers space.
> 

> Are you sure the phy and dfi irq shouldn't be a separate device?

I am sure that the DFI IRQ is a part of the DW uMCTl2 DDR controller
specification. The DFI interface has a special signal called
"dfi_alert_n". It is supposed to be supplied to the DDR controller
from the DDR PHY. The signal state indicates the CRC/Parity errors
detected on the address/command sent to the PHY/SDRAM side. Aside with
some other statuses the signal state is reflected in the DW uMCTL2
CRCPARSTAT register. The CSR state in its turn is then sent out via
the corresponding output wire (dfi_err_int) up to the IRQ controller.
So to speak there is no doubts the DFI errors IRQ is a part of the DW
uMCTL2 DDRC IRQs interface.

Regarding the PHY CSR space. You are right. It is a separate device
indeed. I'll drop the PHY CSR region from here.

-Sergey

> 
> Rob