Change in coreboot[master]: sb/intel/lynxpoint/sata.c: Simplify RMW operations

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47030 )

Change subject: sb/intel/lynxpoint/sata.c: Simplify RMW operations
......................................................................

sb/intel/lynxpoint/sata.c: Simplify RMW operations

Introduce the `sir_unset_and_set_mask` helper and update the one PCI
read-modify-write operation that is somehow not reproducible.

Change-Id: I30ad6ef8ad97ee0a8dc2297fba5bbbfe24f00f1c
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/sata.c
1 file changed, 13 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47030/1

diff --git a/src/southbridge/intel/lynxpoint/sata.c b/src/southbridge/intel/lynxpoint/sata.c
index d0aaa99..4174e1f 100644
--- a/src/southbridge/intel/lynxpoint/sata.c
+++ b/src/southbridge/intel/lynxpoint/sata.c
@@ -25,10 +25,17 @@
    pci_write_config32(dev, SATA_SIRD, value);
 }

+static inline void sir_unset_and_set_mask(struct device *dev, int idx, u32 unset, u32 set)
+{
+	pci_write_config32(dev, SATA_SIRI, idx);
+
+	const u32 value = pci_read_config32(dev, SATA_SIRD) & ~unset;
+	pci_write_config32(dev, SATA_SIRD, value | set);
+}
+
 static void sata_init(struct device *dev)
 {
    u32 reg32;
-	u16 reg16;

u32 *abar;

@@ -70,10 +77,7 @@
    pci_write_config32(dev, IDE_CONFIG, reg32);

/* for AHCI, Port Enable is managed in memory mapped space */
-	reg16 = pci_read_config16(dev, 0x92);
-	reg16 &= ~0x3f;
-	reg16 |= 0x8000 | config->sata_port_map;
-	pci_write_config16(dev, 0x92, reg16);
+	pci_update_config16(dev, 0x92, ~0x3f, 0x8000 | config->sata_port_map);
    udelay(2);

/* Setup register 98h */
@@ -174,25 +178,16 @@
    	sir_write(dev, 0x64, 0x883c9001);

/* Step 2: SIR 68h[15:0] = 880Ah */
-	reg32 = sir_read(dev, 0x68);
-	reg32 &= 0xffff0000;
-	reg32 |= 0x880a;
-	sir_write(dev, 0x68, reg32);
+	sir_unset_and_set_mask(dev, 0x68, 0xffff, 0x880a);

/* Step 3: SIR 60h[3] = 1 */
-	reg32 = sir_read(dev, 0x60);
-	reg32 |= (1 << 3);
-	sir_write(dev, 0x60, reg32);
+	sir_unset_and_set_mask(dev, 0x60, 0, 1 << 3);

/* Step 4: SIR 60h[0] = 1 */
-	reg32 = sir_read(dev, 0x60);
-	reg32 |= (1 << 0);
-	sir_write(dev, 0x60, reg32);
+	sir_unset_and_set_mask(dev, 0x60, 0, 1 << 0);

/* Step 5: SIR 60h[1] = 1 */
-	reg32 = sir_read(dev, 0x60);
-	reg32 |= (1 << 1);
-	sir_write(dev, 0x60, reg32);
+	sir_unset_and_set_mask(dev, 0x60, 0, 1 << 1);

/* Clock Gating */
    sir_write(dev, 0x70, 0x3f00bf1f);

[Angel Pons (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: sb/intel/lynxpoint/sata.c: Simplify RMW operations
......................................................................

sb/intel/lynxpoint/sata.c: Simplify RMW operations

Introduce the `sir_unset_and_set_mask` helper and update the one PCI
read-modify-write operation that is somehow not reproducible.

Change-Id: I30ad6ef8ad97ee0a8dc2297fba5bbbfe24f00f1c
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/sata.c
1 file changed, 13 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47030/2

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47030 )

Change subject: sb/intel/lynxpoint/sata.c: Simplify RMW operations
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/47030/2/src/southbridge/intel/lynxp... 
File src/southbridge/intel/lynxpoint/sata.c:

https://review.coreboot.org/c/coreboot/+/47030/2/src/southbridge/intel/lynxp... 
PS2, Line 16: static inline u32 sir_read(struct device *dev, int idx)
            : {
            : 	pci_write_config32(dev, SATA_SIRI, idx);
            : 	return pci_read_config32(dev, SATA_SIRD);
            : }
            : 
            : static inline void sir_write(struct device *dev, int idx, u32 value)
            : {
            : 	pci_write_config32(dev, SATA_SIRI, idx);
            : 	pci_write_config32(dev, SATA_SIRD, value);
            : }
            : 
            : static inline void sir_unset_and_set_mask(struct device *dev, int idx, u32 unset, u32 set)
this is not platform specific -> move to common code

[Angel Pons (Code Review)]

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

Change subject: sb/intel/lynxpoint/sata.c: Simplify RMW operations
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47030/2/src/southbridge/intel/lynxp... 
File src/southbridge/intel/lynxpoint/sata.c:

https://review.coreboot.org/c/coreboot/+/47030/2/src/southbridge/intel/lynxp... 
PS2, Line 16: static inline u32 sir_read(struct device *dev, int idx)
            : {
            : 	pci_write_config32(dev, SATA_SIRI, idx);
            : 	return pci_read_config32(dev, SATA_SIRD);
            : }
            : 
            : static inline void sir_write(struct device *dev, int idx, u32 value)
            : {
            : 	pci_write_config32(dev, SATA_SIRI, idx);
            : 	pci_write_config32(dev, SATA_SIRD, value);
            : }
            : 
            : static inline void sir_unset_and_set_mask(struct device *dev, int idx, u32 unset, u32 set)
> this is not platform specific -> move to common code

These registers are platform-specific, but seem to match across most platforms. Reasoning this is out of the scope of this change.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47030 )

Change subject: sb/intel/lynxpoint/sata.c: Simplify RMW operations
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47030/2/src/southbridge/intel/lynxp... 
File src/southbridge/intel/lynxpoint/sata.c:

https://review.coreboot.org/c/coreboot/+/47030/2/src/southbridge/intel/lynxp... 
PS2, Line 16: static inline u32 sir_read(struct device *dev, int idx)
            : {
            : 	pci_write_config32(dev, SATA_SIRI, idx);
            : 	return pci_read_config32(dev, SATA_SIRD);
            : }
            : 
            : static inline void sir_write(struct device *dev, int idx, u32 value)
            : {
            : 	pci_write_config32(dev, SATA_SIRI, idx);
            : 	pci_write_config32(dev, SATA_SIRD, value);
            : }
            : 
            : static inline void sir_unset_and_set_mask(struct device *dev, int idx, u32 unset, u32 set)
> These registers are platform-specific, but seem to match across most platforms. […]

The registers may be platform specific but the SIR mechanism and these helpers are not. thus, it should go to common code later

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47030 )

Change subject: sb/intel/lynxpoint/sata.c: Simplify RMW operations
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/47030/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47030/4//COMMIT_MSG@7 
PS4, Line 7: .c
Nit, `.c` seems excessive.

[Angel Pons (Code Review)]

Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/47030 )

Change subject: sb/intel/lynxpoint/sata.c: Simplify RMW operations
......................................................................

Removed Verified+1 by build bot (Jenkins)

Change in coreboot[master]: sb/intel/lynxpoint/sata: Simplify RMW operations

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Nico Huber, Michael Niewöhner, Patrick Rudolph,

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

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

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

Change subject: sb/intel/lynxpoint/sata: Simplify RMW operations
......................................................................

sb/intel/lynxpoint/sata: Simplify RMW operations

Introduce the `sir_unset_and_set_mask` helper and update the one PCI
read-modify-write operation that is somehow not reproducible.

Change-Id: I30ad6ef8ad97ee0a8dc2297fba5bbbfe24f00f1c
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/sata.c
1 file changed, 13 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47030/6

Change in coreboot[master]: sb/intel/lynxpoint/sata: Simplify RMW operations

[Angel Pons (Code Review)]

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

Change subject: sb/intel/lynxpoint/sata: Simplify RMW operations
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47030/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47030/4//COMMIT_MSG@7 
PS4, Line 7: .c
> Nit, `.c` seems excessive.

Done

Change in coreboot[master]: sb/intel/lynxpoint/sata: Simplify RMW operations

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47030 )

Change subject: sb/intel/lynxpoint/sata: Simplify RMW operations
......................................................................

sb/intel/lynxpoint/sata: Simplify RMW operations

Introduce the `sir_unset_and_set_mask` helper and update the one PCI
read-modify-write operation that is somehow not reproducible.

Change-Id: I30ad6ef8ad97ee0a8dc2297fba5bbbfe24f00f1c
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47030
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
Reviewed-by: Michael Niewöhner 
---
M src/southbridge/intel/lynxpoint/sata.c
1 file changed, 13 insertions(+), 18 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Michael Niewöhner: Looks good to me, but someone else must approve

diff --git a/src/southbridge/intel/lynxpoint/sata.c b/src/southbridge/intel/lynxpoint/sata.c
index c9c7f2c..d8eb2a8 100644
--- a/src/southbridge/intel/lynxpoint/sata.c
+++ b/src/southbridge/intel/lynxpoint/sata.c
@@ -25,10 +25,17 @@
    pci_write_config32(dev, SATA_SIRD, value);
 }

+static inline void sir_unset_and_set_mask(struct device *dev, int idx, u32 unset, u32 set)
+{
+	pci_write_config32(dev, SATA_SIRI, idx);
+
+	const u32 value = pci_read_config32(dev, SATA_SIRD) & ~unset;
+	pci_write_config32(dev, SATA_SIRD, value | set);
+}
+
 static void sata_init(struct device *dev)
 {
    u32 reg32;
-	u16 reg16;

u32 *abar;

@@ -69,10 +76,7 @@
    pci_write_config32(dev, IDE_CONFIG, reg32);

/* for AHCI, Port Enable is managed in memory mapped space */
-	reg16 = pci_read_config16(dev, 0x92);
-	reg16 &= ~0x3f;
-	reg16 |= 0x8000 | config->sata_port_map;
-	pci_write_config16(dev, 0x92, reg16);
+	pci_update_config16(dev, 0x92, ~0x3f, 0x8000 | config->sata_port_map);
    udelay(2);

/* Setup register 98h */
@@ -173,25 +177,16 @@
    	sir_write(dev, 0x64, 0x883c9001);

/* Step 2: SIR 68h[15:0] = 880Ah */
-	reg32 = sir_read(dev, 0x68);
-	reg32 &= 0xffff0000;
-	reg32 |= 0x880a;
-	sir_write(dev, 0x68, reg32);
+	sir_unset_and_set_mask(dev, 0x68, 0xffff, 0x880a);

/* Step 3: SIR 60h[3] = 1 */
-	reg32 = sir_read(dev, 0x60);
-	reg32 |= (1 << 3);
-	sir_write(dev, 0x60, reg32);
+	sir_unset_and_set_mask(dev, 0x60, 0, 1 << 3);

/* Step 4: SIR 60h[0] = 1 */
-	reg32 = sir_read(dev, 0x60);
-	reg32 |= (1 << 0);
-	sir_write(dev, 0x60, reg32);
+	sir_unset_and_set_mask(dev, 0x60, 0, 1 << 0);

/* Step 5: SIR 60h[1] = 1 */
-	reg32 = sir_read(dev, 0x60);
-	reg32 |= (1 << 1);
-	sir_write(dev, 0x60, reg32);
+	sir_unset_and_set_mask(dev, 0x60, 0, 1 << 1);

/* Clock Gating */
    sir_write(dev, 0x70, 0x3f00bf1f);