Re: [U-Boot] [PATCH 1/1] stm32: Convert serial driver to DM
Hi Kamil,
On 29 November 2015 at 03:38, Kamil Lulko kamil.lulko@gmail.com wrote:
Signed-off-by: Kamil Lulko kamil.lulko@gmail.com
arch/arm/Kconfig | 2 + arch/arm/include/asm/arch-stm32f4/stm32.h | 10 +- board/st/stm32f429-discovery/stm32f429-discovery.c | 13 +- doc/driver-model/serial-howto.txt | 1 - drivers/serial/serial_stm32.c | 201 ++++++++++----------- include/configs/stm32f429-discovery.h | 10 +- include/dm/platform_data/serial_stm32.h | 16 ++ 7 files changed, 135 insertions(+), 118 deletions(-) create mode 100644 include/dm/platform_data/serial_stm32.h
Looks good, a few nits below.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 6542c38..a611ad9 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -674,6 +674,8 @@ config ARCH_UNIPHIER config TARGET_STM32F429_DISCOVERY bool "Support STM32F429 Discovery" select CPU_V7M
select DMselect DM_SERIALconfig ARCH_ROCKCHIP bool "Support Rockchip SoCs" diff --git a/arch/arm/include/asm/arch-stm32f4/stm32.h b/arch/arm/include/asm/arch-stm32f4/stm32.h index 7ca6dc3..6b64d03 100644 --- a/arch/arm/include/asm/arch-stm32f4/stm32.h +++ b/arch/arm/include/asm/arch-stm32f4/stm32.h @@ -3,7 +3,7 @@
- Yuri Tikhonov, Emcraft Systems, yur@emcraft.com
- (C) Copyright 2015
- Kamil Lulko, rev13@wp.pl
*/
- Kamil Lulko, kamil.lulko@gmail.com
- SPDX-License-Identifier: GPL-2.0+
@@ -106,6 +106,14 @@ struct stm32_flash_regs { #define STM32_FLASH_CR_SNB_OFFSET 3 #define STM32_FLASH_CR_SNB_MASK (15 << STM32_FLASH_CR_SNB_OFFSET)
+/*
- Peripheral base addresses
- */
+#define STM32_USART1_BASE (STM32_APB2PERIPH_BASE + 0x1000) +#define STM32_USART2_BASE (STM32_APB1PERIPH_BASE + 0x4400) +#define STM32_USART3_BASE (STM32_APB1PERIPH_BASE + 0x4800) +#define STM32_USART6_BASE (STM32_APB2PERIPH_BASE + 0x1400)
enum clock { CLOCK_CORE, CLOCK_AHB, diff --git a/board/st/stm32f429-discovery/stm32f429-discovery.c b/board/st/stm32f429-discovery/stm32f429-discovery.c index f418186..8bc2d9e 100644 --- a/board/st/stm32f429-discovery/stm32f429-discovery.c +++ b/board/st/stm32f429-discovery/stm32f429-discovery.c @@ -6,7 +6,7 @@
- Pavel Boldin, Emcraft Systems, paboldin@emcraft.com
- (C) Copyright 2015
- Kamil Lulko, rev13@wp.pl
*/
- Kamil Lulko, kamil.lulko@gmail.com
- SPDX-License-Identifier: GPL-2.0+
@@ -17,6 +17,8 @@ #include <asm/arch/stm32.h> #include <asm/arch/gpio.h> #include <asm/arch/fmc.h> +#include <dm/platdata.h> +#include <dm/platform_data/serial_stm32.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -263,6 +265,15 @@ int dram_init(void) return rv; }
+static const struct stm32_serial_platdata serial_platdata = {
.base = (struct stm32_usart *)STM32_USART1_BASE,+};
+U_BOOT_DEVICE(stm32_serials) = {
.name = "serial_stm32",.platdata = &serial_platdata,+};
u32 get_board_rev(void) { return 0; diff --git a/doc/driver-model/serial-howto.txt b/doc/driver-model/serial-howto.txt index 60483a4..76ad629 100644 --- a/doc/driver-model/serial-howto.txt +++ b/doc/driver-model/serial-howto.txt @@ -18,7 +18,6 @@ is time for maintainers to start converting over the remaining serial drivers: serial_pxa.c serial_s3c24x0.c serial_sa1100.c
- serial_stm32.c serial_xuartlite.c usbtty.c
diff --git a/drivers/serial/serial_stm32.c b/drivers/serial/serial_stm32.c index 8b2830b..df2258e 100644 --- a/drivers/serial/serial_stm32.c +++ b/drivers/serial/serial_stm32.c @@ -1,45 +1,18 @@ /*
- (C) Copyright 2015
- Kamil Lulko, rev13@wp.pl
*/
- Kamil Lulko, kamil.lulko@gmail.com
- SPDX-License-Identifier: GPL-2.0+
#include <common.h> +#include <dm.h> #include <asm/io.h> #include <serial.h> #include <asm/arch/stm32.h> +#include <dm/platform_data/serial_stm32.h>
-/*
- Set up the usart port
- */
-#if (CONFIG_STM32_USART >= 1) && (CONFIG_STM32_USART <= 6) -#define USART_PORT (CONFIG_STM32_USART - 1) -#else -#define USART_PORT 0 -#endif -/*
- Set up the usart base address
- --STM32_USARTD_BASE means default setting
- */
-#define STM32_USART1_BASE (STM32_APB2PERIPH_BASE + 0x1000) -#define STM32_USART2_BASE (STM32_APB1PERIPH_BASE + 0x4400) -#define STM32_USART3_BASE (STM32_APB1PERIPH_BASE + 0x4800) -#define STM32_USART6_BASE (STM32_APB2PERIPH_BASE + 0x1400) -#define STM32_USARTD_BASE STM32_USART1_BASE -/*
- RCC USART specific definitions
- --RCC_ENR_USARTDEN means default setting
- */
-#define RCC_ENR_USART1EN (1 << 4) -#define RCC_ENR_USART2EN (1 << 17) -#define RCC_ENR_USART3EN (1 << 18) -#define RCC_ENR_USART6EN (1 << 5) -#define RCC_ENR_USARTDEN RCC_ENR_USART1EN
-struct stm32_serial { +struct stm32_usart { u32 sr; u32 dr; u32 brr; @@ -49,120 +22,136 @@ struct stm32_serial { u32 gtpr; };
-#define USART_CR1_RE (1 << 2) -#define USART_CR1_TE (1 << 3) -#define USART_CR1_UE (1 << 13) +#define USART_CR1_RE (1 << 2) +#define USART_CR1_TE (1 << 3) +#define USART_CR1_UE (1 << 13)
#define USART_SR_FLAG_RXNE (1 << 5) -#define USART_SR_FLAG_TXE (1 << 7) +#define USART_SR_FLAG_TXE (1 << 7)
-#define USART_BRR_F_MASK 0xF +#define USART_BRR_F_MASK 0xF #define USART_BRR_M_SHIFT 4 #define USART_BRR_M_MASK 0xFFF0
DECLARE_GLOBAL_DATA_PTR;
-static const unsigned long usart_base[] = {
STM32_USART1_BASE,STM32_USART2_BASE,STM32_USART3_BASE,STM32_USARTD_BASE,STM32_USARTD_BASE,STM32_USART6_BASE-}; +#define MAX_SERIAL_PORTS 4
-static const unsigned long rcc_enr_en[] = {
RCC_ENR_USART1EN,RCC_ENR_USART2EN,RCC_ENR_USART3EN,RCC_ENR_USARTDEN,RCC_ENR_USARTDEN,RCC_ENR_USART6EN+/*
- RCC USART specific definitions
- */
+#define RCC_ENR_USART1EN (1 << 4) +#define RCC_ENR_USART2EN (1 << 17) +#define RCC_ENR_USART3EN (1 << 18) +#define RCC_ENR_USART6EN (1 << 5)
+/* Array used to figure out which RCC bit needs to be set */ +static const unsigned long usart_port_rcc_pairs[MAX_SERIAL_PORTS][2] = {
{ STM32_USART1_BASE, RCC_ENR_USART1EN },{ STM32_USART2_BASE, RCC_ENR_USART2EN },{ STM32_USART3_BASE, RCC_ENR_USART3EN },{ STM32_USART6_BASE, RCC_ENR_USART6EN }};
-static void stm32_serial_setbrg(void) -{
serial_init();-}
-static int stm32_serial_init(void) +static int stm32_serial_setbrg(struct udevice *dev, int baudrate) {
struct stm32_serial *usart =(struct stm32_serial *)usart_base[USART_PORT];u32 clock, int_div, frac_div, tmp;
struct stm32_serial_platdata *plat = dev->platdata;struct stm32_usart *const usart = plat->base;u32 clock, int_div, frac_div, tmp;
if ((usart_base[USART_PORT] & STM32_BUS_MASK) ==STM32_APB1PERIPH_BASE) {setbits_le32(&STM32_RCC->apb1enr, rcc_enr_en[USART_PORT]);
if (((u32)usart & STM32_BUS_MASK) == STM32_APB1PERIPH_BASE) clock = clock_get(CLOCK_APB1);
} else if ((usart_base[USART_PORT] & STM32_BUS_MASK) ==STM32_APB2PERIPH_BASE) {setbits_le32(&STM32_RCC->apb2enr, rcc_enr_en[USART_PORT]);
else if (((u32)usart & STM32_BUS_MASK) == STM32_APB2PERIPH_BASE) clock = clock_get(CLOCK_APB2);
} else {
else return -1;
}int_div = (25 * clock) / (4 * gd->baudrate);
int_div = (25 * clock) / (4 * baudrate); tmp = ((int_div / 100) << USART_BRR_M_SHIFT) & USART_BRR_M_MASK; frac_div = int_div - (100 * (tmp >> USART_BRR_M_SHIFT)); tmp |= (((frac_div * 16) + 50) / 100) & USART_BRR_F_MASK;
writel(tmp, &usart->brr);setbits_le32(&usart->cr1, USART_CR1_RE | USART_CR1_TE | USART_CR1_UE); return 0;}
-static int stm32_serial_getc(void) +static int stm32_serial_getc(struct udevice *dev) {
struct stm32_serial *usart =(struct stm32_serial *)usart_base[USART_PORT];while ((readl(&usart->sr) & USART_SR_FLAG_RXNE) == 0);
struct stm32_serial_platdata *plat = dev->platdata;struct stm32_usart *const usart = plat->base;if ((readl(&usart->sr) & USART_SR_FLAG_RXNE) == 0)return -EAGAIN;return readl(&usart->dr);}
-static void stm32_serial_putc(const char c) +static int stm32_serial_putc(struct udevice *dev, const char c) {
struct stm32_serial *usart =(struct stm32_serial *)usart_base[USART_PORT];
struct stm32_serial_platdata *plat = dev->platdata;struct stm32_usart *const usart = plat->base;
if (c == '\n')stm32_serial_putc('\r');
if ((readl(&usart->sr) & USART_SR_FLAG_TXE) == 0)return -EAGAIN;
while ((readl(&usart->sr) & USART_SR_FLAG_TXE) == 0); writel(c, &usart->dr);
return 0;}
-static int stm32_serial_tstc(void) +static int stm32_serial_pending(struct udevice *dev, bool input) {
struct stm32_serial *usart =(struct stm32_serial *)usart_base[USART_PORT];u8 ret;
struct stm32_serial_platdata *plat = dev->platdata;struct stm32_usart *const usart = plat->base;
ret = readl(&usart->sr) & USART_SR_FLAG_RXNE;return ret;
if (input)return readl(&usart->sr) & USART_SR_FLAG_RXNE;
This is supposed to return the number of characters, so you should probably return either 0 or 1 here.
E.g.
return readl(&usart->sr) & USART_SR_FLAG_RXNE ? 1 : 0;
elsereturn !(readl(&usart->sr) & USART_SR_FLAG_TXE);}
-static struct serial_device stm32_serial_drv = {
.name = "stm32_serial",.start = stm32_serial_init,.stop = NULL,
You can omit that line
.setbrg = stm32_serial_setbrg,.putc = stm32_serial_putc,.puts = default_serial_puts,.getc = stm32_serial_getc,.tstc = stm32_serial_tstc,-};
-void stm32_serial_initialize(void) +static int stm32_serial_probe(struct udevice *dev) {
serial_register(&stm32_serial_drv);-}
struct stm32_serial_platdata *plat = dev->platdata;struct stm32_usart *const usart = plat->base;unsigned char usart_port = 0xFF;
Please use int (and perhaps -1) instead of unsigned char. There is no benefit to forcing 8-bits on a 32-bit machine. In fact it may make the generated code worse.
unsigned int i;for (i = 0; i < MAX_SERIAL_PORTS; i++) {if ((u32)usart == usart_port_rcc_pairs[i][0]) {usart_port = i;break;}}-__weak struct serial_device *default_serial_console(void) -{
return &stm32_serial_drv;
if (usart_port == 0xFF)return -1;
-EINVAL
if (((u32)usart & STM32_BUS_MASK) == STM32_APB1PERIPH_BASE)setbits_le32(&STM32_RCC->apb1enr,usart_port_rcc_pairs[usart_port][1]);else if (((u32)usart & STM32_BUS_MASK) == STM32_APB2PERIPH_BASE)setbits_le32(&STM32_RCC->apb2enr,usart_port_rcc_pairs[usart_port][1]);
Is this some sort of pinmux setting? Shouldn't you add the required setting to the platdata instead of comparing against a physical address?
elsereturn -1;
-EINVAL
-1 is -EPERM which probably isn't what you want.
setbits_le32(&usart->cr1, USART_CR1_RE | USART_CR1_TE | USART_CR1_UE);return 0;}
+static const struct dm_serial_ops stm32_serial_ops = {
.putc = stm32_serial_putc,.pending = stm32_serial_pending,.getc = stm32_serial_getc,.setbrg = stm32_serial_setbrg,+};
+U_BOOT_DRIVER(serial_stm32) = {
.name = "serial_stm32",.id = UCLASS_SERIAL,.ops = &stm32_serial_ops,.probe = stm32_serial_probe,.flags = DM_FLAG_PRE_RELOC,+}; diff --git a/include/configs/stm32f429-discovery.h b/include/configs/stm32f429-discovery.h index 8191fb2..3e80861 100644 --- a/include/configs/stm32f429-discovery.h +++ b/include/configs/stm32f429-discovery.h @@ -1,6 +1,6 @@ /*
- (C) Copyright 2015
- Kamil Lulko, rev13@wp.pl
*/
- Kamil Lulko, kamil.lulko@gmail.com
- SPDX-License-Identifier: GPL-2.0+
@@ -51,14 +51,6 @@
#define CONFIG_STM32_GPIO #define CONFIG_STM32_SERIAL -/*
- Configuration of the USART
- 1: TX:PA9 RX:PA10
- 2: TX:PD5 RX:PD6
- 3: TX:PC10 RX:PC11
- 6: TX:PG14 RX:PG9
- */
-#define CONFIG_STM32_USART 1
#define CONFIG_STM32_HSE_HZ 8000000
diff --git a/include/dm/platform_data/serial_stm32.h b/include/dm/platform_data/serial_stm32.h new file mode 100644 index 0000000..d1cfcbe --- /dev/null +++ b/include/dm/platform_data/serial_stm32.h @@ -0,0 +1,16 @@ +/*
- (C) Copyright 2015
- Kamil Lulko, kamil.lulko@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- */
+#ifndef __SERIAL_STM32_H +#define __SERIAL_STM32_H
+/* Information about a serial port */ +struct stm32_serial_platdata {
struct stm32_usart *base; /* address of registers in physical memory */+};
+#endif /* __SERIAL_STM32_H */
2.5.0
Regards, Simon
Hi Simon,
On Monday, November 30, 2015 04:17:07 PM you wrote:
if (((u32)usart & STM32_BUS_MASK) == STM32_APB1PERIPH_BASE)setbits_le32(&STM32_RCC->apb1enr,usart_port_rcc_pairs[usart_port][1]);else if (((u32)usart & STM32_BUS_MASK) == STM32_APB2PERIPH_BASE)setbits_le32(&STM32_RCC->apb2enr,usart_port_rcc_pairs[usart_port][1]);Is this some sort of pinmux setting? Shouldn't you add the required setting to the platdata instead of comparing against a physical address?
This is the clock gating. The problem is that depending on which bus the peripheral sits on (e.g. APB1, APB2), different register will be used to enable peripheral's clock. I think figuring out the bus within the driver (at least until there is a proper clk driver) is better than relying on board/dt to provide the correct data. At later stage I plan to move clock gating code out of drivers to clk driver, but even there I think this physical address <-> bus lookup could be used.
Regarding other points, thanks for the review - I will send v2 shortly.
/Kamil
participants (2)
-
Kamil Lulko -
Simon Glass