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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi,

On 02/04/2024 12:25, Michal Orzel wrote:
> 
> 
> Hello,
> 
> On 29/03/2024 01: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.
> Do you have a link to a manual?
> 
>>
>> 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 +++++++
> Shouldn't all the files (+ other places) have geni in their names? Could qcom 
> refer to other type of serial device?
> 
> 
>>  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
> The list is sorted alphabetically. Please adhere to that.
> 
>> +               select EARLY_UART_QCOM
>> +               bool "Early printk via Qualcomm debug UART"
> Shouldn't you add depends on ARM_64? Isn't this device Arm64 specific like in 
> Linux?
> 
>> +               help
>> +                       Say Y here if you wish the early printk to direct 
>> their
> help text here should be indented by 2 tabs and 2 spaces (do not take example 
> from surrounding code)
> 
>> +                       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
> The list is sorted alphabetically. Please adhere to that.
> 
>>
>>  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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
> No need for the license text. You should use SPDX identifier instead at the 
> top of the file:
> /* SPDX-License-Identifier: GPL-2.0-or-later */
I chatted with Julien and it would be best to use GPL-2.0-only.

~Michal