Hi Kamil,
On 29 November 2015 at 03:38, Kamil Lulko <kamil.lulko(a)gmail.com> wrote:
> Signed-off-by: Kamil Lulko <kamil.lulko(a)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 DM
> + select DM_SERIAL
>
> config 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(a)emcraft.com
> *
> * (C) Copyright 2015
> - * Kamil Lulko, <rev13(a)wp.pl>
> + * Kamil Lulko, <kamil.lulko(a)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(a)emcraft.com
> *
> * (C) Copyright 2015
> - * Kamil Lulko, <rev13(a)wp.pl>
> + * Kamil Lulko, <kamil.lulko(a)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(a)wp.pl>
> + * Kamil Lulko, <kamil.lulko(a)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;
> + else
> + return !(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?
> + else
> + return -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(a)wp.pl>
> + * Kamil Lulko, <kamil.lulko(a)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(a)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
