Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29398 )
Change subject: soc/intel/braswell/southcluster.c: Correct serial IRQ support ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/#/c/29398/10/src/soc/intel/braswell/southcluster... File src/soc/intel/braswell/southcluster.c:
https://review.coreboot.org/#/c/29398/10/src/soc/intel/braswell/southcluster... PS10, Line 633: Why move it from finalization to initialization? Was it tested that this works with both blobs? e.g. it might override our setting in an FSP notification phase.
https://review.coreboot.org/#/c/29398/10/src/soc/intel/braswell/southcluster... PS10, Line 97: (pci_read_config32(dev, IBASE) & ~0xF) Why use ILB_BASE_ADDRESS above and then this here?
https://review.coreboot.org/#/c/29398/10/src/soc/intel/braswell/southcluster... PS10, Line 102: } Why add this in a different place than sc_set_serial_irqs_mode()?
https://review.coreboot.org/#/c/29398/10/src/soc/intel/braswell/southcluster... PS10, Line 304: sc_enable_serial_irqs(dev);
No, required to set continuous mode for at least one frame before switching into quiet mode.
Ack, but it's not obvious, IMO. Please add a comment that we have to start in continuous mode.