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.

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.

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.

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/public/u … y-datasheet.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/Webster … OS/pdf/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’ […]
Show full quote

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:

1    if((!op2) && temp_op2) {
2      // irq has been enabled (tristate high -> irq level)
3      if(!irq_active) PIC_DeActivateIRQ(irq);
4   } else if(op2 && (!temp_op2)) {
5      if(!irq_active) PIC_ActivateIRQ(irq);    <----------- here
6   }

Truth table:

1op2   temp_op2   irq_active   output
20     1          0            deactivate
30     1          1            X
41     0          0            activate
51     0          1            X

My proposal:

1    if((!op2) && temp_op2) {
2      // irq has been enabled (tristate -> irq level)
3      if(irq_active) PIC_ActivateIRQ(irq);
4   } else if(op2 && (!temp_op2)) {
5      // irq has been disabled (irq level -> tristate)
6      if(irq_active) PIC_DeActivateIRQ(irq);
7   }

Truth table:

1op2   temp_op2   irq_active   output
20     1          0            X
30     1          1            activate
41     0          0            X
51     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:

1    if((!op2) && temp_op2) {
2      // irq has been enabled (tristate high -> irq level)
3      if(!irq_active) PIC_ActivateIRQ(irq);
4   } else if(op2 && (!temp_op2)) {
5      if(!irq_active) PIC_DeActivateIRQ(irq);
6   }

Truth table:

1op2   temp_op2   irq_active   output
20     1          0            activate
30     1          1            X
41     0          0            deactivate
51     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!

Thanks for the report!

I checked your documents and mine and agree that the current logic is inconsistent. I have commited a fix in 4020.