Date   

Re: [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

Laszlo Ersek
 

On 08/16/19 21:38, Laszlo Ersek wrote:

I understand that edk2 is a "kit", and quite explicitly caters to
out-of-tree platforms. That's not a positive trait of edk2 however;
it's a negative one, in my judgement.
To clarify... I didn't mean that edk2 should willfully ignore dependent
platforms. Harmony between universal edk2 code and dependent platforms
is important. I meant that more platform code should live inside the
edk2 project. Again, this is a personal opinion.

Laszlo


Re: CPU hotplug using SMM with QEMU+OVMF

Laszlo Ersek
 

+Alex (direct question at the bottom)

On 08/16/19 09:49, Yao, Jiewen wrote:
below

-----Original Message-----
From: Paolo Bonzini [mailto:pbonzini@redhat.com]
Sent: Friday, August 16, 2019 3:20 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek
<lersek@redhat.com>; devel@edk2.groups.io
Cc: edk2-rfc-groups-io <rfc@edk2.groups.io>; qemu devel list
<qemu-devel@nongnu.org>; Igor Mammedov <imammedo@redhat.com>;
Chen, Yingwen <yingwen.chen@intel.com>; Nakajima, Jun
<jun.nakajima@intel.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>;
Joao Marcal Lemos Martins <joao.m.martins@oracle.com>; Phillip Goerl
<phillip.goerl@oracle.com>
Subject: Re: [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

On 16/08/19 04:46, Yao, Jiewen wrote:
Comment below:


-----Original Message-----
From: Paolo Bonzini [mailto:pbonzini@redhat.com]
Sent: Friday, August 16, 2019 12:21 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Yao,
Jiewen
<jiewen.yao@intel.com>
Cc: edk2-rfc-groups-io <rfc@edk2.groups.io>; qemu devel list
<qemu-devel@nongnu.org>; Igor Mammedov
<imammedo@redhat.com>;
Chen, Yingwen <yingwen.chen@intel.com>; Nakajima, Jun
<jun.nakajima@intel.com>; Boris Ostrovsky
<boris.ostrovsky@oracle.com>;
Joao Marcal Lemos Martins <joao.m.martins@oracle.com>; Phillip Goerl
<phillip.goerl@oracle.com>
Subject: Re: [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

On 15/08/19 17:00, Laszlo Ersek wrote:
On 08/14/19 16:04, Paolo Bonzini wrote:
On 14/08/19 15:20, Yao, Jiewen wrote:
- Does this part require a new branch somewhere in the OVMF SEC
code?
How do we determine whether the CPU executing SEC is BSP or
hot-plugged AP?
[Jiewen] I think this is blocked from hardware perspective, since the
first
instruction.
There are some hardware specific registers can be used to determine
if
the CPU is new added.
I don’t think this must be same as the real hardware.
You are free to invent some registers in device model to be used in
OVMF hot plug driver.

Yes, this would be a new operation mode for QEMU, that only applies
to
hot-plugged CPUs. In this mode the AP doesn't reply to INIT or SMI,
in
fact it doesn't reply to anything at all.

- How do we tell the hot-plugged AP where to start execution? (I.e.
that
it should execute code at a particular pflash location.)
[Jiewen] Same real mode reset vector at FFFF:FFF0.
You do not need a reset vector or INIT/SIPI/SIPI sequence at all in
QEMU. The AP does not start execution at all when it is unplugged,
so
no cache-as-RAM etc.

We only need to modify QEMU so that hot-plugged APIs do not reply
to
INIT/SIPI/SMI.

I don’t think there is problem for real hardware, who always has CAR.
Can QEMU provide some CPU specific space, such as MMIO region?
Why is a CPU-specific region needed if every other processor is in SMM
and thus trusted.
I was going through the steps Jiewen and Yingwen recommended.

In step (02), the new CPU is expected to set up RAM access. In step
(03), the new CPU, executing code from flash, is expected to "send
board
message to tell host CPU (GPIO->SCI) -- I am waiting for hot-add
message." For that action, the new CPU may need a stack (minimally if
we
want to use C function calls).

Until step (03), there had been no word about any other (= pre-plugged)
CPUs (more precisely, Jiewen even confirmed "No impact to other
processors"), so I didn't assume that other CPUs had entered SMM.

Paolo, I've attempted to read Jiewen's response, and yours, as carefully
as I can. I'm still very confused. If you have a better understanding,
could you please write up the 15-step process from the thread starter
again, with all QEMU customizations applied? Such as, unnecessary
steps
removed, and platform specifics filled in.
Sure.

(01a) QEMU: create new CPU. The CPU already exists, but it does not
start running code until unparked by the CPU hotplug controller.

(01b) QEMU: trigger SCI

(02-03) no equivalent

(04) Host CPU: (OS) execute GPE handler from DSDT

(05) Host CPU: (OS) Port 0xB2 write, all CPUs enter SMM (NOTE: New CPU
will not enter CPU because SMI is disabled)

(06) Host CPU: (SMM) Save 38000, Update 38000 -- fill simple SMM
rebase code.

(07a) Host CPU: (SMM) Write to CPU hotplug controller to enable
new CPU

(07b) Host CPU: (SMM) Send INIT/SIPI/SIPI to new CPU.
[Jiewen] NOTE: INIT/SIPI/SIPI can be sent by a malicious CPU. There is no
restriction that INIT/SIPI/SIPI can only be sent in SMM.
All of the CPUs are now in SMM, and INIT/SIPI/SIPI will be discarded
before 07a, so this is okay.
[Jiewen] May I know why INIT/SIPI/SIPI is discarded before 07a but is delivered at 07a?
I don’t see any extra step between 06 and 07a.
What is the magic here?
The magic is 07a itself, IIUC. The CPU hotplug controller would be
accessible only in SMM. And until 07a happens, the new CPU ignores
INIT/SIPI/SIPI even if another CPU sends it those, simply because QEMU
would implement the new CPU's behavior like that.




However I do see a problem, because a PCI device's DMA could overwrite
0x38000 between (06) and (10) and hijack the code that is executed in
SMM. How is this avoided on real hardware? By the time the new CPU
enters SMM, it doesn't run off cache-as-RAM anymore.
[Jiewen] Interesting question.
I don’t think the DMA attack is considered in threat model for the virtual environment. We only list adversary below:
-- Adversary: System Software Attacker, who can control any OS memory or silicon register from OS level, or read write BIOS data.
-- Adversary: Simple hardware attacker, who can hot add or hot remove a CPU.
We do have physical PCI(e) device assignment; sorry for not highlighting
that earlier. That feature (VFIO) does rely on the (physical) IOMMU, and
it makes sure that the assigned device can only access physical frames
that belong to the virtual machine that the device is assigned to.

However, as far as I know, VFIO doesn't try to restrict PCI DMA to
subsets of guest RAM... I could be wrong about that, I vaguely recall
RMRR support, which seems somewhat related.

I agree it is a threat from real hardware perspective. SMM may check VTd to make sure the 38000 is blocked.
I doubt if it is a threat in virtual environment. Do we have a way to block DMA in virtual environment?
I think that would be a VFIO feature.

Alex: if we wanted to block PCI(e) DMA to a specific part of guest RAM
(expressed with guest-physical RAM addresses), perhaps permanently,
perhaps just for a while -- not sure about coordination though --, could
VFIO accommodate that (I guess by "punching holes" in the IOMMU page
tables)?

Thanks
Laszlo




Paolo

(08a) New CPU: (Low RAM) Enter protected mode.
[Jiewen] NOTE: The new CPU still cannot use any physical memory,
because
the INIT/SIPI/SIPI may be sent by malicious CPU in non-SMM environment.

(08b) New CPU: (Flash) Signals host CPU to proceed and enter cli;hlt loop.

(09) Host CPU: (SMM) Send SMI to the new CPU only.

(10) New CPU: (SMM) Run SMM code at 38000, and rebase SMBASE to
TSEG.

(11) Host CPU: (SMM) Restore 38000.

(12) Host CPU: (SMM) Update located data structure to add the new CPU
information. (This step will involve CPU_SERVICE protocol)

(13) New CPU: (Flash) do whatever other initialization is needed

(14) New CPU: (Flash) Deadloop, and wait for INIT-SIPI-SIPI.

(15) Host CPU: (OS) Send INIT-SIPI-SIPI to pull new CPU in..


In other words, the cache-as-RAM phase of 02-03 is replaced by the
INIT-SIPI-SIPI sequence of 07b-08a-08b.
[Jiewen] I am OK with this proposal.
I think the rule is same - the new CPU CANNOT touch any system memory,
no matter it is from reset-vector or from INIT/SIPI/SIPI.
Or I would say: if the new CPU want to touch some memory before first
SMI, the memory should be
CPU specific or on the flash.



The QEMU DSDT could be modified (when secure boot is in effect) to
OUT
to 0xB2 when hotplug happens. It could write a well-known value to
0xB2, to be read by an SMI handler in edk2.
I dislike involving QEMU's generated DSDT in anything SMM (even
injecting the SMI), because the AML interpreter runs in the OS.

If a malicious OS kernel is a bit too enlightened about the DSDT, it
could willfully diverge from the process that we design. If QEMU
broadcast the SMI internally, the guest OS could not interfere with that.

If the purpose of the SMI is specifically to force all CPUs into SMM
(and thereby force them into trusted state), then the OS would be
explicitly counter-interested in carrying out the AML operations from
QEMU's DSDT.
But since the hotplug controller would only be accessible from SMM,
there would be no other way to invoke it than to follow the DSDT's
instruction and write to 0xB2. FWIW, real hardware also has plenty of
0xB2 writes in the DSDT or in APEI tables (e.g. for persistent store
access).

Paolo


Re: CPU hotplug using SMM with QEMU+OVMF

Laszlo Ersek
 

On 08/15/19 18:21, Paolo Bonzini wrote:
On 15/08/19 17:00, Laszlo Ersek wrote:
On 08/14/19 16:04, Paolo Bonzini wrote:
On 14/08/19 15:20, Yao, Jiewen wrote:
- Does this part require a new branch somewhere in the OVMF SEC code?
How do we determine whether the CPU executing SEC is BSP or
hot-plugged AP?
[Jiewen] I think this is blocked from hardware perspective, since the first instruction.
There are some hardware specific registers can be used to determine if the CPU is new added.
I don’t think this must be same as the real hardware.
You are free to invent some registers in device model to be used in OVMF hot plug driver.
Yes, this would be a new operation mode for QEMU, that only applies to
hot-plugged CPUs. In this mode the AP doesn't reply to INIT or SMI, in
fact it doesn't reply to anything at all.

- How do we tell the hot-plugged AP where to start execution? (I.e. that
it should execute code at a particular pflash location.)
[Jiewen] Same real mode reset vector at FFFF:FFF0.
You do not need a reset vector or INIT/SIPI/SIPI sequence at all in
QEMU. The AP does not start execution at all when it is unplugged, so
no cache-as-RAM etc.

We only need to modify QEMU so that hot-plugged APIs do not reply to
INIT/SIPI/SMI.

I don’t think there is problem for real hardware, who always has CAR.
Can QEMU provide some CPU specific space, such as MMIO region?
Why is a CPU-specific region needed if every other processor is in SMM
and thus trusted.
I was going through the steps Jiewen and Yingwen recommended.

In step (02), the new CPU is expected to set up RAM access. In step
(03), the new CPU, executing code from flash, is expected to "send board
message to tell host CPU (GPIO->SCI) -- I am waiting for hot-add
message." For that action, the new CPU may need a stack (minimally if we
want to use C function calls).

Until step (03), there had been no word about any other (= pre-plugged)
CPUs (more precisely, Jiewen even confirmed "No impact to other
processors"), so I didn't assume that other CPUs had entered SMM.

Paolo, I've attempted to read Jiewen's response, and yours, as carefully
as I can. I'm still very confused. If you have a better understanding,
could you please write up the 15-step process from the thread starter
again, with all QEMU customizations applied? Such as, unnecessary steps
removed, and platform specifics filled in.
Sure.

(01a) QEMU: create new CPU. The CPU already exists, but it does not
start running code until unparked by the CPU hotplug controller.

(01b) QEMU: trigger SCI

(02-03) no equivalent

(04) Host CPU: (OS) execute GPE handler from DSDT

(05) Host CPU: (OS) Port 0xB2 write, all CPUs enter SMM (NOTE: New CPU
will not enter CPU because SMI is disabled)

(06) Host CPU: (SMM) Save 38000, Update 38000 -- fill simple SMM
rebase code.
(Could Intel open source code for this?)

(07a) Host CPU: (SMM) Write to CPU hotplug controller to enable
new CPU

(07b) Host CPU: (SMM) Send INIT/SIPI/SIPI to new CPU.

(08a) New CPU: (Low RAM) Enter protected mode.
PCI DMA attack might be relevant (but yes, I see you've mentioned that
too, down-thread)


(08b) New CPU: (Flash) Signals host CPU to proceed and enter cli;hlt loop.

(09) Host CPU: (SMM) Send SMI to the new CPU only.

(10) New CPU: (SMM) Run SMM code at 38000, and rebase SMBASE to
TSEG.
I wish we could simply wake the new CPU -- after step 07a -- with an
SMI. IOW, if we could excise steps 07b, 08a, 08b.

Our CPU hotplug controller, and the initial parked state in 01a for the
new CPU, are going to be home-brewed anyway.

On the other hand...

(11) Host CPU: (SMM) Restore 38000.

(12) Host CPU: (SMM) Update located data structure to add the new CPU
information. (This step will involve CPU_SERVICE protocol)

(13) New CPU: (Flash) do whatever other initialization is needed

(14) New CPU: (Flash) Deadloop, and wait for INIT-SIPI-SIPI.
basically step 08b is the environment to which the new CPU returns in
13/14, after the RSM.

Do we absolutely need low RAM for 08a (for entering protected mode)? we
could execute from pflash, no? OTOH we'd still need RAM for the stack,
and that could be attacked with PCI DMA similarly. I believe.

(15) Host CPU: (OS) Send INIT-SIPI-SIPI to pull new CPU in..


In other words, the cache-as-RAM phase of 02-03 is replaced by the
INIT-SIPI-SIPI sequence of 07b-08a-08b.


The QEMU DSDT could be modified (when secure boot is in effect) to OUT
to 0xB2 when hotplug happens. It could write a well-known value to
0xB2, to be read by an SMI handler in edk2.
I dislike involving QEMU's generated DSDT in anything SMM (even
injecting the SMI), because the AML interpreter runs in the OS.

If a malicious OS kernel is a bit too enlightened about the DSDT, it
could willfully diverge from the process that we design. If QEMU
broadcast the SMI internally, the guest OS could not interfere with that.

If the purpose of the SMI is specifically to force all CPUs into SMM
(and thereby force them into trusted state), then the OS would be
explicitly counter-interested in carrying out the AML operations from
QEMU's DSDT.
But since the hotplug controller would only be accessible from SMM,
there would be no other way to invoke it than to follow the DSDT's
instruction and write to 0xB2.
Right.

FWIW, real hardware also has plenty of
0xB2 writes in the DSDT or in APEI tables (e.g. for persistent store
access).
Thanks
Laszlo


Re: [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update

Laszlo Ersek
 

On 08/16/19 17:39, Liming Gao wrote:
From: Mike Turner <miketur@microsoft.com>

The Fpdt driver (FirmwarePerformanceDxe) saves a memory address across
reboots, and then does an AllocatePage for that memory address.
If, on this boot, that memory comes from a Runtime memory bucket,
the MAT table is not updated. This causes Windows to boot into Recovery.

This patch blocks the memory manager from changing the page
from a special bucket to a different memory type. Once the buckets are
allocated, we freeze the memory ranges for the OS, and fragmenting
the special buckets will cause errors resuming from hibernate (S4).

The references to S4 here are the use case that fails. This
failure is root caused to an inconsistent behavior of the
core memory services themselves when type AllocateAddress is used.

The main issue is apparently with the UEFI memory map -- the UEFI memory
map reflects the pre-allocated bins, but the actual allocations at fixed
addresses may go out of sync with that. Everything else, such as:
- EFI_MEMORY_ATTRIBUTES_TABLE (page protections) being out of sync,
- S4 failing
are just symptoms / consequences.

This patch is cherry pick from Project Mu:
https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af94016ebd391ea6f340920735a
With the minor change,
1. Update commit message format to keep the message in 80 characters one line.
2. Remove // MU_CHANGE comments in source code.
3. Update comments style to follow edk2 style.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
In v2, add more description for this issue.

MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index bd9e116aa5..1f0e3d94b9 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
IN BOOLEAN NeedGuard
)
{
- EFI_STATUS Status;
- UINT64 Start;
- UINT64 NumberOfBytes;
- UINT64 End;
- UINT64 MaxAddress;
- UINTN Alignment;
+ EFI_STATUS Status;
+ UINT64 Start;
+ UINT64 NumberOfBytes;
+ UINT64 End;
+ UINT64 MaxAddress;
+ UINTN Alignment;
+ EFI_MEMORY_TYPE CheckType;

if ((UINT32)Type >= MaxAllocateType) {
return EFI_INVALID_PARAMETER;
@@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
// if (Start + NumberOfBytes) rolls over 0 or
// if Start is above MAX_ALLOC_ADDRESS or
// if End is above MAX_ALLOC_ADDRESS,
+ // if Start..End overlaps any tracked MemoryTypeStatistics range
// return EFI_NOT_FOUND.
//
if (Type == AllocateAddress) {
@@ -1336,6 +1338,33 @@ CoreInternalAllocatePages (
(End > MaxAddress)) {
return EFI_NOT_FOUND;
}
+
+ //
+ // A driver is allowed to call AllocatePages using an AllocateAddress type. This type of
+ // AllocatePage request the exact physical address if it is not used. The existing code
+ // will allow this request even in 'special' pages. The problem with this is that the
+ // reason to have 'special' pages for OS hibernate/resume is defeated as memory is
+ // fragmented.
+ //
+
+ for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType < EfiMaxMemoryType; CheckType++) {
+ if (MemoryType != CheckType &&
+ mMemoryTypeStatistics[CheckType].Special &&
+ mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
+ if (Start >= mMemoryTypeStatistics[CheckType].BaseAddress &&
+ Start <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
+ return EFI_NOT_FOUND;
+ }
+ if (End >= mMemoryTypeStatistics[CheckType].BaseAddress &&
+ End <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
+ return EFI_NOT_FOUND;
+ }
+ if (Start < mMemoryTypeStatistics[CheckType].BaseAddress &&
+ End > mMemoryTypeStatistics[CheckType].MaximumAddress) {
+ return EFI_NOT_FOUND;
+ }
+ }
+ }
}

if (Type == AllocateMaxAddress) {
Acked-by: Laszlo Ersek <lersek@redhat.com>

Please wait for maintainer approval too.

Thanks
Laszlo


Re: [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

Laszlo Ersek
 

On 08/15/19 18:08, Michael D Kinney wrote:
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
To summarize: personally, I disgree, but I can accept if the patch goes
in with Mike's R-b.

Thanks,
Laszlo

-----Original Message-----
From: devel@edk2.groups.io
[mailto:devel@edk2.groups.io] On Behalf Of vit9696 via
Groups.Io
Sent: Tuesday, August 13, 2019 1:17 AM
To: devel@edk2.groups.io
Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add
STATIC_ASSERT macro

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048

Provide a macro for compile time assertions.
Equivalent to C11 static_assert macro from assert.h.

Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com>
---
MdePkg/Include/Base.h | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/MdePkg/Include/Base.h
b/MdePkg/Include/Base.h index
ce20b5f01dce..f85f7028a262 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST;
#define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)-
Field)) #endif
+///
+/// Portable definition for compile time assertions.
+/// Equivalent to C11 static_assert macro from
assert.h.
+/// Takes condtion and error message as its arguments.
+///
+#ifdef _MSC_EXTENSIONS
+ #define STATIC_ASSERT static_assert
+#else
+ #define STATIC_ASSERT _Static_assert
+#endif
+
/**
Macro that returns a pointer to the data structure
that contains a specified field of
that data structure. This is a lightweight method
to hide information by placing a
--
2.20.1 (Apple Git-117)


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

View/Reply Online (#45503):
https://edk2.groups.io/g/devel/message/45503
Mute This Topic: https://groups.io/mt/32850582/1643496
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub
[michael.d.kinney@intel.com]
-=-=-=-=-=-=



Re: [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

Laszlo Ersek
 

On 08/16/19 19:23, vit9696@protonmail.com wrote:
Laszlo,

I have already mentioned that the documentation is sufficient as
_Static_assert is C standard
Yes, in a release of the ISO C standard that edk2 does not target.

In addition, edk2 already has several restrictions in place against
standards-conformant code. (Such as bit-shifting of UINT64 values,
initialization of structures with automatic storage duration, structure
assignment, maybe more.)

so I do not plan to make a V3 for this patch.
I find that regrettable.

The patch is merge ready.
Such statements are usually made when people that comment on a patch
arrive at a consensus. The patch may be merge-ready from your
perspective and from Mike's. It is not merge-ready from my perspective.
I hope I'm allowed to comment (constructively) on patches that aren't
strictly aimed at the subsystems I co-maintain.

As for usage examples I have an opposing opinion to yours and believe
it is based on very good reasons. Not using STATIC_ASSERT in the
current release will make the feature optionally available and let
people test it in their setups.
Not using STATIC_ASSERT in the current stable release makes the
STATIC_ASSERT macro definition *dead code* in edk2 proper. I understand
that edk2 is a "kit", and quite explicitly caters to out-of-tree
platforms. That's not a positive trait of edk2 however; it's a negative
one, in my judgement. Whatever we add to the core of edk2, we should
exercise as diligently as we can *inside* of edk2.

In case they notice it does not work for them they will have 3 months
grace period to report it to us and consider making a change.
That is what the feature freezes are for. The feature is reviewed before
the soft feature freeze, merged (at the latest) during the soft feature
freeze, and bugs can still be fixed during the hard feature freeze. The
community is expected to test diligently during the hard feature freeze.
Perhaps we should extend the hard feature freeze.

My problem is not that the change is not "in your face". I'm all for
avoiding regressions. My problem is that the code is dead and untestable
without platform changes, even though it could be put to great use in
core code at once. If you think that's too risky, this close to the
stable tag, then one solution is to resubmit at the beginning of the
next development cycle (again with additional patches that utilize the
STATIC_ASSERT macro at once). Developers will then have close to three
months to report and fix issues.

Another solution would be to conditionally keep VERIFY_SIZE_OF, vs.
using STATIC_ASSERT, for expressing the build-time invariants. The
default would be STATIC_ASSERT. Should it break, people could
immediately switch back to VERIFY_SIZE_OF, without disruption to their
workflows.

We've done similar things in OvmfPkg in the past. For example:
- USE_LEGACY_ISA_STACK (commit a06810229618 / commit 562688707145),
- USE_OLD_BDS (commit 79c098b6d25d / commit dd43486577b3),
- USE_OLD_PCI_HOST (commit 4014885ffdfa / commit cef83a3050e5).

This will also give them 3 months grace period of VERIFY_SIZE_OF macro
removal in favour of STATIC_ASSERT. Making the change now will let
people do seamless transition to the new feature and will avoid
obstacles you are currently trying to create.
Please stop making claims in bad faith. I'm not trying to "create
obstacles". I'm a fan of STATIC_ASSERT. I'm not a fan of dead code.

Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal must both be
separate patchsets with potentially separate BZs.

Thanks for understanding,
Why are you presenting this as a done deal? The v2 patch was submitted
three days ago, IIUC.

Also, I wish we could have this discussion without condescension.

Thanks,
Laszlo


Re: [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

Laszlo Ersek
 

On 08/16/19 17:24, Gao, Liming wrote:
Laszlo:

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Laszlo Ersek
Sent: Friday, August 16, 2019 11:18 PM
To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney,
Michael D <michael.d.kinney@intel.com>
Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>
Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for
missing MAT update

On 08/14/19 17:55, Gao, Liming wrote:

If Platform PEIM doesn't build HOB, DxeIpl will not build HOB,
My reading of the code is the opposite. If the platform PEIM does not
build the HOB, then the DXE IPL PEIM will attempt to build the HOB,
from the UEFI variable.

At commit caa7d3a896f6, in file
"MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(), we
have:

363 if (GetFirstGuidHob ((CONST EFI_GUID *)&gEfiMemoryTypeInformationGuid) == NULL) {
364 //
365 // Don't build GuidHob if GuidHob has been installed.
366 //
367 Status = PeiServicesLocatePpi (
368 &gEfiPeiReadOnlyVariable2PpiGuid,
369 0,
370 NULL,
371 (VOID **)&Variable
372 );
373 if (!EFI_ERROR (Status)) {
374 DataSize = sizeof (MemoryData);
375 Status = Variable->GetVariable (
376 Variable,
377 EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
378 &gEfiMemoryTypeInformationGuid,
379 NULL,
380 &DataSize,
381 &MemoryData
382 );
383 if (!EFI_ERROR (Status) && ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) {
Only when this variable exists, Hob will be built. But, if no PEIM
builds Hob, BDS will not set the variable.
So, there is still no HOB.
So how is a platform supposed to enable this feature?

If PlatformPei never builds the HOB, the variable will never be created,
so the DXE IPL PEIM will also not build the HOB, ever.

If PlatformPei builds the HOB with static data, then BDS will set
(update) the variable, yes, but the DXE IPL PEIM will ignore the
variable, because PlatformPei already built the HOB.

So... Is PlatformPei supposed to use the Variable PPI, check if the
variable exists, and create the static HOB only if the variable is
absent?

... Ugh, wait. I've actually implemented this for OVMF almost 2 years
ago! And the answer to my question is "yes":

https://bugzilla.tianocore.org/show_bug.cgi?id=386
https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html

See the OnReadOnlyVariable2Available() function:

+ //
+ // Check if the "MemoryTypeInformation" variable exists, in the
+ // gEfiMemoryTypeInformationGuid namespace.
+ //
+ ReadOnlyVariable2 = Ppi;
+ DataSize = 0;
+ Status = ReadOnlyVariable2->GetVariable (
+ ReadOnlyVariable2,
+ EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
+ &gEfiMemoryTypeInformationGuid,
+ NULL,
+ &DataSize,
+ NULL
+ );
+ if (Status == EFI_BUFFER_TOO_SMALL) {
+ //
+ // The variable exists; the DXE IPL PEIM will build the HOB from it.
+ //
+ return EFI_SUCCESS;
+ }
+ //
+ // Install the default memory type information HOB.
+ //
+ BuildMemTypeInfoHob ();

Apologies for forgetting about this; you are right.

Too bad my work for TianoCore#386 was rejected. :(

Thanks
Laszlo


Re: Patch List for 201908 stable tag

Laszlo Ersek
 

On 08/16/19 10:00, Gao, Liming wrote:

From: Gao, Liming [mailto:liming.gao@intel.com]
Sent: Friday, August 16, 2019 3:59 PM
To: Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com; Cetola, Stephano <stephano.cetola@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Patch List for 201908 stable tag

Hi Stewards and all:
I collect current patch lists in devel mail list. Those patch contributors request to add them for 201908 stable tag. Because the time is very close to Soft Feature Freeze, I want to collect your feedback for below patches.

Feature List (those all have pass code review):
https://edk2.groups.io/g/devel/message/45734 [PATCH v6 0/5] Build cache enhancement
Seems to be well documented (both in commit messages and in the BZ), and
the series was approved by a BaseTools maintainer [1] before the soft
feature freeze [2].

[1] https://edk2.groups.io/g/devel/message/45783
[2] https://edk2.groups.io/g/devel/message/45806

The series can be pushed during the soft feature freeze, according to
the current SFF definition.

Some requests regarding the bugzilla:

- Comment 0 in the bugzilla says "redesign the platform hash algorithm".
I feel that would be better reflected if we changed the Product field to
TianoCore Feature Requests.

- Please capture the v1..v6 cover letters, with mailing list archive
links, in the bugzilla.

https://edk2.groups.io/g/devel/message/45707 [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf
Accepted by a UefiCpuPkg maintainer [3] before the SFF [2].

[3] https://edk2.groups.io/g/devel/message/45787

Can be pushed during the soft feature freeze, according to the current
SFF definition.

According to my comments sent earlier today for this series, the
documentation and the bugzilla status are extremely lacking. I'm OK with
pushing the series, but those aspects should be fixed in the bugzilla.
Please see <https://edk2.groups.io/g/devel/message/45826>.

In addition, it appears multiple versions of the patch have been sent,
without using "vN" identifiers in the subject prefix. Please collect all
versions of the patch series from the mailing list archive, in
chronological order, and link them all into the bugzilla ticket.

https://edk2.groups.io/g/devel/message/45503 [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro
I commented on this today, as well:
<https://edk2.groups.io/g/devel/message/45827>

I disagree with the maintainer approval for this patch, from a reviewer
perspective. I think the submission isn't complete enough / mature
enough at this stage. The first version of the patch was posted on 12
Aug <https://edk2.groups.io/g/devel/message/45451>, as far as I can see.
Why are we in a mortal hurry to add a central macro without any call
sites in edk2 proper?

Anyway I'm not going to NACK the patch. If Mike is fine with the patch
as-is, I'll live with that, with my disagreement noted.

Bug List:
https://edk2.groups.io/g/devel/message/45794 [PATCH 1/1] CryptoPkg: Fix coding style
Purely from the subject line, looks like a bugfix, so no conflict with
the *soft* feature freeze.

https://edk2.groups.io/g/devel/message/45791 [PATCH v2 1/1] ShellPkg/UefiShellAcpiViewCommandLib: Replace shift logical left
Clearly a bugfix, so ditto.

https://edk2.groups.io/g/devel/message/45793 [Patch 1/1] BaseTools: Fixed issue of incorrect Module Unique Name
Ditto.

https://edk2.groups.io/g/devel/message/45773 [Patch v4 0/6] Add "test then write" mechanism
Sigh, this is a messy question.

To my eyes, this introduces a feature. The cover letter says "add ...
mechanism". Even though it is used to fix a bug, it still introduces a
new facility, in my opinion. So here I disagree with
<https://edk2.groups.io/g/devel/message/45590>.

For UefiCpuPkg, Ray and Eric are Maintainers, and I'm a reviewer. The
submitter is Eric, and Ray is away. Eric asked me to review
<https://edk2.groups.io/g/devel/message/45489>.

I reviewed v2. Then v3 was posted (not sure in response to what, as I
can't see any comments under v2 that requested the v3 changes). V3
received a bunch of comments, so v4 was posted quickly afterwards.

The posting of v4 was still in time for the SFF, but I only arrived at
it after the SFF. (And Ray is still away.) Technically speaking, in my
book, this series has missed the stable tag. But that's not the
submitter's fault, arguably.

On this, I am undecided. I will defer to the other stewards. If they are
fine with the series going in, I'm OK too.

https://edk2.groups.io/g/devel/message/45317 [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
This is a bugfix and therefore there is no conflict with the soft
feature freeze. Jian provided a maintainer R-b, and I asked for a
comment style cleanup and some documentation (commit message)
improvements. In addition, perhaps we should conditionalize the logic on
the gEfiMemoryTypeInformationGuid GUID HOB. (The discussion between
Liming and myself on where the HOB can come from is irrelevant in that
regard -- wherever the HOB comes from, it is suitable for checking in
the DXE core, I think.)

I think it's fine to submit a v2 and merge it during the soft feature
freeze (again, bugfix, so no conflict). I may not be around to review
v2, so don't wait for my feedback.

Thanks
Laszlo


Re: Determining TSC frequency programmatically

Andrew Fish
 

Vitaly,

As Mike mentioned platforms can know more info about how they are constructed thus you may not want to have a lot of generic discovery code floating about if you don't really need it. 

One option could be to pass up the TSC Frequency/Period via some EFI mechanism so generic code can be told by platform specific code. 

The PI spec already has an abstraction for a CPU based timer that is architecture neutral. The CPU Architectural Protocol has a GetTimerValue() member function. 

For X86 it returns TSC

EFI Systems are not required to implement PI so we usually don't encourage generic EFI code to go after PI APIs. 

I'd also point out that using TSC can break in things like the EmulatorPkg as you end up running in ring 0 and TSC access is blocked. 


I would point that a library that did TSC frequency discovery would likely be useful for the UefiCpuPkg CpuDxe driver. 

Thanks,

Andrew Fish

On Aug 15, 2019, at 2:10 PM, Vitaly Cheptsov via Groups.Io <vit9696@...> wrote:

Hello,

I initially raised this question in a new TimerLib patch[1], but as the discussion was getting more distracted, I decided to create a separate thread in hopes new people could join.

The issue is that our UEFI bootloader needs to obtain TSC frequency to pass it to our specialised operating system that uses TSC for scheduling on x86.

For a while we went with ACPI power management timer to measure the frequency, but as modern Intel CPUs support CPUID 15H leaf (CPUID_TIME_STAMP_COUNTER) we try to use where possible for better accuracy. The issue with this CPUID leaf is that the crystal clock frequency returned in ECX register is optional and therefore can be 0. Intel SDM suggests to use a static value in this case[2], but it is completely opaque on how to match the running CPU with its static value from SDM.

Initially we went with CPUID model checking, but this failed badly for Xeon Scalable and Xeon W, as they share the CPUID (06_55H) but have different crystal clock frequencies (25 MHz vs 24 MHz accordingly). Donald Kuo gave a good hint in the previous thread that client CPUs usually get 24 MHz crystal clock, server CPUs have 25, and Atoms have 19.2. This, however, does not make the situation easier as we do not see a way to determine CPU vertical segment without e.g. parsing the CPUID brand string.

Apparently, we are not alone, and different open-source operating systems have different workarounds to this issue. For example, Linux kernel went with using marketing frequency from CPUID 16H leaf (CPUID_PROCESSOR_FREQUENCY)[3], and BSD flavours fallback to older methods when neither crystal clock frequency can be obtained through CPUID 15H, nor unambiguous CPUID models exist to be able to use static values.

Another issue we see with EDK II TimerLib implementations for x86 is that they are very model specific. As Michael Kinney said, the situation is not a problem when you use TimerLib for BSP bringup, as you already know the CPU family you target, however, it is not great for other uses, although they may be discouraged. In our opinion, regardless of the use, this approach has a number of design issues.

All modern Intel x86 CPUs have virtual TSC counter that has fixed frequency. In fact, this is true for most, if not all, modern x86 CPUs by other vendors as well. This makes us believe that EDK II should have generic TscTimerLib for x86, which will work anywhere when given valid TSC frequency, and TscFrequencyLib, which would determine TSC frequency in a vendor-specific method. Ideally there exists GenericTscFrequencyLib that can do this for a wide majority of CPUs through different methods at runtime, including CPUID 15H, ACPI power management, vendor-specific extensions, etc.

To summarise:
- We need to know how to match current running CPU with its crystal clock frequency when CPUID 15H reports 0 ECX.
- We would like to suggest to split TSC-based TimerLib in two libraries: generic with timer implementation and platform-specific with TSC frequency discovery.
- We wonder whether a generic vendor-supported TSC frequency discovery library working on a wide range of CPUs is feasible to have in EDK II mainline.

Best regards,
Vitaly

    [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf
[2] 18.7.3 Determining the Processor Base Frequency
Table 18-75. Nominal Core Crystal Clock Frequency



Re: [Patch V4 09/10] EmulatorPkg/Sec: Change local variable scope for XCODE5

Jordan Justen
 

On 2019-08-15 19:14:36, Michael D Kinney wrote:
The local variable PpiArray[10] is declared in middle
of the SEC module _ModuleEntryPoint(). When building
for XCODE5 with optimizations enabled, this results in
corruption and an exception.
It looks like with old code, the scope containing PpiArray was closed,
but a dangling reference to had been made to it's location on the
stack. So, I think the change is good but we should add this extra
detail to the commit message.

-Jordan

The fix is to move the
declaration of PpiArray[10] to the standard location
at the beginning of the function so the storage for
this local variable is allocated for the entire
lifetime of the function.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Andrew Fish <afish@apple.com>
---
EmulatorPkg/Sec/Sec.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/EmulatorPkg/Sec/Sec.c b/EmulatorPkg/Sec/Sec.c
index 701032233b..b734d2bb87 100644
--- a/EmulatorPkg/Sec/Sec.c
+++ b/EmulatorPkg/Sec/Sec.c
@@ -75,6 +75,7 @@ _ModuleEntryPoint (
EFI_PEI_PPI_DESCRIPTOR *SecPpiList;
UINTN SecReseveredMemorySize;
UINTN Index;
+ EFI_PEI_PPI_DESCRIPTOR PpiArray[10];

EMU_MAGIC_PAGE()->PpiList = PpiList;
ProcessLibraryConstructorList ();
@@ -104,16 +105,13 @@ _ModuleEntryPoint (
SecCoreData->PeiTemporaryRamBase = (VOID *)((UINTN)SecCoreData->PeiTemporaryRamBase + SecReseveredMemorySize);
SecCoreData->PeiTemporaryRamSize -= SecReseveredMemorySize;
#else
- {
- //
- // When I subtrack from SecCoreData->PeiTemporaryRamBase PEI Core crashes? Either there is a bug
- // or I don't understand temp RAM correctly?
- //
- EFI_PEI_PPI_DESCRIPTOR PpiArray[10];
+ //
+ // When I subtrack from SecCoreData->PeiTemporaryRamBase PEI Core crashes? Either there is a bug
+ // or I don't understand temp RAM correctly?
+ //

- SecPpiList = &PpiArray[0];
- ASSERT (sizeof (PpiArray) >= SecReseveredMemorySize);
- }
+ SecPpiList = &PpiArray[0];
+ ASSERT (sizeof (PpiArray) >= SecReseveredMemorySize);
#endif
// Copy existing list, and append our entries.
CopyMem (SecPpiList, PpiList, sizeof (EFI_PEI_PPI_DESCRIPTOR) * Index);
--
2.21.0.windows.1


Re: [Patch V4 06/10] EmulatorPkg: Fix XCODE5 lldb issues

Jordan Justen
 

On 2019-08-16 08:09:55, Kinney, Michael D wrote:
Jordan,

It is not a typo.

Andrew generated the XCODE specific changes, so they have
been tested by him.
In that case, is the git author for the patches set to Andrew? If it
was, I would expect to see a line inside the email body with:

From: Andrew Fish <afish@apple.com>

Git does that for patches where the sender doesn't match the patch
author.

You might want to rebase and run:

git commit --amend --author="Andrew Fish <afish@apple.com>" --reset-author

to change the author.

I expect you might want to add Reviewed-by for yourself on these
patches to help speed things along. If Andrew authored the patch, you
reviewed it, and with a quick review it looked good to me, I would
probably add an Acked-by for some of the patches.

-Jordan

I have also reviewed and tested the XCODE
changes and verified that all 6 combinations build and boot
to shell (IA32/X64 for RELEASE/DEBUG/NOOPT). Since they are
all related to making EmulatorPkg work, we decided to fold
them into the same patch set that was already being reviewed.

I also verified build and boot to shell for 6 combinations
on GCC5 (IA32/X64 for RELEASE/DEBUG/NOOPT) and the 12
combinations of VS2015/VS2017, IA323/X64, RELEASE/DEBUG/NOOPT.

I have been working on some CI experiments using Azure Pipelines.
Here is a pointer to the build logs for all the combinations
listed above.

https://dev.azure.com/mikekinney/edk2-ci/_build/results?buildId=312

Mike

-----Original Message-----
From: Justen, Jordan L
Sent: Friday, August 16, 2019 12:41 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>;
devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Andrew Fish
<afish@apple.com>
Subject: Re: [Patch V4 06/10] EmulatorPkg: Fix XCODE5
lldb issues

On 2019-08-15 19:14:33, Michael D Kinney wrote:
Fix scripts to support lldb symbolic debugging when
using XCODE5 tool
chain.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Andrew Fish <afish@apple.com>
Is this a Cc/Signed-off-by typo? (See also, patches 7-
10).

This makes me wonder if you are taking advantage of the
git commit -s switch to add your Signed-off-by.

Also, I'm wondering if you are taking advantage of git-
send-email automatically Cc'ing the addresses you
listed in the commit message.
(I thought it Cc'd for the author and Cc tags, but I
wasn't sure about the Signed-off-by tag, and yet I see
Andrew was Cc'd.)

There's a couple long lines below. You could use \ at
the end of the line to split the .sh line. I think the
cd can be a separate command in a shell script. (Not in
make)

I hope someone that uses the XCODE toolchain could
review/check the XCODE patches.

-Jordan

---
EmulatorPkg/Unix/lldbefi.py | 8 +++++---
EmulatorPkg/build.sh | 17 ++---------------
2 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/EmulatorPkg/Unix/lldbefi.py
b/EmulatorPkg/Unix/lldbefi.py
index 218326b8cb..099192d8b5 100755
--- a/EmulatorPkg/Unix/lldbefi.py
+++ b/EmulatorPkg/Unix/lldbefi.py
@@ -346,6 +346,7 @@ def TypePrintFormating(debugger):
debugger.HandleCommand("type summary add CHAR8 -
-python-function lldbefi.CHAR8_TypeSummary")
debugger.HandleCommand('type summary add --regex
"CHAR8
\[[0-9]+\]" --python-function
lldbefi.CHAR8_TypeSummary')

+ debugger.HandleCommand('setting set frame-format
"frame
+ #${frame.index}: ${frame.pc}{
+
${module.file.basename}{:${function.name}()${function.p
c-offset}}}{
+ at ${line.file.fullpath}:${line.number}}\n"')

gEmulatorBreakWorkaroundNeeded = True

@@ -381,15 +382,16 @@ def
LoadEmulatorEfiSymbols(frame, bp_loc , internal_dict):
Error = lldb.SBError()
FileNamePtr = frame.FindVariable
("FileName").GetValueAsUnsigned()
FileNameLen = frame.FindVariable
("FileNameLength").GetValueAsUnsigned()
+
FileName =
frame.thread.process.ReadCStringFromMemory
(FileNamePtr, FileNameLen, Error)
if not Error.Success():
print "!ReadCStringFromMemory() did not find
a %d byte C string at %x" % (FileNameLen, FileNamePtr)
# make breakpoint command contiue
- frame.GetThread().GetProcess().Continue()
+ return False

debugger = frame.thread.process.target.debugger
if frame.FindVariable
("AddSymbolFlag").GetValueAsUnsigned() == 1:
- LoadAddress = frame.FindVariable
("LoadAddress").GetValueAsUnsigned()
+ LoadAddress = frame.FindVariable
+ ("LoadAddress").GetValueAsUnsigned() - 0x240

debugger.HandleCommand ("target modules add
%s" % FileName)
print "target modules load --slid 0x%x %s" %
(LoadAddress,
FileName) @@ -405,7 +407,7 @@ def
LoadEmulatorEfiSymbols(frame, bp_loc , internal_dict):
print "!lldb.target.RemoveModule
(%s) FAILED" %
SBModule

# make breakpoint command contiue
- frame.thread.process.Continue()
+ return False

def GuidToCStructStr (guid, Name=False):
#
diff --git a/EmulatorPkg/build.sh
b/EmulatorPkg/build.sh index
60056e1b6c..35912a7775 100755
--- a/EmulatorPkg/build.sh
+++ b/EmulatorPkg/build.sh
@@ -209,21 +209,8 @@ fi
if [[ "$RUN_EMULATOR" == "yes" ]]; then
case `uname` in
Darwin*)
- #
- # On Darwin we can't use dlopen, so we have to
load the real PE/COFF images.
- # This .gdbinit script sets a breakpoint that
loads symbols for the PE/COFFEE
- # images that get loaded in Host
- #
- if [[ "$CLANG_VER" == *-ccc-host-triple* ]]
- then
- # only older versions of Xcode support -ccc-
host-tripe, for newer versions
- # it is -target
- cp $WORKSPACE/EmulatorPkg/Unix/lldbefi.py
"$BUILD_OUTPUT_DIR/${BUILDTARGET}_$TARGET_TOOLS/$PROCES
SOR"
- cd $BUILD_ROOT_ARCH; /usr/bin/lldb --source
$WORKSPACE/EmulatorPkg/Unix/lldbinit Host
- exit $?
- else
- cp $WORKSPACE/EmulatorPkg/Unix/.gdbinit
"$BUILD_OUTPUT_DIR/${BUILDTARGET}_$TARGET_TOOLS/$PROCES
SOR"
- fi
+ cd $BUILD_ROOT_ARCH; /usr/bin/lldb -o "command
script import $WORKSPACE/EmulatorPkg/Unix/lldbefi.py" -
o 'script lldb.debugger.SetAsync(True)' -o "run" ./Host
+ exit $?
;;
esac

--
2.21.0.windows.1


Re: [edk2-platforms: PATCH v2 10/10] Marvell/Drivers: SmbiosPlatformDxe: Use more generic board name

Leif Lindholm
 

On Thu, Aug 15, 2019 at 04:54:14AM +0200, Marcin Wojtas wrote:
SmbiosPlatformDxe is used both by Armada 7k8k and CN913x platforms.
Replace board name placeholder in order to avoid confusion.
Stupid/lazy question - do we already specify the actual platform name
elsewhere?

/
Leif

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c b/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
index 08f4fa7..cdacd90 100644
--- a/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
+++ b/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
@@ -102,7 +102,7 @@ STATIC SMBIOS_TABLE_TYPE1 mArmadaDefaultType1 = {

STATIC CHAR8 CONST *mArmadaDefaultType1Strings[] = {
"Marvell \0",/* Manufacturer */
- "Armada 7k/8k Family Board \0",/* Product Name placeholder*/
+ "Marvell Development Board \0",/* Product Name placeholder*/
"Revision unknown \0",/* Version placeholder */
" \0",/* 32 character buffer */
NULL
@@ -130,7 +130,7 @@ STATIC SMBIOS_TABLE_TYPE2 mArmadaDefaultType2 = {

STATIC CHAR8 CONST *mArmadaDefaultType2Strings[] = {
"Marvell \0",/* Manufacturer */
- "Armada 7k/8k Family Board \0",/* Product Name placeholder*/
+ "Marvell Development Board \0",/* Product Name placeholder*/
"Revision unknown \0",/* Version placeholder */
"Serial Not Set \0",/* Serial */
"Base of Chassis \0",/* Board location */
--
2.7.4




Re: [edk2-platforms: PATCH v2 09/10] Marvell/Cn9132Db: Introduce board support

Leif Lindholm
 

On Thu, Aug 15, 2019 at 04:54:13AM +0200, Marcin Wojtas wrote:
This patch introduces all necessary components required
for building EDK2 firmware for CN9132-DB setup A. Note
the ACPI is not yet available for this variant, due to
the current ICU (CP1xx interrupt controller) support
implementation.

In order to build this variant, '-D CN9132' flag should be added.
Otherwise the default (CN9130) will be compiled.
Same comment on commit message - don't forget to update if logic
changed.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Cn913xDb/Cn9132DbA.dsc.inc | 72 +++++++++++
Platform/Marvell/Cn913xDb/Cn913xDbA.dsc | 15 ++-
Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.inf | 29 +++++
Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9132DbA.inf | 22 ++++
Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h | 4 +
Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.c | 135 ++++++++++++++++++++
Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c | 42 ++++++
Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc | 2 +
Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db-A.dts | 6 -
Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db.dtsi | 20 ++-
10 files changed, 333 insertions(+), 14 deletions(-)
create mode 100644 Platform/Marvell/Cn913xDb/Cn9132DbA.dsc.inc
create mode 100644 Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.inf
create mode 100644 Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9132DbA.inf
create mode 100644 Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.c

diff --git a/Platform/Marvell/Cn913xDb/Cn9132DbA.dsc.inc b/Platform/Marvell/Cn913xDb/Cn9132DbA.dsc.inc
new file mode 100644
index 0000000..a0b90fa
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/Cn9132DbA.dsc.inc
@@ -0,0 +1,72 @@
+## @file
+# Component description file for the CN9132 Development Board (variant A)
+#
+# Copyright (c) 2019 Marvell International Ltd.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+################################################################################
+#
+# Pcd Section - list of all EDK II PCD Entries defined by this Platform
+#
+################################################################################
+[PcdsFixedAtBuild.common]
+ # CP115 count
+ gMarvellTokenSpaceGuid.PcdMaxCpCount|3
+
+ # MPP
+ gMarvellTokenSpaceGuid.PcdMppChipCount|4
+
+ # CP115 #2 MPP
+ gMarvellTokenSpaceGuid.PcdChip3MppReverseFlag|FALSE
+ gMarvellTokenSpaceGuid.PcdChip3MppBaseAddress|0xF6440000
+ gMarvellTokenSpaceGuid.PcdChip3MppPinCount|64
+ gMarvellTokenSpaceGuid.PcdChip3MppSel0|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip3MppSel1|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip3MppSel2|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x9, 0x9, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip3MppSel3|{ 0x0, 0x0, 0x8, 0x0, 0x8, 0x0, 0x0, 0x2, 0x2, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip3MppSel4|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip3MppSel5|{ 0x0, 0x0, 0x0, 0x0, 0xA, 0xB, 0xE, 0xE, 0xE, 0xE }
+ gMarvellTokenSpaceGuid.PcdChip3MppSel6|{ 0xE, 0xE, 0xE, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+
+ # ComPhy
+ gMarvellTokenSpaceGuid.PcdComPhyDevices|{ 0x1, 0x1, 0x1 }
+ # ComPhy1
+ # 0: PCIE0 5 Gbps
+ # 1: PCIE0 5 Gbps
+ # 2: SATA0 5 Gbps
+ # 3: USB3_HOST1 5 Gbps
+ # 4: SFI 10.31 Gbps
+ # 5: PCIE2 5 Gbps
+ gMarvellTokenSpaceGuid.PcdChip2ComPhyTypes|{ $(CP_PCIE0), $(CP_PCIE0), $(CP_SATA0), $(CP_USB3_HOST1), $(CP_SFI), $(CP_PCIE2)}
+ gMarvellTokenSpaceGuid.PcdChip2ComPhySpeeds|{ $(CP_5G), $(CP_5G), $(CP_5G), $(CP_5G), $(CP_10_3125G), $(CP_5G) }
+
+ # UtmiPhy
+ gMarvellTokenSpaceGuid.PcdUtmiControllersEnabled|{ 0x1, 0x1, 0x0, 0x1, 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdUtmiPortType|{ $(UTMI_USB_HOST0), $(UTMI_USB_HOST1), $(UTMI_USB_HOST0), $(UTMI_USB_HOST1), $(UTMI_USB_HOST0), $(UTMI_USB_HOST1) }
+
+ # MDIO
+ gMarvellTokenSpaceGuid.PcdMdioControllersEnabled|{ 0x1, 0x0 }
+
+ # PHY
+ gMarvellTokenSpaceGuid.PcdPhy2MdioController|{ 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPhyDeviceIds|{ 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPhySmiAddresses|{ 0x0, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg|FALSE
+
+ # NET
+ gMarvellTokenSpaceGuid.PcdPp2GopIndexes|{ 0x0, 0x2, 0x3, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPp2InterfaceAlwaysUp|{ 0x0, 0x0, 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPp2InterfaceSpeed|{ $(PHY_SPEED_10000), $(PHY_SPEED_1000), $(PHY_SPEED_1000), $(PHY_SPEED_10000), $(PHY_SPEED_10000) }
+ gMarvellTokenSpaceGuid.PcdPp2PhyConnectionTypes|{ $(PHY_SFI), $(PHY_RGMII), $(PHY_RGMII), $(PHY_SFI), $(PHY_SFI) }
+ gMarvellTokenSpaceGuid.PcdPp2PhyIndexes|{ 0xFF, 0x0, 0x1, 0xFF, 0xFF }
+ gMarvellTokenSpaceGuid.PcdPp2Port2Controller|{ 0x0, 0x0, 0x0, 0x1, 0x2 }
+ gMarvellTokenSpaceGuid.PcdPp2PortIds|{ 0x0, 0x1, 0x2, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPp2Controllers|{ 0x1, 0x1, 0x1 }
+
+ # NonDiscoverableDevices
+ gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x1, 0x1, 0x0, 0x1, 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1, 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x1, 0x1, 0x0, 0x1 }
diff --git a/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc b/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc
index 5aca5a1..1b28fae 100644
--- a/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc
+++ b/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc
@@ -13,7 +13,9 @@
#
################################################################################
[Defines]
-!if $(CN9131)
+!if $(CN9132)
+ PLATFORM_NAME = Cn9132DbA
+!elseif $(CN9131)
PLATFORM_NAME = Cn9131DbA
!else
PLATFORM_NAME = Cn9130DbA
@@ -38,16 +40,25 @@

!include Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
!include Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc
-!if $(CN9131)
+!if $(CN9131) || $(CN9132)
!include Platform/Marvell/Cn913xDb/Cn9131DbA.dsc.inc
!endif
+!if $(CN9132)
+!include Platform/Marvell/Cn913xDb/Cn9132DbA.dsc.inc
+!endif

[Components.common]
Silicon/Marvell/OcteonTx/DeviceTree/T91/$(PLATFORM_NAME).inf

+!ifndef $(CN9132)
[Components.AARCH64]
Silicon/Marvell/OcteonTx/AcpiTables/T91/$(PLATFORM_NAME).inf
+!endif

[LibraryClasses.common]
+!if $(CN9132)
+ ArmadaBoardDescLib|Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.inf
+!else
ArmadaBoardDescLib|Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.inf
+!endif
NonDiscoverableInitLib|Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
diff --git a/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.inf b/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.inf
new file mode 100644
index 0000000..27a0214
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.inf
@@ -0,0 +1,29 @@
+## @file
+#
+# Copyright (C) 2019, Marvell International Ltd. and its affiliates<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = Cn9132DbABoardDescLib
+ FILE_GUID = cf7a0f12-45fe-417b-9c34-053605973b68
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmadaBoardDescLib
+
+[Sources]
+ Cn9132DbABoardDescLib.c
+
+[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Silicon/Marvell/Marvell.dec
+
+[LibraryClasses]
+ DebugLib
+ IoLib
diff --git a/Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9132DbA.inf b/Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9132DbA.inf
new file mode 100644
index 0000000..c9e3b04
--- /dev/null
+++ b/Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9132DbA.inf
@@ -0,0 +1,22 @@
+## @file
+#
+# Device tree description of the Marvell CN9130-DB-A platform
+#
+# Copyright (c) 2019, Marvell International Ltd. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = Cn9132DbADeviceTree
+ FILE_GUID = 25462CDA-221F-47DF-AC1D-259CFAA4E326 # gDtPlatformDefaultDtbFileGuid
+ MODULE_TYPE = USER_DEFINED
+ VERSION_STRING = 1.0
+
+[Sources]
+ cn9132-db-A.dts
+
+[Packages]
+ MdePkg/MdePkg.dec
diff --git a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h
index 6618737..084bea0 100644
--- a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h
+++ b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h
@@ -17,5 +17,9 @@
#define CN9130_DB_SDMMC_VCCQ_PIN 15
#define CN9131_DB_VBUS0_PIN 3
#define CN9131_DB_VBUS0_LIMIT_PIN 2
+#define CN9132_DB_VBUS0_PIN 2
+#define CN9132_DB_VBUS0_LIMIT_PIN 0
+#define CN9132_DB_VBUS1_PIN 3
+#define CN9132_DB_VBUS1_LIMIT_PIN 1

#endif
diff --git a/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.c b/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.c
new file mode 100644
index 0000000..d2846dd
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9132DbABoardDescLib.c
@@ -0,0 +1,135 @@
+/**
+*
+* Copyright (C) 2019, Marvell International Ltd. and its affiliates.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <Uefi.h>
+
+#include <Library/ArmadaBoardDescLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/IoLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/MvGpioLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+//
+// GPIO Expander
+//
+STATIC MV_GPIO_EXPANDER mGpioExpander = {
+ PCA9555_ID,
+ 0x21,
+ 0x0,
+};
+
+
+EFI_STATUS
+EFIAPI
+ArmadaBoardGpioExpanderGet (
+ IN OUT MV_GPIO_EXPANDER **GpioExpanders,
+ IN OUT UINTN *GpioExpanderCount
+ )
+{
+ *GpioExpanderCount = 1;
+ *GpioExpanders = &mGpioExpander;
+
+ return EFI_SUCCESS;
+}
+
+//
+// PCIE
+//
+STATIC
+MV_PCIE_CONTROLLER mPcieController[] = {
+ { /* PCIE0 @0xF2640000 */
+ .PcieDbiAddress = 0xF2600000,
+ .ConfigSpaceAddress = 0xD0000000,
+ .HaveResetGpio = FALSE,
+ .PcieResetGpio = { 0 },
+ .PcieBusMin = 0,
+ .PcieBusMax = 0xFE,
+ .PcieIoTranslation = 0xDFF00000,
+ .PcieIoWinBase = 0x0,
+ .PcieIoWinSize = 0x10000,
+ .PcieMmio32Translation = 0,
+ .PcieMmio32WinBase = 0xC0000000,
+ .PcieMmio32WinSize = 0x10000000,
+ .PcieMmio64Translation = 0,
+ .PcieMmio64WinBase = MAX_UINT64,
+ .PcieMmio64WinSize = 0,
+ }
+};
+
+/**
+ Return the number and description of PCIE controllers used on the platform.
+
+ @param[in out] **PcieControllers Array containing PCIE controllers'
+ description.
+ @param[in out] *PcieControllerCount Amount of used PCIE controllers.
+
+ @retval EFI_SUCCESS The data were obtained successfully.
+ @retval other Return error status.
+
+**/
+EFI_STATUS
+EFIAPI
+ArmadaBoardPcieControllerGet (
+ IN OUT MV_PCIE_CONTROLLER CONST **PcieControllers,
+ IN OUT UINTN *PcieControllerCount
+ )
+{
+ *PcieControllers = mPcieController;
+ *PcieControllerCount = ARRAY_SIZE (mPcieController);
+
+ return EFI_SUCCESS;
+}
+
+//
+// Order of devices in SdMmcDescTemplate has to be in par with ArmadaSoCDescLib
+//
+STATIC
+MV_BOARD_SDMMC_DESC mSdMmcDescTemplate[] = {
+ { /* eMMC 0xF06E0000 */
+ 0, /* SOC will be filled by MvBoardDescDxe */
+ 0, /* SdMmcDevCount will be filled by MvBoardDescDxe */
+ FALSE, /* Xenon1v8Enabled */
+ TRUE, /* Xenon8BitBusEnabled */
+ FALSE, /* XenonSlowModeEnabled */
+ 0x40, /* XenonTuningStepDivisor */
+ EmbeddedSlot /* SlotType */
+ },
+ { /* SD/MMC 0xF2780000 */
+ 0, /* SOC will be filled by MvBoardDescDxe */
+ 0, /* SdMmcDevCount will be filled by MvBoardDescDxe */
+ FALSE, /* Xenon1v8Enabled */
+ FALSE, /* Xenon8BitBusEnabled */
+ FALSE, /* XenonSlowModeEnabled */
+ 0x19, /* XenonTuningStepDivisor */
+ EmbeddedSlot /* SlotType */
+ },
+ { /* SD/MMC 0xF6780000 */
+ 0, /* SOC will be filled by MvBoardDescDxe */
+ 0, /* SdMmcDevCount will be filled by MvBoardDescDxe */
+ FALSE, /* Xenon1v8Enabled */
+ FALSE, /* Xenon8BitBusEnabled */
+ FALSE, /* XenonSlowModeEnabled */
+ 0x19, /* XenonTuningStepDivisor */
+ EmbeddedSlot /* SlotType */
+ }
+};
+
+EFI_STATUS
+EFIAPI
+ArmadaBoardDescSdMmcGet (
+ OUT UINTN *SdMmcDevCount,
+ OUT MV_BOARD_SDMMC_DESC **SdMmcDesc
+ )
+{
+ *SdMmcDesc = mSdMmcDescTemplate;
+ *SdMmcDevCount = ARRAY_SIZE (mSdMmcDescTemplate);
+
+ return EFI_SUCCESS;
+}
diff --git a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c
index dded150..42dc54a 100644
--- a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c
+++ b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c
@@ -118,6 +118,45 @@ Cp1XhciInit (
MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER);
}

+STATIC CONST MV_GPIO_PIN mCp2XhciVbusPins[] = {
+ {
+ MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
+ MV_GPIO_CP2_CONTROLLER0,
+ CN9132_DB_VBUS0_PIN,
+ TRUE,
+ },
+ {
+ MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
+ MV_GPIO_CP2_CONTROLLER0,
+ CN9132_DB_VBUS0_LIMIT_PIN,
+ TRUE,
+ },
+ {
+ MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
+ MV_GPIO_CP2_CONTROLLER0,
+ CN9132_DB_VBUS1_PIN,
+ TRUE,
+ },
+ {
+ MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
+ MV_GPIO_CP2_CONTROLLER0,
+ CN9132_DB_VBUS1_LIMIT_PIN,
+ TRUE,
+ },
+};
+
+STATIC
+EFI_STATUS
+EFIAPI
+Cp2XhciInit (
+ IN NON_DISCOVERABLE_DEVICE *This
+ )
+{
+ return ConfigurePins (mCp2XhciVbusPins,
+ ARRAY_SIZE (mCp2XhciVbusPins),
+ MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER);
+}
+
STATIC CONST MV_GPIO_PIN mCp0SdMmcPins[] = {
{
MV_GPIO_DRIVER_TYPE_PCA95XX,
@@ -159,6 +198,9 @@ NonDiscoverableDeviceInitializerGet (
return Cp0XhciInit;
case 2:
return Cp1XhciInit;
+ case 3:
+ case 4:
+ return Cp2XhciInit;
}
}

diff --git a/Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc b/Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc
index 0c321d1..78bdb79 100644
--- a/Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc
+++ b/Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc
@@ -12,7 +12,9 @@
# DTB
INF RuleOverride = DTB Silicon/Marvell/OcteonTx/DeviceTree/T91/$(PLATFORM_NAME).inf

+!ifndef $(CN9132)
!if $(ARCH) == AARCH64
# ACPI support
INF RuleOverride = ACPITABLE Silicon/Marvell/OcteonTx/AcpiTables/T91/$(PLATFORM_NAME).inf
!endif
+!endif
diff --git a/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db-A.dts b/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db-A.dts
index e9464f8..724d7dc 100644
--- a/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db-A.dts
+++ b/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db-A.dts
@@ -55,12 +55,6 @@

&cp2_sata0 {
status = "okay";
- /* SLM-1521-V2, CON4 */
- sata-port@0 {
- status = "okay";
- /* Generic PHY, providing serdes lanes */
- phys = <&cp2_comphy2 0>;
- };
};

/* CON 2 on SLM-1683 - microSD */
diff --git a/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db.dtsi b/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db.dtsi
index 8613607..7dc6c6e 100644
--- a/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db.dtsi
+++ b/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9132-db.dtsi
Same license comment - break out and submit for edk2-non-osi?

/
Leif

@@ -6,15 +6,23 @@
*/

#undef CP110_NUM
-#undef CP110_PCIE_MEM_SIZE
+#undef CP110_NAME
+#undef CP110_BASE
+#undef CP110_PCIE0_BASE
+#undef CP110_PCIE1_BASE
+#undef CP110_PCIE2_BASE
#undef CP110_PCIEx_CPU_MEM_BASE
-#undef CP110_PCIEx_BUS_MEM_BASE
+#undef CP110_PCIEx_MEM_BASE

/* CP110-1 Settings */
+#define CP110_NAME cp2
#define CP110_NUM 2
-#define CP110_PCIE_MEM_SIZE(iface) (0xf00000)
-#define CP110_PCIEx_CPU_MEM_BASE(iface) (0xe5000000 + (iface) * 0x1000000)
-#define CP110_PCIEx_BUS_MEM_BASE(iface) (CP110_PCIEx_CPU_MEM_BASE(iface))
+#define CP110_BASE f6000000
+#define CP110_PCIE0_BASE f6600000
+#define CP110_PCIE1_BASE f6620000
+#define CP110_PCIE2_BASE f6640000
+#define CP110_PCIEx_CPU_MEM_BASE(iface) (0xe5000000 + (iface) * 0x1000000)
+#define CP110_PCIEx_MEM_BASE(iface) (CP110_PCIEx_CPU_MEM_BASE(iface))

#include "armada-cp110.dtsi"

@@ -124,7 +132,7 @@

&cp2_syscon0 {
cp2_pinctrl: pinctrl {
- compatible = "marvell,cp115-standalone-pinctrl";
+ compatible = "marvell,armada-7k-pinctrl";

cp2_i2c0_pins: cp2-i2c-pins-0 {
marvell,pins = "mpp37", "mpp38";
--
2.7.4


Re: [edk2-platforms: PATCH v2 08/10] Marvell/Cn9131Db: Introduce board support

Leif Lindholm
 

On Thu, Aug 15, 2019 at 04:54:12AM +0200, Marcin Wojtas wrote:
This patch introduces all necessary components required
for building EDK2 firmware for CN9131-DB setup A.

In order to build this variant, '-D CN9131' flag should be added.
Otherwise the default (CN9130) will be compiled.
Yeah, see comment on the CN9130 .dsc - if you change that as
requested, don't forget to update this commit message.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Cn913xDb/Cn9131DbA.dsc.inc | 72 ++++++++++++++
Platform/Marvell/Cn913xDb/Cn913xDbA.dsc | 7 ++
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA.inf | 57 ++++++++++++
Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9131DbA.inf | 22 +++++
Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h | 2 +
Silicon/Marvell/OcteonTx/AcpiTables/T91/AcpiHeader.h | 2 +
Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c | 29 ++++++
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl | 98 ++++++++++++++++++++
Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9131-db.dtsi | 26 +++---
9 files changed, 303 insertions(+), 12 deletions(-)
create mode 100644 Platform/Marvell/Cn913xDb/Cn9131DbA.dsc.inc
create mode 100644 Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA.inf
create mode 100644 Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9131DbA.inf
create mode 100644 Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl

diff --git a/Platform/Marvell/Cn913xDb/Cn9131DbA.dsc.inc b/Platform/Marvell/Cn913xDb/Cn9131DbA.dsc.inc
new file mode 100644
index 0000000..7235b9f
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/Cn9131DbA.dsc.inc
@@ -0,0 +1,72 @@
+## @file
+# Component description file for the CN9131 Development Board (variant A)
+#
+# Copyright (c) 2019 Marvell International Ltd.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+################################################################################
+#
+# Pcd Section - list of all EDK II PCD Entries defined by this Platform
+#
+################################################################################
+[PcdsFixedAtBuild.common]
+ # CP115 count
+ gMarvellTokenSpaceGuid.PcdMaxCpCount|2
+
+ # MPP
+ gMarvellTokenSpaceGuid.PcdMppChipCount|3
+
+ # CP115 #1 MPP
+ gMarvellTokenSpaceGuid.PcdChip2MppReverseFlag|FALSE
+ gMarvellTokenSpaceGuid.PcdChip2MppBaseAddress|0xF4440000
+ gMarvellTokenSpaceGuid.PcdChip2MppPinCount|64
+ gMarvellTokenSpaceGuid.PcdChip2MppSel0|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip2MppSel1|{ 0x0, 0x0, 0x0, 0x3, 0x3, 0x3, 0x3, 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip2MppSel2|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x9, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip2MppSel3|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x7, 0x7, 0x2, 0x2, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip2MppSel4|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip2MppSel5|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdChip2MppSel6|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+
+ # ComPhy
+ gMarvellTokenSpaceGuid.PcdComPhyDevices|{ 0x1, 0x1 }
+ # ComPhy1
+ # 0: PCIE0 5 Gbps
+ # 1: PCIE0 5 Gbps
+ # 2: UNCONNECTED
+ # 3: USB3_HOST1 5 Gbps
+ # 4: SFI 10.31 Gbps
+ # 5: SATA1 5 Gbps
+ gMarvellTokenSpaceGuid.PcdChip1ComPhyTypes|{ $(CP_PCIE0), $(CP_PCIE0), $(CP_UNCONNECTED), $(CP_USB3_HOST1), $(CP_SFI), $(CP_SATA1)}
+ gMarvellTokenSpaceGuid.PcdChip1ComPhySpeeds|{ $(CP_5G), $(CP_5G), $(CP_DEFAULT), $(CP_5G), $(CP_10_3125G), $(CP_5G) }
+
+ # UtmiPhy
+ gMarvellTokenSpaceGuid.PcdUtmiControllersEnabled|{ 0x1, 0x1, 0x0, 0x1 }
+ gMarvellTokenSpaceGuid.PcdUtmiPortType|{ $(UTMI_USB_HOST0), $(UTMI_USB_HOST1), $(UTMI_USB_HOST0), $(UTMI_USB_HOST1) }
+
+ # MDIO
+ gMarvellTokenSpaceGuid.PcdMdioControllersEnabled|{ 0x1, 0x0 }
+
+ # PHY
+ gMarvellTokenSpaceGuid.PcdPhy2MdioController|{ 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPhyDeviceIds|{ 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPhySmiAddresses|{ 0x0, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg|FALSE
+
+ # NET
+ gMarvellTokenSpaceGuid.PcdPp2GopIndexes|{ 0x0, 0x2, 0x3, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPp2InterfaceAlwaysUp|{ 0x0, 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPp2InterfaceSpeed|{ $(PHY_SPEED_10000), $(PHY_SPEED_1000), $(PHY_SPEED_1000), $(PHY_SPEED_10000) }
+ gMarvellTokenSpaceGuid.PcdPp2PhyConnectionTypes|{ $(PHY_SFI), $(PHY_RGMII), $(PHY_RGMII), $(PHY_SFI) }
+ gMarvellTokenSpaceGuid.PcdPp2PhyIndexes|{ 0xFF, 0x0, 0x1, 0xFF }
+ gMarvellTokenSpaceGuid.PcdPp2Port2Controller|{ 0x0, 0x0, 0x0, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPp2PortIds|{ 0x0, 0x1, 0x2, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPp2Controllers|{ 0x1, 0x1 }
+
+ # NonDiscoverableDevices
+ gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x1, 0x1, 0x0, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x1, 0x1 }
diff --git a/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc b/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc
index d77785d..5aca5a1 100644
--- a/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc
+++ b/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc
@@ -13,7 +13,11 @@
#
################################################################################
[Defines]
+!if $(CN9131)
+ PLATFORM_NAME = Cn9131DbA
+!else
PLATFORM_NAME = Cn9130DbA
+!endif
PLATFORM_GUID = 087305a1-8ddd-4027-89ca-68a3ef78fcc7
PLATFORM_VERSION = 0.1
DSC_SPECIFICATION = 0x0001000B
@@ -34,6 +38,9 @@

!include Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
!include Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc
+!if $(CN9131)
+!include Platform/Marvell/Cn913xDb/Cn9131DbA.dsc.inc
+!endif

[Components.common]
Silicon/Marvell/OcteonTx/DeviceTree/T91/$(PLATFORM_NAME).inf
diff --git a/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA.inf b/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA.inf
new file mode 100644
index 0000000..bbf1b51
--- /dev/null
+++ b/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA.inf
@@ -0,0 +1,57 @@
+## @file
+# Component description file for PlatformAcpiTables module.
+#
+# ACPI table data and ASL sources required to boot the platform.
+#
+# Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
+# Copyright (c) 2019, Marvell International Ltd. and its affiliates.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = PlatformAcpiTables
+ FILE_GUID = 7E374E25-8E01-4FEE-87F2-390C23C606CD
+ MODULE_TYPE = USER_DEFINED
+ VERSION_STRING = 1.0
+
+[Sources]
+ Cn9131DbA/Ssdt.asl
+ Cn913xDbA/Dsdt.asl
+ Cn913xDbA/Mcfg.aslc
+ Fadt.aslc
+ Gtdt.aslc
+ Madt.aslc
+ Pptt.aslc
+ Spcr.aslc
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ ArmPlatformPkg/ArmPlatformPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Silicon/Marvell/Marvell.dec
+
+[FixedPcd]
+ gArmPlatformTokenSpaceGuid.PcdCoreCount
+
+ gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum
+ gArmTokenSpaceGuid.PcdArmArchTimerIntrNum
+ gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum
+ gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum
+
+ gArmTokenSpaceGuid.PcdGenericWatchdogControlBase
+ gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum
+ gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase
+
+ gArmTokenSpaceGuid.PcdGicDistributorBase
+ gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase
+
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase
+
+[BuildOptions]
+ *_*_*_ASLCC_FLAGS = -DCN9131
diff --git a/Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9131DbA.inf b/Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9131DbA.inf
new file mode 100644
index 0000000..8108197
--- /dev/null
+++ b/Silicon/Marvell/OcteonTx/DeviceTree/T91/Cn9131DbA.inf
@@ -0,0 +1,22 @@
+## @file
+#
+# Device tree description of the Marvell CN9130-DB-A platform
+#
+# Copyright (c) 2019, Marvell International Ltd. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = Cn9131DbADeviceTree
+ FILE_GUID = 25462CDA-221F-47DF-AC1D-259CFAA4E326 # gDtPlatformDefaultDtbFileGuid
+ MODULE_TYPE = USER_DEFINED
+ VERSION_STRING = 1.0
+
+[Sources]
+ cn9131-db-A.dts
+
+[Packages]
+ MdePkg/MdePkg.dec
diff --git a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h
index 2533c35..6618737 100644
--- a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h
+++ b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h
@@ -15,5 +15,7 @@
#define CN9130_DB_VBUS1_LIMIT_PIN 5
#define CN9130_DB_SDMMC_VCC_PIN 14
#define CN9130_DB_SDMMC_VCCQ_PIN 15
+#define CN9131_DB_VBUS0_PIN 3
+#define CN9131_DB_VBUS0_LIMIT_PIN 2

#endif
diff --git a/Silicon/Marvell/OcteonTx/AcpiTables/T91/AcpiHeader.h b/Silicon/Marvell/OcteonTx/AcpiTables/T91/AcpiHeader.h
index b5fd397..2838676 100644
--- a/Silicon/Marvell/OcteonTx/AcpiTables/T91/AcpiHeader.h
+++ b/Silicon/Marvell/OcteonTx/AcpiTables/T91/AcpiHeader.h
@@ -18,6 +18,8 @@

#if defined(CN9130)
#define ACPI_OEM_TABLE_ID SIGNATURE_64('C','N','9','1','3','0',' ',' ')
+#elif defined (CN9131)
+#define ACPI_OEM_TABLE_ID SIGNATURE_64('C','N','9','1','3','1',' ',' ')
#endif

/**
diff --git a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c
index 598c649..dded150 100644
--- a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c
+++ b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c
@@ -91,6 +91,33 @@ Cp0XhciInit (
MV_GPIO_DRIVER_TYPE_PCA95XX);
}

+STATIC CONST MV_GPIO_PIN mCp1XhciVbusPins[] = {
+ {
+ MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
+ MV_GPIO_CP1_CONTROLLER0,
+ CN9131_DB_VBUS0_PIN,
+ TRUE,
+ },
+ {
+ MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
+ MV_GPIO_CP1_CONTROLLER0,
+ CN9131_DB_VBUS0_LIMIT_PIN,
+ TRUE,
+ },
+};
+
+STATIC
+EFI_STATUS
+EFIAPI
+Cp1XhciInit (
+ IN NON_DISCOVERABLE_DEVICE *This
+ )
+{
+ return ConfigurePins (mCp1XhciVbusPins,
+ ARRAY_SIZE (mCp1XhciVbusPins),
+ MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER);
+}
+
STATIC CONST MV_GPIO_PIN mCp0SdMmcPins[] = {
{
MV_GPIO_DRIVER_TYPE_PCA95XX,
@@ -130,6 +157,8 @@ NonDiscoverableDeviceInitializerGet (
case 0:
case 1:
return Cp0XhciInit;
+ case 2:
+ return Cp1XhciInit;
}
}

diff --git a/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl b/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl
new file mode 100644
index 0000000..99bc751
--- /dev/null
+++ b/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl
@@ -0,0 +1,98 @@
+/** @file
+
+ Secondary System Description Table Fields (SSDT)
+
+ Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
+ Copyright (c) 2019, Marvell International Ltd. and its affiliates.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "IcuInterrupts.h"
+
+DefinitionBlock ("Cn9131DbASsdt.aml", "SSDT", 2, "MVEBU ", "CN9131", 3)
+{
+ Scope (_SB)
+ {
+ Device (AHC1)
+ {
+ Name (_HID, "LNRO001E") // _HID: Hardware ID
+ Name (_UID, 0x00) // _UID: Unique ID
+ Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Name (_CLS, Package (0x03) // _CLS: Class Code
+ {
+ 0x01,
+ 0x06,
+ 0x01
+ })
+
+ Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
+ {
+ Memory32Fixed (ReadWrite,
+ 0xF4540000, // Address Base (MMIO)
+ 0x00030000, // Address Length
+ )
+ Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
+ {
+ CP_GIC_SPI_CP1_SATA_H0
+ }
+ })
+ }
+
+ Device (XHC2)
+ {
+ Name (_HID, "PNP0D10") // _HID: Hardware ID
+ Name (_UID, 0x01) // _UID: Unique ID
+ Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+
+ Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
+ {
+ Memory32Fixed (ReadWrite,
+ 0xF4510000, // Address Base (MMIO)
+ 0x00004000, // Address Length
+ )
+ Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
+ {
+ CP_GIC_SPI_CP1_USB_H1
+ }
+ })
+ }
+ Device (PP21)
+ {
+ Name (_HID, "MRVL0110") // _HID: Hardware ID
+ Name (_CCA, 0x01) // Cache-coherent controller
+ Name (_UID, 0x00) // _UID: Unique ID
+ Name (_CRS, ResourceTemplate ()
+ {
+ Memory32Fixed (ReadWrite, 0xf4000000 , 0x100000)
+ Memory32Fixed (ReadWrite, 0xf4129000 , 0xb000)
+ })
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () { "clock-frequency", 333333333 },
+ }
+ })
+ Device (ETH0)
+ {
+ Name (_ADR, 0x0)
+ Name (_CRS, ResourceTemplate ()
+ {
+ Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
+ {
+ CP_GIC_SPI_PP2_CP1_PORT0
+ }
+ })
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () { "port-id", 0 },
+ Package () { "gop-port-id", 0 },
+ Package () { "phy-mode", "10gbase-kr"},
+ }
+ })
+ }
+ }
+ }
+}
diff --git a/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9131-db.dtsi b/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9131-db.dtsi
index c8e425a..9c9dfb6 100644
--- a/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9131-db.dtsi
+++ b/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9131-db.dtsi
So, based on previous license comment, I think this needs to be broken
out as a separate patch (and go into edk2-non-osi).

/
Leif

@@ -6,15 +6,23 @@
*/

#undef CP110_NUM
-#undef CP110_PCIE_MEM_SIZE
+#undef CP110_NAME
+#undef CP110_BASE
+#undef CP110_PCIE0_BASE
+#undef CP110_PCIE1_BASE
+#undef CP110_PCIE2_BASE
#undef CP110_PCIEx_CPU_MEM_BASE
-#undef CP110_PCIEx_BUS_MEM_BASE
+#undef CP110_PCIEx_MEM_BASE

/* CP110-1 Settings */
+#define CP110_NAME cp1
#define CP110_NUM 1
-#define CP110_PCIE_MEM_SIZE(iface) (0xf00000)
-#define CP110_PCIEx_CPU_MEM_BASE(iface) (0xe2000000 + (iface) * 0x1000000)
-#define CP110_PCIEx_BUS_MEM_BASE(iface) (CP110_PCIEx_CPU_MEM_BASE(iface))
+#define CP110_BASE f4000000
+#define CP110_PCIE0_BASE f4600000
+#define CP110_PCIE1_BASE f4620000
+#define CP110_PCIE2_BASE f4640000
+#define CP110_PCIEx_CPU_MEM_BASE(iface) (0xe2000000 + (iface) * 0x1000000)
+#define CP110_PCIEx_MEM_BASE(iface) (CP110_PCIEx_CPU_MEM_BASE(iface))

#include "armada-cp110.dtsi"

@@ -93,12 +101,6 @@

&cp1_sata0 {
status = "okay";
- /* CON32 */
- sata-port@1 {
- status = "okay";
- /* Generic PHY, providing serdes lanes */
- phys = <&cp1_comphy5 1>;
- };
};

/* U24 */
@@ -138,7 +140,7 @@

&cp1_syscon0 {
cp1_pinctrl: pinctrl {
- compatible = "marvell,cp115-standalone-pinctrl";
+ compatible = "marvell,armada-7k-pinctrl";

cp1_i2c0_pins: cp1-i2c-pins-0 {
marvell,pins = "mpp37", "mpp38";
--
2.7.4


Re: [edk2-platforms: PATCH v2 07/10] Marvell/Library: IcuLib: Fix debug information

Leif Lindholm
 

On Thu, Aug 15, 2019 at 04:54:11AM +0200, Marcin Wojtas wrote:
In case the number of CP11x components exceeded the maximum
of currently supported, the user is informed with the information.
It turned out that the print arguments were incorrect - fix it.
Whoops.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

---
Silicon/Marvell/Library/IcuLib/IcuLib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.c b/Silicon/Marvell/Library/IcuLib/IcuLib.c
index 422388c..8d99409 100644
--- a/Silicon/Marvell/Library/IcuLib/IcuLib.c
+++ b/Silicon/Marvell/Library/IcuLib/IcuLib.c
@@ -285,8 +285,8 @@ ArmadaIcuInitialize (
if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
DEBUG ((DEBUG_ERROR,
"%a: Default ICU to GIC mapping is available for maximum %d CP110 units",
- ICU_MAX_SUPPORTED_UNITS,
- __FUNCTION__));
+ __FUNCTION__,
+ ICU_MAX_SUPPORTED_UNITS));
CpCount = ICU_MAX_SUPPORTED_UNITS;
}

--
2.7.4


Re: [edk2-platforms: PATCH v2 06/10] Marvell/Library: MppLib: Allow to configure more Xenon PHYs

Leif Lindholm
 

On Thu, Aug 15, 2019 at 04:54:10AM +0200, Marcin Wojtas wrote:
Hitherto MppLib code assumed that there could be only two
Xenon SdMmc controllers' PHYs. Remove this limitation, so that to
support CN913x SoCs, which may have up to 4 of such interfaces.
Should this be merged with the preceding patch?

/
Leif

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Silicon/Marvell/Library/MppLib/MppLib.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/Silicon/Marvell/Library/MppLib/MppLib.c b/Silicon/Marvell/Library/MppLib/MppLib.c
index 40d9077..f20668d 100644
--- a/Silicon/Marvell/Library/MppLib/MppLib.c
+++ b/Silicon/Marvell/Library/MppLib/MppLib.c
@@ -139,11 +139,9 @@ SetSdMmcPhyMpp (
case 0:
Offset = SD_MMC_PHY_AP_MPP_OFFSET;
break;
- case 1:
+ default:
Offset = SD_MMC_PHY_CP0_MPP_OFFSET;
break;
- default:
- return;
}

/*
--
2.7.4




Re: [edk2-platforms: PATCH v2 05/10] Marvell/Library: ArmadaSoCDescLib: Extend Xenon information

Leif Lindholm
 

On Thu, Aug 15, 2019 at 04:54:09AM +0200, Marcin Wojtas wrote:
Hitherto SoC description library code assumed that there could
be only two Xenon SdMmc controller instances in the SoC. Remove this
limitation, so that to support CN913x SoCs, which may have up to 4 of
such interfaces.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

---
Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h | 5 +--
Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 34 +++++++++++++-------
2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
index 0296d43..265b4f4 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
@@ -90,8 +90,9 @@
//
// Platform description of SDMMC controllers
//
-#define MV_SOC_MAX_SDMMC_COUNT 2
-#define MV_SOC_SDMMC_BASE(Index) ((Index) == 0 ? 0xF06E0000 : 0xF2780000)
+#define MV_SOC_SDMMC_PER_CP_COUNT 1
+#define MV_SOC_AP80X_SDMMC_BASE 0xF06E0000
+#define MV_SOC_CP_SDMMC_BASE(Cp) (MV_SOC_CP_BASE (Cp) + 0x780000)

//
// Platform description of UTMI PHY's
diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
index 5947601..3ffd57e 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
@@ -349,26 +349,36 @@ EFI_STATUS
EFIAPI
ArmadaSoCDescSdMmcGet (
IN OUT MV_SOC_SDMMC_DESC **SdMmcDesc,
- IN OUT UINTN *DescCount
+ IN OUT UINTN *Count
)
{
- MV_SOC_SDMMC_DESC *Desc;
- UINTN Index;
+ MV_SOC_SDMMC_DESC *SdMmc;
+ UINTN CpCount, CpIndex;

- Desc = AllocateZeroPool (MV_SOC_MAX_SDMMC_COUNT * sizeof (MV_SOC_SDMMC_DESC));
- if (Desc == NULL) {
+ CpCount = FixedPcdGet8 (PcdMaxCpCount);
+
+ *Count = CpCount * MV_SOC_SDMMC_PER_CP_COUNT + MV_SOC_AP806_COUNT;
+ SdMmc = AllocateZeroPool (*Count * sizeof (MV_SOC_SDMMC_DESC));
+ if (SdMmc == NULL) {
DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
return EFI_OUT_OF_RESOURCES;
}

- for (Index = 0; Index < MV_SOC_MAX_SDMMC_COUNT; Index++) {
- Desc[Index].SdMmcBaseAddress = MV_SOC_SDMMC_BASE (Index);
- Desc[Index].SdMmcMemSize = SIZE_1KB;
- Desc[Index].SdMmcDmaType = NonDiscoverableDeviceDmaTypeCoherent;
- }
+ *SdMmcDesc = SdMmc;
+
+ /* AP80x controller */
+ SdMmc->SdMmcBaseAddress = MV_SOC_AP80X_SDMMC_BASE;
+ SdMmc->SdMmcMemSize = SIZE_1KB;
+ SdMmc->SdMmcDmaType = NonDiscoverableDeviceDmaTypeCoherent;
+ SdMmc++;

- *SdMmcDesc = Desc;
- *DescCount = MV_SOC_MAX_SDMMC_COUNT;
+ /* CP11x controllers */
+ for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
+ SdMmc->SdMmcBaseAddress = MV_SOC_CP_SDMMC_BASE (CpIndex);
+ SdMmc->SdMmcMemSize = SIZE_1KB;
+ SdMmc->SdMmcDmaType = NonDiscoverableDeviceDmaTypeCoherent;
+ SdMmc++;
+ }

return EFI_SUCCESS;
}
--
2.7.4


Re: [edk2-platforms: PATCH v2 04/10] Marvell/Cn9130Db: Introduce board support

Leif Lindholm
 

On Thu, Aug 15, 2019 at 04:54:08AM +0200, Marcin Wojtas wrote:
This patch introduces all necessary components required
for building EDK2 firmware for CN9130-DB setup A.
Because the board is modular and can be extended to support
also CN9131 and CN9132 SoC variants, extract common part into
.dsc.inc file, which will be included by them.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc | 107 +++++++++++++++
Platform/Marvell/Cn913xDb/Cn913xDbA.dsc | 46 +++++++
Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.inf | 29 ++++
Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.inf | 37 +++++
Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h | 19 +++
Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.c | 126 +++++++++++++++++
Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c | 144 ++++++++++++++++++++
Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc | 18 +++
8 files changed, 526 insertions(+)
create mode 100644 Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc
create mode 100644 Platform/Marvell/Cn913xDb/Cn913xDbA.dsc
create mode 100644 Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.inf
create mode 100644 Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
create mode 100644 Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h
create mode 100644 Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.c
create mode 100644 Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c
create mode 100644 Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc

diff --git a/Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc b/Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc
new file mode 100644
index 0000000..33fb7cc
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc
@@ -0,0 +1,107 @@
+## @file
+# Component description file for the CN9130 Development Board (variant A)
+#
+# Copyright (c) 2019 Marvell International Ltd.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+################################################################################
+#
+# Pcd Section - list of all EDK II PCD Entries defined by this Platform
+#
+################################################################################
+[PcdsFixedAtBuild.common]
+ # CP115 count
+ gMarvellTokenSpaceGuid.PcdMaxCpCount|1
+
+ # MPP
+ gMarvellTokenSpaceGuid.PcdMppChipCount|2
+
+ # APN807 MPP
+ gMarvellTokenSpaceGuid.PcdChip0MppReverseFlag|FALSE
+ gMarvellTokenSpaceGuid.PcdChip0MppBaseAddress|0xF06F4000
+ gMarvellTokenSpaceGuid.PcdChip0MppPinCount|20
+ gMarvellTokenSpaceGuid.PcdChip0MppSel0|{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdChip0MppSel1|{ 0x1, 0x3, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3 }
+
+ # CP115 #0 MPP
+ gMarvellTokenSpaceGuid.PcdChip1MppReverseFlag|FALSE
+ gMarvellTokenSpaceGuid.PcdChip1MppBaseAddress|0xF2440000
+ gMarvellTokenSpaceGuid.PcdChip1MppPinCount|64
+ gMarvellTokenSpaceGuid.PcdChip1MppSel0|{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, 0x3 }
+ gMarvellTokenSpaceGuid.PcdChip1MppSel1|{ 0x3, 0x3, 0x0, 0x3, 0x3, 0x3, 0x3, 0x1, 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdChip1MppSel2|{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x3, 0x9 }
+ gMarvellTokenSpaceGuid.PcdChip1MppSel3|{ 0x9, 0x3, 0x7, 0x6, 0x7, 0x2, 0x2, 0x2, 0x2, 0x1 }
+ gMarvellTokenSpaceGuid.PcdChip1MppSel4|{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdChip1MppSel5|{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, 0xE, 0xE, 0xE, 0xE }
+ gMarvellTokenSpaceGuid.PcdChip1MppSel6|{ 0xE, 0xE, 0xE, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+
+ # I2C
+ gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses|{ 0x21 }
+ gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0 }
+ gMarvellTokenSpaceGuid.PcdI2cControllersEnabled|{ 0x0, 0x1 }
+ gMarvellTokenSpaceGuid.PcdI2cClockFrequency|250000000
+ gMarvellTokenSpaceGuid.PcdI2cBaudRate|100000
+
+ # SPI
+ gMarvellTokenSpaceGuid.PcdSpiRegBase|0xF2700680
+ gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|10000000
+ gMarvellTokenSpaceGuid.PcdSpiClockFrequency|200000000
+
+ gMarvellTokenSpaceGuid.PcdSpiFlashMode|3
+ gMarvellTokenSpaceGuid.PcdSpiFlashCs|0
+
+ # ComPhy
+ gMarvellTokenSpaceGuid.PcdComPhyDevices|{ 0x1 }
+ # ComPhy0
+ # 0: PCIE0 5 Gbps
+ # 1: PCIE0 5 Gbps
+ # 2: PCIE0 5 Gbps
+ # 3: PCIE0 5 Gbps
+ # 4: SFI 10.31 Gbps
+ # 5: SATA1 5 Gbps
+ gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes|{ $(CP_PCIE0), $(CP_PCIE0), $(CP_PCIE0), $(CP_PCIE0), $(CP_SFI), $(CP_SATA1)}
+ gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds|{ $(CP_5G), $(CP_5G), $(CP_5G), $(CP_5G), $(CP_10_3125G), $(CP_5G) }
+
+ # UtmiPhy
+ gMarvellTokenSpaceGuid.PcdUtmiControllersEnabled|{ 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdUtmiPortType|{ $(UTMI_USB_HOST0), $(UTMI_USB_HOST1) }
+
+ # MDIO
+ gMarvellTokenSpaceGuid.PcdMdioControllersEnabled|{ 0x1, 0x0 }
+
+ # PHY
+ gMarvellTokenSpaceGuid.PcdPhy2MdioController|{ 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPhyDeviceIds|{ 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPhySmiAddresses|{ 0x0, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg|FALSE
+
+ # NET
+ gMarvellTokenSpaceGuid.PcdPp2GopIndexes|{ 0x0, 0x2, 0x3 }
+ gMarvellTokenSpaceGuid.PcdPp2InterfaceAlwaysUp|{ 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPp2InterfaceSpeed|{ $(PHY_SPEED_10000), $(PHY_SPEED_1000), $(PHY_SPEED_1000) }
+ gMarvellTokenSpaceGuid.PcdPp2PhyConnectionTypes|{ $(PHY_SFI), $(PHY_RGMII), $(PHY_RGMII) }
+ gMarvellTokenSpaceGuid.PcdPp2PhyIndexes|{ 0xFF, 0x0, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPp2Port2Controller|{ 0x0, 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPp2PortIds|{ 0x0, 0x1, 0x2 }
+ gMarvellTokenSpaceGuid.PcdPp2Controllers|{ 0x1 }
+
+ # NonDiscoverableDevices
+ gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1 }
+ gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x1, 0x1 }
+
+ # PCIE
+ gArmTokenSpaceGuid.PcdPciIoTranslation|0xDFF00000
+ gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xD0000000
+
+ # RTC
+ gMarvellTokenSpaceGuid.PcdRtcBaseAddress|0xF2284000
+
+ # SoC Configuration Space
+ gMarvellTokenSpaceGuid.PcdConfigSpaceBaseAddress|0xD0000000
+
+ # Variable store
+ gMarvellTokenSpaceGuid.PcdSpiMemoryMapped|FALSE
diff --git a/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc b/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc
new file mode 100644
index 0000000..d77785d
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc
@@ -0,0 +1,46 @@
+## @file
+# Component description file for the CN9130 Development Board (variant A)
+#
+# Copyright (c) 2019 Marvell International Ltd.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+################################################################################
+#
+# Defines Section - statements that will be processed to create a Makefile.
+#
+################################################################################
+[Defines]
+ PLATFORM_NAME = Cn9130DbA
I'll mention in here, but could just as well mention it at the point
where you add Cn9131 - I think it would be better to always mandate
setting the command line option. Otherwise you risk building the wrong
platform based on a typo, and I would prefer for the build to fail in
that case.

+ PLATFORM_GUID = 087305a1-8ddd-4027-89ca-68a3ef78fcc7
+ PLATFORM_VERSION = 0.1
+ DSC_SPECIFICATION = 0x0001000B
+ OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME)-$(ARCH)
+ SUPPORTED_ARCHITECTURES = AARCH64|ARM
+ BUILD_TARGETS = DEBUG|RELEASE|NOOPT
+ SKUID_IDENTIFIER = DEFAULT
+ FLASH_DEFINITION = Silicon/Marvell/Armada7k8k/Armada7k8k.fdf
+ BOARD_DXE_FV_COMPONENTS = Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc
+
+ #
+ # Network definition
+ #
+ DEFINE NETWORK_IP6_ENABLE = FALSE
+ DEFINE NETWORK_TLS_ENABLE = FALSE
+ DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE
+ DEFINE NETWORK_ISCSI_ENABLE = FALSE
+
+!include Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
+!include Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc
+
+[Components.common]
+ Silicon/Marvell/OcteonTx/DeviceTree/T91/$(PLATFORM_NAME).inf
+
+[Components.AARCH64]
+ Silicon/Marvell/OcteonTx/AcpiTables/T91/$(PLATFORM_NAME).inf
+
+[LibraryClasses.common]
+ ArmadaBoardDescLib|Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.inf
+ NonDiscoverableInitLib|Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
diff --git a/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.inf b/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.inf
new file mode 100644
index 0000000..dfbdc84
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.inf
@@ -0,0 +1,29 @@
+## @file
+#
+# Copyright (C) 2019, Marvell International Ltd. and its affiliates<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = Cn9130DbABoardDescLib
+ FILE_GUID = d0f95cbe-c150-47e2-ab8c-b3a3807bcc4b
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmadaBoardDescLib
+
+[Sources]
+ Cn9130DbABoardDescLib.c
+
+[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Silicon/Marvell/Marvell.dec
+
+[LibraryClasses]
+ DebugLib
+ IoLib
diff --git a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.inf b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
new file mode 100644
index 0000000..f7cfb36
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
@@ -0,0 +1,37 @@
+## @file
+#
+# Copyright (c) 2017, Linaro Ltd. All rights reserved.<BR>
+# Copyright (c) 2019, Marvell International Ltd. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = Cn9130DbANonDiscoverableInitLib
+ FILE_GUID = 93886b61-b4f5-4ff3-ba96-6f2f9e7661b9
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = NonDiscoverableInitLib
+
+[Sources]
+ NonDiscoverableInitLib.c
+
+[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ Silicon/Marvell/Marvell.dec
+
+[LibraryClasses]
+ DebugLib
+ IoLib
+ MvGpioLib
+
+[Protocols]
+ gEmbeddedGpioProtocolGuid
+
+[Depex]
+ gMarvellPlatformInitCompleteProtocolGuid
diff --git a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h
new file mode 100644
index 0000000..2533c35
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.h
@@ -0,0 +1,19 @@
+/**
+*
+* Copyright (c) 2019, Marvell International Ltd. All rights reserved.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+#ifndef __NON_DISCOVERABLE_INIT_LIB_H__
+#define __NON_DISCOVERABLE_INIT_LIB_H__
No leading __ for include guards.

/
Leif


+
+#define CN9130_DB_IO_EXPANDER0 0
+#define CN9130_DB_VBUS0_PIN 0
+#define CN9130_DB_VBUS0_LIMIT_PIN 4
+#define CN9130_DB_VBUS1_PIN 1
+#define CN9130_DB_VBUS1_LIMIT_PIN 5
+#define CN9130_DB_SDMMC_VCC_PIN 14
+#define CN9130_DB_SDMMC_VCCQ_PIN 15
+
+#endif
diff --git a/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.c b/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.c
new file mode 100644
index 0000000..2b46d14
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/BoardDescriptionLib/Cn9130DbABoardDescLib.c
@@ -0,0 +1,126 @@
+/**
+*
+* Copyright (C) 2019, Marvell International Ltd. and its affiliates.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <Uefi.h>
+
+#include <Library/ArmadaBoardDescLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/IoLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/MvGpioLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+//
+// GPIO Expander
+//
+STATIC MV_GPIO_EXPANDER mGpioExpander = {
+ PCA9555_ID,
+ 0x21,
+ 0x0,
+};
+
+
+EFI_STATUS
+EFIAPI
+ArmadaBoardGpioExpanderGet (
+ IN OUT MV_GPIO_EXPANDER **GpioExpanders,
+ IN OUT UINTN *GpioExpanderCount
+ )
+{
+ *GpioExpanderCount = 1;
+ *GpioExpanders = &mGpioExpander;
+
+ return EFI_SUCCESS;
+}
+
+//
+// PCIE
+//
+STATIC
+MV_PCIE_CONTROLLER mPcieController[] = {
+ { /* PCIE0 @0xF2640000 */
+ .PcieDbiAddress = 0xF2600000,
+ .ConfigSpaceAddress = 0xD0000000,
+ .HaveResetGpio = FALSE,
+ .PcieResetGpio = { 0 },
+ .PcieBusMin = 0,
+ .PcieBusMax = 0xFE,
+ .PcieIoTranslation = 0xDFF00000,
+ .PcieIoWinBase = 0x0,
+ .PcieIoWinSize = 0x10000,
+ .PcieMmio32Translation = 0,
+ .PcieMmio32WinBase = 0xC0000000,
+ .PcieMmio32WinSize = 0x10000000,
+ .PcieMmio64Translation = 0,
+ .PcieMmio64WinBase = MAX_UINT64,
+ .PcieMmio64WinSize = 0,
+ }
+};
+
+/**
+ Return the number and description of PCIE controllers used on the platform.
+
+ @param[in out] **PcieControllers Array containing PCIE controllers'
+ description.
+ @param[in out] *PcieControllerCount Amount of used PCIE controllers.
+
+ @retval EFI_SUCCESS The data were obtained successfully.
+ @retval other Return error status.
+
+**/
+EFI_STATUS
+EFIAPI
+ArmadaBoardPcieControllerGet (
+ IN OUT MV_PCIE_CONTROLLER CONST **PcieControllers,
+ IN OUT UINTN *PcieControllerCount
+ )
+{
+ *PcieControllers = mPcieController;
+ *PcieControllerCount = ARRAY_SIZE (mPcieController);
+
+ return EFI_SUCCESS;
+}
+
+//
+// Order of devices in SdMmcDescTemplate has to be in par with ArmadaSoCDescLib
+//
+STATIC
+MV_BOARD_SDMMC_DESC mSdMmcDescTemplate[] = {
+ { /* eMMC 0xF06E0000 */
+ 0, /* SOC will be filled by MvBoardDescDxe */
+ 0, /* SdMmcDevCount will be filled by MvBoardDescDxe */
+ FALSE, /* Xenon1v8Enabled */
+ TRUE, /* Xenon8BitBusEnabled */
+ FALSE, /* XenonSlowModeEnabled */
+ 0x40, /* XenonTuningStepDivisor */
+ EmbeddedSlot /* SlotType */
+ },
+ { /* SD/MMC 0xF2780000 */
+ 0, /* SOC will be filled by MvBoardDescDxe */
+ 0, /* SdMmcDevCount will be filled by MvBoardDescDxe */
+ FALSE, /* Xenon1v8Enabled */
+ FALSE, /* Xenon8BitBusEnabled */
+ FALSE, /* XenonSlowModeEnabled */
+ 0x19, /* XenonTuningStepDivisor */
+ EmbeddedSlot /* SlotType */
+ }
+};
+
+EFI_STATUS
+EFIAPI
+ArmadaBoardDescSdMmcGet (
+ OUT UINTN *SdMmcDevCount,
+ OUT MV_BOARD_SDMMC_DESC **SdMmcDesc
+ )
+{
+ *SdMmcDesc = mSdMmcDescTemplate;
+ *SdMmcDevCount = ARRAY_SIZE (mSdMmcDescTemplate);
+
+ return EFI_SUCCESS;
+}
diff --git a/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c
new file mode 100644
index 0000000..598c649
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/NonDiscoverableInitLib/NonDiscoverableInitLib.c
@@ -0,0 +1,144 @@
+/**
+*
+* Copyright (c) 2017, Linaro Ltd. All rights reserved.
+* Copyright (c) 2019, Marvell International Ltd. All rights reserved.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <Uefi.h>
+
+#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/IoLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/MvGpioLib.h>
+#include <Library/NonDiscoverableDeviceRegistrationLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/NonDiscoverableDevice.h>
+
+#include "NonDiscoverableInitLib.h"
+
+STATIC
+EFI_STATUS
+EFIAPI
+ConfigurePins (
+ IN CONST MV_GPIO_PIN *VbusPin,
+ IN UINTN PinCount,
+ IN MV_GPIO_DRIVER_TYPE DriverType
+ )
+{
+ EMBEDDED_GPIO_MODE Mode;
+ EMBEDDED_GPIO_PIN Gpio;
+ EMBEDDED_GPIO *GpioProtocol;
+ EFI_STATUS Status;
+ UINTN Index;
+
+ Status = MvGpioGetProtocol (DriverType, &GpioProtocol);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: Unable to find GPIO protocol\n", __FUNCTION__));
+ return Status;
+ }
+
+ for (Index = 0; Index < PinCount; Index++) {
+ Mode = VbusPin->ActiveHigh ? GPIO_MODE_OUTPUT_1 : GPIO_MODE_OUTPUT_0;
+ Gpio = GPIO (VbusPin->ControllerId, VbusPin->PinNumber);
+ GpioProtocol->Set (GpioProtocol, Gpio, Mode);
+ VbusPin++;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC CONST MV_GPIO_PIN mCp0XhciVbusPins[] = {
+ {
+ MV_GPIO_DRIVER_TYPE_PCA95XX,
+ CN9130_DB_IO_EXPANDER0,
+ CN9130_DB_VBUS0_PIN,
+ TRUE,
+ },
+ {
+ MV_GPIO_DRIVER_TYPE_PCA95XX,
+ CN9130_DB_IO_EXPANDER0,
+ CN9130_DB_VBUS0_LIMIT_PIN,
+ TRUE,
+ },
+ {
+ MV_GPIO_DRIVER_TYPE_PCA95XX,
+ CN9130_DB_IO_EXPANDER0,
+ CN9130_DB_VBUS1_PIN,
+ TRUE,
+ },
+ {
+ MV_GPIO_DRIVER_TYPE_PCA95XX,
+ CN9130_DB_IO_EXPANDER0,
+ CN9130_DB_VBUS1_LIMIT_PIN,
+ TRUE,
+ },
+};
+
+STATIC
+EFI_STATUS
+EFIAPI
+Cp0XhciInit (
+ IN NON_DISCOVERABLE_DEVICE *This
+ )
+{
+ return ConfigurePins (mCp0XhciVbusPins,
+ ARRAY_SIZE (mCp0XhciVbusPins),
+ MV_GPIO_DRIVER_TYPE_PCA95XX);
+}
+
+STATIC CONST MV_GPIO_PIN mCp0SdMmcPins[] = {
+ {
+ MV_GPIO_DRIVER_TYPE_PCA95XX,
+ CN9130_DB_IO_EXPANDER0,
+ CN9130_DB_SDMMC_VCC_PIN,
+ TRUE,
+ },
+ {
+ MV_GPIO_DRIVER_TYPE_PCA95XX,
+ CN9130_DB_IO_EXPANDER0,
+ CN9130_DB_SDMMC_VCCQ_PIN,
+ FALSE,
+ },
+};
+
+STATIC
+EFI_STATUS
+EFIAPI
+Cp0SdMmcInit (
+ IN NON_DISCOVERABLE_DEVICE *This
+ )
+{
+ return ConfigurePins (mCp0SdMmcPins,
+ ARRAY_SIZE (mCp0SdMmcPins),
+ MV_GPIO_DRIVER_TYPE_PCA95XX);
+}
+
+NON_DISCOVERABLE_DEVICE_INIT
+EFIAPI
+NonDiscoverableDeviceInitializerGet (
+ IN NON_DISCOVERABLE_DEVICE_TYPE Type,
+ IN UINTN Index
+ )
+{
+ if (Type == NonDiscoverableDeviceTypeXhci) {
+ switch (Index) {
+ case 0:
+ case 1:
+ return Cp0XhciInit;
+ }
+ }
+
+ if (Type == NonDiscoverableDeviceTypeSdhci) {
+ switch (Index) {
+ case 1:
+ return Cp0SdMmcInit;
+ }
+ }
+
+ return NULL;
+}
diff --git a/Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc b/Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc
new file mode 100644
index 0000000..0c321d1
--- /dev/null
+++ b/Platform/Marvell/Cn913xDb/Cn913xDbA.fdf.inc
@@ -0,0 +1,18 @@
+#
+# Copyright (C) 2019 Marvell International Ltd. and its affiliates
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+# Per-board additional content of the DXE phase firmware volume
+
+ INF Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.inf
+ INF Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
+
+ # DTB
+ INF RuleOverride = DTB Silicon/Marvell/OcteonTx/DeviceTree/T91/$(PLATFORM_NAME).inf
+
+!if $(ARCH) == AARCH64
+ # ACPI support
+ INF RuleOverride = ACPITABLE Silicon/Marvell/OcteonTx/AcpiTables/T91/$(PLATFORM_NAME).inf
+!endif
--
2.7.4


Re: [Patch V4 06/10] EmulatorPkg: Fix XCODE5 lldb issues

Andrew Fish
 

Tested-by: Andrew Fish <afish@...>

I tested the XCODE builds of the emulator and made sure that source level debugging worked probably for builds that generate symbols. 

Thanks,

Andrew Fish

On Aug 16, 2019, at 8:09 AM, Michael D Kinney <michael.d.kinney@...> wrote:

Jordan,

It is not a typo.

Andrew generated the XCODE specific changes, so they have
been tested by him.  I have also reviewed and tested the XCODE
changes and verified that all 6 combinations build and boot
to shell (IA32/X64 for RELEASE/DEBUG/NOOPT). Since they are
all related to making EmulatorPkg work, we decided to fold
them into the same patch set that was already being reviewed.

I also verified build and boot to shell for 6 combinations
on GCC5 (IA32/X64 for RELEASE/DEBUG/NOOPT) and the 12
combinations of VS2015/VS2017, IA323/X64, RELEASE/DEBUG/NOOPT.

I have been working on some CI experiments using Azure Pipelines.
Here is a pointer to the build logs for all the combinations
listed above.

https://dev.azure.com/mikekinney/edk2-ci/_build/results?buildId=312

Mike

-----Original Message-----
From: Justen, Jordan L
Sent: Friday, August 16, 2019 12:41 AM
To: Kinney, Michael D <michael.d.kinney@...>;
devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Andrew Fish
<afish@...>
Subject: Re: [Patch V4 06/10] EmulatorPkg: Fix XCODE5
lldb issues

On 2019-08-15 19:14:33, Michael D Kinney wrote:
Fix scripts to support lldb symbolic debugging when
using XCODE5 tool
chain.

Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ray Ni <ray.ni@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Signed-off-by: Andrew Fish <afish@...>

Is this a Cc/Signed-off-by typo? (See also, patches 7-
10).

This makes me wonder if you are taking advantage of the
git commit -s switch to add your Signed-off-by.

Also, I'm wondering if you are taking advantage of git-
send-email automatically Cc'ing the addresses you
listed in the commit message.
(I thought it Cc'd for the author and Cc tags, but I
wasn't sure about the Signed-off-by tag, and yet I see
Andrew was Cc'd.)

There's a couple long lines below. You could use \ at
the end of the line to split the .sh line. I think the
cd can be a separate command in a shell script. (Not in
make)

I hope someone that uses the XCODE toolchain could
review/check the XCODE patches.

-Jordan

---
EmulatorPkg/Unix/lldbefi.py |  8 +++++---
EmulatorPkg/build.sh        | 17 ++---------------
2 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/EmulatorPkg/Unix/lldbefi.py
b/EmulatorPkg/Unix/lldbefi.py
index 218326b8cb..099192d8b5 100755
--- a/EmulatorPkg/Unix/lldbefi.py
+++ b/EmulatorPkg/Unix/lldbefi.py
@@ -346,6 +346,7 @@ def TypePrintFormating(debugger):
    debugger.HandleCommand("type summary add CHAR8 -
-python-function lldbefi.CHAR8_TypeSummary")
    debugger.HandleCommand('type summary add --regex
"CHAR8
\[[0-9]+\]" --python-function
lldbefi.CHAR8_TypeSummary')

+    debugger.HandleCommand('setting set frame-format
"frame
+ #${frame.index}: ${frame.pc}{
+
${module.file.basename}{:${function.name}()${function.p
c-offset}}}{
+ at ${line.file.fullpath}:${line.number}}\n"')

gEmulatorBreakWorkaroundNeeded = True

@@ -381,15 +382,16 @@ def
LoadEmulatorEfiSymbols(frame, bp_loc , internal_dict):
    Error = lldb.SBError()
    FileNamePtr = frame.FindVariable
("FileName").GetValueAsUnsigned()
    FileNameLen = frame.FindVariable
("FileNameLength").GetValueAsUnsigned()
+
    FileName =
frame.thread.process.ReadCStringFromMemory
(FileNamePtr, FileNameLen, Error)
    if not Error.Success():
        print "!ReadCStringFromMemory() did not find
a %d byte C string at %x" % (FileNameLen, FileNamePtr)
        # make breakpoint command contiue
-        frame.GetThread().GetProcess().Continue()
+        return False

    debugger = frame.thread.process.target.debugger
    if frame.FindVariable
("AddSymbolFlag").GetValueAsUnsigned() == 1:
-        LoadAddress = frame.FindVariable
("LoadAddress").GetValueAsUnsigned()
+        LoadAddress = frame.FindVariable
+ ("LoadAddress").GetValueAsUnsigned() - 0x240

        debugger.HandleCommand ("target modules add
%s" % FileName)
        print "target modules load --slid 0x%x %s" %
(LoadAddress,
FileName) @@ -405,7 +407,7 @@ def
LoadEmulatorEfiSymbols(frame, bp_loc , internal_dict):
                    print "!lldb.target.RemoveModule
(%s) FAILED" %
SBModule

    # make breakpoint command contiue
-    frame.thread.process.Continue()
+    return False

def GuidToCStructStr (guid, Name=False):
  #
diff --git a/EmulatorPkg/build.sh
b/EmulatorPkg/build.sh index
60056e1b6c..35912a7775 100755
--- a/EmulatorPkg/build.sh
+++ b/EmulatorPkg/build.sh
@@ -209,21 +209,8 @@ fi
if [[ "$RUN_EMULATOR" == "yes" ]]; then
  case `uname` in
    Darwin*)
-      #
-      # On Darwin we can't use dlopen, so we have to
load the real PE/COFF images.
-      # This .gdbinit script sets a breakpoint that
loads symbols for the PE/COFFEE
-      # images that get loaded in Host
-      #
-      if [[ "$CLANG_VER" == *-ccc-host-triple* ]]
-      then
-      # only older versions of Xcode support -ccc-
host-tripe, for newer versions
-      # it is -target
-        cp $WORKSPACE/EmulatorPkg/Unix/lldbefi.py
"$BUILD_OUTPUT_DIR/${BUILDTARGET}_$TARGET_TOOLS/$PROCES
SOR"
-        cd $BUILD_ROOT_ARCH; /usr/bin/lldb --source
$WORKSPACE/EmulatorPkg/Unix/lldbinit Host
-        exit $?
-      else
-        cp $WORKSPACE/EmulatorPkg/Unix/.gdbinit
"$BUILD_OUTPUT_DIR/${BUILDTARGET}_$TARGET_TOOLS/$PROCES
SOR"
-      fi
+      cd $BUILD_ROOT_ARCH; /usr/bin/lldb -o "command
script import $WORKSPACE/EmulatorPkg/Unix/lldbefi.py" -
o 'script lldb.debugger.SetAsync(True)' -o "run" ./Host
+      exit $?
      ;;
  esac

--
2.21.0.windows.1






Re: [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

Vitaly Cheptsov
 

Laszlo,

I have already mentioned that the documentation is sufficient as _Static_assert is C standard, so I do not plan to make a V3 for this patch. The patch is merge ready.

As for usage examples I have an opposing opinion to yours and believe it is based on very good reasons. Not using STATIC_ASSERT in the current release will make the feature optionally available and let people test it in their setups. In case they notice it does not work for them they will have 3 months grace period to report it to us and consider making a change. This will also give them 3 months grace period of VERIFY_SIZE_OF macro removal in favour of STATIC_ASSERT. Making the change now will let people do seamless transition to the new feature and will avoid obstacles you are currently trying to create. Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal must both be separate patchsets with potentially separate BZs.

Thanks for understanding,
Vitaly

16 авг. 2019 г., в 19:33, Laszlo Ersek <lersek@redhat.com> написал(а):


On 08/15/19 03:59, Gao, Liming wrote:
Vitaly:
As you know, edk2 201908 stable tag will start soft freeze tomorrow. Dose this patch plan to catch this stable tag?
If yes, please ask the feedback from Tianocore Stewards. I have cc this patch to all Stewards.
If a feature patch (or series) is fully reviewed before the soft feature
freeze (by the respective package maintainers), it can be merged during
the soft feature freeze.

However, I don't think this patch is mature enough for that. As I've
just said up-thread, I'd like to see STATIC_ASSERT being put to use at
once (in the same series, not in the same patch). In addition, the
documentation should be improved (the (constant-expression ,
string-literal) parameter list seems absent, or at least insufficiently
documented).

In turn, I doubt a v3 posting could be reviewed with enough care before
we enter the soft feature freeze. I'd suggest to submit the v3 series as
soon as we start the next development cycle.

Thanks
Laszlo

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Yao, Jiewen
Sent: Thursday, August 15, 2019 9:05 AM
To: devel@edk2.groups.io; vit9696@protonmail.com; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

Good input.
I think we should separate the work to convert all EDKII code to use STATIC_ASSERT.
We can do that work once we add STATIC_ASSERT.

I recommend:

1) Step 1: Add STATIS_ASSERT - this patch and this Bugzilla

2) Step 2: Convert VERIFY_SIZE_OF to STATIS_ASSERT, and remove VERIFY_SIZE_OF – the other patch and the other Bugzilla

3) Step 3: Scan the rest, if there is need. – Another patch and another Bugzilla

Thank you
Yao Jiewen

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptosv via Groups.Io
Sent: Thursday, August 15, 2019 12:23 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro


Michael, Liming, Laszlo,

Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description.

The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below.

In the bottom of this message I included a quote from C17 for the relevant section (6.7.10).

GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about.

As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody.

I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set.

As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used.

Looking forward to hearing your opinions.

Best regards,
Vitaly


6.7.10 Static assertions

Syntax
1 static_assert-declaration:
_Static_assert ( constant-expression , string-literal ) ;

Constraints
2 The constant expression shall compare unequal to 0.

Semantics
3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message.

Forward references: diagnostics (7.2).

7.2 Diagnostics <assert. h>

3 The macro
static_assert
expands to _Static_assert.


14 авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):


Liming,

I think a good candidate to demonstrate this
feature are the checks made in MdePkg/Include/Base.h.
The current implementation forces a divide by 0
in the C pre-processor to break the build.
STATIC_ASSERT() would be a better way to do this.
I would also remove unused externs from the builds.

/**
Verifies the storage size of a given data type.

This macro generates a divide by zero error or a zero size array declaration in
the preprocessor if the size is incorrect. These are declared as "extern" so
the space for these arrays will not be in the modules.

@param TYPE The date type to determine the size of.
@param Size The expected size for the TYPE.

**/
#define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))]

//
// Verify that ProcessorBind.h produced UEFI Data Types that are compliant with
// Section 2.3.1 of the UEFI 2.3 Specification.
//
VERIFY_SIZE_OF (BOOLEAN, 1);
VERIFY_SIZE_OF (INT8, 1);
VERIFY_SIZE_OF (UINT8, 1);
VERIFY_SIZE_OF (INT16, 2);
VERIFY_SIZE_OF (UINT16, 2);
VERIFY_SIZE_OF (INT32, 4);
VERIFY_SIZE_OF (UINT32, 4);
VERIFY_SIZE_OF (INT64, 8);
VERIFY_SIZE_OF (UINT64, 8);
VERIFY_SIZE_OF (CHAR8, 1);
VERIFY_SIZE_OF (CHAR16, 2);

//
// The following three enum types are used to verify that the compiler
// configuration for enum types is compliant with Section 2.3.1 of the
// UEFI 2.3 Specification. These enum types and enum values are not
// intended to be used. A prefix of '__' is used avoid conflicts with
// other types.
//
typedef enum {
__VerifyUint8EnumValue = 0xff
} __VERIFY_UINT8_ENUM_SIZE;

typedef enum {
__VerifyUint16EnumValue = 0xffff
} __VERIFY_UINT16_ENUM_SIZE;

typedef enum {
__VerifyUint32EnumValue = 0xffffffff
} __VERIFY_UINT32_ENUM_SIZE;

VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4);
VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4);
VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4);

A couple examples. Do all the compilers support the message parameter too?

STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements")

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io]
On Behalf Of Liming Gao
Sent: Wednesday, August 14, 2019 6:50 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vit9696@protonmail.com<mailto:vit9696@protonmail.com>
Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add
STATIC_ASSERT macro

Can you add the sample usage of new macro STATIC_ASSERT?

Or, give the link of static_assert or _Static_assert.

If so, the developer knows how to use them in source
code.

Thanks
Liming
-----Original Message-----
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
[mailto:devel@edk2.groups.io] On Behalf Of
vit9696 via Groups.Io
Sent: Tuesday, August 13, 2019 4:17 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add
STATIC_ASSERT macro

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048

Provide a macro for compile time assertions.
Equivalent to C11 static_assert macro from assert.h.

Signed-off-by: Vitaly Cheptsov
<vit9696@protonmail.com<mailto:vit9696@protonmail.com>>
---
MdePkg/Include/Base.h | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/MdePkg/Include/Base.h
b/MdePkg/Include/Base.h index
ce20b5f01dce..f85f7028a262 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST;
#define
OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
#endif

+///
+/// Portable definition for compile time assertions.
+/// Equivalent to C11 static_assert macro from
assert.h.
+/// Takes condtion and error message as its
arguments.
+///
+#ifdef _MSC_EXTENSIONS
+ #define STATIC_ASSERT static_assert #else
+ #define STATIC_ASSERT _Static_assert #endif
+
/**
Macro that returns a pointer to the data structure
that contains a specified field of
that data structure. This is a lightweight method
to hide
information by placing a
--
2.20.1 (Apple Git-117)


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

View/Reply Online (#45503):
https://edk2.groups.io/g/devel/message/45503
Mute This Topic: https://groups.io/mt/32850582/1759384
Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub
[liming.gao@intel.com] -=-=-=-=-=-=