netdev - Re: [PATCH 4/5] net: phy: dp83tg720: Added OA script

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <741f9487-e7f4-4c6e-b933-18cc2761c2f1@lunn.ch>
Date: Thu, 19 Sep 2024 23:44:49 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Alvaro (Al-vuh-roe) Reyes" <a-reyes1@...com>
Cc: netdev@...r.kernel.org, hkallweit1@...il.com, linux@...linux.org.uk,
	maxime.chevallier@...tlin.com, o.rempel@...gutronix.de,
	spatton@...com, r-kommineni@...com, e-mayhew@...com,
	praneeth@...com, p-varis@...com, d-qiu@...com
Subject: Re: [PATCH 4/5] net: phy: dp83tg720: Added OA script

> +struct DP83TG720_init_reg {
> +	int MMD;
> +	int reg;
> +	int val;
> +};
> +
> +/*Refer to SNLA371 for more information*/
> +static const struct DP83TG720_init_reg DP83TG720_cs1_1_master_init[] = {
> +	{0x1F, 0x001F, 0X8000},
> +	{0x1F, 0x0573, 0x0101},
> +	{0x1, 0x0834, 0xC001},

MDIO_MMD_VEND2 etc.

Also 0x834 is BASE-T1 PMA/PMD control. Which is MDIO_PMA_PMD_BT1_CTRL

We also have:
#define MDIO_PMA_PMD_BT1_CTRL_STRAP_B1000 0x0001 /* Select 1000BASE-T1 */
#define MDIO_PMA_PMD_BT1_CTRL_CFG_MST		0x4000 /* MASTER-SLAVE config value */

802.3 says bit 15 is read only, so you don't need to set it.

The rest might be magic which nobody outside of TI will understand,
but you can fully document this.

> +static int dp83tg720_reset(struct phy_device *phydev, bool hw_reset)
> +{
> +	int ret;
> +
> +	if (hw_reset)
> +		ret = phy_write_mmd(phydev, MMD1F, DP83TG720_PHY_RESET_CTRL,
> +				DP83TG720_HW_RESET);
> +	else
> +		ret = phy_write_mmd(phydev, MMD1F, DP83TG720_PHY_RESET_CTRL,
> +				DP83TG720_SW_RESET);
> +	if (ret)
> +		return ret;
> +
> +	mdelay(100);

Does the bit not self clear when it has completed? That would be
common for a reset bit.

> +static int DP83TG720_write_seq(struct phy_device *phydev,
> +			     const struct DP83TG720_init_reg *init_data, int size)
> +{
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < size; i++) {
> +			ret = phy_write_mmd(phydev, init_data[i].MMD, init_data[i].reg,
> +				init_data[i].val);
> +			if (ret)
> +					return ret;
> +	}

More messed up indentation.

> +static int dp83tg720_chip_init(struct phy_device *phydev)
> +{
> +	struct DP83TG720_private *DP83TG720 = phydev->priv;
> +	int ret;
> +
> +	ret = dp83tg720_reset(phydev, true);
> +	if (ret)
> +		return ret;
> +	
> +	phydev->autoneg = AUTONEG_DISABLE;
> +    phydev->speed = SPEED_1000;
> +	phydev->duplex = DUPLEX_FULL;
> +    linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, phydev->supported);

This should not be needed. phylib should be able to figure this out
for itself from the registers. Please check that functions like
genphy_c45_pma_baset1_read_abilities() are doing.

> +
> +	switch (DP83TG720->chip) {
> +	case DP83TG720_CS1_1:
> +		if (DP83TG720->is_master)
> +			ret = DP83TG720_write_seq(phydev, DP83TG720_cs1_1_master_init,
> +						ARRAY_SIZE(DP83TG720_cs1_1_master_init));
> +		else
> +			ret = DP83TG720_write_seq(phydev, DP83TG720_cs1_1_slave_init,
> +						ARRAY_SIZE(DP83TG720_cs1_1_slave_init));
> +
> +		ret = dp83tg720_reset(phydev, false);
> +
> +		return 1;

0 on success, negative error code on error.

	Andrew