diff --git a/src/hal/hisi/hal_hisi.c b/src/hal/hisi/hal_hisi.c index 075eaa6..1a96dd1 100644 --- a/src/hal/hisi/hal_hisi.c +++ b/src/hal/hisi/hal_hisi.c @@ -485,13 +485,25 @@ static void sensor_crg_enable( const uint32_t want = (cur | cken) & ~srst; if (want == cur) { - /* Already fit for a probe -- but by whose hand? If the register still - * holds exactly the word WE left, this is our own ungate being re-armed - * mid-sweep (setup_hal_hisi() arms, then arm_sensor_clock() arms again - * before every bus), and the undo we owe is still owed. Anything else - * is somebody else's clock and rule 3 disowns it. */ - if (!(st->owed && cur == st->wrote)) - st->owed = false; + /* Already fit for a probe, so this arm takes nothing -- and drops + * anything an earlier one held. + * + * UNCONDITIONALLY, and that is the whole point. It is tempting to keep + * the entitlement when the register still holds the word we wrote, on + * the grounds that this is our own ungate being re-armed mid-sweep. + * That was tried and it put the original bug straight back: the word + * we write is 0x11 and 0x11 is also what the vendor SDK writes, so an + * arm at boot that ungated a gated clock, followed by the consumer + * starting its pipeline, followed by a probe half an hour later, looks + * from this register exactly like our own ungate still standing. The + * cleanup then handed a streaming camera's clock back to the gated + * state. It reached the field before it was caught. + * + * The cost is that a probe which ungated a gated clock and then armed + * again may leave it running. That is deliberate: of the two ways to + * be wrong, a clock left on costs microamps and a clock taken away + * costs the picture. */ + st->owed = false; return; } diff --git a/src/hal/hisi/hal_hisi_test.c b/src/hal/hisi/hal_hisi_test.c index c4063b1..832a282 100644 --- a/src/hal/hisi/hal_hisi_test.c +++ b/src/hal/hisi/hal_hisi_test.c @@ -218,23 +218,25 @@ int main(void) { hal_cleanup(); CHECK(writes == before); - /* Qodo #2, under test before it is believed: setup ungates a gated clock, - * the sweep arms again before the first bus, and the cleanup must still - * put back what setup found. */ + /* A second arm drops what the first held, and the clock is LEFT RUNNING. + * Pinned as the deliberate trade it is, not left to drift: keeping the + * entitlement across arms so the restore still fires is what reopened the + * field bug, because our own ungate and the consumer's are the same word. + * A clock left on costs microamps; one taken away costs the picture. */ fresh(V4, 0x10, HISI_V4); setup_hal_hisi(); /* arm 1: 0x10 -> 0x11, entitled */ - hal_enable_sensor_clock(); /* arm 2: already fit */ + hal_enable_sensor_clock(); /* arm 2: already fit, disowns */ hal_cleanup(); - CHECK(peek(V4) == 0x10); + CHECK(peek(V4) == 0x11); CHECK(illegal == NULL); - /* Same, on both OT registers. */ + /* Same on both OT registers. */ fresh(OT, 0x00, HISI_OT); setup_hal_hisi(); hal_enable_sensor_clock(); hal_cleanup(); - CHECK(peek(CV610_PERI_CRG8464_ADDR) == 0x00); - CHECK(peek(CV610_PERI_CRG8472_ADDR) == 0x00); + CHECK(peek(CV610_PERI_CRG8464_ADDR) & CV610_PERI_CRG_SENSOR0_CKEN); + CHECK(peek(CV610_PERI_CRG8472_ADDR) & CV610_PERI_CRG_SENSOR0_CKEN); /* Qodo #1: a read we could not make must not leave an entitlement from an * earlier arm lying about for the cleanup to spend. */ @@ -275,6 +277,22 @@ int main(void) { CHECK(peek(V4) == 0x11); CHECK(illegal == NULL); + /* THE FIELD CASE, and the one that matters most: an arm happens while the + * clock is gated and no probe follows it, the consumer then starts its + * pipeline and writes the very same word we did, and a probe arrives half + * an hour later. The entitlement from that first arm must not still be + * live, or the cleanup hands the consumer's running clock back to the + * gated state. Reported from the field after an earlier fix to the + * mid-sweep case made the entitlement outlive the probe it belonged to. */ + fresh(V4, 0x10, HISI_V4); + setup_hal_hisi(); /* boot-time arm: 0x10 -> 0x11, entitled */ + poke(V4, 0x11); /* the SDK brings the pipeline up: same word */ + reset_counters(); + sweep(); /* the telemetry probe, 30 minutes later */ + CHECK(writes == 0); + CHECK(peek(V4) == 0x11); + CHECK(illegal == NULL); + /* A register somebody else reprogrammed between our ungate and our undo * is theirs now. */ fresh(V4, 0x10, HISI_V4);