|
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 <address@hidden>
wrote:
Il 25/06/2013 22:53, Peter Maydell ha scritto:
On 25 June 2013 19:42, Paolo Bonzini <address@hidden> wrote:
Il 25/06/2013 20:21, Peter Maydell ha scritto:
@@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
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 region
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 descriptive?
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 enough
for now.
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:
(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)
to:
(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)
Names don't feel really useful in the second case as they are
indistinguishable. Is the consensus around "generic" names (like
MMIO or "Ctrl regs") without adding reference to the device a good
one for all platforms?
JC
Paolo
|