Change in coreboot[master]: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode

[Stephen Douthit (Code Review)]

Stephen Douthit has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode

SATA Mode Select is bit 16 of the SATA General Configuration
register.  This code currently incorrectly pokes at the Port Clock
Disable bits in the Port Mapping Register, and without clock the
affected ports can't link.

Change-Id: I37104f520a869bd45a32cfd271d0b893aec3c8ed
Signed-off-by: Stephen Douthit 
---
M src/soc/intel/denverton_ns/sata.c
1 file changed, 3 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/34663/1

diff --git a/src/soc/intel/denverton_ns/sata.c b/src/soc/intel/denverton_ns/sata.c
index ddb8b02..610a4c6 100644
--- a/src/soc/intel/denverton_ns/sata.c
+++ b/src/soc/intel/denverton_ns/sata.c
@@ -31,7 +31,6 @@
 static void sata_init(struct device *dev)
 {
    u32 reg32;
-	u16 reg16;
    u32 abar;

printk(BIOS_DEBUG, "SATA: Initializing...\n");
@@ -46,10 +45,9 @@
    printk(BIOS_DEBUG, "SATA: Controller in AHCI mode.\n");

/* Set the controller mode */
-	reg16 = pci_read_config16(dev, SATA_MAP);
-	reg16 &= ~(3 << 6);
-	reg16 |= SATA_MAP_AHCI;
-	pci_write_config16(dev, SATA_MAP, reg16);
+	reg32 = pci_read_config16(dev, SATAGC);
+	reg32 &= ~SATAGC_AHCI;
+	pci_write_config16(dev, SATAGC, reg32);

/* Initialize AHCI memory-mapped space */
    abar = pci_read_config32(dev, PCI_BASE_ADDRESS_5);

[Stephen Douthit (Code Review)]

Hello Patrick Rudolph, Vanny E, build bot (Jenkins), David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34663

to look at the new patch set (#2).

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode

SATA Mode Select is bit 16 of the SATA General Configuration
register.  This code currently incorrectly pokes at the Port Clock
Disable bits in the Port Mapping Register, and without clock the
affected ports can't link.

Change-Id: I37104f520a869bd45a32cfd271d0b893aec3c8ed
Signed-off-by: Stephen Douthit 
---
M src/soc/intel/denverton_ns/include/soc/sata.h
M src/soc/intel/denverton_ns/sata.c
2 files changed, 6 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/34663/2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

Patch Set 2: Code-Review+1

Nice find. I wonder how this is found out only now.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34663/2/src/soc/intel/denverton_ns/... 
File src/soc/intel/denverton_ns/sata.c:

https://review.coreboot.org/c/coreboot/+/34663/2/src/soc/intel/denverton_ns/... 
PS2, Line 34: 
Any reason to drop this?

[Stephen Douthit (Code Review)]

Stephen Douthit has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+1
>
> (1 comment)

It's now unused.

I do see that I failed to change the width of the config accesses from 16->32, so that needs to be fixed.

[Stephen Douthit (Code Review)]

Stephen Douthit has uploaded a new patch set (#3) to the change originally created by Stephen Douthit. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode

SATA Mode Select is bit 16 of the SATA General Configuration
register.  This code currently incorrectly pokes at the Port Clock
Disable bits in the Port Mapping Register, and without clock the
affected ports can't link.

Change-Id: I37104f520a869bd45a32cfd271d0b893aec3c8ed
Signed-off-by: Stephen Douthit 
---
M src/soc/intel/denverton_ns/include/soc/sata.h
M src/soc/intel/denverton_ns/sata.c
2 files changed, 6 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/34663/3

[Vanny E (Code Review)]

Vanny E has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

Patch Set 3: Code-Review+1

[David Guckian (Code Review)]

David Guckian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

Patch Set 3: Code-Review+1

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34663/2/src/soc/intel/denverton_ns/... 
File src/soc/intel/denverton_ns/sata.c:

https://review.coreboot.org/c/coreboot/+/34663/2/src/soc/intel/denverton_ns/... 
PS2, Line 34: u16 reg16;
> Any reason to drop this?

Done

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/34663 )

Change subject: soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode
......................................................................

soc/intel/dnv: Don't clobber SATA_MAP while trying to set mode

SATA Mode Select is bit 16 of the SATA General Configuration
register.  This code currently incorrectly pokes at the Port Clock
Disable bits in the Port Mapping Register, and without clock the
affected ports can't link.

Change-Id: I37104f520a869bd45a32cfd271d0b893aec3c8ed
Signed-off-by: Stephen Douthit 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34663
Tested-by: build bot (Jenkins) 
Reviewed-by: Vanessa Eusebio 
Reviewed-by: David Guckian
Reviewed-by: Patrick Georgi 
---
M src/soc/intel/denverton_ns/include/soc/sata.h
M src/soc/intel/denverton_ns/sata.c
2 files changed, 6 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Vanessa Eusebio: Looks good to me, but someone else must approve
  David Guckian: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/denverton_ns/include/soc/sata.h b/src/soc/intel/denverton_ns/include/soc/sata.h
index afa39b5..f38b539 100644
--- a/src/soc/intel/denverton_ns/include/soc/sata.h
+++ b/src/soc/intel/denverton_ns/include/soc/sata.h
@@ -23,9 +23,8 @@
 #define PCH_SATA0_DEV PCI_DEV(0, SATA_DEV, SATA_FUNC)
 #define PCH_SATA1_DEV PCI_DEV(0, SATA2_DEV, SATA2_FUNC)

-#define SATA_MAP 0x90
-#define SATA_MAP_AHCI (0 << 6)
-#define SATA_MAP_RAID (1 << 6)
-#define SATA_PSC 0x92
+#define SATAGC 0x9c
+#define SATAGC_AHCI (0 << 16)
+#define SATAGC_RAID (1 << 16)

#endif //_DENVERTON_NS_SATA_H
diff --git a/src/soc/intel/denverton_ns/sata.c b/src/soc/intel/denverton_ns/sata.c
index d53d553..891d95f 100644
--- a/src/soc/intel/denverton_ns/sata.c
+++ b/src/soc/intel/denverton_ns/sata.c
@@ -31,7 +31,6 @@
 static void sata_init(struct device *dev)
 {
    u32 reg32;
-	u16 reg16;
    u32 abar;

printk(BIOS_DEBUG, "SATA: Initializing...\n");
@@ -46,10 +45,9 @@
    printk(BIOS_DEBUG, "SATA: Controller in AHCI mode.\n");

/* Set the controller mode */
-	reg16 = pci_read_config16(dev, SATA_MAP);
-	reg16 &= ~(3 << 6);
-	reg16 |= SATA_MAP_AHCI;
-	pci_write_config16(dev, SATA_MAP, reg16);
+	reg32 = pci_read_config32(dev, SATAGC);
+	reg32 &= ~SATAGC_AHCI;
+	pci_write_config32(dev, SATAGC, reg32);

/* Initialize AHCI memory-mapped space */
    abar = pci_read_config32(dev, PCI_BASE_ADDRESS_5);