netdev - Re: [PATCH net-next v2 2/2] net: phy: Add driver for Motorcomm yt8821 2.5G ethernet phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0d4d3507-f095-47c6-866f-1e850cf7f3d8@motor-comm.com>
Date: Sat, 17 Aug 2024 23:36:38 -0700
From: "Frank.Sae" <Frank.Sae@...or-comm.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 8/17/24 04:36, Russell King (Oracle) wrote:
> 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.

Here not check parameter iface, it is not to say that iface has no relation
with rate matching. when interface is configed with phy-mode property in
DT, modify YT8521_CHIP_CONFIG_REG register bit2:0 dependent on
phydev->interface in yt8821_config_init(), if phy-mode = "sgmii", bit2:0
will be set 3'b000, if phy-mode = "2500base-x", bit2:0 will be set 3'b001.

so that YT8521_CHIP_CONFIG_REG register bit2:0 may decide enable or disable
rate matching feature in yt8821_get_rate_matching() and do not care input
parameter iface here.

>> +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.

yt8821 is pin2pin realtek rtl8221.

please refer to description about interface force 2500base-x and auto
2500base-x_sgmii in datasheet.

In AUTO_BX2500_SGMII mode, The internal flow control buffer is disabled in
this mode.

In FORCE_BX2500, SerDes always works as 2500BASE-X, internal flow control
buffer will be activated if UTP doesn't work at 2.5GBASE-T.

>> +
>> +	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?

it is ok.

>
>
> 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?)

now no supported_interfaces member in struct phy_device.

> Note the point below in my signature.
>