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

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

[Ezequiel Garcia]

Changes from v2:

  * Reworked mvpp2_prs_tcam_first_free() as suggested by Joe and Francois,
    to have a single loop instead of two.

  * Replaced mvpp2_cpu_interrupts_enable/disable(pp, cpu) with one function
    that enables/disable interrupts on all the CPUs at once.

  * Factor out Tx descriptor DMA unmap + descriptor put sequence to have
    more readable code, as suggested by Francois.

  * Remove redundant netif_running() checks in the ingress and egress path,
    as suggested by Francois.

  * Reworked ring parameter, MTU and MAC address setting to produce a
    more gentle modification of the parameter, and have a fallback in the
    event of a failure.

  * Fixed a percpu memory leak on error path, also noted by Francois.

  * Removed the usage of the legacy net_device irq field, requested by
    Francois.

  * Removed the unneeded multiple Tx port support. It was hardcoded to a single
    Tx port in the previous version so we decided to drop it and simplify the
    code.

  * Optimize the on_each_cpu() calls to clear the sent counters and the
    TX_DONE pkts coalescing setting. on_each_cpu is expensive so it's better
    to minize the calls to it.

  * Used sane node names in the devicetree files.

Changes from v1:

  * Marcin Wojtas is the author of the driver, so I fixed authorship
    for patch 1/3:
    "ethernet: Add new driver for Marvell Armada 375 network unit"

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 (2):
  ARM: mvebu: Add support for the network controller in Armada 375 SoC
  ARM: mvebu: Enable the network controller in Armada 375 DB board

Marcin Wojtas (2):
  ethernet: Add new driver for Marvell Armada 375 network unit
  ARM: mvebu: enable Armada 375 network driver in mvebu_v7_defconfig

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

-- 
1.9.1

[PATCH v3 2/4] 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..d4619ad 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 */
+			ethernet 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: eth0 at c4000 {
+					interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+					port-id = <0>;
+					status = "disabled";
+				};
+
+				eth1: eth1 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 v3 3/4] 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..929ae00 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>;
+				};
+			};
+
+			ethernet at f0000 {
+				status = "okay";
+
+				eth0 at c4000 {
+					status = "okay";
+					phy = <&phy0>;
+					phy-mode = "rgmii-id";
+				};
+
+				eth1 at c5000 {
+					status = "okay";
+					phy = <&phy3>;
+					phy-mode = "gmii";
+				};
+			};
 		};
 
 		pcie-controller {
-- 
1.9.1

[PATCH v3 4/4] ARM: mvebu: enable Armada 375 network driver in mvebu_v7_defconfig

[Ezequiel Garcia]

From: Marcin Wojtas <mw@semihalf.com>

Now that the Armada 375 network driver has been pushed, let's enable it
in mvebu_v7_defconfig.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 arch/arm/configs/mvebu_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/mvebu_v7_defconfig b/arch/arm/configs/mvebu_v7_defconfig
index b0bfefa..b477631 100644
--- a/arch/arm/configs/mvebu_v7_defconfig
+++ b/arch/arm/configs/mvebu_v7_defconfig
@@ -46,6 +46,7 @@ CONFIG_AHCI_MVEBU=y
 CONFIG_SATA_MV=y
 CONFIG_NETDEVICES=y
 CONFIG_MVNETA=y
+CONFIG_MVPP2=y
 CONFIG_MARVELL_PHY=y
 CONFIG_MWIFIEX=y
 CONFIG_MWIFIEX_SDIO=y
-- 
1.9.1

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

[Francois Romieu]

Ezequiel Garcia <ezequiel.garcia@free-electrons.com> :
[...]
> diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
> new file mode 100644
> index 0000000..21931a1
> --- /dev/null
> +++ b/drivers/net/ethernet/marvell/mvpp2.c
[...]
> +static inline void mvpp2_mib_counters_clear(struct mvpp2_port *pp)
> +{
> +	int i;
> +	u32 dummy;
> +
> +	/* Perform dummy reads from MIB counters */
> +	for (i = 0; i < MVPP2_MIB_LATE_COLLISION; i += 4)
> +		dummy = readl(pp->pp2->lms_base +
> +			      (MVPP2_MIB_COUNTERS_BASE(pp->id) + i));
                              ^ excess parenthesis

Please reduce the scope of the dummy variable and add curly braces in the
(multi-line) "for" block.

You may remove the "Perform dummy ..." comment. The code is literate enough.

[...]
> +/* Update lookup field in tcam sw entry */
> +static void mvpp2_prs_tcam_lu_set(struct mvpp2_prs_entry *pe, unsigned int lu)
> +{
> +	pe->tcam.byte[MVPP2_PRS_TCAM_LU_BYTE] = lu;
> +	pe->tcam.byte[MVPP2_PRS_TCAM_EN_OFFS(MVPP2_PRS_TCAM_LU_BYTE)] =
> +							      MVPP2_PRS_LU_MASK;

Use a local u8 *b = pe->tcam.byte; ?

> +}
> +
> +/* Update mask for single port in tcam sw entry */
> +static void mvpp2_prs_tcam_port_set(struct mvpp2_prs_entry *pe,
> +				    unsigned int port, bool add)
> +{
> +	if (add)
> +		pe->tcam.byte[MVPP2_PRS_TCAM_EN_OFFS(MVPP2_PRS_TCAM_PORT_BYTE)]
> +								&= ~(1 << port);
> +	else
> +		pe->tcam.byte[MVPP2_PRS_TCAM_EN_OFFS(MVPP2_PRS_TCAM_PORT_BYTE)]
> +								 |= (1 << port);

Use a local u8 *b = pe->tcam.byte; ?

[...]
> +/* Compare tcam data bytes with a pattern */
> +static bool mvpp2_prs_tcam_data_cmp(struct mvpp2_prs_entry *pe,
> +				    unsigned int offs, unsigned int size,
> +					   unsigned char *bytes)
> +{
> +	unsigned char byte, mask;
> +	int i;
> +
> +	for (i = 0; i < size; i++) {
> +		mvpp2_prs_tcam_data_byte_get(pe, offs + i, &byte, &mask);
> +
> +		if (byte != bytes[i])
> +			return false;

Please reduce the scope of "byte" and "mask".

> +	}
> +	return true;
> +}
> +
> +/* Update ai bits in tcam sw entry */
> +static void mvpp2_prs_tcam_ai_update(struct mvpp2_prs_entry *pe,
> +				     unsigned int bits, unsigned int enable)
> +{
> +	int i;
> +
> +	for (i = 0; i < MVPP2_PRS_AI_BITS; i++)
> +		if (enable & BIT(i)) {
> +			if (bits & BIT(i))
> +				pe->tcam.byte[MVPP2_PRS_TCAM_AI_BYTE] |=
> +								       (1 << i);
> +			else
> +				pe->tcam.byte[MVPP2_PRS_TCAM_AI_BYTE] &=
> +								      ~(1 << i);
> +		}

Curly braces for the "for" loop + local variable for &pe->tcam.byte[foo]

[...]
> +/* Update ri bits in sram sw entry */
> +static void mvpp2_prs_sram_ri_update(struct mvpp2_prs_entry *pe,
> +				     unsigned int bits, unsigned int mask)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < MVPP2_PRS_SRAM_RI_CTRL_BITS; i++)

Please add curly braces.

> +		if (mask & BIT(i)) {

Invert the test and add a "continue" statement to recover the missing indent
level ?

> +			if (bits & BIT(i))
> +				mvpp2_prs_sram_bits_set(pe,
> +							MVPP2_PRS_SRAM_RI_OFFS + i, 1);
> +			else
> +				mvpp2_prs_sram_bits_clear(pe,
> +							  MVPP2_PRS_SRAM_RI_OFFS + i, 1);
> +			mvpp2_prs_sram_bits_set(pe,
> +						MVPP2_PRS_SRAM_RI_CTRL_OFFS + i, 1);
> +		}
> +}
> +
> +/* Obtain ri bits and their mask from sram sw entry */
> +static void mvpp2_prs_sram_ri_get(struct mvpp2_prs_entry *pe,
> +				  unsigned int *bits, unsigned int *mask)
> +{
> +	*bits = pe->sram.word[MVPP2_PRS_SRAM_RI_WORD];
> +	*mask = pe->sram.word[MVPP2_PRS_SRAM_RI_CTRL_WORD];
> +}
> +
> +/* Update ai bits in sram sw entry */
> +static void mvpp2_prs_sram_ai_update(struct mvpp2_prs_entry *pe,
> +				     unsigned int bits, unsigned int mask)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < MVPP2_PRS_SRAM_AI_CTRL_BITS; i++)

Curly braces.

> +		if (mask & BIT(i)) {
> +			if (bits & BIT(i))
> +				mvpp2_prs_sram_bits_set(pe,
> +							MVPP2_PRS_SRAM_AI_OFFS + i, 1);
> +			else
> +				mvpp2_prs_sram_bits_clear(pe,
> +							  MVPP2_PRS_SRAM_AI_OFFS + i, 1);
> +			mvpp2_prs_sram_bits_set(pe,
> +						MVPP2_PRS_SRAM_AI_CTRL_OFFS + i, 1);
> +		}
> +}
> +
> +/* Read ai bits from sram sw entry */
> +static void mvpp2_prs_sram_ai_get(struct mvpp2_prs_entry *pe,
> +				  unsigned int *bits, unsigned int *enable)
> +{
> +	*bits = (pe->sram.byte[MVPP2_BIT_TO_BYTE(MVPP2_PRS_SRAM_AI_OFFS)]
> +		>> (MVPP2_PRS_SRAM_AI_OFFS % 8)) |

Move ">>" to the end of the previous line.

Use local variable for pe->sram.byte.

> +		(pe->sram.byte[MVPP2_BIT_TO_BYTE(MVPP2_PRS_SRAM_AI_OFFS +
> +		MVPP2_PRS_SRAM_AI_CTRL_BITS)] <<
> +		(8 - (MVPP2_PRS_SRAM_AI_OFFS % 8)));
> +
> +	*enable = (pe->sram.byte[MVPP2_BIT_TO_BYTE(MVPP2_PRS_SRAM_AI_CTRL_OFFS)]
> +		  >> (MVPP2_PRS_SRAM_AI_CTRL_OFFS % 8)) |
> +		  (pe->sram.byte[MVPP2_BIT_TO_BYTE(MVPP2_PRS_SRAM_AI_CTRL_OFFS +
> +		  MVPP2_PRS_SRAM_AI_CTRL_BITS)] <<
> +		  (8 - (MVPP2_PRS_SRAM_AI_CTRL_OFFS % 8)));
[...]
> +static struct mvpp2_prs_entry *mvpp2_prs_flow_find(struct mvpp2 *pp2, int flow)
> +{
> +	struct mvpp2_prs_entry *pe;
> +	unsigned int enable;
> +	unsigned int bits;

Excess scope.

> +	int tid;
> +
> +	pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> +	if (!pe)
> +		return NULL;
> +	mvpp2_prs_tcam_lu_set(pe, MVPP2_PRS_LU_FLOWS);
> +
> +	/* Go through the all entires with MVPP2_PRS_LU_FLOWS */
> +	for (tid = MVPP2_PRS_TCAM_SRAM_SIZE - 1; tid >= 0; tid--) {
> +		if ((!pp2->prs_shadow[tid].valid) ||

Excess parenthesis.

> +		    (pp2->prs_shadow[tid].lu != MVPP2_PRS_LU_FLOWS))
> +			continue;
> +
> +		pe->index = tid;
> +		mvpp2_prs_hw_read(pp2, pe);
> +		mvpp2_prs_sram_ai_get(pe, &bits, &enable);
> +
> +		/* Sram store classification lookup ID in AI bits [5:0] */
> +		if ((bits & MVPP2_PRS_FLOW_ID_MASK) == flow)
> +			return pe;
> +	}
> +	kfree(pe);
> +
> +	return NULL;
> +}
> +
> +/* Return first free tcam index, seeking from start to end */
> +static int mvpp2_prs_tcam_first_free(struct mvpp2 *pp2, unsigned char start,
> +				     unsigned char end)
> +{
> +	int tid;
> +	bool found = false;
> +
> +	if (start > end)
> +		swap(start, end);
> +
> +	for (tid = start; tid <= end; tid++) {
> +		if (!pp2->prs_shadow[tid].valid) {
> +			found = true;
> +			break;

Don't break: test for tid < MVPP2_PRS_TCAM_SRAM_SIZE and return.

> +		}
> +	}
> +
> +	if (found && (tid < MVPP2_PRS_TCAM_SRAM_SIZE))
> +		return tid;
> +	return -EINVAL;
[...]
> +static void mvpp2_prs_mac_all_multi_set(struct mvpp2 *pp2, int port, bool add)
> +{
> +	struct mvpp2_prs_entry pe;
> +	unsigned int index = 0;
> +	unsigned int i;
> +
> +	/* Ethernet multicast address first byte is
> +	 * 0x01 for IPv4 and 0x33 for IPv6
> +	 */
> +	unsigned char da_mc[MVPP2_PRS_MAX_MAC_MC] = { 0x01, 0x33 };
> +
> +	for (i = MVPP2_PRS_IP4_MAC_MC; i < MVPP2_PRS_MAX_MAC_MC; i++) {
> +		if (i == MVPP2_PRS_IP4_MAC_MC)
> +			index = MVPP2_PE_MAC_MC_ALL;
> +		else
> +			index = MVPP2_PE_MAC_MC_IP6;

Ternary ?

[...]
> +/* Set entry for dsa packets */
> +static void mvpp2_prs_dsa_tag_set(struct mvpp2 *pp2, int port, bool add,
> +				  bool tagged, bool extend)
> +{
> +	struct mvpp2_prs_entry pe;
> +	int tid;
> +	int shift;
> +
> +	if (extend) {
> +		if (tagged)
> +			tid = MVPP2_PE_EDSA_TAGGED;
> +		else
> +			tid = MVPP2_PE_EDSA_UNTAGGED;

Ternary ?

> +		shift = 8;
> +	} else {
> +		if (tagged)
> +			tid = MVPP2_PE_DSA_TAGGED;
> +		else
> +			tid = MVPP2_PE_DSA_UNTAGGED;

Ternary ?

[...]
> +static void mvpp2_prs_dsa_tag_ethertype_set(struct mvpp2 *pp2, int port,
> +					    bool add, bool tagged, bool extend)
> +{
> +	struct mvpp2_prs_entry pe;
> +	int tid, shift, port_mask;
> +
> +	if (extend) {
> +		if (tagged)
> +			tid = MVPP2_PE_ETYPE_EDSA_TAGGED;
> +		else
> +			tid = MVPP2_PE_ETYPE_EDSA_UNTAGGED;

Ternary ?

> +		port_mask = 0;
> +		shift = 8;
> +	} else {
> +		if (tagged)
> +			tid = MVPP2_PE_ETYPE_DSA_TAGGED;
> +		else
> +			tid = MVPP2_PE_ETYPE_DSA_UNTAGGED;

Ternary ?

[...]
> +static struct mvpp2_prs_entry *mvpp2_prs_vlan_find(struct mvpp2 *pp2,
> +						   unsigned short tpid, int ai)
> +{
> +	struct mvpp2_prs_entry *pe;
> +	unsigned int ri_bits, ai_bits;
> +	unsigned int enable;
> +	unsigned char tpid_arr[2];
> +	int tid;

Nitnit: please xmas-treefy (wherever possible).

> +
> +	pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> +	if (!pe)
> +		return NULL;
> +	mvpp2_prs_tcam_lu_set(pe, MVPP2_PRS_LU_VLAN);
> +
> +	tpid_arr[0] = ((unsigned char *)&tpid)[1];
> +	tpid_arr[1] = ((unsigned char *)&tpid)[0];
> +
> +	/* Go through the all entries with MVPP2_PRS_LU_VLAN */
> +	for (tid = MVPP2_PE_FIRST_FREE_TID;
> +	     tid <= MVPP2_PE_LAST_FREE_TID; tid++) {
> +		if ((!pp2->prs_shadow[tid].valid) ||

Excess parenthesis.

> +		    (pp2->prs_shadow[tid].lu != MVPP2_PRS_LU_VLAN))
> +			continue;
> +
> +		pe->index = tid;
> +
> +		mvpp2_prs_hw_read(pp2, pe);
> +		if (mvpp2_prs_tcam_data_cmp(pe, 0, 2, tpid_arr)) {
> +			/* Get vlan type */
> +			mvpp2_prs_sram_ri_get(pe, &ri_bits, &enable);
> +			ri_bits = (ri_bits & MVPP2_PRS_RI_VLAN_MASK);
> +
> +			/* Get current ai value from tcam */
> +			mvpp2_prs_tcam_ai_get(pe, &ai_bits, &enable);
> +			/* Clear double vlan bit */
> +			ai_bits &= ~MVPP2_PRS_DBL_VLAN_AI_BIT;
> +
> +			if (ai != ai_bits)
> +				continue;
> +
> +			if ((ri_bits == MVPP2_PRS_RI_VLAN_SINGLE) ||
> +			    (ri_bits == MVPP2_PRS_RI_VLAN_TRIPLE))

Excess parenthesis.

> +				return pe;
> +		}
> +	}
> +	kfree(pe);
> +
> +	return NULL;
> +}
> +
> +/* Add/update single/triple vlan entry */
> +static int mvpp2_prs_vlan_add(struct mvpp2 *pp2, unsigned short tpid, int ai,
> +			      unsigned int port_map)
> +{
> +	struct mvpp2_prs_entry *pe;
> +	unsigned int bits, enable;
> +	int tid_aux, tid;
> +
> +	pe = mvpp2_prs_vlan_find(pp2, tpid, ai);
> +
> +	if (!pe) {
> +		/* Create new tcam entry */
> +		tid = mvpp2_prs_tcam_first_free(pp2, MVPP2_PE_LAST_FREE_TID,
> +						MVPP2_PE_FIRST_FREE_TID);
> +		if (tid < 0)
> +			return tid;
> +
> +		pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> +		if (!pe)
> +			return -ENOMEM;
> +
> +		/* Get last double vlan tid */
> +		for (tid_aux = MVPP2_PE_LAST_FREE_TID;
> +		     tid_aux >= MVPP2_PE_FIRST_FREE_TID; tid_aux--) {
> +			if ((!pp2->prs_shadow[tid_aux].valid) ||

Excess parenthesis.

[...]
> +static int mvpp2_prs_double_vlan_ai_free_get(struct mvpp2 *pp2)
> +{
> +	int i;
> +
> +	for (i = 1; i < MVPP2_PRS_DBL_VLANS_MAX; i++)
> +		if (!pp2->prs_double_vlans[i])
> +			return i;

Curly braces.

> +
> +	return -EINVAL;
> +}
> +
> +/* Search for existing double vlan entry */
> +static struct mvpp2_prs_entry *mvpp2_prs_double_vlan_find(struct mvpp2 *pp2,
> +							  unsigned short tpid1,
> +							  unsigned short tpid2)
> +{
> +	struct mvpp2_prs_entry *pe;
> +	unsigned int enable, bits;

Excess scope.

> +	unsigned char tpid_arr1[2];
> +	unsigned char tpid_arr2[2];

unsigned char tpid[2][2] ?

> +	int tid;
> +
> +	tpid_arr1[0] = ((unsigned char *)&tpid1)[1];
> +	tpid_arr1[1] = ((unsigned char *)&tpid1)[0];

Use endian safe shift and mask ?

> +
> +	tpid_arr2[0] = ((unsigned char *)&tpid2)[1];
> +	tpid_arr2[1] = ((unsigned char *)&tpid2)[0];

Duplicated code :o)

> +
> +	pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> +	if (!pe)
> +		return NULL;
> +	mvpp2_prs_tcam_lu_set(pe, MVPP2_PRS_LU_VLAN);
> +
> +	/* Go through the all entries with MVPP2_PRS_LU_VLAN */
> +	for (tid = MVPP2_PE_FIRST_FREE_TID;
> +	     tid <= MVPP2_PE_LAST_FREE_TID; tid++) {
> +		if ((!pp2->prs_shadow[tid].valid) ||
> +		    (pp2->prs_shadow[tid].lu != MVPP2_PRS_LU_VLAN))
> +			continue;
> +
> +		pe->index = tid;
> +		mvpp2_prs_hw_read(pp2, pe);

It should be possible to factor out the test or test + pe->... = ... + hw_read.

> +
> +		if (mvpp2_prs_tcam_data_cmp(pe, 0, 2, tpid_arr1) &&
> +		    mvpp2_prs_tcam_data_cmp(pe, 4, 2, tpid_arr2)) {

The "size" parameter of mvpp2_prs_tcam_data_cmp is always "2".
OTOH, if you use a single 4 bytes array for tpid_arr, you'll be able
to issue a single mvpp2_prs_tcam_data_cmp call.

Off to bed.

-- 
Ueimor

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

[Ezequiel Garcia]

On 08 Jul 12:18 AM, Francois Romieu wrote:
> Ezequiel Garcia <ezequiel.garcia@free-electrons.com> :
> [...]
> > diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
> > new file mode 100644
> > index 0000000..21931a1
> > --- /dev/null
> > +++ b/drivers/net/ethernet/marvell/mvpp2.c
> [...]
> > +static inline void mvpp2_mib_counters_clear(struct mvpp2_port *pp)
> > +{
> > +	int i;
> > +	u32 dummy;
> > +
> > +	/* Perform dummy reads from MIB counters */
> > +	for (i = 0; i < MVPP2_MIB_LATE_COLLISION; i += 4)
> > +		dummy = readl(pp->pp2->lms_base +
> > +			      (MVPP2_MIB_COUNTERS_BASE(pp->id) + i));
>                               ^ excess parenthesis
> 
> Please reduce the scope of the dummy variable and add curly braces in the
> (multi-line) "for" block.
> 
> You may remove the "Perform dummy ..." comment. The code is literate enough.
> 

Actually, the dummy variable is not needed: a plain readl() is enough.

> [...]
> > +/* Update lookup field in tcam sw entry */
> > +static void mvpp2_prs_tcam_lu_set(struct mvpp2_prs_entry *pe, unsigned int lu)
> > +{
> > +	pe->tcam.byte[MVPP2_PRS_TCAM_LU_BYTE] = lu;
> > +	pe->tcam.byte[MVPP2_PRS_TCAM_EN_OFFS(MVPP2_PRS_TCAM_LU_BYTE)] =
> > +							      MVPP2_PRS_LU_MASK;
> 
> Use a local u8 *b = pe->tcam.byte; ?
> 

I think keeping the pe->tcam.byte, but using a local for the
MVPP2_PRS_TCAM_EN_OFFS(MVPP2_PRS_TCAM_LU_BYTE) should be more readable.
I've gone through similar patterns and fixed this.

> > +}
> > +
> > +/* Update mask for single port in tcam sw entry */
> > +static void mvpp2_prs_tcam_port_set(struct mvpp2_prs_entry *pe,
> > +				    unsigned int port, bool add)
> > +{
> > +	if (add)
> > +		pe->tcam.byte[MVPP2_PRS_TCAM_EN_OFFS(MVPP2_PRS_TCAM_PORT_BYTE)]
> > +								&= ~(1 << port);
> > +	else
> > +		pe->tcam.byte[MVPP2_PRS_TCAM_EN_OFFS(MVPP2_PRS_TCAM_PORT_BYTE)]
> > +								 |= (1 << port);
> 
> Use a local u8 *b = pe->tcam.byte; ?
> 

Ditto, using a local for the array should be readable enough.

> [...]
> > +/* Compare tcam data bytes with a pattern */
> > +static bool mvpp2_prs_tcam_data_cmp(struct mvpp2_prs_entry *pe,
> > +				    unsigned int offs, unsigned int size,
> > +					   unsigned char *bytes)
> > +{
> > +	unsigned char byte, mask;
> > +	int i;
> > +
> > +	for (i = 0; i < size; i++) {
> > +		mvpp2_prs_tcam_data_byte_get(pe, offs + i, &byte, &mask);
> > +
> > +		if (byte != bytes[i])
> > +			return false;
> 
> Please reduce the scope of "byte" and "mask".
> 

Agreed. This applies on several other places, I'll fix them all.

> > +	}
> > +	return true;
> > +}
> > +
> > +/* Update ai bits in tcam sw entry */
> > +static void mvpp2_prs_tcam_ai_update(struct mvpp2_prs_entry *pe,
> > +				     unsigned int bits, unsigned int enable)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < MVPP2_PRS_AI_BITS; i++)
> > +		if (enable & BIT(i)) {
> > +			if (bits & BIT(i))
> > +				pe->tcam.byte[MVPP2_PRS_TCAM_AI_BYTE] |=
> > +								       (1 << i);
> > +			else
> > +				pe->tcam.byte[MVPP2_PRS_TCAM_AI_BYTE] &=
> > +								      ~(1 << i);
> > +		}
> 
> Curly braces for the "for" loop + local variable for &pe->tcam.byte[foo]
> 

As you suggest below: I'll fix the comparison to recover the indent, use a local
for the array index, and add curly braces.

> [...]
> > +/* Update ri bits in sram sw entry */
> > +static void mvpp2_prs_sram_ri_update(struct mvpp2_prs_entry *pe,
> > +				     unsigned int bits, unsigned int mask)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < MVPP2_PRS_SRAM_RI_CTRL_BITS; i++)
> 
> Please add curly braces.
> 

Sure.

> > +		if (mask & BIT(i)) {
> 
> Invert the test and add a "continue" statement to recover the missing indent
> level ?
> 

Agreed. The same applies on other places.

[..]
> > +/* Return first free tcam index, seeking from start to end */
> > +static int mvpp2_prs_tcam_first_free(struct mvpp2 *pp2, unsigned char start,
> > +				     unsigned char end)
> > +{
> > +	int tid;
> > +	bool found = false;
> > +
> > +	if (start > end)
> > +		swap(start, end);
> > +
> > +	for (tid = start; tid <= end; tid++) {
> > +		if (!pp2->prs_shadow[tid].valid) {
> > +			found = true;
> > +			break;
> 
> Don't break: test for tid < MVPP2_PRS_TCAM_SRAM_SIZE and return.
> 

Yes, this function can be reworked altogether to trim "end" to the size
and remove the "found" variable.

> > +		}
> > +	}
> > +
> > +	if (found && (tid < MVPP2_PRS_TCAM_SRAM_SIZE))
> > +		return tid;
> > +	return -EINVAL;
> [...]
> > +static void mvpp2_prs_mac_all_multi_set(struct mvpp2 *pp2, int port, bool add)
> > +{
> > +	struct mvpp2_prs_entry pe;
> > +	unsigned int index = 0;
> > +	unsigned int i;
> > +
> > +	/* Ethernet multicast address first byte is
> > +	 * 0x01 for IPv4 and 0x33 for IPv6
> > +	 */
> > +	unsigned char da_mc[MVPP2_PRS_MAX_MAC_MC] = { 0x01, 0x33 };
> > +
> > +	for (i = MVPP2_PRS_IP4_MAC_MC; i < MVPP2_PRS_MAX_MAC_MC; i++) {
> > +		if (i == MVPP2_PRS_IP4_MAC_MC)
> > +			index = MVPP2_PE_MAC_MC_ALL;
> > +		else
> > +			index = MVPP2_PE_MAC_MC_IP6;
> 
> Ternary ?
> 

Sure.

[..]
> > +static struct mvpp2_prs_entry *mvpp2_prs_vlan_find(struct mvpp2 *pp2,
> > +						   unsigned short tpid, int ai)
> > +{
> > +	struct mvpp2_prs_entry *pe;
> > +	unsigned int ri_bits, ai_bits;
> > +	unsigned int enable;
> > +	unsigned char tpid_arr[2];
> > +	int tid;
> 
> Nitnit: please xmas-treefy (wherever possible).
> 

OK.

> > +/* Search for existing double vlan entry */
> > +static struct mvpp2_prs_entry *mvpp2_prs_double_vlan_find(struct mvpp2 *pp2,
> > +							  unsigned short tpid1,
> > +							  unsigned short tpid2)
> > +{
> > +	struct mvpp2_prs_entry *pe;
> > +	unsigned int enable, bits;
> 
> Excess scope.
> 
> > +	unsigned char tpid_arr1[2];
> > +	unsigned char tpid_arr2[2];
> 
> unsigned char tpid[2][2] ?
> 

Yeah, this function needs some love.

So, I have fixed all the excess parenthesis, excess scope, missing curly braces,
added ternary uses and did many other style clean-ups, based on your feedback.

In addition, removed the Tx/Rx module parameters and instead using the default
values (which are fixed by the hardware).

Thanks for such detailed review. I'll submit v4 shortly.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[David Laight]

From: Ezequiel Garcia
...
> > > +/* Compare tcam data bytes with a pattern */
> > > +static bool mvpp2_prs_tcam_data_cmp(struct mvpp2_prs_entry *pe,
> > > +				    unsigned int offs, unsigned int size,
> > > +					   unsigned char *bytes)
> > > +{
> > > +	unsigned char byte, mask;
> > > +	int i;

Hmm. should be 'unsigned int' for the comparison against 'size'.

> > > +
> > > +	for (i = 0; i < size; i++) {
> > > +		mvpp2_prs_tcam_data_byte_get(pe, offs + i, &byte, &mask);
> > > +
> > > +		if (byte != bytes[i])
> > > +			return false;
> >
> > Please reduce the scope of "byte" and "mask".
> >
> 
> Agreed. This applies on several other places, I'll fix them all.
> 
> > > +	}
> > > +	return true;

Not sure about other people, but I prefer variables to be defined
at the top of a function so that I can find them.
Minimising the scope tends to make code harder to read.

If this was a big function, then reducing the scope of 'temporary'
variables need for a few lines (like the above loop) can make sense
because it reduces the clutter at the top of the function.

	David

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

['Ezequiel Garcia']

On 10 Jul 03:26 PM, David Laight wrote:
> From: Ezequiel Garcia
> ...
> > > > +/* Compare tcam data bytes with a pattern */
> > > > +static bool mvpp2_prs_tcam_data_cmp(struct mvpp2_prs_entry *pe,
> > > > +				    unsigned int offs, unsigned int size,
> > > > +					   unsigned char *bytes)
> > > > +{
> > > > +	unsigned char byte, mask;
> > > > +	int i;
> 
> Hmm. should be 'unsigned int' for the comparison against 'size'.
> 

Right. I've reworked this entirely, because it was barely readable,
and now the size parameter is gone.

> > > > +
> > > > +	for (i = 0; i < size; i++) {
> > > > +		mvpp2_prs_tcam_data_byte_get(pe, offs + i, &byte, &mask);
> > > > +
> > > > +		if (byte != bytes[i])
> > > > +			return false;
> > >
> > > Please reduce the scope of "byte" and "mask".
> > >
> > 
> > Agreed. This applies on several other places, I'll fix them all.
> > 
> > > > +	}
> > > > +	return true;
> 
> Not sure about other people, but I prefer variables to be defined
> at the top of a function so that I can find them.
> Minimising the scope tends to make code harder to read.
> 
> If this was a big function, then reducing the scope of 'temporary'
> variables need for a few lines (like the above loop) can make sense
> because it reduces the clutter at the top of the function.
> 

Yeah, that makes sense. I'm fine either way; I'll be submitting v4 now
addressing most of Francois' feedback. Feel free to complain if you think
some function is not readable enough and can be improved.

Thanks for taking a look,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com