[U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Simon Glass]

Hi Marek,

On 25 May 2016 at 16:35, Marek Vasut <marex at denx.de> wrote:
> On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
>>
>>
>> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
>>> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>>>>
>>>>
>>>> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
>>>>> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
>>>>> in order to work at all. Add this special case handling into the driver.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>> Cc: Tom Rini <trini at konsulko.com>
>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>>>>> Cc: Paul Burton <paul.burton at imgtec.com>
>>>>> ---
>>>>>  drivers/serial/ns16550.c | 8 ++++++++
>>>>>  1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>>>> index 30ba0aa..1323881 100644
>>>>> --- a/drivers/serial/ns16550.c
>>>>> +++ b/drivers/serial/ns16550.c
>>>>> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>>  #endif
>>>>>  #endif
>>>>>
>>>>> +#ifdef CONFIG_ARCH_JZ47XX
>>>>> +#undef UART_FCRVAL
>>>>> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set */
>>>>> +#define UART_FCR_UME              0x10
>>>>> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
>>>>> +               UART_FCR_TXSR | UART_FCR_UME)
>>>>> +#endif
>>>>
>>>> I think this could be added as DT property
>>>
>>> Not for SPL, which has 14 kiB size limit and it is itching to overflow.
>>> I am literally counting bytes in the SPL and removing slop from
>>> structures to make it fit, just barely. With the USB loader, I can
>>> brutalize the SPL into having extremely rudimentary UART support now
>>> (like printch() being the most advanced output mechanism, but you can
>>> only use it three times, otherwise the code won't fit and the board is
>>> eaten by demons) and this is where this patch comes into play.
>>>
>>> So yes, for full u-boot, this _should_ be part of DT. For SPL, please apply.
>>>
>>
>> ok, but wouldn't it be better to introduce an option like
>> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
>> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)
>
> I was undecided between this (like the IER) and adding new ifdef (like
> SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
> repugnant for sure.
>
>>>>> +
>>>>>  #ifndef CONFIG_SYS_NS16550_IER
>>>>>  #define CONFIG_SYS_NS16550_IER  0x00
>>>>>  #endif /* CONFIG_SYS_NS16550_IER */
>>>>>

That way seems better to me. You should be able to add your UME flag
as a Kconfig for this driver, in drivers/serial/Kconfig, which
defaults to 0. It would be good to keep out board-specific stuff from
this file, as you did with OMAP1510.

Regards,
Simon