Change in coreboot[master]: [WIP] lib/spd_bim,soc/intel/common: Move get_spd_smbus()

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38121 )

Change subject: [WIP] lib/spd_bim,soc/intel/common: Move get_spd_smbus()
......................................................................

[WIP] lib/spd_bim,soc/intel/common: Move get_spd_smbus()

Only smbuslib.c and spd_bin.c share the same prototypes for SMBUS
functions. Therefore, get_spd_smbus() currently only works with
soc/intel/.../smbuslib.c and can be implemented there locally.

This allows removal of <device/early_smbus.h>.

Change-Id: Ic2d9d83ede6388a01d40c6e4768f6bb6bf899c00
Signed-off-by: Kyösti Mälkki 
---
D src/include/device/early_smbus.h
M src/lib/spd_bin.c
M src/soc/intel/common/block/smbus/smbuslib.c
3 files changed, 73 insertions(+), 128 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/38121/1

diff --git a/src/include/device/early_smbus.h b/src/include/device/early_smbus.h
deleted file mode 100644
index 015be67..0000000
--- a/src/include/device/early_smbus.h
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * This file is part of the coreboot project.
- *
- * Copyright (C) 2011  Alexandru Gagniuc 
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-/**
- * @file device/early_smbus.h
- *
- * This file defines a common API for accessing the SMBus during early
- * initialization. It defines the prototypes for common SMBus functions. The
- * actual implementations are hardware-dependent.
- *
- * The first parameter of all SMBus functions take a u32 value smbus_dev which
- * represents some information on how to access the device, and is
- * implementation defined. Usually, it just contains the IO base for the smbus.
- * To get this argument @ref smbus_get_device() can be used.
- *
- * The header only defines the prototypes. Several steps are needed to use
- * these:
- *
- * 1. Include this header
- * @code{.c}
- *	#include <device/early_smbus.h>
- * @endcode
- *
- * 2. Implement early_smbus.c for the hardware, or find a compatible
- * implementation.
- *
- * 3. Link against the file that implements these functions. In the Makefile.inc
- * of the chipset, add:
- * @code
- *	romstage-y += ./path/to/early_smbus.c
- * @endcode
- */
-
-#ifndef DEVICE_EARLY_SMBUS_H
-#define DEVICE_EARLY_SMBUS_H
-
-#include <stdint.h>
-
-u16 smbus_read_word(u8 addr, u8 offset);
-u8 smbus_read_byte(u8 addr, u8 offset);
-u8 smbus_write_byte(u8 addr, u8 offset, u8 value);
-
-#endif				/* DEVICE_EARLY_SMBUS_H */
diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c
index b022200..9e625b5 100644
--- a/src/lib/spd_bin.c
+++ b/src/lib/spd_bin.c
@@ -17,11 +17,8 @@
 #include <console/console.h>
 #include <spd_bin.h>
 #include <string.h>
-#include <device/early_smbus.h>
 #include <device/dram/ddr3.h>

-static u8 spd_data[CONFIG_DIMM_MAX * CONFIG_DIMM_SPD_SIZE];
-
 void dump_spd_info(struct spd_block *blk)
 {
    u8 i;
@@ -189,20 +186,6 @@
    }
 }

-static void update_spd_len(struct spd_block *blk)
-{
-	u8 i, j = 0;
-	for (i = 0 ; i < CONFIG_DIMM_MAX; i++)
-		if (blk->spd_array[i] != NULL)
-			j |= blk->spd_array[i][SPD_DRAM_TYPE];
-
-	/* If spd used is DDR4, then its length is 512 byte. */
-	if (j == SPD_DRAM_DDR4)
-		blk->len = SPD_PAGE_LEN_DDR4;
-	else
-		blk->len = SPD_PAGE_LEN;
-}
-
 int get_spd_cbfs_rdev(struct region_device *spd_rdev, u8 spd_index)
 {
    struct cbfsf fh;
@@ -216,57 +199,6 @@
    						CONFIG_DIMM_SPD_SIZE);
 }

-static void smbus_read_spd(u8 *spd, u8 addr)
-{
-	u16 i;
-	u8 step = 1;
-
-	if (CONFIG(SPD_READ_BY_WORD))
-		step = sizeof(uint16_t);
-
-	for (i = 0; i < SPD_PAGE_LEN; i += step) {
-		if (CONFIG(SPD_READ_BY_WORD))
-			((u16*)spd)[i / sizeof(uint16_t)] =
-				 smbus_read_word(addr, i);
-		else
-			spd[i] = smbus_read_byte(addr, i);
-	}
-}
-
-static void get_spd(u8 *spd, u8 addr)
-{
-	if (smbus_read_byte(addr, 0) == 0xff) {
-		printk(BIOS_INFO, "No memory dimm at address %02X\n",
-			addr << 1);
-		/* Make sure spd is zeroed if dimm doesn't exist. */
-		memset(spd, 0, CONFIG_DIMM_SPD_SIZE);
-		return;
-	}
-	smbus_read_spd(spd, addr);
-
-	/* Check if module is DDR4, DDR4 spd is 512 byte. */
-	if (spd[SPD_DRAM_TYPE] == SPD_DRAM_DDR4 &&
-		CONFIG_DIMM_SPD_SIZE > SPD_PAGE_LEN) {
-		/* Switch to page 1 */
-		smbus_write_byte(SPD_PAGE_1, 0, 0);
-		smbus_read_spd(spd + SPD_PAGE_LEN, addr);
-		/* Restore to page 0 */
-		smbus_write_byte(SPD_PAGE_0, 0, 0);
-	}
-}
-
-void get_spd_smbus(struct spd_block *blk)
-{
-	u8 i;
-	for (i = 0 ; i < CONFIG_DIMM_MAX; i++) {
-		get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE],
-			blk->addr_map[i]);
-		blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE];
-	}
-
-	update_spd_len(blk);
-}
-
 #if CONFIG_DIMM_SPD_SIZE == 128
 int read_ddr3_spd_from_cbfs(u8 *buf, int idx)
 {
diff --git a/src/soc/intel/common/block/smbus/smbuslib.c b/src/soc/intel/common/block/smbus/smbuslib.c
index e2df730..0680ce2 100644
--- a/src/soc/intel/common/block/smbus/smbuslib.c
+++ b/src/soc/intel/common/block/smbus/smbuslib.c
@@ -14,8 +14,10 @@
  */

#include <arch/io.h>
-#include <device/early_smbus.h>
+#include <console/console.h>
 #include <device/smbus_def.h>
+#include <spd_bin.h>
+#include <string.h>
 #include <timer.h>
 #include "smbuslib.h"

@@ -176,17 +178,84 @@
    return data;
 }

-u16 smbus_read_word(u8 addr, u8 offset)
+static u16 smbus_read_word(u8 addr, u8 offset)
 {
    return do_smbus_read_word(SMBUS_IO_BASE, addr, offset);
 }

-u8 smbus_read_byte(u8 addr, u8 offset)
+static u8 smbus_read_byte(u8 addr, u8 offset)
 {
    return do_smbus_read_byte(SMBUS_IO_BASE, addr, offset);
 }

-u8 smbus_write_byte(u8 addr, u8 offset, u8 value)
+static u8 smbus_write_byte(u8 addr, u8 offset, u8 value)
 {
    return do_smbus_write_byte(SMBUS_IO_BASE, addr, offset, value);
 }
+
+static void update_spd_len(struct spd_block *blk)
+{
+	u8 i, j = 0;
+	for (i = 0 ; i < CONFIG_DIMM_MAX; i++)
+		if (blk->spd_array[i] != NULL)
+			j |= blk->spd_array[i][SPD_DRAM_TYPE];
+
+	/* If spd used is DDR4, then its length is 512 byte. */
+	if (j == SPD_DRAM_DDR4)
+		blk->len = SPD_PAGE_LEN_DDR4;
+	else
+		blk->len = SPD_PAGE_LEN;
+}
+
+static void smbus_read_spd(u8 *spd, u8 addr)
+{
+	u16 i;
+	u8 step = 1;
+
+	if (CONFIG(SPD_READ_BY_WORD))
+		step = sizeof(uint16_t);
+
+	for (i = 0; i < SPD_PAGE_LEN; i += step) {
+		if (CONFIG(SPD_READ_BY_WORD))
+			((u16*)spd)[i / sizeof(uint16_t)] =
+				 smbus_read_word(addr, i);
+		else
+			spd[i] = smbus_read_byte(addr, i);
+	}
+}
+
+static void get_spd(u8 *spd, u8 addr)
+{
+	if (smbus_read_byte(addr, 0) == 0xff) {
+		printk(BIOS_INFO, "No memory dimm at address %02X\n",
+			addr << 1);
+		/* Make sure spd is zeroed if dimm doesn't exist. */
+		memset(spd, 0, CONFIG_DIMM_SPD_SIZE);
+		return;
+	}
+	smbus_read_spd(spd, addr);
+
+	/* Check if module is DDR4, DDR4 spd is 512 byte. */
+	if (spd[SPD_DRAM_TYPE] == SPD_DRAM_DDR4 &&
+		CONFIG_DIMM_SPD_SIZE > SPD_PAGE_LEN) {
+		/* Switch to page 1 */
+		smbus_write_byte(SPD_PAGE_1, 0, 0);
+		smbus_read_spd(spd + SPD_PAGE_LEN, addr);
+		/* Restore to page 0 */
+		smbus_write_byte(SPD_PAGE_0, 0, 0);
+	}
+}
+
+static u8 spd_data[CONFIG_DIMM_MAX * CONFIG_DIMM_SPD_SIZE];
+
+void get_spd_smbus(struct spd_block *blk)
+{
+	u8 i;
+	for (i = 0 ; i < CONFIG_DIMM_MAX; i++) {
+		get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE],
+			blk->addr_map[i]);
+		blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE];
+	}
+
+	update_spd_len(blk);
+}

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38121 )

Change subject: [WIP] lib/spd_bim,soc/intel/common: Move get_spd_smbus()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38121/1/src/soc/intel/common/block/... 
File src/soc/intel/common/block/smbus/smbuslib.c:

https://review.coreboot.org/c/coreboot/+/38121/1/src/soc/intel/common/block/... 
PS1, Line 220: 			((u16*)spd)[i / sizeof(uint16_t)] =
"(foo*)" should be "(foo *)"

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38121 )

Change subject: [WIP] lib/spd_bim,soc/intel/common: Move get_spd_smbus()
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38121/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/smbus/smbuslib.c:

https://review.coreboot.org/c/coreboot/+/38121/2/src/soc/intel/common/block/... 
PS2, Line 220: 			((u16*)spd)[i / sizeof(uint16_t)] =
"(foo*)" should be "(foo *)"

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38121 )

Change subject: [WIP] lib/spd_bim,soc/intel/common: Move get_spd_smbus()
......................................................................

Patch Set 2:

So in the end, if one wanted to write a common (lib/) get_spd(),
what interface should be used? or how should it look like compared
to early_smbus?

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38121 )

Change subject: [WIP] lib/spd_bim,soc/intel/common: Move get_spd_smbus()
......................................................................

Patch Set 2:
> Patch Set 2:
>
> So in the end, if one wanted to write a common (lib/) get_spd(),
> what interface should be used? or how should it look like compared
> to early_smbus?

I am trying to figure it out and consider the possibility of i2c multiplexers as I do it. I believe we agreed that host controller (IO/MMIO) base address and slave device (I2C) address that we currently pass is not sufficient with tree-like topology that smbus/i2c might appear in.

In my vision, the low level functions do_smbus_read|write() would keep their current parameters. The API for slave device would oass 'const struct device *' and derive the required host base and multiplex configuration from the topology.

Change in coreboot[master]: lib/spd_bin,soc/intel/common: Move get_spd_smbus()

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38121

to look at the new patch set (#3).

Change subject: lib/spd_bin,soc/intel/common: Move get_spd_smbus()
......................................................................

lib/spd_bin,soc/intel/common: Move get_spd_smbus()

Only smbuslib.c and spd_bin.c share the same prototypes for SMBUS
functions. Therefore, get_spd_smbus() currently only works with
soc/intel/.../smbuslib.c and can be implemented there locally.

This allows removal of <device/early_smbus.h>.

Change-Id: Ic2d9d83ede6388a01d40c6e4768f6bb6bf899c00
Signed-off-by: Kyösti Mälkki 
---
D src/include/device/early_smbus.h
M src/lib/spd_bin.c
M src/soc/intel/common/block/smbus/smbuslib.c
3 files changed, 73 insertions(+), 128 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/38121/3

Change in coreboot[master]: lib/spd_bin,soc/intel/common: Move get_spd_smbus()

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38121 )

Change subject: lib/spd_bin,soc/intel/common: Move get_spd_smbus()
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38121/3/src/soc/intel/common/block/... 
File src/soc/intel/common/block/smbus/smbuslib.c:

https://review.coreboot.org/c/coreboot/+/38121/3/src/soc/intel/common/block/... 
PS3, Line 220: 			((u16*)spd)[i / sizeof(uint16_t)] =
"(foo*)" should be "(foo *)"

Change in coreboot[master]: lib/spd_bin,soc/intel/common: Move get_spd_smbus()

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38121 )

Change subject: lib/spd_bin,soc/intel/common: Move get_spd_smbus()
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/38121/3/src/soc/intel/common/block/... 
File src/soc/intel/common/block/smbus/smbuslib.c:

https://review.coreboot.org/c/coreboot/+/38121/3/src/soc/intel/common/block/... 
PS3, Line 220: 			((u16*)spd)[i / sizeof(uint16_t)] =
> "(foo*)" should be "(foo *)"

Meh, the code should be reflowed as well.

Change in coreboot[master]: lib/spd_bin,soc/intel/common: Move get_spd_smbus()

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38121 )

Change subject: lib/spd_bin,soc/intel/common: Move get_spd_smbus()
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38121/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/smbus/smbuslib.c:

https://review.coreboot.org/c/coreboot/+/38121/4/src/soc/intel/common/block/... 
PS4, Line 220: 			((u16*)spd)[i / sizeof(uint16_t)] =
"(foo*)" should be "(foo *)"

Change in coreboot[master]: lib/spd_bin,soc/intel/common: Move get_spd_smbus()

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38121 )

Change subject: lib/spd_bin,soc/intel/common: Move get_spd_smbus()
......................................................................

lib/spd_bin,soc/intel/common: Move get_spd_smbus()

Only smbuslib.c and spd_bin.c share the same prototypes for SMBUS
functions. Therefore, get_spd_smbus() currently only works with
soc/intel/.../smbuslib.c and can be implemented there locally.

This allows removal of <device/early_smbus.h>.

Change-Id: Ic2d9d83ede6388a01d40c6e4768f6bb6bf899c00
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38121
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
D src/include/device/early_smbus.h
M src/lib/spd_bin.c
M src/soc/intel/common/block/smbus/smbuslib.c
3 files changed, 73 insertions(+), 128 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/include/device/early_smbus.h b/src/include/device/early_smbus.h
deleted file mode 100644
index 015be67..0000000
--- a/src/include/device/early_smbus.h
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * This file is part of the coreboot project.
- *
- * Copyright (C) 2011  Alexandru Gagniuc 
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-/**
- * @file device/early_smbus.h
- *
- * This file defines a common API for accessing the SMBus during early
- * initialization. It defines the prototypes for common SMBus functions. The
- * actual implementations are hardware-dependent.
- *
- * The first parameter of all SMBus functions take a u32 value smbus_dev which
- * represents some information on how to access the device, and is
- * implementation defined. Usually, it just contains the IO base for the smbus.
- * To get this argument @ref smbus_get_device() can be used.
- *
- * The header only defines the prototypes. Several steps are needed to use
- * these:
- *
- * 1. Include this header
- * @code{.c}
- *	#include <device/early_smbus.h>
- * @endcode
- *
- * 2. Implement early_smbus.c for the hardware, or find a compatible
- * implementation.
- *
- * 3. Link against the file that implements these functions. In the Makefile.inc
- * of the chipset, add:
- * @code
- *	romstage-y += ./path/to/early_smbus.c
- * @endcode
- */
-
-#ifndef DEVICE_EARLY_SMBUS_H
-#define DEVICE_EARLY_SMBUS_H
-
-#include <stdint.h>
-
-u16 smbus_read_word(u8 addr, u8 offset);
-u8 smbus_read_byte(u8 addr, u8 offset);
-u8 smbus_write_byte(u8 addr, u8 offset, u8 value);
-
-#endif				/* DEVICE_EARLY_SMBUS_H */
diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c
index b022200..9e625b5 100644
--- a/src/lib/spd_bin.c
+++ b/src/lib/spd_bin.c
@@ -17,11 +17,8 @@
 #include <console/console.h>
 #include <spd_bin.h>
 #include <string.h>
-#include <device/early_smbus.h>
 #include <device/dram/ddr3.h>

-static u8 spd_data[CONFIG_DIMM_MAX * CONFIG_DIMM_SPD_SIZE];
-
 void dump_spd_info(struct spd_block *blk)
 {
    u8 i;
@@ -189,20 +186,6 @@
    }
 }

-static void update_spd_len(struct spd_block *blk)
-{
-	u8 i, j = 0;
-	for (i = 0 ; i < CONFIG_DIMM_MAX; i++)
-		if (blk->spd_array[i] != NULL)
-			j |= blk->spd_array[i][SPD_DRAM_TYPE];
-
-	/* If spd used is DDR4, then its length is 512 byte. */
-	if (j == SPD_DRAM_DDR4)
-		blk->len = SPD_PAGE_LEN_DDR4;
-	else
-		blk->len = SPD_PAGE_LEN;
-}
-
 int get_spd_cbfs_rdev(struct region_device *spd_rdev, u8 spd_index)
 {
    struct cbfsf fh;
@@ -216,57 +199,6 @@
    						CONFIG_DIMM_SPD_SIZE);
 }

-static void smbus_read_spd(u8 *spd, u8 addr)
-{
-	u16 i;
-	u8 step = 1;
-
-	if (CONFIG(SPD_READ_BY_WORD))
-		step = sizeof(uint16_t);
-
-	for (i = 0; i < SPD_PAGE_LEN; i += step) {
-		if (CONFIG(SPD_READ_BY_WORD))
-			((u16*)spd)[i / sizeof(uint16_t)] =
-				 smbus_read_word(addr, i);
-		else
-			spd[i] = smbus_read_byte(addr, i);
-	}
-}
-
-static void get_spd(u8 *spd, u8 addr)
-{
-	if (smbus_read_byte(addr, 0) == 0xff) {
-		printk(BIOS_INFO, "No memory dimm at address %02X\n",
-			addr << 1);
-		/* Make sure spd is zeroed if dimm doesn't exist. */
-		memset(spd, 0, CONFIG_DIMM_SPD_SIZE);
-		return;
-	}
-	smbus_read_spd(spd, addr);
-
-	/* Check if module is DDR4, DDR4 spd is 512 byte. */
-	if (spd[SPD_DRAM_TYPE] == SPD_DRAM_DDR4 &&
-		CONFIG_DIMM_SPD_SIZE > SPD_PAGE_LEN) {
-		/* Switch to page 1 */
-		smbus_write_byte(SPD_PAGE_1, 0, 0);
-		smbus_read_spd(spd + SPD_PAGE_LEN, addr);
-		/* Restore to page 0 */
-		smbus_write_byte(SPD_PAGE_0, 0, 0);
-	}
-}
-
-void get_spd_smbus(struct spd_block *blk)
-{
-	u8 i;
-	for (i = 0 ; i < CONFIG_DIMM_MAX; i++) {
-		get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE],
-			blk->addr_map[i]);
-		blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE];
-	}
-
-	update_spd_len(blk);
-}
-
 #if CONFIG_DIMM_SPD_SIZE == 128
 int read_ddr3_spd_from_cbfs(u8 *buf, int idx)
 {
diff --git a/src/soc/intel/common/block/smbus/smbuslib.c b/src/soc/intel/common/block/smbus/smbuslib.c
index e2df730..0680ce2 100644
--- a/src/soc/intel/common/block/smbus/smbuslib.c
+++ b/src/soc/intel/common/block/smbus/smbuslib.c
@@ -14,8 +14,10 @@
  */

#include <arch/io.h>
-#include <device/early_smbus.h>
+#include <console/console.h>
 #include <device/smbus_def.h>
+#include <spd_bin.h>
+#include <string.h>
 #include <timer.h>
 #include "smbuslib.h"

@@ -176,17 +178,84 @@
    return data;
 }

-u16 smbus_read_word(u8 addr, u8 offset)
+static u16 smbus_read_word(u8 addr, u8 offset)
 {
    return do_smbus_read_word(SMBUS_IO_BASE, addr, offset);
 }

-u8 smbus_read_byte(u8 addr, u8 offset)
+static u8 smbus_read_byte(u8 addr, u8 offset)
 {
    return do_smbus_read_byte(SMBUS_IO_BASE, addr, offset);
 }

-u8 smbus_write_byte(u8 addr, u8 offset, u8 value)
+static u8 smbus_write_byte(u8 addr, u8 offset, u8 value)
 {
    return do_smbus_write_byte(SMBUS_IO_BASE, addr, offset, value);
 }
+
+static void update_spd_len(struct spd_block *blk)
+{
+	u8 i, j = 0;
+	for (i = 0 ; i < CONFIG_DIMM_MAX; i++)
+		if (blk->spd_array[i] != NULL)
+			j |= blk->spd_array[i][SPD_DRAM_TYPE];
+
+	/* If spd used is DDR4, then its length is 512 byte. */
+	if (j == SPD_DRAM_DDR4)
+		blk->len = SPD_PAGE_LEN_DDR4;
+	else
+		blk->len = SPD_PAGE_LEN;
+}
+
+static void smbus_read_spd(u8 *spd, u8 addr)
+{
+	u16 i;
+	u8 step = 1;
+
+	if (CONFIG(SPD_READ_BY_WORD))
+		step = sizeof(uint16_t);
+
+	for (i = 0; i < SPD_PAGE_LEN; i += step) {
+		if (CONFIG(SPD_READ_BY_WORD))
+			((u16*)spd)[i / sizeof(uint16_t)] =
+				 smbus_read_word(addr, i);
+		else
+			spd[i] = smbus_read_byte(addr, i);
+	}
+}
+
+static void get_spd(u8 *spd, u8 addr)
+{
+	if (smbus_read_byte(addr, 0) == 0xff) {
+		printk(BIOS_INFO, "No memory dimm at address %02X\n",
+			addr << 1);
+		/* Make sure spd is zeroed if dimm doesn't exist. */
+		memset(spd, 0, CONFIG_DIMM_SPD_SIZE);
+		return;
+	}
+	smbus_read_spd(spd, addr);
+
+	/* Check if module is DDR4, DDR4 spd is 512 byte. */
+	if (spd[SPD_DRAM_TYPE] == SPD_DRAM_DDR4 &&
+		CONFIG_DIMM_SPD_SIZE > SPD_PAGE_LEN) {
+		/* Switch to page 1 */
+		smbus_write_byte(SPD_PAGE_1, 0, 0);
+		smbus_read_spd(spd + SPD_PAGE_LEN, addr);
+		/* Restore to page 0 */
+		smbus_write_byte(SPD_PAGE_0, 0, 0);
+	}
+}
+
+static u8 spd_data[CONFIG_DIMM_MAX * CONFIG_DIMM_SPD_SIZE];
+
+void get_spd_smbus(struct spd_block *blk)
+{
+	u8 i;
+	for (i = 0 ; i < CONFIG_DIMM_MAX; i++) {
+		get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE],
+			blk->addr_map[i]);
+		blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE];
+	}
+
+	update_spd_len(blk);
+}