raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.77k stars 946 forks source link

Issues with flash writes when VGA PIO output is running (Pico stock speed: 125MHz) #1984

Open XZCE opened 1 month ago

XZCE commented 1 month ago

I'm seeing this in someone else's project (a proper SDK one, but I usually use the Arduino-Pico environment as shown here)...

#include "hardware/structs/xip_ctrl.h"

static uint32_t boot2_copyout[64];

void __no_inline_not_in_flash_func(setup) () {
    memcpy (boot2_copyout, (void *)XIP_BASE, sizeof (boot2_copyout));
    while (1) {
        xip_ctrl_hw->flush = 1;
        while (!(xip_ctrl_hw->stat & XIP_STAT_FLUSH_READY_BITS))
          tight_loop_contents();

        if (memcmp ((void *)XIP_BASE, boot2_copyout, sizeof (boot2_copyout)) != 0)
            break; // Something's wrong!

        digitalWrite (LED_BUILTIN, HIGH);
        delay (100);
        digitalWrite (LED_BUILTIN, LOW);
        delay (100);
    }
}

void __no_inline_not_in_flash_func(loop) () {
}

Back to the real project: I've created a flash memory update routine, and I'm flushing the cache to verify a write, but sometimes it fails to read back any written data - this sketch has the least code which shows what's failing.

Eventually, the memory compare doesn't get back what's in the flash memory chip and the LED stops flashing.

XZCE commented 1 month ago

After some more testing, it seems important to say I was running at 252MHz (what I've heard is "safe" overclocking) but when I run this sketch at 133MHz, it seems quite stable. It may have failed once, but wasn't repeatable. It really starts to show repeatable issues at 200MHz and above. Note there's a lot going on in Arduino-Pico, and the project's SDK firmware

In the Pico SDK firmware, which is copy-to-RAM, I've disabled everything else, no interrupts or anything else going on, and it seems stable at any frequency the main project uses (up to 376MHz - it's a VGA output project, so can use increments of every ~25MHz).

Could this be one part of a RP2040 that just doesn't overclock well in a noisy environment?

XZCE commented 1 month ago

It consistently fails for me at 133MHz when I configure Arduino-Pico "boards.txt" for rpipicow to use the DIV 4 flash boot2 object instead of the default DIV 2 (I know, it should be more stable, but it isn't for me).

Opened a discussion here: https://github.com/earlephilhower/arduino-pico/discussions/2546

peterharperuk commented 1 month ago

for rpipicow to use the DIV 4 flash boot2 object

Is this DIV 4 for the PIO clock? Assuming it is have you tried DIV 4 when you're overclocking the CPU? It doesn't surprise me that you can't set the pio clock to DIV 4 at 133MHz.

XZCE commented 1 month ago

No it's not the PIO clock, it's the SSI clock. Nevertheless, I tested 200MHz with the DIV 4 boot2 and it also fails the same way. Interestingly, it doesn't fail (at least not soon) at 250MHz, but my special build of Arduino-Pico which can overclock to 300MHz (which does lower the PIO clock so I can still control the LED via the WiFi chip) also fails with the DIV 4 boot2.

peterharperuk commented 1 month ago

You might have to attach a full example to demonstrate the problem.

XZCE commented 1 month ago

Yeah, you're probably right, which is why I opened the discussion in the Arduino-Pico repo - since it's running enough additional code in the background to show the issue with the minimal example. Hopefully someone (maybe @earlephilhower ) will try it . Then again, take any moderately complex SDK project (using interrupts, pio and dma) and add in the cache flush & memcmp I've got... and I think that might show up the instability.

XZCE commented 1 month ago

This still uses the VGA library files (fires interrupts, and sends messages to milticore_fifo) but it's pretty concise, the fifo is deep, and the SDK should be discarding messages targeting the fifo if it's full anyway...

#include "vga.h"
#include "vga-modes.h"

//Note: PICO_FLASH_SPI_CLKDIV=4

#include "pico/stdlib.h"
#include "pico/multicore.h"
#include "pico/bootrom.h"
#include "hardware/clocks.h"
#include "hardware/dma.h"
#include "hardware/structs/xip_ctrl.h"

//#define pin 25 // I don't have a Pico
#define pin 1 // LED is connected to this I/O pin (Pico W work-around - I/O pin 1 is next to ground :)

#define PICO_CLOCK_PLL 756000000 // 252MHz - standard voltage
#define PICO_CLOCK_PLL_DIV1 3
#define PICO_CLOCK_PLL_DIV2 1

static uint32_t boot2_copyout[64];
static uint32_t zeros[64];
static volatile uint32_t mytimer;

void __no_inline_not_in_flash_func(sms)(int i)
{
    while (i) {
        busy_wait_us(1000);
        i--;
    }
}

static void __no_inline_not_in_flash_func(flash_enable_xip_via_boot2)(void)
{
    ((void (*)(void))((intptr_t)boot2_copyout+1))();
}

void __no_inline_not_in_flash_func(test)()
{
    gpio_init(pin);
    gpio_put(pin, 0);
    gpio_set_dir(pin, GPIO_OUT);

    memcpy (boot2_copyout, (void *)XIP_BASE, sizeof (boot2_copyout));

    rom_connect_internal_flash_fn connect_internal_flash_func = (rom_connect_internal_flash_fn)rom_func_lookup_inline(ROM_FUNC_CONNECT_INTERNAL_FLASH);
    rom_flash_exit_xip_fn flash_exit_xip_func = (rom_flash_exit_xip_fn)rom_func_lookup_inline(ROM_FUNC_FLASH_EXIT_XIP);
    rom_flash_flush_cache_fn flash_flush_cache_func = (rom_flash_flush_cache_fn)rom_func_lookup_inline(ROM_FUNC_FLASH_FLUSH_CACHE);

    __compiler_memory_barrier();

    while (1)
    {
        connect_internal_flash_func();
        flash_exit_xip_func();
        flash_flush_cache_func(); // Note this is needed to remove CSn IO force as well as cache flushing
        flash_enable_xip_via_boot2();

        if (memcmp ((void *)XIP_BASE, boot2_copyout, sizeof (boot2_copyout)) != 0)
            break; // Something's wrong!

        gpio_put(pin, 1); // LED will look dimly, but constantly, lit
        sms(5);
        gpio_put(pin, 0);
    }
    if (memcmp ((void *)XIP_BASE, zeros, sizeof (zeros)) == 0)
    {
        while (1) // Hardcore slow flash - XIP isn't engaged
        {
            sio_hw->gpio_set = (1 << pin);
            mytimer = 0;
            while (mytimer < 10000000)
                mytimer++;
            sio_hw->gpio_clr = (1 << pin);
            mytimer = 0;
            while (mytimer < 10000000)
                mytimer++;
        }
    }
    while (1) // Well, something weird was read - just leave the LED off
      tight_loop_contents();
}

void __no_inline_not_in_flash_func(proc1Entry)()
{
    //uint32_t dummy;

    while (1)
    {
        multicore_fifo_pop_blocking();
        //while (multicore_fifo_rvalid()) // Clear any others that are stacked up
        //    dummy = sio_hw->fifo_rd;

        busy_wait_us(35); // Weird, but true, this busy delay (pretend scanline render) is needed to show the issue
    }
}

int __no_inline_not_in_flash_func(main)(void)
{
    set_sys_clock_pll(PICO_CLOCK_PLL, PICO_CLOCK_PLL_DIV1, PICO_CLOCK_PLL_DIV2);

    multicore_launch_core1(proc1Entry);

    VgaInitParams params = { 0 };
    params.params = vgaGetParams(VGA_640_480_60HZ);
    setVgaParamsScaleY(&params.params, 2);
    vgaInit(params);

    test();

    return 0;
}

This shows my Pico W with an LED on I/O pin 1 lit solidly, until the issue occurs and it then slow-flashes, showing the XIP device hasn't re-engaged and all I got back were 0x00's.

The VGA files are here:

https://github.com/visrealm/pico9918/tree/main/src/vga

XZCE commented 1 month ago

Ok, that was a bad link to the VGA files, github did that. But you can see where the link should go.

Here is a full example... It sets up PIO, DMA and Ints along with CPU1 doing nothing in particular. This is all copy-to-RAM so nothing is using Flash Memory. Upon loading the example, it may show the LED dimly lit, so it's running and may not show the issue. Unplug the USB port, plug it back in. Again, it might do the same thing, but one in 10 times, it will slow flash soon after power-up. Feel free to use a non-W pico with an LED on IO25 (switch the defines).

//Note: PICO_FLASH_SPI_CLKDIV=4

#include "pico/stdlib.h"
#include "pico/multicore.h"
#include "pico/bootrom.h"
#include "hardware/clocks.h"
#include "hardware/pio.h"
#include "hardware/dma.h"
#include "hardware/structs/xip_ctrl.h"

//#define pin 25 // I don't have a Pico
#define pin 1 // LED is connected to this I/O pin (Pico W work-around - I/O pin 1 is next to ground :)

#define PICO_CLOCK_PLL 756000000 // 252MHz - standard voltage
#define PICO_CLOCK_PLL_DIV1 3
#define PICO_CLOCK_PLL_DIV2 1

#define SYNC_PINS_START 0        // first sync pin gpio number
#define SYNC_PINS_COUNT 2        // number of sync pins (h and v)
#define SYNC_SIZE 1

#define VGA_PIO         pio0_hw  // which pio are we using for vga?
#define SYNC_SM         0        // vga sync state machine index

static uint32_t boot2_copyout[64];
static uint32_t zeros[64];
static volatile uint32_t mytimer;

static uint32_t syncDataSync[SYNC_SIZE];

static int syncDmaChan = 0;
static uint syncDmaChanMask = 1;

static void __no_inline_not_in_flash_func(flash_enable_xip_via_boot2)(void)
{
    ((void (*)(void))((intptr_t)boot2_copyout+1))();
}

void __no_inline_not_in_flash_func(test)()
{
    gpio_init(pin);
    gpio_put(pin, 0);
    gpio_set_dir(pin, GPIO_OUT);

    memcpy (boot2_copyout, (void *)XIP_BASE, sizeof (boot2_copyout));

    rom_connect_internal_flash_fn connect_internal_flash_func = (rom_connect_internal_flash_fn)rom_func_lookup_inline(ROM_FUNC_CONNECT_INTERNAL_FLASH);
    rom_flash_exit_xip_fn flash_exit_xip_func = (rom_flash_exit_xip_fn)rom_func_lookup_inline(ROM_FUNC_FLASH_EXIT_XIP);
    rom_flash_flush_cache_fn flash_flush_cache_func = (rom_flash_flush_cache_fn)rom_func_lookup_inline(ROM_FUNC_FLASH_FLUSH_CACHE);

    __compiler_memory_barrier();

    while (1)
    {
        connect_internal_flash_func();
        flash_exit_xip_func();
        flash_flush_cache_func(); // Note this is needed to remove CSn IO force as well as cache flushing
        flash_enable_xip_via_boot2();

        if (memcmp ((void *)XIP_BASE, boot2_copyout, sizeof (boot2_copyout)) != 0)
            break; // Something's wrong!

        gpio_put(pin, 1);
        busy_wait_us(1);
        gpio_put(pin, 0);
    }
    if (memcmp ((void *)XIP_BASE, zeros, sizeof (zeros)) == 0)
    {
        while (1) // Hardcore slow flash - XIP isn't engaged
        {
            sio_hw->gpio_set = (1 << pin); mytimer = 0; while (mytimer < 10000000) mytimer++;
            sio_hw->gpio_clr = (1 << pin); mytimer = 0; while (mytimer < 10000000) mytimer++;
        }
    }
    while (1) // Well, something weird was read - just leave the LED off
      tight_loop_contents();
}

void __no_inline_not_in_flash_func(proc1Entry)()
{
    while (1)
    {
        //multicore_fifo_pop_blocking();
        busy_wait_us(500);
    }
}

static void __isr __time_critical_func(dmaIrqHandler)(void)
{
  if (dma_hw->ints0 & syncDmaChanMask)
  {
    dma_hw->ints0 = syncDmaChanMask;
    dma_channel_set_read_addr(syncDmaChan, syncDataSync, true);
    //multicore_fifo_push_timeout_us(0, 0);
  }
}

#define vga_sync_wrap_target 0
#define vga_sync_wrap 3

static const uint16_t vga_sync_program_instructions[] = {
            //     .wrap_target
    0x602e, //  0: out    x, 14                      
    0x6002, //  1: out    pins, 2                    
    0x60f0, //  2: out    exec, 16                   
    0x0043, //  3: jmp    x--, 3                     
            //     .wrap
};

static const struct pio_program vga_sync_program = {
    .instructions = vga_sync_program_instructions,
    .length = 4,
    .origin = -1,
    .pio_version = 0,
#if PICO_PIO_VERSION > 0
    .used_gpio_ranges = 0x0
#endif
};

static inline pio_sm_config vga_sync_program_get_default_config(uint offset) {
    pio_sm_config c = pio_get_default_sm_config();
    sm_config_set_wrap(&c, offset + vga_sync_wrap_target, offset + vga_sync_wrap);
    return c;
}

int __no_inline_not_in_flash_func(main)(void)
{
    set_sys_clock_pll(PICO_CLOCK_PLL, PICO_CLOCK_PLL_DIV1, PICO_CLOCK_PLL_DIV2);

    multicore_launch_core1(proc1Entry);

    uint32_t pioDivider = 2;

    for (uint i = 0; i < SYNC_PINS_COUNT; ++i)
      pio_gpio_init(VGA_PIO, SYNC_PINS_START + i);

    // add sync pio program
    uint syncProgOffset = pio_add_program(VGA_PIO, &vga_sync_program);
    pio_sm_set_consecutive_pindirs(VGA_PIO, SYNC_SM, SYNC_PINS_START, SYNC_PINS_COUNT, true);

    // configure sync pio
    pio_sm_config syncConfig = vga_sync_program_get_default_config(syncProgOffset);
    sm_config_set_out_pins(&syncConfig, SYNC_PINS_START, 2);
    sm_config_set_clkdiv(&syncConfig, pioDivider);
    sm_config_set_out_shift(&syncConfig, true, true, 32); // R shift, autopull @ 32 bits
    sm_config_set_fifo_join(&syncConfig, PIO_FIFO_JOIN_TX); // Join FIFOs together to get an 8 entry TX FIFO
    pio_sm_init(VGA_PIO, SYNC_SM, syncProgOffset, &syncConfig);

    // initialise sync dma
    dma_channel_config syncDmaChanConfig = dma_channel_get_default_config(syncDmaChan);
    channel_config_set_transfer_data_size(&syncDmaChanConfig, DMA_SIZE_32);           // transfer 32 bits at a time
    channel_config_set_read_increment(&syncDmaChanConfig, true);                       // increment read
    channel_config_set_write_increment(&syncDmaChanConfig, false);                     // don't increment write 
    channel_config_set_dreq(&syncDmaChanConfig, pio_get_dreq(VGA_PIO, SYNC_SM, true)); // transfer when there's space in fifo

    // setup the dma channel and set it going
    dma_channel_configure(syncDmaChan, &syncDmaChanConfig, &VGA_PIO->txf[SYNC_SM], syncDataSync, SYNC_SIZE, false);
    dma_channel_set_irq0_enabled(syncDmaChan, true);

    irq_set_exclusive_handler(DMA_IRQ_0, dmaIrqHandler);
    irq_set_enabled(DMA_IRQ_0, true);
    pio_sm_set_enabled(VGA_PIO, SYNC_SM, true);
    dma_channel_start(syncDmaChan);

    test();

    return 0;
}
lurch commented 1 month ago

I've created a flash memory update routine, and I'm flushing the cache to verify a write, but sometimes it fails to read back any written data

when I run this sketch at 133MHz, it seems quite stable.

The Pico Datasheet says that the Pico uses a Winbond W25Q16JV flash chip; and looking up the datasheet for that chip it says "133MHz Single, Dual/Quad SPI clocks". So maybe you simply need to slow the system clock down to (at least) 133MHz before trying to write any data to flash (and flushing the cache), and then switch it back up to a higher frequency after you've finished writing? (that's just my uneducated guess :wink: )

XZCE commented 1 month ago

It's a good idea @lurch but the project this is in, is a VGA output device, so when doing flash updates you would lose sync.

I did try though, leaving the speed at the SDK default (I assume 125MHz - by not calling set_sys_clock_pll) along with DIV 4 on the flash chip (32MHz in that case) and my last program I posted kills the flash XIP engine 1 in 3 times at power-up. I've also tried memory-to-memory DMA along with timer Interrupts, but those alone work flawlessly, the XIP engine never crashes at power-up. It's just this combination of PIO, DMA and Ints.

I should also say, this is "copy_to_ram" via the CMakeList.txt file - even though I've got the functions all marked as "not_in_flash" - that's just because Arduino-Pico doesn't support copy_to_ram - but I'm not using that anymore, this is all SDK now.

XZCE commented 1 month ago

I got 2 more Pico W's - one is new and one is really quite old, that came from a disassembled project... Both of these units have no issue with the program I posted, the XIP engine seems to be working solidly on every power-up.

So, 2 devices with issues (one isn't a Pico) and 2 devices without issues :(

peterharperuk commented 1 month ago

Do you have problems on normal Pico's (not Pico W)? Are you referring to the internal led? Some of your posts look like you're talking about an external led which would imply this has nothing to do with Pico W. If you're having issues with the Pico W internal led then it's likely communication with the cyw43 chip has broken down. Do you see any relevant debug on the uart? If you're messing about with the system clock this will impact the pio used for comms to cyw43. I'm not sure you'll get more help unless you can attach a full working example including cmake files.

XZCE commented 1 month ago

I don't have any "normal Pico's" to try and I only used my Pico W's internal LED on the first Arduino sketch, where I quickly realised that might have those issues, so I moved to an external LED with a current limiting resistor on I/O pin 1. The fact that a cyw43 build can't be fully run from RAM on a RP2040 (500KB to control an LED?) made me abandon that.

The Pico (W) isn't crashing, the non-Pico device isn't either, XIP just isn't being re-engaged. In both devices (the Pico W and the non-Pico VGA device) I have worked around the issue by calling the flash-write a 2nd time (I don't think the 256 byte write actually fails, but calling the function a 2nd time doesn't ruin the 1st write, and does give it a 2nd chance to re-engage XIP) and that works... In the real project and the test code. My code change tries a 3rd time too in the project if needed, but I've never seen that happen.

My bad Pico W shows, on a bad boot, it switching successfully in and out of XIP, getting an error, switching back for a very short while, then getting errors again... Repeat.. On a good boot, it just works.

I think the clock is somehow broken, on both these bad devices. All my Pico's have come from the same authorised distributor too, the bad one isn't a fake.

lurch commented 1 month ago

I got 2 more Pico W's - one is new and one is really quite old, that came from a disassembled project... Both of these units have no issue with the program I posted, the XIP engine seems to be working solidly on every power-up.

So, 2 devices with issues (one isn't a Pico) and 2 devices without issues :(

Seems unlikely to me that "the XIP engine" (which is internal to the RP2040) would be "good" in some chips but "bad" in others? It might be the flash chip that is determining the difference in behaviour between your "good" and "bad" devices?

peterharperuk commented 1 month ago

I changed the issue title to be more relevant to the reported problem?

XZCE commented 1 month ago

Yeah, no problem here with a title change... I was hoping for that, not knowing what's going on and all (neither device showing issues is mine). EDIT: mine, as in my design.. obviously I own them.

XZCE commented 4 weeks ago

I've changed the issue title, here's an example project that just has the VGA PIO component running while performing all the SDK ROM calls that disengage and reengage XIP. All 3 of my Pico W's show it failing to reengage, if it doesn't happen soon after booting you should cycle power and try again. This project isn't changing the stock speed from whatever the default is (125MHz?)

pico9918-gpu-dev_f.zip

XZCE commented 4 weeks ago

Let me know, if it doesn't present as a problem on your Pico/Pico W. Peter.

XZCE commented 2 weeks ago

We (the project owner and I) are now testing on RP2350B chips, and I'm happy to report this issue doesn't affect that chip, only RP2040 ones.

Please let me know if you need me refine the test example.

Wren6991 commented 1 day ago

The MCVE from the arduino-pico issue seems (based on my reading) to be able to reproduce this without any PIO involvement, just core 0 doing flash operations. Is that right, or is there some configuration I'm missing?

I can look into this but I'll remove it from the 2.1 release as it's not even clear it's an SDK issue.

XZCE commented 1 day ago

I only tested the Arduino version on one particular Pico-W and it was overclocked to 252MHz. At speeds below 200MHz, it was fine. When I had more Pico-Ws available to try, I was only testing the non-Arduino example code, but most of the Picos worked OK with that, to my surprise. The final example I posted is the one which shows issues on all the Pico-Ws I have access to.

matsobdev commented 1 day ago

The fact that a cyw43 build can't be fully run from RAM on a RP2040 (500KB to control an LED?) made me abandon that.

Just for the record, you can leave Bluetooth firmware and combined WiFi firmware or just WiFi (if using WiFi only) firmware in flash. Example for Bluetooth firmware:

#ifdef MOD_CYW43_IN_FLASH
const unsigned char __attribute__((aligned(4))) __in_flash("CYW43439_FIRMWARE") cyw43_btfw_43439[] CYW43_RESOURCE_ATTRIBUTE = {
#else
const unsigned char cyw43_btfw_43439[] CYW43_RESOURCE_ATTRIBUTE = {
#endif

or just add __in_flash("CYW43439_FIRMWARE") and that's it. And then firmwares won't be copied over to SRAM even for copy_to_ram binaries.