Re: [PATCH] docs: fusa: Add requirements for emulated uart

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 29 Aug 2024, at 15:33, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> wrote:
> 
> From: Michal Orzel <michal.orzel@xxxxxxx>
> 
> Add the requirements for emulated SBSA UART.
> 
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> .../fusa/reqs/design-reqs/arm64/sbsa-uart.rst | 224 ++++++++++++++++++
> docs/fusa/reqs/market-reqs/reqs.rst           |  31 +++
> docs/fusa/reqs/product-reqs/arm64/reqs.rst    |  21 ++
> 3 files changed, 276 insertions(+)
> create mode 100644 docs/fusa/reqs/design-reqs/arm64/sbsa-uart.rst
> 
> diff --git a/docs/fusa/reqs/design-reqs/arm64/sbsa-uart.rst 
> b/docs/fusa/reqs/design-reqs/arm64/sbsa-uart.rst
> new file mode 100644
> index 0000000000..aac3facce6
> --- /dev/null
> +++ b/docs/fusa/reqs/design-reqs/arm64/sbsa-uart.rst
> @@ -0,0 +1,224 @@
> +.. SPDX-License-Identifier: CC-BY-4.0
> +
> +SBSA UART
> +=========
> +
> +The following are the requirements related to SBSA UART [1] emulated and
> +exposed by Xen to Arm64 domains.
> +
> +Probe the UART device tree node from a domain
> +---------------------------------------------
> +
> +`XenSwdgn~arm64_uart_probe_dt~1`
> +
> +Description:
> +Xen shall generate a device tree node for the SBSA UART (in accordance to Arm
> +SBSA UART device tree binding [2]) in the domain device tree.
> +
> +Rationale:
> +
> +Comments:
> +Domains can detect the presence of the SBSA UART device tree node.
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Transmit data in software polling mode
> +--------------------------------------
> +
> +`XenSwdgn~arm64_uart_transmit_data_poll_mode~1`
> +
> +Description:
> +Xen shall enable transmission of data in polling mode for domains.

I would use support instead of enable here and remove "for domains" as it is 
implicit.
This corresponds to the view of features from the guest side of things and 
enable tends
to make the reader think that this relates to something to enable in hardware.

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Transmit data in interrupt driven mode
> +--------------------------------------
> +
> +`XenSwdgn~arm64_uart_transmit_data_interrupt_mode~1`
> +
> +Description:
> +Xen shall enable transmission of data in interrupt driven mode for domains.

Ditto

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Receive data in software polling mode
> +-------------------------------------
> +
> +`XenSwdgn~arm64_uart_receive_data_polling_mode~1`
> +
> +Description:
> +Xen shall enable reception of data in polling mode for domains.

Ditto

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Receive data in interrupt driven mode
> +-------------------------------------
> +
> +`XenSwdgn~arm64_uart_receive_data_interrupt_mode~1`
> +
> +Description:
> +Xen shall enable reception of data in interrupt driven mode for domains.

Ditto

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Access UART data register
> +-------------------------
> +
> +`XenSwdgn~arm64_uart_access_data_register~1`
> +
> +Description:
> +Xen shall expose the UARTDR register to the domains.

I am wondering a bit if exposing is right here. You do not
expose the hardware register, you emulate the register to
provide the functionalities equivalent to the hardware.

Maybe something like:
Xen shall emulate the UARTDR register to the domains to
provide the same features than the real SBSA register.

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Access UART receive status register
> +-----------------------------------
> +
> +`XenSwdgn~arm64_uart_access_receive_status_register~1`
> +
> +Description:
> +Xen shall expose the UARTRSR register to the domains.
DItto

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Access UART flag register
> +-------------------------
> +
> +`XenSwdgn~arm64_uart_access_flag_register~1`
> +
> +Description:
> +Xen shall expose the UARTFR register to the domains.
Dittor
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Access UART mask set/clear register
> +-----------------------------------
> +
> +`XenSwdgn~arm64_uart_access_mask_register~1`
> +
> +Description:
> +Xen shall expose the UARTIMSC register to the domains.
Ditto
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Access UART raw interrupt status register
> +-----------------------------------------
> +
> +`XenSwdgn~arm64_uart_access_raw_interrupt_status_register~1`
> +
> +Description:
> +Xen shall expose the UARTRIS register to the domains.
Ditto
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Access UART masked interrupt status register
> +--------------------------------------------
> +
> +`XenSwdgn~arm64_uart_access_mask_irq_status_register~1`
> +
> +Description:
> +Xen shall expose the UARTMIS register to the domains.
Ditto
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Access UART interrupt clear register
> +------------------------------------
> +
> +`XenSwdgn~arm64_uart_access_irq_clear_register~1`
> +
> +Description:
> +Xen shall expose the UARTICR register to the domains.
Ditto
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Receive UART TX interrupt
> +-------------------------
> +
> +`XenSwdgn~arm64_uart_receive_tx_irq~1`
> +
> +Description:
> +Xen shall generate UART TX interrupt when the UART transmit interrupt 
> condition
> +is met.

Correct me if I'm wrong but there is one interrupt generated and then a status 
register
in which you get what was the reason ?
Here I would make it more generic and point to SBSA behaviour and let the 
details
to the design reqs.

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +Receive UART RX interrupt reception
> +-----------------------------------
> +
> +`XenSwdgn~arm64_uart_receive_rx_irq~1`
> +
> +Description:
> +Xen shall generate UART RX interrupt when the UART receive interrupt 
> condition
> +is met.

DItto

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~emulated_uart~1`
> +
> +[1] Arm Base System Architecture, chapter B
> +[2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> \ No newline at end of file
> diff --git a/docs/fusa/reqs/market-reqs/reqs.rst 
> b/docs/fusa/reqs/market-reqs/reqs.rst
> index 9c98c84a9a..b69699e5fb 100644
> --- a/docs/fusa/reqs/market-reqs/reqs.rst
> +++ b/docs/fusa/reqs/market-reqs/reqs.rst
> @@ -15,6 +15,22 @@ Rationale:
> 
> Comments:
> 
> +Needs:
> + - XenProd
> +
> +Run virtualization unaware VMs
> +------------------------------
> +
> +`XenMkt~run_virtualization_unaware_vms~1`
> +
> +Description:
> +Xen shall run VMs which haven't been modified for Xen.

As discussed during Fusa meeting, shall be removed.

> +
> +Rationale:
> +Any kernel/RTOS can run as a VM on top of Xen.
> +
> +Comments:
> +
> Needs:
>  - XenProd
> 
> @@ -32,3 +48,18 @@ Comments:
> 
> Needs:
>  - XenProd
> +
> +Provide console to the VMs
> +--------------------------
> +
> +`XenMkt~provide_console_vms~1`
> +
> +Description:
> +Xen shall provide a console to a VM.
> +
> +Rationale:
> +
> +Comments:
> +
> +Needs:
> + - XenProd
> \ No newline at end of file
> diff --git a/docs/fusa/reqs/product-reqs/arm64/reqs.rst 
> b/docs/fusa/reqs/product-reqs/arm64/reqs.rst
> index 7aa3eeab6a..e90f275148 100644
> --- a/docs/fusa/reqs/product-reqs/arm64/reqs.rst
> +++ b/docs/fusa/reqs/product-reqs/arm64/reqs.rst
> @@ -17,7 +17,28 @@ Comments:
> 
> Covers:
>  - `XenMkt~run_arm64_vms~1`
> + - `XenMkt~run_virtualization_unaware_vms~1`
>  - `XenMkt~provide_timer_vms~1`
> 
> Needs:
>  - XenSwdgn
> +
> +Emulated UART
> +-------------
> +
> +`XenProd~emulated_uart~1`
> +
> +Description:
> +Xen shall grant access to "Arm SBSA UART" for the domains.

We do not grant access to the real hardware here, the sentence is misleading.

Xen shall provide an Arm SBSA UART compliant device to the domains ?
The design is then detailing how this is achieved.

Cheers
Bertrand


> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenMkt~run_arm64_vms~1`
> + - `XenMkt~run_virtualization_unaware_vms~1`
> + - `XenMkt~provide_console_vms~1`
> +
> +Needs:
> + - XenSwdgn
> \ No newline at end of file
> -- 
> 2.25.1
>