diff mbox

[PATCHv2,net-next,09/11] net: mvpp2: simplify MVPP2_PRS_RI_* definitions

Message ID 1482943567-12483-10-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Petazzoni Dec. 28, 2016, 4:46 p.m. UTC
Some of the MVPP2_PRS_RI_* definitions use the ~(value) syntax, which
doesn't compile nicely on 64-bit. Moreover, those definitions are in
fact unneeded, since they are always used in combination with a bit
mask that ensures only the appropriate bits are modified.

Therefore, such definitions should just be set to 0x0. For example:

 #define MVPP2_PRS_RI_L2_CAST_MASK              0x600
 #define MVPP2_PRS_RI_L2_UCAST                  ~(BIT(9) | BIT(10))
 #define MVPP2_PRS_RI_L2_MCAST                  BIT(9)
 #define MVPP2_PRS_RI_L2_BCAST                  BIT(10)

becomes

 #define MVPP2_PRS_RI_L2_CAST_MASK              0x600
 #define MVPP2_PRS_RI_L2_UCAST                  0x0
 #define MVPP2_PRS_RI_L2_MCAST                  BIT(9)
 #define MVPP2_PRS_RI_L2_BCAST                  BIT(10)

Because the values (MVPP2_PRS_RI_L2_UCAST, MVPP2_PRS_RI_L2_MCAST and
MVPP2_PRS_RI_L2_BCAST) are always applied with
MVPP2_PRS_RI_L2_CAST_MASK, and therefore there is no need for
MVPP2_PRS_RI_L2_UCAST to be defined as ~(BIT(9) | BIT(10)).

It fixes the following warnings when building the driver on a 64-bit
platform (which is not possible as of this commit, but will be enabled
in a follow-up commit):

drivers/net/ethernet/marvell/mvpp2.c: In function ‘mvpp2_prs_mac_promisc_set’:
drivers/net/ethernet/marvell/mvpp2.c:524:33: warning: large integer implicitly truncated to unsigned type [-Woverflow]
 #define MVPP2_PRS_RI_L2_UCAST   ~(BIT(9) | BIT(10))
                                 ^
drivers/net/ethernet/marvell/mvpp2.c:1459:33: note: in expansion of macro ‘MVPP2_PRS_RI_L2_UCAST’
   mvpp2_prs_sram_ri_update(&pe, MVPP2_PRS_RI_L2_UCAST,

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvpp2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Russell King (Oracle) Jan. 6, 2017, 1:07 p.m. UTC | #1
On Wed, Dec 28, 2016 at 05:46:05PM +0100, Thomas Petazzoni wrote:
> Some of the MVPP2_PRS_RI_* definitions use the ~(value) syntax, which
> doesn't compile nicely on 64-bit. Moreover, those definitions are in
> fact unneeded, since they are always used in combination with a bit
> mask that ensures only the appropriate bits are modified.
> 
> Therefore, such definitions should just be set to 0x0. For example:
> 
>  #define MVPP2_PRS_RI_L2_CAST_MASK              0x600
>  #define MVPP2_PRS_RI_L2_UCAST                  ~(BIT(9) | BIT(10))
>  #define MVPP2_PRS_RI_L2_MCAST                  BIT(9)
>  #define MVPP2_PRS_RI_L2_BCAST                  BIT(10)
> 
> becomes
> 
>  #define MVPP2_PRS_RI_L2_CAST_MASK              0x600
>  #define MVPP2_PRS_RI_L2_UCAST                  0x0
>  #define MVPP2_PRS_RI_L2_MCAST                  BIT(9)
>  #define MVPP2_PRS_RI_L2_BCAST                  BIT(10)

So this is a two-bit field in a register with three defined states - I'm
not sure that using BIT() here is really a good idea.  BIT() is fine for
single-bit controls, but I think it adds an additional level of confusion
for multi-bit controls.

Also, the combination of the mask being defined as hex and the controls
using BIT() is particularly not nice.  I think either use one style or
the other, don't mix them.  So either:

  #define MVPP2_PRS_RI_L2_CAST_MASK              0x600
  #define MVPP2_PRS_RI_L2_UCAST                  0x000
  #define MVPP2_PRS_RI_L2_MCAST                  0x200
  #define MVPP2_PRS_RI_L2_BCAST                  0x400

or:

  #define MVPP2_PRS_RI_L2_CAST_MASK              (BIT(10) | BIT(9))
  #define MVPP2_PRS_RI_L2_UCAST                  0
  #define MVPP2_PRS_RI_L2_MCAST                  BIT(9)
  #define MVPP2_PRS_RI_L2_BCAST                  BIT(10)

It then becomes obvious that the mask and the settings are changing the
same bits.
Thomas Petazzoni Feb. 2, 2017, 4:11 p.m. UTC | #2
Hello,

On Fri, 6 Jan 2017 13:07:27 +0000, Russell King - ARM Linux wrote:

>   #define MVPP2_PRS_RI_L2_CAST_MASK              (BIT(10) | BIT(9))
>   #define MVPP2_PRS_RI_L2_UCAST                  0
>   #define MVPP2_PRS_RI_L2_MCAST                  BIT(9)
>   #define MVPP2_PRS_RI_L2_BCAST                  BIT(10)

Your comment makes perfect sense, so I've adopted this second option
you suggested. This is in my v3 I sent a few minutes ago.

Thanks!

Thomas
diff mbox

Patch

diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
index 36c73dc..58cf9af 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -511,19 +511,19 @@  enum mvpp2_tag_type {
 #define MVPP2_PRS_RI_MAC_ME_MASK		0x1
 #define MVPP2_PRS_RI_DSA_MASK			0x2
 #define MVPP2_PRS_RI_VLAN_MASK			0xc
-#define MVPP2_PRS_RI_VLAN_NONE			~(BIT(2) | BIT(3))
+#define MVPP2_PRS_RI_VLAN_NONE			0x0
 #define MVPP2_PRS_RI_VLAN_SINGLE		BIT(2)
 #define MVPP2_PRS_RI_VLAN_DOUBLE		BIT(3)
 #define MVPP2_PRS_RI_VLAN_TRIPLE		(BIT(2) | BIT(3))
 #define MVPP2_PRS_RI_CPU_CODE_MASK		0x70
 #define MVPP2_PRS_RI_CPU_CODE_RX_SPEC		BIT(4)
 #define MVPP2_PRS_RI_L2_CAST_MASK		0x600
-#define MVPP2_PRS_RI_L2_UCAST			~(BIT(9) | BIT(10))
+#define MVPP2_PRS_RI_L2_UCAST			0x0
 #define MVPP2_PRS_RI_L2_MCAST			BIT(9)
 #define MVPP2_PRS_RI_L2_BCAST			BIT(10)
 #define MVPP2_PRS_RI_PPPOE_MASK			0x800
 #define MVPP2_PRS_RI_L3_PROTO_MASK		0x7000
-#define MVPP2_PRS_RI_L3_UN			~(BIT(12) | BIT(13) | BIT(14))
+#define MVPP2_PRS_RI_L3_UN			0x0
 #define MVPP2_PRS_RI_L3_IP4			BIT(12)
 #define MVPP2_PRS_RI_L3_IP4_OPT			BIT(13)
 #define MVPP2_PRS_RI_L3_IP4_OTHER		(BIT(12) | BIT(13))
@@ -531,7 +531,7 @@  enum mvpp2_tag_type {
 #define MVPP2_PRS_RI_L3_IP6_EXT			(BIT(12) | BIT(14))
 #define MVPP2_PRS_RI_L3_ARP			(BIT(13) | BIT(14))
 #define MVPP2_PRS_RI_L3_ADDR_MASK		0x18000
-#define MVPP2_PRS_RI_L3_UCAST			~(BIT(15) | BIT(16))
+#define MVPP2_PRS_RI_L3_UCAST			0x0
 #define MVPP2_PRS_RI_L3_MCAST			BIT(15)
 #define MVPP2_PRS_RI_L3_BCAST			(BIT(15) | BIT(16))
 #define MVPP2_PRS_RI_IP_FRAG_MASK		0x20000