Hi Marek, On 26 May 2016 at 10:47, Marek Vasut <ma...@denx.de> wrote: > On 05/26/2016 06:44 PM, Simon Glass wrote: >> Hi Marek, >> >> On 26 May 2016 at 10:34, Marek Vasut <ma...@denx.de> wrote: >>> On 05/26/2016 03:29 PM, Simon Glass wrote: >>>> Hi Marek, >>>> >>>> On 25 May 2016 at 16:35, Marek Vasut <ma...@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 <ma...@denx.de> >>>>>>>>> Cc: Tom Rini <tr...@konsulko.com> >>>>>>>>> Cc: Simon Glass <s...@chromium.org> >>>>>>>>> Cc: Daniel Schwierzeck <daniel.schwierz...@gmail.com> >>>>>>>>> Cc: Paul Burton <paul.bur...@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! - Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
