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... 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/... 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/... 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.
-
Barnali Sarkar (Code Review)