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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 02/04/2024 23:19, Volodymyr Babchuk wrote:
> 
> 
> Hi Michal,
> 
> Michal Orzel <michal.orzel@xxxxxxx> writes:
> 
>> 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?
> 
> I wish I had. Qualcomm provides HW manuals only under very strict
> NDAs. At the time of writing I don't have access to the manual at
> all. Those patches are based solely on similar drivers in U-Boot and
> Linux kernel.
> 
>>>
>>> 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?
> 
> AFAIK, there is an older type of serial device. Both Linux and U-Boot
> use "msm" instead of "qcom" in drivers names for those devices.
> 
> But I can add "geni" to the names, no big deal.
> 
>>
>>>  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.
>>
> 
> Sure
> 
>>> +               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?
>>
> 
> In linux it depends on ARCH_QCOM which can be enabled both for arm and
> arm64. But for Xen case... I believe it is better to make ARM_64
> dependency.
> 
>>> +               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)
>>
> 
> Would anyone mind if I'll send patch that aligns surrounding code as well?
> 
>>> +                       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 */
>>
>>> + */
>>> +
>>> +#include <asm/qcom-uart.h>
>>> +
>>> +.macro early_uart_init xb c
>> Separate macro parameters with comma (here and elsewhere) and please add a 
>> comment on top clarifying the use
>> Also, do we really need to initialize uart every time? What if firmware does 
>> that?
>>
> 
> You see, this code is working inside-out. early printk code in Xen is
> written with assumption that early_uart_transmit don't need a scratch
> register. But this is not true for GENI... To send one character we
> must write two registers beforehand: TX_TRANS_LEN and CMD0. Only after
> that we can write something to FIFO. But early_uart_transmit has no
> scratch register to prepare values for TX_TRANS_LEN and CMD0. So we
> write what we do here
> 
> 1. Reset the state machine with ABORT command
> 2. Write 1 to TX_TRANS_LEN
> 3. Write START_TX to CMD0
> 
> Now early_uart_transmit can write "wt" to the FIFO and after that it can
> use "wt" as a scratch register to repeat steps 2 and 3.
> 
> Probably I need this as a comment to into the .inc file...
Yes, please add a comment so that a person reading a code can understand the 
rationale.

[...]

>> What about vUART? You don't seem to set vuart information that is used by 
>> vuart.c and vpl011.c
>>
> 
> I am not sure that it is possible to emulate GENI UART with simple vuart
> API. vuart makes assumption that there is one simple status register and
> FIFO register operates on per-character basis, while even earlycon part
> of the driver in Linux tries to pack 4 characters to one FIFO write.
Ok. It might make sense to mention this in commit msg (no vuart, no vpl011 for 
direct mapped domains)

> 
> Thank you for the review. I'll address all other comments as you
> suggested.
> 
> --
> WBR, Volodymyr

~Michal