* [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation @ 2023-10-18 2:42 cp0613 2023-10-23 6:35 ` Guo Ren 2023-11-16 6:12 ` Anup Patel 0 siblings, 2 replies; 9+ messages in thread From: cp0613 @ 2023-10-18 2:42 UTC (permalink / raw) To: opensbi From: Chen Pei <cp0613@linux.alibaba.com> For some debuggers that do not implement SYSWRITEC and SYSREADC operations, we can use SYSWRITE and SYSREAD instead like the implementation of semihosting_getc(). Signed-off-by: Chen Pei <cp0613@linux.alibaba.com> Reviewed-by: Xiang W <wxjstz@126.com> Changed from v1: 1. simplified by calling semihosting_puts directly. --- lib/utils/serial/semihosting.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c index ce65887..a27c69e 100644 --- a/lib/utils/serial/semihosting.c +++ b/lib/utils/serial/semihosting.c @@ -160,11 +160,6 @@ static long semihosting_write(long fd, const void *memp, size_t len) /* clang-format on */ -static void semihosting_putc(char ch) -{ - semihosting_trap(SYSWRITEC, &ch); -} - static unsigned long semihosting_puts(const char *str, unsigned long len) { char ch; @@ -183,6 +178,11 @@ static unsigned long semihosting_puts(const char *str, unsigned long len) return (ret < 0) ? 0 : ret; } +static void semihosting_putc(char ch) +{ + semihosting_puts(&ch, 1); +} + static int semihosting_getc(void) { char ch = 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation 2023-10-18 2:42 [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation cp0613 @ 2023-10-23 6:35 ` Guo Ren 2023-10-23 8:22 ` Xiang W 2023-10-23 8:35 ` Anup Patel 2023-11-16 6:12 ` Anup Patel 1 sibling, 2 replies; 9+ messages in thread From: Guo Ren @ 2023-10-23 6:35 UTC (permalink / raw) To: opensbi On Wed, Oct 18, 2023 at 10:42?AM <cp0613@linux.alibaba.com> wrote: > > From: Chen Pei <cp0613@linux.alibaba.com> > > For some debuggers that do not implement SYSWRITEC and SYSREADC > operations, we can use SYSWRITE and SYSREAD instead like the > implementation of semihosting_getc(). > > Signed-off-by: Chen Pei <cp0613@linux.alibaba.com> > Reviewed-by: Xiang W <wxjstz@126.com> > > Changed from v1: > 1. simplified by calling semihosting_puts directly. > > --- > lib/utils/serial/semihosting.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c > index ce65887..a27c69e 100644 > --- a/lib/utils/serial/semihosting.c > +++ b/lib/utils/serial/semihosting.c > @@ -160,11 +160,6 @@ static long semihosting_write(long fd, const void *memp, size_t len) > > /* clang-format on */ > > -static void semihosting_putc(char ch) > -{ > - semihosting_trap(SYSWRITEC, &ch); > -} > - > static unsigned long semihosting_puts(const char *str, unsigned long len) > { > char ch; > @@ -183,6 +178,11 @@ static unsigned long semihosting_puts(const char *str, unsigned long len) > return (ret < 0) ? 0 : ret; > } > > +static void semihosting_putc(char ch) > +{ > + semihosting_puts(&ch, 1); > +} > + When console_puts() was introduced by commit c43903c4eae41 ("lib: sbi: Add console_puts() callback in the console device"), sbi_putc() became unnecessary and confusing. See below code: void sbi_putc(char ch) { if (console_dev && console_dev->console_putc) { if (ch == '\n') console_dev->console_putc('\r'); console_dev->console_putc(ch); } } static unsigned long nputs(const char *str, unsigned long len) { unsigned long i, ret; if (console_dev && console_dev->console_puts) { ret = console_dev->console_puts(str, len); } else { for (i = 0; i < len; i++) sbi_putc(str[i]); ret = len; } return ret; } Actually, sbi_putc() is duplicated from inputs(), and it's time to remove sbi_putc to make the opensbi code clearer. Here are the statistics of the work: lib/utils/serial/uart8250.c-static struct sbi_console_device uart8250_console = { lib/utils/serial/uart8250.c- .name = "uart8250", lib/utils/serial/uart8250.c: .console_putc = uart8250_putc, lib/utils/serial/uart8250.c- .console_getc = uart8250_getc lib/utils/serial/uart8250.c-}; -- lib/utils/serial/renesas_scif.c-static struct sbi_console_device renesas_scif_console = { lib/utils/serial/renesas_scif.c- .name = "renesas_scif", lib/utils/serial/renesas_scif.c: .console_putc = renesas_scif_putc, lib/utils/serial/renesas_scif.c-}; -- lib/utils/serial/semihosting.c-static struct sbi_console_device semihosting_console = { lib/utils/serial/semihosting.c- .name = "semihosting", lib/utils/serial/semihosting.c: .console_putc = semihosting_putc, lib/utils/serial/semihosting.c- .console_puts = semihosting_puts, lib/utils/serial/semihosting.c- .console_getc = semihosting_getc lib/utils/serial/semihosting.c-}; -- lib/utils/serial/shakti-uart.c-static struct sbi_console_device shakti_console = { lib/utils/serial/shakti-uart.c- .name = "shakti_uart", lib/utils/serial/shakti-uart.c: .console_putc = shakti_uart_putc, lib/utils/serial/shakti-uart.c- .console_getc = shakti_uart_getc lib/utils/serial/shakti-uart.c-}; -- lib/utils/serial/sifive-uart.c-static struct sbi_console_device sifive_console = { lib/utils/serial/sifive-uart.c- .name = "sifive_uart", lib/utils/serial/sifive-uart.c: .console_putc = sifive_uart_putc, lib/utils/serial/sifive-uart.c- .console_getc = sifive_uart_getc lib/utils/serial/sifive-uart.c-}; -- lib/utils/serial/litex-uart.c-static struct sbi_console_device litex_console = { lib/utils/serial/litex-uart.c- .name = "litex_uart", lib/utils/serial/litex-uart.c: .console_putc = litex_uart_putc, lib/utils/serial/litex-uart.c- .console_getc = litex_uart_getc lib/utils/serial/litex-uart.c-}; -- lib/utils/serial/gaisler-uart.c-static struct sbi_console_device gaisler_console = { lib/utils/serial/gaisler-uart.c- .name = "gaisler_uart", lib/utils/serial/gaisler-uart.c: .console_putc = gaisler_uart_putc, lib/utils/serial/gaisler-uart.c- .console_getc = gaisler_uart_getc lib/utils/serial/gaisler-uart.c-}; -- lib/utils/serial/xlnx-uartlite.c-static struct sbi_console_device xlnx_uartlite_console = { lib/utils/serial/xlnx-uartlite.c- .name = "xlnx-uartlite", lib/utils/serial/xlnx-uartlite.c: .console_putc = xlnx_uartlite_putc, lib/utils/serial/xlnx-uartlite.c- .console_getc = xlnx_uartlite_getc lib/utils/serial/xlnx-uartlite.c-}; -- lib/utils/serial/cadence-uart.c-static struct sbi_console_device cadence_console = { lib/utils/serial/cadence-uart.c- .name = "cadence_uart", lib/utils/serial/cadence-uart.c: .console_putc = cadence_uart_putc, lib/utils/serial/cadence-uart.c- .console_getc = cadence_uart_getc lib/utils/serial/cadence-uart.c-}; -- lib/utils/sys/htif.c-static struct sbi_console_device htif_console = { lib/utils/sys/htif.c- .name = "htif", lib/utils/sys/htif.c: .console_putc = htif_putc, lib/utils/sys/htif.c- .console_getc = htif_getc lib/utils/sys/htif.c-}; Is it worth removing .console_putc by implementing .console_puts.? > static int semihosting_getc(void) > { > char ch = 0; > -- > 2.25.1 > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi -- Best Regards Guo Ren ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation 2023-10-23 6:35 ` Guo Ren @ 2023-10-23 8:22 ` Xiang W 2023-10-23 8:35 ` Anup Patel 1 sibling, 0 replies; 9+ messages in thread From: Xiang W @ 2023-10-23 8:22 UTC (permalink / raw) To: opensbi ? 2023-10-23???? 14:35 +0800?Guo Ren??? > On Wed, Oct 18, 2023 at 10:42?AM <cp0613@linux.alibaba.com> wrote: > > > > From: Chen Pei <cp0613@linux.alibaba.com> > > > > For some debuggers that do not implement SYSWRITEC and SYSREADC > > operations, we can use SYSWRITE and SYSREAD instead like the > > implementation of semihosting_getc(). > > > > Signed-off-by: Chen Pei <cp0613@linux.alibaba.com> > > Reviewed-by: Xiang W <wxjstz@126.com> > > > > Changed from v1: > > 1. simplified by calling semihosting_puts directly. > > > > --- > > ?lib/utils/serial/semihosting.c | 10 +++++----- > > ?1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c > > index ce65887..a27c69e 100644 > > --- a/lib/utils/serial/semihosting.c > > +++ b/lib/utils/serial/semihosting.c > > @@ -160,11 +160,6 @@ static long semihosting_write(long fd, const void *memp, size_t len) > > > > ?/* clang-format on */ > > > > -static void semihosting_putc(char ch) > > -{ > > -?????? semihosting_trap(SYSWRITEC, &ch); > > -} > > - > > ?static unsigned long semihosting_puts(const char *str, unsigned long len) > > ?{ > > ??????? char ch; > > @@ -183,6 +178,11 @@ static unsigned long semihosting_puts(const char *str, unsigned long len) > > ??????? return (ret < 0) ? 0 : ret; > > ?} > > > > +static void semihosting_putc(char ch) > > +{ > > +?????? semihosting_puts(&ch, 1); > > +} > > + > When console_puts() was introduced by commit c43903c4eae41 ("lib: sbi: > Add console_puts() callback in the console device"), sbi_putc() became > unnecessary and confusing. See below code: > > void sbi_putc(char ch) > { > ??????? if (console_dev && console_dev->console_putc) { > ??????????????? if (ch == '\n') > ??????????????????????? console_dev->console_putc('\r'); > ??????????????? console_dev->console_putc(ch); > ??????? } > } > > static unsigned long nputs(const char *str, unsigned long len) > { > ??????? unsigned long i, ret; > > ??????? if (console_dev && console_dev->console_puts) { > ??????????????? ret = console_dev->console_puts(str, len); > ??????? } else { > ??????????????? for (i = 0; i < len; i++) > ??????????????????????? sbi_putc(str[i]); > ??????????????? ret = len; > ??????? } > > ??????? return ret; > } > > Actually, sbi_putc() is duplicated from inputs(), and it's time to > remove sbi_putc to make the opensbi code clearer. Here are the > statistics of the work: > > lib/utils/serial/uart8250.c-static struct sbi_console_device > uart8250_console = { > lib/utils/serial/uart8250.c-??? .name = "uart8250", > lib/utils/serial/uart8250.c:??? .console_putc = uart8250_putc, > lib/utils/serial/uart8250.c-??? .console_getc = uart8250_getc > lib/utils/serial/uart8250.c-}; > -- > lib/utils/serial/renesas_scif.c-static struct sbi_console_device > renesas_scif_console = { > lib/utils/serial/renesas_scif.c-??????? .name?????????? = "renesas_scif", > lib/utils/serial/renesas_scif.c:??????? .console_putc?? = renesas_scif_putc, > lib/utils/serial/renesas_scif.c-}; > -- > lib/utils/serial/semihosting.c-static struct sbi_console_device > semihosting_console = { > lib/utils/serial/semihosting.c- .name = "semihosting", > lib/utils/serial/semihosting.c: .console_putc = semihosting_putc, > lib/utils/serial/semihosting.c- .console_puts = semihosting_puts, > lib/utils/serial/semihosting.c- .console_getc = semihosting_getc > lib/utils/serial/semihosting.c-}; > -- > lib/utils/serial/shakti-uart.c-static struct sbi_console_device > shakti_console = { > lib/utils/serial/shakti-uart.c- .name = "shakti_uart", > lib/utils/serial/shakti-uart.c: .console_putc = shakti_uart_putc, > lib/utils/serial/shakti-uart.c- .console_getc = shakti_uart_getc > lib/utils/serial/shakti-uart.c-}; > -- > lib/utils/serial/sifive-uart.c-static struct sbi_console_device > sifive_console = { > lib/utils/serial/sifive-uart.c- .name = "sifive_uart", > lib/utils/serial/sifive-uart.c: .console_putc = sifive_uart_putc, > lib/utils/serial/sifive-uart.c- .console_getc = sifive_uart_getc > lib/utils/serial/sifive-uart.c-}; > -- > lib/utils/serial/litex-uart.c-static struct sbi_console_device litex_console = { > lib/utils/serial/litex-uart.c-? .name = "litex_uart", > lib/utils/serial/litex-uart.c:? .console_putc = litex_uart_putc, > lib/utils/serial/litex-uart.c-? .console_getc = litex_uart_getc > lib/utils/serial/litex-uart.c-}; > -- > lib/utils/serial/gaisler-uart.c-static struct sbi_console_device > gaisler_console = { > lib/utils/serial/gaisler-uart.c-??????? .name???????? = "gaisler_uart", > lib/utils/serial/gaisler-uart.c:??????? .console_putc = gaisler_uart_putc, > lib/utils/serial/gaisler-uart.c-??????? .console_getc = gaisler_uart_getc > lib/utils/serial/gaisler-uart.c-}; > -- > lib/utils/serial/xlnx-uartlite.c-static struct sbi_console_device > xlnx_uartlite_console = { > lib/utils/serial/xlnx-uartlite.c-?????? .name = "xlnx-uartlite", > lib/utils/serial/xlnx-uartlite.c:?????? .console_putc = xlnx_uartlite_putc, > lib/utils/serial/xlnx-uartlite.c-?????? .console_getc = xlnx_uartlite_getc > lib/utils/serial/xlnx-uartlite.c-}; > -- > lib/utils/serial/cadence-uart.c-static struct sbi_console_device > cadence_console = { > lib/utils/serial/cadence-uart.c-??????? .name = "cadence_uart", > lib/utils/serial/cadence-uart.c:??????? .console_putc = cadence_uart_putc, > lib/utils/serial/cadence-uart.c-??????? .console_getc = cadence_uart_getc > lib/utils/serial/cadence-uart.c-}; > -- > lib/utils/sys/htif.c-static struct sbi_console_device htif_console = { > lib/utils/sys/htif.c-?? .name = "htif", > lib/utils/sys/htif.c:?? .console_putc = htif_putc, > lib/utils/sys/htif.c-?? .console_getc = htif_getc > lib/utils/sys/htif.c-}; > > Is it worth removing .console_putc by implementing .console_puts.? I think sbi_console.c can be modified to make console_putc unnecessary. Just implement one of console_putc/console_puts. This can reduce modifications to sbi_console_device. Regards, Xiang W > > > ?static int semihosting_getc(void) > > ?{ > > ??????? char ch = 0; > > -- > > 2.25.1 > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi > > > > -- > Best Regards > ?Guo Ren > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation 2023-10-23 6:35 ` Guo Ren 2023-10-23 8:22 ` Xiang W @ 2023-10-23 8:35 ` Anup Patel 2023-10-23 8:57 ` Guo Ren 1 sibling, 1 reply; 9+ messages in thread From: Anup Patel @ 2023-10-23 8:35 UTC (permalink / raw) To: opensbi On Mon, Oct 23, 2023 at 12:05?PM Guo Ren <guoren@kernel.org> wrote: > > On Wed, Oct 18, 2023 at 10:42?AM <cp0613@linux.alibaba.com> wrote: > > > > From: Chen Pei <cp0613@linux.alibaba.com> > > > > For some debuggers that do not implement SYSWRITEC and SYSREADC > > operations, we can use SYSWRITE and SYSREAD instead like the > > implementation of semihosting_getc(). > > > > Signed-off-by: Chen Pei <cp0613@linux.alibaba.com> > > Reviewed-by: Xiang W <wxjstz@126.com> > > > > Changed from v1: > > 1. simplified by calling semihosting_puts directly. > > > > --- > > lib/utils/serial/semihosting.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c > > index ce65887..a27c69e 100644 > > --- a/lib/utils/serial/semihosting.c > > +++ b/lib/utils/serial/semihosting.c > > @@ -160,11 +160,6 @@ static long semihosting_write(long fd, const void *memp, size_t len) > > > > /* clang-format on */ > > > > -static void semihosting_putc(char ch) > > -{ > > - semihosting_trap(SYSWRITEC, &ch); > > -} > > - > > static unsigned long semihosting_puts(const char *str, unsigned long len) > > { > > char ch; > > @@ -183,6 +178,11 @@ static unsigned long semihosting_puts(const char *str, unsigned long len) > > return (ret < 0) ? 0 : ret; > > } > > > > +static void semihosting_putc(char ch) > > +{ > > + semihosting_puts(&ch, 1); > > +} > > + > When console_puts() was introduced by commit c43903c4eae41 ("lib: sbi: > Add console_puts() callback in the console device"), sbi_putc() became > unnecessary and confusing. See below code: > > void sbi_putc(char ch) > { > if (console_dev && console_dev->console_putc) { > if (ch == '\n') > console_dev->console_putc('\r'); > console_dev->console_putc(ch); > } > } > > static unsigned long nputs(const char *str, unsigned long len) > { > unsigned long i, ret; > > if (console_dev && console_dev->console_puts) { > ret = console_dev->console_puts(str, len); > } else { > for (i = 0; i < len; i++) > sbi_putc(str[i]); > ret = len; > } > > return ret; > } > > Actually, sbi_putc() is duplicated from inputs(), and it's time to > remove sbi_putc to make the opensbi code clearer. Here are the > statistics of the work: > > lib/utils/serial/uart8250.c-static struct sbi_console_device > uart8250_console = { > lib/utils/serial/uart8250.c- .name = "uart8250", > lib/utils/serial/uart8250.c: .console_putc = uart8250_putc, > lib/utils/serial/uart8250.c- .console_getc = uart8250_getc > lib/utils/serial/uart8250.c-}; > -- > lib/utils/serial/renesas_scif.c-static struct sbi_console_device > renesas_scif_console = { > lib/utils/serial/renesas_scif.c- .name = "renesas_scif", > lib/utils/serial/renesas_scif.c: .console_putc = renesas_scif_putc, > lib/utils/serial/renesas_scif.c-}; > -- > lib/utils/serial/semihosting.c-static struct sbi_console_device > semihosting_console = { > lib/utils/serial/semihosting.c- .name = "semihosting", > lib/utils/serial/semihosting.c: .console_putc = semihosting_putc, > lib/utils/serial/semihosting.c- .console_puts = semihosting_puts, > lib/utils/serial/semihosting.c- .console_getc = semihosting_getc > lib/utils/serial/semihosting.c-}; > -- > lib/utils/serial/shakti-uart.c-static struct sbi_console_device > shakti_console = { > lib/utils/serial/shakti-uart.c- .name = "shakti_uart", > lib/utils/serial/shakti-uart.c: .console_putc = shakti_uart_putc, > lib/utils/serial/shakti-uart.c- .console_getc = shakti_uart_getc > lib/utils/serial/shakti-uart.c-}; > -- > lib/utils/serial/sifive-uart.c-static struct sbi_console_device > sifive_console = { > lib/utils/serial/sifive-uart.c- .name = "sifive_uart", > lib/utils/serial/sifive-uart.c: .console_putc = sifive_uart_putc, > lib/utils/serial/sifive-uart.c- .console_getc = sifive_uart_getc > lib/utils/serial/sifive-uart.c-}; > -- > lib/utils/serial/litex-uart.c-static struct sbi_console_device litex_console = { > lib/utils/serial/litex-uart.c- .name = "litex_uart", > lib/utils/serial/litex-uart.c: .console_putc = litex_uart_putc, > lib/utils/serial/litex-uart.c- .console_getc = litex_uart_getc > lib/utils/serial/litex-uart.c-}; > -- > lib/utils/serial/gaisler-uart.c-static struct sbi_console_device > gaisler_console = { > lib/utils/serial/gaisler-uart.c- .name = "gaisler_uart", > lib/utils/serial/gaisler-uart.c: .console_putc = gaisler_uart_putc, > lib/utils/serial/gaisler-uart.c- .console_getc = gaisler_uart_getc > lib/utils/serial/gaisler-uart.c-}; > -- > lib/utils/serial/xlnx-uartlite.c-static struct sbi_console_device > xlnx_uartlite_console = { > lib/utils/serial/xlnx-uartlite.c- .name = "xlnx-uartlite", > lib/utils/serial/xlnx-uartlite.c: .console_putc = xlnx_uartlite_putc, > lib/utils/serial/xlnx-uartlite.c- .console_getc = xlnx_uartlite_getc > lib/utils/serial/xlnx-uartlite.c-}; > -- > lib/utils/serial/cadence-uart.c-static struct sbi_console_device > cadence_console = { > lib/utils/serial/cadence-uart.c- .name = "cadence_uart", > lib/utils/serial/cadence-uart.c: .console_putc = cadence_uart_putc, > lib/utils/serial/cadence-uart.c- .console_getc = cadence_uart_getc > lib/utils/serial/cadence-uart.c-}; > -- > lib/utils/sys/htif.c-static struct sbi_console_device htif_console = { > lib/utils/sys/htif.c- .name = "htif", > lib/utils/sys/htif.c: .console_putc = htif_putc, > lib/utils/sys/htif.c- .console_getc = htif_getc > lib/utils/sys/htif.c-}; > > Is it worth removing .console_putc by implementing .console_puts.? NACK, having console_puts in all serial drivers would mean duplicating generic code into all drivers. Regards, Anup > > > static int semihosting_getc(void) > > { > > char ch = 0; > > -- > > 2.25.1 > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi > > > > -- > Best Regards > Guo Ren > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation 2023-10-23 8:35 ` Anup Patel @ 2023-10-23 8:57 ` Guo Ren 0 siblings, 0 replies; 9+ messages in thread From: Guo Ren @ 2023-10-23 8:57 UTC (permalink / raw) To: opensbi On Mon, Oct 23, 2023 at 4:36?PM Anup Patel <apatel@ventanamicro.com> wrote: > > On Mon, Oct 23, 2023 at 12:05?PM Guo Ren <guoren@kernel.org> wrote: > > > > On Wed, Oct 18, 2023 at 10:42?AM <cp0613@linux.alibaba.com> wrote: > > > > > > From: Chen Pei <cp0613@linux.alibaba.com> > > > > > > For some debuggers that do not implement SYSWRITEC and SYSREADC > > > operations, we can use SYSWRITE and SYSREAD instead like the > > > implementation of semihosting_getc(). > > > > > > Signed-off-by: Chen Pei <cp0613@linux.alibaba.com> > > > Reviewed-by: Xiang W <wxjstz@126.com> > > > > > > Changed from v1: > > > 1. simplified by calling semihosting_puts directly. > > > > > > --- > > > lib/utils/serial/semihosting.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c > > > index ce65887..a27c69e 100644 > > > --- a/lib/utils/serial/semihosting.c > > > +++ b/lib/utils/serial/semihosting.c > > > @@ -160,11 +160,6 @@ static long semihosting_write(long fd, const void *memp, size_t len) > > > > > > /* clang-format on */ > > > > > > -static void semihosting_putc(char ch) > > > -{ > > > - semihosting_trap(SYSWRITEC, &ch); > > > -} > > > - > > > static unsigned long semihosting_puts(const char *str, unsigned long len) > > > { > > > char ch; > > > @@ -183,6 +178,11 @@ static unsigned long semihosting_puts(const char *str, unsigned long len) > > > return (ret < 0) ? 0 : ret; > > > } > > > > > > +static void semihosting_putc(char ch) > > > +{ > > > + semihosting_puts(&ch, 1); > > > +} > > > + > > When console_puts() was introduced by commit c43903c4eae41 ("lib: sbi: > > Add console_puts() callback in the console device"), sbi_putc() became > > unnecessary and confusing. See below code: > > > > void sbi_putc(char ch) > > { > > if (console_dev && console_dev->console_putc) { > > if (ch == '\n') > > console_dev->console_putc('\r'); > > console_dev->console_putc(ch); > > } > > } > > > > static unsigned long nputs(const char *str, unsigned long len) > > { > > unsigned long i, ret; > > > > if (console_dev && console_dev->console_puts) { > > ret = console_dev->console_puts(str, len); > > } else { > > for (i = 0; i < len; i++) > > sbi_putc(str[i]); > > ret = len; > > } > > > > return ret; > > } > > > > Actually, sbi_putc() is duplicated from inputs(), and it's time to > > remove sbi_putc to make the opensbi code clearer. Here are the > > statistics of the work: > > > > lib/utils/serial/uart8250.c-static struct sbi_console_device > > uart8250_console = { > > lib/utils/serial/uart8250.c- .name = "uart8250", > > lib/utils/serial/uart8250.c: .console_putc = uart8250_putc, > > lib/utils/serial/uart8250.c- .console_getc = uart8250_getc > > lib/utils/serial/uart8250.c-}; > > -- > > lib/utils/serial/renesas_scif.c-static struct sbi_console_device > > renesas_scif_console = { > > lib/utils/serial/renesas_scif.c- .name = "renesas_scif", > > lib/utils/serial/renesas_scif.c: .console_putc = renesas_scif_putc, > > lib/utils/serial/renesas_scif.c-}; > > -- > > lib/utils/serial/semihosting.c-static struct sbi_console_device > > semihosting_console = { > > lib/utils/serial/semihosting.c- .name = "semihosting", > > lib/utils/serial/semihosting.c: .console_putc = semihosting_putc, > > lib/utils/serial/semihosting.c- .console_puts = semihosting_puts, > > lib/utils/serial/semihosting.c- .console_getc = semihosting_getc > > lib/utils/serial/semihosting.c-}; > > -- > > lib/utils/serial/shakti-uart.c-static struct sbi_console_device > > shakti_console = { > > lib/utils/serial/shakti-uart.c- .name = "shakti_uart", > > lib/utils/serial/shakti-uart.c: .console_putc = shakti_uart_putc, > > lib/utils/serial/shakti-uart.c- .console_getc = shakti_uart_getc > > lib/utils/serial/shakti-uart.c-}; > > -- > > lib/utils/serial/sifive-uart.c-static struct sbi_console_device > > sifive_console = { > > lib/utils/serial/sifive-uart.c- .name = "sifive_uart", > > lib/utils/serial/sifive-uart.c: .console_putc = sifive_uart_putc, > > lib/utils/serial/sifive-uart.c- .console_getc = sifive_uart_getc > > lib/utils/serial/sifive-uart.c-}; > > -- > > lib/utils/serial/litex-uart.c-static struct sbi_console_device litex_console = { > > lib/utils/serial/litex-uart.c- .name = "litex_uart", > > lib/utils/serial/litex-uart.c: .console_putc = litex_uart_putc, > > lib/utils/serial/litex-uart.c- .console_getc = litex_uart_getc > > lib/utils/serial/litex-uart.c-}; > > -- > > lib/utils/serial/gaisler-uart.c-static struct sbi_console_device > > gaisler_console = { > > lib/utils/serial/gaisler-uart.c- .name = "gaisler_uart", > > lib/utils/serial/gaisler-uart.c: .console_putc = gaisler_uart_putc, > > lib/utils/serial/gaisler-uart.c- .console_getc = gaisler_uart_getc > > lib/utils/serial/gaisler-uart.c-}; > > -- > > lib/utils/serial/xlnx-uartlite.c-static struct sbi_console_device > > xlnx_uartlite_console = { > > lib/utils/serial/xlnx-uartlite.c- .name = "xlnx-uartlite", > > lib/utils/serial/xlnx-uartlite.c: .console_putc = xlnx_uartlite_putc, > > lib/utils/serial/xlnx-uartlite.c- .console_getc = xlnx_uartlite_getc > > lib/utils/serial/xlnx-uartlite.c-}; > > -- > > lib/utils/serial/cadence-uart.c-static struct sbi_console_device > > cadence_console = { > > lib/utils/serial/cadence-uart.c- .name = "cadence_uart", > > lib/utils/serial/cadence-uart.c: .console_putc = cadence_uart_putc, > > lib/utils/serial/cadence-uart.c- .console_getc = cadence_uart_getc > > lib/utils/serial/cadence-uart.c-}; > > -- > > lib/utils/sys/htif.c-static struct sbi_console_device htif_console = { > > lib/utils/sys/htif.c- .name = "htif", > > lib/utils/sys/htif.c: .console_putc = htif_putc, > > lib/utils/sys/htif.c- .console_getc = htif_getc > > lib/utils/sys/htif.c-}; > > > > Is it worth removing .console_putc by implementing .console_puts.? > > NACK, having console_puts in all serial drivers would mean > duplicating generic code into all drivers. Okay, then, I agree with the patch. +static void semihosting_putc(char ch) +{ + semihosting_puts(&ch, 1); +} Acked-by: Guo Ren <guoren@kernel.org> > > Regards, > Anup > > > > > > static int semihosting_getc(void) > > > { > > > char ch = 0; > > > -- > > > 2.25.1 > > > > > > > > > -- > > > opensbi mailing list > > > opensbi at lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/opensbi > > > > > > > > -- > > Best Regards > > Guo Ren > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi -- Best Regards Guo Ren ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation 2023-10-18 2:42 [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation cp0613 2023-10-23 6:35 ` Guo Ren @ 2023-11-16 6:12 ` Anup Patel 2023-11-16 8:45 ` Xiang W 1 sibling, 1 reply; 9+ messages in thread From: Anup Patel @ 2023-11-16 6:12 UTC (permalink / raw) To: opensbi On Wed, Oct 18, 2023 at 8:12?AM <cp0613@linux.alibaba.com> wrote: > > From: Chen Pei <cp0613@linux.alibaba.com> > > For some debuggers that do not implement SYSWRITEC and SYSREADC > operations, we can use SYSWRITE and SYSREAD instead like the > implementation of semihosting_getc(). > > Signed-off-by: Chen Pei <cp0613@linux.alibaba.com> > Reviewed-by: Xiang W <wxjstz@126.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Applied this patch to the riscv/opensbi repo. Thanks, Anup > > Changed from v1: > 1. simplified by calling semihosting_puts directly. > > --- > lib/utils/serial/semihosting.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c > index ce65887..a27c69e 100644 > --- a/lib/utils/serial/semihosting.c > +++ b/lib/utils/serial/semihosting.c > @@ -160,11 +160,6 @@ static long semihosting_write(long fd, const void *memp, size_t len) > > /* clang-format on */ > > -static void semihosting_putc(char ch) > -{ > - semihosting_trap(SYSWRITEC, &ch); > -} > - > static unsigned long semihosting_puts(const char *str, unsigned long len) > { > char ch; > @@ -183,6 +178,11 @@ static unsigned long semihosting_puts(const char *str, unsigned long len) > return (ret < 0) ? 0 : ret; > } > > +static void semihosting_putc(char ch) > +{ > + semihosting_puts(&ch, 1); > +} > + > static int semihosting_getc(void) > { > char ch = 0; > -- > 2.25.1 > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation 2023-11-16 6:12 ` Anup Patel @ 2023-11-16 8:45 ` Xiang W 2023-11-16 16:00 ` Anup Patel 0 siblings, 1 reply; 9+ messages in thread From: Xiang W @ 2023-11-16 8:45 UTC (permalink / raw) To: opensbi ? 2023-11-16???? 11:42 +0530?Anup Patel??? > On Wed, Oct 18, 2023 at 8:12?AM <cp0613@linux.alibaba.com> wrote: > > > > From: Chen Pei <cp0613@linux.alibaba.com> > > > > For some debuggers that do not implement SYSWRITEC and SYSREADC > > operations, we can use SYSWRITE and SYSREAD instead like the > > implementation of semihosting_getc(). > > > > Signed-off-by: Chen Pei <cp0613@linux.alibaba.com> > > Reviewed-by: Xiang W <wxjstz@126.com> > > Looks good to me. > > Reviewed-by: Anup Patel <anup@brainfault.org> > > Applied this patch to the riscv/opensbi repo. I submitted a new workaround. https://patchwork.ozlabs.org/project/opensbi/patch/20231025043104.748104-1-wxjstz at 126.com/ https://patchwork.ozlabs.org/project/opensbi/patch/20231025043104.748104-2-wxjstz at 126.com/ Regards, Xiang W > > Thanks, > Anup > > > > > Changed from v1: > > 1. simplified by calling semihosting_puts directly. > > > > --- > > ?lib/utils/serial/semihosting.c | 10 +++++----- > > ?1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c > > index ce65887..a27c69e 100644 > > --- a/lib/utils/serial/semihosting.c > > +++ b/lib/utils/serial/semihosting.c > > @@ -160,11 +160,6 @@ static long semihosting_write(long fd, const void *memp, size_t len) > > > > ?/* clang-format on */ > > > > -static void semihosting_putc(char ch) > > -{ > > -?????? semihosting_trap(SYSWRITEC, &ch); > > -} > > - > > ?static unsigned long semihosting_puts(const char *str, unsigned long len) > > ?{ > > ??????? char ch; > > @@ -183,6 +178,11 @@ static unsigned long semihosting_puts(const char *str, unsigned long len) > > ??????? return (ret < 0) ? 0 : ret; > > ?} > > > > +static void semihosting_putc(char ch) > > +{ > > +?????? semihosting_puts(&ch, 1); > > +} > > + > > ?static int semihosting_getc(void) > > ?{ > > ??????? char ch = 0; > > -- > > 2.25.1 > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation 2023-11-16 8:45 ` Xiang W @ 2023-11-16 16:00 ` Anup Patel 2023-11-17 1:06 ` Xiang W 0 siblings, 1 reply; 9+ messages in thread From: Anup Patel @ 2023-11-16 16:00 UTC (permalink / raw) To: opensbi On Thu, Nov 16, 2023 at 2:16?PM Xiang W <wxjstz@126.com> wrote: > > ? 2023-11-16???? 11:42 +0530?Anup Patel??? > > On Wed, Oct 18, 2023 at 8:12?AM <cp0613@linux.alibaba.com> wrote: > > > > > > From: Chen Pei <cp0613@linux.alibaba.com> > > > > > > For some debuggers that do not implement SYSWRITEC and SYSREADC > > > operations, we can use SYSWRITE and SYSREAD instead like the > > > implementation of semihosting_getc(). > > > > > > Signed-off-by: Chen Pei <cp0613@linux.alibaba.com> > > > Reviewed-by: Xiang W <wxjstz@126.com> > > > > Looks good to me. > > > > Reviewed-by: Anup Patel <anup@brainfault.org> > > > > Applied this patch to the riscv/opensbi repo. > I submitted a new workaround. > > https://patchwork.ozlabs.org/project/opensbi/patch/20231025043104.748104-1-wxjstz at 126.com/ > https://patchwork.ozlabs.org/project/opensbi/patch/20231025043104.748104-2-wxjstz at 126.com/ Can you rebase and send a new version for review ? Regards, Anup > > Regards, > Xiang W > > > > Thanks, > > Anup > > > > > > > > Changed from v1: > > > 1. simplified by calling semihosting_puts directly. > > > > > > --- > > > lib/utils/serial/semihosting.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c > > > index ce65887..a27c69e 100644 > > > --- a/lib/utils/serial/semihosting.c > > > +++ b/lib/utils/serial/semihosting.c > > > @@ -160,11 +160,6 @@ static long semihosting_write(long fd, const void *memp, size_t len) > > > > > > /* clang-format on */ > > > > > > -static void semihosting_putc(char ch) > > > -{ > > > - semihosting_trap(SYSWRITEC, &ch); > > > -} > > > - > > > static unsigned long semihosting_puts(const char *str, unsigned long len) > > > { > > > char ch; > > > @@ -183,6 +178,11 @@ static unsigned long semihosting_puts(const char *str, unsigned long len) > > > return (ret < 0) ? 0 : ret; > > > } > > > > > > +static void semihosting_putc(char ch) > > > +{ > > > + semihosting_puts(&ch, 1); > > > +} > > > + > > > static int semihosting_getc(void) > > > { > > > char ch = 0; > > > -- > > > 2.25.1 > > > > > > > > > -- > > > opensbi mailing list > > > opensbi at lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/opensbi > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation 2023-11-16 16:00 ` Anup Patel @ 2023-11-17 1:06 ` Xiang W 0 siblings, 0 replies; 9+ messages in thread From: Xiang W @ 2023-11-17 1:06 UTC (permalink / raw) To: opensbi ? 2023-11-16???? 21:30 +0530?Anup Patel??? > On Thu, Nov 16, 2023 at 2:16?PM Xiang W <wxjstz@126.com> wrote: > > > > ? 2023-11-16???? 11:42 +0530?Anup Patel??? > > > On Wed, Oct 18, 2023 at 8:12?AM <cp0613@linux.alibaba.com> wrote: > > > > > > > > From: Chen Pei <cp0613@linux.alibaba.com> > > > > > > > > For some debuggers that do not implement SYSWRITEC and SYSREADC > > > > operations, we can use SYSWRITE and SYSREAD instead like the > > > > implementation of semihosting_getc(). > > > > > > > > Signed-off-by: Chen Pei <cp0613@linux.alibaba.com> > > > > Reviewed-by: Xiang W <wxjstz@126.com> > > > > > > Looks good to me. > > > > > > Reviewed-by: Anup Patel <anup@brainfault.org> > > > > > > Applied this patch to the riscv/opensbi repo. > > I submitted a new workaround. > > > > https://patchwork.ozlabs.org/project/opensbi/patch/20231025043104.748104-1-wxjstz at 126.com/ > > https://patchwork.ozlabs.org/project/opensbi/patch/20231025043104.748104-2-wxjstz at 126.com/ > > Can you rebase and send a new version for review ? New version at: https://patchwork.ozlabs.org/project/opensbi/patch/20231117010440.105759-1-wxjstz at 126.com/ https://patchwork.ozlabs.org/project/opensbi/patch/20231117010440.105759-2-wxjstz at 126.com/ Regards, Xiang W > > Regards, > Anup > > > > > Regards, > > Xiang W > > > > > > Thanks, > > > Anup > > > > > > > > > > > Changed from v1: > > > > 1. simplified by calling semihosting_puts directly. > > > > > > > > --- > > > > ?lib/utils/serial/semihosting.c | 10 +++++----- > > > > ?1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c > > > > index ce65887..a27c69e 100644 > > > > --- a/lib/utils/serial/semihosting.c > > > > +++ b/lib/utils/serial/semihosting.c > > > > @@ -160,11 +160,6 @@ static long semihosting_write(long fd, const void *memp, size_t len) > > > > > > > > ?/* clang-format on */ > > > > > > > > -static void semihosting_putc(char ch) > > > > -{ > > > > -?????? semihosting_trap(SYSWRITEC, &ch); > > > > -} > > > > - > > > > ?static unsigned long semihosting_puts(const char *str, unsigned long len) > > > > ?{ > > > > ??????? char ch; > > > > @@ -183,6 +178,11 @@ static unsigned long semihosting_puts(const char *str, unsigned long len) > > > > ??????? return (ret < 0) ? 0 : ret; > > > > ?} > > > > > > > > +static void semihosting_putc(char ch) > > > > +{ > > > > +?????? semihosting_puts(&ch, 1); > > > > +} > > > > + > > > > ?static int semihosting_getc(void) > > > > ?{ > > > > ??????? char ch = 0; > > > > -- > > > > 2.25.1 > > > > > > > > > > > > -- > > > > opensbi mailing list > > > > opensbi at lists.infradead.org > > > > http://lists.infradead.org/mailman/listinfo/opensbi > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-11-17 1:06 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-18 2:42 [PATCH v2] lib: utils/serial: Optimize semihosting_putc implementation cp0613 2023-10-23 6:35 ` Guo Ren 2023-10-23 8:22 ` Xiang W 2023-10-23 8:35 ` Anup Patel 2023-10-23 8:57 ` Guo Ren 2023-11-16 6:12 ` Anup Patel 2023-11-16 8:45 ` Xiang W 2023-11-16 16:00 ` Anup Patel 2023-11-17 1:06 ` Xiang W
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox