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

[Igor Mitsyanko <i.mitsyanko@samsung.com>]

On 09/18/2012 06:41 AM, Peter Crosthwaite wrote:
> Ping!
>
> Igor, are you able to provide a diff of this patch so I can send the
> next revision?

Sure, but I still don't understand what to do with QEMU-lockup issue, I 
believe the same topic was discussed here 
http://thread.gmane.org/gmane.comp.emulators.qemu/169524, and the 
decision was to use runtime loop detection?
> 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
>>>
>
>