* [PATCH 0/5] Series short description
@ 2021-11-05 9:23 Masami Hiramatsu
2021-11-05 9:23 ` [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy Masami Hiramatsu
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-05 9:23 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm; +Cc: devel, Kazuhiko Sakamoto, Masahisa Kojima
Hello Leif and Ard,
Here are a series of patches to fix some issues on the DeveloperBox.
Our team found those issues when we ran the SystemReady ES ACS tests[1].
[1] https://github.com/ARM-software/arm-systemready/tree/main/ES
The seires has 5 patches, [1/5] is a resend patch which I sent
before[2], others are new fixes. Actually, one another issue
still exists, which will be fixed soon.
[2] https://www.mail-archive.com/devel@edk2.groups.io/msg37170.html
Thank you,
---
Masami Hiramatsu (5):
[RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
[edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number
[edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks
[edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table
[edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
.../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +--
.../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 2 +
Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +-
.../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 +++++++----
.../SynQuacerPlatformFlashAccessLib.c | 2 -
6 files changed, 107 insertions(+), 21 deletions(-)
create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
--
Masami Hiramatsu <masami.hiramatsu@linaro.org>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
2021-11-05 9:23 [PATCH 0/5] Series short description Masami Hiramatsu
@ 2021-11-05 9:23 ` Masami Hiramatsu
2021-11-26 17:47 ` Leif Lindholm
2021-11-05 9:23 ` [PATCH 2/5] [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number Masami Hiramatsu
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-05 9:23 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm; +Cc: devel, Kazuhiko Sakamoto, Masahisa Kojima
If an EFI application frequently repeats SetTime and GetTime,
the I2C bus can be busy and failed to start. To fix this issue,
add waiting loop for the bus busy status. (Usually, it is
enough to read 3 times for checking, but for safety this
sets 10 for timeout.)
This also clean up the code path a bit so that it is easy to
understand what should do on each combinations of BSR.BB and
BCR.MSS.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
---
.../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 ++++++++++++++------
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
index 31f6e3072f..380eba8059 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
@@ -16,6 +16,8 @@
//
#define WAIT_FOR_INTERRUPT_TIMEOUT 50000
+#define WAIT_FOR_BUS_BUSY_TIMEOUT 10
+
/**
Set the frequency for the I2C clock line.
@@ -152,6 +154,7 @@ SynQuacerI2cMasterStart (
IN EFI_I2C_OPERATION *Op
)
{
+ UINTN Timeout = WAIT_FOR_BUS_BUSY_TIMEOUT;
UINT8 Bsr;
UINT8 Bcr;
@@ -167,24 +170,35 @@ SynQuacerI2cMasterStart (
Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
Bcr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BCR);
- if ((Bsr & F_I2C_BSR_BB) && !(Bcr & F_I2C_BCR_MSS)) {
- DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
- return EFI_ALREADY_STARTED;
- }
+ if (!(Bcr & F_I2C_BCR_MSS)) {
- if (Bsr & F_I2C_BSR_BB) { // Bus is busy
- DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
- MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
- } else {
- if (Bcr & F_I2C_BCR_MSS) {
- DEBUG ((DEBUG_WARN,
- "%a: is not in master mode\n", __FUNCTION__));
- return EFI_DEVICE_ERROR;
+ if (Bsr & F_I2C_BSR_BB) { // Bus is busy
+ do {
+ Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
+ } while (Timeout-- && (Bsr & F_I2C_BSR_BB));
+
+ if (Bsr & F_I2C_BSR_BB) {
+ DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
+ return EFI_ALREADY_STARTED;
+ }
}
+
DEBUG ((DEBUG_INFO, "%a: Start Condition\n", __FUNCTION__));
MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR,
Bcr | F_I2C_BCR_MSS | F_I2C_BCR_INTE | F_I2C_BCR_BEIE);
+
+ } else { // F_I2C_BCR_MSS is set
+
+ if (!(Bsr & F_I2C_BSR_BB)) {
+ DEBUG ((DEBUG_WARN,
+ "%a: is not in master mode\n", __FUNCTION__));
+ return EFI_DEVICE_ERROR;
+ }
+
+ DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
+ MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
}
+
return EFI_SUCCESS;
}
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/5] [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number
2021-11-05 9:23 [PATCH 0/5] Series short description Masami Hiramatsu
2021-11-05 9:23 ` [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy Masami Hiramatsu
@ 2021-11-05 9:23 ` Masami Hiramatsu
2021-11-26 17:50 ` Leif Lindholm
2021-11-05 9:23 ` [PATCH 3/5] [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks Masami Hiramatsu
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-05 9:23 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm; +Cc: devel, Kazuhiko Sakamoto, Masahisa Kojima
This fixes Socionext DeveloperBox GenericWatchdog interrupt
number to 93 instead of 94. Since the 93 is the default interrupt
number defined in ArmPkg/ArmPkg.dec, this doesn't redefine
gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
---
.../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 1 +
Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +++---
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
index 96efb2d38e..886777a0fa 100644
--- a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
+++ b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
@@ -50,6 +50,7 @@
gArmTokenSpaceGuid.PcdGenericWatchdogControlBase
gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase
+ gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision
diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc b/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
index c811fc5a0c..b045a49efa 100644
--- a/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
+++ b/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
@@ -74,9 +74,9 @@ EFI_ACPI_6_0_GENERIC_TIMER_DESCRIPTION_TABLES Gtdt = {
// UINT32 GTxCommonFlags
},
EFI_ACPI_6_0_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT (
- FixedPcdGet32 (PcdGenericWatchdogRefreshBase),
- FixedPcdGet32 (PcdGenericWatchdogControlBase),
- 94,
+ FixedPcdGet64 (PcdGenericWatchdogRefreshBase),
+ FixedPcdGet64 (PcdGenericWatchdogControlBase),
+ FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
0),
};
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/5] [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks
2021-11-05 9:23 [PATCH 0/5] Series short description Masami Hiramatsu
2021-11-05 9:23 ` [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy Masami Hiramatsu
2021-11-05 9:23 ` [PATCH 2/5] [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number Masami Hiramatsu
@ 2021-11-05 9:23 ` Masami Hiramatsu
2021-11-26 17:52 ` Leif Lindholm
2021-11-05 9:23 ` [PATCH 4/5] [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table Masami Hiramatsu
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-05 9:23 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm; +Cc: devel, Kazuhiko Sakamoto, Masahisa Kojima
Fix the number of erase blocks by rounding up the result.
The erase blocks must include the last block covered by the
length bytes.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
---
.../SynQuacerPlatformFlashAccessLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
index bded74dc4f..ad4021cf59 100644
--- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
+++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
@@ -283,7 +283,7 @@ PerformFlashWriteWithProgress (
DEBUG ((DEBUG_INFO, "%a: erasing 0x%llx bytes at address %llx (LBA 0x%lx)\n",
__FUNCTION__, Length, FlashAddress, Lba));
- Status = Fvb->EraseBlocks (Fvb, Lba, Length / BlockSize,
+ Status = Fvb->EraseBlocks (Fvb, Lba, (Length + BlockSize - 1) / BlockSize,
EFI_LBA_LIST_TERMINATOR);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "%a: Fvb->EraseBlocks () failed - %r\n",
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/5] [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table
2021-11-05 9:23 [PATCH 0/5] Series short description Masami Hiramatsu
` (2 preceding siblings ...)
2021-11-05 9:23 ` [PATCH 3/5] [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks Masami Hiramatsu
@ 2021-11-05 9:23 ` Masami Hiramatsu
2021-11-26 18:10 ` Leif Lindholm
2021-11-05 9:24 ` [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes Masami Hiramatsu
2021-11-25 11:19 ` [PATCH 0/5] Series short description Masami Hiramatsu
5 siblings, 1 reply; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-05 9:23 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm; +Cc: devel, Kazuhiko Sakamoto, Masahisa Kojima
Add DBG2 table to ACPI tables. The COM1 uart port will be used
for OS debug, and it is 16550 compatible.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
.../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 1
Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
2 files changed, 71 insertions(+)
create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
index 886777a0fa..3023206330 100644
--- a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
+++ b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
@@ -22,6 +22,7 @@
Dsdt.asl
Fadt.aslc
Gtdt.aslc
+ Dbg2.aslc
Iort.aslc
Madt.aslc
Mcfg.aslc
diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc b/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
new file mode 100644
index 0000000000..027b3b658b
--- /dev/null
+++ b/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
@@ -0,0 +1,70 @@
+/** @file
+* Debug Port Table (DBG2)
+*
+* Copyright (c) 2020,2021 Linaro Ltd. All rights reserved.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+#include <IndustryStandard/Acpi.h>
+#include <IndustryStandard/DebugPort2Table.h>
+#include <Library/AcpiLib.h>
+#include <Library/PcdLib.h>
+#include <Platform/MemoryMap.h>
+
+#include "AcpiTables.h"
+
+#pragma pack(1)
+
+#define SYNQUACER_UART1_STR { '\\', '_', 'S', 'B', '.', 'C', 'O', 'M', '1', 0x00 }
+#define SQ_GAS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0, EFI_ACPI_5_0_BYTE, Address }
+
+typedef struct {
+ EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT Dbg2Device;
+ EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE BaseAddressRegister;
+ UINT32 AddressSize;
+ UINT8 NameSpaceString[10];
+} DBG2_DEBUG_DEVICE_INFORMATION;
+
+typedef struct {
+ EFI_ACPI_DEBUG_PORT_2_DESCRIPTION_TABLE Description;
+ DBG2_DEBUG_DEVICE_INFORMATION Dbg2DeviceInfo;
+} DBG2_TABLE;
+
+
+STATIC DBG2_TABLE Dbg2 = {
+ {
+ __ACPI_HEADER (
+ EFI_ACPI_6_3_DEBUG_PORT_2_TABLE_SIGNATURE,
+ DBG2_TABLE,
+ EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION
+ ),
+ OFFSET_OF (DBG2_TABLE, Dbg2DeviceInfo),
+ 1 /* NumberOfDebugPorts */
+ },
+ {
+ {
+ EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
+ sizeof (DBG2_DEBUG_DEVICE_INFORMATION),
+ 1, /* NumberofGenericAddressRegisters */
+ 10, /* NameSpaceStringLength */
+ OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, NameSpaceString),
+ 0, /* OemDataLength */
+ 0, /* OemDataOffset */
+ EFI_ACPI_DBG2_PORT_TYPE_SERIAL,
+ EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_16550_WITH_GAS,
+ {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE},
+ OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, BaseAddressRegister),
+ OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, AddressSize)
+ },
+ SQ_GAS32 (SYNQUACER_UART1_BASE), /* BaseAddressRegister */
+ SYNQUACER_UART1_SIZE, /* AddressSize */
+ SYNQUACER_UART1_STR, /* NameSpaceString */
+ }
+};
+
+#pragma pack()
+
+// Reference the table being generated to prevent the optimizer from removing
+// the data structure from the executable
+VOID* CONST ReferenceAcpiTable = &Dbg2;
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
2021-11-05 9:23 [PATCH 0/5] Series short description Masami Hiramatsu
` (3 preceding siblings ...)
2021-11-05 9:23 ` [PATCH 4/5] [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table Masami Hiramatsu
@ 2021-11-05 9:24 ` Masami Hiramatsu
2021-11-26 18:19 ` Leif Lindholm
2021-11-25 11:19 ` [PATCH 0/5] Series short description Masami Hiramatsu
5 siblings, 1 reply; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-05 9:24 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm; +Cc: devel, Kazuhiko Sakamoto, Masahisa Kojima
Expand NvStorage Variable size and FTW spare/working size
for the DeveloperBox platform.
Since the size of the NvStorage VariableSize is not enough
large, FWTS uefirttime test, which updates the NV
variables in runtime, failes. This expands the size to fix
this issue.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
---
.../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
index 0a364bc457..3baf97ecc0 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
@@ -280,11 +280,11 @@
gFip006DxeTokenSpaceGuid.PcdFip006DxeMemBaseAddress|0x08000000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x08400000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08410000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08420000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00080000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08480000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00080000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08500000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00080000
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId|"SNI "
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId|0x52434155514e5953 # SYNQUACR
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 0/5] Series short description
2021-11-05 9:23 [PATCH 0/5] Series short description Masami Hiramatsu
` (4 preceding siblings ...)
2021-11-05 9:24 ` [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes Masami Hiramatsu
@ 2021-11-25 11:19 ` Masami Hiramatsu
2021-11-25 16:40 ` Leif Lindholm
5 siblings, 1 reply; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-25 11:19 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm; +Cc: devel, Kazuhiko Sakamoto, Masahisa Kojima
Hello Leif and Ard,
Could you give me any feedback on this series?
Thank you,
2021年11月5日(金) 18:23 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>
> Hello Leif and Ard,
>
> Here are a series of patches to fix some issues on the DeveloperBox.
> Our team found those issues when we ran the SystemReady ES ACS tests[1].
>
> [1] https://github.com/ARM-software/arm-systemready/tree/main/ES
>
> The seires has 5 patches, [1/5] is a resend patch which I sent
> before[2], others are new fixes. Actually, one another issue
> still exists, which will be fixed soon.
>
> [2] https://www.mail-archive.com/devel@edk2.groups.io/msg37170.html
>
> Thank you,
>
> ---
>
> Masami Hiramatsu (5):
> [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
> [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number
> [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks
> [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table
> [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
>
>
> .../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +--
> .../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 2 +
> Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
> Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +-
> .../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 +++++++----
> .../SynQuacerPlatformFlashAccessLib.c | 2 -
> 6 files changed, 107 insertions(+), 21 deletions(-)
> create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
>
> --
> Masami Hiramatsu <masami.hiramatsu@linaro.org>
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/5] Series short description
2021-11-25 11:19 ` [PATCH 0/5] Series short description Masami Hiramatsu
@ 2021-11-25 16:40 ` Leif Lindholm
2021-11-26 0:58 ` Masami Hiramatsu
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2021-11-25 16:40 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
Hi Masami,
My feedback was
https://edk2.groups.io/g/devel/message/83641
Best Regards,
Leif
On Thu, Nov 25, 2021 at 20:19:51 +0900, Masami Hiramatsu wrote:
> Hello Leif and Ard,
>
> Could you give me any feedback on this series?
>
> Thank you,
>
> 2021年11月5日(金) 18:23 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
> >
> > Hello Leif and Ard,
> >
> > Here are a series of patches to fix some issues on the DeveloperBox.
> > Our team found those issues when we ran the SystemReady ES ACS tests[1].
> >
> > [1] https://github.com/ARM-software/arm-systemready/tree/main/ES
> >
> > The seires has 5 patches, [1/5] is a resend patch which I sent
> > before[2], others are new fixes. Actually, one another issue
> > still exists, which will be fixed soon.
> >
> > [2] https://www.mail-archive.com/devel@edk2.groups.io/msg37170.html
> >
> > Thank you,
> >
> > ---
> >
> > Masami Hiramatsu (5):
> > [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
> > [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number
> > [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks
> > [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table
> > [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
> >
> >
> > .../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +--
> > .../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 2 +
> > Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
> > Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +-
> > .../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 +++++++----
> > .../SynQuacerPlatformFlashAccessLib.c | 2 -
> > 6 files changed, 107 insertions(+), 21 deletions(-)
> > create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
> >
> > --
> > Masami Hiramatsu <masami.hiramatsu@linaro.org>
>
>
>
> --
> Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/5] Series short description
2021-11-25 16:40 ` Leif Lindholm
@ 2021-11-26 0:58 ` Masami Hiramatsu
0 siblings, 0 replies; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-26 0:58 UTC (permalink / raw)
To: Leif Lindholm; +Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
Hi Leif,
Oh, I missed that. I need to fix my mailing list filter...
Anyway, thanks for the feedback. Let me update if I need.
Regards,
2021年11月26日(金) 1:40 Leif Lindholm <leif@nuviainc.com>:
>
> Hi Masami,
>
> My feedback was
> https://edk2.groups.io/g/devel/message/83641
>
> Best Regards,
>
> Leif
>
> On Thu, Nov 25, 2021 at 20:19:51 +0900, Masami Hiramatsu wrote:
> > Hello Leif and Ard,
> >
> > Could you give me any feedback on this series?
> >
> > Thank you,
> >
> > 2021年11月5日(金) 18:23 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
> > >
> > > Hello Leif and Ard,
> > >
> > > Here are a series of patches to fix some issues on the DeveloperBox.
> > > Our team found those issues when we ran the SystemReady ES ACS tests[1].
> > >
> > > [1] https://github.com/ARM-software/arm-systemready/tree/main/ES
> > >
> > > The seires has 5 patches, [1/5] is a resend patch which I sent
> > > before[2], others are new fixes. Actually, one another issue
> > > still exists, which will be fixed soon.
> > >
> > > [2] https://www.mail-archive.com/devel@edk2.groups.io/msg37170.html
> > >
> > > Thank you,
> > >
> > > ---
> > >
> > > Masami Hiramatsu (5):
> > > [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
> > > [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number
> > > [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks
> > > [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table
> > > [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
> > >
> > >
> > > .../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +--
> > > .../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 2 +
> > > Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
> > > Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +-
> > > .../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 +++++++----
> > > .../SynQuacerPlatformFlashAccessLib.c | 2 -
> > > 6 files changed, 107 insertions(+), 21 deletions(-)
> > > create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
> > >
> > > --
> > > Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >
> >
> >
> > --
> > Masami Hiramatsu
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
2021-11-05 9:23 ` [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy Masami Hiramatsu
@ 2021-11-26 17:47 ` Leif Lindholm
2021-11-27 2:45 ` Masami Hiramatsu
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2021-11-26 17:47 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
On Fri, Nov 05, 2021 at 18:23:28 +0900, Masami Hiramatsu wrote:
> If an EFI application frequently repeats SetTime and GetTime,
> the I2C bus can be busy and failed to start. To fix this issue,
> add waiting loop for the bus busy status. (Usually, it is
> enough to read 3 times for checking, but for safety this
> sets 10 for timeout.)
>
> This also clean up the code path a bit so that it is easy to
> understand what should do on each combinations of BSR.BB and
> BCR.MSS.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> ---
> .../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 ++++++++++++++------
> 1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> index 31f6e3072f..380eba8059 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> +++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> @@ -16,6 +16,8 @@
> //
> #define WAIT_FOR_INTERRUPT_TIMEOUT 50000
>
> +#define WAIT_FOR_BUS_BUSY_TIMEOUT 10
> +
I think it would be more clear English to say that we are waiting
_for_ the bus to be *ready* - meaning that we are waiting _while_ the
bus is *busy*.
So I suggest
WAIT_FOR_BUS_BUSY_TIMEOUT ->
WAIT_FOR_BUS_READY_TIMEOUT
> /**
> Set the frequency for the I2C clock line.
>
> @@ -152,6 +154,7 @@ SynQuacerI2cMasterStart (
> IN EFI_I2C_OPERATION *Op
> )
> {
> + UINTN Timeout = WAIT_FOR_BUS_BUSY_TIMEOUT;
This indentation does not match the subsequent lines.
/
Leif
> UINT8 Bsr;
> UINT8 Bcr;
>
> @@ -167,24 +170,35 @@ SynQuacerI2cMasterStart (
> Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
> Bcr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BCR);
>
> - if ((Bsr & F_I2C_BSR_BB) && !(Bcr & F_I2C_BCR_MSS)) {
> - DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
> - return EFI_ALREADY_STARTED;
> - }
> + if (!(Bcr & F_I2C_BCR_MSS)) {
>
> - if (Bsr & F_I2C_BSR_BB) { // Bus is busy
> - DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
> - MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
> - } else {
> - if (Bcr & F_I2C_BCR_MSS) {
> - DEBUG ((DEBUG_WARN,
> - "%a: is not in master mode\n", __FUNCTION__));
> - return EFI_DEVICE_ERROR;
> + if (Bsr & F_I2C_BSR_BB) { // Bus is busy
> + do {
> + Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
> + } while (Timeout-- && (Bsr & F_I2C_BSR_BB));
> +
> + if (Bsr & F_I2C_BSR_BB) {
> + DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
> + return EFI_ALREADY_STARTED;
> + }
> }
> +
> DEBUG ((DEBUG_INFO, "%a: Start Condition\n", __FUNCTION__));
> MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR,
> Bcr | F_I2C_BCR_MSS | F_I2C_BCR_INTE | F_I2C_BCR_BEIE);
> +
> + } else { // F_I2C_BCR_MSS is set
> +
> + if (!(Bsr & F_I2C_BSR_BB)) {
> + DEBUG ((DEBUG_WARN,
> + "%a: is not in master mode\n", __FUNCTION__));
> + return EFI_DEVICE_ERROR;
> + }
> +
> + DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
> + MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
> }
> +
> return EFI_SUCCESS;
> }
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number
2021-11-05 9:23 ` [PATCH 2/5] [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number Masami Hiramatsu
@ 2021-11-26 17:50 ` Leif Lindholm
2021-11-27 4:43 ` Masami Hiramatsu
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2021-11-26 17:50 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
On Fri, Nov 05, 2021 at 18:23:36 +0900, Masami Hiramatsu wrote:
> This fixes Socionext DeveloperBox GenericWatchdog interrupt
> number to 93 instead of 94. Since the 93 is the default interrupt
> number defined in ArmPkg/ArmPkg.dec, this doesn't redefine
> gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum.
>
That is one thing this patch does.
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> ---
> .../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 1 +
> Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +++---
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> index 96efb2d38e..886777a0fa 100644
> --- a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> +++ b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> @@ -50,6 +50,7 @@
>
> gArmTokenSpaceGuid.PcdGenericWatchdogControlBase
> gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase
> + gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision
> diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc b/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
> index c811fc5a0c..b045a49efa 100644
> --- a/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
> +++ b/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
> @@ -74,9 +74,9 @@ EFI_ACPI_6_0_GENERIC_TIMER_DESCRIPTION_TABLES Gtdt = {
> // UINT32 GTxCommonFlags
> },
> EFI_ACPI_6_0_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT (
> - FixedPcdGet32 (PcdGenericWatchdogRefreshBase),
> - FixedPcdGet32 (PcdGenericWatchdogControlBase),
> - 94,
> + FixedPcdGet64 (PcdGenericWatchdogRefreshBase),
> + FixedPcdGet64 (PcdGenericWatchdogControlBase),
But it also changes these two FixedPcdGet32 calls to FixedPcdGet64.
That should be a separate patch.
/
Leif
> + FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> 0),
> };
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks
2021-11-05 9:23 ` [PATCH 3/5] [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks Masami Hiramatsu
@ 2021-11-26 17:52 ` Leif Lindholm
0 siblings, 0 replies; 21+ messages in thread
From: Leif Lindholm @ 2021-11-26 17:52 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
On Fri, Nov 05, 2021 at 18:23:45 +0900, Masami Hiramatsu wrote:
> Fix the number of erase blocks by rounding up the result.
> The erase blocks must include the last block covered by the
> length bytes.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> ---
> .../SynQuacerPlatformFlashAccessLib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
> index bded74dc4f..ad4021cf59 100644
> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
> @@ -283,7 +283,7 @@ PerformFlashWriteWithProgress (
> DEBUG ((DEBUG_INFO, "%a: erasing 0x%llx bytes at address %llx (LBA 0x%lx)\n",
> __FUNCTION__, Length, FlashAddress, Lba));
>
> - Status = Fvb->EraseBlocks (Fvb, Lba, Length / BlockSize,
> + Status = Fvb->EraseBlocks (Fvb, Lba, (Length + BlockSize - 1) / BlockSize,
> EFI_LBA_LIST_TERMINATOR);
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_ERROR, "%a: Fvb->EraseBlocks () failed - %r\n",
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table
2021-11-05 9:23 ` [PATCH 4/5] [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table Masami Hiramatsu
@ 2021-11-26 18:10 ` Leif Lindholm
2021-11-27 7:52 ` Masami Hiramatsu
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2021-11-26 18:10 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
On Fri, Nov 05, 2021 at 18:23:53 +0900, Masami Hiramatsu wrote:
> Add DBG2 table to ACPI tables. The COM1 uart port will be used
> for OS debug, and it is 16550 compatible.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
> .../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 1
> Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
> 2 files changed, 71 insertions(+)
> create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
>
> diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> index 886777a0fa..3023206330 100644
> --- a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> +++ b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> @@ -22,6 +22,7 @@
> Dsdt.asl
> Fadt.aslc
> Gtdt.aslc
> + Dbg2.aslc
Please move this before Dsdt.asl, to keep the list alphabetically sorted.
> Iort.aslc
> Madt.aslc
> Mcfg.aslc
> diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc b/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
> new file mode 100644
> index 0000000000..027b3b658b
> --- /dev/null
> +++ b/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
> @@ -0,0 +1,70 @@
> +/** @file
> +* Debug Port Table (DBG2)
> +*
> +* Copyright (c) 2020,2021 Linaro Ltd. All rights reserved.
> +*
> +* SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +#include <IndustryStandard/Acpi.h>
> +#include <IndustryStandard/DebugPort2Table.h>
> +#include <Library/AcpiLib.h>
> +#include <Library/PcdLib.h>
> +#include <Platform/MemoryMap.h>
> +
> +#include "AcpiTables.h"
> +
> +#pragma pack(1)
> +
> +#define SYNQUACER_UART1_STR { '\\', '_', 'S', 'B', '.', 'C', 'O', 'M', '1', 0x00 }
> +#define SQ_GAS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0, EFI_ACPI_5_0_BYTE, Address }
Use EFI_ACPI_6_3_ consistently?
/
Leif
> +
> +typedef struct {
> + EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT Dbg2Device;
> + EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE BaseAddressRegister;
> + UINT32 AddressSize;
> + UINT8 NameSpaceString[10];
> +} DBG2_DEBUG_DEVICE_INFORMATION;
> +
> +typedef struct {
> + EFI_ACPI_DEBUG_PORT_2_DESCRIPTION_TABLE Description;
> + DBG2_DEBUG_DEVICE_INFORMATION Dbg2DeviceInfo;
> +} DBG2_TABLE;
> +
> +
> +STATIC DBG2_TABLE Dbg2 = {
> + {
> + __ACPI_HEADER (
> + EFI_ACPI_6_3_DEBUG_PORT_2_TABLE_SIGNATURE,
> + DBG2_TABLE,
> + EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION
> + ),
> + OFFSET_OF (DBG2_TABLE, Dbg2DeviceInfo),
> + 1 /* NumberOfDebugPorts */
> + },
> + {
> + {
> + EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
> + sizeof (DBG2_DEBUG_DEVICE_INFORMATION),
> + 1, /* NumberofGenericAddressRegisters */
> + 10, /* NameSpaceStringLength */
> + OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, NameSpaceString),
> + 0, /* OemDataLength */
> + 0, /* OemDataOffset */
> + EFI_ACPI_DBG2_PORT_TYPE_SERIAL,
> + EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_16550_WITH_GAS,
> + {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE},
> + OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, BaseAddressRegister),
> + OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, AddressSize)
> + },
> + SQ_GAS32 (SYNQUACER_UART1_BASE), /* BaseAddressRegister */
> + SYNQUACER_UART1_SIZE, /* AddressSize */
> + SYNQUACER_UART1_STR, /* NameSpaceString */
> + }
> +};
> +
> +#pragma pack()
> +
> +// Reference the table being generated to prevent the optimizer from removing
> +// the data structure from the executable
> +VOID* CONST ReferenceAcpiTable = &Dbg2;
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
2021-11-05 9:24 ` [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes Masami Hiramatsu
@ 2021-11-26 18:19 ` Leif Lindholm
2021-11-27 7:48 ` Masami Hiramatsu
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2021-11-26 18:19 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
On Fri, Nov 05, 2021 at 18:24:01 +0900, Masami Hiramatsu wrote:
> Expand NvStorage Variable size and FTW spare/working size
> for the DeveloperBox platform.
>
> Since the size of the NvStorage VariableSize is not enough
> large, FWTS uefirttime test, which updates the NV
> variables in runtime, failes. This expands the size to fix
> this issue.
Does this change erase all existing variables?
If so, I think it is worth introducing this as a non-default build
option, in order to not wreck existing installations on a firmware
update.
I think it would also be worth considering whether to update
PcdLowestSupportedFirmwareVersion. PcdFirmwareRevision should
definitely be updated.
/
Leif
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> ---
> .../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> index 0a364bc457..3baf97ecc0 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> @@ -280,11 +280,11 @@
> gFip006DxeTokenSpaceGuid.PcdFip006DxeMemBaseAddress|0x08000000
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x08400000
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08410000
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08420000
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00080000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08480000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00080000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08500000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00080000
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId|"SNI "
> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId|0x52434155514e5953 # SYNQUACR
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
2021-11-26 17:47 ` Leif Lindholm
@ 2021-11-27 2:45 ` Masami Hiramatsu
0 siblings, 0 replies; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-27 2:45 UTC (permalink / raw)
To: Leif Lindholm; +Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
Hi Leif,
2021年11月27日(土) 2:47 Leif Lindholm <leif@nuviainc.com>:
>
> On Fri, Nov 05, 2021 at 18:23:28 +0900, Masami Hiramatsu wrote:
> > If an EFI application frequently repeats SetTime and GetTime,
> > the I2C bus can be busy and failed to start. To fix this issue,
> > add waiting loop for the bus busy status. (Usually, it is
> > enough to read 3 times for checking, but for safety this
> > sets 10 for timeout.)
> >
> > This also clean up the code path a bit so that it is easy to
> > understand what should do on each combinations of BSR.BB and
> > BCR.MSS.
> >
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > ---
> > .../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 ++++++++++++++------
> > 1 file changed, 26 insertions(+), 12 deletions(-)
> >
> > diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> > index 31f6e3072f..380eba8059 100644
> > --- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> > +++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> > @@ -16,6 +16,8 @@
> > //
> > #define WAIT_FOR_INTERRUPT_TIMEOUT 50000
> >
> > +#define WAIT_FOR_BUS_BUSY_TIMEOUT 10
> > +
>
> I think it would be more clear English to say that we are waiting
> _for_ the bus to be *ready* - meaning that we are waiting _while_ the
> bus is *busy*.
>
> So I suggest
> WAIT_FOR_BUS_BUSY_TIMEOUT ->
> WAIT_FOR_BUS_READY_TIMEOUT
Oops, indeed. It is waiting for the bus "ready", not "busy" ...
>
> > /**
> > Set the frequency for the I2C clock line.
> >
> > @@ -152,6 +154,7 @@ SynQuacerI2cMasterStart (
> > IN EFI_I2C_OPERATION *Op
> > )
> > {
> > + UINTN Timeout = WAIT_FOR_BUS_BUSY_TIMEOUT;
>
> This indentation does not match the subsequent lines.
OK, I'll fix that.
Thank you!
>
> /
> Leif
>
> > UINT8 Bsr;
> > UINT8 Bcr;
> >
> > @@ -167,24 +170,35 @@ SynQuacerI2cMasterStart (
> > Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
> > Bcr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BCR);
> >
> > - if ((Bsr & F_I2C_BSR_BB) && !(Bcr & F_I2C_BCR_MSS)) {
> > - DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
> > - return EFI_ALREADY_STARTED;
> > - }
> > + if (!(Bcr & F_I2C_BCR_MSS)) {
> >
> > - if (Bsr & F_I2C_BSR_BB) { // Bus is busy
> > - DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
> > - MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
> > - } else {
> > - if (Bcr & F_I2C_BCR_MSS) {
> > - DEBUG ((DEBUG_WARN,
> > - "%a: is not in master mode\n", __FUNCTION__));
> > - return EFI_DEVICE_ERROR;
> > + if (Bsr & F_I2C_BSR_BB) { // Bus is busy
> > + do {
> > + Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
> > + } while (Timeout-- && (Bsr & F_I2C_BSR_BB));
> > +
> > + if (Bsr & F_I2C_BSR_BB) {
> > + DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
> > + return EFI_ALREADY_STARTED;
> > + }
> > }
> > +
> > DEBUG ((DEBUG_INFO, "%a: Start Condition\n", __FUNCTION__));
> > MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR,
> > Bcr | F_I2C_BCR_MSS | F_I2C_BCR_INTE | F_I2C_BCR_BEIE);
> > +
> > + } else { // F_I2C_BCR_MSS is set
> > +
> > + if (!(Bsr & F_I2C_BSR_BB)) {
> > + DEBUG ((DEBUG_WARN,
> > + "%a: is not in master mode\n", __FUNCTION__));
> > + return EFI_DEVICE_ERROR;
> > + }
> > +
> > + DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
> > + MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
> > }
> > +
> > return EFI_SUCCESS;
> > }
> >
> >
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number
2021-11-26 17:50 ` Leif Lindholm
@ 2021-11-27 4:43 ` Masami Hiramatsu
0 siblings, 0 replies; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-27 4:43 UTC (permalink / raw)
To: Leif Lindholm; +Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
Hi Leif,
2021年11月27日(土) 2:50 Leif Lindholm <leif@nuviainc.com>:
>
> On Fri, Nov 05, 2021 at 18:23:36 +0900, Masami Hiramatsu wrote:
> > This fixes Socionext DeveloperBox GenericWatchdog interrupt
> > number to 93 instead of 94. Since the 93 is the default interrupt
> > number defined in ArmPkg/ArmPkg.dec, this doesn't redefine
> > gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum.
> >
>
> That is one thing this patch does.
>
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > ---
> > .../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 1 +
> > Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +++---
> > 2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> > index 96efb2d38e..886777a0fa 100644
> > --- a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> > +++ b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> > @@ -50,6 +50,7 @@
> >
> > gArmTokenSpaceGuid.PcdGenericWatchdogControlBase
> > gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase
> > + gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
> > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision
> > diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc b/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
> > index c811fc5a0c..b045a49efa 100644
> > --- a/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
> > +++ b/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
> > @@ -74,9 +74,9 @@ EFI_ACPI_6_0_GENERIC_TIMER_DESCRIPTION_TABLES Gtdt = {
> > // UINT32 GTxCommonFlags
> > },
> > EFI_ACPI_6_0_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT (
> > - FixedPcdGet32 (PcdGenericWatchdogRefreshBase),
> > - FixedPcdGet32 (PcdGenericWatchdogControlBase),
> > - 94,
> > + FixedPcdGet64 (PcdGenericWatchdogRefreshBase),
> > + FixedPcdGet64 (PcdGenericWatchdogControlBase),
>
> But it also changes these two FixedPcdGet32 calls to FixedPcdGet64.
> That should be a separate patch.
OK, I'll make it a separate patch.
Thank you,
>
> /
> Leif
>
> > + FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> > 0),
> > };
> >
> >
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
2021-11-26 18:19 ` Leif Lindholm
@ 2021-11-27 7:48 ` Masami Hiramatsu
2021-11-29 13:42 ` Leif Lindholm
0 siblings, 1 reply; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-27 7:48 UTC (permalink / raw)
To: Leif Lindholm; +Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
Hi Leif,
2021年11月27日(土) 3:19 Leif Lindholm <leif@nuviainc.com>:
>
> On Fri, Nov 05, 2021 at 18:24:01 +0900, Masami Hiramatsu wrote:
> > Expand NvStorage Variable size and FTW spare/working size
> > for the DeveloperBox platform.
> >
> > Since the size of the NvStorage VariableSize is not enough
> > large, FWTS uefirttime test, which updates the NV
> > variables in runtime, failes. This expands the size to fix
> > this issue.
>
> Does this change erase all existing variables?
Ah, indeed. It may need to erase all variables.
>
> If so, I think it is worth introducing this as a non-default build
> option, in order to not wreck existing installations on a firmware
> update.
>
> I think it would also be worth considering whether to update
> PcdLowestSupportedFirmwareVersion. PcdFirmwareRevision
> should definitely be updated.
I'm not sure about this point.
You meant we should have 2 different revisions like a branch?
- Branch A(current version): keep the variable area size the same.
- Branch B(new version): expand the variable area.
And a build option will change the branch by updating the
PcdFirmwareRevision?
Also PcdLowestSupportedFirmwareVersion you meant is
in the capsule file?
Thank you,
>
> /
> Leif
>
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > ---
> > .../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > index 0a364bc457..3baf97ecc0 100644
> > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > @@ -280,11 +280,11 @@
> > gFip006DxeTokenSpaceGuid.PcdFip006DxeMemBaseAddress|0x08000000
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x08400000
> > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
> > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08410000
> > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000
> > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08420000
> > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00080000
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08480000
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00080000
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08500000
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00080000
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId|"SNI "
> > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId|0x52434155514e5953 # SYNQUACR
> >
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table
2021-11-26 18:10 ` Leif Lindholm
@ 2021-11-27 7:52 ` Masami Hiramatsu
0 siblings, 0 replies; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-27 7:52 UTC (permalink / raw)
To: Leif Lindholm; +Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
Hi Leif,
2021年11月27日(土) 3:10 Leif Lindholm <leif@nuviainc.com>:
>
> On Fri, Nov 05, 2021 at 18:23:53 +0900, Masami Hiramatsu wrote:
> > Add DBG2 table to ACPI tables. The COM1 uart port will be used
> > for OS debug, and it is 16550 compatible.
> >
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > ---
> > .../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 1
> > Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
> > 2 files changed, 71 insertions(+)
> > create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
> >
> > diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> > index 886777a0fa..3023206330 100644
> > --- a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> > +++ b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
> > @@ -22,6 +22,7 @@
> > Dsdt.asl
> > Fadt.aslc
> > Gtdt.aslc
> > + Dbg2.aslc
>
> Please move this before Dsdt.asl, to keep the list alphabetically sorted.
OK.
>
> > Iort.aslc
> > Madt.aslc
> > Mcfg.aslc
> > diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc b/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
> > new file mode 100644
> > index 0000000000..027b3b658b
> > --- /dev/null
> > +++ b/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
> > @@ -0,0 +1,70 @@
> > +/** @file
> > +* Debug Port Table (DBG2)
> > +*
> > +* Copyright (c) 2020,2021 Linaro Ltd. All rights reserved.
> > +*
> > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > +*
> > +**/
> > +#include <IndustryStandard/Acpi.h>
> > +#include <IndustryStandard/DebugPort2Table.h>
> > +#include <Library/AcpiLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Platform/MemoryMap.h>
> > +
> > +#include "AcpiTables.h"
> > +
> > +#pragma pack(1)
> > +
> > +#define SYNQUACER_UART1_STR { '\\', '_', 'S', 'B', '.', 'C', 'O', 'M', '1', 0x00 }
> > +#define SQ_GAS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0, EFI_ACPI_5_0_BYTE, Address }
>
> Use EFI_ACPI_6_3_ consistently?
OK, got it.
Thank you,
>
> /
> Leif
>
> > +
> > +typedef struct {
> > + EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT Dbg2Device;
> > + EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE BaseAddressRegister;
> > + UINT32 AddressSize;
> > + UINT8 NameSpaceString[10];
> > +} DBG2_DEBUG_DEVICE_INFORMATION;
> > +
> > +typedef struct {
> > + EFI_ACPI_DEBUG_PORT_2_DESCRIPTION_TABLE Description;
> > + DBG2_DEBUG_DEVICE_INFORMATION Dbg2DeviceInfo;
> > +} DBG2_TABLE;
> > +
> > +
> > +STATIC DBG2_TABLE Dbg2 = {
> > + {
> > + __ACPI_HEADER (
> > + EFI_ACPI_6_3_DEBUG_PORT_2_TABLE_SIGNATURE,
> > + DBG2_TABLE,
> > + EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION
> > + ),
> > + OFFSET_OF (DBG2_TABLE, Dbg2DeviceInfo),
> > + 1 /* NumberOfDebugPorts */
> > + },
> > + {
> > + {
> > + EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
> > + sizeof (DBG2_DEBUG_DEVICE_INFORMATION),
> > + 1, /* NumberofGenericAddressRegisters */
> > + 10, /* NameSpaceStringLength */
> > + OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, NameSpaceString),
> > + 0, /* OemDataLength */
> > + 0, /* OemDataOffset */
> > + EFI_ACPI_DBG2_PORT_TYPE_SERIAL,
> > + EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_16550_WITH_GAS,
> > + {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE},
> > + OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, BaseAddressRegister),
> > + OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, AddressSize)
> > + },
> > + SQ_GAS32 (SYNQUACER_UART1_BASE), /* BaseAddressRegister */
> > + SYNQUACER_UART1_SIZE, /* AddressSize */
> > + SYNQUACER_UART1_STR, /* NameSpaceString */
> > + }
> > +};
> > +
> > +#pragma pack()
> > +
> > +// Reference the table being generated to prevent the optimizer from removing
> > +// the data structure from the executable
> > +VOID* CONST ReferenceAcpiTable = &Dbg2;
> >
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
2021-11-27 7:48 ` Masami Hiramatsu
@ 2021-11-29 13:42 ` Leif Lindholm
2021-11-29 22:33 ` Masami Hiramatsu
[not found] ` <16BC25260C31EA7F.23256@groups.io>
0 siblings, 2 replies; 21+ messages in thread
From: Leif Lindholm @ 2021-11-29 13:42 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
On Sat, Nov 27, 2021 at 16:48:45 +0900, Masami Hiramatsu wrote:
> > On Fri, Nov 05, 2021 at 18:24:01 +0900, Masami Hiramatsu wrote:
> > > Expand NvStorage Variable size and FTW spare/working size
> > > for the DeveloperBox platform.
> > >
> > > Since the size of the NvStorage VariableSize is not enough
> > > large, FWTS uefirttime test, which updates the NV
> > > variables in runtime, failes. This expands the size to fix
> > > this issue.
> >
> > Does this change erase all existing variables?
>
> Ah, indeed. It may need to erase all variables.
That is quite likely to lead to upset users.
> > If so, I think it is worth introducing this as a non-default build
> > option, in order to not wreck existing installations on a firmware
> > update.
> >
> > I think it would also be worth considering whether to update
> > PcdLowestSupportedFirmwareVersion. PcdFirmwareRevision
> > should definitely be updated.
>
> I'm not sure about this point.
> You meant we should have 2 different revisions like a branch?
> - Branch A(current version): keep the variable area size the same.
> - Branch B(new version): expand the variable area.
> And a build option will change the branch by updating the
> PcdFirmwareRevision?
Not a branch - just that you need to explicitly build for the size of
flash area you want to use, and if you provide pre-built downloadable
ones - provide two variants.
This becomes a bit of a maintenance nightmare over time.
A better solution would be for the firmware to (somehow) resize the
parameter area - retaining existing values - if it encounters the
smaller version. I don't think we have an example of that.
PcdLowestSupportedFirmwareVersion still needs to be set, to the same
value as the new PcdFirmwareRevision, to prevent downgrading to a
version that does not support the larger size.
> Also PcdLowestSupportedFirmwareVersion you meant is
> in the capsule file?
I meant to change the Pcd value. That implements the change in
SystemFirmwareDescriptorTable.aslc.
Regards,
Leif
> Thank you,
>
>
> >
> > /
> > Leif
> >
> > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > > ---
> > > .../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +++++-----
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > index 0a364bc457..3baf97ecc0 100644
> > > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > @@ -280,11 +280,11 @@
> > > gFip006DxeTokenSpaceGuid.PcdFip006DxeMemBaseAddress|0x08000000
> > >
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x08400000
> > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
> > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08410000
> > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000
> > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08420000
> > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
> > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00080000
> > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08480000
> > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00080000
> > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08500000
> > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00080000
> > >
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId|"SNI "
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId|0x52434155514e5953 # SYNQUACR
> > >
>
>
>
> --
> Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
2021-11-29 13:42 ` Leif Lindholm
@ 2021-11-29 22:33 ` Masami Hiramatsu
[not found] ` <16BC25260C31EA7F.23256@groups.io>
1 sibling, 0 replies; 21+ messages in thread
From: Masami Hiramatsu @ 2021-11-29 22:33 UTC (permalink / raw)
To: Leif Lindholm; +Cc: Ard Biesheuvel, devel, Kazuhiko Sakamoto, Masahisa Kojima
Hi Leif,
2021年11月29日(月) 22:43 Leif Lindholm <leif@nuviainc.com>:
>
> On Sat, Nov 27, 2021 at 16:48:45 +0900, Masami Hiramatsu wrote:
> > > On Fri, Nov 05, 2021 at 18:24:01 +0900, Masami Hiramatsu wrote:
> > > > Expand NvStorage Variable size and FTW spare/working size
> > > > for the DeveloperBox platform.
> > > >
> > > > Since the size of the NvStorage VariableSize is not enough
> > > > large, FWTS uefirttime test, which updates the NV
> > > > variables in runtime, failes. This expands the size to fix
> > > > this issue.
> > >
> > > Does this change erase all existing variables?
> >
> > Ah, indeed. It may need to erase all variables.
>
> That is quite likely to lead to upset users.
OK.
> > > If so, I think it is worth introducing this as a non-default build
> > > option, in order to not wreck existing installations on a firmware
> > > update.
> > >
> > > I think it would also be worth considering whether to update
> > > PcdLowestSupportedFirmwareVersion. PcdFirmwareRevision
> > > should definitely be updated.
> >
> > I'm not sure about this point.
> > You meant we should have 2 different revisions like a branch?
> > - Branch A(current version): keep the variable area size the same.
> > - Branch B(new version): expand the variable area.
> > And a build option will change the branch by updating the
> > PcdFirmwareRevision?
>
> Not a branch - just that you need to explicitly build for the size of
> flash area you want to use, and if you provide pre-built downloadable
> ones - provide two variants.
I got it.
> This becomes a bit of a maintenance nightmare over time.
Actually, I'm considering a kind of "leap" firmware release, which
involves all firmware update by manual (not automatic), because the
SCP-firmware is too old anymore and the new SCP firmware (OSS version)
requires to update TF-A, which is not compatible with old ones.
Obviously, this must be done by manual.
So, afterwards, we will not release old version anymore. Anyway, the
old firmware snapshot image is not updated in one year (since source
repository has not been updated). The firmware on LVFS is released in
2019.
(BTW, can I change the UUID which fwupd detects too?)
I will provide a build option for the users who update EDK2 by themselves.
> A better solution would be for the firmware to (somehow) resize the
> parameter area - retaining existing values - if it encounters the
> smaller version. I don't think we have an example of that.
Hmm, I rather like to erase it while the "leap" update, since the backward
compatibility is not guaranteed. And after the update, user will be able
to choose the U-Boot on the DeveloperBox.
> PcdLowestSupportedFirmwareVersion still needs to be set, to the same
> value as the new PcdFirmwareRevision, to prevent downgrading to a
> version that does not support the larger size.
OK, so this is for protecting rollback. But this means, do I need to make
it optional (switched by build option) too?
>
> > Also PcdLowestSupportedFirmwareVersion you meant is
> > in the capsule file?
>
> I meant to change the Pcd value. That implements the change in
> SystemFirmwareDescriptorTable.aslc.
OK.
Thank you,
>
> Regards,
>
> Leif
>
> > Thank you,
> >
> >
> > >
> > > /
> > > Leif
> > >
> > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > > > ---
> > > > .../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +++++-----
> > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > > index 0a364bc457..3baf97ecc0 100644
> > > > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > > @@ -280,11 +280,11 @@
> > > > gFip006DxeTokenSpaceGuid.PcdFip006DxeMemBaseAddress|0x08000000
> > > >
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x08400000
> > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
> > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08410000
> > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000
> > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08420000
> > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
> > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00080000
> > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08480000
> > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00080000
> > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08500000
> > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00080000
> > > >
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId|"SNI "
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId|0x52434155514e5953 # SYNQUACR
> > > >
> >
> >
> >
> > --
> > Masami Hiramatsu
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes
[not found] ` <16BC25260C31EA7F.23256@groups.io>
@ 2021-12-03 6:17 ` Masami Hiramatsu
0 siblings, 0 replies; 21+ messages in thread
From: Masami Hiramatsu @ 2021-12-03 6:17 UTC (permalink / raw)
To: devel, masami.hiramatsu
Cc: Leif Lindholm, Ard Biesheuvel, Kazuhiko Sakamoto, Masahisa Kojima
Hi Leif,
I found that "gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision" is a
digit number.
On the DeveloperBox, it is the same as the BUILD_NUMBER which was set
when building the EDK2 as below.
Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc:
gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision|$(BUILD_NUMBER)
And now it becomes 99 in official snapshot (this will be the old series)
http://snapshots.linaro.org/components/kernel/leg-96boards-developerbox-edk2/99/
However, this snapshot is actually not updated a while ago since the
source repository is not more maintained.
Anyway, my point is that the FirmwareRevision is not set as a fixed
number, but it will be given at the build time.
In that case, it is hard to set the PcdLowestSupportedFirmwareVersion
because the PcdFirmwareRevision can be freely editable by who is
releasing the firmware (and now no one officially releases it. just a
bot repeating compilation without any update)
My idea is to keep the PcdFirmwareRevision as is, but only set the
PcdLowestSupportedFirmwareVersion as for example 1024 with the
variable area is extended. The person who maintains the firmware
release build needs to care about setting BUILD_NUMBER more than 1024.
This will prevent users to rollback to older image accidentary, but
not prevent the careless developer to build their own binary with
lower BUILD_NUMBER etc.
What do you think?
Thank you,
2021年11月30日(火) 7:33 Masami Hiramatsu via groups.io
<masami.hiramatsu=linaro.org@groups.io>:
>
> Hi Leif,
>
> 2021年11月29日(月) 22:43 Leif Lindholm <leif@nuviainc.com>:
> >
> > On Sat, Nov 27, 2021 at 16:48:45 +0900, Masami Hiramatsu wrote:
> > > > On Fri, Nov 05, 2021 at 18:24:01 +0900, Masami Hiramatsu wrote:
> > > > > Expand NvStorage Variable size and FTW spare/working size
> > > > > for the DeveloperBox platform.
> > > > >
> > > > > Since the size of the NvStorage VariableSize is not enough
> > > > > large, FWTS uefirttime test, which updates the NV
> > > > > variables in runtime, failes. This expands the size to fix
> > > > > this issue.
> > > >
> > > > Does this change erase all existing variables?
> > >
> > > Ah, indeed. It may need to erase all variables.
> >
> > That is quite likely to lead to upset users.
>
> OK.
>
> > > > If so, I think it is worth introducing this as a non-default build
> > > > option, in order to not wreck existing installations on a firmware
> > > > update.
> > > >
> > > > I think it would also be worth considering whether to update
> > > > PcdLowestSupportedFirmwareVersion. PcdFirmwareRevision
> > > > should definitely be updated.
> > >
> > > I'm not sure about this point.
> > > You meant we should have 2 different revisions like a branch?
> > > - Branch A(current version): keep the variable area size the same.
> > > - Branch B(new version): expand the variable area.
> > > And a build option will change the branch by updating the
> > > PcdFirmwareRevision?
> >
> > Not a branch - just that you need to explicitly build for the size of
> > flash area you want to use, and if you provide pre-built downloadable
> > ones - provide two variants.
>
> I got it.
>
> > This becomes a bit of a maintenance nightmare over time.
>
> Actually, I'm considering a kind of "leap" firmware release, which
> involves all firmware update by manual (not automatic), because the
> SCP-firmware is too old anymore and the new SCP firmware (OSS version)
> requires to update TF-A, which is not compatible with old ones.
> Obviously, this must be done by manual.
>
> So, afterwards, we will not release old version anymore. Anyway, the
> old firmware snapshot image is not updated in one year (since source
> repository has not been updated). The firmware on LVFS is released in
> 2019.
> (BTW, can I change the UUID which fwupd detects too?)
>
> I will provide a build option for the users who update EDK2 by themselves.
>
> > A better solution would be for the firmware to (somehow) resize the
> > parameter area - retaining existing values - if it encounters the
> > smaller version. I don't think we have an example of that.
>
> Hmm, I rather like to erase it while the "leap" update, since the backward
> compatibility is not guaranteed. And after the update, user will be able
> to choose the U-Boot on the DeveloperBox.
>
> > PcdLowestSupportedFirmwareVersion still needs to be set, to the same
> > value as the new PcdFirmwareRevision, to prevent downgrading to a
> > version that does not support the larger size.
>
> OK, so this is for protecting rollback. But this means, do I need to make
> it optional (switched by build option) too?
>
> >
> > > Also PcdLowestSupportedFirmwareVersion you meant is
> > > in the capsule file?
> >
> > I meant to change the Pcd value. That implements the change in
> > SystemFirmwareDescriptorTable.aslc.
>
> OK.
>
> Thank you,
>
> >
> > Regards,
> >
> > Leif
> >
> > > Thank you,
> > >
> > >
> > > >
> > > > /
> > > > Leif
> > > >
> > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > > > > ---
> > > > > .../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +++++-----
> > > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > > > index 0a364bc457..3baf97ecc0 100644
> > > > > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > > > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > > > @@ -280,11 +280,11 @@
> > > > > gFip006DxeTokenSpaceGuid.PcdFip006DxeMemBaseAddress|0x08000000
> > > > >
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x08400000
> > > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
> > > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08410000
> > > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000
> > > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08420000
> > > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
> > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00080000
> > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08480000
> > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00080000
> > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08500000
> > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00080000
> > > > >
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId|"SNI "
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId|0x52434155514e5953 # SYNQUACR
> > > > >
> > >
> > >
> > >
> > > --
> > > Masami Hiramatsu
>
>
>
> --
> Masami Hiramatsu
>
>
>
>
>
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-12-03 6:17 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-05 9:23 [PATCH 0/5] Series short description Masami Hiramatsu
2021-11-05 9:23 ` [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy Masami Hiramatsu
2021-11-26 17:47 ` Leif Lindholm
2021-11-27 2:45 ` Masami Hiramatsu
2021-11-05 9:23 ` [PATCH 2/5] [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number Masami Hiramatsu
2021-11-26 17:50 ` Leif Lindholm
2021-11-27 4:43 ` Masami Hiramatsu
2021-11-05 9:23 ` [PATCH 3/5] [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks Masami Hiramatsu
2021-11-26 17:52 ` Leif Lindholm
2021-11-05 9:23 ` [PATCH 4/5] [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table Masami Hiramatsu
2021-11-26 18:10 ` Leif Lindholm
2021-11-27 7:52 ` Masami Hiramatsu
2021-11-05 9:24 ` [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes Masami Hiramatsu
2021-11-26 18:19 ` Leif Lindholm
2021-11-27 7:48 ` Masami Hiramatsu
2021-11-29 13:42 ` Leif Lindholm
2021-11-29 22:33 ` Masami Hiramatsu
[not found] ` <16BC25260C31EA7F.23256@groups.io>
2021-12-03 6:17 ` [edk2-devel] " Masami Hiramatsu
2021-11-25 11:19 ` [PATCH 0/5] Series short description Masami Hiramatsu
2021-11-25 16:40 ` Leif Lindholm
2021-11-26 0:58 ` Masami Hiramatsu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox