Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47835 )
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
mb/clevo/cml-u: Get rid of cnl_configure_pads()
Change-Id: I75dd15ab6d2b3b72b3ad0398df87b349fd00bc3c Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/mainboard/clevo/cml-u/ramstage.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47835/1
diff --git a/src/mainboard/clevo/cml-u/ramstage.c b/src/mainboard/clevo/cml-u/ramstage.c index 037409c..824adfc 100644 --- a/src/mainboard/clevo/cml-u/ramstage.c +++ b/src/mainboard/clevo/cml-u/ramstage.c @@ -1,11 +1,13 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <soc/ramstage.h> +#include <device/device.h> #include <variant/gpio.h>
-void mainboard_silicon_init_params(FSP_S_CONFIG *params) +static void init_mainboard(void *chip_info) { - /* Configure pads prior to SiliconInit() in case there's any - dependencies during hardware initialization. */ - cnl_configure_pads(gpio_table, ARRAY_SIZE(gpio_table)); + gpio_configure_pads(gpio_table, ARRAY_SIZE(gpio_table)); } + +struct chip_operations mainboard_ops = { + .init = init_mainboard, +};
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47835 )
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/47835/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47835/1//COMMIT_MSG@7 PS1, Line 7: mb/clevo/cml-u: Get rid of cnl_configure_pads() describe that the mainboard device driver model is used now
https://review.coreboot.org/c/coreboot/+/47835/1//COMMIT_MSG@8 PS1, Line 8: was this tested?
https://review.coreboot.org/c/coreboot/+/47835/1/src/mainboard/clevo/cml-u/r... File src/mainboard/clevo/cml-u/ramstage.c:
https://review.coreboot.org/c/coreboot/+/47835/1/src/mainboard/clevo/cml-u/r... PS1, Line 12: .init = init_mainboard, not sure if we should used .dev_enable or .init
Hello build bot (Jenkins), Michael Niewöhner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47835
to look at the new patch set (#2).
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
mb/clevo/cml-u: Get rid of cnl_configure_pads()
Change-Id: I75dd15ab6d2b3b72b3ad0398df87b349fd00bc3c Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/mainboard/clevo/cml-u/ramstage.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47835/2
Hello build bot (Jenkins), Michael Niewöhner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47835
to look at the new patch set (#3).
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
mb/clevo/cml-u: Get rid of cnl_configure_pads()
Get rid of cnl_configure_pads() since is a hack for the FSP. Instead, hook up to the mainboard_ops driver and configure the GPIOs while .init.
Tested on clevo/l140cu and it still boots.
Change-Id: I75dd15ab6d2b3b72b3ad0398df87b349fd00bc3c Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/mainboard/clevo/cml-u/ramstage.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47835/3
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47835 )
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47835/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47835/1//COMMIT_MSG@7 PS1, Line 7: mb/clevo/cml-u: Get rid of cnl_configure_pads()
describe that the mainboard device driver model is used now
Done
https://review.coreboot.org/c/coreboot/+/47835/1//COMMIT_MSG@8 PS1, Line 8:
was this tested?
Done
https://review.coreboot.org/c/coreboot/+/47835/1/src/mainboard/clevo/cml-u/r... File src/mainboard/clevo/cml-u/ramstage.c:
https://review.coreboot.org/c/coreboot/+/47835/1/src/mainboard/clevo/cml-u/r... PS1, Line 12: .init = init_mainboard,
not sure if we should used .dev_enable or . […]
.init seems reasonable and most other mainboards are using this too.
Hello build bot (Jenkins), Michael Niewöhner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47835
to look at the new patch set (#4).
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
mb/clevo/cml-u: Get rid of cnl_configure_pads()
Get rid of cnl_configure_pads() since is a hack for the FSP. Instead, hook up to the mainboard_ops driver and configure the GPIOs while .init.
Tested on clevo/l140cu and it still boots.
Change-Id: I75dd15ab6d2b3b72b3ad0398df87b349fd00bc3c Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/mainboard/clevo/cml-u/ramstage.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47835/4
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47835 )
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
Patch Set 4: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/47835/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47835/3//COMMIT_MSG@9 PS3, Line 9: since is a hack for the FSP "since >it< is"; also, maybe "since it is a hack for FSP only needed in special cases" is better?
https://review.coreboot.org/c/coreboot/+/47835/3//COMMIT_MSG@10 PS3, Line 10: while nit: in
Hello build bot (Jenkins), Michael Niewöhner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47835
to look at the new patch set (#5).
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
mb/clevo/cml-u: Get rid of cnl_configure_pads()
Get rid of cnl_configure_pads() since it is a hack for the FSP. Instead, hook up to the mainboard_ops driver and configure the GPIOs while .init.
Tested on clevo/l140cu and it still boots.
Change-Id: I75dd15ab6d2b3b72b3ad0398df87b349fd00bc3c Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/mainboard/clevo/cml-u/ramstage.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47835/5
Hello build bot (Jenkins), Michael Niewöhner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47835
to look at the new patch set (#6).
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
mb/clevo/cml-u: Get rid of cnl_configure_pads()
Get rid of cnl_configure_pads() since it is a hack for the FSP. Instead, hook up to the mainboard_ops driver and configure the GPIOs using .init.
Tested on clevo/l140cu and it still boots.
Change-Id: I75dd15ab6d2b3b72b3ad0398df87b349fd00bc3c Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/mainboard/clevo/cml-u/ramstage.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47835/6
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47835 )
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47835/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47835/3//COMMIT_MSG@9 PS3, Line 9: since is a hack for the FSP
"since >it< is"; also, maybe "since it is a hack for FSP only needed in special cases" is better?
The only "special case" is the FSP here. It configures GPIOs that it shouldn't.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47835 )
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47835/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47835/3//COMMIT_MSG@9 PS3, Line 9: since is a hack for the FSP
The only "special case" is the FSP here. It configures GPIOs that it shouldn't.
Well, but we use FSP and thus it's confusing ;) People will ask "why don't we need the hack when we use fsp?".
What I meant was that it seems only to be required under special circumstances, which we don't know.
Michael Niewöhner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47835 )
Change subject: mb/clevo/cml-u: Get rid of cnl_configure_pads() ......................................................................
mb/clevo/cml-u: Get rid of cnl_configure_pads()
Get rid of cnl_configure_pads() since it is a hack for the FSP. Instead, hook up to the mainboard_ops driver and configure the GPIOs using .init.
Tested on clevo/l140cu and it still boots.
Change-Id: I75dd15ab6d2b3b72b3ad0398df87b349fd00bc3c Signed-off-by: Felix Singer felixsinger@posteo.net Reviewed-on: https://review.coreboot.org/c/coreboot/+/47835 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/clevo/cml-u/ramstage.c 1 file changed, 7 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Michael Niewöhner: Looks good to me, approved
diff --git a/src/mainboard/clevo/cml-u/ramstage.c b/src/mainboard/clevo/cml-u/ramstage.c index 037409c..824adfc 100644 --- a/src/mainboard/clevo/cml-u/ramstage.c +++ b/src/mainboard/clevo/cml-u/ramstage.c @@ -1,11 +1,13 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <soc/ramstage.h> +#include <device/device.h> #include <variant/gpio.h>
-void mainboard_silicon_init_params(FSP_S_CONFIG *params) +static void init_mainboard(void *chip_info) { - /* Configure pads prior to SiliconInit() in case there's any - dependencies during hardware initialization. */ - cnl_configure_pads(gpio_table, ARRAY_SIZE(gpio_table)); + gpio_configure_pads(gpio_table, ARRAY_SIZE(gpio_table)); } + +struct chip_operations mainboard_ops = { + .init = init_mainboard, +};
-
Felix Singer (Code Review) -
Michael Niewöhner (Code Review)