-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix hardware spinlock IDs on RP2350 #2645
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
base: develop
Are you sure you want to change the base?
Conversation
Use spinlock IDs that are unaffected by E2
src/rp2_common/hardware_sync_spin_lock/include/hardware/sync/spin_lock.h
Show resolved
Hide resolved
@@ -19,53 +19,93 @@ | |||
|
|||
// PICO_CONFIG: PICO_SPINLOCK_ID_IRQ, Spinlock ID for IRQ protection, min=0, max=31, default=9, group=hardware_sync | |||
#ifndef PICO_SPINLOCK_ID_IRQ | |||
#if PICO_RP2350 && !PICO_USE_SW_SPIN_LOCKS | |||
#define PICO_SPINLOCK_ID_IRQ 5 | |||
#else | |||
#define PICO_SPINLOCK_ID_IRQ 9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kilograham In the "original" version of this code, do you remember why the "defined" spinlocks start at 9? I.e. is it actually okay for the "modified" version of this code to use spinlocks 5, 6 and 7?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was getting to comment on this; the main reason is that they are used by pico-extras stuff, so yeah, we want to be careful about what we assign into that range (and pick something that isn't likely to be used at runtime) - i just havent spent any brain cycles picking which, but @will-v-pi you are free to ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In pico-extras, scan video uses 2-5, and audio uses 6&7 - so that would mean starting SDK ones from 10. Note that the pico-extras spin locks will be affected by E2 too, so if we wanted to assign all those safe ones too, the SDK ones would need to start from 19
Alternatively, I could put errors in the ifdefs in pico-extras which these clash with, to say that if on RP2350 and not using software spin locks, you need to define your own IDs for the clashes? I think I’d prefer this option, as it gives the SDK all the spin locks, and the people it affects can pick their own spin locks to use (either SDK bits they don’t use, the OS ones, or from the ranges at the end)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened raspberrypi/pico-extras#99 in pico-extras to throw errors when not using software spin locks, with the error message explaining that you need to define the IDs
Use spinlock IDs that are unaffected by errata E2
This allows use of regular spinlocks rather than software spinlocks, which should fix issues like #2495