RE: [PATCH net v2 2/2] net: stmmac: skip only stmmac_ptp_register when resume from suspend

[Ismail, Mohammad Athari]

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Wednesday, January 26, 2022 12:18 PM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@xxxxxxxxx>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@xxxxxx>; Alexandre Torgue
> <alexandre.torgue@xxxxxx>; Jose Abreu <joabreu@xxxxxxxxxxxx>; David
> S . Miller <davem@xxxxxxxxxxxxx>; Maxime Coquelin
> <mcoquelin.stm32@xxxxxxxxx>; Ong, Boon Leong
> <boon.leong.ong@xxxxxxxxx>; Voon, Weifeng <weifeng.voon@xxxxxxxxx>;
> Wong, Vee Khee <vee.khee.wong@xxxxxxxxx>; Huacai Chen
> <chenhuacai@xxxxxxxxxx>; Alexandre Torgue
> <alexandre.torgue@xxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-stm32@st-
> md-mailman.stormreply.com; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net v2 2/2] net: stmmac: skip only stmmac_ptp_register
> when resume from suspend
> 
> On Tue, 25 Jan 2022 11:23:24 +0800 Mohammad Athari Bin Ismail wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index d7e261768f73..b8e5e19e6f7b 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -880,11 +880,12 @@
> EXPORT_SYMBOL_GPL(stmmac_init_tstamp_counter);
> >  /**
> >   * stmmac_init_ptp - init PTP
> >   * @priv: driver private structure
> > + * @ptp_register: register PTP if set
> >   * Description: this is to verify if the HW supports the PTPv1 or PTPv2.
> >   * This is done by looking at the HW cap. register.
> >   * This function also registers the ptp driver.
> >   */
> > -static int stmmac_init_ptp(struct stmmac_priv *priv)
> > +static int stmmac_init_ptp(struct stmmac_priv *priv, bool
> > +ptp_register)
> >  {
> >  	bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
> >  	int ret;
> > @@ -914,7 +915,8 @@ static int stmmac_init_ptp(struct stmmac_priv
> *priv)
> >  	priv->hwts_tx_en = 0;
> >  	priv->hwts_rx_en = 0;
> >
> > -	stmmac_ptp_register(priv);
> > +	if (ptp_register)
> > +		stmmac_ptp_register(priv);
> 
> stmmac_init_ptp() only has one caller, and the registration step is last.
> Wouldn't it be better to move the stmmac_ptp_register() call out to
> stmmac_hw_setup()? That way we don't need to pass extra arguments to
> init.

Yes agree with you suggestion. Will fix it in v3. Thanks.

-Athari-

> 
> >  	return 0;
> >  }
> > @@ -3241,7 +3243,7 @@ static int stmmac_fpe_start_wq(struct
> > stmmac_priv *priv)
> >  /**
> >   * stmmac_hw_setup - setup mac in a usable state.
> >   *  @dev : pointer to the device structure.
> > - *  @init_ptp: initialize PTP if set
> > + *  @ptp_register: register PTP if set
> >   *  Description:
> >   *  this is the main function to setup the HW in a usable state because the
> >   *  dma engine is reset, the core registers are configured (e.g. AXI,
> > @@ -3251,7 +3253,7 @@ static int stmmac_fpe_start_wq(struct
> stmmac_priv *priv)
> >   *  0 on success and an appropriate (-)ve integer as defined in errno.h
> >   *  file on failure.
> >   */
> > -static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
> > +static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
> >  {
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> >  	u32 rx_cnt = priv->plat->rx_queues_to_use; @@ -3308,13 +3310,11
> @@
> > static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
> >
> >  	stmmac_mmc_setup(priv);
> >
> > -	if (init_ptp) {
> > -		ret = stmmac_init_ptp(priv);
> > -		if (ret == -EOPNOTSUPP)
> > -			netdev_warn(priv->dev, "PTP not supported by
> HW\n");
> > -		else if (ret)
> > -			netdev_warn(priv->dev, "PTP init failed\n");
> > -	}
> > +	ret = stmmac_init_ptp(priv, ptp_register);
> > +	if (ret == -EOPNOTSUPP)
> > +		netdev_warn(priv->dev, "PTP not supported by HW\n");
> > +	else if (ret)
> > +		netdev_warn(priv->dev, "PTP init failed\n");