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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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 = hd;
> +      break;
> +    case DT_STOR_MMC:
> +    case DT_STOR_USB:
> +      /* fd */
> +      type = fd;
> +      break;

Problem is that --no-floppy would skip all those devices. This is
problem that uboot will be different from other platforms

> +  d = (struct ubootdisk_data *) grub_malloc (sizeof (struct ubootdisk_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 = devices; d && num; d = 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 = uboot_dev_get (d->handle)) == NULL)
> +    goto fail;

Please put assignment before if.

> +  disk->total_sectors = 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 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);
> +    }
> +  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);
> +    }
> +  else
> +    *path = 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 = uboot_mem_info;
> +  uboot_sys_info.mr_no = sizeof (uboot_mem_info) / sizeof (struct 
> mem_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 = 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__ ((unused)))
> +{
> +  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 = 80;
> +  if (!uboot_console_terminfo_output.height)
> +    uboot_console_terminfo_output.height = 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 = uboot_env_get ("grub_term");
> +  if (!type)
> +    type = "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

signature.asc
Description: OpenPGP digital signature