diff mbox series

net/usb/r8153_ecm: support ECM mode for RTL8153

Message ID 1394712342-15778-387-Taiwan-albertk@realtek.com
State New
Headers show
Series net/usb/r8153_ecm: support ECM mode for RTL8153 | expand

Commit Message

Hayes Wang Oct. 29, 2020, 12:33 p.m. UTC
Support ECM mode based on cdc_ether with relative mii functions,
when CONFIG_USB_RTL8152 is not set, or the device is not supported
by r8152 driver.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/Makefile    |   2 +-
 drivers/net/usb/r8152.c     |   5 +-
 drivers/net/usb/r8153_ecm.c | 174 ++++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+), 3 deletions(-)
 create mode 100644 drivers/net/usb/r8153_ecm.c

Comments

kernel test robot Oct. 29, 2020, 3:04 p.m. UTC | #1
Hi Hayes,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on net-next/master ipvs/master linus/master v5.10-rc1 next-20201029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hayes-Wang/net-usb-r8153_ecm-support-ECM-mode-for-RTL8153/20201029-203440
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git d6535dca28859d8d9ef80894eb287b2ac35a32e8
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e34498795de95fbccb0f2feee72cd8df723f9fd3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Hayes-Wang/net-usb-r8153_ecm-support-ECM-mode-for-RTL8153/20201029-203440
        git checkout e34498795de95fbccb0f2feee72cd8df723f9fd3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/net/usb/r8152.c:6618:4: warning: no previous prototype for 'rtl8152_get_version' [-Wmissing-prototypes]
    6618 | u8 rtl8152_get_version(struct usb_interface *intf)
         |    ^~~~~~~~~~~~~~~~~~~

vim +/rtl8152_get_version +6618 drivers/net/usb/r8152.c

  6617	
> 6618	u8 rtl8152_get_version(struct usb_interface *intf)
  6619	{
  6620		struct usb_device *udev = interface_to_usbdev(intf);
  6621		u32 ocp_data = 0;
  6622		__le32 *tmp;
  6623		u8 version;
  6624		int ret;
  6625	
  6626		tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
  6627		if (!tmp)
  6628			return 0;
  6629	
  6630		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
  6631				      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
  6632				      PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);
  6633		if (ret > 0)
  6634			ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
  6635	
  6636		kfree(tmp);
  6637	
  6638		switch (ocp_data) {
  6639		case 0x4c00:
  6640			version = RTL_VER_01;
  6641			break;
  6642		case 0x4c10:
  6643			version = RTL_VER_02;
  6644			break;
  6645		case 0x5c00:
  6646			version = RTL_VER_03;
  6647			break;
  6648		case 0x5c10:
  6649			version = RTL_VER_04;
  6650			break;
  6651		case 0x5c20:
  6652			version = RTL_VER_05;
  6653			break;
  6654		case 0x5c30:
  6655			version = RTL_VER_06;
  6656			break;
  6657		case 0x4800:
  6658			version = RTL_VER_07;
  6659			break;
  6660		case 0x6000:
  6661			version = RTL_VER_08;
  6662			break;
  6663		case 0x6010:
  6664			version = RTL_VER_09;
  6665			break;
  6666		default:
  6667			version = RTL_VER_UNKNOWN;
  6668			dev_info(&intf->dev, "Unknown version 0x%04x\n", ocp_data);
  6669			break;
  6670		}
  6671	
  6672		dev_dbg(&intf->dev, "Detected version 0x%04x\n", version);
  6673	
  6674		return version;
  6675	}
  6676	EXPORT_SYMBOL_GPL(rtl8152_get_version);
  6677	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Greg Kroah-Hartman Nov. 3, 2020, 9:32 a.m. UTC | #2
On Mon, Nov 02, 2020 at 11:47:18AM -0800, Jakub Kicinski wrote:
> On Mon, 2 Nov 2020 07:20:15 +0000 Hayes Wang wrote:
> > Jakub Kicinski <kuba@kernel.org>
> > > Can you describe the use case in more detail?
> > > 
> > > AFAICT r8152 defines a match for the exact same device.
> > > Does it not mean that which driver is used will be somewhat random
> > > if both are built?  
> > 
> > I export rtl_get_version() from r8152. It would return none zero
> > value if r8152 could support this device. Both r8152 and r8153_ecm
> > would check the return value of rtl_get_version() in porbe().
> > Therefore, if rtl_get_version() return none zero value, the r8152
> > is used for the device with vendor mode. Otherwise, the r8153_ecm
> > is used for the device with ECM mode.
> 
> Oh, I see, I missed that the rtl_get_version() checking is the inverse
> of r8152.
> 
> > > > +/* Define these values to match your device */
> > > > +#define VENDOR_ID_REALTEK		0x0bda
> > > > +#define VENDOR_ID_MICROSOFT		0x045e
> > > > +#define VENDOR_ID_SAMSUNG		0x04e8
> > > > +#define VENDOR_ID_LENOVO		0x17ef
> > > > +#define VENDOR_ID_LINKSYS		0x13b1
> > > > +#define VENDOR_ID_NVIDIA		0x0955
> > > > +#define VENDOR_ID_TPLINK		0x2357  
> > > 
> > > $ git grep 0x2357 | grep -i tplink
> > > drivers/net/usb/cdc_ether.c:#define TPLINK_VENDOR_ID	0x2357
> > > drivers/net/usb/r8152.c:#define VENDOR_ID_TPLINK		0x2357
> > > drivers/usb/serial/option.c:#define TPLINK_VENDOR_ID			0x2357
> > > 
> > > $ git grep 0x17ef | grep -i lenovo
> > > drivers/hid/hid-ids.h:#define USB_VENDOR_ID_LENOVO		0x17ef
> > > drivers/hid/wacom.h:#define USB_VENDOR_ID_LENOVO	0x17ef
> > > drivers/net/usb/cdc_ether.c:#define LENOVO_VENDOR_ID	0x17ef
> > > drivers/net/usb/r8152.c:#define VENDOR_ID_LENOVO		0x17ef
> > > 
> > > Time to consolidate those vendor id defines perhaps?  
> > 
> > It seems that there is no such header file which I could include
> > or add the new vendor IDs.
> 
> Please create one. (Adding Greg KH to the recipients, in case there is
> a reason that USB subsystem doesn't have a common vendor id header.)

There is a reason, it's a nightmare to maintain and handle merges for,
just don't do it.

Read the comments at the top of the pci_ids.h file if you are curious
why we don't even do this for PCI device ids anymore for the past 10+
years.

So no, please do not create such a common file, it is not needed or a
good idea.

thanks,

greg k-h
Hayes Wang Nov. 3, 2020, 9:46 a.m. UTC | #3
v3:
Move original patch to #2. And add a new patch #1 to consolidate vendor ID
of USB devices.

v2:
Add include/linux/usb/r8152.h to avoid the warning about
no previous prototype for rtl8152_get_version.

Hayes Wang (2):
  include/linux/usb: new header file for the vendor ID of USB devices
  net/usb/r8153_ecm: support ECM mode for RTL8153

 drivers/net/usb/Makefile          |   2 +-
 drivers/net/usb/cdc_ether.c       |  93 +++++++----------
 drivers/net/usb/r8152.c           |  68 +++++--------
 drivers/net/usb/r8153_ecm.c       | 162 ++++++++++++++++++++++++++++++
 include/linux/usb/r8152.h         |  30 ++++++
 include/linux/usb/usb_vendor_id.h |  51 ++++++++++
 6 files changed, 306 insertions(+), 100 deletions(-)
 create mode 100644 drivers/net/usb/r8153_ecm.c
 create mode 100644 include/linux/usb/r8152.h
 create mode 100644 include/linux/usb/usb_vendor_id.h
Hayes Wang Nov. 3, 2020, 9:51 a.m. UTC | #4
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Tuesday, November 3, 2020 5:33 PM

[...]
> There is a reason, it's a nightmare to maintain and handle merges for,

> just don't do it.

> 

> Read the comments at the top of the pci_ids.h file if you are curious

> why we don't even do this for PCI device ids anymore for the past 10+

> years.

> 

> So no, please do not create such a common file, it is not needed or a

> good idea.


Oops. I have sent it.
Greg Kroah-Hartman Nov. 3, 2020, 9:55 a.m. UTC | #5
On Tue, Nov 03, 2020 at 05:46:37PM +0800, Hayes Wang wrote:
> diff --git a/include/linux/usb/usb_vendor_id.h b/include/linux/usb/usb_vendor_id.h
> new file mode 100644
> index 000000000000..23b6e6849515
> --- /dev/null
> +++ b/include/linux/usb/usb_vendor_id.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +

<snip>

No, this is not ok, sorry.  Please see the top of the pci_ids.h file why
we do not do this.

There is nothing wrong with putting the individual ids in the different
drivers, we don't want one single huge file that is a pain for merges
and builds.  We learn from our past mistakes, please do not fail to
learn from history :)

thanks,

greg k-h
Jakub Kicinski Nov. 3, 2020, 4:15 p.m. UTC | #6
On Tue, 3 Nov 2020 10:32:41 +0100 Greg Kroah-Hartman wrote:
> On Mon, Nov 02, 2020 at 11:47:18AM -0800, Jakub Kicinski wrote:
> > On Mon, 2 Nov 2020 07:20:15 +0000 Hayes Wang wrote:  
> > > Jakub Kicinski <kuba@kernel.org>  
> > > > Can you describe the use case in more detail?
> > > > 
> > > > AFAICT r8152 defines a match for the exact same device.
> > > > Does it not mean that which driver is used will be somewhat random
> > > > if both are built?    
> > > 
> > > I export rtl_get_version() from r8152. It would return none zero
> > > value if r8152 could support this device. Both r8152 and r8153_ecm
> > > would check the return value of rtl_get_version() in porbe().
> > > Therefore, if rtl_get_version() return none zero value, the r8152
> > > is used for the device with vendor mode. Otherwise, the r8153_ecm
> > > is used for the device with ECM mode.  
> > 
> > Oh, I see, I missed that the rtl_get_version() checking is the inverse
> > of r8152.
> >   
> > > > > +/* Define these values to match your device */
> > > > > +#define VENDOR_ID_REALTEK		0x0bda
> > > > > +#define VENDOR_ID_MICROSOFT		0x045e
> > > > > +#define VENDOR_ID_SAMSUNG		0x04e8
> > > > > +#define VENDOR_ID_LENOVO		0x17ef
> > > > > +#define VENDOR_ID_LINKSYS		0x13b1
> > > > > +#define VENDOR_ID_NVIDIA		0x0955
> > > > > +#define VENDOR_ID_TPLINK		0x2357    
> > > > 
> > > > $ git grep 0x2357 | grep -i tplink
> > > > drivers/net/usb/cdc_ether.c:#define TPLINK_VENDOR_ID	0x2357
> > > > drivers/net/usb/r8152.c:#define VENDOR_ID_TPLINK		0x2357
> > > > drivers/usb/serial/option.c:#define TPLINK_VENDOR_ID			0x2357
> > > > 
> > > > $ git grep 0x17ef | grep -i lenovo
> > > > drivers/hid/hid-ids.h:#define USB_VENDOR_ID_LENOVO		0x17ef
> > > > drivers/hid/wacom.h:#define USB_VENDOR_ID_LENOVO	0x17ef
> > > > drivers/net/usb/cdc_ether.c:#define LENOVO_VENDOR_ID	0x17ef
> > > > drivers/net/usb/r8152.c:#define VENDOR_ID_LENOVO		0x17ef
> > > > 
> > > > Time to consolidate those vendor id defines perhaps?    
> > > 
> > > It seems that there is no such header file which I could include
> > > or add the new vendor IDs.  
> > 
> > Please create one. (Adding Greg KH to the recipients, in case there is
> > a reason that USB subsystem doesn't have a common vendor id header.)  
> 
> There is a reason, it's a nightmare to maintain and handle merges for,
> just don't do it.

Ah! Good that we asked :)

> Read the comments at the top of the pci_ids.h file if you are curious
> why we don't even do this for PCI device ids anymore for the past 10+
> years.
> 
> So no, please do not create such a common file, it is not needed or a
> good idea.

I wouldn't go that far, PCI subsystem just doesn't want everyone to add
IDs to the shared file unless there is a reason.

 *	Do not add new entries to this file unless the definitions
 *	are shared between multiple drivers.

Which seems quite reasonable. But it is most certainly your call :)
Hayes Wang Nov. 4, 2020, 1:39 a.m. UTC | #7
Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, November 4, 2020 12:16 AM
[...]
> > So no, please do not create such a common file, it is not needed or a
> > good idea.
> 
> I wouldn't go that far, PCI subsystem just doesn't want everyone to add
> IDs to the shared file unless there is a reason.
> 
>  *	Do not add new entries to this file unless the definitions
>  *	are shared between multiple drivers.
> 
> Which seems quite reasonable. But it is most certainly your call :)

Do I have to resend this patch?

Best Regards,
Hayes
Jakub Kicinski Nov. 4, 2020, 1:44 a.m. UTC | #8
On Wed, 4 Nov 2020 01:39:52 +0000 Hayes Wang wrote:
> Jakub Kicinski <kuba@kernel.org>

> > Sent: Wednesday, November 4, 2020 12:16 AM  

> [...]

> > > So no, please do not create such a common file, it is not needed or a

> > > good idea.  

> > 

> > I wouldn't go that far, PCI subsystem just doesn't want everyone to add

> > IDs to the shared file unless there is a reason.

> > 

> >  *	Do not add new entries to this file unless the definitions

> >  *	are shared between multiple drivers.

> > 

> > Which seems quite reasonable. But it is most certainly your call :)  

> 

> Do I have to resend this patch?


Yes please, that'd be easiest for me. Also Oliver wasn't CCed on this
posting.
diff mbox series

Patch

diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
index 99fd12be2111..99381e6bea78 100644
--- a/drivers/net/usb/Makefile
+++ b/drivers/net/usb/Makefile
@@ -13,7 +13,7 @@  obj-$(CONFIG_USB_LAN78XX)	+= lan78xx.o
 obj-$(CONFIG_USB_NET_AX8817X)	+= asix.o
 asix-y := asix_devices.o asix_common.o ax88172a.o
 obj-$(CONFIG_USB_NET_AX88179_178A)      += ax88179_178a.o
-obj-$(CONFIG_USB_NET_CDCETHER)	+= cdc_ether.o
+obj-$(CONFIG_USB_NET_CDCETHER)	+= cdc_ether.o r8153_ecm.o
 obj-$(CONFIG_USB_NET_CDC_EEM)	+= cdc_eem.o
 obj-$(CONFIG_USB_NET_DM9601)	+= dm9601.o
 obj-$(CONFIG_USB_NET_SR9700)	+= sr9700.o
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index b1770489aca5..1b5d10f1de5f 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -6615,7 +6615,7 @@  static int rtl_fw_init(struct r8152 *tp)
 	return 0;
 }
 
-static u8 rtl_get_version(struct usb_interface *intf)
+u8 rtl8152_get_version(struct usb_interface *intf)
 {
 	struct usb_device *udev = interface_to_usbdev(intf);
 	u32 ocp_data = 0;
@@ -6673,12 +6673,13 @@  static u8 rtl_get_version(struct usb_interface *intf)
 
 	return version;
 }
+EXPORT_SYMBOL_GPL(rtl8152_get_version);
 
 static int rtl8152_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
 	struct usb_device *udev = interface_to_usbdev(intf);
-	u8 version = rtl_get_version(intf);
+	u8 version = rtl8152_get_version(intf);
 	struct r8152 *tp;
 	struct net_device *netdev;
 	int ret;
diff --git a/drivers/net/usb/r8153_ecm.c b/drivers/net/usb/r8153_ecm.c
new file mode 100644
index 000000000000..723841525a6a
--- /dev/null
+++ b/drivers/net/usb/r8153_ecm.c
@@ -0,0 +1,174 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/mii.h>
+#include <linux/usb.h>
+#include <linux/usb/cdc.h>
+#include <linux/usb/usbnet.h>
+
+#define RTL8153_CMD		0X05
+#define RTL8153_REQT_READ	0xc0
+#define RTL8153_REQT_WRITE	0x40
+
+#define MCU_TYPE_PLA		0x0100
+#define OCP_BASE		0xe86c
+
+#define BYTE_EN_WORD		0x33
+
+#define REALTEK_VENDOR_ID	0x0bda
+
+#if IS_REACHABLE(CONFIG_USB_RTL8152)
+extern u8 rtl8152_get_version(struct usb_interface *intf);
+#endif
+
+static int pla_read_word(struct usbnet *dev, u16 index)
+{
+	u16 byen = BYTE_EN_WORD;
+	u8 shift = index & 2;
+	__le32 tmp;
+	int ret;
+
+	if (shift)
+		byen <<= shift;
+
+	index &= ~3;
+
+	ret = usbnet_read_cmd(dev, RTL8153_CMD, RTL8153_REQT_READ, index,
+			      MCU_TYPE_PLA | byen, &tmp, sizeof(tmp));
+	if (ret < 0)
+		goto out;
+
+	ret = __le32_to_cpu(tmp);
+	ret >>= (shift * 8);
+	ret &= 0xffff;
+
+out:
+	return ret;
+}
+
+static int pla_write_word(struct usbnet *dev, u16 index, u32 data)
+{
+	u32 mask = 0xffff;
+	u16 byen = BYTE_EN_WORD;
+	u8 shift = index & 2;
+	__le32 tmp;
+	int ret;
+
+	data &= mask;
+
+	if (shift) {
+		byen <<= shift;
+		mask <<= (shift * 8);
+		data <<= (shift * 8);
+	}
+
+	index &= ~3;
+
+	ret = usbnet_read_cmd(dev, RTL8153_CMD, RTL8153_REQT_READ, index,
+			      MCU_TYPE_PLA | byen, &tmp, sizeof(tmp));
+
+	if (ret < 0)
+		goto out;
+
+	data |= __le32_to_cpu(tmp) & ~mask;
+	tmp = __cpu_to_le32(data);
+
+	ret = usbnet_write_cmd(dev, RTL8153_CMD, RTL8153_REQT_WRITE, index,
+			       MCU_TYPE_PLA | byen, &tmp, sizeof(tmp));
+
+out:
+	return ret;
+}
+
+static int r8153_ecm_mdio_read(struct net_device *netdev, int phy_id, int reg)
+{
+	struct usbnet *dev = netdev_priv(netdev);
+	int ret;
+
+	ret = pla_write_word(dev, OCP_BASE, 0xa000);
+	if (ret < 0)
+		goto out;
+
+	ret = pla_read_word(dev, 0xb400 + reg * 2);
+
+out:
+	return ret;
+}
+
+static void r8153_ecm_mdio_write(struct net_device *netdev, int phy_id, int reg, int val)
+{
+	struct usbnet *dev = netdev_priv(netdev);
+	int ret;
+
+	ret = pla_write_word(dev, OCP_BASE, 0xa000);
+	if (ret < 0)
+		return;
+
+	ret = pla_write_word(dev, 0xb400 + reg * 2, val);
+}
+
+static int r8153_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int status;
+
+	status = usbnet_cdc_bind(dev, intf);
+	if (status < 0)
+		return status;
+
+	dev->mii.dev = dev->net;
+	dev->mii.mdio_read = r8153_ecm_mdio_read;
+	dev->mii.mdio_write = r8153_ecm_mdio_write;
+	dev->mii.reg_num_mask = 0x1f;
+	dev->mii.supports_gmii = 1;
+
+	return status;
+}
+
+static const struct driver_info r8153_info = {
+	.description =	"RTL8153 ECM Device",
+	.flags =	FLAG_ETHER,
+	.bind =		r8153_bind,
+	.unbind =	usbnet_cdc_unbind,
+	.status =	usbnet_cdc_status,
+	.manage_power =	usbnet_manage_power,
+};
+
+static const struct usb_device_id products[] = {
+{
+	USB_DEVICE_AND_INTERFACE_INFO(REALTEK_VENDOR_ID, 0x8153, USB_CLASS_COMM,
+				      USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
+	.driver_info = (unsigned long)&r8153_info,
+},
+
+	{ },		/* END */
+};
+MODULE_DEVICE_TABLE(usb, products);
+
+static int rtl8153_ecm_probe(struct usb_interface *intf,
+			     const struct usb_device_id *id)
+{
+#if IS_REACHABLE(CONFIG_USB_RTL8152)
+	if (rtl8152_get_version(intf))
+		return -ENODEV;
+#endif
+
+	return usbnet_probe(intf, id);
+}
+
+static struct usb_driver r8153_ecm_driver = {
+	.name =		"r8153_ecm",
+	.id_table =	products,
+	.probe =	rtl8153_ecm_probe,
+	.disconnect =	usbnet_disconnect,
+	.suspend =	usbnet_suspend,
+	.resume =	usbnet_resume,
+	.reset_resume =	usbnet_resume,
+	.supports_autosuspend = 1,
+	.disable_hub_initiated_lpm = 1,
+};
+
+module_usb_driver(r8153_ecm_driver);
+
+MODULE_AUTHOR("Hayes Wang");
+MODULE_DESCRIPTION("Realtek USB ECM device");
+MODULE_LICENSE("GPL");