| Message ID | 20211113070622.90364-1-wxjstz@126.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | lib: utils/serial: Modify uart_getc to a blocking function | expand |
On 13 Nov 2021, at 07:06, Xiang W <wxjstz@126.com> wrote: > > If uart_getc is non-blocking, if the sending end is faster than the > receiving end, the data received by some functions(sbi_gets) is > incomplete Without hardware flow control this is always going to be true. Making getc blocking does not change that. Moreover, making it blocking means there is no longer a way to poll, whereas having it be non-blocking means you can poll, and if you want to make it blocking you can always put a loop in the caller (which also avoids putting it in every single driver). Also, this breaks the SBI getchar call; it’s supposed to be non-blocking, but this makes it blocking, so it is impossible for an OS to poll the SBI console. Thus, NACK of the entire concept. Jess > Signed-off-by: Xiang W <wxjstz@126.com> > --- > lib/utils/serial/gaisler-uart.c | 5 ++--- > lib/utils/serial/shakti-uart.c | 7 +++---- > lib/utils/serial/sifive-uart.c | 9 +++++---- > lib/utils/serial/uart8250.c | 6 +++--- > lib/utils/sys/htif.c | 11 +++++------ > 5 files changed, 18 insertions(+), 20 deletions(-) > > diff --git a/lib/utils/serial/gaisler-uart.c b/lib/utils/serial/gaisler-uart.c > index 49298e9..1dc5ae0 100644 > --- a/lib/utils/serial/gaisler-uart.c > +++ b/lib/utils/serial/gaisler-uart.c > @@ -51,9 +51,8 @@ static void gaisler_uart_putc(char ch) > > static int gaisler_uart_getc(void) > { > - u32 ret = get_reg(UART_REG_STATUS); > - if (!(ret & UART_STATUS_DATAREADY)) > - return -1; > + while (!(get_reg(UART_REG_STATUS) & UART_STATUS_DATAREADY)) > + ; > return get_reg(UART_REG_DATA) & UART_DATA_DATA; > } > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > index e77a985..a5b72aa 100644 > --- a/lib/utils/serial/shakti-uart.c > +++ b/lib/utils/serial/shakti-uart.c > @@ -32,10 +32,9 @@ static void shakti_uart_putc(char ch) > > static int shakti_uart_getc(void) > { > - u16 status = readw(uart_base + REG_STATUS); > - if (status & UART_RX_FULL) > - return readb(uart_base + REG_RX); > - return -1; > + while (!(readw(uart_base + REG_STATUS) & UART_RX_FULL)) > + ; > + return readb(uart_base + REG_RX); > } > > static struct sbi_console_device shakti_console = { > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > index 57d80fa..68a9ca7 100644 > --- a/lib/utils/serial/sifive-uart.c > +++ b/lib/utils/serial/sifive-uart.c > @@ -76,10 +76,11 @@ static void sifive_uart_putc(char ch) > > static int sifive_uart_getc(void) > { > - u32 ret = get_reg(UART_REG_RXFIFO); > - if (!(ret & UART_RXFIFO_EMPTY)) > - return ret & UART_RXFIFO_DATA; > - return -1; > + u32 ret; > + do > + ret = get_reg(UART_REG_RXFIFO); > + while (ret & UART_RXFIFO_EMPTY); > + return ret & UART_RXFIFO_DATA; > } > > static struct sbi_console_device sifive_console = { > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > index 1cf6624..bab8111 100644 > --- a/lib/utils/serial/uart8250.c > +++ b/lib/utils/serial/uart8250.c > @@ -79,9 +79,9 @@ static void uart8250_putc(char ch) > > static int uart8250_getc(void) > { > - if (get_reg(UART_LSR_OFFSET) & UART_LSR_DR) > - return get_reg(UART_RBR_OFFSET); > - return -1; > + while (!(get_reg(UART_LSR_OFFSET) & UART_LSR_DR)) > + ; > + return get_reg(UART_RBR_OFFSET); > } > > static struct sbi_console_device uart8250_console = { > diff --git a/lib/utils/sys/htif.c b/lib/utils/sys/htif.c > index 7c69c7f..dfda30a 100644 > --- a/lib/utils/sys/htif.c > +++ b/lib/utils/sys/htif.c > @@ -47,7 +47,7 @@ > > volatile uint64_t tohost __attribute__((section(".htif"))); > volatile uint64_t fromhost __attribute__((section(".htif"))); > -static int htif_console_buf; > +static int htif_console_buf = -1; > static spinlock_t htif_lock = SPIN_LOCK_INITIALIZER; > > static void __check_fromhost() > @@ -130,12 +130,11 @@ static int htif_getc(void) > > spin_lock(&htif_lock); > > - __check_fromhost(); > + while (htif_console_buf < 0) > + __check_fromhost(); > ch = htif_console_buf; > - if (ch >= 0) { > - htif_console_buf = -1; > - __set_tohost(HTIF_DEV_CONSOLE, HTIF_CONSOLE_CMD_GETC, 0); > - } > + htif_console_buf = -1; > + __set_tohost(HTIF_DEV_CONSOLE, HTIF_CONSOLE_CMD_GETC, 0); > > spin_unlock(&htif_lock); > > -- > 2.30.2 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
在 2021-11-13星期六的 22:11 +0000，Jessica Clarke写道： > On 13 Nov 2021, at 07:06, Xiang W <wxjstz@126.com> wrote: > > > > If uart_getc is non-blocking, if the sending end is faster than the > > receiving end, the data received by some functions(sbi_gets) is > > incomplete > > Without hardware flow control this is always going to be true. Making > getc blocking does not change that. Moreover, making it blocking > means > there is no longer a way to poll, whereas having it be non-blocking > means you can poll, and if you want to make it blocking you can > always > put a loop in the caller (which also avoids putting it in every > single > driver). > > Also, this breaks the SBI getchar call; it’s supposed to be > non-blocking, but this makes it blocking, so it is impossible for an > OS > to poll the SBI console. Linux also needs blocking sbi_getc, the following is the source code in linux static int hvc_sbi_tty_get(uint32_t vtermno, char *buf, int count) { int i, c; for (i = 0; i < count; i++) { c = sbi_console_getchar(); if (c < 0) break; buf[i] = c; } return i; } static const struct hv_ops hvc_sbi_ops = { .get_chars = hvc_sbi_tty_get, .put_chars = hvc_sbi_tty_put, }; Regards, Xiang W > > Thus, NACK of the entire concept. > > Jess > > > Signed-off-by: Xiang W <wxjstz@126.com> > > --- > > lib/utils/serial/gaisler-uart.c | 5 ++--- > > lib/utils/serial/shakti-uart.c | 7 +++---- > > lib/utils/serial/sifive-uart.c | 9 +++++---- > > lib/utils/serial/uart8250.c | 6 +++--- > > lib/utils/sys/htif.c | 11 +++++------ > > 5 files changed, 18 insertions(+), 20 deletions(-) > > > > diff --git a/lib/utils/serial/gaisler-uart.c > > b/lib/utils/serial/gaisler-uart.c > > index 49298e9..1dc5ae0 100644 > > --- a/lib/utils/serial/gaisler-uart.c > > +++ b/lib/utils/serial/gaisler-uart.c > > @@ -51,9 +51,8 @@ static void gaisler_uart_putc(char ch) > > > > static int gaisler_uart_getc(void) > > { > > - u32 ret = get_reg(UART_REG_STATUS); > > - if (!(ret & UART_STATUS_DATAREADY)) > > - return -1; > > + while (!(get_reg(UART_REG_STATUS) & UART_STATUS_DATAREADY)) > > + ; > > return get_reg(UART_REG_DATA) & UART_DATA_DATA; > > } > > > > diff --git a/lib/utils/serial/shakti-uart.c > > b/lib/utils/serial/shakti-uart.c > > index e77a985..a5b72aa 100644 > > --- a/lib/utils/serial/shakti-uart.c > > +++ b/lib/utils/serial/shakti-uart.c > > @@ -32,10 +32,9 @@ static void shakti_uart_putc(char ch) > > > > static int shakti_uart_getc(void) > > { > > - u16 status = readw(uart_base + REG_STATUS); > > - if (status & UART_RX_FULL) > > - return readb(uart_base + REG_RX); > > - return -1; > > + while (!(readw(uart_base + REG_STATUS) & UART_RX_FULL)) > > + ; > > + return readb(uart_base + REG_RX); > > } > > > > static struct sbi_console_device shakti_console = { > > diff --git a/lib/utils/serial/sifive-uart.c > > b/lib/utils/serial/sifive-uart.c > > index 57d80fa..68a9ca7 100644 > > --- a/lib/utils/serial/sifive-uart.c > > +++ b/lib/utils/serial/sifive-uart.c > > @@ -76,10 +76,11 @@ static void sifive_uart_putc(char ch) > > > > static int sifive_uart_getc(void) > > { > > - u32 ret = get_reg(UART_REG_RXFIFO); > > - if (!(ret & UART_RXFIFO_EMPTY)) > > - return ret & UART_RXFIFO_DATA; > > - return -1; > > + u32 ret; > > + do > > + ret = get_reg(UART_REG_RXFIFO); > > + while (ret & UART_RXFIFO_EMPTY); > > + return ret & UART_RXFIFO_DATA; > > } > > > > static struct sbi_console_device sifive_console = { > > diff --git a/lib/utils/serial/uart8250.c > > b/lib/utils/serial/uart8250.c > > index 1cf6624..bab8111 100644 > > --- a/lib/utils/serial/uart8250.c > > +++ b/lib/utils/serial/uart8250.c > > @@ -79,9 +79,9 @@ static void uart8250_putc(char ch) > > > > static int uart8250_getc(void) > > { > > - if (get_reg(UART_LSR_OFFSET) & UART_LSR_DR) > > - return get_reg(UART_RBR_OFFSET); > > - return -1; > > + while (!(get_reg(UART_LSR_OFFSET) & UART_LSR_DR)) > > + ; > > + return get_reg(UART_RBR_OFFSET); > > } > > > > static struct sbi_console_device uart8250_console = { > > diff --git a/lib/utils/sys/htif.c b/lib/utils/sys/htif.c > > index 7c69c7f..dfda30a 100644 > > --- a/lib/utils/sys/htif.c > > +++ b/lib/utils/sys/htif.c > > @@ -47,7 +47,7 @@ > > > > volatile uint64_t tohost __attribute__((section(".htif"))); > > volatile uint64_t fromhost __attribute__((section(".htif"))); > > -static int htif_console_buf; > > +static int htif_console_buf = -1; > > static spinlock_t htif_lock = SPIN_LOCK_INITIALIZER; > > > > static void __check_fromhost() > > @@ -130,12 +130,11 @@ static int htif_getc(void) > > > > spin_lock(&htif_lock); > > > > - __check_fromhost(); > > + while (htif_console_buf < 0) > > + __check_fromhost(); > > ch = htif_console_buf; > > - if (ch >= 0) { > > - htif_console_buf = -1; > > - __set_tohost(HTIF_DEV_CONSOLE, > > HTIF_CONSOLE_CMD_GETC, 0); > > - } > > + htif_console_buf = -1; > > + __set_tohost(HTIF_DEV_CONSOLE, HTIF_CONSOLE_CMD_GETC, 0); > > > > spin_unlock(&htif_lock); > > > > -- > > 2.30.2 > > > > > > -- > > opensbi mailing list > > opensbi@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi
On 14 Nov 2021, at 00:16, Xiang W <wxjstz@126.com> wrote: > > 在 2021-11-13星期六的 22:11 +0000，Jessica Clarke写道： >> On 13 Nov 2021, at 07:06, Xiang W <wxjstz@126.com> wrote: >>> >>> If uart_getc is non-blocking, if the sending end is faster than the >>> receiving end, the data received by some functions(sbi_gets) is >>> incomplete >> >> Without hardware flow control this is always going to be true. Making >> getc blocking does not change that. Moreover, making it blocking >> means >> there is no longer a way to poll, whereas having it be non-blocking >> means you can poll, and if you want to make it blocking you can >> always >> put a loop in the caller (which also avoids putting it in every >> single >> driver). >> >> Also, this breaks the SBI getchar call; it’s supposed to be >> non-blocking, but this makes it blocking, so it is impossible for an >> OS >> to poll the SBI console. > Linux also needs blocking sbi_getc, the following is the source code in > linux > > static int hvc_sbi_tty_get(uint32_t vtermno, char *buf, int count) > { > int i, c; > > for (i = 0; i < count; i++) { > c = sbi_console_getchar(); > if (c < 0) > break; > buf[i] = c; > } > > return i; > } > > static const struct hv_ops hvc_sbi_ops = { > .get_chars = hvc_sbi_tty_get, > .put_chars = hvc_sbi_tty_put, > }; That is non-blocking. It stops as soon as it gets -1. Also, Linux is not the only OS in existence. Even if Linux did want a blocking interface, that doesn’t mean that’s what the SBI should provide. Don’t change the semantics of existing SBI calls, otherwise what’s the point of freezing specs. Jess
在 2021-11-13星期六的 22:11 +0000，Jessica Clarke写道： > On 13 Nov 2021, at 07:06, Xiang W <wxjstz@126.com> wrote: > > > > If uart_getc is non-blocking, if the sending end is faster than the > > receiving end, the data received by some functions(sbi_gets) is > > incomplete > > Without hardware flow control this is always going to be true. Making > getc blocking does not change that. Moreover, making it blocking > means > there is no longer a way to poll, whereas having it be non-blocking > means you can poll, and if you want to make it blocking you can > always > put a loop in the caller (which also avoids putting it in every > single > driver). > > Also, this breaks the SBI getchar call; it’s supposed to be > non-blocking, but this makes it blocking, so it is impossible for an > OS > to poll the SBI console. uart get does not support and returns the same value without receiving data. So polling will also cause problems int sbi_getc(void) { if (console_dev && console_dev->console_getc) // Return -1 without valid data return console_dev->console_getc(); return -1; } Regards, Xiang W > > Thus, NACK of the entire concept. > > Jess > > > Signed-off-by: Xiang W <wxjstz@126.com> > > --- > > lib/utils/serial/gaisler-uart.c | 5 ++--- > > lib/utils/serial/shakti-uart.c | 7 +++---- > > lib/utils/serial/sifive-uart.c | 9 +++++---- > > lib/utils/serial/uart8250.c | 6 +++--- > > lib/utils/sys/htif.c | 11 +++++------ > > 5 files changed, 18 insertions(+), 20 deletions(-) > > > > diff --git a/lib/utils/serial/gaisler-uart.c > > b/lib/utils/serial/gaisler-uart.c > > index 49298e9..1dc5ae0 100644 > > --- a/lib/utils/serial/gaisler-uart.c > > +++ b/lib/utils/serial/gaisler-uart.c > > @@ -51,9 +51,8 @@ static void gaisler_uart_putc(char ch) > > > > static int gaisler_uart_getc(void) > > { > > - u32 ret = get_reg(UART_REG_STATUS); > > - if (!(ret & UART_STATUS_DATAREADY)) > > - return -1; > > + while (!(get_reg(UART_REG_STATUS) & UART_STATUS_DATAREADY)) > > + ; > > return get_reg(UART_REG_DATA) & UART_DATA_DATA; > > } > > > > diff --git a/lib/utils/serial/shakti-uart.c > > b/lib/utils/serial/shakti-uart.c > > index e77a985..a5b72aa 100644 > > --- a/lib/utils/serial/shakti-uart.c > > +++ b/lib/utils/serial/shakti-uart.c > > @@ -32,10 +32,9 @@ static void shakti_uart_putc(char ch) > > > > static int shakti_uart_getc(void) > > { > > - u16 status = readw(uart_base + REG_STATUS); > > - if (status & UART_RX_FULL) > > - return readb(uart_base + REG_RX); > > - return -1; > > + while (!(readw(uart_base + REG_STATUS) & UART_RX_FULL)) > > + ; > > + return readb(uart_base + REG_RX); > > } > > > > static struct sbi_console_device shakti_console = { > > diff --git a/lib/utils/serial/sifive-uart.c > > b/lib/utils/serial/sifive-uart.c > > index 57d80fa..68a9ca7 100644 > > --- a/lib/utils/serial/sifive-uart.c > > +++ b/lib/utils/serial/sifive-uart.c > > @@ -76,10 +76,11 @@ static void sifive_uart_putc(char ch) > > > > static int sifive_uart_getc(void) > > { > > - u32 ret = get_reg(UART_REG_RXFIFO); > > - if (!(ret & UART_RXFIFO_EMPTY)) > > - return ret & UART_RXFIFO_DATA; > > - return -1; > > + u32 ret; > > + do > > + ret = get_reg(UART_REG_RXFIFO); > > + while (ret & UART_RXFIFO_EMPTY); > > + return ret & UART_RXFIFO_DATA; > > } > > > > static struct sbi_console_device sifive_console = { > > diff --git a/lib/utils/serial/uart8250.c > > b/lib/utils/serial/uart8250.c > > index 1cf6624..bab8111 100644 > > --- a/lib/utils/serial/uart8250.c > > +++ b/lib/utils/serial/uart8250.c > > @@ -79,9 +79,9 @@ static void uart8250_putc(char ch) > > > > static int uart8250_getc(void) > > { > > - if (get_reg(UART_LSR_OFFSET) & UART_LSR_DR) > > - return get_reg(UART_RBR_OFFSET); > > - return -1; > > + while (!(get_reg(UART_LSR_OFFSET) & UART_LSR_DR)) > > + ; > > + return get_reg(UART_RBR_OFFSET); > > } > > > > static struct sbi_console_device uart8250_console = { > > diff --git a/lib/utils/sys/htif.c b/lib/utils/sys/htif.c > > index 7c69c7f..dfda30a 100644 > > --- a/lib/utils/sys/htif.c > > +++ b/lib/utils/sys/htif.c > > @@ -47,7 +47,7 @@ > > > > volatile uint64_t tohost __attribute__((section(".htif"))); > > volatile uint64_t fromhost __attribute__((section(".htif"))); > > -static int htif_console_buf; > > +static int htif_console_buf = -1; > > static spinlock_t htif_lock = SPIN_LOCK_INITIALIZER; > > > > static void __check_fromhost() > > @@ -130,12 +130,11 @@ static int htif_getc(void) > > > > spin_lock(&htif_lock); > > > > - __check_fromhost(); > > + while (htif_console_buf < 0) > > + __check_fromhost(); > > ch = htif_console_buf; > > - if (ch >= 0) { > > - htif_console_buf = -1; > > - __set_tohost(HTIF_DEV_CONSOLE, > > HTIF_CONSOLE_CMD_GETC, 0); > > - } > > + htif_console_buf = -1; > > + __set_tohost(HTIF_DEV_CONSOLE, HTIF_CONSOLE_CMD_GETC, 0); > > > > spin_unlock(&htif_lock); > > > > -- > > 2.30.2 > > > > > > -- > > opensbi mailing list > > opensbi@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi > >
On 14 Nov 2021, at 00:23, Xiang W <wxjstz@126.com> wrote: > 在 2021-11-13星期六的 22:11 +0000，Jessica Clarke写道： >> On 13 Nov 2021, at 07:06, Xiang W <wxjstz@126.com> wrote: >>> >>> If uart_getc is non-blocking, if the sending end is faster than the >>> receiving end, the data received by some functions(sbi_gets) is >>> incomplete >> >> Without hardware flow control this is always going to be true. Making >> getc blocking does not change that. Moreover, making it blocking >> means >> there is no longer a way to poll, whereas having it be non-blocking >> means you can poll, and if you want to make it blocking you can >> always >> put a loop in the caller (which also avoids putting it in every >> single >> driver). >> >> Also, this breaks the SBI getchar call; it’s supposed to be >> non-blocking, but this makes it blocking, so it is impossible for an >> OS >> to poll the SBI console. > > uart get does not support and returns the same value without receiving > data. So polling will also cause problems > > int sbi_getc(void) > { > if (console_dev && console_dev->console_getc) > // Return -1 without valid data > return console_dev->console_getc(); > return -1; > } I do not understand what you are saying. There is no problem here. If there is no character available, either because there is no UART or because the UART’s receive buffer has been drained, the OS is told there is no character. What’s wrong with that? The only problem I can see is that BBL returns 0 when there is no UART and -1 when there’s a UART but no character available, so this is not a correct implementation of the legacy SBI, but probably a more sensible interface given 0 is a valid character. Jess
在 2021-11-14星期日的 00:31 +0000，Jessica Clarke写道： > On 14 Nov 2021, at 00:23, Xiang W <wxjstz@126.com> wrote: > > 在 2021-11-13星期六的 22:11 +0000，Jessica Clarke写道： > > > On 13 Nov 2021, at 07:06, Xiang W <wxjstz@126.com> wrote: > > > > > > > > If uart_getc is non-blocking, if the sending end is faster than > > > > the > > > > receiving end, the data received by some functions(sbi_gets) is > > > > incomplete > > > > > > Without hardware flow control this is always going to be true. > > > Making > > > getc blocking does not change that. Moreover, making it blocking > > > means > > > there is no longer a way to poll, whereas having it be non- > > > blocking > > > means you can poll, and if you want to make it blocking you can > > > always > > > put a loop in the caller (which also avoids putting it in every > > > single > > > driver). > > > > > > Also, this breaks the SBI getchar call; it’s supposed to be > > > non-blocking, but this makes it blocking, so it is impossible for > > > an > > > OS > > > to poll the SBI console. > > > > uart get does not support and returns the same value without > > receiving > > data. So polling will also cause problems > > > > int sbi_getc(void) > > { > > if (console_dev && console_dev->console_getc) > > // Return -1 without valid data > > return console_dev->console_getc(); > > return -1; > > } > > I do not understand what you are saying. There is no problem here. If > there is no character available, either because there is no UART or > because the UART’s receive buffer has been drained, the OS is told > there is no character. What’s wrong with that? What I mean is that the program that executes the sbi call cannot confirm whether it has not implemented console_dev->getc or has not received data Found the linux polling code, linux opened a kernel thread for polling. Therefore, no implemented is the same as unreceived data. Thk Xiang W > > The only problem I can see is that BBL returns 0 when there is no > UART > and -1 when there’s a UART but no character available, so this is not > a > correct implementation of the legacy SBI, but probably a more > sensible > interface given 0 is a valid character. > > Jess
在 2021-11-13星期六的 15:06 +0800，Xiang W写道： > If uart_getc is non-blocking, if the sending end is faster than the > receiving end, the data received by some functions(sbi_gets) is > incomplete > Revoke this patch Regards, Xiang W > Signed-off-by: Xiang W <wxjstz@126.com> > --- > lib/utils/serial/gaisler-uart.c | 5 ++--- > lib/utils/serial/shakti-uart.c | 7 +++---- > lib/utils/serial/sifive-uart.c | 9 +++++---- > lib/utils/serial/uart8250.c | 6 +++--- > lib/utils/sys/htif.c | 11 +++++------ > 5 files changed, 18 insertions(+), 20 deletions(-) > > diff --git a/lib/utils/serial/gaisler-uart.c > b/lib/utils/serial/gaisler-uart.c > index 49298e9..1dc5ae0 100644 > --- a/lib/utils/serial/gaisler-uart.c > +++ b/lib/utils/serial/gaisler-uart.c > @@ -51,9 +51,8 @@ static void gaisler_uart_putc(char ch) > > static int gaisler_uart_getc(void) > { > - u32 ret = get_reg(UART_REG_STATUS); > - if (!(ret & UART_STATUS_DATAREADY)) > - return -1; > + while (!(get_reg(UART_REG_STATUS) & UART_STATUS_DATAREADY)) > + ; > return get_reg(UART_REG_DATA) & UART_DATA_DATA; > } > > diff --git a/lib/utils/serial/shakti-uart.c > b/lib/utils/serial/shakti-uart.c > index e77a985..a5b72aa 100644 > --- a/lib/utils/serial/shakti-uart.c > +++ b/lib/utils/serial/shakti-uart.c > @@ -32,10 +32,9 @@ static void shakti_uart_putc(char ch) > > static int shakti_uart_getc(void) > { > - u16 status = readw(uart_base + REG_STATUS); > - if (status & UART_RX_FULL) > - return readb(uart_base + REG_RX); > - return -1; > + while (!(readw(uart_base + REG_STATUS) & UART_RX_FULL)) > + ; > + return readb(uart_base + REG_RX); > } > > static struct sbi_console_device shakti_console = { > diff --git a/lib/utils/serial/sifive-uart.c > b/lib/utils/serial/sifive-uart.c > index 57d80fa..68a9ca7 100644 > --- a/lib/utils/serial/sifive-uart.c > +++ b/lib/utils/serial/sifive-uart.c > @@ -76,10 +76,11 @@ static void sifive_uart_putc(char ch) > > static int sifive_uart_getc(void) > { > - u32 ret = get_reg(UART_REG_RXFIFO); > - if (!(ret & UART_RXFIFO_EMPTY)) > - return ret & UART_RXFIFO_DATA; > - return -1; > + u32 ret; > + do > + ret = get_reg(UART_REG_RXFIFO); > + while (ret & UART_RXFIFO_EMPTY); > + return ret & UART_RXFIFO_DATA; > } > > static struct sbi_console_device sifive_console = { > diff --git a/lib/utils/serial/uart8250.c > b/lib/utils/serial/uart8250.c > index 1cf6624..bab8111 100644 > --- a/lib/utils/serial/uart8250.c > +++ b/lib/utils/serial/uart8250.c > @@ -79,9 +79,9 @@ static void uart8250_putc(char ch) > > static int uart8250_getc(void) > { > - if (get_reg(UART_LSR_OFFSET) & UART_LSR_DR) > - return get_reg(UART_RBR_OFFSET); > - return -1; > + while (!(get_reg(UART_LSR_OFFSET) & UART_LSR_DR)) > + ; > + return get_reg(UART_RBR_OFFSET); > } > > static struct sbi_console_device uart8250_console = { > diff --git a/lib/utils/sys/htif.c b/lib/utils/sys/htif.c > index 7c69c7f..dfda30a 100644 > --- a/lib/utils/sys/htif.c > +++ b/lib/utils/sys/htif.c > @@ -47,7 +47,7 @@ > > volatile uint64_t tohost __attribute__((section(".htif"))); > volatile uint64_t fromhost __attribute__((section(".htif"))); > -static int htif_console_buf; > +static int htif_console_buf = -1; > static spinlock_t htif_lock = SPIN_LOCK_INITIALIZER; > > static void __check_fromhost() > @@ -130,12 +130,11 @@ static int htif_getc(void) > > spin_lock(&htif_lock); > > - __check_fromhost(); > + while (htif_console_buf < 0) > + __check_fromhost(); > ch = htif_console_buf; > - if (ch >= 0) { > - htif_console_buf = -1; > - __set_tohost(HTIF_DEV_CONSOLE, HTIF_CONSOLE_CMD_GETC, > 0); > - } > + htif_console_buf = -1; > + __set_tohost(HTIF_DEV_CONSOLE, HTIF_CONSOLE_CMD_GETC, 0); > > spin_unlock(&htif_lock); >
在 2021-11-14星期日的 00:31 +0000，Jessica Clarke写道： > On 14 Nov 2021, at 00:23, Xiang W <wxjstz@126.com> wrote: > > 在 2021-11-13星期六的 22:11 +0000，Jessica Clarke写道： > > > On 13 Nov 2021, at 07:06, Xiang W <wxjstz@126.com> wrote: > > > > > > > > If uart_getc is non-blocking, if the sending end is faster than > > > > the > > > > receiving end, the data received by some functions(sbi_gets) is > > > > incomplete > > > > > > Without hardware flow control this is always going to be true. > > > Making > > > getc blocking does not change that. Moreover, making it blocking > > > means > > > there is no longer a way to poll, whereas having it be non- > > > blocking > > > means you can poll, and if you want to make it blocking you can > > > always > > > put a loop in the caller (which also avoids putting it in every > > > single > > > driver). > > > > > > Also, this breaks the SBI getchar call; it’s supposed to be > > > non-blocking, but this makes it blocking, so it is impossible for > > > an > > > OS > > > to poll the SBI console. > > > > uart get does not support and returns the same value without > > receiving > > data. So polling will also cause problems > > > > int sbi_getc(void) > > { > > if (console_dev && console_dev->console_getc) > > // Return -1 without valid data > > return console_dev->console_getc(); > > return -1; Hi, Jess Can we return SBI_ENOTSUPP(-2) here to identify that console_dev->console_getc is not implemented. > > } > > I do not understand what you are saying. There is no problem here. If > there is no character available, either because there is no UART or > because the UART’s receive buffer has been drained, the OS is told > there is no character. What’s wrong with that? > > The only problem I can see is that BBL returns 0 when there is no > UART > and -1 when there’s a UART but no character available, so this is not > a > correct implementation of the legacy SBI, but probably a more > sensible > interface given 0 is a valid character. I think 0 is not a good idea. The value obtained by getc is 0-255, which includes 0. Regards, Xiang W > Jess > >
diff --git a/lib/utils/serial/gaisler-uart.c b/lib/utils/serial/gaisler-uart.c index 49298e9..1dc5ae0 100644 --- a/lib/utils/serial/gaisler-uart.c +++ b/lib/utils/serial/gaisler-uart.c @@ -51,9 +51,8 @@ static void gaisler_uart_putc(char ch) static int gaisler_uart_getc(void) { - u32 ret = get_reg(UART_REG_STATUS); - if (!(ret & UART_STATUS_DATAREADY)) - return -1; + while (!(get_reg(UART_REG_STATUS) & UART_STATUS_DATAREADY)) + ; return get_reg(UART_REG_DATA) & UART_DATA_DATA; } diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c index e77a985..a5b72aa 100644 --- a/lib/utils/serial/shakti-uart.c +++ b/lib/utils/serial/shakti-uart.c @@ -32,10 +32,9 @@ static void shakti_uart_putc(char ch) static int shakti_uart_getc(void) { - u16 status = readw(uart_base + REG_STATUS); - if (status & UART_RX_FULL) - return readb(uart_base + REG_RX); - return -1; + while (!(readw(uart_base + REG_STATUS) & UART_RX_FULL)) + ; + return readb(uart_base + REG_RX); } static struct sbi_console_device shakti_console = { diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c index 57d80fa..68a9ca7 100644 --- a/lib/utils/serial/sifive-uart.c +++ b/lib/utils/serial/sifive-uart.c @@ -76,10 +76,11 @@ static void sifive_uart_putc(char ch) static int sifive_uart_getc(void) { - u32 ret = get_reg(UART_REG_RXFIFO); - if (!(ret & UART_RXFIFO_EMPTY)) - return ret & UART_RXFIFO_DATA; - return -1; + u32 ret; + do + ret = get_reg(UART_REG_RXFIFO); + while (ret & UART_RXFIFO_EMPTY); + return ret & UART_RXFIFO_DATA; } static struct sbi_console_device sifive_console = { diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c index 1cf6624..bab8111 100644 --- a/lib/utils/serial/uart8250.c +++ b/lib/utils/serial/uart8250.c @@ -79,9 +79,9 @@ static void uart8250_putc(char ch) static int uart8250_getc(void) { - if (get_reg(UART_LSR_OFFSET) & UART_LSR_DR) - return get_reg(UART_RBR_OFFSET); - return -1; + while (!(get_reg(UART_LSR_OFFSET) & UART_LSR_DR)) + ; + return get_reg(UART_RBR_OFFSET); } static struct sbi_console_device uart8250_console = { diff --git a/lib/utils/sys/htif.c b/lib/utils/sys/htif.c index 7c69c7f..dfda30a 100644 --- a/lib/utils/sys/htif.c +++ b/lib/utils/sys/htif.c @@ -47,7 +47,7 @@ volatile uint64_t tohost __attribute__((section(".htif"))); volatile uint64_t fromhost __attribute__((section(".htif"))); -static int htif_console_buf; +static int htif_console_buf = -1; static spinlock_t htif_lock = SPIN_LOCK_INITIALIZER; static void __check_fromhost() @@ -130,12 +130,11 @@ static int htif_getc(void) spin_lock(&htif_lock); - __check_fromhost(); + while (htif_console_buf < 0) + __check_fromhost(); ch = htif_console_buf; - if (ch >= 0) { - htif_console_buf = -1; - __set_tohost(HTIF_DEV_CONSOLE, HTIF_CONSOLE_CMD_GETC, 0); - } + htif_console_buf = -1; + __set_tohost(HTIF_DEV_CONSOLE, HTIF_CONSOLE_CMD_GETC, 0); spin_unlock(&htif_lock);
If uart_getc is non-blocking, if the sending end is faster than the receiving end, the data received by some functions(sbi_gets) is incomplete Signed-off-by: Xiang W <wxjstz@126.com> --- lib/utils/serial/gaisler-uart.c | 5 ++--- lib/utils/serial/shakti-uart.c | 7 +++---- lib/utils/serial/sifive-uart.c | 9 +++++---- lib/utils/serial/uart8250.c | 6 +++--- lib/utils/sys/htif.c | 11 +++++------ 5 files changed, 18 insertions(+), 20 deletions(-)