> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Tuesday, October 1, 2019 2:31 AM
> To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> <abner.ch...@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.ch...@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
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48970): https://edk2.groups.io/g/devel/message/48970
Mute This Topic: https://groups.io/mt/34258212/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
