Re: [Qemu-devel] [PATCH v1 1/2] SDHCI: inital version

[Igor Mitsyanko <i.mitsyanko@samsung.com>]

On 04/02/2012 05:47 PM, Peter Crosthwaite wrote:
>>
>> Yes, I've been trying to get my sdhc accepted since last year :) I tried to
>> comply with specification entirely, your implementation is obviously much
>> smaller but enough for use with Linux driver (i've tested it with exynos
>> board emulation).
> Does it work, can you add exynos support for SDHCI using this version?
Yes, it works fine with upstream Linux kernel driver (linux boots with 
rootfs on sd card),
  but it doesn't work with exynos-specific-features-aware drivers from 
u-boot for example.

> Do you want to for the sake of immediate exynos sdhci support?
>
Sure, sdhc support in upstream would be great.
>   I have some comments though:
>> 1) It doesn't compile because adma_stride could be used uninitialized in
>> sdhci_dma_transfer() (QEMU is compiled with -Werror);
> Ack, will fix v2
>
>> 2) You shouldn't hw_error() on bad reads/writes.
>> 3) Why do you register memory region with 0x200 size and then print error
>> messages on read and writes in a range 0x100-0x200?
> I can look into cleaning up the undefined behaviours, but for the
> implementation i have (as in the real hardware) bad reads and writes
> perform no action. This is why I just have a warning message rather
> than a hw error.
>
Currently you hw_error on reads/writes from addresses <0x100 which are
not listed in switch statement, for example exynos have registers at 
offsets 0x80, 0x84
and QEMU aborts when driver tries to read/write them.
>> 4) Controller transfers data in blocks of blksize (512 usually) bytes, but
>> you transfer data immediately on writes to registers, you probably should
>> use a buffer for this.
>> 5) You assume that BUFFER_DATAPORT register is always accessed with 4 byte
>> system bus transfers, but it's not true in general.
>> 6) SDMA transfer must continue when you write to the upper byte of
>> SDMA_SYSAD register if it was stoped at page boundary.
>> 7) Maybe it's not important, but you have no support of 64-bit target
>> emulation.
> Yeh these behaviours are outside the implemented subset.
>
>> 8) Why "sysbus_sdhci" instead of simple "sdhci"?
> The original implementation was a dual PCI as well as sysbus. If
> vincent or anyone else wants to put the PCI qdev back in they can.
> the sysbus prefix is thinking forward for when there needs to be a
> distinction between pci_sdhci and sysbus_sdhci.

-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com