linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: vnkgutta@codeaurora.org
To: Borislav Petkov <bp@alien8.de>
Cc: evgreen@chromium.org, robh@kernel.org, mchehab@kernel.org,
	linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
	Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	robh+dt@kernel.org, mark.rutland@arm.com,
	devicetree@vger.kernel.org, tsoni@codeaurora.org,
	ckadabi@codeaurora.org, rishabhb@codeaurora.org,
	swboyd@chromium.org, bjorn.andersson@linaro.org
Subject: Re: [PATCH v3 3/4] drivers: edac: Add EDAC driver support for QCOM SoCs
Date: Tue, 04 Sep 2018 16:19:26 -0700	[thread overview]
Message-ID: <ed22b1a9b894cd0ec323c7596dac27d6@codeaurora.org> (raw)
In-Reply-To: <20180830121137.GD20005@nazgul.tnic>

On 2018-08-30 05:11, Borislav Petkov wrote:
> On Tue, Aug 28, 2018 at 05:42:26PM -0700, Venkata Narendra Kumar Gutta 
> wrote:
>> From: Channagoud Kadabi <ckadabi@codeaurora.org>
>> 
>> Add error reporting driver for Single Bit Errors (SBEs) and Double Bit
>> Errors (DBEs). As of now, this driver supports error reporting for
>> Last Level Cache Controller (LLCC) of Tag RAM and Data RAM. Interrupts
>> are triggered when the errors happen in the cache, the driver handles
>> those interrupts and dumps the syndrome registers.
>> 
>> Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org>
>> Signed-off-by: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
>> Co-developed-by: Venkata Narendra Kumar Gutta 
>> <vnkgutta@codeaurora.org>
>> ---
>>  MAINTAINERS                        |   8 +
>>  drivers/edac/Kconfig               |  22 ++
>>  drivers/edac/Makefile              |   1 +
>>  drivers/edac/qcom_edac.c           | 421 
>> +++++++++++++++++++++++++++++++++++++
>>  include/linux/soc/qcom/llcc-qcom.h |  24 +++
>>  5 files changed, 476 insertions(+)
>>  create mode 100644 drivers/edac/qcom_edac.c
> 
> We'd also need an agreement who picks up the whole pile?

Andy should take care of it.
(Andy Gross <andy.gross@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT))

> 
> Those guys:
> 
> Andy Gross <andy.gross@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT)
> David Brown <david.brown@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT)
> 
> and I ACK the EDAC driver or I do and they ACK the soc pieces.
> 
> I have a hunch the prior would be easier...

You can ACK the EDAC driver, rest should be taken care by
Andy or Bjorn Andersson <bjorn.andersson@linaro.org>


> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0a23427..0bff713 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5227,6 +5227,14 @@ L:	linux-edac@vger.kernel.org
>>  S:	Maintained
>>  F:	drivers/edac/ti_edac.c
>> 
>> +EDAC-QUALCOMM
>> +M:	Channagoud Kadabi <ckadabi@codeaurora.org>
>> +M:	Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
>> +L:	linux-arm-msm@vger.kernel.org
>> +L:	linux-edac@vger.kernel.org
>> +S:	Maintained
>> +F:	drivers/edac/qcom_edac.c
>> +
>>  EDIROL UA-101/UA-1000 DRIVER
>>  M:	Clemens Ladisch <clemens@ladisch.de>
>>  L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
>> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
>> index 57304b2..df58957 100644
>> --- a/drivers/edac/Kconfig
>> +++ b/drivers/edac/Kconfig
>> @@ -460,4 +460,26 @@ config EDAC_TI
>>  	  Support for error detection and correction on the
>>            TI SoCs.
>> 
>> +config EDAC_QCOM
>> +	tristate "QCOM EDAC Controller"
>> +	depends on EDAC
>> +	help
>> +	  Support for error detection and correction on the
>> +	  QCOM SoCs.
>> +
>> +	  This driver reports Single Bit Errors (SBEs) and Double Bit Errors 
>> (DBEs).
>> +	  As of now, it supports error reporting for Last Level Cache 
>> Controller (LLCC)
>> +	  of Tag RAM and Data RAM.
>> +
>> +config EDAC_QCOM_LLCC
>> +	tristate "QCOM EDAC Controller for LLCC Cache"
>> +	depends on EDAC_QCOM && QCOM_LLCC
> 
> This is just silly: two EDAC config options for a single driver and 
> this
> second one only does:
> 
> #ifdef EDAC_QCOM_LLCC
>         { .compatible = "qcom,llcc-edac" },
> #endif
> 
> What for?!
> 
> You do this:
> 
> config EDAC_QCOM
> 	depends on <architecture which this driver runs on> && QCOM_LLCC
> 
> and that's it.
> 

Done, I'll update it the next patch set.

> ...
> 
>> +/* Dump Syndrome registers data for Tag RAM, Data RAM bit errors*/
>> +static int
>> +dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int 
>> err_type)
>> +{
>> +	struct llcc_edac_reg_data reg_data = edac_reg_data[err_type];
>> +	int err_cnt, err_ways, ret, i;
>> +	u32 synd_reg, synd_val;
>> +
>> +	for (i = 0; i < reg_data.reg_cnt; i++) {
>> +		synd_reg = reg_data.synd_reg + (i * 4);
>> +		ret = regmap_read(drv->regmap, drv->offsets[bank] + synd_reg,
>> +				  &synd_val);
>> +		if (ret)
>> +			goto clear;
> 
> <----- newline here.

OK

> 
>> +		edac_printk(KERN_CRIT, EDAC_LLCC, "%s: ECC_SYN%d: 0x%8x\n",
>> +			    reg_data.name, i, synd_val);
>> +	}
>> +
>> +	ret = regmap_read(drv->regmap,
>> +			  drv->offsets[bank] + reg_data.count_status_reg,
>> +			  &err_cnt);
>> +	if (ret)
>> +		goto clear;
>> +
>> +	err_cnt &= reg_data.count_mask;
>> +	err_cnt >>= reg_data.count_shift;
>> +	edac_printk(KERN_CRIT, EDAC_LLCC, "%s: error count: 0x%4x\n",
>> +		    reg_data.name, err_cnt);
>> +
>> +	ret = regmap_read(drv->regmap,
>> +			  drv->offsets[bank] + reg_data.ways_status_reg,
>> +			  &err_ways);
>> +	if (ret)
>> +		goto clear;
>> +
>> +	err_ways &= reg_data.ways_mask;
>> +	err_ways >>= reg_data.ways_shift;
>> +
>> +	edac_printk(KERN_CRIT, EDAC_LLCC, "%s: error ways: 0x%4x\n",
>> +		    reg_data.name, err_ways);
>> +
>> +clear:
>> +	return qcom_llcc_clear_error_status(err_type, drv);
>> +}
>> +
>> +static int
>> +dump_syn_reg(struct edac_device_ctl_info *edev_ctl, int err_type, u32 
>> bank)
>> +{
>> +	struct llcc_drv_data *drv = edev_ctl->pvt_info;
>> +	int ret;
>> +
>> +	ret = dump_syn_reg_values(drv, bank, err_type);
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (err_type) {
>> +	case LLCC_DRAM_CE:
>> +		edac_device_handle_ce(edev_ctl, 0, bank,
>> +				      "LLCC Data RAM correctable Error");
>> +		break;
>> +	case LLCC_DRAM_UE:
>> +		edac_device_handle_ue(edev_ctl, 0, bank,
>> +				      "LLCC Data RAM uncorrectable Error");
>> +		break;
>> +	case LLCC_TRAM_CE:
>> +		edac_device_handle_ce(edev_ctl, 0, bank,
>> +				      "LLCC Tag RAM correctable Error");
>> +		break;
>> +	case LLCC_TRAM_UE:
>> +		edac_device_handle_ue(edev_ctl, 0, bank,
>> +				      "LLCC Tag RAM uncorrectable Error");
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		edac_printk(KERN_CRIT, EDAC_LLCC, "Unexpected error type: %d\n",
>> +			    err_type);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static irqreturn_t
>> +llcc_ecc_irq_handler(int irq, void *edev_ctl)
>> +{
>> +	struct edac_device_ctl_info *edac_dev_ctl = edev_ctl;
>> +	struct llcc_drv_data *drv = edac_dev_ctl->pvt_info;
>> +	irqreturn_t irq_rc = IRQ_NONE;
>> +	u32 drp_error, trp_error, i;
>> +	bool irq_handled;
>> +	int ret;
>> +
>> +	/* Iterate over the banks and look for Tag RAM or Data RAM errors */
>> +	for (i = 0; i < drv->num_banks; i++) {
>> +		ret = regmap_read(drv->regmap,
>> +				  drv->offsets[i] + DRP_INTERRUPT_STATUS,
>> +				  &drp_error);
>> +
>> +		if (!ret && (drp_error & SB_ECC_ERROR)) {
>> +			edac_printk(KERN_CRIT, EDAC_LLCC,
>> +				    "Single Bit Error detected in Data Ram\n");
> 
> "RAM" not "Ram". You have a couple of those wrong spellings.

I'll correct that in the next patchset.

> 
> Otherwise it is starting to look real nice, good!

  reply	other threads:[~2018-09-04 23:19 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-29  0:42 [PATCH v3 0/4] Add EDAC driver for QCOM SoCs Venkata Narendra Kumar Gutta
2018-08-29  0:42 ` [PATCH v3 1/4] drivers: soc: Add broadcast base for Last Level Cache Controller (LLCC) Venkata Narendra Kumar Gutta
2018-08-29  0:42 ` [PATCH v3 2/4] drivers: soc: Add support to register LLCC EDAC driver Venkata Narendra Kumar Gutta
2018-08-29  0:42 ` [PATCH v3 3/4] drivers: edac: Add EDAC driver support for QCOM SoCs Venkata Narendra Kumar Gutta
2018-08-29 14:43   ` kbuild test robot
2018-08-29 14:43   ` [RFC PATCH] drivers: edac: edac_reg_data[] can be static kbuild test robot
2018-08-30 12:11   ` [PATCH v3 3/4] drivers: edac: Add EDAC driver support for QCOM SoCs Borislav Petkov
2018-09-04 23:19     ` vnkgutta [this message]
2018-08-29  0:42 ` [PATCH v3 4/4] dt-bindings: msm: Update documentation of qcom,llcc Venkata Narendra Kumar Gutta
2018-08-29 12:48   ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ed22b1a9b894cd0ec323c7596dac27d6@codeaurora.org \
    --to=vnkgutta@codeaurora.org \
    --cc=andy.gross@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=bp@alien8.de \
    --cc=ckadabi@codeaurora.org \
    --cc=david.brown@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=evgreen@chromium.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mchehab@kernel.org \
    --cc=rishabhb@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --cc=robh@kernel.org \
    --cc=swboyd@chromium.org \
    --cc=tsoni@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).