Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH v5 1/1] EDAC, mellanox: Add ECC support for BlueField DDR4 | From | James Morse <> | Date | Fri, 21 Jun 2019 16:45:10 +0100 |
| |
Hi Shravan,
On 17/06/2019 19:41, Shravan Kumar Ramani wrote: > Add ECC support for Mellanox BlueField SoC DDR controller.
> This requires SMC to the running Arm Trusted Firmware to report > what is the current memory configuration.
> ---
For a single patch, the changelog would usually be in here:
> Changes since v4: > Shorten long macro names. > > Changes since v3: > Replace bitfields with shifts and masks.
(but not all of them!)
> Make use of SZ_ and PAGE_ macros. > Read DIMM count from the ACPI table instead of hard-coding the info in > the driver.
> diff --git a/drivers/edac/bluefield_edac.c b/drivers/edac/bluefield_edac.c > new file mode 100644 > index 0000000..d4a29ba > --- /dev/null > +++ b/drivers/edac/bluefield_edac.c
> +#define MLXBF_SYNDROM 0x35c > +#define MLXBF_SYNDROM__DERR_SHIFT 0 > +#define MLXBF_SYNDROM__DERR_MASK 0x1 > +#define MLXBF_SYNDROM__SERR_SHIFT 1 > +#define MLXBF_SYNDROM__SERR_MASK 0x1 > +#define MLXBF_SYNDROM__SYN_SHIFT 16 > +#define MLXBF_SYNDROM__SYN_MASK 0x3ff > + > +#define MLXBF_ADD_INFO 0x364 > +#define MLXBF_ADD_INFO__ERR_PRANK_SHIFT 8 > +#define MLXBF_ADD_INFO__ERR_PRANK_MASK 0x3
Thanks for changing these to shifts/masks, this is one fewer dependency on the compiler's implementation choices being exactly as you expect.
> +#define MLXBF_EDAC_MAX_DIMM_PER_MC 2 > +#define MLXBF_EDAC_ERROR_GRAIN 8 > + > +/* > + * Request MLNX_SIP_GET_DIMM_INFO > + * > + * Retrieve information about DIMM on a certain slot. > + * > + * Call register usage: > + * a0: MLNX_SIP_GET_DIMM_INFO > + * a1: (Memory controller index) << 16 | (Dimm index in memory controller) > + * a2-7: not used. > + * > + * Return status: > + * a0: dimm_info_smc union defined below describing the DIMM. > + * a1-3: not used. > + */ > +#define MLNX_SIP_GET_DIMM_INFO 0x82000008 > + > +/* Format for the SMC response about the memory information */ > +union dimm_info_smc { > + struct { > + unsigned long size_GB : 16; > + unsigned long is_rdimm : 1; > + unsigned long is_lrdimm : 1; > + unsigned long is_nvdimm : 1; > + unsigned long __reserved0 : 2; > + unsigned long ranks : 3; > + unsigned long package_X : 8; /* Bits per memory package */ > + unsigned long __reserved1 : 32; > + }; > + unsigned long val; > +};
But this is still fragile. The compiler is allowed to pack this thing as it likes. You must have your firmware and kernel built with the same compiler. If firmware/kernel differ in endian-ness (which is entirely possible on arm64), this could go haywire. (the value is only passed via registers, which might save you, but its not clear to me)
> +/* > + * Gather the ECC information from the External Memory Interface registers > + * and report it to the edac handler. > + */ > +static void bluefield_gather_report_ecc(struct mem_ctl_info *mci, > + int error_cnt, > + int is_single_ecc) > +{ > + struct bluefield_edac_priv *priv = mci->pvt_info; > + u32 dram_additional_info, err_prank; > + u32 ecc_latch_select = 0, dram_syndrom = 0, serr, derr, syndrom; > + enum hw_event_mc_err_type ecc_type; > + unsigned long ecc_dimm_addr; > + int ecc_dimm; > + u32 edea0; > + u32 edea1; > + > + ecc_type = is_single_ecc ? HW_EVENT_ERR_CORRECTED : > + HW_EVENT_ERR_UNCORRECTED; > + > + /* > + * Tell the External Memory Interface to populate the relevant > + * registers with information about the last ECC error occurrence. > + */ > + ecc_latch_select |= 1 << MLXBF_ECC_LATCH_SEL__START_SHIFT; > + writel(ecc_latch_select, priv->emi_base + MLXBF_ECC_LATCH_SEL); > + > + /* > + * Verify that the ECC reported info in the registers is of the > + * same type as the one asked to report. If not, just report the > + * error without the detailed information. > + */ > + dram_syndrom = readl(priv->emi_base + MLXBF_SYNDROM); > + serr = (dram_syndrom >> MLXBF_SYNDROM__SERR_SHIFT) & > + MLXBF_SYNDROM__SERR_MASK; > + derr = (dram_syndrom >> MLXBF_SYNDROM__DERR_SHIFT) & > + MLXBF_SYNDROM__DERR_MASK; > + syndrom = (dram_syndrom >> MLXBF_SYNDROM__SYN_SHIFT) & > + MLXBF_SYNDROM__SYN_MASK; > + > + if ((is_single_ecc && !serr) || (!is_single_ecc && !derr)) { > + edac_mc_handle_error(ecc_type, mci, error_cnt, 0, 0, 0, > + 0, 0, -1, mci->ctl_name, ""); > + return; > + } > + > + dram_additional_info = readl(priv->emi_base + MLXBF_ADD_INFO); > + err_prank = (dram_additional_info >> MLXBF_ADD_INFO__ERR_PRANK_SHIFT) & > + MLXBF_ADD_INFO__ERR_PRANK_MASK;
> + ecc_dimm = err_prank >= 2 && priv->dimm_ranks[0] <= 2;
You are relying on this only generating 0 or 1, whereas the expression gives false/true. The compiler may pick a value of true that isn't 1.
Something like?: | ecc_dimm = err_prank; | if (ecc_dimm > priv->dimm_ranks[0]) | ecc_dimm = 0;
> + edea0 = readl(priv->emi_base + MLXBF_ERR_ADDR_0); > + edea1 = readl(priv->emi_base + MLXBF_ERR_ADDR_1); > + > + ecc_dimm_addr = ((unsigned long)edea1 << 32) | edea0; > + > + edac_mc_handle_error(ecc_type, mci, error_cnt,
> + PFN_DOWN(ecc_dimm_addr), ecc_dimm_addr & PAGE_MASK,
Don't you want ~PAGE_MASK here? arm64 has: | #define PAGE_MASK (~(PAGE_SIZE-1))
so this is all the high bits.
I've recently learnt mm.h has offset_in_page() which is going to be the best thing to use here.
> + syndrom, ecc_dimm, 0, 0, mci->ctl_name, ""); > +}
> +/* Initialize the DIMMs information for the given memory controller. */ > +static void bluefield_edac_init_dimms(struct mem_ctl_info *mci) > +{ > + struct bluefield_edac_priv *priv = mci->pvt_info; > + int mem_ctrl_idx = mci->mc_idx; > + union dimm_info_smc smc_info; > + struct dimm_info *dimm; > + unsigned long smc_arg; > + int is_empty = 1; > + int i; > + > + for (i = 0; i < priv->dimm_per_mc; i++) { > + dimm = mci->dimms[i]; > + > + smc_arg = mem_ctrl_idx << 16 | i; > + smc_info = (union dimm_info_smc) { > + .val = smc_call1(MLNX_SIP_GET_DIMM_INFO, smc_arg), > + }; > + if (!smc_info.size_GB) { > + dimm->mtype = MEM_EMPTY; > + continue; > + } > + > + is_empty = 0; > + > + dimm->edac_mode = EDAC_SECDED; > + > + if (smc_info.is_nvdimm) > + dimm->mtype = MEM_NVDIMM; > + else if (smc_info.is_lrdimm) > + dimm->mtype = MEM_LRDDR4; > + else if (smc_info.is_rdimm) > + dimm->mtype = MEM_RDDR4; > + else > + dimm->mtype = MEM_DDR4; > + > + dimm->nr_pages = smc_info.size_GB * (SZ_1G / PAGE_SIZE); > + dimm->grain = MLXBF_EDAC_ERROR_GRAIN; > + > + /* Mem controller for BlueField only supports x4, x8 and x16 */ > + if (smc_info.package_X == 4) > + dimm->dtype = DEV_X4; > + else if (smc_info.package_X == 8) > + dimm->dtype = DEV_X8; > + else if (smc_info.package_X == 16) > + dimm->dtype = DEV_X16; > + else > + dimm->dtype = DEV_UNKNOWN; > + > + priv->dimm_ranks[i] = smc_info.ranks;
dimm_ranks[] has size MLXBF_EDAC_MAX_DIMM_PER_MC, but dimm_per_mc is a number from firmware. Where do you check dimm_per_mc < MLXBF_EDAC_MAX_DIMM_PER_MC?
> + } > + > + if (is_empty) > + mci->edac_cap = EDAC_FLAG_NONE; > + else > + mci->edac_cap = EDAC_FLAG_SECDED; > +}
> +static int bluefield_edac_mc_probe(struct platform_device *pdev) > +{ > + struct bluefield_edac_priv *priv; > + struct device *dev = &pdev->dev; > + struct edac_mc_layer layers[1]; > + struct mem_ctl_info *mci; > + struct resource *emi_res; > + unsigned int mc_idx, dimm_count; > + int rc, ret;
> + /* Read the MSS (Memory SubSystem) index from ACPI table. */ > + if (device_property_read_u32(dev, "mss_number", &mc_idx)) { > + dev_warn(dev, "bf_edac: MSS number unknown\n"); > + return -EINVAL; > + } > + > + /* Read the MSS (Memory SubSystem) index from ACPI table. */ > + if (device_property_read_u32(dev, "dimm_per_mc", &dimm_count)) { > + dev_warn(dev, "bf_edac: DIMMs per MC unknown\n"); > + return -EINVAL; > + }
These two have the same comment, but do different things.
> + emi_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!emi_res) > + return -EINVAL; > + > + layers[0].type = EDAC_MC_LAYER_SLOT; > + layers[0].size = dimm_count; > + layers[0].is_virt_csrow = true; > + > + mci = edac_mc_alloc(mc_idx, ARRAY_SIZE(layers), layers, sizeof(*priv)); > + if (!mci) > + return -ENOMEM; > + > + priv = mci->pvt_info; > + > + priv->dimm_per_mc = dimm_count; > + priv->emi_base = devm_ioremap_resource(dev, emi_res); > + if (IS_ERR(priv->emi_base)) { > + dev_err(dev, "failed to map EMI IO resource\n"); > + ret = PTR_ERR(priv->emi_base); > + goto err; > + } > + > + mci->pdev = dev; > + mci->mtype_cap = MEM_FLAG_DDR4 | MEM_FLAG_RDDR4 | > + MEM_FLAG_LRDDR4 | MEM_FLAG_NVDIMM; > + mci->edac_ctl_cap = EDAC_FLAG_SECDED; > + > + mci->mod_name = DRIVER_NAME; > + mci->ctl_name = "BlueField_Memory_Controller"; > + mci->dev_name = dev_name(dev); > + mci->edac_check = bluefield_edac_check; > + > + /* Initialize mci with the actual populated DIMM information. */ > + bluefield_edac_init_dimms(mci); > + > + platform_set_drvdata(pdev, mci); > + > + /* Register with EDAC core */ > + rc = edac_mc_add_mc(mci); > + if (rc) { > + dev_err(dev, "failed to register with EDAC core\n"); > + ret = rc; > + goto err; > + } > + > + /* Only POLL mode supported so far. */ > + edac_op_state = EDAC_OPSTATE_POLL;
> + return 0; > + > +err: > + edac_mc_free(mci); > + > + return ret; > + > +}
With the MLXBF_EDAC_MAX_DIMM_PER_MC check and offset_in_page()/~PAGE_MASK:
Reviewed-by: James Morse <james.morse@arm.com>
Thanks,
James
[0] https://static.docs.arm.com/den0028/b/ARM_DEN0028B_SMC_Calling_Convention.pdf
| ![\](/images/icornerr.gif) |