On Mon, Dec 16, 2019 at 1:14 AM Philippe Mathieu-Daudé <phi...@redhat.com> wrote:
> 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, > Now it makes more sense to me, thanks Philippe for explaining this! Great, I'll add .impl.min_access_size = 4. At this point, I've processed all the feedback that I received for all of the patches in this series. Is there anything else you would like to see/discuss/review, or shall I send the v2 when I finish testing? Regards, Niek > > > 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. > > -- Niek Linnenbank
