On 3/3/24 18:51, Andrew Lunn wrote:
>> +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?
It takes 1.87 and 0.24 seconds to load the firmware files.
> 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.
The main reason I have it in config_init() is that by then the
rootfs is available. The EN8811H does not depend on the firmware
to respond to get_features(). It is therefore possible to not
have the firmware in initramfs or included in the kernel image.
I could not get this result using EPROBE_DEFER, I think this is
not an option in phylink.
It does however work, loading firmware in probe(), without running
a thread, so it is possible to have it there.
>> + /* Select mode 1, the only mode supported */
>
> Maybe a comment about what mode 1 actually is?
I'll need to contact Airoha to get a better description. There
should be a datasheet coming for external use, but it isn't
published yet.
>> + 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.
I'll add a comment that it set's these 3 gpio's as output for the leds.
>> +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?
Will returning an error in config_aneg() prevent auto-neg being disabled?
If so, then indeed I could drop the first part then.
>> +
>> + /* 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?
Partial output from ethtool:
Supported ports: [ TP MII ]
Supported link modes: 100baseT/Full
1000baseT/Full
2500baseT/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 100baseT/Full
1000baseT/Full
2500baseT/Full
Advertised pause frame use: Symmetric Receive-only
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Best regards,
Eric Woudstra
