target/xtensa: merge from upstream generic xtensa LX #3

erhankur · 2022-09-23T16:34:44Z

Pull Request Template

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality not to work as expected)
This change requires a documentation update

User Impact

Please describe the impact of this change. This can be a list of positive and/or negative impacts.

Performance Impact

Please describe any relevant performance impact of this change. This can be positive or negative impact. How did you characterize/test the performance impact?

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Test A
Test B

Hardware Configuration:

Development Kit:
Module or Chip Used:
Debug Adapter:

Software Configuration:

OpenOCD version:
Branch:
ESP_IDF version:
Operating System:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings

erhankur

@ianstcdns finally merge is completed with little changes. I added some comments.
Automation tests are also running now. So we will be more safe with the next patches.

src/target/xtensa/xtensa.c

erhankur · 2022-09-23T16:41:52Z

src/target/xtensa/xtensa.c

-		} else {
-			reg_list[XT_USR_REG_START + i].valid = false;
-		}
+#if 0	/* TODO: Breaks the tests. Investigate the reason. */


@ianstcdns I had to comment out this lines.

AR3 and AR4 are used as scratch registers (mostly the former but sometimes both). We're really just flagging A3 as dirty so it forces a flush when we resume. Could the value we're setting here in "a3" be wrong for some reason, maybe because of removing the earlier window state save/restore logic?

erhankur · 2022-09-23T16:43:28Z

src/target/xtensa/xtensa.c

+/* NOTE: Assumes A3 has already been saved */
+static int xtensa_window_state_save(struct target *target, uint32_t *woe)
+{
+	return ERROR_OK;/* TODO: xtensa_window_state_save/restore breaks the tests. Investigate the reason. */


@ianstcdns fyi.

please keep me posted. i did a fairly extensive register state comparison before / after various debug sessions but certainly could have missed something.

erhankur · 2022-09-23T16:43:53Z

src/target/xtensa/xtensa.c

-	if (reg_idx == XT_REG_IDX_FCR || reg_idx == XT_REG_IDX_FSR)
-		return xtensa->core_config->fp_coproc;
-	return false;
+	return;	/* TODO: xtensa_window_state_save/restore breaks the tests. Investigate the reason. */


@ianstcdns fyi

erhankur · 2022-09-23T16:44:37Z

src/target/xtensa/xtensa.c

 		return true;

 	return false;
 }

+/* TODO check PS change flow in the upstream. Looks like upstream branch has some issue. This is working version. */


@ianstcdns fyi

was this the issue we fixed upstream with the PS vs. EPSn change? if not, what's happening, and is there a way to reproduce it on mainline (either with my wrover-kit or a generic xtensa target)?

I think, you can simulate with the below values;

oldps = 60024

xtensa->stepping_isr_mode = XT_STEPPING_ISR_OFF (xtensa maskisr on)

xtensa->core_config->debug.irq_level = 6

newps should be set to 60025 before resume

e.g.
icountlvl = MIN((oldps & 0xF) + 1, xtensa->core_config->debug.irq_level); --> calculates 5

if ((oldps & 0xf) >= icountlvl) { --> (4 >= 5) generates false and ps not set to the new value

IIRC, there was a difference between our implementations related to $icountlevel. LX uses this register to determine which instructions "count" during instruction stepping...e.g. if an instruction is run when CINTLEVEL >= ICOUNTLEVEL, then it does not increment $icount. This allows OOCD to set $icountlevel to 1 and single-step user code, and if an overflow exception or L1 interrupt comes in, allow it to run until the core returns to level 0 and $icount increments up to 0. So, we're not masking interrupts (they still occur), but we also don't step into them.

Forcibly disabling interrupts during step can cause unintended side-effects in the system, such as missing timer ticks or watchdog kicks, and should really be avoided.

I think I may have changed the behavior of "xtensa maskisr" to behave more as a "step into ISR" vs. "step over ISR" flag that leave the ISR active in both cases. Could your test be assuming the ISR is getting disabled instead?

Ok I found the old discussion. erhankur/review-openocd#3 (comment)

This is the test we are talking about.
https://github.com/erhankur/openocd-esp32/blob/master/testing/esp/test_step.py#L363
https://github.com/erhankur/openocd-esp32/blob/master/testing/esp/test_apps/gen_ut_app/main/gen_ut_app.c#L307

src/target/xtensa/xtensa.c

erhankur

@ianstcdns Some more remaining work is done. This is about rtos stackings..

src/rtos/FreeRTOS.c

src/rtos/rtos_freertos_stackings.c

ianstcdns · 2022-09-27T13:47:02Z

Thanks for pulling mainline into your branch, @erhankur. Really appreciate all the time and hard work you've put in to make this solution benefit both Espressif and Cadence, and both of our customers. I know some of these changes have not been trivial, but please do let me know how I can help on our end.

FYI, the only remaining significant change I have on my list to eventually make is to add NX support, although that shouldn't affect anything in LX cores. A couple of eventual bug/performance fixes should be pretty small.

erhankur · 2022-09-27T13:55:49Z

@ianstcdns Thanks for your support. My first goal is to get running version with all the changes. static rtos stackings are OK for now but sure I'll implement the dynamic way also. After this work done, I need to create several patches for the minor changes. If you don't mind, can you keep on hold your NX patch until we fully sync with the upstream. Promise will not take too much time :)

ianstcdns · 2022-09-27T13:59:31Z

Yes, I can definitely wait on the NX work. Let’s get your changes merged first. No big rush either—I’ve got a lot of non-OOCD work coming up over the next few months, and was planning to wait until after 0.12 at a minimum. From: Erhan Kurubas ***@***.***> Sent: Tuesday, September 27, 2022 6:56 AM To: erhankur/openocd-esp32 ***@***.***> Cc: Ian Thompson ***@***.***>; Mention ***@***.***> Subject: Re: [erhankur/openocd-esp32] target/xtensa: merge from upstream generic xtensa LX (PR #3) EXTERNAL MAIL @ianstcdns<https://urldefense.com/v3/__https:/github.com/ianstcdns__;!!EHscmS1ygiU1lA!DeOmVBMgHDkrCcS6JjgKPf9_cQtqwKLEGnn1ORNufdBbY2hP08fRuSZ1l97olmsqrMRBB_TNVl30cevyQDc7Vg$> Thanks for your support. My first goal is to get running version with all the changes. static rtos stackings are OK for now but sure I'll implement the dynamic way also. After this work done, I need to create several patches for the minor changes. If you don't mind, can you keep on hold your NX patch until we fully sync with the upstream. Promise will not take too much time :) — Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/erhankur/openocd-esp32/pull/3*issuecomment-1259548575__;Iw!!EHscmS1ygiU1lA!DeOmVBMgHDkrCcS6JjgKPf9_cQtqwKLEGnn1ORNufdBbY2hP08fRuSZ1l97olmsqrMRBB_TNVl30cetoWZc7PA$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AYE3N6I3AJ3L4GJZH764VW3WAL4HBANCNFSM6AAAAAAQUEJ7RQ__;!!EHscmS1ygiU1lA!DeOmVBMgHDkrCcS6JjgKPf9_cQtqwKLEGnn1ORNufdBbY2hP08fRuSZ1l97olmsqrMRBB_TNVl30cevtAaEAdQ$>. You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>

erhankur requested a review from ianstcdns September 23, 2022 16:34

erhankur commented Sep 23, 2022

View reviewed changes

erhankur added 3 commits September 27, 2022 02:32

target/xtensa: merge from upstream part1

77f8be7

target/xtensa: merge from upstream part2

5bac6ea

target/xtensa: merge from upstream part3

3fd1d27

erhankur force-pushed the generic_Xtensa_LX branch from 28578c1 to 3fd1d27 Compare September 27, 2022 00:33

erhankur commented Sep 27, 2022

View reviewed changes

src/rtos/FreeRTOS.c Show resolved Hide resolved

src/rtos/rtos_freertos_stackings.c Show resolved Hide resolved

src/rtos/rtos_freertos_stackings.c Show resolved Hide resolved

target/xtensa: merge from upstream generic xtensa LX #3

Are you sure you want to change the base?

target/xtensa: merge from upstream generic xtensa LX #3

Uh oh!

Conversation

erhankur commented Sep 23, 2022

Pull Request Template

Description

Type of change

User Impact

Performance Impact

How Has This Been Tested?

Checklist:

Uh oh!

erhankur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erhankur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ianstcdns commented Sep 27, 2022

Uh oh!

erhankur commented Sep 27, 2022

Uh oh!

ianstcdns commented Sep 27, 2022 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants