On Fri, 9 Oct 2020 00:11:23 +0800 Voon Weifeng wrote:
> From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kuma...@intel.com>
>
> This patch enables the HW LPI Timer which controls the automatic entry
> and exit of the LPI state.
> The EEE LPI timer value is configured through ethtool. The driver will
> auto select the LPI HW timer if the value in the HW timer supported range.
> Else, the driver will fallback to SW timer.
>
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kuma...@intel.com>
> Signed-off-by: Voon Weifeng <weifeng.v...@intel.com>
minor nits, but the patch makes sense to me
please repost soon so it makes it into 5.10
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h
> b/drivers/net/ethernet/stmicro/stmmac/common.h
> index df7de50497a0..f59c4a1c1674 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -402,6 +402,9 @@ struct dma_features {
> /* Default LPI timers */
> #define STMMAC_DEFAULT_LIT_LS 0x3E8
> #define STMMAC_DEFAULT_TWT_LS 0x1E
> +#define STMMAC_ET_MAX 0xFFFFF
> +#define LPI_ET_ENABLE 1
> +#define LPI_ET_DISABLE 0
Don't think you need defines for true / false values like that.
Just use literals.
>
> #define STMMAC_CHAIN_MODE 0x1
> #define STMMAC_RING_MODE 0x2
> @@ -268,6 +269,7 @@ int stmmac_dvr_probe(struct device *device,
> struct stmmac_resources *res);
> void stmmac_disable_eee_mode(struct stmmac_priv *priv);
> bool stmmac_eee_init(struct stmmac_priv *priv);
> +void stmmac_lpi_entry_timer_enable(struct stmmac_priv *priv, bool en);
This doesn't have to be declared in the header...
> int stmmac_reinit_queues(struct net_device *dev, u32 rx_cnt, u32 tx_cnt);
> int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32
> tx_size);
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 220626a8d499..908df3cb12cd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -327,6 +327,11 @@ static void stmmac_enable_eee_mode(struct stmmac_priv
> *priv)
> */
> void stmmac_disable_eee_mode(struct stmmac_priv *priv)
> {
> + if (!priv->eee_sw_timer_en) {
> + stmmac_lpi_entry_timer_enable(priv, LPI_ET_DISABLE);
> + return;
> + }
> +
> stmmac_reset_eee_mode(priv, priv->hw);
> del_timer_sync(&priv->eee_ctrl_timer);
> priv->tx_path_in_lpi_mode = false;
> @@ -347,6 +352,16 @@ static void stmmac_eee_ctrl_timer(struct timer_list *t)
> mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(priv->tx_lpi_timer));
> }
>
> +void stmmac_lpi_entry_timer_enable(struct stmmac_priv *priv, bool en)
> +{
Just move this function up in the file so it's defined before
stmmac_disable_eee_mode(). We prefer to limit forward declarations
in the kernel.
Also the name of the function would probably be better as
..._timer_config() or such. I find it strange to call a function called
enable() to disable something.
> + int tx_lpi_timer;
> +
> + /* Clear/set the SW EEE timer flag based on LPI ET enablement */
> + priv->eee_sw_timer_en = en ? 0 : 1;
> + tx_lpi_timer = en ? priv->tx_lpi_timer : 0;
> + stmmac_set_eee_lpi_timer(priv, priv->hw, tx_lpi_timer);
> +}
