netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/5] r8152 changes
@ 2020-11-03 19:22 Marek Behún
  2020-11-03 19:22 ` [PATCH net-next 1/5] r8152: use generic USB macros to define product table Marek Behún
                   ` (4 more replies)
  0 siblings, 5 replies; 26+ messages in thread
From: Marek Behún @ 2020-11-03 19:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-usb, Hayes Wang, Marek Behún

Hi,

these are some changes for r8152.

At first I just wanted to add support for rtl8156 (which supports
2500baseT), by porting from Realtek's out-of-tree driver.

It instead turned out into cosmetic changes first, though.

Please review this. I would much like to try to port rtl8156 after
these patches are merged, so that _modify functions can be used.
Thanks.

Marek

Marek Behún (5):
  r8152: use generic USB macros to define product table
  r8152: cosmetic improvement of product table macro
  r8152: add MCU typed read/write functions
  r8152: rename r8153_phy_status to r8153_phy_status_wait
  r8152: use *_modify helpers instead of read/write combos

 drivers/net/usb/r8152.c | 1156 +++++++++++++--------------------------
 1 file changed, 387 insertions(+), 769 deletions(-)


base-commit: 6d89076e6ef09337a29a7b1ea4fdf2d892be9650
-- 
2.26.2


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH net-next 1/5] r8152: use generic USB macros to define product table
  2020-11-03 19:22 [PATCH net-next 0/5] r8152 changes Marek Behún
@ 2020-11-03 19:22 ` Marek Behún
  2020-11-04  1:57   ` Hayes Wang
  2020-11-03 19:22 ` [PATCH net-next 2/5] r8152: cosmetic improvement of product table macro Marek Behún
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-03 19:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-usb, Hayes Wang, Marek Behún

We can now use macros USB_DEVICE_INTERFACE_CLASS and
USB_DEVICE_AND_INTERFACE_INFO to define r8152 product table.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/usb/r8152.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index b1770489aca5..85dda591c838 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -6862,20 +6862,12 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 }
 
 #define REALTEK_USB_DEVICE(vend, prod)	\
-	.match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
-		       USB_DEVICE_ID_MATCH_INT_CLASS, \
-	.idVendor = (vend), \
-	.idProduct = (prod), \
-	.bInterfaceClass = USB_CLASS_VENDOR_SPEC \
+	USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC) \
 }, \
 { \
-	.match_flags = USB_DEVICE_ID_MATCH_INT_INFO | \
-		       USB_DEVICE_ID_MATCH_DEVICE, \
-	.idVendor = (vend), \
-	.idProduct = (prod), \
-	.bInterfaceClass = USB_CLASS_COMM, \
-	.bInterfaceSubClass = USB_CDC_SUBCLASS_ETHERNET, \
-	.bInterfaceProtocol = USB_CDC_PROTO_NONE
+	USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_COMM, \
+				      USB_CDC_SUBCLASS_ETHERNET, \
+				      USB_CDC_PROTO_NONE)
 
 /* table of devices that work with this driver */
 static const struct usb_device_id rtl8152_table[] = {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH net-next 2/5] r8152: cosmetic improvement of product table macro
  2020-11-03 19:22 [PATCH net-next 0/5] r8152 changes Marek Behún
  2020-11-03 19:22 ` [PATCH net-next 1/5] r8152: use generic USB macros to define product table Marek Behún
@ 2020-11-03 19:22 ` Marek Behún
  2020-11-03 19:22 ` [PATCH net-next 3/5] r8152: add MCU typed read/write functions Marek Behún
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 26+ messages in thread
From: Marek Behún @ 2020-11-03 19:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-usb, Hayes Wang, Marek Behún

Improve macro REALTEK_USB_DEVICE cosmetically so that it defines
complete members.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/usb/r8152.c | 44 ++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 85dda591c838..96bef1c027f2 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -6862,33 +6862,31 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 }
 
 #define REALTEK_USB_DEVICE(vend, prod)	\
-	USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC) \
-}, \
-{ \
-	USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_COMM, \
+	{ USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC) }, \
+	{ USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_COMM, \
 				      USB_CDC_SUBCLASS_ETHERNET, \
-				      USB_CDC_PROTO_NONE)
+				      USB_CDC_PROTO_NONE) }
 
 /* table of devices that work with this driver */
 static const struct usb_device_id rtl8152_table[] = {
-	{REALTEK_USB_DEVICE(VENDOR_ID_REALTEK, 0x8050)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_REALTEK, 0x8152)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_REALTEK, 0x8153)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_MICROSOFT, 0x07ab)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_MICROSOFT, 0x07c6)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_MICROSOFT, 0x0927)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_SAMSUNG, 0xa101)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x304f)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x3062)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x3069)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x3082)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7205)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x720c)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7214)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0xa387)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_LINKSYS, 0x0041)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_NVIDIA,  0x09ff)},
-	{REALTEK_USB_DEVICE(VENDOR_ID_TPLINK,  0x0601)},
+	REALTEK_USB_DEVICE(VENDOR_ID_REALTEK, 0x8050),
+	REALTEK_USB_DEVICE(VENDOR_ID_REALTEK, 0x8152),
+	REALTEK_USB_DEVICE(VENDOR_ID_REALTEK, 0x8153),
+	REALTEK_USB_DEVICE(VENDOR_ID_MICROSOFT, 0x07ab),
+	REALTEK_USB_DEVICE(VENDOR_ID_MICROSOFT, 0x07c6),
+	REALTEK_USB_DEVICE(VENDOR_ID_MICROSOFT, 0x0927),
+	REALTEK_USB_DEVICE(VENDOR_ID_SAMSUNG, 0xa101),
+	REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x304f),
+	REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x3062),
+	REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x3069),
+	REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x3082),
+	REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7205),
+	REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x720c),
+	REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7214),
+	REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0xa387),
+	REALTEK_USB_DEVICE(VENDOR_ID_LINKSYS, 0x0041),
+	REALTEK_USB_DEVICE(VENDOR_ID_NVIDIA,  0x09ff),
+	REALTEK_USB_DEVICE(VENDOR_ID_TPLINK,  0x0601),
 	{}
 };
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-03 19:22 [PATCH net-next 0/5] r8152 changes Marek Behún
  2020-11-03 19:22 ` [PATCH net-next 1/5] r8152: use generic USB macros to define product table Marek Behún
  2020-11-03 19:22 ` [PATCH net-next 2/5] r8152: cosmetic improvement of product table macro Marek Behún
@ 2020-11-03 19:22 ` Marek Behún
  2020-11-03 21:47   ` Vladimir Oltean
  2020-11-03 19:22 ` [PATCH net-next 4/5] r8152: rename r8153_phy_status to r8153_phy_status_wait Marek Behún
  2020-11-03 19:22 ` [PATCH net-next 5/5] r8152: use *_modify helpers instead of read/write combos Marek Behún
  4 siblings, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-03 19:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-usb, Hayes Wang, Marek Behún

Add pla_ and usb_ prefixed versions of ocp_read_* and ocp_write_*
functions. This saves us from always writing MCU_TYPE_PLA/MCU_TYPE_USB
as parameter.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/usb/r8152.c | 673 ++++++++++++++++++++--------------------
 1 file changed, 338 insertions(+), 335 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 96bef1c027f2..905859309db4 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1305,18 +1305,37 @@ static void ocp_write_byte(struct r8152 *tp, u16 type, u16 index, u32 data)
 	generic_ocp_write(tp, index, byen, sizeof(tmp), &tmp, type);
 }
 
+#define DEFINE_FUNCS_FOR_MCU_TYPE(_p, _mcutype, _t, _n)			\
+static inline _t _p ## _ocp_read_ ## _n(struct r8152 *tp, u16 index)	\
+{									\
+	return ocp_read_ ## _n(tp, _mcutype, index);			\
+}									\
+static inline void _p ## _ocp_write_ ## _n(struct r8152 *tp, u16 index,	\
+					   _t data)			\
+{									\
+	ocp_write_ ## _n(tp, _mcutype, index, data);			\
+}
+
+DEFINE_FUNCS_FOR_MCU_TYPE(pla, MCU_TYPE_PLA, u8, byte)
+DEFINE_FUNCS_FOR_MCU_TYPE(pla, MCU_TYPE_PLA, u16, word)
+DEFINE_FUNCS_FOR_MCU_TYPE(pla, MCU_TYPE_PLA, u32, dword)
+
+DEFINE_FUNCS_FOR_MCU_TYPE(usb, MCU_TYPE_USB, u8, byte)
+DEFINE_FUNCS_FOR_MCU_TYPE(usb, MCU_TYPE_USB, u16, word)
+DEFINE_FUNCS_FOR_MCU_TYPE(usb, MCU_TYPE_USB, u32, dword)
+
 static u16 ocp_reg_read(struct r8152 *tp, u16 addr)
 {
 	u16 ocp_base, ocp_index;
 
 	ocp_base = addr & 0xf000;
 	if (ocp_base != tp->ocp_base) {
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_OCP_GPHY_BASE, ocp_base);
+		pla_ocp_write_word(tp, PLA_OCP_GPHY_BASE, ocp_base);
 		tp->ocp_base = ocp_base;
 	}
 
 	ocp_index = (addr & 0x0fff) | 0xb000;
-	return ocp_read_word(tp, MCU_TYPE_PLA, ocp_index);
+	return pla_ocp_read_word(tp, ocp_index);
 }
 
 static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
@@ -1325,12 +1344,12 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
 
 	ocp_base = addr & 0xf000;
 	if (ocp_base != tp->ocp_base) {
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_OCP_GPHY_BASE, ocp_base);
+		pla_ocp_write_word(tp, PLA_OCP_GPHY_BASE, ocp_base);
 		tp->ocp_base = ocp_base;
 	}
 
 	ocp_index = (addr & 0x0fff) | 0xb000;
-	ocp_write_word(tp, MCU_TYPE_PLA, ocp_index, data);
+	pla_ocp_write_word(tp, ocp_index, data);
 }
 
 static inline void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
@@ -1405,9 +1424,9 @@ static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
 
 	memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
 
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_CONFIG);
+	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_CONFIG);
 	pla_ocp_write(tp, PLA_IDR, BYTE_EN_SIX_BYTES, 8, addr->sa_data);
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_NORAML);
+	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
 
 	mutex_unlock(&tp->control);
 
@@ -1438,10 +1457,10 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 		mac_strlen = 0x16;
 	} else {
 		/* test for -AD variant of RTL8153 */
-		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+		ocp_data = usb_ocp_read_word(tp, USB_MISC_0);
 		if ((ocp_data & AD_MASK) == 0x1000) {
 			/* test for MAC address pass-through bit */
-			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
+			ocp_data = usb_ocp_read_byte(tp, EFUSE);
 			if ((ocp_data & PASS_THRU_MASK) != 1) {
 				netif_dbg(tp, probe, tp->netdev,
 						"No efuse for RTL8153-AD MAC pass through\n");
@@ -1449,7 +1468,7 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 			}
 		} else {
 			/* test for RTL8153-BND and RTL8153-BD */
-			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
+			ocp_data = usb_ocp_read_byte(tp, USB_MISC_1);
 			if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK) == 0) {
 				netif_dbg(tp, probe, tp->netdev,
 						"Invalid variant for MAC pass through\n");
@@ -2533,7 +2552,7 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
 	u32 ocp_data;
 
 	netif_stop_queue(netdev);
-	ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
+	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
 	ocp_data &= ~RCR_ACPT_ALL;
 	ocp_data |= RCR_AB | RCR_APM;
 
@@ -2566,7 +2585,7 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
 	tmp[1] = __cpu_to_le32(swab32(mc_filter[0]));
 
 	pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(tmp), tmp);
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
+	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
 	netif_wake_queue(netdev);
 }
 
@@ -2614,21 +2633,21 @@ static void r8152b_reset_packet_filter(struct r8152 *tp)
 {
 	u32	ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_FMC);
+	ocp_data = pla_ocp_read_word(tp, PLA_FMC);
 	ocp_data &= ~FMC_FCR_MCU_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_FMC, ocp_data);
+	pla_ocp_write_word(tp, PLA_FMC, ocp_data);
 	ocp_data |= FMC_FCR_MCU_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_FMC, ocp_data);
+	pla_ocp_write_word(tp, PLA_FMC, ocp_data);
 }
 
 static void rtl8152_nic_reset(struct r8152 *tp)
 {
 	int	i;
 
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CR, CR_RST);
+	pla_ocp_write_byte(tp, PLA_CR, CR_RST);
 
 	for (i = 0; i < 1000; i++) {
-		if (!(ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CR) & CR_RST))
+		if (!(pla_ocp_read_byte(tp, PLA_CR) & CR_RST))
 			break;
 		usleep_range(100, 400);
 	}
@@ -2644,7 +2663,7 @@ static void set_tx_qlen(struct r8152 *tp)
 
 static inline u8 rtl8152_get_speed(struct r8152 *tp)
 {
-	return ocp_read_byte(tp, MCU_TYPE_PLA, PLA_PHYSTATUS);
+	return pla_ocp_read_byte(tp, PLA_PHYSTATUS);
 }
 
 static void rtl_set_eee_plus(struct r8152 *tp)
@@ -2654,13 +2673,13 @@ static void rtl_set_eee_plus(struct r8152 *tp)
 
 	speed = rtl8152_get_speed(tp);
 	if (speed & _10bps) {
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EEEP_CR);
+		ocp_data = pla_ocp_read_word(tp, PLA_EEEP_CR);
 		ocp_data |= EEEP_CR_EEEP_TX;
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEEP_CR, ocp_data);
+		pla_ocp_write_word(tp, PLA_EEEP_CR, ocp_data);
 	} else {
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EEEP_CR);
+		ocp_data = pla_ocp_read_word(tp, PLA_EEEP_CR);
 		ocp_data &= ~EEEP_CR_EEEP_TX;
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEEP_CR, ocp_data);
+		pla_ocp_write_word(tp, PLA_EEEP_CR, ocp_data);
 	}
 }
 
@@ -2668,12 +2687,12 @@ static void rxdy_gated_en(struct r8152 *tp, bool enable)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MISC_1);
+	ocp_data = pla_ocp_read_word(tp, PLA_MISC_1);
 	if (enable)
 		ocp_data |= RXDY_GATED_EN;
 	else
 		ocp_data &= ~RXDY_GATED_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MISC_1, ocp_data);
+	pla_ocp_write_word(tp, PLA_MISC_1, ocp_data);
 }
 
 static int rtl_start_rx(struct r8152 *tp)
@@ -2761,8 +2780,7 @@ static int rtl_stop_rx(struct r8152 *tp)
 
 static inline void r8153b_rx_agg_chg_indicate(struct r8152 *tp)
 {
-	ocp_write_byte(tp, MCU_TYPE_USB, USB_UPT_RXDMA_OWN,
-		       OWN_UPDATE | OWN_CLEAR);
+	usb_ocp_write_byte(tp, USB_UPT_RXDMA_OWN, OWN_UPDATE | OWN_CLEAR);
 }
 
 static int rtl_enable(struct r8152 *tp)
@@ -2771,9 +2789,9 @@ static int rtl_enable(struct r8152 *tp)
 
 	r8152b_reset_packet_filter(tp);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CR);
+	ocp_data = pla_ocp_read_byte(tp, PLA_CR);
 	ocp_data |= CR_RE | CR_TE;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CR, ocp_data);
+	pla_ocp_write_byte(tp, PLA_CR, ocp_data);
 
 	switch (tp->version) {
 	case RTL_VER_08:
@@ -2809,8 +2827,7 @@ static void r8153_set_rx_early_timeout(struct r8152 *tp)
 	case RTL_VER_04:
 	case RTL_VER_05:
 	case RTL_VER_06:
-		ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_TIMEOUT,
-			       ocp_data);
+		usb_ocp_write_word(tp, USB_RX_EARLY_TIMEOUT, ocp_data);
 		break;
 
 	case RTL_VER_08:
@@ -2818,10 +2835,8 @@ static void r8153_set_rx_early_timeout(struct r8152 *tp)
 		/* The RTL8153B uses USB_RX_EXTRA_AGGR_TMR for rx timeout
 		 * primarily. For USB_RX_EARLY_TIMEOUT, we fix it to 128ns.
 		 */
-		ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_TIMEOUT,
-			       128 / 8);
-		ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EXTRA_AGGR_TMR,
-			       ocp_data);
+		usb_ocp_write_word(tp, USB_RX_EARLY_TIMEOUT, 128 / 8);
+		usb_ocp_write_word(tp, USB_RX_EXTRA_AGGR_TMR, ocp_data);
 		break;
 
 	default:
@@ -2838,13 +2853,11 @@ static void r8153_set_rx_early_size(struct r8152 *tp)
 	case RTL_VER_04:
 	case RTL_VER_05:
 	case RTL_VER_06:
-		ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE,
-			       ocp_data / 4);
+		usb_ocp_write_word(tp, USB_RX_EARLY_SIZE, ocp_data / 4);
 		break;
 	case RTL_VER_08:
 	case RTL_VER_09:
-		ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE,
-			       ocp_data / 8);
+		usb_ocp_write_word(tp, USB_RX_EARLY_SIZE, ocp_data / 8);
 		break;
 	default:
 		WARN_ON_ONCE(1);
@@ -2865,12 +2878,12 @@ static int rtl8153_enable(struct r8152 *tp)
 	if (tp->version == RTL_VER_09) {
 		u32 ocp_data;
 
-		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_TASK);
+		ocp_data = usb_ocp_read_word(tp, USB_FW_TASK);
 		ocp_data &= ~FC_PATCH_TASK;
-		ocp_write_word(tp, MCU_TYPE_USB, USB_FW_TASK, ocp_data);
+		usb_ocp_write_word(tp, USB_FW_TASK, ocp_data);
 		usleep_range(1000, 2000);
 		ocp_data |= FC_PATCH_TASK;
-		ocp_write_word(tp, MCU_TYPE_USB, USB_FW_TASK, ocp_data);
+		usb_ocp_write_word(tp, USB_FW_TASK, ocp_data);
 	}
 
 	return rtl_enable(tp);
@@ -2886,9 +2899,9 @@ static void rtl_disable(struct r8152 *tp)
 		return;
 	}
 
-	ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
+	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
 	ocp_data &= ~RCR_ACPT_ALL;
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
+	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
 
 	rtl_drop_queued_tx(tp);
 
@@ -2898,14 +2911,14 @@ static void rtl_disable(struct r8152 *tp)
 	rxdy_gated_en(tp, true);
 
 	for (i = 0; i < 1000; i++) {
-		ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
+		ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
 		if ((ocp_data & FIFO_EMPTY) == FIFO_EMPTY)
 			break;
 		usleep_range(1000, 2000);
 	}
 
 	for (i = 0; i < 1000; i++) {
-		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_TCR0) & TCR0_TX_EMPTY)
+		if (pla_ocp_read_word(tp, PLA_TCR0) & TCR0_TX_EMPTY)
 			break;
 		usleep_range(1000, 2000);
 	}
@@ -2919,28 +2932,28 @@ static void r8152_power_cut_en(struct r8152 *tp, bool enable)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_UPS_CTRL);
+	ocp_data = usb_ocp_read_word(tp, USB_UPS_CTRL);
 	if (enable)
 		ocp_data |= POWER_CUT;
 	else
 		ocp_data &= ~POWER_CUT;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_UPS_CTRL, ocp_data);
+	usb_ocp_write_word(tp, USB_UPS_CTRL, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_PM_CTRL_STATUS);
+	ocp_data = usb_ocp_read_word(tp, USB_PM_CTRL_STATUS);
 	ocp_data &= ~RESUME_INDICATE;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_PM_CTRL_STATUS, ocp_data);
+	usb_ocp_write_word(tp, USB_PM_CTRL_STATUS, ocp_data);
 }
 
 static void rtl_rx_vlan_en(struct r8152 *tp, bool enable)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CPCR);
+	ocp_data = pla_ocp_read_word(tp, PLA_CPCR);
 	if (enable)
 		ocp_data |= CPCR_RX_VLAN;
 	else
 		ocp_data &= ~CPCR_RX_VLAN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_CPCR, ocp_data);
+	pla_ocp_write_word(tp, PLA_CPCR, ocp_data);
 }
 
 static int rtl8152_set_features(struct net_device *dev,
@@ -2978,11 +2991,11 @@ static u32 __rtl_get_wol(struct r8152 *tp)
 	u32 ocp_data;
 	u32 wolopts = 0;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CONFIG34);
+	ocp_data = pla_ocp_read_word(tp, PLA_CONFIG34);
 	if (ocp_data & LINK_ON_WAKE_EN)
 		wolopts |= WAKE_PHY;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CONFIG5);
+	ocp_data = pla_ocp_read_word(tp, PLA_CONFIG5);
 	if (ocp_data & UWF_EN)
 		wolopts |= WAKE_UCAST;
 	if (ocp_data & BWF_EN)
@@ -2990,7 +3003,7 @@ static u32 __rtl_get_wol(struct r8152 *tp)
 	if (ocp_data & MWF_EN)
 		wolopts |= WAKE_MCAST;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CFG_WOL);
+	ocp_data = pla_ocp_read_word(tp, PLA_CFG_WOL);
 	if (ocp_data & MAGIC_EN)
 		wolopts |= WAKE_MAGIC;
 
@@ -3001,15 +3014,15 @@ static void __rtl_set_wol(struct r8152 *tp, u32 wolopts)
 {
 	u32 ocp_data;
 
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_CONFIG);
+	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_CONFIG);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CONFIG34);
+	ocp_data = pla_ocp_read_word(tp, PLA_CONFIG34);
 	ocp_data &= ~LINK_ON_WAKE_EN;
 	if (wolopts & WAKE_PHY)
 		ocp_data |= LINK_ON_WAKE_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_CONFIG34, ocp_data);
+	pla_ocp_write_word(tp, PLA_CONFIG34, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CONFIG5);
+	ocp_data = pla_ocp_read_word(tp, PLA_CONFIG5);
 	ocp_data &= ~(UWF_EN | BWF_EN | MWF_EN);
 	if (wolopts & WAKE_UCAST)
 		ocp_data |= UWF_EN;
@@ -3017,15 +3030,15 @@ static void __rtl_set_wol(struct r8152 *tp, u32 wolopts)
 		ocp_data |= BWF_EN;
 	if (wolopts & WAKE_MCAST)
 		ocp_data |= MWF_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_CONFIG5, ocp_data);
+	pla_ocp_write_word(tp, PLA_CONFIG5, ocp_data);
 
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_NORAML);
+	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CFG_WOL);
+	ocp_data = pla_ocp_read_word(tp, PLA_CFG_WOL);
 	ocp_data &= ~MAGIC_EN;
 	if (wolopts & WAKE_MAGIC)
 		ocp_data |= MAGIC_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_CFG_WOL, ocp_data);
+	pla_ocp_write_word(tp, PLA_CFG_WOL, ocp_data);
 
 	if (wolopts & WAKE_ANY)
 		device_set_wakeup_enable(&tp->udev->dev, true);
@@ -3037,22 +3050,21 @@ static void r8153_mac_clk_spd(struct r8152 *tp, bool enable)
 {
 	/* MAC clock speed down */
 	if (enable) {
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL,
-			       ALDPS_SPDWN_RATIO);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL2,
-			       EEE_SPDWN_RATIO);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
-			       PKT_AVAIL_SPDWN_EN | SUSPEND_SPDWN_EN |
-			       U1U2_SPDWN_EN | L1_SPDWN_EN);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4,
-			       PWRSAVE_SPDWN_EN | RXDV_SPDWN_EN | TX10MIDLE_EN |
-			       TP100_SPDWN_EN | TP500_SPDWN_EN | EEE_SPDWN_EN |
-			       TP1000_SPDWN_EN);
+		pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL, ALDPS_SPDWN_RATIO);
+		pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL2, EEE_SPDWN_RATIO);
+		pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL3,
+				   PKT_AVAIL_SPDWN_EN | SUSPEND_SPDWN_EN |
+				   U1U2_SPDWN_EN | L1_SPDWN_EN);
+		pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL4,
+				   PWRSAVE_SPDWN_EN | RXDV_SPDWN_EN |
+				   TX10MIDLE_EN | TP100_SPDWN_EN |
+				   TP500_SPDWN_EN | EEE_SPDWN_EN |
+				   TP1000_SPDWN_EN);
 	} else {
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL, 0);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL2, 0);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, 0);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4, 0);
+		pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL, 0);
+		pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL2, 0);
+		pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL3, 0);
+		pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL4, 0);
 	}
 }
 
@@ -3072,25 +3084,25 @@ static void r8153b_u1u2en(struct r8152 *tp, bool enable)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_LPM_CONFIG);
+	ocp_data = usb_ocp_read_word(tp, USB_LPM_CONFIG);
 	if (enable)
 		ocp_data |= LPM_U1U2_EN;
 	else
 		ocp_data &= ~LPM_U1U2_EN;
 
-	ocp_write_word(tp, MCU_TYPE_USB, USB_LPM_CONFIG, ocp_data);
+	usb_ocp_write_word(tp, USB_LPM_CONFIG, ocp_data);
 }
 
 static void r8153_u2p3en(struct r8152 *tp, bool enable)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_U2P3_CTRL);
+	ocp_data = usb_ocp_read_word(tp, USB_U2P3_CTRL);
 	if (enable)
 		ocp_data |= U2P3_ENABLE;
 	else
 		ocp_data &= ~U2P3_ENABLE;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_U2P3_CTRL, ocp_data);
+	usb_ocp_write_word(tp, USB_U2P3_CTRL, ocp_data);
 }
 
 static void r8153b_ups_flags(struct r8152 *tp)
@@ -3162,7 +3174,7 @@ static void r8153b_ups_flags(struct r8152 *tp)
 		break;
 	}
 
-	ocp_write_dword(tp, MCU_TYPE_USB, USB_UPS_FLAGS, ups_flags);
+	usb_ocp_write_dword(tp, USB_UPS_FLAGS, ups_flags);
 }
 
 static void r8153b_green_en(struct r8152 *tp, bool enable)
@@ -3212,30 +3224,30 @@ static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
 
 static void r8153b_ups_en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_POWER_CUT);
+	u32 ocp_data = usb_ocp_read_byte(tp, USB_POWER_CUT);
 
 	if (enable) {
 		r8153b_ups_flags(tp);
 
 		ocp_data |= UPS_EN | USP_PREWAKE | PHASE2_EN;
-		ocp_write_byte(tp, MCU_TYPE_USB, USB_POWER_CUT, ocp_data);
+		usb_ocp_write_byte(tp, USB_POWER_CUT, ocp_data);
 
-		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, 0xcfff);
+		ocp_data = usb_ocp_read_byte(tp, 0xcfff);
 		ocp_data |= BIT(0);
-		ocp_write_byte(tp, MCU_TYPE_USB, 0xcfff, ocp_data);
+		usb_ocp_write_byte(tp, 0xcfff, ocp_data);
 	} else {
 		u16 data;
 
 		ocp_data &= ~(UPS_EN | USP_PREWAKE);
-		ocp_write_byte(tp, MCU_TYPE_USB, USB_POWER_CUT, ocp_data);
+		usb_ocp_write_byte(tp, USB_POWER_CUT, ocp_data);
 
-		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, 0xcfff);
+		ocp_data = usb_ocp_read_byte(tp, 0xcfff);
 		ocp_data &= ~BIT(0);
-		ocp_write_byte(tp, MCU_TYPE_USB, 0xcfff, ocp_data);
+		usb_ocp_write_byte(tp, 0xcfff, ocp_data);
 
-		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+		ocp_data = usb_ocp_read_word(tp, USB_MISC_0);
 		ocp_data &= ~PCUT_STATUS;
-		ocp_write_word(tp, MCU_TYPE_USB, USB_MISC_0, ocp_data);
+		usb_ocp_write_word(tp, USB_MISC_0, ocp_data);
 
 		data = r8153_phy_status(tp, 0);
 
@@ -3266,52 +3278,52 @@ static void r8153_power_cut_en(struct r8152 *tp, bool enable)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_POWER_CUT);
+	ocp_data = usb_ocp_read_word(tp, USB_POWER_CUT);
 	if (enable)
 		ocp_data |= PWR_EN | PHASE2_EN;
 	else
 		ocp_data &= ~(PWR_EN | PHASE2_EN);
-	ocp_write_word(tp, MCU_TYPE_USB, USB_POWER_CUT, ocp_data);
+	usb_ocp_write_word(tp, USB_POWER_CUT, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+	ocp_data = usb_ocp_read_word(tp, USB_MISC_0);
 	ocp_data &= ~PCUT_STATUS;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_MISC_0, ocp_data);
+	usb_ocp_write_word(tp, USB_MISC_0, ocp_data);
 }
 
 static void r8153b_power_cut_en(struct r8152 *tp, bool enable)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_POWER_CUT);
+	ocp_data = usb_ocp_read_word(tp, USB_POWER_CUT);
 	if (enable)
 		ocp_data |= PWR_EN | PHASE2_EN;
 	else
 		ocp_data &= ~PWR_EN;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_POWER_CUT, ocp_data);
+	usb_ocp_write_word(tp, USB_POWER_CUT, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+	ocp_data = usb_ocp_read_word(tp, USB_MISC_0);
 	ocp_data &= ~PCUT_STATUS;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_MISC_0, ocp_data);
+	usb_ocp_write_word(tp, USB_MISC_0, ocp_data);
 }
 
 static void r8153_queue_wake(struct r8152 *tp, bool enable)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_INDICATE_FALG);
+	ocp_data = pla_ocp_read_byte(tp, PLA_INDICATE_FALG);
 	if (enable)
 		ocp_data |= UPCOMING_RUNTIME_D3;
 	else
 		ocp_data &= ~UPCOMING_RUNTIME_D3;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_INDICATE_FALG, ocp_data);
+	pla_ocp_write_byte(tp, PLA_INDICATE_FALG, ocp_data);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_SUSPEND_FLAG);
+	ocp_data = pla_ocp_read_byte(tp, PLA_SUSPEND_FLAG);
 	ocp_data &= ~LINK_CHG_EVENT;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_SUSPEND_FLAG, ocp_data);
+	pla_ocp_write_byte(tp, PLA_SUSPEND_FLAG, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS);
+	ocp_data = pla_ocp_read_word(tp, PLA_EXTRA_STATUS);
 	ocp_data &= ~LINK_CHANGE_FLAG;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS, ocp_data);
+	pla_ocp_write_word(tp, PLA_EXTRA_STATUS, ocp_data);
 }
 
 static bool rtl_can_wakeup(struct r8152 *tp)
@@ -3328,25 +3340,25 @@ static void rtl_runtime_suspend_enable(struct r8152 *tp, bool enable)
 
 		__rtl_set_wol(tp, WAKE_ANY);
 
-		ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_CONFIG);
+		pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_CONFIG);
 
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CONFIG34);
+		ocp_data = pla_ocp_read_word(tp, PLA_CONFIG34);
 		ocp_data |= LINK_OFF_WAKE_EN;
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_CONFIG34, ocp_data);
+		pla_ocp_write_word(tp, PLA_CONFIG34, ocp_data);
 
-		ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_NORAML);
+		pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
 	} else {
 		u32 ocp_data;
 
 		__rtl_set_wol(tp, tp->saved_wolopts);
 
-		ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_CONFIG);
+		pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_CONFIG);
 
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CONFIG34);
+		ocp_data = pla_ocp_read_word(tp, PLA_CONFIG34);
 		ocp_data &= ~LINK_OFF_WAKE_EN;
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_CONFIG34, ocp_data);
+		pla_ocp_write_word(tp, PLA_CONFIG34, ocp_data);
 
-		ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_NORAML);
+		pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
 	}
 }
 
@@ -3405,10 +3417,10 @@ static void r8153_teredo_off(struct r8152 *tp)
 	case RTL_VER_05:
 	case RTL_VER_06:
 	case RTL_VER_07:
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_TEREDO_CFG);
+		ocp_data = pla_ocp_read_word(tp, PLA_TEREDO_CFG);
 		ocp_data &= ~(TEREDO_SEL | TEREDO_RS_EVENT_MASK |
 			      OOB_TEREDO_EN);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_TEREDO_CFG, ocp_data);
+		pla_ocp_write_word(tp, PLA_TEREDO_CFG, ocp_data);
 		break;
 
 	case RTL_VER_08:
@@ -3416,27 +3428,27 @@ static void r8153_teredo_off(struct r8152 *tp)
 		/* The bit 0 ~ 7 are relative with teredo settings. They are
 		 * W1C (write 1 to clear), so set all 1 to disable it.
 		 */
-		ocp_write_byte(tp, MCU_TYPE_PLA, PLA_TEREDO_CFG, 0xff);
+		pla_ocp_write_byte(tp, PLA_TEREDO_CFG, 0xff);
 		break;
 
 	default:
 		break;
 	}
 
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_WDT6_CTRL, WDT6_SET_MODE);
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_REALWOW_TIMER, 0);
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_TEREDO_TIMER, 0);
+	pla_ocp_write_word(tp, PLA_WDT6_CTRL, WDT6_SET_MODE);
+	pla_ocp_write_word(tp, PLA_REALWOW_TIMER, 0);
+	pla_ocp_write_dword(tp, PLA_TEREDO_TIMER, 0);
 }
 
 static void rtl_reset_bmu(struct r8152 *tp)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_BMU_RESET);
+	ocp_data = usb_ocp_read_byte(tp, USB_BMU_RESET);
 	ocp_data &= ~(BMU_RESET_EP_IN | BMU_RESET_EP_OUT);
-	ocp_write_byte(tp, MCU_TYPE_USB, USB_BMU_RESET, ocp_data);
+	usb_ocp_write_byte(tp, USB_BMU_RESET, ocp_data);
 	ocp_data |= BMU_RESET_EP_IN | BMU_RESET_EP_OUT;
-	ocp_write_byte(tp, MCU_TYPE_USB, USB_BMU_RESET, ocp_data);
+	usb_ocp_write_byte(tp, USB_BMU_RESET, ocp_data);
 }
 
 /* Clear the bp to stop the firmware before loading a new one */
@@ -3457,18 +3469,18 @@ static void rtl_clear_bp(struct r8152 *tp, u16 type)
 	case RTL_VER_09:
 	default:
 		if (type == MCU_TYPE_USB) {
-			ocp_write_byte(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);
+			usb_ocp_write_byte(tp, USB_BP2_EN, 0);
+
+			usb_ocp_write_word(tp, USB_BP_8, 0);
+			usb_ocp_write_word(tp, USB_BP_9, 0);
+			usb_ocp_write_word(tp, USB_BP_10, 0);
+			usb_ocp_write_word(tp, USB_BP_11, 0);
+			usb_ocp_write_word(tp, USB_BP_12, 0);
+			usb_ocp_write_word(tp, USB_BP_13, 0);
+			usb_ocp_write_word(tp, USB_BP_14, 0);
+			usb_ocp_write_word(tp, USB_BP_15, 0);
 		} else {
-			ocp_write_byte(tp, MCU_TYPE_PLA, PLA_BP_EN, 0);
+			pla_ocp_write_byte(tp, PLA_BP_EN, 0);
 		}
 		break;
 	}
@@ -3541,7 +3553,7 @@ static int r8153_post_ram_code(struct r8152 *tp, u16 key_addr)
 
 	r8153_patch_request(tp, false);
 
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_OCP_GPHY_BASE, tp->ocp_base);
+	pla_ocp_write_word(tp, PLA_OCP_GPHY_BASE, tp->ocp_base);
 
 	return 0;
 }
@@ -3987,9 +3999,9 @@ static void rtl8152_fw_mac_apply(struct r8152 *tp, struct fw_mac *mac)
 	 * break points and before applying the PLA firmware.
 	 */
 	if (tp->version == RTL_VER_04 && type == MCU_TYPE_PLA &&
-	    !(ocp_read_word(tp, MCU_TYPE_PLA, PLA_MACDBG_POST) & DEBUG_OE)) {
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MACDBG_PRE, DEBUG_LTSSM);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_MACDBG_POST, DEBUG_LTSSM);
+	    !(pla_ocp_read_word(tp, PLA_MACDBG_POST) & DEBUG_OE)) {
+		pla_ocp_write_word(tp, PLA_MACDBG_PRE, DEBUG_LTSSM);
+		pla_ocp_write_word(tp, PLA_MACDBG_POST, DEBUG_LTSSM);
 	}
 
 	length = __le32_to_cpu(mac->blk_hdr.length);
@@ -4018,8 +4030,7 @@ static void rtl8152_fw_mac_apply(struct r8152 *tp, struct fw_mac *mac)
 
 	fw_ver_reg = __le16_to_cpu(mac->fw_ver_reg);
 	if (fw_ver_reg)
-		ocp_write_byte(tp, MCU_TYPE_USB, fw_ver_reg,
-			       mac->fw_ver_data);
+		usb_ocp_write_byte(tp, fw_ver_reg, mac->fw_ver_data);
 
 	dev_dbg(&tp->intf->dev, "successfully applied %s\n", mac->info);
 }
@@ -4163,7 +4174,7 @@ static void r8152_eee_en(struct r8152 *tp, bool enable)
 	u16 config1, config2, config3;
 	u32 ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EEE_CR);
+	ocp_data = pla_ocp_read_word(tp, PLA_EEE_CR);
 	config1 = ocp_reg_read(tp, OCP_EEE_CONFIG1) & ~sd_rise_time_mask;
 	config2 = ocp_reg_read(tp, OCP_EEE_CONFIG2);
 	config3 = ocp_reg_read(tp, OCP_EEE_CONFIG3) & ~fast_snr_mask;
@@ -4183,7 +4194,7 @@ static void r8152_eee_en(struct r8152 *tp, bool enable)
 		config3 |= fast_snr(511);
 	}
 
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_CR, ocp_data);
+	pla_ocp_write_word(tp, PLA_EEE_CR, ocp_data);
 	ocp_reg_write(tp, OCP_EEE_CONFIG1, config1);
 	ocp_reg_write(tp, OCP_EEE_CONFIG2, config2);
 	ocp_reg_write(tp, OCP_EEE_CONFIG3, config3);
@@ -4194,7 +4205,7 @@ static void r8153_eee_en(struct r8152 *tp, bool enable)
 	u32 ocp_data;
 	u16 config;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EEE_CR);
+	ocp_data = pla_ocp_read_word(tp, PLA_EEE_CR);
 	config = ocp_reg_read(tp, OCP_EEE_CFG);
 
 	if (enable) {
@@ -4205,7 +4216,7 @@ static void r8153_eee_en(struct r8152 *tp, bool enable)
 		config &= ~EEE10_EN;
 	}
 
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_CR, ocp_data);
+	pla_ocp_write_word(tp, PLA_EEE_CR, ocp_data);
 	ocp_reg_write(tp, OCP_EEE_CFG, config);
 
 	tp->ups_info.eee = enable;
@@ -4279,7 +4290,7 @@ static void wait_oob_link_list_ready(struct r8152 *tp)
 	int i;
 
 	for (i = 0; i < 1000; i++) {
-		ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
+		ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
 		if (ocp_data & LINK_LIST_READY)
 			break;
 		usleep_range(1000, 2000);
@@ -4290,107 +4301,103 @@ static void r8152b_exit_oob(struct r8152 *tp)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
+	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
 	ocp_data &= ~RCR_ACPT_ALL;
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
+	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
 
 	rxdy_gated_en(tp, true);
 	r8153_teredo_off(tp);
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_NORAML);
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CR, 0x00);
+	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
+	pla_ocp_write_byte(tp, PLA_CR, 0x00);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
+	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
 	ocp_data &= ~NOW_IS_OOB;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7);
+	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
 	ocp_data &= ~MCU_BORW_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7);
+	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
 	ocp_data |= RE_INIT_LL;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
 
 	wait_oob_link_list_ready(tp);
 
 	rtl8152_nic_reset(tp);
 
 	/* rx share fifo credit full threshold */
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL0, RXFIFO_THR1_NORMAL);
+	pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL0, RXFIFO_THR1_NORMAL);
 
 	if (tp->udev->speed == USB_SPEED_FULL ||
 	    tp->udev->speed == USB_SPEED_LOW) {
 		/* rx share fifo credit near full threshold */
-		ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL1,
-				RXFIFO_THR2_FULL);
-		ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL2,
-				RXFIFO_THR3_FULL);
+		pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL1, RXFIFO_THR2_FULL);
+		pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL2, RXFIFO_THR3_FULL);
 	} else {
 		/* rx share fifo credit near full threshold */
-		ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL1,
-				RXFIFO_THR2_HIGH);
-		ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL2,
-				RXFIFO_THR3_HIGH);
+		pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL1, RXFIFO_THR2_HIGH);
+		pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL2, RXFIFO_THR3_HIGH);
 	}
 
 	/* TX share fifo free credit full threshold */
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_TXFIFO_CTRL, TXFIFO_THR_NORMAL);
+	pla_ocp_write_dword(tp, PLA_TXFIFO_CTRL, TXFIFO_THR_NORMAL);
 
-	ocp_write_byte(tp, MCU_TYPE_USB, USB_TX_AGG, TX_AGG_MAX_THRESHOLD);
-	ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, RX_THR_HIGH);
-	ocp_write_dword(tp, MCU_TYPE_USB, USB_TX_DMA,
-			TEST_MODE_DISABLE | TX_SIZE_ADJUST1);
+	usb_ocp_write_byte(tp, USB_TX_AGG, TX_AGG_MAX_THRESHOLD);
+	usb_ocp_write_dword(tp, USB_RX_BUF_TH, RX_THR_HIGH);
+	usb_ocp_write_dword(tp, USB_TX_DMA,
+			    TEST_MODE_DISABLE | TX_SIZE_ADJUST1);
 
 	rtl_rx_vlan_en(tp, tp->netdev->features & NETIF_F_HW_VLAN_CTAG_RX);
 
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, RTL8152_RMS);
+	pla_ocp_write_word(tp, PLA_RMS, RTL8152_RMS);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_TCR0);
+	ocp_data = pla_ocp_read_word(tp, PLA_TCR0);
 	ocp_data |= TCR0_AUTO_FIFO;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TCR0, ocp_data);
+	pla_ocp_write_word(tp, PLA_TCR0, ocp_data);
 }
 
 static void r8152b_enter_oob(struct r8152 *tp)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
+	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
 	ocp_data &= ~NOW_IS_OOB;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
 
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL0, RXFIFO_THR1_OOB);
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL1, RXFIFO_THR2_OOB);
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL2, RXFIFO_THR3_OOB);
+	pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL0, RXFIFO_THR1_OOB);
+	pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL1, RXFIFO_THR2_OOB);
+	pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL2, RXFIFO_THR3_OOB);
 
 	rtl_disable(tp);
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7);
+	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
 	ocp_data |= RE_INIT_LL;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, RTL8152_RMS);
+	pla_ocp_write_word(tp, PLA_RMS, RTL8152_RMS);
 
 	rtl_rx_vlan_en(tp, true);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_BDC_CR);
+	ocp_data = pla_ocp_read_word(tp, PLA_BDC_CR);
 	ocp_data |= ALDPS_PROXY_MODE;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_BDC_CR, ocp_data);
+	pla_ocp_write_word(tp, PLA_BDC_CR, ocp_data);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
+	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
 	ocp_data |= NOW_IS_OOB | DIS_MCU_CLROOB;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
 
 	rxdy_gated_en(tp, false);
 
-	ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
+	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
 	ocp_data |= RCR_APM | RCR_AM | RCR_AB;
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
+	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
 }
 
 static int r8153_pre_firmware_1(struct r8152 *tp)
@@ -4399,7 +4406,7 @@ static int r8153_pre_firmware_1(struct r8152 *tp)
 
 	/* Wait till the WTD timer is ready. It would take at most 104 ms. */
 	for (i = 0; i < 104; i++) {
-		u32 ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_WDT1_CTRL);
+		u32 ocp_data = usb_ocp_read_byte(tp, USB_WDT1_CTRL);
 
 		if (!(ocp_data & WTD1_EN))
 			break;
@@ -4412,11 +4419,11 @@ static int r8153_pre_firmware_1(struct r8152 *tp)
 static int r8153_post_firmware_1(struct r8152 *tp)
 {
 	/* set USB_BP_4 to support USB_SPEED_SUPER only */
-	if (ocp_read_byte(tp, MCU_TYPE_USB, USB_CSTMR) & FORCE_SUPER)
-		ocp_write_word(tp, MCU_TYPE_USB, USB_BP_4, BP4_SUPER_ONLY);
+	if (usb_ocp_read_byte(tp, USB_CSTMR) & FORCE_SUPER)
+		usb_ocp_write_word(tp, USB_BP_4, BP4_SUPER_ONLY);
 
 	/* reset UPHY timer to 36 ms */
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_UPHY_TIMER, 36000 / 16);
+	pla_ocp_write_word(tp, PLA_UPHY_TIMER, 36000 / 16);
 
 	return 0;
 }
@@ -4427,9 +4434,9 @@ static int r8153_pre_firmware_2(struct r8152 *tp)
 
 	r8153_pre_firmware_1(tp);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_FIX_EN0);
+	ocp_data = usb_ocp_read_word(tp, USB_FW_FIX_EN0);
 	ocp_data &= ~FW_FIX_SUSPEND;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_FW_FIX_EN0, ocp_data);
+	usb_ocp_write_word(tp, USB_FW_FIX_EN0, ocp_data);
 
 	return 0;
 }
@@ -4439,25 +4446,25 @@ static int r8153_post_firmware_2(struct r8152 *tp)
 	u32 ocp_data;
 
 	/* enable bp0 if support USB_SPEED_SUPER only */
-	if (ocp_read_byte(tp, MCU_TYPE_USB, USB_CSTMR) & FORCE_SUPER) {
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_BP_EN);
+	if (usb_ocp_read_byte(tp, USB_CSTMR) & FORCE_SUPER) {
+		ocp_data = pla_ocp_read_word(tp, PLA_BP_EN);
 		ocp_data |= BIT(0);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_BP_EN, ocp_data);
+		pla_ocp_write_word(tp, PLA_BP_EN, ocp_data);
 	}
 
 	/* reset UPHY timer to 36 ms */
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_UPHY_TIMER, 36000 / 16);
+	pla_ocp_write_word(tp, PLA_UPHY_TIMER, 36000 / 16);
 
 	/* enable U3P3 check, set the counter to 4 */
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS, U3P3_CHECK_EN | 4);
+	pla_ocp_write_word(tp, PLA_EXTRA_STATUS, U3P3_CHECK_EN | 4);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_FIX_EN0);
+	ocp_data = usb_ocp_read_word(tp, USB_FW_FIX_EN0);
 	ocp_data |= FW_FIX_SUSPEND;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_FW_FIX_EN0, ocp_data);
+	usb_ocp_write_word(tp, USB_FW_FIX_EN0, ocp_data);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_USB2PHY);
+	ocp_data = usb_ocp_read_byte(tp, USB_USB2PHY);
 	ocp_data |= USB2PHY_L1 | USB2PHY_SUSPEND;
-	ocp_write_byte(tp, MCU_TYPE_USB, USB_USB2PHY, ocp_data);
+	usb_ocp_write_byte(tp, USB_USB2PHY, ocp_data);
 
 	return 0;
 }
@@ -4466,13 +4473,13 @@ static int r8153_post_firmware_3(struct r8152 *tp)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_USB2PHY);
+	ocp_data = usb_ocp_read_byte(tp, USB_USB2PHY);
 	ocp_data |= USB2PHY_L1 | USB2PHY_SUSPEND;
-	ocp_write_byte(tp, MCU_TYPE_USB, USB_USB2PHY, ocp_data);
+	usb_ocp_write_byte(tp, USB_USB2PHY, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_FIX_EN1);
+	ocp_data = usb_ocp_read_word(tp, USB_FW_FIX_EN1);
 	ocp_data |= FW_IP_RESET_EN;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_FW_FIX_EN1, ocp_data);
+	usb_ocp_write_word(tp, USB_FW_FIX_EN1, ocp_data);
 
 	return 0;
 }
@@ -4480,8 +4487,7 @@ static int r8153_post_firmware_3(struct r8152 *tp)
 static int r8153b_pre_firmware_1(struct r8152 *tp)
 {
 	/* enable fc timer and set timer to 1 second. */
-	ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
-		       CTRL_TIMER_EN | (1000 / 8));
+	usb_ocp_write_word(tp, USB_FC_TIMER, CTRL_TIMER_EN | (1000 / 8));
 
 	return 0;
 }
@@ -4491,24 +4497,24 @@ static int r8153b_post_firmware_1(struct r8152 *tp)
 	u32 ocp_data;
 
 	/* enable bp0 for RTL8153-BND */
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
+	ocp_data = usb_ocp_read_byte(tp, USB_MISC_1);
 	if (ocp_data & BND_MASK) {
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_BP_EN);
+		ocp_data = pla_ocp_read_word(tp, PLA_BP_EN);
 		ocp_data |= BIT(0);
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_BP_EN, ocp_data);
+		pla_ocp_write_word(tp, PLA_BP_EN, ocp_data);
 	}
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_CTRL);
+	ocp_data = usb_ocp_read_word(tp, USB_FW_CTRL);
 	ocp_data |= FLOW_CTRL_PATCH_OPT;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_FW_CTRL, ocp_data);
+	usb_ocp_write_word(tp, USB_FW_CTRL, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_TASK);
+	ocp_data = usb_ocp_read_word(tp, USB_FW_TASK);
 	ocp_data |= FC_PATCH_TASK;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_FW_TASK, ocp_data);
+	usb_ocp_write_word(tp, USB_FW_TASK, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_FIX_EN1);
+	ocp_data = usb_ocp_read_word(tp, USB_FW_FIX_EN1);
 	ocp_data |= FW_IP_RESET_EN;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_FW_FIX_EN1, ocp_data);
+	usb_ocp_write_word(tp, USB_FW_FIX_EN1, ocp_data);
 
 	return 0;
 }
@@ -4528,7 +4534,7 @@ static void r8153_aldps_en(struct r8152 *tp, bool enable)
 		ocp_reg_write(tp, OCP_POWER_CFG, data);
 		for (i = 0; i < 20; i++) {
 			usleep_range(1000, 2000);
-			if (ocp_read_word(tp, MCU_TYPE_PLA, 0xe000) & 0x0100)
+			if (pla_ocp_read_word(tp, 0xe000) & 0x0100)
 				break;
 		}
 	}
@@ -4567,9 +4573,9 @@ static void r8153_hw_phy_cfg(struct r8152 *tp)
 	ocp_reg_write(tp, OCP_POWER_CFG, data);
 	sram_write(tp, SRAM_IMPEDANCE, 0x0b13);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR);
+	ocp_data = pla_ocp_read_word(tp, PLA_PHY_PWR);
 	ocp_data |= PFM_PWM_SWITCH;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR, ocp_data);
+	pla_ocp_write_word(tp, PLA_PHY_PWR, ocp_data);
 
 	/* Enable LPF corner auto tune */
 	sram_write(tp, SRAM_LPF_CFG, 0xf70f);
@@ -4602,10 +4608,10 @@ static u32 r8152_efuse_read(struct r8152 *tp, u8 addr)
 {
 	u32 ocp_data;
 
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_EFUSE_CMD, EFUSE_READ_CMD | addr);
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EFUSE_CMD);
+	pla_ocp_write_word(tp, PLA_EFUSE_CMD, EFUSE_READ_CMD | addr);
+	ocp_data = pla_ocp_read_word(tp, PLA_EFUSE_CMD);
 	ocp_data = (ocp_data & EFUSE_DATA_BIT16) << 9;	/* data of bit16 */
-	ocp_data |= ocp_read_word(tp, MCU_TYPE_PLA, PLA_EFUSE_DATA);
+	ocp_data |= pla_ocp_read_word(tp, PLA_EFUSE_DATA);
 
 	return ocp_data;
 }
@@ -4652,14 +4658,14 @@ static void r8153b_hw_phy_cfg(struct r8152 *tp)
 		u32 swr_cnt_1ms_ini;
 
 		swr_cnt_1ms_ini = (16000000 / ocp_data) & SAW_CNT_1MS_MASK;
-		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_UPS_CFG);
+		ocp_data = usb_ocp_read_word(tp, USB_UPS_CFG);
 		ocp_data = (ocp_data & ~SAW_CNT_1MS_MASK) | swr_cnt_1ms_ini;
-		ocp_write_word(tp, MCU_TYPE_USB, USB_UPS_CFG, ocp_data);
+		usb_ocp_write_word(tp, USB_UPS_CFG, ocp_data);
 	}
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR);
+	ocp_data = pla_ocp_read_word(tp, PLA_PHY_PWR);
 	ocp_data |= PFM_PWM_SWITCH;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR, ocp_data);
+	pla_ocp_write_word(tp, PLA_PHY_PWR, ocp_data);
 
 	/* Advnace EEE */
 	if (!r8153_patch_request(tp, true)) {
@@ -4699,47 +4705,47 @@ static void r8153_first_init(struct r8152 *tp)
 	rxdy_gated_en(tp, true);
 	r8153_teredo_off(tp);
 
-	ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
+	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
 	ocp_data &= ~RCR_ACPT_ALL;
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
+	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
 
 	rtl8152_nic_reset(tp);
 	rtl_reset_bmu(tp);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
+	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
 	ocp_data &= ~NOW_IS_OOB;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7);
+	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
 	ocp_data &= ~MCU_BORW_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7);
+	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
 	ocp_data |= RE_INIT_LL;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
 
 	wait_oob_link_list_ready(tp);
 
 	rtl_rx_vlan_en(tp, tp->netdev->features & NETIF_F_HW_VLAN_CTAG_RX);
 
 	ocp_data = tp->netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, ocp_data);
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, MTPS_JUMBO);
+	pla_ocp_write_word(tp, PLA_RMS, ocp_data);
+	pla_ocp_write_byte(tp, PLA_MTPS, MTPS_JUMBO);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_TCR0);
+	ocp_data = pla_ocp_read_word(tp, PLA_TCR0);
 	ocp_data |= TCR0_AUTO_FIFO;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TCR0, ocp_data);
+	pla_ocp_write_word(tp, PLA_TCR0, ocp_data);
 
 	rtl8152_nic_reset(tp);
 
 	/* rx share fifo credit full threshold */
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL0, RXFIFO_THR1_NORMAL);
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL1, RXFIFO_THR2_NORMAL);
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RXFIFO_CTRL2, RXFIFO_THR3_NORMAL);
+	pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL0, RXFIFO_THR1_NORMAL);
+	pla_ocp_write_word(tp, PLA_RXFIFO_CTRL1, RXFIFO_THR2_NORMAL);
+	pla_ocp_write_word(tp, PLA_RXFIFO_CTRL2, RXFIFO_THR3_NORMAL);
 	/* TX share fifo free credit full threshold */
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_TXFIFO_CTRL, TXFIFO_THR_NORMAL2);
+	pla_ocp_write_dword(tp, PLA_TXFIFO_CTRL, TXFIFO_THR_NORMAL2);
 }
 
 static void r8153_enter_oob(struct r8152 *tp)
@@ -4748,32 +4754,32 @@ static void r8153_enter_oob(struct r8152 *tp)
 
 	r8153_mac_clk_spd(tp, true);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
+	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
 	ocp_data &= ~NOW_IS_OOB;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
 
 	rtl_disable(tp);
 	rtl_reset_bmu(tp);
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7);
+	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
 	ocp_data |= RE_INIT_LL;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
 
 	wait_oob_link_list_ready(tp);
 
 	ocp_data = tp->netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, ocp_data);
+	pla_ocp_write_word(tp, PLA_RMS, ocp_data);
 
 	switch (tp->version) {
 	case RTL_VER_03:
 	case RTL_VER_04:
 	case RTL_VER_05:
 	case RTL_VER_06:
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_TEREDO_CFG);
+		ocp_data = pla_ocp_read_word(tp, PLA_TEREDO_CFG);
 		ocp_data &= ~TEREDO_WAKE_MASK;
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_TEREDO_CFG, ocp_data);
+		pla_ocp_write_word(tp, PLA_TEREDO_CFG, ocp_data);
 		break;
 
 	case RTL_VER_08:
@@ -4782,7 +4788,7 @@ static void r8153_enter_oob(struct r8152 *tp)
 		 * type. Set it to zero. bits[7:0] are the W1C bits about
 		 * the events. Set them to all 1 to clear them.
 		 */
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_TEREDO_WAKE_BASE, 0x00ff);
+		pla_ocp_write_word(tp, PLA_TEREDO_WAKE_BASE, 0x00ff);
 		break;
 
 	default:
@@ -4791,19 +4797,19 @@ static void r8153_enter_oob(struct r8152 *tp)
 
 	rtl_rx_vlan_en(tp, true);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_BDC_CR);
+	ocp_data = pla_ocp_read_word(tp, PLA_BDC_CR);
 	ocp_data |= ALDPS_PROXY_MODE;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_BDC_CR, ocp_data);
+	pla_ocp_write_word(tp, PLA_BDC_CR, ocp_data);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
+	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
 	ocp_data |= NOW_IS_OOB | DIS_MCU_CLROOB;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
 
 	rxdy_gated_en(tp, false);
 
-	ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
+	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
 	ocp_data |= RCR_APM | RCR_AM | RCR_AB;
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
+	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
 }
 
 static void rtl8153_disable(struct r8152 *tp)
@@ -4975,17 +4981,17 @@ static void rtl8153_up(struct r8152 *tp)
 	r8153_aldps_en(tp, false);
 	r8153_first_init(tp);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
+	ocp_data = pla_ocp_read_byte(tp, PLA_CONFIG6);
 	ocp_data |= LANWAKE_CLR_EN;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
+	pla_ocp_write_byte(tp, PLA_CONFIG6, ocp_data);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG);
+	ocp_data = pla_ocp_read_byte(tp, PLA_LWAKE_CTRL_REG);
 	ocp_data &= ~LANWAKE_PIN;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
+	pla_ocp_write_byte(tp, PLA_LWAKE_CTRL_REG, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1);
+	ocp_data = usb_ocp_read_word(tp, USB_SSPHYLINK1);
 	ocp_data &= ~DELAY_PHY_PWR_CHG;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1, ocp_data);
+	usb_ocp_write_word(tp, USB_SSPHYLINK1, ocp_data);
 
 	r8153_aldps_en(tp, true);
 
@@ -5012,9 +5018,9 @@ static void rtl8153_down(struct r8152 *tp)
 		return;
 	}
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
+	ocp_data = pla_ocp_read_byte(tp, PLA_CONFIG6);
 	ocp_data &= ~LANWAKE_CLR_EN;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
+	pla_ocp_write_byte(tp, PLA_CONFIG6, ocp_data);
 
 	r8153_u1u2en(tp, false);
 	r8153_u2p3en(tp, false);
@@ -5036,11 +5042,11 @@ static void rtl8153b_up(struct r8152 *tp)
 	r8153_aldps_en(tp, false);
 
 	r8153_first_init(tp);
-	ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, RX_THR_B);
+	usb_ocp_write_dword(tp, USB_RX_BUF_TH, RX_THR_B);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+	ocp_data = pla_ocp_read_word(tp, PLA_MAC_PWR_CTRL3);
 	ocp_data &= ~PLA_MCU_SPDWN_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+	pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL3, ocp_data);
 
 	r8153_aldps_en(tp, true);
 
@@ -5057,9 +5063,9 @@ static void rtl8153b_down(struct r8152 *tp)
 		return;
 	}
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+	ocp_data = pla_ocp_read_word(tp, PLA_MAC_PWR_CTRL3);
 	ocp_data |= PLA_MCU_SPDWN_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+	pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL3, ocp_data);
 
 	r8153b_u1u2en(tp, false);
 	r8153_u2p3en(tp, false);
@@ -5073,10 +5079,10 @@ static bool rtl8152_in_nway(struct r8152 *tp)
 {
 	u16 nway_state;
 
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_OCP_GPHY_BASE, 0x2000);
+	pla_ocp_write_word(tp, PLA_OCP_GPHY_BASE, 0x2000);
 	tp->ocp_base = 0x2000;
-	ocp_write_byte(tp, MCU_TYPE_PLA, 0xb014, 0x4c);		/* phy state */
-	nway_state = ocp_read_word(tp, MCU_TYPE_PLA, 0xb01a);
+	pla_ocp_write_byte(tp, 0xb014, 0x4c);		/* phy state */
+	nway_state = pla_ocp_read_word(tp, 0xb01a);
 
 	/* bit 15: TXDIS_STATE, bit 14: ABD_STATE */
 	if (nway_state & 0xc000)
@@ -5323,9 +5329,9 @@ static void rtl_tally_reset(struct r8152 *tp)
 {
 	u32 ocp_data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_RSTTALLY);
+	ocp_data = pla_ocp_read_word(tp, PLA_RSTTALLY);
 	ocp_data |= TALLY_RESET;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RSTTALLY, ocp_data);
+	pla_ocp_write_word(tp, PLA_RSTTALLY, ocp_data);
 }
 
 static void r8152b_init(struct r8152 *tp)
@@ -5345,30 +5351,30 @@ static void r8152b_init(struct r8152 *tp)
 	r8152_aldps_en(tp, false);
 
 	if (tp->version == RTL_VER_01) {
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_LED_FEATURE);
+		ocp_data = pla_ocp_read_word(tp, PLA_LED_FEATURE);
 		ocp_data &= ~LED_MODE_MASK;
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_LED_FEATURE, ocp_data);
+		pla_ocp_write_word(tp, PLA_LED_FEATURE, ocp_data);
 	}
 
 	r8152_power_cut_en(tp, false);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR);
+	ocp_data = pla_ocp_read_word(tp, PLA_PHY_PWR);
 	ocp_data |= TX_10M_IDLE_EN | PFM_PWM_SWITCH;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR, ocp_data);
-	ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL);
+	pla_ocp_write_word(tp, PLA_PHY_PWR, ocp_data);
+	ocp_data = pla_ocp_read_dword(tp, PLA_MAC_PWR_CTRL);
 	ocp_data &= ~MCU_CLK_RATIO_MASK;
 	ocp_data |= MCU_CLK_RATIO | D3_CLK_GATED_EN;
-	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL, ocp_data);
+	pla_ocp_write_dword(tp, PLA_MAC_PWR_CTRL, ocp_data);
 	ocp_data = GPHY_STS_MSK | SPEED_DOWN_MSK |
 		   SPDWN_RXDV_MSK | SPDWN_LINKCHG_MSK;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_GPHY_INTR_IMR, ocp_data);
+	pla_ocp_write_word(tp, PLA_GPHY_INTR_IMR, ocp_data);
 
 	rtl_tally_reset(tp);
 
 	/* enable rx aggregation */
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_USB_CTRL);
+	ocp_data = usb_ocp_read_word(tp, USB_USB_CTRL);
 	ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
-	ocp_write_word(tp, MCU_TYPE_USB, USB_USB_CTRL, ocp_data);
+	usb_ocp_write_word(tp, USB_USB_CTRL, ocp_data);
 }
 
 static void r8153_init(struct r8152 *tp)
@@ -5383,8 +5389,7 @@ static void r8153_init(struct r8152 *tp)
 	r8153_u1u2en(tp, false);
 
 	for (i = 0; i < 500; i++) {
-		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
-		    AUTOLOAD_DONE)
+		if (pla_ocp_read_word(tp, PLA_BOOT_CTRL) & AUTOLOAD_DONE)
 			break;
 
 		msleep(20);
@@ -5409,69 +5414,69 @@ static void r8153_init(struct r8152 *tp)
 	r8153_u2p3en(tp, false);
 
 	if (tp->version == RTL_VER_04) {
-		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_SSPHYLINK2);
+		ocp_data = usb_ocp_read_word(tp, USB_SSPHYLINK2);
 		ocp_data &= ~pwd_dn_scale_mask;
 		ocp_data |= pwd_dn_scale(96);
-		ocp_write_word(tp, MCU_TYPE_USB, USB_SSPHYLINK2, ocp_data);
+		usb_ocp_write_word(tp, USB_SSPHYLINK2, ocp_data);
 
-		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_USB2PHY);
+		ocp_data = usb_ocp_read_byte(tp, USB_USB2PHY);
 		ocp_data |= USB2PHY_L1 | USB2PHY_SUSPEND;
-		ocp_write_byte(tp, MCU_TYPE_USB, USB_USB2PHY, ocp_data);
+		usb_ocp_write_byte(tp, USB_USB2PHY, ocp_data);
 	} else if (tp->version == RTL_VER_05) {
-		ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_DMY_REG0);
+		ocp_data = pla_ocp_read_byte(tp, PLA_DMY_REG0);
 		ocp_data &= ~ECM_ALDPS;
-		ocp_write_byte(tp, MCU_TYPE_PLA, PLA_DMY_REG0, ocp_data);
+		pla_ocp_write_byte(tp, PLA_DMY_REG0, ocp_data);
 
-		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_CSR_DUMMY1);
-		if (ocp_read_word(tp, MCU_TYPE_USB, USB_BURST_SIZE) == 0)
+		ocp_data = usb_ocp_read_byte(tp, USB_CSR_DUMMY1);
+		if (usb_ocp_read_word(tp, USB_BURST_SIZE) == 0)
 			ocp_data &= ~DYNAMIC_BURST;
 		else
 			ocp_data |= DYNAMIC_BURST;
-		ocp_write_byte(tp, MCU_TYPE_USB, USB_CSR_DUMMY1, ocp_data);
+		usb_ocp_write_byte(tp, USB_CSR_DUMMY1, ocp_data);
 	} else if (tp->version == RTL_VER_06) {
-		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_CSR_DUMMY1);
-		if (ocp_read_word(tp, MCU_TYPE_USB, USB_BURST_SIZE) == 0)
+		ocp_data = usb_ocp_read_byte(tp, USB_CSR_DUMMY1);
+		if (usb_ocp_read_word(tp, USB_BURST_SIZE) == 0)
 			ocp_data &= ~DYNAMIC_BURST;
 		else
 			ocp_data |= DYNAMIC_BURST;
-		ocp_write_byte(tp, MCU_TYPE_USB, USB_CSR_DUMMY1, ocp_data);
+		usb_ocp_write_byte(tp, USB_CSR_DUMMY1, ocp_data);
 
 		r8153_queue_wake(tp, false);
 
-		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS);
+		ocp_data = pla_ocp_read_word(tp, PLA_EXTRA_STATUS);
 		if (rtl8152_get_speed(tp) & LINK_STATUS)
 			ocp_data |= CUR_LINK_OK;
 		else
 			ocp_data &= ~CUR_LINK_OK;
 		ocp_data |= POLL_LINK_CHG;
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS, ocp_data);
+		pla_ocp_write_word(tp, PLA_EXTRA_STATUS, ocp_data);
 	}
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_CSR_DUMMY2);
+	ocp_data = usb_ocp_read_byte(tp, USB_CSR_DUMMY2);
 	ocp_data |= EP4_FULL_FC;
-	ocp_write_byte(tp, MCU_TYPE_USB, USB_CSR_DUMMY2, ocp_data);
+	usb_ocp_write_byte(tp, USB_CSR_DUMMY2, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_WDT11_CTRL);
+	ocp_data = usb_ocp_read_word(tp, USB_WDT11_CTRL);
 	ocp_data &= ~TIMER11_EN;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_WDT11_CTRL, ocp_data);
+	usb_ocp_write_word(tp, USB_WDT11_CTRL, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_LED_FEATURE);
+	ocp_data = pla_ocp_read_word(tp, PLA_LED_FEATURE);
 	ocp_data &= ~LED_MODE_MASK;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_LED_FEATURE, ocp_data);
+	pla_ocp_write_word(tp, PLA_LED_FEATURE, ocp_data);
 
 	ocp_data = FIFO_EMPTY_1FB | ROK_EXIT_LPM;
 	if (tp->version == RTL_VER_04 && tp->udev->speed < USB_SPEED_SUPER)
 		ocp_data |= LPM_TIMER_500MS;
 	else
 		ocp_data |= LPM_TIMER_500US;
-	ocp_write_byte(tp, MCU_TYPE_USB, USB_LPM_CTRL, ocp_data);
+	usb_ocp_write_byte(tp, USB_LPM_CTRL, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_AFE_CTRL2);
+	ocp_data = usb_ocp_read_word(tp, USB_AFE_CTRL2);
 	ocp_data &= ~SEN_VAL_MASK;
 	ocp_data |= SEN_VAL_NORMAL | SEL_RXIDLE;
-	ocp_write_word(tp, MCU_TYPE_USB, USB_AFE_CTRL2, ocp_data);
+	usb_ocp_write_word(tp, USB_AFE_CTRL2, ocp_data);
 
-	ocp_write_word(tp, MCU_TYPE_USB, USB_CONNECT_TIMER, 0x0001);
+	usb_ocp_write_word(tp, USB_CONNECT_TIMER, 0x0001);
 
 	r8153_power_cut_en(tp, false);
 	rtl_runtime_suspend_enable(tp, false);
@@ -5479,21 +5484,21 @@ static void r8153_init(struct r8152 *tp)
 	r8153_mac_clk_spd(tp, false);
 	usb_enable_lpm(tp->udev);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
+	ocp_data = pla_ocp_read_byte(tp, PLA_CONFIG6);
 	ocp_data |= LANWAKE_CLR_EN;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
+	pla_ocp_write_byte(tp, PLA_CONFIG6, ocp_data);
 
-	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG);
+	ocp_data = pla_ocp_read_byte(tp, PLA_LWAKE_CTRL_REG);
 	ocp_data &= ~LANWAKE_PIN;
-	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
+	pla_ocp_write_byte(tp, PLA_LWAKE_CTRL_REG, ocp_data);
 
 	/* rx aggregation */
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_USB_CTRL);
+	ocp_data = usb_ocp_read_word(tp, USB_USB_CTRL);
 	ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
 	if (test_bit(DELL_TB_RX_AGG_BUG, &tp->flags))
 		ocp_data |= RX_AGG_DISABLE;
 
-	ocp_write_word(tp, MCU_TYPE_USB, USB_USB_CTRL, ocp_data);
+	usb_ocp_write_word(tp, USB_USB_CTRL, ocp_data);
 
 	rtl_tally_reset(tp);
 
@@ -5523,8 +5528,7 @@ static void r8153b_init(struct r8152 *tp)
 	r8153b_u1u2en(tp, false);
 
 	for (i = 0; i < 500; i++) {
-		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
-		    AUTOLOAD_DONE)
+		if (pla_ocp_read_word(tp, PLA_BOOT_CTRL) & AUTOLOAD_DONE)
 			break;
 
 		msleep(20);
@@ -5545,52 +5549,52 @@ static void r8153b_init(struct r8152 *tp)
 	r8153_u2p3en(tp, false);
 
 	/* MSC timer = 0xfff * 8ms = 32760 ms */
-	ocp_write_word(tp, MCU_TYPE_USB, USB_MSC_TIMER, 0x0fff);
+	usb_ocp_write_word(tp, USB_MSC_TIMER, 0x0fff);
 
 	/* U1/U2/L1 idle timer. 500 us */
-	ocp_write_word(tp, MCU_TYPE_USB, USB_U1U2_TIMER, 500);
+	usb_ocp_write_word(tp, USB_U1U2_TIMER, 500);
 
 	r8153b_power_cut_en(tp, false);
 	r8153b_ups_en(tp, false);
 	r8153_queue_wake(tp, false);
 	rtl_runtime_suspend_enable(tp, false);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS);
+	ocp_data = pla_ocp_read_word(tp, PLA_EXTRA_STATUS);
 	if (rtl8152_get_speed(tp) & LINK_STATUS)
 		ocp_data |= CUR_LINK_OK;
 	else
 		ocp_data &= ~CUR_LINK_OK;
 	ocp_data |= POLL_LINK_CHG;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS, ocp_data);
+	pla_ocp_write_word(tp, PLA_EXTRA_STATUS, ocp_data);
 
 	if (tp->udev->speed != USB_SPEED_HIGH)
 		r8153b_u1u2en(tp, true);
 	usb_enable_lpm(tp->udev);
 
 	/* MAC clock speed down */
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL2);
+	ocp_data = pla_ocp_read_word(tp, PLA_MAC_PWR_CTRL2);
 	ocp_data |= MAC_CLK_SPDWN_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL2, ocp_data);
+	pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL2, ocp_data);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+	ocp_data = pla_ocp_read_word(tp, PLA_MAC_PWR_CTRL3);
 	ocp_data &= ~PLA_MCU_SPDWN_EN;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+	pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL3, ocp_data);
 
 	if (tp->version == RTL_VER_09) {
 		/* Disable Test IO for 32QFN */
-		if (ocp_read_byte(tp, MCU_TYPE_PLA, 0xdc00) & BIT(5)) {
-			ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR);
+		if (pla_ocp_read_byte(tp, 0xdc00) & BIT(5)) {
+			ocp_data = pla_ocp_read_word(tp, PLA_PHY_PWR);
 			ocp_data |= TEST_IO_OFF;
-			ocp_write_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR, ocp_data);
+			pla_ocp_write_word(tp, PLA_PHY_PWR, ocp_data);
 		}
 	}
 
 	set_bit(GREEN_ETHERNET, &tp->flags);
 
 	/* rx aggregation */
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_USB_CTRL);
+	ocp_data = usb_ocp_read_word(tp, USB_USB_CTRL);
 	ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
-	ocp_write_word(tp, MCU_TYPE_USB, USB_USB_CTRL, ocp_data);
+	usb_ocp_write_word(tp, USB_USB_CTRL, ocp_data);
 
 	rtl_tally_reset(tp);
 
@@ -5756,15 +5760,14 @@ static int rtl8152_runtime_suspend(struct r8152 *tp)
 		if (netif_carrier_ok(netdev)) {
 			u32 ocp_data;
 
-			rcr = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
+			rcr = pla_ocp_read_dword(tp, PLA_RCR);
 			ocp_data = rcr & ~RCR_ACPT_ALL;
-			ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
+			pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
 			rxdy_gated_en(tp, true);
-			ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA,
-						 PLA_OOB_CTRL);
+			ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
 			if (!(ocp_data & RXFIFO_EMPTY)) {
 				rxdy_gated_en(tp, false);
-				ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, rcr);
+				pla_ocp_write_dword(tp, PLA_RCR, rcr);
 				clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 				smp_mb__after_atomic();
 				ret = -EBUSY;
@@ -5783,7 +5786,7 @@ static int rtl8152_runtime_suspend(struct r8152 *tp)
 			napi_disable(napi);
 			rtl_stop_rx(tp);
 			rxdy_gated_en(tp, false);
-			ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, rcr);
+			pla_ocp_write_dword(tp, PLA_RCR, rcr);
 			napi_enable(napi);
 		}
 
@@ -6450,7 +6453,7 @@ static int rtl8152_change_mtu(struct net_device *dev, int new_mtu)
 	if (netif_running(dev)) {
 		u32 rms = new_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
 
-		ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, rms);
+		pla_ocp_write_word(tp, PLA_RMS, rms);
 
 		if (netif_carrier_ok(dev))
 			r8153_set_rx_early_size(tp);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH net-next 4/5] r8152: rename r8153_phy_status to r8153_phy_status_wait
  2020-11-03 19:22 [PATCH net-next 0/5] r8152 changes Marek Behún
                   ` (2 preceding siblings ...)
  2020-11-03 19:22 ` [PATCH net-next 3/5] r8152: add MCU typed read/write functions Marek Behún
@ 2020-11-03 19:22 ` Marek Behún
  2020-11-03 19:22 ` [PATCH net-next 5/5] r8152: use *_modify helpers instead of read/write combos Marek Behún
  4 siblings, 0 replies; 26+ messages in thread
From: Marek Behún @ 2020-11-03 19:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-usb, Hayes Wang, Marek Behún

The idea behind r8153_phy_status function is to wait till status is of
desired value or (if desired value is zero) to wait till status if of
non-zero value. Rename this function to r8153_phy_status_wait.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/usb/r8152.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 905859309db4..1a427061da8e 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3198,7 +3198,7 @@ static void r8153b_green_en(struct r8152 *tp, bool enable)
 	tp->ups_info.green = enable;
 }
 
-static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
+static u16 r8153_phy_status_wait(struct r8152 *tp, u16 desired)
 {
 	u16 data;
 	int i;
@@ -3249,7 +3249,7 @@ static void r8153b_ups_en(struct r8152 *tp, bool enable)
 		ocp_data &= ~PCUT_STATUS;
 		usb_ocp_write_word(tp, USB_MISC_0, ocp_data);
 
-		data = r8153_phy_status(tp, 0);
+		data = r8153_phy_status_wait(tp, 0);
 
 		switch (data) {
 		case PHY_STAT_PWRDN:
@@ -3262,7 +3262,7 @@ static void r8153b_ups_en(struct r8152 *tp, bool enable)
 			data |= BMCR_RESET;
 			r8152_mdio_write(tp, MII_BMCR, data);
 
-			data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
+			data = r8153_phy_status_wait(tp, PHY_STAT_LAN_ON);
 			fallthrough;
 
 		default:
@@ -5397,7 +5397,7 @@ static void r8153_init(struct r8152 *tp)
 			break;
 	}
 
-	data = r8153_phy_status(tp, 0);
+	data = r8153_phy_status_wait(tp, 0);
 
 	if (tp->version == RTL_VER_03 || tp->version == RTL_VER_04 ||
 	    tp->version == RTL_VER_05)
@@ -5409,7 +5409,7 @@ static void r8153_init(struct r8152 *tp)
 		r8152_mdio_write(tp, MII_BMCR, data);
 	}
 
-	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
+	data = r8153_phy_status_wait(tp, PHY_STAT_LAN_ON);
 
 	r8153_u2p3en(tp, false);
 
@@ -5536,7 +5536,7 @@ static void r8153b_init(struct r8152 *tp)
 			break;
 	}
 
-	data = r8153_phy_status(tp, 0);
+	data = r8153_phy_status_wait(tp, 0);
 
 	data = r8152_mdio_read(tp, MII_BMCR);
 	if (data & BMCR_PDOWN) {
@@ -5544,7 +5544,7 @@ static void r8153b_init(struct r8152 *tp)
 		r8152_mdio_write(tp, MII_BMCR, data);
 	}
 
-	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
+	data = r8153_phy_status_wait(tp, PHY_STAT_LAN_ON);
 
 	r8153_u2p3en(tp, false);
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH net-next 5/5] r8152: use *_modify helpers instead of read/write combos
  2020-11-03 19:22 [PATCH net-next 0/5] r8152 changes Marek Behún
                   ` (3 preceding siblings ...)
  2020-11-03 19:22 ` [PATCH net-next 4/5] r8152: rename r8153_phy_status to r8153_phy_status_wait Marek Behún
@ 2020-11-03 19:22 ` Marek Behún
  4 siblings, 0 replies; 26+ messages in thread
From: Marek Behún @ 2020-11-03 19:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-usb, Hayes Wang, Marek Behún

Add *_modify helpers to clear and set bits. Use these helpers such as
  x_modify(addr, clr, set);
instead of:
  reg = x_read(addr);
  reg &= ~clr;
  reg |= set;
  x_write(addr, reg);

This seems to be the way other parts of Linux are taking.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/usb/r8152.c | 841 +++++++++++-----------------------------
 1 file changed, 233 insertions(+), 608 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 1a427061da8e..e8785e9a6407 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1314,6 +1314,14 @@ static inline void _p ## _ocp_write_ ## _n(struct r8152 *tp, u16 index,	\
 					   _t data)			\
 {									\
 	ocp_write_ ## _n(tp, _mcutype, index, data);			\
+}									\
+static inline void _p ## _ocp_modify_ ## _n(struct r8152 *tp,		\
+					    u16 index, _t clr, _t set)	\
+{									\
+	_t val;								\
+	val = _p ## _ocp_read_ ## _n(tp, index);			\
+	val = (val & ~clr) | set;					\
+	_p ## _ocp_write_ ## _n(tp, index, val);			\
 }
 
 DEFINE_FUNCS_FOR_MCU_TYPE(pla, MCU_TYPE_PLA, u8, byte)
@@ -1352,6 +1360,11 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
 	pla_ocp_write_word(tp, ocp_index, data);
 }
 
+static void ocp_reg_modify(struct r8152 *tp, u16 addr, u16 clr, u16 set)
+{
+	ocp_reg_write(tp, addr, (ocp_reg_read(tp, addr) & ~clr) | set);
+}
+
 static inline void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
 {
 	ocp_reg_write(tp, OCP_BASE_MII + reg_addr * 2, value);
@@ -1362,6 +1375,16 @@ static inline int r8152_mdio_read(struct r8152 *tp, u32 reg_addr)
 	return ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2);
 }
 
+static void r8152_mdio_modify(struct r8152 *tp, u32 reg_addr, u16 clr, u16 set)
+{
+	u16 val, nval;
+
+	val = r8152_mdio_read(tp, reg_addr);
+	nval = (val & ~clr) | set;
+	if (val != nval)
+		r8152_mdio_write(tp, reg_addr, nval);
+}
+
 static void sram_write(struct r8152 *tp, u16 addr, u16 data)
 {
 	ocp_reg_write(tp, OCP_SRAM_ADDR, addr);
@@ -1374,6 +1397,11 @@ static u16 sram_read(struct r8152 *tp, u16 addr)
 	return ocp_reg_read(tp, OCP_SRAM_DATA);
 }
 
+static void sram_modify(struct r8152 *tp, u16 addr, u16 clr, u16 set)
+{
+	sram_write(tp, addr, (sram_read(tp, addr) & ~clr) | set);
+}
+
 static int read_mii_word(struct net_device *netdev, int phy_id, int reg)
 {
 	struct r8152 *tp = netdev_priv(netdev);
@@ -2552,9 +2580,7 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
 	u32 ocp_data;
 
 	netif_stop_queue(netdev);
-	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
-	ocp_data &= ~RCR_ACPT_ALL;
-	ocp_data |= RCR_AB | RCR_APM;
+	ocp_data = RCR_AB | RCR_APM;
 
 	if (netdev->flags & IFF_PROMISC) {
 		/* Unconditionally log net taps. */
@@ -2585,7 +2611,7 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
 	tmp[1] = __cpu_to_le32(swab32(mc_filter[0]));
 
 	pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(tmp), tmp);
-	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
+	pla_ocp_modify_dword(tp, PLA_RCR, RCR_ACPT_ALL, ocp_data);
 	netif_wake_queue(netdev);
 }
 
@@ -2631,13 +2657,8 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
 
 static void r8152b_reset_packet_filter(struct r8152 *tp)
 {
-	u32	ocp_data;
-
-	ocp_data = pla_ocp_read_word(tp, PLA_FMC);
-	ocp_data &= ~FMC_FCR_MCU_EN;
-	pla_ocp_write_word(tp, PLA_FMC, ocp_data);
-	ocp_data |= FMC_FCR_MCU_EN;
-	pla_ocp_write_word(tp, PLA_FMC, ocp_data);
+	pla_ocp_modify_word(tp, PLA_FMC, FMC_FCR_MCU_EN, 0);
+	pla_ocp_modify_word(tp, PLA_FMC, 0, FMC_FCR_MCU_EN);
 }
 
 static void rtl8152_nic_reset(struct r8152 *tp)
@@ -2668,31 +2689,17 @@ static inline u8 rtl8152_get_speed(struct r8152 *tp)
 
 static void rtl_set_eee_plus(struct r8152 *tp)
 {
-	u32 ocp_data;
 	u8 speed;
 
 	speed = rtl8152_get_speed(tp);
-	if (speed & _10bps) {
-		ocp_data = pla_ocp_read_word(tp, PLA_EEEP_CR);
-		ocp_data |= EEEP_CR_EEEP_TX;
-		pla_ocp_write_word(tp, PLA_EEEP_CR, ocp_data);
-	} else {
-		ocp_data = pla_ocp_read_word(tp, PLA_EEEP_CR);
-		ocp_data &= ~EEEP_CR_EEEP_TX;
-		pla_ocp_write_word(tp, PLA_EEEP_CR, ocp_data);
-	}
+	pla_ocp_modify_word(tp, PLA_EEEP_CR, EEEP_CR_EEEP_TX,
+			    (speed & _10bps) ? EEEP_CR_EEEP_TX : 0);
 }
 
 static void rxdy_gated_en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data;
-
-	ocp_data = pla_ocp_read_word(tp, PLA_MISC_1);
-	if (enable)
-		ocp_data |= RXDY_GATED_EN;
-	else
-		ocp_data &= ~RXDY_GATED_EN;
-	pla_ocp_write_word(tp, PLA_MISC_1, ocp_data);
+	pla_ocp_modify_word(tp, PLA_MISC_1, RXDY_GATED_EN,
+			    enable ? RXDY_GATED_EN : 0);
 }
 
 static int rtl_start_rx(struct r8152 *tp)
@@ -2785,13 +2792,9 @@ static inline void r8153b_rx_agg_chg_indicate(struct r8152 *tp)
 
 static int rtl_enable(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	r8152b_reset_packet_filter(tp);
 
-	ocp_data = pla_ocp_read_byte(tp, PLA_CR);
-	ocp_data |= CR_RE | CR_TE;
-	pla_ocp_write_byte(tp, PLA_CR, ocp_data);
+	pla_ocp_modify_byte(tp, PLA_CR, 0, CR_RE | CR_TE);
 
 	switch (tp->version) {
 	case RTL_VER_08:
@@ -2876,14 +2879,9 @@ static int rtl8153_enable(struct r8152 *tp)
 	r8153_set_rx_early_size(tp);
 
 	if (tp->version == RTL_VER_09) {
-		u32 ocp_data;
-
-		ocp_data = usb_ocp_read_word(tp, USB_FW_TASK);
-		ocp_data &= ~FC_PATCH_TASK;
-		usb_ocp_write_word(tp, USB_FW_TASK, ocp_data);
+		usb_ocp_modify_word(tp, USB_FW_TASK, FC_PATCH_TASK, 0);
 		usleep_range(1000, 2000);
-		ocp_data |= FC_PATCH_TASK;
-		usb_ocp_write_word(tp, USB_FW_TASK, ocp_data);
+		usb_ocp_modify_word(tp, USB_FW_TASK, 0, FC_PATCH_TASK);
 	}
 
 	return rtl_enable(tp);
@@ -2899,9 +2897,7 @@ static void rtl_disable(struct r8152 *tp)
 		return;
 	}
 
-	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
-	ocp_data &= ~RCR_ACPT_ALL;
-	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
+	pla_ocp_modify_dword(tp, PLA_RCR, RCR_ACPT_ALL, 0);
 
 	rtl_drop_queued_tx(tp);
 
@@ -2930,30 +2926,15 @@ static void rtl_disable(struct r8152 *tp)
 
 static void r8152_power_cut_en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data;
-
-	ocp_data = usb_ocp_read_word(tp, USB_UPS_CTRL);
-	if (enable)
-		ocp_data |= POWER_CUT;
-	else
-		ocp_data &= ~POWER_CUT;
-	usb_ocp_write_word(tp, USB_UPS_CTRL, ocp_data);
-
-	ocp_data = usb_ocp_read_word(tp, USB_PM_CTRL_STATUS);
-	ocp_data &= ~RESUME_INDICATE;
-	usb_ocp_write_word(tp, USB_PM_CTRL_STATUS, ocp_data);
+	usb_ocp_modify_word(tp, USB_UPS_CTRL, POWER_CUT,
+			    enable ? POWER_CUT : 0);
+	usb_ocp_modify_word(tp, USB_PM_CTRL_STATUS, RESUME_INDICATE, 0);
 }
 
 static void rtl_rx_vlan_en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data;
-
-	ocp_data = pla_ocp_read_word(tp, PLA_CPCR);
-	if (enable)
-		ocp_data |= CPCR_RX_VLAN;
-	else
-		ocp_data &= ~CPCR_RX_VLAN;
-	pla_ocp_write_word(tp, PLA_CPCR, ocp_data);
+	pla_ocp_modify_word(tp, PLA_CPCR, CPCR_RX_VLAN,
+			    enable ? CPCR_RX_VLAN : 0);
 }
 
 static int rtl8152_set_features(struct net_device *dev,
@@ -3012,33 +2993,20 @@ static u32 __rtl_get_wol(struct r8152 *tp)
 
 static void __rtl_set_wol(struct r8152 *tp, u32 wolopts)
 {
-	u32 ocp_data;
-
 	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_CONFIG);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_CONFIG34);
-	ocp_data &= ~LINK_ON_WAKE_EN;
-	if (wolopts & WAKE_PHY)
-		ocp_data |= LINK_ON_WAKE_EN;
-	pla_ocp_write_word(tp, PLA_CONFIG34, ocp_data);
+	pla_ocp_modify_word(tp, PLA_CONFIG34, LINK_ON_WAKE_EN,
+			    wolopts & WAKE_PHY ? LINK_ON_WAKE_EN : 0);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_CONFIG5);
-	ocp_data &= ~(UWF_EN | BWF_EN | MWF_EN);
-	if (wolopts & WAKE_UCAST)
-		ocp_data |= UWF_EN;
-	if (wolopts & WAKE_BCAST)
-		ocp_data |= BWF_EN;
-	if (wolopts & WAKE_MCAST)
-		ocp_data |= MWF_EN;
-	pla_ocp_write_word(tp, PLA_CONFIG5, ocp_data);
+	pla_ocp_modify_word(tp, PLA_CONFIG5, UWF_EN | BWF_EN | MWF_EN,
+			    (wolopts & WAKE_UCAST ? UWF_EN : 0) |
+			    (wolopts & WAKE_BCAST ? BWF_EN : 0) |
+			    (wolopts & WAKE_MCAST ? MWF_EN : 0));
 
 	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_CFG_WOL);
-	ocp_data &= ~MAGIC_EN;
-	if (wolopts & WAKE_MAGIC)
-		ocp_data |= MAGIC_EN;
-	pla_ocp_write_word(tp, PLA_CFG_WOL, ocp_data);
+	pla_ocp_modify_word(tp, PLA_CFG_WOL, MAGIC_EN,
+			    wolopts & WAKE_MAGIC ? MAGIC_EN : 0);
 
 	if (wolopts & WAKE_ANY)
 		device_set_wakeup_enable(&tp->udev->dev, true);
@@ -3082,27 +3050,14 @@ static void r8153_u1u2en(struct r8152 *tp, bool enable)
 
 static void r8153b_u1u2en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data;
-
-	ocp_data = usb_ocp_read_word(tp, USB_LPM_CONFIG);
-	if (enable)
-		ocp_data |= LPM_U1U2_EN;
-	else
-		ocp_data &= ~LPM_U1U2_EN;
-
-	usb_ocp_write_word(tp, USB_LPM_CONFIG, ocp_data);
+	usb_ocp_modify_word(tp, USB_LPM_CONFIG, LPM_U1U2_EN,
+			    enable ? LPM_U1U2_EN : 0);
 }
 
 static void r8153_u2p3en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data;
-
-	ocp_data = usb_ocp_read_word(tp, USB_U2P3_CTRL);
-	if (enable)
-		ocp_data |= U2P3_ENABLE;
-	else
-		ocp_data &= ~U2P3_ENABLE;
-	usb_ocp_write_word(tp, USB_U2P3_CTRL, ocp_data);
+	usb_ocp_modify_word(tp, USB_U2P3_CTRL, U2P3_ENABLE,
+			    enable ? U2P3_ENABLE : 0);
 }
 
 static void r8153b_ups_flags(struct r8152 *tp)
@@ -3179,8 +3134,6 @@ static void r8153b_ups_flags(struct r8152 *tp)
 
 static void r8153b_green_en(struct r8152 *tp, bool enable)
 {
-	u16 data;
-
 	if (enable) {
 		sram_write(tp, 0x8045, 0);	/* 10M abiq&ldvbias */
 		sram_write(tp, 0x804d, 0x1222);	/* 100M short abiq&ldvbias */
@@ -3191,9 +3144,7 @@ static void r8153b_green_en(struct r8152 *tp, bool enable)
 		sram_write(tp, 0x805d, 0x2444);	/* 1000M short abiq&ldvbias */
 	}
 
-	data = sram_read(tp, SRAM_GREEN_CFG);
-	data |= GREEN_ETH_EN;
-	sram_write(tp, SRAM_GREEN_CFG, data);
+	sram_modify(tp, SRAM_GREEN_CFG, 0, GREEN_ETH_EN);
 
 	tp->ups_info.green = enable;
 }
@@ -3224,43 +3175,27 @@ static u16 r8153_phy_status_wait(struct r8152 *tp, u16 desired)
 
 static void r8153b_ups_en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data = usb_ocp_read_byte(tp, USB_POWER_CUT);
-
-	if (enable) {
+	if (enable)
 		r8153b_ups_flags(tp);
 
-		ocp_data |= UPS_EN | USP_PREWAKE | PHASE2_EN;
-		usb_ocp_write_byte(tp, USB_POWER_CUT, ocp_data);
+	usb_ocp_modify_byte(tp, USB_POWER_CUT, UPS_EN | USP_PREWAKE,
+			    enable ? UPS_EN | USP_PREWAKE | PHASE2_EN : 0);
+	usb_ocp_modify_byte(tp, 0xcfff, BIT(0), enable ? BIT(0) : 0);
 
-		ocp_data = usb_ocp_read_byte(tp, 0xcfff);
-		ocp_data |= BIT(0);
-		usb_ocp_write_byte(tp, 0xcfff, ocp_data);
-	} else {
+	if (!enable) {
 		u16 data;
 
-		ocp_data &= ~(UPS_EN | USP_PREWAKE);
-		usb_ocp_write_byte(tp, USB_POWER_CUT, ocp_data);
-
-		ocp_data = usb_ocp_read_byte(tp, 0xcfff);
-		ocp_data &= ~BIT(0);
-		usb_ocp_write_byte(tp, 0xcfff, ocp_data);
-
-		ocp_data = usb_ocp_read_word(tp, USB_MISC_0);
-		ocp_data &= ~PCUT_STATUS;
-		usb_ocp_write_word(tp, USB_MISC_0, ocp_data);
+		usb_ocp_modify_word(tp, USB_MISC_0, PCUT_STATUS, 0);
 
 		data = r8153_phy_status_wait(tp, 0);
 
 		switch (data) {
 		case PHY_STAT_PWRDN:
 		case PHY_STAT_EXT_INIT:
-			r8153b_green_en(tp,
-					test_bit(GREEN_ETHERNET, &tp->flags));
+			r8153b_green_en(tp, test_bit(GREEN_ETHERNET,
+						     &tp->flags));
 
-			data = r8152_mdio_read(tp, MII_BMCR);
-			data &= ~BMCR_PDOWN;
-			data |= BMCR_RESET;
-			r8152_mdio_write(tp, MII_BMCR, data);
+			r8152_mdio_modify(tp, MII_BMCR, BMCR_PDOWN, BMCR_RESET);
 
 			data = r8153_phy_status_wait(tp, PHY_STAT_LAN_ON);
 			fallthrough;
@@ -3276,54 +3211,24 @@ static void r8153b_ups_en(struct r8152 *tp, bool enable)
 
 static void r8153_power_cut_en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data;
-
-	ocp_data = usb_ocp_read_word(tp, USB_POWER_CUT);
-	if (enable)
-		ocp_data |= PWR_EN | PHASE2_EN;
-	else
-		ocp_data &= ~(PWR_EN | PHASE2_EN);
-	usb_ocp_write_word(tp, USB_POWER_CUT, ocp_data);
-
-	ocp_data = usb_ocp_read_word(tp, USB_MISC_0);
-	ocp_data &= ~PCUT_STATUS;
-	usb_ocp_write_word(tp, USB_MISC_0, ocp_data);
+	usb_ocp_modify_word(tp, USB_POWER_CUT, PWR_EN | PHASE2_EN,
+			    enable ? PWR_EN | PHASE2_EN : 0);
+	usb_ocp_modify_word(tp, USB_MISC_0, PCUT_STATUS, 0);
 }
 
 static void r8153b_power_cut_en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data;
-
-	ocp_data = usb_ocp_read_word(tp, USB_POWER_CUT);
-	if (enable)
-		ocp_data |= PWR_EN | PHASE2_EN;
-	else
-		ocp_data &= ~PWR_EN;
-	usb_ocp_write_word(tp, USB_POWER_CUT, ocp_data);
-
-	ocp_data = usb_ocp_read_word(tp, USB_MISC_0);
-	ocp_data &= ~PCUT_STATUS;
-	usb_ocp_write_word(tp, USB_MISC_0, ocp_data);
+	usb_ocp_modify_word(tp, USB_POWER_CUT, PWR_EN,
+			    enable ? PWR_EN | PHASE2_EN : 0);
+	usb_ocp_modify_word(tp, USB_MISC_0, PCUT_STATUS, 0);
 }
 
 static void r8153_queue_wake(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data;
-
-	ocp_data = pla_ocp_read_byte(tp, PLA_INDICATE_FALG);
-	if (enable)
-		ocp_data |= UPCOMING_RUNTIME_D3;
-	else
-		ocp_data &= ~UPCOMING_RUNTIME_D3;
-	pla_ocp_write_byte(tp, PLA_INDICATE_FALG, ocp_data);
-
-	ocp_data = pla_ocp_read_byte(tp, PLA_SUSPEND_FLAG);
-	ocp_data &= ~LINK_CHG_EVENT;
-	pla_ocp_write_byte(tp, PLA_SUSPEND_FLAG, ocp_data);
-
-	ocp_data = pla_ocp_read_word(tp, PLA_EXTRA_STATUS);
-	ocp_data &= ~LINK_CHANGE_FLAG;
-	pla_ocp_write_word(tp, PLA_EXTRA_STATUS, ocp_data);
+	pla_ocp_modify_byte(tp, PLA_INDICATE_FALG, UPCOMING_RUNTIME_D3,
+			    enable ? UPCOMING_RUNTIME_D3 : 0);
+	pla_ocp_modify_byte(tp, PLA_SUSPEND_FLAG, LINK_CHG_EVENT, 0);
+	pla_ocp_modify_word(tp, PLA_EXTRA_STATUS, LINK_CHANGE_FLAG, 0);
 }
 
 static bool rtl_can_wakeup(struct r8152 *tp)
@@ -3335,31 +3240,12 @@ static bool rtl_can_wakeup(struct r8152 *tp)
 
 static void rtl_runtime_suspend_enable(struct r8152 *tp, bool enable)
 {
-	if (enable) {
-		u32 ocp_data;
-
-		__rtl_set_wol(tp, WAKE_ANY);
-
-		pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_CONFIG);
-
-		ocp_data = pla_ocp_read_word(tp, PLA_CONFIG34);
-		ocp_data |= LINK_OFF_WAKE_EN;
-		pla_ocp_write_word(tp, PLA_CONFIG34, ocp_data);
-
-		pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
-	} else {
-		u32 ocp_data;
-
-		__rtl_set_wol(tp, tp->saved_wolopts);
-
-		pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_CONFIG);
+	__rtl_set_wol(tp, enable ? WAKE_ANY : tp->saved_wolopts);
 
-		ocp_data = pla_ocp_read_word(tp, PLA_CONFIG34);
-		ocp_data &= ~LINK_OFF_WAKE_EN;
-		pla_ocp_write_word(tp, PLA_CONFIG34, ocp_data);
-
-		pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
-	}
+	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_CONFIG);
+	pla_ocp_modify_word(tp, PLA_CONFIG34, LINK_OFF_WAKE_EN,
+			    enable ? LINK_OFF_WAKE_EN : 0);
+	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
 }
 
 static void rtl8153_runtime_enable(struct r8152 *tp, bool enable)
@@ -3407,8 +3293,6 @@ static void rtl8153b_runtime_enable(struct r8152 *tp, bool enable)
 
 static void r8153_teredo_off(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	switch (tp->version) {
 	case RTL_VER_01:
 	case RTL_VER_02:
@@ -3417,10 +3301,9 @@ static void r8153_teredo_off(struct r8152 *tp)
 	case RTL_VER_05:
 	case RTL_VER_06:
 	case RTL_VER_07:
-		ocp_data = pla_ocp_read_word(tp, PLA_TEREDO_CFG);
-		ocp_data &= ~(TEREDO_SEL | TEREDO_RS_EVENT_MASK |
-			      OOB_TEREDO_EN);
-		pla_ocp_write_word(tp, PLA_TEREDO_CFG, ocp_data);
+		pla_ocp_modify_word(tp, PLA_TEREDO_CFG,
+				    TEREDO_SEL | TEREDO_RS_EVENT_MASK |
+				    OOB_TEREDO_EN, 0);
 		break;
 
 	case RTL_VER_08:
@@ -3442,13 +3325,10 @@ static void r8153_teredo_off(struct r8152 *tp)
 
 static void rtl_reset_bmu(struct r8152 *tp)
 {
-	u32 ocp_data;
+	u8 bits = BMU_RESET_EP_IN | BMU_RESET_EP_OUT;
 
-	ocp_data = usb_ocp_read_byte(tp, USB_BMU_RESET);
-	ocp_data &= ~(BMU_RESET_EP_IN | BMU_RESET_EP_OUT);
-	usb_ocp_write_byte(tp, USB_BMU_RESET, ocp_data);
-	ocp_data |= BMU_RESET_EP_IN | BMU_RESET_EP_OUT;
-	usb_ocp_write_byte(tp, USB_BMU_RESET, ocp_data);
+	usb_ocp_modify_byte(tp, USB_BMU_RESET, bits, 0);
+	usb_ocp_modify_byte(tp, USB_BMU_RESET, 0, bits);
 }
 
 /* Clear the bp to stop the firmware before loading a new one */
@@ -3501,15 +3381,10 @@ static void rtl_clear_bp(struct r8152 *tp, u16 type)
 
 static int r8153_patch_request(struct r8152 *tp, bool request)
 {
-	u16 data;
 	int i;
 
-	data = ocp_reg_read(tp, OCP_PHY_PATCH_CMD);
-	if (request)
-		data |= PATCH_REQUEST;
-	else
-		data &= ~PATCH_REQUEST;
-	ocp_reg_write(tp, OCP_PHY_PATCH_CMD, data);
+	ocp_reg_modify(tp, OCP_PHY_PATCH_CMD, PATCH_REQUEST,
+		       request ? PATCH_REQUEST : 0);
 
 	for (i = 0; request && i < 5000; i++) {
 		usleep_range(1000, 2000);
@@ -3541,13 +3416,9 @@ static int r8153_pre_ram_code(struct r8152 *tp, u16 key_addr, u16 patch_key)
 
 static int r8153_post_ram_code(struct r8152 *tp, u16 key_addr)
 {
-	u16 data;
-
 	sram_write(tp, 0x0000, 0x0000);
 
-	data = ocp_reg_read(tp, OCP_PHY_LOCK);
-	data &= ~PATCH_LOCK;
-	ocp_reg_write(tp, OCP_PHY_LOCK, data);
+	ocp_reg_modify(tp, OCP_PHY_LOCK, PATCH_LOCK, 0);
 
 	sram_write(tp, key_addr, 0x0000);
 
@@ -4202,22 +4073,9 @@ static void r8152_eee_en(struct r8152 *tp, bool enable)
 
 static void r8153_eee_en(struct r8152 *tp, bool enable)
 {
-	u32 ocp_data;
-	u16 config;
-
-	ocp_data = pla_ocp_read_word(tp, PLA_EEE_CR);
-	config = ocp_reg_read(tp, OCP_EEE_CFG);
-
-	if (enable) {
-		ocp_data |= EEE_RX_EN | EEE_TX_EN;
-		config |= EEE10_EN;
-	} else {
-		ocp_data &= ~(EEE_RX_EN | EEE_TX_EN);
-		config &= ~EEE10_EN;
-	}
-
-	pla_ocp_write_word(tp, PLA_EEE_CR, ocp_data);
-	ocp_reg_write(tp, OCP_EEE_CFG, config);
+	pla_ocp_modify_word(tp, PLA_EEE_CR, EEE_RX_EN | EEE_TX_EN,
+			    enable ? EEE_RX_EN | EEE_TX_EN : 0);
+	ocp_reg_modify(tp, OCP_EEE_CFG, EEE10_EN, enable ? EEE10_EN : 0);
 
 	tp->ups_info.eee = enable;
 }
@@ -4258,11 +4116,8 @@ static void rtl_eee_enable(struct r8152 *tp, bool enable)
 
 static void r8152b_enable_fc(struct r8152 *tp)
 {
-	u16 anar;
-
-	anar = r8152_mdio_read(tp, MII_ADVERTISE);
-	anar |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
-	r8152_mdio_write(tp, MII_ADVERTISE, anar);
+	r8152_mdio_modify(tp, MII_ADVERTISE, 0,
+			  ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM);
 
 	tp->ups_info.flow_control = true;
 }
@@ -4299,30 +4154,19 @@ static void wait_oob_link_list_ready(struct r8152 *tp)
 
 static void r8152b_exit_oob(struct r8152 *tp)
 {
-	u32 ocp_data;
-
-	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
-	ocp_data &= ~RCR_ACPT_ALL;
-	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
+	pla_ocp_modify_dword(tp, PLA_RCR, RCR_ACPT_ALL, 0);
 
 	rxdy_gated_en(tp, true);
 	r8153_teredo_off(tp);
 	pla_ocp_write_byte(tp, PLA_CRWECR, CRWECR_NORAML);
 	pla_ocp_write_byte(tp, PLA_CR, 0x00);
 
-	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
-	ocp_data &= ~NOW_IS_OOB;
-	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
-
-	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
-	ocp_data &= ~MCU_BORW_EN;
-	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_modify_byte(tp, PLA_OOB_CTRL, NOW_IS_OOB, 0);
+	pla_ocp_modify_word(tp, PLA_SFF_STS_7, MCU_BORW_EN, 0);
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
-	ocp_data |= RE_INIT_LL;
-	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_modify_word(tp, PLA_SFF_STS_7, 0, RE_INIT_LL);
 
 	wait_oob_link_list_ready(tp);
 
@@ -4354,18 +4198,12 @@ static void r8152b_exit_oob(struct r8152 *tp)
 
 	pla_ocp_write_word(tp, PLA_RMS, RTL8152_RMS);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_TCR0);
-	ocp_data |= TCR0_AUTO_FIFO;
-	pla_ocp_write_word(tp, PLA_TCR0, ocp_data);
+	pla_ocp_modify_word(tp, PLA_TCR0, 0, TCR0_AUTO_FIFO);
 }
 
 static void r8152b_enter_oob(struct r8152 *tp)
 {
-	u32 ocp_data;
-
-	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
-	ocp_data &= ~NOW_IS_OOB;
-	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_modify_byte(tp, PLA_OOB_CTRL, NOW_IS_OOB, 0);
 
 	pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL0, RXFIFO_THR1_OOB);
 	pla_ocp_write_dword(tp, PLA_RXFIFO_CTRL1, RXFIFO_THR2_OOB);
@@ -4375,9 +4213,7 @@ static void r8152b_enter_oob(struct r8152 *tp)
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
-	ocp_data |= RE_INIT_LL;
-	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_modify_word(tp, PLA_SFF_STS_7, 0, RE_INIT_LL);
 
 	wait_oob_link_list_ready(tp);
 
@@ -4385,19 +4221,12 @@ static void r8152b_enter_oob(struct r8152 *tp)
 
 	rtl_rx_vlan_en(tp, true);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_BDC_CR);
-	ocp_data |= ALDPS_PROXY_MODE;
-	pla_ocp_write_word(tp, PLA_BDC_CR, ocp_data);
-
-	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
-	ocp_data |= NOW_IS_OOB | DIS_MCU_CLROOB;
-	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_modify_word(tp, PLA_BDC_CR, 0, ALDPS_PROXY_MODE);
+	pla_ocp_modify_byte(tp, PLA_OOB_CTRL, 0, NOW_IS_OOB | DIS_MCU_CLROOB);
 
 	rxdy_gated_en(tp, false);
 
-	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
-	ocp_data |= RCR_APM | RCR_AM | RCR_AB;
-	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
+	pla_ocp_modify_dword(tp, PLA_RCR, 0, RCR_APM | RCR_AM | RCR_AB);
 }
 
 static int r8153_pre_firmware_1(struct r8152 *tp)
@@ -4406,9 +4235,7 @@ static int r8153_pre_firmware_1(struct r8152 *tp)
 
 	/* Wait till the WTD timer is ready. It would take at most 104 ms. */
 	for (i = 0; i < 104; i++) {
-		u32 ocp_data = usb_ocp_read_byte(tp, USB_WDT1_CTRL);
-
-		if (!(ocp_data & WTD1_EN))
+		if (!(usb_ocp_read_byte(tp, USB_WDT1_CTRL) & WTD1_EN))
 			break;
 		usleep_range(1000, 2000);
 	}
@@ -4430,56 +4257,34 @@ static int r8153_post_firmware_1(struct r8152 *tp)
 
 static int r8153_pre_firmware_2(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	r8153_pre_firmware_1(tp);
 
-	ocp_data = usb_ocp_read_word(tp, USB_FW_FIX_EN0);
-	ocp_data &= ~FW_FIX_SUSPEND;
-	usb_ocp_write_word(tp, USB_FW_FIX_EN0, ocp_data);
+	usb_ocp_modify_word(tp, USB_FW_FIX_EN0, FW_FIX_SUSPEND, 0);
 
 	return 0;
 }
 
 static int r8153_post_firmware_2(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	/* enable bp0 if support USB_SPEED_SUPER only */
-	if (usb_ocp_read_byte(tp, USB_CSTMR) & FORCE_SUPER) {
-		ocp_data = pla_ocp_read_word(tp, PLA_BP_EN);
-		ocp_data |= BIT(0);
-		pla_ocp_write_word(tp, PLA_BP_EN, ocp_data);
-	}
+	if (usb_ocp_read_byte(tp, USB_CSTMR) & FORCE_SUPER)
+		pla_ocp_modify_word(tp, PLA_BP_EN, 0, BIT(0));
 
 	/* reset UPHY timer to 36 ms */
 	pla_ocp_write_word(tp, PLA_UPHY_TIMER, 36000 / 16);
 
 	/* enable U3P3 check, set the counter to 4 */
 	pla_ocp_write_word(tp, PLA_EXTRA_STATUS, U3P3_CHECK_EN | 4);
-
-	ocp_data = usb_ocp_read_word(tp, USB_FW_FIX_EN0);
-	ocp_data |= FW_FIX_SUSPEND;
-	usb_ocp_write_word(tp, USB_FW_FIX_EN0, ocp_data);
-
-	ocp_data = usb_ocp_read_byte(tp, USB_USB2PHY);
-	ocp_data |= USB2PHY_L1 | USB2PHY_SUSPEND;
-	usb_ocp_write_byte(tp, USB_USB2PHY, ocp_data);
+	usb_ocp_modify_word(tp, USB_FW_FIX_EN0, 0, FW_FIX_SUSPEND);
+	usb_ocp_modify_byte(tp, USB_USB2PHY, 0, USB2PHY_L1 | USB2PHY_SUSPEND);
 
 	return 0;
 }
 
 static int r8153_post_firmware_3(struct r8152 *tp)
 {
-	u32 ocp_data;
-
-	ocp_data = usb_ocp_read_byte(tp, USB_USB2PHY);
-	ocp_data |= USB2PHY_L1 | USB2PHY_SUSPEND;
-	usb_ocp_write_byte(tp, USB_USB2PHY, ocp_data);
-
-	ocp_data = usb_ocp_read_word(tp, USB_FW_FIX_EN1);
-	ocp_data |= FW_IP_RESET_EN;
-	usb_ocp_write_word(tp, USB_FW_FIX_EN1, ocp_data);
+	usb_ocp_modify_byte(tp, USB_USB2PHY, 0, USB2PHY_L1 | USB2PHY_SUSPEND);
+	usb_ocp_modify_word(tp, USB_FW_FIX_EN1, 0, FW_IP_RESET_EN);
 
 	return 0;
 }
@@ -4494,44 +4299,24 @@ static int r8153b_pre_firmware_1(struct r8152 *tp)
 
 static int r8153b_post_firmware_1(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	/* enable bp0 for RTL8153-BND */
-	ocp_data = usb_ocp_read_byte(tp, USB_MISC_1);
-	if (ocp_data & BND_MASK) {
-		ocp_data = pla_ocp_read_word(tp, PLA_BP_EN);
-		ocp_data |= BIT(0);
-		pla_ocp_write_word(tp, PLA_BP_EN, ocp_data);
-	}
-
-	ocp_data = usb_ocp_read_word(tp, USB_FW_CTRL);
-	ocp_data |= FLOW_CTRL_PATCH_OPT;
-	usb_ocp_write_word(tp, USB_FW_CTRL, ocp_data);
+	if (usb_ocp_read_byte(tp, USB_MISC_1) & BND_MASK)
+		pla_ocp_modify_word(tp, PLA_BP_EN, 0, BIT(0));
 
-	ocp_data = usb_ocp_read_word(tp, USB_FW_TASK);
-	ocp_data |= FC_PATCH_TASK;
-	usb_ocp_write_word(tp, USB_FW_TASK, ocp_data);
-
-	ocp_data = usb_ocp_read_word(tp, USB_FW_FIX_EN1);
-	ocp_data |= FW_IP_RESET_EN;
-	usb_ocp_write_word(tp, USB_FW_FIX_EN1, ocp_data);
+	usb_ocp_modify_word(tp, USB_FW_CTRL, 0, FLOW_CTRL_PATCH_OPT);
+	usb_ocp_modify_word(tp, USB_FW_TASK, 0, FC_PATCH_TASK);
+	usb_ocp_modify_word(tp, USB_FW_FIX_EN1, 0, FW_IP_RESET_EN);
 
 	return 0;
 }
 
 static void r8153_aldps_en(struct r8152 *tp, bool enable)
 {
-	u16 data;
+	ocp_reg_modify(tp, OCP_POWER_CFG, EN_ALDPS, enable ? EN_ALDPS : 0);
 
-	data = ocp_reg_read(tp, OCP_POWER_CFG);
-	if (enable) {
-		data |= EN_ALDPS;
-		ocp_reg_write(tp, OCP_POWER_CFG, data);
-	} else {
+	if (!enable) {
 		int i;
 
-		data &= ~EN_ALDPS;
-		ocp_reg_write(tp, OCP_POWER_CFG, data);
 		for (i = 0; i < 20; i++) {
 			usleep_range(1000, 2000);
 			if (pla_ocp_read_word(tp, 0xe000) & 0x0100)
@@ -4544,9 +4329,6 @@ static void r8153_aldps_en(struct r8152 *tp, bool enable)
 
 static void r8153_hw_phy_cfg(struct r8152 *tp)
 {
-	u32 ocp_data;
-	u16 data;
-
 	/* disable ALDPS before updating the PHY parameters */
 	r8153_aldps_en(tp, false);
 
@@ -4555,27 +4337,16 @@ static void r8153_hw_phy_cfg(struct r8152 *tp)
 
 	rtl8152_apply_firmware(tp);
 
-	if (tp->version == RTL_VER_03) {
-		data = ocp_reg_read(tp, OCP_EEE_CFG);
-		data &= ~CTAP_SHORT_EN;
-		ocp_reg_write(tp, OCP_EEE_CFG, data);
-	}
+	if (tp->version == RTL_VER_03)
+		ocp_reg_modify(tp, OCP_EEE_CFG, CTAP_SHORT_EN, 0);
 
-	data = ocp_reg_read(tp, OCP_POWER_CFG);
-	data |= EEE_CLKDIV_EN;
-	ocp_reg_write(tp, OCP_POWER_CFG, data);
+	ocp_reg_modify(tp, OCP_POWER_CFG, 0, EEE_CLKDIV_EN);
 
-	data = ocp_reg_read(tp, OCP_DOWN_SPEED);
-	data |= EN_10M_BGOFF;
-	ocp_reg_write(tp, OCP_DOWN_SPEED, data);
-	data = ocp_reg_read(tp, OCP_POWER_CFG);
-	data |= EN_10M_PLLOFF;
-	ocp_reg_write(tp, OCP_POWER_CFG, data);
+	ocp_reg_modify(tp, OCP_DOWN_SPEED, 0, EN_10M_BGOFF);
+	ocp_reg_modify(tp, OCP_POWER_CFG, 0, EN_10M_PLLOFF);
 	sram_write(tp, SRAM_IMPEDANCE, 0x0b13);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_PHY_PWR);
-	ocp_data |= PFM_PWM_SWITCH;
-	pla_ocp_write_word(tp, PLA_PHY_PWR, ocp_data);
+	pla_ocp_modify_word(tp, PLA_PHY_PWR, 0, PFM_PWM_SWITCH);
 
 	/* Enable LPF corner auto tune */
 	sram_write(tp, SRAM_LPF_CFG, 0xf70f);
@@ -4618,8 +4389,7 @@ static u32 r8152_efuse_read(struct r8152 *tp, u8 addr)
 
 static void r8153b_hw_phy_cfg(struct r8152 *tp)
 {
-	u32 ocp_data;
-	u16 data;
+	u32 data;
 
 	/* disable ALDPS before updating the PHY parameters */
 	r8153_aldps_en(tp, false);
@@ -4631,20 +4401,16 @@ static void r8153b_hw_phy_cfg(struct r8152 *tp)
 
 	r8153b_green_en(tp, test_bit(GREEN_ETHERNET, &tp->flags));
 
-	data = sram_read(tp, SRAM_GREEN_CFG);
-	data |= R_TUNE_EN;
-	sram_write(tp, SRAM_GREEN_CFG, data);
-	data = ocp_reg_read(tp, OCP_NCTL_CFG);
-	data |= PGA_RETURN_EN;
-	ocp_reg_write(tp, OCP_NCTL_CFG, data);
+	sram_modify(tp, SRAM_GREEN_CFG, 0, R_TUNE_EN);
+	ocp_reg_modify(tp, OCP_NCTL_CFG, 0, PGA_RETURN_EN);
 
 	/* ADC Bias Calibration:
 	 * read efuse offset 0x7d to get a 17-bit data. Remove the dummy/fake
 	 * bit (bit3) to rebuild the real 16-bit data. Write the data to the
 	 * ADC ioffset.
 	 */
-	ocp_data = r8152_efuse_read(tp, 0x7d);
-	data = (u16)(((ocp_data & 0x1fff0) >> 1) | (ocp_data & 0x7));
+	data = r8152_efuse_read(tp, 0x7d);
+	data = ((data & 0x1fff0) >> 1) | (data & 0x7);
 	if (data != 0xffff)
 		ocp_reg_write(tp, OCP_ADC_IOFFSET, data);
 
@@ -4652,31 +4418,20 @@ static void r8153b_hw_phy_cfg(struct r8152 *tp)
 	 * rg_saw_cnt = OCP reg 0xC426 Bit[13:0]
 	 * swr_cnt_1ms_ini = 16000000 / rg_saw_cnt
 	 */
-	ocp_data = ocp_reg_read(tp, 0xc426);
-	ocp_data &= 0x3fff;
-	if (ocp_data) {
-		u32 swr_cnt_1ms_ini;
+	data = ocp_reg_read(tp, 0xc426) & 0x3fff;
+	if (data)
+		usb_ocp_modify_word(tp, USB_UPS_CFG, SAW_CNT_1MS_MASK,
+				    (16000000 / data) & SAW_CNT_1MS_MASK);
 
-		swr_cnt_1ms_ini = (16000000 / ocp_data) & SAW_CNT_1MS_MASK;
-		ocp_data = usb_ocp_read_word(tp, USB_UPS_CFG);
-		ocp_data = (ocp_data & ~SAW_CNT_1MS_MASK) | swr_cnt_1ms_ini;
-		usb_ocp_write_word(tp, USB_UPS_CFG, ocp_data);
-	}
-
-	ocp_data = pla_ocp_read_word(tp, PLA_PHY_PWR);
-	ocp_data |= PFM_PWM_SWITCH;
-	pla_ocp_write_word(tp, PLA_PHY_PWR, ocp_data);
+	pla_ocp_modify_word(tp, PLA_PHY_PWR, 0, PFM_PWM_SWITCH);
 
 	/* Advnace EEE */
 	if (!r8153_patch_request(tp, true)) {
-		data = ocp_reg_read(tp, OCP_POWER_CFG);
-		data |= EEE_CLKDIV_EN;
-		ocp_reg_write(tp, OCP_POWER_CFG, data);
+		ocp_reg_modify(tp, OCP_POWER_CFG, 0, EEE_CLKDIV_EN);
 		tp->ups_info.eee_ckdiv = true;
 
-		data = ocp_reg_read(tp, OCP_DOWN_SPEED);
-		data |= EN_EEE_CMODE | EN_EEE_1000 | EN_10M_CLKDIV;
-		ocp_reg_write(tp, OCP_DOWN_SPEED, data);
+		ocp_reg_modify(tp, OCP_DOWN_SPEED, 0,
+			       EN_EEE_CMODE | EN_EEE_1000 | EN_10M_CLKDIV);
 		tp->ups_info.eee_cmod_lv = true;
 		tp->ups_info._10m_ckdiv = true;
 		tp->ups_info.eee_plloff_giga = true;
@@ -4699,44 +4454,31 @@ static void r8153b_hw_phy_cfg(struct r8152 *tp)
 
 static void r8153_first_init(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	r8153_mac_clk_spd(tp, false);
 	rxdy_gated_en(tp, true);
 	r8153_teredo_off(tp);
 
-	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
-	ocp_data &= ~RCR_ACPT_ALL;
-	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
+	pla_ocp_modify_dword(tp, PLA_RCR, RCR_ACPT_ALL, 0);
 
 	rtl8152_nic_reset(tp);
 	rtl_reset_bmu(tp);
 
-	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
-	ocp_data &= ~NOW_IS_OOB;
-	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
-
-	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
-	ocp_data &= ~MCU_BORW_EN;
-	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_modify_byte(tp, PLA_OOB_CTRL, NOW_IS_OOB, 0);
+	pla_ocp_modify_word(tp, PLA_SFF_STS_7, MCU_BORW_EN, 0);
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
-	ocp_data |= RE_INIT_LL;
-	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_modify_word(tp, PLA_SFF_STS_7, 0, RE_INIT_LL);
 
 	wait_oob_link_list_ready(tp);
 
 	rtl_rx_vlan_en(tp, tp->netdev->features & NETIF_F_HW_VLAN_CTAG_RX);
 
-	ocp_data = tp->netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
-	pla_ocp_write_word(tp, PLA_RMS, ocp_data);
+	pla_ocp_write_word(tp, PLA_RMS,
+			   tp->netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN);
 	pla_ocp_write_byte(tp, PLA_MTPS, MTPS_JUMBO);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_TCR0);
-	ocp_data |= TCR0_AUTO_FIFO;
-	pla_ocp_write_word(tp, PLA_TCR0, ocp_data);
+	pla_ocp_modify_word(tp, PLA_TCR0, 0, TCR0_AUTO_FIFO);
 
 	rtl8152_nic_reset(tp);
 
@@ -4750,36 +4492,28 @@ static void r8153_first_init(struct r8152 *tp)
 
 static void r8153_enter_oob(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	r8153_mac_clk_spd(tp, true);
 
-	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
-	ocp_data &= ~NOW_IS_OOB;
-	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_modify_byte(tp, PLA_OOB_CTRL, NOW_IS_OOB, 0);
 
 	rtl_disable(tp);
 	rtl_reset_bmu(tp);
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_SFF_STS_7);
-	ocp_data |= RE_INIT_LL;
-	pla_ocp_write_word(tp, PLA_SFF_STS_7, ocp_data);
+	pla_ocp_modify_word(tp, PLA_SFF_STS_7, 0, RE_INIT_LL);
 
 	wait_oob_link_list_ready(tp);
 
-	ocp_data = tp->netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
-	pla_ocp_write_word(tp, PLA_RMS, ocp_data);
+	pla_ocp_write_word(tp, PLA_RMS,
+			   tp->netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN);
 
 	switch (tp->version) {
 	case RTL_VER_03:
 	case RTL_VER_04:
 	case RTL_VER_05:
 	case RTL_VER_06:
-		ocp_data = pla_ocp_read_word(tp, PLA_TEREDO_CFG);
-		ocp_data &= ~TEREDO_WAKE_MASK;
-		pla_ocp_write_word(tp, PLA_TEREDO_CFG, ocp_data);
+		pla_ocp_modify_word(tp, PLA_TEREDO_CFG, TEREDO_WAKE_MASK, 0);
 		break;
 
 	case RTL_VER_08:
@@ -4797,19 +4531,12 @@ static void r8153_enter_oob(struct r8152 *tp)
 
 	rtl_rx_vlan_en(tp, true);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_BDC_CR);
-	ocp_data |= ALDPS_PROXY_MODE;
-	pla_ocp_write_word(tp, PLA_BDC_CR, ocp_data);
-
-	ocp_data = pla_ocp_read_byte(tp, PLA_OOB_CTRL);
-	ocp_data |= NOW_IS_OOB | DIS_MCU_CLROOB;
-	pla_ocp_write_byte(tp, PLA_OOB_CTRL, ocp_data);
+	pla_ocp_modify_word(tp, PLA_BDC_CR, 0, ALDPS_PROXY_MODE);
+	pla_ocp_modify_byte(tp, PLA_OOB_CTRL, 0, NOW_IS_OOB | DIS_MCU_CLROOB);
 
 	rxdy_gated_en(tp, false);
 
-	ocp_data = pla_ocp_read_dword(tp, PLA_RCR);
-	ocp_data |= RCR_APM | RCR_AM | RCR_AB;
-	pla_ocp_write_dword(tp, PLA_RCR, ocp_data);
+	pla_ocp_modify_dword(tp, PLA_RCR, 0, RCR_APM | RCR_AM | RCR_AB);
 }
 
 static void rtl8153_disable(struct r8152 *tp)
@@ -4868,7 +4595,7 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
 
 		tp->mii.force_media = 1;
 	} else {
-		u16 anar, tmp1;
+		u16 anar = 0;
 		u32 support;
 
 		support = RTL_ADVERTISED_10_HALF | RTL_ADVERTISED_10_FULL |
@@ -4880,46 +4607,42 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
 		if (!(advertising & support))
 			return -EINVAL;
 
-		anar = r8152_mdio_read(tp, MII_ADVERTISE);
-		tmp1 = anar & ~(ADVERTISE_10HALF | ADVERTISE_10FULL |
-				ADVERTISE_100HALF | ADVERTISE_100FULL);
 		if (advertising & RTL_ADVERTISED_10_HALF) {
-			tmp1 |= ADVERTISE_10HALF;
+			anar |= ADVERTISE_10HALF;
 			tp->ups_info.speed_duplex = NWAY_10M_HALF;
 		}
 		if (advertising & RTL_ADVERTISED_10_FULL) {
-			tmp1 |= ADVERTISE_10FULL;
+			anar |= ADVERTISE_10FULL;
 			tp->ups_info.speed_duplex = NWAY_10M_FULL;
 		}
 
 		if (advertising & RTL_ADVERTISED_100_HALF) {
-			tmp1 |= ADVERTISE_100HALF;
+			anar |= ADVERTISE_100HALF;
 			tp->ups_info.speed_duplex = NWAY_100M_HALF;
 		}
 		if (advertising & RTL_ADVERTISED_100_FULL) {
-			tmp1 |= ADVERTISE_100FULL;
+			anar |= ADVERTISE_100FULL;
 			tp->ups_info.speed_duplex = NWAY_100M_FULL;
 		}
 
-		if (anar != tmp1) {
-			r8152_mdio_write(tp, MII_ADVERTISE, tmp1);
-			tp->mii.advertising = tmp1;
-		}
+		r8152_mdio_modify(tp, MII_ADVERTISE,
+				  ADVERTISE_10HALF | ADVERTISE_10FULL |
+				  ADVERTISE_100HALF | ADVERTISE_100FULL,
+				  anar);
+		tp->mii.advertising = anar;
 
 		if (tp->mii.supports_gmii) {
-			u16 gbcr;
-
-			gbcr = r8152_mdio_read(tp, MII_CTRL1000);
-			tmp1 = gbcr & ~(ADVERTISE_1000FULL |
-					ADVERTISE_1000HALF);
+			u16 gbcr = 0;
 
 			if (advertising & RTL_ADVERTISED_1000_FULL) {
-				tmp1 |= ADVERTISE_1000FULL;
+				gbcr |= ADVERTISE_1000FULL;
 				tp->ups_info.speed_duplex = NWAY_1000M_FULL;
 			}
 
-			if (gbcr != tmp1)
-				r8152_mdio_write(tp, MII_CTRL1000, tmp1);
+			r8152_mdio_modify(tp, MII_CTRL1000,
+					  ADVERTISE_1000FULL |
+					  ADVERTISE_1000HALF,
+					  gbcr);
 		}
 
 		bmcr = BMCR_ANENABLE | BMCR_ANRESTART;
@@ -4971,8 +4694,6 @@ static void rtl8152_down(struct r8152 *tp)
 
 static void rtl8153_up(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	if (test_bit(RTL8152_UNPLUG, &tp->flags))
 		return;
 
@@ -4981,17 +4702,9 @@ static void rtl8153_up(struct r8152 *tp)
 	r8153_aldps_en(tp, false);
 	r8153_first_init(tp);
 
-	ocp_data = pla_ocp_read_byte(tp, PLA_CONFIG6);
-	ocp_data |= LANWAKE_CLR_EN;
-	pla_ocp_write_byte(tp, PLA_CONFIG6, ocp_data);
-
-	ocp_data = pla_ocp_read_byte(tp, PLA_LWAKE_CTRL_REG);
-	ocp_data &= ~LANWAKE_PIN;
-	pla_ocp_write_byte(tp, PLA_LWAKE_CTRL_REG, ocp_data);
-
-	ocp_data = usb_ocp_read_word(tp, USB_SSPHYLINK1);
-	ocp_data &= ~DELAY_PHY_PWR_CHG;
-	usb_ocp_write_word(tp, USB_SSPHYLINK1, ocp_data);
+	pla_ocp_modify_byte(tp, PLA_CONFIG6, 0, LANWAKE_CLR_EN);
+	pla_ocp_modify_byte(tp, PLA_LWAKE_CTRL_REG, LANWAKE_PIN, 0);
+	usb_ocp_modify_word(tp, USB_SSPHYLINK1, DELAY_PHY_PWR_CHG, 0);
 
 	r8153_aldps_en(tp, true);
 
@@ -5011,16 +4724,12 @@ static void rtl8153_up(struct r8152 *tp)
 
 static void rtl8153_down(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	if (test_bit(RTL8152_UNPLUG, &tp->flags)) {
 		rtl_drop_queued_tx(tp);
 		return;
 	}
 
-	ocp_data = pla_ocp_read_byte(tp, PLA_CONFIG6);
-	ocp_data &= ~LANWAKE_CLR_EN;
-	pla_ocp_write_byte(tp, PLA_CONFIG6, ocp_data);
+	pla_ocp_modify_byte(tp, PLA_CONFIG6, LANWAKE_CLR_EN, 0);
 
 	r8153_u1u2en(tp, false);
 	r8153_u2p3en(tp, false);
@@ -5032,8 +4741,6 @@ static void rtl8153_down(struct r8152 *tp)
 
 static void rtl8153b_up(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	if (test_bit(RTL8152_UNPLUG, &tp->flags))
 		return;
 
@@ -5044,9 +4751,7 @@ static void rtl8153b_up(struct r8152 *tp)
 	r8153_first_init(tp);
 	usb_ocp_write_dword(tp, USB_RX_BUF_TH, RX_THR_B);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_MAC_PWR_CTRL3);
-	ocp_data &= ~PLA_MCU_SPDWN_EN;
-	pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL3, ocp_data);
+	pla_ocp_modify_word(tp, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN, 0);
 
 	r8153_aldps_en(tp, true);
 
@@ -5056,16 +4761,12 @@ static void rtl8153b_up(struct r8152 *tp)
 
 static void rtl8153b_down(struct r8152 *tp)
 {
-	u32 ocp_data;
-
 	if (test_bit(RTL8152_UNPLUG, &tp->flags)) {
 		rtl_drop_queued_tx(tp);
 		return;
 	}
 
-	ocp_data = pla_ocp_read_word(tp, PLA_MAC_PWR_CTRL3);
-	ocp_data |= PLA_MCU_SPDWN_EN;
-	pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL3, ocp_data);
+	pla_ocp_modify_word(tp, PLA_MAC_PWR_CTRL3, 0, PLA_MCU_SPDWN_EN);
 
 	r8153b_u1u2en(tp, false);
 	r8153_u2p3en(tp, false);
@@ -5327,60 +5028,40 @@ static int rtl8152_close(struct net_device *netdev)
 
 static void rtl_tally_reset(struct r8152 *tp)
 {
-	u32 ocp_data;
-
-	ocp_data = pla_ocp_read_word(tp, PLA_RSTTALLY);
-	ocp_data |= TALLY_RESET;
-	pla_ocp_write_word(tp, PLA_RSTTALLY, ocp_data);
+	pla_ocp_modify_word(tp, PLA_RSTTALLY, 0, TALLY_RESET);
 }
 
 static void r8152b_init(struct r8152 *tp)
 {
-	u32 ocp_data;
-	u16 data;
-
 	if (test_bit(RTL8152_UNPLUG, &tp->flags))
 		return;
 
-	data = r8152_mdio_read(tp, MII_BMCR);
-	if (data & BMCR_PDOWN) {
-		data &= ~BMCR_PDOWN;
-		r8152_mdio_write(tp, MII_BMCR, data);
-	}
+	r8152_mdio_modify(tp, MII_BMCR, BMCR_PDOWN, 0);
 
 	r8152_aldps_en(tp, false);
 
-	if (tp->version == RTL_VER_01) {
-		ocp_data = pla_ocp_read_word(tp, PLA_LED_FEATURE);
-		ocp_data &= ~LED_MODE_MASK;
-		pla_ocp_write_word(tp, PLA_LED_FEATURE, ocp_data);
-	}
+	if (tp->version == RTL_VER_01)
+		pla_ocp_modify_word(tp, PLA_LED_FEATURE, LED_MODE_MASK, 0);
 
 	r8152_power_cut_en(tp, false);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_PHY_PWR);
-	ocp_data |= TX_10M_IDLE_EN | PFM_PWM_SWITCH;
-	pla_ocp_write_word(tp, PLA_PHY_PWR, ocp_data);
-	ocp_data = pla_ocp_read_dword(tp, PLA_MAC_PWR_CTRL);
-	ocp_data &= ~MCU_CLK_RATIO_MASK;
-	ocp_data |= MCU_CLK_RATIO | D3_CLK_GATED_EN;
-	pla_ocp_write_dword(tp, PLA_MAC_PWR_CTRL, ocp_data);
-	ocp_data = GPHY_STS_MSK | SPEED_DOWN_MSK |
-		   SPDWN_RXDV_MSK | SPDWN_LINKCHG_MSK;
-	pla_ocp_write_word(tp, PLA_GPHY_INTR_IMR, ocp_data);
+	pla_ocp_modify_word(tp, PLA_PHY_PWR, 0,
+			    TX_10M_IDLE_EN | PFM_PWM_SWITCH);
+	pla_ocp_modify_dword(tp, PLA_MAC_PWR_CTRL, MCU_CLK_RATIO_MASK,
+			     MCU_CLK_RATIO | D3_CLK_GATED_EN);
+	pla_ocp_write_word(tp, PLA_GPHY_INTR_IMR,
+			   GPHY_STS_MSK | SPEED_DOWN_MSK |
+			   SPDWN_RXDV_MSK | SPDWN_LINKCHG_MSK);
 
 	rtl_tally_reset(tp);
 
 	/* enable rx aggregation */
-	ocp_data = usb_ocp_read_word(tp, USB_USB_CTRL);
-	ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
-	usb_ocp_write_word(tp, USB_USB_CTRL, ocp_data);
+	usb_ocp_modify_word(tp, USB_USB_CTRL, RX_AGG_DISABLE | RX_ZERO_EN, 0);
 }
 
 static void r8153_init(struct r8152 *tp)
 {
-	u32 ocp_data;
-	u16 data;
+	u32 set;
 	int i;
 
 	if (test_bit(RTL8152_UNPLUG, &tp->flags))
@@ -5397,84 +5078,54 @@ static void r8153_init(struct r8152 *tp)
 			break;
 	}
 
-	data = r8153_phy_status_wait(tp, 0);
+	r8153_phy_status_wait(tp, 0);
 
 	if (tp->version == RTL_VER_03 || tp->version == RTL_VER_04 ||
 	    tp->version == RTL_VER_05)
 		ocp_reg_write(tp, OCP_ADC_CFG, CKADSEL_L | ADC_EN | EN_EMI_L);
 
-	data = r8152_mdio_read(tp, MII_BMCR);
-	if (data & BMCR_PDOWN) {
-		data &= ~BMCR_PDOWN;
-		r8152_mdio_write(tp, MII_BMCR, data);
-	}
+	r8152_mdio_modify(tp, MII_BMCR, BMCR_PDOWN, 0);
 
-	data = r8153_phy_status_wait(tp, PHY_STAT_LAN_ON);
+	r8153_phy_status_wait(tp, PHY_STAT_LAN_ON);
 
 	r8153_u2p3en(tp, false);
 
 	if (tp->version == RTL_VER_04) {
-		ocp_data = usb_ocp_read_word(tp, USB_SSPHYLINK2);
-		ocp_data &= ~pwd_dn_scale_mask;
-		ocp_data |= pwd_dn_scale(96);
-		usb_ocp_write_word(tp, USB_SSPHYLINK2, ocp_data);
-
-		ocp_data = usb_ocp_read_byte(tp, USB_USB2PHY);
-		ocp_data |= USB2PHY_L1 | USB2PHY_SUSPEND;
-		usb_ocp_write_byte(tp, USB_USB2PHY, ocp_data);
+		usb_ocp_modify_word(tp, USB_SSPHYLINK2, pwd_dn_scale_mask,
+				    pwd_dn_scale(96));
+		usb_ocp_modify_byte(tp, USB_USB2PHY, 0,
+				    USB2PHY_L1 | USB2PHY_SUSPEND);
 	} else if (tp->version == RTL_VER_05) {
-		ocp_data = pla_ocp_read_byte(tp, PLA_DMY_REG0);
-		ocp_data &= ~ECM_ALDPS;
-		pla_ocp_write_byte(tp, PLA_DMY_REG0, ocp_data);
+		pla_ocp_modify_byte(tp, PLA_DMY_REG0, ECM_ALDPS, 0);
 
-		ocp_data = usb_ocp_read_byte(tp, USB_CSR_DUMMY1);
-		if (usb_ocp_read_word(tp, USB_BURST_SIZE) == 0)
-			ocp_data &= ~DYNAMIC_BURST;
-		else
-			ocp_data |= DYNAMIC_BURST;
-		usb_ocp_write_byte(tp, USB_CSR_DUMMY1, ocp_data);
+		set = usb_ocp_read_word(tp, USB_BURST_SIZE) ? DYNAMIC_BURST : 0;
+		usb_ocp_modify_byte(tp, USB_CSR_DUMMY1, DYNAMIC_BURST, set);
 	} else if (tp->version == RTL_VER_06) {
-		ocp_data = usb_ocp_read_byte(tp, USB_CSR_DUMMY1);
-		if (usb_ocp_read_word(tp, USB_BURST_SIZE) == 0)
-			ocp_data &= ~DYNAMIC_BURST;
-		else
-			ocp_data |= DYNAMIC_BURST;
-		usb_ocp_write_byte(tp, USB_CSR_DUMMY1, ocp_data);
+		set = usb_ocp_read_word(tp, USB_BURST_SIZE) ? DYNAMIC_BURST : 0;
+		usb_ocp_modify_byte(tp, USB_CSR_DUMMY1, DYNAMIC_BURST, set);
 
 		r8153_queue_wake(tp, false);
 
-		ocp_data = pla_ocp_read_word(tp, PLA_EXTRA_STATUS);
+		set = POLL_LINK_CHG;
 		if (rtl8152_get_speed(tp) & LINK_STATUS)
-			ocp_data |= CUR_LINK_OK;
-		else
-			ocp_data &= ~CUR_LINK_OK;
-		ocp_data |= POLL_LINK_CHG;
-		pla_ocp_write_word(tp, PLA_EXTRA_STATUS, ocp_data);
+			set |= CUR_LINK_OK;
+		pla_ocp_modify_word(tp, PLA_EXTRA_STATUS, CUR_LINK_OK, set);
 	}
 
-	ocp_data = usb_ocp_read_byte(tp, USB_CSR_DUMMY2);
-	ocp_data |= EP4_FULL_FC;
-	usb_ocp_write_byte(tp, USB_CSR_DUMMY2, ocp_data);
-
-	ocp_data = usb_ocp_read_word(tp, USB_WDT11_CTRL);
-	ocp_data &= ~TIMER11_EN;
-	usb_ocp_write_word(tp, USB_WDT11_CTRL, ocp_data);
+	usb_ocp_modify_byte(tp, USB_CSR_DUMMY2, 0, EP4_FULL_FC);
+	usb_ocp_modify_word(tp, USB_WDT11_CTRL, TIMER11_EN, 0);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_LED_FEATURE);
-	ocp_data &= ~LED_MODE_MASK;
-	pla_ocp_write_word(tp, PLA_LED_FEATURE, ocp_data);
+	pla_ocp_modify_word(tp, PLA_LED_FEATURE, LED_MODE_MASK, 0);
 
-	ocp_data = FIFO_EMPTY_1FB | ROK_EXIT_LPM;
+	set = FIFO_EMPTY_1FB | ROK_EXIT_LPM;
 	if (tp->version == RTL_VER_04 && tp->udev->speed < USB_SPEED_SUPER)
-		ocp_data |= LPM_TIMER_500MS;
+		set |= LPM_TIMER_500MS;
 	else
-		ocp_data |= LPM_TIMER_500US;
-	usb_ocp_write_byte(tp, USB_LPM_CTRL, ocp_data);
+		set |= LPM_TIMER_500US;
+	usb_ocp_write_byte(tp, USB_LPM_CTRL, set);
 
-	ocp_data = usb_ocp_read_word(tp, USB_AFE_CTRL2);
-	ocp_data &= ~SEN_VAL_MASK;
-	ocp_data |= SEN_VAL_NORMAL | SEL_RXIDLE;
-	usb_ocp_write_word(tp, USB_AFE_CTRL2, ocp_data);
+	usb_ocp_modify_word(tp, USB_AFE_CTRL2, SEN_VAL_MASK,
+			    SEN_VAL_NORMAL | SEL_RXIDLE);
 
 	usb_ocp_write_word(tp, USB_CONNECT_TIMER, 0x0001);
 
@@ -5484,21 +5135,13 @@ static void r8153_init(struct r8152 *tp)
 	r8153_mac_clk_spd(tp, false);
 	usb_enable_lpm(tp->udev);
 
-	ocp_data = pla_ocp_read_byte(tp, PLA_CONFIG6);
-	ocp_data |= LANWAKE_CLR_EN;
-	pla_ocp_write_byte(tp, PLA_CONFIG6, ocp_data);
-
-	ocp_data = pla_ocp_read_byte(tp, PLA_LWAKE_CTRL_REG);
-	ocp_data &= ~LANWAKE_PIN;
-	pla_ocp_write_byte(tp, PLA_LWAKE_CTRL_REG, ocp_data);
+	pla_ocp_modify_byte(tp, PLA_CONFIG6, 0, LANWAKE_CLR_EN);
+	pla_ocp_modify_byte(tp, PLA_LWAKE_CTRL_REG, LANWAKE_PIN, 0);
 
 	/* rx aggregation */
-	ocp_data = usb_ocp_read_word(tp, USB_USB_CTRL);
-	ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
-	if (test_bit(DELL_TB_RX_AGG_BUG, &tp->flags))
-		ocp_data |= RX_AGG_DISABLE;
-
-	usb_ocp_write_word(tp, USB_USB_CTRL, ocp_data);
+	usb_ocp_modify_word(tp, USB_USB_CTRL, RX_AGG_DISABLE | RX_ZERO_EN,
+			    test_bit(DELL_TB_RX_AGG_BUG, &tp->flags) ?
+				RX_AGG_DISABLE : 0);
 
 	rtl_tally_reset(tp);
 
@@ -5518,8 +5161,7 @@ static void r8153_init(struct r8152 *tp)
 
 static void r8153b_init(struct r8152 *tp)
 {
-	u32 ocp_data;
-	u16 data;
+	u32 set;
 	int i;
 
 	if (test_bit(RTL8152_UNPLUG, &tp->flags))
@@ -5536,15 +5178,11 @@ static void r8153b_init(struct r8152 *tp)
 			break;
 	}
 
-	data = r8153_phy_status_wait(tp, 0);
+	r8153_phy_status_wait(tp, 0);
 
-	data = r8152_mdio_read(tp, MII_BMCR);
-	if (data & BMCR_PDOWN) {
-		data &= ~BMCR_PDOWN;
-		r8152_mdio_write(tp, MII_BMCR, data);
-	}
+	r8152_mdio_modify(tp, MII_BMCR, BMCR_PDOWN, 0);
 
-	data = r8153_phy_status_wait(tp, PHY_STAT_LAN_ON);
+	r8153_phy_status_wait(tp, PHY_STAT_LAN_ON);
 
 	r8153_u2p3en(tp, false);
 
@@ -5559,42 +5197,30 @@ static void r8153b_init(struct r8152 *tp)
 	r8153_queue_wake(tp, false);
 	rtl_runtime_suspend_enable(tp, false);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_EXTRA_STATUS);
+	set = POLL_LINK_CHG;
 	if (rtl8152_get_speed(tp) & LINK_STATUS)
-		ocp_data |= CUR_LINK_OK;
-	else
-		ocp_data &= ~CUR_LINK_OK;
-	ocp_data |= POLL_LINK_CHG;
-	pla_ocp_write_word(tp, PLA_EXTRA_STATUS, ocp_data);
+		set |= CUR_LINK_OK;
+	pla_ocp_modify_word(tp, PLA_EXTRA_STATUS, CUR_LINK_OK, set);
 
 	if (tp->udev->speed != USB_SPEED_HIGH)
 		r8153b_u1u2en(tp, true);
 	usb_enable_lpm(tp->udev);
 
 	/* MAC clock speed down */
-	ocp_data = pla_ocp_read_word(tp, PLA_MAC_PWR_CTRL2);
-	ocp_data |= MAC_CLK_SPDWN_EN;
-	pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL2, ocp_data);
+	pla_ocp_modify_word(tp, PLA_MAC_PWR_CTRL2, 0, MAC_CLK_SPDWN_EN);
 
-	ocp_data = pla_ocp_read_word(tp, PLA_MAC_PWR_CTRL3);
-	ocp_data &= ~PLA_MCU_SPDWN_EN;
-	pla_ocp_write_word(tp, PLA_MAC_PWR_CTRL3, ocp_data);
+	pla_ocp_modify_word(tp, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN, 0);
 
 	if (tp->version == RTL_VER_09) {
 		/* Disable Test IO for 32QFN */
-		if (pla_ocp_read_byte(tp, 0xdc00) & BIT(5)) {
-			ocp_data = pla_ocp_read_word(tp, PLA_PHY_PWR);
-			ocp_data |= TEST_IO_OFF;
-			pla_ocp_write_word(tp, PLA_PHY_PWR, ocp_data);
-		}
+		if (pla_ocp_read_byte(tp, 0xdc00) & BIT(5))
+			pla_ocp_modify_word(tp, PLA_PHY_PWR, 0, TEST_IO_OFF);
 	}
 
 	set_bit(GREEN_ETHERNET, &tp->flags);
 
 	/* rx aggregation */
-	ocp_data = usb_ocp_read_word(tp, USB_USB_CTRL);
-	ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
-	usb_ocp_write_word(tp, USB_USB_CTRL, ocp_data);
+	usb_ocp_modify_word(tp, USB_USB_CTRL, RX_AGG_DISABLE | RX_ZERO_EN, 0);
 
 	rtl_tally_reset(tp);
 
@@ -6451,9 +6077,8 @@ static int rtl8152_change_mtu(struct net_device *dev, int new_mtu)
 	dev->mtu = new_mtu;
 
 	if (netif_running(dev)) {
-		u32 rms = new_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
-
-		pla_ocp_write_word(tp, PLA_RMS, rms);
+		pla_ocp_write_word(tp, PLA_RMS,
+				   new_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN);
 
 		if (netif_carrier_ok(dev))
 			r8153_set_rx_early_size(tp);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-03 19:22 ` [PATCH net-next 3/5] r8152: add MCU typed read/write functions Marek Behún
@ 2020-11-03 21:47   ` Vladimir Oltean
  2020-11-04  5:55     ` Marek Behún
  0 siblings, 1 reply; 26+ messages in thread
From: Vladimir Oltean @ 2020-11-03 21:47 UTC (permalink / raw)
  To: Marek Behún; +Cc: netdev, linux-usb, Hayes Wang

On Tue, Nov 03, 2020 at 08:22:24PM +0100, Marek Behún wrote:
> Add pla_ and usb_ prefixed versions of ocp_read_* and ocp_write_*
> functions. This saves us from always writing MCU_TYPE_PLA/MCU_TYPE_USB
> as parameter.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---

You just made it harder for everyone to follow the code through pattern
matching. Token concatenation should be banned from the C preprocessor.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH net-next 1/5] r8152: use generic USB macros to define product table
  2020-11-03 19:22 ` [PATCH net-next 1/5] r8152: use generic USB macros to define product table Marek Behún
@ 2020-11-04  1:57   ` Hayes Wang
  2020-11-04  6:02     ` Marek Behún
  2020-11-04  8:53     ` Greg KH
  0 siblings, 2 replies; 26+ messages in thread
From: Hayes Wang @ 2020-11-04  1:57 UTC (permalink / raw)
  To: Marek Behún, netdev; +Cc: linux-usb

Marek Behún <kabel@kernel.org>
> Sent: Wednesday, November 4, 2020 3:22 AM
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index b1770489aca5..85dda591c838 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -6862,20 +6862,12 @@ static void rtl8152_disconnect(struct
> usb_interface *intf)
>  }
> 
>  #define REALTEK_USB_DEVICE(vend, prod)	\
> -	.match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
> -		       USB_DEVICE_ID_MATCH_INT_CLASS, \
> -	.idVendor = (vend), \
> -	.idProduct = (prod), \
> -	.bInterfaceClass = USB_CLASS_VENDOR_SPEC \
> +	USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC)
> \
>  }, \
>  { \
> -	.match_flags = USB_DEVICE_ID_MATCH_INT_INFO | \
> -		       USB_DEVICE_ID_MATCH_DEVICE, \
> -	.idVendor = (vend), \
> -	.idProduct = (prod), \
> -	.bInterfaceClass = USB_CLASS_COMM, \
> -	.bInterfaceSubClass = USB_CDC_SUBCLASS_ETHERNET, \
> -	.bInterfaceProtocol = USB_CDC_PROTO_NONE
> +	USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_COMM, \
> +				      USB_CDC_SUBCLASS_ETHERNET, \
> +				      USB_CDC_PROTO_NONE)
> 
>  /* table of devices that work with this driver */
>  static const struct usb_device_id rtl8152_table[] = {

I don't use these, because checkpatch.pl would show error.

	$ scripts/checkpatch.pl --file --terse drivers/net/usb/r8152.c
	ERROR: Macros with complex values should be enclosed in parentheses

Best Regards,
Hayes


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-03 21:47   ` Vladimir Oltean
@ 2020-11-04  5:55     ` Marek Behún
  2020-11-04  8:47       ` Vladimir Oltean
  0 siblings, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-04  5:55 UTC (permalink / raw)
  To: Vladimir Oltean; +Cc: netdev, linux-usb, Hayes Wang

On Tue, 3 Nov 2020 23:47:12 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Tue, Nov 03, 2020 at 08:22:24PM +0100, Marek Behún wrote:
> > Add pla_ and usb_ prefixed versions of ocp_read_* and ocp_write_*
> > functions. This saves us from always writing MCU_TYPE_PLA/MCU_TYPE_USB
> > as parameter.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---  
> 
> You just made it harder for everyone to follow the code through pattern
> matching. Token concatenation should be banned from the C preprocessor.

So you aren't complaining about the definition of pla_ and usb_
functions, just that they are defined via macros?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 1/5] r8152: use generic USB macros to define product table
  2020-11-04  1:57   ` Hayes Wang
@ 2020-11-04  6:02     ` Marek Behún
  2020-11-04  7:14       ` Hayes Wang
  2020-11-04  8:53     ` Greg KH
  1 sibling, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-04  6:02 UTC (permalink / raw)
  To: Hayes Wang; +Cc: netdev, linux-usb

On Wed, 4 Nov 2020 01:57:10 +0000
Hayes Wang <hayeswang@realtek.com> wrote:

> Marek Behún <kabel@kernel.org>
> > Sent: Wednesday, November 4, 2020 3:22 AM
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index b1770489aca5..85dda591c838 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -6862,20 +6862,12 @@ static void rtl8152_disconnect(struct
> > usb_interface *intf)
> >  }
> > 
> >  #define REALTEK_USB_DEVICE(vend, prod)	\
> > -	.match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
> > -		       USB_DEVICE_ID_MATCH_INT_CLASS, \
> > -	.idVendor = (vend), \
> > -	.idProduct = (prod), \
> > -	.bInterfaceClass = USB_CLASS_VENDOR_SPEC \
> > +	USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC)
> > \
> >  }, \
> >  { \
> > -	.match_flags = USB_DEVICE_ID_MATCH_INT_INFO | \
> > -		       USB_DEVICE_ID_MATCH_DEVICE, \
> > -	.idVendor = (vend), \
> > -	.idProduct = (prod), \
> > -	.bInterfaceClass = USB_CLASS_COMM, \
> > -	.bInterfaceSubClass = USB_CDC_SUBCLASS_ETHERNET, \
> > -	.bInterfaceProtocol = USB_CDC_PROTO_NONE
> > +	USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_COMM, \
> > +				      USB_CDC_SUBCLASS_ETHERNET, \
> > +				      USB_CDC_PROTO_NONE)
> > 
> >  /* table of devices that work with this driver */
> >  static const struct usb_device_id rtl8152_table[] = {  
> 
> I don't use these, because checkpatch.pl would show error.
> 
> 	$ scripts/checkpatch.pl --file --terse drivers/net/usb/r8152.c
> 	ERROR: Macros with complex values should be enclosed in parentheses
> 
> Best Regards,
> Hayes
> 

Hmm, checkpatch did not emit no warnings for me on these patches. Just
two CHECKs for the third patch.

BTW Hayes, is it possible for me gaining access to Realtek
documentation for these chips under NDA? For example via my employer,
CZ.NIC? I can't find any such information on Realtek website.

Also I could not download the driver from Realtek's website, I had to
find it on github. When clicking the download button on [1], it says:
  Warning
  The form #10 does not exist or it is not published.

BTW2 I am interested whether we can make the internal PHY visible to
the Linux PHY subsystem.

Marek

[1]
https://www.realtek.com/en/component/zoo/category/network-interface-controllers-10-100-1000m-gigabit-ethernet-usb-3-0-software

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH net-next 1/5] r8152: use generic USB macros to define product table
  2020-11-04  6:02     ` Marek Behún
@ 2020-11-04  7:14       ` Hayes Wang
  0 siblings, 0 replies; 26+ messages in thread
From: Hayes Wang @ 2020-11-04  7:14 UTC (permalink / raw)
  To: Marek Behún; +Cc: netdev, linux-usb, nic_swsd

Marek Behún <kabel@kernel.org>
> Sent: Wednesday, November 4, 2020 2:03 PM
[...] 
> BTW Hayes, is it possible for me gaining access to Realtek
> documentation for these chips under NDA? For example via my employer,
> CZ.NIC? I can't find any such information on Realtek website.

I have to ask my boss.
Maybe I reply you in private when I get the answer.

> Also I could not download the driver from Realtek's website, I had to
> find it on github. When clicking the download button on [1], it says:
>   Warning
>   The form #10 does not exist or it is not published.

I try to download the driver from our website.
And it seem to work fine.
I will send it to you later.

> BTW2 I am interested whether we can make the internal PHY visible to
> the Linux PHY subsystem.

I think it is possible.
I am not familiar with the Linux PHY subsystem, so I have no idea about
how to start.

Best Regards,
Hayes




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-04  5:55     ` Marek Behún
@ 2020-11-04  8:47       ` Vladimir Oltean
  2020-11-04 10:25         ` Marek Behún
  0 siblings, 1 reply; 26+ messages in thread
From: Vladimir Oltean @ 2020-11-04  8:47 UTC (permalink / raw)
  To: Marek Behún; +Cc: netdev, linux-usb, Hayes Wang

On Wed, Nov 04, 2020 at 06:55:24AM +0100, Marek Behún wrote:
> On Tue, 3 Nov 2020 23:47:12 +0200
> Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > On Tue, Nov 03, 2020 at 08:22:24PM +0100, Marek Behún wrote:
> > > Add pla_ and usb_ prefixed versions of ocp_read_* and ocp_write_*
> > > functions. This saves us from always writing MCU_TYPE_PLA/MCU_TYPE_USB
> > > as parameter.
> > > 
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > ---  
> > 
> > You just made it harder for everyone to follow the code through pattern
> > matching. Token concatenation should be banned from the C preprocessor.
> 
> So you aren't complaining about the definition of pla_ and usb_
> functions, just that they are defined via macros?

Yes.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 1/5] r8152: use generic USB macros to define product table
  2020-11-04  1:57   ` Hayes Wang
  2020-11-04  6:02     ` Marek Behún
@ 2020-11-04  8:53     ` Greg KH
  1 sibling, 0 replies; 26+ messages in thread
From: Greg KH @ 2020-11-04  8:53 UTC (permalink / raw)
  To: Hayes Wang; +Cc: Marek Behún, netdev, linux-usb

On Wed, Nov 04, 2020 at 01:57:10AM +0000, Hayes Wang wrote:
> Marek Behún <kabel@kernel.org>
> > Sent: Wednesday, November 4, 2020 3:22 AM
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index b1770489aca5..85dda591c838 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -6862,20 +6862,12 @@ static void rtl8152_disconnect(struct
> > usb_interface *intf)
> >  }
> > 
> >  #define REALTEK_USB_DEVICE(vend, prod)	\
> > -	.match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
> > -		       USB_DEVICE_ID_MATCH_INT_CLASS, \
> > -	.idVendor = (vend), \
> > -	.idProduct = (prod), \
> > -	.bInterfaceClass = USB_CLASS_VENDOR_SPEC \
> > +	USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC)
> > \
> >  }, \
> >  { \
> > -	.match_flags = USB_DEVICE_ID_MATCH_INT_INFO | \
> > -		       USB_DEVICE_ID_MATCH_DEVICE, \
> > -	.idVendor = (vend), \
> > -	.idProduct = (prod), \
> > -	.bInterfaceClass = USB_CLASS_COMM, \
> > -	.bInterfaceSubClass = USB_CDC_SUBCLASS_ETHERNET, \
> > -	.bInterfaceProtocol = USB_CDC_PROTO_NONE
> > +	USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_COMM, \
> > +				      USB_CDC_SUBCLASS_ETHERNET, \
> > +				      USB_CDC_PROTO_NONE)
> > 
> >  /* table of devices that work with this driver */
> >  static const struct usb_device_id rtl8152_table[] = {
> 
> I don't use these, because checkpatch.pl would show error.
> 
> 	$ scripts/checkpatch.pl --file --terse drivers/net/usb/r8152.c
> 	ERROR: Macros with complex values should be enclosed in parentheses

checkpatch is wrong.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-04  8:47       ` Vladimir Oltean
@ 2020-11-04 10:25         ` Marek Behún
  2020-11-04 10:35           ` Marek Behún
  0 siblings, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-04 10:25 UTC (permalink / raw)
  To: Vladimir Oltean; +Cc: netdev, linux-usb, Hayes Wang

On Wed, 4 Nov 2020 10:47:10 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> > So you aren't complaining about the definition of pla_ and usb_
> > functions, just that they are defined via macros?  
> 
> Yes.

What if concatenation wasn't used, but the functions were still defined
with macro?

DEFINE_READ_FUNC(pla_ocp_read_byte, u8, MCU_TYPE_PLA, ocp_read_byte)
DEFINE_WRITE_FUNC(pla_ocp_write_byte, u8, MCU_TYPE_PLA, ocp_write_byte)

DEFINE_READ_FUNC(pla_ocp_read_word, u16, MCU_TYPE_PLA, ocp_read_word)
DEFINE_WRITE_FUNC(pla_ocp_write_word, u16, MCU_TYPE_PLA, ocp_write_word)

DEFINE_READ_FUNC(pla_ocp_read_dword, u32, MCU_TYPE_PLA, ocp_read_dword)
DEFINE_WRITE_FUNC(pla_ocp_write_dword, u32, MCU_TYPE_PLA, ocp_write_dword)

DEFINE_READ_FUNC(usb_ocp_read_byte, u8, MCU_TYPE_USB, ocp_read_byte)
DEFINE_WRITE_FUNC(usb_ocp_write_byte, u8, MCU_TYPE_USB, ocp_write_byte)

DEFINE_READ_FUNC(usb_ocp_read_word, u16, MCU_TYPE_USB, ocp_read_word)
DEFINE_WRITE_FUNC(usb_ocp_write_word, u16, MCU_TYPE_USB, ocp_write_word)

DEFINE_READ_FUNC(usb_ocp_read_dword, u32, MCU_TYPE_USB, ocp_read_dword)
DEFINE_WRITE_FUNC(usb_ocp_write_dword, u32, MCU_TYPE_USB, ocp_write_dword)

This way there is no concantenation. Or should I abandon macros at all?

Marek


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-04 10:25         ` Marek Behún
@ 2020-11-04 10:35           ` Marek Behún
  2020-11-04 11:00             ` Vladimir Oltean
  0 siblings, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-04 10:35 UTC (permalink / raw)
  To: Vladimir Oltean; +Cc: netdev, linux-usb, Hayes Wang

Or something like this?

#define DEF_R_FUNC(_t, _r, _r_i, _mcu)				\
static inline _t _r(struct r8152 *tp, u16 index)		\
{								\
	return _r_i(tp, _mcu, index);				\
}

#define DEF_W_FUNC(_t, _w, _w_i, _mcu)				\
static inline void _w(struct r8152 *tp, u16 index, _t data)	\
{								\
	_w_i(tp, _mcu, index, data);				\
}

DEF_R_FUNC(u8, pla_ocp_read_byte, ocp_read_byte, MCU_TYPE_PLA)
DEF_W_FUNC(u8, pla_ocp_write_byte, ocp_write_byte, MCU_TYPE_PLA)
DEF_R_FUNC(u16, pla_ocp_read_word, ocp_read_word, MCU_TYPE_PLA)
DEF_W_FUNC(u16, pla_ocp_write_word, ocp_write_word, MCU_TYPE_PLA)
DEF_R_FUNC(u32, pla_ocp_read_dword, ocp_read_dword, MCU_TYPE_PLA)
DEF_W_FUNC(u32, pla_ocp_write_dword, ocp_write_dword, MCU_TYPE_PLA)

DEF_R_FUNC(u8, usb_ocp_read_byte, ocp_read_byte, MCU_TYPE_USB)
DEF_W_FUNC(u8, usb_ocp_write_byte, ocp_write_byte, MCU_TYPE_USB)
DEF_R_FUNC(u16, usb_ocp_read_word, ocp_read_word, MCU_TYPE_USB)
DEF_W_FUNC(u16, usb_ocp_write_word, ocp_write_word, MCU_TYPE_USB)
DEF_R_FUNC(u32, usb_ocp_read_dword, ocp_read_dword, MCU_TYPE_USB)
DEF_W_FUNC(u32, usb_ocp_write_dword, ocp_write_dword, MCU_TYPE_USB)

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-04 10:35           ` Marek Behún
@ 2020-11-04 11:00             ` Vladimir Oltean
  2020-11-04 11:10               ` Marek Behún
  0 siblings, 1 reply; 26+ messages in thread
From: Vladimir Oltean @ 2020-11-04 11:00 UTC (permalink / raw)
  To: Marek Behún; +Cc: netdev, linux-usb, Hayes Wang

On Wed, Nov 04, 2020 at 11:35:45AM +0100, Marek Behún wrote:
> Or something like this?
> 
> #define DEF_R_FUNC(_t, _r, _r_i, _mcu)				\
> static inline _t _r(struct r8152 *tp, u16 index)		\
> {								\
> 	return _r_i(tp, _mcu, index);				\
> }
> 
> #define DEF_W_FUNC(_t, _w, _w_i, _mcu)				\
> static inline void _w(struct r8152 *tp, u16 index, _t data)	\
> {								\
> 	_w_i(tp, _mcu, index, data);				\
> }
> 
> DEF_R_FUNC(u8, pla_ocp_read_byte, ocp_read_byte, MCU_TYPE_PLA)
> DEF_W_FUNC(u8, pla_ocp_write_byte, ocp_write_byte, MCU_TYPE_PLA)
> DEF_R_FUNC(u16, pla_ocp_read_word, ocp_read_word, MCU_TYPE_PLA)
> DEF_W_FUNC(u16, pla_ocp_write_word, ocp_write_word, MCU_TYPE_PLA)
> DEF_R_FUNC(u32, pla_ocp_read_dword, ocp_read_dword, MCU_TYPE_PLA)
> DEF_W_FUNC(u32, pla_ocp_write_dword, ocp_write_dword, MCU_TYPE_PLA)
> 
> DEF_R_FUNC(u8, usb_ocp_read_byte, ocp_read_byte, MCU_TYPE_USB)
> DEF_W_FUNC(u8, usb_ocp_write_byte, ocp_write_byte, MCU_TYPE_USB)
> DEF_R_FUNC(u16, usb_ocp_read_word, ocp_read_word, MCU_TYPE_USB)
> DEF_W_FUNC(u16, usb_ocp_write_word, ocp_write_word, MCU_TYPE_USB)
> DEF_R_FUNC(u32, usb_ocp_read_dword, ocp_read_dword, MCU_TYPE_USB)
> DEF_W_FUNC(u32, usb_ocp_write_dword, ocp_write_dword, MCU_TYPE_USB)

I'm not sure it's worth the change :(
Let's put it another way, your diffstat has 338 insertions and 335
deletions. Aka you're saving 3 lines overall.
With this new approach that doesn't use token concatenation at all,
you're probably not saving anything at all.
Also, I'm not sure that you need to make the functions inline. The
compiler should be smart enough to not generate functions for
usb_ocp_read_byte etc. You can check with
"make drivers/net/usb/r8152.lst".

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-04 11:00             ` Vladimir Oltean
@ 2020-11-04 11:10               ` Marek Behún
  2020-11-04 12:14                 ` Vladimir Oltean
  0 siblings, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-04 11:10 UTC (permalink / raw)
  To: Vladimir Oltean; +Cc: netdev, linux-usb, Hayes Wang

On Wed, 4 Nov 2020 13:00:59 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Wed, Nov 04, 2020 at 11:35:45AM +0100, Marek Behún wrote:
> > Or something like this?
> > 
> > #define DEF_R_FUNC(_t, _r, _r_i, _mcu)				\
> > static inline _t _r(struct r8152 *tp, u16 index)		\
> > {								\
> > 	return _r_i(tp, _mcu, index);				\
> > }
> > 
> > #define DEF_W_FUNC(_t, _w, _w_i, _mcu)				\
> > static inline void _w(struct r8152 *tp, u16 index, _t data)	\
> > {								\
> > 	_w_i(tp, _mcu, index, data);				\
> > }
> > 
> > DEF_R_FUNC(u8, pla_ocp_read_byte, ocp_read_byte, MCU_TYPE_PLA)
> > DEF_W_FUNC(u8, pla_ocp_write_byte, ocp_write_byte, MCU_TYPE_PLA)
> > DEF_R_FUNC(u16, pla_ocp_read_word, ocp_read_word, MCU_TYPE_PLA)
> > DEF_W_FUNC(u16, pla_ocp_write_word, ocp_write_word, MCU_TYPE_PLA)
> > DEF_R_FUNC(u32, pla_ocp_read_dword, ocp_read_dword, MCU_TYPE_PLA)
> > DEF_W_FUNC(u32, pla_ocp_write_dword, ocp_write_dword, MCU_TYPE_PLA)
> > 
> > DEF_R_FUNC(u8, usb_ocp_read_byte, ocp_read_byte, MCU_TYPE_USB)
> > DEF_W_FUNC(u8, usb_ocp_write_byte, ocp_write_byte, MCU_TYPE_USB)
> > DEF_R_FUNC(u16, usb_ocp_read_word, ocp_read_word, MCU_TYPE_USB)
> > DEF_W_FUNC(u16, usb_ocp_write_word, ocp_write_word, MCU_TYPE_USB)
> > DEF_R_FUNC(u32, usb_ocp_read_dword, ocp_read_dword, MCU_TYPE_USB)
> > DEF_W_FUNC(u32, usb_ocp_write_dword, ocp_write_dword, MCU_TYPE_USB)  
> 
> I'm not sure it's worth the change :(
> Let's put it another way, your diffstat has 338 insertions and 335
> deletions. Aka you're saving 3 lines overall.
> With this new approach that doesn't use token concatenation at all,
> you're probably not saving anything at all.
> Also, I'm not sure that you need to make the functions inline. The
> compiler should be smart enough to not generate functions for
> usb_ocp_read_byte etc. You can check with
> "make drivers/net/usb/r8152.lst".

Vladimir, the purpose of this patch isn't to save lines, but to save us
from always writing MCU_TYPE_USB / MCU_TYPE_PLA.
It just transforms forms of
  ocp_read_word(tp, MCU_TYPE_USB, idx);
  ocp_write_dword(tp, MCU_TYPE_PLA, idx, val);
into
  usb_ocp_read_word(tp, idx);
  pla_ocp_write_dword(tp, idx, val);

The fifth patch of this series saves lines by adding _modify functions,
to transform
  val = *_read(idx);
  val &= ~clr;
  val |= set;
  *_write(idx, val);
into
  *_modify(idx, clr, set);


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-04 11:10               ` Marek Behún
@ 2020-11-04 12:14                 ` Vladimir Oltean
  2020-11-04 21:07                   ` Jakub Kicinski
  2020-11-05  9:54                   ` Marek Behún
  0 siblings, 2 replies; 26+ messages in thread
From: Vladimir Oltean @ 2020-11-04 12:14 UTC (permalink / raw)
  To: Marek Behún; +Cc: netdev, linux-usb, Hayes Wang

On Wed, Nov 04, 2020 at 12:10:53PM +0100, Marek Behún wrote:
> > I'm not sure it's worth the change :(
> > Let's put it another way, your diffstat has 338 insertions and 335
> > deletions. Aka you're saving 3 lines overall.
> > With this new approach that doesn't use token concatenation at all,
> > you're probably not saving anything at all.
> > Also, I'm not sure that you need to make the functions inline. The
> > compiler should be smart enough to not generate functions for
> > usb_ocp_read_byte etc. You can check with
> > "make drivers/net/usb/r8152.lst".
> 
> Vladimir, the purpose of this patch isn't to save lines, but to save us
> from always writing MCU_TYPE_USB / MCU_TYPE_PLA.
> It just transforms forms of
>   ocp_read_word(tp, MCU_TYPE_USB, idx);
>   ocp_write_dword(tp, MCU_TYPE_PLA, idx, val);
> into
>   usb_ocp_read_word(tp, idx);
>   pla_ocp_write_dword(tp, idx, val);
> 
> The fifth patch of this series saves lines by adding _modify functions,
> to transform
>   val = *_read(idx);
>   val &= ~clr;
>   val |= set;
>   *_write(idx, val);
> into
>   *_modify(idx, clr, set);
> 

So if the point isn't to save lines, then why don't you go for something
trivial?

static void ocp_modify_byte(struct r8152 *tp, u16 type, u16 index, u8 clr,
			    u8 set)
{
	u8 val = ocp_read_byte(tp, type, index);

	ocp_write_byte(tp, type, index, (val & ~clr) | set);
}

static void ocp_modify_word(struct r8152 *tp, u16 type, u16 index, u16 clr,
			    u16 set)
{
	u16 val = ocp_read_word(tp, type, index);

	ocp_write_word(tp, type, index, (val & ~clr) | set);
}

static void ocp_modify_dword(struct r8152 *tp, u16 type, u16 index, u32 clr,
			     u32 set)
{
	u32 val = ocp_read_dword(tp, type, index);

	ocp_write_dword(tp, type, index, (val & ~clr) | set);
}

#define pla_ocp_read_byte(tp, index)				\
	ocp_read_byte(tp, MCU_TYPE_PLA, index)
#define pla_ocp_write_byte(tp, index, data)			\
	ocp_write_byte(tp, MCU_TYPE_PLA, index, data)
#define pla_ocp_modify_byte(tp, index, clr, set)		\
	ocp_modify_byte(tp, MCU_TYPE_PLA, index, clr, set)
#define pla_ocp_read_word(tp, index)				\
	ocp_read_word(tp, MCU_TYPE_PLA, index)
#define pla_ocp_write_word(tp, index, data)			\
	ocp_write_word(tp, MCU_TYPE_PLA, index, data)
#define pla_ocp_modify_word(tp, index, clr, set)		\
	ocp_modify_word(tp, MCU_TYPE_PLA, index, clr, set)
#define pla_ocp_read_dword(tp, index)				\
	ocp_read_dword(tp, MCU_TYPE_PLA, index)
#define pla_ocp_write_dword(tp, index, data)			\
	ocp_write_dword(tp, MCU_TYPE_PLA, index, data)
#define pla_ocp_modify_dword(tp, index, clr, set)		\
	ocp_modify_dword(tp, MCU_TYPE_PLA, index, clr, set)

#define usb_ocp_read_byte(tp, index)				\
	ocp_read_byte(tp, MCU_TYPE_USB, index)
#define usb_ocp_write_byte(tp, index, data)			\
	ocp_write_byte(tp, MCU_TYPE_USB, index, data)
#define usb_ocp_modify_byte(tp, index, clr, set)		\
	ocp_modify_byte(tp, MCU_TYPE_USB, index, clr, set)
#define usb_ocp_read_word(tp, index)				\
	ocp_read_word(tp, MCU_TYPE_USB, index)
#define usb_ocp_write_word(tp, index, data)			\
	ocp_write_word(tp, MCU_TYPE_USB, index, data)
#define usb_ocp_modify_word(tp, index, clr, set)		\
	ocp_modify_word(tp, MCU_TYPE_USB, index, clr, set)
#define usb_ocp_read_dword(tp, index)				\
	ocp_read_dword(tp, MCU_TYPE_USB, index)
#define usb_ocp_write_dword(tp, index, data)			\
	ocp_write_dword(tp, MCU_TYPE_USB, index, data)
#define usb_ocp_modify_dword(tp, index, clr, set)		\
	ocp_modify_dword(tp, MCU_TYPE_USB, index, clr, set)

To my eyes this is easier to digest.

That is, unless you want to go for function pointers and have separate
structures for PLA and USB...

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-04 12:14                 ` Vladimir Oltean
@ 2020-11-04 21:07                   ` Jakub Kicinski
  2020-11-05  9:54                   ` Marek Behún
  1 sibling, 0 replies; 26+ messages in thread
From: Jakub Kicinski @ 2020-11-04 21:07 UTC (permalink / raw)
  To: Vladimir Oltean; +Cc: Marek Behún, netdev, linux-usb, Hayes Wang

On Wed, 4 Nov 2020 14:14:24 +0200 Vladimir Oltean wrote:
> To my eyes this is easier to digest.

+1

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-04 12:14                 ` Vladimir Oltean
  2020-11-04 21:07                   ` Jakub Kicinski
@ 2020-11-05  9:54                   ` Marek Behún
  2020-11-05 10:56                     ` Vladimir Oltean
  1 sibling, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-05  9:54 UTC (permalink / raw)
  To: Vladimir Oltean; +Cc: netdev, linux-usb, Hayes Wang

On Wed, 4 Nov 2020 14:14:24 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Wed, Nov 04, 2020 at 12:10:53PM +0100, Marek Behún wrote:
> > > I'm not sure it's worth the change :(
> > > Let's put it another way, your diffstat has 338 insertions and 335
> > > deletions. Aka you're saving 3 lines overall.
> > > With this new approach that doesn't use token concatenation at all,
> > > you're probably not saving anything at all.
> > > Also, I'm not sure that you need to make the functions inline. The
> > > compiler should be smart enough to not generate functions for
> > > usb_ocp_read_byte etc. You can check with
> > > "make drivers/net/usb/r8152.lst".  
> > 
> > Vladimir, the purpose of this patch isn't to save lines, but to save us
> > from always writing MCU_TYPE_USB / MCU_TYPE_PLA.
> > It just transforms forms of
> >   ocp_read_word(tp, MCU_TYPE_USB, idx);
> >   ocp_write_dword(tp, MCU_TYPE_PLA, idx, val);
> > into
> >   usb_ocp_read_word(tp, idx);
> >   pla_ocp_write_dword(tp, idx, val);
> > 
> > The fifth patch of this series saves lines by adding _modify functions,
> > to transform
> >   val = *_read(idx);
> >   val &= ~clr;
> >   val |= set;
> >   *_write(idx, val);
> > into
> >   *_modify(idx, clr, set);
> >   
> 
> So if the point isn't to save lines, then why don't you go for something
> trivial?
> 
> static void ocp_modify_byte(struct r8152 *tp, u16 type, u16 index, u8 clr,
> 			    u8 set)
> {
> 	u8 val = ocp_read_byte(tp, type, index);
> 
> 	ocp_write_byte(tp, type, index, (val & ~clr) | set);
> }
> 
> static void ocp_modify_word(struct r8152 *tp, u16 type, u16 index, u16 clr,
> 			    u16 set)
> {
> 	u16 val = ocp_read_word(tp, type, index);
> 
> 	ocp_write_word(tp, type, index, (val & ~clr) | set);
> }
> 
> static void ocp_modify_dword(struct r8152 *tp, u16 type, u16 index, u32 clr,
> 			     u32 set)
> {
> 	u32 val = ocp_read_dword(tp, type, index);
> 
> 	ocp_write_dword(tp, type, index, (val & ~clr) | set);
> }
> 
> #define pla_ocp_read_byte(tp, index)				\
> 	ocp_read_byte(tp, MCU_TYPE_PLA, index)
> #define pla_ocp_write_byte(tp, index, data)			\
> 	ocp_write_byte(tp, MCU_TYPE_PLA, index, data)
> #define pla_ocp_modify_byte(tp, index, clr, set)		\
> 	ocp_modify_byte(tp, MCU_TYPE_PLA, index, clr, set)
> #define pla_ocp_read_word(tp, index)				\
> 	ocp_read_word(tp, MCU_TYPE_PLA, index)
> #define pla_ocp_write_word(tp, index, data)			\
> 	ocp_write_word(tp, MCU_TYPE_PLA, index, data)
> #define pla_ocp_modify_word(tp, index, clr, set)		\
> 	ocp_modify_word(tp, MCU_TYPE_PLA, index, clr, set)
> #define pla_ocp_read_dword(tp, index)				\
> 	ocp_read_dword(tp, MCU_TYPE_PLA, index)
> #define pla_ocp_write_dword(tp, index, data)			\
> 	ocp_write_dword(tp, MCU_TYPE_PLA, index, data)
> #define pla_ocp_modify_dword(tp, index, clr, set)		\
> 	ocp_modify_dword(tp, MCU_TYPE_PLA, index, clr, set)
> 
> #define usb_ocp_read_byte(tp, index)				\
> 	ocp_read_byte(tp, MCU_TYPE_USB, index)
> #define usb_ocp_write_byte(tp, index, data)			\
> 	ocp_write_byte(tp, MCU_TYPE_USB, index, data)
> #define usb_ocp_modify_byte(tp, index, clr, set)		\
> 	ocp_modify_byte(tp, MCU_TYPE_USB, index, clr, set)
> #define usb_ocp_read_word(tp, index)				\
> 	ocp_read_word(tp, MCU_TYPE_USB, index)
> #define usb_ocp_write_word(tp, index, data)			\
> 	ocp_write_word(tp, MCU_TYPE_USB, index, data)
> #define usb_ocp_modify_word(tp, index, clr, set)		\
> 	ocp_modify_word(tp, MCU_TYPE_USB, index, clr, set)
> #define usb_ocp_read_dword(tp, index)				\
> 	ocp_read_dword(tp, MCU_TYPE_USB, index)
> #define usb_ocp_write_dword(tp, index, data)			\
> 	ocp_write_dword(tp, MCU_TYPE_USB, index, data)
> #define usb_ocp_modify_dword(tp, index, clr, set)		\
> 	ocp_modify_dword(tp, MCU_TYPE_USB, index, clr, set)
> 
> To my eyes this is easier to digest.
> 
> That is, unless you want to go for function pointers and have separate
> structures for PLA and USB...

I thought that static inline functions are preferred to macros, since
compiler warns better if they are used incorrectly...

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-05  9:54                   ` Marek Behún
@ 2020-11-05 10:56                     ` Vladimir Oltean
  2020-11-05 11:30                       ` Marek Behún
  2020-11-06  3:01                       ` Hayes Wang
  0 siblings, 2 replies; 26+ messages in thread
From: Vladimir Oltean @ 2020-11-05 10:56 UTC (permalink / raw)
  To: Marek Behún; +Cc: netdev, linux-usb, Hayes Wang

On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote:
> I thought that static inline functions are preferred to macros, since
> compiler warns better if they are used incorrectly...

Citation needed. Also, how do static inline functions wrapped in macros
(i.e. your patch) stack up against your claim about better warnings?
I guess ease of maintainership should prevail here, and Hayes should
have the final word. I don't really have any stake here.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-05 10:56                     ` Vladimir Oltean
@ 2020-11-05 11:30                       ` Marek Behún
  2020-11-05 12:06                         ` Vladimir Oltean
  2020-11-06  3:01                       ` Hayes Wang
  1 sibling, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-05 11:30 UTC (permalink / raw)
  To: Vladimir Oltean; +Cc: netdev, linux-usb, Hayes Wang

On Thu, 5 Nov 2020 12:56:42 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote:
> > I thought that static inline functions are preferred to macros, since
> > compiler warns better if they are used incorrectly...  
> 
> Citation needed.

Just search for substring "instead of macro" in git log, there are
multiple such changes that were accepted since it provides better
typechecking. I am not saying it is documented anywhere, just that I
thought it was preffered.

> Also, how do static inline functions wrapped in macros
> (i.e. your patch) stack up against your claim about better warnings?

If they are defined as functions (they don't have to be inline,
of course) instead of macros and they are used incorrectly, the compiler
provides more readable warnings. (Yes, in current versions of gcc it is
much better than in the past, but still there are more lines of
warnings printed: "in expansion of macro"...).


> I guess ease of maintainership should prevail here, and Hayes should
> have the final word. I don't really have any stake here.

Vladimir, your arguments are valid and I accept the reasoning.
I too can see that the resulting code is a little awkward.

Marek



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-05 11:30                       ` Marek Behún
@ 2020-11-05 12:06                         ` Vladimir Oltean
  0 siblings, 0 replies; 26+ messages in thread
From: Vladimir Oltean @ 2020-11-05 12:06 UTC (permalink / raw)
  To: Marek Behún; +Cc: netdev, linux-usb, Hayes Wang

On Thu, Nov 05, 2020 at 12:30:43PM +0100, Marek Behún wrote:
> On Thu, 5 Nov 2020 12:56:42 +0200
> Vladimir Oltean <olteanv@gmail.com> wrote:
>
> > On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote:
> > > I thought that static inline functions are preferred to macros, since
> > > compiler warns better if they are used incorrectly...
> >
> > Citation needed.
>
> Just search for substring "instead of macro" in git log, there are
> multiple such changes that were accepted since it provides better
> typechecking. I am not saying it is documented anywhere, just that I
> thought it was preffered.
>
> > Also, how do static inline functions wrapped in macros
> > (i.e. your patch) stack up against your claim about better warnings?
>
> If they are defined as functions (they don't have to be inline,
> of course) instead of macros and they are used incorrectly, the compiler
> provides more readable warnings. (Yes, in current versions of gcc it is
> much better than in the past, but still there are more lines of
> warnings printed: "in expansion of macro"...).

Ok, but I mean, we're not even in contradiction at this point? I only
provided you macro definitions of pla_ocp_* and usb_ocp_* to prove that
they can be defined in a cleaner way than your attempt. If you still
think it's worth having the pla_ocp_* and usb_ocp_* helpers defined as
separate functions just to avoid passing the extra MCU_TYPE_* argument,
then go ahead.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-05 10:56                     ` Vladimir Oltean
  2020-11-05 11:30                       ` Marek Behún
@ 2020-11-06  3:01                       ` Hayes Wang
  2020-11-06  6:39                         ` Marek Behún
  1 sibling, 1 reply; 26+ messages in thread
From: Hayes Wang @ 2020-11-06  3:01 UTC (permalink / raw)
  To: Vladimir Oltean, Marek Behún; +Cc: netdev, linux-usb

Vladimir Oltean <olteanv@gmail.com>
> Sent: Thursday, November 5, 2020 6:57 PM
> On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote:
> > I thought that static inline functions are preferred to macros, since
> > compiler warns better if they are used incorrectly...
> 
> Citation needed. Also, how do static inline functions wrapped in macros
> (i.e. your patch) stack up against your claim about better warnings?
> I guess ease of maintainership should prevail here, and Hayes should
> have the final word. I don't really have any stake here.

I agree with Vladimir Oltean.

I prefer to the way of easy maintaining.
I don't understand the advantage which you discuss.
However, if I am not familiar with the code, this patch
would let me take more time to find out the declarations
of these functions. This make it harder to trace the code.

Best Regards,
Hayes


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-06  3:01                       ` Hayes Wang
@ 2020-11-06  6:39                         ` Marek Behún
  2020-11-06  7:39                           ` Hayes Wang
  0 siblings, 1 reply; 26+ messages in thread
From: Marek Behún @ 2020-11-06  6:39 UTC (permalink / raw)
  To: Hayes Wang; +Cc: Vladimir Oltean, netdev, linux-usb

On Fri, 6 Nov 2020 03:01:22 +0000
Hayes Wang <hayeswang@realtek.com> wrote:

> Vladimir Oltean <olteanv@gmail.com>
> > Sent: Thursday, November 5, 2020 6:57 PM
> > On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote:  
> > > I thought that static inline functions are preferred to macros, since
> > > compiler warns better if they are used incorrectly...  
> > 
> > Citation needed. Also, how do static inline functions wrapped in macros
> > (i.e. your patch) stack up against your claim about better warnings?
> > I guess ease of maintainership should prevail here, and Hayes should
> > have the final word. I don't really have any stake here.  
> 
> I agree with Vladimir Oltean.
> 
> I prefer to the way of easy maintaining.
> I don't understand the advantage which you discuss.
> However, if I am not familiar with the code, this patch
> would let me take more time to find out the declarations
> of these functions. This make it harder to trace the code.

Hi Hayes,

just to be clear:
Are you against defining these functions via macros?
If so, I can simply rewrite this so that it does not use macros...

Or are you against implementing these functions themselves? Should I
abandon this at all?

BTW, what about patch 5/5 which introduces *_modify helpers?
Patch 5/5 simplifies the driver a lot, IMO, changing this

  ocp_data = usb_ocp_read_word(tp, USB_PM_CTRL_STATUS);
  ocp_data &= ~RESUME_INDICATE;
  usb_ocp_write_word(tp, USB_PM_CTRL_STATUS, ocp_data);

into this

  usb_ocp_modify_word(tp, USB_PM_CTRL_STATUS, RESUME_INDICATE, 0);

Marek


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH net-next 3/5] r8152: add MCU typed read/write functions
  2020-11-06  6:39                         ` Marek Behún
@ 2020-11-06  7:39                           ` Hayes Wang
  0 siblings, 0 replies; 26+ messages in thread
From: Hayes Wang @ 2020-11-06  7:39 UTC (permalink / raw)
  To: Marek Behún; +Cc: Vladimir Oltean, netdev, linux-usb

Marek Behún <kabel@kernel.org>
> Sent: Friday, November 6, 2020 2:40 PM
[...]
> Hi Hayes,
> 
> just to be clear:
> Are you against defining these functions via macros?
> If so, I can simply rewrite this so that it does not use macros...

I would like the way which let me find the source of the
function easily. I don't like that I couldn't find where these
functions are defined, when I search whole the source code.

For example, for the method which Vladimir Oltean provides,
when I search the keyword "pla_ocp_read_byte", I could easily
find out that the source function is "ocp_read_byte".
However, for your patch, I could only find where the function
is used. I think it is not friendly for the newbie who is not
familiar with this driver.

However, I don't think I am the decision maker. This is
just my view.

> Or are you against implementing these functions themselves?

No.

> 
> BTW, what about patch 5/5 which introduces *_modify helpers?

It is fine.

Best Regards,
Hayes



^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2020-11-06  7:40 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 19:22 [PATCH net-next 0/5] r8152 changes Marek Behún
2020-11-03 19:22 ` [PATCH net-next 1/5] r8152: use generic USB macros to define product table Marek Behún
2020-11-04  1:57   ` Hayes Wang
2020-11-04  6:02     ` Marek Behún
2020-11-04  7:14       ` Hayes Wang
2020-11-04  8:53     ` Greg KH
2020-11-03 19:22 ` [PATCH net-next 2/5] r8152: cosmetic improvement of product table macro Marek Behún
2020-11-03 19:22 ` [PATCH net-next 3/5] r8152: add MCU typed read/write functions Marek Behún
2020-11-03 21:47   ` Vladimir Oltean
2020-11-04  5:55     ` Marek Behún
2020-11-04  8:47       ` Vladimir Oltean
2020-11-04 10:25         ` Marek Behún
2020-11-04 10:35           ` Marek Behún
2020-11-04 11:00             ` Vladimir Oltean
2020-11-04 11:10               ` Marek Behún
2020-11-04 12:14                 ` Vladimir Oltean
2020-11-04 21:07                   ` Jakub Kicinski
2020-11-05  9:54                   ` Marek Behún
2020-11-05 10:56                     ` Vladimir Oltean
2020-11-05 11:30                       ` Marek Behún
2020-11-05 12:06                         ` Vladimir Oltean
2020-11-06  3:01                       ` Hayes Wang
2020-11-06  6:39                         ` Marek Behún
2020-11-06  7:39                           ` Hayes Wang
2020-11-03 19:22 ` [PATCH net-next 4/5] r8152: rename r8153_phy_status to r8153_phy_status_wait Marek Behún
2020-11-03 19:22 ` [PATCH net-next 5/5] r8152: use *_modify helpers instead of read/write combos Marek Behún

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).