From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH for-4.5 V6 14/14] Add ARM EFI boot support
Date: Wed, 24 Sep 2014 17:11:40 +0100
Message-ID: <5422ED3C.3090608@linaro.org>
References: <1411534992-27443-1-git-send-email-roy.franz@linaro.org>
	<1411534992-27443-15-git-send-email-roy.franz@linaro.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1411534992-27443-15-git-send-email-roy.franz@linaro.org>
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: Roy Franz <roy.franz@linaro.org>, xen-devel@lists.xen.org, ian.campbell@citrix.com, stefano.stabellini@citrix.com, tim@xen.org, jbeulich@suse.com, keir@xen.org
Cc: fu.wei@linaro.org
List-Id: xen-devel@lists.xenproject.org

Hi Roy,

On 09/24/2014 06:03 AM, Roy Franz wrote:
> +static int __init fdt_set_reg(void *fdt, int node, int addr_cells,
> +                              int size_cells, uint64_t addr, uint64_t len)
> +{
> +    uint8_t data[16]; /* at most 2 64 bit words */
> +    void *p = data;
> +
> +    /* Make sure that the values provided can be represented in
> +     * the reg property.
> +     */
> +    if ( addr_cells == 1 && (addr >> 32) )
> +        return -1;
> +    if ( size_cells == 1 && (len >> 32) )
> +        return -1;
> +
> +    if ( addr_cells == 1 )
> +    {
> +        *(uint32_t *)p = cpu_to_fdt32(addr);
> +        p += sizeof(uint32_t);
> +    }
> +    else if ( addr_cells == 2 )
> +    {
> +        *(uint64_t *)p = cpu_to_fdt64(addr);
> +        p += sizeof(uint64_t);
> +    }
> +    else
> +        return -1;
> +
> +    if ( size_cells == 1 )
> +    {
> +        *(uint32_t *)p = cpu_to_fdt32(len);
> +        p += sizeof(uint32_t);
> +    }
> +    else if ( size_cells == 2 )
> +    {
> +        *(uint64_t *)p = cpu_to_fdt64(len);
> +        p += sizeof(uint64_t);
> +    }
> +    else
> +        return -1;

You could give a look to dt_set_cell (xen/common/device_tree.c). I think
it will fit to your usage and avoid open coding size in this function.

> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index 166852d..ac9deae 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -4,17 +4,21 @@
>  #include <xen/guest_access.h>
>  #include <xen/irq.h>
>  #include <xen/time.h>
> -#include <asm/mc146818rtc.h>

NIT: Missing a blank line here.

Regards,

-- 
Julien Grall