Hi Simon,
On 19.12.2015 22:30, Simon Glass wrote:
> Hi Vladimir,
>
> On 12 December 2015 at 17:48, Vladimir Zapolskiy <v...@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 <v...@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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
