serial: 16550: Add JZ47xx support
PATCH 1/1 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.
--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/serial/ns16550.c +8 -0
@@ -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 */
Signature · 1 line
2.7.0
PATCH 1/1 serial: 16550: Add JZ47xx support
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 17 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 10 lines
drivers/serial/ns16550.c +7
-0@@ -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
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */
Signature · 9 lines
- Daniel -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: OpenPGP digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160526/54e01771/attachment.sig>
PATCH 1/1 serial: 16550: Add JZ47xx support
Quoted 23 lines · Daniel Schwierzeck
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif
Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 6 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 serial: 16550: Add JZ47xx support
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 42 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 2 lines
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 ;)
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */
Signature · 9 lines
- Daniel -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: OpenPGP digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160526/9924aab7/attachment.sig>
PATCH 1/1 serial: 16550: Add JZ47xx support
Quoted 45 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 42 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 6 lines
Quoted 2 lines
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_UMEinstead of using the SoC-specificCONFIG_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.
Quoted 9 lines
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 serial: 16550: Add JZ47xx support
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#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
PATCH 1/1 serial: 16550: Add JZ47xx support
Quoted 66 lines · Simon Glass
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */
Quoted 4 lines
That way seems better to me. You should be able to add your UME flag as a
Kconfigfor this driver, indrivers/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 ?
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 serial: 16550: Add JZ47xx support
Hi Marek,
Quoted 87 lines · Marek Vasut
Quoted 66 lines · Simon Glass
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */ Quoted 4 lines
That way seems better to me. You should be able to add your UME flag as a
Kconfigfor this driver, indrivers/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_FCRoverride viaKconfig. 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) AddKconfigoption forCONFIG_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) AddKconfigoption selected ifARCH_JZ47XXis selected and use that in the driver to add the extra UME bit.
3) DefineCONFIG_SYS_NS16550_FCRininclude/configs/board.husing the macros fromns16550.h. This makes it obvious which bits are set.
Quoted 2 lines
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
PATCH 1/1 serial: 16550: Add JZ47xx support
Quoted 90 lines · Simon Glass
Hi Marek,
Quoted 87 lines · Marek Vasut
Quoted 66 lines · Simon Glass
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */ Quoted 4 lines
That way seems better to me. You should be able to add your UME flag as a
Kconfigfor this driver, indrivers/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_FCRoverride viaKconfig. 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) AddKconfigoption forCONFIG_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) AddKconfigoption selected ifARCH_JZ47XXis selected and use that in the driver to add the extra UME bit.
3) DefineCONFIG_SYS_NS16550_FCRininclude/configs/board.husing the macros fromns16550.h. This makes it obvious which bits are set.
Quoted 6 lines
Quoted 2 lines
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 is0x10. 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?
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 serial: 16550: Add JZ47xx support
Hi Marek,
Quoted 94 lines · Marek Vasut
Quoted 93 lines · Simon Glass
Hi Marek,
Quoted 89 lines · Marek Vasut
Quoted 66 lines · Simon Glass
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */ Quoted 4 lines
That way seems better to me. You should be able to add your UME flag as a
Kconfigfor this driver, indrivers/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_FCRoverride viaKconfig. 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) AddKconfigoption forCONFIG_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) AddKconfigoption selected ifARCH_JZ47XXis selected and use that in the driver to add the extra UME bit.
3) DefineCONFIG_SYS_NS16550_FCRininclude/configs/board.husing the macros fromns16550.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 ?
Quoted 6 lines
Quoted 3 lines
How about making your feature an option, like
CONFIG_SYS_NS16550_UEN, which defaults to 0, but in your case is0x10. 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
PATCH 1/1 serial: 16550: Add JZ47xx support
Quoted 97 lines · Simon Glass
Hi Marek,
Quoted 94 lines · Marek Vasut
Quoted 93 lines · Simon Glass
Hi Marek,
Quoted 89 lines · Marek Vasut
Quoted 66 lines · Simon Glass
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */ Quoted 4 lines
That way seems better to me. You should be able to add your UME flag as a
Kconfigfor this driver, indrivers/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_FCRoverride viaKconfig. 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) AddKconfigoption forCONFIG_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) AddKconfigoption selected ifARCH_JZ47XXis selected and use that in the driver to add the extra UME bit.
3) DefineCONFIG_SYS_NS16550_FCRininclude/configs/board.husing the macros fromns16550.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 ?
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
How about making your feature an option, like
CONFIG_SYS_NS16550_UEN, which defaults to 0, but in your case is0x10. 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 ?
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 serial: 16550: Add JZ47xx support
Hi Marek,
Quoted 105 lines · Marek Vasut
Quoted 104 lines · Simon Glass
Hi Marek,
Quoted 100 lines · Marek Vasut
Quoted 90 lines · Simon Glass
Hi Marek,
Quoted 87 lines · Marek Vasut
Quoted 66 lines · Simon Glass
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */ Quoted 4 lines
That way seems better to me. You should be able to add your UME flag as a
Kconfigfor this driver, indrivers/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_FCRoverride viaKconfig. 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) AddKconfigoption forCONFIG_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) AddKconfigoption selected ifARCH_JZ47XXis selected and use that in the driver to add the extra UME bit.
3) DefineCONFIG_SYS_NS16550_FCRininclude/configs/board.husing the macros fromns16550.h. This makes it obvious which bits are set.Quoted 6 lines
Quoted 2 lines
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 is0x10. 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?
Quoted 6 lines
Quoted 1 line
Seems like speculative complexity to me - let's worry about it when it happens!
Heh, ok. But if we add
Kconfigstuff, 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
PATCH 1/1 serial: 16550: Add JZ47xx support
Quoted 108 lines · Simon Glass
Hi Marek,
Quoted 105 lines · Marek Vasut
Quoted 104 lines · Simon Glass
Hi Marek,
Quoted 100 lines · Marek Vasut
Quoted 90 lines · Simon Glass
Hi Marek,
Quoted 87 lines · Marek Vasut
Quoted 66 lines · Simon Glass
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */ Quoted 4 lines
That way seems better to me. You should be able to add your UME flag as a
Kconfigfor this driver, indrivers/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_FCRoverride viaKconfig. 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) AddKconfigoption forCONFIG_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) AddKconfigoption selected ifARCH_JZ47XXis selected and use that in the driver to add the extra UME bit.
3) DefineCONFIG_SYS_NS16550_FCRininclude/configs/board.husing the macros fromns16550.h. This makes it obvious which bits are set.Quoted 6 lines
Quoted 2 lines
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 is0x10. 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?
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Seems like speculative complexity to me - let's worry about it when it happens!
Heh, ok. But if we add
Kconfigstuff, 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.
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 serial: 16550: Add JZ47xx support
Hi Marek,
Quoted 118 lines · Marek Vasut
Quoted 108 lines · Simon Glass
Hi Marek,
Quoted 105 lines · Marek Vasut
Quoted 104 lines · Simon Glass
Hi Marek,
Quoted 100 lines · Marek Vasut
Quoted 90 lines · Simon Glass
Hi Marek,
Quoted 87 lines · Marek Vasut
Quoted 66 lines · Simon Glass
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>
Cc: Tom Rini <[email protected]>
Cc: Simon Glass <[email protected]>
Cc: Daniel Schwierzeck <[email protected]>
Cc: Paul Burton <[email protected]>
---
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
#endifQuoted 9 lines
Quoted 7 lines
+#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)
+#endifI 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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
+
#ifndef CONFIG_SYS_NS16550_IER
#define CONFIG_SYS_NS16550_IER 0x00
#endif /* CONFIG_SYS_NS16550_IER */Quoted 4 lines
That way seems better to me. You should be able to add your UME flag as a
Kconfigfor this driver, indrivers/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_FCRoverride viaKconfig. 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) AddKconfigoption forCONFIG_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) AddKconfigoption selected ifARCH_JZ47XXis selected and use that in the driver to add the extra UME bit.
3) DefineCONFIG_SYS_NS16550_FCRininclude/configs/board.husing the macros fromns16550.h. This makes it obvious which bits are set.Quoted 6 lines
Quoted 2 lines
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 is0x10. 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?
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Seems like speculative complexity to me - let's worry about it when it happens!
Heh, ok. But if we add
Kconfigstuff, 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.
Quoted 8 lines
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
PATCH 1/1 serial: 16550: Add JZ47xx support
In reply to Simon GlassQuoted 97 lines · Simon Glass
Hi Marek,
Quoted 94 lines · Marek Vasut
Quoted 93 lines · Simon Glass
Hi Marek,
Quoted 89 lines · Marek Vasut
Quoted 66 lines · Simon Glass
Hi Marek,
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Daniel Schwierzeck
Am 26.05.2016 um 00:21 schrieb Marek Vasut:
Quoted 44 lines · Daniel Schwierzeck
Quoted 23 lines
Am 25.05.2016 um 02:19 schrieb Marek Vasut:
Quoted 20 lines
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 <[email protected]>Cc Tom Rini <[email protected]>Cc Simon Glass <[email protected]>Cc Daniel Schwierzeck <[email protected]>Cc Paul Burton <[email protected]>--- drivers/serial/ns16550.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/serial/ns16550.c +0
-0@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;#endif #endif Quoted 9 lines
drivers/serial/ns16550.c DECLARE_GLOBAL_DATA_PTR; · line 59
Quoted 7 lines
drivers/serial/ns16550.c +7
-0+#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.
Quoted 13 lines
Quoted 3 lines
ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UMEinstead of using the SoC-specificCONFIG_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.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/serial/ns16550.c +1
-0+#ifndef CONFIG_SYS_NS16550_IER #define CONFIG_SYS_NS16550_IER 0x00#endif /* CONFIG_SYS_NS16550_IER */ Quoted 4 lines
That way seems better to me. You should be able to add your UME flag as a
Kconfigfor this driver, indrivers/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_FCRoverride viaKconfig. 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) AddKconfigoption forCONFIG_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) AddKconfigoption selected ifARCH_JZ47XXis selected and use that in the driver to add the extra UME bit.
3) DefineCONFIG_SYS_NS16550_FCRininclude/configs/board.husing the macros fromns16550.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 ?
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
How about making your feature an option, like
CONFIG_SYS_NS16550_UEN, which defaults to 0, but in your case is0x10. 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.
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 serial: 16550: Add JZ47xx support
In reply to Marek VasutQuoted 9 lines · Marek Vasut
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
PATCH 1/1 serial: 16550: Add JZ47xx support
Quoted 9 lines · Paul Burton
Quoted 8 lines · Marek Vasut
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.
Quoted 3 lines
Quoted 1 line
So yes, for full u-boot, this _should_ be part of DT. For SPL, please apply.
Hi Marek,
Hi!
Quoted 4 lines
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 ?
Quoted 5 lines
https://github.com/MIPS/CI20_u-boot
Thanks,
Paul
[1] http://www.catb.org/esr/structure-packing/
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 serial: 16550: Add JZ47xx support
Quoted 12 lines · Marek Vasut
Quoted 4 lines
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.
Quoted 8 lines
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 :)
Quoted 2 lines
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.
Quoted 1 line
btw2. can I get the ci40 anywhere or is that just unobtainium board ?
I'll email you separately about that.
Thanks,
Paul
PATCH 1/1 serial: 16550: Add JZ47xx support
Quoted 22 lines · Paul Burton
Quoted 12 lines · Marek Vasut
Quoted 4 lines
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.
Quoted 8 lines
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.
Quoted 5 lines
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.
Quoted 8 lines
Quoted 2 lines
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 ;-)
Quoted 7 lines
Quoted 1 line
btw2. can I get the ci40 anywhere or is that just unobtainium board ?
I'll email you separately about that.
Thanks,
Paul
Signature · 2 lines
Best regards, Marek Vasut