esp32-core: Fix BOARD_CLOCK_FREQUENCY

[yamt]

Tested on qemu.

I left CONFIG_ESP32CORE_RUN_IRAM case and the "Don't ask me" comment
as it is because I don't understand it or can test it.

[jerpelea]

you should not hardcode the frequency because tehre are 2 available crystal frequencyes
https://bitbucket.org/alinjerpelea/nuttx/src/ce7a0b390c7c983fbf3811734c8d0a4274b9c3bf/boards/xtensa/esp32/esp32-core/Kconfig#lines-9

[yamt]

@jerpelea
it's about BOARD_XTAL_FREQUENCY, isn't it?
this 80MHz is hardcoded in the bootloader. see the code referenced by the code comment.

[jerpelea]

here is the explanation
https://github.com/apache/incubator-nuttx/blob/386bde43cdc0da24c37dbd7b9091eede0b97a35e/boards/xtensa/esp32/esp32-core/include/board.h#L45

By default it is set to 40MHZ and I do not see the reason to hardcode the frequency
https://bitbucket.org/alinjerpelea/nuttx/src/ce7a0b390c7c983fbf3811734c8d0a4274b9c3bf/boards/xtensa/esp32/esp32-core/Kconfig#lines-10

I would investigate why CONFIG_ESP32CORE_RUN_IRAM is setting it to 80MhZ
386bde4#diff-f166ac60a5b51d6d6c90be26a4e4145bR76

[yamt]

@jerpelea
CONFIG_ESP32CORE_XTAL_40MZ is about BOARD_XTAL_FREQUENCY.
this change is about BOARD_CLOCK_FREQUENCY.
the reason to hardcode is explained in the comment in this patch.

are you going to investigate CONFIG_ESP32CORE_RUN_IRAM?
i don't have OpenOCD environment.

[jerpelea]

-# define BOARD_CLOCK_FREQUENCY BOARD_XTAL_FREQUENCY
+# define BOARD_CLOCK_FREQUENCY 80000000

this is your change

[jerpelea]

I do not have OpenOCD

[yamt]

@jerpelea
sorry, i don't understand your comments.
do you mean that, for some reasons, BOARD_CLOCK_FREQUENCY should be same as BOARD_XTAL_FREQUENCY? why?

[jerpelea]

In your change you replace this line with a hardcoded value
I am trying to understand your thinking since nither you or me have the actual HW

[yamt]

my thinking is that

• the current code is broken at least if you run nuttx with esp-idf bootloader on qemu
• the bootloader configures it 80MHz (hardcoded) and nuttx relies on the configuration
• the "Don't ask me for an explanation" comment made me think the current code didn't have a good rationale anyway 89c33e9
• i left the CONFIG_ESP32CORE_RUN_IRAM=y case unchanged because i can't test or understand

[patacongo]

The frequency is only hardcoded if CONFIG_SUPPRESS_CLOCK_CONFIG is selected.

That is because the actual clock configuration logic is (or at least was) proprietary and Expressif never released this in source form, only as a binary library. If that library is downloaded and included in the build, then you can select any of the supported frequencies.

I don't follow ESP32 closely, but this change does not look correct and I do not think that it should be incorporated.

You should also look at TizenRT. Expressif took the NuttX ESP32 port and extended it in Tizen RT. You can bring those improvements in as you see fit, provided that we make sure that the Apache licensing is correct. See https://github.com/Samsung/TizenRT/tree/master/os/arch/xtensa.

Since that code is supported by Expressif, that should be considered an authoritative source.

[yamt]

do you mean non-CONFIG_SUPPRESS_CLOCK_CONFIG code can work? i don't think it even compile.
i agree it's better to fix it though.

i don't know how it was. but at this point the source code of the bootloader and hal are available. and they seem to hardcode 80MHz.

TizenRT just uses "#define BOARD_CLOCK_FREQUENCY (2 * BOARD_XTAL_FREQUENCY)" unconditionally.
which is 80MHz in case of CONFIG_ESP32CORE_XTAL_40MZ.

[jerpelea]

in my opion this is the proper fix instead of hardcoding it
#define BOARD_CLOCK_FREQUENCY (2 * BOARD_XTAL_FREQUENCY)

[yamt]

@jerpelea why do you think it should be 2 * BOARD_XTAL_FREQUENCY?
my understanding is that it should match the bootloader, which seems to hardcode 80MHz.

[jerpelea]

The frequency must match a clock source and if on the board is a 26MHZ quarts it it impossible to get 80MHZ. There may be 2 or more variants of the bootloader with different hardc oded frequencyes.
As a general rule you should avoid hardcoding clock s(I learned it the hard way)

[yamt]

@jerpelea
there seems to be multiple clock sources available for CPU clock.
see esp32_technical_reference_manual_en.pdf 3.2.3 "CPU Clock"
for 80MHz, "PLL" clock, which is 320MHz, is used.
https://github.com/espressif/esp-idf/blob/master/components/soc/src/esp32/rtc_clk.c#L581-L585

[patacongo]

@jerpelea I am expected that you will merge the change (or close it) after your discussion with @yamt concludes. Since you raise the issues, only you will know how to dispose of this PR.

[jerpelea]

@yamt please add the following change (same as TizenRT)so that I can merge the PR
#define BOARD_CLOCK_FREQUENCY (2 * BOARD_XTAL_FREQUENCY)

[jerpelea]

@patacongo yes I will

[yamt]

@jerpelea i won't until i understand why it should be (2 * BOARD_XTAL_FREQUENCY). maybe i will try to ask espressif.

[MasayukiIshikawa]

Hi, @yamt

Tested on qemu.

Could you tell us how to run nuttx for esp32 with qemu?
I'm happy if you update README.txt under boards/xtensa/esp32/esp32-core.

[igrr]

The frequency must match a clock source and if on the board is a 26MHZ quarts it it impossible to get 80MHZ.

On the ESP32, APB/CPU frequencies higher than the XTAL frequency are obtained using one of the two PLLs. So either of 26 or 40 MHz XTAL can be used as clock source for the PLL. PLL clock (320 or 480 MHz) is then internally divided to produce the CPU clock (80/160/240MHz) and the APB clock (80MHz).

[igrr]

By the way, this line:

#ifdef CONFIG_ESP32CORE_RUN_IRAM
#  define BOARD_CLOCK_FREQUENCY (2 * BOARD_XTAL_FREQUENCY)
#else

looks incorrect to me. As I understand, CONFIG_ESP32CORE_RUN_IRAM option means that the program is loaded directly into IRAM over JTAG, and the 2nd stage bootloader is not involved. In this case, CPU and APB frequencies will be the same as after reset, and will exactly match the XTAL frequency, so 2 * should be removed.

[jerpelea]

This is why I usggested that the change should be
define BOARD_CLOCK_FREQUENCY (2 * BOARD_XTAL_FREQUENCY)
instead of a fixed frequency

[maht]

@MasayukiIshikawa

Hi, @yamt

Tested on qemu.

Could you tell us how to run nuttx for esp32 with qemu?
I'm happy if you update README.txt under boards/xtensa/esp32/esp32-core.

By request, I created this pull request after polishing some patches I did some days ago to ease running the ESP32 QEMU: #437

Hope it would be useful. Feedback and/or improvements are welcome.

[igrr]

@jerpelea, sorry, i still don't understand why the BOARD_CLOCK_FREQUENCY should be exactly twice BOARD_XTAL_FREQUENCY. In the ESP32, the CPU frequency can be either of the following:

• XTAL frequency, divided by integer N
• 80MHz
• 160MHz
• 240MHz

Note that 80/160/240 values are independent of the XTAL frequency, which can be 40 or 26 MHz. So in the specific case when XTAL frequency is 40 MHz, indeed the CPU frequency can be twice the XTAL frequency. But in theory you may have a 26MHz XTAL along with 80MHz CPU frequency.