usb_dc_stm32: Minor bugfixes #9412

atx · 2018-08-13T19:46:19Z

A few minor bugs I noticed when playing with an STM32F0 and the USB stack.

EDIT: Added missing S-o-b lines

ydamigos

Which board did you use to test it? Could you add support for this board?

ydamigos · 2018-08-16T10:36:20Z

drivers/usb/device/Kconfig

@@ -95,4 +95,10 @@ config USB_DC_STM32_DISCONN_ENABLE
 	  Say Y if your board uses USB DISCONNECT pin to enable the
 	  pull-up resistor on USB DP.

+config USB_DC_STM32_PA11_PA12_REMAP
+	bool


"prompt" is missing.

ydamigos · 2018-08-16T11:06:06Z

drivers/usb/device/usb_dc_stm32.c

 	if (LL_APB1_GRP2_IsEnabledClock(LL_APB1_GRP2_PERIPH_SYSCFG)) {
 		LL_SYSCFG_EnablePinRemap();
 	} else {
 		SYS_LOG_ERR("System Configuration Controller clock is "
-			    "disable. Unable to enable pin remapping."
+			    "disable. Unable to enable pin remapping.")


; also is missing

atx · 2018-08-16T14:30:21Z

Fixed the commits.

Which board did you use to test it? Could you add support for this board?

I'm developing on a custom STM32F042 board. My patch for the F042 is here but I basically just copied the F072 configs and haven't really checked for correctness.

ydamigos · 2018-08-17T16:47:41Z

drivers/usb/device/Kconfig

@@ -95,4 +95,11 @@ config USB_DC_STM32_DISCONN_ENABLE
 	  Say Y if your board uses USB DISCONNECT pin to enable the
 	  pull-up resistor on USB DP.

+config USB_DC_STM32_PA11_PA12_REMAP


I was thinking if this should be done in DT. We could add an optional property without value in the st,stm32-usb.yaml and add it to STM32F0 SoCs on QFN28 and TSSOP20 packages with USB controller. What do you think?

ydamigos · 2018-08-17T16:55:55Z

drivers/usb/device/usb_dc_stm32.c

+	 * */
+	for (i = 0; i < CONFIG_USB_NUM_BIDIR_ENDPOINTS; i++) {
+		k_sem_init(&usb_dc_stm32_state.in_ep_state[i].write_sem, 1, 1);
+	}


I agree that we should reset semaphores during a reset. Do we need to initialize them again? Did you try to reset them with k_sem_reset() instead?

The comment also is confusing. Something like "Reset IN endpoints semaphores" sounds better to me.

Agree k_sem_reset seems to be the right way, avoiding unbalanced sem after reset.

@atx , reason not to follow @loicpoulain & @ydamigos request?

@erwango See my earlier comment

The semaphores have maximum permitted count of 1 and k_sem_give will therefore either set them to 1 or keep them at 1. Furthermore, it's unclear to me if k_sem_reset is ISR-safe as the documentation does not explicitly say so.

atx · 2018-08-23T18:56:52Z

I have updated the branch. Not sure if moving the pin remap to the device tree is a win for now, as it requires a fixup file (such as the one in the olimexino_stm32).

ydamigos

Not sure if moving the pin remap to the device tree is a win for now, as it requires a fixup file (such as the one in the olimexino_stm32).

Pin remap is related to the SoC and not the board. It is better to add something like the following in stm32f0 fixup file:

#ifdef ST_STM32_USB_40005C00_ENABLE_PIN_REMAP
#define CONFIG_USB_ENABLE_PIN_REMAP			ST_STM32_USB_40005C00_ENABLE_PIN_REMAP
#endif /* ST_STM32_USB_40005C00_ENABLE_PIN_REMAP */

ydamigos · 2018-08-24T17:58:48Z

dts/bindings/usb/st,stm32-usb.yaml

+      type: boolean
+      category: optional
+      description: Some chips allow (or require) remapping the USB data pair to
+                   non-default pins.


Could you update the description with the reason we need this property:

For STM32F0 series SoCs on QFN28 and TSSOP20 packages enable PIN pair PA11/12 mapped instead of PA9/10 (e.g. stm32f070x6)

atx · 2018-08-25T10:37:38Z

Pin remap is related to the SoC and not the board. It is better to add something like the following in stm32f0 fixup file

Oh right, I haven't noticed that there are fixup files for individual SoCs as well.

Updated the commits.

ydamigos

While we are waiting for @loicpoulain to check the PR, could you also fix the errors reported by shippable:

Commit 870f19c038:
1: UC5 Title exceeds max length (73>72): "drivers: usb_dc_stm32: Reinitialize the write semaphores on USB bus reset"
3: UC4 Line exceeds max length (79>72): "If the user attempts to send data before the USB connection is established (see"
4: UC4 Line exceeds max length (74>72): "the HID sample for an example of such code), the DataInCallback never gets"

Commit d29d76ae7c:
3: UC4 Line exceeds max length (81>72): "The SYSCFG_CFGR1_PA11_PA12_RMP define is present even on packages where the remap"
4: UC4 Line exceeds max length (75>72): "isn't strictly required. This commit makes the remap optional based on a DT"

ydamigos

@atx Please resolve the conflicts.
@loicpoulain Could you also check it?

loicpoulain · 2018-09-26T06:45:13Z

drivers/usb/device/usb_dc_stm32.c

+	 * */
+	for (i = 0; i < CONFIG_USB_NUM_BIDIR_ENDPOINTS; i++) {
+		k_sem_init(&usb_dc_stm32_state.in_ep_state[i].write_sem, 1, 1);
+	}


Agree k_sem_reset seems to be the right way, avoiding unbalanced sem after reset.

atx · 2018-09-26T09:55:04Z

@ydamigos I have rebased on top of master
@loicpoulain Already done in the current version (altough I use k_sem_give as we need to set the semaphore to 1 on reset)

loicpoulain · 2018-09-27T07:21:27Z

@loicpoulain Already done in the current version (altough I use k_sem_give as we need to set the semaphore to 1 on reset)

Sorry but I don't see any k_sem_reset prior to the k_sem_give, have you push the new commit?

atx · 2018-09-27T07:36:21Z

Sorry but I don't see any k_sem_reset prior to the k_sem_give, have you push the new commit?

Right, sorry, I misunderstood - you want to call both k_sem_reset and k_sem_give? I sort of figured that it would be redundant, as this is a binary semaphore.

Is calling k_sem_reset in an ISR safe? The docs don't explicitly mention it.

erwango · 2018-10-08T09:14:15Z

@loicpoulain , can you check if @atx answer is ok for you?

erwango · 2018-11-08T09:53:17Z

@atx , can you rebase?

atx · 2018-11-08T10:17:24Z

@erwango Done

loicpoulain · 2018-11-08T11:18:34Z

ok it looks good.

galak

Don't think you need the ifdef in the dts_fixup.h

galak · 2018-11-08T12:47:30Z

soc/arm/st_stm32/stm32f0/dts_fixup.h

@@ -60,6 +60,10 @@
 #define CONFIG_USB_NUM_BIDIR_ENDPOINTS		ST_STM32_USB_40005C00_NUM_BIDIR_ENDPOINTS
 #define CONFIG_USB_RAM_SIZE			ST_STM32_USB_40005C00_RAM_SIZE

+#ifdef ST_STM32_USB_40005C00_ENABLE_PIN_REMAP


Why do you need the ifdef here?

#ifdefs don't do chain resolution right?

e. g.:

#define SOMETHING SOMETHINGELSE #if defined(SOMETHING) #error "Wat?" #endif

So the define() check in the driver code would always succeed.

@galak , are you good?

aescolar · 2019-03-30T11:33:10Z

@atx Please create a gitHub "bug" issue, labeled as medium priority, so it can be properly tracked.
After creating it, add a "Fixes: #XYZ" line in the PR body so it will be linked to the issue itself.
This PR was likely forgotten as there was no linked issue.
I remove the "priority: medium" label from here, as PRs priorities are not looked at by anybody following our current way of working.

You may also want to rebase the changes, as unfortunately there is new merge conflicts.

carlescufi · 2019-04-03T08:50:43Z

@atx can you please rebase this PR? Thanks!

The SYSCFG_CFGR1_PA11_PA12_RMP define is present even on packages where the remap isn't strictly required. This commit makes the remap optional based on a DT property. Also fixes syntax error caused by a missing );. Signed-off-by: Josef Gajdusek <[email protected]>

atx · 2019-04-13T20:50:42Z

@aescolar @carlescufi Rebased and created issue #15443

Hopefully nothing is broken as I don't have time to test everything properly.

zephyrbot · 2019-04-13T21:14:20Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

If the user attempts to send data before the USB connection is established (see the HID sample for an example of such code), the DataInCallback never gets called which leaves the write semaphore in a taken state forever. Signed-off-by: Josef Gajdusek <[email protected]>

stale

atx requested review from finikorg, jfischer-no, loicpoulain and ydamigos as code owners August 13, 2018 19:46

atx force-pushed the usb_dc_stm32 branch from ad1b494 to f7c5c00 Compare August 13, 2018 20:55

erwango added area: USB Universal Serial Bus platform: STM32 ST Micro STM32 labels Aug 14, 2018

ydamigos requested changes Aug 16, 2018

View reviewed changes

atx force-pushed the usb_dc_stm32 branch from f7c5c00 to ee33848 Compare August 16, 2018 14:29

ydamigos reviewed Aug 17, 2018

View reviewed changes

atx force-pushed the usb_dc_stm32 branch from ee33848 to 03d7a0c Compare August 23, 2018 18:54

ydamigos requested changes Aug 24, 2018

View reviewed changes

atx force-pushed the usb_dc_stm32 branch from 03d7a0c to bf06c71 Compare August 25, 2018 10:33

atx requested a review from erwango as a code owner August 25, 2018 10:33

atx force-pushed the usb_dc_stm32 branch 2 times, most recently from 74c11aa to baccfd8 Compare August 25, 2018 10:37

ydamigos requested changes Aug 25, 2018

View reviewed changes

atx force-pushed the usb_dc_stm32 branch from baccfd8 to 350db21 Compare August 25, 2018 17:38

erwango added priority: medium Medium impact/importance bug area: Debugging labels Aug 29, 2018

ydamigos approved these changes Sep 25, 2018

View reviewed changes

loicpoulain requested changes Sep 26, 2018

View reviewed changes

atx force-pushed the usb_dc_stm32 branch from 350db21 to d12f102 Compare September 26, 2018 09:50

atx force-pushed the usb_dc_stm32 branch from d12f102 to c6417d8 Compare November 8, 2018 10:16

loicpoulain approved these changes Nov 8, 2018

View reviewed changes

erwango approved these changes Nov 8, 2018

View reviewed changes

galak previously requested changes Nov 8, 2018

View reviewed changes

aescolar removed the priority: medium Medium impact/importance bug label Mar 30, 2019

aescolar requested a review from galak March 30, 2019 11:32

atx force-pushed the usb_dc_stm32 branch from c6417d8 to 65b146e Compare April 13, 2019 20:45

atx mentioned this pull request Apr 13, 2019

usb_dc_stm32: Missing semaphore initialization and missing pin remapping configuration #15443

Closed

atx force-pushed the usb_dc_stm32 branch from 65b146e to 48ebd3d Compare April 13, 2019 21:49

zephyrproject-rtos deleted a comment from codecov-io Apr 16, 2019

jfischer-no approved these changes Apr 17, 2019

View reviewed changes

jfischer-no mentioned this pull request Apr 17, 2019

USB DFU: STM32: usb dfu mode doesn't work #15497

Closed

galak added backport v1.14-branch bug The issue is a bug, or the PR is fixing a bug labels Apr 17, 2019

galak merged commit f01a725 into zephyrproject-rtos:master Apr 17, 2019

backporting bot mentioned this pull request Apr 17, 2019

[Backport v1.14-branch] usb_dc_stm32: Minor bugfixes #15525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usb_dc_stm32: Minor bugfixes #9412

usb_dc_stm32: Minor bugfixes #9412

atx commented Aug 13, 2018 •

edited

Loading

ydamigos left a comment

ydamigos Aug 16, 2018

ydamigos Aug 16, 2018

atx commented Aug 16, 2018

ydamigos Aug 17, 2018

ydamigos Aug 17, 2018

loicpoulain Sep 26, 2018

erwango Nov 8, 2018

atx Nov 8, 2018

atx commented Aug 23, 2018 •

edited

Loading

ydamigos left a comment

ydamigos Aug 24, 2018

atx commented Aug 25, 2018

ydamigos left a comment

ydamigos left a comment

loicpoulain Sep 26, 2018

atx commented Sep 26, 2018

loicpoulain commented Sep 27, 2018 •

edited

Loading

atx commented Sep 27, 2018

erwango commented Oct 8, 2018

erwango commented Nov 8, 2018

atx commented Nov 8, 2018

loicpoulain commented Nov 8, 2018

galak left a comment

galak Nov 8, 2018

atx Nov 8, 2018 •

edited

Loading

erwango Dec 13, 2018

aescolar commented Mar 30, 2019 •

edited

Loading

carlescufi commented Apr 3, 2019

atx commented Apr 13, 2019

zephyrbot commented Apr 13, 2019 •

edited

Loading

usb_dc_stm32: Minor bugfixes #9412

usb_dc_stm32: Minor bugfixes #9412

Conversation

atx commented Aug 13, 2018 • edited Loading

ydamigos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atx commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atx commented Aug 23, 2018 • edited Loading

ydamigos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atx commented Aug 25, 2018

ydamigos left a comment

Choose a reason for hiding this comment

ydamigos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atx commented Sep 26, 2018

loicpoulain commented Sep 27, 2018 • edited Loading

atx commented Sep 27, 2018

erwango commented Oct 8, 2018

erwango commented Nov 8, 2018

atx commented Nov 8, 2018

loicpoulain commented Nov 8, 2018

galak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atx Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aescolar commented Mar 30, 2019 • edited Loading

carlescufi commented Apr 3, 2019

atx commented Apr 13, 2019

zephyrbot commented Apr 13, 2019 • edited Loading

atx commented Aug 13, 2018 •

edited

Loading

atx commented Aug 23, 2018 •

edited

Loading

loicpoulain commented Sep 27, 2018 •

edited

Loading

atx Nov 8, 2018 •

edited

Loading

aescolar commented Mar 30, 2019 •

edited

Loading

zephyrbot commented Apr 13, 2019 •

edited

Loading