FlashROM improvements and ASCII16-X mapper support #1620
Conversation
|
Test flash program + ROM: ascii16x-test.zip Invoke with e.g.: And then on the MSX-DOS2 command line: To query the mapper in slot 2 for CFI information, do a continuity check, erase, program and verify. (Also works when changing the flash chip to M29W640GB btw.) |
|
I notice GCC and MSVC are having spurious complaints that Clang doesn’t fuss over: GCC: These are MSVC: I wonder if the un-fun complaints about mixing initializers is an MSVC limitation, or maybe these errors (warnings?) could be silenced? Alternatively I could be explicit about the initializers, though it’d be uglier since it duplicates like [Edit: SonarCloud says “mixture of designated and non-designated initializers in the same initializer list is a C99 extension”.] Not sure why it complains about constexpr object initialisation, maybe as a result of the preceding errors. MacOS build fails as usual for pull requests because the signing key is absent (seems unnecessary / fixable). |
|
In c++ it's indeed not allowed to mix designated initializers with unnamed (order-based) initialization. Additionally in c++ (not in C), even when you use designated initializers, the fields must be enumerated in declaration-order (but you can skip fields). In C you can enumerate them in an arbitrary order (and even do crazy stuff like initialize the same field multiple times (only the last assignment will have an effect)). So the fix is indeed to also use designated initializer syntax for the first field (even if that might be a bit redundant). |
|
Hi Grauw, Thanks a lot for this PR. I'll try to review later today. What kind of review do you expect (e.g. how detailed .. how close to 'finished' is the code)? So far I only did a very quick read of the commit messages. Seems like all but the last commit make improvements to the flash-emulation code, only the last commit adds a new mapper. But I am curious about that last one. What's the context of that new mapper? Is it new hardware you're developing? For some new project? Why a new mapper type? E.g. wasn't a mapper like NEO-8 or NEO-16 suitable? (Not saying you should change anything, just curious). |
|
Hi Wouter, thanks,
In principle this code is finished, so a full review would be great. I may add additional functions later but in principle this is as much as needed for me to be able to develop and test the flashing and game software in openMSX.
Correct!
Yes it’s hardware I’ve developed since the beginning of this year. It will be used for Usas 2 (announced @ GOTO40) and possibly another project as well. The extra capacity is the headline feature, because we wanted to use a ROM larger than 4 MB. It will be open source hardware. I will announce the mapper on msx.org soon with a number of them available for developers, and people will be able to order production batches pre-assembled from PCBWay.
I’ve been discussing with Aoineko whether NEO could be implemented with plain logic ICs, or what kind of modifications it would need for that, and I made a rough schematic outlining what would be possible and what not. However we concluded that NEO could only be practically implemented using FPGA, and it would be better to not limit it by what could be done with plain logic. I continued to develop this design as the ASCII-X mapper, through several prototypes, and now it’s about ready.
Yeah the order thing is also a bit unfortunate since struct definition order for the most efficient packing may not be the most intuitive order to group things for reading, so there’s a bit of conflicting interest. |
Interesting! After your discussions, do you happen to know the status of those NEO-x mappers?
I agree with you. C++ does not have a good solution to decouple the human-logical-reading-order from the in-memory-order for struct-fields. On the other hand I do also understand the reasoning behind this limitation. The main difference between C and C++ is that C does not have constructors/destructors ... In general there may be dependencies between the different fields of a struct: construction of one field may already make use of the previous (already constructed) other fields in the same struct. Also in general, for the same reason, it's a good idea to destruct the fields in reverse order of construction. A class/struct can have many different constructors. For a moment suppose it would be allowed to construct the fields in an arbitrary order in each constructor. An object does not remember via which constructor it was constructed, so the (only) destructor can't know this (arbitrary) construction order, and then it can't reverse this order to destruct the fields. To get around this problem c++ always constructs the fields in declaration order, and destructs in reverse declaration order. The above paragraphs talked about constructors, but in a way initializing a struct via designated initializers can have the same problems. But I agree that in practice this is (more) rare. In many cases the fields in a struct are independent, and then it would be useful if they could be initialized in a arbitrary order. |
Hello, The status of the NEO mapper is still the same:
I think it's a pity that we haven't managed to create a format that suits everyone needs (there's even a Japanese guy working on a 3rd 16-bit mapper format), but Grauw's project has a better chance of success: he can create the hardware himself (whereas I can't), and USAS 2 has a much better chance of seeing the light of day than any of the current NEO mapper projects. So if openMSX has to choose one, I wouldn't be shocked if it were ASCII16-X. :) |
Thanks for the update.
It's not that we have to choose only one. If having the NEO-x mappers in openMSX helps then there's no problem to keep them (next to ASCII 16-X). |
There was a problem hiding this comment.
AmdFlash: Support CFI query command.
Implementation looks sane, but I can't really review in detail (i haven't checked it against the datasheet).
I was only wondering: many of the values returned from peekCFI() are 8-bit constants while the method itself returns a 16-bit result. E.g. supply.minVcc is uint8_t. But then currently none of the current chip definitions specifies a non-default (non zero) value for these constants.
| // first 16kB: 0x6000 - 0x6FFF, 0xA000 - 0xAFFF, 0xE000 - 0xEFFF, 0x2000 - 0x2FFF | ||
| // second 16kB: 0x7000 - 0x7FFF, 0xB000 - 0xBFFF, 0xF000 - 0xFFFF, 0x3000 - 0x3FFF | ||
| // | ||
| // 12-bit bank number is composed by address bits 8-11 and the data bits. |
There was a problem hiding this comment.
Not a comment on this PR, but a comment on the mapper design.
Using 4 address bits to "extend" the data-bus to 12-bits is a bit unusual. And unexpected (to me). From a hardware point of view I understand how this could indeed lead to a simpler design. From a software point of view I have mixed feelings:
- Switching to a fixed bank is only two Z80 instructions:
ld a,<n> ; ld (<nn>),a(or equivalent). - But switching to a dynamic bank (e.g. a calculated bank number for a large lookup table) is more complex. Maybe something like this:
Input: 12-bit bank number in HL
ld a,h
or 0x60 ; 0x70 for 2nd bank
ld h,a
ld (hl),l
So actually not too bad, and maybe that or 0x60 could be pre-calculated.
Is it a hard requirement that the switch regions are compatible with the ASCII16 mapper? I often like it that the switch region for a bank is located in the bank itself. That allows to work with the mapper when it's only partially selected in the MSX slots.
Anyway, just my first impression from reading this documentation.
There was a problem hiding this comment.
Using 4 address bits to "extend" the data-bus to 12-bits is a bit unusual. And unexpected (to me). From a hardware point of view I understand how this could indeed lead to a simpler design. From a software point of view I have mixed feelings: [...]
ld (hl),l[...] So actually not too bad, and maybe that or 0x60 could be pre-calculated.
Yes, indeed if you want to do a 16-bit bank switch you’d do ld (hl),l (that’s why it uses address bits 8-11), which is only 7 cycles for a 16-bit switch. You’ll typically want to embed the register address in the bank number.
Another approach (which Usas2 will take probably) is to divide the game data into two 4 MB sections, e.g. level data in the lower half and music data in the upper half. Then continue to use 8-bit bank numbers within them and you’d write to 0x6000 and 0x6100 depending on the type of data.
From a hardware perspective I receive 24 bits (address + data) in a single operation, so I can use them all in one go rather than having two operations. Two operations means they need to be stored in separate registers. This would require two additional 74LVC823 chips (relatively expensive) as well as an extra decoder chip.
So the mapper’s design involves both hardware considerations in terms of cost (minimal chip count, 2-layer board, and components on a single side), as well as software considerations (being fast and versatile).
Is it a hard requirement that the switch regions are compatible with the ASCII16 mapper?
Not per se, I started out with ASCII16 because it is the largest capacity mapper, also fairly simple, and I thought it would add some value if it were compatible. Both in terms of making it easier to use or adapt existing developer tooling and code, as well as to make it useful as a platform to run or release ASCII16 games on.
Usas2 code currectly built for ASCII16 runs on it verbatim, so does Aleste, Penguin Wars, Pointless Fighting, etc.
Also note that ASCII mappers are flash-able because you can write to memory without affecting its bank register. Konami mapper bank registers overlap with the memory they switch, so they can’t be flashed normally.
I often like it that the switch region for a bank is located in the bank itself. That allows to work with the mapper when it's only partially selected in the MSX slots.
The bank registers are mirrored, so you can access them from any page.
The only downside of this mirroring is that they’re also mirrored to page 3, which means the BIOS’s expanded slot detection messes up the initial bank of page 2 (same issue for NEO btw). Unfortunately not something I can solve without adding extra chips or removing the mirroring entirely. It bugs me from a theoretical point of view that it’s no longer fully ASCII16 compatible, but in practice there’s no real problem, even for existing ASCII16 games that I tried, so I kept it as-is.
Perhaps very early flash chips only had the long version, and later the short one was introduced but the long one was kept for compatibility. Practically speaking though, the S29GL064S only accepts the long reset command to exit out of the PRGERR state. |
This is because internally the x8x16 devices are 16-bits, and this affects the values read from the CFI query on the odd addresses. According to the CFI specification they are supposed to read the same as the even addresses, however both the M29W640GB and the S29GL064S instead return the high byte of the 16-bit values. All the CFI values themselves are defined as 8-bit with the upper bits being zero, however the undefined values return 0xFFFF, and the (undocumented) autoselect data visible in the region < 0x10 also includes the MSBs from their 16-bit values. In the end it was more logical to have |
|
There’s one more thing I’d like to do… When used as a ROM mapper with initial content (via So it would be good if the SRAM only stored the modifications. In case of Manbow2 it happens to have the non-savegame sectors hardware write protected, so conveniently So I want to keep track of modified sectors by having an array of My biggest question though is how to best implement the persistence part. The SRAM class has the ability to have a string header (not currently used anywhere I think), I could use this right now to store that modified-array. The extensibility of this scheme is limited though, since I could change the SRAM code to have a more flexible header option, perhaps using the existing serialisation framework which has built in versioning. For back compat, possibly putting it after the data part instead of in front (since the data size should be static), then I could check its presence based on the file size. Or have it support a variable-length ASCIIZ header without bailing out. But maybe also worth considering something even more complex where it doesn’t even store the sectors which haven’t been modified (though I’m not leaning towards that). Do you have any thoughts / ideas? The simpler the better… |
|
The SRAM header functionality is used in e.g. src/memory/MSXPac.cc to have the .sram files in a compatible(?) format with on-disk MSX-Pac backup files. But I'm not sure if this was really a useful feature. Though I don't think reusing this header functionality is the way to go. My current thinking is:
That new file could maybe be an ASCII file with lines like: I have NOT fully worked out this idea. Here are some open questions:
EDIT:
|
Ah I see, just an ID string then, that explains why the header works the way it does.
Also a good option. There is some merit to the
What about relying on the existing serialisation mechanism? That seems quicker to implement and easier to code for… I don’t think there’s a binary on-disk format, so that would mean XML, as used by savestates and replays.
Same base name sounds fair, extension can be bikeshed over in review :).
That makes the most sense to me. Don’t want to lose any existing savegames that people may have.
Aside from OS support, I think this also relies on the file system capabilities? An alternative is to store an offset in the metadata file, and have the memory file be a noncontiguous block of sectors that gets appended to when a new sector gets modified and needs to be persisted. It’s not a feature I particularly care for at the moment though. Both of these options could be added later in a compatible way I think.
In principle I don’t care much about whether
Yeah, I think the new thing should replace the current sparse persistence mechanism based on |
|
Updated, with review comments addressed. I’ll do the sparse persistence thing in a separate PR later, to not conflate this one. |
grauw
force-pushed
the
ascii-x
branch
4 times, most recently
from
ed84c4d
to
5f04601
Compare
Introducing a constexpr power_of_two struct for power of two numbers, with compact storage and built-in value correctness assertion. Additionally, the compiler optimises multiplication / division / modulo operations to bit shifts.
- Use chip & auto select structures for manufacturer / device ID. - Add support for extended device IDs. - Those starting with 0x7E, followed by two additional bytes at 0xE and 0xF. - Require reset command to exit auto select. - For unmatched/complete commands, only reset the command index. - State transitions need to be explicit. - Allows us to fix auto select check, which would consume extra write. - Only allow reset command while in auto select state. - Rename “manufacturer” command functions to common term “auto select”. - Improve auto select address 0x03 code. - Fix forum link. - Add some info from data sheets (there’s no standard). - Implement the value returned by M29W640GB. - Extract peekAutoSelect function which returns the 16-bit result (needed later). - Make auto select address mask mirroring configurable. - Handle some other M29W640GB details wrt undefined values and odd addresses.
Rather than enumerating individual sectors. Additionally; Make `sum` constexpr; seems to work with current compilers. Make static_vector(std::initializer_list<T> list) not explicit; without this GCC refuses to compile. It’s not explicit for std::vector and std::set either. I think for initializer lists implicit is generally considered acceptable.
There is a long version of the reset command.
Only the Micron M29W640GB supports the fast program commands (double, quadruple byte program), so don’t support it on others.
I noticed the MegaFlashRomSCCPlus used Addressing::BITS_11 while it has an 8/16-bit FlashROM chip. BITS_11 should only be used for 8-bit FlashROM chips or 16-bit ones when accessed in 16-bit mode. When accessing a dual-mode 8/16-bit FlashROM chip in 8-bit mode, one should use BITS_12. I could only think of two possible explanations for this; either the chip was operating in 16-bit mode with D8-D15 left unconnected, with only half of the capacity available, or address line A0 (A-1 on datasheet) was connected to CPU line A11 or A12 with all lines in-between shifted down. Manuel Pazos confirmed it was the latter, specifically Flash line A0 is connected to CPU line A12. As a consequence of this, read caching is not possible.
DeviceInterface specifies the inherent property of the Flash chip, being either an 8-bit, 16-bit or dual-mode 8/16-bit Flash ROM chip, as opposed to the addressing parameter which is more loosely coupled.
Common Flash Interface (CFI) queries are supported by most “modern” Flash ROMs and allows software to identify the presence of a Flash device and informs about device capabilities.
When the buffered program is aborted due to not matching one of the conditions after the initial command code, the program error state is entered, which reads the status register until it receives a reset command. Some devices require a longer reset command sequence.
A hard reset initializes the status register to 0x80, whereas a soft reset (in response to commands) only clears certain bits.
This function allows the program to check for correct connectivity of the flash chip, and would fail if there are shorts on adjacent address and data pins. It seems to be specific to the Infineon / Cypress / Spansion S29GL064S, at least I haven’t seen it being offered elsewhere.
I’ve been working on an ASCII16-based mapper with 8 MB of FlashROM called “ASCII16-X Mapper XL”. This contains an implementation of that mapper, as well as a large number of FlashROM emulation improvements done along the way.
Testing has been done on real hardware with the S29GL064S and M29W640GB chips, I also have a M29W800DB laying about but I still need to build the board. I can’t currently test with AM29F040 and AM29F016 so those are based on their datasheets and the comments in the openMSX source code. Anyway, if there are any open questions on hardware behaviour I can answer them with that in mind.
I still want to do some tests before pushing, so marked as draft.
I’d welcome a code review though.