Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 15/29] RiscVPkg/Library: RISC-V CPU library

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Mon, Sep 23, 2019 at 08:31:41AM +0800, Abner Chang wrote:
> This library provides CSR assembly functions to read/write RISC-V
> specific Control and Status registers.
> 
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> ---
>  RiscVPkg/Include/Library/RiscVCpuLib.h       |  68 ++++++++++++++++
>  RiscVPkg/Library/RiscVCpuLib/Cpu.S           | 115 +++++++++++++++++++++++++++
>  RiscVPkg/Library/RiscVCpuLib/RiscVCpuLib.inf |  34 ++++++++

Please ensure you have set up an orderfile, as described on
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers
or by executing BaseTools/Scripts/SetupGit.py inside each repository.

>  3 files changed, 217 insertions(+)
>  create mode 100644 RiscVPkg/Include/Library/RiscVCpuLib.h
>  create mode 100644 RiscVPkg/Library/RiscVCpuLib/Cpu.S
>  create mode 100644 RiscVPkg/Library/RiscVCpuLib/RiscVCpuLib.inf
> 
> diff --git a/RiscVPkg/Include/Library/RiscVCpuLib.h b/RiscVPkg/Include/Library/RiscVCpuLib.h
> new file mode 100644
> index 0000000..c84d599
> --- /dev/null
> +++ b/RiscVPkg/Include/Library/RiscVCpuLib.h
> @@ -0,0 +1,68 @@
> +/** @file
> +  RISC-V CPU library definitions.
> +
> +  Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef _RISCV_CPU_LIB_H_
> +#define _RISCV_CPU_LIB_H_

Please drop leading _.

> +
> +#include "RiscV.h"

Hmm. This raises two concerns.
First - style-wise, "" should not be used for anything but local (same
directory) include files.
Secondly, there are two separate files called RiscV.h introduced by
this patch series:
RiscVPkg/Include/IndustryStandard/RiscV.h
RiscVPkg/Include/RiscV.h
Have these been split solely in order to have one directly includable
in assembler?

If so, please merge them (in the IndustryStandard one), and put the
C-specific bits inside an #ifndef __ASSEMBLY__ statement.
If not, please provide a description of their logical split, and
rename one of them.

> +
> +/**
> +  RISCV_TRAP_HANDLER
> +**/
> +typedef
> +VOID
> +(EFIAPI *RISCV_TRAP_HANDLER)(
> +  VOID
> +  );
> +
> +VOID
> +RiscVSetScratch (RISCV_MACHINE_MODE_CONTEXT *RiscvContext);

Please keep all names referring to architectural registers
identifiable. A quick Internet search suggests this may mean Machine
Scratch Register?

> +
> +UINT32
> +RiscVGetScratch (VOID);
> +
> +UINT32
> +RiscVGetTrapCause (VOID);
> +
> +UINT64
> +RiscVReadMachineTimer (VOID);
> +
> +VOID
> +RiscVSetMachineTimerCmp (UINT64);

Cmp neds expanding to its full name. 

> +
> +UINT64
> +RiscVReadMachineTimerCmp(VOID);
> +
> +UINT64
> +RiscVReadMachineIE(VOID);
> +
> +UINT64
> +RiscVReadMachineIP(VOID);

IE/IP needs expanding, unless these are the names used in the
architecture reference. If it is, this file needs those terms
introduced in a glossary section at the start of this file.

> +
> +UINT64
> +RiscVReadMachineStatus(VOID);
> +
> +VOID
> +RiscVWriteMachineStatus(UINT64);
> +
> +UINT64
> +RiscVReadMachineTvec(VOID);
> +
> +UINT64
> +RiscVReadMisa (VOID);

Tvec/Misa need the same treatment as IE/IP.
If the M stands for Machine, it should be written out fully, and
likely isa should be Isa.

> +
> +UINT64
> +RiscVReadMVendorId (VOID);
> +
> +UINT64
> +RiscVReadMArchId (VOID);
> +
> +UINT64
> +RiscVReadMImplId (VOID);

Impl needs expanding - I can't tell whether that means Implementation
or Implmenter.
For all 3 above, is that M only an abbreviated Machine? If so, please
write it out fully.

> +
> +#endif
> diff --git a/RiscVPkg/Library/RiscVCpuLib/Cpu.S b/RiscVPkg/Library/RiscVCpuLib/Cpu.S
> new file mode 100644
> index 0000000..f372397
> --- /dev/null
> +++ b/RiscVPkg/Library/RiscVCpuLib/Cpu.S
> @@ -0,0 +1,115 @@
> +//------------------------------------------------------------------------------
> +//
> +// RISC-V CPU functions.
> +//
> +// Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +//
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +//------------------------------------------------------------------------------
> +#include <Base.h>
> +#include <RiscV.h>
> +
> +.data
> +
> +.text
> +.align 3
> +
> +.global ASM_PFX(RiscVSetScratch)
> +.global ASM_PFX(RiscVGetScratch)
> +.global ASM_PFX(RiscVGetMachineTrapCause)
> +.global ASM_PFX(RiscVReadMachineIE)
> +.global ASM_PFX(RiscVReadMachineIP)
> +.global ASM_PFX(RiscVReadMachineStatus)
> +.global ASM_PFX(RiscVWriteMachineStatus)
> +.global ASM_PFX(RiscVReadMachineTvec)
> +.global ASM_PFX(RiscVReadMisa)
> +.global ASM_PFX(RiscVReadMVendorId)
> +.global ASM_PFX(RiscVReadMArchId)
> +.global ASM_PFX(RiscVReadMImplId)

This could get a lot neater if you replicate what we have for the
ARM/AARCH64 ports and implement an ASM_FUNC macro that does both the
global, and the prefix (and some more stuff that is less imortant now
we use lto anyway).

Have a look in ArmPkg/Include/AsmMacroIoLib*.h

> +//
> +// Set machine mode scratch.
> +// @param a0 : Pointer to RISCV_MACHINE_MODE_CONTEXT.
> +//
> +ASM_PFX (RiscVSetScratch):
> +    csrrw a1, RISCV_CSR_MACHINE_MSCRATCH, a0
> +    ret
> +
> +//
> +// Get machine mode scratch.
> +// @retval a0 : Pointer to RISCV_MACHINE_MODE_CONTEXT.
> +//
> +ASM_PFX (RiscVGetScratch):
> +    csrrs a0, RISCV_CSR_MACHINE_MSCRATCH, 0
> +    ret
> +
> +//
> +// Get machine trap cause CSR.
> +//
> +ASM_PFX (RiscVGetMachineTrapCause):
> +    csrrs a0, RISCV_CSR_MACHINE_MCAUSE, 0
> +    ret
> +
> +//
> +// Get machine interrupt enable
> +//
> +ASM_PFX (RiscVReadMachineIE):
> +    csrr a0, RISCV_CSR_MACHINE_MIE
> +    ret
> +
> +//
> +// Get machine interrupt pending
> +//
> +ASM_PFX (RiscVReadMachineIP):
> +    csrr a0, RISCV_CSR_MACHINE_MIP
> +    ret
> +
> +//
> +// Get machine status
> +//
> +ASM_PFX(RiscVReadMachineStatus):
> +    csrr a0, RISCV_CSR_MACHINE_MSTATUS
> +    ret
> +
> +//
> +// Set machine status
> +//
> +ASM_PFX(RiscVWriteMachineStatus):
> +    csrw RISCV_CSR_MACHINE_MSTATUS, a0
> +    ret
> +
> +//
> +// Get machine trap vector
> +//
> +ASM_PFX(RiscVReadMachineTvec):
> +    csrr a0, RISCV_CSR_MACHINE_MTVEC
> +    ret
> +
> +//
> +// Read machine ISA
> +//
> +ASM_PFX(RiscVReadMisa):
> +    csrr a0, RISCV_CSR_MACHINE_MISA
> +    ret
> +
> +//
> +// Read machine vendor ID
> +//
> +ASM_PFX(RiscVReadMVendorId):
> +    csrr a0, RISCV_CSR_MACHINE_MVENDORID
> +    ret
> +
> +//
> +// Read machine architecture ID
> +//
> +ASM_PFX(RiscVReadMArchId):
> +    csrr a0, RISCV_CSR_MACHINE_MARCHID
> +    ret
> +
> +//
> +// Read machine implementation ID
> +//
> +ASM_PFX(RiscVReadMImplId):
> +    csrr a0, RISCV_CSR_MACHINE_MIMPID
> +    ret
> +
> diff --git a/RiscVPkg/Library/RiscVCpuLib/RiscVCpuLib.inf b/RiscVPkg/Library/RiscVCpuLib/RiscVCpuLib.inf
> new file mode 100644
> index 0000000..fc9131b
> --- /dev/null
> +++ b/RiscVPkg/Library/RiscVCpuLib/RiscVCpuLib.inf
> @@ -0,0 +1,34 @@
> +## @file
> +# RISC-V RV64 CPU library
> +#
> +# Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001b
> +  BASE_NAME                      = RiscVCpuLib
> +  FILE_GUID                      = 8C6CFB0D-A0EE-40D5-90DA-2E51EAE0583F
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = RiscVCpuLib
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = RISCV64
> +#
> +
> +[Sources]
> +
> +[Sources.RISCV64]
> +  Cpu.S
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec

Please sort the above two alphabetically.

/
    Leif

> +  RiscVPkg/RiscVPkg.dec
> +
> +
> -- 
> 2.7.4
> 
> 
> 
>