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

Francois Romieu romieu at fr.zoreil.com
Mon Jul 7 15:18:23 PDT 2014


Ezequiel Garcia <ezequiel.garcia at 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



More information about the linux-arm-kernel mailing list