Attention is currently required from: Arthur Heymans, Nico Huber, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 7: Code-Review+1
(8 comments)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/e700b346_971c034e PS4, Line 32: .macro find_free_mtrr
nit: Maybe don't indent this?
Done
https://review.coreboot.org/c/coreboot/+/37190/comment/d6652561_d2eaa0a6 PS4, Line 43: 2f
Are you sure one can use names in an assembler macro?
It works if the macro is only used once per file, but it probably won't work if used more than once
https://review.coreboot.org/c/coreboot/+/37190/comment/60d68a1b_b7523c6c PS4, Line 48: decl
Right. I would add a comment (suggestions welcome): […]
Ack
https://review.coreboot.org/c/coreboot/+/37190/comment/1cfee80b_516702d7 PS4, Line 122: cmp $0, %ebx
I'd use the idiomatic way: […]
Done
https://review.coreboot.org/c/coreboot/+/37190/comment/cd41c250_c467300c PS4, Line 123: jne 1f
Or even jz . […]
Done
https://review.coreboot.org/c/coreboot/+/37190/comment/a1dcc856_383ce52e PS4, Line 133: inc %ecx
Right. I wonder why "inc" is used here, but "incl" is used twice on the codepath below.
Ack
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/266975f4_3bc79092 PS6, Line 161: find_free_mtrr
Looks like the suggestions didn't reach this block
Ack
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/745aa384_c00f1d7f PS7, Line 152: cmp $0, %ebx : jne 1f : jmp .halt_forever Simpler:
test %ebx, %ebx jz .halt_forever