Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 38: uint16_t seg_nr = 0;
Assignment is not needed. See below.
agree we can avoid this as local variable
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 39: uint16_t pci_ex_len = (CONFIG_SA_PCIEX_LENGTH >> 20) - 1;
Why is the conversion needed? pci_ex_len and PCIEX_LENGTH sounds the same.
Right now assume CONFIG_SA_PCIEX_LENGTH = 0x1000_0000 which need to covert those into bus hence RSH with 20. Now to address ur comment we can use pciex_bus rather pci_ex_len
Maybe also use `pciex_len` as name?
Yes we will use pciex_bus
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 44: seg_nr = pci_ex_len / 256;
Move this out of the if statement, as x / 256 with x <= 255, is 0 for integers.
make sense
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 48: bus_end_nr = pci_ex_len;
That’s the same as above with `seg_nr = 0`. […]
make sense