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

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6318 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]