QSPI overhaul and QSPI ClockV2

[rnd-ash]

Summary

As part of #912 , this PR overhauls the QSPI module (Only used for ThumbV7 targets) to use the ClockV2 API. In doing so, I have also overhauled the module such that it no longer goes on assumptions like it did before.

New features

• QSPI builder - Set target SPI speed and SPI operation mode
• Use Clock V2 API (user has to pass EnabledGCLK0 to QSPI builder) - EnabledGclk0 is used as QSPI is tied 1:1 to CPU speed, which is tied to Gclk0.
• Add scramble support for QSPI
• Use AHB Token rather than mclk
• Added documentation to the QSPI module on how to use it, and document pitfalls that a user might encounter (Warning of the fact QSPI stalls the CPU so long operations like Erase can make the program freeze)
• Removed QSPI builder functions in the BSPs (Now redundant)
• Migrate pygame QSPI example to Clock::V2

Improvements

• Don't force SPI Mode 0
• Don't force 4Mhz SPI operation by default
• Don't assume the CPU is always running at 120Mhz
• Invalid QSPI freq (more than 1/2 CPU freq) will be caught and returned as an error

Marked as a draft at the moment as examples are broken.

[kyp44]

@rnd-ash I don't know much about the QSPI, but evidently it has two AHB clock buses, CLK_QSPI_AHB and CLK_QSPI2X_AHB, represented in the v2 API by clock::v2::types::Qspi and clock::v2::types::Qspi2x, respectively. Your PR here just requires the AhbClk<Qspi> . Do you know what the AhbClk<Qspi2x> is used for and whether it is needed based on the way the abstraction uses the QSPI?

[rnd-ash]

@kyp44 I had a talk with @bradleyharden about this in Matrix. It seems that 2x AHB clock is only used for DDR mode QSPI. So I added a comment to the QSPI peripheral saying for now DDR is not supported. As the AHB clock system will need to be rewritten to allow the QSPI module to enable/disable the 2x clock at will.

We never supported DDR in the first place with the QSPI module, so we are not loosing any functionality we had before with this PR.

[kyp44]

@rnd-ash Gotcha, it's fine to not offer DDR support at this point. I just wanted the rationale to be documented here.

[rnd-ash]

@kyp44 A thought, I don't have a pygamer. Could you test the QSPI example on it as I know you have one? - I verified this PR works on my own custom board, but that has a different QSPI chip.

Once done, should we merge this to our V2 branch since it doesn't break anything?

[kyp44]

@rnd-ash Sorry for the delayed response, but I was finally able to test this. The first assertion fails, where left side read_buf still contains all zeros.

I also noticed that the pygamer BSP is throwing a warning about an unused import, as well as some various warning in the qspi example itself. I also feel like this import should be combined with this one, and that this import should be moved down with the rest of the hal imports. These are minor nitpicks, obviously.

[rnd-ash]

@kyp44 no problem.

I have updated the code, it should work now, and hopefully removed the warnings in the pygamer example.

The issue was simply down to the QSPI peripheral being clocked too fast. I've now updated the documentation and builder function to prevent someone seeing a QSPI freq of > 0.5x CPU freq

[kyp44]

Ok, so I tested again and at the same assert I no longer get all zeros but it still fails with the left side being [0x40, 0x17, 0xc8] and the right side [0x17, 0x40, 0xc8], that is the first two are swapped. However, if I build with --release the left side becomes [0xc8, 0x40, 0x17], totally reversed. So there may be some weird timing things going here.

I went back and ran the same test with the current master branch and the same thing results. I also tested this with various old HAL releleases all the way back to 0.19.0, and they all panic. I know at one point I verified all the pygamer examples on my hardware, but I think I'd thought that the qspi example worked fine because the LED never came on, but that is because I never enabled the panic-led feature! So I don't think this assertion fails because of anything you did, but rather it never actually worked properly in the first place.

I dug into this a bit. The command being sent to get the JEDEC IDs is Command::ReadId, which equates to 0x9f. According to the GD25Q64C datasheet, this command causes the device to send the Manufacture ID (0xc8 for GigaDevice), the memory type (0x40 for standard NOR), then the memory capacity (0x17 for 128 Mbit) in that order. However, I notice that the latter two are listed as bits 8-15 and 0-7, respectively, of what must be a 16-bit value.

So it's unclear what exactly is going on here or why the two builds produce differerent orders. Any ideas here?

Lastly, if I comment out this JEDEC ID assertaion, the rest of the example executes to completion without an issue in the release build, but somehow is getting stuck in an infinite loop in the debug build. I've seen strange differences between the two builds for other programs in the past, and it's always disconcerting.

[rnd-ash]

Thanks for finding all this out!

I'll do some more testing tomorrow then. I currently have this QSPI branch working such that I can share a fat32 FS over USB to the QSPI chip, so I know read and write and erase work perfectly.

The Chip ID check I think we should remove from this example completely. As it would cause a panic if the pygamer ever came with a different branch of QSPI chip, or if someone replaces the chip. It seems like an unnecessary reason to panic. Maybe we can just print it out instead?

One side note. I'm only ever running my code in release profile. So that's super strange that we get timing issues with debug builds. I think I'll dig some more tomorrow.

Lastly. One thing I think I'll try and do (if it's a reasonable idea) would be to convert the erase command execution in the HAL to firstly switch the QSPI module to normal SPI. This way, erasing does not block the CPU, and can instead be polled.

[kyp44]

No problem. I think probably the order we are getting in the release build is the correct one, which seems to match the order from the datasheet. We could simply check the correct order in the assert, but I'm also not opposed to removing the JEDEC ID check from the example or just printing them out somehow. Printing them to the display would be the best since, like me, I'm sure many people do not have a debugger, but that does require some code to setup the display and print text to it using embedded-graphics. I can handle that part if you want since I do that a lot for my own projects with the PyGamer.

I see that the example does erase the flash chip, so I'll stand by to retest things once you make the changes.