[coreboot-gerrit] Change in coreboot[master]: intel/common/smbus: Add support for reading SPD by block
Youness Alaoui (Code Review)
gerrit at coreboot.org
Thu Dec 21 17:41:42 CET 2017
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 at 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 */
--
To view, visit https://review.coreboot.org/22967
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I14882b646e7c0fa96cf38a698562cc20bbd41095
Gerrit-Change-Number: 22967
Gerrit-PatchSet: 1
Gerrit-Owner: Youness Alaoui <snifikino at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20171221/1362abe8/attachment-0001.html>
More information about the coreboot-gerrit
mailing list