[U-Boot] [PATCH 1/1] Adding MSCC PHY-VSC8530-VSC8531-VSC8540-VSC8541
Alexey Brodkin
Alexey.Brodkin at synopsys.com
Thu Dec 8 13:24:22 CET 2016
Hi John,
First of all I'm wondering if there was a real need to add that many people in Cc?
Normally sybsystem maintainer and active contributors to code you modified are
good options here.
Then have you run your patch through checkpatch?
Obviously it was not the case because it says me:
-------------------------->8---------------------
total: 570 errors, 251 warnings, 0 checks, 1170 lines checked
-------------------------->8---------------------
Please fix all that first. That's way too many errors for such a small patch!
Take a look at this article - http://www.denx.de/wiki/view/U-Boot/Patches.
Still some comments below.
On Wed, 2016-12-07 at 17:11 +0000, John Haechten wrote:
> From 127c5fb9ef390cad5cf58e446110a696cf111345 Mon Sep 17 00:00:00 2001
> From: John Haechten <john.haechten at microsemi.com>
> Date: Wed, 7 Dec 2016 07:51:37 -0800
> Subject: [PATCH] Adding MSCC PHY-VSC8530-VSC8531-VSC8540-VSC8541
Commit title is not very readable.
Remember this title is what people will see later on while looking at git log.
So maybe something like "net/phy: Add Microsemi vsc85xx PHYs"?
> (C) Copyright 2016 Microsemi Corporation, MIT License (MIT)
Why do you need that in commit message?
> Author:John Haechten <john.haechten at microsemi.com>
Shouldn't it be just "Signed-off-by" instead (already existed below)?
> Reviewed-by:Howard Hicks <howard.hicks at microsemi.com>
> Reviewed-by:Joe Hershberger <joe.hershberger at ni.com>
> Tested-by:Howard Hicks <howard.hicks at microsemi.com>
> Version:1
What's this?
> Prefix:None
Ditto.
> Signed-off-by: John Haechten <john.haechten at microsemi.com>
>
> ---
>
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/mscc.c | 480 ++++++++++++++++++++++++++++++++++++
> drivers/net/phy/phy.c | 3 +
> include/config_phylib_all_drivers.h | 1 +
> include/configs/am335x_evm.h | 3 +
> include/phy.h | 1 +
> 6 files changed, 489 insertions(+)
> create mode 100644 drivers/net/phy/mscc.c
>
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 1e299b9..d372971 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -27,3 +27,4 @@ obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
> obj-$(CONFIG_PHY_TI) += ti.o
> obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
> obj-$(CONFIG_PHY_VITESSE) += vitesse.o
> +obj-$(CONFIG_PHY_MSCC) += mscc.o
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> new file mode 100644
> index 0000000..e63bd43
> --- /dev/null
> +++ b/drivers/net/phy/mscc.c
> @@ -0,0 +1,480 @@
> +/*
> + * Driver for Microsemi VSC85xx PHYs
> + *
> + * Author: John Haechten
> + *
> + * The MIT License (MIT)
> + *
> + * Copyright (c) 2016 Microsemi Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
Please check http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign
> + */
> +
> +#include <miiphy.h>
> +#include <bitfield.h>
Maybe swap includes so they are sorted alphabetically or this order is really required?
> +
> +/* Note: To include this PHY Driver file, #define CONFIG_PHY_MSCC */
IMHO that note should be removed. Even though it might not be super obvious but that's
how legacy configuration is done in U-Boot. Moreover I would strongly recommend to use Kconfig
stuff for new drivers instead of legacy approach with includes.
> +/* Microsemi PHY ID's */
> +#define PHY_ID_VSC8530 0x00070560
> +#define PHY_ID_VSC8531 0x00070570
> +#define PHY_ID_VSC8540 0x00070760
> +#define PHY_ID_VSC8541 0x00070770
Maybe add a newline here?
> +/* Microsemi VSC85xx PHY Register Pages */
> +#define MSCC_EXT_PAGE_ACCESS 31 /* Page Access Register */
> +#define MSCC_PHY_PAGE_STD 0x0000 /* Standard registers */
> +#define MSCC_PHY_PAGE_EXT1 0x0001 /* Extended registers - page 1 */
> +#define MSCC_PHY_PAGE_EXT2 0x0002 /* Extended registers - page 2 */
> +#define MSCC_PHY_PAGE_EXT3 0x0003 /* Extended registers - page 3 */
> +#define MSCC_PHY_PAGE_EXT4 0x0004 /* Extended registers - page 4 */
> +#define MSCC_PHY_PAGE_GPIO 0x0010 /* GPIO registers */
> +#define MSCC_PHY_PAGE_TEST 0x2A30 /* TEST Page registers */
> +#define MSCC_PHY_PAGE_TR 0x52B5 /* Token Ring Page registers */
Ditto.
> +/* Std Page Register 28 - PHY AUX Control/Status */
> +#define MIIM_AUX_CNTRL_STAT_REG 0x1c
> +#define MIIM_AUX_CNTRL_STAT_ACTIPHY_TO 0x0004
> +#define MIIM_AUX_CNTRL_STAT_F_DUPLEX 0x0020
> +#define MIIM_AUX_CNTRL_STAT_SPEED_MASK 0x0018
> +#define MIIM_AUX_CNTRL_STAT_SPEED_POS (3)
> +#define MIIM_AUX_CNTRL_STAT_SPEED_10M (0x0)
> +#define MIIM_AUX_CNTRL_STAT_SPEED_100M (0x1)
> +#define MIIM_AUX_CNTRL_STAT_SPEED_1000M (0x2)
> +/* Std Page Register 23 - Extended PHY CTRL_1 */
> +#define MSCC_PHY_EXT_PHY_CNTL_1 23
> +#define MAC_IF_SELECTION_MASK 0x1800
> +#define MAC_IF_SELECTION_GMII 0
> +#define MAC_IF_SELECTION_RMII 1
> +#define MAC_IF_SELECTION_RGMII 2
> +#define MAC_IF_SELECTION_POS 11
> +#define MAC_IF_SELECTION_WIDTH 2
> +/* Extended Page 2 Register 20E2 */
> +#define MSCC_PHY_RGMII_CNTL 20
> +#define VSC_FAST_LINK_FAIL2_ENA_MASK 0x8000
> +#define RX_CLK_OUT_MASK 0x0800
> +#define RX_CLK_OUT_POS 11
> +#define RX_CLK_OUT_WIDTH 1
> +#define RX_CLK_OUT_NORMAL 0
> +#define RX_CLK_OUT_DISABLE 1
> +#define RGMII_RX_CLK_DELAY_POS 4
> +#define RGMII_RX_CLK_DELAY_WIDTH 3
> +#define RGMII_RX_CLK_DELAY_MASK 0x0070
> +#define RGMII_TX_CLK_DELAY_POS 0
> +#define RGMII_TX_CLK_DELAY_WIDTH 3
> +#define RGMII_TX_CLK_DELAY_MASK 0x0007
> +/* Extended Page 2 Register 27E2 */
> +#define MSCC_PHY_WOL_MAC_CONTROL 27
> +#define EDGE_RATE_CNTL_POS 5
> +#define EDGE_RATE_CNTL_WIDTH 3
> +#define EDGE_RATE_CNTL_MASK 0x00E0
> +#define RMII_CLK_OUT_ENABLE_POS 4
> +#define RMII_CLK_OUT_ENABLE_WIDTH 1
> +#define RMII_CLK_OUT_ENABLE_MASK 0x10
> +/* Token Ring Page 0x52B5 Registers */
> +#define MSCC_PHY_REG_TR_ADDR_16 16
> +#define MSCC_PHY_REG_TR_DATA_17 17
> +#define MSCC_PHY_REG_TR_DATA_18 18
> +/* Token Ring Registers */
> +#define MSCC_PHY_TR_LINKDETCTRL_POS 3
> +#define MSCC_PHY_TR_LINKDETCTRL_WIDTH 2
> +#define MSCC_PHY_TR_LINKDETCTRL_MASK 0x18
> +#define MSCC_PHY_TR_VGATHRESH100_POS 0
> +#define MSCC_PHY_TR_VGATHRESH100_WIDTH 7
> +#define MSCC_PHY_TR_VGATHRESH100_MASK 0x7f
> +#define MSCC_PHY_TR_VGAGAIN10_U_POS 0
> +#define MSCC_PHY_TR_VGAGAIN10_U_WIDTH 1
> +#define MSCC_PHY_TR_VGAGAIN10_U_MASK 0x01
> +#define MSCC_PHY_TR_VGAGAIN10_L_POS 12
> +#define MSCC_PHY_TR_VGAGAIN10_L_WIDTH 4
> +#define MSCC_PHY_TR_VGAGAIN10_L_MASK 0xf000
> +/* General Timeout Values */
> +#define MSCC_PHY_RESET_TIMEOUT (100)
> +#define MSCC_PHY_MICRO_TIMEOUT (500)
> +
> +/**< RGMII/GMII Clock Delay (Skew) Options */
Very strange comment like "/**<".
> +enum vsc_phy_rgmii_skew {
> + VSC_PHY_RGMII_DELAY_200_PS,
> + VSC_PHY_RGMII_DELAY_800_PS,
> + VSC_PHY_RGMII_DELAY_1100_PS,
> + VSC_PHY_RGMII_DELAY_1700_PS,
> + VSC_PHY_RGMII_DELAY_2000_PS,
> + VSC_PHY_RGMII_DELAY_2300_PS,
> + VSC_PHY_RGMII_DELAY_2600_PS,
> + VSC_PHY_RGMII_DELAY_3400_PS
That's a good practice to add comma even after the last member of enum.
This makes the next patch much cleaner if you want to add another value to this enum.
> +};
> +
> +/**< MAC i/f Clock Edge Rage Control (Slew), See Reg27E2 */
Again strange comment.
> +enum vsc_phy_clk_slew {
> + VSC_PHY_CLK_SLEW_RATE_0,
> + VSC_PHY_CLK_SLEW_RATE_1,
> + VSC_PHY_CLK_SLEW_RATE_2,
> + VSC_PHY_CLK_SLEW_RATE_3,
> + VSC_PHY_CLK_SLEW_RATE_4,
> + VSC_PHY_CLK_SLEW_RATE_5,
> + VSC_PHY_CLK_SLEW_RATE_6,
> + VSC_PHY_CLK_SLEW_RATE_7
> +};
> +
> +
> +static int mscc_vsc8531_vsc8541_init_scripts(struct phy_device *phydev)
> +{
> + u16 reg_val17;
> + u16 reg_val18;
Any reason to keep variables in different lines?
> +
> + /* Set to Access Token Ring Registers */
> + phy_write(phydev, MDIO_DEVAD_NONE,
> + MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_TR);
> +
> + /* Update LinkDetectCtrl default to optimized values */
> + /* Determined during Silicon Validation Testing */
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0xA7F8);
Cryptic values are not very nice to deal with. I'd advise you to use defines here.
Moreover I'm wondering:
a) why all this complicated setup is required?
b) is it mentioned in any datasheet? If it is mentioned it worth adding a comment
about source of those figures and explanation of the procedure itself.
This will help mere mortals to figure out what is done here and if required people will
use that knowledge as a reference in other projects.
> + reg_val17 = phy_read(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_17);
> + reg_val17 = bitfield_replace(reg_val17, MSCC_PHY_TR_LINKDETCTRL_POS,
> + MSCC_PHY_TR_LINKDETCTRL_WIDTH, 3);
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_17, reg_val17);
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0x87F8);
> +
> + /* Update VgaThresh100 defaults to optimized values */
> + /* Determined during Silicon Validation Testing */
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0xAFA4);
> + reg_val18 = phy_read(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18);
> + reg_val18 = bitfield_replace(reg_val18, MSCC_PHY_TR_VGATHRESH100_POS,
> + MSCC_PHY_TR_VGATHRESH100_WIDTH, 24);
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18, reg_val18);
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0x8FA4);
> +
> + /* Update VgaGain10 defaults to optimized values */
> + /* Determined during Silicon Validation Testing */
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0xAF92);
> + reg_val18 = phy_read(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18);
> + reg_val17 = phy_read(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_17);
> + reg_val18 = bitfield_replace(reg_val18, MSCC_PHY_TR_VGAGAIN10_U_POS,
> + MSCC_PHY_TR_VGAGAIN10_U_WIDTH, 0);
> +
> + reg_val17 = bitfield_replace(reg_val17, MSCC_PHY_TR_VGAGAIN10_L_POS,
> + MSCC_PHY_TR_VGAGAIN10_L_WIDTH, 1);
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18, reg_val18);
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_17, reg_val17);
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0x8F92);
> +
> + /* Set back to Access Standard Page Registers */
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_EXT_PAGE_ACCESS,
> + MSCC_PHY_PAGE_STD);
> +
> + return 0;
> +}
> +
> +static int mscc_parse_status(struct phy_device *phydev)
> +{
> + u16 speed;
> + u16 mii_reg;
> +
> + mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_AUX_CNTRL_STAT_REG);
u16 speed, mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_AUX_CNTRL_STAT_REG);
> + if (mii_reg & MIIM_AUX_CNTRL_STAT_F_DUPLEX)
> + phydev->duplex = DUPLEX_FULL;
> + else
> + phydev->duplex = DUPLEX_HALF;
> +
> + speed = mii_reg & MIIM_AUX_CNTRL_STAT_SPEED_MASK;
> + speed = speed >> MIIM_AUX_CNTRL_STAT_SPEED_POS;
> +
> + switch (speed) {
> + case MIIM_AUX_CNTRL_STAT_SPEED_1000M:
> + phydev->speed = SPEED_1000;
> + break;
> + case MIIM_AUX_CNTRL_STAT_SPEED_100M:
> + phydev->speed = SPEED_100;
> + break;
> + case MIIM_AUX_CNTRL_STAT_SPEED_10M:
> + phydev->speed = SPEED_10;
> + break;
> + default:
> + phydev->speed = SPEED_10;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int mscc_startup(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = genphy_update_link(phydev);
Merge two lines above together?
> + if (ret)
> + return ret;
Newline?
> + return mscc_parse_status(phydev);
> +}
> +
> +static int mscc_phy_soft_reset(struct phy_device *phydev)
> +{
> + int rc = 0;
> + u16 timeout = MSCC_PHY_RESET_TIMEOUT;
> + u16 reg_val = 0;
> +
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_EXT_PAGE_ACCESS,
> + MSCC_PHY_PAGE_STD);
> +
> + reg_val = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
> + reg_val |= BMCR_RESET;
> + phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, reg_val);
I'd merge two lines above, i.e. move "|= BMCR_RESET" right in phy_write().
> + reg_val = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
> +
> + while ((reg_val & BMCR_RESET) && (timeout > 0)) {
> + reg_val = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
if (!(reg_val & BMCR_RESET))
return 0;
> + timeout--;
> + udelay(1000); /* 1 ms */
> + }
If above is done then no need to check timeout here, just bail on timeout here.
And BTW "rc" might be removed than as well.
> + if (timeout == 0) {
> + printf("MSCC PHY Soft_Reset Error: mac i/f = 0x%x\n",
> + phydev->interface);
> + rc = -ETIME;
> + }
> +
> + return rc;
> +}
> +
> +static int vsc8531_vsc8541_mac_config(struct phy_device *phydev)
> +{
> + int rc = 0;
> + u16 reg_val = 0;
> + u16 mac_if = 0;
> + u16 rx_clk_out = 0;
Different spacing?
> +
> + /* For VSC8530/31 the only MAC modes are RMII/RGMII. */
> + /* For VSC8540/41 the only MAC modes are (G)MII and RMII/RGMII. */
> + /* Setup MAC Configuration */
> + switch (phydev->interface) {
> + case PHY_INTERFACE_MODE_MII:
> + case PHY_INTERFACE_MODE_GMII:
> + /* Set Reg23.12:11=0 */
> + mac_if = MAC_IF_SELECTION_GMII;
> + /* Set Reg20E2.11=1 */
> + rx_clk_out = RX_CLK_OUT_DISABLE;
> + break;
> +
> + case PHY_INTERFACE_MODE_RMII:
> + /* Set Reg23.12:11=1 */
> + mac_if = MAC_IF_SELECTION_RMII;
> + /* Set Reg20E2.11=0 */
> + rx_clk_out = RX_CLK_OUT_NORMAL;
> + break;
> +
> + case PHY_INTERFACE_MODE_RGMII:
> + /* Set Reg23.12:11=2 */
> + mac_if = MAC_IF_SELECTION_RGMII;
> + /* Set Reg20E2.11=0 */
> + rx_clk_out = RX_CLK_OUT_NORMAL;
> + break;
> +
> + default:
> + printf("MSCC PHY - INVALID MAC i/f Config: mac i/f = 0x%x\n",
> + phydev->interface);
> + return -EINVAL;
> + }
> +
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_EXT_PAGE_ACCESS,
> + MSCC_PHY_PAGE_STD);
> +
> + /* Read Reg23 */
Completely senseless comment. Code below makes much more sense.
> + reg_val = phy_read(phydev, MDIO_DEVAD_NONE,
> + MSCC_PHY_EXT_PHY_CNTL_1);
> + /* Set MAC i/f bits Reg23.12:11 */
> + reg_val = bitfield_replace(reg_val, MAC_IF_SELECTION_POS,
> + MAC_IF_SELECTION_WIDTH, mac_if);
> + /* Update Reg23.12:11 */
> + phy_write(phydev, MDIO_DEVAD_NONE,
> + MSCC_PHY_EXT_PHY_CNTL_1, reg_val);
> + /* Setup ExtPg_2 Register Access */
> + phy_write(phydev, MDIO_DEVAD_NONE,
> + MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXT2);
> + /* Read Reg20E2 */
> + reg_val = phy_read(phydev, MDIO_DEVAD_NONE,
> + MSCC_PHY_RGMII_CNTL);
> + reg_val = bitfield_replace(reg_val, RX_CLK_OUT_POS,
> + RX_CLK_OUT_WIDTH, rx_clk_out);
> + /* Update Reg20E2.11 */
> + phy_write(phydev, MDIO_DEVAD_NONE,
> + MSCC_PHY_RGMII_CNTL, reg_val);
> + /* Before leaving - Change back to Std Page Register Access */
> + phy_write(phydev, MDIO_DEVAD_NONE, MSCC_EXT_PAGE_ACCESS,
> + MSCC_PHY_PAGE_STD);
> +
> + return rc;
> +}
> +
-Alexey
More information about the U-Boot
mailing list