Re: [Xen-devel] [PATCH ARM v4 11/12] mini-os: get GIC addresses from FDT

[Thomas Leonard <talex5@xxxxxxxxx>]

On 18 June 2014 18:25, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Thomas,
>
> On 06/18/2014 04:08 PM, Thomas Leonard wrote:
>>  //#define VGIC_DEBUG
>>  #ifdef VGIC_DEBUG
>> @@ -168,9 +169,38 @@ static void gic_handler(void) {
>>  }
>>
>>  void gic_init(void) {
>> -    // FIXME Get from dt!
>> -    gic.gicd_base = (char *)0x2c001000ULL;
>> -    gic.gicc_base = (char *)0x2c002000ULL;
>> +    gic.gicd_base = NULL;
>
> Any reason to not fold this patch in patch #7? Or better move the gic
> code in a separate patch?

It was previously requested that I split the FDT patch from the main
ARM one. This patch depends on libfdt being present, so it has to go
after that.

Moving all the GIC code to this patch would mean that the original
patch wouldn't work on its own.

> This would avoid to hardcode a GIC address which is completely wrong
> with Xen unstable.
>
>> +    int node = 0;
>> +    int depth = 0;
>> +    for (;;)
>> +    {
>> +        node = fdt_next_node(device_tree, node, &depth);
>> +        if (node <= 0 || depth < 0)
>> +            break;
>> +
>> +        /*
>> +           int name_len = 0;
>> +           const char *name = fdt_get_name(device_tree, node, &name_len);
>> +           printk("Found node: %d (%.*s)\n", node, name_len, name);
>> +         */
>
> This should be drop.
>
>> +
>> +        if (fdt_getprop(device_tree, node, "interrupt-controller", NULL)) {
>
> You have to check the compatible string here.
>
>> +            int len = 0;
>> +            const uint64_t *reg = fdt_getprop(device_tree, node, "reg", 
>> &len);
>> +            if (reg == NULL || len != 32) {
>
> You made assumption of the layout of the device tree provided by Xen:
>         - #address-cells == #size-cells == 2
>         - regs contains a valid physical address, i.e the device is not under 
> a bus
>
> This can be changed by the toolstack in the future and will likely break
> mini-os.

Is that likely? Seems like using BUG here is the right thing to do
until that happens.

> I would add a layer to support FDT address and interrupt translation
> correctly. I'm not the maintainer of the mini-os, so I let them decide :).
>
> I think, FreeBSD provides a good library for the translation (see
> sys/dev/fdt/fdt_common.c).
>
> I spent lots of time to play with device tree on Xen side, so I can help
> you if you needed.

That would be very helpful - thanks!



-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

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