Re: [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Niek Linnenbank <nieklinnenbank@gmail.com>
Cc: Beniamino Galvani <b.galvani@gmail.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	qemu-arm <qemu-arm@nongnu.org>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller
Date: Mon, 16 Dec 2019 01:14:38 +0100	[thread overview]
Message-ID: <03a78f1d-e8fe-5a53-b061-d39de9ed7a9e@redhat.com> (raw)
In-Reply-To: <CAPan3WpiWahU85-ZgV4BMPU8mUTxsq2TPn-szOHkHc5+PHUqpw@mail.gmail.com>

On 12/16/19 12:07 AM, Niek Linnenbank wrote:
> 
> 
> On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé 
> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> 
>     Hi Niek,
> 
>     On 12/11/19 11:34 PM, Niek Linnenbank wrote:
[...]
>      >     +static uint32_t aw_h3_sdhost_process_desc(AwH3SDHostState *s,
>      >     +                                          hwaddr desc_addr,
>      >     +                                          TransferDescriptor
>     *desc,
>      >     +                                          bool is_write,
>     uint32_t
>      >     max_bytes)
>      >     +{
>      >     +    uint32_t num_done = 0;
>      >     +    uint32_t num_bytes = max_bytes;
>      >     +    uint8_t buf[1024];
>      >     +
>      >     +    /* Read descriptor */
>      >     +    cpu_physical_memory_read(desc_addr, desc, sizeof(*desc));
> 
>     Should we worry about endianess here?
> 
> 
> I tried to figure out what is expected, but the 
> Allwinner_H3_Datasheet_V1.2.pdf does not
> explicitly mention endianness for any of its I/O devices. Currently it 
> seems all devices are
> happy with using the same endianness as the CPUs. In the MemoryRegionOps 
> has DEVICE_NATIVE_ENDIAN
> set to match the behavior seen.

OK.

[...]
>      >     +static const MemoryRegionOps aw_h3_sdhost_ops = {
>      >     +    .read = aw_h3_sdhost_read,
>      >     +    .write = aw_h3_sdhost_write,
>      >     +    .endianness = DEVICE_NATIVE_ENDIAN,
> 
>     I haven't checked .valid accesses from the datasheet.
> 
>     However due to:
> 
>         res = s->data_crc[((offset - REG_SD_DATA7_CRC) / sizeof(uint32_t))];
> 
>     You seem to expect:
> 
>                  .impl.min_access_size = 4,
> 
>     .impl.max_access_size unset is 8, which should works.
> 
> It seems that all registers are aligned on at least 32-bit boundaries. 
> And the section 5.3.5.1 mentions
> that the DMA descriptors must be stored in memory 32-bit aligned. So 
> based on that information,

So you are describing ".valid.min_access_size = 4", which is the minimum 
access size on the bus.
".impl.min_access_size" is different, it is what access sizes is ready 
to handle your model.
Your model read/write handlers expect addresses aligned on 32-bit 
boundary, this is why I suggested to use ".impl.min_access_size = 4". If 
the guest were using a 16-bit access, your model would be buggy. If you 
describe your implementation to accept minimum 32-bit and the guest is 
allowed to use smaller accesses, QEMU will do a 32-bit access to the 
device, and return the 16-bit part to the guest. This way your model is 
safe. This is done by access_with_adjusted_size() in memory.c.
If you restrict with ".valid.min_access_size = 4", you might think we 
don't need ".valid.min_access_size = 4" because all access from guest 
will be at least 32-bit. However keep in mind someone might find this 
device in another datasheet not limited to 32-bit, and let's say change 
to ".valid.min_access_size = 2". Without ".impl.min_access_size = 4" 
your model is buggy. So to be safe I'd use:

   .impl.min_access_size = 4,
   .valid.min_access_size = 4,

> I think 32-bit is a safe choice. I've verified this with Linux mainline 
> and U-Boot drivers and both are still working.

Regards,

Phil.