Change in coreboot[master]: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h>
HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h>
Change-Id: If7d10036ab3047a9f2480e7ed04c6a6c7033ee27 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/drivers/uart/util.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/41678/1
diff --git a/src/drivers/uart/util.c b/src/drivers/uart/util.c index 1ac994e..f18a99d 100644 --- a/src/drivers/uart/util.c +++ b/src/drivers/uart/util.c @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <console/uart.h> -#include <types.h> +#include <commonlib/bsd/helpers.h> #include <timer.h>
/* Calculate divisor. Do not floor but round to nearest integer. */
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41678/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41678/2//COMMIT_MSG@8 PS2, Line 8: Mention that you remove `types.h`?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41678
to look at the new patch set (#3).
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h>
Also remove unused 'include <types.h>'
Change-Id: If7d10036ab3047a9f2480e7ed04c6a6c7033ee27 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/drivers/uart/util.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/41678/3
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41678/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41678/2//COMMIT_MSG@8 PS2, Line 8:
Mention that you remove `types. […]
Thank you
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41678/3/src/drivers/uart/util.c File src/drivers/uart/util.c:
https://review.coreboot.org/c/coreboot/+/41678/3/src/drivers/uart/util.c@a57 PS3, Line 57: MHz needs <commonlib/bsd/helpers.h>
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 3:
Sorry, I feel like we have this discussion over an over again (and usually it sounds like you agree with me, but then you upload another patch like this again a few months later). <types.h> is *intended* to be a catchall header for integer type stuff and associated common helper macros. I do not understand what's so useful about always including every header where every little macro came from directly. <commonlibs/bsd/helpers.h> is hard to write and probably quite meaningless to most people. In fact, for that file in particular I forked it out of <commonlib/helpers.h> solely for license reasons and nothing else (and Aaron originally forked <commonlib/helpers.h> out of <stddef.h> for code sharing reasons and nothing else). Why should the people adding headers to random coreboot files have to care about details like that? Why should we keep churning all files that include things (and these helpers are included in almost all of coreboot!) around when details like that change?
I'm not saying we have to go full libpayload and only have a single header for the whole project -- separating truly separate APIs into their own headers makes sense. But for things like ALIGN_UP() or MiB that are needed literally *everywhere*, is it really that important that we include a separate header each time? Is it really that important that size_t comes from <stddef.h> and uint32_t from <stdint.h>? Do we really need to force everyone committing files anywhere in coreboot to spend headspace on those details?
In my opinion, commonlib is a detail about which code can be shared where and not a core API boundary. For those APIs in commonlib that have a suitable "wrapper" header in normal coreboot code (e.g. cbfs.h, cbmem_id.h, endian.h, helpers.h, loglevel.h, stdlib.h, tcpa_log_serialized.h, timestamp_serialized.h), I think other coreboot code should always just be including that normal coreboot header and rely on it chain-including pieces that have solely for code sharing or licensing reasons been split out into other files.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 3:
Patch Set 3:
Sorry, I feel like we have this discussion over an over again (and usually it sounds like you agree with me, but then you upload another patch like this again a few months later). <types.h> is *intended* to be a catchall header for integer type stuff and associated common helper macros. I do not understand what's so useful about always including every header where every little macro came from directly. <commonlibs/bsd/helpers.h> is hard to write and probably quite meaningless to most people. In fact, for that file in particular I forked it out of <commonlib/helpers.h> solely for license reasons and nothing else (and Aaron originally forked <commonlib/helpers.h> out of <stddef.h> for code sharing reasons and nothing else). Why should the people adding headers to random coreboot files have to care about details like that? Why should we keep churning all files that include things (and these helpers are included in almost all of coreboot!) around when details like that change?
I'm not saying we have to go full libpayload and only have a single header for the whole project -- separating truly separate APIs into their own headers makes sense. But for things like ALIGN_UP() or MiB that are needed literally *everywhere*, is it really that important that we include a separate header each time? Is it really that important that size_t comes from <stddef.h> and uint32_t from <stdint.h>? Do we really need to force everyone committing files anywhere in coreboot to spend headspace on those details?
In my opinion, commonlib is a detail about which code can be shared where and not a core API boundary. For those APIs in commonlib that have a suitable "wrapper" header in normal coreboot code (e.g. cbfs.h, cbmem_id.h, endian.h, helpers.h, loglevel.h, stdlib.h, tcpa_log_serialized.h, timestamp_serialized.h), I think other coreboot code should always just be including that normal coreboot header and rely on it chain-including pieces that have solely for code sharing or licensing reasons been split out into other files.
By my understanding,<types.h> provides <stdbool.h>, <stdint.h>, <stddef.h> and you added <commonlib/bsd/cb_err.h>. This what we have as comment in <types.h>. for current file, we include all of those files for a single "MHz" defined in <commonlib/bsd/helpers.h>. That's why I've changed it to a single include. Maybe I'm wrong, but it looks to me "reasonable" to include only <commonlib/bsd/helpers.h> for a single used macro.
This said, in CB:41677 <types.h> in include but there no a single u8 or size_t ... used. We also have many files where we include <types.h> and <stdint.h> ... , I'll also clean up that duplicated includes.
I'm also fine to drop current patch if we add <commonlib/bsd/helpers.h> to <types.h>.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Sorry, I feel like we have this discussion over an over again (and usually it sounds like you agree with me, but then you upload another patch like this again a few months later). <types.h> is *intended* to be a catchall header for integer type stuff and associated common helper macros. I do not understand what's so useful about always including every header where every little macro came from directly. <commonlibs/bsd/helpers.h> is hard to write and probably quite meaningless to most people. In fact, for that file in particular I forked it out of <commonlib/helpers.h> solely for license reasons and nothing else (and Aaron originally forked <commonlib/helpers.h> out of <stddef.h> for code sharing reasons and nothing else). Why should the people adding headers to random coreboot files have to care about details like that? Why should we keep churning all files that include things (and these helpers are included in almost all of coreboot!) around when details like that change?
I'm not saying we have to go full libpayload and only have a single header for the whole project -- separating truly separate APIs into their own headers makes sense. But for things like ALIGN_UP() or MiB that are needed literally *everywhere*, is it really that important that we include a separate header each time? Is it really that important that size_t comes from <stddef.h> and uint32_t from <stdint.h>? Do we really need to force everyone committing files anywhere in coreboot to spend headspace on those details?
In my opinion, commonlib is a detail about which code can be shared where and not a core API boundary. For those APIs in commonlib that have a suitable "wrapper" header in normal coreboot code (e.g. cbfs.h, cbmem_id.h, endian.h, helpers.h, loglevel.h, stdlib.h, tcpa_log_serialized.h, timestamp_serialized.h), I think other coreboot code should always just be including that normal coreboot header and rely on it chain-including pieces that have solely for code sharing or licensing reasons been split out into other files.
By my understanding,<types.h> provides <stdbool.h>, <stdint.h>, <stddef.h> and you added <commonlib/bsd/cb_err.h>. This what we have as comment in <types.h>. for current file, we include all of those files for a single "MHz" defined in <commonlib/bsd/helpers.h>. That's why I've changed it to a single include. Maybe I'm wrong, but it looks to me "reasonable" to include only <commonlib/bsd/helpers.h> for a single used macro.
This said, in CB:41677 <types.h> in include but there no a single u8 or size_t ... used. We also have many files where we include <types.h> and <stdint.h> ... , I'll also clean up that duplicated includes.
I'm also fine to drop current patch if we add <commonlib/bsd/helpers.h> to <types.h>.
Please see CB:41817
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 3:
By my understanding,<types.h> provides <stdbool.h>, <stdint.h>, <stddef.h> and you added <commonlib/bsd/cb_err.h>. This what we have as comment in <types.h>. for current file, we include all of those files for a single "MHz" defined in <commonlib/bsd/helpers.h>.
Yeah, but stddef.h provides things like MHz. Always has. Like I said, this was only split out so some of the code can be shared with other repositories (see CB:11592).
I'm just saying I don't think every random coreboot file should directly pull in specialty includes from three directories deep that keep changing every year because we found yet another reason to split something into a separate header here or there. Many of these chain includes are there for a reason.
I'm also fine to drop current patch if we add <commonlib/bsd/helpers.h> to <types.h>.
If that makes it look more correct to you, that's fine by me.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 4: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41678/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41678/4//COMMIT_MSG@9 PS4, Line 9: Also remove unused 'include <types.h>' Could have done that in the follow-up
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 4:
(1 comment)
thank you.
https://review.coreboot.org/c/coreboot/+/41678/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41678/4//COMMIT_MSG@9 PS4, Line 9: Also remove unused 'include <types.h>'
Could have done that in the follow-up
you are right. My 1st idea is to make this easy to review.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41678/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41678/4//COMMIT_MSG@9 PS4, Line 9: Also remove unused 'include <types.h>'
you are right. […]
Please do it in the follow-up then.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Patch Set 4:
(1 comment)
Thx
https://review.coreboot.org/c/coreboot/+/41678/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41678/4//COMMIT_MSG@9 PS4, Line 9: Also remove unused 'include <types.h>'
Please do it in the follow-up then.
Ok, I'll drop this one and merge it with the follow-up
HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41678 )
Change subject: drivers/uart/util.c: Add missing 'include <commonlib/bsd/helpers.h> ......................................................................
Abandoned
see https://review.coreboot.org/c/coreboot/+/41677
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Julius Werner (Code Review)
-
Paul Menzel (Code Review)