Change in coreboot[master]: src/soc/intel/common/itss: Add support to get interrupt table for PCI...
Aamir Bohra 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:
(4 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 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 […]
Ack
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 So […]
Ok, can remove, removing the weak soc_irq_devices would make it fail for non supported platforms
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. […]
Yes. Agree, Will revise 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?
Since the implementation was to directly memcpy the interrupt config into FSP intconfig, I had done a memcpy here, or else we will have to move memcpy to SOC code and free the heap there. Which should be fine. thoughts?
-
Aamir Bohra (Code Review)