Change in coreboot[master]: amdblocks/biosram: Force use of abstraction

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

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

Change subject: amdblocks/biosram: Force use of abstraction
......................................................................

amdblocks/biosram: Force use of abstraction

Hide the fundamental BIOSRAM accessors to force use of the
memory space via abstraction functions.

Change-Id: I774b6640cdd9873f52e446c4ca41b7c537a87883
Signed-off-by: Kyösti Mälkki 
---
M src/soc/amd/common/block/acpimmio/biosram.c
M src/soc/amd/common/block/acpimmio/mmio_util.c
M src/soc/amd/common/block/include/amdblocks/acpimmio.h
M src/soc/amd/common/block/include/amdblocks/biosram.h
4 files changed, 48 insertions(+), 51 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/37862/1

diff --git a/src/soc/amd/common/block/acpimmio/biosram.c b/src/soc/amd/common/block/acpimmio/biosram.c
index e33f02d..814fdf3 100644
--- a/src/soc/amd/common/block/acpimmio/biosram.c
+++ b/src/soc/amd/common/block/acpimmio/biosram.c
@@ -11,9 +11,55 @@
  * GNU General Public License for more details.
  */

-#include <cbmem.h>
-#include <amdblocks/acpimmio.h>
+#include <amdblocks/acpimmio_map.h>
 #include <amdblocks/biosram.h>
+#include <cbmem.h>
+#include <device/mmio.h>
+#include <stdint.h>
+
+/* BiosRam Ranges at 0xfed80500 or I/O 0xcd4/0xcd5 */
+#define BIOSRAM_AP_ENTRY		0xe8 /* 8 bytes */
+#define BIOSRAM_CBMEM_TOP		0xf0 /* 4 bytes */
+#define BIOSRAM_UMA_SIZE		0xf4 /* 4 bytes */
+#define BIOSRAM_UMA_BASE		0xf8 /* 8 bytes */
+
+static uint8_t biosram_read8(uint8_t reg)
+{
+	return read8((void *)(ACPIMMIO_BIOSRAM_BASE + reg));
+}
+
+static void biosram_write8(uint8_t reg, uint8_t value)
+{
+	write8((void *)(ACPIMMIO_BIOSRAM_BASE + reg), value);
+}
+
+static uint16_t biosram_read16(uint8_t reg) /* Must be 1 byte at a time */
+{
+	return (biosram_read8(reg + sizeof(uint8_t)) << 8 | biosram_read8(reg));
+}
+
+static uint32_t biosram_read32(uint8_t reg)
+{
+	uint32_t value = biosram_read16(reg + sizeof(uint16_t)) << 16;
+	return value | biosram_read16(reg);
+}
+
+static void biosram_write16(uint8_t reg, uint16_t value)
+{
+	biosram_write8(reg, value & 0xff);
+	value >>= 8;
+	biosram_write8(reg + sizeof(uint8_t), value & 0xff);
+}
+
+static void biosram_write32(uint8_t reg, uint32_t value)
+{
+	biosram_write16(reg, value & 0xffff);
+	value >>= 16;
+	biosram_write16(reg + sizeof(uint16_t), value & 0xffff);
+}
+
+
+/* Access to BIOSRAM is only allowed through the abstractions below. */

void *get_ap_entry_ptr(void)
 {
diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c
index a589ef5..202e47a 100644
--- a/src/soc/amd/common/block/acpimmio/mmio_util.c
+++ b/src/soc/amd/common/block/acpimmio/mmio_util.c
@@ -72,28 +72,3 @@
    value >>= 16;
    pm_io_write16(reg + sizeof(uint16_t), value & 0xffff);
 }
-
-uint16_t biosram_read16(uint8_t reg) /* Must be 1 byte at a time */
-{
-	return (biosram_read8(reg + sizeof(uint8_t)) << 8 | biosram_read8(reg));
-}
-
-uint32_t biosram_read32(uint8_t reg)
-{
-	uint32_t value = biosram_read16(reg + sizeof(uint16_t)) << 16;
-	return value | biosram_read16(reg);
-}
-
-void biosram_write16(uint8_t reg, uint16_t value)
-{
-	biosram_write8(reg, value & 0xff);
-	value >>= 8;
-	biosram_write8(reg + sizeof(uint8_t), value & 0xff);
-}
-
-void biosram_write32(uint8_t reg, uint32_t value)
-{
-	biosram_write16(reg, value & 0xffff);
-	value >>= 16;
-	biosram_write16(reg + sizeof(uint16_t), value & 0xffff);
-}
diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h
index c441ab8..46d088f 100644
--- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h
+++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h
@@ -116,14 +116,6 @@
 void pm_io_write16(uint8_t reg, uint16_t value);
 void pm_io_write32(uint8_t reg, uint32_t value);

-
-/* Access BIOS RAM storage at 0xfed80500 */
-uint16_t biosram_read16(uint8_t reg);
-uint32_t biosram_read32(uint8_t reg);
-void biosram_write16(uint8_t reg, uint16_t value);
-void biosram_write32(uint8_t reg, uint32_t value);
-
-
 static inline uint8_t sm_pci_read8(uint8_t reg)
 {
    return read8((void *)(ACPIMMIO_SM_PCI_BASE + reg));
@@ -214,16 +206,6 @@
    write32((void *)(ACPIMMIO_PMIO_BASE + reg), value);
 }

-static inline uint8_t biosram_read8(uint8_t reg)
-{
-	return read8((void *)(ACPIMMIO_BIOSRAM_BASE + reg));
-}
-
-static inline void biosram_write8(uint8_t reg, uint8_t value)
-{
-	write8((void *)(ACPIMMIO_BIOSRAM_BASE + reg), value);
-}
-
 static inline uint8_t acpi_read8(uint8_t reg)
 {
    return read8((void *)(ACPIMMIO_ACPI_BASE + reg));
diff --git a/src/soc/amd/common/block/include/amdblocks/biosram.h b/src/soc/amd/common/block/include/amdblocks/biosram.h
index db28310..4bfd629 100644
--- a/src/soc/amd/common/block/include/amdblocks/biosram.h
+++ b/src/soc/amd/common/block/include/amdblocks/biosram.h
@@ -16,12 +16,6 @@

#include <stdint.h>

-/* BiosRam Ranges at 0xfed80500 or I/O 0xcd4/0xcd5 */
-#define BIOSRAM_AP_ENTRY		0xe8 /* 8 bytes */
-#define BIOSRAM_CBMEM_TOP		0xf0 /* 4 bytes */
-#define BIOSRAM_UMA_SIZE		0xf4 /* 4 bytes */
-#define BIOSRAM_UMA_BASE		0xf8 /* 8 bytes */
-
 /* Returns the bootblock C entry point for APs */
 void *get_ap_entry_ptr(void);
 /* Used by BSP to store the bootblock entry point for APs */

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37862 )

Change subject: amdblocks/biosram: Force use of abstraction
......................................................................

Patch Set 1:

From what are we trying to protect here? From the unintended change of reserved (for boot process use) BIOSRAM addresses?

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

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

Change subject: amdblocks/biosram: Force use of abstraction
......................................................................

Patch Set 2:
> Patch Set 1:
>
> From what are we trying to protect here? From the unintended change of reserved (for boot process use) BIOSRAM addresses?

Just the general concept of exposing as little as possible to keep it simple to maintain.

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

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

Change subject: amdblocks/biosram: Force use of abstraction
......................................................................

Patch Set 3:

Marshall, Richard: ping

Like with the discussion about GPIO banks, we should limit the exposure of the register (here, nvram memory) accessors.

[Angel Pons (Code Review)]

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

Change subject: amdblocks/biosram: Force use of abstraction
......................................................................

Patch Set 3: Code-Review+2

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37862 )

Change subject: amdblocks/biosram: Force use of abstraction
......................................................................

Patch Set 3: Code-Review+2

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37862 )

Change subject: amdblocks/biosram: Force use of abstraction
......................................................................

Patch Set 4: Code-Review+2

Hmm, I was thinking this had already landed.  Sorry for the delay; was on the road for the last couple of weeks.

I only have small nits.  Rather than ask for changes and delay this one, I'll just push a follow-on patch.

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

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

Change subject: amdblocks/biosram: Force use of abstraction
......................................................................

amdblocks/biosram: Force use of abstraction

Hide the fundamental BIOSRAM accessors to force use of the
memory space via abstraction functions.

Change-Id: I774b6640cdd9873f52e446c4ca41b7c537a87883
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37862
Reviewed-by: Marshall Dawson 
Reviewed-by: Angel Pons 
Reviewed-by: Richard Spiegel 
Tested-by: build bot (Jenkins) 
---
M src/soc/amd/common/block/acpimmio/biosram.c
M src/soc/amd/common/block/acpimmio/mmio_util.c
M src/soc/amd/common/block/include/amdblocks/acpimmio.h
M src/soc/amd/common/block/include/amdblocks/biosram.h
4 files changed, 48 insertions(+), 51 deletions(-)

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

diff --git a/src/soc/amd/common/block/acpimmio/biosram.c b/src/soc/amd/common/block/acpimmio/biosram.c
index e33f02d..814fdf3 100644
--- a/src/soc/amd/common/block/acpimmio/biosram.c
+++ b/src/soc/amd/common/block/acpimmio/biosram.c
@@ -11,9 +11,55 @@
  * GNU General Public License for more details.
  */

-#include <cbmem.h>
-#include <amdblocks/acpimmio.h>
+#include <amdblocks/acpimmio_map.h>
 #include <amdblocks/biosram.h>
+#include <cbmem.h>
+#include <device/mmio.h>
+#include <stdint.h>
+
+/* BiosRam Ranges at 0xfed80500 or I/O 0xcd4/0xcd5 */
+#define BIOSRAM_AP_ENTRY		0xe8 /* 8 bytes */
+#define BIOSRAM_CBMEM_TOP		0xf0 /* 4 bytes */
+#define BIOSRAM_UMA_SIZE		0xf4 /* 4 bytes */
+#define BIOSRAM_UMA_BASE		0xf8 /* 8 bytes */
+
+static uint8_t biosram_read8(uint8_t reg)
+{
+	return read8((void *)(ACPIMMIO_BIOSRAM_BASE + reg));
+}
+
+static void biosram_write8(uint8_t reg, uint8_t value)
+{
+	write8((void *)(ACPIMMIO_BIOSRAM_BASE + reg), value);
+}
+
+static uint16_t biosram_read16(uint8_t reg) /* Must be 1 byte at a time */
+{
+	return (biosram_read8(reg + sizeof(uint8_t)) << 8 | biosram_read8(reg));
+}
+
+static uint32_t biosram_read32(uint8_t reg)
+{
+	uint32_t value = biosram_read16(reg + sizeof(uint16_t)) << 16;
+	return value | biosram_read16(reg);
+}
+
+static void biosram_write16(uint8_t reg, uint16_t value)
+{
+	biosram_write8(reg, value & 0xff);
+	value >>= 8;
+	biosram_write8(reg + sizeof(uint8_t), value & 0xff);
+}
+
+static void biosram_write32(uint8_t reg, uint32_t value)
+{
+	biosram_write16(reg, value & 0xffff);
+	value >>= 16;
+	biosram_write16(reg + sizeof(uint16_t), value & 0xffff);
+}
+
+
+/* Access to BIOSRAM is only allowed through the abstractions below. */

void *get_ap_entry_ptr(void)
 {
diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c
index a589ef5..202e47a 100644
--- a/src/soc/amd/common/block/acpimmio/mmio_util.c
+++ b/src/soc/amd/common/block/acpimmio/mmio_util.c
@@ -72,28 +72,3 @@
    value >>= 16;
    pm_io_write16(reg + sizeof(uint16_t), value & 0xffff);
 }
-
-uint16_t biosram_read16(uint8_t reg) /* Must be 1 byte at a time */
-{
-	return (biosram_read8(reg + sizeof(uint8_t)) << 8 | biosram_read8(reg));
-}
-
-uint32_t biosram_read32(uint8_t reg)
-{
-	uint32_t value = biosram_read16(reg + sizeof(uint16_t)) << 16;
-	return value | biosram_read16(reg);
-}
-
-void biosram_write16(uint8_t reg, uint16_t value)
-{
-	biosram_write8(reg, value & 0xff);
-	value >>= 8;
-	biosram_write8(reg + sizeof(uint8_t), value & 0xff);
-}
-
-void biosram_write32(uint8_t reg, uint32_t value)
-{
-	biosram_write16(reg, value & 0xffff);
-	value >>= 16;
-	biosram_write16(reg + sizeof(uint16_t), value & 0xffff);
-}
diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h
index c441ab8..46d088f 100644
--- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h
+++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h
@@ -116,14 +116,6 @@
 void pm_io_write16(uint8_t reg, uint16_t value);
 void pm_io_write32(uint8_t reg, uint32_t value);

-
-/* Access BIOS RAM storage at 0xfed80500 */
-uint16_t biosram_read16(uint8_t reg);
-uint32_t biosram_read32(uint8_t reg);
-void biosram_write16(uint8_t reg, uint16_t value);
-void biosram_write32(uint8_t reg, uint32_t value);
-
-
 static inline uint8_t sm_pci_read8(uint8_t reg)
 {
    return read8((void *)(ACPIMMIO_SM_PCI_BASE + reg));
@@ -214,16 +206,6 @@
    write32((void *)(ACPIMMIO_PMIO_BASE + reg), value);
 }

-static inline uint8_t biosram_read8(uint8_t reg)
-{
-	return read8((void *)(ACPIMMIO_BIOSRAM_BASE + reg));
-}
-
-static inline void biosram_write8(uint8_t reg, uint8_t value)
-{
-	write8((void *)(ACPIMMIO_BIOSRAM_BASE + reg), value);
-}
-
 static inline uint8_t acpi_read8(uint8_t reg)
 {
    return read8((void *)(ACPIMMIO_ACPI_BASE + reg));
diff --git a/src/soc/amd/common/block/include/amdblocks/biosram.h b/src/soc/amd/common/block/include/amdblocks/biosram.h
index db28310..4bfd629 100644
--- a/src/soc/amd/common/block/include/amdblocks/biosram.h
+++ b/src/soc/amd/common/block/include/amdblocks/biosram.h
@@ -16,12 +16,6 @@

#include <stdint.h>

-/* BiosRam Ranges at 0xfed80500 or I/O 0xcd4/0xcd5 */
-#define BIOSRAM_AP_ENTRY		0xe8 /* 8 bytes */
-#define BIOSRAM_CBMEM_TOP		0xf0 /* 4 bytes */
-#define BIOSRAM_UMA_SIZE		0xf4 /* 4 bytes */
-#define BIOSRAM_UMA_BASE		0xf8 /* 8 bytes */
-
 /* Returns the bootblock C entry point for APs */
 void *get_ap_entry_ptr(void);
 /* Used by BSP to store the bootblock entry point for APs */