Page 1 of 1

Something wrong with my ATA Code.

Posted: Wed Nov 07, 2007 12:48 am
by Shalted
Hmm, I've been working on a bit of an ATA interface for my OS strap loader. It seems to work fine under VMware, but when I run it on real hardware, it pours out wrong data.

Code: Select all

//Important defined constants.
#define ATA_STAT_BSY	BIT(7)//	(busy)
#define ATA_STAT_DRDY	BIT(6)//(device ready)
#define ATA_STAT_DF		BIT(5)//(Device Fault)
#define ATA_STAT_DSC	BIT(4)//	(seek complete)
#define ATA_STAT_DRQ	BIT(3)//	(Data Transfer Requested)
#define ATA_STAT_CORR	BIT(2)//(data corrected)
#define ATA_STAT_IDX	BIT(1)//	(index mark)
#define ATA_STAT_ERR	BIT(0)//	(error)


#define ATA_ERR_ABRT	BIT(2)//(data corrected)

#define ATA_DATA				0
#define ATA_FEATURES_AND_ERRORS	1
#define ATA_SECTOR_COUNT		2
#define ATA_LBA_LOW				3
#define ATA_LBA_MID				4
#define ATA_LBA_HIGH			5
#define ATA_DRIVE_HEAD			6
#define ATA_DEVICE				6

#define ATA_COMMAND_STATUS		7
//Command: Write BSY & DRQ == 0

#define ATA_ALT_STATUS			0x206
#define ATA_DEVICE_CONT			0x206

#define ATA_COMMAND_ID			0xEC

char ReadSector(char drive, int sector, word *buffer, char count){
	word hBase = 0x1F0;
	int i,j;
	if(drive > hard_drive_cnt){
		#ifdef __DEBUG
		DebugPrint("Attempt to read from non-existant drive: %d\n",drive);
		#endif
		return -1;
	}
	hBase = hard_drives[drive].port_range;
	//Wait for status to clear
	while(InPortByte(ATA_COMMAND_STATUS + hBase) & ATA_STAT_BSY);
	
	OutPortByte(2,ATA_DEVICE_CONT		+ hBase);					//Get rid of that pesky interrupt bit
	//Wait for Drive ready(Set), and not Busy(Clear)
	while((InPortByte(ATA_COMMAND_STATUS + hBase) & (ATA_STAT_BSY|ATA_STAT_DRDY))!= ATA_STAT_DRDY);
	//Send a null byte to features.
	OutPortByte(0					, ATA_FEATURES_AND_ERRORS	+ hBase);
	//Send the count
	OutPortByte(count			, ATA_SECTOR_COUNT			+ hBase);
	OutPortByte((char)sector		, ATA_LBA_LOW				+ hBase);
	OutPortByte((char)(sector>>8)	, ATA_LBA_MID				+ hBase);
	OutPortByte((char)(sector>>24)	, ATA_LBA_HIGH				+ hBase);
	//Don't worry about the SLAVE constant, it's just a remnant from a structure
	OutPortByte((char)(0xE0 | ((hard_drives[drive].flags&SLAVE)<<1) | ((sector >> 24) & 0x0F)), ATA_DRIVE_HEAD+hBase);
	while(InPortByte(ATA_COMMAND_STATUS	+ hBase) &  ATA_STAT_DRDY != ATA_STAT_DRDY);
	OutPortByte(0x20				, ATA_COMMAND_STATUS		+ hBase);
	DeviceWait(hBase);
	if(InPortByte(ATA_FEATURES_AND_ERRORS+hBase) & ATA_ERR_ABRT != 0)
		return -1;
	//Wait until there's data to read
	while(InPortByte(ATA_COMMAND_STATUS + hBase) & ATA_STAT_DRQ != ATA_STAT_DRQ);
	for(j=0; j<count; j++){
		//Read in status before every read?
		InPortByte(ATA_ALT_STATUS		+ hBase);
		InPortByte(ATA_COMMAND_STATUS	+ hBase);
		for(i=0; i< 256;i++){ //Endian swap?
			buffer[i] = InPortWord(ATA_DATA+hBase);
		}
	}
	//Clear Status register to remove pending interrupts?
	//Doesn't make sense because interrupts are disabled
	//And reading the ALT_STAT reg doesn't clear that bit.
	InPortByte(ATA_ALT_STATUS		+ hBase);
	InPortByte(ATA_COMMAND_STATUS	+ hBase);
	return 0;
}
The only thing above that needs to be explained is

Code: Select all

DeviceWait(hBase); 
Which reads the status register four times (to wait 400NS).

Like I said, VMWare eats it up like candy, but my laptop hates it. Of course, this is all after the drives have been identified (Which also fails on real hardware, at least the command does, the drive detection code works fine.)

Posted: Wed Nov 07, 2007 2:54 am
by JamesM
Of course, this is all after the drives have been identified (Which also fails on real hardware, at least the command does, the drive detection code works fine.)
Don't you think you should get the IDENTIFY command working before trying to read sectors?

Posted: Wed Nov 07, 2007 9:57 am
by Shalted
At any rate, they both work under a Virtual Machine, so I assume it's the same problem for reading a sector, that it is for reading the Device Identification data. I used the sector read code, because it's somewhat less cluttered than the DRIVE_IDENTIFY code and should be easier for people to understand when reading.

Re: Something wrong with my ATA Code.

Posted: Wed Nov 07, 2007 10:05 am
by AJ
Shalted wrote:...reads the status register four times (to wait 400NS).
I'm no hard disk expert, but 400ns seems like an extremely small delat. Certainly on a FDD controller (okay - apples and oranges...), you need to wait in the order of milliseconds. Are you sure the delay is long enough, because lack of delays seems to be a common cause of disk reads not working when transferred to real hardware. How about waiting a *ridiculously* long time (say 500msec) and gradually reducing the delay?

Cheers,
Adam

Re: Something wrong with my ATA Code.

Posted: Wed Nov 07, 2007 12:46 pm
by exkor
Shalted wrote: The only thing above that needs to be explained is

Code: Select all

DeviceWait(hBase); 
Which reads the status register four times (to wait 400NS).
Some code of mine reads status reg 10-40 times after IdentifyDevice otherwise hard drive detection fails, cd drives may require longer delay.

40 'IN' instruction is huge amount of time, so use interrupts for future and switch to some other task while waiting for the int.

And here is something from ata7 manual: if device doesn't respond for 30sec , after that you may assume that device is not present. I don't remember exact page. (But this delay maybe a bit extreme)

Posted: Wed Nov 07, 2007 1:33 pm
by Shalted
I'll try a longer delay and see if that works. I don't really want to deal with remapping the IRQs and setting up the interrupts in the Kernel Strap, I want that to be dealt with in another module, but maybe I'll setup a temporary IDT to facilitate a the IRQ signals from the harddrive. Or permenant IDT and just pass it on the the Interrupt Manager's initialization routine.

I've been going by the ATA-6 Specs, and other instructions I've seen around.

Posted: Fri Nov 09, 2007 3:24 pm
by bewing
while(InPortByte(ATA_COMMAND_STATUS + hBase) & ATA_STAT_DRQ != ATA_STAT_DRQ);
Uh? Can that possibly work? The & operator has a lower precedence than the != operator. I think you need another pair of parentheses. (And you did that kind of thing 3 times.)

And in any case, if the identify command has already crapped out, then the disk has probably turned itself off, and needs to be reset.

Posted: Mon Nov 26, 2007 1:30 am
by Shalted
You're right, the sad thing is, I knew that, and due to poor awareness of what I was writing (One of the loops even has the extra set of parentheses), I missed it.

Anyway, it was the timing, if I increase the wait time to one second, it will read data from the disk. Unfortunately, I had to implement the interrupt manager in the kernel strap in order to catch and handle the interrupts generated from the device.

Thanks.