Messages in this thread | | | Date | Fri, 13 Dec 2019 14:35:35 +0530 | From | Sai Prakash Ranjan <> | Subject | Re: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches |
| |
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
| |