Re: [Qemu-devel] [PATCH 3/3] qtest: Add PIIX4 SMBus support for libqos

[Paolo Bonzini]

Il 09/06/2014 10:55, Marc Marí ha scritto:
> +static void send_and_receive(void)
> +{
> +    uint8_t i;
> +    uint8_t buf = 0;
> +    uint64_t big_buf = 0;

Probably uint32_t since you only read 4 bytes.

> +    for (i = EEPROM_TEST_ADDR; i < EEPROM_TEST_ADDR+8; ++i) {
> +        i2c_recv(i2c, i, &buf, 1);
> +        g_assert_cmphex(buf, ==, 0);
> +
> +        i2c_recv(i2c, i, (uint8_t *)&big_buf, 4);
> +        g_assert_cmphex(big_buf, ==, 0);
> +    }

Here you should be sending something before.  The protocol is that you 
send the address, then send the offset, then receive bytes.

In fact, right now you're sending a zero here:

+    /* Clear data */
+    outb(s->addr + PIIX4_SMBUS_DAT0, 0);

in libqos.

The problem is that the piix4 smbus interface includes more "magic" than 
the i2c interface that is currently part of libqos.  Basically libqos 
would have to implement the same state machine as hw/i2c/smbus.c in 
order to convert i2c commands (from the test case) to smbus commands 
(for the device).  The opposite also makes sense if you want to have an 
smbus testcase API and you want to make it talk to "basic" i2c devices.

So this testcase by itself is not really meaningful.  This is not your 
fault---I and Stefan aren't 100% comfortable with I2C either. :)  Let's 
discuss it today on the chat.

> +        while (len > 0) {
> +            size = inb(s->addr + PIIX4_SMBUS_DAT0);
> +            if (size == 0) {
> +                break;
> +            }
> +            g_assert_cmpuint((len-size), <, 0);

Asserting len < size does not make much sense.  Should you be asserting 
instead that size <= len here?  Or even len == size (without the outer 
"while (len > 0)" loop)?

> +            while (size > 0) {
> +                buf[0] = readb(s->addr + PIIX4_SMBUS_BLKDAT);
> +                ++buf;
> +                --size;
> +            }
> +
> +            len -= size;
> +        }

size is zero here; the decrement should be before the inner while loop.

Paolo