Skip to content

Conversation

@GUIDINGLI
Copy link
Contributor

@GUIDINGLI GUIDINGLI commented Sep 27, 2024

Summary

xtesa: fix lost save & restore states caused by merge code

this is caused by:
35c8c80

#13651

Impact

xtensa

Testing

Test pass in QEMU:

./tools/configure.sh -E esp32-devkitc:nsh

qemu-system-xtensa -nographic -machine esp32 -drive file=nuttx.merged.bin,if=mtd,format=raw -drive file=qemu_efuse.bin,if=none,format=raw,id=efuse -global driver=nvram.esp32.efuse,property=drive,value=efuse

this is caused by:
35c8c80

Signed-off-by: ligd <liguiding1@xiaomi.com>
@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small labels Sep 27, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 27, 2024

[Experimental Bot, please feedback here]

The PR summary is missing some key information:

  • What functional part of the code is being changed? Be specific. Mentioning "xtesa" is not enough. Is it a driver? A core kernel component?
  • How does the change exactly work? Briefly explain the technical details of your fix.

The Impact section is insufficient:

  • You need to answer all the questions with YES/NO and provide descriptions where applicable. Just stating "xtensa" doesn't offer enough information.

The Testing section needs significant improvement:

  • Build Host(s): Provide details about your development environment.
  • Target(s): Specify the exact xtensa architecture, board, and configuration used for testing.
  • Testing logs: Instead of pointing to documentation, include the actual relevant snippets of logs from before and after your change. This demonstrates the problem and the fix.

In conclusion: This PR does not meet the NuttX requirements in its current state. You need to provide more detailed and complete information in the Summary, Impact, and Testing sections.

@GUIDINGLI
Copy link
Contributor Author

I am trying to test in esp32 qemu:
if there is some th. wrong in Documentation/platforms/xtensa/esp32/boards/esp32-ethernet-kit/README.txt

  1. tools/configure.sh ./boards/xtensa/esp32/esp32-ethernet-kit/configs/nsh -j8
  2. open CONFIG_ESP32_QEMU_IMAGE
  3. make ESPTOOL_BINDIR=~/disk2/opensource/esp32/esp-bins
  4. Got nuttx.merged.bin
  5. esp32/qemu/build/xtensa-softmmu/qemu-system-xtensa -nographic -machine esp32 -drive file=nuttx.merged.bin,if=mtd,format=raw
Adding SPI flash device
ets Jul 29 2019 12:21:46

rst:0x1 (POWERON_RESET),boot:0x12 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:2
load:0x3ffb1f40,len:972
ho 0 tail 12 room 4
load:0x40080000,len:20632
entry 0x4008261c
*** Booting NuttX ***
dram: lma 0x00001020 vma 0x3ffb1f40 len 0x3cc    (972)
iram: lma 0x000013f4 vma 0x40080000 len 0x5098   (20632)
padd: lma 0x00006498 vma 0x00000000 len 0xab60   (43872)
imap: lma 0x00011000 vma 0x400e0000 len 0x10d78  (68984)
padd: lma 0x00021d80 vma 0x00000000 len 0xf298   (62104)
dmap: lma 0x00031020 vma 0x3f400020 len 0x2844   (10308)
total segments stored 6

hang

@yamt
Copy link
Contributor

yamt commented Sep 27, 2024

the "Testing" section in your PR description made me think you have tested it successfully.

otoh, your comment (#13662 (comment)) seems suggesting othewise.

can you correct one of them?

@GUIDINGLI
Copy link
Contributor Author

@yamt

Confirm test pass in QEMU:

./tools/configure.sh -E esp32-devkitc:nsh

qemu-system-xtensa -nographic -machine esp32 -drive file=nuttx.merged.bin,if=mtd,format=raw -drive file=qemu_efuse.bin,if=none,format=raw,id=efuse -global driver=nvram.esp32.efuse,property=drive,value=efuse

@xiaoxiang781216 xiaoxiang781216 merged commit 981fd0c into apache:master Sep 27, 2024
@masayuki2009
Copy link
Contributor

@GUIDINGLI
xtesa -> xtensa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants