[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 18:

(4 comments)

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

Line 22: 
> Why is the below function exposed?
Currently, since I have only pushed the SPI library upto bootblock, I need to make this function as GLOBAL since there are other places (like flash_controller.c, flash_ctrlr.c files), which calls this function to get the FAST_SPI BAR.
So, I have made it non-static to avoid compilation issues as of now.
In future, when the complete library will be formed and when we wont require this function from user side, we can make it Local to this file. I will put a /*TODO*/ comment here for now, to ensure that this gets addressed in future.


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

Line 49: 
> Why is there a new line here and below but there isn't one in the bios inte
will fix this.


PS18, Line 157: device_t dev
> This feedback still isn't addressed.
will fix in the next patch-set.


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

Line 17: #define SOC_INTEL_COMMON_BLOCK_FAST_SPI_DEF_H
> Why does this file need to be in include/intelblocks/fast_spi_def.h?
I have included it here since, in flash_controller.c file, we have lot of these MACRO definitions used as of now (which will be part of this library after we cover romstage part).
But now, we need to include this header file from flash_controller.c to get it built. So, if we keep it local to the "fast_spi" directory, then in flash_controller.c file, we have to include like this -
#include <../common/block/fast_spi/fast_spi_def.h>
which doesn't look good, I thought.

I will make a /*TODO*/ comment in this file, to ensure to change this to block/fast_spi/ directory after the complete library is formed.


-- 
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: 18
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