Skip to content
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

Non-bus-powered MSP430 support. #2134

Merged
merged 11 commits into from
Apr 15, 2024

Conversation

clhenry
Copy link
Contributor

@clhenry clhenry commented Jul 1, 2023

Describe the PR
Adding supported for hot plugging while using external power.

Additional context

@cr1901
Copy link
Collaborator

cr1901 commented Jul 1, 2023

It looks fine to me (I think at the time, I only focused on bus-powered applications b/c I found it difficult to reliably put the MSP-EXP430F5529LP into self-powered mode.

I do have a few questions:

  1. Is USBCTL &= ~FRSTE; and USBCTL |= FRSTE; intended to prevent a race due to power transients causing a second reset while in the process of handling the first reset? What happens if the reset vector is entered while processing the current reset?

  2. I think I can't even find a definition for osal_task_delay when osal_none is being used. Presumably it's a noop and then is optimized out in release mode. I think the whole build should fail due to missing definition when compiled in debug mode.

    It looks like other platforms use an include guard or a board_millis function provided by the application (which is not how I'd do it).

    Personally, I'd use the msp430 __delay_cycles if I absolutely have to have a hardcoded delay, but that's tied to the current CPU frequency. I'm a bit uncomfortable with hard-depending on a timer in the msp430 port, or relying on board_millis to be defined. I'm assuming you're testing without an OS, and so the code works fine without working delays. I would put an include guard for now that doesn't call osal_task_delay when CFG_TUSB_OS == OPT_OS_NONE.

@clhenry
Copy link
Contributor Author

clhenry commented Jul 2, 2023

  1. As this was 6 months ago I had to wrack my brain to try and remember the justification. I believe this was a partial implementation of TI's software fix for the USB4 errata affecting the MSP430F5529. According to the linked document FRSTE should be cleared on receipt of the reset interrupt and set on receipt of the suspend, setup and endpoint 0 interrupts.
    I'll be pushing a change later that correctly implements their recommend fix. In the mean time, if you have any suggestions to address your original concern, I'm all ears.

  2. I'm using FreeRTOS for my project. The initial delay in handle_bus_power_event was the shortest I could do in order to get the functionality working. The delay in the loop can be as short as 500 ns per TI's recommendation. As you pointed out, the intrinsic delay function is frequency dependent. I don't mind modify the PR to support a non-OS environment but I'm at a loss as to what would work for everyone if not board_millis. Again, I'm open to recommendations.

@cr1901
Copy link
Collaborator

cr1901 commented Jul 2, 2023

Ack re: the errata; when I did this port originally, I don't recall looking at the errata. That's fine then (incl. your upcoming changes).

I'm using FreeRTOS for my project.

Ahhh I didn't know FreeRTOS ran on MSP430, tbh :D!

I want to wait and hear what @hathach has to say re: "using board_millis in portable/". #1987 is also relevant.

@clhenry
Copy link
Contributor Author

clhenry commented Jul 8, 2023

@hathach I see that the non-os builds are failing on osal_task_delay. Can you chime in with your thoughts on the current discussion?

@cr1901
Copy link
Collaborator

cr1901 commented Jul 11, 2023

@clhenry Just to be clear, @hathach may be very busy, and while I am a collaborator, I mostly focus on backend stuff. I want to wait for @hathach to be less busy so he can give guidance on "sometimes no OS needs a delay" problem.

You're already expected to instantiate an interrupt handler yourself and place a call into tud_int_handler in your application. I'm not against adding a function like e.g. tud_delay that's called by osal_task_delay for no-OS, that the user is expected to fill in for their application if necessary. But adding this functionality requires some care.

@hathach
Copy link
Owner

hathach commented Jul 13, 2023

sorry, I was a bit busy atm, will check this out whenever I could. Thank you for your patient.

@hathach hathach force-pushed the non-bus-powered-re-enumeration branch from 5d403dd to 5d3b089 Compare April 11, 2024 13:58
@hathach
Copy link
Owner

hathach commented Apr 11, 2024

@clhenry Just to be clear, @hathach may be very busy, and while I am a collaborator, I mostly focus on backend stuff. I want to wait for @hathach to be less busy so he can give guidance on "sometimes no OS needs a delay" problem.

You're already expected to instantiate an interrupt handler yourself and place a call into tud_int_handler in your application. I'm not against adding a function like e.g. tud_delay that's called by osal_task_delay for no-OS, that the user is expected to fill in for their application if necessary. But adding this functionality requires some care.

tinyusb tries not to use additional hardware timer, and does not requires an time keeping function from application. For any short delay without rtos, it should use nop since there is no guarantee of actual time keeping function.

actual osal_task_delay() can be implemented as weak default implementation by dcd. Since I leave this pending for too long, I will try to make an update/test to get it merge myself :)

TU_ATTR_ALWAYS_INLINE static inline void tu_delay(uint32_t ms) {
// msp430 can run up to 25Mhz -> 40ns per cycle. 1 ms = 25000 cycles
// each loop need 4 cycle: 1 sub, 1 cmp, 1 jump, 1 nop
volatile uint32_t cycles = (25000 * ms) >> 2;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delay using nop where msp430 can run up to 25Mhz

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you very much, sorry for late response. I have update the PR to use blocking delay (cycle burning, taking consideration for msp430 can run at max 25Mhz). Since there is no generic delay API for tinyusb at the moment. We can change this later should we decide to have an generic time function later on.

PS: I also add some changes to get cmake build support (which I am using) for msp430 as well.

@hathach hathach merged commit fb21b6a into hathach:master Apr 15, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants