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

2017-02-14 Thread Patrice CHOTARD
Hi Simon

On 02/10/2017 05:22 PM, Simon Glass wrote:
> Hi Patrice,
>
> On 2 February 2017 at 10:00,   wrote:
>> From: Patrice Chotard 
>>
>> 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 
>> ---
>>  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 0x0876
>>  #define STI_A9_GLOBAL_TIMER_BASE   (STI_A9_CONFIG_BASE + 0x0200)
>>
>> +/* STiH410 control registers */
>> +#define STIH410_COMMS_BASE 0x0980
>> +
>> +/* 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 
>> +#include 
>> +#include 
>> +#include 
>>
>>  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 000..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 
>> + *
>> + * SPDX-License-Identifier:GPL-2.0
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>
> move that up one line

ok

>
>> +#include 
>> +
>> +DECLARE_GLOBAL_DAT

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

2017-02-10 Thread Simon Glass
Hi Patrice,

On 2 February 2017 at 10:00,   wrote:
> From: Patrice Chotard 
>
> 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 
> ---
>  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 0x0876
>  #define STI_A9_GLOBAL_TIMER_BASE   (STI_A9_CONFIG_BASE + 0x0200)
>
> +/* STiH410 control registers */
> +#define STIH410_COMMS_BASE 0x0980
> +
> +/* 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()?

> +
>  #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 
> +#include 
> +#include 
> +#include 
>
>  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?

> 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.

> +
>  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 000..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 
> + *
> + * SPDX-License-Identifier:GPL-2.0
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

move that up one line

> +#include 
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define BAUDMODE   0x1000
> +#define RXENABLE   0x0100
> +#define RUN0x0080
> +#define MODE   0x0001
> +#define MODE_8BIT  0x0001
> +#define STOP_1BIT  0x0008
> +#define PARITYODD  0x0020
> +
> +#define STA_TF 0x0200
> +#define STA_RBF