Fill-in more details of boot stub

Fill-in more details of boot stub

BUG=chromium-os:1302
TEST=MAKEALL successfully

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=fd28df1

[sjg, 2011-03-01 05:13:41]

http://codereview.chromium.org/6594043/diff/1/common/cmd_cros_bootstub.c
File common/cmd_cros_bootstub.c (right):

http://codereview.chromium.org/6594043/diff/1/common/cmd_cros_bootstub.c#newc...
common/cmd_cros_bootstub.c:52: int status = !LOAD_FIRMWARE_SUCCESS;
Ick - this is odd. Perhaps just 0 for ok, -1 for error?

http://codereview.chromium.org/6594043/diff/1/common/cmd_cros_bootstub.c#newc...
common/cmd_cros_bootstub.c:100: return 1;
Should this hang, jump to recovery mode, something else?

[Che-Liang Chiou, 2011-03-01 08:45:21]

Hi Simon,

Thanks for your comments. See changes below. PTAL.

http://codereview.chromium.org/6594043/diff/1/common/cmd_cros_bootstub.c
File common/cmd_cros_bootstub.c (right):

http://codereview.chromium.org/6594043/diff/1/common/cmd_cros_bootstub.c#newc...
common/cmd_cros_bootstub.c:52: int status = !LOAD_FIRMWARE_SUCCESS;
On 2011/03/01 05:13:41, sjg wrote:
> Ick - this is odd. Perhaps just 0 for ok, -1 for error?

Change it to LOAD_FIRMWARE_RECOVERY (since we should jump to recovery firmware
when no valid firmware.)

http://codereview.chromium.org/6594043/diff/1/common/cmd_cros_bootstub.c#newc...
common/cmd_cros_bootstub.c:93: CONFIG_FIRMWARE_BODY_BUFFER_0 :
Remove statically allocated buffers. Changes due to earlier code review
comments.

http://codereview.chromium.org/6594043/diff/1/common/cmd_cros_bootstub.c#newc...
common/cmd_cros_bootstub.c:100: return 1;
On 2011/03/01 05:13:41, sjg wrote:
> Should this hang, jump to recovery mode, something else?

We should jump to recovery firmware when rewritable firmware accidentally
returns.

But if recovery firmware accidentally returns, there is nothing we can do. Do
you think it should hang or reset cpu?

http://codereview.chromium.org/6594043/diff/5001/lib/chromeos/load_firmware_fw.c
File lib/chromeos/load_firmware_fw.c (left):

http://codereview.chromium.org/6594043/diff/5001/lib/chromeos/load_firmware_f...
lib/chromeos/load_firmware_fw.c:48: firmware_body = (uint8_t *)
CONFIG_FIRMWARE_BODY_BUFFER_0;
Changes due to earlier code review comments. Remove statically allocated
buffers.

[sjg, 2011-03-02 02:05:23]

Requests for comments, otherwise LGTM

If loading recovery firmware fails we have a brick I think, since the stub
cannot reflash. I'm not sure if this would only be due to an SPI failure or
could happen if the verified code is different?

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c
File common/cmd_cros_bootstub.c (right):

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:39: int firmware_index;
Comments, if you haven't done it in another CL.

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:43: int load_firmware(int primary_firmware, int
boot_flags,
params comment

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:55: int do_cros_bootstub(cmd_tbl_t *cmdtp, int flag,
int argc, char * const argv[])
I think you did tell me where the comment was for this but I can't see it.

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:99: jump_to_firmware((void (*)(void))
This is pretty ugly - since you are probably going to end up passing params to
the chained U-Boot, perhaps pass the vf structure and index? But up to you.

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:109: return 1;
-1?

[sjg, 2011-03-02 02:10:57]

A few more comments.

[Che-Liang Chiou, 2011-03-02 10:10:20]

On 2011/03/02 02:05:23, sjg wrote:
> Requests for comments, otherwise LGTM
> 
> If loading recovery firmware fails we have a brick I think, since the stub
> cannot reflash. I'm not sure if this would only be due to an SPI failure or
> could happen if the verified code is different?

Since recovery firmware is also stored in read-only part of SPI, the only reason
it could fail I can think of is SPI failure. The verified code can't be changed
since they are all (boot stub and recovery firmware) in read-only part.

[Che-Liang Chiou, 2011-03-02 10:39:31]

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c
File common/cmd_cros_bootstub.c (right):

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:39: int firmware_index;
On 2011/03/02 02:05:23, sjg wrote:
> Comments, if you haven't done it in another CL.

Done.

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:43: int load_firmware(int primary_firmware, int
boot_flags,
On 2011/03/02 02:05:23, sjg wrote:
> params comment

Done. Copied and edited from later CL.

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:55: int do_cros_bootstub(cmd_tbl_t *cmdtp, int flag,
int argc, char * const argv[])
On 2011/03/02 02:05:23, sjg wrote:
> I think you did tell me where the comment was for this but I can't see it.

I guess it's in a later CL.

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:99: jump_to_firmware((void (*)(void))
On 2011/03/02 02:05:23, sjg wrote:
> This is pretty ugly - since you are probably going to end up passing params to
> the chained U-Boot, perhaps pass the vf structure and index? But up to you.

I will think about it.

http://codereview.chromium.org/6594043/diff/5001/common/cmd_cros_bootstub.c#n...
common/cmd_cros_bootstub.c:109: return 1;
On 2011/03/02 02:05:23, sjg wrote:
> -1?

u-boot command's error code uses positive values.

[sjg, 2011-03-03 05:29:05]

LGTM, thanks.