On Mon, 11 May 2026 18:35:45 +0800
Zaiyu Wang <[email protected]> wrote:
> According to the PHY vendor, when YT8531S operates in UTP-to-Fiber or
> RGMII-to-Fiber mode with auto-negotiation disabled (Force mode),
> additional CDR (Clock Data Recovery) configuration is required to
> improve link connectivity. Without this config, link may be unstable
> or fail to establish.
>
> Fixes: f1268369403d ("net/ngbe: support autoneg on/off for external PHY SFI
> mode")
> Cc: [email protected]
>
> Signed-off-by: Zaiyu Wang <[email protected]>
> ---
Semi-automated AI review feedback.
# DPDK Patch Review - net/ngbe: add missing CDR config for YT PHY
## Summary
This patch adds Clock Data Recovery (CDR) configuration for YT8531S PHY in
Force mode (auto-negotiation disabled). No errors found.
---
## CORRECTNESS BUGS
None identified.
---
## WARNINGS
### 1. Missing error checks on PHY register writes
**File:** `drivers/net/ngbe/base/ngbe_phy_yt.c`
**Lines:** 267-268
The two `ngbe_write_phy_reg_sds_ext_yt()` calls do not check return values. If
these PHY register writes fail, the link may be unstable (the exact problem
this patch aims to fix), but the function will silently continue.
**Current code:**
```c
/* force mode need to config cdr */
ngbe_write_phy_reg_sds_ext_yt(hw, 0x3, 0, 0x1434);
ngbe_write_phy_reg_sds_ext_yt(hw, 0xe, 0, 0x163);
value = YT_BCR_RESET | YT_BCR_DUPLEX;
```
**Suggested fix:**
```c
/* force mode need to config cdr */
s32 ret;
ret = ngbe_write_phy_reg_sds_ext_yt(hw, 0x3, 0, 0x1434);
if (ret != 0)
return ret;
ret = ngbe_write_phy_reg_sds_ext_yt(hw, 0xe, 0, 0x163);
if (ret != 0)
return ret;
value = YT_BCR_RESET | YT_BCR_DUPLEX;
```
Note: This assumes `ngbe_write_phy_reg_sds_ext_yt()` returns 0 on success,
negative on failure (standard DPDK convention). The existing code pattern in
the function does not check errors on PHY writes in the auto-negotiation path
either (lines 264-265), so this may be intentional design. However, for a fix
addressing link stability, propagating write failures would be more robust.
---
## INFO
### 1. Magic numbers in PHY register writes
**File:** `drivers/net/ngbe/base/ngbe_phy_yt.c`
**Lines:** 267-268
The CDR configuration uses literal hex values `0x1434` and `0x163` without
named constants or comments explaining their meaning. This is vendor-specific
configuration. Consider adding a comment referencing the PHY vendor
documentation or datasheet section, or defining named constants if these values
are used elsewhere.
Example:
```c
/* CDR config per YT8531S datasheet section X.Y for Force mode */
#define YT_CDR_CONFIG_REG3 0x1434
#define YT_CDR_CONFIG_REGE 0x163
ngbe_write_phy_reg_sds_ext_yt(hw, 0x3, 0, YT_CDR_CONFIG_REG3);
ngbe_write_phy_reg_sds_ext_yt(hw, 0xe, 0, YT_CDR_CONFIG_REGE);
```
This is a style suggestion, not a requirement, and may be deferred if the
vendor documentation is not publicly available or these values are unique to
this use case.
