[U-Boot] [PATCH v2 1/4] ns16550: Add function to drain serial output
Sometimes we want to be sure that the output FIFO has fully drained (e.g. because we are about to reset or the port or reset the board). Add a function for this.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: - Add function to drain the ns16550's FIFO
drivers/serial/ns16550.c | 11 +++++++++++ include/ns16550.h | 3 +++ 2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 0c23955..a082112 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -6,6 +6,7 @@
#include <config.h> #include <ns16550.h> +#include <common.h> #include <watchdog.h> #include <linux/types.h> #include <asm/io.h> @@ -115,4 +116,14 @@ int NS16550_tstc(NS16550_t com_port) return (serial_in(&com_port->lsr) & UART_LSR_DR) != 0; }
+/* Wait for the UART's output buffer and holding register to drain */ +void NS16550_drain(NS16550_t regs) +{ + u32 mask = UART_LSR_TEMT | UART_LSR_THRE; + + /* Wait for the UART to finish sending */ + while ((serial_in(®s->lsr) & mask) != mask) + udelay(100); +} + #endif /* CONFIG_NS16550_MIN_FUNCTIONS */ diff --git a/include/ns16550.h b/include/ns16550.h index e9d2eda..16b3828 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -166,4 +166,7 @@ void NS16550_init(NS16550_t com_port, int baud_divisor); void NS16550_putc(NS16550_t com_port, char c); char NS16550_getc(NS16550_t com_port); int NS16550_tstc(NS16550_t com_port); + +/* Wait for the UART's output buffer and holding register to drain */ +void NS16550_drain(NS16550_t regs); void NS16550_reinit(NS16550_t com_port, int baud_divisor);
At present we skip printf() if there is no hope of it making it to the console. But this check ignores CONFIG_PRE_CONSOLE_PUTC at present. Add a check for that also.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: - Allow printf() output to make it to pre-console putc()
common/console.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/common/console.c b/common/console.c index 1d9fd7f..38d7094 100644 --- a/common/console.c +++ b/common/console.c @@ -447,7 +447,7 @@ int vprintf(const char *fmt, va_list args) uint i; char printbuffer[CONFIG_SYS_PBSIZE];
-#ifndef CONFIG_PRE_CONSOLE_BUFFER +#if !defined(CONFIG_PRE_CONSOLE_BUFFER) && !defined(CONFIG_PRE_CONSOLE_PUTC) if (!gd->have_console) return 0; #endif
When there is not device tree file available to U-Boot, we panic. Implement board_pre_console_putc() so that this panic will be displayed on the serial console.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: - Tidy up casting in board_pre_console_putc() - Use 'const' for uart_reg_addr
arch/arm/cpu/armv7/tegra2/board.c | 61 +++++++++++++++++++++++++++++++++++++ 1 files changed, 61 insertions(+), 0 deletions(-)
diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c index 77a627d..0517d72 100644 --- a/arch/arm/cpu/armv7/tegra2/board.c +++ b/arch/arm/cpu/armv7/tegra2/board.c @@ -23,9 +23,11 @@
#include <common.h> #include <asm/io.h> +#include <ns16550.h> #include "ap20.h" #include <asm/arch/clock.h> #include <asm/arch/funcmux.h> +#include <asm/arch/gpio.h> #include <asm/arch/sys_proto.h> #include <asm/arch/tegra2.h> #include <asm/arch/pmc.h> @@ -37,6 +39,7 @@ enum { UARTA = 1 << 0, UARTB = 1 << 1, UARTD = 1 << 3, + UART_ALL = 0xf, UART_COUNT = 4, };
@@ -149,3 +152,61 @@ void enable_caches(void) dcache_enable(); } #endif + + +/* + * Possible UART locations: we ignore UARTC at 0x70006200 and UARTE at + * 0x70006400, since we don't have code to init them + */ +static const u32 uart_reg_addr[] = { + NV_PA_APB_UARTA_BASE, + NV_PA_APB_UARTB_BASE, + NV_PA_APB_UARTD_BASE, + 0 +}; + +#ifdef CONFIG_PRE_CONSOLE_PUTC +/* + * This is called when we have no console. About the only reason that this + * happen is if we don't have a valid fdt. So we don't know what kind of + * Tegra board we are. We blindly try to print a message every which way we + * know. + */ +void board_pre_console_putc(int ch) +{ + int uart_ids = UART_ALL; /* turn it all on! */ + const u32 *uart_addr; + int clock_freq, multiplier, baudrate, divisor; + + /* Try to enable all possible UARTs */ + setup_uarts(uart_ids); + + /* + * Seaboard has a UART switch on PI3. We might be a Seaboard, + * so flip it! + */ +#ifdef CONFIG_SPI_UART_SWITCH + gpio_direction_output(GPIO_PI3, 0); +#endif + + /* + * Now send the string out all the Tegra UARTs. We don't try all + * possible configurations, but this could be added if required. + */ + clock_freq = CONFIG_DEFAULT_NS16550_CLK; + multiplier = CONFIG_DEFAULT_NS16550_MULT; + baudrate = CONFIG_BAUDRATE; + divisor = (clock_freq + (baudrate * (multiplier / 2))) / + (multiplier * baudrate); + + for (uart_addr = uart_reg_addr; *uart_addr; uart_addr++) { + NS16550_t regs = (NS16550_t)*uart_addr; + + NS16550_init(regs, divisor); + NS16550_putc(regs, ch); + if (ch == '\n') + NS16550_putc(regs, '\r'); + NS16550_drain(regs); + } +} +#endif
Dear Simon Glass,
In message 1331325178-14634-3-git-send-email-sjg@chromium.org you wrote:
When there is not device tree file available to U-Boot, we panic. Implement board_pre_console_putc() so that this panic will be displayed on the serial console.
...
+void board_pre_console_putc(int ch) +{
...
- for (uart_addr = uart_reg_addr; *uart_addr; uart_addr++) {
NS16550_t regs = (NS16550_t)*uart_addr;NS16550_init(regs, divisor);NS16550_putc(regs, ch);if (ch == '\n')NS16550_putc(regs, '\r');NS16550_drain(regs);
Why is this needed for every output character?
Actually, why is it needed at all?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Sat, Mar 10, 2012 at 12:16 AM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message 1331325178-14634-3-git-send-email-sjg@chromium.org you wrote:
When there is not device tree file available to U-Boot, we panic. Implement board_pre_console_putc() so that this panic will be displayed on the serial console.
...
+void board_pre_console_putc(int ch) +{
...
- for (uart_addr = uart_reg_addr; *uart_addr; uart_addr++) {
- NS16550_t regs = (NS16550_t)*uart_addr;
- NS16550_init(regs, divisor);
- NS16550_putc(regs, ch);
- if (ch == '\n')
- NS16550_putc(regs, '\r');
- NS16550_drain(regs);
Why is this needed for every output character?
Actually, why is it needed at all?
Of course in this case the init could be done for each UART at the start of the function rather than in the loop, by looping through the UARTs twice.
More generally, I will explain both of these patches in this email.
The genesis of this was a requirement to print some sort of message when things go horribly wrong in early init. This could be in SPL code when we get an error loading U-Boot, or in board_init_f() when using CONFIG_OF_CONTROL and we find there is no device tree.
At present in the case of a missing fdt the board is just dead, there is no message and no indication of what the user should do to sort it out. The normal console code will not get set up, since U-Boot does not know which UART is the console. There is a panic() in fdtdec_check_fdt() to make sure that we don't blindly continue and do strange things in this case. But since there is no console, the panic() displays nothing.
My original idea was board_panic_no_console() which boards or SOCs could implement to print a message on all available UARTs or display, flash lights, etc. to indicate that something went badly wrong.
The initial patch for board_panic_no_console() was here:
http://lists.denx.de/pipermail/u-boot/2011-August/099619.html
After requests on the list for general purpose pre-console output function (the purpose of which I didn't necessary see) I changed this to a putc() mechanism. This means that we need to set up the UARTs each time it is called. We can't really add a flag to global data since this might be called before that is even set up. There might be a better way, but I'm not sure what it is.
For SPL I would like to be able use this same mechanism to call panic() when something goes wrong, and use the same mechanism to get a message to the user. Again, this avoids a bricked unit with no failure indication.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de All a hacker needs is a tight PUSHJ, a loose pair of UUOs, and a warm place to shift.
Dear Simon Glass,
In message CAPnjgZ1=KK9UNy2fyk9WJ-gcyYE5JgwqO_cT8igXjD-OFP_h6w@mail.gmail.com you wrote:
+void board_pre_console_putc(int ch) +{
...
for (uart_addr = uart_reg_addr; *uart_addr; uart_addr++) {NS16550_t regs = (NS16550_t)*uart_addr;NS16550_init(regs, divisor);NS16550_putc(regs, ch);if (ch == '\n')NS16550_putc(regs, '\r');NS16550_drain(regs);Why is this needed for every output character?
Actually, why is it needed at all?
Of course in this case the init could be done for each UART at the start of the function rather than in the loop, by looping through the UARTs twice.
Both the init() and the drain() should never be used in a character output loop.
After requests on the list for general purpose pre-console output function (the purpose of which I didn't necessary see) I changed this to a putc() mechanism. This means that we need to set up the UARTs each time it is called. We can't really add a flag to global data since this might be called before that is even set up. There might be a better way, but I'm not sure what it is.
For SPL I would like to be able use this same mechanism to call panic() when something goes wrong, and use the same mechanism to get a message to the user. Again, this avoids a bricked unit with no failure indication.
I understand your intention, but I disagree with the design, and with the implementation.
I think outputting data to all "potential" console ports is a really bad thing, as you cannot know how your users are using the hardware. They may have attached hardware to the UARTs, and the data you send to the port causes a mis-function of the device. I guess you don't add to your documentation a warning like: select one port as console, and leave allother ports unused, because we may send random date to all ports any time?
If you do not know which UART port to use, then the only consequence can be not to use any UART prt at all. Use a LED with blink codes or whatever your hardwar ehas, but do not mess with random ports.
I also cannot understand why you think you must init() and drain() the UART for each character printed - ok, the latter is probably a consequence of re-initializing the port for each character. Eventually this will be not needed once you get rid of the re-init.
Best regards,
Wolfgang Denk
+Graeme
Hi Wolfgang,
On Sat, Mar 10, 2012 at 12:08 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message CAPnjgZ1=KK9UNy2fyk9WJ-gcyYE5JgwqO_cT8igXjD-OFP_h6w@mail.gmail.com you wrote:
+void board_pre_console_putc(int ch) +{
...
- for (uart_addr = uart_reg_addr; *uart_addr; uart_addr++) {
- NS16550_t regs = (NS16550_t)*uart_addr;
- NS16550_init(regs, divisor);
- NS16550_putc(regs, ch);
- if (ch == '\n')
- NS16550_putc(regs, '\r');
- NS16550_drain(regs);
Why is this needed for every output character?
Actually, why is it needed at all?
Of course in this case the init could be done for each UART at the start of the function rather than in the loop, by looping through the UARTs twice.
Both the init() and the drain() should never be used in a character output loop.
Yes I agree, but I can't move them out with the API as it is. If we move back to a puts() type function then I could do this.
After requests on the list for general purpose pre-console output function (the purpose of which I didn't necessary see) I changed this to a putc() mechanism. This means that we need to set up the UARTs each time it is called. We can't really add a flag to global data since this might be called before that is even set up. There might be a better way, but I'm not sure what it is.
For SPL I would like to be able use this same mechanism to call panic() when something goes wrong, and use the same mechanism to get a message to the user. Again, this avoids a bricked unit with no failure indication.
I understand your intention, but I disagree with the design, and with the implementation.
It's not great, but let's work out something that is better.
I think outputting data to all "potential" console ports is a really bad thing, as you cannot know how your users are using the hardware. They may have attached hardware to the UARTs, and the data you send to the port causes a mis-function of the device. I guess you don't add to your documentation a warning like: select one port as console, and leave allother ports unused, because we may send random date to all ports any time?
Only on a fatal error. Unfortunately this idea of 'fatal error' was lost in the conversion from board_panic_no_console() to board_pre_console_putc(). I would be keen to move back to that original plan, so that the idea of a fatal error is captured. In a fatal error situation there is no prospect of the board working and the only hope is that we can alert the user.
If you do not know which UART port to use, then the only consequence can be not to use any UART prt at all. Use a LED with blink codes or whatever your hardwar ehas, but do not mess with random ports.
I agres with the sentiment and this is a very ugly corner case, but in practice people want to know what happened, not just be presented with a brick.
I also cannot understand why you think you must init() and drain() the UART for each character printed - ok, the latter is probably a consequence of re-initializing the port for each character. Eventually this will be not needed once you get rid of the re-init.
OK so how about moving to a puts()-type interface? Then I can remove this.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "Consistency requires you to be as ignorant today as you were a year ago." - Bernard Berenson
Dear Simon,
In message CAPnjgZ1GzD3KJeaA1LLdYPQsoOLWJPxmX=FHFeUthkdPrUYt2g@mail.gmail.com you wrote:
I think outputting data to all "potential" console ports is a really bad thing, as you cannot know how your users are using the hardware. They may have attached hardware to the UARTs, and the data you send to the port causes a mis-function of the device. =A0I guess you don't add to your documentation a warning like: select one port as console, and leave allother ports unused, because we may send random date to all ports any time?
Only on a fatal error. Unfortunately this idea of 'fatal error' was lost in the conversion from board_panic_no_console() to
This "fatal error" might become a fatal accident in some cases if you send random data to some port that controls some kind of machinery or whatever. Never ever do that!
board_pre_console_putc(). I would be keen to move back to that original plan, so that the idea of a fatal error is captured. In a fatal error situation there is no prospect of the board working and the only hope is that we can alert the user.
Agreed. But you must not do this by performing random actions on random (potentially otherwise used [by the end user]) interfaces.
If you do not know which UART port to use, then the only consequence can be not to use any UART prt at all. Use a LED with blink codes or whatever your hardwar ehas, but do not mess with random ports.
I agres with the sentiment and this is a very ugly corner case, but in practice people want to know what happened, not just be presented with a brick.
Yes, I understand this. But please try to find another way to signal failure. Do not perform random actions on a device - you could as well always output all data on all UART ports because with this codeyou cannot use the other ports for any serious purpose anyway.
I also cannot understand why you think you must init() and drain() the UART for each character printed - ok, the latter is probably a consequence of re-initializing the port for each character. Eventually this will be not needed once you get rid of the re-init.
OK so how about moving to a puts()-type interface? Then I can remove this.
Sorry, but I don't have enough context - pts() interface where or for what?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Sat, Mar 10, 2012 at 2:49 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon,
In message CAPnjgZ1GzD3KJeaA1LLdYPQsoOLWJPxmX=FHFeUthkdPrUYt2g@mail.gmail.com you wrote:
I think outputting data to all "potential" console ports is a really bad thing, as you cannot know how your users are using the hardware. They may have attached hardware to the UARTs, and the data you send to the port causes a mis-function of the device. =A0I guess you don't add to your documentation a warning like: select one port as console, and leave allother ports unused, because we may send random date to all ports any time?
Only on a fatal error. Unfortunately this idea of 'fatal error' was lost in the conversion from board_panic_no_console() to
This "fatal error" might become a fatal accident in some cases if you send random data to some port that controls some kind of machinery or whatever. Never ever do that!
Yes I see that. Scary.
board_pre_console_putc(). I would be keen to move back to that original plan, so that the idea of a fatal error is captured. In a fatal error situation there is no prospect of the board working and the only hope is that we can alert the user.
Agreed. But you must not do this by performing random actions on random (potentially otherwise used [by the end user]) interfaces.
OK I will come up with a patch along these lines. See below for thoughts.
If you do not know which UART port to use, then the only consequence can be not to use any UART prt at all. Use a LED with blink codes or whatever your hardwar ehas, but do not mess with random ports.
I agres with the sentiment and this is a very ugly corner case, but in practice people want to know what happened, not just be presented with a brick.
Yes, I understand this. But please try to find another way to signal failure. Do not perform random actions on a device - you could as well always output all data on all UART ports because with this codeyou cannot use the other ports for any serious purpose anyway.
OK so I think the first step is to move this function out of generic Tegra code and into board-specific code.
Presumably the board owner will know what is safe for that board. In many cases there is only one possible console UART. If there is an LED available then the board could flash that, but it isn't hugely useful. We already know the board is a brick - turning on a light to tell us that it is a brick doesn't help much. So I do want to provide a way to get a message out on boards where it is safe to do so.
In the case of the Tegra board there are four possible UARTs. I wonder whether we could do something like provide a Tegra panic function which is told which UARTs it can use.
Then the board can decide how to implement a board_pre_console_panic() function, which could be a weak function, perhaps by just hanging, perhaps by toggling a few GPIOs to flash LEDs, or perhaps by calling this Tegra panic function asking for a message on UARTs it knows are safe to use.
This would alleviate the concern that we might turn off someone's live support system by mistake.
I also cannot understand why you think you must init() and drain() the UART for each character printed - ok, the latter is probably a consequence of re-initializing the port for each character. Eventually this will be not needed once you get rid of the re-init.
OK so how about moving to a puts()-type interface? Then I can remove this.
Sorry, but I don't have enough context - pts() interface where or for what?
I just meant that this would be much easier if we had a panic function instead of this putc() stuff. We can (for example) flash lights indefinitely and never return.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Each honest calling, each walk of life, has its own elite, its own aristocracy based on excellence of performance. - James Bryant Conant
This is used to display panic messages before the console is active.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/configs/tegra2-common.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/include/configs/tegra2-common.h b/include/configs/tegra2-common.h index 837f859..368f3eb 100644 --- a/include/configs/tegra2-common.h +++ b/include/configs/tegra2-common.h @@ -68,11 +68,18 @@ */ #define V_NS16550_CLK 216000000 /* 216MHz (pllp_out0) */
+/* Default serial clock and multiplier */ +#define CONFIG_DEFAULT_NS16550_CLK V_NS16550_CLK +#define CONFIG_DEFAULT_NS16550_MULT 16 + #define CONFIG_SYS_NS16550 #define CONFIG_SYS_NS16550_SERIAL #define CONFIG_SYS_NS16550_REG_SIZE (-4) #define CONFIG_SYS_NS16550_CLK V_NS16550_CLK
+/* We use this for a warning message when no device tree is present */ +#define CONFIG_PRE_CONSOLE_PUTC + /* * select serial console configuration */
Simon,
-----Original Message----- From: Simon Glass [mailto:sjg@chromium.org] Sent: Friday, March 09, 2012 1:33 PM To: U-Boot Mailing List Cc: Tom Warren; Stephen Warren; Simon Glass Subject: [PATCH v2 1/4] ns16550: Add function to drain serial output
This patch series (v2, 1-4) works for me on my Seaboard - I get a message about 'No valid fdt found', etc.
So: Tested-by: Tom Warren twarren@nvidia.com Acked-by: Tom Warren twarren@nvidia.com
If no one objects, I'll apply this series, and Stephen's 'Makefile: fdt: Make the final build result be u-boot.bin' patch (which also works OK, BTW) to my previous pull request and resend to Albert.
Tom
Dear Tom Warren,
In message 5FBF8E85CA34454794F0F7ECBA79798F37971BC0CD@HQMAIL04.nvidia.com you wrote:
If no one objects, I'll apply this series, and Stephen's 'Makefile: fdt: Make the final build result be u-boot.bin' patch (which also works OK, BTW) to my previous pull request and resend to Albert.
I do object. This affects common code.
Best regards,
Wolfgang Denk
On 03/09/2012 01:32 PM, Simon Glass wrote:
Sometimes we want to be sure that the output FIFO has fully drained (e.g. because we are about to reset or the port or reset the board). Add a function for this.
Signed-off-by: Simon Glass sjg@chromium.org
The series,
Tested-by: Stephen Warren swarren@wwwdotorg.org
Tom,
I'm OK with you adding these patches to u-boot-tegra/master and sending a pull request for that. Just please put these patches before any that actually enable CONFIG_OF_CONTROL for any boards, so there's never a window of broken commits.
I think it'd be nice at this stage to also drop the commit that turns Ventana into a DT board, since re-using the Seaboard .dts file for Ventana isn't correct. However, IIRC that commit was added to WAR some build issue with Simon's patch series? If that's still a problem, then I'm OK with converting Ventana if we have to, but we'll need to be very careful that tegra-seaboard.dts doesn't add stuff that'll fail on, or damage, Ventana.
Hi,
On Fri, Mar 9, 2012 at 1:00 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 03/09/2012 01:32 PM, Simon Glass wrote:
Sometimes we want to be sure that the output FIFO has fully drained (e.g. because we are about to reset or the port or reset the board). Add a function for this.
Signed-off-by: Simon Glass sjg@chromium.org
The series,
Tested-by: Stephen Warren swarren@wwwdotorg.org
Tom,
I'm OK with you adding these patches to u-boot-tegra/master and sending a pull request for that. Just please put these patches before any that actually enable CONFIG_OF_CONTROL for any boards, so there's never a window of broken commits.
I think it'd be nice at this stage to also drop the commit that turns Ventana into a DT board, since re-using the Seaboard .dts file for Ventana isn't correct. However, IIRC that commit was added to WAR some build issue with Simon's patch series? If that's still a problem, then I'm OK with converting Ventana if we have to, but we'll need to be very careful that tegra-seaboard.dts doesn't add stuff that'll fail on, or damage, Ventana.
A few points:
1. Yes you can drop the Ventana commit and I agree it is a good idea. We should have a real Ventana .dts file I think. Ventana should built ok without a device tree now. The problem was in my clock.c patch.
2. I have held off responding to Stephen's patch on the ML to see what other say. My view is that it is controversial since it changes the so-far accepted meaning of u-boot.bin and the behaviour of the U-Boot Makefile. Plus it is not really necessary as a means of informing the user since we put the pre-console putc() for exactly this problem. So I would rather leave Stephen's patch out at until people have time to decide that I am wrong about it. We already have CONFIG_OF_EMBED to build the fdt into u-boot.bin. Grant Likely had big reservations about this feature - let's not bring it in by stealth.
Regards, Simon
Simon/Stephen,
-----Original Message----- From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass Sent: Friday, March 09, 2012 2:09 PM To: Stephen Warren Cc: U-Boot Mailing List; Tom Warren Subject: Re: [PATCH v2 1/4] ns16550: Add function to drain serial output
Hi,
On Fri, Mar 9, 2012 at 1:00 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 03/09/2012 01:32 PM, Simon Glass wrote:
Sometimes we want to be sure that the output FIFO has fully drained (e.g. because we are about to reset or the port or reset the board). Add a function for this.
Signed-off-by: Simon Glass sjg@chromium.org
The series,
Tested-by: Stephen Warren swarren@wwwdotorg.org
Tom,
I'm OK with you adding these patches to u-boot-tegra/master and sending a pull request for that. Just please put these patches before any that actually enable CONFIG_OF_CONTROL for any boards, so there's never a window of broken commits.
I think it'd be nice at this stage to also drop the commit that turns Ventana into a DT board, since re-using the Seaboard .dts file for Ventana isn't correct. However, IIRC that commit was added to WAR some build issue with Simon's patch series? If that's still a problem, then I'm OK with converting Ventana if we have to, but we'll need to be very careful that tegra-seaboard.dts doesn't add stuff that'll fail on, or damage, Ventana.
A few points:
- Yes you can drop the Ventana commit and I agree it is a good idea.
We should have a real Ventana .dts file I think. Ventana should built ok without a device tree now. The problem was in my clock.c patch.
- I have held off responding to Stephen's patch on the ML to see what other
say. My view is that it is controversial since it changes the so-far accepted meaning of u-boot.bin and the behaviour of the U-Boot Makefile. Plus it is not really necessary as a means of informing the user since we put the pre-console putc() for exactly this problem. So I would rather leave Stephen's patch out at until people have time to decide that I am wrong about it. We already have CONFIG_OF_EMBED to build the fdt into u-boot.bin. Grant Likely had big reservations about this feature - let's not bring it in by stealth.
I see now why it's so hard to find custodians for the U-Boot repos. :/
I've done as Stephen requested and re-ordered the commits so his Makefile fix comes before the config file changes to add CONFIG_OF_CONTROL to Seaboard and Ventana.
I've left the Ventana change that uses the Seaboard .dts file in, as I think it can easily be edited to use a Ventana-specific dts file just as soon as one of you finds one and submits a patch for it. For now, the Seaboard .dts just works.
I've pushed my latest changes to u-boot-tegra/master, and I'm going to let it percolate over the weekend. I'd have liked to get a pull request in to Albert today, before the ARM HEAD moves again, but I'm not going sweat to it since there doesn't seem to be a consensus on how to proceed here. IMO, I see no problem with putting these patches in as they stand, since I've tested them to work OK on Seaboard and Ventana, other (non-DT) Tegra2 boards build fine, and anything here that needs to change can always be edited with a future patch. But I'm going to head out early (it's my birthday today), and pick this up again next week.
Tom
Hi Stephen,
On Fri, Mar 9, 2012 at 1:23 PM, Tom Warren TWarren@nvidia.com wrote:
Simon/Stephen,
-----Original Message----- From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass Sent: Friday, March 09, 2012 2:09 PM To: Stephen Warren Cc: U-Boot Mailing List; Tom Warren Subject: Re: [PATCH v2 1/4] ns16550: Add function to drain serial output
Hi,
On Fri, Mar 9, 2012 at 1:00 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 03/09/2012 01:32 PM, Simon Glass wrote:
Sometimes we want to be sure that the output FIFO has fully drained (e.g. because we are about to reset or the port or reset the board). Add a function for this.
Signed-off-by: Simon Glass sjg@chromium.org
The series,
Tested-by: Stephen Warren swarren@wwwdotorg.org
Tom,
I'm OK with you adding these patches to u-boot-tegra/master and sending a pull request for that. Just please put these patches before any that actually enable CONFIG_OF_CONTROL for any boards, so there's never a window of broken commits.
I think it'd be nice at this stage to also drop the commit that turns Ventana into a DT board, since re-using the Seaboard .dts file for Ventana isn't correct. However, IIRC that commit was added to WAR some build issue with Simon's patch series? If that's still a problem, then I'm OK with converting Ventana if we have to, but we'll need to be very careful that tegra-seaboard.dts doesn't add stuff that'll fail on, or damage, Ventana.
A few points:
- Yes you can drop the Ventana commit and I agree it is a good idea.
We should have a real Ventana .dts file I think. Ventana should built ok without a device tree now. The problem was in my clock.c patch.
- I have held off responding to Stephen's patch on the ML to see what other
say. My view is that it is controversial since it changes the so-far accepted meaning of u-boot.bin and the behaviour of the U-Boot Makefile. Plus it is not really necessary as a means of informing the user since we put the pre-console putc() for exactly this problem. So I would rather leave Stephen's patch out at until people have time to decide that I am wrong about it. We already have CONFIG_OF_EMBED to build the fdt into u-boot.bin. Grant Likely had big reservations about this feature - let's not bring it in by stealth.
I see now why it's so hard to find custodians for the U-Boot repos. :/
I've done as Stephen requested and re-ordered the commits so his Makefile fix comes before the config file changes to add CONFIG_OF_CONTROL to Seaboard and Ventana.
I've left the Ventana change that uses the Seaboard .dts file in, as I think it can easily be edited to use a Ventana-specific dts file just as soon as one of you finds one and submits a patch for it. For now, the Seaboard .dts just works.
I've pushed my latest changes to u-boot-tegra/master, and I'm going to let it percolate over the weekend. I'd have liked to get a pull request in to Albert today, before the ARM HEAD moves again, but I'm not going sweat to it since there doesn't seem to be a consensus on how to proceed here. IMO, I see no problem with putting these patches in as they stand, since I've tested them to work OK on Seaboard and Ventana, other (non-DT) Tegra2 boards build fine, and anything here that needs to change can always be edited with a future patch. But I'm going to head out early (it's my birthday today), and pick this up again next week.
Happy Birthday Tom!
Stephen, what you do you about leaving out your Makefile patch for now? It is (I think) the only controversial part of this, and is not needed to make all this work...
Regards, Simon
Tom
-- nvpublic
Regards, Simon
On 03/09/2012 02:29 PM, Simon Glass wrote:
On Fri, Mar 9, 2012 at 1:23 PM, Tom Warren TWarren@nvidia.com wrote:
sjg@google.com wrote at Friday, March 09, 2012 2:09 PM:
...
- I have held off responding to Stephen's patch on the ML to see what other
say. My view is that it is controversial since it changes the so-far accepted meaning of u-boot.bin and the behaviour of the U-Boot Makefile. Plus it is not really necessary as a means of informing the user since we put the pre-console putc() for exactly this problem. So I would rather leave Stephen's patch out at until people have time to decide that I am wrong about it. We already have CONFIG_OF_EMBED to build the fdt into u-boot.bin. Grant Likely had big reservations about this feature - let's not bring it in by stealth.
I don't quite see how this is bringing the feature in by stealth; the exact same set of files (one with and without appended DTB already in place) is still available, just under filenames that are likely to cause less surprise to the user.
...
Stephen, what you do you about leaving out your Makefile patch for now? It is (I think) the only controversial part of this, and is not needed to make all this work...
Yes, I'm happy with that. The issue I had was the surprising result of U-Boot hanging without giving any clues why, and that's addressed by the messages that are now printed with your latest patches.
While I still like the patch to rename the files in the top-level Makefile, I do agree that including it in a pull request without more widespread discussion and agreement is not a good idea.
BTW, happy birthday Tom.
Dear Simon Glass,
In message 1331325178-14634-1-git-send-email-sjg@chromium.org you wrote:
Sometimes we want to be sure that the output FIFO has fully drained (e.g. because we are about to reset or the port or reset the board). Add a
Seems there is one unintentional "or".
function for this.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Add function to drain the ns16550's FIFO
drivers/serial/ns16550.c | 11 +++++++++++ include/ns16550.h | 3 +++ 2 files changed, 14 insertions(+), 0 deletions(-)
I dislike this. This is mostly dead code, enabled for eveybody where it just increases the memory footprint for no use.
Also I do not understand why it would be needed at all. We did not have such a requirement for any system before, so I feel you must be doing something wrong, or at least very exotic.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Sat, Mar 10, 2012 at 12:19 AM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message 1331325178-14634-1-git-send-email-sjg@chromium.org you wrote:
Sometimes we want to be sure that the output FIFO has fully drained (e.g. because we are about to reset or the port or reset the board). Add a
Seems there is one unintentional "or".
Yes, will fix.
function for this.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Add function to drain the ns16550's FIFO
drivers/serial/ns16550.c | 11 +++++++++++ include/ns16550.h | 3 +++ 2 files changed, 14 insertions(+), 0 deletions(-)
I dislike this. This is mostly dead code, enabled for eveybody where it just increases the memory footprint for no use.
Also I do not understand why it would be needed at all. We did not have such a requirement for any system before, so I feel you must be doing something wrong, or at least very exotic.
I put a fully explanation in the previous patch. Let me just add here that I see a 100ms delay in the reset code to allow serial output to flush, so there is at least in practice a need for this sort of thing. For most archs the extra code will be removed by the linker.
You may also recall a discussion about a platform where the SPI flash and console uart were muxed, and we had to flush the console uart before switching to SPI. I feel that draining the FIFO could/should be a useful function.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Extreme feminine beauty is always disturbing. -- Spock, "The Cloud Minders", stardate 5818.4
Dear Simon Glass,
In message CAPnjgZ3wprPDNUbe0fGbsequXJgQ_bwzkDEu6FeuR-8GG1ndQQ@mail.gmail.com you wrote:
Also I do not understand why it would be needed at all. =A0We did not have such a requirement for any system before, so I feel you must be doing something wrong, or at least very exotic.
I put a fully explanation in the previous patch. Let me just add here
Explanations in some other patch are not available when someone reads the commit message ...
that I see a 100ms delay in the reset code to allow serial output to flush, so there is at least in practice a need for this sort of thing.
Note that such a delay is hardware independent. You implement a special solution that works only for a minority of boards.
For most archs the extra code will be removed by the linker.
Ok, granted.
You may also recall a discussion about a platform where the SPI flash and console uart were muxed, and we had to flush the console uart before switching to SPI. I feel that draining the FIFO could/should be a useful function.
You quote an example of highly specific hardware, which I would not hesitate to call a broken design. I do not really fancy adding code to common files for such exceptional situations.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Sat, Mar 10, 2012 at 12:24 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message CAPnjgZ3wprPDNUbe0fGbsequXJgQ_bwzkDEu6FeuR-8GG1ndQQ@mail.gmail.com you wrote:
Also I do not understand why it would be needed at all. =A0We did not have such a requirement for any system before, so I feel you must be doing something wrong, or at least very exotic.
I put a fully explanation in the previous patch. Let me just add here
Explanations in some other patch are not available when someone reads the commit message ...
I did put a motivation in this patch at least:
Sometimes we want to be sure that the output FIFO has fully drained (e.g. because we are about to reset or the port or reset the board). Add a function for this.
that I see a 100ms delay in the reset code to allow serial output to flush, so there is at least in practice a need for this sort of thing.
Note that such a delay is hardware independent. You implement a special solution that works only for a minority of boards.
True, and we could just have the board panic function implement a delay, particularly if we move to the panic idea and away from putc().
For most archs the extra code will be removed by the linker.
Ok, granted.
You may also recall a discussion about a platform where the SPI flash and console uart were muxed, and we had to flush the console uart before switching to SPI. I feel that draining the FIFO could/should be a useful function.
You quote an example of highly specific hardware, which I would not hesitate to call a broken design. I do not really fancy adding code to common files for such exceptional situations.
It is certainly broken, yes.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Hiring experienced unix people is like a built-in filter against idiots. Hiring experienced NT people provides no such guarantee. -- Miguel Cruz in WgL96.349$CC.122704@typhoon2.ba-dsg.net
On Friday 09 March 2012 15:32:55 Simon Glass wrote:
Sometimes we want to be sure that the output FIFO has fully drained (e.g. because we are about to reset or the port or reset the board).
imo, we shouldn't be draining fifos before a reset in hardware that doesn't matter -- it's just a waste of time. if it were a CONFIG knob for people, i wouldn't complain too much, but i still wouldn't see the point ... you'd lose like maybe 2 or 3 chars at the end of a msg ? not that big of a deal. -mike
participants (5)
-
Mike Frysinger -
Simon Glass -
Stephen Warren -
Tom Warren -
Wolfgang Denk