Different transfer type using uhci

[Bonfra]

Here a bunch of relevant files:

general usb/uhci stuffs: /drivers/usb
specific msd stuffs: /drivers/storage/usb_msd.c

I tried to manyally set the interface and the configuration but nothing changed

[BenLunt]

A quick glance at your code, especially line 226, it looks like you are not using Queues at all, and placing each TD in a consecutive Frame Pointer. i.e.: If you have three TDs, the first one is in the first Frame's Frame Pionter, the second in the second, and so on.

This will work for Control transfers, though it is very inefficient, however, the UHCI expects (and if memory serves, requires) all BULK transfers to be within a Queue. Since this is the case, QEMU might be expecting the same thing.

It is highly recommended that you use Queues. There are so many advantages to using Queues, especially when Short Packet Detection is used. Control and Bulk transfers use Queues to their advantage, allowing Breadth or Depth operation, as well as Short Packet operation.

Am I correct in this assumption that you are not using Queues?

Ben

[Bonfra]

Yes, you are right, I'm not using queues. Since I'm still in a prototyping state I didn't want to implement a fancy algorithm to insert packets in the framelist and went for the lazy approach. Remaining lazy, can I create a single queue and put all TDs in there?
As soon as this thing works fine I'll implement something better (like the implementation you suggest in your awesome book :-) )

UPDATE:
Ok so, first of, I don't know if my code is particularly bad or something but it got *a lot* slower executing the queue... (code already pushed)
Now instead of some weird non-sense data in the csw I get all zeros (but control transfers keeps working with this new approach since I'm still able to enumerate the device)

[BenLunt]

...it got *a lot* slower executing the queue...

Remember, if you have only one Queue in the Stack Frame, pointed to by only one Frame Pointer, *and* the current TD in the Queue's Vert Pointer is Breadth First, the controller will only execute one TD every 1024mS, or one a second. That is pretty dang slow. However, if you set the Depth First bit in each TD, the controller will try to execute all TDs in this Queue within this Frame Time, which now is pretty dang fast (for legacy USB anyway).

In Line 207, set the Depth First bit as well.

Also, while you are learning, there is nothing keeping you from pointing each Frame Pointer to the same Queue Head. This way, the controller will try to execute all TDs in that Queue, always, no matter where the controller is in the Stack Frame.

Then using the example code in my book, you can insert Queues in that first Queue as many times as you wish, removing them when completed.

For learning purposes, and for the current task at hand, you need one Queue with three (3) TDs in it, using Depth First processing. The first TD is the CBW, the second is the Data IN, and the last is the CSW. The first TD points to the second, the second to the third, and the third has the T bit set.

After you get that, try a technique that is a little more appropriate. Use one Queue with the Vertical Direction pointing to two (2) TDs. The first TD is the CBW, the second is the Data IN. Then the Horizontal pointer of the Queue points to a single TD, the CSW TD. This technique will get you started on learning Short Packet Detection.

Ben

[Bonfra]

Now setting the Depth First bit is blazing fast! I'll hazard that it's even faster than before with all separate TDs, well it makes sense since it tries to execute more than one in a single frame.

Now I think I understood why I'm reading garbage data. What I'm doing right now is sending the CBW and, right after, the CSW. I'm not sending any other IN packets between these two special packets. It makes sense that if I want to receive something I need to ask for that something, but how much should I ask? I mean, I'm using this bulk protocol to send an SCSI command so depending on the command I'll get a certain response length. Should I switch on the command and send N IN packets (depending on the command) before the CSW?

[nullplan]

If I understood the bulk-only specification correctly, you will always transfer a data buffer in the middle. For SCSI commands, the size is implicit in the command, or given in an "allocation length" parameter (e.g. for IDENTIFY), and for READ DATA and WRITE DATA, the data block is one disk block (which you can get from the command that tells you the disk size, I forget which it was).

So you send the CBW, then send or receive data, then receive the CSW, and only then do you know if the data you just received was garbage.

[Bonfra]

So you send the CBW, then send or receive data, then receive the CSW, and only then do you know if the data you just received was garbage.

Ok, so I tried to implement some code that should do the trick but I always get a CSW of all zeros...

Code: Select all

bool send_scsi_command(void* device, scsi_command_t* command, void* data, size_t transfer_length)
{
    usb_msd_device_t* dev = (usb_msd_device_t*)device;

    usb_msd_cbw_t cbw;
    cbw.tag = dev->tag++;
    cbw.signature = 0x43425355;

    cbw.transfer_length = transfer_length;
    cbw.flags = command->write ? CBW_DIRECTION_OUT : CBW_DIRECTION_IN;
    cbw.lun = 0;
    cbw.command_length = command->packet_len;
    memset(cbw.scsi_command, 0, 16);
    memcpy(cbw.scsi_command, command->packet, command->packet_len);

    usb_transfer_bulk_out(dev->device->bus, dev->device->addr, dev->endpoint_out->descriptor.endpoint_number, &cbw, sizeof(usb_msd_cbw_t));
   
    usb_endpoint_descriptor_t* ep = command->write ? &dev->endpoint_out->descriptor : &dev->endpoint_in->descriptor; 
    if(command->write)
        usb_transfer_bulk_out(dev->device->bus, dev->device->addr, ep->endpoint_number, data, transfer_length);
    else
        usb_transfer_bulk_in(dev->device->bus, dev->device->addr, ep->endpoint_number, data, transfer_length);
   
    usb_msd_csw_t csw;
    usb_transfer_bulk_in(dev->device->bus, dev->device->addr, dev->endpoint_in->descriptor.endpoint_number, &csw, sizeof(usb_msd_csw_t));
    // here csw is all set to zero

    return csw.signature != 0x53425355 || csw.status != 0;
}

[BenLunt]

So, a few more questions/comments then:
1) Do you verify that the CSW TD was executed by the controller?
2) If so, what is the result in the Status byte in the TD?
3) Are you sending physical addresses to the controller?
4) Can you fill the CSW buffer with 0xFF's first, then see if it still returned 0x00's
5) Did the DATA IN return anything? Can you fill that buffer with 0xFF before-hand, to see if the controller actually writes something to it?
6) The Controller will only return a CSW if the DATA IN(s) didn't Stall.
7) If you are waiting for an interrupt to fire to know when to check the CSW, you have to be sure that it is the correct interrupt. An already executed TD, with the IOC bit set, will tell the controller to fire an interrupt at end of frame, even though that TD has already been executed. Therefore, if the controller is running through a different set of TDs, ones you already have executed, and at least one of them has the IOC bit set, the controller will fire an interrupt at end of frame, which might be in a frame well before it gets to your CSW TD. Because of this, you really need to remove a queue once you execute it. If this is the case, your code may think that the interrupt has indicated your CSW has been executed, yet it really hasn't yet, hence the zeros. (However, I think I remember you stating you don't use interrupts)

Dump each TD before and after it is executed and post here. i.e.: Dump contents of the TD just before you place it in the Queue, then after the controller executes it dump it again to show the changes.

Ben

[Bonfra]

1) It should, the last TD of the sequence has the IOC bit set, so (even if I'm not using interrupt) i spin on that bit for the last TD until its cleared.
2) at the end of the post, the dumps of the TDs before and after the execution
3) yes
4, 5, 6) Both the CSW and the data are left untouched: if I fill the memory with 0xFFs before the transfer it has the same value also after the transfer
7) Yes you remember correctly, I'm not using interrupts. Anyway I execute only one queue at a time and completely clear the framelist before starting a new one.



It appears that all the transfers have the stall bit set.
Sidenote: also the transfer for the set interface has the stall bit set after the transfer

[BenLunt]

It appears that all the transfers have the stall bit set.
Sidenote: also the transfer for the set interface has the stall bit set after the transfer

Print the packets (before and after) on all that you send. Find which is the first it Stalls on. This will tell you which packet is in error.

USB 2.0 states that if a device only supports the default interface and you send a SET INTERFACE command, the device may return a STALL on the STATUS Stage. You need to clear that stall by either sending the CLEAR_STALL command or any other Control SETUP packet (along with its optional DATA packets, and the STATUS packet).

Do you have a bootable image file you can put on your github page? I don't 'make' other peoples image files via their code. Too many dependencies, etc, that can go wrong. If you make the image and .zip it down, it probably will be less than a 1meg or so. This way I get the exact test image that you have.

Ben

[Bonfra]

Ok i pushed an image with all the logging you requested in this post. This is the command line i use with qemu:

Code: Select all

qemu-system-x86_64 -M q35 -m 512M -nic none -drive file=./BonsOS.img,index=0,media=disk,format=raw -drive if=none,id=stick,format=raw,file=./stick.img -device piix3-usb-uhci,id=usbport -device usb-storage,bus=usbport.0,drive=stick

Also i suggest to look at the serial output instead of the VGA display, there is possible to scroll through the logs ;)

About the set interface not being supported thing... i tried removing the part where I send that packet but the stall still appears in the subsequent bulk part.

Find which is the first it Stalls on. This will tell you which packet is in error.

It either is the set interface one, or if I skip it, the first bulk one.
In general is the first packet after the set configuration transfer

EDIT:
so I spotted a little bug where before passing the device to the correct driver the correct config/interface is selected, but even with this fix nothing changes in the final result. could it be that I'm not setting the correct configuration so whatever i do next is invalid?

[BenLunt]

First, I don't believe that you have uploaded your 'usb.h' file to the drivers/usb folder of the github. It may be missing. I would like to see it though, please.

Second, I tried to load your .img file in my Bochs USB Test suite and it got as far as 'No compatible vesa mode'.

Code: Select all

Booting from Hard Disk...
VBR loaded!
Jumping to VBR
Booting
CPU Vendor: GenuineIntel
No compatible vesa mode

I tried a few different VGA mode setups and it still failed at that point. Do you require a specific mode, or can you let it be more generic, or simply skip the Vesa stuff altogether?

Ben

[Bonfra]

First, I don't believe that you have uploaded your 'usb.h' file to the drivers/usb folder of the github. It may be missing. I would like to see it though, please.

I keep headers and sources in separate folders, here it is.

I tried a few different VGA mode setups and it still failed at that point. Do you require a specific mode, or can you let it be more generic, or simply skip the Vesa stuff altogether?
Ben

Yea..It needs a very specific 1024x768x32 vesa mode. But anyway I pushed a new image that ignores vesa alltogher and only print to the serial output so now you should be able to get over that phase

[BenLunt]

Sorry, but this is the process of debugging.

My serial out has two lines:

Code: Select all

BonsOS successfully booted
src/kernel.c:146 Couldn't load init process

Did you intend to use the 'a:' in "a:/bin/init.elf". The image file is a bit larger than a floppy, though this assumes you use 'a:' as the first floppy. You may use 'a:' as the first hard drive, and there is nothing wrong with that. Is your code able to use "/bin/init.elf" instead?

Anyway, sorry. This is the processes of testing and debugging your code.

Ben

[Bonfra]

Sorry i didn't specify, the OS only knows how to read from sata ahci drives for the moment.
anyway the USB thing should happen before it tries to load the init process (actually even before the successfully booted message), so something more should appear if a controller with something attached is found