Re: [edk2] [PATCH 2/2] MdePkg: MDE implementations for RISC-V arch.
Sounds good. Thanks!
> Hi Jiewen,
> Below is my answer
>
> >>>1) BaseCacheMaintenanceLib - It seems you just leave empty function
> there, without any implementation.
> >>>If there is no need to implement this, would you please add some
> comments around the code?
> I will have comment on this.
>
> >>>2) IoLib - It seems MMIO and IO is treated to be same. Is that true?
> >>>If there is no special IO instruction in RISC-V, using ASSERT might be
> better. You can take ARM as example.
> Actually, RISC-V only support MMIO, no I/O instruction. However, I use
> PC/AT architecture as RISC-V platform spec. Therefore, I/O access is required
> for those PC/AT component(drivers). I will have comment in I/O lib.
>
> >>>3) BaseSynchronizationLib - It seems you use C-code to perform atomic
> memory read-modify-write. Will the compiler generic atomic memory
> operation? If >>>yes, would you please add some comments around the
> c-code?
> >>>If no, I suggest using AMO instruction in assembly code.
> I don't think compiler will generate AMO instruction. I missed this one, I
> will
> revise it.
>
> Thanks for your comment.
> Abner
>
> -Original Message-
> From: Yao, Jiewen [mailto:jiewen@intel.com]
> Sent: Monday, May 09, 2016 10:04 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.ch...@hpe.com>;
> edk2-devel@lists.01.org
> Cc: AbnerChang <abner.ch...@hp.com>; Gao, Liming
> <liming@intel.com>
> Subject: RE: [edk2] [PATCH 2/2] MdePkg: MDE implementations for RISC-V
> arch.
>
> Hi Abner
> I have a quick review on that and I have question for below library/APIs:
>
> 1) BaseCacheMaintenanceLib - It seems you just leave empty function there,
> without any implementation.
> If there is no need to implement this, would you please add some comments
> around the code?
>
> 2) IoLib - It seems MMIO and IO is treated to be same. Is that true?
> If there is no special IO instruction in RISC-V, using ASSERT might be better.
> You can take ARM as example.
>
> 3) BaseSynchronizationLib - It seems you use C-code to perform atomic
> memory read-modify-write. Will the compiler generic atomic memory
> operation? If yes, would you please add some comments around the c-code?
> If no, I suggest using AMO instruction in assembly code.
>
> UINT32
> EFIAPI
> InternalSyncIncrement (
> IN volatile UINT32 *Value
> )
> {
> return ++*Value;
> }
>
> Thank you
> Yao Jiewen
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Abner Chang
> > Sent: Sunday, May 8, 2016 12:17 PM
> > To: edk2-devel@lists.01.org
> > Cc: AbnerChang <abner.ch...@hp.com>; Gao, Liming
> > <liming@intel.com>
> > Subject: [edk2] [PATCH 2/2] MdePkg: MDE implementations for RISC-V
> arch.
> >
> > From: AbnerChang <abner.ch...@hp.com>
> >
> > The library instances of RISC-V MDE implementations.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> >
> > Signed-off-by: Abner Chang<abner.ch...@hpe.com>
> >
> > ---
> > .../BaseCacheMaintenanceLib.inf| 4 +
> > .../Library/BaseCacheMaintenanceLib/RiscVCache.c | 228
> +++
> > MdePkg/Library/BaseCpuLib/BaseCpuLib.inf | 4 +
> > MdePkg/Library/BaseCpuLib/RiscV/Cpu.s | 25 ++
> > .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf | 8 +-
> > MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c | 418
> > +
> > MdePkg/Library/BaseLib/BaseLib.inf | 14 +
> > MdePkg/Library/BaseLib/RiscV64/CpuBreakpoint.c | 33 ++
> > MdePkg/Library/BaseLib/RiscV64/CpuPause.c | 35 ++
> > MdePkg/Library/BaseLib/RiscV64/DisableInterrupts.c | 30 ++
> > MdePkg/Library/BaseLib/RiscV64/EnableInterrupts.c | 31 ++
> > MdePkg/Library/BaseLib/RiscV64/GetInterruptState.c | 42 +++
> > .../Library/BaseLib/RiscV64/InternalSwitchStack.c | 61 +++
> > MdePkg/Library/BaseLib/RiscV64/LongJump.c | 38 ++
> > .../Library/BaseLib/RiscV64/RiscVCpuBreakpoint.S | 20 +
> > MdePkg/Library/BaseLib/RiscV64/RiscVCpuPause.S | 20 +
> > MdePkg/Library/BaseLib/RiscV64/RiscVInterrupt.S| 33 ++
> > .../Library/BaseLib/RiscV64/RiscVSetJumpLongJump.S | 63
> > MdePkg/Library/BaseLib/RiscV64/Unaligned.c | 270
> > +
> > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 3 +-
> > MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf | 5 +
> &
Re: [edk2] [PATCH 2/2] MdePkg: MDE implementations for RISC-V arch.
Hi Abner
I have a quick review on that and I have question for below library/APIs:
1) BaseCacheMaintenanceLib - It seems you just leave empty function there,
without any implementation.
If there is no need to implement this, would you please add some comments
around the code?
2) IoLib - It seems MMIO and IO is treated to be same. Is that true?
If there is no special IO instruction in RISC-V, using ASSERT might be better.
You can take ARM as example.
3) BaseSynchronizationLib - It seems you use C-code to perform atomic memory
read-modify-write. Will the compiler generic atomic memory operation? If yes,
would you please add some comments around the c-code?
If no, I suggest using AMO instruction in assembly code.
UINT32
EFIAPI
InternalSyncIncrement (
IN volatile UINT32 *Value
)
{
return ++*Value;
}
Thank you
Yao Jiewen
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Abner Chang
> Sent: Sunday, May 8, 2016 12:17 PM
> To: edk2-devel@lists.01.org
> Cc: AbnerChang <abner.ch...@hp.com>; Gao, Liming
> <liming....@intel.com>
> Subject: [edk2] [PATCH 2/2] MdePkg: MDE implementations for RISC-V arch.
>
> From: AbnerChang <abner.ch...@hp.com>
>
> The library instances of RISC-V MDE implementations.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
>
> Signed-off-by: Abner Chang<abner.ch...@hpe.com>
>
> ---
> .../BaseCacheMaintenanceLib.inf| 4 +
> .../Library/BaseCacheMaintenanceLib/RiscVCache.c | 228 +++
> MdePkg/Library/BaseCpuLib/BaseCpuLib.inf | 4 +
> MdePkg/Library/BaseCpuLib/RiscV/Cpu.s | 25 ++
> .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf | 8 +-
> MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c | 418
> +
> MdePkg/Library/BaseLib/BaseLib.inf | 14 +
> MdePkg/Library/BaseLib/RiscV64/CpuBreakpoint.c | 33 ++
> MdePkg/Library/BaseLib/RiscV64/CpuPause.c | 35 ++
> MdePkg/Library/BaseLib/RiscV64/DisableInterrupts.c | 30 ++
> MdePkg/Library/BaseLib/RiscV64/EnableInterrupts.c | 31 ++
> MdePkg/Library/BaseLib/RiscV64/GetInterruptState.c | 42 +++
> .../Library/BaseLib/RiscV64/InternalSwitchStack.c | 61 +++
> MdePkg/Library/BaseLib/RiscV64/LongJump.c | 38 ++
> .../Library/BaseLib/RiscV64/RiscVCpuBreakpoint.S | 20 +
> MdePkg/Library/BaseLib/RiscV64/RiscVCpuPause.S | 20 +
> MdePkg/Library/BaseLib/RiscV64/RiscVInterrupt.S| 33 ++
> .../Library/BaseLib/RiscV64/RiscVSetJumpLongJump.S | 63
> MdePkg/Library/BaseLib/RiscV64/Unaligned.c | 270
> +
> MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 3 +-
> MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf | 5 +
> MdePkg/Library/BasePeCoffLib/BasePeCoffLib.uni | 2 +
> .../Library/BasePeCoffLib/BasePeCoffLibInternals.h | 1 +
> .../Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c | 161
> .../BaseSynchronizationLib.inf | 5 +
> .../RiscV64/Synchronization.c | 147
> 26 files changed, 1698 insertions(+), 3 deletions(-)
> create mode 100644
> MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> create mode 100644 MdePkg/Library/BaseCpuLib/RiscV/Cpu.s
> create mode 100644 MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c
> create mode 100644 MdePkg/Library/BaseLib/RiscV64/CpuBreakpoint.c
> create mode 100644 MdePkg/Library/BaseLib/RiscV64/CpuPause.c
> create mode 100644 MdePkg/Library/BaseLib/RiscV64/DisableInterrupts.c
> create mode 100644 MdePkg/Library/BaseLib/RiscV64/EnableInterrupts.c
> create mode 100644 MdePkg/Library/BaseLib/RiscV64/GetInterruptState.c
> create mode 100644
> MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> create mode 100644 MdePkg/Library/BaseLib/RiscV64/LongJump.c
> create mode 100644
> MdePkg/Library/BaseLib/RiscV64/RiscVCpuBreakpoint.S
> create mode 100644 MdePkg/Library/BaseLib/RiscV64/RiscVCpuPause.S
> create mode 100644 MdePkg/Library/BaseLib/RiscV64/RiscVInterrupt.S
> create mode 100644
> MdePkg/Library/BaseLib/RiscV64/RiscVSetJumpLongJump.S
> create mode 100644 MdePkg/Library/BaseLib/RiscV64/Unaligned.c
> create mode 100644
> MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c
> create mode 100644
> MdePkg/Library/BaseSynchronizationLib/RiscV64/Synchronization.c
>
> diff --git
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.in
> f
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.in
> f
> index d659161..dcb6043 100644
> ---
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.in
> f
> +++
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMai
