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

(9 comments)

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

PS16, Line 26: uint32_t
> uintptr_t
will fix


Line 32:        return (void *)(bar & ~PCI_BASE_ADDRESS_MEM_ATTR_MASK);
> This whole function isn't indented correctly.
will fix


Line 40: 						SPIBAR_BIOS_CONTROL_BILD);
> Just use a variable for the register value. Readability is being sacrificed
ok


Line 42: 	pci_read_config32(PCH_DEV_SPI, SPIBAR_BIOS_CONTROL);
> pci config space by definition doesn't have posted write semantics. Why is 
will check and get back on this.


PS16, Line 86: u32
> uint32_t
will fix


Line 115: size_t get_fast_spi_bios_region(size_t *bios_size)
> Can you please be consistent and make the functions start with fast_spi_ ?
will fix


Line 135: void fast_spi_flash_init(device_t dev)
> So if this is called from a function internal this compilation unit why is 
Currently, as 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, chip.c, smihandler.c files), which calls this function to ensure writes can take place to flash.

So, I have made it non-static to avoid compilation issues.
In future, when the complete library will be formed and if we dont require this function from user side, we can make it Local to this file.


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

Line 157
> > Okay, will check and fix these functions.
Yes, we have analysed Small Core's "flash_ctrlr.c" and Big Core's "flash_controller.c" file, both can be made common, and we will again maintain a library of flash_read, flash_write, flash_erase etc, etc in this FAST_SPI IP block common code. From SOC, it will only call the respective APIs of the library.


https://review.coreboot.org/#/c/18557/16/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
> include/intelblocks/fast_spi_def.h is the external headers for SoC consumpt
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 there 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.


-- 
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: 16
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: build bot (Jenkins)
Gerrit-HasComments: Yes