Re: [Linaro-uefi] [PATCH] OpenPlatformPkg/Drivers/Net/MarvellYukonDxe: Added multi-controller support

On 10/18/2016 12:44 PM, Daniil Egranov wrote:

Hi Ard,

Thanks for the review. Please see my comments related to "m" prefix.

Thanks,

Daniil

On 10/18/2016 05:12 AM, Ard Biesheuvel wrote:

...

Hi Daniil,

Thanks for looking into this.

On 17 October 2016 at 22:45, Daniil Egranov daniil.egranov@arm.com wrote:

...

The patch adds support for multi-controller configuration.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com

---

Drivers/Net/MarvellYukonDxe/DriverBinding.c | 6 +- Drivers/Net/MarvellYukonDxe/Snp.c | 46 +-- Drivers/Net/MarvellYukonDxe/e1000phy.c | 330 +++++++++++---- Drivers/Net/MarvellYukonDxe/e1000phyreg.h | 23 ++ Drivers/Net/MarvellYukonDxe/if_msk.c | 615 +++++++++++++++++++++------- Drivers/Net/MarvellYukonDxe/if_msk.h | 18 +- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 17 + Drivers/Net/MarvellYukonDxe/miivar.h | 15 +- 8 files changed, 786 insertions(+), 284 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c index 95068fa..fc684c0 100644 --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c @@ -192,14 +192,14 @@ MarvellYukonDriverStart ( // // Initialize Yukon card so we can get the MAC address //

• Status = mskc_attach (YukonDriver->PciIo,

&YukonDriver->SnpMode.PermanentAddress);

• Status = mskc_attach (Controller, YukonDriver->PciIo,

&YukonDriver->SnpMode.PermanentAddress);

if (EFI_ERROR (Status)) {
  gBS->FreePool (YukonDriver);
  return Status;
}

• mskc_detach();

• mskc_detach (Controller);

  // // Assign fields for device path

@@ -487,6 +487,8 @@ InitializeMarvellYukonDriver (

if (EFI_ERROR (Status)) {
  DEBUG ((EFI_D_ERROR, "Marvell Yukon: 

InitializeMarvellYukonDriver(): Driver binding failed\n"));

• } else {

• mskc_init_driver_list (); }

  return Status;

Could we have something like this instead, please?

""" if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, "Marvell Yukon: InitializeMarvellYukonDriver(): Driver binding failed\n")); return Status; } mskc_init_driver_list (); return EFI_SUCCESS; """

...

diff --git a/Drivers/Net/MarvellYukonDxe/Snp.c b/Drivers/Net/MarvellYukonDxe/Snp.c index 3d84f84..96cec5f 100644 --- a/Drivers/Net/MarvellYukonDxe/Snp.c +++ b/Drivers/Net/MarvellYukonDxe/Snp.c @@ -121,21 +121,21 @@ SnpGetStatus ( ) { EFI_STATUS Status;

• YUKON_DRIVER *Snp;

• YUKON_DRIVER *YukonDriver;

Could you split off this rename? (and all the associated white space only changes involving 'YUKON_DRIVER')

...

EFI_TPL       OldTpl;

if (This == NULL) {
  return EFI_INVALID_PARAMETER;
}

• Snp = YUKON_DEV_FROM_THIS_SNP (This);
• if (Snp == NULL) {

• YukonDriver = YUKON_DEV_FROM_THIS_SNP (This);

• if (YukonDriver == NULL) { return EFI_INVALID_PARAMETER; }

  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);

• switch (Snp->SnpMode.State) {

• switch (YukonDriver->SnpMode.State) { case EfiSimpleNetworkInitialized: break;

@@ -148,7 +148,7 @@ SnpGetStatus ( goto ON_EXIT; }

• mskc_getstatus (InterruptStatus, TxBuf);

• mskc_getstatus (YukonDriver->Controller, InterruptStatus, TxBuf); Status = EFI_SUCCESS;

  ON_EXIT:

@@ -199,7 +199,7 @@ SnpInitialize ( ) { EFI_STATUS Status;

• YUKON_DRIVER *YukonDriver;

• YUKON_DRIVER *YukonDriver;

... like this one ...

...

EFI_TPL       OldTpl;

DEBUG ((EFI_D_NET, "Marvell Yukon: SnpInitialize()\n"));

@@ -230,7 +230,7 @@ SnpInitialize ( gBS->SetMem (YukonDriver->SnpMode.MCastFilter, sizeof YukonDriver->SnpMode.MCastFilter, 0); gBS->CopyMem (&YukonDriver->SnpMode.CurrentAddress, &YukonDriver->SnpMode.PermanentAddress, sizeof (EFI_MAC_ADDRESS));

• Status = mskc_init ();

• Status = mskc_init (YukonDriver->Controller);

  if (EFI_ERROR (Status)) { goto ON_ERROR_RESTORE_TPL;

@@ -509,18 +509,18 @@ SnpReceive ( ) { EFI_STATUS Status;

• YUKON_DRIVER *Snp;

• YUKON_DRIVER *YukonDriver; EFI_TPL OldTpl;

  if (This == NULL) { return EFI_INVALID_PARAMETER; }

• Snp = YUKON_DEV_FROM_THIS_SNP (This);

• YukonDriver = YUKON_DEV_FROM_THIS_SNP (This);

  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);

• switch (Snp->SnpMode.State) {

• switch (YukonDriver->SnpMode.State) { case EfiSimpleNetworkInitialized: break;

@@ -538,7 +538,7 @@ SnpReceive ( goto ON_EXIT; }

• Status = mskc_receive (BufferSize, Buffer);

• Status = mskc_receive (YukonDriver->Controller, BufferSize, Buffer); if (EFI_ERROR (Status)) { if (Status == EFI_NOT_READY) { goto ON_EXIT_NO_DEBUG;

@@ -678,7 +678,7 @@ SnpReceiveFilters ( ) { EFI_STATUS Status;

• YUKON_DRIVER *YukonDriver;

• YUKON_DRIVER *YukonDriver; EFI_TPL OldTpl; UINT32 newReceiveFilter;

@@ -743,7 +743,7 @@ SnpReceiveFilters ( }

YukonDriver->SnpMode.ReceiveFilterSetting = newReceiveFilter;

• mskc_rxfilter (YukonDriver->SnpMode.ReceiveFilterSetting,

MCastFilterCnt, MCastFilter);

• mskc_rxfilter (YukonDriver->Controller,

YukonDriver->SnpMode.ReceiveFilterSetting, MCastFilterCnt, MCastFilter);

Status = EFI_SUCCESS;
goto ON_EXIT;

@@ -788,7 +788,7 @@ SnpReset ( ) { EFI_STATUS Status;

• YUKON_DRIVER *YukonDriver;

• YUKON_DRIVER *YukonDriver; EFI_TPL OldTpl;

  DEBUG ((EFI_D_NET, "Marvell Yukon: SnpReset()\n"));

@@ -858,7 +858,7 @@ SnpShutdown ( ) { EFI_STATUS Status;

• YUKON_DRIVER *YukonDriver;

• YUKON_DRIVER *YukonDriver; EFI_TPL OldTpl;

  DEBUG ((EFI_D_NET, "Marvell Yukon: SnpShutdown()\n"));

@@ -890,7 +890,7 @@ SnpShutdown ( goto ON_ERROR_RESTORE_TPL; }

• mskc_shutdown ();

• mskc_shutdown (YukonDriver->Controller); YukonDriver->SnpMode.State = EfiSimpleNetworkStarted; Status = EFI_SUCCESS;

@@ -938,7 +938,7 @@ SnpStart ( IN EFI_SIMPLE_NETWORK_PROTOCOL *This ) {

• YUKON_DRIVER *YukonDriver;

• YUKON_DRIVER *YukonDriver; EFI_TPL OldTpl; EFI_STATUS Status;

@@ -966,7 +966,7 @@ SnpStart ( goto ON_ERROR_RESTORE_TPL; }

• Status = mskc_attach (YukonDriver->PciIo,

&YukonDriver->SnpMode.PermanentAddress);

• Status = mskc_attach (YukonDriver->Controller,

YukonDriver->PciIo, &YukonDriver->SnpMode.PermanentAddress);

if (EFI_ERROR (Status)) {
  goto ON_ERROR_RESTORE_TPL;

@@ -1025,7 +1025,7 @@ SnpStationAddress ( IN EFI_MAC_ADDRESS *New OPTIONAL ) {

• YUKON_DRIVER *YukonDriver;

• YUKON_DRIVER *YukonDriver; EFI_TPL OldTpl; EFI_STATUS Status;

@@ -1122,7 +1122,7 @@ SnpStatistics ( ) { EFI_STATUS Status;

• YUKON_DRIVER *YukonDriver;

• YUKON_DRIVER *YukonDriver; EFI_TPL OldTpl;

  DEBUG ((EFI_D_NET, "Marvell Yukon: SnpStatistics()\n"));

@@ -1242,7 +1242,7 @@ SnpStop ( ) { EFI_STATUS Status;

• YUKON_DRIVER *YukonDriver;

• YUKON_DRIVER *YukonDriver; EFI_TPL OldTpl;

  DEBUG ((EFI_D_NET, "Marvell Yukon: SnpStop()\n"));

@@ -1269,7 +1269,7 @@ SnpStop ( goto ON_ERROR_RESTORE_TPL; }

• mskc_detach ();

• mskc_detach (YukonDriver->Controller); YukonDriver->SnpMode.State = EfiSimpleNetworkStopped; gBS->SetMem (&YukonDriver->SnpMode.CurrentAddress, sizeof

(EFI_MAC_ADDRESS), 0); Status = EFI_SUCCESS; @@ -1407,7 +1407,7 @@ SnpTransmit ( gBS->CopyMem (&Frame->EtherType, &ProtocolNet, sizeof (UINT16)); }

• Status = mskc_transmit (BufferSize, Buffer);

• Status = mskc_transmit (YukonDriver->Controller, BufferSize,

Buffer);

ON_EXIT: gBS->RestoreTPL (OldTpl); diff --git a/Drivers/Net/MarvellYukonDxe/e1000phy.c b/Drivers/Net/MarvellYukonDxe/e1000phy.c index dee4cdf..ca84138 100644 --- a/Drivers/Net/MarvellYukonDxe/e1000phy.c +++ b/Drivers/Net/MarvellYukonDxe/e1000phy.c @@ -57,6 +57,7 @@ #include <Library/MemoryAllocationLib.h> #include <Library/DebugLib.h> #include <Library/UefiBootServicesTableLib.h> +#include <Library/BaseLib.h>

#include "if_media.h"

@@ -64,27 +65,30 @@

#include "e1000phyreg.h"

+static LIST_ENTRY e1000phy_drv_data_head;

+static void e1000phy_attach (const struct mii_attach_args *ma, EFI_HANDLE, struct e1000phy_softc *); static EFI_STATUS e1000phy_probe (const struct mii_attach_args *ma); -static void e1000phy_attach (const struct mii_attach_args *ma); static const struct mii_phydesc * mii_phy_match (const struct mii_attach_args *ma, const struct mii_phydesc *mpd); static const struct mii_phydesc * mii_phy_match_gen (const struct mii_attach_args *ma, const struct mii_phydesc *mpd, UINTN endlen); static EFI_STATUS mii_phy_dev_probe (const struct mii_attach_args *ma, const struct mii_phydesc *mpd);

-struct e1000phy_softc {

• struct mii_softc mii_sc;
• INTN mii_model;
• const struct msk_mii_data *mmd;

-}; +static EFI_STATUS e1000phy_get_ctrl_data_phy (EFI_HANDLE, INT32, struct e1000phy_softc **); +static EFI_STATUS e1000phy_add_ctrl_data (EFI_HANDLE, INT32, struct e1000phy_softc *); +static EFI_STATUS e1000phy_del_ctrl_data (EFI_HANDLE, INT32); +static EFI_STATUS e1000phy_find_drv_node (EFI_HANDLE, INT32, E1000PHY_LINKED_DRV_BUF**);

-struct e1000phy_softc *mPhySoftc; +void msk_miibus_statchg (INTN, EFI_HANDLE); +INTN msk_phy_readreg (INTN, INTN, EFI_HANDLE); +INTN msk_phy_writereg (INTN, INTN, INTN, EFI_HANDLE);

-static void mii_phy_update (INTN); -static void e1000phy_service (INTN); -static void e1000phy_status (struct mii_softc *); -static void e1000phy_reset (struct mii_softc *); -static void e1000phy_mii_phy_auto (void); +static void mii_phy_update (INTN, EFI_HANDLE, struct e1000phy_softc *); +static void e1000phy_service (INTN, EFI_HANDLE, struct e1000phy_softc *); +static void e1000phy_status (struct mii_softc *, EFI_HANDLE, struct e1000phy_softc *); +static void e1000phy_reset (struct mii_softc *, EFI_HANDLE, struct e1000phy_softc *); +static void e1000phy_mii_phy_auto (EFI_HANDLE, struct e1000phy_softc *);

static const struct mii_phydesc e1000phys[] = { MII_PHY_DESC (MARVELL, E1000), @@ -113,12 +117,15 @@ static const struct mii_phydesc e1000phys[] = { EFI_STATUS e1000_probe_and_attach ( struct mii_data *mii,

• const struct msk_mii_data *mmd
• )

• const struct msk_mii_data *mmd,
• EFI_HANDLE mController,
• INT32 mPort

Please don't use the m prefix for struct fields or function arguments. It is intended for variables with module scope

I should not be a problem to correct new variables. The mPhySoftc and mSoftc originally were global pointers but now passed as function parameters. Changing them will require a lot of renaming inside of many many functions. I tried to keep the changes to minimum but renaming these variables will make this patch pretty big.

The rename should be put in a separate patch to make it easy to review. That will keep down the size of the main patch.

Alan.