Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes
@ 2020-01-10 11:54 Andre Przywara
  2020-01-10 11:54 ` [PATCH 01/14] net: xilinx: temac: Relax Kconfig dependencies Andre Przywara
                   ` (13 more replies)
  0 siblings, 14 replies; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

Hi,

this series updates the Xilinx Axienet driver to work on our board
here. One big issue was broken SGMII support, which patch 7 fixes.
While debugging and understanding the driver, I found several problems
in the error handling and cleanup paths, which patches 2-6 address.
Patch 8 paves the way for newer revisions of the IP, the following patch
adds mii-tool support, just for good measure.

The next four patches add support for 64-bit DMA. This is an integration
option on newer IP revisions (>= v7.1), and expects MSB bits in formerly
reserved registers. Without writing to those MSB registers, the state
machine won't trigger, so it's mandatory to access them, even if they
are zero. Patches 10 and 11 prepare the code by adding accessors, to
wrap this properly and keep it working on older IP revisions.
Patch 12 enables access to the MSB registers, by trying to write a
non-zero value to them and checking if that sticks. Older IP revisions
always read those registers as zero.

Patch 13 then adjusts the DMA mask, if it finds a "xlnx,addrwidth"
property in the DMA DT node. I did not manage to autodetect this actual
implemented DMA width, so we need to rely on a DT property. If this is
missing, it will fall back to the current default of 32 bit.
The final patch updates the DT binding documentation in this respect.

The Xilinx PG138 and PG021 documents (in versions 7.1 in both cases)
were used for this series.

Please have a look and comment!

Cheers,
Andre

Andre Przywara (14):
  net: xilinx: temac: Relax Kconfig dependencies
  net: axienet: Propagate failure of DMA descriptor setup
  net: axienet: Fix DMA descriptor cleanup path
  net: axienet: Improve DMA error handling
  net: axienet: Factor out TX descriptor chain cleanup
  net: axienet: Check for DMA mapping errors
  net: axienet: Fix SGMII support
  net: axienet: Drop MDIO interrupt registers from ethtools dump
  net: axienet: Add mii-tool support
  net: axienet: Wrap DMA pointer writes to prepare for 64 bit
  net: axienet: Upgrade descriptors to hold 64-bit addresses
  net: axienet: Autodetect 64-bit DMA capability
  net: axienet: Allow DMA to beyond 4GB
  net: axienet: Update devicetree binding documentation

 .../bindings/net/xilinx_axienet.txt           |  57 ++-
 drivers/net/ethernet/xilinx/Kconfig           |   1 -
 drivers/net/ethernet/xilinx/xilinx_axienet.h  |  10 +-
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 366 +++++++++++++-----
 4 files changed, 328 insertions(+), 106 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 01/14] net: xilinx: temac: Relax Kconfig dependencies
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-10 14:19   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 02/14] net: axienet: Propagate failure of DMA descriptor setup Andre Przywara
                   ` (12 subsequent siblings)
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

Similar to axienet, the temac driver is now architecture agnostic, and
can be at least compiled for several architectures.
Especially the fact that this is a soft IP for implementing in FPGAs
makes the current restriction rather pointless, as it could literally
appear on any architecture, as long as an FPGA is connected to the bus.

The driver hasn't been actually tried on any hardware, it is just a
drive-by patch when doing the same for axienet (a similar patch for
axienet is already merged).

This (temac and axienet) have been compile-tested for:
alpha hppa64 microblaze mips64 powerpc powerpc64 riscv64 s390 sparc64
(using kernel.org cross compilers).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/ethernet/xilinx/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/Kconfig b/drivers/net/ethernet/xilinx/Kconfig
index 6304ebd8b5c6..0810af8193cb 100644
--- a/drivers/net/ethernet/xilinx/Kconfig
+++ b/drivers/net/ethernet/xilinx/Kconfig
@@ -32,7 +32,6 @@ config XILINX_AXI_EMAC
 
 config XILINX_LL_TEMAC
 	tristate "Xilinx LL TEMAC (LocalLink Tri-mode Ethernet MAC) driver"
-	depends on PPC || MICROBLAZE || X86 || COMPILE_TEST
 	select PHYLIB
 	---help---
 	  This driver supports the Xilinx 10/100/1000 LocalLink TEMAC
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 02/14] net: axienet: Propagate failure of DMA descriptor setup
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
  2020-01-10 11:54 ` [PATCH 01/14] net: xilinx: temac: Relax Kconfig dependencies Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-10 14:54   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path Andre Przywara
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

When we fail allocating the DMA buffers in axienet_dma_bd_init(), we
report this error, but carry on with initialisation nevertheless.

This leads to a kernel panic when the driver later wants to send a
packet, as it uses uninitialised data structures.

Make the axienet_device_reset() routine return an error value, as it
contains the DMA buffer initialisation. Make sure we propagate the error
up the chain and eventually fail the driver initialisation, to avoid
relying on non-initialised buffers.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 25 +++++++++++++------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 20746b801959..97482cf093ce 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -437,9 +437,10 @@ static void axienet_setoptions(struct net_device *ndev, u32 options)
 	lp->options |= options;
 }
 
-static void __axienet_device_reset(struct axienet_local *lp)
+static int __axienet_device_reset(struct axienet_local *lp)
 {
 	u32 timeout;
+
 	/* Reset Axi DMA. This would reset Axi Ethernet core as well. The reset
 	 * process of Axi DMA takes a while to complete as all pending
 	 * commands/transfers will be flushed or completed during this
@@ -455,9 +456,11 @@ static void __axienet_device_reset(struct axienet_local *lp)
 		if (--timeout == 0) {
 			netdev_err(lp->ndev, "%s: DMA reset timeout!\n",
 				   __func__);
-			break;
+			return -ETIMEDOUT;
 		}
 	}
+
+	return 0;
 }
 
 /**
@@ -471,12 +474,15 @@ static void __axienet_device_reset(struct axienet_local *lp)
  * Ethernet core. No separate hardware reset is done for the Axi Ethernet
  * core.
  */
-static void axienet_device_reset(struct net_device *ndev)
+static int axienet_device_reset(struct net_device *ndev)
 {
 	u32 axienet_status;
 	struct axienet_local *lp = netdev_priv(ndev);
+	int ret;
 
-	__axienet_device_reset(lp);
+	ret = __axienet_device_reset(lp);
+	if (ret)
+		return ret;
 
 	lp->max_frm_size = XAE_MAX_VLAN_FRAME_SIZE;
 	lp->options |= XAE_OPTION_VLAN;
@@ -491,9 +497,11 @@ static void axienet_device_reset(struct net_device *ndev)
 			lp->options |= XAE_OPTION_JUMBO;
 	}
 
-	if (axienet_dma_bd_init(ndev)) {
+	ret = axienet_dma_bd_init(ndev);
+	if (ret) {
 		netdev_err(ndev, "%s: descriptor allocation failed\n",
 			   __func__);
+		return ret;
 	}
 
 	axienet_status = axienet_ior(lp, XAE_RCW1_OFFSET);
@@ -518,6 +526,8 @@ static void axienet_device_reset(struct net_device *ndev)
 	axienet_setoptions(ndev, lp->options);
 
 	netif_trans_update(ndev);
+
+	return 0;
 }
 
 /**
@@ -921,8 +931,9 @@ static int axienet_open(struct net_device *ndev)
 	 */
 	mutex_lock(&lp->mii_bus->mdio_lock);
 	axienet_mdio_disable(lp);
-	axienet_device_reset(ndev);
-	ret = axienet_mdio_enable(lp);
+	ret = axienet_device_reset(ndev);
+	if (ret == 0)
+		ret = axienet_mdio_enable(lp);
 	mutex_unlock(&lp->mii_bus->mdio_lock);
 	if (ret < 0)
 		return ret;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
  2020-01-10 11:54 ` [PATCH 01/14] net: xilinx: temac: Relax Kconfig dependencies Andre Przywara
  2020-01-10 11:54 ` [PATCH 02/14] net: axienet: Propagate failure of DMA descriptor setup Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-10 15:14   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 04/14] net: axienet: Improve DMA error handling Andre Przywara
                   ` (10 subsequent siblings)
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

When axienet_dma_bd_init() bails out during the initialisation process,
it might do so with parts of the structure already allocated and
initialised, while other parts have not been touched yet. Before
returning in this case, we call axienet_dma_bd_release(), which does not
take care of this corner case.
This is most obvious by the first loop happily dereferencing
lp->rx_bd_v, which we actually check to be non NULL *afterwards*.

Make sure we only unmap or free already allocated structures, by:
- directly returning with -ENOMEM if nothing has been allocated at all
- checking for lp->rx_bd_v to be non-NULL *before* using it
- only unmapping allocated DMA RX regions

This avoids NULL pointer dereferences when initialisation fails.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++-------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 97482cf093ce..7e90044cf2d9 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct net_device *ndev)
 	int i;
 	struct axienet_local *lp = netdev_priv(ndev);
 
+	/* If we end up here, tx_bd_v must have been DMA allocated. */
+	dma_free_coherent(ndev->dev.parent,
+			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
+			  lp->tx_bd_v,
+			  lp->tx_bd_p);
+
+	if (!lp->rx_bd_v)
+		return;
+
 	for (i = 0; i < lp->rx_bd_num; i++) {
-		dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
-				 lp->max_frm_size, DMA_FROM_DEVICE);
+		/* A NULL skb means this descriptor has not been initialised
+		 * at all.
+		 */
+		if (!lp->rx_bd_v[i].skb)
+			break;
+
 		dev_kfree_skb(lp->rx_bd_v[i].skb);
-	}
 
-	if (lp->rx_bd_v) {
-		dma_free_coherent(ndev->dev.parent,
-				  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
-				  lp->rx_bd_v,
-				  lp->rx_bd_p);
-	}
-	if (lp->tx_bd_v) {
-		dma_free_coherent(ndev->dev.parent,
-				  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
-				  lp->tx_bd_v,
-				  lp->tx_bd_p);
+		/* For each descriptor, we programmed cntrl with the (non-zero)
+		 * descriptor size, after it had been successfully allocated.
+		 * So a non-zero value in there means we need to unmap it.
+		 */
+		if (lp->rx_bd_v[i].cntrl)
+			dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
+					 lp->max_frm_size, DMA_FROM_DEVICE);
 	}
+
+	dma_free_coherent(ndev->dev.parent,
+			  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
+			  lp->rx_bd_v,
+			  lp->rx_bd_p);
 }
 
 /**
@@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device *ndev)
 					 sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
 					 &lp->tx_bd_p, GFP_KERNEL);
 	if (!lp->tx_bd_v)
-		goto out;
+		return -ENOMEM;
 
 	lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
 					 sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 04/14] net: axienet: Improve DMA error handling
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (2 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-10 15:26   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 05/14] net: axienet: Factor out TX descriptor chain cleanup Andre Przywara
                   ` (9 subsequent siblings)
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

Since 0 is a valid DMA address, we cannot use the physical address to
check whether a TX descriptor is valid and is holding a DMA mapping.

Use the "cntrl" member of the descriptor to make this decision, as it
contains at least the length of the buffer, so 0 points to an
uninitialised buffer.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 7e90044cf2d9..ec5d01adc1d5 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -570,7 +570,7 @@ static void axienet_start_xmit_done(struct net_device *ndev)
 				DMA_TO_DEVICE);
 		if (cur_p->skb)
 			dev_consume_skb_irq(cur_p->skb);
-		/*cur_p->phys = 0;*/
+		cur_p->cntrl = 0;
 		cur_p->app0 = 0;
 		cur_p->app1 = 0;
 		cur_p->app2 = 0;
@@ -1557,7 +1557,7 @@ static void axienet_dma_err_handler(unsigned long data)
 
 	for (i = 0; i < lp->tx_bd_num; i++) {
 		cur_p = &lp->tx_bd_v[i];
-		if (cur_p->phys)
+		if (cur_p->cntrl)
 			dma_unmap_single(ndev->dev.parent, cur_p->phys,
 					 (cur_p->cntrl &
 					  XAXIDMA_BD_CTRL_LENGTH_MASK),
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 05/14] net: axienet: Factor out TX descriptor chain cleanup
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (3 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 04/14] net: axienet: Improve DMA error handling Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-10 18:04   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 06/14] net: axienet: Check for DMA mapping errors Andre Przywara
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

Factor out the code that cleans up a number of connected TX descriptors,
as we will need it to properly roll back a failed _xmit() call.
There are subtle differences between cleaning up a successfully sent
chain (unknown number of involved descriptors, total data size needed)
and a chain that was about to set up (number of descriptors known), so
cater for those variations with some extra parameters.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 75 ++++++++++++-------
 1 file changed, 50 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index ec5d01adc1d5..82abe2b0f16a 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -543,33 +543,37 @@ static int axienet_device_reset(struct net_device *ndev)
 	return 0;
 }
 
-/**
- * axienet_start_xmit_done - Invoked once a transmit is completed by the
- * Axi DMA Tx channel.
- * @ndev:	Pointer to the net_device structure
- *
- * This function is invoked from the Axi DMA Tx isr to notify the completion
- * of transmit operation. It clears fields in the corresponding Tx BDs and
- * unmaps the corresponding buffer so that CPU can regain ownership of the
- * buffer. It finally invokes "netif_wake_queue" to restart transmission if
- * required.
+/* Clean up a series of linked TX descriptors. Would either be called
+ * after a successful transmit operation, or after there was an error
+ * when setting up the chain.
+ * Returns the number of descriptors handled.
  */
-static void axienet_start_xmit_done(struct net_device *ndev)
+static int axienet_free_tx_chain(struct net_device *ndev, u32 first_bd,
+				 int nr_bds, u32 *sizep)
 {
-	u32 size = 0;
-	u32 packets = 0;
 	struct axienet_local *lp = netdev_priv(ndev);
+	int max_bds = (nr_bds != -1) ? nr_bds : lp->tx_bd_num;
 	struct axidma_bd *cur_p;
-	unsigned int status = 0;
+	unsigned int status;
+	int i;
+
+	for (i = 0; i < max_bds; i++) {
+		cur_p = &lp->tx_bd_v[(first_bd + i) % lp->tx_bd_num];
+		status = cur_p->status;
+
+		/* If no number is given, clean up *all* descriptors that have
+		 * been completed by the MAC.
+		 */
+		if (nr_bds == -1 && !(status & XAXIDMA_BD_STS_COMPLETE_MASK))
+			break;
 
-	cur_p = &lp->tx_bd_v[lp->tx_bd_ci];
-	status = cur_p->status;
-	while (status & XAXIDMA_BD_STS_COMPLETE_MASK) {
 		dma_unmap_single(ndev->dev.parent, cur_p->phys,
 				(cur_p->cntrl & XAXIDMA_BD_CTRL_LENGTH_MASK),
 				DMA_TO_DEVICE);
-		if (cur_p->skb)
+
+		if (cur_p->skb && (status & XAXIDMA_BD_STS_COMPLETE_MASK))
 			dev_consume_skb_irq(cur_p->skb);
+
 		cur_p->cntrl = 0;
 		cur_p->app0 = 0;
 		cur_p->app1 = 0;
@@ -578,15 +582,36 @@ static void axienet_start_xmit_done(struct net_device *ndev)
 		cur_p->status = 0;
 		cur_p->skb = NULL;
 
-		size += status & XAXIDMA_BD_STS_ACTUAL_LEN_MASK;
-		packets++;
-
-		if (++lp->tx_bd_ci >= lp->tx_bd_num)
-			lp->tx_bd_ci = 0;
-		cur_p = &lp->tx_bd_v[lp->tx_bd_ci];
-		status = cur_p->status;
+		if (sizep)
+			*sizep += status & XAXIDMA_BD_STS_ACTUAL_LEN_MASK;
 	}
 
+	return i;
+}
+
+/**
+ * axienet_start_xmit_done - Invoked once a transmit is completed by the
+ * Axi DMA Tx channel.
+ * @ndev:	Pointer to the net_device structure
+ *
+ * This function is invoked from the Axi DMA Tx isr to notify the completion
+ * of transmit operation. It clears fields in the corresponding Tx BDs and
+ * unmaps the corresponding buffer so that CPU can regain ownership of the
+ * buffer. It finally invokes "netif_wake_queue" to restart transmission if
+ * required.
+ */
+static void axienet_start_xmit_done(struct net_device *ndev)
+{
+	u32 size = 0;
+	u32 packets = 0;
+	struct axienet_local *lp = netdev_priv(ndev);
+
+	packets = axienet_free_tx_chain(ndev, lp->tx_bd_ci, -1, &size);
+
+	lp->tx_bd_ci += packets;
+	if (lp->tx_bd_ci >= lp->tx_bd_num)
+		lp->tx_bd_ci -= lp->tx_bd_num;
+
 	ndev->stats.tx_packets += packets;
 	ndev->stats.tx_bytes += size;
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 06/14] net: axienet: Check for DMA mapping errors
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (4 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 05/14] net: axienet: Factor out TX descriptor chain cleanup Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-13  5:54   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 07/14] net: axienet: Fix SGMII support Andre Przywara
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

Especially with the default 32-bit DMA mask, DMA buffers are a limited
resource, so their allocation can fail.
So as the DMA API documentation requires, add error checking code after
dma_map_single() calls to catch the case where we run out of "low" memory.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 82abe2b0f16a..8d2b67cbecf9 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -248,6 +248,11 @@ static int axienet_dma_bd_init(struct net_device *ndev)
 						     skb->data,
 						     lp->max_frm_size,
 						     DMA_FROM_DEVICE);
+		if (dma_mapping_error(ndev->dev.parent, lp->rx_bd_v[i].phys)) {
+			dev_kfree_skb(skb);
+			goto out;
+		}
+
 		lp->rx_bd_v[i].cntrl = lp->max_frm_size;
 	}
 
@@ -668,6 +673,7 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	dma_addr_t tail_p;
 	struct axienet_local *lp = netdev_priv(ndev);
 	struct axidma_bd *cur_p;
+	u32 orig_tail_ptr = lp->tx_bd_tail;
 
 	num_frag = skb_shinfo(skb)->nr_frags;
 	cur_p = &lp->tx_bd_v[lp->tx_bd_tail];
@@ -703,9 +709,11 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		cur_p->app0 |= 2; /* Tx Full Checksum Offload Enabled */
 	}
 
-	cur_p->cntrl = skb_headlen(skb) | XAXIDMA_BD_CTRL_TXSOF_MASK;
 	cur_p->phys = dma_map_single(ndev->dev.parent, skb->data,
 				     skb_headlen(skb), DMA_TO_DEVICE);
+	if (dma_mapping_error(ndev->dev.parent, cur_p->phys))
+		return NETDEV_TX_BUSY;
+	cur_p->cntrl = skb_headlen(skb) | XAXIDMA_BD_CTRL_TXSOF_MASK;
 
 	for (ii = 0; ii < num_frag; ii++) {
 		if (++lp->tx_bd_tail >= lp->tx_bd_num)
@@ -716,6 +724,13 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 					     skb_frag_address(frag),
 					     skb_frag_size(frag),
 					     DMA_TO_DEVICE);
+		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
+			axienet_free_tx_chain(ndev, orig_tail_ptr, ii + 1,
+					      NULL);
+			lp->tx_bd_tail = orig_tail_ptr;
+
+			return NETDEV_TX_BUSY;
+		}
 		cur_p->cntrl = skb_frag_size(frag);
 	}
 
@@ -796,6 +811,11 @@ static void axienet_recv(struct net_device *ndev)
 		cur_p->phys = dma_map_single(ndev->dev.parent, new_skb->data,
 					     lp->max_frm_size,
 					     DMA_FROM_DEVICE);
+		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
+			dev_kfree_skb(new_skb);
+			return;
+		}
+
 		cur_p->cntrl = lp->max_frm_size;
 		cur_p->status = 0;
 		cur_p->skb = new_skb;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (5 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 06/14] net: axienet: Check for DMA mapping errors Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-10 14:04   ` Andrew Lunn
  2020-01-10 14:58   ` Russell King - ARM Linux admin
  2020-01-10 11:54 ` [PATCH 08/14] net: axienet: Drop MDIO interrupt registers from ethtools dump Andre Przywara
                   ` (6 subsequent siblings)
  13 siblings, 2 replies; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

With SGMII, the MAC and the PHY can negotiate the link speed between
themselves, without the host needing to mediate between them.
Linux recognises this, and will call phylink's mac_config with the speed
member set to SPEED_UNKNOWN (-1).
Currently the axienet driver will bail out and complain about an
unsupported link speed.

Teach axienet's mac_config callback to leave the MAC's speed setting
alone if the requested speed is SPEED_UNKNOWN.

This fixes axienet operation when the hardware is using SGMII.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 21 ++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 8d2b67cbecf9..e83c7b005f50 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1512,20 +1512,21 @@ static void axienet_mac_config(struct phylink_config *config, unsigned int mode,
 {
 	struct net_device *ndev = to_net_dev(config->dev);
 	struct axienet_local *lp = netdev_priv(ndev);
-	u32 emmc_reg, fcc_reg;
+	u32 fcc_reg, speed_reg = ~0;
 
-	emmc_reg = axienet_ior(lp, XAE_EMMC_OFFSET);
-	emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
 
 	switch (state->speed) {
+	case SPEED_UNKNOWN:
+		/* Keep the current MAC speed setting. Used for SGMII. */
+		break;
 	case SPEED_1000:
-		emmc_reg |= XAE_EMMC_LINKSPD_1000;
+		speed_reg = XAE_EMMC_LINKSPD_1000;
 		break;
 	case SPEED_100:
-		emmc_reg |= XAE_EMMC_LINKSPD_100;
+		speed_reg = XAE_EMMC_LINKSPD_100;
 		break;
 	case SPEED_10:
-		emmc_reg |= XAE_EMMC_LINKSPD_10;
+		speed_reg = XAE_EMMC_LINKSPD_10;
 		break;
 	default:
 		dev_err(&ndev->dev,
@@ -1533,7 +1534,13 @@ static void axienet_mac_config(struct phylink_config *config, unsigned int mode,
 		break;
 	}
 
-	axienet_iow(lp, XAE_EMMC_OFFSET, emmc_reg);
+	if (speed_reg != ~0) {
+		u32 emmc_reg = axienet_ior(lp, XAE_EMMC_OFFSET);
+
+		emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
+		emmc_reg |= speed_reg;
+		axienet_iow(lp, XAE_EMMC_OFFSET, emmc_reg);
+	}
 
 	fcc_reg = axienet_ior(lp, XAE_FCC_OFFSET);
 	if (state->pause & MLO_PAUSE_TX)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 08/14] net: axienet: Drop MDIO interrupt registers from ethtools dump
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (6 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 07/14] net: axienet: Fix SGMII support Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-13  6:02   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 09/14] net: axienet: Add mii-tool support Andre Przywara
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

Newer revisions of the IP don't have these registers. Since we don't
really use them, just drop them from the ethtools dump.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index e83c7b005f50..7a747345e98e 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1239,10 +1239,6 @@ static void axienet_ethtools_get_regs(struct net_device *ndev,
 	data[20] = axienet_ior(lp, XAE_MDIO_MCR_OFFSET);
 	data[21] = axienet_ior(lp, XAE_MDIO_MWD_OFFSET);
 	data[22] = axienet_ior(lp, XAE_MDIO_MRD_OFFSET);
-	data[23] = axienet_ior(lp, XAE_MDIO_MIS_OFFSET);
-	data[24] = axienet_ior(lp, XAE_MDIO_MIP_OFFSET);
-	data[25] = axienet_ior(lp, XAE_MDIO_MIE_OFFSET);
-	data[26] = axienet_ior(lp, XAE_MDIO_MIC_OFFSET);
 	data[27] = axienet_ior(lp, XAE_UAW0_OFFSET);
 	data[28] = axienet_ior(lp, XAE_UAW1_OFFSET);
 	data[29] = axienet_ior(lp, XAE_FMI_OFFSET);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 09/14] net: axienet: Add mii-tool support
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (7 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 08/14] net: axienet: Drop MDIO interrupt registers from ethtools dump Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-13  6:12   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 10/14] net: axienet: Wrap DMA pointer writes to prepare for 64 bit Andre Przywara
                   ` (4 subsequent siblings)
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

mii-tool is useful for debugging, and all it requires to work is to wire
up the ioctl ops function pointer.
Add this to the axienet driver to enable mii-tool.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 7a747345e98e..64f799f3d248 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1152,6 +1152,16 @@ static void axienet_poll_controller(struct net_device *ndev)
 }
 #endif
 
+static int axienet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
+{
+	struct axienet_local *lp = netdev_priv(dev);
+
+	if (!netif_running(dev))
+		return -EINVAL;
+
+	return phylink_mii_ioctl(lp->phylink, rq, cmd);
+}
+
 static const struct net_device_ops axienet_netdev_ops = {
 	.ndo_open = axienet_open,
 	.ndo_stop = axienet_stop,
@@ -1159,6 +1169,7 @@ static const struct net_device_ops axienet_netdev_ops = {
 	.ndo_change_mtu	= axienet_change_mtu,
 	.ndo_set_mac_address = netdev_set_mac_address,
 	.ndo_validate_addr = eth_validate_addr,
+	.ndo_do_ioctl = axienet_ioctl,
 	.ndo_set_rx_mode = axienet_set_multicast_list,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller = axienet_poll_controller,
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 10/14] net: axienet: Wrap DMA pointer writes to prepare for 64 bit
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (8 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 09/14] net: axienet: Add mii-tool support Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-10 11:54 ` [PATCH 11/14] net: axienet: Upgrade descriptors to hold 64-bit addresses Andre Przywara
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

Newer versions of the Xilink DMA IP support busses with more than 32
address bits, by introducing an MSB word for the registers holding DMA
pointers (tail/current, RX/TX descriptor addresses).
On IP configured for more than 32 bits, it is also *required* to write
both words, to let the IP recognise this as a start condition for an
MM2S request, for instance.

Wrap the DMA pointer writes with a separate function, to add this
functionality later. For now we stick to the lower 32 bits.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 26 ++++++++++++-------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 64f799f3d248..bbdda4b0c677 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -147,6 +147,12 @@ static inline void axienet_dma_out32(struct axienet_local *lp,
 	iowrite32(value, lp->dma_regs + reg);
 }
 
+static void axienet_dma_out_addr(struct axienet_local *lp, off_t reg,
+				 dma_addr_t addr)
+{
+	axienet_dma_out32(lp, reg, lower_32_bits(addr));
+}
+
 /**
  * axienet_dma_bd_release - Release buffer descriptor rings
  * @ndev:	Pointer to the net_device structure
@@ -285,18 +291,18 @@ static int axienet_dma_bd_init(struct net_device *ndev)
 	/* Populate the tail pointer and bring the Rx Axi DMA engine out of
 	 * halted state. This will make the Rx side ready for reception.
 	 */
-	axienet_dma_out32(lp, XAXIDMA_RX_CDESC_OFFSET, lp->rx_bd_p);
+	axienet_dma_out_addr(lp, XAXIDMA_RX_CDESC_OFFSET, lp->rx_bd_p);
 	cr = axienet_dma_in32(lp, XAXIDMA_RX_CR_OFFSET);
 	axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET,
 			  cr | XAXIDMA_CR_RUNSTOP_MASK);
-	axienet_dma_out32(lp, XAXIDMA_RX_TDESC_OFFSET, lp->rx_bd_p +
-			  (sizeof(*lp->rx_bd_v) * (lp->rx_bd_num - 1)));
+	axienet_dma_out_addr(lp, XAXIDMA_RX_TDESC_OFFSET, lp->rx_bd_p +
+			     (sizeof(*lp->rx_bd_v) * (lp->rx_bd_num - 1)));
 
 	/* Write to the RS (Run-stop) bit in the Tx channel control register.
 	 * Tx channel is now ready to run. But only after we write to the
 	 * tail pointer register that the Tx channel will start transmitting.
 	 */
-	axienet_dma_out32(lp, XAXIDMA_TX_CDESC_OFFSET, lp->tx_bd_p);
+	axienet_dma_out_addr(lp, XAXIDMA_TX_CDESC_OFFSET, lp->tx_bd_p);
 	cr = axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET);
 	axienet_dma_out32(lp, XAXIDMA_TX_CR_OFFSET,
 			  cr | XAXIDMA_CR_RUNSTOP_MASK);
@@ -739,7 +745,7 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	tail_p = lp->tx_bd_p + sizeof(*lp->tx_bd_v) * lp->tx_bd_tail;
 	/* Start the transfer */
-	axienet_dma_out32(lp, XAXIDMA_TX_TDESC_OFFSET, tail_p);
+	axienet_dma_out_addr(lp, XAXIDMA_TX_TDESC_OFFSET, tail_p);
 	if (++lp->tx_bd_tail >= lp->tx_bd_num)
 		lp->tx_bd_tail = 0;
 
@@ -829,7 +835,7 @@ static void axienet_recv(struct net_device *ndev)
 	ndev->stats.rx_bytes += size;
 
 	if (tail_p)
-		axienet_dma_out32(lp, XAXIDMA_RX_TDESC_OFFSET, tail_p);
+		axienet_dma_out_addr(lp, XAXIDMA_RX_TDESC_OFFSET, tail_p);
 }
 
 /**
@@ -1677,18 +1683,18 @@ static void axienet_dma_err_handler(unsigned long data)
 	/* Populate the tail pointer and bring the Rx Axi DMA engine out of
 	 * halted state. This will make the Rx side ready for reception.
 	 */
-	axienet_dma_out32(lp, XAXIDMA_RX_CDESC_OFFSET, lp->rx_bd_p);
+	axienet_dma_out_addr(lp, XAXIDMA_RX_CDESC_OFFSET, lp->rx_bd_p);
 	cr = axienet_dma_in32(lp, XAXIDMA_RX_CR_OFFSET);
 	axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET,
 			  cr | XAXIDMA_CR_RUNSTOP_MASK);
-	axienet_dma_out32(lp, XAXIDMA_RX_TDESC_OFFSET, lp->rx_bd_p +
-			  (sizeof(*lp->rx_bd_v) * (lp->rx_bd_num - 1)));
+	axienet_dma_out_addr(lp, XAXIDMA_RX_TDESC_OFFSET, lp->rx_bd_p +
+			     (sizeof(*lp->rx_bd_v) * (lp->rx_bd_num - 1)));
 
 	/* Write to the RS (Run-stop) bit in the Tx channel control register.
 	 * Tx channel is now ready to run. But only after we write to the
 	 * tail pointer register that the Tx channel will start transmitting
 	 */
-	axienet_dma_out32(lp, XAXIDMA_TX_CDESC_OFFSET, lp->tx_bd_p);
+	axienet_dma_out_addr(lp, XAXIDMA_TX_CDESC_OFFSET, lp->tx_bd_p);
 	cr = axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET);
 	axienet_dma_out32(lp, XAXIDMA_TX_CR_OFFSET,
 			  cr | XAXIDMA_CR_RUNSTOP_MASK);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 11/14] net: axienet: Upgrade descriptors to hold 64-bit addresses
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (9 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 10/14] net: axienet: Wrap DMA pointer writes to prepare for 64 bit Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-14 16:35   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability Andre Przywara
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

Newer revisions of the AXI DMA IP (>= v7.1) support 64-bit addresses,
both for the descriptors itself, as well as for the buffers they are
pointing to.
This is realised by adding "MSB" words for the next and phys pointer
right behind the existing address word, now named "LSB". These MSB words
live in formerly reserved areas of the descriptor.

If the hardware supports it, write both words when setting an address.
The buffer address is handled by two wrapper functions, the two
occasions where we set the next pointers are open coded.

For now this is guarded by a flag which we don't set yet.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet.h  |   9 +-
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 109 ++++++++++++------
 2 files changed, 81 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
index 2dacfc85b3ba..4aea4c23d3bb 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
@@ -335,6 +335,7 @@
 #define XAE_FEATURE_PARTIAL_TX_CSUM	(1 << 1)
 #define XAE_FEATURE_FULL_RX_CSUM	(1 << 2)
 #define XAE_FEATURE_FULL_TX_CSUM	(1 << 3)
+#define XAE_FEATURE_DMA_64BIT		(1 << 4)
 
 #define XAE_NO_CSUM_OFFLOAD		0
 
@@ -347,9 +348,9 @@
 /**
  * struct axidma_bd - Axi Dma buffer descriptor layout
  * @next:         MM2S/S2MM Next Descriptor Pointer
- * @reserved1:    Reserved and not used
+ * @next_msb:     MM2S/S2MM Next Descriptor Pointer (high 32 bits)
  * @phys:         MM2S/S2MM Buffer Address
- * @reserved2:    Reserved and not used
+ * @phys_msb:     MM2S/S2MM Buffer Address (high 32 bits)
  * @reserved3:    Reserved and not used
  * @reserved4:    Reserved and not used
  * @cntrl:        MM2S/S2MM Control value
@@ -362,9 +363,9 @@
  */
 struct axidma_bd {
 	u32 next;	/* Physical address of next buffer descriptor */
-	u32 reserved1;
+	u32 next_msb;	/* high 32 bits for IP >= v7.1, reserved on older IP */
 	u32 phys;
-	u32 reserved2;
+	u32 phys_msb; 	/* for IP >= v7.1, reserved for older IP */
 	u32 reserved3;
 	u32 reserved4;
 	u32 cntrl;
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index bbdda4b0c677..133f088d797e 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -153,6 +153,25 @@ static void axienet_dma_out_addr(struct axienet_local *lp, off_t reg,
 	axienet_dma_out32(lp, reg, lower_32_bits(addr));
 }
 
+static void desc_set_phys_addr(struct axienet_local *lp, dma_addr_t addr,
+			       struct axidma_bd *desc)
+{
+	desc->phys = lower_32_bits(addr);
+	if (lp->features & XAE_FEATURE_DMA_64BIT)
+		desc->phys_msb = upper_32_bits(addr);
+}
+
+static dma_addr_t desc_get_phys_addr(struct axienet_local *lp,
+				     struct axidma_bd *desc)
+{
+	dma_addr_t ret = desc->phys;
+
+	if (lp->features & XAE_FEATURE_DMA_64BIT)
+		ret |= (dma_addr_t)desc->phys_msb << 32;
+
+	return ret;
+}
+
 /**
  * axienet_dma_bd_release - Release buffer descriptor rings
  * @ndev:	Pointer to the net_device structure
@@ -176,6 +195,8 @@ static void axienet_dma_bd_release(struct net_device *ndev)
 		return;
 
 	for (i = 0; i < lp->rx_bd_num; i++) {
+		dma_addr_t phys;
+
 		/* A NULL skb means this descriptor has not been initialised
 		 * at all.
 		 */
@@ -188,9 +209,11 @@ static void axienet_dma_bd_release(struct net_device *ndev)
 		 * descriptor size, after it had been successfully allocated.
 		 * So a non-zero value in there means we need to unmap it.
 		 */
-		if (lp->rx_bd_v[i].cntrl)
-			dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
+		if (lp->rx_bd_v[i].cntrl) {
+			phys = desc_get_phys_addr(lp, &lp->rx_bd_v[i]);
+			dma_unmap_single(ndev->dev.parent, phys,
 					 lp->max_frm_size, DMA_FROM_DEVICE);
+		}
 	}
 
 	dma_free_coherent(ndev->dev.parent,
@@ -235,29 +258,36 @@ static int axienet_dma_bd_init(struct net_device *ndev)
 		goto out;
 
 	for (i = 0; i < lp->tx_bd_num; i++) {
-		lp->tx_bd_v[i].next = lp->tx_bd_p +
-				      sizeof(*lp->tx_bd_v) *
-				      ((i + 1) % lp->tx_bd_num);
+		dma_addr_t addr = lp->tx_bd_p +
+				  sizeof(*lp->tx_bd_v) *
+				  ((i + 1) % lp->tx_bd_num);
+
+		lp->tx_bd_v[i].next = lower_32_bits(addr);
+		if (lp->features & XAE_FEATURE_DMA_64BIT)
+			lp->tx_bd_v[i].next_msb = upper_32_bits(addr);
 	}
 
 	for (i = 0; i < lp->rx_bd_num; i++) {
-		lp->rx_bd_v[i].next = lp->rx_bd_p +
-				      sizeof(*lp->rx_bd_v) *
-				      ((i + 1) % lp->rx_bd_num);
+		dma_addr_t addr;
+
+		addr = lp->rx_bd_p + sizeof(*lp->rx_bd_v) *
+			((i + 1) % lp->rx_bd_num);
+		lp->rx_bd_v[i].next = lower_32_bits(addr);
+		if (lp->features & XAE_FEATURE_DMA_64BIT)
+			lp->rx_bd_v[i].next_msb = upper_32_bits(addr);
 
 		skb = netdev_alloc_skb_ip_align(ndev, lp->max_frm_size);
 		if (!skb)
 			goto out;
 
 		lp->rx_bd_v[i].skb = skb;
-		lp->rx_bd_v[i].phys = dma_map_single(ndev->dev.parent,
-						     skb->data,
-						     lp->max_frm_size,
-						     DMA_FROM_DEVICE);
-		if (dma_mapping_error(ndev->dev.parent, lp->rx_bd_v[i].phys)) {
+		addr = dma_map_single(ndev->dev.parent, skb->data,
+				      lp->max_frm_size, DMA_FROM_DEVICE);
+		if (dma_mapping_error(ndev->dev.parent, addr)) {
 			dev_kfree_skb(skb);
 			goto out;
 		}
+		desc_set_phys_addr(lp, addr, &lp->rx_bd_v[i]);
 
 		lp->rx_bd_v[i].cntrl = lp->max_frm_size;
 	}
@@ -565,6 +595,7 @@ static int axienet_free_tx_chain(struct net_device *ndev, u32 first_bd,
 	struct axienet_local *lp = netdev_priv(ndev);
 	int max_bds = (nr_bds != -1) ? nr_bds : lp->tx_bd_num;
 	struct axidma_bd *cur_p;
+	dma_addr_t phys;
 	unsigned int status;
 	int i;
 
@@ -578,7 +609,8 @@ static int axienet_free_tx_chain(struct net_device *ndev, u32 first_bd,
 		if (nr_bds == -1 && !(status & XAXIDMA_BD_STS_COMPLETE_MASK))
 			break;
 
-		dma_unmap_single(ndev->dev.parent, cur_p->phys,
+		phys = desc_get_phys_addr(lp, cur_p);
+		dma_unmap_single(ndev->dev.parent, phys,
 				(cur_p->cntrl & XAXIDMA_BD_CTRL_LENGTH_MASK),
 				DMA_TO_DEVICE);
 
@@ -676,7 +708,7 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	u32 csum_start_off;
 	u32 csum_index_off;
 	skb_frag_t *frag;
-	dma_addr_t tail_p;
+	dma_addr_t tail_p, phys;
 	struct axienet_local *lp = netdev_priv(ndev);
 	struct axidma_bd *cur_p;
 	u32 orig_tail_ptr = lp->tx_bd_tail;
@@ -715,10 +747,11 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		cur_p->app0 |= 2; /* Tx Full Checksum Offload Enabled */
 	}
 
-	cur_p->phys = dma_map_single(ndev->dev.parent, skb->data,
-				     skb_headlen(skb), DMA_TO_DEVICE);
-	if (dma_mapping_error(ndev->dev.parent, cur_p->phys))
+	phys = dma_map_single(ndev->dev.parent, skb->data,
+			      skb_headlen(skb), DMA_TO_DEVICE);
+	if (dma_mapping_error(ndev->dev.parent, phys))
 		return NETDEV_TX_BUSY;
+	desc_set_phys_addr(lp, phys, cur_p);
 	cur_p->cntrl = skb_headlen(skb) | XAXIDMA_BD_CTRL_TXSOF_MASK;
 
 	for (ii = 0; ii < num_frag; ii++) {
@@ -726,17 +759,18 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 			lp->tx_bd_tail = 0;
 		cur_p = &lp->tx_bd_v[lp->tx_bd_tail];
 		frag = &skb_shinfo(skb)->frags[ii];
-		cur_p->phys = dma_map_single(ndev->dev.parent,
-					     skb_frag_address(frag),
-					     skb_frag_size(frag),
-					     DMA_TO_DEVICE);
-		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
+		phys = dma_map_single(ndev->dev.parent,
+				      skb_frag_address(frag),
+				      skb_frag_size(frag),
+				      DMA_TO_DEVICE);
+		if (dma_mapping_error(ndev->dev.parent, phys)) {
 			axienet_free_tx_chain(ndev, orig_tail_ptr, ii + 1,
 					      NULL);
 			lp->tx_bd_tail = orig_tail_ptr;
 
 			return NETDEV_TX_BUSY;
 		}
+		desc_set_phys_addr(lp, phys, cur_p);
 		cur_p->cntrl = skb_frag_size(frag);
 	}
 
@@ -775,10 +809,12 @@ static void axienet_recv(struct net_device *ndev)
 	cur_p = &lp->rx_bd_v[lp->rx_bd_ci];
 
 	while ((cur_p->status & XAXIDMA_BD_STS_COMPLETE_MASK)) {
+		dma_addr_t phys;
+
 		tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci;
 
-		dma_unmap_single(ndev->dev.parent, cur_p->phys,
-				 lp->max_frm_size,
+		phys = desc_get_phys_addr(lp, cur_p);
+		dma_unmap_single(ndev->dev.parent, phys, lp->max_frm_size,
 				 DMA_FROM_DEVICE);
 
 		skb = cur_p->skb;
@@ -814,13 +850,14 @@ static void axienet_recv(struct net_device *ndev)
 		if (!new_skb)
 			return;
 
-		cur_p->phys = dma_map_single(ndev->dev.parent, new_skb->data,
-					     lp->max_frm_size,
-					     DMA_FROM_DEVICE);
-		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
+		phys = dma_map_single(ndev->dev.parent, new_skb->data,
+				      lp->max_frm_size,
+				      DMA_FROM_DEVICE);
+		if (dma_mapping_error(ndev->dev.parent, phys)) {
 			dev_kfree_skb(new_skb);
 			return;
 		}
+		desc_set_phys_addr(lp, phys, cur_p);
 
 		cur_p->cntrl = lp->max_frm_size;
 		cur_p->status = 0;
@@ -865,7 +902,8 @@ static irqreturn_t axienet_tx_irq(int irq, void *_ndev)
 		return IRQ_NONE;
 	if (status & XAXIDMA_IRQ_ERROR_MASK) {
 		dev_err(&ndev->dev, "DMA Tx error 0x%x\n", status);
-		dev_err(&ndev->dev, "Current BD is at: 0x%x\n",
+		dev_err(&ndev->dev, "Current BD is at: 0x%x%08x\n",
+			(lp->tx_bd_v[lp->tx_bd_ci]).phys_msb,
 			(lp->tx_bd_v[lp->tx_bd_ci]).phys);
 
 		cr = axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET);
@@ -914,7 +952,8 @@ static irqreturn_t axienet_rx_irq(int irq, void *_ndev)
 		return IRQ_NONE;
 	if (status & XAXIDMA_IRQ_ERROR_MASK) {
 		dev_err(&ndev->dev, "DMA Rx error 0x%x\n", status);
-		dev_err(&ndev->dev, "Current BD is at: 0x%x\n",
+		dev_err(&ndev->dev, "Current BD is at: 0x%x%08x\n",
+			(lp->rx_bd_v[lp->rx_bd_ci]).phys_msb,
 			(lp->rx_bd_v[lp->rx_bd_ci]).phys);
 
 		cr = axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET);
@@ -1622,14 +1661,18 @@ static void axienet_dma_err_handler(unsigned long data)
 
 	for (i = 0; i < lp->tx_bd_num; i++) {
 		cur_p = &lp->tx_bd_v[i];
-		if (cur_p->cntrl)
-			dma_unmap_single(ndev->dev.parent, cur_p->phys,
+		if (cur_p->cntrl) {
+			dma_addr_t addr = desc_get_phys_addr(lp, cur_p);
+
+			dma_unmap_single(ndev->dev.parent, addr,
 					 (cur_p->cntrl &
 					  XAXIDMA_BD_CTRL_LENGTH_MASK),
 					 DMA_TO_DEVICE);
+		}
 		if (cur_p->skb)
 			dev_kfree_skb_irq(cur_p->skb);
 		cur_p->phys = 0;
+		cur_p->phys_msb = 0;
 		cur_p->cntrl = 0;
 		cur_p->status = 0;
 		cur_p->app0 = 0;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (10 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 11/14] net: axienet: Upgrade descriptors to hold 64-bit addresses Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-10 14:08   ` Andrew Lunn
  2020-01-14 17:03   ` Radhey Shyam Pandey
  2020-01-10 11:54 ` [PATCH 13/14] net: axienet: Allow DMA to beyond 4GB Andre Przywara
  2020-01-10 11:54 ` [PATCH 14/14] net: axienet: Update devicetree binding documentation Andre Przywara
  13 siblings, 2 replies; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Robert Hancock, netdev, Michal Simek, linux-arm-kernel, linux-kernel

When newer revisions of the Axienet IP are configured for a 64-bit bus,
we *need* to write to the MSB part of the an address registers,
otherwise the IP won't recognise this as a DMA start condition.
This is even true when the actual DMA address comes from the lower 4 GB.

To autodetect this configuration, at probe time we write all 1's to such
an MSB register, and see if any bits stick. If this is configured for a
32-bit bus, those MSB registers are RES0, so reading back 0 indicates
that no MSB writes are necessary.
On the other hands reading anything other than 0 indicated the need to
write the MSB registers, so we set the respective flag.

For now this leaves the actual DMA mask at 32-bit, as we can't reliably
detect the actually wired number of address lines beyond 32.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet.h  |  1 +
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 27 +++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
index 4aea4c23d3bb..4feaaa02819c 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
@@ -161,6 +161,7 @@
 #define XAE_FCC_OFFSET		0x0000040C /* Flow Control Configuration */
 #define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
 #define XAE_PHYC_OFFSET		0x00000414 /* RGMII/SGMII configuration */
+#define XAE_ID_OFFSET		0x000004F8 /* Identification register */
 #define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
 #define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
 #define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data */
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 133f088d797e..f7f593df0c11 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -151,6 +151,9 @@ static void axienet_dma_out_addr(struct axienet_local *lp, off_t reg,
 				 dma_addr_t addr)
 {
 	axienet_dma_out32(lp, reg, lower_32_bits(addr));
+
+	if (lp->features & XAE_FEATURE_DMA_64BIT)
+		axienet_dma_out32(lp, reg + 4, upper_32_bits(addr));
 }
 
 static void desc_set_phys_addr(struct axienet_local *lp, dma_addr_t addr,
@@ -1934,6 +1937,30 @@ static int axienet_probe(struct platform_device *pdev)
 		goto free_netdev;
 	}
 
+	/*
+	 * Autodetect the need for 64-bit DMA pointers.
+	 * When the IP is configured for a bus width bigger than 32 bits,
+	 * writing the MSB registers is mandatory, even if they are all 0.
+	 * We can detect this case by writing all 1's to one such register
+	 * and see if that sticks: when the IP is configured for 32 bits
+	 * only, those registers are RES0.
+	 * Those MSB registers were introduced in IP v7.1, which we check first.
+	 */
+	if ((axienet_ior(lp, XAE_ID_OFFSET) >> 24) >= 0x9) {
+		void __iomem *desc = lp->dma_regs + XAXIDMA_TX_CDESC_OFFSET + 4;
+
+		iowrite32(0x0, desc);
+		if (ioread32(desc) == 0) {	/* sanity check */
+			iowrite32(0xffffffff, desc);
+			if (ioread32(desc) > 0) {
+				lp->features |= XAE_FEATURE_DMA_64BIT;
+				dev_info(&pdev->dev,
+					 "autodetected 64-bit DMA range\n");
+			}
+			iowrite32(0x0, desc);
+		}
+	}
+
 	/* Check for Ethernet core IRQ (optional) */
 	if (lp->eth_irq <= 0)
 		dev_info(&pdev->dev, "Ethernet core IRQ not defined\n");
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 13/14] net: axienet: Allow DMA to beyond 4GB
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (11 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-10 11:54 ` [PATCH 14/14] net: axienet: Update devicetree binding documentation Andre Przywara
  13 siblings, 0 replies; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Mark Rutland, devicetree, netdev, Michal Simek, linux-kernel,
	Robert Hancock, Rob Herring, linux-arm-kernel

With all DMA address accesses wrapped, we can actually support 64-bit
DMA if this option was chosen at IP integration time.
Since there is no way to autodetect the actual address bus width, we
make use of the existing "xlnx,addrwidth" DT property to let the driver
know about the width. The value in this property should match the
"Address Width" parameter used when synthesizing the IP.

This increases the DMA mask to let the kernel choose buffers from
memory at higher addresses.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index f7f593df0c11..e036834549b3 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1786,6 +1786,7 @@ static int axienet_probe(struct platform_device *pdev)
 	struct net_device *ndev;
 	const void *mac_addr;
 	struct resource *ethres;
+	int addr_width = 32;
 	u32 value;
 
 	ndev = alloc_etherdev(sizeof(*lp));
@@ -1915,6 +1916,8 @@ static int axienet_probe(struct platform_device *pdev)
 						     &dmares);
 		lp->rx_irq = irq_of_parse_and_map(np, 1);
 		lp->tx_irq = irq_of_parse_and_map(np, 0);
+		of_property_read_u32(np, "xlnx,addrwidth", &addr_width);
+
 		of_node_put(np);
 		lp->eth_irq = platform_get_irq(pdev, 0);
 	} else {
@@ -1944,6 +1947,9 @@ static int axienet_probe(struct platform_device *pdev)
 	 * We can detect this case by writing all 1's to one such register
 	 * and see if that sticks: when the IP is configured for 32 bits
 	 * only, those registers are RES0.
+	 * We can't autodetect the actual width this way, so we still use
+	 * a 32-bit DMA mask and rely on the xlnk,addrwidth DT property
+	 * to set this properly.
 	 * Those MSB registers were introduced in IP v7.1, which we check first.
 	 */
 	if ((axienet_ior(lp, XAE_ID_OFFSET) >> 24) >= 0x9) {
@@ -1961,6 +1967,19 @@ static int axienet_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (!(lp->features & XAE_FEATURE_DMA_64BIT)) {
+		if (addr_width > 32)
+			dev_warn(&pdev->dev, "trimming DMA width from %d to 32 bits\n",
+				 addr_width);
+		addr_width = 32;
+	}
+
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(addr_width));
+	if (ret) {
+		dev_err(&pdev->dev, "No suitable DMA available\n");
+		goto free_netdev;
+	}
+
 	/* Check for Ethernet core IRQ (optional) */
 	if (lp->eth_irq <= 0)
 		dev_info(&pdev->dev, "Ethernet core IRQ not defined\n");
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* [PATCH 14/14] net: axienet: Update devicetree binding documentation
  2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
                   ` (12 preceding siblings ...)
  2020-01-10 11:54 ` [PATCH 13/14] net: axienet: Allow DMA to beyond 4GB Andre Przywara
@ 2020-01-10 11:54 ` Andre Przywara
  2020-01-21 21:51   ` Rob Herring
  13 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 11:54 UTC (permalink / raw)
  To: David S . Miller, Radhey Shyam Pandey
  Cc: Mark Rutland, devicetree, netdev, Michal Simek, linux-kernel,
	Robert Hancock, Rob Herring, linux-arm-kernel

This adds documentation about the newly introduced, optional
"xlnx,addrwidth" property to the binding documentation.

While at it, clarify the wording on some properties, replace obsolete
.txt file references with their new .yaml counterparts, and add a more
modern example, using the axistream-connected property.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../bindings/net/xilinx_axienet.txt           | 57 ++++++++++++++++---
 1 file changed, 50 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/xilinx_axienet.txt b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
index 7360617cdedb..78c278c5200f 100644
--- a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
+++ b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
@@ -12,7 +12,8 @@ sent and received through means of an AXI DMA controller. This driver
 includes the DMA driver code, so this driver is incompatible with AXI DMA
 driver.
 
-For more details about mdio please refer phy.txt file in the same directory.
+For more details about mdio please refer to the ethernet-phy.yaml file in
+the same directory.
 
 Required properties:
 - compatible	: Must be one of "xlnx,axi-ethernet-1.00.a",
@@ -27,14 +28,14 @@ Required properties:
 		  instead, and only the Ethernet core interrupt is optionally
 		  specified here.
 - phy-handle	: Should point to the external phy device.
-		  See ethernet.txt file in the same directory.
-- xlnx,rxmem	: Set to allocated memory buffer for Rx/Tx in the hardware
+		  See the ethernet-controller.yaml file in the same directory.
+- xlnx,rxmem	: Size of the RXMEM buffer in the hardware, in bytes.
 
 Optional properties:
-- phy-mode	: See ethernet.txt
+- phy-mode	: See ethernet-controller.yaml.
 - xlnx,phy-type	: Deprecated, do not use, but still accepted in preference
 		  to phy-mode.
-- xlnx,txcsum	: 0 or empty for disabling TX checksum offload,
+- xlnx,txcsum	: 0 for disabling TX checksum offload (default if omitted),
 		  1 to enable partial TX checksum offload,
 		  2 to enable full TX checksum offload
 - xlnx,rxcsum	: Same values as xlnx,txcsum but for RX checksum offload
@@ -48,10 +49,20 @@ Optional properties:
 		       If this is specified, the DMA-related resources from that
 		       device (DMA registers and DMA TX/RX interrupts) rather
 		       than this one will be used.
- - mdio		: Child node for MDIO bus. Must be defined if PHY access is
+- mdio		: Child node for MDIO bus. Must be defined if PHY access is
 		  required through the core's MDIO interface (i.e. always,
 		  unless the PHY is accessed through a different bus).
 
+Required properties for axistream-connected subnode:
+- reg		: Address and length of the AXI DMA controller MMIO space.
+- interrupts	: A list of 2 interrupts: TX DMA and RX DMA, in that order.
+
+Optional properties for axistream-connected subnode:
+- xlnx,addrwidth: Specifies the configured address width of the DMA. Newer
+		  versions of the IP allow setting this to a value between
+		  32 and 64. Defaults to 32 bits if not specified.
+
+
 Example:
 	axi_ethernet_eth: ethernet@40c00000 {
 		compatible = "xlnx,axi-ethernet-1.00.a";
@@ -60,7 +71,7 @@ Example:
 		interrupts = <2 0 1>;
 		clocks = <&axi_clk>;
 		phy-mode = "mii";
-		reg = <0x40c00000 0x40000 0x50c00000 0x40000>;
+		reg = <0x40c00000 0x40000>, <0x50c00000 0x40000>;
 		xlnx,rxcsum = <0x2>;
 		xlnx,rxmem = <0x800>;
 		xlnx,txcsum = <0x2>;
@@ -74,3 +85,35 @@ Example:
 			};
 		};
 	};
+    -----------------
+	axi_ethernet_eth: ethernet@7fe00000 {
+		compatible = "acme,fpga-ethernet", "xlnx,axi-ethernet-1.00.a";
+		reg = <0 0x7fe00000 0 0x40000>;
+		interrupts = <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&soc_refclk100mhz>;
+
+		phy-mode = "sgmii";
+		phy-handle = <&phy0>;
+
+		xlnx,rxmem = <4096>;
+		axistream-connected = <&axi_dma_eth>;
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		axi_dma_eth: axi_dma_ethernet@7fe40000 {
+			reg = <0 0x7fe40000 0 0x10000>;
+			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+			xlnx,addrwidth = <40>;
+		};
+
+		axi_ethernetlite_0_mdio: mdio {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			phy0: phy@1 {
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <1>;
+			};
+		};
+	};
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 11:54 ` [PATCH 07/14] net: axienet: Fix SGMII support Andre Przywara
@ 2020-01-10 14:04   ` Andrew Lunn
  2020-01-10 14:20     ` Andre Przywara
  2020-01-10 14:58   ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 52+ messages in thread
From: Andrew Lunn @ 2020-01-10 14:04 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, Russell King, David S . Miller, linux-arm-kernel

On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> With SGMII, the MAC and the PHY can negotiate the link speed between
> themselves, without the host needing to mediate between them.
> Linux recognises this, and will call phylink's mac_config with the speed
> member set to SPEED_UNKNOWN (-1).
> Currently the axienet driver will bail out and complain about an
> unsupported link speed.
> 
> Teach axienet's mac_config callback to leave the MAC's speed setting
> alone if the requested speed is SPEED_UNKNOWN.

Hi Andre

Is there an interrupt when SGMII signals a change in link state? If
so, you should call phylink_mac_change().

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
  2020-01-10 11:54 ` [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability Andre Przywara
@ 2020-01-10 14:08   ` Andrew Lunn
  2020-01-10 14:13     ` Andre Przywara
  2020-01-14 17:03   ` Radhey Shyam Pandey
  1 sibling, 1 reply; 52+ messages in thread
From: Andrew Lunn @ 2020-01-10 14:08 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, David S . Miller, linux-arm-kernel

> To autodetect this configuration, at probe time we write all 1's to such
> an MSB register, and see if any bits stick.

So there is no register you can read containing the IP version?

   Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
  2020-01-10 14:08   ` Andrew Lunn
@ 2020-01-10 14:13     ` Andre Przywara
  2020-01-10 14:22       ` Andrew Lunn
  0 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 14:13 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, 10 Jan 2020 15:08:52 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

Hi Andrew,

thanks for having a look!

> > To autodetect this configuration, at probe time we write all 1's to such
> > an MSB register, and see if any bits stick.  
> 
> So there is no register you can read containing the IP version?

There is, and I actually read this before doing this check. But the 64-bit DMA capability is optional even in this revision. It depends on what you give it as the address width. If you say 32, the IP config tool disables the 64-bit capability completely, so it stays compatible with older revisions.
Anything beyond 32 will enable the MSB register and will also require you to write to them.

Cheers,
Andre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 01/14] net: xilinx: temac: Relax Kconfig dependencies
  2020-01-10 11:54 ` [PATCH 01/14] net: xilinx: temac: Relax Kconfig dependencies Andre Przywara
@ 2020-01-10 14:19   ` Radhey Shyam Pandey
  0 siblings, 0 replies; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-10 14:19 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 01/14] net: xilinx: temac: Relax Kconfig dependencies
> 
> Similar to axienet, the temac driver is now architecture agnostic, and
> can be at least compiled for several architectures.
> Especially the fact that this is a soft IP for implementing in FPGAs
> makes the current restriction rather pointless, as it could literally
> appear on any architecture, as long as an FPGA is connected to the bus.
> 
> The driver hasn't been actually tried on any hardware, it is just a
> drive-by patch when doing the same for axienet (a similar patch for
> axienet is already merged).
> 
> This (temac and axienet) have been compile-tested for:
> alpha hppa64 microblaze mips64 powerpc powerpc64 riscv64 s390 sparc64
> (using kernel.org cross compilers).
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>

Thanks!
> ---
>  drivers/net/ethernet/xilinx/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/Kconfig
> b/drivers/net/ethernet/xilinx/Kconfig
> index 6304ebd8b5c6..0810af8193cb 100644
> --- a/drivers/net/ethernet/xilinx/Kconfig
> +++ b/drivers/net/ethernet/xilinx/Kconfig
> @@ -32,7 +32,6 @@ config XILINX_AXI_EMAC
> 
>  config XILINX_LL_TEMAC
>  	tristate "Xilinx LL TEMAC (LocalLink Tri-mode Ethernet MAC) driver"
> -	depends on PPC || MICROBLAZE || X86 || COMPILE_TEST
>  	select PHYLIB
>  	---help---
>  	  This driver supports the Xilinx 10/100/1000 LocalLink TEMAC
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 14:04   ` Andrew Lunn
@ 2020-01-10 14:20     ` Andre Przywara
  2020-01-10 14:26       ` Andrew Lunn
  2020-01-10 15:04       ` Russell King - ARM Linux admin
  0 siblings, 2 replies; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 14:20 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, Russell King, David S . Miller, linux-arm-kernel

On Fri, 10 Jan 2020 15:04:15 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

Hi Andrew,

> On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > With SGMII, the MAC and the PHY can negotiate the link speed between
> > themselves, without the host needing to mediate between them.
> > Linux recognises this, and will call phylink's mac_config with the speed
> > member set to SPEED_UNKNOWN (-1).
> > Currently the axienet driver will bail out and complain about an
> > unsupported link speed.
> > 
> > Teach axienet's mac_config callback to leave the MAC's speed setting
> > alone if the requested speed is SPEED_UNKNOWN.  
> 
> Hi Andre
> 
> Is there an interrupt when SGMII signals a change in link state? If
> so, you should call phylink_mac_change().

Good point. The doc describes a "Auto-Negotiation Complete" interrupt status bit, which signal that " ... auto-negotiation of the SGMII or 1000BASE-X interface has completed."
But I have no clue whether that would trigger on a link status *change*. Is there a way to test this without pulling the cable? My board sits in a data centre, so is not easily accessible to me.

Cheers,
Andre.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
  2020-01-10 14:13     ` Andre Przywara
@ 2020-01-10 14:22       ` Andrew Lunn
  2020-01-10 15:08         ` Andre Przywara
  0 siblings, 1 reply; 52+ messages in thread
From: Andrew Lunn @ 2020-01-10 14:22 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, Jan 10, 2020 at 02:13:03PM +0000, Andre Przywara wrote:
> On Fri, 10 Jan 2020 15:08:52 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> Hi Andrew,
> 
> thanks for having a look!
> 
> > > To autodetect this configuration, at probe time we write all 1's to such
> > > an MSB register, and see if any bits stick.  
> > 
> > So there is no register you can read containing the IP version?
> 
> There is, and I actually read this before doing this check. But the 64-bit DMA capability is optional even in this revision. It depends on what you give it as the address width. If you say 32, the IP config tool disables the 64-bit capability completely, so it stays compatible with older revisions.
> Anything beyond 32 will enable the MSB register and will also require you to write to them.

So you are saying there is no way to enumerate the synthesised
configuration of the IP. Great :-(

Do Xilinx at least document you can discover the DMA size by writing
into these upper bits? Does Xilinx own 'vendor crap' driver do this?

Thanks
	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 14:20     ` Andre Przywara
@ 2020-01-10 14:26       ` Andrew Lunn
  2020-01-10 15:04       ` Russell King - ARM Linux admin
  1 sibling, 0 replies; 52+ messages in thread
From: Andrew Lunn @ 2020-01-10 14:26 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, Russell King, David S . Miller, linux-arm-kernel

> But I have no clue whether that would trigger on a link status
> *change*.

It should do, but without testing...

> Is there a way to test this without pulling the cable? My board sits
> in a data centre, so is not easily accessible to me.

Down and up the other end?

     Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 02/14] net: axienet: Propagate failure of DMA descriptor setup
  2020-01-10 11:54 ` [PATCH 02/14] net: axienet: Propagate failure of DMA descriptor setup Andre Przywara
@ 2020-01-10 14:54   ` Radhey Shyam Pandey
  2020-01-10 17:53     ` Radhey Shyam Pandey
  0 siblings, 1 reply; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-10 14:54 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 02/14] net: axienet: Propagate failure of DMA descriptor
> setup
> 
> When we fail allocating the DMA buffers in axienet_dma_bd_init(), we
> report this error, but carry on with initialisation nevertheless.
> 
> This leads to a kernel panic when the driver later wants to send a
> packet, as it uses uninitialised data structures.
> 
> Make the axienet_device_reset() routine return an error value, as it
> contains the DMA buffer initialisation. Make sure we propagate the error
> up the chain and eventually fail the driver initialisation, to avoid
> relying on non-initialised buffers.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>

> ---
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 25 +++++++++++++------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 20746b801959..97482cf093ce 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -437,9 +437,10 @@ static void axienet_setoptions(struct net_device
> *ndev, u32 options)
>  	lp->options |= options;
>  }
> 
> -static void __axienet_device_reset(struct axienet_local *lp)
> +static int __axienet_device_reset(struct axienet_local *lp)
>  {
>  	u32 timeout;
> +
>  	/* Reset Axi DMA. This would reset Axi Ethernet core as well. The
> reset
>  	 * process of Axi DMA takes a while to complete as all pending
>  	 * commands/transfers will be flushed or completed during this
> @@ -455,9 +456,11 @@ static void __axienet_device_reset(struct
> axienet_local *lp)
>  		if (--timeout == 0) {
>  			netdev_err(lp->ndev, "%s: DMA reset timeout!\n",
>  				   __func__);
> -			break;
> +			return -ETIMEDOUT;
>  		}
>  	}
> +
> +	return 0;
>  }
> 
>  /**
> @@ -471,12 +474,15 @@ static void __axienet_device_reset(struct
> axienet_local *lp)
>   * Ethernet core. No separate hardware reset is done for the Axi Ethernet
>   * core.
>   */
> -static void axienet_device_reset(struct net_device *ndev)
> +static int axienet_device_reset(struct net_device *ndev)
>  {
>  	u32 axienet_status;
>  	struct axienet_local *lp = netdev_priv(ndev);
> +	int ret;
> 
> -	__axienet_device_reset(lp);
> +	ret = __axienet_device_reset(lp);
> +	if (ret)
> +		return ret;
> 
>  	lp->max_frm_size = XAE_MAX_VLAN_FRAME_SIZE;
>  	lp->options |= XAE_OPTION_VLAN;
> @@ -491,9 +497,11 @@ static void axienet_device_reset(struct net_device
> *ndev)
>  			lp->options |= XAE_OPTION_JUMBO;
>  	}
> 
> -	if (axienet_dma_bd_init(ndev)) {
> +	ret = axienet_dma_bd_init(ndev);
> +	if (ret) {
>  		netdev_err(ndev, "%s: descriptor allocation failed\n",
>  			   __func__);
> +		return ret;
>  	}
> 
>  	axienet_status = axienet_ior(lp, XAE_RCW1_OFFSET);
> @@ -518,6 +526,8 @@ static void axienet_device_reset(struct net_device
> *ndev)
>  	axienet_setoptions(ndev, lp->options);
> 
>  	netif_trans_update(ndev);
> +
> +	return 0;
>  }
> 
>  /**
> @@ -921,8 +931,9 @@ static int axienet_open(struct net_device *ndev)
>  	 */
>  	mutex_lock(&lp->mii_bus->mdio_lock);
>  	axienet_mdio_disable(lp);
> -	axienet_device_reset(ndev);
> -	ret = axienet_mdio_enable(lp);
> +	ret = axienet_device_reset(ndev);
> +	if (ret == 0)
> +		ret = axienet_mdio_enable(lp);
>  	mutex_unlock(&lp->mii_bus->mdio_lock);
>  	if (ret < 0)
>  		return ret;
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 11:54 ` [PATCH 07/14] net: axienet: Fix SGMII support Andre Przywara
  2020-01-10 14:04   ` Andrew Lunn
@ 2020-01-10 14:58   ` Russell King - ARM Linux admin
  2020-01-10 17:32     ` Andre Przywara
  1 sibling, 1 reply; 52+ messages in thread
From: Russell King - ARM Linux admin @ 2020-01-10 14:58 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> With SGMII, the MAC and the PHY can negotiate the link speed between
> themselves, without the host needing to mediate between them.
> Linux recognises this, and will call phylink's mac_config with the speed
> member set to SPEED_UNKNOWN (-1).

I wonder whether you have read the documentation for the phylink
mac_config() method (if not, please read it, it contains some very
important information about what mac_config() should do.)  When
operating in SGMII in-band mode, state->speed and state->duplex are
not actually valid.

You'll probably want to submit a better patch after reading the
documentation.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 14:20     ` Andre Przywara
  2020-01-10 14:26       ` Andrew Lunn
@ 2020-01-10 15:04       ` Russell King - ARM Linux admin
  2020-01-10 15:22         ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 52+ messages in thread
From: Russell King - ARM Linux admin @ 2020-01-10 15:04 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Andrew Lunn, netdev, Radhey Shyam Pandey, Michal Simek,
	linux-kernel, Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, Jan 10, 2020 at 02:20:38PM +0000, Andre Przywara wrote:
> On Fri, 10 Jan 2020 15:04:15 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> Hi Andrew,
> 
> > On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > > With SGMII, the MAC and the PHY can negotiate the link speed between
> > > themselves, without the host needing to mediate between them.
> > > Linux recognises this, and will call phylink's mac_config with the speed
> > > member set to SPEED_UNKNOWN (-1).
> > > Currently the axienet driver will bail out and complain about an
> > > unsupported link speed.
> > > 
> > > Teach axienet's mac_config callback to leave the MAC's speed setting
> > > alone if the requested speed is SPEED_UNKNOWN.  
> > 
> > Hi Andre
> > 
> > Is there an interrupt when SGMII signals a change in link state? If
> > so, you should call phylink_mac_change().
> 
> Good point. The doc describes a "Auto-Negotiation Complete" interrupt
> status bit, which signal that " ... auto-negotiation of the SGMII or
> 1000BASE-X interface has completed."

It depends what they mean by "Auto-negotiation complete" in SGMII.
SGMII can complete the handshake, yet the config_reg word indicate
link down.  If such an update causes an "Auto-negotiation complete"
interrupt, then that's sufficient.

However, looking at axienet_mac_pcs_get_state(), that is just reading
back what the MAC was set to in axienet_mac_config(), which is not
how this is supposed to work.  axienet_mac_pcs_get_state() is
supposed to get the results of the SGMII/1000BASE-X "negotiation".
That also needs to be fixed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
  2020-01-10 14:22       ` Andrew Lunn
@ 2020-01-10 15:08         ` Andre Przywara
  2020-01-10 15:22           ` Andrew Lunn
  0 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 15:08 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, 10 Jan 2020 15:22:50 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

Hi,

> On Fri, Jan 10, 2020 at 02:13:03PM +0000, Andre Przywara wrote:
> > On Fri, 10 Jan 2020 15:08:52 +0100
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > Hi Andrew,
> > 
> > thanks for having a look!
> >   
> > > > To autodetect this configuration, at probe time we write all 1's to such
> > > > an MSB register, and see if any bits stick.    
> > > 
> > > So there is no register you can read containing the IP version?  
> > 
> > There is, and I actually read this before doing this check. But the 64-bit DMA capability is optional even in this revision. It depends on what you give it as the address width. If you say 32, the IP config tool disables the 64-bit capability completely, so it stays compatible with older revisions.
> > Anything beyond 32 will enable the MSB register and will also require you to write to them.  
> 
> So you are saying there is no way to enumerate the synthesised
> configuration of the IP. Great :-(

Apparently not.

> Do Xilinx at least document you can discover the DMA size by writing
> into these upper bits? Does Xilinx own 'vendor crap' driver do this?

So far I couldn't be bothered to put my asbestos trousers on and go into BSP land ;-)
Now quickly browsing the linux-xlnx github repo suggests they make this MSB register access dependent on CONFIG_PHYS_ADDR_T_64BIT. Which would mean:
- A 32-bit kernel on a device configured for >32 bit DMA would not work.
- They always write to the MSB registers on 64-bit and (L)PAE kernels.

The DMA mask is set to the value of the xlnx,addrwidth, in a similar way I did it in the next patch. Minus the safety check for the 64-bit DMA capability.

I got this idea of probing when I saw that those registers are marked as "Reserved" in earlier revisions of the documentation. I couldn't find an exact definition of "Reserved" in that manual, though.
Then I confirmed that behaviour by testing this on an image configured for only a 32 bit wide address bus, where those registers are apparently hardwired to zero.

So if you were hoping for an official blessing, I have to disappoint you ;-)

We could rely completely on the addrwidth property, at the price of it not working when the IP is configured for >32 bits, but the addrwidth property is missing or erroneously set to 32. But I think their:
struct xxx { ....
	phys_addr_t next;	/* Physical address of next buffer descriptor */
#ifndef CONFIG_PHYS_ADDR_T_64BIT
	u32 reserved1;
#endif

construct is broken, and we should not copy this. Also they do writeq to this register, not sure that's the right thing to do.

Cheers,
Andre.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
  2020-01-10 11:54 ` [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path Andre Przywara
@ 2020-01-10 15:14   ` Radhey Shyam Pandey
  2020-01-10 15:43     ` Andre Przywara
  0 siblings, 1 reply; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-10 15:14 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
> 
> When axienet_dma_bd_init() bails out during the initialisation process,
> it might do so with parts of the structure already allocated and
> initialised, while other parts have not been touched yet. Before
> returning in this case, we call axienet_dma_bd_release(), which does not
> take care of this corner case.
> This is most obvious by the first loop happily dereferencing
> lp->rx_bd_v, which we actually check to be non NULL *afterwards*.
> 
> Make sure we only unmap or free already allocated structures, by:
> - directly returning with -ENOMEM if nothing has been allocated at all
> - checking for lp->rx_bd_v to be non-NULL *before* using it
> - only unmapping allocated DMA RX regions
> 
> This avoids NULL pointer dereferences when initialisation fails.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++-------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 97482cf093ce..7e90044cf2d9 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct
> net_device *ndev)
>  	int i;
>  	struct axienet_local *lp = netdev_priv(ndev);
> 
> +	/* If we end up here, tx_bd_v must have been DMA allocated. */
> +	dma_free_coherent(ndev->dev.parent,
> +			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> +			  lp->tx_bd_v,
> +			  lp->tx_bd_p);
> +
> +	if (!lp->rx_bd_v)
> +		return;
> +
>  	for (i = 0; i < lp->rx_bd_num; i++) {
> -		dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
> -				 lp->max_frm_size, DMA_FROM_DEVICE);
> +		/* A NULL skb means this descriptor has not been initialised
> +		 * at all.
> +		 */
> +		if (!lp->rx_bd_v[i].skb)
> +			break;
> +
>  		dev_kfree_skb(lp->rx_bd_v[i].skb);
> -	}
> 
> -	if (lp->rx_bd_v) {
> -		dma_free_coherent(ndev->dev.parent,
> -				  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> -				  lp->rx_bd_v,
> -				  lp->rx_bd_p);
> -	}
> -	if (lp->tx_bd_v) {
> -		dma_free_coherent(ndev->dev.parent,
> -				  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> -				  lp->tx_bd_v,
> -				  lp->tx_bd_p);
> +		/* For each descriptor, we programmed cntrl with the (non-
> zero)
> +		 * descriptor size, after it had been successfully allocated.
> +		 * So a non-zero value in there means we need to unmap it.
> +		 */

> +		if (lp->rx_bd_v[i].cntrl)

I think it should ok to unmap w/o any check?
> +			dma_unmap_single(ndev->dev.parent, lp-
> >rx_bd_v[i].phys,
> +					 lp->max_frm_size,
> DMA_FROM_DEVICE);
>  	}
> +
> +	dma_free_coherent(ndev->dev.parent,
> +			  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> +			  lp->rx_bd_v,
> +			  lp->rx_bd_p);
>  }
> 
>  /**
> @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device
> *ndev)
>  					 sizeof(*lp->tx_bd_v) * lp-
> >tx_bd_num,
>  					 &lp->tx_bd_p, GFP_KERNEL);
>  	if (!lp->tx_bd_v)
> -		goto out;
> +		return -ENOMEM;
> 
>  	lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
>  					 sizeof(*lp->rx_bd_v) * lp-
> >rx_bd_num,
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 15:04       ` Russell King - ARM Linux admin
@ 2020-01-10 15:22         ` Russell King - ARM Linux admin
  2020-01-10 17:04           ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 52+ messages in thread
From: Russell King - ARM Linux admin @ 2020-01-10 15:22 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Andrew Lunn, netdev, Radhey Shyam Pandey, Michal Simek,
	linux-kernel, Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, Jan 10, 2020 at 03:04:09PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Jan 10, 2020 at 02:20:38PM +0000, Andre Przywara wrote:
> > On Fri, 10 Jan 2020 15:04:15 +0100
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > Hi Andrew,
> > 
> > > On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > > > With SGMII, the MAC and the PHY can negotiate the link speed between
> > > > themselves, without the host needing to mediate between them.
> > > > Linux recognises this, and will call phylink's mac_config with the speed
> > > > member set to SPEED_UNKNOWN (-1).
> > > > Currently the axienet driver will bail out and complain about an
> > > > unsupported link speed.
> > > > 
> > > > Teach axienet's mac_config callback to leave the MAC's speed setting
> > > > alone if the requested speed is SPEED_UNKNOWN.  
> > > 
> > > Hi Andre
> > > 
> > > Is there an interrupt when SGMII signals a change in link state? If
> > > so, you should call phylink_mac_change().
> > 
> > Good point. The doc describes a "Auto-Negotiation Complete" interrupt
> > status bit, which signal that " ... auto-negotiation of the SGMII or
> > 1000BASE-X interface has completed."
> 
> It depends what they mean by "Auto-negotiation complete" in SGMII.
> SGMII can complete the handshake, yet the config_reg word indicate
> link down.  If such an update causes an "Auto-negotiation complete"
> interrupt, then that's sufficient.
> 
> However, looking at axienet_mac_pcs_get_state(), that is just reading
> back what the MAC was set to in axienet_mac_config(), which is not
> how this is supposed to work.  axienet_mac_pcs_get_state() is
> supposed to get the results of the SGMII/1000BASE-X "negotiation".
> That also needs to be fixed.

I found "pg138-axi-ethernet.pdf" online, which I guess is this IP.
It says for SGMII:

The results of the SGMII auto-negotiation can be read from the SGMII
Management Auto-Negotiation Link Partner Ability Base register
(Table 2-54). The speed of the subsystem should then be set to match.

and similar for 1000BASE-X (referencing the same register.)

However, what they give in table 2-54 is the 1000BASE-X version of
the config_reg word, not the SGMII version (which is different.)

Hmm, I guess there's probably some scope for phylink to start
handling an IEEE 802.3 compliant PCS accessed over MDIO rather
than having each network driver implement this, but for now your
axienet_mac_pcs_get_state() implementation needs to be reading
from the register described in table 2-54 and interpreting the
results according to whether state->interface is 802.3z or not.

Also note, don't set state->interface in axienet_mac_pcs_get_state(),
you will be passed the currently selected interface that was last
configured via axienet_mac_config().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
  2020-01-10 15:08         ` Andre Przywara
@ 2020-01-10 15:22           ` Andrew Lunn
  0 siblings, 0 replies; 52+ messages in thread
From: Andrew Lunn @ 2020-01-10 15:22 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, David S . Miller, linux-arm-kernel

> So far I couldn't be bothered to put my asbestos trousers on and go
> into BSP land ;-)

Are you in Cambridge? 7 degrees, so you can pop outside to cool off a
bit :-)

> So if you were hoping for an official blessing, I have to disappoint you ;-)

Well, everything you have done is at least sensible. The patches have
also drawn the interest of Radhey. Let see if he says this is safe for
IP version 0.0 through to 7.1.

   Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 04/14] net: axienet: Improve DMA error handling
  2020-01-10 11:54 ` [PATCH 04/14] net: axienet: Improve DMA error handling Andre Przywara
@ 2020-01-10 15:26   ` Radhey Shyam Pandey
  0 siblings, 0 replies; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-10 15:26 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 04/14] net: axienet: Improve DMA error handling
> 
> Since 0 is a valid DMA address, we cannot use the physical address to
> check whether a TX descriptor is valid and is holding a DMA mapping.
> 
> Use the "cntrl" member of the descriptor to make this decision, as it
> contains at least the length of the buffer, so 0 points to an
> uninitialised buffer.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>

> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 7e90044cf2d9..ec5d01adc1d5 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -570,7 +570,7 @@ static void axienet_start_xmit_done(struct
> net_device *ndev)
>  				DMA_TO_DEVICE);
>  		if (cur_p->skb)
>  			dev_consume_skb_irq(cur_p->skb);
> -		/*cur_p->phys = 0;*/
> +		cur_p->cntrl = 0;
>  		cur_p->app0 = 0;
>  		cur_p->app1 = 0;
>  		cur_p->app2 = 0;
> @@ -1557,7 +1557,7 @@ static void axienet_dma_err_handler(unsigned
> long data)
> 
>  	for (i = 0; i < lp->tx_bd_num; i++) {
>  		cur_p = &lp->tx_bd_v[i];
> -		if (cur_p->phys)
> +		if (cur_p->cntrl)
>  			dma_unmap_single(ndev->dev.parent, cur_p->phys,
>  					 (cur_p->cntrl &
> 
> XAXIDMA_BD_CTRL_LENGTH_MASK),
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
  2020-01-10 15:14   ` Radhey Shyam Pandey
@ 2020-01-10 15:43     ` Andre Przywara
  2020-01-10 17:05       ` Radhey Shyam Pandey
  0 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 15:43 UTC (permalink / raw)
  To: Radhey Shyam Pandey
  Cc: netdev, linux-kernel, Robert Hancock, Michal Simek,
	David S . Miller, linux-arm-kernel

On Fri, 10 Jan 2020 15:14:46 +0000
Radhey Shyam Pandey <radheys@xilinx.com> wrote:

Hi Radhey,

thanks for having a look!

> > -----Original Message-----
> > From: Andre Przywara <andre.przywara@arm.com>
> > Sent: Friday, January 10, 2020 5:24 PM
> > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> > <radheys@xilinx.com>
> > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
> > 
> > When axienet_dma_bd_init() bails out during the initialisation process,
> > it might do so with parts of the structure already allocated and
> > initialised, while other parts have not been touched yet. Before
> > returning in this case, we call axienet_dma_bd_release(), which does not
> > take care of this corner case.
> > This is most obvious by the first loop happily dereferencing
> > lp->rx_bd_v, which we actually check to be non NULL *afterwards*.
> > 
> > Make sure we only unmap or free already allocated structures, by:
> > - directly returning with -ENOMEM if nothing has been allocated at all
> > - checking for lp->rx_bd_v to be non-NULL *before* using it
> > - only unmapping allocated DMA RX regions
> > 
> > This avoids NULL pointer dereferences when initialisation fails.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++-------
> >  1 file changed, 28 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > index 97482cf093ce..7e90044cf2d9 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct
> > net_device *ndev)
> >  	int i;
> >  	struct axienet_local *lp = netdev_priv(ndev);
> > 
> > +	/* If we end up here, tx_bd_v must have been DMA allocated. */
> > +	dma_free_coherent(ndev->dev.parent,
> > +			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> > +			  lp->tx_bd_v,
> > +			  lp->tx_bd_p);
> > +
> > +	if (!lp->rx_bd_v)
> > +		return;
> > +
> >  	for (i = 0; i < lp->rx_bd_num; i++) {
> > -		dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
> > -				 lp->max_frm_size, DMA_FROM_DEVICE);
> > +		/* A NULL skb means this descriptor has not been initialised
> > +		 * at all.
> > +		 */
> > +		if (!lp->rx_bd_v[i].skb)
> > +			break;
> > +
> >  		dev_kfree_skb(lp->rx_bd_v[i].skb);
> > -	}
> > 
> > -	if (lp->rx_bd_v) {
> > -		dma_free_coherent(ndev->dev.parent,
> > -				  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> > -				  lp->rx_bd_v,
> > -				  lp->rx_bd_p);
> > -	}
> > -	if (lp->tx_bd_v) {
> > -		dma_free_coherent(ndev->dev.parent,
> > -				  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> > -				  lp->tx_bd_v,
> > -				  lp->tx_bd_p);
> > +		/* For each descriptor, we programmed cntrl with the (non-
> > zero)
> > +		 * descriptor size, after it had been successfully allocated.
> > +		 * So a non-zero value in there means we need to unmap it.
> > +		 */  
> 
> > +		if (lp->rx_bd_v[i].cntrl)  
> 
> I think it should ok to unmap w/o any check?

Do you mean because .phys would be 0 if not initialised? AFAIK 0 can be a valid DMA address, so there is no special check for that, and unmapping DMA address 0 will probably go wrong at some point. So it's unlike kfree(NULL).

Cheers,
Andre.


> > +			dma_unmap_single(ndev->dev.parent, lp-  
> > >rx_bd_v[i].phys,  
> > +					 lp->max_frm_size,
> > DMA_FROM_DEVICE);
> >  	}
> > +
> > +	dma_free_coherent(ndev->dev.parent,
> > +			  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> > +			  lp->rx_bd_v,
> > +			  lp->rx_bd_p);
> >  }
> > 
> >  /**
> > @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device
> > *ndev)
> >  					 sizeof(*lp->tx_bd_v) * lp-  
> > >tx_bd_num,  
> >  					 &lp->tx_bd_p, GFP_KERNEL);
> >  	if (!lp->tx_bd_v)
> > -		goto out;
> > +		return -ENOMEM;
> > 
> >  	lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> >  					 sizeof(*lp->rx_bd_v) * lp-  
> > >rx_bd_num,  
> > --
> > 2.17.1  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 15:22         ` Russell King - ARM Linux admin
@ 2020-01-10 17:04           ` Russell King - ARM Linux admin
  2020-01-18 11:22             ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 52+ messages in thread
From: Russell King - ARM Linux admin @ 2020-01-10 17:04 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Andrew Lunn, netdev, Radhey Shyam Pandey, Michal Simek,
	linux-kernel, Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, Jan 10, 2020 at 03:22:15PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Jan 10, 2020 at 03:04:09PM +0000, Russell King - ARM Linux admin wrote:
> > On Fri, Jan 10, 2020 at 02:20:38PM +0000, Andre Przywara wrote:
> > > On Fri, 10 Jan 2020 15:04:15 +0100
> > > Andrew Lunn <andrew@lunn.ch> wrote:
> > > 
> > > Hi Andrew,
> > > 
> > > > On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > > > > With SGMII, the MAC and the PHY can negotiate the link speed between
> > > > > themselves, without the host needing to mediate between them.
> > > > > Linux recognises this, and will call phylink's mac_config with the speed
> > > > > member set to SPEED_UNKNOWN (-1).
> > > > > Currently the axienet driver will bail out and complain about an
> > > > > unsupported link speed.
> > > > > 
> > > > > Teach axienet's mac_config callback to leave the MAC's speed setting
> > > > > alone if the requested speed is SPEED_UNKNOWN.  
> > > > 
> > > > Hi Andre
> > > > 
> > > > Is there an interrupt when SGMII signals a change in link state? If
> > > > so, you should call phylink_mac_change().
> > > 
> > > Good point. The doc describes a "Auto-Negotiation Complete" interrupt
> > > status bit, which signal that " ... auto-negotiation of the SGMII or
> > > 1000BASE-X interface has completed."
> > 
> > It depends what they mean by "Auto-negotiation complete" in SGMII.
> > SGMII can complete the handshake, yet the config_reg word indicate
> > link down.  If such an update causes an "Auto-negotiation complete"
> > interrupt, then that's sufficient.
> > 
> > However, looking at axienet_mac_pcs_get_state(), that is just reading
> > back what the MAC was set to in axienet_mac_config(), which is not
> > how this is supposed to work.  axienet_mac_pcs_get_state() is
> > supposed to get the results of the SGMII/1000BASE-X "negotiation".
> > That also needs to be fixed.
> 
> I found "pg138-axi-ethernet.pdf" online, which I guess is this IP.
> It says for SGMII:
> 
> The results of the SGMII auto-negotiation can be read from the SGMII
> Management Auto-Negotiation Link Partner Ability Base register
> (Table 2-54). The speed of the subsystem should then be set to match.
> 
> and similar for 1000BASE-X (referencing the same register.)
> 
> However, what they give in table 2-54 is the 1000BASE-X version of
> the config_reg word, not the SGMII version (which is different.)
> 
> Hmm, I guess there's probably some scope for phylink to start
> handling an IEEE 802.3 compliant PCS accessed over MDIO rather
> than having each network driver implement this, but for now your
> axienet_mac_pcs_get_state() implementation needs to be reading
> from the register described in table 2-54 and interpreting the
> results according to whether state->interface is 802.3z or not.
> 
> Also note, don't set state->interface in axienet_mac_pcs_get_state(),
> you will be passed the currently selected interface that was last
> configured via axienet_mac_config().

Maybe something like the below will help?

Basically, use phylink_mii_pcs_get_state() instead of
axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
access the internal PHY (as per C_PHYADDR parameter.)

You may have some fuzz (with gnu patch) while trying to apply this,
as you won't have the context for the first and last hunks in this
patch.

This will probably not be the final version of the patch anyway;
there's some possibility to pull some of the functionality out of
phylib into a more general library which would avoid some of the
functional duplication.

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 75a74a16dc3d..44198fdb3c01 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2073,4 +2073,105 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
 }
 EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
 
+static void phylink_decode_advertisement(struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
+
+	linkmode_and(u, state->lp_advertising, state->advertising);
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
+		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
+	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, u)) {
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->lp_advertising))
+			state->pause |= MLO_PAUSE_TX;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->advertising))
+			state->pause |= MLO_PAUSE_RX;
+	}
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
+		state->speed = SPEED_2500;
+		state->duplex = DUPLEX_FULL;
+	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, u)) {
+		state->pause = SPEED_1000;
+		state->duplex = DUPLEX_FULL;
+	} else {
+		state->link = false;
+	}
+}
+
+void phylink_mii_pcs_get_state(struct phylink_config *config,
+			       struct phylink_link_state *state)
+{
+	struct mii_bus *bus = config->pcs_mii;
+	int addr = config->pcs_mii_addr;
+	int bmsr, lpa;
+
+	bmsr = mdiobus_read(bus, addr, MII_BMSR);
+	lpa = mdiobus_read(bus, addr, MII_LPA);
+	if (bmsr < 0 || lpa < 0) {
+		state->link = false;
+		return;
+	}
+
+	state->link = !!(bmsr & BMSR_LSTATUS);
+	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+		if (lpa & LPA_1000XFULL)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+					 state->lp_advertising);
+		goto lpa_8023z;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		if (lpa & LPA_1000XFULL)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
+					 state->lp_advertising);
+	lpa_8023z:
+		if (lpa & LPA_1000XPAUSE)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+					 state->lp_advertising);
+		if (lpa & LPA_1000XPAUSE_ASYM)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+					 state->lp_advertising);
+		if (lpa & LPA_LPACK)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					 state->lp_advertising);
+		phylink_decode_advertisement(state);
+		break;
+
+	case PHY_INTERFACE_MODE_SGMII:
+		switch (lpa & 0x8c00) {
+		case 0x8000:
+			state->speed = SPEED_10;
+			break;
+		case 0x8400:
+			state->speed = SPEED_100;
+			break;
+		case 0x8800:
+			state->speed = SPEED_1000;
+			break;
+		default:
+			state->link = false;
+			break;
+		}
+		switch (lpa & 0x9000) {
+		case 0x9000:
+			state->duplex = DUPLEX_FULL;
+			break;
+		case 0x8000:
+			state->duplex = DUPLEX_HALF;
+			break;
+		}
+		break;
+
+	default:
+		state->link = false;
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 4ea76e083847..cf0fa39b4b21 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -65,6 +65,9 @@ enum phylink_op_type {
 struct phylink_config {
 	struct device *dev;
 	enum phylink_op_type type;
+
+	struct mii_bus *pcs_mii;
+	int pcs_mii_addr;
 };
 
 /**
@@ -292,4 +295,7 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
 						const phy_interface_t *pref,
 						size_t nprefs);
 
+void phylink_mii_pcs_get_state(struct phylink_config *config,
+			       struct phylink_link_state *state);
+
 #endif

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
  2020-01-10 15:43     ` Andre Przywara
@ 2020-01-10 17:05       ` Radhey Shyam Pandey
  2020-01-16 18:03         ` Andre Przywara
  0 siblings, 1 reply; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-10 17:05 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, linux-kernel, Robert Hancock, Michal Simek,
	David S . Miller, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 9:13 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: David S . Miller <davem@davemloft.net>; Michal Simek
> <michals@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>;
> netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
> 
> On Fri, 10 Jan 2020 15:14:46 +0000
> Radhey Shyam Pandey <radheys@xilinx.com> wrote:
> 
> Hi Radhey,
> 
> thanks for having a look!
> 
> > > -----Original Message-----
> > > From: Andre Przywara <andre.przywara@arm.com>
> > > Sent: Friday, January 10, 2020 5:24 PM
> > > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> > > <radheys@xilinx.com>
> > > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> > > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
> > >
> > > When axienet_dma_bd_init() bails out during the initialisation process,
> > > it might do so with parts of the structure already allocated and
> > > initialised, while other parts have not been touched yet. Before
> > > returning in this case, we call axienet_dma_bd_release(), which does not
> > > take care of this corner case.
> > > This is most obvious by the first loop happily dereferencing
> > > lp->rx_bd_v, which we actually check to be non NULL *afterwards*.
> > >
> > > Make sure we only unmap or free already allocated structures, by:
> > > - directly returning with -ENOMEM if nothing has been allocated at all
> > > - checking for lp->rx_bd_v to be non-NULL *before* using it
> > > - only unmapping allocated DMA RX regions
> > >
> > > This avoids NULL pointer dereferences when initialisation fails.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++-------
> > >  1 file changed, 28 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > index 97482cf093ce..7e90044cf2d9 100644
> > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct
> > > net_device *ndev)
> > >  	int i;
> > >  	struct axienet_local *lp = netdev_priv(ndev);
> > >
> > > +	/* If we end up here, tx_bd_v must have been DMA allocated. */
> > > +	dma_free_coherent(ndev->dev.parent,
> > > +			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> > > +			  lp->tx_bd_v,
> > > +			  lp->tx_bd_p);
> > > +
> > > +	if (!lp->rx_bd_v)
> > > +		return;
> > > +
> > >  	for (i = 0; i < lp->rx_bd_num; i++) {
> > > -		dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
> > > -				 lp->max_frm_size, DMA_FROM_DEVICE);
> > > +		/* A NULL skb means this descriptor has not been initialised
> > > +		 * at all.
> > > +		 */
> > > +		if (!lp->rx_bd_v[i].skb)
> > > +			break;
> > > +
> > >  		dev_kfree_skb(lp->rx_bd_v[i].skb);
> > > -	}
> > >
> > > -	if (lp->rx_bd_v) {
> > > -		dma_free_coherent(ndev->dev.parent,
> > > -				  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> > > -				  lp->rx_bd_v,
> > > -				  lp->rx_bd_p);
> > > -	}
> > > -	if (lp->tx_bd_v) {
> > > -		dma_free_coherent(ndev->dev.parent,
> > > -				  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> > > -				  lp->tx_bd_v,
> > > -				  lp->tx_bd_p);
> > > +		/* For each descriptor, we programmed cntrl with the (non-
> > > zero)
> > > +		 * descriptor size, after it had been successfully allocated.
> > > +		 * So a non-zero value in there means we need to unmap it.
> > > +		 */
> >
> > > +		if (lp->rx_bd_v[i].cntrl)
> >
> > I think it should ok to unmap w/o any check?
> 
> Do you mean because .phys would be 0 if not initialised? AFAIK 0 can be a
> valid DMA address, so there is no special check for that, and unmapping
> DMA address 0 will probably go wrong at some point. So it's unlike
> kfree(NULL).

I mean if skb allocation is successful in _dma_bd_init then in release path
we can assume .phys is always a valid address and skip rx_bd_v[i].cntrl
check. 
> 
> Cheers,
> Andre.
> 
> 
> > > +			dma_unmap_single(ndev->dev.parent, lp-
> > > >rx_bd_v[i].phys,
> > > +					 lp->max_frm_size,
> > > DMA_FROM_DEVICE);
> > >  	}
> > > +
> > > +	dma_free_coherent(ndev->dev.parent,
> > > +			  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> > > +			  lp->rx_bd_v,
> > > +			  lp->rx_bd_p);
> > >  }
> > >
> > >  /**
> > > @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device
> > > *ndev)
> > >  					 sizeof(*lp->tx_bd_v) * lp-
> > > >tx_bd_num,
> > >  					 &lp->tx_bd_p, GFP_KERNEL);
> > >  	if (!lp->tx_bd_v)
> > > -		goto out;
> > > +		return -ENOMEM;
> > >
> > >  	lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> > >  					 sizeof(*lp->rx_bd_v) * lp-
> > > >rx_bd_num,
> > > --
> > > 2.17.1
> >


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 14:58   ` Russell King - ARM Linux admin
@ 2020-01-10 17:32     ` Andre Przywara
  2020-01-10 18:05       ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-10 17:32 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, 10 Jan 2020 14:58:49 +0000
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > With SGMII, the MAC and the PHY can negotiate the link speed between
> > themselves, without the host needing to mediate between them.
> > Linux recognises this, and will call phylink's mac_config with the speed
> > member set to SPEED_UNKNOWN (-1).  
> 
> I wonder whether you have read the documentation for the phylink
> mac_config() method (if not, please read it, it contains some very
> important information about what mac_config() should do.)  When
> operating in SGMII in-band mode, state->speed and state->duplex are
> not actually valid.
> 
> You'll probably want to submit a better patch after reading the
> documentation.

Sure, I am admittedly quite clueless about phylink in particular, and found the available information quite daunting.
So I tried my best in looking at what other drivers do. From what I got there is that you speed=-1 should be ignored, but the other fields still handled.
Also I was somewhat puzzled, as I was expecting "mode" being MLO_AN_INBAND. But in fact it's called twice with MLO_AN_PHY, and mac_pcs_get_state() never gets called:

[  166.516583] xilinx_axienet 7fe00000.ethernet eth0: PHY [axienet-7fe00000:01] driver [Generic PHY]
[  166.547309] xilinx_axienet 7fe00000.ethernet eth0: configuring for phy/sgmii link mode
[  166.572343] axienet_mac_config(mode=0, speed=-1, duplex=255, pause=16, link=0, an_en=1)
udhcpc: sending discover
[  168.652152] axienet_mac_config(mode=0, speed=-1, duplex=255, pause=0, link=1, an_en=0)
[  168.683538] xilinx_axienet 7fe00000.ethernet eth0: Link is Up - Unknown/Unknown - flow control off
[  168.712560] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
udhcpc: sending discover
udhcpc: sending select for 10.1.x.y
udhcpc: lease of 10.1.x.y obtained, lease time 691200

I was just wondering if the DT description is giving Linux a wrong impression, but I have phy-mode set to sgmii, also just tried phy-connection-type on top of that. The DT snippet is the same as the example in patch 14. The PHY is a Marvell 88E1111, connected via SGMII.
 
I would be grateful for any advice!

Cheers,
Andre.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 02/14] net: axienet: Propagate failure of DMA descriptor setup
  2020-01-10 14:54   ` Radhey Shyam Pandey
@ 2020-01-10 17:53     ` Radhey Shyam Pandey
  0 siblings, 0 replies; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-10 17:53 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Radhey Shyam Pandey
> Sent: Friday, January 10, 2020 8:25 PM
> To: Andre Przywara <andre.przywara@arm.com>; David S . Miller
> <davem@davemloft.net>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 02/14] net: axienet: Propagate failure of DMA descriptor
> setup
> 
> > -----Original Message-----
> > From: Andre Przywara <andre.przywara@arm.com>
> > Sent: Friday, January 10, 2020 5:24 PM
> > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> > <radheys@xilinx.com>
> > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH 02/14] net: axienet: Propagate failure of DMA
> > descriptor setup
> >
> > When we fail allocating the DMA buffers in axienet_dma_bd_init(), we
> > report this error, but carry on with initialisation nevertheless.
> >
> > This leads to a kernel panic when the driver later wants to send a
> > packet, as it uses uninitialised data structures.
> >
> > Make the axienet_device_reset() routine return an error value, as it
> > contains the DMA buffer initialisation. Make sure we propagate the
> > error up the chain and eventually fail the driver initialisation, to
> > avoid relying on non-initialised buffers.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> 
> > ---
> >  .../net/ethernet/xilinx/xilinx_axienet_main.c | 25
> > +++++++++++++------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > index 20746b801959..97482cf093ce 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > @@ -437,9 +437,10 @@ static void axienet_setoptions(struct net_device
> > *ndev, u32 options)
> >  	lp->options |= options;
> >  }
> >
> > -static void __axienet_device_reset(struct axienet_local *lp)
> > +static int __axienet_device_reset(struct axienet_local *lp)
> >  {
> >  	u32 timeout;
> > +
> >  	/* Reset Axi DMA. This would reset Axi Ethernet core as well. The
> > reset
> >  	 * process of Axi DMA takes a while to complete as all pending
> >  	 * commands/transfers will be flushed or completed during this @@
> > -455,9 +456,11 @@ static void __axienet_device_reset(struct
> > axienet_local *lp)
> >  		if (--timeout == 0) {
> >  			netdev_err(lp->ndev, "%s: DMA reset timeout!\n",
> >  				   __func__);
> > -			break;
> > +			return -ETIMEDOUT;
> >  		}
> >  	}
> > +
> > +	return 0;
> >  }
> >
> >  /**
> > @@ -471,12 +474,15 @@ static void __axienet_device_reset(struct
> > axienet_local *lp)
> >   * Ethernet core. No separate hardware reset is done for the Axi Ethernet
> >   * core.
> >   */

Minor nit- we need to add descripton for return value.
drivers/net/ethernet/xilinx/xilinx_axienet_main.c:491: warning: 
No description found for return value of 'axienet_device_reset'

> > -static void axienet_device_reset(struct net_device *ndev)
> > +static int axienet_device_reset(struct net_device *ndev)
> >  {
> >  	u32 axienet_status;
> >  	struct axienet_local *lp = netdev_priv(ndev);
> > +	int ret;
> >
> > -	__axienet_device_reset(lp);
> > +	ret = __axienet_device_reset(lp);
> > +	if (ret)
> > +		return ret;
> >
> >  	lp->max_frm_size = XAE_MAX_VLAN_FRAME_SIZE;
> >  	lp->options |= XAE_OPTION_VLAN;
> > @@ -491,9 +497,11 @@ static void axienet_device_reset(struct
> > net_device
> > *ndev)
> >  			lp->options |= XAE_OPTION_JUMBO;
> >  	}
> >
> > -	if (axienet_dma_bd_init(ndev)) {
> > +	ret = axienet_dma_bd_init(ndev);
> > +	if (ret) {
> >  		netdev_err(ndev, "%s: descriptor allocation failed\n",
> >  			   __func__);
> > +		return ret;
> >  	}
> >
> >  	axienet_status = axienet_ior(lp, XAE_RCW1_OFFSET); @@ -518,6
> +526,8
> > @@ static void axienet_device_reset(struct net_device
> > *ndev)
> >  	axienet_setoptions(ndev, lp->options);
> >
> >  	netif_trans_update(ndev);
> > +
> > +	return 0;
> >  }
> >
> >  /**
> > @@ -921,8 +931,9 @@ static int axienet_open(struct net_device *ndev)
> >  	 */
> >  	mutex_lock(&lp->mii_bus->mdio_lock);
> >  	axienet_mdio_disable(lp);
> > -	axienet_device_reset(ndev);
> > -	ret = axienet_mdio_enable(lp);
> > +	ret = axienet_device_reset(ndev);
> > +	if (ret == 0)
> > +		ret = axienet_mdio_enable(lp);
> >  	mutex_unlock(&lp->mii_bus->mdio_lock);
> >  	if (ret < 0)
> >  		return ret;
> > --
> > 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 05/14] net: axienet: Factor out TX descriptor chain cleanup
  2020-01-10 11:54 ` [PATCH 05/14] net: axienet: Factor out TX descriptor chain cleanup Andre Przywara
@ 2020-01-10 18:04   ` Radhey Shyam Pandey
  0 siblings, 0 replies; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-10 18:04 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 05/14] net: axienet: Factor out TX descriptor chain cleanup
> 
> Factor out the code that cleans up a number of connected TX descriptors,
> as we will need it to properly roll back a failed _xmit() call.
> There are subtle differences between cleaning up a successfully sent
> chain (unknown number of involved descriptors, total data size needed)
> and a chain that was about to set up (number of descriptors known), so
> cater for those variations with some extra parameters.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 75 ++++++++++++-------
>  1 file changed, 50 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index ec5d01adc1d5..82abe2b0f16a 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -543,33 +543,37 @@ static int axienet_device_reset(struct net_device
> *ndev)
>  	return 0;
>  }
> 
> -/**
> - * axienet_start_xmit_done - Invoked once a transmit is completed by the
> - * Axi DMA Tx channel.
> - * @ndev:	Pointer to the net_device structure
> - *
> - * This function is invoked from the Axi DMA Tx isr to notify the completion
> - * of transmit operation. It clears fields in the corresponding Tx BDs and
> - * unmaps the corresponding buffer so that CPU can regain ownership of
> the
> - * buffer. It finally invokes "netif_wake_queue" to restart transmission if
> - * required.
> +/* Clean up a series of linked TX descriptors. Would either be called
> + * after a successful transmit operation, or after there was an error
> + * when setting up the chain.
> + * Returns the number of descriptors handled.
>   */
> -static void axienet_start_xmit_done(struct net_device *ndev)

To be consistent we can add the doxygen function description.
The rest looks good. Feel free to add:
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>

> +static int axienet_free_tx_chain(struct net_device *ndev, u32 first_bd,
> +				 int nr_bds, u32 *sizep)
>  {
> -	u32 size = 0;
> -	u32 packets = 0;
>  	struct axienet_local *lp = netdev_priv(ndev);
> +	int max_bds = (nr_bds != -1) ? nr_bds : lp->tx_bd_num;
>  	struct axidma_bd *cur_p;
> -	unsigned int status = 0;
> +	unsigned int status;
> +	int i;
> +
> +	for (i = 0; i < max_bds; i++) {
> +		cur_p = &lp->tx_bd_v[(first_bd + i) % lp->tx_bd_num];
> +		status = cur_p->status;
> +
> +		/* If no number is given, clean up *all* descriptors that have
> +		 * been completed by the MAC.
> +		 */
> +		if (nr_bds == -1 && !(status &
> XAXIDMA_BD_STS_COMPLETE_MASK))
> +			break;
> 
> -	cur_p = &lp->tx_bd_v[lp->tx_bd_ci];
> -	status = cur_p->status;
> -	while (status & XAXIDMA_BD_STS_COMPLETE_MASK) {
>  		dma_unmap_single(ndev->dev.parent, cur_p->phys,
>  				(cur_p->cntrl &
> XAXIDMA_BD_CTRL_LENGTH_MASK),
>  				DMA_TO_DEVICE);
> -		if (cur_p->skb)
> +
> +		if (cur_p->skb && (status &
> XAXIDMA_BD_STS_COMPLETE_MASK))
>  			dev_consume_skb_irq(cur_p->skb);
> +
>  		cur_p->cntrl = 0;
>  		cur_p->app0 = 0;
>  		cur_p->app1 = 0;
> @@ -578,15 +582,36 @@ static void axienet_start_xmit_done(struct
> net_device *ndev)
>  		cur_p->status = 0;
>  		cur_p->skb = NULL;
> 
> -		size += status & XAXIDMA_BD_STS_ACTUAL_LEN_MASK;
> -		packets++;
> -
> -		if (++lp->tx_bd_ci >= lp->tx_bd_num)
> -			lp->tx_bd_ci = 0;
> -		cur_p = &lp->tx_bd_v[lp->tx_bd_ci];
> -		status = cur_p->status;
> +		if (sizep)
> +			*sizep += status &
> XAXIDMA_BD_STS_ACTUAL_LEN_MASK;
>  	}
> 
> +	return i;
> +}
> +
> +/**
> + * axienet_start_xmit_done - Invoked once a transmit is completed by the
> + * Axi DMA Tx channel.
> + * @ndev:	Pointer to the net_device structure
> + *
> + * This function is invoked from the Axi DMA Tx isr to notify the completion
> + * of transmit operation. It clears fields in the corresponding Tx BDs and
> + * unmaps the corresponding buffer so that CPU can regain ownership of
> the
> + * buffer. It finally invokes "netif_wake_queue" to restart transmission if
> + * required.
> + */
> +static void axienet_start_xmit_done(struct net_device *ndev)
> +{
> +	u32 size = 0;
> +	u32 packets = 0;
> +	struct axienet_local *lp = netdev_priv(ndev);
> +
> +	packets = axienet_free_tx_chain(ndev, lp->tx_bd_ci, -1, &size);
> +
> +	lp->tx_bd_ci += packets;
> +	if (lp->tx_bd_ci >= lp->tx_bd_num)
> +		lp->tx_bd_ci -= lp->tx_bd_num;
> +
>  	ndev->stats.tx_packets += packets;
>  	ndev->stats.tx_bytes += size;
> 
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 17:32     ` Andre Przywara
@ 2020-01-10 18:05       ` Russell King - ARM Linux admin
  2020-01-10 19:33         ` Andrew Lunn
  0 siblings, 1 reply; 52+ messages in thread
From: Russell King - ARM Linux admin @ 2020-01-10 18:05 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, Jan 10, 2020 at 05:32:49PM +0000, Andre Przywara wrote:
> On Fri, 10 Jan 2020 14:58:49 +0000
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > > With SGMII, the MAC and the PHY can negotiate the link speed between
> > > themselves, without the host needing to mediate between them.
> > > Linux recognises this, and will call phylink's mac_config with the speed
> > > member set to SPEED_UNKNOWN (-1).  
> > 
> > I wonder whether you have read the documentation for the phylink
> > mac_config() method (if not, please read it, it contains some very
> > important information about what mac_config() should do.)  When
> > operating in SGMII in-band mode, state->speed and state->duplex are
> > not actually valid.
> > 
> > You'll probably want to submit a better patch after reading the
> > documentation.
> 
> Sure, I am admittedly quite clueless about phylink in particular, and found the available information quite daunting.
> So I tried my best in looking at what other drivers do. From what I got there is that you speed=-1 should be ignored, but the other fields still handled.
> Also I was somewhat puzzled, as I was expecting "mode" being MLO_AN_INBAND. But in fact it's called twice with MLO_AN_PHY, and mac_pcs_get_state() never gets called:

Okay.  When phylink is in PHY mode, it operates just the same as the
more conventional phylib setup: phylib reports the negotiation results
to the network driver which sets the MAC up appropriately.

The only difference between the phylib way of doing things and phylink
is that phylink is in the path, so mac_config() gets called to setup
the MAC with the results of the PHY negotiation.  This will be the case
irrespective of which PHY interface mode is being used.

So, in PHY mode, we don't care whether there is in-band signalling or
not - and the reason that's vague is because it _is_ already vague
with existing phylib setups using SGMII.

So, basically, the MLO_AN_PHY mode is the complete equivalent of
phylib without phylink.


MLO_AN_FIXED is just like MLO_AN_PHY, except phylink is operating in
fixed-link mode - similar to the old fixed-link emulated PHY setup that
phylib offered, but without needing a MII bus and squeezing the
information through phylib's interfaces.  From the point of view of a
MAC driver, however, it's just the same as MLO_AN_PHY.


If you configure phylink for inband mode by placing

	managed = "in-band-status";

in DT, then phylink will operate in MLO_AN_INBAND mode.  It will also
operate in that mode if the MAC is connected directly to a SFP cage
and a SFP is inserted that requires inband mode.

Exactly how inband mode operates depends in the nature of the inband
signalling.  There's two different schemes:

SGMII: the PHY communicates the speed and duplex settings to the MAC
PCS through the in-band control word.  Pause mode is not available
via the in-band control word.  SGMII can operate at 10M, 100M or 1G,
half or full duplex.  The PHY may or may not be accessible.

Here, phylink will read the speed and duplex from the MAC PCS rather
than the PHY, and if the PHY is accessible, phylink will merge the
negotiated pause mode information and pass this over to the MAC.

(Note: there are some vendor extensions to pass pause mode through
SGMII as well, but I haven't seen a MAC that supports them yet.)

1000BASE-X (aka 802.3z): the link partner advertises its capabilities
via the in-band control word, which are:
	- full duplex
	- half duplex
	- pause
	- asym pause

and each end of the link has to resolve the capabilities to agree the
operating mode of the link.  As only a single speed is supported in
this mode, there is no need to advertise any speed capabilities (if
the link operates at dis-similar speeds - for example, 2500BASE-X at
one end and 1000BASE-X on the other, there's no way to get the control
word through.)

Here, phylink will only read from the MAC PCS to discover the results
of the negotiation; there will be no call to mac_config().


Phylink currently expects the result of the in-band negotiation at
the MAC PCS to be propagated to the MAC by hardware (as this is what
happens with mvneta and mvpp2, the first two MACs that phylink
supports.)  If there is hardware that requires something else, then
that will need to be revisited, and will result in not only code but
also documentation updates as well.

I hope this helps you to understand phylink.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 18:05       ` Russell King - ARM Linux admin
@ 2020-01-10 19:33         ` Andrew Lunn
  0 siblings, 0 replies; 52+ messages in thread
From: Andrew Lunn @ 2020-01-10 19:33 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andre Przywara, Radhey Shyam Pandey, Michal Simek, linux-kernel,
	Robert Hancock, netdev, David S . Miller, linux-arm-kernel

> Phylink currently expects the result of the in-band negotiation at
> the MAC PCS to be propagated to the MAC by hardware (as this is what
> happens with mvneta and mvpp2, the first two MACs that phylink
> supports.)  If there is hardware that requires something else, then
> that will need to be revisited, and will result in not only code but
> also documentation updates as well.

Hi Russell

This is an issue i'm having at the moment with Marvell switches. They
do not propagate the results to the MAC. So when i get an interrupt
from the SERDES that the link is up, i'm programming the MAC as
needed.

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 06/14] net: axienet: Check for DMA mapping errors
  2020-01-10 11:54 ` [PATCH 06/14] net: axienet: Check for DMA mapping errors Andre Przywara
@ 2020-01-13  5:54   ` Radhey Shyam Pandey
  0 siblings, 0 replies; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-13  5:54 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 06/14] net: axienet: Check for DMA mapping errors
> 
> Especially with the default 32-bit DMA mask, DMA buffers are a limited
> resource, so their allocation can fail.
> So as the DMA API documentation requires, add error checking code after
> dma_map_single() calls to catch the case where we run out of "low" memory.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 82abe2b0f16a..8d2b67cbecf9 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -248,6 +248,11 @@ static int axienet_dma_bd_init(struct net_device
> *ndev)
>  						     skb->data,
>  						     lp->max_frm_size,
>  						     DMA_FROM_DEVICE);
> +		if (dma_mapping_error(ndev->dev.parent, lp->rx_bd_v[i].phys))

Prefer using unlikely compiler hint for dma_mapping_error. 
Also, we need to add error print to report this condition to the user,
in case it isn't there in dma_mapping_error implementation.

> {
> +			dev_kfree_skb(skb);

free of skb is already handled in _release. We can reuse that?
> +			goto out;
> +		}
> +
>  		lp->rx_bd_v[i].cntrl = lp->max_frm_size;
>  	}
> 
> @@ -668,6 +673,7 @@ axienet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
>  	dma_addr_t tail_p;
>  	struct axienet_local *lp = netdev_priv(ndev);
>  	struct axidma_bd *cur_p;
> +	u32 orig_tail_ptr = lp->tx_bd_tail;
> 
>  	num_frag = skb_shinfo(skb)->nr_frags;
>  	cur_p = &lp->tx_bd_v[lp->tx_bd_tail];
> @@ -703,9 +709,11 @@ axienet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
>  		cur_p->app0 |= 2; /* Tx Full Checksum Offload Enabled */
>  	}
> 
> -	cur_p->cntrl = skb_headlen(skb) | XAXIDMA_BD_CTRL_TXSOF_MASK;
>  	cur_p->phys = dma_map_single(ndev->dev.parent, skb->data,
>  				     skb_headlen(skb), DMA_TO_DEVICE);
> +	if (dma_mapping_error(ndev->dev.parent, cur_p->phys))
> +		return NETDEV_TX_BUSY;

This is not ideally tx busy and related to available mem mapping in the system. 
I just looked at other eth drivers and it seems they return TX_OK with drop
count stats incremented.

> +	cur_p->cntrl = skb_headlen(skb) | XAXIDMA_BD_CTRL_TXSOF_MASK;
> 
>  	for (ii = 0; ii < num_frag; ii++) {
>  		if (++lp->tx_bd_tail >= lp->tx_bd_num) @@ -716,6 +724,13 @@
> axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  					     skb_frag_address(frag),
>  					     skb_frag_size(frag),
>  					     DMA_TO_DEVICE);
> +		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
> +			axienet_free_tx_chain(ndev, orig_tail_ptr, ii + 1,
> +					      NULL);
> +			lp->tx_bd_tail = orig_tail_ptr;
> +
> +			return NETDEV_TX_BUSY;
> +		}
>  		cur_p->cntrl = skb_frag_size(frag);
>  	}
> 
> @@ -796,6 +811,11 @@ static void axienet_recv(struct net_device *ndev)
>  		cur_p->phys = dma_map_single(ndev->dev.parent, new_skb-
> >data,
>  					     lp->max_frm_size,
>  					     DMA_FROM_DEVICE);
> +		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
> +			dev_kfree_skb(new_skb);
> +			return;
> +		}
> +
>  		cur_p->cntrl = lp->max_frm_size;
>  		cur_p->status = 0;
>  		cur_p->skb = new_skb;
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 08/14] net: axienet: Drop MDIO interrupt registers from ethtools dump
  2020-01-10 11:54 ` [PATCH 08/14] net: axienet: Drop MDIO interrupt registers from ethtools dump Andre Przywara
@ 2020-01-13  6:02   ` Radhey Shyam Pandey
  0 siblings, 0 replies; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-13  6:02 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 08/14] net: axienet: Drop MDIO interrupt registers from
> ethtools dump
> 
> Newer revisions of the IP don't have these registers. Since we don't
> really use them, just drop them from the ethtools dump.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index e83c7b005f50..7a747345e98e 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1239,10 +1239,6 @@ static void axienet_ethtools_get_regs(struct
> net_device *ndev,
>  	data[20] = axienet_ior(lp, XAE_MDIO_MCR_OFFSET);
>  	data[21] = axienet_ior(lp, XAE_MDIO_MWD_OFFSET);
>  	data[22] = axienet_ior(lp, XAE_MDIO_MRD_OFFSET);
> -	data[23] = axienet_ior(lp, XAE_MDIO_MIS_OFFSET);
> -	data[24] = axienet_ior(lp, XAE_MDIO_MIP_OFFSET);
> -	data[25] = axienet_ior(lp, XAE_MDIO_MIE_OFFSET);
> -	data[26] = axienet_ior(lp, XAE_MDIO_MIC_OFFSET);

We can also remove these #defines from the header.
Alternatively, we can cherry-pick commit f5b9e58 " net: xilinx: axiethernet:
Fix axiethernet register description" from xilinx tree and include it in this
series.
>  	data[27] = axienet_ior(lp, XAE_UAW0_OFFSET);
>  	data[28] = axienet_ior(lp, XAE_UAW1_OFFSET);
>  	data[29] = axienet_ior(lp, XAE_FMI_OFFSET);
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 09/14] net: axienet: Add mii-tool support
  2020-01-10 11:54 ` [PATCH 09/14] net: axienet: Add mii-tool support Andre Przywara
@ 2020-01-13  6:12   ` Radhey Shyam Pandey
  0 siblings, 0 replies; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-13  6:12 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 09/14] net: axienet: Add mii-tool support
> 
> mii-tool is useful for debugging, and all it requires to work is to wire
> up the ioctl ops function pointer.
> Add this to the axienet driver to enable mii-tool.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 7a747345e98e..64f799f3d248 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1152,6 +1152,16 @@ static void axienet_poll_controller(struct net_device
> *ndev)
>  }
>  #endif
> 
> +static int axienet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> +{
> +	struct axienet_local *lp = netdev_priv(dev);
> +
> +	if (!netif_running(dev))
> +		return -EINVAL;

I think phy ioctl should be allowed even if the device is not up. 
Or is there any specific reason for keeping it?

> +
> +	return phylink_mii_ioctl(lp->phylink, rq, cmd);
> +}
> +
>  static const struct net_device_ops axienet_netdev_ops = {
>  	.ndo_open = axienet_open,
>  	.ndo_stop = axienet_stop,
> @@ -1159,6 +1169,7 @@ static const struct net_device_ops
> axienet_netdev_ops = {
>  	.ndo_change_mtu	= axienet_change_mtu,
>  	.ndo_set_mac_address = netdev_set_mac_address,
>  	.ndo_validate_addr = eth_validate_addr,
> +	.ndo_do_ioctl = axienet_ioctl,
>  	.ndo_set_rx_mode = axienet_set_multicast_list,
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	.ndo_poll_controller = axienet_poll_controller,
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 11/14] net: axienet: Upgrade descriptors to hold 64-bit addresses
  2020-01-10 11:54 ` [PATCH 11/14] net: axienet: Upgrade descriptors to hold 64-bit addresses Andre Przywara
@ 2020-01-14 16:35   ` Radhey Shyam Pandey
  2020-01-14 17:29     ` Andre Przywara
  0 siblings, 1 reply; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-14 16:35 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 11/14] net: axienet: Upgrade descriptors to hold 64-bit
> addresses
> 
> Newer revisions of the AXI DMA IP (>= v7.1) support 64-bit addresses,
> both for the descriptors itself, as well as for the buffers they are
> pointing to.
> This is realised by adding "MSB" words for the next and phys pointer
> right behind the existing address word, now named "LSB". These MSB words
> live in formerly reserved areas of the descriptor.
> 
> If the hardware supports it, write both words when setting an address.
> The buffer address is handled by two wrapper functions, the two
> occasions where we set the next pointers are open coded.
> 
> For now this is guarded by a flag which we don't set yet.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet.h  |   9 +-
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 109 ++++++++++++------
>  2 files changed, 81 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> index 2dacfc85b3ba..4aea4c23d3bb 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> @@ -335,6 +335,7 @@
>  #define XAE_FEATURE_PARTIAL_TX_CSUM	(1 << 1)
>  #define XAE_FEATURE_FULL_RX_CSUM	(1 << 2)
>  #define XAE_FEATURE_FULL_TX_CSUM	(1 << 3)
> +#define XAE_FEATURE_DMA_64BIT		(1 << 4)
> 
>  #define XAE_NO_CSUM_OFFLOAD		0
> 
> @@ -347,9 +348,9 @@
>  /**
>   * struct axidma_bd - Axi Dma buffer descriptor layout
>   * @next:         MM2S/S2MM Next Descriptor Pointer
> - * @reserved1:    Reserved and not used
> + * @next_msb:     MM2S/S2MM Next Descriptor Pointer (high 32 bits)
>   * @phys:         MM2S/S2MM Buffer Address
> - * @reserved2:    Reserved and not used
> + * @phys_msb:     MM2S/S2MM Buffer Address (high 32 bits)
>   * @reserved3:    Reserved and not used
>   * @reserved4:    Reserved and not used
>   * @cntrl:        MM2S/S2MM Control value
> @@ -362,9 +363,9 @@
>   */
>  struct axidma_bd {
>  	u32 next;	/* Physical address of next buffer descriptor */
> -	u32 reserved1;
> +	u32 next_msb;	/* high 32 bits for IP >= v7.1, reserved on older IP */
>  	u32 phys;
> -	u32 reserved2;
> +	u32 phys_msb; 	/* for IP >= v7.1, reserved for older IP */
>  	u32 reserved3;
>  	u32 reserved4;
>  	u32 cntrl;
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index bbdda4b0c677..133f088d797e 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -153,6 +153,25 @@ static void axienet_dma_out_addr(struct axienet_local
> *lp, off_t reg,
>  	axienet_dma_out32(lp, reg, lower_32_bits(addr));
>  }
> 
> +static void desc_set_phys_addr(struct axienet_local *lp, dma_addr_t addr,
> +			       struct axidma_bd *desc)
> +{
> +	desc->phys = lower_32_bits(addr);
> +	if (lp->features & XAE_FEATURE_DMA_64BIT)

Instead of set/get_phys_addr API, we can use writeq to update msb bits.
The previous IP version marks msb bits as reserved, and are used in
future IP version.  We can have two dma write functions that are selected
in probe dependending on IP version(new compatible string).  In this way
we can get rid of runtime lp->features check in each IO. 

> +		desc->phys_msb = upper_32_bits(addr);
> +}
> +
> +static dma_addr_t desc_get_phys_addr(struct axienet_local *lp,
> +				     struct axidma_bd *desc)
> +{
> +	dma_addr_t ret = desc->phys;
> +
> +	if (lp->features & XAE_FEATURE_DMA_64BIT)
> +		ret |= (dma_addr_t)desc->phys_msb << 32;
> +
> +	return ret;
> +}
> +
>  /**
>   * axienet_dma_bd_release - Release buffer descriptor rings
>   * @ndev:	Pointer to the net_device structure
> @@ -176,6 +195,8 @@ static void axienet_dma_bd_release(struct net_device
> *ndev)
>  		return;
> 
>  	for (i = 0; i < lp->rx_bd_num; i++) {
> +		dma_addr_t phys;
> +
>  		/* A NULL skb means this descriptor has not been initialised
>  		 * at all.
>  		 */
> @@ -188,9 +209,11 @@ static void axienet_dma_bd_release(struct net_device
> *ndev)
>  		 * descriptor size, after it had been successfully allocated.
>  		 * So a non-zero value in there means we need to unmap it.
>  		 */
> -		if (lp->rx_bd_v[i].cntrl)
> -			dma_unmap_single(ndev->dev.parent, lp-
> >rx_bd_v[i].phys,
> +		if (lp->rx_bd_v[i].cntrl) {
> +			phys = desc_get_phys_addr(lp, &lp->rx_bd_v[i]);
> +			dma_unmap_single(ndev->dev.parent, phys,
>  					 lp->max_frm_size,
> DMA_FROM_DEVICE);
> +		}
>  	}
> 
>  	dma_free_coherent(ndev->dev.parent,
> @@ -235,29 +258,36 @@ static int axienet_dma_bd_init(struct net_device
> *ndev)
>  		goto out;
> 
>  	for (i = 0; i < lp->tx_bd_num; i++) {
> -		lp->tx_bd_v[i].next = lp->tx_bd_p +
> -				      sizeof(*lp->tx_bd_v) *
> -				      ((i + 1) % lp->tx_bd_num);
> +		dma_addr_t addr = lp->tx_bd_p +
> +				  sizeof(*lp->tx_bd_v) *
> +				  ((i + 1) % lp->tx_bd_num);
> +
> +		lp->tx_bd_v[i].next = lower_32_bits(addr);
> +		if (lp->features & XAE_FEATURE_DMA_64BIT)
> +			lp->tx_bd_v[i].next_msb = upper_32_bits(addr);
>  	}
> 
>  	for (i = 0; i < lp->rx_bd_num; i++) {
> -		lp->rx_bd_v[i].next = lp->rx_bd_p +
> -				      sizeof(*lp->rx_bd_v) *
> -				      ((i + 1) % lp->rx_bd_num);
> +		dma_addr_t addr;
> +
> +		addr = lp->rx_bd_p + sizeof(*lp->rx_bd_v) *
> +			((i + 1) % lp->rx_bd_num);
> +		lp->rx_bd_v[i].next = lower_32_bits(addr);
> +		if (lp->features & XAE_FEATURE_DMA_64BIT)
> +			lp->rx_bd_v[i].next_msb = upper_32_bits(addr);
> 
>  		skb = netdev_alloc_skb_ip_align(ndev, lp->max_frm_size);
>  		if (!skb)
>  			goto out;
> 
>  		lp->rx_bd_v[i].skb = skb;
> -		lp->rx_bd_v[i].phys = dma_map_single(ndev->dev.parent,
> -						     skb->data,
> -						     lp->max_frm_size,
> -						     DMA_FROM_DEVICE);
> -		if (dma_mapping_error(ndev->dev.parent, lp->rx_bd_v[i].phys))
> {
> +		addr = dma_map_single(ndev->dev.parent, skb->data,
> +				      lp->max_frm_size, DMA_FROM_DEVICE);
> +		if (dma_mapping_error(ndev->dev.parent, addr)) {
>  			dev_kfree_skb(skb);
>  			goto out;
>  		}
> +		desc_set_phys_addr(lp, addr, &lp->rx_bd_v[i]);
> 
>  		lp->rx_bd_v[i].cntrl = lp->max_frm_size;
>  	}
> @@ -565,6 +595,7 @@ static int axienet_free_tx_chain(struct net_device
> *ndev, u32 first_bd,
>  	struct axienet_local *lp = netdev_priv(ndev);
>  	int max_bds = (nr_bds != -1) ? nr_bds : lp->tx_bd_num;
>  	struct axidma_bd *cur_p;
> +	dma_addr_t phys;
>  	unsigned int status;
>  	int i;
> 
> @@ -578,7 +609,8 @@ static int axienet_free_tx_chain(struct net_device
> *ndev, u32 first_bd,
>  		if (nr_bds == -1 && !(status &
> XAXIDMA_BD_STS_COMPLETE_MASK))
>  			break;
> 
> -		dma_unmap_single(ndev->dev.parent, cur_p->phys,
> +		phys = desc_get_phys_addr(lp, cur_p);
> +		dma_unmap_single(ndev->dev.parent, phys,
>  				(cur_p->cntrl &
> XAXIDMA_BD_CTRL_LENGTH_MASK),
>  				DMA_TO_DEVICE);
> 
> @@ -676,7 +708,7 @@ axienet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
>  	u32 csum_start_off;
>  	u32 csum_index_off;
>  	skb_frag_t *frag;
> -	dma_addr_t tail_p;
> +	dma_addr_t tail_p, phys;
>  	struct axienet_local *lp = netdev_priv(ndev);
>  	struct axidma_bd *cur_p;
>  	u32 orig_tail_ptr = lp->tx_bd_tail;
> @@ -715,10 +747,11 @@ axienet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
>  		cur_p->app0 |= 2; /* Tx Full Checksum Offload Enabled */
>  	}
> 
> -	cur_p->phys = dma_map_single(ndev->dev.parent, skb->data,
> -				     skb_headlen(skb), DMA_TO_DEVICE);
> -	if (dma_mapping_error(ndev->dev.parent, cur_p->phys))
> +	phys = dma_map_single(ndev->dev.parent, skb->data,
> +			      skb_headlen(skb), DMA_TO_DEVICE);
> +	if (dma_mapping_error(ndev->dev.parent, phys))
>  		return NETDEV_TX_BUSY;
> +	desc_set_phys_addr(lp, phys, cur_p);
>  	cur_p->cntrl = skb_headlen(skb) | XAXIDMA_BD_CTRL_TXSOF_MASK;
> 
>  	for (ii = 0; ii < num_frag; ii++) {
> @@ -726,17 +759,18 @@ axienet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
>  			lp->tx_bd_tail = 0;
>  		cur_p = &lp->tx_bd_v[lp->tx_bd_tail];
>  		frag = &skb_shinfo(skb)->frags[ii];
> -		cur_p->phys = dma_map_single(ndev->dev.parent,
> -					     skb_frag_address(frag),
> -					     skb_frag_size(frag),
> -					     DMA_TO_DEVICE);
> -		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
> +		phys = dma_map_single(ndev->dev.parent,
> +				      skb_frag_address(frag),
> +				      skb_frag_size(frag),
> +				      DMA_TO_DEVICE);
> +		if (dma_mapping_error(ndev->dev.parent, phys)) {
>  			axienet_free_tx_chain(ndev, orig_tail_ptr, ii + 1,
>  					      NULL);
>  			lp->tx_bd_tail = orig_tail_ptr;
> 
>  			return NETDEV_TX_BUSY;
>  		}
> +		desc_set_phys_addr(lp, phys, cur_p);
>  		cur_p->cntrl = skb_frag_size(frag);
>  	}
> 
> @@ -775,10 +809,12 @@ static void axienet_recv(struct net_device *ndev)
>  	cur_p = &lp->rx_bd_v[lp->rx_bd_ci];
> 
>  	while ((cur_p->status & XAXIDMA_BD_STS_COMPLETE_MASK)) {
> +		dma_addr_t phys;
> +
>  		tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci;
> 
> -		dma_unmap_single(ndev->dev.parent, cur_p->phys,
> -				 lp->max_frm_size,
> +		phys = desc_get_phys_addr(lp, cur_p);
> +		dma_unmap_single(ndev->dev.parent, phys, lp->max_frm_size,
>  				 DMA_FROM_DEVICE);
> 
>  		skb = cur_p->skb;
> @@ -814,13 +850,14 @@ static void axienet_recv(struct net_device *ndev)
>  		if (!new_skb)
>  			return;
> 
> -		cur_p->phys = dma_map_single(ndev->dev.parent, new_skb-
> >data,
> -					     lp->max_frm_size,
> -					     DMA_FROM_DEVICE);
> -		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
> +		phys = dma_map_single(ndev->dev.parent, new_skb->data,
> +				      lp->max_frm_size,
> +				      DMA_FROM_DEVICE);
> +		if (dma_mapping_error(ndev->dev.parent, phys)) {
>  			dev_kfree_skb(new_skb);
>  			return;
>  		}
> +		desc_set_phys_addr(lp, phys, cur_p);
> 
>  		cur_p->cntrl = lp->max_frm_size;
>  		cur_p->status = 0;
> @@ -865,7 +902,8 @@ static irqreturn_t axienet_tx_irq(int irq, void *_ndev)
>  		return IRQ_NONE;
>  	if (status & XAXIDMA_IRQ_ERROR_MASK) {
>  		dev_err(&ndev->dev, "DMA Tx error 0x%x\n", status);
> -		dev_err(&ndev->dev, "Current BD is at: 0x%x\n",
> +		dev_err(&ndev->dev, "Current BD is at: 0x%x%08x\n",
> +			(lp->tx_bd_v[lp->tx_bd_ci]).phys_msb,
>  			(lp->tx_bd_v[lp->tx_bd_ci]).phys);
> 
>  		cr = axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET);
> @@ -914,7 +952,8 @@ static irqreturn_t axienet_rx_irq(int irq, void *_ndev)
>  		return IRQ_NONE;
>  	if (status & XAXIDMA_IRQ_ERROR_MASK) {
>  		dev_err(&ndev->dev, "DMA Rx error 0x%x\n", status);
> -		dev_err(&ndev->dev, "Current BD is at: 0x%x\n",
> +		dev_err(&ndev->dev, "Current BD is at: 0x%x%08x\n",
> +			(lp->rx_bd_v[lp->rx_bd_ci]).phys_msb,
>  			(lp->rx_bd_v[lp->rx_bd_ci]).phys);
> 
>  		cr = axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET);
> @@ -1622,14 +1661,18 @@ static void axienet_dma_err_handler(unsigned long
> data)
> 
>  	for (i = 0; i < lp->tx_bd_num; i++) {
>  		cur_p = &lp->tx_bd_v[i];
> -		if (cur_p->cntrl)
> -			dma_unmap_single(ndev->dev.parent, cur_p->phys,
> +		if (cur_p->cntrl) {
> +			dma_addr_t addr = desc_get_phys_addr(lp, cur_p);
> +
> +			dma_unmap_single(ndev->dev.parent, addr,
>  					 (cur_p->cntrl &
>  					  XAXIDMA_BD_CTRL_LENGTH_MASK),
>  					 DMA_TO_DEVICE);
> +		}
>  		if (cur_p->skb)
>  			dev_kfree_skb_irq(cur_p->skb);
>  		cur_p->phys = 0;
> +		cur_p->phys_msb = 0;
>  		cur_p->cntrl = 0;
>  		cur_p->status = 0;
>  		cur_p->app0 = 0;
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
  2020-01-10 11:54 ` [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability Andre Przywara
  2020-01-10 14:08   ` Andrew Lunn
@ 2020-01-14 17:03   ` Radhey Shyam Pandey
  2020-01-14 17:41     ` Andre Przywara
  1 sibling, 1 reply; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-14 17:03 UTC (permalink / raw)
  To: Andre Przywara, David S . Miller
  Cc: Robert Hancock, netdev, Michal Simek, linux-kernel, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Friday, January 10, 2020 5:24 PM
> To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> <radheys@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
> 
> When newer revisions of the Axienet IP are configured for a 64-bit bus,
I assume in design axidma address width is set to 64-bits. 
If not, please provide an overview of the design connections.

> we *need* to write to the MSB part of the an address registers,
> otherwise the IP won't recognise this as a DMA start condition.
> This is even true when the actual DMA address comes from the lower 4 GB.
> 
> To autodetect this configuration, at probe time we write all 1's to such
Is reading address width axidma IP user parameter(c_addr_width) from
the design not sufficient to detect configured bus width?

> an MSB register, and see if any bits stick. If this is configured for a
> 32-bit bus, those MSB registers are RES0, so reading back 0 indicates
> that no MSB writes are necessary.
> On the other hands reading anything other than 0 indicated the need to
> write the MSB registers, so we set the respective flag.
> 
> For now this leaves the actual DMA mask at 32-bit, as we can't reliably
> detect the actually wired number of address lines beyond 32.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet.h  |  1 +
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 27 +++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> index 4aea4c23d3bb..4feaaa02819c 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> @@ -161,6 +161,7 @@
>  #define XAE_FCC_OFFSET		0x0000040C /* Flow Control
> Configuration */
>  #define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode
> configuration */
>  #define XAE_PHYC_OFFSET		0x00000414 /* RGMII/SGMII
> configuration */
> +#define XAE_ID_OFFSET		0x000004F8 /* Identification register
> */
>  #define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management
> Config */
>  #define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management
> Control */
>  #define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write
> Data */
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 133f088d797e..f7f593df0c11 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -151,6 +151,9 @@ static void axienet_dma_out_addr(struct axienet_local
> *lp, off_t reg,
>  				 dma_addr_t addr)
>  {
>  	axienet_dma_out32(lp, reg, lower_32_bits(addr));
> +
> +	if (lp->features & XAE_FEATURE_DMA_64BIT)
> +		axienet_dma_out32(lp, reg + 4, upper_32_bits(addr));
>  }
> 
>  static void desc_set_phys_addr(struct axienet_local *lp, dma_addr_t addr,
> @@ -1934,6 +1937,30 @@ static int axienet_probe(struct platform_device
> *pdev)
>  		goto free_netdev;
>  	}
> 
> +	/*
> +	 * Autodetect the need for 64-bit DMA pointers.
> +	 * When the IP is configured for a bus width bigger than 32 bits,
> +	 * writing the MSB registers is mandatory, even if they are all 0.
> +	 * We can detect this case by writing all 1's to one such register
> +	 * and see if that sticks: when the IP is configured for 32 bits
> +	 * only, those registers are RES0.
> +	 * Those MSB registers were introduced in IP v7.1, which we check first.
> +	 */
> +	if ((axienet_ior(lp, XAE_ID_OFFSET) >> 24) >= 0x9) {
> +		void __iomem *desc = lp->dma_regs +
> XAXIDMA_TX_CDESC_OFFSET + 4;
> +
> +		iowrite32(0x0, desc);
> +		if (ioread32(desc) == 0) {	/* sanity check */
> +			iowrite32(0xffffffff, desc);
> +			if (ioread32(desc) > 0) {
> +				lp->features |= XAE_FEATURE_DMA_64BIT;
> +				dev_info(&pdev->dev,
> +					 "autodetected 64-bit DMA range\n");
> +			}
> +			iowrite32(0x0, desc);
> +		}
> +	}
> +
>  	/* Check for Ethernet core IRQ (optional) */
>  	if (lp->eth_irq <= 0)
>  		dev_info(&pdev->dev, "Ethernet core IRQ not defined\n");
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 11/14] net: axienet: Upgrade descriptors to hold 64-bit addresses
  2020-01-14 16:35   ` Radhey Shyam Pandey
@ 2020-01-14 17:29     ` Andre Przywara
  0 siblings, 0 replies; 52+ messages in thread
From: Andre Przywara @ 2020-01-14 17:29 UTC (permalink / raw)
  To: Radhey Shyam Pandey
  Cc: netdev, linux-kernel, Robert Hancock, Michal Simek,
	David S . Miller, linux-arm-kernel

On Tue, 14 Jan 2020 16:35:10 +0000
Radhey Shyam Pandey <radheys@xilinx.com> wrote:

Hi Radhey,

> > -----Original Message-----
> > From: Andre Przywara <andre.przywara@arm.com>
> > Sent: Friday, January 10, 2020 5:24 PM
> > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> > <radheys@xilinx.com>
> > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH 11/14] net: axienet: Upgrade descriptors to hold 64-bit
> > addresses
> > 
> > Newer revisions of the AXI DMA IP (>= v7.1) support 64-bit addresses,
> > both for the descriptors itself, as well as for the buffers they are
> > pointing to.
> > This is realised by adding "MSB" words for the next and phys pointer
> > right behind the existing address word, now named "LSB". These MSB words
> > live in formerly reserved areas of the descriptor.
> > 
> > If the hardware supports it, write both words when setting an address.
> > The buffer address is handled by two wrapper functions, the two
> > occasions where we set the next pointers are open coded.
> > 
> > For now this is guarded by a flag which we don't set yet.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/net/ethernet/xilinx/xilinx_axienet.h  |   9 +-
> >  .../net/ethernet/xilinx/xilinx_axienet_main.c | 109 ++++++++++++------
> >  2 files changed, 81 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > index 2dacfc85b3ba..4aea4c23d3bb 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > @@ -335,6 +335,7 @@
> >  #define XAE_FEATURE_PARTIAL_TX_CSUM	(1 << 1)
> >  #define XAE_FEATURE_FULL_RX_CSUM	(1 << 2)
> >  #define XAE_FEATURE_FULL_TX_CSUM	(1 << 3)
> > +#define XAE_FEATURE_DMA_64BIT		(1 << 4)
> > 
> >  #define XAE_NO_CSUM_OFFLOAD		0
> > 
> > @@ -347,9 +348,9 @@
> >  /**
> >   * struct axidma_bd - Axi Dma buffer descriptor layout
> >   * @next:         MM2S/S2MM Next Descriptor Pointer
> > - * @reserved1:    Reserved and not used
> > + * @next_msb:     MM2S/S2MM Next Descriptor Pointer (high 32 bits)
> >   * @phys:         MM2S/S2MM Buffer Address
> > - * @reserved2:    Reserved and not used
> > + * @phys_msb:     MM2S/S2MM Buffer Address (high 32 bits)
> >   * @reserved3:    Reserved and not used
> >   * @reserved4:    Reserved and not used
> >   * @cntrl:        MM2S/S2MM Control value
> > @@ -362,9 +363,9 @@
> >   */
> >  struct axidma_bd {
> >  	u32 next;	/* Physical address of next buffer descriptor */
> > -	u32 reserved1;
> > +	u32 next_msb;	/* high 32 bits for IP >= v7.1, reserved on older IP */
> >  	u32 phys;
> > -	u32 reserved2;
> > +	u32 phys_msb; 	/* for IP >= v7.1, reserved for older IP */
> >  	u32 reserved3;
> >  	u32 reserved4;
> >  	u32 cntrl;
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > index bbdda4b0c677..133f088d797e 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > @@ -153,6 +153,25 @@ static void axienet_dma_out_addr(struct axienet_local
> > *lp, off_t reg,
> >  	axienet_dma_out32(lp, reg, lower_32_bits(addr));
> >  }
> > 
> > +static void desc_set_phys_addr(struct axienet_local *lp, dma_addr_t addr,
> > +			       struct axidma_bd *desc)
> > +{
> > +	desc->phys = lower_32_bits(addr);
> > +	if (lp->features & XAE_FEATURE_DMA_64BIT)  
> 
> Instead of set/get_phys_addr API, we can use writeq to update msb bits.
> The previous IP version marks msb bits as reserved, and are used in
> future IP version.  We can have two dma write functions that are selected
> in probe dependending on IP version(new compatible string).  In this way
> we can get rid of runtime lp->features check in each IO.

I am not sure that writeq is a good idea. The documentation clearly speaks of 32-bit registers, so we should access them as such. How the bus implements 64-bit writes is a different story. I see that is seems to work in practice (TM), but what is exactly the problem you want to solve here?
Are you concerned about the extra flag check each time we call desc_set_phys_addr()? And want to replace this with a function pointer?

Cheers,
Andre.

> 
> > +		desc->phys_msb = upper_32_bits(addr);
> > +}
> > +
> > +static dma_addr_t desc_get_phys_addr(struct axienet_local *lp,
> > +				     struct axidma_bd *desc)
> > +{
> > +	dma_addr_t ret = desc->phys;
> > +
> > +	if (lp->features & XAE_FEATURE_DMA_64BIT)
> > +		ret |= (dma_addr_t)desc->phys_msb << 32;
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * axienet_dma_bd_release - Release buffer descriptor rings
> >   * @ndev:	Pointer to the net_device structure
> > @@ -176,6 +195,8 @@ static void axienet_dma_bd_release(struct net_device
> > *ndev)
> >  		return;
> > 
> >  	for (i = 0; i < lp->rx_bd_num; i++) {
> > +		dma_addr_t phys;
> > +
> >  		/* A NULL skb means this descriptor has not been initialised
> >  		 * at all.
> >  		 */
> > @@ -188,9 +209,11 @@ static void axienet_dma_bd_release(struct net_device
> > *ndev)
> >  		 * descriptor size, after it had been successfully allocated.
> >  		 * So a non-zero value in there means we need to unmap it.
> >  		 */
> > -		if (lp->rx_bd_v[i].cntrl)
> > -			dma_unmap_single(ndev->dev.parent, lp-  
> > >rx_bd_v[i].phys,  
> > +		if (lp->rx_bd_v[i].cntrl) {
> > +			phys = desc_get_phys_addr(lp, &lp->rx_bd_v[i]);
> > +			dma_unmap_single(ndev->dev.parent, phys,
> >  					 lp->max_frm_size,
> > DMA_FROM_DEVICE);
> > +		}
> >  	}
> > 
> >  	dma_free_coherent(ndev->dev.parent,
> > @@ -235,29 +258,36 @@ static int axienet_dma_bd_init(struct net_device
> > *ndev)
> >  		goto out;
> > 
> >  	for (i = 0; i < lp->tx_bd_num; i++) {
> > -		lp->tx_bd_v[i].next = lp->tx_bd_p +
> > -				      sizeof(*lp->tx_bd_v) *
> > -				      ((i + 1) % lp->tx_bd_num);
> > +		dma_addr_t addr = lp->tx_bd_p +
> > +				  sizeof(*lp->tx_bd_v) *
> > +				  ((i + 1) % lp->tx_bd_num);
> > +
> > +		lp->tx_bd_v[i].next = lower_32_bits(addr);
> > +		if (lp->features & XAE_FEATURE_DMA_64BIT)
> > +			lp->tx_bd_v[i].next_msb = upper_32_bits(addr);
> >  	}
> > 
> >  	for (i = 0; i < lp->rx_bd_num; i++) {
> > -		lp->rx_bd_v[i].next = lp->rx_bd_p +
> > -				      sizeof(*lp->rx_bd_v) *
> > -				      ((i + 1) % lp->rx_bd_num);
> > +		dma_addr_t addr;
> > +
> > +		addr = lp->rx_bd_p + sizeof(*lp->rx_bd_v) *
> > +			((i + 1) % lp->rx_bd_num);
> > +		lp->rx_bd_v[i].next = lower_32_bits(addr);
> > +		if (lp->features & XAE_FEATURE_DMA_64BIT)
> > +			lp->rx_bd_v[i].next_msb = upper_32_bits(addr);
> > 
> >  		skb = netdev_alloc_skb_ip_align(ndev, lp->max_frm_size);
> >  		if (!skb)
> >  			goto out;
> > 
> >  		lp->rx_bd_v[i].skb = skb;
> > -		lp->rx_bd_v[i].phys = dma_map_single(ndev->dev.parent,
> > -						     skb->data,
> > -						     lp->max_frm_size,
> > -						     DMA_FROM_DEVICE);
> > -		if (dma_mapping_error(ndev->dev.parent, lp->rx_bd_v[i].phys))
> > {
> > +		addr = dma_map_single(ndev->dev.parent, skb->data,
> > +				      lp->max_frm_size, DMA_FROM_DEVICE);
> > +		if (dma_mapping_error(ndev->dev.parent, addr)) {
> >  			dev_kfree_skb(skb);
> >  			goto out;
> >  		}
> > +		desc_set_phys_addr(lp, addr, &lp->rx_bd_v[i]);
> > 
> >  		lp->rx_bd_v[i].cntrl = lp->max_frm_size;
> >  	}
> > @@ -565,6 +595,7 @@ static int axienet_free_tx_chain(struct net_device
> > *ndev, u32 first_bd,
> >  	struct axienet_local *lp = netdev_priv(ndev);
> >  	int max_bds = (nr_bds != -1) ? nr_bds : lp->tx_bd_num;
> >  	struct axidma_bd *cur_p;
> > +	dma_addr_t phys;
> >  	unsigned int status;
> >  	int i;
> > 
> > @@ -578,7 +609,8 @@ static int axienet_free_tx_chain(struct net_device
> > *ndev, u32 first_bd,
> >  		if (nr_bds == -1 && !(status &
> > XAXIDMA_BD_STS_COMPLETE_MASK))
> >  			break;
> > 
> > -		dma_unmap_single(ndev->dev.parent, cur_p->phys,
> > +		phys = desc_get_phys_addr(lp, cur_p);
> > +		dma_unmap_single(ndev->dev.parent, phys,
> >  				(cur_p->cntrl &
> > XAXIDMA_BD_CTRL_LENGTH_MASK),
> >  				DMA_TO_DEVICE);
> > 
> > @@ -676,7 +708,7 @@ axienet_start_xmit(struct sk_buff *skb, struct
> > net_device *ndev)
> >  	u32 csum_start_off;
> >  	u32 csum_index_off;
> >  	skb_frag_t *frag;
> > -	dma_addr_t tail_p;
> > +	dma_addr_t tail_p, phys;
> >  	struct axienet_local *lp = netdev_priv(ndev);
> >  	struct axidma_bd *cur_p;
> >  	u32 orig_tail_ptr = lp->tx_bd_tail;
> > @@ -715,10 +747,11 @@ axienet_start_xmit(struct sk_buff *skb, struct
> > net_device *ndev)
> >  		cur_p->app0 |= 2; /* Tx Full Checksum Offload Enabled */
> >  	}
> > 
> > -	cur_p->phys = dma_map_single(ndev->dev.parent, skb->data,
> > -				     skb_headlen(skb), DMA_TO_DEVICE);
> > -	if (dma_mapping_error(ndev->dev.parent, cur_p->phys))
> > +	phys = dma_map_single(ndev->dev.parent, skb->data,
> > +			      skb_headlen(skb), DMA_TO_DEVICE);
> > +	if (dma_mapping_error(ndev->dev.parent, phys))
> >  		return NETDEV_TX_BUSY;
> > +	desc_set_phys_addr(lp, phys, cur_p);
> >  	cur_p->cntrl = skb_headlen(skb) | XAXIDMA_BD_CTRL_TXSOF_MASK;
> > 
> >  	for (ii = 0; ii < num_frag; ii++) {
> > @@ -726,17 +759,18 @@ axienet_start_xmit(struct sk_buff *skb, struct
> > net_device *ndev)
> >  			lp->tx_bd_tail = 0;
> >  		cur_p = &lp->tx_bd_v[lp->tx_bd_tail];
> >  		frag = &skb_shinfo(skb)->frags[ii];
> > -		cur_p->phys = dma_map_single(ndev->dev.parent,
> > -					     skb_frag_address(frag),
> > -					     skb_frag_size(frag),
> > -					     DMA_TO_DEVICE);
> > -		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
> > +		phys = dma_map_single(ndev->dev.parent,
> > +				      skb_frag_address(frag),
> > +				      skb_frag_size(frag),
> > +				      DMA_TO_DEVICE);
> > +		if (dma_mapping_error(ndev->dev.parent, phys)) {
> >  			axienet_free_tx_chain(ndev, orig_tail_ptr, ii + 1,
> >  					      NULL);
> >  			lp->tx_bd_tail = orig_tail_ptr;
> > 
> >  			return NETDEV_TX_BUSY;
> >  		}
> > +		desc_set_phys_addr(lp, phys, cur_p);
> >  		cur_p->cntrl = skb_frag_size(frag);
> >  	}
> > 
> > @@ -775,10 +809,12 @@ static void axienet_recv(struct net_device *ndev)
> >  	cur_p = &lp->rx_bd_v[lp->rx_bd_ci];
> > 
> >  	while ((cur_p->status & XAXIDMA_BD_STS_COMPLETE_MASK)) {
> > +		dma_addr_t phys;
> > +
> >  		tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci;
> > 
> > -		dma_unmap_single(ndev->dev.parent, cur_p->phys,
> > -				 lp->max_frm_size,
> > +		phys = desc_get_phys_addr(lp, cur_p);
> > +		dma_unmap_single(ndev->dev.parent, phys, lp->max_frm_size,
> >  				 DMA_FROM_DEVICE);
> > 
> >  		skb = cur_p->skb;
> > @@ -814,13 +850,14 @@ static void axienet_recv(struct net_device *ndev)
> >  		if (!new_skb)
> >  			return;
> > 
> > -		cur_p->phys = dma_map_single(ndev->dev.parent, new_skb-  
> > >data,  
> > -					     lp->max_frm_size,
> > -					     DMA_FROM_DEVICE);
> > -		if (dma_mapping_error(ndev->dev.parent, cur_p->phys)) {
> > +		phys = dma_map_single(ndev->dev.parent, new_skb->data,
> > +				      lp->max_frm_size,
> > +				      DMA_FROM_DEVICE);
> > +		if (dma_mapping_error(ndev->dev.parent, phys)) {
> >  			dev_kfree_skb(new_skb);
> >  			return;
> >  		}
> > +		desc_set_phys_addr(lp, phys, cur_p);
> > 
> >  		cur_p->cntrl = lp->max_frm_size;
> >  		cur_p->status = 0;
> > @@ -865,7 +902,8 @@ static irqreturn_t axienet_tx_irq(int irq, void *_ndev)
> >  		return IRQ_NONE;
> >  	if (status & XAXIDMA_IRQ_ERROR_MASK) {
> >  		dev_err(&ndev->dev, "DMA Tx error 0x%x\n", status);
> > -		dev_err(&ndev->dev, "Current BD is at: 0x%x\n",
> > +		dev_err(&ndev->dev, "Current BD is at: 0x%x%08x\n",
> > +			(lp->tx_bd_v[lp->tx_bd_ci]).phys_msb,
> >  			(lp->tx_bd_v[lp->tx_bd_ci]).phys);
> > 
> >  		cr = axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET);
> > @@ -914,7 +952,8 @@ static irqreturn_t axienet_rx_irq(int irq, void *_ndev)
> >  		return IRQ_NONE;
> >  	if (status & XAXIDMA_IRQ_ERROR_MASK) {
> >  		dev_err(&ndev->dev, "DMA Rx error 0x%x\n", status);
> > -		dev_err(&ndev->dev, "Current BD is at: 0x%x\n",
> > +		dev_err(&ndev->dev, "Current BD is at: 0x%x%08x\n",
> > +			(lp->rx_bd_v[lp->rx_bd_ci]).phys_msb,
> >  			(lp->rx_bd_v[lp->rx_bd_ci]).phys);
> > 
> >  		cr = axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET);
> > @@ -1622,14 +1661,18 @@ static void axienet_dma_err_handler(unsigned long
> > data)
> > 
> >  	for (i = 0; i < lp->tx_bd_num; i++) {
> >  		cur_p = &lp->tx_bd_v[i];
> > -		if (cur_p->cntrl)
> > -			dma_unmap_single(ndev->dev.parent, cur_p->phys,
> > +		if (cur_p->cntrl) {
> > +			dma_addr_t addr = desc_get_phys_addr(lp, cur_p);
> > +
> > +			dma_unmap_single(ndev->dev.parent, addr,
> >  					 (cur_p->cntrl &
> >  					  XAXIDMA_BD_CTRL_LENGTH_MASK),
> >  					 DMA_TO_DEVICE);
> > +		}
> >  		if (cur_p->skb)
> >  			dev_kfree_skb_irq(cur_p->skb);
> >  		cur_p->phys = 0;
> > +		cur_p->phys_msb = 0;
> >  		cur_p->cntrl = 0;
> >  		cur_p->status = 0;
> >  		cur_p->app0 = 0;
> > --
> > 2.17.1  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
  2020-01-14 17:03   ` Radhey Shyam Pandey
@ 2020-01-14 17:41     ` Andre Przywara
  2020-01-15  6:02       ` Radhey Shyam Pandey
  0 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-14 17:41 UTC (permalink / raw)
  To: Radhey Shyam Pandey
  Cc: netdev, linux-kernel, Robert Hancock, Michal Simek,
	David S . Miller, linux-arm-kernel

On Tue, 14 Jan 2020 17:03:41 +0000
Radhey Shyam Pandey <radheys@xilinx.com> wrote:

Hi,

> > -----Original Message-----
> > From: Andre Przywara <andre.przywara@arm.com>
> > Sent: Friday, January 10, 2020 5:24 PM
> > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> > <radheys@xilinx.com>
> > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
> > 
> > When newer revisions of the Axienet IP are configured for a 64-bit bus,  
> I assume in design axidma address width is set to 64-bits. 

So I wrote "64-bit bus" here, but really meant: "address bus wider than 32 bits". In our case it's set to 40 bits, because that's how wide the other busses in the system are.
And we have memory from 2GB to 4GB, and from 34GB till 40GB, but not in-between (VExpress/Juno memory layout).

> If not, please provide an overview of the design connections.

The exact parameter name from PG021 is: "Address Width (32-64) / c_addr_width".

> > we *need* to write to the MSB part of the an address registers,
> > otherwise the IP won't recognise this as a DMA start condition.
> > This is even true when the actual DMA address comes from the lower 4 GB.
> > 
> > To autodetect this configuration, at probe time we write all 1's to such  
> Is reading address width axidma IP user parameter(c_addr_width) from
> the design not sufficient to detect configured bus width?

What do you mean by that? Reading from where? Is there a way to access those parameters from a running system?

Cheers,
Andre.

> > an MSB register, and see if any bits stick. If this is configured for a
> > 32-bit bus, those MSB registers are RES0, so reading back 0 indicates
> > that no MSB writes are necessary.
> > On the other hands reading anything other than 0 indicated the need to
> > write the MSB registers, so we set the respective flag.
> > 
> > For now this leaves the actual DMA mask at 32-bit, as we can't reliably
> > detect the actually wired number of address lines beyond 32.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/net/ethernet/xilinx/xilinx_axienet.h  |  1 +
> >  .../net/ethernet/xilinx/xilinx_axienet_main.c | 27 +++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > index 4aea4c23d3bb..4feaaa02819c 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > @@ -161,6 +161,7 @@
> >  #define XAE_FCC_OFFSET		0x0000040C /* Flow Control
> > Configuration */
> >  #define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode
> > configuration */
> >  #define XAE_PHYC_OFFSET		0x00000414 /* RGMII/SGMII
> > configuration */
> > +#define XAE_ID_OFFSET		0x000004F8 /* Identification register
> > */
> >  #define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management
> > Config */
> >  #define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management
> > Control */
> >  #define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write
> > Data */
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > index 133f088d797e..f7f593df0c11 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > @@ -151,6 +151,9 @@ static void axienet_dma_out_addr(struct axienet_local
> > *lp, off_t reg,
> >  				 dma_addr_t addr)
> >  {
> >  	axienet_dma_out32(lp, reg, lower_32_bits(addr));
> > +
> > +	if (lp->features & XAE_FEATURE_DMA_64BIT)
> > +		axienet_dma_out32(lp, reg + 4, upper_32_bits(addr));
> >  }
> > 
> >  static void desc_set_phys_addr(struct axienet_local *lp, dma_addr_t addr,
> > @@ -1934,6 +1937,30 @@ static int axienet_probe(struct platform_device
> > *pdev)
> >  		goto free_netdev;
> >  	}
> > 
> > +	/*
> > +	 * Autodetect the need for 64-bit DMA pointers.
> > +	 * When the IP is configured for a bus width bigger than 32 bits,
> > +	 * writing the MSB registers is mandatory, even if they are all 0.
> > +	 * We can detect this case by writing all 1's to one such register
> > +	 * and see if that sticks: when the IP is configured for 32 bits
> > +	 * only, those registers are RES0.
> > +	 * Those MSB registers were introduced in IP v7.1, which we check first.
> > +	 */
> > +	if ((axienet_ior(lp, XAE_ID_OFFSET) >> 24) >= 0x9) {
> > +		void __iomem *desc = lp->dma_regs +
> > XAXIDMA_TX_CDESC_OFFSET + 4;
> > +
> > +		iowrite32(0x0, desc);
> > +		if (ioread32(desc) == 0) {	/* sanity check */
> > +			iowrite32(0xffffffff, desc);
> > +			if (ioread32(desc) > 0) {
> > +				lp->features |= XAE_FEATURE_DMA_64BIT;
> > +				dev_info(&pdev->dev,
> > +					 "autodetected 64-bit DMA range\n");
> > +			}
> > +			iowrite32(0x0, desc);
> > +		}
> > +	}
> > +
> >  	/* Check for Ethernet core IRQ (optional) */
> >  	if (lp->eth_irq <= 0)
> >  		dev_info(&pdev->dev, "Ethernet core IRQ not defined\n");
> > --
> > 2.17.1  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
  2020-01-14 17:41     ` Andre Przywara
@ 2020-01-15  6:02       ` Radhey Shyam Pandey
  0 siblings, 0 replies; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-15  6:02 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, linux-kernel, Robert Hancock, Michal Simek,
	David S . Miller, linux-arm-kernel

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Tuesday, January 14, 2020 11:12 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: David S . Miller <davem@davemloft.net>; Michal Simek
> <michals@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>;
> netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability
> 
> On Tue, 14 Jan 2020 17:03:41 +0000
> Radhey Shyam Pandey <radheys@xilinx.com> wrote:
> 
> Hi,
> 
> > > -----Original Message-----
> > > From: Andre Przywara <andre.przywara@arm.com>
> > > Sent: Friday, January 10, 2020 5:24 PM
> > > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> > > <radheys@xilinx.com>
> > > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> > > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > Subject: [PATCH 12/14] net: axienet: Autodetect 64-bit DMA
> > > capability
> > >
> > > When newer revisions of the Axienet IP are configured for a 64-bit
> > > bus,
> > I assume in design axidma address width is set to 64-bits.
> 
> So I wrote "64-bit bus" here, but really meant: "address bus wider than 32 bits".
> In our case it's set to 40 bits, because that's how wide the other busses in the
> system are.
> And we have memory from 2GB to 4GB, and from 34GB till 40GB, but not in-
> between (VExpress/Juno memory layout).
> 
> > If not, please provide an overview of the design connections.
> 
> The exact parameter name from PG021 is: "Address Width (32-64) /
> c_addr_width".
Thanks. It's right.

> 
> > > we *need* to write to the MSB part of the an address registers,
> > > otherwise the IP won't recognise this as a DMA start condition.
> > > This is even true when the actual DMA address comes from the lower 4 GB.
> > >
> > > To autodetect this configuration, at probe time we write all 1's to
> > > such
> > Is reading address width axidma IP user parameter(c_addr_width) from
> > the design not sufficient to detect configured bus width?
> 
> What do you mean by that? Reading from where? Is there a way to access those
> parameters from a running system?
Hardware design data i.e IP parameter can be accessed using hsi in-built commands.
Please refer to ug1138-generating-basic-software-platforms.pdf, chapter-4.
The same flow is used in DT,  xilinx device tree generator parses HDF and read IP 
parameters and populate DT binding.

> 
> Cheers,
> Andre.
> 
> > > an MSB register, and see if any bits stick. If this is configured
> > > for a 32-bit bus, those MSB registers are RES0, so reading back 0
> > > indicates that no MSB writes are necessary.
> > > On the other hands reading anything other than 0 indicated the need
> > > to write the MSB registers, so we set the respective flag.
> > >
> > > For now this leaves the actual DMA mask at 32-bit, as we can't
> > > reliably detect the actually wired number of address lines beyond 32.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  drivers/net/ethernet/xilinx/xilinx_axienet.h  |  1 +
> > > .../net/ethernet/xilinx/xilinx_axienet_main.c | 27
> > > +++++++++++++++++++
> > >  2 files changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > > b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > > index 4aea4c23d3bb..4feaaa02819c 100644
> > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > > @@ -161,6 +161,7 @@
> > >  #define XAE_FCC_OFFSET		0x0000040C /* Flow Control
> > > Configuration */
> > >  #define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode
> > > configuration */
> > >  #define XAE_PHYC_OFFSET		0x00000414 /* RGMII/SGMII
> > > configuration */
> > > +#define XAE_ID_OFFSET		0x000004F8 /* Identification register
> > > */
> > >  #define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management
> > > Config */
> > >  #define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management
> > > Control */
> > >  #define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write
> > > Data */
> > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > index 133f088d797e..f7f593df0c11 100644
> > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > @@ -151,6 +151,9 @@ static void axienet_dma_out_addr(struct
> > > axienet_local *lp, off_t reg,
> > >  				 dma_addr_t addr)
> > >  {
> > >  	axienet_dma_out32(lp, reg, lower_32_bits(addr));
> > > +
> > > +	if (lp->features & XAE_FEATURE_DMA_64BIT)
> > > +		axienet_dma_out32(lp, reg + 4, upper_32_bits(addr));
> > >  }
> > >
> > >  static void desc_set_phys_addr(struct axienet_local *lp, dma_addr_t
> > > addr, @@ -1934,6 +1937,30 @@ static int axienet_probe(struct
> > > platform_device
> > > *pdev)
> > >  		goto free_netdev;
> > >  	}
> > >
> > > +	/*
> > > +	 * Autodetect the need for 64-bit DMA pointers.
> > > +	 * When the IP is configured for a bus width bigger than 32 bits,
> > > +	 * writing the MSB registers is mandatory, even if they are all 0.
> > > +	 * We can detect this case by writing all 1's to one such register
> > > +	 * and see if that sticks: when the IP is configured for 32 bits
> > > +	 * only, those registers are RES0.
> > > +	 * Those MSB registers were introduced in IP v7.1, which we check first.
> > > +	 */
> > > +	if ((axienet_ior(lp, XAE_ID_OFFSET) >> 24) >= 0x9) {
> > > +		void __iomem *desc = lp->dma_regs +
> > > XAXIDMA_TX_CDESC_OFFSET + 4;
> > > +
> > > +		iowrite32(0x0, desc);
> > > +		if (ioread32(desc) == 0) {	/* sanity check */
> > > +			iowrite32(0xffffffff, desc);
> > > +			if (ioread32(desc) > 0) {
> > > +				lp->features |= XAE_FEATURE_DMA_64BIT;
> > > +				dev_info(&pdev->dev,
> > > +					 "autodetected 64-bit DMA range\n");
> > > +			}
> > > +			iowrite32(0x0, desc);
> > > +		}
> > > +	}
> > > +
> > >  	/* Check for Ethernet core IRQ (optional) */
> > >  	if (lp->eth_irq <= 0)
> > >  		dev_info(&pdev->dev, "Ethernet core IRQ not defined\n");
> > > --
> > > 2.17.1
> >


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
  2020-01-10 17:05       ` Radhey Shyam Pandey
@ 2020-01-16 18:03         ` Andre Przywara
  2020-01-20 18:32           ` Radhey Shyam Pandey
  0 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-16 18:03 UTC (permalink / raw)
  To: Radhey Shyam Pandey
  Cc: netdev, linux-kernel, Robert Hancock, Michal Simek,
	David S . Miller, linux-arm-kernel

On Fri, 10 Jan 2020 17:05:45 +0000
Radhey Shyam Pandey <radheys@xilinx.com> wrote:

Hi,

> > -----Original Message-----
> > From: Andre Przywara <andre.przywara@arm.com>
> > Sent: Friday, January 10, 2020 9:13 PM
> > To: Radhey Shyam Pandey <radheys@xilinx.com>
> > Cc: David S . Miller <davem@davemloft.net>; Michal Simek
> > <michals@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>;
> > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
> > 
> > On Fri, 10 Jan 2020 15:14:46 +0000
> > Radhey Shyam Pandey <radheys@xilinx.com> wrote:
> > 
> > Hi Radhey,
> > 
> > thanks for having a look!
> >   
> > > > -----Original Message-----
> > > > From: Andre Przywara <andre.przywara@arm.com>
> > > > Sent: Friday, January 10, 2020 5:24 PM
> > > > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> > > > <radheys@xilinx.com>
> > > > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> > > > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > > Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
> > > >
> > > > When axienet_dma_bd_init() bails out during the initialisation process,
> > > > it might do so with parts of the structure already allocated and
> > > > initialised, while other parts have not been touched yet. Before
> > > > returning in this case, we call axienet_dma_bd_release(), which does not
> > > > take care of this corner case.
> > > > This is most obvious by the first loop happily dereferencing
> > > > lp->rx_bd_v, which we actually check to be non NULL *afterwards*.
> > > >
> > > > Make sure we only unmap or free already allocated structures, by:
> > > > - directly returning with -ENOMEM if nothing has been allocated at all
> > > > - checking for lp->rx_bd_v to be non-NULL *before* using it
> > > > - only unmapping allocated DMA RX regions
> > > >
> > > > This avoids NULL pointer dereferences when initialisation fails.
> > > >
> > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > > ---
> > > >  .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++-------
> > > >  1 file changed, 28 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > > index 97482cf093ce..7e90044cf2d9 100644
> > > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > > @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct
> > > > net_device *ndev)
> > > >  	int i;
> > > >  	struct axienet_local *lp = netdev_priv(ndev);
> > > >
> > > > +	/* If we end up here, tx_bd_v must have been DMA allocated. */
> > > > +	dma_free_coherent(ndev->dev.parent,
> > > > +			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> > > > +			  lp->tx_bd_v,
> > > > +			  lp->tx_bd_p);
> > > > +
> > > > +	if (!lp->rx_bd_v)
> > > > +		return;
> > > > +
> > > >  	for (i = 0; i < lp->rx_bd_num; i++) {
> > > > -		dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
> > > > -				 lp->max_frm_size, DMA_FROM_DEVICE);
> > > > +		/* A NULL skb means this descriptor has not been initialised
> > > > +		 * at all.
> > > > +		 */
> > > > +		if (!lp->rx_bd_v[i].skb)
> > > > +			break;
> > > > +
> > > >  		dev_kfree_skb(lp->rx_bd_v[i].skb);
> > > > -	}
> > > >
> > > > -	if (lp->rx_bd_v) {
> > > > -		dma_free_coherent(ndev->dev.parent,
> > > > -				  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> > > > -				  lp->rx_bd_v,
> > > > -				  lp->rx_bd_p);
> > > > -	}
> > > > -	if (lp->tx_bd_v) {
> > > > -		dma_free_coherent(ndev->dev.parent,
> > > > -				  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> > > > -				  lp->tx_bd_v,
> > > > -				  lp->tx_bd_p);
> > > > +		/* For each descriptor, we programmed cntrl with the (non-
> > > > zero)
> > > > +		 * descriptor size, after it had been successfully allocated.
> > > > +		 * So a non-zero value in there means we need to unmap it.
> > > > +		 */  
> > >  
> > > > +		if (lp->rx_bd_v[i].cntrl)  
> > >
> > > I think it should ok to unmap w/o any check?  
> > 
> > Do you mean because .phys would be 0 if not initialised? AFAIK 0 can be a
> > valid DMA address, so there is no special check for that, and unmapping
> > DMA address 0 will probably go wrong at some point. So it's unlike
> > kfree(NULL).  
> 
> I mean if skb allocation is successful in _dma_bd_init then in release path
> we can assume .phys is always a valid address and skip rx_bd_v[i].cntrl
> check.

I don't think we can assume this. If the skb allocation succeeded, but then the dma_map_single failed (which we check with dma_mapping_error()), we would end up with a valid skb, but an uninitialised phys DMA address in the registers. That's why I set .cntrl only after having checked the dma_map_single() result.

Or am I missing something?

Cheers,
Andre
 
> > > > +			dma_unmap_single(ndev->dev.parent, lp-  
> > > > >rx_bd_v[i].phys,  
> > > > +					 lp->max_frm_size,
> > > > DMA_FROM_DEVICE);
> > > >  	}
> > > > +
> > > > +	dma_free_coherent(ndev->dev.parent,
> > > > +			  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> > > > +			  lp->rx_bd_v,
> > > > +			  lp->rx_bd_p);
> > > >  }
> > > >
> > > >  /**
> > > > @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device
> > > > *ndev)
> > > >  					 sizeof(*lp->tx_bd_v) * lp-  
> > > > >tx_bd_num,  
> > > >  					 &lp->tx_bd_p, GFP_KERNEL);
> > > >  	if (!lp->tx_bd_v)
> > > > -		goto out;
> > > > +		return -ENOMEM;
> > > >
> > > >  	lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> > > >  					 sizeof(*lp->rx_bd_v) * lp-  
> > > > >rx_bd_num,  
> > > > --
> > > > 2.17.1  
> > >  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-10 17:04           ` Russell King - ARM Linux admin
@ 2020-01-18 11:22             ` Russell King - ARM Linux admin
  2020-01-20 14:50               ` Andre Przywara
  0 siblings, 1 reply; 52+ messages in thread
From: Russell King - ARM Linux admin @ 2020-01-18 11:22 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Andrew Lunn, netdev, Radhey Shyam Pandey, Michal Simek,
	linux-kernel, Robert Hancock, David S . Miller, linux-arm-kernel

On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin wrote:
> Maybe something like the below will help?
> 
> Basically, use phylink_mii_pcs_get_state() instead of
> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
> access the internal PHY (as per C_PHYADDR parameter.)
> 
> You may have some fuzz (with gnu patch) while trying to apply this,
> as you won't have the context for the first and last hunks in this
> patch.
> 
> This will probably not be the final version of the patch anyway;
> there's some possibility to pull some of the functionality out of
> phylib into a more general library which would avoid some of the
> functional duplication.

Hi Andre,

Did you have a chance to see whether this helps?

Russell.

> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 75a74a16dc3d..44198fdb3c01 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2073,4 +2073,105 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>  }
>  EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
>  
> +static void phylink_decode_advertisement(struct phylink_link_state *state)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
> +
> +	linkmode_and(u, state->lp_advertising, state->advertising);
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
> +		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, u)) {
> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				      state->lp_advertising))
> +			state->pause |= MLO_PAUSE_TX;
> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				      state->advertising))
> +			state->pause |= MLO_PAUSE_RX;
> +	}
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
> +		state->speed = SPEED_2500;
> +		state->duplex = DUPLEX_FULL;
> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, u)) {
> +		state->pause = SPEED_1000;
> +		state->duplex = DUPLEX_FULL;
> +	} else {
> +		state->link = false;
> +	}
> +}
> +
> +void phylink_mii_pcs_get_state(struct phylink_config *config,
> +			       struct phylink_link_state *state)
> +{
> +	struct mii_bus *bus = config->pcs_mii;
> +	int addr = config->pcs_mii_addr;
> +	int bmsr, lpa;
> +
> +	bmsr = mdiobus_read(bus, addr, MII_BMSR);
> +	lpa = mdiobus_read(bus, addr, MII_LPA);
> +	if (bmsr < 0 || lpa < 0) {
> +		state->link = false;
> +		return;
> +	}
> +
> +	state->link = !!(bmsr & BMSR_LSTATUS);
> +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		if (lpa & LPA_1000XFULL)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +					 state->lp_advertising);
> +		goto lpa_8023z;
> +
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		if (lpa & LPA_1000XFULL)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
> +					 state->lp_advertising);
> +	lpa_8023z:
> +		if (lpa & LPA_1000XPAUSE)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +					 state->lp_advertising);
> +		if (lpa & LPA_1000XPAUSE_ASYM)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +					 state->lp_advertising);
> +		if (lpa & LPA_LPACK)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +					 state->lp_advertising);
> +		phylink_decode_advertisement(state);
> +		break;
> +
> +	case PHY_INTERFACE_MODE_SGMII:
> +		switch (lpa & 0x8c00) {
> +		case 0x8000:
> +			state->speed = SPEED_10;
> +			break;
> +		case 0x8400:
> +			state->speed = SPEED_100;
> +			break;
> +		case 0x8800:
> +			state->speed = SPEED_1000;
> +			break;
> +		default:
> +			state->link = false;
> +			break;
> +		}
> +		switch (lpa & 0x9000) {
> +		case 0x9000:
> +			state->duplex = DUPLEX_FULL;
> +			break;
> +		case 0x8000:
> +			state->duplex = DUPLEX_HALF;
> +			break;
> +		}
> +		break;
> +
> +	default:
> +		state->link = false;
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
> +
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 4ea76e083847..cf0fa39b4b21 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -65,6 +65,9 @@ enum phylink_op_type {
>  struct phylink_config {
>  	struct device *dev;
>  	enum phylink_op_type type;
> +
> +	struct mii_bus *pcs_mii;
> +	int pcs_mii_addr;
>  };
>  
>  /**
> @@ -292,4 +295,7 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>  						const phy_interface_t *pref,
>  						size_t nprefs);
>  
> +void phylink_mii_pcs_get_state(struct phylink_config *config,
> +			       struct phylink_link_state *state);
> +
>  #endif
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-18 11:22             ` Russell King - ARM Linux admin
@ 2020-01-20 14:50               ` Andre Przywara
  2020-01-20 15:45                 ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 52+ messages in thread
From: Andre Przywara @ 2020-01-20 14:50 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, netdev, Radhey Shyam Pandey, Michal Simek,
	linux-kernel, Robert Hancock, David S . Miller, linux-arm-kernel

On 18/01/2020 11:22, Russell King - ARM Linux admin wrote:
> On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin wrote:
>> Maybe something like the below will help?
>>
>> Basically, use phylink_mii_pcs_get_state() instead of
>> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
>> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
>> access the internal PHY (as per C_PHYADDR parameter.)
>>
>> You may have some fuzz (with gnu patch) while trying to apply this,
>> as you won't have the context for the first and last hunks in this
>> patch.
>>
>> This will probably not be the final version of the patch anyway;
>> there's some possibility to pull some of the functionality out of
>> phylib into a more general library which would avoid some of the
>> functional duplication.
> 
> Hi Andre,
> 
> Did you have a chance to see whether this helps?

Sorry, I needed some time to wrap my head around your reply first. Am I am still not fully finished with this process ;-)
Anyway I observed that when I add 'managed = "in-band-status"' to the DT, it seems to work, because it actually calls axienet_mac_pcs_get_state() to learn the actual negotiated parameters. Then in turn it calls mac_config with the proper speed instead of -1:
[  151.682532] xilinx_axienet 7fe00000.ethernet eth0: configuring for inband/sgmii link mode
[  151.710743] axienet_mac_config(config, mode=2, speed=-1, duplex=255, pause=16)
...
[  153.818568] axienet_mac_pcs_get_state(config): speed=1000, interface=4, pause=0
[  153.842244] axienet_mac_config(config, mode=2, speed=1000, duplex=1, pause=0)

Without that DT property it never called mac_pcs_get_state(), so never learnt about the actual settings.
But the actual MAC setting was already right (1 GBps, FD). Whether this was by chance (reset value?) or because this was set by the PHY via SGMII, I don't know.
So in my case I think I *need* to have the managed = ... property in my DT.

But I was wondering if we need this patch anyway, regardless of the proper way to check for the connection setting in this case. Because at the moment calling mac_config with speed=-1 will *delete* the current MAC speed setting and leave it as 10 Mbps (because this is encoded as 0), when speed is not one of the well-known values. I am not sure that is desired behaviour, or speed=-1 just means: don't touch the speed setting. After all we call mac_config with speed=-1 first, even when later fixing this up (see above).

Thanks,
Andre.

>>
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 75a74a16dc3d..44198fdb3c01 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -2073,4 +2073,105 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>>  }
>>  EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
>>  
>> +static void phylink_decode_advertisement(struct phylink_link_state *state)
>> +{
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
>> +
>> +	linkmode_and(u, state->lp_advertising, state->advertising);
>> +
>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
>> +		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
>> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, u)) {
>> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>> +				      state->lp_advertising))
>> +			state->pause |= MLO_PAUSE_TX;
>> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>> +				      state->advertising))
>> +			state->pause |= MLO_PAUSE_RX;
>> +	}
>> +
>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
>> +		state->speed = SPEED_2500;
>> +		state->duplex = DUPLEX_FULL;
>> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, u)) {
>> +		state->pause = SPEED_1000;
>> +		state->duplex = DUPLEX_FULL;
>> +	} else {
>> +		state->link = false;
>> +	}
>> +}
>> +
>> +void phylink_mii_pcs_get_state(struct phylink_config *config,
>> +			       struct phylink_link_state *state)
>> +{
>> +	struct mii_bus *bus = config->pcs_mii;
>> +	int addr = config->pcs_mii_addr;
>> +	int bmsr, lpa;
>> +
>> +	bmsr = mdiobus_read(bus, addr, MII_BMSR);
>> +	lpa = mdiobus_read(bus, addr, MII_LPA);
>> +	if (bmsr < 0 || lpa < 0) {
>> +		state->link = false;
>> +		return;
>> +	}
>> +
>> +	state->link = !!(bmsr & BMSR_LSTATUS);
>> +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
>> +
>> +	switch (state->interface) {
>> +	case PHY_INTERFACE_MODE_1000BASEX:
>> +		if (lpa & LPA_1000XFULL)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
>> +					 state->lp_advertising);
>> +		goto lpa_8023z;
>> +
>> +	case PHY_INTERFACE_MODE_2500BASEX:
>> +		if (lpa & LPA_1000XFULL)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
>> +					 state->lp_advertising);
>> +	lpa_8023z:
>> +		if (lpa & LPA_1000XPAUSE)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>> +					 state->lp_advertising);
>> +		if (lpa & LPA_1000XPAUSE_ASYM)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
>> +					 state->lp_advertising);
>> +		if (lpa & LPA_LPACK)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>> +					 state->lp_advertising);
>> +		phylink_decode_advertisement(state);
>> +		break;
>> +
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +		switch (lpa & 0x8c00) {
>> +		case 0x8000:
>> +			state->speed = SPEED_10;
>> +			break;
>> +		case 0x8400:
>> +			state->speed = SPEED_100;
>> +			break;
>> +		case 0x8800:
>> +			state->speed = SPEED_1000;
>> +			break;
>> +		default:
>> +			state->link = false;
>> +			break;
>> +		}
>> +		switch (lpa & 0x9000) {
>> +		case 0x9000:
>> +			state->duplex = DUPLEX_FULL;
>> +			break;
>> +		case 0x8000:
>> +			state->duplex = DUPLEX_HALF;
>> +			break;
>> +		}
>> +		break;
>> +
>> +	default:
>> +		state->link = false;
>> +		break;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
>> +
>>  MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
>> index 4ea76e083847..cf0fa39b4b21 100644
>> --- a/include/linux/phylink.h
>> +++ b/include/linux/phylink.h
>> @@ -65,6 +65,9 @@ enum phylink_op_type {
>>  struct phylink_config {
>>  	struct device *dev;
>>  	enum phylink_op_type type;
>> +
>> +	struct mii_bus *pcs_mii;
>> +	int pcs_mii_addr;
>>  };
>>  
>>  /**
>> @@ -292,4 +295,7 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>>  						const phy_interface_t *pref,
>>  						size_t nprefs);
>>  
>> +void phylink_mii_pcs_get_state(struct phylink_config *config,
>> +			       struct phylink_link_state *state);
>> +
>>  #endif
>>
>> -- 
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
>> According to speedtest.net: 11.9Mbps down 500kbps up
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 07/14] net: axienet: Fix SGMII support
  2020-01-20 14:50               ` Andre Przywara
@ 2020-01-20 15:45                 ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux admin @ 2020-01-20 15:45 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Andrew Lunn, netdev, Radhey Shyam Pandey, Michal Simek,
	linux-kernel, Robert Hancock, David S . Miller, linux-arm-kernel

On Mon, Jan 20, 2020 at 02:50:28PM +0000, Andre Przywara wrote:
> On 18/01/2020 11:22, Russell King - ARM Linux admin wrote:
> > On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin wrote:
> >> Maybe something like the below will help?
> >>
> >> Basically, use phylink_mii_pcs_get_state() instead of
> >> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
> >> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
> >> access the internal PHY (as per C_PHYADDR parameter.)
> >>
> >> You may have some fuzz (with gnu patch) while trying to apply this,
> >> as you won't have the context for the first and last hunks in this
> >> patch.
> >>
> >> This will probably not be the final version of the patch anyway;
> >> there's some possibility to pull some of the functionality out of
> >> phylib into a more general library which would avoid some of the
> >> functional duplication.
> > 
> > Hi Andre,
> > 
> > Did you have a chance to see whether this helps?
> 
> Sorry, I needed some time to wrap my head around your reply first. Am I am still not fully finished with this process ;-)
> Anyway I observed that when I add 'managed = "in-band-status"' to the DT, it seems to work, because it actually calls axienet_mac_pcs_get_state() to learn the actual negotiated parameters. Then in turn it calls mac_config with the proper speed instead of -1:
> [  151.682532] xilinx_axienet 7fe00000.ethernet eth0: configuring for inband/sgmii link mode
> [  151.710743] axienet_mac_config(config, mode=2, speed=-1, duplex=255, pause=16)
> ...
> [  153.818568] axienet_mac_pcs_get_state(config): speed=1000, interface=4, pause=0
> [  153.842244] axienet_mac_config(config, mode=2, speed=1000, duplex=1, pause=0)
> 
> Without that DT property it never called mac_pcs_get_state(), so never learnt about the actual settings.
> But the actual MAC setting was already right (1 GBps, FD). Whether this was by chance (reset value?) or because this was set by the PHY via SGMII, I don't know.
> So in my case I think I *need* to have the managed = ... property in my DT.

I really don't like this guess-work.  The specifications are freely
available out there, so there's really no need for this.

pg051-tri-mode-eth-mac.pdf describes the ethernet controller, and
Table 2-32 therein describes the EMMC register.

Bits 31 and 30 comprise a two-bit field which indicates the speed that
has been configured.  When the Xilinx IP has been configured for a
fixed speed, it adopts a hard-coded value (in other words, it is read-
only).  When it is read-writable, it defaults to "10" - 1G speed.

So, I think this just works by coincidence, not by proper design,
and therefore your patch in this sub-thread is incorrect since it's
masking the problem.

> But I was wondering if we need this patch anyway, regardless of the proper way to check for the connection setting in this case. Because at the moment calling mac_config with speed=-1 will *delete* the current MAC speed setting and leave it as 10 Mbps (because this is encoded as 0), when speed is not one of the well-known values. I am not sure that is desired behaviour, or speed=-1 just means: don't touch the speed setting. After all we call mac_config with speed=-1 first, even when later fixing this up (see above).

Have you tested 100M and 10M speeds as well - I suspect you'll find
that, as you're relying on the IP default EMMC register setting, it
just won't work with your patches as they stand, because there is
nothing to read the in-band result.  I also don't see anything in
either pg051-tri-mode-eth-mac.pdf or pg047-gig-eth-pcs-pma.pdf which
indicates that the PCS negotiation results are passed automatically
between either IP blocks.

Therefore, I think you _will_ need something like the patch I've
proposed to make this Xilinx IP work properly.

I've augmented the patch with further 1000BASE-X support, including
adding support for configuring the advertisement in the PG047 PCS
registers.  To allow this IP to support 1000BASE-X, from what I
read in these documents, that will also be necessary.

8<===
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] net: phylink: helpers for 802.3 clause 37/SGMII register sets

Implement helpers for PCS accessed via the MII bus using register
sets conforming to 802.3 clause 37. Advertisements for clause 37
and Cisco SGMII are supported by these helpers.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 186 ++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |   9 ++
 2 files changed, 195 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e260098d3719..ed82407240b8 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2081,4 +2081,190 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
 }
 EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
 
+static void phylink_decode_advertisement(struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
+
+	linkmode_and(u, state->lp_advertising, state->advertising);
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
+		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
+	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, u)) {
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->lp_advertising))
+			state->pause |= MLO_PAUSE_TX;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->advertising))
+			state->pause |= MLO_PAUSE_RX;
+	}
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
+		state->speed = SPEED_2500;
+		state->duplex = DUPLEX_FULL;
+	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, u)) {
+		state->pause = SPEED_1000;
+		state->duplex = DUPLEX_FULL;
+	} else {
+		state->link = false;
+	}
+}
+
+static void phylink_decode_sgmii_word(struct phylink_link_state *state,
+				      uint16_t config_reg)
+{
+	if (!(lpa & BIT(15))) {
+		state->link = false;
+		return;
+	}
+
+	switch (lpa & 0x0c00) {
+	case 0x0000:
+		state->speed = SPEED_10;
+		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
+		break;
+	case 0x0400:
+		state->speed = SPEED_100;
+		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
+		break;
+	case 0x0800:
+		state->speed = SPEED_1000;
+		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
+		break;
+	default:
+		state->link = false;
+		break;
+	}
+}
+
+/**
+ * phylink_mii_pcs_get_state - read the MAC PCS state
+ * @config: a pointer to a &struct phylink_config.
+ * @state: a pointer to a &struct phylink_link_state.
+ *
+ * Helper for MAC PCS supporting the 802.3 register set for clause 37
+ * negotiation and/or SGMII control.
+ *
+ * Read the MAC PCS state from the MII device configured in @config and
+ * parse the Clause 37 or Cisco SGMII link partner negotiation word into
+ * the phylink @state structure. This is suitable to be directly plugged
+ * into the mac_pcs_get_state() member of the struct phylink_mac_ops
+ * structure.
+ */
+void phylink_mii_pcs_get_state(struct phylink_config *config,
+			       struct phylink_link_state *state)
+{
+	struct mii_bus *bus = config->pcs_mii;
+	int addr = config->pcs_mii_addr;
+	int bmsr, lpa;
+
+	bmsr = mdiobus_read(bus, addr, MII_BMSR);
+	lpa = mdiobus_read(bus, addr, MII_LPA);
+	if (bmsr < 0 || lpa < 0) {
+		state->link = false;
+		return;
+	}
+
+	state->link = !!(bmsr & BMSR_LSTATUS);
+	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
+	if (!state->link)
+		return;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+		if (lpa & LPA_1000XFULL)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+					 state->lp_advertising);
+		goto lpa_8023z;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		if (lpa & LPA_1000XFULL)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
+					 state->lp_advertising);
+	lpa_8023z:
+		if (lpa & LPA_1000XPAUSE)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+					 state->lp_advertising);
+		if (lpa & LPA_1000XPAUSE_ASYM)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+					 state->lp_advertising);
+		if (lpa & LPA_LPACK)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					 state->lp_advertising);
+		phylink_decode_advertisement(state);
+		break;
+
+	case PHY_INTERFACE_MODE_SGMII:
+		phylink_decode_sgmii_word(state, lpa);
+		break;
+
+	default:
+		state->link = false;
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
+
+/**
+ * phylink_mii_pcs_set_advertisement - configure the clause 37 PCS advertisement
+ * @config: a pointer to a &struct phylink_config.
+ * @state: a pointer to the state being configured.
+ *
+ * Helper for MAC PCS supporting the 802.3 register set for clause 37
+ * negotiation and/or SGMII control.
+ *
+ * Configure the clause 37 PCS advertisement as specified by @state. This
+ * does not trigger a renegotiation; phylink will do that via the
+ * mac_an_restart() method of the struct phylink_mac_ops structure.
+ */
+int phylink_mii_pcs_set_advertisement(struct phylink_config *config,
+				      const struct phylink_link_state *state)
+{
+	struct mii_bus *bus = config->pcs_mii;
+	int addr = config->pcs_mii_addr;
+	u16 adv;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		adv = ADVERTISE_1000XFULL;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->advertising))
+			adv |= ADVERTISE_1000XPAUSE;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				      state->advertising))
+			adv |= ADVERTISE_1000XPSE_ASYM;
+		return mdiobus_write(bus, addr, MII_ADVERTISE, adv);
+
+	default:
+		/* Nothing to do for other modes */
+		return 0;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_pcs_set_advertisement);
+
+/**
+ * phylink_mii_pcs_an_restart - restart 802.3z autonegotiation
+ * @config: a pointer to a &struct phylink_config.
+ *
+ * Helper for MAC PCS supporting the 802.3 register set for clause 37
+ * negotiation.
+ *
+ * Restart the clause 37 negotiation with the link partner. This is
+ * suitable to be directly plugged into the mac_pcs_get_state() member
+ * of the struct phylink_mac_ops structure.
+ */
+void phylink_mii_pcs_an_restart(struct phylink_config *config)
+{
+	struct mii_bus *bus = config->pcs_mii;
+	int val, addr = config->pcs_mii_addr;
+
+	val = mdiobus_read(bus, addr, MII_BMCR);
+	if (val >= 0) {
+		val |= BMCR_ANRESTART;
+
+		mdiobus_write(bus, addr, MII_BMCR, val);
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_pcs_an_restart);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 4ea76e083847..d51f45fc5f9a 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -65,6 +65,9 @@ enum phylink_op_type {
 struct phylink_config {
 	struct device *dev;
 	enum phylink_op_type type;
+
+	struct mii_bus *pcs_mii;
+	int pcs_mii_addr;
 };
 
 /**
@@ -292,4 +295,10 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
 						const phy_interface_t *pref,
 						size_t nprefs);
 
+void phylink_mii_pcs_get_state(struct phylink_config *config,
+			       struct phylink_link_state *state);
+int phylink_mii_pcs_set_advertisement(struct phylink_config *config,
+				      const struct phylink_link_state *state);
+void phylink_mii_pcs_an_restart(struct phylink_config *config);
+
 #endif
-- 
2.20.1


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* RE: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
  2020-01-16 18:03         ` Andre Przywara
@ 2020-01-20 18:32           ` Radhey Shyam Pandey
  0 siblings, 0 replies; 52+ messages in thread
From: Radhey Shyam Pandey @ 2020-01-20 18:32 UTC (permalink / raw)
  To: Andre Przywara
  Cc: netdev, linux-kernel, Robert Hancock, Michal Simek,
	David S . Miller, linux-arm-kernel

 -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Thursday, January 16, 2020 11:33 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: David S . Miller <davem@davemloft.net>; Michal Simek
> <michals@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>;
> netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path
> 
> On Fri, 10 Jan 2020 17:05:45 +0000
> Radhey Shyam Pandey <radheys@xilinx.com> wrote:
> 
> Hi,
> 
> > > -----Original Message-----
> > > From: Andre Przywara <andre.przywara@arm.com>
> > > Sent: Friday, January 10, 2020 9:13 PM
> > > To: Radhey Shyam Pandey <radheys@xilinx.com>
> > > Cc: David S . Miller <davem@davemloft.net>; Michal Simek
> > > <michals@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>;
> > > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup
> > > path
> > >
> > > On Fri, 10 Jan 2020 15:14:46 +0000
> > > Radhey Shyam Pandey <radheys@xilinx.com> wrote:
> > >
> > > Hi Radhey,
> > >
> > > thanks for having a look!
> > >
> > > > > -----Original Message-----
> > > > > From: Andre Przywara <andre.przywara@arm.com>
> > > > > Sent: Friday, January 10, 2020 5:24 PM
> > > > > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey
> > > > > <radheys@xilinx.com>
> > > > > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock
> > > > > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm-
> > > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > > > Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup
> > > > > path
> > > > >
> > > > > When axienet_dma_bd_init() bails out during the initialisation
> > > > > process, it might do so with parts of the structure already
> > > > > allocated and initialised, while other parts have not been
> > > > > touched yet. Before returning in this case, we call
> > > > > axienet_dma_bd_release(), which does not take care of this corner case.
> > > > > This is most obvious by the first loop happily dereferencing
> > > > > lp->rx_bd_v, which we actually check to be non NULL *afterwards*.
> > > > >
> > > > > Make sure we only unmap or free already allocated structures, by:
> > > > > - directly returning with -ENOMEM if nothing has been allocated
> > > > > at all
> > > > > - checking for lp->rx_bd_v to be non-NULL *before* using it
> > > > > - only unmapping allocated DMA RX regions
> > > > >
> > > > > This avoids NULL pointer dereferences when initialisation fails.
> > > > >
> > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > > > ---
> > > > >  .../net/ethernet/xilinx/xilinx_axienet_main.c | 43
> > > > > ++++++++++++-------
> > > > >  1 file changed, 28 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > > > index 97482cf093ce..7e90044cf2d9 100644
> > > > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > > > > @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct
> > > > > net_device *ndev)
> > > > >  	int i;
> > > > >  	struct axienet_local *lp = netdev_priv(ndev);
> > > > >
> > > > > +	/* If we end up here, tx_bd_v must have been DMA allocated.
> */
> > > > > +	dma_free_coherent(ndev->dev.parent,
> > > > > +			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> > > > > +			  lp->tx_bd_v,
> > > > > +			  lp->tx_bd_p);
> > > > > +
> > > > > +	if (!lp->rx_bd_v)
> > > > > +		return;
> > > > > +
> > > > >  	for (i = 0; i < lp->rx_bd_num; i++) {
> > > > > -		dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
> > > > > -				 lp->max_frm_size, DMA_FROM_DEVICE);
> > > > > +		/* A NULL skb means this descriptor has not been
> initialised
> > > > > +		 * at all.
> > > > > +		 */
> > > > > +		if (!lp->rx_bd_v[i].skb)
> > > > > +			break;
> > > > > +
> > > > >  		dev_kfree_skb(lp->rx_bd_v[i].skb);
> > > > > -	}
> > > > >
> > > > > -	if (lp->rx_bd_v) {
> > > > > -		dma_free_coherent(ndev->dev.parent,
> > > > > -				  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> > > > > -				  lp->rx_bd_v,
> > > > > -				  lp->rx_bd_p);
> > > > > -	}
> > > > > -	if (lp->tx_bd_v) {
> > > > > -		dma_free_coherent(ndev->dev.parent,
> > > > > -				  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
> > > > > -				  lp->tx_bd_v,
> > > > > -				  lp->tx_bd_p);
> > > > > +		/* For each descriptor, we programmed cntrl with the
> (non-
> > > > > zero)
> > > > > +		 * descriptor size, after it had been successfully
> allocated.
> > > > > +		 * So a non-zero value in there means we need to
> unmap it.
> > > > > +		 */
> > > >
> > > > > +		if (lp->rx_bd_v[i].cntrl)
> > > >
> > > > I think it should ok to unmap w/o any check?
> > >
> > > Do you mean because .phys would be 0 if not initialised? AFAIK 0 can
> > > be a valid DMA address, so there is no special check for that, and
> > > unmapping DMA address 0 will probably go wrong at some point. So
> > > it's unlike kfree(NULL).
> >
> > I mean if skb allocation is successful in _dma_bd_init then in release
> > path we can assume .phys is always a valid address and skip
> > rx_bd_v[i].cntrl check.
> 
> I don't think we can assume this. If the skb allocation succeeded, but then the
> dma_map_single failed (which we check with dma_mapping_error()), we would
> end up with a valid skb, but an uninitialised phys DMA address in the registers.
> That's why I set .cntrl only after having checked the dma_map_single() result.
> 
> Or am I missing something?

Got it. Looks fine then.
> 
> Cheers,
> Andre
> 
> > > > > +			dma_unmap_single(ndev->dev.parent, lp-
> > > > > >rx_bd_v[i].phys,
> > > > > +					 lp->max_frm_size,
> > > > > DMA_FROM_DEVICE);
> > > > >  	}
> > > > > +
> > > > > +	dma_free_coherent(ndev->dev.parent,
> > > > > +			  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
> > > > > +			  lp->rx_bd_v,
> > > > > +			  lp->rx_bd_p);
> > > > >  }
> > > > >
> > > > >  /**
> > > > > @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct
> > > > > net_device
> > > > > *ndev)
> > > > >  					 sizeof(*lp->tx_bd_v) * lp-
> > > > > >tx_bd_num,
> > > > >  					 &lp->tx_bd_p, GFP_KERNEL);
> > > > >  	if (!lp->tx_bd_v)
> > > > > -		goto out;
> > > > > +		return -ENOMEM;
> > > > >
> > > > >  	lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> > > > >  					 sizeof(*lp->rx_bd_v) * lp-
> > > > > >rx_bd_num,
> > > > > --
> > > > > 2.17.1
> > > >
> >


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

* Re: [PATCH 14/14] net: axienet: Update devicetree binding documentation
  2020-01-10 11:54 ` [PATCH 14/14] net: axienet: Update devicetree binding documentation Andre Przywara
@ 2020-01-21 21:51   ` Rob Herring
  0 siblings, 0 replies; 52+ messages in thread
From: Rob Herring @ 2020-01-21 21:51 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Mark Rutland, devicetree, netdev, Radhey Shyam Pandey,
	Michal Simek, linux-kernel, Robert Hancock, David S . Miller,
	linux-arm-kernel

On Fri, Jan 10, 2020 at 11:54:15AM +0000, Andre Przywara wrote:
> This adds documentation about the newly introduced, optional
> "xlnx,addrwidth" property to the binding documentation.
> 
> While at it, clarify the wording on some properties, replace obsolete
> .txt file references with their new .yaml counterparts, and add a more
> modern example, using the axistream-connected property.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../bindings/net/xilinx_axienet.txt           | 57 ++++++++++++++++---
>  1 file changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/xilinx_axienet.txt b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> index 7360617cdedb..78c278c5200f 100644
> --- a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> +++ b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> @@ -12,7 +12,8 @@ sent and received through means of an AXI DMA controller. This driver
>  includes the DMA driver code, so this driver is incompatible with AXI DMA
>  driver.
>  
> -For more details about mdio please refer phy.txt file in the same directory.
> +For more details about mdio please refer to the ethernet-phy.yaml file in
> +the same directory.
>  
>  Required properties:
>  - compatible	: Must be one of "xlnx,axi-ethernet-1.00.a",
> @@ -27,14 +28,14 @@ Required properties:
>  		  instead, and only the Ethernet core interrupt is optionally
>  		  specified here.
>  - phy-handle	: Should point to the external phy device.
> -		  See ethernet.txt file in the same directory.
> -- xlnx,rxmem	: Set to allocated memory buffer for Rx/Tx in the hardware
> +		  See the ethernet-controller.yaml file in the same directory.
> +- xlnx,rxmem	: Size of the RXMEM buffer in the hardware, in bytes.
>  
>  Optional properties:
> -- phy-mode	: See ethernet.txt
> +- phy-mode	: See ethernet-controller.yaml.
>  - xlnx,phy-type	: Deprecated, do not use, but still accepted in preference
>  		  to phy-mode.
> -- xlnx,txcsum	: 0 or empty for disabling TX checksum offload,
> +- xlnx,txcsum	: 0 for disabling TX checksum offload (default if omitted),
>  		  1 to enable partial TX checksum offload,
>  		  2 to enable full TX checksum offload
>  - xlnx,rxcsum	: Same values as xlnx,txcsum but for RX checksum offload
> @@ -48,10 +49,20 @@ Optional properties:
>  		       If this is specified, the DMA-related resources from that
>  		       device (DMA registers and DMA TX/RX interrupts) rather
>  		       than this one will be used.
> - - mdio		: Child node for MDIO bus. Must be defined if PHY access is
> +- mdio		: Child node for MDIO bus. Must be defined if PHY access is
>  		  required through the core's MDIO interface (i.e. always,
>  		  unless the PHY is accessed through a different bus).
>  
> +Required properties for axistream-connected subnode:
> +- reg		: Address and length of the AXI DMA controller MMIO space.
> +- interrupts	: A list of 2 interrupts: TX DMA and RX DMA, in that order.
> +
> +Optional properties for axistream-connected subnode:
> +- xlnx,addrwidth: Specifies the configured address width of the DMA. Newer
> +		  versions of the IP allow setting this to a value between
> +		  32 and 64. Defaults to 32 bits if not specified.

I think this should be expressed using dma-ranges. This is exactly the 
purpose of dma-ranges and we shouldn't need a device specific property 
for this sort of thing.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 52+ messages in thread

end of thread, back to index

Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10 11:54 [PATCH 00/14] net: axienet: Error handling, SGMII and 64-bit DMA fixes Andre Przywara
2020-01-10 11:54 ` [PATCH 01/14] net: xilinx: temac: Relax Kconfig dependencies Andre Przywara
2020-01-10 14:19   ` Radhey Shyam Pandey
2020-01-10 11:54 ` [PATCH 02/14] net: axienet: Propagate failure of DMA descriptor setup Andre Przywara
2020-01-10 14:54   ` Radhey Shyam Pandey
2020-01-10 17:53     ` Radhey Shyam Pandey
2020-01-10 11:54 ` [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path Andre Przywara
2020-01-10 15:14   ` Radhey Shyam Pandey
2020-01-10 15:43     ` Andre Przywara
2020-01-10 17:05       ` Radhey Shyam Pandey
2020-01-16 18:03         ` Andre Przywara
2020-01-20 18:32           ` Radhey Shyam Pandey
2020-01-10 11:54 ` [PATCH 04/14] net: axienet: Improve DMA error handling Andre Przywara
2020-01-10 15:26   ` Radhey Shyam Pandey
2020-01-10 11:54 ` [PATCH 05/14] net: axienet: Factor out TX descriptor chain cleanup Andre Przywara
2020-01-10 18:04   ` Radhey Shyam Pandey
2020-01-10 11:54 ` [PATCH 06/14] net: axienet: Check for DMA mapping errors Andre Przywara
2020-01-13  5:54   ` Radhey Shyam Pandey
2020-01-10 11:54 ` [PATCH 07/14] net: axienet: Fix SGMII support Andre Przywara
2020-01-10 14:04   ` Andrew Lunn
2020-01-10 14:20     ` Andre Przywara
2020-01-10 14:26       ` Andrew Lunn
2020-01-10 15:04       ` Russell King - ARM Linux admin
2020-01-10 15:22         ` Russell King - ARM Linux admin
2020-01-10 17:04           ` Russell King - ARM Linux admin
2020-01-18 11:22             ` Russell King - ARM Linux admin
2020-01-20 14:50               ` Andre Przywara
2020-01-20 15:45                 ` Russell King - ARM Linux admin
2020-01-10 14:58   ` Russell King - ARM Linux admin
2020-01-10 17:32     ` Andre Przywara
2020-01-10 18:05       ` Russell King - ARM Linux admin
2020-01-10 19:33         ` Andrew Lunn
2020-01-10 11:54 ` [PATCH 08/14] net: axienet: Drop MDIO interrupt registers from ethtools dump Andre Przywara
2020-01-13  6:02   ` Radhey Shyam Pandey
2020-01-10 11:54 ` [PATCH 09/14] net: axienet: Add mii-tool support Andre Przywara
2020-01-13  6:12   ` Radhey Shyam Pandey
2020-01-10 11:54 ` [PATCH 10/14] net: axienet: Wrap DMA pointer writes to prepare for 64 bit Andre Przywara
2020-01-10 11:54 ` [PATCH 11/14] net: axienet: Upgrade descriptors to hold 64-bit addresses Andre Przywara
2020-01-14 16:35   ` Radhey Shyam Pandey
2020-01-14 17:29     ` Andre Przywara
2020-01-10 11:54 ` [PATCH 12/14] net: axienet: Autodetect 64-bit DMA capability Andre Przywara
2020-01-10 14:08   ` Andrew Lunn
2020-01-10 14:13     ` Andre Przywara
2020-01-10 14:22       ` Andrew Lunn
2020-01-10 15:08         ` Andre Przywara
2020-01-10 15:22           ` Andrew Lunn
2020-01-14 17:03   ` Radhey Shyam Pandey
2020-01-14 17:41     ` Andre Przywara
2020-01-15  6:02       ` Radhey Shyam Pandey
2020-01-10 11:54 ` [PATCH 13/14] net: axienet: Allow DMA to beyond 4GB Andre Przywara
2020-01-10 11:54 ` [PATCH 14/14] net: axienet: Update devicetree binding documentation Andre Przywara
2020-01-21 21:51   ` Rob Herring

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git