Bug fix for STM32H5 need few cycles for RX PMA descriptor to update

[Lurcy38]

dcd_stm32_fsdev.c: Fix a bug seen with stm32h5xxx when the driver is compiled with CubeIde O1/O2/O3
patch fixes a bug discovered when I build my application for the speed. The new timing introduces a bug in the function pcd_get_ep_rx_cnt().
The function doesn't return the correct value.
After discussion with my colleagues, they told me that this issue is fixed with a workaround.
This workaround is ported from stm32h5xx_hal_pcd.h : PCD_GET_EP_RX_CNT (ST HAL for H5)
Issue observed on Windows 10 and stm32h573i_dk and tud_task() scheduled by IT or by a FreeRtos task. The device is not migrated the USB device is not visible.
I didn’t surrounded the patch by a, ifdef specific to the H5 because I think this issue could be present in various MCU family and is pretty neutral (just a very small delay)

• IAR Warning: Fixed due to an boolean operation between enum (Pa089)*
  Just fix painful waring on IAR 9.40.1

[HiFiPhile]

Thanks for your PR, but it seems there are some unwanted commits included, could you make a rebase ?

[Lurcy38]

rebase on the master
thanks

[Lurcy38]

The pre-commit fails because the build doesn't produce .OUT , it seems that there is no relationship with my patches
my local build produce a fonctional .out on the board stm32h573i_dk
Could please tell me what is wrong ?
I noticed that with GCC cubeide, the build produces some warnings , especialy with __ARMCC_VERSION
in the initial H5 commit, i added -D __ARMCC_VERSION=0 as a workaround to remove this warning

[HiFiPhile]

@hathach it seems something is wrong with unit-test.

[hathach]

just re-run and it works now, I also notice @HiFiPhile try to re-run it a few time before. I guess action has some kind of issues previously. Let me try to reproduce the issue with O1 and freertos example, which example you are testing with ? Otherwise, it would be helpful if you could provide reference to Manual where it say some minimum cycle is required for accessing pma

[Lurcy38]

Hello Tinyusb,
I didn’t reproduce the issue with examples such as uac2_headset using the same toolchain. (ie GCC CubeIDE in command line)
But the issue is clearly visible in our product.
tinyUsb examples implementations are minimalist, the tu_task() is called in a bare-metal loop with no other heavy tasks sing the CPU.
Our application is different in several points.

1. The USB clocking probably different (using HSI48 + CRS)
2. CPU at 250MHZ with loaded at 60% (signal processing)
3. Our application uses CDC HID and UAC2 in/out
4. Tu_task is scheduled using 2 flavours according to the build config:
   a. Using a FreeRtos Task with a delay of 1ms (high priority)
   b. Bare-metal, using a GPIO _EXTI of lower prio triggered by the USB IT
   Both configurations show the issue on H5 and the patch fix it .

The patch comes from the ST standard HAL USB implementation.
tinyusb/hw\mcu\st\stm32h5xx_hal_driver\Inc\stm32h5xx_hal_pcd.h:539
I will try to identify who has done this patch and were come from the issue related to this implementation. I will let you know if I have an answer.
Regards

[hathach]

thank you very much for the PR. After reviweinng your provided code from stm hal driver and a bit of reading (go through Manual but it does not give any hint). This PR is totally spot-on

• this only occurs on recent MCUs such as G0, U5, H5: https://github.com/search?q=org%3ASTMicroelectronics+PCD_RX_PMA_CNT&type=code (PCD_RX_PMA_CNT is only defined for these)
• older MCUs: e.g F3 has no issue with this, PCD_GET_EP_RX_CNT is a clearly defined https://github.com/search?q=org%3ASTMicroelectronics%20PCD_GET_EP_RX_CNT&type=code

So I think it is safe to conclude this change is required for not only H5 but also all with FSDEV_BUS_32BIT.

I still have a question regarding if we need nop in the loop. Let me know if it could work for you without nop ?

[HiFiPhile]

I believe I saw something suspicious with G0 during enumeration, it could be this but I'm not working on it for the moment...

[Lurcy38]

Comment : #2515 (review)

Hello Tinyusb team

Happy this patch fixes a potential issue for other MCU families.

I would like to notice, I found a second issue on synopsis IP ( for H735 & H757), but I didn’t provide the path because the issue vanished with an rebase upstream. But it was characterized with a previous build in CubeIDE and -O3 -oFast.
The characterization was an in an infinite loop in reset_core()
->while (dwc2->grstctl & GRSTCTL_CSRST) {}
I am uncertain that the issue still present. Again, I found the fix in the stm hal h7 source code.
tinyusb\src\portable\synopsys\dwc2\ dcd_dwc2.c: USB_CoreReset()

Adding before the line causing the issue :
uint32_t count = 0U;
/* Wait for AHB master IDLE state. */
do
{
if (++count > 200000U)
{
return ;
}
}
while ((USBx->GRSTCTL & USB_OTG_GRSTCTL_AHBIDL) == 0U);
Probably the delay of this code is too long :)
Thanks again for you work and your amazing support
Regards

[hathach]

Comment : #2515 (review)

Hello Tinyusb team

Happy this patch fixes a potential issue for other MCU families.

I would like to notice, I found a second issue on synopsis IP ( for H735 & H757), but I didn’t provide the path because the issue vanished with an rebase upstream. But it was characterized with a previous build in CubeIDE and -O3 -oFast. The characterization was an in an infinite loop in reset_core() ->while (dwc2->grstctl & GRSTCTL_CSRST) {} I am uncertain that the issue still present. Again, I found the fix in the stm hal h7 source code. tinyusb\src\portable\synopsys\dwc2\ dcd_dwc2.c: USB_CoreReset()

Adding before the line causing the issue : uint32_t count = 0U; /* Wait for AHB master IDLE state. */ do { if (++count > 200000U) { return ; } } while ((USBx->GRSTCTL & USB_OTG_GRSTCTL_AHBIDL) == 0U); Probably the delay of this code is too long :) Thanks again for you work and your amazing support Regards

thank we can review this if you could make an PR for this as well. Please confirm if dropping 'nop' work for you, so that we could merge this fix.

[hathach]

perfect, thank you for the fix

[hathach]

FYI, this seems to be mention in H503 Errata https://www.st.com/resource/en/errata_sheet/es0561-stm32h503cbebkbrb-device-errata-stmicroelectronics.pdf

[HiFiPhile]

FYI, this seems to be mention in H503 Errata https://www.st.com/resource/en/errata_sheet/es0561-stm32h503cbebkbrb-device-errata-stmicroelectronics.pdf

So maybe the delay is not enough for low speed ?

[hathach]

yeah, H5 running at max 250Mhz, which is 4ns per cycle, the above loop do check, minus, then jump which take 3 cycles per run, therefore only delay 3410 = 120ns, I guess other interrupt handler contribute to the delay. We may consider to add back a few nop.

For low speed, it is definitley not enough, we should check the curent linkspeed and do delay accordingly, we need to burn some cycles anyway. I will push an update for this later.

[hathach]

I made an follow up to move delay to the interrupt handler instead. Per errata we should delay first before any attempt to read pma region. This assume read rx count is always the first access to the region (which is true most of time but not always). Also do some reformat and rename the header.

Note: initially I want to check the link speed then delay more if low speed, however, a quick check from manual does not show how link speed is detected, and low speed is super rare these days, so we will skip it for now. Feel free to make an PR if you know how to get the link speed.