public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Leif Lindholm" <leif.lindholm@linaro.org>
To: "Chang, Abner (HPS SW/FW Technologist)" <abner.chang@hpe.com>
Cc: "devel@edk2.groups.io" <devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 02/22]: RiscVPkg/Include: Add header files of RISC-V CPU package
Date: Tue, 17 Sep 2019 14:54:06 +0100	[thread overview]
Message-ID: <20190917135406.GE28454@bivouac.eciton.net> (raw)
In-Reply-To: <CS1PR8401MB1192712B467608B742D3F565FF8C0@CS1PR8401MB1192.NAMPRD84.PROD.OUTLOOK.COM>

On Mon, Sep 16, 2019 at 04:02:10AM +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Leif Lindholm
> > Sent: Thursday, September 5, 2019 2:56 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 v1 02/22]:
> > RiscVPkg/Include: Add header files of RISC-V CPU package
> > 
> > On Wed, Sep 04, 2019 at 06:42:57PM +0800, Abner Chang wrote:
> > > RISC-V package library definitions.
> > >
> > > RiscV.h
> > > -Add RiscV.h which conform with RISC-V Privilege Spec v1.10.
> > >
> > > sbi.h
> > > sbi_bits.h
> > > sbi_types.h
> > > - Add definitions for RISC-V OpenSBI EDK2 port.
> > 
> > A web search suggests this refers to the RISC-V Open Source Supervisor
> > Binary Interface. It would be helpful to expand it on first use.
> > https://github.com/riscv/opensbi/?
> > Is this expected to fluctuate much?
>
> Yes it does change often, the community keeps adding new features to openSBI.

OK. I got some more intro to this at Linux Plumbers Conference last week.

> > I ask for two reasons:
> > 1) Because if it is not, I would much prefer to see the
> >    files/directories renamed to conform the the coding style.
> >    If it is, I would like for us to consider implementing this as a
> >    git submodule instead.
>
> Yes. Please use submodule. Don't touch the open source from openSBI to avoid maintenance effort to edk2.

Sounds good.

...

> > > diff --git a/RiscVPkg/Include/sbi/sbi_bits.h
> > b/RiscVPkg/Include/sbi/sbi_bits.h
> > > new file mode 100644
> > > index 0000000..4116ee6
> > > --- /dev/null
> > > +++ b/RiscVPkg/Include/sbi/sbi_bits.h
> > > @@ -0,0 +1,23 @@
> > > +/** @file
> > > +  RISC-V OpesbSBI header file reference.
> > > +
> > > +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights
> > reserved.<BR>
> > > +
> > > +  This program and the accompanying materials
> > > +  are licensed and made available under the terms and conditions of the
> > BSD License
> > > +  which accompanies this distribution.  The full text of the license may be
> > found at
> > > +  INVALID URI REMOVED
> > 3A__opensource.org_licenses_bsd-
> > 2Dlicense.php&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN4V
> > gi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=iwfkW8MQjzEkixp0gv3xsvh20ei
> > odo7hGcTLXEL_I0o&s=mLKjYgrdQ6MuAN9UVYQeCDB0pNA44m9yBOylxW-
> > Koiw&e=
> > > +
> > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > BASIS,
> > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> > EXPRESS OR IMPLIED.
> > > +
> > > +**/
> > > +#ifndef __EDK2_SBI_BITS_H__
> > > +#define __EDK2_SBI_BITS_H__
> > > +
> > > +#undef MAX
> > > +#undef MIN
> > 
> > Why?
> OpebSBI sbi_bits.h has its own MAX/MIN definitions which are
> duplicated with edk2 ones. OpenSBI is the implementation of  RISC-V
> sbi spec which is similar to edk2 for UEFI, the duplicate macros are
> expected. This is the wrapper file to OpenSBI because of we don't
> want to touch OpenSBI code.

I think we should look at refactoring this in OpenSBI instead.
Especially with us using this as effectively a library, we would need
to be actively monitoring (well, on every update, but you suggested
they may be frequent) whether any new clashes developed.

The guys who attended Plumbers suggested thy would be quite flexible
to restructure code in ways that makes the project more consumable.

I am OK with this being here while it is on the edk2-staging branch.

> > 
> > > +
> > > +#include "../opensbi/include/sbi/sbi_bits.h"
> > 
> > No relative includes. Let's figure out a way to expose the interface properly.
>
> Can be fixed by RiscVPkg.dec

Sounds good.

> > > +
> > > +#endif
> > > \ No newline at end of file
> > > diff --git a/RiscVPkg/Include/sbi/sbi_types.h
> > b/RiscVPkg/Include/sbi/sbi_types.h
> > > new file mode 100644
> > > index 0000000..fe877f2
> > > --- /dev/null
> > > +++ b/RiscVPkg/Include/sbi/sbi_types.h
> > > @@ -0,0 +1,24 @@
> > > +/** @file
> > > +  RISC-V OpesbSBI header file reference.
> > > +
> > > +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights
> > reserved.<BR>
> > > +
> > > +  This program and the accompanying materials
> > > +  are licensed and made available under the terms and conditions of the
> > BSD License
> > > +  which accompanies this distribution.  The full text of the license may be
> > found at
> > > +  INVALID URI REMOVED
> > 3A__opensource.org_licenses_bsd-
> > 2Dlicense.php&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN4V
> > gi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=iwfkW8MQjzEkixp0gv3xsvh20ei
> > odo7hGcTLXEL_I0o&s=mLKjYgrdQ6MuAN9UVYQeCDB0pNA44m9yBOylxW-
> > Koiw&e=
> > > +
> > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > BASIS,
> > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> > EXPRESS OR IMPLIED.
> > > +
> > > +**/
> > > +#ifndef __EDK2_SBI_TYPES_H__
> > > +#define __EDK2_SBI_TYPES_H__
> > > +
> > > +#undef TRUE
> > > +#undef FALSE
> > > +#undef NULL
> > 
> > Why?
> Same reason as above.

OK, same response as above.

> > > +
> > > +#include "../opensbi/include/sbi/sbi_types.h"
> > 
> > No relative includes.
> Can be fixed by RiscVPkg.dec

OK.

/
    Leif

  reply	other threads:[~2019-09-17 13:54 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-04 10:42 [PATCH 00/22] RISC-V EDK2 Port on edk2-staging/RISC-V-V2 branch Abner Chang
2019-09-04 10:42 ` [edk2-staging/RISC-V-V2 PATCH v1 01/22]: RiscVPkg: RISC-V processor package Abner Chang
2019-09-04 17:51   ` [edk2-devel] " Leif Lindholm
2019-09-16  5:15     ` Abner Chang
2019-09-17 14:03       ` Leif Lindholm
2019-09-19  7:10         ` Abner Chang
2019-09-20 17:04           ` Leif Lindholm
2019-09-21  7:14             ` Abner Chang
2019-09-04 10:42 ` [edk2-staging/RISC-V-V2 PATCH v1 02/22]: RiscVPkg/Include: Add header files of RISC-V CPU package Abner Chang
2019-09-04 18:55   ` [edk2-devel] " Leif Lindholm
2019-09-16  4:02     ` Abner Chang
2019-09-17 13:54       ` Leif Lindholm [this message]
2019-09-19  6:58         ` Abner Chang
2019-09-04 10:42 ` [edk2-staging/RISC-V-V2 PATCH v1 03/22]: MdePkg: RISC-V sections in DEC file Abner Chang
2019-09-04 19:02   ` [edk2-devel] " Leif Lindholm
2019-09-16  5:16     ` Abner Chang
2019-09-16  9:17       ` Leif Lindholm
2019-09-04 10:42 ` [edk2-staging/RISC-V-V2 PATCH v1 04/22]: MdePkg/Include: RISC-V definitions Abner Chang
2019-09-04 20:40   ` [edk2-devel] " Leif Lindholm
2019-09-16  5:31     ` Abner Chang
2019-09-17 14:11       ` Leif Lindholm
2019-09-17  8:32     ` Abner Chang
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 05/22]: MdeModulePkg/CapsuleRuntimeDxe: Add RISC-V arch Abner Chang
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 6/22]: MdePkg/BaseCacheMaintenanceLib: RISC-V cache maintenance implementation Abner Chang
2019-09-04 20:49   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 07/22]: MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions Abner Chang
2019-09-05 14:28   ` [edk2-devel] " Leif Lindholm
2019-09-16  5:37     ` Abner Chang
2019-09-17 14:14       ` Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 08/22]: MdePkg/BasePeCoff: Add RISC-V PE/Coff related code Abner Chang
2019-09-05 14:38   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 09/22]: MdePkg/BaseCpuLib: RISC-V Base CPU library implementation Abner Chang
2019-09-05 14:42   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 10/22]: MdePkg/BaseSynchronizationLib: RISC-V cache related code Abner Chang
2019-09-05 14:51   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 11/22]: BaseTools: BaseTools changes for RISC-V platform Abner Chang
2019-09-05 15:44   ` [edk2-devel] " Leif Lindholm
2019-09-16  6:44     ` Abner Chang
2019-09-17 12:15       ` Leif Lindholm
2019-09-09 11:36   ` Leif Lindholm
2019-09-16  7:46     ` Abner Chang
2019-09-17 13:08       ` Leif Lindholm
2019-09-17 14:26         ` Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 12/22]: MdePkg/BaseLib: BaseLib for RISC-V RV64 Processor Abner Chang
2019-09-05 16:11   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 13/22]: MdePkg/Include: Update SmBios header file Abner Chang
2019-09-05 16:16   ` [edk2-devel] " Leif Lindholm
2019-09-16  7:01     ` Abner Chang
2019-09-17 14:15       ` Leif Lindholm
     [not found]     ` <15C4D92300C8E997.28834@groups.io>
2019-09-17  6:58       ` Abner Chang
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 14/22]: RiscVPkg/opesbi: Add opensbi-HOWTO.txt Abner Chang
2019-09-05 16:19   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 15/22]: RiscVPkg/RealTimeClockRuntimeDxe: Add RISC-V RTC Runtime Driver Abner Chang
2019-09-05 16:26   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 16/22]: RiscVPkg/CpuDxe: Add RISC-V CPU DXE driver Abner Chang
2019-09-05 16:28   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 17/22]: RiscVPkg/SmbiosDxe: RISC-V platform generic SMBIOS " Abner Chang
2019-09-05 16:31   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 18/22]: RiscVPkg/Library: Add/Update/Remove Library instances for RISC-V platform Abner Chang
2019-09-05 16:48   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 19/22]: MdeModulePkg/DxeIplPeim:RISC-V platform DXEIPL Abner Chang
2019-09-05 16:50   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 20/22]: MdeModulePkg/Logo Abner Chang
2019-09-05 16:51   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 21/22]: NetworkPkg Abner Chang
2019-09-05 16:52   ` [edk2-devel] " Leif Lindholm
2019-09-04 10:43 ` [edk2-staging/RISC-V-V2 PATCH v1 22/22]: BaseTools/Scripts Abner Chang
2019-09-05 16:54   ` [edk2-devel] " Leif Lindholm
2019-09-05 17:15 ` [edk2-devel] [PATCH 00/22] RISC-V EDK2 Port on edk2-staging/RISC-V-V2 branch Leif Lindholm
2019-09-06  1:27   ` Abner Chang
     [not found]   ` <15C1B52667BA1578.25810@groups.io>
2019-09-23  1:15     ` Abner Chang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190917135406.GE28454@bivouac.eciton.net \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox