Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47095 )
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
sb/intel/lynxpoint: Use common AHCI library
This avoids confusing and potentially dangerous pointer arithmetics. Behaviour before after this patch should be equivalent, too.
Change-Id: I62a26d74fe78dc0158b383da9126c56746722c53 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/lynxpoint/sata.c 1 file changed, 39 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47095/1
diff --git a/src/southbridge/intel/lynxpoint/sata.c b/src/southbridge/intel/lynxpoint/sata.c index d8eb2a8..e4aa083 100644 --- a/src/southbridge/intel/lynxpoint/sata.c +++ b/src/southbridge/intel/lynxpoint/sata.c @@ -7,6 +7,7 @@ #include <device/pci.h> #include <device/pci_ids.h> #include <delay.h> +#include <southbridge/intel/common/ahci.h> #include "chip.h" #include "iobp.h" #include "pch.h" @@ -37,8 +38,6 @@ { u32 reg32;
- u32 *abar; - /* Get the chip configuration */ config_t *config = dev->chip_info;
@@ -51,7 +50,7 @@
/* SATA configuration */
- /* Enable memory space decoding for ABAR */ + /* Enable memory space decoding for AHCI BAR */ pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MEMORY | PCI_COMMAND_IO);
printk(BIOS_DEBUG, "SATA: Controller in AHCI mode.\n"); @@ -107,31 +106,47 @@ pci_write_config32(dev, 0x94, reg32);
/* Initialize AHCI memory-mapped space */ - abar = (u32 *)pci_read_config32(dev, PCI_BASE_ADDRESS_5); - printk(BIOS_DEBUG, "ABAR: %p\n", abar); - /* CAP (HBA Capabilities) : enable power management */ - reg32 = read32(abar + 0x00); - reg32 |= 0x0c006000; // set PSC+SSC+SALP+SSS - reg32 &= ~0x00020060; // clear SXS+EMS+PMS + const uintptr_t ahci_bar = pci_read_config32(dev, PCI_BASE_ADDRESS_5); + printk(BIOS_DEBUG, "AHCI BAR: %p\n", (void *)ahci_bar); + + union ahci_reg_cap ahci_cap = { + .raw = ahci_read32(ahci_bar, AHCI_REG_CAP), + }; + + ahci_cap.external_sata = 0; /* Should be configurable */ + ahci_cap.enclosure_management = 0; + ahci_cap.port_multiplier = 0; + + ahci_cap.partial_state = 1; + ahci_cap.slumber_state = 1; + ahci_cap.aggressive_link_pm = 1; + ahci_cap.staggered_spinup = 1; + if (pch_is_lp()) - reg32 |= (1 << 18); // SAM: SATA AHCI MODE ONLY - write32(abar + 0x00, reg32); - /* PI (Ports implemented) */ - write32(abar + 0x03, config->sata_port_map); - (void)read32(abar + 0x03); /* Read back 1 */ - (void)read32(abar + 0x03); /* Read back 2 */ - /* CAP2 (HBA Capabilities Extended)*/ - reg32 = read32(abar + 0x09); - /* Enable DEVSLP */ + ahci_cap.ahci_mode_only = 1; + + ahci_write32(ahci_bar, AHCI_REG_CAP, ahci_cap.raw); + + ahci_write_ports_implemented(ahci_bar, config->sata_port_map); + + union ahci_reg_cap_2 ahci_cap_2 = { + .raw = ahci_read32(ahci_bar, AHCI_REG_CAP_2), + }; + if (pch_is_lp()) { - if (config->sata_devslp_disable) - reg32 &= ~(1 << 3); - else - reg32 |= (1 << 5)|(1 << 4)|(1 << 3)|(1 << 2); + if (config->sata_devslp_disable) { + ahci_cap_2.supports_device_sleep = 0; + } else { + ahci_cap_2.auto_partial_to_slumber = 1; + ahci_cap_2.supports_device_sleep = 1; + ahci_cap_2.aggressive_devslp_mgmt = 1; + ahci_cap_2.devslp_entry_slumber_only = 1; + } } else { - reg32 &= ~0x00000002; + ahci_cap_2.nvm_hci_present = 0; } - write32(abar + 0x09, reg32); + + ahci_write32(ahci_bar, AHCI_REG_CAP_2, ahci_cap_2.raw);
/* Set Gen3 Transmitter settings if needed */ if (config->sata_port0_gen3_tx)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47095 )
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
Patch Set 2: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/sata.c:
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... PS2, Line 116: /* Should be configurable */ is that a TODO? then mark it as such, please
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... PS2, Line 136: if (pch_is_lp()) { is devsleep really only supported on lp? if H supports it, this should be dropped (in a separate change ofc)
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... PS2, Line 141: ahci_cap_2.supports_device_sleep = 1; nit: move one line up
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47095 )
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/sata.c:
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... PS2, Line 116: /* Should be configurable */
is that a TODO? then mark it as such, please
I don't plan on doing it, though.
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... PS2, Line 136: if (pch_is_lp()) {
is devsleep really only supported on lp? if H supports it, this should be dropped (in a separate cha […]
No idea. I don't care about such things in refactoring changes (the intention is to preserve behavior)
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... PS2, Line 141: ahci_cap_2.supports_device_sleep = 1;
nit: move one line up
why?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47095 )
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/sata.c:
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... PS2, Line 116: /* Should be configurable */
I don't plan on doing it, though.
clarification: I'll update the comment, but I don't think I'll implement this in the foreseeable future (I'd rather deduplicate the code first).
Hello build bot (Jenkins), Michael Niewöhner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47095
to look at the new patch set (#3).
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
sb/intel/lynxpoint: Use common AHCI library
This avoids confusing and potentially dangerous pointer arithmetics. Behaviour before after this patch should be equivalent, too.
Change-Id: I62a26d74fe78dc0158b383da9126c56746722c53 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/lynxpoint/sata.c 1 file changed, 39 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47095/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47095 )
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/sata.c:
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... PS2, Line 116: /* Should be configurable */
clarification: I'll update the comment, but I don't think I'll implement this in the foreseeable fut […]
Done
https://review.coreboot.org/c/coreboot/+/47095/2/src/southbridge/intel/lynxp... PS2, Line 141: ahci_cap_2.supports_device_sleep = 1;
why?
FWIW, the order logic here is the ordering in the bitfield definition
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47095 )
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47095/4/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/sata.c:
https://review.coreboot.org/c/coreboot/+/47095/4/src/southbridge/intel/lynxp... PS4, Line 97: const uintptr_t ahci_bar = pci_read_config32(dev, PCI_BASE_ADDRESS_5); this should use probe_resource instead
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47095 )
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47095/4/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/sata.c:
https://review.coreboot.org/c/coreboot/+/47095/4/src/southbridge/intel/lynxp... PS4, Line 97: const uintptr_t ahci_bar = pci_read_config32(dev, PCI_BASE_ADDRESS_5);
this should use probe_resource instead
Yeah, I'm going to make a follow-up and update all platforms at once
Attention is currently required from: Angel Pons. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47095 )
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
Patch Set 4:
(1 comment)
File src/southbridge/intel/lynxpoint/sata.c:
https://review.coreboot.org/c/coreboot/+/47095/comment/5abfc788_6af608e2 PS4, Line 97: const uintptr_t ahci_bar = pci_read_config32(dev, PCI_BASE_ADDRESS_5);
Yeah, I'm going to make a follow-up and update all platforms at once
Ack
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/47095?usp=email )
Change subject: sb/intel/lynxpoint: Use common AHCI library ......................................................................
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) -
Martin L Roth (Code Review)
-
Michael Niewöhner (Code Review)
-
Patrick Rudolph (Code Review)