[patch] PPC cleanups

[patch] PPC cleanups

[Hollis Blanchard]

This patch has a lot of long-overdue PPC cleanups, including build
warnings. Since we all know the Changelog doesn't explain enough about a
patch, a quick overview:
- include/grub/powerpc/ieee1275/init.h was useless.
- roundup() was unused.
- rather than everyone accessing grub_ieee1275_flags directly, it's now
  static and provides test/set accessors.
- grub_ieee1275_realmode should have always been a flag instead of a
  global; I don't know what I was thinking at the time.
- ofdisk.h is a now real header (like biosdisk.h); no more
  grub_ofdisk_fini/grub_ofdisk_init "missing prototype" warnings.

Important new functionality:
- actually boot a Linux kernel successfully (don't prematurely free memory)
- properly handle partition numbering on briQ and Pegasos

I have boot-tested this patch on briQ and G3, and would like to check it
in soon.

I also have my sights on include/grub/powerpc/ieee1275/biosdisk.h. Any
suggestions on how to handle using util/i386/pc/biosdisk.c on PPC for
grub-emu?

-Hollis

2005-04-21  Hollis Blanchard  <hollis@penguinppc.org>

	* boot/powerpc/ieee1275/cmain.c: Don't include grub/machine/init.h.
	(roundup): Remove macro.
	(grub_ieee1275_flags): Make static.
	(grub_ieee1275_realmode): Remove.
	(grub_ieee1275_test_flag): New function.
	(grub_ieee1275_set_flag): Likewise.
	(find_options): Rename to `grub_ieee1275_find_options'; update
	callers. Set GRUB_IEEE1275_REAL_MODE and
	GRUB_IEEE1275_0_BASED_PARTITIONS.
	(cmain): New prototype.
	(cmain): Use `grub_ieee1275_set_flag' instead of accessing
	`grub_ieee1275_flags' directly.
	* conf/powerpc-ieee1275.rmk (grubof_HEADERS): Remove
	machine/biosdisk.h.
	* disk/powerpc/ieee1275/ofdisk.c: Include grub/machine/ofdisk.h. Don't
	include grub/machine/init.h.
	(grub_ofdisk_open): Call `grub_ieee1275_test_flag'.
	* include/grub/powerpc/ieee1275/ieee1275.h (grub_ieee1275_flags):
	Remove prototype.
	(grub_ieee1275_realmode): Likewise.
	(grub_ieee1275_flag): New enum.
	(grub_ieee1275_test_flag): New prototype.
	(grub_ieee1275_set_flag): New prototype.
	* include/grub/powerpc/ieee1275/init.h: Remove file.
	* include/grub/powerpc/ieee1275/ofdisk.h: New file.
	* kern/powerpc/ieee1275/init.c: Don't include grub/machine/init.h.
	Include grub/machine/console.h. Include grub/machine/ofdisk.h.
	(abort): Correct whitespace.
	(grub_machine_fini): Don't call `grub_ieee1275_release'. Remove
	comment.
	* kern/powerpc/ieee1275/openfw.c (grub_claimmap): Call
	`grub_ieee1275_test_flag'.
	(grub_ieee1275_encode_devname): Likewise.

Index: boot/powerpc/ieee1275/cmain.c
===================================================================
RCS file: /cvsroot/grub/grub2/boot/powerpc/ieee1275/cmain.c,v
retrieving revision 1.6
diff -u -p -r1.6 cmain.c
--- boot/powerpc/ieee1275/cmain.c	3 Jan 2005 17:44:25 -0000	1.6
+++ boot/powerpc/ieee1275/cmain.c	22 Apr 2005 03:44:22 -0000
@@ -23,7 +23,6 @@
 
 #include <grub/machine/ieee1275.h>
 #include <grub/kernel.h>
-#include <grub/machine/init.h>
 
 struct module_info
 {
@@ -31,26 +30,47 @@ struct module_info
   uint32_t end;
 };
 
-#define roundup(a, s) (((a) + ((s) - 1)) & ~((s) - 1))
-
 /* OpenFirmware entry point passed to us from the real bootloader.  */
 intptr_t (*grub_ieee1275_entry_fn) (void *);
 
-grub_uint32_t grub_ieee1275_flags;
-int grub_ieee1275_realmode;
+static grub_uint32_t grub_ieee1275_flags;
 
 \f
 
+int
+grub_ieee1275_test_flag (enum grub_ieee1275_flag flag)
+{
+  return (grub_ieee1275_flags & (1 << flag));
+}
+
+void
+grub_ieee1275_set_flag (enum grub_ieee1275_flag flag)
+{
+  grub_ieee1275_flags |= (1 << flag);
+}
+
 static void
-find_options (void)
+grub_ieee1275_find_options (void)
 {
   grub_ieee1275_phandle_t options;
+  grub_ieee1275_phandle_t openprom;
+  int realmode;
+  int smartfw;
 
   grub_ieee1275_finddevice ("/options", &options);
-  grub_ieee1275_get_property (options, "real-mode?", &grub_ieee1275_realmode,
-			      sizeof (grub_ieee1275_realmode), 0);
+  grub_ieee1275_get_property (options, "real-mode?", &realmode,
+			      sizeof (realmode), 0);
+  if (realmode)
+    grub_ieee1275_set_flag (GRUB_IEEE1275_REAL_MODE);
+
+  grub_ieee1275_finddevice ("/openprom", &openprom);
+  smartfw = grub_ieee1275_get_property (openprom, "SmartFirmware-version",
+			       0, 0, 0);
+  if (smartfw)
+    grub_ieee1275_set_flag (GRUB_IEEE1275_0_BASED_PARTITIONS);
 }
 
+void cmain (uint32_t r3, uint32_t r4, uint32_t r5);
 /* Setup the argument vector and pass control over to the main
    function.  */
 void
@@ -68,7 +88,7 @@ cmain (uint32_t r3, uint32_t r4 __attrib
 
       grub_ieee1275_entry_fn = (intptr_t (*)(void *)) r3;
 
-      grub_ieee1275_flags = GRUB_IEEE1275_NO_PARTITION_0;
+      grub_ieee1275_set_flag (GRUB_IEEE1275_NO_PARTITION_0);
 
       /* Old World Open Firmware may use 4M-5M without claiming it.  */
       grub_ieee1275_claim (0x00400000, 0x00100000, 0, 0);
@@ -84,7 +104,7 @@ cmain (uint32_t r3, uint32_t r4 __attrib
       grub_ieee1275_entry_fn = (intptr_t (*)(void *)) r5;
     }
 
-  find_options ();
+  grub_ieee1275_find_options ();
 
   /* If any argument was passed to the kernel (us), they are
      put in the bootargs property of /chosen.  The string can
Index: conf/powerpc-ieee1275.rmk
===================================================================
RCS file: /cvsroot/grub/grub2/conf/powerpc-ieee1275.rmk,v
retrieving revision 1.29
diff -u -p -r1.29 powerpc-ieee1275.rmk
--- conf/powerpc-ieee1275.rmk	26 Mar 2005 17:34:50 -0000	1.29
+++ conf/powerpc-ieee1275.rmk	22 Apr 2005 03:44:25 -0000
@@ -11,7 +11,7 @@ DEFSYMFILES += kernel_syms.lst
 
 grubof_HEADERS = arg.h boot.h device.h disk.h dl.h elf.h env.h err.h \
 	file.h fs.h kernel.h misc.h mm.h net.h rescue.h symbol.h \
-	term.h types.h machine/biosdisk.h powerpc/libgcc.h loader.h \
+	term.h types.h powerpc/libgcc.h loader.h \
 	partition.h pc_partition.h machine/time.h machine/ieee1275.h
 
 grubof_symlist.c: $(addprefix include/grub/,$(grubof_HEADERS)) gensymlist.sh
Index: disk/powerpc/ieee1275/ofdisk.c
===================================================================
RCS file: /cvsroot/grub/grub2/disk/powerpc/ieee1275/ofdisk.c,v
retrieving revision 1.8
diff -u -p -r1.8 ofdisk.c
--- disk/powerpc/ieee1275/ofdisk.c	26 Mar 2005 17:34:50 -0000	1.8
+++ disk/powerpc/ieee1275/ofdisk.c	22 Apr 2005 03:44:25 -0000
@@ -22,7 +22,7 @@
 #include <grub/disk.h>
 #include <grub/mm.h>
 #include <grub/machine/ieee1275.h>
-#include <grub/machine/init.h>
+#include <grub/machine/ofdisk.h>
 
 static int
 grub_ofdisk_iterate (int (*hook) (const char *name))
@@ -60,7 +60,7 @@ grub_ofdisk_open (const char *name, grub
     return grub_errno;
 
   /* To access the complete disk add `:0'.  */
-  if (! (grub_ieee1275_flags & GRUB_IEEE1275_NO_PARTITION_0))
+  if (! grub_ieee1275_test_flag (GRUB_IEEE1275_NO_PARTITION_0))
     grub_strcat (devpath, ":0");
 
   grub_ieee1275_open (devpath, &dev_ihandle);
Index: include/grub/powerpc/ieee1275/ieee1275.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/powerpc/ieee1275/ieee1275.h,v
retrieving revision 1.16
diff -u -p -r1.16 ieee1275.h
--- include/grub/powerpc/ieee1275/ieee1275.h	22 Apr 2005 02:32:37 -0000	1.16
+++ include/grub/powerpc/ieee1275/ieee1275.h	22 Apr 2005 03:44:25 -0000
@@ -65,11 +65,24 @@ typedef intptr_t grub_ieee1275_ihandle_t
 typedef intptr_t grub_ieee1275_phandle_t;
 
 extern intptr_t (*grub_ieee1275_entry_fn) (void *);
-extern grub_uint32_t grub_ieee1275_flags;
-extern int grub_ieee1275_realmode;
 
-/* Old World firmware fails seek when "dev:0" is opened.  */
-#define GRUB_IEEE1275_NO_PARTITION_0 0x1
+enum grub_ieee1275_flag
+{
+  /* Old World firmware fails seek when "dev:0" is opened.  */
+  GRUB_IEEE1275_NO_PARTITION_0,
+
+  /* Apple firmware runs in translated mode and requires use of the "map"
+     method.  Other firmware runs in untranslated mode and doesn't like "map"
+     calls.  */
+  GRUB_IEEE1275_REAL_MODE,
+
+  /* CHRP specifies partitions are numbered from 1 (partition 0 refers to the
+     whole disk). However, CodeGen firmware numbers partitions from 0.  */
+  GRUB_IEEE1275_0_BASED_PARTITIONS,
+};
+
+extern int EXPORT_FUNC(grub_ieee1275_test_flag) (enum grub_ieee1275_flag flag);
+extern void EXPORT_FUNC(grub_ieee1275_set_flag) (enum grub_ieee1275_flag flag);
 
 \f
 
Index: include/grub/powerpc/ieee1275/init.h
===================================================================
RCS file: include/grub/powerpc/ieee1275/init.h
diff -N include/grub/powerpc/ieee1275/init.h
--- include/grub/powerpc/ieee1275/init.h	26 Mar 2005 17:34:50 -0000	1.2
+++ /dev/null	1 Jan 1970 00:00:00 -0000
@@ -1,27 +0,0 @@
-/*
- *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2004  Free Software Foundation, Inc.
- *
- *  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.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
- */
-
-#ifndef GRUB_INIT_MACHINE_HEADER
-#define GRUB_INIT_MACHINE_HEADER	1
-
-void cmain (uint32_t r3, uint32_t r4, uint32_t r5);
-void grub_ofdisk_init (void);
-void grub_console_init (void);
-
-#endif /* ! GRUB_INIT_MACHINE_HEADER */
Index: include/grub/powerpc/ieee1275/ofdisk.h
===================================================================
RCS file: include/grub/powerpc/ieee1275/ofdisk.h
diff -N include/grub/powerpc/ieee1275/ofdisk.h
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ include/grub/powerpc/ieee1275/ofdisk.h	22 Apr 2005 03:44:25 -0000
@@ -0,0 +1,26 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2005  Free Software Foundation, Inc.
+ *
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef GRUB_OFDISK_MACHINE_HEADER
+#define GRUB_OFDISK_MACHINE_HEADER	1
+
+extern void grub_ofdisk_init (void);
+extern void grub_ofdisk_fini (void);
+
+#endif /* ! GRUB_INIT_MACHINE_HEADER */
Index: kern/powerpc/ieee1275/init.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/init.c,v
retrieving revision 1.16
diff -u -p -r1.16 init.c
--- kern/powerpc/ieee1275/init.c	22 Apr 2005 02:32:37 -0000	1.16
+++ kern/powerpc/ieee1275/init.c	22 Apr 2005 03:44:25 -0000
@@ -29,9 +29,10 @@
 #include <grub/setjmp.h>
 #include <grub/env.h>
 #include <grub/misc.h>
-#include <grub/machine/init.h>
 #include <grub/machine/time.h>
 #include <grub/machine/kernel.h>
+#include <grub/machine/console.h>
+#include <grub/machine/ofdisk.h>
 
 /* Apple OF 1.0.5 reserves 0x0 to 0x4000 for the exception handlers.  */
 static const grub_addr_t grub_heap_start = 0x4000;
@@ -42,7 +43,7 @@ abort (void)
 {
   /* Trap to Open Firmware.  */
   asm ("trap");
-  
+
   for (;;);
 }
 
@@ -138,9 +139,6 @@ grub_machine_fini (void)
 {
   grub_ofdisk_fini ();
   grub_console_fini ();
-
-  grub_ieee1275_release (grub_heap_start, grub_heap_len);
-  /* XXX Release memory claimed for Old World firmware.  */
 }
 
 void
Index: kern/powerpc/ieee1275/openfw.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/openfw.c,v
retrieving revision 1.9
diff -u -p -r1.9 openfw.c
--- kern/powerpc/ieee1275/openfw.c	22 Apr 2005 02:32:37 -0000	1.9
+++ kern/powerpc/ieee1275/openfw.c	22 Apr 2005 03:44:25 -0000
@@ -196,7 +196,8 @@ grub_claimmap (grub_addr_t addr, grub_si
   if (grub_ieee1275_claim (addr, size, 0, 0))
     return -1;
 
-  if ((! grub_ieee1275_realmode) && grub_map (addr, addr, size, 0x00))
+  if (! grub_ieee1275_test_flag (GRUB_IEEE1275_REAL_MODE)
+      && grub_map (addr, addr, size, 0x00))
     {
       grub_printf ("map failed: address 0x%x, size 0x%x\n", addr, size);
       grub_ieee1275_release (addr, size);
@@ -338,7 +339,10 @@ grub_ieee1275_encode_devname (const char
   if (partition)
     {
       unsigned int partno = grub_strtoul (partition, 0, 0);
-      partno--; /* GRUB partition numbering is 0-based.  */
+
+      /* GRUB partition numbering is 0-based.  */
+      if (! grub_ieee1275_test_flag (GRUB_IEEE1275_0_BASED_PARTITIONS))
+	partno--;
 
       /* Assume partno will require less than five bytes to encode.  */
       encoding = grub_malloc (grub_strlen (device) + 3 + 5);

Re: [patch] PPC cleanups

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> This patch has a lot of long-overdue PPC cleanups, including build
> warnings. Since we all know the Changelog doesn't explain enough about a
> patch, a quick overview:
> - include/grub/powerpc/ieee1275/init.h was useless.
> - roundup() was unused.
> - rather than everyone accessing grub_ieee1275_flags directly, it's now
>   static and provides test/set accessors.
> - grub_ieee1275_realmode should have always been a flag instead of a
>   global; I don't know what I was thinking at the time.
> - ofdisk.h is a now real header (like biosdisk.h); no more
>   grub_ofdisk_fini/grub_ofdisk_init "missing prototype" warnings.
>
> Important new functionality:
> - actually boot a Linux kernel successfully (don't prematurely free memory)
> - properly handle partition numbering on briQ and Pegasos
>
> I have boot-tested this patch on briQ and G3, and would like to check it
> in soon.

The patch looks fine to me, although I have a few small comments, see
what I write below.  This evening I am going to test the patch to see
if it works for me.

> I also have my sights on include/grub/powerpc/ieee1275/biosdisk.h. Any
> suggestions on how to handle using util/i386/pc/biosdisk.c on PPC for
> grub-emu?

There was a discussion about this before, please read it for details I
might have forgotten.  But ATM the PC version works for the PPC as
well.  There is no platform specific code there yet and I want to keep
things shared as long as it is possible.  Doesn't it work for you or
are there any issues?  The biosdisk code is the code, together with
the i386 specific code, I am the least familiar with.

> 2005-04-21  Hollis Blanchard  <hollis@penguinppc.org>

> 	* disk/powerpc/ieee1275/ofdisk.c: Include grub/machine/ofdisk.h. Don't
> 	include grub/machine/init.h.

Two spaces are required here, please fix this in the other places in
the changelog entry/comments as well.

> 	* include/grub/powerpc/ieee1275/ieee1275.h (grub_ieee1275_flags):
> 	Remove prototype.
> 	(grub_ieee1275_realmode): Likewise.
> 	(grub_ieee1275_flag): New enum.

Can you give the enums a prefix?  For example
`GRUB_IEEE1275_NO_PARTITION_0' can be changed to
`GRUB_IEEE1275_FLAGS_NO_PARTITION_0'.

> 	* kern/powerpc/ieee1275/init.c: Don't include grub/machine/init.h.
> 	Include grub/machine/console.h. Include grub/machine/ofdisk.h.
> 	(abort): Correct whitespace.

Huh?  What is this whitespace change?  And there is no need to mention
it in the changelog in case it is required.

--
Marco

Re: [patch] PPC cleanups

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

Hi Hollis,

Here is my final report.  I have tested your patch on the PegasosII.
I assume you properly tested it on the G3.  You can check in your
patch if you make the changes I proposed in this and my previous
email.  Please update the copyright years of the files you changes, if
they are not set correctly.

> +  grub_ieee1275_finddevice ("/openprom", &openprom);
> +  smartfw = grub_ieee1275_get_property (openprom, "SmartFirmware-version",
> +			       0, 0, 0);

Please align the make sure `0, 0, 0' are properly aligned with
`openprom, "SmartF...'.

> +  if (smartfw)
> +    grub_ieee1275_set_flag (GRUB_IEEE1275_0_BASED_PARTITIONS);

This did not work on the PegasosII.  I fixed this by changing this
test to:

if (smartfw == 0)
  grub_ieee1275_set_flag (GRUB_IEEE1275_0_BASED_PARTITIONS);

Thanks,
Marco