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

[Joe Hershberger]

Hi Alexey,

On Fri, Dec 9, 2016 at 3:06 AM, Alexey Brodkin
<Alexey.Brodkin at synopsys.com> wrote:
> Hi Joe,
>
>> -----Original Message-----
>> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
>> Sent: Thursday, December 08, 2016 8:05 PM
>> To: Alexey Brodkin <Alexey.Brodkin at synopsys.com>
>> Cc: john.haechten at microsemi.com; u-boot at lists.denx.de; joe.hershberger at ni.com
>> Subject: Re: [U-Boot] [PATCH 1/1] Adding MSCC PHY-VSC8530-VSC8531-VSC8540-VSC8541
>>
>> 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.
>
> Well especially here IMHO there's no need to separate 2 temporary vars.
> Still it's more a matter of your taste/preference.
> Moreover I'm not really asking to do that change but wondering about reasons
> and with good explanation I'm fine with it as it is.

The way the code is currently written the two variables are used
concurrently, but I'd be surprised if that was a requirement. Maybe
John can confirm otherwise, but simply doing each read/modify/write
operations sequentially instead of mixing the order would allow a
single temp variable for the operation.

>> >> +
>> >> +     /* 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.
>
> Again just an example of how I would do that.
> But I may agree that it may look a bit ugly :)
>
> Anyways thanks for your input.
>
> -Alexey