Re: [Xen-devel] [PATCH V2 09/33] xen/arm: Add helpers to retrieve an address from the device tree

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> ---
>  xen/common/device_tree.c      |  343 
> +++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h |   22 +++
>  2 files changed, 365 insertions(+)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 449c332..8d37018 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -62,10 +62,38 @@ static void (*dt_printk)(const char *fmt, ...) = 
> early_printk;
> 
>  #ifdef DEBUG_DT
>  # define dt_dprintk(fmt, args...) dt_printk(XENLOG_DEBUG fmt, ##args)
> +static void dt_dump_addr(const char *s, const __be32 *addr, int na)
> +{
> +    dt_dprintk("%s", s);
> +    while ( na-- )
> +        dt_dprintk(" %08x", be32_to_cpu(*(addr++)));
> +    dt_dprintk("\n");
> +}
>  #else
>  # define dt_dprintk(fmt, args...) do {} while ( 0 )
> +static void dt_dump_addr(const char *s, const __be32 *addr, int na) { }
>  #endif
> 
> +#define DT_BAD_ADDR ((u64)-1)
> +
> +/* Max address size we deal with */
> +#define DT_MAX_ADDR_CELLS 4
> +#define DT_CHECK_ADDR_COUNT(na) ((na) > 0 && (na) <= DT_MAX_ADDR_CELLS)
> +#define DT_CHECK_COUNTS(na, ns) (DT_CHECK_ADDR_COUNT(na) && (ns) > 0)
> +
> +/* Callbacks for bus specific translators */
> +struct dt_bus
> +{
> +    const char *name;
> +    const char *addresses;
> +    int (*match)(const struct dt_device_node *parent);
> +    void (*count_cells)(const struct dt_device_node *child,
> +                        int *addrc, int *sizec);
> +    u64        (*map)(__be32 *addr, const __be32 *range, int na, int ns, int 
> pna);
> +       int (*translate)(__be32 *addr, u64 offset, int na);
> +       unsigned int (*get_flags)(const __be32 *addr);

Spacing is weird here. Hard tabs perhaps?

> +static int dt_translate_one(const struct dt_device_node *parent,
> +                            const struct dt_bus *bus,
> +                            const struct dt_bus *pbus,
> +                            __be32 *addr, int na, int ns,
> +                            int pna, const char *rprop)
> +{
> +    const __be32 *ranges;
> +    unsigned int rlen;
> +    int rone;
> +    u64 offset = DT_BAD_ADDR;
> +
> +    ranges = dt_get_property(parent, rprop, &rlen);
> +    if ( ranges == NULL )
> +    {
> +               dt_printk(XENLOG_ERR "DT: no ranges; cannot translate\n");
> +               return 1;
> +       }

Indentation.

> +    if ( ranges == NULL || rlen == 0 )
> +    {
> +        offset = dt_read_number(addr, na);
> +        memset(addr, 0, pna * 4);
> +        dt_dprintk("DT: empty ranges; 1:1 translation\n");
> +        goto finish;
> +    }
> +
> +    dt_dprintk("DT: walking ranges...\n");
> +
> +    /* Now walk through the ranges */
> +    rlen /= 4;
> +    rone = na + pna + ns;
> +    for ( ; rlen >= rone; rlen -= rone, ranges += rone )
> +    {
> +        offset = bus->map(addr, ranges, na, ns, pna);
> +        if ( offset != DT_BAD_ADDR )
> +            break;
> +       }

Indentation.

> +    if ( offset == DT_BAD_ADDR )
> +    {
> +        dt_dprintk("DT: not found !\n");
> +        return 1;
> +    }
> +    memcpy(addr, ranges + na, 4 * pna);
> +
> +finish:
> +    dt_dump_addr("DT: parent translation for:", addr, pna);
> +    dt_dprintk("DT: with offset: %llx\n", (unsigned long long)offset);
> +
> +    /* Translate it into parent bus space */
> +    return pbus->translate(addr, offset, pna);
> +}
> +
> +/*
> + * Translate an address from the device-tree into a CPU physical address,
> + * this walks up the tree and applies the various bus mappings on the
> + * way.
> + *
> + * Note: We consider that crossing any level with #size-cells == 0 to mean
> + * that translation is impossible (that is we are not dealing with a value
> + * that can be mapped to a cpu physical address). This is not really 
> specified
> + * that way, but this is traditionally the way IBM at least do things
> + */
> +static u64 __dt_translate_address(const struct dt_device_node *dev,
> +                                  const __be32 *in_addr, const char *rprop)
> +{
> +    const struct dt_device_node *parent = NULL;
> +    const struct dt_bus *bus, *pbus;
> +    __be32 addr[DT_MAX_ADDR_CELLS];
> +    int na, ns, pna, pns;
> +    u64 result = DT_BAD_ADDR;
> +
> +       dt_dprintk("DT: ** translation for device %s **\n", dev->full_name);

Ide...

> +
> +    /* Get parent & match bus type */
> +    parent = dt_get_parent(dev);
> +    if ( parent == NULL )
> +        goto bail;
> +    bus = dt_match_bus(parent);
> +
> +    /* Count address cells & copy address locally */
> +    bus->count_cells(dev, &na, &ns);
> +    if ( !DT_CHECK_COUNTS(na, ns) )
> +    {
> +        dt_printk(XENLOG_ERR "dt_parse: Bad cell count for %s\n",
> +                  dev->full_name);
> +        goto bail;
> +    }
> +    memcpy(addr, in_addr, na * 4);
> +
> +    dt_dprintk("DT: bus is %s (na=%d, ns=%d) on %s\n",
> +               bus->name, na, ns, parent->full_name);
> +       dt_dump_addr("DT: translating address:", addr, na);

Inde...

> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 879b75d..ce34ba5 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -278,6 +278,28 @@ struct dt_device_node *dt_find_node_by_path(const char 
> *path);
>  const struct dt_device_node *dt_get_parent(const struct dt_device_node 
> *node);
> 
>  /**
> + * dt_device_get_address - Resolve an address for a device
> + * @device: the device whose address is to be resolved
> + * @index: index of the addres to resolve

address

> + * @addr: address filled by this function
> + * @size: size filled by this function
> + *
> + * This function resolves an address, walking the tree, for a give
> + * device-tree node. It returns 0 on success.
> + */
> +int dt_device_get_address(const struct dt_device_node *dev, int index,
> +                          u64 *addr, u64 *size);
> +
> +/**
> + * dt_number_of_address - Get the number of addresse for a device

addresses

> + * @device: the device whose number of address is to be retrieved
> + *
> + * Return the number of address for this device or 0 if there is no
> + * address or an error occured/

occurred

Apart from the indentation and the spelling errors I'm inclined to trust
that the actual logic is ok. So if you fix the above (and any
indentation I missed):
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> + */
> +unsigned int dt_number_of_address(const struct dt_device_node *device);
> +
> +/**
>   * dt_n_size_cells - Helper to retrieve the number of cell for the size
>   * @np: node to get the value
>   *
> --
> Julien Grall
> 



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