Attention is currently required from: Shelley Chen, Hung-Te Lin, Furquan Shaikh, Paul Menzel, Yu-Ping Wu. Jianjun Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56791 )
Change subject: soc/mediatek: Add PCIe support ......................................................................
Patch Set 5:
(2 comments)
File src/soc/mediatek/common/pcie.c:
https://review.coreboot.org/c/coreboot/+/56791/comment/32324deb_b173de57 PS4, Line 194: - 1
There's already "(size) - 1" in PCIE_ATR_SIZE(). Please confirm if this extra "- 1" is needed.
Yes, the extra "-1" is needed.
fls(size) will get a value with one more bit offset, we need to get the actually value here. And the value we write to the ATR_SIZE still need to be subtracted by one base on the hardware design.
Here is the example: If we need to assign a table to 16MBytes size, the res->size will be 0x100_0000, and fls(res->size) = 25, but the actual bit offset should be 24. After all these operations, we write the value 23 into the ATR_SIZE register.
Base on the hardware design, the table size will be 2^(ATR_SIZE +1) = 2^(23 + 1)= 0x100_0000
https://review.coreboot.org/c/coreboot/+/56791/comment/3c4c3e47_085a8df2 PS4, Line 216: if (!res->size) : continue;
When will this happen?
I found some resource size will be 0, such as the resource which index is PCI_IO_BASE or PCI_MEMORY_BASE, I'm not sure if there is something incorrect, so I added this check to prevent break their resource size.
-
Jianjun Wang (Code Review)