[PATCH net-next 0/3] Network driver for Armada 375 SoC

[PATCH net-next 0/3] Network driver for Armada 375 SoC

[Ezequiel Garcia]

This patchset adds a new network driver to support the network controller
in Armada 375 SoC.

The network interfaces share a common hardware unit called Packet Processor,
which contains a common register space and per-port register spaces.
    
The new network unit has different RXQ and TXQ management. The ports
associate so-called per-port "logical queues" which are mapped to "physical
queues". The latter are shared among the ports.

Fo the egress part, the mapping for each port is predefined by hardware.
The egress path incorporates so-called aggregation queues (one per CPU),
from where the data is passed to the physical queues and then via prefetch
buffer to the TxDMA.
    
The ingress path has a Parser and Classifier (PnC) and a Buffer Manager (BM)
whose usage is obligatory. We are only implementing a simple configuration
for the Parser and Classifier, yet the code is considerably large.
    
This network unit has other optional features like xPON, WoL, Hardware
Forwarding, and more. This initial commit doesn't provide support for these.
    
The mvpp2 network driver has been written by Marcin Wojtas and then reviewed
and cleaned up by Ezequiel Garcia.

Ezequiel Garcia (3):
  ethernet: Add new driver for Marvell Armada 375 network unit
  ARM: mvebu: Add support for the network controller in Armada 375 SoC
  ARM: mvebu: Enable the network controller in Armada 375 DB board

 .../devicetree/bindings/net/marvell-pp2.txt        |   61 +
 arch/arm/boot/dts/armada-375-db.dts                |   26 +
 arch/arm/boot/dts/armada-375.dtsi                  |   31 +
 drivers/net/ethernet/marvell/Kconfig               |    8 +
 drivers/net/ethernet/marvell/Makefile              |    1 +
 drivers/net/ethernet/marvell/mvpp2.c               | 6378 ++++++++++++++++++++
 6 files changed, 6505 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/marvell-pp2.txt
 create mode 100644 drivers/net/ethernet/marvell/mvpp2.c

-- 
1.9.1

[PATCH 2/3] ARM: mvebu: Add support for the network controller in Armada 375 SoC

[Ezequiel Garcia]

This commit adds the support for the network controller in Marvell
Armada 375 SoC devicetree.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-375.dtsi | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm/boot/dts/armada-375.dtsi b/arch/arm/boot/dts/armada-375.dtsi
index fb92551..5e897d2 100644
--- a/arch/arm/boot/dts/armada-375.dtsi
+++ b/arch/arm/boot/dts/armada-375.dtsi
@@ -151,6 +151,37 @@
 				      <0xc100 0x100>;
 			};
 
+			mdio {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "marvell,orion-mdio";
+				reg = <0xc0054 0x4>;
+			};
+
+			/* Network controller */
+			pp2 at f0000 {
+				compatible = "marvell,armada-375-pp2";
+				reg = <0xf0000 0xa000>, /* Packet Processor regs */
+				      <0xc0000 0x3060>, /* LMS regs */
+				      <0xc4000 0x100>,  /* eth0 regs */
+				      <0xc5000 0x100>;  /* eth1 regs */
+				clocks = <&gateclk 3>, <&gateclk 19>;
+				clock-names = "pp_clk", "gop_clk";
+				status = "disabled";
+
+				eth0: ethernet at c4000 {
+					interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+					port-id = <0>;
+					status = "disabled";
+				};
+
+				eth1: ethernet at c5000 {
+					interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+					port-id = <1>;
+					status = "disabled";
+				};
+			};
+
 			spi0: spi at 10600 {
 				compatible = "marvell,orion-spi";
 				reg = <0x10600 0x50>;
-- 
1.9.1

[PATCH 3/3] ARM: mvebu: Enable the network controller in Armada 375 DB board

[Ezequiel Garcia]

This commit enables the network controller in the Armada 375 DB board,
and configures the two available ethernet interfaces.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-375-db.dts | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/armada-375-db.dts b/arch/arm/boot/dts/armada-375-db.dts
index 1e2919d..43ac381 100644
--- a/arch/arm/boot/dts/armada-375-db.dts
+++ b/arch/arm/boot/dts/armada-375-db.dts
@@ -123,6 +123,32 @@
 				cd-gpios = <&gpio1 12 GPIO_ACTIVE_HIGH>;
 				wp-gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>;
 			};
+
+			mdio {
+				phy0: ethernet-phy at 0 {
+					reg = <0>;
+				};
+
+				phy3: ethernet-phy at 3 {
+					reg = <3>;
+				};
+			};
+
+			pp2 at f0000 {
+				status = "okay";
+
+				ethernet at c4000 {
+					status = "okay";
+					phy = <&phy0>;
+					phy-mode = "rgmii-id";
+				};
+
+				ethernet at c5000 {
+					status = "okay";
+					phy = <&phy3>;
+					phy-mode = "gmii";
+				};
+			};
 		};
 
 		pcie-controller {
-- 
1.9.1

[PATCH 2/3] ARM: mvebu: Add support for the network controller in Armada 375 SoC

[Sergei Shtylyov]

Hello.

On 07/04/2014 10:31 PM, Ezequiel Garcia wrote:

> This commit adds the support for the network controller in Marvell
> Armada 375 SoC devicetree.

> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>   arch/arm/boot/dts/armada-375.dtsi | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)

> diff --git a/arch/arm/boot/dts/armada-375.dtsi b/arch/arm/boot/dts/armada-375.dtsi
> index fb92551..5e897d2 100644
> --- a/arch/arm/boot/dts/armada-375.dtsi
> +++ b/arch/arm/boot/dts/armada-375.dtsi
> @@ -151,6 +151,37 @@
>   				      <0xc100 0x100>;
>   			};
>
> +			mdio {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				compatible = "marvell,orion-mdio";
> +				reg = <0xc0054 0x4>;
> +			};
> +
> +			/* Network controller */
> +			pp2 at f0000 {
> +				compatible = "marvell,armada-375-pp2";
> +				reg = <0xf0000 0xa000>, /* Packet Processor regs */
> +				      <0xc0000 0x3060>, /* LMS regs */
> +				      <0xc4000 0x100>,  /* eth0 regs */
> +				      <0xc5000 0x100>;  /* eth1 regs */

    Hm, why are the above two ranges not listed under the "ethernet" subnodes?

> +				clocks = <&gateclk 3>, <&gateclk 19>;
> +				clock-names = "pp_clk", "gop_clk";
> +				status = "disabled";
> +
> +				eth0: ethernet at c4000 {
> +					interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> +					port-id = <0>;
> +					status = "disabled";
> +				};
> +
> +				eth1: ethernet at c5000 {
> +					interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
> +					port-id = <1>;
> +					status = "disabled";
> +				};
> +			};
> +

WBR, Sergei

[PATCH 2/3] ARM: mvebu: Add support for the network controller in Armada 375 SoC

[Ezequiel Garcia]

On 04 Jul 10:51 PM, Sergei Shtylyov wrote:
> >+			/* Network controller */
> >+			pp2 at f0000 {
> >+				compatible = "marvell,armada-375-pp2";
> >+				reg = <0xf0000 0xa000>, /* Packet Processor regs */
> >+				      <0xc0000 0x3060>, /* LMS regs */
> >+				      <0xc4000 0x100>,  /* eth0 regs */
> >+				      <0xc5000 0x100>;  /* eth1 regs */
> 
>    Hm, why are the above two ranges not listed under the "ethernet" subnodes?
> 

Because as far as I know, if we want to put reg = <...> properties in a sub-node,
then the parent node has to be a bus, but that's not the case.

I can't remember where have I read that, I'll take a look at the reference.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 1/3] ethernet: Add new driver for Marvell Armada 375 network unit

[Francois Romieu]

Partial review below.

> diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
> new file mode 100644
> index 0000000..154b866
> --- /dev/null
> +++ b/drivers/net/ethernet/marvell/mvpp2.c
[...]
> +static int mvpp2_prs_tcam_first_free(struct mvpp2 *pp2, int start, int end)
> +{
> +	int tid;
> +	bool found = false;
> +
> +	if (start < end)
> +		for (tid = start; tid <= end; tid++) {
> +			if (!pp2->prs_shadow[tid].valid) {
> +				found = true;
> +				break;
> +			}
> +		}
> +	else
> +		for (tid = start; tid >= end; tid--) {
> +			if (!pp2->prs_shadow[tid].valid) {
> +				found = true;
> +				break;
> +			}
> +		}

Missing parenthsesis in "if ... else ..." block.

[...]
> +static void mvpp2_defaults_set(struct mvpp2_port *pp)
> +{
[...]
> +	/* At default, mask all interrupts to all present cpus */
> +	for_each_present_cpu(cpu)
> +		mvpp2_cpu_interrupts_disable(pp, cpu);

Would it hurt to issue a single write and disable all irqs ?

[...]
> +static int mvpp2_tx_frag_process(struct mvpp2_port *pp, struct sk_buff *skb,
> +				 struct mvpp2_tx_queue *aggr_txq,
> +				 struct mvpp2_tx_queue *txq)
> +{
[...]
> +error:
> +	/* Release all descriptors that were used to map fragments of
> +	 * this packet, as well as the corresponding DMA mappings
> +	 */
> +	for (i = i - 1; i >= 0; i--) {

You may consider "while (--i >= 0) {" as an idiom to balance a
"for (i = 0; .." loop. Your call.

> +		tx_desc = txq->descs + i;
> +		dma_unmap_single(pp->dev->dev.parent,
> +				 tx_desc->buf_phys_addr,
> +				 tx_desc->data_size,
> +				 DMA_TO_DEVICE);

		dma_unmap_single(pp->dev->dev.parent, tx_desc->buf_phys_addr,
				 tx_desc->data_size, DMA_TO_DEVICE);

You may also factor out mvpp2_dma_unmap_single(pp, tx_desc) or the whole
dma_unmap_single + mvpp2_txq_desc_put sequence.

[...]
> +/* Main tx processing */
> +static int mvpp2_tx(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct mvpp2_port *pp = netdev_priv(dev);
> +	struct mvpp2_tx_queue *txq, *aggr_txq;
> +	struct mvpp2_txq_pcpu *txq_pcpu;
> +	struct mvpp2_tx_desc *tx_desc;
> +	dma_addr_t buf_phys_addr;
> +	int frags = 0;
> +	u16 txq_id;
> +	u32 tx_cmd;
> +
> +	if (!netif_running(dev))
> +		goto out;

The driver is expected to netif_stop_queue when the device goes down, not
the opposite.

[...]
> +static int mvpp2_poll(struct napi_struct *napi, int budget)
> +{
> +	u32 cause_rx_tx, cause_rx;
> +	int rx_done = 0;
> +	struct mvpp2_port *pp = netdev_priv(napi->dev);
> +
> +	if (!netif_running(pp->dev)) {
> +		napi_complete(napi);
> +		return rx_done;
> +	}

Same thing as above.

[...]
> +static int mvpp2_ethtool_set_ringparam(struct net_device *dev,
> +				       struct ethtool_ringparam *ring)
> +{
> +	struct mvpp2_port *pp = netdev_priv(dev);
> +
> +	if ((ring->rx_pending == 0) || (ring->tx_pending == 0))
> +		return -EINVAL;
> +	pp->rx_ring_size = ring->rx_pending < MVPP2_MAX_RXD ?
> +		ring->rx_pending : MVPP2_MAX_RXD;
> +	pp->tx_ring_size = ring->tx_pending < MVPP2_MAX_TXD ?
> +		ring->tx_pending : MVPP2_MAX_TXD;
> +
> +	if (netif_running(dev)) {
> +		mvpp2_stop(dev);
> +		if (mvpp2_open(dev)) {

You aren't supposed to (ab)use net_device_ops.{ndo_open / stop} like that.
mvpp2_tx() is still racing with mvpp2_cleanup_txqs(). Even if you go through
the hassle of (1) avoiding this race, then (2) enforcing a safe double call
of mvpp2_stop without any mvpp2_open inbetween, nobody wants its device to
become unresponsive because of a failed ring parameter change: the driver
must stop and configure the physical device more gently.

Depending on the rework, you may consider postponing ethtool ring change to
a separate patch series.

[...]
> +static const struct net_device_ops mvpp2_netdev_ops = {
> +	.ndo_open            = mvpp2_open,
> +	.ndo_stop            = mvpp2_stop,
> +	.ndo_start_xmit      = mvpp2_tx,
> +	.ndo_set_rx_mode     = mvpp2_set_rx_mode,
> +	.ndo_set_mac_address = mvpp2_set_mac_address,
> +	.ndo_change_mtu      = mvpp2_change_mtu,
> +	.ndo_get_stats64     = mvpp2_get_stats64,
> +};

Please replace spaces by tabs before "=".

> +
> +static const struct ethtool_ops mvpp2_eth_tool_ops = {
> +	.get_link       = ethtool_op_get_link,
> +	.get_settings   = mvpp2_ethtool_get_settings,
> +	.set_settings   = mvpp2_ethtool_set_settings,
> +	.set_coalesce   = mvpp2_ethtool_set_coalesce,
> +	.get_coalesce   = mvpp2_ethtool_get_coalesce,
> +	.get_drvinfo    = mvpp2_ethtool_get_drvinfo,
> +	.get_ringparam  = mvpp2_ethtool_get_ringparam,
> +	.set_ringparam	= mvpp2_ethtool_set_ringparam,
> +};

Same as above.

[...]
> +static int mvpp2_port_init(struct mvpp2_port *pp)
> +{
> +	struct device *dev = pp->dev->dev.parent;
> +	struct mvpp2 *pp2 = pp->pp2;
> +	struct mvpp2_txq_pcpu *txq_pcpu;
> +	int queue, txp, cpu;
> +
> +	if (pp->first_rxq + rxq_number > MVPP2_RXQ_TOTAL_NUM)
> +		return -EINVAL;
> +
> +	/* Disable port */
> +	mvpp2_egress_disable(pp);
> +	mvpp2_port_disable(pp);
> +
> +	pp->txqs = devm_kzalloc(dev, pp->txp_num * txq_number *
> +				sizeof(struct mvpp2_tx_queue *), GFP_KERNEL);
> +	if (!pp->txqs)
> +		return -ENOMEM;
> +
> +	/* Associate physical Tx queues to this port and initialize.
> +	 * The mapping is predefined.
> +	 */
> +	for (txp = 0; txp < pp->txp_num; txp++) {
> +		for (queue = 0; queue < txq_number; queue++) {
> +			int txq_idx = txp * txq_number + queue;
> +			int queue_phy_id = mvpp2_txq_phys(pp->id, queue);
> +			struct mvpp2_tx_queue *txq;
> +
> +			txq = devm_kzalloc(dev, sizeof(*txq), GFP_KERNEL);
> +			if (!txq)
> +				return -ENOMEM;
> +
> +			txq->pcpu = alloc_percpu(struct mvpp2_txq_pcpu);
> +			if (!txq->pcpu)
> +				return -ENOMEM;

There is a per_cpu leak if mvpp2_port_init fails.

[...]
> +/* Ports initialization */
> +static int mvpp2_port_probe(struct platform_device *pdev,
> +			    struct device_node *port_node,
> +			    struct mvpp2 *pp2,
> +			    int *next_first_rxq)
> +{
> +	struct device_node *phy_node;
> +	struct mvpp2_port *pp;

(nit below)

I am not a huge fan of the "pp2" variable as there's no "pp1" but I've seen
worse. However naming "pp" some completely unrelated data imho verges on
confusing

[...]
> +	dev->irq = irq_of_parse_and_map(port_node, 0);
> +	if (dev->irq == 0) {
> +		err = -EINVAL;
> +		goto err_free_netdev;
> +	}

net_device.irq should be considered legacy. It would be nice to avoid
more uses of it. You may store it near pp->base for instance.

-- 
Ueimor

[PATCH 1/3] ethernet: Add new driver for Marvell Armada 375 network unit

[Joe Perches]

On Sat, 2014-07-05 at 23:03 +0200, Francois Romieu wrote:
> Partial review below.

trivia:

> > diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
[]
> > +static int mvpp2_prs_tcam_first_free(struct mvpp2 *pp2, int start, int end)
> > +{
> > +	int tid;
> > +	bool found = false;
> > +
> > +	if (start < end)
> > +		for (tid = start; tid <= end; tid++) {
> > +			if (!pp2->prs_shadow[tid].valid) {
> > +				found = true;
> > +				break;
> > +			}
> > +		}
> > +	else
> > +		for (tid = start; tid >= end; tid--) {
> > +			if (!pp2->prs_shadow[tid].valid) {
> > +				found = true;
> > +				break;
> > +			}
> > +		}
> 
> Missing parenthsesis in "if ... else ..." block.

Perhaps it's better to use swap if start > end

	if (start > end)
		swap(start, end);

so there's just one loop

[PATCH 1/3] ethernet: Add new driver for Marvell Armada 375 network unit

[Ezequiel Garcia]

Hello Francois,

First of all, thanks a lot for such a detailed review!

On 05 Jul 11:03 PM, Francois Romieu wrote:
> > diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
> > new file mode 100644
> > index 0000000..154b866
> > --- /dev/null
> > +++ b/drivers/net/ethernet/marvell/mvpp2.c
> [...]
> > +static int mvpp2_prs_tcam_first_free(struct mvpp2 *pp2, int start, int end)
> > +{
> > +	int tid;
> > +	bool found = false;
> > +
> > +	if (start < end)
> > +		for (tid = start; tid <= end; tid++) {
> > +			if (!pp2->prs_shadow[tid].valid) {
> > +				found = true;
> > +				break;
> > +			}
> > +		}
> > +	else
> > +		for (tid = start; tid >= end; tid--) {
> > +			if (!pp2->prs_shadow[tid].valid) {
> > +				found = true;
> > +				break;
> > +			}
> > +		}
> 
> Missing parenthsesis in "if ... else ..." block.
> 
> [...]

Right, I'll do as Joe suggests and use swap() to have just one loop.

> > +static void mvpp2_defaults_set(struct mvpp2_port *pp)
> > +{
> [...]
> > +	/* At default, mask all interrupts to all present cpus */
> > +	for_each_present_cpu(cpu)
> > +		mvpp2_cpu_interrupts_disable(pp, cpu);
> 
> Would it hurt to issue a single write and disable all irqs ?
> 

No, that sounds just fine. I'll fix it.

> [...]
> > +static int mvpp2_tx_frag_process(struct mvpp2_port *pp, struct sk_buff *skb,
> > +				 struct mvpp2_tx_queue *aggr_txq,
> > +				 struct mvpp2_tx_queue *txq)
> > +{
> [...]
> > +error:
> > +	/* Release all descriptors that were used to map fragments of
> > +	 * this packet, as well as the corresponding DMA mappings
> > +	 */
> > +	for (i = i - 1; i >= 0; i--) {
> 
> You may consider "while (--i >= 0) {" as an idiom to balance a
> "for (i = 0; .." loop. Your call.
> 

Hm... we've used the same idiom in the mvneta driver so I'd rather keep it
just to have some consistency between the mvneta and the mvpp2 drivers.

> > +		tx_desc = txq->descs + i;
> > +		dma_unmap_single(pp->dev->dev.parent,
> > +				 tx_desc->buf_phys_addr,
> > +				 tx_desc->data_size,
> > +				 DMA_TO_DEVICE);
> 
> 		dma_unmap_single(pp->dev->dev.parent, tx_desc->buf_phys_addr,
> 				 tx_desc->data_size, DMA_TO_DEVICE);
> 
> You may also factor out mvpp2_dma_unmap_single(pp, tx_desc) or the whole
> dma_unmap_single + mvpp2_txq_desc_put sequence.
> 

Refactoring those two sounds a good cleanup.

> [...]
> > +/* Main tx processing */
> > +static int mvpp2_tx(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +	struct mvpp2_port *pp = netdev_priv(dev);
> > +	struct mvpp2_tx_queue *txq, *aggr_txq;
> > +	struct mvpp2_txq_pcpu *txq_pcpu;
> > +	struct mvpp2_tx_desc *tx_desc;
> > +	dma_addr_t buf_phys_addr;
> > +	int frags = 0;
> > +	u16 txq_id;
> > +	u32 tx_cmd;
> > +
> > +	if (!netif_running(dev))
> > +		goto out;
> 
> The driver is expected to netif_stop_queue when the device goes down, not
> the opposite.
> 

Right, these netif_running() checks are redundant: the driver already stops
the queues when it goes down, so it seems safe to drop them.

> [...]
> > +static int mvpp2_poll(struct napi_struct *napi, int budget)
> > +{
> > +	u32 cause_rx_tx, cause_rx;
> > +	int rx_done = 0;
> > +	struct mvpp2_port *pp = netdev_priv(napi->dev);
> > +
> > +	if (!netif_running(pp->dev)) {
> > +		napi_complete(napi);
> > +		return rx_done;
> > +	}
> 
> Same thing as above.
> 
> [...]
> > +static int mvpp2_ethtool_set_ringparam(struct net_device *dev,
> > +				       struct ethtool_ringparam *ring)
> > +{
> > +	struct mvpp2_port *pp = netdev_priv(dev);
> > +
> > +	if ((ring->rx_pending == 0) || (ring->tx_pending == 0))
> > +		return -EINVAL;
> > +	pp->rx_ring_size = ring->rx_pending < MVPP2_MAX_RXD ?
> > +		ring->rx_pending : MVPP2_MAX_RXD;
> > +	pp->tx_ring_size = ring->tx_pending < MVPP2_MAX_TXD ?
> > +		ring->tx_pending : MVPP2_MAX_TXD;
> > +
> > +	if (netif_running(dev)) {
> > +		mvpp2_stop(dev);
> > +		if (mvpp2_open(dev)) {
> 
> You aren't supposed to (ab)use net_device_ops.{ndo_open / stop} like that.
> mvpp2_tx() is still racing with mvpp2_cleanup_txqs(). Even if you go through
> the hassle of (1) avoiding this race, then (2) enforcing a safe double call
> of mvpp2_stop without any mvpp2_open inbetween, nobody wants its device to
> become unresponsive because of a failed ring parameter change: the driver
> must stop and configure the physical device more gently.
> 
> Depending on the rework, you may consider postponing ethtool ring change to
> a separate patch series.
> 

Right, we will rework this.

> [...]
> > +static const struct net_device_ops mvpp2_netdev_ops = {
> > +	.ndo_open            = mvpp2_open,
> > +	.ndo_stop            = mvpp2_stop,
> > +	.ndo_start_xmit      = mvpp2_tx,
> > +	.ndo_set_rx_mode     = mvpp2_set_rx_mode,
> > +	.ndo_set_mac_address = mvpp2_set_mac_address,
> > +	.ndo_change_mtu      = mvpp2_change_mtu,
> > +	.ndo_get_stats64     = mvpp2_get_stats64,
> > +};
> 
> Please replace spaces by tabs before "=".
> 

Done.

> > +
> > +static const struct ethtool_ops mvpp2_eth_tool_ops = {
> > +	.get_link       = ethtool_op_get_link,
> > +	.get_settings   = mvpp2_ethtool_get_settings,
> > +	.set_settings   = mvpp2_ethtool_set_settings,
> > +	.set_coalesce   = mvpp2_ethtool_set_coalesce,
> > +	.get_coalesce   = mvpp2_ethtool_get_coalesce,
> > +	.get_drvinfo    = mvpp2_ethtool_get_drvinfo,
> > +	.get_ringparam  = mvpp2_ethtool_get_ringparam,
> > +	.set_ringparam	= mvpp2_ethtool_set_ringparam,
> > +};
> 
> Same as above.
> 

Done.

> [...]
> > +static int mvpp2_port_init(struct mvpp2_port *pp)
> > +{
> > +	struct device *dev = pp->dev->dev.parent;
> > +	struct mvpp2 *pp2 = pp->pp2;
> > +	struct mvpp2_txq_pcpu *txq_pcpu;
> > +	int queue, txp, cpu;
> > +
> > +	if (pp->first_rxq + rxq_number > MVPP2_RXQ_TOTAL_NUM)
> > +		return -EINVAL;
> > +
> > +	/* Disable port */
> > +	mvpp2_egress_disable(pp);
> > +	mvpp2_port_disable(pp);
> > +
> > +	pp->txqs = devm_kzalloc(dev, pp->txp_num * txq_number *
> > +				sizeof(struct mvpp2_tx_queue *), GFP_KERNEL);
> > +	if (!pp->txqs)
> > +		return -ENOMEM;
> > +
> > +	/* Associate physical Tx queues to this port and initialize.
> > +	 * The mapping is predefined.
> > +	 */
> > +	for (txp = 0; txp < pp->txp_num; txp++) {
> > +		for (queue = 0; queue < txq_number; queue++) {
> > +			int txq_idx = txp * txq_number + queue;
> > +			int queue_phy_id = mvpp2_txq_phys(pp->id, queue);
> > +			struct mvpp2_tx_queue *txq;
> > +
> > +			txq = devm_kzalloc(dev, sizeof(*txq), GFP_KERNEL);
> > +			if (!txq)
> > +				return -ENOMEM;
> > +
> > +			txq->pcpu = alloc_percpu(struct mvpp2_txq_pcpu);
> > +			if (!txq->pcpu)
> > +				return -ENOMEM;
> 
> There is a per_cpu leak if mvpp2_port_init fails.
> 

Yes, good catch.

> [...]
> > +/* Ports initialization */
> > +static int mvpp2_port_probe(struct platform_device *pdev,
> > +			    struct device_node *port_node,
> > +			    struct mvpp2 *pp2,
> > +			    int *next_first_rxq)
> > +{
> > +	struct device_node *phy_node;
> > +	struct mvpp2_port *pp;
> 
> (nit below)
> 
> I am not a huge fan of the "pp2" variable as there's no "pp1" but I've seen
> worse. However naming "pp" some completely unrelated data imho verges on
> confusing
> 

Hm, yes, I see what you mean. These names bother me too, and I think we could
probably change it so "pp2" is "dev", and "pp" is "port". But on the other
side, I'm not sure it's worth doing such an intrusive change just to fix these
names.

What do you think?

> [...]
> > +	dev->irq = irq_of_parse_and_map(port_node, 0);
> > +	if (dev->irq == 0) {
> > +		err = -EINVAL;
> > +		goto err_free_netdev;
> > +	}
> 
> net_device.irq should be considered legacy. It would be nice to avoid
> more uses of it. You may store it near pp->base for instance.
> 

Done.

Thanks a lot for the feedback!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 1/3] ethernet: Add new driver for Marvell Armada 375 network unit

[Francois Romieu]

Ezequiel Garcia <ezequiel.garcia@free-electrons.com> :
[...]
> Hm, yes, I see what you mean. These names bother me too, and I think we could
> probably change it so "pp2" is "dev", and "pp" is "port". But on the other
> side, I'm not sure it's worth doing such an intrusive change just to fix these
> names.
> 
> What do you think ?

Yike. Please don't substitute "pp2" with some "dev": it's a "priv" more than
a "dev". s/pp/port/ would be good enough. Your call.

-- 
Ueimor