Re: Intel ICHx bus driver

[Felix Rubinstein <felixru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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