[PATCH edk2-platforms 0/3] Silicon/SynQuacer: preparatory ConnectAll fixes

[PATCH edk2-platforms 0/3] Silicon/SynQuacer: preparatory ConnectAll fixes

[Ard Biesheuvel]

The ArmPkg BDS platform lib still calls ConnectAll(), which is something
we should try to remove at some point.

This series does some preparatory work on SynQuacer so that everything works
as expected regardless of whether ConnectAll() is used: connect the I2C
buses, which carry devices that should be connected regardless (as they
are not boot targets). Also, let's fix the clunky PHY detect handling in
the NETSEC driver so that even if we connect the driver at boot, we only
delay the boot if the upper networking layers require it.


Ard Biesheuvel (3):
  Platform/96Boards/96BoardsI2cDxe: connect I2C controllers at EndOfDxe
  Silicon/SynQuacer/NetsecDxe: drop false dependency on device path
    protocol
  Silicon/SynQuacer/NetsecDxe: avoid media detection delay at boot

 .../Drivers/Net/NetsecDxe/NetsecDxe.dec       |   1 -
 .../Socionext/DeveloperBox/DeveloperBox.dsc   |   1 -
 .../96BoardsI2cDxe/96BoardsI2cDxe.inf         |   5 +-
 .../Drivers/Net/NetsecDxe/NetsecDxe.inf       |   4 +-
 .../Drivers/Net/NetsecDxe/NetsecDxe.h         |   5 +
 .../96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c  |  18 +++
 .../Drivers/Net/NetsecDxe/NetsecDxe.c         | 112 +++++++++++++++---
 7 files changed, 125 insertions(+), 21 deletions(-)

-- 
2.17.1

[PATCH edk2-platforms 1/3] Platform/96Boards/96BoardsI2cDxe: connect I2C controllers at EndOfDxe

[Ard Biesheuvel]

The 96boards I2C driver currently relies on the platform to connect
all controllers, or I2C peripherals will not be exposed if they are
not the active boot target. Since I2C peripherals are not boot targets
in the first place, but are used to expose things like random number
generators, let's connect the I2C controllers specifically at EndOfDxe
so that the devices living on it will be available regardless of the
boot policy.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf |  5 ++++-
 Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c   | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
index ae69f0933e93..3d9ca559e60b 100644
--- a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
+++ b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
@@ -36,10 +36,13 @@ [Protocols]
 [Guids]
   g96BoardsI2c0MasterGuid
   g96BoardsI2c1MasterGuid
+  gEfiEndOfDxeEventGroupGuid
 
 [FixedPcd]
   g96BoardsTokenSpaceGuid.PcdI2c0BusFrequencyHz
   g96BoardsTokenSpaceGuid.PcdI2c1BusFrequencyHz
 
 [Depex]
-  g96BoardsMezzanineProtocolGuid AND g96BoardsI2c0MasterGuid OR g96BoardsI2c1MasterGuid
+  g96BoardsMezzanineProtocolGuid AND (
+    g96BoardsI2c0MasterGuid OR g96BoardsI2c1MasterGuid
+  )
diff --git a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
index e4ecbca62c0c..a751769cf691 100644
--- a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
+++ b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
@@ -179,6 +179,19 @@ RegisterI2cBus (
   ASSERT_EFI_ERROR (Status);
 }
 
+STATIC
+VOID
+EFIAPI
+OnEndOfDxe (
+  IN EFI_EVENT  Event,
+  IN VOID       *Context
+  )
+{
+  gBS->CloseEvent (Event);
+  gBS->ConnectController (mI2cBus0.I2cMasterHandle, NULL, NULL, TRUE);
+  gBS->ConnectController (mI2cBus1.I2cMasterHandle, NULL, NULL, TRUE);
+}
+
 EFI_STATUS
 EFIAPI
 EntryPoint (
@@ -187,6 +200,7 @@ EntryPoint (
   )
 {
   EFI_STATUS    Status;
+  EFI_EVENT     EndOfDxeEvent;
 
   Status = gBS->LocateProtocol (&g96BoardsMezzanineProtocolGuid, NULL,
                   (VOID **)&mMezzanine);
@@ -197,5 +211,9 @@ EntryPoint (
   RegisterI2cBus (&g96BoardsI2c1MasterGuid, &mI2cBus1,
     mMezzanine->I2c1NumDevices, mMezzanine->I2c1DeviceArray);
 
+  Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_CALLBACK, OnEndOfDxe,
+                  NULL, &gEfiEndOfDxeEventGroupGuid, &EndOfDxeEvent);
+  ASSERT_EFI_ERROR (Status);
+
   return EFI_SUCCESS;
 }
-- 
2.17.1

[PATCH edk2-platforms 2/3] Silicon/SynQuacer/NetsecDxe: drop false dependency on device path protocol

[Ard Biesheuvel]

The device path protocol is no longer installed by the NetSec driver,
but by the platform driver. So drop it from the .INF.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf
index abc98183668a..0efba0bbbf94 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf
+++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf
@@ -52,7 +52,6 @@ [Guids]
 [Protocols]
   gEfiCpuArchProtocolGuid                     ## CONSUMES
   gEdkiiNonDiscoverableDeviceProtocolGuid     ## TO_START
-  gEfiDevicePathProtocolGuid                  ## BY_START
   gEfiSimpleNetworkProtocolGuid               ## BY_START
 
 [FixedPcd]
-- 
2.17.1

[PATCH edk2-platforms 3/3] Silicon/SynQuacer/NetsecDxe: avoid media detection delay at boot

[Ard Biesheuvel]

Instead of unconditionally delaying the boot up to 5 seconds, even
if no network cable is connected in the first place, provide an
implementation of the EFI adapter information protocol so that the
upper networking layers can wait gracefully for the link to come up,
but only when the network is actually used to boot from.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec |   1 -
 Platform/Socionext/DeveloperBox/DeveloperBox.dsc                |   1 -
 Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf |   3 +-
 Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h   |   5 +
 Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c   | 112 +++++++++++++++++---
 5 files changed, 103 insertions(+), 19 deletions(-)

diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec
index 3b1de62c6e31..6b9f60293879 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec
+++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec
@@ -37,5 +37,4 @@ [PcdsFixedAtBuild.common]
   gNetsecDxeTokenSpaceGuid.PcdFlowCtrl|0x0|UINT8|0x00000005
   gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStartThreshold|0x0|UINT16|0x00000006
   gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStopThreshold|0x0|UINT16|0x00000007
-  gNetsecDxeTokenSpaceGuid.PcdMediaDetectTimeoutOnBoot|0x0|UINT8|0x00000009
   gNetsecDxeTokenSpaceGuid.PcdPauseTime|0x0|UINT16|0x00000008
diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
index 9307edefb11a..a459079b1f26 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
@@ -165,7 +165,6 @@ [PcdsFixedAtBuild]
   gNetsecDxeTokenSpaceGuid.PcdFlowCtrl|0
   gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStartThreshold|36
   gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStopThreshold|48
-  gNetsecDxeTokenSpaceGuid.PcdMediaDetectTimeoutOnBoot|5
   gNetsecDxeTokenSpaceGuid.PcdPauseTime|256
 
   gSynQuacerTokenSpaceGuid.PcdNetsecEepromBase|0x08080000
diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf
index 0efba0bbbf94..8ea959c37bab 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf
+++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf
@@ -47,11 +47,13 @@ [LibraryClasses]
   UefiLib
 
 [Guids]
+  gEfiAdapterInfoMediaStateGuid
   gNetsecNonDiscoverableDeviceGuid
 
 [Protocols]
   gEfiCpuArchProtocolGuid                     ## CONSUMES
   gEdkiiNonDiscoverableDeviceProtocolGuid     ## TO_START
+  gEfiAdapterInformationProtocolGuid          ## BY_START
   gEfiSimpleNetworkProtocolGuid               ## BY_START
 
 [FixedPcd]
@@ -61,5 +63,4 @@ [FixedPcd]
   gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStartThreshold
   gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStopThreshold
   gNetsecDxeTokenSpaceGuid.PcdJumboPacket
-  gNetsecDxeTokenSpaceGuid.PcdMediaDetectTimeoutOnBoot
   gNetsecDxeTokenSpaceGuid.PcdPauseTime
diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h
index 17a7032f0f41..cf2abb0ab108 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h
+++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h
@@ -20,6 +20,7 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
 
+#include <Protocol/AdapterInformation.h>
 #include <Protocol/NonDiscoverableDevice.h>
 
 #include "netsec_for_uefi/netsec_sdk/include/ogma_api.h"
@@ -50,6 +51,9 @@ typedef struct {
   // EFI Snp statistics instance
   EFI_NETWORK_STATISTICS            Stats;
 
+  // Adapter Information protocol
+  EFI_ADAPTER_INFORMATION_PROTOCOL  Aip;
+
   // ogma handle
   ogma_handle_t                     Handle;
 
@@ -65,6 +69,7 @@ typedef struct {
 
 #define NETSEC_SIGNATURE            SIGNATURE_32('n', 't', 's', 'c')
 #define INSTANCE_FROM_SNP_THIS(a)   CR((a), NETSEC_DRIVER, Snp, NETSEC_SIGNATURE)
+#define INSTANCE_FROM_AIP_THIS(a)   CR((a), NETSEC_DRIVER, Aip, NETSEC_SIGNATURE)
 
 /*------------------------------------------------------------------------------
 
diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c
index 4e3c4e6c807a..c9fc4d6e2d8e 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c
@@ -279,8 +279,6 @@ SnpInitialize (
 
   ogma_err_t              ogma_err;
 
-  UINT32                  Index;
-
   // Check Snp Instance
   if (Snp == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -363,20 +361,6 @@ SnpInitialize (
   ogma_disable_desc_ring_irq (LanDriver->Handle, OGMA_DESC_RING_ID_NRM_TX,
                               OGMA_CH_IRQ_REG_EMPTY);
 
-  // Wait for media linking up
-  for (Index = 0; Index < (UINT32)FixedPcdGet8 (PcdMediaDetectTimeoutOnBoot) * 10; Index++) {
-    Status = NetsecUpdateLink (Snp);
-    if (Status != EFI_SUCCESS) {
-      ReturnUnlock (EFI_DEVICE_ERROR);
-    }
-
-    if (Snp->Mode->MediaPresent) {
-      break;
-    }
-
-    MicroSecondDelay(100000);
-  }
-
   // Declare the driver as initialized
   Snp->Mode->State = EfiSimpleNetworkInitialized;
   Status = EFI_SUCCESS;
@@ -948,6 +932,96 @@ SnpReceive (
   return Status;
 }
 
+STATIC
+EFI_STATUS
+EFIAPI
+NetsecAipGetInformation (
+  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
+  IN  EFI_GUID                          *InformationType,
+  OUT VOID                              **InformationBlock,
+  OUT UINTN                             *InformationBlockSize
+  )
+{
+  EFI_ADAPTER_INFO_MEDIA_STATE  *AdapterInfo;
+  NETSEC_DRIVER                 *LanDriver;
+
+  if (This == NULL || InformationBlock == NULL ||
+      InformationBlockSize == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (!CompareGuid (InformationType, &gEfiAdapterInfoMediaStateGuid)) {
+    return EFI_UNSUPPORTED;
+  }
+
+  AdapterInfo = AllocateZeroPool (sizeof (EFI_ADAPTER_INFO_MEDIA_STATE));
+  if (AdapterInfo == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  *InformationBlock = AdapterInfo;
+  *InformationBlockSize = sizeof (EFI_ADAPTER_INFO_MEDIA_STATE);
+
+  LanDriver = INSTANCE_FROM_AIP_THIS (This);
+  if (LanDriver->Snp.Mode->MediaPresent) {
+    AdapterInfo->MediaState = EFI_SUCCESS;
+  } else {
+    AdapterInfo->MediaState = EFI_NOT_READY;
+  }
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+NetsecAipSetInformation (
+  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
+  IN  EFI_GUID                          *InformationType,
+  IN  VOID                              *InformationBlock,
+  IN  UINTN                             InformationBlockSize
+  )
+{
+  if (This == NULL || InformationBlock == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (CompareGuid (InformationType, &gEfiAdapterInfoMediaStateGuid)) {
+    return EFI_WRITE_PROTECTED;
+  }
+
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+NetsecAipGetSupportedTypes (
+  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
+  OUT EFI_GUID                          **InfoTypesBuffer,
+  OUT UINTN                             *InfoTypesBufferCount
+  )
+{
+  EFI_GUID    *Guid;
+
+  if (This == NULL || InfoTypesBuffer == NULL ||
+      InfoTypesBufferCount == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Guid = AllocatePool (sizeof *Guid);
+  if (Guid == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  CopyGuid (Guid, &gEfiAdapterInfoMediaStateGuid);
+
+  *InfoTypesBuffer      = Guid;
+  *InfoTypesBufferCount = 1;
+
+  return EFI_SUCCESS;
+}
+
 EFI_STATUS
 NetsecInit (
   IN      EFI_HANDLE        DriverBindingHandle,
@@ -1046,6 +1120,10 @@ NetsecInit (
   SnpMode->MediaPresentSupported = TRUE;
   SnpMode->MediaPresent = FALSE;
 
+  LanDriver->Aip.GetInformation     = NetsecAipGetInformation;
+  LanDriver->Aip.SetInformation     = NetsecAipSetInformation;
+  LanDriver->Aip.GetSupportedTypes  = NetsecAipGetSupportedTypes;
+
   //  Set broadcast address
   SetMem (&SnpMode->BroadcastAddress, sizeof (EFI_MAC_ADDRESS), 0xFF);
 
@@ -1055,6 +1133,7 @@ NetsecInit (
   Status = gBS->InstallMultipleProtocolInterfaces (
                   &ControllerHandle,
                   &gEfiSimpleNetworkProtocolGuid, Snp,
+                  &gEfiAdapterInformationProtocolGuid, &LanDriver->Aip,
                   NULL);
 
   LanDriver->ControllerHandle = ControllerHandle;
@@ -1100,6 +1179,7 @@ NetsecRelease (
 
   Status = gBS->UninstallMultipleProtocolInterfaces (ControllerHandle,
                   &gEfiSimpleNetworkProtocolGuid, Snp,
+                  &gEfiAdapterInformationProtocolGuid, &LanDriver->Aip,
                   NULL);
   if (EFI_ERROR (Status)) {
     return Status;
-- 
2.17.1

Re: [PATCH edk2-platforms 1/3] Platform/96Boards/96BoardsI2cDxe: connect I2C controllers at EndOfDxe

[Leif Lindholm]

On Sat, May 16, 2020 at 19:59:32 +0200, Ard Biesheuvel wrote:
> The 96boards I2C driver currently relies on the platform to connect
> all controllers, or I2C peripherals will not be exposed if they are
> not the active boot target. Since I2C peripherals are not boot targets
> in the first place, but are used to expose things like random number
> generators, let's connect the I2C controllers specifically at EndOfDxe
> so that the devices living on it will be available regardless of the
> boot policy.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf |  5 ++++-
>  Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c   | 18 ++++++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
> index ae69f0933e93..3d9ca559e60b 100644
> --- a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
> +++ b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
> @@ -36,10 +36,13 @@ [Protocols]
>  [Guids]
>    g96BoardsI2c0MasterGuid
>    g96BoardsI2c1MasterGuid
> +  gEfiEndOfDxeEventGroupGuid
>  
>  [FixedPcd]
>    g96BoardsTokenSpaceGuid.PcdI2c0BusFrequencyHz
>    g96BoardsTokenSpaceGuid.PcdI2c1BusFrequencyHz
>  
>  [Depex]
> -  g96BoardsMezzanineProtocolGuid AND g96BoardsI2c0MasterGuid OR g96BoardsI2c1MasterGuid
> +  g96BoardsMezzanineProtocolGuid AND (
> +    g96BoardsI2c0MasterGuid OR g96BoardsI2c1MasterGuid
> +  )

Is this change actually a bugfix?
It appears unrelated to the patch description as such, although
clearly an improvement.

/
    Leif

> diff --git a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
> index e4ecbca62c0c..a751769cf691 100644
> --- a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
> +++ b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
> @@ -179,6 +179,19 @@ RegisterI2cBus (
>    ASSERT_EFI_ERROR (Status);
>  }
>  
> +STATIC
> +VOID
> +EFIAPI
> +OnEndOfDxe (
> +  IN EFI_EVENT  Event,
> +  IN VOID       *Context
> +  )
> +{
> +  gBS->CloseEvent (Event);
> +  gBS->ConnectController (mI2cBus0.I2cMasterHandle, NULL, NULL, TRUE);
> +  gBS->ConnectController (mI2cBus1.I2cMasterHandle, NULL, NULL, TRUE);
> +}
> +
>  EFI_STATUS
>  EFIAPI
>  EntryPoint (
> @@ -187,6 +200,7 @@ EntryPoint (
>    )
>  {
>    EFI_STATUS    Status;
> +  EFI_EVENT     EndOfDxeEvent;
>  
>    Status = gBS->LocateProtocol (&g96BoardsMezzanineProtocolGuid, NULL,
>                    (VOID **)&mMezzanine);
> @@ -197,5 +211,9 @@ EntryPoint (
>    RegisterI2cBus (&g96BoardsI2c1MasterGuid, &mI2cBus1,
>      mMezzanine->I2c1NumDevices, mMezzanine->I2c1DeviceArray);
>  
> +  Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_CALLBACK, OnEndOfDxe,
> +                  NULL, &gEfiEndOfDxeEventGroupGuid, &EndOfDxeEvent);
> +  ASSERT_EFI_ERROR (Status);
> +
>    return EFI_SUCCESS;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms 1/3] Platform/96Boards/96BoardsI2cDxe: connect I2C controllers at EndOfDxe

[Ard Biesheuvel]

On 5/18/20 7:26 PM, Leif Lindholm wrote:
> On Sat, May 16, 2020 at 19:59:32 +0200, Ard Biesheuvel wrote:
>> The 96boards I2C driver currently relies on the platform to connect
>> all controllers, or I2C peripherals will not be exposed if they are
>> not the active boot target. Since I2C peripherals are not boot targets
>> in the first place, but are used to expose things like random number
>> generators, let's connect the I2C controllers specifically at EndOfDxe
>> so that the devices living on it will be available regardless of the
>> boot policy.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>   Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf |  5 ++++-
>>   Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c   | 18 ++++++++++++++++++
>>   2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
>> index ae69f0933e93..3d9ca559e60b 100644
>> --- a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
>> +++ b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
>> @@ -36,10 +36,13 @@ [Protocols]
>>   [Guids]
>>     g96BoardsI2c0MasterGuid
>>     g96BoardsI2c1MasterGuid
>> +  gEfiEndOfDxeEventGroupGuid
>>   
>>   [FixedPcd]
>>     g96BoardsTokenSpaceGuid.PcdI2c0BusFrequencyHz
>>     g96BoardsTokenSpaceGuid.PcdI2c1BusFrequencyHz
>>   
>>   [Depex]
>> -  g96BoardsMezzanineProtocolGuid AND g96BoardsI2c0MasterGuid OR g96BoardsI2c1MasterGuid
>> +  g96BoardsMezzanineProtocolGuid AND (
>> +    g96BoardsI2c0MasterGuid OR g96BoardsI2c1MasterGuid
>> +  )
> 
> Is this change actually a bugfix?
> It appears unrelated to the patch description as such, although
> clearly an improvement.
> 

Apologies, I should have dropped that. I though it was a bug, but when i 
looked at the resulting DEPEX, they are actually the same.



> 
>> diff --git a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
>> index e4ecbca62c0c..a751769cf691 100644
>> --- a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
>> +++ b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
>> @@ -179,6 +179,19 @@ RegisterI2cBus (
>>     ASSERT_EFI_ERROR (Status);
>>   }
>>   
>> +STATIC
>> +VOID
>> +EFIAPI
>> +OnEndOfDxe (
>> +  IN EFI_EVENT  Event,
>> +  IN VOID       *Context
>> +  )
>> +{
>> +  gBS->CloseEvent (Event);
>> +  gBS->ConnectController (mI2cBus0.I2cMasterHandle, NULL, NULL, TRUE);
>> +  gBS->ConnectController (mI2cBus1.I2cMasterHandle, NULL, NULL, TRUE);
>> +}
>> +
>>   EFI_STATUS
>>   EFIAPI
>>   EntryPoint (
>> @@ -187,6 +200,7 @@ EntryPoint (
>>     )
>>   {
>>     EFI_STATUS    Status;
>> +  EFI_EVENT     EndOfDxeEvent;
>>   
>>     Status = gBS->LocateProtocol (&g96BoardsMezzanineProtocolGuid, NULL,
>>                     (VOID **)&mMezzanine);
>> @@ -197,5 +211,9 @@ EntryPoint (
>>     RegisterI2cBus (&g96BoardsI2c1MasterGuid, &mI2cBus1,
>>       mMezzanine->I2c1NumDevices, mMezzanine->I2c1DeviceArray);
>>   
>> +  Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_CALLBACK, OnEndOfDxe,
>> +                  NULL, &gEfiEndOfDxeEventGroupGuid, &EndOfDxeEvent);
>> +  ASSERT_EFI_ERROR (Status);
>> +
>>     return EFI_SUCCESS;
>>   }
>> -- 
>> 2.17.1
>>

Re: [PATCH edk2-platforms 1/3] Platform/96Boards/96BoardsI2cDxe: connect I2C controllers at EndOfDxe

[Leif Lindholm]

On Tue, May 19, 2020 at 12:23:21 +0200, Ard Biesheuvel wrote:
> On 5/18/20 7:26 PM, Leif Lindholm wrote:
> > On Sat, May 16, 2020 at 19:59:32 +0200, Ard Biesheuvel wrote:
> > > The 96boards I2C driver currently relies on the platform to connect
> > > all controllers, or I2C peripherals will not be exposed if they are
> > > not the active boot target. Since I2C peripherals are not boot targets
> > > in the first place, but are used to expose things like random number
> > > generators, let's connect the I2C controllers specifically at EndOfDxe
> > > so that the devices living on it will be available regardless of the
> > > boot policy.
> > > 
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > ---
> > >   Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf |  5 ++++-
> > >   Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c   | 18 ++++++++++++++++++
> > >   2 files changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
> > > index ae69f0933e93..3d9ca559e60b 100644
> > > --- a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
> > > +++ b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.inf
> > > @@ -36,10 +36,13 @@ [Protocols]
> > >   [Guids]
> > >     g96BoardsI2c0MasterGuid
> > >     g96BoardsI2c1MasterGuid
> > > +  gEfiEndOfDxeEventGroupGuid
> > >   [FixedPcd]
> > >     g96BoardsTokenSpaceGuid.PcdI2c0BusFrequencyHz
> > >     g96BoardsTokenSpaceGuid.PcdI2c1BusFrequencyHz
> > >   [Depex]
> > > -  g96BoardsMezzanineProtocolGuid AND g96BoardsI2c0MasterGuid OR g96BoardsI2c1MasterGuid
> > > +  g96BoardsMezzanineProtocolGuid AND (
> > > +    g96BoardsI2c0MasterGuid OR g96BoardsI2c1MasterGuid
> > > +  )
> > 
> > Is this change actually a bugfix?
> > It appears unrelated to the patch description as such, although
> > clearly an improvement.
> > 
> 
> Apologies, I should have dropped that. I though it was a bug, but when i
> looked at the resulting DEPEX, they are actually the same.

If you submit it separately, I'll ack it - it is clearly a readability
improvement.

With that bit dropped, for the series:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> 
> 
> 
> > 
> > > diff --git a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
> > > index e4ecbca62c0c..a751769cf691 100644
> > > --- a/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
> > > +++ b/Platform/96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c
> > > @@ -179,6 +179,19 @@ RegisterI2cBus (
> > >     ASSERT_EFI_ERROR (Status);
> > >   }
> > > +STATIC
> > > +VOID
> > > +EFIAPI
> > > +OnEndOfDxe (
> > > +  IN EFI_EVENT  Event,
> > > +  IN VOID       *Context
> > > +  )
> > > +{
> > > +  gBS->CloseEvent (Event);
> > > +  gBS->ConnectController (mI2cBus0.I2cMasterHandle, NULL, NULL, TRUE);
> > > +  gBS->ConnectController (mI2cBus1.I2cMasterHandle, NULL, NULL, TRUE);
> > > +}
> > > +
> > >   EFI_STATUS
> > >   EFIAPI
> > >   EntryPoint (
> > > @@ -187,6 +200,7 @@ EntryPoint (
> > >     )
> > >   {
> > >     EFI_STATUS    Status;
> > > +  EFI_EVENT     EndOfDxeEvent;
> > >     Status = gBS->LocateProtocol (&g96BoardsMezzanineProtocolGuid, NULL,
> > >                     (VOID **)&mMezzanine);
> > > @@ -197,5 +211,9 @@ EntryPoint (
> > >     RegisterI2cBus (&g96BoardsI2c1MasterGuid, &mI2cBus1,
> > >       mMezzanine->I2c1NumDevices, mMezzanine->I2c1DeviceArray);
> > > +  Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_CALLBACK, OnEndOfDxe,
> > > +                  NULL, &gEfiEndOfDxeEventGroupGuid, &EndOfDxeEvent);
> > > +  ASSERT_EFI_ERROR (Status);
> > > +
> > >     return EFI_SUCCESS;
> > >   }
> > > -- 
> > > 2.17.1
> > > 
> 

Re: [PATCH edk2-platforms 0/3] Silicon/SynQuacer: preparatory ConnectAll fixes

[Ard Biesheuvel]

On 5/16/20 7:59 PM, Ard Biesheuvel wrote:
> The ArmPkg BDS platform lib still calls ConnectAll(), which is something
> we should try to remove at some point.
> 
> This series does some preparatory work on SynQuacer so that everything works
> as expected regardless of whether ConnectAll() is used: connect the I2C
> buses, which carry devices that should be connected regardless (as they
> are not boot targets). Also, let's fix the clunky PHY detect handling in
> the NETSEC driver so that even if we connect the driver at boot, we only
> delay the boot if the upper networking layers require it.
> 
> 
> Ard Biesheuvel (3):
>    Platform/96Boards/96BoardsI2cDxe: connect I2C controllers at EndOfDxe
>    Silicon/SynQuacer/NetsecDxe: drop false dependency on device path
>      protocol
>    Silicon/SynQuacer/NetsecDxe: avoid media detection delay at boot
> 

Series pushed @ b29531f3186a0f65f7fe6870cc877dac5be7e7d3

thanks

>   .../Drivers/Net/NetsecDxe/NetsecDxe.dec       |   1 -
>   .../Socionext/DeveloperBox/DeveloperBox.dsc   |   1 -
>   .../96BoardsI2cDxe/96BoardsI2cDxe.inf         |   5 +-
>   .../Drivers/Net/NetsecDxe/NetsecDxe.inf       |   4 +-
>   .../Drivers/Net/NetsecDxe/NetsecDxe.h         |   5 +
>   .../96Boards/96BoardsI2cDxe/96BoardsI2cDxe.c  |  18 +++
>   .../Drivers/Net/NetsecDxe/NetsecDxe.c         | 112 +++++++++++++++---
>   7 files changed, 125 insertions(+), 21 deletions(-)
>