search

tinygo-org / tinygo

Go compiler for small places. Microcontrollers, WebAssembly (WASM/WASI), and command-line tools. Based on LLVM.
https://tinygo.org
Other
15.43k stars 911 forks source link

register.ReplaceBits not compatible with SVD generated mask #2166

Closed zdima closed 2 years ago

zdima commented 3 years ago

The SVD define all bit mask (*Msk) as a physical location of the bits, i.e. 0x00300000. The mask argument of a register.ReplaceBits is not really a bit mask, but the size of a field in bits. To get correct result, one should add extra shift when calling it and use position value 0:

https://github.com/tinygo-org/tinygo/blob/64d048c47c95bc0a70b4cff109afc1f9039705a4/src/machine/machine_stm32f103.go#L699

The usage on register.ReplaceBits is mixed with assumptions that mask argument is a positioned bit mask or it is a size of a field mask.

This may produce incorrect result when position is anything but 0, and lead to many hours of debugging "why it is not working?!". https://github.com/tinygo-org/tinygo/blob/157382600535c95bdbf5d6e8c4b9e5ca71749018/src/machine/machine_k210.go#L540 And when correction is made, it introduce unnecessary shift calls.

kendryte.SYSCTL.CLK_TH5.ReplaceBits((0x03<<kendryte.SYSCTL_CLK_TH5_I2C2_CLK_Pos),
     kendryte.SYSCTL_CLK_TH5_I2C2_CLK_Msk, 0)

The common sense and instinct call to use predefined (generated) bit mask as is:

kendryte.SYSCTL.CLK_TH5.ReplaceBits(0x03, kendryte.SYSCTL_CLK_TH5_I2C2_CLK_Msk,
     kendryte.SYSCTL_CLK_TH5_I2C2_CLK_Pos)

The ReplaceBits need a fix to use a real bit mask and code that uses the mask as 'size of a field' fixed.

StoreUint#(&r.Reg, LoadUint#(&r.Reg)&^mask|value<<pos)
zdima commented 2 years ago

The existing ReplaceBits call changes will cause regression in local repo and outside projects. The set of setter and getters provided as part of the SVD with PR #2325.