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

[Niek Linnenbank <nieklinnenbank@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3983 bytes --]

On Mon, Dec 16, 2019 at 1:14 AM Philippe Mathieu-Daudé <philmd@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é
> > <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,
>

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

[-- Attachment #2: Type: text/html, Size: 5511 bytes --]