coreboot-gerrit June 2019

coreboot-gerrit@coreboot.org

1 participants
1454 discussions

Change in ...coreboot[master]: mb/google/poppy/variants/atlas: enable NVMe
by caveh jalali (Code Review) 11 Jun '19

11 Jun '19

caveh jalali has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/28535 ) Change subject: mb/google/poppy/variants/atlas: enable NVMe ...................................................................... -- To view, visit https://review.coreboot.org/c/coreboot/+/28535 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I08e7c4d65662ddbb7d936915c896eb1fcb240ba8 Gerrit-Change-Number: 28535 Gerrit-PatchSet: 2 Gerrit-Owner: Caveh Jalali <caveh(a)google.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: caveh jalali <caveh(a)chromium.org> Gerrit-MessageType: revert

1 0

Change in ...coreboot[master]: mb/google/hatch: Disable dynamic clock gating for cr50's GPIO
by Subrata Banik (Code Review) 10 Jun '19

10 Jun '19

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33326 ) Change subject: mb/google/hatch: Disable dynamic clock gating for cr50's GPIO ...................................................................... Patch Set 2: > Patch Set 2: > > This works to observe even the skinniest cr50 irqs. Cool! I've updated it to only mess with COMM_4, since that's the one cr50 is on. Subrata, is it better to clobber all the flags that were in here, or is there a more minimal set of changes that can be made? i think your patchset 2 makes more sense as it just disable all gpio pm dynamic clock gating option. can you please share your cbmem -t (with release cb build) after this modification -- To view, visit https://review.coreboot.org/c/coreboot/+/33326 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I34d14fb7cc97e33eecfda2c99cc53a541c87662d Gerrit-Change-Number: 33326 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Evan Green <evgreen(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 11 Jun 2019 02:16:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: mb/google/hatch: Disable dynamic clock gating for cr50's GPIO
by Subrata Banik (Code Review) 10 Jun '19

10 Jun '19

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33326 ) Change subject: mb/google/hatch: Disable dynamic clock gating for cr50's GPIO ...................................................................... Patch Set 2: This change is ready for review. -- To view, visit https://review.coreboot.org/c/coreboot/+/33326 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I34d14fb7cc97e33eecfda2c99cc53a541c87662d Gerrit-Change-Number: 33326 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Evan Green <evgreen(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 11 Jun 2019 02:13:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: mb/google/octopus: Add API to disable USB devices from devicetree
by Karthik Ramasubramanian (Code Review) 10 Jun '19

10 Jun '19

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33377 ) Change subject: mb/google/octopus: Add API to disable USB devices from devicetree ...................................................................... Patch Set 1: (1 comment) This change is ready for review. https://review.coreboot.org/#/c/33377/1/src/mainboard/google/octopus/mainbo… File src/mainboard/google/octopus/mainboard.c: https://review.coreboot.org/#/c/33377/1/src/mainboard/google/octopus/mainbo… PS1, Line 191: usb_device_disable Prefer two improvements: 1) move inside src/soc/intel/common/block/xhci.c 2) Check Current Connect Status(CCS) bit in the USB Port Status and Control register in the XHCI register block and then disable the unconnected devices. I believe it works for externally visible ports too. -- To view, visit https://review.coreboot.org/c/coreboot/+/33377 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ied12faabee1b8c096f2b27de89ab42ee8be5d94d Gerrit-Change-Number: 33377 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Justin TerAvest <teravest(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Karthikeyan Ramasubramanian <kramasub(a)chromium.org> Gerrit-Comment-Date: Mon, 10 Jun 2019 21:22:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: device: Move pci_irq_info out of early devicetree
by Karthik Ramasubramanian (Code Review) 10 Jun '19

10 Jun '19

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33376 ) Change subject: device: Move pci_irq_info out of early devicetree ...................................................................... Patch Set 1: This change is ready for review. -- To view, visit https://review.coreboot.org/c/coreboot/+/33376 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icde376f9cf7431f09d7eb7e738b38e7dd97be8eb Gerrit-Change-Number: 33376 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Justin TerAvest <teravest(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Karthikeyan Ramasubramanian <kramasub(a)chromium.org> Gerrit-Comment-Date: Mon, 10 Jun 2019 21:18:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
by Mike Hsieh (Code Review) 10 Jun '19

10 Jun '19

Mike Hsieh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33360 Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF ...................................................................... mb/google/sarien/variants/arcada: Update thermal configuration for DPTF Update dptf for arcada DVT2. BUG=b:123924662 TEST=Built and tested on arcada system Signed-off-by: Mike Hsieh <mike_hsieh(a)wistron.corp-partner.google.com> Change-Id: I302b7cd4c7e0579acb5482800241b5229cfc49f7 --- M src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl 1 file changed, 10 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/33360/1 diff --git a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl index 4d38071..73e1dec 100644 --- a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl +++ b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl @@ -13,26 +13,26 @@ * GNU General Public License for more details. */ -#define DPTF_CPU_PASSIVE 90 -#define DPTF_CPU_CRITICAL 105 +#define DPTF_CPU_PASSIVE 98 +#define DPTF_CPU_CRITICAL 108 /* Skin Sensor for CPU VR temperature monitor */ #define DPTF_TSR0_SENSOR_ID 1 #define DPTF_TSR0_SENSOR_NAME "Skin" -#define DPTF_TSR0_PASSIVE 60 -#define DPTF_TSR0_CRITICAL 105 +#define DPTF_TSR0_PASSIVE 55 +#define DPTF_TSR0_CRITICAL 100 /* Memory Sensor for DDR temperature monitor */ #define DPTF_TSR1_SENSOR_ID 2 #define DPTF_TSR1_SENSOR_NAME "DDR" -#define DPTF_TSR1_PASSIVE 70 -#define DPTF_TSR1_CRITICAL 95 +#define DPTF_TSR1_PASSIVE 53 +#define DPTF_TSR1_CRITICAL 100 /* M.2 Sensor for Ambient temperature monitor */ #define DPTF_TSR2_SENSOR_ID 3 #define DPTF_TSR2_SENSOR_NAME "Ambient" -#define DPTF_TSR2_PASSIVE 37 -#define DPTF_TSR2_CRITICAL 80 +#define DPTF_TSR2_PASSIVE 38 +#define DPTF_TSR2_CRITICAL 93 #undef DPTF_ENABLE_FAN_CONTROL #undef DPTF_ENABLE_CHARGER @@ -42,10 +42,10 @@ Package () { \_SB.PCI0.TCPU, \_SB.PCI0.TCPU, 500, 100, 0, 0, 0, 0 }, /* CPU Throttle Effect on Skin (TSR0) */ - Package () { \_SB.PCI0.TCPU, \_SB.DPTF.TSR0, 500, 30, 0, 0, 0, 0 }, + Package () { \_SB.PCI0.TCPU, \_SB.DPTF.TSR0, 400, 40, 0, 0, 0, 0 }, /* CPU Throttle Effect on DDR (TSR1) */ - Package () { \_SB.PCI0.TCPU, \_SB.DPTF.TSR1, 100, 50, 2, 0, 0, 0 }, + Package () { \_SB.PCI0.TCPU, \_SB.DPTF.TSR1, 300, 50, 2, 0, 0, 0 }, /* CPU Throttle Effect on Ambient (TSR2) */ Package () { \_SB.PCI0.TCPU, \_SB.DPTF.TSR2, 1000, 100, 1, 0, 0, 0 }, -- To view, visit https://review.coreboot.org/c/coreboot/+/33360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I302b7cd4c7e0579acb5482800241b5229cfc49f7 Gerrit-Change-Number: 33360 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Hsieh <mike_hsieh(a)wistron.corp-partner.google.com> Gerrit-MessageType: newchange

3 3

Change in ...coreboot[master]: mb/google/poppy/variants/nami: remove redundant break
by EricR Lai (Code Review) 10 Jun '19

10 Jun '19

EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33299 Change subject: mb/google/poppy/variants/nami: remove redundant break ...................................................................... mb/google/poppy/variants/nami: remove redundant break Break never comes after return, remove it. BUG=N/A BRANCH=firmware-nami-10775.108.B TEST=N/A Signed-off-by: Eric Lai <ericr_lai(a)compal.corp-partner.google.com> Change-Id: I005918d6a04cd21df496dea0f2cb1ed6108675af --- M src/mainboard/google/poppy/variants/nami/mainboard.c 1 file changed, 0 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/33299/1 diff --git a/src/mainboard/google/poppy/variants/nami/mainboard.c b/src/mainboard/google/poppy/variants/nami/mainboard.c index 2387017..adb8c00 100644 --- a/src/mainboard/google/poppy/variants/nami/mainboard.c +++ b/src/mainboard/google/poppy/variants/nami/mainboard.c @@ -215,7 +215,6 @@ return "vbt-bard.bin"; default: return "vbt.bin"; - break; } } -- To view, visit https://review.coreboot.org/c/coreboot/+/33299 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I005918d6a04cd21df496dea0f2cb1ed6108675af Gerrit-Change-Number: 33299 Gerrit-PatchSet: 1 Gerrit-Owner: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-MessageType: newchange

3 3

Change in ...coreboot[master]: sdm845: qspi: Add Dual SPI support
by Julius Werner (Code Review) 10 Jun '19

10 Jun '19

Hello Aaron Durbin, Furquan Shaikh, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/33284 to review the following change. Change subject: sdm845: qspi: Add Dual SPI support ...................................................................... sdm845: qspi: Add Dual SPI support This patch adds support for the Dual SPI feature (SDR 2-bit in Qualcomm terminology) to the QSPI controller. Change-Id: I7aed2ccd9627f5de5dd760b418f74d56d2c031d3 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/soc/qualcomm/sdm845/include/soc/qspi.h M src/soc/qualcomm/sdm845/qspi.c M src/soc/qualcomm/sdm845/spi.c 3 files changed, 17 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/33284/1 diff --git a/src/soc/qualcomm/sdm845/include/soc/qspi.h b/src/soc/qualcomm/sdm845/include/soc/qspi.h index ac9fd50..3f83421 100644 --- a/src/soc/qualcomm/sdm845/include/soc/qspi.h +++ b/src/soc/qualcomm/sdm845/include/soc/qspi.h @@ -116,4 +116,6 @@ void sdm845_release_bus(const struct spi_slave *slave); int sdm845_xfer(const struct spi_slave *slave, const void *dout, size_t out_bytes, void *din, size_t in_bytes); +int sdm845_xfer_dual(const struct spi_slave *slave, const void *dout, + size_t out_bytes, void *din, size_t in_bytes); #endif /* __SOC_QUALCOMM_SDM845_QSPI_H__ */ diff --git a/src/soc/qualcomm/sdm845/qspi.c b/src/soc/qualcomm/sdm845/qspi.c index b9a5194..cced567 100644 --- a/src/soc/qualcomm/sdm845/qspi.c +++ b/src/soc/qualcomm/sdm845/qspi.c @@ -277,11 +277,9 @@ cs_change(CS_DEASSERT); } -int sdm845_xfer(const struct spi_slave *slave, const void *dout, - size_t out_bytes, void *din, size_t in_bytes) +static int xfer(enum qspi_mode mode, const void *dout, size_t out_bytes, + void *din, size_t in_bytes) { - enum qspi_mode mode = SDR_1BIT; - if ((out_bytes && !dout) || (in_bytes && !din) || (in_bytes && out_bytes)) { return -1; @@ -294,3 +292,15 @@ return 0; } + +int sdm845_xfer(const struct spi_slave *slave, const void *dout, + size_t out_bytes, void *din, size_t in_bytes) +{ + return xfer(SDR_1BIT, dout, out_bytes, din, in_bytes); +} + +int sdm845_xfer_dual(const struct spi_slave *slave, const void *dout, + size_t out_bytes, void *din, size_t in_bytes) +{ + return xfer(SDR_2BIT, dout, out_bytes, din, in_bytes); +} diff --git a/src/soc/qualcomm/sdm845/spi.c b/src/soc/qualcomm/sdm845/spi.c index e276e1d..27aafa7 100644 --- a/src/soc/qualcomm/sdm845/spi.c +++ b/src/soc/qualcomm/sdm845/spi.c @@ -21,6 +21,7 @@ .claim_bus = sdm845_claim_bus, .release_bus = sdm845_release_bus, .xfer = sdm845_xfer, + .xfer_dual = sdm845_xfer_dual, .max_xfer_size = QSPI_MAX_PACKET_COUNT, }; -- To view, visit https://review.coreboot.org/c/coreboot/+/33284 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7aed2ccd9627f5de5dd760b418f74d56d2c031d3 Gerrit-Change-Number: 33284 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newchange

3 4

Change in ...coreboot[master]: spi_flash: Add Dual SPI support
by Julius Werner (Code Review) 10 Jun '19

10 Jun '19

Hello Aaron Durbin, Furquan Shaikh, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/33283 to review the following change. Change subject: spi_flash: Add Dual SPI support ...................................................................... spi_flash: Add Dual SPI support This patch adds support to read SPI flash in Dual SPI mode, where both MISO and MOSI lines are used for output mode (specifically Fast Read Dual Output (0x3b) where the command is still sent normally, not Fast Read Dual I/O (0xbb) whose additional benefit should be extremely marginal for our use cases but which would be more complicated to implement). This feature needs to be supported by both the flash chip and the controller, so we add a new dual_spi flag (and a new flags field to hold it) to the spi_flash structure and a new optional xfer_dual() function pointer to the spi_ctrlr structure. When both are provided, Dual SPI mode is used automatically, otherwise things work as before. This patch only adds the dual_spi flag exemplary to all Winbond flash chips, other vendors need to be added as needed. Change-Id: Ic6808224c99af32b6c5c43054135c8f4c03c1feb Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/winbond.c M src/include/spi-generic.h M src/include/spi_flash.h 5 files changed, 72 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/33283/1 diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index d152027..dcb1108 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -58,6 +58,34 @@ return ret; } +static int do_dual_read_cmd(const struct spi_slave *spi, const void *dout, + size_t bytes_out, void *din, size_t bytes_in) +{ + int ret; + + /* + * spi_xfer_vector() will automatically fall back to .xfer() if + * .xfer_vector() is unimplemented. So using vector API here is more + * flexible, even though a controller that implements .xfer_vector() + * and (the non-vector based) .xfer_dual() but not .xfer() would be + * pretty odd. + */ + struct spi_op vector = { .dout = dout, .bytesout = bytes_out, + .din = NULL, .bytesin = 0 }; + + if ((ret = spi_claim_bus(spi))) + return ret; + + if ((ret = spi_xfer_vector(spi, &vector, 1))) + goto release; + + ret = spi->ctrlr->xfer_dual(spi, NULL, 0, din, bytes_in); + +release: + spi_release_bus(spi); + return ret; +} + int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, size_t len) { int ret = do_spi_flash_cmd(spi, &cmd, sizeof(cmd), response, len); @@ -104,6 +132,11 @@ cmd_len = 4; cmd[0] = CMD_READ_ARRAY_SLOW; do_cmd = do_spi_flash_cmd; + } else if (flash->flags.dual_spi && flash->spi.ctrlr->xfer_dual) { + cmd_len = 5; + cmd[0] = CMD_READ_FAST_DUAL_OUTPUT; + cmd[4] = 0; + do_cmd = do_dual_read_cmd; } else { cmd_len = 5; cmd[0] = CMD_READ_ARRAY_FAST; @@ -346,8 +379,12 @@ return -1; } - printk(BIOS_INFO, "SF: Detected %s with sector size 0x%x, total 0x%x\n", - flash->name, flash->sector_size, flash->size); + const char *mode_string = ""; + if (flash->flags.dual_spi && spi.ctrlr->xfer_dual) + mode_string = " (Dual SPI mode)"; + printk(BIOS_INFO, + "SF: Detected %s with sector size 0x%x, total 0x%x%s\n", + flash->name, flash->sector_size, flash->size, mode_string); if (bus == CONFIG_BOOT_DEVICE_SPI_FLASH_BUS && flash->size != CONFIG_ROM_SIZE) { printk(BIOS_ERR, "SF size 0x%x does not correspond to" diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index f15c737..4a9e289 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -23,6 +23,8 @@ #define CMD_READ_ARRAY_FAST 0x0b #define CMD_READ_ARRAY_LEGACY 0xe8 +#define CMD_READ_FAST_DUAL_OUTPUT 0x3b + #define CMD_READ_STATUS 0x05 #define CMD_WRITE_ENABLE 0x06 diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index d0ef3cd..9e9bb00 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -26,7 +26,9 @@ struct winbond_spi_flash_params { uint16_t id; - uint8_t l2_page_size_shift; + uint8_t dual_spi : 1; + uint8_t _reserved_for_flags : 3; + uint8_t l2_page_size_shift : 4; uint8_t pages_per_sector_shift : 4; uint8_t sectors_per_block_shift : 4; uint8_t nr_blocks_shift; @@ -123,6 +125,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 4, .name = "W25X80", + .dual_spi = 1, }, { .id = 0x3015, @@ -131,6 +134,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 5, .name = "W25X16", + .dual_spi = 1, }, { .id = 0x3016, @@ -139,6 +143,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 6, .name = "W25X32", + .dual_spi = 1, }, { .id = 0x3017, @@ -147,6 +152,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 7, .name = "W25X64", + .dual_spi = 1, }, { .id = 0x4014, @@ -155,6 +161,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 4, .name = "W25Q80_V", + .dual_spi = 1, }, { .id = 0x4015, @@ -163,6 +170,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 5, .name = "W25Q16_V", + .dual_spi = 1, .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -173,6 +181,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 5, .name = "W25Q16DW", + .dual_spi = 1, .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -183,6 +192,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 6, .name = "W25Q32_V", + .dual_spi = 1, .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -193,6 +203,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 6, .name = "W25Q32DW", + .dual_spi = 1, .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -203,6 +214,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 7, .name = "W25Q64_V", + .dual_spi = 1, .protection_granularity_shift = 17, .bp_bits = 3, }, @@ -213,6 +225,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 7, .name = "W25Q64DW", + .dual_spi = 1, .protection_granularity_shift = 17, .bp_bits = 3, }, @@ -223,6 +236,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 8, .name = "W25Q128_V", + .dual_spi = 1, .protection_granularity_shift = 18, .bp_bits = 3, }, @@ -233,6 +247,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 8, .name = "W25Q128FW", + .dual_spi = 1, .protection_granularity_shift = 18, .bp_bits = 3, }, @@ -243,6 +258,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 8, .name = "W25Q128J", + .dual_spi = 1, .protection_granularity_shift = 18, .bp_bits = 3, }, @@ -253,6 +269,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 9, .name = "W25Q256_V", + .dual_spi = 1, .protection_granularity_shift = 16, .bp_bits = 4, }, @@ -263,6 +280,7 @@ .sectors_per_block_shift = 4, .nr_blocks_shift = 9, .name = "W25Q256J", + .dual_spi = 1, .protection_granularity_shift = 16, .bp_bits = 4, }, @@ -681,6 +699,8 @@ flash->erase_cmd = CMD_W25_SE; flash->status_cmd = CMD_W25_RDSR; + flash->flags.dual_spi = params->dual_spi; + flash->ops = &spi_flash_ops; flash->driver_private = params; diff --git a/src/include/spi-generic.h b/src/include/spi-generic.h index c24aadd..d0f957f 100644 --- a/src/include/spi-generic.h +++ b/src/include/spi-generic.h @@ -125,6 +125,7 @@ * setup: Setup given SPI device bus. * xfer: Perform one SPI transfer operation. * xfer_vector: Vector of SPI transfer operations. + * xfer_dual: (optional) Perform one SPI transfer in Dual SPI mode. * max_xfer_size: Maximum transfer size supported by the controller * (0 = invalid, * SPI_CTRLR_DEFAULT_MAX_XFER_SIZE = unlimited) @@ -145,6 +146,8 @@ size_t bytesout, void *din, size_t bytesin); int (*xfer_vector)(const struct spi_slave *slave, struct spi_op vectors[], size_t count); + int (*xfer_dual)(const struct spi_slave *slave, const void *dout, + size_t bytesout, void *din, size_t bytesin); uint32_t max_xfer_size; uint32_t flags; int (*flash_probe)(const struct spi_slave *slave, diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h index 936b0ab..3a0c383 100644 --- a/src/include/spi_flash.h +++ b/src/include/spi_flash.h @@ -90,6 +90,13 @@ struct spi_flash { struct spi_slave spi; u8 vendor; + union { + u8 raw; + struct { + u8 dual_spi : 1; + u8 _reserved : 7; + }; + } flags; u16 model; const char *name; u32 size; -- To view, visit https://review.coreboot.org/c/coreboot/+/33283 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic6808224c99af32b6c5c43054135c8f4c03c1feb Gerrit-Change-Number: 33283 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newchange

4 10

Change in ...coreboot[master]: spi_flash: Make .read() callback optional
by Julius Werner (Code Review) 10 Jun '19

10 Jun '19

Hello Aaron Durbin, Furquan Shaikh, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/33282 to review the following change. Change subject: spi_flash: Make .read() callback optional ...................................................................... spi_flash: Make .read() callback optional All SPI flash chip drivers currently in coreboot use the generic read functions (spi_flash_cmd_read_fast()/_slow()) as their read callback. The only use case for specialized read callbacks we have left is with specialized flash controllers like Intel fast_spi (which sort of impersonate the flash chip driver by implementing their own probe function). This patch unifies the behavior for all normal flash drivers by making the read callback optional and letting them all fall back to a default read implementation that handles normal fast/slow reading. Most of the drivers used to install the respective callback after checking CONFIG_SPI_FLASH_NO_FAST_READ, but some hardcoded either slow or fast writes. I have found no indications for why this is and spot-checked datasheets for affected vendors to make sure they all support both commands, so I assume this is just some old inaccuracy rather than important differences that need preserving. (Please yell if you disagree.) Also take the opportunity to refactor some of the common spi_flash.c code a bit because I felt there are too many nested functions that don't really do enough on their own, and centralizing stuff a bit should make it easier to follow the code flow. (Some of this is in preparation for the next patch.) Change-Id: I2096a3ce619767b41b1b0c0c2b8e95b2bd90a419 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/drivers/spi/adesto.c M src/drivers/spi/amic.c M src/drivers/spi/atmel.c M src/drivers/spi/eon.c M src/drivers/spi/gigadevice.c M src/drivers/spi/macronix.c M src/drivers/spi/spansion.c M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/sst.c M src/drivers/spi/stmicro.c M src/drivers/spi/winbond.c 12 files changed, 34 insertions(+), 103 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/33282/1 diff --git a/src/drivers/spi/adesto.c b/src/drivers/spi/adesto.c index a805609..4e1043e 100644 --- a/src/drivers/spi/adesto.c +++ b/src/drivers/spi/adesto.c @@ -201,11 +201,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = adesto_write, .erase = spi_flash_cmd_erase, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif }; int spi_flash_probe_adesto(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/amic.c b/src/drivers/spi/amic.c index 64c91cc..b580dc3 100644 --- a/src/drivers/spi/amic.c +++ b/src/drivers/spi/amic.c @@ -176,11 +176,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = amic_write, .erase = spi_flash_cmd_erase, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif }; int spi_flash_probe_amic(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/atmel.c b/src/drivers/spi/atmel.c index 7d6e172..58a2862 100644 --- a/src/drivers/spi/atmel.c +++ b/src/drivers/spi/atmel.c @@ -157,11 +157,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = atmel_write, .erase = spi_flash_cmd_erase, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif }; int spi_flash_probe_atmel(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/eon.c b/src/drivers/spi/eon.c index 33e12a0..f3cf70e 100644 --- a/src/drivers/spi/eon.c +++ b/src/drivers/spi/eon.c @@ -292,7 +292,6 @@ .write = eon_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, - .read = spi_flash_cmd_read_fast, }; int spi_flash_probe_eon(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/gigadevice.c b/src/drivers/spi/gigadevice.c index 83216c0..194c5ec 100644 --- a/src/drivers/spi/gigadevice.c +++ b/src/drivers/spi/gigadevice.c @@ -222,11 +222,6 @@ .write = gigadevice_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif }; int spi_flash_probe_gigadevice(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c index 1610ca1..5a97b8f 100644 --- a/src/drivers/spi/macronix.c +++ b/src/drivers/spi/macronix.c @@ -268,11 +268,6 @@ .write = macronix_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif }; int spi_flash_probe_macronix(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/spansion.c b/src/drivers/spi/spansion.c index e687bf8..4a241ba 100644 --- a/src/drivers/spi/spansion.c +++ b/src/drivers/spi/spansion.c @@ -282,7 +282,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = spansion_write, .erase = spi_flash_cmd_erase, - .read = spi_flash_cmd_read_slow, .status = spi_flash_cmd_status, }; diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index ae1d2ef..d152027 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -31,7 +31,7 @@ static int do_spi_flash_cmd(const struct spi_slave *spi, const void *dout, size_t bytes_out, void *din, size_t bytes_in) { - int ret = 1; + int ret; /* * SPI flash requires command-response kind of behavior. Thus, two * separate SPI vectors are required -- first to transmit dout and other @@ -49,11 +49,10 @@ if (!bytes_in) count = 1; - if (spi_claim_bus(spi)) + if ((ret = spi_claim_bus(spi))) return ret; - if (spi_xfer_vector(spi, vectors, count) == 0) - ret = 0; + ret = spi_xfer_vector(spi, vectors, count); spi_release_bus(spi); return ret; @@ -68,18 +67,6 @@ return ret; } -static int spi_flash_cmd_read(const struct spi_slave *spi, const u8 *cmd, - size_t cmd_len, void *data, size_t data_len) -{ - int ret = do_spi_flash_cmd(spi, cmd, cmd_len, data, data_len); - if (ret) { - printk(BIOS_WARNING, "SF: Failed to send read command (%zu bytes): %d\n", - data_len, ret); - } - - return ret; -} - /* TODO: This code is quite possibly broken and overflowing stacks. Fix ASAP! */ #pragma GCC diagnostic push #if defined(__GNUC__) && !defined(__clang__) @@ -103,34 +90,38 @@ } #pragma GCC diagnostic pop -static int spi_flash_cmd_read_array(const struct spi_slave *spi, u8 *cmd, - size_t cmd_len, u32 offset, - size_t len, void *data) -{ - spi_flash_addr(offset, cmd); - return spi_flash_cmd_read(spi, cmd, cmd_len, data, len); -} - /* Perform the read operation honoring spi controller fifo size, reissuing * the read command until the full request completed. */ -static int spi_flash_cmd_read_array_wrapped(const struct spi_slave *spi, - u8 *cmd, size_t cmd_len, u32 offset, - size_t len, void *buf) +static int spi_flash_read_chunked(const struct spi_flash *flash, u32 offset, + size_t len, void *buf) { - int ret; - size_t xfer_len; + u8 cmd[5]; + int ret, cmd_len; + int (*do_cmd)(const struct spi_slave *, const void *, size_t, + void *, size_t); + + if (CONFIG(SPI_FLASH_NO_FAST_READ)) { + cmd_len = 4; + cmd[0] = CMD_READ_ARRAY_SLOW; + do_cmd = do_spi_flash_cmd; + } else { + cmd_len = 5; + cmd[0] = CMD_READ_ARRAY_FAST; + cmd[4] = 0; + do_cmd = do_spi_flash_cmd; + } + uint8_t *data = buf; - while (len) { - xfer_len = spi_crop_chunk(spi, cmd_len, len); - - /* Perform the read. */ - ret = spi_flash_cmd_read_array(spi, cmd, cmd_len, - offset, xfer_len, data); - - if (ret) + size_t xfer_len = spi_crop_chunk(&flash->spi, sizeof(cmd), len); + spi_flash_addr(offset, cmd); + ret = do_cmd(&flash->spi, cmd, cmd_len, data, xfer_len); + if (ret) { + printk(BIOS_WARNING, + "SF: Failed to send read command %#.2x(%#x, %#zx): %d\n", + cmd[0], offset, xfer_len, ret); return ret; - + } offset += xfer_len; data += xfer_len; len -= xfer_len; @@ -139,28 +130,6 @@ return 0; } -int spi_flash_cmd_read_fast(const struct spi_flash *flash, u32 offset, - size_t len, void *data) -{ - u8 cmd[5]; - - cmd[0] = CMD_READ_ARRAY_FAST; - cmd[4] = 0x00; - - return spi_flash_cmd_read_array_wrapped(&flash->spi, cmd, sizeof(cmd), - offset, len, data); -} - -int spi_flash_cmd_read_slow(const struct spi_flash *flash, u32 offset, - size_t len, void *data) -{ - u8 cmd[4]; - - cmd[0] = CMD_READ_ARRAY_SLOW; - return spi_flash_cmd_read_array_wrapped(&flash->spi, cmd, sizeof(cmd), - offset, len, data); -} - int spi_flash_cmd_poll_bit(const struct spi_flash *flash, unsigned long timeout, u8 cmd, u8 poll_bit) { @@ -174,7 +143,7 @@ mono_time_add_msecs(&end, timeout); do { - ret = spi_flash_cmd_read(spi, &cmd, 1, &status, 1); + ret = do_spi_flash_cmd(spi, &cmd, 1, &status, 1); if (ret) return -1; if ((status & poll_bit) == 0) @@ -391,7 +360,10 @@ int spi_flash_read(const struct spi_flash *flash, u32 offset, size_t len, void *buf) { - return flash->ops->read(flash, offset, len, buf); + if (flash->ops->read) + return flash->ops->read(flash, offset, len, buf); + + return spi_flash_read_chunked(flash, offset, len, buf); } int spi_flash_write(const struct spi_flash *flash, u32 offset, size_t len, diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index a89610a..f15c737 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -34,12 +34,6 @@ /* Send a single-byte command to the device and read the response */ int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, size_t len); -int spi_flash_cmd_read_fast(const struct spi_flash *flash, u32 offset, - size_t len, void *data); - -int spi_flash_cmd_read_slow(const struct spi_flash *flash, u32 offset, - size_t len, void *data); - /* * Send a multi-byte command to the device followed by (optional) * data. Used for programming the flash array, etc. diff --git a/src/drivers/spi/sst.c b/src/drivers/spi/sst.c index e4ea7805..abe3f2a 100644 --- a/src/drivers/spi/sst.c +++ b/src/drivers/spi/sst.c @@ -55,14 +55,12 @@ .write = sst_write_ai, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, - .read = spi_flash_cmd_read_fast, }; static const struct spi_flash_ops spi_flash_ops_write_256 = { .write = sst_write_256, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, - .read = spi_flash_cmd_read_fast, }; #define SST_SECTOR_SIZE (4 * 1024) diff --git a/src/drivers/spi/stmicro.c b/src/drivers/spi/stmicro.c index fb24b27..6625764 100644 --- a/src/drivers/spi/stmicro.c +++ b/src/drivers/spi/stmicro.c @@ -351,7 +351,6 @@ static const struct spi_flash_ops spi_flash_ops = { .write = stmicro_write, .erase = spi_flash_cmd_erase, - .read = spi_flash_cmd_read_fast, }; int spi_flash_probe_stmicro(const struct spi_slave *spi, u8 *idcode, diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 11a5187..d0ef3cd 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -646,11 +646,6 @@ .write = winbond_write, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, -#if CONFIG(SPI_FLASH_NO_FAST_READ) - .read = spi_flash_cmd_read_slow, -#else - .read = spi_flash_cmd_read_fast, -#endif .get_write_protection = winbond_get_write_protection, .set_write_protection = winbond_set_write_protection, }; -- To view, visit https://review.coreboot.org/c/coreboot/+/33282 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2096a3ce619767b41b1b0c0c2b8e95b2bd90a419 Gerrit-Change-Number: 33282 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newchange

5 12

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2019