[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZsCLMQWoZcVV+7xR@shell.armlinux.org.uk>
Date: Sat, 17 Aug 2024 12:36:17 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Frank Sae <Frank.Sae@...or-comm.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
yuanlai.cui@...or-comm.com, hua.sun@...or-comm.com,
xiaoyong.li@...or-comm.com, suting.hu@...or-comm.com,
jie.han@...or-comm.com
Subject: Re: [PATCH net-next v2 2/2] net: phy: Add driver for Motorcomm
yt8821 2.5G ethernet phy
On Thu, Aug 15, 2024 at 11:09:55PM -0700, Frank Sae wrote:
> +static int yt8821_get_rate_matching(struct phy_device *phydev,
> + phy_interface_t iface)
> +{
> + int val;
> +
> + val = ytphy_read_ext_with_lock(phydev, YT8521_CHIP_CONFIG_REG);
> + if (val < 0)
> + return val;
> +
> + if (FIELD_GET(YT8521_CCR_MODE_SEL_MASK, val) ==
> + YT8821_CHIP_MODE_FORCE_BX2500)
> + return RATE_MATCH_PAUSE;
Does this device do rate matching for _any_ interface mode if it has
this bit set - because that's what you're saying here by not testing
"iface". From what I understand from your previous posting which
included a DT update, this only applies when 2500base-X is being
used as the interface mode.
> +static int yt8821_aneg_done(struct phy_device *phydev)
> +{
> + int link;
> +
> + link = yt8521_aneg_done_paged(phydev, YT8521_RSSR_UTP_SPACE);
> +
> + return link;
> +}
Why not just:
return yt8521_aneg_done_paged(phydev, YT8521_RSSR_UTP_SPACE);
?
> +/**
> + * yt8821_config_init() - phy initializatioin
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8821_config_init(struct phy_device *phydev)
> +{
> + u8 mode = YT8821_CHIP_MODE_AUTO_BX2500_SGMII;
> + int ret;
> + u16 set;
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX)
> + mode = YT8821_CHIP_MODE_FORCE_BX2500;
Hmm, I think this is tying us into situations we don't want. What if the
host supports 2500base-X and SGMII, but does not support pause (for
example, Marvell PP2 based hosts.) In that situation, we don't want to
lock-in to using pause based rate adaption, which I fear will become
a behaviour that would be risky to change later on.
> +
> + set = FIELD_PREP(YT8521_CCR_MODE_SEL_MASK, mode);
> + ret = ytphy_modify_ext_with_lock(phydev,
> + YT8521_CHIP_CONFIG_REG,
> + YT8521_CCR_MODE_SEL_MASK,
> + set);
> + if (ret < 0)
> + return ret;
> +
> + if (mode == YT8821_CHIP_MODE_AUTO_BX2500_SGMII) {
> + __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> + phydev->possible_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_SGMII,
> + phydev->possible_interfaces);
> +
> + phydev->rate_matching = RATE_MATCH_NONE;
> + } else if (mode == YT8821_CHIP_MODE_FORCE_BX2500) {
__set_bit(PHY_INTERFACE_MODE_2500BASEX,
phydev->possible_interfaces);
so that phylink knows you're only going to be using a single interface
mode. Even better, since this is always supported, move it out of these
if() statements?
Also, it would be nice to have phydev->supported_interfaces populated
(which has to be done when the PHY is probed) so that phylink knows
before connecting with the PHY which interface modes are supported by
the PHY. (Andrew - please can we make this a condition for any new PHYs
supported by phylib in the future?)
Note the point below in my signature.
--
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists