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 IRQ configuration for PCI devices ......................................................................
Patch Set 9:
(12 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
Done
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 46: void *int_cfg_ptr, uint8_t *int_num
Ack
Done
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... File src/soc/intel/common/block/itss/irq.c:
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 144: }
From the rest of the CLs, it seems FSP is passing the int_cfg_ptr buffer and also the int_num as an […]
FSP is expecting the pointer to interrupt config table and size of the table. Updated the implementation to pass the pointer to FSP and data structure is now reserved in coreboot, This would now ensure callee does not have to reserve the buffer space and just point to buffer populated in coreboot.
Does this resolve the concern?
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; : }
Yes. Agree, Will revise it.
Done
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 36: node = (struct irq_node *)malloc(sizeof(struct irq_node)); : node->next = NULL;
What if malloc fails?
yeah , I had added this check in add_irq_entry function if (!temp)
return dev;
revised the implementation now, no longer creating a list
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 41: dev
nit: This isn't a device, so maybe 'node' would be a better name?
Ack, revised the implementation now, no longer creating a list
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 71: static struct irq_node *info; : static int slot; : /* PIRx mapped from IRQ# 16:23 starting from PIRQA */ : static int int_line = PIRQA_APIC_IRQ; : static int int_lpss = PIRQA_APIC_IRQ;
Will this really be called frequently enough to warrant storing all this state between calls?
This is more to keep track on previous assignment and use it abide by following IRQ allocation rules
/* * Add device IRQ information following below rules: * * 1. For single function PCI device capable of generating * interrupt, use INTA(IRQ16) * 2. For multi function PCI device capable of generating * interrupt, alteast one should use INTA(IRQ16) * 3. LPSS controllers need to be assigned unique IRQs */
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 93: * only interrupt 16-23 can be shared */ : if (int_line > 23)
If the comment is true, should you check if " || int_line < 16" as well ?
I think initializing it with static int int_line = PIRQA_APIC_IRQ would ensure it.
The check is only to reset IRQ allocation to 16 if it exceeds 23.
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 102: static bool is_irq_dev(struct device *dev) : { : static size_t size; : static const int *devlist; : : if (!devlist) : devlist = soc_irq_devices_list(&size); : for (int i = 0; i < size; i++) { : if (dev->path.pci.devfn == devlist[i]) : return true; : } : return false; : } :
Is this really going to be called frequently enough to need to store the devlist between calls?
yeah, for each pci device in the tree , wanted to avoid soc call backs
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 141: void *int_cfg_ptr
Can this be of type "struct dev_irq *" instead? void* makes me nervous :)
Ack
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 148: if (!irq_config) : return -1;
To be safe, and potentially not need callers to check for NULL, could this set *int_num = 0 as well?
Done. The caller from SOC is checking for it and setting int_num=0
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++; : }
I think the basic question here would be why do you need the list at all? The caller knows how many […]
yeah, that seems to be correct thing to do. removed the list implementation now.