[edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 02/14] Silicon/SiFive: Add library module of SiFive RISC-V cores

[Leif Lindholm]

On Wed, Oct 16, 2019 at 01:36:07AM +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
> > -----Original Message-----
> > From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> > Leif Lindholm
> > Sent: Wednesday, October 2, 2019 5:15 AM
> > To: devel at edk2.groups.io; Chen, Gilbert <gilbert.chen at hpe.com>
> > Subject: Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 02/14]
> > Silicon/SiFive: Add library module of SiFive RISC-V cores
> > 
> > On Thu, Sep 19, 2019 at 11:51:19AM +0800, Gilbert Chen wrote:
> > > Initial version of SiFive RISC-V core libraries. Library of each core
> > > creates processor core SMBIOS data hob for building SMBIOS  records in
> > > DXE phase.
> > 
> > So yes, this implementation needs to change.
> > These should all implement the same LibraryClass.
>
> No. It shouldn't be the same library class (If you were saying the
> same LibraryClass). RISC-V SoC could be the combination of different
> RISC-V cores, or even the cores from different vendors. This depends
> on how SoC vendor combine those IPs.

Ah, OK. Sorry, did not realise this aspect.

> Either U54 or E51 could be a standalone SoC, while U54MC is the
> combination of 4 x U54 core and one E51 core.
> 
> U5MC under Platform/SiFive could be 1-8 U5 core and optionally
> support E5 core.  This is the special case for U500 VC707 platform
> because the core number could be customized.
> 
> > Also, U54 appears to be a simple superset of U51.
> U54 is a single core. 
> 
> > 
> > What I would suggest is creating a
> > Silicon/SiFive/Library/SiFiveCoreInfoLib, which calls into a
> > SiFiveSoCCoreInfoLib in Silicon/SiFive/<SoC>/Library, providing the acual SoC-
> > specific bits.
> 
> Platform system firmware integrator just pull in the necessary core
> libraries from Silicon/{vendor}  and invoke the function to create
> specific core bits.
>
> I think this implementation is well and flexible which has no need to change.

Oh, my criticism was that it was *too* flexible (with resulting code
overhead). I still feel the very small source differences, especially
between E51/U54, indicate that these could be implemented by the same
source file but different .inf files.

But this is not something that needs to be addressed before this patch
goes into -staging.

Regards,

Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#49178): https://edk2.groups.io/g/devel/message/49178
Mute This Topic: https://groups.io/mt/34196349/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-