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

["Abner Chang" <abner.chang@hpe.com>]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Tuesday, October 1, 2019 2:31 AM
> To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>
> Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 15/29]
> RiscVPkg/Library: RISC-V CPU library
> 
> 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.
I have same concerns of the same naming as well. 
Actually one is for RISC-V industrial standard, another one is the definitions of EDK2 RISC-V implementation. I will name the last one to RiscVImpl.h.

> 
> > +
> > +/**
> > +  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
> >
> >
> > 
> >