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@gmail.com>
---
.../net/ethernet/stmicro/stmmac/dwxgmac2.h | 1 +
.../net/ethernet/stmicro/stmmac/stmmac_fpe.c | 149 ++++++++++++++----
.../net/ethernet/stmicro/stmmac/stmmac_tc.c | 4 +-
3 files changed, 121 insertions(+), 33 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index c66fa6040672..e1c54f3a8ee7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -96,6 +96,7 @@
#define XGMAC_LPIIS BIT(5)
#define XGMAC_PMTIS BIT(4)
#define XGMAC_INT_EN 0x000000b4
+#define XGMAC_FPEIE BIT(15)
#define XGMAC_TSIE BIT(12)
#define XGMAC_LPIIE BIT(5)
#define XGMAC_PMTIE BIT(4)
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)
{
u32 value;
if (tx_enable) {
cfg->fpe_csr = STMMAC_MAC_FPE_CTRL_STS_EFPE;
- value = readl(ioaddr + GMAC_RXQ_CTRL1);
- value &= ~GMAC_RXQCTRL_FPRQ;
- value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT;
- writel(value, ioaddr + GMAC_RXQ_CTRL1);
+ value = readl(ioaddr + rxq_addr);
+ value &= ~fprq_mask;
+ value |= (rxq - 1) << fprq_shift;
+ writel(value, ioaddr + rxq_addr);
} else {
cfg->fpe_csr = 0;
}
- writel(cfg->fpe_csr, ioaddr + GMAC5_MAC_FPE_CTRL_STS);
+ writel(cfg->fpe_csr, ioaddr + mac_fpe_addr);
- value = readl(ioaddr + GMAC_INT_EN);
+ value = readl(ioaddr + int_en_addr);
if (pmac_enable) {
- if (!(value & GMAC_INT_FPE_EN)) {
+ if (!(value & int_en_bit)) {
/* Dummy read to clear any pending masked interrupts */
- readl(ioaddr + GMAC5_MAC_FPE_CTRL_STS);
+ readl(ioaddr + mac_fpe_addr);
- value |= GMAC_INT_FPE_EN;
+ value |= int_en_bit;
}
} else {
- value &= ~GMAC_INT_FPE_EN;
+ value &= ~int_en_bit;
}
- writel(value, ioaddr + GMAC_INT_EN);
+ writel(value, ioaddr + int_en_addr);
}
-static int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
+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)
+{
+ common_fpe_configure(ioaddr, cfg, num_rxq, tx_enable,
+ pmac_enable, GMAC_RXQ_CTRL1, GMAC_RXQCTRL_FPRQ,
+ GMAC_RXQCTRL_FPRQ_SHIFT, GMAC5_MAC_FPE_CTRL_STS,
+ GMAC_INT_EN, GMAC_INT_FPE_EN);
+}
+
+static int common_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
{
u32 value;
int status;
@@ -204,7 +217,7 @@ static int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
/* Reads from the MAC_FPE_CTRL_STS register should only be performed
* here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
*/
- value = readl(ioaddr + GMAC5_MAC_FPE_CTRL_STS);
+ value = readl(ioaddr);
if (value & STMMAC_MAC_FPE_CTRL_STS_TRSP) {
status |= FPE_EVENT_TRSP;
@@ -229,7 +242,12 @@ static int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
return status;
}
-static void dwmac5_fpe_send_mpacket(void __iomem *ioaddr,
+static int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
+{
+ return common_fpe_irq_status(ioaddr + GMAC5_MAC_FPE_CTRL_STS, dev);
+}
+
+static void common_fpe_send_mpacket(void __iomem *ioaddr,
struct stmmac_fpe_cfg *cfg,
enum stmmac_mpacket_type type)
{
@@ -240,7 +258,14 @@ static void dwmac5_fpe_send_mpacket(void __iomem *ioaddr,
else if (type == MPACKET_RESPONSE)
value |= STMMAC_MAC_FPE_CTRL_STS_SRSP;
- writel(value, ioaddr + GMAC5_MAC_FPE_CTRL_STS);
+ writel(value, ioaddr);
+}
+
+static void dwmac5_fpe_send_mpacket(void __iomem *ioaddr,
+ struct stmmac_fpe_cfg *cfg,
+ enum stmmac_mpacket_type type)
+{
+ common_fpe_send_mpacket(ioaddr + GMAC5_MAC_FPE_CTRL_STS, cfg, type);
}
static int dwmac5_fpe_get_add_frag_size(const void __iomem *ioaddr)
@@ -319,26 +344,83 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr,
struct stmmac_fpe_cfg *cfg,
u32 num_txq, u32 num_rxq,
bool tx_enable, bool pmac_enable)
+{
+ common_fpe_configure(ioaddr, cfg, num_rxq, tx_enable,
+ pmac_enable, XGMAC_RXQ_CTRL1, XGMAC_FPRQ,
+ XGMAC_FPRQ_SHIFT, XGMAC_MAC_FPE_CTRL_STS,
+ XGMAC_INT_EN, XGMAC_FPEIE);
+}
+
+static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
+{
+ return common_fpe_irq_status(ioaddr + XGMAC_MAC_FPE_CTRL_STS, dev);
+}
+
+static void dwxgmac3_fpe_send_mpacket(void __iomem *ioaddr,
+ struct stmmac_fpe_cfg *cfg,
+ enum stmmac_mpacket_type type)
+{
+ common_fpe_send_mpacket(ioaddr + XGMAC_MAC_FPE_CTRL_STS, cfg, type);
+}
+
+static int dwxgmac3_fpe_get_add_frag_size(const void __iomem *ioaddr)
+{
+ return FIELD_GET(FPE_MTL_ADD_FRAG_SZ,
+ readl(ioaddr + XGMAC_MTL_FPE_CTRL_STS));
+}
+
+static void dwxgmac3_fpe_set_add_frag_size(void __iomem *ioaddr,
+ u32 add_frag_size)
{
u32 value;
- if (!tx_enable) {
- value = readl(ioaddr + XGMAC_MAC_FPE_CTRL_STS);
+ value = readl(ioaddr + XGMAC_MTL_FPE_CTRL_STS);
+ writel(u32_replace_bits(value, add_frag_size, FPE_MTL_ADD_FRAG_SZ),
+ ioaddr + XGMAC_MTL_FPE_CTRL_STS);
+}
- value &= ~STMMAC_MAC_FPE_CTRL_STS_EFPE;
+static int dwxgmac3_fpe_map_preemption_class(struct net_device *ndev,
+ struct netlink_ext_ack *extack,
+ u32 pclass)
+{
+ u32 val, offset, count, preemptible_txqs = 0;
+ struct stmmac_priv *priv = netdev_priv(ndev);
+ u32 num_tc = ndev->num_tc;
- writel(value, ioaddr + XGMAC_MAC_FPE_CTRL_STS);
- return;
+ if (!num_tc) {
+ /* Restore default TC:Queue mapping */
+ for (u32 i = 0; i < priv->plat->tx_queues_to_use; i++) {
+ val = readl(priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(i));
+ writel(u32_replace_bits(val, i, XGMAC_Q2TCMAP),
+ priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(i));
+ }
}
- value = readl(ioaddr + XGMAC_RXQ_CTRL1);
- value &= ~XGMAC_FPRQ;
- value |= (num_rxq - 1) << XGMAC_FPRQ_SHIFT;
- writel(value, ioaddr + XGMAC_RXQ_CTRL1);
+ /* Synopsys Databook:
+ * "All Queues within a traffic class are selected in a round robin
+ * fashion (when packets are available) when the traffic class is
+ * selected by the scheduler for packet transmission. This is true for
+ * any of the scheduling algorithms."
+ */
+ for (u32 tc = 0; tc < num_tc; tc++) {
+ count = ndev->tc_to_txq[tc].count;
+ offset = ndev->tc_to_txq[tc].offset;
+
+ if (pclass & BIT(tc))
+ preemptible_txqs |= GENMASK(offset + count - 1, offset);
- value = readl(ioaddr + XGMAC_MAC_FPE_CTRL_STS);
- value |= STMMAC_MAC_FPE_CTRL_STS_EFPE;
- writel(value, ioaddr + XGMAC_MAC_FPE_CTRL_STS);
+ for (u32 i = 0; i < count; i++) {
+ val = readl(priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(offset + i));
+ writel(u32_replace_bits(val, tc, XGMAC_Q2TCMAP),
+ priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(offset + i));
+ }
+ }
+
+ val = readl(priv->ioaddr + XGMAC_MTL_FPE_CTRL_STS);
+ writel(u32_replace_bits(val, preemptible_txqs, FPE_MTL_PREEMPTION_CLASS),
+ priv->ioaddr + XGMAC_MTL_FPE_CTRL_STS);
+
+ return 0;
}
const struct stmmac_fpe_ops dwmac5_fpe_ops = {
@@ -352,4 +434,9 @@ const struct stmmac_fpe_ops dwmac5_fpe_ops = {
const struct stmmac_fpe_ops dwxgmac_fpe_ops = {
.fpe_configure = dwxgmac3_fpe_configure,
+ .fpe_send_mpacket = dwxgmac3_fpe_send_mpacket,
+ .fpe_irq_status = dwxgmac3_fpe_irq_status,
+ .fpe_get_add_frag_size = dwxgmac3_fpe_get_add_frag_size,
+ .fpe_set_add_frag_size = dwxgmac3_fpe_set_add_frag_size,
+ .fpe_map_preemption_class = dwxgmac3_fpe_map_preemption_class,
};
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,
};
--
2.34.1On Tue, Oct 15, 2024 at 05:09:26PM +0800, Furong Xu wrote:
> 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)
11 arguments to a function is a bit too much. Could you introduce a
structure with FPE constants per hardware IP, and just pass a pointer to
that?
> {
> u32 value;
>
> - writel(value, ioaddr + XGMAC_MAC_FPE_CTRL_STS);
> - return;
> + if (!num_tc) {
> + /* Restore default TC:Queue mapping */
> + for (u32 i = 0; i < priv->plat->tx_queues_to_use; i++) {
> + val = readl(priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(i));
> + writel(u32_replace_bits(val, i, XGMAC_Q2TCMAP),
> + priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(i));
> + }
> }
>
> - value = readl(ioaddr + XGMAC_RXQ_CTRL1);
> - value &= ~XGMAC_FPRQ;
> - value |= (num_rxq - 1) << XGMAC_FPRQ_SHIFT;
> - writel(value, ioaddr + XGMAC_RXQ_CTRL1);
> + /* Synopsys Databook:
> + * "All Queues within a traffic class are selected in a round robin
> + * fashion (when packets are available) when the traffic class is
> + * selected by the scheduler for packet transmission. This is true for
> + * any of the scheduling algorithms."
> + */
> + for (u32 tc = 0; tc < num_tc; tc++) {
> + count = ndev->tc_to_txq[tc].count;
> + offset = ndev->tc_to_txq[tc].offset;
> +
> + if (pclass & BIT(tc))
> + preemptible_txqs |= GENMASK(offset + count - 1, offset);
>
> - value = readl(ioaddr + XGMAC_MAC_FPE_CTRL_STS);
> - value |= STMMAC_MAC_FPE_CTRL_STS_EFPE;
> - writel(value, ioaddr + XGMAC_MAC_FPE_CTRL_STS);
> + for (u32 i = 0; i < count; i++) {
> + val = readl(priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(offset + i));
> + writel(u32_replace_bits(val, tc, XGMAC_Q2TCMAP),
> + priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(offset + i));
> + }
> + }
I agree with Simon that this patch is hard to review. The diff looks
like a jungle here, the portion with - has nothing to do with the
portion with +. Please try to do as suggested, first refactor existing
code into the common stuff, then call common stuff from new places.
Also try to keep an eye on how things look in git diff, and splitting
even further if it gets messy.
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@gmail.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.
© 2016 - 2024 Red Hat, Inc.