From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50767) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyMRT-00021Y-Ie for qemu-devel@nongnu.org; Mon, 06 Aug 2012 08:29:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SyMRS-0002zU-Ch for qemu-devel@nongnu.org; Mon, 06 Aug 2012 08:29:51 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:54884) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyMRS-0002yO-6p for qemu-devel@nongnu.org; Mon, 06 Aug 2012 08:29:50 -0400 Received: from eusync4.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by mailout1.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0M8C00ELB2QDPQ50@mailout1.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 06 Aug 2012 13:30:13 +0100 (BST) Received: from [106.109.9.232] by eusync4.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTPA id <0M8C00IR22PKAB30@eusync4.samsung.com> for qemu-devel@nongnu.org; Mon, 06 Aug 2012 13:29:45 +0100 (BST) Message-id: <501FB8B8.6050308@samsung.com> Date: Mon, 06 Aug 2012 16:29:44 +0400 From: Igor Mitsyanko MIME-version: 1.0 References: <2d69772ed8963e572fb75f735043f2eb1c7ed4de.1344223191.git.peter.crosthwaite@petalogix.com> In-reply-to: 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 Maydell Cc: e.voevodin@samsung.com, qemu-devel@nongnu.org, "Peter A. G. Crosthwaite" , kyungmin.park@samsung.com, d.solodkiy@samsung.com, edgar.iglesias@gmail.com, m.kozlov@samsung.com, john.williams@petalogix.com 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 >