[M] Change in coreboot[main]: soc/intel: Enable crashlog IP for both 32-bit and 64-bit architectures

[Subrata Banik (Code Review)]

Attention is currently required from: Angel Pons, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Nick Vaccaro, Pratikkumar V Prajapati, Sowmya Aralguppe, Tarun.

Subrata Banik has posted comments on this change by Sowmya Aralguppe. ( https://review.coreboot.org/c/coreboot/+/83106?usp=email )

Change subject: soc/intel: Enable crashlog IP for both 32-bit and 64-bit architectures
......................................................................

Patch Set 19:

(6 comments)

File src/soc/intel/common/block/crashlog/crashlog.c:

https://review.coreboot.org/c/coreboot/+/83106/comment/c40bae6a_f71e82a9?usp... :
PS15, Line 321: sizeof(uintptr_t)
> I have  changed dest_addr to be u32 *,IMHO that is right solution

isn't `u32 *dest_addr` also 8-byte in 64-bit arch?

if we decide to do DW by DW copy then we need to ensure that both src and dest addr are getting incremented by 4-bytes post copy.

Looking at `dest_addr` i felt this is okay as the declaration and increment both would be 4-bytes irrespective of 32-bit vs 64bit arch.

but looks like `src_addr` is not using fixed data type.

32-bit systems: uintptr_t is 32 bits (4 bytes)
64-bit systems: uintptr_t is 64 bits (8 bytes)

Let's say src_addr initially holds the value 0x1000.

32-bit: After src_addr += sizeof(uintptr_t);, it will hold the value 0x1004.
64-bit: After src_addr += sizeof(uintptr_t);, it will hold the value 0x1008.

can you please fix this problem?

File src/soc/intel/meteorlake/crashlog.c:

https://review.coreboot.org/c/coreboot/+/83106/comment/919333fb_73b727fb?usp... :
PS19, Line 352: cl_buffer
this is now combined dw0 and dw1, if so, then why are we calling it as `cl_buffer`.  IMO, a buffer is either array or pointer type

https://review.coreboot.org/c/coreboot/+/83106/comment/67839746_29932be0?usp... :
PS19, Line 353: */
space before ?

https://review.coreboot.org/c/coreboot/+/83106/comment/262f39a8_ac483e59?usp... :
PS19, Line 354: 		if (!(cl_buffer >> 32))
this can check if MSB is zero or not. Not sure what is the purpose ?

mostly to eliminate the zero based CL entries (which caused hang in MTL)

https://review.coreboot.org/c/coreboot/+/83106/comment/61e33fdd_4021116f?usp... :
PS19, Line 357: 	u32 dw0 = read32p(disc_tab_addr + cur_offset);
this is same as https://review.coreboot.org/c/coreboot/+/83106/19/src/soc/intel/meteorlake/c..., if so, how this is different than what we have inside `cl_buffer` LSB already ?

why no `if (!is_crashlog_data_valid(dw0))` checks?

File src/soc/intel/tigerlake/crashlog_lib.c:

https://review.coreboot.org/c/coreboot/+/83106/comment/e559107e_4f6c6048?usp... :
PS19, Line 165: bar_addr
we should check if this value is non-zero ?