[edk2-platforms: PATCH 0/4] Armada 7k8k network improvements

[edk2-platforms: PATCH 0/4] Armada 7k8k network improvements

[Marcin Wojtas]

Hi,

This small patchset extends PP2 NIC driver with the basic
AdapterInformationProtocol support and adds 88E1112 PHY support.
Also a minor fix is included on top.

The patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/net-upstream-r20190503

I'm looking forward to your comments or remarks.

Best regards,
Marcin

Marcin Wojtas (3):
  Marvell/Drivers: MvPhyDxe: Improve 88E1510 initialization
  Marvell/Drivers: MvPhyDxe: Introduce 88E1112 initialization
  Marvell/Drivers: MvPhyDxe: Reset PHY only once

Tomasz Michalec (1):
  Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol

 Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf   |   1 +
 Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.h |  17 ++-
 Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h     |   3 +
 Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c | 111 ++++++++++----
 Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c     | 157 ++++++++++++++++++++
 Silicon/Marvell/Documentation/PortingGuide.txt  |   7 +-
 6 files changed, 265 insertions(+), 31 deletions(-)

-- 
2.7.4

[edk2-platforms: PATCH 1/4] Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol

[Marcin Wojtas]

From: Tomasz Michalec <tm@semihalf.com>

Add implementation of Adapter Information Protocol in Armada 7k8k
PP2 NIC driver. Support retrieving information about media state
which allows to use NetLibDetectMediaWaitTimeout on network interface.
This patch fixes possible hangs when attempting to PXE boot on
unconnected interfaces.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Tomasz Michalec <tm@semihalf.com>
---
 Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf |   1 +
 Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h   |   3 +
 Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c   | 157 ++++++++++++++++++++
 3 files changed, 161 insertions(+)

diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
index be536ab..73aa413 100644
--- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
+++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
@@ -64,6 +64,7 @@
   CacheMaintenanceLib
 
 [Protocols]
+  gEfiAdapterInformationProtocolGuid
   gEfiSimpleNetworkProtocolGuid
   gEfiDevicePathProtocolGuid
   gEfiCpuArchProtocolGuid
diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
index b8a5dae..66bd46a 100644
--- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
+++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
@@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #ifndef __PP2_DXE_H__
 #define __PP2_DXE_H__
 
+#include <Protocol/AdapterInformation.h>
 #include <Protocol/Cpu.h>
 #include <Protocol/DevicePath.h>
 #include <Protocol/DriverBinding.h>
@@ -59,6 +60,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #define MVPP2_MAX_PORT  3
 
 #define PP2DXE_SIGNATURE                    SIGNATURE_32('P', 'P', '2', 'D')
+#define INSTANCE_FROM_AIP(a)                CR((a), PP2DXE_CONTEXT, Aip, PP2DXE_SIGNATURE)
 #define INSTANCE_FROM_SNP(a)                CR((a), PP2DXE_CONTEXT, Snp, PP2DXE_SIGNATURE)
 
 /* OS API */
@@ -365,6 +367,7 @@ typedef struct {
   UINTN                       CompletionQueueTail;
   EFI_EVENT                   EfiExitBootServicesEvent;
   PP2_DEVICE_PATH             *DevicePath;
+  EFI_ADAPTER_INFORMATION_PROTOCOL Aip;
 } PP2DXE_CONTEXT;
 
 /* Inline helpers */
diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
index 02b2798..473a2a1 100644
--- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
+++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
@@ -1129,6 +1129,7 @@ Pp2DxeSnpInstall (
       &Handle,
       &gEfiSimpleNetworkProtocolGuid, &Pp2Context->Snp,
       &gEfiDevicePathProtocolGuid, Pp2DevicePath,
+      &gEfiAdapterInformationProtocolGuid, &Pp2Context->Aip,
       NULL
       );
 
@@ -1139,6 +1140,157 @@ Pp2DxeSnpInstall (
   return Status;
 }
 
+/**
+  Returns the current state information for the adapter.
+
+  This function returns information of type InformationType from the adapter.
+  If an adapter does not support the requested informational type, then
+  EFI_UNSUPPORTED is returned.
+
+  @param[in]  This                   A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
+  @param[in]  InformationType        A pointer to an EFI_GUID that defines the contents of InformationBlock.
+  @param[out] InforamtionBlock       The service returns a pointer to the buffer with the InformationBlock
+                                     structure which contains details about the data specific to InformationType.
+  @param[out] InforamtionBlockSize   The driver returns the size of the InformationBlock in bytes.
+
+  @retval EFI_SUCCESS                The InformationType information was retrieved.
+  @retval EFI_UNSUPPORTED            The InformationType is not known.
+  @retval EFI_DEVICE_ERROR           The device reported an error.
+  @retval EFI_OUT_OF_RESOURCES       The request could not be completed due to a lack of resources.
+  @retval EFI_INVALID_PARAMETER      This is NULL.
+  @retval EFI_INVALID_PARAMETER      InformationBlock is NULL.
+  @retval EFI_INVALID_PARAMETER      InformationBlockSize is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+Pp2AipGetInformation (
+  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
+  IN  EFI_GUID                          *InformationType,
+  OUT VOID                              **InformationBlock,
+  OUT UINTN                             *InformationBlockSize
+  )
+{
+  EFI_ADAPTER_INFO_MEDIA_STATE  *AdapterInfo;
+  PP2DXE_CONTEXT                *Pp2Context;
+  EFI_STATUS                     Status;
+
+  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);
+
+  Pp2Context = INSTANCE_FROM_AIP (This);
+
+  Status = Pp2Context->Snp.GetStatus (&(Pp2Context->Snp), NULL, NULL);
+  if (Status == EFI_NOT_READY){
+    AdapterInfo->MediaState = EFI_NOT_READY;
+    return EFI_SUCCESS;
+  }
+
+  AdapterInfo->MediaState = (Pp2Context->Snp.Mode->MediaPresent ?
+                             EFI_SUCCESS : EFI_NOT_READY);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Sets state information for an adapter.
+
+  This function sends information of type InformationType for an adapter.
+  If an adapter does not support the requested information type, then EFI_UNSUPPORTED
+  is returned.
+
+  @param[in]  This                   A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
+  @param[in]  InformationType        A pointer to an EFI_GUID that defines the contents of InformationBlock.
+  @param[in]  InforamtionBlock       A pointer to the InformationBlock structure which contains details
+                                     about the data specific to InformationType.
+  @param[in]  InforamtionBlockSize   The size of the InformationBlock in bytes.
+
+  @retval EFI_SUCCESS                The information was received and interpreted successfully.
+  @retval EFI_UNSUPPORTED            The InformationType is not known.
+  @retval EFI_DEVICE_ERROR           The device reported an error.
+  @retval EFI_INVALID_PARAMETER      This is NULL.
+  @retval EFI_INVALID_PARAMETER      InformationBlock is NULL.
+  @retval EFI_WRITE_PROTECTED        The InformationType cannot be modified using EFI_ADAPTER_INFO_SET_INFO().
+
+**/
+EFI_STATUS
+EFIAPI
+Pp2AipSetInformation (
+  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;
+}
+
+/**
+  Get a list of supported information types for this instance of the protocol.
+
+  This function returns a list of InformationType GUIDs that are supported on an
+  adapter with this instance of EFI_ADAPTER_INFORMATION_PROTOCOL. The list is returned
+  in InfoTypesBuffer, and the number of GUID pointers in InfoTypesBuffer is returned in
+  InfoTypesBufferCount.
+
+  @param[in]  This                  A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
+  @param[out] InfoTypesBuffer       A pointer to the array of InformationType GUIDs that are supported
+                                    by This.
+  @param[out] InfoTypesBufferCount  A pointer to the number of GUIDs present in InfoTypesBuffer.
+
+  @retval EFI_SUCCESS               The list of information type GUIDs that are supported on this adapter was
+                                    returned in InfoTypesBuffer. The number of information type GUIDs was
+                                    returned in InfoTypesBufferCount.
+  @retval EFI_INVALID_PARAMETER     This is NULL.
+  @retval EFI_INVALID_PARAMETER     InfoTypesBuffer is NULL.
+  @retval EFI_INVALID_PARAMETER     InfoTypesBufferCount is NULL.
+  @retval EFI_OUT_OF_RESOURCES      There is not enough pool memory to store the results.
+
+**/
+EFI_STATUS
+EFIAPI
+Pp2AipGetSupportedTypes (
+  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
+  OUT EFI_GUID                          **InfoTypesBuffer,
+  OUT UINTN                             *InfoTypesBufferCount
+  )
+{
+  if (This == NULL || InfoTypesBuffer == NULL || InfoTypesBufferCount == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  *InfoTypesBuffer = AllocateZeroPool (sizeof (EFI_GUID));
+  if (*InfoTypesBuffer == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  *InfoTypesBufferCount = 1;
+  (*InfoTypesBuffer)[0] = gEfiAdapterInfoMediaStateGuid;
+
+  return EFI_SUCCESS;
+}
+
 STATIC
 VOID
 Pp2DxeParsePortPcd (
@@ -1290,6 +1442,11 @@ Pp2DxeInitialiseController (
     Pp2Context->Instance = DeviceInstance;
     DeviceInstance++;
 
+    /* Prepare AIP Protocol */
+    Pp2Context->Aip.GetInformation    = Pp2AipGetInformation;
+    Pp2Context->Aip.SetInformation    = Pp2AipSetInformation;
+    Pp2Context->Aip.GetSupportedTypes = Pp2AipGetSupportedTypes;
+
     /* Install SNP protocol */
     Status = Pp2DxeSnpInstall(Pp2Context);
     if (EFI_ERROR(Status)) {
-- 
2.7.4

Re: [edk2-platforms: PATCH 1/4] Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol

[Ard Biesheuvel]

On Fri, 3 May 2019 at 01:50, Marcin Wojtas <mw@semihalf.com> wrote:
>
> From: Tomasz Michalec <tm@semihalf.com>
>
> Add implementation of Adapter Information Protocol in Armada 7k8k
> PP2 NIC driver. Support retrieving information about media state
> which allows to use NetLibDetectMediaWaitTimeout on network interface.
> This patch fixes possible hangs when attempting to PXE boot on
> unconnected interfaces.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Tomasz Michalec <tm@semihalf.com>

Please put your own signoff here. You can retain the one from Tomasz
if you like.

> ---
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf |   1 +
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h   |   3 +
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c   | 157 ++++++++++++++++++++
>  3 files changed, 161 insertions(+)
>
> diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
> index be536ab..73aa413 100644
> --- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
> +++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
> @@ -64,6 +64,7 @@
>    CacheMaintenanceLib
>
>  [Protocols]
> +  gEfiAdapterInformationProtocolGuid
>    gEfiSimpleNetworkProtocolGuid
>    gEfiDevicePathProtocolGuid
>    gEfiCpuArchProtocolGuid
> diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
> index b8a5dae..66bd46a 100644
> --- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
> +++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
> @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #ifndef __PP2_DXE_H__
>  #define __PP2_DXE_H__
>
> +#include <Protocol/AdapterInformation.h>
>  #include <Protocol/Cpu.h>
>  #include <Protocol/DevicePath.h>
>  #include <Protocol/DriverBinding.h>
> @@ -59,6 +60,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #define MVPP2_MAX_PORT  3
>
>  #define PP2DXE_SIGNATURE                    SIGNATURE_32('P', 'P', '2', 'D')
> +#define INSTANCE_FROM_AIP(a)                CR((a), PP2DXE_CONTEXT, Aip, PP2DXE_SIGNATURE)
>  #define INSTANCE_FROM_SNP(a)                CR((a), PP2DXE_CONTEXT, Snp, PP2DXE_SIGNATURE)
>
>  /* OS API */
> @@ -365,6 +367,7 @@ typedef struct {
>    UINTN                       CompletionQueueTail;
>    EFI_EVENT                   EfiExitBootServicesEvent;
>    PP2_DEVICE_PATH             *DevicePath;
> +  EFI_ADAPTER_INFORMATION_PROTOCOL Aip;
>  } PP2DXE_CONTEXT;
>
>  /* Inline helpers */
> diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> index 02b2798..473a2a1 100644
> --- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> +++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> @@ -1129,6 +1129,7 @@ Pp2DxeSnpInstall (
>        &Handle,
>        &gEfiSimpleNetworkProtocolGuid, &Pp2Context->Snp,
>        &gEfiDevicePathProtocolGuid, Pp2DevicePath,
> +      &gEfiAdapterInformationProtocolGuid, &Pp2Context->Aip,
>        NULL
>        );
>
> @@ -1139,6 +1140,157 @@ Pp2DxeSnpInstall (
>    return Status;
>  }
>
> +/**
> +  Returns the current state information for the adapter.
> +
> +  This function returns information of type InformationType from the adapter.
> +  If an adapter does not support the requested informational type, then
> +  EFI_UNSUPPORTED is returned.
> +
> +  @param[in]  This                   A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
> +  @param[in]  InformationType        A pointer to an EFI_GUID that defines the contents of InformationBlock.
> +  @param[out] InforamtionBlock       The service returns a pointer to the buffer with the InformationBlock
> +                                     structure which contains details about the data specific to InformationType.
> +  @param[out] InforamtionBlockSize   The driver returns the size of the InformationBlock in bytes.

Please fix these typos. I know they were copy/pasted from the protocol
definition, but I'd still prefer them to be fixed.

> +
> +  @retval EFI_SUCCESS                The InformationType information was retrieved.
> +  @retval EFI_UNSUPPORTED            The InformationType is not known.
> +  @retval EFI_DEVICE_ERROR           The device reported an error.
> +  @retval EFI_OUT_OF_RESOURCES       The request could not be completed due to a lack of resources.
> +  @retval EFI_INVALID_PARAMETER      This is NULL.
> +  @retval EFI_INVALID_PARAMETER      InformationBlock is NULL.
> +  @retval EFI_INVALID_PARAMETER      InformationBlockSize is NULL.
> +
> +**/

STATIC?

> +EFI_STATUS
> +EFIAPI
> +Pp2AipGetInformation (
> +  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
> +  IN  EFI_GUID                          *InformationType,
> +  OUT VOID                              **InformationBlock,
> +  OUT UINTN                             *InformationBlockSize
> +  )
> +{
> +  EFI_ADAPTER_INFO_MEDIA_STATE  *AdapterInfo;
> +  PP2DXE_CONTEXT                *Pp2Context;
> +  EFI_STATUS                     Status;
> +
> +  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);
> +
> +  Pp2Context = INSTANCE_FROM_AIP (This);
> +
> +  Status = Pp2Context->Snp.GetStatus (&(Pp2Context->Snp), NULL, NULL);
> +  if (Status == EFI_NOT_READY){
> +    AdapterInfo->MediaState = EFI_NOT_READY;
> +    return EFI_SUCCESS;
> +  }

What happens if GetStatus returns something other than EFI_SUCCESS or
EFI_NOT_READY?

> +
> +  AdapterInfo->MediaState = (Pp2Context->Snp.Mode->MediaPresent ?
> +                             EFI_SUCCESS : EFI_NOT_READY);
> +

Please get rid of the ternary expression, just fold the condition into
the preceding if()


> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Sets state information for an adapter.
> +
> +  This function sends information of type InformationType for an adapter.
> +  If an adapter does not support the requested information type, then EFI_UNSUPPORTED
> +  is returned.
> +
> +  @param[in]  This                   A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
> +  @param[in]  InformationType        A pointer to an EFI_GUID that defines the contents of InformationBlock.
> +  @param[in]  InforamtionBlock       A pointer to the InformationBlock structure which contains details
> +                                     about the data specific to InformationType.
> +  @param[in]  InforamtionBlockSize   The size of the InformationBlock in bytes.
> +

More typos

> +  @retval EFI_SUCCESS                The information was received and interpreted successfully.
> +  @retval EFI_UNSUPPORTED            The InformationType is not known.
> +  @retval EFI_DEVICE_ERROR           The device reported an error.
> +  @retval EFI_INVALID_PARAMETER      This is NULL.
> +  @retval EFI_INVALID_PARAMETER      InformationBlock is NULL.
> +  @retval EFI_WRITE_PROTECTED        The InformationType cannot be modified using EFI_ADAPTER_INFO_SET_INFO().
> +
> +**/

STATIC?

> +EFI_STATUS
> +EFIAPI
> +Pp2AipSetInformation (
> +  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;
> +}
> +
> +/**
> +  Get a list of supported information types for this instance of the protocol.
> +
> +  This function returns a list of InformationType GUIDs that are supported on an
> +  adapter with this instance of EFI_ADAPTER_INFORMATION_PROTOCOL. The list is returned
> +  in InfoTypesBuffer, and the number of GUID pointers in InfoTypesBuffer is returned in
> +  InfoTypesBufferCount.
> +
> +  @param[in]  This                  A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
> +  @param[out] InfoTypesBuffer       A pointer to the array of InformationType GUIDs that are supported
> +                                    by This.
> +  @param[out] InfoTypesBufferCount  A pointer to the number of GUIDs present in InfoTypesBuffer.
> +
> +  @retval EFI_SUCCESS               The list of information type GUIDs that are supported on this adapter was
> +                                    returned in InfoTypesBuffer. The number of information type GUIDs was
> +                                    returned in InfoTypesBufferCount.
> +  @retval EFI_INVALID_PARAMETER     This is NULL.
> +  @retval EFI_INVALID_PARAMETER     InfoTypesBuffer is NULL.
> +  @retval EFI_INVALID_PARAMETER     InfoTypesBufferCount is NULL.
> +  @retval EFI_OUT_OF_RESOURCES      There is not enough pool memory to store the results.
> +
> +**/

STATIC

> +EFI_STATUS
> +EFIAPI
> +Pp2AipGetSupportedTypes (
> +  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
> +  OUT EFI_GUID                          **InfoTypesBuffer,
> +  OUT UINTN                             *InfoTypesBufferCount
> +  )
> +{
> +  if (This == NULL || InfoTypesBuffer == NULL || InfoTypesBufferCount == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *InfoTypesBuffer = AllocateZeroPool (sizeof (EFI_GUID));

No need to zero if you assign the whole thing immediately

> +  if (*InfoTypesBuffer == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  *InfoTypesBufferCount = 1;
> +  (*InfoTypesBuffer)[0] = gEfiAdapterInfoMediaStateGuid;
> +

Use CopyGuid() not struct assignment

> +  return EFI_SUCCESS;
> +}
> +
>  STATIC
>  VOID
>  Pp2DxeParsePortPcd (
> @@ -1290,6 +1442,11 @@ Pp2DxeInitialiseController (
>      Pp2Context->Instance = DeviceInstance;
>      DeviceInstance++;
>
> +    /* Prepare AIP Protocol */
> +    Pp2Context->Aip.GetInformation    = Pp2AipGetInformation;
> +    Pp2Context->Aip.SetInformation    = Pp2AipSetInformation;
> +    Pp2Context->Aip.GetSupportedTypes = Pp2AipGetSupportedTypes;
> +
>      /* Install SNP protocol */
>      Status = Pp2DxeSnpInstall(Pp2Context);
>      if (EFI_ERROR(Status)) {
> --
> 2.7.4
>

Re: [edk2-platforms: PATCH 1/4] Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol

[Marcin Wojtas]

Hi Ard,

pt., 3 maj 2019 o 08:33 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> On Fri, 3 May 2019 at 01:50, Marcin Wojtas <mw@semihalf.com> wrote:
> >
> > From: Tomasz Michalec <tm@semihalf.com>
> >
> > Add implementation of Adapter Information Protocol in Armada 7k8k
> > PP2 NIC driver. Support retrieving information about media state
> > which allows to use NetLibDetectMediaWaitTimeout on network interface.
> > This patch fixes possible hangs when attempting to PXE boot on
> > unconnected interfaces.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Tomasz Michalec <tm@semihalf.com>
>
> Please put your own signoff here. You can retain the one from Tomasz
> if you like.
>
> > ---
> >  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf |   1 +
> >  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h   |   3 +
> >  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c   | 157 ++++++++++++++++++++
> >  3 files changed, 161 insertions(+)
> >
> > diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
> > index be536ab..73aa413 100644
> > --- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
> > +++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
> > @@ -64,6 +64,7 @@
> >    CacheMaintenanceLib
> >
> >  [Protocols]
> > +  gEfiAdapterInformationProtocolGuid
> >    gEfiSimpleNetworkProtocolGuid
> >    gEfiDevicePathProtocolGuid
> >    gEfiCpuArchProtocolGuid
> > diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
> > index b8a5dae..66bd46a 100644
> > --- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
> > +++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
> > @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >  #ifndef __PP2_DXE_H__
> >  #define __PP2_DXE_H__
> >
> > +#include <Protocol/AdapterInformation.h>
> >  #include <Protocol/Cpu.h>
> >  #include <Protocol/DevicePath.h>
> >  #include <Protocol/DriverBinding.h>
> > @@ -59,6 +60,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >  #define MVPP2_MAX_PORT  3
> >
> >  #define PP2DXE_SIGNATURE                    SIGNATURE_32('P', 'P', '2', 'D')
> > +#define INSTANCE_FROM_AIP(a)                CR((a), PP2DXE_CONTEXT, Aip, PP2DXE_SIGNATURE)
> >  #define INSTANCE_FROM_SNP(a)                CR((a), PP2DXE_CONTEXT, Snp, PP2DXE_SIGNATURE)
> >
> >  /* OS API */
> > @@ -365,6 +367,7 @@ typedef struct {
> >    UINTN                       CompletionQueueTail;
> >    EFI_EVENT                   EfiExitBootServicesEvent;
> >    PP2_DEVICE_PATH             *DevicePath;
> > +  EFI_ADAPTER_INFORMATION_PROTOCOL Aip;
> >  } PP2DXE_CONTEXT;
> >
> >  /* Inline helpers */
> > diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> > index 02b2798..473a2a1 100644
> > --- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> > +++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> > @@ -1129,6 +1129,7 @@ Pp2DxeSnpInstall (
> >        &Handle,
> >        &gEfiSimpleNetworkProtocolGuid, &Pp2Context->Snp,
> >        &gEfiDevicePathProtocolGuid, Pp2DevicePath,
> > +      &gEfiAdapterInformationProtocolGuid, &Pp2Context->Aip,
> >        NULL
> >        );
> >
> > @@ -1139,6 +1140,157 @@ Pp2DxeSnpInstall (
> >    return Status;
> >  }
> >
> > +/**
> > +  Returns the current state information for the adapter.
> > +
> > +  This function returns information of type InformationType from the adapter.
> > +  If an adapter does not support the requested informational type, then
> > +  EFI_UNSUPPORTED is returned.
> > +
> > +  @param[in]  This                   A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
> > +  @param[in]  InformationType        A pointer to an EFI_GUID that defines the contents of InformationBlock.
> > +  @param[out] InforamtionBlock       The service returns a pointer to the buffer with the InformationBlock
> > +                                     structure which contains details about the data specific to InformationType.
> > +  @param[out] InforamtionBlockSize   The driver returns the size of the InformationBlock in bytes.
>
> Please fix these typos. I know they were copy/pasted from the protocol
> definition, but I'd still prefer them to be fixed.
>
> > +
> > +  @retval EFI_SUCCESS                The InformationType information was retrieved.
> > +  @retval EFI_UNSUPPORTED            The InformationType is not known.
> > +  @retval EFI_DEVICE_ERROR           The device reported an error.
> > +  @retval EFI_OUT_OF_RESOURCES       The request could not be completed due to a lack of resources.
> > +  @retval EFI_INVALID_PARAMETER      This is NULL.
> > +  @retval EFI_INVALID_PARAMETER      InformationBlock is NULL.
> > +  @retval EFI_INVALID_PARAMETER      InformationBlockSize is NULL.
> > +
> > +**/
>
> STATIC?
>
> > +EFI_STATUS
> > +EFIAPI
> > +Pp2AipGetInformation (
> > +  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
> > +  IN  EFI_GUID                          *InformationType,
> > +  OUT VOID                              **InformationBlock,
> > +  OUT UINTN                             *InformationBlockSize
> > +  )
> > +{
> > +  EFI_ADAPTER_INFO_MEDIA_STATE  *AdapterInfo;
> > +  PP2DXE_CONTEXT                *Pp2Context;
> > +  EFI_STATUS                     Status;
> > +
> > +  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);
> > +
> > +  Pp2Context = INSTANCE_FROM_AIP (This);
> > +
> > +  Status = Pp2Context->Snp.GetStatus (&(Pp2Context->Snp), NULL, NULL);
> > +  if (Status == EFI_NOT_READY){
> > +    AdapterInfo->MediaState = EFI_NOT_READY;
> > +    return EFI_SUCCESS;
> > +  }
>
> What happens if GetStatus returns something other than EFI_SUCCESS or
> EFI_NOT_READY?
>

In Pp2Dxe those are the only 2 possible return values, but despite
that I'll add handling for:
} else if (EFI_ERROR(Status) {

> > +
> > +  AdapterInfo->MediaState = (Pp2Context->Snp.Mode->MediaPresent ?
> > +                             EFI_SUCCESS : EFI_NOT_READY);
> > +
>
> Please get rid of the ternary expression, just fold the condition into
> the preceding if()
>
>
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Sets state information for an adapter.
> > +
> > +  This function sends information of type InformationType for an adapter.
> > +  If an adapter does not support the requested information type, then EFI_UNSUPPORTED
> > +  is returned.
> > +
> > +  @param[in]  This                   A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
> > +  @param[in]  InformationType        A pointer to an EFI_GUID that defines the contents of InformationBlock.
> > +  @param[in]  InforamtionBlock       A pointer to the InformationBlock structure which contains details
> > +                                     about the data specific to InformationType.
> > +  @param[in]  InforamtionBlockSize   The size of the InformationBlock in bytes.
> > +
>
> More typos
>
> > +  @retval EFI_SUCCESS                The information was received and interpreted successfully.
> > +  @retval EFI_UNSUPPORTED            The InformationType is not known.
> > +  @retval EFI_DEVICE_ERROR           The device reported an error.
> > +  @retval EFI_INVALID_PARAMETER      This is NULL.
> > +  @retval EFI_INVALID_PARAMETER      InformationBlock is NULL.
> > +  @retval EFI_WRITE_PROTECTED        The InformationType cannot be modified using EFI_ADAPTER_INFO_SET_INFO().
> > +
> > +**/
>
> STATIC?
>
> > +EFI_STATUS
> > +EFIAPI
> > +Pp2AipSetInformation (
> > +  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;
> > +}
> > +
> > +/**
> > +  Get a list of supported information types for this instance of the protocol.
> > +
> > +  This function returns a list of InformationType GUIDs that are supported on an
> > +  adapter with this instance of EFI_ADAPTER_INFORMATION_PROTOCOL. The list is returned
> > +  in InfoTypesBuffer, and the number of GUID pointers in InfoTypesBuffer is returned in
> > +  InfoTypesBufferCount.
> > +
> > +  @param[in]  This                  A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
> > +  @param[out] InfoTypesBuffer       A pointer to the array of InformationType GUIDs that are supported
> > +                                    by This.
> > +  @param[out] InfoTypesBufferCount  A pointer to the number of GUIDs present in InfoTypesBuffer.
> > +
> > +  @retval EFI_SUCCESS               The list of information type GUIDs that are supported on this adapter was
> > +                                    returned in InfoTypesBuffer. The number of information type GUIDs was
> > +                                    returned in InfoTypesBufferCount.
> > +  @retval EFI_INVALID_PARAMETER     This is NULL.
> > +  @retval EFI_INVALID_PARAMETER     InfoTypesBuffer is NULL.
> > +  @retval EFI_INVALID_PARAMETER     InfoTypesBufferCount is NULL.
> > +  @retval EFI_OUT_OF_RESOURCES      There is not enough pool memory to store the results.
> > +
> > +**/
>
> STATIC
>
> > +EFI_STATUS
> > +EFIAPI
> > +Pp2AipGetSupportedTypes (
> > +  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
> > +  OUT EFI_GUID                          **InfoTypesBuffer,
> > +  OUT UINTN                             *InfoTypesBufferCount
> > +  )
> > +{
> > +  if (This == NULL || InfoTypesBuffer == NULL || InfoTypesBufferCount == NULL) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  *InfoTypesBuffer = AllocateZeroPool (sizeof (EFI_GUID));
>
> No need to zero if you assign the whole thing immediately
>
> > +  if (*InfoTypesBuffer == NULL) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  *InfoTypesBufferCount = 1;
> > +  (*InfoTypesBuffer)[0] = gEfiAdapterInfoMediaStateGuid;
> > +
>
> Use CopyGuid() not struct assignment
>
> > +  return EFI_SUCCESS;
> > +}
> > +
> >  STATIC
> >  VOID
> >  Pp2DxeParsePortPcd (
> > @@ -1290,6 +1442,11 @@ Pp2DxeInitialiseController (
> >      Pp2Context->Instance = DeviceInstance;
> >      DeviceInstance++;
> >
> > +    /* Prepare AIP Protocol */
> > +    Pp2Context->Aip.GetInformation    = Pp2AipGetInformation;
> > +    Pp2Context->Aip.SetInformation    = Pp2AipSetInformation;
> > +    Pp2Context->Aip.GetSupportedTypes = Pp2AipGetSupportedTypes;
> > +
> >      /* Install SNP protocol */
> >      Status = Pp2DxeSnpInstall(Pp2Context);
> >      if (EFI_ERROR(Status)) {
> > --
> > 2.7.4
> >

[edk2-platforms: PATCH 2/4] Marvell/Drivers: MvPhyDxe: Improve 88E1510 initialization

[Marcin Wojtas]

This patch adds only a non-functional change, extracting common
startup autonegotiation configuration into a separate routine.
It will be re-used in 88E1112 PHY support addition in a following
patch.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c | 86 +++++++++++++-------
 1 file changed, 57 insertions(+), 29 deletions(-)

diff --git a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
index 9be0489..780e8bd 100644
--- a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
+++ b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
@@ -251,6 +251,58 @@ MvPhyParseStatus (
   return EFI_SUCCESS;
 }
 
+/**
+  Configure PHY device autonegotiation.
+
+  @param[in out]   *PhyDevice      A pointer to configured PHY device structure.
+
+**/
+STATIC
+EFI_STATUS
+MvPhyConfigureAutonegotiation (
+  IN OUT PHY_DEVICE *PhyDevice
+  )
+{
+  UINT32 Data;
+  INTN Index;
+
+  /* Read BMSR register in order to check autoneg capabilities and status. */
+  Mdio->Read (Mdio, PhyDevice->Addr, PhyDevice->MdioIndex, MII_BMSR, &Data);
+
+  if ((Data & BMSR_ANEGCAPABLE) && !(Data & BMSR_ANEGCOMPLETE)) {
+
+    DEBUG ((DEBUG_INFO,
+      "%a: Waiting for PHY auto negotiation...",
+      __FUNCTION__));
+
+    /* Wait for autonegotiation to complete and read media status */
+    for (Index = 0; !(Data & BMSR_ANEGCOMPLETE); Index++) {
+      if (Index > PHY_AUTONEGOTIATE_TIMEOUT) {
+        DEBUG ((DEBUG_ERROR, "%a: Timeout\n", __FUNCTION__));
+        PhyDevice->LinkUp = FALSE;
+        return EFI_TIMEOUT;
+      }
+      gBS->Stall(1000);  /* 1 ms */
+      Mdio->Read (Mdio, PhyDevice->Addr, PhyDevice->MdioIndex, MII_BMSR, &Data);
+    }
+
+    PhyDevice->LinkUp = TRUE;
+    DEBUG ((DEBUG_INFO, "%a: link up\n", __FUNCTION__));
+  } else {
+    Mdio->Read (Mdio, PhyDevice->Addr, PhyDevice->MdioIndex, MII_BMSR, &Data);
+
+    if (Data & BMSR_LSTATUS) {
+      PhyDevice->LinkUp = TRUE;
+      DEBUG ((DEBUG_INFO, "%a: link up\n", __FUNCTION__));
+    } else {
+      PhyDevice->LinkUp = FALSE;
+      DEBUG ((DEBUG_INFO, "%a: link down\n", __FUNCTION__));
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
 STATIC
 VOID
 MvPhy1512WriteBits (
@@ -282,8 +334,7 @@ MvPhyInit1512 (
     IN OUT PHY_DEVICE *PhyDev
     )
 {
-  UINT32 Data;
-  INTN i;
+  EFI_STATUS Status;
 
   if (PhyDev->Connection == PHY_CONNECTION_SGMII) {
     /* Select page 0xff and update configuration registers according to
@@ -321,34 +372,11 @@ MvPhyInit1512 (
   if (!PcdGetBool (PcdPhyStartupAutoneg))
     return EFI_SUCCESS;
 
-  Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
-
-  if ((Data & BMSR_ANEGCAPABLE) && !(Data & BMSR_ANEGCOMPLETE)) {
-
-    DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
-    for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
-      if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
-        DEBUG((DEBUG_ERROR, "timeout\n"));
-        PhyDev->LinkUp = FALSE;
-        return EFI_TIMEOUT;
-      }
-
-      gBS->Stall(1000);  /* 1 ms */
-      Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
-    }
-    PhyDev->LinkUp = TRUE;
-    DEBUG((DEBUG_INFO, "MvPhyDxe: link up\n"));
-  } else {
-    Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
-
-    if (Data & BMSR_LSTATUS) {
-      PhyDev->LinkUp = TRUE;
-      DEBUG((DEBUG_INFO, "MvPhyDxe: link up\n"));
-    } else {
-      PhyDev->LinkUp = FALSE;
-      DEBUG((DEBUG_INFO, "MvPhyDxe: link down\n"));
-    }
+  Status = MvPhyConfigureAutonegotiation (PhyDev);
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
+
   MvPhyParseStatus (PhyDev);
 
   return EFI_SUCCESS;
-- 
2.7.4

Re: [edk2-platforms: PATCH 2/4] Marvell/Drivers: MvPhyDxe: Improve 88E1510 initialization

[Ard Biesheuvel]

On Fri, 3 May 2019 at 01:50, Marcin Wojtas <mw@semihalf.com> wrote:
>
> This patch adds only a non-functional change, extracting common
> startup autonegotiation configuration into a separate routine.
> It will be re-used in 88E1112 PHY support addition in a following
> patch.
>

In that case, call it 'refactor' not 'improve'

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c | 86 +++++++++++++-------
>  1 file changed, 57 insertions(+), 29 deletions(-)
>
> diff --git a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
> index 9be0489..780e8bd 100644
> --- a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
> +++ b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
> @@ -251,6 +251,58 @@ MvPhyParseStatus (
>    return EFI_SUCCESS;
>  }
>
> +/**
> +  Configure PHY device autonegotiation.
> +
> +  @param[in out]   *PhyDevice      A pointer to configured PHY device structure.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +MvPhyConfigureAutonegotiation (
> +  IN OUT PHY_DEVICE *PhyDevice
> +  )
> +{
> +  UINT32 Data;
> +  INTN Index;
> +
> +  /* Read BMSR register in order to check autoneg capabilities and status. */
> +  Mdio->Read (Mdio, PhyDevice->Addr, PhyDevice->MdioIndex, MII_BMSR, &Data);
> +
> +  if ((Data & BMSR_ANEGCAPABLE) && !(Data & BMSR_ANEGCOMPLETE)) {
> +
> +    DEBUG ((DEBUG_INFO,
> +      "%a: Waiting for PHY auto negotiation...",
> +      __FUNCTION__));
> +
> +    /* Wait for autonegotiation to complete and read media status */
> +    for (Index = 0; !(Data & BMSR_ANEGCOMPLETE); Index++) {
> +      if (Index > PHY_AUTONEGOTIATE_TIMEOUT) {
> +        DEBUG ((DEBUG_ERROR, "%a: Timeout\n", __FUNCTION__));
> +        PhyDevice->LinkUp = FALSE;
> +        return EFI_TIMEOUT;
> +      }
> +      gBS->Stall(1000);  /* 1 ms */

Space before (

> +      Mdio->Read (Mdio, PhyDevice->Addr, PhyDevice->MdioIndex, MII_BMSR, &Data);
> +    }
> +
> +    PhyDevice->LinkUp = TRUE;
> +    DEBUG ((DEBUG_INFO, "%a: link up\n", __FUNCTION__));
> +  } else {
> +    Mdio->Read (Mdio, PhyDevice->Addr, PhyDevice->MdioIndex, MII_BMSR, &Data);
> +
> +    if (Data & BMSR_LSTATUS) {
> +      PhyDevice->LinkUp = TRUE;
> +      DEBUG ((DEBUG_INFO, "%a: link up\n", __FUNCTION__));
> +    } else {
> +      PhyDevice->LinkUp = FALSE;
> +      DEBUG ((DEBUG_INFO, "%a: link down\n", __FUNCTION__));
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  STATIC
>  VOID
>  MvPhy1512WriteBits (
> @@ -282,8 +334,7 @@ MvPhyInit1512 (
>      IN OUT PHY_DEVICE *PhyDev
>      )
>  {
> -  UINT32 Data;
> -  INTN i;
> +  EFI_STATUS Status;
>
>    if (PhyDev->Connection == PHY_CONNECTION_SGMII) {
>      /* Select page 0xff and update configuration registers according to
> @@ -321,34 +372,11 @@ MvPhyInit1512 (
>    if (!PcdGetBool (PcdPhyStartupAutoneg))
>      return EFI_SUCCESS;
>
> -  Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
> -
> -  if ((Data & BMSR_ANEGCAPABLE) && !(Data & BMSR_ANEGCOMPLETE)) {
> -
> -    DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
> -    for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
> -      if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
> -        DEBUG((DEBUG_ERROR, "timeout\n"));
> -        PhyDev->LinkUp = FALSE;
> -        return EFI_TIMEOUT;
> -      }
> -
> -      gBS->Stall(1000);  /* 1 ms */
> -      Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
> -    }
> -    PhyDev->LinkUp = TRUE;
> -    DEBUG((DEBUG_INFO, "MvPhyDxe: link up\n"));
> -  } else {
> -    Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
> -
> -    if (Data & BMSR_LSTATUS) {
> -      PhyDev->LinkUp = TRUE;
> -      DEBUG((DEBUG_INFO, "MvPhyDxe: link up\n"));
> -    } else {
> -      PhyDev->LinkUp = FALSE;
> -      DEBUG((DEBUG_INFO, "MvPhyDxe: link down\n"));
> -    }
> +  Status = MvPhyConfigureAutonegotiation (PhyDev);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
>    }
> +
>    MvPhyParseStatus (PhyDev);
>
>    return EFI_SUCCESS;
> --
> 2.7.4
>

[edk2-platforms: PATCH 3/4] Marvell/Drivers: MvPhyDxe: Introduce 88E1112 initialization

[Marcin Wojtas]

This patch adds 88E1112 PHY support and updates PortingGuide
accordingly.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.h | 17 ++++++++++-
 Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c | 31 ++++++++++++++++++++
 Silicon/Marvell/Documentation/PortingGuide.txt  |  7 +++--
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.h b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.h
index 66974bb..cd5a475 100644
--- a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.h
+++ b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.h
@@ -75,7 +75,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #define MIIM_88E1111_HWCFG_FIBER_COPPER_RES   0x2000
 
 typedef enum {
-  MV_PHY_DEVICE_1512
+  MV_PHY_DEVICE_1512,
+  MV_PHY_DEVICE_1112
 } MV_PHY_DEVICE_ID;
 
 typedef
@@ -97,4 +98,18 @@ MvPhyInit1512 (
     IN OUT PHY_DEVICE *PhyDev
     );
 
+/**
+  Initialize Marvell 88E1112 PHY.
+
+  @param[in]      MvPhyProtocol   Marvell PHY protocol instance.
+  @param[in out] *PhyDevice       PHY device structure.
+
+**/
+STATIC
+EFI_STATUS
+MvPhyInit1112 (
+  IN CONST MARVELL_PHY_PROTOCOL  *MvPhyProtocol,
+  IN OUT PHY_DEVICE              *PhyDevice
+  );
+
 #endif
diff --git a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
index 780e8bd..08a8ed0 100644
--- a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
+++ b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
@@ -66,6 +66,7 @@ STATIC UINT8 * CONST PhySmiAddresses = PcdGetPtr (PcdPhySmiAddresses);
 
 STATIC MV_PHY_DEVICE MvPhyDevices[] = {
   { MV_PHY_DEVICE_1512, MvPhyInit1512 },
+  { MV_PHY_DEVICE_1112, MvPhyInit1112 },
   { 0, NULL }
 };
 
@@ -382,6 +383,36 @@ MvPhyInit1512 (
   return EFI_SUCCESS;
 }
 
+/**
+  Initialize Marvell 88E1112 PHY.
+
+  @param[in]      MvPhyProtocol   Marvell PHY protocol instance.
+  @param[in out] *PhyDevice       PHY device structure.
+
+**/
+STATIC
+EFI_STATUS
+MvPhyInit1112 (
+  IN CONST MARVELL_PHY_PROTOCOL  *MvPhyProtocol,
+  IN OUT PHY_DEVICE              *PhyDevice
+  )
+{
+  EFI_STATUS Status;
+
+  MvPhyM88e1111sConfig (PhyDevice);
+
+  if (PcdGetBool (PcdPhyStartupAutoneg)) {
+    Status = MvPhyConfigureAutonegotiation (PhyDevice);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+
+    MvPhyParseStatus (PhyDevice);
+  }
+
+  return EFI_SUCCESS;
+}
+
 EFI_STATUS
 MvPhyInit (
   IN CONST MARVELL_PHY_PROTOCOL *Snp,
diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
index 2603980..9dee5c8 100644
--- a/Silicon/Marvell/Documentation/PortingGuide.txt
+++ b/Silicon/Marvell/Documentation/PortingGuide.txt
@@ -146,7 +146,7 @@ Example
 PHY Driver configuration
 ========================
 MvPhyDxe provides basic initialization and status routines for Marvell PHYs.
-Currently only 1518 series PHYs are supported. Following PCDs are required:
+Currently only 1512 and 1112 series PHYs are supported. Following PCDs are required:
 
   - gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg
         (boolean - if true, driver waits for autonegotiation on startup)
@@ -162,6 +162,7 @@ MV_PHY_DEVICE_ID:
 
                 typedef enum {
                         0    MV_PHY_DEVICE_1512,
+                        1    MV_PHY_DEVICE_1112,
                 } MV_PHY_DEVICE_ID;
 
 It should be extended when adding support for other PHY models.
@@ -170,9 +171,9 @@ Disable autonegotiation:
 
  gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg|FALSE
 
-assuming, that PHY models are 1512:
+assuming, that PHY models are 1512 and 1112 for two consecutive ports:
 
- gMarvellTokenSpaceGuid.PcdPhyDeviceIds|{ 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPhyDeviceIds|{ 0x0, 0x1 }
 
 
 MDIO configuration
-- 
2.7.4

[edk2-platforms: PATCH 4/4] Marvell/Drivers: MvPhyDxe: Reset PHY only once

[Marcin Wojtas]

At the end of PHY low level configuration a soft reset
was performed twice. It is not necessary, so remove
redundant reset call.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
index 08a8ed0..c8ec6d3 100644
--- a/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
+++ b/Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c
@@ -180,8 +180,6 @@ MvPhyM88e1111sConfig (
   /* Soft reset */
   MvPhyReset (PhyDev);
 
-  MvPhyReset (PhyDev);
-
   return EFI_SUCCESS;
 }
 
-- 
2.7.4

Re: [edk2-platforms: PATCH 0/4] Armada 7k8k network improvements

[Ard Biesheuvel]

On Fri, 3 May 2019 at 01:50, Marcin Wojtas <mw@semihalf.com> wrote:
>
> Hi,
>
> This small patchset extends PP2 NIC driver with the basic
> AdapterInformationProtocol support and adds 88E1112 PHY support.
> Also a minor fix is included on top.
>
> The patches are available in the github:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/net-upstream-r20190503
>
> I'm looking forward to your comments or remarks.
>
> Best regards,
> Marcin
>
> Marcin Wojtas (3):
>   Marvell/Drivers: MvPhyDxe: Improve 88E1510 initialization
>   Marvell/Drivers: MvPhyDxe: Introduce 88E1112 initialization
>   Marvell/Drivers: MvPhyDxe: Reset PHY only once
>
> Tomasz Michalec (1):
>   Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol
>

Patches 1 and 2 need some work, 3 and 4 look fine.

After this series, I am not taking any more Armada7k8k patches until
we get the long promised PCIe support :-)

Re: [edk2-platforms: PATCH 0/4] Armada 7k8k network improvements

[Marcin Wojtas]

pt., 3 maj 2019 o 08:37 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> On Fri, 3 May 2019 at 01:50, Marcin Wojtas <mw@semihalf.com> wrote:
> >
> > Hi,
> >
> > This small patchset extends PP2 NIC driver with the basic
> > AdapterInformationProtocol support and adds 88E1112 PHY support.
> > Also a minor fix is included on top.
> >
> > The patches are available in the github:
> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/net-upstream-r20190503
> >
> > I'm looking forward to your comments or remarks.
> >
> > Best regards,
> > Marcin
> >
> > Marcin Wojtas (3):
> >   Marvell/Drivers: MvPhyDxe: Improve 88E1510 initialization
> >   Marvell/Drivers: MvPhyDxe: Introduce 88E1112 initialization
> >   Marvell/Drivers: MvPhyDxe: Reset PHY only once
> >
> > Tomasz Michalec (1):
> >   Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol
> >
>
> Patches 1 and 2 need some work, 3 and 4 look fine.
>
> After this series, I am not taking any more Armada7k8k patches until
> we get the long promised PCIe support :-)

Understood and agree :)

Thanks!
Marcin

Re: [edk2-platforms: PATCH 0/4] Armada 7k8k network improvements

[Leif Lindholm]

On Fri, May 03, 2019 at 01:50:12AM +0200, Marcin Wojtas wrote:
> Hi,
> 
> This small patchset extends PP2 NIC driver with the basic
> AdapterInformationProtocol support and adds 88E1112 PHY support.
> Also a minor fix is included on top.
> 
> The patches are available in the github:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/net-upstream-r20190503
> 
> I'm looking forward to your comments or remarks.

For v2, please add a comment to 0/4 on what user(or
developer)-observable effect this has on existing edk2-platforms
... platforms.

No comments (beyond Ard's) on the actual patches.

Best Regards,

Leif

> Best regards,
> Marcin
> 
> Marcin Wojtas (3):
>   Marvell/Drivers: MvPhyDxe: Improve 88E1510 initialization
>   Marvell/Drivers: MvPhyDxe: Introduce 88E1112 initialization
>   Marvell/Drivers: MvPhyDxe: Reset PHY only once
> 
> Tomasz Michalec (1):
>   Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol
> 
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf   |   1 +
>  Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.h |  17 ++-
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h     |   3 +
>  Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c | 111 ++++++++++----
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c     | 157 ++++++++++++++++++++
>  Silicon/Marvell/Documentation/PortingGuide.txt  |   7 +-
>  6 files changed, 265 insertions(+), 31 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [edk2-platforms: PATCH 0/4] Armada 7k8k network improvements

[Marcin Wojtas]

Hi Leif,

pt., 3 maj 2019 o 09:36 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Fri, May 03, 2019 at 01:50:12AM +0200, Marcin Wojtas wrote:
> > Hi,
> >
> > This small patchset extends PP2 NIC driver with the basic
> > AdapterInformationProtocol support and adds 88E1112 PHY support.
> > Also a minor fix is included on top.
> >
> > The patches are available in the github:
> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/net-upstream-r20190503
> >
> > I'm looking forward to your comments or remarks.
>
> For v2, please add a comment to 0/4 on what user(or
> developer)-observable effect this has on existing edk2-platforms
> ... platforms.
>

Not sure I get it - do you want me to describe a change between v1 and
v2 or explanation, why I submit these patches at all? Or something
else?

Best regards,
Marcin

> No comments (beyond Ard's) on the actual patches.
>
> Best Regards,
>
> Leif
>
> > Best regards,
> > Marcin
> >
> > Marcin Wojtas (3):
> >   Marvell/Drivers: MvPhyDxe: Improve 88E1510 initialization
> >   Marvell/Drivers: MvPhyDxe: Introduce 88E1112 initialization
> >   Marvell/Drivers: MvPhyDxe: Reset PHY only once
> >
> > Tomasz Michalec (1):
> >   Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol
> >
> >  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf   |   1 +
> >  Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.h |  17 ++-
> >  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h     |   3 +
> >  Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c | 111 ++++++++++----
> >  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c     | 157 ++++++++++++++++++++
> >  Silicon/Marvell/Documentation/PortingGuide.txt  |   7 +-
> >  6 files changed, 265 insertions(+), 31 deletions(-)
> >
> > --
> > 2.7.4
> >

Re: [edk2-platforms: PATCH 0/4] Armada 7k8k network improvements

[Leif Lindholm]

On Fri, May 03, 2019 at 09:51:41AM +0200, Marcin Wojtas wrote:
> Hi Leif,
> 
> pt., 3 maj 2019 o 09:36 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
> >
> > On Fri, May 03, 2019 at 01:50:12AM +0200, Marcin Wojtas wrote:
> > > Hi,
> > >
> > > This small patchset extends PP2 NIC driver with the basic
> > > AdapterInformationProtocol support and adds 88E1112 PHY support.
> > > Also a minor fix is included on top.
> > >
> > > The patches are available in the github:
> > > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/net-upstream-r20190503
> > >
> > > I'm looking forward to your comments or remarks.
> >
> > For v2, please add a comment to 0/4 on what user(or
> > developer)-observable effect this has on existing edk2-platforms
> > ... platforms.
> >
> 
> Not sure I get it - do you want me to describe a change between v1 and
> v2 or explanation, why I submit these patches at all? Or something
> else?

The latter - why the patches were submitted at all.

Best Regards,

Leif

> 
> Best regards,
> Marcin
> 
> > No comments (beyond Ard's) on the actual patches.
> >
> > Best Regards,
> >
> > Leif
> >
> > > Best regards,
> > > Marcin
> > >
> > > Marcin Wojtas (3):
> > >   Marvell/Drivers: MvPhyDxe: Improve 88E1510 initialization
> > >   Marvell/Drivers: MvPhyDxe: Introduce 88E1112 initialization
> > >   Marvell/Drivers: MvPhyDxe: Reset PHY only once
> > >
> > > Tomasz Michalec (1):
> > >   Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol
> > >
> > >  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf   |   1 +
> > >  Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.h |  17 ++-
> > >  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h     |   3 +
> > >  Silicon/Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.c | 111 ++++++++++----
> > >  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c     | 157 ++++++++++++++++++++
> > >  Silicon/Marvell/Documentation/PortingGuide.txt  |   7 +-
> > >  6 files changed, 265 insertions(+), 31 deletions(-)
> > >
> > > --
> > > 2.7.4
> > >