Change in coreboot[master]: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration

[Felix Singer (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration

`mainboard_silicon_init_params()` should *only* be used for configuring
FSP options which can not be configured anywhere else. Therefore, use
the init phase from the mainboard_ops driver for configuring the GPIOs.

Signed-off-by: Felix Singer 
Change-Id: Ia01091938ac113cb5cf95f046609a1ebf3620806
---
M src/mainboard/kontron/mal10/ramstage.c
1 file changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/48143/1

diff --git a/src/mainboard/kontron/mal10/ramstage.c b/src/mainboard/kontron/mal10/ramstage.c
index 48194e6..87ead15 100644
--- a/src/mainboard/kontron/mal10/ramstage.c
+++ b/src/mainboard/kontron/mal10/ramstage.c
@@ -1,13 +1,17 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

+#include <device/device.h>
 #include <soc/ramstage.h>
 #include <carrier/gpio.h>
 #include <stddef.h>

-void mainboard_silicon_init_params(FSP_S_CONFIG *silconfig)
+static void init_mainboard(void *chip_info)
 {
    carrier_gpio_configure();
+}

+void mainboard_silicon_init_params(FSP_S_CONFIG *silconfig)
+{
    /*
     * CPU Power Management Configuration correspond to the BIOS Setup menu settings
     * in the AMI UEFI v112.
@@ -45,3 +49,7 @@
    silconfig->IoApicDeviceNumber = 0x1F;
    silconfig->IoApicFunctionNumber = 0;
 }
+
+struct chip_operations mainboard_ops = {
+	.init = init_mainboard,
+};

[Felix Singer (Code Review)]

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov,

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

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

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration

`mainboard_silicon_init_params()` should *only* be used for configuring
FSP options which can not be configured anywhere else. Therefore, use
the init phase from the mainboard_ops driver for configuring the GPIOs.

Signed-off-by: Felix Singer 
Change-Id: Ia01091938ac113cb5cf95f046609a1ebf3620806
---
M src/mainboard/kontron/mal10/ramstage.c
1 file changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/48143/3

[Angel Pons (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG@11 
PS6, Line 11: the init phase from the mainboard_ops driver for configuring the GPIOs.
Have you tested whether the final GPIO settings remain unaffected with this patch? If GPIOs are now configured after FSP-S has run, there could be differences.

[Felix Singer (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 6: Code-Review+2

[Felix Singer (Code Review)]

Felix Singer has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/48143 )

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Removed Code-Review+2 by  Felix Singer

[Felix Singer (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 6:

oops, that was not intended.

[Felix Singer (Code Review)]

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Angel Pons,

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

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

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration

`mainboard_silicon_init_params()` should *only* be used for configuring
FSP options which can not be configured anywhere else. Therefore, use
the init phase from the mainboard_ops driver for configuring the GPIOs.

Signed-off-by: Felix Singer 
Change-Id: Ia01091938ac113cb5cf95f046609a1ebf3620806
---
M src/mainboard/kontron/mal10/ramstage.c
1 file changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/48143/7

[Felix Singer (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG@11 
PS6, Line 11: the init phase from the mainboard_ops driver for configuring the GPIOs.
> Have you tested whether the final GPIO settings remain unaffected with this patch? If GPIOs are now  […]

I don't have that carrier board, which is currently upstream. I need to rebase mine first, before I can test this. I can do that these week.

I don't really know how I can test this. I would just diff inteltool dumps before/after?

[Angel Pons (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG@11 
PS6, Line 11: the init phase from the mainboard_ops driver for configuring the GPIOs.
> I don't have that carrier board, which is currently upstream. […]

Should be good enough, though there might be differences in stuff like input state.

[Michael Niewöhner (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 8: Code-Review+2

[Michael Niewöhner (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG@11 
PS6, Line 11: the init phase from the mainboard_ops driver for configuring the GPIOs.
> Should be good enough, though there might be differences in stuff like input state.

Well, if you have access to schematics, I'd rather check if the gpios are correct. If not, diffing an inteltool dump should be ok. I don't expect much differences, since it looks like the gpios were converted from an inteltool dump already, which usually is done from OS and thus after FSP/refcode.

[Angel Pons (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48143/6//COMMIT_MSG@11 
PS6, Line 11: the init phase from the mainboard_ops driver for configuring the GPIOs.
> Well, if you have access to schematics, I'd rather check if the gpios are correct. […]

Well, initial inteltool GPIO dump is taken from vendor firmware, so coreboot code ordering didn't matter at that point.

[Patrick Georgi (Code Review)]

Attention is currently required from:  Felix Singer, Angel Pons, Michael Niewöhner.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48143 )

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 9:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48143/comment/53261f59_383843d3 
PS6, Line 11: the init phase from the mainboard_ops driver for configuring the GPIOs.
> Well, initial inteltool GPIO dump is taken from vendor firmware, so coreboot code ordering didn't ma […]

What needs to happen to resolve this discussion?

[Maxim Polyakov (Code Review)]

Attention is currently required from:  Felix Singer, Angel Pons, Michael Niewöhner.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48143 )

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 10: Code-Review+2

[Maxim Polyakov (Code Review)]

Attention is currently required from:  Felix Singer, Angel Pons, Michael Niewöhner.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48143 )

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 10:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48143/comment/ff5e9b63_a60a33ff 
PS6, Line 11: the init phase from the mainboard_ops driver for configuring the GPIOs.
> What needs to happen to resolve this discussion?

Ack

[Angel Pons (Code Review)]

Attention is currently required from:  Felix Singer, Maxim Polyakov, Michael Niewöhner.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48143 )

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

Patch Set 10: Code-Review+1

[Felix Held (Code Review)]

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

Change subject: mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration
......................................................................

mb/kontron/mal10: Use mainboard_ops driver for GPIO configuration

`mainboard_silicon_init_params()` should *only* be used for configuring
FSP options which can not be configured anywhere else. Therefore, use
the init phase from the mainboard_ops driver for configuring the GPIOs.

Signed-off-by: Felix Singer 
Change-Id: Ia01091938ac113cb5cf95f046609a1ebf3620806
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48143
Reviewed-by: Maxim Polyakov 
Reviewed-by: Angel Pons 
Reviewed-by: Michael Niewöhner 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/kontron/mal10/ramstage.c
1 file changed, 9 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, but someone else must approve
  Maxim Polyakov: Looks good to me, approved
  Michael Niewöhner: Looks good to me, approved

diff --git a/src/mainboard/kontron/mal10/ramstage.c b/src/mainboard/kontron/mal10/ramstage.c
index 3d259d8..f03006a 100644
--- a/src/mainboard/kontron/mal10/ramstage.c
+++ b/src/mainboard/kontron/mal10/ramstage.c
@@ -1,12 +1,16 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

+#include <device/device.h>
 #include <soc/ramstage.h>
 #include <carrier/gpio.h>

-void mainboard_silicon_init_params(FSP_S_CONFIG *silconfig)
+static void init_mainboard(void *chip_info)
 {
    carrier_gpio_configure();
+}

+void mainboard_silicon_init_params(FSP_S_CONFIG *silconfig)
+{
    /*
     * CPU Power Management Configuration correspond to the BIOS Setup menu settings
     * in the AMI UEFI v112.
@@ -44,3 +48,7 @@
    silconfig->IoApicDeviceNumber = 0x1F;
    silconfig->IoApicFunctionNumber = 0;
 }
+
+struct chip_operations mainboard_ops = {
+	.init = init_mainboard,
+};