Youness Alaoui has uploaded this change for review. ( https://review.coreboot.org/22967
Change subject: intel/common/smbus: Add support for reading SPD by block ......................................................................
intel/common/smbus: Add support for reading SPD by block
This change adds support for reading from the SMBus using the Block Read command, which speeds up the transfer of data significantly (~75%) when compared to Byte Read or Word Read commands.
There are two methods for reading block data, either with the E32B option or without it. When enabling E32B, a 32 Byte array is used to return up to 32 bytes of block data. Otherwise, each byte needs to be read from a single byte register whenever it becomes available.
When testing on Purism Librem 13 v2, the E32B option did not work, as it would return mangled data every time and at the wrong offsets from what was requested. Doing a byte-by-byte read works correctly however.
Unfortunately, there is a quirk in the SMBus controller where the first byte of data is returned in the DAT0 register and the rest is returned in the HBD register (as expected). The datasheet says that the DAT0 register should instead contain the number of bytes being returned.
This change adds three new options, one for using the Block Read to read the SPD data, one to specify whether to use the E32B feature or not, and one for the DAT0 quirk mentioned above.
Note that the E32B implementation was not tested on compatible hardware but it was written to follow the specification and should work if the hardware supports E32B and has no additional quirks.
Speed improvements : - With Byte Read : 218 ms to dump DDR4 SPD data (512 bytes) - With Word Read : 134 ms to dump DDR4 SPD data (512 bytes) - With Block Read : 62 ms to dump DDR4 SPD data (512 bytes) - With Byte Read : 108 ms to dump DDR3 SPD data (256 bytes) - With Word Read : 66 ms to dump DDR3 SPD data (256 bytes) - With Block Read : 34 ms to dump DDR3 SPD data (256 bytes)
Tested on Purism Librem 13 v2 and Librem 15 v3 with the DAT0 Quirk option enabled.
Change-Id: I14882b646e7c0fa96cf38a698562cc20bbd41095 Signed-off-by: Youness Alaoui youness.alaoui@puri.sm --- M src/Kconfig M src/include/device/early_smbus.h M src/lib/spd_bin.c M src/soc/intel/common/block/smbus/smbus_early.c M src/soc/intel/common/block/smbus/smbuslib.c M src/soc/intel/common/block/smbus/smbuslib.h 6 files changed, 195 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/22967/1
diff --git a/src/Kconfig b/src/Kconfig index 6896d0e..8c524a5 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -1200,6 +1200,34 @@
config SPD_READ_BY_WORD bool + help + Use the Read Word command to the SMBus to read the SPD data. + This option can speed up boot time by ~80 to ~90ms + +config SPD_READ_BY_BLOCK + bool + default n + help + Use the Read Block command to the SMBus to read the SPD data. + This option can speed up boot time by ~150ms + +config SMBUS_BLOCK_READ_SUPPORTS_E32B + bool + default n + help + Enable if the SMBus supports the E32B option to use a 32-byte array + for the Read Block command. Some interfaces may not support this + feature, and it can cause invalid data to be returned. + When in doubt, keep disabled, and the Read Block command will use a + single byte register to read data from the controller instead. + +config SMBUS_BLOCK_READ_DAT0_QUIRK + bool + default n + help + This quirk was experienced on Skylake (Purism Librem) machines where the + DAT0 register contains the first byte of the block instead of containing + the number of bytes returned.
config BOOTBLOCK_CUSTOM # To be selected by arch, SoC or mainboard if it does not want use the normal diff --git a/src/include/device/early_smbus.h b/src/include/device/early_smbus.h index c907396..84baddb 100644 --- a/src/include/device/early_smbus.h +++ b/src/include/device/early_smbus.h @@ -66,6 +66,7 @@ u16 smbus_read_word(u32 smbus_dev, u8 addr, u8 offset); u8 smbus_read_byte(u32 smbus_dev, u8 addr, u8 offset); u8 smbus_write_byte(u32 smbus_dev, u8 addr, u8 offset, u8 value); +u16 smbus_read_block(u32 smbus_dev, u8 addr, u8 offset, u8 *data, u16 max_size); void smbus_delay(void);
#endif /* DEVICE_EARLY_SMBUS_H */ diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c index 79bda1e..37c8032 100644 --- a/src/lib/spd_bin.c +++ b/src/lib/spd_bin.c @@ -15,6 +15,7 @@
#include <arch/byteorder.h> #include <cbfs.h> +#include <lib.h> #include <console/console.h> #include <spd_bin.h> #include <string.h> @@ -128,18 +129,32 @@ static void smbus_read_spd(u8 *spd, u8 addr) { u16 i; - u8 step = 1;
- if (IS_ENABLED(CONFIG_SPD_READ_BY_WORD)) - step = sizeof(uint16_t); + if (IS_ENABLED(CONFIG_SPD_READ_BY_BLOCK)) { + u16 read_size;
- for (i = 0; i < SPD_PAGE_LEN; i += step) { + for (i = 0; i < SPD_PAGE_LEN;) { + read_size = smbus_read_block(0, addr, i, &spd[i], + SPD_PAGE_LEN - i); + i += read_size; + if (read_size == 0) + break; + } + } else { + u8 step = 1; + if (IS_ENABLED(CONFIG_SPD_READ_BY_WORD)) - ((u16*)spd)[i / sizeof(uint16_t)] = - smbus_read_word(0, addr, i); - else - spd[i] = smbus_read_byte(0, addr, i); + step = sizeof(uint16_t); + + for (i = 0; i < SPD_PAGE_LEN; i += step) { + if (IS_ENABLED(CONFIG_SPD_READ_BY_WORD)) + ((u16 *)spd)[i / sizeof(uint16_t)] = + smbus_read_word(0, addr, i); + else + spd[i] = smbus_read_byte(0, addr, i); + } } + }
static void get_spd(u8 *spd, u8 addr) diff --git a/src/soc/intel/common/block/smbus/smbus_early.c b/src/soc/intel/common/block/smbus/smbus_early.c index 9e6afc4..c244eeb 100644 --- a/src/soc/intel/common/block/smbus/smbus_early.c +++ b/src/soc/intel/common/block/smbus/smbus_early.c @@ -46,6 +46,15 @@ return smbus_read8(SMBUS_IO_BASE, addr, offset); }
+u16 smbus_read_block(u32 smbus_dev, u8 addr, u8 offset, u8 *data, u16 max_size) +{ + unsigned int read_size = 0; + if (smbus_read_block_data(SMBUS_IO_BASE, addr, offset, data, + max_size, &read_size) != 0) + return 0; + return read_size; +} + u8 smbus_write_byte(u32 smbus_dev, u8 addr, u8 offset, u8 value) { return smbus_write8(SMBUS_IO_BASE, addr, offset, value); diff --git a/src/soc/intel/common/block/smbus/smbuslib.c b/src/soc/intel/common/block/smbus/smbuslib.c index 0d3901f..e1a57af 100644 --- a/src/soc/intel/common/block/smbus/smbuslib.c +++ b/src/soc/intel/common/block/smbus/smbuslib.c @@ -46,6 +46,21 @@ return -1; }
+static int smbus_wait_till_byte_done(u16 smbus_base) +{ + struct stopwatch sw; + unsigned char byte; + + stopwatch_init_msecs_expire(&sw, SMBUS_TIMEOUT); + do { + byte = inb(smbus_base + SMBHSTSTAT); + if (!((byte & 1) || (byte & ~((1 << 6) | (1 << 0))) == 0) || + (byte & (1 << 7))) + return 0; + } while (!stopwatch_expired(&sw)); + return -1; +} + int smbus_read8(unsigned int smbus_base, unsigned int device, unsigned int address) { @@ -176,3 +191,116 @@
return data; } + +/* + * Read from SMBus using the Block Data command. + * Data is stored in the @data variable up to @max_size bytes. + * The number of bytes read will be returned in @read_size argument. + * Returns 0 if successful. + */ +int smbus_read_block_data(unsigned int smbus_base, unsigned int device, + unsigned int address, unsigned char *data, unsigned int max_size, + unsigned int *read_size) +{ + unsigned char global_status_register; + unsigned char count; + unsigned int i; + + if (smbus_wait_till_ready(smbus_base) < 0) + return SMBUS_WAIT_UNTIL_READY_TIMEOUT; + + /* Set up transaction */ + /* Disable interrupts */ + outb(inb(smbus_base + SMBHSTCTL) & ~1, smbus_base + SMBHSTCTL); + /* Set the device I'm talking to */ + outb(((device & 0x7f) << 1) | 1, smbus_base + SMBXMITADD); + /* Set the command/address... */ + outb(address & 0xff, smbus_base + SMBHSTCMD); + if (IS_ENABLED(CONFIG_SMBUS_BLOCK_READ_SUPPORTS_E32B)) { + /* Enable E32B */ + outb(inb(smbus_base + SMBHSTAUXC) | (1 << 1), + smbus_base + SMBHSTAUXC); + } else { + /* Disable E32B */ + outb(inb(smbus_base + SMBHSTAUXC) & ~(1 << 1), + smbus_base + SMBHSTAUXC); + } + + /* Set up for a block data read */ + outb((inb(smbus_base + SMBHSTCTL) & 0xe3) | (0x5 << 2), + (smbus_base + SMBHSTCTL)); + /* Clear any lingering errors, so the transaction will run */ + outb(inb(smbus_base + SMBHSTSTAT), smbus_base + SMBHSTSTAT); + + /* Start the command */ + outb((inb(smbus_base + SMBHSTCTL) | 0x40), + smbus_base + SMBHSTCTL); + + /* Read results of transaction */ + if (IS_ENABLED(CONFIG_SMBUS_BLOCK_READ_SUPPORTS_E32B)) { + /* Poll for transaction completion */ + if (smbus_wait_till_done(smbus_base) < 0) + return SMBUS_WAIT_UNTIL_DONE_TIMEOUT; + + global_status_register = inb(smbus_base + SMBHSTSTAT); + /* Ignore the "In Use" status... */ + if ((global_status_register & ~(3 << 5)) != (1 << 1)) + return SMBUS_ERROR; + + count = inb(smbus_base + SMBHSTDAT0); + if (count < 1 || count > 32) + return SMBUS_ERROR; + for (i = 0; i < count && i < max_size; i++) + data[i] = inb(smbus_base + SMBHSTHBD); + + *read_size = i; + + /* Disable E32B */ + outb(inb(smbus_base + SMBHSTAUXC) & ~(1 << 1), + smbus_base + SMBHSTAUXC); + } else { + i = 0; + while (i < max_size) { + /* Poll for transaction completion */ + if (smbus_wait_till_byte_done(smbus_base) < 0) { + /* Recover idle status of controller */ + while (inb(smbus_base + SMBHSTSTAT) & 1) + outb(1 << 7, smbus_base + SMBHSTSTAT); + return SMBUS_WAIT_UNTIL_DONE_TIMEOUT; + } + + /* Check if we're done and no more bytes are returned */ + global_status_register = inb(smbus_base + SMBHSTSTAT); + if ((global_status_register & (1 << 7 | 1 << 0)) == 0) + break; + + if (i == 0 && + IS_ENABLED(CONFIG_SMBUS_BLOCK_READ_DAT0_QUIRK)) { + /* Contrary to what the datasheet says, the DAT0 + register contains the first byte of data + instead of containing the number of bytes + being returned */ + data[i++] = inb(smbus_base + SMBHSTDAT0); + if (i >= max_size) + break; + } + data[i++] = inb(smbus_base + SMBHSTHBD); + + /* Clear the Byte Done bit */ + outb(1 << 7, smbus_base + SMBHSTSTAT); + } + + /* In case more bytes are available, ignore the rest and + recover idle status of the controller */ + while (inb(smbus_base + SMBHSTSTAT) & 1) + outb(1 << 7, smbus_base + SMBHSTSTAT); + + /* Ignore the "In Use" status... */ + global_status_register = inb(smbus_base + SMBHSTSTAT); + if ((global_status_register & ~(3 << 5)) != (1 << 1)) + return SMBUS_ERROR; + *read_size = i; + } + + return 0; +} diff --git a/src/soc/intel/common/block/smbus/smbuslib.h b/src/soc/intel/common/block/smbus/smbuslib.h index 05dafe9..1ec99fd 100644 --- a/src/soc/intel/common/block/smbus/smbuslib.h +++ b/src/soc/intel/common/block/smbus/smbuslib.h @@ -27,6 +27,9 @@ #define SMBHSTCMD 0x3 #define SMBXMITADD 0x4 #define SMBHSTDAT0 0x5 +#define SMBHSTDAT1 0x6 +#define SMBHSTHBD 0x7 +#define SMBHSTAUXC 0xD
#define SMBUS_TIMEOUT 15 /* 15ms */
@@ -36,5 +39,8 @@ unsigned int address, unsigned int data); int smbus_read16(unsigned int smbus_base, unsigned int device, unsigned int address); +int smbus_read_block_data(unsigned int smbus_base, unsigned int device, + unsigned int address, unsigned char *data, + unsigned int max_size, unsigned int *read_size);
#endif /* SOC_INTEL_COMMON_BLOCK_SMBUS__LIB_H */
-
Youness Alaoui (Code Review)