[RFC/PATCH 1/4 PATCH] ARM: mvebu: use dt_fixup to provide fallback for enable-method

[arnd@arndb.de (Arnd Bergmann)]

On Monday 15 December 2014 00:57:49 Chris Packham wrote:
> On 12/15/2014 10:11 AM, Chris Packham wrote:
> >>> +
> >>> +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
> >>> \ No newline at end of file
> >>
> >> Why is this needed? Can't you just include <linux/libfdt.h> ?
> >>
> >
> > I am already including linux/libfdt.h. Without the CFLAGS change I get
> > the following compile error.
> >
> >    In file included from include/linux/libfdt.h:6:0,
> >                     from arch/arm/mach-mvebu/board-v7.c:21:
> >    include/linux/../../scripts/dtc/libfdt/libfdt.h:54:24: fatal error:
> > libfdt_env.h: No such file or directory
> >
> > There seems to be precedence in other architectures/drivers
> >
> >    $ git grep -e '-I.*libfdt'
> >    arch/mips/cavium-octeon/Makefile:CFLAGS_octeon-platform.o =
> > -I$(src)/../../../scripts/dtc/libfdt
> >    arch/mips/cavium-octeon/Makefile:CFLAGS_setup.o =
> > -I$(src)/../../../scripts/dtc/libfdt
> >    arch/mips/mti-sead3/Makefile:CFLAGS_sead3-setup.o =
> > -I$(src)/../../../scripts/dtc/libfdt
> >    arch/powerpc/kernel/Makefile:CFLAGS_prom.o              =
> > -I$(src)/../../../scripts/dtc/libfdt
> >    drivers/firmware/efi/libstub/Makefile:CFLAGS_fdt.o    +=
> > -I$(srctree)/scripts/dtc/libfdt/
> >    drivers/of/Makefile:CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
> >    drivers/of/Makefile:CFLAGS_fdt_address.o =
> > -I$(src)/../../scripts/dtc/libfdt
> >

I still think we should not duplicate this but instead change the
header files to make the include hack unnecessary in all those places.

Looking at the header file, it only contains

#include <linux/libfdt_env.h>
#include "../../scripts/dtc/libfdt/fdt.h"
#include "../../scripts/dtc/libfdt/libfdt.h"

so two of them get the headers from exactly that place, while the
libfdt_env.h file contains the linux-specific implementation of
a couple of functions instead of the baremetal implementation.

It looks like the intention was that when you already include the
linux version of the header, you no longer need the other one, but
that assumption is broken by current implementation.

One way to solve this would be

diff --git a/scripts/dtc/libfdt/libfdt.h b/scripts/dtc/libfdt/libfdt.h
index 73f49759a5e7..23c55baa8e18 100644
--- a/scripts/dtc/libfdt/libfdt.h
+++ b/scripts/dtc/libfdt/libfdt.h
@@ -51,7 +51,7 @@
  *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <libfdt_env.h>
+#include "libfdt_env.h"
 #include <fdt.h>
 
 #define FDT_FIRST_SUPPORTED_VERSION	0x10


but I'm not sure if that is the best way.

> >> I think it would be good to first check whether you are running on
> >> Armada XP
> >> or Armada 370, because the latter does not require this.
> >>
> >>     Arnd
> >>
> >
> > Any suggestion as to what to check for. Based on the dts files in the
> > current source seem compatible == "marvell,armada370" seems like a good
> > choice. But I don't know for sure that there isn't a armada-xp variant
> > out using a .dts with this set.
> 
> Adding the following at the top will do the trick (assuming
> "marvell,armada370" is the right compatible string to use).
> 
>         unsigned long root = of_get_flat_dt_root();
> 
>         if (of_flat_dt_is_compatible(root, "marvell,armada370"))
>                 return;

Right. You could also add 

	if (!IS_ENABLED(CONFIG_SMP) || !of_flat_dt_is_compatible(root, "marvell,armadaxp")))

to again eliminate that check for uniprocessor kernels, and to not
propagate the fixup to future machines but make it clear that
it is only needed for armadaxp.

	Arnd