driver: usb: stm32: add partial host support #71449
Conversation
|
With a device connected to a NUCLEO-H723ZG running samples/subsys/usb/shell/host : |
zephyrbot
added
area: Devicetree Binding
zephyrbot
requested review from
FRASTM,
GeorgeCGV,
ajarmouni-st,
decsny,
galak,
gautierg-st,
jfischer-no,
kartben,
nashif and
tmon-nordic
faloj
force-pushed
the
dev/stm32_usb_host
branch
4 times, most recently
from
fe5d4be to
6c42168
Compare
faloj
changed the title
faloj
force-pushed
the
dev/stm32_usb_host
branch
from
6c42168 to
9fc0d4a
Compare
drivers/usb/uhc/uhc_stm32.c
Outdated
There was a problem hiding this comment.
can you return an error in such non-bulk case? to prevent hidden issue.
There was a problem hiding this comment.
As of now I don't think there is a way to know the desired transfer type as the USB host stack layer is still in development.
This is what is also done in uhc_max3421e.c
faloj
force-pushed
the
dev/stm32_usb_host
branch
from
9fc0d4a to
4746f50
Compare
|
Any news on this PR ? |
Agree, sorry for the delay. I'm willing to approve the driver part (after quick round of review before end of this week), but can't comment on the sample. |
|
I tested this PR with a device connected to a nucleo_h723zg running samples/subsys/usb_host/shell/: uart:~$ usbh init
host: USB host initialized
uart:~$ usbh enable
host: USB host enabled
uart:~$ usbh device address 1
host: New device address is 0x01
[00:00:25.263,000] <err> uhc: Transfer is still queued
uart:~$ usbh device descriptor device
bLength 108
bDescriptorType 250
bcdUSB 800
bDeviceClass 14
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize0 0
idVendor 178c
idProduct 2400
bcdDevice 0
iManufacturer 0
iProduct 0
iSerial 101
bNumConfigurations 130
[00:02:16.081,000] <err> uhc: Transfer is still queued
uart:~$ usbh device descriptor configuration 0
bLength 170
bDescriptorType 170
wTotalLength aaaa
bNumInterfaces 170
bConfigurationValue 170
iConfiguration 170
bmAttributes aa
bMaxPower 340 mA
[00:02:30.440,000] <err> uhc: Transfer is still queued
uart:~$ ==>There is an issue with releasing the uhc buffer . |
|
@marwaiehm-st This is generaly due to a somehow silent (or absent) usb device plugged to the host. The timeout of 5 seconds used in usbh_req_setup() (subsys/usb/host/usbh_ch9.c) beeing reached an attempt to free the buffer is done while the driver is still holding it. I am not shure I should do much about it. Maybe I can at least avoid accepting a transfer if it appears no device is plugged. But USB host implementation beeing in it's early stage of development it is not clear to me if it should be the driver responsability or not to do so. |
|
@faloj Can you rebase ? |
There was a problem hiding this comment.
Where did you get the name for this test feature?
There was a problem hiding this comment.
Why do we need a new sample that does nothing new?
drivers/usb/uhc/uhc_stm32.c
Outdated
There was a problem hiding this comment.
I have to complain again that it is not mentioned for which controller this driver is.
As far as I know there are two different ones that STM uses, Synopsys DWC USB 2.0 and something of their own (no idea if it can host). A look at stm32cube/stm32h7xx/soc/stm32h723xx.h confirms that this is a shim driver for the STM HAL DWC2 driver. We should have a native implementation for this (we will sooner or later anyway), similar to drivers/usb/udc/udc_dwc2.c (this also applies to other third party controllers or standardized designs used by many vendors, such as EHCI). So that we do not end up in the situation we have with drivers/usb/device/usb_dc_stm32.c and drivers/usb/udc/udc_stm32.c.
There was a problem hiding this comment.
I have to complain again that it is not mentioned for which controller this driver is.
Well this is quite a new requirement that you come up with.
We should have a native implementation for this
Is that a new Zephyr requirement? Can you state doc for this ?
So that we do not end up in the situation we have with
drivers/usb/device/usb_dc_stm32.canddrivers/usb/udc/udc_stm32.c.
Can you elaborate ?
There was a problem hiding this comment.
Additional note on:
We should have a native implementation for this
Why not. But this is not a valid reason to block a HAL based implementation which respect usual Zephyr criteria.
There was a problem hiding this comment.
See #61464 (comment) for more details on the constraints of not using HAL.
As mentioned, STM32 USB IPs encompasses DWC2 but also 2 ST own USB IPs, and one of those actually has host capabilities. So current HAL based host STM32 driver will anyway be required as well for this IP.
There was a problem hiding this comment.
@jfischer-no I added a comment on top of the file for more explanation on the scope of the this driver. I hope it correspond to what you were expecting.
Also can you elaborate on what is problematic with the stm32 USB device driver ? One issue I saw is that parts with multiple USB cores are not supported but I tried to adresse such issue in this driver.
Also what is there any decision regarding DWC2 USB ? In any case part of the work done here can still be usefull for the stm32 parts not based on DWC2.
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
|
Sorry I was quite busy working on other subjects lately so I had to pause the work on this for a while. I am currently on vacation but I plan to respond to @jfischer-no and work on requested modifications the second week of september. In the meantime can the stall label be removed to prevent this PR from beeing closed ? |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
faloj
force-pushed
the
dev/stm32_usb_host
branch
from
d1ea530 to
531cc89
Compare
faloj
force-pushed
the
dev/stm32_usb_host
branch
from
531cc89 to
f72f462
Compare
Add the "num-host-channels" property to "st,stm32-otghs" and "st,stm32-otgfs" compatible dts bindings and to dtsi files comprising such peripheral. Values are taken from AN4879 and from corresponding reference manual when missing. Signed-off-by: Johan Lafon <[email protected]>
Implement uhc driver for STM32. This as only been tested on a nucleo-h723zg which USB core COMPAT is st_stm32_otghs. Adjustments will be necessary in order to support other STM32 parts with st_stm32_otghs/st_stm32_otgfs USB core. Parts with st_stm32_usb USB core are not supported yet. Only control transfers are implemented. Support for bulk transfers have also been added but hasn't been fully tested due to the lack of a proper way to do so. Signed-off-by: Johan Lafon <[email protected]>
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
6 participants
Implement uhc driver for STM32. This as only been tested on a nucleo-h723zg which USB core COMPAT is st_stm32_otghs. Adjustements will be necessary in order to support other STM32 parts with st_stm32_otghs/st_stm32_otgfs USB core. Parts with st_stm32_usb USB core are not supported yet.
Only control transfers are implemented. Support for bulk transfers have also been added but hasn't been fully tested due to the lack of a proper way to do so.
Also add a USB host only shell sample.