Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35521 )
Change subject: [WIP] intel/pci: Utilise pci_def.h for PCI_BRIDGE_CONTROL ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35521/1/src/northbridge/intel/i945/... File src/northbridge/intel/i945/early_init.c:
https://review.coreboot.org/c/coreboot/+/35521/1/src/northbridge/intel/i945/... PS1, Line 555: pci_write_config16(PCI_DEV(0, 0x01, 0), PCI_BRIDGE_CONTROL, reg16); Looks like we could use pci_secondary_reset(int assert), the sequence here repeats a lot. Ir will be followup work at a later date.
https://review.coreboot.org/c/coreboot/+/35521/1/src/northbridge/intel/i945/... PS1, Line 666: reg16 |= PCI_BRIDGE_CTL_VGA; We can delay setting this to ramstage (with CTL_VGA16)?
https://review.coreboot.org/c/coreboot/+/35521/1/src/soc/intel/common/block/... File src/soc/intel/common/block/pcie/pcie.c:
https://review.coreboot.org/c/coreboot/+/35521/1/src/soc/intel/common/block/... PS1, Line 40: /* disable parity error response, enable ISA */ This "enable ISA" is interesting. Since specs calls it "ISA Enable" I think we should drop _NO_ from the bit definition name below, like PCI_CB_BRIDGE_CTL_ISA has done.
Also, I belive it is only necessary set the bit if there is some ISA hardware present that only does 10bit address decode. We are currently not consistent, we set this on PCIe root ports but not on other PCI-to-PCI bridges.
https://review.coreboot.org/c/coreboot/+/35521/1/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/pci.c:
https://review.coreboot.org/c/coreboot/+/35521/1/src/southbridge/intel/bd82x... PS1, Line 78: dev->command |= PCI_COMMAND_IO;
Yeah, now that I found cardbus specs, I think both IO and MEMORY should be set in PCI_COMMAND.
Ack
https://review.coreboot.org/c/coreboot/+/35521/1/src/southbridge/intel/bd82x... PS1, Line 86: ich_pci_dev_enable_resources(dev);
I am also staring at this comment, looking into CB:31987 that seems to be related. […]
Ack
https://review.coreboot.org/c/coreboot/+/35521/1/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/pci.c:
https://review.coreboot.org/c/coreboot/+/35521/1/src/southbridge/intel/i8280... PS1, Line 77: dev->command |= PCI_COMMAND_IO;
Hmm.. I think specs says VGA IO decode is enabled regardless of PCI_COMMAND_IO bit.
nvm.. I read the specs again.
https://review.coreboot.org/c/coreboot/+/35521/1/src/southbridge/intel/i8280... PS1, Line 80: ctrl |= (PCI_BRIDGE_CTL_PARITY | PCI_BRIDGE_CTL_SERR); /* error check */ Not sure why we set these for a short period of time, init() above disables them again.