On 12/16/19 12:07 AM, Niek Linnenbank wrote:
On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé
<phi...@redhat.com <mailto:phi...@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.
