Message-ID: <AANLkTimG7u5eXWSn5VznKfuFcFD+evTzo1_QpAFmCdpL@mail.gmail.com>
Date:	Sun, 8 Aug 2010 23:12:21 -0600
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Andres Salomon <dilinger@...ued.net>
Cc:	devicetree-discuss@...ts.ozlabs.org, sparclinux@...r.kernel.org,
	davem@...emloft.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] sparc: make driver/of/pdt no longer sparc-specific

Hi Andres, thanks for the patch.  Comments below.

g.

On Sun, Aug 8, 2010 at 9:11 PM, Andres Salomon <dilinger@...ued.net> wrote:
>
> Clean up pdt.c:
>  - make build dependent upon config OF_PROMTREE
>  - #ifdef out the sparc-specific stuff
>  - create pdt-specific header
>  - create a pdt_ops struct that pdt uses to call arch-specific prom routines
>
> Signed-off-by: Andres Salomon <dilinger@...ued.net>
> ---
>  arch/sparc/Kconfig              |    1 +
>  arch/sparc/include/asm/prom.h   |    5 +-
>  arch/sparc/kernel/prom.h        |    6 --
>  arch/sparc/kernel/prom_common.c |   57 ++++++++++++++++++++++-
>  drivers/of/Kconfig              |    4 ++
>  drivers/of/Makefile             |    1 +
>  drivers/of/pdt.c                |   98 +++++++++++++++++++++++++-------------
>  include/linux/of_pdt.h          |   42 +++++++++++++++++
>  8 files changed, 171 insertions(+), 43 deletions(-)
>  create mode 100644 include/linux/of_pdt.h
>
> diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
> index 13a9f2f..ed3f009 100644
> --- a/arch/sparc/Kconfig
> +++ b/arch/sparc/Kconfig
> @@ -24,6 +24,7 @@ config SPARC
>        select HAVE_ARCH_KGDB if !SMP || SPARC64
>        select HAVE_ARCH_TRACEHOOK
>        select ARCH_WANT_OPTIONAL_GPIOLIB
> +       select OF_PROMTREE

Group this with the select OF from earlier in the config SPARC option.

>        select RTC_CLASS
>        select RTC_DRV_M48T59
>        select HAVE_PERF_EVENTS
> diff --git a/arch/sparc/include/asm/prom.h b/arch/sparc/include/asm/prom.h
> index f845828..329a976 100644
> --- a/arch/sparc/include/asm/prom.h
> +++ b/arch/sparc/include/asm/prom.h
> @@ -18,6 +18,7 @@
>  * 2 of the License, or (at your option) any later version.
>  */
>  #include <linux/types.h>
> +#include <linux/of_pdt.h>
>  #include <linux/proc_fs.h>
>  #include <linux/mutex.h>
>  #include <asm/atomic.h>
> @@ -65,8 +66,8 @@ extern struct device_node *of_console_device;
>  extern char *of_console_path;
>  extern char *of_console_options;
>
> -extern void (*prom_build_more)(struct device_node *dp, struct device_node ***nextp);
> -extern char *build_full_name(struct device_node *dp);
> +extern void irq_trans_init(struct device_node *dp);
> +extern char *build_path_component(struct device_node *dp);
>
>  #endif /* __KERNEL__ */
>  #endif /* _SPARC_PROM_H */
> diff --git a/arch/sparc/kernel/prom.h b/arch/sparc/kernel/prom.h
> index eeb04a7..cf5fe1c 100644
> --- a/arch/sparc/kernel/prom.h
> +++ b/arch/sparc/kernel/prom.h
> @@ -4,12 +4,6 @@
>  #include <linux/spinlock.h>
>  #include <asm/prom.h>
>
> -extern void * prom_early_alloc(unsigned long size);
> -extern void irq_trans_init(struct device_node *dp);
> -
> -extern unsigned int prom_unique_id;
> -
> -extern char *build_path_component(struct device_node *dp);
>  extern void of_console_init(void);
>
>  extern unsigned int prom_early_allocated;
> diff --git a/arch/sparc/kernel/prom_common.c b/arch/sparc/kernel/prom_common.c
> index 7b454f6..4c5f67f 100644
> --- a/arch/sparc/kernel/prom_common.c
> +++ b/arch/sparc/kernel/prom_common.c
> @@ -20,6 +20,7 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/of.h>
> +#include <linux/of_pdt.h>
>  #include <asm/prom.h>
>  #include <asm/oplib.h>
>  #include <asm/leon.h>
> @@ -117,6 +118,60 @@ int of_find_in_proplist(const char *list, const char *match, int len)
>  }
>  EXPORT_SYMBOL(of_find_in_proplist);
>
> +/*
> + * SPARC32 and SPARC64's prom_firstprop/prom_nextprop do things differently
> + * here, despite sharing the same interface.  SPARC32 doesn't fill in 'buf',
> + * returning NULL on an error.  SPARC64 fills in 'buf', but sets it to an
> + * empty string upon error.
> + */
> +static int __init handle_prop_quirks(char *buf, const char *name)
> +{
> +       if (!name || strlen(name) == 0)
> +               return -1;
> +
> +#ifdef CONFIG_SPARC32
> +       strcpy(buf, name);
> +#endif
> +       return 0;
> +}
> +
> +static int __init prom_common_firstprop(phandle node, char *buf)
> +{
> +       const char *name;
> +
> +       buf[0] = '\0';
> +       name = prom_firstprop(node, buf);
> +       return handle_prop_quirks(buf, name);
> +}
> +
> +static int __init prom_common_nextprop(phandle node, const char *prev,
> +               char *buf)
> +{
> +       const char *name;
> +
> +       buf[0] = '\0';
> +       name = prom_nextprop(node, prev, buf);
> +       return handle_prop_quirks(buf, name);
> +}

Rather than having both prom_common_{firstprop,nextprop}(), there only
needs to be one hook; prom_common_nextprop().  Make it use the
firstprop behaviour when it is passed a NULL in the prev pointer.
This will simplify the users of this code further down.

> +
>  unsigned int prom_early_allocated __initdata;
>
> -#include "../../../drivers/of/pdt.c"
> +static struct of_pdt_ops prom_sparc_ops __initdata = {
> +       .firstprop = prom_common_firstprop,
> +       .nextprop = prom_common_nextprop,
> +       .getproplen = (int (*)(phandle, const char *))prom_getproplen,
> +       .getproperty = (int (*)(phandle, const char *, char *, int))prom_getproperty,
> +       .getchild = (phandle (*)(phandle))prom_getchild,
> +       .getsibling = (phandle (*)(phandle))prom_getsibling,

If you have to explicitly cast these function pointers, then you're
doing it wrong.  :-)  Listen to and fix the compiler complaint here.

> +};
> +
> +void __init prom_build_devicetree(void)
> +{
> +       of_pdt_set_ops(&prom_sparc_ops);
> +       of_pdt_build_devicetree(prom_root_node);

Maybe I'm being nitpicky here, but I would pass the ops structure into
of_pdt_build_devicetree() directly.  I don't like the implied state of
setting the ops pointer separate from parsing the tree.

> +
> +       of_console_init();
> +
> +       printk(KERN_INFO "PROM: Built device tree with %u bytes of memory.\n",
> +              prom_early_allocated);

pr_info()

> +}
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 1678dbc..c8a4b7c 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -5,6 +5,10 @@ config OF_FLATTREE
>        bool
>        depends on OF
>
> +config OF_PROMTREE
> +       bool
> +       depends on OF
> +

I can tell from the context here you're working from an older tree.
Please rebase onto Linus' current top-of-tree.  :-)  A bunch of OF
related patches have been merged for 2.6.36 that will conflict with
this patch.

>  config OF_DYNAMIC
>        def_bool y
>        depends on OF && PPC_OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index f232cc9..54e8517 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,5 +1,6 @@
>  obj-y = base.o
>  obj-$(CONFIG_OF_FLATTREE) += fdt.o
> +obj-$(CONFIG_OF_PROMTREE) += pdt.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_GPIO)   += gpio.o
>  obj-$(CONFIG_OF_I2C)   += of_i2c.o
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 61d9477..22f46fb 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -1,5 +1,4 @@
> -/* prom_common.c: OF device tree support common code.
> - *
> +/*

You should still retain a one-line description of what this file is for.

>  * Paul Mackerras      August 1996.
>  * Copyright (C) 1996-2005 Paul Mackerras.
>  *
> @@ -7,6 +6,7 @@
>  *    {engebret|bergner}@...ibm.com
>  *
>  *  Adapted for sparc by David S. Miller davem@...emloft.net
> + *  Adapted for multiple architectures by Andres Salomon <dilinger@...ued.net>
>  *
>  *      This program is free software; you can redistribute it and/or
>  *      modify it under the terms of the GNU General Public License
> @@ -20,13 +20,44 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/of.h>
> +#include <linux/of_pdt.h>
>  #include <asm/prom.h>
> -#include <asm/oplib.h>
> -#include <asm/leon.h>
>
> -void (*prom_build_more)(struct device_node *dp, struct device_node ***nextp);
> +void __initdata (*prom_build_more)(struct device_node *dp,
> +               struct device_node ***nextp);
> +
> +static struct of_pdt_ops prom_ops __initdata;

Why a full copy of the structure instead of a pointer?

> +
> +#if defined(CONFIG_SPARC)
> +static unsigned int prom_unique_id __initdata;
> +
> +#define inc_unique_id(p) do { \
> +       (p)->unique_id = prom_unique_id++; \
> +} while (0)

Use a static inline.  C code is preferred over preprocessor code.
Also preserver the namespace and use the of_pdt_ prefix (that goes for
all the new functions here in this file).

> +
> +static inline const char *fetch_node_name(struct device_node *dp)
> +{
> +       return dp->path_component_name;
> +}
> +
> +#else
> +
> +static inline void inc_unique_id(void *p)
> +{
> +       /* unused on non-SPARC architectures */
> +}
> +
> +static inline const char *fetch_node_name(struct device_node *dp)
> +{
> +       return dp->name;
> +}

It would be nice to rationalize the differences between how sparc and
powerpc use the ->name/->path_component_name fields; but I haven't
investigated what the differences are.

> +
> +static inline void irq_trans_init(struct device_node *dp)
> +{
> +       /* unused on non-SPARC architectures */
> +}

For empty statics like this; I'm fine with this more concise form:

+static inline void inc_unique_id(void *p) { }
+static inline void irq_trans_init(struct device_node *dp) { }

(again, add the of_pdt_ prefix)

>
> -unsigned int prom_unique_id;
> +#endif /* !CONFIG_SPARC */
>
>  static struct property * __init build_one_prop(phandle node, char *prev,
>                                               char *special_name,
> @@ -35,7 +66,6 @@ static struct property * __init build_one_prop(phandle node, char *prev,
>  {
>        static struct property *tmp = NULL;
>        struct property *p;
> -       const char *name;
>
>        if (tmp) {
>                p = tmp;
> @@ -43,7 +73,7 @@ static struct property * __init build_one_prop(phandle node, char *prev,
>                tmp = NULL;
>        } else {
>                p = prom_early_alloc(sizeof(struct property) + 32);
> -               p->unique_id = prom_unique_id++;
> +               inc_unique_id(p);
>        }
>
>        p->name = (char *) (p + 1);
> @@ -53,27 +83,24 @@ static struct property * __init build_one_prop(phandle node, char *prev,
>                p->value = prom_early_alloc(special_len);
>                memcpy(p->value, special_val, special_len);
>        } else {
> -               if (prev == NULL) {
> -                       name = prom_firstprop(node, p->name);
> -               } else {
> -                       name = prom_nextprop(node, prev, p->name);
> -               }
> +               int err;
>
> -               if (!name || strlen(name) == 0) {
> +               if (prev == NULL)
> +                       err = prom_ops.firstprop(node, p->name);
> +               else
> +                       err = prom_ops.nextprop(node, prev, p->name);

As mentioned earlier, this is better with a single .nextprop() hook
that behaves differently when a NULL prev pointer is passed.

> +               if (err) {
>                        tmp = p;
>                        return NULL;
>                }
> -#ifdef CONFIG_SPARC32
> -               strcpy(p->name, name);
> -#endif
> -               p->length = prom_getproplen(node, p->name);
> +               p->length = prom_ops.getproplen(node, p->name);
>                if (p->length <= 0) {
>                        p->length = 0;
>                } else {
>                        int len;
>
>                        p->value = prom_early_alloc(p->length + 1);
> -                       len = prom_getproperty(node, p->name, p->value,
> +                       len = prom_ops.getproperty(node, p->name, p->value,
>                                               p->length);
>                        if (len <= 0)
>                                p->length = 0;
> @@ -106,10 +133,10 @@ static char * __init get_one_property(phandle node, const char *name)
>        char *buf = "<NULL>";
>        int len;
>
> -       len = prom_getproplen(node, name);
> +       len = prom_ops.getproplen(node, name);
>        if (len > 0) {
>                buf = prom_early_alloc(len);
> -               len = prom_getproperty(node, name, buf, len);
> +               len = prom_ops.getproperty(node, name, buf, len);
>        }
>
>        return buf;
> @@ -124,7 +151,7 @@ static struct device_node * __init prom_create_node(phandle node,
>                return NULL;
>
>        dp = prom_early_alloc(sizeof(*dp));
> -       dp->unique_id = prom_unique_id++;
> +       inc_unique_id(dp);
>        dp->parent = parent;
>
>        kref_init(&dp->kref);
> @@ -140,13 +167,13 @@ static struct device_node * __init prom_create_node(phandle node,
>        return dp;
>  }
>
> -char * __init build_full_name(struct device_node *dp)
> +static char * __init build_full_name(struct device_node *dp)
>  {
>        int len, ourlen, plen;
>        char *n;
>
>        plen = strlen(dp->parent->full_name);
> -       ourlen = strlen(dp->path_component_name);
> +       ourlen = strlen(fetch_node_name(dp));
>        len = ourlen + plen + 2;
>
>        n = prom_early_alloc(len);
> @@ -155,7 +182,7 @@ char * __init build_full_name(struct device_node *dp)
>                strcpy(n + plen, "/");
>                plen++;
>        }
> -       strcpy(n + plen, dp->path_component_name);
> +       strcpy(n + plen, fetch_node_name(dp));
>
>        return n;
>  }
> @@ -182,36 +209,39 @@ static struct device_node * __init prom_build_tree(struct device_node *parent,
>                *(*nextp) = dp;
>                *nextp = &dp->allnext;
>
> +#if defined(CONFIG_SPARC)
>                dp->path_component_name = build_path_component(dp);
> +#endif
>                dp->full_name = build_full_name(dp);
>
> -               dp->child = prom_build_tree(dp, prom_getchild(node), nextp);
> +               dp->child = prom_build_tree(dp, prom_ops.getchild(node), nextp);
>
>                if (prom_build_more)
>                        prom_build_more(dp, nextp);
>
> -               node = prom_getsibling(node);
> +               node = prom_ops.getsibling(node);
>        }
>
>        return ret;
>  }
>
> -void __init prom_build_devicetree(void)
> +void __init of_pdt_build_devicetree(int root_node)
>  {
>        struct device_node **nextp;
>
> -       allnodes = prom_create_node(prom_root_node, NULL);
> +       allnodes = prom_create_node(root_node, NULL);
>        allnodes->path_component_name = "";
>        allnodes->full_name = "/";
>
>        nextp = &allnodes->allnext;
>        allnodes->child = prom_build_tree(allnodes,
> -                       prom_getchild(allnodes->phandle),
> +                       prom_ops.getchild(allnodes->phandle),
>                        &nextp);
> +}
>
> -       of_console_init();
> +void __init of_pdt_set_ops(struct of_pdt_ops *ops)
> +{
> +       BUG_ON(!ops);
>
> -       printk("PROM: Built device tree with %u bytes of memory.\n",
> -              prom_early_allocated);
> +       prom_ops = *ops;

As mentioned above, why is the structure copied instead of just
storing the pointer.

>  }
> -
> diff --git a/include/linux/of_pdt.h b/include/linux/of_pdt.h
> new file mode 100644
> index 0000000..1324ba5
> --- /dev/null
> +++ b/include/linux/of_pdt.h
> @@ -0,0 +1,42 @@
> +/*
> + * Definitions for building a device tree by calling into the
> + * Open Firmware PROM.
> + *
> + * Copyright (C) 2010  Andres Salomon <dilinger@...ued.net>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _LINUX_OF_PDT_H
> +#define _LINUX_OF_PDT_H
> +
> +extern void *prom_early_alloc(unsigned long size);
> +
> +/* overridable operations for calling into the PROM */
> +struct of_pdt_ops {
> +       /* buffers passed should be 32 bytes; return 0 on success */
> +       int (*firstprop)(phandle node, char *buf);
> +       int (*nextprop)(phandle node, const char *prev, char *buf);
> +
> +       /* for both functions, return proplen on success; -1 on error */
> +       int (*getproplen)(phandle node, const char *prop);
> +       int (*getproperty)(phandle node, const char *prop, char *buf,
> +                       int bufsize);
> +
> +       /* phandles are 0 if no child or sibling exists */
> +       phandle (*getchild)(phandle parent);
> +       phandle (*getsibling)(phandle node);
> +};
> +
> +extern void of_pdt_set_ops(struct of_pdt_ops *ops);
> +
> +/* for building the device tree */
> +extern void of_pdt_build_devicetree(int root_node);
> +
> +extern void (*prom_build_more)(struct device_node *dp,
> +               struct device_node ***nextp);
> +
> +#endif /* _LINUX_OF_PDT_H */
> --
> 1.5.6.5
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists