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

[Marek Vasut]

On 05/26/2016 06:44 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 26 May 2016 at 10:34, Marek Vasut <marex at denx.de> wrote:
>> 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 ?
> 
> How about making your feature an option, like CONFIG_SYS_NS16550_UEN,
> which defaults to 0, but in your case is 0x10. You can OR it into the
> value.

That's quite close to 1) , but then what if someone comes and wants to
remove some bits from the FCR instead of adding some?

-- 
Best regards,
Marek Vasut