Re: [Qemu-devel] [PATCH v3 4/7] bcm2835_peripherals: add rollup device for bcm2835 peripherals

[Alistair Francis]

On Wed, Jan 6, 2016 at 5:32 AM, Peter Crosthwaite
<address@hidden> wrote:
> On Tue, Jan 5, 2016 at 10:07 PM, Andrew Baumann
> <address@hidden> wrote:
>>> From: Alistair Francis [mailto:address@hidden
>>> Sent: Tuesday, 5 January 2016 18:14
>>> On Thu, Dec 31, 2015 at 4:31 PM, Andrew Baumann
>>> <address@hidden> wrote:
>>> > This device maintains all the non-CPU peripherals on bcm2835 (Pi1)
>>> > which are also present on bcm2836 (Pi2). It also implements the
>>> > private address spaces used for DMA and mailboxes.
>> [...]
>>> > +    obj = object_property_get_link(OBJECT(dev), "ram", &err);
>>> > +    if (obj == NULL) {
>>> > +        error_setg(errp, "%s: required ram link not found: %s",
>>> > +                   __func__, error_get_pretty(err));
>>> > +        return;
>>> > +    }
>>>
>>> I only had a quick read of this patch, but this RAM linking looks fine
>>> to me. Out of curiosity is there a reason you use
>>> object_property_get_link() instead of object_property_add_link() in
>>> the init?
>>
>
> The const link system removes the need for the object to have storage
> for the link pointer in state. This means you don't need the state
> field or add_link(), but the only way to get the pointer for your own
> use is to get_link() on yourself. This is slightly simpler but has the
> disadvantage that you cannot unlink and then relink something else (I
> think?).

Ok, that makes sense.

Thanks,

Alistair

>
> I don't have an opinion over which way is more correct so both are
> fine for me but if the QOM people have a preferred style we should
> probably make the two patches consistent.
>
> Regards,
> Peter
>
>> I'm not sure I understand your question... it wouldn't work the other way. I 
>> allocate the ram and add the link using object_property_add_const_link() in 
>> hw/arm/raspi.c. This file needs to consume the ram to setup alias mappings, 
>> so it is using get_link(). (Note there's also level of indirection; raspi 
>> creates bcm2836, which does nothing but get the link set by its parent and 
>> add it to its bcm2835_peripherals child.)
>>
>> I suppose I could do it the other way around (allocate and set link in 
>> bcm2835_peripherals, based on a size passed from the board), but it seemed 
>> more logical to treat the RAM as created/owned of the board rather than the 
>> SoC.
>>
>> Cheers,
>> Andrew