From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v5,3/4] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC From: Borislav Petkov Message-Id: <20180905101935.GD2237@zn.tnic> Date: Wed, 5 Sep 2018 12:19:35 +0200 To: Manish Narani Cc: robh+dt@kernel.org, mark.rutland@arm.com, michal.simek@xilinx.com, mchehab@kernel.org, leoyang.li@nxp.com, amit.kucheria@linaro.org, olof@lixom.net, sgoud@xilinx.com, anirudh@xilinx.com, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-edac@vger.kernel.org List-ID: T24gRnJpLCBBdWcgMzEsIDIwMTggYXQgMDY6NTc6NDlQTSArMDUzMCwgTWFuaXNoIE5hcmFuaSB3 cm90ZToKPiBBZGQgRURBQyBFQ0Mgc3VwcG9ydCBmb3IgWnlucU1QIEREUkMgSVAuIEFsc28gYWRk IHN1cHBvcnQgZm9yIEVDQyBFcnJvcgo+IEluamVjdGlvbiBpbiBaeW5xTVAuIFRoZSBjb3JyZWN0 ZWQgYW5kIHVuY29ycmVjdGVkIGVycm9yIGludGVycnVwdHMKPiBzdXBwb3J0IGlzIGFkZGVkLiBU aGUgUm93LCBDb2x1bW4sIEJhbmssIEJhbmsgR3JvdXAgYW5kIFJhbmsgYml0cwo+IHBvc2l0aW9u cyBhcmUgZGV0ZXJtaW5lZCB2aWEgQWRkcmVzcyBNYXAgcmVnaXN0ZXJzIG9mIFN5bm9wc3lzIERE UkMuCj4gTWlub3IgaW5kZW50YXRpb24gY2hhbmdlcyBhcmUgYWxzbyBkb25lIGZvciBiZXR0ZXIg cmVhZGFiaWxpdHkuCj4gCj4gU2lnbmVkLW9mZi1ieTogTWFuaXNoIE5hcmFuaSA8bWFuaXNoLm5h cmFuaUB4aWxpbnguY29tPgo+IC0tLQo+ICBkcml2ZXJzL2VkYWMvS2NvbmZpZyAgICAgICAgIHwg ICAgMiArLQo+ICBkcml2ZXJzL2VkYWMvc3lub3BzeXNfZWRhYy5jIHwgMTA1NCArKysrKysrKysr KysrKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0KPiAgMiBmaWxlcyBjaGFuZ2VkLCA5Mzcg aW5zZXJ0aW9ucygrKSwgMTE5IGRlbGV0aW9ucygtKQoKLi4uCgo+IEBAIC0yMjIsMTEgKzQwOCw3 NyBAQCBzdGF0aWMgaW50IHN5bnBzX2VkYWNfZ2V0ZXJyb3JfaW5mbyhzdHJ1Y3Qgc3lucHNfZWRh Y19wcml2ICpwcml2KQo+ICB9Cj4gIAo+ICAvKioKPiArICogenlucW1wX2dldGVycm9yX2luZm8g LSBHZXQgdGhlIGN1cnJlbnQgRUNDIGVycm9yIGluZm8KPiArICogQHByaXY6CUREUiBtZW1vcnkg Y29udHJvbGxlciBwcml2YXRlIGluc3RhbmNlIGRhdGEKPiArICoKPiArICogR2V0IHRoZSBFQ0Mg ZXJyb3IgaW5mb3JtYXRpb24gZnJvbSB0aGUgRUNDIHJlZ2lzdGVycyBhbmQgY2xlYXIgdGhlIGVy cm9yCj4gKyAqIHN0YXR1cyBiaXRzIGluIHRoZSBFQ0MgcmVnaXN0ZXJzLgo+ICsgKgo+ICsgKiBS ZXR1cm46IDAgaWYgdGhlcmUgaXMgbm8gZXJyb3IsIG90aGVyd2lzZSByZXR1cm4gMQo+ICsgKi8K PiArc3RhdGljIGludCB6eW5xbXBfZ2V0ZXJyb3JfaW5mbyhzdHJ1Y3Qgc3lucHNfZWRhY19wcml2 ICpwcml2KQo+ICt7Cj4gKwl2b2lkIF9faW9tZW0gKmJhc2U7Cj4gKwlzdHJ1Y3Qgc3lucHNfZWNj X3N0YXR1cyAqcDsKPiArCXUzMiByZWd2YWwsIGNsZWFydmFsID0gMDsKPiArCj4gKwlpZiAoIXBy aXYpCj4gKwkJcmV0dXJuIDE7CgpTYW1lIGFzIGZvciB0aGUgcHJldmlvdXMgcGF0Y2g6IHdoeSBh cmUgeW91IHRlc3RpbmcgdGhpcyBzaW5jZSB5b3UncmUKZGVyZWZlcmVuY2luZyBpdCBiZWZvcmUg Y2FsbGluZyB0aGlzIGZ1bmN0aW9uPwoKLi4uCgo+IEBAIC0yNTgsMTAgKzUzNiw0NiBAQCBzdGF0 aWMgdm9pZCBzeW5wc19lZGFjX2hhbmRsZV9lcnJvcihzdHJ1Y3QgbWVtX2N0bF9pbmZvICptY2ks Cj4gIH0KPiAgCj4gIC8qKgo+ICsgKiBzeW5wc19lZGFjX2ludHJfaGFuZGxlciAtIEludGVycnVw dCBIYW5kbGVyIGZvciBFQ0MgaW50ZXJydXB0cwo+ICsgKiBAaXJxOiAgICAgICAgaXJxIG51bWJl cgo+ICsgKiBAZGV2X2lkOiAgICAgZGV2aWNlIGlkIHBvaW50ZXIKPiArICoKPiArICogVGhpcyBp cyB0aGUgSVNSIGNhbGxlZCBieSBFREFDIGNvcmUgaW50ZXJydXB0IHRocmVhZC4KClRoZXJlJ3Mg YW4gIkVEQUMgY29yZSBpbnRlcnJ1cHQgdGhyZWFkIj8hPyBUaGlzIGlzIHRoZSBmaXJzdCB0aW1l IEkgaGVhcgpvZiBpdC4KClRyeSBhZ2Fpbi4KCj4gKyAqIFRoaXMgaXMgdXNlZCB0byBjaGVjayBh bmQgcG9zdCBFQ0MgZXJyb3JzLgo+ICsgKgo+ICsgKiBSZXR1cm46IElSUV9OT05FLCBpZiBpbnRl cnJ1cHQgbm90IHNldCBvciBJUlFfSEFORExFRCBvdGhlcndpc2UuCj4gKyAqLwo+ICtzdGF0aWMg aXJxcmV0dXJuX3Qgc3lucHNfZWRhY19pbnRyX2hhbmRsZXIoaW50IGlycSwgdm9pZCAqZGV2X2lk KQo+ICt7Cj4gKwlzdHJ1Y3QgbWVtX2N0bF9pbmZvICptY2kgPSBkZXZfaWQ7Cj4gKwlzdHJ1Y3Qg c3lucHNfZWRhY19wcml2ICpwcml2ID0gbWNpLT5wdnRfaW5mbzsKPiArCWNvbnN0IHN0cnVjdCBz eW5wc19wbGF0Zm9ybV9kYXRhICpwX2RhdGEgPSBwcml2LT5wX2RhdGE7Cj4gKwlpbnQgc3RhdHVz LCByZWd2YWw7Cj4gKwo+ICsJcmVndmFsID0gcmVhZGwocHJpdi0+YmFzZWFkZHIgKyBERFJfUU9T X0lSUV9TVEFUX09GU1QpOwo+ICsJcmVndmFsICY9IChERFJfUU9TQ0VfTUFTSyB8IEREUl9RT1NV RV9NQVNLKTsKPiArCWlmICghKHJlZ3ZhbCAmIEVDQ19DRV9VRV9JTlRSX01BU0spKQo+ICsJCXJl dHVybiBJUlFfTk9ORTsKPiArCj4gKwlzdGF0dXMgPSBwX2RhdGEtPmdldGVycm9yX2luZm8ocHJp dik7Cj4gKwlpZiAoc3RhdHVzKQo+ICsJCXJldHVybiBJUlFfTk9ORTsKPiArCj4gKwlwcml2LT5j ZV9jbnQgKz0gcHJpdi0+c3RhdC5jZV9jbnQ7Cj4gKwlwcml2LT51ZV9jbnQgKz0gcHJpdi0+c3Rh dC51ZV9jbnQ7Cj4gKwlzeW5wc19lZGFjX2hhbmRsZV9lcnJvcihtY2ksICZwcml2LT5zdGF0KTsK PiArCj4gKwllZGFjX2RiZygzLCAiVG90YWwgZXJyb3IgY291bnQgY2UgJWQgdWUgJWRcbiIsCgoi Y2UiIGFuZCAidWUiIGFyZSBhbHNvIGFiYnJldmlhdGlvbnMgYW5kIHNob3VsZCBiZSBpbiBjYXBz LgoKLi4uCgo+ICtzdGF0aWMgREVWSUNFX0FUVFJfUlcoaW5qZWN0X2RhdGFfZXJyb3IpOwo+ICtz dGF0aWMgREVWSUNFX0FUVFJfUlcoaW5qZWN0X2RhdGFfcG9pc29uKTsKPiArCj4gK3N0YXRpYyBp bnQgc3lucHNfZWRhY19jcmVhdGVfc3lzZnNfYXR0cmlidXRlcyhzdHJ1Y3QgbWVtX2N0bF9pbmZv ICptY2kpCj4gK3sKPiArCWludCByYzsKPiArCj4gKwlyYyA9IGRldmljZV9jcmVhdGVfZmlsZSgm bWNpLT5kZXYsICZkZXZfYXR0cl9pbmplY3RfZGF0YV9lcnJvcik7Cj4gKwlpZiAocmMgPCAwKQo+ ICsJCXJldHVybiByYzsKPiArCXJjID0gZGV2aWNlX2NyZWF0ZV9maWxlKCZtY2ktPmRldiwgJmRl dl9hdHRyX2luamVjdF9kYXRhX3BvaXNvbik7Cj4gKwlpZiAocmMgPCAwKQo+ICsJCXJldHVybiBy YzsKPiArCXJldHVybiAwOwo+ICt9CgpJIHRoaW5rIEknbSBoYXZpbmcgYSBkZWphLXZ1OgoKTGFz dCB0aW1lIEkgc2FpZDoKCiJNb3JlIGltcG9ydGFudGx5LCB5b3UgbmVlZCB0byBwdXQgYWxsIHRo YXQgaW5qZWN0aW9uIGZ1bmN0aW9uYWxpdHkKYmVoaW5kIENPTkZJR19FREFDX0RFQlVHIC0geW91 IGRvbid0IHdhbnQgdG8gZXhwb3NlIHRoZSBpbmplY3Rpb24KY2FwYWJpbGl0aWVzIG9uIGEgcHJv ZHVjdGlvbiBtYWNoaW5lLiIKCmFuZCB5b3Ugc2FpZDoKCiJJIGFncmVlLiBJIHdpbGwgdXBkYXRl IHRoZSBzYW1lIGJ5IGtlZXBpbmcgdGhlIGFib3ZlIG1lbnRpb25lZCBtYWNyby4iCgpCdXQgbWF5 YmUgeW91J3ZlIG1pc3VuZGVyc3Rvb2QgbWUuCgpHcmVwIHRoZSBvdGhlciBFREFDIGRyaXZlcnMg dG8gZmluZCBvdXQgaG93IHRvIGhpZGUgdGhlIGluamVjdGlvbgpmdW5jdGlvbmFsaXR5IGJlaGlu ZCBDT05GSUdfRURBQ19ERUJVRy4KCkFuZCBtYXliZSB0aGlzIHBhdGNoIGlzIGJlY29taW5nIGh1 dWdlIGFuZCB0b28gdW53aWVsZHkgdG8gcmV2aWV3CnByb3Blcmx5IGFuZCBmb3IgeW91IHRvIGlu Y29ycG9yYXRlIGFsbCB0aGUgZmVlZGJhY2suCgpUaGVyZWZvcmUsIHBsZWFzZSBzcGxpdCBpdCBp biBzaW5nbGUgcGF0Y2hlcywgZWFjaCBvZiB3aGljaCBpcyBkb2luZwpkaWZmZXJlbnQgdGhpbmdz OgoKMS4gZml4dXAvY2hhbmdlIGNvbW1lbnRzCjIuIGFkZCBkZWZpbmVzCjMuIGFkZCBmdW5jdGlv bmFsaXR5IFgKNC4gYWRkIGZ1bmN0aW9uYWxpdHkgWQouLi4KNS4gYWRkIGluamVjdGlvbgo2LiB0 aWUgaXQgYWxsIHRvZ2V0aGVyCgpBcHBseSBzb21lIGNvbW1vbiBzZW5zZSBhbmQgcHV0IHlvdXJz ZWxmIGluIHRoZSByZXZpZXdlcidzIHNob2VzIHdoZW4KZG9pbmcgc28uCgpUaHguCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: bp@alien8.de (Borislav Petkov) Date: Wed, 5 Sep 2018 12:19:35 +0200 Subject: [PATCH v5 3/4] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC In-Reply-To: <1535722070-10394-4-git-send-email-manish.narani@xilinx.com> References: <1535722070-10394-1-git-send-email-manish.narani@xilinx.com> <1535722070-10394-4-git-send-email-manish.narani@xilinx.com> Message-ID: <20180905101935.GD2237@zn.tnic> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Aug 31, 2018 at 06:57:49PM +0530, Manish Narani wrote: > Add EDAC ECC support for ZynqMP DDRC IP. Also add support for ECC Error > Injection in ZynqMP. The corrected and uncorrected error interrupts > support is added. The Row, Column, Bank, Bank Group and Rank bits > positions are determined via Address Map registers of Synopsys DDRC. > Minor indentation changes are also done for better readability. > > Signed-off-by: Manish Narani > --- > drivers/edac/Kconfig | 2 +- > drivers/edac/synopsys_edac.c | 1054 +++++++++++++++++++++++++++++++++++++----- > 2 files changed, 937 insertions(+), 119 deletions(-) ... > @@ -222,11 +408,77 @@ static int synps_edac_geterror_info(struct synps_edac_priv *priv) > } > > /** > + * zynqmp_geterror_info - Get the current ECC error info > + * @priv: DDR memory controller private instance data > + * > + * Get the ECC error information from the ECC registers and clear the error > + * status bits in the ECC registers. > + * > + * Return: 0 if there is no error, otherwise return 1 > + */ > +static int zynqmp_geterror_info(struct synps_edac_priv *priv) > +{ > + void __iomem *base; > + struct synps_ecc_status *p; > + u32 regval, clearval = 0; > + > + if (!priv) > + return 1; Same as for the previous patch: why are you testing this since you're dereferencing it before calling this function? ... > @@ -258,10 +536,46 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci, > } > > /** > + * synps_edac_intr_handler - Interrupt Handler for ECC interrupts > + * @irq: irq number > + * @dev_id: device id pointer > + * > + * This is the ISR called by EDAC core interrupt thread. There's an "EDAC core interrupt thread"?!? This is the first time I hear of it. Try again. > + * This is used to check and post ECC errors. > + * > + * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise. > + */ > +static irqreturn_t synps_edac_intr_handler(int irq, void *dev_id) > +{ > + struct mem_ctl_info *mci = dev_id; > + struct synps_edac_priv *priv = mci->pvt_info; > + const struct synps_platform_data *p_data = priv->p_data; > + int status, regval; > + > + 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 = p_data->geterror_info(priv); > + if (status) > + return IRQ_NONE; > + > + priv->ce_cnt += priv->stat.ce_cnt; > + priv->ue_cnt += priv->stat.ue_cnt; > + synps_edac_handle_error(mci, &priv->stat); > + > + edac_dbg(3, "Total error count ce %d ue %d\n", "ce" and "ue" are also abbreviations and should be in caps. ... > +static DEVICE_ATTR_RW(inject_data_error); > +static DEVICE_ATTR_RW(inject_data_poison); > + > +static int synps_edac_create_sysfs_attributes(struct mem_ctl_info *mci) > +{ > + int rc; > + > + rc = device_create_file(&mci->dev, &dev_attr_inject_data_error); > + if (rc < 0) > + return rc; > + rc = device_create_file(&mci->dev, &dev_attr_inject_data_poison); > + if (rc < 0) > + return rc; > + return 0; > +} I think I'm having a deja-vu: Last time I said: "More importantly, you need to put all that injection functionality behind CONFIG_EDAC_DEBUG - you don't want to expose the injection capabilities on a production machine." and you said: "I agree. I will update the same by keeping the above mentioned macro." But maybe you've misunderstood me. Grep the other EDAC drivers to find out how to hide the injection functionality behind CONFIG_EDAC_DEBUG. And maybe this patch is becoming huuge and too unwieldy to review properly and for you to incorporate all the feedback. Therefore, please split it in single patches, each of which is doing different things: 1. fixup/change comments 2. add defines 3. add functionality X 4. add functionality Y ... 5. add injection 6. tie it all together Apply some common sense and put yourself in the reviewer's shoes when doing so. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Borislav Petkov Subject: Re: [PATCH v5 3/4] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC Date: Wed, 5 Sep 2018 12:19:35 +0200 Message-ID: <20180905101935.GD2237@zn.tnic> References: <1535722070-10394-1-git-send-email-manish.narani@xilinx.com> <1535722070-10394-4-git-send-email-manish.narani@xilinx.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Return-path: Content-Disposition: inline In-Reply-To: <1535722070-10394-4-git-send-email-manish.narani@xilinx.com> Sender: linux-kernel-owner@vger.kernel.org To: Manish Narani Cc: robh+dt@kernel.org, mark.rutland@arm.com, michal.simek@xilinx.com, mchehab@kernel.org, leoyang.li@nxp.com, amit.kucheria@linaro.org, olof@lixom.net, sgoud@xilinx.com, anirudh@xilinx.com, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-edac@vger.kernel.org List-Id: devicetree@vger.kernel.org On Fri, Aug 31, 2018 at 06:57:49PM +0530, Manish Narani wrote: > Add EDAC ECC support for ZynqMP DDRC IP. Also add support for ECC Error > Injection in ZynqMP. The corrected and uncorrected error interrupts > support is added. The Row, Column, Bank, Bank Group and Rank bits > positions are determined via Address Map registers of Synopsys DDRC. > Minor indentation changes are also done for better readability. > > Signed-off-by: Manish Narani > --- > drivers/edac/Kconfig | 2 +- > drivers/edac/synopsys_edac.c | 1054 +++++++++++++++++++++++++++++++++++++----- > 2 files changed, 937 insertions(+), 119 deletions(-) ... > @@ -222,11 +408,77 @@ static int synps_edac_geterror_info(struct synps_edac_priv *priv) > } > > /** > + * zynqmp_geterror_info - Get the current ECC error info > + * @priv: DDR memory controller private instance data > + * > + * Get the ECC error information from the ECC registers and clear the error > + * status bits in the ECC registers. > + * > + * Return: 0 if there is no error, otherwise return 1 > + */ > +static int zynqmp_geterror_info(struct synps_edac_priv *priv) > +{ > + void __iomem *base; > + struct synps_ecc_status *p; > + u32 regval, clearval = 0; > + > + if (!priv) > + return 1; Same as for the previous patch: why are you testing this since you're dereferencing it before calling this function? ... > @@ -258,10 +536,46 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci, > } > > /** > + * synps_edac_intr_handler - Interrupt Handler for ECC interrupts > + * @irq: irq number > + * @dev_id: device id pointer > + * > + * This is the ISR called by EDAC core interrupt thread. There's an "EDAC core interrupt thread"?!? This is the first time I hear of it. Try again. > + * This is used to check and post ECC errors. > + * > + * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise. > + */ > +static irqreturn_t synps_edac_intr_handler(int irq, void *dev_id) > +{ > + struct mem_ctl_info *mci = dev_id; > + struct synps_edac_priv *priv = mci->pvt_info; > + const struct synps_platform_data *p_data = priv->p_data; > + int status, regval; > + > + 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 = p_data->geterror_info(priv); > + if (status) > + return IRQ_NONE; > + > + priv->ce_cnt += priv->stat.ce_cnt; > + priv->ue_cnt += priv->stat.ue_cnt; > + synps_edac_handle_error(mci, &priv->stat); > + > + edac_dbg(3, "Total error count ce %d ue %d\n", "ce" and "ue" are also abbreviations and should be in caps. ... > +static DEVICE_ATTR_RW(inject_data_error); > +static DEVICE_ATTR_RW(inject_data_poison); > + > +static int synps_edac_create_sysfs_attributes(struct mem_ctl_info *mci) > +{ > + int rc; > + > + rc = device_create_file(&mci->dev, &dev_attr_inject_data_error); > + if (rc < 0) > + return rc; > + rc = device_create_file(&mci->dev, &dev_attr_inject_data_poison); > + if (rc < 0) > + return rc; > + return 0; > +} I think I'm having a deja-vu: Last time I said: "More importantly, you need to put all that injection functionality behind CONFIG_EDAC_DEBUG - you don't want to expose the injection capabilities on a production machine." and you said: "I agree. I will update the same by keeping the above mentioned macro." But maybe you've misunderstood me. Grep the other EDAC drivers to find out how to hide the injection functionality behind CONFIG_EDAC_DEBUG. And maybe this patch is becoming huuge and too unwieldy to review properly and for you to incorporate all the feedback. Therefore, please split it in single patches, each of which is doing different things: 1. fixup/change comments 2. add defines 3. add functionality X 4. add functionality Y ... 5. add injection 6. tie it all together Apply some common sense and put yourself in the reviewer's shoes when doing so. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.