mm58167: put the milliseconds digit in the high nibble of register 0 - #15860
Open
Gersoft-lab wants to merge 1 commit into
Open
mm58167: put the milliseconds digit in the high nibble of register 0#15860Gersoft-lab wants to merge 1 commit into
Gersoft-lab wants to merge 1 commit into
Conversation
Contributor
|
Please get rid of the unnecessary, overly verbose AI comment and I'll merge this. |
TABLE I of the MM58167A datasheet (National Semiconductor Real Time Clock Handbook, p. 1-97) places the milliseconds digit in the high nibble of register 0; the low nibble counts ten-thousandths of a second and always reads 0 on the real part. The rest of this file already assumed that: the alarm comparator only examines the high nibble of that register, and update_rtc() reads the digit back with a >> 4. Only the write in clock_tick() disagreed, so a value written by the device could not be read back correctly by the device itself.
Author
|
Done — comment removed; the rationale is in the commit message. I've also added an AI-assistance disclosure to the description. Thanks for the review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A one-line fix to the National Semiconductor MM58167 real-time clock: register 0 (the milliseconds counter) writes its digit into the wrong nibble.
The change is confined to
src/devices/machine/mm58167.cpp.Symptom
clock_tick()writes the milliseconds digit (0-9) into the low nibble ofR_CNT_MILLISECONDS:Any code that reads register 0 expecting the digit in the high nibble sees a nibble that stays at 0 while the clock is actually ticking.
Cause — the file already disagrees with itself
Two other places in this same file read register 0 assuming the digit is in the high nibble:
0xf0/0xc0(high-nibble) mask pair toR_CNT_MILLISECONDS— the0x0f/0x0c(low-nibble) term is unconditionally skipped for that register;update_rtc()extracts the digit withbcd_to_integer(m_regs[R_CNT_MILLISECONDS] >> 4).So today, right after a tick: the write puts the digit in the low nibble, the comparator and
update_rtc()both read the high nibble, and the low nibble that the comparator actually checks against the RAM alarm setting stays 0 regardless of the real millisecond count. The write and the two reads cannot all be correct at once; this change makes the write agree with both reads.Datasheet
National Semiconductor Real Time Clock Handbook (1989 Edition), MM58167A section, Table I ("Real Time Counter Format"), p. 1-97: for the milliseconds counter (address 00H), the "Units" nibble (D0-D3) is listed with dashes (unused, Max BCD Code 0) and the digit is in the "Tens" nibble (D4-D7, Max BCD Code 9).
Change
Same write, shifted into the nibble the other two sites already read from.
Testing
Builds clean. Functional testing is on a work-in-progress driver, not yet upstream: its power-on self-test polls register 0 waiting for the high nibble to go from non-zero back to zero as proof the clock is running. With the digit in the low nibble that test never observes a change and loops forever; with the fix applied it passes and the machine proceeds to boot. That is the extent of the claim — one driver, one code path exercised, not broad validation of the device.
Impact on other users of this device
mm58167_deviceis instantiated by 17 machines/boards in tree:src/mame/hp/hp_ipc.cpp,src/mame/tektronix/tekigw.cpp,src/mame/sun/sun2.cpp,src/mame/skeleton/optomaxv.cpp,src/mame/skeleton/lft.cpp,src/mame/skeleton/gimix.cpp,src/mame/siemens/pg685.cpp,src/mame/sfrj/idpartner.cpp,src/mame/regnecentralen/rc759.cpp,src/mame/kaypro/kaypro.cpp,src/mame/chromatics/cgc7900.cpp,src/mame/apple/apple3.cpp,src/mame/altos/altos586.cpp,src/devices/bus/vme/sys68k_cpu1.cpp,src/devices/bus/ti99/peb/pgram.cpp,src/devices/bus/multibus/labtam_z80sbc.cpp,src/devices/bus/dmv/k803.cpp.I have not tested any of these against this change — no hardware for any of them, and I did not attempt to boot their software.
One thing I noticed while grepping for users, offered without drawing a conclusion from it:
src/mame/altos/altos586.cppmaps the device with the comment "RTC - Counter - thousandths of seconds" on the register-0 address range, next to an openTODO: The RTC seems to run approx. 2 times slower. Why?a few lines below, at theMM58167(config, ...)line itself. I don't know whether that TODO is related to this bug or to something unrelated elsewhere in that driver; I am flagging it because it is something a maintainer familiar withaltos586could check quickly, not because I believe the two are connected.I have left the
copyright-holdersline unchanged; add me or not, whichever matches your preference for a fix of this size.This change was developed with AI assistance (Anthropic Claude — Claude Code with Opus and Sonnet models, most recently Claude Opus 5); all output was manually reviewed, and the fix was verified against the datasheet as described above.