Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33934 )
Change subject: lib: Rewrite qemu-armv7 ramdetect ......................................................................
Patch Set 5:
(10 comments)
https://review.coreboot.org/c/coreboot/+/33934/3/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/33934/3/src/lib/ramdetect.c@14 PS3, Line 14: stddef.h
<types.h> […]
Done
https://review.coreboot.org/c/coreboot/+/33934/3/src/lib/ramdetect.c@23 PS3, Line 23: #define OVERLAP(a, b, s, e) ((b) > (a) && (a) < (e))
Um, how does this work? I think the first comparison is wrong, but I am not sure.
Done
https://review.coreboot.org/c/coreboot/+/33934/3/src/lib/ramdetect.c@37 PS3, Line 37: write32(ptr, PATTERN1); : if (read32(ptr) != PATTERN1) : goto out; : write32(ptr, PATTERN2); : if (read32(ptr) != PATTERN2) : goto out;
Maybe write this as a loop, and put the patterns in an array? This would also allow removing the "re […]
Done
https://review.coreboot.org/c/coreboot/+/33934/3/src/lib/ramdetect.c@52 PS3, Line 52: int i; : int msb = 1;
Shouldn't these be unsigned?
Done
https://review.coreboot.org/c/coreboot/+/33934/3/src/lib/ramdetect.c@61 PS3, Line 61: size_t tmp = probe_size; : while (tmp >>= 1) : msb++;
Wouldn't this work as well? […]
Done
https://review.coreboot.org/c/coreboot/+/33934/3/src/lib/ramdetect.c@66 PS3, Line 66: if (msb > (sizeof(size_t) * 8 - 20)) /* subtract 20 as probe_size is in MiB */ : msb = sizeof(size_t) * 8 - 20;
#define MAX_ADDRESSABLE_SPACE (sizeof(size_t) * 8 - 20) […]
Done
https://review.coreboot.org/c/coreboot/+/33934/3/src/lib/ramdetect.c@70 PS3, Line 70: for (i = msb; i >= 0; i--)
If using unsigned for i and msb, this loop might need to be rewritten. This may work: […]
using signed integer. Prevent an overflow by subtracting 1 in MAX_ADDRESSABLE_SPACE.
https://review.coreboot.org/c/coreboot/+/33934/3/src/lib/ramdetect.c@71 PS3, Line 71: 1 << i
If using unsigned values, this should be: '(1U << i)' […]
Done
https://review.coreboot.org/c/coreboot/+/33934/3/src/mainboard/emulation/qem... File src/mainboard/emulation/qemu-riscv/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33934/3/src/mainboard/emulation/qem... PS3, Line 30: << 10
- KiB ?
Done
https://review.coreboot.org/c/coreboot/+/33934/3/src/soc/ucb/riscv/cbmem.c File src/soc/ucb/riscv/cbmem.c:
https://review.coreboot.org/c/coreboot/+/33934/3/src/soc/ucb/riscv/cbmem.c@2... PS3, Line 20: << 20
- MiB ?
Done