Re: [PATCH 1/2] serial: Add riscv_sbi console support
Hi,
There are a couple instances where your code is not formatted in the
correct style [1]. You can use tools/checkpatch.pl to help you fix
these.
[1] https://www.denx.de/wiki/U-Boot/CodingStyle
On 5/20/20 1:33 AM, Kongou Hikari wrote:
> - This patch supports debug serial and console from SBI syscall.
>
> Signed-off-by: Kongou Hikari
> ---
> drivers/serial/Kconfig| 17 +
> drivers/serial/Makefile | 1 +
> drivers/serial/serial_riscv_sbi.c | 104 ++
> 3 files changed, 122 insertions(+)
> create mode 100644 drivers/serial/serial_riscv_sbi.c
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 90e3983170..60dcf9bc9a 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -388,12 +388,20 @@ config DEBUG_UART_MTK
> driver will be available until the real driver model serial is
> running.
>
> +
> +config DEBUG_UART_RISCV_SBI
> +bool "RISC-V SBI CONSOLE"
> +depends on RISCV_SBI_CONSOLE
> +help
> + Select this to enable a debug UART using RISC-V SBI console driver.
> +
> endchoice
>
> config DEBUG_UART_BASE
> hex "Base address of UART"
> depends on DEBUG_UART
> default 0 if DEBUG_UART_SANDBOX
> + default 0 if DEBUG_UART_RISCV_SBI
> help
> This is the base address of your UART for memory-mapped UARTs.
>
> @@ -404,6 +412,7 @@ config DEBUG_UART_CLOCK
> int "UART input clock"
> depends on DEBUG_UART
> default 0 if DEBUG_UART_SANDBOX
> + default 0 if DEBUG_UART_RISCV_SBI
> help
> The UART input clock determines the speed of the internal UART
> circuitry. The baud rate is derived from this by dividing the input
> @@ -481,6 +490,14 @@ config ALTERA_JTAG_UART_BYPASS
> output will wait forever until a JTAG terminal is connected. If you
> not are sure, say Y.
>
> +config RISCV_SBI_CONSOLE
> + bool "RISC-V SBI console support"
> + depends on RISCV
> + help
> + This enables support for console via RISC-V SBI calls.
> +
> + If you don't know what do to here, say Y.
> +
> config ALTERA_UART
> bool "Altera UART support"
> depends on DM_SERIAL
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index e4a927..15b2a3ea6f 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_MXC_UART) += serial_mxc.o
> obj-$(CONFIG_PXA_SERIAL) += serial_pxa.o
> obj-$(CONFIG_MESON_SERIAL) += serial_meson.o
> obj-$(CONFIG_INTEL_MID_SERIAL) += serial_intel_mid.o
> +obj-$(CONFIG_RISCV_SBI_CONSOLE) += serial_riscv_sbi.o
> ifdef CONFIG_SPL_BUILD
> obj-$(CONFIG_ROCKCHIP_SERIAL) += serial_rockchip.o
> endif
> diff --git a/drivers/serial/serial_riscv_sbi.c
> b/drivers/serial/serial_riscv_sbi.c
> new file mode 100644
> index 00..add11be04e
> --- /dev/null
> +++ b/drivers/serial/serial_riscv_sbi.c
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2008 David Gibson, IBM Corporation
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2020 Nuclei System Technologies
> + * Copyright (C) 2020 Ruigang Wan
> + */
> +
> +#include
The following includes should be sorted alphabetially
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +
I believe it's conventional to put the debug uart at the end of a file.
> +#ifdef CONFIG_DEBUG_UART_RISCV_SBI
> +
This include should probably be at the top and outside of this ifdef.
> +#include
> +
> +
> +static inline void _debug_uart_init(void)
> +{
Don't use C++-style comments, except in SPDX identifiers.
> + //Nothing
> +}
> +
> +static inline void _debug_uart_putc(int ch)
> +{
> + sbi_console_putchar(ch);
> +}
> +
> +DEBUG_UART_FUNCS
> +
> +#endif
> +
> +static int sbi_tty_pending_char = -1;
> +
> +static int sbi_tty_put(struct udevice *dev, const char ch)
> +{
> +
> + sbi_console_putchar(ch);
> +
> + return 0;
> +}
> +
> +static int sbi_tty_get(struct udevice *dev)
> +{
> + int c;
There should be a blank line here.
> + if (sbi_tty_pending_char != -1)
Opening braces go on the same line as statement; the only exception are
functions. For example,
if (foo) {
} else {
}
> + {
> + c = sbi_tty_pending_char;
> + sbi_tty_pending_char = -1;
> + }
> + else
> + {
> + c = sbi_console_getchar();
> + if (c < 0)
> + return -EAGAIN;
> + }
> +
> + return c;
> +}
> +
> +static int sbi_tty_setbrg(struct udevice *dev, int baudrate)
> +{
> + return 0;
> +}
> +
> +static int sbi_tty_pending(struct udevice *dev, bool input)
> +{
> + int c;
> + if (input)
> + {
> + if (sbi_tty_pending_char != -1)
> + return 1;
> +
> + c = sbi_console_getchar();
> + if(c < 0)
> + return 0;
> +
Re: [PATCH 1/2] serial: Add riscv_sbi console support
Hi, On Wed, May 20, 2020 at 7:16 PM Kongou Hikari wrote: > > - This patch supports debug serial and console from SBI syscall. > > Signed-off-by: Kongou Hikari > --- > drivers/serial/Kconfig| 17 + > drivers/serial/Makefile | 1 + > drivers/serial/serial_riscv_sbi.c | 104 ++ > 3 files changed, 122 insertions(+) > create mode 100644 drivers/serial/serial_riscv_sbi.c > The SBI calls sbi_console_getchar & sbi_console_putchar are marked as legacy calls, and in U-Boot we default to use SBI v0.2 There is not a big value of adding that to the mainline IMHO. Regards, Bin
[PATCH 1/2] serial: Add riscv_sbi console support
- This patch supports debug serial and console from SBI syscall.
Signed-off-by: Kongou Hikari
---
drivers/serial/Kconfig| 17 +
drivers/serial/Makefile | 1 +
drivers/serial/serial_riscv_sbi.c | 104 ++
3 files changed, 122 insertions(+)
create mode 100644 drivers/serial/serial_riscv_sbi.c
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 90e3983170..60dcf9bc9a 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -388,12 +388,20 @@ config DEBUG_UART_MTK
driver will be available until the real driver model serial is
running.
+
+config DEBUG_UART_RISCV_SBI
+bool "RISC-V SBI CONSOLE"
+depends on RISCV_SBI_CONSOLE
+help
+ Select this to enable a debug UART using RISC-V SBI console driver.
+
endchoice
config DEBUG_UART_BASE
hex "Base address of UART"
depends on DEBUG_UART
default 0 if DEBUG_UART_SANDBOX
+ default 0 if DEBUG_UART_RISCV_SBI
help
This is the base address of your UART for memory-mapped UARTs.
@@ -404,6 +412,7 @@ config DEBUG_UART_CLOCK
int "UART input clock"
depends on DEBUG_UART
default 0 if DEBUG_UART_SANDBOX
+ default 0 if DEBUG_UART_RISCV_SBI
help
The UART input clock determines the speed of the internal UART
circuitry. The baud rate is derived from this by dividing the input
@@ -481,6 +490,14 @@ config ALTERA_JTAG_UART_BYPASS
output will wait forever until a JTAG terminal is connected. If you
not are sure, say Y.
+config RISCV_SBI_CONSOLE
+ bool "RISC-V SBI console support"
+ depends on RISCV
+ help
+ This enables support for console via RISC-V SBI calls.
+
+ If you don't know what do to here, say Y.
+
config ALTERA_UART
bool "Altera UART support"
depends on DM_SERIAL
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index e4a927..15b2a3ea6f 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_MXC_UART) += serial_mxc.o
obj-$(CONFIG_PXA_SERIAL) += serial_pxa.o
obj-$(CONFIG_MESON_SERIAL) += serial_meson.o
obj-$(CONFIG_INTEL_MID_SERIAL) += serial_intel_mid.o
+obj-$(CONFIG_RISCV_SBI_CONSOLE) += serial_riscv_sbi.o
ifdef CONFIG_SPL_BUILD
obj-$(CONFIG_ROCKCHIP_SERIAL) += serial_rockchip.o
endif
diff --git a/drivers/serial/serial_riscv_sbi.c
b/drivers/serial/serial_riscv_sbi.c
new file mode 100644
index 00..add11be04e
--- /dev/null
+++ b/drivers/serial/serial_riscv_sbi.c
@@ -0,0 +1,104 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2008 David Gibson, IBM Corporation
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2020 Nuclei System Technologies
+ * Copyright (C) 2020 Ruigang Wan
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+
+#ifdef CONFIG_DEBUG_UART_RISCV_SBI
+
+#include
+
+
+static inline void _debug_uart_init(void)
+{
+ //Nothing
+}
+
+static inline void _debug_uart_putc(int ch)
+{
+ sbi_console_putchar(ch);
+}
+
+DEBUG_UART_FUNCS
+
+#endif
+
+static int sbi_tty_pending_char = -1;
+
+static int sbi_tty_put(struct udevice *dev, const char ch)
+{
+
+ sbi_console_putchar(ch);
+
+ return 0;
+}
+
+static int sbi_tty_get(struct udevice *dev)
+{
+ int c;
+ if (sbi_tty_pending_char != -1)
+ {
+ c = sbi_tty_pending_char;
+ sbi_tty_pending_char = -1;
+ }
+ else
+ {
+ c = sbi_console_getchar();
+ if (c < 0)
+ return -EAGAIN;
+ }
+
+ return c;
+}
+
+static int sbi_tty_setbrg(struct udevice *dev, int baudrate)
+{
+ return 0;
+}
+
+static int sbi_tty_pending(struct udevice *dev, bool input)
+{
+ int c;
+ if (input)
+ {
+ if (sbi_tty_pending_char != -1)
+ return 1;
+
+ c = sbi_console_getchar();
+ if(c < 0)
+ return 0;
+ sbi_tty_pending_char = c;
+ return 1;
+ }
+ return 0;
+}
+
+static const struct udevice_id serial_riscv_sbi_ids[] = {
+ { .compatible = "sbi,console" },
+ { }
+};
+
+const struct dm_serial_ops serial_riscv_sbi_ops = {
+ .putc = sbi_tty_put,
+ .pending = sbi_tty_pending,
+ .getc = sbi_tty_get,
+ .setbrg = sbi_tty_setbrg,
+};
+
+U_BOOT_DRIVER(serial_riscv_sbi) = {
+ .name = "serial_riscv_sbi",
+ .id = UCLASS_SERIAL,
+ .of_match = serial_riscv_sbi_ids,
+ .ops= _riscv_sbi_ops,
+};
--
2.17.1
