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

crt0.S: Do not clear XIP control registers except in bootloader mode #318

Merged
merged 3 commits into from
May 20, 2022

Conversation

jpf91
Copy link
Contributor

@jpf91 jpf91 commented May 19, 2022

When the bootloader executes an application from XIP, the startup code should not disable the XIP unit, otherwise the application will crash.

@stnolting
Copy link
Owner

stnolting commented May 19, 2022

@jpf91 in #317:

The application startup code clears all registers in the peripheral space. This disables the XIP unit, which is of course a problem if the application is executing from XIP.

@stnolting stnolting added enhancement New feature or request SW software-related labels May 19, 2022
@stnolting stnolting self-requested a review May 19, 2022 16:47
@stnolting
Copy link
Owner

stnolting commented May 19, 2022

Nice fix! 👍 😉
But somehow this feels like a very application-specific "workaround".

I think this is a general problem - not limited to the XIP module. If you configure any IO/peripheral device in the bootloader and then jump to another executable, all the modifications will be lost as the other executable will re-run the IO reset loop in crt0.

Maybe it would be better to have a general "do or do not IO reset" switch for the start-up code. For example we could add something like this around the IO reset loop:

#ifndef NO_IO_RESET

__crt0_reset_io_loop:
  sw   zero, 0(x8)
  addi x8,   x8, 4
  bne  x8,   x9, __crt0_reset_io_loop

#endif

Then we can switch off the reset loop while invoking the makfile by defining NO_IO_RESET via the USER_FLAGS variable:

$ make USER_FLAGS+=-DNO_IO_RESET clean_all exe

What do you think about this? 🤔


@GideonZ maybe this is also interesting for you ;)

@jpf91
Copy link
Contributor Author

jpf91 commented May 19, 2022

What do you think about this?

Works for me as well :-) Not sure whether we'd have to reset the UART in the bootloader then though...

@stnolting
Copy link
Owner

stnolting commented May 19, 2022

Do you might want to update this PR (or open a new one)? 😉
I would take care of updating the documentation.

Not sure whether we'd have to reset the UART in the bootloader then though...

I am not sure what you mean here 😅
The UARTs would be always reset by crt0 (which in case of the bootloader would always do a full reset of all IO modules) and configured by the bootloader itself. If the actual application (that is called by the bootloader) requires a different UART configuration, it can just redo the setup.

@jpf91
Copy link
Contributor Author

jpf91 commented May 19, 2022

I am not sure what you mean here sweat_smile

I was just thinking that the bootloader uses the UART, so it stays initialized after the user application is started. But I guess that should not really matter. As you say, it will be reinitialized anyway when the application uses it.

Do you might want to update this PR (or open a new one)?

Sure. I'll have to delay this until tomorrow though.

By the way: What should be the default? Reset enabled or not enabled? Or maybe default to enabled for the bootloader and default to disabled for applications?

@stnolting
Copy link
Owner

Sure. I'll have to delay this until tomorrow though.

Thanks! 👍

By the way: What should be the default? Reset enabled or not enabled? Or maybe default to enabled for the bootloader and default to disabled for applications?

I think the default should be the current situation, so resetting IO is always enabled - unless it is explicitly disabled by the user.

@stnolting stnolting marked this pull request as draft May 19, 2022 18:07
@stnolting stnolting removed their request for review May 19, 2022 19:46
@jpf91 jpf91 force-pushed the jpf91-keep-xip branch 2 times, most recently from 3c9714c to 105fd9f Compare May 20, 2022 08:21
@jpf91
Copy link
Contributor Author

jpf91 commented May 20, 2022

I've updated the PR as discussed. I also tested this with the hello_world example and it's working as expected.

Maybe it makes sense to include a note in the documentation that make clean_all needs to be run when initially starting to use this flag. this initially confused me slightly :-)

@stnolting stnolting marked this pull request as ready for review May 20, 2022 09:06
sw/common/crt0.S Outdated Show resolved Hide resolved
@stnolting
Copy link
Owner

I've updated the PR as discussed. I also tested this with the hello_world example and it's working as expected.

🚀

Maybe it makes sense to include a note in the documentation that make clean_all needs to be run when initially starting to use this flag. this initially confused me slightly :-)

clean_all removes all object files, which is required here because we need to recompile crt0. I thought this was obvious 😅 but I will add some notes to the documentation regarding this.

jpf91 and others added 3 commits May 20, 2022 11:54
Define NO_IO_RESET to disable the peripheral reset in the early startup
code. This is useful when running code from XIP flash, as we do not want
to reset the XIP controller in that case.

Note: Make sure to run "make clean_all" if you changed this flag.
Usage: make USER_FLAGS+=-DNO_IO_RESET exe
Compile-time switch to turn off the default "software reset" of all IO/peripheral devices executed by the crt0 start-up code.
@jpf91
Copy link
Contributor Author

jpf91 commented May 20, 2022

I thought this was obvious

It kinda is, it was the first thing I've tried when make clean didn't work :-)

@stnolting stnolting merged commit ece84c1 into stnolting:main May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request SW software-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants