Re: [PATCH net-next v3 1/3] net: phy: qcom: Add PHY counter support

[Luo Jie <quic_luoj@quicinc.com>]

On 7/17/2025 9:46 PM, Andrew Lunn wrote:
> On Thu, Jul 17, 2025 at 03:23:16PM +0200, Paolo Abeni wrote:
>> On 7/16/25 12:15 PM, Luo Jie wrote:
>>> On 7/16/2025 12:11 AM, Andrew Lunn wrote:
>>>>> +int qcom_phy_update_stats(struct phy_device *phydev,
>>>>> +			  struct qcom_phy_hw_stats *hw_stats)
>>>>> +{
>>>>> +	int ret;
>>>>> +	u32 cnt;
>>>>> +
>>>>> +	/* PHY 32-bit counter for RX packets. */
>>>>> +	ret = phy_read_mmd(phydev, MDIO_MMD_AN, QCA808X_MMD7_CNT_RX_PKT_15_0);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	cnt = ret;
>>>>> +
>>>>> +	ret = phy_read_mmd(phydev, MDIO_MMD_AN, QCA808X_MMD7_CNT_RX_PKT_31_16);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>
>>>> Does reading QCA808X_MMD7_CNT_RX_PKT_15_0 cause
>>>> QCA808X_MMD7_CNT_RX_PKT_31_16 to latch?
>>>
>>> Checked with the hardware design team: The high 16-bit counter register
>>> does not latch when reading the low 16 bits.
>>>
>>>>
>>>> Sometimes you need to read the high part, the low part, and then
>>>> reread the high part to ensure it has not incremented. But this is
>>>> only needed if the hardware does not latch.
>>>>
>>>> 	Andrew
>>>
>>> Since the counter is configured to clear after reading, the clear action
>>> takes priority over latching the count. This means that when reading the
>>> low 16 bits, the high 16-bit counter value cannot increment, any new
>>> packet events occurring during the read will be recorded after the
>>> 16-bit counter is cleared.
>>
>> Out of sheer ignorance and language bias on my side, based on the above
>> I would have assumed that the registers do latch ;)
> 
> I interpret it differently. The register is set to clear on read. So
> you read and clear the least significant word. Even if that word
> starts incriminating, you have 65535 increments before it will
> overflow into the next word. So you can read the most significant word
> before such an overflow happens. It does not latch, you just have a
> time window when it is safe.
> 
> What i actually find odd is that clear on read works on words, not the
> full counter. I assume that is documented in the datasheet, and
> tested, because i've never seen hardware do that before.
> 
> 	Andrew

Thank you for the review. The PHY counter functionality is also used in
the downstream code, and this patch series has been validated
accordingly. However, please note that the PHY counter is intended as a
debug feature and may not be documented in the datasheet. I will share
this feedback with the hardware team in the hope that we can include
documentation for this feature in the datasheet for future chips.