Hi Marek,
On 26 May 2016 at 11:35, Marek Vasut marex@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@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@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: > 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