|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller |
Date: | Mon, 16 Dec 2019 01:14:38 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
On 12/16/19 12:07 AM, Niek Linnenbank wrote:
On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé <address@hidden <mailto:address@hidden>> 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_ENDIANset 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.
[Prev in Thread] | Current Thread | [Next in Thread] |