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

[Leif Lindholm]

On Mon, Apr 01, 2013 at 04:08:59AM +0200, Vladimir '??-coder/phcoder' 
Serbinenko wrote:
> 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.
 
So, move stack to after heap? Sure.

> > +#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
 
Yes, sorry, I meant to get rid of that special case handling even after talking
to Colin back in November.

> > +  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?

Not that I've found. As in, not that can be relied on.
 
> > +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.

Will do.
 
> > +/*
> > + * 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.
 
No, it's just an image loaded from memory as far as U-Boot is concerned.
That it might previously have been loaded from a filesystem is not kept
around anywhere. Anyway - this would only end up being used if the
embedded config failed.

> > +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?
 
To be honest, I don't end up calling it anywhere except for a very
early error message before the console is initialised. And that call
would likely fail.

It's provided by the API, so I implemeted the wrapper. Can drop?

> > +  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?

A bit lazily ...
The API_GET_SYS_INFO call returns the number of DRAM banks in the
system into this array. I can make it dynamic, but it is always
going to be a fairly low number (not like EFI).

> > + * 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
 
OK.

> > +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?
 
Originally to permit range checking. Superflouos?

> > +/* 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"
 
That's certainly an option. Should I do that?

> > +void
> > +grub_halt (void)
> > +{
> > +  grub_machine_fini ();
> > +
> > +  /* Just stop here */
> > +
> 
> Doesn't uboot have a way to shutdown a machine?
 
No. It has reset, but I couldn't convince myself that was more right than
just looping..

> > +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.
 
Yeah, I might have been cargo culting ieee1275 around this region.
All changed as suggested.

> > + */
> > +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
 
No strong reason, I suppose.
Was useful during development.
Could drop.

/
    Leif