| Message ID | 1435078136-22809-2-git-send-email-codrin.ciubotariu@freescale.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
Hi Joe, > > > > /* 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. Yes, I should use values from errno.h . -ENOMEM seems more appropriate to me. What do you think? > > + 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); Ok. > > 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. Ok, I will add another patch before this one. > > +/* 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. I will remove one of them. > > > > +/* 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. Ok. Thank you for your review. I will make v3. Best regards, Codrin
Hi Codrin, On Thu, Jun 25, 2015 at 11:35 AM, Codrin Constantin Ciubotariu <codrin.ciubotariu@freescale.com> wrote: > Hi Joe, > >> > >> > /* 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. > > Yes, I should use values from errno.h . -ENOMEM seems more appropriate to me. What do you think? Yes, sorry. I didn't look at the line above! :/ >> > + 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); > > Ok. > >> > 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. > > Ok, I will add another patch before this one. > >> > +/* 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. > > I will remove one of them. > >> > >> > +/* 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. > > Ok. > > Thank you for your review. I will make v3. Sure thing. I'll try to get through the rest of them today. Cheers, -Joe
Hi Joe, > > -#define CONFIG_VSC9953_PORT_ENA 0x00003a00 > > Why is this value changing? Was it just wrong before? Sorry, I missed this comment. Yes, the correct value that only enables the port is 0x00002000. The rest of the bits are set to 1 because they are reserved and default values. Since this patch changes the way l2qsys_reg->sys.switch_port_mode[i] is set (out_le32() replaced by setbits_le32()), the other bits are no longer touched when enabling the port: - 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) Should I make a different patch for this change? Best regards, Codrin
Hi Codrin, On Thu, Jul 2, 2015 at 9:32 AM, Codrin Constantin Ciubotariu <codrin.ciubotariu@freescale.com> wrote: > Hi Joe, > >> > -#define CONFIG_VSC9953_PORT_ENA 0x00003a00 >> >> Why is this value changing? Was it just wrong before? > Sorry, I missed this comment. Yes, the correct value that only enables the port is 0x00002000. The rest of the bits are set to 1 because they are reserved and default values. Since this patch changes the way l2qsys_reg->sys.switch_port_mode[i] is set (out_le32() replaced by setbits_le32()), the other bits are no longer touched when enabling the port: OK. This is (obviously) unclear as is. > - 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) > > Should I make a different patch for this change? For sure. Include the comment you made above in the log. Thanks, -Joe
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; - 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); /* 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 + +/* 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 +/* 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 + +/* 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 + +/* 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_ */
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(-)