[i2c] [PATCH 2of3] change i2c-i801 driver to use internal 32-byte buffer on ICH4+
Oleg Ryjkov
olegr at google.com
Wed Jun 20 03:22:14 CEST 2007
Hi Jean,
I've changed your patch in accordance with your comments. It should
apply after my first patch to this file(and the changes you mentioned in
the previous email to me).
Thanks for your thorough review.
Oleg
On Fri, Jun 15, 2007 at 05:15:46PM +0200, Jean Delvare wrote:
> Hi Oleg,
>
> On Mon, 11 Jun 2007 04:05:33 -0700, Oleg Ryjkov wrote:
> > Second part. The main change is here. See i2c mailing list for the
> > description of all the changes. ( Thread subject: "[i2c] [PATCH] change
> > to i2c-i801 driver to use internal 32-byte buffer on ICH4+".
> >
> > Signed-off-by: Oleg Ryjkov <olegr at google.com>
>
> A couple comments:
>
> > --- linux-2.6-untouched/drivers/i2c/busses/i2c-i801.c 2007-06-11 03:43:18.000000000 -0700
> > +++ linux-2.6/drivers/i2c/busses/i2c-i801.c 2007-06-11 03:43:25.000000000 -0700
> > @@ -26,8 +26,8 @@
> > 82801AB 2423
> > 82801BA 2443
> > 82801CA/CAM 2483
> > - 82801DB 24C3 (HW PEC supported, 32 byte buffer not supported)
> > - 82801EB 24D3 (HW PEC supported, 32 byte buffer not supported)
> > + 82801DB 24C3 (HW PEC supported)
> > + 82801EB 24D3 (HW PEC supported)
> > 6300ESB 25A4
> > ICH6 266A
> > ICH7 27DA
> > @@ -114,7 +114,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;
> > @@ -139,7 +139,9 @@
> > }
> > }
> >
> > - 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);
> >
> > /* We will always wait for a fraction of a second! */
> > do {
>
> This could have been part of the previous cleanup patch, right?
>
Agreed.
> > @@ -207,44 +209,53 @@
> > outb_p(temp, SMBHSTSTS);
> > }
> >
> > -/* All-inclusive block transaction function */
> > -static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
> > - int command, int hwpec)
> > +static int i801_block_transaction_by_block(union i2c_smbus_data *data,
> > + char read_write, int hwpec)
> > {
> > int i, len;
> > - int smbcmd;
> > +
> > + inb_p(SMBHSTCNT); /* reset the data buffer index */
> > +
> > + /* Use 32-byte buffer to process this transaction */
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + len = data->block[0];
> > + outb_p(len, SMBHSTDAT0);
> > + for (i = 0; i < len; i++)
> > + outb_p(data->block[i+1], SMBBLKDAT);
> > + }
> > +
> > + if (i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 | hwpec))
>
> The "hwpec" isn't correct, as it is a boolean value and not a bit
> constant. The PEC_EN flag in register HST_CNT is bit 7, and here you're
> instead setting bit 0 which is INTREN aka ENABLE_INT9.
>
I've changed this to hwpec*I801_PEC_EN, since it does not require
addition of a new define or an extra if statement.
> > + return -1;
> > +
> > + if (read_write == I2C_SMBUS_READ) {
> > + len = inb_p(SMBHSTDAT0);
> > + if (len < 1 || len > 32)
>
> I2C_SMBUS_BLOCK_MAX
Done.
>
> > + return -1;
> > +
> > + data->block[0] = len;
> > + for (i = 0; i < len; 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, int hwpec)
> > +{
> > +
> > int temp;
> > + int i;
> > int result = 0;
> > + int smbcmd;
> > int timeout;
> > - unsigned char hostc, errmask;
> > + int len;
>
> Moving the declarations of i, len and smbcmd at the top as:
>
> int i, len;
> int smbcmd;
>
> minimizes the differences compared to the original code, making the
> patch slightly smaller and more readable.
>
Done.
> > + unsigned char 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;
> > - }
> > - }
> > + len = data->block[0];
> >
> > if (read_write == I2C_SMBUS_WRITE) {
> > - len = data->block[0];
> > - if (len < 1)
> > - len = 1;
> > - 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) {
> > - /* set 32 byte buffer */
> > }
> >
> > for (i = 1; i <= len; i++) {
> > @@ -277,14 +288,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)
> > @@ -326,10 +334,8 @@
> >
> > if (i == 1 && read_write == I2C_SMBUS_READ) {
> > len = inb_p(SMBHSTDAT0);
> > - if (len < 1 || len > 32) {
> > - result = -1;
> > - goto END;
> > - }
> > + if (len < 1 || len > 32)
> > + return -1;
> > data->block[0] = len;
> > }
> >
> > @@ -352,14 +358,65 @@
> > inb_p(SMBHSTDAT0), inb_p(SMBBLKDAT));
> >
> > if (result < 0)
> > - goto END;
> > + return result;
> > }
> > + return result;
> > +}
> >
> > - if (hwpec)
> > +static int i801_set_block_buffer_mode(void)
> > +{
> > + int temp;
> > + temp = inb_p(SMBAUXCTL);
> > + if (temp & SMBAUXCTL_E32B)
> > + return 0;
> > + dev_info(&I801_dev->dev, "Enabling 32-byte data buffer\n");
> > + outb_p(inb_p(SMBAUXCTL) | SMBAUXCTL_E32B, SMBAUXCTL);
>
> Why read SMBAUXCTL again when you have it in temp?
>
> > + if ((inb_p(SMBAUXCTL) & SMBAUXCTL_E32B) == 0) {
> > + dev_err(&I801_dev->dev, "Failed to set E32B bit\n");
> > + return -1;
> > + }
> > + return 0;
> > +}
>
> This implementation isn't compatible with the fact that you now clear
> the E32B bit after each transaction. You will flood the logs with
> "Enabling 32-byte data buffer" messages. And there's no point in
> optimizing the "nothing to do" case, as it will virtually never happen.
>
Fixed - to make it compliant with the fact that E32B is reset after
every transaction, the driver no longer checks the bit before setting
it. I still have the check at the end, since if E32B is not set and a
block buffer is used, only the last byte will be written to the
hardware, and cause the controller to time out.
> > +
> > +/* Block transaction function */
> > +static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
> > + int command, int hwpec)
> > +{
> > + 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;
> > + }
> > + }
> > +
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + if (data->block[0] < 1)
> > + data->block[0] = 1;
> > + if (data->block[0] > 32)
> > + data->block[0] = 32;
>
> I2C_SMBUS_BLOCK_MAX
>
Modified.
> > + } else {
> > + data->block[0] = 32; /* max for reads */
> > + }
> > +
> > + if (isich4 && i801_set_block_buffer_mode() == 0 )
> > + result = i801_block_transaction_by_block(data, read_write,
> > + hwpec);
> > + else
> > + result = i801_block_transaction_byte_by_byte(data, read_write,
> > + hwpec);
> > +
> > + if (result == 0 && hwpec)
> > i801_wait_hwpec();
> >
> > - result = 0;
> > -END:
> > if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
> > /* restore saved configuration register value */
> > pci_write_config_byte(I801_dev, SMBHSTCFG, hostc);
> > @@ -430,15 +487,15 @@
> >
> > 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. Turn off
> > + E32B for the same reason. */
> > if (hwpec)
> > - outb_p(inb_p(SMBAUXCTL) & (~SMBAUXCTL_CRC), SMBAUXCTL);
> > + outb_p(inb_p(SMBAUXCTL) & ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B),
> > + SMBAUXCTL);
> >
> > if(block)
> > return ret;
>
> Care to resend a patch addressing these last few issues?
>
> Thanks,
> --
> Jean Delvare
>
Signed-off-by: Oleg Ryjkov <olegr at google.com>
--- linux-2.6-untouched/drivers/i2c/busses/i2c-i801.c 2007-06-19 18:21:18.000000000 -0700
+++ linux-2.6/drivers/i2c/busses/i2c-i801.c 2007-06-19 18:19:15.000000000 -0700
@@ -26,8 +26,8 @@
82801AB 2423
82801BA 2443
82801CA/CAM 2483
- 82801DB 24C3 (HW PEC supported, 32 byte buffer not supported)
- 82801EB 24D3 (HW PEC supported, 32 byte buffer not supported)
+ 82801DB 24C3 (HW PEC supported)
+ 82801EB 24D3 (HW PEC supported)
6300ESB 25A4
ICH6 266A
ICH7 27DA
@@ -114,7 +114,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;
@@ -139,7 +139,9 @@
}
}
- 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);
/* We will always wait for a fraction of a second! */
do {
@@ -207,44 +209,54 @@
outb_p(temp, SMBHSTSTS);
}
-/* All-inclusive block transaction function */
-static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
- int command, int hwpec)
+static int i801_block_transaction_by_block(union i2c_smbus_data *data,
+ char read_write, int hwpec)
{
int i, len;
+
+ inb_p(SMBHSTCNT); /* reset the data buffer index */
+
+ /* Use 32-byte buffer to process this transaction */
+ if (read_write == I2C_SMBUS_WRITE) {
+ len = data->block[0];
+ outb_p(len, SMBHSTDAT0);
+ for (i = 0; i < len; i++)
+ outb_p(data->block[i+1], SMBBLKDAT);
+ }
+
+ if (i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 |
+ I801_PEC_EN * hwpec))
+ return -1;
+
+ if (read_write == I2C_SMBUS_READ) {
+ len = inb_p(SMBHSTDAT0);
+ if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
+ return -1;
+
+ data->block[0] = len;
+ for (i = 0; i < len; 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, int hwpec)
+{
+
+ int i;
+ int len;
int smbcmd;
int temp;
int result = 0;
int timeout;
- unsigned char hostc, errmask;
+ unsigned char 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;
- }
- }
+ len = data->block[0];
if (read_write == I2C_SMBUS_WRITE) {
- len = data->block[0];
- if (len < 1)
- len = 1;
- 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) {
- /* set 32 byte buffer */
}
for (i = 1; i <= len; i++) {
@@ -277,14 +289,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)
@@ -326,10 +335,8 @@
if (i == 1 && read_write == I2C_SMBUS_READ) {
len = inb_p(SMBHSTDAT0);
- if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
- result = -1;
- goto END;
- }
+ if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
+ return -1;
data->block[0] = len;
}
@@ -352,14 +359,58 @@
inb_p(SMBHSTDAT0), inb_p(SMBBLKDAT));
if (result < 0)
- goto END;
+ return result;
}
+ return result;
+}
- if (hwpec)
+static int i801_set_block_buffer_mode(void)
+{
+ outb_p(inb_p(SMBAUXCTL) | SMBAUXCTL_E32B, SMBAUXCTL);
+ if ((inb_p(SMBAUXCTL) & SMBAUXCTL_E32B) == 0)
+ return -1;
+ return 0;
+}
+
+/* Block transaction function */
+static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
+ int command, int hwpec)
+{
+ 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;
+ }
+ }
+
+ if (read_write == I2C_SMBUS_WRITE) {
+ if (data->block[0] < 1)
+ data->block[0] = 1;
+ if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
+ data->block[0] = I2C_SMBUS_BLOCK_MAX;
+ } else {
+ data->block[0] = I2C_SMBUS_BLOCK_MAX; /* max for reads */
+ }
+
+ if (isich4 && i801_set_block_buffer_mode() == 0 )
+ result = i801_block_transaction_by_block(data, read_write,
+ hwpec);
+ else
+ result = i801_block_transaction_byte_by_byte(data, read_write,
+ hwpec);
+
+ if (result == 0 && hwpec)
i801_wait_hwpec();
- result = 0;
-END:
if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
/* restore saved configuration register value */
pci_write_config_byte(I801_dev, SMBHSTCFG, hostc);
@@ -431,15 +482,15 @@
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. Turn off
+ E32B for the same reason. */
if (hwpec)
- outb_p(inb_p(SMBAUXCTL) & (~SMBAUXCTL_CRC), SMBAUXCTL);
+ outb_p(inb_p(SMBAUXCTL) & ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B),
+ SMBAUXCTL);
if(block)
return ret;
More information about the i2c
mailing list