Re: [Xen-devel] [PATCH v2 15/15] xen/arm: update GIC dt node with GIC v3 information

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Fri, Apr 4, 2014 at 9:30 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello Vijaya,
>
> On 04/04/2014 04:45 PM, Vijay Kilari wrote:
>> On Fri, Apr 4, 2014 at 7:52 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
>>> On 04/04/2014 12:56 PM, vijay.kilari@xxxxxxxxx wrote:
>>>> +    if ( hw_type == GIC_VERSION_V3 )
>>>> +    {
>>>> +        res = dt_property_read_u32(gic, "redistributor-stride", 
>>>> &rd_stride);
>>>> +        if ( !res )
>>>> +            rd_stride = 0;
>>>> +    }
>>>
>>>
>>> You have skipped some of my remarks on V1. I would definitely prefer to
>>> have a callback in your {v,}GIC structure which add the specific GIC
>>> properties following the version supported by DOM0.
>>>
>> Yes, I recognized your comment. My understanding is that while
>> creating this gic node for
>> dom0, we should follow the order in creating cells.
>> Also, the cells are allocated here and these should be passed to gic
>> driver to fill it.
>> So looks ugly.
>
> I don't understand the issue with the order and the allocation...
>
   I mean if we change the order of calling fdt_set_property for
various properties, the node creation goes wrong.  something like
this
  fdt_set_property(fdt, "mycommonprop2",..)
 fdt_set_property(fdt, "mycommonprop1",..)

I have given a try and dom0 fails to boot.

> You have bunch of code in if (hw_type == GICV3) than can be easily move
> to the GIC driver.
>
> You could have something like:
>
> fdt_set_property(fdt, "mycommonprop1",..)
> fdt_set_property(fdt, "mycommonprop2",..)
> gic_node_specific_properties();
> fdt_end();
>
>> I prefer completely move this code to gic.c and from gic.c based on
>> registered callback, let gic-{v2/v3}.c manage version specific dt cells 
>> update.
>> Is this OK?
>
> What do you mean?
>
 I mean why can't we move, why can't we move complete make_gic_node
to gic.c? as below and let it handle hw specific things?

fdt_set_property(fdt, "mycommonprop1",..)
fdt_set_property(fdt, "mycommonprop2",..)
gic_node_specific_properties();
fdt_end();

Anyways, I will relook and see if I can simplify as you suggested

> Regards,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel