[U-Boot] [PATCH 0/8] usb: driver-model fixes and dm support sunxi-ehci.c

[U-Boot] [PATCH 0/8] usb: driver-model fixes and dm support sunxi-ehci.c

[Hans de Goede]

Hi Simon and Marek,

This series completes my work on converting the sunxi usb/ehci code to
the driver model. With this series everything works as it did before,
and all the issues I've been seeing when switching to the driver model
are resolved.

Please review / merge. I think it would be best to merge this through
the dm tree.

Regards,

Hans

[U-Boot] [PATCH 1/8] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Hans de Goede]

Currently we copy over a number of usb_device values stored in the on stack
struct usb_device probed in usb_scan_device() to the final driver-model managed
struct usb_device in usb_child_pre_probe() through usb_device_platdata, and
then call usb_select_config() to fill in the rest.

There are 2 problems with this approach:

1) It does not fill in enough fields before calling usb_select_config(),
specifically it does not fill in ep0's maxpacketsize causing a div by zero
exception in the ehci driver.

2) It unnecessarily redoes a number of usb requests making usb probing slower,
and potentially upsetting some devices.

This commit fixes both issues by removing the usb_select_config() call from
usb_child_pre_probe(), and instead of copying over things field by field
through usb_device_platdata, store a pointer to the in stack usb_device
(which is still valid when usb_child_pre_probe() gets called) and copy
over the entire struct.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/usb-uclass.c | 27 ++++++---------------------
 include/usb.h                 |  5 +----
 2 files changed, 7 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 714bc0e..95e371d 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -535,12 +535,7 @@ int usb_scan_device(struct udevice *parent, int port,
 	}
 	plat = dev_get_parent_platdata(dev);
 	debug("%s: Probing '%s', plat=%p\n", __func__, dev->name, plat);
-	plat->devnum = udev->devnum;
-	plat->speed = udev->speed;
-	plat->slot_id = udev->slot_id;
-	plat->portnr = port;
-	debug("** device '%s': stashing slot_id=%d\n", dev->name,
-	      plat->slot_id);
+	plat->udev = udev;
 	priv->next_addr++;
 	ret = device_probe(dev);
 	if (ret) {
@@ -599,25 +594,15 @@ int usb_get_bus(struct udevice *dev, struct udevice **busp)
 
 int usb_child_pre_probe(struct udevice *dev)
 {
-	struct udevice *bus;
 	struct usb_device *udev = dev_get_parentdata(dev);
 	struct usb_dev_platdata *plat = dev_get_parent_platdata(dev);
-	int ret;
 
-	ret = usb_get_bus(dev, &bus);
-	if (ret)
-		return ret;
-	udev->controller_dev = bus;
+	/*
+	 * Copy over all the values set in the on stack struct usb_device in
+	 * usb_scan_device() to our final struct usb_device for this dev.
+	 */
+	*udev = *(plat->udev);
 	udev->dev = dev;
-	udev->devnum = plat->devnum;
-	udev->slot_id = plat->slot_id;
-	udev->portnr = plat->portnr;
-	udev->speed = plat->speed;
-	debug("** device '%s': getting slot_id=%d\n", dev->name, plat->slot_id);
-
-	ret = usb_select_config(udev);
-	if (ret)
-		return ret;
 
 	return 0;
 }
diff --git a/include/usb.h b/include/usb.h
index 1984e8f..1b667ff 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -581,10 +581,7 @@ struct usb_platdata {
  */
 struct usb_dev_platdata {
 	struct usb_device_id id;
-	enum usb_device_speed speed;
-	int devnum;
-	int slot_id;
-	int portnr;	/* Hub port number, 1..n */
+	struct usb_device *udev;
 #ifdef CONFIG_SANDBOX
 	struct usb_string *strings;
 	/* NULL-terminated list of descriptor pointers */
-- 
2.3.6

[U-Boot] [PATCH 1/8] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Simon Glass]

Hi Hans,

On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
> Currently we copy over a number of usb_device values stored in the on stack
> struct usb_device probed in usb_scan_device() to the final driver-model managed
> struct usb_device in usb_child_pre_probe() through usb_device_platdata, and
> then call usb_select_config() to fill in the rest.
>
> There are 2 problems with this approach:
>
> 1) It does not fill in enough fields before calling usb_select_config(),
> specifically it does not fill in ep0's maxpacketsize causing a div by zero
> exception in the ehci driver.

Didn't you have another patch that fixes that?

>
> 2) It unnecessarily redoes a number of usb requests making usb probing slower,
> and potentially upsetting some devices.

Does it actually upset anything?

The extra requests are in the second call to usb_select_config().

Do you think we could put the things we want to copy in a struct, and
copy just those?

>
> This commit fixes both issues by removing the usb_select_config() call from
> usb_child_pre_probe(), and instead of copying over things field by field
> through usb_device_platdata, store a pointer to the in stack usb_device
> (which is still valid when usb_child_pre_probe() gets called) and copy
> over the entire struct.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/usb-uclass.c | 27 ++++++---------------------
>  include/usb.h                 |  5 +----
>  2 files changed, 7 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index 714bc0e..95e371d 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -535,12 +535,7 @@ int usb_scan_device(struct udevice *parent, int port,
>         }
>         plat = dev_get_parent_platdata(dev);
>         debug("%s: Probing '%s', plat=%p\n", __func__, dev->name, plat);
> -       plat->devnum = udev->devnum;
> -       plat->speed = udev->speed;
> -       plat->slot_id = udev->slot_id;
> -       plat->portnr = port;
> -       debug("** device '%s': stashing slot_id=%d\n", dev->name,
> -             plat->slot_id);
> +       plat->udev = udev;
>         priv->next_addr++;
>         ret = device_probe(dev);
>         if (ret) {
> @@ -599,25 +594,15 @@ int usb_get_bus(struct udevice *dev, struct udevice **busp)
>
>  int usb_child_pre_probe(struct udevice *dev)
>  {
> -       struct udevice *bus;
>         struct usb_device *udev = dev_get_parentdata(dev);
>         struct usb_dev_platdata *plat = dev_get_parent_platdata(dev);
> -       int ret;
>
> -       ret = usb_get_bus(dev, &bus);
> -       if (ret)
> -               return ret;
> -       udev->controller_dev = bus;
> +       /*
> +        * Copy over all the values set in the on stack struct usb_device in
> +        * usb_scan_device() to our final struct usb_device for this dev.
> +        */
> +       *udev = *(plat->udev);
>         udev->dev = dev;
> -       udev->devnum = plat->devnum;
> -       udev->slot_id = plat->slot_id;
> -       udev->portnr = plat->portnr;
> -       udev->speed = plat->speed;
> -       debug("** device '%s': getting slot_id=%d\n", dev->name, plat->slot_id);
> -
> -       ret = usb_select_config(udev);
> -       if (ret)
> -               return ret;
>
>         return 0;
>  }
> diff --git a/include/usb.h b/include/usb.h
> index 1984e8f..1b667ff 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -581,10 +581,7 @@ struct usb_platdata {
>   */
>  struct usb_dev_platdata {
>         struct usb_device_id id;
> -       enum usb_device_speed speed;
> -       int devnum;
> -       int slot_id;
> -       int portnr;     /* Hub port number, 1..n */
> +       struct usb_device *udev;
>  #ifdef CONFIG_SANDBOX
>         struct usb_string *strings;
>         /* NULL-terminated list of descriptor pointers */
> --
> 2.3.6
>

Regards,
Simon

[U-Boot] [PATCH 1/8] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Hans de Goede]

Hi,

On 01-05-15 06:11, Simon Glass wrote:
> Hi Hans,
>
> On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
>> Currently we copy over a number of usb_device values stored in the on stack
>> struct usb_device probed in usb_scan_device() to the final driver-model managed
>> struct usb_device in usb_child_pre_probe() through usb_device_platdata, and
>> then call usb_select_config() to fill in the rest.
>>
>> There are 2 problems with this approach:
>>
>> 1) It does not fill in enough fields before calling usb_select_config(),
>> specifically it does not fill in ep0's maxpacketsize causing a div by zero
>> exception in the ehci driver.
>
> Didn't you have another patch that fixes that?

Yes, but as explained in the coverletter of that patch I believe that
this version is better. It would seem we disagree on that though :)

>> 2) It unnecessarily redoes a number of usb requests making usb probing slower,
>> and potentially upsetting some devices.
>
> Does it actually upset anything?

Not to my knowledge, but I'm afraid that with some devices it may.

> The extra requests are in the second call to usb_select_config().

Correct.

> Do you think we could put the things we want to copy in a struct, and
> copy just those?

We would end up pretty much duplicating usb_device AFAICT, since we need
the descriptors, the max packet sizes per endpoint parsed from them, etc.

Anyways since you clearly dislike this patch I'll drop it for v2, replacing
it with my original fix for the ep0 maxpacket not being set issue, assuming
that doing so does not cause any regressions during my testing.

Regards,

Hans


>
>>
>> This commit fixes both issues by removing the usb_select_config() call from
>> usb_child_pre_probe(), and instead of copying over things field by field
>> through usb_device_platdata, store a pointer to the in stack usb_device
>> (which is still valid when usb_child_pre_probe() gets called) and copy
>> over the entire struct.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/host/usb-uclass.c | 27 ++++++---------------------
>>   include/usb.h                 |  5 +----
>>   2 files changed, 7 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>> index 714bc0e..95e371d 100644
>> --- a/drivers/usb/host/usb-uclass.c
>> +++ b/drivers/usb/host/usb-uclass.c
>> @@ -535,12 +535,7 @@ int usb_scan_device(struct udevice *parent, int port,
>>          }
>>          plat = dev_get_parent_platdata(dev);
>>          debug("%s: Probing '%s', plat=%p\n", __func__, dev->name, plat);
>> -       plat->devnum = udev->devnum;
>> -       plat->speed = udev->speed;
>> -       plat->slot_id = udev->slot_id;
>> -       plat->portnr = port;
>> -       debug("** device '%s': stashing slot_id=%d\n", dev->name,
>> -             plat->slot_id);
>> +       plat->udev = udev;
>>          priv->next_addr++;
>>          ret = device_probe(dev);
>>          if (ret) {
>> @@ -599,25 +594,15 @@ int usb_get_bus(struct udevice *dev, struct udevice **busp)
>>
>>   int usb_child_pre_probe(struct udevice *dev)
>>   {
>> -       struct udevice *bus;
>>          struct usb_device *udev = dev_get_parentdata(dev);
>>          struct usb_dev_platdata *plat = dev_get_parent_platdata(dev);
>> -       int ret;
>>
>> -       ret = usb_get_bus(dev, &bus);
>> -       if (ret)
>> -               return ret;
>> -       udev->controller_dev = bus;
>> +       /*
>> +        * Copy over all the values set in the on stack struct usb_device in
>> +        * usb_scan_device() to our final struct usb_device for this dev.
>> +        */
>> +       *udev = *(plat->udev);
>>          udev->dev = dev;
>> -       udev->devnum = plat->devnum;
>> -       udev->slot_id = plat->slot_id;
>> -       udev->portnr = plat->portnr;
>> -       udev->speed = plat->speed;
>> -       debug("** device '%s': getting slot_id=%d\n", dev->name, plat->slot_id);
>> -
>> -       ret = usb_select_config(udev);
>> -       if (ret)
>> -               return ret;
>>
>>          return 0;
>>   }
>> diff --git a/include/usb.h b/include/usb.h
>> index 1984e8f..1b667ff 100644
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> @@ -581,10 +581,7 @@ struct usb_platdata {
>>    */
>>   struct usb_dev_platdata {
>>          struct usb_device_id id;
>> -       enum usb_device_speed speed;
>> -       int devnum;
>> -       int slot_id;
>> -       int portnr;     /* Hub port number, 1..n */
>> +       struct usb_device *udev;
>>   #ifdef CONFIG_SANDBOX
>>          struct usb_string *strings;
>>          /* NULL-terminated list of descriptor pointers */
>> --
>> 2.3.6
>>
>
> Regards,
> Simon
>

[U-Boot] [PATCH 1/8] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Hans de Goede]

Hi,

On 01-05-15 09:21, Hans de Goede wrote:
> Hi,
>
> On 01-05-15 06:11, Simon Glass wrote:
>> Hi Hans,
>>
>> On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Currently we copy over a number of usb_device values stored in the on stack
>>> struct usb_device probed in usb_scan_device() to the final driver-model managed
>>> struct usb_device in usb_child_pre_probe() through usb_device_platdata, and
>>> then call usb_select_config() to fill in the rest.
>>>
>>> There are 2 problems with this approach:
>>>
>>> 1) It does not fill in enough fields before calling usb_select_config(),
>>> specifically it does not fill in ep0's maxpacketsize causing a div by zero
>>> exception in the ehci driver.
>>
>> Didn't you have another patch that fixes that?
>
> Yes, but as explained in the coverletter of that patch I believe that
> this version is better. It would seem we disagree on that though :)
>
>>> 2) It unnecessarily redoes a number of usb requests making usb probing slower,
>>> and potentially upsetting some devices.
>>
>> Does it actually upset anything?
>
> Not to my knowledge, but I'm afraid that with some devices it may.
>
>> The extra requests are in the second call to usb_select_config().
>
> Correct.
>
>> Do you think we could put the things we want to copy in a struct, and
>> copy just those?
>
> We would end up pretty much duplicating usb_device AFAICT, since we need
> the descriptors, the max packet sizes per endpoint parsed from them, etc.
>
> Anyways since you clearly dislike this patch I'll drop it for v2, replacing
> it with my original fix for the ep0 maxpacket not being set issue, assuming
> that doing so does not cause any regressions during my testing.

Ok, so my first test, which is hooking up a sunxi device to my dvi/usb kvm switch
fails immediately when I use the maxpacketsize fix instead of my fix to avoid
calling get_config twice. This is actually a pretty though test-case as the
kvm presents itself + the keyboard and mouse as a complex hub hierarchy with
both usb-2 and usb-1 hubs in there, which is what makes it a great test-case.

Now I could spend a couple of hours debugging this and maybe find a different
fix, but this really shows that there is a reason why all usb stacks do the
device probing / descriptor reading all in the exact same sequence, because
usb devices are cheap and there qa consists of plug it into $random windows
version running machine, does it work? Yes -> ship it.

So I really believe that my original fix for this is best. As for trying to
pass all the bits we need through platdata rather then passing the struct
usb_device itself. I can see value in that as part of a patch-set to get rid
of usb_device, iow as part of a larger patchset but until then it just feels
like make work to me.

So I'm going to stick with my original approach for v1 of this patch.

Regards,

Hans

[U-Boot] [PATCH 2/8] dm: usb: Use controller_dev in dm ehci code

[Hans de Goede]

Use the controller_dev pointer in the ehci hcd code rather then going up
the tree till we find the first UCLASS_USB device.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/ehci-hcd.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index bd9861d..19f1e29 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -125,14 +125,7 @@ static struct descriptor {
 static struct ehci_ctrl *ehci_get_ctrl(struct usb_device *udev)
 {
 #ifdef CONFIG_DM_USB
-	struct udevice *dev;
-
-	/* Find the USB controller */
-	for (dev = udev->dev;
-	     device_get_uclass_id(dev) != UCLASS_USB;
-	     dev = dev->parent)
-		;
-	return dev_get_priv(dev);
+	return dev_get_priv(udev->controller_dev);
 #else
 	return udev->controller;
 #endif
-- 
2.3.6

[U-Boot] [PATCH 2/8] dm: usb: Use controller_dev in dm ehci code

[Simon Glass]

Hi Hans,

On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
> Use the controller_dev pointer in the ehci hcd code rather then going up
> the tree till we find the first UCLASS_USB device.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/ehci-hcd.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index bd9861d..19f1e29 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -125,14 +125,7 @@ static struct descriptor {
>  static struct ehci_ctrl *ehci_get_ctrl(struct usb_device *udev)
>  {
>  #ifdef CONFIG_DM_USB
> -       struct udevice *dev;
> -
> -       /* Find the USB controller */
> -       for (dev = udev->dev;
> -            device_get_uclass_id(dev) != UCLASS_USB;
> -            dev = dev->parent)
> -               ;
> -       return dev_get_priv(dev);
> +       return dev_get_priv(udev->controller_dev);
>  #else
>         return udev->controller;
>  #endif

My intent was to remove udev->controller_dev, since I was hoping to
remove all pointers other than those maintained by driver model. Does
this patch actually fix a bug?

Regards,
Simon

[U-Boot] [PATCH 2/8] dm: usb: Use controller_dev in dm ehci code

[Hans de Goede]

Hi,

On 01-05-15 06:11, Simon Glass wrote:
> Hi Hans,
>
> On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
>> Use the controller_dev pointer in the ehci hcd code rather then going up
>> the tree till we find the first UCLASS_USB device.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/host/ehci-hcd.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> index bd9861d..19f1e29 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>> @@ -125,14 +125,7 @@ static struct descriptor {
>>   static struct ehci_ctrl *ehci_get_ctrl(struct usb_device *udev)
>>   {
>>   #ifdef CONFIG_DM_USB
>> -       struct udevice *dev;
>> -
>> -       /* Find the USB controller */
>> -       for (dev = udev->dev;
>> -            device_get_uclass_id(dev) != UCLASS_USB;
>> -            dev = dev->parent)
>> -               ;
>> -       return dev_get_priv(dev);
>> +       return dev_get_priv(udev->controller_dev);
>>   #else
>>          return udev->controller;
>>   #endif
>
> My intent was to remove udev->controller_dev, since I was hoping to
> remove all pointers other than those maintained by driver model. Does
> this patch actually fix a bug?

I initially wrote this because my plan was to set usb_device.dev to NULL
during the initial probing in usb_scan_device, as setting it to parent
at that point is somewhat bogus.

I dropped the setting of usb_device.dev to NULL later because I was
afraid it may cause regressions for other hcd code. But I kept this
as it seemed like a nice cleanup.

I still think this needs a bit of cleanup, if we keep doing the lookup
this way, at least it should use usb_get_bus rather then looping itself.

I'll put a patch for that in v2 of this set replacing this one.

Regards,

Hans

[U-Boot] [PATCH 3/8] dm: usb: Store usb_device parent pointer in usb_device

[Hans de Goede]

When calling into the hcd code in usb_scan_device() we do not yet have
the actual udevice for the device we are scanning, so we temporarily set
usb_device.dev to the parent.

This means that we cannot use usb_device.dev to accurately determine our
place in the usb topology when reading descriptors, and this is necessary
to correctly program the usb-2 hub tt settings when a usb-1 device is
connected to a usb-2 hub.

This commit (re)adds a direct parent usb_device pointer to struct usb_device
so that the usb hcd code can get to the usb_device parent without needing to
go through the dm.

This fixes usb-1 devices plugged into usb-2 hubs not working with the driver
model.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/ehci-hcd.c   | 24 +-----------------------
 drivers/usb/host/usb-uclass.c |  5 ++---
 include/usb.h                 |  2 +-
 3 files changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 19f1e29..00c038c 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -292,7 +292,6 @@ static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
 					  struct QH *qh)
 {
 	struct usb_device *ttdev;
-	int parent_devnum;
 
 	if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
 		return;
@@ -302,35 +301,14 @@ static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
 	 * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
 	 * in the tree before that one!
 	 */
-#ifdef CONFIG_DM_USB
-	struct udevice *parent;
-
-	for (ttdev = udev; ; ) {
-		struct udevice *dev = ttdev->dev;
-
-		if (dev->parent &&
-		    device_get_uclass_id(dev->parent) == UCLASS_USB_HUB)
-			parent = dev->parent;
-		else
-			parent = NULL;
-		if (!parent)
-			return;
-		ttdev = dev_get_parentdata(parent);
-		if (!ttdev->speed != USB_SPEED_HIGH)
-			break;
-	}
-	parent_devnum = ttdev->devnum;
-#else
 	ttdev = udev;
 	while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
 		ttdev = ttdev->parent;
 	if (!ttdev->parent)
 		return;
-	parent_devnum = ttdev->parent->devnum;
-#endif
 
 	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
-				     QH_ENDPT2_HUBADDR(parent_devnum));
+				     QH_ENDPT2_HUBADDR(ttdev->parent->devnum));
 }
 
 static int
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 95e371d..0157bc6 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -470,7 +470,6 @@ int usb_scan_device(struct udevice *parent, int port,
 	bool created = false;
 	struct usb_dev_platdata *plat;
 	struct usb_bus_priv *priv;
-	struct usb_device *parent_udev;
 	int ret;
 	ALLOC_CACHE_ALIGN_BUFFER(struct usb_device, udev, 1);
 	struct usb_interface_descriptor *iface = &udev->config.if_desc[0].desc;
@@ -515,9 +514,9 @@ int usb_scan_device(struct udevice *parent, int port,
 	udev->devnum = priv->next_addr + 1;
 	udev->portnr = port;
 	debug("Calling usb_setup_device(), portnr=%d\n", udev->portnr);
-	parent_udev = device_get_uclass_id(parent) == UCLASS_USB_HUB ?
+	udev->parent = device_get_uclass_id(parent) == UCLASS_USB_HUB ?
 		dev_get_parentdata(parent) : NULL;
-	ret = usb_setup_device(udev, priv->desc_before_addr, parent_udev, port);
+	ret = usb_setup_device(udev, priv->desc_before_addr, udev->parent, port);
 	debug("read_descriptor for '%s': ret=%d\n", parent->name, ret);
 	if (ret)
 		return ret;
diff --git a/include/usb.h b/include/usb.h
index 1b667ff..7876df4 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -141,9 +141,9 @@ struct usb_device {
 	int act_len;			/* transfered bytes */
 	int maxchild;			/* Number of ports if hub */
 	int portnr;			/* Port number, 1=first */
-#ifndef CONFIG_DM_USB
 	/* parent hub, or NULL if this is the root hub */
 	struct usb_device *parent;
+#ifndef CONFIG_DM_USB
 	struct usb_device *children[USB_MAXCHILDREN];
 	void *controller;		/* hardware controller private data */
 #endif
-- 
2.3.6

[U-Boot] [PATCH 3/8] dm: usb: Store usb_device parent pointer in usb_device

[Simon Glass]

Hi Hans,

On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
> When calling into the hcd code in usb_scan_device() we do not yet have
> the actual udevice for the device we are scanning, so we temporarily set
> usb_device.dev to the parent.
>
> This means that we cannot use usb_device.dev to accurately determine our
> place in the usb topology when reading descriptors, and this is necessary
> to correctly program the usb-2 hub tt settings when a usb-1 device is
> connected to a usb-2 hub.
>
> This commit (re)adds a direct parent usb_device pointer to struct usb_device
> so that the usb hcd code can get to the usb_device parent without needing to
> go through the dm.
>
> This fixes usb-1 devices plugged into usb-2 hubs not working with the driver
> model.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/ehci-hcd.c   | 24 +-----------------------
>  drivers/usb/host/usb-uclass.c |  5 ++---
>  include/usb.h                 |  2 +-
>  3 files changed, 4 insertions(+), 27 deletions(-)
>

While I agree this is expedient, is there really no other way? How are
we going to move to driver model if we add back in the
non-driver-model pointers, etc.? We'll end up with a hybrid approach
and a real mess.

Is it possible to bug-fix the driver model code (presumably below) instead?

> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 19f1e29..00c038c 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -292,7 +292,6 @@ static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
>                                           struct QH *qh)
>  {
>         struct usb_device *ttdev;
> -       int parent_devnum;
>
>         if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
>                 return;
> @@ -302,35 +301,14 @@ static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
>          * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
>          * in the tree before that one!
>          */
> -#ifdef CONFIG_DM_USB
> -       struct udevice *parent;
> -
> -       for (ttdev = udev; ; ) {
> -               struct udevice *dev = ttdev->dev;
> -
> -               if (dev->parent &&
> -                   device_get_uclass_id(dev->parent) == UCLASS_USB_HUB)
> -                       parent = dev->parent;
> -               else
> -                       parent = NULL;
> -               if (!parent)
> -                       return;
> -               ttdev = dev_get_parentdata(parent);
> -               if (!ttdev->speed != USB_SPEED_HIGH)
> -                       break;
> -       }
> -       parent_devnum = ttdev->devnum;
> -#else
>         ttdev = udev;
>         while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
>                 ttdev = ttdev->parent;
>         if (!ttdev->parent)
>                 return;
> -       parent_devnum = ttdev->parent->devnum;
> -#endif
>
>         qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
> -                                    QH_ENDPT2_HUBADDR(parent_devnum));
> +                                    QH_ENDPT2_HUBADDR(ttdev->parent->devnum));
>  }
>
>  static int
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index 95e371d..0157bc6 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -470,7 +470,6 @@ int usb_scan_device(struct udevice *parent, int port,
>         bool created = false;
>         struct usb_dev_platdata *plat;
>         struct usb_bus_priv *priv;
> -       struct usb_device *parent_udev;
>         int ret;
>         ALLOC_CACHE_ALIGN_BUFFER(struct usb_device, udev, 1);
>         struct usb_interface_descriptor *iface = &udev->config.if_desc[0].desc;
> @@ -515,9 +514,9 @@ int usb_scan_device(struct udevice *parent, int port,
>         udev->devnum = priv->next_addr + 1;
>         udev->portnr = port;
>         debug("Calling usb_setup_device(), portnr=%d\n", udev->portnr);
> -       parent_udev = device_get_uclass_id(parent) == UCLASS_USB_HUB ?
> +       udev->parent = device_get_uclass_id(parent) == UCLASS_USB_HUB ?
>                 dev_get_parentdata(parent) : NULL;
> -       ret = usb_setup_device(udev, priv->desc_before_addr, parent_udev, port);
> +       ret = usb_setup_device(udev, priv->desc_before_addr, udev->parent, port);
>         debug("read_descriptor for '%s': ret=%d\n", parent->name, ret);
>         if (ret)
>                 return ret;
> diff --git a/include/usb.h b/include/usb.h
> index 1b667ff..7876df4 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -141,9 +141,9 @@ struct usb_device {
>         int act_len;                    /* transfered bytes */
>         int maxchild;                   /* Number of ports if hub */
>         int portnr;                     /* Port number, 1=first */
> -#ifndef CONFIG_DM_USB
>         /* parent hub, or NULL if this is the root hub */
>         struct usb_device *parent;
> +#ifndef CONFIG_DM_USB
>         struct usb_device *children[USB_MAXCHILDREN];
>         void *controller;               /* hardware controller private data */
>  #endif
> --
> 2.3.6
>

[U-Boot] [PATCH 3/8] dm: usb: Store usb_device parent pointer in usb_device

[Hans de Goede]

Hi,

On 01-05-15 06:11, Simon Glass wrote:
> Hi Hans,
>
> On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
>> When calling into the hcd code in usb_scan_device() we do not yet have
>> the actual udevice for the device we are scanning, so we temporarily set
>> usb_device.dev to the parent.
>>
>> This means that we cannot use usb_device.dev to accurately determine our
>> place in the usb topology when reading descriptors, and this is necessary
>> to correctly program the usb-2 hub tt settings when a usb-1 device is
>> connected to a usb-2 hub.
>>
>> This commit (re)adds a direct parent usb_device pointer to struct usb_device
>> so that the usb hcd code can get to the usb_device parent without needing to
>> go through the dm.
>>
>> This fixes usb-1 devices plugged into usb-2 hubs not working with the driver
>> model.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/host/ehci-hcd.c   | 24 +-----------------------
>>   drivers/usb/host/usb-uclass.c |  5 ++---
>>   include/usb.h                 |  2 +-
>>   3 files changed, 4 insertions(+), 27 deletions(-)
>>
>
> While I agree this is expedient, is there really no other way? How are
> we going to move to driver model if we add back in the
> non-driver-model pointers, etc.? We'll end up with a hybrid approach
> and a real mess.
>
> Is it possible to bug-fix the driver model code (presumably below) instead?
>

That is what I tried first, and failed todo. But after implementing the fix
as posted to the list I've been thinking about it some more and I think it should
be possible so I'll try again.

Regards,

Hans


>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> index 19f1e29..00c038c 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>> @@ -292,7 +292,6 @@ static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
>>                                            struct QH *qh)
>>   {
>>          struct usb_device *ttdev;
>> -       int parent_devnum;
>>
>>          if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
>>                  return;
>> @@ -302,35 +301,14 @@ static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
>>           * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
>>           * in the tree before that one!
>>           */
>> -#ifdef CONFIG_DM_USB
>> -       struct udevice *parent;
>> -
>> -       for (ttdev = udev; ; ) {
>> -               struct udevice *dev = ttdev->dev;
>> -
>> -               if (dev->parent &&
>> -                   device_get_uclass_id(dev->parent) == UCLASS_USB_HUB)
>> -                       parent = dev->parent;
>> -               else
>> -                       parent = NULL;
>> -               if (!parent)
>> -                       return;
>> -               ttdev = dev_get_parentdata(parent);
>> -               if (!ttdev->speed != USB_SPEED_HIGH)
>> -                       break;
>> -       }
>> -       parent_devnum = ttdev->devnum;
>> -#else
>>          ttdev = udev;
>>          while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
>>                  ttdev = ttdev->parent;
>>          if (!ttdev->parent)
>>                  return;
>> -       parent_devnum = ttdev->parent->devnum;
>> -#endif
>>
>>          qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
>> -                                    QH_ENDPT2_HUBADDR(parent_devnum));
>> +                                    QH_ENDPT2_HUBADDR(ttdev->parent->devnum));
>>   }
>>
>>   static int
>> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>> index 95e371d..0157bc6 100644
>> --- a/drivers/usb/host/usb-uclass.c
>> +++ b/drivers/usb/host/usb-uclass.c
>> @@ -470,7 +470,6 @@ int usb_scan_device(struct udevice *parent, int port,
>>          bool created = false;
>>          struct usb_dev_platdata *plat;
>>          struct usb_bus_priv *priv;
>> -       struct usb_device *parent_udev;
>>          int ret;
>>          ALLOC_CACHE_ALIGN_BUFFER(struct usb_device, udev, 1);
>>          struct usb_interface_descriptor *iface = &udev->config.if_desc[0].desc;
>> @@ -515,9 +514,9 @@ int usb_scan_device(struct udevice *parent, int port,
>>          udev->devnum = priv->next_addr + 1;
>>          udev->portnr = port;
>>          debug("Calling usb_setup_device(), portnr=%d\n", udev->portnr);
>> -       parent_udev = device_get_uclass_id(parent) == UCLASS_USB_HUB ?
>> +       udev->parent = device_get_uclass_id(parent) == UCLASS_USB_HUB ?
>>                  dev_get_parentdata(parent) : NULL;
>> -       ret = usb_setup_device(udev, priv->desc_before_addr, parent_udev, port);
>> +       ret = usb_setup_device(udev, priv->desc_before_addr, udev->parent, port);
>>          debug("read_descriptor for '%s': ret=%d\n", parent->name, ret);
>>          if (ret)
>>                  return ret;
>> diff --git a/include/usb.h b/include/usb.h
>> index 1b667ff..7876df4 100644
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> @@ -141,9 +141,9 @@ struct usb_device {
>>          int act_len;                    /* transfered bytes */
>>          int maxchild;                   /* Number of ports if hub */
>>          int portnr;                     /* Port number, 1=first */
>> -#ifndef CONFIG_DM_USB
>>          /* parent hub, or NULL if this is the root hub */
>>          struct usb_device *parent;
>> +#ifndef CONFIG_DM_USB
>>          struct usb_device *children[USB_MAXCHILDREN];
>>          void *controller;               /* hardware controller private data */
>>   #endif
>> --
>> 2.3.6
>>

[U-Boot] [PATCH 4/8] dm: usb: Set desc_before_addr from ehci dm code

[Hans de Goede]

Without this usb-1 device descriptors do not get read properly.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/ehci-hcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 00c038c..9fc1e33 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1547,12 +1547,15 @@ int ehci_register(struct udevice *dev, struct ehci_hccr *hccr,
 		  struct ehci_hcor *hcor, const struct ehci_ops *ops,
 		  uint tweaks, enum usb_init_type init)
 {
+	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
 	struct ehci_ctrl *ctrl = dev_get_priv(dev);
 	int ret;
 
 	debug("%s: dev='%s', ctrl=%p, hccr=%p, hcor=%p, init=%d\n", __func__,
 	      dev->name, ctrl, hccr, hcor, init);
 
+	priv->desc_before_addr = true;
+
 	ehci_setup_ops(ctrl, ops);
 	ctrl->hccr = hccr;
 	ctrl->hcor = hcor;
-- 
2.3.6

[U-Boot] [PATCH 4/8] dm: usb: Set desc_before_addr from ehci dm code

[Simon Glass]

On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
> Without this usb-1 device descriptors do not get read properly.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/ehci-hcd.c | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 00c038c..9fc1e33 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -1547,12 +1547,15 @@ int ehci_register(struct udevice *dev, struct ehci_hccr *hccr,
>                   struct ehci_hcor *hcor, const struct ehci_ops *ops,
>                   uint tweaks, enum usb_init_type init)
>  {
> +       struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>         struct ehci_ctrl *ctrl = dev_get_priv(dev);
>         int ret;
>
>         debug("%s: dev='%s', ctrl=%p, hccr=%p, hcor=%p, init=%d\n", __func__,
>               dev->name, ctrl, hccr, hcor, init);
>
> +       priv->desc_before_addr = true;
> +
>         ehci_setup_ops(ctrl, ops);
>         ctrl->hccr = hccr;
>         ctrl->hcor = hcor;
> --
> 2.3.6
>

[U-Boot] [PATCH 5/8] dm: usb: Add support for interrupt queues to the dm usb code

[Hans de Goede]

Interrupt endpoints typically are polled for a long time by the usb
controller before they return anything, so calls to submit_int_msg() can
take a long time to complete this.

To avoid this the u-boot code has the an interrupt queue mechanism / API,
add support for this to the driver-model usb code.

See the added doc comments for more details.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/usb-uclass.c | 36 ++++++++++++++++++++++++++++++++
 include/usb.h                 | 48 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 0157bc6..9144f6b 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -65,6 +65,42 @@ int submit_bulk_msg(struct usb_device *udev, unsigned long pipe, void *buffer,
 	return ops->bulk(bus, udev, pipe, buffer, length);
 }
 
+struct int_queue *create_int_queue(struct usb_device *udev,
+		unsigned long pipe, int queuesize, int elementsize,
+		void *buffer, int interval)
+{
+	struct udevice *bus = udev->controller_dev;
+	struct dm_usb_ops *ops = usb_get_ops(bus);
+
+	if (!ops->create_int_queue)
+		return NULL;
+
+	return ops->create_int_queue(bus, udev, pipe, queuesize, elementsize,
+				     buffer, interval);
+}
+
+void *poll_int_queue(struct usb_device *udev, struct int_queue *queue)
+{
+	struct udevice *bus = udev->controller_dev;
+	struct dm_usb_ops *ops = usb_get_ops(bus);
+
+	if (!ops->poll_int_queue)
+		return NULL;
+
+	return ops->poll_int_queue(bus, udev, queue);
+}
+
+int destroy_int_queue(struct usb_device *udev, struct int_queue *queue)
+{
+	struct udevice *bus = udev->controller_dev;
+	struct dm_usb_ops *ops = usb_get_ops(bus);
+
+	if (!ops->destroy_int_queue)
+		return -ENOSYS;
+
+	return ops->destroy_int_queue(bus, udev, queue);
+}
+
 int usb_alloc_device(struct usb_device *udev)
 {
 	struct udevice *bus = udev->controller_dev;
diff --git a/include/usb.h b/include/usb.h
index 7876df4..77bdfd9 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -198,7 +198,7 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 			int transfer_len, int interval);
 
-#if defined CONFIG_USB_EHCI || defined CONFIG_MUSB_HOST
+#if defined CONFIG_USB_EHCI || defined CONFIG_MUSB_HOST || defined(CONFIG_DM_USB)
 struct int_queue *create_int_queue(struct usb_device *dev, unsigned long pipe,
 	int queuesize, int elementsize, void *buffer, int interval);
 int destroy_int_queue(struct usb_device *dev, struct int_queue *queue);
@@ -654,6 +654,52 @@ struct dm_usb_ops {
 	int (*interrupt)(struct udevice *bus, struct usb_device *udev,
 			 unsigned long pipe, void *buffer, int length,
 			 int interval);
+
+	/**
+	 * create_int_queue() - Create and queue interrupt packets
+	 *
+	 * Create and queue @queuesize number of interrupt usb packets of
+	 * @elementsize bytes each. @buffer must be atleast @queuesize *
+	 * @elementsize bytes.
+	 *
+	 * Note some controllers only support a queuesize of 1.
+	 *
+	 * @interval: Interrupt interval
+	 *
+	 * @return A pointer to the created interrupt queue or NULL on error
+	 */
+	struct int_queue *(*create_int_queue)(struct udevice *bus,
+				struct usb_device *udev, unsigned long pipe,
+				int queuesize, int elementsize, void *buffer,
+				int interval);
+
+	/**
+	 * poll_int_queue() - Poll an interrupt queue for completed packets
+	 *
+	 * Poll an interrupt queue for completed packets. The return value
+	 * points to the part of the buffer passed to create_int_queue()
+	 * corresponding to the completed packet.
+	 *
+	 * @queue: queue to poll
+	 *
+	 * @return Pointer to the data of the first completed packet, or
+	 *         NULL if no packets are ready
+	 */
+	void *(*poll_int_queue)(struct udevice *bus, struct usb_device *udev,
+				struct int_queue *queue);
+
+	/**
+	 * destroy_int_queue() - Destroy an interrupt queue
+	 *
+	 * Destroy an interrupt queue created by create_int_queue().
+	 *
+	 * @queue: queue to poll
+	 *
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*destroy_int_queue)(struct udevice *bus, struct usb_device *udev,
+				 struct int_queue *queue);
+
 	/**
 	 * alloc_device() - Allocate a new device context (XHCI)
 	 *
-- 
2.3.6

[U-Boot] [PATCH 5/8] dm: usb: Add support for interrupt queues to the dm usb code

[Simon Glass]

On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
> Interrupt endpoints typically are polled for a long time by the usb
> controller before they return anything, so calls to submit_int_msg() can
> take a long time to complete this.
>
> To avoid this the u-boot code has the an interrupt queue mechanism / API,
> add support for this to the driver-model usb code.
>
> See the added doc comments for more details.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/usb-uclass.c | 36 ++++++++++++++++++++++++++++++++
>  include/usb.h                 | 48 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 83 insertions(+), 1 deletion(-)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 6/8] dm: usb: Prefix ehci interrupt-queue functions with _ehci_

[Hans de Goede]

This is a preparation patch for adding interrupt-queue support to the
ehci dm code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/ehci-hcd.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 9fc1e33..12145ed 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1220,9 +1220,9 @@ disable_periodic(struct ehci_ctrl *ctrl)
 	return 0;
 }
 
-struct int_queue *
-create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize,
-		 int elementsize, void *buffer, int interval)
+static struct int_queue *_ehci_create_int_queue(struct usb_device *dev,
+			unsigned long pipe, int queuesize, int elementsize,
+			void *buffer, int interval)
 {
 	struct ehci_ctrl *ctrl = ehci_get_ctrl(dev);
 	struct int_queue *result = NULL;
@@ -1378,7 +1378,8 @@ fail1:
 	return NULL;
 }
 
-void *poll_int_queue(struct usb_device *dev, struct int_queue *queue)
+static void *_ehci_poll_int_queue(struct usb_device *dev,
+				  struct int_queue *queue)
 {
 	struct QH *cur = queue->current;
 	struct qTD *cur_td;
@@ -1413,8 +1414,8 @@ void *poll_int_queue(struct usb_device *dev, struct int_queue *queue)
 }
 
 /* Do not free buffers associated with QHs, they're owned by someone else */
-int
-destroy_int_queue(struct usb_device *dev, struct int_queue *queue)
+static int _ehci_destroy_int_queue(struct usb_device *dev,
+				   struct int_queue *queue)
 {
 	struct ehci_ctrl *ctrl = ehci_get_ctrl(dev);
 	int result = -1;
@@ -1515,6 +1516,24 @@ int submit_int_msg(struct usb_device *dev, unsigned long pipe,
 {
 	return _ehci_submit_int_msg(dev, pipe, buffer, length, interval);
 }
+
+struct int_queue *create_int_queue(struct usb_device *dev,
+		unsigned long pipe, int queuesize, int elementsize,
+		void *buffer, int interval)
+{
+	return _ehci_create_int_queue(dev, pipe, queuesize, elementsize,
+				      buffer, interval);
+}
+
+void *poll_int_queue(struct usb_device *dev, struct int_queue *queue)
+{
+	return _ehci_poll_int_queue(dev, queue);
+}
+
+int destroy_int_queue(struct usb_device *dev, struct int_queue *queue)
+{
+	return _ehci_destroy_int_queue(dev, queue);
+}
 #endif
 
 #ifdef CONFIG_DM_USB
-- 
2.3.6

[U-Boot] [PATCH 6/8] dm: usb: Prefix ehci interrupt-queue functions with _ehci_

[Simon Glass]

On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
> This is a preparation patch for adding interrupt-queue support to the
> ehci dm code.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/ehci-hcd.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 7/8] dm: usb: Add support for interrupt queues to the dm ehci code

[Hans de Goede]

Add support for interrupt queues to the dm ehci code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/ehci-hcd.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 12145ed..9e8bacb 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1562,6 +1562,29 @@ static int ehci_submit_int_msg(struct udevice *dev, struct usb_device *udev,
 	return _ehci_submit_int_msg(udev, pipe, buffer, length, interval);
 }
 
+static struct int_queue *ehci_create_int_queue(struct udevice *dev,
+		struct usb_device *udev, unsigned long pipe, int queuesize,
+		int elementsize, void *buffer, int interval)
+{
+	debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev);
+	return _ehci_create_int_queue(udev, pipe, queuesize, elementsize,
+				      buffer, interval);
+}
+
+static void *ehci_poll_int_queue(struct udevice *dev, struct usb_device *udev,
+				 struct int_queue *queue)
+{
+	debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev);
+	return _ehci_poll_int_queue(udev, queue);
+}
+
+static int ehci_destroy_int_queue(struct udevice *dev, struct usb_device *udev,
+				  struct int_queue *queue)
+{
+	debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev);
+	return _ehci_destroy_int_queue(udev, queue);
+}
+
 int ehci_register(struct udevice *dev, struct ehci_hccr *hccr,
 		  struct ehci_hcor *hcor, const struct ehci_ops *ops,
 		  uint tweaks, enum usb_init_type init)
@@ -1610,6 +1633,9 @@ struct dm_usb_ops ehci_usb_ops = {
 	.control = ehci_submit_control_msg,
 	.bulk = ehci_submit_bulk_msg,
 	.interrupt = ehci_submit_int_msg,
+	.create_int_queue = ehci_create_int_queue,
+	.poll_int_queue = ehci_poll_int_queue,
+	.destroy_int_queue = ehci_destroy_int_queue,
 };
 
 #endif
-- 
2.3.6

[U-Boot] [PATCH 7/8] dm: usb: Add support for interrupt queues to the dm ehci code

[Simon Glass]

On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
> Add support for interrupt queues to the dm ehci code.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/ehci-hcd.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 8/8] sunxi: ehci: Convert to the driver-model

[Hans de Goede]

Convert sunxi-boards which use the sunxi-ehci code to the driver-model.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 board/sunxi/Kconfig           |  3 ++
 drivers/usb/host/ehci-sunxi.c | 95 ++++++++++++++++++++++++++++++-------------
 2 files changed, 69 insertions(+), 29 deletions(-)

diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index 18e5561..6dcbff3 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -559,4 +559,7 @@ config DM_ETH
 config DM_SERIAL
 	default y
 
+config DM_USB
+	default y if !USB_MUSB_SUNXI
+
 endif
diff --git a/drivers/usb/host/ehci-sunxi.c b/drivers/usb/host/ehci-sunxi.c
index 0edb643..9c6703c 100644
--- a/drivers/usb/host/ehci-sunxi.c
+++ b/drivers/usb/host/ehci-sunxi.c
@@ -14,53 +14,90 @@
 #include <asm/arch/clock.h>
 #include <asm/arch/usb_phy.h>
 #include <asm/io.h>
+#include <dm.h>
 #include "ehci.h"
 
-int ehci_hcd_init(int index, enum usb_init_type init, struct ehci_hccr **hccr,
-		struct ehci_hcor **hcor)
+struct ehci_sunxi_priv {
+	struct ehci_ctrl ehci;
+	int ahb_gate_mask;
+	int phy_index;
+};
+
+static int ehci_usb_ofdata_to_platdata(struct udevice *dev)
+{
+	return 0;
+}
+
+static int ehci_usb_probe(struct udevice *dev)
 {
 	struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
-	int ahb_gate_offset;
+	struct usb_platdata *plat = dev_get_platdata(dev);
+	struct ehci_sunxi_priv *priv = dev_get_priv(dev);
+	struct ehci_hccr *hccr = (struct ehci_hccr *)dev_get_addr(dev);
+	struct ehci_hcor *hcor;
+
+	if (hccr == (void *)SUNXI_USB1_BASE) {
+		priv->ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI0;
+		priv->phy_index = 1;
+	} else {
+		priv->ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI1;
+		priv->phy_index = 2;
+	}
 
-	ahb_gate_offset = index ? AHB_GATE_OFFSET_USB_EHCI1 :
-				  AHB_GATE_OFFSET_USB_EHCI0;
-	setbits_le32(&ccm->ahb_gate0, 1 << ahb_gate_offset);
+	setbits_le32(&ccm->ahb_gate0, priv->ahb_gate_mask);
 #ifdef CONFIG_SUNXI_GEN_SUN6I
-	setbits_le32(&ccm->ahb_reset0_cfg, 1 << ahb_gate_offset);
+	setbits_le32(&ccm->ahb_reset0_cfg, priv->ahb_gate_mask);
 #endif
 
-	sunxi_usb_phy_init(index + 1);
-	sunxi_usb_phy_power_on(index + 1);
+	sunxi_usb_phy_init(priv->phy_index);
+	sunxi_usb_phy_power_on(priv->phy_index);
 
-	if (index == 0)
-		*hccr = (void *)SUNXI_USB1_BASE;
-	else
-		*hccr = (void *)SUNXI_USB2_BASE;
+	hcor = (struct ehci_hcor *)((uint32_t)hccr +
+				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
 
-	*hcor = (struct ehci_hcor *)((uint32_t) *hccr
-				+ HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
-
-	debug("sunxi-ehci: init hccr %x and hcor %x hc_length %d\n",
-	      (uint32_t)*hccr, (uint32_t)*hcor,
-	      (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
-
-	return 0;
+	return ehci_register(dev, hccr, hcor, NULL, 0, plat->init_type);
 }
 
-int ehci_hcd_stop(int index)
+static int ehci_usb_remove(struct udevice *dev)
 {
 	struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
-	int ahb_gate_offset;
+	struct ehci_sunxi_priv *priv = dev_get_priv(dev);
+	int ret;
 
-	sunxi_usb_phy_power_off(index + 1);
-	sunxi_usb_phy_exit(index + 1);
+	ret = ehci_deregister(dev);
+	if (ret)
+		return ret;
+
+	sunxi_usb_phy_power_off(priv->phy_index);
+	sunxi_usb_phy_exit(priv->phy_index);
 
-	ahb_gate_offset = index ? AHB_GATE_OFFSET_USB_EHCI1 :
-				  AHB_GATE_OFFSET_USB_EHCI0;
 #ifdef CONFIG_SUNXI_GEN_SUN6I
-	clrbits_le32(&ccm->ahb_reset0_cfg, 1 << ahb_gate_offset);
+	clrbits_le32(&ccm->ahb_reset0_cfg, priv->ahb_gate_mask);
 #endif
-	clrbits_le32(&ccm->ahb_gate0, 1 << ahb_gate_offset);
+	clrbits_le32(&ccm->ahb_gate0, priv->ahb_gate_mask);
 
 	return 0;
 }
+
+static const struct udevice_id ehci_usb_ids[] = {
+	{ .compatible = "allwinner,sun4i-a10-ehci", },
+	{ .compatible = "allwinner,sun5i-a13-ehci", },
+	{ .compatible = "allwinner,sun6i-a31-ehci", },
+	{ .compatible = "allwinner,sun7i-a20-ehci", },
+	{ .compatible = "allwinner,sun8i-a23-ehci", },
+	{ .compatible = "allwinner,sun9i-a80-ehci", },
+	{ }
+};
+
+U_BOOT_DRIVER(usb_ehci) = {
+	.name	= "ehci_sunxi",
+	.id	= UCLASS_USB,
+	.of_match = ehci_usb_ids,
+	.ofdata_to_platdata = ehci_usb_ofdata_to_platdata,
+	.probe = ehci_usb_probe,
+	.remove = ehci_usb_remove,
+	.ops	= &ehci_usb_ops,
+	.platdata_auto_alloc_size = sizeof(struct usb_platdata),
+	.priv_auto_alloc_size = sizeof(struct ehci_sunxi_priv),
+	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
+};
-- 
2.3.6

[U-Boot] [PATCH 8/8] sunxi: ehci: Convert to the driver-model

[Simon Glass]

Hi Hans,

On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
> Convert sunxi-boards which use the sunxi-ehci code to the driver-model.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  board/sunxi/Kconfig           |  3 ++
>  drivers/usb/host/ehci-sunxi.c | 95 ++++++++++++++++++++++++++++++-------------
>  2 files changed, 69 insertions(+), 29 deletions(-)

A few nits, but otherwise:

Acked-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
> index 18e5561..6dcbff3 100644
> --- a/board/sunxi/Kconfig
> +++ b/board/sunxi/Kconfig
> @@ -559,4 +559,7 @@ config DM_ETH
>  config DM_SERIAL
>         default y
>
> +config DM_USB
> +       default y if !USB_MUSB_SUNXI
> +
>  endif
> diff --git a/drivers/usb/host/ehci-sunxi.c b/drivers/usb/host/ehci-sunxi.c
> index 0edb643..9c6703c 100644
> --- a/drivers/usb/host/ehci-sunxi.c
> +++ b/drivers/usb/host/ehci-sunxi.c
> @@ -14,53 +14,90 @@
>  #include <asm/arch/clock.h>
>  #include <asm/arch/usb_phy.h>
>  #include <asm/io.h>
> +#include <dm.h>
>  #include "ehci.h"
>
> -int ehci_hcd_init(int index, enum usb_init_type init, struct ehci_hccr **hccr,
> -               struct ehci_hcor **hcor)
> +struct ehci_sunxi_priv {
> +       struct ehci_ctrl ehci;

Comment for these two: ?

> +       int ahb_gate_mask;
> +       int phy_index;
> +};
> +
> +static int ehci_usb_ofdata_to_platdata(struct udevice *dev)
> +{
> +       return 0;

Maybe can drop this function if not used? Or do you plan to use it later?

> +}
> +
> +static int ehci_usb_probe(struct udevice *dev)
>  {
>         struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> -       int ahb_gate_offset;
> +       struct usb_platdata *plat = dev_get_platdata(dev);
> +       struct ehci_sunxi_priv *priv = dev_get_priv(dev);
> +       struct ehci_hccr *hccr = (struct ehci_hccr *)dev_get_addr(dev);
> +       struct ehci_hcor *hcor;
> +
> +       if (hccr == (void *)SUNXI_USB1_BASE) {
> +               priv->ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI0;
> +               priv->phy_index = 1;
> +       } else {
> +               priv->ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI1;
> +               priv->phy_index = 2;
> +       }
>
> -       ahb_gate_offset = index ? AHB_GATE_OFFSET_USB_EHCI1 :
> -                                 AHB_GATE_OFFSET_USB_EHCI0;
> -       setbits_le32(&ccm->ahb_gate0, 1 << ahb_gate_offset);
> +       setbits_le32(&ccm->ahb_gate0, priv->ahb_gate_mask);
>  #ifdef CONFIG_SUNXI_GEN_SUN6I
> -       setbits_le32(&ccm->ahb_reset0_cfg, 1 << ahb_gate_offset);
> +       setbits_le32(&ccm->ahb_reset0_cfg, priv->ahb_gate_mask);
>  #endif
>
> -       sunxi_usb_phy_init(index + 1);
> -       sunxi_usb_phy_power_on(index + 1);
> +       sunxi_usb_phy_init(priv->phy_index);
> +       sunxi_usb_phy_power_on(priv->phy_index);
>
> -       if (index == 0)
> -               *hccr = (void *)SUNXI_USB1_BASE;
> -       else
> -               *hccr = (void *)SUNXI_USB2_BASE;
> +       hcor = (struct ehci_hcor *)((uint32_t)hccr +
> +                                   HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>
> -       *hcor = (struct ehci_hcor *)((uint32_t) *hccr
> -                               + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
> -
> -       debug("sunxi-ehci: init hccr %x and hcor %x hc_length %d\n",
> -             (uint32_t)*hccr, (uint32_t)*hcor,
> -             (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
> -
> -       return 0;
> +       return ehci_register(dev, hccr, hcor, NULL, 0, plat->init_type);
>  }
>
> -int ehci_hcd_stop(int index)
> +static int ehci_usb_remove(struct udevice *dev)
>  {
>         struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> -       int ahb_gate_offset;
> +       struct ehci_sunxi_priv *priv = dev_get_priv(dev);
> +       int ret;
>
> -       sunxi_usb_phy_power_off(index + 1);
> -       sunxi_usb_phy_exit(index + 1);
> +       ret = ehci_deregister(dev);
> +       if (ret)
> +               return ret;
> +
> +       sunxi_usb_phy_power_off(priv->phy_index);
> +       sunxi_usb_phy_exit(priv->phy_index);
>
> -       ahb_gate_offset = index ? AHB_GATE_OFFSET_USB_EHCI1 :
> -                                 AHB_GATE_OFFSET_USB_EHCI0;
>  #ifdef CONFIG_SUNXI_GEN_SUN6I
> -       clrbits_le32(&ccm->ahb_reset0_cfg, 1 << ahb_gate_offset);
> +       clrbits_le32(&ccm->ahb_reset0_cfg, priv->ahb_gate_mask);
>  #endif
> -       clrbits_le32(&ccm->ahb_gate0, 1 << ahb_gate_offset);
> +       clrbits_le32(&ccm->ahb_gate0, priv->ahb_gate_mask);
>
>         return 0;
>  }
> +
> +static const struct udevice_id ehci_usb_ids[] = {
> +       { .compatible = "allwinner,sun4i-a10-ehci", },
> +       { .compatible = "allwinner,sun5i-a13-ehci", },
> +       { .compatible = "allwinner,sun6i-a31-ehci", },
> +       { .compatible = "allwinner,sun7i-a20-ehci", },
> +       { .compatible = "allwinner,sun8i-a23-ehci", },
> +       { .compatible = "allwinner,sun9i-a80-ehci", },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(usb_ehci) = {
> +       .name   = "ehci_sunxi",
> +       .id     = UCLASS_USB,
> +       .of_match = ehci_usb_ids,
> +       .ofdata_to_platdata = ehci_usb_ofdata_to_platdata,
> +       .probe = ehci_usb_probe,
> +       .remove = ehci_usb_remove,
> +       .ops    = &ehci_usb_ops,
> +       .platdata_auto_alloc_size = sizeof(struct usb_platdata),
> +       .priv_auto_alloc_size = sizeof(struct ehci_sunxi_priv),
> +       .flags  = DM_FLAG_ALLOC_PRIV_DMA,
> +};
> --
> 2.3.6
>

Regards,
Simon

[U-Boot] [PATCH 8/8] sunxi: ehci: Convert to the driver-model

[Hans de Goede]

Hi,

On 01-05-15 06:12, Simon Glass wrote:
> Hi Hans,
>
> On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
>> Convert sunxi-boards which use the sunxi-ehci code to the driver-model.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   board/sunxi/Kconfig           |  3 ++
>>   drivers/usb/host/ehci-sunxi.c | 95 ++++++++++++++++++++++++++++++-------------
>>   2 files changed, 69 insertions(+), 29 deletions(-)
>
> A few nits, but otherwise:
>
> Acked-by: Simon Glass <sjg@chromium.org>
>
>>
>> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
>> index 18e5561..6dcbff3 100644
>> --- a/board/sunxi/Kconfig
>> +++ b/board/sunxi/Kconfig
>> @@ -559,4 +559,7 @@ config DM_ETH
>>   config DM_SERIAL
>>          default y
>>
>> +config DM_USB
>> +       default y if !USB_MUSB_SUNXI
>> +
>>   endif
>> diff --git a/drivers/usb/host/ehci-sunxi.c b/drivers/usb/host/ehci-sunxi.c
>> index 0edb643..9c6703c 100644
>> --- a/drivers/usb/host/ehci-sunxi.c
>> +++ b/drivers/usb/host/ehci-sunxi.c
>> @@ -14,53 +14,90 @@
>>   #include <asm/arch/clock.h>
>>   #include <asm/arch/usb_phy.h>
>>   #include <asm/io.h>
>> +#include <dm.h>
>>   #include "ehci.h"
>>
>> -int ehci_hcd_init(int index, enum usb_init_type init, struct ehci_hccr **hccr,
>> -               struct ehci_hcor **hcor)
>> +struct ehci_sunxi_priv {
>> +       struct ehci_ctrl ehci;
>
> Comment for these two: ?
>
>> +       int ahb_gate_mask;
>> +       int phy_index;
>> +};
>> +
>> +static int ehci_usb_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +       return 0;
>
> Maybe can drop this function if not used? Or do you plan to use it later?

Both are fixed in my sunxi tree now.

Regards,

Hans

>
>> +}
>> +
>> +static int ehci_usb_probe(struct udevice *dev)
>>   {
>>          struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>> -       int ahb_gate_offset;
>> +       struct usb_platdata *plat = dev_get_platdata(dev);
>> +       struct ehci_sunxi_priv *priv = dev_get_priv(dev);
>> +       struct ehci_hccr *hccr = (struct ehci_hccr *)dev_get_addr(dev);
>> +       struct ehci_hcor *hcor;
>> +
>> +       if (hccr == (void *)SUNXI_USB1_BASE) {
>> +               priv->ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI0;
>> +               priv->phy_index = 1;
>> +       } else {
>> +               priv->ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI1;
>> +               priv->phy_index = 2;
>> +       }
>>
>> -       ahb_gate_offset = index ? AHB_GATE_OFFSET_USB_EHCI1 :
>> -                                 AHB_GATE_OFFSET_USB_EHCI0;
>> -       setbits_le32(&ccm->ahb_gate0, 1 << ahb_gate_offset);
>> +       setbits_le32(&ccm->ahb_gate0, priv->ahb_gate_mask);
>>   #ifdef CONFIG_SUNXI_GEN_SUN6I
>> -       setbits_le32(&ccm->ahb_reset0_cfg, 1 << ahb_gate_offset);
>> +       setbits_le32(&ccm->ahb_reset0_cfg, priv->ahb_gate_mask);
>>   #endif
>>
>> -       sunxi_usb_phy_init(index + 1);
>> -       sunxi_usb_phy_power_on(index + 1);
>> +       sunxi_usb_phy_init(priv->phy_index);
>> +       sunxi_usb_phy_power_on(priv->phy_index);
>>
>> -       if (index == 0)
>> -               *hccr = (void *)SUNXI_USB1_BASE;
>> -       else
>> -               *hccr = (void *)SUNXI_USB2_BASE;
>> +       hcor = (struct ehci_hcor *)((uint32_t)hccr +
>> +                                   HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>>
>> -       *hcor = (struct ehci_hcor *)((uint32_t) *hccr
>> -                               + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
>> -
>> -       debug("sunxi-ehci: init hccr %x and hcor %x hc_length %d\n",
>> -             (uint32_t)*hccr, (uint32_t)*hcor,
>> -             (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
>> -
>> -       return 0;
>> +       return ehci_register(dev, hccr, hcor, NULL, 0, plat->init_type);
>>   }
>>
>> -int ehci_hcd_stop(int index)
>> +static int ehci_usb_remove(struct udevice *dev)
>>   {
>>          struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>> -       int ahb_gate_offset;
>> +       struct ehci_sunxi_priv *priv = dev_get_priv(dev);
>> +       int ret;
>>
>> -       sunxi_usb_phy_power_off(index + 1);
>> -       sunxi_usb_phy_exit(index + 1);
>> +       ret = ehci_deregister(dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       sunxi_usb_phy_power_off(priv->phy_index);
>> +       sunxi_usb_phy_exit(priv->phy_index);
>>
>> -       ahb_gate_offset = index ? AHB_GATE_OFFSET_USB_EHCI1 :
>> -                                 AHB_GATE_OFFSET_USB_EHCI0;
>>   #ifdef CONFIG_SUNXI_GEN_SUN6I
>> -       clrbits_le32(&ccm->ahb_reset0_cfg, 1 << ahb_gate_offset);
>> +       clrbits_le32(&ccm->ahb_reset0_cfg, priv->ahb_gate_mask);
>>   #endif
>> -       clrbits_le32(&ccm->ahb_gate0, 1 << ahb_gate_offset);
>> +       clrbits_le32(&ccm->ahb_gate0, priv->ahb_gate_mask);
>>
>>          return 0;
>>   }
>> +
>> +static const struct udevice_id ehci_usb_ids[] = {
>> +       { .compatible = "allwinner,sun4i-a10-ehci", },
>> +       { .compatible = "allwinner,sun5i-a13-ehci", },
>> +       { .compatible = "allwinner,sun6i-a31-ehci", },
>> +       { .compatible = "allwinner,sun7i-a20-ehci", },
>> +       { .compatible = "allwinner,sun8i-a23-ehci", },
>> +       { .compatible = "allwinner,sun9i-a80-ehci", },
>> +       { }
>> +};
>> +
>> +U_BOOT_DRIVER(usb_ehci) = {
>> +       .name   = "ehci_sunxi",
>> +       .id     = UCLASS_USB,
>> +       .of_match = ehci_usb_ids,
>> +       .ofdata_to_platdata = ehci_usb_ofdata_to_platdata,
>> +       .probe = ehci_usb_probe,
>> +       .remove = ehci_usb_remove,
>> +       .ops    = &ehci_usb_ops,
>> +       .platdata_auto_alloc_size = sizeof(struct usb_platdata),
>> +       .priv_auto_alloc_size = sizeof(struct ehci_sunxi_priv),
>> +       .flags  = DM_FLAG_ALLOC_PRIV_DMA,
>> +};
>> --
>> 2.3.6
>>
>
> Regards,
> Simon
>

[U-Boot] [PATCH 8/8] sunxi: ehci: Convert to the driver-model

[Ian Campbell]

On Thu, 2015-04-30 at 16:35 +0200, Hans de Goede wrote:
> +	if (hccr == (void *)SUNXI_USB1_BASE) {
> +		priv->ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI0;
> +		priv->phy_index = 1;

Inferring these from the base address is a bit unfortunate, should we
not get told this by the DT, or from something higher up?

I have a feeling the answer will be "this can go away when X, Y & X have
happened", in which case perhaps a comment to that affect?

BTW I ignored patches 1..7 based on the diffstats, please let me know if
I should take a look for some reason.

Ian.

[U-Boot] [PATCH 8/8] sunxi: ehci: Convert to the driver-model

[Ian Campbell]

On Sat, 2015-05-02 at 15:03 +0100, Ian Campbell wrote:

I see I missed v2, oh well, this bit of code looks the same AFAICT.

> On Thu, 2015-04-30 at 16:35 +0200, Hans de Goede wrote:
> > +	if (hccr == (void *)SUNXI_USB1_BASE) {
> > +		priv->ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI0;
> > +		priv->phy_index = 1;
> 
> Inferring these from the base address is a bit unfortunate, should we
> not get told this by the DT, or from something higher up?
> 
> I have a feeling the answer will be "this can go away when X, Y & X have
> happened", in which case perhaps a comment to that affect?
> 
> BTW I ignored patches 1..7 based on the diffstats, please let me know if
> I should take a look for some reason.
> 
> Ian.

[U-Boot] [PATCH 8/8] sunxi: ehci: Convert to the driver-model

[Hans de Goede]

Hi,

On 05/02/2015 04:03 PM, Ian Campbell wrote:
> On Thu, 2015-04-30 at 16:35 +0200, Hans de Goede wrote:
>> +	if (hccr == (void *)SUNXI_USB1_BASE) {
>> +		priv->ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI0;
>> +		priv->phy_index = 1;
>
> Inferring these from the base address is a bit unfortunate, should we
> not get told this by the DT, or from something higher up?
>
> I have a feeling the answer will be "this can go away when X, Y & X have
> happened", in which case perhaps a comment to that affect?

Yes getting rid of this this requires us to move to the driver-model for
the phys reps. clocks. I'll add a comment to this extend.

Regards,

Hans

[U-Boot] [PATCH 0/8] usb: driver-model fixes and dm support sunxi-ehci.c

[Hans de Goede]

Hi,

On 30-04-15 16:35, Hans de Goede wrote:
> Hi Simon and Marek,
>
> This series completes my work on converting the sunxi usb/ehci code to
> the driver model. With this series everything works as it did before,
> and all the issues I've been seeing when switching to the driver model
> are resolved.
>
> Please review / merge. I think it would be best to merge this through
> the dm tree.

Note that the last patch (the actual conversion of the ehci-sunxi.c code)
depends on the 6 patch sunxi series starting with this patch:

http://patchwork.ozlabs.org/patch/465369/

So it is probably best if I take 8/8 upstream through the sunxi tree.

So assuming that the patch get a favorable review, the plan would be
to merge 1-7 through the dm tree and once that is done I'll take
care of the sunxi side if things.

Regards,

Hans


p.s.

Next stop: "Adding dm support to the ohci code and get it to work on sunxi"

[U-Boot] [PATCH 0/8] usb: driver-model fixes and dm support sunxi-ehci.c

[Simon Glass]

Hi Hans,

On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi Simon and Marek,
>
> This series completes my work on converting the sunxi usb/ehci code to
> the driver model. With this series everything works as it did before,
> and all the issues I've been seeing when switching to the driver model
> are resolved.
>
> Please review / merge. I think it would be best to merge this through
> the dm tree.

Great to see this, thanks for all the work and fixes.

I am not too keen on the passing through of struct usb_device, in fact
I went to some lengths to avoid that.

I'll read through the patches again, but I wonder if we can avoid
heading in that direction? Conceptually in driver model the device
does not exist until we find out what it is and can locate a device
driver.

Regards,
Simon

[U-Boot] [PATCH 0/8] usb: driver-model fixes and dm support sunxi-ehci.c

[Hans de Goede]

Hi,

On 30-04-15 16:48, Simon Glass wrote:
> Hi Hans,
>
> On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi Simon and Marek,
>>
>> This series completes my work on converting the sunxi usb/ehci code to
>> the driver model. With this series everything works as it did before,
>> and all the issues I've been seeing when switching to the driver model
>> are resolved.
>>
>> Please review / merge. I think it would be best to merge this through
>> the dm tree.
>
> Great to see this, thanks for all the work and fixes.
>
> I am not too keen on the passing through of struct usb_device, in fact
> I went to some lengths to avoid that.
>
> I'll read through the patches again, but I wonder if we can avoid
> heading in that direction? Conceptually in driver model the device
> does not exist until we find out what it is and can locate a device
> driver.

Right, the problem is that AFAICT the way the driver-model works is
that the driver needs to be known at device creation time, this
probably is a result of the auto alloc mem on behalf of the device
driver feature.

But for usb this is problematic as we need to read descriptors and
whats not before we can find the right driver, and then we do not
want to re-read those descriptors and the driver needs access to
them too.

What you've been doing sofar is in essence passing through
struct usb_device from usb_scan_device to the pre_child_probe
method, with the difference that you've been doing it field by
field. My patch which just adds the maxpacket size to the fields
you're passing (via platdata) also works, but requires re-reading
all the descriptors which is slow(ish) and may upset some devices,
this can be avoided by stuffing more of usb_device into the
plat_data passed from sb_scan_device to the pre_child_probe, but
I decided it would be easier to just pass all of the usb_device

Another slightly cleaner (IMHO) option then copying over the entire
usb_device would be to dynamically alloc usb_device in usb_scan_device,
and keep using the same usb_device all the time, so remove the on
stack copy, and do not auto-alloc the per child data for the uclass
as it is already allocated manually in usb_scan_device.

This effectively gives us the kernel model of allocating the
generic usb_device bits before looking for a driver.

Regards,

Hans

[U-Boot] [PATCH 0/8] usb: driver-model fixes and dm support sunxi-ehci.c

[Simon Glass]

Hi Hans,

On 30 April 2015 at 13:38, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 30-04-15 16:48, Simon Glass wrote:
>>
>> Hi Hans,
>>
>> On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi Simon and Marek,
>>>
>>> This series completes my work on converting the sunxi usb/ehci code to
>>> the driver model. With this series everything works as it did before,
>>> and all the issues I've been seeing when switching to the driver model
>>> are resolved.
>>>
>>> Please review / merge. I think it would be best to merge this through
>>> the dm tree.
>>
>>
>> Great to see this, thanks for all the work and fixes.
>>
>> I am not too keen on the passing through of struct usb_device, in fact
>> I went to some lengths to avoid that.
>>
>> I'll read through the patches again, but I wonder if we can avoid
>> heading in that direction? Conceptually in driver model the device
>> does not exist until we find out what it is and can locate a device
>> driver.
>
>
> Right, the problem is that AFAICT the way the driver-model works is
> that the driver needs to be known at device creation time, this
> probably is a result of the auto alloc mem on behalf of the device
> driver feature.

It's really a design decision.

>
> But for usb this is problematic as we need to read descriptors and
> whats not before we can find the right driver, and then we do not
> want to re-read those descriptors and the driver needs access to
> them too.
>
> What you've been doing sofar is in essence passing through
> struct usb_device from usb_scan_device to the pre_child_probe
> method, with the difference that you've been doing it field by
> field. My patch which just adds the maxpacket size to the fields
> you're passing (via platdata) also works, but requires re-reading
> all the descriptors which is slow(ish) and may upset some devices,
> this can be avoided by stuffing more of usb_device into the
> plat_data passed from sb_scan_device to the pre_child_probe, but
> I decided it would be easier to just pass all of the usb_device

It might take a few extra milliseconds (I have not timed it) but given
the >1000ms it seems to take to start up USB I don't think it matters.
It would be a strange device that refuses to let you read the
descriptor twice.

While it is of course easier to pass all of usb_device, I am not keen.
It is there not clear which bits are passed through and which are not.
If you like we could put the relevant bits in a struct.

>
> Another slightly cleaner (IMHO) option then copying over the entire
> usb_device would be to dynamically alloc usb_device in usb_scan_device,
> and keep using the same usb_device all the time, so remove the on
> stack copy, and do not auto-alloc the per child data for the uclass
> as it is already allocated manually in usb_scan_device.

But then we have a struct usb_device before a struct udevice. That
just cements what I think is wrong with the code.

>
> This effectively gives us the kernel model of allocating the
> generic usb_device bits before looking for a driver.

I'd be happier with that approach if we actually could split the
generic bits into some sort of 'struct urb' or similar. It represents
a destination for a request, and the request itself, not the device. I
think it is unfortunate that U-Boot conflates the device and the
request. About all we need to send a request to a device is its pipe
word and packet sizes. The device is a U-Boot concept - we can after
all fire requests all over the USB bus without a 'device'.

Regards,
Simon

[U-Boot] [PATCH 0/8] usb: driver-model fixes and dm support sunxi-ehci.c

[Hans de Goede]

Hi,

On 01-05-15 00:04, Simon Glass wrote:
> Hi Hans,
>
> On 30 April 2015 at 13:38, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 30-04-15 16:48, Simon Glass wrote:
>>>
>>> Hi Hans,
>>>
>>> On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi Simon and Marek,
>>>>
>>>> This series completes my work on converting the sunxi usb/ehci code to
>>>> the driver model. With this series everything works as it did before,
>>>> and all the issues I've been seeing when switching to the driver model
>>>> are resolved.
>>>>
>>>> Please review / merge. I think it would be best to merge this through
>>>> the dm tree.
>>>
>>>
>>> Great to see this, thanks for all the work and fixes.
>>>
>>> I am not too keen on the passing through of struct usb_device, in fact
>>> I went to some lengths to avoid that.
>>>
>>> I'll read through the patches again, but I wonder if we can avoid
>>> heading in that direction? Conceptually in driver model the device
>>> does not exist until we find out what it is and can locate a device
>>> driver.
>>
>>
>> Right, the problem is that AFAICT the way the driver-model works is
>> that the driver needs to be known at device creation time, this
>> probably is a result of the auto alloc mem on behalf of the device
>> driver feature.
>
> It's really a design decision.

TBH I'm not sure if it is the best decision (in hindsight) we may hit
similar issues in other cases. Anyways this is how things work now,
changing this is going to be a lot of work, so lets just roll with
it.

>
>>
>> But for usb this is problematic as we need to read descriptors and
>> whats not before we can find the right driver, and then we do not
>> want to re-read those descriptors and the driver needs access to
>> them too.
>>
>> What you've been doing sofar is in essence passing through
>> struct usb_device from usb_scan_device to the pre_child_probe
>> method, with the difference that you've been doing it field by
>> field. My patch which just adds the maxpacket size to the fields
>> you're passing (via platdata) also works, but requires re-reading
>> all the descriptors which is slow(ish) and may upset some devices,
>> this can be avoided by stuffing more of usb_device into the
>> plat_data passed from sb_scan_device to the pre_child_probe, but
>> I decided it would be easier to just pass all of the usb_device
>
> It might take a few extra milliseconds (I have not timed it) but given
> the >1000ms it seems to take to start up USB I don't think it matters.
> It would be a strange device that refuses to let you read the
> descriptor twice.

I've done a lot of USB work (including redirection of USB devices
into virtual machines for qemu) and I've seen stranger devices :)

But I must admit that I cannot actually name an example, it is just that
I've a feeling that this may become a problem.

>
> While it is of course easier to pass all of usb_device, I am not keen.
> It is there not clear which bits are passed through and which are not.
> If you like we could put the relevant bits in a struct.
>
>>
>> Another slightly cleaner (IMHO) option then copying over the entire
>> usb_device would be to dynamically alloc usb_device in usb_scan_device,
>> and keep using the same usb_device all the time, so remove the on
>> stack copy, and do not auto-alloc the per child data for the uclass
>> as it is already allocated manually in usb_scan_device.
>
> But then we have a struct usb_device before a struct udevice. That
> just cements what I think is wrong with the code.

We already have a struct usb_device before a struct udevice, just
because the struct usb_device is hiding on the stack rather then
sitting in the heap does not mean it is not there.

>
>>
>> This effectively gives us the kernel model of allocating the
>> generic usb_device bits before looking for a driver.
>
> I'd be happier with that approach if we actually could split the
> generic bits into some sort of 'struct urb' or similar. It represents
> a destination for a request, and the request itself, not the device. I
> think it is unfortunate that U-Boot conflates the device and the
> request. About all we need to send a request to a device is its pipe
> word and packet sizes. The device is a U-Boot concept - we can after
> all fire requests all over the USB bus without a 'device'.

We do not just need the usb_device to send usb requests before we
have a udevice (which a struct urb would fix) we also need it to
store the descriptors read before we've a udevice.

Also see my reply to your review of patch 1/8.

Regards,

Hans