From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=UNvyCCsm; spf=pass (domain: linaro.org, ip: 209.85.167.67, mailfrom: masahisa.kojima@linaro.org) Received: from mail-lf1-f67.google.com (mail-lf1-f67.google.com [209.85.167.67]) by groups.io with SMTP; Tue, 23 Jul 2019 23:43:00 -0700 Received: by mail-lf1-f67.google.com with SMTP id b17so31143239lff.7 for ; Tue, 23 Jul 2019 23:42:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=xBSIP7utHSlHAa1DJ8JWg/R5c4IN/3JGdBuGLGJOwa0=; b=UNvyCCsmHGiXBirf2ql6G8jIAlZbUcE7GGkrWr5zbYlz23A1DNgaCy7JfBR5qOs/xy E1ru1Fv1ILrnG9GoxeC9zDpZwAd7MVem7p5xf4Urac9phjAmbw6rU+655UZwbClMzbBe PXDlu1A2f6AAibbJLvIx8caYNtNGh1zFe2d1jEViAhXEwF4DZdtLz9r4ujX1gK+3LaBb bwwubFQy7hBGSjtZTfzD3xxHjn2WOVGmTrwSCjdCg8sWTSWXgUJMaQ95zI4/6Fmu0gdp /mpVlEYYyHOu7wMkYAyQ6KBJy1V+Q18lZqvHgfLkoKs5HRPb7YSTprdtO2VXYohBCAJ5 8W+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=xBSIP7utHSlHAa1DJ8JWg/R5c4IN/3JGdBuGLGJOwa0=; b=ngV2qdyNLx2QDIKf65u/nGPPAtUHPa43+Z7yB/1DwkS5Cl6L4wc77QRyP0ivSiMIOl +4B73QTg7Lr1+jGJsuBl0vVHvBzDsr/TbtevDKVVw8XYhgQuJVJy5DCS27c85CTHs/Dn g2o94rTGRFDcbilxC5kQIAg8lDdonGEln0qIFJ1kry1uXjrkopGGbWWNoyAqD8ucVuB+ vuF9HZEUFuIU9Ni4yD2zlzvRrf1CK1ENtPjdBVKptX2BeNMYdgHUKasuM27gbgfA2INd /3ctNM6svR9nKSqBKdC0JSVGzv3UcyiuNusBr+vhOI2e4EXbn0bXldn7xf0Ya0HkNkmw Cwrw== X-Gm-Message-State: APjAAAU6j4Tq5fNaDzhKz1Jf7B3ykdrH19AkU0O7w52FcLIx9y3Sir17 Ay+n+CCEqmtoNtRskPWGAE2V9ktImM8i/lh99W93XQ== X-Google-Smtp-Source: APXvYqwnPI0K4FULoqsoOrl1HN3Ym/qXv7TJ1eZtP7cfZqXcKL6DRqoivuZvtxyfrsAsHYh+z8Tmu8SuQs5Mv4fWbdo= X-Received: by 2002:ac2:4c37:: with SMTP id u23mr23308233lfq.119.1563950578046; Tue, 23 Jul 2019 23:42:58 -0700 (PDT) MIME-Version: 1.0 References: <20190722115636.3413-1-masahisa.kojima@linaro.org> <20190722115636.3413-4-masahisa.kojima@linaro.org> <20190723161405.GL11541@bivouac.eciton.net> In-Reply-To: <20190723161405.GL11541@bivouac.eciton.net> From: "Masahisa Kojima" Date: Wed, 24 Jul 2019 15:42:46 +0900 Message-ID: Subject: Re: [edk2-platforms PATCH v1 3/3] NetsecDxe: SnpInitialize() waits for media linking up To: Leif Lindholm Cc: edk2-devel-groups-io , Ard Biesheuvel , Satoru Okamoto Content-Type: text/plain; charset="UTF-8" On Wed, 24 Jul 2019 at 01:14, Leif Lindholm wrote: > > On Mon, Jul 22, 2019 at 08:56:36PM +0900, Masahisa Kojima wrote: > > The latest NetsecDxe requires issueing phy reset at the > > last stage of initialization to safely exit loopback mode. > > However, as a result, it takes a couple of seconds for link state > > to get stable, which could cause auto-chosen pxeboot to fail > > due to MediaPresent check error. > > > > This patch adds link state check with 5s timeout in NetsecDxe > > initialization. The timeout value can be adjustable via > > configuration file. > > > > Signed-off-by: Masahisa Kojima > > Signed-off-by: Satoru Okamoto > > --- > > .../Socionext/DeveloperBox/DeveloperBox.dsc | 1 + > > .../Drivers/Net/NetsecDxe/NetsecDxe.c | 232 ++++++++---------- > > .../Drivers/Net/NetsecDxe/NetsecDxe.dec | 1 + > > .../Drivers/Net/NetsecDxe/NetsecDxe.inf | 1 + > > 4 files changed, 110 insertions(+), 125 deletions(-) > > Please run edk2/BaseTools/Scripts/SetupGit.py from within this > repository. It sets up some common options that help with reviewing > patches. > > (--stat=1000 is one option that can unfortunately not be overridden in > this way) I understand. I address all your comments, then send v2 patch. > > > > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > > index 97fb8c410c60..af149607f3ce 100644 > > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > > @@ -138,6 +138,7 @@ > > gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStartThreshold|36 > > gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStopThreshold|48 > > gNetsecDxeTokenSpaceGuid.PcdPauseTime|256 > > + gNetsecDxeTokenSpaceGuid.PcdMediaDetectTimeoutOnBoot|5 > > Please insert alphabetically sorted. > > > > > gSynQuacerTokenSpaceGuid.PcdNetsecEepromBase|0x08080000 > > gSynQuacerTokenSpaceGuid.PcdNetsecPhyAddress|7 > > diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c > > index 0b91d4af44a3..a304e02208fa 100644 > > --- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c > > +++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c > > @@ -169,6 +169,98 @@ ExitUnlock: > > return Status; > > } > > > > +EFI_STATUS > > +EFIAPI > > +NetsecUpdateLink ( > > + IN EFI_SIMPLE_NETWORK_PROTOCOL *Snp > > + ) > > +{ > > + NETSEC_DRIVER *LanDriver; > > + ogma_phy_link_status_t phy_link_status; > > + ogma_gmac_mode_t ogma_gmac_mode; > > + ogma_err_t ogma_err; > > + BOOLEAN ValidFlag; > > + ogma_gmac_mode_t GmacMode; > > + BOOLEAN RxRunningFlag; > > + BOOLEAN TxRunningFlag; > > + EFI_STATUS ErrorStatus; > > + > > + LanDriver = INSTANCE_FROM_SNP_THIS (Snp); > > + > > + // Update the media status > > + ogma_err = ogma_get_phy_link_status (LanDriver->Handle, > > + &phy_link_status); > > + if (ogma_err != OGMA_ERR_OK) { > > + DEBUG ((DEBUG_ERROR, > > + "NETSEC: ogma_get_phy_link_status failed with error code: %d\n", > > + (INT32)ogma_err)); > > + ErrorStatus = EFI_DEVICE_ERROR; > > + goto Fail; > > + } > > + > > + // Update the GMAC status > > + ogma_err = ogma_get_gmac_status (LanDriver->Handle, &ValidFlag, &GmacMode, > > + &RxRunningFlag, &TxRunningFlag); > > + if (ogma_err != OGMA_ERR_OK) { > > + DEBUG ((DEBUG_ERROR, > > + "NETSEC: ogma_get_gmac_status failed with error code: %d\n", > > + (INT32)ogma_err)); > > + ErrorStatus = EFI_DEVICE_ERROR; > > + goto Fail; > > + } > > + > > + // Stop GMAC when GMAC is running and physical link is down > > + if (RxRunningFlag && TxRunningFlag && !phy_link_status.up_flag) { > > + ogma_err = ogma_stop_gmac (LanDriver->Handle, OGMA_TRUE, OGMA_TRUE); > > + if (ogma_err != OGMA_ERR_OK) { > > + DEBUG ((DEBUG_ERROR, > > + "NETSEC: ogma_stop_gmac() failed with error status %d\n", > > + ogma_err)); > > + ErrorStatus = EFI_DEVICE_ERROR; > > + goto Fail; > > + } > > + } > > + > > + // Start GMAC when GMAC is stopped and physical link is up > > + if (!RxRunningFlag && !TxRunningFlag && phy_link_status.up_flag) { > > + ZeroMem (&ogma_gmac_mode, sizeof (ogma_gmac_mode_t)); > > + ogma_gmac_mode.link_speed = phy_link_status.link_speed; > > + ogma_gmac_mode.half_duplex_flag = (ogma_bool)phy_link_status.half_duplex_flag; > > + if (!phy_link_status.half_duplex_flag && FixedPcdGet8 (PcdFlowCtrl)) { > > + ogma_gmac_mode.flow_ctrl_enable_flag = FixedPcdGet8 (PcdFlowCtrl); > > + ogma_gmac_mode.flow_ctrl_start_threshold = FixedPcdGet16 (PcdFlowCtrlStartThreshold); > > + ogma_gmac_mode.flow_ctrl_stop_threshold = FixedPcdGet16 (PcdFlowCtrlStopThreshold); > > + ogma_gmac_mode.pause_time = FixedPcdGet16 (PcdPauseTime); > > + } > > + > > + ogma_err = ogma_set_gmac_mode (LanDriver->Handle, &ogma_gmac_mode); > > + if (ogma_err != OGMA_ERR_OK) { > > + DEBUG ((DEBUG_ERROR, > > + "NETSEC: ogma_set_gmac() failed with error status %d\n", > > + (INT32)ogma_err)); > > + ErrorStatus = EFI_DEVICE_ERROR; > > + goto Fail; > > + } > > + > > + ogma_err = ogma_start_gmac (LanDriver->Handle, OGMA_TRUE, OGMA_TRUE); > > + if (ogma_err != OGMA_ERR_OK) { > > + DEBUG ((DEBUG_ERROR, > > + "NETSEC: ogma_start_gmac() failed with error status %d\n", > > + (INT32)ogma_err)); > > + ErrorStatus = EFI_DEVICE_ERROR; > > + goto Fail; > > + } > > + } > > + > > + /* Updating link status for external guery */ > > + Snp->Mode->MediaPresent = phy_link_status.up_flag; > > + return EFI_SUCCESS; > > + > > +Fail: > > + Snp->Mode->MediaPresent = FALSE; > > + return ErrorStatus; > > +} > > + > > /* > > * UEFI Initialize() function > > */ > > @@ -185,9 +277,9 @@ SnpInitialize ( > > EFI_TPL SavedTpl; > > EFI_STATUS Status; > > > > - ogma_phy_link_status_t phy_link_status; > > ogma_err_t ogma_err; > > - ogma_gmac_mode_t ogma_gmac_mode; > > + > > + UINT32 Index; > > > > // Check Snp Instance > > if (Snp == NULL) { > > @@ -271,48 +363,18 @@ SnpInitialize ( > > ogma_disable_desc_ring_irq (LanDriver->Handle, OGMA_DESC_RING_ID_NRM_TX, > > OGMA_CH_IRQ_REG_EMPTY); > > > > - // Stop and restart the physical link > > - ogma_err = ogma_stop_gmac (LanDriver->Handle, OGMA_TRUE, OGMA_TRUE); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_stop_gmac() failed with error status %d\n", > > - ogma_err)); > > - ReturnUnlock (EFI_DEVICE_ERROR); > > - } > > - > > - ogma_err = ogma_get_phy_link_status (LanDriver->Handle, > > - &phy_link_status); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_get_phy_link_status() failed error code %d\n", > > - (INT32)ogma_err)); > > - ReturnUnlock (EFI_DEVICE_ERROR); > > - } > > - > > - SetMem (&ogma_gmac_mode, sizeof (ogma_gmac_mode_t), 0); > > - ogma_gmac_mode.link_speed = phy_link_status.link_speed; > > - ogma_gmac_mode.half_duplex_flag = (ogma_bool)phy_link_status.half_duplex_flag; > > - if ((!phy_link_status.half_duplex_flag) && FixedPcdGet8 (PcdFlowCtrl)) { > > - ogma_gmac_mode.flow_ctrl_enable_flag = FixedPcdGet8 (PcdFlowCtrl); > > - ogma_gmac_mode.flow_ctrl_start_threshold = FixedPcdGet16 (PcdFlowCtrlStartThreshold); > > - ogma_gmac_mode.flow_ctrl_stop_threshold = FixedPcdGet16 (PcdFlowCtrlStopThreshold); > > - ogma_gmac_mode.pause_time = FixedPcdGet16 (PcdPauseTime); > > - } > > - > > - ogma_err = ogma_set_gmac_mode (LanDriver->Handle, &ogma_gmac_mode); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_set_gmac() failed with error status %d\n", > > - (INT32)ogma_err)); > > - ReturnUnlock (EFI_DEVICE_ERROR); > > - } > > - > > - ogma_err = ogma_start_gmac (LanDriver->Handle, OGMA_TRUE, OGMA_TRUE); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_start_gmac() failed with error status %d\n", > > - (INT32)ogma_err)); > > - ReturnUnlock (EFI_DEVICE_ERROR); > > + // 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 > > @@ -420,14 +482,6 @@ NetsecPollPhyStatus ( > > ) > > { > > EFI_SIMPLE_NETWORK_PROTOCOL *Snp; > > - NETSEC_DRIVER *LanDriver; > > - ogma_phy_link_status_t phy_link_status; > > - ogma_gmac_mode_t ogma_gmac_mode; > > - ogma_err_t ogma_err; > > - BOOLEAN ValidFlag; > > - ogma_gmac_mode_t GmacMode; > > - BOOLEAN RxRunningFlag; > > - BOOLEAN TxRunningFlag; > > > > Snp = (EFI_SIMPLE_NETWORK_PROTOCOL *)Context; > > if (Snp == NULL) { > > @@ -435,66 +489,7 @@ NetsecPollPhyStatus ( > > return; > > } > > > > - LanDriver = INSTANCE_FROM_SNP_THIS (Snp); > > - > > - // Update the media status > > - ogma_err = ogma_get_phy_link_status (LanDriver->Handle, > > - &phy_link_status); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_get_phy_link_status failed with error code: %d\n", > > - (INT32)ogma_err)); > > - return; > > - } > > - > > - // Update the GMAC status > > - ogma_err = ogma_get_gmac_status (LanDriver->Handle, &ValidFlag, &GmacMode, > > - &RxRunningFlag, &TxRunningFlag); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_get_gmac_status failed with error code: %d\n", > > - (INT32)ogma_err)); > > - return; > > - } > > - > > - // Stop GMAC when GMAC is running and physical link is down > > - if (RxRunningFlag && TxRunningFlag && !phy_link_status.up_flag) { > > - ogma_err = ogma_stop_gmac (LanDriver->Handle, OGMA_TRUE, OGMA_TRUE); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_stop_gmac() failed with error status %d\n", > > - ogma_err)); > > - return; > > - } > > - } > > - > > - // Start GMAC when GMAC is stopped and physical link is up > > - if (!RxRunningFlag && !TxRunningFlag && phy_link_status.up_flag) { > > - ZeroMem (&ogma_gmac_mode, sizeof (ogma_gmac_mode_t)); > > - ogma_gmac_mode.link_speed = phy_link_status.link_speed; > > - ogma_gmac_mode.half_duplex_flag = (ogma_bool)phy_link_status.half_duplex_flag; > > - if (!phy_link_status.half_duplex_flag && FixedPcdGet8 (PcdFlowCtrl)) { > > - ogma_gmac_mode.flow_ctrl_enable_flag = FixedPcdGet8 (PcdFlowCtrl); > > - ogma_gmac_mode.flow_ctrl_start_threshold = FixedPcdGet16 (PcdFlowCtrlStartThreshold); > > - ogma_gmac_mode.flow_ctrl_stop_threshold = FixedPcdGet16 (PcdFlowCtrlStopThreshold); > > - ogma_gmac_mode.pause_time = FixedPcdGet16 (PcdPauseTime); > > - } > > - > > - ogma_err = ogma_set_gmac_mode (LanDriver->Handle, &ogma_gmac_mode); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_set_gmac() failed with error status %d\n", > > - (INT32)ogma_err)); > > - return; > > - } > > - > > - ogma_err = ogma_start_gmac (LanDriver->Handle, OGMA_TRUE, OGMA_TRUE); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_start_gmac() failed with error status %d\n", > > - (INT32)ogma_err)); > > - } > > - } > > + NetsecUpdateLink (Snp); > > } > > > > /* > > @@ -631,7 +626,6 @@ SnpGetStatus ( > > pfdep_pkt_handle_t pkt_handle; > > LIST_ENTRY *Link; > > > > - ogma_phy_link_status_t phy_link_status; > > ogma_err_t ogma_err; > > > > // Check preliminaries > > @@ -661,18 +655,6 @@ SnpGetStatus ( > > // Find the LanDriver structure > > LanDriver = INSTANCE_FROM_SNP_THIS (Snp); > > > > - // Update the media status > > - ogma_err = ogma_get_phy_link_status (LanDriver->Handle, > > - &phy_link_status); > > - if (ogma_err != OGMA_ERR_OK) { > > - DEBUG ((DEBUG_ERROR, > > - "NETSEC: ogma_get_phy_link_status failed with error code: %d\n", > > - (INT32)ogma_err)); > > - ReturnUnlock (EFI_DEVICE_ERROR); > > - } > > - > > - Snp->Mode->MediaPresent = phy_link_status.up_flag; > > - > > ogma_err = ogma_clean_tx_desc_ring (LanDriver->Handle, > > OGMA_DESC_RING_ID_NRM_TX); > > > > diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec > > index 6b9f60293879..016eba5ef695 100644 > > --- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec > > +++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec > > @@ -38,3 +38,4 @@ > > gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStartThreshold|0x0|UINT16|0x00000006 > > gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStopThreshold|0x0|UINT16|0x00000007 > > gNetsecDxeTokenSpaceGuid.PcdPauseTime|0x0|UINT16|0x00000008 > > + gNetsecDxeTokenSpaceGuid.PcdMediaDetectTimeoutOnBoot|0x0|UINT8|0x00000009 > > Please insert alphabetically sorted. > > > diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf > > index 49dd28efc65b..818014e6d257 100644 > > --- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf > > +++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.inf > > @@ -62,3 +62,4 @@ > > gNetsecDxeTokenSpaceGuid.PcdFlowCtrlStopThreshold > > gNetsecDxeTokenSpaceGuid.PcdJumboPacket > > gNetsecDxeTokenSpaceGuid.PcdPauseTime > > + gNetsecDxeTokenSpaceGuid.PcdMediaDetectTimeoutOnBoot > > Please insert slphabetcally sorted. > > / > Leif > > > -- > > 2.17.1 > >