From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LBuHp-0002eM-RR for qemu-devel@nongnu.org; Sun, 14 Dec 2008 11:57:45 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LBuHo-0002dr-U9 for qemu-devel@nongnu.org; Sun, 14 Dec 2008 11:57:45 -0500 Received: from [199.232.76.173] (port=59190 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LBuHo-0002do-Mt for qemu-devel@nongnu.org; Sun, 14 Dec 2008 11:57:44 -0500 Received: from mx20.gnu.org ([199.232.41.8]:23335) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LBuHo-0004CX-A9 for qemu-devel@nongnu.org; Sun, 14 Dec 2008 11:57:44 -0500 Received: from mail.codesourcery.com ([65.74.133.4]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LBuHm-0006BF-Fa for qemu-devel@nongnu.org; Sun, 14 Dec 2008 11:57:42 -0500 From: Vladimir Prus Subject: Re: [Qemu-devel] SH: Improve the interrupt controller Date: Sun, 14 Dec 2008 19:57:36 +0300 References: <200812112252.17620.vladimir@codesourcery.com> <200812141646.mBEGkdGR017687@smtp11.dti.ne.jp> In-Reply-To: <200812141646.mBEGkdGR017687@smtp11.dti.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200812141957.37246.vladimir@codesourcery.com> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: takasi-y@ops.dti.ne.jp Cc: paul@codesourcery.com, qemu-devel@nongnu.org On Sunday 14 December 2008 19:46:39 takasi-y@ops.dti.ne.jp wrote: > # a patch attached is not to be commit to repos, but for explanation. > Hi, > > > I have tested this only with SH4A and it's desirable to test > > with 7751/R2D. However, I no longer sure I know which kernel > > to use for that. Can anybody either provide me with instructions, > > or test this patch with R2D for me? > > It doesn't work at least here for me (for r2d). > It stops with CPU load 100% (and I had to power my PC off...). Bummer. Can you send me the kernel/rootfs combo you are using (either link, or complete binary offlist) and I'll see what I've broken. > BTW, I think your patch is not following to current sh_intc's design model. > # please read to the end. this will be turned over later. > > I think its design model is > - registering memory regions for each registers. > - read/write function doesn't check address > But, you have add address check in read/write function. It seems to me that read/write function does check address. Consider: static uint32_t sh_intc_read(void *opaque, target_phys_addr_t offset) { ... sh_intc_locate(desc, (unsigned long)offset, &valuep, &enum_ids, &first, &width, &mode); ... } static void sh_intc_locate(struct intc_desc *desc, unsigned long address, unsigned long **datap, intc_enum **enums, unsigned int *first, unsigned int *width, unsigned int *modep) { .... if (desc->mask_regs) { ... mode = sh_intc_mode(address, mr->set_reg, mr->clr_reg); if (mode == INTC_MODE_NONE) continue; ... } ... } } So, read/write function do check addresses. On the other hand, each register is registered separately. > > Mine(which I have forgotten to send..., only to move icr) is attached at the > end of this mail. This one is based on the recognition above. > > It works, but I wonder if this model is valid or not on mmio system after #5849. > Especially here is in question, > + cpu_register_physical_memory(P4ADDR(base), 2, io_memory); > + cpu_register_physical_memory(A7ADDR(base), 2, io_memory); > > Paul(the author of #5849) said, > > [1] It's actually the offset from the start of the first page of that region. > > In practice this difference doesn't matter, and makes the implementation a > > lot simpler. > > I don't know why he thought it "doesn't matter", but from discussions on ML, > I guess the model he have is > - register one memory region for each modules. > - read/write function check address > - most of modules are aligned to page. It is my understanding too. It seems that it's not hard to move sh_intc to this model -- we just need to register entire region, as opposed to registering memory for each register. > In that case, your patch is nearer to the model. > But, then, we should done with these, > cpu_register_physical_memory_offset(P4ADDR(address), 4, > desc->iomemtype, INTC_A7(address)); > cpu_register_physical_memory_offset(A7ADDR(address), 4, > desc->iomemtype, INTC_A7(address)); Yes, both these, and sh_intc_register should not do piecewise registration if we decide to change model. - Volodya