Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
sb/intel/common: Add AHCI code
Implement a clean implementation based on AHCI spec 1.0-1.3 and the Intel ICH/PCH datasheets.
The common implementation should combine all features of the duplicated code present in the various southbridge implementations.
It compiles on x86_64, uses less magic values and detects the AHCI version at runtime.
Change-Id: I5714788aa74b2d8bd855cbc65dd3d78c0a0101b8 Signed-off-by: Patrick Rudolph siro@das-labor.org --- A src/include/ahci.h M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/Makefile.inc A src/southbridge/intel/common/ahci.c A src/southbridge/intel/common/ahci.h 5 files changed, 252 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/37137/1
diff --git a/src/include/ahci.h b/src/include/ahci.h new file mode 100644 index 0000000..e7818fb --- /dev/null +++ b/src/include/ahci.h @@ -0,0 +1,74 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Patrick Rudolph siro@das-labor.org + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __AHCI_H__ +#define __AHCI_H__ + +/* Serial ATA AHCI 1.3.1 Specification */ + +#define AHCI_CAP 0x00 +#define AHCI_CAP_NP 0x1f +#define AHCI_CAP_PSC (1u << 13) +#define AHCI_CAP_SSC (1u << 14) +#define AHCI_CAP_PMD (1u << 15) +#define AHCI_CAP_SPM (1u << 17) +#define AHCI_CAP_SAM (1u << 18) +#define AHCI_CAP_SNZO (1u << 19) +#define AHCI_CAP_ISS(x) ((x) << 20) +#define AHCI_CAP_SCLO (1u << 24) +#define AHCI_CAP_SAL (1u << 25) +#define AHCI_CAP_SALP (1u << 26) +#define AHCI_CAP_SSS (1u << 27) +#define AHCI_CAP_SIS (1u << 28) +#define AHCI_CAP_SNCQ (1u << 30) +#define AHCI_CAP_S64A (1u << 31) +/* Added in AHCI 1.1 */ +#define AHCI_CAP_SXS (1u << 5) +#define AHCI_CAP_EMS (1u << 6) +#define AHCI_CAP_CCCS (1u << 7) +/* Added in AHCI 1.2 */ +#define AHCI_CAP_SSNTF (1u << 29) + +#define AHCI_GHC 0x04 +#define AHCI_GHC_AE (1u << 31) +#define AHCI_PI 0x0c +#define AHCI_VS 0x10 +#define AHCI_VS_MAJOR(x) ((x >> 16) & 0xf) +#define AHCI_VS_MINOR(x) ((x >> 8) & 0xf) + +/* Added in AHCI 1.1 */ +#define AHCI_CCC_CTL 0x14 +#define AHCI_CCC_PORTS 0x18 +#define AHCI_EM_LOC 0x1c +#define AHCI_EM_CTL 0x20 +/* Added in AHCI 1.2 */ +#define AHCI_CAP2 0x24 +#define AHCI_CAP2_BOH (1u << 0) +/* Added in AHCI 1.3 */ +#define AHCI_CAP2_NVMP (1u << 1) +#define AHCI_CAP2_APST (1u << 2) +#define AHCI_CAP2_SDS (1u << 3) +#define AHCI_CAP2_SADM (1u << 4) +#define AHCI_CAP2_DESO (1u << 5) + +#define AHCI_BOHC 0x28 + +#define AHCI_VENDOR 0xa0 +#define AHCI_PORT 0x100 +#define AHCI_PORT_PxCMD 0x18 +#define AHCI_PORT_LEN 0x80 + + +#endif diff --git a/src/southbridge/intel/common/Kconfig b/src/southbridge/intel/common/Kconfig index 18bcd2e..376f077 100644 --- a/src/southbridge/intel/common/Kconfig +++ b/src/southbridge/intel/common/Kconfig @@ -2,6 +2,10 @@ def_bool n select HAVE_CF9_RESET
+config SOUTHBRIDGE_INTEL_COMMON_AHCI + def_bool n + depends on PCI + config SOUTHBRIDGE_INTEL_COMMON_RTC def_bool n
diff --git a/src/southbridge/intel/common/Makefile.inc b/src/southbridge/intel/common/Makefile.inc index c8521e1..e87ae82 100644 --- a/src/southbridge/intel/common/Makefile.inc +++ b/src/southbridge/intel/common/Makefile.inc @@ -18,6 +18,8 @@
all-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_RESET) += reset.c
+ramstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_AHCI) += ahci.c + romstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_SMBUS) += smbus.c ramstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_SMBUS) += smbus.c
diff --git a/src/southbridge/intel/common/ahci.c b/src/southbridge/intel/common/ahci.c new file mode 100644 index 0000000..ca20d30 --- /dev/null +++ b/src/southbridge/intel/common/ahci.c @@ -0,0 +1,142 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Patrick Rudolph siro@das-labor.org + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <ahci.h> +#include <arch/io.h> +#include <device/device.h> +#include <device/pci.h> +#include <console/console.h> + +#include "ahci.h" + +/** + * Initialize the AHCI config space. + * @param port_map The ports to enable + * @param sam Set SAM bit (no legacy device support) + * @param cccs Set CCCS bit + * @param iss Limit the controller to sata gen x + * @param devslp Enable DEVSLP feature if supported + * @param clear_vnd Clear bits in vendor config space + */ +void sb_ahci_init(struct device *const dev, + const u32 port_map, + const bool sam, + const bool cccs, + const u8 iss, + const bool devslp, + const u32 clear_vnd) +{ + u32 reg32; + u8 major, minor; + struct resource *res; + u8 *abar; + unsigned int ports; + + /* Initialize AHCI memory-mapped space */ + res = find_resource(dev, PCI_BASE_ADDRESS_5); + if (!res) + return; + + abar = res2mmio(res, 0, 0); + /* + * Set AHCI access mode. + * Spec: "No other ABAR registers should be accessed before this." + */ + reg32 = read32(abar + AHCI_GHC); + if (!(reg32 & AHCI_GHC_AE)) { + reg32 |= AHCI_GHC_AE; + write32(abar + AHCI_GHC, reg32); + } + + /* Get AHCI version */ + reg32 = read32(abar + AHCI_VS); + major = AHCI_VS_MAJOR(reg32); + minor = AHCI_VS_MINOR(reg32); + printk(BIOS_DEBUG, "AHCI: implements AHCI %d.%d\n", major, minor); + + reg32 = read32(abar + AHCI_CAP); + printk(BIOS_DEBUG, "AHCI: supports SATA Gen%d\n", (reg32 >> 20) & 0xf); + + ports = reg32 & AHCI_CAP_NP; + printk(BIOS_DEBUG, "AHCI: supports up to %d ports\n", ports); + + /* CAP (HBA Capabilities) : enable power management */ + /* Program Write-Once bits (this isn't part of the AHCI spec, but PCH). */ + if (sam) + reg32 |= AHCI_CAP_SAM; + if (cccs && major >= 1 && minor >= 1) + reg32 |= AHCI_CAP_CCCS; + if (iss) { + u8 max_iss; + if (major >= 1 && minor >= 2) + max_iss = MIN(iss, 3); + else + max_iss = MIN(iss, 2); + reg32 &= ~AHCI_CAP_ISS(0xf); + reg32 |= AHCI_CAP_ISS(max_iss); + printk(BIOS_DEBUG, "AHCI: limited to Gen%d\n", max_iss); + } + reg32 |= AHCI_CAP_PSC; + reg32 |= AHCI_CAP_SSC; + reg32 |= AHCI_CAP_SALP; + reg32 |= AHCI_CAP_SSS; + if (major >= 1 && minor >= 1) { + reg32 &= ~AHCI_CAP_SXS; + reg32 &= ~AHCI_CAP_EMS; + reg32 &= ~AHCI_CAP_SPM; + } + write32(abar + AHCI_CAP, reg32); + + if (major >= 1 && minor >= 2) { + reg32 = read32(abar + AHCI_CAP2); + /* Clear BIOS handoff support */ + reg32 &= ~AHCI_CAP2_BOH; + if (major >= 1 && minor >= 3) { + /* Configure DEVSLP support */ + if (devslp) { + reg32 |= AHCI_CAP2_APST; + reg32 |= AHCI_CAP2_SDS; + reg32 |= AHCI_CAP2_SADM; + reg32 |= AHCI_CAP2_DESO; + } else { + reg32 &= ~AHCI_CAP2_SDS; + } + /* Disable NVMHCI */ + reg32 &= ~AHCI_CAP2_NVMP; + } + write32(abar + AHCI_CAP2, reg32); + } + + if (port_map & ~((1 << ports) - 1)) + printk(BIOS_ERR, "AHCI: Invalid port map given!\n"); + + write32(abar + AHCI_PI, port_map & ((1 << ports) - 1)); + /* PCH code reads back twice, do we need it, too? */ + (void) read32(abar + AHCI_PI); /* Read back 1 */ + (void) read32(abar + AHCI_PI); /* Read back 2 */ + + /* Clear vendor specific bits */ + reg32 = read32(abar + AHCI_VENDOR); + reg32 &= ~clear_vnd; + write32(abar + AHCI_VENDOR, reg32); + + /* Lock R/WO bits in Port command registers. */ + for (size_t i = 0; i < ports; ++i) { + if (!((1 << i) & port_map)) + continue; + u8 *addr = abar + AHCI_PORT + (i * AHCI_PORT_LEN); + write32(addr, read32(addr + AHCI_PORT_PxCMD)); + } +} diff --git a/src/southbridge/intel/common/ahci.h b/src/southbridge/intel/common/ahci.h new file mode 100644 index 0000000..9d4a025 --- /dev/null +++ b/src/southbridge/intel/common/ahci.h @@ -0,0 +1,30 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Patrick Rudolph siro@das-labor.org + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef SOUTHBRIDGE_INTEL_COMMON_AHCI_H +#define SOUTHBRIDGE_INTEL_COMMON_AHCI_H + +#include <device/device.h> + +void sb_ahci_init(struct device *const dev, + const u32 port_map, + const bool sam, + const bool cccs, + const u8 iss, + const bool devslp, + const u32 clear_vnd); + +#endif /* SOUTHBRIDGE_INTEL_COMMON_AHCI_H */ +
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37137
to look at the new patch set (#2).
Change subject: sb/intel/common: Add AHCI code ......................................................................
sb/intel/common: Add AHCI code
Implement a clean implementation based on AHCI spec 1.0-1.3 and the Intel ICH/PCH datasheets.
The common implementation should combine all features of the duplicated code present in the various southbridge implementations.
It compiles on x86_64, uses less magic values and detects the AHCI version at runtime.
Change-Id: I5714788aa74b2d8bd855cbc65dd3d78c0a0101b8 Signed-off-by: Patrick Rudolph siro@das-labor.org --- A src/include/ahci.h M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/Makefile.inc A src/southbridge/intel/common/ahci.c A src/southbridge/intel/common/ahci.h 5 files changed, 251 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/37137/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 2:
(12 comments)
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h File src/include/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@22 PS2, Line 22: #define AHCI_CAP_NP 0x1f please name it _MASK or something similar
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@40 PS2, Line 40: odd whitespace
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@48 PS2, Line 48: #define AHCI_VS_MAJOR(x) ((x >> 16) & 0xf) ((((x) >> 24) & 0xf) * 10 + (((x) >> 16) & 0xf))
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@49 PS2, Line 49: #define AHCI_VS_MINOR(x) ((x >> 8) & 0xf) ((x) & 0xf ? ((((x) >> 8) & 0xf) * 10 + ((x) & 0xf)) : (((x) >> 8) & 0xf))
At least this seems how they write it, no trailing 0. OTOH, this makes it harder for comparisons, so maybe we should just print 1.10 as the hardware reports instead of 1.1.
(Intel seems to love to make it confusing. I guess 0.95 should have been 0.9.5, then their counting would make sense.)
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 27: const u32 clear_vnd); Qualifications of the parameter type like the `const` here are meaningless in an interface. For the caller, it's just noise. (Exception are pointers to qualified types ofc., where the qualification becomes part of the pointer type.)
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 55: * Spec: "No other ABAR registers should be accessed before this." I can't find this quote?
Spec talks about software and I'm sure it doesn't apply to Intel firmware, rather AHCI drivers. It also says that AE depends on SAM which is only configured later.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 67: BIOS_DEBUG I think BIOS_INFO would suit most of the messages better.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 73: ports + 1
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 75: : enable power management it's more than that
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 79: if (cccs && major >= 1 && minor >= 1) || major > 1
Versions are easier to compare in an encoded form. For instance with
#define AHCI_VERSION (mj, mn) ((mj) << 16 | (mn)))
this could become
if (read32(abar + AHCI_VS) >= AHCI_VERSION(1, 10))
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 80: reg32 |= AHCI_CAP_CCCS; Note, this wasn't explicitly set for bd82x6x.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 102: if (major >= 1 && minor >= 2) { Hmmm, it seems ICH9 advertises 1.20, but doesn't document this register.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h File src/include/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@49 PS2, Line 49: #define AHCI_VS_MINOR(x) ((x >> 8) & 0xf)
((x) & 0xf ? ((((x) >> 8) & 0xf) * 10 + ((x) & 0xf)) : (((x) >> 8) & 0xf)) […]
when you compare chapter 3.1.5.6 and 3.1.5.1 it's clear that 0.95 is actually 0.9.5 as 1.31 is actually 1.3.1
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 55: * Spec: "No other ABAR registers should be accessed before this."
I can't find this quote? […]
it's in AHCI chapter 3.1.2. yes that might be true.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 80: reg32 |= AHCI_CAP_CCCS;
Note, this wasn't explicitly set for bd82x6x.
yes, not sure if that was by accident or requirement.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 102: if (major >= 1 && minor >= 2) {
Hmmm, it seems ICH9 advertises 1.20, but doesn't document this register.
yes, seems like documentation is wrong. Let's hope hardware is not.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37137
to look at the new patch set (#3).
Change subject: sb/intel/common: Add AHCI code ......................................................................
sb/intel/common: Add AHCI code
Implement a clean implementation based on AHCI spec 1.0-1.3 and the Intel ICH/PCH datasheets.
The common implementation should combine all features of the duplicated code present in the various southbridge implementations.
It compiles on x86_64, uses less magic values and detects the AHCI version at runtime.
Change-Id: I5714788aa74b2d8bd855cbc65dd3d78c0a0101b8 Signed-off-by: Patrick Rudolph siro@das-labor.org --- A src/include/ahci.h M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/Makefile.inc A src/southbridge/intel/common/ahci.c A src/southbridge/intel/common/ahci.h 5 files changed, 254 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/37137/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37137/3/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
https://review.coreboot.org/c/coreboot/+/37137/3/src/southbridge/intel/commo... PS3, Line 17: #include <arch/io.h> unused.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h File src/include/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@22 PS2, Line 22: #define AHCI_CAP_NP 0x1f
please name it _MASK or something similar
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@40 PS2, Line 40:
odd whitespace
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@48 PS2, Line 48: #define AHCI_VS_MAJOR(x) ((x >> 16) & 0xf)
((((x) >> 24) & 0xf) * 10 + (((x) >> 16) & 0xf))
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@49 PS2, Line 49: #define AHCI_VS_MINOR(x) ((x >> 8) & 0xf)
when you compare chapter 3.1.5.6 and 3.1.5.1 it's clear that 0.95 is actually 0.9.5 as 1. […]
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 67: BIOS_DEBUG
I think BIOS_INFO would suit most of the messages better.
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 73: ports
- 1
What I meant here is that the register value is 0-based, hence we need `ports + 1` here.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 75: : enable power management
it's more than that
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 79: if (cccs && major >= 1 && minor >= 1)
|| major > 1 […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 3:
I really would like to see this submitted/merged. Arthur’s comment needs to be addressed.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 3: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/37137/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37137/3//COMMIT_MSG@15 PS3, Line 15: version Put this on the next line? It's over 72 chars
https://review.coreboot.org/c/coreboot/+/37137/3/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
PS3: SPDX please
Marcello Sylvester Bauer has uploaded a new patch set (#4) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
sb/intel/common: Add AHCI code
Implement a clean implementation based on AHCI spec 1.0-1.3 and the Intel ICH/PCH datasheets.
The common implementation should combine all features of the duplicated code present in the various southbridge implementations.
It compiles on x86_64, uses less magic values and detects the AHCI version at runtime.
Change-Id: I5714788aa74b2d8bd855cbc65dd3d78c0a0101b8 Signed-off-by: Patrick Rudolph siro@das-labor.org --- A src/include/ahci.h M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/Makefile.inc A src/southbridge/intel/common/ahci.c A src/southbridge/intel/common/ahci.h 5 files changed, 246 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/37137/4
Marcello Sylvester Bauer has uploaded a new patch set (#5) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
sb/intel/common: Add AHCI code
Implement a clean implementation based on AHCI spec 1.0-1.3 and the Intel ICH/PCH datasheets.
The common implementation should combine all features of the duplicated code present in the various southbridge implementations.
It compiles on x86_64, uses less magic values and detects the AHCI version at runtime.
Change-Id: I5714788aa74b2d8bd855cbc65dd3d78c0a0101b8 Signed-off-by: Patrick Rudolph siro@das-labor.org --- A src/include/ahci.h M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/Makefile.inc A src/southbridge/intel/common/ahci.c A src/southbridge/intel/common/ahci.h 5 files changed, 246 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/37137/5
Marcello Sylvester Bauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/37137/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37137/3//COMMIT_MSG@15 PS3, Line 15: version
Put this on the next line? It's over 72 chars
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 27: const u32 clear_vnd);
Qualifications of the parameter type like the `const` here are meaningless in […]
Ack
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 73: ports
What I meant here is that the register value is 0-based, hence we need […]
Ack
https://review.coreboot.org/c/coreboot/+/37137/3/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
PS3:
SPDX please
Done
https://review.coreboot.org/c/coreboot/+/37137/3/src/southbridge/intel/commo... PS3, Line 17: #include <arch/io.h>
unused.
Done
Masanori Ogino has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 6:
(8 comments)
Great work to reduce duplicates!
I have some minor comments, not affecting main functionalities.
https://review.coreboot.org/c/coreboot/+/37137/6/src/include/ahci.h File src/include/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/6/src/include/ahci.h@1 PS6, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2019 Patrick Rudolph siro@das-labor.org : * : * This program is free software; you can redistribute it and/or : * modify it under the terms of the GNU General Public License as : * published by the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ You can replace this copyright and license information with:
/* SPDX-License-Identifier: GPL-2.0-only */
as per recent treewide changes.
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2019 Patrick Rudolph siro@das-labor.org : * : * This program is free software; you can redistribute it and/or : * modify it under the terms of the GNU General Public License as : * published by the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ /* SPDX-License-Identifier: GPL-2.0-only */
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2019 Patrick Rudolph siro@das-labor.org : * : * SPDX-License-Identifier: GPL-2.0-or-later : */ /* SPDX-License-Identifier: GPL-2.0-or-later */
(By the way, is it intentional to publish headers in GPLv2 only while this file in GPLv2 or later?)
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 21: * @param iss Limit the controller to sata gen x s/sata gen x/SATA Gen x/
It might be better if this states that `iss == 0` means no limit.
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 62: printk(BIOS_INFO, "AHCI: supports SATA Gen%d\n", (reg32 >> 20) & 0xf); Nits: a space between "Gen" and "%d"?
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 81: printk(BIOS_INFO, "AHCI: limited to Gen%d\n", max_iss); Nits: a space between "Gen" and "%d"?
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 115: printk(BIOS_ERR, "AHCI: Invalid port map given!\n"); It might be better if this message contains the port map (in hexadecimal).
https://review.coreboot.org/c/coreboot/+/37137/6/src/southbridge/intel/commo... PS6, Line 123: reg32 = read32(abar + AHCI_VENDOR); : reg32 &= ~clear_vnd; : write32(abar + AHCI_VENDOR, reg32); Not sure but you could save 1 read + 1 write by surrounding this block with `if (clear_vnd) { ... }`. Please ignore this comment if it is needed even if clear_vnd is zero.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37137?usp=email )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Marcello Sylvester Bauer (Code Review)
-
Martin L Roth (Code Review)
-
Masanori Ogino (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)