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

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

On 04/02/2012 03:05 PM, Peter Crosthwaite wrote:
> On Mon, Apr 2, 2012 at 6:38 PM, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>> On 04/02/2012 12:00 PM, Andreas Färber wrote:
>>>
>>> Am 02.04.2012 09:20, schrieb Peter Maydell:
>>>>
>>>> On 2 April 2012 07:24, Peter A. G. Crosthwaite
>>>> <peter.crosthwaite@petalogix.com>    wrote:
>>>>>
>>>>> device more for standard SD host controller interface (SDHCI).
>>>>>
>>>>> Signed-off-by: Peter A. G. Crosthwaite<peter.crosthwaite@petalogix.com>
>>>>
>>>>
>>>> So how does this compare with Vincent Palatin's version?
>>>> (http://patchwork.ozlabs.org/patch/106767/) I'm guessing from
>>>> the copyright string that it's a modified version -- it would
>>>> be nice to say what the differences are.
>>>
>>>
>>> ...and what the differences to Samsung's version are.
>>>
>>> We should probably also cc Kevin on the topic.
>>>
>>> Andreas
>>
>>
>> It looks like this sdhc implements version 1 of standard SDHC specification,
>> while ours implements second version.
>
> The implementation is not a fully 100% complete SDHCI (indeed very few
> QEMU models of anything are 100% true to the hardware),   Id have to
> have a good look through the spec to give you what v2 features are and
> arent implemented, but this is supposed to have some level of v2
> support.
>
>   Second version should be backwards
>> compatible with first,
>
> Yes, I had to add somebackward compatibility to Vincents model around
> the ADMA stuff.
>
> I didn't want to submit it yet to see if vmstate
>> issue will be resolved or not. And also the whole QEMU SD emulation scheme
>> needs some refactoring which I've been busy with for a while now. The
>> biggest issues are that currently we cannot hot-insert SD card into virtual
>> board interface and we do not take any advantage of QOM (which is hard until
>> Paolo's and Andrea's patches land in master).
>> The one advantage of our SDHCI implementation is that it provides interface
>> to implement SoC-specific successors of standart SDHCI. I'll send these
>> patches today (even though I wanted to do a lot more refactoring with them),
>> Peter, could you please test them to find out if you can use them instead of
>> your SDHCI implementation?
>
> Yes i can but will they these patches be immediately mergable?
>
> My primary motivation for pushing this is to get my platform upstream
> and maintained. To that end, if major construction effort is required
> to get your SDHCI up to acceptable (which you have indicated in your
> comments) then can we push this anyway, and then when your refactoring
> happens, your model can replace this model.

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). 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);
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?
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.
8) Why "sysbus_sdhci" instead of simple "sdhci"?



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