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

drivers: serial: rpi migration to pl011 broke driver #72153

Closed
Explorer001 opened this issue Apr 30, 2024 · 5 comments
Closed

drivers: serial: rpi migration to pl011 broke driver #72153

Explorer001 opened this issue Apr 30, 2024 · 5 comments
Assignees
@soburi
Labels
bug The issue is a bug, or the PR is fixing a bug platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) priority: low Low impact/importance bug Regression Something, which was working, does not anymore

Comments

@Explorer001
Copy link

Describe the bug
Prior to #71074 it was possible to use the uart_pl011 driver with the rpi_pico.
After this pull request the build fails.

To Reproduce

  1. Add the pl011 driver to a rpi_pico based board:
&uart0 {
    compatible = "arm,pl011";
    /delete-property/ resets;
    pinctrl-0 = <&uart0_default>;
    pinctrl-names = "default";
    current-speed = <115200>;
    status = "okay";
    clocks = <&clk_peri>;
};

  1. Try to build with west build

  2. Switching to a commit prior to drivers: serial: rpi_pico: Migrate to pl011 driver #71074 works (e.g 5f6a68a)

Expected behavior
A clean build.

Impact
Driver is not usable.

Logs and console output

Build log:

/data/projects/u2c/firmware_dwt9946/build/app/zephyr/include/generated/devicetree_generated.h:6857:32: error: 'DT_N_S_soc_S_uart_40038000_P_clocks_IDX_0_VAL_ARM_PL011_CLOCK_CTLR_SUBSYS_CELL' undeclared here (not in a function)
 6857 | #define DT_N_INST_1_arm_pl011  DT_N_S_soc_S_uart_40038000
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/sys/util_internal.h:72:26: note: in definition of macro '__DEBRACKET'
   72 | #define __DEBRACKET(...) __VA_ARGS__
      |                          ^~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/sys/util_internal.h:64:9: note: in expansion of macro '__GET_ARG2_DEBRACKET'
   64 |         __GET_ARG2_DEBRACKET(one_or_two_args _if_code, _else_code)
      |         ^~~~~~~~~~~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/sys/util_internal.h:59:9: note: in expansion of macro '__COND_CODE'
   59 |         __COND_CODE(_XXXX##_flag, _if_1_code, _else_code)
      |         ^~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/sys/util_macro.h:180:9: note: in expansion of macro 'Z_COND_CODE_1'
  180 |         Z_COND_CODE_1(_flag, _if_1_code, _else_code)
      |         ^~~~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/devicetree.h:4263:9: note: in expansion of macro 'COND_CODE_1'
 4263 |         COND_CODE_1(DT_HAS_COMPAT_STATUS_OKAY(DT_DRV_COMPAT),   \
      |         ^~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/sys/util_internal.h:69:53: note: in expansion of macro '__DEBRACKET'
   69 | #define __GET_ARG2_DEBRACKET(ignore_this, val, ...) __DEBRACKET val
      |                                                     ^~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/sys/util_internal.h:64:9: note: in expansion of macro '__GET_ARG2_DEBRACKET'
   64 |         __GET_ARG2_DEBRACKET(one_or_two_args _if_code, _else_code)
      |         ^~~~~~~~~~~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/sys/util_internal.h:59:9: note: in expansion of macro '__COND_CODE'
   59 |         __COND_CODE(_XXXX##_flag, _if_1_code, _else_code)
      |         ^~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/sys/util_macro.h:180:9: note: in expansion of macro 'Z_COND_CODE_1'
  180 |         Z_COND_CODE_1(_flag, _if_1_code, _else_code)
      |         ^~~~~~~~~~~~~
/data/projects/u2c/zephyr/drivers/serial/uart_pl011.c:571:9: note: in expansion of macro 'COND_CODE_1'
  571 |         COND_CODE_1(DT_NODE_HAS_COMPAT(DT_INST_CLOCKS_CTLR(n), fixed_clock), (),                   \
      |         ^~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/devicetree.h:1362:9: note: in expansion of macro 'DT_CAT7'
 1362 |         DT_CAT7(node_id, _P_, pha, _IDX_, idx, _VAL_, cell)
      |         ^~~~~~~
/data/projects/u2c/zephyr/include/zephyr/devicetree/clocks.h:208:9: note: in expansion of macro 'DT_PHA_BY_IDX'
  208 |         DT_PHA_BY_IDX(node_id, clocks, idx, cell)
      |         ^~~~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/devicetree/clocks.h:327:9: note: in expansion of macro 'DT_CLOCKS_CELL_BY_IDX'
  327 |         DT_CLOCKS_CELL_BY_IDX(DT_DRV_INST(inst), idx, cell)
      |         ^~~~~~~~~~~~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/sys/util_internal.h:105:36: note: in expansion of macro 'DT_N_INST_1_arm_pl011'
  105 | #define UTIL_PRIMITIVE_CAT(a, ...) a##__VA_ARGS__
      |                                    ^
/data/projects/u2c/zephyr/include/zephyr/sys/util_internal.h:104:26: note: in expansion of macro 'UTIL_PRIMITIVE_CAT'
  104 | #define UTIL_CAT(a, ...) UTIL_PRIMITIVE_CAT(a, __VA_ARGS__)
      |                          ^~~~~~~~~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/devicetree.h:336:31: note: in expansion of macro 'UTIL_CAT'
  336 | #define DT_INST(inst, compat) UTIL_CAT(DT_N_INST, DT_DASH(inst, compat))
      |                               ^~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/devicetree.h:3413:27: note: in expansion of macro 'DT_INST'
 3413 | #define DT_DRV_INST(inst) DT_INST(inst, DT_DRV_COMPAT)
      |                           ^~~~~~~
/data/projects/u2c/zephyr/include/zephyr/devicetree/clocks.h:327:31: note: in expansion of macro 'DT_DRV_INST'
  327 |         DT_CLOCKS_CELL_BY_IDX(DT_DRV_INST(inst), idx, cell)
      |                               ^~~~~~~~~~~
/data/projects/u2c/zephyr/include/zephyr/devicetree/clocks.h:348:9: note: in expansion of macro 'DT_INST_CLOCKS_CELL_BY_IDX'
  348 |         DT_INST_CLOCKS_CELL_BY_IDX(inst, 0, cell)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
/data/projects/u2c/zephyr/drivers/serial/uart_pl011.c:573:58: note: in expansion of macro 'DT_INST_CLOCKS_CELL'
  573 |                      .clock_id = (clock_control_subsys_t)DT_INST_CLOCKS_CELL(n,                    \
      |                                                          ^~~~~~~~~~~~~~~~~~~
/data/projects/u2c/zephyr/drivers/serial/uart_pl011.c:618:17: note: in expansion of macro 'CLOCK_INIT'
  618 |                 CLOCK_INIT(n)                                                           \
      |                 ^~~~~~~~~~
/data/projects/u2c/zephyr/drivers/serial/uart_pl011.c:636:9: note: in expansion of macro 'PL011_CONFIG_PORT'
  636 |         PL011_CONFIG_PORT(n)                                    \
      |         ^~~~~~~~~~~~~~~~~
/data/projects/u2c/firmware_dwt9946/build/app/zephyr/include/generated/devicetree_generated.h:8316:50: note: in expansion of macro 'PL011_INIT'
 8316 | #define DT_FOREACH_OKAY_INST_arm_pl011(fn) fn(0) fn(1)
      |                                                  ^~
/data/projects/u2c/zephyr/include/zephyr/sys/util_internal.h:105:36: note: in expansion of macro 'DT_FOREACH_OKAY_INST_arm_pl011'
  105 | #define UTIL_PRIMITIVE_CAT(a, ...) a##__VA_ARGS__
      |                                    ^
/data/projects/u2c/zephyr/drivers/serial/uart_pl011.c:659:1: note: in expansion of macro 'DT_INST_FOREACH_STATUS_OKAY'
  659 | DT_INST_FOREACH_STATUS_OKAY(PL011_INIT)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~

Environment (please complete the following information):

  • Toolchain: zephyr sdk 0.16.3
  • Zephyr Commit: 950382c
@Explorer001 Explorer001 added the bug The issue is a bug, or the PR is fixing a bug label Apr 30, 2024
@nordicjm nordicjm added platform: Raspberry Pi Raspberry Pi (Broadcom BCM27xx) platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) and removed platform: Raspberry Pi Raspberry Pi (Broadcom BCM27xx) labels Apr 30, 2024
@soburi
Copy link
Member

soburi commented Apr 30, 2024 •
edited

@Explorer001

One question.

Please let me know if there is a reason why you changed this setting from DTS.

    compatible = "arm,pl011";
    /delete-property/ resets;

If you are using rpi_pico's PL011, the default should be equivalent to the behavior of the previous pico driver.

compatible = "arm,pl011"; to explicitly use the pl011 driver. The /delete-property/ resets; is necessary because the pl011 device tree binding has no reset property. With this dts description it was possible to use the pl011 driver prior to the pull request

@Explorer001
Copy link
Author

compatible = "arm,pl011"; to explicitly use the pl011 driver. The /delete-property/ resets; is necessary because the pl011 device tree binding has no reset property. With this dts description it was possible to use the pl011 driver prior to the pull request

@soburi
Copy link
Member

soburi commented Apr 30, 2024

If you use RPI Pico's pl011, I think it is better to use the original configuration.

compatible = "raspberrypi,pico-uart", "arm,pl011";
resets = <&reset RPI_PICO_RESETS_RESET_UART0>;

With this configuration, the pl011 driver will be used, and in the case of rpi_pico, reset at initialization will be handled correctly.

("raspberrypi,pico-uart" is used only as an identifier.)

I don't think so, but If there are additional PL011s other than those implemented in rp2040,

compatible = "arm,pl011";

may need to be done, but the answers above believe otherwise.

@aescolar aescolar added Regression Something, which was working, does not anymore priority: low Low impact/importance bug labels Apr 30, 2024
@soburi
Copy link
Member

soburi commented Apr 30, 2024

@Explorer001

    clocks = <&clk_peri>;

As I can see from this line, I think the example you provided is not working after #62186.
(might not after #71074)

As the rpi_pico platform, the pl011 driver cannot be used as is (because reset and clock are not set correctly), so we created rpi_pico specific driver. So, at that point, I was not expecting to use it in combination with the pl011 driver.

After #71074, we added a mechanism to handle reset and clock to the pl011 driver, so rpi_pico also uses the pl011 driver with the default settings.

In other words, I think you can do what you want with the default settings.

@Explorer001
Copy link
Author

Building with default settings seems to be possible. Ty

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

@soburi soburi

Labels
bug The issue is a bug, or the PR is fixing a bug platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) priority: low Low impact/importance bug Regression Something, which was working, does not anymore
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@soburi @Explorer001 @aescolar @nordicjm