Re: [RFC net-next] net: phy: Add basic support for Synopsys XPCS using a PHY driver
From: Andy Shevchenko
Date: Mon Jan 13 2020 - 08:43:03 EST
On Mon, Jan 13, 2020 at 3:13 PM Jose Abreu <Jose.Abreu@xxxxxxxxxxxx> 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