usb: dc_dw: Introduce transfer method

[loicpoulain]

Transfer is an important concept of USB specification.
During a transfer, several packets can be transmitted.
Today the usb API only provides ep_read and ep_write
functions but the transfer concept is missing.

This is typically ok for basic drivers which don't have
to take care of how data is transferred/split.
However there are some drivers like CDC EEM, in which
transfer concept is important for packet management.

Moreover, current ep-write and ep_read methods have a
different implementation in usb_dc_dw and usb_dc_stm32
device drivers. For example usb_dc_dw supports only
1-data-packet transfers due to its implementation.
This can increase latency and reduce performance.

I think this is something we need to fix/improve by
implementing better transfer management.

This patch introduces usb_dc_ep_transfer method which
can be used to configure IN/OUT transfers of any size.

This method has asynchronous and synchronous mode.
Synchronous mode waits for transfer completion before
returning. Asynchronouse mode (irq safe) configures
the transfer and returns immediately, the provided
callback will be then called on transfer completion.

This patch preserves 'legacy' ep_read/write behavior.

Note that for now this method is local to dc_dw device
driver, however the goal would be to expose this function
as a new USB device driver API method so that class
drivers use it.

Signed-off-by: Loic Poulain loic.poulain@linaro.org

[loicpoulain]

• @jfischer-phytec-iot, @finikorg

[loicpoulain]

any feedback on this ?

[jfischer-no]

unfortunately had no time for review, please give me a few more days.

[loicpoulain]

I would like to continue 'cleaning' the stack, and so would like ack/nack on this patch. If you need help on USB domain maintaining, please let me know.

[jfischer-no]

Note that for now this method is local to dc_dw device
driver, however the goal would be to expose this function
as a new USB device driver API method so that class
drivers use it.

I have just seen that the patch is for the dw driver, I thought you done it only for the STM32 (#5214). If it is necessary, then I tend to changing the API, as only to patch the drivers.

[codecov-io]

Codecov Report

Merging #5231 into master will decrease coverage by 0.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5231      +/-   ##
==========================================
- Coverage   54.59%   54.32%   -0.28%     
==========================================
  Files         457      457              
  Lines       43298    43298              
  Branches     8302     8302              
==========================================
- Hits        23638    23521     -117     
- Misses      19521    19638     +117     
  Partials      139      139

Impacted Files	Coverage Δ
tests/kernel/obj_tracing/src/philosopher.c	72.22% <0%> (-16.67%)	⬇️
subsys/net/ip/rpl.h	78% <0%> (-8%)	⬇️
subsys/net/ip/rpl.c	19.63% <0%> (-6.04%)	⬇️
include/net/net_pkt.h	85.4% <0%> (-3.65%)	⬇️
tests/net/rpl/src/main.c	84.78% <0%> (-2.72%)	⬇️
subsys/net/ip/net_if.c	61.74% <0%> (-0.82%)	⬇️
subsys/net/ip/net_pkt.c	56.32% <0%> (-0.57%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8d40df...be27879. Read the comment docs.

[finikorg]

I am generally OK with these changes.

[finikorg]

I would separate comments from the code by either tabs or spaces, not mixed in the same structure, better tabs.

[loicpoulain]

thanks, comment addressed.

[finikorg]

I see that usb_dc_ep_transfer() is used only in usb_dc_ep_write(). Are you going to export it to API?
Using it inside usb_dc_ep_write() changes a bit logic. Now with every packet send I get warning with EAGAIN error returned, my USB try_write() helper handles this and another attempt succeeds. You get this warning only when other debug disabled, timing does matter.

[loicpoulain]

Hi @finikorg, you're correct, plan is to export this API, but before, I would like to have the implementation enabled in both drivers (dw, stm). Then I think we could use this method in all existing class drivers progressively and maybe remove the current usb_read/write API.

I agree that this change a bit the behavior by returning EAGAIN when transfer is ongoing, but I think this is aligned with the original usb_write behavior. Documentation says:

"To transmit data to the host, the class driver should call usb_write(). Upon completion the registered endpoint callback will be called. Before sending another packet the class driver should wait for the completion of the previous transfer."

In current usb_write implementation we've workaround-ed/hacked a bit the behavior to actively wait for the FIFO availability which basically works but is not the cleanest way.

I'm not sure about the right way to address your comment, if this is OK for you, I propose to add a patch in this PR removing the warning message (causing delays) since EAGAIN is expected when using ep_write without waiting for the ep_callback. Let me know...

[loicpoulain]

@finikorg, In order to avoid this regression, I updated usb_dc_ep_write to keep the legacy behavior (active waiting). let me know if it's ok for you now.

[finikorg]

Now it looks better that we keep legacy logic, when you finish the API we might moved to the new one.

[finikorg]

newline not needed

[finikorg]

With small comments

[finikorg]

Now it looks better that we keep legacy logic, when you finish the API we might moved to the new one.

[finikorg]

Is this typo with

cb ? "no" : "yes"

[finikorg]

BTW, can we now use 0x%02x format string in SYSLOGs?

[loicpoulain]

If caller does not provide a callback, function is synchronous (sync) and wait for the transfer completion, if caller provide a callback, function is asynchronous and callback is called on transfer completion.
Did not see any issue with %02x formating, some other zephyr code use it btw.

[finikorg]

OK, right.

[jfischer-no]

Sorry, It may need more discussion. In the case of usb_dc_ep_transfer, the driver is blocked until it is done, it means that no other endpoint can be written or read, right? If so, then it is not acceptable. Then it is better to keep drivers as simple as possible and to realize a packet manager on the device stack level.

[loicpoulain]

@jfischer-phytec-iot, transfer is attached to a particular endpoint, others are not impacted and parallel transfer is possible.
It's certainly possible to move some code at upper layer (generic usb device), however transfer itself is hardware specific (need to configure hw registers with pkt-number, transfer size, etc...) so I assume we can not just split packet at higher level.

[jfischer-no]

@loicpoulain sorry, got it now too. Can you please open an issue and describe how the API should be changed and what the driver should do then. Maybe it would be easier to implement a packet manager in the device stack and keep the drivers as simple as possible. It is also easier to review and maintain a change like packet manager in device stack than every usb device driver. See also the discussion on the devel about NRF52 driver.

[jfischer-no]

@loicpoulain @finikorg I guess the API change for the device drivers is not really necessary because the usb_dc_ep_write and the usb_dc_ep_read already allow larger transfers. I suggest not changing the API of the driver yet. The drivers can or should be kept simple as possible and depending on the hardware the driver may treat the endpoint buffer differently, e.g. DMA and double buffering or larger fifo, the the usb_dc_ep_write / usb_dc_ep_read can be implemented accordingly. However, to make the packet transfer comfortable for the functions like CDC EEM, it would make sense to extend the stack API and implement a simple packet or buffer manager that uses the usb_dc_ep_write / usb_dc_ep_read and handles the transfer buffer. It also takes less maintenance to the code and the existing functions do not have to be changed immediately. What do you say to that?

[loicpoulain]

Like I said transfer configuration/completion is specific to hardware (reg, irq...), I agree we can probably move some generic code (semaphore, callback mgmt) to the generic device layer, but a change in the driver interface seems necessary. I need to dig on this point.

[jfischer-no]

but a change in the driver interface seems necessary.

No, why? The interface of a typical usb controller is not like a interface of a hal driver. The controller may have a larger fifo or second buffer, but it can be abstracted and handled inside usb_dc_ep_write / usb_dc_ep_read. And does not assume that a ethernet packet can always fit into the endpoint fifo (or buffer).

[loicpoulain]

To clarify my point, I'm going to take dw driver as example. dw controller has a register to configure transfer size and pkt count. Bulk transfer is a series of one or more data transactions. One transaction consists of sending content of the fifo (64 bytes with USB FS).

USB_WRITE

expected behavior

So the 'expected' driver job is to retrieve the buffer to send (e.g 1000 bytes), configure controller registers with transfer-size=1000, pkt-count=16 and then write the FIFO with the first 64 bytes. once the FIFO is empty because first transaction has been completed we receive a FIFO-EMPTY IRQ and we can then feed the FIFO with the next 64 bytes, and so on.... when all expected transactions have been performed, device driver receives a TRANSFER-COMPLETE IRQ from the controller.

Current behavior

With current usb_write implementation, we only retrieve the 64 first bytes of the buffer to write the fifo and then program transfer-size=64,pkt-count=1, the transfer is then completed after the first packet. This is then up to the upper-layer to call write 16 times to send the 1000 bytes. I agree this solution works, but this is a lot of extra work for class drivers and maybe not really optimized.

Solution ?

There are several possibilities to fix this limitation:

• having a buffer (much larger than fifo size) inside the driver but that means copy the usb_write buffer data to the driver buffer and then to the fifo. Don't like this because of this extra copy and memory usage.
• consider the buffer owned by the driver during all the transfer. Driver keeps a pointer to the usb_write buffer and consumes (write fifo) until transfer completion. This is actually what is done by the stm32 driver. This is absolutely reproducible with the dw driver.

Blocking or not Blocking ?

If I call usb_write with a 1000, 100 or 64 bytes buffer, does the write needs to block until completion?

• If yes, this a simple case but that means function can not be called in IRQ context, or have a different behavior depending the context.
• If not that means we need to wait for an asynchronous callback before performing a new write or releasing / updating the buffer passed to the usb_write function, then each driver class needs to ensure the a correct write synchronization => which is not the case, Actually this is why we introduced some active waiting hacks on the FIFO availability in dw and stm32 drivers. This explain why I wanted to introduce a transfer method with optional callback in order to cover both blocking/non-blocking cases.

USB_READ

A read transfer is performed in the same way as a write at hardware level. Driver needs to configure hardware with transfer-size and pkt-cnt.

Current behavior

Read is a weird case. Today the the current usb_read API considers USB as a data stream. Device drivers have to start read transfer once the device driver is initialized (1-pkt 64 bytes transfer) even before any usb_read occurs. Once transfer is completed, device driver calls the 'endpoint callback' which is typically a usb class driver function. On endpoint callback, class driver calls usb_read, device drivers then copies the 64-byte FIFO to the provided buffer and then can re-trigger a new read transfer, and so on...

Expected behavior (in my opinion)

I think the correct behavior is to not trigger any transfer until call usb_read, on usb_read device driver can then configure hardware with correct transfer size (buffer size) and then progressively fill it from FIFO up to the transfer completion. On completion read is achieved (in case of blocking method) or a callback is called.

This is for all these reasons I wanted to have a usb_transfer (URB like) function ;-)

So, for me we have 'at least' to clarify the read/write API so that device drivers implement them in a common way (blocking ? non-blocking ? return code ? callback ? buffer owned by the driver?).

[jfischer-no]

USB_WRITE
expected behavior

So the 'expected' driver job is to retrieve the buffer to send (e.g 1000 bytes), configure controller registers with transfer-size=1000, pkt-count=16 and then write the FIFO with the first 64 bytes. once the FIFO is empty because first transaction has been completed we receive a FIFO-EMPTY IRQ and we can then feed the FIFO with the next 64 bytes, and so on.... when all expected transactions have been performed, device driver receives a TRANSFER-COMPLETE IRQ from the controller.

Although it is predetermined by the API if the bytes_ret is NULL, does it make sense to make the drivers so complex? And the DW driver is not exactly what I would take as a reference for a API change, has some issues and this patch does not make it any better and does not fix it.

Current behavior

With current usb_write implementation, we only retrieve the 64 first bytes of the buffer to write the fifo and then program transfer-size=64,pkt-count=1, the transfer is then completed after the first packet. This is then up to the upper-layer to call write 16 times to send the 1000 bytes. I agree this solution works, but this is a lot of extra work for class drivers and maybe not really optimized.

Does the function usb_dw_tx always work that way? Is pkt_cnt always 1?
As proposed, it would make sense to extend the stack API and implement a simple packet or buffer manager that uses the usb_dc_ep_write / usb_dc_ep_read and handles the transfer buffer.

Solution ?
There are several possibilities to fix this limitation:
having a buffer (much larger than fifo size) inside the driver but that means copy the usb_write buffer data to the driver buffer and then to the fifo. Don't like this because of this extra copy and memory usage.
consider the buffer owned by the driver during all the transfer. Driver keeps a pointer to the usb_write buffer and consumes (write fifo) until transfer completion. This is actually what is done by the stm32 driver. This is absolutely reproducible with the dw driver.

If the size of the "fifo" is equal MPS of the endpoint then it is a endpoint buffer not fifo. If the controller can handle more than one MPS for a endpoint (it has a fifo or second buffer), then the usb_dc_ep_write is already designed for it. If the fifo or buffer of a endpoint is smaller then the packet size then the transfer could be managed at the stack level.

Blocking or not Blocking ?
If I call usb_write with a 1000, 100 or 64 bytes buffer, does the write needs to block until completion?

If yes, this a simple case but that means function can not be called in IRQ context, or have a different behavior depending the context.
If not that means we need to wait for an asynchronous callback before performing a new write or releasing / updating the buffer passed to the usb_write function, then each driver class needs to ensure the a correct write synchronization => which is not the case, Actually this is why we introduced some active waiting hacks on the FIFO availability in dw and stm32 drivers. This explain why I wanted to introduce a transfer method with optional callback in order to cover both blocking/non-blocking cases.

Yes, each class needs to ensure the a correct write synchronization.
Also here, a packet manager on the stack level can do it for the class driver, also the timeout treatment can be done there, I see no reason why every driver has to implement it for himself.

USB_READ

A read transfer is performed in the same way as a write at hardware level. Driver needs to configure hardware with transfer-size and pkt-cnt.
Current behavior

Read is a weird case. Today the the current usb_read API considers USB as a data stream. Device drivers have to start read transfer once the device driver is initialized (1-pkt 64 bytes transfer) even before any usb_read occurs. Once transfer is completed, device driver calls the 'endpoint callback' which is typically a usb class driver function. On endpoint callback, class driver calls usb_read, device drivers then copies the 64-byte FIFO to the provided buffer and then can re-trigger a new read transfer, and so on...

No read is fine. The preparations for the read are like this: configure endpoint register, configure buffer or fifo, enable endpoint, signal the controller to own the the buffer or fifo. And then if the generous usb host sends a packet that is accepted by the controller (crc, toggle bit, token...), then an IRQ will be triggered and the device stack can read the buffer or fifo.

Expected behavior (in my opinion)

I think the correct behavior is to not trigger any transfer until call usb_read, on usb_read device driver can then configure hardware with correct transfer size (buffer size) and then progressively fill it from FIFO up to the transfer completion. On completion read is achieved (in case of blocking method) or a callback is called.

It is all driven by host, the usb device does not trigger anything, the host does it. And how should the driver or controller always know when a transfer is over? Should the transfer logic be moved from the stack to every driver?

This is for all these reasons I wanted to have a usb_transfer (URB like) function ;-)

So, for me we have 'at least' to clarify the read/write API so that device drivers implement them in a common way (blocking ? non-blocking ? return code ? callback ? buffer owned by the driver?).

[loicpoulain]

Thanks @jfischer-phytec-iot, I had to say it sounds fair enough and rational. So I agree that dw is a specific example and I assume all controllers do not have transfer control capabilities. This transfer/pkt management could indeed take place at upper layer, relying on existing ep_write/ep_read functions. I will come back with a solution now we are aligned ;-).