Rockbox

Tasklist

FS#6929 - Gigabeat bootloader improvements

Attached to Project: Rockbox
Opened by Barry Wardell (barrywardell) - Wednesday, 28 March 2007, 23:44 GMT
Task Type Patches
Category Bootloader
Status Closed
Assigned To No-one
Operating System Gigabeat F/X
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch brings some improvements for the Gigabeat bootloader:
1) Large amount of code simplification
2) Only show boot messages if there is either an error or a button is pressed
3) Remove non-functional dual-boot code. Print a warning about it not working instead.
4) Restore the logo to the main Rockbox binary - otherwise we just see a blue screen while Rockbox is loading

The code for the bootsplash is still present, although I wonder how useful it is given the splash is shown for less than 1 second. The only benefit I see is if the bootsplash matched the rockbox splash, so that it's a seamless transition.

The main problem currently is that now that boot messages are not shown, the bootloader doesn't update the framebuffer at all. However, lcd_init() seems to cause the LCD to update. Since there has been no updates to the framebuffer it updates the LCD with what was last in the framebuffer, which is normally the "Shutting down..." message from the previous shutdown. The options I see for fixing this are:

1) Make lcd_init() not do a LCD update.
2) Make the address of the LCD framebuffer the same as in the Gigabeat bootloader (the one with the progress bar). I think this would make the LCD be updated with what's currently on screen, so there would be no noticable update.
3) Don't do lcd_init() in the bootloader unless there is an error or a button is pressed or the bootsplash is to be shown.

I haven't been successful in getting any of the three solutions working satisfactorily, but that was mainly due to lac of time rather than major problems encountered.
This task depends upon

Closed by  Karl Kurbjun (kkurbjun)
Saturday, 21 April 2007, 16:20 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed to svn
Comment by Robert Kukla (roolku) - Thursday, 29 March 2007, 13:35 GMT
Looks good so far. I think I would prefer not to have many different screens displayed during startup. So the transition gigabeat bootloader image to rockbox logo would get my vote (and do away with the bootsplash). So option 1 or 2 I guess.

Did you by any chance look into the making the gigabeat use the 'proper' rockbox format (with checksum and version) instead of a raw image?
Comment by Barry Wardell (barrywardell) - Thursday, 29 March 2007, 15:48 GMT
I agree with you on the bootsplash.

I looked into using the 'proper' rockbox format. It's an easy change to do. The only problem is backwards compaibility. How do we know whether the rockbox.gigabeat we are loading is in the correct format? One suggestion on IRC was to change the extension, but there were no suggestions for what that extension should be. Another option would be to check bytes 4-7 for the model ID. In the 'proper' format, these bytes would be present. In the older files, I guess it would be at least highly unlikely that these 4 bytes would happen to match the model ID. In fact, we should probably do this for all bootloaders anyway.

Either option would be an easy fix to do, requiring only a couple of extra lines of code.
Comment by Tom Ross (midgey34) - Thursday, 29 March 2007, 18:04 GMT
Ha, I was just looking at doing the same thing; I guess you beat me. Anyway, it looks good from what I can see. I haven't tested the code yet since I don't have my Gigabeat near me.

Some comments about the Gigabeat bootloader in general (unrelated to your patch). I'm pretty sure that "Vol+ button to restore original kernel" is slightly too long for the Gigabeat's screen. Perhaps we should reword the message since having it trail off the screen doesn't look very good. Also, is there any reason we have to hold Menu for rescue mode? Is there anything different about Rescue Mode and if not, why don't we treat USB insertion like Bootloader USB mode on other targets (H300 etc)?

I'd also agree with roolku that bootsplash should be removed and the plan for a "proper" Rockbox sounds good to me.
Comment by Robert Kukla (roolku) - Thursday, 29 March 2007, 18:25 GMT
> The only problem is backwards compaibility.

Yes, especially the case of using an old bootloader (which obviously can't be changed) with a new image. But I don't think there is much we can do (apart from renaming the image so it is a "file not found" error rather than a crash, but I consider this cosmetic) and the earlier it gets changed the less people are affected.

> I'm pretty sure that "Vol+ button to restore original kernel" is slightly too long for the Gigabeat's screen.

I don't see the need for any of the strings at all. No other units have them (afaik) and you have to be a fast reader to read them.

The original kernel boot is not working anyway.

> Also, is there any reason we have to hold Menu for rescue mode? Is there anything different about Rescue Mode and if not, why don't we treat USB insertion like Bootloader USB mode on other targets (H300 etc)?

If I understand correctly the rescue mode doesn't work for the X series. So this would result in a crash if they switch the unit on with USB plugged in. But I agree it would be neater without the key press.
Comment by Karl Kurbjun (kkurbjun) - Monday, 09 April 2007, 02:22 GMT
Here is my patch against the bootloader. This closely follows the H320 bootloader. It adds proper Rockbox format support, adds coldstart detection, and is a start on rolo support - rolo does not work reliably yet though, sometimes it loads fine, other times it freezes after it tries to execute. This bootloader goes directly to USB mode if a cable is detected. As a note, it takes a few seconds on my computer for windows to fully recognize the drive when in bootloader USB mode.

I pulled out the code for bootsplash, and copying FWIMG01.DAT.ORIG to FWIMG01.DAT (it's one direction anyway, and if you have applied the minimal zip in the wiki and ran that code then you can't boot your player and have to do the recovery procedure).

This patch also removes the toggling of GPG12 as this pin does not seem to be attached to anything. Eventually I would like to add in full initialization of all the GPIO ports so that we are not relying on a previous state set in the OF bootloader.

It would be great if a Gigabeat X user could try the bootloader USB mode in this patch to make sure everything is ok. I don't believe this code should crash the X series.

Also as a note, I think the gigabeat booatloader should do an all or nothing approach to the proper format. If you are using the new bootloader you need to use a newer build. I don't think that code should be added in for loading an old raw format image - chances are that if you are using a new BL you will be using the latest builds anyway. It is extremely unlikely that an updated bootloader will confuse a raw image for the proper format as well (from what I have seen the images all start with a bunch of zeros anyway which won't pass the checksum). If they are using the old bootloader there's a chance that the new proper images will still run, but if they don't they can go into recovery mode and update their BL.

Let me know if the patch does not apply - I had to pull out alot of other code I'm working on and I may have messed something up.

Comments are greatly appreciated.
Comment by Karl Kurbjun (kkurbjun) - Monday, 09 April 2007, 02:28 GMT
Comment by Karl Kurbjun (kkurbjun) - Tuesday, 10 April 2007, 01:59 GMT
Updated patch, nothing functional changed
Comment by Barry Wardell (barrywardell) - Tuesday, 10 April 2007, 10:00 GMT
Looks good. A couple of questions:

1) What is the purpose of the changes to enable_mmu()?
2) Why do you change map_memory() to memory_init()?
3) Can you explain what coldstart detection does?
Comment by Barry Wardell (barrywardell) - Tuesday, 10 April 2007, 10:04 GMT
Also, here's an updated version of my own patch with a fix for the problem where the lcd showed old framebuffer data when lcd_init() was done. It also moves to the 'proper' format for the rockbox.gigabeat file, removes the code for restoring FWIMG01.DAT and removes the bootsplash.
Comment by Barry Wardell (barrywardell) - Tuesday, 10 April 2007, 10:06 GMT
oops, forgot to attach the patch.
Comment by Karl Kurbjun (kkurbjun) - Tuesday, 10 April 2007, 14:09 GMT
Barry, thanks for the comments.
1) I changed enable_mmu in an attempt to make rolo work more reliably. Right now it invalidates the cache and I was hoping to make sure it didn't destroy any writes to the memory done by rolo. I may move the cache cleaning call to within rolo itself though.

2) I changed it to memory_init as I was playing around with having it called within the system_init function so that the bootloader and the main file(rockbox.gigabeat) were more consistent between eachother. I was doing this because eventually I would like to play around with a rombox type of setup for the gigabeat. Right now I don't think the code is ready to be put in flash, but hopefully one day it will be suitable. I felt that memory_init was a more accurate function name as it's doing a bit more then just mapping memory.

3) Coldstart detection checks if the player is in reset (the pin is low) as was done on the Irivers. It reduces the clicks on startup as the code doesn't think it needs to initialize and reset the drive twice between the bootloader and the main code. I am guessing it is designed for when the player is initially powered on as the GPIO pins should be initially set in high impedance or their function, once they are configured as outputs they may output a zero initially (depending on how the port configuration is done, or if the function output does this already) which is a reset on the ATA lines (in this case it's also connected to the ARESET on the C7C68300 as well)

I am not seeing old FB data displayed on my player with the patch I have. Is it showing on yours? It would be nice if we didn't have to have #ifdefs for bootloaders in the gigabeat port if avoidable for the above mentioned possibility of a rombox for the gigabeat.
Comment by Karl Kurbjun (kkurbjun) - Wednesday, 11 April 2007, 04:46 GMT
Sorry, I have a correction, the coldstart detection was incorrect. It should have been: (GPGCON & 0x00300000) == 0; The proper way is to check the configuration of the port (input, output, func1, or func2). The irivers check the initial configuration of the pin as well so my explanation for 3 is wrong. The default configuration is an input (00 on bits 20 and 21).
Comment by Karl Kurbjun (kkurbjun) - Wednesday, 11 April 2007, 05:54 GMT
Here is the latest patch updated again as I just commited the USB and ATA work. This just has the rolo and bootloader work.
Comment by Karl Kurbjun (kkurbjun) - Wednesday, 11 April 2007, 06:27 GMT
Barry, I was able to reproduce the LCD bug you fixed in your code. I wasn't fully following the change for the new bootloaders, and when I added support for the verbose mode I saw the bug. I really like the code that you did for lcd_init and I think it would be fine to include as I think it makes the bootloader look much better while we are relying on the OF. I've added an updated patch with your lcd_init code. I also added in verbose support to this patch.
Comment by Karl Kurbjun (kkurbjun) - Thursday, 12 April 2007, 00:00 GMT
This patch makes the bootloader USB mode more reliable and faster. I also pulled out the debug menu changes I did and the rolo code as it does not work 100% yet. I think this patch is ready for commit unless anyone has any objections. I will go ahead and commit this tomorrow with a nice warning about updating the bootloader unless something comes up.
Comment by Barry Wardell (barrywardell) - Thursday, 12 April 2007, 20:44 GMT
I had a look at the patch and here's an updated version I've made based off it. The changes I made are:

1) Remove unrelated whitespace change in apps/main.c
2) #include mmu-meg-fx.h in bootloader/gigabeat.c - this also means we can remove the extern declaration of memory_init()
3) Use the SYSFONT_WIDTH and SYSFONT_HEIGHT constants for the USB Bootloader mode message
4) Remove the wait for button press on ATA error. I don't see what purpose this serves. Instead, show the error, then shut down.
5) Remove the wait for USB on disk error. I don't see what purpose this serves. Instead, show the error, then shut down.
6) Use the error() function when there is a problem with the BOOTFILE.
7) Remove the declaration of memory_init() in mmu-meg-fx.c because it's already done in the header.

The only thing I'm wondering about now is the ALARM_MASK. Is that something that will come later?
Comment by Karl Kurbjun (kkurbjun) - Thursday, 12 April 2007, 21:18 GMT
Looks good, I have no complaints on the changes.

I was going to hold off on the alarm_mask until we can actually write code to test against. Right now the OF bootloader interferes with booting from the alarm. The change when it comes also should not be as intrusive as this patch in that users should not have to update the bootloader unless they want to use the alarm clock feature(although they will have to flash). There is still alot of initialization that needs to be written in Rockbox before we should play around with flashing. Right now I've just been working on pin initialization, and I think the ata and usb interfaces are safe, but I don't know about the lcd, the pins that are not part of the ata/usb interface, etc..
Comment by Tom Ross (midgey34) - Friday, 13 April 2007, 04:05 GMT
I've had a look at the latest patch that Barry posted. Everything looks good from what I can see; the code seems straight-forward enough. This seems commit-worthy in my book :) .
Comment by Robert Kukla (roolku) - Friday, 20 April 2007, 15:53 GMT
before this one gets buried on the tracker - are there still issues or why has this not been committed?
Comment by Karl Kurbjun (kkurbjun) - Friday, 20 April 2007, 16:34 GMT
It came up recently that the X series had some problems with a commit I did previous to this one. I wanted to make sure I had those problems sorted out before I went ahead and committed this. The plus side is that I was able to also get this new bootloader tested on the X series before committing. I plan on adding my work to fix the X series and the new bootloader to SVN later today.

Loading...