dmitrystu / libusb_stm32

Lightweight USB device Stack for STM32 microcontrollers
Apache License 2.0
721 stars 165 forks source link

STM32L151C8 pma_write and pma_read BUG? #103

Closed covsh closed 3 years ago

covsh commented 3 years ago

Hello. I am using STM32L151C8 with usbd_stm32l100_devfs.c driver. Noticed that pma_write and pma_read don't read the data correctly. If we look into STM32 HAL Library function looks like:

void USB_ReadPMA(USB_TypeDef *USBx, uint8_t *pbUsrBuf, uint16_t wPMABufAddr, uint16_t wNBytes)
{
  uint32_t n = (uint32_t)wNBytes >> 1;
  uint32_t BaseAddr = (uint32_t)USBx;
  uint32_t i, temp;
  __IO uint16_t *pdwVal;
  uint8_t *pBuf = pbUsrBuf;

  pdwVal = (__IO uint16_t *)(BaseAddr + 0x400U + ((uint32_t)wPMABufAddr * PMA_ACCESS));

  for (i = n; i != 0U; i--)
  {
    temp = *(__IO uint16_t *)pdwVal;
    pdwVal++;
    *pBuf = (uint8_t)((temp >> 0) & 0xFFU);
    pBuf++;
    *pBuf = (uint8_t)((temp >> 8) & 0xFFU);
    pBuf++;

#if PMA_ACCESS > 1U
    pdwVal++;
#endif
  }

  if ((wNBytes % 2U) != 0U)
  {
    temp = *pdwVal;
    *pBuf = (uint8_t)((temp >> 0) & 0xFFU);
  }
}

pdwVal pointer is incremented twice (PMA_ACCESS is 2)

But in pma_read (and pma_write)

static uint16_t pma_read (uint8_t *buf, uint16_t blen, pma_rec *rx) {
    uint16_t tmp;
    uint16_t *pma = (void*)(USB_PMAADDR + 2 * rx->addr);
    uint16_t rxcnt = rx->cnt & 0x03FF;
    rx->cnt &= ~0x3FF;
    for(int idx = 0; idx < rxcnt; idx++) {
        if ((idx & 0x01) == 0) {
            tmp = *pma++;
        }
        if (idx < blen) {
            buf[idx] = tmp & 0xFF;
            tmp >>= 8;
        } else {
            return blen;
        }
    }
    return rxcnt;
}

pma is incremented only ones Adding pma++; after tmp = *pma++; solving problem.

Surprisingly usbd_stm32l100_devfs_asm.S work correct

dmitrystu commented 3 years ago

OMG. Some things from ST make me crazy. Is this fix works for the multiple endpoints? I have no L151 in my lab, so I can't do a test.

covsh commented 3 years ago

OMG. Some things from ST make me crazy. Is this fix works for the multiple endpoints? I have no L151 in my lab, so I can't do a test.

I have Control Endpoint and 2 Bulk IN endpoints. Fix works well.

dmitrystu commented 3 years ago

Could you please test with the assembly version of the driver? Looks like it's correct for L151. https://github.com/dmitrystu/libusb_stm32/blob/0a0a24f590844a9dab1557be054d43c128d0d0a8/src/usbd_stm32l100_devfs_asm.S#L419 and https://github.com/dmitrystu/libusb_stm32/blob/0a0a24f590844a9dab1557be054d43c128d0d0a8/src/usbd_stm32l100_devfs_asm.S#L499

For L100 it works well in both cases. Perhaps it's a cause that i didn't discover this bug.

covsh commented 3 years ago

Just testing assembler version. pma_write and pma_read works well. But my BULK endpoint didn't works.

Endpoints configure as:

usbd_ep_config(dev, 0x02, USB_EPTYPE_BULK /*| USB_EPTYPE_DBLBUF*/, 0x40);
usbd_ep_config(dev, 0x05, USB_EPTYPE_BULK /*| USB_EPTYPE_DBLBUF*/, 0x40);
usbd_reg_endpoint(dev, 0x02, txd1_ep_cb);
usbd_reg_endpoint(dev, 0x05, txd2_ep_cb);

And i have STALL packet on USB analyzer on both endpoints. image Seems with asm driver endpoint didn't config correctly (C version works well)

I not familiar with asm, its hard to debug this for me

dmitrystu commented 3 years ago

Confirmed for large (>=64 bytes) endpoints.

dmitrystu commented 3 years ago

Looks like it's fixed. Could you please do an independent test then i will merge this branch.

covsh commented 3 years ago

Test done. Asm and C version works well. Thanks for fixing.