From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1UMUBI-000300-Ko for mharc-grub-devel@gnu.org; Sun, 31 Mar 2013 22:09:08 -0400 Received: from eggs.gnu.org ([208.118.235.92]:35928) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UMUBF-0002vj-14 for grub-devel@gnu.org; Sun, 31 Mar 2013 22:09:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UMUBD-0003bc-66 for grub-devel@gnu.org; Sun, 31 Mar 2013 22:09:04 -0400 Received: from mail-ee0-f44.google.com ([74.125.83.44]:61959) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UMUBC-0003bY-Ro for grub-devel@gnu.org; Sun, 31 Mar 2013 22:09:03 -0400 Received: by mail-ee0-f44.google.com with SMTP id l10so860865eei.17 for ; Sun, 31 Mar 2013 19:09:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:subject :references:in-reply-to:x-enigmail-version:content-type; bh=7hzPdnC5g+KcsyTBTwdQjzjBz4KADCfJnSB0L1UUB9Q=; b=wp5WnWIN6foMh8Jjz3b76RLHZMMGBkWKZeL9+LhIoo2DudIcBFzBAQkg2vxBERSbPA HrVfmLYpw9st3+otU7c/9bvZpY4wR95ZfiAXEST1Xa/s+OYGJRHGFB0rYhjFkQbKIlg1 HHv/bw4IFjC1A5gEwhCK8IUjT0fkdijBcDr73+azp5PpHKvOJA3CAMp3AYDBHyFmCgwS ThRxLefHHSJCqW6F1862kr7dYd284obvBVJKsItZ3rzN47FrvAJhBmtob7cDSFbX3kgW 9f8NZvzTl6HQLJq3SyWE7yKvBD+y/cb28JsAJjA1kmEW4XyTaMPRnVxqNCToadx4ws/8 qn1Q== X-Received: by 10.14.223.69 with SMTP id u45mr32450235eep.23.1364782142035; Sun, 31 Mar 2013 19:09:02 -0700 (PDT) Received: from debian.x201.phnet (245-188.1-85.cust.bluewin.ch. [85.1.188.245]) by mx.google.com with ESMTPS id d47sm18537205eem.9.2013.03.31.19.09.00 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sun, 31 Mar 2013 19:09:01 -0700 (PDT) Message-ID: <5158EC3B.8050504@gmail.com> Date: Mon, 01 Apr 2013 04:08:59 +0200 From: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130116 Icedove/10.0.12 MIME-Version: 1.0 To: The development of GNU GRUB Subject: Re: [PATCH 3/7] Initial support for U-Boot platforms References: In-Reply-To: X-Enigmail-Version: 1.4.1 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="------------enig26CCABEAD10EA2095BC41891" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 74.125.83.44 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Apr 2013 02:09:07 -0000 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig26CCABEAD10EA2095BC41891 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 24.03.2013 18:01, Leif Lindholm wrote: About memory map: It would make sense to put modules right before heap as module space is reused later as heap if this is enabled. > +#define STOR_TYPE(x) ((x) & 0x0ff0) > + switch (STOR_TYPE (newdev->type)) > + { > + case DT_STOR_IDE: > + case DT_STOR_SATA: > + /* hd */ > + type =3D hd; > + break; > + case DT_STOR_MMC: > + case DT_STOR_USB: > + /* fd */ > + type =3D fd; > + break; Problem is that --no-floppy would skip all those devices. This is problem that uboot will be different from other platforms > + d =3D (struct ubootdisk_data *) grub_malloc (sizeof (struct ubootdis= k_data)); > + if (!d) > + return GRUB_ERR_OUT_OF_MEMORY; Just "return grub_errno;" > +/* Helper function for uboot_disk_open. */ > +static struct ubootdisk_data * > +get_device (struct ubootdisk_data *devices, int num) > +{ > + struct ubootdisk_data *d; > + > + for (d =3D devices; d && num; d =3D d->next, num--) > + ; Why not just use an array and either double iteration to fill it (first count, allocate, then fill) or double its size every time as needed (like x2realloc) > + /* Device has previously been enumerated, so this should never fail = */ > + if ((devinfo =3D uboot_dev_get (d->handle)) =3D=3D NULL) > + goto fail; Please put assignment before if. > + disk->total_sectors =3D GRUB_DISK_SIZE_UNKNOWN; Is there any way to get size from uboot? > +static grub_err_t > +uboot_disk_write (struct grub_disk *disk __attribute__ ((unused)), > + grub_disk_addr_t sector __attribute__ ((unused)), > + grub_size_t size __attribute__ ((unused)), > + const char *buf __attribute__ ((unused))) > +{ > + grub_dprintf ("ubootdisk", "attempt to write\n"); > + return GRUB_ERR_NOT_IMPLEMENTED_YET; > +} Could you make it use grub_error ?> +grub_uint32_t > +uboot_get_machine_type (void) > +{ > + return uboot_machine_type; > +} > + Please use grub_ prefix for all global functions. > +/* > + * grub_machine_get_bootlocation(): > + * Called from kern/main.c, which expects a device name (minus paren= theses) > + * and a filesystem path back, if any are known. > + * Any returned values must be pointers to dynamically allocated str= ings. > + */ > +void > +grub_machine_get_bootlocation (char **device, char **path) > +{ > + char *tmp; > + > + tmp =3D uboot_env_get ("grub_bootdev"); > + if (tmp) > + { > + *device =3D grub_malloc (grub_strlen (tmp) + 1); > + if (*device =3D=3D NULL) > + return; > + grub_strncpy (*device, tmp, grub_strlen (tmp) + 1); > + } > + else > + *device =3D NULL; > + > + tmp =3D uboot_env_get ("grub_bootpath"); > + if (tmp) > + { > + *path =3D grub_malloc (grub_strlen (tmp) + 1); > + if (*path =3D=3D NULL) > + return; > + grub_strncpy (*path, tmp, grub_strlen (tmp) + 1); > + } > + else > + *path =3D NULL; > +} Why special variables for GRUB? Doesn't uboot tell where .elf was loaded from. > +extern int (*uboot_syscall_ptr) (int, int *, ...); > +extern int uboot_syscall (int, int *, ...); > +extern grub_addr_t uboot_search_hint; Please grub_ prefix > +/* > + * int API_puts(const char *s) > + */ > +void > +uboot_puts (const char *s) > +{ > + uboot_syscall (API_PUTS, NULL, s); > +} Why do you need puts rather than use xputs? > + grub_memset (&uboot_sys_info, 0, sizeof (uboot_sys_info)); > + grub_memset (&uboot_mem_info, 0, sizeof (uboot_mem_info)); > + uboot_sys_info.mr =3D uboot_mem_info; > + uboot_sys_info.mr_no =3D sizeof (uboot_mem_info) / sizeof (struct me= m_region); How is the size of this table chosen? Shouldn't you retry the call with more entries if it fails? > + * int API_dev_enum(struct device_info *) > + * > + */ > +int > +uboot_dev_enum (void) > +{ > + int max; > + > + grub_memset (&uboot_devices, 0, sizeof (uboot_devices)); > + max =3D sizeof (uboot_devices) / sizeof (struct device_info); > + Please avoid arbitrary limits. In this case it's better to export iterator as-is and allow all users to store the results it uses. Or use realloc in x2realloc algorithm > +struct device_info * > +uboot_dev_get (int handle) > +{ > + if (VALID_DEV (handle)) > + return &uboot_devices[handle]; > + > + return NULL; > +} > + Why have handles and not just pass the structure through? > +/* No simple platform-independent RTC access exists in U-Boot. */ > + > +grub_err_t > +grub_get_datetime (struct grub_datetime *datetime __attribute__ ((unus= ed))) > +{ > + return grub_error (GRUB_ERR_INVALID_COMMAND, > + "can\'t get datetime using U-Boot"); GRUB_ERR_IO, not GRUB_ERR_INVALID_COMMAND. Perhaps it would be a good thing to skip compiling all datetime users on uboot by defining a group "datetime" > +void > +grub_halt (void) > +{ > + grub_machine_fini (); > + > + /* Just stop here */ > + Doesn't uboot have a way to shutdown a machine? > +static void > +uboot_console_setcursor (struct grub_term_output *term > + __attribute__ ((unused)), int on > + __attribute__ ((unused))) > +{ > + grub_terminfo_setcursor (term, on); > +} Just use grub_terminfo_setcursor directly > + > +static grub_err_t > +uboot_console_init_input (struct grub_term_input *term) > +{ > + return grub_terminfo_input_init (term); > +} Likewise > +static void > +uboot_console_dimensions (void) > +{ > + /* Use a small console by default. */ > + if (!uboot_console_terminfo_output.width) > + uboot_console_terminfo_output.width =3D 80; > + if (!uboot_console_terminfo_output.height) > + uboot_console_terminfo_output.height =3D 24; > +} Does uboot interpret this consoel to the screen? You don't need to set 80x24 since it's already statically set to this value. > + */ > +void > +grub_console_init_lately (void) > +{ > + const char *type; > + > + /* See if explicitly set by U-Boot environment */ > + type =3D uboot_env_get ("grub_term"); > + if (!type) > + type =3D "vt100"; Why do you go for a variable rather than adding terminfo in configfile if needed? Does uboot interpret this console or is it relayed by serial? In former case we probably need more appropriate console type --------------enig26CCABEAD10EA2095BC41891 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iF4EAREKAAYFAlFY7DwACgkQNak7dOguQgkU/gEAqQVPyIBljIzU50B422aJrM/w vQIUr/YljgaHbeId/ukA/0dM+sjgiWmPgJIGxhQ4b72xE1aKfYN8ok3HIEOPkr2V =+/qN -----END PGP SIGNATURE----- --------------enig26CCABEAD10EA2095BC41891--