linux-edac.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches
@ 2019-12-13  9:05 Sai Prakash Ranjan
  0 siblings, 0 replies; 16+ messages in thread
From: Sai Prakash Ranjan @ 2019-12-13  9:05 UTC (permalink / raw)
  To: Evan Green
  Cc: Andy Gross, Bjorn Andersson, Mark Rutland, Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Borislav Petkov, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Robert Richter, linux-edac, linux-arm Mailing List, LKML,
	linux-arm-msm, Stephen Boyd, tsoni, psodagud

Hi Evan,

Thanks for the review comments.

On 2019-12-12 01:02, Evan Green wrote:
> 
> No name?
> 

Will add them in next spin.

> 
> A comment is warranted to indicate that err_type is indexed by the
> enum, as this would be easy to mess up in later changes.
> 

Will use array index as suggested by Stephen.

>> +static const char *get_error_msg(u64 errxstatus)
>> +{
>> +       const struct error_record *rec;
>> +       u32 errxstatus_serr;
>> +
>> +       errxstatus_serr = FIELD_GET(KRYO_ERRXSTATUS_SERR, errxstatus);
>> +
>> +       for (rec = serror_record; rec->error_code; rec++) {
> 
> It looks like you expect the table to be zero terminated, but it's
> not. Add the missing zero entry.
> 

Will add it.

>> +
>> +static inline void kryo_clear_error(u64 errxstatus)
>> +{
>> +       write_sysreg_s(errxstatus, SYS_ERXSTATUS_EL1);
>> +       isb();
> 
> Is the isb() necessary? If so, why not a dsb as well?
> 

We usually use isb() with cache and system control registers.
I do not see anything about isb or dsb mentioned in the TRM
for error record registers so it's probably OK to remove this.
James can help us here.

>> +
>> +static void kryo_check_l1_l2_ecc(void *info)
>> +{
>> +       struct edac_device_ctl_info *edev_ctl = info;
>> +       u64 errxstatus;
>> +       u64 errxmisc;
>> +       int cpu;
>> +
>> +       cpu = smp_processor_id();
>> +       /* We know record 0 is L1 and L2 */
>> +       write_sysreg_s(0, SYS_ERRSELR_EL1);
>> +       isb();
> 
> Another isb I'm not sure about. Is this meant to provide a barrier
> between ERRSELR and ERXSTATUS? Wouldn't that be dsb, not isb?
> 

Same as above.

I will repost with your comments addressed once I get more feedbacks 
from EDAC maintainers.

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-02-26 17:12 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1575529553.git.saiprakash.ranjan@codeaurora.org>
2019-12-05  9:53 ` [PATCH 1/2] dt-bindings: edac: Add DT bindings for Kryo EDAC Sai Prakash Ranjan
2019-12-18 23:37   ` Rob Herring
2019-12-19  6:50     ` Sai Prakash Ranjan
2019-12-19 13:58       ` Rob Herring
2019-12-19 14:48         ` Sai Prakash Ranjan
2020-01-15 18:48   ` James Morse
2020-01-24 14:21     ` Sai Prakash Ranjan
2020-02-26 17:12       ` James Morse
2019-12-05  9:53 ` [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches Sai Prakash Ranjan
     [not found] ` <0101016ed57a6311-e815485c-4b77-4342-a3de-203673941602-000000@us-west-2.amazonses.com>
2019-12-11 19:32   ` Evan Green
     [not found]     ` <5df16ebe.1c69fb81.6481f.a011@mx.google.com>
2019-12-13  5:31       ` Sai Prakash Ranjan
     [not found] ` <0101016ed57a6559-46c6c649-db28-4945-a11c-7441b8e9ac5b-000000@us-west-2.amazonses.com>
2019-12-30 11:50   ` Borislav Petkov
2020-01-13  5:44     ` Sai Prakash Ranjan
2020-01-15 18:49       ` James Morse
2020-01-24 14:52         ` Sai Prakash Ranjan
2019-12-13  9:05 Sai Prakash Ranjan

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).