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.

Sign up for GitHub

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

Jump to bottom

os/arch/arm/src/amebasmart: Fix UART data loss when wakeup from PG #6497

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

os/arch/arm/src/amebasmart: Fix UART data loss when wakeup from PG #6497

wants to merge 1 commit into from

Conversation

lhenry-realtek
Copy link
Contributor

@lhenry-realtek lhenry-realtek commented Nov 11, 2024 •
edited
Loading

Required: This commit requires #6482, please apply it as a patch if it is not already merged

  • Issue is encountered where bytes are lost on UART wakeup source during resume from sleep
  • Analysis: UART FIFO is started but not drained during resume. When the peripheral is initialized again, it causes the FIFO to also be cleared and cause data loss
  • Similarly if more than 64 bytes is passed in together, since there is no mechanism to drain the FIFO into RAM it will cause LSR overrun error
  • This fix drains the FIFO first at KM4, then again at CA32 until it fully wake, then normal Tizen ISR handler resume operation
  • The drained buffer is also passed to application layer

@lhenry-realtek
Copy link
Contributor Author

Please hold on merge, MAP and ASM file was not updated

edwakuwaku
edwakuwaku reviewed Nov 11, 2024
View reviewed changes
os/arch/arm/src/amebasmart/amebasmart_serial.c Outdated
UART_TypeDef* uartx = UART_DEV_TABLE[uart_index_get(g_uart1priv.tx)].UARTx;

/* prevent PM transition while fetching remainder FIFO */
pm_timedsuspend(pm_domain_register("UART_PG_WAKE"), 5000);
Copy link
Contributor

Choose a reason for hiding this comment

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

during uart bringup, a domain has been registered, how about refer to that pm id

Copy link
Contributor

Choose a reason for hiding this comment

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

why 5000, any reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that the board can be observed to actually be awake for a short time and to ensure that anything in the FIFO is fully cleared

Also see tash_main.c it has been defined to 5000ms as well

@edwakuwaku
Copy link
Contributor

@lhenry-realtek first commit of this PR shouldn't exist, please rebase it

@lhenry-realtek
Copy link
Contributor Author

bin file will be added after resolving conflicts on our side, do not merge yet

ewoodev
ewoodev reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@ewoodev ewoodev left a comment

Choose a reason for hiding this comment

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

Could you please verify using below tools??
apps/examples/uart_loopback/uart_loopback.c
tools/uart_loopback_test/uart_loopback_test.py
apps/examples/power/power_main.c

TASH >> uart_loopback start
TASH >> power start

@ewoodev
Copy link
Contributor

ewoodev commented Nov 14, 2024

@lhenry-realtek Could you please let me know how many bps are supported?

Based on a 16-byte FIFO: ???
Based on a 64-byte FIFO: ???

@zhongnuo-tang
Copy link
Contributor

@lhenry-realtek Could you please let me know how many bps are supported?

Based on a 16-byte FIFO: ??? Based on a 64-byte FIFO: ???

Hi @ewoodev ,
This PR is for UART1 64-byte FIFO, it support up to 115200bps.
For LOGUART 16-byte FIFO, we are not able to prevent data loss for both 115200/1.5M bps.

sunghan-chang
sunghan-chang reviewed Nov 19, 2024
View reviewed changes
os/arch/arm/src/amebasmart/amebasmart_serial.c Outdated
Comment on lines 1243 to 1244
memset(flag, 0, sizeof(flag));
memset(uart_data, 0, sizeof(uart_data));
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Could you check performance difference between 0 initialization at variable define and memset like this?
  2. What is the flag usage in this function?
  3. You add little bit heavy operation in suspend and resume. How does they affect sleep and wakeup time? Could you check them for before and after?
    1. when it enters sleep, before it takes xx ms, after yyms
    2. when it wakes up, before and after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. changed to = { 0 } initialization
  2. flag is used to check when the IPC operation has finished. In this case the KM4 side will transfer control back to CA32 after it has deinitialized UART1 on its side
  3. measurement in progress

os/arch/arm/src/amebasmart/amebasmart_serial.c Outdated
ipc_req_msg.rsvd = (u32)flag;

memset(flag, 0, sizeof(flag));
memset(uart_data, 0, sizeof(uart_data));
Copy link
Contributor

Choose a reason for hiding this comment

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

uart_data type is u32, but memset is working in byte (u8).
Could you compare for loop for each member set with 0 to memset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@sunghan-chang
Copy link
Contributor

@lhenry-realtek If this affects sleep and wakeup times, it has huge effect on several features. Could you check it ASAP?

@lhenry-realtek
Copy link
Contributor Author

lhenry-realtek commented Nov 20, 2024 •
edited
Loading

I have found some bug in the logic, currently refactoring

Initial measurement based on latest commit:

Enter Sleep (ms) Exit Sleep (ms)
SMP=0 w/o PR 2.054935 4.875004
SMP=1 w/o PR 2.0377216 5.6893256
SMP=0 w/ PR PENDING PENDING
SMP=1 w/ PR PENDING PENDING

SMP add roughly 0.8ms when waking up, mainly due to waiting for core1 to stabilize

Readings are taken from average of 10 samples per test

@sunghan-chang
Copy link
Contributor

@lhenry-realtek Thank you for sharing status. Let's talk this again with final result.

@lhenry-realtek lhenry-realtek force-pushed the fix_pg_uart_data_loss branch 2 times, most recently from db928a6 to d17656f Compare November 21, 2024 06:19
@lhenry-realtek
Copy link
Contributor Author

Latest measurement (each reading is average of 10 samples)

@sunghan-chang

Base Commit: 646445f
Configuration:

  • Board: AI_LITE_PF3_V1.0 88x40xT1.2mm
  • Config: loadable_ext_ddr
  • Baudrate: 1.5M LOGUART, 115200 UART1
  • USB=0, GPIO driver=1
  • Examples: LED, PM, UART Loopback
  • LED GPIO (PB20) pad used to measure wakeup/sleep at certain points of kernel (in KM0 and in Tizen)

Note: One shot 260 bytes test is done by sending 260 char size data at once into UART1 via Tera Term
Note 2: UART1 buffer is constantly being read by Rx Thread, pm_resume is manually started via TASH command
hello_main.txt
Note 3: Bin file will be uploaded in a force push, this is for update

Flow:

hello start
pm_resume
Begin Test

Before PR

  • SMP = ON
SMP Exit Sleep (ms) Enter Sleep (ms) Delta Exit<->Next Enter (ms)
Single Key (x10) 5.6541796 1.9748452 1.4811564
Press and Hold Key 5.6341944 2.2721228 1.299164
One Shot 260 bytes N/A (Data Lost) N/A (Data Lost) N/A (Data Lost)
  • SMP = AMP
AMP Exit Sleep (ms) Enter Sleep (ms) Delta Exit<->Next Enter (ms)
Single Key (x10) 5.6703732 1.9735384 1.475472
Press and Hold Key 5.6339996 2.1854916 1.2922396
One Shot 260 bytes N/A (Data Lost) N/A (Data Lost) N/A (Data Lost)
  • SMP = OFF
OFF Exit Sleep (ms) Enter Sleep (ms) Delta Exit<->Next Enter (ms)
Single Key (x10) 4.8589508 1.8909452 1.3775044
Press and Hold Key 4.8395508 2.0381852 1.254374
One Shot 260 bytes N/A (Data Lost) N/A (Data Lost) N/A (Data Lost)

After PR

  • SMP = ON
SMP Exit Sleep (ms) Enter Sleep (ms) Delta Exit<->Next Enter (ms)
Single Key (x10) 5.7609876 1.8512616 1.5626514
Press and Hold Key 5.7422752 2.0995372 1.3290596
One Shot 260 bytes 5.7880912 2.2587168 18.2019352
  • SMP = AMP
AMP Exit Sleep (ms) Enter Sleep (ms) Delta Exit<->Next Enter (ms)
Single Key (x10) 5.7873244 1.8747712 1.5201764
Press and Hold Key 5.7510772 2.103554 1.3058848
One Shot 260 bytes 5.8428524 2.4325212 18.2866932
  • SMP = OFF
OFF Exit Sleep (ms) Enter Sleep (ms) Delta Exit<->Next Enter (ms)
Single Key (x10) 4.9601192 1.7771004 1.4247744
Press and Hold Key 4.9740144 1.9423296 1.280094
One Shot 260 bytes 5.0025952 2.4726248 18.2814612

@lhenry-realtek
Copy link
Contributor Author

Hi @ewoodev I have run the tests

CA32PG-KM4PG-Wakeup : HW TIMER
CA32PG-KM4PG-Wakeup : HW TIMER
SEND UART(999): 1234567890abcdefghijklmnopqrstuvwxyz
RECEIVE(999): 1234567890abcdefghijklmnopqrstuvwxyz
RESULT(999): PASSED
CA32PG-KM4PG-Wakeup : HW TIMER
CA32PG-KM4PG-Wakeup : HW TIMER
SEND UART(1000): 1234567890abcdefghijklmnopqrstuvwxyz
RECEIVE(1000): 1234567890abcdefghijklmnopqrstuvwxyz
RESULT(1000): PASSED
CA32PG-KM4PG-Wakeup : HW TIMER
CA32PG-KM4PG-Wakeup : HW TIMER
SEND UART(1001): 1234567890abcdefghijklmnopqrstuvwxyz
RECEIVE(1001): 1234567890abcdefghijklmnopqrstuvwxyz
RESULT(1001): PASSED
CA32PG-KM4PG-Wakeup : HW TIMER
call suspend (11)

@lhenry-realtek lhenry-realtek changed the title os/arch/arm/src/amebasmart: Fix UART data loss when wakeup from PG [DO NOT MERGE] os/arch/arm/src/amebasmart: Fix UART data loss when wakeup from PG Nov 21, 2024
@lhenry-realtek lhenry-realtek changed the title [DO NOT MERGE] os/arch/arm/src/amebasmart: Fix UART data loss when wakeup from PG os/arch/arm/src/amebasmart: Fix UART data loss when wakeup from PG Nov 22, 2024
@lhenry-realtek
Copy link
Contributor Author

bin/asm/map added

sunghan-chang
sunghan-chang reviewed Nov 22, 2024
View reviewed changes
build/tools/amebasmart/gnu_utility/km0_km4_app_RELEASE_NOTE.txt Outdated
@@ -1,5 +1,11 @@
/* == "version" + "Realtek git version" + "compile date" + "compile time" == */

== version c7e8f3b164 2024/11/22-07:56:05 ==
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you check PR #6525 ? That also has a change on km0_km4_app. That is missing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

A rebase is needed, Hnery is working on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been updated @sunghan-chang

Copy link
Contributor

Choose a reason for hiding this comment

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

@lhenry-realtek @jwei5 changed them already. Need to update them again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sunghan-chang this bin also includes the changes made by @jwei5, it is safe to resolve with this set of files

@lhenry-realtek lhenry-realtek force-pushed the fix_pg_uart_data_loss branch 3 times, most recently from a08aee4 to d2fc335 Compare November 25, 2024 09:19
@lhenry-realtek
Copy link
Contributor Author

rebase 8b183fc

@ewoodev
Copy link
Contributor

ewoodev commented Nov 25, 2024

Hi @ewoodev I have run the tests

CA32PG-KM4PG-Wakeup : HW TIMER
CA32PG-KM4PG-Wakeup : HW TIMER
SEND UART(999): 1234567890abcdefghijklmnopqrstuvwxyz
RECEIVE(999): 1234567890abcdefghijklmnopqrstuvwxyz
RESULT(999): PASSED
CA32PG-KM4PG-Wakeup : HW TIMER
CA32PG-KM4PG-Wakeup : HW TIMER
SEND UART(1000): 1234567890abcdefghijklmnopqrstuvwxyz
RECEIVE(1000): 1234567890abcdefghijklmnopqrstuvwxyz
RESULT(1000): PASSED
CA32PG-KM4PG-Wakeup : HW TIMER
CA32PG-KM4PG-Wakeup : HW TIMER
SEND UART(1001): 1234567890abcdefghijklmnopqrstuvwxyz
RECEIVE(1001): 1234567890abcdefghijklmnopqrstuvwxyz
RESULT(1001): PASSED
CA32PG-KM4PG-Wakeup : HW TIMER
call suspend (11)

the string is short... Please increase string size of test over than 128byte and test again

@lhenry-realtek
Copy link
Contributor Author

@ewoodev

test string: Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte

test string length: 260

delay: 0.2 (200ms), anything lower than this is too fast for 115200 and will overflow or the device does not wakeup in time

SEND UART(1924): Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte
RECEIVE(1920): Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte
RESULT(1920): PASSED
call resume(20)
CA32PG-KM4PG-Wakeup : UART
SEND UART(1925): Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte
RECEIVE(1921): Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte
RESULT(1921): PASSED
CA32PG-KM4PG-Wakeup : UART
SEND UART(1926): Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte
RECEIVE(1922): Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte
RESULT(1922): PASSED

@sunghan-chang
Copy link
Contributor

sunghan-chang commented Nov 28, 2024 •
edited
Loading

@lhenry-realtek As you know, the files were updated. Could you resolve the conflict?
@gSahitya-samsung @ritesh55555 @neel-samsung Realtek verified the change. Could you confirm this once @lhenry-realtek resolves the conflicts?

@lhenry-realtek
Copy link
Contributor Author

It will be rebased again.

@lhenry-realtek
Copy link
Contributor Author 

call resume(13)
CA32PG-KM4PG-Wakeup : UART
RECEIVE(1277): Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte
RESULT(1277): PASSED
CA32PG-KM4PG-Wakeup : HW TIMER
SEND UART(1295): Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte
CA32PG-KM4PG-Wakeup : UART
RECEIVE(1278): Lorem ipsum dolor sit amet non velit labore ut commodo. Veniam occaecat, dolor ullamco quis consectetur occaecat ipsum excepteur labore nisi laboris tempor duis esse. Tempor consequat est culpa, qui ut excepteur irure. Proident minim velit sunt aliquip excepte
RESULT(1278): PASSED

stress tested 260 char 0.2 delay

@lhenry-realtek
os/arch/arm/src/amebasmart: Fix UART data loss when wakeup from PG
57f2e1a 
Issue is encountered where bytes are lost on UART wakeup source during resume from sleep

Test Requirements:
Please ensure PR Samsung#6482 is applied before testing

Analysis:
- UART FIFO is started but not drained during resume. When the peripheral is initialized again, it causes the FIFO to also be cleared and cause data loss
- Similarly if more than 64 bytes is passed in together, since there is no mechanism to drain the FIFO into RAM it will cause LSR overrun error

This fix drains the FIFO first at KM4, then again at CA32 until it fully wake, then normal Tizen ISR handler resume operation
- The drained buffer is also passed to application layer
- Watchdog is used to monitor UART1 until the FIFO is completely drained
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwakuwaku edwakuwaku edwakuwaku left review comments

@ewoodev ewoodev ewoodev left review comments

@zhongnuo-tang zhongnuo-tang zhongnuo-tang left review comments

@ziyik ziyik ziyik left review comments

@sunghan-chang sunghan-chang sunghan-chang left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lhenry-realtek @edwakuwaku @ewoodev @zhongnuo-tang @sunghan-chang @ziyik