Replies: 3 comments 14 replies
-
|
your suggestion make sense, section attribute should go with global variable. Though I have no issue using this with other MCUs. Would you mind telling your gcc version ( |
Beta Was this translation helpful? Give feedback.
-
|
Hi @Hatach , I think this discussion can be closed now. Kind Regards, Pete |
Beta Was this translation helpful? Give feedback.
-
|
Caching is an known issue with current ehci driver since I mostly tested this driver with lpc1800 m3 mcu which does not have cache. Later I will try to make some tests with rt10xx m7 mcu to have a basic cache support. ps: discussion is fine without closing. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @hathach , To date whilst writing my Xilinx Zynq 7000 host port of tinyUSB I have chosen to allocate a block of 4K aligned un-cached SDRAM and allocated all USB related buffers in that area through the use of your macro struct
{
tusb_control_request_t request TU_ATTR_ALIGNED(4);
uint8_t* buffer;
tuh_xfer_cb_t complete_cb;
uintptr_t user_data;
uint8_t daddr;
volatile uint8_t stage;
volatile uint16_t actual_len;
}_ctrl_xfer;to the Currently I am overcoming the problem of this request buffer not being in un-cached memory by the modification of the bool hcd_setup_send(uint8_t rhport, uint8_t dev_addr, uint8_t const setup_packet[8])
{
(void) rhport;
ehci_qhd_t* qhd = &ehci_data.control[dev_addr].qhd;
ehci_qtd_t* td = &ehci_data.control[dev_addr].qtd;
Xil_DCacheFlush(); //Flush the cache!
qtd_init(td, setup_packet, 8);
td->pid = EHCI_PID_SETUP;
td->int_on_complete = 1;
td->next.terminate = 1;
// sw region
qhd->p_qtd_list_head = td;
qhd->p_qtd_list_tail = td;
// attach TD
qhd->qtd_overlay.next.address = (uint32_t) td;
return true;
}Personally I am not keen on having a non-cached memory scheme with one small buffer in a cached memory area and then having to flush the cache just for that one buffer. I would either like to be able to have a completely non-cached solution or a fully cached solution or even better still be able to do both 😄 Is there anyway we can revisit my suggestion of allocating the Also I would be very happy to work through the EHCI driver and add cache related code to create a flexible version to work with cached memory correctly. I would suggest adding a simple user definable macro so it is flexible for other platforms. In /* Platform dependant cache flushing function */
#ifndef CFG_TUSB_FLUSH_CACHE
#define CFG_TUSB_FLUSH_CACHE
#endifThen the user defines their requirement in #define CFG_TUSB_FLUSH_CACHE Xil_DCacheFlush() Then we implement it in the library code as follows( taking my example above): bool hcd_setup_send(uint8_t rhport, uint8_t dev_addr, uint8_t const setup_packet[8])
{
(void) rhport;
ehci_qhd_t* qhd = &ehci_data.control[dev_addr].qhd;
ehci_qtd_t* td = &ehci_data.control[dev_addr].qtd;
CFG_TUSB_FLUSH_CACHE; //Flush the cache!
qtd_init(td, setup_packet, 8);
td->pid = EHCI_PID_SETUP;
td->int_on_complete = 1;
td->next.terminate = 1;
// sw region
qhd->p_qtd_list_head = td;
qhd->p_qtd_list_tail = td;
// attach TD
qhd->qtd_overlay.next.address = (uint32_t) td;
return true;
}With these additions we can enhance the EHCI support to work with non-cached and cached implementations making it easy for the library user to choose their preferred method. I am happy to do all this work and test it on my local platform and submit it as a pull request if you are interested in my input? Also eventually I hope to be able to provide a Xilinx Zynq Host port to use with the tinyUSB library, although there are some challenges due to parts of the hardware implementation being FPGA based and we may have to add some functions which the user needs to fill in to do things like reset the USB PHY etc. as these are never going to be standard! I am still thinking about the best way to make this a sharable solution for all... I look forward to hearing your comments. Kind Regards, Pete PS I have been working in the embedded world for more than 20 years and have contributed to FreeRTOS and LVGL my linked in profile is here |
Beta Was this translation helpful? Give feedback.
-
|
ah, I think you are totally right, since setup request is passed to ehci via hcd_setup_send(), and therefore the whole _ctrl_xfer should be placed into CFG_TUSB_MEM_SECTION as well. So yes, if possible please submit a pr to re-add this. Sorry, my memory fades. Regarding the cache flush, it is not common in the MCU world, but getting more common recently. However, since it is only used within host/device controller layer, we should make it as part of hcd/dcd API instead of global config scope. What I am thinking is having an extra API to hcd.h as follow TU_ATTR_WEAK extern hcd_cache_flush(uint32_t addr, uint32_t data_size);which can be optionally implemented by the hcd_platform.c . What do you think ? |
Beta Was this translation helpful? Give feedback.
-
|
Hi @Hatach , Thank you for your reply here, yes this all works for me 🙂 I will sort out a PR...
No worries, mine too! Kind Regards, Pete |
Beta Was this translation helpful? Give feedback.
-
|
Hi @hathach , I hope you are well! I have been looking into another issue with my Zynq Host port, where my USB driver is hanging when I unplug my touch screen. The code is getting stuck in a for loop in the function static void list_remove_qhd_by_addr(ehci_link_t* list_head, uint8_t dev_addr)
{
for(ehci_link_t* prev = list_head;
!prev->terminate && (tu_align32(prev->address) != (uint32_t) list_head) && prev != NULL;
prev = list_next(prev) )
{
// TODO check type for ISO iTD and siTD
// TODO Suppress cast-align warning
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wcast-align"
ehci_qhd_t* qhd = (ehci_qhd_t*) list_next(prev);
#pragma GCC diagnostic pop
if ( qhd->dev_addr == dev_addr )
{
// TODO deactivate all TD, wait for QHD to inactive before removal
prev->address = qhd->next.address;
// EHCI 4.8.2 link the removed qhd to async head (which always reachable by Host Controller)
qhd->next.address = ((uint32_t) list_head) | (EHCI_QTYPE_QHD << 1);
if ( qhd->int_smask )
{
// period list queue element is guarantee to be free in the next frame (1 ms)
qhd->used = 0;
}else
{
// async list use async advance handshake
// mark as removing, will completely re-usable when async advance isr occurs
qhd->removing = 1;
}
}
}
}For reference here is a capture of the debug output from tinyUSB when the device is plugged in and removed: Insert Device here...Remove device here...If you need any further information please don't hesitate to ask. Kind Regards, Pete |
Beta Was this translation helpful? Give feedback.
-
|
thank you for the update, a quick glance at your change. It looks like ehci driver is trying to remove an queue head that is not in the list. I haven't tried to reproduce and/or analyses your log just yet, but the change look great, it is always good to guard again reference by NULL pointer. So please also include this in your PR as well ( along with the _ctrl_xfer ). |
Beta Was this translation helpful? Give feedback.
-
|
Hi @hathach , No problem I will make this change also... Kind Regards, Pete |
Beta Was this translation helpful? Give feedback.
-
|
Hi @hathach , If it is helpful it would be no trouble for me to fix this in my PR also please let me know and I will include it.... Kind Regards, Pete |
Beta Was this translation helpful? Give feedback.
-
|
@pete-pjb sure thing, if you already have the fix ready. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @hathach , Hope you are well! Just a quick update and a change of approach. Having spent a considerable amount of time attempting to implement cache control and USB mass storage driver on my Zynq platform. I have not been successful in implementing cache control and I am finding it difficult to integrate the platform cache with tinyUSB, so I am going to stick with allocating un-cached data buffers for now as I don't have time to look into what's going on. I will continue to contribute to the tinyUSB library if I discover any other bugs or see an opportunity for adding something useful, I will be sure to send a PR. Once I have a working solution for the USB storage I will share it with my Zynq Template Project for LVGL & FreeRTOS and link the tinyUSB library as a submodule to support it. I will report back here if I have any useful news. Kind Regards, Pete |
Beta Was this translation helpful? Give feedback.
-
|
not a problem at all, thank you for letting me know. We can always come back and address these caching later. |
Beta Was this translation helpful? Give feedback.
-
Hi @hathach ,
Firstly, thank you very much for creating and sharing this library it has been very helpful indeed!
I have a rudimentary host only port of this library working with the Zedboard (Xilinx Zynq7000).
I am currently looking at the changes I have made to the stock library in order to make it work with my Xilinx Vitis (Eclipse/GCC based IDE) environment and I was hoping I can make some changes so the tinyUSB library can be used with the Xilinx environment without modifications.
In the file
ehci.c (line 88)I have found an issue where my definedCFG_TUSB_MEM_SECTIONis being ignored. Having explored the situation further I believe it is not valid to associate the static declaration with a variable which is declared in a specific memory block.So I wanted to create a quick discussion as to how to move forward and clear up the issue in line with your preferred resolution.
My immediate suggestion would be to simply remove the
statickeyword from the declaration. This will probably be fine for most situations unless I am missing something:If you would prefer some other method of resolution please can you let me know, I would be interested to hear your thoughts on the issue... (I am happy to submit a pull request should you require me to make any changes...)
Kind Regards,
Pete
Beta Was this translation helpful? Give feedback.
All reactions