Received-SPF: pass (google.com: domain of linux-kernel+bounces-4068-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 18 Dec 2023 10:31:04 -0600
From: Andrew Halaney <ahalaney@redhat.com>
To: Sneh Shah <quic_snehshah@quicinc.com>
Cc: Vinod Koul <vkoul@kernel.org>, 
	Bhupesh Sharma <bhupesh.sharma@linaro.org>, Alexandre Torgue <alexandre.torgue@foss.st.com>, 
	Jose Abreu <joabreu@synopsys.com>, "David S. Miller" <davem@davemloft.net>, 
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, 
	Paolo Abeni <pabeni@redhat.com>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, 
	netdev@vger.kernel.org, linux-arm-msm@vger.kernel.org, 
	linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, 
	linux-kernel@vger.kernel.org, kernel@quicinc.com
Subject: Re: [PATCH net-next] net: stmmac: dwmac-qcom-ethqos: Add support for
 2.5G SGMII
Message-ID: <pdoedu3n7rdl6ifrfyugoa7pbjougqj4cg6mxyerhu5udf4e2h@unjqgqjt7odi>
References: <20231218071118.21879-1-quic_snehshah@quicinc.com>
 <4zbf5fmijxnajk7kygcjrcusf6tdnuzsqqboh23nr6f3rb3c4g@qkfofhq7jmv6>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4zbf5fmijxnajk7kygcjrcusf6tdnuzsqqboh23nr6f3rb3c4g@qkfofhq7jmv6>

On Mon, Dec 18, 2023 at 10:20:03AM -0600, Andrew Halaney wrote:
> On Mon, Dec 18, 2023 at 12:41:18PM +0530, Sneh Shah wrote:
> > Serdes phy needs to operate at 2500 mode for 2.5G speed and 1000
> > mode for 1G/100M/10M speed.
> > Added changes to configure serdes phy and mac based on link speed.
> > 
> > Signed-off-by: Sneh Shah <quic_snehshah@quicinc.com>
> > ---
> >  .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 31 +++++++++++++++++--
> >  1 file changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > index d3bf42d0fceb..b3a28dc19161 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > @@ -21,6 +21,7 @@
> >  #define RGMII_IO_MACRO_CONFIG2		0x1C
> >  #define RGMII_IO_MACRO_DEBUG1		0x20
> >  #define EMAC_SYSTEM_LOW_POWER_DEBUG	0x28
> > +#define ETHQOS_MAC_AN_CTRL		0xE0
> >  
> >  /* RGMII_IO_MACRO_CONFIG fields */
> >  #define RGMII_CONFIG_FUNC_CLK_EN		BIT(30)
> > @@ -78,6 +79,10 @@
> >  #define ETHQOS_MAC_CTRL_SPEED_MODE		BIT(14)
> >  #define ETHQOS_MAC_CTRL_PORT_SEL		BIT(15)
> >  
> > +/*ETHQOS_MAC_AN_CTRL bits */
> > +#define ETHQOS_MAC_AN_CTRL_RAN			BIT(9)
> > +#define ETHQOS_MAC_AN_CTRL_ANE			BIT(12)
> > +
> 
> nit: space please add a space before ETHQOS_MAC_AN_CTRL
> 
> >  struct ethqos_emac_por {
> >  	unsigned int offset;
> >  	unsigned int value;
> > @@ -109,6 +114,7 @@ struct qcom_ethqos {
> >  	unsigned int num_por;
> >  	bool rgmii_config_loopback_en;
> >  	bool has_emac_ge_3;
> > +	unsigned int serdes_speed;
> >  };
> >  
> >  static int rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
> > @@ -600,27 +606,47 @@ static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos)
> >  
> >  static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos)
> >  {
> > -	int val;
> > -
> > +	int val, mac_an_value;
> >  	val = readl(ethqos->mac_base + MAC_CTRL_REG);
> > +	mac_an_value = readl(ethqos->mac_base + ETHQOS_MAC_AN_CTRL);
> >  
> >  	switch (ethqos->speed) {
> > +	case SPEED_2500:
> > +		val &= ~ETHQOS_MAC_CTRL_PORT_SEL;
> > +		rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
> > +			      RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
> > +			      RGMII_IO_MACRO_CONFIG2);
> > +		if (ethqos->serdes_speed != SPEED_2500)
> > +			phy_set_speed(ethqos->serdes_phy, ethqos->speed);

Also, please capture the return value here and propagate the error as
appropriate.

> > +		mac_an_value &= ~ETHQOS_MAC_AN_CTRL_ANE;
> > +		break;
> >  	case SPEED_1000:
> >  		val &= ~ETHQOS_MAC_CTRL_PORT_SEL;
> >  		rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
> >  			      RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
> >  			      RGMII_IO_MACRO_CONFIG2);
> > +		if (ethqos->serdes_speed != SPEED_1000)
> > +			phy_set_speed(ethqos->serdes_phy, ethqos->speed);
> > +		mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE;
> >  		break;
> >  	case SPEED_100:
> >  		val |= ETHQOS_MAC_CTRL_PORT_SEL | ETHQOS_MAC_CTRL_SPEED_MODE;
> > +		if (ethqos->serdes_speed != SPEED_1000)
> > +			phy_set_speed(ethqos->serdes_phy, ethqos->speed);
> > +		mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE;
> >  		break;
> >  	case SPEED_10:
> >  		val |= ETHQOS_MAC_CTRL_PORT_SEL;
> >  		val &= ~ETHQOS_MAC_CTRL_SPEED_MODE;
> > +		if (ethqos->serdes_speed != SPEED_1000)
> > +			phy_set_speed(ethqos->serdes_phy, ethqos->speed);
> > +		mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE;
> >  		break;
> >  	}
> >  
> >  	writel(val, ethqos->mac_base + MAC_CTRL_REG);
> > +	writel(mac_an_value, ethqos->mac_base + ETHQOS_MAC_AN_CTRL);
> > +	ethqos->serdes_speed = ethqos->speed;
> 
> I see these bits are generic and there's some functions in stmmac_pcs.h
> that muck with these...
> 
> Could you help me understand if this really should be Qualcomm specific,
> or if this is something that should be considered for the more core bits
> of the driver? I feel in either case we should take advantage of the
> common definitions in that file if possible.
> 
> >  
> >  	return val;
> >  }
> > @@ -789,6 +815,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
> >  				     "Failed to get serdes phy\n");
> >  
> >  	ethqos->speed = SPEED_1000;
> > +	ethqos->serdes_speed = SPEED_1000;
> >  	ethqos_update_link_clk(ethqos, SPEED_1000);
> >  	ethqos_set_func_clk_en(ethqos);
> >  
> > -- 
> > 2.17.1
> >