Rp2350 b support #4723
Rp2350 b support #4723
Conversation
a. Support for Pins 31 thru 47 b. Updated ADC c. Updated PWM d. Updated Pico2, elecrow and tiny2350 json to add rp2350a buildtag e. Added Amken Max14 board
a. Updated ADC b. including tiny2350.json updates
…add additional PWM channels of Rp2350B
| @@ -2,7 +2,7 @@ | |||
| "inherits": [ | |||
| "rp2350" | |||
| ], | |||
| "build-tags": ["pico2"], | |||
| "build-tags": ["pico2", "rp2350a"], | |||
There was a problem hiding this comment.
I wonder if we should deprecate the existing rp2350 target in favor of a new rp2350a.json so specific boards can just inherit from either rp2350a or rp2350b depending on which chip they are using? That just seems a bit more straightforward to me.
If we really do not want to remove the rp2350 target completely, it could inherit from rp2350a.
There was a problem hiding this comment.
FWIW, I argue for the other way around: define only the rp2350 that covers both rp2350a and rp2350b by exposing all the extra pins etc (rp2350b in this PR). It's then up to boards and/or particular applications to know which model they're running on.
There was a problem hiding this comment.
It is not completely clear that we can do that with just pin mapping, since it appears that there are also things like additional ADC channels that only exist on the "B" version. But perhaps that does not matter for compilation as long as you do not try to use a thing that does not exist on the board you are targeting?
I personally still think being explicit about which variation you are using with our current build tags based system is probably a good thing.
There was a problem hiding this comment.
(Still FWIW) I see your point about being explicit, however consider the advantage of portable firmware builds. That is, assuming (1) that model A is a subset of B, and (2) the added pins/units can easily be detected from registers, I consider it a win that you can build 1 firmware image for both variants.
As a personal example, I'm building on rp2350a but may run out of pins for a future hardware revision and upgrade to rp2350b. I'd love to have 1 firmware image for both boards.
There was a problem hiding this comment.
I support being able to build 1 firmware for both- if it means an extra register read so be it :)
There was a problem hiding this comment.
You still need two build tags as the pin alignment is different. ADC0 starts at GPIO26 for A and at GPIO40 for B variant.
|
|
||
| // Period returns the used PWM period in nanoseconds. | ||
| func (p *pwmGroup) Period() uint64 { | ||
| periodPerCycle := cpuPeriod() |
There was a problem hiding this comment.
Given #4674 (comment), I don't think cpuPeriod should be used anymore; its calculation 1e9/CPUFrequency() won't be integer at 133MHz or 150MHz.
There was a problem hiding this comment.
Why should we not use it anymore?
There was a problem hiding this comment.
cpuPeriod's value is 1e9/CPUFrequency() which for 125MHz is a nice round 8, but for 133MHz or 150MHz is not and thus PWM accuracy will suffer.
There was a problem hiding this comment.
You will like the change that this new commit introduces. You can select from a wide variety of good frequencies all the way upto 300Mhz, many of which produce clean CPU periods.
There was a problem hiding this comment.
Thank you! However, do you know whether CPUFrequency is still constant enough to satisfy the constraints of delay.Sleep? See #4674 (comment)
There was a problem hiding this comment.
I will take a look at it again.
My simplistic solution was that if you cared about "uneven" frequency, just change it to one where it is even. I provided the following carefully selected options
[50,100,125,133,150,175,200,225,240,250,275,300]
But to your point, there are some on this list which are not factors of 2
There was a problem hiding this comment.
I see. To be clear, the current frequency, 125MHz, that both rp2040 and rp2350 run on work fine with cpuPeriod. I bring this is up now because you're in the area, and I'd like to bump both variants to their datasheet maximum (133MHz and 150MHz) in a future PR. I don't want to use frequencies outside datasheet guaranteed frequencies.
There was a problem hiding this comment.
133 and 150 are for the reference design of the Pico. Anyway the datasheet says "Most chips in most normal
environments can run significantly faster than the quoted maximum, and therefore support overclocking" pg 512. The speeds that they choose is a function of the crystal that they used. In my boards, I use a much higher quality cmos oscillator at 40mhz, which i share with the CAN controller as well, which has error in the <10ppm range. In practical applications, even on the pico, you can run 300 mhz without the chip complaining or overheating. I have run rp2040 at 500Mhz with a higher core voltage. The datasheet even tells you how to do it.
There was a problem hiding this comment.
Sure, I'm not saying overclocking is impossible or undesirable. I'm only saying that the PWM divider calculations should handle frequencies that don't nicely divide 1e9, as cpuPeriod assumes.
There was a problem hiding this comment.
Yes. Agreed. I will do that. Thanks for reminding.
src/machine/machine_rp2_pins.go
Outdated
| @@ -1,4 +1,4 @@ | |||
| //go:build rp2040 || rp2350 || ae_rp2040 || badger2040 || challenger_rp2040 || feather_rp2040 || gopher_badge || kb2040 || macropad_rp2040 || nano_rp2040 || pico || qtpy_rp2040 || thingplus_rp2040 || thumby || tufty2040 || waveshare_rp2040_zero || xiao_rp2040 | |||
| //go:build rp2040 || rp2350a || ae_rp2040 || badger2040 || challenger_rp2040 || feather_rp2040 || gopher_badge || kb2040 || macropad_rp2040 || nano_rp2040 || pico || qtpy_rp2040 || thingplus_rp2040 || thumby || tufty2040 || waveshare_rp2040_zero || xiao_rp2040 | |||
There was a problem hiding this comment.
i feel like most of these tags are superfluous, no? Shouldn't it work with just rp2040 || rp2350a ?
Updated all boards to remove xoscFreq const and added an init function to initialize SysClockFrequency (new var) and xoscFreq (changed from const to var). The upshot is that system frequency is now changable in runtime. Added a predefined frequencies map created using the sdk's vcocalc logic.
|
Tested on Pico and Pico2. Good stable overclocking is achieved on both Frequency can now be set at runtime. :) |
|
@amken3d please note build failures in latest commit. |
|
@deadprogram Is there some caching in circleci? I am able to build cleanly with gopher_board selected as target. I updated the board file in [28f938f], but the smoke test keep failing at that spot. machine/root/project/src/machine/board_gopher-badge.go:19:15: undefined: GPIO13 |
|
@deadprogram , need some help. I am not able to figure out why it fails. make test runs clean on my end. |
|
https://github.com/tinygo-org/tinygo/actions/runs/13227290372/job/36919834194?pr=4723#step:10:151 I don't think is cache related |
|
The build failures are related to missing pin mappings when building |
| @@ -2,6 +2,11 @@ | |||
|
|
|||
| package machine | |||
|
|
|||
| func init() { | |||
| SysClockFrequency = Freq133MHz | |||
There was a problem hiding this comment.
This exposes new API on machine package. Why would we want to expose a variable that can be modified by users? Is CPUFrequency() not good enough for your applications?
There was a problem hiding this comment.
CPUFrequency() only gave me what the current sysClk frequency was and not the ability to overclock or under clock. There are several use cases for being able to change system frequency at runtime like peri synchronization, power management etc. This change makes changing clock speeds on the fly possible.
There was a problem hiding this comment.
I don't think changing clock frequency at runtime is in scope of this PR. If for nothing else, runtime clock support doesn't answer the question of what to do about all the code that assumes CPUFrequency never changes.
There was a problem hiding this comment.
I would agree that any changes about clock frequency should be in a separate PR.
| @@ -2,6 +2,11 @@ | |||
|
|
|||
| package machine | |||
|
|
|||
| func init() { | |||
There was a problem hiding this comment.
Avoid init functions in machine package always unless absolutely required. I'd like to understand the problem you are trying to solve so I can propose a better way of doing this.
|
@soypat , Thanks for the review and PR. Your changes for the ADC looks fine to me. My PR does include other things as well. Do you plan to address those as well? |
|
@amken3d @soypat seems from a superficial overview that since the pins and ADC are addressed already in PR #4728 the only thing really "missing" is the PWM support, which would appear to be similar to the ADC changes? Probably another commit to #4728 would do it? After that, adding the new board support could be done in a separate PR @amken3d Then any changes to clock speed can be discussed in yet another separate PR/issue. @cibomahto I think had some ideas on how to do this, before it got pulled into a bigger set of objectives and dropped. |
|
@deadprogram , thanks for the guidance. I will close this PR for now. |
|
Got intellisense back working. Will try to get around to porting your changes @amken3d. Thank you so much for the work you put in :D |
Adding support for the RP2350B
a. Support for Pins 31 thru 47
b. Updated ADC
c. Updated PWM
d. Updated Pico2, elecrow and tiny2350 json to add rp2350a buildtag
e. Added Amken Max14 board (Intelligent motor controller board in Alpha testing stages)
All test pass using make test
ok github.com/tinygo-org/tinygo/builder (cached)
ok github.com/tinygo-org/tinygo/cgo (cached)
ok github.com/tinygo-org/tinygo/compileopts (cached)
ok github.com/tinygo-org/tinygo/compiler (cached)
ok github.com/tinygo-org/tinygo/interp (cached)
ok github.com/tinygo-org/tinygo/transform (cached)
ok github.com/tinygo-org/tinygo 60.810s