* [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