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

[Igor Mitsyanko]

On 06/29/2012 02:59 PM, Peter Maydell wrote:
> 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?

SLOTINT is supposed to help us track which SD slot generated interrupt 
request in multislot controller configuration. Each bit of SLOTINT is 
logically ORed interrupt and wakeup signals of single SD slot. We model 
controller with single slot hence only bit0 of SLOTINT is used.

> 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.
Ok
> Is there anything preventing the guest from setting up a set of
> ADMA descriptors such that the loop in sdhci_start_adma never
> terminates?
Nothing, according to specification length of data transfer is 
designated by descriptor table only. And also documentation states that 
if ADMA transfer was not terminated, it should be aborted by data timeout.
> Does the uninit function need to stop the qemu timer? (I have no
> idea, I haven't had to write an uninit function yet :-))
Right, I think not only it should stop this timer, but it also should 
free it, and free eject_cb and ro_cb also
> 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)) {
Ok, i'll try to make it more readable)
> 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.
Sure, full interrupt trigger condition would be
if ((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)))

but there's no point in using full check everywhere, if execution of 
some specific code block could result in generation of normal interrupt 
request only (bits in norintsts) or error interrupt request only (bits 
in errintsts), then its the only condition we need to check.
> Is it really a good idea for this class to define Properties which
> are setting superclass struct fields?
now that you mentioned it, capareg better be set to required value in 
exynos4210_sdhci_realize()
> 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.
Ok
> > 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() ?
Probably because it marked as legacy function?
> -- PMM