diff for duplicates of <DLYC540CZECH.27M5I74J8RPRE@bootlin.com> diff --git a/a/1.txt b/N1/1.txt index b63569b..e6d664e 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -355,3 +355,404 @@ Best regards, Paul Louvel, Bootlin Embedded Linux and Kernel engineering https://bootlin.com + +text: +Return-Path: <paul.louvel@bootlin.com> +Delivered-To: paul.louvel@bootlin.com +Received: from mail.galae.net ([172.22.1.253]) + by 9e7fea892943 with LMTP + id Ke3hCSfQxWoZ4wkAtaWaaQ + (envelope-from <paul.louvel@bootlin.com>) + for <paul.louvel@bootlin.com>; Wed, 07 Oct 2026 06:52:55 +0200 +X-Original-To: paul.louvel@bootlin.com +Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id C4A6410330332; + Wed, 7 Oct 2026 06:52:50 +0200 (CEST) +DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; + t\x1791348774; h=from:subject:date:message-id:to:cc:mime-version:content-type: + content-transfer-encoding:in-reply-to:references; + bh=j7IEOp9lHLCsZ7esGUkRf4X2OUXpqeqqHgfGcznUyME=; + b=jFh4B2+teVqEuDYvLCOd6LoEpnGRXu02Gvdq4n9H1QMo3OI4ZFWwwU/56L1VKaqNBFQzbQ + lx1AgVzVsS16Yc2n5Z3lalM3LZRyUXaDC1A7TF0BBLmqFjPyZJnmqKlhv3TIaK3f2Nrrai + WIxaQkczuwYsnedgW9RXsUWNgURA217cjmV/kIX18+rYrJPsQsuB84xq7Yvj5eSJ+IJeeO + uuLib4CkGWh3dPsr8kzVGqJrU+UXgzLfrHaZK3tFyICnJ+Tq8hILEZQuDn35VeNAf0J8f5 + jTHHc8IW130QCal3JH3U4ccYOpxxzM/ns1QKPvciLqiaLuYXDt9VWLjCbxuCbQ=Mime-Version: 1.0 +Content-Transfer-Encoding: quoted-printable +Content-Type: text/plain; charset=UTF-8 +Date: Wed, 07 Oct 2026 06:52:49 +0200 +Message-Id: <DLYC540CZECH.27M5I74J8RPRE@bootlin.com> +Cc: "Tony Luck" <tony.luck@intel.com>, "Rob Herring" <robh@kernel.org>, + "Krzysztof Kozlowski" <krzk+dt@kernel.org>, "Conor Dooley" + <conor+dt@kernel.org>, "Geert Uytterhoeven" <geert+renesas@glider.be>, + "Magnus Damm" <magnus.damm@gmail.com>, <linux-kernel@vger.kernel.org>, + <linux-renesas-soc@vger.kernel.org>, <linux-edac@vger.kernel.org>, + <devicetree@vger.kernel.org>, "Thomas Petazzoni" + <thomas.petazzoni@bootlin.com>, "Miquel Raynal" + <miquel.raynal@bootlin.com>, "Herve Codina" <herve.codina@bootlin.com> +Subject: Re: [PATCH v5 2/3] EDAC/cadence: Add Cadence DDR EDAC driver +From: "Paul Louvel" <paul.louvel@bootlin.com> +To: "Borislav Petkov" <bp@alien8.de>, "Paul Louvel" + <paul.louvel@bootlin.com> +X-Mailer: aerc 0.21.0-0-g5549850facc2 +References: <20261005-paul-v7-3-rc1-edac-v5-0-140a0f124bc0@bootlin.com> + <20261005-paul-v7-3-rc1-edac-v5-2-140a0f124bc0@bootlin.com> + <20261007021142.GAasWqXsuPpjYUIYIv@fat_crate.local> +In-Reply-To: <20261007021142.GAasWqXsuPpjYUIYIv@fat_crate.local> +X-Last-TLS-Session-Version: TLSv1.3 + +On Wed Oct 7, 2026 at 4:11 AM CEST, Borislav Petkov wrote: +> On Mon, Oct 05, 2026 at 02:49:23PM +0200, Paul Louvel wrote: +>> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile +>> index a37534300ab9..0d66a072b15c 100644 +>> --- a/drivers/edac/Makefile +>> +++ b/drivers/edac/Makefile +>> @@ -82,6 +82,7 @@ obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o +>> obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o +>> obj-$(CONFIG_EDAC_TI) += ti_edac.o +>> obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o +>> +obj-$(CONFIG_EDAC_CADENCE) += cadence_edac.o +> +> This goes at the end of that file. +> +>> obj-$(CONFIG_EDAC_ASPEED) += aspeed_edac.o +>> obj-$(CONFIG_EDAC_BLUEFIELD) += bluefield_edac.o +>> obj-$(CONFIG_EDAC_DMC520) += dmc520_edac.o +>> diff --git a/drivers/edac/cadence_edac.c b/drivers/edac/cadence_edac.c +>> new file mode 100644 +>> index 000000000000..26b9a7facd57 +>> --- /dev/null +>> +++ b/drivers/edac/cadence_edac.c +>> @@ -0,0 +1,399 @@ +>> +// SPDX-License-Identifier: GPL-2.0+ +>> +/* +>> + * Copyright 2015 Renesas Electronics Europe Ltd. +>> + * Copyright 2026 Bootlin +>> + * +>> + * Based on highbank EDAC driver: +>> + * +>> + * Copyright 2011-2012 Calxeda, Inc. +>> + */ +>> + +>> +#include <linux/bits.h> +>> +#include <linux/bitfield.h> +>> +#include <linux/clk.h> +>> +#include <linux/edac.h> +>> +#include <linux/interrupt.h> +>> +#include <linux/io.h> +>> +#include <linux/kernel.h> +>> +#include <linux/mm.h> + ^^^^^^^^^^^^ +I think this one is not needed. + +>> +#include <linux/of_address.h> +>> +#include <linux/string.h> + ^^^^^^^^^^^^^^^^ +This one too since I will remove the port channel protection feature, and the +snprintf will go away. + +>> +#include <linux/spinlock.h> +>> +#include <linux/platform_device.h> +> +> How many of those includes are *actually* needed? +> +>> +#include "edac_mc.h" +>> +#include "edac_module.h" +>> + +>> +#define DRV_NAME "cdns_edac" +> +> "cadence_edac" is a perfectly fine name. +> +>> +#define REG_BYTE_SZ 4 +>> +#define DDR_CTL(n) ((n) * REG_BYTE_SZ) +>> + +>> +#define CDNS_DDR_DDR_STAT DDR_CTL(0) +> +> Align all defines vertically like this: +> +> #define REG_BYTE_SZ 4 +> #define DDR_CTL(n) ((n) * REG_BYTE_SZ) +> +> #define CDNS_DDR_DDR_STAT DDR_CTL(0) +> ... +> +>> +#define CDNS_DDR_DDR_STAT_DRAM_CLASS GENMASK_U32(11, 8) +>> +#define CDNS_DDR_DDR_STAT_DRAM_DDR2 BIT(2) +>> +#define CDNS_DDR_DDR_STAT_GET_DRAM_CLASS(reg) FIELD_GET(CDNS_DDR_DDR_STAT_DRAM_CLASS, reg) +> +> Also, I would shorten those long mouthfuls so that the code remains relatively +> readable. The "DDR_DDR" thing above is the first I'd whack. And so on. + +I will drop the first "DDR" prefix. + +> +>> +#define CDNS_DDR_ECC_STAT DDR_CTL(36) +>> +#define CDNS_DDR_ECC_STAT_ENABLED BIT(16) +>> +#define CDNS_DDR_ECC_STAT_IS_ENABLED(reg) FIELD_GET(CDNS_DDR_ECC_STAT_ENABLED, reg) +>> +#define CDNS_DDR_ECC_STAT_FWC BIT(24) +>> + +>> +#define CDNS_DDR_ECC_XOR DDR_CTL(37) +>> +#define CDNS_DDR_ECC_XOR_CHECK_BITS GENMASK_U32(13, 0) +>> + +>> +#define CDNS_DDR_BUS_CTRL DDR_CTL(54) +>> +#define CDNS_DDR_BUS_CTRL_REDUC BIT(1) +>> + +>> +/* DDR Controller Error Registers */ +>> + +>> +#define CDNS_DDR_ECC_U_ERR_ADDR DDR_CTL(38) +>> +#define CDNS_DDR_ECC_U_ERR_STAT DDR_CTL(39) +>> + +>> +#define CDNS_DDR_ECC_C_ERR_ADDR DDR_CTL(41) +>> +#define CDNS_DDR_ECC_C_ERR_STAT DDR_CTL(42) +>> + +>> +#define CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg) FIELD_GET(GENMASK(6, 0), reg) +>> + +>> +#define CDNS_DDR_PORT_CMD_ERR_ADDR DDR_CTL(61) +>> +#define CDNS_DDR_PORT_CMD_ERR_TYPE DDR_CTL(62) +>> +#define CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg) FIELD_GET(GENMASK_U32(10, 8), reg) +>> + +>> +/* DDR Controller Interrupt Registers */ +>> + +>> +#define CDNS_DDR_ECC_INT_STAT DDR_CTL(56) +>> +#define CDNS_DDR_ECC_INT_STAT_CE BIT(3) +>> +#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE BIT(4) +>> +#define CDNS_DDR_ECC_INT_STAT_UE BIT(5) +>> +#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE BIT(6) +>> +#define CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN BIT(7) +>> + +>> +#define CDNS_DDR_ECC_INT_ACK DDR_CTL(57) +>> +#define CDNS_DDR_ECC_INT_ACK_MASK GENMASK_U32(21, 0) +>> + +>> +#define CDNS_DDR_ECC_INT_CTRL DDR_CTL(58) +>> +#define CDNS_DDR_ECC_INT_CTRL_MASK GENMASK_U32(21, 0) +>> +#define CDNS_DDR_ECC_INT_CTRL_MASK_ALL BIT(22) +>> +#define CDNS_DDR_ECC_INT_CTRL_UNMASK(i) ((~(i)) & CDNS_DDR_ECC_INT_CTRL_MASK) +>> + +>> +struct cdns_mc_priv { +> +> For all privately used struct names and static functions, drop the "cdns_" +> namespace prefix - it is not necessary. +> +>> + int irq; +>> + void __iomem *io_base; +>> + struct dentry *debugfs; +>> + spinlock_t lock; +>> + u16 xor_check_bits; +>> +}; +>> + +>> +static irqreturn_t cdns_mc_err_handler(int irq, void *dev_id) +>> +{ +>> + struct mem_ctl_info *mci = dev_id; +>> + struct cdns_mc_priv *priv = mci->pvt_info; +>> + u32 addr, status, err_addr, syndrome, reg; +>> + char other_details_str[32]; +>> + u8 type; +>> + +>> + /* Read the interrupt status register */ +> +> The fact that you have to put an obvious comment above the read of a register +> basically says that your register naming is not optimal enough. If you name it +> properly, you don't need a comment. +> + +I do think the register naming is fine and follows common conventions +(INT=interrupt, STAT=status). Also, the datasheet does not give any meaningful +name to these registers (generic like DDR_CTL_##) and usually a single register +is used for multiple purposes.. +I will remove the comment. + +>> + status = readl(priv->io_base + CDNS_DDR_ECC_INT_STAT); +>> + if (!status) +>> + return IRQ_NONE; +>> + +>> + /* +>> + * We can't know how many CE / UE occurred since last ACK in case of +> +> Please use passive voice: no "we" or "I", etc, and describe things in an +> imperative mood. +> +>> + * multiple errors. Just report it. +>> + */ +>> + +>> + if ((status & CDNS_DDR_ECC_INT_STAT_UE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE)) { +> +> This is what I mean with too long lines. That one and others like it needs +> shortening. +> +> Also this test can be merged into a single one by ORing the flags. +> +>> + reg = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_STAT); +>> + syndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg); +>> + +>> + err_addr = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_ADDR); +>> + +>> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1, err_addr >> PAGE_SHIFT, +>> + err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, ""); +>> + } +> +> ditto for that one below: +> +>> + if ((status & CDNS_DDR_ECC_INT_STAT_CE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE)) { +>> + reg = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_STAT); +>> + syndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg); +>> + +>> + err_addr = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_ADDR); +>> + +>> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1, err_addr >> PAGE_SHIFT, +>> + err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, ""); +>> + } +>> + +>> + if (status & CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN) { +> +> What kind of an error is that one so that you have to call +> edac_mc_handle_error() for it separately? + +This is a leftover from the downstream driver I picked up. +TBH, I do not think this error belongs in an EDAC driver so I will remove it. +This is a protection feature of the memory controller that is not related to +ECC. + +> +>> + addr = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_ADDR); +>> + reg = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_TYPE); +>> + type = CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg); +>> + +>> + snprintf(other_details_str, sizeof(other_details_str), "type 0x%02x", type); +>> + +>> + edac_mc_handle_error(HW_EVENT_ERR_INFO, mci, 1, addr >> PAGE_SHIFT, +>> + addr & ~PAGE_MASK, 0, 0, 0, -1, mci->ctl_name, +>> + other_details_str); +>> + } +>> + +>> + /* clear the error, clears the interrupt */ +> +> No need for obvious comments. Audit your whole driver pls. +> +>> + writel(status & CDNS_DDR_ECC_INT_ACK_MASK, priv->io_base + CDNS_DDR_ECC_INT_ACK); +>> + +>> + return IRQ_HANDLED; +>> +} +>> + +>> +static int cdns_get_mem_sz(resource_size_t *mem_sz) +> +> Do not use an I/O function param but return the correct size or an error and +> have call site handle that. +> +> Looking how that function is called only once, simply merge it into the call +> site. + +Liked it that way so I can avoid any potential overflow situation. +But yeah, will merge it. + +> +>> +{ +>> + struct device_node *np; +>> + struct resource res; +>> + int ret; +>> + +>> + np = of_find_node_by_name(NULL, "memory"); +>> + if (!np) +>> + return -ENODEV; +>> + +>> + ret = of_address_to_resource(np, 0, &res); +>> + +>> + of_node_put(np); +>> + +>> + if (ret) +>> + return ret; +>> + +>> + *mem_sz = resource_size(&res); +>> + +>> + return 0; +>> +} +> +>> + +>> +#ifdef CONFIG_EDAC_DEBUG +>> + +>> +static void cdns_rmw(struct cdns_mc_priv *priv, u32 reg, u32 mask, u32 val) +>> +{ +>> + u32 regval; +>> + +>> + regval = readl(priv->io_base + reg); +>> + regval &= ~mask; +>> + regval |= (val << __bf_shf(mask)) & mask; +>> + writel(regval, priv->io_base + reg); +>> +} +>> + +>> +static ssize_t cdns_force_ecc_error(struct file *file, const char __user *data, size_t count, +>> + loff_t *ppos) +>> +{ +>> + struct device *dev = file->private_data; +>> + struct mem_ctl_info *mci = to_mci(dev); +>> + struct cdns_mc_priv *priv = mci->pvt_info; +>> + +>> + spin_lock(&priv->lock); +>> + +>> + cdns_rmw(priv, CDNS_DDR_ECC_XOR, CDNS_DDR_ECC_XOR_CHECK_BITS, priv->xor_check_bits); +>> + cdns_rmw(priv, CDNS_DDR_ECC_STAT, CDNS_DDR_ECC_STAT_FWC, 1); +>> + +>> + spin_unlock(&priv->lock); +>> + +>> + return count; +>> +} +>> + +>> +static const struct file_operations cdns_ecc_error_fops = { +>> + .open = simple_open, +>> + .write = cdns_force_ecc_error, +>> + .llseek = generic_file_llseek, +>> +}; +> +> #else +> static ssize_t cdns_force_ecc_error(struct file *file, const char __user *data, size_t count, loff_t *ppos) +> { +> return 0 +> } +> #endif +> +> and get rid of the ifdeffery below. +> +>> +static void cdns_setup_debugfs(struct mem_ctl_info *mci) +>> +{ +>> + struct cdns_mc_priv *priv = mci->pvt_info; +>> + +>> + priv->debugfs = edac_debugfs_create_dir(DRV_NAME); +>> + if (!priv->debugfs) { +>> + dev_dbg(mci->pdev, "failed to create debugfs dir\n"); +>> + return; +>> + } +>> + +>> + edac_debugfs_create_x16("bits", 0644, priv->debugfs, &priv->xor_check_bits); +>> + edac_debugfs_create_file("inject", 0200, priv->debugfs, &mci->dev, &cdns_ecc_error_fops); +>> +} +>> + +>> +#endif +> +> ... +> +>> + +>> + /* +>> + * Unmask ECC recoverable and unrecoverable interrupts, and port +>> + * command errors. +>> + */ +>> + writel(CDNS_DDR_ECC_INT_CTRL_UNMASK( +>> + CDNS_DDR_ECC_INT_STAT_CE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE | +>> + CDNS_DDR_ECC_INT_STAT_UE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE | +>> + CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN), +> +> Yah, unreadable mess that. Shorten pls. + +I do not see any better way to express it. +I could define a macro for the OR'ing mess sure. + +> +>> + io_base + CDNS_DDR_ECC_INT_CTRL); +>> + +>> + return 0; +>> +} +> +> Thx. + +Thanks. +Best regards, +-- +Paul Louvel, Bootlin +Embedded Linux and Kernel engineering +https://bootlin.com diff --git a/a/content_digest b/N1/content_digest index b3b77dc..20b767f 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -377,6 +377,407 @@ "-- \n" "Paul Louvel, Bootlin\n" "Embedded Linux and Kernel engineering\n" + "https://bootlin.com\n" + "\n" + "text:\n" + "Return-Path: <paul.louvel@bootlin.com>\n" + "Delivered-To: paul.louvel@bootlin.com\n" + "Received: from mail.galae.net ([172.22.1.253])\n" + "\tby 9e7fea892943 with LMTP\n" + "\tid Ke3hCSfQxWoZ4wkAtaWaaQ\n" + "\t(envelope-from <paul.louvel@bootlin.com>)\n" + "\tfor <paul.louvel@bootlin.com>; Wed, 07 Oct 2026 06:52:55 +0200\n" + "X-Original-To: paul.louvel@bootlin.com\n" + "Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id C4A6410330332;\n" + "\tWed, 7 Oct 2026 06:52:50 +0200 (CEST)\n" + "DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim;\n" + "\tt\02791348774; h=from:subject:date:message-id:to:cc:mime-version:content-type:\n" + "\t content-transfer-encoding:in-reply-to:references;\n" + "\tbh=j7IEOp9lHLCsZ7esGUkRf4X2OUXpqeqqHgfGcznUyME=;\n" + "\tb=jFh4B2+teVqEuDYvLCOd6LoEpnGRXu02Gvdq4n9H1QMo3OI4ZFWwwU/56L1VKaqNBFQzbQ\n" + "\tlx1AgVzVsS16Yc2n5Z3lalM3LZRyUXaDC1A7TF0BBLmqFjPyZJnmqKlhv3TIaK3f2Nrrai\n" + "\tWIxaQkczuwYsnedgW9RXsUWNgURA217cjmV/kIX18+rYrJPsQsuB84xq7Yvj5eSJ+IJeeO\n" + "\tuuLib4CkGWh3dPsr8kzVGqJrU+UXgzLfrHaZK3tFyICnJ+Tq8hILEZQuDn35VeNAf0J8f5\n" + "\tjTHHc8IW130QCal3JH3U4ccYOpxxzM/ns1QKPvciLqiaLuYXDt9VWLjCbxuCbQ=Mime-Version: 1.0\n" + "Content-Transfer-Encoding: quoted-printable\n" + "Content-Type: text/plain; charset=UTF-8\n" + "Date: Wed, 07 Oct 2026 06:52:49 +0200\n" + "Message-Id: <DLYC540CZECH.27M5I74J8RPRE@bootlin.com>\n" + "Cc: \"Tony Luck\" <tony.luck@intel.com>, \"Rob Herring\" <robh@kernel.org>,\n" + " \"Krzysztof Kozlowski\" <krzk+dt@kernel.org>, \"Conor Dooley\"\n" + " <conor+dt@kernel.org>, \"Geert Uytterhoeven\" <geert+renesas@glider.be>,\n" + " \"Magnus Damm\" <magnus.damm@gmail.com>, <linux-kernel@vger.kernel.org>,\n" + " <linux-renesas-soc@vger.kernel.org>, <linux-edac@vger.kernel.org>,\n" + " <devicetree@vger.kernel.org>, \"Thomas Petazzoni\"\n" + " <thomas.petazzoni@bootlin.com>, \"Miquel Raynal\"\n" + " <miquel.raynal@bootlin.com>, \"Herve Codina\" <herve.codina@bootlin.com>\n" + "Subject: Re: [PATCH v5 2/3] EDAC/cadence: Add Cadence DDR EDAC driver\n" + "From: \"Paul Louvel\" <paul.louvel@bootlin.com>\n" + "To: \"Borislav Petkov\" <bp@alien8.de>, \"Paul Louvel\"\n" + " <paul.louvel@bootlin.com>\n" + "X-Mailer: aerc 0.21.0-0-g5549850facc2\n" + "References: <20261005-paul-v7-3-rc1-edac-v5-0-140a0f124bc0@bootlin.com>\n" + " <20261005-paul-v7-3-rc1-edac-v5-2-140a0f124bc0@bootlin.com>\n" + " <20261007021142.GAasWqXsuPpjYUIYIv@fat_crate.local>\n" + "In-Reply-To: <20261007021142.GAasWqXsuPpjYUIYIv@fat_crate.local>\n" + "X-Last-TLS-Session-Version: TLSv1.3\n" + "\n" + "On Wed Oct 7, 2026 at 4:11 AM CEST, Borislav Petkov wrote:\n" + "> On Mon, Oct 05, 2026 at 02:49:23PM +0200, Paul Louvel wrote:\n" + ">> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile\n" + ">> index a37534300ab9..0d66a072b15c 100644\n" + ">> --- a/drivers/edac/Makefile\n" + ">> +++ b/drivers/edac/Makefile\n" + ">> @@ -82,6 +82,7 @@ obj-$(CONFIG_EDAC_SYNOPSYS)\t\t+= synopsys_edac.o\n" + ">> obj-$(CONFIG_EDAC_XGENE)\t\t+= xgene_edac.o\n" + ">> obj-$(CONFIG_EDAC_TI)\t\t\t+= ti_edac.o\n" + ">> obj-$(CONFIG_EDAC_QCOM)\t\t\t+= qcom_edac.o\n" + ">> +obj-$(CONFIG_EDAC_CADENCE)\t\t+= cadence_edac.o\n" + ">\n" + "> This goes at the end of that file.\n" + ">\n" + ">> obj-$(CONFIG_EDAC_ASPEED)\t\t+= aspeed_edac.o\n" + ">> obj-$(CONFIG_EDAC_BLUEFIELD)\t\t+= bluefield_edac.o\n" + ">> obj-$(CONFIG_EDAC_DMC520)\t\t+= dmc520_edac.o\n" + ">> diff --git a/drivers/edac/cadence_edac.c b/drivers/edac/cadence_edac.c\n" + ">> new file mode 100644\n" + ">> index 000000000000..26b9a7facd57\n" + ">> --- /dev/null\n" + ">> +++ b/drivers/edac/cadence_edac.c\n" + ">> @@ -0,0 +1,399 @@\n" + ">> +// SPDX-License-Identifier: GPL-2.0+\n" + ">> +/*\n" + ">> + * Copyright 2015 Renesas Electronics Europe Ltd.\n" + ">> + * Copyright 2026 Bootlin\n" + ">> + *\n" + ">> + * Based on highbank EDAC driver:\n" + ">> + *\n" + ">> + * Copyright 2011-2012 Calxeda, Inc.\n" + ">> + */\n" + ">> +\n" + ">> +#include <linux/bits.h>\n" + ">> +#include <linux/bitfield.h>\n" + ">> +#include <linux/clk.h>\n" + ">> +#include <linux/edac.h>\n" + ">> +#include <linux/interrupt.h>\n" + ">> +#include <linux/io.h>\n" + ">> +#include <linux/kernel.h>\n" + ">> +#include <linux/mm.h>\n" + " ^^^^^^^^^^^^\n" + "I think this one is not needed.\n" + "\n" + ">> +#include <linux/of_address.h>\n" + ">> +#include <linux/string.h>\n" + " ^^^^^^^^^^^^^^^^\n" + "This one too since I will remove the port channel protection feature, and the\n" + "snprintf will go away.\n" + "\n" + ">> +#include <linux/spinlock.h>\n" + ">> +#include <linux/platform_device.h>\n" + ">\n" + "> How many of those includes are *actually* needed?\n" + ">\n" + ">> +#include \"edac_mc.h\"\n" + ">> +#include \"edac_module.h\"\n" + ">> +\n" + ">> +#define DRV_NAME \"cdns_edac\"\n" + ">\n" + "> \"cadence_edac\" is a perfectly fine name.\n" + ">\n" + ">> +#define REG_BYTE_SZ 4\n" + ">> +#define DDR_CTL(n) ((n) * REG_BYTE_SZ)\n" + ">> +\n" + ">> +#define CDNS_DDR_DDR_STAT DDR_CTL(0)\n" + ">\n" + "> Align all defines vertically like this:\n" + ">\n" + "> #define REG_BYTE_SZ\t\t4\n" + "> #define DDR_CTL(n)\t\t((n) * REG_BYTE_SZ)\n" + ">\n" + "> #define CDNS_DDR_DDR_STAT\tDDR_CTL(0)\n" + "> ...\n" + ">\n" + ">> +#define CDNS_DDR_DDR_STAT_DRAM_CLASS GENMASK_U32(11, 8)\n" + ">> +#define CDNS_DDR_DDR_STAT_DRAM_DDR2 BIT(2)\n" + ">> +#define CDNS_DDR_DDR_STAT_GET_DRAM_CLASS(reg) FIELD_GET(CDNS_DDR_DDR_STAT_DRAM_CLASS, reg)\n" + ">\n" + "> Also, I would shorten those long mouthfuls so that the code remains relatively\n" + "> readable. The \"DDR_DDR\" thing above is the first I'd whack. And so on.\n" + "\n" + "I will drop the first \"DDR\" prefix.\n" + "\n" + ">\n" + ">> +#define CDNS_DDR_ECC_STAT DDR_CTL(36)\n" + ">> +#define CDNS_DDR_ECC_STAT_ENABLED BIT(16)\n" + ">> +#define CDNS_DDR_ECC_STAT_IS_ENABLED(reg) FIELD_GET(CDNS_DDR_ECC_STAT_ENABLED, reg)\n" + ">> +#define CDNS_DDR_ECC_STAT_FWC BIT(24)\n" + ">> +\n" + ">> +#define CDNS_DDR_ECC_XOR DDR_CTL(37)\n" + ">> +#define CDNS_DDR_ECC_XOR_CHECK_BITS GENMASK_U32(13, 0)\n" + ">> +\n" + ">> +#define CDNS_DDR_BUS_CTRL DDR_CTL(54)\n" + ">> +#define CDNS_DDR_BUS_CTRL_REDUC BIT(1)\n" + ">> +\n" + ">> +/* DDR Controller Error Registers */\n" + ">> +\n" + ">> +#define CDNS_DDR_ECC_U_ERR_ADDR DDR_CTL(38)\n" + ">> +#define CDNS_DDR_ECC_U_ERR_STAT DDR_CTL(39)\n" + ">> +\n" + ">> +#define CDNS_DDR_ECC_C_ERR_ADDR DDR_CTL(41)\n" + ">> +#define CDNS_DDR_ECC_C_ERR_STAT DDR_CTL(42)\n" + ">> +\n" + ">> +#define CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg) FIELD_GET(GENMASK(6, 0), reg)\n" + ">> +\n" + ">> +#define CDNS_DDR_PORT_CMD_ERR_ADDR DDR_CTL(61)\n" + ">> +#define CDNS_DDR_PORT_CMD_ERR_TYPE DDR_CTL(62)\n" + ">> +#define CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg) FIELD_GET(GENMASK_U32(10, 8), reg)\n" + ">> +\n" + ">> +/* DDR Controller Interrupt Registers */\n" + ">> +\n" + ">> +#define CDNS_DDR_ECC_INT_STAT DDR_CTL(56)\n" + ">> +#define CDNS_DDR_ECC_INT_STAT_CE BIT(3)\n" + ">> +#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE BIT(4)\n" + ">> +#define CDNS_DDR_ECC_INT_STAT_UE BIT(5)\n" + ">> +#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE BIT(6)\n" + ">> +#define CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN BIT(7)\n" + ">> +\n" + ">> +#define CDNS_DDR_ECC_INT_ACK DDR_CTL(57)\n" + ">> +#define CDNS_DDR_ECC_INT_ACK_MASK GENMASK_U32(21, 0)\n" + ">> +\n" + ">> +#define CDNS_DDR_ECC_INT_CTRL DDR_CTL(58)\n" + ">> +#define CDNS_DDR_ECC_INT_CTRL_MASK GENMASK_U32(21, 0)\n" + ">> +#define CDNS_DDR_ECC_INT_CTRL_MASK_ALL BIT(22)\n" + ">> +#define CDNS_DDR_ECC_INT_CTRL_UNMASK(i) ((~(i)) & CDNS_DDR_ECC_INT_CTRL_MASK)\n" + ">> +\n" + ">> +struct cdns_mc_priv {\n" + ">\n" + "> For all privately used struct names and static functions, drop the \"cdns_\"\n" + "> namespace prefix - it is not necessary.\n" + ">\n" + ">> +\tint irq;\n" + ">> +\tvoid __iomem *io_base;\n" + ">> +\tstruct dentry *debugfs;\n" + ">> +\tspinlock_t lock;\n" + ">> +\tu16 xor_check_bits;\n" + ">> +};\n" + ">> +\n" + ">> +static irqreturn_t cdns_mc_err_handler(int irq, void *dev_id)\n" + ">> +{\n" + ">> +\tstruct mem_ctl_info *mci = dev_id;\n" + ">> +\tstruct cdns_mc_priv *priv = mci->pvt_info;\n" + ">> +\tu32 addr, status, err_addr, syndrome, reg;\n" + ">> +\tchar other_details_str[32];\n" + ">> +\tu8 type;\n" + ">> +\n" + ">> +\t/* Read the interrupt status register */\n" + ">\n" + "> The fact that you have to put an obvious comment above the read of a register\n" + "> basically says that your register naming is not optimal enough. If you name it\n" + "> properly, you don't need a comment.\n" + ">\n" + "\n" + "I do think the register naming is fine and follows common conventions\n" + "(INT=interrupt, STAT=status). Also, the datasheet does not give any meaningful\n" + "name to these registers (generic like DDR_CTL_##) and usually a single register\n" + "is used for multiple purposes..\n" + "I will remove the comment.\n" + "\n" + ">> +\tstatus = readl(priv->io_base + CDNS_DDR_ECC_INT_STAT);\n" + ">> +\tif (!status)\n" + ">> +\t\treturn IRQ_NONE;\n" + ">> +\n" + ">> +\t/*\n" + ">> +\t * We can't know how many CE / UE occurred since last ACK in case of\n" + ">\n" + "> Please use passive voice: no \"we\" or \"I\", etc, and describe things in an\n" + "> imperative mood.\n" + ">\n" + ">> +\t * multiple errors. Just report it.\n" + ">> +\t */\n" + ">> +\n" + ">> +\tif ((status & CDNS_DDR_ECC_INT_STAT_UE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE)) {\n" + ">\n" + "> This is what I mean with too long lines. That one and others like it needs\n" + "> shortening.\n" + ">\n" + "> Also this test can be merged into a single one by ORing the flags.\n" + ">\n" + ">> +\t\treg = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_STAT);\n" + ">> +\t\tsyndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg);\n" + ">> +\n" + ">> +\t\terr_addr = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_ADDR);\n" + ">> +\n" + ">> +\t\tedac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1, err_addr >> PAGE_SHIFT,\n" + ">> +\t\t\t\t err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, \"\");\n" + ">> +\t}\n" + ">\n" + "> ditto for that one below:\n" + ">\n" + ">> +\tif ((status & CDNS_DDR_ECC_INT_STAT_CE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE)) {\n" + ">> +\t\treg = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_STAT);\n" + ">> +\t\tsyndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg);\n" + ">> +\n" + ">> +\t\terr_addr = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_ADDR);\n" + ">> +\n" + ">> +\t\tedac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1, err_addr >> PAGE_SHIFT,\n" + ">> +\t\t\t\t err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, \"\");\n" + ">> +\t}\n" + ">> +\n" + ">> +\tif (status & CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN) {\n" + ">\n" + "> What kind of an error is that one so that you have to call\n" + "> edac_mc_handle_error() for it separately?\n" + "\n" + "This is a leftover from the downstream driver I picked up.\n" + "TBH, I do not think this error belongs in an EDAC driver so I will remove it.\n" + "This is a protection feature of the memory controller that is not related to\n" + "ECC.\n" + "\n" + ">\n" + ">> +\t\taddr = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_ADDR);\n" + ">> +\t\treg = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_TYPE);\n" + ">> +\t\ttype = CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg);\n" + ">> +\n" + ">> +\t\tsnprintf(other_details_str, sizeof(other_details_str), \"type 0x%02x\", type);\n" + ">> +\n" + ">> +\t\tedac_mc_handle_error(HW_EVENT_ERR_INFO, mci, 1, addr >> PAGE_SHIFT,\n" + ">> +\t\t\t\t addr & ~PAGE_MASK, 0, 0, 0, -1, mci->ctl_name,\n" + ">> +\t\t\t\t other_details_str);\n" + ">> +\t}\n" + ">> +\n" + ">> +\t/* clear the error, clears the interrupt */\n" + ">\n" + "> No need for obvious comments. Audit your whole driver pls.\n" + ">\n" + ">> +\twritel(status & CDNS_DDR_ECC_INT_ACK_MASK, priv->io_base + CDNS_DDR_ECC_INT_ACK);\n" + ">> +\n" + ">> +\treturn IRQ_HANDLED;\n" + ">> +}\n" + ">> +\n" + ">> +static int cdns_get_mem_sz(resource_size_t *mem_sz)\n" + ">\n" + "> Do not use an I/O function param but return the correct size or an error and\n" + "> have call site handle that.\n" + ">\n" + "> Looking how that function is called only once, simply merge it into the call\n" + "> site.\n" + "\n" + "Liked it that way so I can avoid any potential overflow situation.\n" + "But yeah, will merge it.\n" + "\n" + ">\n" + ">> +{\n" + ">> +\tstruct device_node *np;\n" + ">> +\tstruct resource res;\n" + ">> +\tint ret;\n" + ">> +\n" + ">> +\tnp = of_find_node_by_name(NULL, \"memory\");\n" + ">> +\tif (!np)\n" + ">> +\t\treturn -ENODEV;\n" + ">> +\n" + ">> +\tret = of_address_to_resource(np, 0, &res);\n" + ">> +\n" + ">> +\tof_node_put(np);\n" + ">> +\n" + ">> +\tif (ret)\n" + ">> +\t\treturn ret;\n" + ">> +\n" + ">> +\t*mem_sz = resource_size(&res);\n" + ">> +\n" + ">> +\treturn 0;\n" + ">> +}\n" + ">\n" + ">> +\n" + ">> +#ifdef CONFIG_EDAC_DEBUG\n" + ">> +\n" + ">> +static void cdns_rmw(struct cdns_mc_priv *priv, u32 reg, u32 mask, u32 val)\n" + ">> +{\n" + ">> +\tu32 regval;\n" + ">> +\n" + ">> +\tregval = readl(priv->io_base + reg);\n" + ">> +\tregval &= ~mask;\n" + ">> +\tregval |= (val << __bf_shf(mask)) & mask;\n" + ">> +\twritel(regval, priv->io_base + reg);\n" + ">> +}\n" + ">> +\n" + ">> +static ssize_t cdns_force_ecc_error(struct file *file, const char __user *data, size_t count,\n" + ">> +\t\t\t\t loff_t *ppos)\n" + ">> +{\n" + ">> +\tstruct device *dev = file->private_data;\n" + ">> +\tstruct mem_ctl_info *mci = to_mci(dev);\n" + ">> +\tstruct cdns_mc_priv *priv = mci->pvt_info;\n" + ">> +\n" + ">> +\tspin_lock(&priv->lock);\n" + ">> +\n" + ">> +\tcdns_rmw(priv, CDNS_DDR_ECC_XOR, CDNS_DDR_ECC_XOR_CHECK_BITS, priv->xor_check_bits);\n" + ">> +\tcdns_rmw(priv, CDNS_DDR_ECC_STAT, CDNS_DDR_ECC_STAT_FWC, 1);\n" + ">> +\n" + ">> +\tspin_unlock(&priv->lock);\n" + ">> +\n" + ">> +\treturn count;\n" + ">> +}\n" + ">> +\n" + ">> +static const struct file_operations cdns_ecc_error_fops = {\n" + ">> +\t.open = simple_open,\n" + ">> +\t.write = cdns_force_ecc_error,\n" + ">> +\t.llseek = generic_file_llseek,\n" + ">> +};\n" + ">\n" + "> #else\n" + "> static ssize_t cdns_force_ecc_error(struct file *file, const char __user *data, size_t count, loff_t *ppos)\n" + "> {\n" + "> \treturn 0\n" + "> }\n" + "> #endif\n" + ">\n" + "> and get rid of the ifdeffery below.\n" + ">\n" + ">> +static void cdns_setup_debugfs(struct mem_ctl_info *mci)\n" + ">> +{\n" + ">> +\tstruct cdns_mc_priv *priv = mci->pvt_info;\n" + ">> +\n" + ">> +\tpriv->debugfs = edac_debugfs_create_dir(DRV_NAME);\n" + ">> +\tif (!priv->debugfs) {\n" + ">> +\t\tdev_dbg(mci->pdev, \"failed to create debugfs dir\\n\");\n" + ">> +\t\treturn;\n" + ">> +\t}\n" + ">> +\n" + ">> +\tedac_debugfs_create_x16(\"bits\", 0644, priv->debugfs, &priv->xor_check_bits);\n" + ">> +\tedac_debugfs_create_file(\"inject\", 0200, priv->debugfs, &mci->dev, &cdns_ecc_error_fops);\n" + ">> +}\n" + ">> +\n" + ">> +#endif\n" + ">\n" + "> ...\n" + ">\n" + ">> +\n" + ">> +\t/*\n" + ">> +\t * Unmask ECC recoverable and unrecoverable interrupts, and port\n" + ">> +\t * command errors.\n" + ">> +\t */\n" + ">> +\twritel(CDNS_DDR_ECC_INT_CTRL_UNMASK(\n" + ">> +\t\t CDNS_DDR_ECC_INT_STAT_CE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE |\n" + ">> +\t\t CDNS_DDR_ECC_INT_STAT_UE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE |\n" + ">> +\t\t CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN),\n" + ">\n" + "> Yah, unreadable mess that. Shorten pls.\n" + "\n" + "I do not see any better way to express it.\n" + "I could define a macro for the OR'ing mess sure.\n" + "\n" + ">\n" + ">> +\t io_base + CDNS_DDR_ECC_INT_CTRL);\n" + ">> +\n" + ">> +\treturn 0;\n" + ">> +}\n" + ">\n" + "> Thx.\n" + "\n" + "Thanks.\n" + "Best regards,\n" + "-- \n" + "Paul Louvel, Bootlin\n" + "Embedded Linux and Kernel engineering\n" https://bootlin.com -38aefd980aee7b7b0d854922ebf1f6c4207f65662f0e2cfc4500de26351edbe9 +7152b6c830ba6784b11e91695cc255693a15133893eadeb5c5f54c7820e7c147
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.