From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Thu, 26 May 2016 18:53:09 +0200 Subject: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support In-Reply-To: References: <1464135571-7113-1-git-send-email-marex@denx.de> <5746247F.9010600@gmail.com> <57462563.4040606@denx.de> <574627C3.8060906@gmail.com> <574628B0.6050708@denx.de> <574725B2.3080207@denx.de> <57472892.7010605@denx.de> Message-ID: <574729F5.3070206@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 05/26/2016 06:48 PM, Simon Glass wrote: > Hi Marek, > > On 26 May 2016 at 10:47, Marek Vasut wrote: >> On 05/26/2016 06:44 PM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 26 May 2016 at 10:34, Marek Vasut wrote: >>>> On 05/26/2016 03:29 PM, Simon Glass wrote: >>>>> Hi Marek, >>>>> >>>>> On 25 May 2016 at 16:35, Marek Vasut 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 >>>>>>>>>> Cc: Tom Rini >>>>>>>>>> Cc: Simon Glass >>>>>>>>>> Cc: Daniel Schwierzeck >>>>>>>>>> Cc: Paul Burton >>>>>>>>>> --- >>>>>>>>>> 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 ? -- Best regards, Marek Vasut