Change in ...coreboot[master]: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h>
HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31979
Change subject: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h> ......................................................................
soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h>
This fix the error introduced in Change-Id: Ie7afe77 : tegra_lp0_resume.c:431:40: error: static declaration of 'halt' follows non-static declaration static __always_inline void __noreturn halt(void) ^~~~ In file included from tegra_lp0_resume.c:17: ../../../../include/halt.h:26:17: note: previous declaration of 'halt' was here void __noreturn halt(void); ^~~~
Change-Id: I3a67abb77846172597b8ebde779878b9aa2ff8d7 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c M src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c 2 files changed, 0 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/31979/1
diff --git a/src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c b/src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c index 4f219e4..f268ba1 100644 --- a/src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c +++ b/src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c @@ -264,11 +264,6 @@
/* Utility functions. */
-static __always_inline void __noreturn halt(void) -{ - for (;;); -} - static inline uint32_t read32(const void *addr) { return *(volatile uint32_t *)addr; diff --git a/src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c b/src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c index ddd1f4f..acfcd48 100644 --- a/src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c +++ b/src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c @@ -428,11 +428,6 @@
/* Utility functions. */
-static __always_inline void __noreturn halt(void) -{ - for (;;); -} - static inline uint32_t read32(const void *addr) { return *(volatile uint32_t *)addr;
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h> ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
Seems like jenkins flagged these with +1 even with the error present.
https://review.coreboot.org/#/c/31979/1/src/soc/nvidia/tegra124/lp0/tegra_lp... File src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c:
https://review.coreboot.org/#/c/31979/1/src/soc/nvidia/tegra124/lp0/tegra_lp... PS1, Line 267: static inline uint32_t read32(const void *addr) Follow-up, these should come from device/mmio.h
https://review.coreboot.org/#/c/31979/1/src/soc/nvidia/tegra210/lp0/tegra_lp... File src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c:
https://review.coreboot.org/#/c/31979/1/src/soc/nvidia/tegra210/lp0/tegra_lp... PS1, Line 431: static inline uint32_t read32(const void *addr) Same
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h> ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31979/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31979/1//COMMIT_MSG@10 PS1, Line 10: tegra_lp0_resume.c:431:40: error: static declaration of 'halt' follows non-static declaration Split line please.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h> ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/#/c/31979/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31979/1//COMMIT_MSG@7 PS1, Line 7: Use 'halt()' already defined in <halt.h> rather, `Drop local definition of halt()`
https://review.coreboot.org/#/c/31979/1//COMMIT_MSG@9 PS1, Line 9: This fix the error introduced in Change-Id: Ie7afe77 : : tegra_lp0_resume.c:431:40: error: static declaration of 'halt' follows non-static declaration : static __always_inline void __noreturn halt(void) : ^~~~ : In file included from tegra_lp0_resume.c:17: : ../../../../include/halt.h:26:17: note: previous declaration of 'halt' was here : void __noreturn halt(void); : ^~~~ : Please wrap this up, a single sentence should suffice, e.g.
Commit 74aa99a (src: Drop unused '#include <halt.h>') accidentally added redundant definitons of halt(), choose the generic one over the local.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h> ......................................................................
Patch Set 1: Code-Review-2
Well quess if I trusted +1 from jenkins here, error is still present.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h> ......................................................................
Patch Set 1: Code-Review-1
No, wait, this doesn't work like this. The lp0/ directory is special and not linked into the rest of coreboot. You can't pull in code from halt.c. The proper fix is to remove the <halt.h> #include again.
This was originally supposed to be standalone and we just put it into coreboot because the Linux kernel didn't want to take it and we didn't find a better repo for it. We made an exception for <stdint.h> because that's a standard POSIX header, so you can somewhat expect what it contains and that it won't pull an unpredictable chain of other header in behind it. But we shouldn't pull in anything else. That's why I also think this should probably not use <device/mmio.h>, although that should theoretically work because those are all inlines.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h> ......................................................................
Patch Set 1:
It looks like Jenkins doesn't flag this because the coreboot build system doesn't build this at all. I guess we could change that if we want to (just having a target to call '$(MAKE) -C .../lp0/ and adding that as a prerequisite somewhere should be enough), but that would also be a little weird because this is technically not a prerequisite for the firmware image. For reference, the way Chromium OS builds this is with a separate ebuild (independent of coreboot) here https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/r... , and then the resulting binary must be installed to the disk under /lib/firmware to be picked up by Linux. (So really, if you ask me this is a Linux thing, not a coreboot thing, but when we tried to upstream it the Linux maintainers thought differently. It is used during S3 resume.)
Alternatively, we can also remove it from the tree, but then that means this code isn't publicly hosted anywhere at more which would also be unfortunate.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h> ......................................................................
Patch Set 1: -Code-Review
We should get this lp0 stuff build tested...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Use 'halt()' already defined in <halt.h> ......................................................................
Patch Set 1:
`make what-jenkins-does` already builds it. Just Jenkins ignores the error...
Hello Kyösti Mälkki, Julius Werner, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31979
to look at the new patch set (#2).
Change subject: soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>' ......................................................................
soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>'
Commit 74aa99a (src: Drop unused '#include <halt.h>') accidentally added '#include <halt.h>', however tegra_lp0 directory is not linked into the rest of coreboot. So we can't use generic halt() from halt.c file.
Change-Id: I3a67abb77846172597b8ebde779878b9aa2ff8d7 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c M src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c 2 files changed, 0 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/31979/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>' ......................................................................
Patch Set 2: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>' ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>' ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>' ......................................................................
soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>'
Commit 74aa99a (src: Drop unused '#include <halt.h>') accidentally added '#include <halt.h>', however tegra_lp0 directory is not linked into the rest of coreboot. So we can't use generic halt() from halt.c file.
Change-Id: I3a67abb77846172597b8ebde779878b9aa2ff8d7 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr Reviewed-on: https://review.coreboot.org/c/coreboot/+/31979 Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c M src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c 2 files changed, 0 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, but someone else must approve Nico Huber: Looks good to me, approved Julius Werner: Looks good to me, approved
diff --git a/src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c b/src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c index 4f219e4..7cc3204 100644 --- a/src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c +++ b/src/soc/nvidia/tegra124/lp0/tegra_lp0_resume.c @@ -14,7 +14,6 @@ * GNU General Public License for more details. */
-#include <halt.h> #include <stdint.h>
/* Function unit addresses. */ diff --git a/src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c b/src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c index ddd1f4f..51e75f5 100644 --- a/src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c +++ b/src/soc/nvidia/tegra210/lp0/tegra_lp0_resume.c @@ -14,7 +14,6 @@ * GNU General Public License for more details. */
-#include <halt.h> #include <stdint.h>
/* Function unit addresses. */
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>' ......................................................................
Patch Set 3:
This was originally supposed to be standalone and we just put it into coreboot because the Linux kernel didn't want to take it and we didn't find a better repo for it. We made an exception for <stdint.h> because that's a standard POSIX header, so you can somewhat expect what it contains and that it won't pull an unpredictable chain of other header in behind it. But we shouldn't pull in anything else. That's why I also think this should probably not use <device/mmio.h>, although that should theoretically work because those are all inlines.
Hmm, we know the architecture... so we could `-include .../stdint.h` directly, I guess? Instead of adding the coreboot include path.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>' ......................................................................
Patch Set 3:
Hmm, we know the architecture... so we could `-include .../stdint.h` directly, I guess? Instead of adding the coreboot include path.
Yeah, that might be a good idea. The arch include path is already hardcoded in the Makefile anyway, may as well hardcode the file. (Alternatively we could also just directly define the 3 types it uses.)
Patrick Georgi has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/31979 )
Change subject: soc/nvidia/tegra{124,210}: Remove unneeded 'include <halt.h>' ......................................................................
-
HAOUAS Elyes (Code Review) -
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)