Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor mmio_mux #16

Merged
merged 1 commit into from
Aug 1, 2020
Merged

Refactor mmio_mux #16

merged 1 commit into from
Aug 1, 2020

Conversation

MJoergen
Copy link
Collaborator

Replace the verbose if-statements with a set of simple assignments. This makes the code easier to maintain.

This change is verified using the test programs:

  • eae.asm
  • cpu_test.asm
  • ise.asm

It is possible to simplify the code even further, if each device (except VGA) gets exactly 8 words of address space, such that all offset values are a multiple of 8. This will make the decoding much simpler, but would require changing the memory map in monitor/sysdef.asm, and perhaps other places too. In other words, a lot more testing needs to be done in that case.

Replace the verbose if-statements with a set of simple assignments. This
makes the code easier to maintain.

This change is verified using the test programs:
* eae.asm
* cpu_test.asm
* ise.asm
@bernd-ulmann
Copy link
Collaborator

I, personally, would love to clear up the IO-address space. A "real" UART (I think about Chris Giles' TTL-implementation of QNICE) typically has 16 IO registers, but if I remember correctly, the lower eight should be sufficient for all real applications. Thinking of timers which currently have three registers (prescaler, counter, interrupt address) per timer, we might need to have more than one timer IO-module, each with two independent timers.

Alternatively, we could settle on 16 words address space per IO device which would still allow for 16 IO-devices. We also could increase the address space for these devices to 1 k word.

@mirko: What do you think?

Best regards - Bernd. :-)

@sy2002
Copy link
Owner

sy2002 commented Jul 31, 2020

@MJoergen WOW - This is truely a great improvement in the readability and maintainability of the code! 👍

I will also test it tomorrow (Saturday is normally my "nerd day" ;-)) using a few more test programs and then finally commit it and close the PR. It is really great that you are on board: The quality of QNICE-FPGA is improving with each and every of your changes! 😃

Would you like to get write access to the repo itself? Then you would not need to do PRs. The only thing that I would need to do beforehand is to write two small docs, that I wanted to write anyway since a while: Some programming conventions we have and some conventions about the repo (e.g. the master branch is always equal to the latest official stable release, e.g. currently V1.5, while the development takes place at the develop branch and larger, "dangerous" developments even in separate branches such as dev-int, etc. Exception to the master branch rule are super small changes, such as the one you proposed with your first PR).

One question about this PR: "How does this work?": For example:

vga_offset    <= std_logic_vector(unsigned(addr) - x"FF00");  vga_cs    <= '1' when unsigned(vga_offset)    < 16 else '0';
til_offset    <= std_logic_vector(unsigned(addr) - x"FF10");  til_cs    <= '1' when unsigned(til_offset)    <  2 else '0';
switch_offset <= std_logic_vector(unsigned(addr) - x"FF12");  switch_cs <= '1' when unsigned(switch_offset) <  1 else '0';
kbd_offset    <= std_logic_vector(unsigned(addr) - x"FF13");  kbd_cs    <= '1' when unsigned(kbd_offset)    <  4 else '0';

If addr is for example FF00 then switch_offset turns into a negative number or into a high positive number, respectively: switch_offset <= std_logic_vector(unsigned(x"FF00") - x"FF12") = ??? Is it FFEE and therefore will it interfere with another device sitting at FFEE then?

How does VHDL handle this?

@bernd-ulmann You are right, we need to do this clean-up. I would strongly opt for a standard of 8 words per device with the options for some devices (such as VGA and maybe a "advanced IO device" for the MEGA65 that includes multiple sub devices such as HyperRAM and others) to have 16 words. Reason: I really do not want to increase the I/O space larger than 256 words and on the other hand I feel that 16 devices will soon be not enough room for our creativity 💥 As long as we don't have an MMU (and for simple programming maybe even after we have an MMU), we need to greedily count each and every word of RAM that we have. ;-) Think about the C standard lib for example. I suggest that we split up the further way ahead (including the discussion) to a new Issue and I will invite both of you to this issue. Also, I would only want to introduce this, after we are stable again (i.e. dev-int successful).

This pull request remains for the final discussion of the MMIO_MUX refactoring.

@bernd-ulmann
Copy link
Collaborator

Hi - thinking of the PDP11 architecture which reserved 8 kB for IO-space (which is pretty substantial given its 16 bit address space and no MMU in the small models such as the 11/03, 11/04, 11/05 etc.), I think we would not be too wasteful if we reserved 512 word for IO but having 8 registers/device with 256 word IO space should be sufficient, too. Maybe we should think about merging some IO devices such as the cycle/instruction counter and maybe a simple interrupt-enable register or the like, what do you think?

@sy2002 sy2002 merged commit e5db6e8 into sy2002:develop Aug 1, 2020
@sy2002
Copy link
Owner

sy2002 commented Aug 1, 2020

@MJoergen I merged the PR, synthesized with Vivado and then run into a strange effect, when performing some additional tests on my loccal Nexys4DDR hardware.

Let's start first with what works fine:

  • You already tested eae.asm, cpu_test.asm and ise.asm: I retested: Works.
  • To check if the VGA registers and the SD card registers and the PS2/USB keyboard stills work, I loaded Q-TRIS from qbin/q-tris.out from the SD card and played it: Works only partially: The keyboard behaves very strange: see description below (But VGA and SD card seem to work fine)
  • To test if the TIL leds still work, I ran regbank.asm: Works.
  • To check the UART, I switched STDIN/OUT and connected by a serial program and I used qtransfer: Works.

PS2/USB Keyboard does not work reliably any more:

Here is the strange effect I observe on the PS2/USB keyboard: Many times, I need to hit keys 3-4 times until they are registered. Example: When trying to load Q-Tris from the SD Card, I need to enter normally F L qbin/q-tris.out but now I need to enter F LL qqqbin///q--trisss...outtt, i.e. I sometimes (not predictable when and which key) need to repeat the keys.

When I revert the PR to the old design and synthesize again, then it works fine.

So for now, I reverted the develop branch back to the old design.

Since I consider your design as much more elegant, it would be great, if we could hunt down this bug together :-)

At this moment in time I am stunned: Is your new elegant design maybe faster than my old one and uncovered a timing problem that was sleeping there since years? Or do we all just overlook some obvious typo?

This was referenced Aug 1, 2020
@MJoergen
Copy link
Collaborator Author

MJoergen commented Aug 3, 2020

Ok, thank you for your additional testing. Indeed, I did not test with the keyboard. I will try to reproduce and see if I can figure out what has gone wrong.

@MJoergen
Copy link
Collaborator Author

MJoergen commented Aug 3, 2020

Would you like to get write access to the repo itself?"

Sure, that would be nice, and convenient too. Good idea with some coding guidelines and project guidelines. I want to fix this MMIO refactoring bug first.

@sy2002
Copy link
Owner

sy2002 commented Aug 3, 2020

re the bug/problem: Great thank you for your help - please look at issue #22 and let's discuss it there.

re Write access to the repo: I will write the guideline docs this evening and grant you write acccess afterwards.

MJoergen added a commit that referenced this pull request Aug 5, 2020
This problem was seen in PR #16 and is mentioned in Issue #22.

There was a race condition caused by incorrect clocking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants