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

[Peter Maydell]

On 6 August 2012 04:25, Peter A. G. Crosthwaite
<address@hidden> 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.

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

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

-- PMM