Allow lengthening xosc startup delay with a compile option #457

2021-05-28T09:01:01-05:00

dhalbert commented

2021-05-28 09:01:01 -05:00

(Migrated from github.com)

This PR allows you to specify a multiplier, at compile time to lengthen the default XOSC startup time. The multiplier, PICO_XOSC_STARTUP_DELAY_MULTIPLIER, is 1 by default.

Motivation: On a few samples of some Adafruit RP2040 boards, the XOSC takes much longer to stabilize than would be expected. This appears to be due to some mismatch between the chosen XOSC crystal and capacitors. Sometimes the problem is intermittent, and may depend on handling the board while restarting, humidity, etc. We are going to fix future boards, but this PR is a software fix that accommodates all boards.

The Adafruit board include files set PICO_XOSC_STARTUP_DELAY_MULTIPLIER to be 64. This was chosen empirically. 16 can be too low sometimes. 32 seems to work, and so 64 includes a safety margin.

This issue has been discussed at length in #401. The default XOSC startup time is used in the boot ROM when using USB, but the XOSC startup problem does not seem to be an issue: the boards all can connect to USB successfully. Also relevant: https://github.com/raspberrypi/pico-feedback/issues/158/.

I've tested this on an Adafruit QT Py RP2040 that exhibits the problem, with the pico-examples blink example.

One minor thing: xosc.c did not end in a newline, and my editor fixed that automatically.

This PR allows you to specify a multiplier, at compile time to lengthen the default XOSC startup time. The multiplier, `PICO_XOSC_STARTUP_DELAY_MULTIPLIER`, is `1` by default. Motivation: On a few samples of some Adafruit RP2040 boards, the XOSC takes much longer to stabilize than would be expected. This appears to be due to some mismatch between the chosen XOSC crystal and capacitors. Sometimes the problem is intermittent, and may depend on handling the board while restarting, humidity, etc. We are going to fix future boards, but this PR is a software fix that accommodates all boards. The Adafruit board include files set `PICO_XOSC_STARTUP_DELAY_MULTIPLIER` to be 64. This was chosen empirically. 16 can be too low sometimes. 32 seems to work, and so 64 includes a safety margin. This issue has been discussed at length in #401. The default XOSC startup time is used in the boot ROM when using USB, but the XOSC startup problem does not seem to be an issue: the boards all can connect to USB successfully. Also relevant: https://github.com/raspberrypi/pico-feedback/issues/158/. I've tested this on an Adafruit QT Py RP2040 that exhibits the problem, with the `pico-examples` `blink` example. One minor thing: `xosc.c` did not end in a newline, and my editor fixed that automatically.

👍 1

kilograham (Migrated from github.com) requested changes

2021-05-28 10:52:11 -05:00

src/rp2_common/hardware_xosc/xosc.c

					
				@ -11,16 +11,25 @@

				#include "hardware/platform_defs.h"

				#include "hardware/regs/xosc.h"

Can you change this to a // PICO_CONFIG: so that it shows up in the docs (see other places)

Can you change this to a `// PICO_CONFIG: ` so that it shows up in the docs (see other places)

dhalbert (Migrated from github.com) reviewed

2021-05-28 11:14:48 -05:00

src/rp2_common/hardware_xosc/xosc.c

					
				@ -11,16 +11,25 @@

				#include "hardware/platform_defs.h"

				#include "hardware/regs/xosc.h"

Sure. Should I also do some kind of checking on the value to make sure the resultant multiplication doesn't overflow the register field of 14 bits? The struct definition for the register does not take that into account.

Sure. Should I also do some kind of checking on the value to make sure the resultant multiplication doesn't overflow the register field of 14 bits? The `struct` definition for the register does not take that into account.

dhalbert (Migrated from github.com) reviewed

2021-05-28 11:16:45 -05:00

src/rp2_common/hardware_xosc/xosc.c

					
				@ -11,16 +11,25 @@

				#include "hardware/platform_defs.h"

				#include "hardware/regs/xosc.h"

github.com/raspberrypi/pico-sdk@afc10f3599/src/rp2040/hardware_structs/include/hardware/structs/xosc.h (L15-L22)

![image](https://user-images.githubusercontent.com/2847802/120013197-77335680-bfae-11eb-96ea-b6b15ac58416.png) https://github.com/raspberrypi/pico-sdk/blob/afc10f3599c27147a6f34781b7102d86f58aa5f6/src/rp2040/hardware_structs/include/hardware/structs/xosc.h#L15-L22

dhalbert commented

2021-05-28 11:54:51 -05:00

(Migrated from github.com)

I pushed a PICO_CONFIG: fix, though I'm a little confused as to where I should see it. I built the doxygen docs and PICO_XOSC_STARTUP_DELAY_MULTIPLIER shows up in the doxygen/xml/ but not the doxygen/html. However, there are many other PICO_CONFIG'd definitions that similarly are only in the xml. They don't show up in the search, for instance.

I pushed a `PICO_CONFIG:` fix, though I'm a little confused as to where I should see it. I built the doxygen docs and `PICO_XOSC_STARTUP_DELAY_MULTIPLIER` shows up in the `doxygen/xml/` but not the `doxygen/html`. However, there are many other `PICO_CONFIG`'d definitions that similarly are only in the xml. They don't show up in the search, for instance.

lurch commented

2021-05-28 12:02:19 -05:00

(Migrated from github.com)

The PICO_CONFIG entries are extracted by tools/extract_configs.py to pico_configs.tsv which is then used by e.g. https://github.com/raspberrypi/pico-project-generator
They're also included into Appendix B of https://datasheets.raspberrypi.org/pico/raspberry-pi-pico-c-sdk.pdf by internal build-tools.

The `PICO_CONFIG` entries are extracted by `tools/extract_configs.py` to `pico_configs.tsv` which is then used by e.g. https://github.com/raspberrypi/pico-project-generator They're also included into Appendix B of https://datasheets.raspberrypi.org/pico/raspberry-pi-pico-c-sdk.pdf by internal build-tools.

kilograham (Migrated from github.com) reviewed

2021-05-31 12:54:57 -05:00

src/rp2_common/hardware_xosc/xosc.c

					
				@ -11,16 +11,25 @@

				#include "hardware/platform_defs.h"

				#include "hardware/regs/xosc.h"

yes makes sense - please follow invalid_params_if() style... (which will require you to add some macros to xosc.h - just copy/paste from somewhere else)

dhalbert commented

2021-05-31 20:55:41 -05:00

(Migrated from github.com)

I ended up not using invalid_params_if(), which seems more meant for run-time checking. Instead I did some cpp-compile-time checking, which is done similarly in other places.

Most recent changes:

Check at compile-time that XOSC_MHZ is in range (was done with an assert()).
Check at compile-time that the delay calculation fits in 14 bits.
Use XOSC_MHZ instead of the literal 12 in the delay calculation.

I ended up not using `invalid_params_if()`, which seems more meant for run-time checking. Instead I did some cpp-compile-time checking, which is done similarly in other places. Most recent changes: - Check at compile-time that `XOSC_MHZ` is in range (was done with an `assert()`). - Check at compile-time that the delay calculation fits in 14 bits. - Use `XOSC_MHZ` instead of the literal `12` in the delay calculation.

kilograham (Migrated from github.com) approved these changes

2021-06-01 09:15:10 -05:00

kilograham (Migrated from github.com) requested changes

2021-06-01 11:15:26 -05:00

src/rp2_common/hardware_xosc/xosc.c

					
				@ -11,16 +11,25 @@

				#include "hardware/platform_defs.h"

				#include "hardware/regs/xosc.h"

stylistically (going forward, there may be a few stragglers) we are trying to put these defaulted values in the header in case any other code (source files) cares about the actual value. can you move this to xsoc.h

kilograham (Migrated from github.com) approved these changes

2021-06-01 14:23:04 -05:00

dhalbert (Migrated from github.com) reviewed

2021-06-01 14:25:14 -05:00

src/rp2_common/hardware_xosc/xosc.c

					
				@ -11,16 +11,25 @@

				#include "hardware/platform_defs.h"

				#include "hardware/regs/xosc.h"

Sure. Moved PICO_CONFIG and default setting to src/rp2_common/hardware_xosc/include/hardware/xosc.h, and changed #include in xosc.c to accommodate this.

Sure. Moved PICO_CONFIG and default setting to `src/rp2_common/hardware_xosc/include/hardware/xosc.h`, and changed `#include` in xosc.c to accommodate this.