Re: [PATCH net-next 2/2] net: broadcom: share header defining UniMAC registers

On 1/6/21 12:37 PM, Rafał Miłecki wrote:

On 06.01.2021 20:26, Florian Fainelli wrote:

quoted

On 1/5/21 11:32 PM, Rafał Miłecki wrote:

quoted

From: Rafał Miłecki <rafal@milecki.pl>

UniMAC is integrated into multiple Broadcom's Ethernet controllers so
use a shared header file for it and avoid some code duplication.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
  MAINTAINERS                                   |  2 +

Don't you need to update the BGMAC section to also list unimac.h since
it is a shared header now? This looks good to me, the conversion does
produce the following warnings on x86-64 (and probably arm64, too):

drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_set_rx_mode':
drivers/net/ethernet/broadcom/bgmac.c:788:33: warning: conversion from
'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
'18446744073709551599' to '4294967279' [-Woverflow]
   788 |   bgmac_umac_cmd_maskset(bgmac, ~CMD_PROMISC, 0, true);
drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_mac_speed':
drivers/net/ethernet/broadcom/bgmac.c:828:13: warning: conversion from
'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
'18446744073709550579' to '4294966259' [-Woverflow]
   828 |  u32 mask = ~(CMD_SPEED_MASK << CMD_SPEED_SHIFT | CMD_HD_EN);
       |             ^
drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_chip_reset':
drivers/net/ethernet/broadcom/bgmac.c:999:11: warning: conversion from
'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
'18446744073197811804' to '3783227484' [-Woverflow]
   999 |           ~(CMD_TX_EN |
       |           ^~~~~~~~~~~~~
  1000 |      CMD_RX_EN |
       |      ~~~~~~~~~~~
  1001 |      CMD_RX_PAUSE_IGNORE |
       |      ~~~~~~~~~~~~~~~~~~~~~
  1002 |      CMD_TX_ADDR_INS |
       |      ~~~~~~~~~~~~~~~~~
  1003 |      CMD_HD_EN |
       |      ~~~~~~~~~~~
  1004 |      CMD_LCL_LOOP_EN |
       |      ~~~~~~~~~~~~~~~~~
  1005 |      CMD_CNTL_FRM_EN |
       |      ~~~~~~~~~~~~~~~~~
  1006 |      CMD_RMT_LOOP_EN |
       |      ~~~~~~~~~~~~~~~~~
  1007 |      CMD_RX_ERR_DISC |
       |      ~~~~~~~~~~~~~~~~~
  1008 |      CMD_PRBL_EN |
       |      ~~~~~~~~~~~~~
  1009 |      CMD_TX_PAUSE_IGNORE |
       |      ~~~~~~~~~~~~~~~~~~~~~
  1010 |      CMD_PAD_EN |
       |      ~~~~~~~~~~~~
  1011 |      CMD_PAUSE_FWD),
       |      ~~~~~~~~~~~~~~
drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_enable':
drivers/net/ethernet/broadcom/bgmac.c:1057:32: warning: conversion from
'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
'18446744073709551612' to '4294967292' [-Woverflow]
  1057 |  bgmac_umac_cmd_maskset(bgmac, ~(CMD_TX_EN | CMD_RX_EN),
       |                                ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_chip_init':
drivers/net/ethernet/broadcom/bgmac.c:1108:32: warning: conversion from
'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
'18446744073709551359' to '4294967039' [-Woverflow]
  1108 |  bgmac_umac_cmd_maskset(bgmac, ~CMD_RX_PAUSE_IGNORE, 0, true);
drivers/net/ethernet/broadcom/bgmac.c:1117:33: warning: conversion from
'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
'18446744073709518847' to '4294934527' [-Woverflow]
  1117 |   bgmac_umac_cmd_maskset(bgmac, ~CMD_LCL_LOOP_EN, 0, false);

I can reproduce that after switching from mips to arm64. Before this
change bgmac.h was not using BIT() macro. Now it does and that macro
forces UL (unsigned long).

Is there any cleaner solution than below one?

Don't use BIT(), if the constants are 32-bit unsigned integer, maybe
open coding them as (1 << x) is acceptable for that purpose.
-- 
Florian