[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241017122936.GF1697@kernel.org>
Date: Thu, 17 Oct 2024 13:29:36 +0100
From: Simon Horman <horms@...nel.org>
To: Furong Xu <0x1207@...il.com>
Cc: netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Vladimir Oltean <olteanv@...il.com>, Andrew Lunn <andrew@...n.ch>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Jose Abreu <joabreu@...opsys.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>, xfr@...look.com
Subject: Re: [PATCH net-next v1 5/5] net: stmmac: xgmac: Complete FPE support
On Tue, Oct 15, 2024 at 05:09:26PM +0800, Furong Xu wrote:
> FPE implementation for DWMAC4 and DWXGMAC differs only for:
> 1) Offset address of MAC_FPE_CTRL_STS and MTL_FPE_CTRL_STS
> 2) FPRQ(Frame Preemption Residue Queue) field in MAC_RxQ_Ctrl1
>
> Refactor stmmac_fpe_ops callback functions to avoid code duplication
> between gmac4 and xgmac.
>
> Signed-off-by: Furong Xu <0x1207@...il.com>
Hi Furong Xu,
I think it would be best to split this patch so that the refactor of dwmac4
code is in one patch, and adding xgmac code is in another.
...
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> index 6060a1d702c6..80f12b6e84e6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> @@ -160,41 +160,54 @@ void stmmac_fpe_apply(struct stmmac_priv *priv)
> }
> }
>
> -static void dwmac5_fpe_configure(void __iomem *ioaddr,
> - struct stmmac_fpe_cfg *cfg,
> - u32 num_txq, u32 num_rxq,
> - bool tx_enable, bool pmac_enable)
> +static void common_fpe_configure(void __iomem *ioaddr,
> + struct stmmac_fpe_cfg *cfg, u32 rxq,
> + bool tx_enable, bool pmac_enable,
> + u32 rxq_addr, u32 fprq_mask, u32 fprq_shift,
> + u32 mac_fpe_addr, u32 int_en_addr,
> + u32 int_en_bit)
This function now has a lot of parameters. Could we consider another way?
One idea I had was that describes the addresses for different chips.
...
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 75ad2da1a37f..6a79e6a111ed 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -1290,8 +1290,8 @@ const struct stmmac_tc_ops dwxgmac_tc_ops = {
> .setup_cls_u32 = tc_setup_cls_u32,
> .setup_cbs = tc_setup_cbs,
> .setup_cls = tc_setup_cls,
> - .setup_taprio = tc_setup_taprio_without_fpe,
> + .setup_taprio = tc_setup_taprio,
> .setup_etf = tc_setup_etf,
> .query_caps = tc_query_caps,
> - .setup_mqprio = tc_setup_mqprio_unimplemented,
> + .setup_mqprio = tc_setup_dwmac510_mqprio,
> };
It is not clear to me how this hunk relates to the rest of the patch.
Powered by blists - more mailing lists