Re: [PATCH v2] bsps: Add BSP_FLUSH_KERNEL_CHAR_OUTPUT BSP option
On 21/3/2024 5:39 pm, Sebastian Huber wrote:> On 21.03.24 00:28, Chris Johns wrote: >> On 21/3/2024 2:11 am, Sebastian Huber wrote: >>> On 19.03.24 18:44, Chris Johns wrote: On 20/3/2024 2:03 am, Sebastian Huber wrote: > On 19.03.24 14:50, Kinsey Moore wrote: >> The xilinx-zynqmp-rpu bsp_reset() is modified, but not included in the >> spec >> file for the new option. Its family differs from the arm/xilinx-zynqmp >> BSP >> family with a -rpu suffix. > Yes, but this BSP is quite new. I would prefer to let it not flush > anything by > default to carry out a reset. > >> I'd be fine with this being enabled for the AArch64 BSPs as well, but I >> imagine that's better as a separate patch. > Why should it be enabled by default? The arm/xilinx-zynq and > arm/xilinx-zynqmp > BSPs were the only ones doing an UART flush in the general termination > procedure. It probably was done to address a specific use case (maybe test > runs). Is the issue the flush is before an infinite loop which means the UART FIFO should drain? >> >> What is the issue you are wanting to solve removing the flush? > > The bsp_reset() function should reset the system and do nothing more. Doing > additional things like flushing an UART device may not make sense for all > applications. Some applications may not use the UART device, so it may not be > initialized and powered off. Some applications may use it with an > application-specific protocol which doesn't like the additional four '\r' > during > reset. Doing a UART flush takes some time and some applications may prefer a > fast reset time. The bsp_reset() is the wrong place to do add specific cleanup > functions. Applications can customize the termination procedure with their own > fatal error extension, destructors, and exit handlers. This makes sense however the console needs this. Removing the code as my reading of this changes does breaks those systems where the console is in use and relies on the UART being flushed. A solution that addresses the console side of things would be more acceptable. You present some valid cases for making a change but a user should be able to enter reset in a shell and see all output or invoke a fatal error and expect to see the full error message. Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2] bsps: Add BSP_FLUSH_KERNEL_CHAR_OUTPUT BSP option
On 21.03.24 00:28, Chris Johns wrote: On 21/3/2024 2:11 am, Sebastian Huber wrote: On 19.03.24 18:44, Chris Johns wrote: On 20/3/2024 2:03 am, Sebastian Huber wrote: On 19.03.24 14:50, Kinsey Moore wrote: The xilinx-zynqmp-rpu bsp_reset() is modified, but not included in the spec file for the new option. Its family differs from the arm/xilinx-zynqmp BSP family with a -rpu suffix. Yes, but this BSP is quite new. I would prefer to let it not flush anything by default to carry out a reset. I'd be fine with this being enabled for the AArch64 BSPs as well, but I imagine that's better as a separate patch. Why should it be enabled by default? The arm/xilinx-zynq and arm/xilinx-zynqmp BSPs were the only ones doing an UART flush in the general termination procedure. It probably was done to address a specific use case (maybe test runs). Is the issue the flush is before an infinite loop which means the UART FIFO should drain? What is the issue you are wanting to solve removing the flush? The bsp_reset() function should reset the system and do nothing more. Doing additional things like flushing an UART device may not make sense for all applications. Some applications may not use the UART device, so it may not be initialized and powered off. Some applications may use it with an application-specific protocol which doesn't like the additional four '\r' during reset. Doing a UART flush takes some time and some applications may prefer a fast reset time. The bsp_reset() is the wrong place to do add specific cleanup functions. Applications can customize the termination procedure with their own fatal error extension, destructors, and exit handlers. -- embedded brains GmbH & Co. KG Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/ ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2] bsps: Add BSP_FLUSH_KERNEL_CHAR_OUTPUT BSP option
On 21/3/2024 2:11 am, Sebastian Huber wrote: > On 19.03.24 18:44, Chris Johns wrote: >> On 20/3/2024 2:03 am, Sebastian Huber wrote: >>> On 19.03.24 14:50, Kinsey Moore wrote: The xilinx-zynqmp-rpu bsp_reset() is modified, but not included in the spec file for the new option. Its family differs from the arm/xilinx-zynqmp BSP family with a -rpu suffix. >>> Yes, but this BSP is quite new. I would prefer to let it not flush anything >>> by >>> default to carry out a reset. >>> I'd be fine with this being enabled for the AArch64 BSPs as well, but I imagine that's better as a separate patch. >>> Why should it be enabled by default? The arm/xilinx-zynq and >>> arm/xilinx-zynqmp >>> BSPs were the only ones doing an UART flush in the general termination >>> procedure. It probably was done to address a specific use case (maybe test >>> runs). >> Is the issue the flush is before an infinite loop which means the UART FIFO >> should drain? What is the issue you are wanting to solve removing the flush? Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2] bsps: Add BSP_FLUSH_KERNEL_CHAR_OUTPUT BSP option
On 19.03.24 18:44, Chris Johns wrote: On 20/3/2024 2:03 am, Sebastian Huber wrote: On 19.03.24 14:50, Kinsey Moore wrote: The xilinx-zynqmp-rpu bsp_reset() is modified, but not included in the spec file for the new option. Its family differs from the arm/xilinx-zynqmp BSP family with a -rpu suffix. Yes, but this BSP is quite new. I would prefer to let it not flush anything by default to carry out a reset. I'd be fine with this being enabled for the AArch64 BSPs as well, but I imagine that's better as a separate patch. Why should it be enabled by default? The arm/xilinx-zynq and arm/xilinx-zynqmp BSPs were the only ones doing an UART flush in the general termination procedure. It probably was done to address a specific use case (maybe test runs). Is the issue the flush is before an infinite loop which means the UART FIFO should drain? I don't really like the new bsp_flush_kernel_char_output() function. Another approach could be an API change of /** * @ingroup RTEMSAPIKernelCharIO * * @brief Polled character output functions shall have this type. */ typedef void ( *BSP_output_char_function_type )( char ); to something like this typedef int ( *BSP_output_char_function_type )( int action ); If action in {0, ..., 255}, then print out the character. If 0x100 is set, then flush the output device. If 0x200 is set, then do Y... The return value could be used to give a status indication. This could then be use for example by test runs, to flush the test output after the end of the test. This also requires a code change so is a flush function that bad an option? You can change the character output handler since this is only a global variable (BSP_output_char). So, this bsp_flush_kernel_char_output() may not flush the device used by BSP_output_char. Doing a flush through the output handler lets you do the flush for the currently used device. If we change the function type to typedef int ( *BSP_output_char_function_type )( int action ); then we can add more features later if needed. Another feature which could be useful is: Output char immediately if possible and return 0, otherwise do nothing and return -1. This can be used to implement a full-duplex transfer in polling mode for the kernel I/O stream. -- embedded brains GmbH & Co. KG Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/ ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2] bsps: Add BSP_FLUSH_KERNEL_CHAR_OUTPUT BSP option
On 19/3/2024 7:39 pm, Sebastian Huber wrote: > Make the kernel I/O output character device flushing configurable. The > bsp_reset() function should reset the unit and do nothing else. This changes existing behaviour. RTEMS is poor at cleanly handling the console output on reset. Working with the powerpc/mvme2700 and EPICS is hard because the output is lost on reset and assert and crash output is corrupted. As an OS I think we should be heading other way and if you want just a reset you provide it. Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2] bsps: Add BSP_FLUSH_KERNEL_CHAR_OUTPUT BSP option
On 20/3/2024 2:03 am, Sebastian Huber wrote: > On 19.03.24 14:50, Kinsey Moore wrote: >> The xilinx-zynqmp-rpu bsp_reset() is modified, but not included in the spec >> file for the new option. Its family differs from the arm/xilinx-zynqmp BSP >> family with a -rpu suffix. > > Yes, but this BSP is quite new. I would prefer to let it not flush anything by > default to carry out a reset. > >> I'd be fine with this being enabled for the AArch64 BSPs as well, but I >> imagine that's better as a separate patch. > > Why should it be enabled by default? The arm/xilinx-zynq and arm/xilinx-zynqmp > BSPs were the only ones doing an UART flush in the general termination > procedure. It probably was done to address a specific use case (maybe test > runs). Is the issue the flush is before an infinite loop which means the UART FIFO should drain? > I don't really like the new bsp_flush_kernel_char_output() function. Another > approach could be an API change of > > /** > * @ingroup RTEMSAPIKernelCharIO > * > * @brief Polled character output functions shall have this type. > */ > typedef void ( *BSP_output_char_function_type )( char ); > > to something like this > > typedef int ( *BSP_output_char_function_type )( int action ); > > If action in {0, ..., 255}, then print out the character. If 0x100 is set, > then > flush the output device. If 0x200 is set, then do Y... The return value could > be > used to give a status indication. > > This could then be use for example by test runs, to flush the test output > after > the end of the test. This also requires a code change so is a flush function that bad an option? Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2] bsps: Add BSP_FLUSH_KERNEL_CHAR_OUTPUT BSP option
On 19.03.24 14:50, Kinsey Moore wrote: The xilinx-zynqmp-rpu bsp_reset() is modified, but not included in the spec file for the new option. Its family differs from the arm/xilinx-zynqmp BSP family with a -rpu suffix. Yes, but this BSP is quite new. I would prefer to let it not flush anything by default to carry out a reset. I'd be fine with this being enabled for the AArch64 BSPs as well, but I imagine that's better as a separate patch. Why should it be enabled by default? The arm/xilinx-zynq and arm/xilinx-zynqmp BSPs were the only ones doing an UART flush in the general termination procedure. It probably was done to address a specific use case (maybe test runs). I don't really like the new bsp_flush_kernel_char_output() function. Another approach could be an API change of /** * @ingroup RTEMSAPIKernelCharIO * * @brief Polled character output functions shall have this type. */ typedef void ( *BSP_output_char_function_type )( char ); to something like this typedef int ( *BSP_output_char_function_type )( int action ); If action in {0, ..., 255}, then print out the character. If 0x100 is set, then flush the output device. If 0x200 is set, then do Y... The return value could be used to give a status indication. This could then be use for example by test runs, to flush the test output after the end of the test. -- embedded brains GmbH & Co. KG Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/ ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2] bsps: Add BSP_FLUSH_KERNEL_CHAR_OUTPUT BSP option
The xilinx-zynqmp-rpu bsp_reset() is modified, but not included in the spec file for the new option. Its family differs from the arm/xilinx-zynqmp BSP family with a -rpu suffix. I'd be fine with this being enabled for the AArch64 BSPs as well, but I imagine that's better as a separate patch. Kinsey On Tue, Mar 19, 2024 at 3:39 AM Sebastian Huber < sebastian.hu...@embedded-brains.de> wrote: > Make the kernel I/O output character device flushing configurable. The > bsp_reset() function should reset the unit and do nothing else. > > The arm/xilinx-zynq and arm/xilinx-zynqmp BSPs were the only ones doing an > UART > flush in bsp_reset(). The bsp_reset() function should reset the system > and do > nothing more. Doing additional things like flushing an UART device may not > make sense for all applications. Some applications may not use the UART > device, so it may not be initialized and powered off. Some applications > may > use it with an application-specific protocol which doesn't like the > additional > four '\r' during reset. Doing a UART flush takes some time and some > applications may prefer a fast reset cycle. The bsp_reset() is the wrong > place > to do specific cleanup functions. > > Flushing the kernel I/O output character device is now optionally done in > bsp_fatal_extension() depending on the new BSP option > BSP_FLUSH_KERNEL_CHAR_OUTPUT. > --- > bsps/aarch64/xilinx-zynqmp/console/console.c | 3 ++- > bsps/aarch64/xilinx-zynqmp/include/bsp.h | 2 -- > bsps/arm/xilinx-zynq/console/debug-console.c | 10 + > bsps/arm/xilinx-zynq/start/bspreset.c | 4 > .../console/console-config.c | 3 ++- > bsps/arm/xilinx-zynqmp-rpu/include/bsp.h | 2 -- > bsps/arm/xilinx-zynqmp-rpu/start/bspreset.c | 3 --- > .../xilinx-zynqmp/console/console-config.c| 2 +- > bsps/arm/xilinx-zynqmp/include/bsp.h | 2 -- > bsps/arm/xilinx-zynqmp/start/bspreset.c | 4 +--- > bsps/include/bsp/bootcard.h | 5 + > bsps/shared/start/bspfatal-default.c | 4 > spec/build/bsps/bspopts.yml | 2 ++ > spec/build/bsps/optflushkerncharout.yml | 22 +++ > 14 files changed, 49 insertions(+), 19 deletions(-) > create mode 100644 spec/build/bsps/optflushkerncharout.yml > > diff --git a/bsps/aarch64/xilinx-zynqmp/console/console.c > b/bsps/aarch64/xilinx-zynqmp/console/console.c > index 9ce0b1da63..d1b2850952 100644 > --- a/bsps/aarch64/xilinx-zynqmp/console/console.c > +++ b/bsps/aarch64/xilinx-zynqmp/console/console.c > @@ -41,6 +41,7 @@ > #include > > #include > +#include > #include > #include > > @@ -234,7 +235,7 @@ rtems_status_code console_initialize( >return RTEMS_SUCCESSFUL; > } > > -void zynqmp_debug_console_flush(void) > +void bsp_flush_kernel_char_output(void) > { >zynq_uart_reset_tx_flush(&zynqmp_uart_instances[BSP_CONSOLE_MINOR]); > } > diff --git a/bsps/aarch64/xilinx-zynqmp/include/bsp.h > b/bsps/aarch64/xilinx-zynqmp/include/bsp.h > index 0ccca8b196..d36abde415 100644 > --- a/bsps/aarch64/xilinx-zynqmp/include/bsp.h > +++ b/bsps/aarch64/xilinx-zynqmp/include/bsp.h > @@ -86,8 +86,6 @@ BSP_START_TEXT_SECTION void > zynqmp_setup_mmu_and_cache(void); > */ > BSP_START_TEXT_SECTION void zynqmp_setup_secondary_cpu_mmu_and_cache( > void ); > > -void zynqmp_debug_console_flush(void); > - > uint32_t zynqmp_clock_i2c0(void); > > uint32_t zynqmp_clock_i2c1(void); > diff --git a/bsps/arm/xilinx-zynq/console/debug-console.c > b/bsps/arm/xilinx-zynq/console/debug-console.c > index d398ca7988..67fcbdf4a1 100644 > --- a/bsps/arm/xilinx-zynq/console/debug-console.c > +++ b/bsps/arm/xilinx-zynq/console/debug-console.c > @@ -38,10 +38,20 @@ > #include > > #include > +#include > #include > +#include > > #include > > +void bsp_flush_kernel_char_output(void) > +{ > + rtems_termios_device_context *base = > +&zynq_uart_instances[BSP_CONSOLE_MINOR].base; > + > + zynq_uart_reset_tx_flush(base); > +} > + > static void zynq_debug_console_out(char c) > { >rtems_termios_device_context *base = > diff --git a/bsps/arm/xilinx-zynq/start/bspreset.c > b/bsps/arm/xilinx-zynq/start/bspreset.c > index f8cc3b6999..fcdb1b8ded 100644 > --- a/bsps/arm/xilinx-zynq/start/bspreset.c > +++ b/bsps/arm/xilinx-zynq/start/bspreset.c > @@ -33,17 +33,13 @@ > * POSSIBILITY OF SUCH DAMAGE. > */ > > -#include > #include > -#include > > void bsp_reset(void) > { >volatile uint32_t *slcr_unlock = (volatile uint32_t *) 0xf808; >volatile uint32_t *pss_rst_ctrl = (volatile uint32_t *) 0xf8000200; > > - zynq_uart_reset_tx_flush(&zynq_uart_instances[BSP_CONSOLE_MINOR]); > - >while (true) { > *slcr_unlock = 0xdf0d; > *pss_rst_ctrl = 0x1; > diff --git a/bsps/arm/xilinx-zynqmp-rpu/console/console-config.c > b/bsps/arm/xilinx-zynqmp-rpu/console/console-config.c > index f52e008f2b..196b2dec58 100644 > --- a/bsps/arm/xilinx-zynqmp-rpu/console/console-c
[PATCH v2] bsps: Add BSP_FLUSH_KERNEL_CHAR_OUTPUT BSP option
Make the kernel I/O output character device flushing configurable. The bsp_reset() function should reset the unit and do nothing else. The arm/xilinx-zynq and arm/xilinx-zynqmp BSPs were the only ones doing an UART flush in bsp_reset(). The bsp_reset() function should reset the system and do nothing more. Doing additional things like flushing an UART device may not make sense for all applications. Some applications may not use the UART device, so it may not be initialized and powered off. Some applications may use it with an application-specific protocol which doesn't like the additional four '\r' during reset. Doing a UART flush takes some time and some applications may prefer a fast reset cycle. The bsp_reset() is the wrong place to do specific cleanup functions. Flushing the kernel I/O output character device is now optionally done in bsp_fatal_extension() depending on the new BSP option BSP_FLUSH_KERNEL_CHAR_OUTPUT. --- bsps/aarch64/xilinx-zynqmp/console/console.c | 3 ++- bsps/aarch64/xilinx-zynqmp/include/bsp.h | 2 -- bsps/arm/xilinx-zynq/console/debug-console.c | 10 + bsps/arm/xilinx-zynq/start/bspreset.c | 4 .../console/console-config.c | 3 ++- bsps/arm/xilinx-zynqmp-rpu/include/bsp.h | 2 -- bsps/arm/xilinx-zynqmp-rpu/start/bspreset.c | 3 --- .../xilinx-zynqmp/console/console-config.c| 2 +- bsps/arm/xilinx-zynqmp/include/bsp.h | 2 -- bsps/arm/xilinx-zynqmp/start/bspreset.c | 4 +--- bsps/include/bsp/bootcard.h | 5 + bsps/shared/start/bspfatal-default.c | 4 spec/build/bsps/bspopts.yml | 2 ++ spec/build/bsps/optflushkerncharout.yml | 22 +++ 14 files changed, 49 insertions(+), 19 deletions(-) create mode 100644 spec/build/bsps/optflushkerncharout.yml diff --git a/bsps/aarch64/xilinx-zynqmp/console/console.c b/bsps/aarch64/xilinx-zynqmp/console/console.c index 9ce0b1da63..d1b2850952 100644 --- a/bsps/aarch64/xilinx-zynqmp/console/console.c +++ b/bsps/aarch64/xilinx-zynqmp/console/console.c @@ -41,6 +41,7 @@ #include #include +#include #include #include @@ -234,7 +235,7 @@ rtems_status_code console_initialize( return RTEMS_SUCCESSFUL; } -void zynqmp_debug_console_flush(void) +void bsp_flush_kernel_char_output(void) { zynq_uart_reset_tx_flush(&zynqmp_uart_instances[BSP_CONSOLE_MINOR]); } diff --git a/bsps/aarch64/xilinx-zynqmp/include/bsp.h b/bsps/aarch64/xilinx-zynqmp/include/bsp.h index 0ccca8b196..d36abde415 100644 --- a/bsps/aarch64/xilinx-zynqmp/include/bsp.h +++ b/bsps/aarch64/xilinx-zynqmp/include/bsp.h @@ -86,8 +86,6 @@ BSP_START_TEXT_SECTION void zynqmp_setup_mmu_and_cache(void); */ BSP_START_TEXT_SECTION void zynqmp_setup_secondary_cpu_mmu_and_cache( void ); -void zynqmp_debug_console_flush(void); - uint32_t zynqmp_clock_i2c0(void); uint32_t zynqmp_clock_i2c1(void); diff --git a/bsps/arm/xilinx-zynq/console/debug-console.c b/bsps/arm/xilinx-zynq/console/debug-console.c index d398ca7988..67fcbdf4a1 100644 --- a/bsps/arm/xilinx-zynq/console/debug-console.c +++ b/bsps/arm/xilinx-zynq/console/debug-console.c @@ -38,10 +38,20 @@ #include #include +#include #include +#include #include +void bsp_flush_kernel_char_output(void) +{ + rtems_termios_device_context *base = +&zynq_uart_instances[BSP_CONSOLE_MINOR].base; + + zynq_uart_reset_tx_flush(base); +} + static void zynq_debug_console_out(char c) { rtems_termios_device_context *base = diff --git a/bsps/arm/xilinx-zynq/start/bspreset.c b/bsps/arm/xilinx-zynq/start/bspreset.c index f8cc3b6999..fcdb1b8ded 100644 --- a/bsps/arm/xilinx-zynq/start/bspreset.c +++ b/bsps/arm/xilinx-zynq/start/bspreset.c @@ -33,17 +33,13 @@ * POSSIBILITY OF SUCH DAMAGE. */ -#include #include -#include void bsp_reset(void) { volatile uint32_t *slcr_unlock = (volatile uint32_t *) 0xf808; volatile uint32_t *pss_rst_ctrl = (volatile uint32_t *) 0xf8000200; - zynq_uart_reset_tx_flush(&zynq_uart_instances[BSP_CONSOLE_MINOR]); - while (true) { *slcr_unlock = 0xdf0d; *pss_rst_ctrl = 0x1; diff --git a/bsps/arm/xilinx-zynqmp-rpu/console/console-config.c b/bsps/arm/xilinx-zynqmp-rpu/console/console-config.c index f52e008f2b..196b2dec58 100644 --- a/bsps/arm/xilinx-zynqmp-rpu/console/console-config.c +++ b/bsps/arm/xilinx-zynqmp-rpu/console/console-config.c @@ -35,6 +35,7 @@ #include #include +#include #include #include @@ -79,7 +80,7 @@ rtems_status_code console_initialize( return RTEMS_SUCCESSFUL; } -void zynqmp_debug_console_flush(void) +void bsp_flush_kernel_char_output(void) { zynq_uart_reset_tx_flush(&zynqmp_uart_instances[BSP_CONSOLE_MINOR]); } diff --git a/bsps/arm/xilinx-zynqmp-rpu/include/bsp.h b/bsps/arm/xilinx-zynqmp-rpu/include/bsp.h index e386bd4b26..060147967c 100644 --- a/bsps/arm/xilinx-zynqmp-rpu/include/bsp.h +++ b/bsps/arm/xilinx-zynqmp-rpu/include/bsp.h @@ -83,8