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

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

On 07/17/2012 05:37 PM, Peter Maydell wrote:
> On 17 July 2012 13:55, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> On 07/16/2012 09:13 PM, Peter Maydell wrote:
>>> 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?
>> 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))"
> We should be modelling what the hardware does, not just what Linux happens
> to use.
>
> I've now gone and found the SDHCI specification. (figure 1-6 and table 1-6
> in the simplified spec v3.00 are relevant here.) What happens is that the
> SLOTINT bit tracks the external interrupt line's state, and that interrupt
> line is the logical combination of the various *sts/*sigen registers.
> I would suggest two functions:
>
> int sdhci_slotint(SDHCIState *s)
> which just calculates and returns the external interrupt line state
> (might be able to make this 'static'), and
>
> void sdhci_update_irq(SDHCIState *s)
> {
>      qemu_set_irq(sdhci_slotint(s));
> }
>
> Then just call sdhci_update_irq() any time you update state that
> can affect the interrupt line.
>
> The 'read the SLOTINT register' implementation just calls
> sdhci_slotint(), obviously.
>
> This approach:
>   * doesn't store any extra state in our state struct that the hardware
>     doesn't also have as stored state
>   * doesn't prematurely optimise the calculation of the interrupt line
>     state, so it's nice and clear how the irq line works and that it
>     follows the spec
>
> -- PMM
>

Ok, but I'd rather make sdhci_slotint() return uint8_t, to emphasize 
that this function returns
a value of hardware SLOTINT register.