From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45396) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SEi3h-000814-FX for qemu-devel@nongnu.org; Mon, 02 Apr 2012 10:16:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SEi3c-0005tv-54 for qemu-devel@nongnu.org; Mon, 02 Apr 2012 10:16:37 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:16385) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SEi3b-0005tk-Uo for qemu-devel@nongnu.org; Mon, 02 Apr 2012 10:16:32 -0400 Received: from euspt1 (mailout2.w1.samsung.com [210.118.77.12]) by mailout2.w1.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTP id <0M1U00G2ZVNCCA@mailout2.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 02 Apr 2012 15:16:24 +0100 (BST) Received: from [106.109.8.162] by spt1.w1.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTPA id <0M1U00FA8VNC6D@spt1.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 02 Apr 2012 15:16:26 +0100 (BST) Date: Mon, 02 Apr 2012 18:16:24 +0400 From: Igor Mitsyanko In-reply-to: Message-id: <4F79B4B8.3040900@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7BIT References: <4F795CB7.9060404@suse.de> <4F796593.1080306@samsung.com> <4F79AA18.9070202@samsung.com> Subject: Re: [Qemu-devel] [PATCH v1 1/2] SDHCI: inital version Reply-To: i.mitsyanko@samsung.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Kevin Wolf , Peter Maydell , vpalatin@chromium.org, qemu-devel@nongnu.org, paul@codesourcery.com, duyl@xilinx.com, linnj@xilinx.com, edgar.iglesias@gmail.com, =?ISO-8859-1?Q?Andreas_F=E4rber?= , john.williams@petalogix.com, Dmitry Solodkiy On 04/02/2012 05:47 PM, Peter Crosthwaite wrote: >> >> Yes, I've been trying to get my sdhc accepted since last year :) I tried to >> comply with specification entirely, your implementation is obviously much >> smaller but enough for use with Linux driver (i've tested it with exynos >> board emulation). > Does it work, can you add exynos support for SDHCI using this version? Yes, it works fine with upstream Linux kernel driver (linux boots with rootfs on sd card), but it doesn't work with exynos-specific-features-aware drivers from u-boot for example. > Do you want to for the sake of immediate exynos sdhci support? > Sure, sdhc support in upstream would be great. > I have some comments though: >> 1) It doesn't compile because adma_stride could be used uninitialized in >> sdhci_dma_transfer() (QEMU is compiled with -Werror); > Ack, will fix v2 > >> 2) You shouldn't hw_error() on bad reads/writes. >> 3) Why do you register memory region with 0x200 size and then print error >> messages on read and writes in a range 0x100-0x200? > I can look into cleaning up the undefined behaviours, but for the > implementation i have (as in the real hardware) bad reads and writes > perform no action. This is why I just have a warning message rather > than a hw error. > Currently you hw_error on reads/writes from addresses <0x100 which are not listed in switch statement, for example exynos have registers at offsets 0x80, 0x84 and QEMU aborts when driver tries to read/write them. >> 4) Controller transfers data in blocks of blksize (512 usually) bytes, but >> you transfer data immediately on writes to registers, you probably should >> use a buffer for this. >> 5) You assume that BUFFER_DATAPORT register is always accessed with 4 byte >> system bus transfers, but it's not true in general. >> 6) SDMA transfer must continue when you write to the upper byte of >> SDMA_SYSAD register if it was stoped at page boundary. >> 7) Maybe it's not important, but you have no support of 64-bit target >> emulation. > Yeh these behaviours are outside the implemented subset. > >> 8) Why "sysbus_sdhci" instead of simple "sdhci"? > The original implementation was a dual PCI as well as sysbus. If > vincent or anyone else wants to put the PCI qdev back in they can. > the sysbus prefix is thinking forward for when there needs to be a > distinction between pci_sdhci and sysbus_sdhci. -- Mitsyanko Igor ASWG, Moscow R&D center, Samsung Electronics email: i.mitsyanko@samsung.com