Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33934 )
Change subject: lib: Rewrite qemu-armv7 ramdetect ......................................................................
Patch Set 3:
(9 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@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.
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 "ret" variable.
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?
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?
msb = 0;
/* other code */
do { msb++; } while (probe_size >> msb)
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)
/* other code */
msb = MIN(msb, MAX_ADDRESSABLE_SPACE);
(macro name is an example)
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:
i = msb + 1; do { i--; if (probe_mb(dram_start, (discovered | (1 << i)))) discovered |= (1 << i); } while (i > 0)
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)' Maybe 1UL or 1ULL if the unsigned type is larger.
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 ?
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 ?