From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46267) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ury57-0002bY-LQ for qemu-devel@nongnu.org; Wed, 26 Jun 2013 18:20:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ury56-0004F2-Fo for qemu-devel@nongnu.org; Wed, 26 Jun 2013 18:20:53 -0400 Received: from cantor2.suse.de ([195.135.220.15]:42120 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ury26-0003Jg-Vu for qemu-devel@nongnu.org; Wed, 26 Jun 2013 18:17:47 -0400 Message-ID: <51CB6887.2070602@suse.de> Date: Thu, 27 Jun 2013 00:17:43 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1372184516-32397-1-git-send-email-peter.maydell@linaro.org> <1372184516-32397-3-git-send-email-peter.maydell@linaro.org> <51C9E490.7090304@redhat.com> <51CA9086.80802@redhat.com> <51CB39C3.30707@tribudubois.net> <51CB5995.6020702@tribudubois.net> <51CB5AB8.9030409@redhat.com> <51CB63C4.1080307@tribudubois.net> In-Reply-To: <51CB63C4.1080307@tribudubois.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jean-Christophe DUBOIS Cc: Peter Maydell , Peter Crosthwaite , qemu-devel@nongnu.org, Anthony Liguori , Paul Brook , Paolo Bonzini Am 26.06.2013 23:57, schrieb Jean-Christophe DUBOIS: > On 06/26/2013 11:18 PM, Paolo Bonzini wrote: >> Il 26/06/2013 23:13, Jean-Christophe DUBOIS ha scritto: >>> On 06/26/2013 08:58 PM, Jean-Christophe DUBOIS wrote: >>>> On 06/26/2013 09:21 AM, Peter Crosthwaite wrote: >>>>> Hi >>>>> >>>>> On Wed, Jun 26, 2013 at 4:56 PM, Paolo Bonzini >>>>> wrote: >>>>>> Il 25/06/2013 22:53, Peter Maydell ha scritto: >>>>>>> On 25 June 2013 19:42, Paolo Bonzini wrote: >>>>>>>> Il 25/06/2013 20:21, Peter Maydell ha scritto: >>>>>>>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *de= v) >>>>>>>>> >>>>>>>>> sysbus_init_irq(dev, &s->irq); >>>>>>>>> memory_region_init_io(&s->iomem, &imx_timerg_ops, >>>>>>>>> - s, "imxg-timer", >>>>>>>>> + s, TYPE_IMX_GPT, >>>>>>>>> 0x00001000); >>>>>>>>> sysbus_init_mmio(dev, &s->iomem); >>>>>>>>> >>>>>>>> There was some agreement that this is not a good change. >>>>>>> I agree (and more so regarding the use of the macro in the >>>>>>> vmstate name), but nobody actually posted any comment to >>>>>>> that effect against any of the versions of this patch that >>>>>>> got sent out for review... >>>>>> Yeah, the timing was bad... Can you post a revert, though? >>>>>> >>>>> The original string being replaced was a poor choice as well. IIUC = the >>>>> consensus was string field of the memory regions is supposed to >>>>> indicate the purpose of the memory region for the device. Good >>>>> examples would be "Control regs" or "MMIO". Naming the memory regio= n >>>>> after the device type is a redundancy as that info will come via >>>>> memory region owners. >>>>> >>>>> So rather than revert could you just choose something more descript= ive? >>>> Peter (Maydell), >>>> >>>> How do you want to work this out? >>>> >>>> Do you revert it and we start over? >>>> >>>> Or should I provide a patch on top of the actual file to change the >>>> "wrong name"? >>>> >>>> Please advise. >>> I browsed through the various timer implementations in the hw/timer >>> directory and I was not able to spot one that would follow the >>> convention you indicated. >>> >>> Could you point me to a "good citizen" example? >> Here is one example (hw/pci-host/piix.c): >> >> memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s, >> "pci-conf-idx", 4); >> sysbus_add_io(dev, 0xcf8, &s->conf_mem); >> sysbus_init_ioports(&s->busdev, 0xcf8, 4); >> >> memory_region_init_io(&s->data_mem, &pci_host_data_le_ops, s, >> "pci-conf-data", 4); >> sysbus_add_io(dev, 0xcfc, &s->data_mem); >> sysbus_init_ioports(&s->busdev, 0xcfc, 4); >> >> Just reverting the changes to memory regions and vmstate names is enou= gh >> for now. >=20 > I tried to change the memory region name for the timer registers to > something not prepended wit the device name (I choose "regs") and here > is the result in the qemu console (I changes both EPIT and GPT timers). > We went from: >=20 > (qemu) info mtree > memory > 0000000000000000-7ffffffffffffffe (prio 0, RW): system > 000000001fffc000-000000001fffffff (prio 0, RW): kzm.sram > 0000000043f90000-0000000043f90fff (prio 0, RW): imx-serial > 0000000043f94000-0000000043f94fff (prio 0, RW): imx-serial > 0000000053f80000-0000000053f80fff (prio 0, RW): imx_ccm > 0000000053f90000-0000000053f90fff (prio 0, RW): imx.gpt > 0000000053f94000-0000000053f94fff (prio 0, RW): imx.epit > 0000000053f98000-0000000053f98fff (prio 0, RW): imx.epit > 0000000068000000-0000000068000fff (prio 0, RW): imx_avic > 0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram > 0000000088000000-000000008bffffff (prio 0, RW): alias ram.alias > @kzm.ram 0000000000000000-0000000003ffffff > 00000000b6000000-00000000b60000ff (prio 0, RW): lan9118-mmio > I/O > 0000000000000000-000000000000ffff (prio 0, RW): io > aliases > kzm.ram > 0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram > (qemu) >=20 > to: >=20 > (qemu) info mtree > memory > 0000000000000000-7ffffffffffffffe (prio 0, RW): system > 000000001fffc000-000000001fffffff (prio 0, RW): kzm.sram > 0000000043f90000-0000000043f90fff (prio 0, RW): imx-serial > 0000000043f94000-0000000043f94fff (prio 0, RW): imx-serial > 0000000053f80000-0000000053f80fff (prio 0, RW): imx_ccm > 0000000053f90000-0000000053f90fff (prio 0, RW): regs > 0000000053f94000-0000000053f94fff (prio 0, RW): regs > 0000000053f98000-0000000053f98fff (prio 0, RW): regs > 0000000068000000-0000000068000fff (prio 0, RW): imx_avic > 0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram > 0000000088000000-000000008bffffff (prio 0, RW): alias ram.alias > @kzm.ram 0000000000000000-0000000003ffffff > 00000000b6000000-00000000b60000ff (prio 0, RW): lan9118-mmio > I/O > 0000000000000000-000000000000ffff (prio 0, RW): io > aliases > kzm.ram > 0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram > (qemu) >=20 >=20 > Names don't feel really useful in the second case as they are > indistinguishable. Is the consensus around "generic" names (like MMIO o= r > "Ctrl regs") without adding reference to the device a good one for all > platforms? Paolo's series adds the MemoryRegion owner but hasn't been merged yet. Just revert the names so that Paolo's series applies cleanly. Any name changes can then still be done as follow-ups. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg