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

[Julien Grall <julien.grall@linaro.org>]

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?

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.

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.

-- 
Julien Grall