Change in ...coreboot[master]: sb/intel/i82801gx: Detect if the southbridge supports AHCI
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30822
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
sb/intel/i82801gx: Detect if the southbridge supports AHCI
This automatically detects whether the southbridge supports AHCI. If AHCI support is selected it will be used unless "sata_no_ahci" is set in the devicetree to override the behavior.
Change-Id: I8d9f4e63ae8b2862c422938f3103c44e761bcda4 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/apple/macbook21/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb M src/mainboard/asus/p5gc-mx/devicetree.cb M src/mainboard/asus/p5qpl-am/devicetree.cb M src/mainboard/foxconn/d41s/devicetree.cb M src/mainboard/foxconn/g41s-k/devicetree.cb M src/mainboard/getac/p470/devicetree.cb M src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb M src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb M src/mainboard/ibase/mb899/devicetree.cb M src/mainboard/intel/d510mo/devicetree.cb M src/mainboard/intel/d945gclf/devicetree.cb M src/mainboard/intel/dg41wv/devicetree.cb M src/mainboard/kontron/986lcd-m/devicetree.cb M src/mainboard/lenovo/t60/devicetree.cb M src/mainboard/lenovo/thinkcentre_a58/devicetree.cb M src/mainboard/lenovo/x60/devicetree.cb M src/mainboard/lenovo/z61t/devicetree.cb M src/mainboard/roda/rk886ex/devicetree.cb M src/southbridge/intel/i82801gx/chip.h M src/southbridge/intel/i82801gx/i82801gx.h M src/southbridge/intel/i82801gx/sata.c 25 files changed, 18 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/30822/1
diff --git a/src/mainboard/apple/macbook21/devicetree.cb b/src/mainboard/apple/macbook21/devicetree.cb index 898aae4..84c923b 100644 --- a/src/mainboard/apple/macbook21/devicetree.cb +++ b/src/mainboard/apple/macbook21/devicetree.cb @@ -64,7 +64,6 @@ register "gpi1_routing" = "2" register "gpi7_routing" = "2"
- register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x04"
register "gpe0_en" = "0x11000006" diff --git a/src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb b/src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb index 156fe3f..acb8ac6 100644 --- a/src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb +++ b/src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb @@ -51,7 +51,6 @@ register "gpi13_routing" = "2"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x440"
diff --git a/src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb b/src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb index ba2f00d..f4d1dc4 100644 --- a/src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb +++ b/src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb @@ -46,7 +46,6 @@ register "pirqh_routing" = "0x0b"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x440"
diff --git a/src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb b/src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb index 2fa0fe4..2b74e7e 100644 --- a/src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb +++ b/src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb @@ -46,7 +46,6 @@ register "pirqh_routing" = "0x0b"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x440"
diff --git a/src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb b/src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb index b458115..5479faf 100644 --- a/src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb +++ b/src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb @@ -46,7 +46,6 @@ register "pirqh_routing" = "0x0b"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "gpe0_en" = "0x440"
device pci 1b.0 on # Audio diff --git a/src/mainboard/asus/p5gc-mx/devicetree.cb b/src/mainboard/asus/p5gc-mx/devicetree.cb index 2f7d278..de63da2 100644 --- a/src/mainboard/asus/p5gc-mx/devicetree.cb +++ b/src/mainboard/asus/p5gc-mx/devicetree.cb @@ -53,7 +53,6 @@ register "ide_legacy_combined" = "0x0" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0"
register "p_cnt_throttling_supported" = "0"
diff --git a/src/mainboard/asus/p5qpl-am/devicetree.cb b/src/mainboard/asus/p5qpl-am/devicetree.cb index 5bf582b..4e39ac5 100644 --- a/src/mainboard/asus/p5qpl-am/devicetree.cb +++ b/src/mainboard/asus/p5qpl-am/devicetree.cb @@ -43,7 +43,6 @@ # 2 SCI (if corresponding GPIO_EN bit is also set)
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "gpe0_en" = "0x04000440"
device pci 1b.0 on end # Audio diff --git a/src/mainboard/foxconn/d41s/devicetree.cb b/src/mainboard/foxconn/d41s/devicetree.cb index 75df88e..a611ee3 100644 --- a/src/mainboard/foxconn/d41s/devicetree.cb +++ b/src/mainboard/foxconn/d41s/devicetree.cb @@ -40,7 +40,6 @@ register "pirqf_routing" = "0x0b" register "pirqg_routing" = "0x0b" register "pirqh_routing" = "0x0b" - register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x441"
diff --git a/src/mainboard/foxconn/g41s-k/devicetree.cb b/src/mainboard/foxconn/g41s-k/devicetree.cb index 237e22d..a00718f 100644 --- a/src/mainboard/foxconn/g41s-k/devicetree.cb +++ b/src/mainboard/foxconn/g41s-k/devicetree.cb @@ -48,7 +48,6 @@
register "ide_enable_primary" = "0x0" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0" # AHCI does not work register "sata_ports_implemented" = "0x3"
device pci 1b.0 on end # Audio diff --git a/src/mainboard/getac/p470/devicetree.cb b/src/mainboard/getac/p470/devicetree.cb index a81ef3a..f2a3912 100644 --- a/src/mainboard/getac/p470/devicetree.cb +++ b/src/mainboard/getac/p470/devicetree.cb @@ -57,7 +57,6 @@ register "ide_legacy_combined" = "0x1" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0"
register "c3_latency" = "85" register "docking_supported" = "1" diff --git a/src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb b/src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb index 7ed4d19..1c69613 100644 --- a/src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb +++ b/src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb @@ -76,7 +76,6 @@ register "ide_legacy_combined" = "0x0" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0" register "c3_latency" = "85"
register "p_cnt_throttling_supported" = "0" diff --git a/src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb b/src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb index 8b47c4f..d24eb5d 100644 --- a/src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb +++ b/src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb @@ -48,7 +48,6 @@ register "ide_legacy_combined" = "0x0" # Combined mode broken register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0" # AHCI does not work register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x40"
diff --git a/src/mainboard/ibase/mb899/devicetree.cb b/src/mainboard/ibase/mb899/devicetree.cb index 3544c96..8b170e1 100644 --- a/src/mainboard/ibase/mb899/devicetree.cb +++ b/src/mainboard/ibase/mb899/devicetree.cb @@ -36,7 +36,6 @@ register "ide_legacy_combined" = "0x0" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x1"
register "c3_latency" = "85" register "p_cnt_throttling_supported" = "0" diff --git a/src/mainboard/intel/d510mo/devicetree.cb b/src/mainboard/intel/d510mo/devicetree.cb index a008610..825611e 100644 --- a/src/mainboard/intel/d510mo/devicetree.cb +++ b/src/mainboard/intel/d510mo/devicetree.cb @@ -38,7 +38,6 @@ register "pirqf_routing" = "0x0b" register "pirqg_routing" = "0x0b" register "pirqh_routing" = "0x0b" - register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x20000040"
diff --git a/src/mainboard/intel/d945gclf/devicetree.cb b/src/mainboard/intel/d945gclf/devicetree.cb index 716654c..573b9c8 100644 --- a/src/mainboard/intel/d945gclf/devicetree.cb +++ b/src/mainboard/intel/d945gclf/devicetree.cb @@ -50,7 +50,6 @@ register "ide_legacy_combined" = "0x0" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0" register "c3_latency" = "85" register "p_cnt_throttling_supported" = "0"
diff --git a/src/mainboard/intel/dg41wv/devicetree.cb b/src/mainboard/intel/dg41wv/devicetree.cb index be28763..295fbc4 100644 --- a/src/mainboard/intel/dg41wv/devicetree.cb +++ b/src/mainboard/intel/dg41wv/devicetree.cb @@ -63,7 +63,6 @@ register "gpi15_routing" = "2"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "gpe0_en" = "0x440"
device pci 1b.0 on # Audio diff --git a/src/mainboard/kontron/986lcd-m/devicetree.cb b/src/mainboard/kontron/986lcd-m/devicetree.cb index 06f29e0..023c516 100644 --- a/src/mainboard/kontron/986lcd-m/devicetree.cb +++ b/src/mainboard/kontron/986lcd-m/devicetree.cb @@ -36,7 +36,6 @@ register "ide_legacy_combined" = "0x1" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x1" - register "sata_ahci" = "0x0" register "c3_latency" = "85" register "p_cnt_throttling_supported" = "0"
diff --git a/src/mainboard/lenovo/t60/devicetree.cb b/src/mainboard/lenovo/t60/devicetree.cb index 7b1721e..67b7794 100644 --- a/src/mainboard/lenovo/t60/devicetree.cb +++ b/src/mainboard/lenovo/t60/devicetree.cb @@ -72,7 +72,6 @@ register "gpi12_routing" = "2" register "gpi8_routing" = "2"
- register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x01"
register "gpe0_en" = "0x11000006" diff --git a/src/mainboard/lenovo/thinkcentre_a58/devicetree.cb b/src/mainboard/lenovo/thinkcentre_a58/devicetree.cb index cc3ef49..ace2bfb 100644 --- a/src/mainboard/lenovo/thinkcentre_a58/devicetree.cb +++ b/src/mainboard/lenovo/thinkcentre_a58/devicetree.cb @@ -44,7 +44,6 @@ register "gpi13_routing" = "1" # ??vendor
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "gpe0_en" = "0x440"
device pci 1b.0 on end # Audio diff --git a/src/mainboard/lenovo/x60/devicetree.cb b/src/mainboard/lenovo/x60/devicetree.cb index 2a60b87..ad31557 100644 --- a/src/mainboard/lenovo/x60/devicetree.cb +++ b/src/mainboard/lenovo/x60/devicetree.cb @@ -65,7 +65,6 @@ register "gpi12_routing" = "1" register "gpi8_routing" = "2"
- register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x01"
register "gpe0_en" = "0x11000006" diff --git a/src/mainboard/lenovo/z61t/devicetree.cb b/src/mainboard/lenovo/z61t/devicetree.cb index 92bf3ce..6e2450a 100644 --- a/src/mainboard/lenovo/z61t/devicetree.cb +++ b/src/mainboard/lenovo/z61t/devicetree.cb @@ -71,7 +71,6 @@ register "gpi12_routing" = "2" register "gpi8_routing" = "2"
- register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x01"
register "gpe0_en" = "0x11000006" diff --git a/src/mainboard/roda/rk886ex/devicetree.cb b/src/mainboard/roda/rk886ex/devicetree.cb index ec42766..efe8ce1 100644 --- a/src/mainboard/roda/rk886ex/devicetree.cb +++ b/src/mainboard/roda/rk886ex/devicetree.cb @@ -61,7 +61,6 @@ register "ide_legacy_combined" = "0x1" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0"
device pci 1b.0 off end # High Definition Audio device pci 1c.0 on end # PCIe port 1 diff --git a/src/southbridge/intel/i82801gx/chip.h b/src/southbridge/intel/i82801gx/chip.h index db27ef7..019c65e 100644 --- a/src/southbridge/intel/i82801gx/chip.h +++ b/src/southbridge/intel/i82801gx/chip.h @@ -65,7 +65,7 @@ uint32_t ide_legacy_combined; uint32_t ide_enable_primary; uint32_t ide_enable_secondary; - uint32_t sata_ahci; + uint32_t sata_no_ahci; uint32_t sata_ports_implemented;
/* Enable linear PCIe Root Port function numbers starting at zero */ diff --git a/src/southbridge/intel/i82801gx/i82801gx.h b/src/southbridge/intel/i82801gx/i82801gx.h index d37a822..521d372 100644 --- a/src/southbridge/intel/i82801gx/i82801gx.h +++ b/src/southbridge/intel/i82801gx/i82801gx.h @@ -90,6 +90,7 @@
#define FDVCT 0xe4 #define PCIE_4_PORTS_MAX (1 << 7) +#define AHCI_UNSUPPORTED (1 << 3)
/* GEN_PMCON_3 bits */ #define RTC_BATTERY_DEAD (1 << 2) diff --git a/src/southbridge/intel/i82801gx/sata.c b/src/southbridge/intel/i82801gx/sata.c index 567c1e5..dcd9b80 100644 --- a/src/southbridge/intel/i82801gx/sata.c +++ b/src/southbridge/intel/i82801gx/sata.c @@ -71,6 +71,10 @@ u32 *ahci_bar; u8 ports;
+ + struct device *lpc_dev = pcidev_path_on_root(PCI_DEVFN(31, 0)); + bool ahci_supported = pci_read_config32(lpc_dev, FDVCT) & ACHI_UNSUPPORTED ? false : true; + /* Get the chip configuration */ config_t *config = dev->chip_info;
@@ -87,7 +91,18 @@ /* Enable BARs */ pci_write_config16(dev, PCI_COMMAND, 0x0007);
- if (config->ide_legacy_combined) { + if (ahci_supported && !config->sata_no_ahci) { + printk(BIOS_DEBUG, "SATA controller in AHCI mode.\n"); + /* Allow both Legacy and Native mode */ + pci_write_config8(dev, 0x09, 0x8f); + + /* Set Interrupt Line */ + /* Interrupt Pin is set by D31IP.PIP */ + pci_write_config8(dev, INTR_LN, 0x0a); + + ahci_bar = (u32 *)(pci_read_config32(dev, 0x27) & ~0x3ff); + ahci_bar[3] = config->sata_ports_implemented; + } else if (config->ide_legacy_combined) { printk(BIOS_DEBUG, "SATA controller in combined mode.\n"); /* No AHCI: clear AHCI base */ pci_write_config32(dev, 0x24, 0x00000000); @@ -118,17 +133,6 @@
/* Restrict ports - 0 and 2 only available */ ports &= 0x5; - } else if (config->sata_ahci) { - printk(BIOS_DEBUG, "SATA controller in AHCI mode.\n"); - /* Allow both Legacy and Native mode */ - pci_write_config8(dev, 0x09, 0x8f); - - /* Set Interrupt Line */ - /* Interrupt Pin is set by D31IP.PIP */ - pci_write_config8(dev, INTR_LN, 0x0a); - - ahci_bar = (u32 *)(pci_read_config32(dev, 0x27) & ~0x3ff); - ahci_bar[3] = config->sata_ports_implemented; } else { printk(BIOS_DEBUG, "SATA controller in plain mode.\n"); /* Set Sata Controller Mode. No Mapping(?) */
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... PS5, Line 75: ret = ahci_supported && config->sata_no_ahci; What's the reason for having the sata_no_ahci option?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... PS5, Line 75: ret = ahci_supported && config->sata_no_ahci;
What's the reason for having the sata_no_ahci option?
previously there was an option to set whatever mode you wanted on systems supporting AHCI which this option allows for. If that is not desirable, I can drop it.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 5: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... PS5, Line 75: ret = ahci_supported && config->sata_no_ahci;
previously there was an option to set whatever mode you wanted on systems supporting AHCI which this […]
I don't see a reason to have this option
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... PS5, Line 75: ret = ahci_supported && config->sata_no_ahci;
I don't see a reason to have this option
Without this option, the `else if (config->ide_legacy_combined)` below would be unreachable on boards that support AHCI. I wouldn't mind to remove combined mode altogether. But as long as we maintain it, we should provide reasonable devicetree options.
IMHO, it should be an `enum sata_mode { ahci, ide, legacy, legacy_ combined };`. Unless we'd ditch combined mode. Then we could also unify the sata code with everything up to BDW.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/5/src/southbridge/intel/i82801gx/sata.... PS5, Line 75: ret = ahci_supported && config->sata_no_ahci;
Without this option, the `else if (config->ide_legacy_combined)` below would be unreachable on boards that support AHCI. I wouldn't mind to remove combined mode altogether. But as long as we maintain it, we should provide reasonable devicetree options.
IMHO, it should be an `enum sata_mode { ahci, ide, legacy, legacy_ combined };`. Unless we'd ditch combined mode. Then we could also unify the sata code with everything up to BDW.
Iirc combined mode does not even work properly with this code...
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30822
to look at the new patch set (#7).
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
sb/intel/i82801gx: Detect if the southbridge supports AHCI
This automatically detects whether the southbridge supports AHCI. If AHCI support is selected it will be used unless "sata_no_ahci" is set in the devicetree to override the behavior.
Change-Id: I8d9f4e63ae8b2862c422938f3103c44e761bcda4 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/apple/macbook21/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb M src/mainboard/asus/p5gc-mx/devicetree.cb M src/mainboard/asus/p5qpl-am/devicetree.cb M src/mainboard/foxconn/d41s/devicetree.cb M src/mainboard/foxconn/g41s-k/devicetree.cb M src/mainboard/getac/p470/devicetree.cb M src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb M src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb M src/mainboard/ibase/mb899/devicetree.cb M src/mainboard/intel/d510mo/devicetree.cb M src/mainboard/intel/d945gclf/devicetree.cb M src/mainboard/intel/dg41wv/devicetree.cb M src/mainboard/kontron/986lcd-m/devicetree.cb M src/mainboard/lenovo/t60/devicetree.cb M src/mainboard/lenovo/thinkcentre_a58/devicetree.cb M src/mainboard/lenovo/x60/devicetree.cb M src/mainboard/lenovo/z61t/devicetree.cb M src/mainboard/roda/rk886ex/devicetree.cb M src/southbridge/intel/i82801gx/chip.h M src/southbridge/intel/i82801gx/i82801gx.h M src/southbridge/intel/i82801gx/ide.c M src/southbridge/intel/i82801gx/sata.c 26 files changed, 56 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/30822/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/30822/7/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/7/src/southbridge/intel/i82801gx/sata.... PS7, Line 159: ahci_bar = (u32 *)(pci_read_config32(dev, 0x27) & ~0x3ff); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... PS8, Line 159: ahci_bar = (u32 *)(pci_read_config32(dev, 0x27) & ~0x3ff); line over 80 characters
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/chip.... File src/southbridge/intel/i82801gx/chip.h:
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/chip.... PS8, Line 20: #include <southbridge/intel/i82801gx/i82801gx.h> Would it be better to define the enum here?
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/ide.c File src/southbridge/intel/i82801gx/ide.c:
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/ide.c... PS8, Line 22: #include "chip.h" ...
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... PS8, Line 53: static int ahci_supported = -1; Some idea: Update `config->sata_mode` instead. That would save some lines, only have to be called once and probably make the `switch` logic in sata_init() easier.
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... PS8, Line 58: pcidev_path_on_root or:
pcidev_on_root(31, 0)
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... PS8, Line 160: ahci_bar[3] = config->sata_ports_implemented; break here?
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30822
to look at the new patch set (#9).
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
sb/intel/i82801gx: Detect if the southbridge supports AHCI
This automatically detects whether the southbridge supports AHCI. If AHCI support is selected it will be used unless "sata_no_ahci" is set in the devicetree to override the behavior.
Change-Id: I8d9f4e63ae8b2862c422938f3103c44e761bcda4 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/apple/macbook21/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb M src/mainboard/asus/p5gc-mx/devicetree.cb M src/mainboard/asus/p5qpl-am/devicetree.cb M src/mainboard/foxconn/d41s/devicetree.cb M src/mainboard/foxconn/g41s-k/devicetree.cb M src/mainboard/getac/p470/devicetree.cb M src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb M src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb M src/mainboard/ibase/mb899/devicetree.cb M src/mainboard/intel/d510mo/devicetree.cb M src/mainboard/intel/d945gclf/devicetree.cb M src/mainboard/intel/dg41wv/devicetree.cb M src/mainboard/kontron/986lcd-m/devicetree.cb M src/mainboard/lenovo/t60/devicetree.cb M src/mainboard/lenovo/thinkcentre_a58/devicetree.cb M src/mainboard/lenovo/x60/devicetree.cb M src/mainboard/lenovo/z61t/devicetree.cb M src/mainboard/roda/rk886ex/devicetree.cb M src/southbridge/intel/i82801gx/chip.h M src/southbridge/intel/i82801gx/i82801gx.h M src/southbridge/intel/i82801gx/sata.c 25 files changed, 37 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/30822/9
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 9:
(10 comments)
i don't see a reason why AHCI should be disabled on ICHs that support AHCI; just want the confirmation that the changes are intended and probably won't make a difference
https://review.coreboot.org/#/c/30822/9/src/mainboard/asus/p5gc-mx/devicetre... File src/mainboard/asus/p5gc-mx/devicetree.cb:
https://review.coreboot.org/#/c/30822/9/src/mainboard/asus/p5gc-mx/devicetre... PS9, Line 56: does the ICH on this board support AHCI?
https://review.coreboot.org/#/c/30822/9/src/mainboard/foxconn/g41s-k/devicet... File src/mainboard/foxconn/g41s-k/devicetree.cb:
https://review.coreboot.org/#/c/30822/9/src/mainboard/foxconn/g41s-k/devicet... PS9, Line 50: does this change behavior?
https://review.coreboot.org/#/c/30822/9/src/mainboard/getac/p470/devicetree.... File src/mainboard/getac/p470/devicetree.cb:
https://review.coreboot.org/#/c/30822/9/src/mainboard/getac/p470/devicetree.... PS9, Line 60: same question here
https://review.coreboot.org/#/c/30822/9/src/mainboard/gigabyte/ga-945gcm-s2l... File src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb:
https://review.coreboot.org/#/c/30822/9/src/mainboard/gigabyte/ga-945gcm-s2l... PS9, Line 79: same question here
https://review.coreboot.org/#/c/30822/9/src/mainboard/gigabyte/ga-g41m-es2l/... File src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb:
https://review.coreboot.org/#/c/30822/9/src/mainboard/gigabyte/ga-g41m-es2l/... PS9, Line 51: same question here
https://review.coreboot.org/#/c/30822/9/src/mainboard/intel/d945gclf/devicet... File src/mainboard/intel/d945gclf/devicetree.cb:
https://review.coreboot.org/#/c/30822/9/src/mainboard/intel/d945gclf/devicet... PS9, Line 53: same question here
https://review.coreboot.org/#/c/30822/9/src/mainboard/kontron/986lcd-m/devic... File src/mainboard/kontron/986lcd-m/devicetree.cb:
https://review.coreboot.org/#/c/30822/9/src/mainboard/kontron/986lcd-m/devic... PS9, Line 39: same question here
https://review.coreboot.org/#/c/30822/9/src/mainboard/roda/rk886ex/devicetre... File src/mainboard/roda/rk886ex/devicetree.cb:
https://review.coreboot.org/#/c/30822/9/src/mainboard/roda/rk886ex/devicetre... PS9, Line 64: same question here
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... PS9, Line 58: likely to be able to use additional functions of a pci device, function 0 has to be enabled
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... PS9, Line 63: return; is it correct that pci_write_config8(dev, SATA_MAP, pci_read_config8(dev, SATA_MAP) & ~0xc3) doesn't get run in this case? this is at least a difference to the previous the code flow.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/30822/9/src/mainboard/asus/p5gc-mx/devicetre... File src/mainboard/asus/p5gc-mx/devicetree.cb:
https://review.coreboot.org/#/c/30822/9/src/mainboard/asus/p5gc-mx/devicetre... PS9, Line 56:
does the ICH on this board support AHCI?
No, ICH7 has variants that don't support it. Now the code checks for support in the code.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/ide.c File src/southbridge/intel/i82801gx/ide.c:
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/ide.c... PS8, Line 22: #include "chip.h"
...
Done
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... PS8, Line 53: static int ahci_supported = -1;
Some idea: Update `config->sata_mode` instead. That would save some […]
Done
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... PS8, Line 58: pcidev_path_on_root
or: […]
Done
https://review.coreboot.org/#/c/30822/8/src/southbridge/intel/i82801gx/sata.... PS8, Line 160: ahci_bar[3] = config->sata_ports_implemented;
break here?
Done
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... PS9, Line 54: struct southbridge_intel_i82801gx_config *config = dev->chip_info; The whole part to check for AHCI support needs only be run `if (config->sata_mode == SATA_MODE_AHCI)`.
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... PS9, Line 63: return;
is it correct that pci_write_config8(dev, SATA_MAP, pci_read_config8(dev, SATA_MAP) & ~0xc3) doesn't […]
I guess it should be
config->sata_mode = SATA_MODE_IDE_PLAIN; } else { /* check ahci_supported */
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30822
to look at the new patch set (#10).
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
sb/intel/i82801gx: Detect if the southbridge supports AHCI
This automatically detects whether the southbridge supports AHCI. If AHCI support is selected it will be used unless "sata_no_ahci" is set in the devicetree to override the behavior.
Change-Id: I8d9f4e63ae8b2862c422938f3103c44e761bcda4 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/apple/macbook21/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb M src/mainboard/asus/p5gc-mx/devicetree.cb M src/mainboard/asus/p5qpl-am/devicetree.cb M src/mainboard/foxconn/d41s/devicetree.cb M src/mainboard/foxconn/g41s-k/devicetree.cb M src/mainboard/getac/p470/devicetree.cb M src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb M src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb M src/mainboard/ibase/mb899/devicetree.cb M src/mainboard/intel/d510mo/devicetree.cb M src/mainboard/intel/d945gclf/devicetree.cb M src/mainboard/intel/dg41wv/devicetree.cb M src/mainboard/kontron/986lcd-m/devicetree.cb M src/mainboard/lenovo/t60/devicetree.cb M src/mainboard/lenovo/thinkcentre_a58/devicetree.cb M src/mainboard/lenovo/x60/devicetree.cb M src/mainboard/lenovo/z61t/devicetree.cb M src/mainboard/roda/rk886ex/devicetree.cb M src/southbridge/intel/i82801gx/chip.h M src/southbridge/intel/i82801gx/i82801gx.h M src/southbridge/intel/i82801gx/sata.c 25 files changed, 46 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/30822/10
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/#/c/30822/10/src/southbridge/intel/i82801gx/sata... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/10/src/southbridge/intel/i82801gx/sata... PS10, Line 84: /* Set map to ide */ code indent should use tabs where possible
https://review.coreboot.org/#/c/30822/10/src/southbridge/intel/i82801gx/sata... PS10, Line 84: /* Set map to ide */ please, no space before tabs
https://review.coreboot.org/#/c/30822/10/src/southbridge/intel/i82801gx/sata... PS10, Line 84: /* Set map to ide */ please, no spaces at the start of a line
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30822
to look at the new patch set (#11).
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
sb/intel/i82801gx: Detect if the southbridge supports AHCI
This automatically detects whether the southbridge supports AHCI. If AHCI support is selected it will be used unless "sata_no_ahci" is set in the devicetree to override the behavior.
Change-Id: I8d9f4e63ae8b2862c422938f3103c44e761bcda4 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/apple/macbook21/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb M src/mainboard/asus/p5gc-mx/devicetree.cb M src/mainboard/asus/p5qpl-am/devicetree.cb M src/mainboard/foxconn/d41s/devicetree.cb M src/mainboard/foxconn/g41s-k/devicetree.cb M src/mainboard/getac/p470/devicetree.cb M src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb M src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb M src/mainboard/ibase/mb899/devicetree.cb M src/mainboard/intel/d510mo/devicetree.cb M src/mainboard/intel/d945gclf/devicetree.cb M src/mainboard/intel/dg41wv/devicetree.cb M src/mainboard/kontron/986lcd-m/devicetree.cb M src/mainboard/lenovo/t60/devicetree.cb M src/mainboard/lenovo/thinkcentre_a58/devicetree.cb M src/mainboard/lenovo/x60/devicetree.cb M src/mainboard/lenovo/z61t/devicetree.cb M src/mainboard/roda/rk886ex/devicetree.cb M src/southbridge/intel/i82801gx/chip.h M src/southbridge/intel/i82801gx/i82801gx.h M src/southbridge/intel/i82801gx/sata.c 25 files changed, 46 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/30822/11
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... PS9, Line 54: struct southbridge_intel_i82801gx_config *config = dev->chip_info;
The whole part to check for AHCI support needs only be run […]
Done
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... PS11, Line 64: return; This still bails out too early. It should skip the `ahci_supported` part but still to the SATA_MAP programming below.
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... PS11, Line 87: /* At this point, the new pci id will appear on the bus */ I think code flow could benefit here by further separating the support-check from the hardware configuration. e.g.
if (config->sata_mode == SATA_MODE_AHCI) { ahci_supported = lpc_dev && !(pci_read_config32(...) & AHCI_UNSUPPORTED); if (!ahci_supported) config->sata_mode = SATA_MODE_IDE_PLAIN; }
switch (config->sata_mode) { /* hw config */ }
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... PS11, Line 64: return;
This still bails out too early. It should skip the `ahci_supported` part […]
The print message needs updating, but if the first function on the PCI device cannot be found it makes little sense to try to configure other ones?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... PS11, Line 64: return;
The print message needs updating, but if the first function on the PCI device cannot be found it mak […]
Yeah, technically we could die() here. Though, even with that information in mind, wouldn't it be nicer if one can read (and follow) the code without having to think about it?
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30822
to look at the new patch set (#12).
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
sb/intel/i82801gx: Detect if the southbridge supports AHCI
This automatically detects whether the southbridge supports AHCI. If AHCI support is selected it will be used unless "sata_no_ahci" is set in the devicetree to override the behavior.
Change-Id: I8d9f4e63ae8b2862c422938f3103c44e761bcda4 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/apple/macbook21/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb M src/mainboard/asus/p5gc-mx/devicetree.cb M src/mainboard/asus/p5qpl-am/devicetree.cb M src/mainboard/foxconn/d41s/devicetree.cb M src/mainboard/foxconn/g41s-k/devicetree.cb M src/mainboard/getac/p470/devicetree.cb M src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb M src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb M src/mainboard/ibase/mb899/devicetree.cb M src/mainboard/intel/d510mo/devicetree.cb M src/mainboard/intel/d945gclf/devicetree.cb M src/mainboard/intel/dg41wv/devicetree.cb M src/mainboard/kontron/986lcd-m/devicetree.cb M src/mainboard/lenovo/t60/devicetree.cb M src/mainboard/lenovo/thinkcentre_a58/devicetree.cb M src/mainboard/lenovo/x60/devicetree.cb M src/mainboard/lenovo/z61t/devicetree.cb M src/mainboard/roda/rk886ex/devicetree.cb M src/southbridge/intel/i82801gx/chip.h M src/southbridge/intel/i82801gx/i82801gx.h M src/southbridge/intel/i82801gx/sata.c 25 files changed, 49 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/30822/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/#/c/30822/12/src/southbridge/intel/i82801gx/sata... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/12/src/southbridge/intel/i82801gx/sata... PS12, Line 63: bridge, it makes little sense to continue. */ code indent should use tabs where possible
https://review.coreboot.org/#/c/30822/12/src/southbridge/intel/i82801gx/sata... PS12, Line 63: bridge, it makes little sense to continue. */ please, no space before tabs
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30822
to look at the new patch set (#13).
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
sb/intel/i82801gx: Detect if the southbridge supports AHCI
This automatically detects whether the southbridge supports AHCI. If AHCI support is selected it will be used unless "sata_no_ahci" is set in the devicetree to override the behavior.
Change-Id: I8d9f4e63ae8b2862c422938f3103c44e761bcda4 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/apple/macbook21/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb M src/mainboard/asus/p5gc-mx/devicetree.cb M src/mainboard/asus/p5qpl-am/devicetree.cb M src/mainboard/foxconn/d41s/devicetree.cb M src/mainboard/foxconn/g41s-k/devicetree.cb M src/mainboard/getac/p470/devicetree.cb M src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb M src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb M src/mainboard/ibase/mb899/devicetree.cb M src/mainboard/intel/d510mo/devicetree.cb M src/mainboard/intel/d945gclf/devicetree.cb M src/mainboard/intel/dg41wv/devicetree.cb M src/mainboard/kontron/986lcd-m/devicetree.cb M src/mainboard/lenovo/t60/devicetree.cb M src/mainboard/lenovo/thinkcentre_a58/devicetree.cb M src/mainboard/lenovo/x60/devicetree.cb M src/mainboard/lenovo/z61t/devicetree.cb M src/mainboard/roda/rk886ex/devicetree.cb M src/southbridge/intel/i82801gx/chip.h M src/southbridge/intel/i82801gx/i82801gx.h M src/southbridge/intel/i82801gx/sata.c 25 files changed, 49 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/30822/13
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 13: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
sb/intel/i82801gx: Detect if the southbridge supports AHCI
This automatically detects whether the southbridge supports AHCI. If AHCI support is selected it will be used unless "sata_no_ahci" is set in the devicetree to override the behavior.
Change-Id: I8d9f4e63ae8b2862c422938f3103c44e761bcda4 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/30822 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/mainboard/apple/macbook21/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb M src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb M src/mainboard/asus/p5gc-mx/devicetree.cb M src/mainboard/asus/p5qpl-am/devicetree.cb M src/mainboard/foxconn/d41s/devicetree.cb M src/mainboard/foxconn/g41s-k/devicetree.cb M src/mainboard/getac/p470/devicetree.cb M src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb M src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb M src/mainboard/ibase/mb899/devicetree.cb M src/mainboard/intel/d510mo/devicetree.cb M src/mainboard/intel/d945gclf/devicetree.cb M src/mainboard/intel/dg41wv/devicetree.cb M src/mainboard/kontron/986lcd-m/devicetree.cb M src/mainboard/lenovo/t60/devicetree.cb M src/mainboard/lenovo/thinkcentre_a58/devicetree.cb M src/mainboard/lenovo/x60/devicetree.cb M src/mainboard/lenovo/z61t/devicetree.cb M src/mainboard/roda/rk886ex/devicetree.cb M src/southbridge/intel/i82801gx/chip.h M src/southbridge/intel/i82801gx/i82801gx.h M src/southbridge/intel/i82801gx/sata.c 25 files changed, 49 insertions(+), 35 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/mainboard/apple/macbook21/devicetree.cb b/src/mainboard/apple/macbook21/devicetree.cb index 9da3891..5ce28a2 100644 --- a/src/mainboard/apple/macbook21/devicetree.cb +++ b/src/mainboard/apple/macbook21/devicetree.cb @@ -64,7 +64,6 @@ register "gpi1_routing" = "2" register "gpi7_routing" = "2"
- register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x04"
register "gpe0_en" = "0x11000006" diff --git a/src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb b/src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb index 156fe3f..acb8ac6 100644 --- a/src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb +++ b/src/mainboard/asrock/g41c-gs/variants/g41c-gs-r2/devicetree.cb @@ -51,7 +51,6 @@ register "gpi13_routing" = "2"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x440"
diff --git a/src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb b/src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb index ba2f00d..f4d1dc4 100644 --- a/src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb +++ b/src/mainboard/asrock/g41c-gs/variants/g41m-gs/devicetree.cb @@ -46,7 +46,6 @@ register "pirqh_routing" = "0x0b"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x440"
diff --git a/src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb b/src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb index 45a2014..2fd6e4f 100644 --- a/src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb +++ b/src/mainboard/asrock/g41c-gs/variants/g41m-s3/devicetree.cb @@ -44,7 +44,6 @@ register "pirqh_routing" = "0x0b"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x440"
diff --git a/src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb b/src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb index b458115..5479faf 100644 --- a/src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb +++ b/src/mainboard/asrock/g41c-gs/variants/g41m-vs3-r2/devicetree.cb @@ -46,7 +46,6 @@ register "pirqh_routing" = "0x0b"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "gpe0_en" = "0x440"
device pci 1b.0 on # Audio diff --git a/src/mainboard/asus/p5gc-mx/devicetree.cb b/src/mainboard/asus/p5gc-mx/devicetree.cb index 2f7d278..de63da2 100644 --- a/src/mainboard/asus/p5gc-mx/devicetree.cb +++ b/src/mainboard/asus/p5gc-mx/devicetree.cb @@ -53,7 +53,6 @@ register "ide_legacy_combined" = "0x0" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0"
register "p_cnt_throttling_supported" = "0"
diff --git a/src/mainboard/asus/p5qpl-am/devicetree.cb b/src/mainboard/asus/p5qpl-am/devicetree.cb index 63ae8ce..bc023d2 100644 --- a/src/mainboard/asus/p5qpl-am/devicetree.cb +++ b/src/mainboard/asus/p5qpl-am/devicetree.cb @@ -43,7 +43,6 @@ # 2 SCI (if corresponding GPIO_EN bit is also set)
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "gpe0_en" = "0x04000440"
device pci 1b.0 on end # Audio diff --git a/src/mainboard/foxconn/d41s/devicetree.cb b/src/mainboard/foxconn/d41s/devicetree.cb index 75df88e..a611ee3 100644 --- a/src/mainboard/foxconn/d41s/devicetree.cb +++ b/src/mainboard/foxconn/d41s/devicetree.cb @@ -40,7 +40,6 @@ register "pirqf_routing" = "0x0b" register "pirqg_routing" = "0x0b" register "pirqh_routing" = "0x0b" - register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x441"
diff --git a/src/mainboard/foxconn/g41s-k/devicetree.cb b/src/mainboard/foxconn/g41s-k/devicetree.cb index ca952ba..b196e24 100644 --- a/src/mainboard/foxconn/g41s-k/devicetree.cb +++ b/src/mainboard/foxconn/g41s-k/devicetree.cb @@ -47,7 +47,6 @@
register "ide_enable_primary" = "0x0" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0" # AHCI does not work register "sata_ports_implemented" = "0x3"
device pci 1b.0 on end # Audio diff --git a/src/mainboard/getac/p470/devicetree.cb b/src/mainboard/getac/p470/devicetree.cb index c0cad6a..c994553 100644 --- a/src/mainboard/getac/p470/devicetree.cb +++ b/src/mainboard/getac/p470/devicetree.cb @@ -57,7 +57,6 @@ register "ide_legacy_combined" = "0x1" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0"
register "c3_latency" = "85" register "docking_supported" = "1" diff --git a/src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb b/src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb index 7ed4d19..1c69613 100644 --- a/src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb +++ b/src/mainboard/gigabyte/ga-945gcm-s2l/devicetree.cb @@ -76,7 +76,6 @@ register "ide_legacy_combined" = "0x0" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0" register "c3_latency" = "85"
register "p_cnt_throttling_supported" = "0" diff --git a/src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb b/src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb index 8b47c4f..d24eb5d 100644 --- a/src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb +++ b/src/mainboard/gigabyte/ga-g41m-es2l/devicetree.cb @@ -48,7 +48,6 @@ register "ide_legacy_combined" = "0x0" # Combined mode broken register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0" # AHCI does not work register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x40"
diff --git a/src/mainboard/ibase/mb899/devicetree.cb b/src/mainboard/ibase/mb899/devicetree.cb index f81a68b..0c5962f 100644 --- a/src/mainboard/ibase/mb899/devicetree.cb +++ b/src/mainboard/ibase/mb899/devicetree.cb @@ -36,7 +36,6 @@ register "ide_legacy_combined" = "0x0" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x1"
register "c3_latency" = "85" register "p_cnt_throttling_supported" = "0" diff --git a/src/mainboard/intel/d510mo/devicetree.cb b/src/mainboard/intel/d510mo/devicetree.cb index a008610..825611e 100644 --- a/src/mainboard/intel/d510mo/devicetree.cb +++ b/src/mainboard/intel/d510mo/devicetree.cb @@ -38,7 +38,6 @@ register "pirqf_routing" = "0x0b" register "pirqg_routing" = "0x0b" register "pirqh_routing" = "0x0b" - register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x3" register "gpe0_en" = "0x20000040"
diff --git a/src/mainboard/intel/d945gclf/devicetree.cb b/src/mainboard/intel/d945gclf/devicetree.cb index 716654c..573b9c8 100644 --- a/src/mainboard/intel/d945gclf/devicetree.cb +++ b/src/mainboard/intel/d945gclf/devicetree.cb @@ -50,7 +50,6 @@ register "ide_legacy_combined" = "0x0" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0" register "c3_latency" = "85" register "p_cnt_throttling_supported" = "0"
diff --git a/src/mainboard/intel/dg41wv/devicetree.cb b/src/mainboard/intel/dg41wv/devicetree.cb index be28763..295fbc4 100644 --- a/src/mainboard/intel/dg41wv/devicetree.cb +++ b/src/mainboard/intel/dg41wv/devicetree.cb @@ -63,7 +63,6 @@ register "gpi15_routing" = "2"
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "gpe0_en" = "0x440"
device pci 1b.0 on # Audio diff --git a/src/mainboard/kontron/986lcd-m/devicetree.cb b/src/mainboard/kontron/986lcd-m/devicetree.cb index 5e00109..cd7929c 100644 --- a/src/mainboard/kontron/986lcd-m/devicetree.cb +++ b/src/mainboard/kontron/986lcd-m/devicetree.cb @@ -36,7 +36,6 @@ register "ide_legacy_combined" = "0x1" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x1" - register "sata_ahci" = "0x0" register "c3_latency" = "85" register "p_cnt_throttling_supported" = "0"
diff --git a/src/mainboard/lenovo/t60/devicetree.cb b/src/mainboard/lenovo/t60/devicetree.cb index b4c8fb8..28e0574 100644 --- a/src/mainboard/lenovo/t60/devicetree.cb +++ b/src/mainboard/lenovo/t60/devicetree.cb @@ -72,7 +72,6 @@ register "gpi12_routing" = "2" register "gpi8_routing" = "2"
- register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x01"
register "gpe0_en" = "0x11000006" diff --git a/src/mainboard/lenovo/thinkcentre_a58/devicetree.cb b/src/mainboard/lenovo/thinkcentre_a58/devicetree.cb index cc3ef49..ace2bfb 100644 --- a/src/mainboard/lenovo/thinkcentre_a58/devicetree.cb +++ b/src/mainboard/lenovo/thinkcentre_a58/devicetree.cb @@ -44,7 +44,6 @@ register "gpi13_routing" = "1" # ??vendor
register "ide_enable_primary" = "0x1" - register "sata_ahci" = "0x0" # AHCI not supported on this ICH7 variant register "gpe0_en" = "0x440"
device pci 1b.0 on end # Audio diff --git a/src/mainboard/lenovo/x60/devicetree.cb b/src/mainboard/lenovo/x60/devicetree.cb index 87cfa84..b3d87cc 100644 --- a/src/mainboard/lenovo/x60/devicetree.cb +++ b/src/mainboard/lenovo/x60/devicetree.cb @@ -65,7 +65,6 @@ register "gpi12_routing" = "1" register "gpi8_routing" = "2"
- register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x01"
register "gpe0_en" = "0x11000006" diff --git a/src/mainboard/lenovo/z61t/devicetree.cb b/src/mainboard/lenovo/z61t/devicetree.cb index 8519b31..d35c62b 100644 --- a/src/mainboard/lenovo/z61t/devicetree.cb +++ b/src/mainboard/lenovo/z61t/devicetree.cb @@ -71,7 +71,6 @@ register "gpi12_routing" = "2" register "gpi8_routing" = "2"
- register "sata_ahci" = "0x1" register "sata_ports_implemented" = "0x01"
register "gpe0_en" = "0x11000006" diff --git a/src/mainboard/roda/rk886ex/devicetree.cb b/src/mainboard/roda/rk886ex/devicetree.cb index 082a9e8..3ba9d2c 100644 --- a/src/mainboard/roda/rk886ex/devicetree.cb +++ b/src/mainboard/roda/rk886ex/devicetree.cb @@ -61,7 +61,6 @@ register "ide_legacy_combined" = "0x1" register "ide_enable_primary" = "0x1" register "ide_enable_secondary" = "0x0" - register "sata_ahci" = "0x0"
device pci 1b.0 off end # High Definition Audio device pci 1c.0 on end # PCIe port 1 diff --git a/src/southbridge/intel/i82801gx/chip.h b/src/southbridge/intel/i82801gx/chip.h index db27ef7..8909f50 100644 --- a/src/southbridge/intel/i82801gx/chip.h +++ b/src/southbridge/intel/i82801gx/chip.h @@ -18,6 +18,12 @@
#include <stdint.h>
+enum sata_mode { + SATA_MODE_AHCI = 0, + SATA_MODE_IDE_LEGACY_COMBINED, + SATA_MODE_IDE_PLAIN, +}; + struct southbridge_intel_i82801gx_config { /** * Interrupt Routing configuration @@ -65,7 +71,7 @@ uint32_t ide_legacy_combined; uint32_t ide_enable_primary; uint32_t ide_enable_secondary; - uint32_t sata_ahci; + enum sata_mode sata_mode; uint32_t sata_ports_implemented;
/* Enable linear PCIe Root Port function numbers starting at zero */ diff --git a/src/southbridge/intel/i82801gx/i82801gx.h b/src/southbridge/intel/i82801gx/i82801gx.h index e44fcf5..76420b4 100644 --- a/src/southbridge/intel/i82801gx/i82801gx.h +++ b/src/southbridge/intel/i82801gx/i82801gx.h @@ -82,6 +82,7 @@
#define FDVCT 0xe4 #define PCIE_4_PORTS_MAX (1 << 7) +#define AHCI_UNSUPPORTED (1 << 3)
/* GEN_PMCON_3 bits */ #define RTC_BATTERY_DEAD (1 << 2) diff --git a/src/southbridge/intel/i82801gx/sata.c b/src/southbridge/intel/i82801gx/sata.c index b657513..24dbf7c 100644 --- a/src/southbridge/intel/i82801gx/sata.c +++ b/src/southbridge/intel/i82801gx/sata.c @@ -51,18 +51,42 @@ void sata_enable(struct device *dev) { /* Get the chip configuration */ - config_t *config = dev->chip_info; + struct southbridge_intel_i82801gx_config *config = dev->chip_info;
- if (config->sata_ahci) { - /* Set map to ahci */ - pci_write_config8(dev, SATA_MAP, - (pci_read_config8(dev, SATA_MAP) & ~0xc3) | 0x40); - } else { - /* Set map to ide */ - pci_write_config8(dev, SATA_MAP, - pci_read_config8(dev, SATA_MAP) & ~0xc3); + if (config->sata_mode == SATA_MODE_AHCI) { + /* Check if the southbridge supports AHCI */ + struct device *lpc_dev = pcidev_on_root(31, 0); + if (!lpc_dev) { + /* According to the PCI spec function 0 on a bus:device + needs to be active for other functions to be enabled. + Since SATA is on the same bus:device as the LPC + bridge, it makes little sense to continue. */ + die("Couldn't find the LPC device!\n"); + } + + const bool ahci_supported = !(pci_read_config32(lpc_dev, FDVCT) + & AHCI_UNSUPPORTED); + + if (!ahci_supported) { + /* Fallback to IDE PLAIN for sata for the rest of the + initialization */ + config->sata_mode = SATA_MODE_IDE_PLAIN; + printk(BIOS_DEBUG, + "AHCI not supported, falling back to plain mode.\n"); + } + }
+ if (config->sata_mode == SATA_MODE_AHCI) { + /* Set map to ahci */ + pci_write_config8(dev, SATA_MAP, + (pci_read_config8(dev, SATA_MAP) + & ~0xc3) | 0x40); + } else { + /* Set map to ide */ + pci_write_config8(dev, SATA_MAP, + pci_read_config8(dev, SATA_MAP) & ~0xc3); + } /* At this point, the new pci id will appear on the bus */ }
@@ -89,7 +113,8 @@ /* Enable BARs */ pci_write_config16(dev, PCI_COMMAND, 0x0007);
- if (config->ide_legacy_combined) { + switch (config->sata_mode) { + case SATA_MODE_IDE_LEGACY_COMBINED: printk(BIOS_DEBUG, "SATA controller in combined mode.\n"); /* No AHCI: clear AHCI base */ pci_write_config32(dev, 0x24, 0x00000000); @@ -120,7 +145,8 @@
/* Restrict ports - 0 and 2 only available */ ports &= 0x5; - } else if (config->sata_ahci) { + break; + case SATA_MODE_AHCI: printk(BIOS_DEBUG, "SATA controller in AHCI mode.\n"); /* Allow both Legacy and Native mode */ pci_write_config8(dev, 0x09, 0x8f); @@ -131,7 +157,9 @@
ahci_bar = (u32 *)(pci_read_config32(dev, 0x27) & ~0x3ff); ahci_bar[3] = config->sata_ports_implemented; - } else { + break; + default: + case SATA_MODE_IDE_PLAIN: printk(BIOS_DEBUG, "SATA controller in plain mode.\n"); /* Set Sata Controller Mode. No Mapping(?) */ pci_write_config8(dev, SATA_MAP, 0x00); @@ -168,6 +196,7 @@ /* Set IDE I/O Configuration */ reg32 = SIG_MODE_PRI_NORMAL | FAST_PCB1 | FAST_PCB0 | PCB1 | PCB0; pci_write_config32(dev, IDE_CONFIG, reg32); + break; }
/* Set port control */
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)