Re: [PATCH net-next v2 4/8] r8152: refactor r8157_hw_phy_cfg

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 2

- [Medium] r8159_hw_phy_cfg() (new RTL_VER_17_QFN68/QFN100 callback,
  drivers/net/usb/r8152.c:8332) drops the unconditional…
- [Low] The commit is presented as a refactor ('Split r8157_hw_phy_cfg
  into ... and add sram2 bitwise operation helpers') and only vaguely…
- [Low] The newly added EN_ALDPS_PLLOFF define in the OCP_POWER_CFG block
  uses space alignment while all neighbouring defines in the same block…

Pre-existing issues:
- [Medium] Both r8157_hw_phy_cfg() and the new r8159_hw_phy_cfg() return
  early when `rtl_phy_patch_request(tp, true, true)` fails, after ALDPS…

--

r8152: refactor r8157_hw_phy_cfg

This splits the shared r8157_hw_phy_cfg() into r8157_hw_phy_cfg() for
RTL_VER_16 and a new r8159_hw_phy_cfg() for RTL_VER_17_QFN68/QFN100, adds
sram2_set_bits()/sram2_clr_bits() helpers, and updates PHY parameters for
RTL8156B/8157/8159.  A few questions below.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 6c189790b0ba..679aead731f7 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -655,6 +655,7 @@ enum spd_duplex {
>  /* OCP_POWER_CFG */
>  #define EEE_CLKDIV_EN		0x8000
>  #define EN_ALDPS		0x0004
> +#define EN_ALDPS_PLLOFF         0x0002
>  #define EN_10M_PLLOFF		0x0001

[Severity: Low]
This isn't a bug, but the new define uses spaces for alignment while
EEE_CLKDIV_EN, EN_ALDPS and EN_10M_PLLOFF in the same block use tabs.
Could it use a tab as well?

[ ... ]

> @@ -8075,6 +8086,9 @@ static void r8156b_hw_phy_cfg(struct r8152 *tp)
>  		sram_write(tp, 0x8074, 0x2417);
>  		sram_write(tp, 0x807a, 0x2417);
>  
> +		/* Nway DACONB parameters */
> +		ocp_reg_w0w1(tp, 0xa4ca, 0x6000, 0x0040);
> +
>  		/* XG PLL */
>  		ocp_reg_w0w1(tp, 0xbf84, 0xe000, 0xa000);
>  		break;

[Severity: Low]
The subject says "refactor" and the changelog says "update PHY parameters
for RTL8156B/57/59", but several hardware behaviour changes for already
shipped parts are mixed in with the mechanical sram/sram2 helper
conversions.  Could these be split out, or at least described?

This new Nway DACONB write lands in r8156b_hw_phy_cfg(), reached through
the RTL_VER_13 fallthrough and the RTL_VER_15 case, so it applies to
shipped RTL8156B devices even though the commit is about the
r8157/r8159 split.

In r8157_hw_phy_cfg() below, the 0xd3c8 BIT(0) clear moves out of the
RTL_VER_17 case into the common preamble, so RTL8157 (RTL_VER_16) now
also gets the ALDPS clock-bypass change.

In the r8159_hw_phy_cfg() tail below, EN_10M_CLKDIV changes from set to
cleared and tp->ups_info._10m_ckdiv flips from true to false, which
changes the UPS flag word programmed on every runtime suspend:

	if (tp->ups_info._10m_ckdiv)
		ups_flags |= UPS_FLAGS_EN_10M_CKDIV;

0xa5ea additionally clears BIT(2), sram2 0x8ffe changes from 0xf450 to
0xf4ff, and there are new CLKS latency, CMP_Timer, spdchg_pga1_lpf_cap
and ENET PLL writes.  What failure modes do these address, and which
package or revision were they tested on?  Should any of them carry a
Fixes tag?

> @@ -8151,11 +8165,14 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_PHY_PWR, PFM_PWM_SWITCH);
>  
>  	/* Advanced Power Saving parameter */
> -	ocp_reg_set_bits(tp, 0xa430, BIT(0) | BIT(1));
> +	ocp_reg_set_bits(tp, OCP_POWER_CFG, EN_10M_PLLOFF | EN_ALDPS_PLLOFF);
>  
>  	/* Disable ALDPS force mode */
>  	ocp_reg_clr_bits(tp, 0xa44a, BIT(2));
>  
> +	/* Disable bypass_turn_off_clk_in_aldps */
> +	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
> +
>  	switch (tp->version) {
>  	case RTL_VER_16:
>  		/* XG_INRX parameter */

[ ... ]

> @@ -8277,12 +8294,85 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  		sram2_write_w0w1(tp, 0x807c, 0xff00, 0x5000);
>  		sram2_write_w0w1(tp, 0x809d, 0xff00, 0x5000);
>  		break;
> +	default:
> +		break;
> +	}
> +
> +	if (rtl_phy_patch_request(tp, true, true))
> +		return;

[Severity: Medium]
This isn't a bug introduced by this patch, but the early return without
rollback is now duplicated into the RTL8159 path as well.  By this point
r8153_aldps_en(tp, false) and rtl_eee_enable(tp, false) have already run
and most per-version parameters have been written, and
rtl_phy_patch_request() can fail on timeout or when RTL8152_INACCESSIBLE
is set:

	if (request && wait &&
	    !(ocp_reg_read(tp, OCP_PHY_PATCH_STAT) & PATCH_READY)) {
		dev_err(&tp->intf->dev, "PHY patch request fail\n");
		rtl_phy_patch_request(tp, false, false);
		return -ETIME;
	}

On that path r8153_aldps_en(tp, true), r8152b_enable_fc() and
set_bit(PHY_RESET, &tp->flags) are all skipped, so the device is left
with ALDPS and EEE off, pause advertisement unrefreshed, the freshly
written SRAM/OCP parameters never latched by a PHY reset, and
tp->ups_info out of sync with the hardware.  Would it make sense to undo
the ALDPS/EEE disable before returning?

> +
> +	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4, EEE_SPDWN_EN);
> +
> +	ocp_reg_w0w1(tp, OCP_DOWN_SPEED, EN_EEE_100 | EN_EEE_1000, EN_10M_CLKDIV);
> +
> +	tp->ups_info._10m_ckdiv = true;
> +	tp->ups_info.eee_plloff_100 = false;
> +	tp->ups_info.eee_plloff_giga = false;
> +
> +	ocp_reg_set_bits(tp, OCP_POWER_CFG, EEE_CLKDIV_EN);
> +	tp->ups_info.eee_ckdiv = true;
> +
> +	rtl_phy_patch_request(tp, false, true);
> +
> +	rtl_green_en(tp, test_bit(GREEN_ETHERNET, &tp->flags));
> +
> +	ocp_reg_clr_bits(tp, 0xa428, BIT(9));
> +	ocp_reg_clr_bits(tp, 0xa5ea, BIT(0) | BIT(1));
> +	tp->ups_info.lite_mode = 0;
> +
> +	if (tp->eee_en)
> +		rtl_eee_enable(tp, true);
> +
> +	r8153_aldps_en(tp, true);
> +	r8152b_enable_fc(tp);
> +
> +	set_bit(PHY_RESET, &tp->flags);
> +}
> +
> +static void r8159_hw_phy_cfg(struct r8152 *tp)
> +{
> +	u16 data;
> +
> +	r8156b_wait_loading_flash(tp);
> +
> +	ocp_word_test_and_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);
> +
> +	data = r8153_phy_status(tp, 0);
> +	switch (data) {
> +	case PHY_STAT_EXT_INIT:
> +		rtl8152_apply_firmware(tp, true);
> +		ocp_reg_clr_bits(tp, 0xa466, BIT(0));
> +		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
> +		break;
> +	case PHY_STAT_LAN_ON:
> +	case PHY_STAT_PWRDN:
> +	default:
> +		rtl8152_apply_firmware(tp, false);
> +		break;
> +	}
>  
> +	r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
> +
> +	r8153_aldps_en(tp, false);
> +
> +	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
> +	WARN_ON_ONCE(data != PHY_STAT_LAN_ON);

[Severity: Medium]
Is the missing rtl_eee_enable(tp, false) here intentional?  Before the
split, RTL_VER_17_QFN68/QFN100 went through the shared
r8157_hw_phy_cfg() which did:

	r8153_aldps_en(tp, false);
	rtl_eee_enable(tp, false);

The retained r8157_hw_phy_cfg() still has both calls, and
r8156b_hw_phy_cfg() carries the same step with the comment
"/* disable EEE before updating the PHY parameters */", but
r8159_hw_phy_cfg() only disables ALDPS, so the whole RTL8159
SRAM/SRAM2/OCP block and the rtl_phy_patch_request() window run with EEE
still enabled.

The tail of the function is enable-only:

	if (tp->eee_en)
		rtl_eee_enable(tp, true);

so when the user has turned EEE off via r8152_set_eee() (tp->eee_en ==
false), nothing clears OCP_EEE_CFG/OCP_EEE_ADV after hw_phy_cfg() runs
again from rtl_hw_phy_work_func_t() following a power cut, firmware
reload or resume.  Can hardware EEE state and the tp->eee_en /
tp->ups_info.eee shadow diverge in that case?  rtl_eee_enable() does
handle RTL_VER_17_QFN68/QFN100, so this does not look like a hardware
limitation.

[ ... ]

> @@ -8460,8 +8563,15 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  		sram2_write(tp, 0x8ff8, 0xaa5a);
>  
>  		sram2_write_w0w1(tp, 0x88d5, 0xff00, 0x0200);
> -		break;
>  
> +		/* spdchg_pga1_lpf_cap */
> +		sram_write_w0w1(tp, 0x84bb, 0xff00, 0x0a00);
> +		sram_write_w0w1(tp, 0x84c0, 0xff00, 0x1600);
> +
> +		/* ENET PLL jitter improvement */
> +		ocp_reg_w0w1(tp, 0xbf8a, 0xfc00, 0x2000);
> +		ocp_reg_set_bits(tp, 0xbf88, BIT(2));
> +		break;
>  	default:
>  		break;
>  	}
> @@ -8471,9 +8581,9 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  
>  	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4, EEE_SPDWN_EN);
>  
> -	ocp_reg_w0w1(tp, OCP_DOWN_SPEED, EN_EEE_100 | EN_EEE_1000, EN_10M_CLKDIV);
> -
> -	tp->ups_info._10m_ckdiv = true;
> +	ocp_reg_clr_bits(tp, OCP_DOWN_SPEED,
> +			 EN_EEE_100 | EN_EEE_1000 | EN_10M_CLKDIV);
> +	tp->ups_info._10m_ckdiv = false;
>  	tp->ups_info.eee_plloff_100 = false;
>  	tp->ups_info.eee_plloff_giga = false;
>  
> @@ -8485,7 +8595,7 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  	rtl_green_en(tp, test_bit(GREEN_ETHERNET, &tp->flags));
>  
>  	ocp_reg_clr_bits(tp, 0xa428, BIT(9));
> -	ocp_reg_clr_bits(tp, 0xa5ea, BIT(0) | BIT(1));
> +	ocp_reg_clr_bits(tp, 0xa5ea, BIT(0) | BIT(1) | BIT(2));
>  	tp->ups_info.lite_mode = 0;
>  
>  	if (tp->eee_en)
> @@ -10284,7 +10394,7 @@ static int rtl_ops_init(struct r8152 *tp)
>  		ops->eee_get		= r8153_get_eee;
>  		ops->eee_set		= r8152_set_eee;
>  		ops->in_nway		= rtl8153_in_nway;
> -		ops->hw_phy_cfg		= r8157_hw_phy_cfg;
> +		ops->hw_phy_cfg		= r8159_hw_phy_cfg;
>  		ops->autosuspend_en	= rtl8157_runtime_enable;
>  		ops->change_mtu		= rtl8156_change_mtu;
>  		tp->rx_buf_sz		= 48 * 1024;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903072852.58695-1-nic_swsd%40realtek.com