Subrata Banik has posted comments on this change. ( https://review.coreboot.org/18669 )
Change subject: soc/intel/common/block: Add Intel common PCR support ......................................................................
Patch Set 3:
(13 comments)
https://review.coreboot.org/#/c/18669/3/src/soc/intel/apollolake/include/soc... File src/soc/intel/apollolake/include/soc/pcr_ids.h:
PS3, Line 43: ~3
This should have parens
Done
https://review.coreboot.org/#/c/18669/3/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/pcr.h:
Line 19: #if !defined(__ASSEMBLER__) && !defined(__ACPI__)
Why would this header be included from assembler or ACPI code when it's all
Done
Line 23: int pcr_read32(u8 pid, u16 offset, u32 *outdata);
Can you just use uintX_t variants, please?
Done
https://review.coreboot.org/#/c/18669/3/src/soc/intel/common/block/pcr/pcr.c File src/soc/intel/common/block/pcr/pcr.c:
PS3, Line 42: u32
size_t for size
Done
PS3, Line 65: (u32 *)
cast not needed
Done
PS3, Line 70: (u32 *)
cast is wrong
Done
PS3, Line 75: (u32 *)
cast is wrong
Done
PS3, Line 93: (u32)
casts not needed here
Done
Line 109: pcr_read32(pid, offset, &data);
This is actually a property of inconsistent designed IPs on certain SoCs th
This is actually a property of inconsistent designed IPs on certain SoCs that have posted write semantics. Is every SoC supposed to be impacted by that going forward?
expectation may not be true for all future soc as well but its a safe mechanism to perform a read after write via IOSF-SB.
And why was the construction (including comment) completely dropped. Presumably you were using skylake as the basis for "everything common" ?
Apparently pcr library has been taken from big core code and after comparing small core IOSF read/write, its seems same as i have explained over commit message, both are performing access through PSF via IOSF_SB to PCRs. So, i don;t think there might be any reason to call it even skylake code or apollolake code, its a IP CR access mechanism. But i'm agree with you on SKL, we have seen some case where immediate read is needed after writing through SB registers. Hence we have added those "safety" piece of code in common library. I guess time taken for MMIO read should be marginal enough so, we should be good.
Lastly, if you already had the address calculated (as you should pcr_reg_address(). Then all you need to do is read. But here you are reading a 32-bit address regardless of size passed in. That's wrong as well.
good point, i have addressed
PS3, Line 116: u32
you should replace all these types with the 'indata' since the compiler kno
Done
PS3, Line 137: u32
size_t for size
Done
PS3, Line 141: data32 = 0;
Why does this need to be initialized?
Done
Line 150: status = pch_pcr_write(pid, offset, size, data32);
Just 'return pch_pcr_write()'?
Done