Patchwork [for-1.4] libi2c-omap: Fix endianness dependency

login
register
mail settings
Submitter Andreas Färber
Date Feb. 2, 2013, 4:45 p.m.
Message ID <1359823557-5748-1-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/217694/
State New
Headers show

Comments

Andreas Färber - Feb. 2, 2013, 4:45 p.m.
From: Andreas Färber <andreas.faerber@web.de>

The libqos driver for omap_i2c currently does not work on Big Endian.
Introduce helpers for reading from and writing to 16-bit armel registers.

This fixes tmp105-test failures on ppc.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 tests/libi2c-omap.c |   51 ++++++++++++++++++++++++++++++++-------------------
 1 Datei geändert, 32 Zeilen hinzugefügt(+), 19 Zeilen entfernt(-)
Peter Maydell - Feb. 2, 2013, 4:49 p.m.
On 2 February 2013 16:45, Andreas Färber <afaerber@suse.de> wrote:
> From: Andreas Färber <andreas.faerber@web.de>
>
> The libqos driver for omap_i2c currently does not work on Big Endian.
> Introduce helpers for reading from and writing to 16-bit armel registers.
>
> This fixes tmp105-test failures on ppc.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  tests/libi2c-omap.c |   51 ++++++++++++++++++++++++++++++++-------------------
>  1 Datei geändert, 32 Zeilen hinzugefügt(+), 19 Zeilen entfernt(-)
>
> diff --git a/tests/libi2c-omap.c b/tests/libi2c-omap.c
> index 9be57e9..7d50ef2 100644
> --- a/tests/libi2c-omap.c
> +++ b/tests/libi2c-omap.c
> @@ -12,6 +12,7 @@
>  #include <string.h>
>
>  #include "qemu/osdep.h"
> +#include "qemu/bswap.h"
>  #include "libqtest.h"
>
>  enum OMAPI2CRegisters {
> @@ -48,12 +49,24 @@ typedef struct OMAPI2C {
>  } OMAPI2C;
>
>
> +static inline void omap_i2c_read16(uint64_t addr, uint16_t *data)
> +{
> +    memread(addr, data, 2);
> +    *data = le16_to_cpu(*data);
> +}
> +
> +static inline void omap_i2c_write16(uint64_t addr, uint16_t data)
> +{
> +    data = cpu_to_le16(data);
> +    memwrite(addr, &data, 2);
> +}

There's nothing special about the OMAP i2c device that I know of:
shouldn't the test code just be using a generic "write 16 bit value
to memory with appropriate endianness for target CPU" function ?

-- PMM
Andreas Färber - Feb. 2, 2013, 5:37 p.m.
Am 02.02.2013 17:49, schrieb Peter Maydell:
> On 2 February 2013 16:45, Andreas Färber <afaerber@suse.de> wrote:
>> From: Andreas Färber <andreas.faerber@web.de>
>>
>> The libqos driver for omap_i2c currently does not work on Big Endian.
>> Introduce helpers for reading from and writing to 16-bit armel registers.
>>
>> This fixes tmp105-test failures on ppc.
>>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>>  tests/libi2c-omap.c |   51 ++++++++++++++++++++++++++++++++-------------------
>>  1 Datei geändert, 32 Zeilen hinzugefügt(+), 19 Zeilen entfernt(-)
>>
>> diff --git a/tests/libi2c-omap.c b/tests/libi2c-omap.c
>> index 9be57e9..7d50ef2 100644
>> --- a/tests/libi2c-omap.c
>> +++ b/tests/libi2c-omap.c
>> @@ -12,6 +12,7 @@
>>  #include <string.h>
>>
>>  #include "qemu/osdep.h"
>> +#include "qemu/bswap.h"
>>  #include "libqtest.h"
>>
>>  enum OMAPI2CRegisters {
>> @@ -48,12 +49,24 @@ typedef struct OMAPI2C {
>>  } OMAPI2C;
>>
>>
>> +static inline void omap_i2c_read16(uint64_t addr, uint16_t *data)
>> +{
>> +    memread(addr, data, 2);
>> +    *data = le16_to_cpu(*data);
>> +}
>> +
>> +static inline void omap_i2c_write16(uint64_t addr, uint16_t data)
>> +{
>> +    data = cpu_to_le16(data);
>> +    memwrite(addr, &data, 2);
>> +}
> 
> There's nothing special about the OMAP i2c device that I know of:
> shouldn't the test code just be using a generic "write 16 bit value
> to memory with appropriate endianness for target CPU" function ?

I asked about where to address this issue [1], no concrete answers, and
this is the only solution I came up with.

libqtest.h has no generic endian-aware memread functions unlike Alex,
you or me expected. It reads a sequence of bytes from guest memory and
transmits them one-by-one over the text-based qtest protocol. Since they
are raw bytes I need to convert them here. tmp105-test itself was
transmitting byte arrays, so it did not need changes.

Looking into where that annoying "mipid" debug output is coming from is
a bit further down on my to-do list for 1.4.

I tested this patch to resolve the test breakage on ppc and not to
introduce a regression for x86_64. I don't want arm disabled like sparc.
Any qtest API remodelling would be 1.5 material now, I guess, and could
be hidden inside my helper functions.

Andreas

[1] http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg05081.html
Peter Maydell - Feb. 2, 2013, 5:44 p.m.
On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
> Am 02.02.2013 17:49, schrieb Peter Maydell:
>> There's nothing special about the OMAP i2c device that I know of:
>> shouldn't the test code just be using a generic "write 16 bit value
>> to memory with appropriate endianness for target CPU" function ?
>
> I asked about where to address this issue [1], no concrete answers, and
> this is the only solution I came up with.
>
> libqtest.h has no generic endian-aware memread functions unlike Alex,
> you or me expected. It reads a sequence of bytes from guest memory and
> transmits them one-by-one over the text-based qtest protocol.

OK, so this is just busted for accessing devices. The protocol
has to have some way of letting you do a 32 bit / 16 bit / 8 bit
access (and maybe 64 bit as well while we're here). memread
and memwrite are OK for RAM accesses [ie anything you'd be
happy to have cached or buffered in a real system] but for
memory mapped registers we need to have an equivalent of
inb/inw/inl/outb/outw/outl that guarantee to do exactly one
access of exactly the required width.

(If you want to do a workaround for 1.4 to get the test suite
running again I don't object; this is just a discussion about
what the right long term fix should be.)

-- PMM
Andreas Färber - Feb. 2, 2013, 5:50 p.m.
Am 02.02.2013 18:44, schrieb Peter Maydell:
> On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
>> Am 02.02.2013 17:49, schrieb Peter Maydell:
>>> There's nothing special about the OMAP i2c device that I know of:
>>> shouldn't the test code just be using a generic "write 16 bit value
>>> to memory with appropriate endianness for target CPU" function ?
>>
>> I asked about where to address this issue [1], no concrete answers, and
>> this is the only solution I came up with.
>>
>> libqtest.h has no generic endian-aware memread functions unlike Alex,
>> you or me expected. It reads a sequence of bytes from guest memory and
>> transmits them one-by-one over the text-based qtest protocol.
> 
> OK, so this is just busted for accessing devices. The protocol
> has to have some way of letting you do a 32 bit / 16 bit / 8 bit
> access (and maybe 64 bit as well while we're here). memread
> and memwrite are OK for RAM accesses [ie anything you'd be
> happy to have cached or buffered in a real system] but for
> memory mapped registers we need to have an equivalent of
> inb/inw/inl/outb/outw/outl that guarantee to do exactly one
> access of exactly the required width.
> 
> (If you want to do a workaround for 1.4 to get the test suite
> running again I don't object; this is just a discussion about
> what the right long term fix should be.)

OK. As discussed with Alex in IRC, I think the issue is that libqtest
does not know about which endianness the guest uses, so the only thing
we can do on the library side is generalize my helpers (e.g.,
memread_le16u). Anything else would need to be done in qtest.c, i.e.
adding new commands to the protocol (e.g., memread16) and implementing
them on both sides.

m48t59-test, as only other memread() user, gets around this due to doing
byte access only.

Andreas
Blue Swirl - Feb. 2, 2013, 6:26 p.m.
On Sat, Feb 2, 2013 at 5:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
>> Am 02.02.2013 17:49, schrieb Peter Maydell:
>>> There's nothing special about the OMAP i2c device that I know of:
>>> shouldn't the test code just be using a generic "write 16 bit value
>>> to memory with appropriate endianness for target CPU" function ?
>>
>> I asked about where to address this issue [1], no concrete answers, and
>> this is the only solution I came up with.
>>
>> libqtest.h has no generic endian-aware memread functions unlike Alex,
>> you or me expected. It reads a sequence of bytes from guest memory and
>> transmits them one-by-one over the text-based qtest protocol.
>
> OK, so this is just busted for accessing devices. The protocol
> has to have some way of letting you do a 32 bit / 16 bit / 8 bit
> access (and maybe 64 bit as well while we're here). memread
> and memwrite are OK for RAM accesses [ie anything you'd be
> happy to have cached or buffered in a real system] but for
> memory mapped registers we need to have an equivalent of
> inb/inw/inl/outb/outw/outl that guarantee to do exactly one
> access of exactly the required width.

I was also just making a patch (but not so nice as Andreas'). My
analysis was that qtest.c and libqtest.c just pass the result of
cpu_physical_memory_rw() as is, with no endian conversion. So the
result needs to be converted to host CPU order just like Andreas did.
I think the protocol is OK, my initial reaction was to put byte
swapping there but that's not right.

>
> (If you want to do a workaround for 1.4 to get the test suite
> running again I don't object; this is just a discussion about
> what the right long term fix should be.)
>
> -- PMM
Peter Maydell - Feb. 2, 2013, 8:18 p.m.
On 2 February 2013 18:26, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sat, Feb 2, 2013 at 5:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 02.02.2013 17:49, schrieb Peter Maydell:
>>> libqtest.h has no generic endian-aware memread functions unlike Alex,
>>> you or me expected. It reads a sequence of bytes from guest memory and
>>> transmits them one-by-one over the text-based qtest protocol.
>>
>> OK, so this is just busted for accessing devices. The protocol
>> has to have some way of letting you do a 32 bit / 16 bit / 8 bit
>> access (and maybe 64 bit as well while we're here). memread
>> and memwrite are OK for RAM accesses [ie anything you'd be
>> happy to have cached or buffered in a real system] but for
>> memory mapped registers we need to have an equivalent of
>> inb/inw/inl/outb/outw/outl that guarantee to do exactly one
>> access of exactly the required width.
>
> I was also just making a patch (but not so nice as Andreas'). My
> analysis was that qtest.c and libqtest.c just pass the result of
> cpu_physical_memory_rw() as is, with no endian conversion. So the
> result needs to be converted to host CPU order just like Andreas did.
> I think the protocol is OK, my initial reaction was to put byte
> swapping there but that's not right.

No, I think the protocol is broken, even if you ignore the
question of endianness. Consider that a device's MemoryRegion
can have different behaviour depending on whether you access it
as a byte, halfword or word size. cpu_physical_memory_rw() won't
let you make a word size access to an unaligned address.

-- PMM
Blue Swirl - Feb. 2, 2013, 8:24 p.m.
On Sat, Feb 2, 2013 at 8:18 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 February 2013 18:26, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sat, Feb 2, 2013 at 5:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
>>>> Am 02.02.2013 17:49, schrieb Peter Maydell:
>>>> libqtest.h has no generic endian-aware memread functions unlike Alex,
>>>> you or me expected. It reads a sequence of bytes from guest memory and
>>>> transmits them one-by-one over the text-based qtest protocol.
>>>
>>> OK, so this is just busted for accessing devices. The protocol
>>> has to have some way of letting you do a 32 bit / 16 bit / 8 bit
>>> access (and maybe 64 bit as well while we're here). memread
>>> and memwrite are OK for RAM accesses [ie anything you'd be
>>> happy to have cached or buffered in a real system] but for
>>> memory mapped registers we need to have an equivalent of
>>> inb/inw/inl/outb/outw/outl that guarantee to do exactly one
>>> access of exactly the required width.
>>
>> I was also just making a patch (but not so nice as Andreas'). My
>> analysis was that qtest.c and libqtest.c just pass the result of
>> cpu_physical_memory_rw() as is, with no endian conversion. So the
>> result needs to be converted to host CPU order just like Andreas did.
>> I think the protocol is OK, my initial reaction was to put byte
>> swapping there but that's not right.
>
> No, I think the protocol is broken, even if you ignore the
> question of endianness. Consider that a device's MemoryRegion
> can have different behaviour depending on whether you access it
> as a byte, halfword or word size. cpu_physical_memory_rw() won't
> let you make a word size access to an unaligned address.

But the width is transmitted too, so any size accesses can be performed.

>
> -- PMM
Anthony Liguori - Feb. 4, 2013, 8:42 p.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 2 February 2013 18:26, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sat, Feb 2, 2013 at 5:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
>>>> Am 02.02.2013 17:49, schrieb Peter Maydell:
>>>> libqtest.h has no generic endian-aware memread functions unlike Alex,
>>>> you or me expected. It reads a sequence of bytes from guest memory and
>>>> transmits them one-by-one over the text-based qtest protocol.
>>>
>>> OK, so this is just busted for accessing devices. The protocol
>>> has to have some way of letting you do a 32 bit / 16 bit / 8 bit
>>> access (and maybe 64 bit as well while we're here). memread
>>> and memwrite are OK for RAM accesses [ie anything you'd be
>>> happy to have cached or buffered in a real system] but for
>>> memory mapped registers we need to have an equivalent of
>>> inb/inw/inl/outb/outw/outl that guarantee to do exactly one
>>> access of exactly the required width.
>>
>> I was also just making a patch (but not so nice as Andreas'). My
>> analysis was that qtest.c and libqtest.c just pass the result of
>> cpu_physical_memory_rw() as is, with no endian conversion. So the
>> result needs to be converted to host CPU order just like Andreas did.
>> I think the protocol is OK, my initial reaction was to put byte
>> swapping there but that's not right.
>
> No, I think the protocol is broken, even if you ignore the
> question of endianness. Consider that a device's MemoryRegion
> can have different behaviour depending on whether you access it
> as a byte, halfword or word size. cpu_physical_memory_rw() won't
> let you make a word size access to an unaligned address.

memread/memwrite is for bulk data transfer.  It's a byte API.

It would be a good idea to add word-sized I/O requests to the protocol.

Regards,

Anthony Liguori


>
> -- PMM
Anthony Liguori - Feb. 5, 2013, 4:58 p.m.
Andreas Färber <afaerber@suse.de> writes:

> From: Andreas Färber <andreas.faerber@web.de>
>
> The libqos driver for omap_i2c currently does not work on Big Endian.
> Introduce helpers for reading from and writing to 16-bit armel registers.
>
> This fixes tmp105-test failures on ppc.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  tests/libi2c-omap.c |   51 ++++++++++++++++++++++++++++++++-------------------
>  1 Datei geändert, 32 Zeilen hinzugefügt(+), 19 Zeilen entfernt(-)
>
> diff --git a/tests/libi2c-omap.c b/tests/libi2c-omap.c
> index 9be57e9..7d50ef2 100644
> --- a/tests/libi2c-omap.c
> +++ b/tests/libi2c-omap.c
> @@ -12,6 +12,7 @@
>  #include <string.h>
>  
>  #include "qemu/osdep.h"
> +#include "qemu/bswap.h"
>  #include "libqtest.h"
>  
>  enum OMAPI2CRegisters {
> @@ -48,12 +49,24 @@ typedef struct OMAPI2C {
>  } OMAPI2C;
>  
>  
> +static inline void omap_i2c_read16(uint64_t addr, uint16_t *data)
> +{
> +    memread(addr, data, 2);
> +    *data = le16_to_cpu(*data);
> +}
> +
> +static inline void omap_i2c_write16(uint64_t addr, uint16_t data)
> +{
> +    data = cpu_to_le16(data);
> +    memwrite(addr, &data, 2);
> +}
> +
>  static void omap_i2c_set_slave_addr(OMAPI2C *s, uint8_t addr)
>  {
>      uint16_t data = addr;
>  
> -    memwrite(s->addr + OMAP_I2C_SA, &data, 2);
> -    memread(s->addr + OMAP_I2C_SA, &data, 2);
> +    omap_i2c_write16(s->addr + OMAP_I2C_SA, data);
> +    omap_i2c_read16(s->addr + OMAP_I2C_SA, &data);
>      g_assert_cmphex(data, ==, addr);
>  }
>  
> @@ -66,22 +79,22 @@ static void omap_i2c_send(I2CAdapter *i2c, uint8_t addr,
>      omap_i2c_set_slave_addr(s, addr);
>  
>      data = len;
> -    memwrite(s->addr + OMAP_I2C_CNT, &data, 2);
> +    omap_i2c_write16(s->addr + OMAP_I2C_CNT, data);
>  
>      data = OMAP_I2C_CON_I2C_EN |
>             OMAP_I2C_CON_TRX |
>             OMAP_I2C_CON_MST |
>             OMAP_I2C_CON_STT |
>             OMAP_I2C_CON_STP;
> -    memwrite(s->addr + OMAP_I2C_CON, &data, 2);
> -    memread(s->addr + OMAP_I2C_CON, &data, 2);
> +    omap_i2c_write16(s->addr + OMAP_I2C_CON, data);
> +    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
>      g_assert((data & OMAP_I2C_CON_STP) != 0);
>  
> -    memread(s->addr + OMAP_I2C_STAT, &data, 2);
> +    omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
>      g_assert((data & OMAP_I2C_STAT_NACK) == 0);
>  
>      while (len > 1) {
> -        memread(s->addr + OMAP_I2C_STAT, &data, 2);
> +        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
>          g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
>  
>          memwrite(s->addr + OMAP_I2C_DATA, buf, 2);
> @@ -89,13 +102,13 @@ static void omap_i2c_send(I2CAdapter *i2c, uint8_t addr,
>          len -= 2;
>      }
>      if (len == 1) {
> -        memread(s->addr + OMAP_I2C_STAT, &data, 2);
> +        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
>          g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
>  
>          memwrite(s->addr + OMAP_I2C_DATA, buf, 1);
>      }
>  
> -    memread(s->addr + OMAP_I2C_CON, &data, 2);
> +    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
>      g_assert((data & OMAP_I2C_CON_STP) == 0);
>  }
>  
> @@ -108,32 +121,32 @@ static void omap_i2c_recv(I2CAdapter *i2c, uint8_t addr,
>      omap_i2c_set_slave_addr(s, addr);
>  
>      data = len;
> -    memwrite(s->addr + OMAP_I2C_CNT, &data, 2);
> +    omap_i2c_write16(s->addr + OMAP_I2C_CNT, data);
>  
>      data = OMAP_I2C_CON_I2C_EN |
>             OMAP_I2C_CON_MST |
>             OMAP_I2C_CON_STT |
>             OMAP_I2C_CON_STP;
> -    memwrite(s->addr + OMAP_I2C_CON, &data, 2);
> -    memread(s->addr + OMAP_I2C_CON, &data, 2);
> +    omap_i2c_write16(s->addr + OMAP_I2C_CON, data);
> +    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
>      g_assert((data & OMAP_I2C_CON_STP) == 0);
>  
> -    memread(s->addr + OMAP_I2C_STAT, &data, 2);
> +    omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
>      g_assert((data & OMAP_I2C_STAT_NACK) == 0);
>  
> -    memread(s->addr + OMAP_I2C_CNT, &data, 2);
> +    omap_i2c_read16(s->addr + OMAP_I2C_CNT, &data);
>      g_assert_cmpuint(data, ==, len);
>  
>      while (len > 0) {
> -        memread(s->addr + OMAP_I2C_STAT, &data, 2);
> +        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
>          g_assert((data & OMAP_I2C_STAT_RRDY) != 0);
>          g_assert((data & OMAP_I2C_STAT_ROVR) == 0);
>  
>          memread(s->addr + OMAP_I2C_DATA, &data, 2);
>  
> -        memread(s->addr + OMAP_I2C_STAT, &stat, 2);
> +        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &stat);
>          if (unlikely(len == 1)) {
> -            *buf = data & 0xf;
> +            *buf = le16_to_cpu(data) & 0xf;

I don't really get this part.  You're effectively unswapping the bytes,
right?

Regards,

Anthony Liguori

>              buf++;
>              len--;
>          } else {
> @@ -143,7 +156,7 @@ static void omap_i2c_recv(I2CAdapter *i2c, uint8_t addr,
>          }
>      }
>  
> -    memread(s->addr + OMAP_I2C_CON, &data, 2);
> +    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
>      g_assert((data & OMAP_I2C_CON_STP) == 0);
>  }
>  
> @@ -159,7 +172,7 @@ I2CAdapter *omap_i2c_create(uint64_t addr)
>      i2c->recv = omap_i2c_recv;
>  
>      /* verify the mmio address by looking for a known signature */
> -    memread(addr + OMAP_I2C_REV, &data, 2);
> +    omap_i2c_read16(addr + OMAP_I2C_REV, &data);
>      g_assert_cmphex(data, ==, 0x34);
>  
>      return i2c;
> -- 
> 1.7.10.4
Andreas Färber - Feb. 5, 2013, 5:31 p.m.
Am 05.02.2013 17:58, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>>          memread(s->addr + OMAP_I2C_DATA, &data, 2);
>>  
>> -        memread(s->addr + OMAP_I2C_STAT, &stat, 2);
>> +        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &stat);
>>          if (unlikely(len == 1)) {
>> -            *buf = data & 0xf;
>> +            *buf = le16_to_cpu(data) & 0xf;
> 
> I don't really get this part.  You're effectively unswapping the bytes,
> right?

No. I am reading into "data" unswapped above. This line swaps "data"
when only half of the register is used for the 0xF mask, which assumes
host endianness - I can re-test tonight to verify whether it breaks without.

Probably I should insert a white line to make the "data" vs. "stat"
distinction more clear.
I was using a broken STAT register bit before, if you check the
libi2c-omap patch history, where we couldn't agree on how to fix
properly without spec. Thus I changed the condition to not rely on stat;
reading the register seemed necessary though.

Alternatively the I2C device has a Big Endian bit that we could enable -
but I would rather not have the tests run different device code paths
based on host.

Regards,
Andreas
Anthony Liguori - Feb. 13, 2013, 1:05 a.m.
Applied.  Thanks.

Regards,

Anthony Liguori

Patch

diff --git a/tests/libi2c-omap.c b/tests/libi2c-omap.c
index 9be57e9..7d50ef2 100644
--- a/tests/libi2c-omap.c
+++ b/tests/libi2c-omap.c
@@ -12,6 +12,7 @@ 
 #include <string.h>
 
 #include "qemu/osdep.h"
+#include "qemu/bswap.h"
 #include "libqtest.h"
 
 enum OMAPI2CRegisters {
@@ -48,12 +49,24 @@  typedef struct OMAPI2C {
 } OMAPI2C;
 
 
+static inline void omap_i2c_read16(uint64_t addr, uint16_t *data)
+{
+    memread(addr, data, 2);
+    *data = le16_to_cpu(*data);
+}
+
+static inline void omap_i2c_write16(uint64_t addr, uint16_t data)
+{
+    data = cpu_to_le16(data);
+    memwrite(addr, &data, 2);
+}
+
 static void omap_i2c_set_slave_addr(OMAPI2C *s, uint8_t addr)
 {
     uint16_t data = addr;
 
-    memwrite(s->addr + OMAP_I2C_SA, &data, 2);
-    memread(s->addr + OMAP_I2C_SA, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_SA, data);
+    omap_i2c_read16(s->addr + OMAP_I2C_SA, &data);
     g_assert_cmphex(data, ==, addr);
 }
 
@@ -66,22 +79,22 @@  static void omap_i2c_send(I2CAdapter *i2c, uint8_t addr,
     omap_i2c_set_slave_addr(s, addr);
 
     data = len;
-    memwrite(s->addr + OMAP_I2C_CNT, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_CNT, data);
 
     data = OMAP_I2C_CON_I2C_EN |
            OMAP_I2C_CON_TRX |
            OMAP_I2C_CON_MST |
            OMAP_I2C_CON_STT |
            OMAP_I2C_CON_STP;
-    memwrite(s->addr + OMAP_I2C_CON, &data, 2);
-    memread(s->addr + OMAP_I2C_CON, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_CON, data);
+    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
     g_assert((data & OMAP_I2C_CON_STP) != 0);
 
-    memread(s->addr + OMAP_I2C_STAT, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
     g_assert((data & OMAP_I2C_STAT_NACK) == 0);
 
     while (len > 1) {
-        memread(s->addr + OMAP_I2C_STAT, &data, 2);
+        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
         g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
 
         memwrite(s->addr + OMAP_I2C_DATA, buf, 2);
@@ -89,13 +102,13 @@  static void omap_i2c_send(I2CAdapter *i2c, uint8_t addr,
         len -= 2;
     }
     if (len == 1) {
-        memread(s->addr + OMAP_I2C_STAT, &data, 2);
+        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
         g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
 
         memwrite(s->addr + OMAP_I2C_DATA, buf, 1);
     }
 
-    memread(s->addr + OMAP_I2C_CON, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
     g_assert((data & OMAP_I2C_CON_STP) == 0);
 }
 
@@ -108,32 +121,32 @@  static void omap_i2c_recv(I2CAdapter *i2c, uint8_t addr,
     omap_i2c_set_slave_addr(s, addr);
 
     data = len;
-    memwrite(s->addr + OMAP_I2C_CNT, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_CNT, data);
 
     data = OMAP_I2C_CON_I2C_EN |
            OMAP_I2C_CON_MST |
            OMAP_I2C_CON_STT |
            OMAP_I2C_CON_STP;
-    memwrite(s->addr + OMAP_I2C_CON, &data, 2);
-    memread(s->addr + OMAP_I2C_CON, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_CON, data);
+    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
     g_assert((data & OMAP_I2C_CON_STP) == 0);
 
-    memread(s->addr + OMAP_I2C_STAT, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
     g_assert((data & OMAP_I2C_STAT_NACK) == 0);
 
-    memread(s->addr + OMAP_I2C_CNT, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_CNT, &data);
     g_assert_cmpuint(data, ==, len);
 
     while (len > 0) {
-        memread(s->addr + OMAP_I2C_STAT, &data, 2);
+        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
         g_assert((data & OMAP_I2C_STAT_RRDY) != 0);
         g_assert((data & OMAP_I2C_STAT_ROVR) == 0);
 
         memread(s->addr + OMAP_I2C_DATA, &data, 2);
 
-        memread(s->addr + OMAP_I2C_STAT, &stat, 2);
+        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &stat);
         if (unlikely(len == 1)) {
-            *buf = data & 0xf;
+            *buf = le16_to_cpu(data) & 0xf;
             buf++;
             len--;
         } else {
@@ -143,7 +156,7 @@  static void omap_i2c_recv(I2CAdapter *i2c, uint8_t addr,
         }
     }
 
-    memread(s->addr + OMAP_I2C_CON, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
     g_assert((data & OMAP_I2C_CON_STP) == 0);
 }
 
@@ -159,7 +172,7 @@  I2CAdapter *omap_i2c_create(uint64_t addr)
     i2c->recv = omap_i2c_recv;
 
     /* verify the mmio address by looking for a known signature */
-    memread(addr + OMAP_I2C_REV, &data, 2);
+    omap_i2c_read16(addr + OMAP_I2C_REV, &data);
     g_assert_cmphex(data, ==, 0x34);
 
     return i2c;