[coreboot-gerrit] Change in coreboot[master]: soc/intel/skylake: Allow mainboards to pass in SPI config

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19095 )

Change subject: soc/intel/skylake: Allow mainboards to pass in SPI config
......................................................................


Patch Set 1:

(3 comments)

> This CL seems to be in conflict with https://review.coreboot.org/#/c/18557 which breaks out the common fast spi routines. We probably want to keep that distinction in that fast spi is not like gspi. We can still keep the bus number notion but the ip blocks truly are completely different.

That's right. I was just trying to put the BAR init into a common place for Fast SPI and GSPI -- this includes the early init config and speed (which is valid only for GSPI since GSPI controller actually uses it). Does it seem like keeping Fast SPI init separate is better? Then, I can just use the configs only GSPI bus only.

https://review.coreboot.org/#/c/19095/1/src/soc/intel/skylake/chip.h
File src/soc/intel/skylake/chip.h:

PS1, Line 37: 3
> Is this covering fast spi too? We can't control speed of that device in the
That is correct. This is supposed to cover all SPI buses including Fast SPI. Speed for Fast SPI would just be ignored as it is currently used only by the GSPI controller driver.


https://review.coreboot.org/#/c/19095/1/src/soc/intel/skylake/include/soc/bootblock.h
File src/soc/intel/skylake/include/soc/bootblock.h:

Line 29: void spi_early_init(void);
> Note that this function just enables the BAR? Should we rename it according
Makes sense. I will udpate the name accordingly.


https://review.coreboot.org/#/c/19095/1/src/soc/intel/skylake/include/soc/iomap.h
File src/soc/intel/skylake/include/soc/iomap.h:

Line 36: #define EARLY_SPI_BASE(x)	(EARLY_SPI_BASE_ADDRESS + (0x1000 * (x)))
> fast spi is 4KiB BAR as well as the gspi ones too?
It seems like Fast SPI MMIO space is smaller than the GSPI space.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I18844e2e3fc862006df072444f397d31e091e3fd
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Duncan Laurie <dlaurie at chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes