Re: [edk2-platforms PATCH v1 2/3] NetsecDxe: put phy in loopback mode in order to guarantee stable RXCLK input

["Leif Lindholm" <leif.lindholm@linaro.org>]

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 <masahisa.kojima@linaro.org>
> Signed-off-by: Satoru Okamoto <okamoto.satoru@socionext.com>
> ---
>  .../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
>