Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40010 )
Change subject: sb/intel/bd82x6x/sata: Fix IDE legacy mode, plus more ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/c/coreboot/+/40010/2/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/sata.c:
https://review.coreboot.org/c/coreboot/+/40010/2/src/southbridge/intel/bd82x... PS2, Line 131: printk(BIOS_DEBUG, "SATA: Controller in plain mode.\n"); Into CB:39828, please.
https://review.coreboot.org/c/coreboot/+/40010/2/src/southbridge/intel/bd82x... PS2, Line 133: /* No AHCI: clear AHCI base */ : pci_write_config32(dev, 0x24, 0x00000000); : : /* And without AHCI BAR no memory decoding */ Felix, this should be a separate change. For the commit message:
Assignment of PCI resource registers is up to the allocator.
https://review.coreboot.org/c/coreboot/+/40010/2/src/southbridge/intel/bd82x... PS2, Line 149: /* Set Interrupt Line */ : /* Interrupt Pin is set by D31IP.PIP */ : pci_write_config8(dev, INTR_LN, 0xff); Can be a separate change:
Don't try to write read-only register.
https://review.coreboot.org/c/coreboot/+/40010/2/src/southbridge/intel/bd82x... PS2, Line 153: /* Set timings */ : pci_write_config16(dev, IDE_TIM_PRI, IDE_DECODE_ENABLE | : IDE_ISP_3_CLOCKS | IDE_RCT_1_CLOCKS | : IDE_PPE0 | IDE_IE0 | IDE_TIME0); : pci_write_config16(dev, IDE_TIM_SEC, IDE_DECODE_ENABLE | : IDE_SITRE | IDE_ISP_3_CLOCKS | : IDE_RCT_1_CLOCKS | IDE_IE0 | IDE_TIME0); : : /* Sync DMA */ : pci_write_config16(dev, IDE_SDMA_CNT, IDE_SSDE0 | IDE_PSDE0); : pci_write_config16(dev, IDE_SDMA_TIM, 0x0201); : : /* Set IDE I/O Configuration */ : reg32 = SIG_MODE_PRI_NORMAL | FAST_PCB1 | FAST_PCB0 | PCB1 | PCB0; : pci_write_config32(dev, IDE_CONFIG, reg32); These don't affect hardware according to documentation and, IIRC, vendor didn't set them either.
https://review.coreboot.org/c/coreboot/+/40010/2/src/southbridge/intel/bd82x... PS2, Line 43: static void sata_read_resources(struct device *dev) : { : struct resource *res; : : pci_dev_read_resources(dev); : : /* Assign fixed resources for IDE legacy mode */ : : u8 sata_mode = 0; : get_option(&sata_mode, "sata_mode"); : if (sata_mode != 2) : return; : : res = find_resource(dev, PCI_BASE_ADDRESS_0); : if (res) { : res->base = 0x1f0; : res->size = 8; : res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; : } : : res = find_resource(dev, PCI_BASE_ADDRESS_1); : if (res) { : res->base = 0x3f4; : res->size = 4; : res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; : } : : res = find_resource(dev, PCI_BASE_ADDRESS_2); : if (res) { : res->base = 0x170; : res->size = 8; : res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; : } : : res = find_resource(dev, PCI_BASE_ADDRESS_3); : if (res) { : res->base = 0x374; : res->size = 4; : res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; : } : } : : static void sata_set_resources(struct device *dev) : { : /* work around bug in pci_dev_set_resources(), it bails out on FIXED */ : u8 sata_mode = 0; : get_option(&sata_mode, "sata_mode"); : if (sata_mode == 2) { : unsigned int i; : for (i = PCI_BASE_ADDRESS_0; i <= PCI_BASE_ADDRESS_3; i += 4) { : struct resource *const res find_resource(dev, i); : if (res) : res->flags &= ~IORESOURCE_FIXED; : } : } : : pci_dev_set_resources(dev); : } Felix, please squash these into CB:39828.
https://review.coreboot.org/c/coreboot/+/40010/2/src/southbridge/intel/bd82x... PS2, Line 199: printk(BIOS_DEBUG, "SATA: Controller in IDE compat mode.\n"); Into CB:39828, please.
https://review.coreboot.org/c/coreboot/+/40010/2/src/southbridge/intel/bd82x... PS2, Line 203: printk(BIOS_DEBUG, "SATA: Controller in IDE legacy mode.\n"); Into CB:39828, please.
https://review.coreboot.org/c/coreboot/+/40010/2/src/southbridge/intel/bd82x... PS2, Line 260: 1 << 18 | 1 << 14 | 0x04 << 7 | 1 << 3); Separate change, please. For the commit message:
Set some things missed originally because of formatting issues in the BIOS spec. Values were compared with a vendor dump.
(it won't get better than this, the spec also just lists numbers)