Nico Huber 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:
(5 comments)
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.
Yes, how could it be more clear?
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 ?
2f means 2 in forward direction, i.e. the next 2: in line 47
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 46: 1b
Is this a loop?
1 backwards, line 40
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 48: decl
why decl here?
MTRRs come in pairs, one BASE followed by one MASK reg. We've checked a bit in the MASK reg here, but want to return BASE, hence -1.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 133: inc %ecx
This looks wrong. […]
See above, returned %ecx is the BASE reg, +1 for the MASK reg.