Re: [Qemu-devel] [PULL] Standard SD host controller model

[Peter Maydell]

On 26 June 2012 06:13, Peter A. G. Crosthwaite
<address@hidden> wrote:
>  target-ppc: Fix 2nd parameter for tcg_gen_shri_tl (2012-06-24 22:52:11 +0200)
>
> are available in the git repository at:
>  git://developer.petalogix.com/public/qemu.git third-party/igor-sdhci.next

Sorry I haven't got round to reviewing this patch series earlier.

Pull requests should ideally be sent to the mailing list including
a mail for each patch in the series (eg see one of my recent arm-devs
pull requests), not just as the pull request email on its own.

> Igor Mitsyanko (2):
>      hw: introduce standard SD host controller

sdhci.h:
s/sdhc_not_stoped/sdhc_not_stopped/
s/stoped_state/stopped_state/
Interrupt handling code looks a little suspicious -- is s->slotint
supposed to track the state of the outgoing interrupt line or is
it distinct state?
Reset function needs to stop the qemu timers in case they were running.
In sdhci_read_dataport:
              if ((s->trnmod & SDHC_TRNS_MULTI) == 0 ||
                ((s->trnmod & SDHC_TRNS_MULTI) &&
                 (s->trnmod & SDHC_TRNS_BLK_CNT_EN) && (s->blkcnt == 0)) ||
                 /* stop at gap request */
                (s->stoped_state == sdhc_gap_read &&
                 !(s->prnsts & SDHC_DAT_LINE_ACTIVE))) {

the "(s->trnmod & SDHC_TRNS_MULTI)" clause is pointless because
you know it must be true if the first clause in the if failed.

Is there anything preventing the guest from setting up a set of
ADMA descriptors such that the loop in sdhci_start_adma never
terminates?

Does the uninit function need to stop the qemu timer? (I have no
idea, I haven't had to write an uninit function yet :-))

The Property array could use some comments documenting what the
properties do.

>      exynos4210: Added SD host controller model

Exynos4SDHCIState: field 'stoped_adma' should be 'stopped_adma'.
The conditional in exynos4210_sdhci_can_issue_command() is pretty
much unreadable, especially given the way it's indented. This one
is the worst offender and needs to be split up IMHO. There are
other conditionals in the code which aren't too bad but where
the linebreaking is unhelpful, eg:
        if (((sdhci->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
             (sdhci->blkcnt == 0)) || (attributes & SDHC_ADMA_ATTR_END)) {
would be clearer if the linebreaking matched the logic:
        if (((sdhci->trnmod & SDHC_TRNS_BLK_CNT_EN) && (sdhci->blkcnt == 0)) ||
            (attributes & SDHC_ADMA_ATTR_END)) {

The irq code looks dubious: we seem to have a single output
irq, but the code does things like:
    qemu_set_irq(sdhci->irq, sdhci->errintsigen & sdhci->errintsts);
but in other code paths:
    qemu_set_irq(sdhci->irq, sdhci->norintsigen & sdhci->norintsts);
It seems much more likely that the hardware has various interrupt
conditions which are effectively ORed together than that there
are some cases where the interrupt line is driven by one condition
and some where it's driven by the other.
Is it really a good idea for this class to define Properties which
are setting superclass struct fields?
exynos4210_sdhci_writefn(): the SDHC_CLKCON case could use a comment
/* Break out to superclass write to handle the rest of this register */
before the 'break' I think.

> Peter A. G. Crosthwaite (2):
>      vl.c: allow for repeated -sd arguments

This patch looks good but:
 * you haven't included Igor's Acked-by line
 * you haven't fixed the typo in the commit message Andreas pointed out

>      xilinx_zynq: Added SD controllers

Unnecessary braces.
Any reason why you didn't use sysbus_create_simple() ?

-- PMM