From: "Masahisa Kojima" <masahisa.kojima@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Satoru Okamoto <okamoto.satoru@socionext.com>
Subject: Re: [edk2-platforms PATCH v1 3/3] NetsecDxe: SnpInitialize() waits for media linking up
Date: Wed, 24 Jul 2019 15:42:46 +0900 [thread overview]
Message-ID: <CADQ0-X-ROdJfFQ6D7d_rBrwvqYorS8+DCxdfM5nyg_tvrHn6tQ@mail.gmail.com> (raw)
In-Reply-To: <20190723161405.GL11541@bivouac.eciton.net>
On Wed, 24 Jul 2019 at 01:14, Leif Lindholm <leif.lindholm@linaro.org> 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 <masahisa.kojima@linaro.org>
> > Signed-off-by: Satoru Okamoto <okamoto.satoru@socionext.com>
> > ---
> > .../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
> >
prev parent reply other threads:[~2019-07-24 6:43 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-22 11:56 [edk2-platforms PATCH v1 0/3] Robust Netsec Initialiation Masahisa Kojima
2019-07-22 11:56 ` [edk2-platforms PATCH v1 1/3] NetsecDxe: embed phy address into NETSEC SDK internal structure Masahisa Kojima
2019-07-23 15:41 ` Leif Lindholm
2019-07-22 11:56 ` [edk2-platforms PATCH v1 2/3] NetsecDxe: put phy in loopback mode in order to guarantee stable RXCLK input Masahisa Kojima
2019-07-23 15:49 ` Leif Lindholm
2019-07-22 11:56 ` [edk2-platforms PATCH v1 3/3] NetsecDxe: SnpInitialize() waits for media linking up Masahisa Kojima
2019-07-23 16:14 ` Leif Lindholm
2019-07-24 6:42 ` Masahisa Kojima [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADQ0-X-ROdJfFQ6D7d_rBrwvqYorS8+DCxdfM5nyg_tvrHn6tQ@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox