Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Terminal: add history and auto completion #474

Merged
merged 5 commits into from
May 2, 2020

Conversation

fjullien
Copy link
Contributor

This serie adds history and auto-completion to the BIOS terminal.
The BIOS size increase is very small (about 3K of ROM and 1K of RAM).
This serie needs some testing as I wasn't able to test all available commands.

@mithro
Copy link
Collaborator

mithro commented Apr 26, 2020

@fjullien - This seems like a cool feature! However, as the BIOS is frequently embedded in blockram in the gateware an increase of 3K ROM and 1K RAM will be too big for many targets (specially for targets like the iCE40 which only have tiny amounts of space). If this feature can be optionally enabled then we should most certainly merge it.

You might be able to reduce the size quite a bit by using link time optimization (LTO). @mateusz-holenko was working on getting that re-enabled in LiteX. I'm sure he would love help working on that.

Copy link
Collaborator

@mithro mithro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor code comments, otherwise I like the way this is heading a lot!


/**************************************
* Ethernet boot *
**************************************
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like that this kind of structure also leads the way towards liteeth and litedram providing the BIOS features as compile time plugins rather than it all being in stuck in the litex repo.

// Largely inspired/copied from U-boot and Barebox projects wich are:
// Sascha Hauer, Pengutronix, <s.hauer@pengutronix.de>

// License: BSD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is mostly there, could you use an SPDX identifier tag here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@fjullien
Copy link
Contributor Author

If this feature can be optionally enabled then we should most certainly merge it.

I'll work on this.

@fjullien fjullien closed this Apr 26, 2020
@fjullien fjullien reopened this Apr 26, 2020
@fjullien
Copy link
Contributor Author

fjullien commented Apr 26, 2020

If this feature can be optionally enabled then we should most certainly merge it.

Here is numbers:

Original:

ROM usage: 18.07KiB     (56.47%)
RAM usage: 0.30KiB      (7.42%)

Complete + hist:

ROM usage: 21.73KiB     (67.90%)
RAM usage: 2.23KiB      (55.66%)

----- Every new case feature is additive -----

Without complete:

ROM usage: 20.58KiB     (64.31%)
RAM usage: 0.94KiB      (23.44%)

Without history:

ROM usage: 20.08KiB     (62.74%)
RAM usage: 0.30KiB      (7.42%)

struct member .func point to function directly:

ROM usage: 19.77KiB     (61.79%)
RAM usage: 0.30KiB      (7.42%)

Terminal does no handle special keys:

ROM usage: 19.00KiB     (59.38%)
RAM usage: 0.30KiB      (7.42%)

Use old readline (readstr) conditionally but still use new parser and cmd struct:

ROM usage: 18.64KiB     (58.25%)
RAM usage: 0.30KiB      (7.42%)

What degree of granularity do we want in the configuration ?
We can easily have all.

I need to do some testing before I submit a v2 of this PR.

@mithro
Copy link
Collaborator

mithro commented Apr 26, 2020

@xobs and @mateusz-holenko probably have a better understanding of what sizes we should be targetting.

@enjoy-digital
Copy link
Owner

Thanks @fjullien for this great contribution! Having auto-complete and history will be very useful. We should indeed make this configurable. I'm not sure we need too much granularity, maybe just being able enable auto-complete and history separately.

@mithro, @mateusz-holenko, @xobs: that would indeed be useful to define the constraints on the different targets for the BIOS ROM/RAM usage to avoid being too restrictive on the additional features. Since the BIOS can be used on iCE40 to Ultrascale+ FPGAs, even if we try to be efficient on all targets, "big" and "small" are still a bit relative to the FPGA.

I'm not sure for example to be well aware of the constraints on very small FPGAs, since from my understanding, on iCE40 the available ROM memory is already too small to fit the current BIOS and when the SoC is using the LiteX BIOS it is always stored in SPI Flash and executed from there. So in this case the constraint seems more related to the RAM and the ROM size is not really an issue? (it could be if we wanted to maximize speed in the BIOS, but i don't think it's the case).

The BIOS can also be used for different use cases: as the main bootloader, as a first stage bootloader before a proper bootloader (like Barebox for example), just to initialize some peripherals with no user interaction (LiteDRAM generated in standalone for example), etc... so a next step would maybe to have flags to enable/disable features more easily.

@xobs
Copy link
Collaborator

xobs commented Apr 27, 2020

The BIOS seems to be useful for the same reasons that a PC BIOS is useful: Initializing RAM and loading the main program. However, even PC BIOSes nowadays have full GUI environments.

I believe we talked about this in an earlier patch, but U-Boot has a concept of the SPL whose job it is to initialize memory and then load the rest of U-Boot. It's designed to be relatively small, whereas U-Boot itself is relatively big.

Overall I find the BIOS has four uses:

  1. Initialize DDR
  2. Act as a rudimentary development environment (with peek and poke)
  3. Load programs, either off SPI or over serial
  4. Provide that "A-ha!" moment when people first port litex to their platform and see the banner

Once you have a payload such as linux or zephyr then items (2)-(4) become less useful. This patch helps with item (2).

On ice40 we skip (1) because RAM doesn't need to be initialized, and (2)-(4) are not as useful on a shipping product, which is why I very much appreciate being able to remove the BIOS.

So ultimately I think this is a good thing, because it definitely helps with (2) and to a lesser extent (4).

@mateusz-holenko
Copy link
Collaborator

@fjullien Size reduction we where able to obtain aftert enabling LTO is presented in #253. We had problems with getting rid of -lgcc flag and got it working at some point with #401.

Unfortunatelly it turned out that the solution was not versatile enough and didn't work in all cases, so it was reverted.

@fjullien fjullien force-pushed the term_hist_auto_compl branch from 6d7345b to 6643bb9 Compare April 29, 2020 20:00
@fjullien
Copy link
Contributor Author

I added an optional parameter to Builder in order to pass options to the BIOS.
This way, we can configure what termial feature we want. For example:
builder_kwargs["bios_options"] = ['TERM_MINI']
So we have:

  1. TERM_MINI -> the smallest one
  2. TERM_NO_COMPLETE standard terminal with no command completion
  3. TERM_NO_HIST standard terminal with no history

@enjoy-digital
Copy link
Owner

Thanks for the update @fjullien, we'll review it soon.

@fjullien fjullien force-pushed the term_hist_auto_compl branch from 6643bb9 to 895a276 Compare May 1, 2020 09:43
fjullien added 5 commits May 1, 2020 12:10
Terminal history and characters parsing is done in readline.c.
Passing TERM_NO_HIST disable terminal history.
Passing TERM_MINI use a simple terminal implementation in order to save
more space.
Command are now described with a structure. A pointer to this
structure is placed in a dedicated linker section.
@fjullien fjullien force-pushed the term_hist_auto_compl branch from 895a276 to 74dc444 Compare May 1, 2020 10:19
@enjoy-digital
Copy link
Owner

I think all @mithro's comments have been addressed and i've been able to test it successfully in simulation and on hardware, so think we can merge now. We'll maybe have to discuss the default settings depending the use cases/targets, but otherwise this is really cleaning up and improving the BIOS, thanks for this work @fjullien!

@enjoy-digital enjoy-digital merged commit 705d388 into enjoy-digital:master May 2, 2020
@fjullien fjullien deleted the term_hist_auto_compl branch September 17, 2021 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants