[PATCH net-next v4 0/9] net: stmmac: add new features to xgmac

[PATCH net-next v4 0/9] net: stmmac: add new features to xgmac

[Jisheng Zhang]

This series add below new features to xgmac:

correct RX COE parsing
add more feature parsing from hw cap
enlarge C22 ADDR and rx/tx channels
support parse safety ce/ue irq from DT
support per channel irq

Since v3:
 - collect Acked-by tag
 - remove patch which enlarges the max XGMAC C22 ADDR to 31 since it's
   merged
 - s/stmmac_request_irq_multi/stmmac_request_irq_multi_channel
 - update the dt-binding to refelct the optional per-channel irq:
     - use enum
     - add additionalItems and maxItems to fix the "interupt-names ..
       is too long"

Since v2:
 - check per channel irq by (res->rx_irq[0] > 0 && res->tx_irq[0] > 0)
   rather than (res->rx_irq[0] && res->tx_irq[0])
 - bypass if (irq <= 0) when request rx/tx irq

Since v1:
 - remove "_irq" suffix from safety irqs dt binding
 - remove "snps,per-channel-interrupt" dt binding, check the channel irq
   instead.
 - more renaming about "msi" to reflect per channel irq isn't MSI
   specific


Jisheng Zhang (9):
  net: stmmac: correct RX COE parsing for xgmac
  net: stmmac: xgmac: add more feature parsing from hw cap
  net: stmmac: enlarge max rx/tx queues and channels to 16
  net: stmmac: reflect multi irqs for tx/rx channels and mac and safety
  net: stmmac: xgmac: support per-channel irq
  dt-bindings: net: snps,dwmac: add safety irq support
  net: stmmac: platform: support parsing safety irqs from DT
  dt-bindings: net: snps,dwmac: add per channel irq support
  net: stmmac: platform: support parsing per channel irq from DT

 .../devicetree/bindings/net/snps,dwmac.yaml   | 77 ++++++++++++++++++-
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c |  4 +-
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  2 +
 .../ethernet/stmicro/stmmac/dwxgmac2_core.c   |  5 +-
 .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    | 34 ++++----
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 56 +++++++-------
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 35 +++++++++
 include/linux/stmmac.h                        | 10 +--
 9 files changed, 172 insertions(+), 53 deletions(-)

-- 
2.40.1

[PATCH net-next v4 1/9] net: stmmac: correct RX COE parsing for xgmac

[Jisheng Zhang]

xgmac can support RX COE, but there's no two kinds of COE, I.E type 1
and type 2 COE.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Acked-by: Alexandre TORGUE <alexandre.torgue@foss.st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 733b5e900817..3d90ca983389 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7035,7 +7035,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 	if (priv->plat->rx_coe) {
 		priv->hw->rx_csum = priv->plat->rx_coe;
 		dev_info(priv->device, "RX Checksum Offload Engine supported\n");
-		if (priv->synopsys_id < DWMAC_CORE_4_00)
+		if (priv->synopsys_id < DWMAC_CORE_4_00 && !priv->plat->has_xgmac)
 			dev_info(priv->device, "COE Type %d\n", priv->hw->rx_csum);
 	}
 	if (priv->plat->tx_coe)
-- 
2.40.1

[PATCH net-next v4 2/9] net: stmmac: xgmac: add more feature parsing from hw cap

[Jisheng Zhang]

The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps
or not.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Acked-by: Alexandre TORGUE <alexandre.torgue@foss.st.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
index 3aacf791efeb..1ef8fc132c2d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
@@ -410,6 +410,7 @@ static int dwxgmac2_get_hw_feature(void __iomem *ioaddr,
 	dma_cap->vlhash = (hw_cap & XGMAC_HWFEAT_VLHASH) >> 4;
 	dma_cap->half_duplex = (hw_cap & XGMAC_HWFEAT_HDSEL) >> 3;
 	dma_cap->mbps_1000 = (hw_cap & XGMAC_HWFEAT_GMIISEL) >> 1;
+	dma_cap->mbps_10_100 = (hw_cap & XGMAC_HWFEAT_GMIISEL) >> 1;
 
 	/* MAC HW feature 1 */
 	hw_cap = readl(ioaddr + XGMAC_HW_FEATURE1);
-- 
2.40.1

[PATCH net-next v4 3/9] net: stmmac: enlarge max rx/tx queues and channels to 16

[Jisheng Zhang]

xgmac supports up to 16 rx/tx queues and up to 16 channels.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Acked-by: Alexandre TORGUE <alexandre.torgue@foss.st.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 5 ++---
 include/linux/stmmac.h                              | 6 +++---
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index 38782662ff98..8ac994553bc1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -232,9 +232,8 @@ static void dwxgmac2_map_mtl_to_dma(struct mac_device_info *hw, u32 queue,
 	void __iomem *ioaddr = hw->pcsr;
 	u32 value, reg;
 
-	reg = (queue < 4) ? XGMAC_MTL_RXQ_DMA_MAP0 : XGMAC_MTL_RXQ_DMA_MAP1;
-	if (queue >= 4)
-		queue -= 4;
+	reg = XGMAC_MTL_RXQ_DMA_MAP0 + (queue & ~0x3);
+	queue &= 0x3;
 
 	value = readl(ioaddr + reg);
 	value &= ~XGMAC_QxMDMACH(queue);
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 784277d666eb..9c90e2e295d4 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -15,9 +15,9 @@
 #include <linux/platform_device.h>
 #include <linux/phy.h>
 
-#define MTL_MAX_RX_QUEUES	8
-#define MTL_MAX_TX_QUEUES	8
-#define STMMAC_CH_MAX		8
+#define MTL_MAX_RX_QUEUES	16
+#define MTL_MAX_TX_QUEUES	16
+#define STMMAC_CH_MAX		16
 
 #define STMMAC_RX_COE_NONE	0
 #define STMMAC_RX_COE_TYPE1	1
-- 
2.40.1

[PATCH net-next v4 4/9] net: stmmac: reflect multi irqs for tx/rx channels and mac and safety

[Jisheng Zhang]

The IP supports per channel interrupt, when intel adds the per channel
interrupt support, the per channel irq is from MSI vector, but this
feature can also be supported on non-MSI platforms. Do some necessary
renaming to reflects this fact.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c |  4 +-
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 48 +++++++++----------
 include/linux/stmmac.h                        |  4 +-
 4 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 979c755964b1..9050de31ed76 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -952,7 +952,7 @@ static int stmmac_config_single_msi(struct pci_dev *pdev,
 
 	res->irq = pci_irq_vector(pdev, 0);
 	res->wol_irq = res->irq;
-	plat->flags &= ~STMMAC_FLAG_MULTI_MSI_EN;
+	plat->flags &= ~STMMAC_FLAG_PERCH_IRQ_EN;
 	dev_info(&pdev->dev, "%s: Single IRQ enablement successful\n",
 		 __func__);
 
@@ -1004,7 +1004,7 @@ static int stmmac_config_multi_msi(struct pci_dev *pdev,
 	if (plat->msi_sfty_ue_vec < STMMAC_MSI_VEC_MAX)
 		res->sfty_ue_irq = pci_irq_vector(pdev, plat->msi_sfty_ue_vec);
 
-	plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
+	plat->flags |= STMMAC_FLAG_PERCH_IRQ_EN;
 	dev_info(&pdev->dev, "%s: multi MSI enablement successful\n", __func__);
 
 	return 0;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index 84d3a8551b03..9bf8adf466a2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -175,7 +175,7 @@ static void dwmac4_dma_init(void __iomem *ioaddr,
 
 	value = readl(ioaddr + DMA_BUS_MODE);
 
-	if (dma_cfg->multi_msi_en) {
+	if (dma_cfg->perch_irq_en) {
 		value &= ~DMA_BUS_MODE_INTM_MASK;
 		value |= (DMA_BUS_MODE_INTM_MODE1 << DMA_BUS_MODE_INTM_SHIFT);
 	}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3d90ca983389..64c55024d69d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -126,11 +126,11 @@ module_param(chain_mode, int, 0444);
 MODULE_PARM_DESC(chain_mode, "To use chain instead of ring mode");
 
 static irqreturn_t stmmac_interrupt(int irq, void *dev_id);
-/* For MSI interrupts handling */
+/* For multi channel interrupts handling */
 static irqreturn_t stmmac_mac_interrupt(int irq, void *dev_id);
 static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id);
-static irqreturn_t stmmac_msi_intr_tx(int irq, void *data);
-static irqreturn_t stmmac_msi_intr_rx(int irq, void *data);
+static irqreturn_t stmmac_queue_intr_tx(int irq, void *data);
+static irqreturn_t stmmac_queue_intr_rx(int irq, void *data);
 static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue);
 static void stmmac_reset_tx_queue(struct stmmac_priv *priv, u32 queue);
 static void stmmac_reset_queues_param(struct stmmac_priv *priv);
@@ -3520,7 +3520,7 @@ static void stmmac_free_irq(struct net_device *dev,
 	}
 }
 
-static int stmmac_request_irq_multi_msi(struct net_device *dev)
+static int stmmac_request_irq_multi_channel(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 	enum request_irq_err irq_err;
@@ -3537,7 +3537,7 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 			  0, int_name, dev);
 	if (unlikely(ret < 0)) {
 		netdev_err(priv->dev,
-			   "%s: alloc mac MSI %d (error: %d)\n",
+			   "%s: alloc mac irq %d (error: %d)\n",
 			   __func__, dev->irq, ret);
 		irq_err = REQ_IRQ_ERR_MAC;
 		goto irq_error;
@@ -3554,7 +3554,7 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 				  0, int_name, dev);
 		if (unlikely(ret < 0)) {
 			netdev_err(priv->dev,
-				   "%s: alloc wol MSI %d (error: %d)\n",
+				   "%s: alloc wol irq %d (error: %d)\n",
 				   __func__, priv->wol_irq, ret);
 			irq_err = REQ_IRQ_ERR_WOL;
 			goto irq_error;
@@ -3572,7 +3572,7 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 				  0, int_name, dev);
 		if (unlikely(ret < 0)) {
 			netdev_err(priv->dev,
-				   "%s: alloc lpi MSI %d (error: %d)\n",
+				   "%s: alloc lpi irq %d (error: %d)\n",
 				   __func__, priv->lpi_irq, ret);
 			irq_err = REQ_IRQ_ERR_LPI;
 			goto irq_error;
@@ -3590,7 +3590,7 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 				  0, int_name, dev);
 		if (unlikely(ret < 0)) {
 			netdev_err(priv->dev,
-				   "%s: alloc sfty ce MSI %d (error: %d)\n",
+				   "%s: alloc sfty ce irq %d (error: %d)\n",
 				   __func__, priv->sfty_ce_irq, ret);
 			irq_err = REQ_IRQ_ERR_SFTY_CE;
 			goto irq_error;
@@ -3608,14 +3608,14 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 				  0, int_name, dev);
 		if (unlikely(ret < 0)) {
 			netdev_err(priv->dev,
-				   "%s: alloc sfty ue MSI %d (error: %d)\n",
+				   "%s: alloc sfty ue irq %d (error: %d)\n",
 				   __func__, priv->sfty_ue_irq, ret);
 			irq_err = REQ_IRQ_ERR_SFTY_UE;
 			goto irq_error;
 		}
 	}
 
-	/* Request Rx MSI irq */
+	/* Request Rx queue irq */
 	for (i = 0; i < priv->plat->rx_queues_to_use; i++) {
 		if (i >= MTL_MAX_RX_QUEUES)
 			break;
@@ -3625,11 +3625,11 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 		int_name = priv->int_name_rx_irq[i];
 		sprintf(int_name, "%s:%s-%d", dev->name, "rx", i);
 		ret = request_irq(priv->rx_irq[i],
-				  stmmac_msi_intr_rx,
+				  stmmac_queue_intr_rx,
 				  0, int_name, &priv->dma_conf.rx_queue[i]);
 		if (unlikely(ret < 0)) {
 			netdev_err(priv->dev,
-				   "%s: alloc rx-%d  MSI %d (error: %d)\n",
+				   "%s: alloc rx-%d irq %d (error: %d)\n",
 				   __func__, i, priv->rx_irq[i], ret);
 			irq_err = REQ_IRQ_ERR_RX;
 			irq_idx = i;
@@ -3640,7 +3640,7 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 		irq_set_affinity_hint(priv->rx_irq[i], &cpu_mask);
 	}
 
-	/* Request Tx MSI irq */
+	/* Request Tx queue irq */
 	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
 		if (i >= MTL_MAX_TX_QUEUES)
 			break;
@@ -3650,11 +3650,11 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 		int_name = priv->int_name_tx_irq[i];
 		sprintf(int_name, "%s:%s-%d", dev->name, "tx", i);
 		ret = request_irq(priv->tx_irq[i],
-				  stmmac_msi_intr_tx,
+				  stmmac_queue_intr_tx,
 				  0, int_name, &priv->dma_conf.tx_queue[i]);
 		if (unlikely(ret < 0)) {
 			netdev_err(priv->dev,
-				   "%s: alloc tx-%d  MSI %d (error: %d)\n",
+				   "%s: alloc tx-%d irq %d (error: %d)\n",
 				   __func__, i, priv->tx_irq[i], ret);
 			irq_err = REQ_IRQ_ERR_TX;
 			irq_idx = i;
@@ -3729,8 +3729,8 @@ static int stmmac_request_irq(struct net_device *dev)
 	int ret;
 
 	/* Request the IRQ lines */
-	if (priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN)
-		ret = stmmac_request_irq_multi_msi(dev);
+	if (priv->plat->flags & STMMAC_FLAG_PERCH_IRQ_EN)
+		ret = stmmac_request_irq_multi_channel(dev);
 	else
 		ret = stmmac_request_irq_single(dev);
 
@@ -5945,7 +5945,7 @@ static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
+static irqreturn_t stmmac_queue_intr_tx(int irq, void *data)
 {
 	struct stmmac_tx_queue *tx_q = (struct stmmac_tx_queue *)data;
 	struct stmmac_dma_conf *dma_conf;
@@ -5977,7 +5977,7 @@ static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t stmmac_msi_intr_rx(int irq, void *data)
+static irqreturn_t stmmac_queue_intr_rx(int irq, void *data)
 {
 	struct stmmac_rx_queue *rx_q = (struct stmmac_rx_queue *)data;
 	struct stmmac_dma_conf *dma_conf;
@@ -6014,12 +6014,12 @@ static void stmmac_poll_controller(struct net_device *dev)
 	if (test_bit(STMMAC_DOWN, &priv->state))
 		return;
 
-	if (priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN) {
+	if (priv->plat->flags & STMMAC_FLAG_PERCH_IRQ_EN) {
 		for (i = 0; i < priv->plat->rx_queues_to_use; i++)
-			stmmac_msi_intr_rx(0, &priv->dma_conf.rx_queue[i]);
+			stmmac_queue_intr_rx(0, &priv->dma_conf.rx_queue[i]);
 
 		for (i = 0; i < priv->plat->tx_queues_to_use; i++)
-			stmmac_msi_intr_tx(0, &priv->dma_conf.tx_queue[i]);
+			stmmac_queue_intr_tx(0, &priv->dma_conf.tx_queue[i]);
 	} else {
 		disable_irq(dev->irq);
 		stmmac_interrupt(dev->irq, dev);
@@ -7300,8 +7300,8 @@ int stmmac_dvr_probe(struct device *device,
 	priv->plat = plat_dat;
 	priv->ioaddr = res->addr;
 	priv->dev->base_addr = (unsigned long)res->addr;
-	priv->plat->dma_cfg->multi_msi_en =
-		(priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN);
+	priv->plat->dma_cfg->perch_irq_en =
+		(priv->plat->flags & STMMAC_FLAG_PERCH_IRQ_EN);
 
 	priv->dev->irq = res->irq;
 	priv->wol_irq = res->wol_irq;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 9c90e2e295d4..c052c222fa3e 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -98,7 +98,7 @@ struct stmmac_dma_cfg {
 	int mixed_burst;
 	bool aal;
 	bool eame;
-	bool multi_msi_en;
+	bool perch_irq_en;
 	bool dche;
 };
 
@@ -213,7 +213,7 @@ struct dwmac4_addrs {
 #define STMMAC_FLAG_TSO_EN			BIT(4)
 #define STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP	BIT(5)
 #define STMMAC_FLAG_VLAN_FAIL_Q_EN		BIT(6)
-#define STMMAC_FLAG_MULTI_MSI_EN		BIT(7)
+#define STMMAC_FLAG_PERCH_IRQ_EN		BIT(7)
 #define STMMAC_FLAG_EXT_SNAPSHOT_EN		BIT(8)
 #define STMMAC_FLAG_INT_SNAPSHOT_EN		BIT(9)
 #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI		BIT(10)
-- 
2.40.1

[PATCH net-next v4 5/9] net: stmmac: xgmac: support per-channel irq

[Jisheng Zhang]

The IP supports per channel interrupt, add support for this usage case.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  2 ++
 .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    | 33 +++++++++++--------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index 7f68bef456b7..18a042834d75 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -340,6 +340,8 @@
 
 /* DMA Registers */
 #define XGMAC_DMA_MODE			0x00003000
+#define XGMAC_INTM			GENMASK(13, 12)
+#define XGMAC_INTM_MODE1		0x1
 #define XGMAC_SWR			BIT(0)
 #define XGMAC_DMA_SYSBUS_MODE		0x00003004
 #define XGMAC_WR_OSR_LMT		GENMASK(29, 24)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
index 1ef8fc132c2d..ce228c362403 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
@@ -31,6 +31,13 @@ static void dwxgmac2_dma_init(void __iomem *ioaddr,
 		value |= XGMAC_EAME;
 
 	writel(value, ioaddr + XGMAC_DMA_SYSBUS_MODE);
+
+	if (dma_cfg->perch_irq_en) {
+		value = readl(ioaddr + XGMAC_DMA_MODE);
+		value &= ~XGMAC_INTM;
+		value |= FIELD_PREP(XGMAC_INTM, XGMAC_INTM_MODE1);
+		writel(value, ioaddr + XGMAC_DMA_MODE);
+	}
 }
 
 static void dwxgmac2_dma_init_chan(struct stmmac_priv *priv,
@@ -365,20 +372,20 @@ static int dwxgmac2_dma_interrupt(struct stmmac_priv *priv,
 	}
 
 	/* TX/RX NORMAL interrupts */
-	if (likely(intr_status & XGMAC_NIS)) {
-		if (likely(intr_status & XGMAC_RI)) {
-			u64_stats_update_begin(&rx_q->rxq_stats.syncp);
-			rx_q->rxq_stats.rx_normal_irq_n++;
-			u64_stats_update_end(&rx_q->rxq_stats.syncp);
-			ret |= handle_rx;
-		}
-		if (likely(intr_status & (XGMAC_TI | XGMAC_TBU))) {
-			u64_stats_update_begin(&tx_q->txq_stats.syncp);
-			tx_q->txq_stats.tx_normal_irq_n++;
-			u64_stats_update_end(&tx_q->txq_stats.syncp);
-			ret |= handle_tx;
-		}
+	if (likely(intr_status & XGMAC_RI)) {
+		u64_stats_update_begin(&rx_q->rxq_stats.syncp);
+		rx_q->rxq_stats.rx_normal_irq_n++;
+		u64_stats_update_end(&rx_q->rxq_stats.syncp);
+		ret |= handle_rx;
+	}
+	if (likely(intr_status & XGMAC_TI)) {
+		u64_stats_update_begin(&tx_q->txq_stats.syncp);
+		tx_q->txq_stats.tx_normal_irq_n++;
+		u64_stats_update_end(&tx_q->txq_stats.syncp);
+		ret |= handle_tx;
 	}
+	if (unlikely(intr_status & XGMAC_TBU))
+		ret |= handle_tx;
 
 	/* Clear interrupts */
 	writel(intr_en & intr_status, ioaddr + XGMAC_DMA_CH_STATUS(chan));
-- 
2.40.1

[PATCH net-next v4 6/9] dt-bindings: net: snps,dwmac: add safety irq support

[Jisheng Zhang]

The snps dwmac IP support safety features, and those Safety Feature
Correctible Error and Uncorrectible Error irqs may be separate irqs.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 Documentation/devicetree/bindings/net/snps,dwmac.yaml | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index ddf9522a5dc2..b8d01bba6e1a 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -107,13 +107,18 @@ properties:
       - description: Combined signal for various interrupt events
       - description: The interrupt to manage the remote wake-up packet detection
       - description: The interrupt that occurs when Rx exits the LPI state
+      - description: The interrupt that occurs when Safety Feature Correctible Errors happen
+      - description: The interrupt that occurs when Safety Feature Uncorrectible Errors happen
 
   interrupt-names:
     minItems: 1
     items:
       - const: macirq
-      - enum: [eth_wake_irq, eth_lpi]
-      - const: eth_lpi
+      - enum:
+          - eth_wake_irq
+          - eth_lpi
+          - sfty_ce
+          - sfty_ue
 
   clocks:
     minItems: 1
-- 
2.40.1

[PATCH net-next v4 7/9] net: stmmac: platform: support parsing safety irqs from DT

[Jisheng Zhang]

The snps dwmac IP may support safety features, and those Safety
Feature Correctible Error and Uncorrectible Error irqs may be
separate irqs. Add support to parse the safety irqs from DT.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Acked-by: Alexandre TORGUE <alexandre.torgue@foss.st.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c    | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index be8e79c7aa34..4a2002eea870 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -737,6 +737,18 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
 		dev_info(&pdev->dev, "IRQ eth_lpi not found\n");
 	}
 
+	stmmac_res->sfty_ce_irq = platform_get_irq_byname_optional(pdev, "sfty_ce");
+	if (stmmac_res->sfty_ce_irq < 0) {
+		if (stmmac_res->sfty_ce_irq == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+	}
+
+	stmmac_res->sfty_ue_irq = platform_get_irq_byname_optional(pdev, "sfty_ue");
+	if (stmmac_res->sfty_ue_irq < 0) {
+		if (stmmac_res->sfty_ue_irq == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+	}
+
 	stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0);
 
 	return PTR_ERR_OR_ZERO(stmmac_res->addr);
-- 
2.40.1

[PATCH net-next v4 8/9] dt-bindings: net: snps,dwmac: add per channel irq support

[Jisheng Zhang]

The IP supports optional per channel interrupt, add support for this
usage case.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 .../devicetree/bindings/net/snps,dwmac.yaml   | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index b8d01bba6e1a..a916701474dc 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -103,15 +103,51 @@ properties:
 
   interrupts:
     minItems: 1
+    maxItems: 37
+    additionalItems: true
     items:
       - description: Combined signal for various interrupt events
       - description: The interrupt to manage the remote wake-up packet detection
       - description: The interrupt that occurs when Rx exits the LPI state
       - description: The interrupt that occurs when Safety Feature Correctible Errors happen
       - description: The interrupt that occurs when Safety Feature Uncorrectible Errors happen
+      - description: rx0 per-channel interrupt
+      - description: rx1 per-channel interrupt
+      - description: rx2 per-channel interrupt
+      - description: rx3 per-channel interrupt
+      - description: rx4 per-channel interrupt
+      - description: rx5 per-channel interrupt
+      - description: rx6 per-channel interrupt
+      - description: rx7 per-channel interrupt
+      - description: rx8 per-channel interrupt
+      - description: rx9 per-channel interrupt
+      - description: rx10 per-channel interrupt
+      - description: rx11 per-channel interrupt
+      - description: rx12 per-channel interrupt
+      - description: rx13 per-channel interrupt
+      - description: rx14 per-channel interrupt
+      - description: rx15 per-channel interrupt
+      - description: tx0 per-channel interrupt
+      - description: tx1 per-channel interrupt
+      - description: tx2 per-channel interrupt
+      - description: tx3 per-channel interrupt
+      - description: tx4 per-channel interrupt
+      - description: tx5 per-channel interrupt
+      - description: tx6 per-channel interrupt
+      - description: tx7 per-channel interrupt
+      - description: tx8 per-channel interrupt
+      - description: tx9 per-channel interrupt
+      - description: tx10 per-channel interrupt
+      - description: tx11 per-channel interrupt
+      - description: tx12 per-channel interrupt
+      - description: tx13 per-channel interrupt
+      - description: tx14 per-channel interrupt
+      - description: tx15 per-channel interrupt
 
   interrupt-names:
     minItems: 1
+    maxItems: 37
+    additionalItems: true
     items:
       - const: macirq
       - enum:
@@ -119,6 +155,38 @@ properties:
           - eth_lpi
           - sfty_ce
           - sfty_ue
+          - rx0
+          - rx1
+          - rx2
+          - rx3
+          - rx4
+          - rx5
+          - rx6
+          - rx7
+          - rx8
+          - rx9
+          - rx10
+          - rx11
+          - rx12
+          - rx13
+          - rx14
+          - rx15
+          - tx0
+          - tx1
+          - tx2
+          - tx3
+          - tx4
+          - tx5
+          - tx6
+          - tx7
+          - tx8
+          - tx9
+          - tx10
+          - tx11
+          - tx12
+          - tx13
+          - tx14
+          - tx15
 
   clocks:
     minItems: 1
-- 
2.40.1

[PATCH net-next v4 9/9] net: stmmac: platform: support parsing per channel irq from DT

[Jisheng Zhang]

The snps dwmac IP may support per channel interrupt. Add support to
parse the per channel irq from DT.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++----
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 23 +++++++++++++++++++
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 64c55024d69d..d4a8d7b48ad2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3619,7 +3619,7 @@ static int stmmac_request_irq_multi_channel(struct net_device *dev)
 	for (i = 0; i < priv->plat->rx_queues_to_use; i++) {
 		if (i >= MTL_MAX_RX_QUEUES)
 			break;
-		if (priv->rx_irq[i] == 0)
+		if (priv->rx_irq[i] <= 0)
 			continue;
 
 		int_name = priv->int_name_rx_irq[i];
@@ -3644,7 +3644,7 @@ static int stmmac_request_irq_multi_channel(struct net_device *dev)
 	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
 		if (i >= MTL_MAX_TX_QUEUES)
 			break;
-		if (priv->tx_irq[i] == 0)
+		if (priv->tx_irq[i] <= 0)
 			continue;
 
 		int_name = priv->int_name_tx_irq[i];
@@ -7300,8 +7300,10 @@ int stmmac_dvr_probe(struct device *device,
 	priv->plat = plat_dat;
 	priv->ioaddr = res->addr;
 	priv->dev->base_addr = (unsigned long)res->addr;
-	priv->plat->dma_cfg->perch_irq_en =
-		(priv->plat->flags & STMMAC_FLAG_PERCH_IRQ_EN);
+	if (res->rx_irq[0] > 0 && res->tx_irq[0] > 0) {
+		priv->plat->flags |= STMMAC_FLAG_PERCH_IRQ_EN;
+		priv->plat->dma_cfg->perch_irq_en = true;
+	}
 
 	priv->dev->irq = res->irq;
 	priv->wol_irq = res->wol_irq;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 4a2002eea870..0fb9868aeffc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -704,6 +704,9 @@ EXPORT_SYMBOL_GPL(stmmac_remove_config_dt);
 int stmmac_get_platform_resources(struct platform_device *pdev,
 				  struct stmmac_resources *stmmac_res)
 {
+	char irq_name[8];
+	int i;
+
 	memset(stmmac_res, 0, sizeof(*stmmac_res));
 
 	/* Get IRQ information early to have an ability to ask for deferred
@@ -737,6 +740,26 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
 		dev_info(&pdev->dev, "IRQ eth_lpi not found\n");
 	}
 
+	for (i = 0; i < MTL_MAX_RX_QUEUES; i++) {
+		snprintf(irq_name, sizeof(irq_name), "rx%i", i);
+		stmmac_res->rx_irq[i] = platform_get_irq_byname_optional(pdev, irq_name);
+		if (stmmac_res->rx_irq[i] < 0) {
+			if (stmmac_res->rx_irq[i] == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			break;
+		}
+	}
+
+	for (i = 0; i < MTL_MAX_TX_QUEUES; i++) {
+		snprintf(irq_name, sizeof(irq_name), "tx%i", i);
+		stmmac_res->tx_irq[i] = platform_get_irq_byname_optional(pdev, irq_name);
+		if (stmmac_res->tx_irq[i] < 0) {
+			if (stmmac_res->tx_irq[i] == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			break;
+		}
+	}
+
 	stmmac_res->sfty_ce_irq = platform_get_irq_byname_optional(pdev, "sfty_ce");
 	if (stmmac_res->sfty_ce_irq < 0) {
 		if (stmmac_res->sfty_ce_irq == -EPROBE_DEFER)
-- 
2.40.1

Re: [PATCH net-next v4 6/9] dt-bindings: net: snps,dwmac: add safety irq support

[Rob Herring]

On Wed, 16 Aug 2023 23:29:23 +0800, Jisheng Zhang wrote:
> The snps dwmac IP support safety features, and those Safety Feature
> Correctible Error and Uncorrectible Error irqs may be separate irqs.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  Documentation/devicetree/bindings/net/snps,dwmac.yaml | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.example.dtb: ethernet@16030000: interrupt-names: ['macirq', 'eth_wake_irq', 'eth_lpi'] is too long
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.example.dtb: ethernet@16030000: Unevaluated properties are not allowed ('mdio', 'phy-handle', 'phy-mode', 'rx-fifo-depth', 'snps,axi-config', 'snps,en-tx-lpi-clockgating', 'snps,fixed-burst', 'snps,force_thresh_dma_mode', 'snps,multicast-filter-bins', 'snps,no-pbl-x8', 'snps,perfect-filter-entries', 'snps,rxpbl', 'snps,tso', 'snps,txpbl', 'stmmac-axi-config', 'tx-fifo-depth' were unexpected)
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.example.dtb: ethernet@16030000: interrupt-names: ['macirq', 'eth_wake_irq', 'eth_lpi'] is too long
	from schema $id: http://devicetree.org/schemas/net/snps,dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/snps,dwmac.example.dtb: ethernet@e0800000: interrupt-names: ['macirq', 'eth_wake_irq', 'eth_lpi'] is too long
	from schema $id: http://devicetree.org/schemas/net/snps,dwmac.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230816152926.4093-7-jszhang@kernel.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH net-next v4 2/9] net: stmmac: xgmac: add more feature parsing from hw cap

[Andrew Lunn]

On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote:
> The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps
> or not.

The commit message fails to explain the 'Why?' question. GMII does
normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also
implies 10/100/1000? So why also set dma_cap->mbps_10_100?

Maybe a better change would be to modify:

        seq_printf(seq, "\t1000 Mbps: %s\n",
                   (priv->dma_cap.mbps_1000) ? "Y" : "N");

to actually say 10/100/1000 Mbps? It does not appear this is used for
anything other than debugfs?

	Andrew

Re: [PATCH net-next v4 3/9] net: stmmac: enlarge max rx/tx queues and channels to 16

[Andrew Lunn]

On Wed, Aug 16, 2023 at 11:29:20PM +0800, Jisheng Zhang wrote:
> xgmac supports up to 16 rx/tx queues and up to 16 channels.

What is the effect on memory usage with this? Does this only affect
the limits with ethtool --set-ring, but has no effect by default?

	Andrew

Re: [PATCH net-next v4 7/9] net: stmmac: platform: support parsing safety irqs from DT

[Andrew Lunn]

On Wed, Aug 16, 2023 at 11:29:24PM +0800, Jisheng Zhang wrote:
> The snps dwmac IP may support safety features, and those Safety
> Feature Correctible Error and Uncorrectible Error irqs may be
> separate irqs. Add support to parse the safety irqs from DT.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> Acked-by: Alexandre TORGUE <alexandre.torgue@foss.st.com>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_platform.c    | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index be8e79c7aa34..4a2002eea870 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -737,6 +737,18 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
>  		dev_info(&pdev->dev, "IRQ eth_lpi not found\n");
>  	}
>  
> +	stmmac_res->sfty_ce_irq = platform_get_irq_byname_optional(pdev, "sfty_ce");
> +	if (stmmac_res->sfty_ce_irq < 0) {
> +		if (stmmac_res->sfty_ce_irq == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +	}

I think the error checking should be better than this. If i'm reading
the code correctly, it should return -ENXIO if it does not exist, and
since it is supposed to be optional, you can continue. Other values <
0 are real errors, and it should be returned.

	Andrew

Re: [PATCH net-next v4 9/9] net: stmmac: platform: support parsing per channel irq from DT

[Andrew Lunn]

> +	for (i = 0; i < MTL_MAX_RX_QUEUES; i++) {
> +		snprintf(irq_name, sizeof(irq_name), "rx%i", i);
> +		stmmac_res->rx_irq[i] = platform_get_irq_byname_optional(pdev, irq_name);
> +		if (stmmac_res->rx_irq[i] < 0) {
> +			if (stmmac_res->rx_irq[i] == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;

Same comment here as i made in a previous patch.

     Andrew

Re: [PATCH net-next v4 2/9] net: stmmac: xgmac: add more feature parsing from hw cap

[Russell King (Oracle)]

On Sun, Aug 20, 2023 at 09:15:06PM +0200, Andrew Lunn wrote:
> On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote:
> > The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps
> > or not.
> 
> The commit message fails to explain the 'Why?' question. GMII does
> normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also
> implies 10/100/1000? So why also set dma_cap->mbps_10_100?
> 
> Maybe a better change would be to modify:
> 
>         seq_printf(seq, "\t1000 Mbps: %s\n",
>                    (priv->dma_cap.mbps_1000) ? "Y" : "N");
> 
> to actually say 10/100/1000 Mbps? It does not appear this is used for
> anything other than debugfs?

Indeed, it also looks to me like mbps_1000 and mbps_10_100 are only
used to print things in the debugfs file, and do not have any effect
on the driver.

Moreover:

drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_FEAT_GMIISEL      BIT(1)
drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_GMIISEL       0x00000002       /* 1000 Mbps Support */
drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h:#define XGMAC_HWFEAT_GMIISEL    BIT(1)

Seems to be all the same bit, and:

drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_FEAT_MIISEL       BIT(0)
drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_MIISEL 0x00000001      /* 10/100 Mbps Support */

So, if everyone defines the first few bits of the hw_cap identically,
is there any point to decoding this separately in each driver? Couldn't
the debugfs "show" function just parse the hw_cap directly? Wouldn't it
make more sense to print MII / GMII rather than 10/100 and 1000 ?

It does bring up one last question though: if the driver makes no use
of these hw_cap bits, then is there any point in printing them in the
debugfs file?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v4 2/9] net: stmmac: xgmac: add more feature parsing from hw cap

[Serge Semin]

Hi Russel, Andrew

On Sun, Aug 20, 2023 at 08:51:41PM +0100, Russell King (Oracle) wrote:
> On Sun, Aug 20, 2023 at 09:15:06PM +0200, Andrew Lunn wrote:
> > On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote:

> > > The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps
> > > or not.

> > 
> > The commit message fails to explain the 'Why?' question. GMII does
> > normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also
> > implies 10/100/1000? So why also set dma_cap->mbps_10_100?

Regarding DW XGMAC. I can't say for sure. Based on DW XGMAC v2.10
IP-core HW manual it has MAC_HW_Feature0.GMIISEL(1) flag indicating
whether there is GMII interface besides of the XGMII interface. But in
my databook MAC_HW_Feature0.BIT(0) is marked as reserved and
MAC_Tx_Configuration.SS field doesn't have 10/100Mbps modes despite of
what is defined in dwxgmac2.h by means of the XGMAC_CONFIG_SS_10_MII
and XGMAC_CONFIG_SS_1000_GMII macros.

But DW GMAC or DW Eth QoS can be synthesized with the 1000-only
mode enabled. GMIISEL and MIISEL flags reflect the OP_MODE IP-core
synthesize parameter state. It can have three different values:

Mode of Operation	Description: Configures the MAC to work in
			10/100/1000 Mbps mode. Select 10/100/1000
			Mbps for enabling both Fast Ethernet and Gigabit
			operations, 10/100 Mbps for Fast Ethernet-only
			operations, and 1000 Mbps for Gigabit-only operations.
!!!			Value Range: 10/100/1000 Mbps, 10/100 Mbps, or 1000 Mbps
			Default Value:
				10/100/1000 Mbps with Gigabit License
				10/100 with Fast Ethernet license
HDL Parameter Name: OP_MODE

> > 
> > Maybe a better change would be to modify:
> > 
> >         seq_printf(seq, "\t1000 Mbps: %s\n",
> >                    (priv->dma_cap.mbps_1000) ? "Y" : "N");
> > 
> > to actually say 10/100/1000 Mbps? It does not appear this is used for
> > anything other than debugfs?
> 

> Indeed, it also looks to me like mbps_1000 and mbps_10_100 are only
> used to print things in the debugfs file, and do not have any effect
> on the driver.

They should have been utilized somehow in the stmmac_mac_link_up() and
in the dwmac1000_setup(), dwmac4_setup(), etc methods in order to
select the proper speed. But yeah, currently they are used to print the
DebugFS node data only.

> 
> Moreover:
> 
> drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_FEAT_GMIISEL      BIT(1)
> drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_GMIISEL       0x00000002       /* 1000 Mbps Support */
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h:#define XGMAC_HWFEAT_GMIISEL    BIT(1)
> 
> Seems to be all the same bit, and:
> 
> drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_FEAT_MIISEL       BIT(0)
> drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_MIISEL 0x00000001      /* 10/100 Mbps Support */
> 
> So, if everyone defines the first few bits of the hw_cap identically,
> is there any point to decoding this separately in each driver? Couldn't
> the debugfs "show" function just parse the hw_cap directly? 

The rest of the data in the HW-feature registers is almost completely
different. DW GMAC (common.h) has a single HW-Feature register which
has very little in common with the DW XGMAC (dwxgmac2.h) and DW Eth
QoS (dwmac4.h) MAC_HW_Feature0 register. The later two IP-cores have
the HW-feature registers looking very similar but still differing in
some flags. So in order not to have a partly measured change I would
suggest to preserve the separate HW-features macros space for each
type of the devices for now. If somebody cares to have them indicating
common and separate flags one could provide a comprehensive patch
fixing the entire HW-feature macros definitions. Although I don't see
this being that much necessary.

> Wouldn't it
> make more sense to print MII / GMII rather than 10/100 and 1000 ?
> 

Based on the GMIISEL and MIISEL flags description they are
speed-related, not the interface type:
GMIISEL		1000 Mbps Support
MIISEL		10 or 100 Mbps support

> It does bring up one last question though: if the driver makes no use
> of these hw_cap bits, then is there any point in printing them in the
> debugfs file?

This question can be applied to almost the half of the dma_feature
structure fields.) One more patch extends it with even more mainly
unused fields:
https://lore.kernel.org/netdev/20230819105440.226892-1-0x1207@gmail.com/

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 

Re: [PATCH net-next v4 2/9] net: stmmac: xgmac: add more feature parsing from hw cap

[Russell King (Oracle)]

On Mon, Aug 21, 2023 at 04:25:42PM +0300, Serge Semin wrote:
> Hi Russel, Andrew
> 
> On Sun, Aug 20, 2023 at 08:51:41PM +0100, Russell King (Oracle) wrote:
> > On Sun, Aug 20, 2023 at 09:15:06PM +0200, Andrew Lunn wrote:
> > > On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote:
> 
> > > > The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps
> > > > or not.
> 
> > > 
> > > The commit message fails to explain the 'Why?' question. GMII does
> > > normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also
> > > implies 10/100/1000? So why also set dma_cap->mbps_10_100?
> 
> Regarding DW XGMAC. I can't say for sure. Based on DW XGMAC v2.10
> IP-core HW manual it has MAC_HW_Feature0.GMIISEL(1) flag indicating
> whether there is GMII interface besides of the XGMII interface. But in
> my databook MAC_HW_Feature0.BIT(0) is marked as reserved and
> MAC_Tx_Configuration.SS field doesn't have 10/100Mbps modes despite of
> what is defined in dwxgmac2.h by means of the XGMAC_CONFIG_SS_10_MII
> and XGMAC_CONFIG_SS_1000_GMII macros.
> 
> But DW GMAC or DW Eth QoS can be synthesized with the 1000-only
> mode enabled. GMIISEL and MIISEL flags reflect the OP_MODE IP-core
> synthesize parameter state. It can have three different values:
> 
> Mode of Operation	Description: Configures the MAC to work in
> 			10/100/1000 Mbps mode. Select 10/100/1000
> 			Mbps for enabling both Fast Ethernet and Gigabit
> 			operations, 10/100 Mbps for Fast Ethernet-only
> 			operations, and 1000 Mbps for Gigabit-only operations.
> !!!			Value Range: 10/100/1000 Mbps, 10/100 Mbps, or 1000 Mbps
> 			Default Value:
> 				10/100/1000 Mbps with Gigabit License
> 				10/100 with Fast Ethernet license
> HDL Parameter Name: OP_MODE
> 
> > > 
> > > Maybe a better change would be to modify:
> > > 
> > >         seq_printf(seq, "\t1000 Mbps: %s\n",
> > >                    (priv->dma_cap.mbps_1000) ? "Y" : "N");
> > > 
> > > to actually say 10/100/1000 Mbps? It does not appear this is used for
> > > anything other than debugfs?
> > 
> 
> > Indeed, it also looks to me like mbps_1000 and mbps_10_100 are only
> > used to print things in the debugfs file, and do not have any effect
> > on the driver.
> 
> They should have been utilized somehow in the stmmac_mac_link_up() and

No, definitely not in mac_link_up(). If these flags indicate what speeds
are available, then what would mac_link_up() do if, e.g. the core says
"I don't support 1G" and phylink determines that the result of
negotiation is 1G?

This is clearly not the right place. The right place is when
initialising the phylink MAC capabilities, which is currently done in
stmmac_phy_setup() without *any* regard what so ever for what speeds
are actually supported, with the exception of "oh, is that the maximum
speed".

> > It does bring up one last question though: if the driver makes no use
> > of these hw_cap bits, then is there any point in printing them in the
> > debugfs file?
> 
> This question can be applied to almost the half of the dma_feature
> structure fields.) One more patch extends it with even more mainly
> unused fields:
> https://lore.kernel.org/netdev/20230819105440.226892-1-0x1207@gmail.com/

If the hw_cap field is specific, then how about some hardware specific
data giving something like an enum listing the capabilties, the enum
used to index a string table of capabilities, and an array of bit
numbers in hw_cap for those fields, or an array of masks? This data
could be const, which means that stmmac_dma_cap_show() only needs
the hw_cap value and the struct.

That also means that stmmac_phy_setup() could also index the
array of bit numbers to test for e.g. GMII/MII support to determine
whether 10/100 and 1000 capabilities should be added for phylink.

If we look at the "half_duplex" dma capability, things are similarly
stupid. Pulling out of dwmac4:

        dma_cap->half_duplex = (hw_cap & GMAC_HW_FEAT_HDSEL) >> 2;

This is not tested elsewhere from what I can find - neither the
hwcap nor the half_duplex field except for reporting in debugfs.
It isn't used to restrict the phylink capabilities for HD, since
the only test is this:

        /* Half-Duplex can only work with single queue */
        if (priv->plat->tx_queues_to_use > 1)
                priv->phylink_config.mac_capabilities &=
                        ~(MAC_10HD | MAC_100HD | MAC_1000HD);

So, the reporting of "half duplex" mode in debugfs has absolutely
nothing to do with whether we try to use half duplex modes in the
driver.

This is rubbish. Utter rubbish.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v4 2/9] net: stmmac: xgmac: add more feature parsing from hw cap

[Serge Semin]

On Mon, Aug 21, 2023 at 02:57:51PM +0100, Russell King (Oracle) wrote:
> On Mon, Aug 21, 2023 at 04:25:42PM +0300, Serge Semin wrote:
> > Hi Russel, Andrew
> > 
> > On Sun, Aug 20, 2023 at 08:51:41PM +0100, Russell King (Oracle) wrote:
> > > On Sun, Aug 20, 2023 at 09:15:06PM +0200, Andrew Lunn wrote:
> > > > On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote:
> > 
> > > > > The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps
> > > > > or not.
> > 
> > > > 
> > > > The commit message fails to explain the 'Why?' question. GMII does
> > > > normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also
> > > > implies 10/100/1000? So why also set dma_cap->mbps_10_100?
> > 
> > Regarding DW XGMAC. I can't say for sure. Based on DW XGMAC v2.10
> > IP-core HW manual it has MAC_HW_Feature0.GMIISEL(1) flag indicating
> > whether there is GMII interface besides of the XGMII interface. But in
> > my databook MAC_HW_Feature0.BIT(0) is marked as reserved and
> > MAC_Tx_Configuration.SS field doesn't have 10/100Mbps modes despite of
> > what is defined in dwxgmac2.h by means of the XGMAC_CONFIG_SS_10_MII
> > and XGMAC_CONFIG_SS_1000_GMII macros.
> > 
> > But DW GMAC or DW Eth QoS can be synthesized with the 1000-only
> > mode enabled. GMIISEL and MIISEL flags reflect the OP_MODE IP-core
> > synthesize parameter state. It can have three different values:
> > 
> > Mode of Operation	Description: Configures the MAC to work in
> > 			10/100/1000 Mbps mode. Select 10/100/1000
> > 			Mbps for enabling both Fast Ethernet and Gigabit
> > 			operations, 10/100 Mbps for Fast Ethernet-only
> > 			operations, and 1000 Mbps for Gigabit-only operations.
> > !!!			Value Range: 10/100/1000 Mbps, 10/100 Mbps, or 1000 Mbps
> > 			Default Value:
> > 				10/100/1000 Mbps with Gigabit License
> > 				10/100 with Fast Ethernet license
> > HDL Parameter Name: OP_MODE
> > 
> > > > 
> > > > Maybe a better change would be to modify:
> > > > 
> > > >         seq_printf(seq, "\t1000 Mbps: %s\n",
> > > >                    (priv->dma_cap.mbps_1000) ? "Y" : "N");
> > > > 
> > > > to actually say 10/100/1000 Mbps? It does not appear this is used for
> > > > anything other than debugfs?
> > > 
> > 
> > > Indeed, it also looks to me like mbps_1000 and mbps_10_100 are only
> > > used to print things in the debugfs file, and do not have any effect
> > > on the driver.
> > 
> > They should have been utilized somehow in the stmmac_mac_link_up() and
> 
> No, definitely not in mac_link_up(). If these flags indicate what speeds
> are available, then what would mac_link_up() do if, e.g. the core says
> "I don't support 1G" and phylink determines that the result of
> negotiation is 1G?
> 
> This is clearly not the right place. The right place is when
> initialising the phylink MAC capabilities, which is currently done in
> stmmac_phy_setup() without *any* regard what so ever for what speeds
> are actually supported, with the exception of "oh, is that the maximum
> speed".

Ok. My suggestion was based on the current stmmac_mac_link_up()
implementation which configures the MAC speed based on the speed
coming from the phylink core and silently returns if the speed is
unsupported.) 

> 
> > > It does bring up one last question though: if the driver makes no use
> > > of these hw_cap bits, then is there any point in printing them in the
> > > debugfs file?
> > 
> > This question can be applied to almost the half of the dma_feature
> > structure fields.) One more patch extends it with even more mainly
> > unused fields:
> > https://lore.kernel.org/netdev/20230819105440.226892-1-0x1207@gmail.com/
> 

> If the hw_cap field is specific, then how about some hardware specific
> data giving something like an enum listing the capabilties, the enum
> used to index a string table of capabilities, and an array of bit
> numbers in hw_cap for those fields, or an array of masks? This data
> could be const, which means that stmmac_dma_cap_show() only needs
> the hw_cap value and the struct.

That would have been a great solution.

> 
> That also means that stmmac_phy_setup() could also index the
> array of bit numbers to test for e.g. GMII/MII support to determine
> whether 10/100 and 1000 capabilities should be added for phylink.
> 
> If we look at the "half_duplex" dma capability, things are similarly
> stupid. Pulling out of dwmac4:
> 
>         dma_cap->half_duplex = (hw_cap & GMAC_HW_FEAT_HDSEL) >> 2;
> 
> This is not tested elsewhere from what I can find - neither the
> hwcap nor the half_duplex field except for reporting in debugfs.
> It isn't used to restrict the phylink capabilities for HD, since
> the only test is this:
> 
>         /* Half-Duplex can only work with single queue */
>         if (priv->plat->tx_queues_to_use > 1)
>                 priv->phylink_config.mac_capabilities &=
>                         ~(MAC_10HD | MAC_100HD | MAC_1000HD);
> 
> So, the reporting of "half duplex" mode in debugfs has absolutely
> nothing to do with whether we try to use half duplex modes in the
> driver.
> 
> This is rubbish. Utter rubbish.

So is a lot of stuff in the STMMAC driver. Look closely at what is
implemented there. One bright example is the plat_stmmacenet_data
structure content. For instance, msi_mac_vec, msi_wol_vec,
msi_lpi_vec, msi_sfty_ce_vec, msi_sfty_ue_vec, msi_rx_base_vec,
msi_tx_base_vec aren't even utilized in the core driver, but in the
Intel glue driver only. Some other plat_stmmacenet_data fields are
utilized to either override the dma_features fields or being utilized
even though there is a auto-detectable HW-features field.

All of the HW-abstraction macros accept stmmac_priv pointer as a
parameter meanwhile the abstracting functions don't. So the respective
functions need to have all of parameters passed as arguments
which makes some function prototypes too bulky and would require the
prototype modification should some additional data is required in the
function implementation. Moreover the HW-abstraction function
prototypes aren't unified: some accept the regs base address, some
mac_device_info pointer, etc.

mac_device_info instance is always required but it's separately malloced all
the time the stmmac_drv_probe() is called. It should have been just
embedded into the stmmac_priv data.

and so on and so forth.

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!