Re: [PATCH v4 03/20] net/ngbe: add missing CDR config for YT PHY

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 11 May 2026 18:35:45 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> 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: stable@dpdk.org
> 
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---

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.