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.

Sign up for GitHub

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

Jump to bottom

Raspberry Pi Pico smorgasboard of bugfixes and improvements #80707

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+65 −98
Open

Raspberry Pi Pico smorgasboard of bugfixes and improvements #80707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e161e17
dts: arm: Move rpi_pico under raspberrypi
ajf58 Aug 27, 2024
22e20f9
dts: arm: rp2040: Improve naming of included files
ajf58 Aug 27, 2024
c4cfe63
drivers: clock_control: rpi_pico: Correct bitfields during init
ajf58 Aug 29, 2024
cbf1a58
dts: bindings: clock: rpi_pico: Add default value matching the Pico SDK
ajf58 Oct 6, 2024
9964470
drivers: clock_control: rpi_pico: Make pinctrl-0 optional
ajf58 Sep 27, 2024
c359065
boards: rp2040: Lint whitespace in board.cmake files
ajf58 Oct 9, 2024
7f7c0c6
drivers: dma: rpi_pico: Correct handling of NULL filter_param
ajf58 Oct 18, 2024
6fce055
tests: drivers: dma: Increase test coverage for RP2040
ajf58 Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion MAINTAINERS.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3515,7 +3515,7 @@ Raspberry Pi Pico Platforms:
- boards/raspberrypi/
- boards/adafruit/kb2040/
- boards/sparkfun/pro_micro_rp2040/
- dts/arm/rpi_pico/
- dts/arm/raspberrypi/rpi_pico/
- dts/bindings/*/raspberrypi,pico*
- drivers/*/*rpi_pico
- drivers/*/*rpi_pico*/
Expand Down
3 changes: 0 additions & 3 deletions boards/adafruit/kb2040/adafruit_kb2040-pinctrl.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@
};
};

clocks_default: clocks_default {
};

ws2812_pio0_default: ws2812_pio0_default {
ws2812 {
pinmux = <PIO0_P17>;
Expand Down
7 changes: 1 addition & 6 deletions boards/adafruit/kb2040/adafruit_kb2040.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

/dts-v1/;

#include <rpi_pico/rp2040.dtsi>
#include <raspberrypi/rpi_pico/rp2040.dtsi>
#include "adafruit_kb2040-pinctrl.dtsi"
#include "sparkfun_pro_micro_connector.dtsi"
#include <freq.h>
Expand Down Expand Up @@ -56,11 +56,6 @@
};
};

&clocks {
pinctrl-0 = <&clocks_default>;
pinctrl-names = "default";
};

&uart0 {
current-speed = <115200>;
status = "okay";
Expand Down
3 changes: 0 additions & 3 deletions boards/adafruit/qt_py_rp2040/adafruit_qt_py_rp2040-pinctrl.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,6 @@
};
};

clocks_default: clocks_default {
};

ws2812_pio1_default: ws2812_pio1_default {
ws2812 {
pinmux = <PIO1_P12>;
Expand Down
7 changes: 1 addition & 6 deletions boards/adafruit/qt_py_rp2040/adafruit_qt_py_rp2040.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

/dts-v1/;

#include <rpi_pico/rp2040.dtsi>
#include <raspberrypi/rpi_pico/rp2040.dtsi>
#include "adafruit_qt_py_rp2040-pinctrl.dtsi"
#include "seeed_xiao_connector.dtsi"
#include <freq.h>
Expand Down Expand Up @@ -56,11 +56,6 @@
};
};

&clocks {
pinctrl-0 = <&clocks_default>;
pinctrl-names = "default";
};

&uart1 {
current-speed = <115200>;
status = "okay";
Expand Down
2 changes: 1 addition & 1 deletion boards/raspberrypi/rpi_pico/board.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# The value must be the 'stem' part of the name of one of the files
# in the openocd interface configuration file.
# The setting is store to CMakeCache.txt.
if ("${RPI_PICO_DEBUG_ADAPTER}" STREQUAL "")
if("${RPI_PICO_DEBUG_ADAPTER}" STREQUAL "")
set(RPI_PICO_DEBUG_ADAPTER "cmsis-dap")
endif()

Expand Down
7 changes: 1 addition & 6 deletions boards/raspberrypi/rpi_pico/rpi_pico-common.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

#include <freq.h>

#include <rpi_pico/rp2040.dtsi>
#include <raspberrypi/rpi_pico/rp2040.dtsi>
#include "rpi_pico-pinctrl.dtsi"
#include <zephyr/dt-bindings/pwm/pwm.h>

Expand Down Expand Up @@ -89,11 +89,6 @@
};
};

&clocks {
pinctrl-0 = <&clocks_default>;
pinctrl-names = "default";
};

&uart0 {
current-speed = <115200>;
status = "okay";
Expand Down
3 changes: 0 additions & 3 deletions boards/raspberrypi/rpi_pico/rpi_pico-pinctrl.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,4 @@
input-enable;
};
};

clocks_default: clocks_default {
};
};
3 changes: 0 additions & 3 deletions boards/seeed/xiao_rp2040/xiao_rp2040-pinctrl.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@
};
};

clocks_default: clocks_default {
};

ws2812_pio0_default: ws2812_pio0_default {
ws2812 {
pinmux = <PIO0_P12>;
Expand Down
7 changes: 1 addition & 6 deletions boards/seeed/xiao_rp2040/xiao_rp2040.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

/dts-v1/;

#include <rpi_pico/rp2040.dtsi>
#include <raspberrypi/rpi_pico/rp2040.dtsi>
#include "xiao_rp2040-pinctrl.dtsi"
#include "seeed_xiao_connector.dtsi"
#include <freq.h>
Expand Down Expand Up @@ -86,11 +86,6 @@
};
};

&clocks {
pinctrl-0 = <&clocks_default>;
pinctrl-names = "default";
};

&timer {
status = "okay";
};
Expand Down
2 changes: 1 addition & 1 deletion boards/sparkfun/pro_micro_rp2040/board.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
# The value must be the 'stem' part of the name of one of the files
# in the openocd interface configuration file.
# The setting is store to CMakeCache.txt.
if ("${RPI_PICO_DEBUG_ADAPTER}" STREQUAL "")
if("${RPI_PICO_DEBUG_ADAPTER}" STREQUAL "")
set(RPI_PICO_DEBUG_ADAPTER "cmsis-dap")
endif()

Expand Down
3 changes: 0 additions & 3 deletions boards/sparkfun/pro_micro_rp2040/sparkfun_pro_micro_rp2040-pinctrl.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@
};
};

clocks_default: clocks_default {
};

ws2812_pio0_default: ws2812_pio_default {
ws2812 {
pinmux = <PIO0_P25>;
Expand Down
7 changes: 1 addition & 6 deletions boards/sparkfun/pro_micro_rp2040/sparkfun_pro_micro_rp2040.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

/dts-v1/;

#include <rpi_pico/rp2040.dtsi>
#include <raspberrypi/rpi_pico/rp2040.dtsi>
#include "sparkfun_pro_micro_rp2040-pinctrl.dtsi"
#include "sparkfun_pro_micro_connector.dtsi"
#include <freq.h>
Expand Down Expand Up @@ -58,11 +58,6 @@
};
};

&clocks {
pinctrl-0 = <&clocks_default>;
pinctrl-names = "default";
};

&uart0 {
current-speed = <115200>;
status = "okay";
Expand Down
2 changes: 1 addition & 1 deletion boards/wiznet/w5500_evb_pico/board.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# The value must be the 'stem' part of the name of one of the files
# in the openocd interface configuration file.
# The setting is store to CMakeCache.txt.
if ("${RPI_PICO_DEBUG_ADAPTER}" STREQUAL "")
if("${RPI_PICO_DEBUG_ADAPTER}" STREQUAL "")
set(RPI_PICO_DEBUG_ADAPTER "cmsis-dap")
endif()

Expand Down
3 changes: 0 additions & 3 deletions boards/wiznet/w5500_evb_pico/w5500_evb_pico-pinctrl.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,4 @@
input-enable;
};
};

clocks_default: clocks_default {
};
};
7 changes: 1 addition & 6 deletions boards/wiznet/w5500_evb_pico/w5500_evb_pico.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

#include <freq.h>

#include <rpi_pico/rp2040.dtsi>
#include <raspberrypi/rpi_pico/rp2040.dtsi>
#include "w5500_evb_pico-pinctrl.dtsi"
#include <zephyr/dt-bindings/pwm/pwm.h>

Expand Down Expand Up @@ -112,11 +112,6 @@
};
};

&clocks {
pinctrl-0 = <&clocks_default>;
pinctrl-names = "default";
};

&uart0 {
current-speed = <115200>;
status = "okay";
Expand Down
7 changes: 3 additions & 4 deletions drivers/clock_control/clock_control_rpi_pico.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,14 +608,13 @@ static int clock_control_rpi_pico_init(const struct device *dev)
/* Reset all function before clock configuring */
reset_block(~(RESETS_RESET_IO_QSPI_BITS | RESETS_RESET_PADS_QSPI_BITS |
RESETS_RESET_PLL_USB_BITS | RESETS_RESET_USBCTRL_BITS |
RESETS_RESET_PLL_USB_BITS | RESETS_RESET_SYSCFG_BITS |
RESETS_RESET_PLL_SYS_BITS));
RESETS_RESET_SYSCFG_BITS | RESETS_RESET_PLL_SYS_BITS));

unreset_block_wait(RESETS_RESET_BITS &
~(RESETS_RESET_ADC_BITS | RESETS_RESET_RTC_BITS |
RESETS_RESET_SPI0_BITS | RESETS_RESET_SPI1_BITS |
RESETS_RESET_UART0_BITS | RESETS_RESET_UART1_BITS |
RESETS_RESET_USBCTRL_BITS | RESETS_RESET_PWM_BITS));
RESETS_RESET_USBCTRL_BITS));

/* Start tick in watchdog */
watchdog_hw->tick = ((CLOCK_FREQ_xosc/1000000) | WATCHDOG_TICK_ENABLE_BITS);
Expand Down Expand Up @@ -714,7 +713,7 @@ static int clock_control_rpi_pico_init(const struct device *dev)
}

ret = pinctrl_apply_state(config->pcfg, PINCTRL_STATE_DEFAULT);
if (ret < 0) {
if (ret < 0 && ret != -ENOENT) {
return ret;
}

Expand Down
3 changes: 1 addition & 2 deletions drivers/dma/dma_rpi_pico.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,7 @@ static bool dma_rpi_pico_api_chan_filter(const struct device *dev, int ch, void
uint32_t filter;

if (!filter_param) {
LOG_ERR("filter_param must not be NULL");
return false;
return true;
}

filter = *((uint32_t *)filter_param);
Expand Down
0 dts/arm/rpi_pico/override.dtsi → dts/arm/raspberrypi/rpi_pico/override.dtsi
View file
Open in desktop
File renamed without changes.
5 changes: 3 additions & 2 deletions dts/arm/rpi_pico/rp2040.dtsi → dts/arm/raspberrypi/rpi_pico/rp2040.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,15 @@
#include <zephyr/dt-bindings/clock/rpi_pico_clock.h>
#include <zephyr/dt-bindings/i2c/i2c.h>
#include <zephyr/dt-bindings/regulator/rpi_pico.h>
#include <zephyr/dt-bindings/reset/rpi_pico_reset.h>
#include <zephyr/dt-bindings/reset/rp2040_reset.h>
#include <mem.h>

#include <arm/rpi_pico/override.dtsi>
#include <arm/raspberrypi/rpi_pico/override.dtsi>
/*
* This value can be overridden at the board level or in an application specific
* override.dtsi file.
*/

#ifndef RPI_PICO_DEFAULT_IRQ_PRIORITY
#define RPI_PICO_DEFAULT_IRQ_PRIORITY 3
#endif
Expand Down
4 changes: 3 additions & 1 deletion dts/bindings/clock/raspberrypi,pico-xosc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,6 @@ include: raspberrypi,pico-clock.yaml
properties:
startup-delay-multiplier:
type: int
description: Startup delay multiplier
default: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is set to 1 here, I would suggest removing

zephyr/dts/arm/raspberrypi/rpi_pico/rp2040.dtsi

Line 165 in 0954e44

startup-delay-multiplier = <64>;
so the default is consistent across RP2040 and RP2350.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ajf58 can you please address this comment.
Either by following the suggestion or reply why that's not a good idea in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either by following the suggestion or reply why that's not a good idea in this case.

@xudongzheng was the original author of this change, so they're kinda arguing with their past self here, with me as a proxy. So the full suite of options are:

  1. Leave as I've done. This will not change the behaviour of the in-tree RP2040-based boards. This, implicitly, has been approved many many times over as part of another PR.
  2. Only remove startup-delay-multiplier from rp2040.dtsi. This will effectively revert what @xudongzheng added in the first place.
  3. Remove startup-delay-multiplier from rp2040.dtsi and only add it to the in-tree boards where it is known to be needed, i.e. cross match against https://github.com/raspberrypi/pico-sdk/tree/master/src/boards/include/boards . This, IMHO is arguably what the original PR that introduced the delay should have done: it fixes things that are known to need this, and doesn't alter out-of-tree boards' behaviour.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@xudongzheng was the original author of this change, so they're kinda arguing with their past self here,

Ever written code where you later discover this should probably have been done differently or at another location ?
I have done that many times.

Of course I cannot answer for @xudongzheng , but to me this looks like such a case, and when this PR broadly states improvements, then this seems to be one such improvement which can be taken in.

Though I agree it more seems like something which should be a regular comment, and not a change request.

Remove startup-delay-multiplier from rp2040.dtsi and only add it to the in-tree boards where it is known to be needed, i.e. cross match against https://github.com/raspberrypi/pico-sdk/tree/master/src/boards/include/boards . This, IMHO is arguably what the original PR that introduced the delay should have done: it fixes things that are known to need this, and doesn't alter out-of-tree boards' behaviour.

To me this also sounds like the ideal approach.

And as PR author you are free to decide if you want to include this change or not.

But thanks for replying to the comment, as that is the only way for others to know if it has been seen / considered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are free to decide if you want to include this change or not.

I'm not going to be extending the scope of this change beyond what it currently does, i.e. "prevent out-of-tree boards based that use this DTS binding requiring to define a parameter."

It's a remarkable coincidence that in the "future" (subsequent commits)[1] it means that if another SoC uses this same driver it won't have to keep defining something to be equal to one. Someone should make one of those and put it on some boards... maybe get a PR to put them in-tree.

The value is board, not SoC specific, and really shouldn't be in rp2040.dtsi whatsoever. @xudongzheng , perhaps you can raise an issue referencing back to your own PR with these suggestions? I don't have time to do that, nor to action it.

[1] or ~weeks in the past for those humans who measure their life like this.

Copy link
Contributor

@xudongzheng xudongzheng Nov 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have it in my internal RP2350 notes to follow up on startup-delay-multiplier with a PR if it's not resolved here. Making the clock configuration consistent with https://github.com/raspberrypi/pico-sdk/tree/master/src/boards/include/boards seems reasonable.

The important part of #78200 is making the delay configurable, not so much what the default value is. I prefer 64, @soburi somewhere in #77368 mentioned preferring 1 to be consistent with the Pico SDK. I'm inclined to defer to his opinion as the maintainer.

description: |
Startup delay multiplier. The default value matches the Pico SDK.
36 changes: 36 additions & 0 deletions include/zephyr/dt-bindings/reset/rp2040_reset.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright (c) 2021 Yonatan Schachter
*
* SPDX-License-Identifier: Apache-2.0
*/

#ifndef ZEPHYR_INCLUDE_DT_BINDINGS_RESET_RP2040_RESET_H_
#define ZEPHYR_INCLUDE_DT_BINDINGS_RESET_RP2040_RESET_H_

#define RPI_PICO_RESETS_RESET_ADC 0
#define RPI_PICO_RESETS_RESET_BUSCTRL 1
#define RPI_PICO_RESETS_RESET_DMA 2
#define RPI_PICO_RESETS_RESET_I2C0 3
#define RPI_PICO_RESETS_RESET_I2C1 4
#define RPI_PICO_RESETS_RESET_IO_BANK0 5
#define RPI_PICO_RESETS_RESET_IO_QSPI 6
#define RPI_PICO_RESETS_RESET_JTAG 7
#define RPI_PICO_RESETS_RESET_PADS_BANK0 8
#define RPI_PICO_RESETS_RESET_PADS_QSPI 9
#define RPI_PICO_RESETS_RESET_PIO0 10
#define RPI_PICO_RESETS_RESET_PIO1 11
#define RPI_PICO_RESETS_RESET_PLL_SYS 12
#define RPI_PICO_RESETS_RESET_PLL_USB 13
#define RPI_PICO_RESETS_RESET_PWM 14
#define RPI_PICO_RESETS_RESET_RTC 15
#define RPI_PICO_RESETS_RESET_SPI0 16
#define RPI_PICO_RESETS_RESET_SPI1 17
#define RPI_PICO_RESETS_RESET_SYSCFG 18
#define RPI_PICO_RESETS_RESET_SYSINFO 19
#define RPI_PICO_RESETS_RESET_TBMAN 20
#define RPI_PICO_RESETS_RESET_TIMER 21
#define RPI_PICO_RESETS_RESET_UART0 22
#define RPI_PICO_RESETS_RESET_UART1 23
#define RPI_PICO_RESETS_RESET_USBCTRL 24

#endif /* ZEPHYR_INCLUDE_DT_BINDINGS_RESET_RP2040_RESET_H_ */
31 changes: 0 additions & 31 deletions include/zephyr/dt-bindings/reset/rpi_pico_reset.h
View file
Open in desktop

This file was deleted.

9 changes: 9 additions & 0 deletions tests/drivers/dma/chan_blen_transfer/socs/rp2040.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright (c) 2024 Andrew Featherstone <[email protected]>
*
* SPDX-License-Identifier: Apache-2.0
*/

tst_dma0: &dma {
status = "okay";
};
Loading