[PATCH net-next v2 0/2] r8152: reduce control transfer

[PATCH net-next v2 0/2] r8152: reduce control transfer

[Hayes Wang]

v2:
For patch #1, fix the typo of the commit message.

v1:
The two patches are used to reduce the number of control transfer when
access the registers in bulk.

Hayes Wang (2):
  r8152: adjust generic_ocp_write function
  r8152: set bp in bulk

 drivers/net/usb/r8152.c | 104 +++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 61 deletions(-)

-- 
2.41.0

[PATCH net-next v2 1/2] r8152: adjust generic_ocp_write function

[Hayes Wang]

Reduce the control transfer if all bytes of first or the last DWORD are
written.

The original method is to split the control transfer into three parts
(the first DWORD, middle continuous data, and the last DWORD). However,
they could be combined if whole bytes of the first DWORD or last DWORD
are written. That is, the first DWORD or the last DWORD could be combined
with the middle continuous data, if the byte_en is 0xff.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0738baa5b82e..f6578a99dbac 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1314,16 +1314,24 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
 	byteen_end = byteen & BYTE_EN_END_MASK;
 
 	byen = byteen_start | (byteen_start << 4);
-	ret = set_registers(tp, index, type | byen, 4, data);
-	if (ret < 0)
-		goto error1;
 
-	index += 4;
-	data += 4;
-	size -= 4;
+	/* Split the first DWORD if the byte_en is not 0xff */
+	if (byen != BYTE_EN_DWORD) {
+		ret = set_registers(tp, index, type | byen, 4, data);
+		if (ret < 0)
+			goto error1;
 
-	if (size) {
+		index += 4;
+		data += 4;
 		size -= 4;
+	}
+
+	if (size) {
+		byen = byteen_end | (byteen_end >> 4);
+
+		/* Split the last DWORD if the byte_en is not 0xff */
+		if (byen != BYTE_EN_DWORD)
+			size -= 4;
 
 		while (size) {
 			if (size > limit) {
@@ -1350,10 +1358,9 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
 			}
 		}
 
-		byen = byteen_end | (byteen_end >> 4);
-		ret = set_registers(tp, index, type | byen, 4, data);
-		if (ret < 0)
-			goto error1;
+		/* Set the last DWORD */
+		if (byen != BYTE_EN_DWORD)
+			ret = set_registers(tp, index, type | byen, 4, data);
 	}
 
 error1:
-- 
2.41.0

Re: [PATCH net-next v2 1/2] r8152: adjust generic_ocp_write function

[Andrew Lunn]

On Wed, Jul 26, 2023 at 11:08:07AM +0800, Hayes Wang wrote:
> Reduce the control transfer if all bytes of first or the last DWORD are
> written.
> 
> The original method is to split the control transfer into three parts
> (the first DWORD, middle continuous data, and the last DWORD). However,
> they could be combined if whole bytes of the first DWORD or last DWORD
> are written. That is, the first DWORD or the last DWORD could be combined
> with the middle continuous data, if the byte_en is 0xff.

How often is byte_en 0xff? Do you have some benchmark numbers to show
it is worth the complexity?

   Andrew

RE: [PATCH net-next v2 1/2] r8152: adjust generic_ocp_write function

[Hayes Wang]

Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, July 26, 2023 4:37 PM
[...]
> How often is byte_en 0xff? Do you have some benchmark numbers to show
> it is worth the complexity?

It is usually used for writing firmware.
The firmware contains several blocks of continuous registers.

I think it is worth, even this only saves several numbers of control transfer.
This patch could replace 3 control transfers with 1 control transfer.
If you could do it with one step, why do you use 3 steps?
Besides, a control transfer is a complex process.
I think this could reduce the loading of both software and hardware.

Best Regards,
Hayes

[PATCH net-next v2 2/2] r8152: set bp in bulk

[Hayes Wang]

PLA_BP_0 ~ PLA_BP_15 (0xfc28 ~ 0xfc46) are continuous registers, so we
could combine the control transfers into one control transfer.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 75 ++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 50 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f6578a99dbac..db9897e825b4 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3978,29 +3978,10 @@ static void rtl_reset_bmu(struct r8152 *tp)
 /* Clear the bp to stop the firmware before loading a new one */
 static void rtl_clear_bp(struct r8152 *tp, u16 type)
 {
-	switch (tp->version) {
-	case RTL_VER_01:
-	case RTL_VER_02:
-	case RTL_VER_07:
-		break;
-	case RTL_VER_03:
-	case RTL_VER_04:
-	case RTL_VER_05:
-	case RTL_VER_06:
-		ocp_write_byte(tp, type, PLA_BP_EN, 0);
-		break;
-	case RTL_VER_14:
-		ocp_write_word(tp, type, USB_BP2_EN, 0);
+	u16 bp[16] = {0};
+	u16 bp_num;
 
-		ocp_write_word(tp, type, USB_BP_8, 0);
-		ocp_write_word(tp, type, USB_BP_9, 0);
-		ocp_write_word(tp, type, USB_BP_10, 0);
-		ocp_write_word(tp, type, USB_BP_11, 0);
-		ocp_write_word(tp, type, USB_BP_12, 0);
-		ocp_write_word(tp, type, USB_BP_13, 0);
-		ocp_write_word(tp, type, USB_BP_14, 0);
-		ocp_write_word(tp, type, USB_BP_15, 0);
-		break;
+	switch (tp->version) {
 	case RTL_VER_08:
 	case RTL_VER_09:
 	case RTL_VER_10:
@@ -4008,32 +3989,31 @@ static void rtl_clear_bp(struct r8152 *tp, u16 type)
 	case RTL_VER_12:
 	case RTL_VER_13:
 	case RTL_VER_15:
-	default:
 		if (type == MCU_TYPE_USB) {
 			ocp_write_word(tp, MCU_TYPE_USB, USB_BP2_EN, 0);
-
-			ocp_write_word(tp, MCU_TYPE_USB, USB_BP_8, 0);
-			ocp_write_word(tp, MCU_TYPE_USB, USB_BP_9, 0);
-			ocp_write_word(tp, MCU_TYPE_USB, USB_BP_10, 0);
-			ocp_write_word(tp, MCU_TYPE_USB, USB_BP_11, 0);
-			ocp_write_word(tp, MCU_TYPE_USB, USB_BP_12, 0);
-			ocp_write_word(tp, MCU_TYPE_USB, USB_BP_13, 0);
-			ocp_write_word(tp, MCU_TYPE_USB, USB_BP_14, 0);
-			ocp_write_word(tp, MCU_TYPE_USB, USB_BP_15, 0);
-		} else {
-			ocp_write_byte(tp, MCU_TYPE_PLA, PLA_BP_EN, 0);
+			bp_num = 16;
+			break;
 		}
+		fallthrough;
+	case RTL_VER_03:
+	case RTL_VER_04:
+	case RTL_VER_05:
+	case RTL_VER_06:
+		ocp_write_byte(tp, type, PLA_BP_EN, 0);
+		fallthrough;
+	case RTL_VER_01:
+	case RTL_VER_02:
+	case RTL_VER_07:
+		bp_num = 8;
+		break;
+	case RTL_VER_14:
+	default:
+		ocp_write_word(tp, type, USB_BP2_EN, 0);
+		bp_num = 16;
 		break;
 	}
 
-	ocp_write_word(tp, type, PLA_BP_0, 0);
-	ocp_write_word(tp, type, PLA_BP_1, 0);
-	ocp_write_word(tp, type, PLA_BP_2, 0);
-	ocp_write_word(tp, type, PLA_BP_3, 0);
-	ocp_write_word(tp, type, PLA_BP_4, 0);
-	ocp_write_word(tp, type, PLA_BP_5, 0);
-	ocp_write_word(tp, type, PLA_BP_6, 0);
-	ocp_write_word(tp, type, PLA_BP_7, 0);
+	generic_ocp_write(tp, PLA_BP_0, BYTE_EN_DWORD, bp_num << 1, bp, type);
 
 	/* wait 3 ms to make sure the firmware is stopped */
 	usleep_range(3000, 6000);
@@ -5007,10 +4987,9 @@ static void rtl8152_fw_phy_nc_apply(struct r8152 *tp, struct fw_phy_nc *phy)
 
 static void rtl8152_fw_mac_apply(struct r8152 *tp, struct fw_mac *mac)
 {
-	u16 bp_en_addr, bp_index, type, bp_num, fw_ver_reg;
+	u16 bp_en_addr, type, fw_ver_reg;
 	u32 length;
 	u8 *data;
-	int i;
 
 	switch (__le32_to_cpu(mac->blk_hdr.type)) {
 	case RTL_FW_PLA:
@@ -5052,12 +5031,8 @@ static void rtl8152_fw_mac_apply(struct r8152 *tp, struct fw_mac *mac)
 	ocp_write_word(tp, type, __le16_to_cpu(mac->bp_ba_addr),
 		       __le16_to_cpu(mac->bp_ba_value));
 
-	bp_index = __le16_to_cpu(mac->bp_start);
-	bp_num = __le16_to_cpu(mac->bp_num);
-	for (i = 0; i < bp_num; i++) {
-		ocp_write_word(tp, type, bp_index, __le16_to_cpu(mac->bp[i]));
-		bp_index += 2;
-	}
+	generic_ocp_write(tp, __le16_to_cpu(mac->bp_start), BYTE_EN_DWORD,
+			  __le16_to_cpu(mac->bp_num) << 1, mac->bp, type);
 
 	bp_en_addr = __le16_to_cpu(mac->bp_en_addr);
 	if (bp_en_addr)
-- 
2.41.0

Re: [PATCH net-next v2 2/2] r8152: set bp in bulk

[Andrew Lunn]

> -	ocp_write_word(tp, type, PLA_BP_0, 0);
> -	ocp_write_word(tp, type, PLA_BP_1, 0);
> -	ocp_write_word(tp, type, PLA_BP_2, 0);
> -	ocp_write_word(tp, type, PLA_BP_3, 0);
> -	ocp_write_word(tp, type, PLA_BP_4, 0);
> -	ocp_write_word(tp, type, PLA_BP_5, 0);
> -	ocp_write_word(tp, type, PLA_BP_6, 0);
> -	ocp_write_word(tp, type, PLA_BP_7, 0);
> +	generic_ocp_write(tp, PLA_BP_0, BYTE_EN_DWORD, bp_num << 1, bp, type);
>  
>  	/* wait 3 ms to make sure the firmware is stopped */
>  	usleep_range(3000, 6000);

How much time do you save compared to this 3ms - 6ms sleep?

    Andrew

RE: [PATCH net-next v2 2/2] r8152: set bp in bulk

[Hayes Wang]

Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, July 26, 2023 4:39 PM
[...]
> >       /* wait 3 ms to make sure the firmware is stopped */
> >       usleep_range(3000, 6000);
> 
> How much time do you save compared to this 3ms - 6ms sleep?

I think it depends on the CPU and the USB host controller.
Take the PC produced this year for example, I think it saves less than 3ms normally.
However, I think saving the number of control transfer is better for both software and hardware.

Best Regards,
Hayes

Re: [PATCH net-next v2 0/2] r8152: reduce control transfer

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 26 Jul 2023 11:08:06 +0800 you wrote:
> v2:
> For patch #1, fix the typo of the commit message.
> 
> v1:
> The two patches are used to reduce the number of control transfer when
> access the registers in bulk.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/2] r8152: adjust generic_ocp_write function
    https://git.kernel.org/netdev/net-next/c/57df0fb9d511
  - [net-next,v2,2/2] r8152: set bp in bulk
    https://git.kernel.org/netdev/net-next/c/e5c266a61186

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html