[sw/bootloader] add TWI boot option

[LukasP46]

Add ability to read program from TWI (also known as I2C) memory like common EEPROMs with one address byte.

[LukasP46]

The bootloader gets to big if all options are enables. How should we handle this? Maybe disable TWI by default but add an additional test where SPI is disabled instead?

(I'm gonna extend the documentation when we resolved this)

[stnolting]

Add ability to read program from TWI

Nice! Thank you very much!

The bootloader gets to big if all options are enables.

Oh I know this problem... 🙈
How much more ROM do you need for your modified bootloader?

How should we handle this? Maybe disable TWI by default but add an additional test where SPI is disabled instead?

I was thinking about rewriting the bootloader in plain assembly... But to be honest, I'm too lazy for that... 😅

So yes, we could disable the TWI option by default and let the user decide if this is a relevant feature.

[LukasP46]

How much more ROM do you need for your modified bootloader?

Memory utilization:
   text    data     bss     dec     hex filename
   4456       0       8    4464    1170 main.elf

I was thinking about rewriting the bootloader in plain assembly... But to be honest, I'm too lazy for that... 😅

too lazy not crazy
Surely it can be done, but it sounds really time consuming and rather hard to debug (and contribute to).

I actually use the C ISA extension so the bootloader is always small enough, but I guess I'll change the default and then the user can enable it if needed. I would actually suggest changing the warning as well and recommend recompiling the bootloader for each processor to enable a potentially smaller bootloader, what do you think?

[LukasP46]

While writing to the TWI memory via the bootloader is not planned, I will probably upload a Python script to flash via an USB I2C adapter.

[stnolting]

I actually use the C ISA extension so the bootloader is always small enough

That's a good solution! But I would recommend to provide the pre-built bootloader image compiled with the minimal ISA options only - so it is compatible to any ISA configuration.

I would actually suggest changing the warning as well and recommend recompiling the bootloader for each processor to enable a potentially smaller bootloader, what do you think?

Which warning?

Sure, we could add a note/tip to the according section of the documentation for this.

Btw, we have already enabled link-time-optimization and Os as compile-time switches. I am not sure if there is anything more we could do to further shrink the bootloader.

However, maybe it is worth a try to compile the bootloader with the absolute minimal ISA configuration which is rv32e. The E ISA extension only provides 16 x registers. Maybe the overhead of register spilling could be reduced by this?! 🤔

While writing to the TWI memory via the bootloader is not planned, I will probably upload a Python script to flash via an USB I2C adapter.

I think we could re-use parts of the "SPI write" code?

I just scanned your code briefly, but I think you are using a fixed number of address bytes for accessing the TWI flash, right? Maybe we could also make this configurable just like we did for the SPI accesses.

[LukasP46]

That's a good solution! But I would recommend to provide the pre-built bootloader image compiled with the minimal ISA options only - so it is compatible to any ISA configuration.

I agree, the pre-built should be compatible with any (but E currently) ISA configuration. But we could encourage the user to build the bootloader to their needs and ISA.

Which warning?

This one in bootloader.c:

/**********************************************************************//**
 * Sanity check: Base RV32I ISA only!
 **************************************************************************/
#if defined __riscv_atomic || defined __riscv_a || __riscv_b || __riscv_compressed || defined __riscv_c || defined __riscv_mul || defined __riscv_m
  #warning In order to allow the bootloader to run on *any* CPU configuration it should be compiled using the base rv32i ISA only.
#endif

But actually I'm fine with it, because the first part says to allow, which actually implies if you need it...

Btw, we have already enabled link-time-optimization and Os as compile-time switches. I am not sure if there is anything more we could do to further shrink the bootloader.

Probably not? -Oz is even more aggressive, but is currently the same size as -Os.

However, maybe it is worth a try to compile the bootloader with the absolute minimal ISA configuration which is rv32e. The E ISA extension only provides 16 x registers. Maybe the overhead of register spilling could be reduced by this?!

I'll look into it. This would also have the advantage that the bootloader would actually work on all ISA configurations.

I think we could re-use parts of the "SPI write" code?

Probably yes? Unfortunately I don't have a use case at the moment, but I keep it in mind. Probably not in this PR though. Some other bootloader extensions have a higher priority, you'll see :)

I just scanned your code briefly, but I think you are using a fixed number of address bytes for accessing the TWI flash, right? Maybe we could also make this configurable just like we did for the SPI accesses.

I agree, I'll add that one!

[stnolting]

I agree, the pre-built should be compatible with any (but E currently) ISA configuration. But we could encourage the user to build the bootloader to their needs and ISA.

👍

This one in bootloader.c:

Oh, right. I forgot about that...

I think we could remove that. It does not capture all possible ISA extensions. So basically it is useless.

Probably not? -Oz is even more aggressive, but is currently the same size as -Os.

Interesting! I was not aware of that option. I'll look it up.

I'll look into it. This would also have the advantage that the bootloader would actually work on all ISA configurations.

I just tested that and it saves more than 100 bytes (yeay!).

-> #1118

I agree, I'll add that one!

Cool, thanks!