Hi,
This series enable the use of mvneta driver on the Armada 3700
SoCs. Armada 3700 is a new ARMv8 SoC from Marvell using same network
controller as older Armada 370/38x/XP.
Besides the changes needed to be used on 64-bits architecture done in
the 1st patch, there are also few difference related to the Armada
3700 SoC. The main one being the used of shared interrupt instead of
the private ones. It has been addressed in the 3rd patch.
Not all the feature supported on the older Soc have been ported yet
for this new SoC.
Gregory CLEMENT (2):
net: mvneta: Only disable mvneta_bm for 64-bits
ARM64: dts: marvell: Add network support for Armada 3700
Marcin Wojtas (2):
net: mvneta: Convert to be 64 bits compatible
net: mvneta: Add network support for Armada 3700 SoC
.../bindings/net/marvell-armada-370-neta.txt | 7 +-
arch/arm64/boot/dts/marvell/armada-3720-db.dts | 23 ++
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 23 ++
drivers/net/ethernet/marvell/Kconfig | 10 +-
drivers/net/ethernet/marvell/mvneta.c | 364 ++++++++++++++++-----
5 files changed, 333 insertions(+), 94 deletions(-)
--
2.10.2
From: Marcin Wojtas <[email protected]>
Prepare the mvneta driver in order to be usable on the 64 bits platform
such as the Armada 3700.
[[email protected]]: this patch was extract from a larger
one to ease review and maintenance.
Signed-off-by: Marcin Wojtas <[email protected]>
Signed-off-by: Gregory CLEMENT <[email protected]>
---
drivers/net/ethernet/marvell/mvneta.c | 77 ++++++++++++++++++++++++++++++++---
1 file changed, 71 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 87274d4ab102..67f6465d96ba 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -296,6 +296,12 @@
/* descriptor aligned size */
#define MVNETA_DESC_ALIGNED_SIZE 32
+/* Number of bytes to be taken into account by HW when putting incoming data
+ * to the buffers. It is needed in case NET_SKB_PAD exceeds maximum packet
+ * offset supported in MVNETA_RXQ_CONFIG_REG(q) registers.
+ */
+#define MVNETA_RX_PKT_OFFSET_CORRECTION 64
+
#define MVNETA_RX_PKT_SIZE(mtu) \
ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
ETH_HLEN + ETH_FCS_LEN, \
@@ -416,8 +422,11 @@ struct mvneta_port {
u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
+#ifdef CONFIG_64BIT
+ u64 data_high;
+#endif
+ u16 rx_offset_correction;
};
-
/* The mvneta_tx_desc and mvneta_rx_desc structures describe the
* layout of the transmit and reception DMA descriptors, and their
* layout is therefore defined by the hardware design
@@ -1791,6 +1800,10 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
if (!data)
return -ENOMEM;
+#ifdef CONFIG_64BIT
+ if (unlikely(pp->data_high != (u64)upper_32_bits((u64)data) << 32))
+ return -ENOMEM;
+#endif
phys_addr = dma_map_single(pp->dev->dev.parent, data,
MVNETA_RX_BUF_SIZE(pp->pkt_size),
DMA_FROM_DEVICE);
@@ -1799,7 +1812,8 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
return -ENOMEM;
}
- mvneta_rx_desc_fill(rx_desc, phys_addr, (u32)data);
+ phys_addr += pp->rx_offset_correction;
+ mvneta_rx_desc_fill(rx_desc, phys_addr, (uintptr_t)data);
return 0;
}
@@ -1861,8 +1875,16 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
for (i = 0; i < rxq->size; i++) {
struct mvneta_rx_desc *rx_desc = rxq->descs + i;
- void *data = (void *)rx_desc->buf_cookie;
-
+ void *data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
+#ifdef CONFIG_64BIT
+ /* In Neta HW only 32 bits data is supported, so in
+ * order to obtain whole 64 bits address from RX
+ * descriptor, we store the upper 32 bits when
+ * allocating buffer, and put it back when using
+ * buffer cookie for accessing packet in memory.
+ */
+ data = (u8 *)(pp->data_high | (u64)data);
+#endif
dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
MVNETA_RX_BUF_SIZE(pp->pkt_size), DMA_FROM_DEVICE);
mvneta_frag_free(pp->frag_size, data);
@@ -1899,7 +1921,17 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int rx_todo,
rx_done++;
rx_status = rx_desc->status;
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
+#ifdef CONFIG_64BIT
+ /* In Neta HW only 32 bits data is supported, so in
+ * order to obtain whole 64 bits address from RX
+ * descriptor, we store the upper 32 bits when
+ * allocating buffer, and put it back when using
+ * buffer cookie for accessing packet in memory.
+ */
+ data = (u8 *)(pp->data_high | (u64)rx_desc->buf_cookie);
+#else
data = (unsigned char *)rx_desc->buf_cookie;
+#endif
phys_addr = rx_desc->buf_phys_addr;
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
@@ -2020,7 +2052,17 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int rx_todo,
rx_done++;
rx_status = rx_desc->status;
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
- data = (unsigned char *)rx_desc->buf_cookie;
+#ifdef CONFIG_64BIT
+ /* In Neta HW only 32 bits data is supported, so in
+ * order to obtain whole 64 bits address from RX
+ * descriptor, we store the upper 32 bits when
+ * allocating buffer, and put it back when using
+ * buffer cookie for accessing packet in memory.
+ */
+ data = (u8 *)(pp->data_high | (u64)rx_desc->buf_cookie);
+#else
+ data = (u8 *)rx_desc->buf_cookie;
+#endif
phys_addr = rx_desc->buf_phys_addr;
pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
bm_pool = &pp->bm_priv->bm_pools[pool_id];
@@ -2773,7 +2815,7 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
/* Set Offset */
- mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD);
+ mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD - pp->rx_offset_correction);
/* Set coalescing pkts and time */
mvneta_rx_pkts_coal_set(pp, rxq, rxq->pkts_coal);
@@ -2930,6 +2972,22 @@ static void mvneta_cleanup_rxqs(struct mvneta_port *pp)
static int mvneta_setup_rxqs(struct mvneta_port *pp)
{
int queue;
+#ifdef CONFIG_64BIT
+ void *data_tmp;
+
+ /* In Neta HW only 32 bits data is supported, so in order to
+ * obtain whole 64 bits address from RX descriptor, we store
+ * the upper 32 bits when allocating buffer, and put it back
+ * when using buffer cookie for accessing packet in memory.
+ * Frags should be allocated from single 'memory' region,
+ * hence common upper address half should be sufficient.
+ */
+ data_tmp = mvneta_frag_alloc(pp->frag_size);
+ if (data_tmp) {
+ pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
+ mvneta_frag_free(pp->frag_size, data_tmp);
+ }
+#endif
for (queue = 0; queue < rxq_number; queue++) {
int err = mvneta_rxq_init(pp, &pp->rxqs[queue]);
@@ -4019,6 +4077,13 @@ static int mvneta_probe(struct platform_device *pdev)
pp->rxq_def = rxq_def;
+ /* Set RX packet offset correction for platforms, whose
+ * NET_SKB_PAD, exceeds 64B. It should be 64B for 64-bit
+ * platforms and 0B for 32-bit ones.
+ */
+ pp->rx_offset_correction =
+ max(0, NET_SKB_PAD - MVNETA_RX_PKT_OFFSET_CORRECTION);
+
pp->indir[0] = rxq_def;
pp->clk = devm_clk_get(&pdev->dev, "core");
--
2.10.2
From: Marcin Wojtas <[email protected]>
Armada 3700 is a new ARMv8 SoC from Marvell using same network controller
as older Armada 370/38x/XP. There are however some differences that
needed taking into account when adding support for it:
* open default MBUS window to 4GB of DRAM - Armada 3700 SoC's Mbus
configuration for network controller has to be done on two levels:
global and per-port. The first one is inherited from the
bootloader. The latter can be opened in a default way, leaving
arbitration to the bus controller. Hence filled mbus_dram_target_info
structure is not needed
* make per-CPU operation optional - Recent patches adding RSS and XPS
support for Armada 38x/XP enabled per-CPU operation of the controller
by default. Contrary to older SoC's Armada 3700 SoC's network
controller is not capable of per-CPU processing due to interrupt lines'
connectivity. This patch restores non-per-CPU operation, which is now
optional and depends on neta_armada3700 flag value in mvneta_port
structure. In order not to complicate the code, separate interrupt
subroutine is implemented.
For now, on the Armada 3700, RSS is disabled as the current
implementation depend on precpu interrupt.
[[email protected]: extract from a larger patch, replace
some ifdef and port to net-next for v4.10]
Signed-off-by: Marcin Wojtas <[email protected]>
Signed-off-by: Gregory CLEMENT <[email protected]>
---
.../bindings/net/marvell-armada-370-neta.txt | 7 +-
drivers/net/ethernet/marvell/Kconfig | 7 +-
drivers/net/ethernet/marvell/mvneta.c | 287 +++++++++++++++------
3 files changed, 214 insertions(+), 87 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
index 73be8970815e..7aa840c8768d 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -1,7 +1,10 @@
-* Marvell Armada 370 / Armada XP Ethernet Controller (NETA)
+* Marvell Armada 370 / Armada XP / Armada 3700 Ethernet Controller (NETA)
Required properties:
-- compatible: "marvell,armada-370-neta" or "marvell,armada-xp-neta".
+- compatible: could be one of the followings
+ "marvell,armada-370-neta"
+ "marvell,armada-xp-neta"
+ "marvell,armada-3700-neta"
- reg: address and length of the register set for the device.
- interrupts: interrupt for the device
- phy: See ethernet.txt file in the same directory.
diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index 2ccea9dd9248..3b8f11fe5e13 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -56,14 +56,15 @@ config MVNETA_BM_ENABLE
buffer management.
config MVNETA
- tristate "Marvell Armada 370/38x/XP network interface support"
- depends on PLAT_ORION || COMPILE_TEST
+ tristate "Marvell Armada 370/38x/XP/37xx network interface support"
+ depends on ARCH_MVEBU || COMPILE_TEST
depends on HAS_DMA
select MVMDIO
select FIXED_PHY
---help---
This driver supports the network interface units in the
- Marvell ARMADA XP, ARMADA 370 and ARMADA 38x SoC family.
+ Marvell ARMADA XP, ARMADA 370, ARMADA 38x and
+ ARMADA 37xx SoC family.
Note that this driver is distinct from the mv643xx_eth
driver, which should be used for the older Marvell SoCs
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 67f6465d96ba..7438ffd5639a 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -397,6 +397,9 @@ struct mvneta_port {
spinlock_t lock;
bool is_stopped;
+ u32 cause_rx_tx;
+ struct napi_struct napi;
+
/* Core clock */
struct clk *clk;
/* AXI clock */
@@ -422,6 +425,9 @@ struct mvneta_port {
u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
+
+ /* Flags for special SoC configurations */
+ bool neta_armada3700;
#ifdef CONFIG_64BIT
u64 data_high;
#endif
@@ -964,14 +970,9 @@ static int mvneta_mbus_io_win_set(struct mvneta_port *pp, u32 base, u32 wsize,
return 0;
}
-/* Assign and initialize pools for port. In case of fail
- * buffer manager will remain disabled for current port.
- */
-static int mvneta_bm_port_init(struct platform_device *pdev,
- struct mvneta_port *pp)
+static int mvneta_bm_port_mbus_init(struct mvneta_port *pp)
{
- struct device_node *dn = pdev->dev.of_node;
- u32 long_pool_id, short_pool_id, wsize;
+ u32 wsize;
u8 target, attr;
int err;
@@ -990,6 +991,25 @@ static int mvneta_bm_port_init(struct platform_device *pdev,
netdev_info(pp->dev, "fail to configure mbus window to BM\n");
return err;
}
+ return 0;
+}
+
+/* Assign and initialize pools for port. In case of fail
+ * buffer manager will remain disabled for current port.
+ */
+static int mvneta_bm_port_init(struct platform_device *pdev,
+ struct mvneta_port *pp)
+{
+ struct device_node *dn = pdev->dev.of_node;
+ u32 long_pool_id, short_pool_id;
+
+ if (!pp->neta_armada3700) {
+ int ret;
+
+ ret = mvneta_bm_port_mbus_init(pp);
+ if (ret)
+ return ret;
+ }
if (of_property_read_u32(dn, "bm,pool-long", &long_pool_id)) {
netdev_info(pp->dev, "missing long pool id\n");
@@ -1358,22 +1378,27 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
for_each_present_cpu(cpu) {
int rxq_map = 0, txq_map = 0;
int rxq, txq;
+ if (!pp->neta_armada3700) {
+ for (rxq = 0; rxq < rxq_number; rxq++)
+ if ((rxq % max_cpu) == cpu)
+ rxq_map |= MVNETA_CPU_RXQ_ACCESS(rxq);
+
+ for (txq = 0; txq < txq_number; txq++)
+ if ((txq % max_cpu) == cpu)
+ txq_map |= MVNETA_CPU_TXQ_ACCESS(txq);
+
+ /* With only one TX queue we configure a special case
+ * which will allow to get all the irq on a single
+ * CPU
+ */
+ if (txq_number == 1)
+ txq_map = (cpu == pp->rxq_def) ?
+ MVNETA_CPU_TXQ_ACCESS(1) : 0;
- for (rxq = 0; rxq < rxq_number; rxq++)
- if ((rxq % max_cpu) == cpu)
- rxq_map |= MVNETA_CPU_RXQ_ACCESS(rxq);
-
- for (txq = 0; txq < txq_number; txq++)
- if ((txq % max_cpu) == cpu)
- txq_map |= MVNETA_CPU_TXQ_ACCESS(txq);
-
- /* With only one TX queue we configure a special case
- * which will allow to get all the irq on a single
- * CPU
- */
- if (txq_number == 1)
- txq_map = (cpu == pp->rxq_def) ?
- MVNETA_CPU_TXQ_ACCESS(1) : 0;
+ } else {
+ txq_map = MVNETA_CPU_TXQ_ACCESS_ALL_MASK;
+ rxq_map = MVNETA_CPU_RXQ_ACCESS_ALL_MASK;
+ }
mvreg_write(pp, MVNETA_CPU_MAP(cpu), rxq_map | txq_map);
}
@@ -2652,6 +2677,17 @@ static void mvneta_set_rx_mode(struct net_device *dev)
/* Interrupt handling - the callback for request_irq() */
static irqreturn_t mvneta_isr(int irq, void *dev_id)
{
+ struct mvneta_port *pp = (struct mvneta_port *)dev_id;
+
+ mvreg_write(pp, MVNETA_INTR_NEW_MASK, 0);
+ napi_schedule(&pp->napi);
+
+ return IRQ_HANDLED;
+}
+
+/* Interrupt handling - the callback for request_percpu_irq() */
+static irqreturn_t mvneta_percpu_isr(int irq, void *dev_id)
+{
struct mvneta_pcpu_port *port = (struct mvneta_pcpu_port *)dev_id;
disable_percpu_irq(port->pp->dev->irq);
@@ -2699,7 +2735,7 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
struct mvneta_pcpu_port *port = this_cpu_ptr(pp->ports);
if (!netif_running(pp->dev)) {
- napi_complete(&port->napi);
+ napi_complete(napi);
return rx_done;
}
@@ -2728,7 +2764,8 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
*/
rx_queue = fls(((cause_rx_tx >> 8) & 0xff));
- cause_rx_tx |= port->cause_rx_tx;
+ cause_rx_tx |= pp->neta_armada3700 ? pp->cause_rx_tx :
+ port->cause_rx_tx;
if (rx_queue) {
rx_queue = rx_queue - 1;
@@ -2742,11 +2779,27 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
if (budget > 0) {
cause_rx_tx = 0;
- napi_complete(&port->napi);
- enable_percpu_irq(pp->dev->irq, 0);
+ napi_complete(napi);
+
+ if (pp->neta_armada3700) {
+ unsigned long flags;
+
+ local_irq_save(flags);
+ mvreg_write(pp, MVNETA_INTR_NEW_MASK,
+ MVNETA_RX_INTR_MASK(rxq_number) |
+ MVNETA_TX_INTR_MASK(txq_number) |
+ MVNETA_MISCINTR_INTR_MASK);
+ local_irq_restore(flags);
+ } else {
+ enable_percpu_irq(pp->dev->irq, 0);
+ }
}
- port->cause_rx_tx = cause_rx_tx;
+ if (pp->neta_armada3700)
+ pp->cause_rx_tx = cause_rx_tx;
+ else
+ port->cause_rx_tx = cause_rx_tx;
+
return rx_done;
}
@@ -3032,11 +3085,16 @@ static void mvneta_start_dev(struct mvneta_port *pp)
/* start the Rx/Tx activity */
mvneta_port_enable(pp);
- /* Enable polling on the port */
- for_each_online_cpu(cpu) {
- struct mvneta_pcpu_port *port = per_cpu_ptr(pp->ports, cpu);
+ if (!pp->neta_armada3700) {
+ /* Enable polling on the port */
+ for_each_online_cpu(cpu) {
+ struct mvneta_pcpu_port *port =
+ per_cpu_ptr(pp->ports, cpu);
- napi_enable(&port->napi);
+ napi_enable(&port->napi);
+ }
+ } else {
+ napi_enable(&pp->napi);
}
/* Unmask interrupts. It has to be done from each CPU */
@@ -3058,10 +3116,15 @@ static void mvneta_stop_dev(struct mvneta_port *pp)
phy_stop(ndev->phydev);
- for_each_online_cpu(cpu) {
- struct mvneta_pcpu_port *port = per_cpu_ptr(pp->ports, cpu);
+ if (!pp->neta_armada3700) {
+ for_each_online_cpu(cpu) {
+ struct mvneta_pcpu_port *port =
+ per_cpu_ptr(pp->ports, cpu);
- napi_disable(&port->napi);
+ napi_disable(&port->napi);
+ }
+ } else {
+ napi_disable(&pp->napi);
}
netif_carrier_off(pp->dev);
@@ -3471,31 +3534,37 @@ static int mvneta_open(struct net_device *dev)
goto err_cleanup_rxqs;
/* Connect to port interrupt line */
- ret = request_percpu_irq(pp->dev->irq, mvneta_isr,
- MVNETA_DRIVER_NAME, pp->ports);
+ if (pp->neta_armada3700)
+ ret = request_irq(pp->dev->irq, mvneta_isr, 0,
+ dev->name, pp);
+ else
+ ret = request_percpu_irq(pp->dev->irq, mvneta_percpu_isr,
+ dev->name, pp->ports);
if (ret) {
netdev_err(pp->dev, "cannot request irq %d\n", pp->dev->irq);
goto err_cleanup_txqs;
}
- /* Enable per-CPU interrupt on all the CPU to handle our RX
- * queue interrupts
- */
- on_each_cpu(mvneta_percpu_enable, pp, true);
+ if (!pp->neta_armada3700) {
+ /* Enable per-CPU interrupt on all the CPU to handle our RX
+ * queue interrupts
+ */
+ on_each_cpu(mvneta_percpu_enable, pp, true);
- pp->is_stopped = false;
- /* Register a CPU notifier to handle the case where our CPU
- * might be taken offline.
- */
- ret = cpuhp_state_add_instance_nocalls(online_hpstate,
- &pp->node_online);
- if (ret)
- goto err_free_irq;
+ pp->is_stopped = false;
+ /* Register a CPU notifier to handle the case where our CPU
+ * might be taken offline.
+ */
+ ret = cpuhp_state_add_instance_nocalls(online_hpstate,
+ &pp->node_online);
+ if (ret)
+ goto err_free_irq;
- ret = cpuhp_state_add_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
- &pp->node_dead);
- if (ret)
- goto err_free_online_hp;
+ ret = cpuhp_state_add_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
+ &pp->node_dead);
+ if (ret)
+ goto err_free_online_hp;
+ }
/* In default link is down */
netif_carrier_off(pp->dev);
@@ -3511,13 +3580,20 @@ static int mvneta_open(struct net_device *dev)
return 0;
err_free_dead_hp:
- cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
- &pp->node_dead);
+ if (!pp->neta_armada3700)
+ cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
+ &pp->node_dead);
err_free_online_hp:
- cpuhp_state_remove_instance_nocalls(online_hpstate, &pp->node_online);
+ if (!pp->neta_armada3700)
+ cpuhp_state_remove_instance_nocalls(online_hpstate,
+ &pp->node_online);
err_free_irq:
- on_each_cpu(mvneta_percpu_disable, pp, true);
- free_percpu_irq(pp->dev->irq, pp->ports);
+ if (pp->neta_armada3700) {
+ free_irq(pp->dev->irq, pp);
+ } else {
+ on_each_cpu(mvneta_percpu_disable, pp, true);
+ free_percpu_irq(pp->dev->irq, pp->ports);
+ }
err_cleanup_txqs:
mvneta_cleanup_txqs(pp);
err_cleanup_rxqs:
@@ -3530,23 +3606,30 @@ static int mvneta_stop(struct net_device *dev)
{
struct mvneta_port *pp = netdev_priv(dev);
- /* Inform that we are stopping so we don't want to setup the
- * driver for new CPUs in the notifiers. The code of the
- * notifier for CPU online is protected by the same spinlock,
- * so when we get the lock, the notifer work is done.
- */
- spin_lock(&pp->lock);
- pp->is_stopped = true;
- spin_unlock(&pp->lock);
+ if (!pp->neta_armada3700) {
+ /* Inform that we are stopping so we don't want to setup the
+ * driver for new CPUs in the notifiers. The code of the
+ * notifier for CPU online is protected by the same spinlock,
+ * so when we get the lock, the notifer work is done.
+ */
+ spin_lock(&pp->lock);
+ pp->is_stopped = true;
+ spin_unlock(&pp->lock);
- mvneta_stop_dev(pp);
- mvneta_mdio_remove(pp);
+ mvneta_stop_dev(pp);
+ mvneta_mdio_remove(pp);
cpuhp_state_remove_instance_nocalls(online_hpstate, &pp->node_online);
cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
&pp->node_dead);
- on_each_cpu(mvneta_percpu_disable, pp, true);
- free_percpu_irq(dev->irq, pp->ports);
+ on_each_cpu(mvneta_percpu_disable, pp, true);
+ free_percpu_irq(dev->irq, pp->ports);
+ } else {
+ mvneta_stop_dev(pp);
+ mvneta_mdio_remove(pp);
+ free_irq(dev->irq, pp);
+ }
+
mvneta_cleanup_rxqs(pp);
mvneta_cleanup_txqs(pp);
@@ -3825,6 +3908,11 @@ static int mvneta_ethtool_set_rxfh(struct net_device *dev, const u32 *indir,
const u8 *key, const u8 hfunc)
{
struct mvneta_port *pp = netdev_priv(dev);
+
+ /* Current code for Armada 3700 doesn't support RSS features yet */
+ if (pp->neta_armada3700)
+ return -EOPNOTSUPP;
+
/* We require at least one supported parameter to be changed
* and no change in any of the unsupported parameters
*/
@@ -3845,6 +3933,10 @@ static int mvneta_ethtool_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
{
struct mvneta_port *pp = netdev_priv(dev);
+ /* Current code for Armada 3700 doesn't support RSS features yet */
+ if (pp->neta_armada3700)
+ return -EOPNOTSUPP;
+
if (hfunc)
*hfunc = ETH_RSS_HASH_TOP;
@@ -3947,16 +4039,29 @@ static void mvneta_conf_mbus_windows(struct mvneta_port *pp,
win_enable = 0x3f;
win_protect = 0;
- for (i = 0; i < dram->num_cs; i++) {
- const struct mbus_dram_window *cs = dram->cs + i;
- mvreg_write(pp, MVNETA_WIN_BASE(i), (cs->base & 0xffff0000) |
- (cs->mbus_attr << 8) | dram->mbus_dram_target_id);
+ if (dram) {
+ for (i = 0; i < dram->num_cs; i++) {
+ const struct mbus_dram_window *cs = dram->cs + i;
+
+ mvreg_write(pp, MVNETA_WIN_BASE(i),
+ (cs->base & 0xffff0000) |
+ (cs->mbus_attr << 8) |
+ dram->mbus_dram_target_id);
- mvreg_write(pp, MVNETA_WIN_SIZE(i),
- (cs->size - 1) & 0xffff0000);
+ mvreg_write(pp, MVNETA_WIN_SIZE(i),
+ (cs->size - 1) & 0xffff0000);
- win_enable &= ~(1 << i);
- win_protect |= 3 << (2 * i);
+ win_enable &= ~(1 << i);
+ win_protect |= 3 << (2 * i);
+ }
+ } else {
+ /* For Armada3700 open default 4GB Mbus window, leaving
+ * arbitration of target/attribute to a different layer
+ * of configuration.
+ */
+ mvreg_write(pp, MVNETA_WIN_SIZE(0), 0xffff0000);
+ win_enable &= ~BIT(0);
+ win_protect = 3;
}
mvreg_write(pp, MVNETA_BASE_ADDR_ENABLE, win_enable);
@@ -4086,6 +4191,10 @@ static int mvneta_probe(struct platform_device *pdev)
pp->indir[0] = rxq_def;
+ /* Get special SoC configurations */
+ if (of_device_is_compatible(dn, "marvell,armada-3700-neta"))
+ pp->neta_armada3700 = true;
+
pp->clk = devm_clk_get(&pdev->dev, "core");
if (IS_ERR(pp->clk))
pp->clk = devm_clk_get(&pdev->dev, NULL);
@@ -4153,7 +4262,11 @@ static int mvneta_probe(struct platform_device *pdev)
pp->tx_csum_limit = tx_csum_limit;
dram_target_info = mv_mbus_dram_info();
- if (dram_target_info)
+ /* Armada3700 requires setting default configuration of Mbus
+ * windows, however without using filled mbus_dram_target_info
+ * structure.
+ */
+ if (dram_target_info || pp->neta_armada3700)
mvneta_conf_mbus_windows(pp, dram_target_info);
pp->tx_ring_size = MVNETA_MAX_TXD;
@@ -4186,11 +4299,20 @@ static int mvneta_probe(struct platform_device *pdev)
goto err_netdev;
}
- for_each_present_cpu(cpu) {
- struct mvneta_pcpu_port *port = per_cpu_ptr(pp->ports, cpu);
+ /* Armada3700 network controller does not support per-cpu
+ * operation, so only single NAPI should be initialized.
+ */
+ if (pp->neta_armada3700) {
+ netif_napi_add(dev, &pp->napi, mvneta_poll, NAPI_POLL_WEIGHT);
+ } else {
+ for_each_present_cpu(cpu) {
+ struct mvneta_pcpu_port *port =
+ per_cpu_ptr(pp->ports, cpu);
- netif_napi_add(dev, &port->napi, mvneta_poll, NAPI_POLL_WEIGHT);
- port->pp = pp;
+ netif_napi_add(dev, &port->napi, mvneta_poll,
+ NAPI_POLL_WEIGHT);
+ port->pp = pp;
+ }
}
dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
@@ -4275,6 +4397,7 @@ static int mvneta_remove(struct platform_device *pdev)
static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
+ { .compatible = "marvell,armada-3700-neta" },
{ }
};
MODULE_DEVICE_TABLE(of, mvneta_match);
--
2.10.2
Add neta nodes for network support both in device tree for the SoC and
the board.
Signed-off-by: Gregory CLEMENT <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-3720-db.dts | 23 +++++++++++++++++++++++
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 23 +++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-db.dts b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
index 1372e9a6aaa4..c8b82e4145de 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
@@ -81,3 +81,26 @@
&pcie0 {
status = "okay";
};
+
+&mdio {
+ status = "okay";
+ phy0: ethernet-phy@0 {
+ reg = <0>;
+ };
+
+ phy1: ethernet-phy@1 {
+ reg = <1>;
+ };
+};
+
+ð0 {
+ phy-mode = "rgmii-id";
+ phy = <&phy0>;
+ status = "okay";
+};
+
+ð1 {
+ phy-mode = "rgmii-id";
+ phy = <&phy1>;
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index c4762538ec01..a7278ce9e523 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -140,6 +140,29 @@
};
};
+ eth0: ethernet@30000 {
+ compatible = "marvell,armada-3700-neta";
+ reg = <0x30000 0x4000>;
+ interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&sb_periph_clk 8>;
+ status = "disabled";
+ };
+
+ mdio: mdio@32004 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "marvell,orion-mdio";
+ reg = <0x32004 0x4>;
+ };
+
+ eth1: ethernet@40000 {
+ compatible = "marvell,armada-3700-neta";
+ reg = <0x40000 0x4000>;
+ interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&sb_periph_clk 7>;
+ status = "disabled";
+ };
+
usb3: usb@58000 {
compatible = "marvell,armada3700-xhci",
"generic-xhci";
--
2.10.2
Actually only the mvneta_bm support is not 64-bits compatible.
The mvneta code itself can run on 64-bits architecture.
Signed-off-by: Gregory CLEMENT <[email protected]>
---
drivers/net/ethernet/marvell/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index 66fd9dbb2ca7..2ccea9dd9248 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -44,6 +44,7 @@ config MVMDIO
config MVNETA_BM_ENABLE
tristate "Marvell Armada 38x/XP network interface BM support"
depends on MVNETA
+ depends on !64BIT
---help---
This driver supports auxiliary block of the network
interface units in the Marvell ARMADA XP and ARMADA 38x SoC
@@ -58,7 +59,6 @@ config MVNETA
tristate "Marvell Armada 370/38x/XP network interface support"
depends on PLAT_ORION || COMPILE_TEST
depends on HAS_DMA
- depends on !64BIT
select MVMDIO
select FIXED_PHY
---help---
@@ -71,6 +71,7 @@ config MVNETA
config MVNETA_BM
tristate
+ depends on !64BIT
default y if MVNETA=y && MVNETA_BM_ENABLE!=n
default MVNETA_BM_ENABLE
select HWBM
--
2.10.2
On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
> +#ifdef CONFIG_64BIT
> + void *data_tmp;
> +
> + /* In Neta HW only 32 bits data is supported, so in order to
> + * obtain whole 64 bits address from RX descriptor, we store
> + * the upper 32 bits when allocating buffer, and put it back
> + * when using buffer cookie for accessing packet in memory.
> + * Frags should be allocated from single 'memory' region,
> + * hence common upper address half should be sufficient.
> + */
> + data_tmp = mvneta_frag_alloc(pp->frag_size);
> + if (data_tmp) {
> + pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
> + mvneta_frag_free(pp->frag_size, data_tmp);
> + }
>
How does this work when the region spans a n*4GB address boundary?
Arnd
On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
> On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
> > +#ifdef CONFIG_64BIT
> > + void *data_tmp;
> > +
> > + /* In Neta HW only 32 bits data is supported, so in order to
> > + * obtain whole 64 bits address from RX descriptor, we store
> > + * the upper 32 bits when allocating buffer, and put it back
> > + * when using buffer cookie for accessing packet in memory.
> > + * Frags should be allocated from single 'memory' region,
> > + * hence common upper address half should be sufficient.
> > + */
> > + data_tmp = mvneta_frag_alloc(pp->frag_size);
> > + if (data_tmp) {
> > + pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
> > + mvneta_frag_free(pp->frag_size, data_tmp);
> > + }
> >
>
> How does this work when the region spans a n*4GB address boundary?
indeed. We also make use of this driver on 64bit platforms. We use
different solution to make the driver 64bit safe.
solA: make use of the reserved field in the mvneta_rx_desc, such
as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
now it's not used at all. This is one possible solution however.
solB: allocate a shadow buf cookie during init, e.g
rxq->descs_bufcookie = kmalloc(rxq->size * sizeof(void*), GFP_KERNEL);
then modify mvneta_rx_desc_fill a bit to save the 64bit pointer in
the shadow buf cookie, e.g
static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
u32 phys_addr, u32 cookie,
struct mvneta_rx_queue *rxq)
{
int i;
rx_desc->buf_cookie = cookie;
rx_desc->buf_phys_addr = phys_addr;
i = rx_desc - rxq->descs;
rxq->descs_bufcookie[i] = cookie;
}
then fetch the desc from the shadow buf cookie in all code path, such
as mvneta_rx() etc.
Both solutions should not have the problems pointed out by Arnd.
Thanks,
Jisheng
On Wednesday, November 23, 2016 5:53:41 PM CET Jisheng Zhang wrote:
> On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
>
> > On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
> > > +#ifdef CONFIG_64BIT
> > > + void *data_tmp;
> > > +
> > > + /* In Neta HW only 32 bits data is supported, so in order to
> > > + * obtain whole 64 bits address from RX descriptor, we store
> > > + * the upper 32 bits when allocating buffer, and put it back
> > > + * when using buffer cookie for accessing packet in memory.
> > > + * Frags should be allocated from single 'memory' region,
> > > + * hence common upper address half should be sufficient.
> > > + */
> > > + data_tmp = mvneta_frag_alloc(pp->frag_size);
> > > + if (data_tmp) {
> > > + pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
> > > + mvneta_frag_free(pp->frag_size, data_tmp);
> > > + }
> > >
> >
> > How does this work when the region spans a n*4GB address boundary?
>
> indeed. We also make use of this driver on 64bit platforms. We use
> different solution to make the driver 64bit safe.
>
> solA: make use of the reserved field in the mvneta_rx_desc, such
> as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
> now it's not used at all. This is one possible solution however.
Right, this sounds like the most straightforward choice.
> solB: allocate a shadow buf cookie during init, e.g
>
> rxq->descs_bufcookie = kmalloc(rxq->size * sizeof(void*), GFP_KERNEL);
>
> then modify mvneta_rx_desc_fill a bit to save the 64bit pointer in
> the shadow buf cookie, e.g
> static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
> u32 phys_addr, u32 cookie,
> struct mvneta_rx_queue *rxq)
>
> {
> int i;
>
> rx_desc->buf_cookie = cookie;
> rx_desc->buf_phys_addr = phys_addr;
> i = rx_desc - rxq->descs;
> rxq->descs_bufcookie[i] = cookie;
> }
>
> then fetch the desc from the shadow buf cookie in all code path, such
> as mvneta_rx() etc.
>
> Both solutions should not have the problems pointed out by Arnd.
Wait, since you compute an index 'i' here, can't you just store 'i'
directly in the descriptor instead of the pointer?
Arnd
Hi Arnd,
On Wed, 23 Nov 2016 11:15:32 +0100 Arnd Bergmann wrote:
> On Wednesday, November 23, 2016 5:53:41 PM CET Jisheng Zhang wrote:
> > On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
> >
> > > On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
> > > > +#ifdef CONFIG_64BIT
> > > > + void *data_tmp;
> > > > +
> > > > + /* In Neta HW only 32 bits data is supported, so in order to
> > > > + * obtain whole 64 bits address from RX descriptor, we store
> > > > + * the upper 32 bits when allocating buffer, and put it back
> > > > + * when using buffer cookie for accessing packet in memory.
> > > > + * Frags should be allocated from single 'memory' region,
> > > > + * hence common upper address half should be sufficient.
> > > > + */
> > > > + data_tmp = mvneta_frag_alloc(pp->frag_size);
> > > > + if (data_tmp) {
> > > > + pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
> > > > + mvneta_frag_free(pp->frag_size, data_tmp);
> > > > + }
> > > >
> > >
> > > How does this work when the region spans a n*4GB address boundary?
> >
> > indeed. We also make use of this driver on 64bit platforms. We use
> > different solution to make the driver 64bit safe.
> >
> > solA: make use of the reserved field in the mvneta_rx_desc, such
> > as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
> > now it's not used at all. This is one possible solution however.
>
> Right, this sounds like the most straightforward choice.
>
> > solB: allocate a shadow buf cookie during init, e.g
> >
> > rxq->descs_bufcookie = kmalloc(rxq->size * sizeof(void*), GFP_KERNEL);
> >
> > then modify mvneta_rx_desc_fill a bit to save the 64bit pointer in
> > the shadow buf cookie, e.g
> > static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
> > u32 phys_addr, u32 cookie,
sorry, this line should be:
u32 phys_addr, void *cookie
> > struct mvneta_rx_queue *rxq)
> >
> > {
> > int i;
> >
> > rx_desc->buf_cookie = cookie;
> > rx_desc->buf_phys_addr = phys_addr;
> > i = rx_desc - rxq->descs;
> > rxq->descs_bufcookie[i] = cookie;
> > }
> >
> > then fetch the desc from the shadow buf cookie in all code path, such
> > as mvneta_rx() etc.
> >
> > Both solutions should not have the problems pointed out by Arnd.
>
> Wait, since you compute an index 'i' here, can't you just store 'i'
> directly in the descriptor instead of the pointer?
>
we need to store the pointer, it's to store the buffer allocated by
mvneta_frag_alloc()
Thanks,
Jisheng
Hi Jisheng, Arnd,
Thanks for your feedback.
On mer., nov. 23 2016, Arnd Bergmann <[email protected]> wrote:
> On Wednesday, November 23, 2016 5:53:41 PM CET Jisheng Zhang wrote:
>> On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
>>
>> > On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
>> > > +#ifdef CONFIG_64BIT
>> > > + void *data_tmp;
>> > > +
>> > > + /* In Neta HW only 32 bits data is supported, so in order to
>> > > + * obtain whole 64 bits address from RX descriptor, we store
>> > > + * the upper 32 bits when allocating buffer, and put it back
>> > > + * when using buffer cookie for accessing packet in memory.
>> > > + * Frags should be allocated from single 'memory' region,
>> > > + * hence common upper address half should be sufficient.
>> > > + */
>> > > + data_tmp = mvneta_frag_alloc(pp->frag_size);
>> > > + if (data_tmp) {
>> > > + pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
>> > > + mvneta_frag_free(pp->frag_size, data_tmp);
>> > > + }
>> > >
>> >
>> > How does this work when the region spans a n*4GB address boundary?
>>
>> indeed. We also make use of this driver on 64bit platforms. We use
>> different solution to make the driver 64bit safe.
>>
>> solA: make use of the reserved field in the mvneta_rx_desc, such
>> as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
>> now it's not used at all. This is one possible solution however.
>
> Right, this sounds like the most straightforward choice.
The PnC (which stands for Parsing and Classification) is not used yet
indeed but this field will be needed when we will enable it. It is
something we want to do but it is not planned in a near future. However
from the datasheets I have it seems only present on the Armada XP. It is
not mentioned on datasheets for the Armada 38x or the Armada 3700.
That would mean it was safe to use on of this field in 64-bits mode on
the Armada 3700.
So I am going to take this approach.
Thanks,
Gregory
>
>> solB: allocate a shadow buf cookie during init, e.g
>>
>> rxq->descs_bufcookie = kmalloc(rxq->size * sizeof(void*), GFP_KERNEL);
>>
>> then modify mvneta_rx_desc_fill a bit to save the 64bit pointer in
>> the shadow buf cookie, e.g
>> static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
>> u32 phys_addr, u32 cookie,
>> struct mvneta_rx_queue *rxq)
>>
>> {
>> int i;
>>
>> rx_desc->buf_cookie = cookie;
>> rx_desc->buf_phys_addr = phys_addr;
>> i = rx_desc - rxq->descs;
>> rxq->descs_bufcookie[i] = cookie;
>> }
>>
>> then fetch the desc from the shadow buf cookie in all code path, such
>> as mvneta_rx() etc.
>>
>> Both solutions should not have the problems pointed out by Arnd.
>
> Wait, since you compute an index 'i' here, can't you just store 'i'
> directly in the descriptor instead of the pointer?
>
> Arnd
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Hi Gregory,
2016-11-23 14:07 GMT+01:00 Gregory CLEMENT <[email protected]>:
> Hi Jisheng, Arnd,
>
>
> Thanks for your feedback.
>
>
> On mer., nov. 23 2016, Arnd Bergmann <[email protected]> wrote:
>
>> On Wednesday, November 23, 2016 5:53:41 PM CET Jisheng Zhang wrote:
>>> On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
>>>
>>> > On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
>>> > > +#ifdef CONFIG_64BIT
>>> > > + void *data_tmp;
>>> > > +
>>> > > + /* In Neta HW only 32 bits data is supported, so in order to
>>> > > + * obtain whole 64 bits address from RX descriptor, we store
>>> > > + * the upper 32 bits when allocating buffer, and put it back
>>> > > + * when using buffer cookie for accessing packet in memory.
>>> > > + * Frags should be allocated from single 'memory' region,
>>> > > + * hence common upper address half should be sufficient.
>>> > > + */
>>> > > + data_tmp = mvneta_frag_alloc(pp->frag_size);
>>> > > + if (data_tmp) {
>>> > > + pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
>>> > > + mvneta_frag_free(pp->frag_size, data_tmp);
>>> > > + }
>>> > >
>>> >
>>> > How does this work when the region spans a n*4GB address boundary?
>>>
>>> indeed. We also make use of this driver on 64bit platforms. We use
>>> different solution to make the driver 64bit safe.
>>>
>>> solA: make use of the reserved field in the mvneta_rx_desc, such
>>> as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
>>> now it's not used at all. This is one possible solution however.
>>
>> Right, this sounds like the most straightforward choice.
>
> The PnC (which stands for Parsing and Classification) is not used yet
> indeed but this field will be needed when we will enable it. It is
> something we want to do but it is not planned in a near future. However
> from the datasheets I have it seems only present on the Armada XP. It is
> not mentioned on datasheets for the Armada 38x or the Armada 3700.
>
It is not mentioned in A38x spec, but this SoC has exactly the same
PnC as Armada XP (they differ only with used SRAM details). I wouldn't
be surprised if it was supported on A3700 as well.
> That would mean it was safe to use on of this field in 64-bits mode on
> the Armada 3700.
>
> So I am going to take this approach.
>
I think for now it's safe and is much easier than handling extra
software ring for virtual addresses.
Best regards,
Marcin
Hi Marcin, Gregory, Arnd,
On Wed, 23 Nov 2016 17:02:11 +0100 Marcin Wojtas wrote:
> Hi Gregory,
>
> 2016-11-23 14:07 GMT+01:00 Gregory CLEMENT:
> > Hi Jisheng, Arnd,
> >
> >
> > Thanks for your feedback.
> >
> >
> > On mer., nov. 23 2016, Arnd Bergmann wrote:
> >
> >> On Wednesday, November 23, 2016 5:53:41 PM CET Jisheng Zhang wrote:
> >>> On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
> >>>
> >>> > On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
> >>> > > +#ifdef CONFIG_64BIT
> >>> > > + void *data_tmp;
> >>> > > +
> >>> > > + /* In Neta HW only 32 bits data is supported, so in order to
> >>> > > + * obtain whole 64 bits address from RX descriptor, we store
> >>> > > + * the upper 32 bits when allocating buffer, and put it back
> >>> > > + * when using buffer cookie for accessing packet in memory.
> >>> > > + * Frags should be allocated from single 'memory' region,
> >>> > > + * hence common upper address half should be sufficient.
> >>> > > + */
> >>> > > + data_tmp = mvneta_frag_alloc(pp->frag_size);
> >>> > > + if (data_tmp) {
> >>> > > + pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
> >>> > > + mvneta_frag_free(pp->frag_size, data_tmp);
> >>> > > + }
> >>> > >
> >>> >
> >>> > How does this work when the region spans a n*4GB address boundary?
> >>>
> >>> indeed. We also make use of this driver on 64bit platforms. We use
> >>> different solution to make the driver 64bit safe.
> >>>
> >>> solA: make use of the reserved field in the mvneta_rx_desc, such
> >>> as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
> >>> now it's not used at all. This is one possible solution however.
> >>
> >> Right, this sounds like the most straightforward choice.
> >
> > The PnC (which stands for Parsing and Classification) is not used yet
> > indeed but this field will be needed when we will enable it. It is
> > something we want to do but it is not planned in a near future. However
> > from the datasheets I have it seems only present on the Armada XP. It is
> > not mentioned on datasheets for the Armada 38x or the Armada 3700.
> >
>
> It is not mentioned in A38x spec, but this SoC has exactly the same
> PnC as Armada XP (they differ only with used SRAM details). I wouldn't
> be surprised if it was supported on A3700 as well.
>
> > That would mean it was safe to use on of this field in 64-bits mode on
> > the Armada 3700.
> >
> > So I am going to take this approach.
> >
>
> I think for now it's safe and is much easier than handling extra
> software ring for virtual addresses.
>
solB (a SW shadow cookie) perhaps gives a better performance: in hot path,
such as mvneta_rx(), the driver accesses buf_cookie and buf_phys_addr of
rx_desc which is allocated by dma_alloc_coherent, it's noncacheable if the
device isn't cache-coherent. I didn't measure the performance difference,
because in fact we take solA as well internally. From your experience,
can the performance gain deserve the complex code?
Thanks,
Jisheng
On Thursday, November 24, 2016 4:37:36 PM CET Jisheng Zhang wrote:
> solB (a SW shadow cookie) perhaps gives a better performance: in hot path,
> such as mvneta_rx(), the driver accesses buf_cookie and buf_phys_addr of
> rx_desc which is allocated by dma_alloc_coherent, it's noncacheable if the
> device isn't cache-coherent. I didn't measure the performance difference,
> because in fact we take solA as well internally. From your experience,
> can the performance gain deserve the complex code?
Yes, a read from uncached memory is fairly slow, so if you have a chance
to avoid that it will probably help. When adding complexity to the code,
it probably makes sense to take a runtime profile anyway quantify how
much it gains.
On machines that have cache-coherent DMA, accessing the descriptor
should be fine, as you already have to load the entire cache line
to read the status field.
Looking at this snippet:
rx_status = rx_desc->status;
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
data = (unsigned char *)rx_desc->buf_cookie;
phys_addr = rx_desc->buf_phys_addr;
pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
bm_pool = &pp->bm_priv->bm_pools[pool_id];
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
(rx_status & MVNETA_RXD_ERR_SUMMARY)) {
err_drop_frame_ret_pool:
/* Return the buffer to the pool */
mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
rx_desc->buf_phys_addr);
err_drop_frame:
I think there is more room for optimizing if you start: you read
the status field twice (the second one in MVNETA_RX_GET_BM_POOL_ID)
and you can cache the buf_phys_addr along with the virtual address
once you add that.
Generally speaking, I'd recommend using READ_ONCE()/WRITE_ONCE()
to access the descriptor fields, to ensure the compiler doesn't
add extra references as well as to annotate the expensive
operations.
Arnd
On Thu, 24 Nov 2016 10:00:36 +0100
Arnd Bergmann <[email protected]> wrote:
> On Thursday, November 24, 2016 4:37:36 PM CET Jisheng Zhang wrote:
> > solB (a SW shadow cookie) perhaps gives a better performance: in hot path,
> > such as mvneta_rx(), the driver accesses buf_cookie and buf_phys_addr of
> > rx_desc which is allocated by dma_alloc_coherent, it's noncacheable if the
> > device isn't cache-coherent. I didn't measure the performance difference,
> > because in fact we take solA as well internally. From your experience,
> > can the performance gain deserve the complex code?
>
> Yes, a read from uncached memory is fairly slow, so if you have a chance
> to avoid that it will probably help. When adding complexity to the code,
> it probably makes sense to take a runtime profile anyway quantify how
> much it gains.
>
> On machines that have cache-coherent DMA, accessing the descriptor
> should be fine, as you already have to load the entire cache line
> to read the status field.
>
> Looking at this snippet:
>
> rx_status = rx_desc->status;
> rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
> data = (unsigned char *)rx_desc->buf_cookie;
> phys_addr = rx_desc->buf_phys_addr;
> pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
> bm_pool = &pp->bm_priv->bm_pools[pool_id];
>
> if (!mvneta_rxq_desc_is_first_last(rx_status) ||
> (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
> err_drop_frame_ret_pool:
> /* Return the buffer to the pool */
> mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
> rx_desc->buf_phys_addr);
> err_drop_frame:
>
>
> I think there is more room for optimizing if you start: you read
> the status field twice (the second one in MVNETA_RX_GET_BM_POOL_ID)
> and you can cache the buf_phys_addr along with the virtual address
> once you add that.
oh, yeah! buf_phy_addr could be included too.
>
> Generally speaking, I'd recommend using READ_ONCE()/WRITE_ONCE()
> to access the descriptor fields, to ensure the compiler doesn't
> add extra references as well as to annotate the expensive
> operations.
>
> Arnd
Got it. Thanks so much for the detailed guide.
Hi Arnd,
On jeu., nov. 24 2016, Arnd Bergmann <[email protected]> wrote:
> On Thursday, November 24, 2016 4:37:36 PM CET Jisheng Zhang wrote:
>> solB (a SW shadow cookie) perhaps gives a better performance: in hot path,
>> such as mvneta_rx(), the driver accesses buf_cookie and buf_phys_addr of
>> rx_desc which is allocated by dma_alloc_coherent, it's noncacheable if the
>> device isn't cache-coherent. I didn't measure the performance difference,
>> because in fact we take solA as well internally. From your experience,
>> can the performance gain deserve the complex code?
>
> Yes, a read from uncached memory is fairly slow, so if you have a chance
> to avoid that it will probably help. When adding complexity to the code,
> it probably makes sense to take a runtime profile anyway quantify how
> much it gains.
>
> On machines that have cache-coherent DMA, accessing the descriptor
> should be fine, as you already have to load the entire cache line
> to read the status field.
>
> Looking at this snippet:
>
> rx_status = rx_desc->status;
> rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
> data = (unsigned char *)rx_desc->buf_cookie;
> phys_addr = rx_desc->buf_phys_addr;
> pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
> bm_pool = &pp->bm_priv->bm_pools[pool_id];
>
> if (!mvneta_rxq_desc_is_first_last(rx_status) ||
> (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
> err_drop_frame_ret_pool:
> /* Return the buffer to the pool */
> mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
> rx_desc->buf_phys_addr);
> err_drop_frame:
>
>
> I think there is more room for optimizing if you start: you read
> the status field twice (the second one in MVNETA_RX_GET_BM_POOL_ID)
> and you can cache the buf_phys_addr along with the virtual address
> once you add that.
I agree we can optimize this code but it is not related to the 64 bits
conversion. Indeed this part is running when we use the HW buffer
management, however currently this part is not ready at all for 64
bits. The virtual address is directly handled by the hardware but it has
only 32 bits to store it in the cookie. So if we want to use the HWBM in
64 bits we need to redesign the code, (maybe by storing the virtual
address in a array and pass the index in the cookie).
Gregory
>
> Generally speaking, I'd recommend using READ_ONCE()/WRITE_ONCE()
> to access the descriptor fields, to ensure the compiler doesn't
> add extra references as well as to annotate the expensive
> operations.
>
> Arnd
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Hi Gregory,
2016-11-24 16:01 GMT+01:00 Gregory CLEMENT <[email protected]>:
> Hi Arnd,
>
> On jeu., nov. 24 2016, Arnd Bergmann <[email protected]> wrote:
>
>> On Thursday, November 24, 2016 4:37:36 PM CET Jisheng Zhang wrote:
>>> solB (a SW shadow cookie) perhaps gives a better performance: in hot path,
>>> such as mvneta_rx(), the driver accesses buf_cookie and buf_phys_addr of
>>> rx_desc which is allocated by dma_alloc_coherent, it's noncacheable if the
>>> device isn't cache-coherent. I didn't measure the performance difference,
>>> because in fact we take solA as well internally. From your experience,
>>> can the performance gain deserve the complex code?
>>
>> Yes, a read from uncached memory is fairly slow, so if you have a chance
>> to avoid that it will probably help. When adding complexity to the code,
>> it probably makes sense to take a runtime profile anyway quantify how
>> much it gains.
>>
>> On machines that have cache-coherent DMA, accessing the descriptor
>> should be fine, as you already have to load the entire cache line
>> to read the status field.
>>
>> Looking at this snippet:
>>
>> rx_status = rx_desc->status;
>> rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
>> data = (unsigned char *)rx_desc->buf_cookie;
>> phys_addr = rx_desc->buf_phys_addr;
>> pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
>> bm_pool = &pp->bm_priv->bm_pools[pool_id];
>>
>> if (!mvneta_rxq_desc_is_first_last(rx_status) ||
>> (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
>> err_drop_frame_ret_pool:
>> /* Return the buffer to the pool */
>> mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
>> rx_desc->buf_phys_addr);
>> err_drop_frame:
>>
>>
>> I think there is more room for optimizing if you start: you read
>> the status field twice (the second one in MVNETA_RX_GET_BM_POOL_ID)
>> and you can cache the buf_phys_addr along with the virtual address
>> once you add that.
>
> I agree we can optimize this code but it is not related to the 64 bits
> conversion. Indeed this part is running when we use the HW buffer
> management, however currently this part is not ready at all for 64
> bits. The virtual address is directly handled by the hardware but it has
> only 32 bits to store it in the cookie. So if we want to use the HWBM in
> 64 bits we need to redesign the code, (maybe by storing the virtual
> address in a array and pass the index in the cookie).
>
How about storing data (virt address and maybe other stuff) as a part
of data buffer and using rx_packet_offset? It has to be used for a3700
anyway. No need of additional rings whatsoever.
Best regards,
Marcin
Le 24/11/2016 à 07:01, Gregory CLEMENT a écrit :
> Hi Arnd,
>
> On jeu., nov. 24 2016, Arnd Bergmann <[email protected]> wrote:
>
>> On Thursday, November 24, 2016 4:37:36 PM CET Jisheng Zhang wrote:
>>> solB (a SW shadow cookie) perhaps gives a better performance: in hot path,
>>> such as mvneta_rx(), the driver accesses buf_cookie and buf_phys_addr of
>>> rx_desc which is allocated by dma_alloc_coherent, it's noncacheable if the
>>> device isn't cache-coherent. I didn't measure the performance difference,
>>> because in fact we take solA as well internally. From your experience,
>>> can the performance gain deserve the complex code?
>>
>> Yes, a read from uncached memory is fairly slow, so if you have a chance
>> to avoid that it will probably help. When adding complexity to the code,
>> it probably makes sense to take a runtime profile anyway quantify how
>> much it gains.
>>
>> On machines that have cache-coherent DMA, accessing the descriptor
>> should be fine, as you already have to load the entire cache line
>> to read the status field.
>>
>> Looking at this snippet:
>>
>> rx_status = rx_desc->status;
>> rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
>> data = (unsigned char *)rx_desc->buf_cookie;
>> phys_addr = rx_desc->buf_phys_addr;
>> pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
>> bm_pool = &pp->bm_priv->bm_pools[pool_id];
>>
>> if (!mvneta_rxq_desc_is_first_last(rx_status) ||
>> (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
>> err_drop_frame_ret_pool:
>> /* Return the buffer to the pool */
>> mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
>> rx_desc->buf_phys_addr);
>> err_drop_frame:
>>
>>
>> I think there is more room for optimizing if you start: you read
>> the status field twice (the second one in MVNETA_RX_GET_BM_POOL_ID)
>> and you can cache the buf_phys_addr along with the virtual address
>> once you add that.
>
> I agree we can optimize this code but it is not related to the 64 bits
> conversion. Indeed this part is running when we use the HW buffer
> management, however currently this part is not ready at all for 64
> bits. The virtual address is directly handled by the hardware but it has
> only 32 bits to store it in the cookie.So if we want to use the HWBM in
> 64 bits we need to redesign the code, (maybe by storing the virtual
> address in a array and pass the index in the cookie).
Can't you make sure that skb->data is aligned to a value big enough that
you can still cover the <N> bit physical address space of the adapter
within a 32-bit quantity if you drop the low bits that would be all zeroes?
That way, even though you only have 32-bits of storage/cookie, these
don't have to be the actual 32-bits of your original address, but could
be addr >> 8 for instance?
As you indicate using an index stored in the cookie might be a better
scheme though, since you could attach a lot more metadata to an index in
an local array (which could be in cached memory) as opposed to just an
address.
--
Florian