Hello Richard Spiegel, Martin Roth, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32660
to review the following change.
Change subject: soc/amd/stoneyridge: Split sata functionality ......................................................................
soc/amd/stoneyridge: Split sata functionality
Separate chipset-specific source from sata_init(), and modify it to better match coreboot conventions. A subsequent patch will move the generic portion to soc/amd/common.
The support for enabling port multipliers appears to have been first added for Kabini. Although missing from the documentation, the ability to affect the HBA Capabilities Register seems to remain for Stoney Ridge.
Change-Id: I5dd9f613d36badc3e4d185a22b4475cb82ce187e Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/stoneyridge/include/soc/southbridge.h M src/soc/amd/stoneyridge/sata.c 2 files changed, 28 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32660/1
diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h index 99e5eef..334d281 100644 --- a/src/soc/amd/stoneyridge/include/soc/southbridge.h +++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h @@ -250,6 +250,13 @@ /* ISA Bridge SPI BASE at D14F3xA0 */ #define SPI_BASE_ALIGNMENT 32
+/* SATA Controller D11F0 */ +#define SATA_MISC_CONTROL_REG 0x40 +#define SATA_MISC_SUBCLASS_WREN BIT(0) +/* Register in AHCIBaseAddress (BAR5 at D11F0x24) */ +#define SATA_CAPABILITIES_REG 0xfc +#define SATA_CAPABILITY_SPM BIT(12) + /* SPI Controller (base address in D14F3xA0) */ #define SPI_CNTRL0 0x00 #define SPI_BUSY BIT(31) diff --git a/src/soc/amd/stoneyridge/sata.c b/src/soc/amd/stoneyridge/sata.c index 52932e2..f0dabe0 100644 --- a/src/soc/amd/stoneyridge/sata.c +++ b/src/soc/amd/stoneyridge/sata.c @@ -17,40 +17,40 @@ #include <device/pci.h> #include <device/pci_ids.h> #include <device/pci_ops.h> +#include <device/pci_def.h> #include <soc/southbridge.h>
- -static void sata_init(struct device *dev) +static void soc_enable_sata_features(struct device *dev) { - /************************************** - * Configure the SATA port multiplier * - **************************************/ - #define BYTE_TO_DWORD_OFFSET(x) (x/4) - #define AHCI_BASE_ADDRESS_REG 0x24 - #define MISC_CONTROL_REG 0x40 - #define UNLOCK_BIT (1<<0) - #define SATA_CAPABILITIES_REG 0xfc - #define CFG_CAP_SPM (1<<12) + u8 *ahci_ptr; + u32 misc_ctl, cap_cfg;
- u32 *ahci_ptr = (void *)(uintptr_t)ALIGN_DOWN( - pci_read_config32(dev, AHCI_BASE_ADDRESS_REG), 256); u32 temp;
/* unlock the write-protect */ - temp = pci_read_config32(dev, MISC_CONTROL_REG); - temp |= UNLOCK_BIT; - pci_write_config32(dev, MISC_CONTROL_REG, temp); + misc_ctl = pci_read_config32(dev, SATA_MISC_CONTROL_REG); + misc_ctl |= SATA_MISC_SUBCLASS_WREN; + pci_write_config32(dev, SATA_MISC_CONTROL_REG, misc_ctl);
/* set the SATA AHCI mode to allow port expanders */ - *(ahci_ptr + BYTE_TO_DWORD_OFFSET(SATA_CAPABILITIES_REG)) - |= CFG_CAP_SPM; + ahci_ptr = (u8 *)(uintptr_t)ALIGN_DOWN( + pci_read_config32(dev, PCI_BASE_ADDRESS_5), 256); + + cap_cfg = read32(ahci_ptr + SATA_CAPABILITIES_REG); + cap_cfg |= SATA_CAPABILITY_SPM; + write32(ahci_ptr + SATA_CAPABILITIES_REG, cap_cfg);
/* lock the write-protect */ - temp = pci_read_config32(dev, MISC_CONTROL_REG); - temp &= ~UNLOCK_BIT; - pci_write_config32(dev, MISC_CONTROL_REG, temp); + temp = pci_read_config32(dev, SATA_MISC_CONTROL_REG); + temp &= ~SATA_MISC_SUBCLASS_WREN; + pci_write_config32(dev, SATA_MISC_CONTROL_REG, temp); };
+static void sata_init(struct device *dev) +{ + soc_enable_sata_features(dev); +} + static struct pci_operations lops_pci = { /* .set_subsystem = pci_dev_set_subsystem, */ };
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32660 )
Change subject: soc/amd/stoneyridge: Split sata functionality ......................................................................
Patch Set 1: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32660 )
Change subject: soc/amd/stoneyridge: Split sata functionality ......................................................................
Patch Set 4: Code-Review+2
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32660 )
Change subject: soc/amd/stoneyridge: Split sata functionality ......................................................................
Patch Set 5: Code-Review+2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32660 )
Change subject: soc/amd/stoneyridge: Split sata functionality ......................................................................
Patch Set 6: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32660 )
Change subject: soc/amd/stoneyridge: Split sata functionality ......................................................................
soc/amd/stoneyridge: Split sata functionality
Separate chipset-specific source from sata_init(), and modify it to better match coreboot conventions. A subsequent patch will move the generic portion to soc/amd/common.
The support for enabling port multipliers appears to have been first added for Kabini. Although missing from the documentation, the ability to affect the HBA Capabilities Register seems to remain for Stoney Ridge.
Change-Id: I5dd9f613d36badc3e4d185a22b4475cb82ce187e Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32660 Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Richard Spiegel richard.spiegel@silverbackltd.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/stoneyridge/include/soc/southbridge.h M src/soc/amd/stoneyridge/sata.c 2 files changed, 28 insertions(+), 21 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Richard Spiegel: Looks good to me, approved
diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h index 88e0225..ad40407 100644 --- a/src/soc/amd/stoneyridge/include/soc/southbridge.h +++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h @@ -247,6 +247,13 @@ #define GPE0_LIMIT 28 #define TOTAL_BITS(a) (8 * sizeof(a))
+/* SATA Controller D11F0 */ +#define SATA_MISC_CONTROL_REG 0x40 +#define SATA_MISC_SUBCLASS_WREN BIT(0) +/* Register in AHCIBaseAddress (BAR5 at D11F0x24) */ +#define SATA_CAPABILITIES_REG 0xfc +#define SATA_CAPABILITY_SPM BIT(12) + /* SPI Controller (base address in D14F3xA0) */ #define SPI_BASE_ALIGNMENT BIT(6)
diff --git a/src/soc/amd/stoneyridge/sata.c b/src/soc/amd/stoneyridge/sata.c index 52932e2..f0dabe0 100644 --- a/src/soc/amd/stoneyridge/sata.c +++ b/src/soc/amd/stoneyridge/sata.c @@ -17,40 +17,40 @@ #include <device/pci.h> #include <device/pci_ids.h> #include <device/pci_ops.h> +#include <device/pci_def.h> #include <soc/southbridge.h>
- -static void sata_init(struct device *dev) +static void soc_enable_sata_features(struct device *dev) { - /************************************** - * Configure the SATA port multiplier * - **************************************/ - #define BYTE_TO_DWORD_OFFSET(x) (x/4) - #define AHCI_BASE_ADDRESS_REG 0x24 - #define MISC_CONTROL_REG 0x40 - #define UNLOCK_BIT (1<<0) - #define SATA_CAPABILITIES_REG 0xfc - #define CFG_CAP_SPM (1<<12) + u8 *ahci_ptr; + u32 misc_ctl, cap_cfg;
- u32 *ahci_ptr = (void *)(uintptr_t)ALIGN_DOWN( - pci_read_config32(dev, AHCI_BASE_ADDRESS_REG), 256); u32 temp;
/* unlock the write-protect */ - temp = pci_read_config32(dev, MISC_CONTROL_REG); - temp |= UNLOCK_BIT; - pci_write_config32(dev, MISC_CONTROL_REG, temp); + misc_ctl = pci_read_config32(dev, SATA_MISC_CONTROL_REG); + misc_ctl |= SATA_MISC_SUBCLASS_WREN; + pci_write_config32(dev, SATA_MISC_CONTROL_REG, misc_ctl);
/* set the SATA AHCI mode to allow port expanders */ - *(ahci_ptr + BYTE_TO_DWORD_OFFSET(SATA_CAPABILITIES_REG)) - |= CFG_CAP_SPM; + ahci_ptr = (u8 *)(uintptr_t)ALIGN_DOWN( + pci_read_config32(dev, PCI_BASE_ADDRESS_5), 256); + + cap_cfg = read32(ahci_ptr + SATA_CAPABILITIES_REG); + cap_cfg |= SATA_CAPABILITY_SPM; + write32(ahci_ptr + SATA_CAPABILITIES_REG, cap_cfg);
/* lock the write-protect */ - temp = pci_read_config32(dev, MISC_CONTROL_REG); - temp &= ~UNLOCK_BIT; - pci_write_config32(dev, MISC_CONTROL_REG, temp); + temp = pci_read_config32(dev, SATA_MISC_CONTROL_REG); + temp &= ~SATA_MISC_SUBCLASS_WREN; + pci_write_config32(dev, SATA_MISC_CONTROL_REG, temp); };
+static void sata_init(struct device *dev) +{ + soc_enable_sata_features(dev); +} + static struct pci_operations lops_pci = { /* .set_subsystem = pci_dev_set_subsystem, */ };
-
Furquan Shaikh (Code Review) -
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Richard Spiegel (Code Review)