earlgrey: Properly ack flash operation done interrupt

[jesultra]

Delay ack'ing of interrupts with the PLIC until the handling routine has run, and has had the opportunity to address the situation with the IP, to cause interrupt request to be deasserted.

Before this change, the PLIC would re-latch the interrupt immediately after it being ack'ed, as the interrupt request was still active at the flash IP. As a result, next flash operation would erroneously be considered done immediately.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jesultra / name: Jes B. Klinke (ea33d2e)

[jesultra]

With this change, my Teacup board is able to copy an image from external flash to slot B in the internal flash. I suppose the existing code worked on FPGAs only because the simulation of writing to internal flash must be unrealistic, in the sense that it completes instantly, thereby hiding the improper handling of flash operation completion interrupts.

After the transport image has flashed the slot B, we expect the new image to boot.  On my board, however, the same transport image boots again (and attempts flashing again repeatedly.)
I guess I have a few things to do:

1. Understand why the new image does not boot, this could be either because it is improperly signed/built, or because version numbers or some other header field causes ROM_EXT to choose the transport image instead, or I suppose, it could be that the transport image fails to make the requests to ROM_EXT to boot the new image once.  I assume that is what we want, booting it once.  Then the new image should be responsible for telling ROM_EXT to permanently switch to booting the new image, after it has successfully booted.

2. We should have a guard against wearing down the internal flash by repeated writing of the same image.  Would it be too much to compare the content if the internal flash slot B with the content of the external flash, and only write the pages that differ?

3. I think the proposal above leaves a tiny tiny gap.  If power is lost while writing the last flash page, I assume that it could happen that the flash write did not have quite enough time to build up/drain charges from the storage capacitors, and worst case, the data will read correctly the next few days, but fail before the rated retention time, because the programming did not adhere to specification.  To guard against something like that, we need to keep a "transaction log" in flash elsewhere (in slot A), and only after writing the last page of slot B, we write a log entry stating that flashing completed (with a checksum of the written image probably.)  Then on next boot, if we see such a record (even if written incompletely) we know that the slot B image was written properly, and can skip re-writing if the checksum in the record matches the checksum of the image in external flash. Such a transaction log would also eliminate the need to calculate checksum of the internal flash when deciding whether to skip re-writing, thereby avoiding a slowdown to the "happy case".

[jesultra]

About item 1 on my list above, I have found my issue to be that I had incorrectly signed the image on the external SPI flash. With that fixed, the transport image was able to flash and boot the next image. Though I discovered then that the next time, the transport image under test is not loaded correctly by "opentitantool rescue firmware" (ROM_EXT continues to boot slot B) due to a missing cherry-pick:
lowRISC/opentitan#25887

Item 2 and 3 remain.

[jesultra]

I have noticed one more issue directly related to the flash driver. Some operations in eflash_driver.rs are asynchronous, relying on the caller to call complete_op() in response to an interrupt, but read() is synchronous, which is fine. But when read() runs, it will nevertheless trigger an interrupt request, meaning that if start_erase() or start_program() is subsequently called, the interrupt will have already latched, and complete_op() will be called prematurely, resulting in an error when complete_op() inspects the status.

[cfrantz]

I think it might be better to have a specific trait for notification acknowledgement and declare that as the bound rather than Drop:

trait BlockingAck {
  fn ack(&self);
}

We can still implement Drop on the type implementing BlockingAck:

struct InterruptAckToken { ... }

impl BlockingAck for InterruptAckToken {
  fn ack(&self) { let _ = syscall::interrupt_ack(...); }
}

impl Drop for InterruptAckToken {
  fn drop(&mut self) { self.ack(); }
}

Also, I think we want to keep the trait definitions separate from the implementations. We want to eventually get to a point where there can be non-pigweed OS uses of some of the Earlgrey driver code.

[jesultra]

I get your point about keeping the traits separate from the structs that implement on top of Pigweed. Can you suggest which directory should depend on Pigweed and which should be for the generic traits? (I just blindly added under util.)

About the Drop vs. explicit ack() I am not sure I agree. It would be confusing if traits that implement Blocking can choose whether the ack() is automatic or not. If the BlockingInterrupt is implemented for some kernel besides Pigweed, we do not want to have to edit hal/blocking/flash/flash.rs to possibly call ack() explicitly. I think we should either say that ack() is always needed, or that it is always done through Drop. Or am I missing something?