Change in coreboot[master]: src/soc/intel/commmon/itss: Add support to pass interrupt config to FSP
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 )
Change subject: src/soc/intel/commmon/itss: Add support to pass interrupt config to FSP ......................................................................
Patch Set 5:
(11 comments)
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/itss.h:
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 56: itss_configure_pirx It would be a good idea to add a comment indicating what the function really does and what params it is expecting.
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 56: void Why void?
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 56: intconfigptr int_config_ptr?
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... File src/soc/intel/common/block/itss/itss.c:
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 192: PIRx mapped from IRQ# 16:23 starting from PIRQA Shouldn't this info be provided by SoC specific code?
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 198: if (!islpss) { It would be good to add comments indicating the rules you are following for assigning IRQ#.
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 207: 16-23 Wouldn't this be dependent on SoC implementation?
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 225: if (!is_lpss(dev)) : /* Devices that have shared PIRx routing */ : irqlist = add_dev_irq(dev, false); : else : /* LPSS controllers needs unique IRQ assignments */ : irqlist = add_dev_irq(dev, true); This can just be: add_dev_irq(dev, is_lpss(dev));
In fact you can just make it add_dev_irq(dev) and let that function call is_lpss(dev).
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 236: itss_configure_pirx Can't this common block driver maintain a pointer to the interrupt config list and return that pointer when requested for instead of doing a memcpy always?
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... File src/soc/intel/common/block/itss/itss_def.h:
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 19: #define PIRQA 16 Shouldn't this be provided by SoC header? I believe it already exists somewhere in SoC header files.
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 22: uint8_t slot; : uint8_t func; : uint8_t int_pin; : uint8_t int_line; Can you please add comments indicating what these members mean? And what this structure represents?
https://review.coreboot.org/c/coreboot/+/34089/5/src/soc/intel/common/block/... PS5, Line 30: struct irq_node *next; Can we not use list_node here?
-
Furquan Shaikh (Code Review)