[PATCH 1/2] serial: Add riscv_sbi console support

[Sean Anderson]

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 <hikari at nucleisys.com>
> ---
>  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 e4a92bbbb7..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 0000000000..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 <rgwan at nucleisys.com>
> + */
> +
> +#include <common.h>

The following includes should be sorted alphabetially

> +#include <serial.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +
> +#include <asm/sbi.h>
> +
> +

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 <debug_uart.h>
> +
> +
> +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;
> +		sbi_tty_pending_char = c;
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +static const struct udevice_id serial_riscv_sbi_ids[] = {
> +	{ .compatible = "sbi,console" },
> +	{ }
> +};
> +

This should also be static.

> +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	= &serial_riscv_sbi_ops,
> +};
> 

--Sean