STM32F3: Correct handling of USB ISTR and endpoint registers #4149
Conversation
|
Can you please sign https://developer.mbed.org/contributor_agreement/ ? |
|
@0xc0170 CLA has been signed under the mbed.org username "bscott". |
|
bump @bcostm @adustm @jeromecoutant @LMESTM Could you review? |
| @@ -232,7 +232,7 @@ typedef struct | |||
| * @{ | |||
| */ | |||
| #define __HAL_PCD_GET_FLAG(__HANDLE__, __INTERRUPT__) ((((__HANDLE__)->Instance->ISTR) & (__INTERRUPT__)) == (__INTERRUPT__)) | |||
| #define __HAL_PCD_CLEAR_FLAG(__HANDLE__, __INTERRUPT__) ((((__HANDLE__)->Instance->ISTR) &= (uint16_t)(~(__INTERRUPT__)))) | |||
| #define __HAL_PCD_CLEAR_FLAG(__HANDLE__, __INTERRUPT__) ((((__HANDLE__)->Instance->ISTR) = (uint16_t)(~(__INTERRUPT__)))) | |||
There was a problem hiding this comment.
LGTM
Can you add a comment at the end of the line like: "// MBED fix". This will help when we will update the Cube HAL files with newer version.
| @@ -622,9 +622,9 @@ PCD_StateTypeDef HAL_PCD_GetState(PCD_HandleTypeDef *hpcd); | |||
| * @retval None | |||
| */ | |||
| #define PCD_CLEAR_RX_EP_CTR(USBx, bEpNum) (PCD_SET_ENDPOINT((USBx), (bEpNum),\ | |||
| PCD_GET_ENDPOINT((USBx), (bEpNum)) & 0x7FFFU & USB_EPREG_MASK)) | |||
| ( PCD_GET_ENDPOINT((USBx), (bEpNum)) | USB_EP_CTR_TX ) & ~USB_EP_CTR_RX & USB_EPREG_MASK)) | |||
There was a problem hiding this comment.
LGTM but I need to check with the original writter of this file to see if it is 100% safe.
Can you add a comment at the end of the line like: "// MBED fix". This will help when we will update the Cube HAL files with newer version.
| #define PCD_CLEAR_TX_EP_CTR(USBx, bEpNum) (PCD_SET_ENDPOINT((USBx), (bEpNum),\ | ||
| PCD_GET_ENDPOINT((USBx), (bEpNum)) & 0xFF7FU & USB_EPREG_MASK)) | ||
| ( PCD_GET_ENDPOINT((USBx), (bEpNum)) | USB_EP_CTR_RX ) & ~USB_EP_CTR_TX & USB_EPREG_MASK)) |
The USB ISTR register consists of a mix of bits that are write-zero-to-clear and read only bits. As such, to clear a bit in the ISTR, you should simply write the bitwise-NOT of the bit to clear. Previously, the __HAL_PCD_CLEAR_FLAG() macro would do a bitwise-AND with the ISTR register contents to clear a bit, but this could result in another bit being inadvertently cleared if it is set by hardware between the read and the write of the ISTR register. Similarly, the USB endpoint registers have two bits that are write-zero-to-clear, USB_EP_CTR_RX and USB_EP_CTR_TX, but the PCD_CLEAR_RX_EP_CTR() and PCD_CLEAR_TX_EP_CTR() macros wrote back the last read value for one of these bits when clearing the other bit. This could result in inadvertent clearing of one of these bits if it were set by the hardware between the read and the write. These macros have now both been adjusted to always write one to the bit not being cleared to prevent inadvertent clears.
|
@bcostm Thanks for the review. |
|
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
|
@bcostm could you please confirm you are happy with the review rework? |
|
I didn't receive any feedback from ST guys in charge of this file but tests made by @monkiineko are ok so let's merge it! Thanks. |
|
ST_INTERNAL_REF 33668 |
Description
The USB ISTR register consists of a mix of bits that are
write-zero-to-clear and read only bits. As such, to clear a bit in
the ISTR, you should simply write the bitwise-NOT of the bit to clear.
Previously, the __HAL_PCD_CLEAR_FLAG() macro would do a bitwise-AND
with the ISTR register contents to clear a bit, but this could result
in another bit being inadvertently cleared if it is set by hardware
between the read and the write of the ISTR register.
Similarly, the USB endpoint registers have two bits that are
write-zero-to-clear, USB_EP_CTR_RX and USB_EP_CTR_TX, but the
PCD_CLEAR_RX_EP_CTR() and PCD_CLEAR_TX_EP_CTR() macros wrote back the
last read value for one of these bits when clearing the other bit.
This could result in inadvertent clearing of one of these bits if it
were set by the hardware between the read and the write. These macros
have now both been adjusted to always write one to the bit not being
cleared to prevent inadvertent clears.
Status
READY
Migrations
NO
Related PRs
None
Todos
None
Deploy notes
None
Steps to test or reproduce
This check-in corrects very rare, timing specific USB issues of bits inadvertently getting cleared if they are set by the hardware in between read and write instructions when clearing other bits in the same register. Testing was done by creating a USB HID device and test software that would simultaneously transfer about 1000 packets per seconds in both IN and OUT directions. With this change, over 10 million packets were transferred without any lost packets or other behavioural issues detected.