search

esp-rs / esp32-hal

A hardware abstraction layer for the esp32 written in Rust.
Apache License 2.0
192 stars 28 forks source link

Enable support for the 2nd core #15

Closed arjanmels closed 4 years ago

arjanmels commented 4 years ago

Adds support for dual core mode

Builds on pull request #11, so that one should be merged first

arjanmels commented 4 years ago

Working nicely, ready to merge after #11 is merged

MabezDev commented 4 years ago

Would you mind rebasing now #11 has been merged?

arjanmels commented 4 years ago

Will do (probably tomorrow)

arjanmels commented 4 years ago

Rebased.

Also added enabling of cache on APP CPU (this is not done by the bootloader as the memory range overlaps). Becomes relevant when running code from flash (#16).

@MabezDev & @Gasper Please note that this pull request also has a couple of changes to efuse.rs slipped in to align the fields to this module (e.g. u32 for core count and Herz units for frequency.

arjanmels commented 4 years ago

Small fixes. Should be good to go!

Gasper commented 4 years ago

I don't think those efuse.rs functions are used anywhere else, so feel free to break them.

I quickly checked the code against reference manual, but haven't tested it yet, I wonder if some of these things might be unintentional:

  1. For the CPU stall you use constant 0x21, while TRM says 0x86: image

  2. You flush the cache before checking if the CPU is already, running ( https://github.com/esp-rs/esp32-hal/pull/15/files#diff-1c70323480a69dae0d91c911d05d952bR125 ). According to TRM, the cache will not be back-written, and will just be cleared, so some cached data could be lost on the running core: image

  3. The start_core function can only be used for APP core, so you could remove that runtime-checked parameter core. This way you could also simplify flush_cache and enable_cache, since they are not public and cannot be called for PRO core. Btw, calling start_core with a main-like function seems very beautiful and user-friendly to me.

arjanmels commented 4 years ago

  1. The 0x86 is divided over bit in the C0 and C1 registers. (upper 6 bits in C1, lower 2 bit in C0: 10000110=0x86, split in: 100001=0x21 10=002).

  2. Flushing before run check: you are right: I will correct.

  3. You are in principle right. I can imagine some future use cases for the flush cache and enable cache for both cores, so I prefer to keep these generic.

start_core is a matter of taste: keep it aligned to other similar functions, or indeed custom for APP_CORE.

The other matter of taste is the core id for the other functions: use a u32, u8, or switch to an enum: the enum can be checked at compile time, but I am also considering future work for the esp32s (single core) or maybe future esp32xxx with who knows how many cores..

As they are a matter of taste, I will take votes and will update accordingly ;-)

Gasper commented 4 years ago

I wonder if they picked 0x86 as some kind of joke: it's quite arbitrary

arjanmels commented 4 years ago

@MabezDev @jeandudey Can this one be merged?

MabezDev commented 4 years ago

Apologies for the delay! This looks good to me :)

arjanmels commented 4 years ago

Thanks!