New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32F3: Correct handling of USB ISTR and endpoint registers #4149

Merged
merged 1 commit into from May 8, 2017

Conversation

Projects
None yet
7 participants
@monkiineko
Contributor

monkiineko commented Apr 10, 2017

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.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 11, 2017

@0xc0170 0xc0170 added the needs: CLA label Apr 11, 2017

@0xc0170 0xc0170 requested a review from LMESTM Apr 11, 2017

@monkiineko

This comment has been minimized.

Contributor

monkiineko commented Apr 11, 2017

@0xc0170 CLA has been signed under the mbed.org username "bscott".

@0xc0170 0xc0170 added needs: review and removed needs: CLA labels Apr 12, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 20, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 24, 2017

bump @bcostm @adustm @jeromecoutant @LMESTM Could you review?

targets/TARGET_STM/TARGET_STM32F3/device/stm32f3xx_hal_pcd.h Outdated
@@ -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__))))

This comment has been minimized.

@bcostm

bcostm Apr 26, 2017

Contributor

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.

targets/TARGET_STM/TARGET_STM32F3/device/stm32f3xx_hal_pcd.h Outdated
@@ -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))

This comment has been minimized.

@bcostm

bcostm Apr 26, 2017

Contributor

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.

targets/TARGET_STM/TARGET_STM32F3/device/stm32f3xx_hal_pcd.h Outdated
#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))

This comment has been minimized.

@bcostm

bcostm Apr 26, 2017

Contributor

same as above

STM32F3: Correct handling of USB ISTR and endpoint registers
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.

@monkiineko monkiineko force-pushed the monkiineko:master branch to 7f12ad2 Apr 26, 2017

@monkiineko

This comment has been minimized.

Contributor

monkiineko commented Apr 26, 2017

@bcostm Thanks for the review.
I've pushed a new commit with the addition of the "MBED fix" comments, as you requested.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels May 2, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 2, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented May 2, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 123

All builds and test passed!

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 4, 2017

@bcostm could you please confirm you are happy with the review rework?

@bcostm

This comment has been minimized.

Contributor

bcostm commented May 4, 2017

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.

@theotherjimmy theotherjimmy merged commit 6aca976 into ARMmbed:master May 8, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sg- sg- removed the ready for merge label May 8, 2017

@bcostm

This comment has been minimized.

Contributor

bcostm commented May 18, 2017

ST_INTERNAL_REF 33668

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment