| Message ID | 1396846600-15386-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
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
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.
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@denx.de > To be a winner, all you need to give is all you have. > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
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@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
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
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.
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
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
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!
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 */
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(+)