[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block: Add Intel common PCR support

[Subrata Banik (Code Review)]

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/pcr_ids.h
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/intelblocks/pcr.h
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


-- 
To view, visit https://review.coreboot.org/18669
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I78526a86b6d10f226570c08050327557e0bb2c78
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan at intel.com>
Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar at intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik at intel.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Shaunak Saha <shaunak.saha at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma at intel.com>
Gerrit-HasComments: Yes