Change in coreboot[master]: mb/purism/librem_skl: Clean up FSP-M RCOMP settings

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46252 )

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

mb/purism/librem_skl: Clean up FSP-M RCOMP settings

There's no need to use static functions to fill these settings in. Also,
add missing include for <stdint.h> and constify the `mem_cfg` pointer.

Change-Id: I82b0997846d4ec40cf9b1a8ebfb1e881b194e078
Signed-off-by: Angel Pons 
---
M src/mainboard/purism/librem_skl/romstage.c
1 file changed, 11 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/46252/1

diff --git a/src/mainboard/purism/librem_skl/romstage.c b/src/mainboard/purism/librem_skl/romstage.c
index 4982836..698cb37 100644
--- a/src/mainboard/purism/librem_skl/romstage.c
+++ b/src/mainboard/purism/librem_skl/romstage.c
@@ -3,37 +3,30 @@
 #include <assert.h>
 #include <soc/romstage.h>
 #include <spd_bin.h>
+#include <stdint.h>
 #include <string.h>

-static void mainboard_fill_rcomp_res_data(void *rcomp_ptr)
-{
-	/* Rcomp resistor */
-	const u16 RcompResistor[3] = { 121, 81, 100 };
-	memcpy(rcomp_ptr, RcompResistor, sizeof(RcompResistor));
-}
-
-static void mainboard_fill_rcomp_strength_data(void *rcomp_strength_ptr)
-{
-	/* Rcomp target */
-	const u16 RcompTarget[5] = { 100, 40, 20, 20, 26 };
-	memcpy(rcomp_strength_ptr, RcompTarget, sizeof(RcompTarget));
-}
-
 void mainboard_memory_init_params(FSPM_UPD *mupd)
 {
-	FSP_M_CONFIG *mem_cfg;
+	const u16 rcomp_resistors[3] = { 121, 81, 100 };
+
+	const u16 rcomp_targets[5] = { 100, 40, 20, 20, 26 };
+
+	FSP_M_CONFIG *const mem_cfg = &mupd->FspmConfig;
+
    struct spd_block blk = {
    	.addr_map = { 0x50 },
    };

-	mem_cfg = &mupd->FspmConfig;
+	assert(sizeof(mem_cfg->RcompResistor) == sizeof(rcomp_resistors));
+	assert(sizeof(mem_cfg->RcompTarget)   == sizeof(rcomp_targets));

get_spd_smbus(&blk);
    dump_spd_info(&blk);
    assert(blk.spd_array[0][0] != 0);

-	mainboard_fill_rcomp_res_data(&mem_cfg->RcompResistor);
-	mainboard_fill_rcomp_strength_data(&mem_cfg->RcompTarget);
+	memcpy(mem_cfg->RcompResistor, rcomp_resistors, sizeof(mem_cfg->RcompResistor));
+	memcpy(mem_cfg->RcompTarget,   rcomp_targets,   sizeof(mem_cfg->RcompTarget));

mem_cfg->DqPinsInterleaved = TRUE;
    mem_cfg->MemorySpdDataLen = blk.len;

[build bot (Jenkins) (Code Review)]

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

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
File src/mainboard/purism/librem_skl/romstage.c:

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
PS1, Line 15: 	FSP_M_CONFIG *const mem_cfg = &mupd->FspmConfig;
need consistent spacing around '*' (ctx:WxV)

[Angel Pons (Code Review)]

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

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
File src/mainboard/purism/librem_skl/romstage.c:

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
PS1, Line 15: 	FSP_M_CONFIG *const mem_cfg = &mupd->FspmConfig;
> need consistent spacing around '*' (ctx:WxV)

Jenkins, you're drunk.

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46252 )

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
File src/mainboard/purism/librem_skl/romstage.c:

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
PS1, Line 15: 	FSP_M_CONFIG *const mem_cfg = &mupd->FspmConfig;
> Jenkins, you're drunk.

shouldn't it be `const FSP_M_CONFIG *mem_cfg` ?

[Angel Pons (Code Review)]

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

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
File src/mainboard/purism/librem_skl/romstage.c:

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
PS1, Line 15: 	FSP_M_CONFIG *const mem_cfg = &mupd->FspmConfig;
> shouldn't it be `const FSP_M_CONFIG *mem_cfg` ?

No, that would declare a read-only pointer and would not even build. To understand what `const` means on a pointer, you need to read the declaration backwards:

/*
  * mem_cfg is a 'const pointer' to 'FSP_M_CONFIG'.
  * Its address cannot be reassigned, but it can be
  * used to read and write to the pointed memory.
  */
 FSP_M_CONFIG *const mem_cfg;

/* 
  * mem_cfg is a 'pointer' to 'const FSP_M_CONFIG'.
  * Its address can be changed at will, but it can
  * only be used to read the memory it points to.
  */
 const FSP_M_CONFIG *mem_cfg;

Jenkins failed the lint check because it does not understand `FSP_M_CONFIG`.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46252 )

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

Patch Set 1: Code-Review+2

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Matt DeVillier, Michael Niewöhner,

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

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

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

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

mb/purism/librem_skl: Clean up FSP-M RCOMP settings

There's no need to use static functions to fill these settings in. Also,
add missing include for <stdint.h> and constify the `mem_cfg` pointer.

Change-Id: I82b0997846d4ec40cf9b1a8ebfb1e881b194e078
Signed-off-by: Angel Pons 
---
M src/mainboard/purism/librem_skl/romstage.c
1 file changed, 11 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/46252/3

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Matt DeVillier, Michael Niewöhner,

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

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

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

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

mb/purism/librem_skl: Clean up FSP-M RCOMP settings

There's no need to use static functions to fill these settings in. Also,
add missing include for <stdint.h> and initialize `mem_cfg` in one line.

Change-Id: I82b0997846d4ec40cf9b1a8ebfb1e881b194e078
Signed-off-by: Angel Pons 
---
M src/mainboard/purism/librem_skl/romstage.c
1 file changed, 11 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/46252/4

[Angel Pons (Code Review)]

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

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
File src/mainboard/purism/librem_skl/romstage.c:

https://review.coreboot.org/c/coreboot/+/46252/1/src/mainboard/purism/librem... 
PS1, Line 15: 	FSP_M_CONFIG *const mem_cfg = &mupd->FspmConfig;
> No, that would declare a read-only pointer and would not even build. […]

Ack

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46252 )

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

Patch Set 4:

As an alternative, or as a follow up, SKL could be unified with the other platforms (memcfg struct)

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46252 )

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

Patch Set 4: Code-Review+2

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46252 )

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

Patch Set 4: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46252 )

Change subject: mb/purism/librem_skl: Clean up FSP-M RCOMP settings
......................................................................

mb/purism/librem_skl: Clean up FSP-M RCOMP settings

There's no need to use static functions to fill these settings in. Also,
add missing include for <stdint.h> and initialize `mem_cfg` in one line.

Change-Id: I82b0997846d4ec40cf9b1a8ebfb1e881b194e078
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46252
Reviewed-by: Michael Niewöhner 
Reviewed-by:  Felix Singer 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/purism/librem_skl/romstage.c
1 file changed, 11 insertions(+), 18 deletions(-)

Approvals:
  build bot (Jenkins): Verified
   Felix Singer: Looks good to me, approved
  Michael Niewöhner: Looks good to me, approved

diff --git a/src/mainboard/purism/librem_skl/romstage.c b/src/mainboard/purism/librem_skl/romstage.c
index 4982836..1c90771 100644
--- a/src/mainboard/purism/librem_skl/romstage.c
+++ b/src/mainboard/purism/librem_skl/romstage.c
@@ -3,37 +3,30 @@
 #include <assert.h>
 #include <soc/romstage.h>
 #include <spd_bin.h>
+#include <stdint.h>
 #include <string.h>

-static void mainboard_fill_rcomp_res_data(void *rcomp_ptr)
-{
-	/* Rcomp resistor */
-	const u16 RcompResistor[3] = { 121, 81, 100 };
-	memcpy(rcomp_ptr, RcompResistor, sizeof(RcompResistor));
-}
-
-static void mainboard_fill_rcomp_strength_data(void *rcomp_strength_ptr)
-{
-	/* Rcomp target */
-	const u16 RcompTarget[5] = { 100, 40, 20, 20, 26 };
-	memcpy(rcomp_strength_ptr, RcompTarget, sizeof(RcompTarget));
-}
-
 void mainboard_memory_init_params(FSPM_UPD *mupd)
 {
-	FSP_M_CONFIG *mem_cfg;
+	const u16 rcomp_resistors[3] = { 121, 81, 100 };
+
+	const u16 rcomp_targets[5] = { 100, 40, 20, 20, 26 };
+
+	FSP_M_CONFIG *mem_cfg = &mupd->FspmConfig;
+
    struct spd_block blk = {
    	.addr_map = { 0x50 },
    };

-	mem_cfg = &mupd->FspmConfig;
+	assert(sizeof(mem_cfg->RcompResistor) == sizeof(rcomp_resistors));
+	assert(sizeof(mem_cfg->RcompTarget)   == sizeof(rcomp_targets));

get_spd_smbus(&blk);
    dump_spd_info(&blk);
    assert(blk.spd_array[0][0] != 0);

-	mainboard_fill_rcomp_res_data(&mem_cfg->RcompResistor);
-	mainboard_fill_rcomp_strength_data(&mem_cfg->RcompTarget);
+	memcpy(mem_cfg->RcompResistor, rcomp_resistors, sizeof(mem_cfg->RcompResistor));
+	memcpy(mem_cfg->RcompTarget,   rcomp_targets,   sizeof(mem_cfg->RcompTarget));

mem_cfg->DqPinsInterleaved = TRUE;
    mem_cfg->MemorySpdDataLen = blk.len;