From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760545Ab0J0KTk (ORCPT ); Wed, 27 Oct 2010 06:19:40 -0400 Received: from mail-wy0-f174.google.com ([74.125.82.174]:51509 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753180Ab0J0KTi (ORCPT ); Wed, 27 Oct 2010 06:19:38 -0400 Date: Wed, 27 Oct 2010 11:19:33 +0100 From: Grant Likely To: Andres Salomon Cc: devicetree-discuss@lists.ozlabs.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , linux-kernel@vger.kernel.org Subject: Re: [PATCH] x86: OLPC: add OLPC device-tree support (v3) Message-ID: <20101027101933.GD6797@angua.secretlab.ca> References: <20101022155846.66cde32f@queued.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20101022155846.66cde32f@queued.net> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 22, 2010 at 03:58:46PM -0700, Andres Salomon wrote: > > Make use of PROC_DEVICETREE to export the tree, and sparc's PROMTREE code to > call into OLPC's Open Firmware to build the tree. > > v3: rename olpc_prom to olpc_dt > - rework Kconfig entries > - drop devtree build hook from proc, instead adding a call to x86's > paging_init (similarly to how sparc64 does it) > - switch allocation from using slab to alloc_bootmem. this allows > the DT to be built earlier during boot (during setup_arch); the > downside is that there are some 1200 bootmem reservations that are > done during boot. Not ideal.. > - add a helper olpc_ofw_is_installed function to test for the > existence and successful detection of OLPC's OFW. > > Signed-off-by: Andres Salomon Overall this patch looks fine, but it needs to be acked from the x86 maintainers, and I'd like it to have a cycle through linux-next before it gets merged, so that means 2.6.38 because the 2.6.37 merge window has already been open for almost a week. The promtree patches have been merged though. Comments below. g. > --- > arch/x86/Kconfig | 6 ++ > arch/x86/include/asm/olpc_ofw.h | 9 ++ > arch/x86/include/asm/prom.h | 1 + > arch/x86/kernel/Makefile | 1 + > arch/x86/kernel/olpc_dt.c | 165 +++++++++++++++++++++++++++++++++++++++ > arch/x86/kernel/olpc_ofw.c | 5 + > arch/x86/mm/init_32.c | 2 + > 7 files changed, 189 insertions(+), 0 deletions(-) > create mode 100644 arch/x86/include/asm/prom.h > create mode 100644 arch/x86/kernel/olpc_dt.c > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index cea0cd9..1d57e2b 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -2069,11 +2069,17 @@ config OLPC_OPENFIRMWARE > bool "Support for OLPC's Open Firmware" > depends on !X86_64 && !X86_PAE > default y if OLPC > + select OF > help > This option adds support for the implementation of Open Firmware > that is used on the OLPC XO-1 Children's Machine. > If unsure, say N here. > > +config OLPC_OPENFIRMWARE_DT > + bool > + default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE > + select OF_PROMTREE > + > endif # X86_32 > > config K8_NB > diff --git a/arch/x86/include/asm/olpc_ofw.h b/arch/x86/include/asm/olpc_ofw.h > index 08fde47..a41250e 100644 > --- a/arch/x86/include/asm/olpc_ofw.h > +++ b/arch/x86/include/asm/olpc_ofw.h > @@ -8,6 +8,8 @@ > > #ifdef CONFIG_OLPC_OPENFIRMWARE > > +extern bool olpc_ofw_is_installed(void); > + > /* run an OFW command by calling into the firmware */ > #define olpc_ofw(name, args, res) \ > __olpc_ofw((name), ARRAY_SIZE(args), args, ARRAY_SIZE(res), res) > @@ -23,9 +25,16 @@ extern void setup_olpc_ofw_pgd(void); > > #else /* !CONFIG_OLPC_OPENFIRMWARE */ > > +static inline bool olpc_ofw_is_installed(void) { return false; } > static inline void olpc_ofw_detect(void) { } > static inline void setup_olpc_ofw_pgd(void) { } > > #endif /* !CONFIG_OLPC_OPENFIRMWARE */ > > +#ifdef CONFIG_OLPC_OPENFIRMWARE_DT > +extern void olpc_dt_build_devicetree(void); > +#else > +static inline void olpc_dt_build_devicetree(void) { } > +#endif /* CONFIG_OLPC_OPENFIRMWARE_DT */ > + > #endif /* _ASM_X86_OLPC_OFW_H */ > diff --git a/arch/x86/include/asm/prom.h b/arch/x86/include/asm/prom.h > new file mode 100644 > index 0000000..b4ec95f > --- /dev/null > +++ b/arch/x86/include/asm/prom.h > @@ -0,0 +1 @@ > +/* dummy prom.h; here to make linux/of.h's #includes happy */ > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > index fedf32a..519539a 100644 > --- a/arch/x86/kernel/Makefile > +++ b/arch/x86/kernel/Makefile > @@ -107,6 +107,7 @@ scx200-y += scx200_32.o > > obj-$(CONFIG_OLPC) += olpc.o > obj-$(CONFIG_OLPC_OPENFIRMWARE) += olpc_ofw.o > +obj-$(CONFIG_OLPC_OPENFIRMWARE_DT) += olpc_dt.o olpc_ofw_dt.o perhaps? > obj-$(CONFIG_X86_MRST) += mrst.o > > microcode-y := microcode_core.o > diff --git a/arch/x86/kernel/olpc_dt.c b/arch/x86/kernel/olpc_dt.c > new file mode 100644 > index 0000000..f660a11 > --- /dev/null > +++ b/arch/x86/kernel/olpc_dt.c > @@ -0,0 +1,165 @@ > +/* > + * olpc_dt.c: OLPC-specific OFW device tree support code. Nit: I personally prefer not to see the file name encoded in the header block. > + * > + * Paul Mackerras August 1996. > + * Copyright (C) 1996-2005 Paul Mackerras. > + * > + * Adapted for 64bit PowerPC by Dave Engebretsen and Peter Bergner. > + * {engebret|bergner}@us.ibm.com > + * > + * Adapted for sparc by David S. Miller davem@davemloft.net > + * Adapted for x86/OLPC by Andres Salomon > + * > + * 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. > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +static phandle __init olpc_dt_getsibling(phandle node) > +{ > + const void *args[] = { (void *)node }; > + void *res[] = { &node }; > + > + if (node == -1) > + return 0; phandle is a u32, so testing against a negative value is not sane. > + > + if (olpc_ofw("peer", args, res) || node == -1) > + return 0; > + > + return node; > +} > + > +static phandle __init olpc_dt_getchild(phandle node) > +{ > + const void *args[] = { (void *)node }; > + void *res[] = { &node }; > + > + if (node == -1) > + return 0; > + > + if (olpc_ofw("child", args, res) || node == -1) { > + pr_err("PROM: %s: fetching child failed!\n", __func__); > + return 0; > + } > + > + return node; > +} > + > +static int __init olpc_dt_getproplen(phandle node, const char *prop) > +{ > + const void *args[] = { (void *)node, prop }; > + int len; > + void *res[] = { &len }; > + > + if (node == -1) > + return -1; > + > + if (olpc_ofw("getproplen", args, res)) { > + pr_err("PROM: %s: getproplen failed!\n", __func__); > + return -1; > + } > + > + return len; > +} > + > +static int __init olpc_dt_getproperty(phandle node, const char *prop, > + char *buf, int bufsize) > +{ > + int plen; > + > + plen = olpc_dt_getproplen(node, prop); > + if (plen > bufsize || plen < 1) > + return -1; > + else { Nit: If there are braces on the else clause, then please use them on the if() clause also. > + const void *args[] = { (void *)node, prop, buf, (void *)plen }; > + void *res[] = { &plen }; > + > + if (olpc_ofw("getprop", args, res)) { > + pr_err("PROM: %s: getprop failed!\n", __func__); > + return -1; > + } > + } > + > + return plen; > +} > + > +static int __init olpc_dt_nextprop(phandle node, char *prev, char *buf) > +{ > + const void *args[] = { (void *)node, prev, buf }; This looks wrong. It does not look sane to cast node as a pointer (which won't be the same size on x86_64), and then recast it to an int inside __olpc_ofw(). Do you know why the api is implemented in this way? > + int success; > + void *res[] = { &success }; > + > + buf[0] = '\0'; > + > + if (node == -1) > + return -1; > + > + if (olpc_ofw("nextprop", args, res) || success != 1) > + return -1; > + > + return 0; > +} > + > +static int __init olpc_dt_pkg2path(phandle node, char *buf, > + const int buflen, int *len) > +{ > + const void *args[] = { (void *)node, buf, (void *)buflen }; > + void *res[] = { len }; > + > + if (node == -1) > + return -1; > + > + if (olpc_ofw("package-to-path", args, res) || *len < 1) > + return -1; > + > + return 0; > +} > + > +static unsigned int prom_early_allocated __initdata; > + > +void * __init prom_early_alloc(unsigned long size) > +{ > + void *res; > + > + res = alloc_bootmem(size); > + if (res) > + memset(res, 0, size); > + > + prom_early_allocated += size; > + > + return res; > +} > + > +static struct of_pdt_ops prom_olpc_ops __initdata = { > + .nextprop = olpc_dt_nextprop, > + .getproplen = olpc_dt_getproplen, > + .getproperty = olpc_dt_getproperty, > + .getchild = olpc_dt_getchild, > + .getsibling = olpc_dt_getsibling, > + .pkg2path = olpc_dt_pkg2path, > +}; > + > +void __init olpc_dt_build_devicetree(void) > +{ > + phandle root; > + > + if (!olpc_ofw_is_installed()) > + return; > + > + root = olpc_dt_getsibling(0); > + if (root < 0) { > + pr_err("PROM: unable to get root node from OFW!\n"); > + return; > + } > + of_pdt_build_devicetree(root, &prom_olpc_ops); > + > + pr_info("PROM DT: Built device tree with %u bytes of memory.\n", > + prom_early_allocated); > +} > diff --git a/arch/x86/kernel/olpc_ofw.c b/arch/x86/kernel/olpc_ofw.c > index 3218aa7..2176048 100644 > --- a/arch/x86/kernel/olpc_ofw.c > +++ b/arch/x86/kernel/olpc_ofw.c > @@ -104,3 +104,8 @@ void __init olpc_ofw_detect(void) > (unsigned long)olpc_ofw_cif, (-start) >> 20); > reserve_top_address(-start); > } > + > +bool __init olpc_ofw_is_installed(void) > +{ > + return olpc_ofw_cif != NULL; > +} > diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c > index bca7909..41bf85f 100644 > --- a/arch/x86/mm/init_32.c > +++ b/arch/x86/mm/init_32.c > @@ -44,6 +44,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -833,6 +834,7 @@ void __init paging_init(void) > /* > * NOTE: at this point the bootmem allocator is fully available. > */ > + olpc_dt_build_devicetree(); > sparse_init(); > zone_sizes_init(); > } > -- > 1.5.6.5 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grant Likely Subject: Re: [PATCH] x86: OLPC: add OLPC device-tree support (v3) Date: Wed, 27 Oct 2010 11:19:33 +0100 Message-ID: <20101027101933.GD6797@angua.secretlab.ca> References: <20101022155846.66cde32f@queued.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20101022155846.66cde32f-pFFUokh25LWsTnJN9+BGXg@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org To: Andres Salomon Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" List-Id: devicetree@vger.kernel.org On Fri, Oct 22, 2010 at 03:58:46PM -0700, Andres Salomon wrote: > > Make use of PROC_DEVICETREE to export the tree, and sparc's PROMTREE code to > call into OLPC's Open Firmware to build the tree. > > v3: rename olpc_prom to olpc_dt > - rework Kconfig entries > - drop devtree build hook from proc, instead adding a call to x86's > paging_init (similarly to how sparc64 does it) > - switch allocation from using slab to alloc_bootmem. this allows > the DT to be built earlier during boot (during setup_arch); the > downside is that there are some 1200 bootmem reservations that are > done during boot. Not ideal.. > - add a helper olpc_ofw_is_installed function to test for the > existence and successful detection of OLPC's OFW. > > Signed-off-by: Andres Salomon Overall this patch looks fine, but it needs to be acked from the x86 maintainers, and I'd like it to have a cycle through linux-next before it gets merged, so that means 2.6.38 because the 2.6.37 merge window has already been open for almost a week. The promtree patches have been merged though. Comments below. g. > --- > arch/x86/Kconfig | 6 ++ > arch/x86/include/asm/olpc_ofw.h | 9 ++ > arch/x86/include/asm/prom.h | 1 + > arch/x86/kernel/Makefile | 1 + > arch/x86/kernel/olpc_dt.c | 165 +++++++++++++++++++++++++++++++++++++++ > arch/x86/kernel/olpc_ofw.c | 5 + > arch/x86/mm/init_32.c | 2 + > 7 files changed, 189 insertions(+), 0 deletions(-) > create mode 100644 arch/x86/include/asm/prom.h > create mode 100644 arch/x86/kernel/olpc_dt.c > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index cea0cd9..1d57e2b 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -2069,11 +2069,17 @@ config OLPC_OPENFIRMWARE > bool "Support for OLPC's Open Firmware" > depends on !X86_64 && !X86_PAE > default y if OLPC > + select OF > help > This option adds support for the implementation of Open Firmware > that is used on the OLPC XO-1 Children's Machine. > If unsure, say N here. > > +config OLPC_OPENFIRMWARE_DT > + bool > + default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE > + select OF_PROMTREE > + > endif # X86_32 > > config K8_NB > diff --git a/arch/x86/include/asm/olpc_ofw.h b/arch/x86/include/asm/olpc_ofw.h > index 08fde47..a41250e 100644 > --- a/arch/x86/include/asm/olpc_ofw.h > +++ b/arch/x86/include/asm/olpc_ofw.h > @@ -8,6 +8,8 @@ > > #ifdef CONFIG_OLPC_OPENFIRMWARE > > +extern bool olpc_ofw_is_installed(void); > + > /* run an OFW command by calling into the firmware */ > #define olpc_ofw(name, args, res) \ > __olpc_ofw((name), ARRAY_SIZE(args), args, ARRAY_SIZE(res), res) > @@ -23,9 +25,16 @@ extern void setup_olpc_ofw_pgd(void); > > #else /* !CONFIG_OLPC_OPENFIRMWARE */ > > +static inline bool olpc_ofw_is_installed(void) { return false; } > static inline void olpc_ofw_detect(void) { } > static inline void setup_olpc_ofw_pgd(void) { } > > #endif /* !CONFIG_OLPC_OPENFIRMWARE */ > > +#ifdef CONFIG_OLPC_OPENFIRMWARE_DT > +extern void olpc_dt_build_devicetree(void); > +#else > +static inline void olpc_dt_build_devicetree(void) { } > +#endif /* CONFIG_OLPC_OPENFIRMWARE_DT */ > + > #endif /* _ASM_X86_OLPC_OFW_H */ > diff --git a/arch/x86/include/asm/prom.h b/arch/x86/include/asm/prom.h > new file mode 100644 > index 0000000..b4ec95f > --- /dev/null > +++ b/arch/x86/include/asm/prom.h > @@ -0,0 +1 @@ > +/* dummy prom.h; here to make linux/of.h's #includes happy */ > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > index fedf32a..519539a 100644 > --- a/arch/x86/kernel/Makefile > +++ b/arch/x86/kernel/Makefile > @@ -107,6 +107,7 @@ scx200-y += scx200_32.o > > obj-$(CONFIG_OLPC) += olpc.o > obj-$(CONFIG_OLPC_OPENFIRMWARE) += olpc_ofw.o > +obj-$(CONFIG_OLPC_OPENFIRMWARE_DT) += olpc_dt.o olpc_ofw_dt.o perhaps? > obj-$(CONFIG_X86_MRST) += mrst.o > > microcode-y := microcode_core.o > diff --git a/arch/x86/kernel/olpc_dt.c b/arch/x86/kernel/olpc_dt.c > new file mode 100644 > index 0000000..f660a11 > --- /dev/null > +++ b/arch/x86/kernel/olpc_dt.c > @@ -0,0 +1,165 @@ > +/* > + * olpc_dt.c: OLPC-specific OFW device tree support code. Nit: I personally prefer not to see the file name encoded in the header block. > + * > + * Paul Mackerras August 1996. > + * Copyright (C) 1996-2005 Paul Mackerras. > + * > + * Adapted for 64bit PowerPC by Dave Engebretsen and Peter Bergner. > + * {engebret|bergner}@us.ibm.com > + * > + * Adapted for sparc by David S. Miller davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org > + * Adapted for x86/OLPC by Andres Salomon > + * > + * 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. > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +static phandle __init olpc_dt_getsibling(phandle node) > +{ > + const void *args[] = { (void *)node }; > + void *res[] = { &node }; > + > + if (node == -1) > + return 0; phandle is a u32, so testing against a negative value is not sane. > + > + if (olpc_ofw("peer", args, res) || node == -1) > + return 0; > + > + return node; > +} > + > +static phandle __init olpc_dt_getchild(phandle node) > +{ > + const void *args[] = { (void *)node }; > + void *res[] = { &node }; > + > + if (node == -1) > + return 0; > + > + if (olpc_ofw("child", args, res) || node == -1) { > + pr_err("PROM: %s: fetching child failed!\n", __func__); > + return 0; > + } > + > + return node; > +} > + > +static int __init olpc_dt_getproplen(phandle node, const char *prop) > +{ > + const void *args[] = { (void *)node, prop }; > + int len; > + void *res[] = { &len }; > + > + if (node == -1) > + return -1; > + > + if (olpc_ofw("getproplen", args, res)) { > + pr_err("PROM: %s: getproplen failed!\n", __func__); > + return -1; > + } > + > + return len; > +} > + > +static int __init olpc_dt_getproperty(phandle node, const char *prop, > + char *buf, int bufsize) > +{ > + int plen; > + > + plen = olpc_dt_getproplen(node, prop); > + if (plen > bufsize || plen < 1) > + return -1; > + else { Nit: If there are braces on the else clause, then please use them on the if() clause also. > + const void *args[] = { (void *)node, prop, buf, (void *)plen }; > + void *res[] = { &plen }; > + > + if (olpc_ofw("getprop", args, res)) { > + pr_err("PROM: %s: getprop failed!\n", __func__); > + return -1; > + } > + } > + > + return plen; > +} > + > +static int __init olpc_dt_nextprop(phandle node, char *prev, char *buf) > +{ > + const void *args[] = { (void *)node, prev, buf }; This looks wrong. It does not look sane to cast node as a pointer (which won't be the same size on x86_64), and then recast it to an int inside __olpc_ofw(). Do you know why the api is implemented in this way? > + int success; > + void *res[] = { &success }; > + > + buf[0] = '\0'; > + > + if (node == -1) > + return -1; > + > + if (olpc_ofw("nextprop", args, res) || success != 1) > + return -1; > + > + return 0; > +} > + > +static int __init olpc_dt_pkg2path(phandle node, char *buf, > + const int buflen, int *len) > +{ > + const void *args[] = { (void *)node, buf, (void *)buflen }; > + void *res[] = { len }; > + > + if (node == -1) > + return -1; > + > + if (olpc_ofw("package-to-path", args, res) || *len < 1) > + return -1; > + > + return 0; > +} > + > +static unsigned int prom_early_allocated __initdata; > + > +void * __init prom_early_alloc(unsigned long size) > +{ > + void *res; > + > + res = alloc_bootmem(size); > + if (res) > + memset(res, 0, size); > + > + prom_early_allocated += size; > + > + return res; > +} > + > +static struct of_pdt_ops prom_olpc_ops __initdata = { > + .nextprop = olpc_dt_nextprop, > + .getproplen = olpc_dt_getproplen, > + .getproperty = olpc_dt_getproperty, > + .getchild = olpc_dt_getchild, > + .getsibling = olpc_dt_getsibling, > + .pkg2path = olpc_dt_pkg2path, > +}; > + > +void __init olpc_dt_build_devicetree(void) > +{ > + phandle root; > + > + if (!olpc_ofw_is_installed()) > + return; > + > + root = olpc_dt_getsibling(0); > + if (root < 0) { > + pr_err("PROM: unable to get root node from OFW!\n"); > + return; > + } > + of_pdt_build_devicetree(root, &prom_olpc_ops); > + > + pr_info("PROM DT: Built device tree with %u bytes of memory.\n", > + prom_early_allocated); > +} > diff --git a/arch/x86/kernel/olpc_ofw.c b/arch/x86/kernel/olpc_ofw.c > index 3218aa7..2176048 100644 > --- a/arch/x86/kernel/olpc_ofw.c > +++ b/arch/x86/kernel/olpc_ofw.c > @@ -104,3 +104,8 @@ void __init olpc_ofw_detect(void) > (unsigned long)olpc_ofw_cif, (-start) >> 20); > reserve_top_address(-start); > } > + > +bool __init olpc_ofw_is_installed(void) > +{ > + return olpc_ofw_cif != NULL; > +} > diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c > index bca7909..41bf85f 100644 > --- a/arch/x86/mm/init_32.c > +++ b/arch/x86/mm/init_32.c > @@ -44,6 +44,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -833,6 +834,7 @@ void __init paging_init(void) > /* > * NOTE: at this point the bootmem allocator is fully available. > */ > + olpc_dt_build_devicetree(); > sparse_init(); > zone_sizes_init(); > } > -- > 1.5.6.5 >