netdev - Re: [PATCH v2 2/2] net: phy: dp83867: Add SGMII mode type switching

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Sep 2019 07:23:16 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vitaly Gaiduk <vitaly.gaiduk@...udbear.ru>, davem@...emloft.net,
        robh+dt@...nel.org
Cc:     Mark Rutland <mark.rutland@....com>, Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Trent Piepho <tpiepho@...inj.com>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] net: phy: dp83867: Add SGMII mode type switching



On 9/9/2019 4:02 AM, Vitaly Gaiduk wrote:
> This patch adds ability to switch beetween two PHY SGMII modes.
> Some hardware, for example, FPGA IP designs may use 6-wire mode
> which enables differential SGMII clock to MAC.
> 
> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@...udbear.ru>
> ---
> Changes in v2:
> - changed variable sgmii_type name to sgmii_ref_clk_en
> 
>  drivers/net/phy/dp83867.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 1f1ecee..cd6260e 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -37,6 +37,7 @@
>  #define DP83867_STRAP_STS2	0x006f
>  #define DP83867_RGMIIDCTL	0x0086
>  #define DP83867_IO_MUX_CFG	0x0170
> +#define DP83867_SGMIICTL	0x00D3
>  #define DP83867_10M_SGMII_CFG   0x016F
>  #define DP83867_10M_SGMII_RATE_ADAPT_MASK BIT(7)
> 
> @@ -61,6 +62,9 @@
>  #define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
>  #define DP83867_RGMII_RX_CLK_DELAY_EN		BIT(0)
> 
> +/* SGMIICTL bits */
> +#define DP83867_SGMII_TYPE		BIT(14)
> +
>  /* STRAP_STS1 bits */
>  #define DP83867_STRAP_STS1_RESERVED		BIT(11)
> 
> @@ -109,6 +113,7 @@ struct dp83867_private {
>  	bool rxctrl_strap_quirk;
>  	bool set_clk_output;
>  	u32 clk_output_sel;
> +	bool sgmii_ref_clk_en;
>  };
> 
>  static int dp83867_ack_interrupt(struct phy_device *phydev)
> @@ -197,6 +202,9 @@ static int dp83867_of_init(struct phy_device *phydev)
>  	dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
>  					"ti,dp83867-rxctrl-strap-quirk");
> 
> +	dp83867->sgmii_ref_clk_en = of_property_read_bool(of_node,
> +					"ti,sgmii-ref-clock-output-enable");
> +
>  	/* Existing behavior was to use default pin strapping delay in rgmii
>  	 * mode, but rgmii should have meant no delay.  Warn existing users.
>  	 */
> @@ -389,6 +397,14 @@ static int dp83867_config_init(struct phy_device *phydev)
> 
>  		if (ret)
>  			return ret;
> +
> +		/* SGMII type is set to 4-wire mode by default */
> +		if (dp83867->sgmii_ref_clk_en) {
> +			/* Switch on 6-wire mode */
> +			val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
> +			val |= DP83867_SGMII_TYPE;
> +			phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);
> +		}

Is there a case where the value could be retained across a power
on/reset cycle and you would want to make sure you do write the intended
"wire mode" here? What I am suggesting is just changing this into a:

	val =  phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
	if (dp83867->sgmii_ref_clk_en)
		val |= DP83867_SGMII_TYPE;
	else
		val &= ~DP83867_SGMII_TYPE;
	phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);
	
Other than that, LGTM
-- 
Florian