Patchwork [U-Boot] common/dlmalloc: support reinit bin for fast reset

login
register
mail settings
Submitter Macpaul Lin
Date Dec. 31, 2010, 7:13 a.m.
Message ID <1293779628-9804-1-git-send-email-macpaul@andestech.com>
Download mbox | patch
Permalink /patch/77048/
State Rejected
Headers show

Comments

Macpaul Lin - Dec. 31, 2010, 7:13 a.m.
There are several way to reset the u-boot.
Some platform will use watchdog timeout to reset the system.
Some platfrom will jump to 0x0 to reload the u-boot.
Some platform will jump to CONFIG_SYS_TEXT_BASE to do fast reset.

This patch fixed the problem of static varible didn't cleared on
the platforms which "CONFIG_SKIP_LOWLEVEL_INIT" is enabled and do
software reset by simply jump to the address "CONFIG_SYS_TEXT_BASE".

This patch also introduce a new define named "CONFIG_FAST_RESET" for
developer to distinguish this method from other reset methods.

Signed-off-by: Macpaul Lin <macpaul@andestech.com>
---
 README            |   10 ++++++++++
 common/dlmalloc.c |   24 ++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)
Mike Frysinger - Dec. 31, 2010, 7:42 a.m.
On Friday, December 31, 2010 02:13:48 Macpaul Lin wrote:
> There are several way to reset the u-boot.
> Some platform will use watchdog timeout to reset the system.
> Some platfrom will jump to 0x0 to reload the u-boot.
> Some platform will jump to CONFIG_SYS_TEXT_BASE to do fast reset.
> 
> This patch fixed the problem of static varible didn't cleared on
> the platforms which "CONFIG_SKIP_LOWLEVEL_INIT" is enabled and do
> software reset by simply jump to the address "CONFIG_SYS_TEXT_BASE".

i have no idea what "CONFIG_SKIP_LOWLEVEL_INIT" is, so i cant really comment 
on what you're trying to do

> +void reinit_bin()

needs to be (void)

> +extern void reinit_malloc_pool()

here too, and putting "extern" on the actual func def makes no sense
-mike
Wolfgang Denk - Dec. 31, 2010, 8:49 a.m.
Dear Macpaul Lin,

In message <1293779628-9804-1-git-send-email-macpaul@andestech.com> you wrote:
> There are several way to reset the u-boot.

To avoid confusion, we should try and agree on certain terms.

Reset means (at least in U-Boot context) to bring a system to an
initial state, similar to what whappens when you press the reset
button.  "Initial state" also means that the hardware, including
memory and peripherals, are reset to an initial state where they can
reliably be accessed.  As a result of a "reset", the CPU will begin
execution at it's reset entry point, loading code from the configured
boot device.

This differs from "restart", where an existing instance of a software
component (which might be an U-Boot image loaded to memory) begins
execution from some starting point.  In U-Boot context, a different
term for a restart might be "warm start" or "warm boot".

U-Boot only supports "reset".  It does NOT support any way of
restart.  THere are a number of reasons for that:

1) We do not keep a copy of the initial state of the data segment.
   Thus any changes to data that are used by the code as indicators
   for steps already performed will remain, which will cause such
   steps will be missing or will run based on incorrect assumptions.

2) There are situations where a restart will not be possible, For
   example, if you have a system which boots from NOR flash, and you
   attempt a restart when the flash chips just happen to be in some
   command mode.  The you cannot read normal data from them, and the
   CPU will not be able to fetch instructions from the boot device.

> Some platform will use watchdog timeout to reset the system.

Correct.  Using a watchdog reset is one of the common ways to cause a
hard reset on some systems.

> Some platfrom will jump to 0x0 to reload the u-boot.

Wrong. This may attempt to restart U-Boot, and if you are lucky it
will eventually even load a fresh copy of U-Boot to RAM.  But this is
highly SoC and board specific, and it is only a restart, and NOT a
reset.

> Some platform will jump to CONFIG_SYS_TEXT_BASE to do fast reset.

Wrong. This is not a reset either. Even worse, here you don;t even
attempt to reload U-Boot and thus to re-initialize the data and bss
segments, which is just begging for all sorts of trouble.

This is a fundamentally broken design.  Please do not try to paper
over problems, fix the design.


> This patch fixed the problem of static varible didn't cleared on
> the platforms which "CONFIG_SKIP_LOWLEVEL_INIT" is enabled and do
> software reset by simply jump to the address "CONFIG_SYS_TEXT_BASE".

It makes no sense to try and fix individual variables to their
initial state - you would have to reload the whole data segment from
some copy holding it's original values, and you have to re-initialize
the whole bss segment to zero.  [And even then you are still trying to
o somthing which is unsupported.]

> This patch also introduce a new define named "CONFIG_FAST_RESET" for
> developer to distinguish this method from other reset methods.

This is a misnomer in severasl ways: it does not perform any reset
operation of the system (but attempts to perform a (unsupported)
restart.

> +- CONFIG_FAST_RESET
> +		If both CONFIG_SKIP_LOWLEVEL_INIT and this varible is defined,
> +		the system could use do fast software reset by simply reinit
> +		dlmalloc module then jump to the starting address
> +		"CONFIG_SYS_TEXT_BASE".
> +
> +		To use this feature, the extern function "reinit_malloc_pool()"
> +		in dlmalloc.c should be called inside
> +		reset_cpu(CONFIG_SYS_TEXT_BASE).

This makes the problems of your design obvious.

Why should resetting the processor include a reinitalization of malloc?

I will not accept any such changes.  Please fix your design and
implement a real reset.


If you want to extend the U-Boot design by adding support for a
restart feature (or "warm start"), then this needs to be done right;
setting of a few selected variables is definitely not sufficient.

Best regards,

Wolfgang Denk
Macpaul Lin - Jan. 3, 2011, 7:43 a.m.
HI Wolfgang,

2010/12/31 Wolfgang Denk <wd@denx.de>:
> Dear Macpaul Lin,
>
> In message <1293779628-9804-1-git-send-email-macpaul@andestech.com> you wrote:
>> There are several way to reset the u-boot.
>
> To avoid confusion, we should try and agree on certain terms.
>
> Reset means (at least in U-Boot context) to bring a system to an
> initial state, similar to what whappens when you press the reset
> button.  "Initial state" also means that the hardware, including
> memory and peripherals, are reset to an initial state where they can
> reliably be accessed.  As a result of a "reset", the CPU will begin
> execution at it's reset entry point, loading code from the configured
> boot device.

> This differs from "restart", where an existing instance of a software
> component (which might be an U-Boot image loaded to memory) begins
> execution from some starting point.  In U-Boot context, a different
> term for a restart might be "warm start" or "warm boot".


Thanks for your clarification.

The reason of the need of implementing "warm start" is for the following
scenario.

There are 2 boot loaders in the same flash on our evaluation board
"ADP-AG101".

The flash layout of evb "ADP-AG101" is like this:
The starting address of flash is 0x80400000
0x80400000: evb_loader
0x80500000: u-boot
0x80600000: non-OS demo programs
0x80800000: OS (Linux)
0x80980000: rootfs

One is customized boot loader, e.g, "an evaluation boot loader" (evb_loader),
which is used for quick application demo and diagnostic the peripherals.This
evaluation boot loader also deals with loading and execution OS
(LInux, RTOS) and non-OS application in the memory. This boot loader
also load and jump to generic u-boot (CONFIG_SKIP_LOWLEVEL_INIT is enabled
in this u-boot) for demo, training, and development purpose.

The second boot loader is generic u-boot. Since the evaluation board is booted
by evb_loader, then the memory controller and mem relocation will be initialized
by evb_loader.

User could evaluate the board function with evb_board, then develops the product
software by using u-boot and Linux kernel to simulate the real product behavior.
Once the user has done development process, he/she can replace the evb_loader
with real generic u-boot (with CONFIG_SKIP_LOWLEVEL_INIT is disabled).

That's why we may need "warm start" or "warm boot" which let user
restart the u-boot
without resetting the board and jumping to the evb_loader again..

Even there exist a need with "warm boot", the NDS32 architecture still do
CPU and memory reset. In the current reset_cpu() function whihc NDS32
has provided
(it could be hooked to watchdog reset or warm boot), reset still do
the following reset
process.
1. do_reset(). (cpu.c)
2. disable interrupts. (cpu.c)
3. reset_cpu(). (cpu.c)
3.1. invalidate I/D cache.
3.2. reset MMU, TLB,
3.3. flush TLB,
3.4. reset cache control registers.
if use watchdog reset
3.5 telling watchdog to reset the hardware.
4. system loads the boot loader from flash. (start.S)
else if use warm boot.
3.5. jump the to the CONFIG_SYS_TEXT_BASE,
4. execute start up code in memory. (start.S)

> U-Boot only supports "reset".  It does NOT support any way of
> restart.  There are a number of reasons for that:
>
> 1) We do not keep a copy of the initial state of the data segment.
>   Thus any changes to data that are used by the code as indicators
>   for steps already performed will remain, which will cause such
>   steps will be missing or will run based on incorrect assumptions.
>
> 2) There are situations where a restart will not be possible, For
>   example, if you have a system which boots from NOR flash, and you
>   attempt a restart when the flash chips just happen to be in some
>   command mode.  Then you cannot read normal data from them, and the
>   CPU will not be able to fetch instructions from the boot device.
>

[deleted]

> Why should resetting the processor include a reinitalization of malloc?
>
> I will not accept any such changes.  Please fix your design and
> implement a real reset.
>
>
> If you want to extend the U-Boot design by adding support for a
> restart feature (or "warm start"), then this needs to be done right;
> setting of a few selected variables is definitely not sufficient.
>

Okay, if I want to make U-boot supports "restart feature", it seems I need to
find a reasonable method to reload the data segment and clear bss. It seems
the most secure method is to load code (data segment) from flash.

Do you have further idea of supporting feature "restart" (warm start)?
Wolfgang Denk - Jan. 3, 2011, 9:31 a.m.
Dear Macpaul Lin,

In message <AANLkTimK=c1+aVsJcRwtPG4hPKp6Lvvu0Ji6a_7B3O_u@mail.gmail.com> you wrote:
> 
> User could evaluate the board function with evb_board, then develops the product
> software by using u-boot and Linux kernel to simulate the real product behavior.
> Once the user has done development process, he/she can replace the evb_loader
> with real generic u-boot (with CONFIG_SKIP_LOWLEVEL_INIT is disabled).
>
> That's why we may need "warm start" or "warm boot" which let user
> restart the u-boot
> without resetting the board and jumping to the evb_loader again..

All understood, except for the "That's why" - IO do not see any part
in your explanation above why the "reset" in U-Boot should not perform
a real reset operation?

[Of course I wonder which features your evb_loader provides that are
missing in U-Boot and cannot be added there, but that's your decision.]

> Even there exist a need with "warm boot", the NDS32 architecture still do
> CPU and memory reset. In the current reset_cpu() function whihc NDS32
> has provided
> (it could be hooked to watchdog reset or warm boot), reset still do
> the following reset
> process.

Why?  All this is bogus if you perform a real hardware reset as you
are supposed to do.

> > If you want to extend the U-Boot design by adding support for a
> > restart feature (or "warm start"), then this needs to be done right;
> > setting of a few selected variables is definitely not sufficient.
>
> Okay, if I want to make U-boot supports "restart feature", it seems I need to
> find a reasonable method to reload the data segment and clear bss. It seems
> the most secure method is to load code (data segment) from flash.

...or form another copy of the data segment.

> Do you have further idea of supporting feature "restart" (warm start)?

Well, I can just say that I see no need for such a fature - doing a
real reset is much more reliable and convenient, as you do not need
any extra code.

Best regards,

Wolfgang Denk
Macpaul Lin - Jan. 3, 2011, 10:58 a.m.
Dear Wolfgang,

2011/1/3 Wolfgang Denk <wd@denx.de>:
> Dear Macpaul Lin,
>
> In message <AANLkTimK=c1+aVsJcRwtPG4hPKp6Lvvu0Ji6a_7B3O_u@mail.gmail.com> you wrote:
>>
>> User could evaluate the board function with evb_board, then develops the product
>> software by using u-boot and Linux kernel to simulate the real product behavior.
>> Once the user has done development process, he/she can replace the evb_loader
>> with real generic u-boot (with CONFIG_SKIP_LOWLEVEL_INIT is disabled).
>>
>> That's why we may need "warm start" or "warm boot" which let user
>> restart the u-boot
>> without resetting the board and jumping to the evb_loader again..
>
> All understood, except for the "That's why" - IO do not see any part
> in your explanation above why the "reset" in U-Boot should not perform
> a real reset operation?

Sorry for the description before was not clear enough.
I agree that "reset" in u-boot should perform real reset no matter
the u-boot is the 1st boot loader or the 2nd boot loader.
Of course, the flaw of the original design should be changed.

However, according to the current design specification, AndesTech is trying to
provide a feature which made user could "warm restart" back into u-boot when
u-boot is the 2nd boot loader.

> [Of course I wonder which features your evb_loader provides that are
> missing in U-Boot and cannot be added there, but that's your decision.]

The start.S and lowlevel_init.S in evb_loader and in u-boot are the same.
They perform code copying, relocation, setup stack, and etc. Only the difference
is NDS32 support (CONFIG_SKIP_LOWLEVEL_INIT) which skips some
memory controller and remap configuration. If user disable
CONFIG_SKIP_LOWLEVEL_INIT, u-boot could replace that evb_loader for mass
production purpose.

>> Even there exist a need with "warm boot", the NDS32 architecture still do
>> CPU and memory reset. In the current reset_cpu() function whihc NDS32
>> has provided
>> (it could be hooked to watchdog reset or warm boot), reset still do
>> the following reset
>> process.
>
> Why?  All this is bogus if you perform a real hardware reset as you
> are supposed to do.

The above is just the description of the current design.

If we want to have a warm restart, I fully agree there must be another
program logic or API inside the u-boot to support the correctness of "restart".

> Well, I can just say that I see no need for such a fature - doing a
> real reset is much more reliable and convenient, as you do not need
> any extra code.
>
> Best regards,
>
> Wolfgang Denk
>

Okay, understood.
Wolfgang Denk - Jan. 3, 2011, 11:54 a.m.
Dear Macpaul Lin,

In message <AANLkTinXXE=CLRNXe48AHBOM9m5JGYxncOU+x0-6c1T=@mail.gmail.com> you wrote:
> 
> However, according to the current design specification, AndesTech is trying to
> provide a feature which made user could "warm restart" back into u-boot when
> u-boot is the 2nd boot loader.

What would be the benefit of such an option?

> The start.S and lowlevel_init.S in evb_loader and in u-boot are the same.

Then your evb_loader is GPLed, too?


Best regards,

Wolfgang Denk
Macpaul Lin - Jan. 3, 2011, 12:21 p.m.
Dear Wolfgang,

2011/1/3 Wolfgang Denk <wd@denx.de>:
> Dear Macpaul Lin,
>
> In message <AANLkTinXXE=CLRNXe48AHBOM9m5JGYxncOU+x0-6c1T=@mail.gmail.com> you wrote:
>>
>> However, according to the current design specification, AndesTech is trying to
>> provide a feature which made user could "warm restart" back into u-boot when
>> u-boot is the 2nd boot loader.
>
> What would be the benefit of such an option?

For example, if user have did some env setting manually before booting OS,
and then he want's to drop all those previous env modification. He
just need to restart the
u-boot which is already in the memory, then the default env parameters
will comes back.

>> The start.S and lowlevel_init.S in evb_loader and in u-boot are the same.
>
> Then your evb_loader is GPLed, too?

Yes, although the evb_loader department have created two different
evb_loader, one is
proprietary license, and the other is GPL license. Which evb_loader
I'm working with is the GPL
one. All our customers who bought the evb could choose which one
software package
to be shipped (supported) with. However, I'm not belong to that department.

>
>
> 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@denx.de
> Es gibt immer genug fuer die Beduerfnisse aller, aber  niemals  genug
> fuer die Gier einzelner.                                    -- Ghandi
>
Wolfgang Denk - Jan. 3, 2011, 1:19 p.m.
Dear Macpaul Lin,

In message <AANLkTin9L0NhBASkgFDP6xR6W=LjRsoYeNkS_RRWpf_b@mail.gmail.com> you wrote:
> 
> > What would be the benefit of such an option?
> 
> For example, if user have did some env setting manually before booting OS,
> and then he want's to drop all those previous env modification. He
> just need to restart the
> u-boot which is already in the memory, then the default env parameters
> will comes back.

Entering the command "env default -f" will do the same.  No restart
needed at all.

Anything else?

Best regards,

Wolfgang Denk
Macpaul Lin - Jan. 3, 2011, 1:58 p.m.
Dear Wolfgang,

2011/1/3 Wolfgang Denk <wd@denx.de>:
> Dear Macpaul Lin,
>
> In message <AANLkTin9L0NhBASkgFDP6xR6W=LjRsoYeNkS_RRWpf_b@mail.gmail.com> you wrote:
>>
>
> Anything else?
>

Thanks for your explanation about this during your vacation.
I'll report this issue to suggestion the company to modify the origin spec.
And also tell the customer to use the correct command instead of their requests.
Wolfgang Denk - Jan. 3, 2011, 3:27 p.m.
Dear Macpaul Lin,

In message <AANLkTikX=V1ZLj70dGfdOMTLvA5b1n6MT6Xi8ncrjmGU@mail.gmail.com> you wrote:
>
> Thanks for your explanation about this during your vacation.

You're welcome.

> I'll report this issue to suggestion the company to modify the origin spec.
> And also tell the customer to use the correct command instead of their requests.

Actually this is a pretty common pattern: when a customer asks for
some new feature, it always makes sense to ask what exactly he is
trying to do with that feature, and then think if the same purpose
might be acchieved using existing, standard commands.

Best regards,

Wolfgang Denk

Patch

diff --git a/README b/README
index 1acf9a3..ec00294 100644
--- a/README
+++ b/README
@@ -2846,6 +2846,16 @@  Low Level (hardware related) configuration options:
                 other boot loader or by a debugger which performs
                 these initializations itself.
 
+- CONFIG_FAST_RESET
+		If both CONFIG_SKIP_LOWLEVEL_INIT and this varible is defined,
+		the system could use do fast software reset by simply reinit
+		dlmalloc module then jump to the starting address
+		"CONFIG_SYS_TEXT_BASE".
+
+		To use this feature, the extern function "reinit_malloc_pool()"
+		in dlmalloc.c should be called inside
+		reset_cpu(CONFIG_SYS_TEXT_BASE).
+
 - CONFIG_PRELOADER
 		Modifies the behaviour of start.S when compiling a loader
 		that is executed before the actual U-Boot. E.g. when
diff --git a/common/dlmalloc.c b/common/dlmalloc.c
index e9bab09..97849a5 100644
--- a/common/dlmalloc.c
+++ b/common/dlmalloc.c
@@ -3233,6 +3233,30 @@  int mALLOPt(param_number, value) int param_number; int value;
   }
 }
 
+#if defined(CONFIG_FAST_RESET) || defined(CONFIG_SKIP_LOWLEVEL_INIT)
+void reinit_bin()
+{
+	int i = 0;
+
+	av_[0] = 0;
+	av_[1] = 0;
+
+	for (i = 0; i < NAV ; i++) {
+		av_[i*2+2] = bin_at(i);
+		av_[i*2+2+1] = bin_at(i);
+	}
+}
+
+extern void reinit_malloc_pool()
+{
+	/* fast reset sbrk_base */
+	sbrk_base = (char *)(-1);
+
+	/* re-initial bin */
+	reinit_bin();
+}
+#endif
+
 /*
 
 History: