Re: [Qemu-arm] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Philippe Mathieu-Daudé]

On 7/5/19 7:06 PM, Philippe Mathieu-Daudé wrote:
> On 7/5/19 5:53 PM, Francisco Iglesias wrote:
>> Hi Philippe,
>>
>> On [2019 Jul 05] Fri 17:08:49, Philippe Mathieu-Daudé wrote:
>>> In the next commit we will implement the write_with_attrs()
>>> handler. To avoid using different APIs, convert the read()
>>> handler first.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>> ---
>>>  hw/ssi/xilinx_spips.c | 20 ++++++++++----------
>>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>>> index 8115bb6d46..e80619aece 100644
>>> --- a/hw/ssi/xilinx_spips.c
>>> +++ b/hw/ssi/xilinx_spips.c
>>> @@ -1202,27 +1202,27 @@ 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) {

Looking at Frederic's commit 252b99baeb9, it seems this "4" has to be
replaced by "size", or cleaner, use .impl.min_access_size = 4.

Currently we have:

static const MemoryRegionOps lqspi_ops = {
    .valid = {
        .min_access_size = 1,
        .max_access_size = 4
    }
};

If we use:

- size = 1
- addr = LQSPI_CACHE_SIZE - 1

We have

'addr >= q->lqspi_cached_addr': true
'addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4': true

>>>          uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
>>> -        ret = cpu_to_le32(*(uint32_t *)retp);

Are we reading 3 extra bytes?

>>> -        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
>>> -                   (unsigned)ret);
>>> -        return ret;
>>> +        *value = cpu_to_le32(*(uint32_t *)retp);
>>> +        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
>>> +                   addr, *value);
>>>      } else {
>>>          lqspi_load_cache(opaque, addr);
>>> -        return lqspi_read(opaque, addr, size);
>>> +        lqspi_read(opaque, addr, value, size, attrs);
>>
>> If you don't want to leave the return value floating you can always keep the
>> 'return' (I'm unsure if coverity will complain about that).
> 
> Ah, I missed that, I'll fix.
> 
>>
>> Either way:
>>
>> Reviewed-by: Francisco Iglesias <address@hidden>
> 
> Thanks!
> 
> I'll wait some more time of other want to review, then I'll respin with
> the typo you corrected in the 2nd patch fixed.
> 
>>
>>>      }
>>> +
>>> +    return MEMTX_OK;
>>>  }
>>>  
>>>  static const MemoryRegionOps lqspi_ops = {
>>> -    .read = lqspi_read,
>>> +    .read_with_attrs = lqspi_read,
>>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>>      .valid = {
>>>          .min_access_size = 1,
>>> -- 
>>> 2.20.1
>>>