Hi Codrin,
On Tue, Jun 23, 2015 at 11:48 AM, Codrin Ciubotariu
<codrin.ciubota...@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.ciubota...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
