Re: [PATCH net-next v7 05/10] r8152: fix up and down register settings for RTL8156/8156B/8157/8159

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8152: fix up and down register settings for RTL8156/8156B/8157/8159

This commit corrects several up/down and OOB register settings for the
RTL8156, RTL8156B, RTL8157 and RTL8159 chips, following the earlier
per-generation split of the up/down callbacks. It changes the PLA_RCR1,
USB_SPEED_OPTION, Clear_SDR, PLA_BDC_CR/PLA_OOB_CTRL and OOB
PLA_RMS/PLA_MTPS settings, and drops PLA_MCU_SPDWN_EN handling from
rtl8157_up()/rtl8157_down().

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index b245b63fb6aaf..045460a35f383 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -6938,8 +6937,18 @@ static void r8156_enter_oob(struct r8152 *tp)
>  	rtl_disable(tp);
>  	rtl_reset_bmu(tp);
>  
> -	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, 1522);
> -	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, MTPS_DEFAULT);
> +	switch (tp->version) {
> +	case RTL_VER_16:
> +	case RTL_VER_17_QFN68:
> +	case RTL_VER_17_QFN100:

[Severity: Medium]
This patch has three Fixes: tags:

  Fixes: 195aae321c82 ("r8152: support new chips")
  Fixes: fd3c7d080df5 ("r8152: Add support for the RTL8157 hardware")
  Fixes: ad2a55b63b41 ("r8152: Add support for the RTL8159 chip")

Will it apply and build on the trees those tags point to?

RTL_VER_17_QFN68 and RTL_VER_17_QFN100 come from an earlier patch in this
series, "r8152: split RTL_VER_17 into QFN68 and QFN100 package variants".
Before that patch the enum only has RTL_VER_17.

rtl8157_up() and rtl8157_down() also only exist after "r8152: split
rtl8156_enable/up/down into per-generation functions", which is described
as no functional change. Before that patch there is one shared
rtl8156_up() with tp->version >= RTL_VER_16 and tp->version < RTL_VER_16
checks.

So a stable backport would either conflict or need both non-fix patches
as prerequisites. One of them is the package detection change, which
makes an unrecognised PLA_PKG_DET value fall back to RTL_VER_UNKNOWN.

Could these fixes go to net separately, written against the pre-refactor
code? Or could the commit message at least name the prerequisites?

> +		ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, 1526);
> +		ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, 10 * 1024 / 64);
> +		break;

[ ... ]

> @@ -6997,6 +7008,11 @@ static void rtl8157_up(struct r8152 *tp)
>  	}
>  
>  	r8153_aldps_en(tp, true);
> +
> +	/* Enable Clear_SDR */
> +	ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xd378, BIT(7));
> +	/* Clear SDR bit */
> +	ocp_word_clr_bits(tp, MCU_TYPE_USB, 0xcd06, BIT(15));
>  }

[Severity: High]
The commit message says:

  - RTL8156B (VER_13/15) need Clear_SDR enabled on up, which nothing
    ever did for them.

Is the RTL8156B part missing from this patch? These are the only
Clear_SDR writes the patch adds. rtl8157_up() is only used for
RTL_VER_16, RTL_VER_17_QFN68 and RTL_VER_17_QFN100.

In rtl_ops_init(), RTL_VER_13 and RTL_VER_15 are bound to rtl8156_up():

	case RTL_VER_12:
	case RTL_VER_13:
		tp->support_2500full	= 1;
		fallthrough;
	case RTL_VER_15:
		...
		ops->up			= rtl8156_up;

This patch does not change rtl8156_up(), and it still writes no
Clear_SDR register:

	r8156_exit_oob(tp);

	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN);

	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
			  RG_PWRDN_EN | ALL_SPEED_OFF);

	ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, 0x00600400);
	...
	r8153_aldps_en(tp, true);
	r8153_u2p3en(tp, true);

So every open or resume on an RTL8156B goes through ops->up ->
rtl8156_up() without the Clear_SDR enable. The later patches in the
series don't add it to rtl8156_up() either.

Should rtl8156_up() get the RTL8156B Clear_SDR writes under an
RTL_VER_13/RTL_VER_15 check, or should this bullet come out of the commit
message? The 0xd378/0xcd06 writes used for RTL8157/8159 can't be assumed
to apply to RTL8156B.