> +static int en8811h_config_init(struct phy_device *phydev)
> +{
> + struct en8811h_priv *priv = phydev->priv;
> + struct device *dev = &phydev->mdio.dev;
> + int ret, pollret, reg_value;
> + u32 pbus_value;
> +
> + if (!priv->firmware_version)
> + ret = en8811h_load_firmware(phydev);
How long does this take for your hardware?
We have a phylib design issue with loading firmware. It would be
better if it happened during probe, but it might not be finished by
the time the MAC driver tries to attach to the PHY and so that fails.
This is the second PHY needing this, so maybe we need to think about
adding a thread kicked off in probe to download the firmware? But
probably later, not part of this patchset.
> + else
> + ret = en8811h_restart_host(phydev);
> + if (ret < 0)
> + return ret;
> +
> + /* Because of mdio-lock, may have to wait for multiple loads */
> + pollret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
> + EN8811H_PHY_FW_STATUS, reg_value,
> + reg_value == EN8811H_PHY_READY,
> + 20000, 7500000, true);
> +
> + ret = air_buckpbus_reg_read(phydev, EN8811H_FW_VERSION, &pbus_value);
> + if (ret < 0)
> + return ret;
> +
> + if (pollret || !pbus_value) {
> + phydev_err(phydev, "Firmware not ready: 0x%x\n", reg_value);
> + return -ENODEV;
> + }
> +
> + if (!priv->firmware_version) {
> + phydev_info(phydev, "MD32 firmware version: %08x\n", pbus_value);
> + priv->firmware_version = pbus_value;
> + }
> +
> + /* Select mode 1, the only mode supported */
Maybe a comment about what mode 1 actually is?
> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AIR_PHY_HOST_CMD_1,
> + AIR_PHY_HOST_CMD_1_MODE1);
> + if (ret < 0)
> + return ret;
> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AIR_PHY_HOST_CMD_2,
> + AIR_PHY_HOST_CMD_2_MODE1);
> + if (ret < 0)
> + return ret;
> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AIR_PHY_HOST_CMD_3,
> + AIR_PHY_HOST_CMD_3_MODE1);
> + if (ret < 0)
> + return ret;
> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AIR_PHY_HOST_CMD_4,
> + AIR_PHY_HOST_CMD_4_MODE1);
> + if (ret < 0)
> + return ret;
> +
> + /* Serdes polarity */
> + pbus_value = 0;
> + if (device_property_read_bool(dev, "airoha,pnswap-rx"))
> + pbus_value |= EN8811H_POLARITY_RX_REVERSE;
> + else
> + pbus_value &= ~EN8811H_POLARITY_RX_REVERSE;
> + if (device_property_read_bool(dev, "airoha,pnswap-tx"))
> + pbus_value &= ~EN8811H_POLARITY_TX_NORMAL;
> + else
> + pbus_value |= EN8811H_POLARITY_TX_NORMAL;
> + ret = air_buckpbus_reg_modify(phydev, EN8811H_POLARITY,
> + EN8811H_POLARITY_RX_REVERSE |
> + EN8811H_POLARITY_TX_NORMAL, pbus_value);
> + if (ret < 0)
> + return ret;
> +
> + ret = air_leds_init(phydev, EN8811H_LED_COUNT, AIR_PHY_LED_DUR,
> + AIR_LED_MODE_USER_DEFINE);
> + if (ret < 0) {
> + phydev_err(phydev, "Failed to initialize leds: %d\n", ret);
> + return ret;
> + }
> +
> + ret = air_buckpbus_reg_modify(phydev, EN8811H_GPIO_OUTPUT,
> + EN8811H_GPIO_OUTPUT_345,
> + EN8811H_GPIO_OUTPUT_345);
What does this do? Configure them as inputs? Hopefully they are inputs
by default, or at least Hi-Z.
> +static int en8811h_get_features(struct phy_device *phydev)
> +{
> + linkmode_set_bit_array(phy_basic_ports_array,
> + ARRAY_SIZE(phy_basic_ports_array),
> + phydev->supported);
> +
> + return genphy_c45_pma_read_abilities(phydev);
> +}
> +
> +static int en8811h_config_aneg(struct phy_device *phydev)
> +{
> + bool changed = false;
> + int ret;
> + u32 adv;
> +
> + adv = linkmode_adv_to_mii_10gbt_adv_t(phydev->advertising);
> +
> + ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> + MDIO_AN_10GBT_CTRL_ADV2_5G, adv);
> + if (ret < 0)
> + return ret;
> + if (ret > 0)
> + changed = true;
> +
> + return __genphy_config_aneg(phydev, changed);
There was a comment that it does not support forced link mode, only
auto-neg? It would be good to test the configuration here and return
EOPNOTSUPP, or EINVAL if auto-neg is turned off.
> +}
> +
> +static int en8811h_read_status(struct phy_device *phydev)
> +{
> + struct en8811h_priv *priv = phydev->priv;
> + u32 pbus_value;
> + int ret, val;
> +
> + ret = genphy_update_link(phydev);
> + if (ret)
> + return ret;
> +
> + phydev->master_slave_get = MASTER_SLAVE_CFG_UNSUPPORTED;
> + phydev->master_slave_state = MASTER_SLAVE_STATE_UNSUPPORTED;
> + phydev->speed = SPEED_UNKNOWN;
> + phydev->duplex = DUPLEX_UNKNOWN;
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> +
> + ret = genphy_read_master_slave(phydev);
> + if (ret < 0)
> + return ret;
> +
> + ret = genphy_read_lpa(phydev);
> + if (ret < 0)
> + return ret;
> +
> + /* Get link partner 2.5GBASE-T ability from vendor register */
> + ret = air_buckpbus_reg_read(phydev, EN8811H_2P5G_LPA, &pbus_value);
> + if (ret < 0)
> + return ret;
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> + phydev->lp_advertising,
> + pbus_value & EN8811H_2P5G_LPA_2P5G);
> +
> + if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
Is the first part of that expression needed? I thought you could not
turn auto-neg off?
> +
> + /* Only supports full duplex */
> + phydev->duplex = DUPLEX_FULL;
What does en8811h_get_features() indicate the PHY can do? Are any 1/2
duplex modes listed?
Andrew
