On 02/04/2024 23:19, Volodymyr Babchuk wrote:
>
>
> Hi Michal,
>
> Michal Orzel <michal.or...@amd.com> 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_babc...@epam.com>
>>> ---
>>> 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_babc...@epam.com>
>>> + * 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
