Re: [U-Boot] [PATCH 01/11 v2] drivers/net/vsc9953: Cleanup patch

Hi Codrin,

On Tue, Jun 23, 2015 at 11:48 AM, Codrin Ciubotariu codrin.ciubotariu@freescale.com wrote:

This patch groups some macros defined for registers and replaces some magic numbers from vsc9953 with macros. Also, "port" and "port_nr" keywords are replaced with "port_no".

Also, in some places, this patch replaces in_le32 and out_le32 with clrbits_le32 and setbits_le32 to reduce the number of code lines and to assure that only intended bits of a register are changed.

Signed-off-by: Codrin Ciubotariu codrin.ciubotariu@freescale.com

Changes for v2: - removed Change-id field;

drivers/net/vsc9953.c | 100 +++++++++++++++++++++++++------------------------- include/vsc9953.h | 47 ++++++++++++++++++++---- 2 files changed, 88 insertions(+), 59 deletions(-)

diff --git a/drivers/net/vsc9953.c b/drivers/net/vsc9953.c index fed7358..720ae47 100644 --- a/drivers/net/vsc9953.c +++ b/drivers/net/vsc9953.c @@ -25,44 +25,44 @@ static struct vsc9953_info vsc9953_l2sw = { .port[9] = VSC9953_PORT_INFO_INITIALIZER(9), };

-void vsc9953_port_info_set_mdio(int port, struct mii_dev *bus) +void vsc9953_port_info_set_mdio(int port_no, struct mii_dev *bus) {

•   if (!VSC9953_PORT_CHECK(port))
  

•   if (!VSC9953_PORT_CHECK(port_no))
            return;
  
  

•   vsc9953_l2sw.port[port].bus = bus;
  

•   vsc9953_l2sw.port[port_no].bus = bus;
  

}

-void vsc9953_port_info_set_phy_address(int port, int address) +void vsc9953_port_info_set_phy_address(int port_no, int address) {

•   if (!VSC9953_PORT_CHECK(port))
  

•   if (!VSC9953_PORT_CHECK(port_no))
            return;
  
  

•   vsc9953_l2sw.port[port].phyaddr = address;
  

•   vsc9953_l2sw.port[port_no].phyaddr = address;
  

}

-void vsc9953_port_info_set_phy_int(int port, phy_interface_t phy_int) +void vsc9953_port_info_set_phy_int(int port_no, phy_interface_t phy_int) {

•   if (!VSC9953_PORT_CHECK(port))
  

•   if (!VSC9953_PORT_CHECK(port_no))
            return;
  
  

•   vsc9953_l2sw.port[port].enet_if = phy_int;
  

•   vsc9953_l2sw.port[port_no].enet_if = phy_int;
  

}

-void vsc9953_port_enable(int port) +void vsc9953_port_enable(int port_no) {

•   if (!VSC9953_PORT_CHECK(port))
  

•   if (!VSC9953_PORT_CHECK(port_no))
            return;
  
  

•   vsc9953_l2sw.port[port].enabled = 1;
  

•   vsc9953_l2sw.port[port_no].enabled = 1;
  

}

-void vsc9953_port_disable(int port) +void vsc9953_port_disable(int port_no) {

•   if (!VSC9953_PORT_CHECK(port))
  

•   if (!VSC9953_PORT_CHECK(port_no))
            return;
  
  

•   vsc9953_l2sw.port[port].enabled = 0;
  

•   vsc9953_l2sw.port[port_no].enabled = 0;
  

}

static void vsc9953_mdio_write(struct vsc9953_mii_mng *phyregs, int port_addr, @@ -148,21 +148,21 @@ static int init_phy(struct eth_device *dev) return 0; }

-static int vsc9953_port_init(int port) +static int vsc9953_port_init(int port_no) { struct eth_device *dev;

    /* Internal ports never have a PHY */

•   if (VSC9953_INTERNAL_PORT_CHECK(port))
  

•   if (VSC9953_INTERNAL_PORT_CHECK(port_no))
            return 0;
  
    /* alloc eth device */
    dev = (struct eth_device *)calloc(1, sizeof(struct eth_device));
    if (!dev)
  

•           return 1;
  

•           return -1;
  

Is it reasonable to use values from asm/errno.h here and elsewhere in the driver? This seems like it should return -ENODEV instead of -EPERM.

•   sprintf(dev->name, "SW@PORT%d", port);
  

•   dev->priv = &vsc9953_l2sw.port[port];
  

•   sprintf(dev->name, "SW@PORT%d", port_no);
  

•   dev->priv = &vsc9953_l2sw.port[port_no];
    dev->init = NULL;
    dev->halt = NULL;
    dev->send = NULL;
  

@@ -170,7 +170,7 @@ static int vsc9953_port_init(int port)

    if (init_phy(dev)) {
            free(dev);

•           return 1;
  

•           return -1;
    }
  
    return 0;
  

@@ -255,8 +255,8 @@ void vsc9953_init(bd_t *bis) out_le32(&l2dev_gmii_reg->mac_cfg_status.mac_hdx_cfg, hdx_cfg); out_le32(&l2sys_reg->sys.front_port_mode[i], CONFIG_VSC9953_FRONT_PORT_MODE);

•           out_le32(&l2qsys_reg->sys.switch_port_mode[i],
  

•                    CONFIG_VSC9953_PORT_ENA);
  

•           setbits_le32(&l2qsys_reg->sys.switch_port_mode[i],
  

•                        CONFIG_VSC9953_PORT_ENA);
            out_le32(&l2dev_gmii_reg->mac_cfg_status.mac_maxlen_cfg,
                     CONFIG_VSC9953_MAC_MAX_LEN);
            out_le32(&l2sys_reg->pause_cfg.pause_cfg[i],
  

@@ -312,25 +312,23 @@ void vsc9953_init(bd_t *bis)

#ifdef CONFIG_VSC9953_CMD /* Enable/disable status of a VSC9953 port */ -static void vsc9953_port_status_set(int port_nr, u8 enabled) +static void vsc9953_port_status_set(int port_no, u8 enabled) {

•   u32                     val;
    struct vsc9953_qsys_reg *l2qsys_reg;
  
    /* Administrative down */
  

•   if (vsc9953_l2sw.port[port_nr].enabled == 0)
  

•   if (!vsc9953_l2sw.port[port_no].enabled)
            return;
  
    l2qsys_reg = (struct vsc9953_qsys_reg *)(VSC9953_OFFSET +
                    VSC9953_QSYS_OFFSET);
  
  

•   val = in_le32(&l2qsys_reg->sys.switch_port_mode[port_nr]);
  

•   if (enabled == 1)
  

•           val |= (1 << 13);
  

•   if (enabled)
  

•           setbits_le32(&l2qsys_reg->sys.switch_port_mode[port_no],
  

•                        CONFIG_VSC9953_PORT_ENA);
    else
  

•           val &= ~(1 << 13);
  

•   out_le32(&l2qsys_reg->sys.switch_port_mode[port_nr], val);
  

•           clrbits_le32(&l2qsys_reg->sys.switch_port_mode[port_no],
  

•                        CONFIG_VSC9953_PORT_ENA);
  

}

/* Set all VSC9953 ports' status */ @@ -343,14 +341,14 @@ static void vsc9953_port_all_status_set(u8 enabled) }

/* Start autonegotiation for a VSC9953 PHY */ -static void vsc9953_phy_autoneg(int port_nr) +static void vsc9953_phy_autoneg(int port_no) {

•   if (!vsc9953_l2sw.port[port_nr].phydev)
  

•   if (!vsc9953_l2sw.port[port_no].phydev)
            return;
  
  

•   if (vsc9953_l2sw.port[port_nr].phydev->drv->startup(
  

•                   vsc9953_l2sw.port[port_nr].phydev))
  

•           printf("Failed to start PHY for port %d\n", port_nr);
  

•   if (vsc9953_l2sw.port[port_no].phydev->drv->startup(
  

•                   vsc9953_l2sw.port[port_no].phydev))
  

•           printf("Failed to start PHY for port %d\n", port_no);
  

}

/* Start autonegotiation for all VSC9953 PHYs */ @@ -363,7 +361,7 @@ static void vsc9953_phy_all_autoneg(void) }

/* Print a VSC9953 port's configuration */ -static void vsc9953_port_config_show(int port) +static void vsc9953_port_config_show(int port_no) { int speed; int duplex; @@ -375,20 +373,20 @@ static void vsc9953_port_config_show(int port) l2qsys_reg = (struct vsc9953_qsys_reg *)(VSC9953_OFFSET + VSC9953_QSYS_OFFSET);

•   val = in_le32(&l2qsys_reg->sys.switch_port_mode[port]);
  

•   enabled = vsc9953_l2sw.port[port].enabled &
  

•                   ((val & 0x00002000) >> 13);
  

•   val = in_le32(&l2qsys_reg->sys.switch_port_mode[port_no]);
  

•   enabled = !!(vsc9953_l2sw.port[port_no].enabled &
  

•                val & CONFIG_VSC9953_PORT_ENA);
  

This is incorrect... Should be:

+ enabled = vsc9953_l2sw.port[port_no].enabled && + (val & CONFIG_VSC9953_PORT_ENA);

    /* internal ports (8 and 9) are fixed */

•   if (VSC9953_INTERNAL_PORT_CHECK(port)) {
  

•   if (VSC9953_INTERNAL_PORT_CHECK(port_no)) {
            link = 1;
            speed = SPEED_2500;
            duplex = DUPLEX_FULL;
    } else {
  

•           if (vsc9953_l2sw.port[port].phydev) {
  

•                   link = vsc9953_l2sw.port[port].phydev->link;
  

•                   speed = vsc9953_l2sw.port[port].phydev->speed;
  

•                   duplex = vsc9953_l2sw.port[port].phydev->duplex;
  

•           if (vsc9953_l2sw.port[port_no].phydev) {
  

•                   link = vsc9953_l2sw.port[port_no].phydev->link;
  

•                   speed = vsc9953_l2sw.port[port_no].phydev->speed;
  

•                   duplex = vsc9953_l2sw.port[port_no].phydev->duplex;
            } else {
                    link = -1;
                    speed = -1;
  

@@ -396,7 +394,7 @@ static void vsc9953_port_config_show(int port) } }

•   printf("%8d ", port);
  

•   printf("%8d ", port_no);
    printf("%8s ", enabled == 1 ? "enabled" : "disabled");
    printf("%8s ", link == 1 ? "up" : "down");
  
  

@@ -487,11 +485,11 @@ static int do_ethsw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

U_BOOT_CMD(ethsw, 5, 0, do_ethsw, "vsc9953 l2 switch commands",

•      "port <port_nr> enable|disable\n"
  

•      "port <port_no> enable|disable\n"
       "    - enable/disable an l2 switch port\n"
  

•      "      port_nr=0..9; use \"all\" for all ports\n"
  

•      "ethsw port <port_nr> show\n"
  

•      "      port_no=0..9; use \"all\" for all ports\n"
  

•      "ethsw port <port_no> show\n"
       "    - show an l2 switch port's configuration\n"
  

•      "      port_nr=0..9; use \"all\" for all ports\n"
  

•      "      port_no=0..9; use \"all\" for all ports\n"
  

); #endif /* CONFIG_VSC9953_CMD */ diff --git a/include/vsc9953.h b/include/vsc9953.h index 3d11b87..920402f 100644 --- a/include/vsc9953.h +++ b/include/vsc9953.h @@ -33,29 +33,60 @@ #define T1040_SWITCH_GMII_DEV_OFFSET 0x010000 #define VSC9953_PHY_REGS_OFFST 0x0000AC

+/* Macros for vsc9953_chip_regs.soft_rst register */ #define CONFIG_VSC9953_SOFT_SWC_RST_ENA 0x00000001

All of there that are register constants should not have "CONFIG_" prepended to them. That should only be for constants that configure something, eventually only from Kconfig. Please add another patch before this one that removes that.

+/* Macros for vsc9953_sys_sys.reset_cfg register */ #define CONFIG_VSC9953_CORE_ENABLE 0x80 #define CONFIG_VSC9953_MEM_ENABLE 0x40 #define CONFIG_VSC9953_MEM_INIT 0x20

-#define CONFIG_VSC9953_PORT_ENA 0x00003a00

Why is this value changing? Was it just wrong before?

+/* Macros for vsc9953_dev_gmii_mac_cfg_status.mac_ena_cfg register */ #define CONFIG_VSC9953_MAC_ENA_CFG 0x00000011

+/* Macros for vsc9953_dev_gmii_mac_cfg_status.mac_mode_cfg register */ #define CONFIG_VSC9953_MAC_MODE_CFG 0x00000011

+/* Macros for vsc9953_dev_gmii_mac_cfg_status.mac_ifg_cfg register */ #define CONFIG_VSC9953_MAC_IFG_CFG 0x00000515

+/* Macros for vsc9953_dev_gmii_mac_cfg_status.mac_hdx_cfg register */ #define CONFIG_VSC9953_MAC_HDX_CFG 0x00001043

+/* Macros for vsc9953_dev_gmii_mac_cfg_status.mac_maxlen_cfg register */ +#define CONFIG_VSC9953_MAC_MAX_LEN 0x000005ee

+/* Macros for vsc9953_dev_gmii_port_mode.clock_cfg register */ #define CONFIG_VSC9953_CLOCK_CFG 0x00000001 #define CONFIG_VSC9953_CLOCK_CFG_1000M 0x00000001

+/* Macros for vsc9953_sys_sys.front_port_mode register */ +#define CONFIG_VSC9953_FRONT_PORT_MODE 0x00000000

+/* Macros for vsc9953_ana_pfc.pfc_cfg register */ #define CONFIG_VSC9953_PFC_FC 0x00000001 #define CONFIG_VSC9953_PFC_FC_QSGMII 0x00000000

+/* Macros for vsc9953_sys_pause_cfg.mac_fc_cfg register */ #define CONFIG_VSC9953_MAC_FC_CFG 0x04700000 #define CONFIG_VSC9953_MAC_FC_CFG_QSGMII 0x00700000

+/* Macros for vsc9953_sys_pause_cfg.pause_cfg register */ #define CONFIG_VSC9953_PAUSE_CFG 0x001ffffe

+/* Macros for vsc9953_sys_pause_cfg.pause_cfg register */ +#define CONFIG_VSC9953_PAUSE_CFG 0x001ffffe

This adds a duplicate of the define above it.

+/* Macros for vsc9953_sys_pause_cfgtot_tail_drop_lvl register */ #define CONFIG_VSC9953_TOT_TAIL_DROP_LVL 0x000003ff -#define CONFIG_VSC9953_FRONT_PORT_MODE 0x00000000 -#define CONFIG_VSC9953_MAC_MAX_LEN 0x000005ee

+/* Macros for vsc9953_vcap_core_cfg.vcap_mv_cfg register */ #define CONFIG_VSC9953_VCAP_MV_CFG 0x0000ffff #define CONFIG_VSC9953_VCAP_UPDATE_CTRL 0x01000004

May as well get rid of the tabs here after the defines.

+/* Macros for vsc9953_qsys_sys.switch_port_mode register */ +#define CONFIG_VSC9953_PORT_ENA 0x00002000

#define VSC9953_MAX_PORTS 10 #define VSC9953_PORT_CHECK(port) \ (((port) < 0 || (port) >= VSC9953_MAX_PORTS) ? 0 : 1) @@ -393,10 +424,10 @@ struct vsc9953_info {

void vsc9953_init(bd_t *bis);

-void vsc9953_port_info_set_mdio(int port, struct mii_dev *bus); -void vsc9953_port_info_set_phy_address(int port, int address); -void vsc9953_port_enable(int port); -void vsc9953_port_disable(int port); -void vsc9953_port_info_set_phy_int(int port, phy_interface_t phy_int); +void vsc9953_port_info_set_mdio(int port_no, struct mii_dev *bus); +void vsc9953_port_info_set_phy_address(int port_no, int address); +void vsc9953_port_enable(int port_no); +void vsc9953_port_disable(int port_no); +void vsc9953_port_info_set_phy_int(int port_no, phy_interface_t phy_int);

#endif /* _VSC9953_H_ */

1.9.3

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot