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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel