net: qcom/emac: various minor fixes
Subsystems (3)
cover net: qcom/emac: various minor fixes
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(-)
Signature · 3 lines
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
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.
--- drivers/net/ethernet/qualcomm/emac/emac.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
drivers/net/ethernet/qualcomm/emac/emac.c +4 -13
@@ -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 ));
Signature · 3 lines
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
In reply to Timur TabiThe 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.
--- drivers/net/ethernet/qualcomm/emac/emac-mac.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
drivers/net/ethernet/qualcomm/emac/emac-mac.c +3 -13
@@ -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 );
Signature · 3 lines
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
In reply to Timur TabiThe 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.
--- drivers/net/ethernet/qualcomm/emac/emac-mac.c | 39 ++++++++++++++++----------- 1 file changed, 24 insertions(+), 15 deletions(-)
drivers/net/ethernet/qualcomm/emac/emac-mac.c +24 -15
@@ -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);
Signature · 3 lines
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
From: Timur Tabi
Quoted 20 lines
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 <[email protected]>--- drivers/net/ethernet/qualcomm/emac/emac-mac.c | 39 ++++++++++++++++----------- 1 file changed, 24 insertions(+), 15 deletions(-)drivers/net/ethernet/qualcomm/emac/emac- renamed from drivers/net/ethernet/qualcomm/emac/emac-mac.c +0
-0
mac.c
index9cbb27263742..0f5ece5d9507100644
drivers/net/ethernet/qualcomm/emac/emac-mac.c static int emac_rx_descs_alloc(struct emac_adapter *adpt) · line 741
Quoted 8 lines
drivers/net/ethernet/qualcomm/emac/emac-mac.c +5
-0@@ -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
Quoted 1 line · David Laight
Isn't the memory allocated by a single
kzalloc()call?
dma_alloc_coherenent, actually.
Quoted 2 lines
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.
Quoted 4 lines
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.
Signature · 3 lines
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
From: Timur Tabi
Quoted 11 lines
Sent: 12 October 2017 15:13
Quoted 1 line · David Laight
Isn't the memory allocated by a single
kzalloc()call?
dma_alloc_coherenent, actually.Quoted 2 lines
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_coherentdoes 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
Quoted 5 lines · David Laight
Quoted 2 lines
I'm pretty sure that kzalloc does not make that guarantee, and I don't think
dma_alloc_coherentdoes 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.
Signature · 3 lines
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.
David Miller
maintainerRe: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions
In reply to Timur TabiFrom: Timur Tabi <[email protected]>
Date: Thu, 12 Oct 2017 09:13:25 -0500
Quoted 10 lines · David Laight
Quoted 1 line
Isn't the memory allocated by a single
kzalloc()call?
dma_alloc_coherenent, actually.Quoted 2 lines
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_coherentdoes either.
Both make that guarantee, even when an IOMMU is used.
Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions
Quoted 4 lines · David Miller
Quoted 2 lines
I'm pretty sure that kzalloc does not make that guarantee, and I don't think
dma_alloc_coherentdoes either.Both make that guarantee, even when an IOMMU is used.
Ok, Dave, then can you drop this patch?
Signature · 3 lines
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.
David Miller
maintainerRe: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions
From: Timur Tabi <[email protected]>
Date: Thu, 12 Oct 2017 12:15:12 -0500
Quoted 7 lines · David Miller
Quoted 4 lines
Quoted 2 lines
I'm pretty sure that kzalloc does not make that guarantee, and I don't think
dma_alloc_coherentdoes 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
In reply to Timur TabiSome 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.
--- drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 15 ++++++--------- drivers/net/ethernet/qualcomm/emac/emac.c | 8 ++++---- 2 files changed, 10 insertions(+), 13 deletions(-)
drivers/net/ethernet/qualcomm/emac/emac-sgmii.c +6 -9
@@ -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 ; }
drivers/net/ethernet/qualcomm/emac/emac.c +4 -4
@@ -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 */
Signature · 3 lines
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.
David Miller
maintainerFrom: Timur Tabi <[email protected]>
Date: Wed, 11 Oct 2017 14:52:22 -0500
Quoted 3 lines
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.