You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/03/12 19:15:18 UTC
[GitHub] [incubator-nuttx] Ouss4 opened a new pull request #5729: arch/xtensa: Small improvements around xtensa_context
Ouss4 opened a new pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729
## Summary
- `xtensa_context_save` can use A1 directly no need to pass the saving area in SP. This will free A2 to use as a scratch register.
- A1 register was being restored twice.
- Use Zephyr's small version of window spill.
- Closes #574
## Impact
Xtensa chips.
## Testing
ESP32xx defconfigs all build and run successfully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825361463
##########
File path: arch/xtensa/src/common/xtensa_user_handler.S
##########
@@ -441,8 +427,8 @@ _xtensa_syscall_handler:
l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_1
l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
```suggestion
l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825341976
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -164,16 +161,28 @@ _xtensa_context_save:
#error Overlay support is not implemented
#endif
- s32i a0, a2, (4 * REG_TMP0) /* Save return address */
- s32i sp, a2, (4 * REG_TMP1) /* Save current stack pointer */
- wsr a2, EXCSAVE_1 /* Preserve register save area */
-
- l32i sp, a2, (4 * REG_A1) /* Restore the interruptee's SP */
- call0 _xtensa_window_spill /* Preserves only a4-a5, a8-a9, a12-a13 */
+ s32i a0, sp, (4 * REG_TMP0)
+ rsr a2, PS /* to be restored after SPILL_ALL_WINDOWS */
+ movi a0, PS_INTLEVEL_MASK
+ and a3, a2, a0 /* get the current INTLEVEL */
+ bgeui a3, XCHAL_EXCM_LEVEL, 1f /* calculate max(INTLEVEL, XCHAL_EXCM_LEVEL) */
Review comment:
Nice, didn't know about this one. Thanks for the clarification!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825469995
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
No. The issue is that other lines use tabs for alignment, but you added 2 spaces per per TAB on this change and GitHub shows that. If someone will open the file with editor that has 4 space per TAB view (for example), then formatting will be lost. I just propose to use TABs in this file (actually either TABs or spaces, but not a mix) so view will be agnostic to editor settings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825472351
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
Yes, I agree that the tab/space usage should be consistent, either tab or space is fine, but not mix. Since many assemble files under other arch use tab, if you decide switch to space, please change all of them, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825341279
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -164,16 +161,28 @@ _xtensa_context_save:
#error Overlay support is not implemented
#endif
- s32i a0, a2, (4 * REG_TMP0) /* Save return address */
- s32i sp, a2, (4 * REG_TMP1) /* Save current stack pointer */
- wsr a2, EXCSAVE_1 /* Preserve register save area */
-
- l32i sp, a2, (4 * REG_A1) /* Restore the interruptee's SP */
- call0 _xtensa_window_spill /* Preserves only a4-a5, a8-a9, a12-a13 */
+ s32i a0, sp, (4 * REG_TMP0)
+ rsr a2, PS /* to be restored after SPILL_ALL_WINDOWS */
+ movi a0, PS_INTLEVEL_MASK
+ and a3, a2, a0 /* get the current INTLEVEL */
+ bgeui a3, XCHAL_EXCM_LEVEL, 1f /* calculate max(INTLEVEL, XCHAL_EXCM_LEVEL) */
+ movi a3, XCHAL_EXCM_LEVEL
+1:
+ movi a0, PS_UM | PS_WOE /* clear EXCM, enable window overflow, set new INTLEVEL */
+ or a3, a3, a0
+ wsr a3, ps
+ rsync
+ rsr a0, EPC1 /* to be restored after SPILL_ALL_WINDOWS */
+
+ addi sp, sp, XCPTCONTEXT_SIZE /* go back to spill register region */
+ SPILL_ALL_WINDOWS /* place the live register windows there */
+ addi sp, sp, -XCPTCONTEXT_SIZE /* return the current stack pointer and proceed with context save*/
+
+ wsr a2, PS /* restore to the value at entry */
+ wsr a0, EPC1 /* likewise */
+ rsync
+ l32i a0, sp, (4 * REG_TMP0)
Review comment:
I replaced all spaces with tabs in the whole file.
Some places like multiline comments are not possible to replace, so kept them as they are.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825358028
##########
File path: arch/xtensa/src/common/xtensa_asm_utils.h
##########
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2017, Intel Corporation
+ *
+ * SPDX-License-Identifier: Apache-2.0
Review comment:
We can't use ASF headers here since the code was not donated to the ASF. It's however in a compatible license so we can use it freely.
I don't know if we can remove the SPDX format though. I guess we can. At least I can do the formatting needed to pass nxstyle checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825341161
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -164,16 +161,28 @@ _xtensa_context_save:
#error Overlay support is not implemented
#endif
- s32i a0, a2, (4 * REG_TMP0) /* Save return address */
- s32i sp, a2, (4 * REG_TMP1) /* Save current stack pointer */
- wsr a2, EXCSAVE_1 /* Preserve register save area */
-
- l32i sp, a2, (4 * REG_A1) /* Restore the interruptee's SP */
- call0 _xtensa_window_spill /* Preserves only a4-a5, a8-a9, a12-a13 */
+ s32i a0, sp, (4 * REG_TMP0)
+ rsr a2, PS /* to be restored after SPILL_ALL_WINDOWS */
+ movi a0, PS_INTLEVEL_MASK
+ and a3, a2, a0 /* get the current INTLEVEL */
+ bgeui a3, XCHAL_EXCM_LEVEL, 1f /* calculate max(INTLEVEL, XCHAL_EXCM_LEVEL) */
Review comment:
`f` means `forward` (and `b` means `backwards`). It's just a way in assembly to limit the number of labels. Without it we would have to add unique labels for all jumps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825341700
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -353,7 +352,7 @@ _xtensa_level1_handler:
l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_1
l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
Review comment:
Thanks. I replaces all spaces with tabs in this file too, indentation should look better now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825389413
##########
File path: arch/xtensa/src/common/xtensa_asm_utils.h
##########
@@ -0,0 +1,76 @@
+/****************************************************************************
+ * arch/xtensa/src/common/xtensa_asm_utils.h
+ *
+ * Copyright (c) 2017, Intel Corporation
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ ****************************************************************************/
+
+#ifndef __ARCH_XTENSA_SRC_COMMON_XTENSA_ASM_UTILS_H
+#define __ARCH_XTENSA_SRC_COMMON_XTENSA_ASM_UTILS_H
+
+/****************************************************************************
+ * Assembly Language Macros
+ ****************************************************************************/
+
+/****************************************************************************
+ *
+ * Name: SPILL_ALL_WINDOWS
+ *
+ * Spills all windowed registers (i.e. registers not visible as
+ * A0-A15) to their ABI-defined spill regions on the stack.
+ *
+ * Unlike the Xtensa HAL implementation, this code requires that the
+ * EXCM and WOE bit be enabled in PS, and relies on repeated hardware
+ * exception handling to do the register spills. The trick is to do a
+ * noop write to the high registers, which the hardware will trap
+ * (into an overflow exception) in the case where those registers are
+ * already used by an existing call frame. Then it rotates the window
+ * and repeats until all but the A0-A3 registers of the original frame
+ * are guaranteed to be spilled, eventually rotating back around into
+ * the original frame. Advantages:
+ *
+ * - Vastly smaller code size
+ *
+ * - More easily maintained if changes are needed to window over/underflow
+ * exception handling.
+ *
+ * - Requires no scratch registers to do its work, so can be used safely in
+ * any context.
+ *
+ * - If the WOE bit is not enabled (for example, in code written for
+ * the CALL0 ABI), this becomes a silent noop and operates compatbily.
+ *
+ * - Hilariously it's ACTUALLY FASTER than the HAL routine. And not
+ * just a little bit, it's MUCH faster. With a mostly full register
+ * file on an LX6 core (ESP-32) I'm measuring 145 cycles to spill
+ * registers with this vs. 279 (!) to do it with
+ * xthal_spill_windows().
+ ****************************************************************************/
+
+.macro SPILL_ALL_WINDOWS
Review comment:
why not put into xtensa_context.S? If only xtensa_context.S will call SPILL_ALL_WINDOWS?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825340332
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -164,16 +161,28 @@ _xtensa_context_save:
#error Overlay support is not implemented
#endif
- s32i a0, a2, (4 * REG_TMP0) /* Save return address */
- s32i sp, a2, (4 * REG_TMP1) /* Save current stack pointer */
- wsr a2, EXCSAVE_1 /* Preserve register save area */
-
- l32i sp, a2, (4 * REG_A1) /* Restore the interruptee's SP */
- call0 _xtensa_window_spill /* Preserves only a4-a5, a8-a9, a12-a13 */
+ s32i a0, sp, (4 * REG_TMP0)
+ rsr a2, PS /* to be restored after SPILL_ALL_WINDOWS */
+ movi a0, PS_INTLEVEL_MASK
+ and a3, a2, a0 /* get the current INTLEVEL */
+ bgeui a3, XCHAL_EXCM_LEVEL, 1f /* calculate max(INTLEVEL, XCHAL_EXCM_LEVEL) */
+ movi a3, XCHAL_EXCM_LEVEL
+1:
+ movi a0, PS_UM | PS_WOE /* clear EXCM, enable window overflow, set new INTLEVEL */
+ or a3, a3, a0
+ wsr a3, ps
+ rsync
+ rsr a0, EPC1 /* to be restored after SPILL_ALL_WINDOWS */
+
+ addi sp, sp, XCPTCONTEXT_SIZE /* go back to spill register region */
+ SPILL_ALL_WINDOWS /* place the live register windows there */
+ addi sp, sp, -XCPTCONTEXT_SIZE /* return the current stack pointer and proceed with context save*/
+
+ wsr a2, PS /* restore to the value at entry */
+ wsr a0, EPC1 /* likewise */
+ rsync
+ l32i a0, sp, (4 * REG_TMP0)
Review comment:
This new piece of code is indented with spaces, while the others are indented with tabs.
Please, uniformize it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825363170
##########
File path: arch/xtensa/src/common/xtensa_asm_utils.h
##########
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2017, Intel Corporation
+ *
+ * SPDX-License-Identifier: Apache-2.0
Review comment:
I looked into `arch/risc-v/src/mpfs/mpfs_opensbi_utils.S` that has both `SPDX identifier` and licence text so was thinking that we can do the same, but let's keep it as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825349105
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -80,32 +81,28 @@
*
* Description:
*
- * NOTE: MUST BE CALLED ONLY BY 'CALL0' INSTRUCTION!
+ * NOTE: MUST BE CALLED ONLY BY 'CALL0' INSTRUCTION!
Review comment:
Please revert to spaces in comments in this file as well
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -185,114 +194,19 @@ _xtensa_context_save:
*
* Description:
*
- * This functions implements the moral equivalent of setjmp(). It is
- * called from user code (with interrupts disabled) to save the current
- * state of the running thread. This function always returns zero.
- * However, it sets the saved value of the return address (A2) to 1.
- * If the thread is s via _xtensa_context_restore or
- * xtensa_context_restore, it will appear as a second return from
- * xtensa_context_save but with the returned value of 1 to distinguish
- * the two cases.
+ * This function implements the moral equivalent of setjmp(). It is
Review comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825348518
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -3,12 +3,12 @@
*
* Adapted from use in NuttX by:
*
- * Copyright (C) 2016 Gregory Nutt. All rights reserved.
- * Author: Gregory Nutt <gn...@nuttx.org>
+ * Copyright (C) 2016 Gregory Nutt. All rights reserved.
Review comment:
This should've been reverted for both file now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825342214
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -3,12 +3,12 @@
*
* Adapted from use in NuttX by:
*
- * Copyright (C) 2016 Gregory Nutt. All rights reserved.
- * Author: Gregory Nutt <gn...@nuttx.org>
+ * Copyright (C) 2016 Gregory Nutt. All rights reserved.
Review comment:
Please get back spaces in comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825356789
##########
File path: arch/xtensa/src/common/xtensa_asm_utils.h
##########
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2017, Intel Corporation
+ *
+ * SPDX-License-Identifier: Apache-2.0
Review comment:
So we are not going to use standard NuttX Apache 2.0 header?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#issuecomment-1065960386
Github actually makes it look even worst, I see a line that extends too long. I will drop the last commit all together and address individual spaces I introduced.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825351318
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -80,32 +81,28 @@
*
* Description:
*
- * NOTE: MUST BE CALLED ONLY BY 'CALL0' INSTRUCTION!
+ * NOTE: MUST BE CALLED ONLY BY 'CALL0' INSTRUCTION!
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825363284
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
This is just to aline with code `call0 _xtensa_context_restore /* (preserves a2) */` above as it was initially. But let's keep spaces and migrate to spaces globally for the whole project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825341700
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -353,7 +352,7 @@ _xtensa_level1_handler:
l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_1
l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
Review comment:
Thanks. I replaced all spaces with tabs in this file too, indentation should look better now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825361954
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
```suggestion
l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
```
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
Review comment:
```suggestion
l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
```
##########
File path: arch/xtensa/src/common/xtensa_user_handler.S
##########
@@ -299,8 +297,8 @@ _xtensa_user_handler:
l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_1
l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
```suggestion
l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
```
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
Review comment:
```suggestion
l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#issuecomment-1065960123
> I see that many many .S files use TABs and I'm fine with that. But spaces are used in comments.
There are also many assembly files that are not consistent in the usage of spaces and tabs for code indentation. It's a bit cumbersome when you want to do a mass find/replace. Anyway, I will keep the Xtensa files as they are for now. I reverted the tabs introduced in comments from the last commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825358463
##########
File path: arch/xtensa/src/common/xtensa_asm_utils.h
##########
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2017, Intel Corporation
+ *
+ * SPDX-License-Identifier: Apache-2.0
Review comment:
Yeah, let's fix NuttX style issue and maybe we can add Apache-2.0 license text. I think that should be fine
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825469000
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
Yes it is 2 spaces. This is how many spaces we've been using for tabs, whether for Makefiles or assembly files. Github doesn't necessary follow that and this is why it looks different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825340951
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -353,7 +352,7 @@ _xtensa_level1_handler:
l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_1
l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
Review comment:
```suggestion
l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#issuecomment-1065955839
Regarding spaces and tabs, it's simpler to just use spaces everywhere (also easy to replace existing tabs). We don't have any rule for what to use for indentation in assembly files. I can change everything in Xtensa to use spaces if we agree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825454231
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
It depends how many spaces are configured to display with a single TAB. You can try to play with that setting to see how formatting changes. Probably in your editor it is 2 spaces per TAB
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825416189
##########
File path: arch/xtensa/src/common/xtensa_asm_utils.h
##########
@@ -0,0 +1,76 @@
+/****************************************************************************
+ * arch/xtensa/src/common/xtensa_asm_utils.h
+ *
+ * Copyright (c) 2017, Intel Corporation
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ ****************************************************************************/
+
+#ifndef __ARCH_XTENSA_SRC_COMMON_XTENSA_ASM_UTILS_H
+#define __ARCH_XTENSA_SRC_COMMON_XTENSA_ASM_UTILS_H
+
+/****************************************************************************
+ * Assembly Language Macros
+ ****************************************************************************/
+
+/****************************************************************************
+ *
+ * Name: SPILL_ALL_WINDOWS
+ *
+ * Spills all windowed registers (i.e. registers not visible as
+ * A0-A15) to their ABI-defined spill regions on the stack.
+ *
+ * Unlike the Xtensa HAL implementation, this code requires that the
+ * EXCM and WOE bit be enabled in PS, and relies on repeated hardware
+ * exception handling to do the register spills. The trick is to do a
+ * noop write to the high registers, which the hardware will trap
+ * (into an overflow exception) in the case where those registers are
+ * already used by an existing call frame. Then it rotates the window
+ * and repeats until all but the A0-A3 registers of the original frame
+ * are guaranteed to be spilled, eventually rotating back around into
+ * the original frame. Advantages:
+ *
+ * - Vastly smaller code size
+ *
+ * - More easily maintained if changes are needed to window over/underflow
+ * exception handling.
+ *
+ * - Requires no scratch registers to do its work, so can be used safely in
+ * any context.
+ *
+ * - If the WOE bit is not enabled (for example, in code written for
+ * the CALL0 ABI), this becomes a silent noop and operates compatbily.
+ *
+ * - Hilariously it's ACTUALLY FASTER than the HAL routine. And not
+ * just a little bit, it's MUCH faster. With a mostly full register
+ * file on an LX6 core (ESP-32) I'm measuring 145 cycles to spill
+ * registers with this vs. 279 (!) to do it with
+ * xthal_spill_windows().
+ ****************************************************************************/
+
+.macro SPILL_ALL_WINDOWS
Review comment:
I considered that, but I am thinking to also use this macro when doing the backtrace (which doesn't seem to work in ESP32, this is something I'll have to come back to later).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825438352
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
Well, in my editor they are already aligned (this is the issue with tabs).
<img width="766" alt="image" src="https://user-images.githubusercontent.com/12075908/158058705-683dece4-04a1-4aad-a881-43c59e05a6aa.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825356946
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -353,7 +352,7 @@ _xtensa_level1_handler:
l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_1
l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
```suggestion
l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#issuecomment-1066289806
> I applied all suggestions and squashed the commits.
>
> > Yes, I agree that the tab/space usage should be consistent, either tab or space is fine, but not mix. Since many assemble files under other arch use tab, if you decide switch to space, please change all of them, thanks.
>
> Very few assembly files are consistent in their indentation. Xtensa ones are also very inconsistent, so fixing just a snippet feels pointless, besides we have no rule on what to use.
>
> Regarding changing tabs to spaces, that would be my preference too, but changing all of the assembly fils in tree is out of the scope of this PR.
Agree, it's enough to keep the consistent style in this PR. The tab/space conversion need a new PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#issuecomment-1066186162
I applied all suggestions and squashed the commits.
> Yes, I agree that the tab/space usage should be consistent, either tab or space is fine, but not mix. Since many assemble files under other arch use tab, if you decide switch to space, please change all of them, thanks.
Very few assembly files are consistent in their indentation. Xtensa ones are also very inconsistent, so fixing just a snippet feels pointless, besides we have no rule on what to use.
Regarding changing tabs to spaces, that would be my preference too, but changing all of the assembly fils in tree is out of the scope of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825469995
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
> Yes it is 2 spaces. This is how many spaces we've been using for tabs, whether for Makefiles or assembly files. Github doesn't necessary follow that and this is why it looks different.
No. The issue is that other lines use tabs for alignment, but you added 2 spaces per per TAB on this change and GitHub shows that. If someone will open the file with editor that has 4 space per TAB view (for example), then formatting will be lost. I just propose to use TABs in this file (actually either TABs or spaces, but not a mix) so view will be agnostic to editor settings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825469890
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
But, the assemble file under arm assume 8 space per tab, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#issuecomment-1065956570
> Regarding spaces and tabs, it's simpler to just use spaces everywhere (also easy to replace existing tabs). We don't have any rule for what to use for indentation in assembly files. I can change everything in Xtensa to use spaces if we agree.
I see that many many `.S` files use TABs and I'm fine with that. But spaces are used in comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825340332
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -164,16 +161,28 @@ _xtensa_context_save:
#error Overlay support is not implemented
#endif
- s32i a0, a2, (4 * REG_TMP0) /* Save return address */
- s32i sp, a2, (4 * REG_TMP1) /* Save current stack pointer */
- wsr a2, EXCSAVE_1 /* Preserve register save area */
-
- l32i sp, a2, (4 * REG_A1) /* Restore the interruptee's SP */
- call0 _xtensa_window_spill /* Preserves only a4-a5, a8-a9, a12-a13 */
+ s32i a0, sp, (4 * REG_TMP0)
+ rsr a2, PS /* to be restored after SPILL_ALL_WINDOWS */
+ movi a0, PS_INTLEVEL_MASK
+ and a3, a2, a0 /* get the current INTLEVEL */
+ bgeui a3, XCHAL_EXCM_LEVEL, 1f /* calculate max(INTLEVEL, XCHAL_EXCM_LEVEL) */
+ movi a3, XCHAL_EXCM_LEVEL
+1:
+ movi a0, PS_UM | PS_WOE /* clear EXCM, enable window overflow, set new INTLEVEL */
+ or a3, a3, a0
+ wsr a3, ps
+ rsync
+ rsr a0, EPC1 /* to be restored after SPILL_ALL_WINDOWS */
+
+ addi sp, sp, XCPTCONTEXT_SIZE /* go back to spill register region */
+ SPILL_ALL_WINDOWS /* place the live register windows there */
+ addi sp, sp, -XCPTCONTEXT_SIZE /* return the current stack pointer and proceed with context save*/
+
+ wsr a2, PS /* restore to the value at entry */
+ wsr a0, EPC1 /* likewise */
+ rsync
+ l32i a0, sp, (4 * REG_TMP0)
Review comment:
This new piece of code is indented with spaces, while the others are indented with tabs.
Please, uniformize them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825340591
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -164,16 +161,28 @@ _xtensa_context_save:
#error Overlay support is not implemented
#endif
- s32i a0, a2, (4 * REG_TMP0) /* Save return address */
- s32i sp, a2, (4 * REG_TMP1) /* Save current stack pointer */
- wsr a2, EXCSAVE_1 /* Preserve register save area */
-
- l32i sp, a2, (4 * REG_A1) /* Restore the interruptee's SP */
- call0 _xtensa_window_spill /* Preserves only a4-a5, a8-a9, a12-a13 */
+ s32i a0, sp, (4 * REG_TMP0)
+ rsr a2, PS /* to be restored after SPILL_ALL_WINDOWS */
+ movi a0, PS_INTLEVEL_MASK
+ and a3, a2, a0 /* get the current INTLEVEL */
+ bgeui a3, XCHAL_EXCM_LEVEL, 1f /* calculate max(INTLEVEL, XCHAL_EXCM_LEVEL) */
Review comment:
I see that the third argument for `bgeui` is the label that will be the branch target, but I would expect it to be just `1`.
What does the `f` in `1f` mean? Or it it a typo?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825357096
##########
File path: arch/xtensa/src/common/xtensa_context.S
##########
@@ -324,9 +252,9 @@ xtensa_context_save:
*
* NOTE: MUST BE CALLED ONLY BY 'CALL0' INSTRUCTION!
*
- * These functions restores Xtensa processor state and differ in which
- * registers are saved: _xtensa_context_restore() restores all registers
- * except PC, PS, A0, and A2
+ * This functions restores Xtensa processor state.
Review comment:
```suggestion
* This function restores Xtensa processor state.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825361090
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -348,13 +346,13 @@ _xtensa_level1_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, PS
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_1
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
Review comment:
```suggestion
l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825362043
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
Why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#issuecomment-1065981864
This is the inconsistency I was referring to earlier. Fixing these tabs in the middle of instructions and comments in assembly files in a snippet of code feels pointless, I see the rest of the file still messed up. The only reasonable solution is to go full spaces.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825362684
##########
File path: arch/xtensa/src/common/xtensa_asm_utils.h
##########
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2017, Intel Corporation
+ *
+ * SPDX-License-Identifier: Apache-2.0
Review comment:
I fixed the style, but kept the SPDX license format.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5729: arch/xtensa: Small improvements around xtensa_context
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5729:
URL: https://github.com/apache/incubator-nuttx/pull/5729#discussion_r825438352
##########
File path: arch/xtensa/src/common/xtensa_int_handlers.S
##########
@@ -768,13 +756,13 @@ _xtensa_level6_handler:
/* Restore only level-specific regs (the rest were already restored) */
- l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
+ l32i a0, a2, (4 * REG_PS) /* Retrieve interruptee's PS */
wsr a0, EPS_6
- l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
+ l32i a0, a2, (4 * REG_PC) /* Retrieve interruptee's PC */
wsr a0, EPC_6
- l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
- l32i sp, a2, (4 * REG_A1) /* Remove interrupt stack frame */
- l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
+ l32i a0, a2, (4 * REG_A0) /* Retrieve interruptee's A0 */
+ l32i sp, a2, (4 * REG_A1) /* Retrieve interrupt stack frame */
+ l32i a2, a2, (4 * REG_A2) /* Retrieve interruptee's A2 */
Review comment:
Well, in my editor they are already aligned (this is the issue with tabs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org