-
Notifications
You must be signed in to change notification settings - Fork 104
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
Adding VCOM GPIO and USART pin exmple for the stm32l479disco board #30
Conversation
b76d1e8
to
0750519
Compare
I fixed my mistake on the alternate function register for GPIOE |
Recent rcc change need a recent version of stm32l4. I have a PR for the stm32l4x6: stm32-rs/stm32-rs#141 |
I have fix also the clock requirement to select the correct one on chip that does not have HSI48 |
373b992
to
1e227e3
Compare
Can you link me to the board you are using, I can only find a STM32L476 not stm32l479 board? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work so far, could you take a look at the review notes and see what you think?
@@ -52,6 +52,7 @@ stm32l4x2 = ["stm32l4/stm32l4x2"] | |||
stm32l4x3 = ["stm32l4/stm32l4x3"] | |||
stm32l4x5 = ["stm32l4/stm32l4x5"] | |||
stm32l4x6 = ["stm32l4/stm32l4x6"] | |||
stm32l47x = ["stm32l4x6"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment there aren't any plans to support boards by themselves
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add, mainly because it's quite time consuming to get everything behind a feature gate. If you could get some more stuff behind feature gates for your board (usarts, gpio etc), I would definitely be interested in having your board as the first one! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok the idea here is not board specific. The stm32l476 and stm32l475 does not have the HSI48 clock. Looking a the datasheet the HSI48 clock seems to be mainly for the USB, RNG, ... clock source. So Mi idea was to abstract this mater of fact so that we chose the best clock source depending on the MCU capability.
src/rcc.rs
Outdated
@@ -15,6 +15,8 @@ pub trait RccExt { | |||
} | |||
|
|||
impl RccExt for RCC { | |||
|
|||
#[cfg(not(feature = "stm32l47x"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to only feature gate specific fields, instead of the whole RccExt impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crrcr does not exit for stm3247x how do you see implementation of the constrain ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using #[cfg(not(feature = "stm32l47x"))]
on the fields instead of the entire impl, e.g
fn constrain(self) -> Rcc {
Rcc {
ahb1: AHB1 { _0: () },
ahb2: AHB2 { _0: () },
ahb3: AHB3 { _0: () },
apb1r1: APB1R1 { _0: () },
apb1r2: APB1R2 { _0: () },
apb2: APB2 { _0: () },
bdcr: BDCR { _0: () },
csr: CSR { _0: () },
#[cfg(not(feature = "stm32l47x"))]
crrcr: CRRCR { _0: () },
cfgr: CFGR {
hclk: None,
usb48: false,
lsi: false,
pclk1: None,
pclk2: None,
sysclk: None,
pllcfg: None,
},
}
}
src/rcc.rs
Outdated
@@ -28,7 +30,31 @@ impl RccExt for RCC { | |||
crrcr: CRRCR { _0: () }, | |||
cfgr: CFGR { | |||
hclk: None, | |||
hsi48: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HSI48, should stay as it is a valid clock for other L4 chips, and calling it usb48 doesnt quite fit in my mind as it can be used for rng, sdio etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about naming it clk48 ? The idea here is that some MCU does not have the HSI48 clock. A more generic naming would allow use to have the same user code and be able to run it on different L4 MCU.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not that HSI48 is used only for USB, RNG SDMMC clock or MCO output. It look to me that it has been design for USB,RNG SDMMC purose in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like the idea of treating hsi48 and msi as one 48mhz field, I think the clocks
returned from the rcc
should be as close to what the hardware is as possible, and then it should be left up to the peripherals to check it either has hsi48 or msi or pll etc when initializing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so to me there is input clock and output clock. Maybe the idea is to configure a pair of clock:
(in, out, [scale]) ?
may be for now I will just feature gate the HSI48 and add the MSI clock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be for now I will just feature gate the HSI48 and add the MSI clock.
That sounds good to me :).
src/rcc.rs
Outdated
// p. 180 in ref-manual | ||
rcc.crrcr.modify(|_, w| w.hsi48on().set_bit()); | ||
// Wait until HSI48 is running | ||
while rcc.crrcr.read().hsi48rdy().bit_is_clear() {} | ||
} | ||
|
||
|
||
|
||
if cfg!(feature = "stm32l47x") && self.usb48 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for MSI selection, I was thinking about maybe an Enum that has all possible pre configurable speeds, or taking the speed from an Into<Hertz>
value, this would allow future users to use the MSI easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that would be intresting to have it. We need to solve how to enforce 48Mh if MSI is selected for the USB, RNG and SDMMC clock source. The enum seems a better choise as there is a fix set of speed that can be selected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your right, the enum method makes sense. Using the enum allows peripherals easily to check what speed the MSI is running at.
src/serial.rs
Outdated
@@ -181,7 +184,9 @@ macro_rules! hal { | |||
// NOTE(unsafe) atomic read with no side effects | |||
let isr = unsafe { (*$USARTX::ptr()).isr.read() }; | |||
|
|||
Err(if isr.pe().bit_is_set() { | |||
Err(if isr.rtof().bit_is_set() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice this error has been added, for time out detection, but I don't see any code that configures the relevant registers? Could you explain what this does and how it fixes the framing error issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No actually this was an attempt to solve framing error but it seems not link to that change. I can either add the timeout support or remove this branch. Let me know. BTW I didn't saw framing error happening so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As were not really sure what it does, I think its best to remove it. We can always add it back later if its required :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap reverted
src/rcc.rs
Outdated
@@ -28,7 +30,31 @@ impl RccExt for RCC { | |||
crrcr: CRRCR { _0: () }, | |||
cfgr: CFGR { | |||
hclk: None, | |||
hsi48: false, | |||
usb48: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my other comments address this.
@MabezDev I have address your comment let me know if you need some change. |
src/rcc.rs
Outdated
hsi48: bool, | ||
msi: MsiFreq, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have Option instead? That way we can remove the need for a NOMSI variant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming along nicely, I think we're nearly there.
src/rcc.rs
Outdated
RANGE32M = 10, | ||
#[doc = "range 11 around 48 MHz"] | ||
RANGE48M = 11, | ||
#[doc = "no msi selected"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove after changing to Option in Clocks
src/rcc.rs
Outdated
@@ -456,27 +503,73 @@ impl CFGR { | |||
while rcc.csr.read().lsirdy().bit_is_clear() {} | |||
} | |||
|
|||
// Turn on HSI48 if required | |||
if self.hsi48 { | |||
if self.msi != MsiFreq::NOMSI { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the enum is changed, if self.msi
is not None
we can just write the enum as a u8 to the register
pub fn freeze(self, acr: &mut ACR) -> Clocks { | ||
|
||
let (hclk, pclk1, pclk2, ppre1, ppre2, sysclk) = self.common_freeze(acr); | ||
let mut usb_rng = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right in saying these extra freeze function just set usb_rng
? If so what is useful about knowing usb_rng is enabled? I think most peripherals need to know what is providing the clock, so it can set its source appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the both freeze set the usb_rng boolean. The idea here is because of the RNG. The Rng need a clock source at 48Mhz (proven by stm to work correctly) (usb and rng clock share the same line IIUC). But on some mcu you do have HSI48 that once enable will clock the rng source by default. On the other hand if you do not have HSI48 you can always back to the PLL48 clock source or the MSI possibly other clock source but that need to respect the 48Mhs constrain. So I can only enable the usb_rng with MSI (clk48sel register) if the MSI is 48Mhz.
To conclude I need to feature gate this 2 branch in separate freeze as it have to take 2 different decision depending on the HSI48 being available or not.
Looks good, thanks for your contribution :) |
I'm sorry, I missed this activity. Some late comments:
Generally, reconsidering whether mixing L4x2 and L47x (aka L4x6?) is a good idea:
A more specific case in point:
What are the other options?
Thoughts? |
@nickray usb_rng is not a duplicate flags. The clock line is independent from msi or hs48. |
If you check the reference manual this is all about whether HSI48 is available or not. only 476 475 471 does not posses this clock |
VCOM is working as expected. No framing error.