Attention is currently required from: Angel Pons, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Nick Vaccaro, Paul Menzel, Pratikkumar V Prajapati, Subrata Banik, Tarun.
Sowmya Aralguppe has posted comments on this change by Sowmya Aralguppe. ( https://review.coreboot.org/c/coreboot/+/83106?usp=email )
Change subject: soc/intel: Adapt crashlog IP to also support 64-bit ......................................................................
Patch Set 21:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83106/comment/016116e8_bfcb43ac?usp... : PS19, Line 7: Enable crashlog IP for both 32-bit and 64-bit architectures
Adapt crashlog IP to also support 64-bit
Done
https://review.coreboot.org/c/coreboot/+/83106/comment/ce53f46a_98bd12b1?usp... : PS19, Line 10: future generation SoCs
Aren’t current SoCs already 64-bit?
From PTL onwards FSP will support only 64bit architecture .I think LNL had both 32 and 64 bit support
https://review.coreboot.org/c/coreboot/+/83106/comment/f5d660e3_cd394946?usp... : PS19, Line 12:
Please summarize the changes. […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/83106/comment/41ea6fc8_25c8b91f?usp... : PS19, Line 14: tested
Could you please elaborate on what was tested? As per my review comments, I doubt that reading Crash […]
I am testing and comparing the boot logs - local copy for testing has src address incremented by 4 .
File src/soc/intel/common/block/crashlog/crashlog.c:
https://review.coreboot.org/c/coreboot/+/83106/comment/01c789f6_3819329e?usp... : PS15, Line 321: sizeof(uintptr_t)
no sorry - in my local copy src_address is incremented by 4 since we are reading dw and it is uintp […]
Done
File src/soc/intel/common/block/crashlog/crashlog.c:
https://review.coreboot.org/c/coreboot/+/83106/comment/299ef9cb_1a8090e8?usp... : PS19, Line 318: /* DW by DW copy: byte access to PMC SRAM not allowed */
Try adding this, then build for x86_64: […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/83106/comment/b6e82152_d4b37e75?usp... : PS19, Line 321: src_addr += sizeof(uintptr_t);
On 64-bit builds, this can't possibly work. `read32p()` always reads 4 bytes, but the address gets incremented by `sizeof(uintptr_t)`, which is 4 bytes on 32-bit builds but 8 bytes on 64-bit builds.
Please change:
src_addr += sizeof(u32);
I had uploaded the wrong patchset sorry - In my local copy i had incremented by 4 while testing
File src/soc/intel/meteorlake/crashlog.c:
https://review.coreboot.org/c/coreboot/+/83106/comment/d8c5960a_77b00f35?usp... : PS19, Line 354: if (!(cl_buffer >> 32))
it is cl buffer because it is same as cpu_cl_disc_tab.buffers[i]. […]
Done
https://review.coreboot.org/c/coreboot/+/83106/comment/5483d56f_b029875f?usp... : PS19, Line 357: u32 dw0 = read32p(disc_tab_addr + cur_offset);
Crashlog_data_valid This is done for reading the header in line 342 […]
Done
-
Sowmya Aralguppe (Code Review)