Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

["Andreas Färber" <afaerber@suse.de>]

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 <pbonzini@redhat.com>
>>>>> wrote:
>>>>>> Il 25/06/2013 22:53, Peter Maydell ha scritto:
>>>>>>> On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> 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?

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg