[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block: 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: Add Intel common FAST_SPI code
......................................................................


Patch Set 28:

(10 comments)

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

PS28, Line 16: #include <arch/early_variables.h>
> Is this required?
not required, will remove.


PS28, Line 38: bar = pci_read_config32(dev, PCI_BASE_ADDRESS_0);
> Should we assert if bar is 0? To make sure it is set by SoC in early stages
Yes, we can add an ASSERT for False case.
Will add it.


PS28, Line 49: fast_spi_flash_init
> Seems odd that one function in this has _flash_ in the name? Should this be
yes, will change


PS28, Line 66: /*
             :  * Set FAST_SPIBAR BIOS Control BILD bit.
             :  */
             : void fast_spi_set_bios_interface_lock_down(void)
             : {
             : 	device_t dev = PCH_DEV_SPI;
             : 	uintptr_t bc_bild;
             : 
             : 	bc_bild = pci_read_config32(dev, SPIBAR_BIOS_CONTROL);
             : 	bc_bild |= SPIBAR_BIOS_CONTROL_BILD;
             : 	pci_write_config32(dev, SPIBAR_BIOS_CONTROL, bc_bild);
             : }
             : 
             : /*
             :  * Set FAST_SPIBAR BIOS Control LE bit.
             :  */
             : void fast_spi_set_lock_enable(void)
             : {
             : 	device_t dev = PCH_DEV_SPI;
             : 	uint8_t bc_le;
             : 
             : 	bc_le = pci_read_config8(dev, SPIBAR_BIOS_CONTROL);
             : 	bc_le |= SPIBAR_BIOS_CONTROL_LOCK_ENABLE;
             : 	pci_write_config8(dev, SPIBAR_BIOS_CONTROL, bc_le);
             : }
             : 
             : /*
             :  * Set FAST_SPIBAR BIOS Control EISS bit.
             :  */
             : void fast_spi_set_eiss(void)
             : {
             : 	device_t dev = PCH_DEV_SPI;
             : 	uint8_t bc_eiss;
             : 
             : 	bc_eiss = pci_read_config8(dev, SPIBAR_BIOS_CONTROL);
             : 	bc_eiss |= SPIBAR_BIOS_CONTROL_EISS;
             : 	pci_write_config8(dev, SPIBAR_BIOS_CONTROL, bc_eiss);
             : }
> Can we have one common function:
yes sure, will make this change


PS28, Line 141: ssl |= SPIBAR_RESET_LOCK_DISABLE;
> This doesn't look right. Shouldn't this be something like:
yes, thats correct. Will change it.


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

PS28, Line 53: uint32_t bar;
             : 
             : 	/* FIXME: use device definition */
             : 	ctx->pci_dev = PCH_DEV_SPI;
             : 
             : 	bar = pci_read_config32(ctx->pci_dev, PCI_BASE_ADDRESS_0);
             : 	ctx->mmio_base = bar & ~PCI_BASE_ADDRESS_MEM_ATTR_MASK;
> Can't we use fast_spi_get_bar();
Thats correct, will change it.


PS28, Line 86: SFPD
> SFDP
will change


PS28, Line 101: static void fill_xfer_fifo(struct fast_spi_flash_ctx *ctx, const void *data,
              : 			   size_t len)
              : {
              : 	len = min(len, SPIBAR_FDATA_FIFO_SIZE);
              : 
              : 	/* YES! memcpy() works. FDATAn does not require 32-bit accesses. */
              : 	memcpy((void *)(ctx->mmio_base + SPIBAR_FDATA(0)), data, len);
              : }
              : 
              : /* Drain FDATAn FIFO after a read transaction populates data. */
              : static void drain_xfer_fifo(struct fast_spi_flash_ctx *ctx, void *dest,
              : 				size_t len)
              : {
              : 	len = min(len, SPIBAR_FDATA_FIFO_SIZE);
              : 
              : 	/* YES! memcpy() works. FDATAn does not require 32-bit accesses. */
              : 	memcpy(dest, (void *)(ctx->mmio_base + SPIBAR_FDATA(0)), len);
              : }
> Shouldn't this return len so that the caller knows if less than requested b
yes, will modify these.


PS28, Line 147: ,
> space after ,
ok


PS28, Line 357: fast_spi_read_wpsr
> fast_spi_flash_read_wpsr
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: 28
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