On 7/9/19 1:11 PM, Peter Maydell wrote:
> On Mon, 8 Jul 2019 at 11:48, Philippe Mathieu-Daudé <phi...@redhat.com> 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 <frasse.igles...@gmail.com>
>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>> ---
>> 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.
Well, other places check GQSPI_CFG.ENDIAN bit for switching endianess,
here we don't... Dubious code? Per the code DMA accesses seems
little-endian. However tx_data_bytes() handles endian swaping, while
rx_data_bytes() doesn't.
It seems wise to postpone this after the current release, indeed.
Thanks,
Phil.
