Xilinx / embeddedsw

Xilinx Embedded Software (embeddedsw) Development
Other
929 stars 1.06k forks source link

xusbps_ch9 command verifier tests failing #277

Open veljkosvica opened 9 months ago

veljkosvica commented 9 months ago

In the "/drivers/usbps/examples /xusbps_ch9.c" I found a few things that make my usb device fail the Chapter 9 tests, I would consider them bugs. Firstly, the endianess of the endpoint status seems to be wrong : ` case XUSBPS_STATUS_ENDPOINT: { u32 Status; int EpNum = SetupData->wIndex;

                    Status = XUsbPs_ReadReg(InstancePtr->Config.BaseAddress,
                                XUSBPS_EPCRn_OFFSET(EpNum & 0xF));

                    if (EpNum & 0x80) { /* In EP */
                        if (Status & XUSBPS_EPCR_TXS_MASK) {
                            *((u16 *) &Reply[0]) = 0x0100;
                        } else {
                            *((u16 *) &Reply[0]) = 0x0000;
                        }
                    } else {    /* Out EP */
                        if (Status & XUSBPS_EPCR_RXS_MASK) {
                            *((u16 *) &Reply[0]) = 0x0100;
                        } else {
                            *((u16 *) &Reply[0]) = 0x0000;
                        }
                    }
                    break;
                }`

What worked for me was just changing the two bytes around: if(EpNum & 0x80) { // In EP if(Status & XUSBPS_EPCR_TXS_MASK) { *((u16 *) &Reply[0]) = 0x0001; // Endpoint is halted } else { *((u16 *) &Reply[0]) = 0x0000; // Endpoint is not halted } } else { // Out EP if(Status & XUSBPS_EPCR_RXS_MASK) { *((u16 *) &Reply[0]) = 0x0001; // Endpoint is halted } else { *((u16 *) &Reply[0]) = 0x0000; // Endpoint is not halted } } Furthermore, for the usb device to be conform, it needs to allow for other states then the configured state, so apart from allowing value 1, value zero also needs to be processed correctly where the device is put into unconfigured state: ` case XUSBPS_REQ_SET_CONFIGURATION:

        /*
         * Only allow configuration index 1 as this is the only one we
         * have.
         */
        if ((SetupData->wValue & 0xff) != 1) {
            Error = 1;
            break;
        }`

And lastly get_config is returning alt settings instead of current config: ` case XUSBPS_REQ_GET_CONFIGURATION:

        if (InstancePtr->AppData != NULL) {

            /* When we run CV test suite application in Windows, need to
             * add GET_CONFIGURATION command to pass test suite
             */
            *((u8 *) &Reply[0]) = XUsbPs_GetConfigDone((XUsbPs *)InstancePtr);
            Status = XUsbPs_EpBufferSend((XUsbPs *)InstancePtr, 0, Reply,
                             SetupData->wLength);
        } else {
            Response = (u8)InstancePtr->CurrentAltSetting;
            XUsbPs_EpBufferSend(InstancePtr, 0,
                        &Response, 1);
        }
        break;`

So it should be: Response = (u8)InstancePtr->CurrentConfig;

mubinsyed commented 9 months ago

Hi @veljkosvica,

Thanks for reporting issue. We will analyze and fix it in 2024.1 release.