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

[Peter Crosthwaite <peter.crosthwaite@petalogix.com>]

On Fri, 2012-06-29 at 11:59 +0100, Peter Maydell wrote:
> On 26 June 2012 06:13, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> 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