Problem with reading atapi

[Klakap]

Good day!

I have problem with my atapi driver. It is return 0 forever in qemu and Bochs and 65535 in Virtualbox. Please where is bug?

Code: Select all

void atapi_read(uint16_t byte_count, uint32_t sector) {
    //select drive master
    outb(0x176, 0xE0);

    //Use PIO
    outb(0x171, 0);

    //Maximal byte count
    outb(0x174, (byte_count & 0xff));
    outb(0x175, (byte_count >> 8));

    //ATAPI packet command
    outb (0x177, 0xA0);

    tp("debug 1");

    //Wait
    while( (inb(0x177) & 0x88)!=0x08 ) {}

    //Command set
    outw(0x170, 0xA8);
    outw(0x170, 0);
    outw(0x170, (unsigned char)(sector >> 24));
    outw(0x170, (unsigned char)(sector >> 16));
    outw(0x170, (unsigned char)(sector >> 8));
    outw(0x170, (unsigned char)(sector >> 0));

    tp("debug 2");

    //Wait for irq
    while( ide_secondary_interrupt==0 ) {} 

    //Read lenght of input/output
    uint16_t size = ( ((unsigned short)(inb(0x175) << 8)) | ((unsigned short)inb(0x174)) );
    uint16_t lenght = (size / 2);

    tp_var(size);

    //Write or read data
    for(int i=0; i<lenght; i++) {
        cdrom_buffer[i]=inw(0x170);
    }

    tp("debug 3");
} 

In terminal, Bochs is print this:

Code: Select all

00095487733i[HD   ] READ(12) with transfer length <= 0, ok (0)
00095488831e[HD   ] IO read(0x0170) with drq == 0: last command was a0h
...

Virtualbox output(I want read 256 bytes):

[zity]

Hi Klakap,

First of all, before doing a drive select, you should check that the controller is ready, and afterwards ensure that the drive select succeeded.

In your code, the ATAPI packet is wrong. An ATAPI packet normally consists of 12 bytes (in rare cases 16 bytes, you can check this in the data returned by IDENTIFY command), sent to the controller as 6 words. The 12 bytes should look like this:

Code: Select all

packet[0] = ATAPI_CMD_READ;
packet[1] = 0x0;
packet[2] = ((sector >> 24) & 0xFF);
packet[3] = ((sector >> 16) & 0xFF);
packet[4] = ((sector >> 8) & 0xFF);
packet[5] = ((sector >> 0) & 0xFF);
packet[7] = ((count >> 16) & 0xFF);
packet[6] = ((count >> 24) & 0xFF);
packet[8] = ((count >> 8) & 0xFF);
packet[9] = ((count >> 0) & 0xFF);
packet[10] = 0x0;
packet[11] = 0x0;

Moreover, make sure the byte_count is 2048, otherwise it will also fail.

Perhaps you should take a look in the wiki, there are several good articles dealing with ATA and ATAPI: https://wiki.osdev.org/Category:ATA

EDIT: I would also suggest that you #define the different port offsets, such that it's easier to read the code.

[Klakap]

Thank you! I make little change in my code:

Code: Select all

    outw(0x170, 0xA8);
    outw(0x170, (unsigned short)(sector >> 16));
    outw(0x170, (unsigned short)(sector >> 0));
    outw(0x170, (unsigned short)(sector >> 16));
    outw(0x170, (unsigned short)(sector >> 0));
    outw(0x170, 0);

and byte_count is 2048. Bochs now havent any error messages, but size is 0. Please what I am doing wrong?

[zity]

You are still constructing the packet incorrectly. First notice that I have two different variables in the code (sector and count). Sector is the LBA and count is the number of sectors you want to read. If you only want to read a single sector, I believe the code should be something like:

Code: Select all

outw(0x170, 0xA8);
outw(0x170, ((sector >> 8) & 0xFF00) | ((sector >> 24) & 0xFF));
outw(0x170, ((sector << 8) & 0xFF00) | ((sector >> 8) & 0xFF));
outw(0x170, 0);
outw(0x170, 1);
outw(0x170, 0);

I did not test this, but hopefully I got the bitshifting correct.

[Klakap]

Very thank you! But variabile size is still 0 and I cant read anything. Please what is wrong?

[Klakap]

Hm. I change my for cycle on this:

Code: Select all

    for(int i=0; i<2048; i++) {
        cdrom_buffer[i]=inw(0x170);
    }

and it seems, then it is working good! But I am little confused for variabile size - it is in my code with value 0. Please, what is with it?

[zity]

It's difficult to say without more information. The for-loop should be from 0 to 1024, because you are reading words.

Before sending the ATAPI packet, you should set "ide_secondary_interrupt = 0" because otherwise it might be in a unknown state (unless you made sure to set it to zero after the last interrupt was fired).

Perhaps you already did this, but make sure ide_secondary_interrupt is defined as volatile.

[Klakap]

Very thank you! This is my code:

Code: Select all

#define ATAPI_MASTER 0xE0
#define ATAPI_SLAVE 0xF0

#define ATAPI_PIO_MODE 0
#define ATAPI_DMA_MODE 1

#define ATAPI_PRIMARY 0x1F0
#define ATAPI_SECONDARY 0x170

uint16_t cdrom_buffer[2048];

volatile uint8_t ide_secondary_interrupt=0;

void atapi_read(uint32_t sector, uint16_t base, uint8_t drive) {
    //select drive
    outb(base+6, ATAPI_MASTER);

    //Use PIO
    outb(base+1, ATAPI_PIO_MODE);

    //Maximal byte count
    outb(base+4, (1024 & 0xff));
    outb(base+5, (1024 >> 8));

    //ATAPI packet command
    outb (base+7, 0xA0);

    //Wait
    while( (inb(base+7) & 0x88)!=0x08 ) {}

    //Command set
    outw(base+0, 0xA8);
    outw(base+0, ((sector >> 8) & 0xFF00) | ((sector >> 24) & 0xFF));
    outw(base+0, ((sector << 8) & 0xFF00) | ((sector >> 8) & 0xFF));
    outw(base+0, 0);
    outw(base+0, 1);
    outw(base+0, 0);

    //Wait for irq
    while( ide_secondary_interrupt==0 ) {}

    //Read lenght of input/output
    uint16_t size = ( ((unsigned char)(inb(base+5) << 8)) | ((unsigned char)inb(base+4)) );
    uint16_t lenght = (size / 2);

    tp_var(size);  //this output is 0

    //Write or read data
    for(int i=0; i<1024; i++) {
        cdrom_buffer[i]=inw(base+0);
    }

    //Prepare for next command
    ide_secondary_interrupt=0;

} 

It work, but variabile size is 0.

[zity]

What I wanted was to insert "ide_secondary_interrupt = 0" like this:

Code: Select all

    //Wait
    while( (inb(base+7) & 0x88)!=0x08 ) {}

    // Reset interrupt variable before sending ATAPI packet
    ide_secondary_interrupt = 0;

    //Command set
    outw(base+0, 0xA8);
    outw(base+0, ((sector >> 8) & 0xFF00) | ((sector >> 24) & 0xFF));
    outw(base+0, ((sector << 8) & 0xFF00) | ((sector >> 8) & 0xFF));
    outw(base+0, 0);
    outw(base+0, 1);
    outw(base+0, 0);

I assume you installed an interrupt handler that sets "ide_secondary_interrupt = 1" when an interrupt has fired.

EDIT: Now I also noticed that you changed the byte count to 1024, instead of 2048. The code should be:

Code: Select all

    outb(base+4, (2048 & 0xff));
    outb(base+5, (2048 >> 8));

[Klakap]

Very thank you! All work good, but size is still 0. I am confused for it.

[BenLunt]

First let me apologize for not getting back with you on the other thread. I only get around here about once a week or so.

Second, zity has made a few good comments and hopefully has pointed you in the correct direction, though I would like to emphasize his/her remarks about using #defines. It sure makes it a lot easier.

Anyway, let me comment within your code.

Very thank you! This is my code:

Code: Select all

// You don't need these.  Your 'drive' parameter is either 0 or 1, is it not?
//#define ATAPI_MASTER 0xE0
//#define ATAPI_SLAVE 0xF0

#define ATAPI_PIO_MODE 0
#define ATAPI_DMA_MODE 1

// You should really send the base address along with the call (as you do now).  These two values below
//  are for Legacy drives only and could be anything on modern machines...
//#define ATAPI_PRIMARY 0x1F0
//#define ATAPI_SECONDARY 0x170

// You only need 2048 bytes, not 2048 words, but too much is always better than too little.
uint16_t cdrom_buffer[2048];

// if you use the BOOL typedef here, the compiler will use the "best" sized variable for a "TRUE" and "FALSE"
volatile uint8_t ide_secondary_interrupt=0;

// see comments below for why you need this
uint8_t global = 0xFF;

// sector is probably a zero based lba, correct?
// base is probably ATAPI_PRIMARY or ATAPI_SECONDARY, correct?
// drive is probably 0 or 1, hopefully?
void atapi_read(uint32_t sector, uint16_t base, uint8_t drive) {
    
    //select drive
    // As zity mentioned, you should check to see if it actually was selected successfully
    // Also, I have mentioned before, selecting the drive takes time.  You should have a
    // 'global' variable to hold the value written below.  This way if the drive has already
    //   been selected, you don't select it again.
    uint8_t select = 0xE0 | (drive << 4) | ( (sector & 0x0F000000) >> 24);
    if (global != select) {
      outb(base+6, select);
      mdelay(2); // delay 2mS
      global = select;
    }
    // Also, since you are sending to an ATAPI device, using a 12-byte/16-byte packet, there
    //   is no such thing as a 28-bit LBA command.  Therefore, the  "((sector & 0x0F000000) >> 24)" is redundant.
    
    // now you should wait for the controller to not by busy.  Again, selecting the drive takes time...
    
    //Use PIO
    outb(base+1, ATAPI_PIO_MODE);
    // wouldn't the following look a lot better and easier to understand when reading it?
    //#define  ATA_FEATURES         0x001    // wpc4/features register
    //outb(base + ATA_FEATURES, ATAPI_PIO_MODE);
    
    //Maximal byte count
    outb(base+4, (1024 & 0xff));
    outb(base+5, (1024 >> 8));
    // the above (as zity mentioned) should be 2048 since it is a "byte" count...
    
    // the SECTOR register ( offset 2 ) should be zero
    // the LOW BYTE register ( offset 3 ) is not used, but should still be zero
    
    //ATAPI packet command
    outb (base+7, 0xA0);

    //Wait
    while( (inb(base+7) & 0x88)!=0x08 ) {}
    // again, what if the controller never becomes non-busy or the DRQ bit never becomes active????
    
    //Command set
    //outw(base+0, 0xA8);
    //outw(base+0, ((sector >> 8) & 0xFF00) | ((sector >> 24) & 0xFF));
    //outw(base+0, ((sector << 8) & 0xFF00) | ((sector >> 8) & 0xFF));
    //outw(base+0, 0);
    //outw(base+0, 1);
    //outw(base+0, 0);

    // Try:
    // of coarse placing your packet bytes in a byte array at "packet"
    for (i=0; i<ata->packet_size; i+=2)
      outpw(base + ATA_DATA, * (uint16_t *) &packet[i]);
    // for example, the READ TOC command might look like:
    // uint8_t packet[ATAPI_MAX_PACKET_SIZE];
    // memset(packet, 0, ATAPI_MAX_PACKET_SIZE);
    // packet[ 0] = ATAPI_CMD_READ_TOC;
    // if (msf_flag)
    //   packet[ 1] = 2;
    // packet[ 6] = 0;
    // packet[ 7] = (uint8_t) ((buflen >>  8) & 0xFF);
    // packet[ 8] = (uint8_t) ((buflen >>  0) & 0xFF);
    // packet[ 9] = (uint8_t) (format <<  6);
    // also, please note that this assumes a specific packet type.  The IDENTIFY command will
    //  return the type of packet (SCSI, RBC, etc) that you should send.
    
    //Wait for irq
    while( ide_secondary_interrupt==0 ) {}
    // what if the IRQ never fires????
    
    // It would be a good idea to check the DRQ bit here as well.
    
    //Read length of input/output
    uint16_t size = ( ((unsigned char)(inb(base+5) << 8)) | ((unsigned char)inb(base+4)) );
    //  ******
    //  Okay, here is why your size == 0....
    //    Let's look at what the first few tokens of your code above does:
    //       ((unsigned char)(inb(base+5) << 8))
    //    The code will input a byte from the port
    //    The compiler will then take that value and make it a machine sized type (probably 32-bit)
    //    The compiler will then shift that value 8 bits to the left.
    //    We now have a value that takes 16-bits to store.
    //    * You then cast that value back to a byte, which in turn, zeros the value because you just shifted it to the left 8 bits.......
    //    Your 'size' will never be greater than 0xFF and since the count of words read is (should be) 2048 which is 0x0800 in hex, the lower half will be zero...
    //    Hence, this is why you are getting a value of zero as the length....
    //
    uint16_t length = (size / 2);
    
    // ??????????????
    // I don't know what this does....
    tp_var(size);  //this output is 0

    // Write or read data
    // You get the length above, why not use it here?
    for (int i=0; i<1024; i++) {
        cdrom_buffer[i]=inw(base+0);
    }
    
    //Prepare for next command
    ide_secondary_interrupt=0;

} 

It work, but variabile size is 0.

Does this help?

Ben
- http://www.fysnet.net/osdesign_book_series.htm

[Klakap]

Very thank you! This really help me. I change my code and now is all working good. But I have one next question. I want write driver for read/write with DMA but on PCI I have not ATA device. Please, must I specialy defined then I want use DMA? Or something other?

[BenLunt]

I want write driver for read/write with DMA but on PCI I have not ATA device.

Sorry, I don't quite understand that question. You want to use DMA transfers, but can't find an ATA device when you enumerate the PCI?

Please, must I specialy defined then I want use DMA? Or something other?

To use DMA, you *must* enumerate the PCI so that you can get the Bus Master I/O address. Also, not all controllers/devices support DMA. For example, some older drives only support non-DMA reads and writes. i.e.: PortIO only. Second, you have to program the DMA Bus Master registers which is a bit more work.

Unless you have a well written scheduler, there is no advantage to using DMA. For example, if you use DMA and then sit and wait for the interrupt, why not just use Port I/O? The DMA Bus Master was/is used so that once you start the command, your scheduler can move on to something else and come back only when the transfer has been completed.

- Ben
http://www.fysnet.net/osdesign_book_series.htm

[Klakap]

Thank you for explanation. I now think than I dont need DMA.