Change in coreboot[master]: sb/intel/common: Add SMBUS register read-modify-write

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38151 )

Change subject: sb/intel/common: Add SMBUS register read-modify-write
......................................................................

sb/intel/common: Add SMBUS register read-modify-write

Change-Id: Ibe967d02fd05f4a8f643a5c5b17885701946d1c7
Signed-off-by: Kyösti Mälkki 
---
M src/southbridge/intel/common/smbus.c
1 file changed, 13 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/38151/1

diff --git a/src/southbridge/intel/common/smbus.c b/src/southbridge/intel/common/smbus.c
index 00ff1dd..262de79 100644
--- a/src/southbridge/intel/common/smbus.c
+++ b/src/southbridge/intel/common/smbus.c
@@ -82,6 +82,15 @@
    return inb(base + reg);
 }

+static void host_update(unsigned int base, u8 reg, u8 mask, u8 or)
+{
+	u8 value;
+	value = host_inb(base, reg);
+	value &= mask;
+	value |= or;
+	host_outb(base, reg, value);
+}
+
 static int host_completed(u8 status)
 {
    if (status & SMBHSTSTS_HOST_BUSY)
@@ -129,7 +138,7 @@
    			      SMBUS_WAIT_UNTIL_READY_TIMEOUT);

/* Clear any lingering errors, so the transaction will run. */
-	host_outb(smbus_base, SMBHSTSTAT, host_inb(smbus_base, SMBHSTSTAT));
+	host_update(smbus_base, SMBHSTSTAT, 0xff, 0);

/* Set up transaction */
    /* Disable interrupts */
@@ -147,7 +156,7 @@
    u8 status;

/* Start the command. */
-	host_outb(smbus_base, SMBHSTCTL, host_inb(smbus_base, SMBHSTCTL) | SMBHSTCNT_START);
+	host_update(smbus_base, SMBHSTCTL, 0xff, SMBHSTCNT_START);

/* Poll for it to start. */
    do {
@@ -298,9 +307,8 @@

/* Indicate that next byte is the last one. */
    			if (sw_drives_nak && (bytes + 1 >= max_bytes)) {
-					host_outb(smbus_base, SMBHSTCTL,
-						host_inb(smbus_base, SMBHSTCTL) |
-							SMBHSTCNT_LAST_BYTE);
+					host_update(smbus_base, SMBHSTCTL,
+						    0xff, SMBHSTCNT_LAST_BYTE);
    			}

}

[Paul Menzel (Code Review)]

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

Change subject: sb/intel/common: Add SMBUS register read-modify-write
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/38151/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38151/1//COMMIT_MSG@7 
PS1, Line 7: SMBUS
SMBus

http://www.smbus.org/

[Angel Pons (Code Review)]

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

Change subject: sb/intel/common: Add SMBUS register read-modify-write
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

I'm not sure if the current usecases are enough to warrant this new function. Is this used later on?

https://review.coreboot.org/c/coreboot/+/38151/2/src/southbridge/intel/commo... 
File src/southbridge/intel/common/smbus.c:

https://review.coreboot.org/c/coreboot/+/38151/2/src/southbridge/intel/commo... 
PS2, Line 85: host_update
MCHBAR macros with a similar purpose call this operation `_and_or`. I like it because the name describes the order of the applied operations

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins),

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

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

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

Change subject: sb/intel/common: Add SMBUS register read-modify-write
......................................................................

sb/intel/common: Add SMBUS register read-modify-write

Change-Id: Ibe967d02fd05f4a8f643a5c5b17885701946d1c7
Signed-off-by: Kyösti Mälkki 
---
M src/southbridge/intel/common/smbus.c
1 file changed, 13 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/38151/3

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38151 )

Change subject: sb/intel/common: Add SMBUS register read-modify-write
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38151/2/src/southbridge/intel/commo... 
File src/southbridge/intel/common/smbus.c:

https://review.coreboot.org/c/coreboot/+/38151/2/src/southbridge/intel/commo... 
PS2, Line 85: host_update
> MCHBAR macros with a similar purpose call this operation `_and_or`. […]

Done

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38151 )

Change subject: sb/intel/common: Add SMBUS register read-modify-write
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38151/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38151/1//COMMIT_MSG@7 
PS1, Line 7: SMBUS
> SMBus […]

Ack

[Angel Pons (Code Review)]

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

Change subject: sb/intel/common: Add SMBUS register read-modify-write
......................................................................

Patch Set 3: Code-Review+2

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38151 )

Change subject: sb/intel/common: Add SMBUS register read-modify-write
......................................................................

sb/intel/common: Add SMBUS register read-modify-write

Change-Id: Ibe967d02fd05f4a8f643a5c5b17885701946d1c7
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38151
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/southbridge/intel/common/smbus.c
1 file changed, 13 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/southbridge/intel/common/smbus.c b/src/southbridge/intel/common/smbus.c
index 00ff1dd..30d0cd0 100644
--- a/src/southbridge/intel/common/smbus.c
+++ b/src/southbridge/intel/common/smbus.c
@@ -82,6 +82,15 @@
    return inb(base + reg);
 }

+static void host_and_or(unsigned int base, u8 reg, u8 mask, u8 or)
+{
+	u8 value;
+	value = host_inb(base, reg);
+	value &= mask;
+	value |= or;
+	host_outb(base, reg, value);
+}
+
 static int host_completed(u8 status)
 {
    if (status & SMBHSTSTS_HOST_BUSY)
@@ -129,7 +138,7 @@
    			      SMBUS_WAIT_UNTIL_READY_TIMEOUT);

/* Clear any lingering errors, so the transaction will run. */
-	host_outb(smbus_base, SMBHSTSTAT, host_inb(smbus_base, SMBHSTSTAT));
+	host_and_or(smbus_base, SMBHSTSTAT, 0xff, 0);

/* Set up transaction */
    /* Disable interrupts */
@@ -147,7 +156,7 @@
    u8 status;

/* Start the command. */
-	host_outb(smbus_base, SMBHSTCTL, host_inb(smbus_base, SMBHSTCTL) | SMBHSTCNT_START);
+	host_and_or(smbus_base, SMBHSTCTL, 0xff, SMBHSTCNT_START);

/* Poll for it to start. */
    do {
@@ -298,9 +307,8 @@

/* Indicate that next byte is the last one. */
    			if (sw_drives_nak && (bytes + 1 >= max_bytes)) {
-					host_outb(smbus_base, SMBHSTCTL,
-						host_inb(smbus_base, SMBHSTCTL) |
-							SMBHSTCNT_LAST_BYTE);
+					host_and_or(smbus_base, SMBHSTCTL,
+						    0xff, SMBHSTCNT_LAST_BYTE);
    			}

}