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 4:
(8 comments)
I'm not sure if the number labels work as expected. They can be avoided.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 27: * macro: find_free_mtrr : * Clobbers %eax, %ebx, %ecx, %edx. : * If not found %ebx is 0. : * If found MTRR_BASE is at %ecx. Is this macro returning a tuple as (%ebx, %ecx) ? The comment is not very clear.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 32: .macro find_free_mtrr nit: Maybe don't indent this?
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 43: 2f What does this refer to? Maybe call it "mtrr_found" instead ?
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 46: 1b Is this a loop?
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 48: decl why decl here?
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 122: cmp $0, %ebx I'd use the idiomatic way:
test %ebx, %ebx
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 123: jne 1f
je . […]
Or even jz .halt_forever
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 133: inc %ecx This looks wrong. Why is the mask the result of incrementing ecx?