VOGONS


First post, by ardovm

User metadata
Rank Newbie
Rank
Newbie

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:

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:

	// 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:

	// 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

Reply 1 of 5, by Qbix

User metadata
Rank DOSBox Author
Rank
DOSBox Author

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!

Reply 2 of 5, by ardovm

User metadata
Rank Newbie
Rank
Newbie

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

Reply 3 of 5, by Qbix

User metadata
Rank DOSBox Author
Rank
DOSBox Author

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!

Reply 4 of 5, by ardovm

User metadata
Rank Newbie
Rank
Newbie

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:

    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:

op2   temp_op2   irq_active   output
0 1 0 deactivate
0 1 1 X
1 0 0 activate
1 0 1 X

My proposal:

    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:

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:

    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:

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

Reply 5 of 5, by Qbix

User metadata
Rank DOSBox Author
Rank
DOSBox Author

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.

Water flows down the stream
How to ask questions the smart way!