On 17 July 2012 13:55, Igor Mitsyanko <address@hidden> 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