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>.