[U-Boot] [PATCH 06/11] mx7dsabresd: Add Windows boot support for iMX7 Sabre

[Henry Beberman]

> -----Original Message-----
> From: Trent Piepho <tpiepho at impinj.com>
> Sent: Tuesday, July 17, 2018 10:03 AM
> To: Henry Beberman <Henry.Beberman at microsoft.com>; u-
> boot at lists.denx.de
> Cc: fabio.estevam at nxp.com; adrian.alonso at nxp.com
> Subject: Re: [U-Boot] [PATCH 06/11] mx7dsabresd: Add Windows boot
> support for iMX7 Sabre
> 
> On Tue, 2018-07-17 at 01:41 +0000, Henry Beberman wrote:
> > > -----Original Message-----
> > > From: Trent Piepho <tpiepho at impinj.com>
> > > Sent: Monday, July 16, 2018 11:22 AM
> > > To: Henry Beberman <Henry.Beberman at microsoft.com>; u-
> > > boot at lists.denx.de
> > > Cc: fabio.estevam at nxp.com; adrian.alonso at nxp.com
> > > Subject: Re: [U-Boot] [PATCH 06/11] mx7dsabresd: Add Windows boot
> > > support for iMX7 Sabre
> > >
> > > On Sat, 2018-07-14 at 00:11 +0000, Henry Beberman wrote:
> > > > From: Henry Beberman <henry.beberman at microsoft.com>
> > > >
> > > > This patch adds a new bootable configuration for Windows 10 IoT
> > > > Core on the i.MX7 Dual Sabre board.
> > > >
> > > > It enables SPL on the i.MX7 Sabre in order to support the FIT load
> > > > of OP-TEE and U-Boot proper.
> > > >
> > > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index
> > > > f186120684..3fb9b72703 100644
> > > > --- a/drivers/gpio/Makefile
> > > > +++ b/drivers/gpio/Makefile
> > > > @@ -7,10 +7,13 @@ ifndef CONFIG_SPL_BUILD
> > > >  obj-$(CONFIG_DWAPB_GPIO)	+= dwapb_gpio.o
> > > >  obj-$(CONFIG_AXP_GPIO)		+= axp_gpio.o
> > > >  endif
> > > > +
> > > > +ifeq ($(CONFIG_$(SPL_TPL_)DM),y)
> > > >  obj-$(CONFIG_DM_GPIO)		+= gpio-uclass.o
> > > >
> > > >  obj-$(CONFIG_DM_PCA953X)	+= pca953x_gpio.o
> > > >  obj-$(CONFIG_DM_74X164)		+= 74x164_gpio.o
> > > > +endif
> > >
> > > It doesn't look like this patch is specific to mx7 or Windows.
> > > Perhaps it should be in a different commit?
> >
> > I agree that this change probably belongs in its own patch.
> > I've left it in this patch so far because it's required to build the new
> mx7dsabresd_nt_defconfig.
> 
> Easy enough to split out and put first.  The person on different hardware
> trying to figure out the u-boot build spaghetti it's helped much knowing the
> person who added it was working on windows 10 support, but nothing about
> the reason for this change.
> 
> > > Also, the help text for SPL_DM says it turns on basic DM support in
> > > SPL.  But not any specific hardware drivers.  Those all have
> > > additional config options to turn them on.  It doesn't seems right
> > > to bundle a selection of GPIO drivers along with CONFIG_SPL_DM.
> >
> > This issue has cropped up because this patch adds SPL support to
> mx7dsabresd, but intentionally does not enable SPL_DM.
> > The defconfig contains CONFIG_DM_74X164 for use in U-Boot Proper, but
> because drivers/gpio/Makefile also runs during the SPL portion of the build
> that config still pulls in the 74X164 driver which fails to build due to missing
> dependencies.
> >
> > Wrapping the DM objects in CONFIG_$(SPL_TPL_)DM ensures that DM is
> actually enabled for the current portion of the build when deciding to build
> the DM drivers.
> 
> But isn't it odd that not a single other driver is wrapped this way?
> What's special here?
> 
> I think the underlying flaw is the the kbuild system doesn't support the
> concept of building multiple times in different modes, e.g. main, SPL, TPL.
> And rather than design something, one gets layers of board specific hacks.

There are a few other instances of omitting DM drivers from SPL due to size concerns, see drivers/power/regulator/Makefile (commit a35332f) and drivers/mmc/Makefile (commit c4d660d)

However instead of wrapping the block in CONFIG_$(SPL_TPL_)DM, the intended way is to include $(SPL_) in line with the driver you need to disable for SPL.
- obj-$(CONFIG_DM_74X164)  += 74x164_gpio.o
+ obj-$(CONFIG_$(SPL_)DM_74X164)  += 74x164_gpio.o

I'll switch to this in-line method and split out this change into a separate patch so the commit message explains it in isolation.

Thanks,
Henry