Skip to content

Fix DMA to SPI transmits on RP2350 #4903

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.

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

Merged
merged 16 commits into from
May 24, 2025
Merged

Fix DMA to SPI transmits on RP2350 #4903

merged 16 commits into from
May 24, 2025

Conversation

hb9cwp
Copy link

@hb9cwp hb9cwp commented May 22, 2025

DMA DREQ line numbers for "flow control" between SPI bus and DMA channels on RP2350 differ from RP2040. Tested with st7789 driver for Pico-1.14-LCD from Waveshare on Pico 2W. Without this fix func st7789.tx() blocks indefinitely while attempting to use DMA to SPI transfers, see #4690.

hb9cwp added 3 commits May 22, 2025 07:47
DMA DREQ line numbers for "flow control" between SPI bus and DMA channels on RP2350 differ from RP2040.
Tested with st7789 driver for Pico-1.14-LCD from Waveshare on Pico 2W. Without this fix func st7789.tx() blocks indefinitely while attempting to use DMA to SPI transfers.
Specific for RP2350, missing in generated src/device/rp/rp2350.go
Specific for RP2040, missing in generated src/device/rp/rp2040.go
@hb9cwp hb9cwp marked this pull request as ready for review May 22, 2025 06:35
@hb9cwp hb9cwp changed the base branch from release to dev May 22, 2025 07:08
Comment on lines 76 to 79
DMA_DREQ_SPI0_TX = 16
DMA_DREQ_SPI0_RX = 17
DMA_DREQ_SPI1_TX = 18
DMA_DREQ_SPI1_RX = 19
Copy link
Contributor

@eliasnaur eliasnaur May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why export these? They're useful, sure, but do they belong in the machine package from the perspective of importing packages?

Copy link
Author

@hb9cwp hb9cwp May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why export these?

Because the generated src/device/rp/rp2040|rp2350.go don't provide them, and they are global and specific for these processors as well.

but do they belong in the machine package

What alternatives do you propose which would be a better fit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/tinygo-org/tinygo/blob/release/src/device/sam/atsamd51x-bitfields.go for example of how to handle values that are not part of the files generated from the SVDs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why export these?

Because the generated src/device/rp/rp2040|rp2350.go don't provide them, and they are global and specific for these processors as well.

but do they belong in the machine package

What alternatives do you propose which would be a better fit?

For every exported API the question should be: what are the use cases for outside users? If there's no identifiable outside need, it's better to keep the API unexported and deal with isssues such as naming, other platforms etc. when the need arises.

If you or some other maintainer (@soypat?) decide to export anyway, I suggest package device/rp. The machine package is for abstractions across a wide range of hardware, and I know of no restriction that say device/rp is for machine generated code only.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, device/rp would be a good place, similar to device/tkey or device/riscv for example.

Unfortunately, it is currently excluded by .gitignore. It would be helpful to have also generated *.go and *.s files in the repo that are versioned. This would facilitate searching and automatic cross-referencing, etc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the additional global constants for DMA to device/rp/rp2nnn0-plus.go, see refactoring below.
But I did not touch some other lines in src/machine/machine_rp2_2nnn.go that could be refactored as well, such as:

const (
	cpuFreq          = 150 * MHz
	_NUMBANK0_GPIOS  = 48
	_NUMBANK0_IRQS   = 6
	rp2350ExtraReg   = 1
	_NUMIRQ          = 51
	notimpl          = "rp2350: not implemented"
	...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the additional global constants for DMA to device/rp/rp2nnn0-plus.go, see refactoring below. But I did not touch some other lines in src/machine/machine_rp2_2nnn.go that could be refactored as well, such as:

const (
	cpuFreq          = 150 * MHz
	_NUMBANK0_GPIOS  = 48
	_NUMBANK0_IRQS   = 6
	rp2350ExtraReg   = 1
	_NUMIRQ          = 51
	notimpl          = "rp2350: not implemented"
	...

These constants are internal implementation details. Why should they be exported?

@hb9cwp hb9cwp changed the title Fix DMA to SPI transfers on RP2350 Fix DMA to SPI transmits on RP2350 May 22, 2025
Comment on lines 1 to 3
// Hand created file. DO NOT DELETE.
// Definitions that are missing in src/device/rp/rp2040.go generated from SVDs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@deadprogram
Copy link
Member

Looks like the new files need to have go fmt run on them, if you please:

Unformatted:
  src/device/rp/rp2040-plus.go
  src/device/rp/rp2350-plus.go
make: *** [GNUmakefile:185: fmt-check] Error 1

@deadprogram
Copy link
Member

Thanks for the format fixes @hb9cwp

Only suggestion I have left is perhaps name the new files src/device/rp/rp2040-bitfields.go to match the sam files.

@hb9cwp
Copy link
Author

hb9cwp commented May 23, 2025

@deadprogram Thanks for re-launching the tests. Yesterday, I could not figure out why one failed. Eventually #4898 is affected as well.

About the naming, I was considering -const first, then contemplated more generic -bis, -extra, or -plus that stipulate extensions of the generated files. Because later, we might also include additional types/classes and functions/methods, much like the generated files. Unless we add those to additional files with specific suffixes rp2nnn-xyz.go?

@deadprogram
Copy link
Member

@hb9cwp I think -extra would be my personal choice of the options, now that you have explained your thinking. Whichever we end up with here, we should consider changing the names of the existing similar files to match.

@deadprogram
Copy link
Member

Thank you for adding this @hb9cwp and for making all the requested changes. Also, thanks @eliasnaur for your in-depth reviews and feedback!

Now squash/merging.

@deadprogram deadprogram merged commit c1bff3d into tinygo-org:dev May 24, 2025
19 checks passed
@hb9cwp hb9cwp deleted the patch-3 branch May 24, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants