RTC, weird time and not working interrupts

[Matt1223]

Hi,
I've got a problem with getting RTC to cooperate. First of all, when I read CMOS values for minutes and hours it returns weird values. For example, minutes seem to be in the range of 0-100, and hours are quite random and also extend 24 hours. So I get times like 32:72, which is quite weird. This problem occurs on both bochs and quemu, although values differ a little bit from one emulator to another.

My second problem is that I try to enable RTC interrupts but they do not occur.

The code I use is below, can you spot, a problem in it?

Code: Select all

void RTC_init()
{
	uint8_t m;
	uint8_t h;
	bool update_in_progress;

	disable_interrupts();

	// Read from CMOS
	
	// may take up to 1 second, prevents from reading dodgy data for example minute: 60
	outb(0x70, (1 << 7) | (0x0A));
	update_in_progress = ((inb(0x71) << 1) >> 7);
	while(!update_in_progress)
	{
		outb(0x70, (1 << 7) | (0x0A));
		update_in_progress = ((inb(0x71) << 1) >> 7);
	}
	outb(0x70, (1 << 7) | (0x0A));
	update_in_progress = ((inb(0x71) << 1) >> 7);
	while(update_in_progress)
	{
		outb(0x70, (1 << 7) | (0x0A));
		update_in_progress = ((inb(0x71) << 1) >> 7);
	}

	outb(0x70, (1 << 7) | (0x02));
	io_wait();
	m = inb (0x71);

	outb(0x70, (1 << 7) | (0x04));
	io_wait();
	h = inb (0x71);

	// Enable RTC interrupts

	int rate = 14;
	rate &= 0x0F;			// rate must be above 2 and not over 15
	outb(0x70, (1 << 7) | (0x0A));		// set index to register A, disable NMI
	uint8_t prev = inb(0x71);	// get initial value of register A
	outb(0x70, (1 << 7) | (0x0A));		// reset index to A
	outb(0x71, (prev & 0xF0) | rate); //write only our rate to A. Note, rate is the bottom 4 bits.

	outb(0x70, (1 << 7) | (0x0B));
	prev = inb(0x71);
	outb(0x70, (1 << 7) | (0x0B));
	outb(0x71, prev | 0x40);

	enable_interrupts();
	enable_RTC_irq();

	outb(0x70, 0x0C);	// select register C
	inb(0x71);		// just throw away contents

	RTCminute = (int)m;
	RTChour = (int)h;

	terminal_print(debugTerminal, "RTC ready, time: %d:%d\n", RTChour, RTCminute);
}

[Octocontrabass]

First of all, when I read CMOS values for minutes and hours it returns weird values.

Bits 1 and 2 of register B tell you how to interpret the time and date. Did you check those bits?

My second problem is that I try to enable RTC interrupts but they do not occur.

How is the interrupt controller configured?

[nullplan]

Polling the "update in progress" flag is always racy. You can, however, query it once at the start and once at the end, and retry if an update started in the meantime. So something like

Code: Select all

do {
  do {
    update_in_progress = (cmos_read(0x0a) >> 6) & 1;
  } while (update_in_progress);
  m = cmos_read(0x02);
  h = cmos_read(0x04);
  update_in_progress = (cmos_read(0x0a) >> 6) & 1;
} while (update_in_progress);

See, in your code it is possible that you check the update flag, and it is not set, but it becomes set while you are reading the time, and that's why you are reading garbage. The alternative is, of course, to get an after-update interrupt and read the time then.

[Matt1223]

Bits 1 and 2 of register B tell you how to interpret the time and date. Did you check those bits?

Thank you, I didn't know about this data in register B. Now It works fine.

How is the interrupt controller configured?

Well, the master PIC's offset is set to 0x20 and the slave's offset is set to 0x28.
enable_RTC_irq(); calls this function with parameter 8:

Code: Select all

void IRQ_clear_mask(uint8_t IRQline)
{
	uint16_t port;
	uint8_t value;
	
	if(IRQline >=8)
	{
		port = PIC2_DATA;
		IRQline -=8;
	}
	else{
		port = PIC1_DATA;
	}
	value = inb(port) & ~(1 << IRQline);
	outb(port, value);
}

So there shouldn't be any problem with PIC configuration.
Below is the PIC interrupts handler code.

Code: Select all

void pic_handler()
{
	uint16_t isr = pic_get_isr();
	uint8_t isr1 = isr >> 8;
	uint8_t isr2 = isr << 8;
	int irq_num;
	
	if(isr2 == 0)
		irq_num = isr1 - 1;
	else
		irq_num = isr2 + 7;

	switch( irq_num )
	{
	case 1:
	    keyboard_irq();
	    break;
	   
	case 8:
	    RTC_irq();
	    break;
	   
	default:
	    ;
	    break;
	}
	
	PIC_sendEOI(irq_num);
}

[Matt1223]

Polling the "update in progress" flag is always racy. You can, however, query it once at the start and once at the end, and retry if an update started in the meantime. So something like

Code: Select all

do {
  do {
    update_in_progress = (cmos_read(0x0a) >> 6) & 1;
  } while (update_in_progress);
  m = cmos_read(0x02);
  h = cmos_read(0x04);
  update_in_progress = (cmos_read(0x0a) >> 6) & 1;
} while (update_in_progress);

Don't you think, there is a possibility that the update will start while reading and also end before we check it. I know that in this case just two values are read so there won't be time for the update to finish, but assuming we read more values isn't it a dangerous situation?

[nullplan]

Don't you think, there is a possibility that the update will start while reading and also end before we check it. I know that in this case just two values are read so there won't be time for the update to finish, but assuming we read more values isn't it a dangerous situation?

That is (in theory) absolutely possible, but the CMOS offers no latching mechanism of any kind, so no way to do read the time without that possibility. Even reading the time directly after the update interrupt merely gives you the most amount of time possible to read the time, it doesn't prevent an update from occurring while you read the time. The CMOS interface isn't very good. But we have to make do with it, since the RTC is one of the few components in the PC that hasn't been updated since the original IBM 5150. Oh sure, it has been rolled into the Southbridge chip (or whatever the kids are calling it these days), but the interface and its weaknesses remain.

[Octocontrabass]

enable_RTC_irq(); calls this function with parameter 8:

When is that function called with parameter 2? The IRQ2 mask inhibits all of IRQ8-15.

Below is the PIC interrupts handler code.

The interrupt vector number already tells you which IRQ you're handling, so you're wasting time by reading the PIC ISR. You're also interpreting the contents of the PIC ISR incorrectly.

[Matt1223]

When is that function called with parameter 2? The IRQ2 mask inhibits all of IRQ8-15.

Well, it wasn't called with parameter 2. Anyway, I call it now and it still doesn't work.

The interrupt vector number already tells you which IRQ you're handling, so you're wasting time by reading the PIC ISR. You're also interpreting the contents of the PIC ISR incorrectly.

By "Interrupt vector number tells you which IRQ you're handling" do you mean that my irq handler in IDT should push an interrupt number before calling pic_handler? I'm using one irq_handler for all 0 to 15 PIC interrupts right now:

Code: Select all

for(int i=0; i<32; i++)
	SETIDTDESCR(IDT[i], int_handlers[i]);
	
for(int i=0; i<16; i++)
	SETIDTDESCR(IDT[i+32], irq_handler);
	
__asm("lidt [%0]" : : "r"(&ptr));

Why am I interpreting the contents of the PIC ISR incorrectly? pic_get_isr() returns 16 bit combined value of Master and Slave ISR, so that's why it might be confusing.

[Octocontrabass]

By "Interrupt vector number tells you which IRQ you're handling" do you mean that my irq handler in IDT should push an interrupt number before calling pic_handler?

That's one way of doing it. Another way is to have a separate code path for each vector instead of using a shared handler.

Code: Select all

__asm("lidt [%0]" : : "r"(&ptr));

The compiler may break your code when optimizing. Do this instead:

Code: Select all

__asm("lidt %0" : : "m"(ptr));

Why am I interpreting the contents of the PIC ISR incorrectly?

Code: Select all

	uint16_t isr = pic_get_isr();
	uint8_t isr1 = isr >> 8;
	uint8_t isr2 = isr << 8;

The "isr2" variable will always be 0.

Code: Select all

	if(isr2 == 0)
		irq_num = isr1 - 1;
	else
		irq_num = isr2 + 7;

The PIC ISR is a bit field with one bit set for each IRQ in service, but you're interpreting it as if it contained the number of the IRQ in service.

[Matt1223]

The "isr2" variable will always be 0.

Code: Select all

	if(isr2 == 0)
		irq_num = isr1 - 1;
	else
		irq_num = isr2 + 7;

The PIC ISR is a bit field with one bit set for each IRQ in service, but you're interpreting it as if it contained the number of the IRQ in service.

Ok, that was the problem. Everything works fine yet. Thank you for your help.

Code: Select all

__asm("lidt [%0]" : : "r"(&ptr));

The compiler may break your code when optimizing. Do this instead:

Code: Select all

__asm("lidt %0" : : "m"(ptr));

Can you explain to me why the compiler may break this code?

[Octocontrabass]

Can you explain to me why the compiler may break this code?

The code you wrote tells the compiler "address of ptr" is significant to the inline assembly, but not "value of ptr", so the compiler will optimize on the assumption that your inline assembly doesn't read the value of ptr. Those optimizations can include setting ptr after the inline assembly instead of before, or not setting ptr at all!

The code I wrote tells the compiler "value of ptr" is significant, so the compiler will ensure ptr is set to the appropriate value before the LIDT instruction executes.