Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 13 Jan 2020 15:42:47 +0200 | Subject | Re: [RFC net-next] net: phy: Add basic support for Synopsys XPCS using a PHY driver |
| |
On Mon, Jan 13, 2020 at 3:13 PM Jose Abreu <Jose.Abreu@synopsys.com> wrote: > > Adds the basic support for XPCS including support for USXGMII.
...
> +#define SYNOPSYS_XPHY_ID 0x7996ced0 > +#define SYNOPSYS_XPHY_MASK 0xffffffff
GENMASK() ? (It seems bits.h is missed in the headers block)
...
> +#define DW_USXGMII_2500 (BIT(5)) > +#define DW_USXGMII_1000 (BIT(6)) > +#define DW_USXGMII_100 (BIT(13)) > +#define DW_USXGMII_10 (0)
Useless parentheses.
...
> +static int dw_poll_reset(struct phy_device *phydev, int dev) > +{ > + /* Poll until the reset bit clears (50ms per retry == 0.6 sec) */ > + unsigned int retries = 12; > + int ret; > + > + do {
> + msleep(50);
It's a bit unusual to have timeout loop to be started with sleep. Imagine the case when between writing a reset bit and actual checking the scheduling happens and reset has been done You add here unnecessary 50 ms of waiting.
> + ret = phy_read_mmd(phydev, dev, MDIO_CTRL1); > + if (ret < 0) > + return ret; > + } while (ret & MDIO_CTRL1_RESET && --retries); > + if (ret & MDIO_CTRL1_RESET) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +static int __dw_soft_reset(struct phy_device *phydev, int dev, int reg) > +{ > + int val;
val?! Perhaps ret is better name? Applicable to the rest of functions.
> + > + val = phy_write_mmd(phydev, dev, reg, MDIO_CTRL1_RESET); > + if (val < 0) > + return val; > +
> + val = dw_poll_reset(phydev, dev); > + if (val < 0) > + return val; > + > + return 0;
return dw_poll_reset(...);
> +} > + > +static int dw_soft_reset(struct phy_device *phydev, u32 mmd_mask) > +{ > + int val, devad; > + > + while (mmd_mask) { > + devad = __ffs(mmd_mask); > + mmd_mask &= ~BIT(devad);
for_each_set_bit()
> + > + val = __dw_soft_reset(phydev, devad, MDIO_CTRL1); > + if (val < 0) > + return val; > + } > + > + return 0; > +} > + > +static int dw_read_link(struct phy_device *phydev, u32 mmd_mask) > +{ > + bool link = true; > + int val, devad; > + > + while (mmd_mask) { > + devad = __ffs(mmd_mask); > + mmd_mask &= ~BIT(devad);
Ditto.
> + > + val = phy_read_mmd(phydev, devad, MDIO_STAT1); > + if (val < 0) > + return val; > + > + if (!(val & MDIO_STAT1_LSTATUS)) > + link = false; > + } > + > + return link; > +}
> +#define dw_warn(__phy, __args...) \
dw_warn() -> dw_warn_if_phy_link()
> +({ \ > + if ((__phy)->link) \ > + dev_warn(&(__phy)->mdio.dev, ##__args); \ > +})
...
> + int val, speed_sel = 0x0;
Redundant assignment.
> + switch (phydev->speed) { > + case SPEED_10: > + speed_sel = DW_USXGMII_10; > + break; > + case SPEED_100: > + speed_sel = DW_USXGMII_100; > + break; > + case SPEED_1000: > + speed_sel = DW_USXGMII_1000; > + break; > + case SPEED_2500: > + speed_sel = DW_USXGMII_2500; > + break; > + case SPEED_5000: > + speed_sel = DW_USXGMII_5000; > + break; > + case SPEED_10000: > + speed_sel = DW_USXGMII_10000; > + break; > + default: > + /* Nothing to do here */ > + return 0; > + }
...
> +static int dw_config_aneg_c73(struct phy_device *phydev) > +{ > + u32 adv = 0;
Redundant assignment.
> + int ret; > + > + /* SR_AN_ADV3 */ > + adv = phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_ADV3); > + if (adv < 0) > + return adv;
> +}
...
> + do { > + msleep(50);
Same as above about timeout loops.
> + val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1); > + if (val < 0) > + return val; > + } while (val & MDIO_AN_CTRL1_RESTART && --retries);
...
> +static int dw_aneg_done(struct phy_device *phydev) > +{ > + int val; > + > + val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1); > + if (val < 0) > + return val; > + > + if (val & MDIO_AN_STAT1_COMPLETE) { > + val = phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_LP_ABL1); > + if (val < 0) > + return val; > + > + /* Check if Aneg outcome is valid */ > + if (!(val & 0x1)) > + goto fault; > +
> + return 1;
1?! What does it mean?
> + } > + > + if (val & MDIO_AN_STAT1_RFAULT) > + goto fault; > + > + return 0; > +fault: > + dev_err(&phydev->mdio.dev, "Invalid Autoneg result!\n"); > + dev_err(&phydev->mdio.dev, "CTRL1=0x%x, STAT1=0x%x\n", > + phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1), > + phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1)); > + dev_err(&phydev->mdio.dev, "ADV1=0x%x, ADV2=0x%x, ADV3=0x%x\n", > + phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_ADV1), > + phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_ADV2), > + phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_ADV3)); > + dev_err(&phydev->mdio.dev, "LP_ADV1=0x%x, LP_ADV2=0x%x, LP_ADV3=0x%x\n", > + phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_LP_ABL1), > + phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_LP_ABL2), > + phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_LP_ABL3)); > + > + val = dw_soft_reset(phydev, MDIO_DEVS_PCS); > + if (val < 0) > + return val; > + > + return dw_config_aneg(phydev); > +}
...
> + phydev->pause = val & DW_C73_PAUSE ? 1 : 0; > + phydev->asym_pause = val & DW_C73_ASYM_PAUSE ? 1 : 0;
!!(x) should work as well. But I think compiler optimizes that ternary well.
...
> + val = dw_aneg_done(phydev); > + if (val <= 0) {
This <= 0 should be explained. Why we set link when it's < 0 or otherwise why we return 0 when link is set to false.
> + phydev->link = false; > + return val; > + }
...
> +static struct mdio_device_id __maybe_unused dw_tbl[] = { > + { SYNOPSYS_XPHY_ID, SYNOPSYS_XPHY_MASK }, > + { },
Comma is not needed.
> +};
-- With Best Regards, Andy Shevchenko
| |