Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <7f0df23b-f00e-fef8-fa03-314fcfe136eb@quicinc.com>
Date:   Sat, 4 Nov 2023 14:25:25 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] net: phy: at803x: add QCA8084 ethernet phy support
To:     Andrew Lunn <andrew@lunn.ch>
CC:     <hkallweit1@gmail.com>, <linux@armlinux.org.uk>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20231103123538.15735-1-quic_luoj@quicinc.com>
 <806fb6b6-d9b6-457b-b079-48f8b958cc5a@lunn.ch>
Content-Language: en-US
From:   Jie Luo <quic_luoj@quicinc.com>
In-Reply-To: <806fb6b6-d9b6-457b-b079-48f8b958cc5a@lunn.ch>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/3/2023 9:01 PM, Andrew Lunn wrote:
>>   #define QCA8081_PHY_ID				0x004dd101
>> +#define QCA8081_PHY_MASK			0xffffff00
> 
> That is an unusual mask. Please check it is correct. All you should
> need its PHY_ID_MATCH_EXACT, PHY_ID_MATCH_MODEL, PHY_ID_MATCH_VENDOR.

Thanks Andrew for the review.
The PHY ID of qca8084 is correct, i will update to use 
PHY_ID_MATCH_EXACT in the new added entry for qca8084.

> 
>> @@ -1767,6 +1781,20 @@ static int qca808x_config_init(struct phy_device *phydev)
>>   {
>>   	int ret;
>>   
>> +	if (phydev->phy_id == QCA8084_PHY_ID) {
>> +		/* Invert ADC clock edge */
>> +		ret = at803x_debug_reg_mask(phydev, QCA8084_ADC_CLK_SEL,
>> +					    QCA8084_ADC_CLK_SEL_ACLK,
>> +					    FIELD_PREP(QCA8084_ADC_CLK_SEL_ACLK,
>> +						       QCA8084_ADC_CLK_SEL_ACLK_FALL));
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		/* Adjust MSE threshold value to avoid link issue with some link partner */
>> +		return phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
>> +				QCA8084_MSE_THRESHOLD, QCA8084_MSE_THRESHOLD_2P5G_VAL);
>> +	}
>> +
> 
> Please add a qca8084_config_init() and use that from the phy_driver
> structure.

OK.

> 
>>   	/* Active adc&vga on 802.3az for the link 1000M and 100M */
>>   	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_ADDR_CLD_CTRL7,
>>   			QCA808X_8023AZ_AFE_CTRL_MASK, QCA808X_8023AZ_AFE_EN);
>> @@ -1958,6 +1986,11 @@ static int qca808x_cable_test_start(struct phy_device *phydev)
>>   	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x807a, 0xc060);
>>   	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x807e, 0xb060);
>>   
>> +	if (phydev->phy_id == QCA8084_PHY_ID) {
>> +		phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8075, 0xa060);
>> +		phy_write_mmd(phydev, MDIO_MMD_PCS, 0x807f, 0x1eb0);
>> +	}
>> +
> 
> Please add a comment what this is doing.

Ok, will add comments in the next patch.

> 
>>   }, {
>>   	/* Qualcomm QCA8081 */
>> -	PHY_ID_MATCH_EXACT(QCA8081_PHY_ID),
>> -	.name			= "Qualcomm QCA8081",
>> +	.phy_id			= QCA8081_PHY_ID,
>> +	.phy_id_mask		= QCA8081_PHY_MASK,
>> +	.name			= "Qualcomm QCA808X",
> 
> Please add a new entry for the 8084.
> 
>         Andrew

Will add it in the next patch, thanks.