[WIP] Separate system clock config for rp2xxx devices

[cibomahto]

Here is a first pass at separating the PLL configuration options for the rp2xxx devices, to allow the rp2350 to be configured for 150MHz. I tested (using a logic analyzer) that the UART, USB, I2C, and SPI generate roughly correct speeds.

I moved the configuration into the machine files, but it could be nice to allow a board to override them. If you have a suggestion for how to do that, please let me know.

I know of three issues that should be fixed before submitting the pull request:

• I2C is slow (90khz instead of 100khz), but this appears to be an existing issue and is similar on both parts.
• The 150MHz clock creates a non-integer 6.66ns period, which makes the PWM output inaccurate on the rp2350
• Currently, the board files have a separate, nonfuctional configuration for the on-board crystal. I created a separate issue for this: rp2040 / rp2350 xoscFreq setting is nonfuctional #4673 , but this touches on the same code.

cc @soypat @deadprogram

[deadprogram]

Thank you for the PR @cibomahto

Can you please run go fmt on the changed files?

/bin/sh: 1: node: not found
Unformatted:
  src/machine/machine_rp2_2040.go
  src/machine/machine_rp2_2350.go
make: *** [GNUmakefile:185: fmt-check] Error 1

[eliasnaur]

The rp2040 datasheet says that its cores are capable of 133 MHz. A cursory glance at the datasheet didn't mention any downside to that speed, so why is TinyGo running at 125 MHz? Perhaps it's because of non-integer periods that you mention in the PR description for rp2350? If so, this PR might consider rp2040 at 133 MHz for the same reasons you chose 150 MHz for rp2350.

Failing all that, a comment would be nice.

[cibomahto]

That's a good point, I don't know why it was running at 125 either, but this seems plausible.

[soypat]

Changing out the xoscFreq constant for a package level variable feels off. I also don't understand what we gained from switching out the constant for a xoscFreq field on clockCfg. In any case the constants could remain in each board-level file and the variable be set in a single package level file.

[cibomahto]

Changing out the xoscFreq constant for a package level variable feels off. I also don't understand what we gained from switching out the constant for a xoscFreq field on clockCfg. In any case the constants could remain in each board-level file and the variable be set in a single package level file.

As is, the xoscFreq constant doesn't work- you need to configure the PLLs to match the xoscFreq setting, and they were hard-coded in the clock configuration routine. Otherwise, you won't get the correct CPU clock speed, or the PLL might not actually start.

I guess some other options are:

• Port the PLL configuration solver to go, and run it during the configuration routine to determine the needed settings. Usually this isn't done because it is a little heavy for a boot routine, and there is some nuance about which configuration is best for a given design.
• Make a table of known PLL configurations for the given xoscFreq, and panic if the correct one isn't on the list.
• Drop xoscFreq, it's unlikely that any boards will use a different value than the default. This also implies that you can only run the RP2040 or RP2350 at a single clock speed, but maybe that's good enough.
• Copy all the PLL and clock settings into each board as constants, as you suggested, but then there is a lot of duplication.

In C-style apis, it's a common pattern to have default tables that you can pass to init routines, but I haven't been able to find something similar in TinyGo. The closest so far is for instance the SPI or I2C configuration objects (for example), but in this case the defaults are 'secret', and applied when fields are left at 0, which seems a little weird to me.

[eliasnaur]

Gentle ping from a nobody who looks forward to run my pico2 at 150MHz :)

It seems to me dropping xoscFreq (or its configurability) is the shortest path forward, given that its value is fixed anyway. Then another PR can deal with any future board that uses a different crystal.

[soypat]

My main gripe is preserving the clarity of the existing code. From what I can tell the actual changes to the logic of what is happening are:

• pllSysFreq, dependent on rp2350 vs rp2040
• pllSysPostDiv1, dependent on rp2350 vs rp2040

The way we express hardware/board dependent constants in tinygo are using constants guarded by build tags. This gives us clarity of hardware values which are constant throughout the life of the program. Even in the case where their conceptual value may vary we can still choose to express their default/startup value as a constant.

To implement these constants I'd suggest creating two constants inside src/machine/machine_rp2_2040.go and src/machine/machine_rp2_2350.go to express this hardware dependent variation.

// src/machine/machine_rp2_2040.go
const (
    pllSysFreq     = 125 * MHz
    pllSysPostDiv1 = 6
)

// src/machine/machine_rp2_2350.go
const (
    pllSysFreq     = 150 * MHz
    pllSysPostDiv1 = 5
)

And then use these values where they correspond.

Let me know if it is still unclear and I can try my hand at adding a commit here

[cibomahto]

@soypat That sounds good. Is there a mechanism that allows constants defined in machine-level files to be overridden by a board-level tag?

Two notes:

• Those values for 'pllSysFreq' and 'pllSysPostDiv1' assume xoscFreq is 12, so xoscFreq should be removed from the board files, and moved to machine_rp2_clocks.
• The 133 and 150MHz clock speeds cause don't divide into an integer ns clock frequency, so moving to these frequencies will make at least the PWM peripheral timings inaccurate.

[deadprogram]

Is there a mechanism that allows constants defined in machine-level files to be overridden by a board-level tag?

No, however those could need to be var instead of const, which would allow that to be changed in a board file.

Those values for 'pllSysFreq' and 'pllSysPostDiv1' assume xoscFreq is 12, so xoscFreq should be removed from the board files, and moved to machine_rp2_clocks.

Seems reasonable.

The 133 and 150MHz clock speeds cause don't divide into an integer ns clock frequency, so moving to these frequencies will make at least the PWM peripheral timings inaccurate.

That is however possibly a problem, since things like servos would malfunction.

[eliasnaur]

Is there a mechanism that allows constants defined in machine-level files to be overridden by a board-level tag?

No, however those could need to be var instead of const, which would allow that to be changed in a board file.

Note that some code, such as delay.Sleep, depends on machine.CPUFrequency being constant.

The 133 and 150MHz clock speeds cause don't divide into an integer ns clock frequency, so moving to these frequencies will make at least the PWM peripheral timings inaccurate.

That is however possibly a problem, since things like servos would malfunction.

Is there no way to work around these malfunctions? Does the frequency really have to be runtime configurable to avoid them?

[cibomahto]

Is there a mechanism that allows constants defined in machine-level files to be overridden by a board-level tag?

No, however those could need to be var instead of const, which would allow that to be changed in a board file.

Note that some code, such as delay.Sleep, depends on machine.CPUFrequency being constant.

That's good insight! I'm confused by that code and comment. It seems like the function will work even if CPUFrequency() isn't constant, but just be somewhat inaccurate for short delays. It also seems confusing that it expects a function call to evaluate to a constant.

The 133 and 150MHz clock speeds cause don't divide into an integer ns clock frequency, so moving to these frequencies will make at least the PWM peripheral timings inaccurate.

That is however possibly a problem, since things like servos would malfunction.

Is there no way to work around these malfunctions? Does the frequency really have to be runtime configurable to avoid them?

The scope of this commit is to allow rp2040 and rp2350 to run at different (fixed) clock speeds, and it wasn't intended to be run-time configurable. The PWM timing issue can be corrected by updating the rp2 PWM driver to work with these clock frequencies.

[eliasnaur]

Is there a mechanism that allows constants defined in machine-level files to be overridden by a board-level tag?

No, however those could need to be var instead of const, which would allow that to be changed in a board file.

Note that some code, such as delay.Sleep, depends on machine.CPUFrequency being constant.

That's good insight! I'm confused by that code and comment. It seems like the function will work even if CPUFrequency() isn't constant, but just be somewhat inaccurate for short delays. It also seems confusing that it expects a function call to evaluate to a constant.

Note that I'm not familiar enough with the compiler to know, but: it's possible your clockCfg variable will be deemed read-only, placed in ROM, and then CPUFrequency may still be constant enough for delay.Sleep.

[cibomahto]

I've checked in an updated version of this pull request, changing the clock configurations back to constants.

The PWM generation is still a blocker, and since the performance gain from 125 to 150MHz isn't really significant, I'm no longer sure it's worth rewriting, so I'd be fine with dropping this change set.

[cibomahto]

Closing this for now, if someone wants to take over please do.

[amken3d]

Addressed in #4723.