qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC] hw/arm/exynos: Add generic SDHCI devices


From: Krzysztof Kozlowski
Subject: Re: [Qemu-devel] [RFC] hw/arm/exynos: Add generic SDHCI devices
Date: Sat, 22 Apr 2017 20:46:32 +0200
User-agent: Mutt/1.6.2-neo (2016-08-21)

On Thu, Apr 20, 2017 at 02:51:00PM +0100, Peter Maydell wrote:
> On 16 April 2017 at 16:16, Krzysztof Kozlowski <address@hidden> wrote:
> > Exynos4210 has four SD/MMC controllers supporting:
> >  - SD Standard Host Specification Version 2.0,
> >  - MMC Specification Version 4.3,
> >  - SDIO Card Specification Version 2.0,
> >  - DMA and ADMA.
> >
> > Add emulation of SDHCI devices which allows accessing storage through SD
> > cards. Differences from real hardware:
> >  - Devices are shipped with eMMC memory, not SD card.
> >  - The Exynos4210 SDHCI has few more registers, e.g. for
> >    controlling the clocks, additional status (0x80, 0x84, 0x8c). These
> >    are not implemented.
> >
> > Testing on smdkc210 machine with "-drive file=FILE,if=sd,bus=0,index=2".
> >
> > Signed-off-by: Krzysztof Kozlowski <address@hidden>
> >
> > ---
> >
> > Mostly it works:
> > [   11.763786] sdhci: Secure Digital Host Controller Interface driver
> > [   11.764593] sdhci: Copyright(c) Pierre Ossman
> > [   11.777295] s3c-sdhci 12530000.sdhci: clock source 2: mmc_busclk.2 
> > (12000000 Hz)
> > [   11.976250] mmc0: SDHCI controller on samsung-hsmmc [12530000.sdhci] 
> > using ADMA
> > [   11.980283] Synopsys Designware Multimedia Card Interface Driver
> > [   12.086757] mmc0: new SDHC card at address 4567
> > [   12.151653] mmcblk0: mmc0:4567 QEMU! 4.00 GiB
> >
> > ... except that for guest, the storage starts from 0x50000. It just
> > skips first 0x50000 bytes thus the paritions (MBR) and initial data is
> > not visible.
> >
> > I don't know what is the cause.
> >
> > Any hints?
> 
> That is strange and it sounds like we should try to track down
> what is going on there. Hopefully it shouldn't be too hard to trace
> through what happens when the guest accesses what it thinks is the
> first block on the SD card...
> 
> Otherwise I just have a couple of nits below.

Nah, I am just total dumbass. Skipped bytes from image are okay because
that is the header of QEMU image (e.g. QCOW2). The partitions were not
visible because I tried to attach snapshot of partition itself, not
entire disk.

Everything works fine.

> 
> > Signed-off-by: Krzysztof Kozlowski <address@hidden>
> > ---
> >  hw/arm/exynos4210.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> > index 0ec4250f0c05..d581f2217253 100644
> > --- a/hw/arm/exynos4210.c
> > +++ b/hw/arm/exynos4210.c
> > @@ -32,6 +32,7 @@
> >  #include "hw/arm/arm.h"
> >  #include "hw/loader.h"
> >  #include "hw/arm/exynos4210.h"
> > +#include "hw/sd/sd.h"
> >  #include "hw/usb/hcd-ehci.h"
> >
> >  #define EXYNOS4210_CHIPID_ADDR         0x10000000
> > @@ -72,6 +73,13 @@
> >  #define EXYNOS4210_EXT_COMBINER_BASE_ADDR   0x10440000
> >  #define EXYNOS4210_INT_COMBINER_BASE_ADDR   0x10448000
> >
> > +/* SD/MMC host controllers */
> > +#define EXYNOS4210_SDHCI_CAPABILITIES       0x05E80080
> > +#define EXYNOS4210_SDHCI_BASE_ADDR          0x12510000
> > +#define EXYNOS4210_SDHCI_ADDR(n)            (EXYNOS4210_SDHCI_BASE_ADDR + \
> > +                                                0x00010000 * (n))
> > +#define EXYNOS4210_SDHCI_NUMBER             4
> > +
> >  /* PMU SFR base address */
> >  #define EXYNOS4210_PMU_BASE_ADDR            0x10020000
> >
> > @@ -386,6 +394,27 @@ Exynos4210State *exynos4210_init(MemoryRegion 
> > *system_mem,
> >                             EXYNOS4210_UART3_FIFO_SIZE, 3, NULL,
> >                    s->irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP, 
> > 3)]);
> >
> > +    /*** SD/MMC host controllers ***/
> > +    for (n = 0; n < EXYNOS4210_SDHCI_NUMBER; n++) {
> > +        DeviceState *carddev;
> > +        DriveInfo *di;
> > +        BlockBackend *blk;
> > +
> > +        dev = qdev_create(NULL, "generic-sdhci");
> > +        qdev_prop_set_uint32(dev, "capareg", 
> > EXYNOS4210_SDHCI_CAPABILITIES);
> > +        qdev_init_nofail(dev);
> > +
> > +        busdev = SYS_BUS_DEVICE(dev);
> > +        sysbus_mmio_map(busdev, 0, EXYNOS4210_SDHCI_ADDR(n));
> > +        sysbus_connect_irq(busdev, 0, s->irq_table[exynos4210_get_irq(29, 
> > n)]);
> > +
> > +        di = drive_get_next(IF_SD);
> 
>     di = drive_get(IF_SD, 0, n);
> 
> would be better -- this explicitly states that SD cards 0,1,2,3
> connect to these controllers, rather than implicitly assigning
> whatever the "next" ones happen to be.

Sure.

> 
> > +        blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > +        carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), 
> > TYPE_SD_CARD);
> > +        qdev_prop_set_drive(carddev, "drive", blk, &error_abort);
> > +        qdev_prop_set_bit(carddev, "realized", true);
> 
> This isn't the right way to set the realized property. You can either:
>  (a) use qdev_init_nofail(), which sets the property and prints an
> error and exits if the device realize fails
>  (b) use object_property_set_bool() to set the property and handle
> errors yourself

I'll fix it.

> 
> > +    }
> > +
> >      /*** Display controller (FIMD) ***/
> >      sysbus_create_varargs("exynos4210.fimd", EXYNOS4210_FIMD0_BASE_ADDR,
> >              s->irq_table[exynos4210_get_irq(11, 0)],
> > --
> > 2.9.3
> 
> Incidentally someday maybe we should convert this Exynos4210 code
> to a proper QOM SoC container object, but that would be a lot of
> work.

Any existing platforms which I could take as an good example? Maybe I'll
have some time to do it.

Best regards,
Krzysztof




reply via email to

[Prev in Thread] Current Thread [Next in Thread]