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=ijFriQsf; spf=pass (domain: linaro.org, ip: 209.85.128.67, mailfrom: leif.lindholm@linaro.org) Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by groups.io with SMTP; Tue, 23 Jul 2019 08:49:58 -0700 Received: by mail-wm1-f67.google.com with SMTP id l2so38943517wmg.0 for ; Tue, 23 Jul 2019 08:49:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=2AkY3+4mbr5UXdty8CsVvQsBqdPke40VNBiTTdVcpnw=; b=ijFriQsfqKO6CNCzv5j0/CgO0xiE2EQD8e+9HtYoAnTBpEH200XWsw5P0totIumeO6 yfxtsNqd8WvTLfiPxy1wPpNFRc49colwrxT2lan/rtmgsapW8ZMSP6N7fc0gxAb0K2/V X5yO7KjKAhTV9u3LaOaVcO8GOIi4wJy8PIgZ6oj0ToPQqO/3f+CXVf2ZEwfMOMbtaSMr HodMsMgm/euFrhWwSwXEpE1hBxyeboZGS3D9FjQDMGqUrVDGtzmmOCXiNOGpxld2z+/o dPIrI4GSyJG0ppPsVQAWM6VIH0pc3E5WHmLyt0xp19WtvITm2eh5j7jzIn6FAKNohRqy 6SoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=2AkY3+4mbr5UXdty8CsVvQsBqdPke40VNBiTTdVcpnw=; b=uLZnDjIWbjbzDD4W8/SKj8U2AK7LzfiKtprLeZQ5Dr3/o3l994gJXX4JInTBIM62+U 10Uo90pnZ02qA3K0qkoG7HRPAT94+Kfqc8wkmOlb0NRkGaMYqcMzYDbI1+L04FJcaTbE FHtZOs0TafKiHlFPAy/hH7abtTThZQz71/J5qsDabAbkb1YntuUvi/G5fmexyX4JUwdE D3sLDNi4b+rsXAl/kk5wyVui2iX0NwhUr7+BVusPdd29W5MbwhUWMGZ+NwXzkLUM+zI7 SFoqAgAlGUlhpL5ubZ8YPrOzm+hFDXKxMd0Yyvmd+ZZnlPFe2xonXP3s/twDOSEmbqd1 10Mw== X-Gm-Message-State: APjAAAX9H/LXR4RIjMCIKUw1JNoY4bKi+Pj4ppzP1A+gBeSUSFSz4Az/ EteC62uyXbfqge8PEvhWmlR/5Q== X-Google-Smtp-Source: APXvYqwyCQ4NCZnqOoTX3VNWJ6573F3rOU0Yh8115HX+Dc2I5AIfLKJ4XJdxVjayXmy28/LV0pT6oQ== X-Received: by 2002:a05:600c:10ce:: with SMTP id l14mr67847037wmd.118.1563896996492; Tue, 23 Jul 2019 08:49:56 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id y24sm31457283wmi.10.2019.07.23.08.49.55 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Tue, 23 Jul 2019 08:49:55 -0700 (PDT) Date: Tue, 23 Jul 2019 16:49:53 +0100 From: "Leif Lindholm" To: Masahisa Kojima Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org, okamoto.satoru@socionext.com Subject: Re: [edk2-platforms PATCH v1 2/3] NetsecDxe: put phy in loopback mode in order to guarantee stable RXCLK input Message-ID: <20190723154953.GK11541@bivouac.eciton.net> References: <20190722115636.3413-1-masahisa.kojima@linaro.org> <20190722115636.3413-3-masahisa.kojima@linaro.org> MIME-Version: 1.0 In-Reply-To: <20190722115636.3413-3-masahisa.kojima@linaro.org> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Jul 22, 2019 at 08:56:35PM +0900, Masahisa Kojima wrote: > NETSEC hardware requires stable RXCLK input upon initialization > triggered with DISCORE = 0. > However, RXCLK input could be unstable depending on phy chipset > and deployed network environment, which could cause NETSEC to > hang up during initialization. > > We solve this platform/environment dependent issue by temporarily > putting phy in loopback mode, then we can expect the stable RXCLK input. > > Signed-off-by: Masahisa Kojima > Signed-off-by: Satoru Okamoto > --- > .../netsec_sdk/src/ogma_misc.c | 72 ++++++++++++++++++- > .../netsec_for_uefi/netsec_sdk/src/ogma_reg.h | 4 ++ > 2 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_misc.c b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_misc.c > index 7481d2da2d24..07308d38a5c2 100644 > --- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_misc.c > +++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_misc.c > @@ -35,7 +35,6 @@ static const ogma_uint32 desc_ring_config_reg_addr[OGMA_DESC_RING_ID_MAX + 1] = > > }; > > - Please, no unrelated whitespace changes. > /* Internal function definition*/ > #ifndef OGMA_CONFIG_DISABLE_CLK_CTRL > STATIC void ogma_set_clk_en_reg ( > @@ -327,6 +326,60 @@ STATIC ogma_uint32 ogma_calc_pkt_ctrl_reg_param ( > return param; > } > > +STATIC > +void > +ogma_pre_init_microengine ( > + ogma_handle_t ogma_handle > + ) > +{ > + UINT16 Data; > + > + /* Remove dormant settings */ > + Data = ogma_get_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL) & > + ~((1U << OGMA_PHY_CONTROL_REG_POWER_DOWN) | > + (1U << OGMA_PHY_CONTROL_REG_ISOLATE)); I am unsure of how much we should try to adhere to TianoCore coding style in this imported module, but the above does not look correct regardless. Should not the second/third lines should be aligned relative to the ogma_get_phy_reg call rather than the variable name... > + > + ogma_set_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL, Data); > + > + while ((ogma_get_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL) & > + ((1U << OGMA_PHY_CONTROL_REG_POWER_DOWN) | > + (1U << OGMA_PHY_CONTROL_REG_ISOLATE))) != 0); ...like it is here. > + > + /* Put phy in loopback mode to guarantee RXCLK input */ > + Data |= (1U << OGMA_PHY_CONTROL_REG_LOOPBACK); > + > + ogma_set_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL, Data); > + > + while ((ogma_get_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL) & > + (1U << OGMA_PHY_CONTROL_REG_LOOPBACK)) == 0); > +} > + > +STATIC > +void > +ogma_post_init_microengine ( > + IN ogma_handle_t ogma_handle > + ) > +{ > + UINT16 Data; > + > + /* Get phy back to normal operation */ > + Data = ogma_get_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL) & > + ~(1U << OGMA_PHY_CONTROL_REG_LOOPBACK); Same indentation comment as above. > + > + ogma_set_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL, Data); > + > + while ((ogma_get_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL) & > + (1U << OGMA_PHY_CONTROL_REG_LOOPBACK)) != 0); > + > + Data |= (1U << OGMA_PHY_CONTROL_REG_RESET); > + > + /* Apply software reset */ > + ogma_set_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL, Data); > + > + while ((ogma_get_phy_reg (ogma_handle, OGMA_PHY_REG_ADDR_CONTROL) & > + (1U << OGMA_PHY_CONTROL_REG_RESET)) != 0); > +} > + > ogma_err_t ogma_init ( > void *base_addr, > pfdep_dev_handle_t dev_handle, > @@ -551,6 +604,16 @@ ogma_err_t ogma_init ( > ogma_write_reg( ctrl_p, OGMA_REG_ADDR_DMA_TMR_CTRL, > ( ogma_uint32)( ( OGMA_CONFIG_CLK_HZ / OGMA_CLK_MHZ) - 1) ); > > + /* > + * Do pre-initialization tasks for microengine > + * > + * In particular, we put phy in loopback mode > + * in order to make sure RXCLK keeps provided to mac > + * irrespective of phy link status, > + * which is required for microengine intialization. > + */ Good use of comment here, and below. *But*, if the next thing was not the bit taking the PHY back out of loopback mode, I would have had to spend time searching for whether that was done. Could you add a line saying "this will be disabled once initialization complete"? / Leif > + ogma_pre_init_microengine (ctrl_p); > + > /* start microengines */ > ogma_write_reg( ctrl_p, OGMA_REG_ADDR_DIS_CORE, 0); > > @@ -573,6 +636,13 @@ ogma_err_t ogma_init ( > goto err; > } > > + /* > + * Do post-initialization tasks for microengine > + * > + * We put phy in normal mode and apply reset. > + */ > + ogma_post_init_microengine (ctrl_p); > + > /* clear microcode load end status */ > ogma_write_reg( ctrl_p, OGMA_REG_ADDR_TOP_STATUS, > OGMA_TOP_IRQ_REG_ME_START); > diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_reg.h b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_reg.h > index 30c716352b37..ca769084cb31 100644 > --- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_reg.h > +++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_reg.h > @@ -138,8 +138,12 @@ > /* bit fields for PHY CONTROL Register */ > #define OGMA_PHY_CONTROL_REG_SPEED_SELECTION_MSB (6) > #define OGMA_PHY_CONTROL_REG_DUPLEX_MODE (8) > +#define OGMA_PHY_CONTROL_REG_ISOLATE (10) > +#define OGMA_PHY_CONTROL_REG_POWER_DOWN (11) > #define OGMA_PHY_CONTROL_REG_AUTO_NEGO_ENABLE (12) > #define OGMA_PHY_CONTROL_REG_SPEED_SELECTION_LSB (13) > +#define OGMA_PHY_CONTROL_REG_LOOPBACK (14) > +#define OGMA_PHY_CONTROL_REG_RESET (15) > > /* bit fields for PHY STATUS Register */ > #define OGMA_PHY_STATUS_REG_LINK_STATUS (2) > -- > 2.17.1 >