RE: [PATCH] loader/i386/linux: Add device tree support

[Mislav Stublić]

> > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > index 8022e1c..e499057 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -227,7 +227,9 @@ kernel = {
> >    x86_64_xen = kern/x86_64/dl.c;
> >    x86_64_efi = kern/x86_64/efi/callwrap.S;
> >    x86_64_efi = kern/i386/efi/init.c;
> > +  x86_64_efi = kern/efi/fdt.c;
> >    x86_64_efi = bus/pci.c;
> > +  x86_64_efi = lib/fdt.c;
> Any reason to put it into kernel rather than enabling fdt module on x86?
I followed  a similar setup that is done for arm coreboot, it looks as if it's 
just a common lib to be included. Are modules added through GROUPS in 
gentpl.py. Makefile.am generator in grub is a bit opaque to me.

> >  #ifdef GRUB_MACHINE_EFI
> >    {
> >      grub_efi_uintn_t efi_desc_size;
> > @@ -591,9 +680,9 @@ grub_linux_boot (void)
> >                                          &efi_desc_size, &efi_desc_version);
> >      if (err)
> >        return err;
> > -
> > +
> Please don't include empty line changes
Here I am cleaning up whitespace, empty line already existed.

> > +  if (current_fdt)
> > +    grub_free (current_fdt);
> > +  current_fdt = NULL;
> > +
> > +#ifdef GRUB_MACHINE_EFI
> > +  /* no dtb file, try firmware */
> > +  if (argc == 0)
> > +    {
> > +      current_fdt = grub_efi_get_firmware_fdt();
> You may want to have an implementation than always returns NULL on
> other platforms
You mean to limit the scope of EFI ifdef to just firmware call and otherwise 
just set/leave current_dtb to NULL? Sure.

> > @@ -1131,6 +1279,9 @@ GRUB_MOD_INIT(linux)
> >                                      0, N_("Load Linux."));
> >    cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd,
> >                                       0, N_("Load initrd."));
> > +  cmd_devicetree = grub_register_command_lockdown ("devicetree",
> grub_cmd_devicetree,
> > +                                                  /* TRANSLATORS: DTB 
> > stands for device tree
> blob.  */
> > +                                                  0, N_("Load DTB file."));
> Please use the same sentence as in another loaders
Hm, this was c/p from another loader so it must have gotten messed up at some 
point.

Mislav


----- Disclaimer -----
This e-mail message and its attachments may contain privileged and/or 
confidential information. Please do not read the message if You are not its 
designated recipient. If You have received this message by mistake, please 
inform its sender and destroy the original message and its attachments without 
reading or storing of any kind. Any unauthorized use, distribution, 
reproduction or publication of this message is forbidden.

----- Pravne napomene -----
Ova elektronicka poruka i njeni prilozi mogu sadrzavati povlastene informacije 
i/ili povjerljive informacije. Molimo Vas da poruku ne citate ako niste njen 
naznaceni primatelj. Ako ste ovu poruku primili greskom, molimo Vas da o tome 
obavijestite posiljatelja i da izvornu poruku i njene privitke unistite bez 
citanja ili bilo kakvog pohranjivanja. Svaka neovlastena upotreba, 
distribucija, reprodukcija ili priopcavanje ove poruke zabranjena je.