Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38150 )
Change subject: sb/intel/common: Replace inb/outb() ......................................................................
sb/intel/common: Replace inb/outb()
At least since ICH10 SMBUS controller implements register bank access via both IO and MMIO, we may want to change for the latter.
Change-Id: I67fcbc7b6f6be61c93bc608e556a577ef9e52325 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/intel/common/smbus.c 1 file changed, 41 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38150/1
diff --git a/src/southbridge/intel/common/smbus.c b/src/southbridge/intel/common/smbus.c index 9546258..00ff1dd 100644 --- a/src/southbridge/intel/common/smbus.c +++ b/src/southbridge/intel/common/smbus.c @@ -72,6 +72,16 @@ inb(0x80); }
+static void host_outb(unsigned int base, u8 reg, u8 value) +{ + outb(value, base + reg); +} + +static u8 host_inb(unsigned int base, u8 reg) +{ + return inb(base + reg); +} + static int host_completed(u8 status) { if (status & SMBHSTSTS_HOST_BUSY) @@ -111,7 +121,7 @@
do { smbus_delay(); - host_busy = inb(smbus_base + SMBHSTSTAT) & SMBHSTSTS_HOST_BUSY; + host_busy = host_inb(smbus_base, SMBHSTSTAT) & SMBHSTSTS_HOST_BUSY; } while (--loops && host_busy);
if (loops == 0) @@ -119,14 +129,14 @@ SMBUS_WAIT_UNTIL_READY_TIMEOUT);
/* Clear any lingering errors, so the transaction will run. */ - outb(inb(smbus_base + SMBHSTSTAT), smbus_base + SMBHSTSTAT); + host_outb(smbus_base, SMBHSTSTAT, host_inb(smbus_base, SMBHSTSTAT));
/* Set up transaction */ /* Disable interrupts */ - outb(ctrl, (smbus_base + SMBHSTCTL)); + host_outb(smbus_base, SMBHSTCTL, ctrl);
/* Set the device I'm talking to. */ - outb(xmitadd, smbus_base + SMBXMITADD); + host_outb(smbus_base, SMBXMITADD, xmitadd);
return 0; } @@ -137,8 +147,7 @@ u8 status;
/* Start the command. */ - outb((inb(smbus_base + SMBHSTCTL) | SMBHSTCNT_START), - smbus_base + SMBHSTCTL); + host_outb(smbus_base, SMBHSTCTL, host_inb(smbus_base, SMBHSTCTL) | SMBHSTCNT_START);
/* Poll for it to start. */ do { @@ -147,7 +156,7 @@ /* If we poll too slow, we could miss HOST_BUSY flag * set and detect INTR or x_ERR flags instead here. */ - status = inb(smbus_base + SMBHSTSTAT); + status = host_inb(smbus_base, SMBHSTSTAT); status &= ~(SMBHSTSTS_SMBALERT_STS | SMBHSTSTS_INUSE_STS); } while (--loops && status == 0);
@@ -165,7 +174,7 @@
do { smbus_delay(); - status = inb(smbus_base + SMBHSTSTAT); + status = host_inb(smbus_base, SMBHSTSTAT); } while (--loops && !host_completed(status));
if (loops == 0) @@ -187,11 +196,11 @@ return ret;
/* Set the command/address... */ - outb(address, smbus_base + SMBHSTCMD); + host_outb(smbus_base, SMBHSTCMD, address);
/* Clear the data bytes... */ - outb(0, smbus_base + SMBHSTDAT0); - outb(0, smbus_base + SMBHSTDAT1); + host_outb(smbus_base, SMBHSTDAT0, 0); + host_outb(smbus_base, SMBHSTDAT1, 0);
/* Start the command */ ret = execute_command(smbus_base); @@ -204,9 +213,9 @@ return ret;
/* Read results of transaction */ - word = inb(smbus_base + SMBHSTDAT0); + word = host_inb(smbus_base, SMBHSTDAT0); if (ctrl == I801_WORD_DATA) - word |= inb(smbus_base + SMBHSTDAT1) << 8; + word |= host_inb(smbus_base, SMBHSTDAT1) << 8;
return word; } @@ -222,12 +231,12 @@ return ret;
/* Set the command/address... */ - outb(address, smbus_base + SMBHSTCMD); + host_outb(smbus_base, SMBHSTCMD, address);
/* Set the data bytes... */ - outb(data & 0xff, smbus_base + SMBHSTDAT0); + host_outb(smbus_base, SMBHSTDAT0, data & 0xff); if (ctrl == I801_WORD_DATA) - outb(data >> 8, smbus_base + SMBHSTDAT1); + host_outb(smbus_base, SMBHSTDAT1, data >> 8);
/* Start the command */ ret = execute_command(smbus_base); @@ -256,16 +265,16 @@ * was really updated with the transaction. */ if (!sw_drives_nak) { if (is_write_cmd) - outb(max_bytes, smbus_base + SMBHSTDAT0); + host_outb(smbus_base, SMBHSTDAT0, max_bytes); else - outb(0, smbus_base + SMBHSTDAT0); + host_outb(smbus_base, SMBHSTDAT0, 0); }
/* Send first byte from buffer, bytes_sent increments after * hardware acknowledges it. */ if (is_write_cmd) - outb(*buf++, smbus_base + SMBBLKDAT); + host_outb(smbus_base, SMBBLKDAT, *buf++);
/* Start the command */ ret = execute_command(smbus_base); @@ -274,24 +283,24 @@
/* Poll for transaction completion */ do { - status = inb(smbus_base + SMBHSTSTAT); + status = host_inb(smbus_base, SMBHSTSTAT);
if (status & SMBHSTSTS_BYTE_DONE) { /* Byte done */
if (is_write_cmd) { bytes++; if (bytes < max_bytes) - outb(*buf++, smbus_base + SMBBLKDAT); + host_outb(smbus_base, SMBBLKDAT, *buf++); } else { if (bytes < max_bytes) - *buf++ = inb(smbus_base + SMBBLKDAT); + *buf++ = host_inb(smbus_base, SMBBLKDAT); bytes++;
/* Indicate that next byte is the last one. */ if (sw_drives_nak && (bytes + 1 >= max_bytes)) { - outb(inb(smbus_base + SMBHSTCTL) - | SMBHSTCNT_LAST_BYTE, - smbus_base + SMBHSTCTL); + host_outb(smbus_base, SMBHSTCTL, + host_inb(smbus_base, SMBHSTCTL) | + SMBHSTCNT_LAST_BYTE); }
} @@ -300,7 +309,7 @@ * and clears HOST_BUSY flag once the byte count * has been reached or LAST_BYTE was set. */ - outb(SMBHSTSTS_BYTE_DONE, smbus_base + SMBHSTSTAT); + host_outb(smbus_base, SMBHSTSTAT, SMBHSTSTS_BYTE_DONE); }
} while (--loops && !host_completed(status)); @@ -354,7 +363,7 @@ return ret;
/* Set the command/address... */ - outb(cmd, smbus_base + SMBHSTCMD); + host_outb(smbus_base, SMBHSTCMD, cmd);
/* Execute block transaction. */ ret = block_cmd_loop(smbus_base, buf, max_bytes, BLOCK_READ); @@ -362,7 +371,7 @@ return ret;
/* Post-check we received complete message. */ - slave_bytes = inb(smbus_base + SMBHSTDAT0); + slave_bytes = host_inb(smbus_base, SMBHSTDAT0); if (ret < slave_bytes) return SMBUS_ERROR;
@@ -383,7 +392,7 @@ return ret;
/* Set the command/address... */ - outb(cmd, smbus_base + SMBHSTCMD); + host_outb(smbus_base, SMBHSTCMD, cmd);
/* Execute block transaction. */ ret = block_cmd_loop(smbus_base, (u8 *)buf, bytes, BLOCK_WRITE); @@ -425,7 +434,7 @@ return ret;
/* device offset */ - outb(offset, smbus_base + SMBHSTDAT1); + host_outb(smbus_base, SMBHSTDAT1, offset);
/* Execute block transaction. */ ret = block_cmd_loop(smbus_base, buf, bytes, BLOCK_READ | BLOCK_I2C); @@ -468,8 +477,8 @@ */ cmd = *buf++; bytes--; - outb(cmd, smbus_base + SMBHSTCMD); - outb(cmd, smbus_base + SMBHSTDAT1); + host_outb(smbus_base, SMBHSTCMD, cmd); + host_outb(smbus_base, SMBHSTDAT1, cmd);
/* Execute block transaction. */ ret = block_cmd_loop(smbus_base, buf, bytes, BLOCK_WRITE);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38150 )
Change subject: sb/intel/common: Replace inb/outb() ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38150 )
Change subject: sb/intel/common: Replace inb/outb() ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38150/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38150/2//COMMIT_MSG@9 PS2, Line 9: At least since ICH10 SMBUS controller implements register I'm not sure if I follow. I would rephrase this as:
sb/intel/common/smbus.c: Wrap inb/outb()
On Intel, accessing the SMBus register banks can be done via IO and, since at least ICH10, via MMIO. We may want to use the latter in the future.
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/+/38150
to look at the new patch set (#3).
Change subject: sb/intel/common: Wrap inb/outb() ......................................................................
sb/intel/common: Wrap inb/outb()
On Intel, accessing the SMBus register banks can be done via IO and, since at least ICH10, via MMIO. We may want to use the latter in the future.
Change-Id: I67fcbc7b6f6be61c93bc608e556a577ef9e52325 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/intel/common/smbus.c 1 file changed, 41 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38150/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38150 )
Change subject: sb/intel/common: Wrap inb/outb() ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38150/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38150/2//COMMIT_MSG@9 PS2, Line 9: At least since ICH10 SMBUS controller implements register
I'm not sure if I follow. I would rephrase this as: […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38150 )
Change subject: sb/intel/common: Wrap inb/outb() ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38150 )
Change subject: sb/intel/common: Wrap inb/outb() ......................................................................
sb/intel/common: Wrap inb/outb()
On Intel, accessing the SMBus register banks can be done via IO and, since at least ICH10, via MMIO. We may want to use the latter in the future.
Change-Id: I67fcbc7b6f6be61c93bc608e556a577ef9e52325 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38150 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/common/smbus.c 1 file changed, 41 insertions(+), 32 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 9546258..00ff1dd 100644 --- a/src/southbridge/intel/common/smbus.c +++ b/src/southbridge/intel/common/smbus.c @@ -72,6 +72,16 @@ inb(0x80); }
+static void host_outb(unsigned int base, u8 reg, u8 value) +{ + outb(value, base + reg); +} + +static u8 host_inb(unsigned int base, u8 reg) +{ + return inb(base + reg); +} + static int host_completed(u8 status) { if (status & SMBHSTSTS_HOST_BUSY) @@ -111,7 +121,7 @@
do { smbus_delay(); - host_busy = inb(smbus_base + SMBHSTSTAT) & SMBHSTSTS_HOST_BUSY; + host_busy = host_inb(smbus_base, SMBHSTSTAT) & SMBHSTSTS_HOST_BUSY; } while (--loops && host_busy);
if (loops == 0) @@ -119,14 +129,14 @@ SMBUS_WAIT_UNTIL_READY_TIMEOUT);
/* Clear any lingering errors, so the transaction will run. */ - outb(inb(smbus_base + SMBHSTSTAT), smbus_base + SMBHSTSTAT); + host_outb(smbus_base, SMBHSTSTAT, host_inb(smbus_base, SMBHSTSTAT));
/* Set up transaction */ /* Disable interrupts */ - outb(ctrl, (smbus_base + SMBHSTCTL)); + host_outb(smbus_base, SMBHSTCTL, ctrl);
/* Set the device I'm talking to. */ - outb(xmitadd, smbus_base + SMBXMITADD); + host_outb(smbus_base, SMBXMITADD, xmitadd);
return 0; } @@ -137,8 +147,7 @@ u8 status;
/* Start the command. */ - outb((inb(smbus_base + SMBHSTCTL) | SMBHSTCNT_START), - smbus_base + SMBHSTCTL); + host_outb(smbus_base, SMBHSTCTL, host_inb(smbus_base, SMBHSTCTL) | SMBHSTCNT_START);
/* Poll for it to start. */ do { @@ -147,7 +156,7 @@ /* If we poll too slow, we could miss HOST_BUSY flag * set and detect INTR or x_ERR flags instead here. */ - status = inb(smbus_base + SMBHSTSTAT); + status = host_inb(smbus_base, SMBHSTSTAT); status &= ~(SMBHSTSTS_SMBALERT_STS | SMBHSTSTS_INUSE_STS); } while (--loops && status == 0);
@@ -165,7 +174,7 @@
do { smbus_delay(); - status = inb(smbus_base + SMBHSTSTAT); + status = host_inb(smbus_base, SMBHSTSTAT); } while (--loops && !host_completed(status));
if (loops == 0) @@ -187,11 +196,11 @@ return ret;
/* Set the command/address... */ - outb(address, smbus_base + SMBHSTCMD); + host_outb(smbus_base, SMBHSTCMD, address);
/* Clear the data bytes... */ - outb(0, smbus_base + SMBHSTDAT0); - outb(0, smbus_base + SMBHSTDAT1); + host_outb(smbus_base, SMBHSTDAT0, 0); + host_outb(smbus_base, SMBHSTDAT1, 0);
/* Start the command */ ret = execute_command(smbus_base); @@ -204,9 +213,9 @@ return ret;
/* Read results of transaction */ - word = inb(smbus_base + SMBHSTDAT0); + word = host_inb(smbus_base, SMBHSTDAT0); if (ctrl == I801_WORD_DATA) - word |= inb(smbus_base + SMBHSTDAT1) << 8; + word |= host_inb(smbus_base, SMBHSTDAT1) << 8;
return word; } @@ -222,12 +231,12 @@ return ret;
/* Set the command/address... */ - outb(address, smbus_base + SMBHSTCMD); + host_outb(smbus_base, SMBHSTCMD, address);
/* Set the data bytes... */ - outb(data & 0xff, smbus_base + SMBHSTDAT0); + host_outb(smbus_base, SMBHSTDAT0, data & 0xff); if (ctrl == I801_WORD_DATA) - outb(data >> 8, smbus_base + SMBHSTDAT1); + host_outb(smbus_base, SMBHSTDAT1, data >> 8);
/* Start the command */ ret = execute_command(smbus_base); @@ -256,16 +265,16 @@ * was really updated with the transaction. */ if (!sw_drives_nak) { if (is_write_cmd) - outb(max_bytes, smbus_base + SMBHSTDAT0); + host_outb(smbus_base, SMBHSTDAT0, max_bytes); else - outb(0, smbus_base + SMBHSTDAT0); + host_outb(smbus_base, SMBHSTDAT0, 0); }
/* Send first byte from buffer, bytes_sent increments after * hardware acknowledges it. */ if (is_write_cmd) - outb(*buf++, smbus_base + SMBBLKDAT); + host_outb(smbus_base, SMBBLKDAT, *buf++);
/* Start the command */ ret = execute_command(smbus_base); @@ -274,24 +283,24 @@
/* Poll for transaction completion */ do { - status = inb(smbus_base + SMBHSTSTAT); + status = host_inb(smbus_base, SMBHSTSTAT);
if (status & SMBHSTSTS_BYTE_DONE) { /* Byte done */
if (is_write_cmd) { bytes++; if (bytes < max_bytes) - outb(*buf++, smbus_base + SMBBLKDAT); + host_outb(smbus_base, SMBBLKDAT, *buf++); } else { if (bytes < max_bytes) - *buf++ = inb(smbus_base + SMBBLKDAT); + *buf++ = host_inb(smbus_base, SMBBLKDAT); bytes++;
/* Indicate that next byte is the last one. */ if (sw_drives_nak && (bytes + 1 >= max_bytes)) { - outb(inb(smbus_base + SMBHSTCTL) - | SMBHSTCNT_LAST_BYTE, - smbus_base + SMBHSTCTL); + host_outb(smbus_base, SMBHSTCTL, + host_inb(smbus_base, SMBHSTCTL) | + SMBHSTCNT_LAST_BYTE); }
} @@ -300,7 +309,7 @@ * and clears HOST_BUSY flag once the byte count * has been reached or LAST_BYTE was set. */ - outb(SMBHSTSTS_BYTE_DONE, smbus_base + SMBHSTSTAT); + host_outb(smbus_base, SMBHSTSTAT, SMBHSTSTS_BYTE_DONE); }
} while (--loops && !host_completed(status)); @@ -354,7 +363,7 @@ return ret;
/* Set the command/address... */ - outb(cmd, smbus_base + SMBHSTCMD); + host_outb(smbus_base, SMBHSTCMD, cmd);
/* Execute block transaction. */ ret = block_cmd_loop(smbus_base, buf, max_bytes, BLOCK_READ); @@ -362,7 +371,7 @@ return ret;
/* Post-check we received complete message. */ - slave_bytes = inb(smbus_base + SMBHSTDAT0); + slave_bytes = host_inb(smbus_base, SMBHSTDAT0); if (ret < slave_bytes) return SMBUS_ERROR;
@@ -383,7 +392,7 @@ return ret;
/* Set the command/address... */ - outb(cmd, smbus_base + SMBHSTCMD); + host_outb(smbus_base, SMBHSTCMD, cmd);
/* Execute block transaction. */ ret = block_cmd_loop(smbus_base, (u8 *)buf, bytes, BLOCK_WRITE); @@ -425,7 +434,7 @@ return ret;
/* device offset */ - outb(offset, smbus_base + SMBHSTDAT1); + host_outb(smbus_base, SMBHSTDAT1, offset);
/* Execute block transaction. */ ret = block_cmd_loop(smbus_base, buf, bytes, BLOCK_READ | BLOCK_I2C); @@ -468,8 +477,8 @@ */ cmd = *buf++; bytes--; - outb(cmd, smbus_base + SMBHSTCMD); - outb(cmd, smbus_base + SMBHSTDAT1); + host_outb(smbus_base, SMBHSTCMD, cmd); + host_outb(smbus_base, SMBHSTDAT1, cmd);
/* Execute block transaction. */ ret = block_cmd_loop(smbus_base, buf, bytes, BLOCK_WRITE);
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)