[U-Boot] [PATCH] serial: 16550: Add JZ47xx support
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 + #ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00 #endif /* CONFIG_SYS_NS16550_IER */
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
#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00 #endif /* CONFIG_SYS_NS16550_IER */
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.
#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00 #endif /* CONFIG_SYS_NS16550_IER */
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 ;)
#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00 #endif /* CONFIG_SYS_NS16550_IER */
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 */
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.
Regards, Simon
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 ?
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.
Regards, Simon
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?
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
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:
- 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!
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 ?
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:
- 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!
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.
- Simon
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:
- 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!
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.
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
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:
- 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!
Maybe this is not even worth the effort and I should just ignore serial support in SPL and only go with serial support in full u-boot, where I can configure the FCR from DT. Then we avoid all this complex stuff.
On Thu, May 26, 2016 at 12:21:23AM +0200, Marek Vasut wrote:
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.
Hi Marek,
Interesting :) May I ask which platform/SoC you're working with? Have you seen the (unfortunately currently out of tree) port we did for the JZ4780-based Ci20? I recall the pain of squeezing SPL down to a small enough size well...
https://github.com/MIPS/CI20_u-boot
Thanks, Paul
On 05/26/2016 01:46 PM, Paul Burton wrote:
On Thu, May 26, 2016 at 12:21:23AM +0200, Marek Vasut wrote:
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.
Hi Marek,
Hi!
Interesting :) May I ask which platform/SoC you're working with? Have you seen the (unfortunately currently out of tree) port we did for the JZ4780-based Ci20? I recall the pain of squeezing SPL down to a small enough size well...
I bought the CI20, yeah. I started with that tree and am now getting it into mainline shape. I actually managed to optimize the SPL, so it does not overflow. One of the things which helped a lot was removing slop[1] from the structures and using appropriate data types in them (like don't use u32 to store an 8bit number). I also tinified the MMC stack, so that it won't pull in too much useless code.
At this point, I have a u-boot running from SD card and the SD driver in full U-Boot is already probing from DT and using DM. I sent Daniel the updated patches, but they are still work in progress. Nonetheless, if you want me forward that to you as well, let me know.
I plan to get this CI20 submitted at some point this or next week, so it can land in 2016.07 (I hope!) and then debian and fedora-mips (and other distros) can pick it up and package proper. That's how I see it.
btw. what is the linux support status of the ci20 ? Is anyone working on that? I saw the DTS in Linux, but it's pretty bare-bones.
btw2. can I get the ci40 anywhere or is that just unobtainium board ?
https://github.com/MIPS/CI20_u-boot
Thanks, Paul
[1] http://www.catb.org/esr/structure-packing/
On Thu, May 26, 2016 at 02:05:07PM +0200, Marek Vasut wrote:
Interesting :) May I ask which platform/SoC you're working with? Have you seen the (unfortunately currently out of tree) port we did for the JZ4780-based Ci20? I recall the pain of squeezing SPL down to a small enough size well...
I bought the CI20, yeah. I started with that tree and am now getting it into mainline shape. I actually managed to optimize the SPL, so it does not overflow. One of the things which helped a lot was removing slop[1] from the structures and using appropriate data types in them (like don't use u32 to store an 8bit number). I also tinified the MMC stack, so that it won't pull in too much useless code.
At this point, I have a u-boot running from SD card and the SD driver in full U-Boot is already probing from DT and using DM. I sent Daniel the updated patches, but they are still work in progress. Nonetheless, if you want me forward that to you as well, let me know.
I plan to get this CI20 submitted at some point this or next week, so it can land in 2016.07 (I hope!) and then debian and fedora-mips (and other distros) can pick it up and package proper. That's how I see it.
That sounds great :) I'd be interested in seeing them but probably won't have any time to look at them for a few weeks anyway (busy with other work things & then getting married & going off on honeymoon!). So hopefully by the time I'd get round to having a look they'll be in mainline :)
btw. what is the linux support status of the ci20 ? Is anyone working on that? I saw the DTS in Linux, but it's pretty bare-bones.
Yeah the support in mainline Linux is a bit basic at the moment. The biggest holdup is pinctrl support which I need to do some work on - the DT bindings are more complicated than they need to be, and the driver should probably be made more generic to cover other Ingenic SoCs before it goes into mainline.
btw2. can I get the ci40 anywhere or is that just unobtainium board ?
I'll email you separately about that.
Thanks, Paul
On 05/26/2016 03:55 PM, Paul Burton wrote:
On Thu, May 26, 2016 at 02:05:07PM +0200, Marek Vasut wrote:
Interesting :) May I ask which platform/SoC you're working with? Have you seen the (unfortunately currently out of tree) port we did for the JZ4780-based Ci20? I recall the pain of squeezing SPL down to a small enough size well...
I bought the CI20, yeah. I started with that tree and am now getting it into mainline shape. I actually managed to optimize the SPL, so it does not overflow. One of the things which helped a lot was removing slop[1] from the structures and using appropriate data types in them (like don't use u32 to store an 8bit number). I also tinified the MMC stack, so that it won't pull in too much useless code.
At this point, I have a u-boot running from SD card and the SD driver in full U-Boot is already probing from DT and using DM. I sent Daniel the updated patches, but they are still work in progress. Nonetheless, if you want me forward that to you as well, let me know.
I plan to get this CI20 submitted at some point this or next week, so it can land in 2016.07 (I hope!) and then debian and fedora-mips (and other distros) can pick it up and package proper. That's how I see it.
That sounds great :) I'd be interested in seeing them but probably won't have any time to look at them for a few weeks anyway (busy with other work things & then getting married & going off on honeymoon!). So hopefully by the time I'd get round to having a look they'll be in mainline :)
Congratulations ! I sent you a separate mail and I'm going to submit some of the stuff now too.
btw. what is the linux support status of the ci20 ? Is anyone working on that? I saw the DTS in Linux, but it's pretty bare-bones.
Yeah the support in mainline Linux is a bit basic at the moment. The biggest holdup is pinctrl support which I need to do some work on - the DT bindings are more complicated than they need to be, and the driver should probably be made more generic to cover other Ingenic SoCs before it goes into mainline.
I wonder if I should drill into that now ;-)
btw2. can I get the ci40 anywhere or is that just unobtainium board ?
I'll email you separately about that.
Thanks, Paul
participants (4)
-
Daniel Schwierzeck -
Marek Vasut -
Paul Burton -
Simon Glass