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

[Simon Glass]

Hi Marek,

On 26 May 2016 at 11:35, Marek Vasut <marex at denx.de> wrote:
> On 05/26/2016 07:07 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 26 May 2016 at 10:53, Marek Vasut <marex at denx.de> wrote:
>>> On 05/26/2016 06:48 PM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On 26 May 2016 at 10:47, Marek Vasut <marex at denx.de> wrote:
>>>>> 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?
>>>>
>>>> Seems like speculative complexity to me - let's worry about it when it happens!
>>>
>>> Heh, ok. But if we add Kconfig stuff, we can as well go with option 2) ,
>>> which would only show up if JZ47XX is selected. Since this bits is
>>> specific to this lineup of SoCs , that makes sense in my head. What do
>>> you think ?
>>
>> SGTM.
>
> No, whichever I slice it as I plumb in this, I just don't like adding
> this complexity only because I might sometimes use UART on this platform
> in SPL. I can hack that support in, but in general let's just not do it
> afterall to make the driver a little simple(r).
>
> I just added DM/DT support into the JZ4780 and it supports serial too
> with minor tweaks to the ns16550 driver and it looks much less
> convoluted, so I will just opt for that.

Even better!

- Simon