[U-Boot] [PATCH 1/1] Adding MSCC PHY-VSC8530-VSC8531-VSC8540-VSC8541

[Joe Hershberger]

Hi Alexey,

Thanks for the review. I agree with most of your comments, but I'll
note where I don't.

On Thu, Dec 8, 2016 at 6:24 AM, Alexey Brodkin
<Alexey.Brodkin at synopsys.com> wrote:

...

>> +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?

I think separate lines is cleaner and prefer it stay this way.

>> +
>> +     /* 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);

This is especially ugly. It looks more like a python function
returning a tuple than clean C code. Please do not do this.

>> +     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?

I could go either way on this one. I certainly wouldn't ask for this
change, but I'm ok with it.

>> +     if (ret)
>> +           return ret;