Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/cannonlake/in... PS2, Line 56: true
Using something like sa_mmio_descriptor_64 to move the "true" into header file?
yes, this is possible to create but then we might not be able to use a single function due to 2 different structure dependency in argument hence decided of having single function which will act based on is_64_bit structure variable. Right now all these fixed resources are 64 bit wide that might be the reason it looks like little odd as multiple true but avoiding that might required more work to be done to support dedicated structure and separation in calling function from soc to common code as i have explained in common file review comments
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 94: if (is64bit)
Without that condition? Writing zero shall be no harm?
Code here is to check if BAR has be implemented or not
uint64_t base = 0; unsigned int index; bool is64bit = fixed_set_resources[i].is_64_bit;
index = fixed_set_resources[i].index; /* Check if PCI BAR already enabled */ if (is64bit) { base = pci_read_config32(SA_DEV_ROOT, index + 4); base <<= 32; } base |= pci_read_config32(SA_DEV_ROOT, index);
And writing into register would only depends on base = fixed_set_resources[i].base; so i don/'t think we are writing all zero ? isn't it ?
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 80: /* Check if PCI BAR already enabled */ : if (is64bit) { : base = pci_read_config32(SA_DEV_ROOT, index + 4); : base <<= 32; : } : base |= pci_read_config32(SA_DEV_ROOT, index); : : /* If enabled don't program it. */ : if (base & 0x1) : return; : : base = fixed_set_resources[i].base; : : pci_write_config32(SA_DEV_ROOT, index, base | 1); : if (is64bit) : pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32); : }
My personal opinion will have two separated body, one for 32 bit and another for 64 bit will be more […]
yes we can do that, but then it would be redundant and caller also might not 100% aware which function to call 32bit or 64 bit write hence decided of having single function which will act based on is_64_bit structure variable.