On Mon, Jun 17, 2024 at 01:38:41PM +0200, Kamil Horák - 2N wrote:
> +int bcm_config_lre_advert(struct phy_device *phydev)
> +{
> + int err;
> + u32 adv;
> +
> + /* Only allow advertising what this PHY supports */
> + linkmode_and(phydev->advertising, phydev->advertising,
> + phydev->supported);
Isn't this already done by phy_ethtool_ksettings_set() ?
linkmode_copy(advertising, cmd->link_modes.advertising);
...
/* We make sure that we don't pass unsupported values in to the PHY */
linkmode_and(advertising, advertising, phydev->supported);
...
linkmode_copy(phydev->advertising, advertising);
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 370e4ed45098..5e590c8f82c4 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -5,6 +5,9 @@
> * Broadcom BCM5411, BCM5421 and BCM5461 Gigabit Ethernet
> * transceivers.
> *
> + * Broadcom BCM54810, BCM54811 BroadR-Reach transceivers.
> + *
> + *
Nit: why two blank lines?
> +static int bcm54811_read_abilities(struct phy_device *phydev)
> +{
> + static const int modes_array[] = { ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> + ETHTOOL_LINK_MODE_10baseT1BRR_Full_BIT,
> + ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> + ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> + ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> + ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> + ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> + ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> + ETHTOOL_LINK_MODE_10baseT_Half_BIT };
Formatting...
static const int modes_array[] = {
ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
...
ETHTOOL_LINK_MODE_10baseT_Half_BIT
};
please. This avoids wrapping beyond column 80, and is to kernel coding
standards.
> + int i, val, err;
> + u8 brr_mode;
> +
> + for (i = 0; i < ARRAY_SIZE(modes_array); i++)
> + linkmode_clear_bit(modes_array[i], phydev->supported);
> +
> + err = bcm5481x_get_brrmode(phydev, &brr_mode);
> + if (err)
> + return err;
> +
> + if (brr_mode) {
> + linkmode_set_bit_array(phy_basic_ports_array,
> + ARRAY_SIZE(phy_basic_ports_array),
> + phydev->supported);
> +
> + val = phy_read(phydev, MII_BCM54XX_LRESR);
> + if (val < 0)
> + return val;
> +
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> + phydev->supported, 1);
linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
phydev->supported);
...
> + /* Ensure LRE or IEEE register set is accessed according to the brr on/off,
> + * thus set the override
> + */
> + return bcm_phy_write_exp(phydev, BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL,
> + BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN |
> + (on ? 0 : BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL));
Needless parens, wrong formatting. Consider a local variable:
val = BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN;
if (!on)
val |= BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL;
return bcm_phy_write_exp(phydev,
BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL, val);
would be much nicer to read.
> + if (phydev->autoneg != AUTONEG_ENABLE) {
> + if (!phydev->autoneg_complete) {
> + /* aneg not yet done, reset all relevant bits */
> + static const int br_bits[] = { ETHTOOL_LINK_MODE_Autoneg_BIT,
> + ETHTOOL_LINK_MODE_Pause_BIT,
> + ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> + ETHTOOL_LINK_MODE_10baseT1BRR_Full_BIT,
> + ETHTOOL_LINK_MODE_100baseT1_Full_BIT };
More formatting issues. Maybe consider moving these out of the function?
> + for (i = 0; i < ARRAY_SIZE(br_bits); i++)
> + linkmode_clear_bit(br_bits[i], phydev->lp_advertising);
Formatting issue...
...
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> + phydev->lp_advertising, lrelpa & LRELPA_PAUSE_ASYM);
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> + phydev->lp_advertising, lrelpa & LRELPA_PAUSE);
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> + phydev->lp_advertising, lrelpa & LRELPA_100_1PAIR);
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1BRR_Full_BIT,
> + phydev->lp_advertising, lrelpa & LRELPA_10_1PAIR);
More formatting issues.
> +static int bcm54811_read_status(struct phy_device *phydev)
> +{
> + int err;
> + u8 brr_mode;
Reverse Christmas tree please.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
