rp2040: Added PIO support

[yonsch]

Adds support for PIO for the rp2040 soc. PIO programs are not assembled by Zephyr (currently). They should instead be assembled manually and embedded in source code.
The PIO API should preferably be used to implement drivers. For example, to implement UART over the PIO, it should be implemented as a serial driver, selectable by dts. (this is my suggestion at least).
The PIO API from pico-sdk can still be used in application code to implement custom applications.

[nordicjm]

Not tested it but looks good!

[nordicjm]

Comment/details on what's special about the magic number 3?

[yonsch]

It's a byte pointer to a 32 bit register, so adding 3 gives the MSB of the register, which is denoted in the variable name. But I can document it if it's not clear enough.

[rivimey]

Just browsing, but yes, the 3 needs documenting. I guess there's no need to be worried about big/little endianness because the PIO is built into the soc, but if it were me I would include a comment including "BYTE_ORDER" just to be clear.

[yonsch]

This code is based on the uart examples from pico-examples (bsd-3), and the instructions were generated from those examples. Is this problematic in terms of licensing? Should a copyright be added?

[mbolivar-nordic]

@petejohanson would you be able to take a look?

[nordicjm]

@yonsch please rebase

[MrGreensWorkshop]

Hi, I wanted to thank you @yonsch for this PR, it gives wide perspective about PIO usage on ZephyrOS.
I think it's better to split PIO support and to have uart driver as an example in documents or somewhere else. Because we can write almost anything with PIO, but we can't put everything into Zephyr OS right?

It's better to have PIO initiator in driver. (It's good to have consts like frequency in rp2040.dtsi)

static inline void __program_init(PIO pio, uint sm, uint offset, uint in_pin, uint in_pin_count, uint out_pin, uint out_pin_count, float frequency) {
  // 1. Define a config object
  pio_sm_config config = __program_get_default_config(offset);  // 2. Set and initialize the input pins
  sm_config_set_in_pins(&config, in_pin);
  pio_sm_set_consecutive_pindirs(pio, sm, in_pin, in_pin_count, 1);
  pio_gpio_init(pio, in_pin);  // 3. Set and initialize the output pins
  sm_config_set_out_pins(&config, out_pin, out_pin_count);
  pio_sm_set_consecutive_pindirs(pio, sm, out_pin, out_pin_count, 0);  // 4. Set clock divider
  if (frequency < 2000) {
    frequency = 2000;
  }
  float clock_divider = (float) clock_get_hz(clk_sys) / frequency * 1000;
  sm_config_set_clkdiv(&config, clock_divider);  // 5. Configure input shift register
  // args: BOOL right_shift, BOOL auto_push, 1..32 push_threshold
  sm_config_set_in_shift(&config, true, false, 32);  // 6. Configure output shift register
  // args: BOOL right_shift, BOOL auto_push, 1..32 push_threshold
  sm_config_set_out_shift(&config, true, false, 32);  // 7. Join the ISR & OSR
  // PIO_FIFO_JOIN_NONE = 0, PIO_FIFO_JOIN_TX = 1, PIO_FIFO_JOIN_RX = 2
  sm_config_set_fifo_join(&config, PIO_FIFO_JOIN_NONE);  // 8. Apply the configuration
  pio_sm_init(pio, sm, offset, &config);  // 9. Activate the State Machine
  pio_sm_set_enabled(pio, sm, true);
}

taken from : https://medium.com/geekculture/raspberry-pico-programming-with-pio-state-machines-e4610e6b0f29

[yonsch]

@MrGreensWorkshop Thanks for your feedback. Do I understand correctly that you are proposing a generic PIO driver instead of a per peripheral one? There were a few reasons for implementing it this way:

1. PIO is most useful for implementing uncommon peripherals such as PS/2 or DVI. Therefore, it was assumed that most users would prefer ready to use peripherals, that could be enabled as if they were "regular" peripherals, instead of having a generic PIO driver which would require some extra work to make into a specific peripheral.
2. Implementing PIO peripherals through specific driver APIs allows users to use those peripherals using existing Zephyr API. For example, now you can use the PIO UART as a console, or write a character with uart_poll_out, instead of having to use something like pio_uart_poll_out or pio_write.
3. We preferred not to bother with distributing pioasm, since it was only provided as source code as part of pico-sdk. With the current implementation, only the driver's author has to bother with installing the assembler.

And finally, the current implementation still gives access to PIO API, so for special cases a user can still write a custom PIO driver, as part of an out of tree Zephyr driver, or simply as part of the application.

[MrGreensWorkshop]

@yonsch, Actually I gave my insights from hardware driver point of view. As user point view, probably users prefer to use ready to use peripherals and I agree with you. Does anyone have any thoughts?

[iocapa]

@yonsch

If any of your other changes would require breaking an API later, please say so now, so that I might fix it before this is merged.

I don't think you can change the current API to be compatible with #56678, as the changes would break the current implementation.

#56678 cannot really coexist with PICOSDK_USE_PIO since you would end up with different paths trying to allocate the same resources. Also i see PICOSDK_USE_CLAIM creating unnecessary dependencies since any multicore topics should be solved through zephyr specific APIs.

The PIO parent driver in #56678 was designed just as a resource allocator/interrupt router. All child devices need to use the PIO registers directly.
Technically, the PIO part in picosdk is just a wrapper over basic operations and IMO contains a lot of unnecessary overhead for what is does, also some use-cases will result in runtime issues.

For example, when you allocate SMs to different devices you will need to modify some bits in some registers applicable only to those SMs. This requires atomic operations and picosdk functions accessing those registers don't support that. In case of UART, if you have the misfortune to end up in a RMW situation on the same register with another device you could end up in an undefined state.

#56678 uses these to solve that:

2.1.2. Atomic Register Access
Each peripheral register block is allocated 4kB of address space, with registers accessed using one of 4 methods,
selected by address decode.
• Addr + 0x0000 : normal read write access
• Addr + 0x1000 : atomic XOR on write
• Addr + 0x2000 : atomic bitmask set on write
• Addr + 0x3000 : atomic bitmask clear on write

To summarize, my approach would be to break any dependencies on external rpi code (other that the HW structs). This might prove beneficial in the long run as external SW could change and break existing implementations.

[yonsch]

@iocapa

the scheme should be decided by the child driver in a way that makes sense for that specific driver.

Okay, I'm convinced.

I don't think you can change the current API to be compatible with #56678, as the changes would break the current implementation.

It will break some of the implementation, but the question is if it will break an API, or at least require massive changes. For example, if we merged this PR and then you'll need to move lots of DTS around, that would be counter productive. However if you're just replacing a call to pio_add_program with your own implementation, that would be more of an "incremental improvement".
I agree with you that some HAL functionality could be implemented better inside Zephyr, and in the future we might even get rid of the HAL completely (except for hardware definitions as you mentioned).
I would really like to see some of your improvements added, but it is much easier to review in incremental PRs. This PR is pretty old now and has gone through some reviews, so we'd like to merge it. The question is if you see any fundamental problem that will make it too hard for you in the future.

[iocapa]

It will break some of the implementation, but the question is if it will break an API, or at least require massive changes. For example, if we merged this PR and then you'll need to move lots of DTS around, that would be counter productive. However if you're just replacing a call to pio_add_program with your own implementation, that would be more of an "incremental improvement".

#56678 breaks pretty much everything on the source code side:
rpi_pico_pio.h
rpi_pico_pio_util.h
rpi_pico_pio.c
uart_rpi_pico_pio.c

So, technically, it's a redesign. On a diff, the only similar thing remaining is DT_RPI_PICO_PIO_PIN_BY_NAME.
pio_rpi_pico_allocate_sm though it looks similar, is not compatible since there's a use-case where you need to allocate more than 1 state machine in a single call because you need explicit ordering for the INLINE_OUT_EN / OUT_STICKY feature.

Another issue is that going with incremental merges will not really work since the UART in #56678 needs the PIO in the same PR. So, it could be an incremental review, but needs to be within a single PR.

As I said in the previous comment, #56678 is not compatible with pico-sdk. You could technically have an out-of-tree custom driver and still use the SDK (by manually adjusting the allocation scheme), but you cannot really have a in-tree driver using the SDK.
This break was intentional. While the SDK looks ok and there are some examples, they are not really zephyr driver ready and there are some things that the PIO could do which are not really possible with the SDK.
IMO, adding a new driver should follow the same process as other zephyr drivers do: Read the datasheet / Design / Implement.

So, to summarize:
I'm not against merging the current PR.
Just keep in mind that if #56678 should enter (will be rebased as much as possible), in essence it will be just a big diff on the source side (see the files linked above).

[carlescufi]

I'm not against merging the current PR.

Thanks @iocapa. Will merge this one then, and then we can follow-up with improvements.

[carlescufi]

Removed DNM as the Arch WG already decided in favor of this PR

[nordicjm]

Looks good, tested it and it works, with a general serial throughput test the error rate is about 15-20% but there isn't flow control and this is a simple demo so that's acceptable.

[nordicjm]

Suggested change

	The RP2040 SoC comes with two PIO periherals. These are two simple
	The RP2040 SoC comes with two PIO peripherals. These are two simple

[nordicjm]

Query on this sample: this has both UART drivers enabled, will that cause a problem?

[nordicjm]

Pretty sure this can be omitted and parameter set to NULL - @gmarull ?

[gmarull]

+1, can be removed

[gmarull]

int

[gmarull]

Removed DNM as the Arch WG already decided in favor of this PR

A note here: we should not merge something because it comes first, but evaluate both solutions and merge the one that is better.

pinctrl concerns addressed

[MrGreensWorkshop]

LGTM, all of my concerns were fixed. @yonsch Thank you for your time and effort.

There may be a better solution than this, but in my humble opinion, I think we shouldn't be waiting for those to come for months. (fist PR was made on Mar 30, 2022) Let's give people a chance to use it and give some feedback.

[MrGreensWorkshop]

@gmarull
When there is no sm left, I don't see any point in returning EBUSY (Mount device busy); it's not busy at all, so it can't be used.

If you still want to add ENOMEM or ENOSPC, that would be a better choice.

https://docs.zephyrproject.org/latest/develop/languages/c/minimal_libc.html#c.EBUSY

[MrGreensWorkshop]

Could you please explain why you think so?

-EBUSY "Mount device busy" is used for busy. But in this situation, the state machine is not busy. It's not available. Because it's already taken. We are not checking a hardware process to see if it's finished or not. Using busy would be confusing.

EADDRNOTAVAIL would be a better choice. Or something else like EAGAIN (No more contexts).

[MrGreensWorkshop]

@yonsch Since you already changed to -EBUSY, just ignore my comment.

[carlescufi]

Removed DNM as the Arch WG already decided in favor of this PR

A note here: we should not merge something because it comes first, but evaluate both solutions and merge the one that is better.

Agreed, this is not the case here. We took this to the architecture WG, and both PRs were scrutinized. No objections were raised to start with this one and then ask @iocapa and @yonsch to bring features from #56678 in progressively.

[gmarull]

note: this has been merged with a broken sample 6c93cbf. Not all comments were addressed, either. @yonsch please submit a follow-up PR or @iocapa feel free to fix in your PR.

[yonsch]

Well there were still some unresolved issues (like moving the driver under gio/swio/whatever) but I guess @iocapa can take it from here

[iocapa]

Rebased #56678

Please have a look, and let's continue the discussion there.

Ionut