drivers: edac: Add EDAC support for Kryo CPU caches
Subsystems (2)
Hi Evan,
Thanks for the review comments.
Quoted 3 lines · Evan Green
No name?
Will add them in next spin.
Quoted 4 lines
A comment is warranted to indicate that
err_typeis indexed by the enum, as this would be easy to mess up in later changes.
Will use array index as suggested by Stephen.
Quoted 12 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+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.
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ +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.
Quoted 7 lines
Quoted 7 lines
(quoted diff) +7
-0+ +static void kryo_check_l1_l2_ecc(void *info) +{ + struct edac_device_ctl_info *edev_ctl = info; + u64 errxstatus; + u64 errxmisc; + int cpu;
Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ + 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
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation