[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[Nobuhiro Iwamatsu]

Usually, when CONFIG_OF_LIBFDT is enabled, U-Boot is set to
the FDT memory information that is set in the U-Boot. This patch
disables this behavior.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
---
 README                   | 8 ++++++++
 arch/arm/lib/bootm-fdt.c | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/README b/README
index d337374..73453fe 100644
--- a/README
+++ b/README
@@ -650,6 +650,14 @@ The following options need to be configured:
 		in a single configuration file and the machine type is
 		runtime discoverable, do not have to use this setting.
 
+		CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE
+
+		Usually, when CONFIG_OF_LIBFDT is enabled, U-Boot is set to
+		the FDT memory information that is set in the U-Boot. This will
+		disable this behavior.
+		If you do not use the memory configuration of U-Boot, you want
+		to set the priority of the FDT, please enable this.
+
 - vxWorks boot parameters:
 
 		bootvx constructs a valid bootline using the following
diff --git a/arch/arm/lib/bootm-fdt.c b/arch/arm/lib/bootm-fdt.c
index e40691d..8da9dac 100644
--- a/arch/arm/lib/bootm-fdt.c
+++ b/arch/arm/lib/bootm-fdt.c
@@ -18,6 +18,7 @@
 #include <common.h>
 #include <fdt_support.h>
 
+#ifndef CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE
 DECLARE_GLOBAL_DATA_PTR;
 
 int arch_fixup_memory_node(void *blob)
@@ -34,3 +35,4 @@ int arch_fixup_memory_node(void *blob)
 
 	return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
 }
+#endif  /* CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE */
-- 
1.8.5

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[Wolfgang Denk]

Dear Nobuhiro Iwamatsu,

In message <1396846600-15386-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com> you wrote:
> Usually, when CONFIG_OF_LIBFDT is enabled, U-Boot is set to
> the FDT memory information that is set in the U-Boot. This patch
> disables this behavior.
> 
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
> ---
>  README                   | 8 ++++++++
>  arch/arm/lib/bootm-fdt.c | 2 ++
>  2 files changed, 10 insertions(+)

Please explain why you would want to do this.  To me it makes no
sense.  Either U-Boot knows the correct memory size, then it should
pass it to Linux.  Or it does not, then U-Boot should be fixed.

Also, I object that your implementation is ARM specific.  If such a
feature gets added, it should be architecture independent.

Thanks.

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
To be a winner, all you need to give is all you have.

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[Tom Rini]

On Mon, Apr 07, 2014 at 08:53:39AM +0200, Wolfgang Denk wrote:
> Dear Nobuhiro Iwamatsu,
> 
> In message <1396846600-15386-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com> you wrote:
> > Usually, when CONFIG_OF_LIBFDT is enabled, U-Boot is set to
> > the FDT memory information that is set in the U-Boot. This patch
> > disables this behavior.
> > 
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
> > ---
> >  README                   | 8 ++++++++
> >  arch/arm/lib/bootm-fdt.c | 2 ++
> >  2 files changed, 10 insertions(+)
> 
> Please explain why you would want to do this.  To me it makes no
> sense.  Either U-Boot knows the correct memory size, then it should
> pass it to Linux.  Or it does not, then U-Boot should be fixed.
> 
> Also, I object that your implementation is ARM specific.  If such a
> feature gets added, it should be architecture independent.

This has shown up before in the context of platforms with >4GB memory
and we "correct" the node by reducing the system memory, which is
incorrect.  I agree this needs to be done for more than just ARM
however.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140407/0ae7f440/attachment.pgp>

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[Nobuhiro Iwamatsu]

Hi,

2014-04-08 2:41 GMT+09:00 Tom Rini <trini@ti.com>:
> On Mon, Apr 07, 2014 at 08:53:39AM +0200, Wolfgang Denk wrote:
>> Dear Nobuhiro Iwamatsu,
>>
>> In message <1396846600-15386-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com> you wrote:
>> > Usually, when CONFIG_OF_LIBFDT is enabled, U-Boot is set to
>> > the FDT memory information that is set in the U-Boot. This patch
>> > disables this behavior.
>> >
>> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
>> > ---
>> >  README                   | 8 ++++++++
>> >  arch/arm/lib/bootm-fdt.c | 2 ++
>> >  2 files changed, 10 insertions(+)
>>
>> Please explain why you would want to do this.  To me it makes no
>> sense.  Either U-Boot knows the correct memory size, then it should
>> pass it to Linux.  Or it does not, then U-Boot should be fixed.
>>
>> Also, I object that your implementation is ARM specific.  If such a
>> feature gets added, it should be architecture independent.
>
> This has shown up before in the context of platforms with >4GB memory
> and we "correct" the node by reducing the system memory, which is
> incorrect.  I agree this needs to be done for more than just ARM
> however.

It is one of the reasons of this patch that you wrote.

Thanks,
  Nobuhiro
>
> --
> Tom
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>



-- 
Nobuhiro Iwamatsu

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[Nobuhiro Iwamatsu]

Hi,

Thanks for your comment.

2014-04-07 15:53 GMT+09:00 Wolfgang Denk <wd@denx.de>:
> Dear Nobuhiro Iwamatsu,
>
> In message <1396846600-15386-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com> you wrote:
>> Usually, when CONFIG_OF_LIBFDT is enabled, U-Boot is set to
>> the FDT memory information that is set in the U-Boot. This patch
>> disables this behavior.
>>
>> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
>> ---
>>  README                   | 8 ++++++++
>>  arch/arm/lib/bootm-fdt.c | 2 ++
>>  2 files changed, 10 insertions(+)
>
> Please explain why you would want to do this.  To me it makes no
> sense.  Either U-Boot knows the correct memory size, then it should
> pass it to Linux.  Or it does not, then U-Boot should be fixed.

For example, I can access the memory of all in the U-Boot, but I may
want to control
the highmem on Linux,I do not want to show a specific area from kernel
and userland.

>
> Also, I object that your implementation is ARM specific.  If such a
> feature gets added, it should be architecture independent.

I see. But arch_fixup_memory_node() is used by ARM only.
So, we see to be dependent on the ARM is only this.

>
> Thanks.
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> To be a winner, all you need to give is all you have.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Nobuhiro Iwamatsu

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[Wolfgang Denk]

Dear Nobuhiro,

In message <CABMQnV+k+Rmx7E8o-nfBpYg5-nWrXi6Oz_+BCYs-vDNdv_z-rw@mail.gmail.com> you wrote:
> 
> > Please explain why you would want to do this.  To me it makes no
> > sense.  Either U-Boot knows the correct memory size, then it should
> > pass it to Linux.  Or it does not, then U-Boot should be fixed.
> 
> For example, I can access the memory of all in the U-Boot, but I may
> want to control
> the highmem on Linux,I do not want to show a specific area from kernel
> and userland.

Is it not sufficient to pass some "mem=" boot argument?  We even have
automatic support for this in U-Boot (see the CONFIG_PRAM feature).

> > Also, I object that your implementation is ARM specific.  If such a
> > feature gets added, it should be architecture independent.
> 
> I see. But arch_fixup_memory_node() is used by ARM only.
> So, we see to be dependent on the ARM is only this.

All architectures that support the device tree update the memory size
for Linux, so we should find a generic way to handle this.  Actually
we should always strive to reduce this arhitecture specific code.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
This restaurant was advertising breakfast  any  time.  So  I  ordered
french toast in the renaissance.            - Steven Wright, comedian

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[Tom Rini]

On Tue, Apr 08, 2014 at 03:05:36PM +0200, Wolfgang Denk wrote:
> Dear Nobuhiro,
> 
> In message <CABMQnV+k+Rmx7E8o-nfBpYg5-nWrXi6Oz_+BCYs-vDNdv_z-rw@mail.gmail.com> you wrote:
> > 
> > > Please explain why you would want to do this.  To me it makes no
> > > sense.  Either U-Boot knows the correct memory size, then it should
> > > pass it to Linux.  Or it does not, then U-Boot should be fixed.
> > 
> > For example, I can access the memory of all in the U-Boot, but I may
> > want to control
> > the highmem on Linux,I do not want to show a specific area from kernel
> > and userland.
> 
> Is it not sufficient to pass some "mem=" boot argument?  We even have
> automatic support for this in U-Boot (see the CONFIG_PRAM feature).

There's various ways to do this, yes.  But it doesn't cover the >4GB
case.

> > > Also, I object that your implementation is ARM specific.  If such a
> > > feature gets added, it should be architecture independent.
> > 
> > I see. But arch_fixup_memory_node() is used by ARM only.
> > So, we see to be dependent on the ARM is only this.
> 
> All architectures that support the device tree update the memory size
> for Linux, so we should find a generic way to handle this.  Actually
> we should always strive to reduce this arhitecture specific code.

Note that ARM provides arch_fixup_memory_node to make sure we have all
of the bank information populated and then calls fdt_fixup_memory_banks,
while PowerPC just calls fdt_fixup_memory which calls banks with a '1'
for number of banks.  MIPS (and everyone else) isn't doing anything
about this atm, but probably should.

At the high level, we need to see if we _really_ do need to be using
arch_fixup_memory_node at all because my gut feeling is (a) we've
already always filled in the bank info and if not (b) that is a bug to
correct.  But I haven't dived in to the relevant code here yet.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140408/71b974f4/attachment.pgp>

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[thomas.langer at lantiq.com]

Hello Tom,

> Note that ARM provides arch_fixup_memory_node to make sure we have all
> of the bank information populated and then calls fdt_fixup_memory_banks,
> while PowerPC just calls fdt_fixup_memory which calls banks with a '1'
> for number of banks.  MIPS (and everyone else) isn't doing anything
> about this atm, but probably should.

I assume the main reason this is not done for MIPS is the missing support for
providing devicetrees from U-Boot.

Daniel: Do you know if there is any activity to add this?

Best Regards,
Thomas

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[Masahiro Yamada]

Hi Nobuhiro, Tom,


> diff --git a/arch/arm/lib/bootm-fdt.c b/arch/arm/lib/bootm-fdt.c
> index e40691d..8da9dac 100644
> --- a/arch/arm/lib/bootm-fdt.c
> +++ b/arch/arm/lib/bootm-fdt.c
> @@ -18,6 +18,7 @@
>  #include <common.h>
>  #include <fdt_support.h>
>  
> +#ifndef CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  int arch_fixup_memory_node(void *blob)
> @@ -34,3 +35,4 @@ int arch_fixup_memory_node(void *blob)
>  
>  	return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
>  }
> +#endif  /* CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE */


I am not happy about defining CONFIG macro to disable some code.

Please do

#ifdef CONFIG_FDT_FIXUP_MEMORY_NODE
   .....
#endif

rather than

#ifndef CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE
   .....
#endif



We expect most of boards should be fixed-up by U-Boot.
So, add 

 #define CONFIG_FDT_FIXUP_MEMORY_NODE

to include/config_defaults.h

and

#undef  CONFIG_FDT_FIXUP_MEMORY_NODE

only to boards for which you want to skip memory fix-up.




Basically, we should not use CONFIG macros for negation.

CONFIG_SKIP_LOWLEVEL_INIT, CONFIG_SYS_DCACHE_OFF,
are examples of bad macros.


See 

ifndef CONFIG_SKIP_LOWLEVEL_INIT
obj-y	+= lowlevel_init.o
endif

everywhere in Makefiles,
which suggests we had chosen a bad macro name.



Best Regards
Masahiro Yamada

[U-Boot] [PATCH] lib: Add CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE

[Tom Rini]

On Wed, Apr 09, 2014 at 12:20:43PM +0900, Masahiro Yamada wrote:

> Hi Nobuhiro, Tom,
> 
> 
> > diff --git a/arch/arm/lib/bootm-fdt.c b/arch/arm/lib/bootm-fdt.c
> > index e40691d..8da9dac 100644
> > --- a/arch/arm/lib/bootm-fdt.c
> > +++ b/arch/arm/lib/bootm-fdt.c
> > @@ -18,6 +18,7 @@
> >  #include <common.h>
> >  #include <fdt_support.h>
> >  
> > +#ifndef CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE
> >  DECLARE_GLOBAL_DATA_PTR;
> >  
> >  int arch_fixup_memory_node(void *blob)
> > @@ -34,3 +35,4 @@ int arch_fixup_memory_node(void *blob)
> >  
> >  	return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
> >  }
> > +#endif  /* CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE */
> 
> 
> I am not happy about defining CONFIG macro to disable some code.
> 
> Please do
> 
> #ifdef CONFIG_FDT_FIXUP_MEMORY_NODE
>    .....
> #endif
> 
> rather than
> 
> #ifndef CONFIG_FDT_IGNORE_FIXUP_MEMORY_NODE
>    .....
> #endif
> 
> 
> 
> We expect most of boards should be fixed-up by U-Boot.
> So, add 
> 
>  #define CONFIG_FDT_FIXUP_MEMORY_NODE
> 
> to include/config_defaults.h
> 
> and
> 
> #undef  CONFIG_FDT_FIXUP_MEMORY_NODE
> 
> only to boards for which you want to skip memory fix-up.

Agreed.

> Basically, we should not use CONFIG macros for negation.
> 
> CONFIG_SKIP_LOWLEVEL_INIT, CONFIG_SYS_DCACHE_OFF,
> are examples of bad macros.

Lets hold off on fixing these until we're farther along with the
conversion to Kconfig.  Unless it'll be really problematic not to..
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140409/937df1b2/attachment.pgp>