Re: [PATCH 3/7] Initial support for U-Boot platforms

[Francesco Lavra]

Mostly cosmetic comments from my side here...

On 03/24/2013 06:01 PM, Leif Lindholm wrote:
[...]
> === added file 'grub-core/disk/uboot/ubootdisk.c'
> --- grub-core/disk/uboot/ubootdisk.c  1970-01-01 00:00:00 +0000
> +++ grub-core/disk/uboot/ubootdisk.c  2013-03-24 12:58:23 +0000
[...]
> +/*
> + * uboot_disk_iterate():
> + *   Itarator over enumerated disk devices.

s/Itarator/Iterator

[...]
> === added file 'grub-core/kern/uboot/init.c'
> --- grub-core/kern/uboot/init.c       1970-01-01 00:00:00 +0000
> +++ grub-core/kern/uboot/init.c       2013-03-24 12:58:23 +0000
[...]
> +/*
> + * grub_machine_get_bootlocation():
> + *   Called from kern/main.c, which expects a device name (minus parentheses)
> + *   and a filesystem path back, if any are known.
> + *   Any returned values must be pointers to dynamically allocated strings.
> + */
> +void
> +grub_machine_get_bootlocation (char **device, char **path)
> +{
> +  char *tmp;
> +
> +  tmp = uboot_env_get ("grub_bootdev");
> +  if (tmp)
> +    {
> +      *device = grub_malloc (grub_strlen (tmp) + 1);
> +      if (*device == NULL)
> +     return;
> +      grub_strncpy (*device, tmp, grub_strlen (tmp) + 1);

Why not simply call grub_strcpy (*device, tmp)?

> +    }
> +  else
> +    *device = NULL;
> +
> +  tmp = uboot_env_get ("grub_bootpath");
> +  if (tmp)
> +    {
> +      *path = grub_malloc (grub_strlen (tmp) + 1);
> +      if (*path == NULL)
> +     return;
> +      grub_strncpy (*path, tmp, grub_strlen (tmp) + 1);

Ditto, grub_strncpy is not needed.

> +    }
> +  else
> +    *path = NULL;
> +}
> +
> +void
> +grub_uboot_fini (void)
> +{
> +  grub_ubootdisk_fini ();
> +  grub_console_fini ();
> +}
> 
> === added file 'grub-core/kern/uboot/uboot.c'
> --- grub-core/kern/uboot/uboot.c      1970-01-01 00:00:00 +0000
> +++ grub-core/kern/uboot/uboot.c      2013-03-24 12:58:23 +0000
[...]
> +/* All functions below are wrappers around the uboot_syscall() function */
> +
> +/*
> + * int API_getc(int *c)
> + */
> +int
> +uboot_getc (void)

The comment preceding the function does not correspond to the function
prototype. This applies as well to all the subsequent functions in this
file.

[...]
> +int
> +uboot_dev_enum (void)
> +{
> +  int max;
> +
> +  grub_memset (&uboot_devices, 0, sizeof (uboot_devices));
> +  max = sizeof (uboot_devices) / sizeof (struct device_info);
> +
> +  /*
> +   * The API_DEV_ENUM call starts a fresh enumeration when passed a
> +   * struct device_info with a NULL cookie, and then depends on having
> +   * the prevoiusly enumerated device cookie "seeded" into the target

s/prevoiusly/previously.

[...]
> === added file 'include/grub/uboot/uboot.h'
> --- include/grub/uboot/uboot.h        1970-01-01 00:00:00 +0000
> +++ include/grub/uboot/uboot.h        2013-03-24 12:58:23 +0000
> @@ -0,0 +1,150 @@
> +/* uboot.h - declare variables and functions for U-Boot support */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2013 Free Software Foundation, Inc.
> + *
> + *  GRUB 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 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB 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 GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef GRUB_UBOOT_UBOOT_HEADER
> +#define GRUB_UBOOT_UBOOT_HEADER      1
> +
> +#include <grub/types.h>
> +#include <grub/dl.h>
> +
> +/* Functions.  */
> +void grub_uboot_mm_init (void);
> +void grub_uboot_init (void);
> +void grub_uboot_fini (void);
> +
> +void uboot_return (int) __attribute__ ((noreturn));
> +
> +grub_addr_t uboot_get_real_bss_start (void);
> +
> +grub_err_t grub_uboot_probe_hardware (void);
> +
> +extern grub_addr_t EXPORT_VAR (start_of_ram);
> +
> +grub_uint32_t EXPORT_FUNC (uboot_get_machine_type) (void);
> +grub_addr_t EXPORT_FUNC (uboot_get_boot_data) (void);
> +
> +
> +/*
> + * The U-Boot API operates through a "syscall" interface, consisting of an
> + * entry point address and a set of syscall numbers. The location of this
> + * entry point is described in a structure allocated on the U-Boot heap.
> + * We scan through a defined region around the hint address passed to us
> + * from U-Boot.
> + */
> +#include <grub/uboot/api_public.h>
> +
> +#define UBOOT_API_SEARCH_LEN (3 * 1024 * 1024)
> +int uboot_api_init (void);
> +
> +/* All functions below are wrappers around the uboot_syscall() function */
> +
> +/*
> + * int API_getc(int *c)
> + */
> +int uboot_getc (void);

Same as for kern/uboot/uboot.c: the comments preceding the functions do
not correspond to the function prototypes.

--
Francesco