[PATCH 0/4] net: qcom/emac: various minor fixes

[PATCH 0/4] net: qcom/emac: various minor fixes

[Timur Tabi]

A set of patches for 4.15 that clean up some code, apply minors fixes,
and so on.  Some of the code also prepares the driver for a future 
version of the EMAC controller.

Timur Tabi (4):
  net: qcom/emac: specify the correct DMA mask
  net: qcom/emac: remove unused address arrays
  net: qcom/emac: enforce DMA address restrictions
  net: qcom/emac: clean up some TX/RX error messages

 drivers/net/ethernet/qualcomm/emac/emac-mac.c   | 55 ++++++++++++-------------
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 15 +++----
 drivers/net/ethernet/qualcomm/emac/emac.c       | 25 ++++-------
 3 files changed, 41 insertions(+), 54 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/4] net: qcom/emac: specify the correct DMA mask

[Timur Tabi]

The 64/32-bit DMA mask hackery in the EMAC driver is not actually necessary,
and is technically not accurate.  The EMAC hardware is limted to a 45-bit
DMA address.  Although no EMAC-enabled system can have that much DDR,
an IOMMU could possible provide a larger address.  Rather than play games
with the DMA mappings, the driver should provide a correct value and
trust the DMA/IOMMU layers to do the right thing.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index f477ba29c569..ee6f2d27502c 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -615,20 +615,11 @@ static int emac_probe(struct platform_device *pdev)
 	u32 reg;
 	int ret;
 
-	/* The EMAC itself is capable of 64-bit DMA, so try that first. */
-	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	/* The TPD buffer address is limited to 45 bits. */
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(45));
 	if (ret) {
-		/* Some platforms may restrict the EMAC's address bus to less
-		 * then the size of DDR. In this case, we need to try a
-		 * smaller mask.  We could try every possible smaller mask,
-		 * but that's overkill.  Instead, just fall to 32-bit, which
-		 * should always work.
-		 */
-		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
-		if (ret) {
-			dev_err(&pdev->dev, "could not set DMA mask\n");
-			return ret;
-		}
+		dev_err(&pdev->dev, "could not set DMA mask\n");
+		return ret;
 	}
 
 	netdev = alloc_etherdev(sizeof(struct emac_adapter));
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/4] net: qcom/emac: remove unused address arrays

[Timur Tabi]

The EMAC is capable of multiple TX and RX rings, but the driver only
supports one ring for each.  One function had some left-over unused
code that supports multiple rings, but all it did was make the code
harder to read.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 3ed9033e56db..9cbb27263742 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -309,22 +309,12 @@ void emac_mac_mode_config(struct emac_adapter *adpt)
 /* Config descriptor rings */
 static void emac_mac_dma_rings_config(struct emac_adapter *adpt)
 {
-	static const unsigned short tpd_q_offset[] = {
-		EMAC_DESC_CTRL_8,        EMAC_H1TPD_BASE_ADDR_LO,
-		EMAC_H2TPD_BASE_ADDR_LO, EMAC_H3TPD_BASE_ADDR_LO};
-	static const unsigned short rfd_q_offset[] = {
-		EMAC_DESC_CTRL_2,        EMAC_DESC_CTRL_10,
-		EMAC_DESC_CTRL_12,       EMAC_DESC_CTRL_13};
-	static const unsigned short rrd_q_offset[] = {
-		EMAC_DESC_CTRL_5,        EMAC_DESC_CTRL_14,
-		EMAC_DESC_CTRL_15,       EMAC_DESC_CTRL_16};
-
 	/* TPD (Transmit Packet Descriptor) */
 	writel(upper_32_bits(adpt->tx_q.tpd.dma_addr),
 	       adpt->base + EMAC_DESC_CTRL_1);
 
 	writel(lower_32_bits(adpt->tx_q.tpd.dma_addr),
-	       adpt->base + tpd_q_offset[0]);
+	       adpt->base + EMAC_DESC_CTRL_8);
 
 	writel(adpt->tx_q.tpd.count & TPD_RING_SIZE_BMSK,
 	       adpt->base + EMAC_DESC_CTRL_9);
@@ -334,9 +324,9 @@ static void emac_mac_dma_rings_config(struct emac_adapter *adpt)
 	       adpt->base + EMAC_DESC_CTRL_0);
 
 	writel(lower_32_bits(adpt->rx_q.rfd.dma_addr),
-	       adpt->base + rfd_q_offset[0]);
+	       adpt->base + EMAC_DESC_CTRL_2);
 	writel(lower_32_bits(adpt->rx_q.rrd.dma_addr),
-	       adpt->base + rrd_q_offset[0]);
+	       adpt->base + EMAC_DESC_CTRL_5);
 
 	writel(adpt->rx_q.rfd.count & RFD_RING_SIZE_BMSK,
 	       adpt->base + EMAC_DESC_CTRL_3);
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[Timur Tabi]

The EMAC has a restriction that the upper 32 bits of the base addresses
for the RFD and RRD rings must be the same.  The ensure that restriction,
we allocate twice the space for the RRD and locate it at an appropriate
address.

We also re-arrange the allocations so that invalid addresses are even
less likely.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 39 ++++++++++++++++-----------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 9cbb27263742..0f5ece5d9507 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -734,6 +734,11 @@ static int emac_rx_descs_alloc(struct emac_adapter *adpt)
 	rx_q->rrd.size = rx_q->rrd.count * (adpt->rrd_size * 4);
 	rx_q->rfd.size = rx_q->rfd.count * (adpt->rfd_size * 4);
 
+	/* Check if the RRD and RFD are aligned properly, and if not, adjust. */
+	if (upper_32_bits(ring_header->dma_addr) !=
+	    upper_32_bits(ring_header->dma_addr + ALIGN(rx_q->rrd.size, 8)))
+		ring_header->used = ALIGN(rx_q->rrd.size, 8);
+
 	rx_q->rrd.dma_addr = ring_header->dma_addr + ring_header->used;
 	rx_q->rrd.v_addr   = ring_header->v_addr + ring_header->used;
 	ring_header->used += ALIGN(rx_q->rrd.size, 8);
@@ -767,11 +772,18 @@ int emac_mac_rx_tx_rings_alloc_all(struct emac_adapter *adpt)
 
 	/* Ring DMA buffer. Each ring may need up to 8 bytes for alignment,
 	 * hence the additional padding bytes are allocated.
+	 *
+	 * Also double the memory allocated for the RRD so that we can
+	 * re-align it if necessary.  The EMAC has a restriction that the
+	 * upper 32 bits of the base addresses for the RFD and RRD rings
+	 * must be the same.  It is extremely unlikely that this is not the
+	 * case, since the rings are only a few KB in size.  However, we
+	 * need to check for this anyway, and if the two rings are not
+	 * compliant, then we re-align.
 	 */
-	ring_header->size = num_tx_descs * (adpt->tpd_size * 4) +
-			    num_rx_descs * (adpt->rfd_size * 4) +
-			    num_rx_descs * (adpt->rrd_size * 4) +
-			    8 + 2 * 8; /* 8 byte per one Tx and two Rx rings */
+	ring_header->size = ALIGN(num_tx_descs * (adpt->tpd_size * 4), 8) +
+			    ALIGN(num_rx_descs * (adpt->rfd_size * 4), 8) +
+			    ALIGN(num_rx_descs * (adpt->rrd_size * 4), 8) * 2;
 
 	ring_header->used = 0;
 	ring_header->v_addr = dma_zalloc_coherent(dev, ring_header->size,
@@ -780,26 +792,23 @@ int emac_mac_rx_tx_rings_alloc_all(struct emac_adapter *adpt)
 	if (!ring_header->v_addr)
 		return -ENOMEM;
 
-	ring_header->used = ALIGN(ring_header->dma_addr, 8) -
-							ring_header->dma_addr;
-
-	ret = emac_tx_q_desc_alloc(adpt, &adpt->tx_q);
-	if (ret) {
-		netdev_err(adpt->netdev, "error: Tx Queue alloc failed\n");
-		goto err_alloc_tx;
-	}
-
 	ret = emac_rx_descs_alloc(adpt);
 	if (ret) {
 		netdev_err(adpt->netdev, "error: Rx Queue alloc failed\n");
 		goto err_alloc_rx;
 	}
 
+	ret = emac_tx_q_desc_alloc(adpt, &adpt->tx_q);
+	if (ret) {
+		netdev_err(adpt->netdev, "transmit queue allocation failed\n");
+		goto err_alloc_tx;
+	}
+
 	return 0;
 
-err_alloc_rx:
-	emac_tx_q_bufs_free(adpt);
 err_alloc_tx:
+	emac_rx_q_bufs_free(adpt);
+err_alloc_rx:
 	dma_free_coherent(dev, ring_header->size,
 			  ring_header->v_addr, ring_header->dma_addr);
 
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

RE: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[David Laight]

From: Timur Tabi
> Sent: 11 October 2017 20:52
> The EMAC has a restriction that the upper 32 bits of the base addresses
> for the RFD and RRD rings must be the same.  The ensure that restriction,
> we allocate twice the space for the RRD and locate it at an appropriate
> address.
> 
> We also re-arrange the allocations so that invalid addresses are even
> less likely.
> 
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  drivers/net/ethernet/qualcomm/emac/emac-mac.c | 39 ++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-
> mac.c
> index 9cbb27263742..0f5ece5d9507 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
> +++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
> @@ -734,6 +734,11 @@ static int emac_rx_descs_alloc(struct emac_adapter *adpt)
>  	rx_q->rrd.size = rx_q->rrd.count * (adpt->rrd_size * 4);
>  	rx_q->rfd.size = rx_q->rfd.count * (adpt->rfd_size * 4);
> 
> +	/* Check if the RRD and RFD are aligned properly, and if not, adjust. */
> +	if (upper_32_bits(ring_header->dma_addr) !=
> +	    upper_32_bits(ring_header->dma_addr + ALIGN(rx_q->rrd.size, 8)))
> +		ring_header->used = ALIGN(rx_q->rrd.size, 8);
> +

Isn't the memory allocated by a single kzalloc() call?
IIRC that guarantees it doesn't cross a power or 2 boundary less than
the size.
So if you allocate any size between 4k and 8k it won't cross an odd
4k boundary (etc).

So these checks are entirely pointless.

	David

Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[Timur Tabi]

On 10/12/17 4:30 AM, David Laight wrote:
> Isn't the memory allocated by a single kzalloc() call?

dma_alloc_coherenent, actually.

> IIRC that guarantees it doesn't cross a power or 2 boundary less than
> the size.

I'm pretty sure that kzalloc does not make that guarantee, and I don't 
think dma_alloc_coherent does either.

> So if you allocate any size between 4k and 8k it won't cross an odd
> 4k boundary (etc).
> 
> So these checks are entirely pointless.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

RE: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[David Laight]

From: Timur Tabi
> Sent: 12 October 2017 15:13
> On 10/12/17 4:30 AM, David Laight wrote:
> > Isn't the memory allocated by a single kzalloc() call?
> 
> dma_alloc_coherenent, actually.
> 
> > IIRC that guarantees it doesn't cross a power or 2 boundary less than
> > the size.
> 
> I'm pretty sure that kzalloc does not make that guarantee, and I don't
> think dma_alloc_coherent does either.

dma_alloc_coherent() definitely does.
And I've a driver that relies on it (for 16k blocks).

	David

Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[Timur Tabi]

On 10/12/2017 11:20 AM, David Laight wrote:
>> I'm pretty sure that kzalloc does not make that guarantee, and I don't
>> think dma_alloc_coherent does either.

> dma_alloc_coherent() definitely does.
> And I've a driver that relies on it (for 16k blocks).

What about when an IOMMU is used?  The DMA address that gets returned is 
not necessarily the physical address of the memory buffer.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[David Miller]

From: Timur Tabi <timur@codeaurora.org>
Date: Thu, 12 Oct 2017 09:13:25 -0500

> On 10/12/17 4:30 AM, David Laight wrote:
>> Isn't the memory allocated by a single kzalloc() call?
> 
> dma_alloc_coherenent, actually.
> 
>> IIRC that guarantees it doesn't cross a power or 2 boundary less than
>> the size.
> 
> I'm pretty sure that kzalloc does not make that guarantee, and I don't
> think dma_alloc_coherent does either.

Both make that guarantee, even when an IOMMU is used.

Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[Timur Tabi]

On 10/12/2017 11:58 AM, David Miller wrote:
>> I'm pretty sure that kzalloc does not make that guarantee, and I don't
>> think dma_alloc_coherent does either.
> Both make that guarantee, even when an IOMMU is used.
> 

Ok, Dave, then can you drop this patch?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[David Miller]

From: Timur Tabi <timur@codeaurora.org>
Date: Thu, 12 Oct 2017 12:15:12 -0500

> On 10/12/2017 11:58 AM, David Miller wrote:
>>> I'm pretty sure that kzalloc does not make that guarantee, and I don't
>>> think dma_alloc_coherent does either.
>> Both make that guarantee, even when an IOMMU is used.
>> 
> 
> Ok, Dave, then can you drop this patch?

Please send me a patch.

[PATCH 4/4] net: qcom/emac: clean up some TX/RX error messages

[Timur Tabi]

Some of the error messages that are printed by the interrupt handlers
are poorly written.  For example, many don't include a device prefix,
so there's no indication that they are EMAC errors.

Also use rate limiting for all messages that could be printed from
interrupt context.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 15 ++++++---------
 drivers/net/ethernet/qualcomm/emac/emac.c       |  8 ++++----
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index 29ba37a08372..e8ab512ee7e3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -68,10 +68,10 @@ static void emac_sgmii_link_init(struct emac_adapter *adpt)
 	writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
 }
 
-static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u32 irq_bits)
+static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u8 irq_bits)
 {
 	struct emac_sgmii *phy = &adpt->phy;
-	u32 status;
+	u8 status;
 
 	writel_relaxed(irq_bits, phy->base + EMAC_SGMII_PHY_INTERRUPT_CLEAR);
 	writel_relaxed(IRQ_GLOBAL_CLEAR, phy->base + EMAC_SGMII_PHY_IRQ_CMD);
@@ -86,9 +86,8 @@ static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u32 irq_bits)
 				      EMAC_SGMII_PHY_INTERRUPT_STATUS,
 				      status, !(status & irq_bits), 1,
 				      SGMII_PHY_IRQ_CLR_WAIT_TIME)) {
-		netdev_err(adpt->netdev,
-			   "error: failed clear SGMII irq: status:0x%x bits:0x%x\n",
-			   status, irq_bits);
+		net_err_ratelimited("%s: failed to clear SGMII irq: status:0x%x bits:0x%x\n",
+				    adpt->netdev->name, status, irq_bits);
 		return -EIO;
 	}
 
@@ -109,7 +108,7 @@ static irqreturn_t emac_sgmii_interrupt(int irq, void *data)
 {
 	struct emac_adapter *adpt = data;
 	struct emac_sgmii *phy = &adpt->phy;
-	u32 status;
+	u8 status;
 
 	status = readl(phy->base + EMAC_SGMII_PHY_INTERRUPT_STATUS);
 	status &= SGMII_ISR_MASK;
@@ -139,10 +138,8 @@ static irqreturn_t emac_sgmii_interrupt(int irq, void *data)
 		atomic_set(&phy->decode_error_count, 0);
 	}
 
-	if (emac_sgmii_irq_clear(adpt, status)) {
-		netdev_warn(adpt->netdev, "failed to clear SGMII interrupt\n");
+	if (emac_sgmii_irq_clear(adpt, status))
 		schedule_work(&adpt->work_thread);
-	}
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index ee6f2d27502c..70c92b649b29 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -148,9 +148,8 @@ static irqreturn_t emac_isr(int _irq, void *data)
 		goto exit;
 
 	if (status & ISR_ERROR) {
-		netif_warn(adpt,  intr, adpt->netdev,
-			   "warning: error irq status 0x%lx\n",
-			   status & ISR_ERROR);
+		net_err_ratelimited("%s: error interrupt 0x%lx\n",
+				    adpt->netdev->name, status & ISR_ERROR);
 		/* reset MAC */
 		schedule_work(&adpt->work_thread);
 	}
@@ -169,7 +168,8 @@ static irqreturn_t emac_isr(int _irq, void *data)
 		emac_mac_tx_process(adpt, &adpt->tx_q);
 
 	if (status & ISR_OVER)
-		net_warn_ratelimited("warning: TX/RX overflow\n");
+		net_warn_ratelimited("%s: TX/RX overflow interrupt\n",
+				     adpt->netdev->name);
 
 exit:
 	/* enable the interrupt */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 0/4] net: qcom/emac: various minor fixes

[David Miller]

From: Timur Tabi <timur@codeaurora.org>
Date: Wed, 11 Oct 2017 14:52:22 -0500

> A set of patches for 4.15 that clean up some code, apply minors fixes,
> and so on.  Some of the code also prepares the driver for a future 
> version of the EMAC controller.

Series applied, thank you.