Skip to content

Conversation

@aykevl
Copy link
Member

@aykevl aykevl commented Jan 4, 2021

I have not yet tested this on an actual stm32 device, because I can't seem to flash the only device I have (a bluepill).

There are a few changes that might affect behavior, both are bugfixes:

  • The OTYPER register only has single bit wide fields while they were treated as two bit wide fields. This could affect I2C on stm32f4/stm32f7 devices. I have fixed this issue but haven't tested whether I2C still works.
  • The constant stm32.FLASH_ACR_LATENCY_2 (value 4) was incorrect, it is now replaced with the correct value of stm32.FLASH_ACR_LATENCY_WS2 (value 2). I suspect there was a confusion between values and bit fields somewhere (2 vs 1 << 2).

If you work with a local checkout of TinyGo, you will need to remove all existing stm32 files (rm src/device/stm32/*) and generate the updated versions (make gen-device-stm32).

@deadprogram
Copy link
Member

Tested blinky1, serial, and mpu6050 examples on my bluepill and it worked.

I did have to hold the reset button and release right after running tinygo flash in order to get my st-link v2 clone to work.

@deadprogram
Copy link
Member

Correction: mpu6050 does not work, seems like you were correct about i2c problem with this change, @aykevl

@aykevl
Copy link
Member Author

aykevl commented Jan 5, 2021

@deadprogram the I2C change is only for other stm32 chips, not for the stm32f103. I'm surprised it stopped working. Can you maybe test whether this PR is really to blame by testing the parent commit as well?

@deadprogram
Copy link
Member

I did test i2c against the dev branch, which did work. Hence why I made the second comment.

@aykevl
Copy link
Member Author

aykevl commented Jan 5, 2021

Huh, strange. Can you post the code you use for testing? I should be able to figure out where it differs.

@deadprogram
Copy link
Member

@aykevl
Copy link
Member Author

aykevl commented Jan 5, 2021

Thank you, I see two differences in runtime.initCLK.

@aykevl
Copy link
Member Author

aykevl commented Jan 5, 2021

I hope I have fixed the issue. I found a bug when configuring the PLL, this should fix it:

  	stm32.RCC.CFGR.SetBits(stm32.RCC_CFGR_SW_PLL) // set clock source to pll
 
 	// wait for PLL to be CLK
-	for !stm32.RCC.CFGR.HasBits(stm32.RCC_CFGR_SWS_PLL) {
+	for !stm32.RCC.CFGR.HasBits(stm32.RCC_CFGR_SWS_PLL << stm32.RCC_CFGR_SWS_Pos) {
 	}
 }

This PR changes the value of RCC_CFGR_SWS_PLL from one that doesn't need shifting to one that does. But because the value exists under the same name in the generated Go file, it didn't result in a compiler error and I didn't check it manually.

(Sidenote: this is even more reason to come up with an alternative to SetBits / ClearBits / HasBits which are too easy to misuse).

@kenbell
Copy link
Member

kenbell commented Jan 6, 2021

I've verified on the nucleo-f722ze with blinky1 and echo - both worked fine

@kenbell
Copy link
Member

kenbell commented Jan 6, 2021

I've also verified on STM32F407G-DISCO1 - blinky1 and echo

@deadprogram
Copy link
Member

Still does not work as expected with this branch on bluepill mpu6050 program. dev branch still works.

@aykevl
Copy link
Member Author

aykevl commented Jan 6, 2021

Okay thanks again for testing, will check again soon what this difference is.

@aykevl
Copy link
Member Author

aykevl commented Jan 6, 2021

This is all very strange. The only difference I can find (that should matter) is the flash latency.

To rule that out, can you try this patch (on the stm32-rs branch)?

diff --git a/src/runtime/runtime_stm32f103.go b/src/runtime/runtime_stm32f103.go
index 9ecfcfd1..52a6d614 100644
--- a/src/runtime/runtime_stm32f103.go
+++ b/src/runtime/runtime_stm32f103.go
@@ -23,7 +23,7 @@ func putchar(c byte) {
 
 // initCLK sets clock to 72MHz using HSE 8MHz crystal w/ PLL X 9 (8MHz x 9 = 72MHz).
 func initCLK() {
-       stm32.FLASH.ACR.SetBits(stm32.FLASH_ACR_LATENCY_WS2)                          // Two wait states, per datasheet
+       stm32.FLASH.ACR.SetBits(4)                                                    // Two wait states, per datasheet
        stm32.RCC.CFGR.SetBits(stm32.RCC_CFGR_PPRE1_Div2 << stm32.RCC_CFGR_PPRE1_Pos) // prescale PCLK1 = HCLK/2
        stm32.RCC.CFGR.SetBits(stm32.RCC_CFGR_PPRE2_Div1 << stm32.RCC_CFGR_PPRE2_Pos) // prescale PCLK2 = HCLK/1
        stm32.RCC.CR.SetBits(stm32.RCC_CR_HSEON)                                      // enable HSE clock

The only other change I can find is related to IRQ_DMA2_Channel3, I'm not sure what's going on there but we shouldn't be using that interrupt.

@deadprogram
Copy link
Member

The patch for stm32.FLASH.ACR worked. Seems like stm32.FLASH_ACR_LATENCY_WS2 is not a valid value here for some reason?

@aykevl
Copy link
Member Author

aykevl commented Jan 8, 2021

Figured out the problem and fixed it (i2cTimeout for the stm32f103). This PR should now work correctly with the stm32f103.

@deadprogram
Copy link
Member

OK, confirmed that does indeed fix the problem. Thanks @aykevl will merge after CI build.

@aykevl
Copy link
Member Author

aykevl commented Jan 8, 2021

Maybe #1561 should be merged first? After which this PR should be updated.

(Haven't yet looked since my review)

@deadprogram
Copy link
Member

@aykevl if you can please give a quick lookover of #1561 seems like everything has been addressed now and we can squash/merge it first as you suggest.

@deadprogram
Copy link
Member

Was trying to resolve the merge conflict, and seems like I must have done something wrong. Trying to fix the CI build now 😿

@deadprogram
Copy link
Member

Seems like some difference in the names generated for the STM32L0x wrapper vs. the other STM32 boards we support:

Here is the code generated in src/device/stm32/stm32f407.go

	GPIO_MODER_MODER0_Pos        = 0x0        // Position of MODER0 field.
	GPIO_MODER_MODER0_Msk        = 0x3        // Bit mask of MODER0 field.
	GPIO_MODER_MODER0_Input      = 0x0        // Input mode (reset state)
	GPIO_MODER_MODER0_Output     = 0x1        // General purpose output mode
	GPIO_MODER_MODER0_Alternate  = 0x2        // Alternate function mode
	GPIO_MODER_MODER0_Analog     = 0x3        // Analog mode

vs. the code generated in src/device/stm32/stm32l0x1.go:

	GPIO_MODER_MODE0_Pos        = 0x0        // Position of MODE0 field.
	GPIO_MODER_MODE0_Msk        = 0x3        // Bit mask of MODE0 field.
	GPIO_MODER_MODE0_Input      = 0x0        // Input mode (reset state)
	GPIO_MODER_MODE0_Output     = 0x1        // General purpose output mode
	GPIO_MODER_MODE0_Alternate  = 0x2        // Alternate function mode
	GPIO_MODER_MODE0_Analog     = 0x3        // Analog mode

This is one of the const values that is causing the build to fail. @aykevl what do you think?

@aykevl
Copy link
Member Author

aykevl commented Jan 9, 2021

Yes, that's the exact kind of thing I had to fix to use the new SVD files.

@aykevl
Copy link
Member Author

aykevl commented Jan 9, 2021

It was a bit more complicated than I thought, in the end I just redefined all the MODER/OSPEEDR/PUPDR bitifields.

This commit changes the number of wait states for the stm32f103 chip to
2 instead of 4. This gets it back in line with the datasheet, but it
also has the side effect of breaking I2C. Therefore, another (seemingly
unrelated) change is needed: the i2cTimeout constant must be increased
to a higher value to adjust to the lower flash wait states - presumably
because the lower number of wait states allows the chip to run code
faster.
@aykevl
Copy link
Member Author

aykevl commented Jan 9, 2021

@ofauchon can you test the LGT92 with this branch? You will need to run git submodule update --init and make gen-device-stm32 after checking out this branch.

@ofauchon
Copy link
Contributor

ofauchon commented Jan 9, 2021

I checked out this branch and regenerate everything from the svds.

Then I built a couple of sample applications and ran them on the LGT92 device.

Everything seems OK for me !

@deadprogram
Copy link
Member

Awesome! Now merging.

@deadprogram deadprogram merged commit 154c7c6 into dev Jan 9, 2021
@deadprogram deadprogram deleted the stm32-rs branch January 9, 2021 20:45
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.

5 participants