Attention is currently required from: David Milosevic, Julius Werner, Lean Sheng Tan, Martin L Roth, Maximilian Brune.
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78285?usp=email )
Change subject: arch/arm64: Implement initial set of SMBIOS tables ......................................................................
Patch Set 5:
(6 comments)
File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/78285/comment/303263ad_dc0ca93a : PS3, Line 919: depends on ARCH_X86 || ARCH_ARM64
I think adding the 'depends on ARCH_ARM64` is fine, but we should probably add a 'default n if ARCH_ […]
That sounds good to me. While it uses TFA services, which happen to need ARM64_CURRENT_EL < 3 (or there's a dead code related compiler error), SMBIOS isn't TFA related so I don't want to tie their configs together. We'll select it in our mainboard.
File src/arch/arm64/smbios.c:
https://review.coreboot.org/c/coreboot/+/78285/comment/51c7cf98_919f59b4 : PS3, Line 22: processor_id[1] = (midr_el1 >> 32);
The high 32 bits of MIDR_EL1 are reserved 0 anyway. […]
That won't return values anything else would return. Is that something to be concerned about?
https://review.coreboot.org/c/coreboot/+/78285/comment/b2cedacd_802c476d : PS3, Line 29: char buf[18];
Uhh... I think this is too short? I think you're cutting off the terminating NUL byte here. […]
Oops! Done.
https://review.coreboot.org/c/coreboot/+/78285/comment/19d24b69_95c42e15 : PS3, Line 34: implementor
nit: typo […]
It's similar to the Linux kernel's output, from memory.
https://review.coreboot.org/c/coreboot/+/78285/comment/794b50eb_3f59f4bc : PS3, Line 50: void __weak smbios_cpu_get_core_counts(u16 *core_count, u16 *thread_count)
Does this need to be implemented per platform? Aren't there architectural ID registers that could be […]
There's the MPIDR register, but that only contains the executing CPU's values. So, to get the largest value, we'd need to use MP services to start each possible CPU. Of course, by that point we could also use PSCI_CPU_ON errors to determine which is the largest.
The whole thing seemed too roundabout when TFA FDT's usually had CPU nodes we could count instead. The layout of those (as in, nesting) seems common, so the weak implementation could just count that. It's just a matter of getting the pointer to the FDT.
https://review.coreboot.org/c/coreboot/+/78285/comment/65baf234_63db196e : PS3, Line 56: #define MAX_CPUS_ENABLED (CONFIG_MAX_CPUS > 0xff ? 0xff : CONFIG_MAX_CPUS)
Note that CONFIG_MAX_CPUS is always 1 on Arm (we don't support SMP in coreboot on Arm and I don't th […]
CONFIG_MAX_CPUS is a Kconfig exclusively for SMP's use? Anyways, I'll change this to the macro we actually intend to use here.
-
Benjamin Doron (Code Review)