From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35555) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sr9ES-0005wP-8K for qemu-devel@nongnu.org; Tue, 17 Jul 2012 10:58:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sr9EN-0003oS-R9 for qemu-devel@nongnu.org; Tue, 17 Jul 2012 10:58:36 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:58032) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sr9EN-0003oC-Ky for qemu-devel@nongnu.org; Tue, 17 Jul 2012 10:58:31 -0400 Received: from eusync2.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 <0M7B00CIB8AKKYA0@mailout4.w1.samsung.com> for qemu-devel@nongnu.org; Tue, 17 Jul 2012 15:59:08 +0100 (BST) Received: from [106.109.9.94] by eusync2.samsung.com (Oracle Communications Messaging Server 7u4-23.01(7.0.4.23.0) 64bit (built Aug 10 2011)) with ESMTPA id <0M7B006U08974850@eusync2.samsung.com> for qemu-devel@nongnu.org; Tue, 17 Jul 2012 15:58:28 +0100 (BST) Message-id: <50057D8C.8090705@samsung.com> Date: Tue, 17 Jul 2012 18:58:20 +0400 From: Igor Mitsyanko MIME-version: 1.0 References: <0020234e7fc3001f66ef67a1d3e19a01b6e498a5.1341457220.git.peter.crosthwaite@petalogix.com> <500560A6.9000500@samsung.com> In-reply-to: Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 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 07/17/2012 05:37 PM, Peter Maydell wrote: > On 17 July 2012 13:55, Igor Mitsyanko wrote: >> On 07/16/2012 09:13 PM, Peter Maydell wrote: >>> The IRQ handling code still looks really weird. I would expect >>> that the code would be: >>> [code which updates various kinds of irq related state] >>> sdhci_update_irq(); >>> >>> where sdhci_update_irq() calls qemu_set_irq() based on the state. >>> >>> At the moment it looks as if you're using slotint as a cached value >>> of the expression >>> "((s->norintsts & s->norintsigen) || (s->errintsts & s->errintsigen) || >>> ((s->norintsts & SDHC_NIS_INSERT) && (s->wakcon & SDHC_WKUP_ON_INS)) || >>> ((s->norintsts & SDHC_NIS_REMOVE) && (s->wakcon & SDHC_WKUP_ON_RMV)))" >>> >>> [can these two ever have different values?] and also attempting to >>> shortcut by manually updating slotint in codepaths which change only >>> parts of the state which this expression is testing. Why not just do >>> things the simple and straightforward way and get rid of slotint >>> completely? >> Linux seems to ignore SLOTINT status register, probably because it only >> supports single slot configuration while SLOTINT really required for >> multislot controllers only, so I think we can remove it completely and >> simply return 0 on reads. Same for status of WAKCON register, nobody cares >> about controller's wakeup functionality. Then update irq function could be >> simplified to >> >> "qemu_set_irq(s->irq, (s->norintsts & s->norintsigen) || (s->errintsts & >> s->errintsigen))" > We should be modelling what the hardware does, not just what Linux happens > to use. > > I've now gone and found the SDHCI specification. (figure 1-6 and table 1-6 > in the simplified spec v3.00 are relevant here.) What happens is that the > SLOTINT bit tracks the external interrupt line's state, and that interrupt > line is the logical combination of the various *sts/*sigen registers. > I would suggest two functions: > > int sdhci_slotint(SDHCIState *s) > which just calculates and returns the external interrupt line state > (might be able to make this 'static'), and > > void sdhci_update_irq(SDHCIState *s) > { > qemu_set_irq(sdhci_slotint(s)); > } > > Then just call sdhci_update_irq() any time you update state that > can affect the interrupt line. > > The 'read the SLOTINT register' implementation just calls > sdhci_slotint(), obviously. > > This approach: > * doesn't store any extra state in our state struct that the hardware > doesn't also have as stored state > * doesn't prematurely optimise the calculation of the interrupt line > state, so it's nice and clear how the irq line works and that it > follows the spec > > -- PMM > Ok, but I'd rather make sdhci_slotint() return uint8_t, to emphasize that this function returns a value of hardware SLOTINT register.