[coreboot-gerrit] [XL] Change in coreboot[master]: ACPI,SMBIOS : Provide ACPI and SMBIOS supports for RISC-V

[Martin L Roth (Code Review)]

Attention is currently required from: Philipp Hug, Ron Minnich, Tim Wawrzynczak, weidongwd.

Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74799?usp=email )

Change subject: ACPI,SMBIOS : Provide ACPI and SMBIOS supports for RISC-V
......................................................................

Patch Set 1:

(20 comments)

Patchset:

PS1: 
First, Thank you for the patch. I know that getting large patches into coreboot can be difficult until you learn the conventions.

This patch should probably be broken up into several different patches.the first few patches into coreboot 
ACPI, SMBIOS, and cbmem are at least three different patches.

The whitespace issues also definitely need to be fixed.

Since there hasn't been any update since the initial patch, if you'd like someone to take over the patch and get it merged, please say so, otherwise nobody else is going to work on it.

Thanks again.

File src/Kconfig:

https://review.coreboot.org/c/coreboot/+/74799/comment/3a21f9dd_40917802 :
PS1, Line 859: 	#depends on ARCH_X86
> `depends on ARCH_X86 || ARCH_RISCV` ?

Please - this should still be limited to architectures that use SMBIOS.

File src/acpi/Kconfig:

https://review.coreboot.org/c/coreboot/+/74799/comment/e3e0e17c_afdd95e3 :
PS1, Line 75: 	bool
depends on HAVE_ACPI_TABLES

https://review.coreboot.org/c/coreboot/+/74799/comment/e8aeada7_cd70ba66 :
PS1, Line 80: 	bool
Default on if RISC-V?

Depends on HAVE_ACPI_TABLES

File src/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/74799/comment/21d2f292_d0a62060 :
PS1, Line 19: #if ENV_X86
We shouldn't ever need to #ifdef out header files.

https://review.coreboot.org/c/coreboot/+/74799/comment/f7e6c96c_42998163 :
PS1, Line 131: #if ENV_X86
Using an ifdef here isn't the right solution - if RISCV doesn't use this, maybe check and return if it's risc-v?

https://review.coreboot.org/c/coreboot/+/74799/comment/c0f22832_d8954334 :
PS1, Line 252: #if ENV_X86
return if not ENV_X86

https://review.coreboot.org/c/coreboot/+/74799/comment/fc026796_9232e549 :
PS1, Line 368: #if ENV_X86
Just use a regular `if (ENV_X86)` no need for #ifdef

https://review.coreboot.org/c/coreboot/+/74799/comment/6cb04d87_577a3249 :
PS1, Line 390: static void acpi_create_pptt (acpi_header_t *pptt)
> space prohibited between function name and open parenthesis '('

Please fix.

https://review.coreboot.org/c/coreboot/+/74799/comment/3f2e2c36_d6d6093f :
PS1, Line 418: 
             : #if CONFIG(ACPI_RTDT)
call the function and return if ACPI_RTDT isn't enabled?

https://review.coreboot.org/c/coreboot/+/74799/comment/1d8e5c2d_69af78e3 :
PS1, Line 1069: #if ENV_X86
unnecessary?

https://review.coreboot.org/c/coreboot/+/74799/comment/3f002784_3ed0976a :
PS1, Line 1115: #if ENV_X86
unnecessary?

https://review.coreboot.org/c/coreboot/+/74799/comment/a5eb3376_ad1e06b2 :
PS1, Line 1440: #if ENV_X86
Unnecessary?

https://review.coreboot.org/c/coreboot/+/74799/comment/c3ddb3ff_c5f99a80 :
PS1, Line 1826: #if CONFIG(USE_PC_CMOS_ALTCENTURY)
same - regular `if ()`

https://review.coreboot.org/c/coreboot/+/74799/comment/34d6d71f_bc1bf297 :
PS1, Line 1919: #if CONFIG(ACPI_RTDT)
Any reason these can't just be in all the time, even if they're not used?

https://review.coreboot.org/c/coreboot/+/74799/comment/7d0f8391_2f852ae6 :
PS1, Line 2151: #if CONFIG(ACPI_PPTT)
if()

File src/arch/riscv/smbios.c:

PS1:
> Much of this looks common to x86 & RISC-V, seems like it would be simpler to have a common file and  […]

Agreed - it would be better to not just copy the x86 version completely.  Maybe that's better done as a refactor later though? I expect that might be a bit much to expect from people new to coreboot.

File src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h:

https://review.coreboot.org/c/coreboot/+/74799/comment/3f2e5dfb_53cdad35 :
PS1, Line 18: #define CBMEM_ID_FDT		0x4c474455
Nit: Please put in alphabetical order, the way the rest of this table is.

Same below.

File src/include/acpi/acpi.h:

https://review.coreboot.org/c/coreboot/+/74799/comment/8b771d91_6198c184 :
PS1, Line 242:     u64 lo;
> please, no spaces at the start of a line

Please fix.

File src/soc/ucb/riscv/cbmem.c:

https://review.coreboot.org/c/coreboot/+/74799/comment/1ecb96d3_ee283635 :
PS1, Line 7: uintptr_t cbmem_top_chipset(void)
Break this change out into a separate patch?