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

[Philippe Mathieu-Daudé]

Le lun. 16 déc. 2019 20:46, Niek Linnenbank  a
écrit :

>
>
> On Mon, Dec 16, 2019 at 1:14 AM Philippe Mathieu-Daudé 
> wrote:
>
>> On 12/16/19 12:07 AM, Niek Linnenbank wrote:
>> >
>> >
>> > On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé
>> > 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?
>

Send it! We'll discuss on updated v2 :)

Regards,

Phil.

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

[Niek Linnenbank]

On Mon, Dec 16, 2019 at 1:14 AM Philippe Mathieu-Daudé 
wrote:

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

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

[Philippe Mathieu-Daudé]

On 12/16/19 12:07 AM, Niek Linnenbank wrote:



On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé 
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.

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

[Niek Linnenbank]

On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé 
wrote:

> Hi Niek,
>
> On 12/11/19 11:34 PM, Niek Linnenbank wrote:
> > Ping!
> >
> > Anyone would like to comment on this driver?
> >
> > I finished the rework on all previous comments in this series.
> >
> > Currently debugging the hflags error reported by Philippe.
> > After that, I'm ready to send out v2 of these patches.
> >
> > Regards,
> > Niek
> >
> > On Mon, Dec 2, 2019 at 10:10 PM Niek Linnenbank
> > mailto:nieklinnenb...@gmail.com>> wrote:
> >
> > The Allwinner H3 System on Chip contains an integrated storage
> > controller for Secure Digital (SD) and Multi Media Card (MMC)
> > interfaces. This commit adds support for the Allwinner H3
> > SD/MMC storage controller with the following emulated features:
> >
> >   * DMA transfers
> >   * Direct FIFO I/O
> >   * Short/Long format command responses
> >   * Auto-Stop command (CMD12)
> >   * Insert & remove card detection
> >
> > Signed-off-by: Niek Linnenbank  > >
> > ---
> >   hw/arm/allwinner-h3.c   |  20 +
> >   hw/arm/orangepi.c   |  17 +
> >   hw/sd/Makefile.objs |   1 +
> >   hw/sd/allwinner-h3-sdhost.c | 791
> 
> >   hw/sd/trace-events  |   7 +
> >   include/hw/arm/allwinner-h3.h   |   2 +
> >   include/hw/sd/allwinner-h3-sdhost.h |  73 +++
> >   7 files changed, 911 insertions(+)
> >   create mode 100644 hw/sd/allwinner-h3-sdhost.c
> >   create mode 100644 include/hw/sd/allwinner-h3-sdhost.h
> >
> > diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> > index 4fc4c8c725..c2972caf88 100644
> > --- a/hw/arm/allwinner-h3.c
> > +++ b/hw/arm/allwinner-h3.c
> > @@ -50,6 +50,9 @@ static void aw_h3_init(Object *obj)
> >
> >   sysbus_init_child_obj(obj, "sid", >sid, sizeof(s->sid),
> > TYPE_AW_H3_SID);
> > +
> > +sysbus_init_child_obj(obj, "mmc0", >mmc0, sizeof(s->mmc0),
> > +  TYPE_AW_H3_SDHOST);
> >   }
> >
> >   static void aw_h3_realize(DeviceState *dev, Error **errp)
> > @@ -217,6 +220,23 @@ static void aw_h3_realize(DeviceState *dev,
> > Error **errp)
> >   }
> >   sysbus_mmio_map(SYS_BUS_DEVICE(>sid), 0, AW_H3_SID_BASE);
> >
> > +/* SD/MMC */
> > +object_property_set_bool(OBJECT(>mmc0), true, "realized",
> );
> > +if (err != NULL) {
> > +error_propagate(errp, err);
> > +return;
> > +}
> > +sysbusdev = SYS_BUS_DEVICE(>mmc0);
> > +sysbus_mmio_map(sysbusdev, 0, AW_H3_MMC0_BASE);
> > +sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_MMC0]);
> > +
> > +object_property_add_alias(OBJECT(s), "sd-bus", OBJECT(>mmc0),
> > +  "sd-bus", );
> > +if (err) {
> > +error_propagate(errp, err);
> > +return;
> > +}
> > +
> >   /* Universal Serial Bus */
> >   sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
> >s->irq[AW_H3_GIC_SPI_EHCI0]);
> > diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> > index 5ef2735f81..dee3efaf08 100644
> > --- a/hw/arm/orangepi.c
> > +++ b/hw/arm/orangepi.c
> > @@ -39,6 +39,10 @@ typedef struct OrangePiState {
> >   static void orangepi_init(MachineState *machine)
> >   {
> >   OrangePiState *s = g_new(OrangePiState, 1);
> > +DriveInfo *di;
> > +BlockBackend *blk;
> > +BusState *bus;
> > +DeviceState *carddev;
> >   Error *err = NULL;
> >
> >   s->h3 = AW_H3(object_new(TYPE_AW_H3));
> > @@ -64,6 +68,18 @@ static void orangepi_init(MachineState *machine)
> >   exit(1);
> >   }
> >
> > +/* Create and plug in the SD card */
> > +di = drive_get_next(IF_SD);
> > +blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > +bus = qdev_get_child_bus(DEVICE(s->h3), "sd-bus");
> > +if (bus == NULL) {
> > +error_report("No SD/MMC found in H3 object");
> > +exit(1);
> > +}
>
> Your device always creates a bus, so I don't think the if(bus) check is
> worthwhile. Eventually use an assert(bus)?
>
> > +carddev = qdev_create(bus, TYPE_SD_CARD);
> > +qdev_prop_set_drive(carddev, "drive", blk, _fatal);
> > +object_property_set_bool(OBJECT(carddev), true, "realized",
> > _fatal);
> > +
> >   /* RAM */
> >   memory_region_allocate_system_memory(>sdram, NULL,
> > "orangepi.ram",
> >machine->ram_size);
> > @@ -80,6 +96,7 @@ static void orangepi_machine_init(MachineClass *mc)
> >   {
> >   mc->desc = "Orange Pi PC";
> >   mc->init = 

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

[Philippe Mathieu-Daudé]

On 12/13/19 10:00 PM, Niek Linnenbank wrote:
On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé 
mailto:phi...@redhat.com>> wrote:


Hi Niek,

On 12/11/19 11:34 PM, Niek Linnenbank wrote:
 > Ping!
 >
 > Anyone would like to comment on this driver?
 >
 > I finished the rework on all previous comments in this series.
 >
 > Currently debugging the hflags error reported by Philippe.
 > After that, I'm ready to send out v2 of these patches.
 >
 > Regards,
 > Niek
 >
 > On Mon, Dec 2, 2019 at 10:10 PM Niek Linnenbank
 > mailto:nieklinnenb...@gmail.com>
>>
wrote:
 >
 >     The Allwinner H3 System on Chip contains an integrated storage
 >     controller for Secure Digital (SD) and Multi Media Card (MMC)
 >     interfaces. This commit adds support for the Allwinner H3
 >     SD/MMC storage controller with the following emulated features:
 >
 >       * DMA transfers
 >       * Direct FIFO I/O
 >       * Short/Long format command responses
 >       * Auto-Stop command (CMD12)
 >       * Insert & remove card detection
 >
 >     Signed-off-by: Niek Linnenbank mailto:nieklinnenb...@gmail.com>
 >     >>
 >     ---
 >       hw/arm/allwinner-h3.c               |  20 +
 >       hw/arm/orangepi.c                   |  17 +
 >       hw/sd/Makefile.objs                 |   1 +
 >       hw/sd/allwinner-h3-sdhost.c         | 791

 >       hw/sd/trace-events                  |   7 +
 >       include/hw/arm/allwinner-h3.h       |   2 +
 >       include/hw/sd/allwinner-h3-sdhost.h |  73 +++
 >       7 files changed, 911 insertions(+)
 >       create mode 100644 hw/sd/allwinner-h3-sdhost.c
 >       create mode 100644 include/hw/sd/allwinner-h3-sdhost.h

[...]

Thanks again for all of your helpful comments Philippe!
I've started to rework the patch.

One question about adding tags in the commit message: should I
already add 'Reviewed-by: ' when I re-send v2 of this patch? Or should
that be added after you have seen the v2 changes?


You shouldn't add the Reviewed-by tag until explicitly given by the 
reviewer. If the changes are trivial, it is easy to conditionally give 
the tag such "If ... is done: R-b", "With ... fixed: R-b".


Since this is your first contribution, I have been more careful. Also 
since your patch is already of very good quality, I'v been a bit picky 
regarding few details.


Since there are too many comments, so I prefer to fully review the v2 of 
this patch again.


Regards,

Phil.

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

[Niek Linnenbank]

On Sat, Dec 14, 2019 at 2:59 PM Philippe Mathieu-Daudé 
wrote:

> On 12/13/19 10:00 PM, Niek Linnenbank wrote:
> > On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé
> > mailto:phi...@redhat.com>> wrote:
> >
> > Hi Niek,
> >
> > On 12/11/19 11:34 PM, Niek Linnenbank wrote:
> >  > Ping!
> >  >
> >  > Anyone would like to comment on this driver?
> >  >
> >  > I finished the rework on all previous comments in this series.
> >  >
> >  > Currently debugging the hflags error reported by Philippe.
> >  > After that, I'm ready to send out v2 of these patches.
> >  >
> >  > Regards,
> >  > Niek
> >  >
> >  > On Mon, Dec 2, 2019 at 10:10 PM Niek Linnenbank
> >  > mailto:nieklinnenb...@gmail.com>
> > >>
> > wrote:
> >  >
> >  > The Allwinner H3 System on Chip contains an integrated storage
> >  > controller for Secure Digital (SD) and Multi Media Card (MMC)
> >  > interfaces. This commit adds support for the Allwinner H3
> >  > SD/MMC storage controller with the following emulated
> features:
> >  >
> >  >   * DMA transfers
> >  >   * Direct FIFO I/O
> >  >   * Short/Long format command responses
> >  >   * Auto-Stop command (CMD12)
> >  >   * Insert & remove card detection
> >  >
> >  > Signed-off-by: Niek Linnenbank  > 
> >  >  > >>
> >  > ---
> >  >   hw/arm/allwinner-h3.c   |  20 +
> >  >   hw/arm/orangepi.c   |  17 +
> >  >   hw/sd/Makefile.objs |   1 +
> >  >   hw/sd/allwinner-h3-sdhost.c | 791
> > 
> >  >   hw/sd/trace-events  |   7 +
> >  >   include/hw/arm/allwinner-h3.h   |   2 +
> >  >   include/hw/sd/allwinner-h3-sdhost.h |  73 +++
> >  >   7 files changed, 911 insertions(+)
> >  >   create mode 100644 hw/sd/allwinner-h3-sdhost.c
> >  >   create mode 100644 include/hw/sd/allwinner-h3-sdhost.h
> [...]
> > Thanks again for all of your helpful comments Philippe!
> > I've started to rework the patch.
> >
> > One question about adding tags in the commit message: should I
> > already add 'Reviewed-by: ' when I re-send v2 of this patch? Or should
> > that be added after you have seen the v2 changes?
>
> You shouldn't add the Reviewed-by tag until explicitly given by the
> reviewer. If the changes are trivial, it is easy to conditionally give
> the tag such "If ... is done: R-b", "With ... fixed: R-b".
>

OK, thanks for clarifying, I'll keep that in mind.


>
> Since this is your first contribution, I have been more careful. Also
> since your patch is already of very good quality, I'v been a bit picky
> regarding few details.
>

Sure, that makes sense indeed. And I very much appreciate your feedback
Philippe,
so please keep doing that, even about the small details ;-)


>
> Since there are too many comments, so I prefer to fully review the v2 of
> this patch again.
>
> Yes, true indeed.

Regards,
Niek


> Regards,
>
> Phil.
>
>

-- 
Niek Linnenbank

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

[Niek Linnenbank]

On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé 
wrote:

> Hi Niek,
>
> On 12/11/19 11:34 PM, Niek Linnenbank wrote:
> > Ping!
> >
> > Anyone would like to comment on this driver?
> >
> > I finished the rework on all previous comments in this series.
> >
> > Currently debugging the hflags error reported by Philippe.
> > After that, I'm ready to send out v2 of these patches.
> >
> > Regards,
> > Niek
> >
> > On Mon, Dec 2, 2019 at 10:10 PM Niek Linnenbank
> > mailto:nieklinnenb...@gmail.com>> wrote:
> >
> > The Allwinner H3 System on Chip contains an integrated storage
> > controller for Secure Digital (SD) and Multi Media Card (MMC)
> > interfaces. This commit adds support for the Allwinner H3
> > SD/MMC storage controller with the following emulated features:
> >
> >   * DMA transfers
> >   * Direct FIFO I/O
> >   * Short/Long format command responses
> >   * Auto-Stop command (CMD12)
> >   * Insert & remove card detection
> >
> > Signed-off-by: Niek Linnenbank  > >
> > ---
> >   hw/arm/allwinner-h3.c   |  20 +
> >   hw/arm/orangepi.c   |  17 +
> >   hw/sd/Makefile.objs |   1 +
> >   hw/sd/allwinner-h3-sdhost.c | 791
> 
> >   hw/sd/trace-events  |   7 +
> >   include/hw/arm/allwinner-h3.h   |   2 +
> >   include/hw/sd/allwinner-h3-sdhost.h |  73 +++
> >   7 files changed, 911 insertions(+)
> >   create mode 100644 hw/sd/allwinner-h3-sdhost.c
> >   create mode 100644 include/hw/sd/allwinner-h3-sdhost.h
> >
> > diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> > index 4fc4c8c725..c2972caf88 100644
> > --- a/hw/arm/allwinner-h3.c
> > +++ b/hw/arm/allwinner-h3.c
> > @@ -50,6 +50,9 @@ static void aw_h3_init(Object *obj)
> >
> >   sysbus_init_child_obj(obj, "sid", >sid, sizeof(s->sid),
> > TYPE_AW_H3_SID);
> > +
> > +sysbus_init_child_obj(obj, "mmc0", >mmc0, sizeof(s->mmc0),
> > +  TYPE_AW_H3_SDHOST);
> >   }
> >
> >   static void aw_h3_realize(DeviceState *dev, Error **errp)
> > @@ -217,6 +220,23 @@ static void aw_h3_realize(DeviceState *dev,
> > Error **errp)
> >   }
> >   sysbus_mmio_map(SYS_BUS_DEVICE(>sid), 0, AW_H3_SID_BASE);
> >
> > +/* SD/MMC */
> > +object_property_set_bool(OBJECT(>mmc0), true, "realized",
> );
> > +if (err != NULL) {
> > +error_propagate(errp, err);
> > +return;
> > +}
> > +sysbusdev = SYS_BUS_DEVICE(>mmc0);
> > +sysbus_mmio_map(sysbusdev, 0, AW_H3_MMC0_BASE);
> > +sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_MMC0]);
> > +
> > +object_property_add_alias(OBJECT(s), "sd-bus", OBJECT(>mmc0),
> > +  "sd-bus", );
> > +if (err) {
> > +error_propagate(errp, err);
> > +return;
> > +}
> > +
> >   /* Universal Serial Bus */
> >   sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
> >s->irq[AW_H3_GIC_SPI_EHCI0]);
> > diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> > index 5ef2735f81..dee3efaf08 100644
> > --- a/hw/arm/orangepi.c
> > +++ b/hw/arm/orangepi.c
> > @@ -39,6 +39,10 @@ typedef struct OrangePiState {
> >   static void orangepi_init(MachineState *machine)
> >   {
> >   OrangePiState *s = g_new(OrangePiState, 1);
> > +DriveInfo *di;
> > +BlockBackend *blk;
> > +BusState *bus;
> > +DeviceState *carddev;
> >   Error *err = NULL;
> >
> >   s->h3 = AW_H3(object_new(TYPE_AW_H3));
> > @@ -64,6 +68,18 @@ static void orangepi_init(MachineState *machine)
> >   exit(1);
> >   }
> >
> > +/* Create and plug in the SD card */
> > +di = drive_get_next(IF_SD);
> > +blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > +bus = qdev_get_child_bus(DEVICE(s->h3), "sd-bus");
> > +if (bus == NULL) {
> > +error_report("No SD/MMC found in H3 object");
> > +exit(1);
> > +}
>
> Your device always creates a bus, so I don't think the if(bus) check is
> worthwhile. Eventually use an assert(bus)?
>
> > +carddev = qdev_create(bus, TYPE_SD_CARD);
> > +qdev_prop_set_drive(carddev, "drive", blk, _fatal);
> > +object_property_set_bool(OBJECT(carddev), true, "realized",
> > _fatal);
> > +
> >   /* RAM */
> >   memory_region_allocate_system_memory(>sdram, NULL,
> > "orangepi.ram",
> >machine->ram_size);
> > @@ -80,6 +96,7 @@ static void orangepi_machine_init(MachineClass *mc)
> >   {
> >   mc->desc = "Orange Pi PC";
> >   mc->init = 

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

[Philippe Mathieu-Daudé]

Hi Niek,

On 12/11/19 11:34 PM, Niek Linnenbank wrote:

Ping!

Anyone would like to comment on this driver?

I finished the rework on all previous comments in this series.

Currently debugging the hflags error reported by Philippe.
After that, I'm ready to send out v2 of these patches.

Regards,
Niek

On Mon, Dec 2, 2019 at 10:10 PM Niek Linnenbank 
mailto:nieklinnenb...@gmail.com>> wrote:


The Allwinner H3 System on Chip contains an integrated storage
controller for Secure Digital (SD) and Multi Media Card (MMC)
interfaces. This commit adds support for the Allwinner H3
SD/MMC storage controller with the following emulated features:

  * DMA transfers
  * Direct FIFO I/O
  * Short/Long format command responses
  * Auto-Stop command (CMD12)
  * Insert & remove card detection

Signed-off-by: Niek Linnenbank mailto:nieklinnenb...@gmail.com>>
---
  hw/arm/allwinner-h3.c               |  20 +
  hw/arm/orangepi.c                   |  17 +
  hw/sd/Makefile.objs                 |   1 +
  hw/sd/allwinner-h3-sdhost.c         | 791 
  hw/sd/trace-events                  |   7 +
  include/hw/arm/allwinner-h3.h       |   2 +
  include/hw/sd/allwinner-h3-sdhost.h |  73 +++
  7 files changed, 911 insertions(+)
  create mode 100644 hw/sd/allwinner-h3-sdhost.c
  create mode 100644 include/hw/sd/allwinner-h3-sdhost.h

diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index 4fc4c8c725..c2972caf88 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -50,6 +50,9 @@ static void aw_h3_init(Object *obj)

      sysbus_init_child_obj(obj, "sid", >sid, sizeof(s->sid),
                            TYPE_AW_H3_SID);
+
+    sysbus_init_child_obj(obj, "mmc0", >mmc0, sizeof(s->mmc0),
+                          TYPE_AW_H3_SDHOST);
  }

  static void aw_h3_realize(DeviceState *dev, Error **errp)
@@ -217,6 +220,23 @@ static void aw_h3_realize(DeviceState *dev,
Error **errp)
      }
      sysbus_mmio_map(SYS_BUS_DEVICE(>sid), 0, AW_H3_SID_BASE);

+    /* SD/MMC */
+    object_property_set_bool(OBJECT(>mmc0), true, "realized", );
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbusdev = SYS_BUS_DEVICE(>mmc0);
+    sysbus_mmio_map(sysbusdev, 0, AW_H3_MMC0_BASE);
+    sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_MMC0]);
+
+    object_property_add_alias(OBJECT(s), "sd-bus", OBJECT(>mmc0),
+                              "sd-bus", );
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
      /* Universal Serial Bus */
      sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
                           s->irq[AW_H3_GIC_SPI_EHCI0]);
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index 5ef2735f81..dee3efaf08 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -39,6 +39,10 @@ typedef struct OrangePiState {
  static void orangepi_init(MachineState *machine)
  {
      OrangePiState *s = g_new(OrangePiState, 1);
+    DriveInfo *di;
+    BlockBackend *blk;
+    BusState *bus;
+    DeviceState *carddev;
      Error *err = NULL;

      s->h3 = AW_H3(object_new(TYPE_AW_H3));
@@ -64,6 +68,18 @@ static void orangepi_init(MachineState *machine)
          exit(1);
      }

+    /* Create and plug in the SD card */
+    di = drive_get_next(IF_SD);
+    blk = di ? blk_by_legacy_dinfo(di) : NULL;
+    bus = qdev_get_child_bus(DEVICE(s->h3), "sd-bus");
+    if (bus == NULL) {
+        error_report("No SD/MMC found in H3 object");
+        exit(1);
+    }


Your device always creates a bus, so I don't think the if(bus) check is 
worthwhile. Eventually use an assert(bus)?



+    carddev = qdev_create(bus, TYPE_SD_CARD);
+    qdev_prop_set_drive(carddev, "drive", blk, _fatal);
+    object_property_set_bool(OBJECT(carddev), true, "realized",
_fatal);
+
      /* RAM */
      memory_region_allocate_system_memory(>sdram, NULL,
"orangepi.ram",
                                           machine->ram_size);
@@ -80,6 +96,7 @@ static void orangepi_machine_init(MachineClass *mc)
  {
      mc->desc = "Orange Pi PC";
      mc->init = orangepi_init;
+    mc->block_default_type = IF_SD;
      mc->units_per_default_bus = 1;
      mc->min_cpus = AW_H3_NUM_CPUS;
      mc->max_cpus = AW_H3_NUM_CPUS;
diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
index a884c238df..e7cc5ab739 100644
--- a/hw/sd/Makefile.objs
+++ b/hw/sd/Makefile.objs
@@ -4,6 +4,7 @@ common-obj-$(CONFIG_SD) += sd.o core.o sdmmc-internal.o
  common-obj-$(CONFIG_SDHCI) += sdhci.o
  common-obj-$(CONFIG_SDHCI_PCI) += sdhci-pci.o

+obj-$(CONFIG_ALLWINNER_H3) 

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

[Niek Linnenbank]

Ping!

Anyone would like to comment on this driver?

I finished the rework on all previous comments in this series.

Currently debugging the hflags error reported by Philippe.
After that, I'm ready to send out v2 of these patches.

Regards,
Niek

On Mon, Dec 2, 2019 at 10:10 PM Niek Linnenbank 
wrote:

> The Allwinner H3 System on Chip contains an integrated storage
> controller for Secure Digital (SD) and Multi Media Card (MMC)
> interfaces. This commit adds support for the Allwinner H3
> SD/MMC storage controller with the following emulated features:
>
>  * DMA transfers
>  * Direct FIFO I/O
>  * Short/Long format command responses
>  * Auto-Stop command (CMD12)
>  * Insert & remove card detection
>
> Signed-off-by: Niek Linnenbank 
> ---
>  hw/arm/allwinner-h3.c   |  20 +
>  hw/arm/orangepi.c   |  17 +
>  hw/sd/Makefile.objs |   1 +
>  hw/sd/allwinner-h3-sdhost.c | 791 
>  hw/sd/trace-events  |   7 +
>  include/hw/arm/allwinner-h3.h   |   2 +
>  include/hw/sd/allwinner-h3-sdhost.h |  73 +++
>  7 files changed, 911 insertions(+)
>  create mode 100644 hw/sd/allwinner-h3-sdhost.c
>  create mode 100644 include/hw/sd/allwinner-h3-sdhost.h
>
> diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> index 4fc4c8c725..c2972caf88 100644
> --- a/hw/arm/allwinner-h3.c
> +++ b/hw/arm/allwinner-h3.c
> @@ -50,6 +50,9 @@ static void aw_h3_init(Object *obj)
>
>  sysbus_init_child_obj(obj, "sid", >sid, sizeof(s->sid),
>TYPE_AW_H3_SID);
> +
> +sysbus_init_child_obj(obj, "mmc0", >mmc0, sizeof(s->mmc0),
> +  TYPE_AW_H3_SDHOST);
>  }
>
>  static void aw_h3_realize(DeviceState *dev, Error **errp)
> @@ -217,6 +220,23 @@ static void aw_h3_realize(DeviceState *dev, Error
> **errp)
>  }
>  sysbus_mmio_map(SYS_BUS_DEVICE(>sid), 0, AW_H3_SID_BASE);
>
> +/* SD/MMC */
> +object_property_set_bool(OBJECT(>mmc0), true, "realized", );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
> +sysbusdev = SYS_BUS_DEVICE(>mmc0);
> +sysbus_mmio_map(sysbusdev, 0, AW_H3_MMC0_BASE);
> +sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_MMC0]);
> +
> +object_property_add_alias(OBJECT(s), "sd-bus", OBJECT(>mmc0),
> +  "sd-bus", );
> +if (err) {
> +error_propagate(errp, err);
> +return;
> +}
> +
>  /* Universal Serial Bus */
>  sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
>   s->irq[AW_H3_GIC_SPI_EHCI0]);
> diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> index 5ef2735f81..dee3efaf08 100644
> --- a/hw/arm/orangepi.c
> +++ b/hw/arm/orangepi.c
> @@ -39,6 +39,10 @@ typedef struct OrangePiState {
>  static void orangepi_init(MachineState *machine)
>  {
>  OrangePiState *s = g_new(OrangePiState, 1);
> +DriveInfo *di;
> +BlockBackend *blk;
> +BusState *bus;
> +DeviceState *carddev;
>  Error *err = NULL;
>
>  s->h3 = AW_H3(object_new(TYPE_AW_H3));
> @@ -64,6 +68,18 @@ static void orangepi_init(MachineState *machine)
>  exit(1);
>  }
>
> +/* Create and plug in the SD card */
> +di = drive_get_next(IF_SD);
> +blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +bus = qdev_get_child_bus(DEVICE(s->h3), "sd-bus");
> +if (bus == NULL) {
> +error_report("No SD/MMC found in H3 object");
> +exit(1);
> +}
> +carddev = qdev_create(bus, TYPE_SD_CARD);
> +qdev_prop_set_drive(carddev, "drive", blk, _fatal);
> +object_property_set_bool(OBJECT(carddev), true, "realized",
> _fatal);
> +
>  /* RAM */
>  memory_region_allocate_system_memory(>sdram, NULL, "orangepi.ram",
>   machine->ram_size);
> @@ -80,6 +96,7 @@ static void orangepi_machine_init(MachineClass *mc)
>  {
>  mc->desc = "Orange Pi PC";
>  mc->init = orangepi_init;
> +mc->block_default_type = IF_SD;
>  mc->units_per_default_bus = 1;
>  mc->min_cpus = AW_H3_NUM_CPUS;
>  mc->max_cpus = AW_H3_NUM_CPUS;
> diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
> index a884c238df..e7cc5ab739 100644
> --- a/hw/sd/Makefile.objs
> +++ b/hw/sd/Makefile.objs
> @@ -4,6 +4,7 @@ common-obj-$(CONFIG_SD) += sd.o core.o sdmmc-internal.o
>  common-obj-$(CONFIG_SDHCI) += sdhci.o
>  common-obj-$(CONFIG_SDHCI_PCI) += sdhci-pci.o
>
> +obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3-sdhost.o
>  obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
>  obj-$(CONFIG_OMAP) += omap_mmc.o
>  obj-$(CONFIG_PXA2XX) += pxa2xx_mmci.o
> diff --git a/hw/sd/allwinner-h3-sdhost.c b/hw/sd/allwinner-h3-sdhost.c
> new file mode 100644
> index 00..26e113a144
> --- /dev/null
> +++ b/hw/sd/allwinner-h3-sdhost.c
> @@ -0,0 +1,791 @@
> +/*
> + * Allwinner H3 SD Host Controller emulation
> + *
> + * Copyright (C) 2019 Niek Linnenbank 
> + *
> + * This program is free 

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

[Niek Linnenbank]

The Allwinner H3 System on Chip contains an integrated storage
controller for Secure Digital (SD) and Multi Media Card (MMC)
interfaces. This commit adds support for the Allwinner H3
SD/MMC storage controller with the following emulated features:

 * DMA transfers
 * Direct FIFO I/O
 * Short/Long format command responses
 * Auto-Stop command (CMD12)
 * Insert & remove card detection

Signed-off-by: Niek Linnenbank 
---
 hw/arm/allwinner-h3.c   |  20 +
 hw/arm/orangepi.c   |  17 +
 hw/sd/Makefile.objs |   1 +
 hw/sd/allwinner-h3-sdhost.c | 791 
 hw/sd/trace-events  |   7 +
 include/hw/arm/allwinner-h3.h   |   2 +
 include/hw/sd/allwinner-h3-sdhost.h |  73 +++
 7 files changed, 911 insertions(+)
 create mode 100644 hw/sd/allwinner-h3-sdhost.c
 create mode 100644 include/hw/sd/allwinner-h3-sdhost.h

diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index 4fc4c8c725..c2972caf88 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -50,6 +50,9 @@ static void aw_h3_init(Object *obj)
 
 sysbus_init_child_obj(obj, "sid", >sid, sizeof(s->sid),
   TYPE_AW_H3_SID);
+
+sysbus_init_child_obj(obj, "mmc0", >mmc0, sizeof(s->mmc0),
+  TYPE_AW_H3_SDHOST);
 }
 
 static void aw_h3_realize(DeviceState *dev, Error **errp)
@@ -217,6 +220,23 @@ static void aw_h3_realize(DeviceState *dev, Error **errp)
 }
 sysbus_mmio_map(SYS_BUS_DEVICE(>sid), 0, AW_H3_SID_BASE);
 
+/* SD/MMC */
+object_property_set_bool(OBJECT(>mmc0), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
+sysbusdev = SYS_BUS_DEVICE(>mmc0);
+sysbus_mmio_map(sysbusdev, 0, AW_H3_MMC0_BASE);
+sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_MMC0]);
+
+object_property_add_alias(OBJECT(s), "sd-bus", OBJECT(>mmc0),
+  "sd-bus", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
+
 /* Universal Serial Bus */
 sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
  s->irq[AW_H3_GIC_SPI_EHCI0]);
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index 5ef2735f81..dee3efaf08 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -39,6 +39,10 @@ typedef struct OrangePiState {
 static void orangepi_init(MachineState *machine)
 {
 OrangePiState *s = g_new(OrangePiState, 1);
+DriveInfo *di;
+BlockBackend *blk;
+BusState *bus;
+DeviceState *carddev;
 Error *err = NULL;
 
 s->h3 = AW_H3(object_new(TYPE_AW_H3));
@@ -64,6 +68,18 @@ static void orangepi_init(MachineState *machine)
 exit(1);
 }
 
+/* Create and plug in the SD card */
+di = drive_get_next(IF_SD);
+blk = di ? blk_by_legacy_dinfo(di) : NULL;
+bus = qdev_get_child_bus(DEVICE(s->h3), "sd-bus");
+if (bus == NULL) {
+error_report("No SD/MMC found in H3 object");
+exit(1);
+}
+carddev = qdev_create(bus, TYPE_SD_CARD);
+qdev_prop_set_drive(carddev, "drive", blk, _fatal);
+object_property_set_bool(OBJECT(carddev), true, "realized", _fatal);
+
 /* RAM */
 memory_region_allocate_system_memory(>sdram, NULL, "orangepi.ram",
  machine->ram_size);
@@ -80,6 +96,7 @@ static void orangepi_machine_init(MachineClass *mc)
 {
 mc->desc = "Orange Pi PC";
 mc->init = orangepi_init;
+mc->block_default_type = IF_SD;
 mc->units_per_default_bus = 1;
 mc->min_cpus = AW_H3_NUM_CPUS;
 mc->max_cpus = AW_H3_NUM_CPUS;
diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
index a884c238df..e7cc5ab739 100644
--- a/hw/sd/Makefile.objs
+++ b/hw/sd/Makefile.objs
@@ -4,6 +4,7 @@ common-obj-$(CONFIG_SD) += sd.o core.o sdmmc-internal.o
 common-obj-$(CONFIG_SDHCI) += sdhci.o
 common-obj-$(CONFIG_SDHCI_PCI) += sdhci-pci.o
 
+obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3-sdhost.o
 obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
 obj-$(CONFIG_OMAP) += omap_mmc.o
 obj-$(CONFIG_PXA2XX) += pxa2xx_mmci.o
diff --git a/hw/sd/allwinner-h3-sdhost.c b/hw/sd/allwinner-h3-sdhost.c
new file mode 100644
index 00..26e113a144
--- /dev/null
+++ b/hw/sd/allwinner-h3-sdhost.c
@@ -0,0 +1,791 @@
+/*
+ * Allwinner H3 SD Host Controller emulation
+ *
+ * Copyright (C) 2019 Niek Linnenbank 
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public