Re: [Qemu-devel] [PATCH v5 2/4] exynos4210: Added SD host controller model

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

On 07/16/2012 09:13 PM, Peter Maydell wrote:
> On 5 July 2012 05:04, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> From: Igor Mitsyanko <i.mitsyanko@samsung.com>
>>
>> Custom Exynos4210 SD/MMC host controller, based on SD association standard host
>> controller ver. 2.00.
>>
>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> ---
>> changed from v4 (Igor):
>> set irq on SLOTINT status instead of interrupt registers status; instead;
> The IRQ handling code still looks really weird. I would expect
> that the code would be:
>    [code which updates various kinds of irq related state]
>    sdhci_update_irq();
>
> where sdhci_update_irq() calls qemu_set_irq() based on the state.
>
> At the moment it looks as if you're using slotint as a cached value
> of the expression
> "((s->norintsts & s->norintsigen) || (s->errintsts & s->errintsigen) ||
>   ((s->norintsts & SDHC_NIS_INSERT) && (s->wakcon & SDHC_WKUP_ON_INS)) ||
>   ((s->norintsts & SDHC_NIS_REMOVE) && (s->wakcon & SDHC_WKUP_ON_RMV)))"
>
> [can these two ever have different values?] and also attempting to
> shortcut by manually updating slotint in codepaths which change only
> parts of the state which this expression is testing. Why not just do
> things the simple and straightforward way and get rid of slotint
> completely?
>
> -- PMM
>

Linux seems to ignore SLOTINT status register, probably because it only 
supports single slot configuration while SLOTINT really required for 
multislot controllers only, so I think we can remove it completely and 
simply return 0 on reads. Same for status of WAKCON register, nobody 
cares about controller's wakeup functionality. Then update irq function 
could be simplified to

  "qemu_set_irq(s->irq, (s->norintsts & s->norintsigen) || (s->errintsts 
& s->errintsigen))"

I think thats how it was done originally. I'll send incremental patch to 
Peter off-list, if he and everyone else agree to handle interrupts this way.