qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH-for-4.1 v5 1/3] hw/ssi/xilinx_spips: Convert lqspi


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-arm] [PATCH-for-4.1 v5 1/3] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
Date: Tue, 9 Jul 2019 14:10:13 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0

On 7/9/19 1:11 PM, Peter Maydell wrote:
> 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.

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]