[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block: [WIP]Add Intel common FAST_SPI code

[Barnali Sarkar (Code Review)]

Barnali Sarkar has posted comments on this change. ( https://review.coreboot.org/18557 )

Change subject: soc/intel/common/block: [WIP]Add Intel common FAST_SPI code
......................................................................


Patch Set 22:

(9 comments)

https://review.coreboot.org/#/c/18557/22//COMMIT_MSG
Commit Message:

PS22, Line 9: Create Intel Common FAST_SPI Controller code.
            : This code currently only contains the code for SPI initialization
            : required in Bootblock phase, which has the following programming -
            : * Get BIOS Rom Region Size
            : * Enable SPIBAR
            : * Disable the BIOS write protect so write commands are allowed
            : * Enable SPI Prefetching and Caching.
            : * SPI Controller register offsets in the common header fast_spi.h
            : More code will get added up in the subsequent phases.
> Please add blank lines between paragraphs, and around lists/enumerations.
ok will fix


https://review.coreboot.org/#/c/18557/22/src/soc/intel/common/block/fast_spi/Kconfig
File src/soc/intel/common/block/fast_spi/Kconfig:

Line 4: 	  Intel Processor common FAST_SPI support
> Should this be user selectable?
yes, this Kconfig can be selected from <soc>/Kconfig


https://review.coreboot.org/#/c/18557/22/src/soc/intel/common/block/fast_spi/fast_spi.c
File src/soc/intel/common/block/fast_spi/fast_spi.c:

PS22, Line 23: /* TODO: Make this function local(static) once the entire
             :  * flash_controller.c(for SKL) and flash_ctrlr.c(for APL) are
             :  * ported as a common library. Remove this comment once its done.
             :  */
> Please use the elaborate form, and add spaces before `(`.
ok will fix


PS22, Line 36: /* Bits 31-12 are the base address as per EDS for SPI,
             : 	 *  Don't care about  0-11 bit
             : 	 */
> That’s not the agreed upon concise style.
will fix


PS22, Line 154: /* TODO: Make this function local(static) once the entire
              :  * flash_controller.c(for SKL) and flash_ctrlr.c(for APL) are
              :  * ported as a common library. Remove this comment once its done.
              :  */
> Ditto.
will fix


https://review.coreboot.org/#/c/18557/22/src/soc/intel/common/block/include/intelblocks/fast_spi_def.h
File src/soc/intel/common/block/include/intelblocks/fast_spi_def.h:

PS22, Line 19: /* TODO: Change this file location to "block/fast_spi/fast_spi_def.h"
             :  * once the entire flash_controller.c(for SKL) and flash_ctrlr.c(for APL) are
             :  * ported as a common library. After that, no user of this fast_spi library will
             :  * require these register bitmap definitions to be exposed.
             :  *
             :  * Remove this comment once the change is done.
             :  */
> Ditto.
will fix


Line 26: 
> Maybe add a comment, in which datasheet (name, revision) these definitions 
ok


PS22, Line 151: /* Set_Strap Lock(SSL) Bit=0 */
              : #define SPIBAR_RESET_LOCK_ENABLE	1 /* Set_Strap Lock(SSL) Bit=1 */
              : 
              : /* Programmable options for Set STRAP MSG Control (0xF4) Register*/
              : #define SPIBAR_RESET_CTRL_SSMC		1 /* Set_Strap Mux Select(SSMS) Bit=1*/
> Please add a space before the `(`.
ok


PS22, Line 154: /* Programmable options for Set STRAP MSG Control (0xF4) Register*/
              : #define SPIBAR_RESET_CTRL_SSMC		1 /* Set_Strap Mux Select(SSMS) Bit=1*/
> Please add spaces at the end.
ok


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I046e3b30c8efb172851dd17f49565c9ec4cb38cb
Gerrit-PatchSet: 22
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Barnali Sarkar <barnali.sarkar 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: 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: Pratikkumar Prajapati <pratikkumar.v.prajapati at intel.corp-partner.google.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes