[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Igor Mitsyanko
Subject: Re: [Qemu-devel] [PULL] Standard SD host controller model
Date: Fri, 29 Jun 2012 18:52:41 +0400
User-agent: Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120615 Thunderbird/13.0.1

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
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.

Is there anything preventing the guest from setting up a set of
ADMA descriptors such that the loop in sdhci_start_adma never
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.

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]