Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 )
Change subject: src/soc/intel/common/itss: Add support to get interrupt table for PCI devices ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/irq.h:
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 41: enrty entry
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 46: void *int_cfg_ptr, uint8_t *int_num What are these params? Can you please add help text? Also, what is the return value of this function?
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... File src/soc/intel/common/block/itss/Kconfig:
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 10: This feature is currently supported only for cannonlake : and icelake SOCs Do we need this text in here? I am just concerned that it is not feasible to update this when new SoCs use this config.
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... File src/soc/intel/common/block/itss/irq.c:
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 26: __weak const int *soc_irq_devices_list(size_t *size) : { : *size = 0; : return NULL; : } This file is guarded by SOC_INTEL_COMMON_ITSS_INTERRUPT_OVERRIDE. If a SoC is selecting this config, it should also be providing implementation of soc_irq_devices_list. We should not add a weak function for it.
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 150: while (irq_config) { : memcpy(dev, &(irq_config->irq), sizeof(struct dev_irq)); : tmp = irq_config; : irq_config = irq_config->next; : free(tmp); : dev++; : int_dev_entry++; : } Why is this doing a memcpy of the entire table? Can it not just pass a pointer to head of the list?