Re: [Qemu-devel] [PATCH v6 2/4] exynos4210: Added SD host controller model

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

Ping!

Igor, are you able to provide a diff of this patch so I can send the
next revision?

Regards,
Peter

On Mon, 2012-08-06 at 16:29 +0400, Igor Mitsyanko wrote:
> On 08/06/2012 02:56 PM, Peter Maydell wrote:
> > On 6 August 2012 04:25, Peter A. G. Crosthwaite
> > <peter.crosthwaite@petalogix.com> wrote:
> >
> >> +static uint64_t
> >> +exynos4210_sdhci_readfn(void *opaque, target_phys_addr_t offset, unsigned size)
> >> +{
> >> +    Exynos4SDHCIState *s = (Exynos4SDHCIState *)opaque;
> >> +    uint32_t ret;
> >> +
> >> +    switch (offset & ~0x3) {
> >> +    case SDHC_BDATA:
> >> +        /* Buffer data port read can be disabled by CONTROL2 register */
> >> +        if (s->control2 & EXYNOS4_SDHC_DISBUFRD) {
> >> +            ret = 0;
> >> +        } else {
> >> +            ret = SDHCI_GET_CLASS(s)->mem_read(SDHCI(s), offset, size);
> >> +        }
> >> +        break;
> >> +    case SDHC_ADMAERR:
> >> +        ret = (s->admaerr >> 8 * (offset - SDHC_ADMAERR)) &
> >> +                ((1 << 8 * size) - 1);
> > If size == 4 you've just shifted right by 32, which is undefined behaviour
> > when ints are 32 bits. Try
> >
> >     ret = extract32(s->admaerr, (offset & 3) << 3, size * 8);
> >
> > and similarly below.
> 
> Ok
> 
> >> +static void exynos4210_sdhci_writefn(void *opaque, target_phys_addr_t offset,
> >> +        uint64_t val, unsigned size)
> >> +{
> >> +    Exynos4SDHCIState *s = (Exynos4SDHCIState *)opaque;
> >> +    SDHCIState *sdhci = SDHCI(s);
> >> +    unsigned shift;
> >> +
> >> +    DPRINT_L2("write %ub: addr[0x%04x] <- %u(0x%x)\n", size, (uint32_t)offset,
> >> +            (uint32_t)val, (uint32_t)val);
> >> +
> >> +    switch (offset) {
> >> +    case SDHC_CLKCON:
> >> +        if ((val & SDHC_CLOCK_SDCLK_EN) &&
> >> +                (sdhci->prnsts & SDHC_CARD_PRESENT)) {
> >> +            val |= EXYNOS4_SDHC_SDCLK_STBL;
> >> +        } else {
> >> +            val &= ~EXYNOS4_SDHC_SDCLK_STBL;
> >> +        }
> >> +        /* Break out to superclass write to handle the rest of this register */
> >> +        break;
> >> +    case EXYNOS4_SDHC_CONTROL2 ... EXYNOS4_SDHC_CONTROL2 + 3:
> > Why do we switch (offset & 3) in the readfn but switch (offset)
> > and use case FOO ... FOO + 3 in the writefn? Consistency would be
> > nice.
> 
> I think I'll change readfn() switch to match writefn then, to avoid 
> complicating SDHC_CLKON case.
> 
> >> +        shift = (offset - EXYNOS4_SDHC_CONTROL2) * 8;
> >> +        s->control2 = (s->control2 & ~(((1 << 8 * size) - 1) << shift)) |
> >> +                (val << shift);
> >    s->control2 = deposit32(s->control2, (offset & 3) << 3, size * 8, val);
> >
> > and similarly below.
> >
> >> +    case SDHC_ADMAERR ... SDHC_ADMAERR + 3:
> >> +        if (size == 4 || (size == 2 && offset == SDHC_ADMAERR) ||
> >> +                (size == 1 && offset == (SDHC_ADMAERR + 1))) {
> >> +            uint32_t mask = 0;
> >> +
> >> +            if (size == 2) {
> >> +                mask = 0xFFFF0000;
> >> +            } else if (size == 1) {
> >> +                mask = 0xFFFF00FF;
> >> +                val <<= 8;
> >> +            }
> >> +
> >> +            s->admaerr = (s->admaerr & (mask | EXYNOS4_SDHC_FINAL_BLOCK |
> >> +               EXYNOS4_SDHC_IRQ_STAT)) | (val & ~(EXYNOS4_SDHC_FINAL_BLOCK |
> >> +               EXYNOS4_SDHC_IRQ_STAT | EXYNOS4_SDHC_CONTINUE_REQ));
> >> +            s->admaerr &= ~(val & EXYNOS4_SDHC_IRQ_STAT);
> >> +            if ((s->stopped_adma) && (val & EXYNOS4_SDHC_CONTINUE_REQ) &&
> >> +                (SDHC_DMA_TYPE(sdhci->hostctl) == SDHC_CTRL_ADMA2_32)) {
> >> +                s->stopped_adma = false;
> >> +                SDHCI_GET_CLASS(sdhci)->do_adma(sdhci);
> >> +            }
> >> +        } else {
> >> +            uint32_t mask = (1 << (size * 8)) - 1;
> >> +            shift = 8 * (offset & 0x3);
> >> +            val <<= shift;
> >> +            mask = ~(mask << shift);
> >> +            s->admaerr = (s->admaerr & mask) | val;
> >> +        }
> >> +        return;
> > This case just looks odd. I think it would be clearer to first
> > calculate the updated value of admaerr (using deposit32) and
> > then act on the changes (xor of old and new value is handy
> > to identify which bits are changed).
> 
> ok
> >
> > -- PMM
> >
>