Re: [Qemu-devel] [PATCH-for-4.1 v5 1/3] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Peter Maydell]

On Mon, 8 Jul 2019 at 11:48, Philippe Mathieu-Daudé <address@hidden> wrote:
>
> In the next commit we will implement the write_with_attrs()
> handler. To avoid using different APIs, convert the read()
> handler first.
>
> Reviewed-by: Francisco Iglesias <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> v4: Do not ignore lqspi_read() return value (Francisco)
> ---
>  hw/ssi/xilinx_spips.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 8115bb6d46..b7c7275dbe 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1202,27 +1202,26 @@ static void lqspi_load_cache(void *opaque, hwaddr 
> addr)
>      }
>  }
>
> -static uint64_t
> -lqspi_read(void *opaque, hwaddr addr, unsigned int size)
> +static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
> +                              unsigned size, MemTxAttrs attrs)
>  {
> -    XilinxQSPIPS *q = opaque;
> -    uint32_t ret;
> +    XilinxQSPIPS *q = XILINX_QSPIPS(opaque);
>
>      if (addr >= q->lqspi_cached_addr &&
>              addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
>          uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
> -        ret = cpu_to_le32(*(uint32_t *)retp);
> -        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
> -                   (unsigned)ret);
> -        return ret;
> -    } else {
> -        lqspi_load_cache(opaque, addr);
> -        return lqspi_read(opaque, addr, size);
> +        *value = cpu_to_le32(*(uint32_t *)retp);

If you find yourself casting a uint8_t* to uint32_t* in
order to pass it to cpu_to_le32(), it's a sign that you
should instead be using one of the "load/store value in
appropriate endianness" operations. In this case I think
you want
    *value = ldl_le_p(retp);

That looks like it was an issue already present in this code,
though, (we do it several times in various places in the source file)
so we can fix it later.

thanks
-- PMM