Change in coreboot[master]: soc/amd/cezanne: add config.c and minimal chip.h

[Felix Held (Code Review)]

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48317 )

Change subject: soc/amd/cezanne: add config.c and minimal chip.h
......................................................................

soc/amd/cezanne: add config.c and minimal chip.h

Change-Id: I89f08c201bd7d9a11b186ef960abe9714a76fb97
Signed-off-by: Felix Held 
---
M src/soc/amd/cezanne/Makefile.inc
M src/soc/amd/cezanne/chip.c
A src/soc/amd/cezanne/chip.h
A src/soc/amd/cezanne/config.c
4 files changed, 26 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/48317/1

diff --git a/src/soc/amd/cezanne/Makefile.inc b/src/soc/amd/cezanne/Makefile.inc
index d4f585f..df3355e 100644
--- a/src/soc/amd/cezanne/Makefile.inc
+++ b/src/soc/amd/cezanne/Makefile.inc
@@ -2,6 +2,8 @@

ifeq ($(CONFIG_SOC_AMD_CEZANNE),y)

+all-y += config.c
+
 bootblock-y	+= bootblock.c

romstage-y	+= romstage.c
diff --git a/src/soc/amd/cezanne/chip.c b/src/soc/amd/cezanne/chip.c
index 357d745..39c7083 100644
--- a/src/soc/amd/cezanne/chip.c
+++ b/src/soc/amd/cezanne/chip.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

#include <device/device.h>
+#include "chip.h"

struct chip_operations soc_amd_cezanne_ops = { NULL };
diff --git a/src/soc/amd/cezanne/chip.h b/src/soc/amd/cezanne/chip.h
new file mode 100644
index 0000000..b4e94a4
--- /dev/null
+++ b/src/soc/amd/cezanne/chip.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef CEZANNE_CHIP_H
+#define CEZANNE_CHIP_H
+
+#include <amdblocks/chip.h>
+
+struct soc_amd_cezanne_config {
+	struct soc_amd_common_config common_config;
+};
+
+#endif /* CEZANNE_CHIP_H */
diff --git a/src/soc/amd/cezanne/config.c b/src/soc/amd/cezanne/config.c
new file mode 100644
index 0000000..15600ce
--- /dev/null
+++ b/src/soc/amd/cezanne/config.c
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <amdblocks/chip.h>
+#include <device/device.h>
+#include "chip.h"
+
+const struct soc_amd_common_config *soc_get_common_config()
+{
+	const struct soc_amd_cezanne_config *cfg = config_of_soc();
+	return &cfg->common_config;
+}

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/cezanne: add config.c and minimal chip.h
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48317/1/src/soc/amd/cezanne/config.... 
File src/soc/amd/cezanne/config.c:

https://review.coreboot.org/c/coreboot/+/48317/1/src/soc/amd/cezanne/config.... 
PS1, Line 9: cfg 
It looks like the soc_get_config() return value is typically checked for non-0.  But we'll always return non-0 due to line 10.  Should probably be

const struct soc_amd_cezanne_config *cfg = config_of_soc();
    if (!cfg)
      return NULL;
    return &cfg->common_config;

Maybe fix it in picasso first, then add a consistent function to cezanne.

[Felix Held (Code Review)]

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

Change subject: soc/amd/cezanne: add config.c and minimal chip.h
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48317/1/src/soc/amd/cezanne/config.... 
File src/soc/amd/cezanne/config.c:

https://review.coreboot.org/c/coreboot/+/48317/1/src/soc/amd/cezanne/config.... 
PS1, Line 9: cfg
> It looks like the soc_get_config() return value is typically checked for non-0. […]

cfg is always non-NULL, so no need to re-check that here: config_of_soc calls config_of which calls die() if either dev or dev->chip_info are NULL. I can add a comment about that to make that a bit clearer.
What I do wonder though is is I should add a check if cfg->common_config is non-NULL and call die("common_config missing in SoC config!") if it is NULL

[Felix Held (Code Review)]

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

Change subject: soc/amd/cezanne: add config.c and minimal chip.h
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48317/1/src/soc/amd/cezanne/config.... 
File src/soc/amd/cezanne/config.c:

https://review.coreboot.org/c/coreboot/+/48317/1/src/soc/amd/cezanne/config.... 
PS1, Line 9: cfg
> cfg is always non-NULL, so no need to re-check that here: config_of_soc calls config_of which calls  […]

ah, common_config isn't a pointer, so no need to NULL-check it. I'll add a comment about the soc_get_config call though

[Felix Held (Code Review)]

Hello build bot (Jenkins), Jason Glenesk, Patrick Georgi, Martin Roth, Marshall Dawson,

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

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

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

Change subject: soc/amd/cezanne: add config.c and minimal chip.h
......................................................................

soc/amd/cezanne: add config.c and minimal chip.h

Change-Id: I89f08c201bd7d9a11b186ef960abe9714a76fb97
Signed-off-by: Felix Held 
---
M src/soc/amd/cezanne/Makefile.inc
M src/soc/amd/cezanne/chip.c
A src/soc/amd/cezanne/chip.h
A src/soc/amd/cezanne/config.c
4 files changed, 27 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/48317/2

[Felix Held (Code Review)]

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

Change subject: soc/amd/cezanne: add config.c and minimal chip.h
......................................................................

Patch Set 2:

(1 comment)

i'll add the comment in picasso in a separate patch

https://review.coreboot.org/c/coreboot/+/48317/1/src/soc/amd/cezanne/config.... 
File src/soc/amd/cezanne/config.c:

https://review.coreboot.org/c/coreboot/+/48317/1/src/soc/amd/cezanne/config.... 
PS1, Line 9: cfg
> ah, common_config isn't a pointer, so no need to NULL-check it. […]

Done

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/cezanne: add config.c and minimal chip.h
......................................................................

Patch Set 2: Code-Review+2

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48317 )

Change subject: soc/amd/cezanne: add config.c and minimal chip.h
......................................................................

soc/amd/cezanne: add config.c and minimal chip.h

Change-Id: I89f08c201bd7d9a11b186ef960abe9714a76fb97
Signed-off-by: Felix Held 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48317
Tested-by: build bot (Jenkins) 
Reviewed-by: Marshall Dawson 
---
M src/soc/amd/cezanne/Makefile.inc
M src/soc/amd/cezanne/chip.c
A src/soc/amd/cezanne/chip.h
A src/soc/amd/cezanne/config.c
4 files changed, 27 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/amd/cezanne/Makefile.inc b/src/soc/amd/cezanne/Makefile.inc
index d4f585f..df3355e 100644
--- a/src/soc/amd/cezanne/Makefile.inc
+++ b/src/soc/amd/cezanne/Makefile.inc
@@ -2,6 +2,8 @@

ifeq ($(CONFIG_SOC_AMD_CEZANNE),y)

+all-y += config.c
+
 bootblock-y	+= bootblock.c

romstage-y	+= romstage.c
diff --git a/src/soc/amd/cezanne/chip.c b/src/soc/amd/cezanne/chip.c
index 357d745..39c7083 100644
--- a/src/soc/amd/cezanne/chip.c
+++ b/src/soc/amd/cezanne/chip.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

#include <device/device.h>
+#include "chip.h"

struct chip_operations soc_amd_cezanne_ops = { NULL };
diff --git a/src/soc/amd/cezanne/chip.h b/src/soc/amd/cezanne/chip.h
new file mode 100644
index 0000000..b4e94a4
--- /dev/null
+++ b/src/soc/amd/cezanne/chip.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef CEZANNE_CHIP_H
+#define CEZANNE_CHIP_H
+
+#include <amdblocks/chip.h>
+
+struct soc_amd_cezanne_config {
+	struct soc_amd_common_config common_config;
+};
+
+#endif /* CEZANNE_CHIP_H */
diff --git a/src/soc/amd/cezanne/config.c b/src/soc/amd/cezanne/config.c
new file mode 100644
index 0000000..b5855eb
--- /dev/null
+++ b/src/soc/amd/cezanne/config.c
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <amdblocks/chip.h>
+#include <device/device.h>
+#include "chip.h"
+
+const struct soc_amd_common_config *soc_get_common_config()
+{
+	/* config_of_soc calls die() internally if cfg was NULL, so no need to re-check */
+	const struct soc_amd_cezanne_config *cfg = config_of_soc();
+	return &cfg->common_config;
+}