Re: STM32F103x port

Kalowsky, Daniel <daniel.kalowsky@...>

General feedback inline below.

-----Original Message-----
From: Maciek Borzecki [mailto:maciek.borzecki(a)]
Sent: Sunday, February 28, 2016 2:27 AM
To: devel(a); users(a)
Subject: [devel] Re: STM32F103x port

Hi list,

It has been a slow weekend, as part of my self-doubt recovery after a bad
Codility experience I've started writing drivers for RCC, UART and pinmux for
STM32F10x chips. The changes are pushed to bboozzoo/stm32f103-next
branch here:
Beware, I'm treating this branch as a backup of my local work, so there might
be force pushes from time to time.
As said before, I've also been working on an STM32F2xx port. Looking at your changes, it might make sense to do something a little different for both of us, and try to re-use portions of code from each.

First suggestion, create an arch/arm/soc/stm32, and use the Kconfig to allow selecting of the various flavors of the STM32 chip. This would be similar to what you've already got with the Kconfig.soc.stm32f103ve file, merged with the values from your Kconfig.soc. Then keeping the Kconfig to the pieces generic to all the STM32 portions (i.e. flash size, base address, etc).


Second thing to add, in your use of addresses, please add a comment where these values were originally sourced from (aka the DataSheet document to be used for cross-referencing). Specifically looking at your soc.h.

Third, I like your rcc.h, using the union for structs. In my opinion this makes things a lot cleaner. This is also has been a bit of a contention for the project between several of us. :-) Two things on this. 1) rename val to raw, it keeps it consistent with other locations where this has been in use. 2) you may also need to add #define register definitions for these. I've got a bunch already typed up that I can share with you off-list to save some typing (if you want it).

The demo code has been archived in bboozzoo/stm32f103-demo branch.

Once I deem the work somewhat feature complete, I'll clean that up and
push for review. I'd be glad if someone took a look at the code and shared
their opinion on whether the path I took seems reasonable.

I think there might be some room for extending clock control driver API. The
problem comes form the fact that some chips may a more elaborate clock
distribution within the SoC itself. For instance, inside the STM32F103x chip,
there are at least 2 clock domains driving the peripherals (low speed clock
PCLK1 and high speed PCLK2). When setting up UARTx baud rate one needs
to know the clock rate in order to calculate the timings for the peripheral.
Also, on this particular chip
USART1 is driven by PCLK2, while the remaining for UARTx are driven by
PLCK1. Finding out the rate of the clock driving particular peripheral is useful
if we want to keep things generic to some extent.

I've added the following call to driver specific part of the API:

void stm32f10x_clock_control_get_subsys_rate(struct device *clock,
clock_control_subsys_t subsys,
uint32_t *rate);

where `subsys` is the regular clock subsystem and the clock rate is returned
in `*rate` field.

Since this might be a more general problem, I think the functionality can be
added to the clock_control API:

typedef void (*clock_control_get_clock)(struct device *dev,
clock_control_subsys_t sys,
uint32_t *rate);

struct clock_control_driver_api {
clock_control_get_clock get_clock;

As for the drivers. The RCC (Reset & Clock Control) driver mostly delivers the
CC part of the name. I have intentionally specified a low priority (1) in
DEVICE_INIT() call. The RCC has to be initialized early in the startup process,
otherwise no peripherals will work.

RCC subsytem mapping enums have been put in driver specific header. I did
not feel like these belonged to the SoC specific part as the mappings are
shared by the whole family of SoCs.
I need to look more at this, as in my own port for STM32F2xx I've left the RCC in the SOC section. Not saying that is right, just have left it there for now.

The pinmux driver contains only the parts essential for getting the UART to
work. Again, this is not part of the board specific code, neither the SoC
specific one, as the driver is shared by a family of MCUs. I have looked at the
pinmux driver for Galileo and I understand the the API has been shaped
having this board in mind. While the API methods are sufficient, I have only
implemented the *_get() and *_set() calls. The pin config on STM32F10x is a
bit elaborate so I reused the `func` parameter in *_get()/*_set() calls to pass
driver specific function mappings. The function mapping names are currently
shaped after pinconf-generic Linux driver. Perhaps I'm being too pragmatic
here, but I'd like to avoid replication of STM32Cube's functionality and typing
in all possible pin mappings.
I'm 90% sure that the pinmux can probably be renamed to something like pinmux_stm32, as I believe the functions are the same for the F1xx and F2xx series of chips. I would strongly encourage you to read some just recently posted messages on the mailing list for changes that are coming to the pinmux. It would be best to utilize those early on.

The pinmux you're providing is very SOC specific, which is good.

The UART driver is still using polling, however drive init has been reworked to
use the pinmux and clock_control APIs. The baud rate is not hardcoded
anymore and is calculated based on configuration. The fixed point arithmetic
should be correct for low speeds and close enough for higher speeds.
The UART is looking like it is coming along nicely. Again I think this is code that can be re-used on many of the STM32 chips.

Join to automatically receive all group messages.