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 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@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;

•   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@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