Re: [PATCH 2/3] drivers: serial: add Qualcomm GENI-based serial driver

[Julien Grall <julien@xxxxxxx>]

Hi,

On 29/03/2024 00:08, Volodymyr Babchuk wrote:
> Generic Interface (GENI) is a newer interface for low speed interfaces
> like UART, I2C or SPI. This patch adds the simple driver for the UART
> instance of GENI. Code is based on similar drivers in U-Boot and Linux
> kernel.
If this is based on Linux/U-boot, then I assume you read the code and 
possibly copy/paste some of it. Therefore...
> This driver implements only simple synchronous mode, because although
> GENI supports FIFO mode, it needs to know number of
> characters **before** starting TX transaction. This is a stark
> contrast when compared to other UART peripherals, which allow adding
> characters to a FIFO while TX operation is running.
>
> The patch adds both normal UART driver and earlyprintk version.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
>   xen/arch/arm/Kconfig.debug           |  19 +-
>   xen/arch/arm/arm64/debug-qcom.inc    |  76 +++++++
>   xen/arch/arm/include/asm/qcom-uart.h |  48 +++++
>   xen/drivers/char/Kconfig             |   8 +
>   xen/drivers/char/Makefile            |   1 +
>   xen/drivers/char/qcom-uart.c         | 288 +++++++++++++++++++++++++++
>   6 files changed, 439 insertions(+), 1 deletion(-)
>   create mode 100644 xen/arch/arm/arm64/debug-qcom.inc
>   create mode 100644 xen/arch/arm/include/asm/qcom-uart.h
>   create mode 100644 xen/drivers/char/qcom-uart.c
>
> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> index eec860e88e..f6ab0bb30e 100644
> --- a/xen/arch/arm/Kconfig.debug
> +++ b/xen/arch/arm/Kconfig.debug
> @@ -119,6 +119,19 @@ choice
>                         selecting one of the platform specific options below if
>                         you know the parameters for the port.
>   
> +			This option is preferred over the platform specific
> +                       options; the platform specific options are deprecated
> +                       and will soon be removed.
> +       config EARLY_UART_CHOICE_QCOM
> +               select EARLY_UART_QCOM
> +               bool "Early printk via Qualcomm debug UART"
> +               help
> +                       Say Y here if you wish the early printk to direct their
> +                       output to a Qualcomm debug UART. You can use this 
> option to
> +                       provide the parameters for the debug UART rather than
> +                       selecting one of the platform specific options below if
> +                       you know the parameters for the port.
> +
>                         This option is preferred over the platform specific
>                         options; the platform specific options are deprecated
>                         and will soon be removed.
> @@ -211,6 +224,9 @@ config EARLY_UART_PL011
>   config EARLY_UART_SCIF
>         select EARLY_PRINTK
>         bool
> +config EARLY_UART_QCOM
> +       select EARLY_PRINTK
> +       bool
>   
>   config EARLY_PRINTK
>         bool
> @@ -261,7 +277,7 @@ config EARLY_UART_PL011_MMIO32
>           will be done using 32-bit only accessors.
>   
>   config EARLY_UART_INIT
> -       depends on EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0
> +       depends on (EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0) || 
> EARLY_UART_QCOM
>         def_bool y
>   
>   config EARLY_UART_8250_REG_SHIFT
> @@ -308,3 +324,4 @@ config EARLY_PRINTK_INC
>         default "debug-mvebu.inc" if EARLY_UART_MVEBU
>         default "debug-pl011.inc" if EARLY_UART_PL011
>         default "debug-scif.inc" if EARLY_UART_SCIF
> +       default "debug-qcom.inc" if EARLY_UART_QCOM
> diff --git a/xen/arch/arm/arm64/debug-qcom.inc 
> b/xen/arch/arm/arm64/debug-qcom.inc
> new file mode 100644
> index 0000000000..130d08d6e9
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-qcom.inc
> @@ -0,0 +1,76 @@
> +/*
> + * xen/arch/arm/arm64/debug-qcom.inc
> + *
> + * Qualcomm debug UART specific debug code
> + *
> + * Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> + * Copyright (C) 2024, EPAM Systems.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
... I don't think you can use GPLv2+ license here. It has to be the same 
as Linux/U-boot.
Also, as there is no public manual, can you provide some pointer to the 
code from both repo (including version)? This would help the reviewer to 
confirm you code.
Cheers,

--
Julien Grall