On 03/13/2017 01:25 PM, Karl Palsson wrote: > Matthew Lai <m...@matthewlai.ca> wrote: > >>> I would ignore it. There's very few places where it will really matter, >>> and the people who will need it, will be handling it themselves anyway, not >>> using high level helper functions. >> It would mean things like: >> rcc_periph_clock_enable(RCC_TIM3); >> timer_set_period(TIM3, 1024); >> >> Would violate this, at high prescalar settings (or low >> prescalar settings with LTO). That is actually the example >> given here: >> https://github.com/libopencm3/libopencm3/blob/master/lib/stm32/common/timer_common_all.c#L70 >> >> It would be a potential source of very difficult to find bugs. > There's plenty of edge cases if you really want to go looking. > I'm not really entirely sure how far to go trying to protect > people. [1] That is true, but in this case, an user that uses libopencm3 functions to setup clocks, enables clock for a timer, and uses it right away will be bitten.
I'd say we should protect the user in this case, since 1) it's a very common sequence, 2) most users probably don't know about it (it's not mentioned at all in reference manuals until f7), 3) resulting bugs will be very hard to find, and 4) I think there's a pretty easy fix (see below). APB1 is usually AHB/4 on f2, f4, and f7 at least. That means 5 cycles are required for f2 and f4, and 8 cycles on f7. I wouldn't want to rely on the compiler to happen to generate that much delay, especially on f7 which is dual-issue (8 cycles can mean 16 instructions). >> Since I imagine rcc_periph_clock_enable() to not be >> performance-critical in most applications, wouldn't adding the >> wait states be better default behaviour, and people who really >> need to enable clocks very quickly can go to registers >> directly? > It's more that I'm not sure you can do it neatly, and be robust > enough to be worth trying. You'll need to interpret what bus a > peripheral is on, as well as what speed it's currently running > at. Doing this using the rcc_axx_speed globals is... ok, but > they're not set if people didn't use clock helpers to set them > up. > > Adding your peripheral speed as a parameter is a gross user API. > For the vast majority of use cases, this is simply never going to > be a problem. Doing any sort of meaningful cycle delay is going > to just add code that most people will never need. > > In my opinion at least :) According to the errata sheets, it's very easy on pre-f7 chips. All that's required is a DSB barrier instruction. We don't actually have to do any cycle counting, or determining which bus the peripheral is on. Unfortunately that option is not mentioned for f7, which requires 2x peripheral clock cycles (could be because of the new AXIM bus). However, there's still no need to figure out which bus a peripheral is on - we can always just do enough delay for APB1. There's no harm in delaying for longer than necessary, unless someone needs to enable clocks really quickly. Figuring out APB1 frequency is not easy. I would just use the globals and add a note in the documentation that users are on their own if they are setting up their own clocks. Matthew ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ libopencm3-devel mailing list libopencm3-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libopencm3-devel
