Re: [PATCH net-next v3 2/3] r8152: Add support for the RTL8159 chip

[Jakub Kicinski <kuba@kernel.org>]

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: Add support for the RTL8159 chip

This patch adds support for the RTL8159 chip as RTL_VER_17, reusing the
RTL8157 initialization and USB power management paths. The bulk of the
new code lives in r8157_hw_phy_cfg() to configure the RTL8159 PHY, and
the USB product ID 0x815a is added to the device table.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 05abfab1df94..a2cdd57558c7 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -7996,7 +8005,7 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  	/* Advanced Power Saving parameter */
>  	ocp_reg_set_bits(tp, 0xa430, BIT(0) | BIT(1));
>  
> -	/* aldpsce force mode */
> +	/* Disable ALDPS force mode */
>  	ocp_reg_clr_bits(tp, 0xa44a, BIT(2));
>  
>  	switch (tp->version) {
> @@ -8120,6 +8129,190 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  		sram2_write_w0w1(tp, 0x807c, 0xff00, 0x5000);
>  		sram2_write_w0w1(tp, 0x809d, 0xff00, 0x5000);
>  		break;
> +
> +	case RTL_VER_17:
> +		/* Disable bypass turn off clk in ALDPS */
> +		 ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
         ^

Is the leading space before ocp_byte_clr_bits here intentional?  The rest
of the switch statement uses a single tab for indentation.  checkpatch.pl
should catch this.

> +
> +		/* Power level tuning
> +		 * test mode power level
> +		 */
> +		sram_write_w0w1(tp, 0x8415, 0xff00, 0x9300);
> +		/* normal link power level 10G, 5G, 2.5G */
> +		sram_write_w0w1(tp, 0x81a3, 0xff00, 0x0f00);
> +		sram_write_w0w1(tp, 0x81ae, 0xff00, 0x0f00);
> +		sram_write_w0w1(tp, 0x81b9, 0xff00, 0xb900);
> +		/* nomal link TX filter */
              ^^^^^

Small typo, "nomal" -> "normal".

> +		sram2_write_w0w1(tp, 0x83b0, 0x0e00, 0);
> +		sram2_write_w0w1(tp, 0x83c5, 0x0e00, 0);
> +		sram2_write_w0w1(tp, 0x83da, 0x0e00, 0);
> +		sram2_write_w0w1(tp, 0x83ef, 0x0e00, 0);

[ ... ]

> +		/* Slave about EC mu of datamode AAGC  and DAC BG */
> +		sram2_write_w0w1(tp, 0x88ac, 0xff00, 0x2300);
> +		/* improve UBE */
> +		ocp_reg_set_bits(tp, 0xbf0c, 0x7 << 11);
> +		/* close Sparse NEC, improve connect 5EUU calble performace */
                                                      ^^^^^  ^^^^^^^^^

Two typos here, "calble" -> "cable" and "performace" -> "performance".

> +		sram2_write_w0w1(tp, 0x88de, 0xff00, 0);
> +		/* 5G slave compatibility issue (will include in v10) */
                                                 ^^^^^^^^^^^^^^^^^^

Is this "will include in v10" comment a reference to an internal Realtek
source-tree revision?  It does not appear to have meaning in the upstream
tree and could be dropped or rephrased.

> +		sram2_write(tp, 0x80b4, 0x5195);

[ ... ]

> +		sram2_write_w0w1(tp, 0x88d5, 0xff00, 0x0200);
> +		break;
> +
>  	default:
>  		break;
>  	}

[ ... ]

> @@ -8155,6 +8348,18 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  	set_bit(PHY_RESET, &tp->flags);
>  }
>  
> +static int r8159_wait_backup_restore(struct r8152 *tp)
> +{
> +	u32 ocp_data;
> +
> +	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> +	if (!(ocp_data & PCUT_STATUS))
> +		return 0;
> +
> +	return poll_timeout_us(ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_GPHY_CTRL),
> +			       ocp_data & BACKUP_RESTRORE, 200, 20000, false);
> +}
> +
>  static void r8156_init(struct r8152 *tp)
>  {
>  	u32 ocp_data;

[ ... ]

> @@ -8201,6 +8407,11 @@ static void r8156_init(struct r8152 *tp)
>  			return;
>  	}
>  
> +	if (tp->version == RTL_VER_17 && r8159_wait_backup_restore(tp)) {
> +		dev_err(&tp->intf->dev, "init failed, backup-restore timed out\n");
> +		return;
> +	}
> +

On timeout this returns early from r8156_init() and skips the remainder of
the init sequence, including r8157_u2p3en()/r8157_power_cut_en(),
r8156_ups_en(), r8153_queue_wake(), rtl_runtime_suspend_enable(),
r8156_mac_clk_spd(), the rx aggregation / 16-byte Rx descriptor
configuration, r8156_mdio_force_mode(), rtl_tally_reset() and the
tp->coalesce assignment.

r8156_init() is void and rtl8152_probe_once() does not check init status,
so probe continues and register_netdev() runs after this early return.

Should this path also set RTL8152_INACCESSIBLE on tp->flags, or should the
init callback be changed to return an error that probe can honor, so that
a half-initialized device is not exposed to userspace?

>  	data = r8153_phy_status(tp, 0);
>  	if (data == PHY_STAT_EXT_INIT) {
>  		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));

[ ... ]

> @@ -8274,7 +8485,10 @@ static void r8156_init(struct r8152 *tp)
>  	set_bit(GREEN_ETHERNET, &tp->flags);
>  
>  	/* rx aggregation / 16 bytes Rx descriptor */
> -	if (tp->version == RTL_VER_16)
> +	if (tp->version == RTL_VER_17)
> +		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> +				  RX_AGG_DISABLE | RX_DESC_16B | BIT(11));
                                                                 ^^^^^^

What does BIT(11) in USB_USB_CTRL correspond to here, and is it
RTL8159-specific?

The commit message says the RTL8159 "reuses hardware features of the
RTL8157 (RTL_VER_16)", but this path clears an extra bit that the
RTL_VER_16 path does not.  Since there is no symbolic name or comment,
it is hard to tell whether RTL_VER_16 is missing this clear, whether
RTL_VER_17 should not be clearing it, or whether it really is an
RTL8159-only quirk.

Could a named define and/or a short comment be added to describe what
this bit does?

> +	else if (tp->version == RTL_VER_16)
>  		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL, RX_AGG_DISABLE | RX_DESC_16B);
>  	else
>  		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL, RX_AGG_DISABLE | RX_ZERO_EN);

[ ... ]