From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38033) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TDrWI-0008Rh-KF for qemu-devel@nongnu.org; Tue, 18 Sep 2012 02:42:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TDrWH-0003tP-8u for qemu-devel@nongnu.org; Tue, 18 Sep 2012 02:42:54 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:20638) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TDrWH-0003st-17 for qemu-devel@nongnu.org; Tue, 18 Sep 2012 02:42:53 -0400 Received: from eusync1.samsung.com (mailout4.w1.samsung.com [210.118.77.14]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MAJ008YD9CAAK00@mailout4.w1.samsung.com> for qemu-devel@nongnu.org; Tue, 18 Sep 2012 07:43:22 +0100 (BST) Received: from [106.109.9.185] by eusync1.samsung.com (Oracle Communications Messaging Server 7u4-23.01(7.0.4.23.0) 64bit (built Aug 10 2011)) with ESMTPA id <0MAJ000I99BAHH50@eusync1.samsung.com> for qemu-devel@nongnu.org; Tue, 18 Sep 2012 07:42:47 +0100 (BST) Message-id: <505817E6.6050109@samsung.com> Date: Tue, 18 Sep 2012 10:42:46 +0400 From: Igor Mitsyanko MIME-version: 1.0 References: <2d69772ed8963e572fb75f735043f2eb1c7ed4de.1344223191.git.peter.crosthwaite@petalogix.com> <501FB8B8.6050308@samsung.com> <1347936093.9401.0.camel@PetaLogix-ws2> In-reply-to: <1347936093.9401.0.camel@PetaLogix-ws2> Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 2/4] exynos4210: Added SD host controller model List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , e.voevodin@samsung.com, qemu-devel@nongnu.org, kyungmin.park@samsung.com, d.solodkiy@samsung.com, edgar.iglesias@gmail.com, m.kozlov@samsung.com, john.williams@petalogix.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 >>> 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 >>> > >