Change in ...coreboot[master]: soc/cavium/cn81xx: Enable RNG for DRAM init

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31567

Change subject: soc/cavium/cn81xx: Enable RNG for DRAM init
......................................................................

soc/cavium/cn81xx: Enable RNG for DRAM init

The Cavium DRAM init might use the RNG for pattern generation.
Initialize it before running DRAM init.

Tested on OpenCellular Elgon.
The RNG generates non identical numbers.

Change-Id: I886f920e9941793fb76b56cc5a24a42e23b082e0
Signed-off-by: Patrick Rudolph 
---
M src/soc/cavium/cn81xx/sdram.c
1 file changed, 50 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/31567/1

diff --git a/src/soc/cavium/cn81xx/sdram.c b/src/soc/cavium/cn81xx/sdram.c
index 2342b04..f434265 100644
--- a/src/soc/cavium/cn81xx/sdram.c
+++ b/src/soc/cavium/cn81xx/sdram.c
@@ -3,6 +3,7 @@
  *
  * Copyright 2018       Facebook, Inc.
  * Copyright 2003-2017  Cavium Inc.  
+ * Copyright 2019       9elements Agency GmbH
  *
  * 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
@@ -28,17 +29,66 @@
 #include <libbdk-hal/bdk-utils.h>
 #include <libbdk-hal/bdk-l2c.h>
 #include <libdram/libdram-config.h>
+#include <soc/ecam.h>
+#include <device/pci_ops.h>
+#include <device/pci.h>

size_t sdram_size_mb(void)
 {
    return bdk_dram_get_size_mbytes(0);
 }

+/* Enable RNG for DRAM init */
+static void rnm_init(void)
+{
+	u64 *bar = NULL;
+
+	#define BDK_RNM_CTL_STATUS	0
+	#define BDK_RNM_RANDOM		0x100000
+
+	/* Bus numbers are hardcoded in ASIC */
+#ifdef __SIMPLE_DEVICE__
+	pci_devfn_t dev = pci_locate_device_on_bus(0xa018177d, 2);
+	if (dev == PCI_DEV_INVALID) {
+		printk(BIOS_ERR, "RNG: Failed to find PCI device\n");
+		return;
+	}
+
+	bar = (u64 *)ecam0_get_bar_val(dev, 0);
+#endif
+	if (!bar) {
+		printk(BIOS_ERR, "RNG: Failed to get BAR0\n");
+		return;
+	}
+
+	printk(BIOS_DEBUG, "RNG: BAR0 at %p\n", bar);
+
+	u64 reg = read64(&bar[BDK_RNM_CTL_STATUS]);
+	/**
+	 * Enables the output of the RNG.
+	 * Entropy enable for random number generator.
+	 */
+	reg |= 3;
+	write64(&bar[BDK_RNM_CTL_STATUS], reg);
+
+	/* Read back after enable so we know it is done. */
+	reg = read64(&bar[BDK_RNM_CTL_STATUS]);
+	/* Errata (RNM-22528) First consecutive reads to RNM_RANDOM return same
+	 * value. Before using the random entropy, read RNM_RANDOM at least once
+	 * and discard the data */
+	reg = read64(&bar[BDK_RNM_RANDOM]);
+	printk(BIOS_DEBUG, "RNG: RANDOM %llx\n", reg);
+	reg = read64(&bar[BDK_RNM_RANDOM]);
+	printk(BIOS_DEBUG, "RNG: RANDOM %llx\n", reg);
+}
+
 /* based on bdk_boot_dram() */
 void sdram_init(void)
 {
    printk(BIOS_DEBUG, "Initializing DRAM\n");

+	rnm_init();
+
    /**
     * FIXME: second arg is actually a desired frequency if set (the
     * function usually obtains frequency via the config). That might

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31567 )

Change subject: soc/cavium/cn81xx: Enable RNG for DRAM init
......................................................................

Patch Set 1:

(2 comments)

So DRAM init didn’t work before?

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c 
File src/soc/cavium/cn81xx/sdram.c:

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@67 
PS1, Line 67: /**
Just one asterisk?

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@76 
PS1, Line 76: 	/* Errata (RNM-22528) First consecutive reads to RNM_RANDOM return same
Please use the allowed comment style for concise comments.

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

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

Change subject: soc/cavium/cn81xx: Enable RNG for DRAM init
......................................................................

Patch Set 1:

(5 comments)

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c 
File src/soc/cavium/cn81xx/sdram.c:

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@47 
PS1, Line 47: 	#define BDK_RNM_RANDOM		0x100000
I'd remove indentation and move them outside function body. Their scope is the file (even if not used elsewhere). Seem to be register definitions so they often go in separate files entirely.

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@50 
PS1, Line 50: #ifdef __SIMPLE_DEVICE__
You should not need this, unless you really need to build this file for ramstage.

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@51 
PS1, Line 51: 	pci_devfn_t dev = pci_locate_device_on_bus(0xa018177d, 2);
See comments above pci_locate_device() implementation. We tend to use PCI_DEV() here usually. So this hardware has PCI bus 2 somehow hardwired and routed without programming any PCI bridges?

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@72 
PS1, Line 72: 	write64(&bar[BDK_RNM_CTL_STATUS], reg);
I prefer syntax with 'bar + reg_offset' but your choice.

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@82 
PS1, Line 82: 	printk(BIOS_DEBUG, "RNG: RANDOM %llx\n", reg);
BIOS_SPEW?

[Patrick Rudolph (Code Review)]

Hello Kyösti Mälkki, David Hendricks, David Hendricks, Philipp Deppenwiese, build bot (Jenkins),

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

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

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

Change subject: soc/cavium/cn81xx: Enable RNG for DRAM init
......................................................................

soc/cavium/cn81xx: Enable RNG for DRAM init

The Cavium DRAM init might use the RNG for pattern generation.
Initialize it before running DRAM init.

Tested on OpenCellular Elgon.
The RNG generates non identical numbers.

Change-Id: I886f920e9941793fb76b56cc5a24a42e23b082e0
Signed-off-by: Patrick Rudolph 
---
M src/soc/cavium/cn81xx/sdram.c
1 file changed, 46 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/31567/2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31567 )

Change subject: soc/cavium/cn81xx: Enable RNG for DRAM init
......................................................................

Patch Set 2:

(6 comments)
> Patch Set 1:
>
> (2 comments)
>
> So DRAM init didn’t work before?

It worked fine. There are some extended DRAM tests that rely on the RNG. Those are disabled by default.

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c 
File src/soc/cavium/cn81xx/sdram.c:

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@47 
PS1, Line 47: 	#define BDK_RNM_RANDOM		0x100000
> I'd remove indentation and move them outside function body. […]

Done

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@50 
PS1, Line 50: #ifdef __SIMPLE_DEVICE__
> You should not need this, unless you really need to build this file for ramstage.

It's compiled in ramstage, but not used.

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@51 
PS1, Line 51: 	pci_devfn_t dev = pci_locate_device_on_bus(0xa018177d, 2);
> See comments above pci_locate_device() implementation. We tend to use PCI_DEV() here usually. […]

Done

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@67 
PS1, Line 67: /**
> Just one asterisk?

Done

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@76 
PS1, Line 76: 	/* Errata (RNM-22528) First consecutive reads to RNM_RANDOM return same
> Please use the allowed comment style for concise comments.

Done

https://review.coreboot.org/#/c/31567/1/src/soc/cavium/cn81xx/sdram.c@82 
PS1, Line 82: 	printk(BIOS_DEBUG, "RNG: RANDOM %llx\n", reg);
> BIOS_SPEW?

Done

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

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

Change subject: soc/cavium/cn81xx: Enable RNG for DRAM init
......................................................................

Patch Set 2: Code-Review+2

(2 comments)

https://review.coreboot.org/#/c/31567/2/src/soc/cavium/cn81xx/sdram.c 
File src/soc/cavium/cn81xx/sdram.c:

https://review.coreboot.org/#/c/31567/2/src/soc/cavium/cn81xx/sdram.c@44 
PS2, Line 44: #if ENV_ROMSTAGE
Acceptable, I just like it more when "raminit" is only built for romstage.

https://review.coreboot.org/#/c/31567/2/src/soc/cavium/cn81xx/sdram.c@67 
PS2, Line 67: 	/* Read back after enable so we know it is done. */
You don't need to poll any bits here know it's ready to accept the reads below? As printk() had some delay, run some test to make sure the 3rd read value is still properly random after loglevel change.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31567 )

Change subject: soc/cavium/cn81xx: Enable RNG for DRAM init
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31567/2/src/soc/cavium/cn81xx/sdram.c 
File src/soc/cavium/cn81xx/sdram.c:

https://review.coreboot.org/#/c/31567/2/src/soc/cavium/cn81xx/sdram.c@67 
PS2, Line 67: 	/* Read back after enable so we know it is done. */
> You don't need to poll any bits here know it's ready to accept the reads below? As printk() had some […]

That's what the reference code does without further comments.
Cavium hardware likes to raise exceptions if do not read certain registers after writing to them.

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

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

Change subject: soc/cavium/cn81xx: Enable RNG for DRAM init
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31567/2/src/soc/cavium/cn81xx/sdram.c 
File src/soc/cavium/cn81xx/sdram.c:

https://review.coreboot.org/#/c/31567/2/src/soc/cavium/cn81xx/sdram.c@67 
PS2, Line 67: 	/* Read back after enable so we know it is done. */
> That's what the reference code does without further comments. […]

Sounds fun :(

Is it a small set of registesr or some known banks? I don't know the platform, are there any attributes to set "strong uncacheable memory" like x86 where memory writes cannot be combined or posted.

Do you know for sure PCI MMCONF space does not require reads after writes to sync things?

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31567 )

Change subject: soc/cavium/cn81xx: Enable RNG for DRAM init
......................................................................

soc/cavium/cn81xx: Enable RNG for DRAM init

The Cavium DRAM init might use the RNG for pattern generation.
Initialize it before running DRAM init.

Tested on OpenCellular Elgon.
The RNG generates non identical numbers.

Change-Id: I886f920e9941793fb76b56cc5a24a42e23b082e0
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/31567
Tested-by: build bot (Jenkins) 
Reviewed-by: Kyösti Mälkki 
---
M src/soc/cavium/cn81xx/sdram.c
1 file changed, 46 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Kyösti Mälkki: Looks good to me, approved

diff --git a/src/soc/cavium/cn81xx/sdram.c b/src/soc/cavium/cn81xx/sdram.c
index 2342b04..05546cc 100644
--- a/src/soc/cavium/cn81xx/sdram.c
+++ b/src/soc/cavium/cn81xx/sdram.c
@@ -3,6 +3,7 @@
  *
  * Copyright 2018       Facebook, Inc.
  * Copyright 2003-2017  Cavium Inc.  
+ * Copyright 2019       9elements Agency GmbH
  *
  * 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
@@ -28,17 +29,61 @@
 #include <libbdk-hal/bdk-utils.h>
 #include <libbdk-hal/bdk-l2c.h>
 #include <libdram/libdram-config.h>
+#include <soc/ecam.h>
+#include <device/pci_ops.h>
+#include <device/pci.h>

size_t sdram_size_mb(void)
 {
    return bdk_dram_get_size_mbytes(0);
 }

+#define BDK_RNM_CTL_STATUS	0
+#define BDK_RNM_RANDOM		0x100000
+
+#if ENV_ROMSTAGE
+/* Enable RNG for DRAM init */
+static void rnm_init(void)
+{
+	/* Bus numbers are hardcoded in ASIC. No need to program bridges. */
+	pci_devfn_t dev = PCI_DEV(2, 0, 0);
+
+	u64 *bar = (u64 *)ecam0_get_bar_val(dev, 0);
+	if (!bar) {
+		printk(BIOS_ERR, "RNG: Failed to get BAR0\n");
+		return;
+	}
+
+	printk(BIOS_DEBUG, "RNG: BAR0 at %p\n", bar);
+
+	u64 reg = read64(&bar[BDK_RNM_CTL_STATUS]);
+	/*
+	 * Enables the output of the RNG.
+	 * Entropy enable for random number generator.
+	 */
+	reg |= 3;
+	write64(&bar[BDK_RNM_CTL_STATUS], reg);
+
+	/* Read back after enable so we know it is done. */
+	reg = read64(&bar[BDK_RNM_CTL_STATUS]);
+	/*
+	 * Errata (RNM-22528) First consecutive reads to RNM_RANDOM return same
+	 * value. Before using the random entropy, read RNM_RANDOM at least once
+	 * and discard the data
+	 */
+	reg = read64(&bar[BDK_RNM_RANDOM]);
+	printk(BIOS_SPEW, "RNG: RANDOM %llx\n", reg);
+	reg = read64(&bar[BDK_RNM_RANDOM]);
+	printk(BIOS_SPEW, "RNG: RANDOM %llx\n", reg);
+}
+
 /* based on bdk_boot_dram() */
 void sdram_init(void)
 {
    printk(BIOS_DEBUG, "Initializing DRAM\n");

+	rnm_init();
+
    /**
     * FIXME: second arg is actually a desired frequency if set (the
     * function usually obtains frequency via the config). That might
@@ -100,3 +145,4 @@

printk(BIOS_INFO, "SDRAM initialization finished.\n");
 }
+#endif