On Fri, Sep 21, 2007 at 06:03:24PM -0500, Milton Miller wrote:
> Some platforms have a boot agent that can create or modify properties in
> the device-tree and load images into memory. Provide a helper to set
> loader_info used by prep_initrd().
>
> Signed-off-by: Milton Miller <[EMAIL PROTECTED]>
> Acked-by: David Gibson <[EMAIL PROTECTED]>
Hrm, despite my earlier ack, I'm going to whinge about a few nits
here.
> ---
> re 12168
> rediffed types.h, offset in ops.h
>
> Index: kernel/arch/powerpc/boot/ops.h
> ===================================================================
> --- kernel.orig/arch/powerpc/boot/ops.h 2007-09-17 22:12:47.000000000
> -0500
> +++ kernel/arch/powerpc/boot/ops.h 2007-09-17 22:12:51.000000000 -0500
> @@ -163,6 +163,7 @@ void dt_fixup_clock(const char *path, u3
> void __dt_fixup_mac_addresses(u32 startindex, ...);
> #define dt_fixup_mac_addresses(...) \
> __dt_fixup_mac_addresses(0, __VA_ARGS__, NULL)
> +void dt_find_initrd(void);
>
>
> static inline void *find_node_by_linuxphandle(const u32 linuxphandle)
> Index: kernel/arch/powerpc/boot/types.h
> ===================================================================
> --- kernel.orig/arch/powerpc/boot/types.h 2007-09-17 22:12:47.000000000
> -0500
> +++ kernel/arch/powerpc/boot/types.h 2007-09-17 22:12:51.000000000 -0500
> @@ -12,6 +12,8 @@ typedef short s16;
> typedef int s32;
> typedef long long s64;
>
> +#define UINT_MAX 0xFFFFFFFF
I actually don't like this constant - at the point you compare you
care, explicitly, about the value not being over 32-bits, rather than
whether it fits a uint, so the named constant is more misleading than
helpful.
> +
> #define min(x,y) ({ \
> typeof(x) _x = (x); \
> typeof(y) _y = (y); \
> Index: kernel/arch/powerpc/boot/devtree.c
> ===================================================================
> --- kernel.orig/arch/powerpc/boot/devtree.c 2007-09-17 22:12:47.000000000
> -0500
> +++ kernel/arch/powerpc/boot/devtree.c 2007-09-17 22:12:51.000000000
> -0500
> @@ -1,6 +1,7 @@
> /*
> * devtree.c - convenience functions for device tree manipulation
> * Copyright 2007 David Gibson, IBM Corporation.
> + * Copyright 2007 Milton Miller, IBM Corporation.
> * Copyright (c) 2007 Freescale Semiconductor, Inc.
> *
> * Authors: David Gibson <[EMAIL PROTECTED]>
> @@ -333,3 +334,68 @@ int dt_is_compatible(void *node, const c
>
> return 0;
> }
> +
> +/**
> + * dt_find_initrd - set loader initrd location based on existing properties
> + *
> + * finds the linux,initrd-start and linux,initrd-end properties in
> + * the /chosen node and sets the loader initrd fields accordingly.
> + *
> + * Use this if your loader sets the properties to allow other code to
> + * relocate the tree and/or cause r3 and r4 to be set on true OF
> + * platforms.
I am unable to make sense of the paragraph above.
> + */
> +void dt_find_initrd(void)
> +{
> + int rc;
> + unsigned long long initrd_start, initrd_end;
> + void *devp;
> + static const char start_prop[] = "linux,initrd-start";
> + static const char end_prop[] = "linux,initrd-end";
I think these constants are more obscuring than useful.
> +
> + devp = finddevice("/chosen");
> + if (! devp) {
> + return;
> + }
CodingStyle would not put { } here.
> +
> + rc = getprop(devp, start_prop, &initrd_start, sizeof(initrd_start));
> + if (rc < 0)
> + return; /* not found */
> + /* The properties had to be 8 bytes until 2.6.22 */
> + if (rc == sizeof(unsigned long)) {
> + unsigned long tmp;
> + memcpy(&tmp, &initrd_start, rc);
> + initrd_start = tmp;
> + } else if (rc != sizeof(initrd_start)) { /* now they
> can be 4 */
Right. 8 bytes and 4 bytes, so you should be using explicit length
types instead of long and long long.
> + printf("unexpected length of %s in /chosen!\n\r", start_prop);
> + return;
All these printf() / return stanzas add a lot of verbosity to this
function. Any way they can be consolidated a bit, maybe a single
error path that just prints the property values, so the user can
figure out what was wrong with them.
> + }
> +
> + rc = getprop(devp, end_prop, &initrd_end, sizeof(initrd_end));
> + if (rc < 0) {
> + printf("chosen has %s but no %s!\n\r", start_prop, end_prop);
> + return;
> + }
> + if (rc == sizeof(unsigned long)) {
> + unsigned long tmp;
> + memcpy(&tmp, &initrd_end, rc);
> + initrd_end = tmp;
> + } else if (rc != sizeof(initrd_end)) {
> + printf("unexpected length of %s in /chosen!\n\r", end_prop);
> + return;
> + }
> +
> + /* Check for presence, ignore if (partially) loaded above 32 bits */
> + if (initrd_start == initrd_end) {
> + printf("ignoring empty device-tree supplied initrd\n");
> + } else if (initrd_start > initrd_end) {
> + printf("ignoring device-tree supplied initrd: start 0x%llx"
> + " > end 0x%llx \n", initrd_start, initrd_end);
> + } else if (initrd_end > UINT_MAX) {
> + printf("ignoring device-tree supplied initrd:"
> + " end 0x%llx > 32 bits\n", initrd_end);
> + } else {
> + loader_info.initrd_addr = initrd_start;
> + loader_info.initrd_size = initrd_end - initrd_start;
> + }
> +}
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
