[coreboot-gerrit] Change in coreboot[master]: soc/intel/apollolake: Use common PCR module

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/18673 )

Change subject: soc/intel/apollolake: Use common PCR module
......................................................................


Patch Set 7:

(6 comments)

https://review.coreboot.org/#/c/18673/10/src/soc/intel/apollolake/gpio.c
File src/soc/intel/apollolake/gpio.c:

PS10, Line 123: r_read32(p
> I think I commented on this before. An OR variant should just be provided s
okay, we will create a pcr_or_32 wrapper to satisfy this


Line 187: 	 * returns - address of GPIO
> > 80
will fix


https://review.coreboot.org/#/c/18673/7/src/soc/intel/apollolake/include/soc/pcr_ids.h
File src/soc/intel/apollolake/include/soc/pcr_ids.h:

PS7, Line 33: /* RTC configuration */
            : #define R_PCH_PCR_RTC_CONF		0x3400
            : #define  B_PCH_PCR_RTC_CONF_UCMOS_EN	(1 << 2)
            : #define  B_PCH_PCR_RTC_CONF_LCMOS_LOCK	(1 << 3)
            : #define  B_PCH_PCR_RTC_CONF_UCMOS_LOCK	(1 << 4)
            : #define  B_PCH_PCR_RTC_CONF_RESERVED	(1 << 31)
> Why is this in here?
right now RTC enabloing upper bank code resides inside bootblock.c for small core and RTC code has dependency over PCR access. hence i have created dependencies such as with RTC.c we will take this code into rtc.c itself and delete from here 

https://review.coreboot.org/#/c/18700/5/src/soc/intel/apollolake/include/soc/pcr_ids.h


https://review.coreboot.org/#/c/18673/10/src/soc/intel/apollolake/include/soc/pcr_ids.h
File src/soc/intel/apollolake/include/soc/pcr_ids.h:

PS10, Line 30: /* ITSS PCRs*/
             : #define R_PCH_PCR_IPC0_CONF		0x3200
> This can just move to itss.c, right?
yes with common ITSS.c this code will move into respective soc and gets hidden from user header


PS10, Line 33: /* RTC configuration */
             : #define R_PCH_PCR_RTC_CONF		0x3400
             : #define  B_PCH_PCR_RTC_CONF_UCMOS_EN	(1 << 2)
             : #define  B_PCH_PCR_RTC_CONF_LCMOS_LOCK	(1 << 3)
             : #define  B_PCH_PCR_RTC_CONF_UCMOS_LOCK	(1 << 4)
             : #define  B_PCH_PCR_RTC_CONF_RESERVED	(1
> Do we need these here? Can't they live in the module using them? And then d
Yes, if you see RTC common patch. https://review.coreboot.org/#/c/18558/6/src/soc/intel/common/block/rtc/rtc.c
this definition moved into local file and user shouldn't aware of such details.


PS10, Line 40: 
             : /* Port Id lives in bits 23:16 and register offset lives in 15:0 of address. */
             : #define PCR_PORTID_SHIFT	1
> Why are these here? Are the PCR access methods changing? And why is REG_MAS
this mask and port id shift are common between big & small core, i would have kept these 2 into pcr.h but this macros are getting referred from ASL code in case of big core hence if i need to utilize those, i need to use compiler flag to guard PCR prototype from getting exposed in ASL code, hence i thought og kept those 2 line into specific soc header itself. And in other words its good for future socs as well to have PCR registers bit field out of common code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iacbf58dbd55bf3915676d875fcb484362d357a44
Gerrit-PatchSet: 7
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Subrata Banik <subrata.banik 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: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi 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