Hi Marek,
On 26 May 2016 at 10:47, Marek Vasut marex@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@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@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@denx.de >>> Cc: Tom Rini trini@konsulko.com >>> Cc: Simon Glass sjg@chromium.org >>> Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com >>> Cc: Paul Burton paul.burton@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:
- 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.
- Add Kconfig option selected if ARCH_JZ47XX is selected and use that in the driver to add the extra UME bit.
- 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!
- Simon