-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OHCI usbh, tweaks and improvements #1491
Conversation
72856e2
to
fba2efc
Compare
superb !! thank you very much for the PR, indeed, I have skipped lots of cpu detail and I think cut lots of corner to just get it running on small MCU. It has been awhile since I work with OHCI, give me a bit of time to revise the spec/code and pull out the old nxp devkit to test with. |
No rush at all I will follow up with proper periodic schedule handling (Ohci spec Figure 5-5 tree). I may put this in a #ifdef guard so it can be disabled by the user as it uses a bit of RAM, however this will allows transfers to respect bIntervals and spreads transfers across frames to maximise total available bandwidth. And I have isochronous support locally, however we need a host isochronous sample to demonstrate :) |
fcc97bf
to
cf96461
Compare
Yeah, thanks, I actually build the periodical tree as OHCI specs suggested in early implementation but make it simpler to save footprint. Having it configurable, I think having support 4/8ms (configurable) is better way. Sorry, I am still busy with paid works, will check/test this out whenever I could. |
@Ryzee119 I've given this PR a go and it's working on my EA4088 board with both OHCI root hub ports which is excellent. I needed to make some changes for my environment (OPT_OS_NONE, gcc 10.3.1) to fix some compilation issues: diff --git a/src/portable/ohci/ohci.c b/src/portable/ohci/ohci.c
index 76f97844c..cd747093a 100644
--- a/src/portable/ohci/ohci.c
+++ b/src/portable/ohci/ohci.c
@@ -35,7 +35,11 @@
//--------------------------------------------------------------------+
// INCLUDE
//--------------------------------------------------------------------+
+#if CFG_TUSB_OS == OPT_OS_NONE
+#include "bsp/board.h"
+#else
#include "osal/osal.h"
+#endif
#include "host/hcd.h"
#include "ohci.h"
@@ -166,17 +170,27 @@ static void ed_list_remove_by_addr(ohci_ed_t * p_head, uint8_t dev_addr);
//tuh_get_phys_addr and tuh_get_virt_addr in your application.
TU_ATTR_WEAK void *tuh_get_phys_addr(void *virtual_address);
TU_ATTR_WEAK void *tuh_get_virt_addr(void *physical_address);
-TU_ATTR_ALWAYS_INLINE static void *_phys_addr(void *virtual_address)
+TU_ATTR_ALWAYS_INLINE static inline void *_phys_addr(void *virtual_address)
{
if (tuh_get_phys_addr) return tuh_get_phys_addr(virtual_address);
return virtual_address;
}
-TU_ATTR_ALWAYS_INLINE static void *_virt_addr(void *physical_address)
+TU_ATTR_ALWAYS_INLINE static inline void *_virt_addr(void *physical_address)
{
if (tuh_get_virt_addr) return tuh_get_virt_addr(physical_address);
return physical_address;
}
+static void delay_ms(uint32_t ms)
+{
+#if CFG_TUSB_OS == OPT_OS_NONE
+ int now = board_millis();
+ while (board_millis() - now <= ms) asm("nop");
+#else
+ osal_task_delay(ms);
+#endif
+}
+
// Initialization according to 5.1.1.4
bool hcd_init(uint8_t rhport)
{
@@ -206,7 +220,7 @@ bool hcd_init(uint8_t rhport)
{
//Wait 20 ms. (Ref Usb spec 7.1.7.7)
OHCI_REG->control_bit.hc_functional_state = OHCI_CONTROL_FUNCSTATE_RESUME;
- osal_task_delay(20);
+ delay_ms(20);
}
// reset controller
@@ -233,7 +247,7 @@ bool hcd_init(uint8_t rhport)
OHCI_REG->control_bit.hc_functional_state = OHCI_CONTROL_FUNCSTATE_OPERATIONAL; // make HC's state to operational state TODO use this to suspend (save power)
OHCI_REG->rh_status_bit.local_power_status_change = 1; // set global power for ports
- osal_task_delay(OHCI_REG->rh_descriptorA_bit.power_on_to_good_time * 2); // Wait POTG after power up
+ delay_ms(OHCI_REG->rh_descriptorA_bit.power_on_to_good_time * 2); // Wait POTG after power up
return true;
}
@@ -612,7 +626,10 @@ static void done_queue_isr(uint8_t hostid)
{
// TODO check if td_head is iso td
//------------- Non ISO transfer -------------//
+ #pragma GCC diagnostic push
+ #pragma GCC diagnostic ignored "-Wcast-align"
ohci_gtd_t * const qtd = (ohci_gtd_t *) td_head;
+ #pragma GCC diagnostic pop
xfer_result_t const event = (qtd->condition_code == OHCI_CCODE_NO_ERROR) ? XFER_RESULT_SUCCESS :
(qtd->condition_code == OHCI_CCODE_STALL) ? XFER_RESULT_STALLED : XFER_RESULT_FAILED; |
also add place holder for tusb_app_dcache_flush() and tusb_app_dcache_invalidate()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brilliant !! Thank you very much for the PR and sorry for the huge delay. It is my bads, I should have reviewed this earlier. This add great improvement for ohci driver. I did some changes mostly rename to make it more consistent (in my pov):
- OHCI_RHPORTS to TUP_OHCI_RHPORTS (so that we know where it is defined)
- tuh_get_virt_addr/tuh_get_phys_addr() to tusb_app_virt_to_phys/tusb_app_phys_to_virt() since the term is more popular
- as @wooyay pointed out, osal_task_delay() cannot be used with os_none, since that delay is implemented using ohci frame counter register which is not enable just yet and also requires an plugged device in order to have SOF packet. It is temporarily commented out. tinyusb does not include any additional timer to actual delay. I will add an API to get CPU frequence so that we could do _nop() delay later on for these rare cases.
Thank you again for your effort and also your patient.
src/portable/ohci/ohci.c
Outdated
ed->used = 0; | ||
ed->skip = 0; | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch !!!
@wooyay thanks for pointed out the issue with osal_task_delay(), though board_millis() cannot be used, since it is application function. We will do it better with additional API later on. |
Describe the PR
A number of tweaks and improves to the OHCI usb host backend
Additional context
ed_list_remove_by_addr
that caused some EDs not to be removed on disconnect. This would cause an assert on re-connect as the old ED would continue on. Also seted->skip
to 1, prior to removing it from the queue. OHCI Spec seems to do this (Ref pseudo code in chapter 5.2.7.1.2).Probably will help to review on a per commit basis