[PATCH v2 0/6] usb: gadget: aspeed: allow to customize vhub device

[PATCH v2 0/6] usb: gadget: aspeed: allow to customize vhub device

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

This patch series allows people to customize aspeed-usb-vhub device IDs
and strings via device tree.

Patch #1 converts the single "usb_gadget_strings" instance to a list of
"usb_gadget_strings" so it's more convenient to support miltiple
languages.

Patch #2 moves LANGID validation code from configfs.c to usbstring.c so
it can be used by aspeed-vhub driver.

Patch #3 initializes aspeed-vhub strings with default, or from device
tree if according device tree properties are defined.

Patch #4 overrides aspeed-vhub's device IDs if according properties are
defined in device tree.

Patch #5 moves fixup-usb1-dev-desc logic from get-descriptor handler to
vhub-init time so the descriptor is patched only once.

Patch #6 documents new device tree properties in dt-binding document.

Tao Ren (6):
  usb: gadget: aspeed: support multiple language strings
  usb: gadget: add "usb_validate_langid" function
  usb: gadget: aspeed: allow to set usb strings in device tree
  usb: gadget: aspeed: allow to set device IDs in device tree
  usb: gadget: aspeed: fixup usb1 device descriptor at init time
  dt-bindings: usb: document aspeed vhub device ID/string properties

 .../bindings/usb/aspeed,usb-vhub.yaml         |  68 +++++
 drivers/usb/gadget/configfs.c                 |  14 +-
 drivers/usb/gadget/udc/aspeed-vhub/core.c     |   4 +-
 drivers/usb/gadget/udc/aspeed-vhub/hub.c      | 236 ++++++++++++++++--
 drivers/usb/gadget/udc/aspeed-vhub/vhub.h     |   4 +-
 drivers/usb/gadget/usbstring.c                |  24 ++
 include/linux/usb/gadget.h                    |   3 +
 7 files changed, 312 insertions(+), 41 deletions(-)

-- 
2.17.1

[PATCH v2 1/6] usb: gadget: aspeed: support multiple language strings

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

This patch introduces a link list to store string descriptors with
different languages, and "ast_vhub_rep_string" function is also improved
to support multiple language usb strings.

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 No change in v2:
   - the patch is added into the series since v2.

 drivers/usb/gadget/udc/aspeed-vhub/core.c |   4 +-
 drivers/usb/gadget/udc/aspeed-vhub/hub.c  | 140 +++++++++++++++++++---
 drivers/usb/gadget/udc/aspeed-vhub/vhub.h |   4 +-
 3 files changed, 131 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c
index 555e8645fb1e..cdf96911e4b1 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
@@ -408,7 +408,9 @@ static int ast_vhub_probe(struct platform_device *pdev)
 		goto err;
 
 	/* Init hub emulation */
-	ast_vhub_init_hub(vhub);
+	rc = ast_vhub_init_hub(vhub);
+	if (rc)
+		goto err;
 
 	/* Initialize HW */
 	ast_vhub_init_hw(vhub);
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index 6e565c3dbb5b..35edf37553f0 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -50,6 +50,7 @@
 #define KERNEL_VER	bin2bcd(((LINUX_VERSION_CODE >> 8) & 0x0ff))
 
 enum {
+	AST_VHUB_STR_INDEX_MAX = 4,
 	AST_VHUB_STR_MANUF = 3,
 	AST_VHUB_STR_PRODUCT = 2,
 	AST_VHUB_STR_SERIAL = 1,
@@ -310,23 +311,77 @@ static int ast_vhub_rep_desc(struct ast_vhub_ep *ep,
 	return ast_vhub_reply(ep, NULL, len);
 }
 
+static struct usb_gadget_strings*
+ast_vhub_str_of_container(struct usb_gadget_string_container *container)
+{
+	return (struct usb_gadget_strings *)container->stash;
+}
+
+static int ast_vhub_collect_languages(struct ast_vhub *vhub, void *buf,
+				      size_t size)
+{
+	int rc, hdr_len, nlangs, max_langs;
+	struct usb_gadget_strings *lang_str;
+	struct usb_gadget_string_container *container;
+	struct usb_string_descriptor *sdesc = buf;
+
+	nlangs = 0;
+	hdr_len = sizeof(struct usb_descriptor_header);
+	max_langs = (size - hdr_len) / sizeof(sdesc->wData[0]);
+	list_for_each_entry(container, &vhub->vhub_str_desc, list) {
+		if (nlangs >= max_langs)
+			break;
+
+		lang_str = ast_vhub_str_of_container(container);
+		sdesc->wData[nlangs++] = cpu_to_le16(lang_str->language);
+	}
+
+	rc = hdr_len + nlangs * sizeof(sdesc->wData[0]);
+	sdesc->bLength = rc;
+	sdesc->bDescriptorType = USB_DT_STRING;
+
+	return rc;
+}
+
+static struct usb_gadget_strings *ast_vhub_lookup_string(struct ast_vhub *vhub,
+							 u16 lang_id)
+{
+	struct usb_gadget_strings *lang_str;
+	struct usb_gadget_string_container *container;
+
+	list_for_each_entry(container, &vhub->vhub_str_desc, list) {
+		lang_str = ast_vhub_str_of_container(container);
+		if (lang_str->language == lang_id)
+			return lang_str;
+	}
+
+	return NULL;
+}
+
 static int ast_vhub_rep_string(struct ast_vhub_ep *ep,
 			       u8 string_id, u16 lang_id,
 			       u16 len)
 {
-	int rc = usb_gadget_get_string(&ep->vhub->vhub_str_desc,
-					string_id, ep->buf);
+	int rc;
+	u8 buf[256];
+	struct ast_vhub *vhub = ep->vhub;
+	struct usb_gadget_strings *lang_str;
 
-	/*
-	 * This should never happen unless we put too big strings in
-	 * the array above
-	 */
-	BUG_ON(rc >= AST_VHUB_EP0_MAX_PACKET);
+	if (string_id == 0) {
+		rc = ast_vhub_collect_languages(vhub, buf, sizeof(buf));
+	} else {
+		lang_str = ast_vhub_lookup_string(vhub, lang_id);
+		if (!lang_str)
+			return std_req_stall;
+
+		rc = usb_gadget_get_string(lang_str, string_id, buf);
+	}
 
-	if (rc < 0)
+	if (rc < 0 || rc >= AST_VHUB_EP0_MAX_PACKET)
 		return std_req_stall;
 
 	/* Shoot it from the EP buffer */
+	memcpy(ep->buf, buf, rc);
 	return ast_vhub_reply(ep, NULL, min_t(u16, rc, len));
 }
 
@@ -832,8 +887,64 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
 	writel(0, vhub->regs + AST_VHUB_EP1_STS_CHG);
 }
 
-static void ast_vhub_init_desc(struct ast_vhub *vhub)
+static struct usb_gadget_string_container*
+ast_vhub_str_container_alloc(struct ast_vhub *vhub)
+{
+	unsigned int size;
+	struct usb_string *str_array;
+	struct usb_gadget_strings *lang_str;
+	struct usb_gadget_string_container *container;
+
+	size = sizeof(*container);
+	size += sizeof(struct usb_gadget_strings);
+	size += sizeof(struct usb_string) * AST_VHUB_STR_INDEX_MAX;
+	container = devm_kzalloc(&vhub->pdev->dev, size, GFP_KERNEL);
+	if (!container)
+		return ERR_PTR(-ENOMEM);
+
+	lang_str = ast_vhub_str_of_container(container);
+	str_array = (struct usb_string *)(lang_str + 1);
+	lang_str->strings = str_array;
+	return container;
+}
+
+static void ast_vhub_str_deep_copy(struct usb_gadget_strings *dest,
+				   const struct usb_gadget_strings *src)
+{
+	struct usb_string *src_array = src->strings;
+	struct usb_string *dest_array = dest->strings;
+
+	dest->language = src->language;
+	if (src_array && dest_array) {
+		do {
+			*dest_array = *src_array;
+			dest_array++;
+			src_array++;
+		} while (src_array->s);
+	}
+}
+
+static int ast_vhub_str_alloc_add(struct ast_vhub *vhub,
+				  const struct usb_gadget_strings *src_str)
 {
+	struct usb_gadget_strings *dest_str;
+	struct usb_gadget_string_container *container;
+
+	container = ast_vhub_str_container_alloc(vhub);
+	if (IS_ERR(container))
+		return PTR_ERR(container);
+
+	dest_str = ast_vhub_str_of_container(container);
+	ast_vhub_str_deep_copy(dest_str, src_str);
+	list_add_tail(&container->list, &vhub->vhub_str_desc);
+
+	return 0;
+}
+
+static int ast_vhub_init_desc(struct ast_vhub *vhub)
+{
+	int ret;
+
 	/* Initialize vhub Device Descriptor. */
 	memcpy(&vhub->vhub_dev_desc, &ast_vhub_dev_desc,
 		sizeof(vhub->vhub_dev_desc));
@@ -848,15 +959,16 @@ static void ast_vhub_init_desc(struct ast_vhub *vhub)
 	vhub->vhub_hub_desc.bNbrPorts = vhub->max_ports;
 
 	/* Initialize vhub String Descriptors. */
-	memcpy(&vhub->vhub_str_desc, &ast_vhub_strings,
-		sizeof(vhub->vhub_str_desc));
+	INIT_LIST_HEAD(&vhub->vhub_str_desc);
+	ret = ast_vhub_str_alloc_add(vhub, &ast_vhub_strings);
+
+	return ret;
 }
 
-void ast_vhub_init_hub(struct ast_vhub *vhub)
+int ast_vhub_init_hub(struct ast_vhub *vhub)
 {
 	vhub->speed = USB_SPEED_UNKNOWN;
 	INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
 
-	ast_vhub_init_desc(vhub);
+	return ast_vhub_init_desc(vhub);
 }
-
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
index 23a1ac91f8d2..2e5a1ef14a75 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
+++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
@@ -421,7 +421,7 @@ struct ast_vhub {
 	struct usb_device_descriptor	vhub_dev_desc;
 	struct ast_vhub_full_cdesc	vhub_conf_desc;
 	struct usb_hub_descriptor	vhub_hub_desc;
-	struct usb_gadget_strings	vhub_str_desc;
+	struct list_head		vhub_str_desc;
 };
 
 /* Standard request handlers result codes */
@@ -531,7 +531,7 @@ int __ast_vhub_simple_reply(struct ast_vhub_ep *ep, int len, ...);
 			       __VA_ARGS__)
 
 /* hub.c */
-void ast_vhub_init_hub(struct ast_vhub *vhub);
+int ast_vhub_init_hub(struct ast_vhub *vhub);
 enum std_req_rc ast_vhub_std_hub_request(struct ast_vhub_ep *ep,
 					 struct usb_ctrlrequest *crq);
 enum std_req_rc ast_vhub_class_hub_request(struct ast_vhub_ep *ep,
-- 
2.17.1

[PATCH v2 2/6] usb: gadget: add "usb_validate_langid" function

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

The USB LANGID validation code in "check_user_usb_string" function is
moved to "usb_validate_langid" function which can be used by other usb
gadget drivers.

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 No change in v2:
   - the patch is added into the series since v2.

 drivers/usb/gadget/configfs.c  | 14 +-------------
 drivers/usb/gadget/usbstring.c | 24 ++++++++++++++++++++++++
 include/linux/usb/gadget.h     |  3 +++
 3 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 32b637e3e1fa..822ef0470c5f 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -13,8 +13,6 @@
 int check_user_usb_string(const char *name,
 		struct usb_gadget_strings *stringtab_dev)
 {
-	unsigned primary_lang;
-	unsigned sub_lang;
 	u16 num;
 	int ret;
 
@@ -22,17 +20,7 @@ int check_user_usb_string(const char *name,
 	if (ret)
 		return ret;
 
-	primary_lang = num & 0x3ff;
-	sub_lang = num >> 10;
-
-	/* simple sanity check for valid langid */
-	switch (primary_lang) {
-	case 0:
-	case 0x62 ... 0xfe:
-	case 0x100 ... 0x3ff:
-		return -EINVAL;
-	}
-	if (!sub_lang)
+	if (!usb_validate_langid(num))
 		return -EINVAL;
 
 	stringtab_dev->language = num;
diff --git a/drivers/usb/gadget/usbstring.c b/drivers/usb/gadget/usbstring.c
index 7c24d1ce1088..58a4d3325090 100644
--- a/drivers/usb/gadget/usbstring.c
+++ b/drivers/usb/gadget/usbstring.c
@@ -65,3 +65,27 @@ usb_gadget_get_string (const struct usb_gadget_strings *table, int id, u8 *buf)
 	return buf [0];
 }
 EXPORT_SYMBOL_GPL(usb_gadget_get_string);
+
+/**
+ * usb_validate_langid - validate usb language identifiers
+ * @lang: usb language identifier
+ *
+ * Returns true for valid language identifier, otherwise false.
+ */
+bool usb_validate_langid(u16 langid)
+{
+	u16 primary_lang = langid & 0x3ff;	/* bit [9:0] */
+	u16 sub_lang = langid >> 10;		/* bit [15:10] */
+
+	switch (primary_lang) {
+	case 0:
+	case 0x62 ... 0xfe:
+	case 0x100 ... 0x3ff:
+		return false;
+	}
+	if (!sub_lang)
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(usb_validate_langid);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 124462d65eac..1a05227bdfae 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -773,6 +773,9 @@ struct usb_gadget_string_container {
 /* put descriptor for string with that id into buf (buflen >= 256) */
 int usb_gadget_get_string(const struct usb_gadget_strings *table, int id, u8 *buf);
 
+/* check if the given language identifier is valid */
+bool usb_validate_langid(u16 langid);
+
 /*-------------------------------------------------------------------------*/
 
 /* utility to simplify managing config descriptors */
-- 
2.17.1

[PATCH v2 3/6] usb: gadget: aspeed: allow to set usb strings in device tree

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

If "vhub,string-descriptor" device tree property is defined, the driver
will load string descriptors from device tree; otherwise, the default
string descriptors will be used.

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 No change in v2:
   - the patch is added into the series since v2.

 drivers/usb/gadget/udc/aspeed-vhub/hub.c | 58 +++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index 35edf37553f0..421631d86a17 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -941,9 +941,61 @@ static int ast_vhub_str_alloc_add(struct ast_vhub *vhub,
 	return 0;
 }
 
+static const struct {
+	const char *name;
+	u8 id;
+} str_id_map[] = {
+	{"manufacturer",	AST_VHUB_STR_MANUF},
+	{"product",		AST_VHUB_STR_PRODUCT},
+	{"serial-number",	AST_VHUB_STR_SERIAL},
+	{},
+};
+
+static int ast_vhub_of_parse_str_desc(struct ast_vhub *vhub,
+				      const struct device_node *desc_np)
+{
+	u32 langid;
+	int ret = 0;
+	int i, offset;
+	const char *str;
+	struct device_node *child;
+	struct usb_string str_array[AST_VHUB_STR_INDEX_MAX];
+	struct usb_gadget_strings lang_str = {
+		.strings = (struct usb_string *)str_array,
+	};
+
+	for_each_child_of_node(desc_np, child) {
+		if (of_property_read_u32(child, "reg", &langid))
+			continue; /* no language identifier specified */
+
+		if (!usb_validate_langid(langid))
+			continue; /* invalid language identifier */
+
+		lang_str.language = langid;
+		for (i = offset = 0; str_id_map[i].name; i++) {
+			str = of_get_property(child, str_id_map[i].name, NULL);
+			if (str) {
+				str_array[offset].s = str;
+				str_array[offset].id = str_id_map[i].id;
+				offset++;
+			}
+		}
+		str_array[offset].id = 0;
+		str_array[offset].s = NULL;
+
+		ret = ast_vhub_str_alloc_add(vhub, &lang_str);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
 static int ast_vhub_init_desc(struct ast_vhub *vhub)
 {
 	int ret;
+	struct device_node *desc_np;
+	const struct device_node *vhub_np = vhub->pdev->dev.of_node;
 
 	/* Initialize vhub Device Descriptor. */
 	memcpy(&vhub->vhub_dev_desc, &ast_vhub_dev_desc,
@@ -960,7 +1012,11 @@ static int ast_vhub_init_desc(struct ast_vhub *vhub)
 
 	/* Initialize vhub String Descriptors. */
 	INIT_LIST_HEAD(&vhub->vhub_str_desc);
-	ret = ast_vhub_str_alloc_add(vhub, &ast_vhub_strings);
+	desc_np = of_get_child_by_name(vhub_np, "vhub-strings");
+	if (desc_np)
+		ret = ast_vhub_of_parse_str_desc(vhub, desc_np);
+	else
+		ret = ast_vhub_str_alloc_add(vhub, &ast_vhub_strings);
 
 	return ret;
 }
-- 
2.17.1

[PATCH v2 4/6] usb: gadget: aspeed: allow to set device IDs in device tree

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

The patch overrides idVendor, idProduct and bcdDevice fields in vhub
Device Descriptor if according device tree properties are defined.

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 Changes in v2:
   - update per-vhub device descriptor instance instead of the global
     default descriptor.

 drivers/usb/gadget/udc/aspeed-vhub/hub.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index 421631d86a17..13fba91aad6a 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -887,6 +887,26 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
 	writel(0, vhub->regs + AST_VHUB_EP1_STS_CHG);
 }
 
+static void ast_vhub_of_parse_dev_desc(struct ast_vhub *vhub,
+				       const struct device_node *vhub_np)
+{
+	u16 id;
+	u32 data;
+
+	if (!of_property_read_u32(vhub_np, "vhub-vendor-id", &data)) {
+		id = (u16)data;
+		vhub->vhub_dev_desc.idVendor = cpu_to_le16(id);
+	}
+	if (!of_property_read_u32(vhub_np, "vhub-product-id", &data)) {
+		id = (u16)data;
+		vhub->vhub_dev_desc.idProduct = cpu_to_le16(id);
+	}
+	if (!of_property_read_u32(vhub_np, "vhub-device-revision", &data)) {
+		id = (u16)data;
+		vhub->vhub_dev_desc.bcdDevice = cpu_to_le16(id);
+	}
+}
+
 static struct usb_gadget_string_container*
 ast_vhub_str_container_alloc(struct ast_vhub *vhub)
 {
@@ -1000,6 +1020,7 @@ static int ast_vhub_init_desc(struct ast_vhub *vhub)
 	/* Initialize vhub Device Descriptor. */
 	memcpy(&vhub->vhub_dev_desc, &ast_vhub_dev_desc,
 		sizeof(vhub->vhub_dev_desc));
+	ast_vhub_of_parse_dev_desc(vhub, vhub_np);
 
 	/* Initialize vhub Configuration Descriptor. */
 	memcpy(&vhub->vhub_conf_desc, &ast_vhub_conf_desc,
-- 
2.17.1

[PATCH v2 5/6] usb: gadget: aspeed: fixup usb1 device descriptor at init time

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

This patch moves patch-usb1-dev-desc logic from get-descriptor handler
to "ast_vhub_fixup_usb1_dev_desc" function so the code is executed only
once (at vhub initial time).

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 Changes in v2:
   - update per-vhub device descriptor instance instead of the global
     default descriptor.

 drivers/usb/gadget/udc/aspeed-vhub/hub.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index 13fba91aad6a..6497185ec4e7 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -73,13 +73,6 @@ static const struct usb_device_descriptor ast_vhub_dev_desc = {
 	.bNumConfigurations	= 1,
 };
 
-/* Patches to the above when forcing USB1 mode */
-static void ast_vhub_patch_dev_desc_usb1(struct usb_device_descriptor *desc)
-{
-	desc->bcdUSB = cpu_to_le16(0x0100);
-	desc->bDeviceProtocol = 0;
-}
-
 /*
  * Configuration descriptor: same comments as above
  * regarding handling USB1 mode.
@@ -303,10 +296,6 @@ static int ast_vhub_rep_desc(struct ast_vhub_ep *ep,
 	if (len > dsize)
 		len = dsize;
 
-	/* Patch it if forcing USB1 */
-	if (desc_type == USB_DT_DEVICE && ep->vhub->force_usb1)
-		ast_vhub_patch_dev_desc_usb1(ep->buf);
-
 	/* Shoot it from the EP buffer */
 	return ast_vhub_reply(ep, NULL, len);
 }
@@ -907,6 +896,12 @@ static void ast_vhub_of_parse_dev_desc(struct ast_vhub *vhub,
 	}
 }
 
+static void ast_vhub_fixup_usb1_dev_desc(struct ast_vhub *vhub)
+{
+	vhub->vhub_dev_desc.bcdUSB = cpu_to_le16(0x0100);
+	vhub->vhub_dev_desc.bDeviceProtocol = 0;
+}
+
 static struct usb_gadget_string_container*
 ast_vhub_str_container_alloc(struct ast_vhub *vhub)
 {
@@ -1021,6 +1016,8 @@ static int ast_vhub_init_desc(struct ast_vhub *vhub)
 	memcpy(&vhub->vhub_dev_desc, &ast_vhub_dev_desc,
 		sizeof(vhub->vhub_dev_desc));
 	ast_vhub_of_parse_dev_desc(vhub, vhub_np);
+	if (vhub->force_usb1)
+		ast_vhub_fixup_usb1_dev_desc(vhub);
 
 	/* Initialize vhub Configuration Descriptor. */
 	memcpy(&vhub->vhub_conf_desc, &ast_vhub_conf_desc,
-- 
2.17.1

[PATCH v2 6/6] dt-bindings: usb: document aspeed vhub device ID/string properties

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

Update device tree binding document for aspeed vhub's device IDs and
string properties.

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 No change in v2:
   - the patch is added into the series since v2.

 .../bindings/usb/aspeed,usb-vhub.yaml         | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
index 06399ba0d9e4..5b2e8d867219 100644
--- a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
+++ b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
@@ -52,6 +52,59 @@ properties:
         minimum: 1
         maximum: 21
 
+  vhub-vendor-id:
+    description: vhub Vendor ID
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - maximum: 65535
+
+  vhub-product-id:
+    description: vhub Product ID
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - maximum: 65535
+
+  vhub-device-revision:
+    description: vhub Device Revision in binary-coded decimal
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - maximum: 65535
+
+  vhub-strings:
+    type: object
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      '^string@[0-9a-f]+$':
+        type: object
+        description: string descriptors of the specific language
+
+        properties:
+          reg:
+            maxItems: 1
+            description: 16-bit Language Identifier defined by USB-IF
+
+          manufacturer:
+            description: vhub manufacturer
+            allOf:
+              - $ref: /schemas/types.yaml#/definitions/string
+
+          product:
+            description: vhub product name
+            allOf:
+              - $ref: /schemas/types.yaml#/definitions/string
+
+          serial-number:
+            description: vhub device serial number
+            allOf:
+              - $ref: /schemas/types.yaml#/definitions/string
+
 required:
   - compatible
   - reg
@@ -74,4 +127,19 @@ examples:
             aspeed,vhub-generic-endpoints = <15>;
             pinctrl-names = "default";
             pinctrl-0 = <&pinctrl_usb2ad_default>;
+
+            vhub-vendor-id = <0x1d6b>;
+            vhub-product-id = <0x0107>;
+            vhub-device-revision = <0x0100>;
+            vhub-strings {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                string@0409 {
+                        reg = <0x0409>;
+                        manufacturer = "ASPEED";
+                        product = "USB Virtual Hub";
+                        serial-number = "0000";
+                };
+            };
     };
-- 
2.17.1

Re: [PATCH v2 6/6] dt-bindings: usb: document aspeed vhub device ID/string properties

[Rob Herring]

On Sun, Mar 15, 2020 at 12:16:32PM -0700, rentao.bupt@gmail.com wrote:
> From: Tao Ren <rentao.bupt@gmail.com>
> 
> Update device tree binding document for aspeed vhub's device IDs and
> string properties.
> 
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  No change in v2:
>    - the patch is added into the series since v2.
> 
>  .../bindings/usb/aspeed,usb-vhub.yaml         | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> index 06399ba0d9e4..5b2e8d867219 100644
> --- a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> +++ b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> @@ -52,6 +52,59 @@ properties:
>          minimum: 1
>          maximum: 21
>  
> +  vhub-vendor-id:
> +    description: vhub Vendor ID
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - maximum: 65535
> +
> +  vhub-product-id:
> +    description: vhub Product ID
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - maximum: 65535

There's already standard 'vendor-id' and 'device-id' properties. Use 
those.

> +
> +  vhub-device-revision:

Specific to USB, not vhub.

> +    description: vhub Device Revision in binary-coded decimal
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - maximum: 65535
> +
> +  vhub-strings:
> +    type: object
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      '^string@[0-9a-f]+$':
> +        type: object
> +        description: string descriptors of the specific language
> +
> +        properties:
> +          reg:
> +            maxItems: 1
> +            description: 16-bit Language Identifier defined by USB-IF
> +
> +          manufacturer:
> +            description: vhub manufacturer
> +            allOf:
> +              - $ref: /schemas/types.yaml#/definitions/string
> +
> +          product:
> +            description: vhub product name
> +            allOf:
> +              - $ref: /schemas/types.yaml#/definitions/string
> +
> +          serial-number:
> +            description: vhub device serial number
> +            allOf:
> +              - $ref: /schemas/types.yaml#/definitions/string

For all of this, it's USB specific, not vhub specific. I'm not sure this 
is the right approach. It might be better to just define properties 
which are just raw USB descriptors rather than inventing some DT format 
that then has to be converted into USB descriptors.

Rob

Re: [PATCH v2 6/6] dt-bindings: usb: document aspeed vhub device ID/string properties

[Benjamin Herrenschmidt]

On Mon, 2020-03-30 at 13:23 -0600, Rob Herring wrote:
> On Sun, Mar 15, 2020 at 12:16:32PM -0700, rentao.bupt@gmail.com wrote:
> > From: Tao Ren <rentao.bupt@gmail.com>
> > 
> > Update device tree binding document for aspeed vhub's device IDs and
> > string properties.
> > 
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > ---
> >  No change in v2:
> >    - the patch is added into the series since v2.
> > 
> >  .../bindings/usb/aspeed,usb-vhub.yaml         | 68 +++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> > index 06399ba0d9e4..5b2e8d867219 100644
> > --- a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> > +++ b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> > @@ -52,6 +52,59 @@ properties:
> >          minimum: 1
> >          maximum: 21
> >  
> > +  vhub-vendor-id:
> > +    description: vhub Vendor ID
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > +      - maximum: 65535
> > +
> > +  vhub-product-id:
> > +    description: vhub Product ID
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > +      - maximum: 65535
> 
> There's already standard 'vendor-id' and 'device-id' properties. Use 
> those.

So yes and no... I don't fundamentally object but keep in mind that
traditionally, the properties are about matching with a physical
hardware.

In this case however, we are describing a virtual piece of HW and so
those IDs are going to be picked up to be exposed as the USB
vendor/device of the vhub on the USB bus.

Not necessarily an issue but it's more "configuration" than "matching"
and as such, it might make sense to expose that with a prefix, though I
would prefer something like usb-vendor-id or usb,vendor-id...

> > +
> > +  vhub-device-revision:
> 
> Specific to USB, not vhub.

Same as the above.

> > +    description: vhub Device Revision in binary-coded decimal
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > +      - maximum: 65535
> > +
> > +  vhub-strings:
> > +    type: object
> > +
> > +    properties:
> > +      '#address-cells':
> > +        const: 1
> > +
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    patternProperties:
> > +      '^string@[0-9a-f]+$':
> > +        type: object
> > +        description: string descriptors of the specific language
> > +
> > +        properties:
> > +          reg:
> > +            maxItems: 1
> > +            description: 16-bit Language Identifier defined by USB-IF
> > +
> > +          manufacturer:
> > +            description: vhub manufacturer
> > +            allOf:
> > +              - $ref: /schemas/types.yaml#/definitions/string
> > +
> > +          product:
> > +            description: vhub product name
> > +            allOf:
> > +              - $ref: /schemas/types.yaml#/definitions/string
> > +
> > +          serial-number:
> > +            description: vhub device serial number
> > +            allOf:
> > +              - $ref: /schemas/types.yaml#/definitions/string
> 
> For all of this, it's USB specific, not vhub specific. I'm not sure this 
> is the right approach. It might be better to just define properties 
> which are just raw USB descriptors rather than inventing some DT format 
> that then has to be converted into USB descriptors.

Raw blob in the DT is rather annoying and leads to hard to parse stuff
for both humans and scripts. The main strenght of the DT is it's easy
to read and manipulate.

Also not the entire descriptor is configurable this way.

That said, it could be that using  the DT for the above is overkill and
instead, we should consider a configfs like the rest of USB gadget.
Though it isn't obvious how to do that, the current gadget stuff
doesn't really "fit" what we need here.

Maybe we could expose the port as UDCs but not actually expose them on
the bus until the hub is "activated" via a special configfs entry...

Cheers,
Ben.

> os the 
> Rob

Re: [PATCH v2 6/6] dt-bindings: usb: document aspeed vhub device ID/string properties

[Rob Herring]

On Mon, Mar 30, 2020 at 6:14 PM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Mon, 2020-03-30 at 13:23 -0600, Rob Herring wrote:
> > On Sun, Mar 15, 2020 at 12:16:32PM -0700, rentao.bupt@gmail.com wrote:
> > > From: Tao Ren <rentao.bupt@gmail.com>
> > >
> > > Update device tree binding document for aspeed vhub's device IDs and
> > > string properties.
> > >
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > > ---
> > >  No change in v2:
> > >    - the patch is added into the series since v2.
> > >
> > >  .../bindings/usb/aspeed,usb-vhub.yaml         | 68 +++++++++++++++++++
> > >  1 file changed, 68 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> > > index 06399ba0d9e4..5b2e8d867219 100644
> > > --- a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> > > @@ -52,6 +52,59 @@ properties:
> > >          minimum: 1
> > >          maximum: 21
> > >
> > > +  vhub-vendor-id:
> > > +    description: vhub Vendor ID
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > +      - maximum: 65535
> > > +
> > > +  vhub-product-id:
> > > +    description: vhub Product ID
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > +      - maximum: 65535
> >
> > There's already standard 'vendor-id' and 'device-id' properties. Use
> > those.
>
> So yes and no... I don't fundamentally object but keep in mind that
> traditionally, the properties are about matching with a physical
> hardware.
>
> In this case however, we are describing a virtual piece of HW and so
> those IDs are going to be picked up to be exposed as the USB
> vendor/device of the vhub on the USB bus.
>
> Not necessarily an issue but it's more "configuration" than "matching"
> and as such, it might make sense to expose that with a prefix, though I
> would prefer something like usb-vendor-id or usb,vendor-id...

For FDT uses, it's pretty much been configuration. It's mostly been
for PCI that I've seen these properties used.

> > > +
> > > +  vhub-device-revision:
> >
> > Specific to USB, not vhub.
>
> Same as the above.
>
> > > +    description: vhub Device Revision in binary-coded decimal
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > +      - maximum: 65535
> > > +
> > > +  vhub-strings:
> > > +    type: object
> > > +
> > > +    properties:
> > > +      '#address-cells':
> > > +        const: 1
> > > +
> > > +      '#size-cells':
> > > +        const: 0
> > > +
> > > +    patternProperties:
> > > +      '^string@[0-9a-f]+$':
> > > +        type: object
> > > +        description: string descriptors of the specific language
> > > +
> > > +        properties:
> > > +          reg:
> > > +            maxItems: 1
> > > +            description: 16-bit Language Identifier defined by USB-IF
> > > +
> > > +          manufacturer:
> > > +            description: vhub manufacturer
> > > +            allOf:
> > > +              - $ref: /schemas/types.yaml#/definitions/string
> > > +
> > > +          product:
> > > +            description: vhub product name
> > > +            allOf:
> > > +              - $ref: /schemas/types.yaml#/definitions/string
> > > +
> > > +          serial-number:
> > > +            description: vhub device serial number
> > > +            allOf:
> > > +              - $ref: /schemas/types.yaml#/definitions/string
> >
> > For all of this, it's USB specific, not vhub specific. I'm not sure this
> > is the right approach. It might be better to just define properties
> > which are just raw USB descriptors rather than inventing some DT format
> > that then has to be converted into USB descriptors.
>
> Raw blob in the DT is rather annoying and leads to hard to parse stuff
> for both humans and scripts. The main strenght of the DT is it's easy
> to read and manipulate.

True, and I'd certainly agree when we're talking about some vendor
specific blob. but there's already code/tools to parse USB
descriptors.

> Also not the entire descriptor is configurable this way.
>
> That said, it could be that using  the DT for the above is overkill and
> instead, we should consider a configfs like the rest of USB gadget.
> Though it isn't obvious how to do that, the current gadget stuff
> doesn't really "fit" what we need here.

Surely the descriptor building code can be shared at a minimum.

Regardless of whether gadget configfs fits, usually it is pretty clear
whether something belongs in DT or userspace. That should be decided
first.

> Maybe we could expose the port as UDCs but not actually expose them on
> the bus until the hub is "activated" via a special configfs entry...

If control of the hub is done by userspace, I'd think configuration
should be there too.

Rob

Re: [PATCH v2 6/6] dt-bindings: usb: document aspeed vhub device ID/string properties

[Tao Ren]

On Tue, Mar 31, 2020 at 10:21:10AM -0600, Rob Herring wrote:
> On Mon, Mar 30, 2020 at 6:14 PM Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > On Mon, 2020-03-30 at 13:23 -0600, Rob Herring wrote:
> > > On Sun, Mar 15, 2020 at 12:16:32PM -0700, rentao.bupt@gmail.com wrote:
> > > > From: Tao Ren <rentao.bupt@gmail.com>
> > > >
> > > > Update device tree binding document for aspeed vhub's device IDs and
> > > > string properties.
> > > >
> > > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > > > ---
> > > >  No change in v2:
> > > >    - the patch is added into the series since v2.
> > > >
> > > >  .../bindings/usb/aspeed,usb-vhub.yaml         | 68 +++++++++++++++++++
> > > >  1 file changed, 68 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> > > > index 06399ba0d9e4..5b2e8d867219 100644
> > > > --- a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> > > > +++ b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml
> > > > @@ -52,6 +52,59 @@ properties:
> > > >          minimum: 1
> > > >          maximum: 21
> > > >
> > > > +  vhub-vendor-id:
> > > > +    description: vhub Vendor ID
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > > +      - maximum: 65535
> > > > +
> > > > +  vhub-product-id:
> > > > +    description: vhub Product ID
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > > +      - maximum: 65535
> > >
> > > There's already standard 'vendor-id' and 'device-id' properties. Use
> > > those.
> >
> > So yes and no... I don't fundamentally object but keep in mind that
> > traditionally, the properties are about matching with a physical
> > hardware.
> >
> > In this case however, we are describing a virtual piece of HW and so
> > those IDs are going to be picked up to be exposed as the USB
> > vendor/device of the vhub on the USB bus.
> >
> > Not necessarily an issue but it's more "configuration" than "matching"
> > and as such, it might make sense to expose that with a prefix, though I
> > would prefer something like usb-vendor-id or usb,vendor-id...
> 
> For FDT uses, it's pretty much been configuration. It's mostly been
> for PCI that I've seen these properties used.

Thank you Rob and Ben for the comments. I thought about using "vendor-id"
or "idVendor" prefixed with "usb", "hub" or "vhub", and I chose "vhub"
just to distinguish from per-port usb devices' properties in the future.
Anyways I'm very happy to update the names per your suggestions.

> 
> > > > +
> > > > +  vhub-device-revision:
> > >
> > > Specific to USB, not vhub.
> >
> > Same as the above.
> >
> > > > +    description: vhub Device Revision in binary-coded decimal
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > > +      - maximum: 65535
> > > > +
> > > > +  vhub-strings:
> > > > +    type: object
> > > > +
> > > > +    properties:
> > > > +      '#address-cells':
> > > > +        const: 1
> > > > +
> > > > +      '#size-cells':
> > > > +        const: 0
> > > > +
> > > > +    patternProperties:
> > > > +      '^string@[0-9a-f]+$':
> > > > +        type: object
> > > > +        description: string descriptors of the specific language
> > > > +
> > > > +        properties:
> > > > +          reg:
> > > > +            maxItems: 1
> > > > +            description: 16-bit Language Identifier defined by USB-IF
> > > > +
> > > > +          manufacturer:
> > > > +            description: vhub manufacturer
> > > > +            allOf:
> > > > +              - $ref: /schemas/types.yaml#/definitions/string
> > > > +
> > > > +          product:
> > > > +            description: vhub product name
> > > > +            allOf:
> > > > +              - $ref: /schemas/types.yaml#/definitions/string
> > > > +
> > > > +          serial-number:
> > > > +            description: vhub device serial number
> > > > +            allOf:
> > > > +              - $ref: /schemas/types.yaml#/definitions/string
> > >
> > > For all of this, it's USB specific, not vhub specific. I'm not sure this
> > > is the right approach. It might be better to just define properties
> > > which are just raw USB descriptors rather than inventing some DT format
> > > that then has to be converted into USB descriptors.
> >
> > Raw blob in the DT is rather annoying and leads to hard to parse stuff
> > for both humans and scripts. The main strenght of the DT is it's easy
> > to read and manipulate.
> 
> True, and I'd certainly agree when we're talking about some vendor
> specific blob. but there's already code/tools to parse USB
> descriptors.
> 
> > Also not the entire descriptor is configurable this way.
> >
> > That said, it could be that using  the DT for the above is overkill and
> > instead, we should consider a configfs like the rest of USB gadget.
> > Though it isn't obvious how to do that, the current gadget stuff
> > doesn't really "fit" what we need here.
> 
> Surely the descriptor building code can be shared at a minimum.
> 
> Regardless of whether gadget configfs fits, usually it is pretty clear
> whether something belongs in DT or userspace. That should be decided
> first.
> 
> > Maybe we could expose the port as UDCs but not actually expose them on
> > the bus until the hub is "activated" via a special configfs entry...
> 
> If control of the hub is done by userspace, I'd think configuration
> should be there too.
> 
> Rob

Perhaps it's my lack of greater knowledge in gadget/dt areas, and I'm not
quite clear what would be the recommended/accepted approach for this
case. I'm looking forward for more suggestions.


Cheers,

Tao

Re: [PATCH v2 6/6] dt-bindings: usb: document aspeed vhub device ID/string properties

[Benjamin Herrenschmidt]

On Tue, 2020-03-31 at 10:21 -0600, Rob Herring wrote:
> Surely the descriptor building code can be shared at a minimum.
> 
> Regardless of whether gadget configfs fits, usually it is pretty clear
> whether something belongs in DT or userspace. That should be decided
> first.
> 
> > Maybe we could expose the port as UDCs but not actually expose them on
> > the bus until the hub is "activated" via a special configfs entry...
> 
> If control of the hub is done by userspace, I'd think configuration
> should be there too.

It's not in the current driver. For now, I expose the hub when the
driver loads/initializes, and it creates UDCs for each port, which are
then controlled from userspace.

That said, I did it this way because it was easy, not because there are
fundamental reasons to do so...

The main reason to want to change the hub descriptor is for the device
to advertise a vendor/device ID rather than our generic linux one,
which some vendors might want to do for ... reasons. I didn't implement
that functionality initially as in openbmc case, we didn't care. But I
know some vendors would like to, if anything because from a user
perspective, it's actually nice to have the string tell you that it's
your BMC rather than Linux Fundation Hub.

Originally I suggested we allow that via the device-tree because it was
the simplest way to get there and I love have to deal with less code ..
:)

However, if we want to support the whole language string set etc... it
gets really clumsy. So if there's a strong will to get there all the
way, then configfs is probably the way to go.

In that case, some sugery will probably be needed to make the gadget
descriptor building code a bit less dependent on the overall gadget
stuff... either that, or pre-create a "hub" gadget at driver loading
time that userspace can modify before "plugging".

In that case, the discussion should move back to linux-usb...

Cheers,
Ben.