Serial port IRQ and the MCR UART register

Developer's Forum, for discussion of bugs, code, and other developmental aspects of DOSBox.

Serial port IRQ and the MCR UART register

Postby ardovm » 2017-5-19 @ 14:40

Hello All,

I am opening this topic to discuss the generation of serial port related IRQ's as triggered by the OP2 bit in the MCR register.

Short description:
IMHO the ``tri-state logic'' in serialport.cpp around line 785 is flawed. A ``tristated'' IRQ should be disabled, while it is in fact enabled.
Shall I file a bug for this?

Long description:
I was working on a program that works with COM1 and COM3 (they share IRQ 4), and saw that COM1 stopped receiving IRQ's after I shut down COM3 (by setting, among others, its MCR register to zero).

I found out that the IRQ 4 was disabled by the standard IRQ handler in src/bios.cpp, line 966:
Code: Select all
static Bitu Default_IRQ_Handler(void)

That function correctly assumes that unhandled IRQ's shall be stopped at the source (i.e. PIC).

The problem is that the IRQ line is raised when the OUT2 bit of MCR is set to zero:
Code: Select all
   // interrupt logic: if OP2 is 0, the IRQ line is tristated (pulled high)
   if((!op2) && temp_op2) {
      // irq has been enabled (tristate high -> irq level)
      if(!irq_active) PIC_DeActivateIRQ(irq);
   } else if(op2 && (!temp_op2)) {
      if(!irq_active) PIC_ActivateIRQ(irq);    <----------- here
   }

If ``tristated'' means ``disabled'', as it should IMHO, the correct code should be as follows:
Code: Select all
   // interrupt logic: if OP2 is 0, the IRQ line is tristated (disabled)
   if((!op2) && temp_op2) {
      // irq has been enabled (tristate -> irq level)
      if(irq_active) PIC_ActivateIRQ(irq);
   } else if(op2 && (!temp_op2)) {
      // irq has been disabled (irq level -> tristate)
      if(irq_active) PIC_DeActivateIRQ(irq);
   }

I looked for the corresponding lines in the QEMU and VirtualBox sources, and if I understood them correctly, they just ignore the OP2 bit of the MCR register: they raise the IRQ only when the IER register requests so.

Shall I file a bug for this? Or is this forum the best place to discuss this?

Thank you in advance!

Arrigo
Arrigo
User avatar
ardovm
Newbie
 
Posts: 3
Joined: 2017-5-19 @ 14:20

Re: Serial port IRQ and the MCR UART register

Postby Qbix » 2017-5-19 @ 15:41

Here is fine.

Ah, this was probably revealed due to the new spurious interrupt handling code.

So if I get it right, you write a new mcr value of 0 : so temp_op2 = false and op2 = true.
I am not sure if tri-stated means disabled (as I don't know if the OP2 in the comment is for the new op2 or the old op2, but it does seem plausible that the logic is reversed, as in the current code disabling the aux 2, raises the IRQ line. Is there a reason why you are swapping the if condition for irq_active ? Wouldn't be swapping PIC_Activate and Pic_Deactivate be enough ? I wonder why both are checking for the same value of irq_active... ComputeInterrupts might reveal some insight on why it was coded like that.(or how it should be).

Ideally I'd like to find some chip reference guide. because that whole piece of code might be off given your reseach in Qemu and virtualbox. I couldn't find the right portion in the bochs sourcecode in my quick scan.
Water flows down the stream
How to ask questions the smart way!
User avatar
Qbix
DOSBox Author
 
Posts: 10295
Joined: 2002-11-27 @ 14:50
Location: Fryslan

Re: Serial port IRQ and the MCR UART register

Postby ardovm » 2017-5-19 @ 18:37

Thank you for your reply, Qbix.

Qbix wrote:So if I get it right, you write a new mcr value of 0 : so temp_op2 = false and op2 = true.
Correct.
I am not sure if tri-stated means disabled (as I don't know if the OP2 in the comment is for the new op2 or the old op2, but it does seem plausible that the logic is reversed, as in the current code disabling the aux 2, raises the IRQ line.
The variable temp_op2 is the new value; op2 is the old one.

The only clear assertion I could find, that ``tri-stated'' means ``disabled'', is here: https://www.freebsd.org/doc/en/articles/serial-uart/
Is there a reason why you are swapping the if condition for irq_active ? Wouldn't be swapping PIC_Activate and Pic_Deactivate be enough ? I wonder why both are checking for the same value of irq_active... ComputeInterrupts might reveal some insight on why it was coded like that.(or how it should be).
Yes. CSerial::ComputeInterrupts() sets irq_active, and then calls PIC_ActivateIRQ() (when irq_active becomes true) or PIC_DeActivateIRQ() (when irq_active becomes false) only if the op2 attribute is set.
Ideally I'd like to find some chip reference guide. because that whole piece of code might be off given your reseach in Qemu and virtualbox. I couldn't find the right portion in the bochs sourcecode in my quick scan.
For what I have understood, the UART chip itself just has that auxiliary output signal, like a GPIO. Where that signal is connected, depends on the rest of the board (motherboard? chipset? whatever?...)

edit: I could find the above documentation from the FreeBSD web site by googling "UART MCR IRQ tristate".

I hope this helps.
Arrigo
User avatar
ardovm
Newbie
 
Posts: 3
Joined: 2017-5-19 @ 14:20

Re: Serial port IRQ and the MCR UART register

Postby Qbix » 2017-5-19 @ 19:30

Thanking you for continuing the discussion and providing the very useful freeBSD link.

If you look at :
http://www.ti.com/lit/ds/symlink/pc16550d.pdf (not sure about the suffix)
page 5 and 23, then writing a zero does set the output (1 and 2) to high (like the current comment mentions). Unfortunately while there is a hint on page 23 that DTS can be inverted, it is not written for the outputs and in the 2 provided typical wiring, the 2 ports are not connected. The freebsd thing does mention it as disable though. Guess we have to find a wiring diagram to see how this tri-state, which might be outside the chip itself, is achieved in a PC.

Given that the default value is actually zero (and set to zero on reset) for MCR (according to the linked document), would that mean that you always have to write a non-zero value there if you want to enable IRQs (following along with your logic that zero disables irqs) ?

I did find an book that does support your theory
Bit 3 controls another general purpose output line (GPO2) which is used in conjunction with enabling interrupts ; this bit must be set to 1 in order to enable interrupts set by the Interrupt Enable Register


I could imagine that the purpose of the current code is to fire an interrupt if an interrupt is present in the UART when interrupts are allowed again.
Although, that does conflict with the current code of ComputeInterrupts, which does support your code/reasoning in that irqs are only raised if op2 is true.

I am still wondering about why you reversed the irq_active ifs. I haven't looked that often at this code, so hence my careful questions about the proposed code changes.
Water flows down the stream
How to ask questions the smart way!
User avatar
Qbix
DOSBox Author
 
Posts: 10295
Joined: 2002-11-27 @ 14:50
Location: Fryslan

Re: Serial port IRQ and the MCR UART register

Postby ardovm » 2017-5-19 @ 22:22

Hello,
Qbix wrote:If you look at :
http://www.ti.com/lit/ds/symlink/pc16550d.pdf (not sure about the suffix)
page 5 and 23, then writing a zero does set the output (1 and 2) to high (like the current comment mentions). Unfortunately while there is a hint on page 23 that DTS can be inverted, it is not written for the outputs and in the 2 provided typical wiring, the 2 ports are not connected. The freebsd thing does mention it as disable though. Guess we have to find a wiring diagram to see how this tri-state, which might be outside the chip itself, is achieved in a PC.
I can just confirm your findings, from the Intel Atom E3800 datasheet: http://www.intel.com/content/dam/www/pu ... asheet.pdf
Chapter 32.6.5 states that the OUT2 output is "inactive high" and "active low".
Qbix wrote:Given that the default value is actually zero (and set to zero on reset) for MCR (according to the linked document), would that mean that you always have to write a non-zero value there if you want to enable IRQs (following along with your logic that zero disables irqs) ?
Yes.
Qbix wrote:I did find an book that does support your theory
Bit 3 controls another general purpose output line (GPO2) which is used in conjunction with enabling interrupts ; this bit must be set to 1 in order to enable interrupts set by the Interrupt Enable Register

Aha! Look what I found here: http://www.plantation-productions.com/W ... f/ch22.pdf on page 1229 (i.e. 7 of the PDF) says:
The Interrupt Enable bit is a PC-specific item. This is normally a general purpose output (OUT 2) on
the 8250 SCC. However, IBM’s designers connected this output to an external gate to enable or disable all
interrupts from the SCC. This bit must be programmed with a one to enable interrupts. Likewise, you must
ensure that this bit contains a zero if you are not using interrupts.

Qbix wrote:I could imagine that the purpose of the current code is to fire an interrupt if an interrupt is present in the UART when interrupts are allowed again.
Although, that does conflict with the current code of ComputeInterrupts, which does support your code/reasoning in that irqs are only raised if op2 is true.

I am still wondering about why you reversed the irq_active ifs. I haven't looked that often at this code, so hence my careful questions about the proposed code changes.

I just re-wrote the condition, in a way that looked more similar to my personal reasoning behind it: if op2 is being enabled, then the IRQ line is surely de-asserted, and must be only asserted if required. And vice-versa: if op2 is being disabled, then the IRQ line must be de-asserted if it was.

Let's try comparing truth tables.

Old version:
Code: Select all
    if((!op2) && temp_op2) {
      // irq has been enabled (tristate high -> irq level)
      if(!irq_active) PIC_DeActivateIRQ(irq);
   } else if(op2 && (!temp_op2)) {
      if(!irq_active) PIC_ActivateIRQ(irq);    <----------- here
   }

Truth table:
Code: Select all
op2   temp_op2   irq_active   output
0     1          0            deactivate
0     1          1            X
1     0          0            activate
1     0          1            X


My proposal:
Code: Select all
    if((!op2) && temp_op2) {
      // irq has been enabled (tristate -> irq level)
      if(irq_active) PIC_ActivateIRQ(irq);
   } else if(op2 && (!temp_op2)) {
      // irq has been disabled (irq level -> tristate)
      if(irq_active) PIC_DeActivateIRQ(irq);
   }

Truth table:
Code: Select all
op2   temp_op2   irq_active   output
0     1          0            X
0     1          1            activate
1     0          0            X
1     0          1            deactivate


If I understood correctly your last post:
Qbix wrote:Wouldn't be swapping PIC_Activate and Pic_Deactivate be enough ?

it should be:
Code: Select all
    if((!op2) && temp_op2) {
      // irq has been enabled (tristate high -> irq level)
      if(!irq_active) PIC_ActivateIRQ(irq);
   } else if(op2 && (!temp_op2)) {
      if(!irq_active) PIC_DeActivateIRQ(irq);
   }

Truth table:
Code: Select all
op2   temp_op2   irq_active   output
0     1          0            activate
0     1          1            X
1     0          0            deactivate
1     0          1            X

that does not give the same outcome.

By the way, now that we are discussing this piece of code extensively, please consider another, more important patch: renaming the variables temp_XXX into new_XXX, for clarity's sake! ;-)

Thank you for taking the time to look into this, so deeply!
Arrigo
User avatar
ardovm
Newbie
 
Posts: 3
Joined: 2017-5-19 @ 14:20


Return to DOSBox Development

Who is online

Users browsing this forum: No registered users and 2 guests