Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esp32: Re-organize the interrupt handling process to correctly handle the SMP case #4388

Merged
merged 9 commits into from
Aug 25, 2021

Conversation

Ouss4
Copy link
Member

@Ouss4 Ouss4 commented Aug 20, 2021

Summary

This PR has 3 major changes:

  • Simplify the interrupt API exported for other drivers. Mainly, this hides the details of allocating and attaching an interrupt. A driver will just have to "setup" and "teardown" an interrupt, the details are handled internally (this also helps with the next parts).
  • Merge the different interrupt implementation into one file. This simplifies sharing variables and functions and removes the necessity of global, extern'd, variables.
  • When possible, use the Interrupt Matrix to disable/enable an interrupt. Unlike the in-core INTENABLE register, the Interrupt Matrix can be accessed by both CPUs. Hence, manipulating interrupts can be done without inter-CPU calls.

Impact

ESP32 chip.

Testing

All of the in-tree ESP32 defconfigs were tested.

@Ouss4
Copy link
Member Author

Ouss4 commented Aug 20, 2021

@masayuki2009 this is a draft just because I might create separate PRs for the different changes if required. Otherwise, the change is ready. It implements enabling/disabling interrupts using the Interrupt Matrix.

@Ouss4 Ouss4 force-pushed the esp32_interrupt branch 2 times, most recently from c997e18 to eaed2aa Compare August 20, 2021 18:16
@masayuki2009
Copy link
Contributor

@Ouss4

Hmm, I tried esp32-devkitc:wapi_smp but I was not able to connect to my Wi-Fi access point.

nsh> uname -a
NuttX  3.6.1 eaed2aaa94-dirty Aug 22 2021 01:18:26 xtensa esp32-devkitc
nsh> ps
  PID CPU PRI POLICY   TYPE    NPX STATE    EVENT     SIGMASK   STACK   USED  FILLED COMMAND
    0   0   0 FIFO     Kthread N-- Assigned           00000000 002016 000736  36.5%  CPU0 IDLE
    1   1   0 FIFO     Kthread N-- Running            00000000 002016 000608  30.1%  CPU1 IDLE
    2 --- 100 RR       Kthread --- Waiting  Semaphore 00000000 002080 000944  45.3%  lpwork 0x3ffc02b8
    3   0 100 RR       Task    --- Running            00000000 003120 001744  55.8%  init
    4 --- 223 RR       Kthread --- Waiting  Semaphore 00000000 002080 000544  26.1%  rt_timer
    5 --- 253 RR       Kthread --- Waiting  MQ empty  00000000 006704 001728  25.7%  wifi
    6 --- 100 RR       Task    --- Waiting  Semaphore 00000000 002064 000528  25.5%  Telnet daemon 0x3ffe3ef0
nsh> free
                   total       used       free    largest  nused  nfree
        Umem:     241984      58256     183728     110000    144      4
nsh> mount
  /mnt/esp/wifi type spiffs
  /proc type procfs
nsh> ifconfig
wlan0	Link encap:Ethernet HWaddr ac:67:b2:71:65:2c at UP
	inet addr:10.0.0.2 DRaddr:10.0.0.1 Mask:255.255.255.0

             IPv4   TCP   UDP  ICMP
Received     0000  0000  0000  0000
Dropped      0000  0000  0000  0000
  IPv4        VHL: 0000   Frg: 0000
  Checksum   0000  0000  0000  ----
  TCP         ACK: 0000   SYN: 0000
              RST: 0000  0000
  Type       0000  ----  ----  0000
Sent         0000  0000  0000  0000
  Rexmit     ----  0000  ----  ----
nsh> wapi psk wlan0 wifi-test-24g 1
nsh> wapi essid wlan0 raspi3-g 1

@Ouss4
Copy link
Member Author

Ouss4 commented Aug 21, 2021

Hmm, I tried esp32-devkitc:wapi_smp but I was not able to connect to my Wi-Fi access point.

@masayuki2009 thanks for testing, let me check.

@masayuki2009
Copy link
Contributor

@Ouss4
esp32-devkitc:wapi has the same problems too.

nsh> uname -a
NuttX  3.6.1 eaed2aaa94-dirty Aug 22 2021 01:30:30 xtensa esp32-devkitc
nsh> ps
  PID PRI POLICY   TYPE    NPX STATE    EVENT     SIGMASK   STACK   USED  FILLED COMMAND
    0   0 FIFO     Kthread N-- Ready              00000000 003040 000624  20.5%  Idle Task
    1 100 RR       Kthread --- Waiting  Semaphore 00000000 004128 000448  10.8%  lpwork 0x3ffb983c
    2 100 RR       Task    --- Running            00000000 004144 001744  42.0%  init
    3 223 RR       Kthread --- Waiting  Semaphore 00000000 002080 000448  21.5%  rt_timer
    4 253 RR       Kthread --- Waiting  MQ empty  00000000 006704 001696  25.2%  wifi
    5 100 RR       Task    --- Waiting  Semaphore 00000000 004112 000528  12.8%  Telnet daemon 0x3ffbe950
nsh> free
                   total       used       free    largest  nused  nfree
        Umem:     273248      62480     210768     135232    136      4
nsh> mount
  /mnt/esp/wifi type spiffs
  /proc type procfs
nsh> ifconfig
wlan0	Link encap:Ethernet HWaddr ac:67:b2:71:65:2c at UP
	inet addr:10.0.0.2 DRaddr:10.0.0.1 Mask:255.255.255.0

             IPv4   TCP   UDP  ICMP
Received     0000  0000  0000  0000
Dropped      0000  0000  0000  0000
  IPv4        VHL: 0000   Frg: 0000
  Checksum   0000  0000  0000  ----
  TCP         ACK: 0000   SYN: 0000
              RST: 0000  0000
  Type       0000  ----  ----  0000
Sent         0000  0000  0000  0000
  Rexmit     ----  0000  ----  ----
nsh> wapi psk wlan0 wifi-test-24g 1
nsh> wapi essid wlan0 raspi3-g 1

@Ouss4
Copy link
Member Author

Ouss4 commented Aug 21, 2021

@masayuki2009 enabling the Wi-fi interrupt at the core level was lost in the middle of my rebasing. I added it again. Please check.

Since now we control peripheral interrupt only based on the Interrupt Matrix, the in-core interrupts has to be enabled at start-up. This is done during the setup of an interrupt, but since Wi-fi is setup with the external library, it has to be handled separately.

@masayuki2009
Copy link
Contributor

@Ouss4
Now, the Wi-Fi works both in non-SMP and in SMP mode.

@Ouss4
Copy link
Member Author

Ouss4 commented Aug 24, 2021

@masayuki2009 @gustavonihei I'm keeping the commits as they are to ease reviewing. Once that's done, I'll squash some of them.

@Ouss4 Ouss4 marked this pull request as ready for review August 24, 2021 10:20
Copy link
Contributor

@gustavonihei gustavonihei left a comment

Choose a reason for hiding this comment

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

LGTM

@masayuki2009
Copy link
Contributor

@Ouss4
The code looks good but I think the following code should be atomic which means that we need to use enter_critical_section() or spin_lock_irqsave().

cpu = up_cpu_index();
esp32_setup_irq(cpu, ...);

For example, if the code is not atomic, the currently running CPU would be switched to another CPU after calling up_cpu_index(). So please be careful when calling up_cpu_index().

@Ouss4
Copy link
Member Author

Ouss4 commented Aug 25, 2021

@Ouss4
The code looks good but I think the following code should be atomic which means that we need to use enter_critical_section() or spin_lock_irqsave().

cpu = up_cpu_index();
esp32_setup_irq(cpu, ...);

For example, if the code is not atomic, the currently running CPU would be switched to another CPU after calling up_cpu_index(). So please be careful when calling up_cpu_index().

@masayuki2009 if I'm not mistaken all of those calls are within a critical section. I'll check again tomorrow from my computer.

@masayuki2009
Copy link
Contributor

@masayuki2009 if I'm not mistaken all of those calls are within a critical section. I'll check again tomorrow from my computer.

@Ouss4
No problem, I will wait for your investigation results. And if it is OK, I will merge this PR.

interrupt.

That function will have a parameter to decide whether to allocate a
level sensitive interrupt or an edge sensitive interrupt.

All the drivers are also updated with this API change.

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
Allocating and attaching interrupts were both exported outside, however
these two move hand in hand and we don't have to expose these details.
Also, the parameters passed are saved and will be used to retrieve
information about the interrupt and the attached peripheral.

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
address of a peripheral.

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
They do the same thing (manipulate interrupts) keeping them separated
was making things harder.

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
only inside this file.

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
xtensa/include/irq.h

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
Matrix.

This allows manipulating interrupts from both CPUs.  Internal interrupts
however, still need to be disabled/enabled by each CPU.

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
status of the interrupt (enabled/disabled).

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
@Ouss4
Copy link
Member Author

Ouss4 commented Aug 25, 2021

@masayuki2009 I have checked that all the calls are from within a critical section. There is only the emac driver initialization and the serial DMA initialization that don't explicitly call enter_critical_section but that part is initialized early in the bring-up process when we still didn't start the other CPUs.

I have also done some squashing.

@masayuki2009 masayuki2009 merged commit fc594c5 into apache:master Aug 25, 2021
@Ouss4 Ouss4 deleted the esp32_interrupt branch September 18, 2021 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants