Hi Boris, > -----Original Message----- > From: Borislav Petkov <bp@xxxxxxxxx> > Sent: Sunday, January 8, 2023 10:28 PM > To: Potthuri, Sai Krishna <sai.krishna.potthuri@xxxxxxx> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Michal Simek > <michal.simek@xxxxxxxxxx>; Mauro Carvalho Chehab > <mchehab@xxxxxxxxxx>; Tony Luck <tony.luck@xxxxxxxxx>; James Morse > <james.morse@xxxxxxx>; Robert Richter <rric@xxxxxxxxxx>; > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; linux-edac@xxxxxxxxxxxxxxx; > saikrishna12468@xxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>; Datta, > Shubhrajyoti <shubhrajyoti.datta@xxxxxxx>; kernel test robot > <lkp@xxxxxxxxx> > Subject: Re: [PATCH v7 2/2] EDAC/zynqmp: Add EDAC support for Xilinx > ZynqMP OCM > > On Wed, Jan 04, 2023 at 02:15:12PM +0530, Sai Krishna Potthuri wrote: > > Add EDAC support for Xilinx ZynqMP OCM Controller, so this driver > > reports CE and UE errors upon interrupt generation, and also creates > > UE/CE debugfs entries for error injection. > > On Xilinx ZynqMP platform, both OCM Controller driver(zynqmp_edac) and > > DDR Memory Controller driver(synopsys_edac) co-exist which means both > > can be loaded at a time. This scenario is tested on Xilinx ZynqMP > > platform. > > > > Fix following issue reported by the robot. > > "MAINTAINERS references a file that doesn't exist: > > Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml" > > > > Co-developed-by: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxx> > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxx> > > Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@xxxxxxx> > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > --- > > MAINTAINERS | 7 + > > drivers/edac/Kconfig | 9 + > > drivers/edac/Makefile | 1 + > > drivers/edac/zynqmp_edac.c | 465 > > +++++++++++++++++++++++++++++++++++++ > > 4 files changed, 482 insertions(+) > > create mode 100644 drivers/edac/zynqmp_edac.c > > Some touchups ontop, see below. > > I had to revert back to the #ifdeffery because IS_ENABLED doesn't prevent > the compiler from looking inside the conditional... > > Anyway, inter-diff below. Holler if something's still amiss. > > Thx. > > --- > > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig -index > 58ab63642e72..7944e40c67da 100644 > +index 4cfdefbd744d..68f576700911 100644 > --- a/drivers/edac/Kconfig > +++ b/drivers/edac/Kconfig > -@@ -539,4 +539,13 @@ config EDAC_DMC520 > +@@ -542,4 +542,12 @@ config EDAC_DMC520 > Support for error detection and correction on the > SoCs with ARM DMC-520 DRAM controller. > > -+config EDAC_ZYNQMP_OCM > ++config EDAC_ZYNQMP > + tristate "Xilinx ZynqMP OCM Controller" > + depends on ARCH_ZYNQMP || COMPILE_TEST > + help > + This driver supports error detection and correction for the > -+ Xilinx ZynqMP OCM (On Chip Memory) controller. > -+ This driver can also be built as a module. If so, the module > -+ will be called zynqmp_ocm_edac. > ++ Xilinx ZynqMP OCM (On Chip Memory) controller. It can also be > ++ built as a module. In that case it will be called zynqmp_edac. > + > endif # EDAC > diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile -index > 2d1641a27a28..47cbda06d7b0 100644 > +index 2d1641a27a28..9b025c5b3061 100644 > --- a/drivers/edac/Makefile > +++ b/drivers/edac/Makefile > @@ -84,3 +84,4 @@ obj-$(CONFIG_EDAC_QCOM) += > qcom_edac.o > obj-$(CONFIG_EDAC_ASPEED) += aspeed_edac.o > obj-$(CONFIG_EDAC_BLUEFIELD) += bluefield_edac.o > obj-$(CONFIG_EDAC_DMC520) += dmc520_edac.o > -+obj-$(CONFIG_EDAC_ZYNQMP_OCM) += zynqmp_edac.o > ++obj-$(CONFIG_EDAC_ZYNQMP) += zynqmp_edac.o > diff --git a/drivers/edac/zynqmp_edac.c b/drivers/edac/zynqmp_edac.c > new file mode 100644 -index 000000000000..69069028457b > +index 000000000000..b11f1157d4bb > --- /dev/null > +++ b/drivers/edac/zynqmp_edac.c > -@@ -0,0 +1,465 @@ > +@@ -0,0 +1,469 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Xilinx ZynqMP OCM ECC Driver > @@ -220,12 +224,14 @@ index 000000000000..69069028457b > + p->ceinfo.fault_hi = readl(base + CE_FFD1_OFST); > + p->ceinfo.addr = (OCM_BASEVAL | readl(base + > CE_FFA_OFST)); > + writel(ECC_CTRL_CLR_CE_ERR, base + OCM_ISR_OFST); > -+ } else { > ++ } else if (mask & OCM_UEINTR_MASK) { > + p->ue_cnt++; > + p->ueinfo.fault_lo = readl(base + UE_FFD0_OFST); > + p->ueinfo.fault_hi = readl(base + UE_FFD1_OFST); > + p->ueinfo.addr = (OCM_BASEVAL | readl(base + > UE_FFA_OFST)); > + writel(ECC_CTRL_CLR_UE_ERR, base + OCM_ISR_OFST); > ++ } else { > ++ WARN_ON_ONCE(1); > + } As we are raising a warning message in intr_handler() if the flagged interrupt is not UE or CE and we return from there, so do we really need else if{} and WARN_ON_ONCE() in else{} here? Regards Sai Krishna