[U-Boot] [PATCH 1/2] arm: lpc32xx: switch serial console to driver model

[Vladimir Zapolskiy <vz@mleia.com>]

Hi Simon,

On 19.12.2015 22:30, Simon Glass wrote:
> Hi Vladimir,
> 
> On 12 December 2015 at 17:48, Vladimir Zapolskiy <vz@mleia.com> wrote:
>> On NXP LPC32xx platform for non-SPL builds the change adds
>> standard (NS16550) and high-speed UARTs to driver model.
>> Due to specific of DM NS16550 device description UART clock can not be
>> got in runtime and by default it is set to 13MHz, if board PERIPH_CLK
>> is different, this should be specified in board configuration file.
>>
>> For SPL builds HSUARTs are disabled and non-DM NS16550 driver is
>> compiled, if needed.
>>
>> The change also updates default configs of devkit3250 and work_92105
>> boards to reflect updates in platform files.
>>
>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>> ---
>>  arch/arm/cpu/arm926ejs/lpc32xx/devices.c   | 37 ++++++++++++++++++++++++++++--
>>  arch/arm/include/asm/arch-lpc32xx/config.h | 32 +++++++++++++++-----------
>>  configs/devkit3250_defconfig               |  1 +
>>  configs/work_92105_defconfig               |  1 +
>>  4 files changed, 56 insertions(+), 15 deletions(-)
> 
>  Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Nits below.
> 
>>
>> diff --git a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
>> index b1c3f8f..447d0cd 100644
>> --- a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
>> +++ b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
>> @@ -5,12 +5,14 @@
>>   */
>>
>>  #include <common.h>
>> -#include <asm/arch/cpu.h>
>> +#include <dm.h>
>> +#include <dm/platform_data/lpc32xx_hsuart.h>
>> +#include <ns16550.h>
> 
> nit: please put this before the dm/ include. Sub-directory include
> should go at the end. See:
> 
> http://www.denx.de/wiki/U-Boot/CodingStyle

Ok, thanks for the hint, I relied only on coding style description from
README.

>> +
>>  #include <asm/arch/clk.h>
>>  #include <asm/arch/uart.h>
>>  #include <asm/arch/mux.h>
>>  #include <asm/io.h>
>> -#include <dm.h>
>>
>>  static struct clk_pm_regs    *clk  = (struct clk_pm_regs *)CLK_PM_BASE;
>>  static struct uart_ctrl_regs *ctrl = (struct uart_ctrl_regs *)UART_CTRL_BASE;
>> @@ -41,6 +43,37 @@ void lpc32xx_uart_init(unsigned int uart_id)
>>                &clk->u3clk + (uart_id - 3));
>>  }
>>
>> +#if !CONFIG_IS_ENABLED(OF_CONTROL) && !defined(CONFIG_SPL_BUILD)
> 
> #ifndef CONFIG_OF_CONTROL
> 
> should be equivalent to this.

Here I want to emphasize that the change is non-SPL specific.

If this patch is applied, then SPL image still contains legacy NS16550
driver, and U-boot image is switched to its DM driver.

!defined(CONFIG_SPL_BUILD) is removed in the second change.

>> +static const struct ns16550_platdata lpc32xx_uart[] = {
>> +       { UART3_BASE, 2, CONFIG_SYS_NS16550_CLK },
>> +       { UART4_BASE, 2, CONFIG_SYS_NS16550_CLK },
>> +       { UART5_BASE, 2, CONFIG_SYS_NS16550_CLK },
>> +       { UART6_BASE, 2, CONFIG_SYS_NS16550_CLK },
>> +};
>> +
>> +#if defined(CONFIG_LPC32XX_HSUART)
>> +static const struct lpc32xx_hsuart_platdata lpc32xx_hsuart[] = {
>> +       { HS_UART1_BASE, },
>> +       { HS_UART2_BASE, },
>> +       { HS_UART7_BASE, },
>> +};
>> +#endif
>> +
>> +U_BOOT_DEVICES(lpc32xx_uarts) = {
>> +#if defined(CONFIG_LPC32XX_HSUART)
>> +       { "lpc32xx_hsuart", &lpc32xx_hsuart[0], },
>> +       { "lpc32xx_hsuart", &lpc32xx_hsuart[1], },
>> +#endif
>> +       { "ns16550_serial", &lpc32xx_uart[0], },
>> +       { "ns16550_serial", &lpc32xx_uart[1], },
>> +       { "ns16550_serial", &lpc32xx_uart[2], },
>> +       { "ns16550_serial", &lpc32xx_uart[3], },
>> +#if defined(CONFIG_LPC32XX_HSUART)
>> +       { "lpc32xx_hsuart", &lpc32xx_hsuart[2], },
>> +#endif
>> +};
>> +#endif
>> +
>>  void lpc32xx_dma_init(void)
>>  {
>>         /* Enable DMA interface */
>> diff --git a/arch/arm/include/asm/arch-lpc32xx/config.h b/arch/arm/include/asm/arch-lpc32xx/config.h
>> index 521bff1..27e60e1 100644
>> --- a/arch/arm/include/asm/arch-lpc32xx/config.h
>> +++ b/arch/arm/include/asm/arch-lpc32xx/config.h
>> @@ -16,16 +16,21 @@
>>  #define CONFIG_NR_DRAM_BANKS_MAX       2
>>
>>  /* UART configuration */
>> -#if (CONFIG_SYS_LPC32XX_UART >= 3) && (CONFIG_SYS_LPC32XX_UART <= 6)
>> -#define CONFIG_SYS_NS16550_SERIAL
>> -#define CONFIG_CONS_INDEX              (CONFIG_SYS_LPC32XX_UART - 2)
>> -#elif  (CONFIG_SYS_LPC32XX_UART == 1) || (CONFIG_SYS_LPC32XX_UART == 2) || \
>> +#if    (CONFIG_SYS_LPC32XX_UART == 1) || (CONFIG_SYS_LPC32XX_UART == 2) || \
>>         (CONFIG_SYS_LPC32XX_UART == 7)
>> +#if defined(CONFIG_SPL_BUILD)
>> +/* SPL images do not support LPC32xx HSUART, UART5 is selected for SPL */
>> +#undef CONFIG_SYS_LPC32XX_UART
>> +#define CONFIG_SYS_LPC32XX_UART                5
>> +#endif
>> +
>> +#if !defined(CONFIG_LPC32XX_HSUART)
>>  #define CONFIG_LPC32XX_HSUART
>> -#else
>> -#error "define CONFIG_SYS_LPC32XX_UART in the range from 1 to 7"
>> +#endif
>>  #endif
>>
>> +#if defined(CONFIG_SPL_BUILD)
>> +#define CONFIG_SYS_NS16550_SERIAL
>>  #define CONFIG_SYS_NS16550_REG_SIZE    -4
>>  #define CONFIG_SYS_NS16550_CLK         get_serial_clock()
>>
>> @@ -33,15 +38,16 @@
>>  #define CONFIG_SYS_NS16550_COM2                UART4_BASE
>>  #define CONFIG_SYS_NS16550_COM3                UART5_BASE
>>  #define CONFIG_SYS_NS16550_COM4                UART6_BASE
>> +#endif
>>
>> -#if defined(CONFIG_LPC32XX_HSUART)
>> -#if    CONFIG_SYS_LPC32XX_UART == 1
>> -#define HS_UART_BASE                   HS_UART1_BASE
>> -#elif  CONFIG_SYS_LPC32XX_UART == 2
>> -#define HS_UART_BASE                   HS_UART2_BASE
>> -#else  /* CONFIG_SYS_LPC32XX_UART == 7 */
>> -#define HS_UART_BASE                   HS_UART7_BASE
>> +#if !defined(CONFIG_SYS_NS16550_CLK)
>> +#define CONFIG_SYS_NS16550_CLK         13000000
>>  #endif
>> +
>> +#if !defined(CONFIG_LPC32XX_HSUART) || defined(CONFIG_SPL_BUILD)
>> +#define CONFIG_CONS_INDEX              (CONFIG_SYS_LPC32XX_UART - 2)
>> +#else
>> +#define CONFIG_CONS_INDEX              CONFIG_SYS_LPC32XX_UART
>>  #endif
>>
>>  #define CONFIG_SYS_BAUDRATE_TABLE      \
>> diff --git a/configs/devkit3250_defconfig b/configs/devkit3250_defconfig
>> index 64a0fb0..0abb8e0 100644
>> --- a/configs/devkit3250_defconfig
>> +++ b/configs/devkit3250_defconfig
>> @@ -1,5 +1,6 @@
>>  CONFIG_ARM=y
>>  CONFIG_TARGET_DEVKIT3250=y
>> +CONFIG_DM_SERIAL=y
>>  CONFIG_DM_GPIO=y
>>  CONFIG_SPL=y
>>  # CONFIG_CMD_FPGA is not set
>> diff --git a/configs/work_92105_defconfig b/configs/work_92105_defconfig
>> index 1cad3a2..a5a108e 100644
>> --- a/configs/work_92105_defconfig
>> +++ b/configs/work_92105_defconfig
>> @@ -1,5 +1,6 @@
>>  CONFIG_ARM=y
>>  CONFIG_TARGET_WORK_92105=y
>> +CONFIG_DM_SERIAL=y
>>  CONFIG_DM_GPIO=y
>>  CONFIG_SPL=y
>>  # CONFIG_CMD_IMLS is not set
>> --
>> 2.1.4
>>
> 
> BTW you should be able to adjust it to work in SPL also.
>

That is done in the second patch.

Do you think it makes sense to squash them?

With best wishes,
Vladimir