Re: [Qemu-devel] [Qemu-trivial] [PATCH] hw/arm/highbank: Simplify code (memory region in device state)

[Peter Crosthwaite]

On Sat, Dec 7, 2013 at 5:24 AM, Peter Maydell <address@hidden> wrote:
> On 6 December 2013 19:12, Michael Tokarev <address@hidden> wrote:
>> 06.12.2013 22:43, Stefan Weil wrote:
>>> The memory region can be included by value instead of by reference in the
>>> device state (like it is done in other SoCs).

So following on from the logic below you'd be best to just drop the
parenthetic in the commit message. The change isn't exactly aligned to
the current ARM SOCification efforts. Just the sentance on its own is
enough motivation.

>>>
>>> Signed-off-by: Stefan Weil <address@hidden>
>>> ---
>>>  hw/arm/highbank.c |    7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
>>> index fe98ef1..54384b3 100644
>>> --- a/hw/arm/highbank.c
>>> +++ b/hw/arm/highbank.c
>>> @@ -125,7 +125,7 @@ typedef struct {
>>>      SysBusDevice parent_obj;
>>>      /*< public >*/
>>>
>>> -    MemoryRegion *iomem;
>>> +    MemoryRegion iomem;
>>>      uint32_t regs[NUM_REGS];
>>>  } HighbankRegsState;
>>
>> Don't we have active maintainer for arm? (Who is Cc'd on the original patch).
>
> Yeah, and there's highbank related patches currently going through
> review so it might be better for me to take this in the target-arm queue
> (though for a trivial one like this it doesn't make much difference).
>
>>> @@ -154,10 +154,9 @@ static int highbank_regs_init(SysBusDevice *dev)
>>>  {
>>>      HighbankRegsState *s = HIGHBANK_REGISTERS(dev);
>>>
>>> -    s->iomem = g_new(MemoryRegion, 1);
>>> -    memory_region_init_io(s->iomem, OBJECT(s), &hb_mem_ops, s->regs,
>>> +    memory_region_init_io(&s->iomem, OBJECT(s), &hb_mem_ops, s->regs,
>>>                            "highbank_regs", 0x1000);
>>
>> I know right to nothing about arm, does it have any alignment requiriments,
>> which may break here?
>
> Hmm? This is just putting the MemoryRegion struct in the HighBankRegsState
> structure rather than allocating it via g_new(). That's all host related
> and in any case is the host C compiler's problem to deal with.
>

So I guess the correct course of action (ultimately) is to:

A: Core-ify these registers.
B: Factor out all devices including Coreified regs into a SoC container.

The state struct being change here would correspond to the corified
register block and not the SoC container.

But if you were to execute step A, you would make this change upon
migration of the device state struct to the new device. The patch also
simply stands in its own right as just the right thing to do. So.

> Reviewed-by: Peter Maydell <address@hidden>
>

Reviewed-by: Peter Crosthwaite <address@hidden>

> thanks
> -- PMM
>