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

[Marek Vasut]

On 05/26/2016 03:29 PM, Simon Glass wrote:
> 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.

I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
via Kconfig. The extra bits should be set via DT props
unless there is some really good reason why that cannot be done
(like size limitation in SPL or DT not available).

I tried these approaches:
1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
   set specific byte value. This makes it hard to figure out
   which bits are set in the FCR just by looking at the value.
2) Add Kconfig option selected if ARCH_JZ47XX is selected and
   use that in the driver to add the extra UME bit.
3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
   using the macros from ns16550.h . This makes it obvious
   which bits are set.

I am undecided between 2 and 3, but inclined to go with 3.
What do you think ?

-- 
Best regards,
Marek Vasut