Re: [U-Boot] [PATCH 3/7] STiH410: Add STi serial driver

Hi Simon

On 02/10/2017 05:22 PM, Simon Glass wrote:

Hi Patrice,

On 2 February 2017 at 10:00, patrice.chotard@st.com wrote:

...

From: Patrice Chotard patrice.chotard@st.com

This patch adds support to ASC (asynchronous serial controller) driver, which is basically a standard serial driver. This IP is common across other STMicroelectronics SoCs

Signed-off-by: Patrice Chotard patrice.chotard@st.com

arch/arm/Kconfig | 2 + arch/arm/include/asm/arch-stih410/sti.h | 6 + board/st/stih410-b2260/board.c | 12 ++ drivers/serial/Kconfig | 7 + drivers/serial/Makefile | 1 + drivers/serial/serial_sti_asc.c | 219 ++++++++++++++++++++++++++++++ include/configs/stih410-b2260.h | 1 + include/dm/platform_data/serial_sti_asc.h | 17 +++ 8 files changed, 265 insertions(+) create mode 100644 drivers/serial/serial_sti_asc.c create mode 100644 include/dm/platform_data/serial_sti_asc.h

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 4aa5eb9..b91a5b7 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -985,6 +985,8 @@ config STM32 config ARCH_STI bool "Support STMicrolectronics SoCs" select CPU_V7

•   select DM
  

•   select DM_SERIAL
    help
      Support for STMicroelectronics STiH407/10 SoC family.
      This SoC is used on Linaro 96Board STiH410-B2260
  

diff --git a/arch/arm/include/asm/arch-stih410/sti.h b/arch/arm/include/asm/arch-stih410/sti.h index d35c4f0..f167560 100644 --- a/arch/arm/include/asm/arch-stih410/sti.h +++ b/arch/arm/include/asm/arch-stih410/sti.h @@ -11,4 +11,10 @@ #define STI_A9_CONFIG_BASE 0x08760000 #define STI_A9_GLOBAL_TIMER_BASE (STI_A9_CONFIG_BASE + 0x0200)

+/* STiH410 control registers */ +#define STIH410_COMMS_BASE 0x09800000

+/* ASC UART located in the main "COMMs" block */ +#define STIH410_ASC1_BASE (STIH410_COMMS_BASE + 0x00031000)

Can you get the address of the serial port from the device tree instead, e.g. with dev_get_addr_ptr()?

Will be fixed

...

#endif /* _STI_H_ */ diff --git a/board/st/stih410-b2260/board.c b/board/st/stih410-b2260/board.c index 0c06bca..b2cfa7f 100644 --- a/board/st/stih410-b2260/board.c +++ b/board/st/stih410-b2260/board.c @@ -7,6 +7,9 @@ */

#include <common.h> +#include <asm/arch/sti.h> +#include <dm/platdata.h> +#include <dm/platform_data/serial_sti_asc.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -26,3 +29,12 @@ int board_init(void) { return 0; }

+static const struct sti_asc_serial_platdata serial_platdata = {

•   .base = (struct sti_asc_uart *)STIH410_ASC1_BASE,
  

+};

+U_BOOT_DEVICE(sti_asc) = {

•   .name = "serial_sti_asc",
  

•   .platdata = &serial_platdata,
  

+};

Can you use DT instead of platform data? You have the code for it, so why is this needed?

Yes, i simply forgot to fully convert this driver to DT, i will fix this.

...

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index b11f3ff..7557632 100644 --- a/drivers/serial/Kconfig +++ b/drivers/serial/Kconfig @@ -413,4 +413,11 @@ config PXA_SERIAL If you have a machine based on a Marvell XScale PXA2xx CPU you can enable its onboard serial ports by enabling this option.

+config STI_ASC_SERIAL

•   bool "STMicroelectronics on-chip UART"
  

•   depends on DM_SERIAL && ARCH_STI
  

•   help
  

•     Select this to enable Asynchronous Serial Controller available
  

•     on STiH410 SoC.
  

Anything more to say? Maximum baud rate, or what hardware features the driver supports? E.g. it only seems to support a subset of baud rates.

Will be fixed

...

endmenu diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile index 8430668..84a22ce 100644 --- a/drivers/serial/Makefile +++ b/drivers/serial/Makefile @@ -41,6 +41,7 @@ obj-$(CONFIG_FSL_LINFLEXUART) += serial_linflexuart.o obj-$(CONFIG_ARC_SERIAL) += serial_arc.o obj-$(CONFIG_UNIPHIER_SERIAL) += serial_uniphier.o obj-$(CONFIG_STM32_SERIAL) += serial_stm32.o +obj-$(CONFIG_STI_ASC_SERIAL) += serial_sti_asc.o obj-$(CONFIG_PIC32_SERIAL) += serial_pic32.o obj-$(CONFIG_STM32X7_SERIAL) += serial_stm32x7.o obj-$(CONFIG_BCM283X_MU_SERIAL) += serial_bcm283x_mu.o diff --git a/drivers/serial/serial_sti_asc.c b/drivers/serial/serial_sti_asc.c new file mode 100644 index 0000000..5279836 --- /dev/null +++ b/drivers/serial/serial_sti_asc.c @@ -0,0 +1,219 @@ +/*

• • Support for Serial I/O using STMicroelectronics' on-chip ASC.
• • Copyright (c) 2017
• • Patrice Chotard patrice.chotard@st.com
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h> +#include <dm.h> +#include <serial.h> +#include <asm/arch/sti.h> +#include <asm/io.h>

move that up one line

ok

...

+#include <dm/platform_data/serial_sti_asc.h>

+DECLARE_GLOBAL_DATA_PTR;

+#define BAUDMODE 0x00001000 +#define RXENABLE 0x00000100 +#define RUN 0x00000080 +#define MODE 0x00000001 +#define MODE_8BIT 0x0001 +#define STOP_1BIT 0x0008 +#define PARITYODD 0x0020

+#define STA_TF 0x0200 +#define STA_RBF 0x0001

You could use BIT() if you like

ok

...

+struct sti_asc_uart {

•   u32 baudrate;
  

•   u32 txbuf;
  

•   u32 rxbuf;
  

•   u32 control;
  

•   u32 inten;
  

•   u32 status;
  

•   u32 guardtime;
  

•   u32 timeout;
  

•   u32 txreset;
  

•   u32 rxreset;
  

+};

+/*---- Values for the BAUDRATE Register -----------------------*/

/* Values for the BAUDRATE Register */

ok

...

+#define PCLK (200ul * 1000000ul) +#define BAUDRATE_VAL_M0(bps) (PCLK / (16 * (bps))) +#define BAUDRATE_VAL_M1(bps) ((bps * (1 << 14)) + (1<<13)) / (PCLK/(1 << 6))

+/*

• • MODE 0

• •                   ICCLK
    

• • ASCBaudRate = ----------------

• •               baudrate * 16
    

• • MODE 1

• •               baudrate * 16 * 2^16
    

• • ASCBaudRate = ------------------------

• •                      ICCLK
    

• • NOTE:
• • Mode 1 should be used for baudrates of 19200, and above, as it
• • has a lower deviation error than Mode 0 for higher frequencies.
• • Mode 0 should be used for all baudrates below 19200.
• */

+static int sti_asc_pending(struct udevice *dev, bool input) +{

•   struct sti_asc_serial_platdata *plat = dev->platdata;
  

•   struct sti_asc_uart *const uart = plat->base;
  

•   unsigned long status;
  

•   status = readl(&uart->status);
  

•   if (input)
  

•           return status & STA_RBF;
  

•   else
  

•           return status & STA_TF;
  

+}

+/* called to adjust baud-rate */ +static int sti_asc_serial_setbrg(struct udevice *dev, int baudrate) +{

•   struct sti_asc_serial_platdata *plat = dev->platdata;
  

•   struct sti_asc_uart *const uart = plat->base;
  

drop blank line

ok

...

•   unsigned long val;
  

•   int t, mode = 1;
  

•   switch (baudrate) {
  

•   case 9600:
  

•           t = BAUDRATE_VAL_M0(9600);
  

•           mode = 0;
  

•           break;
  

•   case 19200:
  

•           t = BAUDRATE_VAL_M1(19200);
  

•           break;
  

•   case 38400:
  

•           t = BAUDRATE_VAL_M1(38400);
  

•           break;
  

•   case 57600:
  

•           t = BAUDRATE_VAL_M1(57600);
  

•           break;
  

•   default:
  

•           printf("ASC: unsupported baud rate: %d, using 115200 instead.\n",
  

•                  baudrate);
  

debug() ?

ok

...

•   case 115200:
  

•           t = BAUDRATE_VAL_M1(115200);
  

•           break;
  

•   }
  

•   /* wait for end of current transmission */
  

•   while (sti_asc_pending(dev, false))
  

•           ;
  

Hmm this should not be here. We should be able to call serial_pending() to check that the device is idle, then avoid the loop here.

Maybe there is something needed in serial_uclass.c?

you are right, it's useless

...

•   /* disable the baudrate generator */
  

•   val = readl(&uart->control);
  

•   writel((val & ~RUN), &uart->control);
  

Drop extra brackets

ok

...

•   /* set baud generator reload value */
  

•   writel(t, &uart->baudrate);
  

•   /* reset the RX & TX buffers */
  

•   writel(1, &uart->txreset);
  

•   writel(1, &uart->rxreset);
  

•   /* set baud generator mode */
  

•   if (mode)
  

•           val |= BAUDMODE;
  

•   /* finally, write value and enable ASC */
  

•   writel(val, &uart->control);
  

•   return 0;
  

+}

+/* initialize the ASC */ +static int sti_asc_serial_probe(struct udevice *dev) +{

•   struct sti_asc_serial_platdata *plat = dev->platdata;
  

•   struct sti_asc_uart *const uart = plat->base;
  

•   unsigned long val;
  

•   sti_asc_serial_setbrg(dev, gd->baudrate);
  

•   /*
  

•    * build up the value to be written to CONTROL
  

•    * set character length, bit stop number, odd parity
  

•    */
  

•   val = RXENABLE | RUN | MODE_8BIT | STOP_1BIT | PARITYODD;
  

•   writel(val, &uart->control);
  

•   return 0;
  

+}

+/* blocking function, that returns next char */ +static int sti_asc_serial_getc(struct udevice *dev) +{

•   struct sti_asc_serial_platdata *plat = dev->platdata;
  

•   struct sti_asc_uart *const uart = plat->base;
  

•   /* polling wait: for a char to be read */
  

•   if (!sti_asc_pending(dev, true))
  

•           return -EAGAIN;
  

•   return readl(&uart->rxbuf);
  

+}

+/* write write out a single char */ +static int sti_asc_serial_putc(struct udevice *dev, const char c) +{

•   struct sti_asc_serial_platdata *plat = dev->platdata;
  

•   struct sti_asc_uart *const uart = plat->base;
  

•   /* Stream-LF to CR+LF conversion */
  

•   if (c == 10)
  

•           sti_asc_serial_putc(dev, '\r');
  

This happens in the uclass, you don't need it here.

ok

...

•   /* wait till safe to write next char */
  

•   while (sti_asc_pending(dev, false))
  

•           ;
  

if (sti_asc_pending(dev, false)) return -EAGAIN

(no loops needed / wanted in drivers)

ok

...

•   /* finally, write next char */
  

•   writel(c, &uart->txbuf);
  

•   return 0;
  

+}

+static int sti_ofdata_to_platdata(struct udevice *dev) +{

•   struct sti_asc_serial_platdata *priv = dev_get_priv(dev);
  

•   fdt_addr_t base;
  

•   base = dev_get_addr(dev);
  

•   if (base == FDT_ADDR_T_NONE)
  

•           return -EINVAL;
  

•   priv->base = (struct sti_asc_uart *)base;
  

Can I suggest the name 'regs' for the registers? Normally 'base' is the base address. It's the same value, but regs is a pointer and base is a ulong.

ok will fix this

...

•   return 0;
  

+}

+static const struct dm_serial_ops sti_asc_serial_ops = {

•   .putc = sti_asc_serial_putc,
  

•   .pending = sti_asc_pending,
  

•   .getc = sti_asc_serial_getc,
  

•   .setbrg = sti_asc_serial_setbrg,
  

+};

+static const struct udevice_id sti_serial_of_match[] = {

•   { .compatible = "st,asc" },
  

•   { }
  

+};

+U_BOOT_DRIVER(serial_sti_asc) = {

•   .name = "serial_sti_asc",
  

•   .id = UCLASS_SERIAL,
  

•   .of_match = sti_serial_of_match,
  

•   .ops = &sti_asc_serial_ops,
  

•   .ofdata_to_platdata = sti_ofdata_to_platdata,
  

•   .probe = sti_asc_serial_probe,
  

•   .flags = DM_FLAG_PRE_RELOC,
  

+}; diff --git a/include/configs/stih410-b2260.h b/include/configs/stih410-b2260.h index fade7a1..4fea22a 100644 --- a/include/configs/stih410-b2260.h +++ b/include/configs/stih410-b2260.h @@ -17,6 +17,7 @@ #define CONFIG_SYS_LOAD_ADDR PHYS_SDRAM_1 /* default load addr */

#define CONFIG_BAUDRATE 115200 +#define CONFIG_SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200 }

#define CONFIG_SYS_HZ_CLOCK 1000000000 /* 1 GHz */

diff --git a/include/dm/platform_data/serial_sti_asc.h b/include/dm/platform_data/serial_sti_asc.h new file mode 100644 index 0000000..c8631f1 --- /dev/null +++ b/include/dm/platform_data/serial_sti_asc.h @@ -0,0 +1,17 @@ +/*

• • (C) Copyright 2017
• • Patrice Chotard patrice.chotard@st.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef __SERIAL_STI_ASC_H +#define __SERIAL_STI_ASC_H

+/* Information about a serial port */ +struct sti_asc_serial_platdata {

•   /* address of registers in physical memory */
  

•   struct sti_asc_uart *base;
  

+};

+#endif /* __SERIAL_STI_ASC_H */

1.9.1

Regards, Simon