annotate doc/Nucleus-change @ 220:0ed36de51973

ABB semaphore protection overhaul The ABB semaphone protection logic that came with TCS211 from TI was broken in several ways: * Some semaphore-protected functions were called from Application_Initialize() context. NU_Obtain_Semaphore() called with NU_SUSPEND fails with NU_INVALID_SUSPEND in this context, but the return value wasn't checked, and NU_Release_Semaphore() would be called unconditionally at the end. The latter call would increment the semaphore count past 1, making the semaphore no longer binary and thus no longer effective for resource protection. The fix is to check the return value from NU_Obtain_Semaphore() and skip the NU_Release_Semaphore() call if the semaphore wasn't properly obtained. * Some SPI hardware manipulation was being done before entering the semaphore- protected critical section. The fix is to reorder the code: first obtain the semaphore, then do everything else. * In the corner case of L1/DSP recovery, l1_abb_power_on() would call some non-semaphore-protected ABB & SPI init functions. The fix is to skip those calls in the case of recovery. * A few additional corner cases existed, all of which are fixed by making ABB semaphore protection 100% consistent for all ABB functions and code paths. There is still one remaining problem of priority inversion: suppose a low- priority task calls an ABB function, and some medium-priority task just happens to preempt right in the middle of that semaphore-protected ABB operation. Then the high-priority SPI task is locked out for a non-deterministic time until that medium-priority task finishes its work and goes back to sleep. This priority inversion problem remains outstanding for now.
author Mychaela Falconia <falcon@freecalypso.org>
date Mon, 26 Apr 2021 20:55:25 +0000
parents a1799f6d6aa7
children
Ignore whitespace changes - Everywhere: Within whitespace: At end of lines:
rev   line source
54
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
1 The specific integration of ATI Nucleus PLUS RTOS in TI's stable TCS211 fw
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
2 (which served as the baseline for several vendors' production fw) exhibits one
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
3 hair-raising bug. While we don't know for sure where and how they maintained
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
4 Nucleus library sources for compilation (the version we got has them censored
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
5 out), we do see that Nucleus header files (nucleus.h and ??_defs.h) exist in
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
6 two different locations in the source tree in two different versions:
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
7
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
8 * One version exists under chipsetsw/os/nucleus
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
9 * The other version exists under gpf/inc/nuc & gpf/inc/nuc/arm7
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
10
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
11 The two versions of these header files under these two paths in TCS211 are not
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
12 the same! The main nucleus.h header file is the same in both places, cs_defs.h
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
13 and tm_defs.h versions differ only in comments, but tc_defs.h is the real
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
14 kicker: the version under gpf/inc/nuc has an extra field added to the TC_HCB
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
15 aka NU_HISR structure, making this structure one word longer than in the other
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
16 version! More specifically, in ATI's original Nucleus this structure is 22
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
17 words long with 4 unused dummy words at the end; TI's GPF version adds a fifth
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
18 dummy word (thankfully toward the end, not shifting any actually-used members
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
19 of the struct), putting the total struct size at 23 words.
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
20
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
21 It would be one thing if TI had made this change consistently, but they didn't:
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
22 some modules were compiled with one version of the headers and got the 22-word
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
23 version of the struct, while other modules were compiled with the other header
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
24 file version and got the 23-word version of the struct. How can their fw work
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
25 with this bug in it? Answer: TCS211 fw works despite this Nucleus integration
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
26 bug because:
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
27
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
28 * None of the actually-used members of the struct change offsets between the
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
29 two versions;
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
30
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
31 * Some places in the code have 22-word structs allocated in memory while other
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
32 places have 23-word structs, but when they pass pointers to these structs to
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
33 Nucleus API functions, those functions don't access past the actually-used
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
34 part at the beginning (the part before dummy words), and they never do
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
35 anything like zeroing out the full size of the expected struct.
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
36
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
37 * The only place in TCS211 fw where the total size of the struct matters is
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
38 where NU_HISR is embedded in another structure, and there is one such place
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
39 in GPF. Here breakage would result if different modules using these structs
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
40 and arrays were compiled with different header file versions, but all modules
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
41 that touch this part are compiled with the GPF version of nucleus.h, NU_DEBUG
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
42 and tc_defs.h.
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
43
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
44 Needless to say, resolving this bogosity has been an important part of
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
45 FreeCalypso firmware deblobbing. Naturally the most ideal solution would have
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
46 been to remove the bogus extra word added by TI and consistently use the
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
47 original 22-word struct everywhere, but there is one further complication: I
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
48 (Mother Mychaela) don't feel comfortable with moving away from the original blob
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
49 version of the OSL component of GPF, and these COFF objects have been compiled
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
50 with the 23-word version of TC_HCB aka NU_HISR.
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
51
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
52 The following alternative approach has been implemented in FC Tourmaline:
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
53
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
54 * The new source version of Nucleus by Comrade XVilka has been checked in under
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
55 src/nucleus, and this new source version is the one we are using instead of
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
56 TI's binary object version.
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
57
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
58 * The new Nucleus header files src/nucleus/nucleus.h and src/nucleus/??_defs.h
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
59 are the only ones used in Tourmaline - both old versions have been removed
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
60 from active -I include paths.
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
61
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
62 * The new src/nucleus/tc_defs.h header file has been patched to replicate TI's
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
63 23-word version of TC_HCB aka NU_HISR, and the NU_HISR_SIZE definition in
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
64 src/nucleus/nucleus.h has also been adjusted to match.
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
65
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
66 Thus we are using the 23-word version of TC_HCB aka NU_HISR everywhere, with 5
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
67 dummy words at the end rather than 4, adding 4 extra bytes of wasted RAM space
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
68 to every instance of this struct throughout the firmware - but there are only a
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
69 small number of these instances, thus the waste is negligible. In return we
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
70 gain 100% consistency (the same version of the struct is used everywhere in our
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
71 fw), and we retain the ability to keep the original OSL blobs which I am not
a1799f6d6aa7 doc/Nucleus-change article written
Mychaela Falconia <falcon@freecalypso.org>
parents:
diff changeset
72 ready to give up.