Patchwork [U-Boot] fdt: Enhance dts/Makefile to be all things to all men

login
register
mail settings
Submitter Simon Glass
Date May 28, 2013, 7:36 p.m.
Message ID <1369769778-12455-1-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/246950/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Simon Glass - May 28, 2013, 7:36 p.m.
There are a few partially conflicting requirements in compiling the device
tree, since U-Boot relies on whatever is installed on the build machine.

Some versions of dtc support -i, and this is often better than using #include
since you get correct line number information in errors. Unfortunately this
version must be installed manually in current Linux distributions.

Some device tree files use the word 'linux' which gets replaced with '1' by
many version of gcc, including version 4.7. So undefine this.

When an device tree file has an error we want to direct the user to the
right file and line number. So instead of piping the file into dts through
stdin, put it in a real file so that we get a fairly sensible error message
from dts. Then print out the original file details to help further.

This is based on a commit from Tom Warren. It would help if people can
test it in different environments.

Signed-off-by: Tom Warren <twarren@nvidia.com>
Signed-off-by: Simon Glass <sjg@chromium.org>
---
 dts/Makefile | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)
Stephen Warren - May 28, 2013, 8:57 p.m.
On 05/28/2013 01:36 PM, Simon Glass wrote:
> There are a few partially conflicting requirements in compiling the device
> tree, since U-Boot relies on whatever is installed on the build machine.
> 
> Some versions of dtc support -i, and this is often better than using #include
> since you get correct line number information in errors.

What issue is there with line numbers when using dtc? Recent dtc
supports #line directives from the pre-processing results, and hence
reports errors in terms of the source files, just like raw dtc without cpp.

> Unfortunately this
> version must be installed manually in current Linux distributions.

Well, then that gets into the problem of some .dts files choosing to use
/include/ and rely on -i, but others using #include and not. I don't
really think it's a good idea to propagate both versions. Picking one
and using it would be far better.

I really do think we should simply require a reasonably recent dtc and
be done with it.

> Some device tree files use the word 'linux' which gets replaced with '1' by
> many version of gcc, including version 4.7. So undefine this.

Linux chose to use -undef rather than undefining specific/individual
defines. It'd be best to be consistent to that .dts files are more
likely to be portable between the two.

> diff --git a/dts/Makefile b/dts/Makefile

> +# Provide a list of include directories for dtc
> +DTS_INCS-y := -i $(SRCTREE)/arch/$(ARCH)/dts
> +
> +DTS_INCS-y += -i $(SRCTREE)/board/$(VENDOR)/dts

That has arch/ first then board/ ... (continued a few comments below)

> +DTS_INCS-$(CONFIG_CHROMEOS) += -i $(SRCTREE)/cros/dts

Is that meant to be upstream?

> +# Check if our dtc includes the -i option
> +DTS_FLAGS := $(shell if ! dtc -i 2>&1 | grep -q "invalid option"; then \
> +		echo $(DTS_INCS-y); fi)
> +
>  # We preprocess the device tree file provide a useful define
> -DTS_CPPFLAGS := -x assembler-with-cpp \
> +# Undefine 'linux' since it might be used in device tree files
> +DTS_CPPFLAGS := -x assembler-with-cpp -Ulinux \

I'll repeat my request to use -undef instead.

>  		-DARCH_CPU_DTS=\"$(SRCTREE)/arch/$(ARCH)/dts/$(CONFIG_ARCH_DEVICE_TREE).dtsi\" \
>  		-DBOARD_DTS=\"$(SRCTREE)/board/$(VENDOR)/$(BOARD)/dts/$(DEVICE_TREE).dts\" \
> -		-I$(SRCTREE)/board/$(VENDOR)/dts -I$(SRCTREE)/arch/$(ARCH)/dts
> +		-D__ASSEMBLY__ -I$(OBJTREE)/include -I$(SRCTREE)/include \
> +		-I$(OBJTREE)/include2 \

Do we really want include or include2 (what's that?) at all? The .dts
files really should be standalone, and not interact with the U-Boot
headers at all.

> +		-I$(SRCTREE)/board/$(VENDOR)/dts -I$(SRCTREE)/arch/$(ARCH)/dts \

... whereas this has board/ first then arch/. It'd be better to be
consistent.

> +		-include $(OBJTREE)/include/config.h
> +
> +DTS_TMP := $(OBJTREE)/include/generated/$(DEVICE_TREE).dts.in

Hmm. This really isn't a generated header file. Can this instead be
$(OBJTREE)/$(dir $@).$(notdir $@).dts or something like that?

> +DTS_SRC := board/$(VENDOR)/dts/$(DEVICE_TREE).dts
>  
>  all:	$(obj).depend $(LIB)
>  
> @@ -50,13 +68,19 @@ all:	$(obj).depend $(LIB)
>  # the filename.
>  DT_BIN	:= $(obj)dt.dtb
>  
> -$(DT_BIN): $(TOPDIR)/board/$(VENDOR)/dts/$(DEVICE_TREE).dts
> +DTC_CMD := $(DTC) -R 4 -p 0x1000 -O dtb -o ${DT_BIN} $(DTS_FLAGS) $(DTS_TMP)

It may be better to leave $(DTS_TMP) in the make script below, so it's
more obvious what file is being compiled; the re-direct to $(DTS_TMP) is
left in the $(CPP) invocation below, so it'd also be consistent with that.

> +$(DT_BIN): $(TOPDIR)/$(DTS_SRC)
>  	rc=$$( \
> -		cat $< | $(CPP) -P $(DTS_CPPFLAGS) - | \
> -		{ { $(DTC) -R 4 -p 0x1000 -O dtb -o ${DT_BIN} - 2>&1 ; \
> +		cat $< | $(CPP) -P $(DTS_CPPFLAGS) - > $(DTS_TMP); \
> +		{ { $(DTC_CMD)  2>&1 ; \
...

> +	if [ $$rc != 0 ]; then \
> +		echo "Source file is $(DTS_SRC)"; \
> +		echo "Compiler: $(DTC_CMD)"; \
> +	fi; \

Isn't that what make V=1 is for?
Wolfgang Denk - May 28, 2013, 9:08 p.m.
Dear Simon Glass,

In message <1369769778-12455-1-git-send-email-sjg@chromium.org> you wrote:
> 
> Some device tree files use the word 'linux' which gets replaced with '1' by
> many version of gcc, including version 4.7. So undefine this.

I think this is not a good way to address this issue.  The GCC
documentation (section "System-specific Predefined Macros" [1])
desribes how this should be handled.  The "correct" (TM) way to fix
this is by adding "-ansi" or any "-std" option that requests strict
conformance to the compiler/preprocessor command line.

[1] http://gcc.gnu.org/onlinedocs/cpp/System_002dspecific-Predefined-Macros.html#System_002dspecific-Predefined-Macros


Best regards,

Wolfgang Denk
Simon Glass - May 29, 2013, 3:59 p.m.
Hi Stephen,

On Tue, May 28, 2013 at 1:57 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/28/2013 01:36 PM, Simon Glass wrote:
>> There are a few partially conflicting requirements in compiling the device
>> tree, since U-Boot relies on whatever is installed on the build machine.
>>
>> Some versions of dtc support -i, and this is often better than using #include
>> since you get correct line number information in errors.
>
> What issue is there with line numbers when using dtc? Recent dtc
> supports #line directives from the pre-processing results, and hence
> reports errors in terms of the source files, just like raw dtc without cpp.

That's the issue. What does dtc v1.3 do?

>
>> Unfortunately this
>> version must be installed manually in current Linux distributions.
>
> Well, then that gets into the problem of some .dts files choosing to use
> /include/ and rely on -i, but others using #include and not. I don't
> really think it's a good idea to propagate both versions. Picking one
> and using it would be far better.
>
> I really do think we should simply require a reasonably recent dtc and
> be done with it.

I would be happy with that, but it can be an extra barrier to getting
U-Boot building. So do we need using #include at all if we are using
-i ?

>
>> Some device tree files use the word 'linux' which gets replaced with '1' by
>> many version of gcc, including version 4.7. So undefine this.
>
> Linux chose to use -undef rather than undefining specific/individual
> defines. It'd be best to be consistent to that .dts files are more
> likely to be portable between the two.

Seems a bit extreme, but OK. Are you worried that gcc defines other
things without the double underscore?

>
>> diff --git a/dts/Makefile b/dts/Makefile
>
>> +# Provide a list of include directories for dtc
>> +DTS_INCS-y := -i $(SRCTREE)/arch/$(ARCH)/dts
>> +
>> +DTS_INCS-y += -i $(SRCTREE)/board/$(VENDOR)/dts
>
> That has arch/ first then board/ ... (continued a few comments below)
>
>> +DTS_INCS-$(CONFIG_CHROMEOS) += -i $(SRCTREE)/cros/dts
>
> Is that meant to be upstream?
>
>> +# Check if our dtc includes the -i option
>> +DTS_FLAGS := $(shell if ! dtc -i 2>&1 | grep -q "invalid option"; then \
>> +             echo $(DTS_INCS-y); fi)
>> +
>>  # We preprocess the device tree file provide a useful define
>> -DTS_CPPFLAGS := -x assembler-with-cpp \
>> +# Undefine 'linux' since it might be used in device tree files
>> +DTS_CPPFLAGS := -x assembler-with-cpp -Ulinux \
>
> I'll repeat my request to use -undef instead.
>
>>               -DARCH_CPU_DTS=\"$(SRCTREE)/arch/$(ARCH)/dts/$(CONFIG_ARCH_DEVICE_TREE).dtsi\" \
>>               -DBOARD_DTS=\"$(SRCTREE)/board/$(VENDOR)/$(BOARD)/dts/$(DEVICE_TREE).dts\" \
>> -             -I$(SRCTREE)/board/$(VENDOR)/dts -I$(SRCTREE)/arch/$(ARCH)/dts
>> +             -D__ASSEMBLY__ -I$(OBJTREE)/include -I$(SRCTREE)/include \
>> +             -I$(OBJTREE)/include2 \
>
> Do we really want include or include2 (what's that?) at all? The .dts
> files really should be standalone, and not interact with the U-Boot
> headers at all.

I understood that you were wanting to make use of U-Boot defines. If
you want to include include/config.h then I think you need these.

>
>> +             -I$(SRCTREE)/board/$(VENDOR)/dts -I$(SRCTREE)/arch/$(ARCH)/dts \
>
> ... whereas this has board/ first then arch/. It'd be better to be
> consistent.

OK

>
>> +             -include $(OBJTREE)/include/config.h
>> +
>> +DTS_TMP := $(OBJTREE)/include/generated/$(DEVICE_TREE).dts.in
>
> Hmm. This really isn't a generated header file. Can this instead be
> $(OBJTREE)/$(dir $@).$(notdir $@).dts or something like that?

I didn't say header file.

The nice thing about having everything in include/generated is that it
doesn't pollute the source for in-tree builds.

>
>> +DTS_SRC := board/$(VENDOR)/dts/$(DEVICE_TREE).dts
>>
>>  all: $(obj).depend $(LIB)
>>
>> @@ -50,13 +68,19 @@ all:      $(obj).depend $(LIB)
>>  # the filename.
>>  DT_BIN       := $(obj)dt.dtb
>>
>> -$(DT_BIN): $(TOPDIR)/board/$(VENDOR)/dts/$(DEVICE_TREE).dts
>> +DTC_CMD := $(DTC) -R 4 -p 0x1000 -O dtb -o ${DT_BIN} $(DTS_FLAGS) $(DTS_TMP)
>
> It may be better to leave $(DTS_TMP) in the make script below, so it's
> more obvious what file is being compiled; the re-direct to $(DTS_TMP) is
> left in the $(CPP) invocation below, so it'd also be consistent with that.
>
>> +$(DT_BIN): $(TOPDIR)/$(DTS_SRC)
>>       rc=$$( \
>> -             cat $< | $(CPP) -P $(DTS_CPPFLAGS) - | \
>> -             { { $(DTC) -R 4 -p 0x1000 -O dtb -o ${DT_BIN} - 2>&1 ; \
>> +             cat $< | $(CPP) -P $(DTS_CPPFLAGS) - > $(DTS_TMP); \
>> +             { { $(DTC_CMD)  2>&1 ; \
> ...
>
>> +     if [ $$rc != 0 ]; then \
>> +             echo "Source file is $(DTS_SRC)"; \
>> +             echo "Compiler: $(DTC_CMD)"; \
>> +     fi; \
>
> Isn't that what make V=1 is for?

It produces about 800KB of other spiel though. If the build fails it
is already printing stuff out - so I find this useful.

>

Regards,
Simon
Simon Glass - May 29, 2013, 4 p.m.
Hi Wolfgang,

On Tue, May 28, 2013 at 2:08 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1369769778-12455-1-git-send-email-sjg@chromium.org> you wrote:
>>
>> Some device tree files use the word 'linux' which gets replaced with '1' by
>> many version of gcc, including version 4.7. So undefine this.
>
> I think this is not a good way to address this issue.  The GCC
> documentation (section "System-specific Predefined Macros" [1])
> desribes how this should be handled.  The "correct" (TM) way to fix
> this is by adding "-ansi" or any "-std" option that requests strict
> conformance to the compiler/preprocessor command line.
>
> [1] http://gcc.gnu.org/onlinedocs/cpp/System_002dspecific-Predefined-Macros.html#System_002dspecific-Predefined-Macros

Stephen suggested a slightly more extreme version too - I was worried
that all the typing stuff in U-Boot headers would break in this case,
but I didn't actually test it, so perhaps it is fine.

Regards,
Simon
Stephen Warren - May 29, 2013, 4:40 p.m.
On 05/29/2013 09:59 AM, Simon Glass wrote:
> Hi Stephen,
> 
> On Tue, May 28, 2013 at 1:57 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 05/28/2013 01:36 PM, Simon Glass wrote:
>>> There are a few partially conflicting requirements in compiling the device
>>> tree, since U-Boot relies on whatever is installed on the build machine.
>>>
>>> Some versions of dtc support -i, and this is often better than using #include
>>> since you get correct line number information in errors.
>>
>> What issue is there with line numbers when using dtc? Recent dtc
>> supports #line directives from the pre-processing results, and hence
>> reports errors in terms of the source files, just like raw dtc without cpp.
> 
> That's the issue. What does dtc v1.3 do?

v1.3 doesn't include the feature, but it also doesn't support -i either.

>>> Unfortunately this
>>> version must be installed manually in current Linux distributions.
>>
>> Well, then that gets into the problem of some .dts files choosing to use
>> /include/ and rely on -i, but others using #include and not. I don't
>> really think it's a good idea to propagate both versions. Picking one
>> and using it would be far better.
>>
>> I really do think we should simply require a reasonably recent dtc and
>> be done with it.
> 
> I would be happy with that, but it can be an extra barrier to getting
> U-Boot building.

The Linux kernel chose to solve this by bundling the required dtc source
inside the kernel source tree as a tool. This seems by far the simplest
way to solve the problem for U-Boot too. If not, it's not exactly hard to:

git clone
make

... given that one is already building U-Boot from source anyway.


> So do we need using #include at all if we are using -i ?

Well, *.dts are moving to #include (and other cpp constructs) rather
than /include/ in the copies in the Linux kernel, which I think will
eventually make their way into U-Boot for consistency. Equally, if/when
*.dts get separated out into a separate project from the kernel, I think
we'd want the same to happen for U-Boot so that the same *.dts is used.
Then, there won't be a choice.

>>> Some device tree files use the word 'linux' which gets replaced with '1' by
>>> many version of gcc, including version 4.7. So undefine this.
>>
>> Linux chose to use -undef rather than undefining specific/individual
>> defines. It'd be best to be consistent to that .dts files are more
>> likely to be portable between the two.
> 
> Seems a bit extreme, but OK. Are you worried that gcc defines other
> things without the double underscore?

IIRC there were some other issues, yes. "unix" may have been one of
them. The problem was reported (and I think that fix suggested) by
someone else, so I don't remember the full details, although they're in
the mailing list archives I suppose.

>>> diff --git a/dts/Makefile b/dts/Makefile

>>>               -DARCH_CPU_DTS=\"$(SRCTREE)/arch/$(ARCH)/dts/$(CONFIG_ARCH_DEVICE_TREE).dtsi\" \
>>>               -DBOARD_DTS=\"$(SRCTREE)/board/$(VENDOR)/$(BOARD)/dts/$(DEVICE_TREE).dts\" \
>>> -             -I$(SRCTREE)/board/$(VENDOR)/dts -I$(SRCTREE)/arch/$(ARCH)/dts
>>> +             -D__ASSEMBLY__ -I$(OBJTREE)/include -I$(SRCTREE)/include \
>>> +             -I$(OBJTREE)/include2 \
>>
>> Do we really want include or include2 (what's that?) at all? The .dts
>> files really should be standalone, and not interact with the U-Boot
>> headers at all.
> 
> I understood that you were wanting to make use of U-Boot defines. If
> you want to include include/config.h then I think you need these.

I hope I didn't want to:-) The DT should really represent the HW not
anything about the way U-Boot is configured.

>>> +             -include $(OBJTREE)/include/config.h
>>> +
>>> +DTS_TMP := $(OBJTREE)/include/generated/$(DEVICE_TREE).dts.in
>>
>> Hmm. This really isn't a generated header file. Can this instead be
>> $(OBJTREE)/$(dir $@).$(notdir $@).dts or something like that?
> 
> I didn't say header file.
> 
> The nice thing about having everything in include/generated is that it
> doesn't pollute the source for in-tree builds.

Well, it's in a directory that's for generated headers; it has
"/include/" in the path. I don't think we should put it somewhere that C
code could accidentally #include it, irrespective of how (un-)likely
that is to get passed review. Also, for in-tree builds, doesn't every
single other derived file get put into the source tree? I'm not sure why
this file would need to be special?

>>> +$(DT_BIN): $(TOPDIR)/$(DTS_SRC)
>>>       rc=$$( \
>>> -             cat $< | $(CPP) -P $(DTS_CPPFLAGS) - | \
>>> -             { { $(DTC) -R 4 -p 0x1000 -O dtb -o ${DT_BIN} - 2>&1 ; \
>>> +             cat $< | $(CPP) -P $(DTS_CPPFLAGS) - > $(DTS_TMP); \
>>> +             { { $(DTC_CMD)  2>&1 ; \
>> ...
>>
>>> +     if [ $$rc != 0 ]; then \
>>> +             echo "Source file is $(DTS_SRC)"; \
>>> +             echo "Compiler: $(DTC_CMD)"; \
>>> +     fi; \
>>
>> Isn't that what make V=1 is for?
> 
> It produces about 800KB of other spiel though. If the build fails it
> is already printing stuff out - so I find this useful.

But again, why be special? I could apply the same argument to every
single other C file where I might have typo'd something.
Stephen Warren - May 29, 2013, 5:02 p.m.
On 05/28/2013 03:08 PM, Wolfgang Denk wrote:
> Dear Simon Glass,
> 
> In message <1369769778-12455-1-git-send-email-sjg@chromium.org> you wrote:
>>
>> Some device tree files use the word 'linux' which gets replaced with '1' by
>> many version of gcc, including version 4.7. So undefine this.
> 
> I think this is not a good way to address this issue.  The GCC
> documentation (section "System-specific Predefined Macros" [1])
> desribes how this should be handled.  The "correct" (TM) way to fix
> this is by adding "-ansi" or any "-std" option that requests strict
> conformance to the compiler/preprocessor command line.
> 
> [1] http://gcc.gnu.org/onlinedocs/cpp/System_002dspecific-Predefined-Macros.html#System_002dspecific-Predefined-Macros

-ansi at least was considered when the Linux kernel patches for dtc+cpp
support were being developed, but it was rejected. While it possibly
does solve this specific issue fine, there were other more general
problems; IIRC (and I might not) it completely changes the way macro
expansion happens, which results in it being pretty useless. Hence, "-x
assembler-with-cpp" was chosen over e.g. "-ansi".
Wolfgang Denk - May 29, 2013, 9:02 p.m.
Dear Simon,

In message <CAPnjgZ2a+qrsPWTz5Y=48m_LCRqAikY0-seJudW8AY5asdwmxw@mail.gmail.com> you wrote:
> 
> > I think this is not a good way to address this issue.  The GCC
> > documentation (section "System-specific Predefined Macros" [1])
> > desribes how this should be handled.  The "correct" (TM) way to fix
> > this is by adding "-ansi" or any "-std" option that requests strict
> > conformance to the compiler/preprocessor command line.
> >
> > [1] http://gcc.gnu.org/onlinedocs/cpp/System_002dspecific-Predefined-Macros.html#System_002dspecific-Predefined-Macros
> 
> Stephen suggested a slightly more extreme version too - I was worried
> that all the typing stuff in U-Boot headers would break in this case,
> but I didn't actually test it, so perhaps it is fine.

Yes, I've seen it.  I think either approach is better that manually
undef-ing specific variables.  From my personal point of view I think
the -undef approach is a bit of overkill, but I would not protest.

Best regards,

Wolfgang Denk
Wolfgang Denk - May 29, 2013, 9:31 p.m.
Dear Stephen,

In message <51A62F8D.9010208@wwwdotorg.org> you wrote:
>
> The Linux kernel chose to solve this by bundling the required dtc source
> inside the kernel source tree as a tool. This seems by far the simplest
> way to solve the problem for U-Boot too. If not, it's not exactly hard to:

Actually it's a horrible approach to fixing tool issues upstream.
Or rather to NOT fixing issues.  Instead of pushing forward that
distros distribute useful, recent versions we simply copy the dtc
source.  Tomorrow we include the source tree for "make", and next week
for "binutils" and "gcc".  And in a few months we build a full distro.

It's the same with the Kconfig approach to print only short compile
messages - instead of fixing this once where it belongs (in "make"), a
zillion projects copy the pretty expensive code around, because it is
so much easier.

It's frustrating to watch how good old methods that brought free
software to the state we have today (or we had some time ago already)
more and more get lost and forgotten :-(

Best regards,

Wolfgang Denk
Wolfgang Denk - May 29, 2013, 9:33 p.m.
Dear Stephen,

In message <51A634B5.5060309@wwwdotorg.org> you wrote:
>
> > I think this is not a good way to address this issue.  The GCC
> > documentation (section "System-specific Predefined Macros" [1])
> > desribes how this should be handled.  The "correct" (TM) way to fix
> > this is by adding "-ansi" or any "-std" option that requests strict
> > conformance to the compiler/preprocessor command line.
> > 
> > [1] http://gcc.gnu.org/onlinedocs/cpp/System_002dspecific-Predefined-Macros.html#System_002dspecific-Predefined-Macros
> 
> -ansi at least was considered when the Linux kernel patches for dtc+cpp
> support were being developed, but it was rejected. While it possibly

Can you provide references?  I'd like to understand why it was
rejected - it seems to be the "official" approach to the problem.

> does solve this specific issue fine, there were other more general
> problems; IIRC (and I might not) it completely changes the way macro
> expansion happens, which results in it being pretty useless. Hence, "-x
> assembler-with-cpp" was chosen over e.g. "-ansi".

Again, do you have any reference?  "completely changes the way macro
expansion happens" sounds terribly dangerous, so it would be better to
know about that exactly...

Best regards,

Wolfgang Denk
Stephen Warren - May 29, 2013, 10:18 p.m.
On 05/29/2013 03:31 PM, Wolfgang Denk wrote:
> Dear Stephen,
> 
> In message <51A62F8D.9010208@wwwdotorg.org> you wrote:
>>
>> The Linux kernel chose to solve this by bundling the required dtc source
>> inside the kernel source tree as a tool. This seems by far the simplest
>> way to solve the problem for U-Boot too. If not, it's not exactly hard to:
> 
> Actually it's a horrible approach to fixing tool issues upstream.
> Or rather to NOT fixing issues.  Instead of pushing forward that
> distros distribute useful, recent versions we simply copy the dtc
> source.

I don't understand the hangup about the version of dtc that distros package.

Sure, it'd be nice if distros updated the the (currently) latest version
of dtc and packaged that, so that at some time the desired version was
there, and everything "just worked".

However, that's not going to outright solve the problem for a /long/ time.

What if someone wants to build U-Boot on Ubuntu 10.04 or RHEL 5. It
seems quite reasonable for someone to be using those for development
since they're long-term supported stable releases. Those releases don't
have the (current) latest version of dtc packaged, and it's exceedingly
unlikely anyone could push an update into those distros to update dtc,
since they're probably in bug-fix-only mode right now, and a dtc update
would be to add new features.

So, to cover that issue we must:

a) Get the latest dtc into distros right now. Wait until everyone has
updated. Then, we can use the new features. This could take many many years.

b) Simply require people to install dtc from source, if their distro
doesn't already package the desired version. This will immediately solve
the problem, and is honestly quite simple if you're already building
other things (U-Boot) from source.

I prefer option (b) here. And given that, I assert that whatever version
distros package is largely irrelevant, since there's a trivial
workaround if they don't have the desired version.

Longer-term distros will pick up a new version, and remove the need for
build-from-source, thus streamlining the process.

To keep this process in check a bit, we could always pick a specific git
commit or release version of dtc that each U-Boot version (release) will
be allowed to assume. That will limit the number of times people need to
update their locally-built dtc to at most once per U-Boot release.
Hopefully much less often.
Wolfgang Denk - May 29, 2013, 10:36 p.m.
Dear Stephen Warren,

In message <51A67EC1.2000208@wwwdotorg.org> you wrote:
>
> To keep this process in check a bit, we could always pick a specific git
> commit or release version of dtc that each U-Boot version (release) will
> be allowed to assume. That will limit the number of times people need to
> update their locally-built dtc to at most once per U-Boot release.
> Hopefully much less often.

I think this is broken by design, in several aspects.  First, U-Boot
is usually not the only user of DTC.  Second, assume you run a "git
bisect" over a U-Boot tree - would you really want to switch DTC in-
flight?

Sorry, instead we should strive to be compatible to a reasonably old,
stable version of DTC, like we do for all other tools as well.  As
mentioned before - just because RHEL 5 ships an ancient version of -
say - "make" we will NOT start building this from the sources ourself.
This cannot be the way to go.

Best regards,

Wolfgang Denk
Stephen Warren - May 29, 2013, 10:52 p.m.
On 05/29/2013 03:33 PM, Wolfgang Denk wrote:
> Dear Stephen,
> 
> In message <51A634B5.5060309@wwwdotorg.org> you wrote:
>>
>>> I think this is not a good way to address this issue.  The GCC
>>> documentation (section "System-specific Predefined Macros" [1])
>>> desribes how this should be handled.  The "correct" (TM) way to fix
>>> this is by adding "-ansi" or any "-std" option that requests strict
>>> conformance to the compiler/preprocessor command line.
>>>
>>> [1] http://gcc.gnu.org/onlinedocs/cpp/System_002dspecific-Predefined-Macros.html#System_002dspecific-Predefined-Macros
>>
>> -ansi at least was considered when the Linux kernel patches for dtc+cpp
>> support were being developed, but it was rejected. While it possibly
> 
> Can you provide references?  I'd like to understand why it was
> rejected - it seems to be the "official" approach to the problem.
> 
>> does solve this specific issue fine, there were other more general
>> problems; IIRC (and I might not) it completely changes the way macro
>> expansion happens, which results in it being pretty useless. Hence, "-x
>> assembler-with-cpp" was chosen over e.g. "-ansi".
> 
> Again, do you have any reference?  "completely changes the way macro
> expansion happens" sounds terribly dangerous, so it would be better to
> know about that exactly...

Sorry, I was thinking about -traditional-cpp, not -ansi. We had
considered -traditional-cpp as a workaround because DT uses property
names that start with #, and this triggered cpp to treat them as macro
names, which went wrong. However, due to -traditional-cpp being quite
different from ISO cpp, we selected -x assembler-with-cpp instead, which
both implements an ISO cpp, but also only handles pre-processing
directives with the # in column 1.

Now, let's discuss -ansi vs. -undef some more.

Since DT is supposed to be a HW description, it shouldn't be using cpp's
built-in macros to compile in different ways; there really isn't a
concept of the "target arch of compilation"; a .dts file should simply
compile the same way everywhere. Different DTs represent different HW;
we don't use a single DT with conditional compilation to represent
different HW. This led Rob Herring (one of the kernel device tree
maintainers) to propose the following rules for cpp usage on device trees:

https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-October/020831.html

One of which was:

- No gcc built-in define usage

I don't see any disagreement with that bullet point in the thread, and
indeed it makes sense to me for the reasons I outlined above.

Some history on why we needed to not-define, or un-define, some macros
(such as the linux macro we're discussing here) can be found in:

http://linux-kernel.2935.n7.nabble.com/PATCH-V7-kbuild-create-a-rule-to-run-the-pre-processor-on-dts-files-td575632.html

The last few messages there end up mentioning -undef, which we/I ended
up using. It looks like -ansi wasn't mentioned at all when discussing
this issue.

However, I assert that -undef is better, since we end up without any
pre-defined macros, which DT files shouldn't be using anyway. By my
reading of the cpp manual link you send, -ansi simply stops
non-conforming pre-defined macros from being defined, but doesn't stop
them all being defined as -undef does. Hence, I prefer -undef.

Oh, and another of Rob's proposed rules:

- No kernel kconfig option usage

Would also imply that U-Boots config headers shouldn't be accessible
when compiling device trees. The last few kernel patches I sent to
enable dtc+cpp usage were specifically aimed at limiting the set of
headers that DT files can use to those specifically part of the DT
bindings, and not general Linux headers, For example, see the following
Linux kernel commit:

==========
kbuild: create an "include chroot" for DT bindings

The recent dtc+cpp support allows header files and C pre-processor
defines/macros to be used when compiling device tree files. These
headers will typically define various constants that are part of the
device tree bindings.

The original patch which set up the dtc+cpp include path only considered
using those headers from device tree files. However, most are also
useful for kernel code which needs to interpret the device tree.

In both the DT files and the kernel, I'd like to include the DT-related
headers in the same way, for example, <dt-bindings/gpio/tegra-gpio.h>.
That will simplify any text which discusses the DT header locations.

Creating a <dt-bindings/> for kernel source to use is as simple as
placing files into include/dt-bindings/.

However, when compiling DT files, the include path should be restricted
so that only the dt-bindings path is available; arbitrary kernel headers
shouldn't be exposed. For this reason, create a specific include
directory for use by dtc+cpp, and symlink dt-bindings from there to the
actual location of include/dt-bindings/. For want of a better location,
place this "include chroot" into the existing dts/ directory.

arch/*/boot/dts/include/dt-bindings -> ../../../../../include/dt-bindings

Some headers used by device tree files may not be useful to the kernel;
they may be used simply to aid in constructing the DT file (e.g. macros
to create a node), but not define any information that the kernel needs
to share. These may be placed directly into arch/*/boot/dts/ along with
the DT files themselves.
==========
Stephen Warren - May 29, 2013, 11:07 p.m.
On 05/29/2013 04:36 PM, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <51A67EC1.2000208@wwwdotorg.org> you wrote:
>>
>> To keep this process in check a bit, we could always pick a specific git
>> commit or release version of dtc that each U-Boot version (release) will
>> be allowed to assume. That will limit the number of times people need to
>> update their locally-built dtc to at most once per U-Boot release.
>> Hopefully much less often.
> 
> I think this is broken by design, in several aspects.  First, U-Boot
> is usually not the only user of DTC.  Second, assume you run a "git
> bisect" over a U-Boot tree - would you really want to switch DTC in-
> flight?
> 
> Sorry, instead we should strive to be compatible to a reasonably old,
> stable version of DTC, like we do for all other tools as well.  As
> mentioned before - just because RHEL 5 ships an ancient version of -
> say - "make" we will NOT start building this from the sources ourself.
> This cannot be the way to go.

So the result of that is that we can never ever use new features in any
tool, at least in any meaningful time-frame.

I think we need to differentiate between tools that are already stable
and/or core to all aspects of the U-Boot build process (such as make),
and tools that enable new features that are under development.

Clearly the U-Boot makefiles have to be fairly cautious in their use of
any new make features, so that everyone with any reasonable version of
make can build U-Boot.

However, when enabling a new feature, such as using device trees to
configure U-Boot[1], for which tool support is new and evolving along
with the feature itself, and which is only used on a very very few
boards and even fewer SoCs right now within U-Boot, it seems entirely
reasonable to demand that the people working on/with that new feature
are aware that it's evolving, and that they may need to take a few extra
steps to go out and get tools that support that new feature. No doubt
once this feature has settled down a bit, and distros have pulled in
newer versions of dtc, everthing will "just work" just like any other
stable feature.

If you don't accept this, then we simply have to ban any include use in
U-Boot; dtc -i isn't in distro-packaged versions of dtc, so we'd need to
stop using that. We'd need to move *.dts into a single directory within
U-Boot to work around that, or perhaps hard-coding relative include
paths in *.dts might work. Similarly for the patches to support dtc+cpp
usage, so we wouldn't be able to use named constants in DT files for
many years, which would prevent sharing DT files with the kernel and/or
any other standard repository of DT files, which are bound to use this
feature.

[1] Which is very specifically a different feature than simply having
U-Boot pass a DT to the Linux kernel during boot, and perhaps modify it
a little, which clearly is not a new feature.
Simon Glass - May 30, 2013, 4:46 a.m.
Hi,

On Wed, May 29, 2013 at 4:07 PM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> On 05/29/2013 04:36 PM, Wolfgang Denk wrote:
> > Dear Stephen Warren,
> >
> > In message <51A67EC1.2000208@wwwdotorg.org> you wrote:
> >>
> >> To keep this process in check a bit, we could always pick a specific git
> >> commit or release version of dtc that each U-Boot version (release) will
> >> be allowed to assume. That will limit the number of times people need to
> >> update their locally-built dtc to at most once per U-Boot release.
> >> Hopefully much less often.
> >
> > I think this is broken by design, in several aspects.  First, U-Boot
> > is usually not the only user of DTC.  Second, assume you run a "git
> > bisect" over a U-Boot tree - would you really want to switch DTC in-
> > flight?
> >
> > Sorry, instead we should strive to be compatible to a reasonably old,
> > stable version of DTC, like we do for all other tools as well.  As
> > mentioned before - just because RHEL 5 ships an ancient version of -
> > say - "make" we will NOT start building this from the sources ourself.
> > This cannot be the way to go.
>
> So the result of that is that we can never ever use new features in any
> tool, at least in any meaningful time-frame.
>
> I think we need to differentiate between tools that are already stable
> and/or core to all aspects of the U-Boot build process (such as make),
> and tools that enable new features that are under development.
>
> Clearly the U-Boot makefiles have to be fairly cautious in their use of
> any new make features, so that everyone with any reasonable version of
> make can build U-Boot.
>
> However, when enabling a new feature, such as using device trees to
> configure U-Boot[1], for which tool support is new and evolving along
> with the feature itself, and which is only used on a very very few
> boards and even fewer SoCs right now within U-Boot, it seems entirely
> reasonable to demand that the people working on/with that new feature
> are aware that it's evolving, and that they may need to take a few extra
> steps to go out and get tools that support that new feature. No doubt
> once this feature has settled down a bit, and distros have pulled in
> newer versions of dtc, everthing will "just work" just like any other
> stable feature.
>
> If you don't accept this, then we simply have to ban any include use in
> U-Boot; dtc -i isn't in distro-packaged versions of dtc, so we'd need to
> stop using that. We'd need to move *.dts into a single directory within
> U-Boot to work around that, or perhaps hard-coding relative include
> paths in *.dts might work. Similarly for the patches to support dtc+cpp
> usage, so we wouldn't be able to use named constants in DT files for
> many years, which would prevent sharing DT files with the kernel and/or
> any other standard repository of DT files, which are bound to use this
> feature.
>
> [1] Which is very specifically a different feature than simply having
> U-Boot pass a DT to the Linux kernel during boot, and perhaps modify it
> a little, which clearly is not a new feature.
>

My patch:

- doesn't require -i but uses it if available (ARCH_CPU_DTS works around
this)
- honours #define, #include, etc.
- works with old and new versions of dtc
- uses -Ulinux which we are thinking might be better done another way

Let's not throw the baby out with the bathwater. I have no problem with
updating my dtc as needed, but it would certainly be nice if U-Boot didn't
require this.

However, let's say Tegra needs a newer dtc than 1.3. I wonder whether we
should just add a setting to tell U-Boot to not build the device tree at
all? The same U-Boot binary can run on many different board times, just
needing a different device tree. Rather than pick one, we could just pick
none. Then if you want to use new features in dtc, just define
CONFIG_OF_NO_BUILD, or similar, and you can deal with the device tree
details yourself. MAKEALL/buildman will be happy with this.

Otherwise for now I think my patch is a reasonable compromise in terms of
features.

Regards,
Simon
Stephen Warren - May 30, 2013, 5:11 a.m.
On 05/29/2013 10:46 PM, Simon Glass wrote:
> Hi,
> 
> On Wed, May 29, 2013 at 4:07 PM, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
> 
>     On 05/29/2013 04:36 PM, Wolfgang Denk wrote:
>     > Dear Stephen Warren,
>     >
>     > In message <51A67EC1.2000208@wwwdotorg.org
>     <mailto:51A67EC1.2000208@wwwdotorg.org>> you wrote:
>     >>
>     >> To keep this process in check a bit, we could always pick a
>     specific git
>     >> commit or release version of dtc that each U-Boot version
>     (release) will
>     >> be allowed to assume. That will limit the number of times people
>     need to
>     >> update their locally-built dtc to at most once per U-Boot release.
>     >> Hopefully much less often.
>     >
>     > I think this is broken by design, in several aspects.  First, U-Boot
>     > is usually not the only user of DTC.  Second, assume you run a "git
>     > bisect" over a U-Boot tree - would you really want to switch DTC in-
>     > flight?
>     >
>     > Sorry, instead we should strive to be compatible to a reasonably old,
>     > stable version of DTC, like we do for all other tools as well.  As
>     > mentioned before - just because RHEL 5 ships an ancient version of -
>     > say - "make" we will NOT start building this from the sources ourself.
>     > This cannot be the way to go.
> 
>     So the result of that is that we can never ever use new features in any
>     tool, at least in any meaningful time-frame.
> 
>     I think we need to differentiate between tools that are already stable
>     and/or core to all aspects of the U-Boot build process (such as make),
>     and tools that enable new features that are under development.
> 
>     Clearly the U-Boot makefiles have to be fairly cautious in their use of
>     any new make features, so that everyone with any reasonable version of
>     make can build U-Boot.
> 
>     However, when enabling a new feature, such as using device trees to
>     configure U-Boot[1], for which tool support is new and evolving along
>     with the feature itself, and which is only used on a very very few
>     boards and even fewer SoCs right now within U-Boot, it seems entirely
>     reasonable to demand that the people working on/with that new feature
>     are aware that it's evolving, and that they may need to take a few extra
>     steps to go out and get tools that support that new feature. No doubt
>     once this feature has settled down a bit, and distros have pulled in
>     newer versions of dtc, everthing will "just work" just like any other
>     stable feature.
> 
>     If you don't accept this, then we simply have to ban any include use in
>     U-Boot; dtc -i isn't in distro-packaged versions of dtc, so we'd need to
>     stop using that. We'd need to move *.dts into a single directory within
>     U-Boot to work around that, or perhaps hard-coding relative include
>     paths in *.dts might work. Similarly for the patches to support dtc+cpp
>     usage, so we wouldn't be able to use named constants in DT files for
>     many years, which would prevent sharing DT files with the kernel and/or
>     any other standard repository of DT files, which are bound to use this
>     feature.
> 
>     [1] Which is very specifically a different feature than simply having
>     U-Boot pass a DT to the Linux kernel during boot, and perhaps modify it
>     a little, which clearly is not a new feature.
> 
> 
> My patch:
> 
> - doesn't require -i but uses it if available (ARCH_CPU_DTS works around
> this)

If we ever need to support a dtc that doesn't implement -i, then we
always need to support that. So, there's no point using -i when it's
available; we should simply have a single way of writing the *.dts files
that doesn't ever assume dtc -i is available. Otherwise, there will be
rampant inconsistency between different *.dts files; how they're
written, the flow by which they're compiled, etc.

In other words, we /always/ have to use "ARCH_CPU_DTS" in *.dts
throughout the entire U-Boot source tree if we're to support not using
dtc -i.

> - honours #define, #include, etc.
> - works with old and new versions of dtc

If we are forced to rely only upon features in old versions of dtc, we
specifically shouldn't allow use of features that will only work with
newer dtc. If we don't actively enforce this rule, we haven't achieved
the goal of not relying upon new versions of dtc.

...
> However, let's say Tegra needs a newer dtc than 1.3. I wonder whether we

This shouldn't be about whether a specific Soc's DT requires some
feature or not. All the U-Boot *.dts should work the same way.

> should just add a setting to tell U-Boot to not build the device tree at
> all? The same U-Boot binary can run on many different board times, just
> needing a different device tree.

Unfortunately, that's not remotely true yet for any Tegra system at
least; there's still a bunch of C code specific to each board. The
amount is reducing as things get migrated to DT, but we certainly aren't
there yet.

> Rather than pick one, we could just
> pick none. Then if you want to use new features in dtc, just define
> CONFIG_OF_NO_BUILD, or similar, and you can deal with the device tree
> details yourself. MAKEALL/buildman will be happy with this.

So you're saying: move the *.dts files out of U-Boot into some separate
project. It'd be nice to do that; hopefully that separate project could
be the one where the kernel's *.dts files move to. The big issue here is
that the DT bindings U-Boot uses are often different to the standard
bindings, so U-Boot currently contains U-Boot-specific *.dts so there
wouldn't be much point moving them out of U-Boot into some common DT
repository. Again, hopefully that's changing over time, but we're still
not there yet.

> Otherwise for now I think my patch is a reasonable compromise in terms
> of features.

It seems to be aimed specifically at enabling use of new dtc features
when present. That seems to be specifically against Wolfgang's goal of
not requiring new dtc features. There's no point allowing use of new dtc
features if we can't require them, since there's no fallback when they
aren't there to use.
Simon Glass - May 30, 2013, 5:33 a.m.
Hi Stephen,

On Wed, May 29, 2013 at 10:11 PM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> On 05/29/2013 10:46 PM, Simon Glass wrote:
> > Hi,
> >
> > On Wed, May 29, 2013 at 4:07 PM, Stephen Warren <swarren@wwwdotorg.org
> > <mailto:swarren@wwwdotorg.org>> wrote:
> >
> >     On 05/29/2013 04:36 PM, Wolfgang Denk wrote:
> >     > Dear Stephen Warren,
> >     >
> >     > In message <51A67EC1.2000208@wwwdotorg.org
> >     <mailto:51A67EC1.2000208@wwwdotorg.org>> you wrote:
> >     >>
> >     >> To keep this process in check a bit, we could always pick a
> >     specific git
> >     >> commit or release version of dtc that each U-Boot version
> >     (release) will
> >     >> be allowed to assume. That will limit the number of times people
> >     need to
> >     >> update their locally-built dtc to at most once per U-Boot release.
> >     >> Hopefully much less often.
> >     >
> >     > I think this is broken by design, in several aspects.  First,
> U-Boot
> >     > is usually not the only user of DTC.  Second, assume you run a "git
> >     > bisect" over a U-Boot tree - would you really want to switch DTC
> in-
> >     > flight?
> >     >
> >     > Sorry, instead we should strive to be compatible to a reasonably
> old,
> >     > stable version of DTC, like we do for all other tools as well.  As
> >     > mentioned before - just because RHEL 5 ships an ancient version of
> -
> >     > say - "make" we will NOT start building this from the sources
> ourself.
> >     > This cannot be the way to go.
> >
> >     So the result of that is that we can never ever use new features in
> any
> >     tool, at least in any meaningful time-frame.
> >
> >     I think we need to differentiate between tools that are already
> stable
> >     and/or core to all aspects of the U-Boot build process (such as
> make),
> >     and tools that enable new features that are under development.
> >
> >     Clearly the U-Boot makefiles have to be fairly cautious in their use
> of
> >     any new make features, so that everyone with any reasonable version
> of
> >     make can build U-Boot.
> >
> >     However, when enabling a new feature, such as using device trees to
> >     configure U-Boot[1], for which tool support is new and evolving along
> >     with the feature itself, and which is only used on a very very few
> >     boards and even fewer SoCs right now within U-Boot, it seems entirely
> >     reasonable to demand that the people working on/with that new feature
> >     are aware that it's evolving, and that they may need to take a few
> extra
> >     steps to go out and get tools that support that new feature. No doubt
> >     once this feature has settled down a bit, and distros have pulled in
> >     newer versions of dtc, everthing will "just work" just like any other
> >     stable feature.
> >
> >     If you don't accept this, then we simply have to ban any include use
> in
> >     U-Boot; dtc -i isn't in distro-packaged versions of dtc, so we'd
> need to
> >     stop using that. We'd need to move *.dts into a single directory
> within
> >     U-Boot to work around that, or perhaps hard-coding relative include
> >     paths in *.dts might work. Similarly for the patches to support
> dtc+cpp
> >     usage, so we wouldn't be able to use named constants in DT files for
> >     many years, which would prevent sharing DT files with the kernel
> and/or
> >     any other standard repository of DT files, which are bound to use
> this
> >     feature.
> >
> >     [1] Which is very specifically a different feature than simply having
> >     U-Boot pass a DT to the Linux kernel during boot, and perhaps modify
> it
> >     a little, which clearly is not a new feature.
> >
> >
> > My patch:
> >
> > - doesn't require -i but uses it if available (ARCH_CPU_DTS works around
> > this)
>

I had to remove all sharp objects from the room half-way through reading
your email :-) Gosh, things aren't that bad!


>
> If we ever need to support a dtc that doesn't implement -i, then we
> always need to support that. So, there's no point using -i when it's
> available; we should simply have a single way of writing the *.dts files
> that doesn't ever assume dtc -i is available. Otherwise, there will be
> rampant inconsistency between different *.dts files; how they're
> written, the flow by which they're compiled, etc.
>
> In other words, we /always/ have to use "ARCH_CPU_DTS" in *.dts
> throughout the entire U-Boot source tree if we're to support not using
> dtc -i.
>

Well realistically at some point there will be a new dtc release, and
perhaps a year or so after that we can maybe start deprecating this and
requiring -i.


>
> > - honours #define, #include, etc.
> > - works with old and new versions of dtc
>
> If we are forced to rely only upon features in old versions of dtc, we
> specifically shouldn't allow use of features that will only work with
> newer dtc. If we don't actively enforce this rule, we haven't achieved
> the goal of not relying upon new versions of dtc.
>

As you know I'm uncomfortable with the idea of using CPP to do things that
I feel dtc should do for itself, but yes if dtc does not grow these
features, we have little choice. But as an example, both #include and
symbols can be syntactically very similar whether CPP is used or not.


>
> ...
> > However, let's say Tegra needs a newer dtc than 1.3. I wonder whether we
>
> This shouldn't be about whether a specific Soc's DT requires some
> feature or not. All the U-Boot *.dts should work the same way.
>
> > should just add a setting to tell U-Boot to not build the device tree at
> > all? The same U-Boot binary can run on many different board times, just
> > needing a different device tree.
>
> Unfortunately, that's not remotely true yet for any Tegra system at
> least; there's still a bunch of C code specific to each board. The
> amount is reducing as things get migrated to DT, but we certainly aren't
> there yet.
>

OK, but that wasn't quite my point....

>
> > Rather than pick one, we could just
> > pick none. Then if you want to use new features in dtc, just define
> > CONFIG_OF_NO_BUILD, or similar, and you can deal with the device tree
> > details yourself. MAKEALL/buildman will be happy with this.
>
> So you're saying: move the *.dts files out of U-Boot into some separate
> project. It'd be nice to do that; hopefully that separate project could
> be the one where the kernel's *.dts files move to. The big issue here is
> that the DT bindings U-Boot uses are often different to the standard
> bindings, so U-Boot currently contains U-Boot-specific *.dts so there
> wouldn't be much point moving them out of U-Boot into some common DT
> repository. Again, hopefully that's changing over time, but we're still
> not there yet.
>

a. I didn't say move them out of U-Boot
b. If there are differences between the U-Boot and kernel bindings, we
should fix that
c. My point was that 'building nothing' fudges the 'failed build' problem
and might be an acceptable compromise. Then Tegra can use the whiz-bang new
features without breaking the build.


>
> > Otherwise for now I think my patch is a reasonable compromise in terms
> > of features.
>
> It seems to be aimed specifically at enabling use of new dtc features
> when present. That seems to be specifically against Wolfgang's goal of
> not requiring new dtc features. There's no point allowing use of new dtc
> features if we can't require them, since there's no fallback when they
> aren't there to use.
>

My patch is specifically designed to provide a fallback when the are not
there to use. I'm not sure how feasible this is long term, but it works for
now.

I have no problem with including dtc in with U-Boot if that is the chosen
approach. It solves some problems. In fact my Kbuild patch set could make
that quite easy. However I do understand Wolfgang's POV that it is
philosophically a dangerous path.

You know my strong support for some sort of symbolic support in dtc, and I
share your frustration that it hasn't happened yet.

Is my patch better than what is there, or is it just making things worse?

Regards,
Simon
Wolfgang Denk - May 30, 2013, 7:05 a.m.
Dear Stephen,

In message <51A6869F.1020004@wwwdotorg.org> you wrote:
>
> Since DT is supposed to be a HW description, it shouldn't be using cpp's
> built-in macros to compile in different ways; there really isn't a
> concept of the "target arch of compilation"; a .dts file should simply
> compile the same way everywhere. Different DTs represent different HW;

I am not 100% sure of that.  First, it's not correct to say "compiled
compile the same way everywhere" - it's not that "where" it compiled,
butthe "for which machine", i. e. which exact cross tool chain is
selected.

And I can well imagine DTs that include parts for specific IP blocks,
where the exact details might depend on the target architecture.  We
see an incresing number of cases where the same IP blocks are being
used in different architectures, even of different endianess, etc.
So even if this appears not useful or needed today, I think we should
not intentionally barr the way unless that would realy cause problems
to us.  

Or do you see a real potentiol of conflict with the classed "reserved
namespaces" in GCC with double underscores?

> we don't use a single DT with conditional compilation to represent
> different HW. This led Rob Herring (one of the kernel device tree
> maintainers) to propose the following rules for cpp usage on device trees:
> 
> https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-October/020831.html
> 
> One of which was:
> 
> - No gcc built-in define usage

I understand that was part of some discussion then.  But has it been
generally agreed on, and eventually even turned into a DT policy
document?

> Some history on why we needed to not-define, or un-define, some macros
> (such as the linux macro we're discussing here) can be found in:
> 
> http://linux-kernel.2935.n7.nabble.com/PATCH-V7-kbuild-create-a-rule-to-run-the-pre-processor-on-dts-files-td575632.html
> 
> The last few messages there end up mentioning -undef, which we/I ended
> up using. It looks like -ansi wasn't mentioned at all when discussing
> this issue.

"-x assembler-with-cpp" makes sense for other reasons, i. e. being
able to mark related lines incommonly used header files with
__ASSEMBLER__ and making sure that '#' gets only processed when in
column 1.  It does NOT solve the indetifier problem, though, as GCC
will still define "unix" and :linux" (and eventually more):

	-> gcc -E -dM -x assembler-with-cpp - < /dev/null | grep -v _
	#define unix 1
	#define linux 1

> However, I assert that -undef is better, since we end up without any
> pre-defined macros, which DT files shouldn't be using anyway. By my
> reading of the cpp manual link you send, -ansi simply stops
> non-conforming pre-defined macros from being defined, but doesn't stop
> them all being defined as -undef does. Hence, I prefer -undef.

Why do you want to stop something which doesn't hurt you, but might
eventually be useful in other cases?

No, I do not have any real such use cases at hand, but I can see that
it might be for example useful to auto-detect the certain properties
of the system we're building for.

> Oh, and another of Rob's proposed rules:

Agreed, another propsal...

> shouldn't be exposed. For this reason, create a specific include
> directory for use by dtc+cpp, and symlink dt-bindings from there to the
> actual location of include/dt-bindings/. For want of a better location,
> place this "include chroot" into the existing dts/ directory.
> 
> arch/*/boot/dts/include/dt-bindings -> ../../../../../include/dt-bindings

OT here, but: Don't you see a risk that such arch specific things
might become a problem soon?


Best regards,

Wolfgang Denk
Wolfgang Denk - May 30, 2013, 7:49 a.m.
Dear Stephen Warren,

In message <51A68A4C.4060505@wwwdotorg.org> you wrote:
>
> > Sorry, instead we should strive to be compatible to a reasonably old,
> > stable version of DTC, like we do for all other tools as well.  As
> > mentioned before - just because RHEL 5 ships an ancient version of -
> > say - "make" we will NOT start building this from the sources ourself.
> > This cannot be the way to go.
> 
> So the result of that is that we can never ever use new features in any
> tool, at least in any meaningful time-frame.

I wrote "we should strive", not "this is the only way".

> However, when enabling a new feature, such as using device trees to
> configure U-Boot[1], for which tool support is new and evolving along
> with the feature itself, and which is only used on a very very few
> boards and even fewer SoCs right now within U-Boot, it seems entirely
> reasonable to demand that the people working on/with that new feature
> are aware that it's evolving, and that they may need to take a few extra
> steps to go out and get tools that support that new feature. No doubt
> once this feature has settled down a bit, and distros have pulled in
> newer versions of dtc, everthing will "just work" just like any other
> stable feature.

Agreed.  And nothing prevents you to installl on your system another,
more recent version of dtc or any other tools needed for specific
purposes.

> If you don't accept this, then we simply have to ban any include use in
> U-Boot; dtc -i isn't in distro-packaged versions of dtc, so we'd need to

This has never been my intention. I object only against including the
dtc source into U-Boot, and against automatic methods to build dtc as
part of U-Boot, even in a way that depends on the U-Boot version.

dtc is a tool, like gcc or others.  We may test against specific
versions of the tools in the Makefiles, and even abort a build if an
incompatible version is found.  But we will not include tool sources,
nor build / install rules for tools.

Best regards,

Wolfgang Denk
Wolfgang Denk - May 30, 2013, 7:56 a.m.
Dear Stephen,

In message <51A6DF7C.30903@wwwdotorg.org> you wrote:
>
> It seems to be aimed specifically at enabling use of new dtc features
> when present. That seems to be specifically against Wolfgang's goal of
> not requiring new dtc features. There's no point allowing use of new dtc

Please stop planting statements on me that I did not make.

Best regards,

Wolfgang Denk
Stephen Warren - May 30, 2013, 5:38 p.m.
On 05/30/2013 01:56 AM, Wolfgang Denk wrote:
> Dear Stephen,
> 
> In message <51A6DF7C.30903@wwwdotorg.org> you wrote:
>>
>> It seems to be aimed specifically at enabling use of new dtc features
>> when present. That seems to be specifically against Wolfgang's goal of
>> not requiring new dtc features. There's no point allowing use of new dtc
> 
> Please stop planting statements on me that I did not make.

Sorry, but that's what I believe you meant. If you didn't mean that,
well then let's clear this up, and the rest of the conversation will be
a lot simpler:

I believe that for building the *.dts in U-Boot we should simply require
the user to have a version of dtc that supports the recently added
features such as:

* -i directive.
* Ability to parse the output of cpp well (e.g. #line directives, emit
useful error/warning messages in this case).
* Cell expression support.

Those are all present in the latest git repo for dtc.

If we do that, then we won't need any conditional logic in the U-Boot
makefiles. At least parts of Simon's patch won't be necessary.

I'm quite happy to achieve this requirement by having the user install
that dtc into the $PATH prior to running any U-Boot make, either
manually or via distro packages once they're available.

It is my understanding that you object to requiring such a new version
of dtc, irrespective of the means by which it's provided. If this is not
true, then please do let me know; it would vastly simplify matters.

Patch

diff --git a/dts/Makefile b/dts/Makefile
index 03e163e..1f6fabb 100644
--- a/dts/Makefile
+++ b/dts/Makefile
@@ -37,11 +37,29 @@  $(if $(CONFIG_ARCH_DEVICE_TREE),,\
 $(error Your architecture does not have device tree support enabled. \
 Please define CONFIG_ARCH_DEVICE_TREE))
 
+# Provide a list of include directories for dtc
+DTS_INCS-y := -i $(SRCTREE)/arch/$(ARCH)/dts
+
+DTS_INCS-y += -i $(SRCTREE)/board/$(VENDOR)/dts
+
+DTS_INCS-$(CONFIG_CHROMEOS) += -i $(SRCTREE)/cros/dts
+
+# Check if our dtc includes the -i option
+DTS_FLAGS := $(shell if ! dtc -i 2>&1 | grep -q "invalid option"; then \
+		echo $(DTS_INCS-y); fi)
+
 # We preprocess the device tree file provide a useful define
-DTS_CPPFLAGS := -x assembler-with-cpp \
+# Undefine 'linux' since it might be used in device tree files
+DTS_CPPFLAGS := -x assembler-with-cpp -Ulinux \
 		-DARCH_CPU_DTS=\"$(SRCTREE)/arch/$(ARCH)/dts/$(CONFIG_ARCH_DEVICE_TREE).dtsi\" \
 		-DBOARD_DTS=\"$(SRCTREE)/board/$(VENDOR)/$(BOARD)/dts/$(DEVICE_TREE).dts\" \
-		-I$(SRCTREE)/board/$(VENDOR)/dts -I$(SRCTREE)/arch/$(ARCH)/dts
+		-D__ASSEMBLY__ -I$(OBJTREE)/include -I$(SRCTREE)/include \
+		-I$(OBJTREE)/include2 \
+		-I$(SRCTREE)/board/$(VENDOR)/dts -I$(SRCTREE)/arch/$(ARCH)/dts \
+		-include $(OBJTREE)/include/config.h
+
+DTS_TMP := $(OBJTREE)/include/generated/$(DEVICE_TREE).dts.in
+DTS_SRC := board/$(VENDOR)/dts/$(DEVICE_TREE).dts
 
 all:	$(obj).depend $(LIB)
 
@@ -50,13 +68,19 @@  all:	$(obj).depend $(LIB)
 # the filename.
 DT_BIN	:= $(obj)dt.dtb
 
-$(DT_BIN): $(TOPDIR)/board/$(VENDOR)/dts/$(DEVICE_TREE).dts
+DTC_CMD := $(DTC) -R 4 -p 0x1000 -O dtb -o ${DT_BIN} $(DTS_FLAGS) $(DTS_TMP)
+
+$(DT_BIN): $(TOPDIR)/$(DTS_SRC)
 	rc=$$( \
-		cat $< | $(CPP) -P $(DTS_CPPFLAGS) - | \
-		{ { $(DTC) -R 4 -p 0x1000 -O dtb -o ${DT_BIN} - 2>&1 ; \
+		cat $< | $(CPP) -P $(DTS_CPPFLAGS) - > $(DTS_TMP); \
+		{ { $(DTC_CMD)  2>&1 ; \
 		    echo $$? >&3 ; } | \
 		  grep -v '^DTC: dts->dtb  on file' ; \
 	        } 3>&1 1>&2 ) ; \
+	if [ $$rc != 0 ]; then \
+		echo "Source file is $(DTS_SRC)"; \
+		echo "Compiler: $(DTC_CMD)"; \
+	fi; \
 	exit $$rc
 
 process_lds = \