Intel ICHx bus driver

Intel ICHx bus driver

[Felix Rubinstein]

Running i2cdetect on ICH9 says:
"I2C Block Write                  yes"

But having a closer look at how the whole thing is implemented proves
that no i2c block write is supported.

The proof:
1. i801_probe turns i801_features to FEATURE_I2C_BLOCK_READ
2. as a sequence in i801_block_transaction
i801_block_transaction_byte_by_byte is called
3. on the other hand i801_block_transaction_byte_by_byte does
   if (read_write == I2C_SMBUS_WRITE)
              outb_p(len, SMBHSTDAT0);
meaning SMBus length.

But what if I want to write I2C's multi-block (without length or even
command before)?
I cannot understand why i801_func turns on I2C_FUNC_SMBUS_WRITE_I2C_BLOCK?

Thanks,
Felix R.

Re: Intel ICHx bus driver

[Jean Delvare]

Hi Felix,

On Wed, 27 Jan 2010 19:56:02 +0200, Felix Rubinstein wrote:
> Running i2cdetect on ICH9 says:
> "I2C Block Write                  yes"
> 
> But having a closer look at how the whole thing is implemented proves
> that no i2c block write is supported.
>
> The proof:
> 1. i801_probe turns i801_features to FEATURE_I2C_BLOCK_READ

Yes it does, but this is totally unrelated to I2C block _writes_.

> 2. as a sequence in i801_block_transaction
> i801_block_transaction_byte_by_byte is called

No. Read the code again, for the ICH9, i801_block_transaction_by_block
is called, not i801_block_transaction_byte_by_byte.

> 3. on the other hand i801_block_transaction_byte_by_byte does
>    if (read_write == I2C_SMBUS_WRITE)
>               outb_p(len, SMBHSTDAT0);
> meaning SMBus length.

This is correct, and as a matter of fact,
i801_block_transaction_by_block does the same. And this is the right
thing to do: the SMBus controller must know how many bytes it must send
to the target slave.

> But what if I want to write I2C's multi-block (without length or even
> command before)?

I have no idea what you mean with "I2C's multi-block". Please be
specific.

> I cannot understand why i801_func turns on I2C_FUNC_SMBUS_WRITE_I2C_BLOCK?

Because it supports that transaction type. Why would you want it to not
advertise a transaction type it supports?

Please refer to Documentation/i2c/smbus-protocol for what exactly each
supported transaction type is doing at the wire level.

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Felix Rubinstein]

Hi Jean,
Thanks for clarifying things, but I still have got some misunderstandings :)
Inlined.

On Thu, Jan 28, 2010 at 9:59 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Felix,
>
> On Wed, 27 Jan 2010 19:56:02 +0200, Felix Rubinstein wrote:
>> Running i2cdetect on ICH9 says:
>> "I2C Block Write                  yes"
>>
>> But having a closer look at how the whole thing is implemented proves
>> that no i2c block write is supported.
>>
>> The proof:
>> 1. i801_probe turns i801_features to FEATURE_I2C_BLOCK_READ
>
> Yes it does, but this is totally unrelated to I2C block _writes_.
>
>> 2. as a sequence in i801_block_transaction
>> i801_block_transaction_byte_by_byte is called
>
> No. Read the code again, for the ICH9, i801_block_transaction_by_block
> is called, not i801_block_transaction_byte_by_byte.
Pardon, I was wrong didn't pay careful attention to the fall through
in i801_probe when FEATURE_BLOCK_BUFFER is also turned on.

>
>> 3. on the other hand i801_block_transaction_byte_by_byte does
>>    if (read_write == I2C_SMBUS_WRITE)
>>               outb_p(len, SMBHSTDAT0);
>> meaning SMBus length.
>
> This is correct, and as a matter of fact,
> i801_block_transaction_by_block does the same. And this is the right
> thing to do: the SMBus controller must know how many bytes it must send
> to the target slave.
Please explain to me how do you interpret "I2C Block Write"?
To my understanding it's I2C, not SMBus, transaction on the bus,
meaning (from Documentation/i2c/i2c-protocol)
S Addr Wr [A] Data [A] Data [A] ... [A] Data [A] P

on the other hand SMBus transaction looks like this:
S Addr Wr [A] Comm [A] Count [A] Data [A] Data [A] ... [A] Data [A] P

The diff is obvious, no Count (not to say Comm) bytes in I2C
transaction (well, it's clear, ICH9 is SMBus, not I2C bus). But what
does "I2C Block Write" then means?

>
>> But what if I want to write I2C's multi-block (without length or even
>> command before)?
>
> I have no idea what you mean with "I2C's multi-block". Please be
> specific.
S Addr Wr [A] Data [A] Data [A] ... [A] Data [A] P

>
>> I cannot understand why i801_func turns on I2C_FUNC_SMBUS_WRITE_I2C_BLOCK?
>
> Because it supports that transaction type. Why would you want it to not
> advertise a transaction type it supports?
>
Does  i2c-i801.c  support the following kind of transactions?
S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P
(note, no count byte on the bus, taken from Documentation/i2c/smbus-protocol)

looking at i801_block_transaction_by_block:

        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);
        }

Meaning len is put on the bus.

> Please refer to Documentation/i2c/smbus-protocol for what exactly each
> supported transaction type is doing at the wire level.
>
> --
> Jean Delvare
> http://khali.linux-fr.org/wishlist.html
>

Re: Intel ICHx bus driver

[Jean Delvare]

Hi Felix,

On Thu, 28 Jan 2010 11:32:28 +0200, Felix Rubinstein wrote:
> Please explain to me how do you interpret "I2C Block Write"?
> To my understanding it's I2C, not SMBus, transaction on the bus,
> meaning (from Documentation/i2c/i2c-protocol)
> S Addr Wr [A] Data [A] Data [A] ... [A] Data [A] P

No, the above isn't what people commonly call "I2C block write". What
people commonly call "I2C block write", and which would probably be
more appropriately called "1-byte addressing I2C block write" is (as
documented in Documentation/i2c/i2c-protocol):

S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P

That being said, please note that the difference is only theoretical:
nothing differentiates command bytes from data bytes on the wire.

> on the other hand SMBus transaction looks like this:
> S Addr Wr [A] Comm [A] Count [A] Data [A] Data [A] ... [A] Data [A] P
> 
> The diff is obvious, no Count (not to say Comm) bytes in I2C
> transaction (well, it's clear, ICH9 is SMBus, not I2C bus). But what
> does "I2C Block Write" then means?

See "I2C block write" as functionally equivalent to "SMBus block write"
but for non-SMBus devices such as I2C EEPROMs. The goal is the same
(write a series of bytes to the device at a given sub-address) but the
on-the-wire format is different (size goes on the wire for SMBus, not
for I2C.)

While "I2C block write" isn't part of the SMBus specification, we have
implemented it in a similar way, simply because many SMBus controllers
implement that transaction type.

> > I have no idea what you mean with "I2C's multi-block". Please be
> > specific.
>
> S Addr Wr [A] Data [A] Data [A] ... [A] Data [A] P

OK, that's a straight I2C write of arbitrary length.

> Does  i2c-i801.c  support the following kind of transactions?
> S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P
> (note, no count byte on the bus, taken from Documentation/i2c/smbus-protocol)

Yes, it does. Use i2c_smbus_write_i2c_block_data(). And if you don't
need the command byte, you can abuse it by passing the first data byte
as the command.

> looking at i801_block_transaction_by_block:
> 
>         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);
>         }
> 
> Meaning len is put on the bus.

No, the above piece of code doesn't imply this. The length is written
to one register of the SMBus controller. It doesn't imply in any way
that the controller will push that value on the wire. In the case of
I2C block transactions, it does not.

BTW, the ICH9 datasheet is public, so you can easily check what exactly
the controller does for each transaction type.

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Felix Rubinstein]

Hi Jean,

On Thu, Jan 28, 2010 at 11:53 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Felix,
>
> On Thu, 28 Jan 2010 11:32:28 +0200, Felix Rubinstein wrote:
>> Please explain to me how do you interpret "I2C Block Write"?
>> To my understanding it's I2C, not SMBus, transaction on the bus,
>> meaning (from Documentation/i2c/i2c-protocol)
>> S Addr Wr [A] Data [A] Data [A] ... [A] Data [A] P
>
> No, the above isn't what people commonly call "I2C block write". What
> people commonly call "I2C block write", and which would probably be
> more appropriately called "1-byte addressing I2C block write" is (as
> documented in Documentation/i2c/i2c-protocol):
>
> S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P
>
> That being said, please note that the difference is only theoretical:
> nothing differentiates command bytes from data bytes on the wire.
>
>> on the other hand SMBus transaction looks like this:
>> S Addr Wr [A] Comm [A] Count [A] Data [A] Data [A] ... [A] Data [A] P
>>
>> The diff is obvious, no Count (not to say Comm) bytes in I2C
>> transaction (well, it's clear, ICH9 is SMBus, not I2C bus). But what
>> does "I2C Block Write" then means?
>
> See "I2C block write" as functionally equivalent to "SMBus block write"
> but for non-SMBus devices such as I2C EEPROMs. The goal is the same
> (write a series of bytes to the device at a given sub-address) but the
> on-the-wire format is different (size goes on the wire for SMBus, not
> for I2C.)
>
> While "I2C block write" isn't part of the SMBus specification, we have
> implemented it in a similar way, simply because many SMBus controllers
> implement that transaction type.
>
>> > I have no idea what you mean with "I2C's multi-block". Please be
>> > specific.
>>
>> S Addr Wr [A] Data [A] Data [A] ... [A] Data [A] P
>
> OK, that's a straight I2C write of arbitrary length.
>
>> Does  i2c-i801.c  support the following kind of transactions?
>> S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P
>> (note, no count byte on the bus, taken from Documentation/i2c/smbus-protocol)
>
> Yes, it does. Use i2c_smbus_write_i2c_block_data(). And if you don't
> need the command byte, you can abuse it by passing the first data byte
> as the command.
>
>> looking at i801_block_transaction_by_block:
>>
>>         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);
>>         }
>>
>> Meaning len is put on the bus.
>
> No, the above piece of code doesn't imply this. The length is written
> to one register of the SMBus controller. It doesn't imply in any way
> that the controller will push that value on the wire. In the case of
> I2C block transactions, it does not.
How exactly the len is not pushed on the wire?
In i801_transaction, the outb_p(xact | I801_START, SMBHSTCNT);
where xact has I801_BLOCK_DATA (101b) turned on.

here is an excerpt from ICH9 datasheet:
101 = Block: This command uses the transmit slave address, command, DATA0
registers, and the Block Data Byte register. For block write, the
count is stored
in the DATA0 register and indicates how many bytes of data will be transferred.

But DATA0 was assigned a value of len before that by means of the
above code snipet.
Please shed the light why SMBus controller will not push the len byte
on the wire if DATA0 equals to len and Block SMB_CMD (as defined in
the datasheet on page 761) equals to 101b (I801_BLOCK_DATA).


Thanks,
Felix R.


>
> BTW, the ICH9 datasheet is public, so you can easily check what exactly
> the controller does for each transaction type.
>
> --
> Jean Delvare
> http://khali.linux-fr.org/wishlist.html
>

Re: Intel ICHx bus driver

[Jean Delvare]

Hi Felix,

On Thu, 28 Jan 2010 14:46:23 +0200, Felix Rubinstein wrote:
> On Thu, Jan 28, 2010 at 11:53 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > No, the above piece of code doesn't imply this. The length is written
> > to one register of the SMBus controller. It doesn't imply in any way
> > that the controller will push that value on the wire. In the case of
> > I2C block transactions, it does not.
> How exactly the len is not pushed on the wire?
> In i801_transaction, the outb_p(xact | I801_START, SMBHSTCNT);
> where xact has I801_BLOCK_DATA (101b) turned on.
> 
> here is an excerpt from ICH9 datasheet:
> 101 = Block: This command uses the transmit slave address, command, DATA0
> registers, and the Block Data Byte register. For block write, the
> count is stored
> in the DATA0 register and indicates how many bytes of data will be transferred.
> 
> But DATA0 was assigned a value of len before that by means of the
> above code snipet.
> Please shed the light why SMBus controller will not push the len byte
> on the wire if DATA0 equals to len and Block SMB_CMD (as defined in
> the datasheet on page 761) equals to 101b (I801_BLOCK_DATA).

You didn't finish your homework... ;)

Straight from the ICH9 datasheet:

Note: For Block Write, if the I2C_EN bit is set, the format of the command changes slightly.
      The ICH9 will still send the number of bytes (on writes) or receive the number of bytes
      (on reads) indicated in the DATA0 register. However, it will not send the contents of the
      DATA0 register as part of the message. Also, the Block Write protocol sequence
      changes slightly: the Byte Count (bits 27:20 in the bit sequence) are not sent - as a
      result, the slave will not acknowledge (bit 28 in the sequence).

And we do set the I2C_EN bit in the i2c_smbus_write_i2c_block_data()
case:

	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);
		} [...]

I hope this clarifies the situation.

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Jean Delvare]

Hi Felix,

On Fri, 19 Feb 2010 09:10:03 +0200, Felix Rubinstein wrote:
> Does ICHx support straight I2C read of arbitrary length, i.e. without doing
> write first?

No, it doesn't. The only straight read lengths supported are 0 byte
(SMBus quick command) and 1 byte (SMBus receive byte).

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Felix Rubinstein]

Hi Jean/i2c guys,

I'm having hard time to send straight I2C transaction of arbitrary length.
I use i2c_smbus_write_i2c_block_data(fd, buf[0], datac - 1, &buf[1]);
from user-space to send the aforementioned transaction, but I get
operation not permitted as a result.
I tried to understand what's going on in the driver and found out that
timeout occurs.

As a consequence, I'd like to get something clear to me:
Since ICH9 supports 32-Byte Buffer (E32B) and according to the data
sheet the DS bit of the HST_STS is meaningless when E32B is enabled
and it's (E32B) enabled by the i801 driver.
On the other hand, in i801_check_pre called (from
i801_block_transaction_by_block) the macro STATUS_FLAGS verifies the
DS bit (designated as SMBHSTSTS_BYTE_DONE in the code).
Looking at the previous version of the driver, like in 2.5.25 there
was a mask 0x1f but it has gone since.
Is this behavior correct?

And another question related. Have you/anyone tried to output I2C
write transaction of arbitrary length, i.e. DATA0 is not sent on the
bus, from ICH9?

Thank you in advance,
Felix R.


On Fri, Feb 19, 2010 at 11:58 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Felix,
>
> On Fri, 19 Feb 2010 09:10:03 +0200, Felix Rubinstein wrote:
>> Does ICHx support straight I2C read of arbitrary length, i.e. without doing
>> write first?
>
> No, it doesn't. The only straight read lengths supported are 0 byte
> (SMBus quick command) and 1 byte (SMBus receive byte).
>
> --
> Jean Delvare
> http://khali.linux-fr.org/wishlist.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: Intel ICHx bus driver

[Jean Delvare]

Hi Felix,

On Mon, 22 Feb 2010 18:12:41 +0200, Felix Rubinstein wrote:
> Hi Jean/i2c guys,
> 
> I'm having hard time to send straight I2C transaction of arbitrary length.
> I use i2c_smbus_write_i2c_block_data(fd, buf[0], datac - 1, &buf[1]);
> from user-space to send the aforementioned transaction, but I get
> operation not permitted as a result.
> I tried to understand what's going on in the driver and found out that
> timeout occurs.

Unlikely. If a timeout occurred, the error message would say so.

What is the exact error message? Can we see your complete code?

Don't forget that you must be root to use i2c-dev.

> As a consequence, I'd like to get something clear to me:
> Since ICH9 supports 32-Byte Buffer (E32B) and according to the data
> sheet the DS bit of the HST_STS is meaningless when E32B is enabled
> and it's (E32B) enabled by the i801 driver.
> On the other hand, in i801_check_pre called (from
> i801_block_transaction_by_block) the macro STATUS_FLAGS verifies the
> DS bit (designated as SMBHSTSTS_BYTE_DONE in the code).
> Looking at the previous version of the driver, like in 2.5.25 there
> was a mask 0x1f but it has gone since.
> Is this behavior correct?

The change happened in this patch:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=cf898dc5e9dfd1487b28ca0176b68722f05d4d48

The new behavior is believed to be correct. I agree that
SMBHSTSTS_BYTE_DONE doesn't matter for block transactions, but checking
this bit can't hurt.

> And another question related. Have you/anyone tried to output I2C
> write transaction of arbitrary length, i.e. DATA0 is not sent on the
> bus, from ICH9?

I don't have any device on my ICH SMBus to which I can issue an I2C
block write, sorry. I did test block reads though, and successfully.

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Felix Rubinstein]

Here is my code:

------------
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <stdlib.h>

#include "i2c-dev.h"
#include "i2cbusses.h"
#include "util.h"

/* actually smbus allows up to 32 and i2c even more */
#define I2CRWL_MAX_PARAMS 10
#define I2CRWL_PARAMS_SHIFT 3

static int i2c_writel(int fd, int datac, char *datav[])
{
        int i;
        unsigned char buf[I2CRWL_MAX_PARAMS];
        unsigned int data;

        for (i = 0; i < datac && i < I2CRWL_MAX_PARAMS; i++) {
                sscanf(datav[i], "%x", &data);
                buf[i] = (unsigned char)data;
        }

        if (i2c_smbus_write_i2c_block_data(fd, buf[0], datac - 1,
&buf[1]) < 0) {
                perror("\n");
                return 1;
        }

        return 0;
}


static void help(const char *progname)
{
        fprintf(stderr,
                        "Usage: %s I2CBUS CHIP-ADDRESS DATA0 [DATA1
... DATAn]\n"
                        "  I2CBUS is an integer or an I2C bus name\n"
                        "  CHIP-ADDRESS is an integer (0x03 - 0x77)\n"
                        "  DATAx is data to be written to the chip,
where 0 <= x <= n\n\n", progname);
        exit(1);
}

int main(int argc, char *argv[])
{
        int fd, i2cbus, addr, ret = 0;
        char filename[20];

        if ((argc < I2CRWL_PARAMS_SHIFT + 1) || (I2CRWL_MAX_PARAMS +
I2CRWL_PARAMS_SHIFT < argc))
                help(argv[0]);

        i2cbus = lookup_i2c_bus(argv[1]);
        if (i2cbus < 0)
                help(argv[0]);

        addr = parse_i2c_address(argv[2]);
        if (addr < 0)
                help(argv[0]);

        fd = open_i2c_dev(i2cbus, filename, 0);
        if (fd < 0)
                exit(1);

        if (ioctl(fd, I2C_SLAVE, addr) < 0) {
                ret = 1;
                perror("");
                goto out;
        }


        if (i2c_writel(fd, argc - I2CRWL_PARAMS_SHIFT,
&argv[I2CRWL_PARAMS_SHIFT])) {
                ret = 1;
                goto out;
        }


out:
        close(fd);

        return ret;
}
------------

BTW, I've disabled the FEATURE_BLOCK_BUFFER

--- i2c-i801.c     2010-02-24 10:50:50.060209638 +0200
+++ i2c-i801.c.orig    2010-02-24 13:55:29.664070673 +0200
@@ -603,7 +603,6 @@
                /* fall through */
        case PCI_DEVICE_ID_INTEL_82801DB_3:
                i801_features |= FEATURE_SMBUS_PEC;
-               i801_features |= FEATURE_BLOCK_BUFFER;
                break;
        }

and now everything works smoothly. I2C write transaction of arbitrary
length are seen even by scope :)

In case if I don't, here is what I get:

$ dmesg | tail
Transaction timeout
Terminating the current operation
Failed terminating the transaction
Failed clearing status flags at end of transaction ...

Thanks,
Felix R.

On Wed, Feb 24, 2010 at 1:21 AM, Felix Rubinstein <felixru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Jean,
>
> On Mon, Feb 22, 2010 at 11:58 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>> Hi Felix,
>>
>> On Mon, 22 Feb 2010 18:12:41 +0200, Felix Rubinstein wrote:
>>> Hi Jean/i2c guys,
>>>
>>> I'm having hard time to send straight I2C transaction of arbitrary length.
>>> I use i2c_smbus_write_i2c_block_data(fd, buf[0], datac - 1, &buf[1]);
>>> from user-space to send the aforementioned transaction, but I get
>>> operation not permitted as a result.
>>> I tried to understand what's going on in the driver and found out that
>>> timeout occurs.
>>
>> Unlikely. If a timeout occurred, the error message would say so.
>>
>> What is the exact error message? Can we see your complete code?
>
> Sure, I'm not at the office right now, but will post it asap.
>
> But hey, I think I've found an issue here.
> Let's delve into the i801 driver code for a moment please.
>
> in i801_transaction:
> ...
> /* We will always wait for a fraction of a second! */
>         do {
>                 msleep(1);
>                 status = inb_p(SMBHSTSTS);
>         } while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_TIMEOUT));
> ...
>
> The data sheet states for HST_STS reg for HOST_BUSY bit:
> 1 = Indicates that the ICH9 is running a command from the host interface. No SMB
> registers should be accessed while this bit is set, except the BLOCK DATA BYTE
> Register. The BLOCK DATA BYTE Register can be accessed when this bit is set only
> when the SMB_CMD bits in the Host Control Register are programmed for Block
> command or I2C Read command. This is necessary in order to check the
> DONE_STS bit.
>
> Remember my case? I'm issuing plain I2C multi byte (straight I2C with
> arbitrary length) transaction, in ICH9 words SMB_CMD is set to Block
> command. Since E32B is enabled, DONE_STS is irrelevant for us in this
> case. As I understand, in this case we should relay on interrupts and
> not on polling, as both: E32B and Block (write) command are enabled.
>
> That is why in my case I'm seeing timeout > MAX_TIMEOUT.
>
> As an alternative, I could try without E32B?
>
> What do you think?
>
> Thanks,
> Felix R.
>

Re: Intel ICHx bus driver

[Jean Delvare]

Hi Felix,

Please keep the linux-i2c list in Cc.

On Wed, 24 Feb 2010 01:21:51 +0200, Felix Rubinstein wrote:
> On Mon, Feb 22, 2010 at 11:58 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > Unlikely. If a timeout occurred, the error message would say so.
> >
> > What is the exact error message? Can we see your complete code?
> 
> Sure, I'm not at the office right now, but will post it asap.
> 
> But hey, I think I've found an issue here.
> Let's delve into the i801 driver code for a moment please.
> 
> in i801_transaction:
> ...
> /* We will always wait for a fraction of a second! */
>          do {
>                  msleep(1);
>                  status = inb_p(SMBHSTSTS);
>          } while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_TIMEOUT));
> ...
> 
> The data sheet states for HST_STS reg for HOST_BUSY bit:
> 1 = Indicates that the ICH9 is running a command from the host interface. No SMB
> registers should be accessed while this bit is set, except the BLOCK DATA BYTE
> Register. The BLOCK DATA BYTE Register can be accessed when this bit is set only
> when the SMB_CMD bits in the Host Control Register are programmed for Block
> command or I2C Read command. This is necessary in order to check the
> DONE_STS bit.
> 
> Remember my case? I'm issuing plain I2C multi byte (straight I2C with
> arbitrary length) transaction, in ICH9 words SMB_CMD is set to Block
> command. Since E32B is enabled, DONE_STS is irrelevant for us in this
> case.

This is correct, assuming you mean I2C block writes and not reads. I2C
block reads are always done in byte-by-byte mode (E32B not set).

> As I understand, in this case we should relay on interrupts and
> not on polling, as both: E32B and Block (write) command are enabled.
>
> That is why in my case I'm seeing timeout > MAX_TIMEOUT.

I fail to see any relation between using interrupts and transaction
types. The i2c-i801 driver does not use interrupts at all at the
moment, it is always polling.

> As an alternative, I could try without E32B?
> 
> What do you think?

You could try this, yes. In fact, it might be a good idea to add a
module parameter to the i2c-i801 driver to let the user disable
features (PEC, block buffer and I2C block read) to use alternative code
paths in case the advanced ones don't work for some reason.

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Felix Rubinstein]

Hi Jean,


On Sun, Feb 28, 2010 at 1:08 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi Felix,
>
> Please keep the linux-i2c list in Cc.
>
> On Wed, 24 Feb 2010 01:21:51 +0200, Felix Rubinstein wrote:
> > On Mon, Feb 22, 2010 at 11:58 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > > Unlikely. If a timeout occurred, the error message would say so.
> > >
> > > What is the exact error message? Can we see your complete code?
> >
> > Sure, I'm not at the office right now, but will post it asap.
> >
> > But hey, I think I've found an issue here.
> > Let's delve into the i801 driver code for a moment please.
> >
> > in i801_transaction:
> > ...
> > /* We will always wait for a fraction of a second! */
> >          do {
> >                  msleep(1);
> >                  status = inb_p(SMBHSTSTS);
> >          } while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_TIMEOUT));
> > ...
> >
> > The data sheet states for HST_STS reg for HOST_BUSY bit:
> > 1 = Indicates that the ICH9 is running a command from the host interface. No SMB
> > registers should be accessed while this bit is set, except the BLOCK DATA BYTE
> > Register. The BLOCK DATA BYTE Register can be accessed when this bit is set only
> > when the SMB_CMD bits in the Host Control Register are programmed for Block
> > command or I2C Read command. This is necessary in order to check the
> > DONE_STS bit.
> >
> > Remember my case? I'm issuing plain I2C multi byte (straight I2C with
> > arbitrary length) transaction, in ICH9 words SMB_CMD is set to Block
> > command. Since E32B is enabled, DONE_STS is irrelevant for us in this
> > case.
>
> This is correct, assuming you mean I2C block writes and not reads. I2C
> block reads are always done in byte-by-byte mode (E32B not set).
What I mean was that HOST_BUSY bit is relevant during the transaction
in case E32B is _not_ set (since DONE_STS is irrelevant once E32B is
set).
>
> > As I understand, in this case we should relay on interrupts and
> > not on polling, as both: E32B and Block (write) command are enabled.
> >
> > That is why in my case I'm seeing timeout > MAX_TIMEOUT.
>
> I fail to see any relation between using interrupts and transaction
> types. The i2c-i801 driver does not use interrupts at all at the
> moment, it is always polling.
Meaning we cannot relay on HOST_BUSY bit when we _both_ issue Block
transaction and E32B is set. Since we cannot relay on it, the only
option left is interrupts.
What I'm trying to say is that we cannot relay (and in my case I get
transaction timeout if E32B is set) on HOST_BUSY bit when E32B is
enabled.

BTW, I've posted my code in the previous email posted on the list.

Thanks,
Felix R.

>
> > As an alternative, I could try without E32B?
> >
> > What do you think?
>
> You could try this, yes. In fact, it might be a good idea to add a
> module parameter to the i2c-i801 driver to let the user disable
> features (PEC, block buffer and I2C block read) to use alternative code
> paths in case the advanced ones don't work for some reason.
>
> --
> Jean Delvare
> http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Jean Delvare]

On Sun, 28 Feb 2010 15:45:38 +0200, Felix Rubinstein wrote:
> On Sun, Feb 28, 2010 at 1:08 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Wed, 24 Feb 2010 01:21:51 +0200, Felix Rubinstein wrote:
> > > But hey, I think I've found an issue here.
> > > Let's delve into the i801 driver code for a moment please.
> > >
> > > in i801_transaction:
> > > ...
> > > /* We will always wait for a fraction of a second! */
> > >          do {
> > >                  msleep(1);
> > >                  status = inb_p(SMBHSTSTS);
> > >          } while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_TIMEOUT));
> > > ...
> > >
> > > The data sheet states for HST_STS reg for HOST_BUSY bit:
> > > 1 = Indicates that the ICH9 is running a command from the host interface. No SMB
> > > registers should be accessed while this bit is set, except the BLOCK DATA BYTE
> > > Register. The BLOCK DATA BYTE Register can be accessed when this bit is set only
> > > when the SMB_CMD bits in the Host Control Register are programmed for Block
> > > command or I2C Read command. This is necessary in order to check the
> > > DONE_STS bit.
> > >
> > > Remember my case? I'm issuing plain I2C multi byte (straight I2C with
> > > arbitrary length) transaction, in ICH9 words SMB_CMD is set to Block
> > > command. Since E32B is enabled, DONE_STS is irrelevant for us in this
> > > case.
> >
> > This is correct, assuming you mean I2C block writes and not reads. I2C
> > block reads are always done in byte-by-byte mode (E32B not set).
> What I mean was that HOST_BUSY bit is relevant during the transaction
> in case E32B is _not_ set (since DONE_STS is irrelevant once E32B is
> set).
> >
> > > As I understand, in this case we should relay on interrupts and
> > > not on polling, as both: E32B and Block (write) command are enabled.
> > >
> > > That is why in my case I'm seeing timeout > MAX_TIMEOUT.
> >
> > I fail to see any relation between using interrupts and transaction
> > types. The i2c-i801 driver does not use interrupts at all at the
> > moment, it is always polling.
> Meaning we cannot relay on HOST_BUSY bit when we _both_ issue Block
> transaction and E32B is set. Since we cannot relay on it, the only
> option left is interrupts.

Please point me to the part of the datasheet which says this.

> What I'm trying to say is that we cannot relay (and in my case I get
> transaction timeout if E32B is set) on HOST_BUSY bit when E32B is
> enabled.

And I think you are wrong. The HOST_BUSY bit is perfectly valid for
block transactions under E32B. It wears off at the end of the block
transaction, as it does for every other transaction. I can't think of
any reason why we couldn't use it.

> BTW, I've posted my code in the previous email posted on the list.

I've seen this and will comment on it as my time permits.

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Felix Rubinstein]

Hi Jean,

On Sun, Feb 28, 2010 at 10:19 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>
> On Sun, 28 Feb 2010 15:45:38 +0200, Felix Rubinstein wrote:
> > On Sun, Feb 28, 2010 at 1:08 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > > On Wed, 24 Feb 2010 01:21:51 +0200, Felix Rubinstein wrote:
> > > > But hey, I think I've found an issue here.
> > > > Let's delve into the i801 driver code for a moment please.
> > > >
> > > > in i801_transaction:
> > > > ...
> > > > /* We will always wait for a fraction of a second! */
> > > >          do {
> > > >                  msleep(1);
> > > >                  status = inb_p(SMBHSTSTS);
> > > >          } while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_TIMEOUT));
> > > > ...
> > > >
> > > > The data sheet states for HST_STS reg for HOST_BUSY bit:
> > > > 1 = Indicates that the ICH9 is running a command from the host interface. No SMB
> > > > registers should be accessed while this bit is set, except the BLOCK DATA BYTE
> > > > Register. The BLOCK DATA BYTE Register can be accessed when this bit is set only
> > > > when the SMB_CMD bits in the Host Control Register are programmed for Block
> > > > command or I2C Read command. This is necessary in order to check the
> > > > DONE_STS bit.
> > > >
> > > > Remember my case? I'm issuing plain I2C multi byte (straight I2C with
> > > > arbitrary length) transaction, in ICH9 words SMB_CMD is set to Block
> > > > command. Since E32B is enabled, DONE_STS is irrelevant for us in this
> > > > case.
> > >
> > > This is correct, assuming you mean I2C block writes and not reads. I2C
> > > block reads are always done in byte-by-byte mode (E32B not set).
> > What I mean was that HOST_BUSY bit is relevant during the transaction
> > in case E32B is _not_ set (since DONE_STS is irrelevant once E32B is
> > set).
> > >
> > > > As I understand, in this case we should relay on interrupts and
> > > > not on polling, as both: E32B and Block (write) command are enabled.
> > > >
> > > > That is why in my case I'm seeing timeout > MAX_TIMEOUT.
> > >
> > > I fail to see any relation between using interrupts and transaction
> > > types. The i2c-i801 driver does not use interrupts at all at the
> > > moment, it is always polling.
> > Meaning we cannot relay on HOST_BUSY bit when we _both_ issue Block
> > transaction and E32B is set. Since we cannot relay on it, the only
> > option left is interrupts.
>
> Please point me to the part of the datasheet which says this.

Well, this part is quite vague and it's my interpretation however data
sheet states that during polling for HOST_BUSY bit, other registers
could be accesses iff we're executing Block transaction (my case) ...
and I quote: "This is necessary in order to check the DONE_STS bit."
But on the other hand DONE_STS bit, once again quoting : "... has no
meaning for block transfers when the 32-byte buffer is enabled."
In my case I fail to issue I2C write Block transaction iff E32B is
enabled and succeed if it's disabled.

One more thing which popped up recently.
If I work with E32B disabled, I paid attention that msleep(1) in
i801_block_transaction_byte_by_byte makes the write transaction too
long in the scope, i.e. bytes frequency is too long and they are too
far apart which makes them harder to be read. I think they should be
closer together, like this one.

                /* We will always wait for a fraction of a second! */
                timeout = 0;
                do {
-                       msleep(1);
+                      udelay(100);
                        status = inb_p(SMBHSTSTS);
                }
                while ((!(status & SMBHSTSTS_BYTE_DONE))
                          && (timeout++ < MAX_TIMEOUT));

or something like this.Even if it will timeout for a few times,
MAX_TIMEOUT will take care of the rest. In my case it hasn't even
once. All this, of course, not just for scope :)

Thanks,
Felix R.

>
> > What I'm trying to say is that we cannot relay (and in my case I get
> > transaction timeout if E32B is set) on HOST_BUSY bit when E32B is
> > enabled.
>
> And I think you are wrong. The HOST_BUSY bit is perfectly valid for
> block transactions under E32B. It wears off at the end of the block
> transaction, as it does for every other transaction. I can't think of
> any reason why we couldn't use it.
>
> > BTW, I've posted my code in the previous email posted on the list.
>
> I've seen this and will comment on it as my time permits.
>
> --
> Jean Delvare
> http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Jean Delvare]

Hi Felix,

On Wed, 24 Feb 2010 14:01:56 +0200, Felix Rubinstein wrote:
> Here is my code:
> 
> ------------
> #include <stdio.h>
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <stdlib.h>
> 
> #include "i2c-dev.h"
> #include "i2cbusses.h"
> #include "util.h"
> 
> /* actually smbus allows up to 32 and i2c even more */
> #define I2CRWL_MAX_PARAMS 10
> #define I2CRWL_PARAMS_SHIFT 3
> 
> static int i2c_writel(int fd, int datac, char *datav[])
> {
>         int i;
>         unsigned char buf[I2CRWL_MAX_PARAMS];
>         unsigned int data;
> 
>         for (i = 0; i < datac && i < I2CRWL_MAX_PARAMS; i++) {
>                 sscanf(datav[i], "%x", &data);
>                 buf[i] = (unsigned char)data;
>         }
> 
>         if (i2c_smbus_write_i2c_block_data(fd, buf[0], datac - 1,
> &buf[1]) < 0) {
>                 perror("\n");

You'd rather use:

		perror("i2c_smbus_write_i2c_block_data");

so that error messages are clearer.

>                 return 1;
>         }
> 
>         return 0;
> }
> 
> 
> static void help(const char *progname)
> {
>         fprintf(stderr,
>                         "Usage: %s I2CBUS CHIP-ADDRESS DATA0 [DATA1
> ... DATAn]\n"
>                         "  I2CBUS is an integer or an I2C bus name\n"
>                         "  CHIP-ADDRESS is an integer (0x03 - 0x77)\n"
>                         "  DATAx is data to be written to the chip,
> where 0 <= x <= n\n\n", progname);
>         exit(1);
> }
> 
> int main(int argc, char *argv[])
> {
>         int fd, i2cbus, addr, ret = 0;
>         char filename[20];
> 
>         if ((argc < I2CRWL_PARAMS_SHIFT + 1) || (I2CRWL_MAX_PARAMS +
> I2CRWL_PARAMS_SHIFT < argc))
>                 help(argv[0]);
> 
>         i2cbus = lookup_i2c_bus(argv[1]);
>         if (i2cbus < 0)
>                 help(argv[0]);
> 
>         addr = parse_i2c_address(argv[2]);
>         if (addr < 0)
>                 help(argv[0]);
> 
>         fd = open_i2c_dev(i2cbus, filename, 0);
>         if (fd < 0)
>                 exit(1);
> 
>         if (ioctl(fd, I2C_SLAVE, addr) < 0) {
>                 ret = 1;
>                 perror("");

Same here, perror("ioctl(I2C_SLAVE)") or similar.

>                 goto out;
>         }
> 
> 
>         if (i2c_writel(fd, argc - I2CRWL_PARAMS_SHIFT,
> &argv[I2CRWL_PARAMS_SHIFT])) {
>                 ret = 1;
>                 goto out;
>         }
> 
> 
> out:
>         close(fd);
> 
>         return ret;
> }
> ------------
> 
> BTW, I've disabled the FEATURE_BLOCK_BUFFER
> 
> --- i2c-i801.c     2010-02-24 10:50:50.060209638 +0200
> +++ i2c-i801.c.orig    2010-02-24 13:55:29.664070673 +0200
> @@ -603,7 +603,6 @@
>                 /* fall through */
>         case PCI_DEVICE_ID_INTEL_82801DB_3:
>                 i801_features |= FEATURE_SMBUS_PEC;
> -               i801_features |= FEATURE_BLOCK_BUFFER;
>                 break;
>         }
> 
> and now everything works smoothly. I2C write transaction of arbitrary
> length are seen even by scope :)
> 
> In case if I don't, here is what I get:
> 
> $ dmesg | tail
> Transaction timeout
> Terminating the current operation
> Failed terminating the transaction
> Failed clearing status flags at end of transaction ...

Obviously, if disabling the block buffer makes the same transaction
work, then it has to be a bug in the driver. And the good news is: I
was able to reproduce the bug using your test program, on an ICH5
running kernel 2.6.27.45. On the same system, I can get SMBus block
reads to work with or without the block buffer, so block buffer support
is not entirely broken (if it was, we'd certainly have noticed earlier.)

Now ideally we need to figure out whether SMBus block writes are
affected as well. We already know that SMBus block reads are not, and
I2C block writes are. As I2C block reads are excluded (the block buffer
can not be used for them according to the datasheet, and the driver
already does the right thing), checking whether SMBus block writes are
affected will tell us whether all block writes are affected, or if I2C
block writes only are affected. This should help us find out where and
what the bug could be.

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Felix Rubinstein]

Hi Jean,

On Tue, Mar 2, 2010 at 11:22 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi Felix,
>
> On Wed, 24 Feb 2010 14:01:56 +0200, Felix Rubinstein wrote:
> > Here is my code:
> >
> > ------------
> > #include <stdio.h>
> > #include <unistd.h>
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <fcntl.h>
> > #include <sys/ioctl.h>
> > #include <stdlib.h>
> >
> > #include "i2c-dev.h"
> > #include "i2cbusses.h"
> > #include "util.h"
> >
> > /* actually smbus allows up to 32 and i2c even more */
> > #define I2CRWL_MAX_PARAMS 10
> > #define I2CRWL_PARAMS_SHIFT 3
> >
> > static int i2c_writel(int fd, int datac, char *datav[])
> > {
> >         int i;
> >         unsigned char buf[I2CRWL_MAX_PARAMS];
> >         unsigned int data;
> >
> >         for (i = 0; i < datac && i < I2CRWL_MAX_PARAMS; i++) {
> >                 sscanf(datav[i], "%x", &data);
> >                 buf[i] = (unsigned char)data;
> >         }
> >
> >         if (i2c_smbus_write_i2c_block_data(fd, buf[0], datac - 1,
> > &buf[1]) < 0) {
> >                 perror("\n");
>
> You'd rather use:
>
>                perror("i2c_smbus_write_i2c_block_data");
>
> so that error messages are clearer.
>
> >                 return 1;
> >         }
> >
> >         return 0;
> > }
> >
> >
> > static void help(const char *progname)
> > {
> >         fprintf(stderr,
> >                         "Usage: %s I2CBUS CHIP-ADDRESS DATA0 [DATA1
> > ... DATAn]\n"
> >                         "  I2CBUS is an integer or an I2C bus name\n"
> >                         "  CHIP-ADDRESS is an integer (0x03 - 0x77)\n"
> >                         "  DATAx is data to be written to the chip,
> > where 0 <= x <= n\n\n", progname);
> >         exit(1);
> > }
> >
> > int main(int argc, char *argv[])
> > {
> >         int fd, i2cbus, addr, ret = 0;
> >         char filename[20];
> >
> >         if ((argc < I2CRWL_PARAMS_SHIFT + 1) || (I2CRWL_MAX_PARAMS +
> > I2CRWL_PARAMS_SHIFT < argc))
> >                 help(argv[0]);
> >
> >         i2cbus = lookup_i2c_bus(argv[1]);
> >         if (i2cbus < 0)
> >                 help(argv[0]);
> >
> >         addr = parse_i2c_address(argv[2]);
> >         if (addr < 0)
> >                 help(argv[0]);
> >
> >         fd = open_i2c_dev(i2cbus, filename, 0);
> >         if (fd < 0)
> >                 exit(1);
> >
> >         if (ioctl(fd, I2C_SLAVE, addr) < 0) {
> >                 ret = 1;
> >                 perror("");
>
> Same here, perror("ioctl(I2C_SLAVE)") or similar.
>
> >                 goto out;
> >         }
> >
> >
> >         if (i2c_writel(fd, argc - I2CRWL_PARAMS_SHIFT,
> > &argv[I2CRWL_PARAMS_SHIFT])) {
> >                 ret = 1;
> >                 goto out;
> >         }
> >
> >
> > out:
> >         close(fd);
> >
> >         return ret;
> > }
> > ------------
> >
> > BTW, I've disabled the FEATURE_BLOCK_BUFFER
> >
> > --- i2c-i801.c     2010-02-24 10:50:50.060209638 +0200
> > +++ i2c-i801.c.orig    2010-02-24 13:55:29.664070673 +0200
> > @@ -603,7 +603,6 @@
> >                 /* fall through */
> >         case PCI_DEVICE_ID_INTEL_82801DB_3:
> >                 i801_features |= FEATURE_SMBUS_PEC;
> > -               i801_features |= FEATURE_BLOCK_BUFFER;
> >                 break;
> >         }
> >
> > and now everything works smoothly. I2C write transaction of arbitrary
> > length are seen even by scope :)
> >
> > In case if I don't, here is what I get:
> >
> > $ dmesg | tail
> > Transaction timeout
> > Terminating the current operation
> > Failed terminating the transaction
> > Failed clearing status flags at end of transaction ...
>
> Obviously, if disabling the block buffer makes the same transaction
> work, then it has to be a bug in the driver. And the good news is: I
> was able to reproduce the bug using your test program, on an ICH5
> running kernel 2.6.27.45. On the same system, I can get SMBus block
> reads to work with or without the block buffer, so block buffer support
> is not entirely broken (if it was, we'd certainly have noticed earlier.)
>
> Now ideally we need to figure out whether SMBus block writes are
> affected as well. We already know that SMBus block reads are not, and
> I2C block writes are. As I2C block reads are excluded (the block buffer
> can not be used for them according to the datasheet, and the driver
> already does the right thing), checking whether SMBus block writes are
> affected will tell us whether all block writes are affected, or if I2C
> block writes only are affected. This should help us find out where and
> what the bug could be.

Ok, here is what I did.
I run the code with  i2c_smbus_write_block_data with E32B enabled,
i.e. the original version of the driver and SMBus multi-byte write
transactions were successful (I also verified it against the scope
analyzer).

But what I run the code with i2c_smbus_write_i2c_block_data, here is what I got:

# ./i2c_wl 0 0x28 0x1 0xff 0xff

i2c_smbus_write_i2c_block_data: Operation not permitted
l# ./i2c_wl 0 0x28 0x1 0xff 0xff 0xff 0xff
Success ...
# dmesg | tail
[  430.580131] i801_smbus 0000:00:1f.3: Transaction (post): CNT=14,
CMD=01, ADD=50, DAT0=02, DAT1=00
[  430.580312] i2c-adapter i2c-0: ioctl, cmd=0x708, arg=0x01
[  464.280155] i2c-adapter i2c-0: ioctl, cmd=0x703, arg=0x28
[  464.280155] i2c-adapter i2c-0: ioctl, cmd=0x708, arg=0x00
[  464.280155] i2c-adapter i2c-0: ioctl, cmd=0x720, arg=0xbfdaa63c
[  464.280155] i801_smbus 0000:00:1f.3: Transaction (pre): CNT=14,
CMD=01, ADD=50, DAT0=04, DAT1=00
[  464.280155] i801_smbus 0000:00:1f.3: SMBus busy (14). Resetting...
[  464.280155] i801_smbus 0000:00:1f.3: Successful!
[  464.287033] i801_smbus 0000:00:1f.3: Transaction (post): CNT=14,
CMD=01, ADD=50, DAT0=04, DAT1=00
[  464.287080] i2c-adapter i2c-0: ioctl, cmd=0x708, arg=0x01

In addition, the scope analyzer showed me that (when I run this
command ./i2c_wl 0 0x28 0x1 0xff 0xff) the last byte 0xff wasn't sent
on the bus. In contrary, every byte was on the bus with
i2c_smbus_write_block_data.

Thanks,
Felix R.

>
> --
> Jean Delvare
> http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Jean Delvare]

Hi again Felix,

On Tue, 2 Mar 2010 22:22:03 +0100, Jean Delvare wrote:
> Obviously, if disabling the block buffer makes the same transaction
> work, then it has to be a bug in the driver. And the good news is: I
> was able to reproduce the bug using your test program, on an ICH5
> running kernel 2.6.27.45. On the same system, I can get SMBus block
> reads to work with or without the block buffer, so block buffer support
> is not entirely broken (if it was, we'd certainly have noticed earlier.)
> 
> Now ideally we need to figure out whether SMBus block writes are
> affected as well. We already know that SMBus block reads are not, and
> I2C block writes are. As I2C block reads are excluded (the block buffer
> can not be used for them according to the datasheet, and the driver
> already does the right thing), checking whether SMBus block writes are
> affected will tell us whether all block writes are affected, or if I2C
> block writes only are affected. This should help us find out where and
> what the bug could be.

Further testing today shows that SMBus block writes work just fine with
the block buffer feature enabled. So, the only problem is with I2C
block writes. My last attempt locked the SMBus, but I was able to
recover by repeatedly writing to the HST_STS register, as may times as
the block length. That is, the device operates in byte-by-byte mode
regardless of the block buffer mode being set. This leads me to the
conclusion that the E32B flag is ignored for I2C block write
transactions, exactly as it is for I2C block read transactions in my
experience.

Digging up a mailing list post dating to when I added support for I2C
block read support [1], I found the following statement of mine which is
certainly relevant:

"Note that I could not get the I2C block read to work with the block
buffer enabled, so for now it is implemented with the block buffer
disabled, which means that the performance isn't that good. I couldn't
find anything in the ICH datasheets suggesting that the two features
are mutually exclusive, so maybe I did something wrong, or maybe the
hardware really can't do it and it should be documented."

This was about I2C block reads, not writes, but given that the
datasheet doesn't mention anything about a possible incompatibility
between I2C block transactions and the use of the block buffer anyway,
what seems to apply to I2C block reads in practice might reasonably
apply to I2C block writes as well. So, at this point, I am inclined to
simply disable the block buffer for I2C block writes as we already do
for I2C block reads. This should be a rather simple fix, I'll post a
patch later today or tomorrow.

That being said, I can't exclude that I missed something and block
buffer support could work with I2C block transactions with some more
work, maybe using a trick not mentioned in the datasheet. If someone
can get it to work, I would be very grateful, as the block buffer is a
great feature.

While working on this issue, I noticed that the piece of code which is
supposed to let the i2c-i801 driver recover in case of a transaction
timeout, did not always work. I will try to improve it to correctly
handle the bug you reported, before we fix that bug. Hopefully it will
help recovery from other cases as well in the future.

[1] http://marc.info/?l=linux-i2c&m=119780045620870&w=2

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: Intel ICHx bus driver

[Jean Delvare]

Hi Felix,

On Tue, 2 Mar 2010 14:53:41 +0200, Felix Rubinstein wrote:
> One more thing which popped up recently.
> If I work with E32B disabled, I paid attention that msleep(1) in
> i801_block_transaction_byte_by_byte makes the write transaction too
> long in the scope, i.e. bytes frequency is too long and they are too
> far apart which makes them harder to be read. I think they should be
> closer together, like this one.
> 
>                 /* We will always wait for a fraction of a second! */
>                 timeout = 0;
>                 do {
> -                       msleep(1);
> +                      udelay(100);
>                         status = inb_p(SMBHSTSTS);
>                 }
>                 while ((!(status & SMBHSTSTS_BYTE_DONE))
>                           && (timeout++ < MAX_TIMEOUT));
> 
> or something like this.Even if it will timeout for a few times,
> MAX_TIMEOUT will take care of the rest. In my case it hasn't even
> once. All this, of course, not just for scope :)

This is an interesting point. During simple transactions, or block
transactions under E32B, we don't really care about the delay, because
the controller takes care of the transaction on its own, and the only
thing the delay affects is how long the user waits for the answer. For
block transactions without E32B, it's different because the bus is
really waiting for us to trigger the sending or receiving of the next
data byte, so if we wait too long, we keep the bus busy for a long
time, which is bad both for multi-master buses, and when talking to a
slave which stops data processing while an I2C transaction is in
progress. I presume that some masters or slaves would even consider
that a transaction is stuck if it waits too long without sending
data. The SMBus specification defines the hardware timeout as 25 ms.

The SMBus specification defines Tlow:mext as the maximum delay between
data bytes on the wire, with value 10 ms. While msleep(1) at HZ=1000
should be fine in this respect (it will sleep 2 ms max), and even at
HS=250 (it will sleep 8 ms max), it is not OK at HZ=100, as msleep(1)
may sleep up to 20 ms. Fortunately this is still below the hardware
timeout of 25 ms. msleep(0) may be better, but I'm not sure if it is
semantically acceptable (only one driver is using it so far). So maybe
we should indeed not sleep at this point. That being said, your naive
patch is hardly acceptable as is, as it may lead us to busy-wait for
100 * 100 us = 10 ms. It also shortens the timeout from 200 ms (at
HZ=1000) to 10 ms, which I think is too short.

If you have observed the ICH9 SMBus on the scope, can you tell me at
which frequency it is operating? If udelay(100) was always sufficient,
this suggests a bus frequency of at least 90 kHz, which is much faster
than I expected...

-- 
Jean Delvare

Re: Intel ICHx bus driver

[Jean Delvare]

On Fri, 12 Mar 2010 14:19:01 +0100, Jean Delvare wrote:
> This is an interesting point. During simple transactions, or block
> transactions under E32B, we don't really care about the delay, because
> the controller takes care of the transaction on its own, and the only
> thing the delay affects is how long the user waits for the answer. For
> block transactions without E32B, it's different because the bus is
> really waiting for us to trigger the sending or receiving of the next
> data byte, so if we wait too long, we keep the bus busy for a long
> time, which is bad both for multi-master buses, and when talking to a
> slave which stops data processing while an I2C transaction is in
> progress. I presume that some masters or slaves would even consider
> that a transaction is stuck if it waits too long without sending
> data. The SMBus specification defines the hardware timeout as 25 ms.
> 
> The SMBus specification defines Tlow:mext as the maximum delay between
> data bytes on the wire, with value 10 ms. While msleep(1) at HZ=1000
> should be fine in this respect (it will sleep 2 ms max), and even at
> HS=250 (it will sleep 8 ms max), it is not OK at HZ=100, as msleep(1)
> may sleep up to 20 ms. Fortunately this is still below the hardware
> timeout of 25 ms. msleep(0) may be better, but I'm not sure if it is
> semantically acceptable (only one driver is using it so far). So maybe
> we should indeed not sleep at this point. That being said, your naive
> patch is hardly acceptable as is, as it may lead us to busy-wait for
> 100 * 100 us = 10 ms. It also shortens the timeout from 200 ms (at
> HZ=1000) to 10 ms, which I think is too short.
> 
> If you have observed the ICH9 SMBus on the scope, can you tell me at
> which frequency it is operating? If udelay(100) was always sufficient,
> this suggests a bus frequency of at least 90 kHz, which is much faster
> than I expected...

FWIW, testing on my ICH3-M (SMBus block read), I get a delay between
bytes of 460 us. This is a maximum bus speed of 19.6 kHz. Same test on
an ICH5 reports a delay of 670 us, which would be 13.5 kHz max. I have
a hard time believing that you got delays below 100 us on your ICH9...

Also note that, in both cases, the first delay is always much larger,
as the controller must send the beginning of the transaction up to the
second data byte. The SMBus block read is the worst case scenario, as
it must first send an address byte, the command byte, then the address
byte again (direction change), then it reads the block length and
finally the first data byte. This is 45 bits on the wire, not counting
the start conditions. I get a ~2380 us delay on ICH3-M and ~3420 us
delay on ICH5.

So changing the code the way you suggested isn't trivial. Busy-waiting
for up to 3500 us for the first transaction isn't very appealing.
Busy-waiting for a total of 3500 + 670 * 31 (worst case) = 24270 us or
almost 25 ms, is hardly pleasant either, latency will suffer, it's
almost worse than software bit-banging. Now I have to agree that the
current implementation ("long" sleeps) isn't good either. Maybe the
msleep(0) approach would be the least evil... Oh well, at some point we
really want to switch to interrupts.

-- 
Jean Delvare

Re: Intel ICHx bus driver

[Felix Rubinstein]

Hi Jean,

On Fri, Mar 12, 2010 at 6:24 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Fri, 12 Mar 2010 14:19:01 +0100, Jean Delvare wrote:
>> This is an interesting point. During simple transactions, or block
>> transactions under E32B, we don't really care about the delay, because
>> the controller takes care of the transaction on its own, and the only
>> thing the delay affects is how long the user waits for the answer. For
>> block transactions without E32B, it's different because the bus is
>> really waiting for us to trigger the sending or receiving of the next
>> data byte, so if we wait too long, we keep the bus busy for a long
>> time, which is bad both for multi-master buses, and when talking to a
>> slave which stops data processing while an I2C transaction is in
>> progress. I presume that some masters or slaves would even consider
>> that a transaction is stuck if it waits too long without sending
>> data. The SMBus specification defines the hardware timeout as 25 ms.
>>
>> The SMBus specification defines Tlow:mext as the maximum delay between
>> data bytes on the wire, with value 10 ms. While msleep(1) at HZ=1000
>> should be fine in this respect (it will sleep 2 ms max), and even at
>> HS=250 (it will sleep 8 ms max), it is not OK at HZ=100, as msleep(1)
>> may sleep up to 20 ms. Fortunately this is still below the hardware
>> timeout of 25 ms. msleep(0) may be better, but I'm not sure if it is
>> semantically acceptable (only one driver is using it so far). So maybe
>> we should indeed not sleep at this point. That being said, your naive
>> patch is hardly acceptable as is, as it may lead us to busy-wait for
>> 100 * 100 us = 10 ms. It also shortens the timeout from 200 ms (at
>> HZ=1000) to 10 ms, which I think is too short.
>>
>> If you have observed the ICH9 SMBus on the scope, can you tell me at
>> which frequency it is operating? If udelay(100) was always sufficient,
>> this suggests a bus frequency of at least 90 kHz, which is much faster
>> than I expected...
>
> FWIW, testing on my ICH3-M (SMBus block read), I get a delay between
> bytes of 460 us. This is a maximum bus speed of 19.6 kHz. Same test on
> an ICH5 reports a delay of 670 us, which would be 13.5 kHz max. I have
> a hard time believing that you got delays below 100 us on your ICH9...
>
> Also note that, in both cases, the first delay is always much larger,
> as the controller must send the beginning of the transaction up to the
> second data byte. The SMBus block read is the worst case scenario, as
> it must first send an address byte, the command byte, then the address
> byte again (direction change), then it reads the block length and
> finally the first data byte. This is 45 bits on the wire, not counting
> the start conditions. I get a ~2380 us delay on ICH3-M and ~3420 us
> delay on ICH5.
>
> So changing the code the way you suggested isn't trivial. Busy-waiting
> for up to 3500 us for the first transaction isn't very appealing.
> Busy-waiting for a total of 3500 + 670 * 31 (worst case) = 24270 us or
> almost 25 ms, is hardly pleasant either, latency will suffer, it's
> almost worse than software bit-banging. Now I have to agree that the
> current implementation ("long" sleeps) isn't good either. Maybe the
> msleep(0) approach would be the least evil... Oh well, at some point we
> really want to switch to interrupts.
Thanks for prompt reply :)
I'll provided more number later on, once I get to it in the lab.
I have it in my mind to dig into ICH9 data sheet (for the start) to
add interrupt support.
Btw, have anyone started it?

Thanks,
Felix R.

Re: Intel ICHx bus driver

[Jean Delvare]

On Mon, 15 Mar 2010 11:43:48 +0200, Felix Rubinstein wrote:
> Hi Jean,
> 
> On Fri, Mar 12, 2010 at 6:24 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > FWIW, testing on my ICH3-M (SMBus block read), I get a delay between
> > bytes of 460 us. This is a maximum bus speed of 19.6 kHz. Same test on
> > an ICH5 reports a delay of 670 us, which would be 13.5 kHz max. I have
> > a hard time believing that you got delays below 100 us on your ICH9...
> >
> > Also note that, in both cases, the first delay is always much larger,
> > as the controller must send the beginning of the transaction up to the
> > second data byte. The SMBus block read is the worst case scenario, as
> > it must first send an address byte, the command byte, then the address
> > byte again (direction change), then it reads the block length and
> > finally the first data byte. This is 45 bits on the wire, not counting
> > the start conditions. I get a ~2380 us delay on ICH3-M and ~3420 us
> > delay on ICH5.
> >
> > So changing the code the way you suggested isn't trivial. Busy-waiting
> > for up to 3500 us for the first transaction isn't very appealing.
> > Busy-waiting for a total of 3500 + 670 * 31 (worst case) = 24270 us or
> > almost 25 ms, is hardly pleasant either, latency will suffer, it's
> > almost worse than software bit-banging. Now I have to agree that the
> > current implementation ("long" sleeps) isn't good either. Maybe the
> > msleep(0) approach would be the least evil... Oh well, at some point we
> > really want to switch to interrupts.
>
> Thanks for prompt reply :)
> I'll provided more number later on, once I get to it in the lab.
> I have it in my mind to dig into ICH9 data sheet (for the start) to
> add interrupt support.
> Btw, have anyone started it?

Oh yes, twice. Mark M. Hoffman tried first, with a brand new driver.
Then Ivo Manca (Cc'd) gave it a try, this time modifying the existing
driver.

The problem with Mark's implementation was that it lacked support for
block transactions, so we couldn't replace the original driver with
his. We did not want to maintain two drivers in parallel, and block
support was never added to the new driver, so Mark's driver did not go
anywhere.

The problem with Ivo's work was that the driver would lock up the bus in
some cases (I can't remember the details any longer, sorry) and we did
not manage to solve this bug. This issue blocked further integration,
and once again the efforts were lost. I can provide Ivo's latest
patches if you want to see what they looked like. With some work, it
should be possible to get them to apply again. I would much prefer
fixing Ivo's patches than starting from scratch again.

-- 
Jean Delvare

Re: Intel ICHx bus driver

[Felix Rubinstein]

Perfect :)

Btw, you mentioned to disable block buffer for I2C block writes in
2.6.24 but I don't see any change regarding it. Is it in
git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git

Thanks,
Felix R.

On Mon, Mar 15, 2010 at 12:06 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, 15 Mar 2010 11:43:48 +0200, Felix Rubinstein wrote:
>> Hi Jean,
>>
>> On Fri, Mar 12, 2010 at 6:24 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>> > FWIW, testing on my ICH3-M (SMBus block read), I get a delay between
>> > bytes of 460 us. This is a maximum bus speed of 19.6 kHz. Same test on
>> > an ICH5 reports a delay of 670 us, which would be 13.5 kHz max. I have
>> > a hard time believing that you got delays below 100 us on your ICH9...
>> >
>> > Also note that, in both cases, the first delay is always much larger,
>> > as the controller must send the beginning of the transaction up to the
>> > second data byte. The SMBus block read is the worst case scenario, as
>> > it must first send an address byte, the command byte, then the address
>> > byte again (direction change), then it reads the block length and
>> > finally the first data byte. This is 45 bits on the wire, not counting
>> > the start conditions. I get a ~2380 us delay on ICH3-M and ~3420 us
>> > delay on ICH5.
>> >
>> > So changing the code the way you suggested isn't trivial. Busy-waiting
>> > for up to 3500 us for the first transaction isn't very appealing.
>> > Busy-waiting for a total of 3500 + 670 * 31 (worst case) = 24270 us or
>> > almost 25 ms, is hardly pleasant either, latency will suffer, it's
>> > almost worse than software bit-banging. Now I have to agree that the
>> > current implementation ("long" sleeps) isn't good either. Maybe the
>> > msleep(0) approach would be the least evil... Oh well, at some point we
>> > really want to switch to interrupts.
>>
>> Thanks for prompt reply :)
>> I'll provided more number later on, once I get to it in the lab.
>> I have it in my mind to dig into ICH9 data sheet (for the start) to
>> add interrupt support.
>> Btw, have anyone started it?
>
> Oh yes, twice. Mark M. Hoffman tried first, with a brand new driver.
> Then Ivo Manca (Cc'd) gave it a try, this time modifying the existing
> driver.
>
> The problem with Mark's implementation was that it lacked support for
> block transactions, so we couldn't replace the original driver with
> his. We did not want to maintain two drivers in parallel, and block
> support was never added to the new driver, so Mark's driver did not go
> anywhere.
>
> The problem with Ivo's work was that the driver would lock up the bus in
> some cases (I can't remember the details any longer, sorry) and we did
> not manage to solve this bug. This issue blocked further integration,
> and once again the efforts were lost. I can provide Ivo's latest
> patches if you want to see what they looked like. With some work, it
> should be possible to get them to apply again. I would much prefer
> fixing Ivo's patches than starting from scratch again.
>
> --
> Jean Delvare
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: Intel ICHx bus driver

[Jean Delvare]

On Mon, 15 Mar 2010 13:12:12 +0200, Felix Rubinstein wrote:
> Btw, you mentioned to disable block buffer for I2C block writes in
> 2.6.24 but I don't see any change regarding it. Is it in
> git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git

I guess you really mean 2.6.34 and not 2.6.24. The patch is in Linus'
tree since yesterday:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=c074c39d62306efa5ba7c69c1a1531bc7333d252

It will also be applied to the 2.6.33, 2.6.32 and 2.6.27 stable trees.

-- 
Jean Delvare

Re: Intel ICHx bus driver

[Felix Rubinstein]

Oh, right :)
Thanks!

On Mon, Mar 15, 2010 at 1:46 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, 15 Mar 2010 13:12:12 +0200, Felix Rubinstein wrote:
>> Btw, you mentioned to disable block buffer for I2C block writes in
>> 2.6.24 but I don't see any change regarding it. Is it in
>> git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git
>
> I guess you really mean 2.6.34 and not 2.6.24. The patch is in Linus'
> tree since yesterday:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=c074c39d62306efa5ba7c69c1a1531bc7333d252
>
> It will also be applied to the 2.6.33, 2.6.32 and 2.6.27 stable trees.
>
> --
> Jean Delvare
>