From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48587) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmcCb-0001jI-6P for qemu-devel@nongnu.org; Wed, 04 Jul 2012 22:53:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SmcCY-0005M9-Tr for qemu-devel@nongnu.org; Wed, 04 Jul 2012 22:53:56 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:58393) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmcCY-0005Ed-Hz for qemu-devel@nongnu.org; Wed, 04 Jul 2012 22:53:54 -0400 Received: by pbbro12 with SMTP id ro12so13194742pbb.4 for ; Wed, 04 Jul 2012 19:53:51 -0700 (PDT) From: Peter Crosthwaite In-Reply-To: References: <1340687588-21585-1-git-send-email-peter.crosthwaite@petalogix.com> Content-Type: text/plain; charset="UTF-8" Date: Thu, 05 Jul 2012 12:53:43 +1000 Message-ID: <1341456823.3337.9.camel@PetaLogix-ws2> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL] Standard SD host controller model List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: aliguori@us.ibm.com, i.mitsyanko@samsung.com, e.voevodin@samsung.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, kyungmin.park@samsung.com, d.solodkiy@samsung.com, edgar.iglesias@gmail.com, m.kozlov@samsung.com, john.williams@petalogix.com On Fri, 2012-06-29 at 11:59 +0100, Peter Maydell wrote: > On 26 June 2012 06:13, Peter A. G. Crosthwaite > 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. > Recreating v5 as a series rather than a pull. > > Igor Mitsyanko (2): > > hw: introduce standard SD host controller > > sdhci.h: Igor has sent me an increment patch offlist. I have rolled his edits into patch 1/2. > 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 > Done. There where two typos and I though he was pointing out something else. Now fixed both. Apologies. > > xilinx_zynq: Added SD controllers > > Unnecessary braces. Removed > Any reason why you didn't use sysbus_create_simple() ? Not really. But sysbus is marked for imminent death by one of Anythonys series, so my logic there was the less sysbusisms the better. Depracating these little bits of wrapper logic in Sysbus and QDev brings us closer to the long term goals of QOM no? Regards, Peter > > -- PMM