G1-ATA bugs

[SWAT]

Hello BlueCrab, this topic for you
For some time, I use the G1-ATA driver in practice.
During this time I have found a few bugs. For the most part it was easy to fix, but there is one that I can not fix for now.

The main problems with the DMA.
Firstly you do not set the protection code (in 0xA05F74B8 reg), maybe it's not important if you boot from GD drive, but when booting from the custom BIOS it must be set. If you do not, it will appear interrupts with illegal address and overflow. In the driver, all interrupts are interpreted as complete status.
Ideally, it would be good to set the value depending on the address of the buffer, and not always put it into one space (0x0Cxxxxxx), but it is not very important.

Second, the DMA does not work properly (no matter where boot), the result data obtained a spaces. Seems DMA skip some memory, not zeros write, it just missed memory. As if there is a problem with timing or mode of transmission.
It looks something like this:

Code: Select all

100 bytes untouched space
20 bytes of data
40 bytes untouched space
20 bytes of data
100 bytes untouched space
etc...

Offsets are pieces of data coincide with reality!
I've tried and checked everything but still cannot find the cause yet.

The second problems with PIO.
There is problem with the PIO write. At the end of the function, before select the master device, you need to wait NBSY flag, otherwise in the write loops, some data may be lost.
And you need to check the buffers in PIO transfer like DMA, if it is not 2-byte aligned, you can get an exception. For himself I'm just added by-byte parsing the words, this have very small regression of speed, but fix all issues with buffers from all libraryes (freetype for example works incorrectly).

...
There is one more request to finalize the functional, it would be not bad to make a public function to obtain Max LBA and Capabilities.
All of what I wrote above, I have made ​​for himself and not insist on the same in KOS. I'm just sharing experiences.
For me the important thing to solve the problem with the DMA, it does not work with 3 different devices from other people who have tested it.

[BlueCrab]

With regard to the protection register, you'll notice that most of KOS is that way -- it doesn't mess with those registers that should be set by the BIOS. For instance, the PVR DMA code doesn't mess with the protection register for that. There's a lot of things in KOS that probably don't work 100% when booted without the BIOS (I'm sure you should have found many other such examples by now). I suppose it could be used for buffer overflow protection, but of course that is somewhat superfluous when just being careful about how the registers are set in the first place. I don't think the BIOS uses the register in this way, as IIRC, the DMA read system call just sets the protection register to cover the entire RAM.

With regard to the DMA not copying data properly... That's very odd... It worked fine on the drive that I tested it with. I'll have to look into it when I get some time, although I may not be able to do much without hardware that exhibits this problem...

With regard to word-alignment for PIO transfers, that is, once again, a general problem with the CPU on the Dreamcast. Without either allocating internal buffers to copy things into or taking a performance hit by doing everything byte-by-byte, there's really not much that can be done to fix that. I suppose the fix would be to test if the address is aligned and if not do the read/write from/to an internal buffer then memcpy it. That seems like a reasonable enough fix that I could probably implement relatively easily for PIO at least. By the way, many CPUs do the same thing -- throw an exception on unaligned accesses. Heck, I think that even x86 does such reads/writes slower than if they were aligned properly.

With regard to switching devices, the real solution would be to defer switching devices until it is really necessary (i.e, don't switch the device back to the GD-ROM unless someone is actually trying to use the GD-ROM). Of course, this would require routing all the GD-ROM access code through g1ata, which wouldn't be particularly hard (I have had code to do all the GD-ROM accesses without the syscalls for a while), I haven't really looked terribly much into doing so for various reasons (like not wanting to break everything that depends on the syscalls being used ). I guess I could add a function to g1ata for setting the device and just have the existing GD-ROM code call it, which would be a good compromise.

I'll look into the other stuff you mentioned up there too as I get time, but I'm not entirely sure when that will be.

[SWAT]

What the drive you tested?
I'm use HDD Seagate barracuda 7200 40 GB.
And other people tested Matrox HDD and simplest CF card.
Result is the same for everyone.

About protection code. In syscalls and docs this reg setup every time before transfer...
Is it enough to set this register once?
If yes, I'm just change value in my bootstrap for custom bios and forget about it.

[BlueCrab]

The protection register doesn't need to be reset later if it is set to cover the entirety of RAM. You'd only have to set it once. It doesn't change once you set it, unless you set it again.

As for what drive I used, I'm not entirely sure off the top of my head. I'll check when I get back home this evening. I do know that it was a 7200RPM drive that supports UDMA and that was made around 2005. I don't recall the brand off the top of my head (I want to say it was a Seagate Barracuda drive as well, but I might be mistaken).

[SWAT]

Forgot one more thing ...
It would be good to give the driver the opportunity to work with two devices (not necessarily with the GD drive), for example, with two hard drives. For those who have a bios mod, it will be very good. And it can be help with some *only master* adapters eg. SATA->IDE...

[BlueCrab]

So, the drive I was using for my testing is a Seagate Barracuda 7200.7 80GB drive, manufactured in August of 2005 (if I'm reading the date code on it correctly).

[SWAT]

ST380817A
ST380013A
ST380011A

Which of them do you have? Firmware version?
I have tester with same HDD (ST380011A), and he have same problem with DMA.
Can you give me working for you compiled test? You can send data by dc-load.

[BlueCrab]

I have the ST380011A drive with firmware 8.16.

My test program wasn't anything special... I pretty much just read the sectors with DMA and with PIO and just compared the two methods with a memcmp.

EDIT: One idea that had happened into my head is that there is something going wonky with the CPU's cache on your end. Upon further thought though, that seems unlikely, since the untouched spaces should be 32-bytes (or multiples thereof) if that was the case...

[BlueCrab]

So, here's the source code to a test program I just threw together that works fine on my end:

Code: Select all

/* KallistiOS ##version##

   hdtest.c
   Copyright (C) 2014 Lawrence Sebald

   This example program simply attempts to read some sectors from the first
   partition of an ATA device attached to G1, both over PIO and DMA and then
   compares the timing information from both. PIO reads seem to run at about
   3.5 MB/sec, whereas DMA gets around 12.5 MB/sec (quite the improvement).
*/

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>
#include <errno.h>

#include <dc/g1ata.h>
#include <dc/cdrom.h>

#include <arch/timer.h>
#include <arch/types.h>

#include <kos/dbglog.h>
#include <kos/blockdev.h>

static unsigned char dmabuf[1024 * 512] __attribute__((aligned(32)));
static unsigned char piobuf[1024 * 512] __attribute__((aligned(32)));
static unsigned char tmp[512] __attribute__((aligned(32)));

int main(int argc, char *argv[]) {
    kos_blockdev_t bd_pio, bd_dma;
    uint64 spio, epio, sdma, edma, timer;
    uint8_t pt;

    dbglog(DBG_DEBUG, "Starting G1 ATA test program...\n");
    g1_ata_init();

    /* Grab the blockdevs that we'll use to access the partitions. */
    dbglog(DBG_DEBUG, "Looking for first partition...\n");
    if(g1_ata_blockdev_for_partition(0, 0, &bd_pio, &pt)) {
        dbglog(DBG_DEBUG, "Couldn't get PIO blockdev for partition!\n");
        return -1;
    }

    if(g1_ata_blockdev_for_partition(0, 1, &bd_dma, &pt)) {
        dbglog(DBG_DEBUG, "Couldn't get DMA blockdev for partition!\n");
        return -1;
    }

    /* For some reason, the first DMA read takes a while... So, read one sector
       and discard it so to not mess up the timing stuff below. */
    if(bd_dma.read_blocks(&bd_dma, 1024, 1, tmp)) {
        dbglog(DBG_DEBUG, "Couldn't read block 1024 by dma: %s\n",
               strerror(errno));
        return -1;
    }

    /* Read blocks 0 - 1023 by DMA and print out timing information. */
    dbglog(DBG_DEBUG, "Reading 1024 blocks by DMA!\n");

    sdma = timer_ms_gettime64();
    if(bd_dma.read_blocks(&bd_dma, 0, 1024, dmabuf)) {
        dbglog(DBG_DEBUG, "couldn't read block by DMA: %s\n", strerror(errno));
        return -1;
    }
    edma = timer_ms_gettime64();
    timer = edma - sdma;

    dbglog(DBG_DEBUG, "DMA read took %llu ms (%f MB/sec)\n", timer,
           (512 * 1024) / ((double)timer) / 1000.0);

    /* Read blocks 0 - 1023 by PIO and print out timing information. */
    dbglog(DBG_DEBUG, "Reading 1024 blocks by PIO!\n");

    spio = timer_ms_gettime64();
    if(bd_pio.read_blocks(&bd_pio, 0, 1024, piobuf)) {
        dbglog(DBG_DEBUG, "couldn't read block by PIO: %s\n", strerror(errno));
        return -1;
    }
    epio = timer_ms_gettime64();
    timer = epio - spio;

    dbglog(DBG_DEBUG, "PIO read took %llu ms (%f MB/sec)\n", timer,
           (512 * 1024) / ((double)timer) / 1000.0);

    /* Check the buffers for consistency... */
    if(memcmp(piobuf, dmabuf, 1024 * 512)) {
        dbglog(DBG_DEBUG, "Buffers do not match?!\n");
    }
    else {
        dbglog(DBG_DEBUG, "Both buffers matched!\n");
    }

    /* Clean up... */
    g1_ata_shutdown();
    cdrom_shutdown();
    cdrom_init();

    return 0;
}

I've attached a stripped elf of the binary to this post (zipped). If you run it with dcload, you should get output similar to the following:

Code: Select all

[lj@yuki:~] $ dc-tool -t 192.168.1.203 -x hdtest.elf 
Console enabled
Upload <hdtest.elf>
File format is ELF, start address is 0x8c010000
Section .text, lma 0x8c010000, size 215112
Section .init, lma 0x8c044848, size 54
Section .fini, lma 0x8c044880, size 42
Section .rodata, lma 0x8c0448ac, size 36892
Section .eh_frame, lma 0x8c04d8c8, size 316
Section .ctors, lma 0x8c04da84, size 8
Section .dtors, lma 0x8c04da8c, size 8
Section .jcr, lma 0x8c04da94, size 4
Section .data, lma 0x8c04daa0, size 12100
Section .got, lma 0x8c0509e4, size 12
transferred 264548 bytes at 340747.681407 bytes / sec
Executing at <0x8c010000>
Sending execute command (0x8c010000, console=1, cdfsredir=0)...executing

--
KallistiOS Git revision 4514543: Tue Apr 22 12:48:35 EDT 2014
  lj@yuki.local:/Users/lj/Projects/git/KallistiOS
thd: pre-emption enabled, HZ=100
maple: active drivers:
    Dreameye (Camera): Camera
    Sound Input Peripheral: Microphone
    PuruPuru (Vibration) Pack: JumpPack
    VMU Driver: Clock, LCD, MemoryCard
    Mouse Driver: Mouse
    Keyboard Driver: Keyboard
    Controller Driver: Controller
  DMA Buffer at ac19a0e0
vid_set_mode: 640x480IL NTSC
dc-load console support enabled
maple: attached devices:
Starting G1 ATA test program...
g1_ata_scan: found device with LBA48: 156250000
Looking for first partition...
Reading 1024 blocks by DMA!
DMA read took 41 ms (12.787512 MB/sec)
Reading 1024 blocks by PIO!
PIO read took 150 ms (3.495253 MB/sec)
Both buffers matched!
arch: shutting down kernel
maple: final stats -- device count = 0, vbl_cntr = 60, dma_cntr = 59
vid_set_mode: 640x480IL NTSC

[SWAT]

I had to rebuild the example with the addition of the protection code. But it does not matter, it's works ...
I found the reason, I allocate a buffer with the memalign(32, 512)
Thanks for help!

Code: Select all

$ dc-tool -t 192.168.1.110 -x hdtest2.elf
Console enabled
Chroot enabled
Upload <hdtest2.elf>
File format is ELF, start address is 0x8c010000
Section .text, lma 0x8c010000, size 269184
Section .init, lma 0x8c051b80, size 54
Section .fini, lma 0x8c051bb8, size 42
Section .rodata, lma 0x8c051be4, size 36460
Section .eh_frame, lma 0x8c05aa50, size 116
Section .ctors, lma 0x8c05ab44, size 8
Section .dtors, lma 0x8c05ab4c, size 8
Section .jcr, lma 0x8c05ab54, size 4
Section .data, lma 0x8c05ab60, size 12100
Section .got, lma 0x8c05daa4, size 12
transferred 317988 bytes at 760693.165919 bytes / sec
Executing at <0x8c010000>
Sending execute command (0x8c010000, console=1, cdfsredir=0)...executing

--
KallistiOS Git revision 924da83: Пн. апр. 21 17:57:36 NOVT 2014
  swat@SWAT-PC:/usr/local/dc/kos/kos
thd: pre-emption enabled, HZ=100
maple: active drivers:
    Dreameye (Camera): Camera
    Sound Input Peripheral: Microphone
    PuruPuru (Vibration) Pack: JumpPack
    VMU Driver: Clock, LCD, MemoryCard
    Mouse Driver: Mouse
    Keyboard Driver: Keyboard
    Controller Driver: Controller
  DMA Buffer at ac1a71a0
vid_set_mode: 640x480 NTSC
dc-load console support enabled
maple: attached devices:
  A0: Dreamcast Controller          (01000000: Controller)
  A1: Visual Memory                 (0e000000: Clock, LCD, MemoryCard)
  A2: Visual Memory                 (0e000000: Clock, LCD, MemoryCard)
Starting G1 ATA test program...
g1_ata_scan: found device with LBA48: 78165360
Looking for first partition...
Reading 1024 blocks by DMA!
DMA read took 44 ms (11.915637 MB/sec)
Reading 1024 blocks by PIO!
PIO read took 150 ms (3.495253 MB/sec)
Both buffers matched!
arch: shutting down kernel
maple: final stats -- device count = 3, vbl_cntr = 56, dma_cntr = 55
vid_set_mode: 640x480 NTSC

[BlueCrab]

Glad that you got it working, but memalign shouldn't really have anything to do with it. That really makes me think it's some kind of cache-related issue, to be honest.

I probably should invalidate the operand cache in the read sector function...

[SWAT]

I probably should invalidate the operand cache in the read sector function...

Yep, it's solve that problem.

BTW, DMA write have a some bug as is in PIO write. I wrote about it in the first post.

[SWAT]

I see your last update. Thanks.
You forgot to add at end of function g1_ata_write_lba_dma:

Code: Select all

    /* Wait for the device to signal that it has finished writing the data. */
    g1_ata_wait_nbsy();

Its need and here too.

Why you do not add dcache_inval_range in g1_ata_read_lba_dma? It's really solve cache issues.

P.S.
A small bug report about the GD-ROM driver.
At changing sector size to 2352, you need replace:

Code: Select all

params[2] = 1024;

to

Code: Select all

params[2] = 0;

Otherwise it's not works.

[BlueCrab]

I see your last update. Thanks.
You forgot to add at end of function g1_ata_write_lba_dma:

Code: Select all

    /* Wait for the device to signal that it has finished writing the data. */
    g1_ata_wait_nbsy();

Its need and here too.

The problem is that that would break non-blocking mode (which is already probably broken, but that's beside the point). With the recent changeset however, that problem is mostly solved already though (really the "correct" fix at this point is to remove the switching of the device select register at the ends of the functions, since the cdrom code now does that itself when you try to access the cd). That should be pushed in relatively short order.

Why you do not add dcache_inval_range in g1_ata_read_lba_dma? It's really solve cache issues.

This set of changes was already mostly done before I had thought about invalidating the cache, so I wanted to push them up as soon as I finished testing things before I made any other changes. As soon as I finish with the stuff mentioned above, that'll get done too.

P.S.
A small bug report about the GD-ROM driver.
At changing sector size to 2352, you need replace:

Code: Select all

params[2] = 1024;

to

Code: Select all

params[2] = 0;

Otherwise it's not works.

I'm actually pretty sure that you are wrong there... A couple of different sources say that params[2] must be one of a certain set of values for the sector read to work properly in that mode, and I can confirm that those parameters that are set work perfectly for the Dreamcast port of Yabause. Technically, params[2] should be set based on the type of disc, as the second source there notes, but 0 isn't a valid value there at all, I'm pretty sure.

[SWAT]

Ok, thanks.

I'm actually pretty sure that you are wrong there... A couple of different sources say that params[2] must be one of a certain set of values for the sector read to work properly in that mode, and I can confirm that those parameters that are set work perfectly for the Dreamcast port of Yabause. Technically, params[2] should be set based on the type of disc, as the second source there notes, but 0 isn't a valid value there at all, I'm pretty sure.

0 - It's a value for Auto Detect and it's works fine.
1024 - Doesn't work with audio track, I have reading error. Maybe it's works with RAW data reading, I don't know.
So maybe for audio need 512 value and for data 1024. But auto detect is easy to use.

[BlueCrab]

Ok, thanks.

I'm actually pretty sure that you are wrong there... A couple of different sources say that params[2] must be one of a certain set of values for the sector read to work properly in that mode, and I can confirm that those parameters that are set work perfectly for the Dreamcast port of Yabause. Technically, params[2] should be set based on the type of disc, as the second source there notes, but 0 isn't a valid value there at all, I'm pretty sure.

0 - It's a value for Auto Detect and it's works fine.
1024 - Doesn't work with audio track, I have reading error. Maybe it's works with RAW data reading, I don't know.
So maybe for audio need 512 value and for data 1024. But auto detect is easy to use.

Indeed with 8192 as param[1], 0 does work like an auto detect, but with 4096 as param[1], param[2] is checked to see if it is in a certain set of values (0x0200, 0x0400, 0x0600, 0x0800, 0x0a00, 0x0c00), and if not the parameters are not written to memory for later syscalls and the change data type syscall effectively does nothing. If you look at the first source I linked to there (the word different links to it), look for the function gdGdcChangeDataType() to see what the code for the syscall does.

This also matches up with what I've found from playing around with disassembling the bios myself, as well as from playing around with direct access to the GD-ROM drive.

[megavolt85]

Hi, this is worked and tested code

Code: Select all

  /* Re-init the drive, e.g., after a disc change, etc */
int g1_reinit() {
    int rv = ERR_OK;
    int r = -1, cdxa;
    uint32  params[4];
    int timeout;

    mutex_lock(&mutex);

    /* Try a few times; it might be busy. If it's still busy
       after this loop then it's probably really dead. */
    timeout = 10 * 1000 / 20; /* 10 second timeout */

    while(timeout > 0) {
        r = g1_exec_cmd(CMD_INIT, NULL);

        if(r == 0) break;

        if(r == ERR_NO_DISC) {
            rv = r;
            goto exit;
        }
        else if(r == ERR_SYS) {
            rv = r;
            goto exit;
        }

        /* Still trying.. sleep a bit and check again */
        thd_sleep(20);
        timeout--;
    }

    if(timeout <= 0) {
        rv = r;
        goto exit;
    }

    /* Check disc type and set parameters */
    gdc_get_drv_stat(params);
    if(sector_size != 2352) {
        cdxa = params[1] == 32;
        params[0] = 0;              /* 0 = set, 1 = get */
        params[1] = 8192;           /* ? */
        params[2] = cdxa ? 2048 : 1024;     /* CD-XA mode 1/2 */
        params[3] = sector_size;        /* sector size */
    }
    else {
        params[0] = 0;
        params[1] = 4096;
        params[2] = 0;
        params[3] = 2352;
    }

    if(gdc_change_data_type(params) < 0) {
        rv = ERR_SYS;
        goto exit;
    }

exit:
    mutex_unlock(&mutex);
    return rv;
}  

[BlueCrab]

The code in the BIOS will not actually do anything with the change data type syscall if you pass 0 as params[2] with params[1] set to 4096. If it works at all it is just a happy coincidence that I'm not counting on.

Passing 0 there is not valid at all. It does not function as an auto-select with params[1] set to 4096. It does not do anything useful at all.

Heck, this is basically what the code for that system call does (taken from here):

Code: Select all

int
gdGdcChangeDataType(arg1)	/* _8c0034a6 */
   int *arg1;
{

   GDS *my_gds;

   if (allocate_GD() != 0) {
      return (4);
   }

   my_gds = get_GDS();

   switch (arg1[0]) {
      case 0:
	 if (arg1[1] & 0x1000 == 0) {
	    switch (arg1[2]) {
	       case 0x0200:
	       case 0x0400:
	       case 0x0600:
	       case 0x0800:
	       case 0x0a00:
	       case 0x0c00:
		  break;
	       default:
		  release_GD();
		  return (-1);
		  break;
	    }
	 }
	 my_gds->sector_mode = arg1[1] | arg1[2];
	 my_gds->sector_size = arg1[3];
	 release_GD();
	 return (0);
	 break;
	 
      case 1:
	 arg1[1] = my_gds->sector_mode & 0xf000;
	 arg1[2] = my_gds->sector_mode & 0x0e00;
	 arg1[3] = my_gds->sector_size;
	 release_GD();
	 return (0);
	 break;

      default:
	 release_GD();
	 return (-1);
	 break;
   }

}

Note the if that checks if 0x1000 (4096) is set in arg1[1]. Note how the switch does not have a value of 0. I have verified that this matches the behavior in the BIOS. If it somehow works for you, that's great, but I know it does not work in general.

I can also explain a bit about how that works from knowing the low-level details of how to communicate with the drive... Basically, the upper byte of the sector_mode variable match up with the parameters fed to the drive for reading (ignore the lower byte, so for simplicity, I'll only consider that byte in the rest of this).

The parameter is divided up into 8 bits as follows:
bit 0 = set to 1 for MSF addressing, 0 for FAD addressing (you'd have to do this in values[1] if you wanted MSF addressing for some reason).
bit 1 - 3 = sector data type (000 = "auto detect", which is not valid as I mentioned, 001 = Red Book (CDDA), 010 = Yellow Book Mode 1 (CD-ROM), 011 = Yellow Book Mode 2 (CD-ROM), 100 = Green Book (Mode 2 XA 1), 101 = Green Book (Mode 2 XA 2), 110 = Mode 2 Non-XA, 111 = Invalid).
bit 4 - 7 = requested data (0001 = All data, 0010 = Data areas only, 0100 = Subheaders only, 1000 = Headers only -- you can OR any of the last 3 together to get bits and pieces of the data).

So, if you want all the data, you have to set values[1] to 4096 (or 4097 for MSF addressing), which matches up with 0001 in bits 4-7 above. Since values[1] is 4096, that if statement up there is run through, which checks the sector data type against the valid types up there.

[SWAT]

Just try to read sectors of audio track on MIL-CD or GD-R and you'll realize that your theory is broken by practice.

Note the if that checks if 0x1000 (4096) is set in arg1[1]. Note how the switch does not have a value of 0. I have verified that this matches the behavior in the BIOS. If it somehow works for you, that's great, but I know it does not work in general.

In BootROM reverse code, switch-case just checking for valid value for param[2] only if param[1] is NOT SET to 4096.
if (arg1[1] & 0x1000 == 0)

[megavolt85]

If it works at all it is just a happy coincidence that I'm not counting on.

It is the only way with which I managed to read track02.raw (sector size 2352) from disk GD-ROM. The theory it is good, but practice is more real