Re: [PATCH net-next v2 1/2] net: phy: phy-c45: add SQI and SQI+ support for OATC14 10Base-T1S PHYs

[<Parthiban.Veerasooran@microchip.com>]

Hi Simon,

Thank you for reviewing the patch.

On 22/11/25 5:16 pm, Simon Horman wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Nov 18, 2025 at 10:29:45AM +0530, Parthiban Veerasooran wrote:
> 
> ...
> 
>> @@ -1695,3 +1695,89 @@ int genphy_c45_oatc14_cable_test_start(struct phy_device *phydev)
>>                                OATC14_HDD_START_CONTROL);
>>   }
>>   EXPORT_SYMBOL(genphy_c45_oatc14_cable_test_start);
>> +
>> +/**
>> + * genphy_c45_oatc14_get_sqi_max - Get maximum supported SQI or SQI+ level of
>> + *                              OATC14 10Base-T1S PHY
>> + * @phydev: pointer to the PHY device structure
>> + *
>> + * This function reads the advanced capability register to determine the maximum
>> + * supported Signal Quality Indicator (SQI) or SQI+ level
>> + *
>> + * Return:
>> + * * Maximum SQI/SQI+ level (≥0)
>> + * * -EOPNOTSUPP if not supported
>> + * * Negative errno on read failure
>> + */
>> +int genphy_c45_oatc14_get_sqi_max(struct phy_device *phydev)
>> +{
>> +     u8 bits;
>> +     int ret;
>> +
>> +     ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_ADFCAP);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     /* Check for SQI+ capability
>> +      * 0 - SQI+ is not supported
>> +      * (3-8) bits for (8-256) SQI+ levels supported
>> +      */
>> +     bits = FIELD_GET(OATC14_ADFCAP_SQIPLUS_CAPABILITY, ret);
>> +     if (bits) {
>> +             phydev->oatc14_sqiplus_bits = bits;
>> +             /* Max sqi+ level supported: (2 ^ bits) - 1 */
>> +             return BIT(bits) - 1;
>> +     }
>> +
>> +     /* Check for SQI capability
>> +      * 0 - SQI is not supported
>> +      * 1 - SQI is supported (0-7 levels)
>> +      */
>> +     if (ret & OATC14_ADFCAP_SQI_CAPABILITY)
>> +             return OATC14_SQI_MAX_LEVEL;
>> +
>> +     return -EOPNOTSUPP;
>> +}
>> +EXPORT_SYMBOL(genphy_c45_oatc14_get_sqi_max);
>> +
>> +/**
>> + * genphy_c45_oatc14_get_sqi - Get Signal Quality Indicator (SQI) from an OATC14
>> + *                          10Base-T1S PHY
>> + * @phydev: pointer to the PHY device structure
>> + *
>> + * This function reads the SQI+ or SQI value from an OATC14-compatible
>> + * 10Base-T1S PHY. If SQI+ capability is supported, the function returns the
>> + * extended SQI+ value; otherwise, it returns the basic SQI value.
>> + *
>> + * Return:
>> + * * SQI/SQI+ value on success
>> + * * Negative errno on read failure
>> + */
>> +int genphy_c45_oatc14_get_sqi(struct phy_device *phydev)
>> +{
>> +     u8 shift;
>> +     int ret;
>> +
>> +     /* Calculate and return SQI+ value if supported */
>> +     if (phydev->oatc14_sqiplus_bits) {
> 
> Hi Parthiban,
> 
> AFAICT oatc14_sqiplus_bits will always be 0 until
> genphy_c45_oatc14_get_sqi_max() is called, after which
> it may be some other value.
> 
> But according to the flow of linkstate_prepare_data()
> it seems that genphy_c45_oatc14_get_sqi_max()
> will be called after genphy_c45_oatc14_get_sqi().
> 
> In which case the condition above will be false
> (unless genphy_c45_oatc14_get_sqi_max was somehow already
> called by some other means.)
> 
> This doesn't seem to be in line with the intention of this code.
> 
> Flagged by Claude Code with https://github.com/masoncl/review-prompts/
Good catch! oatc14_sqiplus_bits is not updated the first time 
genphy_c45_oatc14_get_sqi() is called, and is later updated by 
genphy_c45_oatc14_get_sqi_max(). Thank you for pointing it out; I will 
fix it in the next version.

Best regards,
Parthiban V
> 
>> +             ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
>> +                                MDIO_OATC14_DCQ_SQIPLUS);
>> +             if (ret < 0)
>> +                     return ret;
>> +
>> +             /* SQI+ uses N MSBs out of 8 bits, left-aligned with padding 1's
>> +              * Calculate the right-shift needed to isolate the N bits.
>> +              */
>> +             shift = 8 - phydev->oatc14_sqiplus_bits;
>> +
>> +             return (ret & OATC14_DCQ_SQIPLUS_VALUE) >> shift;
>> +     }
>> +
>> +     /* Read and return SQI value if SQI+ capability is not supported */
>> +     ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_DCQ_SQI);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return ret & OATC14_DCQ_SQI_VALUE;
>> +}
>> +EXPORT_SYMBOL(genphy_c45_oatc14_get_sqi);
> 
> ...