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

[Daniel Kiper]

Hi Mislav,

First of all, please do not top post and wrap your long lines...

On Thu, Oct 07, 2021 at 09:23:14AM +0000, Mislav Stublić wrote:
> Hi,
>
> There is a lot of duplication as I have used existing implementation
> as a reference (arm, aarch64 and fdt). But refactoring existing
> implementation atop of implementing x86 didn’t seem realistic as those
> modules are quite different... at least when I first looked at this.
> My biggest question is how to unify existing memory allocation
> procedures as those are different, at least  in a sense x86 has
> relocator and the setup around it (or can/should this unification be
> avoided). Also, it seems there is a lot of divergence in UEFI
> implementations for which I'm not sure it can be avoided without
> bigger refactor of both UEFI and x86 code. But I would need to think
> some more on your suggestion, maybe it's not so complicated.

I am not sure you understand what I want. I do not ask you to unify UEFI
device tree code with non-UEFI device tree code. This can be difficult (but
not impossible). So, I want to have two device tree implementations: one
for UEFI platforms and another for non-UEFI platforms (when you start
working on this there is a chance we will see how to merge both into one;
though let's start with two implementations for simplicity). Then every
architecture/CPU implementation should use the former or the latter.
Does it sound OK for you?

Daniel

> Mislav
>
> > -----Original Message-----
> > From: Daniel Kiper [mailto:dkiper@net-space.pl]
> > Sent: Wednesday, October 06, 2021 2:29 PM
> > To: Mislav Stublić
> > Cc: grub-devel@gnu.org
> > Subject: Re: [PATCH] loader/i386/linux: Add device tree support
> >
> > Hi Mislav,
> >
> > On Tue, Sep 21, 2021 at 03:37:45PM +0000, Mislav Stublić wrote:
> > > Hi,
> > >
> > > This implements device tree support for x86. Unfortunately I haven't
> > > been able to test this on latest master but I have tested against 2.04
> > > and it works fine. I will test on master later but I wanted to get
> > > some initial comments if this is going in the right direction. What I
> > > haven't tested is firmware DTB loading support which only calls
> > > grub_efi_get_firmware_fdt from kern/efi implementation as I don't have
> > > hardware to test this scenario.
> >
> > I skimmed through your patch and it seems to me you are duplicating at
> > least some code from grub-core/loader/arm/linux.c. I would do this a bit
> > differently. First of all I would factor out non-UEFI device tree code
> > from grub-core/loader/arm/linux.c to a separate module. This should be
> > a separate patch. Then I would try to unify the interface for UEFI and
> > non-UEFI device tree code (again, separate patch). Of course if needed.
> > Then the last patch should add device tree support to the x86. If you
> > have any questions drop me a line.
> >
> > Anyway, thank you for taking a stab at this.
> >
> > Daniel