From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH ARM v4 11/12] mini-os: get GIC addresses
 from FDT
Date: Wed, 18 Jun 2014 18:25:03 +0100
Message-ID: <53A1CB6F.4050209@linaro.org>
References: <1403104106-32538-1-git-send-email-talex5@gmail.com>
	<1403104106-32538-12-git-send-email-talex5@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta4.messagelabs.com ([85.158.143.247])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <julien.grall@linaro.org>) id 1WxJbh-00019W-De
	for xen-devel@lists.xenproject.org; Wed, 18 Jun 2014 17:25:09 +0000
Received: by mail-wi0-f180.google.com with SMTP id hi2so1558380wib.1
	for <xen-devel@lists.xenproject.org>;
	Wed, 18 Jun 2014 10:25:07 -0700 (PDT)
In-Reply-To: <1403104106-32538-12-git-send-email-talex5@gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Thomas Leonard <talex5@gmail.com>, xen-devel@lists.xenproject.org
Cc: samuel.thibault@ens-lyon.org, stefano.stabellini@eu.citrix.com, Dave.Scott@eu.citrix.com, anil@recoil.org
List-Id: xen-devel@lists.xenproject.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