usb: device_next: allow to pass maximum possible MPS value to drivers #92831
usb: device_next: allow to pass maximum possible MPS value to drivers #92831jfischer-no wants to merge 7 commits intozephyrproject-rtos:mainfrom
Conversation
jfischer-no
added
area: USB
github-actions
Bot
added
platform: STM32
github-actions
Bot
requested review from
AlessandroLuo,
Dino-Li,
FRASTM,
GTLin08,
MarkWangChinese,
RichardSWheatley,
RuibinChang,
aaronyegx,
erwango,
fabiobaltieri,
jackrosenthal,
keith-zephyr and
mmahadevan108
| * | ||
| * @return 0 on success, all other values should be treated as error. | ||
| * @retval -ENODEV endpoint is not assigned or no configuration found | ||
| * @retval -EALREADY endpoint is already enabled |
There was a problem hiding this comment.
This -EALRADY description seems wrong. It is disable function.
| err1 = udc_ep_disable_internal(dev, USB_CONTROL_EP_IN); | ||
| err2 = udc_ep_disable_internal(dev, USB_CONTROL_EP_OUT); | ||
|
|
||
| return err1 ? err1 : err2; |
There was a problem hiding this comment.
This modifies the sequence that is performed on endpoint disable. Now the disable function will be called on both IN and OUT (before it was called first on OUT and then on IN) regardless of the returned error. Before first OUT endpoint was disabled and then IN endpoint was disabled only if OUT endpoint disable succeeded.
There was a problem hiding this comment.
I am not sure what makes more sense, but I changed it to maintain the previous behavior.
| * | ||
| * @return 0 on success, all other values should be treated as error. | ||
| * @retval -ENODEV endpoint is not assigned or no configuration found | ||
| * @retval -EALREADY endpoint is already enabled |
There was a problem hiding this comment.
Looks like copy&paste error, if endpoint is "already enabled" then disable should succeed.
jfischer-no
force-pushed
the
pr-device_next-maximum-mps
branch
from
9da6d90 to
5d918d3
Compare
| */ | ||
| int usbd_ep_enable(const struct device *dev, | ||
| const struct usb_ep_descriptor *const ed, | ||
| const uint16_t m_mps, |
There was a problem hiding this comment.
This seems to be solving the issue for DWC2 driver if the SPRAM is large enough and thresholding is not needed. However, it leaves the driver with no clue e.g. how large the RxFIFO should be until it is already too late (i.e. DWC2 driver can no longer reconfigure RxFIFO).
I think to determine the RxFIFO size, the driver would have to know beforehand (even before the first bus reset) what is the maximum possible maximum packet size of all OUT endpoints in all configurations (and how many OUT endpoints there are).
Then there is also the "do we need thresholding" problem, i.e. will the suitable RxFIFO and all the TxFIFOs fit simultaneously in SPRAM, or do we need to allocate e.g. smaller TxFIFOs and enable TX thresholding.
Do you have any idea how to solve this problem on top of this PR?
There was a problem hiding this comment.
I think to determine the RxFIFO size, the driver would have to know beforehand (even before the first bus reset) what is the maximum possible maximum packet size of all OUT endpoints in all configurations (and how many OUT endpoints there are).
I left out the number of endpoints for now, but that should not be a problem to add it.
Next, I will look at the DWC2 RxFIFO size calculation.
Then there is also the "do we need thresholding" problem, i.e. will the suitable RxFIFO and all the TxFIFOs fit simultaneously in SPRAM, or do we need to allocate e.g. smaller TxFIFOs and enable TX thresholding.
I probably will not have time to look into thresholding, but the required size information is now available.
There was a problem hiding this comment.
What is currently implemented is only suitable for OUT endpoints, but not for IN endpoints on DWC2. For IN endpoints each endpoint occupies SPRAM locations, so endpoint with wMaxPacketSize=1 requires the same amount of SPRAM locations as endpoint with wMaxPacketSize=4. Simiarly endpoint with wMaxPacketSize=5 requires as many SPRAM locations as endpoint with wMaxPacketSize=8.
There was a problem hiding this comment.
Is there anything preventing us from setting a four-byte granularity requirement for wMaxPacketSize in periodic endpoint descriptors?
There was a problem hiding this comment.
Yes. Explicit feedback wMaxPacketSize at Full-Speed is 3.
Add helper function to enable/disable control endpoint. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
The wMaxPacketSize (MPS) value of an endpoint may vary depending on the interface's alternate settings. Device controller, which uses dedicated memory for the endpoint FIFO, may end up in a locked state during endpoint reconfiguration if the allocated memory is too small but the higher memory window is still used. Although the driver could allocate a new, larger memory window, the last used window would likely remain unused, wasting a bit of dedicated RAM. Allow the stack to pass the maximum possible MPS value within alternate settings. For a controller with endpoint FIFOs and dedicated RAM, the driver should use this value for the FIFO configuration. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
To ensure that the alternate endpoint setting does not result in a memory window that is too small being allocated and locked because a higher FIFO is still in use, use the maximum MPS value for FIFO memory allocation. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
UDC controllers with internal endpoint FIFO memory may require information about the maximum possible memory usage to configure the FIFOs or verify that the memory is large enough. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Organize the initialization arguments in a structure to make it easily extensible. Since some future arguments may not be known beforehand, move the controller initialization after the stack initialization. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Drivers that depend on memory usage information should select the Kconfig option UDC_DRIVER_REQUIRES_MEMORY_STATS. To save memory or when interfaces do not have any endpoints, the user may disable memory usage information calculation in the stack. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Check that the memory usage calculated by the stack does not exceed the available DPRAM space for the interface endpoints. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
jfischer-no
force-pushed
the
pr-device_next-maximum-mps
branch
from
5d918d3 to
411cb55
Compare
|
|
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. |
4 participants
Allow the stack to pass the maximum possible MPS value within alternate
settings.