chore: Minor QSPI code deduplication

[aapoalas]

Since this caught my eye in #2089 and was merged in #2071, I thought I'd put my hands where my mouth is and do the few deduplications that I thought might be considered useful or "pretty".

Adjacent code error?

In drv/auxflash-api/src/lib.rs:191 the call to self.get_blob_by_tag(tag) seems to be invalid; that function takes two parameters, (slot, tag). I wonder if this is dead code or what's going on with that?

[labbott]

The diff stat on this is +136 −93 i.e. a net increase of code. That's a hard sell for code deduplication.

[aapoalas]

The diff stat on this is +136 −93 i.e. a net increase of code. That's a hard sell for code deduplication.

A good bit of that is coming from the added and/or copied comments, but that's absolutely true.

My thinking was that getting rid of superfluous/"not functionality wise critical" error conversions would make the code a bit less "noisy", and perhaps also improve generated code quality / icache usage. I didn't check the output though; I'll take a look at that to see if this actually has other upsides than just personal code esthetics :)

The changes in the STM32 QSPI driver code I do think pulls its own weight, regardless.

[aapoalas]

After deeper analysis, the helper methods were a total dud. The STM32 QSPI driver changes are less clear but a possible dud as well. Here're the numbers up front:

• write_impl changes from 306 bytes to 250 bytes. (measured on cosmo-a auxflash and gimlet-f hf)
• read_impl changes from 302 bytes to 246 bytes. (same sources; auxflash had a slightly smaller size both before and after, probably +/-0 difference)
• get_fifo_level is an added 60 bytes.
• wait_for_transfer_complete is an added 72 bytes.

In total, that's a 20 byte increase in assembly size. Binary size of cosmo-a auxflash increases by a whopping 2868 bytes. This is a loss in total, but perhaps it is acceptable given the actually negative number of code lines? Applying #[inline(always)] can of course be done to (mostly) return to status quo ante bellum (though there's still a binary size increase of 276 bytes for cosmo-a auxflash after this).

Final side note: disable_all_interrupts is actually being done unconditionally no matter how read/write_impl_inner ends; the source code could optionally be simplified to reflect this as well.

[aapoalas]

@labbott Would you be willing to re-review this?

I removed the .map_err(qspi_to_auxflash) helper functions that I added as they did not improve code generation (using #[inline(always)] fixed this), and as per your previous review you didn't consider them helpful from a readability standpoint. I also tried to remove the singular helper function like this that already exists: this is the fn read_qspi_status in gimlet-hf-server's ServerImpl. Removing that increased code size. (I assume adding such a helper to auxflash-server's ServerImpl would reduce code size a little, since it has very much the same code structure as gimlet-hf-server.)

EDIT: I checked; adding the read_qspi_status helper to auxflash results in a 28 byte code size reduction.

The get_fifo_level and wait_for_transfer_complete helper functions that I added previously are now basically the only thing that this PR creates, and those are also set #[inline(always)] as without that, the diff resulted in a code size increase. I think they're still code structure wise worthwhile.

I additionally moved the disable_all_interrupts calls to the read_impl / write_impl functions; the Ok-path for the inner functions would end with a disable_all_interrupts call (or equivalent), and the outer function would check if the result is Err and call disable_all_interrupts; ie. both paths always end up disabling all interrupts. The compiler understood this, so this doesn't cause changes in the final executable with one exception: the read_impl grew by 4 bytes. I believe that this is because the code previously skipped disabling one interrupt due to knowing it's statically already, and I replaced that with the generic disable_all_interrupts to make the code "simpler" and match write_impl_inner's Ok-path end.

[aapoalas]

note: So here the code previously cleared just TCIE, TEIE, and TOIE. Now the Ok-path goes into disable_all_interrupts, clearing one more register which then causes a 4 byte increase in code size.

I would've actually thought the compiler could combine the error and success paths here but that doesn't seem to be the case; the 4 byte increase suggests that the Ok-path is independent of the Err-path, even though they both do the same thing.