[i2c] [PATCH] change to i2c-i801 driver to use internal 32-byte buffer on ICH4+
Oleg Ryjkov
olegr at google.com
Mon Jun 11 12:54:37 CEST 2007
On Fri, Jun 08, 2007 at 12:09:18PM +0200, Jean Delvare wrote:
> Hi Oleg,
>
> On Thu, 7 Jun 2007 11:46:09 -0700, Oleg Ryjkov wrote:
> > This patch adds an ability to utilize the internal SRAM buffer on ICH4
> > and newer host controllers to speed up execution of block operations.
> >
> > I've split the code so that it is more clear which block transaction is
> > performed.
> >
> > Firs of all the host controller's type is identified. isich4 is set when
> > we think that the controller has the internal buffer.
> >
> > Before every block transaction, we check whether the E32B bit in
> > SMBAUXCTL register is set(to enable the controller's buffer), otherwise
> > we do an attempt to enable it.
>
> Great, thanks for working on this. Here's my review. I'll test the next
> iteration.
>
You are welcome :) I've split the patch in 3 parts and will send them
out shortly. Thank you for the feedback. Unless I mentioned otherwise,
your comments have all been applied.
> > I've also made some cosmetic changes to i801_transaction, introducing
> > some defines for status bits and an attempt to kill the current
> > trancaction if it has timed out.
>
> While these cleanups are welcome, they do not belong to this patch.
> Your patch is already quite big, and they only make it bigger and
> harder to review. Please split these improvements to a separate patch
> (applying either before or after this one, at your option.) Same for
> the idea of splitting the PEC timeout check to a separate function, I
> like it, but that would be best done in the cleanup patch.
>
> > --- linux-2.6-untouched/drivers/i2c/busses/i2c-i801.c 2007-05-17 16:10:33.000000000 -0700
> > +++ linux-2.6/drivers/i2c/busses/i2c-i801.c 2007-06-07 11:38:24.000000000 -0700
>
> In the header comment of this file, there's a list of supported
> devices, with the mention "32 byte buffer not supported". Your patch
> should remove that mention. Same for Documentation/i2c/busses/i2c-i801.
>
> > @@ -68,6 +68,11 @@
> > /* PCI Address Constants */
> > #define SMBBAR 4
> > #define SMBHSTCFG 0x040
> > +/* 32 byte block bit for SMBHSTAUXCTL */
> > +#define SMBAUXCTL_E32B 2
> > +
> > +/* kill bit for SMBHSTCNT */
> > +#define SMBHSTCNT_KILL 2
>
> Please don't insert these defines in the middle of the PCI defines,
> it's confusing.
>
> >
> > /* Host configuration bits for SMBHSTCFG */
> > #define SMBHSTCFG_HST_EN 1
> > @@ -91,8 +96,17 @@
> > #define I801_START 0x40
> > #define I801_PEC_EN 0x80 /* ICH4 only */
> >
> > +/* I801 Hosts Status register bits */
> > +#define I801_HST_STS_BYTE_DONE (0x01<<7)
> > +#define I801_HST_STS_INUSE_STS (0x01<<6)
> > +#define I801_HST_STS_SMBALERT_STS (0x01<<5)
> > +#define I801_HST_STS_FAILED (0x01<<4)
> > +#define I801_HST_STS_BUS_ERR (0x01<<3)
> > +#define I801_HST_STS_DEV_ERR (0x01<<2)
> > +#define I801_HST_STS_INTR (0x01<<1)
> > +#define I801_HST_STS_HOST_BUSY (0x01<<0)
> >
>
> This is inconsistent. The other register bits were given shorter names
> (e.g. SMBHSTCFG_HST_EN) and are defined directly with values rather
> than bit-shifting. Please make it consistent.
>
These names came out of the Intel specs. I've tried to reduce the
length, hope it is more consistent now.
> > -static int i801_transaction(void);
> > +static int i801_transaction(int xact);
> > static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
> > int command, int hwpec);
>
> You could even discard these forward declarations, they are no longer
> needed (if they ever were.)
>
> >
> > @@ -102,7 +116,7 @@
> > static struct pci_dev *I801_dev;
> > static int isich4;
> >
> > -static int i801_transaction(void)
> > +static int i801_transaction(int xact)
> > {
> > int temp;
> > int result = 0;
> > @@ -127,13 +141,15 @@
> > }
> > }
> >
> > - outb_p(inb(SMBHSTCNT) | I801_START, SMBHSTCNT);
> > + /* the current contents of SMBHSTCNT can be overwritten, since PEC,
> > + * INTREN, SMBSCMD are passed in xact */
> > + outb_p( xact | I801_START, SMBHSTCNT);
>
> Coding style: no space after opening parenthesis.
>
> >
> > /* We will always wait for a fraction of a second! */
> > do {
> > msleep(1);
> > temp = inb_p(SMBHSTSTS);
> > - } while ((temp & 0x01) && (timeout++ < MAX_TIMEOUT));
> > + } while ((temp & I801_HST_STS_HOST_BUSY) && (timeout++ < MAX_TIMEOUT));
> >
> > /* If the SMBus is still busy, we give up */
> > if (timeout >= MAX_TIMEOUT) {
> > @@ -141,19 +157,27 @@
> > result = -1;
> > }
> >
> > - if (temp & 0x10) {
> > + /* try to stop the current command */
> > + if (temp & I801_HST_STS_HOST_BUSY) {
> > + dev_dbg(&I801_dev->dev,"Terminating the current operation\n");
>
> Coding style: space after comma.
>
> > + outb_p(inb_p(SMBHSTCNT) | SMBHSTCNT_KILL, SMBHSTCNT);
> > + msleep(1);
> > + outb_p(inb_p(SMBHSTCNT) & (~SMBHSTCNT_KILL), SMBHSTCNT);
> > + }
>
> You could merge this block and the block before. "timeout >=
> MAX_TIMEOUT" and "temp & I801_HST_STS_HOST_BUSY" are the same condition.
>
> > +
> > + if (temp & I801_HST_STS_FAILED) {
> > result = -1;
> > dev_dbg(&I801_dev->dev, "Error: Failed bus transaction\n");
> > }
> >
> > - if (temp & 0x08) {
> > + if (temp & I801_HST_STS_BUS_ERR) {
> > result = -1;
> > dev_err(&I801_dev->dev, "Bus collision! SMBus may be locked "
> > "until next hard reset. (sorry!)\n");
> > /* Clock stops and slave is stuck in mid-transmission */
> > }
> >
> > - if (temp & 0x04) {
> > + if (temp & I801_HST_STS_DEV_ERR) {
> > result = -1;
> > dev_dbg(&I801_dev->dev, "Error: no response!\n");
> > }
> > @@ -172,45 +196,65 @@
> > return result;
> > }
> >
> > -/* All-inclusive block transaction function */
> > -static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
> > - int command, int hwpec)
> > +/* wait for INTR bit as advised by Intel */
> > +static void i801_wait_hwpec(void)
> > {
> > - int i, len;
> > - int smbcmd;
> > + int timeout = 0;
> > int temp;
> > - int result = 0;
> > - int timeout;
> > - unsigned char hostc, errmask;
> >
> > - if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
> > - if (read_write == I2C_SMBUS_WRITE) {
> > - /* set I2C_EN bit in configuration register */
> > - pci_read_config_byte(I801_dev, SMBHSTCFG, &hostc);
> > - pci_write_config_byte(I801_dev, SMBHSTCFG,
> > - hostc | SMBHSTCFG_I2C_EN);
> > - } else {
> > - dev_err(&I801_dev->dev,
> > - "I2C_SMBUS_I2C_BLOCK_READ not DB!\n");
> > - return -1;
> > - }
> > + do {
> > + msleep(1);
> > + temp = inb_p(SMBHSTSTS);
> > + } while ((!(temp & I801_HST_STS_INTR))
> > + && (timeout++ < MAX_TIMEOUT));
> > +
> > + if (timeout >= MAX_TIMEOUT) {
> > + dev_dbg(&I801_dev->dev, "PEC Timeout!\n");
> > }
> > + outb_p(temp, SMBHSTSTS);
> > +}
> >
> > - if (read_write == I2C_SMBUS_WRITE) {
> > - len = data->block[0];
> > +
> > +static int i801_block_transaction_by_block(union i2c_smbus_data *data, char read_write,
>
> Line too long, please fold.
>
> > + int len, int hwpec) {
>
> You shouldn't need a separate parameter to pass the length,
> data->block[0] is typically used for that.
>
> > + int i;
> > +
> > + inb_p(SMBHSTCNT); /* reset the data buffer index */
> > +
> > + /* Use 32-byte buffer to process this transaction */
> > + if (read_write == I2C_SMBUS_WRITE)
> > + for (i = 0; i < len; i += 1 )
>
> "i += 1" is better written "i++".
>
That's debatable.
My habit is to use "+=" operator instead of "++", though I'll use "++"
when changing any other linux kernel code. :)
> > + outb_p(data->block[i+1], SMBBLKDAT);
> > +
> > + if (i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 | hwpec))
> > + return -1;
> > +
> > + if (read_write == I2C_SMBUS_READ) {
> > + len = inb_p(SMBHSTDAT0);
> > if (len < 1)
> > len = 1;
>
> This doesn't look correct to me. If the transaction returned a length
> of 0, we shouldn't attempt to read one byte from the buffer.
>
> > if (len > 32)
> > len = 32;
>
> And in fact, I am wondering why we don't simply return with an error
> when the length is invalid. If the slave didn't reply with a valid
> length, why would the following data be valid?
>
> > - outb_p(len, SMBHSTDAT0);
> > - outb_p(data->block[1], SMBBLKDAT);
> > - } else {
> > - len = 32; /* max for reads */
> > - }
> >
> > - if(isich4 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
> > - /* set 32 byte buffer */
> > + data->block[0] = len;
> > + for (i = 0; i < len; i += 1)
>
> i++
>
> > + data->block[i + 1] = inb_p(SMBBLKDAT);
> > }
> > + return 0;
> > +}
> > +
> > +static int i801_block_transaction_byte_by_byte(union i2c_smbus_data *data, char read_write,
>
> Line too long, please fold.
>
> > + int len, int hwpec) {
>
> Here too, data->block[0] should be used to pass the block length.
>
> > +
> > + int temp;
> > + int i;
> > + int result = 0;
> > + int smbcmd;
> > + int timeout;
> > + unsigned char errmask;
> > +
> > + if (read_write == I2C_SMBUS_WRITE)
> > + outb_p(len, SMBHSTDAT0);
>
> Wasn't this already done in i801_block_transaction()?
>
> >
> > for (i = 1; i <= len; i++) {
> > if (i == len && read_write == I2C_SMBUS_READ)
> > @@ -227,13 +271,14 @@
> > /* Make sure the SMBus host is ready to start transmitting */
> > temp = inb_p(SMBHSTSTS);
> > if (i == 1) {
> > - /* Erronenous conditions before transaction:
> > - * Byte_Done, Failed, Bus_Err, Dev_Err, Intr, Host_Busy */
> > - errmask=0x9f;
> > + /* Erronenous conditions before transaction:
> > + * Byte_Done, Failed, Bus_Err, Dev_Err, Intr,
> > + * Host_Busy */
> > + errmask=0x9f;
> > } else {
> > - /* Erronenous conditions during transaction:
> > + /* Erronenous conditions during transaction:
> > * Failed, Bus_Err, Dev_Err, Intr */
> > - errmask=0x1e;
> > + errmask=0x1e;
> > }
> > if (temp & errmask) {
> > dev_dbg(&I801_dev->dev, "SMBus busy (%02x). "
>
> These white space cleanups should be moved to yet another patch. Here
> they only make it harder to focus on the real changes.
>
> > @@ -242,14 +287,11 @@
> > if (((temp = inb_p(SMBHSTSTS)) & errmask) != 0x00) {
> > dev_err(&I801_dev->dev,
> > "Reset failed! (%02x)\n", temp);
> > - result = -1;
> > - goto END;
> > + return -1;
> > }
> > - if (i != 1) {
> > + if (i != 1)
> > /* if die in middle of block transaction, fail */
> > - result = -1;
> > - goto END;
> > - }
> > + return -1;
> > }
> >
> > if (i == 1)
> > @@ -261,8 +303,8 @@
> > msleep(1);
> > temp = inb_p(SMBHSTSTS);
> > }
> > - while ((!(temp & 0x80))
> > - && (timeout++ < MAX_TIMEOUT));
> > + while ((!(temp & 0x80))
> > + && (timeout++ < MAX_TIMEOUT));
> >
> > /* If the SMBus is still busy, we give up */
> > if (timeout >= MAX_TIMEOUT) {
>
> Same here. BTW, shouldn't this 0x80 become I801_HST_STS_BYTE_DONE?
>
> > @@ -310,25 +352,70 @@
> > inb_p(SMBHSTDAT0), inb_p(SMBBLKDAT));
> >
> > if (result < 0)
> > - goto END;
> > + return result;
> > }
> >
> > - if (hwpec) {
> > - /* wait for INTR bit as advised by Intel */
> > - timeout = 0;
> > - do {
> > - msleep(1);
> > - temp = inb_p(SMBHSTSTS);
> > - } while ((!(temp & 0x02))
> > - && (timeout++ < MAX_TIMEOUT));
> > + return result;
> > +}
> >
> > - if (timeout >= MAX_TIMEOUT) {
> > - dev_dbg(&I801_dev->dev, "PEC Timeout!\n");
> > +static int i801_try_set_block_buffer_mode(void)
>
> The "try" in the function name isn't really needed. All functions try
> to do things, and sometimes fail.
>
> > +{
> > + int temp;
> > + temp = inb_p(SMBAUXCTL);
> > + if (temp & SMBAUXCTL_E32B)
> > + return 0;
> > + dev_dbg(&I801_dev->dev, "Enabling 32-bit data buffer\n");
> > + /* enable 32-bit data buffer */
>
> 32-byte, not 32-bit. This could be made a dev_info(), I think, so that
> it shows in user bug reports. The comment isn't really needed, as it
> says the same as the log message.
>
> > + outb_p(inb_p(SMBAUXCTL) | SMBAUXCTL_E32B, SMBAUXCTL);
>
> You don't reuse the value of temp here?
>
> > + if ((inb_p(SMBAUXCTL) & SMBAUXCTL_E32B) == 0) {
> > + dev_err(&I801_dev->dev, "Failed to set E32B bit\n");
> > + return -1;
> > + }
> > + return 0;
> > +}
> > +
> > +/* All-inclusive block transaction function */
>
> It's no longer "all-inclusive".
>
> > +static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
> > + int command, int hwpec)
> > +{
> > + int len;
> > + int result = 0;
> > + unsigned char hostc;
> > +
> > + if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + /* set I2C_EN bit in configuration register */
> > + pci_read_config_byte(I801_dev, SMBHSTCFG, &hostc);
> > + pci_write_config_byte(I801_dev, SMBHSTCFG,
> > + hostc | SMBHSTCFG_I2C_EN);
> > + } else {
> > + dev_err(&I801_dev->dev,
> > + "I2C_SMBUS_I2C_BLOCK_READ not DB!\n");
> > + return -1;
> > }
> > - outb_p(temp, SMBHSTSTS);
> > }
> > - result = 0;
> > -END:
> > +
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + len = data->block[0];
> > + if (len < 1)
> > + len = 1;
>
> This again is wrong (how is block[1] supposed to contain something
> meaningful if the caller set the length to 0?) I would simply return
> with an error when the length isn't valid.
>
> > + if (len > 32)
> > + len = 32;
> > + outb_p(len, SMBHSTDAT0);
> > + outb_p(data->block[1], SMBBLKDAT);
> > + } else {
> > + len = 32; /* max for reads */
> > + }
> > +
> > + if (isich4 && command != I2C_SMBUS_I2C_BLOCK_DATA
> > + && i801_try_set_block_buffer_mode() == 0 )
>
> Are you certain that the block buffer cannot be used in I2C mode?
No. But the inability to test it and the presense of this comment in
the current driver forced me to leave it disabled:
if(isich4 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
/* set 32 byte buffer */
}
However, I've checked several manuals of ICH4 and newer hosts and none
of them mention anything about not being able to use the SRAM buffer
for I2C transactions. So I removing this condition from the if
statement.
>
> Coding style: no space before closing parenthesis.
>
> > + result = i801_block_transaction_by_block(data, read_write, len, hwpec);
> > + else
> > + result = i801_block_transaction_byte_by_byte(data, read_write, len, hwpec);
> > +
> > + if (result == 0 && hwpec)
> > + i801_wait_hwpec();
> > +
> > if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
> > /* restore saved configuration register value */
> > pci_write_config_byte(I801_dev, SMBHSTCFG, hostc);
> > @@ -393,19 +480,20 @@
> > return -1;
> > }
> >
> > - outb_p(hwpec, SMBAUXCTL); /* enable/disable hardware PEC */
> > + if (hwpec)
> > + outb_p(inb_p(SMBAUXCTL) | hwpec, SMBAUXCTL); /* enable/disable
> > + hardware PEC */
> >
> > if(block)
> > ret = i801_block_transaction(data, read_write, size, hwpec);
> > - else {
> > - outb_p(xact | ENABLE_INT9, SMBHSTCNT);
> > - ret = i801_transaction();
> > - }
> > + else
> > + ret = i801_transaction(xact | ENABLE_INT9);
> >
> > /* Some BIOSes don't like it when PEC is enabled at reboot or resume
> > - time, so we forcibly disable it after every transaction. */
> > + * time, so we forcibly disable it after every transaction. Make sure
> > + * we preserve the E32B bit. */
>
> Why do you actually want to preserve it? You will set it again as
> needed for the next transaction. And the same problem some BIOS have
> when PEC is left enabled could happen with the E32B bit. So I'd rather
> suggest that we always restore the AUX_CTL register to the exact same
> value we found it in.
>
> > if (hwpec)
> > - outb_p(0, SMBAUXCTL);
> > + outb_p(inb_p(SMBAUXCTL) & 0xfe, SMBAUXCTL);
>
> It would be nice to have a define for this bit.
>
> As a side note, I am curious if PEC support really works. According to
> the datasheet, two bits need to be set to enable PEC, bit 7 of register
> HST_CNT, and bit 1 of register AUX_CTL. We only set the latter. We
> define I801_PEC_EN for the former but don't use it anywhere.
>
Unfortunately, I have no clue about the state of PEC mode since I don't
have access to any i2c devices that support this mode. Thus the I tried
not to change any of the PEC functionality that the driver currently
has.
Then again, the PEC changes should be in a different patch anyways. ;)
> >
> > if(block)
> > return ret;
> > @@ -480,10 +568,13 @@
> > case PCI_DEVICE_ID_INTEL_ESB2_17:
> > case PCI_DEVICE_ID_INTEL_ICH8_5:
> > case PCI_DEVICE_ID_INTEL_ICH9_6:
> > + /* ICH4 and the newer host controllers also support 32-byte
> > + * data buffer */
>
> "also" suggests that another difference has been mentioned before. This
> comment should be moved there.
>
> > isich4 = 1;
> > break;
> > default:
> > isich4 = 0;
> > + dev_dbg(&dev->dev, "32-bit data buffer is not present\n");
>
> I don't think this is really worth logging.
>
> > }
> >
> > err = pci_enable_device(dev);
>
> OK, can you please resend two or three patches according to my comments?
>
> Thanks,
> --
> Jean Delvare
>
Thanks again for you review.
Oleg
More information about the i2c
mailing list