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

[Igor Mitsyanko]

On 08/06/2012 02:56 PM, Peter Maydell wrote:
> 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.

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