From: Tao Ren <[email protected]>
This patch series allows people to customize IDs/strings in vhub's
device descriptor through device tree properties.
Patch #1 overrides IDs and strings in device descriptor if according
device tree properties are defined.
Patch #2 moves fixup-usb1-device-descriptor logic from get_descriptor
handler to "ast_vhub_fixup_dev_desc" function for consistency.
The patch series needs to be applied on top of "aspeed-g6: enable usb
support" patch set to avoid merge conflicts.
Tao Ren (2):
usb: gadget: aspeed: allow to customize vhub device IDs/strings
usb: gadget: aspeed: fixup usb1 device descriptor at init time
drivers/usb/gadget/udc/aspeed-vhub/hub.c | 93 +++++++++++++++++-------
1 file changed, 68 insertions(+), 25 deletions(-)
--
2.17.1
From: Tao Ren <[email protected]>
This patch allows people to customize vendor/product/device IDs and
manufacture/product/serial strings in vhub's device descriptor through
device tree properties.
Signed-off-by: Tao Ren <[email protected]>
---
drivers/usb/gadget/udc/aspeed-vhub/hub.c | 73 +++++++++++++++++++-----
1 file changed, 59 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index 9c7e57fbd8ef..4e3ef83283a6 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -43,19 +43,23 @@
* - We may need to indicate TT support
* - We may need a device qualifier descriptor
* as devices can pretend to be usb1 or 2
- * - Make vid/did overridable
* - make it look like usb1 if usb1 mode forced
*/
#define KERNEL_REL bin2bcd(((LINUX_VERSION_CODE >> 16) & 0x0ff))
#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,
};
-static const struct usb_device_descriptor ast_vhub_dev_desc = {
+/*
+ * Below is the default Device Descriptor of the vhub device. Some fields
+ * may be updated in "ast_vhub_fixup_dev_desc" function.
+ */
+static struct usb_device_descriptor ast_vhub_dev_desc = {
.bLength = USB_DT_DEVICE_SIZE,
.bDescriptorType = USB_DT_DEVICE,
.bcdUSB = cpu_to_le16(0x0200),
@@ -148,10 +152,14 @@ static struct usb_hub_descriptor ast_vhub_hub_desc = {
};
/*
- * These strings converted to UTF-16 must be smaller than
- * our EP0 buffer.
+ * Below tables define the default Language ID and String Descriptors of
+ * the vhub. Language ID and strings may be overridden if according device
+ * tree properties are defined. Refer to "ast_vhub_fixup_dev_desc" function
+ * for details.
+ * Note: these strings converted to UTF-16 must be smaller than vhub EP0
+ * buffer size.
*/
-static const struct usb_string ast_vhub_str_array[] = {
+static struct usb_string ast_vhub_str_array[] = {
{
.id = AST_VHUB_STR_SERIAL,
.s = "00000000"
@@ -167,7 +175,7 @@ static const struct usb_string ast_vhub_str_array[] = {
{ }
};
-static const struct usb_gadget_strings ast_vhub_strings = {
+static struct usb_gadget_strings ast_vhub_strings = {
.language = 0x0409,
.strings = (struct usb_string *)ast_vhub_str_array
};
@@ -320,18 +328,15 @@ 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 (&ast_vhub_strings, string_id, ep->buf);
-
- /*
- * This should never happen unless we put too big strings in
- * the array above
- */
- BUG_ON(rc >= AST_VHUB_EP0_MAX_PACKET);
+ int rc;
+ u8 buf[256]; /* buffer size required by usb_gadget_get_string */
- if (rc < 0)
+ rc = usb_gadget_get_string(&ast_vhub_strings, string_id, buf);
+ 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));
}
@@ -837,11 +842,51 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
writel(0, vhub->regs + AST_VHUB_EP1_STS_CHG);
}
+static void ast_vhub_fixup_dev_desc(struct ast_vhub *vhub)
+{
+ int i;
+ u8 id;
+ u16 of_id;
+ const char *of_str[AST_VHUB_STR_INDEX_MAX] = {NULL};
+ struct device_node *np = vhub->pdev->dev.of_node;
+
+ /*
+ * Update IDs in device descriptor if according properties are
+ * defined in device tree.
+ */
+ if (!of_property_read_u16(np, "vendor-id", &of_id))
+ ast_vhub_dev_desc.idVendor = cpu_to_le16(of_id);
+ if (!of_property_read_u16(np, "product-id", &of_id))
+ ast_vhub_dev_desc.idProduct = cpu_to_le16(of_id);
+ if (!of_property_read_u16(np, "device-id", &of_id))
+ ast_vhub_dev_desc.bcdDevice = cpu_to_le16(of_id);
+
+ /*
+ * Update string descriptors if according properties are defined
+ * in device tree.
+ */
+ if (!of_property_read_u16(np, "language-id", &of_id))
+ ast_vhub_strings.language = of_id;
+
+ of_str[AST_VHUB_STR_MANUF] = of_get_property(np, "manufacturer", NULL);
+ of_str[AST_VHUB_STR_PRODUCT] = of_get_property(np, "product", NULL);
+ of_str[AST_VHUB_STR_SERIAL] = of_get_property(np, "serial-number",
+ NULL);
+
+ for (i = 0; ast_vhub_str_array[i].s != NULL; i++) {
+ id = ast_vhub_str_array[i].id;
+ if (of_str[id])
+ ast_vhub_str_array[i].s = of_str[id];
+ }
+}
+
void ast_vhub_init_hub(struct ast_vhub *vhub)
{
vhub->speed = USB_SPEED_UNKNOWN;
INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
+ ast_vhub_fixup_dev_desc(vhub);
+
/*
* Fixup number of ports in hub descriptor.
*/
--
2.17.1
From: Tao Ren <[email protected]>
This patch moves fixup-usb1-device-descriptor logic from get_descriptor
handler to "ast_vhub_fixup_dev_desc" function so the device descriptor
is only patched once (at vhub init time).
Signed-off-by: Tao Ren <[email protected]>
---
drivers/usb/gadget/udc/aspeed-vhub/hub.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index 4e3ef83283a6..b8bf54b12adc 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -76,13 +76,6 @@ static 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.
@@ -316,10 +309,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);
}
@@ -878,6 +867,15 @@ static void ast_vhub_fixup_dev_desc(struct ast_vhub *vhub)
if (of_str[id])
ast_vhub_str_array[i].s = of_str[id];
}
+
+ /*
+ * Update USB Release Number and Protocol code if vhub is running
+ * at USB 1.x speed.
+ */
+ if (vhub->force_usb1) {
+ ast_vhub_dev_desc.bcdUSB = cpu_to_le16(0x0100);
+ ast_vhub_dev_desc.bDeviceProtocol = 0;
+ }
}
void ast_vhub_init_hub(struct ast_vhub *vhub)
--
2.17.1
On Tue, 2020-02-18 at 15:55 -0800, [email protected] wrote:
> From: Tao Ren <[email protected]>
>
> This patch allows people to customize vendor/product/device IDs and
> manufacture/product/serial strings in vhub's device descriptor through
> device tree properties.
You should probably add a binding file to Documentation/devicetree/bindings/usb/*
We got away without one bcs there was no funky properties there but
now that we are adding some, we need to document them.
Also...
> Signed-off-by: Tao Ren <[email protected]>
> ---
> drivers/usb/gadget/udc/aspeed-vhub/hub.c | 73 +++++++++++++++++++-----
> 1 file changed, 59 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> index 9c7e57fbd8ef..4e3ef83283a6 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> @@ -43,19 +43,23 @@
> * - We may need to indicate TT support
> * - We may need a device qualifier descriptor
> * as devices can pretend to be usb1 or 2
> - * - Make vid/did overridable
> * - make it look like usb1 if usb1 mode forced
> */
> #define KERNEL_REL bin2bcd(((LINUX_VERSION_CODE >> 16) & 0x0ff))
> #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,
> };
>
> -static const struct usb_device_descriptor ast_vhub_dev_desc = {
> +/*
> + * Below is the default Device Descriptor of the vhub device. Some fields
> + * may be updated in "ast_vhub_fixup_dev_desc" function.
> + */
> +static struct usb_device_descriptor ast_vhub_dev_desc = {
> .bLength = USB_DT_DEVICE_SIZE,
> .bDescriptorType = USB_DT_DEVICE,
> .bcdUSB = cpu_to_le16(0x0200),
> @@ -148,10 +152,14 @@ static struct usb_hub_descriptor ast_vhub_hub_desc = {
> };
>
> /*
> - * These strings converted to UTF-16 must be smaller than
> - * our EP0 buffer.
> + * Below tables define the default Language ID and String Descriptors of
> + * the vhub. Language ID and strings may be overridden if according device
> + * tree properties are defined. Refer to "ast_vhub_fixup_dev_desc" function
> + * for details.
> + * Note: these strings converted to UTF-16 must be smaller than vhub EP0
> + * buffer size.
> */
> -static const struct usb_string ast_vhub_str_array[] = {
> +static struct usb_string ast_vhub_str_array[] = {
> {
> .id = AST_VHUB_STR_SERIAL,
> .s = "00000000"
> @@ -167,7 +175,7 @@ static const struct usb_string ast_vhub_str_array[] = {
> { }
> };
I dislike this. The array should remain static and contain the
defaults. The properties shouldn't modify the global array, there could
be a future chip with multiple vhubs and that would make them stomp on
each other.
Instead, duplicate the properties into the per-vhub instance data and
update the content there.
You could also skip using usb_gadget_get_string() and expose the low
level conversion function directly though that's trickier.
Also have you thought about supporting a list of strings along with an
array of language IDs ? Vendors might want to provide multiple
languages...
> -static const struct usb_gadget_strings ast_vhub_strings = {
> +static struct usb_gadget_strings ast_vhub_strings = {
> .language = 0x0409,
> .strings = (struct usb_string *)ast_vhub_str_array
> };
> @@ -320,18 +328,15 @@ 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 (&ast_vhub_strings, string_id, ep->buf);
> -
> - /*
> - * This should never happen unless we put too big strings in
> - * the array above
> - */
> - BUG_ON(rc >= AST_VHUB_EP0_MAX_PACKET);
> + int rc;
> + u8 buf[256]; /* buffer size required by usb_gadget_get_string */
>
> - if (rc < 0)
> + rc = usb_gadget_get_string(&ast_vhub_strings, string_id, buf);
> + 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));
> }
>
> @@ -837,11 +842,51 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> writel(0, vhub->regs + AST_VHUB_EP1_STS_CHG);
> }
>
> +static void ast_vhub_fixup_dev_desc(struct ast_vhub *vhub)
> +{
> + int i;
> + u8 id;
> + u16 of_id;
> + const char *of_str[AST_VHUB_STR_INDEX_MAX] = {NULL};
> + struct device_node *np = vhub->pdev->dev.of_node;
> +
> + /*
> + * Update IDs in device descriptor if according properties are
> + * defined in device tree.
> + */
> + if (!of_property_read_u16(np, "vendor-id", &of_id))
> + ast_vhub_dev_desc.idVendor = cpu_to_le16(of_id);
> + if (!of_property_read_u16(np, "product-id", &of_id))
> + ast_vhub_dev_desc.idProduct = cpu_to_le16(of_id);
> + if (!of_property_read_u16(np, "device-id", &of_id))
> + ast_vhub_dev_desc.bcdDevice = cpu_to_le16(of_id);
> +
> + /*
> + * Update string descriptors if according properties are defined
> + * in device tree.
> + */
> + if (!of_property_read_u16(np, "language-id", &of_id))
> + ast_vhub_strings.language = of_id;
> +
> + of_str[AST_VHUB_STR_MANUF] = of_get_property(np, "manufacturer", NULL);
> + of_str[AST_VHUB_STR_PRODUCT] = of_get_property(np, "product", NULL);
> + of_str[AST_VHUB_STR_SERIAL] = of_get_property(np, "serial-number",
> + NULL);
> +
> + for (i = 0; ast_vhub_str_array[i].s != NULL; i++) {
> + id = ast_vhub_str_array[i].id;
> + if (of_str[id])
> + ast_vhub_str_array[i].s = of_str[id];
> + }
> +}
> +
> void ast_vhub_init_hub(struct ast_vhub *vhub)
> {
> vhub->speed = USB_SPEED_UNKNOWN;
> INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
>
> + ast_vhub_fixup_dev_desc(vhub);
> +
> /*
> * Fixup number of ports in hub descriptor.
> */
On Tue, 2020-02-18 at 15:56 -0800, [email protected] wrote:
> From: Tao Ren <[email protected]>
>
> This patch moves fixup-usb1-device-descriptor logic from get_descriptor
> handler to "ast_vhub_fixup_dev_desc" function so the device descriptor
> is only patched once (at vhub init time).
I don't like this either. We should make ast_vhub_dev_desc and patch a
copy here too. I know today there's only one instance of the vhub in a
given SoC but that might not always be the case.
> Signed-off-by: Tao Ren <[email protected]>
> ---
> drivers/usb/gadget/udc/aspeed-vhub/hub.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> index 4e3ef83283a6..b8bf54b12adc 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> @@ -76,13 +76,6 @@ static 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.
> @@ -316,10 +309,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);
> }
> @@ -878,6 +867,15 @@ static void ast_vhub_fixup_dev_desc(struct ast_vhub *vhub)
> if (of_str[id])
> ast_vhub_str_array[i].s = of_str[id];
> }
> +
> + /*
> + * Update USB Release Number and Protocol code if vhub is running
> + * at USB 1.x speed.
> + */
> + if (vhub->force_usb1) {
> + ast_vhub_dev_desc.bcdUSB = cpu_to_le16(0x0100);
> + ast_vhub_dev_desc.bDeviceProtocol = 0;
> + }
> }
>
> void ast_vhub_init_hub(struct ast_vhub *vhub)
Hi Ben,
On Thu, Feb 20, 2020 at 12:38:10PM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2020-02-18 at 15:55 -0800, [email protected] wrote:
> > From: Tao Ren <[email protected]>
> >
> > This patch allows people to customize vendor/product/device IDs and
> > manufacture/product/serial strings in vhub's device descriptor through
> > device tree properties.
>
> You should probably add a binding file to Documentation/devicetree/bindings/usb/*
>
> We got away without one bcs there was no funky properties there but
> now that we are adding some, we need to document them.
Sure. Andrew also reminded me about the binding document. Will include
the document in patch v2.
>
> Also...
>
> > Signed-off-by: Tao Ren <[email protected]>
> > ---
> > drivers/usb/gadget/udc/aspeed-vhub/hub.c | 73 +++++++++++++++++++-----
> > 1 file changed, 59 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > index 9c7e57fbd8ef..4e3ef83283a6 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > @@ -43,19 +43,23 @@
> > * - We may need to indicate TT support
> > * - We may need a device qualifier descriptor
> > * as devices can pretend to be usb1 or 2
> > - * - Make vid/did overridable
> > * - make it look like usb1 if usb1 mode forced
> > */
> > #define KERNEL_REL bin2bcd(((LINUX_VERSION_CODE >> 16) & 0x0ff))
> > #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,
> > };
> >
> > -static const struct usb_device_descriptor ast_vhub_dev_desc = {
> > +/*
> > + * Below is the default Device Descriptor of the vhub device. Some fields
> > + * may be updated in "ast_vhub_fixup_dev_desc" function.
> > + */
> > +static struct usb_device_descriptor ast_vhub_dev_desc = {
> > .bLength = USB_DT_DEVICE_SIZE,
> > .bDescriptorType = USB_DT_DEVICE,
> > .bcdUSB = cpu_to_le16(0x0200),
> > @@ -148,10 +152,14 @@ static struct usb_hub_descriptor ast_vhub_hub_desc = {
> > };
> >
> > /*
> > - * These strings converted to UTF-16 must be smaller than
> > - * our EP0 buffer.
> > + * Below tables define the default Language ID and String Descriptors of
> > + * the vhub. Language ID and strings may be overridden if according device
> > + * tree properties are defined. Refer to "ast_vhub_fixup_dev_desc" function
> > + * for details.
> > + * Note: these strings converted to UTF-16 must be smaller than vhub EP0
> > + * buffer size.
> > */
> > -static const struct usb_string ast_vhub_str_array[] = {
> > +static struct usb_string ast_vhub_str_array[] = {
> > {
> > .id = AST_VHUB_STR_SERIAL,
> > .s = "00000000"
> > @@ -167,7 +175,7 @@ static const struct usb_string ast_vhub_str_array[] = {
> > { }
> > };
>
> I dislike this. The array should remain static and contain the
> defaults. The properties shouldn't modify the global array, there could
> be a future chip with multiple vhubs and that would make them stomp on
> each other.
>
> Instead, duplicate the properties into the per-vhub instance data and
> update the content there.
Okay. I will include a copy of the descriptors in struct ast_vhub and
override per-hub instances if needed.
>
> You could also skip using usb_gadget_get_string() and expose the low
> level conversion function directly though that's trickier.
>
> Also have you thought about supporting a list of strings along with an
> array of language IDs ? Vendors might want to provide multiple
> languages...
I thought people (aspeed bmc users) won't care much about multi-language
usb strings in the near future. Maybe I'm wrong, but if this is what we
want for now, I will try to add the support, but will need more guidance
from you (such as device tree structure, dt property value to utf-16
conversion, and etc.).
Cheers,
Tao
>
> > -static const struct usb_gadget_strings ast_vhub_strings = {
> > +static struct usb_gadget_strings ast_vhub_strings = {
> > .language = 0x0409,
> > .strings = (struct usb_string *)ast_vhub_str_array
> > };
> > @@ -320,18 +328,15 @@ 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 (&ast_vhub_strings, string_id, ep->buf);
> > -
> > - /*
> > - * This should never happen unless we put too big strings in
> > - * the array above
> > - */
> > - BUG_ON(rc >= AST_VHUB_EP0_MAX_PACKET);
> > + int rc;
> > + u8 buf[256]; /* buffer size required by usb_gadget_get_string */
> >
> > - if (rc < 0)
> > + rc = usb_gadget_get_string(&ast_vhub_strings, string_id, buf);
> > + 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));
> > }
> >
> > @@ -837,11 +842,51 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> > writel(0, vhub->regs + AST_VHUB_EP1_STS_CHG);
> > }
> >
> > +static void ast_vhub_fixup_dev_desc(struct ast_vhub *vhub)
> > +{
> > + int i;
> > + u8 id;
> > + u16 of_id;
> > + const char *of_str[AST_VHUB_STR_INDEX_MAX] = {NULL};
> > + struct device_node *np = vhub->pdev->dev.of_node;
> > +
> > + /*
> > + * Update IDs in device descriptor if according properties are
> > + * defined in device tree.
> > + */
> > + if (!of_property_read_u16(np, "vendor-id", &of_id))
> > + ast_vhub_dev_desc.idVendor = cpu_to_le16(of_id);
> > + if (!of_property_read_u16(np, "product-id", &of_id))
> > + ast_vhub_dev_desc.idProduct = cpu_to_le16(of_id);
> > + if (!of_property_read_u16(np, "device-id", &of_id))
> > + ast_vhub_dev_desc.bcdDevice = cpu_to_le16(of_id);
> > +
> > + /*
> > + * Update string descriptors if according properties are defined
> > + * in device tree.
> > + */
> > + if (!of_property_read_u16(np, "language-id", &of_id))
> > + ast_vhub_strings.language = of_id;
> > +
> > + of_str[AST_VHUB_STR_MANUF] = of_get_property(np, "manufacturer", NULL);
> > + of_str[AST_VHUB_STR_PRODUCT] = of_get_property(np, "product", NULL);
> > + of_str[AST_VHUB_STR_SERIAL] = of_get_property(np, "serial-number",
> > + NULL);
> > +
> > + for (i = 0; ast_vhub_str_array[i].s != NULL; i++) {
> > + id = ast_vhub_str_array[i].id;
> > + if (of_str[id])
> > + ast_vhub_str_array[i].s = of_str[id];
> > + }
> > +}
> > +
> > void ast_vhub_init_hub(struct ast_vhub *vhub)
> > {
> > vhub->speed = USB_SPEED_UNKNOWN;
> > INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
> >
> > + ast_vhub_fixup_dev_desc(vhub);
> > +
> > /*
> > * Fixup number of ports in hub descriptor.
> > */
>
On Thu, Feb 20, 2020 at 12:39:45PM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2020-02-18 at 15:56 -0800, [email protected] wrote:
> > From: Tao Ren <[email protected]>
> >
> > This patch moves fixup-usb1-device-descriptor logic from get_descriptor
> > handler to "ast_vhub_fixup_dev_desc" function so the device descriptor
> > is only patched once (at vhub init time).
>
> I don't like this either. We should make ast_vhub_dev_desc and patch a
> copy here too. I know today there's only one instance of the vhub in a
> given SoC but that might not always be the case.
Sure. I will introduce per-hub descripor instances in patch v2.
Cheers,
Tao
>
> > Signed-off-by: Tao Ren <[email protected]>
> > ---
> > drivers/usb/gadget/udc/aspeed-vhub/hub.c | 20 +++++++++-----------
> > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > index 4e3ef83283a6..b8bf54b12adc 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > @@ -76,13 +76,6 @@ static 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.
> > @@ -316,10 +309,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);
> > }
> > @@ -878,6 +867,15 @@ static void ast_vhub_fixup_dev_desc(struct ast_vhub *vhub)
> > if (of_str[id])
> > ast_vhub_str_array[i].s = of_str[id];
> > }
> > +
> > + /*
> > + * Update USB Release Number and Protocol code if vhub is running
> > + * at USB 1.x speed.
> > + */
> > + if (vhub->force_usb1) {
> > + ast_vhub_dev_desc.bcdUSB = cpu_to_le16(0x0100);
> > + ast_vhub_dev_desc.bDeviceProtocol = 0;
> > + }
> > }
> >
> > void ast_vhub_init_hub(struct ast_vhub *vhub)
>